ENG-1870 Copy Roam-origin assets into Supabase on publish - #1394
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
d058750 to
14f6b43
Compare
14f6b43 to
6f408af
Compare
6f408af to
d891c94
Compare
d891c94 to
3010d2d
Compare
3010d2d to
7839b4c
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:
|
![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.
2 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| fname: assetUrl, | ||
| sourcePath: descriptor.filename, |
There was a problem hiding this comment.
🟡 Imported copies remain disconnected
Published Roam assets put their filename in source_path, but the importer ignores that field. It names files from the Firebase URL and leaves HTTP links unchanged, so imported notes never use the copied asset.
Prompt for agents
Complete the source_path contract across Roam publication and Obsidian import. Roam correctly stores the markdown token as FileReference.filepath and the original filename as source_path in apps/roam/src/utils/copyAssetToSharedStorage.ts. However, apps/obsidian/src/utils/importNodes.ts fetchFileReferences does not select source_path, importAssetsForNode derives its target path from filepath, and updateMarkdownAssetLinks skips HTTP URLs. For Roam-origin references this creates a local file from the Firebase URL but leaves the imported note linked to Firebase. Update the importer to use source_path for local naming and placement while retaining filepath as the lookup token, and rewrite matching HTTP asset tokens when a copied FileReference exists. Preserve existing Obsidian path behavior and add an integration-style test for a Roam Firebase image URL with source_path.
Was this helpful? React with 👍 or 👎 to provide feedback.
7839b4c to
264ccb5
Compare
1 participant
Reviewer brief
This is a large PR, there are a lot of moving pieces.
There are actually five subtasks, in distinct commits, which may be easier to understand:
https://roamresearch.com/#/app/discourse-graphs/page/4o_mub_mn
Verification
There are unit tests.
Loom video
Code walkthrough (long): https://www.loom.com/share/91078efd00b14f959ebf0ba07950a4fe
Note: I mention something missing during the code walkthrough, but upon verification it's fine.
Demo: https://www.loom.com/share/ecdd8cdfaca141cfb26216ecda5cdc08
Scope check
$scope-checkagainst the ENG ticket and final diff.Done When: None.Actually it called me on the change to return parameters of addFile, but I told it that CopiedAsset is the stage's record of what it stored, a stored asset's identity in shared storage is its hash, and the summary was previously conflating references with blobs. addFile's return is what makes that available without re-hashing. It accepted that justification as valid.
It's also pending the issue of whether the size limit should have been split between tickets, but right now it's within 1876.
Local delegated full review
$dg-delegated-full-reviewwhen no other full-review workflow is available.https://linear.app/discourse-graphs/issue/ENG-1870/copy-roam-origin-assets-into-supabase-on-publish