From 8b1839971041b3426283c210e9035fab28b1012f Mon Sep 17 00:00:00 2001 From: dev_agent Date: Wed, 7 Oct 2026 03:36:19 -0400 Subject: [PATCH] Settle a failed Discover decision on the deck read it makes itself A failed decision now reads the deck as its own fetch and waits for it, and that read's answer travels on the DecisionError: still pending puts the Side quest back on top with a Retry, decided shows the informational banner, and a failed read shows the error without Retry. The page no longer classifies from whatever the deck cache holds. A batch poll's deck read no longer cancels a read already out (which may be a decision's): it waits for it, then reads again, as that read may predate what the batch reported. Co-Authored-By: Claude Opus 5.5 --- web/src/discover/DiscoverPage.test.tsx | 84 +++++++++++++++++++++++ web/src/discover/DiscoverPage.tsx | 28 +++++--- web/src/discover/api.ts | 92 ++++++++++++++++++-------- 3 files changed, 166 insertions(+), 38 deletions(-) diff --git a/web/src/discover/DiscoverPage.test.tsx b/web/src/discover/DiscoverPage.test.tsx index 180846f..1120491 100644 --- a/web/src/discover/DiscoverPage.test.tsx +++ b/web/src/discover/DiscoverPage.test.tsx @@ -864,6 +864,90 @@ describe('DiscoverPage', () => { await waitFor(() => expect(sent(server.fetchMock)).toEqual(['POST /api/v1/suggestions/s1/accept', 'POST /api/v1/suggestions/s1/accept'])) }) + it('settles a failed decision on its own read of the deck when a batch poll reads the deck again during it', async () => { + const job = (suggestionIds: string[]) => + jsonResponse(200, { + id: 'j1', + kind: 'suggestion_batch', + status: 'running', + result: { batch_id: 'b1', stage: 'suggesting', suggestion_ids: suggestionIds, dropped: 0 }, + error: null, + }) + let poll: ReturnType | null = null + let read: ReturnType | null = null + let refuse = true + const server = stubServer([BOWLS, TIKKA, ORZO, PAELLA], { + 'POST /api/v1/suggestions/s1/decline': () => { + server.deck = [TIKKA, ORZO, PAELLA] + return jsonResponse(200, { suggestion: { ...BOWLS, status: 'declined' }, replenish_job_id: 'j1' }) + }, + 'POST /api/v1/suggestions/s2/accept': () => { + if (!refuse) return accepted(TIKKA) + read = deferred() + return errorResponse(500, 'internal') + }, + 'GET /api/v1/jobs/j1': () => poll?.promise ?? job([]), + [`GET ${DECK}`]: () => read?.promise ?? jsonResponse(200, { suggestions: server.deck }), + }) + const queryClient = createQueryClient() + await renderPage(queryClient) + await screen.findByRole('article', { name: 'Korean beef bowls' }) + + // A Pass starts a replenish batch, whose job is polled. + fireEvent.click(screen.getByRole('button', { name: 'Pass' })) + await waitFor(() => expect(jobPolls(server.fetchMock)).toBe(1)) + await waitFor(() => expect(screen.getByRole('button', { name: 'Try it' })).toBeEnabled()) + expect(topCard()).toHaveAccessibleName('Crock pot chicken tikka masala') + poll = deferred() + const polled = poll + await waitFor(() => expect(jobPolls(server.fetchMock)).toBe(2), { timeout: 2000 }) + + // Try it fails without being saved; the deck read it makes is slow. + const reads = deckReads(server.fetchMock) + fireEvent.click(screen.getByRole('button', { name: 'Try it' })) + await waitFor(() => expect(deckReads(server.fetchMock)).toBe(reads + 1)) + const reconciling = read! + read = deferred() + const after = read + expect(screen.getByRole('button', { name: 'Try it' })).toBeDisabled() + + // The batch stores a Side quest while that read is out: its read of + // the deck waits for the decision's rather than replacing it. + const SOUP = suggestionBody('s5', 'Tortellini soup') + server.deck = [TIKKA, ORZO, PAELLA, SOUP] + await act(async () => polled.resolve(job(['s5']))) + await act(async () => await new Promise((resolve) => setTimeout(resolve, 0))) + expect(deckReads(server.fetchMock)).toBe(reads + 1) + expect(screen.getByRole('button', { name: 'Try it' })).toBeDisabled() + + // The decision's read has the Side quest still pending: the card goes + // back on top with a Retry, and the decision settles on that read. + await act(async () => reconciling.resolve(jsonResponse(200, { suggestions: server.deck }))) + await waitFor(() => expect(screen.getByRole('button', { name: 'Try it' })).toBeEnabled()) + const alert = screen.getByRole('alert') + expect(alert).toHaveTextContent("Couldn't save that choice.") + expect(within(alert).getByRole('button', { name: 'Retry' })).toBeInTheDocument() + expect(topCard()).toHaveAccessibleName('Crock pot chicken tikka masala') + + // The batch's read goes out after it, and brings its Side quest. + await waitFor(() => expect(deckReads(server.fetchMock)).toBe(reads + 2)) + read = null + await act(async () => after.resolve(jsonResponse(200, { suggestions: server.deck }))) + await waitFor(() => expect(queryClient.getQueryData<{ id: string }[]>(DECK_KEY)?.map((q) => q.id)).toEqual(['s2', 's3', 's4', 's5'])) + expect(topCard()).toHaveAccessibleName('Crock pot chicken tikka masala') + + // Retry sends it again. + refuse = false + fireEvent.click(within(alert).getByRole('button', { name: 'Retry' })) + await waitFor(() => + expect(sent(server.fetchMock)).toEqual([ + 'POST /api/v1/suggestions/s1/decline', + 'POST /api/v1/suggestions/s2/accept', + 'POST /api/v1/suggestions/s2/accept', + ]), + ) + }) + it('reads the deck again after a 500 to a decision, rather than putting the card back', async () => { const decline = deferred() const server = stubServer([BOWLS, TIKKA, ORZO, PAELLA], { diff --git a/web/src/discover/DiscoverPage.tsx b/web/src/discover/DiscoverPage.tsx index 46c8bb9..1ab836c 100644 --- a/web/src/discover/DiscoverPage.tsx +++ b/web/src/discover/DiscoverPage.tsx @@ -90,10 +90,10 @@ function QuestLogPill({ dinners, ref }: { dinners: { id: string; name: string }[ // at a time from the top card. A decision from the buttons, the ← / → keys // or a swipe of the top card throws it out (motion.ts) and sends the // decision once it is gone, taking the card off the deck at once (see -// useDecide); Retry sends a failed one again while the deck read since still -// has its Side quest pending. Input is locked from the press until the -// decision settles, which is once the deck and the Quest log have been read -// again after it, so no decision starts under those reads. The lock is a +// useDecide); Retry sends a failed one again while the deck it read after +// failing still had its Side quest pending. Input is locked from the press +// until the decision settles, which is once the deck and the Quest log have +// been read again after it, so no decision starts under those reads. The lock is a // ref, set before anything starts, so a second press before the next render // does nothing, and busy mirrors it to disable the buttons. export function DiscoverPage() { @@ -310,17 +310,23 @@ export function DiscoverPage() { }, [decide]) // The batch stores its Side quests as it goes, so the deck is read again - // whenever it reports more, and once it ends. + // whenever it reports more, and once it ends. A read already out may be a + // decision's, which must not be cut short: the batch's read waits for it, + // then goes out after it, as it may predate what the batch reported. const { stored } = batch useEffect(() => { if (job.status === undefined) return - void queryClient.invalidateQueries({ queryKey: DECK_KEY }) + const after = queryClient.isFetching({ queryKey: DECK_KEY }) > 0 + void queryClient + .invalidateQueries({ queryKey: DECK_KEY }, { cancelRefetch: false }) + .then(() => (after ? queryClient.invalidateQueries({ queryKey: DECK_KEY }, { cancelRefetch: false }) : undefined)) }, [queryClient, job.status, stored]) - // A failed decision can be retried only while the deck, read again after - // it, still has its Side quest pending: anything else decided it already. - const failed = decision.isError ? decision.variables.quest.id : null - const retryable = failed !== null && !deck.isError && deck.data !== undefined && deck.data.some((q) => q.id === failed) + // A failed decision can be retried only while the deck it read after + // failing still has its Side quest pending: anything else decided it + // already. + const pending = decision.isError ? decision.error.pending : undefined + const retryable = pending !== undefined && pending !== null let body if (deck.isError) { @@ -392,7 +398,7 @@ export function DiscoverPage() { {decision.isError && (retryable ? ( submit(decision.variables)}>Couldn't save that choice. {decision.error.message} - ) : deck.isError ? ( + ) : pending === undefined ? ( Couldn't save that choice. {decision.error.message} ) : ( That side quest has been decided already. {decision.error.message} diff --git a/web/src/discover/api.ts b/web/src/discover/api.ts index 47b78eb..9ee08c4 100644 --- a/web/src/discover/api.ts +++ b/web/src/discover/api.ts @@ -130,6 +130,20 @@ export function useStartBatch() { }) } +// DecisionError is a failed decision, with what the deck read after it +// found: pending is its Side quest while the server still has it pending, +// null once it is decided, and undefined when that read failed too (the +// deck query holds the read's error). +export class DecisionError extends Error { + readonly pending: SideQuest | null | undefined + + constructor(cause: unknown, pending: SideQuest | null | undefined) { + super(cause instanceof Error ? cause.message : String(cause), { cause }) + this.name = 'DecisionError' + this.pending = pending + } +} + // useDecide records Try it (POST /api/v1/suggestions/{id}/accept) or Pass // (POST /api/v1/suggestions/{id}/decline) on the Side quest the household // decided on, whether or not a read of the deck has since taken it away. @@ -141,30 +155,51 @@ export function useStartBatch() { // Nothing else is worked out here: the server is the record of both. However // the decision ends, saved or failed, the deck and the Quest log are read // again, and the decision is pending until those reads land, so the next one -// (which DiscoverPage locks out until then) cannot start under them. No -// decision cancels a read; one under way when the decision ends is replaced -// by the read made after it. A failed decision whose Side quest that read -// still has is still pending, and goes back on top of the deck. +// (which DiscoverPage locks out until then) cannot start under them. A read +// under way when the decision ends is replaced by the read made after it, +// and DiscoverPage's batch reads wait for that one rather than replace it. +// A failed decision is a DecisionError, settled on the read it made and +// waited for itself: a Side quest that read still has is still pending, and +// goes back on top of the deck. export function useDecide() { const { apiFetch } = useApi() const queryClient = useQueryClient() - return useMutation({ - mutationFn: async ({ quest: { id }, choice }: { quest: SideQuest; choice: Choice }): Promise => { - const action = choice === 'try' ? 'accept' : 'decline' - const path = `${SUGGESTIONS_PATH}/${encodeURIComponent(id)}/${action}` - const body = await apiFetch(path, { method: 'POST' }) - if ( - !isRecord(body) || - !isRecord(body.suggestion) || - body.suggestion.id !== id || - body.suggestion.status !== DECISION_STATUS[choice] - ) { - throw new Error(`POST ${path}: response is not the ${DECISION_STATUS[choice]} suggestion`) + + // send POSTs the decision and checks the answer. + async function send(id: string, choice: Choice): Promise { + const action = choice === 'try' ? 'accept' : 'decline' + const path = `${SUGGESTIONS_PATH}/${encodeURIComponent(id)}/${action}` + const body = await apiFetch(path, { method: 'POST' }) + if ( + !isRecord(body) || + !isRecord(body.suggestion) || + body.suggestion.id !== id || + body.suggestion.status !== DECISION_STATUS[choice] + ) { + throw new Error(`POST ${path}: response is not the ${DECISION_STATUS[choice]} suggestion`) + } + if (body.replenish_job_id !== null && typeof body.replenish_job_id !== 'string') { + throw new Error(`POST ${path}: response has no replenish_job_id`) + } + return body.replenish_job_id + } + + return useMutation({ + mutationFn: async ({ quest, choice }) => { + try { + return await send(quest.id, choice) + } catch (error) { + // The deck is read again as the decision's own fetch, replacing any + // read already out; its answer, not the cache's, settles it. + let read: SideQuest[] + try { + read = await queryClient.getQueryCache().build(queryClient, { queryKey: DECK_KEY }).fetch(undefined, { cancelRefetch: true }) + } catch { + // The deck query holds the read's error, shown with the deck. + throw new DecisionError(error, undefined) + } + throw new DecisionError(error, read.find((q) => q.id === quest.id) ?? null) } - if (body.replenish_job_id !== null && typeof body.replenish_job_id !== 'string') { - throw new Error(`POST ${path}: response has no replenish_job_id`) - } - return body.replenish_job_id }, onMutate: ({ quest, choice }) => { queryClient.setQueryData(DECK_KEY, (deck) => deck?.filter((q) => q.id !== quest.id)) @@ -174,13 +209,16 @@ export function useDecide() { dinners === undefined || dinners.some((d) => d.id === quest.dinner_id) ? dinners : dinners.concat({ id: quest.dinner_id, name: quest.name }), ) }, - onSettled: async (_replenishJobId, error, { quest: { id } }) => { - await Promise.all([queryClient.invalidateQueries({ queryKey: DECK_KEY }), queryClient.invalidateQueries({ queryKey: QUEST_LOG_KEY })]) - if (error === null) return - queryClient.setQueryData(DECK_KEY, (deck) => { - const pending = deck?.find((q) => q.id === id) - return pending === undefined ? deck : [pending, ...deck!.filter((q) => q !== pending)] - }) + onSettled: async (_replenishJobId, error) => { + if (error === null) { + await Promise.all([queryClient.invalidateQueries({ queryKey: DECK_KEY }), queryClient.invalidateQueries({ queryKey: QUEST_LOG_KEY })]) + return + } + // A failed decision has read the deck already. + await queryClient.invalidateQueries({ queryKey: QUEST_LOG_KEY }) + const { pending } = error + if (pending === null || pending === undefined) return + queryClient.setQueryData(DECK_KEY, (deck) => [pending, ...(deck ?? []).filter((q) => q.id !== pending.id)]) }, }) }