Skip to content

ENG-2216 Add a graph-level asset registry accessor for Roam - #1402

Open
maparent wants to merge 1 commit into
eng-2230-split-filepath-and-source_path-across-obsidian-publish-andfrom
eng-2216-Add-a-graph-level-asset-registry-accessor-for-Roam
Open

ENG-2216 Add a graph-level asset registry accessor for Roam#1402
maparent wants to merge 1 commit into
eng-2230-split-filepath-and-source_path-across-obsidian-publish-andfrom
eng-2216-Add-a-graph-level-asset-registry-accessor-for-Roam

Conversation

@maparent

@maparent maparent commented Sep 6, 2026

Copy link
Copy Markdown
Collaborator

Reviewer brief

These are prerequisites to eng-2219, and are not yet connected.

Verification

Has unit tests. Tested after integration in eng-2219.

Loom video

Forthcoming.

Scope check

  • Ran $scope-check against the ENG ticket and final diff.
  • Scope beyond Done When: Originally none. After corrections from review:

A shared isJsonObject guard moved from importedSourceIdentity.ts into getBlockProps.ts (pure move, no behavior change), and a console.warn when the registry page exists without its block.

  • Required now: The accessor needs the same JSON guard, so the alternative was a third copy of it, which AGENTS.md rules out. The warning covers the one detectable way the registry is silently orphaned; without it the graph re-uploads every asset it holds with no explanation.
  • Anyone affected or consulted: Yes — both came out of the delegated review on this branch.
  • Decision: Not documented. Record the two in a Linear comment on ENG-2216 before merge.

Local delegated full review

  • Ran a comprehensive review of the entire final diff in a subagent with a fresh context. Use $dg-delegated-full-review when no other full-review workflow is available.

Three findings were left unfixed, all of them for the same underlying reason: the module currently has no callers, so each one is a guess about a call site that hasn't been written yet.

  1. getOrCreateRegistryBlockUid can create duplicate registry blocks under concurrency — assetRegistry.ts:44
    The existence check and the create are separated by an await, so N overlapping first-writes on a fresh graph each see no block and each create one. The suggested fix was a module-level in-flight promise memo.

Set aside because no in-process parallel caller exists, and a module memo would not help the case that actually occurs in the wild: two Roam tabs on the same graph are two module instances, so the memo cannot span them. Adding it would buy protection only against a caller nobody has written, while implying a safety guarantee it doesn't provide. The cross-client race is the one ENG-2216 already priced: last write wins, cost is one redundant upload, do not add locking.

  1. readMirroredAssetUrl re-reads the whole registry on every lookup — assetRegistry.ts:81
    Each call costs a page-uid pull, a shallow-tree read, and a props pull, then rebuilds a filtered copy of the entire map to answer about one hash. Checking N assets costs 3N graph reads.

Set aside because the fix belongs in the caller, not here. A caller looping over a node's assets should hoist one readAssetRegistry() and index it; caching inside the module would instead need an invalidation story (when does the memo go stale relative to another tab's write?) that nothing can specify until the call site exists. readMirroredAssetUrl stays a correct convenience for one-off lookups, and the per-import path can bypass it.

  1. No invalidation path for stale URLs — assetRegistry.ts:88
    If a user deletes a mirrored file from Roam storage, the hash→URL entry survives and hands out a dead URL, which lands in imported blocks as a broken image. There is no removal export.

Set aside as YAGNI: nothing in the codebase can currently detect a dead URL, so there is no caller to hold the other end of a forgetMirroredAsset. Adding the export now would ship an untested, unreachable code path and lock in a signature before the detection mechanism — whatever eventually notices the 404 — exists to shape it. The registry is documented as a cache rebuildable from the imported- file names, so the failure is recoverable rather than data-losing.

The common thread worth recording on the ticket: 1 and 4 should be revisited when the first caller lands (whoever writes the per-node asset loop owns both the hoist and the question of whether parallel mirroring is worth it), and 5 when something can detect a dead URL.

https://linear.app/discourse-graphs/issue/ENG-2216/add-a-graph-level-asset-registry-accessor-for-roam

@linear-code

linear-code Bot commented Sep 6, 2026

Copy link
Copy Markdown

ENG-2216

@vercel

vercel Bot commented Sep 6, 2026

Copy link
Copy Markdown

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

1 Skipped Deployment
Project Deployment Actions Updated
discourse-graph Skipped Skipped Sep 6, 2026 10:40pm UTC

Request Review

@supabase

supabase Bot commented Sep 6, 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 ↗︎.

@maparent
maparent force-pushed the eng-2216-Add-a-graph-level-asset-registry-accessor-for-Roam branch from 401a8e6 to 2a3ca2f Compare September 6, 2026 16:00
@maparent
maparent marked this pull request as ready for review September 6, 2026 16:07

@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: No Issues Found

Devin Review analyzed this PR and found no bugs or issues to report.

Devin Review

@maparent
maparent force-pushed the eng-2216-Add-a-graph-level-asset-registry-accessor-for-Roam branch from 2a3ca2f to bc186d0 Compare September 6, 2026 22:32
@maparent
maparent force-pushed the eng-2216-Add-a-graph-level-asset-registry-accessor-for-Roam branch from bc186d0 to 0331964 Compare September 6, 2026 22:40
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.

1 participant