fix(recents): merge the recent-files list instead of overwriting it

scripts/recentFilesMultiWindow.test.ts

@@ -0,0 +1,225 @@
+import assert from 'node:assert/strict';
+import { readFileSync } from 'node:fs';
+import test from 'node:test';
+
+/*
+ * The recent-file list was written back as `JSON.stringify(recentFiles)` from
+ * the window's own in-memory copy, in three places, with no re-read and no
+ * `storage` listener. Markpad opens several windows; each is a separate webview
+ * with its own copy of that array and all of them share one localStorage. So
+ * every write published a snapshot taken whenever that window last looked, and
+ * the later write erased the other window's entries. There was no ordering in
+ * which both survived.
+ *
+ * The fix is the one #370 established for settings: re-read before writing, and
+ * let a `storage` listener fold in what the siblings did. This file runs the
+ * real module for the merge semantics; the three call sites are asserted
+ * against the component's source, because they live in a Svelte file.
+ *
+ * `recentFiles.ts` reaches `writeStoredSetting` in `settings.svelte.ts`, which
+ * is a runes module — hence the shims below, the same ones
+ * settingsPersistence.test.ts uses and for the same reason. Node's test runner
+ * gives every file its own process, so they cannot leak.
+ */
+
+const runes = globalThis as unknown as { $state: unknown; $derived: unknown; $effect: unknown };
+const identity = (value: unknown) => value;
+runes.$state = Object.assign(identity, { raw: identity, snapshot: identity });
+runes.$derived = Object.assign(identity, { by: (fn: () => unknown) => fn() });
+runes.$effect = Object.assign((fn: () => void) => fn(), {
+	root: (fn: () => void) => {
+		fn();
+		return () => {};
+	},
+	pre: (fn: () => void) => fn(),
+	tracking: () => false,
+});
+
+const backing = new Map<string, string>();
+/** Every `setItem` that actually happened. A no-op write must not appear. */
+const writes: string[] = [];
+
+const localStorageShim = {
+	getItem: (key: string) => (backing.has(key) ? backing.get(key)! : null),
+	setItem: (key: string, value: string) => {
+		writes.push(key);
+		backing.set(key, String(value));
+	},
+	removeItem: (key: string) => {
+		writes.push(key);
+		backing.delete(key);
+	},
+	clear: () => backing.clear(),
+};
+
+Object.defineProperty(globalThis, 'localStorage', { value: localStorageShim, configurable: true });
+Object.defineProperty(globalThis, 'window', {
+	value: { __TAURI_INTERNALS__: { invoke: async () => 'macos' }, addEventListener: () => {}, removeEventListener: () => {} },
+	configurable: true,
+});
+Object.defineProperty(globalThis, 'navigator', { value: { language: 'en-US' }, configurable: true });
+
+const {
+	RECENT_FILES_KEY,
+	RECENT_FILES_LIMIT,
+	dropRecentFile,
+	isRecentFilesStorageEvent,
+	parseRecentFiles,
+	promoteRecentFile,
+	readStoredRecentFiles,
+	renameRecentFile,
+	updateStoredRecentFiles,
+} = await import('../src/lib/utils/recentFiles.js');
+
+function seed(files: string[]) {
+	backing.clear();
+	writes.length = 0;
+	backing.set(RECENT_FILES_KEY, JSON.stringify(files));
+}
+
+const stored = () => parseRecentFiles(backing.get(RECENT_FILES_KEY) ?? null);
+
+/**
+ * A window: its own in-memory copy of the list, exactly as the component holds
+ * one. `open`/`remove` are what the component's `saveRecentFile` and
+ * `deleteRecentFile` now do.
+ */
+function makeWindow(initial: string[]) {
+	const self = {
+		files: [...initial],
+		open(path: string) {
+			self.files = updateStoredRecentFiles((current) => promoteRecentFile(current, path));
+		},
+		remove(path: string) {
+			self.files = updateStoredRecentFiles((current) => dropRecentFile(current, path));
+		},
+		rename(from: string, to: string) {
+			self.files = updateStoredRecentFiles((current) => renameRecentFile(current, from, to));
+		},
+	};
+	return self;
+}
+
+test('a second window opening a file does not erase the first one', () => {
+	// The reported defect. Both windows start from the same list, both open
+	// something, and the loser used to be whoever wrote first.
+	seed(['/notes/old.md']);
+	const a = makeWindow(['/notes/old.md']);
+	const b = makeWindow(['/notes/old.md']);
+
+	a.open('/notes/from-a.md');
+	b.open('/notes/from-b.md');
+
+	assert.deepEqual(stored(), ['/notes/from-b.md', '/notes/from-a.md', '/notes/old.md']);
+	// And the window that wrote last knows the merged list without a reload.
+	assert.deepEqual(b.files, stored());
+});
+
+test('a removal in one window is not undone by an unrelated open in another', () => {
+	seed(['/notes/a.md', '/notes/b.md']);
+	const a = makeWindow(['/notes/a.md', '/notes/b.md']);
+	const b = makeWindow(['/notes/a.md', '/notes/b.md']);
+
+	a.remove('/notes/a.md');
+	b.open('/notes/c.md');
+
+	assert.deepEqual(stored(), ['/notes/c.md', '/notes/b.md']);
+	assert.ok(!stored().includes('/notes/a.md'), 'the deletion must survive the other window');
+});
+
+test('a rename in one window survives a concurrent open in another', () => {
+	seed(['/notes/draft.md']);
+	const a = makeWindow(['/notes/draft.md']);
+	const b = makeWindow(['/notes/draft.md']);
+
+	a.rename('/notes/draft.md', '/notes/final.md');
+	b.open('/notes/other.md');
+
+	assert.deepEqual(stored(), ['/notes/other.md', '/notes/final.md']);
+});
+
+test('reopening a file moves it to the front without duplicating it', () => {
+	seed(['/a.md', '/b.md', '/c.md']);
+	makeWindow([]).open('/c.md');
+	assert.deepEqual(stored(), ['/c.md', '/a.md', '/b.md']);
+});
+
+test('renaming onto a path already in the list leaves one entry', () => {
+	// The UI removes one entry per click, so a duplicate would be a row the
+	// user cannot get rid of.
+	seed(['/notes/a.md', '/notes/b.md']);
+	makeWindow([]).rename('/notes/a.md', '/notes/b.md');
+	assert.deepEqual(stored(), ['/notes/b.md']);
+});
+
+test('the list stays bounded no matter how the entries arrived', () => {
+	const many = Array.from({ length: RECENT_FILES_LIMIT + 5 }, (_, index) => `/f${index}.md`);
+	seed(many);
+	makeWindow([]).open('/new.md');
+	assert.equal(stored().length, RECENT_FILES_LIMIT);
+	assert.equal(stored()[0], '/new.md');
+});
+
+test('a write that changes nothing does not wake the other windows', () => {
+	// This is the anti-echo half. localStorage only fires `storage` when the
+	// value changes, so a compare-and-set write ends the
+	// storage -> state -> write -> storage loop after one hop. A flag would
+	// not: effects are flushed asynchronously (see settings.svelte.ts).
+	seed(['/a.md']);
+	makeWindow([]).open('/a.md');
+	assert.deepEqual(stored(), ['/a.md']);
+	assert.deepEqual(writes, [], 'an identical list must not be written back');
+});
+
+test('a corrupt entry is survivable', () => {
+	backing.clear();
+	backing.set(RECENT_FILES_KEY, '{not json');
+	assert.deepEqual(readStoredRecentFiles(), []);
+	backing.set(RECENT_FILES_KEY, '{"a":1}');
+	assert.deepEqual(readStoredRecentFiles(), []);
+	backing.set(RECENT_FILES_KEY, '["/a.md", 7, null]');
+	assert.deepEqual(readStoredRecentFiles(), ['/a.md']);
+	// And a corrupt entry must not stop the next open from being recorded.
+	backing.set(RECENT_FILES_KEY, 'garbage');
+	makeWindow([]).open('/b.md');
+	assert.deepEqual(stored(), ['/b.md']);
+});
+
+test('only this key, and a wholesale clear, count as a remote change', () => {
+	assert.equal(isRecentFilesStorageEvent({ key: RECENT_FILES_KEY, storageArea: null }), true);
+	// `key: null` is localStorage being cleared.
+	assert.equal(isRecentFilesStorageEvent({ key: null, storageArea: null }), true);
+	assert.equal(isRecentFilesStorageEvent({ key: 'theme', storageArea: null }), false);
+	assert.equal(
+		isRecentFilesStorageEvent({ key: RECENT_FILES_KEY, storageArea: {} as Storage }),
+		false,
+		'sessionStorage is a different store',
+	);
+});
+
+// --- wiring ------------------------------------------------------------------
+
+const viewer = readFileSync('src/lib/MarkdownViewer.svelte', 'utf8');
+
+test('every mutation goes through the read-modify-write helper', () => {
+	assert.match(viewer, /recentFiles = updateStoredRecentFiles\(\(current\) => promoteRecentFile\(current, path\)\)/);
+	assert.match(viewer, /recentFiles = updateStoredRecentFiles\(\(current\) => dropRecentFile\(current, path\)\)/);
+	assert.match(viewer, /recentFiles = updateStoredRecentFiles\(\(current\) => renameRecentFile\(current, oldPath, newPath\)\)/);
+	assert.equal(viewer.match(/updateStoredRecentFiles\(/g)?.length, 3, 'exactly three call sites, all of them shown above');
+});
+
+test('nothing writes the key directly any more', () => {
+	// Any surviving `setItem('recent-files', ...)` is a window publishing its
+	// own snapshot again, which is the whole defect.
+	assert.doesNotMatch(viewer, /setItem\('recent-files'/);
+	assert.doesNotMatch(viewer, /getItem\('recent-files'/);
+});
+
+test('a window folds in what its siblings did', () => {
+	// localStorage fires `storage` in every OTHER same-origin document. Without
+	// this the home screen showed a stale list until restart — and that stale
+	// list was what the next write published.
+	assert.match(viewer, /isRecentFilesStorageEvent\(event\)\) recentFiles = readStoredRecentFiles\(\)/);
+	assert.match(viewer, /window\.addEventListener\('storage', onStorage\)/);
+	assert.match(viewer, /window\.removeEventListener\('storage', onStorage\)/);
+});

src/lib/MarkdownViewer.svelte

@@ -50,6 +50,14 @@ import {
 	resolveMarkdownTargetPath,
 	type MarkdownLinkTarget as RelativeMarkdownTarget,
 } from './utils/markdownLinks.js';
+import {
+	dropRecentFile,
+	isRecentFilesStorageEvent,
+	promoteRecentFile,
+	readStoredRecentFiles,
+	renameRecentFile,
+	updateStoredRecentFiles,
+} from './utils/recentFiles.js';
 
 	const appWindow = getCurrentWindow();
 
@@ -1624,29 +1632,39 @@ import { createDocumentSession, type LoadMarkdownOptions } from './sessions/docu
 
 
 
+	/*
+	 * All three mutations go through `updateStoredRecentFiles`, which re-reads
+	 * the stored list first. Writing `JSON.stringify(recentFiles)` from this
+	 * window's copy published a snapshot from whenever this window last looked,
+	 * so with two windows open the later write erased the other's entries.
+	 */
 	function saveRecentFile(path: string) {
-		let files = [...recentFiles].filter((f) => f !== path);
-		files.unshift(path);
-		recentFiles = files.slice(0, 9);
-		localStorage.setItem('recent-files', JSON.stringify(recentFiles));
+		recentFiles = updateStoredRecentFiles((current) => promoteRecentFile(current, path));
 	}
 
 	function loadRecentFiles() {
-		const stored = localStorage.getItem('recent-files');
-		if (stored) {
-			try {
-				recentFiles = JSON.parse(stored);
-			} catch (e) {
-				console.error('Error parsing recent files:', e);
-			}
-		}
+		recentFiles = readStoredRecentFiles();
 	}
 
 	function deleteRecentFile(path: string) {
-		recentFiles = recentFiles.filter((f) => f !== path);
-		localStorage.setItem('recent-files', JSON.stringify(recentFiles));
+		recentFiles = updateStoredRecentFiles((current) => dropRecentFile(current, path));
 	}
 
+	/*
+	 * localStorage fires `storage` in every *other* same-origin document, so
+	 * this is how a window learns that a sibling opened or removed a file. Home
+	 * screens in other windows used to keep showing a stale list until restart,
+	 * and — worse — that stale list was what their next write published.
+	 */
+	$effect(() => {
+		if (typeof window === 'undefined') return;
+		const onStorage = (event: StorageEvent) => {
+			if (isRecentFilesStorageEvent(event)) recentFiles = readStoredRecentFiles();
+		};
+		window.addEventListener('storage', onStorage);
+		return () => window.removeEventListener('storage', onStorage);
+	});
+
 	function removeRecentFile(path: string, event: MouseEvent) {
 		event.stopPropagation();
 		deleteRecentFile(path);
@@ -2864,8 +2882,7 @@ import { createDocumentSession, type LoadMarkdownOptions } from './sessions/docu
 							await invoke('rename_file', { oldPath, newPath });
 							tabManager.renameTab(tabId, newPath);
 							// Update recent files if needed
-							recentFiles = recentFiles.map((f) => (f === oldPath ? newPath : f));
-							localStorage.setItem('recent-files', JSON.stringify(recentFiles));
+							recentFiles = updateStoredRecentFiles((current) => renameRecentFile(current, oldPath, newPath));
 						} catch (e) {
 							console.error('Failed to rename file', e);
 							await askCustom(`Failed to rename file: ${e}`, { title: 'Error', kind: 'error' });

src/lib/utils/recentFiles.ts

@@ -0,0 +1,80 @@
+import { writeStoredSetting } from '../stores/settings.svelte.js';
+
+export const RECENT_FILES_KEY = 'recent-files';
+export const RECENT_FILES_LIMIT = 9;
+
+/**
+ * The recent-file list, as stored. Anything that is not an array of strings is
+ * treated as absent rather than thrown: a corrupt entry must not be able to
+ * stop the app from recording new files.
+ */
+export function parseRecentFiles(raw: string | null): string[] {
+	if (raw === null) return [];
+	try {
+		const parsed = JSON.parse(raw);
+		return Array.isArray(parsed) ? parsed.filter((item): item is string => typeof item === 'string') : [];
+	} catch {
+		return [];
+	}
+}
+
+function dedupe(files: readonly string[]): string[] {
+	return [...new Set(files)];
+}
+
+/** Moves `path` to the front, keeping the list at most {@link RECENT_FILES_LIMIT} long. */
+export function promoteRecentFile(stored: readonly string[], path: string): string[] {
+	return dedupe([path, ...stored]).slice(0, RECENT_FILES_LIMIT);
+}
+
+export function dropRecentFile(stored: readonly string[], path: string): string[] {
+	return stored.filter((file) => file !== path);
+}
+
+/**
+ * Follows a renamed file. Deduplicates afterwards: the new name may already be
+ * in the list (the user renamed a file back onto a path they had opened
+ * before), and two entries for one file is a list that cannot be cleaned up
+ * from the UI, which removes one entry per click.
+ */
+export function renameRecentFile(stored: readonly string[], oldPath: string, newPath: string): string[] {
+	return dedupe(stored.map((file) => (file === oldPath ? newPath : file)));
+}
+
+export function readStoredRecentFiles(): string[] {
+	if (typeof localStorage === 'undefined') return [];
+	return parseRecentFiles(localStorage.getItem(RECENT_FILES_KEY));
+}
+
+/**
+ * Read-modify-write against localStorage, and the only way this list is
+ * changed.
+ *
+ * Every Markpad window is a separate webview with its own copy of the list and
+ * one shared localStorage. Writing `JSON.stringify(recentFiles)` from an
+ * in-memory copy therefore published a snapshot taken when that window last
+ * looked — so opening a file in window B erased everything window A had opened
+ * since, and there was no ordering in which both survived. Re-reading first
+ * makes each change a change to the *stored* list rather than to a stale copy
+ * of it.
+ *
+ * The write goes through {@link writeStoredSetting} (#370) so that a write
+ * which changes nothing does not fire a `storage` event in the other windows.
+ * That is what keeps the listener below from bouncing an update back and
+ * forth; the reasoning for preferring compare-and-set over an
+ * "I am applying a remote change" flag is documented there.
+ */
+export function updateStoredRecentFiles(mutate: (current: string[]) => string[]): string[] {
+	const next = dedupe(mutate(readStoredRecentFiles())).slice(0, RECENT_FILES_LIMIT);
+	writeStoredSetting(RECENT_FILES_KEY, JSON.stringify(next));
+	return next;
+}
+
+/**
+ * True when `event` means the recent-file list changed somewhere else.
+ * A null key is localStorage being cleared wholesale.
+ */
+export function isRecentFilesStorageEvent(event: Pick<StorageEvent, 'key' | 'storageArea'>): boolean {
+	if (typeof localStorage !== 'undefined' && event.storageArea && event.storageArea !== localStorage) return false;
+	return event.key === null || event.key === RECENT_FILES_KEY;
+}