diff --git a/scripts/tabReadingPosition.test.ts b/scripts/tabReadingPosition.test.ts new file mode 100644 index 0000000..9660044 --- /dev/null +++ b/scripts/tabReadingPosition.test.ts @@ -0,0 +1,293 @@ +/** + * A tab that is pointed at a different document must forget where the reader + * was in the old one. + * + * `Tab` records the reading position four times, and both restore paths are + * fallback cascades that stop at the first entry that resolves: the preview + * tries `anchorLine`, then `scrollPercentage`, then `scrollTop` + * (`MarkdownViewer.svelte:1133-1162`), and the editor tries `editorViewState`, + * then `anchorLine`, then `scrollPercentage` (`components/Editor.svelte:290-311`). + * + * `navigate()` used to end with `tab.scrollTop = 0` and nothing else, which is + * the entry each cascade consults LAST. For any tab that had actually been + * scrolled, that reset was unreachable, and the next activation of the tab + * restored the previous document's position into the new document. `goBack` and + * `goForward` repoint a tab the same way and cleared nothing at all. + * + * Two things are checked here, and neither reads an implementation file as text: + * + * - the REAL `TabManager`, driven through every route that changes which + * document a tab shows and every route that changes only its path; + * - the REAL `findAnchorElement` over the REAL `processMarkdownHtml` output of + * two DIFFERENT documents, which is what turns "the field is stale" into a + * rate: how often a line number carried over from document A resolves to + * some unrelated block of document B, so the cascade stops there and the + * later entries are never consulted. + */ + +import assert from 'node:assert/strict'; +import test from 'node:test'; + +import { installShimDom, parseHtml, type ShimElement } from './renderProtocolDom.ts'; + +// ---------------------------------------------------------------- environment + +const g = globalThis as any; +const runeEffect = (fn: () => void) => { + void fn; +}; +runeEffect.root = (fn: () => unknown) => fn(); +g.$state = (value: unknown) => value; +g.$state.raw = (value: unknown) => value; +g.$state.snapshot = (value: unknown) => value; +g.$derived = (value: unknown) => value; +g.$derived.by = (fn: () => unknown) => fn(); +g.$effect = runeEffect; +g.window = g.window ?? {}; +g.window.__TAURI_INTERNALS__ = g.window.__TAURI_INTERNALS__ ?? { + metadata: { currentWindow: { label: 'main' }, currentWebview: { windowLabel: 'main', label: 'main' } }, + invoke: (cmd: string) => Promise.resolve(cmd === 'get_os_type' ? 'macos' : null), +}; + +const localStore = new Map(); +g.localStorage = g.localStorage ?? { + getItem: (key: string) => (localStore.has(key) ? localStore.get(key)! : null), + setItem: (key: string, value: string) => void localStore.set(key, String(value)), + removeItem: (key: string) => void localStore.delete(key), + clear: () => localStore.clear(), +}; + +installShimDom(); + +const { tabManager } = await import('../src/lib/stores/tabs.svelte.js'); +const { processMarkdownHtml } = await import('../src/lib/utils/markdown.ts'); +const { findAnchorElement } = await import('../src/lib/utils/previewAnchor.ts'); + +type Tab = (typeof tabManager.tabs)[number]; + +function reset() { + tabManager.closeAll(); + tabManager.recentlyClosed.length = 0; + localStore.clear(); +} + +/** A tab whose reader is a long way down a document, all four fields written. */ +function openScrolledTab(path: string): Tab { + tabManager.addTab(path, '# doc\n'); + const tab = tabManager.tabs.find((item) => item.path === path)!; + tab.scrollTop = 4820; + tab.scrollPercentage = 0.73; + tab.anchorLine = 812; + tab.editorViewState = { cursorState: 'monaco-live-object' }; + return tab; +} + +/** Every field either restore cascade reads, in the order it reads them. */ +function readingPosition(tab: Tab) { + return { + editorViewState: tab.editorViewState, + anchorLine: tab.anchorLine, + scrollPercentage: tab.scrollPercentage, + scrollTop: tab.scrollTop, + }; +} + +const AT_TOP = { editorViewState: null, anchorLine: 0, scrollPercentage: 0, scrollTop: 0 }; + +// --------------------------------------------- the routes that change document + +test('following a link to another file leaves the tab at the top of it', () => { + reset(); + const tab = openScrolledTab('/notes/a.md'); + + tabManager.navigate(tab.id, '/notes/b.md'); + + assert.equal(tab.path, '/notes/b.md'); + assert.deepEqual(readingPosition(tab), AT_TOP); +}); + +test('going back to the previous file leaves the tab at the top of it', () => { + reset(); + const tab = openScrolledTab('/notes/a.md'); + tabManager.navigate(tab.id, '/notes/b.md'); + tab.anchorLine = 44; + tab.scrollPercentage = 0.2; + tab.scrollTop = 900; + tab.editorViewState = { cursorState: 'monaco-live-object' }; + + const back = tabManager.goBack(tab.id); + + assert.equal(back, '/notes/a.md'); + assert.equal(tab.path, '/notes/a.md'); + assert.deepEqual(readingPosition(tab), AT_TOP); +}); + +test('going forward again leaves the tab at the top of that file', () => { + reset(); + const tab = openScrolledTab('/notes/a.md'); + tabManager.navigate(tab.id, '/notes/b.md'); + tabManager.goBack(tab.id); + tab.anchorLine = 300; + tab.scrollPercentage = 0.5; + tab.scrollTop = 2000; + tab.editorViewState = { cursorState: 'monaco-live-object' }; + + const forward = tabManager.goForward(tab.id); + + assert.equal(forward, '/notes/b.md'); + assert.equal(tab.path, '/notes/b.md'); + assert.deepEqual(readingPosition(tab), AT_TOP); +}); + +// ------------------------------------- the routes that change only the path +// +// The document on screen does not change in either of these, so the reader has +// not moved and the position must survive. This is the half of the rule a +// blanket "clear on every path write" would break. + +test('Save As renames the tab without moving the reader', () => { + reset(); + const tab = openScrolledTab('/notes/a.md'); + const before = readingPosition(tab); + + tabManager.updateTabPath(tab.id, '/notes/copy.md'); + + assert.equal(tab.path, '/notes/copy.md'); + assert.deepEqual(readingPosition(tab), before); +}); + +test('renaming the file on disk does not move the reader', () => { + reset(); + const tab = openScrolledTab('/notes/a.md'); + const before = readingPosition(tab); + + tabManager.renameTab(tab.id, '/notes/renamed.md'); + + assert.equal(tab.path, '/notes/renamed.md'); + assert.deepEqual(readingPosition(tab), before); +}); + +test('a link that resolves to the file already open is not a navigation', () => { + reset(); + const tab = openScrolledTab('/notes/a.md'); + const before = readingPosition(tab); + + tabManager.navigate(tab.id, '/notes/a.md'); + + assert.deepEqual(readingPosition(tab), before); +}); + +/* ------------------------------------------------------------------ */ +/* what a carried-over anchor line does to the next document */ +/* ------------------------------------------------------------------ */ + +// comrak's shape, as recorded in renderProtocolFixtures.ts: every block carries +// a `data-sourcepos` line range, and `processMarkdownHtml` then re-parents +// everything after a heading into a wrapper that carries none. + +class DocumentBuilder { + private line = 1; + private readonly parts: string[] = []; + readonly lines: number[] = []; + + heading(text: string, slug: string) { + const line = this.line; + this.line += 2; + this.lines.push(line); + this.parts.push( + `

