test(editor): cover the vault-path race the image resolver hit - #911
Merged
Conversation
#910 fixed the race where BlockNote's one-and-only `resolveFileUrl` call ran before the vault path had loaded, leaving the image broken for the life of the block. That fix shipped without a test, because driving BlockNote's image-block DOM construction in jsdom is disproportionate. Extract the callback into `createNoteFileUrlResolver` so the behaviour is testable on its own, and pin the three properties that matter: it waits for a vault path that has not arrived yet, it looks the vault up once however many images a note has, and a failed lookup degrades to leaving the URL alone rather than throwing inside the editor. Verified by mutation — restoring the pre-#910 "read whatever has loaded" shape fails three of the five cases.
|
React Doctor found no new issues. 🎉 Reviewed by React Doctor for commit |
There was a problem hiding this comment.
Pull request overview
Adds targeted renderer-unit coverage for the vault-path resolution race that previously broke note-relative images, by extracting the BlockNote resolveFileUrl callback into a testable helper and wiring it into the editor.
Changes:
- Introduces
createNoteFileUrlResolverto encapsulate (and cache) the async vault-path lookup used for resolving note-relative asset URLs. - Adds a focused Vitest suite that pins the “await vault path”, “single lookup”, and “degrade gracefully on lookup failure” behaviors (plus two note-path boundary cases).
- Updates
ContentAreato use the new resolver factory instead of inlining the logic.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| apps/desktop/src/renderer/src/lib/create-note-file-url-resolver.ts | New helper to build a cached, async resolveFileUrl callback that waits for the vault path before resolving note-relative URLs. |
| apps/desktop/src/renderer/src/lib/create-note-file-url-resolver.test.ts | New unit tests covering the vault-path race, caching behavior, and failure/boundary handling. |
| apps/desktop/src/renderer/src/components/note/content-area/ContentArea.tsx | Wires the editor’s resolveFileUrl to the new helper and removes the inline implementation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+223
to
+228
| const resolveFileUrl = useRef( | ||
| createNoteFileUrlResolver( | ||
| () => notePathRef.current, | ||
| async () => (await vaultService.getStatus()).path ?? null | ||
| ) | ||
| ).current |
Comment on lines
+26
to
+31
| return async (url: string) => { | ||
| const notePath = getNotePath() | ||
| if (!notePath) return url | ||
| if (!vaultPath) vaultPath = fetchVaultPath().catch(() => null) | ||
| return resolveNoteRelativeUrl(url, notePath, await vaultPath) | ||
| } |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
#910 fixed a real race: BlockNote calls
resolveFileUrlexactly once while building an image block's DOM, and #907 read the vault path fromuseVault(), which starts atnulland fills in after an IPC round-trip. The editor mount won that race, so the resolver saw a null vault path, correctly left the ref alone, and the image stayed broken for the life of the block.That fix shipped without a test — I said so in #910, because driving BlockNote's image-block DOM construction in jsdom is disproportionate for a change that size. This closes that gap without the jsdom cost.
The callback moves into
createNoteFileUrlResolver, which takes the note path and the vault lookup as plain functions.ContentAreawires the real ones; the test wires a vault lookup that has deliberately not answered yet. Behaviour is unchanged.Three properties are now pinned:
Plus two boundary cases: no note path yet (no vault lookup fired at all), and a note path that arrives after mount.
Reviewer notes
docs:impactwas skipped for this branch (MEMRY_DOCS_IMPACT_SKIP=1). Nothing user-visible changes — the behaviour this describes is already documented under "Images From Another App's Vault" inapps/docs/src/user-guide/notes/attachments.md, added by fix(editor): render note-relative image paths against the vault #907 and refined by fix(editor): treat backslashes as separators when resolving image paths #908.Release note
none
Test plan
pnpm --filter @memry/desktop test:renderer— 518 files, 5672 passed, 0 failedpnpm --filter @memry/desktop typecheck:webeslint --no-cache --max-warnings=0on both changed source files