NON-BREAKING: add optional longDescription and visibility fields to organization - #174
Conversation
🦋 Changeset detectedLatest commit: c456cda 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:
📝 WalkthroughWalkthroughThis PR extends the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 7✅ Passed checks (7 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/validate-actor-organization.test.ts (1)
38-74: Add explicit rejection tests for the two new fields.Great baseline coverage, but this PR adds constraints that should be asserted directly: reject invalid
visibilityvalues and reject invalidpagetypes.✅ Suggested test additions
- it("should accept record with all optional fields populated", () => { + it("should accept record with several optional fields populated", () => { const result = Organization.validateMain({ $type: ids.AppCertifiedActorOrganization, organizationType: ["nonprofit", "ngo"], urls: [{ url: "https://example.org", label: "Website" }], foundedDate: "2010-01-01T00:00:00.000Z", visibility: "public", createdAt: "2024-01-01T00:00:00.000Z", }); expect(result.success).toBe(true); }); + + it("should reject record with invalid visibility value", () => { + const result = validate( + { + createdAt: "2024-01-01T00:00:00.000Z", + visibility: "private", + }, + ids.AppCertifiedActorOrganization, + "main", + false, + ); + expect(result.success).toBe(false); + }); + + it("should reject record with invalid page type", () => { + const result = validate( + { + createdAt: "2024-01-01T00:00:00.000Z", + page: "not-a-record", + }, + ids.AppCertifiedActorOrganization, + "main", + false, + ); + expect(result.success).toBe(false); + });As per coding guidelines: "For negative tests in lexicon validation tests, use
validate()fromgenerated/lexicons.js."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/validate-actor-organization.test.ts` around lines 38 - 74, Add two negative tests using validate() to assert the new constraints: one that calls validate(..., ids.AppCertifiedActorOrganization, "main", false) with an invalid visibility (e.g., visibility: "privateish") and expects result.success to be false and result.error to be defined, and another that calls validate(..., ids.AppCertifiedActorOrganization, "main", false) with an invalid page type (e.g., page: 123 or page: {type: "unknown"}) and expects result.success to be false; place these alongside the existing tests and use validate() (not Organization.validateMain) per the lexicon testing guideline.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/validate-actor-organization.test.ts`:
- Around line 38-74: Add two negative tests using validate() to assert the new
constraints: one that calls validate(..., ids.AppCertifiedActorOrganization,
"main", false) with an invalid visibility (e.g., visibility: "privateish") and
expects result.success to be false and result.error to be defined, and another
that calls validate(..., ids.AppCertifiedActorOrganization, "main", false) with
an invalid page type (e.g., page: 123 or page: {type: "unknown"}) and expects
result.success to be false; place these alongside the existing tests and use
validate() (not Organization.validateMain) per the lexicon testing guideline.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 23b5336f-181f-4baf-b293-4aabf413eb80
📒 Files selected for processing (6)
.changeset/add-organization-page-and-visibility.mdERD.pumlREADME.mdSCHEMAS.mdlexicons/app/certified/actor/organization.jsontests/validate-actor-organization.test.ts
holkexyz
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Line 150: Update the phrasing in the table cell that currently reads "future
proof methods" to the hyphenated compound adjective "future-proof methods" so
the phrase modifying "methods" is grammatically correct (refer to the text
inside the table row containing "**EVM Link**" and "`app.certified.link.evm`").
- Line 146: Update the README entry for app.certified.actor.organization to
include the missing non-facet property createdAt and simplify the wording by
replacing "discoverability visibility" with "visibility"; specifically, in the
table row for `app.certified.actor.organization` add "createdAt" to the list of
documented properties (with its brief meaning, e.g., creation timestamp) and
change the phrase "discoverability visibility" to just "visibility" so the
description reads: Organization metadata: legal structure, URLs, location,
founding date, optional long description, createdAt, and visibility.
🪄 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: 0e3ca883-032d-49fa-8407-0784c04f9182
📒 Files selected for processing (7)
.changeset/add-organization-page-and-visibility.mdERD.pumlREADME.mdSCHEMAS.mdlexicons/app/certified/actor/organization.jsonlexicons/org/hypercerts/defs.jsontests/validate-actor-organization.test.ts
✅ Files skipped from review due to trivial changes (3)
- .changeset/add-organization-page-and-visibility.md
- lexicons/org/hypercerts/defs.json
- tests/validate-actor-organization.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- ERD.puml
- lexicons/app/certified/actor/organization.json
- SCHEMAS.md
|
@holkexyz |
Yes, the idea is just that we don't want to force leaflet. You can still use leaflet via the strongRef. Does that work for you? From the other PR: "Now a union of descriptionString (inline plain text or markdown with optional rich-text facets) and strongRef (for any rich-text document, including Leaflet)." PS.: The nice thing with the bsky facets is that if it is used but not rendered by a frontend, it is still shown in a reasonable way (just the string). So, the facet is just an add-on. See https://www.pfrazee.com/blog/why-facets EDIT: Adam raised a similar question in #181. I guess maybe we should add it directly and not just use strongRef. WDYT @satyam-mishra-pce ? |
Thanks for clarifying, Holke. I am open to both the ideas of having a |
|
I think an open union which allows both inline linearDocuments and strongRefs is OK. A As always being less opinionated means more flexibility, but the downside is that it makes interoperability between apps harder. A good SDK can help mitigate the latter to a large degree. |
b7b1d89 to
22884e7
Compare
I agree. Added the leaflet lexicon support inline. Can this be merged now? |
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 `@scripts/check-lexicon-style.js`:
- Around line 611-621: The code unconditionally returns when ref.startsWith any
knownExternalPrefixes (knownExternalPrefixes), which skips union ref validation
even if the target lexicon is vendored and present locally; change the
early-return logic to only skip when the referenced namespace is not present in
the local index. Specifically, replace the plain if
(knownExternalPrefixes.some((prefix) => ref.startsWith(prefix))) return; with a
check that first determines whether the ref corresponds to a locally indexed
lexicon (e.g., consult the existing indexedLexicons map or isLocalLexicon
function), and only return/skip when the namespace is both in
knownExternalPrefixes and NOT found in the local index; otherwise continue with
the unresolved/type checks for that ref so typos and invalid union variants are
caught.
🪄 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: 35f9581b-620f-468a-9aef-0e4ec4a08c50
📒 Files selected for processing (8)
.changeset/add-organization-page-and-visibility.mdERD.pumlREADME.mdSCHEMAS.mdlexicons/app/certified/actor/organization.jsonlexicons/org/hypercerts/defs.jsonscripts/check-lexicon-style.jstests/validate-actor-organization.test.ts
✅ Files skipped from review due to trivial changes (4)
- ERD.puml
- lexicons/org/hypercerts/defs.json
- lexicons/app/certified/actor/organization.json
- tests/validate-actor-organization.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- .changeset/add-organization-page-and-visibility.md
aspiers
changed the title
…organization - page: optional ref to pub.leaflet.pages.linearDocument#main for rich long-form org content (mission, history, etc.) - visibility: optional string with knownValues [public, unlisted] to control discoverability on platforms that honor the setting Closes #169
- Renamed `page` to `longDescription` on organization for clarity - Changed from bare Leaflet ref to union of `descriptionString` and `strongRef`, matching the pattern used across activity, collection, and attachment (CRITICAL-01) - Added `descriptionString` shared def to `org.hypercerts.defs` - Updated ERD, README, SCHEMAS.md, changeset, and tests Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
22884e7 to
8173936
Compare
Insert pub.leaflet.pages.linearDocument#main as the second variant in the longDescription union, between descriptionString and strongRef, so clients can embed a Leaflet document directly rather than only via a strongRef indirection. Also fix the style checker to skip ref-resolution for known-external namespaces (pub.leaflet.*, app.bsky.*, com.atproto.*) in union types, consistent with how file-level checks already exempt those namespaces.
8173936 to
c456cda
Compare
Two follow-ups to PR #174: 1. Drop the redundant `#main` suffix from the `longDescription` union's Leaflet ref in `app.certified.actor.organization`, so it matches the bare `pub.leaflet.pages.linearDocument` form already used on activity, collection, and attachment after the cleanup in 06fce2c. PR #174 was rebased onto that cleanup but re-introduced `#main` for the new organization field. Purely a ref-string normalization — the target def is identical. 2. Update the unreleased changeset description to describe the actual 3-way union (inline string / embedded Leaflet linear document / strongRef), not a 2-way union, so the v0.12.0 CHANGELOG entry is accurate. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
4 participants
Summary
Implements #169, incorporating feedback from issue comments.
Adds two optional fields to the
app.certified.actor.organizationlexicon:longDescription— a union ofdescriptionString(inline plain text or markdown with optional rich-text facets) andstrongRef(for any rich-text document, including Leaflet). This follows the description union pattern established acrossactivity,collection, andattachment. Mirrors thelongDescriptionfield from the legacyapp.gainforest.organization.infolexicon that GainForest is migrating away from.visibility— astringwithknownValues: ["public", "unlisted"]to let organizations control whether they are publicly discoverable on platforms that honor this setting.Also adds the shared
descriptionStringdef toorg.hypercerts.defs— the inline variant used by the description union pattern across all schemas.Changes
lexicons/app/certified/actor/organization.json— addlongDescription(union) andvisibilityfieldslexicons/org/hypercerts/defs.json— adddescriptionStringshared defERD.puml— addlongDescription?andvisibility?to the organization entityREADME.md— update Organization row description in the Certified tableSCHEMAS.md— regenerated (includes new fields in the property table)tests/validate-actor-organization.test.ts— new test file (7 tests, including inline longDescription).changeset/add-organization-page-and-visibility.md— minor bump changesetBackwards Compatibility
Notes on design decisions
longDescriptionuses the description union pattern (descriptionString | strongRef) rather than a bare Leaflet ref, for consistency with CRITICAL-01 changes across the protocol. The field is namedlongDescription(notdescription) because the baseactor.profilerecord already owns a shortdescriptionfield, and the organization record extends it.visibilityusesknownValues(notenum) per ATProto style convention, as requested by @holkexyzvisibilityvalues are lowercase (public,unlisted) consistent with all otherknownValuesfields in this repovisibilitydescription is generalized and does not reference any specific platform, as requested by @holkexyzTest plan
npm run checkpasses (pre-existing viem import errors in EVM link tests are unrelated)🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Documentation