Skip to content

fix(tabs): forget a document's folds when the tab is pointed at another one - #448

Merged
PathGao merged 1 commit into
masterfrom
fix/navigate-leaves-stale-folds
Aug 3, 2026
Merged

fix(tabs): forget a document's folds when the tab is pointed at another one#448
PathGao merged 1 commit into
masterfrom
fix/navigate-leaves-stale-folds

Conversation

@PathGao

@PathGao PathGao commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

Fold ## Introduction, click a link to another file that also has an ## Introduction, and it opens with that section already shut — in a document you have never folded anything in.

This is the third instance of one shape. #425 found it between documents in different tabs and moved collapsedHeaders from the window onto the Tab. #447 found it in the scroll cascade, where navigate / goBack / goForward repoint a tab at a different document and carried the reader's position over; its "Not covered" section names this one and scopes it out. A tab is not a document, and the routes that change which document a tab holds are where the difference shows.

Rebased onto df80968 (#447). npm test 592/592, npm run check 638 files / 0 errors, npm run build clean, all measured on the rebased branch. No Rust touched.

Mechanism

Three routes keep a tab and point it at a different file: navigate() (following a Markdown link), goBack() and goForward(). None of them touched collapsedHeaders, and loadMarkdown renders the incoming document with the tab's set on the line straight after:

if (pendingNavigateTabId) tabManager.navigate(pendingNavigateTabId, filePath, pathKey);
const processed = await options.renderMarkdown(content, filePath, foldsForTab(activeId));

foldsForTab returns tab.collapsedHeaders — which at that instant still holds the keys of the document being left.

Nothing is deferred. Unlike #447, this needs no restore effect: processMarkdownHtml writes is-collapsed onto the heading and its wrapper while it builds the HTML, so the very first frame of the new document has the section shut. The outline reads the same field through the viewer's $derived, so it hides that section's children on the same render.

The key collides because it is a slug. h.id || h.textContent?.trim() is unique within a document and nowhere else. Measured over the 202 real Markdown files in this checkout with ≥3 headings (the repo's own, plus its dependencies' READMEs and changelogs):

ordered document pairs where ≥1 heading slug of A names a heading in B 27.9% (55520 / 199347 sampled)
individual carried keys landing on a heading in B 3.4% (114074 / 3401213)
documents containing license 31.7%
documents containing installation 29.7%
documents containing usage 27.7%

So a reader who folds ## Installation in one README and follows a link has roughly a 1-in-3 chance of the next one arriving with its Installation section hidden.

Measured in the running frontend

Driven through the real Svelte frontend in a browser against a stubbed Tauri bridge (render_markdown emitting comrak's shape, two in-memory documents that share ## Introduction with a ### Detail under it). The number is the fold wrapper's measured height — 0px is a genuinely hidden section, not just a class. Both columns re-measured after the rebase, against df80968 and against this branch.

step before (df80968) after
fold ## Introduction in a.md 0px, collapsed same
follow the link to b.md 0px — b.md opens with the section shut 137px, open
click b.md's chevron once 137px — the click reads as an unfold of a section the reader never folded 0px, folds it
Back to a.md 137px — the fold the reader did make in a.md is gone, spent on b.md 137px, open
Forward to b.md 0px 137px, open

A second run, folding only in b.md and then pressing Back, lands on a.md at 0px — the same leak in the other direction, this time with the folded tab's own document nowhere on screen.

Both symptoms in the "before" column come from one Set: because the two documents share the key, a toggle in either one moves the fold for both, so the leak does not only hide text the reader did not hide — it also loses folds they did make. #447 changed nothing here; the two tables measured before and after that merge are identical.

The fix, and the shape

A private clearCollapsedHeaders(tab) that replaces the set. The interesting decision is where it is called from.

The mechanical rebase onto #447 gave each of navigate / goBack / goForward two calls — clearReadingPosition and clearCollapsedHeaders. That is the shape #447 set out to remove: three routes that must each remember two things, and a fourth route later that must remember both. This repo has been bitten by it twice recently (#436: two of five explicit saves remembered to cancel the auto-save timer; #439: six sites cleared one flag while two cleared the other).

So the three routes now call one forgetPreviousDocument(tab), and that calls both helpers:

private forgetPreviousDocument(tab: Tab) {
	this.clearReadingPosition(tab);
	this.clearCollapsedHeaders(tab);
}

The helpers are not merged. A stale reading position moves the viewport; a stale fold hides text. They invalidate for different reasons, they fail differently, and each earns its own doc comment. What they share is the trigger, and the tell that the trigger was an unnamed concept is that both helpers' comments opened by restating it in slightly different words. forgetPreviousDocument is that name, and it is now the single place stating which routes reach it and which deliberately do not.

Replaces rather than empties. Tab.collapsedHeaders is documented as replace-only: the viewer holds the Set through a $derived, and Svelte cannot see a .clear() of a Set it is already holding — the outline would keep hiding the section. Tested.

Save As and rename must not get there. updateTabPath and renameTab change the tab's path while the text on screen stays put, so the reader has not moved and the folds they put in that text still describe it. Every path-changing site in TabManager was checked; those two are the only others.

Cross-window transfer is unchanged. tabTransfer.ts already excludes collapsedHeaders deliberately (#425) — the destination re-renders from source with everything open, which is already the cleared state.

Is that now the whole set?

With folds added, forgetPreviousDocument clears editorViewState, anchorLine, scrollPercentage, scrollTop, collapsedHeaders. Every other Tab field was classified:

category fields
cleared at the trigger the five above
written by the routes themselves path, pathKey, title, isDirty, history, historyIndex
overwritten by the load that follows, never stale content, rawContent, originalContent, isTruncated (setTabRawContent), hasReplacementChars (setTabDecodedLossy, called on both load branches and documented as clearing)
per tab / per user, not per document isEditing, isSplit, splitRatio, isScrollSynced

No sixth field of the same shape. One near-miss, checked and left alone: _lastRenderedRawContent, an ad-hoc property compared against rawContent to skip a redundant render. It does hold the previous document's text after a navigate, but a stale value can only skip a render when the two documents are byte-identical — in which case the render would be identical anyway.

Tests

Eight tests added to scripts/foldStatePerDocument.test.ts — the file that owns this rule — driving the real TabManager, the real processMarkdownHtml, the real toggleFold out of MarkdownViewer.svelte, the real visibleItems out of Toc.svelte, and now the real foldsForTab plucked out of documentSession.svelte.ts, which is the reader that decides what the incoming HTML arrives with. No file is asserted on as text.

test what can go wrong
following a link renders the new document with its own fold state the set travels through navigate
the table of contents shows the linked document's children the outline reads the same field and hides them
going back renders the previous document with its own fold state goBack clears nothing
going forward renders that document with its own fold state goForward clears nothing
the tab gets a new set rather than the old one emptied an in-place .clear() the $derived cannot see
Save As keeps the folds, because the document on screen has not changed a blanket "clear on every path write"
renaming the file on disk keeps the folds same
a link that resolves to the file already open is not a navigation the tab.path === path early return

Red on df80968: 5 fail / 10 pass, with #447's own tabReadingPosition.test.ts at 8/8 on the same source. The three of mine that pass on master are the guards — they must be green on both sides or they are not guarding anything.

One draft of the back/forward tests passed on master for the wrong reason: the setup folded in a.md first, so toggleFold in b.md found the carried key and removed it, leaving an empty set that satisfied the assertion. They now fold only in the document on screen and assert the precondition.

Mutation check

Each mutation applied alone on top of the fix, on df80968, run against both suites — including the two new ways a single entry point can break, which is dropping a call from inside it:

mutation fold tests #447's tests
baseline 15 pass 8 pass
clearReadingPosition dropped inside forgetPreviousDocument 15 pass 4 fail
clearCollapsedHeaders dropped inside forgetPreviousDocument 5 fail 8 pass
entry point call removed from navigate 3 fail 2 fail
entry point call removed from goBack 1 fail 1 fail
entry point call removed from goForward 1 fail 1 fail
fold set emptied in place instead of replaced 1 fail 8 pass
entry point also called from updateTabPath (over-reach) 1 fail 1 fail
entry point also called from renameTab (over-reach) 1 fail 1 fail

The first two rows are the point of the table: each helper is covered by exactly one suite, so the entry point cannot quietly lose either half. The rest fail in both, which is what a shared entry point should do.

Not covered

Forward no longer restores a fold made before you went Back. In the table above, Forward to b.md used to land at 0px because the key was still in the tab's set; it now opens flush. That is the same call the fix makes everywhere else — the tab's set describes the document it is leaving, not the one being restored — and it matches #447, which clears the reading position on back/forward too. Per-document fold history would need folds keyed by document, which #425 argued against for persistence for the same reason: a fold key describes a heading in a particular revision of a file.

The outline was not measured in the browser. The overlay renders at zero height in the headless viewport, so the live ToC could not be read. Its evidence is the real visibleItems in the test, plus #425 having established it reads the same field. The preview measurements above are real.

Stale keys still accumulate in a tab that stays on one document while its headings are renamed — pre-existing, listed in #425's own "Not covered", untouched here.

No cargo test. No Rust in the diff.

Severity. A hidden section is quieter than a lost scroll position: the text is simply not there, the chevron looks the same as any other collapsed heading, and the first click on it opens a section instead of closing one — which is the moment the reader learns their fold in the previous document is also gone.

🤖 Generated with Claude Code

…er one

#425 gave each document its own heading-fold state by moving the set from
the window onto the tab. A tab is not a document, though: `navigate`
(following a Markdown link), `goBack` and `goForward` keep the tab and
swap the file under it, and `collapsedHeaders` travelled with it.

A fold key is a heading slug, unique only within a document, so the
incoming file was rendered with any section whose slug happened to match
already shut. `loadMarkdown` reads the set on the line after the
`navigate` call, so nothing is deferred — the HTML that first appears
carries `is-collapsed`, and the outline hides the same section's children
on the same render, with nothing on screen to explain either.

Measured over 202 real Markdown documents (this repo and its
dependencies' READMEs and changelogs): for 27.9% of ordered document
pairs, at least one heading slug of the first names a heading in the
second. `installation` occurs in 29.7% of them, `usage` in 27.7%.

#447 established that the same three routes must clear the reading
position, and left folds for a separate change. Rather than have each
route remember two resets — the shape #436 and #439 were both about —
those routes now call one `forgetPreviousDocument(tab)`, which calls
#447's `clearReadingPosition` and a new `clearCollapsedHeaders`. The two
helpers stay separate: a stale position moves the viewport, a stale fold
hides text, and each needs its own explanation. What they shared was the
trigger, which had no name until now.

Save As (`updateTabPath`) and rename (`renameTab`) change the path while
the text on screen stays put, so they do not get there. Tests guard both
directions.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@PathGao
PathGao force-pushed the fix/navigate-leaves-stale-folds branch from 35631c7 to 4153c79 Compare August 3, 2026 13:10
@PathGao
PathGao merged commit 5be7c18 into master Aug 3, 2026
4 checks passed
@PathGao
PathGao deleted the fix/navigate-leaves-stale-folds branch August 3, 2026 13:36
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