From 2ccdd554664ade5aad15425bf5962de2e65059d2 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Mon, 20 Jul 2026 20:11:38 -0700 Subject: [PATCH] fix(editor): preserve failed save result --- .../src/components/editor/ChangesModeView.tsx | 2 +- .../src/components/editor/EditorContent.tsx | 8 ++-- .../src/components/editor/EditorPanel.tsx | 21 ++++------ .../components/editor/EditorPanelShell.tsx | 4 +- .../src/components/editor/IpynbViewer.tsx | 7 +++- .../editor/editor-file-save-attempt.test.ts | 42 +++++++++++++++++++ .../editor/editor-file-save-attempt.ts | 20 +++++++++ .../editor/editor-save-failure-notice.test.ts | 36 ---------------- .../editor/editor-save-failure-notice.ts | 15 ------- .../editor/useEditorCmdSaveRequest.ts | 2 +- .../editor/useMarkdownDocuments.test.ts | 23 ++++++++++ .../components/editor/useMarkdownDocuments.ts | 20 +++++++-- 12 files changed, 123 insertions(+), 77 deletions(-) create mode 100644 src/renderer/src/components/editor/editor-file-save-attempt.test.ts create mode 100644 src/renderer/src/components/editor/editor-file-save-attempt.ts delete mode 100644 src/renderer/src/components/editor/editor-save-failure-notice.test.ts delete mode 100644 src/renderer/src/components/editor/editor-save-failure-notice.ts create mode 100644 src/renderer/src/components/editor/useMarkdownDocuments.test.ts diff --git a/src/renderer/src/components/editor/ChangesModeView.tsx b/src/renderer/src/components/editor/ChangesModeView.tsx index 27d2ce943b3..9fac8364e80 100644 --- a/src/renderer/src/components/editor/ChangesModeView.tsx +++ b/src/renderer/src/components/editor/ChangesModeView.tsx @@ -34,7 +34,7 @@ export function ChangesModeView({ viewStateScopeId: string diffViewStateKey: string onContentChange: (content: string) => void - onSave: (content: string) => Promise + onSave: (content: string) => Promise }): React.JSX.Element { if (!dc) { return ( diff --git a/src/renderer/src/components/editor/EditorContent.tsx b/src/renderer/src/components/editor/EditorContent.tsx index e6ee0f33257..270622fdaf3 100644 --- a/src/renderer/src/components/editor/EditorContent.tsx +++ b/src/renderer/src/components/editor/EditorContent.tsx @@ -43,7 +43,7 @@ const IpynbViewer = lazy(() => import('./IpynbViewer')) // Why: module-level for a stable no-op identity so read-only tabs don't rebuild callbacks each render. const noopEditorContentChange = (_content: string): void => {} -const noopEditorSave = (_content: string): void => {} +const noopEditorSave = async (_content: string): Promise => false export function getMarkdownSourceLineOffset(frontMatterRaw: string): number { let offset = 0 @@ -164,8 +164,8 @@ export function EditorContent({ handleContentChange: (content: string) => void handleContentChangeForFile: (file: OpenFile, content: string) => void handleDirtyStateHint: (dirty: boolean) => void - handleSave: (content: string) => Promise - handleSaveForFile: (file: OpenFile, content: string) => Promise + handleSave: (content: string) => Promise + handleSaveForFile: (file: OpenFile, content: string) => Promise reloadContent: (file: OpenFile) => void }): React.JSX.Element { const editorViewStateKey = @@ -376,7 +376,7 @@ export function EditorContent({ : handleContentChange const onSaveWithFm = fm - ? (body: string): Promise => md.mdSave(prependFrontMatter(fm.raw, body)) + ? (body: string): Promise => md.mdSave(prependFrontMatter(fm.raw, body)) : md.mdSave return ( diff --git a/src/renderer/src/components/editor/EditorPanel.tsx b/src/renderer/src/components/editor/EditorPanel.tsx index c264553d3ee..932806f58f4 100644 --- a/src/renderer/src/components/editor/EditorPanel.tsx +++ b/src/renderer/src/components/editor/EditorPanel.tsx @@ -6,7 +6,6 @@ import { openFilePreviewToSide } from '@/lib/file-preview' import { getEditorHeaderCopyState } from './editor-header' import { isLocalPathOpenBlocked, showLocalPathOpenBlockedToast } from '@/lib/local-path-open-guard' import { settingsForRuntimeOwner } from '@/runtime/runtime-rpc-client' -import { requestEditorFileSave } from './editor-autosave' import { exportActiveMarkdownToPdf } from './export-active-markdown' import type { EditorToggleValue } from './EditorViewToggle' import { EditorPanelShell } from './EditorPanelShell' @@ -24,7 +23,7 @@ import { selectEditorPanelGitStatusEntries } from './editor-panel-git-entry-selector' import { createEditorPanelDraftSelector } from './editor-panel-draft-selector' -import { notifyEditorSaveFailure } from './editor-save-failure-notice' +import { attemptEditorFileSave } from './editor-file-save-attempt' function EditorPanelInner({ activeFileId: activeFileIdProp, @@ -174,9 +173,9 @@ function EditorPanelInner({ ) const handleSaveForFile = useCallback( - async (file: typeof activeFile, content: string) => { + async (file: typeof activeFile, content: string): Promise => { if (!file) { - return + return false } const saveTargetFile = file.mode === 'markdown-preview' @@ -186,24 +185,20 @@ function EditorPanelInner({ ) ?? null) : file if (!saveTargetFile) { - return + return false } if (saveTargetFile.isUntitled) { requestRenameForFile(saveTargetFile.id) - return - } - try { - await requestEditorFileSave({ fileId: saveTargetFile.id, fallbackContent: content }) - } catch (error) { - notifyEditorSaveFailure(error) + return false } + return attemptEditorFileSave({ fileId: saveTargetFile.id, fallbackContent: content }) }, [openFiles, requestRenameForFile] ) const handleSave = useCallback( - async (content: string) => { - await handleSaveForFile(activeFile, content) + async (content: string): Promise => { + return handleSaveForFile(activeFile, content) }, [activeFile, handleSaveForFile] ) diff --git a/src/renderer/src/components/editor/EditorPanelShell.tsx b/src/renderer/src/components/editor/EditorPanelShell.tsx index de63cd78b2d..b2636949df3 100644 --- a/src/renderer/src/components/editor/EditorPanelShell.tsx +++ b/src/renderer/src/components/editor/EditorPanelShell.tsx @@ -44,8 +44,8 @@ type EditorPanelShellProps = { onContentChange: (content: string) => void onContentChangeForFile: (file: OpenFile, content: string) => void onDirtyStateHint: (dirty: boolean) => void - onSave: (content: string) => Promise - onSaveForFile: (file: OpenFile, content: string) => Promise + onSave: (content: string) => Promise + onSaveForFile: (file: OpenFile, content: string) => Promise onReloadContent: (file: OpenFile) => void onCloseMarkdownTableOfContents: () => void onCloseRenameDialog: () => void diff --git a/src/renderer/src/components/editor/IpynbViewer.tsx b/src/renderer/src/components/editor/IpynbViewer.tsx index ecb20f2ff63..b71afececb2 100644 --- a/src/renderer/src/components/editor/IpynbViewer.tsx +++ b/src/renderer/src/components/editor/IpynbViewer.tsx @@ -80,7 +80,7 @@ type IpynbViewerProps = { scrollCacheKey: string onContentChange: (content: string) => void onDirtyStateHint: (dirty: boolean) => void - onSave: (content: string) => Promise + onSave: (content: string) => Promise } const NOTEBOOK_SOURCE_COMMIT_DELAY_MS = 400 @@ -843,7 +843,10 @@ export default function IpynbViewer({ setRunError(null) setRunningCellIndex(index) try { - await onSave(latestContent) + const didSave = await onSave(latestContent) + if (!didSave) { + return + } const result = await window.api.notebook.runPythonCell({ filePath, code: cell.source, diff --git a/src/renderer/src/components/editor/editor-file-save-attempt.test.ts b/src/renderer/src/components/editor/editor-file-save-attempt.test.ts new file mode 100644 index 00000000000..f194c33d9e4 --- /dev/null +++ b/src/renderer/src/components/editor/editor-file-save-attempt.test.ts @@ -0,0 +1,42 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +const { requestEditorFileSave, toastError } = vi.hoisted(() => ({ + requestEditorFileSave: vi.fn(), + toastError: vi.fn() +})) +vi.mock('./editor-autosave', () => ({ requestEditorFileSave })) +vi.mock('sonner', () => ({ toast: { error: toastError } })) +// Return the English fallback so the assertion is stable without initializing i18n. +vi.mock('@/i18n/i18n', () => ({ translate: (_key: string, fallback: string) => fallback })) + +import { attemptEditorFileSave } from './editor-file-save-attempt' + +describe('attemptEditorFileSave', () => { + beforeEach(() => { + requestEditorFileSave.mockReset() + toastError.mockClear() + }) + afterEach(() => { + vi.restoreAllMocks() + }) + + it('reports success only after the save request resolves', async () => { + requestEditorFileSave.mockResolvedValue(undefined) + + await expect(attemptEditorFileSave({ fileId: 'file-1' })).resolves.toBe(true) + + expect(toastError).not.toHaveBeenCalled() + }) + + it('surfaces a failed save and reports failure to dependent actions (STA-2027)', async () => { + const consoleError = vi.spyOn(console, 'error').mockImplementation(() => {}) + const cause = new Error('disk full') + requestEditorFileSave.mockRejectedValue(cause) + + await expect(attemptEditorFileSave({ fileId: 'file-1' })).resolves.toBe(false) + + expect(toastError).toHaveBeenCalledTimes(1) + expect(toastError).toHaveBeenCalledWith('Failed to save the file. Please try again.') + expect(consoleError).toHaveBeenCalledWith('[editor] file save failed', cause) + }) +}) diff --git a/src/renderer/src/components/editor/editor-file-save-attempt.ts b/src/renderer/src/components/editor/editor-file-save-attempt.ts new file mode 100644 index 00000000000..9dcd8712942 --- /dev/null +++ b/src/renderer/src/components/editor/editor-file-save-attempt.ts @@ -0,0 +1,20 @@ +import { toast } from 'sonner' +import { translate } from '@/i18n/i18n' +import { requestEditorFileSave, type EditorSaveFileTarget } from './editor-autosave' + +export async function attemptEditorFileSave(target: EditorSaveFileTarget): Promise { + try { + await requestEditorFileSave(target) + return true + } catch (error) { + // Why: shortcut handlers need a non-throwing result, while dependent actions must not treat a rejected write as success. + console.error('[editor] file save failed', error) + toast.error( + translate( + 'auto.components.editor.editor.save.failure.notice.8c59ce5075', + 'Failed to save the file. Please try again.' + ) + ) + return false + } +} diff --git a/src/renderer/src/components/editor/editor-save-failure-notice.test.ts b/src/renderer/src/components/editor/editor-save-failure-notice.test.ts deleted file mode 100644 index 844bc3c2f77..00000000000 --- a/src/renderer/src/components/editor/editor-save-failure-notice.test.ts +++ /dev/null @@ -1,36 +0,0 @@ -import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' - -const { toastError } = vi.hoisted(() => ({ toastError: vi.fn() })) -vi.mock('sonner', () => ({ toast: { error: toastError } })) -// Return the English fallback so the assertion is stable without initializing i18n. -vi.mock('@/i18n/i18n', () => ({ translate: (_key: string, fallback: string) => fallback })) - -import { notifyEditorSaveFailure } from './editor-save-failure-notice' - -describe('notifyEditorSaveFailure', () => { - beforeEach(() => { - toastError.mockClear() - }) - afterEach(() => { - vi.restoreAllMocks() - }) - - it('surfaces a failed save as a toast instead of swallowing it (STA-2027)', () => { - // Why: the regression was silent data loss — a save rejection must reach the user, not vanish. - vi.spyOn(console, 'error').mockImplementation(() => {}) - - notifyEditorSaveFailure(new Error('disk full')) - - expect(toastError).toHaveBeenCalledTimes(1) - expect(toastError).toHaveBeenCalledWith('Failed to save the file. Please try again.') - }) - - it('logs the underlying error for diagnosis', () => { - const consoleError = vi.spyOn(console, 'error').mockImplementation(() => {}) - const cause = new Error('EPERM') - - notifyEditorSaveFailure(cause) - - expect(consoleError).toHaveBeenCalledWith('[editor] file save failed', cause) - }) -}) diff --git a/src/renderer/src/components/editor/editor-save-failure-notice.ts b/src/renderer/src/components/editor/editor-save-failure-notice.ts deleted file mode 100644 index f39a0d8f1f7..00000000000 --- a/src/renderer/src/components/editor/editor-save-failure-notice.ts +++ /dev/null @@ -1,15 +0,0 @@ -import { toast } from 'sonner' -import { translate } from '@/i18n/i18n' - -// Why: requestEditorFileSave rejects on a dropped write, but callers used to swallow it — turning a -// failed save into silent data loss (the edit stays dirty in the buffer with no signal it never -// persisted, STA-2027). Surface it so Cmd/Ctrl+S can't look successful while dropping the save. -export function notifyEditorSaveFailure(error: unknown): void { - console.error('[editor] file save failed', error) - toast.error( - translate( - 'auto.components.editor.editor.save.failure.notice.8c59ce5075', - 'Failed to save the file. Please try again.' - ) - ) -} diff --git a/src/renderer/src/components/editor/useEditorCmdSaveRequest.ts b/src/renderer/src/components/editor/useEditorCmdSaveRequest.ts index f89a0ef24f8..41df5a64037 100644 --- a/src/renderer/src/components/editor/useEditorCmdSaveRequest.ts +++ b/src/renderer/src/components/editor/useEditorCmdSaveRequest.ts @@ -8,7 +8,7 @@ type UseEditorCmdSaveRequestParams = { activeFile: OpenFile | null openFiles: OpenFile[] fileContents: Record - handleSave: (content: string) => Promise + handleSave: (content: string) => Promise } export function useEditorCmdSaveRequest({ diff --git a/src/renderer/src/components/editor/useMarkdownDocuments.test.ts b/src/renderer/src/components/editor/useMarkdownDocuments.test.ts new file mode 100644 index 00000000000..a6d553c0f2b --- /dev/null +++ b/src/renderer/src/components/editor/useMarkdownDocuments.test.ts @@ -0,0 +1,23 @@ +import { describe, expect, it, vi } from 'vitest' +import { saveMarkdownAndRefreshDocuments } from './useMarkdownDocuments' + +describe('saveMarkdownAndRefreshDocuments', () => { + it('does not refresh the document index after a failed write', async () => { + const save = vi.fn().mockResolvedValue(false) + const refresh = vi.fn().mockResolvedValue(undefined) + + await expect(saveMarkdownAndRefreshDocuments('draft', save, refresh)).resolves.toBe(false) + + expect(save).toHaveBeenCalledWith('draft') + expect(refresh).not.toHaveBeenCalled() + }) + + it('refreshes the document index once after a successful write', async () => { + const save = vi.fn().mockResolvedValue(true) + const refresh = vi.fn().mockResolvedValue(undefined) + + await expect(saveMarkdownAndRefreshDocuments('draft', save, refresh)).resolves.toBe(true) + + expect(refresh).toHaveBeenCalledTimes(1) + }) +}) diff --git a/src/renderer/src/components/editor/useMarkdownDocuments.ts b/src/renderer/src/components/editor/useMarkdownDocuments.ts index ae7fca13de6..5e8e5a17fb8 100644 --- a/src/renderer/src/components/editor/useMarkdownDocuments.ts +++ b/src/renderer/src/components/editor/useMarkdownDocuments.ts @@ -17,6 +17,19 @@ type OpenMarkdownDocumentOptions = { anchor?: string | null } +export async function saveMarkdownAndRefreshDocuments( + content: string, + save: (content: string) => Promise, + refresh: () => Promise +): Promise { + const didSave = await save(content) + if (!didSave) { + return false + } + await refresh() + return true +} + type UseMarkdownDocumentsResult = { markdownDocuments: MarkdownDocument[] openMarkdownDocument: ( @@ -31,14 +44,14 @@ type UseMarkdownDocumentsResult = { options?: OpenMarkdownDocumentOptions ) => Promise } - mdSave: (content: string) => Promise + mdSave: (content: string) => Promise } export function useMarkdownDocuments( activeFile: OpenFile, isMarkdown: boolean, viewMode: MarkdownViewMode, - onSave: (content: string) => Promise + onSave: (content: string) => Promise ): UseMarkdownDocumentsResult { const worktreeId = activeFile.worktreeId // Why: PTY activity replaces worktree metadata; only a routing-path change @@ -179,7 +192,8 @@ export function useMarkdownDocuments( ) const mdSave = useCallback( - (content: string) => onSave(content).then(() => refreshMarkdownDocuments(true)), + (content: string) => + saveMarkdownAndRefreshDocuments(content, onSave, () => refreshMarkdownDocuments(true)), [onSave, refreshMarkdownDocuments] )