From 3e3d52b1381c707451355a2b336460323746a839 Mon Sep 17 00:00:00 2001 From: Alex Soffronow Pagonidis <237136924+alex-clickhouse@users.noreply.github.com> Date: Wed, 5 Aug 2026 12:50:32 +0000 Subject: [PATCH] Stop a failed save from looking like a saved one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `saveTaskContent` swallowed its exception and returned void, and `handleSave` cleared `dirty` regardless. So an offline save, a 500, or an expired session all ended the same way a success does: the Save button disappears and the editor looks settled, while the only copy of the text is still in the browser. The user finds out on the next reload. The lost-edit path is worse than the missing button suggests. `dirty` is also what arms the content-sync guard, so clearing it on a failure hands the unsaved text to the next re-render that carries a `content` — the edit is not merely un-persisted, it is gone from the textarea too. The store now resolves a boolean instead of hiding the outcome, and the editor keeps `dirty` set unless the write actually landed, which keeps both the retry and the guard in place. A message next to the button says so, because the detail view had no error surface at all; it clears on the next keystroke, since by then it is describing text the user has moved past. `TaskDetailBody` is the only caller, so the signature change is contained. Errors are reported rather than rethrown: the editor needs to know the write failed so it can hold on to the text, not to handle the error. `updateStatus` and the other store actions swallow errors the same way. Same class of bug, much smaller blast radius — left for its own pass rather than widening this one. Co-Authored-By: Claude Opus 5 --- .../components/Tasks/TaskDetailBody.test.tsx | 93 ++++++++++++++++++- web/src/components/Tasks/TaskDetailBody.tsx | 32 +++++-- web/src/stores/taskStore.ts | 7 +- 3 files changed, 118 insertions(+), 14 deletions(-) diff --git a/web/src/components/Tasks/TaskDetailBody.test.tsx b/web/src/components/Tasks/TaskDetailBody.test.tsx index 3bc9aa1e..636fe5a9 100644 --- a/web/src/components/Tasks/TaskDetailBody.test.tsx +++ b/web/src/components/Tasks/TaskDetailBody.test.tsx @@ -1,8 +1,9 @@ -import { render, screen } from '@testing-library/react'; +import { render, screen, waitFor } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; -import { describe, expect, it, vi } from 'vitest'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import type { MockInstance } from 'vitest'; -import type { Task } from '../../api/client'; +import { api, type Task } from '../../api/client'; import { TaskDetailBody } from './TaskDetailBody'; vi.mock('../../api/client', () => ({ @@ -27,6 +28,7 @@ function task(content: string): Task & { content: string } { } const editor = () => screen.getByPlaceholderText('Task content...'); +const saveButton = () => screen.queryByRole('button', { name: /save/i }); async function startEditing() { await userEvent.click(screen.getByTitle('Edit')); @@ -66,3 +68,88 @@ describe('TaskDetailBody content sync', () => { expect(screen.getByRole('button', { name: /save/i })).toBeInTheDocument(); }); }); + +/** + * A save that fails has to look different from a save that worked. The old + * code cleared `dirty` either way, which hid the Save button and left the + * editor looking settled while the only copy of the text was still in the + * browser — the user found out on the next reload. + */ +describe('TaskDetailBody failed saves', () => { + const updateTask = vi.mocked(api.updateTask); + let consoleError: MockInstance; + + const savedResponse = (content: string) => ({ + task: task(content), task_id: 't1', updated: true, + }); + + beforeEach(() => { + updateTask.mockReset(); + // The store logs the rejection deliberately. Swallow it here so a genuine + // React warning still stands out in the run output. + consoleError = vi.spyOn(console, 'error').mockImplementation(() => {}); + }); + + afterEach(() => { + consoleError.mockRestore(); + }); + + async function typeAndFailToSave(addition = ' plus my notes') { + render(); + await startEditing(); + await userEvent.type(editor(), addition); + await userEvent.click(screen.getByRole('button', { name: /save/i })); + return screen.findByRole('alert'); + } + + it('says the save failed and keeps offering the retry', async () => { + updateTask.mockRejectedValue(new Error('network down')); + + expect(await typeAndFailToSave()).toHaveTextContent(/save failed/i); + + // The edit is still in the box, and still offered for saving. Before the + // fix the button vanished, which read as "saved". + expect(editor()).toHaveValue('# original plus my notes'); + expect(saveButton()).toBeInTheDocument(); + }); + + it('keeps the failed edit safe from an update arriving underneath', async () => { + updateTask.mockRejectedValue(new Error('network down')); + const { rerender } = render(); + await startEditing(); + await userEvent.type(editor(), ' plus my notes'); + await userEvent.click(screen.getByRole('button', { name: /save/i })); + await screen.findByRole('alert'); + + rerender(); + + // `dirty` staying set is what keeps the content-sync guard armed. Clearing + // it on a failure would hand the only copy of the text to the next + // re-render, which is how the edit actually got lost. + expect(editor()).toHaveValue('# original plus my notes'); + }); + + it('clears the failure once a retry succeeds', async () => { + updateTask + .mockRejectedValueOnce(new Error('network down')) + .mockResolvedValueOnce(savedResponse('# original!')); + + await typeAndFailToSave('!'); + await userEvent.click(screen.getByRole('button', { name: /save/i })); + + await waitFor(() => expect(screen.queryByRole('alert')).not.toBeInTheDocument()); + expect(saveButton()).not.toBeInTheDocument(); + }); + + it('clears the failure as soon as the user edits again', async () => { + updateTask.mockRejectedValue(new Error('network down')); + + await typeAndFailToSave('!'); + await userEvent.type(editor(), '?'); + + // Stale complaint about text the user has already moved past, but the + // edit is still unsaved so Save has to stay. + expect(screen.queryByRole('alert')).not.toBeInTheDocument(); + expect(saveButton()).toBeInTheDocument(); + }); +}); diff --git a/web/src/components/Tasks/TaskDetailBody.tsx b/web/src/components/Tasks/TaskDetailBody.tsx index 1bbc16d8..13f9b982 100644 --- a/web/src/components/Tasks/TaskDetailBody.tsx +++ b/web/src/components/Tasks/TaskDetailBody.tsx @@ -23,6 +23,7 @@ export function TaskDetailBody({ task }: { task: Task }) { const [showHistory, setShowHistory] = useState(false); const [localContent, setLocalContent] = useState(''); const [dirty, setDirty] = useState(false); + const [saveError, setSaveError] = useState(false); // Adopt the loaded markdown, but never clobber unsaved edits. // @@ -40,8 +41,12 @@ export function TaskDetailBody({ task }: { task: Task }) { const handleSave = useCallback(async () => { if (!dirty) return; - await saveTaskContent(task.id, localContent); - setDirty(false); + // Keep `dirty` set when the save fails. It is what keeps the Save button + // on screen, so the retry stays available and the editor cannot look + // settled while the content is still only in the browser. + const saved = await saveTaskContent(task.id, localContent); + setSaveError(!saved); + if (saved) setDirty(false); }, [task.id, dirty, localContent, saveTaskContent]); const handleKeyDown = useCallback((e: React.KeyboardEvent) => { @@ -89,13 +94,20 @@ export function TaskDetailBody({ task }: { task: Task }) { {dirty && ( - + <> + {saveError && ( + + Save failed — not saved yet. + + )} + + )}