Skip to content

fix(preview): resolve relative images of imported documents correctly - #313

Merged
petertzy merged 5 commits into
petertzy:mainfrom
harsh-thakkar7:fix/openfile-converted-base-dir
Oct 2, 2026
Merged

petertzy merged 5 commits into
petertzy:mainfrom
harsh-thakkar7:fix/openfile-converted-base-dir

Conversation

@harsh-thakkar7

Copy link
Copy Markdown
Contributor

Summary

Opening an imported document (.html, .pdf, .docx) rendered its relative
images against the wrong folder
, so they were broken in the preview.

Root cause

openFile()'s conversion branch was the only refreshPreview call site that did
not pass a base directory:

const newTab = { ...makeTab(id, label, markdown, null, null), dirty: true };
setTabs((prev) => [...prev, newTab]);
setActiveTabId(id);
refreshPreview(markdown);          // <- no baseDirOverride

The fallback inside refreshPreview then used activeTab.filePath. But the
closure belongs to the render before the tab switch, and setActiveTabId
has not re-rendered yet — so it read the previously active tab's path.

Every other call site passes the path explicitly:

call site passes base dir
handleContentChange yes
openFile (already-open tab) yes
openFile (plain read) yes
openTextAsTab yes
openFile (conversion) no

Why it cannot just use filePath

A converted tab is created with filePath: null, on purpose — so Save asks for a
destination instead of overwriting the source .html. But its image paths are
relative to where that file lived.

The backend resolves them with fix_image_paths():

abs_path = os.path.abspath(os.path.join(base_path, src))

So the wrong base_dir produces concretely wrong file:// URLs. Verified
end-to-end from the real HTML conversion:

>>> md = convert_html_to_markdown('<img src="chart.png">')
'![chart](chart.png)'                      # relative path survives conversion
>>> fix_image_paths(md, "/work/notes")
'![chart](file:///work/notes/chart.png)'   # correct
>>> fix_image_paths(md, "/elsewhere")
'![chart](file:///elsewhere/chart.png)'    # broken: previous tab's folder

If the previously active tab was untitled, filePath is null, so base_dir
was undefined and the relative src resolved against the app origin inside
the sandboxed preview iframe — also broken.

Repro: keep /elsewhere/old.md active, then open
/work/notes/report.html which references chart.png sitting beside it. Every
relative image renders broken.

Changes

frontend/src/lib/preview-base-dir.mjs (new) — parentDirOf() and
resolvePreviewBaseDir(), holding the rule in a plain ESM module so the Node
regression suite can exercise the exact code the app runs (same pattern as
http-timeout.mjs).

frontend/src/hooks/useEditor.ts

  • Tab gains previewBaseDir, defaulting to the parent of filePath. For a
    converted tab it is set to the folder the source file came from.
  • refreshPreview's fallback consults previewBaseDir before filePath, so the
    correct directory survives later re-renders too — dark-mode toggles,
    font-size changes and every keystroke all re-render through the fallback.
  • The conversion branch passes the base dir explicitly for the first paint,
    before the tab switch has taken effect.

Behaviour for plain Markdown tabs is unchanged: previewBaseDir is just
parentDirOf(filePath).

I deliberately left the DOCX/PDF export paths (useFileIO.ts,
useActions.ts) alone. They derive base_dir the same way and so have the same
latent issue for converted documents, but that is a separate behaviour change and
this PR stays on the preview.

Testing

frontend/tests/preview-base-dir.test.mjs, 9 cases exercising the real helper:

  • parentDirOf for POSIX and Windows separators, root-level, and no-directory
  • an explicit override wins
  • a plain Markdown tab resolves against its own folder
  • a converted document resolves against the folder it was imported from
  • previewBaseDir wins when both are present
  • an untitled tab, null, and missing arguments return undefined
  • the result is a directory ending in a separator, never a file path

Verified 3 fail when the previewBaseDir branch is removed from
resolvePreviewBaseDir.

