From 0ab6c4f237b37adcea3d23e4b0ec2be1a163b44a Mon Sep 17 00:00:00 2001 From: PathGao Date: Mon, 3 Aug 2026 20:18:28 +0800 Subject: [PATCH] fix(tabs): clear the whole reading position when a tab changes document MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Following a link, and back/forward, keep the tab and swap the document under it. `navigate()` ended that with `tab.scrollTop = 0` and nothing else; `goBack`/`goForward` cleared nothing. Both restore paths are fallback cascades that stop at the first entry that resolves — the preview tries `anchorLine`, then `scrollPercentage`, then `scrollTop`, and the editor tries `editorViewState`, then `anchorLine`, then `scrollPercentage`. `scrollTop` is the entry each one consults LAST, so for any tab that had been scrolled the reset was unreachable and the tab restored the previous document's position. The three routes that repoint a tab now clear all four fields together through one private `clearReadingPosition`. Save As and rename change the path while the text on screen stays put, so they keep the position. The preview restore effect also gains a dependency on which document the active tab holds. It is the only thing that moves the preview to a tab's recorded position, and it ran on tab activation only — so before this the reset reached nothing at the moment of the navigation either, and the container kept the pixel offset the reader had in the previous document. Measured in the running frontend, scrolled 7000px into a 60-section document and following a link to a 40-section one: the new document opened at 7000px on "beta section 16 paragraph 9", and switching away and back restored 6623px from the old document's `anchorLine` of 361. Both are 0 after this change. Back and forward were the same: Back left the preview at 11000px in a document it had been at the top of. Co-Authored-By: Claude Opus 5 --- scripts/tabReadingPosition.test.ts | 293 +++++++++++++++++++++++++++++ src/lib/MarkdownViewer.svelte | 8 + src/lib/stores/tabs.svelte.ts | 39 +++- 3 files changed, 339 insertions(+), 1 deletion(-) create mode 100644 scripts/tabReadingPosition.test.ts 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;