feat(editor): resolve markdown images with note-relative or absolute paths#543
feat(editor): resolve markdown images with note-relative or absolute paths#543trapias wants to merge 3 commits into
Conversation
…paths Teach the editor to resolve standard markdown image embeds whose URL is a note-relative path (./foo.png, ../shared/x.png) or an absolute filesystem path, in addition to the already-supported attachments/ and wikilink forms. This is the shape Obsidian and most markdown editors emit by default. portableImageUrls now emits a note-relative path when the image is inside the vault but outside attachments/, so saved markdown stays portable across machines. URLs with a scheme (https:, data:, etc.) are explicitly guarded and passed through unchanged. The image regex is made non-greedy so URLs containing raw spaces match correctly when followed by an optional title. 14 new tests cover note-relative resolution, parent traversal, %-decoding, Windows separators, round-trip stability, and spaces.
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 16 |
| Duplication | 0 |
AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.
TIP This summary will be updated as you push new changes.
There was a problem hiding this comment.
Pull Request Overview
This PR successfully introduces support for note-relative and absolute image paths, improving compatibility with tools like Obsidian. However, there is a logic error in the path extraction utility that will cause incorrect image resolution for notes located at the root of the filesystem or vault.
Furthermore, the serialization logic in portableImageUrls returns null for non-relativizable assets, which could lead to data loss or broken links in the saved Markdown. There is also a lack of test coverage for absolute path resolution and remote scheme (HTTPS) pass-through. These issues should be addressed to ensure robust handling of filesystem-linked assets.
About this PR
- While the code contains logic for absolute path resolution and remote scheme guarding, these paths are not covered in
src/utils/vaultImages.test.ts. Please add unit tests for these cases.
Test suggestions
- Resolve a note-relative image path using './' prefix
- Resolve a note-relative image path using '../' for parent directory traversal
- Resolve a bare relative image path (no leading dot) against the note directory
- Resolve an absolute filesystem path
- Resolve a Markdown image URL containing spaces with and without a title
- Round-trip an image from a resolved asset URL back to a note-relative path
- Pass through remote HTTPS image URLs without modification
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Resolve an absolute filesystem path
2. Pass through remote HTTPS image URLs without modification
Low confidence findings
- The implementation treats paths starting with '/' as system-absolute. Note that in some Markdown editors (e.g., Obsidian), a leading slash can represent the vault root. If this tool aims for full Obsidian compatibility, consider if vault-root resolution should be supported.
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
| function noteDirectoryPath(notePath: NotePath): AbsolutePath { | ||
| const idx = Math.max(notePath.lastIndexOf('/'), notePath.lastIndexOf('\\')) | ||
| if (idx <= 0) return notePath | ||
| return notePath.slice(0, idx) | ||
| } |
There was a problem hiding this comment.
🔴 HIGH RISK
The noteDirectoryPath function incorrectly returns the full path of the note when a file is in the root directory (idx === 0). This causes subsequent path joining to append the image filename to the note's filename.
| function noteDirectoryPath(notePath: NotePath): AbsolutePath { | |
| const idx = Math.max(notePath.lastIndexOf('/'), notePath.lastIndexOf('\\')) | |
| if (idx <= 0) return notePath | |
| return notePath.slice(0, idx) | |
| } | |
| function noteDirectoryPath(notePath: NotePath): AbsolutePath { | |
| const idx = Math.max(notePath.lastIndexOf('/'), notePath.lastIndexOf('\\')) | |
| if (idx === -1) return '.' | |
| if (idx === 0) return notePath.charAt(0) | |
| return notePath.slice(0, idx) | |
| } |
There was a problem hiding this comment.
Fixed in da5f8f4. noteDirectoryPath now splits the three real cases explicitly: idx === -1 returns '.', idx === 0 returns the separator itself ('/' or '\\'), and idx > 0 returns the prefix slice. Two regression tests added in vaultImages.test.ts covering both edge cases (note at filesystem root, note path without separator).
| return rewriteMarkdownImages(markdown, (url) => { | ||
| if (!isAssetUrl(url)) return null | ||
|
|
||
| return currentVaultAttachmentPath(url, vaultPath) | ||
| const attachmentPath = currentVaultAttachmentPath(url, vaultPath) | ||
| if (attachmentPath) return attachmentPath | ||
|
|
||
| if (notePath) { | ||
| return noteRelativeFromAssetUrl(url, notePath) | ||
| } | ||
|
|
||
| return null | ||
| }) |
There was a problem hiding this comment.
🟡 MEDIUM RISK
Suggestion: To ensure the markdown file remains portable across different environments, URLs that cannot be made relative should be converted back to their absolute filesystem paths instead of returning null or keeping the internal asset:// scheme. Use decodeAssetPath(url) as a fallback.
There was a problem hiding this comment.
Fixed in da5f8f4. portableImageUrls now falls back to decodeAssetPath(url) when the asset URL cannot be mapped to attachments/ or to a note-relative form, so the internal asset://localhost scheme never survives into saved markdown. One existing test updated to assert the new unwrap-to-absolute-path behavior, two new tests added including a round-trip check.
…set URLs noteDirectoryPath previously returned the full notePath whenever the last separator sat at index 0 or was missing, causing path joins to treat the note's filename as a directory (e.g. /note.md + img.png became /note.md/img.png). Split the branch into the three real cases: no separator -> '.', leading-only separator -> the separator itself, otherwise the prefix. portableImageUrls now decodes asset URLs that cannot be mapped to attachments/ or to a note-relative form back to their absolute filesystem path, so the internal asset://localhost scheme never survives into saved markdown. Addresses Codacy review on PR refactoringhq#543.
|
Both Codacy findings addressed in da5f8f4: 🔴 HIGH RISK — Tests: |
|
Merged after some tweaks — closing this now. Thanks again for the contribution! |
|
Merged after some tweaks — closing this now. |
Problem
Markdown image embeds with note-relative paths (
,) or absolute paths () were not resolved by the renderer, leaving the embed broken. This is the default shape Obsidian and most markdown editors emit, so importing content from those tools left images visibly missing in Tolaria.Solution
Teach
vaultImages.tsto resolve these forms inresolveImageUrls, and to emit note-relative paths back inportableImageUrlsso saved markdown stays portable across machines. URLs with a scheme (https:,data:, …) are explicitly guarded so they keep flowing through to the browser unchanged.The note path is threaded from the active tab through
editorBlockResolution,editorRawModeSync, anduseEditorTabSwapso resolution has the context it needs.attachments/x.png![[x.png]](wikilink)./x.png,x.png../shared/x.png..traversal/Users/me/x.pnghttps://…,data:…The image regex was also made non-greedy (
[^)"]+?instead of[^)\s"]+) so URLs with raw spaces () match correctly when followed by an optional title.Files changed
src/utils/vaultImages.ts— resolution + reverse-mapping helpers (noteDirectoryPath,joinNoteRelativePath,relativeFromNoteDirectory,hasUrlScheme,isAbsolutePath)src/utils/vaultImages.test.ts— +14 test casessrc/hooks/editorBlockResolution.ts— threadnotePaththroughpreProcessEditorMarkdownsrc/components/editorRawModeSync.ts— thread the active tab path throughserializeEditorDocumentToMarkdownsrc/hooks/useEditorTabSwap.ts— pass note path toportableImageUrlson editor changesTest plan
pnpm vitest run src/utils/vaultImages.test.ts src/hooks/useEditorTabSwap.test.ts— 89/89 passblockNoteSuggestionWrapper.regression.test.tsx,blockNoteTableHandles.regression.test.ts) are pre-existing oneb6b58eband unrelated to this change (verified via stash-and-rerun)npx tsc --noEmitpassespnpm lintpassesand a real file next to it → image renders→ image renders, traversal honouredattachments/and round-trips correctlyreference → still loads from network, untouchedNotes
CONTRIBUTING.md.