From 9205933a244b785634bbca64094a30389cf8e76a Mon Sep 17 00:00:00 2001 From: PathGao <42336971+PathGao@users.noreply.github.com> Date: Mon, 13 Jul 2026 00:25:58 +0900 Subject: [PATCH 1/6] fix: close dirty tabs one at a time on window close Replace the aggregate "you have N unsaved files" modal with a per-tab walk (issue #189): activate each dirty tab and run the same localized unsaved-changes dialog a single tab close shows (canCloseTab), then close the tab. Cancel stops the walk and keeps the window open with the remaining tabs; the window closes only after every dirty tab is resolved. The auto-save fast path and the restore-on-reopen branch are untouched original behavior. Co-Authored-By: Claude Opus 4.8 --- scripts/windowClosePerTab.test.ts | 62 +++++++++++++++++++++++++++++++ src/lib/MarkdownViewer.svelte | 45 +++++++++------------- 2 files changed, 79 insertions(+), 28 deletions(-) create mode 100644 scripts/windowClosePerTab.test.ts diff --git a/scripts/windowClosePerTab.test.ts b/scripts/windowClosePerTab.test.ts new file mode 100644 index 00000000..01f134ea --- /dev/null +++ b/scripts/windowClosePerTab.test.ts @@ -0,0 +1,62 @@ +import assert from 'node:assert/strict'; +import { readFileSync } from 'node:fs'; +import test from 'node:test'; + +const viewer = readFileSync('src/lib/MarkdownViewer.svelte', 'utf8'); + +// Window close (issue #189): instead of one aggregate "you have N unsaved +// files" modal, the red close button walks the dirty tabs one at a time — +// activating each and showing the SAME localized unsaved-changes dialog a +// single tab close shows (canCloseTab). Cancel stops the walk; the window +// stays open with the remaining tabs. + +function closeHandler(): string { + const start = viewer.indexOf('appWindow.onCloseRequested'); + assert.ok(start !== -1, 'close handler registration not found'); + return viewer.slice(start, viewer.indexOf('onDragDropEvent', start)); +} + +test('the aggregate unsaved-files modal is gone from the close handler', () => { + const handler = closeHandler(); + assert.doesNotMatch(handler, /youHaveUnsavedFiles/); + // and the old "clear all dirty flags then close" discard path with it + assert.doesNotMatch(handler, /tabManager\.tabs\.forEach\(\(t\) => \(t\.isDirty = false\)\)/); +}); + +test('dirty tabs are reviewed one at a time through the existing canCloseTab flow', () => { + const handler = closeHandler(); + // activate the tab under review so the user sees what the dialog is about + assert.match(handler, /tabManager\.setActive\(dirty\.id\);/); + // the existing localized per-tab dialog decides save / discard / cancel + assert.match(handler, /await canCloseTab\(dirty\.id\)/); + // a resolved tab actually closes before moving on + assert.match(handler, /tabManager\.closeTab\(dirty\.id\);/); +}); + +test('cancelling the per-tab dialog stops the walk and keeps the window open', () => { + const handler = closeHandler(); + assert.match(handler, /if \(!\(await canCloseTab\(dirty\.id\)\)\) return;/); +}); + +test('the close is prevented synchronously before the per-tab walk', () => { + const handler = closeHandler(); + const branchStart = handler.indexOf('if (dirtyTabs.length > 0) {'); + assert.ok(branchStart !== -1, 'dirty branch not found'); + const prevent = handler.indexOf('event.preventDefault()', branchStart); + const walk = handler.indexOf('canCloseTab(dirty.id)', branchStart); + assert.ok(prevent !== -1 && walk !== -1 && prevent < walk); +}); + +test('the window closes only after every dirty tab is resolved', () => { + const handler = closeHandler(); + const walk = handler.indexOf('canCloseTab(dirty.id)'); + const close = handler.indexOf('appWindow.close()', walk); + assert.ok(walk !== -1 && close !== -1 && walk < close); +}); + +test('the restore-on-reopen branch is untouched original behavior', () => { + const handler = closeHandler(); + assert.match(handler, /localStorage\.setItem\('savedTabsData', stateStr\);/); + // no durable-write experiment left behind + assert.doesNotMatch(viewer, /saveSessionState|sessionState\.js/); +}); diff --git a/src/lib/MarkdownViewer.svelte b/src/lib/MarkdownViewer.svelte index 4e357e87..1b156a49 100644 --- a/src/lib/MarkdownViewer.svelte +++ b/src/lib/MarkdownViewer.svelte @@ -2579,34 +2579,23 @@ import { t } from './utils/i18n.js'; // hasUntitled: skip toast, just fall through to the modal. } - const response = await askCustom(t('modal.youHaveUnsavedFiles', settings.language).replace('{{count}}', dirtyTabs.length.toString()), { - title: t('modal.unsavedChanges', settings.language), - kind: 'warning', - showSave: true, - }); - - if (response === 'save') { - // Attempt to save all dirty tabs - for (const tab of dirtyTabs) { - tabManager.setActive(tab.id); - await tick(); - cancelPendingAutoSave(tab.id); - const saved = await saveContent(tab.id); - if (!saved) return; // Cancelled or failed - } - // If all saved successfully, close the app - appWindow.close(); - } else if (response === 'discard') { - // Force close by removing this listener or skipping check? - // Since we are inside the event handler, we can't easily remove "this" listener specifically - // without refactoring how unlisteners are stored/accessed relative to this callback. - // However, if we just want to exit, we can use exit() from rust or just appWindow.destroy()? - // WebviewWindow.close() triggers this event again. - // Solution: invoke a command to exit forcefully or set a flag. - // The simplest might be to just clear the dirty flags and close. - tabManager.tabs.forEach((t) => (t.isDirty = false)); - appWindow.close(); - } + // Close the dirty tabs one at a time (issue #189): activate + // each tab and run the same localized unsaved-changes dialog + // a single tab close shows. Cancel stops the walk and keeps + // the window open with the remaining tabs. Re-find the next + // dirty tab every round — a save can leave a tab dirty again + // (TOCTOU) and tabs can change while a dialog is up. + while (true) { + const dirty = tabManager.tabs.find((t) => t.isDirty); + if (!dirty) break; + tabManager.setActive(dirty.id); + await tick(); + if (!(await canCloseTab(dirty.id))) return; + tabManager.closeTab(dirty.id); + } + // Every dirty tab is resolved; this close re-enters the + // handler, finds nothing dirty, and proceeds. + appWindow.close(); } }), ); From 29dec4bcde51fbc97812ad77a1aea0c908502690 Mon Sep 17 00:00:00 2001 From: PathGao <42336971+PathGao@users.noreply.github.com> Date: Mon, 13 Jul 2026 00:25:58 +0900 Subject: [PATCH 2/6] feat: restore window state only; content always comes from disk MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The restore snapshot previously carried every tab's full document content (rawContent + originalContent + history), doubling as an unreliable unsaved-content store and letting restored tabs show stale bytes when the file changed on disk while the app was closed. Separate the concerns. serializeState (v2) records window state only: open file paths, active tab, edit mode, split, scroll. On startup each restored tab reads its file from disk; unreadable files drop their tab. The close flow resolves dirty tabs FIRST through the per-tab dialogs — regardless of the restore setting — then writes the snapshot: Save persists to disk, Don't Save reverts the tab to its last saved content, Cancel keeps the window open. Untitled tabs are never persisted; the auto-save fast path still saves titled tabs silently. Legacy snapshots restore through the same path (window-state fields only). Co-Authored-By: Claude Opus 4.8 --- scripts/windowStateRestore.test.ts | 81 +++++++++++++++ src/lib/MarkdownViewer.svelte | 154 ++++++++++++++--------------- src/lib/stores/tabs.svelte.ts | 64 +++++++++++- 3 files changed, 214 insertions(+), 85 deletions(-) create mode 100644 scripts/windowStateRestore.test.ts diff --git a/scripts/windowStateRestore.test.ts b/scripts/windowStateRestore.test.ts new file mode 100644 index 00000000..2e3cabeb --- /dev/null +++ b/scripts/windowStateRestore.test.ts @@ -0,0 +1,81 @@ +import assert from 'node:assert/strict'; +import { readFileSync } from 'node:fs'; +import test from 'node:test'; + +const tabs = readFileSync('src/lib/stores/tabs.svelte.ts', 'utf8'); +const viewer = readFileSync('src/lib/MarkdownViewer.svelte', 'utf8'); + +// Session restore persists WINDOW state only: which files are open, the +// active tab, and per-tab UI (edit mode, split, scroll). Document content +// always lives on disk — the snapshot never carries rawContent, so unsaved +// changes are handled exclusively by the per-tab close dialogs. + +function slice(source: string, from: string, to: string): string { + const start = source.indexOf(from); + assert.ok(start !== -1, `${from} not found`); + const end = source.indexOf(to, start); + assert.ok(end !== -1, `${to} not found after ${from}`); + return source.slice(start, end); +} + +test('serializeState writes window state only', () => { + const fn = slice(tabs, 'serializeState()', 'restoreState('); + assert.match(fn, /version: 2/); + // untitled tabs have no disk backing; they are resolved at close, never persisted + assert.match(fn, /filter\(\(t\) => t\.path !== ''\)/); + // no full-object spread and no content fields in the snapshot + assert.doesNotMatch(fn, /\.\.\.t/); + assert.doesNotMatch(fn, /rawContent/); + assert.doesNotMatch(fn, /originalContent/); + assert.doesNotMatch(fn, /isDirty/); + assert.doesNotMatch(fn, /history/); +}); + +test('restoreState rebuilds clean tabs and drops legacy untitled entries', () => { + const fn = slice(tabs, 'restoreState(', 'addTab('); + // path is the identity of a restored tab; entries without one are skipped + assert.match(fn, /saved\.path === ''\) continue;/); + // restored tabs start clean; content is read from disk afterwards + assert.match(fn, /isDirty: false/); + assert.match(fn, /rawContent: ''/); + // a stale activeTabId falls back to the first restored tab + assert.match(fn, /activeTabId/); +}); + +test('startup restore reads content from disk, not from the snapshot', () => { + const init = slice(viewer, "localStorage.getItem('savedTabsData')", 'urlParams'); + assert.match(init, /read_file_content/); + // a missing file drops its tab instead of restoring a ghost + assert.match(init, /closeTab\(/); +}); + +test('the discard choice reverts the tab to its last saved content', () => { + const fn = slice(viewer, 'async function canCloseTab', 'async function toggleEdit'); + assert.match(fn, /tab\.rawContent = tab\.originalContent;/); + assert.match(fn, /tab\.isDirty = false;/); +}); + +test('the close flow resolves dirty tabs before serializing window state', () => { + const handlerStart = viewer.indexOf('appWindow.onCloseRequested'); + const handler = viewer.slice(handlerStart, viewer.indexOf('onDragDropEvent', handlerStart)); + const walk = handler.indexOf('canCloseTab(dirty.id)'); + const persist = handler.indexOf("localStorage.setItem('savedTabsData'"); + assert.ok(walk !== -1, 'per-tab walk not found'); + assert.ok(persist !== -1, 'window-state serialization not found'); + assert.ok(walk < persist, 'dirty tabs must be resolved before the snapshot is written'); +}); + +test('with restore enabled resolved titled tabs stay open for the snapshot', () => { + const handlerStart = viewer.indexOf('appWindow.onCloseRequested'); + const handler = viewer.slice(handlerStart, viewer.indexOf('onDragDropEvent', handlerStart)); + // tabs are closed one-by-one only when restore is off (or untitled) + assert.match(handler, /restoreStateOnReopen \|\| dirty\.path === ''/); +}); + +test('auto-save fast path silently saves titled tabs before the walk', () => { + const handlerStart = viewer.indexOf('appWindow.onCloseRequested'); + const handler = viewer.slice(handlerStart, viewer.indexOf('onDragDropEvent', handlerStart)); + const fastPath = handler.indexOf('settings.autoSave && !settings.confirmBeforeSave'); + const walk = handler.indexOf('canCloseTab(dirty.id)'); + assert.ok(fastPath !== -1 && walk !== -1 && fastPath < walk); +}); diff --git a/src/lib/MarkdownViewer.svelte b/src/lib/MarkdownViewer.svelte index 1b156a49..069182c1 100644 --- a/src/lib/MarkdownViewer.svelte +++ b/src/lib/MarkdownViewer.svelte @@ -1301,9 +1301,12 @@ import { t } from './utils/i18n.js'; } // Discard: drop pending save so we don't write what the user just - // threw away. The effect will re-arm a timer if the tab gets edited - // again. + // threw away, and revert to the last saved content so the tab is + // clean — callers either close it (tab close) or keep it open for the + // window-state snapshot (window close with restore enabled). cancelPendingAutoSave(tabId); + tab.rawContent = tab.originalContent; + tab.isDirty = false; return true; } @@ -2369,17 +2372,23 @@ import { t } from './utils/i18n.js'; const savedData = localStorage.getItem('savedTabsData'); if (savedData) { tabManager.restoreState(savedData); - for (const tab of tabManager.tabs) { - if (!tab.content && tab.rawContent) { - invoke('render_markdown', { content: tab.rawContent }) - .then((html) => { - const processed = processMarkdownHtml(html as string, tab.path, collapsedHeaders); - tabManager.updateTabContent(tab.id, processed); - if (tabManager.activeTabId === tab.id) { - tick().then(renderRichContent); - } - }) - .catch(console.error); + // The snapshot carries window state only — content always + // comes from disk, so restored tabs show the file's real + // current bytes. A file that no longer exists drops its tab. + for (const tab of [...tabManager.tabs]) { + try { + const raw = (await invoke('read_file_content', { path: tab.path })) as string; + tab.rawContent = raw; + tab.originalContent = raw; + const html = await invoke('render_markdown', { content: raw }); + const processed = processMarkdownHtml(html as string, tab.path, collapsedHeaders); + tabManager.updateTabContent(tab.id, processed); + if (tabManager.activeTabId === tab.id) { + tick().then(renderRichContent); + } + } catch (e) { + console.warn('Restore: dropping tab for unreadable file', tab.path, e); + tabManager.closeTab(tab.id); } } } @@ -2516,26 +2525,57 @@ import { t } from './utils/i18n.js'; console.log('onCloseRequested triggered'); if (isForceExiting) return; - // CRITICAL: before serializing tab state to localStorage - // (the restore-on-reopen path), make sure all pending - // auto-save edits are actually flushed to disk. Without - // this, closing the window with auto-save on but - // confirmBeforeSave off would silently put unsaved edits - // in localStorage only and never persist them to file. - if (settings.autoSave && !settings.confirmBeforeSave) { - const dirtyWithPath = tabManager.tabs.filter( - (t) => t.isDirty && t.path !== '', - ); - for (const tab of dirtyWithPath) { - cancelPendingAutoSave(tab.id); - try { - await saveContent(tab.id); - } catch (e) { - console.error('Flush-on-close save failed for tab', tab.id, e); + // Unsaved content and session restore are separate concerns: + // dirty tabs are resolved FIRST through the per-tab dialogs, + // then the restore snapshot records window state only (open + // files, active tab, edit mode, split, scroll) — it never + // carries document content. + const dirtyTabs = tabManager.tabs.filter((t) => t.isDirty); + if (dirtyTabs.length > 0) { + event.preventDefault(); + + // Auto-save without confirmation: silently save every + // dirty tab that has a real path. Untitled tabs need a + // Save dialog, so the walk below handles them. A failed + // silent save is surfaced and its tab also goes to the + // walk. Timers are cancelled per tab right before its + // save to avoid duplicate writes. + if (settings.autoSave && !settings.confirmBeforeSave) { + for (const tab of dirtyTabs.filter((t) => t.path !== '')) { + cancelPendingAutoSave(tab.id); + const ok = await saveContent(tab.id); + if (!ok) { + addToast(t('toast.autoSaveFailed', settings.language), 'error'); + break; + } + } + } + + // Close review (issue #189): walk the remaining dirty + // tabs one at a time — activate each and run the same + // localized unsaved-changes dialog a single tab close + // shows. Cancel stops the walk and keeps the window + // open. Re-find the next dirty tab every round — a save + // can leave a tab dirty again (TOCTOU) and tabs can + // change while a dialog is up. + while (true) { + const dirty = tabManager.tabs.find((t) => t.isDirty); + if (!dirty) break; + tabManager.setActive(dirty.id); + await tick(); + if (!(await canCloseTab(dirty.id))) return; + // Resolved tabs (saved, or reverted by Don't Save) + // stay open for the window-state snapshot when + // restore is enabled; untitled tabs have nothing to + // restore, and with restore off the red button + // closes tabs one by one. + if (!settings.restoreStateOnReopen || dirty.path === '') { + tabManager.closeTab(dirty.id); } } } + // Session is clean now; record the window state for restore. if (settings.restoreStateOnReopen) { try { const stateStr = tabManager.serializeState(); @@ -2543,60 +2583,12 @@ import { t } from './utils/i18n.js'; } catch (e) { console.error('Failed to save state on close:', e); } - return; - } - - const dirtyTabs = tabManager.tabs.filter((t) => t.isDirty); - console.log('Dirty tabs:', dirtyTabs.length); - if (dirtyTabs.length > 0) { - console.log('Preventing default close'); - event.preventDefault(); - - // Auto-save without confirmation: try silently saving every dirty - // tab that has a real path. If untitled tabs exist they need a - // Save dialog, so we just fall through to the modal — that is - // NOT a failure case and shouldn't show an error toast. We - // also DON'T clear pending timers up front: if the user picks - // Cancel in the modal below, we want the timers to keep - // running for tabs that are still dirty. - if (settings.autoSave && !settings.confirmBeforeSave) { - const tabsWithPath = dirtyTabs.filter((t) => t.path !== ''); - const hasUntitled = dirtyTabs.some((t) => t.path === ''); - if (!hasUntitled) { - let allOk = true; - for (const tab of tabsWithPath) { - cancelPendingAutoSave(tab.id); - const ok = await saveContent(tab.id); - if (!ok) { allOk = false; break; } - } - if (allOk) { - appWindow.close(); - return; - } - // A real save failure happened — surface it. - addToast(t('toast.autoSaveFailed', settings.language), 'error'); } - // hasUntitled: skip toast, just fall through to the modal. - } - // Close the dirty tabs one at a time (issue #189): activate - // each tab and run the same localized unsaved-changes dialog - // a single tab close shows. Cancel stops the walk and keeps - // the window open with the remaining tabs. Re-find the next - // dirty tab every round — a save can leave a tab dirty again - // (TOCTOU) and tabs can change while a dialog is up. - while (true) { - const dirty = tabManager.tabs.find((t) => t.isDirty); - if (!dirty) break; - tabManager.setActive(dirty.id); - await tick(); - if (!(await canCloseTab(dirty.id))) return; - tabManager.closeTab(dirty.id); - } - // Every dirty tab is resolved; this close re-enters the - // handler, finds nothing dirty, and proceeds. - appWindow.close(); - } + // If we intercepted the close to run the review, re-trigger + // it: the handler re-enters, finds nothing dirty, and the + // close proceeds. + if (dirtyTabs.length > 0) appWindow.close(); }), ); diff --git a/src/lib/stores/tabs.svelte.ts b/src/lib/stores/tabs.svelte.ts index 73506d44..ec8226c4 100644 --- a/src/lib/stores/tabs.svelte.ts +++ b/src/lib/stores/tabs.svelte.ts @@ -54,21 +54,77 @@ class TabManager { return this.tabs.find((t) => t.id === this.activeTabId); } + /** + * Serialize WINDOW state only: which files are open, the active tab, and + * per-tab UI (edit mode, split, scroll). Document content always lives on + * disk — the snapshot never carries rawContent, so unsaved changes are + * handled exclusively by the close dialogs, never smuggled through here. + * Untitled tabs have no disk backing and are resolved at close, so they + * are not persisted. + */ serializeState(): string { const stateData = { + version: 2, activeTabId: this.activeTabId, - tabs: this.tabs.map(t => ({ ...t, editorViewState: null, content: '' })) + tabs: this.tabs + .filter((t) => t.path !== '') + .map((t) => ({ + id: t.id, + path: t.path, + title: t.title, + isEditing: t.isEditing, + isSplit: t.isSplit, + splitRatio: t.splitRatio, + isScrollSynced: t.isScrollSynced, + scrollTop: t.scrollTop, + scrollPercentage: t.scrollPercentage, + anchorLine: t.anchorLine + })) }; return JSON.stringify(stateData); } + /** + * Rebuild clean tabs from a window-state snapshot. Content starts empty — + * the caller reads each file from disk afterwards. Also accepts the legacy + * full-tab format, from which only the window-state fields are taken + * (legacy untitled entries are dropped). + */ restoreState(jsonBuffer: string) { try { const data = JSON.parse(jsonBuffer); - if (data && Array.isArray(data.tabs)) { - this.tabs = data.tabs; - this.activeTabId = data.activeTabId; + if (!data || !Array.isArray(data.tabs)) return; + + const restored: Tab[] = []; + for (const saved of data.tabs) { + if (!saved || typeof saved.path !== 'string' || saved.path === '') continue; + const filename = saved.path.split('\\').pop()?.split('/').pop() || saved.path; + const fileHistory = createFileHistory(saved.path, ''); + restored.push({ + id: typeof saved.id === 'string' ? saved.id : crypto.randomUUID(), + path: saved.path, + title: typeof saved.title === 'string' && saved.title !== '' ? saved.title : filename, + content: '', + rawContent: '', + originalContent: '', + scrollTop: typeof saved.scrollTop === 'number' ? saved.scrollTop : 0, + isDirty: false, + isEditing: saved.isEditing === true, + history: fileHistory.history, + historyIndex: fileHistory.historyIndex, + editorViewState: null, + scrollPercentage: typeof saved.scrollPercentage === 'number' ? saved.scrollPercentage : 0, + anchorLine: typeof saved.anchorLine === 'number' ? saved.anchorLine : 0, + isSplit: saved.isSplit === true, + splitRatio: typeof saved.splitRatio === 'number' ? saved.splitRatio : 0.5, + isScrollSynced: saved.isScrollSynced === true + }); } + + this.tabs = restored; + this.activeTabId = restored.some((t) => t.id === data.activeTabId) + ? data.activeTabId + : restored[0]?.id ?? null; } catch (e) { console.error('Failed to restore tab state', e); } From 60c751fc1471f04619b06509183440035721fee4 Mon Sep 17 00:00:00 2001 From: PathGao <42336971+PathGao@users.noreply.github.com> Date: Mon, 13 Jul 2026 00:25:58 +0900 Subject: [PATCH 3/6] fix: keep the close walk's dialog matched to the highlighted tab MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The red button is a native control, so the in-app dialog overlay does not block it: a second click while the walk had a dialog up re-entered the handler and started a competing walk whose setActive calls fought the first one — with two untitled tabs the highlight and the dialog visibly disagreed. Guard the walk with a re-entrancy flag (released in finally, including the cancel path), and start each round from the tab the user is already looking at so the highlight only jumps when it has to. Co-Authored-By: Claude Opus 4.8 --- scripts/windowClosePerTab.test.ts | 15 +++++ src/lib/MarkdownViewer.svelte | 94 +++++++++++++++++++------------ 2 files changed, 74 insertions(+), 35 deletions(-) diff --git a/scripts/windowClosePerTab.test.ts b/scripts/windowClosePerTab.test.ts index 01f134ea..04142523 100644 --- a/scripts/windowClosePerTab.test.ts +++ b/scripts/windowClosePerTab.test.ts @@ -54,6 +54,21 @@ test('the window closes only after every dirty tab is resolved', () => { assert.ok(walk !== -1 && close !== -1 && walk < close); }); +test('a second close request cannot start a competing walk', () => { + const handler = closeHandler(); + // The native red button bypasses the dialog overlay; re-entry must be + // swallowed while a walk is active, or two walks fight over setActive + // and the highlighted tab stops matching the dialog. + assert.match(handler, /if \(isCloseWalkActive\) \{\s*event\.preventDefault\(\);\s*return;\s*\}/); + // and the flag is always released, even when the user cancels mid-walk + assert.match(handler, /finally \{\s*isCloseWalkActive = false;\s*\}/); +}); + +test('the walk starts from the tab the user is already looking at', () => { + const handler = closeHandler(); + assert.match(handler, /active\?\.isDirty\s*\?\s*active\s*:\s*tabManager\.tabs\.find\(\(t\) => t\.isDirty\)/); +}); + test('the restore-on-reopen branch is untouched original behavior', () => { const handler = closeHandler(); assert.match(handler, /localStorage\.setItem\('savedTabsData', stateStr\);/); diff --git a/src/lib/MarkdownViewer.svelte b/src/lib/MarkdownViewer.svelte index 069182c1..057e27cb 100644 --- a/src/lib/MarkdownViewer.svelte +++ b/src/lib/MarkdownViewer.svelte @@ -338,6 +338,10 @@ import { t } from './utils/i18n.js'; } let isForceExiting = $state(false); + // True while the window-close walk is showing per-tab dialogs; the native + // red button is not blocked by the dialog overlay, so this keeps a second + // close request from starting a competing walk. + let isCloseWalkActive = false; async function appExit() { if (settings.restoreStateOnReopen) { @@ -2525,6 +2529,17 @@ import { t } from './utils/i18n.js'; console.log('onCloseRequested triggered'); if (isForceExiting) return; + // The red button is a native control, so it is NOT blocked + // by the in-app dialog overlay: a second click while the + // walk below is showing a dialog would re-enter this handler + // and start a competing walk whose setActive calls fight the + // first one — the highlighted tab stops matching the dialog. + // One walk at a time. + if (isCloseWalkActive) { + event.preventDefault(); + return; + } + // Unsaved content and session restore are separate concerns: // dirty tabs are resolved FIRST through the per-tab dialogs, // then the restore snapshot records window state only (open @@ -2533,45 +2548,54 @@ import { t } from './utils/i18n.js'; const dirtyTabs = tabManager.tabs.filter((t) => t.isDirty); if (dirtyTabs.length > 0) { event.preventDefault(); - - // Auto-save without confirmation: silently save every - // dirty tab that has a real path. Untitled tabs need a - // Save dialog, so the walk below handles them. A failed - // silent save is surfaced and its tab also goes to the - // walk. Timers are cancelled per tab right before its - // save to avoid duplicate writes. - if (settings.autoSave && !settings.confirmBeforeSave) { - for (const tab of dirtyTabs.filter((t) => t.path !== '')) { - cancelPendingAutoSave(tab.id); - const ok = await saveContent(tab.id); - if (!ok) { - addToast(t('toast.autoSaveFailed', settings.language), 'error'); - break; + isCloseWalkActive = true; + try { + // Auto-save without confirmation: silently save every + // dirty tab that has a real path. Untitled tabs need a + // Save dialog, so the walk below handles them. A failed + // silent save is surfaced and its tab also goes to the + // walk. Timers are cancelled per tab right before its + // save to avoid duplicate writes. + if (settings.autoSave && !settings.confirmBeforeSave) { + for (const tab of dirtyTabs.filter((t) => t.path !== '')) { + cancelPendingAutoSave(tab.id); + const ok = await saveContent(tab.id); + if (!ok) { + addToast(t('toast.autoSaveFailed', settings.language), 'error'); + break; + } } } - } - // Close review (issue #189): walk the remaining dirty - // tabs one at a time — activate each and run the same - // localized unsaved-changes dialog a single tab close - // shows. Cancel stops the walk and keeps the window - // open. Re-find the next dirty tab every round — a save - // can leave a tab dirty again (TOCTOU) and tabs can - // change while a dialog is up. - while (true) { - const dirty = tabManager.tabs.find((t) => t.isDirty); - if (!dirty) break; - tabManager.setActive(dirty.id); - await tick(); - if (!(await canCloseTab(dirty.id))) return; - // Resolved tabs (saved, or reverted by Don't Save) - // stay open for the window-state snapshot when - // restore is enabled; untitled tabs have nothing to - // restore, and with restore off the red button - // closes tabs one by one. - if (!settings.restoreStateOnReopen || dirty.path === '') { - tabManager.closeTab(dirty.id); + // Close review (issue #189): walk the remaining dirty + // tabs one at a time — activate each and run the same + // localized unsaved-changes dialog a single tab close + // shows. Cancel stops the walk and keeps the window + // open. Prefer the tab the user is already looking at + // so the highlight only jumps when it has to, and + // re-find every round — a save can leave a tab dirty + // again (TOCTOU) and tabs can change while a dialog + // is up. + while (true) { + const active = tabManager.activeTab; + const dirty = active?.isDirty + ? active + : tabManager.tabs.find((t) => t.isDirty); + if (!dirty) break; + tabManager.setActive(dirty.id); + await tick(); + if (!(await canCloseTab(dirty.id))) return; + // Resolved tabs (saved, or reverted by Don't Save) + // stay open for the window-state snapshot when + // restore is enabled; untitled tabs have nothing to + // restore, and with restore off the red button + // closes tabs one by one. + if (!settings.restoreStateOnReopen || dirty.path === '') { + tabManager.closeTab(dirty.id); + } } + } finally { + isCloseWalkActive = false; } } From e6f3776fafb804b3d2c0874ad8be89045e65fcbf Mon Sep 17 00:00:00 2001 From: PathGao <42336971+PathGao@users.noreply.github.com> Date: Mon, 13 Jul 2026 01:26:16 +0900 Subject: [PATCH 4/6] fix: keep v2 window-state snapshots invisible to legacy builds MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A legacy build restoring a v2 window-state snapshot reconstructs tabs whose rawContent is undefined (it expects full-tab snapshots). Its editor then fails to swap buffers on tab switch and attributes the previous tab's still-visible content to the newly active tab — and auto-save writes that misattributed content to disk. Observed live: after a v2-format close, an older build showed 4.md's content under the 3.md tab; one edit event away from corrupting 3.md on disk. Write v2 snapshots under their own key (savedTabsDataV2) and remove the legacy key on every write, so an older build sharing the same storage container never sees a format it cannot restore. Startup reads the v2 key first and falls back to the legacy key once for migration; explicit exit clears both. Co-Authored-By: Claude Opus 4.8 --- scripts/windowClosePerTab.test.ts | 4 ++-- scripts/windowStateRestore.test.ts | 25 +++++++++++++++++++-- src/lib/MarkdownViewer.svelte | 35 ++++++++++++++++++++++-------- 3 files changed, 51 insertions(+), 13 deletions(-) diff --git a/scripts/windowClosePerTab.test.ts b/scripts/windowClosePerTab.test.ts index 04142523..c71ceae3 100644 --- a/scripts/windowClosePerTab.test.ts +++ b/scripts/windowClosePerTab.test.ts @@ -69,9 +69,9 @@ test('the walk starts from the tab the user is already looking at', () => { assert.match(handler, /active\?\.isDirty\s*\?\s*active\s*:\s*tabManager\.tabs\.find\(\(t\) => t\.isDirty\)/); }); -test('the restore-on-reopen branch is untouched original behavior', () => { +test('the restore-on-reopen branch persists window state via the shared helper', () => { const handler = closeHandler(); - assert.match(handler, /localStorage\.setItem\('savedTabsData', stateStr\);/); + assert.match(handler, /persistWindowState\(\);/); // no durable-write experiment left behind assert.doesNotMatch(viewer, /saveSessionState|sessionState\.js/); }); diff --git a/scripts/windowStateRestore.test.ts b/scripts/windowStateRestore.test.ts index 2e3cabeb..b2268e73 100644 --- a/scripts/windowStateRestore.test.ts +++ b/scripts/windowStateRestore.test.ts @@ -43,7 +43,7 @@ test('restoreState rebuilds clean tabs and drops legacy untitled entries', () => }); test('startup restore reads content from disk, not from the snapshot', () => { - const init = slice(viewer, "localStorage.getItem('savedTabsData')", 'urlParams'); + const init = slice(viewer, 'localStorage.getItem(WINDOW_STATE_KEY)', 'urlParams'); assert.match(init, /read_file_content/); // a missing file drops its tab instead of restoring a ghost assert.match(init, /closeTab\(/); @@ -59,12 +59,33 @@ test('the close flow resolves dirty tabs before serializing window state', () => const handlerStart = viewer.indexOf('appWindow.onCloseRequested'); const handler = viewer.slice(handlerStart, viewer.indexOf('onDragDropEvent', handlerStart)); const walk = handler.indexOf('canCloseTab(dirty.id)'); - const persist = handler.indexOf("localStorage.setItem('savedTabsData'"); + const persist = handler.indexOf('persistWindowState()'); assert.ok(walk !== -1, 'per-tab walk not found'); assert.ok(persist !== -1, 'window-state serialization not found'); assert.ok(walk < persist, 'dirty tabs must be resolved before the snapshot is written'); }); +test('v2 snapshots live under their own key, invisible to legacy builds', () => { + // An older build restoring a v2 snapshot ends up with undefined tab + // content; its editor then attributes a stale buffer to the wrong tab and + // auto-save writes it to disk. Separate keys keep the formats apart. + const helper = viewer.slice(viewer.indexOf('function persistWindowState')); + const scope = helper.slice(0, helper.indexOf('\n\t}')); + assert.match(scope, /setItem\(WINDOW_STATE_KEY/); + assert.match(scope, /removeItem\(LEGACY_STATE_KEY\)/); + assert.match(viewer, /const WINDOW_STATE_KEY = 'savedTabsDataV2';/); + // startup prefers the v2 key and falls back to legacy for migration + assert.match( + viewer, + /localStorage\.getItem\(WINDOW_STATE_KEY\) \?\? localStorage\.getItem\(LEGACY_STATE_KEY\)/, + ); + // explicit exit clears both + const exitFn = viewer.slice(viewer.indexOf('async function appExit')); + const exitScope = exitFn.slice(0, exitFn.indexOf('\n\t}')); + assert.match(exitScope, /removeItem\(WINDOW_STATE_KEY\)/); + assert.match(exitScope, /removeItem\(LEGACY_STATE_KEY\)/); +}); + test('with restore enabled resolved titled tabs stay open for the snapshot', () => { const handlerStart = viewer.indexOf('appWindow.onCloseRequested'); const handler = viewer.slice(handlerStart, viewer.indexOf('onDragDropEvent', handlerStart)); diff --git a/src/lib/MarkdownViewer.svelte b/src/lib/MarkdownViewer.svelte index 057e27cb..e6efec60 100644 --- a/src/lib/MarkdownViewer.svelte +++ b/src/lib/MarkdownViewer.svelte @@ -343,6 +343,24 @@ import { t } from './utils/i18n.js'; // close request from starting a competing walk. let isCloseWalkActive = false; + // v2 window-state snapshots live under their own key, and the legacy key + // is removed on every write: an older Markpad build restoring a v2 + // snapshot it cannot understand ends up with undefined tab content, and + // its editor then attributes a stale buffer to the wrong tab — which + // auto-save happily writes to disk. Keeping the formats on separate keys + // makes old and new builds invisible to each other. + const WINDOW_STATE_KEY = 'savedTabsDataV2'; + const LEGACY_STATE_KEY = 'savedTabsData'; + + function persistWindowState() { + try { + localStorage.setItem(WINDOW_STATE_KEY, tabManager.serializeState()); + localStorage.removeItem(LEGACY_STATE_KEY); + } catch (e) { + console.error('Failed to save state on close:', e); + } + } + async function appExit() { if (settings.restoreStateOnReopen) { const hasUnsaved = tabManager.tabs.some((t) => t.isDirty || (t.path === '' && t.rawContent.trim() !== '')); @@ -354,7 +372,8 @@ import { t } from './utils/i18n.js'; }); if (response !== 'discard') return; } - localStorage.removeItem('savedTabsData'); + localStorage.removeItem(WINDOW_STATE_KEY); + localStorage.removeItem(LEGACY_STATE_KEY); isForceExiting = true; } appWindow.close(); @@ -1698,7 +1717,7 @@ import { t } from './utils/i18n.js'; async function destroyWindowAfterTabsClosed() { if (settings.restoreStateOnReopen) { - localStorage.setItem('savedTabsData', tabManager.serializeState()); + persistWindowState(); } await appWindow.destroy(); @@ -2373,7 +2392,10 @@ import { t } from './utils/i18n.js'; const appMode = (await invoke('get_app_mode')) as any; if (settings.restoreStateOnReopen) { - const savedData = localStorage.getItem('savedTabsData'); + // v2 key first; the legacy key is only read for one-time + // migration of a snapshot written by an older build. + const savedData = + localStorage.getItem(WINDOW_STATE_KEY) ?? localStorage.getItem(LEGACY_STATE_KEY); if (savedData) { tabManager.restoreState(savedData); // The snapshot carries window state only — content always @@ -2601,12 +2623,7 @@ import { t } from './utils/i18n.js'; // Session is clean now; record the window state for restore. if (settings.restoreStateOnReopen) { - try { - const stateStr = tabManager.serializeState(); - localStorage.setItem('savedTabsData', stateStr); - } catch (e) { - console.error('Failed to save state on close:', e); - } + persistWindowState(); } // If we intercepted the close to run the review, re-trigger From 85c129a6fdffa9a9f7098029f978f20ae5ae53fa Mon Sep 17 00:00:00 2001 From: PathGao <42336971+PathGao@users.noreply.github.com> Date: Mon, 13 Jul 2026 01:38:31 +0900 Subject: [PATCH 5/6] feat: number untitled tabs so close dialogs can name them With two untitled tabs both called "Untitled", the per-tab unsaved-changes dialog at window close could not tell the user which tab it was asking about. Give untitled tabs numbered titles ("Untitled 1", "Untitled 2", reusing the smallest free number), so the tab strip and every dialog naming a tab become unambiguous. A legacy unnumbered title counts as slot 1; localized bases work unchanged. Co-Authored-By: Claude Opus 4.8 --- scripts/untitledTitle.test.ts | 45 ++++++++++++++++++++++++++++++++++ src/lib/stores/tabs.svelte.ts | 13 ++++++++-- src/lib/utils/untitledTitle.ts | 24 ++++++++++++++++++ 3 files changed, 80 insertions(+), 2 deletions(-) create mode 100644 scripts/untitledTitle.test.ts create mode 100644 src/lib/utils/untitledTitle.ts diff --git a/scripts/untitledTitle.test.ts b/scripts/untitledTitle.test.ts new file mode 100644 index 00000000..f87b283a --- /dev/null +++ b/scripts/untitledTitle.test.ts @@ -0,0 +1,45 @@ +import assert from 'node:assert/strict'; +import { readFileSync } from 'node:fs'; +import test from 'node:test'; + +import { nextUntitledTitle } from '../src/lib/utils/untitledTitle.js'; + +// Untitled tabs get distinct numbered titles ("Untitled 1", "Untitled 2", …) +// so the tab strip — and the per-tab close dialogs — can name exactly which +// tab they are talking about. Two dirty untitled tabs used to both be called +// "Untitled", making the close dialog ambiguous. + +test('the first untitled tab is number 1', () => { + assert.equal(nextUntitledTitle([], 'Untitled'), 'Untitled 1'); +}); + +test('numbers increment past the ones in use', () => { + assert.equal(nextUntitledTitle(['Untitled 1', 'Untitled 2'], 'Untitled'), 'Untitled 3'); +}); + +test('freed numbers are reused (smallest available wins)', () => { + assert.equal(nextUntitledTitle(['Untitled 1', 'Untitled 3'], 'Untitled'), 'Untitled 2'); +}); + +test('a legacy unnumbered title occupies slot 1', () => { + assert.equal(nextUntitledTitle(['Untitled'], 'Untitled'), 'Untitled 2'); +}); + +test('titled tabs and lookalike names are ignored', () => { + assert.equal( + nextUntitledTitle(['notes.md', 'Untitled draft', 'Untitled 1x'], 'Untitled'), + 'Untitled 1', + ); +}); + +test('works with localized bases', () => { + assert.equal(nextUntitledTitle(['无标题 1'], '无标题'), '无标题 2'); +}); + +test('new tabs are created with numbered untitled titles', () => { + const tabs = readFileSync('src/lib/stores/tabs.svelte.ts', 'utf8'); + assert.match(tabs, /nextUntitledTitle\(/); + // both creation paths go through the helper + const addNewTab = tabs.slice(tabs.indexOf('addNewTab()'), tabs.indexOf('addHomeTab()')); + assert.match(addNewTab, /nextUntitledTitle\(/); +}); diff --git a/src/lib/stores/tabs.svelte.ts b/src/lib/stores/tabs.svelte.ts index ec8226c4..982362f5 100644 --- a/src/lib/stores/tabs.svelte.ts +++ b/src/lib/stores/tabs.svelte.ts @@ -1,4 +1,5 @@ import { t } from '../utils/i18n.js'; +import { nextUntitledTitle } from '../utils/untitledTitle.js'; import { settings } from './settings.svelte.js'; import { canGoBackInHistory, @@ -132,7 +133,12 @@ class TabManager { addTab(path: string, content: string = '') { const id = crypto.randomUUID(); - const filename = path.split('\\').pop()?.split('/').pop() || t('tabs.untitled', settings.language); + const filename = + path.split('\\').pop()?.split('/').pop() || + nextUntitledTitle( + this.tabs.map((tab) => tab.title), + t('tabs.untitled', settings.language), + ); const fileHistory = createFileHistory(path, content); this.tabs.push({ @@ -165,7 +171,10 @@ class TabManager { this.tabs.push({ id, path: '', - title: t('tabs.untitled', settings.language), + title: nextUntitledTitle( + this.tabs.map((tab) => tab.title), + t('tabs.untitled', settings.language), + ), content, rawContent: content, originalContent: content, diff --git a/src/lib/utils/untitledTitle.ts b/src/lib/utils/untitledTitle.ts new file mode 100644 index 00000000..042d7b05 --- /dev/null +++ b/src/lib/utils/untitledTitle.ts @@ -0,0 +1,24 @@ +/** + * Numbered titles for untitled tabs ("Untitled 1", "Untitled 2", …). + * + * Untitled tabs used to share one identical title, so any UI that names a + * tab — the tab strip, and especially the per-tab unsaved-changes dialog at + * window close — could not tell the user which tab it was talking about. + * The smallest free number is reused, matching common editor behavior. + */ +export function nextUntitledTitle(existingTitles: readonly string[], base: string): string { + const used = new Set(); + for (const title of existingTitles) { + if (title === base) { + // A legacy unnumbered tab occupies slot 1. + used.add(1); + continue; + } + if (!title.startsWith(base + ' ')) continue; + const suffix = title.slice(base.length + 1); + if (/^[1-9][0-9]*$/.test(suffix)) used.add(Number(suffix)); + } + let n = 1; + while (used.has(n)) n++; + return `${base} ${n}`; +} From 7c0e76dbc8be14f65ee590e4678e584d50074e6a Mon Sep 17 00:00:00 2001 From: PathGao <42336971+PathGao@users.noreply.github.com> Date: Mon, 13 Jul 2026 01:46:12 +0900 Subject: [PATCH 6/6] fix: walk dirty tabs in tab-strip order and name them in Save As MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The close walk preferred the active tab, then wrapped around the strip — starting on the third of three tabs produced the sequence 3, 1, 2, which reads as random. Walk strictly left to right instead: numbered untitled titles already keep the dialog unambiguous, so predictability wins over avoiding one highlight jump. Also prefill the untitled Save As dialog with the numbered tab title so the save panel itself says which tab is being saved. Co-Authored-By: Claude Opus 4.8 --- scripts/windowClosePerTab.test.ts | 13 +++++++++++-- src/lib/MarkdownViewer.svelte | 19 +++++++++---------- 2 files changed, 20 insertions(+), 12 deletions(-) diff --git a/scripts/windowClosePerTab.test.ts b/scripts/windowClosePerTab.test.ts index c71ceae3..dbcad0cb 100644 --- a/scripts/windowClosePerTab.test.ts +++ b/scripts/windowClosePerTab.test.ts @@ -64,9 +64,18 @@ test('a second close request cannot start a competing walk', () => { assert.match(handler, /finally \{\s*isCloseWalkActive = false;\s*\}/); }); -test('the walk starts from the tab the user is already looking at', () => { +test('the walk proceeds in strict tab-strip order', () => { const handler = closeHandler(); - assert.match(handler, /active\?\.isDirty\s*\?\s*active\s*:\s*tabManager\.tabs\.find\(\(t\) => t\.isDirty\)/); + // Predictable left-to-right order: always the first dirty tab in the + // array; no active-first shortcut that made the sequence look random. + assert.match(handler, /const dirty = tabManager\.tabs\.find\(\(t\) => t\.isDirty\);/); + assert.doesNotMatch(handler, /active\?\.isDirty/); +}); + +test('the untitled save dialog prefills the numbered tab title', () => { + const fn = viewer.slice(viewer.indexOf('async function saveContent')); + const scope = fn.slice(0, fn.indexOf('async function saveContentAs')); + assert.match(scope, /defaultPath: tab\.title/); }); test('the restore-on-reopen branch persists window state via the shared helper', () => { diff --git a/src/lib/MarkdownViewer.svelte b/src/lib/MarkdownViewer.svelte index e6efec60..051d9c38 100644 --- a/src/lib/MarkdownViewer.svelte +++ b/src/lib/MarkdownViewer.svelte @@ -1449,12 +1449,14 @@ import { t } from './utils/i18n.js'; let targetPath = tab.path; if (!targetPath) { - // Special handling for new (untitled) files + // Special handling for new (untitled) files. Prefill the numbered + // tab title so the dialog itself names which tab is being saved. const selected = await save({ filters: [ { name: 'Markdown', extensions: ['md'] }, { name: 'All Files', extensions: ['*'] }, ], + defaultPath: tab.title, }); if (selected) { targetPath = selected; @@ -2593,16 +2595,13 @@ import { t } from './utils/i18n.js'; // tabs one at a time — activate each and run the same // localized unsaved-changes dialog a single tab close // shows. Cancel stops the walk and keeps the window - // open. Prefer the tab the user is already looking at - // so the highlight only jumps when it has to, and - // re-find every round — a save can leave a tab dirty - // again (TOCTOU) and tabs can change while a dialog - // is up. + // open. Strict tab-strip order (left to right) so the + // sequence is predictable; numbered untitled titles + // let the dialog name each tab. Re-find every round — + // a save can leave a tab dirty again (TOCTOU) and + // tabs can change while a dialog is up. while (true) { - const active = tabManager.activeTab; - const dirty = active?.isDirty - ? active - : tabManager.tabs.find((t) => t.isDirty); + const dirty = tabManager.tabs.find((t) => t.isDirty); if (!dirty) break; tabManager.setActive(dirty.id); await tick();