diff --git a/scripts/foldStatePerDocument.test.ts b/scripts/foldStatePerDocument.test.ts index d07cb45..5c65779 100644 --- a/scripts/foldStatePerDocument.test.ts +++ b/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; +})(); + 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); +}); diff --git a/src/lib/stores/tabs.svelte.ts b/src/lib/stores/tabs.svelte.ts index 724c1c7..d6e0855 100644 --- a/src/lib/stores/tabs.svelte.ts +++ b/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(); + } + 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;