diff --git a/web/src/discover/DiscoverPage.test.tsx b/web/src/discover/DiscoverPage.test.tsx index 1120491..5ff2518 100644 --- a/web/src/discover/DiscoverPage.test.tsx +++ b/web/src/discover/DiscoverPage.test.tsx @@ -948,6 +948,60 @@ describe('DiscoverPage', () => { ) }) + it('does not put a failed decision back on the deck when a later read, landing before it settles, has it decided', async () => { + let log: ReturnType | null = null + const server = stubServer([BOWLS, TIKKA, ORZO, PAELLA], { + 'POST /api/v1/suggestions/s1/accept': () => { + log = deferred() + return errorResponse(500, 'internal') + }, + [`GET ${QUEST_LOG}`]: () => log?.promise ?? jsonResponse(200, { dinners: [] }), + }) + const queryClient = createQueryClient() + await renderPage(queryClient) + await screen.findByRole('article', { name: 'Korean beef bowls' }) + + // The decision fails; the deck read after it still has the Side quest + // pending, and the Quest log read after it is slow. + fireEvent.click(screen.getByRole('button', { name: 'Try it' })) + await waitFor(() => expect(log).not.toBeNull()) + const slow = log! + log = null + await waitFor(() => expect(deckReads(server.fetchMock)).toBe(2)) + + // Another session decides it, and a read of the deck lands meanwhile. + server.deck = [TIKKA, ORZO, PAELLA] + await act(async () => await queryClient.refetchQueries({ queryKey: DECK_KEY })) + await act(async () => slow.resolve(jsonResponse(200, { dinners: [] }))) + + const alert = await screen.findByRole('alert') + expect(screen.queryByRole('article', { name: 'Korean beef bowls' })).not.toBeInTheDocument() + expect(topCard()).toHaveAccessibleName('Crock pot chicken tikka masala') + expect(alert).toHaveTextContent('That side quest has been decided already.') + expect(within(alert).queryByRole('button', { name: 'Retry' })).not.toBeInTheDocument() + }) + + it('withdraws the Retry for a failed decision once a later read of the deck has it decided', async () => { + const server = stubServer([BOWLS, TIKKA, ORZO, PAELLA], { + 'POST /api/v1/suggestions/s1/accept': () => errorResponse(500, 'internal'), + }) + const queryClient = createQueryClient() + await renderPage(queryClient) + await screen.findByRole('article', { name: 'Korean beef bowls' }) + + fireEvent.click(screen.getByRole('button', { name: 'Try it' })) + const alert = await screen.findByRole('alert') + expect(within(alert).getByRole('button', { name: 'Retry' })).toBeInTheDocument() + expect(topCard()).toHaveAccessibleName('Korean beef bowls') + + // Another session decides it, and the next read of the deck says so. + server.deck = [TIKKA, ORZO, PAELLA] + await act(async () => await queryClient.refetchQueries({ queryKey: DECK_KEY })) + await waitFor(() => expect(topCard()).toHaveAccessibleName('Crock pot chicken tikka masala')) + expect(screen.getByRole('alert')).toHaveTextContent('That side quest has been decided already.') + expect(screen.queryByRole('button', { name: 'Retry' })).not.toBeInTheDocument() + }) + 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 1ab836c..b3e3a0f 100644 --- a/web/src/discover/DiscoverPage.tsx +++ b/web/src/discover/DiscoverPage.tsx @@ -12,6 +12,7 @@ import { BATCH_JOB_KIND, DECK_KEY, parseBatchResult, + stillPending, useDeck, useDecide, useQuestLog, @@ -90,8 +91,8 @@ 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 it read after -// failing still had its Side quest pending. Input is locked from the press +// useDecide); Retry sends a failed one again while the deck's newest read +// 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 // ref, set before anything starts, so a second press before the next render @@ -322,10 +323,9 @@ export function DiscoverPage() { .then(() => (after ? queryClient.invalidateQueries({ queryKey: DECK_KEY }, { cancelRefetch: false }) : undefined)) }, [queryClient, job.status, stored]) - // 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 + // A failed decision can be retried only while the deck's newest read + // still has its Side quest pending: anything else decided it already. + const pending = decision.isError ? stillPending(deck, decision.variables.quest.id) : undefined const retryable = pending !== undefined && pending !== null let body diff --git a/web/src/discover/api.ts b/web/src/discover/api.ts index 9ee08c4..1251e5a 100644 --- a/web/src/discover/api.ts +++ b/web/src/discover/api.ts @@ -130,18 +130,13 @@ 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 - } +// stillPending says whether the deck's newest read has the Side quest id +// pending, after a failed decision on it: the Side quest when it does, null +// when it does not, and undefined when that read failed, which says neither. +// deck is the deck query's state, from the cache or useDeck. +export function stillPending(deck: { status: string; data: SideQuest[] | undefined } | undefined, id: string): SideQuest | null | undefined { + if (deck?.status !== 'success') return undefined + return deck.data!.find((q) => q.id === id) ?? null } // useDecide records Try it (POST /api/v1/suggestions/{id}/accept) or Pass @@ -158,9 +153,10 @@ export class DecisionError extends Error { // (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. +// A failed decision waits for the deck read it makes itself. Whether its +// Side quest is still pending is then the deck query's to say, from its +// newest successful read: once the reads have landed, a Side quest the deck +// still has goes back on top of it. export function useDecide() { const { apiFetch } = useApi() const queryClient = useQueryClient() @@ -184,21 +180,20 @@ export function useDecide() { return body.replenish_job_id } - return useMutation({ + 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) + // read already out. A failed read is the deck query's error, shown + // with the deck; the decision's own error is the one thrown. + await queryClient + .getQueryCache() + .build(queryClient, { queryKey: DECK_KEY }) + .fetch(undefined, { cancelRefetch: true }) + .catch(() => undefined) + throw error } }, onMutate: ({ quest, choice }) => { @@ -209,16 +204,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) => { + onSettled: async (_replenishJobId, error, { quest }) => { 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. + // A failed decision has read the deck already; any read since is newer. await queryClient.invalidateQueries({ queryKey: QUEST_LOG_KEY }) - const { pending } = error + const pending = stillPending(queryClient.getQueryState(DECK_KEY), quest.id) if (pending === null || pending === undefined) return - queryClient.setQueryData(DECK_KEY, (deck) => [pending, ...(deck ?? []).filter((q) => q.id !== pending.id)]) + queryClient.setQueryData(DECK_KEY, (deck) => [pending, ...deck!.filter((q) => q.id !== pending.id)]) }, }) }