` + + `${text}

`, + ); + } + + paragraph(text: string) { + const line = this.line; + this.line += 2; + this.lines.push(line); + this.parts.push(`

${text}

`); + } + + html() { + return this.parts.join('\n') + '\n'; + } +} + +/** + * Two different documents. `paragraphsPerSection` is what makes them different + * documents rather than two renders of one: the same source line lands in a + * different section of each. + */ +function buildDocument(name: string, sections: number, paragraphsPerSection: number) { + const doc = new DocumentBuilder(); + for (let s = 1; s <= sections; s += 1) { + doc.heading(`${name} section ${s}`, `${name}-section-${s}`); + for (let p = 1; p <= paragraphsPerSection; p += 1) { + doc.paragraph(`${name} section ${s} paragraph ${p}, a sentence of prose.`); + } + } + return doc; +} + +const DOC_A = buildDocument('alpha', 60, 6); +const DOC_B = buildDocument('beta', 40, 11); + +function render(doc: DocumentBuilder, path: string): ShimElement { + return parseHtml(processMarkdownHtml(doc.html(), path, new Set())).body; +} + +const BODY_B = render(DOC_B, '/notes/b.md'); + +function textOf(element: unknown): string { + return String((element as { textContent?: string }).textContent ?? ''); +} + +test('a line number carried over from the previous document resolves into this one', (t) => { + // Every source line the reader could have been parked on in document A. + const carried = DOC_A.lines; + + let resolved = 0; + for (const line of carried) { + if (findAnchorElement(BODY_B, line)) resolved += 1; + } + const percent = Math.round((resolved / carried.length) * 1000) / 10; + + t.diagnostic(`document A: ${carried.length} anchorable source lines`); + t.diagnostic(`document B: ${BODY_B.querySelectorAll('[data-sourcepos]').length} blocks with data-sourcepos`); + t.diagnostic(`carried anchors resolving inside document B: ${resolved}/${carried.length} (${percent}%)`); + + // This is the mechanism, not a property of these two fixtures: while the + // carried line is inside the other document's range it resolves, the first + // cascade entry reports success, and `scrollPercentage` and `scrollTop` — + // the field the old `navigate()` reset — are never consulted. + assert.ok( + percent > 50, + `a stale anchor line is expected to resolve in a document of similar length, got ${percent}%`, + ); + + // And it resolves to the wrong text: the block at that line in B, which + // says `beta`, has nothing to do with the block the reader left in A. + const sample = DOC_A.lines[Math.floor(DOC_A.lines.length / 2)]; + const match = findAnchorElement(BODY_B, sample)!; + t.diagnostic(`line ${sample} of A resolves to B's ${textOf(match.element).trim().slice(0, 48)}`); + assert.ok(match, `expected line ${sample} to resolve inside document B`); + assert.match(textOf(match.element), /beta/); +}); + +test('a cleared reading position resolves to nothing, so the cascade falls through to the top', () => { + reset(); + const tab = openScrolledTab('/notes/a.md'); + tabManager.navigate(tab.id, '/notes/b.md'); + + // Stage 1 of the preview cascade is guarded on `anchorLine > 0`, and + // `findAnchorElement` refuses a non-positive line outright. Asserted as a + // boolean: a returned match holds a live DOM node whose parent chain the + // assertion printer would try to serialize. + assert.ok( + findAnchorElement(BODY_B, tab.anchorLine) === null, + 'a cleared anchorLine must not resolve to an element in the new document', + ); + // Stage 2 is guarded on `scrollPercentage > 0`, so the cascade reaches + // stage 3 — and stage 3 is the top of the document. + assert.equal(tab.scrollPercentage, 0); + assert.equal(tab.scrollTop, 0); +}); diff --git a/src/lib/MarkdownViewer.svelte b/src/lib/MarkdownViewer.svelte index ed4a6be..f44e41e 100644 --- a/src/lib/MarkdownViewer.svelte +++ b/src/lib/MarkdownViewer.svelte @@ -1123,6 +1123,14 @@ import { createDocumentSession, type LoadMarkdownOptions } from './sessions/docu // Depend on the ID and body existence to trigger restore const id = tabManager.activeTabId; const body = markdownBody; + // ...and on WHICH DOCUMENT that tab holds. Following a link, and + // back/forward, keep the tab and swap the document under it, and + // `clearReadingPosition` puts the tab at the top of the new one. This + // effect is the only thing that moves the preview to a tab's recorded + // position, so without this dependency that reset reaches nothing: the + // container keeps the pixel offset the reader had in the PREVIOUS + // document, and the new document opens part-way down. + void currentFile; if (id && body) { untrack(() => { diff --git a/src/lib/stores/tabs.svelte.ts b/src/lib/stores/tabs.svelte.ts index f6a8f0a..724c1c7 100644 --- a/src/lib/stores/tabs.svelte.ts +++ b/src/lib/stores/tabs.svelte.ts @@ -92,6 +92,10 @@ export interface Tab { * recently written is not the most precise, and one of them can be current * while its neighbours are not. None of them scrolls anything when * assigned: the restore runs on tab activation, not on write. + * + * Because a cascade stops at its first resolved entry, they can only be + * INVALIDATED as a set — pointing the tab at another document clears all of + * them, and `editorViewState`, through `clearReadingPosition`. */ scrollPercentage: number; anchorLine: number; @@ -785,6 +789,37 @@ 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. + * + * 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 + * (see `scrollPercentage` above): the preview tries `anchorLine`, then + * `scrollPercentage`, then `scrollTop`, and the editor tries + * `editorViewState`, then `anchorLine`, then `scrollPercentage`. So a reset + * of only the last entry is unreachable for any tab that had been scrolled, + * and the tab restores the OLD document's position — `anchorLine` is a + * source line, and the line the reader left in one file names an unrelated + * block in the next one. + * + * Nothing here scrolls anything; a restore runs on tab activation and on + * 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. + */ + private clearReadingPosition(tab: Tab) { + tab.editorViewState = null; + tab.anchorLine = 0; + tab.scrollPercentage = 0; + tab.scrollTop = 0; + } + navigate(id: string, path: string, pathKey?: string) { const tab = this.tabs.find(t => t.id === id); if (tab) { @@ -810,7 +845,7 @@ class TabManager { tab.pathKey = pathKey; tab.title = path.split(/[/\\]/).pop() || 'Untitled'; tab.isDirty = false; - tab.scrollTop = 0; + this.clearReadingPosition(tab); } } @@ -842,6 +877,7 @@ class TabManager { tab.pathKey = undefined; tab.title = path.split(/[/\\]/).pop() || 'Untitled'; tab.isDirty = false; + this.clearReadingPosition(tab); return path; } return null; @@ -860,6 +896,7 @@ class TabManager { tab.pathKey = undefined; tab.title = path.split(/[/\\]/).pop() || 'Untitled'; tab.isDirty = false; + this.clearReadingPosition(tab); return path; } return null;