Skip to content

ENG-2217 Rewrite asset links in Obsidian-origin markdown for Roam - #1398

Open
maparent wants to merge 3 commits into
eng-2218-Copy-assets-from-supabase-into-Roam-storagefrom
eng-2217-rewrite-asset-links-in-obsidian-origin-markdown-for-roam
Open

ENG-2217 Rewrite asset links in Obsidian-origin markdown for Roam#1398
maparent wants to merge 3 commits into
eng-2218-Copy-assets-from-supabase-into-Roam-storagefrom
eng-2217-rewrite-asset-links-in-obsidian-origin-markdown-for-roam

Conversation

@maparent

@maparent maparent commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator

Reviewer brief

Given a markdown and assets, rewrite the links to use the firebase links.
We have to look up the file type to make the right kind of Roam link; since Obsidian does not have distinct link shapes, we have to infer from mimetype or file extension.

Verification

Unit tests. Real tests in the next PR.

Loom video

https://www.loom.com/share/ac85dc0a4df9411c84858e965dadb86f

Note: I did push a refactor from 2219 to 2217 after the loom, but no behaviour change.
I also renamed Token, which was ambiguous, to Locator.

Scope check

  • Ran $scope-check against ENG-2217 and the final diff.
  • Scope beyond Done When: Adds mime-db (+ @types/mime-db) as an apps/roam dependency.
  • Required now: Done When requires correct Roam syntax per media type, and the type set is not enumerable by hand: the extension table is derived from all ~1000 mime-db types (223 extensions) so an imported asset renders as the same file would if uploaded natively. Cost: db.json is not tree-shakeable, so ~204 KB lands in extension.js (23KB compressed); a build-time codegen of the table would remove it later.
  • Anyone affected or consulted: No.
  • Decision: Rationale recorded in the EXTENSION_KINDS docstring; no Linear decision.
    • I thought that maintaining mimetype information ourselves was inviting drift, and wanted to rely on a library.
    • Obsidian relies on mime-types already, so there is symmetry. mime-types imports path, which would not work in a browser build.

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.

https://linear.app/discourse-graphs/issue/ENG-2217/rewrite-asset-links-in-obsidian-origin-markdown-for-roam

@linear-code

linear-code Bot commented Sep 5, 2026

Copy link
Copy Markdown

ENG-2217

@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 10, 2026 10:06pm UTC

Request Review

@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 ↗︎.

@maparent
maparent force-pushed the eng-2217-rewrite-asset-links-in-obsidian-origin-markdown-for-roam branch from 1b550fd to 4ce0832 Compare September 6, 2026 14:38
@maparent
maparent force-pushed the eng-2217-rewrite-asset-links-in-obsidian-origin-markdown-for-roam branch from 4ce0832 to 2950a2a Compare September 6, 2026 15:00
@maparent
maparent force-pushed the eng-2217-rewrite-asset-links-in-obsidian-origin-markdown-for-roam branch from 2950a2a to 28e9eaa Compare September 6, 2026 16:01
@maparent
maparent force-pushed the eng-2217-rewrite-asset-links-in-obsidian-origin-markdown-for-roam branch from 28e9eaa to 7bb3b3d Compare September 6, 2026 16:37
@maparent
maparent force-pushed the eng-2217-rewrite-asset-links-in-obsidian-origin-markdown-for-roam branch from 7bb3b3d to 1179bdb Compare September 6, 2026 17:05
@maparent
maparent force-pushed the eng-2217-rewrite-asset-links-in-obsidian-origin-markdown-for-roam branch from 1179bdb to e0588dd Compare September 6, 2026 18:09
@maparent
maparent marked this pull request as ready for review September 6, 2026 20:09
@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

@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.

Note

This report is out of date. Scroll down for Devin Review's latest report on this PR.

Devin Review found 4 potential issues.

2 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)

Devin Review

