diff --git a/web/src/plan/PlanPage.test.tsx b/web/src/plan/PlanPage.test.tsx index 33b584a..d9ceca8 100644 --- a/web/src/plan/PlanPage.test.tsx +++ b/web/src/plan/PlanPage.test.tsx @@ -1392,6 +1392,64 @@ describe('PlanPage', () => { expect(within(tile('FRI')).getByText('Pizza night')).toBeInTheDocument() }) + it("still shows a swap's failure on coming back to the week, through another, and asks for the swap again only on Retry", async () => { + const fetchMock = stubServer({ + [`GET ${THIS_WEEK}`]: () => jsonResponse(200, planBody('draft')), + [`GET ${NEXT_WEEK}`]: () => errorResponse(404, 'not_found'), + [`GET /api/v1/plans/2026-10-19`]: () => errorResponse(404, 'not_found'), + ...lists, + [`PUT ${FRIDAY}`]: () => errorResponse(409, 'dinner_not_plannable'), + }) + await renderPage() + + fireEvent.click(await within(await openFriday()).findByRole('button', { name: /Lemon orzo/ })) + expect(await screen.findByRole('alert')).toHaveTextContent("Couldn't swap Friday's dinner. dinner_not_plannable message") + fireEvent.click(screen.getByRole('button', { name: 'Next week' })) + expect(await screen.findByRole('button', { name: 'Draft this week' })).toBeInTheDocument() + fireEvent.click(screen.getByRole('button', { name: 'Next week' })) + await waitFor(() => expect(screen.getByTestId('search')).toHaveTextContent('?week=2026-10-19')) + expect(await screen.findByRole('button', { name: 'Draft this week' })).toBeInTheDocument() + fireEvent.click(screen.getByRole('button', { name: 'Previous week' })) + await waitFor(() => expect(screen.getByTestId('search')).toHaveTextContent('?week=2026-10-12')) + fireEvent.click(screen.getByRole('button', { name: 'Previous week' })) + + expect(await screen.findByRole('alert')).toHaveTextContent("Couldn't swap Friday's dinner. dinner_not_plannable message") + expect(within(tile('FRI')).getByText('Pizza night')).toBeInTheDocument() + expect(puts(fetchMock)).toHaveLength(1) + }) + + it('reads the plan again when the dinner is already on another day, and offers to choose another rather than send it again', async () => { + // Meanwhile lemon orzo was planned on Wednesday. + let read = planBody('draft') + const fetchMock = stubServer({ + [`GET ${THIS_WEEK}`]: () => jsonResponse(200, read), + ...lists, + [`PUT ${FRIDAY}`]: () => errorResponse(409, 'dinner_already_planned'), + }) + await renderPage() + + const sheet = await openFriday() + read = planBody('draft') + read.days[2] = { ...read.days[2]!, dinner: { ...read.days[2]!.dinner, id: 'q1', name: 'Lemon orzo' }, is_new: true } + fireEvent.click(await within(sheet).findByRole('button', { name: /Lemon orzo/ })) + + const alert = await screen.findByRole('alert') + expect(alert).toHaveTextContent("Couldn't swap Friday's dinner. Lemon orzo is already on another day this week.") + expect(within(alert).queryByRole('button', { name: 'Retry' })).not.toBeInTheDocument() + await waitFor(() => expect(within(tile('WED · NEW')).getByText('Lemon orzo')).toBeInTheDocument()) + expect(within(tile('FRI')).getByText('Pizza night')).toBeInTheDocument() + + // Choosing again opens Friday's sheet on the plan read, where lemon + // orzo is already this week; nothing is sent again. + fireEvent.click(within(alert).getByRole('button', { name: 'Choose another dinner' })) + const again = screen.getByRole('dialog', { name: 'Swap Friday' }) + expect(screen.queryByRole('alert')).not.toBeInTheDocument() + const orzo = within(within(again).getByRole('list', { name: 'Quest log' })).getByRole('listitem') + expect(within(orzo).getByText('Already this week')).toBeInTheDocument() + expect(within(orzo).queryByRole('button')).not.toBeInTheDocument() + expect(puts(fetchMock)).toHaveLength(1) + }) + it('waits for a pending swap before Rebuild week, keeping its failure', async () => { let refuse!: () => void const fetchMock = stubServer({ diff --git a/web/src/plan/PlanPage.tsx b/web/src/plan/PlanPage.tsx index 471268e..710dec2 100644 --- a/web/src/plan/PlanPage.tsx +++ b/web/src/plan/PlanPage.tsx @@ -127,6 +127,12 @@ export function ago(at: Date, now: Date): string { return days === 1 ? '1 day ago' : `${days} days ago` } +// alreadyPlanned says whether a swap was refused because its dinner is +// already on another day of the week: sending it again cannot succeed. +function alreadyPlanned(error: Error): boolean { + return error instanceof ApiError && error.code === 'dinner_already_planned' +} + // pushedToCalendar says whether drafting over a plan of this status needs // confirm: it was pushed to Calendar. function pushedToCalendar(status: PlanStatus): boolean { @@ -398,9 +404,18 @@ function WeekBoard({ ) ) : swap.failed !== null ? ( - swapIn(swap.failed!.swap)}> - Couldn't swap {weekdayName(swap.failed.swap.date)}'s dinner. {swap.failed.error.message} - + alreadyPlanned(swap.failed.error) ? ( + // The plan is read again (useSwapDay): the household chooses another + // dinner from it rather than sending the refused swap again. + openSwap(swap.failed!.swap.date)} retryLabel="Choose another dinner"> + Couldn't swap {weekdayName(swap.failed.swap.date)}'s dinner. {swap.failed.swap.dinner.name} is already on another day + this week. + + ) : ( + swapIn(swap.failed!.swap)}> + Couldn't swap {weekdayName(swap.failed.swap.date)}'s dinner. {swap.failed.error.message} + + ) ) : startDraft.isError ? ( Couldn't start drafting this week. {startDraft.error.message} ) : result.error !== null ? ( diff --git a/web/src/plan/api.test.tsx b/web/src/plan/api.test.tsx index 301203f..32daa99 100644 --- a/web/src/plan/api.test.tsx +++ b/web/src/plan/api.test.tsx @@ -127,6 +127,17 @@ describe('usePlan', () => { ['a method it does not know', (body: ReturnType) => (body.days[0]!.dinner.method = 'microwave'), 'method "microwave"'], ['no method', (body: ReturnType) => Reflect.deleteProperty(body.days[0]!.dinner, 'method'), 'method undefined'], ['a sync time that is no instant', (body: ReturnType) => (body.calendar_synced_at = 'yesterday'), 'calendar_synced_at "yesterday"'], + // Date reads both, in this device's zone: neither says which instant. + [ + 'a sync time with no offset', + (body: ReturnType) => (body.calendar_synced_at = '2026-10-07T15:58:00'), + 'calendar_synced_at "2026-10-07T15:58:00" is not an instant', + ], + [ + 'a sync date with no time', + (body: ReturnType) => (body.calendar_synced_at = '2026-10-07'), + 'calendar_synced_at "2026-10-07" is not an instant', + ], ])('refuses a plan with %s', async (_, change, said) => { const body = planBody() change(body) @@ -208,6 +219,72 @@ describe('useSwapDay', () => { expect(result.current.swap.failed?.error.message).toBe('dinner_not_plannable message') }) + it('leaves a day a later plan read landed, the very day the swap wrote, when the swap of it fails', async () => { + const friday = deferred() + let read = planBody() + stubServer({ + [`GET ${PLAN}`]: () => jsonResponse(200, read), + [`PUT ${FRIDAY}`]: () => friday.reply, + }) + const { result, plan, queryClient } = await renderSwaps() + + // The read finds Friday lemon orzo, field for field the day the swap + // wrote: the server has it, whatever this swap's PUT is answered with. + const swapped = { date: '2026-10-09', dinner: ORZO.dinner, is_new: true, start_by: null } + act(() => result.current.swap.mutate(ORZO)) + await waitFor(() => expect(plan().days[4]).toEqual(swapped)) + read = planBody({ 4: { id: 'q1', name: 'Lemon orzo', is_new: true } }) + await act(() => result.current.plan.refetch()) + await act(async () => friday.send(errorResponse(500, 'internal'))) + + await waitFor(() => expect(queryClient.isMutating()).toBe(0)) + expect(plan().days[4]).toEqual(swapped) + expect(result.current.swap.failed?.error.message).toBe('internal message') + }) + + it('puts the day back when the swap of it fails with no plan read since, after a later swap of another day', async () => { + const friday = deferred() + const monday = deferred() + stubServer({ + [`GET ${PLAN}`]: () => jsonResponse(200, planBody()), + [`PUT ${FRIDAY}`]: () => friday.reply, + [`PUT ${MONDAY}`]: () => monday.reply, + }) + const { result, plan, queryClient } = await renderSwaps() + + act(() => result.current.swap.mutate(ORZO)) + act(() => result.current.swap.mutate(FISH)) + await waitFor(() => expect(plan().days[0]!.dinner.name).toBe('Fish tacos')) + await act(async () => friday.send(errorResponse(500, 'internal'))) + + await waitFor(() => expect(plan().days[4]!.dinner.name).toBe('Dinner 4')) + expect(plan().days[0]!.dinner.name).toBe('Fish tacos') + await act(async () => monday.send(errorResponse(500, 'internal'))) + await waitFor(() => expect(queryClient.isMutating()).toBe(0)) + expect(plan().days.map((day) => day.dinner.name)).toEqual(planBody().days.map((day) => day.dinner.name)) + }) + + it('reads the plan again when the dinner is already on another day of the week, and sends the swap only once', async () => { + let read = planBody() + stubServer({ + [`GET ${PLAN}`]: () => jsonResponse(200, read), + [`PUT ${FRIDAY}`]: () => errorResponse(409, 'dinner_already_planned'), + }) + const { result, plan, queryClient } = await renderSwaps() + + // Meanwhile lemon orzo was planned on Wednesday. + read = planBody({ 2: { id: 'q1', name: 'Lemon orzo', is_new: true } }) + act(() => result.current.swap.mutate(ORZO)) + + await waitFor(() => expect(plan().days[2]!.dinner.name).toBe('Lemon orzo')) + expect(plan().days[4]!.dinner.name).toBe('Dinner 4') + await waitFor(() => expect(queryClient.isMutating()).toBe(0)) + expect(result.current.swap.failed?.error.message).toBe('dinner_already_planned message') + const sent = vi.mocked(fetch).mock.calls.map(([input, init]) => `${init?.method ?? 'GET'} ${String(input)}`) + expect(sent.filter((call) => call === `PUT ${FRIDAY}`)).toHaveLength(1) + expect(sent.filter((call) => call === `GET ${PLAN}`)).toHaveLength(2) + }) + it('refuses to dismiss a pending swap', async () => { const friday = deferred() stubServer({ [`GET ${PLAN}`]: () => jsonResponse(200, planBody()), [`PUT ${FRIDAY}`]: () => friday.reply }) @@ -353,6 +430,61 @@ describe('week mutations', () => { expect(weekMutations(queryClient, LATER)).toEqual([]) }) + it("leaves behind a week's settled swaps but for the failure that speaks for it, which stays until its week leaves it behind", async () => { + let refuse = true + stubServer({ + [`GET ${PLAN}`]: () => jsonResponse(200, planBody()), + [`PUT ${FRIDAY}`]: () => (refuse ? errorResponse(409, 'dinner_not_plannable') : jsonResponse(200, planBody())), + [`PUT ${MONDAY}`]: () => jsonResponse(200, planBody({ 0: { id: 'r1', name: 'Fish tacos' } })), + [`GET /api/v1/plans/${NEXT}`]: () => errorResponse(404, 'not_found'), + [`GET /api/v1/plans/${LATER}`]: () => errorResponse(404, 'not_found'), + }) + const api = createApiClient(fakeLocation('/plan'), createNetworkStatus(window), createVerifiedSession(localStorage)) + await api.loadMe() + const queryClient = createQueryClient() + const wrapper = ({ children }: { children: ReactNode }) => ( + + {children} + + ) + const hook = renderHook(({ week }) => ({ plan: usePlan(week), swap: useSwapDay(week) }), { + wrapper, + initialProps: { week: WEEK }, + }) + await waitFor(() => expect(hook.result.current.plan.isSuccess).toBe(true)) + + // Two swaps settle while the week is shown: the earlier is needed for + // ordering no longer, and only the latest stays. + act(() => hook.result.current.swap.mutate(FISH)) + act(() => hook.result.current.swap.mutate(ORZO)) + await waitFor(() => expect(weekMutations(queryClient, WEEK)).toEqual(['swap error'])) + + // Leaving the week, through two others, keeps the failure alone. + for (const week of [NEXT, LATER, NEXT]) { + hook.rerender({ week }) + await waitFor(() => expect(hook.result.current.plan.isSuccess).toBe(true)) + expect(weekMutations(queryClient, WEEK)).toEqual(['swap error']) + } + + // Coming back shows it; it fails again, and again one stays. + hook.rerender({ week: WEEK }) + expect(hook.result.current.swap.failed?.error.message).toBe('dinner_not_plannable message') + act(() => hook.result.current.swap.dismiss()) + act(() => hook.result.current.swap.mutate(ORZO)) + await waitFor(() => expect(hook.result.current.swap.failed).not.toBeNull()) + expect(weekMutations(queryClient, WEEK)).toEqual(['swap error']) + + // A swap that succeeds is left behind once the week is. + refuse = false + act(() => hook.result.current.swap.dismiss()) + act(() => hook.result.current.swap.mutate(ORZO)) + await waitFor(() => expect(hook.result.current.swap.succeeded).not.toBeNull()) + expect(weekMutations(queryClient, WEEK)).toEqual(['swap success']) + hook.rerender({ week: NEXT }) + await waitFor(() => expect(hook.result.current.plan.isSuccess).toBe(true)) + expect(weekMutations(queryClient, WEEK)).toEqual([]) + }) + it('keeps a push whose Supply run was not made, settled in a week no longer shown, to be shown on coming back', async () => { const nextPush = deferred() stubServer({ diff --git a/web/src/plan/api.ts b/web/src/plan/api.ts index 0150466..b571b99 100644 --- a/web/src/plan/api.ts +++ b/web/src/plan/api.ts @@ -1,5 +1,14 @@ import { useEffect } from 'react' -import { useMutation, useMutationState, useQuery, useQueryClient, type Mutation, type QueryClient } from '@tanstack/react-query' +import { + useMutation, + useMutationState, + useQuery, + useQueryClient, + type Mutation, + type Query, + type QueryClient, + type QueryKey, +} from '@tanstack/react-query' import { ApiError, useApi, type ApiRead } from '../api/index.ts' import { isRecord } from '../api/errors.ts' import { METHODS, oneOf, photoId, type Method } from '../dinners/api.ts' @@ -83,10 +92,16 @@ function parseDay(where: string, body: unknown): PlanDay { return { date: body.date, dinner, is_new: body.is_new, start_by: body.start_by } } +// RFC3339 is an RFC 3339 instant as the server writes it: a full date and +// time, to the second or finer, and its offset, Z or ±hh:mm. A date alone, +// or a time with no offset, which Date would read in this device's zone, is +// not one. +const RFC3339 = /^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}(?:\.\d+)?(?:Z|[+-]\d{2}:\d{2})$/ + // syncedAt reads calendar_synced_at, an RFC 3339 instant or null. function syncedAt(where: string, value: unknown): Date | null { if (value === null) return null - const at = typeof value === 'string' ? new Date(value) : null + const at = typeof value === 'string' && RFC3339.test(value) ? new Date(value) : null if (at === null || Number.isNaN(at.getTime())) { throw new Error(`${where}: calendar_synced_at ${JSON.stringify(value)} is not an instant`) } @@ -264,38 +279,54 @@ const weeksShown = new WeakMap() // time a week is shown, and is never removed, so a swap or push that settles // after its Draft board, or the whole Plan, is gone is still collected. // -// When a week is shown, the settled ones of every other week are removed, -// unless that week still has one pending: a pending one, and those an -// earlier pending one must see, stay across leaving its week and coming -// back. When one settles, the same is done, except that a week's latest, -// when it failed, stays until a week is next shown, so that coming straight -// back to it shows the failure. A week's latest push whose Supply run was -// not made stays whatever is shown: the week has no shopping list until one -// is made, which settles the push, or a push sent since replaces it, and -// making it may still be under way when the week is left. The cache holds -// at most the weeks shown's, those of weeks left pending, one push of each -// week whose Supply run was not made, and one failure of each week whose -// last pending one settled since a week was last shown. +// A swap or push pending is kept, with every other of its kind and week: +// an earlier pending one must see those sent after it, across leaving its +// week and coming back. Once none of a kind and week is pending, only the +// latest is needed, and the rest are removed, whenever a week is shown or +// one settles. The latest itself stays while its week is shown. In a week +// not shown it is removed, unless: +// +// - it is a failed swap: it stays, whatever is shown, until the week's +// board leaves it behind (useWeekSwap, Rebuild week, Push to Calendar), +// so coming back to the week shows the failure; +// - it is a push whose Supply run was not made: the week has no shopping +// list until one is made, which settles the push, or a push sent since +// replaces it, and making it may still be under way when the week is +// left; +// - it is a failed push that settled after its week was left: it stays +// until a week is next shown, so coming straight back shows it. +// +// So once none of its kind is pending, a week keeps at most one settled +// swap and one settled push, and a week left keeps none but those above: +// settled swaps and pushes never accumulate. function keepWeekMutations(queryClient: QueryClient): WeeksShown { const kept = weeksShown.get(queryClient) if (kept !== undefined) return kept const mutations = queryClient.getMutationCache() // How many times each week is shown at once. const shown = new Map() + // keep says whether the latest of its kind and week, none of which is + // pending, stays; settled says a mutation settling swept, not a week + // shown. + function keep(mutation: Mutation, of: { kind: 'swap' | 'push'; week: string }, settled: boolean): boolean { + if (shown.has(of.week)) return true + if (of.kind === 'swap') return failed(mutation) + return unmade(mutation) || (settled && failed(mutation)) + } function sweep(settled: boolean) { - // The mutations of each kind of each week not shown, in the order sent. - const left = new Map() + // The mutations of each kind of each week, in the order sent. + const sent = new Map() for (const mutation of mutations.getAll()) { const of = weekMutation(mutation) - if (of === null || shown.has(of.week)) continue + if (of === null) continue const id = `${of.kind} ${of.week}` - left.set(id, [...(left.get(id) ?? []), mutation]) + sent.set(id, { of, week: [...(sent.get(id)?.week ?? []), mutation] }) } - for (const week of left.values()) { + for (const { of, week } of sent.values()) { if (week.some((mutation) => mutation.state.status === 'pending')) continue const latest = week.at(-1) for (const mutation of week) { - if (mutation === latest && (unmade(mutation) || (settled && failed(mutation)))) continue + if (mutation === latest && keep(mutation, of, settled)) continue mutations.remove(mutation) } } @@ -324,6 +355,30 @@ function useWeekShown(weekStart: string) { useEffect(() => keepWeekMutations(queryClient).show(weekStart), [queryClient, weekStart]) } +// swapWrites counts, for each plan query, its updates that were a swap's +// own optimistic write or rollback, not a plan read from the server. +const swapWrites = new WeakMap() + +// serverReads is how many plans read from the server have landed in the +// plan query: its updates, less the swaps' own writes. A plan read lands +// whether or not it differs from the plan shown. +function serverReads(query: Query): number { + return query.state.dataUpdateCount - (swapWrites.get(query) ?? 0) +} + +// writeSwap updates the plan query as a swap's own write, which serverReads +// does not count. +function writeSwap(queryClient: QueryClient, query: Query, update: (plan: Plan | null | undefined) => Plan | null | undefined) { + const before = query.state.dataUpdateCount + queryClient.setQueryData(query.queryKey, update) + swapWrites.set(query, (swapWrites.get(query) ?? 0) + query.state.dataUpdateCount - before) +} + +// planQuery is the plan query keyed key, if the query cache has it. +function planQuery(queryClient: QueryClient, key: QueryKey): Query | undefined { + return queryClient.getQueryCache().find({ queryKey: key, exact: true }) +} + // swapKey keys the swaps of the week starting on weekStart in the mutation // cache, where they outlive the Draft board that sent them. function swapKey(weekStart: string) { @@ -345,9 +400,13 @@ export type SwapDay = { // (PUT /api/v1/plans/{weekStart}/days/{date}). The day shows the dinner at // once, NEW when it came from the Quest log, with no start-by time until the // server answers with the plan as swapped, which then replaces it. A refused -// swap puts the day back as it was, unless it has changed again meanwhile. -// However the swap ends, the Quest log is read again: the server leaves a -// dinner on the plan out of it. +// swap puts the day back as it was, unless it has changed again meanwhile: +// another swap wrote over it, or any plan read from the server landed +// since, even one with the very day the swap wrote. A swap refused because +// the dinner is already on another day of the week (409 +// dinner_already_planned) reads the plan again, so the household chooses +// again from the plan as it is. However the swap ends, the Quest log is +// read again: the server leaves a dinner on the plan out of it. // // The week's swaps live in the mutation cache, not in the component, so // leaving the week and coming back keeps them: pending and failed are read @@ -374,7 +433,7 @@ export function useSwapDay(weekStart: string): SwapDay { function mutate(chosen: Swap) { // isLatest says whether no swap of the week was sent after this one. const isLatest = () => mutations.findAll({ mutationKey }).every((other) => other.mutationId <= mutation.mutationId) - const mutation = mutations.build(queryClient, { + const mutation = mutations.build(queryClient, { mutationKey, // Kept until dismissed or settled in a week not shown // (keepWeekMutations): a later swap must stay in the cache while an @@ -387,24 +446,32 @@ export function useSwapDay(weekStart: string): SwapDay { }, onMutate: async ({ date, dinner, fromQuestLog }) => { await queryClient.cancelQueries({ queryKey: key }) - const plan = queryClient.getQueryData(key) + const query = planQuery(queryClient, key) + const plan = query?.state.data as Plan | null | undefined const previous = plan?.days.find((day) => day.date === date) - if (!plan || previous === undefined) throw new Error(`The week of ${weekStart} has no plan day ${date} to swap.`) + if (query === undefined || !plan || previous === undefined) { + throw new Error(`The week of ${weekStart} has no plan day ${date} to swap.`) + } const swapped: PlanDay = { date, dinner, is_new: fromQuestLog, start_by: null } - queryClient.setQueryData(key, { ...plan, days: plan.days.map((day) => (day.date === date ? swapped : day)) }) - return { previous, swapped } + writeSwap(queryClient, query, () => ({ ...plan, days: plan.days.map((day) => (day.date === date ? swapped : day)) })) + return { previous, swapped, query, reads: serverReads(query) } }, onSuccess: (plan) => { if (isLatest()) queryClient.setQueryData(key, plan) }, // The day goes back only while it is still, field for field, the day - // this swap wrote: a later swap or plan read that replaced it is left - // alone. - onError: (_error, _swap, context) => { - if (context === undefined) return - queryClient.setQueryData(key, (plan) => - plan && { ...plan, days: plan.days.map((day) => (sameDay(day, context.swapped) ? context.previous : day)) }, - ) + // this swap wrote, and no plan read from the server has landed since + // (serverReads): a later swap that replaced it, or a read that did, + // even with the same day, is left alone. + onError: (error, _swap, context) => { + if (context !== undefined && planQuery(queryClient, key) === context.query && serverReads(context.query) === context.reads) { + writeSwap(queryClient, context.query, (plan) => + plan && { ...plan, days: plan.days.map((day) => (sameDay(day, context.swapped) ? context.previous : day)) }, + ) + } + if (error instanceof ApiError && error.code === 'dinner_already_planned') { + void queryClient.invalidateQueries({ queryKey: key }) + } }, onSettled: () => queryClient.invalidateQueries({ queryKey: QUEST_LOG_KEY }), })