From e22007dc500924c502a213fce8ab3002bfe2c342 Mon Sep 17 00:00:00 2001 From: Jacob Hagstedt Date: Mon, 14 Sep 2026 22:51:15 +0200 Subject: [PATCH] Detect wish save failures directly instead of inferring them Plan.tsx's handlers swallowed their rejections and onUpdate was typed to return void, so WishEditor could not observe a failed save. It inferred the outcome instead -- comparing the draft it sent against the next wishes prop, and reading "isSaving went false without a match" as failure. That coupled save status to refetch timing and could not tell a network error from a slow refresh. The handlers now return their promise, onUpdate accepts `void | Promise` so void-returning callers still typecheck, and WishEditor awaits and catches the real outcome. The inference machinery (sentDraftRef, prevIsSavingRef and the comparison effect) is removed rather than left alongside, since two sources of truth can disagree. The save/refetch/save loop guard is untouched: it lives in the draft reseed effect, which only fills a row that has no draft yet, and saves only ever originate from an explicit onChange. Added a comment saying so. A failed row now keeps its edit and offers an explicit retry, rather than requiring another edit to retrigger the debounce. Per-row failures report inline only -- reporting them in the page banner too would double-report the same error. Also removes wishEditor.saveButton/savingButton, dead since the per-row Save button was replaced by auto-save. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01RXMhc9nT6sNz1SL4j8dznL --- .../components/planning/WishEditor.test.tsx | 84 +++++++++-- .../src/components/planning/WishEditor.tsx | 136 ++++++++++-------- frontend/src/i18n/en/wishEditor.ts | 3 +- frontend/src/i18n/sv/wishEditor.ts | 3 +- frontend/src/pages/Plan.tsx | 23 +-- frontend/src/pages/plan.css | 7 + 6 files changed, 172 insertions(+), 84 deletions(-) diff --git a/frontend/src/components/planning/WishEditor.test.tsx b/frontend/src/components/planning/WishEditor.test.tsx index 7f5a7a0..78ee74b 100644 --- a/frontend/src/components/planning/WishEditor.test.tsx +++ b/frontend/src/components/planning/WishEditor.test.tsx @@ -543,39 +543,99 @@ describe('WishEditor', () => { expect(onUpdate).not.toHaveBeenCalled() }) - it('shows "Sparar…" while a save is in flight and "Sparat" once the wishes prop confirms it landed', () => { - const { rerender } = setup({ wishes: [mockWish], isSaving: false }) + it('shows "Sparar…" while a save is in flight and "Sparat" once onUpdate resolves', async () => { + const { onUpdate } = setup({ wishes: [mockWish] }) + let resolveUpdate: (() => void) | undefined + onUpdate.mockImplementationOnce( + () => + new Promise((resolve) => { + resolveUpdate = resolve + }), + ) fireEvent.change(screen.getByTestId('wish-weeks-lw1'), { target: { value: '9' } }) advanceAutosave() expect(screen.getByTestId('wish-save-status-lw1')).toHaveTextContent('Sparar') - rerender({ wishes: [{ ...mockWish, targetWeeks: 9 }], isSaving: false }) + await act(async () => { + resolveUpdate?.() + await Promise.resolve() + }) expect(screen.getByTestId('wish-save-status-lw1')).toHaveTextContent('Sparat') advanceAutosave(2000) expect(screen.queryByTestId('wish-save-status-lw1')).not.toBeInTheDocument() }) - it('shows a failure status and keeps the edit when the parent finishes saving without the change landing', () => { - const { onUpdate, rerender } = setup({ wishes: [mockWish], isSaving: false }) + it('shows a failure status and keeps the edit when onUpdate rejects', async () => { + const { onUpdate } = setup({ wishes: [mockWish] }) + onUpdate.mockImplementationOnce(() => Promise.reject(new Error('network error'))) fireEvent.change(screen.getByTestId('wish-weeks-lw1'), { target: { value: '9' } }) advanceAutosave() expect(onUpdate).toHaveBeenCalledTimes(1) - // Parent starts, then finishes the (failed) request; wishes never - // change because the underlying update rejected. - rerender({ wishes: [mockWish], isSaving: true }) - rerender({ wishes: [mockWish], isSaving: false }) + await act(async () => { + await Promise.resolve() + await Promise.resolve() + }) - expect(screen.getByTestId('wish-save-status-lw1')).toHaveTextContent( - 'Kunde inte spara', - ) + expect(screen.getByTestId('wish-save-status-lw1')).toHaveTextContent('Kunde inte spara') // The edit itself must not be discarded from the input. expect(screen.getByTestId('wish-weeks-lw1')).toHaveValue(9) }) + it('treats a void-returning onUpdate (no promise) as an immediate success — backward compatibility', async () => { + // setup()'s default onUpdate is a plain `vi.fn()`, which returns + // `undefined` — exactly what a caller that hasn't adopted the + // `Promise` return would produce. + setup({ wishes: [mockWish] }) + + fireEvent.change(screen.getByTestId('wish-weeks-lw1'), { target: { value: '9' } }) + advanceAutosave() + + await act(async () => { + await Promise.resolve() + }) + + expect(screen.getByTestId('wish-save-status-lw1')).toHaveTextContent('Sparat') + }) + + describe('retry', () => { + it('re-sends the still-pending draft and clears the error once it succeeds', async () => { + const { onUpdate } = setup({ wishes: [mockWish] }) + onUpdate.mockImplementationOnce(() => Promise.reject(new Error('boom'))) + + fireEvent.change(screen.getByTestId('wish-weeks-lw1'), { target: { value: '9' } }) + advanceAutosave() + + await act(async () => { + await Promise.resolve() + await Promise.resolve() + }) + expect(screen.getByTestId('wish-save-status-lw1')).toHaveTextContent('Kunde inte spara') + expect(screen.queryByTestId('wish-retry-lw1')).toBeInTheDocument() + + onUpdate.mockImplementationOnce(() => Promise.resolve()) + fireEvent.click(screen.getByTestId('wish-retry-lw1')) + expect(screen.getByTestId('wish-save-status-lw1')).toHaveTextContent('Sparar') + + await act(async () => { + await Promise.resolve() + }) + + expect(onUpdate).toHaveBeenCalledTimes(2) + expect(onUpdate).toHaveBeenLastCalledWith('lw1', expect.objectContaining({ targetWeeks: 9 })) + expect(screen.getByTestId('wish-save-status-lw1')).toHaveTextContent('Sparat') + expect(screen.queryByTestId('wish-retry-lw1')).not.toBeInTheDocument() + }) + + it('does not show a retry button while a row is saving or has no error', () => { + setup({ wishes: [mockWish] }) + expect(screen.queryByTestId('wish-retry-lw1')).not.toBeInTheDocument() + }) + }) + it('cleans up a pending debounce timer on unmount so it never fires', () => { const { onUpdate, unmount } = setup({ wishes: [mockWish] }) diff --git a/frontend/src/components/planning/WishEditor.tsx b/frontend/src/components/planning/WishEditor.tsx index dea3eb2..a9a6d38 100644 --- a/frontend/src/components/planning/WishEditor.tsx +++ b/frontend/src/components/planning/WishEditor.tsx @@ -21,11 +21,14 @@ * * Edits auto-save per row (800ms debounce, collapsing rapid edits into one * call) instead of requiring an explicit Save click — see `scheduleSave`/ - * `performSave` below. There is deliberately no promise/return value coming - * back from `onUpdate` (see `WishEditorProps`), so success/failure of a - * given row's save is *inferred*: a match between what was sent and the - * freshly-arrived `wishes` prop means success; the parent's `isSaving` flag - * dropping back to false without that match means the save failed. + * `performSave` below. `onUpdate` may return a promise (see + * `WishEditorProps`); `performSave` awaits it directly and drives the row's + * `rowStatus` off the real outcome — a resolved promise (or a synchronous, + * non-throwing `void` return, for a caller that hasn't adopted the promise + * form) means success, a rejection means failure. A failed row keeps its + * draft (nothing is discarded) and gets an explicit retry action that + * re-sends the same still-pending draft via `performSave` directly, + * bypassing the debounce. */ import { ReactNode, useEffect, useRef, useState } from 'react' import type { @@ -42,9 +45,9 @@ export interface WishEditorProps { wishes: LeaveWish[] members: PersonalData[] isSaving?: boolean - onAdd: (payload: LeaveWishCreateRequest) => void - onUpdate: (id: string, patch: LeaveWishUpdateRequest) => void - onRemove: (id: string) => void + onAdd: (payload: LeaveWishCreateRequest) => void | Promise + onUpdate: (id: string, patch: LeaveWishUpdateRequest) => void | Promise + onRemove: (id: string) => void | Promise } type WindowMode = 'fixed' | 'flexible' @@ -272,13 +275,18 @@ export function WishEditor({ // Per-row debounce timers (auto-save) and per-row "Sparat" auto-clear timers. const timersRef = useRef>>({}) const savedTimersRef = useRef>>({}) - // The exact draft snapshot sent to `onUpdate` for a row still awaiting a - // result, keyed by wish id — cleared once that row resolves (success or - // failure). Used to detect success by comparing against the next `wishes` - // the parent hands down. - const sentDraftRef = useRef>({}) - const prevIsSavingRef = useRef(isSaving) + // Guards the async continuation after `await onUpdate(...)` in `performSave` + // from setting state once this component has unmounted. + const isMountedRef = useRef(true) + // THE SAVE -> REFETCH -> SAVE LOOP GUARD: only fills in a draft for a wish + // id that doesn't have one yet (`prev[wish.id] ?? draftFromWish(...)`) — + // an existing draft is never overwritten by data arriving from `wishes`. + // A parent refetch after a successful save (or any other reason `wishes` + // changes identity) can therefore never clobber a live edit back to the + // server's value. Since `scheduleSave`/`performSave` only ever run from + // `updateDraft`, which only fires from an explicit user `onChange`, a + // `wishes` refresh has no path back into triggering another save. useEffect(() => { setDrafts((prev) => { const next: Record = {} @@ -289,54 +297,18 @@ export function WishEditor({ }) }, [wishes, t]) - // Resolves in-flight auto-saves: a row whose sent draft now matches the - // freshly-arrived `wishes` succeeded; a row still unresolved when the - // parent's `isSaving` flag drops back to false failed. Checked in that - // order so a same-tick success always wins over a stale "just finished" - // read of `isSaving`. This is the guard against a save -> refetch -> save - // loop: it only ever *reads* `wishes`/`isSaving` to update `rowStatus` — - // it never touches `drafts`, so a parent refetch can never trigger another - // scheduleSave call. - useEffect(() => { - const wasSaving = prevIsSavingRef.current - prevIsSavingRef.current = isSaving - - for (const id of Object.keys(sentDraftRef.current)) { - const sentDraft = sentDraftRef.current[id] - const wish = wishes.find((w) => w.id === id) - const succeeded = - wish !== undefined && sentDraft !== undefined && draftEqualsWish(sentDraft, wish, t) - - if (succeeded) { - delete sentDraftRef.current[id] - setRowStatus((prev) => ({ ...prev, [id]: 'saved' })) - const existingSavedTimer = savedTimersRef.current[id] - if (existingSavedTimer) clearTimeout(existingSavedTimer) - savedTimersRef.current[id] = setTimeout(() => { - delete savedTimersRef.current[id] - setRowStatus((prev) => { - if (prev[id] !== 'saved') return prev - const next = { ...prev } - delete next[id] - return next - }) - }, SAVED_STATUS_TTL_MS) - } else if (wasSaving && !isSaving) { - delete sentDraftRef.current[id] - setRowStatus((prev) => ({ ...prev, [id]: 'error' })) - } - } - }, [wishes, isSaving, t]) - // Clean up every outstanding timer on unmount so a debounced save (or a // "Sparat" auto-clear) never fires against an unmounted component. Both // registries are mutated in place (never reassigned), so capturing the // object reference here still sees every timer added later, right up to - // unmount. + // unmount. Also flips `isMountedRef` so a `performSave` awaiting + // `onUpdate` when unmount happens skips its post-await state updates. useEffect(() => { + isMountedRef.current = true const timers = timersRef.current const savedTimers = savedTimersRef.current return () => { + isMountedRef.current = false Object.values(timers).forEach(clearTimeout) Object.values(savedTimers).forEach(clearTimeout) } @@ -422,18 +394,52 @@ export function WishEditor({ }) } - /** Fires (at most) `AUTOSAVE_DEBOUNCE_MS` after the last edit to this row. + /** Fires (at most) `AUTOSAVE_DEBOUNCE_MS` after the last edit to this row + * (via `scheduleSave`), or immediately on an explicit retry click. * Re-validates against the latest draft/wish at fire time rather than - * trusting whatever was true when the timer was scheduled. */ - const performSave = (id: string) => { + * trusting whatever was true when the timer was scheduled. + * + * Awaits `onUpdate` directly and drives `rowStatus` off the real outcome: + * a resolved promise (or a synchronous `void` return that doesn't throw, + * for backward compatibility with a caller that hasn't adopted the + * promise form) means success; a rejection means failure. Either way the + * draft itself is left untouched here — a failed save keeps the user's + * edit exactly as typed, ready for `onChange` to reschedule or for the + * retry button to resend it as-is. */ + const performSave = async (id: string) => { const wish = wishes.find((w) => w.id === id) const draft = draftsRef.current[id] if (!wish || !draft) return if (isBoundTooShort(draft) || draftEqualsWish(draft, wish, t)) return - sentDraftRef.current[id] = draft setRowStatus((prev) => ({ ...prev, [id]: 'saving' })) - onUpdate(id, buildPatch(draft)) + try { + await onUpdate(id, buildPatch(draft)) + if (!isMountedRef.current) return + setRowStatus((prev) => ({ ...prev, [id]: 'saved' })) + const existingSavedTimer = savedTimersRef.current[id] + if (existingSavedTimer) clearTimeout(existingSavedTimer) + savedTimersRef.current[id] = setTimeout(() => { + delete savedTimersRef.current[id] + setRowStatus((prev) => { + if (prev[id] !== 'saved') return prev + const next = { ...prev } + delete next[id] + return next + }) + }, SAVED_STATUS_TTL_MS) + } catch { + if (!isMountedRef.current) return + setRowStatus((prev) => ({ ...prev, [id]: 'error' })) + } + } + + /** Explicit retry for a row whose last save failed: re-sends the exact + * still-pending draft immediately, bypassing the debounce. The draft was + * never discarded on failure, so this is just `performSave` run on + * demand instead of from the debounce timer. */ + const retrySave = (id: string) => { + void performSave(id) } /** Debounces `performSave` for one row; rapid successive edits keep @@ -443,7 +449,7 @@ export function WishEditor({ if (existing) clearTimeout(existing) timersRef.current[id] = setTimeout(() => { delete timersRef.current[id] - performSave(id) + void performSave(id) }, AUTOSAVE_DEBOUNCE_MS) } @@ -826,6 +832,16 @@ export function WishEditor({ {status === 'error' && t('wishEditor.saveFailedStatus')} )} + {status === 'error' && ( + + )}