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

scripts/foldStatePerDocument.test.ts

@@ -10,6 +10,12 @@
  * to contain that heading opened with the section already shut and nothing on
  * screen to explain it.
  *
+ * Per tab is not the same as per document. A tab can be repointed at another
+ * file without a tab switch — following a link, and back/forward — and the set
+ * used to travel with it, so the same collision reappeared one route over. The
+ * second half of this file drives those three routes, and the routes that
+ * change only the path and must leave the folds alone.
+ *
  * These tests run the REAL `toggleFold` and `handleLinkClick` out of
  * MarkdownViewer.svelte, the REAL `visibleItems` out of Toc.svelte and the REAL
  * `processMarkdownHtml`, against the REAL TabManager. Nothing here asserts on
@@ -63,6 +69,7 @@ const { processMarkdownHtml } = await import('../src/lib/utils/markdown.ts');
 
 const viewer = readFileSync(new URL('../src/lib/MarkdownViewer.svelte', import.meta.url), 'utf8');
 const toc = readFileSync(new URL('../src/lib/components/Toc.svelte', import.meta.url), 'utf8');
+const session = readFileSync(new URL('../src/lib/sessions/documentSession.svelte.ts', import.meta.url), 'utf8');
 
 // ------------------------------------------------------------ source plucking
 
