Skip to content

[ENG-1848] Add Roam full markdown content variant for shared nodes#1134

Open
sid597 wants to merge 6 commits into
mainfrom
eng-1848-add-roam-full-markdown-content-variant-for-shared-nodes
Open

[ENG-1848] Add Roam full markdown content variant for shared nodes#1134
sid597 wants to merge 6 commits into
mainfrom
eng-1848-add-roam-full-markdown-content-variant-for-shared-nodes

Conversation

@sid597

@sid597 sid597 commented Jun 19, 2026

Copy link
Copy Markdown
Collaborator

Summary

Rebases ENG-1848 on the final ENG-1847 baseline (5954c548) and keeps the ticket focused on Roam emitting the required markdown full content variant for shared nodes.

  • Adds convertRoamNodeToFullContent so the existing Roam sync loop uploads full content alongside the current direct content.
  • Keeps direct content and embedding behavior unchanged: direct rows are embedded, full rows are not embedded.
  • Shapes Roam full content as # {title} plus the node body via the existing toMarkdown serializer with refs/embeds inlined.
  • Updates the Roam reference fixture to the final CrossAppNode contract shape: CrossAppNode["content"]["full"] with inline format: "text/markdown".
  • Does not export or recreate the deleted draft persistence fixtures or package-level content constants.

Validation

  • pnpm exec prettier --check apps/roam/src/utils/convertRoamNodeToFullContent.fixture.ts apps/roam/src/utils/convertRoamNodeToFullContent.ts apps/roam/src/utils/syncDgNodesToSupabase.ts
  • pnpm --filter roam check-types
  • pnpm --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.


Open in Devin Review

@linear-code

linear-code Bot commented Jun 19, 2026

Copy link
Copy Markdown

ENG-1848

@supabase

supabase Bot commented Jun 19, 2026

Copy link
Copy Markdown

This pull request has been ignored for the connected project zytfjzqyijgagqxrzbmz because there are no changes detected in packages/database/supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.


Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

@vercel

vercel Bot commented Jun 19, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
discourse-graph Skipped Skipped Jun 23, 2026 9:49am

Request Review

@graphite-app

graphite-app Bot commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

PR size/scope check

This PR is over our review-size guideline.

  • Recommended: ~200 lines changed
  • Acceptable limit: up to 400 lines when well-scoped/self-contained
  • Preferred file count: fewer than 5 files

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:

  • What single problem this PR solves
  • Why the files/changes are coupled

@sid597 sid597 changed the base branch from main to eng-1847-define-shared-cross-app-node-content-contract June 19, 2026 08:44

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread apps/roam/src/utils/syncDgNodesToSupabase.ts Outdated
Comment thread apps/roam/src/utils/convertRoamNodeToFullContent.ts

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 1 potential issue.

Open in Devin Review

Comment thread apps/roam/src/utils/convertRoamNodeToFullContent.ts
@sid597 sid597 force-pushed the eng-1848-add-roam-full-markdown-content-variant-for-shared-nodes branch from add4831 to 43646a5 Compare June 19, 2026 10:02
@sid597 sid597 changed the title Eng 1848 add roam full markdown content variant for shared nodes [ENG-1848] Add Roam full markdown content variant for shared nodes Jun 19, 2026
@sid597 sid597 force-pushed the eng-1848-add-roam-full-markdown-content-variant-for-shared-nodes branch from 43646a5 to b4f758c Compare June 19, 2026 16:37
@sid597 sid597 changed the base branch from eng-1847-define-shared-cross-app-node-content-contract to main June 19, 2026 16:39
Comment thread apps/roam/src/utils/syncDgNodesToSupabase.ts Outdated

sid597 commented Jun 21, 2026

Copy link
Copy Markdown
Collaborator Author

Reviewer verification note:

There is no Loom for this PR because ENG-1848 has no user-facing UI surface; it adds the Roam full markdown producer plus the gated Supabase upload path.

Final-branch checks run:

pnpm exec prettier --check apps/roam/src/utils/convertRoamNodeToFullContent.ts apps/roam/src/utils/convertRoamNodeToFullContent.example.ts apps/roam/src/utils/syncDgNodesToSupabase.ts
pnpm --filter roam check-types
SUPABASE_USE_DB=local pnpm --filter roam build

The representative Roam input is captured in apps/roam/src/utils/convertRoamNodeToFullContent.example.ts from page UID dnHNmYwe5; typecheck verifies the generated markdown fits CrossAppNode["content"]["full"].

Optional historical smoke-test reproduction:

The temporary console hook used during review exists only in commit f29b4b9d and was intentionally removed in 430f0e2a.

git checkout f29b4b9d
SUPABASE_USE_DB=local pnpm --filter roam build

Then load the built Roam extension in graph plugin-testing-akamatsulab2, open page UID dnHNmYwe5, and run:

await window.dgSyncEng1848ReviewPage()

That smoke hook writes the page's full markdown row through upsert_content and reads back the matching my_contents row.

Note: the hook is not part of the final PR surface. The final branch keeps only the reusable producer/upload path, with full gated by includeFullContent: true; the real shared-node caller belongs to ENG-1851/ENG-1852.

allNodes: [] as DiscourseNode[],
};

export const buildFullMarkdown = ({

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This intentionally reuses Roam’s existing toMarkdown serializer instead of defining a new markdown format here. ENG-1848’s job is just to adapt Roam page/block content into the existing cross-app full markdown shape.

}
};

if (includeFullContent) {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

full is opt-in because Roam should only write the full variant for shared/published nodes. Existing background embedding sync stays direct/direct-ish only.

const fullContent = convertRoamNodeToFullContent({
nodes: roamNodes,
});
await uploadBatches(chunk(fullContent, BATCH_SIZE));

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

full is uploaded before fetching embeddings so markdown persistence is not blocked by embedding service failures. Embeddings remain only on direct / direct_and_description.

* node contract.
*
* Source Roam page:
* https://roamresearch.com/#/app/plugin-testing-akamatsulab2/page/dnHNmYwe5

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This example uses the real getFullTreeByParentUid(...).children shape from the linked Roam page. We also smoke-tested that page against local Supabase and read back the expected variant = "full" markdown row.

@sid597 sid597 requested review from maparent and mdroidian June 23, 2026 10:15

@maparent maparent left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
Question in a line comment.

embeds: true,
simplifiedFilename: false,
removeSpecialCharacters: false,
maxFilenameLength: 64,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious about this value: Does it affect the title? (I expect not.) Asset filenames?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants