From af58caac94cc645548a4b35568e1e17d78b5d59e Mon Sep 17 00:00:00 2001 From: dev_agent Date: Wed, 7 Oct 2026 05:04:20 -0400 Subject: [PATCH] Surface replenish failures, follow the active batch, scope deck keys, keep three deck depths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes the Feature 13 review findings on the Discover deck (Maestro task 4fc551f0-3c11-4c15-b638-f2c08a28eb29): - F13-01: a batch failure (start, poll or job) is shown with its Retry whatever the deck has, so a replenish that fails while cards remain is no longer silent until the deck runs out. - F13-02: GET /api/v1/suggestions/deck reports the batch pending or running (active_batch, read in the deck's snapshot), and a decision that calls for a replenish returns the job of the batch it started or joined. The page follows a newly reported active batch, so a reopened page or a second session shows skeleton cards while the batch runs instead of "All caught up". - F13-03: ← / → leave the deck alone while it is inert behind a modal (the Quest log sheet or its confirmation). - F13-04: a one- or two-card deck keeps all three depths: skeleton cards stand in for the missing ones while a batch finds more Side quests, blank stack-edge cards while none is coming. Co-Authored-By: Claude Opus 5.5 --- docs/ARCHITECTURE.md | 14 +- internal/server/suggestions.go | 10 +- internal/server/suggestions_test.go | 40 +++- internal/suggest/suggest.go | 58 ++++-- internal/suggest/suggest_test.go | 43 ++++- web/src/App.test.tsx | 2 +- web/src/discover/DiscoverPage.module.css | 8 + web/src/discover/DiscoverPage.test.tsx | 228 +++++++++++++++++++++-- web/src/discover/DiscoverPage.tsx | 66 +++++-- web/src/discover/api.ts | 44 +++-- 10 files changed, 424 insertions(+), 89 deletions(-) diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 740d7f9..007ae36 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -310,9 +310,11 @@ automatic** (D-021). block the next. `GET /api/v1/suggestions/deck` returns `{suggestions: [{id, dinner_id, name, photo_url, difficulty, method, minutes_total, cuisine, reason, - cited_regular_ids, status, dinner_status, decided_at, created_at}]}`: - the `pending` suggestions whose dinner is still `suggested`, oldest - first. `POST /api/v1/suggestions/{id}/accept` (Try it) sets the + cited_regular_ids, status, dinner_status, decided_at, created_at}], + active_batch}`: the `pending` suggestions whose dinner is still + `suggested`, oldest first, and the batch `pending` or `running` + (`{batch_id, job_id, trigger, status}`, or null), read in one snapshot, + so a client that did not start the batch can follow its job. `POST /api/v1/suggestions/{id}/accept` (Try it) sets the suggestion `accepted` and moves its dinner to `quest_log`; `POST /api/v1/suggestions/{id}/decline` (Pass) sets it `declined` and archives the dinner; every later batch, and the household context's passed list, @@ -323,9 +325,9 @@ automatic** (D-021). and answers 200; the opposite decision is 409 `suggestion_decided`, a suggestion whose dinner left the deck some other way 409 `dinner_not_suggested`, and an unknown id 404. A decision that leaves - fewer than 5 in the deck starts a `replenish` batch, whose job id it - returns, unless a batch is already pending or running - (`replenish_job_id` is then null). The batch and its job are written in + fewer than 5 in the deck starts a `replenish` batch, or joins the batch + already pending or running, and returns that batch's job id either way + (`replenish_job_id` is null when the deck keeps 5 or more). The batch and its job are written in the decision's own transaction, so a decision is never recorded without the replenish it calls for; when they cannot be written, neither is the decision, and a retry decides again. diff --git a/internal/server/suggestions.go b/internal/server/suggestions.go index dbe1b84..f6e7711 100644 --- a/internal/server/suggestions.go +++ b/internal/server/suggestions.go @@ -10,10 +10,6 @@ import ( "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/suggest" ) -type deck struct { - Suggestions []suggest.Suggestion `json:"suggestions"` -} - // postSuggestionBatch starts a manual suggestion batch and answers 202 with // its batch_id and the job_id that GET /jobs/{id} reports on, without // waiting for the job. When a batch is already pending or running it @@ -30,15 +26,15 @@ func postSuggestionBatch(logger *slog.Logger, svc *suggest.Service) http.Handler } // getSuggestionDeck answers GET /suggestions/deck with the pending -// suggestions, oldest first. +// suggestions, oldest first, and the batch pending or running, or null. func getSuggestionDeck(logger *slog.Logger, svc *suggest.Service) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { - list, err := svc.Deck(r.Context()) + d, err := svc.Deck(r.Context()) if err != nil { writeSuggestionError(w, r, logger, "list suggestion deck", err) return } - writeJSON(w, r, logger, http.StatusOK, deck{Suggestions: list}) + writeJSON(w, r, logger, http.StatusOK, d) } } diff --git a/internal/server/suggestions_test.go b/internal/server/suggestions_test.go index 7f33f3f..f81dbe2 100644 --- a/internal/server/suggestions_test.go +++ b/internal/server/suggestions_test.go @@ -148,8 +148,44 @@ func TestSuggestionDecisions(t *testing.T) { if rec.Code != http.StatusOK { t.Fatalf("decline = %d: %s", rec.Code, rec.Body) } - if id, _ := body["replenish_job_id"].(string); id == "" { - t.Errorf("decline leaving 4 = %v, want a replenish_job_id", body) + replenish, _ := body["replenish_job_id"].(string) + if replenish == "" { + t.Fatalf("decline leaving 4 = %v, want a replenish_job_id", body) + } + + // A fourth decision joins the batch already pending and reports its job. + rec, body = callDinners(t, h, http.MethodPost, suggestionsPath+"/"+ids[3]+"/accept", "") + if rec.Code != http.StatusOK { + t.Fatalf("accept = %d: %s", rec.Code, rec.Body) + } + if body["replenish_job_id"] != replenish { + t.Errorf("accept leaving 3 = %v, want replenish_job_id %s", body, replenish) + } +} + +func TestSuggestionDeckActiveBatch(t *testing.T) { + t.Parallel() + h, _, _ := newSuggestionsRouter(t, "Pho") + + rec, body := callDinners(t, h, http.MethodGet, suggestionsPath+"/deck", "") + if rec.Code != http.StatusOK { + t.Fatalf("GET deck = %d: %s", rec.Code, rec.Body) + } + if active, ok := body["active_batch"]; !ok || active != nil { + t.Fatalf("deck = %v, want active_batch null", body) + } + + rec, started := callDinners(t, h, http.MethodPost, suggestionsPath+"/batches", "") + if rec.Code != http.StatusAccepted { + t.Fatalf("POST batches = %d: %s", rec.Code, rec.Body) + } + rec, body = callDinners(t, h, http.MethodGet, suggestionsPath+"/deck", "") + if rec.Code != http.StatusOK { + t.Fatalf("GET deck = %d: %s", rec.Code, rec.Body) + } + active, _ := body["active_batch"].(map[string]any) + if active == nil || active["batch_id"] != started["batch_id"] || active["job_id"] != started["job_id"] || active["status"] != "pending" { + t.Errorf("deck active_batch = %v, want the started batch %v", body["active_batch"], started) } } diff --git a/internal/suggest/suggest.go b/internal/suggest/suggest.go index efed277..81f398d 100644 --- a/internal/suggest/suggest.go +++ b/internal/suggest/suggest.go @@ -261,25 +261,54 @@ type Suggestion struct { CreatedAt string `json:"created_at"` } -// Deck returns the pending suggestions whose dinner is still suggested, -// oldest first. -func (s *Service) Deck(ctx context.Context) ([]Suggestion, error) { - rows, err := db.New(s.st.Reader).ListDeck(ctx) +// Deck is the household's deck: the pending suggestions whose dinner is +// still suggested, oldest first, and the batch pending or running, if any, +// so a client can follow the job producing more. +type Deck struct { + Suggestions []Suggestion `json:"suggestions"` + ActiveBatch *Batch `json:"active_batch"` +} + +// Deck reads the deck and its active batch in one read transaction, so +// both come from the same snapshot. +func (s *Service) Deck(ctx context.Context) (deck Deck, err error) { + tx, err := s.st.Reader.BeginTx(ctx, nil) if err != nil { - return nil, fmt.Errorf("suggest: list deck: %w", err) + return Deck{}, fmt.Errorf("suggest: begin deck read: %w", err) } - out := make([]Suggestion, len(rows)) + defer func() { + err = errors.Join(err, tx.Rollback()) + }() + q := db.New(tx) + rows, err := q.ListDeck(ctx) + if err != nil { + return Deck{}, fmt.Errorf("suggest: list deck: %w", err) + } + deck.Suggestions = make([]Suggestion, len(rows)) for i, row := range rows { - out[i], err = fromRow(row.Suggestion, row.Dinner) + deck.Suggestions[i], err = fromRow(row.Suggestion, row.Dinner) if err != nil { - return nil, err + return Deck{}, err } } - return out, nil + cur, err := q.GetActiveSuggestionBatch(ctx) + if errors.Is(err, sql.ErrNoRows) { + return deck, nil + } + if err != nil { + return Deck{}, fmt.Errorf("suggest: read active batch: %w", err) + } + jobID, err := q.GetSuggestionBatchJobID(ctx, db.GetSuggestionBatchJobIDParams{Kind: KindSuggestionBatch, BatchID: cur.ID}) + if err != nil { + return Deck{}, fmt.Errorf("suggest: read job of batch %s: %w", cur.ID, err) + } + deck.ActiveBatch = &Batch{ID: cur.ID, JobID: jobID, Trigger: cur.Trigger, Status: cur.Status} + return deck, nil } // Decision is a suggestion once decided, with the id of the replenish -// batch's job when the decision started one. +// batch's job when the decision called for one: the batch it started, or +// the one already pending or running that it joined. type Decision struct { Suggestion Suggestion `json:"suggestion"` ReplenishJobID *string `json:"replenish_job_id"` @@ -303,8 +332,8 @@ func (s *Service) Decline(ctx context.Context, id, updatedBy string) (Decision, // dinnerStatus. Repeating a decision changes nothing and returns the // suggestion as it is; the opposite decision is a *DecidedError. A // decision that leaves the deck under ReplenishBelow starts a replenish -// batch unless one is already pending or running, in the decision's own -// transaction: the decision is never recorded without the replenish it +// batch, or joins the one already pending or running, in the decision's own +// transaction, and returns its job id either way: the decision is never recorded without the replenish it // calls for, and a failure to start the batch records neither. func (s *Service) decide(ctx context.Context, id, decision, dinnerStatus, updatedBy string) (Decision, error) { if updatedBy == "" { @@ -369,8 +398,7 @@ func (s *Service) decide(ctx context.Context, id, decision, dinnerStatus, update if err != nil { return Decision{}, err } - created := replenish != nil && replenish.Created - if created { + if replenish != nil && replenish.Created { s.batchJob.Wake() } sug, err := fromRow(row.Suggestion, row.Dinner) @@ -378,7 +406,7 @@ func (s *Service) decide(ctx context.Context, id, decision, dinnerStatus, update return Decision{}, err } d := Decision{Suggestion: sug} - if created { + if replenish != nil { d.ReplenishJobID = &replenish.JobID } return d, nil diff --git a/internal/suggest/suggest_test.go b/internal/suggest/suggest_test.go index 157f1a5..ee850a3 100644 --- a/internal/suggest/suggest_test.go +++ b/internal/suggest/suggest_test.go @@ -329,7 +329,7 @@ func (e *env) deckNames(t *testing.T) []string { t.Fatal(err) } names := []string{} - for _, s := range deck { + for _, s := range deck.Suggestions { names = append(names, s.Name) } return names @@ -362,15 +362,15 @@ func TestBatchStoresGroundedSideQuests(t *testing.T) { if err != nil { t.Fatal(err) } - if len(deck) != 2 { - t.Fatalf("deck has %d suggestions, want 2", len(deck)) + if len(deck.Suggestions) != 2 { + t.Fatalf("deck has %d suggestions, want 2", len(deck.Suggestions)) } want := []struct{ name, reason, regular string }{ {"Chicken Shawarma Bowls", "Spiced chicken, like your Chicken Fajitas.", "Chicken Fajitas"}, {"Chili Verde", "Your Beef Chili gone green.", "Beef Chili"}, } for i, w := range want { - s := deck[i] + s := deck.Suggestions[i] if s.ID != res.SuggestionIDs[i] || s.Name != w.name || s.Reason != w.reason || s.Status != suggest.StatusPending { t.Errorf("deck[%d] = %+v, want %s pending with reason %q", i, s, w.name, w.reason) } @@ -452,6 +452,33 @@ func TestDeckIsOldestFirst(t *testing.T) { } } +func TestDeckReportsTheActiveBatch(t *testing.T) { + e := newEnv(t) // runner not started: the batch stays pending + e.seedDeck(t, "Pho") + deck, err := e.svc.Deck(t.Context()) + if err != nil { + t.Fatal(err) + } + if deck.ActiveBatch != nil { + t.Fatalf("active batch = %+v with none pending or running, want nil", deck.ActiveBatch) + } + b, err := e.svc.StartBatch(t.Context(), suggest.TriggerManual) + if err != nil { + t.Fatal(err) + } + deck, err = e.svc.Deck(t.Context()) + if err != nil { + t.Fatal(err) + } + if len(deck.Suggestions) != 1 { + t.Errorf("deck has %d suggestions, want 1", len(deck.Suggestions)) + } + want := suggest.Batch{ID: b.ID, JobID: b.JobID, Trigger: suggest.TriggerManual, Status: suggest.BatchPending} + if deck.ActiveBatch == nil || *deck.ActiveBatch != want { + t.Errorf("active batch = %+v, want %+v", deck.ActiveBatch, want) + } +} + func TestDeckLeavesOutDinnersNoLongerSuggested(t *testing.T) { e := newEnv(t) ids := e.seedDeck(t, "Pho", "Ramen") @@ -459,7 +486,7 @@ func TestDeckLeavesOutDinnersNoLongerSuggested(t *testing.T) { if err != nil { t.Fatal(err) } - if _, err := db.New(e.st.Writer).ArchiveDinner(t.Context(), db.ArchiveDinnerParams{ID: sug[0].DinnerID, UpdatedAt: store.Now().String()}); err != nil { + if _, err := db.New(e.st.Writer).ArchiveDinner(t.Context(), db.ArchiveDinnerParams{ID: sug.Suggestions[0].DinnerID, UpdatedAt: store.Now().String()}); err != nil { t.Fatal(err) } if got := e.deckNames(t); !slices.Equal(got, []string{"Ramen"}) { @@ -608,8 +635,10 @@ func TestReplenishIsSingleFlight(t *testing.T) { if err != nil { t.Fatal(err) } - if second.ReplenishJobID != nil { - t.Errorf("second decision enqueued replenish job %s while one is pending", *second.ReplenishJobID) + // The second decision joins the pending batch and reports its job, so a + // client that did not start it can follow it. + if second.ReplenishJobID == nil || *second.ReplenishJobID != *first.ReplenishJobID { + t.Errorf("second decision replenish job = %v, want the pending batch's job %s", second.ReplenishJobID, *first.ReplenishJobID) } manual, err := e.svc.StartBatch(t.Context(), suggest.TriggerManual) if err != nil { diff --git a/web/src/App.test.tsx b/web/src/App.test.tsx index fb9df89..60a5e3a 100644 --- a/web/src/App.test.tsx +++ b/web/src/App.test.tsx @@ -107,7 +107,7 @@ describe('App routes', () => { mockViewport(390) stubFetchByPath({ ...SIGNED_IN, - '/api/v1/suggestions/deck': () => jsonResponse(200, { suggestions: [] }), + '/api/v1/suggestions/deck': () => jsonResponse(200, { suggestions: [], active_batch: null }), '/api/v1/dinners?status=quest_log': () => jsonResponse(200, { dinners: [] }), }) renderAt('/discover') diff --git a/web/src/discover/DiscoverPage.module.css b/web/src/discover/DiscoverPage.module.css index ae35eb2..d3d53ef 100644 --- a/web/src/discover/DiscoverPage.module.css +++ b/web/src/discover/DiscoverPage.module.css @@ -224,6 +224,14 @@ border-radius: var(--radius-card); } +/* A blank card holds a depth a short deck has no card for, as the stack's + edge, in that depth's place and colour. */ +.blankCard { + position: absolute; + inset: 0; + border-radius: var(--radius-card); +} + .skeletonCard.depth0, .skeletonCard.depth1, .skeletonCard.depth2 { diff --git a/web/src/discover/DiscoverPage.test.tsx b/web/src/discover/DiscoverPage.test.tsx index f1f73f0..68b0b99 100644 --- a/web/src/discover/DiscoverPage.test.tsx +++ b/web/src/discover/DiscoverPage.test.tsx @@ -50,14 +50,15 @@ const ORZO = suggestionBody('s3', 'Lemon garlic shrimp orzo', { difficulty: 'har type Responder = () => Response | Promise // stubServer stands in for the API: GET of the deck answers the current -// server.deck and the Quest log the current server.questLog dinners; every +// server.deck and server.activeBatch, and the Quest log the current +// server.questLog dinners; every // other request is answered by responders, keyed "METHOD path". Any other // request fails the test. function stubServer(deck: unknown[], responders: Record = {}, questLog = 6) { - const server = { deck, questLog, fetchMock: undefined as unknown as Mock } + const server = { deck, questLog, activeBatch: null as { batch_id: string; job_id: string } | null, fetchMock: undefined as unknown as Mock } const all: Record = { 'GET /api/v1/me': () => jsonResponse(200, ME), - [`GET ${DECK}`]: () => jsonResponse(200, { suggestions: server.deck }), + [`GET ${DECK}`]: () => jsonResponse(200, { suggestions: server.deck, active_batch: server.activeBatch }), [`GET ${QUEST_LOG}`]: () => jsonResponse(200, { dinners: Array.from({ length: server.questLog }, (_, i) => questLogBody(`q${i}`, `Quest ${i}`)) }), ...responders, @@ -89,6 +90,13 @@ function jobPolls(fetchMock: Mock) { return fetchMock.mock.calls.filter(([input, init]) => (init?.method ?? 'GET') === 'GET' && String(input) === '/api/v1/jobs/j1').length } +// batchJob is the suggestion batch job j as GET /api/v1/jobs/{id} answers +// it, having stored ids. +function batchJob(j: string, status: string, ids: string[] = [], error: string | null = null) { + const result = status === 'failed' ? null : { batch_id: `b-${j}`, stage: status === 'succeeded' ? 'done' : 'grounding', suggestion_ids: ids, dropped: 0 } + return { id: j, kind: 'suggestion_batch', status, result, error } +} + // Every animation finishes as it starts unless a test stubs animate again. beforeEach(() => { mockViewport(390) @@ -370,7 +378,7 @@ describe('DiscoverPage', () => { it('shows a deck that fails to load with a retry', async () => { let fail = true - stubServer([BOWLS], { [`GET ${DECK}`]: () => (fail ? errorResponse(500, 'internal') : jsonResponse(200, { suggestions: [BOWLS] })) }) + stubServer([BOWLS], { [`GET ${DECK}`]: () => (fail ? errorResponse(500, 'internal') : jsonResponse(200, { suggestions: [BOWLS], active_batch: null })) }) await renderPage() const alert = await screen.findByRole('alert', {}, { timeout: 5000 }) expect(alert).toHaveTextContent("Couldn't load your side quests.") @@ -503,6 +511,160 @@ describe('DiscoverPage', () => { expect(screen.getByRole('heading', { name: 'All caught up' })).toBeInTheDocument() }) + describe('batch and deck depth', () => { + const PAELLA = suggestionBody('s4', 'Sheet pan paella') + const SOUP = suggestionBody('s5', 'Tortellini soup') + const CURRY = suggestionBody('s6', 'Green curry') + const declined = (quest: typeof BOWLS, replenish: string | null) => + jsonResponse(200, { suggestion: { ...quest, status: 'declined' }, replenish_job_id: replenish }) + // deckLayers is the deck's three depths, top first: its cards, then the + // blank or skeleton cards standing in for those it has none for. + const deckLayers = (container: HTMLElement) => [...container.querySelector('article')!.parentElement!.children] + + it('shows a replenish batch that fails while cards remain, beside the deck, with a retry that asks again', async () => { + const server = stubServer([BOWLS, TIKKA], { + 'POST /api/v1/suggestions/s1/decline': () => { + server.deck = [TIKKA] + return declined(BOWLS, 'j1') + }, + 'GET /api/v1/jobs/j1': () => jsonResponse(200, batchJob('j1', 'failed', [], 'model unavailable')), + 'POST /api/v1/suggestions/batches': () => jsonResponse(202, { batch_id: 'b-j2', job_id: 'j2', trigger: 'manual', status: 'pending' }), + 'GET /api/v1/jobs/j2': () => { + server.deck = [TIKKA, ORZO] + return jsonResponse(200, batchJob('j2', 'succeeded', ['s3'])) + }, + }) + await renderPage() + await screen.findByRole('article', { name: 'Korean beef bowls' }) + + fireEvent.click(screen.getByRole('button', { name: 'Pass' })) + const alert = await screen.findByRole('alert') + expect(alert).toHaveTextContent("Couldn't find new side quests. model unavailable") + // The deck stays in play beside it. + expect(screen.getByRole('article', { name: 'Crock pot chicken tikka masala' })).toBeInTheDocument() + await waitFor(() => expect(screen.getByRole('button', { name: 'Try it' })).toBeEnabled()) + + fireEvent.click(within(alert).getByRole('button', { name: 'Retry' })) + await waitFor(() => expect(screen.getAllByRole('article', { hidden: true })).toHaveLength(2)) + expect(screen.queryByRole('alert')).not.toBeInTheDocument() + expect(sent(server.fetchMock)).toEqual(['POST /api/v1/suggestions/s1/decline', 'POST /api/v1/suggestions/batches']) + }) + + it('shows a replenish batch whose job cannot be polled while cards remain, with a retry that polls it again', async () => { + let failPoll = true + const server = stubServer([BOWLS, TIKKA], { + 'POST /api/v1/suggestions/s1/decline': () => { + server.deck = [TIKKA] + return declined(BOWLS, 'j1') + }, + 'GET /api/v1/jobs/j1': () => { + if (failPoll) return errorResponse(500, 'internal') + server.deck = [TIKKA, ORZO] + return jsonResponse(200, batchJob('j1', 'succeeded', ['s3'])) + }, + }) + await renderPage() + await screen.findByRole('article', { name: 'Korean beef bowls' }) + + fireEvent.click(screen.getByRole('button', { name: 'Pass' })) + const alert = await screen.findByRole('alert') + expect(alert).toHaveTextContent("Couldn't check on new side quests.") + expect(screen.getByRole('article', { name: 'Crock pot chicken tikka masala' })).toBeInTheDocument() + + failPoll = false + const polls = jobPolls(server.fetchMock) + fireEvent.click(within(alert).getByRole('button', { name: 'Retry' })) + await waitFor(() => expect(jobPolls(server.fetchMock)).toBe(polls + 1), { timeout: 200 }) + await waitFor(() => expect(screen.getAllByRole('article', { hidden: true })).toHaveLength(2)) + expect(screen.queryByRole('alert')).not.toBeInTheDocument() + expect(sent(server.fetchMock)).toEqual(['POST /api/v1/suggestions/s1/decline']) + }) + + it('follows the batch the deck reports running when the page opens, with skeleton cards rather than "All caught up"', async () => { + const jobs = [batchJob('j1', 'running'), batchJob('j1', 'succeeded', ['s1'])] + const server = stubServer([], { + 'GET /api/v1/jobs/j1': () => { + const job = jobs.length > 1 ? jobs.shift()! : jobs[0]! + if (job.status === 'succeeded') { + server.deck = [BOWLS] + server.activeBatch = null + } + return jsonResponse(200, job) + }, + }) + server.activeBatch = { batch_id: 'b-j1', job_id: 'j1' } + await renderPage() + + expect(await screen.findByText('Finding new side quests…')).toBeInTheDocument() + expect(screen.getAllByTestId('skeleton-card')).toHaveLength(3) + expect(screen.queryByRole('heading', { name: 'All caught up' })).not.toBeInTheDocument() + + await screen.findByRole('article', { name: 'Korean beef bowls' }, { timeout: 3000 }) + expect(screen.queryByText('Finding new side quests…')).not.toBeInTheDocument() + expect(sent(server.fetchMock)).toEqual([]) + }) + + it('follows a batch another session started once a read of the deck reports it', async () => { + let done = false + const server = stubServer([BOWLS, TIKKA, ORZO, PAELLA, SOUP, CURRY], { + // Five are left: this decision calls for no batch of its own. + 'POST /api/v1/suggestions/s1/decline': () => { + server.deck = [TIKKA, ORZO, PAELLA, SOUP, CURRY] + return declined(BOWLS, null) + }, + 'GET /api/v1/jobs/j1': () => jsonResponse(200, batchJob('j1', done ? 'succeeded' : 'running')), + }) + await renderPage() + await screen.findByRole('article', { name: 'Korean beef bowls' }) + expect(jobPolls(server.fetchMock)).toBe(0) + + // Another session asks for more meanwhile. + server.activeBatch = { batch_id: 'b-j1', job_id: 'j1' } + fireEvent.click(screen.getByRole('button', { name: 'Pass' })) + expect(await screen.findByText('Finding new side quests…')).toBeInTheDocument() + await waitFor(() => expect(jobPolls(server.fetchMock)).toBeGreaterThan(0)) + + server.activeBatch = null + done = true + await waitFor(() => expect(screen.queryByText('Finding new side quests…')).not.toBeInTheDocument(), { timeout: 3000 }) + expect(sent(server.fetchMock)).toEqual(['POST /api/v1/suggestions/s1/decline']) + }) + + it.each([ + ['one card', [BOWLS], ['translateY(8px) scale(0.95)', 'translateY(16px) scale(0.9)']], + ['two cards', [BOWLS, TIKKA], ['translateY(16px) scale(0.9)']], + ])('keeps three deck depths with %s, filling the rest with blank cards while no batch runs', async (_, deck, blankTransforms) => { + stubServer(deck) + const { container } = await renderPage() + await screen.findByRole('article', { name: 'Korean beef bowls' }) + + const layers = deckLayers(container) + expect(layers).toHaveLength(3) + expect(layers.filter((layer) => layer.tagName === 'ARTICLE')).toHaveLength(deck.length) + const blanks = screen.getAllByTestId('blank-card') + expect(blanks).toEqual(layers.slice(deck.length)) + expect(blanks.map((blank) => declaredStyle(blank).transform)).toEqual(blankTransforms) + for (const blank of blanks) expect(blank).toHaveAttribute('aria-hidden', 'true') + // Nothing is coming: no skeleton cards and no "Finding" line. + expect(screen.queryByTestId('skeleton-card')).not.toBeInTheDocument() + expect(screen.queryByText('Finding new side quests…')).not.toBeInTheDocument() + }) + + it('fills a short deck with skeleton cards while a batch finds more Side quests', async () => { + const server = stubServer([BOWLS], { 'GET /api/v1/jobs/j1': () => jsonResponse(200, batchJob('j1', 'running')) }) + server.activeBatch = { batch_id: 'b-j1', job_id: 'j1' } + const { container } = await renderPage() + await screen.findByRole('article', { name: 'Korean beef bowls' }) + + expect(await screen.findByText('Finding new side quests…')).toBeInTheDocument() + const layers = deckLayers(container) + expect(layers).toHaveLength(3) + expect(screen.getAllByTestId('skeleton-card')).toEqual(layers.slice(1)) + expect(screen.queryByTestId('blank-card')).not.toBeInTheDocument() + expect(screen.getByRole('button', { name: 'Try it' })).toBeEnabled() + }) + }) + describe('Quest log sheet', () => { const PAD_THAI = questLogBody('q1', 'Pad thai', { photo_url: '/media/photos/p9-400.jpg', difficulty: 'medium', minutes_total: 35 }) const GUMBO = questLogBody('q2', 'Shrimp gumbo', { difficulty: 'hard', minutes_total: null }) @@ -749,6 +911,38 @@ describe('DiscoverPage', () => { expect(within(sheet).getByText('Your Quest log is empty — swipe right on a side quest to add one.')).toBeInTheDocument() expect(within(sheet).queryByRole('list')).not.toBeInTheDocument() }) + + it('leaves ← / → to the Quest log sheet and its confirmation while they are open', async () => { + const server = stubQuestLog([PAD_THAI, GUMBO], { + 'POST /api/v1/suggestions/s1/accept': () => { + server.deck = [] + return jsonResponse(200, { suggestion: { ...BOWLS, status: 'accepted' }, replenish_job_id: null }) + }, + }) + const { container } = await renderPage() + await screen.findByRole('article', { name: 'Korean beef bowls' }) + const pressArrows = (target: Element | Window) => { + for (const key of ['ArrowRight', 'ArrowLeft']) fireEvent.keyDown(target, { key }) + } + + const sheet = await openSheet(2) + pressArrows(sheet) + pressArrows(window) + fireEvent.click(within(sheet).getByRole('button', { name: 'Remove Pad thai' })) + const confirm = screen.getByRole('dialog', { name: 'Remove Pad thai?' }) + pressArrows(confirm) + pressArrows(window) + await act(async () => await new Promise((resolve) => setTimeout(resolve, 0))) + expect(sent(server.fetchMock)).toEqual([]) + expect(container.querySelector('article')).toHaveAccessibleName('Korean beef bowls') + + // Once both are closed, the keys are the deck's again. + fireEvent.click(within(confirm).getByRole('button', { name: 'Keep it' })) + fireEvent.keyDown(sheet, { key: 'Escape' }) + expect(screen.queryByRole('dialog')).not.toBeInTheDocument() + fireEvent.keyDown(window, { key: 'ArrowRight' }) + await waitFor(() => expect(sent(server.fetchMock)).toEqual(['POST /api/v1/suggestions/s1/accept'])) + }) }) describe('optimistic decisions', () => { @@ -860,7 +1054,7 @@ describe('DiscoverPage', () => { let late: ReturnType | null = null const server = stubServer([BOWLS, TIKKA, ORZO, PAELLA], { 'POST /api/v1/suggestions/s1/decline': () => decline.promise, - [`GET ${DECK}`]: () => late?.promise ?? jsonResponse(200, { suggestions: server.deck }), + [`GET ${DECK}`]: () => late?.promise ?? jsonResponse(200, { suggestions: server.deck, active_batch: null }), }) const queryClient = createQueryClient() await renderPage(queryClient) @@ -880,7 +1074,7 @@ describe('DiscoverPage', () => { server.deck = [TIKKA, ORZO, PAELLA] decline.resolve(declined(BOWLS)) await waitFor(() => expect(screen.getByRole('button', { name: 'Pass' })).toBeEnabled()) - await act(async () => stale.resolve(jsonResponse(200, { suggestions: [BOWLS, TIKKA, ORZO, PAELLA] }))) + await act(async () => stale.resolve(jsonResponse(200, { suggestions: [BOWLS, TIKKA, ORZO, PAELLA], active_batch: null }))) // The late answer is dropped, and the deck is read again after the save. await act(async () => await new Promise((resolve) => setTimeout(resolve, 0))) @@ -909,7 +1103,7 @@ describe('DiscoverPage', () => { await act(async () => await queryClient.refetchQueries({ queryKey: DECK_KEY })) await act(async () => await new Promise((resolve) => setTimeout(resolve, 0))) expect(deckReads(server.fetchMock)).toBe(2) - expect(queryClient.getQueryData<{ id: string }[]>(DECK_KEY)?.map((q) => q.id)).toEqual(['s1', 's2', 's3', 's4']) + expect(queryClient.getQueryData<{ suggestions: { id: string }[] }>(DECK_KEY)?.suggestions.map((q) => q.id)).toEqual(['s1', 's2', 's3', 's4']) expect(screen.getAllByRole('article', { hidden: true }).map((card) => card.querySelector('h2')!.textContent)).toEqual([ 'Crock pot chicken tikka masala', 'Lemon garlic shrimp orzo', @@ -1054,7 +1248,7 @@ describe('DiscoverPage', () => { let late: ReturnType | null = null const server = stubServer([BOWLS, TIKKA, ORZO, PAELLA], { 'POST /api/v1/suggestions/s1/accept': () => accept, - [`GET ${DECK}`]: () => late?.promise ?? jsonResponse(200, { suggestions: server.deck }), + [`GET ${DECK}`]: () => late?.promise ?? jsonResponse(200, { suggestions: server.deck, active_batch: null }), }) const queryClient = createQueryClient() await renderPage(queryClient) @@ -1071,7 +1265,7 @@ describe('DiscoverPage', () => { await act(async () => void queryClient.invalidateQueries({ queryKey: DECK_KEY })) late = null server.deck = [TIKKA, ORZO, PAELLA] - await act(async () => delayed.resolve(jsonResponse(200, { suggestions: server.deck }))) + await act(async () => delayed.resolve(jsonResponse(200, { suggestions: server.deck, active_batch: null }))) const reads = deckReads(server.fetchMock) await act(async () => lose(new TypeError('Failed to fetch'))) @@ -1088,7 +1282,7 @@ describe('DiscoverPage', () => { let read: ReturnType | null = null const server = stubServer([BOWLS, TIKKA, ORZO, PAELLA], { 'POST /api/v1/suggestions/s1/accept': () => accept.promise, - [`GET ${DECK}`]: () => read?.promise ?? jsonResponse(200, { suggestions: server.deck }), + [`GET ${DECK}`]: () => read?.promise ?? jsonResponse(200, { suggestions: server.deck, active_batch: null }), }) await renderPage() await screen.findByRole('article', { name: 'Korean beef bowls' }) @@ -1108,7 +1302,7 @@ describe('DiscoverPage', () => { expect(screen.queryByRole('alert')).not.toBeInTheDocument() expect(screen.getByRole('button', { name: 'Try it' })).toBeDisabled() - await act(async () => pending.resolve(jsonResponse(200, { suggestions: server.deck }))) + await act(async () => pending.resolve(jsonResponse(200, { suggestions: server.deck, active_batch: null }))) const alert = await screen.findByRole('alert') expect(screen.queryByRole('article', { name: 'Korean beef bowls' })).not.toBeInTheDocument() expect(topCard()).toHaveAccessibleName('Crock pot chicken tikka masala') @@ -1145,7 +1339,7 @@ describe('DiscoverPage', () => { read = deferred() return errorResponse(500, 'internal') }, - [`GET ${DECK}`]: () => read?.promise ?? jsonResponse(200, { suggestions: server.deck }), + [`GET ${DECK}`]: () => read?.promise ?? jsonResponse(200, { suggestions: server.deck, active_batch: null }), }) await renderPage() await screen.findByRole('article', { name: 'Korean beef bowls' }) @@ -1165,7 +1359,7 @@ describe('DiscoverPage', () => { 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 }))) + await act(async () => slow.resolve(jsonResponse(200, { suggestions: server.deck, active_batch: null }))) const alert = await screen.findByRole('alert') expect(alert).toHaveTextContent("Couldn't save that choice.") expect(topCard()).toHaveAccessibleName('Korean beef bowls') @@ -1199,7 +1393,7 @@ describe('DiscoverPage', () => { return errorResponse(500, 'internal') }, 'GET /api/v1/jobs/j1': () => poll?.promise ?? job([]), - [`GET ${DECK}`]: () => read?.promise ?? jsonResponse(200, { suggestions: server.deck }), + [`GET ${DECK}`]: () => read?.promise ?? jsonResponse(200, { suggestions: server.deck, active_batch: null }), }) const queryClient = createQueryClient() await renderPage(queryClient) @@ -1234,7 +1428,7 @@ describe('DiscoverPage', () => { // 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 act(async () => reconciling.resolve(jsonResponse(200, { suggestions: server.deck, active_batch: null }))) await waitFor(() => expect(screen.getByRole('button', { name: 'Try it' })).toBeEnabled()) const alert = screen.getByRole('alert') expect(alert).toHaveTextContent("Couldn't save that choice.") @@ -1244,8 +1438,8 @@ describe('DiscoverPage', () => { // 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'])) + await act(async () => after.resolve(jsonResponse(200, { suggestions: server.deck, active_batch: null }))) + await waitFor(() => expect(queryClient.getQueryData<{ suggestions: { id: string }[] }>(DECK_KEY)?.suggestions.map((q) => q.id)).toEqual(['s2', 's3', 's4', 's5'])) expect(topCard()).toHaveAccessibleName('Crock pot chicken tikka masala') // Retry sends it again. diff --git a/web/src/discover/DiscoverPage.tsx b/web/src/discover/DiscoverPage.tsx index 5e9e010..db17367 100644 --- a/web/src/discover/DiscoverPage.tsx +++ b/web/src/discover/DiscoverPage.tsx @@ -91,13 +91,24 @@ export function DiscoverPage() { // The deck shown: the deck's newest read, less the Side quests being // decided, which a read landing mid-decision may still have pending. const inFlight = useDeciding() - const cards = deck.data?.filter((quest) => !inFlight.has(quest.id)) + const cards = deck.data?.suggestions.filter((quest) => !inFlight.has(quest.id)) const questLog = useQuestLog() const decision = useDecide() const startBatch = useStartBatch() // The job of the batch last known to be finding Side quests: the one Get - // more now started or joined, or the replenish a decision started. + // more now started or joined, the replenish a decision started or joined, + // or the deck's active batch, which may have been started elsewhere or + // before the page opened. const [batchJobId, setBatchJobId] = useState(null) + // A read of the deck reporting a new active batch follows its job from + // that render on; one reporting none leaves the last job followed, so its + // end and any failure stay shown. + const activeJobId = deck.data?.activeBatch?.job_id ?? null + const [seenActiveJobId, setSeenActiveJobId] = useState(null) + if (activeJobId !== seenActiveJobId) { + setSeenActiveJobId(activeJobId) + if (activeJobId !== null) setBatchJobId(activeJobId) + } const job = useJob(BATCH_JOB_KIND, batchJobId) const top = cards?.[0] @@ -146,8 +157,8 @@ export function DiscoverPage() { return true }, []) // 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. + // useDecide put back on the deck is at rest; a saved one that started or + // joined a replenish batch follows its job. const send = useCallback( (variables: { quest: SideQuest; choice: Choice }) => { leaving.current = variables.quest.id @@ -293,6 +304,9 @@ export function DiscoverPage() { function onKeyDown(event: KeyboardEvent) { if (event.defaultPrevented || event.altKey || event.ctrlKey || event.metaKey || event.shiftKey) return if (isEditable(event.target)) return + // A modal over the page (the Quest log sheet or its confirmation) makes + // the deck inert: its keys are the modal's, not the deck's. + if (deckRef.current === null || deckRef.current.closest('[inert]') !== null) return if (event.key === 'ArrowLeft') decide('pass') else if (event.key === 'ArrowRight') decide('try') else return @@ -320,6 +334,17 @@ export function DiscoverPage() { const pending = decision.isError ? stillPending(deck, decision.variables.quest.id) : undefined const retryable = pending !== undefined && pending !== null + // What went wrong asking for or finding more Side quests, shown whatever + // the deck has: a replenish can fail while cards remain. While Get more now + // is asking again, the last batch's failure is not the news. + const batchFailure = startBatch.isPending ? null : startBatch.isError ? ( + Couldn't ask for more side quests. {startBatch.error.message} + ) : batchError === null ? null : jobRunning ? ( + Couldn't check on new side quests. {batchError.message} + ) : ( + Couldn't find new side quests. {batchError.message} + ) + let body if (deck.isError) { body = void deck.refetch()}>Couldn't load your side quests. {deck.error.message} @@ -334,32 +359,33 @@ export function DiscoverPage() { ))} {finding &&

Finding new side quests…

} - {jobRunning && batchError !== null && ( - Couldn't check on new side quests. {batchError.message} - )} ) } else if (top === undefined) { body = ( - <> - {startBatch.isError && ( - Couldn't ask for more side quests. {startBatch.error.message} - )} - {batchError !== null && ( - Couldn't find new side quests. {batchError.message} - )} - - New side quests arrive Sunday. - - + + New side quests arrive Sunday. + ) } else { + const shown = cards!.slice(0, DECK_DEPTH.length) body = ( <> + {/* The deck keeps all three depths: a short deck fills those it has + no card for with skeleton cards while a batch finds more Side + quests, and with blank cards, the stack's edge, while none is + coming. */}
- {cards!.slice(0, DECK_DEPTH.length).map((quest, depth) => ( + {shown.map((quest, depth) => ( ))} + {DECK_DEPTH.slice(shown.length).map((depth) => + finding ? ( + + ) : ( +
decide('pass')}> @@ -369,6 +395,7 @@ export function DiscoverPage() {
+ {finding &&

Finding new side quests…

} ) } @@ -386,6 +413,7 @@ export function DiscoverPage() { void questLog.refetch()}>Couldn't load the Quest log. {questLog.error.message} )} {body} + {batchFailure} {/* A failed decision is shown whatever the deck has come to since. */} {decision.isError && (retryable ? ( diff --git a/web/src/discover/api.ts b/web/src/discover/api.ts index 84a1917..0a927c7 100644 --- a/web/src/discover/api.ts +++ b/web/src/discover/api.ts @@ -29,10 +29,19 @@ export type SideQuest = { // Choice is the household's decision on a Side quest: Try it or Pass. export type Choice = 'try' | 'pass' -// Batch is a POST /api/v1/suggestions/batches answer: the batch and the job -// running it, which GET /api/v1/jobs/{id} reports on. +// Batch is a POST /api/v1/suggestions/batches answer, or the deck's active +// batch: the batch and the job running it, which GET /api/v1/jobs/{id} +// reports on. export type Batch = { batch_id: string; job_id: string } +// Deck is the deck as GET /api/v1/suggestions/deck reads it: the pending +// Side quests, oldest first, and the batch pending or running, if any. +export type Deck = { suggestions: SideQuest[]; activeBatch: Batch | null } + +function isBatch(body: unknown): body is Batch { + return isRecord(body) && typeof body.batch_id === 'string' && typeof body.job_id === 'string' +} + // BATCH_JOB_KIND is the kind of the job running a suggestion batch. export const BATCH_JOB_KIND = 'suggestion_batch' @@ -82,17 +91,21 @@ function parseSideQuest(where: string, body: unknown): SideQuest { } } -// useDeck is the pending Side quests, oldest first (GET -// /api/v1/suggestions/deck). +// useDeck is the pending Side quests, oldest first, and the batch pending +// or running (GET /api/v1/suggestions/deck). export function useDeck() { const { apiFetch } = useApi() return useQuery({ queryKey: DECK_KEY, - queryFn: async () => { + queryFn: async (): Promise => { const where = `GET ${DECK_PATH}` const body = await apiFetch(DECK_PATH) if (!isRecord(body) || !Array.isArray(body.suggestions)) throw new Error(`${where}: response has no suggestions list`) - return body.suggestions.map((suggestion) => parseSideQuest(where, suggestion)) + if (body.active_batch !== null && !isBatch(body.active_batch)) throw new Error(`${where}: response has no active_batch`) + return { + suggestions: body.suggestions.map((suggestion) => parseSideQuest(where, suggestion)), + activeBatch: body.active_batch === null ? null : { batch_id: body.active_batch.batch_id, job_id: body.active_batch.job_id }, + } }, }) } @@ -141,9 +154,7 @@ export function useStartBatch() { return useMutation({ mutationFn: async (): Promise => { const body = await apiFetch(BATCHES_PATH, { method: 'POST' }) - if (!isRecord(body) || typeof body.batch_id !== 'string' || typeof body.job_id !== 'string') { - throw new Error(`POST ${BATCHES_PATH}: response is not a batch`) - } + if (!isBatch(body)) throw new Error(`POST ${BATCHES_PATH}: response is not a batch`) return { batch_id: body.batch_id, job_id: body.job_id } }, }) @@ -153,9 +164,9 @@ export function useStartBatch() { // 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 { +export function stillPending(deck: { status: string; data: Deck | undefined } | undefined, id: string): SideQuest | null | undefined { if (deck?.status !== 'success') return undefined - return deck.data!.find((q) => q.id === id) ?? null + return deck.data!.suggestions.find((q) => q.id === id) ?? null } // useDeciding is the ids of the Side quests whose decisions are pending: @@ -173,7 +184,7 @@ export function useDeciding(): Set { // (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. // The answer must be that suggestion, decided that way; the mutation's data -// is the id of the replenish job the decision started, or null. +// is the id of the replenish job the decision started or joined, or null. // // The decision is applied at once: the Side quest leaves the deck shown // while it is pending (see useDeciding) and, on Try it, its dinner joins the @@ -222,7 +233,7 @@ export function useDecide() { // with the deck; the decision's own error is the one thrown. await queryClient .getQueryCache() - .build(queryClient, { queryKey: DECK_KEY }) + .build(queryClient, { queryKey: DECK_KEY }) .fetch(undefined, { cancelRefetch: true }) .catch(() => undefined) throw error @@ -252,9 +263,12 @@ export function useDecide() { } // A failed decision has read the deck already; any read since is newer. await queryClient.invalidateQueries({ queryKey: QUEST_LOG_KEY }) - const pending = stillPending(queryClient.getQueryState(DECK_KEY), quest.id) + 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) => ({ + ...deck!, + suggestions: [pending, ...deck!.suggestions.filter((q) => q.id !== pending.id)], + })) }, }) }