fix(tabs): clear the whole reading position when a tab changes document

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<string, string>();
+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(
+			`<h2 data-sourcepos="${line}:1-${line}:${text.length + 3}">` +
+				`<a href="#${slug}" aria-hidden="true" class="anchor" id="${slug}"></a>${text}</h2>`,
+		);
+	}
+
+	paragraph(text: string) {
+		const line = this.line;
+		this.line += 2;
+		this.lines.push(line);
+		this.parts.push(`<p data-sourcepos="${line}:1-${line}:${text.length}">${text}</p>`);
+	}
+
+	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<string>())).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);
+});

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(() => {

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;