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<typeof deferred> | 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())
     })

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 = <ErrorBanner onRetry={() => void deck.refetch()}>Couldn't load your side quests. {deck.error.message}</ErrorBanner>
@@ -380,10 +388,15 @@ export function DiscoverPage() {
         <ErrorBanner onRetry={() => void questLog.refetch()}>Couldn't load the Quest log. {questLog.error.message}</ErrorBanner>
       )}
       {body}
-      {/* A refused decision is shown whatever the deck has come to since. */}
-      {decision.isError && (
-        <ErrorBanner onRetry={() => submit(decision.variables!)}>Couldn't save that choice. {decision.error.message}</ErrorBanner>
-      )}
+      {/* A failed decision is shown whatever the deck has come to since. */}
+      {decision.isError &&
+        (retryable ? (
+          <ErrorBanner onRetry={() => submit(decision.variables)}>Couldn't save that choice. {decision.error.message}</ErrorBanner>
+        ) : deck.isError ? (
+          <ErrorBanner>Couldn't save that choice. {decision.error.message}</ErrorBanner>
+        ) : (
+          <ErrorBanner>That side quest has been decided already. {decision.error.message}</ErrorBanner>
+        ))}
     </div>
   )
 }

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<Choice, string> = { 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<Decided> => {
-      // 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<SideQuest[]>(DECK_KEY)
-      const taken = deck !== undefined && deck.some((q) => q.id === quest.id)
-      if (taken) queryClient.setQueryData<SideQuest[]>(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<SideQuest[]>(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<QuestLogDinner[]>(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<SideQuest[]>(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<QuestLogDinner[]>(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<QuestLogDinner[]>(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<SideQuest[]>(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<QuestLogDinner[]>(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<SideQuest[]>(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(),
   })
 }

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(<ErrorBanner>That was done already.</ErrorBanner>)
+    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(<ErrorBanner onRetry={() => {}}>Offline</ErrorBanner>)
     expect(declaredStyle(screen.getByRole('alert')).background).toBe('var(--color-neutral-100)')

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 (
     <Card tone="muted" role="alert" className={styles.banner}>
       <p className={styles.message}>{children}</p>
-      <Button onClick={onRetry}>{retryLabel}</Button>
+      {onRetry !== undefined && <Button onClick={onRetry}>{retryLabel}</Button>}
     </Card>
   )
 }