From 0848dbfcc48cad36de5ce31a16735d2b43934be9 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Tue, 20 Oct 2026 05:00:00 -0700 Subject: [PATCH] fix(editor): disk-checked save follow-ups: repeatable stale reloads, Markdown whitespace, Undo of Reload (#31113) * fix(editor): follow-ups to disk-checked saves: repeatable stale reloads, Markdown whitespace, Undo of Reload - A stale cached copy can reload again once the disk baseline moves or a mount sees fresh text. - A clean Markdown tab keeps a trailing-whitespace-only draft across a reload of the same text. - Undo of Reload re-reads the file's tabs, so typing in a tab that joined after the reload wins. * fix(editor): Undo of Reload skips tabs that left the model; keep whitespace drafts only when disk is unchanged --- .../editor/ExternalFileChangeBanner.tsx | 11 ++-- .../editor/editor-clean-tab-disk-baseline.ts | 27 ++++++++-- .../editor-clean-tab-draft-invariant.test.ts | 23 +++++++- .../editor/editor-save-shared-model.test.ts | 42 +++++++++++++++ .../editor/editor-stale-mount-guard.ts | 15 ++++-- ...e-monaco-editor-mount.stale-cache.test.tsx | 52 +++++++++++++++++-- .../editor/use-monaco-editor-mount.ts | 6 ++- 7 files changed, 158 insertions(+), 18 deletions(-) diff --git a/src/renderer/src/components/editor/ExternalFileChangeBanner.tsx b/src/renderer/src/components/editor/ExternalFileChangeBanner.tsx index e5dd070a22c..c72912007da 100644 --- a/src/renderer/src/components/editor/ExternalFileChangeBanner.tsx +++ b/src/renderer/src/components/editor/ExternalFileChangeBanner.tsx @@ -74,12 +74,15 @@ export function reloadTabContentFromDisk( // signal: the editor content-sync repopulates editorDrafts with the // reloaded content itself, so draft existence proves nothing. // Why every sibling: they share the reloaded text, so typing in any of them is newer work. - const liveSiblings = siblings.flatMap( - (tab) => current.openFiles.find((openFile) => openFile.id === tab.id) ?? [] - ) - if (!liveFile || liveSiblings.some((tab) => tab.isDirty)) { + // Why re-read: a tab that joined the model since the reload (another workspace) counts too. + const modelTabs = liveFile ? editorModelSiblingTabs(current, liveFile) : [] + if (!liveFile || modelTabs.some((tab) => tab.isDirty)) { return } + // Why: a reloaded tab that has since left the model holds its own text; leave it alone. + const liveSiblings = modelTabs.filter((tab) => + siblings.some((reloaded) => reloaded.id === tab.id) + ) if (discardedDiskSignature !== undefined) { // Why: the reload re-stamped the baseline to the new disk content; // restoring the pre-reload signature with the draft keeps the diff --git a/src/renderer/src/components/editor/editor-clean-tab-disk-baseline.ts b/src/renderer/src/components/editor/editor-clean-tab-disk-baseline.ts index 8331d9ec6bf..b9dde358223 100644 --- a/src/renderer/src/components/editor/editor-clean-tab-disk-baseline.ts +++ b/src/renderer/src/components/editor/editor-clean-tab-disk-baseline.ts @@ -1,4 +1,6 @@ -import { useAppStore } from '@/store' +import { useAppStore, type AppState } from '@/store' +import type { OpenFile } from '@/store/slices/editor' +import { isEditorContentUnchanged } from './editor-content-dirty-state' import { getDiskBaselineSignature } from './diff-content-signature' import type { FileContent } from './editor-panel-content-types' import { @@ -9,6 +11,22 @@ import { import { setModelExternalMutation } from './editor-save-rebaseline' import { notifyEditorFileCacheStale } from './editor-autosave' +/** `tab` is the pre-load snapshot, so its baseline is the version the draft was typed over. */ +function clearDraftUnlessWhitespaceOnly(state: AppState, tab: OpenFile, diskContent: string): void { + const draft = state.editorDrafts[tab.id] + if ( + draft !== undefined && + draft !== diskContent && + // Why: if disk moved at all (even whitespace), keeping the draft would undo an outside change. + tab.lastKnownDiskSignature === getDiskBaselineSignature(diskContent) && + tab.language === 'markdown' && + isEditorContentUnchanged(draft, diskContent, true) + ) { + return + } + state.clearEditorDraft(tab.id) +} + // Why: a clean load re-baselines what this tab's future edits are based on; a // dirty tab keeps its baseline (its draft still derives from the older content // the signature was taken over). Best-effort metadata — a failure here must @@ -36,10 +54,11 @@ export function stampCleanTabDiskBaseline(id: string, result: FileContent): void } if (loadedFile) { const moved = setEditorDiskBaseline(state, id, getDiskBaselineSignature(result.content)) - // Why: the model is clean, so any draft left on its tabs is stale text from an older version. - state.clearEditorDraft(id) + // Why: the model is clean, so any draft left on its tabs is stale text from an older version, + // except a Markdown draft that differs from this disk text only in trailing whitespace. + clearDraftUnlessWhitespaceOnly(state, loadedFile, result.content) for (const tab of moved) { - state.clearEditorDraft(tab.id) + clearDraftUnlessWhitespaceOnly(state, tab, result.content) notifyEditorFileCacheStale(tab.id) } } diff --git a/src/renderer/src/components/editor/editor-clean-tab-draft-invariant.test.ts b/src/renderer/src/components/editor/editor-clean-tab-draft-invariant.test.ts index 333b40a803e..cb895bdac7e 100644 --- a/src/renderer/src/components/editor/editor-clean-tab-draft-invariant.test.ts +++ b/src/renderer/src/components/editor/editor-clean-tab-draft-invariant.test.ts @@ -7,6 +7,7 @@ import { __clearSelfWriteRegistryForTests } from './editor-self-write-registry' import { createEditorStore } from './editor-autosave-controller-test-fixture' import { reloadTabContentFromDisk } from './ExternalFileChangeBanner' import { mayPushTabTextIntoModel } from './editor-stale-mount-guard' +import { stampCleanTabDiskBaseline } from './editor-clean-tab-disk-baseline' import { rollBackFailedMobileSave } from '@/runtime/mobile-markdown-save-rollback' import { getDiskBaselineSignature } from '../../../../shared/disk-baseline-signature' import { GUARDED_PATH, stubGuardedDisk } from './editor-save-guarded-disk-fixture' @@ -19,14 +20,14 @@ vi.mock('sonner', () => ({ toast: Object.assign(vi.fn(), { error: vi.fn() }) })) vi.mock('@/lib/connection-context', () => ({ getConnectionIdForFile: () => undefined })) vi.mock('@/store', () => ({ useAppStore: { getState: () => storeRef.current?.getState() } })) -function openCleanTab(disk: string) { +function openCleanTab(disk: string, language = 'typescript') { const store = createEditorStore() storeRef.current = store store.getState().openFile({ filePath: GUARDED_PATH, relativePath: 'file.ts', worktreeId: 'wt-1', - language: 'typescript', + language, mode: 'edit' }) const id = store.getState().openFiles[0].id @@ -112,3 +113,21 @@ it("never restores a clean tab's leftover draft into the model", () => { store.getState().markFileDirty(id, true) expect(mayPushTabTextIntoModel(id)).toBe(true) }) + +it("a reload keeps a clean Markdown tab's trailing-whitespace draft, but not one over older text", () => { + const { store, id } = openCleanTab('# notes', 'markdown') + store.getState().setEditorDraft(id, '# notes\n\n') + stampCleanTabDiskBaseline(id, { content: '# notes', isBinary: false }) + expect(store.getState().editorDrafts[id]).toBe('# notes\n\n') + + stampCleanTabDiskBaseline(id, { content: '# newer notes', isBinary: false }) + expect(store.getState().editorDrafts[id]).toBeUndefined() +}) + +it('a reload drops a whitespace-only Markdown draft when disk moved, even by whitespace alone', () => { + const { store, id } = openCleanTab('# notes', 'markdown') + store.getState().setEditorDraft(id, '# notes\n\n') + // An outside write added a trailing newline; keeping the draft would undo it. + stampCleanTabDiskBaseline(id, { content: '# notes\n', isBinary: false }) + expect(store.getState().editorDrafts[id]).toBeUndefined() +}) diff --git a/src/renderer/src/components/editor/editor-save-shared-model.test.ts b/src/renderer/src/components/editor/editor-save-shared-model.test.ts index 7d17fdd6556..4b440282d1e 100644 --- a/src/renderer/src/components/editor/editor-save-shared-model.test.ts +++ b/src/renderer/src/components/editor/editor-save-shared-model.test.ts @@ -339,6 +339,48 @@ describe('two tabs sharing one model', () => { expect(tab(b.id)?.externalMutation).toBeUndefined() }) + it('Undo of Reload yields to typing in a workspace tab that joined the model after the reload', () => { + const { store, a, b, edit } = openSharedTabs('A') + store.getState().setEditorViewMode(b.id, 'changes') + edit(a.id, 'mine') + const toastMock = vi.mocked(toast) + toastMock.mockClear() + reloadTabContentFromDisk(store.getState().openFiles[0], vi.fn()) + // The other workspace's tab switches to the code view, joining the model, and types. + store.getState().setEditorViewMode(b.id, 'edit') + edit(b.id, 'typed in the joined tab') + + const undo: unknown = toastMock.mock.calls[0]?.[1]?.action + expect(isClickAction(undo)).toBe(true) + if (isClickAction(undo)) { + undo.onClick() + } + + expect(store.getState().editorDrafts[a.id]).toBeUndefined() + expect(store.getState().editorDrafts[b.id]).toBe('typed in the joined tab') + }) + + it('Undo of Reload leaves alone a reloaded tab that left the model and typed', () => { + const { store, a, b, edit, tab } = openSharedTabs('A') + edit(a.id, 'mine') + const toastMock = vi.mocked(toast) + toastMock.mockClear() + reloadTabContentFromDisk(store.getState().openFiles[0], vi.fn()) + // The other workspace's tab switches to the Changes view, leaving the model, and types. + store.getState().setEditorViewMode(b.id, 'changes') + edit(b.id, 'typed in the Changes view') + + const undo: unknown = toastMock.mock.calls[0]?.[1]?.action + expect(isClickAction(undo)).toBe(true) + if (isClickAction(undo)) { + undo.onClick() + } + + expect(store.getState().editorDrafts[a.id]).toBe('mine') + expect(store.getState().editorDrafts[b.id]).toBe('typed in the Changes view') + expect(tab(b.id)?.externalMutation).toBeUndefined() + }) + it('never reaches a Changes-view tab through Keep or Reload', async () => { stubGuardedDisk({ [GUARDED_PATH]: 'B' }) const { store, a, b, edit, tab } = openSharedTabs('A') diff --git a/src/renderer/src/components/editor/editor-stale-mount-guard.ts b/src/renderer/src/components/editor/editor-stale-mount-guard.ts index c0fccc78d39..90aeee061e8 100644 --- a/src/renderer/src/components/editor/editor-stale-mount-guard.ts +++ b/src/renderer/src/components/editor/editor-stale-mount-guard.ts @@ -17,18 +17,25 @@ export function isStaleCachedEditorText(fileId: string, content: string): boolea return baseline !== undefined && getDiskBaselineSignature(content) !== baseline } +// Why keyed by baseline too: once the baseline moves, the same stale text must be able to reload again. const requestedReloads = new Map() -/** Drops a stale cache so it reloads from disk, once per cached text: a reload that lands the same text can't loop. */ +/** Drops a stale cache so it reloads from disk, once per cached text and baseline: a reload that lands the same text can't loop. */ export function requestStaleCacheReload(fileId: string, content: string): void { - const signature = getDiskBaselineSignature(content) - if (requestedReloads.get(fileId) === signature) { + const file = useAppStore.getState()?.openFiles?.find((openFile) => openFile.id === fileId) + const stamp = `${file?.lastKnownDiskSignature ?? ''}\n${getDiskBaselineSignature(content)}` + if (requestedReloads.get(fileId) === stamp) { return } - requestedReloads.set(fileId, signature) + requestedReloads.set(fileId, stamp) notifyEditorFileCacheStale(fileId) } +/** A mount found the tab's text fresh, so an earlier reload settled; a later stale copy may reload again. */ +export function settleStaleCacheReload(fileId: string): void { + requestedReloads.delete(fileId) +} + /** * The one rule for pushing a tab's text into its live model: a clean tab's text (disk-derived, or a * leftover draft) never enters a dirty model. Reload and Overwrite clear the model's edits first. diff --git a/src/renderer/src/components/editor/use-monaco-editor-mount.stale-cache.test.tsx b/src/renderer/src/components/editor/use-monaco-editor-mount.stale-cache.test.tsx index da91d850278..0b780ae5a0c 100644 --- a/src/renderer/src/components/editor/use-monaco-editor-mount.stale-cache.test.tsx +++ b/src/renderer/src/components/editor/use-monaco-editor-mount.stale-cache.test.tsx @@ -9,6 +9,7 @@ import type { MonacoEditorMountParams } from './monaco-editor-mount-params' import { useMonacoEditorMount } from './use-monaco-editor-mount' const filePath = '/repo/file.ts' +const disk = vi.hoisted(() => ({ text: 'B' })) vi.mock('@/store', () => ({ useAppStore: { @@ -22,7 +23,7 @@ vi.mock('@/store', () => ({ id: 'tab-b', filePath: '/repo/file.ts', mode: 'edit', - lastKnownDiskSignature: getDiskBaselineSignature('B') + lastKnownDiskSignature: getDiskBaselineSignature(disk.text) } ] }) @@ -39,9 +40,12 @@ vi.mock('./monaco-editor-input-bindings', () => ({ installMonacoEditorInputBindings: () => ({ disposeInputBindings: vi.fn() }) })) -afterEach(cleanup) +afterEach(() => { + cleanup() + disk.text = 'B' +}) -it('mounts the live model over a stale retained cache, editable, and reloads a model-less stale cache once', () => { +function setupMount() { const params: MonacoEditorMountParams = { fileId: 'tab-b', filePath, @@ -96,6 +100,7 @@ it('mounts the live model over a stale retained cache, editable, and reloads a m const modelText = { value: 'B' } const model = { getValue: () => modelText.value, + getEOL: () => '\n', pushEditOperations: vi.fn(), getFullModelRange: vi.fn() } @@ -117,6 +122,12 @@ it('mounts the live model over a stale retained cache, editable, and reloads a m const editorInstance = mockEditor as unknown as editor.IStandaloneCodeEditor // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: The mocked mount integrations do not access Monaco. const monaco = {} as Parameters[1] + return { params, modelText, model, updateOptions, executeEdits, editorInstance, monaco } +} + +it('mounts the live model over a stale retained cache, editable, and reloads a model-less stale cache once', () => { + const { params, modelText, model, updateOptions, executeEdits, editorInstance, monaco } = + setupMount() const dropped: string[] = [] const onStale = (event: Event): void => { if (event instanceof CustomEvent) { @@ -146,3 +157,38 @@ it('mounts the live model over a stale retained cache, editable, and reloads a m window.removeEventListener(ORCA_EDITOR_FILE_CACHE_STALE_EVENT, onStale) } }) + +it('reloads the same stale text again once the baseline moves or a fresh mount settles the reload', () => { + const { params, modelText, editorInstance, monaco } = setupMount() + const dropped: string[] = [] + const onStale = (event: Event): void => { + if (event instanceof CustomEvent) { + dropped.push(event.detail.fileId) + } + } + window.addEventListener(ORCA_EDITOR_FILE_CACHE_STALE_EVENT, onStale) + const view = renderHook(() => useMonacoEditorMount(params)) + const mountWith = (text: string): void => { + params.contentSync.contentRef.current = text + modelText.value = text + view.result.current(editorInstance, monaco) + } + try { + mountWith('A2 (stale cache)') + mountWith('A2 (stale cache)') + expect(dropped).toHaveLength(1) + + // Another write moved the baseline: the same stale text must reload again. + disk.text = 'C' + mountWith('A2 (stale cache)') + expect(dropped).toHaveLength(2) + + // The reload landed fresh text; stale text returning later reloads again. + mountWith('C') + expect(dropped).toHaveLength(2) + mountWith('A2 (stale cache)') + expect(dropped).toHaveLength(3) + } finally { + window.removeEventListener(ORCA_EDITOR_FILE_CACHE_STALE_EVENT, onStale) + } +}) diff --git a/src/renderer/src/components/editor/use-monaco-editor-mount.ts b/src/renderer/src/components/editor/use-monaco-editor-mount.ts index e7591187169..0e56908db31 100644 --- a/src/renderer/src/components/editor/use-monaco-editor-mount.ts +++ b/src/renderer/src/components/editor/use-monaco-editor-mount.ts @@ -22,7 +22,8 @@ import { import { isStaleCachedEditorText, mayPushTabTextIntoModel, - requestStaleCacheReload + requestStaleCacheReload, + settleStaleCacheReload } from './editor-stale-mount-guard' // Why: binds a freshly created Monaco instance to the app; reads everything through refs so the callback identity stays stable across renders. @@ -112,6 +113,9 @@ export function useMonacoEditorMount(params: MonacoEditorMountParams): OnMount { // Why: see contentRef — reconcile the retained model to the current prop before user interaction (surfaces edits made while unmounted). const modelKey = toEditorModelUri(filePath, modelOwnerKey) const staleCache = isStaleCachedEditorText(fileIdRef.current, contentRef.current) + if (!staleCache) { + settleStaleCacheReload(fileIdRef.current) + } const liveTextDiffers = editorInstance.getModel()?.getValue() !== contentRef.current if ( (staleCache && liveTextDiffers) ||