ENG-2158 Publish node type format with schemas - #1316
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
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:
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
sid597
left a comment
sid597
left a comment
There was a problem hiding this comment.
Notes on the non-obvious decisions, one per thread.
| label: string; | ||
| template?: string; | ||
| templateTitle?: string; | ||
| format?: string; |
There was a problem hiding this comment.
Flat literal_content.format, per the decision on the ticket (MG, team chat 2026-08-19). Obsidian's schema parser already reads flat format as a fallback, so the Obsidian producer stays unchanged. A separate key also avoids template, which the sync producer already uses for the block template.
| const literalInfo = filterUndefined({ | ||
| template: node.templateTitle, | ||
| template_content: node.template, | ||
| format: node.format, |
There was a problem hiding this comment.
The publish path writes only format here, no label or template. That asymmetry with the sync producer is pre-existing: publishNodesToGroups only upserts schemas that are not yet in my_concepts, and the next periodic sync rewrites the row with the full key set. Unifying the two shapes is out of scope for this ticket.
| ): LocalConceptDataInput => { | ||
| const titleParts = node.text.split("/"); | ||
| const label = titleParts[titleParts.length - 1] ?? node.text; | ||
| const literalContent: { [key: string]: Json } = { |
There was a problem hiding this comment.
literal_content is now built once, then the template branch adds its key. The old shape replaced the whole object in that branch, so every new key had to be written in two places. No behavior change for label and template.
| label: s.text, | ||
| authorId: userUid, | ||
| createdAt: new Date(relData[":create/time"] || Date.now()), | ||
| format: s.format, |
There was a problem hiding this comment.
DiscourseNode.format is marked deprecated in favor of specification, but that deprecation is about node identification. format is still the title pattern, and the title pattern is what ENG-2156 and ENG-2157 read from the schema row.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3ea0a1b019
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…-type-format-with-schemas # Conflicts: # packages/database/doc/concept_design.md
1 participant
This is what we are following:
This PR is the format edge of the diagram: publishing the node type format that both directions depend on.
Roam now publishes a node type's format with its schema row, so an importer can seed a new local type from it (ENG-2156, ENG-2157).
CrossAppNodeSchemagainsformat?: string, mapped toliteral_content.formatincrossAppNodeSchemaToDbConcept.nodeSchemaToCrossApp(publish) anddiscourseNodeSchemaToLocalConcept(periodic sync).upsert_conceptsassignsliteral_contentwholesale, so a producer that omits the key erases it.formatas a fallback.Tests cover both producers, key preservation with a template, and the publish path end to end.
Closes ENG-2158