From af58caac94cc645548a4b35568e1e17d78b5d59e Mon Sep 17 00:00:00 2001 From: dev_agent Date: Wed, 7 Oct 2026 05:04:20 -0400 Subject: [PATCH 1/4] 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)], + })) }, }) } -- 2.55.0 From 71060515bd724e32e4dc2029ed687ed886f7953a Mon Sep 17 00:00:00 2001 From: dev_agent Date: Wed, 7 Oct 2026 05:13:48 -0400 Subject: [PATCH 2/4] Fail orphaned suggestion batches before reporting the deck's active batch Deck returned a pending or running batch's job id without checking that the job was still queued or running, so after a restart failed the job the page polled a dead job as the active batch. Deck and startBatch now share activeBatch, which runs FailOrphanedSuggestionBatches before reading the active batch; Deck therefore uses a write transaction. Co-Authored-By: Claude Opus 5.5 --- internal/suggest/suggest.go | 97 ++++++++++++++++---------------- internal/suggest/suggest_test.go | 37 ++++++++++++ 2 files changed, 87 insertions(+), 47 deletions(-) diff --git a/internal/suggest/suggest.go b/internal/suggest/suggest.go index 81f398d..33d5491 100644 --- a/internal/suggest/suggest.go +++ b/internal/suggest/suggest.go @@ -171,34 +171,47 @@ func (s *Service) StartBatch(ctx context.Context, trigger string) (Batch, error) return b, nil } -// startBatch does StartBatch's work in tx: the batch and the job that runs -// it are committed together, so a batch is never seen without its job. A -// batch still pending or running whose job is no longer queued or running -// is failed first, so a batch whose job ended without recording its end -// (the process stopped, or the write failed) never blocks a later one. The -// caller wakes the job runner once tx has committed and Created is true. -func (s *Service) startBatch(ctx context.Context, tx *sql.Tx, trigger string) (Batch, error) { - q := db.New(tx) +// activeBatch returns the batch pending or running in q's transaction, or +// nil when there is none. A batch still pending or running whose job is no +// longer queued or running is failed first, so a batch whose job ended +// without recording its end (the process stopped, or the write failed) is +// never reported as active and never blocks a later one. +func (s *Service) activeBatch(ctx context.Context, q *db.Queries) (*Batch, error) { n, err := q.FailOrphanedSuggestionBatches(ctx, db.FailOrphanedSuggestionBatchesParams{ UpdatedAt: store.Now().String(), Kind: KindSuggestionBatch, }) if err != nil { - return Batch{}, fmt.Errorf("suggest: fail orphaned batches: %w", err) + return nil, fmt.Errorf("suggest: fail orphaned batches: %w", err) } if n > 0 { s.logger.WarnContext(ctx, "failed suggestion batches whose job ended", "count", n) } cur, err := q.GetActiveSuggestionBatch(ctx) - if err == nil { - jobID, err := q.GetSuggestionBatchJobID(ctx, db.GetSuggestionBatchJobIDParams{Kind: KindSuggestionBatch, BatchID: cur.ID}) - if err != nil { - return Batch{}, fmt.Errorf("suggest: read job of batch %s: %w", cur.ID, err) - } - return Batch{ID: cur.ID, JobID: jobID, Trigger: cur.Trigger, Status: cur.Status}, nil + if errors.Is(err, sql.ErrNoRows) { + return nil, nil } - if !errors.Is(err, sql.ErrNoRows) { - return Batch{}, fmt.Errorf("suggest: read active batch: %w", err) + if err != nil { + return nil, fmt.Errorf("suggest: read active batch: %w", err) + } + jobID, err := q.GetSuggestionBatchJobID(ctx, db.GetSuggestionBatchJobIDParams{Kind: KindSuggestionBatch, BatchID: cur.ID}) + if err != nil { + return nil, fmt.Errorf("suggest: read job of batch %s: %w", cur.ID, err) + } + return &Batch{ID: cur.ID, JobID: jobID, Trigger: cur.Trigger, Status: cur.Status}, nil +} + +// startBatch does StartBatch's work in tx: the batch and the job that runs +// it are committed together, so a batch is never seen without its job. The +// caller wakes the job runner once tx has committed and Created is true. +func (s *Service) startBatch(ctx context.Context, tx *sql.Tx, trigger string) (Batch, error) { + q := db.New(tx) + cur, err := s.activeBatch(ctx, q) + if err != nil { + return Batch{}, err + } + if cur != nil { + return *cur, nil } now := store.Now().String() row, err := q.CreateSuggestionBatch(ctx, db.CreateSuggestionBatchParams{ @@ -269,40 +282,30 @@ type Deck struct { 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 Deck{}, fmt.Errorf("suggest: begin deck read: %w", err) - } - 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 { - deck.Suggestions[i], err = fromRow(row.Suggestion, row.Dinner) +// Deck reads the deck and its active batch in one transaction, so both come +// from the same snapshot. It is a write transaction because reading the +// active batch first fails any batch whose job has ended (see activeBatch). +func (s *Service) Deck(ctx context.Context) (Deck, error) { + var deck Deck + err := s.st.WithTx(ctx, func(tx *sql.Tx) error { + q := db.New(tx) + rows, err := q.ListDeck(ctx) if err != nil { - return Deck{}, err + return fmt.Errorf("suggest: list deck: %w", err) } - } - cur, err := q.GetActiveSuggestionBatch(ctx) - if errors.Is(err, sql.ErrNoRows) { - return deck, nil - } + deck.Suggestions = make([]Suggestion, len(rows)) + for i, row := range rows { + deck.Suggestions[i], err = fromRow(row.Suggestion, row.Dinner) + if err != nil { + return err + } + } + deck.ActiveBatch, err = s.activeBatch(ctx, q) + return err + }) if err != nil { - return Deck{}, fmt.Errorf("suggest: read active batch: %w", err) + return Deck{}, 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 } diff --git a/internal/suggest/suggest_test.go b/internal/suggest/suggest_test.go index ee850a3..683f046 100644 --- a/internal/suggest/suggest_test.go +++ b/internal/suggest/suggest_test.go @@ -2,6 +2,7 @@ package suggest_test import ( "context" + "database/sql" "encoding/json" "errors" "fmt" @@ -479,6 +480,42 @@ func TestDeckReportsTheActiveBatch(t *testing.T) { } } +func TestDeckFailsBatchInterruptedByRestart(t *testing.T) { + e := newEnv(t) + e.seedDeck(t, "Pho") + b, err := e.svc.StartBatch(t.Context(), suggest.TriggerManual) + if err != nil { + t.Fatal(err) + } + // A previous process claimed the job and started the batch, then + // stopped before recording its end. + now := store.Now().String() + if _, err := db.New(e.st.Writer).ClaimNextJob(t.Context(), sql.NullString{String: now, Valid: true}); err != nil { + t.Fatal(err) + } + if n, err := db.New(e.st.Writer).StartSuggestionBatch(t.Context(), db.StartSuggestionBatchParams{UpdatedAt: now, ID: b.ID}); err != nil || n != 1 { + t.Fatalf("start batch: %d rows, %v", n, err) + } + e.start(t) // fails the running job as interrupted + if job := e.waitJob(t, b.JobID); job.Status != jobs.StatusFailed { + t.Fatalf("job status = %s, want failed", job.Status) + } + + deck, err := e.svc.Deck(t.Context()) + if err != nil { + t.Fatal(err) + } + if deck.ActiveBatch != nil { + t.Errorf("active batch = %+v after its job failed, want nil", deck.ActiveBatch) + } + if len(deck.Suggestions) != 1 { + t.Errorf("deck has %d suggestions, want 1", len(deck.Suggestions)) + } + if got := e.batchStatus(t, b.ID); got != suggest.BatchFailed { + t.Errorf("interrupted batch status = %s, want failed", got) + } +} + func TestDeckLeavesOutDinnersNoLongerSuggested(t *testing.T) { e := newEnv(t) ids := e.seedDeck(t, "Pho", "Ramen") -- 2.55.0 From b611d1bbcec00d2cf7a89f31541d6493746031f3 Mon Sep 17 00:00:00 2001 From: dev_agent Date: Wed, 7 Oct 2026 05:29:10 -0400 Subject: [PATCH 3/4] Report the latest failed suggestion batch on the deck The deck reported only a pending or running batch, so a Discover page opened after a batch failed (or was interrupted by a restart) had no job to follow and showed "All caught up" or the remaining cards with no error. GET /api/v1/suggestions/deck now also returns failed_batch: the newest batch when it failed, read in the same snapshot after orphaned batches are failed, so at most one of active_batch and failed_batch is set and a later batch supersedes the failure. The page follows the failed batch's job like the active one, so its failure shows with the existing Retry, which asks for a new batch. Co-Authored-By: Claude Opus 5.5 --- docs/ARCHITECTURE.md | 11 ++-- internal/db/querier.go | 3 + internal/db/suggestions.sql.go | 22 ++++++++ internal/server/suggestions_test.go | 3 + internal/suggest/suggest.go | 42 ++++++++++++-- internal/suggest/suggest_test.go | 43 ++++++++++++++ migrations/queries/suggestions.sql | 7 +++ web/src/App.test.tsx | 2 +- web/src/discover/DiscoverPage.test.tsx | 78 +++++++++++++++++++++----- web/src/discover/DiscoverPage.tsx | 29 +++++----- web/src/discover/api.ts | 17 ++++-- 11 files changed, 213 insertions(+), 44 deletions(-) diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 007ae36..b566ef0 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -311,10 +311,13 @@ automatic** (D-021). `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}], - 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 + active_batch, failed_batch}`: the `pending` suggestions whose dinner is + still `suggested`, oldest first; the batch `pending` or `running` + (`{batch_id, job_id, trigger, status}`, or null); and the newest batch + when it `failed` (same shape, or null; a later batch, done or under + way, supersedes the failure), read in one snapshot, so a client that + did not start the batch can follow its job and learns of a failure it + was not following. At most one of the two is set. `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, diff --git a/internal/db/querier.go b/internal/db/querier.go index c9754c9..da18d8e 100644 --- a/internal/db/querier.go +++ b/internal/db/querier.go @@ -78,6 +78,9 @@ type Querier interface { // Returns sql.ErrNoRows until the row exists. GetIntegrationsGoogle(ctx context.Context) (IntegrationsGoogle, error) GetJob(ctx context.Context, id string) (Job, error) + // The newest batch, whatever its status. Returns sql.ErrNoRows when there + // is none. + GetLatestSuggestionBatch(ctx context.Context) (SuggestionBatch, error) GetPhoto(ctx context.Context, id string) (Photo, error) // Finds stored image bytes by content hash. The schema does not make sha256 // unique, so the earliest-created (lowest id) match is returned. diff --git a/internal/db/suggestions.sql.go b/internal/db/suggestions.sql.go index 4106a2a..3829c5a 100644 --- a/internal/db/suggestions.sql.go +++ b/internal/db/suggestions.sql.go @@ -187,6 +187,28 @@ func (q *Queries) GetActiveSuggestionBatch(ctx context.Context) (SuggestionBatch return i, err } +const getLatestSuggestionBatch = `-- name: GetLatestSuggestionBatch :one +SELECT id, "trigger", status, ai_run_id, created_at, updated_at FROM suggestion_batches +ORDER BY id DESC +LIMIT 1 +` + +// The newest batch, whatever its status. Returns sql.ErrNoRows when there +// is none. +func (q *Queries) GetLatestSuggestionBatch(ctx context.Context) (SuggestionBatch, error) { + row := q.db.QueryRowContext(ctx, getLatestSuggestionBatch) + var i SuggestionBatch + err := row.Scan( + &i.ID, + &i.Trigger, + &i.Status, + &i.AiRunID, + &i.CreatedAt, + &i.UpdatedAt, + ) + return i, err +} + const getSuggestion = `-- name: GetSuggestion :one SELECT suggestions.id, suggestions.dinner_id, suggestions.batch_id, suggestions.name, suggestions.reason, suggestions.cited_regular_ids, suggestions.status, suggestions.decided_at, suggestions.created_at, suggestions.updated_at, dinners.id, dinners.name, dinners.normalized_name, dinners.status, dinners.origin, dinners.method, dinners.minutes_total, dinners.difficulty, dinners.cuisine, dinners.is_favorite, dinners.recipe_id, dinners.photo_id, dinners.grounding_status, dinners.updated_by, dinners.created_at, dinners.updated_at, dinners.recipe_url, dinners.grounding_error, dinners.grounding_revision FROM suggestions diff --git a/internal/server/suggestions_test.go b/internal/server/suggestions_test.go index f81dbe2..e50448b 100644 --- a/internal/server/suggestions_test.go +++ b/internal/server/suggestions_test.go @@ -174,6 +174,9 @@ func TestSuggestionDeckActiveBatch(t *testing.T) { if active, ok := body["active_batch"]; !ok || active != nil { t.Fatalf("deck = %v, want active_batch null", body) } + if failed, ok := body["failed_batch"]; !ok || failed != nil { + t.Fatalf("deck = %v, want failed_batch null", body) + } rec, started := callDinners(t, h, http.MethodPost, suggestionsPath+"/batches", "") if rec.Code != http.StatusAccepted { diff --git a/internal/suggest/suggest.go b/internal/suggest/suggest.go index 33d5491..287ad59 100644 --- a/internal/suggest/suggest.go +++ b/internal/suggest/suggest.go @@ -201,6 +201,28 @@ func (s *Service) activeBatch(ctx context.Context, q *db.Queries) (*Batch, error return &Batch{ID: cur.ID, JobID: jobID, Trigger: cur.Trigger, Status: cur.Status}, nil } +// failedBatch returns the newest batch in q's transaction when it failed, +// or nil when it did not or there is none: a later batch, done or under +// way, supersedes an earlier failure. Run after activeBatch, so a batch +// whose job ended without recording its end reads as failed. +func failedBatch(ctx context.Context, q *db.Queries) (*Batch, error) { + cur, err := q.GetLatestSuggestionBatch(ctx) + if errors.Is(err, sql.ErrNoRows) { + return nil, nil + } + if err != nil { + return nil, fmt.Errorf("suggest: read latest batch: %w", err) + } + if cur.Status != BatchFailed { + return nil, nil + } + jobID, err := q.GetSuggestionBatchJobID(ctx, db.GetSuggestionBatchJobIDParams{Kind: KindSuggestionBatch, BatchID: cur.ID}) + if err != nil { + return nil, fmt.Errorf("suggest: read job of batch %s: %w", cur.ID, err) + } + return &Batch{ID: cur.ID, JobID: jobID, Trigger: cur.Trigger, Status: cur.Status}, nil +} + // startBatch does StartBatch's work in tx: the batch and the job that runs // it are committed together, so a batch is never seen without its job. The // caller wakes the job runner once tx has committed and Created is true. @@ -275,16 +297,22 @@ type Suggestion struct { } // 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. +// still suggested, oldest first; the batch pending or running, if any, so +// a client can follow the job producing more; and the newest batch when it +// failed, so a client that was not following its job still learns of the +// failure. At most one of ActiveBatch and FailedBatch is set: a batch is +// only started when none is pending or running, so the active batch is +// always the newest. type Deck struct { Suggestions []Suggestion `json:"suggestions"` ActiveBatch *Batch `json:"active_batch"` + FailedBatch *Batch `json:"failed_batch"` } -// Deck reads the deck and its active batch in one transaction, so both come -// from the same snapshot. It is a write transaction because reading the -// active batch first fails any batch whose job has ended (see activeBatch). +// Deck reads the deck and its active and failed batches in one +// transaction, so all come from the same snapshot. It is a write +// transaction because reading the active batch first fails any batch whose +// job has ended (see activeBatch), which is then the failed batch. func (s *Service) Deck(ctx context.Context) (Deck, error) { var deck Deck err := s.st.WithTx(ctx, func(tx *sql.Tx) error { @@ -301,6 +329,10 @@ func (s *Service) Deck(ctx context.Context) (Deck, error) { } } deck.ActiveBatch, err = s.activeBatch(ctx, q) + if err != nil || deck.ActiveBatch != nil { + return err + } + deck.FailedBatch, err = failedBatch(ctx, q) return err }) if err != nil { diff --git a/internal/suggest/suggest_test.go b/internal/suggest/suggest_test.go index 683f046..915b789 100644 --- a/internal/suggest/suggest_test.go +++ b/internal/suggest/suggest_test.go @@ -463,6 +463,9 @@ func TestDeckReportsTheActiveBatch(t *testing.T) { if deck.ActiveBatch != nil { t.Fatalf("active batch = %+v with none pending or running, want nil", deck.ActiveBatch) } + if deck.FailedBatch != nil { + t.Fatalf("failed batch = %+v with none failed, want nil", deck.FailedBatch) + } b, err := e.svc.StartBatch(t.Context(), suggest.TriggerManual) if err != nil { t.Fatal(err) @@ -478,6 +481,42 @@ func TestDeckReportsTheActiveBatch(t *testing.T) { if deck.ActiveBatch == nil || *deck.ActiveBatch != want { t.Errorf("active batch = %+v, want %+v", deck.ActiveBatch, want) } + if deck.FailedBatch != nil { + t.Errorf("failed batch = %+v with a batch pending, want nil", deck.FailedBatch) + } +} + +func TestDeckReportsTheLatestFailedBatch(t *testing.T) { + e := newEnv(t) + e.start(t) + e.side.QueueGenerate(llm.GenerateResult{Err: errors.New("provider down")}) + b, err := e.svc.StartBatch(t.Context(), suggest.TriggerManual) + if err != nil { + t.Fatal(err) + } + if job := e.waitJob(t, b.JobID); job.Status != jobs.StatusFailed { + t.Fatalf("job status = %s, want failed", job.Status) + } + deck, err := e.svc.Deck(t.Context()) + if err != nil { + t.Fatal(err) + } + want := suggest.Batch{ID: b.ID, JobID: b.JobID, Trigger: suggest.TriggerManual, Status: suggest.BatchFailed} + if deck.ActiveBatch != nil || deck.FailedBatch == nil || *deck.FailedBatch != want { + t.Fatalf("active batch = %+v, failed batch = %+v; want no active batch and failed %+v", deck.ActiveBatch, deck.FailedBatch, want) + } + + // A later batch that succeeds supersedes the failure. + e.recipe(t, "Chili Verde") + e.reply(sideQuest("Chili Verde", "Your Beef Chili gone green.", "Beef Chili")) + e.runBatch(t) + deck, err = e.svc.Deck(t.Context()) + if err != nil { + t.Fatal(err) + } + if deck.ActiveBatch != nil || deck.FailedBatch != nil { + t.Errorf("active batch = %+v, failed batch = %+v after a batch succeeded, want neither", deck.ActiveBatch, deck.FailedBatch) + } } func TestDeckFailsBatchInterruptedByRestart(t *testing.T) { @@ -508,6 +547,10 @@ func TestDeckFailsBatchInterruptedByRestart(t *testing.T) { if deck.ActiveBatch != nil { t.Errorf("active batch = %+v after its job failed, want nil", deck.ActiveBatch) } + want := suggest.Batch{ID: b.ID, JobID: b.JobID, Trigger: suggest.TriggerManual, Status: suggest.BatchFailed} + if deck.FailedBatch == nil || *deck.FailedBatch != want { + t.Errorf("failed batch = %+v after its job was interrupted, want %+v", deck.FailedBatch, want) + } if len(deck.Suggestions) != 1 { t.Errorf("deck has %d suggestions, want 1", len(deck.Suggestions)) } diff --git a/migrations/queries/suggestions.sql b/migrations/queries/suggestions.sql index 3a42470..2d81299 100644 --- a/migrations/queries/suggestions.sql +++ b/migrations/queries/suggestions.sql @@ -11,6 +11,13 @@ WHERE status IN ('pending', 'running') ORDER BY id LIMIT 1; +-- name: GetLatestSuggestionBatch :one +-- The newest batch, whatever its status. Returns sql.ErrNoRows when there +-- is none. +SELECT * FROM suggestion_batches +ORDER BY id DESC +LIMIT 1; + -- name: GetSuggestionBatchJobID :one -- The id of the job that runs batch batch_id. SELECT id FROM jobs diff --git a/web/src/App.test.tsx b/web/src/App.test.tsx index 60a5e3a..1d2a4d9 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: [], active_batch: null }), + '/api/v1/suggestions/deck': () => jsonResponse(200, { suggestions: [], active_batch: null, failed_batch: null }), '/api/v1/dinners?status=quest_log': () => jsonResponse(200, { dinners: [] }), }) renderAt('/discover') diff --git a/web/src/discover/DiscoverPage.test.tsx b/web/src/discover/DiscoverPage.test.tsx index 68b0b99..e723e3a 100644 --- a/web/src/discover/DiscoverPage.test.tsx +++ b/web/src/discover/DiscoverPage.test.tsx @@ -50,15 +50,21 @@ 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 server.activeBatch, and the Quest log the current +// server.deck, server.activeBatch and server.failedBatch, 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, activeBatch: null as { batch_id: string; job_id: string } | null, fetchMock: undefined as unknown as Mock } + const server = { + deck, + questLog, + activeBatch: null as { batch_id: string; job_id: string } | null, + failedBatch: 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, active_batch: server.activeBatch }), + [`GET ${DECK}`]: () => jsonResponse(200, { suggestions: server.deck, active_batch: server.activeBatch, failed_batch: server.failedBatch }), [`GET ${QUEST_LOG}`]: () => jsonResponse(200, { dinners: Array.from({ length: server.questLog }, (_, i) => questLogBody(`q${i}`, `Quest ${i}`)) }), ...responders, @@ -378,7 +384,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], active_batch: null })) }) + stubServer([BOWLS], { [`GET ${DECK}`]: () => (fail ? errorResponse(500, 'internal') : jsonResponse(200, { suggestions: [BOWLS], active_batch: null, failed_batch: null })) }) await renderPage() const alert = await screen.findByRole('alert', {}, { timeout: 5000 }) expect(alert).toHaveTextContent("Couldn't load your side quests.") @@ -580,6 +586,48 @@ describe('DiscoverPage', () => { expect(sent(server.fetchMock)).toEqual(['POST /api/v1/suggestions/s1/decline']) }) + it.each([ + ['the AI failed', 'an empty deck', 'ai_failed', []], + ['the AI failed', 'cards left', 'ai_failed', [BOWLS, TIKKA]], + ['a restart interrupted it', 'an empty deck', 'interrupted by restart', []], + ['a restart interrupted it', 'cards left', 'interrupted by restart', [BOWLS, TIKKA]], + ])('shows the failed batch the deck reports when the page opens after %s, with %s, and a retry that asks again', async (_, __, reason, deck) => { + const server = stubServer(deck, { + 'GET /api/v1/jobs/j1': () => jsonResponse(200, batchJob('j1', 'failed', [], reason)), + 'POST /api/v1/suggestions/batches': () => { + server.failedBatch = null + server.activeBatch = { batch_id: 'b-j2', job_id: 'j2' } + return jsonResponse(202, { batch_id: 'b-j2', job_id: 'j2', trigger: 'manual', status: 'pending' }) + }, + 'GET /api/v1/jobs/j2': () => { + server.activeBatch = null + server.deck = [...deck, ORZO] + return jsonResponse(200, batchJob('j2', 'succeeded', ['s3'])) + }, + }) + // The batch failed while the page was closed. + server.failedBatch = { batch_id: 'b-j1', job_id: 'j1' } + await renderPage() + + const alert = await screen.findByRole('alert') + expect(alert).toHaveTextContent(`Couldn't find new side quests. ${reason}`) + if (deck.length === 0) { + expect(screen.getByRole('heading', { name: 'All caught up' })).toBeInTheDocument() + } else { + expect(screen.getByRole('article', { name: 'Korean beef bowls' })).toBeInTheDocument() + expect(screen.getAllByTestId('blank-card')).toHaveLength(1) + } + // A failed batch is not finding anything. + expect(screen.queryByText('Finding new side quests…')).not.toBeInTheDocument() + expect(screen.queryByTestId('skeleton-card')).not.toBeInTheDocument() + + fireEvent.click(within(alert).getByRole('button', { name: 'Retry' })) + // The new batch's Side quest joins the deck. + await waitFor(() => expect(screen.getAllByRole('article', { hidden: true })).toHaveLength(deck.length + 1), { timeout: 3000 }) + await waitFor(() => expect(screen.queryByRole('alert')).not.toBeInTheDocument()) + expect(sent(server.fetchMock)).toEqual(['POST /api/v1/suggestions/batches']) + }) + 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([], { @@ -1054,7 +1102,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, active_batch: null }), + [`GET ${DECK}`]: () => late?.promise ?? jsonResponse(200, { suggestions: server.deck, active_batch: null, failed_batch: null }), }) const queryClient = createQueryClient() await renderPage(queryClient) @@ -1074,7 +1122,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], active_batch: null }))) + await act(async () => stale.resolve(jsonResponse(200, { suggestions: [BOWLS, TIKKA, ORZO, PAELLA], active_batch: null, failed_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))) @@ -1248,7 +1296,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, active_batch: null }), + [`GET ${DECK}`]: () => late?.promise ?? jsonResponse(200, { suggestions: server.deck, active_batch: null, failed_batch: null }), }) const queryClient = createQueryClient() await renderPage(queryClient) @@ -1265,7 +1313,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, active_batch: null }))) + await act(async () => delayed.resolve(jsonResponse(200, { suggestions: server.deck, active_batch: null, failed_batch: null }))) const reads = deckReads(server.fetchMock) await act(async () => lose(new TypeError('Failed to fetch'))) @@ -1282,7 +1330,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, active_batch: null }), + [`GET ${DECK}`]: () => read?.promise ?? jsonResponse(200, { suggestions: server.deck, active_batch: null, failed_batch: null }), }) await renderPage() await screen.findByRole('article', { name: 'Korean beef bowls' }) @@ -1302,7 +1350,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, active_batch: null }))) + await act(async () => pending.resolve(jsonResponse(200, { suggestions: server.deck, active_batch: null, failed_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') @@ -1339,7 +1387,7 @@ describe('DiscoverPage', () => { read = deferred() return errorResponse(500, 'internal') }, - [`GET ${DECK}`]: () => read?.promise ?? jsonResponse(200, { suggestions: server.deck, active_batch: null }), + [`GET ${DECK}`]: () => read?.promise ?? jsonResponse(200, { suggestions: server.deck, active_batch: null, failed_batch: null }), }) await renderPage() await screen.findByRole('article', { name: 'Korean beef bowls' }) @@ -1359,7 +1407,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, active_batch: null }))) + await act(async () => slow.resolve(jsonResponse(200, { suggestions: server.deck, active_batch: null, failed_batch: null }))) const alert = await screen.findByRole('alert') expect(alert).toHaveTextContent("Couldn't save that choice.") expect(topCard()).toHaveAccessibleName('Korean beef bowls') @@ -1393,7 +1441,7 @@ describe('DiscoverPage', () => { return errorResponse(500, 'internal') }, 'GET /api/v1/jobs/j1': () => poll?.promise ?? job([]), - [`GET ${DECK}`]: () => read?.promise ?? jsonResponse(200, { suggestions: server.deck, active_batch: null }), + [`GET ${DECK}`]: () => read?.promise ?? jsonResponse(200, { suggestions: server.deck, active_batch: null, failed_batch: null }), }) const queryClient = createQueryClient() await renderPage(queryClient) @@ -1428,7 +1476,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, active_batch: null }))) + await act(async () => reconciling.resolve(jsonResponse(200, { suggestions: server.deck, active_batch: null, failed_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.") @@ -1438,7 +1486,7 @@ 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, active_batch: null }))) + await act(async () => after.resolve(jsonResponse(200, { suggestions: server.deck, active_batch: null, failed_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') diff --git a/web/src/discover/DiscoverPage.tsx b/web/src/discover/DiscoverPage.tsx index db17367..c4fabfb 100644 --- a/web/src/discover/DiscoverPage.tsx +++ b/web/src/discover/DiscoverPage.tsx @@ -95,25 +95,28 @@ export function DiscoverPage() { 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, the replenish a decision started or joined, - // or the deck's active batch, which may have been started elsewhere or - // before the page opened. + // The job of the batch last known to be finding Side quests, or to have + // failed to: the one Get more now started or joined, the replenish a + // decision started or joined, or the deck's active or failed 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) + // A read of the deck reporting a new active or failed batch follows its + // job from that render on; one reporting neither leaves the last job + // followed, so its end and any failure stay shown. + const failedJobId = deck.data?.failedBatch?.job_id ?? null + const deckJobId = deck.data?.activeBatch?.job_id ?? failedJobId + const [seenDeckJobId, setSeenDeckJobId] = useState(null) + if (deckJobId !== seenDeckJobId) { + setSeenDeckJobId(deckJobId) + if (deckJobId !== null) setBatchJobId(deckJobId) } const job = useJob(BATCH_JOB_KIND, batchJobId) const top = cards?.[0] const [busy, setBusy] = useState(false) - const jobRunning = batchJobId !== null && job.status !== 'succeeded' && job.status !== 'failed' + // The deck's failed batch is not finding anything, even before its job's + // first poll says why it failed. + const jobRunning = batchJobId !== null && batchJobId !== failedJobId && job.status !== 'succeeded' && job.status !== 'failed' const finding = startBatch.isPending || jobRunning const { mutate: mutateStartBatch } = startBatch const getMore = useCallback(() => mutateStartBatch(undefined, { onSuccess: (b) => setBatchJobId(b.job_id) }), [mutateStartBatch]) diff --git a/web/src/discover/api.ts b/web/src/discover/api.ts index 0a927c7..ea8d070 100644 --- a/web/src/discover/api.ts +++ b/web/src/discover/api.ts @@ -30,13 +30,15 @@ export type SideQuest = { export type Choice = 'try' | 'pass' // 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. +// or failed 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 } +// Side quests, oldest first; the batch pending or running, if any; and the +// newest batch when it failed, which a later batch supersedes. At most one +// of the two batches is set. +export type Deck = { suggestions: SideQuest[]; activeBatch: Batch | null; failedBatch: Batch | null } function isBatch(body: unknown): body is Batch { return isRecord(body) && typeof body.batch_id === 'string' && typeof body.job_id === 'string' @@ -91,8 +93,9 @@ function parseSideQuest(where: string, body: unknown): SideQuest { } } -// useDeck is the pending Side quests, oldest first, and the batch pending -// or running (GET /api/v1/suggestions/deck). +// useDeck is the pending Side quests, oldest first, the batch pending or +// running and the newest batch when it failed (GET +// /api/v1/suggestions/deck). export function useDeck() { const { apiFetch } = useApi() return useQuery({ @@ -102,9 +105,11 @@ export function useDeck() { const body = await apiFetch(DECK_PATH) if (!isRecord(body) || !Array.isArray(body.suggestions)) throw new Error(`${where}: response has no suggestions list`) if (body.active_batch !== null && !isBatch(body.active_batch)) throw new Error(`${where}: response has no active_batch`) + if (body.failed_batch !== null && !isBatch(body.failed_batch)) throw new Error(`${where}: response has no failed_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 }, + failedBatch: body.failed_batch === null ? null : { batch_id: body.failed_batch.batch_id, job_id: body.failed_batch.job_id }, } }, }) -- 2.55.0 From c21d03dec68013200d62912aedd1cfdbcc8eb3e8 Mon Sep 17 00:00:00 2001 From: dev_agent Date: Wed, 7 Oct 2026 05:41:06 -0400 Subject: [PATCH 4/4] Drop a followed batch failure once the deck reports it superseded MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After Discover showed failed batch j1, another session could finish a newer batch. The deck then reported neither an active nor a failed batch, as the server suppresses a failure a later batch supersedes, but the page kept following j1 and its error stayed shown (or, before j1's first poll, "Finding new side quests…" did, and the error arrived later). Each new read of the deck is now reconciled: one reporting neither batch stops following a job known to have failed, by its poll or by the deck's previous read. A followed job that succeeded is still left followed, so its end is read as before. The replenish-failure test's stub now reports its failed batch on the deck, as the server does. Co-Authored-By: Claude Opus 5.5 --- web/src/discover/DiscoverPage.test.tsx | 44 ++++++++++++++++++++++++-- web/src/discover/DiscoverPage.tsx | 17 ++++++---- 2 files changed, 52 insertions(+), 9 deletions(-) diff --git a/web/src/discover/DiscoverPage.test.tsx b/web/src/discover/DiscoverPage.test.tsx index e723e3a..dec24cd 100644 --- a/web/src/discover/DiscoverPage.test.tsx +++ b/web/src/discover/DiscoverPage.test.tsx @@ -531,11 +531,21 @@ describe('DiscoverPage', () => { const server = stubServer([BOWLS, TIKKA], { 'POST /api/v1/suggestions/s1/decline': () => { server.deck = [TIKKA] + server.activeBatch = { batch_id: 'b-j1', job_id: 'j1' } 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/j1': () => { + server.activeBatch = null + server.failedBatch = { batch_id: 'b-j1', job_id: 'j1' } + return jsonResponse(200, batchJob('j1', 'failed', [], 'model unavailable')) + }, + 'POST /api/v1/suggestions/batches': () => { + server.failedBatch = null + server.activeBatch = { batch_id: 'b-j2', job_id: 'j2' } + return jsonResponse(202, { batch_id: 'b-j2', job_id: 'j2', trigger: 'manual', status: 'pending' }) + }, 'GET /api/v1/jobs/j2': () => { + server.activeBatch = null server.deck = [TIKKA, ORZO] return jsonResponse(200, batchJob('j2', 'succeeded', ['s3'])) }, @@ -628,6 +638,36 @@ describe('DiscoverPage', () => { expect(sent(server.fetchMock)).toEqual(['POST /api/v1/suggestions/batches']) }) + it.each([ + ['once its job has been polled', false], + ['before its job has been polled', true], + ])('drops a failed batch %s once a read of the deck reports a later batch another session finished', async (_, slowPoll) => { + const poll = deferred() + const server = stubServer([BOWLS, TIKKA], { + 'GET /api/v1/jobs/j1': () => (slowPoll ? poll.promise : jsonResponse(200, batchJob('j1', 'failed', [], 'ai_failed'))), + }) + server.failedBatch = { batch_id: 'b-j1', job_id: 'j1' } + const queryClient = createQueryClient() + await renderPage(queryClient) + await screen.findByRole('article', { name: 'Korean beef bowls' }) + if (slowPoll) await waitFor(() => expect(jobPolls(server.fetchMock)).toBe(1)) + else expect(await screen.findByRole('alert')).toHaveTextContent("Couldn't find new side quests. ai_failed") + + // Another session's batch j2 succeeds, superseding j1's failure. + server.failedBatch = null + server.deck = [BOWLS, TIKKA, ORZO] + const reads = deckReads(server.fetchMock) + await act(() => queryClient.invalidateQueries({ queryKey: DECK_KEY })) + await waitFor(() => expect(deckReads(server.fetchMock)).toBe(reads + 1)) + await waitFor(() => expect(screen.getAllByRole('article', { hidden: true })).toHaveLength(3)) + // A poll of j1 still out when the deck moved on does not bring it back. + await act(async () => poll.resolve(jsonResponse(200, batchJob('j1', 'failed', [], 'ai_failed')))) + + expect(screen.queryByRole('alert')).not.toBeInTheDocument() + expect(screen.queryByText('Finding new side quests…')).not.toBeInTheDocument() + expect(sent(server.fetchMock)).toEqual([]) + }) + 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([], { diff --git a/web/src/discover/DiscoverPage.tsx b/web/src/discover/DiscoverPage.tsx index c4fabfb..fd77c50 100644 --- a/web/src/discover/DiscoverPage.tsx +++ b/web/src/discover/DiscoverPage.tsx @@ -100,17 +100,20 @@ export function DiscoverPage() { // decision started or joined, or the deck's active or failed batch, which // may have been started elsewhere or before the page opened. const [batchJobId, setBatchJobId] = useState(null) + const job = useJob(BATCH_JOB_KIND, batchJobId) // A read of the deck reporting a new active or failed batch follows its - // job from that render on; one reporting neither leaves the last job - // followed, so its end and any failure stay shown. + // job from that render on. One reporting neither leaves the last job + // followed, so its end stays shown — unless that job is known to have + // failed, by its poll or the deck's last read: the deck reports the newest + // batch's failure, so a later batch has superseded it, and it goes. const failedJobId = deck.data?.failedBatch?.job_id ?? null const deckJobId = deck.data?.activeBatch?.job_id ?? failedJobId - const [seenDeckJobId, setSeenDeckJobId] = useState(null) - if (deckJobId !== seenDeckJobId) { - setSeenDeckJobId(deckJobId) - if (deckJobId !== null) setBatchJobId(deckJobId) + const [seenRead, setSeenRead] = useState({ at: 0, jobId: null as string | null, failedJobId: null as string | null }) + if (deck.dataUpdatedAt !== seenRead.at) { + setSeenRead({ at: deck.dataUpdatedAt, jobId: deckJobId, failedJobId }) + if (deckJobId !== null && deckJobId !== seenRead.jobId) setBatchJobId(deckJobId) + else if (deckJobId === null && batchJobId !== null && (job.status === 'failed' || batchJobId === seenRead.failedJobId)) setBatchJobId(null) } - const job = useJob(BATCH_JOB_KIND, batchJobId) const top = cards?.[0] const [busy, setBusy] = useState(false) -- 2.55.0