Skip to content

ENG-2230 Split filepath and source_path across Obsidian publish and import - #1395

Open
maparent wants to merge 2 commits into
eng-1870-copy-roam-origin-assets-into-supabase-on-publishfrom
eng-2230-split-filepath-and-source_path-across-obsidian-publish-and
Open

ENG-2230 Split filepath and source_path across Obsidian publish and import#1395
maparent wants to merge 2 commits into
eng-1870-copy-roam-origin-assets-into-supabase-on-publishfrom
eng-2230-split-filepath-and-source_path-across-obsidian-publish-and

Conversation

@maparent

@maparent maparent commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator

Reviewer brief

This touches two things, in separate commits. (Could be split easily if desired.)

  1. Allow importing Roam file references correctly, using the source_path as a way to get a proper filename.
  2. Publish the Obsidian information in a way that clients like Roam can interpret without deep knowledge of Obsidian file search mechanisms.
  • This has a small consequence worth noting: if a node contains the same asset with different paths, we will get multiple FileReference rows. (Scope check point 2)
    While doing that, we discovered and fixed a related bug, see scope check point 3.

Corresponds to these OpenSpec tasks:
https://roamresearch.com/#/app/discourse-graphs/page/fmJkSx36O

Verification

Loom checks:

Importing roam data in Obsidian, collision detection:
https://www.loom.com/share/ae2f845d4b7c48e0a74adfdda12bb5bc

Import legacy asset
https://www.loom.com/share/44cdf4b1ec93428fb3c32cc591b0b057

Loom video

Code walkthrough forthcoming.

Scope check

  • Ran $scope-check against the ENG ticket and final diff.

  • Scope beyond Done When: Three items.

    1. Publish now records one FileReference per link spelling within a single note, where the old keying on the resolved path (attachment.path) collapsed them to one row (syncDgNodesToSupabase.ts:741-757, findEmbeddedAttachments at :693-708). Done When 6 covers spellings across two notes, which per-node keying already gave.
    2. updateMarkdownAssetLinks now rewrites links whose text is a storage URL, consulting pathMapping before the http(s) early return in both the markdown-link and image branches (importNodes.ts:741-751, :774-800).
    3. Two pre-existing importer bugs fixed alongside it: the metadataCache.getFirstLinkpathDest round trip on a just-written file is removed (importNodes.ts:673-678, :798-800), and setPathMapping is hoisted to one call with unsetPathMapping on failure so the reuse branch no longer drops the mapping (importNodes.ts:941-945, :1030-1035, :1125-1127).
  • Required now:

  1. follows from keying rows on the link, which Done When 5 and 7 require; the same-note case is a side effect of that keying rather than a separate decision, and it is bounded — storage is content-addressed (packages/database/src/lib/files.ts:36-43), and import reuses one vault file per filehash.
  2. is required for the co-ship with ENG-1870: source_path ?? filepath satisfies Done When 1 but leaves the note pointing at the storage URL, so the asset would import orphaned.
  3. is a correctness fix on the same code path, not required by any criterion on its own. (Actually the manual tests failed until this bug was corrected.)
  • Anyone affected or consulted: No
  • Decision: Not documented. No

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.
    Two observations were postponed, written as "Decision taken" in the linear ticket.

https://linear.app/discourse-graphs/issue/ENG-2230/split-filepath-and-source-path-across-obsidian-publish-and-import

@linear-code

linear-code Bot commented Sep 5, 2026

Copy link
Copy Markdown

ENG-2230

@supabase

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

@vercel

vercel Bot commented Sep 5, 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:32pm UTC

Request Review

@maparent
maparent force-pushed the eng-2230-split-filepath-and-source_path-across-obsidian-publish-and branch from 6f81dd5 to 2add534 Compare September 5, 2026 14:42
@maparent
maparent marked this pull request as ready for review September 5, 2026 14:55
@maparent
maparent force-pushed the eng-2230-split-filepath-and-source_path-across-obsidian-publish-and branch from 2add534 to 1a826e3 Compare September 6, 2026 14:37
@graphite-app

graphite-app Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

PR size/scope check

This PR is over our review-size guideline.

  • Recommended: ~200 lines changed
  • Acceptable limit: up to 400 lines when well-scoped/self-contained
  • Preferred file count: fewer than 5 files

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:

  • What single problem this PR solves
  • Why the files/changes are coupled

@maparent
maparent force-pushed the eng-2230-split-filepath-and-source_path-across-obsidian-publish-and branch 2 times, most recently from 1a826e3 to 4831cd1 Compare September 6, 2026 15:01
@maparent
maparent force-pushed the eng-2230-split-filepath-and-source_path-across-obsidian-publish-and branch from 401a8e6 to 4831cd1 Compare September 6, 2026 15:03
@maparent
maparent force-pushed the eng-2230-split-filepath-and-source_path-across-obsidian-publish-and branch from 4831cd1 to 7121858 Compare September 6, 2026 22:32
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