fix: widen description fields from Leaflet ref to union type - #181
Conversation
🦋 Changeset detectedLatest commit: 06fce2c The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughExpands the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
aspiers
left a comment
There was a problem hiding this comment.
Breaking change, but I'm fine with this in principle if everyone else is and we're confident there aren't any apps yet relying on the presence this field which would be too uncomfortable to break. As with the other breaking changes being proposed now, either we swallow the pain now or go with new NSID prefix which feels like it would create even more pain.
I would like answers to my two questions before merging though.
02802ff to
490013f
Compare
|
Unfortunately we are locked in with this change. There are already existing apps that have data stored in this format, and changing it to a union would invalidate all previous certs. Summary:
|
|
Pull request reopened, we got one argument wrong. "We decided not to adopt this change: switching description to a union would change the stored JSON shape (adds $type), which would break all existing records and apps." The current implementation uses leaflet, which already has $type. The proposal to widen the description fields is therefore safe and non-breaking. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.changeset/widen-description-to-union.md:
- Around line 1-5: The changeset `.changeset/widen-description-to-union.md` is a
duplicate of `.changeset/description-union-type.md`; delete one of them (either
remove `.changeset/widen-description-to-union.md` or
`.changeset/description-union-type.md`) so only a single changeset describing
the widening of `description` fields to the union type remains, ensuring the
package bump for "@hypercerts-org/lexicon" appears only once in the changelog.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7d9c2df9-3f98-4005-b81b-3127a1b4fa19
📒 Files selected for processing (8)
.changeset/description-union-type.md.changeset/widen-description-to-union.mdERD.pumlSCHEMAS.mdlexicons/org/hypercerts/claim/activity.jsonlexicons/org/hypercerts/collection.jsonlexicons/org/hypercerts/context/attachment.jsonlexicons/org/hypercerts/defs.json
7001037 to
4cbf8b0
Compare
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ion union Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
434ac8f to
719cdc6
Compare
Allows descriptions to be an inline string, a Leaflet linear document, or a strong reference to an external description record. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lexicons/org/hypercerts/claim/activity.json (1)
33-39: Clarify the inlinedescriptionvariant as an object, not a bare string.
org.hypercerts.defs#descriptionStringis an object ({ value, facets? }), so “inline string” is misleading and can lead to invalid client payloads. Please reword this to describe the inline object shape, then mirror the same wording in the matchingdescriptionfields incollection,attachment, and regeneratedSCHEMAS.md.Proposed wording tweak
- "description": "Long-form description of the activity. An inline string for plain text or markdown, a Leaflet linear document for rich-text content, or a strong reference to an external description record." + "description": "Long-form description of the activity. Either an inline description object (`value` with optional `facets`) for plain text or markdown, a Leaflet linear document for rich-text content, or a strong reference to an external description record."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lexicons/org/hypercerts/claim/activity.json` around lines 33 - 39, The current "description" union text incorrectly calls org.hypercerts.defs#descriptionString an "inline string"; update the prose to state that the inline variant is an object matching org.hypercerts.defs#descriptionString (e.g., { value, facets? }) rather than a bare string, and apply the same wording change to the "description" fields in collection and attachment schemas; after updating the JSON schema descriptions, regenerate SCHEMAS.md so the docs reflect the corrected object shape.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lexicons/org/hypercerts/claim/activity.json`:
- Around line 33-39: The current "description" union text incorrectly calls
org.hypercerts.defs#descriptionString an "inline string"; update the prose to
state that the inline variant is an object matching
org.hypercerts.defs#descriptionString (e.g., { value, facets? }) rather than a
bare string, and apply the same wording change to the "description" fields in
collection and attachment schemas; after updating the JSON schema descriptions,
regenerate SCHEMAS.md so the docs reflect the corrected object shape.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d10699cb-bc24-4419-8204-729139e8fd72
📒 Files selected for processing (4)
SCHEMAS.mdlexicons/org/hypercerts/claim/activity.jsonlexicons/org/hypercerts/collection.jsonlexicons/org/hypercerts/context/attachment.json
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Clarified the impact of the schema change on existing records.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
PR #181 was merged but missing from the release notes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
PR #181 was merged but missing from the release notes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary
descriptionfield onactivity,collection, andattachmentwas a bare ref topub.leaflet.pages.linearDocument#main— a hard dependency on a single rich-text format that could never be widened without a breaking change.descriptionString(inline plain text or markdown with optional rich-text facets) andstrongRef(for any rich-text document, including Leaflet).descriptionStringdef toorg.hypercerts.defsto keep the inline variant DRY across all three schemas.Why
Leaflet is one possible rich-text backend, not the only one. By making this a union, clients can use simple inline markdown for lightweight descriptions or reference any external document format. This matches AT Protocol patterns where unions allow protocol evolution without breaking changes.
Test plan
npm run checkpasses🤖 Generated with Claude Code
Summary by CodeRabbit