ENG-2142 Map source to the Roam referenced node on pull - #1382
Conversation
…annot fill
decorateTitle now returns null for formats without a {content} placeholder or
with placeholders such as {Source}: substituting the empty string dropped the
source from a Roam-format Evidence name and produced a title that no longer
matched the format. The Obsidian format-expression helper reuses the shared
placeholder pattern so decorate and match agree.
createDiscourseNodeType only invalidates the new-store cache; with the store flag off getDiscourseNodes reads discourseConfigRef.nodes, so a created type stayed invisible and every later import re-entered the create branch. Mirror the settings panel and call refreshConfigTree after a successful create, and only count the type as created once the create resolved. Built-in types now take part in name matching so a remote schema named Page resolves to Roam's Page instead of creating a user type that shadows it. The resolver returns the map keyed by schema id, which both callers already hold. Format precedence follows the Obsidian reader (source_data first, ||), and the Roam format-expression helper reuses the shared placeholder pattern.
…am-from-core_title
…orted-node-titles-in-roam-from-core_title
…ning identity paths
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.
|
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. |
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
sid597
left a comment
sid597
left a comment
There was a problem hiding this comment.
Guideposts for the non-obvious decisions in this diff.
| // A shared node refers to another node by its bare local id when both live in the same | ||
| // space, and by a RID otherwise. "note" is the subtype node RIDs carry; URL-shaped Roam | ||
| // RIDs ignore it. | ||
| export const sharedReferenceRid = ( |
There was a problem hiding this comment.
The isRid branch is required by the contract, not defensive. A slot value is a bare local id only when the referring and referred nodes share a space, and per maparent on #1214 a source_local_id may itself be a RID. No producer emits one today: Roam writes 9-char uids and Obsidian writes note paths. "note" is the subtype sharedNodes.ts gives node RIDs; URL-shaped Roam RIDs ignore it, so one literal serves both platforms.
|
|
||
| // The local page for a node another space refers to: its own uid when the RID points | ||
| // into this graph, else the page imported from it. | ||
| export const findTargetUid = async ( |
There was a problem hiding this comment.
Moved out of importSharedRelations.ts so the node importer can share it. Two deltas from the original, both in the parity table in the description: the local graph URL is read per call instead of at module load, which is what lets this be unit tested, and the unused ridType parameter is gone. The bare-local-id path now also runs the own-space check; it cannot change relation import, because importRelations derives spaceUri from the relation's own RID and discovery excludes the local space.
| const rid = sharedReferenceRid(localOrRid, spaceUri); | ||
| const { spaceUri: ridSpaceUri, sourceLocalId } = ridToSpaceUriAndLocalId(rid); | ||
| if (ridSpaceUri === canonicalRoamUrl()) { | ||
| const result = window.roamAlphaAPI.q( |
There was a problem hiding this comment.
Moved as is, sync q included. The query binds :block/uid, so it matches blocks as well as pages; relations legitimately target blocks. On the pull path a block uid has no page title, so resolveSourceTitle treats it as missing and keeps the incoming title.
| // Inverse of sourceUidOfNode, for the pull side: the local title of a node whose format | ||
| // names a source, built from its core title and the Source page's title. Null when the | ||
| // format has a placeholder neither fills, so the caller keeps the incoming title. | ||
| export const titleWithSource = ({ |
There was a problem hiding this comment.
Roam-only on purpose, and not a fields parameter on decorateTitle. {Source} in Roam titles is slated for removal (ENG-2043, DES-368) and ENG-2176 unifies the format grammar, so the fill stays next to the push-side hack it inverts and both retire together. Obsidian's pull path keeps the published title for these formats; ENG-2140 owns Obsidian's source mapping.
Known limitation, same grammar weakness ENG-2176 addresses: a core title containing the format separator does not survive a republish. coreTitle = "a - b" yields [[EVD]] - a - b - [[Src]], and the lazy capture groups in getDiscourseNodeFormatExpression then read the source as b - [[Src]].
| // nodes other batch members refer to are materialized first. One level only: a source | ||
| // that itself names a source in the batch is not ordered after it, and its title is | ||
| // filled on the next refresh instead. | ||
| const orderSourcesFirst = (sharedNodes: SharedNode[]): SharedNode[] => { |
There was a problem hiding this comment.
One pass, not a topological sort. The slot accepts any discourse node (sourceSlot.ts), so a source that itself names a source in the batch is possible; that node keeps its incoming title with a warning and is filled on the next Refresh. resolveSharedNodeTypes still receives the original order; only the materialization loop is reordered.
There was a problem hiding this comment.
Why not reuse the topological sort that already exists as orderConceptsRec ?
There was a problem hiding this comment.
Reused orderConceptsByDependency, which calls orderConceptsRec. A small adapter maps each SharedNode and its source reference to RIDs before sorting, then returns the original SharedNode objects. This avoids collisions between bare IDs from different spaces. The separate ordering algorithm is gone. Tests cover chained sources, cycles, and references absent from the batch.
| title: pageTitle, | ||
| }); | ||
| })); | ||
| return result.success && warning ? { ...result, warning } : result; |
There was a problem hiding this comment.
Attached here rather than passed into createImportedPage and updateImportedPage: the warning belongs to the title decision, which both share, and neither helper would do anything with it but echo it.
| >; | ||
| type Platform = Enums<"Platform">; | ||
|
|
||
| const nodeRidSubtype = (platform: Platform): string | undefined => |
There was a problem hiding this comment.
Bug fix in its own commit. Node RIDs already carried the platform subtype, but cross-space slot RIDs did not, so a slot pointing at an Obsidian node produced orn:obsidian:vault/x while the same node's stored importedFrom.sourceNodeRid was orn:obsidian.note:vault/x, and the provenance lookup never matched. No migration: stored RIDs already carry the subtype. dbToCrossAppConverters.ts:32 builds relation endpoint RIDs the same way and still omits it; that is ENG-2215.
| : "refresh-imported-node-failed", | ||
| intent: result.success ? "success" : "danger", | ||
| content: result.message, | ||
| intent: !result.success |
There was a problem hiding this comment.
A refresh that kept the title because the source is missing succeeded, so the toast stays a success message, but it carries a warning, so its intent matches the import dialog's callout for the same case.
| ? [{ sharedNode: item.sharedNode, message: item.warning }] | ||
| : [], | ||
| ); | ||
| const importNotices = [...failedImports, ...warnings]; |
There was a problem hiding this comment.
Warnings are listed with the failures in the same per-node form and counted in the title, so a yellow callout never shows bullets its title does not account for. The warning text says the title was kept, which is what distinguishes it from a failure.
| })); | ||
|
|
||
| // Runs before the imports above: getDiscourseNodes calls generateUID at module load. | ||
| vi.hoisted(() => { |
There was a problem hiding this comment.
Same block in importSharedNodes.test.ts, refreshImportedNode.test.ts and sourceSlot.test.ts. getDiscourseNodes.ts builds DEFAULT_NODES at module load and calls generateUID while doing it; importing sourceSlot pulls that in, so window has to exist before the imports run. Copied from roamToCrossAppConverters.test.ts.
|
Size justification, per the guideline: one problem, resolving a pulled node's source into its Roam title. 232 lines outside tests across 9 files; 480 of the 712 lines are unit tests for the identity paths and fallbacks the ticket lists. The files are coupled: the RID subtype fix in |
…orted-node-titles-in-roam-from-core_title
…orted-node-titles-in-roam-from-core_title
…titles-in-roam-from-core_title' into codex/workday-20260906-pr-1382
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| const result = window.roamAlphaAPI.q( | ||
| `[:find (?e) :where [?e :block/uid "${sourceLocalId}"]]`, | ||
| ); |
There was a problem hiding this comment.
🟥 Shared source IDs inject Datalog
A shared Roam source ID is interpolated into roamAlphaAPI.q without parameters. Crafted IDs can alter the query and resolve an attacker-chosen page.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
The starting head 80acda8 already binds the source ID through :in instead of interpolating it. That binding is preserved with data.async.fast.q. The regression test passes an ID containing Datalog syntax as a separate argument.
|
@coderabbitai full review |
|
|
Warning Review limit reachedNext included review available in 58 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (19)
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 |
maparent
left a comment
maparent
left a comment
There was a problem hiding this comment.
LGTM, but I think one function can just go.
| // nodes other batch members refer to are materialized first. One level only: a source | ||
| // that itself names a source in the batch is not ordered after it, and its title is | ||
| // filled on the next refresh instead. | ||
| const orderSourcesFirst = (sharedNodes: SharedNode[]): SharedNode[] => { |
There was a problem hiding this comment.
Why not reuse the topological sort that already exists as orderConceptsRec ?
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
Missing {source} requires a placeholder, otherwise it won't be recognized as the original node type.
Decision notes: https://roamresearch.com/#/app/discourse-graphs/page/7H06vXWQc
| const rid = sharedReferenceRid(localOrRid, spaceUri); | ||
| const { spaceUri: ridSpaceUri, sourceLocalId } = ridToSpaceUriAndLocalId(rid); | ||
| if (ridSpaceUri === canonicalRoamUrl()) { | ||
| const result = window.roamAlphaAPI.q( |
There was a problem hiding this comment.
Never use window.roamAlphaAPI.q
Always use window.roamAlphaAPI.data
Prefer data.async.fast.q
There was a problem hiding this comment.
Changed the existence lookup to await window.roamAlphaAPI.data.async.fast.q. The shared UID stays bound through :in, and the tests now return promises so they exercise the async path.
| // either of them. | ||
|
|
||
| export const SOURCE_SLOT = "sourceDocument"; | ||
| export const MISSING_SOURCE_PLACEHOLDER = "(source missing)"; |
There was a problem hiding this comment.
Change to [[@placeholder]] otherwise it won't serve the intended purpose. Currently this placeholder would not let the Evidence node be identified as an evidence node.
EG: it has to match: [[EVD]] - {content} - {Source} and typically {Source} is defined as a page with this format: @title
Ideally it should grab the format from the linked reference node (eg: {Source} would see what the {Source} format is and adhere to that. But we can defer that to v1. If you decide to defer it, please create the ticket and link it here.
There was a problem hiding this comment.
I think we should fix this using the format. For now this will work because [[EVD]] - {content} - {Source} turns into ^[[EVD]] - (.*?) - (.*?)$ so any text after the second dash matches I checked the 4 graphs that we have discourse-graphs, megacoglab, akamatsulab and discourse-dev-graph (the one I tested on). Also you can see in the image there is the publish button that only appears because this page is being recognised as a dg node
…o-the-roam-referenced-node-on-pull # Conflicts: # apps/roam/src/utils/__tests__/materializeSharedNode.test.ts # apps/roam/src/utils/importSharedNodes.ts # apps/roam/src/utils/importSharedRelations.ts # apps/roam/src/utils/materializeSharedNode.ts # packages/database/src/lib/__tests__/sharedNodes.test.ts # packages/database/src/lib/sharedNodes.ts
3 participants
Reviewer brief
{Source}gets its title rebuilt fromcore_titleand the local page of the source named inslots.sourceDocument, for example[[EVD]] - REM sleep and recall - [[@Smith 2020]]. The source is found in this order: a RID in this graph's space is the page uid; otherwise the page whoseimportedFrom.sourceNodeRidmatches, which also covers a source in the same import batch because referenced sources are materialized first. A missing source fills{Source}with[[@placeholder]]so the title keeps the node type's format, and reports a warning in the import dialog and the Refresh toast; Refresh replaces the placeholder once the source is imported (95eab0d, per mdroidian's review).findTargetUidmoved out ofimportSharedRelations.tsinto its own util so the node importer shares it (parity table below). The{Source}fill is Roam-only insourceSlot.tsanddecorateTitleis unchanged, because{Source}in Roam titles is slated for removal (ENG-2043, DES-368) and ENG-2176 unifies the format grammar. The decision trace with the alternatives considered is the ENG-2142 comment of 2026-09-02.dbToCrossAppConverters.ts:32builds cross-space relation endpoint RIDs without the platform subtype, the same bug this PR fixes for slot RIDs insharedNodes.ts; that is ENG-2215. Obsidian's pull side still keeps the published title for{Source}formats (decorateTitlereturns null); ENG-2140 owns Obsidian's source mapping. A core title containing the format separator does not survive a republish (lazy capture groups; ENG-2176). The batch ordering is one level deep: a source that itself names a source in the batch is filled on the next Refresh. The up-to-date skip stays timestamp-only (ENG-2156 Decorate imported node titles in Roam from core_title #1331 decision), so an Evidence whose source arrives later keeps its title until Refresh.Parity table for the moved
findTargetUid:importSharedRelations.ts)findTargetUid.ts)localSpaceUrlread fromroamAlphaAPI.graph.nameat module loadcanonicalRoamUrl()called inside the functionroamAlphaAPI, which is what makes the util unit-testableridType?: stringparameter,ridType ?? "note""note"passed directlysharedReferenceRid, then the own-space check, then the lookupimportRelationsderivesspaceUrifrom the relation's own RID and discovery excludes the local space, so relation import is unaffectedsharedReferenceRidorderSourcesFirstneeds the same encoding to match a batch member'sridwindow.roamAlphaAPI.qexistence checkVerification
pnpm ci:validateexit 0 (type checks in 12 packages, unit tests in 3).apps/roamvitest 221 passed, including new cases for each identity path, a missing source and its placeholder title, no published source, an unchanged refresh, a refresh after the source arrives, the up-to-date skip, batch ordering, and warning pass-through.packages/databasevitest 17 passed.Loom video
eng-2142-20260907.mp4
Recorded before the placeholder change (95eab0d).
Scope check
$scope-checkagainst ENG-2142 and the final diff.Done When: None.Local delegated full review
[[@placeholder]]reference parses as a Source on republish and reachessourceDocument, and two imports with the same core title and unresolved Sources now collide. Hold review of the placeholder change until the follow-up commit lands.