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..dec24cd 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.") @@ -525,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'])) }, @@ -580,6 +596,78 @@ 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.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([], { @@ -1054,7 +1142,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 +1162,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 +1336,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 +1353,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 +1370,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 +1390,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 +1427,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 +1447,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 +1481,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 +1516,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 +1526,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..fd77c50 100644 --- a/web/src/discover/DiscoverPage.tsx +++ b/web/src/discover/DiscoverPage.tsx @@ -95,25 +95,31 @@ 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) - } 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 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 [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 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 }, } }, })