From fe87968ac9c89541cbfddbe8b91fa9fc2f42a80a Mon Sep 17 00:00:00 2001 From: dev_agent Date: Wed, 7 Oct 2026 01:13:00 -0400 Subject: [PATCH] Fix the Discover deck review findings The Quest log pill is now a button that shows and hides the names of the dinners in the Quest log, read from GET /api/v1/dinners?status=quest_log; the designed Quest log sheet replaces this list in its own task. A batch whose job cannot be polled, or whose result cannot be read, now shows an error with a Retry that asks for the batch again, instead of leaving skeleton cards on screen with no way out. Decisions go through one submit guarded by a ref set before the request is sent, so presses before the next render cannot send a second, conflicting decision. A decision counts as saved only when the answer is that suggestion, accepted or declined as chosen; anything else shows in the "Couldn't save that choice" banner. Pass and Try it are IconButtons: IconButton gains a neutral tone and showLabel, which shows the label as an 11 px caption under the circle. Maestro task 1ecc121c-86eb-4784-a9fb-1314709b4a59. Co-Authored-By: Claude Opus 5.5 --- web/src/discover/DiscoverPage.module.css | 91 ++++++++---------- web/src/discover/DiscoverPage.test.tsx | 113 +++++++++++++++++++++++ web/src/discover/DiscoverPage.tsx | 110 ++++++++++++++++------ web/src/discover/api.ts | 51 ++++++++-- web/src/ui/IconButton.module.css | 19 ++++ web/src/ui/IconButton.test.tsx | 18 ++++ web/src/ui/IconButton.tsx | 16 +++- 7 files changed, 325 insertions(+), 93 deletions(-) diff --git a/web/src/discover/DiscoverPage.module.css b/web/src/discover/DiscoverPage.module.css index cd72438..bcae6e1 100644 --- a/web/src/discover/DiscoverPage.module.css +++ b/web/src/discover/DiscoverPage.module.css @@ -10,6 +10,7 @@ .header { display: flex; + flex-wrap: wrap; align-items: flex-end; justify-content: space-between; gap: 16px; @@ -33,7 +34,20 @@ line-height: 1.1; } -/* The Quest log count pill. */ +/* The Quest log count pill, in a 44 px tall hit box. */ +.questLogButton { + flex: none; + display: inline-flex; + align-items: center; + min-width: 44px; + min-height: 44px; + padding: 0; + border: 0; + border-radius: var(--radius-pill); + background: transparent; + cursor: pointer; +} + .questLog { flex: none; padding: 7px 12px; @@ -45,6 +59,26 @@ font-weight: 700; } +/* The Quest log's dinner names, under the header while the pill is open. */ +.questLogList { + flex: 1 0 100%; + font-family: var(--font-body); + font-size: 14px; +} + +.questLogNames { + margin: 0; + padding-left: 20px; + display: flex; + flex-direction: column; + gap: 6px; +} + +.questLogEmpty { + margin: 0; + color: var(--color-neutral-700); +} + /* The deck: three cards stacked in the same box, the top one in front. */ .deck { position: relative; @@ -172,7 +206,7 @@ color: var(--color-neutral-900); } -/* Pass and Try it: a 62 px circle over an 11 px label, one button each. */ +/* Pass and Try it: IconButtons with their 11 px labels shown. */ .actions { display: flex; justify-content: center; @@ -180,53 +214,6 @@ margin-top: 4px; } -.action { - display: flex; - flex-direction: column; - align-items: center; - gap: 6px; - min-width: 62px; - min-height: 62px; - padding: 0; - border: 0; - background: none; - color: var(--color-neutral-700); - font-family: var(--font-body); - font-size: 11px; - font-weight: 700; - cursor: pointer; -} - -.action:disabled { - opacity: 0.45; - cursor: not-allowed; -} - -.circle { - display: flex; - align-items: center; - justify-content: center; - width: 62px; - height: 62px; - border-radius: var(--radius-pill); - box-shadow: var(--shadow-md); - transition: transform 100ms var(--ease-out-soft); -} - -.action:active:not(:disabled) .circle { - transform: scale(0.97); -} - -.pass { - background: var(--color-neutral-100); - color: var(--color-text); -} - -.tryIt { - background: var(--color-accent-2-600); - color: var(--on-accent); -} - /* Skeleton cards while the deck loads or a batch finds Side quests. */ .loading { display: flex; @@ -256,9 +243,3 @@ font-weight: 700; color: var(--color-neutral-700); } - -@media (prefers-reduced-motion: reduce) { - .circle { - transition: none; - } -} diff --git a/web/src/discover/DiscoverPage.test.tsx b/web/src/discover/DiscoverPage.test.tsx index 202fb9e..3589aa1 100644 --- a/web/src/discover/DiscoverPage.test.tsx +++ b/web/src/discover/DiscoverPage.test.tsx @@ -328,4 +328,117 @@ describe('DiscoverPage', () => { fireEvent.click(within(alert).getByRole('button', { name: 'Retry' })) await screen.findByRole('article', { name: 'Korean beef bowls' }) }) + + it('opens and closes the Quest log from its pill, a keyboard-reachable button', async () => { + stubServer([BOWLS], {}, 2) + await renderPage() + const pill = await screen.findByRole('button', { name: 'Quest log · 2' }) + expect(pill).toHaveAttribute('aria-expanded', 'false') + expect(screen.queryByRole('list', { name: 'Quest log' })).not.toBeInTheDocument() + expectMinHitArea(pill) + pill.focus() + expect(pill).toHaveFocus() + + fireEvent.click(pill) + expect(pill).toHaveAttribute('aria-expanded', 'true') + const list = screen.getByRole('list', { name: 'Quest log' }) + expect(pill).toHaveAttribute('aria-controls', list.parentElement!.id) + expect(within(list).getAllByRole('listitem').map((item) => item.textContent)).toEqual(['Quest 0', 'Quest 1']) + + fireEvent.click(pill) + expect(pill).toHaveAttribute('aria-expanded', 'false') + expect(screen.queryByRole('list', { name: 'Quest log' })).not.toBeInTheDocument() + }) + + it('says so when the Quest log it opens is empty', async () => { + stubServer([BOWLS], {}, 0) + await renderPage() + fireEvent.click(await screen.findByRole('button', { name: 'Quest log · 0' })) + expect(screen.getByText('No dinners in the Quest log yet. Try it on a Side quest to add one.')).toBeInTheDocument() + }) + + it('sends one decision for presses made before the page re-renders', async () => { + const accept = deferred() + const server = stubServer([BOWLS, TIKKA], { 'POST /api/v1/suggestions/s1/accept': () => accept.promise }) + await renderPage() + await screen.findByRole('article', { name: 'Korean beef bowls' }) + + fireEvent.keyDown(window, { key: 'ArrowRight' }) + fireEvent.keyDown(window, { key: 'ArrowLeft' }) + fireEvent.click(screen.getByRole('button', { name: 'Pass' })) + await waitFor(() => expect(sent(server.fetchMock)).toContain('POST /api/v1/suggestions/s1/accept')) + expect(sent(server.fetchMock)).toEqual(['POST /api/v1/suggestions/s1/accept']) + expect(screen.getByRole('button', { name: 'Try it' })).toBeDisabled() + + server.deck = [TIKKA] + accept.resolve(jsonResponse(200, { suggestion: { ...BOWLS, status: 'accepted' }, replenish_job_id: null })) + await screen.findByRole('article', { name: 'Crock pot chicken tikka masala' }) + fireEvent.keyDown(window, { key: 'ArrowLeft' }) + await waitFor(() => + expect(sent(server.fetchMock)).toEqual(['POST /api/v1/suggestions/s1/accept', 'POST /api/v1/suggestions/s2/decline']), + ) + }) + + it.each([ + ['not a decision', {}], + ['another suggestion', { suggestion: { ...TIKKA, status: 'accepted' }, replenish_job_id: null }], + ['not accepted', { suggestion: { ...BOWLS, status: 'pending' }, replenish_job_id: null }], + ])('shows a decision answered with %s as unsaved, keeping the card', async (_, answer) => { + stubServer([BOWLS], { 'POST /api/v1/suggestions/s1/accept': () => jsonResponse(200, answer) }) + await renderPage() + await screen.findByRole('article', { name: 'Korean beef bowls' }) + + fireEvent.click(screen.getByRole('button', { name: 'Try it' })) + const alert = await screen.findByRole('alert') + expect(alert).toHaveTextContent("Couldn't save that choice. POST /api/v1/suggestions/s1/accept: response is not the accepted suggestion") + expect(within(alert).getByRole('button', { name: 'Retry' })).toBeInTheDocument() + expect(screen.getByRole('article', { name: 'Korean beef bowls' })).toBeInTheDocument() + }) + + it('shows a batch whose job cannot be polled with a retry, keeping the skeleton cards', async () => { + let failPoll = true + const server = stubServer([], { + 'POST /api/v1/suggestions/batches': () => + jsonResponse(202, { batch_id: 'b1', job_id: 'j1', trigger: 'manual', status: 'pending' }), + 'GET /api/v1/jobs/j1': () => { + if (failPoll) return errorResponse(500, 'internal') + server.deck = [BOWLS] + return jsonResponse(200, { + id: 'j1', + kind: 'suggestion_batch', + status: 'succeeded', + result: { batch_id: 'b1', stage: 'done', suggestion_ids: ['s1'], dropped: 0 }, + error: null, + }) + }, + }) + await renderPage() + fireEvent.click(await screen.findByRole('button', { name: 'Get more now' })) + + const alert = await screen.findByRole('alert') + expect(alert).toHaveTextContent("Couldn't check on new side quests.") + expect(screen.getAllByTestId('skeleton-card')).toHaveLength(3) + + failPoll = false + fireEvent.click(within(alert).getByRole('button', { name: 'Retry' })) + await screen.findByRole('article', { name: 'Korean beef bowls' }) + expect(screen.queryByRole('alert')).not.toBeInTheDocument() + expect(sent(server.fetchMock)).toEqual(['POST /api/v1/suggestions/batches', 'POST /api/v1/suggestions/batches']) + }) + + it('shows a batch whose job result is unreadable with a retry', async () => { + stubServer([], { + 'POST /api/v1/suggestions/batches': () => + jsonResponse(202, { batch_id: 'b1', job_id: 'j1', trigger: 'manual', status: 'pending' }), + 'GET /api/v1/jobs/j1': () => + jsonResponse(200, { id: 'j1', kind: 'suggestion_batch', status: 'succeeded', result: { batch_id: 'b1' }, error: null }), + }) + await renderPage() + fireEvent.click(await screen.findByRole('button', { name: 'Get more now' })) + + const alert = await screen.findByRole('alert') + expect(alert).toHaveTextContent("Couldn't find new side quests. GET /api/v1/jobs/j1: result is not a suggestion batch") + expect(within(alert).getByRole('button', { name: 'Retry' })).toBeInTheDocument() + expect(screen.getByRole('heading', { name: 'All caught up' })).toBeInTheDocument() + }) }) diff --git a/web/src/discover/DiscoverPage.tsx b/web/src/discover/DiscoverPage.tsx index 6ec9e04..45f8a4e 100644 --- a/web/src/discover/DiscoverPage.tsx +++ b/web/src/discover/DiscoverPage.tsx @@ -1,18 +1,20 @@ import { useQueryClient } from '@tanstack/react-query' import { Heart, X } from 'lucide-react' -import { useCallback, useEffect } from 'react' +import { useCallback, useEffect, useId, useMemo, useRef, useState } from 'react' import { useJob } from '../api/index.ts' +import { Card } from '../ui/Card.tsx' import { EmptyState } from '../ui/EmptyState.tsx' import { ErrorBanner } from '../ui/ErrorBanner.tsx' import { Icon } from '../ui/Icon.tsx' +import { IconButton } from '../ui/IconButton.tsx' import { Skeleton } from '../ui/Skeleton.tsx' import { DECK_KEY, + parseBatchResult, useDeck, useDecide, - useQuestLogCount, + useQuestLog, useStartBatch, - type BatchResult, type Choice, } from './api.ts' import { DeckCard, DECK_DEPTH } from './DeckCard.tsx' @@ -27,29 +29,88 @@ function isEditable(target: EventTarget | null): boolean { ) } +// QuestLogPill is the "Quest log · N" pill. It shows and hides the names +// of the dinners in the Quest log; the Quest log sheet replaces this list. +function QuestLogPill({ dinners }: { dinners: { id: string; name: string }[] }) { + const [open, setOpen] = useState(false) + const listId = useId() + return ( + <> + + {open && ( + + {dinners.length === 0 ? ( +

