feat: add pub.leaflet lexicons - #165
Conversation
🦋 Changeset detectedLatest commit: 238f8a1 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 |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 3 minutes and 26 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds vendored external lexicons ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
27e6e7c to
ffd3bde
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lexicons/pub/leaflet/blocks/orderedList.json`:
- Around line 9-12: The startIndex property in orderedList.json currently allows
non-positive integers; update the "startIndex" schema (the property named
"startIndex") to enforce positivity by adding a lower bound (e.g., add
"minimum": 1) so only integers >=1 are valid while keeping the existing "type":
"integer" and description.
In `@lexicons/pub/leaflet/pages/linearDocument.json`:
- Around line 86-94: The schema currently allows negative integers for path
indices and text offsets; update the "block" array item schema and the "offset"
property in linearDocument.json to enforce non-negative values by adding a
minimum: 0 constraint so both the "block" items (the path indices) and the
"offset" (text offset) cannot be negative.
In `@lexicons/pub/leaflet/richtext/facet.json`:
- Around line 54-56: Add the proper format constraints to the URI fields in the
facet schema: update the link.uri property to include "format": "uri" so it
validates as an RFC-3986 URL, and change the atMention.atURI property's format
from "uri" to "at-uri" so it enforces the AT URI pattern
(at://AUTHORITY[/COLLECTION[/RKEY]]); locate and edit the properties named
"link.uri" and "atMention.atURI" in the lexicon to apply these format updates.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8c9d85c2-f279-4d2b-96f9-2b37fd2f8969
📒 Files selected for processing (20)
.changeset/add-pub-leaflet-lexicons.mdSCHEMAS.mdlexicons/app/bsky/richtext/facet.jsonlexicons/pub/leaflet/blocks/blockquote.jsonlexicons/pub/leaflet/blocks/bskyPost.jsonlexicons/pub/leaflet/blocks/button.jsonlexicons/pub/leaflet/blocks/code.jsonlexicons/pub/leaflet/blocks/header.jsonlexicons/pub/leaflet/blocks/horizontalRule.jsonlexicons/pub/leaflet/blocks/iframe.jsonlexicons/pub/leaflet/blocks/image.jsonlexicons/pub/leaflet/blocks/math.jsonlexicons/pub/leaflet/blocks/orderedList.jsonlexicons/pub/leaflet/blocks/page.jsonlexicons/pub/leaflet/blocks/poll.jsonlexicons/pub/leaflet/blocks/text.jsonlexicons/pub/leaflet/blocks/unorderedList.jsonlexicons/pub/leaflet/blocks/website.jsonlexicons/pub/leaflet/pages/linearDocument.jsonlexicons/pub/leaflet/richtext/facet.json
55cd4b8 to
ffd3bde
Compare
aspiers
left a comment
There was a problem hiding this comment.
As discussed a while back, I'm not yet convinced that including copies of third party lexicons is needed or the best way. We were already importing these and I thought it was working fine, e.g. see https://github.com/hypercerts-org/hypercerts-lexicon/blob/main/scripts/create-shims.sh. This seems to be a large part of the reason https://github.com/mary-ext/atcute exists.
That said, I could be wrong! Worth doing some research on how other projects handle this.
|
(Also, the checks are failing.) |
|
should we close this PR for now? |
|
I'm worried about third-party lexicons, we don't control the leaflet domain and can't publish and validate it. I prefer hosting it under our own hypercerts domain |
|
Do we need this? @s-adamantine @aspiers |
|
I investigated whether TL;DR: it can't. The What
The actual bug: Our Why the shim approach was always type-only: So this PR's approach (vendoring the JSONs into
|
|
Regarding @daviddao's concern about depending on third-party lexicons we don't control: Firstly, worth noting this concern is more relevant for third-party lexicons like Leaflet. The This is a perfectly reasonable concern so thank you for raising. I think the risk is very low and well-mitigated. Here's why (and BTW this is not pure AI slop, I thought about this carefully then asked the AI to sanity check it and help with wording!): Breaking changes are essentially ruled out by the ATProto spec. As we are already painfully aware from our own experience, the Lexicon Evolution rules mean breaking changes require a new NSID. If Leaflet violated this, it would break every app in the ATmosphere that uses their lexicons, not just us. Unlike Hypercerts, the Leaflet lexicons are already mature, widely recognised, and have been in broad use for quite a long time already. We also know the Leaflet team personally since Vancouver and could ask them directly for extra assurances on this. Apps compile schemas in at build time, not at runtime. It would be very unusual for an app to dynamically resolve lexicons from ATProto at runtime on every use. Schemas are baked in via codegen ( Standard dependency management handles the rest. We pin Re-hosting under our own domain would actually break interoperability. The {
"blocks": [{
"block": {
"$type": "pub.leaflet.blocks.text",
"plaintext": "Hello",
"facets": [...]
}
}]
}If we forked these to If something did go wrong, we'd still have an upgrade path at least for our top-level This is also an argument against the earlier idea (PR #181, now closed) of widening That said, we could probably use https://panproto.dev and the lexicon lensing idea to solve even the stickiest of migration problems. In summary:
Finally, from a more philosophical perspective, we are placing a bet on ATProto, which I see as more or less equivalent to placing a bet on the ATmosphere community and the vision of interoperability. If everyone in the community published their own lexicons in silos without reusing lexicons from others, then personally I would be much less optimistic about ATProto's chances of success. If the lensing idea gains traction in the future then this could change for the better, but we're not there yet. |
- Update vendored leaflet lexicons to match upstream (add footnote
facet, checklist support in list blocks)
- Remove create-shims.sh and @atcute/{leaflet,bluesky} dependencies;
vendored JSONs now provide both types and runtime validation
- Fix duplicate export names in generate-exports.js for external
lexicon namespaces (BSKY_RICHTEXT_FACET vs LEAFLET_RICHTEXT_FACET)
- Add validate-external-lexicons.test.ts covering description and
facet validation for activity, collection, attachment, measurement
- Update AGENTS.md and docs/AUTO_GENERATION.md to remove shim refs
95a3003 to
8657f1d
Compare
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 `@tests/validate-external-lexicons.test.ts`:
- Around line 11-23: The fixture leafletDescription currently has an empty
facets array which skips exercising the pub.leaflet.richtext.facet lexicon; add
a minimal facet object into leafletDescription.blocks[0].block.facets such that
each entry is a ref with $type "pub.leaflet.richtext.facet" (and the minimal
required properties per the lexicon, e.g., an id/name or value field) so the
tests that reuse leafletDescription in positive cases will exercise
pub.leaflet.richtext.facet registration at runtime; update any positive test
usages to reuse this shared facet fixture.
🪄 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: 21382cb3-a835-4d6f-ab67-8a24a9c35edd
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (26)
.changeset/add-pub-leaflet-lexicons.mdAGENTS.mdSCHEMAS.mddocs/AUTO_GENERATION.mdlexicons/app/bsky/richtext/facet.jsonlexicons/pub/leaflet/blocks/blockquote.jsonlexicons/pub/leaflet/blocks/bskyPost.jsonlexicons/pub/leaflet/blocks/button.jsonlexicons/pub/leaflet/blocks/code.jsonlexicons/pub/leaflet/blocks/header.jsonlexicons/pub/leaflet/blocks/horizontalRule.jsonlexicons/pub/leaflet/blocks/iframe.jsonlexicons/pub/leaflet/blocks/image.jsonlexicons/pub/leaflet/blocks/math.jsonlexicons/pub/leaflet/blocks/orderedList.jsonlexicons/pub/leaflet/blocks/page.jsonlexicons/pub/leaflet/blocks/poll.jsonlexicons/pub/leaflet/blocks/text.jsonlexicons/pub/leaflet/blocks/unorderedList.jsonlexicons/pub/leaflet/blocks/website.jsonlexicons/pub/leaflet/pages/linearDocument.jsonlexicons/pub/leaflet/richtext/facet.jsonpackage.jsonscripts/create-shims.shscripts/generate-exports.jstests/validate-external-lexicons.test.ts
💤 Files with no reviewable changes (1)
- scripts/create-shims.sh
✅ Files skipped from review due to trivial changes (16)
- lexicons/pub/leaflet/blocks/page.json
- docs/AUTO_GENERATION.md
- lexicons/pub/leaflet/blocks/math.json
- lexicons/pub/leaflet/blocks/horizontalRule.json
- lexicons/pub/leaflet/blocks/button.json
- AGENTS.md
- lexicons/pub/leaflet/blocks/website.json
- lexicons/pub/leaflet/blocks/image.json
- lexicons/pub/leaflet/blocks/code.json
- lexicons/pub/leaflet/blocks/poll.json
- lexicons/pub/leaflet/blocks/bskyPost.json
- lexicons/pub/leaflet/blocks/iframe.json
- lexicons/pub/leaflet/blocks/text.json
- lexicons/app/bsky/richtext/facet.json
- lexicons/pub/leaflet/blocks/orderedList.json
- lexicons/pub/leaflet/pages/linearDocument.json
🚧 Files skipped from review as they are similar to previous changes (6)
- .changeset/add-pub-leaflet-lexicons.md
- lexicons/pub/leaflet/blocks/header.json
- lexicons/pub/leaflet/blocks/blockquote.json
- lexicons/pub/leaflet/blocks/unorderedList.json
- lexicons/pub/leaflet/richtext/facet.json
- SCHEMAS.md
Fixes gen-schemas-md output to use backticks around MIME types in accept fields, improving readability and avoiding Prettier escaping `image/*` to `image/\*`.
84b1f95 to
137a044
Compare
Add a bold facet to the leafletDescription fixture so the test exercises the pub.leaflet.richtext.facet lexicon at runtime, not just an empty array.
When a lexicon has properties: {} (e.g. horizontalRule), the gen
script emitted a trailing blank line after the Properties heading
plus another from the section separator, producing a double blank
line that Prettier collapsed but CI's raw gen output preserved.
Also wraps MIME types in accept fields with backticks.
7b7fce1 to
238f8a1
Compare
Comprehensive analysis: why
|
| `ref` (current) | open `union` (proposed) | |
|---|---|---|
| Existing 27 Leaflet records | ✅ Pass | ✅ Pass (same data, $type already present) |
| Add new format in 2027 | ❌ Breaking type-change | ✅ Add ref, non-breaking |
| Leaflet ships breaking change | ❌ No control | ✅ Remove ref, non-breaking |
| Gradual format migration | ❌ All-or-nothing | ✅ Coexist and deprecate |
| Schema evolution over years | ❌ Every change is a crisis | ✅ Routine additions/removals |
| Third-party resilience | Social trust only | Structural + social trust |
The full interactive analysis with code paths, real record data, and spec citations is at https://daviddao.github.io/lexicon-demo/
|
I also want to flag that the initial merge by @aspiers, which overrode |
s-adamantine
left a comment
There was a problem hiding this comment.
matches the published lexicons, lgtm
Summary
pub.leafletlexicons (blocks, pages, richtext) vendored from the Leaflet protocolpub.leaflet.richtext.facetwhich was missing but referenced byblockquote,header, andtextblocksapp.bsky.richtext.facetshim for ATProto richtext compatibilitySCHEMAS.mdto include the new lexiconsSummary by CodeRabbit