[ENG-1849] Persist Roam node type schema labels#1131
Conversation
The cross-app node contract (ENG-1847) carries nodeType = { sourceLocalId, label } and the instance Concept references it via schema_represented_by_local_id, but the fixtures persisted no is_schema:true schema Concept for that id. Add the required schemaConcept (Roam Claim, Obsidian Evidence) carrying stable source identity + label only -- no source_data/format/color/tag, per the contract's "without redefining schema shape".
Roam's existing 5-min sync already persists the schema Concept per contract (convertDgToSupabaseConcepts -> discourseNodeSchemaToLocalConcept; all types on initial sync, edited types incrementally via nodeTypeSince), so no Roam sync code change is needed for F4/F9/F13. Stacked on eng-1847 (PR #1129, unmerged).
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
📝 WalkthroughWalkthrough
ChangesCrossAppNodeFixture schemaConcept extension
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
…de-type-schema-metadata-for-shared-nodes # Conflicts: # packages/database/package.json # packages/database/src/crossAppNodeContract.ts # packages/database/src/lib/rid.ts
PR size/scope checkThis PR is over our review-size guideline.
Please split this into smaller PRs unless there is a clear reason the changes need to land together. If keeping it as one PR, please add a brief justification covering:
|
sid597
changed the title
sid597
changed the base branch from
eng-1847-define-shared-cross-app-node-content-contract
to
main
| node: DiscourseNode, | ||
| ): LocalConceptDataInput => { | ||
| const titleParts = node.text.split("/"); | ||
| const label = titleParts[titleParts.length - 1] ?? node.text; |
There was a problem hiding this comment.
Before a template-less node types could have name, but not the explicit literal_content.label field.
3 participants
Summary
Re-scopes ENG-1849 against the final merged ENG-1847 contract baseline.
The old fixture-based patch was built on a draft ENG-1847 artifact that no longer exists on
main. Final ENG-1847 intentionally keeps the public surface to@repo/database/crossAppNodeContractand@repo/database/lib/rid, with typed examples only and no exported DB fixture API.This PR now updates the actual Roam producer path instead:
discourseNodeSchemaToLocalConceptalways persistsliteral_content.labelfor node-type schema Concepts, including node types with no template. When a template exists, the existingliteral_content.templatebehavior is preserved.Why this is the ENG-1849 fix
Roam already syncs node-type schema Concepts through the existing sync loop:
source_local_id = node.typeis_schema: trueschema_represented_by_local_idThe remaining gap was that
literal_content.labelwas only written whennode.template !== undefined. Template-less node types, especially via the new settings store where empty templates becomeundefined, could sync without an explicit label field. ENG-1849 requires stable source identity plus label/name metadata for downstream mapping, so the label now becomes unconditional.Out of scope
packages/database/src/fixtures/crossAppNodes.tsValidation
pnpm --filter @repo/database check-typespnpm --filter roam exec eslint src/utils/conceptConversion.tsgit diff --check origin/main...HEADFull
pnpm --filter roam check-typesis currently blocked by unrelated merged-main Roam API typing errors insrc/utils/discourseNodeSearchProviders.ts(semanticSearchEnabled/semanticSearch), not by this diff.