ENG-2141 Map the Roam referenced node to source on push - #1380
Conversation
Publish now looks the referenced Source up in my_concepts alongside the schemas, drops the sourceDocument slot with a warning when it is absent, and orders the upsert so an in-batch Source precedes the node that references it. An imported Source publishes its origin RID instead of its local uid; sourceUidOfNode becomes sourceIdOfNode because it can return either.
|
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.
|
sid597
left a comment
sid597
left a comment
There was a problem hiding this comment.
Inline notes on the decisions in this diff. Three are open questions for the reviewers.
| ), | ||
| ...missingRelations.map((r) => crossAppRelationToDbConcept(r)), | ||
| ].filter((r) => r !== undefined); | ||
| const omitMissingSource = (node: CrossAppNode): CrossAppNode => { |
There was a problem hiding this comment.
Decision point. A missing Source drops the slot instead of publishing the Source as a dependency (ticket default; scoping doc F1 and F5). If that decision changes, this closure is the one branch to replace.
There was a problem hiding this comment.
Decided: option 1 stands (Michael on ENG-2141, 2026-09-02). Publishing the Source with the Evidence is ENG-2232, for after v0.
| const sourceId = node.slots?.[SOURCE_SLOT]; | ||
| if ( | ||
| sourceId === undefined || | ||
| isRid(sourceId) || |
There was a problem hiding this comment.
Open question. A RID is trusted without a lookup. The my_concepts query above is filtered to this space, and checking a RID needs its origin space id (a Space lookup or the rid_to_space_id_and_local_id RPC). rid_or_local_id_to_concept_db_id runs under the publisher's read access, so a revoked grant or a deleted origin concept still fails the row with -2. Proposal: accept for v0 and file a follow-up.
| syncedUids.has(sourceId) | ||
| ) | ||
| return node; | ||
| console.warn( |
There was a problem hiding this comment.
Open question. This matches the sync path's console.warn for dependencies missing from a batch. Export.tsx already builds the publish toast from PublishNodesResult, so an omitted-source count there plus a PostHog property is about 10 lines. Do we want it in this PR? The title is re-read by uid because threading it needs a field on CrossAppNode, which the ticket puts out of scope.
| console.warn( | ||
| `Source "${getPageTitleByPageUid(sourceId)}" (${sourceId}) is not in this space yet; publishing "${node.content.direct.value}" without it.`, | ||
| ); | ||
| return { ...node, slots: undefined }; |
There was a problem hiding this comment.
slots carries only sourceDocument in v0; roamToCrossAppConverters.ts:149 is the single producer. Dropping the object equals dropping the key until a second slot exists.
| return { ...node, slots: undefined }; | ||
| }; | ||
|
|
||
| const { ordered: upsertConcepts } = orderConceptsByDependency( |
There was a problem hiding this comment.
Reuses the sync path's ordering so an in-batch Source is upserted before the Evidence that references it; upsert_concepts processes rows in order. The old order (schemas, nodes, relation schemas, relations) is preserved, so the index mapping to response.data below still holds. missing is ignored on purpose: already-synced schemas are not in the batch, so every node's schema would appear there.
| // placeholder is usually filled with a page reference, and a title holding a slash is | ||
| // a namespaced page rather than a source, so it is left alone. | ||
| export const sourceUidOfNode = ( | ||
| export const sourceIdOfNode = ( |
There was a problem hiding this comment.
Renamed from sourceUidOfNode because it can now return a RID. Both callers renamed their local variable to match. No behavior change for the uid case.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Devin Review found 1 potential issue.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
![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: 4bd9ba0bc5
ℹ️ 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".
isRid only checks the prefix, so a value like orn:bad replaced a good uid and then skipped the publish existence check. Require a space URI and a local id from ridToSpaceUriAndLocalId, else keep the uid.
| const sourceUid = getPageUidByPageTitle(sourceTitle); | ||
| if (!sourceUid) return undefined; | ||
| const sourceRid = readImportedSourceIdentity(sourceUid)?.sourceNodeRid; | ||
| return sourceRid !== undefined && isWellFormedRid(sourceRid) |
There was a problem hiding this comment.
Per maparent on #1214, a localId may itself be a RID, so CrossAppNode.slots keeps its LocalId type; rid_or_local_id_to_concept_db_id accepts both. The value must parse to a space URI and a local id because block props are user-editable; anything else keeps the page uid. Existing rows: sync now writes the origin RID where it used to write the Roam mirror's uid, so already-synced Evidence repoints on the next sync.
The shared parser falls back to splitting at the last slash, so checking its output accepted values like orn:broken/node-1 that the database cannot resolve; the null reference would fail the Evidence row's upsert. Match the two RID shapes the database recognises instead.
|
Decision: a missing referenced Source drops the |
…-referenced-node-to-source-on-push # Conflicts: # apps/roam/src/utils/__tests__/publishNodesToGroups.test.ts # apps/roam/src/utils/publishNodesToGroups.ts
1 participant
Reviewer brief
sourceDocumentslot when the referenced Source is not a Concept in the space, warns with the page name, and orders the upsert so an in-batch Source precedes the node that references it. An imported Source publishes its origin RID instead of its local uid.omitMissingSource(publishNodesToGroups.ts). Decision points are marked as inline comments.rid_to_space_id_and_local_id?console.warnonly. Open question: also count omitted sources onPublishNodesResultand show it in the Export toast?CrossAppNode.slotson the Obsidian side yet. ENG-2140 owns that.Verification
tsc, Prettier, and 173 unit tests pass.turbo run test:unitpasses across workspaces.reference_contentand a warning that names the Source, and after the Source is published the slot resolves to its concept row. The imported-RID path is covered by unit tests only.video
eng-2141-full.mp4
Scope check
$scope-checkagainst ENG-2141 and the final diff.Done When: None.Local delegated full review
$dg-delegated-full-reviewwhen no other full-review workflow is available.pre-pr-reviewpass ran before the last fixes (source uid dedupe, RID guard, RID parse check). The full pass on the pushed diff is still to run.