[ENG-1848] Add Roam full markdown content variant for shared nodes#1134
[ENG-1848] Add Roam full markdown content variant for shared nodes#1134sid597 wants to merge 5 commits into
Conversation
|
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. 1 Skipped Deployment
|
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 base branch from
main
to
eng-1847-define-shared-cross-app-node-content-contract
![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: add4831df7
ℹ️ 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".
| const fullContent = convertRoamNodeToFullContent({ | ||
| nodes: roamNodes, | ||
| }); |
There was a problem hiding this comment.
The new full rows are only created for the roamNodes passed into this function. On the first sync after upgrading an existing graph, getAllMissingOrNewDiscourseNodes filters out nodes that already have any my_contents row plus a concept, without checking for variant = 'full', so nodes synced before this change are skipped and never get the required full variant until the Roam page is edited. Please include nodes missing the full content variant or run a one-time backfill before relying on this new contract.
Useful? React with 👍 / 👎.
| nodes.flatMap((node) => { | ||
| try { | ||
| const title = node.node_title ?? node.text; | ||
| const blocks = getFullTreeByParentUid(node.source_local_id).children; |
There was a problem hiding this comment.
For node types backed by an Embedding Block Ref, getDiscourseNodeTypeWithSettingsBlockNodes sets source_local_id to the child block UID and stores that block's text in node.text; reading only .children here serializes descendants but drops the source block itself. A block-backed discourse node with no children now uploads # <page title> as its full content, and one with children still omits the actual claim/evidence text, so imported materializations are incomplete.
Useful? React with 👍 / 👎.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| } catch (error) { | ||
| console.error( | ||
| `convertRoamNodeToFullContent: failed to build full markdown for ${node.source_local_id}:`, | ||
| error, | ||
| ); | ||
| return []; |
There was a problem hiding this comment.
🚩 Full content silently skipped on error doesn't propagate to caller
When getFullTreeByParentUid or toMarkdown throws for a specific node (line 74-79), the try/catch returns [], meaning that node silently won't have a full content variant uploaded. The contract (crossAppNodeContract.ts:28-31) declares both direct and full as required variants (SHARED_NODE_CONTENT_VARIANTS). If downstream importers strictly validate for the presence of both variants, nodes that failed full generation would appear incomplete. This graceful degradation is intentional per the error log, but worth noting for consumers.
Was this helpful? React with 👍 or 👎 to provide feedback.
sid597
force-pushed
the
eng-1848-add-roam-full-markdown-content-variant-for-shared-nodes
branch
from
add4831 to
43646a5
Compare
sid597
changed the title
sid597
force-pushed
the
eng-1848-add-roam-full-markdown-content-variant-for-shared-nodes
branch
from
43646a5 to
b4f758c
Compare
sid597
changed the base branch from
eng-1847-define-shared-cross-app-node-content-contract
to
main
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
|
Verification note for ENG-1848:
[
{
"source_local_id": "dnHNmYwe5",
"variant": "full",
"text": "# [[CLM]] - Actin assembly peaks 8 seconds before endocytic scission\n\n- ## Source of Claim ..."
}
]
Open scope question from review: whether ENG-1848 should also backfill unchanged already-synced nodes that are missing |
1 participant
Summary
Rebases ENG-1848 on the final ENG-1847 baseline (
5954c548) and keeps the ticket focused on Roam emitting the required markdownfullcontent variant for shared nodes.convertRoamNodeToFullContentso the existing Roam sync loop uploadsfullcontent alongside the current direct content.# {title}plus the node body via the existingtoMarkdownserializer with refs/embeds inlined.CrossAppNodecontract shape:CrossAppNode["content"]["full"]with inlineformat: "text/markdown".Validation
pnpm exec prettier --check apps/roam/src/utils/convertRoamNodeToFullContent.fixture.ts apps/roam/src/utils/convertRoamNodeToFullContent.ts apps/roam/src/utils/syncDgNodesToSupabase.tspnpm --filter roam check-typespnpm --filter roam lint(0 errors; existing repo warnings remain)Manual follow-up
A real Roam graph with sync enabled still needs the Supabase smoke check: confirm a discourse node writes a
variant: "full"markdown row alongside its direct content row.