No dinners in the Quest log yet. Try it on a Side quest to add one.

+ ) : ( +
    + {dinners.map((dinner) => ( +
  • {dinner.name}
    • + ))} +
+ )} + + )} + + ) +} + // DiscoverPage is mock 1a: the deck of Side quests to Try it or Pass, one // at a time from the top card. Every decision — the buttons, the ← / → -// keys — goes through decide, which does nothing while busy. +// keys, Retry — goes through submit, which does nothing while one is in +// flight. The lock is a ref, set before the decision is sent, so a second +// press before the next render cannot send another. export function DiscoverPage() { const queryClient = useQueryClient() const deck = useDeck() - const questLog = useQuestLogCount() + const questLog = useQuestLog() const decision = useDecide() const startBatch = useStartBatch() - const job = useJob(startBatch.data?.job_id ?? null) + const job = useJob(startBatch.data?.job_id ?? null) const top = deck.data?.[0] const busy = decision.isPending const jobRunning = startBatch.isSuccess && job.status !== 'succeeded' && job.status !== 'failed' const finding = startBatch.isPending || jobRunning + // The batch's result as far as it has gone, or why it cannot be read. + const batch = useMemo(() => { + if (job.result === null) return { stored: 0, error: null } + try { + return { stored: parseBatchResult(`GET /api/v1/jobs/${startBatch.data?.job_id}`, job.result).suggestion_ids.length, error: null } + } catch (error) { + return { stored: 0, error: error as Error } + } + }, [job.result, startBatch.data?.job_id]) + // The job's failure, the latest poll's failure, or an unreadable result. + const batchError = job.error ?? batch.error + const { mutate: mutateDecision } = decision + const deciding = useRef(false) + const submit = useCallback( + (variables: { id: string; choice: Choice }) => { + if (deciding.current) return + deciding.current = true + mutateDecision(variables, { + onSettled: () => { + deciding.current = false + }, + }) + }, + [mutateDecision], + ) const decide = useCallback( (choice: Choice) => { - if (busy || top === undefined) return - mutateDecision({ id: top.id, choice }) + if (top !== undefined) submit({ id: top.id, choice }) }, - [busy, top, mutateDecision], + [top, submit], ) useEffect(() => { @@ -67,7 +128,7 @@ export function DiscoverPage() { // The batch stores its Side quests as it goes, so the deck is read again // whenever it reports more, and once it ends. - const stored = job.result?.suggestion_ids.length ?? 0 + const { stored } = batch useEffect(() => { if (job.status === undefined) return void queryClient.invalidateQueries({ queryKey: DECK_KEY }) @@ -85,6 +146,9 @@ export function DiscoverPage() { ))} {finding &&