Verification

  • npm test — 32 passed (was 23 on main; this PR adds 9)
  • npm run lint — 0 errors, 0 warnings on the changed file
  • npm run build — clean
  • backend pytest — 195 passed, unchanged

openFile()'s conversion branch was the only refreshPreview call site that did
not pass a base directory, so the fallback read activeTab.filePath from the
closure created before setActiveTabId -- i.e. the previously active tab. Every
other call site (content change, already-open tab, plain read, openTextAsTab)
passes the path explicitly.

The backend resolves relative image sources with
os.path.abspath(os.path.join(base_dir, src)), so the wrong folder yields a
broken file:// URL. If the previous tab was untitled, base_dir was undefined
and the relative src resolved against the app origin in the sandboxed iframe.

A converted tab cannot simply use filePath: it is created with filePath null
on purpose so Save asks for a destination instead of overwriting the source
.html, yet its image paths still belong to that file's folder.

Add Tab.previewBaseDir, defaulting to the parent of filePath and set to the
source folder for converted documents, and consult it in refreshPreview's
fallback so the right directory survives later re-renders too (dark mode,
font size, every keystroke).

Export/docx export paths derive base_dir the same way and share the latent
issue, but that is a separate behaviour change; this stays on the preview.

Adds preview-base-dir.mjs plus 9 cases in tests/preview-base-dir.test.mjs;
verified 3 fail without the previewBaseDir branch.
@harsh-thakkar7
harsh-thakkar7 force-pushed the fix/openfile-converted-base-dir branch from bf77e52 to e35c92b Compare October 2, 2026 09:01
petertzy and others added 4 commits October 2, 2026 11:51
`next build` rejects `previewTab.previewBaseDir` on useEditor.ts:270 because
the `PreviewTab` JSDoc typedef in tabLifecycle.mjs lists only the fields the
helper itself reads. Tabs are handed back whole, so a field the *caller*
consumes has to be declared even though resolveTabClose never touches it.

Declare it optional (plain Markdown tabs have none) and note the pass-through
rule in the typedef's doc comment so the next field does not repeat the miss.

Adds a regression test covering the case that broke: closing a plain tab makes
a converted (filePath === null) tab active again, and its base directory has to
survive the hand-off or every relative image falls back to the app origin.
@petertzy

petertzy commented Oct 2, 2026

Copy link
Copy Markdown
Owner

Approved after follow-up fixes. The original change correctly preserves the source directory for imported HTML/PDF/DOCX documents and passes it explicitly during the initial preview render. I also fixed two related lifecycle cases: switching to a converted tab after closing another tab, and saving a converted document to a new path. These cases now preserve or update the preview base directory correctly. Frontend tests pass (48/48) and lint passes.

@petertzy
petertzy merged commit 3c32825 into petertzy:main Oct 2, 2026
2 checks passed
harsh-thakkar7 added a commit to harsh-thakkar7/markdown-reader that referenced this pull request Oct 2, 2026
`saveFile` captured `activeTab.content`, awaited `Files.write`, then applied a
patch built from that stale snapshot. `updateTab` merged the patch into whatever
the tab held *by then*, so `dirty: false` was stamped onto a buffer that had
advanced during the round trip — the title bar claimed "saved" while the edits
were still only in memory.

Settle the tab from the buffer observed after the write, and only clear `dirty`
when it still holds exactly what reached disk.

Two related changes fall out of this:

- The snapshot patch is replaced by a functional `setTabs`, so the comparison
  happens against live state. That removes `updateTab`'s last two call sites,
  making it dead: it is dropped rather than left as an unused helper.
- `previewBaseDir` is re-pointed at the destination folder when a save moves the
  document, preserving the behaviour from petertzy#313. It moves inside `settleSavedTab`
  instead of being inlined at the two call sites, so the rule stays covered by
  the Node suite instead of sitting untested in TypeScript.

frontend/tests/save-settle.test.mjs covers the compare-after-write rule and the
base-directory move; 6 of 6 mutants killed, including "previewBaseDir only
updated when clean" and "previewBaseDir never updated".
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.

2 participants