From 568a6ddfb86df77dc615324951651c8fdf2b1f4e Mon Sep 17 00:00:00 2001 From: dev_agent Date: Tue, 6 Oct 2026 17:26:27 -0400 Subject: [PATCH] Keep same-name add refusals and refused removal order in onboarding Step 2 keyed each refusal by the dinner's name, so a second add of the same name still in flight replaced the first add's refusal when it was refused too. Each attempt now keys its refusal uniquely; trying a change again once it has been refused still clears that earlier refusal. A refused removal was put back at the index it had in the list as it stood when it was removed, so refusing two overlapping removals of A and B restored B, A, C until a reread corrected it. A removal now only marks its Regular in the cache and the list hides it; a refusal clears the mark, so the Regular returns to its own place whatever else changed. Co-Authored-By: Claude Opus 5.5 --- web/src/dinners/api.ts | 53 +++++++++++--------- web/src/onboarding/OnboardingPage.test.tsx | 58 +++++++++++++++++++++- web/src/onboarding/RegularsStep.tsx | 23 +++++---- 3 files changed, 98 insertions(+), 36 deletions(-) diff --git a/web/src/dinners/api.ts b/web/src/dinners/api.ts index d342de7..28ed091 100644 --- a/web/src/dinners/api.ts +++ b/web/src/dinners/api.ts @@ -9,6 +9,15 @@ export const REGULARS_PATH = `${DINNERS_PATH}?status=regular` // while an optimistic add waits for the server; its id is not the server's. export type Regular = { id: string; name: string; pending?: true } +// CachedRegular is a Regular as the cache holds it. removing is true while +// an optimistic removal waits for the server: the entry keeps its place, so +// a refused removal returns exactly there whatever else changed meanwhile. +type CachedRegular = Regular & { removing?: true } + +function shown(list: CachedRegular[]): Regular[] { + return list.filter((regular) => !regular.removing) +} + // REGULARS_KEY prefixes every Regulars query, so a change refreshes them all // (the settings home's count included). export const REGULARS_KEY = ['dinners', 'regular'] as const @@ -31,19 +40,21 @@ export function useRegulars() { const where = `GET ${REGULARS_PATH}` const body = await apiFetch(REGULARS_PATH) if (!isRecord(body) || !Array.isArray(body.dinners)) throw new Error(`${where}: response has no dinners list`) - return body.dinners.map((dinner) => parseDinner(where, dinner)) + return body.dinners.map((dinner): CachedRegular => parseDinner(where, dinner)) }, + select: shown, }) } // changeOptimistically stops Regulars reads in flight, so they cannot -// overwrite the optimistic list, then applies change to it. It returns the -// list as it was. -async function changeOptimistically(queryClient: QueryClient, change: (list: Regular[]) => Regular[]) { +// overwrite the optimistic list, then applies change to it. +async function changeOptimistically(queryClient: QueryClient, change: (list: CachedRegular[]) => CachedRegular[]) { await queryClient.cancelQueries({ queryKey: REGULARS_LIST_KEY }) - const before = queryClient.getQueryData(REGULARS_LIST_KEY) ?? [] - queryClient.setQueryData(REGULARS_LIST_KEY, change(before)) - return before + setRegulars(queryClient, change) +} + +function setRegulars(queryClient: QueryClient, change: (list: CachedRegular[]) => CachedRegular[]) { + queryClient.setQueryData(REGULARS_LIST_KEY, (list) => change(list ?? [])) } // settle refetches the Regulars once the last change in flight is done, so @@ -71,22 +82,19 @@ export function useAddRegular() { return { id } }, onSuccess: (dinner, _name, context) => { - queryClient.setQueryData(REGULARS_LIST_KEY, (list) => - (list ?? []).map((regular) => (regular.id === context.id ? dinner : regular)), - ) + setRegulars(queryClient, (list) => list.map((regular) => (regular.id === context.id ? dinner : regular))) }, onError: (_error, _name, context) => { if (context === undefined) return - queryClient.setQueryData(REGULARS_LIST_KEY, (list) => - (list ?? []).filter((regular) => regular.id !== context.id), - ) + setRegulars(queryClient, (list) => list.filter((regular) => regular.id !== context.id)) }, onSettled: () => settle(queryClient), }) } // useRemoveRegular archives a Regular (DELETE /api/v1/dinners/{id}). It -// leaves the list at once and is put back if the server refuses. +// leaves the list at once and is put back in its place if the server +// refuses. export function useRemoveRegular() { const { apiFetch } = useApi() const queryClient = useQueryClient() @@ -96,16 +104,15 @@ export function useRemoveRegular() { await apiFetch(`${DINNERS_PATH}/${encodeURIComponent(regular.id)}`, { method: 'DELETE' }) }, onMutate: async (regular) => { - const before = await changeOptimistically(queryClient, (list) => list.filter((r) => r.id !== regular.id)) - return { index: before.findIndex((r) => r.id === regular.id) } + await changeOptimistically(queryClient, (list) => + list.map((r): CachedRegular => (r.id === regular.id ? { ...r, removing: true } : r)), + ) }, - onError: (_error, regular, context) => { - if (context === undefined) return - queryClient.setQueryData(REGULARS_LIST_KEY, (list) => { - const next = [...(list ?? [])] - next.splice(context.index === -1 ? next.length : context.index, 0, regular) - return next - }) + onSuccess: (_data, regular) => { + setRegulars(queryClient, (list) => list.filter((r) => r.id !== regular.id)) + }, + onError: (_error, regular) => { + setRegulars(queryClient, (list) => list.map((r) => (r.id === regular.id ? { id: r.id, name: r.name } : r))) }, onSettled: () => settle(queryClient), }) diff --git a/web/src/onboarding/OnboardingPage.test.tsx b/web/src/onboarding/OnboardingPage.test.tsx index f2bb78d..41615b2 100644 --- a/web/src/onboarding/OnboardingPage.test.tsx +++ b/web/src/onboarding/OnboardingPage.test.tsx @@ -22,9 +22,11 @@ type Held = { method: string; path: string; body: unknown; answer: (response: Re // stubDinnersServer stands in for the server a household meets on first // run: signed in, onboarding not finished and the given Regulars. GET answers at once; // each POST and DELETE is held in `held` until the test answers it, so the -// test sees the optimistic list first. +// test sees the optimistic list first. Once failReads is set, every GET of +// the Regulars fails, so the test sees the list the client keeps by itself. function stubDinnersServer(regulars: Dinner[] = []) { const held: Held[] = [] + const reads = { fail: false } const fetchMock: Mock = vi.fn((input, init) => { const path = String(input) const method = init?.method ?? 'GET' @@ -33,6 +35,7 @@ function stubDinnersServer(regulars: Dinner[] = []) { return Promise.resolve(jsonResponse(200, { ...HOUSEHOLD_SETTINGS, onboarding_completed_at: null })) } if (method === 'GET' && path === '/api/v1/dinners?status=regular') { + if (reads.fail) return Promise.resolve(errorResponse(500, 'internal')) return Promise.resolve(jsonResponse(200, { dinners: regulars })) } if ((method === 'POST' && path === '/api/v1/dinners') || (method === 'DELETE' && path.startsWith('/api/v1/dinners/'))) { @@ -43,7 +46,7 @@ function stubDinnersServer(regulars: Dinner[] = []) { throw new Error(`unexpected fetch: ${method} ${path}`) }) vi.stubGlobal('fetch', fetchMock) - return { fetchMock, held, regulars } + return { fetchMock, held, regulars, reads } } // created answers a held POST /api/v1/dinners as the server does, adding the @@ -243,6 +246,31 @@ describe('onboarding step 2 (mock 1h)', () => { expect(regularNames()).toEqual([]) }) + it('says why each of two overlapping adds of the same name was refused', async () => { + const server = await regularsStep() + fireEvent.change(input(), { target: { value: 'Lasagne' } }) + fireEvent.submit(input()) + await vi.waitFor(() => expect(server.held).toHaveLength(1)) + fireEvent.change(input(), { target: { value: 'Lasagne' } }) + fireEvent.submit(input()) + await vi.waitFor(() => expect(server.held).toHaveLength(2)) + expect(regularNames()).toEqual(['Lasagne', 'Lasagne']) + + server.held[0]!.answer(errorResponse(409, 'duplicate_dinner')) + expect(await screen.findByRole('alert')).toHaveTextContent('Could not add Lasagne: duplicate_dinner message') + expect(regularNames()).toEqual(['Lasagne']) + + // The second add's refusal shows beside the first instead of replacing it. + server.held[1]!.answer(errorResponse(500, 'internal')) + await vi.waitFor(() => + expect(screen.getAllByRole('alert').map((alert) => alert.textContent)).toEqual([ + 'Could not add Lasagne: duplicate_dinner message', + 'Could not add Lasagne: internal message', + ]), + ) + expect(regularNames()).toEqual([]) + }) + it('removes a Regular at once and archives it on the server', async () => { const server = await regularsStep([ { id: 'd1', name: 'Beef tacos' }, @@ -295,6 +323,32 @@ describe('onboarding step 2 (mock 1h)', () => { expect(screen.getByRole('alert')).toHaveTextContent('Could not remove Beef tacos: internal message') }) + it('puts two refused removals back in their places without rereading the list', async () => { + const server = await regularsStep([ + { id: 'd1', name: 'Beef tacos' }, + { id: 'd2', name: 'Pizza night' }, + { id: 'd3', name: 'Chili' }, + ]) + server.reads.fail = true + fireEvent.click(screen.getByRole('button', { name: 'Remove Beef tacos' })) + await vi.waitFor(() => expect(server.held).toHaveLength(1)) + fireEvent.click(screen.getByRole('button', { name: 'Remove Pizza night' })) + await vi.waitFor(() => expect(server.held).toHaveLength(2)) + expect(regularNames()).toEqual(['Chili']) + + server.held[0]!.answer(errorResponse(500, 'internal')) + await screen.findByRole('alert') + server.held[1]!.answer(errorResponse(500, 'internal')) + await screen.findByText('Could not remove Pizza night: internal message') + // The reread after the last answer fails, so the order is the client's own. + expect(await screen.findByText(/Your Regulars could not load/)).toBeInTheDocument() + expect(screen.getAllByRole('button', { name: /^Remove / }).map((button) => button.getAttribute('aria-label'))).toEqual([ + 'Remove Beef tacos', + 'Remove Pizza night', + 'Remove Chili', + ]) + }) + it('enables "Next: notifications" once the server has saved 3 Regulars', async () => { const server = await regularsStep([ { id: 'd1', name: 'Beef tacos' }, diff --git a/web/src/onboarding/RegularsStep.tsx b/web/src/onboarding/RegularsStep.tsx index 09ac65e..c867732 100644 --- a/web/src/onboarding/RegularsStep.tsx +++ b/web/src/onboarding/RegularsStep.tsx @@ -1,4 +1,4 @@ -import { useState, type FormEvent } from 'react' +import { useRef, useState, type FormEvent } from 'react' import { Plus } from 'lucide-react' import { useNavigate } from 'react-router' import { useAddRegular, useRegulars, useRemoveRegular, type Regular } from '../dinners/api.ts' @@ -31,9 +31,11 @@ function sameName(a: string, b: string): boolean { return a.trim().toLowerCase() === b.trim().toLowerCase() } -// Refusal is one add or removal the server turned down. key names the -// change, so trying it again replaces its refusal and no other. -type Refusal = { key: string; text: string } +// Refusal is one add or removal the server turned down. key is unique to +// that attempt, so two attempts of the same change in flight each keep their +// own refusal. change names what was tried, so trying it again once it has +// been refused clears the earlier refusal and no other. +type Refusal = { key: number; change: string; text: string } // RegularsStep is onboarding step 2, handoff mock 1h: the household adds // the dinners it makes on repeat. Each add and remove shows at once and is @@ -48,6 +50,7 @@ export function RegularsStep() { // latest change, which would hide a refusal while another change is in // flight. const [refusals, setRefusals] = useState([]) + const attempts = useRef(0) const list = regulars.data ?? [] const picks = commonPicks.filter((pick) => !list.some((regular) => sameName(regular.name, pick))) // Next waits for every add to be answered, so a refused add's error shows @@ -55,13 +58,11 @@ export function RegularsStep() { const adding = list.some((regular) => regular.pending) const saved = list.filter((regular) => !regular.pending).length - function track(key: string, what: string, change: Promise) { - setRefusals((all) => all.filter((refusal) => refusal.key !== key)) - change.catch((error: Error) => { - setRefusals((all) => [ - ...all.filter((refusal) => refusal.key !== key), - { key, text: `Could not ${what}: ${error.message}` }, - ]) + function track(change: string, what: string, answer: Promise) { + const key = ++attempts.current + setRefusals((all) => all.filter((refusal) => refusal.change !== change)) + answer.catch((error: Error) => { + setRefusals((all) => [...all, { key, change, text: `Could not ${what}: ${error.message}` }]) }) }