ENG-2219 Add an asset copy stage to Roam node materialization - #1401
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
cd7452d to
9c38544
Compare
9c38544 to
8f65942
Compare
8f65942 to
4620a4f
Compare
4620a4f to
45d86a6
Compare
45d86a6 to
06a464c
Compare
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:
|
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| // The asset stage never ran, so nothing was uploaded. A rejected import must leave no | ||
| // residue: a copy into Roam storage cannot be undone or even found afterwards. | ||
| expect(from).not.toHaveBeenCalledWith("my_file_references"); |
There was a problem hiding this comment.
Test assertion doesn't validate what the comment claims. Since importNodeAssets is mocked in beforeEach (lines 134-136), calling it would never invoke from regardless. This assertion always passes even if the asset stage runs.
// The asset stage never ran, so nothing was uploaded. A rejected import must leave no
// residue: a copy into Roam storage cannot be undone or even found afterwards.
expect(mockedImportNodeAssets).not.toHaveBeenCalled();| // The asset stage never ran, so nothing was uploaded. A rejected import must leave no | |
| // residue: a copy into Roam storage cannot be undone or even found afterwards. | |
| expect(from).not.toHaveBeenCalledWith("my_file_references"); | |
| // The asset stage never ran, so nothing was uploaded. A rejected import must leave no | |
| // residue: a copy into Roam storage cannot be undone or even found afterwards. | |
| expect(mockedImportNodeAssets).not.toHaveBeenCalled(); | |
Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
![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 3 potential issues.
2 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| const assets = await importNodeAssets({ | ||
| client, | ||
| sharedNode, | ||
| markdown: content.markdown, | ||
| }).catch((error: unknown) => ({ error })); |
There was a problem hiding this comment.
🔴 Failed imports leave permanent uploads
When importNodeAssets succeeds before a later page operation fails, its uploads remain unreferenced and cannot be deleted. Retrying can consume more storage without completing the import.
Prompt for agents
The new asset stage runs before createImportedPage or updateImportedPage. Successful Roam uploads are irreversible, but either subsequent function can fail during page creation, block replacement, rename, or source-identity persistence. In those cases materializeSharedNode reports failure after leaving permanent uploads that no imported page references. Rework the transaction order or failure semantics so uploaded assets remain associated with a successful materialization. Account for retries and for both create and update paths. If uploads must remain first so rewritten links are available, preserve enough state to avoid orphaning or re-uploading bytes when later stages fail.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const resolvable = new Set( | ||
| collectAssetTokens(markdown).flatMap(lookupCandidates), | ||
| ); | ||
| const referenced = references.filter(({ filepath }) => | ||
| resolvable.has(filepath), | ||
| ); |
There was a problem hiding this comment.
🟡 Spaced attachment links remain broken
For , collectAssetTokens finds no token, so the recorded attachment is excluded. The imported page retains a vault-only path that Roam cannot render.
Prompt for agents
importNodeAssets filters reference rows through collectAssetTokens before mirroring. The LINK_PATTERN used by collectAssetTokens only accepts unwrapped destinations without whitespace. Valid Markdown angle-bracket destinations such as  therefore never match, even when FileReference.filepath contains my folder/diagram.png. Extend token parsing and rewriting together to support angle-bracket Markdown destinations, and add importNodeAssets and rewriteAssetLinks tests proving the row is copied and the link is rewritten.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const resolvable = new Set( | ||
| collectAssetTokens(markdown).flatMap(lookupCandidates), | ||
| ); | ||
| const referenced = references.filter(({ filepath }) => | ||
| resolvable.has(filepath), | ||
| ); |
There was a problem hiding this comment.
🟡 Encoded aliases create unused uploads
When rows exist for both an encoded token and its decoded spelling, resolvable selects both. rewriteAssetLinks uses only the raw match, leaving the other upload permanently unused.
Prompt for agents
The resolvable Set flattens every lookup candidate, so it cannot preserve rewriteAssetLinks' ordered choice. For markdown containing my%20folder/d.png and reference rows named both my%20folder/d.png and my folder/d.png, importNodeAssets mirrors both rows. rewriteAssetLinks then chooses the raw candidate first and never references the decoded row's upload. Select at most the first matching reference row per parsed token using the same candidate priority as rewriteAssetLinks, while still supporting multiple distinct tokens and deduplicating repeated references.
Was this helpful? React with 👍 or 👎 to provide feedback.
06a464c to
1043e32
Compare
1043e32 to
61bedb2
Compare
61bedb2 to
4aa1ebe
Compare
4aa1ebe to
09adb17
Compare
1 participant
Reviewer brief
Verification
Loom video
Scope check
$scope-checkagainst the ENG ticket and final diff.Done When: Two additions.titleCollisionFailureand is now made inmaterializeSharedNodebefore the asset stage, andrewriteAssetLinksgained exportedlookupCandidates/collectAssetTokens(withparseMatch) so the stage copies only references the rewriter can resolve.getErrorMessagemoved to a newapps/roam/src/utils/getErrorMessage.ts, replacing the copies inmaterializeSharedNodeand inmirrorAssetToRoamStorage(an ENG-2218 file).importNodeAssetssatisfied the ticket; the shared module is cleanup that also touches files outside it. But we now had four shared copies.Local delegated full review
$dg-delegated-full-reviewwhen no other full-review workflow is available.https://linear.app/discourse-graphs/issue/ENG-2219/add-an-asset-copy-stage-to-roam-node-materialization