updates to match latest lexicons#112
Conversation
Without this patch, the collection item weights feature introduced in lexicon v0.10.0-beta.7 lacks comprehensive documentation explaining how to use optional weights for proportional attribution in collections. This is a problem because developers need clear examples and explanation of the itemWeight field to effectively use the weighted collections feature for proportional impact attribution. This patch solves the problem by adding comprehensive JSDoc comments to HypercertCollectionItem and CollectionItemInput types with: - Explanation of itemIdentifier (required) and itemWeight (optional) - Three usage examples: basic item, weighted item, and nested collection - Helper type documentation for SDK operations Part 1/6 of lexicon sync v0.10.0-beta.4 → v0.10.0-beta.11 Co-authored-by: Claude Code <claude-code@anthropic.noreply.com>
Without this patch, activity descriptions in hypercerts could only contain plain text without any inline markup or annotations. This is a problem because the AT Protocol supports rich text facets for mentions, URLs, hashtags, and other inline markup, and the hypercerts lexicon v0.10.0-beta.7 added support for these facets in activity descriptions. This patch solves the problem by adding optional shortDescriptionFacets and descriptionFacets fields to CreateHypercertParams, updating the createHypercertRecord() method to include these fields in the record, and enhancing documentation with comprehensive examples showing proper byte indexing for facets. This enables developers to create richer, more interactive hypercert descriptions following AT Protocol facet standards. Changes: - Add AppBskyRichtextFacet import to repository interfaces - Add shortDescriptionFacets and descriptionFacets to CreateHypercertParams - Update createHypercertRecord() to pass facets through to record - Enhance HypercertClaim type documentation with facet usage examples - Add changeset documenting this enhancement - Mark Change 2 complete in lexicon sync plan (part 2 of 6) Co-authored-by: Claude Code <noreply@anthropic.com>
These were missed from the previous item weights commit.
Without this patch, the collection avatar and banner feature (added in beta.7) lacked comprehensive test coverage and documentation examples showing how to use these visual branding features. This is a problem because users need clear examples showing how to: - Create collections with avatar/banner images using Blobs or URIs - Update collection images (add, modify, remove, or preserve) - Use avatar/banner in both collections and projects This patch solves the problem by adding: - 7 new tests for avatar/banner CRUD operations (create, update, remove) - Tests for both Blob uploads and URI string references - Tests for preserving images during partial updates - Changeset documenting the new test coverage - Update spec marking Change 3 (Collection Avatar and Banner) complete Tests added: - Create collection with avatar/banner (Blob) - Create collection with avatar/banner (URI strings) - Create project with avatar/banner - Update collection avatar/banner (Blob) - Update collection avatar/banner (URI strings) - Remove avatar/banner when set to null - Preserve avatar/banner when not updating them All tests passing: 632 tests in sdk-core, 136 in sdk-react Spec: specs/lexicon-sync/v0.10.0-beta.4-v0.10.0-beta.11.md (Change 3) Co-authored-by: Claude Code <claude-code@noreply.anthropic.com>
…tion Without this patch, the lexicon sync skill and spec incorrectly emphasized documenting types rather than methods. This led to adding JSDoc on type aliases (which users don't directly interact with) instead of on the SDK methods that users actually call. This is a problem because: - Users call methods like createCollection(), not types like HypercertCollection - Method documentation is where users look for usage examples - Type documentation becomes redundant when methods are well-documented - Previous changes incorrectly marked type docs as complete when method docs weren't done This patch solves the problem by: - Updating sync-lexicons SKILL.md to emphasize method documentation over type docs - Clarifying that method JSDoc should include @example tags showing new features - Updating the spec to accurately reflect what documentation was actually completed - Marking Change 2 and 3 as partially complete (tests done, method docs still needed) - Adding notes explaining that Change 1 already has method docs with examples Changes to sync-lexicons skill: - Step 7 now focuses on documenting methods in HypercertOperationsImpl.ts - Added guidance to keep type documentation minimal - Updated example task list to mention method documentation explicitly - Added "IMPORTANT: Document methods, not types" callout Changes to spec: - Change 1: Removed type doc tasks, noted method docs already exist - Change 2: Marked method doc tasks as incomplete, added note - Change 3: Marked method doc tasks as incomplete, added note Spec: specs/lexicon-sync/v0.10.0-beta.4-v0.10.0-beta.11.md Co-authored-by: Claude Code <claude-code@noreply.anthropic.com>
🦋 Changeset detectedLatest commit: 060516a The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds rich-text facets for descriptions, collection item weights, and avatar/banner support for collections/projects; updates SDK interfaces and implementation, expands tests, and adds changeset docs and lexicon-sync planning. Changes
Sequence Diagram(s)(omitted — changes are API/SDK additions, docs, and tests; no multi-component control-flow requiring visualization) Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR updates the SDK to match lexicon changes from v0.10.0-beta.4 to v0.10.0-beta.11. The changes add support for new features in the hypercerts lexicon including rich text facets, collection item weights, and collection avatar/banner images. The PR includes comprehensive tests, type definitions, and changesets, though some documentation work is noted as pending in the sync plan.
Changes:
- Added rich text facet support (
shortDescriptionFacets,descriptionFacets) for hypercert activity descriptions - Added comprehensive tests for collection item weights (weighted, unweighted, and mixed collections)
- Added comprehensive tests for collection and project avatar/banner images (Blob and URI formats, creation, updating, removal)
- Enhanced type documentation with examples for
HypercertClaimandHypercertCollectionItem - Updated SKILL.md to emphasize documenting methods over types
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| specs/lexicon-sync/v0.10.0-beta.4-v0.10.0-beta.11.md | Sync plan document tracking implementation progress for lexicon updates |
| packages/sdk-core/tests/repository/HypercertOperationsImpl.test.ts | Comprehensive tests for rich text facets, weighted collection items, and avatar/banner images |
| packages/sdk-core/src/services/hypercerts/types.ts | Enhanced JSDoc documentation with examples for HypercertClaim and HypercertCollectionItem types |
| packages/sdk-core/src/repository/interfaces.ts | Added shortDescriptionFacets and descriptionFacets fields to CreateHypercertParams with documentation |
| packages/sdk-core/src/repository/HypercertOperationsImpl.ts | Implementation to pass facet fields from params to hypercert records |
| .claude/skills/sync-lexicons/SKILL.md | Updated sync process guidance to emphasize method documentation over type documentation |
| .changeset/add-rich-text-facets.md | Changeset documenting rich text facet support (minor bump) |
| .changeset/add-collection-item-weight-docs.md | Changeset documenting collection item weights (patch bump) |
| .changeset/add-collection-avatar-banner-docs.md | Changeset documenting avatar/banner support (minor bump) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| **What's Included:** | ||
|
|
||
| - Comprehensive JSDoc documentation for `HypercertCollection` type explaining avatar and banner usage |
There was a problem hiding this comment.
The changeset claims "Comprehensive JSDoc documentation for HypercertCollection type explaining avatar and banner usage" but the HypercertCollection type at line 152 has no JSDoc documentation - it's just a bare type alias. The avatar/banner documentation exists only in the CreateCollectionParams type comment (line 319). Consider either adding JSDoc documentation to the HypercertCollection type itself, or updating the changeset description to accurately reflect where the documentation was added (e.g., "Comprehensive JSDoc documentation in CreateCollectionParams explaining avatar and banner usage").
| - [ ] Add JSDoc documentation to `createCollection()` and `updateCollection()` methods about avatar | ||
| - [ ] Add usage examples in method documentation showing avatar in collections and projects | ||
| - [x] Add/update tests for collections with avatar and banner | ||
| - [x] Build and test | ||
| - [x] Create changeset (minor - new feature available) | ||
|
|
||
| **Note**: Method documentation mentions banner but not avatar. Tests are complete. | ||
|
|
||
| **Validation**: | ||
|
|
||
| - [x] Format check passes (`pnpm format:check`) | ||
| - [x] Lint passes (`pnpm lint`) | ||
| - [x] Typecheck passes (`pnpm typecheck`) | ||
| - [x] Build passes (`pnpm build`) | ||
| - [x] Tests pass (`pnpm test` - 632 tests in sdk-core, 136 in sdk-react) | ||
| - [x] Types export correctly | ||
|
|
||
| **Status**: ✅ Complete |
There was a problem hiding this comment.
The sync plan note at line 103 states "Method documentation mentions banner but not avatar. Tests are complete," and the SDK Tasks list shows two unchecked items related to JSDoc documentation (lines 97-98). However, Change 3 is marked as "✅ Complete" (line 114). The createCollection() method documentation at line 1269 in HypercertOperationsImpl.ts mentions "banner" but not "avatar", despite avatar being a supported parameter. Either the documentation should be updated to include avatar, or the status should be updated to reflect that documentation work remains pending.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@packages/sdk-core/src/repository/HypercertOperationsImpl.ts`:
- Around line 286-293: The current truthy checks in HypercertOperationsImpl that
set hypercertRecord.shortDescriptionFacets and hypercertRecord.descriptionFacets
drop empty arrays; change the guards to explicit undefined checks (e.g., if
(params.shortDescriptionFacets !== undefined) and if (params.descriptionFacets
!== undefined)) so empty arrays are preserved as intentional values when
assigning to hypercertRecord.shortDescriptionFacets and
hypercertRecord.descriptionFacets.
In `@specs/lexicon-sync/v0.10.0-beta.4-v0.10.0-beta.11.md`:
- Around line 162-192: The SDK Tasks under "Change 5: Collection Location
Property" were mistakenly copied from the facets change; replace the
facet-specific checklist items (references to AppBskyRichtextFacet,
shortDescriptionFacets, descriptionFacets) with location-related tasks: import
the appropriate Location/strongRef type, add a `location` field to
`CreateCollectionParams`, update the `create()` method in
`CollectionOperationsImpl` to accept and persist `location`, add docs to the
`Collection`/`CollectionClaim` type describing the `location` property and a
usage example for referencing location records, ensure lexicon validation and
unit tests cover the new `location` field, run build/typecheck/tests, and add a
minor changeset entry.
🧹 Nitpick comments (1)
packages/sdk-core/src/services/hypercerts/types.ts (1)
82-142: Excellent documentation forHypercertClaimtype.The comprehensive JSDoc with facet examples showing byte indexing for mentions, tags, and links will be very helpful for SDK users. The examples demonstrate proper
$typevalues and index structures.One minor note: the
@seelink on line 140 points to AT Protocol record key documentation, but this type is about facets. Consider updating to a facet-specific reference.Optional: Update `@see` link to facet documentation
- * `@see` {`@link` https://atproto.com/specs/record-key#record-key-type|AT Protocol Facets} + * `@see` {`@link` https://atproto.com/specs/lexicon#facet|AT Protocol Facets}
| if (params.shortDescriptionFacets) { | ||
| hypercertRecord.shortDescriptionFacets = params.shortDescriptionFacets; | ||
| } | ||
|
|
||
| if (params.descriptionFacets) { | ||
| hypercertRecord.descriptionFacets = params.descriptionFacets; | ||
| } | ||
|
|
There was a problem hiding this comment.
Preserve empty facet arrays.
Line 286 and Line 290 use truthy checks, which drop empty arrays; that loses an explicit “no facets” intent. Prefer an explicit !== undefined check.
🐛 Proposed fix
- if (params.shortDescriptionFacets) {
+ if (params.shortDescriptionFacets !== undefined) {
hypercertRecord.shortDescriptionFacets = params.shortDescriptionFacets;
}
- if (params.descriptionFacets) {
+ if (params.descriptionFacets !== undefined) {
hypercertRecord.descriptionFacets = params.descriptionFacets;
}🤖 Prompt for AI Agents
In `@packages/sdk-core/src/repository/HypercertOperationsImpl.ts` around lines 286
- 293, The current truthy checks in HypercertOperationsImpl that set
hypercertRecord.shortDescriptionFacets and hypercertRecord.descriptionFacets
drop empty arrays; change the guards to explicit undefined checks (e.g., if
(params.shortDescriptionFacets !== undefined) and if (params.descriptionFacets
!== undefined)) so empty arrays are preserved as intentional values when
assigning to hypercertRecord.shortDescriptionFacets and
hypercertRecord.descriptionFacets.
|
@copilot open a new pull request to apply changes based on the comments in this thread |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1,12 @@ | |||
| --- | |||
| "@hypercerts-org/sdk-core": patch | |||
There was a problem hiding this comment.
This changeset is marked as "patch" but describes adding documentation and tests for a new feature (collection item weights). According to semantic versioning, a "patch" bump should be for bug fixes or trivial documentation-only changes. Since this changeset includes:
- Comprehensive test coverage for the new feature (tests at lines 990-1072)
- Documentation for new functionality
- Examples showing how to use the feature
It should be categorized as "minor" instead of "patch", as it's documenting a new feature that's now available to users. If this were truly documentation-only without any functionality change, "patch" might be appropriate, but the tests indicate this is exposing new functionality.
| "@hypercerts-org/sdk-core": patch | |
| "@hypercerts-org/sdk-core": minor |
| - [x] Verify CRUD operations support itemWeight (createCollection/updateCollection already pass items through) | ||
| - [x] Add/update tests for collections with weighted items | ||
| - [x] Build and test | ||
| - [x] Create changeset (minor - new feature available) |
There was a problem hiding this comment.
The sync plan indicates this change should have a changeset with "minor - new feature available" (line 33), but the actual changeset created (.changeset/add-collection-item-weight-docs.md) is marked as "patch". This inconsistency should be resolved - either update the plan to reflect that it's a patch (documentation-only), or update the changeset to be "minor" if it's exposing new functionality to users.
… changes The tests "should update collection adding weights to existing items" and "should update collection removing weights from items" were only checking that the update payload was correct, but weren't actually verifying that the weight properties were added or removed from the items. This patch fixes both tests to explicitly verify: - Adding weights: checks that itemWeight properties are present with correct values - Removing weights: checks that itemWeight properties are absent Without this fix, the tests could pass even if the implementation didn't actually modify the weight properties, providing false confidence in the functionality. Co-authored-by: Claude Code <noreply@anthropic.com>
Change truthy checks to explicit !== undefined checks for shortDescriptionFacets and descriptionFacets to ensure empty arrays are preserved as valid values. Fixes unresolved comment from PR #112 review thread #3700691486
Change truthy checks to explicit !== undefined checks for shortDescriptionFacets and descriptionFacets to ensure empty arrays are preserved as valid values. Fixes unresolved comment from PR #112 review thread #3700691486
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.