Finding new side quests…

} + {jobRunning && batchError !== null && ( + startBatch.mutate()}>Couldn't check on new side quests. {batchError.message} + )} ) } else if (top === undefined) { @@ -93,8 +157,8 @@ export function DiscoverPage() { {startBatch.isError && ( startBatch.mutate()}>Couldn't ask for more side quests. {startBatch.error.message} )} - {job.status === 'failed' && job.error !== null && ( - startBatch.mutate()}>Couldn't find new side quests. {job.error.message} + {batchError !== null && ( + startBatch.mutate()}>Couldn't find new side quests. {batchError.message} )} startBatch.mutate() }}> New side quests arrive Sunday. @@ -110,21 +174,15 @@ export function DiscoverPage() { ))}
- - + decide('pass')}> + + + decide('try')}> + +
{decision.isError && ( - mutateDecision(decision.variables!)}> + submit(decision.variables!)}> Couldn't save that choice. {decision.error.message} )} @@ -139,7 +197,7 @@ export function DiscoverPage() {

Discover

Side quests

- {questLog.isSuccess && Quest log · {questLog.data}} + {questLog.isSuccess && } {questLog.isError && ( void questLog.refetch()}>Couldn't load the Quest log. {questLog.error.message} diff --git a/web/src/discover/api.ts b/web/src/discover/api.ts index 0773611..b0a0a77 100644 --- a/web/src/discover/api.ts +++ b/web/src/discover/api.ts @@ -32,6 +32,28 @@ export type Batch = { batch_id: string; job_id: string } // BatchResult is the suggestion batch job's result, published as it goes. 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. +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' } + +// parseBatchResult checks a suggestion batch job's result, which useJob +// passes on unread. +export function parseBatchResult(where: string, body: unknown): BatchResult { + if ( + !isRecord(body) || + typeof body.batch_id !== 'string' || + typeof body.stage !== 'string' || + !Array.isArray(body.suggestion_ids) || + !body.suggestion_ids.every((id) => typeof id === 'string') || + typeof body.dropped !== 'number' + ) { + throw new Error(`${where}: result is not a suggestion batch`) + } + return { batch_id: body.batch_id, stage: body.stage, suggestion_ids: body.suggestion_ids as string[], dropped: body.dropped } +} + function parseSideQuest(where: string, body: unknown): SideQuest { if ( !isRecord(body) || @@ -65,16 +87,22 @@ export function useDeck() { }) } -// useQuestLogCount is how many dinners are in the Quest log (GET +// useQuestLog is the dinners in the Quest log, in the server's order (GET // /api/v1/dinners?status=quest_log). -export function useQuestLogCount() { +export function useQuestLog() { const { apiFetch } = useApi() return useQuery({ queryKey: QUEST_LOG_KEY, - queryFn: async () => { + queryFn: async (): Promise => { + const where = `GET ${QUEST_LOG_PATH}` const body = await apiFetch(QUEST_LOG_PATH) - if (!isRecord(body) || !Array.isArray(body.dinners)) throw new Error(`GET ${QUEST_LOG_PATH}: response has no dinners list`) - return body.dinners.length + if (!isRecord(body) || !Array.isArray(body.dinners)) throw new Error(`${where}: response has no dinners list`) + return body.dinners.map((dinner) => { + if (!isRecord(dinner) || typeof dinner.id !== 'string' || typeof dinner.name !== 'string') { + throw new Error(`${where}: response is not a dinner`) + } + return { id: dinner.id, name: dinner.name } + }) }, }) } @@ -97,14 +125,23 @@ export function useStartBatch() { // useDecide records Try it (POST /api/v1/suggestions/{id}/accept) or Pass // (POST /api/v1/suggestions/{id}/decline), then reads the deck and the -// Quest log again. +// Quest log again. The answer must be that suggestion, decided that way. export function useDecide() { const { apiFetch } = useApi() const queryClient = useQueryClient() return useMutation({ mutationFn: async ({ id, choice }: { id: string; choice: Choice }) => { const action = choice === 'try' ? 'accept' : 'decline' - await apiFetch(`${SUGGESTIONS_PATH}/${encodeURIComponent(id)}/${action}`, { method: 'POST' }) + 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`) + } }, onSuccess: async () => { await Promise.all([ diff --git a/web/src/ui/IconButton.module.css b/web/src/ui/IconButton.module.css index 7dd0c66..2995110 100644 --- a/web/src/ui/IconButton.module.css +++ b/web/src/ui/IconButton.module.css @@ -29,6 +29,20 @@ height: 62px; } +/* A shown label sits 6 px under the circle, and the button grows to hold it. */ +.labelled { + flex-direction: column; + gap: 6px; + width: auto; + min-width: 62px; + height: auto; + min-height: 62px; + color: var(--color-neutral-700); + font-family: var(--font-body); + font-size: 11px; + font-weight: 700; +} + .face { display: inline-flex; align-items: center; @@ -57,6 +71,11 @@ color: var(--color-text); } +.neutral { + background: var(--color-neutral-100); + color: var(--color-text); +} + .accent { background: var(--color-accent); color: var(--on-accent); diff --git a/web/src/ui/IconButton.test.tsx b/web/src/ui/IconButton.test.tsx index 0341c1a..e909cdd 100644 --- a/web/src/ui/IconButton.test.tsx +++ b/web/src/ui/IconButton.test.tsx @@ -1,5 +1,6 @@ import { fireEvent, render, screen } from '@testing-library/react' import { describe, expect, it, vi } from 'vitest' +import { declaredStyle } from '../test/cssRules.ts' import { hitArea, expectMinHitArea } from '../test/hitArea.ts' import { IconButton } from './IconButton.tsx' @@ -31,4 +32,21 @@ describe('IconButton', () => { expectMinHitArea(button) expect(hitArea(button)).toEqual({ width: 62, height: 62 }) }) + + it('shows its label as an 11 px caption under the circle, which names it', () => { + render( + + + , + ) + const button = screen.getByRole('button', { name: 'Pass' }) + expect(button).not.toHaveAttribute('aria-label') + expect(button).toHaveTextContent('Pass') + expect(getComputedStyle(button).fontSize).toBe('11px') + expect(getComputedStyle(button).flexDirection).toBe('column') + expectMinHitArea(button) + const face = button.firstElementChild! + expect(hitArea(face)).toEqual({ width: 62, height: 62 }) + expect(declaredStyle(face).background).toBe('var(--color-neutral-100)') + }) }) diff --git a/web/src/ui/IconButton.tsx b/web/src/ui/IconButton.tsx index 1a5ab73..6da1b22 100644 --- a/web/src/ui/IconButton.tsx +++ b/web/src/ui/IconButton.tsx @@ -2,32 +2,38 @@ import type { ComponentPropsWithRef } from 'react' import styles from './IconButton.module.css' export type IconButtonSize = 40 | 62 -export type IconButtonTone = 'surface' | 'accent' | 'teal' +export type IconButtonTone = 'surface' | 'neutral' | 'accent' | 'teal' export type IconButtonProps = Omit, 'aria-label'> & { // Accessible name; an icon alone never names a button. label: string size?: IconButtonSize tone?: IconButtonTone + // Shows the label as an 11 px caption under the circle, which then names + // the button. + showLabel?: boolean } -// Round icon-only button. The 40 px size keeps a 44 px hit box (see the CSS); -// the 62 px size is the round action button with --shadow-md. +// Round icon button, named by its label and showing it only with showLabel. +// The 40 px size keeps a 44 px hit box (see the CSS); the 62 px size is the +// round action button with --shadow-md. export function IconButton({ label, size = 40, tone = 'surface', + showLabel = false, type = 'button', className, children, ...rest }: IconButtonProps) { - const classes = [styles.button, styles[`size${size}`], className].filter(Boolean).join(' ') + const classes = [styles.button, styles[`size${size}`], showLabel && styles.labelled, className].filter(Boolean).join(' ') return ( - ) }