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' && ( + + )}