Fix offline onboarding lockout and stale candidates after rename
MQ-98-04: the household settings read that OnboardingGate makes before every page now runs with networkMode 'always', like /me. After the browser emits an offline event it fails into the saved pages instead of pausing on the loading screen. A test emits a real offline event before /me fails. MQ-98-05: renaming a dinner now clears its candidate offers and makes a running candidate search stale, and an offer is bound to the normalized name it was searched for, so a candidate offered before a rename cannot ground the renamed dinner. Regression tests cover a pending dinner renamed after candidates were offered, and a rename during a running search. Maestro task: 4fd737b2-fdcb-48ad-858a-ecdd95595e0c Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
@@ -309,7 +309,8 @@ func (s *Service) List(ctx context.Context, status string) ([]Dinner, error) {
|
||||
// clears its recipe_url, raises its grounding_revision and enqueues a fresh
|
||||
// KindGroundDinner job that grounds the new name: a job queued or running
|
||||
// for the old name or link carries the old revision, so it can no longer
|
||||
// write the dinner, and one running is stopped.
|
||||
// write the dinner, and one running is stopped. Renaming any dinner drops
|
||||
// the candidates offered for its old name: none of them grounds it.
|
||||
func (s *Service) Update(ctx context.Context, id string, p Patch, updatedBy string) (Dinner, error) {
|
||||
if updatedBy == "" {
|
||||
return Dinner{}, errors.New("dinners: update: no member to record as updated_by")
|
||||
@@ -320,7 +321,7 @@ func (s *Service) Update(ctx context.Context, id string, p Patch, updatedBy stri
|
||||
}
|
||||
|
||||
var row db.Dinner
|
||||
var regrounding bool
|
||||
var renamed, regrounding bool
|
||||
err = s.st.WithTx(ctx, func(tx *sql.Tx) error {
|
||||
q := db.New(tx)
|
||||
cur, err := q.GetDinner(ctx, id)
|
||||
@@ -374,12 +375,16 @@ func (s *Service) Update(ctx context.Context, id string, p Patch, updatedBy stri
|
||||
if err != nil {
|
||||
return fmt.Errorf("dinners: update %s: %w", id, err)
|
||||
}
|
||||
renamed = row.NormalizedName != cur.NormalizedName
|
||||
regrounding = row.GroundingRevision != cur.GroundingRevision
|
||||
return nil
|
||||
})
|
||||
if err != nil {
|
||||
return Dinner{}, err
|
||||
}
|
||||
if renamed {
|
||||
s.clearOffers(row.ID)
|
||||
}
|
||||
if regrounding {
|
||||
s.stopGroundings(row.ID, row.GroundingRevision)
|
||||
jobID, err := s.enqueueGrounding(ctx, row.ID, row.GroundingRevision)
|
||||
|
||||
@@ -304,10 +304,12 @@ var offerLifetime = 30 * time.Minute
|
||||
var now = time.Now
|
||||
|
||||
// offerSet is the candidates last offered for a dinner: the page URL of
|
||||
// each candidate id. They ground the dinner until deadline; expiry drops
|
||||
// the set from offers once it has passed.
|
||||
// each candidate id, found by searching the dinner's name, normalized as
|
||||
// name. They ground the dinner until deadline, and only while it still has
|
||||
// that name; expiry drops the set from offers once deadline has passed.
|
||||
type offerSet struct {
|
||||
urls map[string]string
|
||||
name string
|
||||
deadline time.Time
|
||||
expiry *time.Timer
|
||||
}
|
||||
@@ -322,11 +324,11 @@ type offerSearch struct {
|
||||
// recipe pages to ground it in, best first; the list may be empty. They
|
||||
// replace the dinner's earlier candidates: one not offered again no longer
|
||||
// grounds it. They ground it for offerLifetime, and not at all once it is
|
||||
// grounded by hand or archived, even while this search runs. A search that
|
||||
// fails wraps recipes.ErrSearchFailed.
|
||||
// grounded by hand, renamed or archived, even while this search runs. A
|
||||
// search that fails wraps recipes.ErrSearchFailed.
|
||||
func (s *Service) Candidates(ctx context.Context, id string) ([]Candidate, error) {
|
||||
// Tracked before the dinner is read: grounding or archiving it after
|
||||
// the read makes this search stale.
|
||||
// Tracked before the dinner is read: grounding, renaming or archiving
|
||||
// it after the read makes this search stale.
|
||||
keep, done := s.trackOffers(id)
|
||||
defer done()
|
||||
row, err := s.get(ctx, id)
|
||||
@@ -349,15 +351,16 @@ func (s *Service) Candidates(ctx context.Context, id string) ([]Candidate, error
|
||||
}
|
||||
}
|
||||
if row.Status != StatusArchived {
|
||||
keep(urls)
|
||||
keep(urls, row.NormalizedName)
|
||||
}
|
||||
return out, nil
|
||||
}
|
||||
|
||||
// trackOffers registers a Candidates search for dinner id. It returns keep,
|
||||
// which stores urls as the dinner's offers for offerLifetime unless
|
||||
// clearOffers has run for it since, and done, to call when the search ends.
|
||||
func (s *Service) trackOffers(id string) (keep func(urls map[string]string), done func()) {
|
||||
// which stores urls, found for the normalized name, as the dinner's offers
|
||||
// for offerLifetime unless clearOffers has run for it since, and done, to
|
||||
// call when the search ends.
|
||||
func (s *Service) trackOffers(id string) (keep func(urls map[string]string, name string), done func()) {
|
||||
search := &offerSearch{}
|
||||
s.offersMu.Lock()
|
||||
if s.offerSearches[id] == nil {
|
||||
@@ -365,14 +368,14 @@ func (s *Service) trackOffers(id string) (keep func(urls map[string]string), don
|
||||
}
|
||||
s.offerSearches[id][search] = struct{}{}
|
||||
s.offersMu.Unlock()
|
||||
keep = func(urls map[string]string) {
|
||||
keep = func(urls map[string]string, name string) {
|
||||
s.offersMu.Lock()
|
||||
defer s.offersMu.Unlock()
|
||||
if search.stale {
|
||||
return
|
||||
}
|
||||
s.dropOffers(id)
|
||||
set := &offerSet{urls: urls, deadline: now().Add(offerLifetime)}
|
||||
set := &offerSet{urls: urls, name: name, deadline: now().Add(offerLifetime)}
|
||||
// expiry waits for offersMu, so it finds set stored even when it
|
||||
// fires at once, and drops set only, never offers stored later.
|
||||
set.expiry = time.AfterFunc(offerLifetime, func() {
|
||||
@@ -397,7 +400,7 @@ func (s *Service) trackOffers(id string) (keep func(urls map[string]string), don
|
||||
|
||||
// clearOffers drops dinner id's offers and makes every Candidates search
|
||||
// running for it stale, so no candidate offered before grounds it. Call
|
||||
// it once the dinner is grounded by hand or archived.
|
||||
// it once the dinner is grounded by hand, renamed or archived.
|
||||
func (s *Service) clearOffers(id string) {
|
||||
s.offersMu.Lock()
|
||||
defer s.offersMu.Unlock()
|
||||
@@ -425,15 +428,15 @@ func (s *Service) candidateID(id, rawURL string) string {
|
||||
return base64.RawURLEncoding.EncodeToString(mac.Sum(nil)[:16])
|
||||
}
|
||||
|
||||
// candidateURL is the page URL of candidate cid last offered for dinner id.
|
||||
// candidateURL is the page URL of candidate cid last offered for dinner d.
|
||||
// A cid not among the dinner's live offers, or whose page is not a valid
|
||||
// recipe link, is a *ValidationError. Offers past their deadline are not
|
||||
// live, even before their expiry drops them.
|
||||
func (s *Service) candidateURL(id, cid string) (string, error) {
|
||||
// recipe link, is a *ValidationError. Offers past their deadline, or found
|
||||
// for a name d no longer has, are not live, even before they are dropped.
|
||||
func (s *Service) candidateURL(d db.Dinner, cid string) (string, error) {
|
||||
var raw string
|
||||
var ok bool
|
||||
s.offersMu.Lock()
|
||||
if set := s.offers[id]; set != nil && now().Before(set.deadline) {
|
||||
if set := s.offers[d.ID]; set != nil && set.name == d.NormalizedName && now().Before(set.deadline) {
|
||||
raw, ok = set.urls[cid]
|
||||
}
|
||||
s.offersMu.Unlock()
|
||||
@@ -499,7 +502,7 @@ func (s *Service) Ground(ctx context.Context, id string, f Fix, updatedBy string
|
||||
return Dinner{}, ErrArchived
|
||||
}
|
||||
if f.CandidateID != nil {
|
||||
if link, err = s.candidateURL(cur.ID, *f.CandidateID); err != nil {
|
||||
if link, err = s.candidateURL(cur, *f.CandidateID); err != nil {
|
||||
return Dinner{}, err
|
||||
}
|
||||
}
|
||||
|
||||
@@ -804,8 +804,8 @@ func wantStaleCandidate(t *testing.T, e *env, id, cid string) {
|
||||
}
|
||||
|
||||
// TestGroundingOrArchivingClearsCandidates: once a dinner is grounded by
|
||||
// hand — by candidate, link or manual recipe — or archived, the service
|
||||
// holds none of its candidates' URLs and none of them grounds it.
|
||||
// hand — by candidate, link or manual recipe — renamed or archived, the
|
||||
// service holds none of its candidates' URLs and none of them grounds it.
|
||||
func TestGroundingOrArchivingClearsCandidates(t *testing.T) {
|
||||
t.Parallel()
|
||||
tests := []struct {
|
||||
@@ -834,6 +834,11 @@ func TestGroundingOrArchivingClearsCandidates(t *testing.T) {
|
||||
t.Fatal(err)
|
||||
}
|
||||
}, false},
|
||||
{"rename", func(t *testing.T, e *env, id, _ string) {
|
||||
if _, err := e.svc.Update(t.Context(), id, dinners.Patch{Name: ptr("Beef stew")}, member); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
}, false},
|
||||
{"archive", func(t *testing.T, e *env, id, _ string) {
|
||||
if _, err := e.svc.Archive(t.Context(), id, member); err != nil {
|
||||
t.Fatal(err)
|
||||
@@ -860,9 +865,56 @@ func TestGroundingOrArchivingClearsCandidates(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// TestRenamePendingRefusesOldCandidates: candidates offered for a pending
|
||||
// dinner's old name no longer ground it once it is renamed — Ground refuses
|
||||
// them and leaves the old recipe off it — while candidates for the new name
|
||||
// do.
|
||||
func TestRenamePendingRefusesOldCandidates(t *testing.T) {
|
||||
t.Parallel()
|
||||
// Unstarted, so the dinner stays pending with its jobs queued.
|
||||
e := newUnstartedEnv(t)
|
||||
e.search.Results["Beef stew"] = []recipes.Candidate{{URL: stew}}
|
||||
c, err := e.svc.CreateRegular(t.Context(), dinners.NewRegular{Name: ptr("Chicken curry")}, member)
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
old := candidateID(t, e, c.ID, "Chicken Curry")
|
||||
|
||||
d, err := e.svc.Update(t.Context(), c.ID, dinners.Patch{Name: ptr("Beef stew")}, member)
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if d.GroundingStatus != dinners.GroundingPending {
|
||||
t.Fatalf("renamed = %+v, want still pending", d)
|
||||
}
|
||||
if e.svc.HasOffers(c.ID) {
|
||||
t.Errorf("service still holds the old name's candidates of dinner %s", c.ID)
|
||||
}
|
||||
wantStaleCandidate(t, e, c.ID, old)
|
||||
if row := e.dinner(t, c.ID); row.RecipeID.Valid || row.GroundingStatus != dinners.GroundingPending {
|
||||
t.Fatalf("dinner = %+v, want pending with no recipe", row)
|
||||
}
|
||||
|
||||
e.model.QueueGenerate(recipestest.NormalizedReply("oven", 150))
|
||||
g, err := e.svc.Ground(t.Context(), c.ID, dinners.Fix{CandidateID: ptr(candidateID(t, e, c.ID, "Beef Stew"))}, member)
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if g.GroundingStatus != dinners.GroundingGrounded || g.RecipeID == nil {
|
||||
t.Fatalf("dinner = %+v, want grounded for the new name", g)
|
||||
}
|
||||
rec, err := db.New(e.st.Reader).GetRecipe(t.Context(), *g.RecipeID)
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if rec.SourceUrl != stew {
|
||||
t.Errorf("recipe source = %s, want %s", rec.SourceUrl, stew)
|
||||
}
|
||||
}
|
||||
|
||||
// TestCandidatesRunningWhenCleared: a candidate search still running when
|
||||
// its dinner is grounded by hand or archived stores none of the candidates
|
||||
// it returns, so none of them grounds the dinner.
|
||||
// its dinner is grounded by hand, renamed or archived stores none of the
|
||||
// candidates it returns, so none of them grounds the dinner.
|
||||
func TestCandidatesRunningWhenCleared(t *testing.T) {
|
||||
t.Parallel()
|
||||
tests := []struct {
|
||||
@@ -877,6 +929,11 @@ func TestCandidatesRunningWhenCleared(t *testing.T) {
|
||||
t.Fatal(err)
|
||||
}
|
||||
}, false},
|
||||
{"rename", func(t *testing.T, e *env, id string) {
|
||||
if _, err := e.svc.Update(t.Context(), id, dinners.Patch{Name: ptr("Beef stew")}, member); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
}, false},
|
||||
{"archive", func(t *testing.T, e *env, id string) {
|
||||
if _, err := e.svc.Archive(t.Context(), id, member); err != nil {
|
||||
t.Fatal(err)
|
||||
|
||||
+33
-1
@@ -1,4 +1,4 @@
|
||||
import { act, render, screen } from '@testing-library/react'
|
||||
import { act, cleanup, render, screen } from '@testing-library/react'
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
import { createApiClient, createVerifiedSession } from './api/index.ts'
|
||||
import { createNetworkStatus } from './pwa/networkStatus.ts'
|
||||
@@ -236,6 +236,13 @@ describe('App offline bootstrap', () => {
|
||||
vi.spyOn(navigator, 'onLine', 'get').mockImplementation(() => onLine)
|
||||
})
|
||||
|
||||
// A test that emits an offline event leaves TanStack Query's online
|
||||
// manager, shared by every test, offline until an online event.
|
||||
afterEach(() => {
|
||||
cleanup()
|
||||
window.dispatchEvent(new Event('online'))
|
||||
})
|
||||
|
||||
it.each([
|
||||
['/plan', 'Plan'],
|
||||
['/dinner/01J9ZK', 'Dinner'],
|
||||
@@ -250,6 +257,31 @@ describe('App offline bootstrap', () => {
|
||||
expect(screen.queryByRole('alert')).not.toBeInTheDocument()
|
||||
})
|
||||
|
||||
it('opens the saved page when the browser has emitted an offline event', async () => {
|
||||
await visitOnline()
|
||||
// /me is still loading, with the app mounted, when the browser emits
|
||||
// its offline event; only then does /me fail.
|
||||
let failMe!: () => void
|
||||
vi.stubGlobal(
|
||||
'fetch',
|
||||
vi.fn<typeof fetch>().mockImplementationOnce(
|
||||
() => new Promise((_, reject) => (failMe = () => reject(new TypeError('Failed to fetch')))),
|
||||
).mockRejectedValue(new TypeError('Failed to fetch')),
|
||||
)
|
||||
renderAt('/plan')
|
||||
await vi.waitFor(() => expect(failMe).toBeDefined())
|
||||
onLine = false
|
||||
act(() => {
|
||||
window.dispatchEvent(new Event('offline'))
|
||||
})
|
||||
act(() => failMe())
|
||||
|
||||
expect(await screen.findByRole('heading', { level: 1, name: 'Plan' })).toBeInTheDocument()
|
||||
expect(screen.queryByRole('main', { name: 'Loading MealQuest' })).not.toBeInTheDocument()
|
||||
expect(screen.getByRole('status')).toHaveTextContent('Offline — showing saved data')
|
||||
expect(screen.queryByRole('alert')).not.toBeInTheDocument()
|
||||
})
|
||||
|
||||
it('opens the saved pages when the server is unreachable but the browser reports online', async () => {
|
||||
await visitOnline()
|
||||
vi.stubGlobal('fetch', vi.fn<typeof fetch>().mockRejectedValue(new TypeError('Failed to fetch')))
|
||||
|
||||
@@ -19,7 +19,8 @@ function inOnboarding(pathname: string): boolean {
|
||||
// The service worker never caches the settings, so offline on the saved
|
||||
// pages (see AuthBootstrap) they cannot be read: the saved pages open as they
|
||||
// are, since onboarding needs the server anyway, and the settings are read
|
||||
// again when the browser is back online.
|
||||
// again when the browser is back online. The read runs even while the
|
||||
// browser reports offline, so that case fails fast instead of waiting.
|
||||
export function OnboardingGate() {
|
||||
const api = useApi()
|
||||
const { pathname } = useLocation()
|
||||
|
||||
@@ -367,12 +367,16 @@ function parseHouseholdSettings(where: string, body: unknown): HouseholdSettings
|
||||
|
||||
export const HOUSEHOLD_SETTINGS_KEY = ['settings', 'household'] as const
|
||||
|
||||
// useHouseholdSettings runs even while the browser reports offline, like
|
||||
// /me: OnboardingGate reads it before every page, so offline it must fail
|
||||
// into the saved pages instead of waiting for the network.
|
||||
export function useHouseholdSettings() {
|
||||
const { apiFetch } = useApi()
|
||||
return useQuery({
|
||||
queryKey: HOUSEHOLD_SETTINGS_KEY,
|
||||
queryFn: async () =>
|
||||
parseHouseholdSettings(`GET ${HOUSEHOLD_SETTINGS_PATH}`, await apiFetch<unknown>(HOUSEHOLD_SETTINGS_PATH)),
|
||||
networkMode: 'always',
|
||||
})
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user