diff --git a/scripts/recentFilesMultiWindow.test.ts b/scripts/recentFilesMultiWindow.test.ts new file mode 100644 index 00000000..e5829db4 --- /dev/null +++ b/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(); +/** 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\)/); +}); diff --git a/src/lib/MarkdownViewer.svelte b/src/lib/MarkdownViewer.svelte index a05d027d..389cebdb 100644 --- a/src/lib/MarkdownViewer.svelte +++ b/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' }); diff --git a/src/lib/utils/recentFiles.ts b/src/lib/utils/recentFiles.ts new file mode 100644 index 00000000..dc40a1df --- /dev/null +++ b/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): boolean { + if (typeof localStorage !== 'undefined' && event.storageArea && event.storageArea !== localStorage) return false; + return event.key === null || event.key === RECENT_FILES_KEY; +}