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 <noreply@anthropic.com>

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<unknown>(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<Regular[]>(REGULARS_LIST_KEY) ?? []
-  queryClient.setQueryData<Regular[]>(REGULARS_LIST_KEY, change(before))
-  return before
+  setRegulars(queryClient, change)
+}
+
+function setRegulars(queryClient: QueryClient, change: (list: CachedRegular[]) => CachedRegular[]) {
+  queryClient.setQueryData<CachedRegular[]>(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<Regular[]>(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<Regular[]>(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<unknown>(`${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<Regular[]>(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),
   })

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<typeof fetch> = vi.fn<typeof fetch>((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' },

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<Refusal[]>([])
+  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<unknown>) {
-    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<unknown>) {
+    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}` }])
     })
   }