diff --git a/web/src/discover/DiscoverPage.test.tsx b/web/src/discover/DiscoverPage.test.tsx index 67cf6dc..180846f 100644 --- a/web/src/discover/DiscoverPage.test.tsx +++ b/web/src/discover/DiscoverPage.test.tsx @@ -549,8 +549,8 @@ describe('DiscoverPage', () => { await waitFor(() => expect(screen.getByRole('button', { name: 'Try it' })).toBeEnabled()) expect(screen.getByText('Quest log · 7')).toBeInTheDocument() expect(topCard()).toHaveAccessibleName('Crock pot chicken tikka masala') - // Three Side quests are left: the deck is not read again. - expect(deckReads(server.fetchMock)).toBe(1) + // The deck is read again once the decision is saved. + expect(deckReads(server.fetchMock)).toBe(2) expect(screen.queryByRole('alert')).not.toBeInTheDocument() }) @@ -571,7 +571,7 @@ describe('DiscoverPage', () => { decline.resolve(declined(BOWLS)) await waitFor(() => expect(screen.getByRole('button', { name: 'Pass' })).toBeEnabled()) expect(screen.getByText('Quest log · 6')).toBeInTheDocument() - expect(deckReads(server.fetchMock)).toBe(1) + expect(deckReads(server.fetchMock)).toBe(2) }) it('puts a refused Try it back on top of the deck, reverts the Quest log count and offers Retry', async () => { @@ -588,12 +588,14 @@ describe('DiscoverPage', () => { await screen.findByText('Quest log · 7') expect(topCard()).toHaveAccessibleName('Crock pot chicken tikka masala') - // The server refuses the request itself: the Side quest is still pending. + // The server refuses the request itself: the deck read again after it + // still has the Side quest pending, in the server's order. + server.deck = [TIKKA, BOWLS, ORZO, PAELLA] accept.resolve(errorResponse(400, 'invalid_request')) const alert = await screen.findByRole('alert') expect(alert).toHaveTextContent("Couldn't save that choice.") expect(topCard()).toHaveAccessibleName('Korean beef bowls') - expect(deckReads(server.fetchMock)).toBe(1) + expect(deckReads(server.fetchMock)).toBe(2) expect(screen.getAllByRole('article', { hidden: true }).map((card) => card.querySelector('h2')!.textContent)).toEqual([ 'Korean beef bowls', 'Crock pot chicken tikka masala', @@ -707,10 +709,9 @@ describe('DiscoverPage', () => { expect(screen.getByText('Quest log · 2')).toBeInTheDocument() }) - it('sends a decision for the card thrown out even when a read of the deck took it away during the exit, and Retry sends it again', async () => { - let refuse = true + it('sends a decision for the card thrown out even when a read of the deck took it away during the exit, offering no Retry for it', async () => { const server = stubServer([BOWLS], { - 'POST /api/v1/suggestions/s1/accept': () => (refuse ? errorResponse(500, 'internal') : accepted(BOWLS)), + 'POST /api/v1/suggestions/s1/accept': () => errorResponse(500, 'internal'), }) const queryClient = createQueryClient() const animations = stubAnimate({ autoFinish: false }) @@ -726,15 +727,12 @@ describe('DiscoverPage', () => { await finishRunning(animations) await finishRunning(animations) const alert = await screen.findByRole('alert') - expect(alert).toHaveTextContent("Couldn't save that choice.") expect(sent(server.fetchMock)).toEqual(['POST /api/v1/suggestions/s1/accept']) - // The read had taken the card away: refused, it is not put back. + // The deck read after it confirms the Side quest is decided: the card + // is not put back, and there is nothing to retry. + expect(alert).toHaveTextContent('That side quest has been decided already.') + expect(within(alert).queryByRole('button')).not.toBeInTheDocument() expect(screen.queryByRole('article')).not.toBeInTheDocument() - - refuse = false - fireEvent.click(within(alert).getByRole('button', { name: 'Retry' })) - await waitFor(() => expect(screen.queryByRole('alert')).not.toBeInTheDocument()) - expect(sent(server.fetchMock)).toEqual(['POST /api/v1/suggestions/s1/accept', 'POST /api/v1/suggestions/s1/accept']) expect(screen.getByRole('heading', { name: 'All caught up' })).toBeInTheDocument() }) @@ -792,14 +790,78 @@ describe('DiscoverPage', () => { const pending = read server.deck = [TIKKA, ORZO, PAELLA] await act(async () => accept.resolve(errorResponse(409, 'suggestion_decided'))) - await screen.findByRole('alert') - expect(screen.queryByRole('article', { name: 'Korean beef bowls' })).not.toBeInTheDocument() await waitFor(() => expect(deckReads(server.fetchMock)).toBe(2)) expect(screen.queryByRole('article', { name: 'Korean beef bowls' })).not.toBeInTheDocument() + // The decision is not over until that read lands. + expect(screen.queryByRole('alert')).not.toBeInTheDocument() + expect(screen.getByRole('button', { name: 'Try it' })).toBeDisabled() await act(async () => pending.resolve(jsonResponse(200, { suggestions: server.deck }))) + const alert = await screen.findByRole('alert') expect(screen.queryByRole('article', { name: 'Korean beef bowls' })).not.toBeInTheDocument() expect(topCard()).toHaveAccessibleName('Crock pot chicken tikka masala') + // It was decided: there is no decision to retry. + expect(alert).toHaveTextContent('That side quest has been decided already.') + expect(within(alert).queryByRole('button')).not.toBeInTheDocument() + expect(screen.getByRole('button', { name: 'Try it' })).toBeEnabled() + }) + + it('reads the Quest log again after a Pass another session beat with Try it (409), counting its dinner', async () => { + const server = stubServer([BOWLS, TIKKA, ORZO, PAELLA], { + 'POST /api/v1/suggestions/s1/decline': () => { + // Another session tried it first: its dinner is in the Quest log. + server.deck = [TIKKA, ORZO, PAELLA] + server.questLog = 7 + return errorResponse(409, 'suggestion_decided') + }, + }) + await renderPage() + await screen.findByRole('article', { name: 'Korean beef bowls' }) + await screen.findByText('Quest log · 6') + + fireEvent.click(screen.getByRole('button', { name: 'Pass' })) + const alert = await screen.findByRole('alert') + expect(alert).toHaveTextContent('That side quest has been decided already.') + expect(screen.getByText('Quest log · 7')).toBeInTheDocument() + expect(topCard()).toHaveAccessibleName('Crock pot chicken tikka masala') + }) + + it('locks the next decision until the reads after a failed one land, then puts the still-pending card back on top with a Retry', async () => { + let read: ReturnType | null = null + const server = stubServer([BOWLS, TIKKA, ORZO, PAELLA], { + 'POST /api/v1/suggestions/s1/accept': () => { + read = deferred() + return errorResponse(500, 'internal') + }, + [`GET ${DECK}`]: () => read?.promise ?? jsonResponse(200, { suggestions: server.deck }), + }) + await renderPage() + await screen.findByRole('article', { name: 'Korean beef bowls' }) + + // The decision failed and was not saved; the deck read after it is slow. + fireEvent.click(screen.getByRole('button', { name: 'Try it' })) + await waitFor(() => expect(deckReads(server.fetchMock)).toBe(2)) + const slow = read! + read = null + expect(topCard()).toHaveAccessibleName('Crock pot chicken tikka masala') + + // Deciding the next card does nothing while that read is out. + expect(screen.getByRole('button', { name: 'Pass' })).toBeDisabled() + fireEvent.click(screen.getByRole('button', { name: 'Pass' })) + fireEvent.keyDown(window, { key: 'ArrowLeft' }) + await act(async () => await new Promise((resolve) => setTimeout(resolve, 0))) + expect(sent(server.fetchMock)).toEqual(['POST /api/v1/suggestions/s1/accept']) + expect(screen.queryByRole('alert')).not.toBeInTheDocument() + + await act(async () => slow.resolve(jsonResponse(200, { suggestions: server.deck }))) + const alert = await screen.findByRole('alert') + expect(alert).toHaveTextContent("Couldn't save that choice.") + expect(topCard()).toHaveAccessibleName('Korean beef bowls') + expect(screen.getByRole('button', { name: 'Try it' })).toBeEnabled() + + // Retry sends it again. + fireEvent.click(within(alert).getByRole('button', { name: 'Retry' })) + await waitFor(() => expect(sent(server.fetchMock)).toEqual(['POST /api/v1/suggestions/s1/accept', 'POST /api/v1/suggestions/s1/accept'])) }) it('reads the deck again after a 500 to a decision, rather than putting the card back', async () => { @@ -1380,12 +1442,16 @@ describe('DiscoverPage', () => { const server = stubServer([BOWLS, TIKKA], { 'POST /api/v1/suggestions/s1/accept': () => (++calls === 1 ? errorResponse(500, 'internal') : retry.promise), }) - await renderPage() + 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') - // The refused Side quest is back on top of the deck. - const card = screen.getByRole('article', { name: 'Korean beef bowls' }) + // A read of the deck puts the refused Side quest back in the server's + // order, still pending, behind the card dragged during its Retry. + server.deck = [TIKKA, BOWLS] + await act(async () => await queryClient.invalidateQueries({ queryKey: DECK_KEY })) + const card = await screen.findByRole('article', { name: 'Crock pot chicken tikka masala' }) press(card) drag(card, 60, 0) @@ -1402,7 +1468,8 @@ describe('DiscoverPage', () => { expect(sent(server.fetchMock)).toEqual(['POST /api/v1/suggestions/s1/accept', 'POST /api/v1/suggestions/s1/accept']), ) // The retried Side quest leaves the deck while the Retry is out. - await screen.findByRole('article', { name: 'Crock pot chicken tikka masala' }) + await waitFor(() => expect(screen.queryByRole('article', { name: 'Korean beef bowls' })).not.toBeInTheDocument()) + server.deck = [TIKKA] retry.resolve(accepted(BOWLS)) await waitFor(() => expect(screen.getByRole('button', { name: 'Try it' })).toBeEnabled()) }) diff --git a/web/src/discover/DiscoverPage.tsx b/web/src/discover/DiscoverPage.tsx index 491b967..46c8bb9 100644 --- a/web/src/discover/DiscoverPage.tsx +++ b/web/src/discover/DiscoverPage.tsx @@ -90,10 +90,12 @@ 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 refused one again. Input is -// locked from the press until the decision settles: 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. +// 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 +// 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() { const queryClient = useQueryClient() const deck = useDeck() @@ -150,8 +152,9 @@ export function DiscoverPage() { setBusy(true) return true }, []) - // send records a decision. A refused one puts the card back; a saved one - // that started a replenish batch follows its job. + // send records a decision. A failed one stops the card's exit, so a card + // useDecide put back on the deck is at rest; a saved one that started a + // replenish batch follows its job. const send = useCallback( (variables: { quest: SideQuest; choice: Choice }) => { leaving.current = variables.quest.id @@ -314,6 +317,11 @@ export function DiscoverPage() { void queryClient.invalidateQueries({ queryKey: DECK_KEY }) }, [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) + let body if (deck.isError) { body = void deck.refetch()}>Couldn't load your side quests. {deck.error.message} @@ -380,10 +388,15 @@ export function DiscoverPage() { void questLog.refetch()}>Couldn't load the Quest log. {questLog.error.message} )} {body} - {/* A refused decision is shown whatever the deck has come to since. */} - {decision.isError && ( - submit(decision.variables!)}>Couldn't save that choice. {decision.error.message} - )} + {/* A failed decision is shown whatever the deck has come to since. */} + {decision.isError && + (retryable ? ( + submit(decision.variables)}>Couldn't save that choice. {decision.error.message} + ) : deck.isError ? ( + 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 d7ee63f..47b78eb 100644 --- a/web/src/discover/api.ts +++ b/web/src/discover/api.ts @@ -1,5 +1,5 @@ -import { hashKey, useMutation, useQuery, useQueryClient } from '@tanstack/react-query' -import { ApiError, useApi } from '../api/index.ts' +import { useMutation, useQuery, useQueryClient } from '@tanstack/react-query' +import { useApi } from '../api/index.ts' import { isRecord } from '../api/errors.ts' import { DIFFICULTIES, DINNERS_PATH, oneOf, photoId, type Difficulty } from '../dinners/api.ts' @@ -38,13 +38,7 @@ export const BATCH_JOB_KIND = 'suggestion_batch' export type BatchResult = { batch_id: string; stage: string; suggestion_ids: string[]; dropped: number } // QuestLogDinner is a dinner in the Quest log, as the Quest log pill lists it. -// adding marks the entry a Try it put in before it was saved, so its -// rollback takes out that entry and no other. -export type QuestLogDinner = { id: string; name: string; adding?: symbol } - -// REFILL_BELOW is the deck size, the depths the deck shows, under which a -// saved decision reads the deck again. -export const REFILL_BELOW = 3 +export type QuestLogDinner = { id: string; name: string } // DECISION_STATUS is the status a suggestion has once each choice is saved. const DECISION_STATUS: Record = { try: 'accepted', pass: 'declined' } @@ -136,24 +130,6 @@ export function useStartBatch() { }) } -// Decided is the context a decision's optimistic update leaves for its -// rollback: whether the Side quest was taken off the deck, the mark of the -// Quest log entry it added, if it added one, and what the reads of the deck -// that landed since have said of the Side quest. -type Decided = { taken: boolean; adding: symbol | null; reads: DeckReads } - -// DeckReads follows the reads of the deck that land while a decision is in -// flight: gone is whether the latest left the Side quest out. -type DeckReads = { gone: boolean; stop: () => void } - -// stillPending is whether a failed decision's error shows its Side quest -// still pending: an API error answer refusing the request itself. A 404 or -// 409 says it is gone or decided already, a 5xx answer may have come after -// the decision was saved, and a failure without an answer may have too. -function stillPending(error: Error): boolean { - return error instanceof ApiError && error.status >= 400 && error.status < 500 && error.status !== 404 && error.status !== 409 -} - // 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. @@ -161,16 +137,14 @@ function stillPending(error: Error): boolean { // is the id of the replenish job the decision started, or null. // // The decision is applied at once: the Side quest leaves the deck and, on -// Try it, its dinner joins the Quest log unless it is there already. A -// failed decision takes out of the Quest log only the entry it added and -// reads the Quest log again, since the decision may have been saved all the -// same. One the server refused with the Side quest still pending (see -// stillPending) puts it back on top of the deck if it took it off and no -// read of the deck since has left it out; any other failure reads the deck -// again instead, leaving the Side quest off it until that read answers. A -// saved one reads the Quest log again, and the deck too once fewer than -// REFILL_BELOW Side quests are left in it or a read of the deck started -// before the decision was saved. +// Try it, its dinner joins the Quest log unless it is there already. +// 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. export function useDecide() { const { apiFetch } = useApi() const queryClient = useQueryClient() @@ -192,47 +166,21 @@ export function useDecide() { } return body.replenish_job_id }, - onMutate: async ({ quest, choice }): Promise => { - // A read already under way would answer with the Side quest still - // pending, undoing the update. - await Promise.all([queryClient.cancelQueries({ queryKey: DECK_KEY }), queryClient.cancelQueries({ queryKey: QUEST_LOG_KEY })]) - const deck = queryClient.getQueryData(DECK_KEY) - const taken = deck !== undefined && deck.some((q) => q.id === quest.id) - if (taken) queryClient.setQueryData(DECK_KEY, deck.filter((q) => q.id !== quest.id)) - const reads: DeckReads = { gone: false, stop: () => {} } - const deckHash = hashKey(DECK_KEY) - reads.stop = queryClient.getQueryCache().subscribe((event) => { - if (event.type !== 'updated' || event.query.queryHash !== deckHash || event.action.type !== 'success' || event.action.manual) return - reads.gone = !(event.action.data as SideQuest[]).some((q) => q.id === quest.id) + onMutate: ({ quest, choice }) => { + queryClient.setQueryData(DECK_KEY, (deck) => deck?.filter((q) => q.id !== quest.id)) + if (choice !== 'try') return + // A stale deck can offer a Side quest whose dinner the Quest log has. + queryClient.setQueryData(QUEST_LOG_KEY, (dinners) => + 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)] }) - // A stale deck can offer a Side quest whose dinner a read of the Quest - // log already has. - const dinners = queryClient.getQueryData(QUEST_LOG_KEY) - if (choice !== 'try' || dinners === undefined || dinners.some((d) => d.id === quest.dinner_id)) return { taken, adding: null, reads } - const adding = Symbol(quest.dinner_id) - queryClient.setQueryData(QUEST_LOG_KEY, dinners.concat({ id: quest.dinner_id, name: quest.name, adding })) - return { taken, adding, reads } }, - onError: (error, { quest }, context) => { - const { taken, adding, reads } = context! - if (!stillPending(error)) void queryClient.invalidateQueries({ queryKey: DECK_KEY }) - else if (taken && !reads.gone) queryClient.setQueryData(DECK_KEY, (deck) => deck && [quest, ...deck.filter((q) => q.id !== quest.id)]) - if (adding === null) return - // A read of the Quest log that landed meanwhile has replaced the entry. - queryClient.setQueryData(QUEST_LOG_KEY, (dinners) => dinners?.filter((d) => d.adding !== adding)) - void queryClient.invalidateQueries({ queryKey: QUEST_LOG_KEY }) - }, - onSuccess: async (_replenishJobId, { quest: { id } }) => { - // A read that landed while the decision was in flight may have put - // the Side quest back, and one still under way may yet: invalidating - // cancels it for a read made after the decision was saved. - const reading = queryClient.isFetching({ queryKey: DECK_KEY }) > 0 - const deck = queryClient.setQueryData(DECK_KEY, (current) => current?.filter((q) => q.id !== id)) - await Promise.all([ - reading || (deck !== undefined && deck.length < REFILL_BELOW) ? queryClient.invalidateQueries({ queryKey: DECK_KEY }) : undefined, - queryClient.invalidateQueries({ queryKey: QUEST_LOG_KEY }), - ]) - }, - onSettled: (_data, _error, _variables, context) => context?.reads.stop(), }) } diff --git a/web/src/ui/ErrorBanner.test.tsx b/web/src/ui/ErrorBanner.test.tsx index 811b5e5..71a91f0 100644 --- a/web/src/ui/ErrorBanner.test.tsx +++ b/web/src/ui/ErrorBanner.test.tsx @@ -24,6 +24,12 @@ describe('ErrorBanner', () => { expect(screen.getByRole('button', { name: 'Rebuild week' })).toBeInTheDocument() }) + it('has no button without onRetry', () => { + render(That was done already.) + expect(screen.getByRole('alert')).toHaveTextContent('That was done already.') + expect(screen.queryByRole('button')).not.toBeInTheDocument() + }) + it('is a neutral-100 card with grape-700 text', () => { render( {}}>Offline) expect(declaredStyle(screen.getByRole('alert')).background).toBe('var(--color-neutral-100)') diff --git a/web/src/ui/ErrorBanner.tsx b/web/src/ui/ErrorBanner.tsx index 1274bf9..5537f43 100644 --- a/web/src/ui/ErrorBanner.tsx +++ b/web/src/ui/ErrorBanner.tsx @@ -6,16 +6,18 @@ import styles from './ErrorBanner.module.css' export type ErrorBannerProps = { // What went wrong, in the caller's words. children: ReactNode - onRetry: () => void + // What Retry does; without it there is nothing to retry and no button. + onRetry?: () => void retryLabel?: string } -// Muted neutral-100 card with grape-700 text and a Retry button. +// Muted neutral-100 card with grape-700 text and, given onRetry, a Retry +// button. export function ErrorBanner({ children, onRetry, retryLabel = 'Retry' }: ErrorBannerProps) { return (

{children}

- + {onRetry !== undefined && } ) }