From 63cedf565e92046210b3703f09327b0d7bc143c8 Mon Sep 17 00:00:00 2001 From: PathGao Date: Mon, 3 Aug 2026 18:38:48 +0800 Subject: [PATCH] fix(save): stop a stale lossy-save refusal from silencing real auto-save failures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `isLossySaveRefused` decided from set membership alone. The set records what was SAID to the tab; it is emptied only by `loadMarkdown` and `saveContentAs`. Whether the refusal still STANDS is a property of the tab, re-decided by four other reads that fill the buffer — `ensureFullContent`, entering the editor, entering split view, a cross-window arrival — every one of which can clear `hasReplacementChars` for a file converted to UTF-8 since the load, and none of which touches the set. The auto-save timer's failure handler reads the predicate unconditionally: if (documentSession.isLossySaveRefused(s.id)) return; // no toast so a tab that had once been refused, and had since stopped decoding lossily, went on swallowing `toast.autoSaveFailed` for the rest of its life — including for a genuine write failure. The intent (a refusal already explained itself, so the generic message adds nothing) is right; the condition was broader than it. The predicate now asks the tab as well as the memory. This also answers for a tab that is gone: `closeTab` splices with no dispose hook, so closed ids stay in the set, and a tab nobody can find is refusing nothing. Behaviour while the tab IS still lossy is unchanged: the first attempt still produces the one explanation, further attempts still say nothing, and the eligibility gate in MarkdownViewer still drops the tab from the timer. Co-Authored-By: Claude Opus 5 --- scripts/checkedReadMigration.test.ts | 15 +- scripts/lossySaveRefusalScope.test.ts | 182 +++++++++++++++++++++ src/lib/sessions/documentSession.svelte.ts | 24 ++- 3 files changed, 214 insertions(+), 7 deletions(-) create mode 100644 scripts/lossySaveRefusalScope.test.ts diff --git a/scripts/checkedReadMigration.test.ts b/scripts/checkedReadMigration.test.ts index bfaaad4..8991005 100644 --- a/scripts/checkedReadMigration.test.ts +++ b/scripts/checkedReadMigration.test.ts @@ -2,7 +2,7 @@ import assert from 'node:assert/strict'; import { readFileSync } from 'node:fs'; import test from 'node:test'; -import { readSourceFiles } from './sourceTree.js'; +import { readSourceFiles, sliceBetween } from './sourceTree.js'; // Runes and the Tauri bridge, shimmed the way truncatedBufferGuard.test.ts // shims them: the stores are runes modules, and Node's test runner gives every @@ -197,11 +197,18 @@ test('entering split view reads the fidelity and stores it', () => { test('the session can tell a refusal from a failure', () => { // `saveContent` returns false for both, which is why the auto-save timer - // could not tell them apart. - assert.match( + // could not tell them apart. The predicate is no longer set membership on + // its own: the set records what was SAID, and whether the refusal still + // stands is asked of the tab. `lossySaveRefusalScope.test.ts` exercises + // both halves for real; this only pins that the tab is consulted at all, + // since a predicate that answers from memory alone is the defect. + const body = sliceBetween( readFileSync('src/lib/sessions/documentSession.svelte.ts', 'utf8'), - /function isLossySaveRefused\(tabId: string\): boolean \{\s*return lossySaveWarnedTabs\.has\(tabId\);/, + 'function isLossySaveRefused(', + 'function updateLoading', ); + assert.match(body, /lossySaveWarnedTabs\.has\(tabId\)/); + assert.match(body, /hasReplacementChars/, 'the live tab decides whether anything is still being refused'); }); test('a refused save does not add a generic toast to its own explanation', () => { diff --git a/scripts/lossySaveRefusalScope.test.ts b/scripts/lossySaveRefusalScope.test.ts new file mode 100644 index 0000000..1faeff6 --- /dev/null +++ b/scripts/lossySaveRefusalScope.test.ts @@ -0,0 +1,182 @@ +import assert from 'node:assert/strict'; +import test from 'node:test'; + +/* + * `isLossySaveRefused` is read by the auto-save timer's FAILURE handler: + * + * if (documentSession.isLossySaveRefused(s.id)) return; // no toast + * addToast(t('toast.autoSaveFailed'), 'error'); + * + * The intent is right — a refused save already explained itself, naming the + * file and the way out, so "auto-save failed" on top says nothing new. But the + * predicate was `lossySaveWarnedTabs.has(tabId)`, membership in a set that is + * only ever cleared by `loadMarkdown` and `saveContentAs`. The tab's decode + * fidelity is re-decided by four other sites — `ensureFullContent`, entering + * the editor, entering split view, a cross-window arrival — every one of which + * can clear `hasReplacementChars` for a file converted to UTF-8 since the load, + * and none of which touches the set. + * + * So a tab could stop decoding lossily, start auto-saving again (the + * eligibility gate is `decodedLossily && isLossySaveRefused`, which correctly + * re-opens), and then have every subsequent auto-save failure — disk full, + * permission denied, a network volume that went away — silently swallow its + * toast, for the rest of that tab's life. + * + * The condition the name promises is "this tab's saves are being refused for + * the lossy-decode reason, right now", and "right now" is a property of the + * tab, not a memory of what was once said about it. + */ + +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 ?? {}; +Object.defineProperty(g, 'navigator', { value: { language: 'en-US' }, configurable: true }); +Object.defineProperty(g, 'localStorage', { + value: { getItem: () => null, setItem: () => {}, removeItem: () => {}, clear: () => {} }, + configurable: true, +}); + +/** Whether `save_file_content` succeeds. Set per test. */ +let writeFails = false; +const LOSSY = 'text with � in it'; + +g.window.__TAURI_INTERNALS__ = { + invoke: (command: string, args: Record) => { + const cmd = command.replace(/^plugin:[^|]*\|/, ''); + if (cmd === 'get_os_type') return Promise.resolve('macos'); + if (cmd === 'canonicalize_path') return Promise.resolve(args.path); + if (cmd === 'open_markdown_preview') return Promise.resolve(['', LOSSY, true, true]); + if (cmd === 'read_file_content_checked') return Promise.resolve([LOSSY, true]); + if (cmd === 'save_file_content') { + return writeFails ? Promise.reject(new Error('Os { code: 28, kind: StorageFull }')) : Promise.resolve(null); + } + return Promise.resolve(null); + }, +}; + +const { tabManager } = await import('../src/lib/stores/tabs.svelte.js'); +const { createDocumentSession } = await import('../src/lib/sessions/documentSession.svelte.js'); + +/** Every message the user would actually see, in order. */ +let toasts: string[] = []; + +function makeSession() { + return createDocumentSession({ + setShowHome: () => {}, + currentFile: () => tabManager.activeTab?.path ?? '', + resetScrollHistory: () => {}, + renderMarkdown: async () => '', + afterLoad: async () => {}, + saveRecentFile: () => {}, + deleteRecentFile: () => {}, + setLoadingTabs: () => {}, + measureInitialViewport: () => {}, + isScrolling: () => false, + renderRichContent: () => {}, + onError: (message) => void toasts.push(message), + selfWriteGraceMs: 400, + cancelPendingAutoSave: () => {}, + askClose: async () => 'discard' as const, + onCloseSaveNewerEdits: () => {}, + onCloseAutoSaveFailed: () => {}, + }); +} + +/** + * A tab holding a buffer that came out of a lossy decode, edited, and refused + * once — which is the state the refusal set records. + */ +async function refusedTab() { + tabManager.closeAll(); + toasts = []; + writeFails = false; + const session = makeSession(); + await session.loadMarkdown('/notes/legacy.md'); + const tab = tabManager.activeTab!; + assert.equal(tab.hasReplacementChars, true, 'precondition: the decode was lossy'); + + tabManager.updateTabRawContent(tab.id, 'edited'); + assert.equal(await session.saveContent(tab.id), false, 'the guard must refuse this write'); + assert.equal(toasts.length, 1, 'and explain itself exactly once'); + assert.equal(session.isLossySaveRefused(tab.id), true); + return { session, tab }; +} + +test('a refusal still speaks once, and stays quiet afterwards', async () => { + // The behaviour that must survive: while the tab still decodes lossily, + // every further attempt is the same standing condition, already explained. + const { session, tab } = await refusedTab(); + + tabManager.updateTabRawContent(tab.id, 'edited again'); + assert.equal(await session.saveContent(tab.id), false); + tabManager.updateTabRawContent(tab.id, 'and again'); + assert.equal(await session.saveContent(tab.id), false); + + assert.equal(toasts.length, 1, 'a standing condition is not re-reported'); + assert.equal(session.isLossySaveRefused(tab.id), true, 'and the timer keeps its toast suppressed'); +}); + +test('a tab that no longer decodes lossily is no longer being refused', async () => { + // The file was converted to UTF-8 and the tab re-read it. This is what + // `ensureFullContent`, `toggleEdit`, split view and a cross-window arrival + // all do — none of them goes through `loadMarkdown`, so none of them + // clears the set. + const { session, tab } = await refusedTab(); + tabManager.setTabDecodedLossy(tab.id, false); + + assert.equal(session.isLossySaveRefused(tab.id), false, 'nothing is being refused any more'); +}); + +test('a real auto-save failure on that tab is still reportable', async () => { + // End to end, in the shape the auto-save timer sees it: `saveContent` + // returns false, and the predicate the timer consults before deciding + // whether to raise `toast.autoSaveFailed` must not claim this was a + // refusal. Under the stale-set predicate it did, and the user was left + // believing a full disk had saved their work. + const { session, tab } = await refusedTab(); + tabManager.setTabDecodedLossy(tab.id, false); + writeFails = true; + + tabManager.updateTabRawContent(tab.id, 'work the user expects to be saved'); + assert.equal(await session.saveContent(tab.id), false, 'the write really did fail'); + assert.equal( + session.isLossySaveRefused(tab.id), + false, + 'a failure is not a refusal, so the timer must be free to report it', + ); +}); + +test('the flag goes back up if the file is lossy again', async () => { + // Symmetry: the predicate answers about the tab as it is now, in both + // directions. A tab that was converted back (or a second file opened into + // the same tab that is lossy) is refused again — and having already been + // told once, it is not told twice. + const { session, tab } = await refusedTab(); + tabManager.setTabDecodedLossy(tab.id, false); + tabManager.setTabDecodedLossy(tab.id, true); + + assert.equal(session.isLossySaveRefused(tab.id), true); + assert.equal(await session.saveContent(tab.id), false); + assert.equal(toasts.length, 1, 'still only the one explanation'); +}); + +test('a closed tab is not remembered as refused', async () => { + // `closeTab` splices the tab out with no dispose hook, so the set keeps + // the id forever. Ids are `crypto.randomUUID()` and never reused, so this + // is a bounded leak rather than a correctness problem — but the predicate + // must not answer "yes" about a tab that no longer exists. + const { session, tab } = await refusedTab(); + const id = tab.id; + tabManager.closeTab(id); + + assert.equal(session.isLossySaveRefused(id), false); +}); diff --git a/src/lib/sessions/documentSession.svelte.ts b/src/lib/sessions/documentSession.svelte.ts index 53dfc8b..6cba92f 100644 --- a/src/lib/sessions/documentSession.svelte.ts +++ b/src/lib/sessions/documentSession.svelte.ts @@ -200,8 +200,8 @@ export function createDocumentSession(options: DocumentSessionOptions) { } /** - * True once this tab has been told, in words, that its buffer cannot be - * written back over its own file. + * True when this tab's saves are being refused for the lossy-decode reason + * RIGHT NOW, and it has already been told so in words. * * The explanation is deduplicated per tab above, but the callers' own * failure reporting was not: `saveContent` returns `false` for a refusal @@ -210,9 +210,27 @@ export function createDocumentSession(options: DocumentSessionOptions) { * every keystroke, repeated it every 1.5s for as long as the user kept * typing. A refusal is not a failure to report again; it is a standing * condition the user has already been told about and given an exit from. + * + * Both halves are needed, and the second is why set membership alone was + * not enough. `lossySaveWarnedTabs` records what was SAID; whether the + * refusal still stands is a property of the tab, decided fresh by every + * read that fills the buffer — `ensureFullContent`, entering the editor, + * entering split view, a cross-window arrival. Any of those clears + * `hasReplacementChars` for a file converted to UTF-8 since the load, and + * none of them goes through `loadMarkdown`, the only place that empties the + * set. A tab that had once been refused therefore went on suppressing the + * generic message for the rest of its life, including for a genuine write + * failure — a full disk, a revoked permission, a network volume that went + * away — which is the opposite of "already explained". + * + * Asking the tab also answers for one that is gone: `closeTab` splices with + * no dispose hook, so the set keeps closed ids, and a tab nobody can find + * is refusing nothing. */ function isLossySaveRefused(tabId: string): boolean { - return lossySaveWarnedTabs.has(tabId); + if (!lossySaveWarnedTabs.has(tabId)) return false; + const tab = tabManager.tabs.find((item) => item.id === tabId); + return tab?.hasReplacementChars === true; } function updateLoading(tabId: string, loading: boolean) {