Comment thread apps/roam/src/utils/rewriteAssetLinks.ts
Comment on lines +270 to +274
return markdown.replace(
LINK_PATTERN,
// One capture group per branch, in the order the pattern lists them. The trailing
// offset and input arguments the replacer also receives are simply not destructured.
(match: string, ...groups: (string | undefined)[]) => {

@devin-ai-integration devin-ai-integration Bot Sep 6, 2026

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.

🟡 Code examples become live assets

When markdown demonstrates a recorded asset inside code, replace rewrites that example too. The imported page no longer preserves its source text.

Learn more

Make rewriteAssetLinks syntax-aware so replacements occur only in markdown link and embed nodes, not inside inline code, fenced code blocks, or other literal markdown regions. A markdown parser is preferable to expanding the global regular expression. Add cases where the same sourceRef appears once as an actual embed and once inside inline and fenced code.

Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is accurate, but solving it expands the scope considerably. Proposing to punt.
Written as an issue candidate in Roam.

Comment thread apps/roam/src/utils/rewriteAssetLinks.ts Outdated
Comment thread apps/roam/src/utils/rewriteAssetLinks.ts Outdated
@maparent
maparent force-pushed the eng-2217-rewrite-asset-links-in-obsidian-origin-markdown-for-roam branch from e0588dd to 0f5a93e Compare September 6, 2026 22:32
@maparent
maparent force-pushed the eng-2217-rewrite-asset-links-in-obsidian-origin-markdown-for-roam branch from 0f5a93e to c1d603a Compare September 6, 2026 22:41
@maparent
maparent force-pushed the eng-2217-rewrite-asset-links-in-obsidian-origin-markdown-for-roam branch from ef15f98 to 349c0e0 Compare September 9, 2026 13:26
@maparent
maparent force-pushed the eng-2217-rewrite-asset-links-in-obsidian-origin-markdown-for-roam branch from 349c0e0 to 0f71a67 Compare September 9, 2026 15:58
@maparent
maparent force-pushed the eng-2217-rewrite-asset-links-in-obsidian-origin-markdown-for-roam branch from 0f71a67 to f225e53 Compare September 9, 2026 16:10
@maparent
maparent force-pushed the eng-2217-rewrite-asset-links-in-obsidian-origin-markdown-for-roam branch from f225e53 to d77dd2c Compare September 9, 2026 16:29
@maparent
maparent force-pushed the eng-2217-rewrite-asset-links-in-obsidian-origin-markdown-for-roam branch from d77dd2c to 0f834a4 Compare September 9, 2026 18:24
@maparent
maparent force-pushed the eng-2217-rewrite-asset-links-in-obsidian-origin-markdown-for-roam branch from 0f834a4 to e3c12d5 Compare September 9, 2026 18:37
@maparent
maparent force-pushed the eng-2217-rewrite-asset-links-in-obsidian-origin-markdown-for-roam branch from e3c12d5 to a37d0c0 Compare September 9, 2026 18:43
@maparent
maparent force-pushed the eng-2217-rewrite-asset-links-in-obsidian-origin-markdown-for-roam branch from a37d0c0 to b94dd69 Compare September 9, 2026 18:51
@maparent
maparent force-pushed the eng-2217-rewrite-asset-links-in-obsidian-origin-markdown-for-roam branch from b94dd69 to f2ce51a Compare September 9, 2026 19:13
graphite-app[bot]

This comment was marked as resolved.

@maparent

Copy link
Copy Markdown
Collaborator Author

Added two commits, to handle two more cases.
First two angle bracket forms:
// bracketed destination
https://storage.test/k.png // autolink
Second, make sure to capture labels of non-images:
![[vault/i.docx|a label]] // the new alias group

Those all fall into the original scope of "Find asset references in the markdown", and were just missed in first implementation.

@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 found 2 new potential issues.

2 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)

Devin Review

Comment on lines +348 to +349
// A wikilink embed carries no separate text, so its label comes from the asset.
linkText: imageLocator ? (imageAlt ?? "") : (linkLabel ?? wikiLabel ?? ""),

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.

🟡 Unaliased wikilinks become media embeds

parseMatch gives [[image.png]] no link text, so render treats the link like an embed. The imported page embeds media instead of preserving the source link.

Learn more

An Obsidian wikilink without an alias still has visible link text derived from its target. The parser currently represents that text as empty. The early link-preservation branch then does not run, and the resolved extension selects an image, PDF, audio, or video embed.

Example: [[attachments/diagram.png]] is a normal link to an image file. After resolution, it becomes ![](firebase-url) rather than a link such as diagram.png.

Recommended fix: Distinguish unaliased wikilinks from genuinely empty Markdown links. Supply their implicit display text to render, or add an explicit wikilink form whose media rendering always remains a link.

Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment on lines +365 to +366
return markdown.replace(
LINK_PATTERN,

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.

🟡 Code examples silently change

rewriteAssetLinks applies LINK_PATTERN inside fenced and inline code when a matching asset exists. Literal examples then contain destination URLs instead of the published text.

Learn more

Markdown code spans and fenced code blocks treat link syntax as literal text. A regex over the complete document cannot distinguish those regions from rendered links. If the same locator appears as a real asset elsewhere, the map contains a matching row and the literal occurrence changes too.

Example: A note embeds ![[diagram.png]] and later documents that syntax inside a fenced block. Both occurrences become Firebase syntax, although the fenced example must remain ![[diagram.png]].

Recommended fix: Parse the markdown into syntax nodes and rewrite only link, image, and supported embed nodes. If the existing regex remains, first segment the document with a Markdown-aware tokenizer and exclude code spans and fenced code blocks from replacement.

Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

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