@@ -256,6 +263,25 @@ function buildTocFilter() {
 
 const tocVisibleItems = buildTocFilter();
 
+/**
+ * The real `foldsForTab` out of documentSession — the set the LOAD path hands
+ * the renderer, looked up at render time.
+ *
+ * The viewer's `collapsedHeaders` derived (above) is what the outline and the
+ * live preview DOM read; this is what decides which sections the incoming
+ * HTML is built with `is-collapsed` already on. They are different readers of
+ * the same field and both are exercised below, because a fold leak shows up in
+ * each of them separately.
+ */
+const foldsForTab = (() => {
+	const js = ts.transpileModule(pluckFunction(session, 'foldsForTab'), {
+		compilerOptions: { target: ts.ScriptTarget.ES2022 },
+	}).outputText;
+	return new Function('tabManager', `"use strict";\n${js}\nreturn foldsForTab;`)(tabManager) as (
+		tabId: string,
+	) => Set<string>;
+})();
+
 const TOC_ITEMS = [
 	{ id: FOLD_KEY, text: 'Introduction', level: 2, isBlock: false, hasChildren: true },
 	{ id: 'details', text: 'Details', level: 3, isBlock: false, hasChildren: false },
@@ -407,3 +433,156 @@ test('closing a tab takes its fold state with it', () => {
 	tabManager.addTab('/notes/a.md', DOC_A);
 	assert.equal(app.foldsOnScreen().has(FOLD_KEY), false, 'reopening the file starts fresh');
 });
+
+// ------------------------------------------- the routes that swap the document
+//
+// A tab does not only change document when the user switches to another tab.
+// Three routes keep the tab and point it at a DIFFERENT file: `navigate`
+// (following a Markdown link), `goBack` and `goForward`. `loadMarkdown` then
+// renders the incoming file with `foldsForTab(activeId)` on the very next line
+// (`documentSession.svelte.ts`), so the fold keys of the document the reader
+// just LEFT decide which sections of the new one arrive shut.
+//
+// Nothing is deferred here: the HTML that first appears already carries
+// `is-collapsed`, and the outline hides the same section's children on the same
+// render. #425 gave the set a per-tab home, which fixed the tab-switch route;
+// this is the same key collision reached through the navigation route. #447
+// clears the reading position at these three sites for the same reason and
+// scoped folds out of it — a position moves the viewport, a fold hides text.
+
+/** Fold a heading in a.md, then follow a link to b.md, which shares the heading. */
+function foldInAThenFollowLinkTo(path: string) {
+	reset();
+	const app = buildViewer();
+
+	tabManager.addTab('/notes/a.md', DOC_A);
+	const id = tabManager.activeTabId!;
+	app.showDocument(render(DOC_A, '/notes/a.md', app.foldsOnScreen()));
+	app.toggleFold(FOLD_KEY);
+	assert.equal(app.foldsOnScreen().has(FOLD_KEY), true, 'precondition: a.md has the section folded');
+
+	tabManager.navigate(id, path);
+	return { app, id };
+}
+
+test('following a link renders the new document with its own fold state', () => {
+	const { app, id } = foldInAThenFollowLinkTo('/notes/b.md');
+
+	const onScreen = render(DOC_B, '/notes/b.md', foldsForTab(id));
+	assert.equal(
+		isSectionCollapsed(onScreen),
+		false,
+		'the document the link led to must render with its own fold state, which is none',
+	);
+	assert.match(onScreen.textContent, /Beta body\./);
+	assert.equal(app.foldsOnScreen().has(FOLD_KEY), false);
+});
+
+test("the table of contents shows the linked document's children", () => {
+	// The outline reads the same field through the viewer's derived, so it hides
+	// the section's children in a document the user never folded anything in.
+	const { app } = foldInAThenFollowLinkTo('/notes/b.md');
+
+	assert.deepEqual(
+		tocVisibleItems(TOC_ITEMS, app.foldsOnScreen()).map((item) => item.id),
+		[FOLD_KEY, 'details'],
+		'the outline must list the linked document’s own headings',
+	);
+});
+
+/**
+ * Land on `path` with the section folded in the document currently on screen —
+ * and nothing folded before that, so the assertion cannot be satisfied by
+ * `toggleFold` merely undoing a fold that was carried in.
+ */
+function foldHereThen(app: Viewer, id: string, html: string, path: string) {
+	app.showDocument(render(html, path, foldsForTab(id)));
+	app.toggleFold(FOLD_KEY);
+	assert.equal(app.foldsOnScreen().has(FOLD_KEY), true, `precondition: ${path} has the section folded`);
+}
+
+test('going back renders the previous document with its own fold state', () => {
+	reset();
+	const app = buildViewer();
+	tabManager.addTab('/notes/a.md', DOC_A);
+	const id = tabManager.activeTabId!;
+	tabManager.navigate(id, '/notes/b.md');
+	foldHereThen(app, id, DOC_B, '/notes/b.md');
+
+	assert.equal(tabManager.goBack(id), '/notes/a.md');
+	assert.equal(isSectionCollapsed(render(DOC_A, '/notes/a.md', foldsForTab(id))), false);
+});
+
+test('going forward renders that document with its own fold state', () => {
+	reset();
+	const app = buildViewer();
+	tabManager.addTab('/notes/a.md', DOC_A);
+	const id = tabManager.activeTabId!;
+	tabManager.navigate(id, '/notes/b.md');
+	tabManager.goBack(id);
+	foldHereThen(app, id, DOC_A, '/notes/a.md');
+
+	assert.equal(tabManager.goForward(id), '/notes/b.md');
+	assert.equal(isSectionCollapsed(render(DOC_B, '/notes/b.md', foldsForTab(id))), false);
+});
+
+test('the tab gets a new set rather than the old one emptied', () => {
+	// `Tab.collapsedHeaders` is replaced, never mutated: the viewer's derived
+	// holds the Set itself, and Svelte cannot see a `.clear()` of a Set it is
+	// already holding — the outline would keep hiding the section.
+	reset();
+	tabManager.addTab('/notes/a.md', DOC_A);
+	const id = tabManager.activeTabId!;
+	const before = foldsForTab(id);
+
+	tabManager.navigate(id, '/notes/b.md');
+
+	assert.notEqual(foldsForTab(id), before, 'the navigation must install a different Set');
+});
+
+// ------------------------------------ the routes that change only the path
+//
+// The text on screen does not change in any of these, so the folds the reader
+// put there still describe it. This is the half of the rule a blanket "clear on
+// every path write" would break — the same guard #447 keeps for the position.
+
+test('Save As keeps the folds, because the document on screen has not changed', () => {
+	reset();
+	const app = buildViewer();
+	tabManager.addTab('/notes/a.md', DOC_A);
+	const id = tabManager.activeTabId!;
+	app.showDocument(render(DOC_A, '/notes/a.md', app.foldsOnScreen()));
+	app.toggleFold(FOLD_KEY);
+
+	tabManager.updateTabPath(id, '/notes/copy.md');
+
+	assert.equal(tabManager.tabs.find((tab) => tab.id === id)!.path, '/notes/copy.md');
+	assert.equal(isSectionCollapsed(render(DOC_A, '/notes/copy.md', foldsForTab(id))), true);
+});
+
+test('renaming the file on disk keeps the folds', () => {
+	reset();
+	const app = buildViewer();
+	tabManager.addTab('/notes/a.md', DOC_A);
+	const id = tabManager.activeTabId!;
+	app.showDocument(render(DOC_A, '/notes/a.md', app.foldsOnScreen()));
+	app.toggleFold(FOLD_KEY);
+
+	tabManager.renameTab(id, '/notes/renamed.md');
+
+	assert.equal(tabManager.tabs.find((tab) => tab.id === id)!.path, '/notes/renamed.md');
+	assert.equal(isSectionCollapsed(render(DOC_A, '/notes/renamed.md', foldsForTab(id))), true);
+});
+
+test('a link that resolves to the file already open is not a navigation', () => {
+	reset();
+	const app = buildViewer();
+	tabManager.addTab('/notes/a.md', DOC_A);
+	const id = tabManager.activeTabId!;
+	app.showDocument(render(DOC_A, '/notes/a.md', app.foldsOnScreen()));
+	app.toggleFold(FOLD_KEY);
+
+	tabManager.navigate(id, '/notes/a.md');
+
+	assert.equal(isSectionCollapsed(render(DOC_A, '/notes/a.md', foldsForTab(id))), true);
+});

src/lib/stores/tabs.svelte.ts

@@ -118,6 +118,11 @@ export interface Tab {
 	 *
 	 * A tab field also covers untitled buffers, which have no path to key by.
 	 *
+	 * Per tab is not the same as per document, because a tab can be repointed at
+	 * another file without a tab switch — following a link, and back/forward.
+	 * Those routes clear this set, together with the reading position above; see
+	 * `forgetPreviousDocument`.
+	 *
 	 * Replace the set to change it — never mutate in place. The preview render
 	 * reads this value, and Svelte cannot see a mutation of a Set it is already
 	 * holding.
@@ -790,10 +795,36 @@ class TabManager {
 	}
 
 	/**
-	 * This tab is about to show a DIFFERENT document, so everything that says
-	 * where the reader was in the old one is void. The three routes that do
-	 * that — `navigate` (following a link), `goBack` and `goForward` — all end
-	 * here, because the fields have to be cleared as a set.
+	 * This tab now shows a DIFFERENT document, so everything the TAB records
+	 * about the one it was showing is void. The three routes that repoint a tab
+	 * — `navigate` (following a link), `goBack` and `goForward` — call this and
+	 * nothing else, so a fourth route has one question to answer ("does this
+	 * change which document the tab holds?") rather than one per field group.
+	 *
+	 * The two helpers stay separate because they are separate concerns with
+	 * separate reasons: a stale reading position moves the viewport, a stale
+	 * fold hides text, and each needs its own explanation. What they share is
+	 * this trigger — both doc comments below used to open by restating it, which
+	 * is what an unnamed shared concept looks like. This is its name.
+	 *
+	 * Not the buffer: `rawContent`, `originalContent` and `content` are
+	 * overwritten by the load that follows, not cleared here. This is only the
+	 * state that describes a document without being it.
+	 *
+	 * Only a change of DOCUMENT gets here. Save As and rename (`updateTabPath`,
+	 * `renameTab`) change the tab's path while the text on screen stays put: the
+	 * reader has not moved, and the folds they put in that text still describe
+	 * it. Tests guard both directions.
+	 */
+	private forgetPreviousDocument(tab: Tab) {
+		this.clearReadingPosition(tab);
+		this.clearCollapsedHeaders(tab);
+	}
+
+	/**
+	 * Everything that says where the reader was in the document this tab has
+	 * just left. Called only from `forgetPreviousDocument`; the fields have to
+	 * be cleared as a set.
 	 *
 	 * Clearing one of them is not enough and reads as if it were. Both restore
 	 * paths are fallback cascades that stop at the first entry that resolves
@@ -809,9 +840,8 @@ class TabManager {
 	 * editor mount, which is why the symptom of getting this wrong shows up on
 	 * a later tab switch rather than at the moment of the navigation.
 	 *
-	 * Only a change of DOCUMENT clears them. Save As and rename
-	 * (`updateTabPath`, `renameTab`) change the tab's path while the text on
-	 * screen stays put, and the reader has not moved.
+	 * Which routes reach here, and which deliberately do not, is stated once in
+	 * `forgetPreviousDocument` rather than in each helper.
 	 */
 	private clearReadingPosition(tab: Tab) {
 		tab.editorViewState = null;
@@ -820,6 +850,27 @@ class TabManager {
 		tab.scrollTop = 0;
 	}
 
+	/**
+	 * The folded-heading set of the document this tab has just left. Called only
+	 * from `forgetPreviousDocument`.
+	 *
+	 * A fold key is a heading slug (see `Tab.collapsedHeaders`), which is only
+	 * unique WITHIN a document, so carrying the set across means the incoming
+	 * document is rendered with any section whose slug happens to match already
+	 * shut. `loadMarkdown` reads the set on the line after the `navigate` call,
+	 * so nothing is deferred: the HTML that first appears already carries
+	 * `is-collapsed`, and the outline hides the same section's children on the
+	 * same render, with nothing on screen to explain either. That is #425's
+	 * failure reached through the navigation route instead of a window-wide set.
+	 *
+	 * Replaces the set rather than emptying it, as every other writer does — the
+	 * viewer holds it through a `$derived`, and Svelte cannot see a `.clear()`
+	 * of a Set it is already holding.
+	 */
+	private clearCollapsedHeaders(tab: Tab) {
+		tab.collapsedHeaders = new Set<string>();
+	}
+
 	navigate(id: string, path: string, pathKey?: string) {
 		const tab = this.tabs.find(t => t.id === id);
 		if (tab) {
@@ -845,7 +896,7 @@ class TabManager {
 			tab.pathKey = pathKey;
 			tab.title = path.split(/[/\\]/).pop() || 'Untitled';
 			tab.isDirty = false;
-			this.clearReadingPosition(tab);
+			this.forgetPreviousDocument(tab);
 		}
 	}
 
@@ -877,7 +928,7 @@ class TabManager {
 			tab.pathKey = undefined;
 			tab.title = path.split(/[/\\]/).pop() || 'Untitled';
 			tab.isDirty = false;
-			this.clearReadingPosition(tab);
+			this.forgetPreviousDocument(tab);
 			return path;
 		}
 		return null;
@@ -896,7 +947,7 @@ class TabManager {
 			tab.pathKey = undefined;
 			tab.title = path.split(/[/\\]/).pop() || 'Untitled';
 			tab.isDirty = false;
-			this.clearReadingPosition(tab);
+			this.forgetPreviousDocument(tab);
 			return path;
 		}
 		return null;