From c15c8ec284c085086e158bf1f99fe9a1b6628b49 Mon Sep 17 00:00:00 2001 From: dev_agent Date: Tue, 6 Oct 2026 20:35:19 -0400 Subject: [PATCH 1/5] Keep recipe_url out of dinner responses (Feature 12 review F1) A pasted recipe link may carry a personal token in its query, and the dinners API returned it from create, list, get, patch and ground, where other household clients and the PWA's cache of dinner reads could keep it. Drop recipe_url from the public Dinner shape so it stays in the database for grounding and dedupe only. TestCreateDinnerByURL and TestCreateDinnerByName now assert the field is absent; TestDinnerResponsesHideRecipeURL creates a Regular from a token-bearing link and checks neither the create nor the list response holds the link, host or token while the stored link remains. Co-Authored-By: Claude Opus 5.5 --- docs/ARCHITECTURE.md | 3 +- internal/dinners/dinners.go | 6 +-- internal/dinners/grounding_test.go | 16 ++++--- internal/server/dinners_test.go | 43 ++++++++++++++++++- .../settings/RegularsSettingsPage.test.tsx | 2 +- web/src/test/dinners.ts | 2 - 6 files changed, 58 insertions(+), 14 deletions(-) diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 161102d..9a33eec 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -180,7 +180,8 @@ automatic** (D-021). each skill run, so a saved change applies to the next run. - Dinners (`dinners.Service`). `POST /api/v1/dinners` adds a Regular from exactly one of `{name}` or `{recipe_url}` (an https:// link, kept in - `recipe_url` for grounding) with status and origin `regular` and + `recipe_url` for grounding and never returned in a dinner response, as + it may carry a personal token) with status and origin `regular` and grounding_status `pending`, answering 201 with the dinner. A link's dinner is named from its last path segment (separators become spaces, an extension and all-digit words are dropped: `…/12345/beef-stew/` is diff --git a/internal/dinners/dinners.go b/internal/dinners/dinners.go index 605d675..7a39832 100644 --- a/internal/dinners/dinners.go +++ b/internal/dinners/dinners.go @@ -55,7 +55,9 @@ var listable = []string{StatusRegular, StatusQuestLog} // ErrNotFound is returned for an unknown dinner id. var ErrNotFound = errors.New("dinner not found") -// Dinner is a dinner as the API shows it. +// Dinner is a dinner as the API shows it. It has no recipe_url: a pasted +// link may carry a personal token, so it stays in the database for +// grounding and never leaves the server. type Dinner struct { ID string `json:"id"` Name string `json:"name"` @@ -67,7 +69,6 @@ type Dinner struct { Cuisine *string `json:"cuisine"` IsFavorite bool `json:"is_favorite"` RecipeID *string `json:"recipe_id"` - RecipeURL *string `json:"recipe_url"` PhotoURL *string `json:"photo_url"` GroundingStatus string `json:"grounding_status"` // GroundingError is why grounding failed, one of the Reason codes; @@ -565,7 +566,6 @@ func fromRow(r db.Dinner) Dinner { Cuisine: nullString(r.Cuisine), IsFavorite: r.IsFavorite == 1, RecipeID: nullString(r.RecipeID), - RecipeURL: nullString(r.RecipeUrl), GroundingStatus: r.GroundingStatus, GroundingError: nullString(r.GroundingError), UpdatedBy: nullString(r.UpdatedBy), diff --git a/internal/dinners/grounding_test.go b/internal/dinners/grounding_test.go index 2578083..c6ada37 100644 --- a/internal/dinners/grounding_test.go +++ b/internal/dinners/grounding_test.go @@ -383,8 +383,11 @@ func TestRenameLinkWhilePendingGroundsNewName(t *testing.T) { if err != nil { t.Fatal(err) } - if d.GroundingStatus != dinners.GroundingPending || d.RecipeURL != nil { - t.Fatalf("renamed = %+v, want still pending with no recipe_url", d) + if d.GroundingStatus != dinners.GroundingPending { + t.Fatalf("renamed = %+v, want still pending", d) + } + if row, err := db.New(e.st.Reader).GetDinner(t.Context(), c.ID); err != nil || row.RecipeUrl.Valid { + t.Fatalf("renamed row = %+v (err %v), want no recipe_url", row, err) } e.start(t) @@ -749,12 +752,15 @@ func TestFixReplacesPendingLink(t *testing.T) { t.Fatal(err) } e.model.QueueGenerate(recipestest.NormalizedReply("stovetop", 25)) - d, err := e.svc.Ground(t.Context(), c.ID, tt.fix, member) + if _, err := e.svc.Ground(t.Context(), c.ID, tt.fix, member); err != nil { + t.Fatal(err) + } + row, err := db.New(e.st.Reader).GetDinner(t.Context(), c.ID) if err != nil { t.Fatal(err) } - if d.RecipeURL != nil { - t.Errorf("recipe_url = %q after the fix, want the unusable link dropped", *d.RecipeURL) + if row.RecipeUrl.Valid { + t.Errorf("recipe_url = %q after the fix, want the unusable link dropped", row.RecipeUrl.String) } if _, err := e.svc.CreateRegular(t.Context(), dinners.NewRegular{RecipeURL: ptr(lost)}, member); err != nil { diff --git a/internal/server/dinners_test.go b/internal/server/dinners_test.go index 402b90d..a8285e2 100644 --- a/internal/server/dinners_test.go +++ b/internal/server/dinners_test.go @@ -159,7 +159,6 @@ func TestCreateDinnerByName(t *testing.T) { "origin": "regular", "grounding_status": "pending", "is_favorite": false, - "recipe_url": nil, "photo_url": nil, "method": nil, "minutes_total": nil, @@ -170,6 +169,9 @@ func TestCreateDinnerByName(t *testing.T) { t.Errorf("%s = %#v (present %v), want %#v", k, got, ok, v) } } + if got, ok := body["recipe_url"]; ok { + t.Errorf("recipe_url = %#v, want no recipe_url in a dinner response", got) + } var normalized, updatedBy string if err := st.Reader.QueryRow(`SELECT normalized_name, updated_by FROM dinners WHERE id = ?`, body["id"]).Scan(&normalized, &updatedBy); err != nil { t.Fatal(err) @@ -190,7 +192,6 @@ func TestCreateDinnerByURL(t *testing.T) { } for k, v := range map[string]any{ "name": "Beef and broccoli stir fry", - "recipe_url": link, "status": "regular", "origin": "regular", "grounding_status": "pending", @@ -200,6 +201,10 @@ func TestCreateDinnerByURL(t *testing.T) { t.Errorf("%s = %#v, want %#v", k, body[k], v) } } + // The link stays on the server for grounding; responses never carry it. + if got, ok := body["recipe_url"]; ok { + t.Errorf("recipe_url = %#v, want no recipe_url in a dinner response", got) + } // The same link again is a duplicate even though its name would differ. rec, body = callDinners(t, router, http.MethodPost, dinnersPath, `{"name":"Something else entirely"}`) @@ -227,6 +232,40 @@ func TestCreateDinnerByURL(t *testing.T) { } } +// TestDinnerResponsesHideRecipeURL: a pasted link may carry a personal +// token, so neither the create response nor a later list read shows any +// part of it, while the stored link still awaits grounding. +func TestDinnerResponsesHideRecipeURL(t *testing.T) { + t.Parallel() + router, st := newDinnersRouter(t) + + const link = "https://c.example/recipes/stew?access_token=personal-token-abc123" + rec, body := callDinners(t, router, http.MethodPost, dinnersPath, `{"recipe_url":"`+link+`"}`) + if rec.Code != http.StatusCreated { + t.Fatalf("status = %d, want 201: %s", rec.Code, rec.Body) + } + created := rec.Body.String() + rec, _ = callDinners(t, router, http.MethodGet, dinnersPath+"?status=regular", "") + if rec.Code != http.StatusOK { + t.Fatalf("GET status=regular = %d: %s", rec.Code, rec.Body) + } + for _, resp := range []struct{ name, body string }{{"create", created}, {"list", rec.Body.String()}} { + for _, secret := range []string{link, "c.example", "access_token", "personal-token-abc123", "recipe_url"} { + if strings.Contains(resp.body, secret) { + t.Errorf("%s response contains %q: %s", resp.name, secret, resp.body) + } + } + } + + var stored string + if err := st.Reader.QueryRow(`SELECT recipe_url FROM dinners WHERE id = ?`, body["id"]).Scan(&stored); err != nil { + t.Fatal(err) + } + if stored != link { + t.Errorf("stored recipe_url = %q, want %q kept for grounding", stored, link) + } +} + func TestCreateDinnerDuplicate(t *testing.T) { t.Parallel() router, st := newDinnersRouter(t) diff --git a/web/src/settings/RegularsSettingsPage.test.tsx b/web/src/settings/RegularsSettingsPage.test.tsx index ba7371c..990081b 100644 --- a/web/src/settings/RegularsSettingsPage.test.tsx +++ b/web/src/settings/RegularsSettingsPage.test.tsx @@ -196,7 +196,7 @@ describe('RegularsSettingsPage', () => { const link = 'https://example.com/recipes/12345/beef-stew/' const server = stubServer([], { 'POST /api/v1/dinners': () => { - const created = dinnerBody('d9', 'Beef stew', { grounding_status: 'pending', recipe_url: link }) + const created = dinnerBody('d9', 'Beef stew', { grounding_status: 'pending' }) server.regulars = [created] return jsonResponse(201, { ...created, grounding_job_id: 'job-1' }) }, diff --git a/web/src/test/dinners.ts b/web/src/test/dinners.ts index f2a1cb2..f316391 100644 --- a/web/src/test/dinners.ts +++ b/web/src/test/dinners.ts @@ -10,7 +10,6 @@ export type DinnerBody = { cuisine: string | null is_favorite: boolean recipe_id: string | null - recipe_url: string | null photo_url: string | null grounding_status: string grounding_error: string | null @@ -33,7 +32,6 @@ export function dinnerBody(id: string, name: string, overrides: Partial Date: Tue, 6 Oct 2026 20:45:37 -0400 Subject: [PATCH 2/5] Drop cached dinner responses and keep link tokens out of names (PR #97 review F1, F2) F1: bump the service worker cache version to v2 so activate deletes the v1 API cache, whose dinner responses still carried the pasted recipe_url and could be served offline. F2: nameFromURL now drops words holding a digit and ends the name at the first credential word (token, access, secret, ...), so a token in the link's last path segment never reaches the public dinner name. /recipes/beef-stew is still "Beef stew". Co-Authored-By: Claude Opus 5.5 --- internal/dinners/dinners.go | 23 ++++++++++++++++++----- internal/dinners/dinners_test.go | 7 +++++++ internal/server/dinners_test.go | 30 ++++++++++++++++++++++++++++++ web/src/sw.ts | 2 +- web/src/sw/routes.test.ts | 11 ++++++++++- web/src/sw/routes.ts | 4 +++- web/src/sw/worker.test.ts | 9 ++++++++- 7 files changed, 77 insertions(+), 9 deletions(-) diff --git a/internal/dinners/dinners.go b/internal/dinners/dinners.go index 7a39832..9dbaddc 100644 --- a/internal/dinners/dinners.go +++ b/internal/dinners/dinners.go @@ -496,10 +496,19 @@ func checkRecipeURL(raw string) (*url.URL, error) { return u, nil } +// credentialWords end the name nameFromURL takes from a slug: a pasted link +// can carry a personal token in its path, as in /stew-access_token-abc123, +// and the name is shown in every dinner response. +var credentialWords = map[string]bool{ + "access": true, "api": true, "apikey": true, "auth": true, "credential": true, + "credentials": true, "passcode": true, "password": true, "passwd": true, + "secret": true, "session": true, "sig": true, "signature": true, "token": true, +} + // nameFromURL names a dinner from the last path segment of u: separators -// become spaces, a file extension and words of only digits are dropped and -// the first letter is capitalized, so -// https://example.com/recipes/12345/chicken-tikka-masala/ is +// become spaces, a file extension and words holding a digit are dropped, the +// name ends at the first of credentialWords and the first letter is +// capitalized, so https://example.com/recipes/12345/chicken-tikka-masala/ is // "Chicken tikka masala". ok is false when no usable name is left. func nameFromURL(u *url.URL) (name string, ok bool) { segments := strings.FieldsFunc(u.Path, func(r rune) bool { return r == '/' }) @@ -512,8 +521,12 @@ func nameFromURL(u *url.URL) (name string, ok bool) { } var words []string for _, w := range strings.FieldsFunc(slug, func(r rune) bool { return r == '-' || r == '_' || r == '+' || unicode.IsSpace(r) }) { - if strings.IndexFunc(w, func(r rune) bool { return !unicode.IsDigit(r) }) >= 0 { - words = append(words, strings.ToLower(w)) + w = strings.ToLower(w) + if credentialWords[w] { + break + } + if strings.IndexFunc(w, unicode.IsDigit) < 0 { + words = append(words, w) } } name = strings.Join(words, " ") diff --git a/internal/dinners/dinners_test.go b/internal/dinners/dinners_test.go index 9f8bd80..accd4b1 100644 --- a/internal/dinners/dinners_test.go +++ b/internal/dinners/dinners_test.go @@ -19,6 +19,13 @@ func TestNameFromURL(t *testing.T) { {"https://example.com/", ""}, {"https://example.com/recipe/98765", ""}, {"https://example.com/the-best-easy-recipe", ""}, + {"https://example.com/best-chili-v2", "Best chili"}, + {"https://example.com/stew-access_token-personaltokenabc123", "Stew"}, + {"https://example.com/stew-token-abcdefghijk", "Stew"}, + {"https://example.com/chili-api-key-zyxwvut", "Chili"}, + {"https://example.com/stew-x7k2p9q4", "Stew"}, + {"https://example.com/recipes/token-abcdefghijk", ""}, + {"https://example.com/key-lime-pie", "Key lime pie"}, } for _, tt := range tests { u, err := url.Parse(tt.link) diff --git a/internal/server/dinners_test.go b/internal/server/dinners_test.go index a8285e2..b956266 100644 --- a/internal/server/dinners_test.go +++ b/internal/server/dinners_test.go @@ -266,6 +266,36 @@ func TestDinnerResponsesHideRecipeURL(t *testing.T) { } } +// TestDinnerResponsesHideTokenInLinkPath: a personal token can sit in the +// link's last path segment, which names the dinner, so the name stops before +// it and neither the create response nor a later list read shows it. +func TestDinnerResponsesHideTokenInLinkPath(t *testing.T) { + t.Parallel() + router, _ := newDinnersRouter(t) + + const link = "https://c.example/recipes/stew-access_token-personaltokenabc123" + rec, body := callDinners(t, router, http.MethodPost, dinnersPath, `{"recipe_url":"`+link+`"}`) + if rec.Code != http.StatusCreated { + t.Fatalf("status = %d, want 201: %s", rec.Code, rec.Body) + } + if body["name"] != "Stew" { + t.Errorf("name = %v, want Stew", body["name"]) + } + created := rec.Body.String() + rec, _ = callDinners(t, router, http.MethodGet, dinnersPath+"?status=regular", "") + if rec.Code != http.StatusOK { + t.Fatalf("GET status=regular = %d: %s", rec.Code, rec.Body) + } + for _, resp := range []struct{ name, body string }{{"create", created}, {"list", rec.Body.String()}} { + lower := strings.ToLower(resp.body) + for _, secret := range []string{"personaltokenabc123", "abc123", "access", "token"} { + if strings.Contains(lower, secret) { + t.Errorf("%s response contains %q: %s", resp.name, secret, resp.body) + } + } + } +} + func TestCreateDinnerDuplicate(t *testing.T) { t.Parallel() router, st := newDinnersRouter(t) diff --git a/web/src/sw.ts b/web/src/sw.ts index ca712ff..8e797c9 100644 --- a/web/src/sw.ts +++ b/web/src/sw.ts @@ -28,7 +28,7 @@ declare const self: ServiceWorkerGlobalScope & { __WB_MANIFEST: Array<{ url: string; revision: string | null }> } -// The precache becomes mealquest-precache-v1. +// The precache becomes mealquest-precache-v2. setCacheNameDetails({ prefix: 'mealquest', precache: 'precache', suffix: cacheVersion }) const day = 24 * 60 * 60 diff --git a/web/src/sw/routes.test.ts b/web/src/sw/routes.test.ts index 9b57d20..273df82 100644 --- a/web/src/sw/routes.test.ts +++ b/web/src/sw/routes.test.ts @@ -146,11 +146,20 @@ describe('cache names', () => { it('all start with mealquest-', () => { for (const name of Object.values(cacheNames)) expect(name).toMatch(/^mealquest-/) }) + + it('are v2, so activate deletes the v1 caches', () => { + // v1 API responses held dinners with the pasted recipe_url, which can + // carry a personal token; offline, the worker would still serve them. + expect(cacheNames.api).toBe('mealquest-api-v2') + const current = ['mealquest-precache-v2', ...Object.values(cacheNames)] + const v1 = ['mealquest-precache-v1', 'mealquest-api-v1', 'mealquest-media-v1', 'mealquest-fonts-v1'] + expect(staleCaches([...v1, ...current], current)).toEqual(v1) + }) }) describe('staleCaches', () => { it('deletes only MealQuest caches that are not current', () => { - const current = ['mealquest-precache-v1', ...Object.values(cacheNames)] + const current = ['mealquest-precache-v2', ...Object.values(cacheNames)] const names = [ ...current, 'mealquest-precache', diff --git a/web/src/sw/routes.ts b/web/src/sw/routes.ts index 895fd40..f865452 100644 --- a/web/src/sw/routes.ts +++ b/web/src/sw/routes.ts @@ -25,7 +25,9 @@ export interface RouteRequest { } // Bumping the version renames every cache; activate then deletes the old ones. -export const cacheVersion = 'v1' +// v2 drops v1 API responses, whose dinners still carried the pasted +// recipe_url. +export const cacheVersion = 'v2' export const cachePrefix = 'mealquest-' export const cacheNames = { diff --git a/web/src/sw/worker.test.ts b/web/src/sw/worker.test.ts index 215b9cb..4f223c4 100644 --- a/web/src/sw/worker.test.ts +++ b/web/src/sw/worker.test.ts @@ -1,7 +1,8 @@ // The service worker itself needs worker globals, so these tests read its // source: they pin the parts routes.ts cannot express — the API cache's // strategy, the font strategy's expiration, the font route's place ahead of -// the precache route and network-only requests left to the browser. +// the precache route, network-only requests left to the browser and stale +// caches deleted on activate. import { describe, expect, it } from 'vitest' import source from '../sw.ts?raw' @@ -37,6 +38,12 @@ describe('service worker source', () => { expect(source).not.toContain('registerRoute(() => true') }) + it('deletes every stale cache on activate', () => { + expect(source).toMatch( + /self\.addEventListener\('activate', \(event\) => \{\s*const current = \[workboxCacheNames\.precache, \.\.\.Object\.values\(cacheNames\)\][\s\S]*?staleCaches\(names, current\)\.map\(\(name\) => caches\.delete\(name\)\)/, + ) + }) + it('activates a waiting worker when the update prompt sends SKIP_WAITING', () => { expect(source).toMatch( /self\.addEventListener\('message', \(event\) => \{\s*if \(event\.data\?\.type === 'SKIP_WAITING'\) void self\.skipWaiting\(\)/, -- 2.55.0 From 4e0c31b7d609268d6047f9eb1b60259f32d60d34 Mon Sep 17 00:00:00 2001 From: dev_agent Date: Tue, 6 Oct 2026 20:48:03 -0400 Subject: [PATCH 3/5] Name dinners from links with letter-only words (PR #97 review F2) Keep only words of at most 20 letters from the link's last path segment, so punctuated or long token text such as /stew-t=abcdef never reaches the dinner name, and stop at combined credential words like accesstoken. Co-Authored-By: Claude Opus 5.5 --- internal/dinners/dinners.go | 25 ++++++++++++++++++------- internal/dinners/dinners_test.go | 4 ++++ 2 files changed, 22 insertions(+), 7 deletions(-) diff --git a/internal/dinners/dinners.go b/internal/dinners/dinners.go index 9dbaddc..8a72385 100644 --- a/internal/dinners/dinners.go +++ b/internal/dinners/dinners.go @@ -500,15 +500,21 @@ func checkRecipeURL(raw string) (*url.URL, error) { // can carry a personal token in its path, as in /stew-access_token-abc123, // and the name is shown in every dinner response. var credentialWords = map[string]bool{ - "access": true, "api": true, "apikey": true, "auth": true, "credential": true, - "credentials": true, "passcode": true, "password": true, "passwd": true, - "secret": true, "session": true, "sig": true, "signature": true, "token": true, + "access": true, "accesstoken": true, "api": true, "apikey": true, "auth": true, + "authtoken": true, "bearer": true, "credential": true, "credentials": true, + "jwt": true, "passcode": true, "password": true, "passwd": true, "secret": true, + "session": true, "sig": true, "signature": true, "token": true, } +// maxSlugWordChars bounds a word nameFromURL keeps: a longer run of letters +// is more likely a token than a dish. +const maxSlugWordChars = 20 + // nameFromURL names a dinner from the last path segment of u: separators -// become spaces, a file extension and words holding a digit are dropped, the -// name ends at the first of credentialWords and the first letter is -// capitalized, so https://example.com/recipes/12345/chicken-tikka-masala/ is +// become spaces, a file extension is dropped, only words of at most +// maxSlugWordChars letters are kept, the name ends at the first of +// credentialWords and the first letter is capitalized, so +// https://example.com/recipes/12345/chicken-tikka-masala/ is // "Chicken tikka masala". ok is false when no usable name is left. func nameFromURL(u *url.URL) (name string, ok bool) { segments := strings.FieldsFunc(u.Path, func(r rune) bool { return r == '/' }) @@ -525,7 +531,7 @@ func nameFromURL(u *url.URL) (name string, ok bool) { if credentialWords[w] { break } - if strings.IndexFunc(w, unicode.IsDigit) < 0 { + if utf8.RuneCountInString(w) <= maxSlugWordChars && strings.IndexFunc(w, notLetter) < 0 { words = append(words, w) } } @@ -539,6 +545,11 @@ func nameFromURL(u *url.URL) (name string, ok bool) { return name, true } +// notLetter reports whether r is neither a letter nor a combining mark. +func notLetter(r rune) bool { + return !unicode.IsLetter(r) && !unicode.IsMark(r) +} + // checkUnique returns a *DuplicateError when name is harness.Similar to an // unarchived dinner other than self, or recipeURL is one's recipe link: the // source_url of the recipe it is grounded in, else the recipe_url still diff --git a/internal/dinners/dinners_test.go b/internal/dinners/dinners_test.go index accd4b1..4d4f077 100644 --- a/internal/dinners/dinners_test.go +++ b/internal/dinners/dinners_test.go @@ -26,6 +26,10 @@ func TestNameFromURL(t *testing.T) { {"https://example.com/stew-x7k2p9q4", "Stew"}, {"https://example.com/recipes/token-abcdefghijk", ""}, {"https://example.com/key-lime-pie", "Key lime pie"}, + {"https://example.com/stew-t=abcdef", "Stew"}, + {"https://example.com/stew-accesstoken-abcdefghijk", "Stew"}, + {"https://example.com/stew-abcdefghijklmnopqrstuvwxyz", "Stew"}, + {"https://example.com/cre%CC%80me-bru%CC%82le%CC%81e", "Cre\u0300me bru\u0302le\u0301e"}, } for _, tt := range tests { u, err := url.Parse(tt.link) -- 2.55.0 From 9574d1920eff973797333441c6a81bf78a0266c3 Mon Sep 17 00:00:00 2001 From: dev_agent Date: Tue, 6 Oct 2026 21:02:16 -0400 Subject: [PATCH 4/5] Retire only the API cache when its version changes (PR #97 review F2, F3) F2: one shared cache version renamed every cache, so the v2 bump that drops the v1 API cache (dinner responses with the pasted recipe_url) also deleted saved photos, fonts and the precache. Each cache now carries its own version in cacheVersions; only api is v2, so activate deletes mealquest-api-v1 and keeps mealquest-precache-v1, -media-v1 and -fonts-v1. F3: replace the source-text activation test with src/sw.test.ts, which loads the worker with the Workbox precache and router stubbed, dispatches activate against mocked Cache Storage, and asserts caches.delete is called for mealquest-api-v1 only. tsconfig.sw.json type-checks the test with the worker's types. Co-Authored-By: Claude Opus 5.5 --- web/src/sw.test.ts | 47 +++++++++++++++++++++++++++++++++++++++ web/src/sw.ts | 6 ++--- web/src/sw/routes.test.ts | 17 ++++++++------ web/src/sw/routes.ts | 21 +++++++++++------ web/src/sw/worker.test.ts | 10 ++------- web/tsconfig.json | 2 +- web/tsconfig.sw.json | 2 +- 7 files changed, 78 insertions(+), 27 deletions(-) create mode 100644 web/src/sw.test.ts diff --git a/web/src/sw.test.ts b/web/src/sw.test.ts new file mode 100644 index 0000000..75e6d20 --- /dev/null +++ b/web/src/sw.test.ts @@ -0,0 +1,47 @@ +// Runs the service worker's activate handler against mocked Cache Storage, +// with the Workbox precache and router stubbed out. tsconfig.sw.json checks +// this file with the worker's types, like src/sw.ts itself. +import { afterEach, describe, expect, it, vi } from 'vitest' + +vi.mock('workbox-precaching', () => ({ + PrecacheFallbackPlugin: class {}, + matchPrecache: vi.fn(), + precacheAndRoute: vi.fn(), +})) +vi.mock('workbox-routing', () => ({ registerRoute: vi.fn() })) + +describe('service worker activation', () => { + afterEach(() => { + vi.unstubAllGlobals() + }) + + it('deletes the v1 API cache and keeps the precache, photo and font caches', async () => { + // A household on the v1 worker: its API cache held dinners with the + // pasted recipe_url; its photos and fonts are not in the precache. + const names = [ + 'mealquest-precache-v1', + 'mealquest-api-v1', + 'mealquest-media-v1', + 'mealquest-fonts-v1', + 'other-app-cache', + ] + const cacheStorage = { + keys: vi.fn(async () => names), + delete: vi.fn(async () => true), + } + vi.stubGlobal('caches', cacheStorage) + vi.stubGlobal('__WB_MANIFEST', []) + await import('./sw') + + const pending: Promise[] = [] + const activate = Object.assign(new Event('activate'), { + waitUntil: (promise: Promise) => pending.push(promise), + }) + self.dispatchEvent(activate) + await Promise.all(pending) + + expect(pending).toHaveLength(1) + expect(cacheStorage.delete).toHaveBeenCalledWith('mealquest-api-v1') + expect(cacheStorage.delete).toHaveBeenCalledTimes(1) + }) +}) diff --git a/web/src/sw.ts b/web/src/sw.ts index 8e797c9..3983585 100644 --- a/web/src/sw.ts +++ b/web/src/sw.ts @@ -18,7 +18,7 @@ import { CacheFirst, NetworkOnly, type Strategy } from 'workbox-strategies' import { createApiStrategy } from './sw/apiStrategy' import { cacheNames, - cacheVersion, + cacheVersions, routeFor, staleCaches, type Route, @@ -28,8 +28,8 @@ declare const self: ServiceWorkerGlobalScope & { __WB_MANIFEST: Array<{ url: string; revision: string | null }> } -// The precache becomes mealquest-precache-v2. -setCacheNameDetails({ prefix: 'mealquest', precache: 'precache', suffix: cacheVersion }) +// The precache becomes mealquest-precache-v1. +setCacheNameDetails({ prefix: 'mealquest', precache: 'precache', suffix: cacheVersions.precache }) const day = 24 * 60 * 60 diff --git a/web/src/sw/routes.test.ts b/web/src/sw/routes.test.ts index 273df82..62685b3 100644 --- a/web/src/sw/routes.test.ts +++ b/web/src/sw/routes.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from 'vitest' -import { cacheFallbackHeader, cacheNames, markCacheFallback, routeFor, staleCaches, type Route } from './routes' +import { cacheFallbackHeader, cacheNames, cacheVersions, markCacheFallback, routeFor, staleCaches, type Route } from './routes' const origin = 'https://mealquest.test' @@ -147,19 +147,22 @@ describe('cache names', () => { for (const name of Object.values(cacheNames)) expect(name).toMatch(/^mealquest-/) }) - it('are v2, so activate deletes the v1 caches', () => { + it('retire only the v1 API cache', () => { // v1 API responses held dinners with the pasted recipe_url, which can // carry a personal token; offline, the worker would still serve them. - expect(cacheNames.api).toBe('mealquest-api-v2') - const current = ['mealquest-precache-v2', ...Object.values(cacheNames)] - const v1 = ['mealquest-precache-v1', 'mealquest-api-v1', 'mealquest-media-v1', 'mealquest-fonts-v1'] - expect(staleCaches([...v1, ...current], current)).toEqual(v1) + // The precache, photos and fonts keep their v1 caches. + expect(cacheNames).toEqual({ + api: 'mealquest-api-v2', + media: 'mealquest-media-v1', + fonts: 'mealquest-fonts-v1', + }) + expect(cacheVersions.precache).toBe('v1') }) }) describe('staleCaches', () => { it('deletes only MealQuest caches that are not current', () => { - const current = ['mealquest-precache-v2', ...Object.values(cacheNames)] + const current = ['mealquest-precache-v1', ...Object.values(cacheNames)] const names = [ ...current, 'mealquest-precache', diff --git a/web/src/sw/routes.ts b/web/src/sw/routes.ts index f865452..e1f0ea3 100644 --- a/web/src/sw/routes.ts +++ b/web/src/sw/routes.ts @@ -24,16 +24,23 @@ export interface RouteRequest { mode: RequestMode } -// Bumping the version renames every cache; activate then deletes the old ones. -// v2 drops v1 API responses, whose dinners still carried the pasted -// recipe_url. -export const cacheVersion = 'v2' +// Each cache carries its own version. Bumping one renames only that cache; +// activate then deletes its old name and keeps every other cache, so saved +// photos and fonts stay available offline. +export const cacheVersions = { + precache: 'v1', + // v2 retires the v1 API cache, whose dinner responses still carried the + // pasted recipe_url. + api: 'v2', + media: 'v1', + fonts: 'v1', +} as const export const cachePrefix = 'mealquest-' export const cacheNames = { - api: `${cachePrefix}api-${cacheVersion}`, - media: `${cachePrefix}media-${cacheVersion}`, - fonts: `${cachePrefix}fonts-${cacheVersion}`, + api: `${cachePrefix}api-${cacheVersions.api}`, + media: `${cachePrefix}media-${cacheVersions.media}`, + fonts: `${cachePrefix}fonts-${cacheVersions.fonts}`, } as const export const networkTimeoutSeconds = 3 diff --git a/web/src/sw/worker.test.ts b/web/src/sw/worker.test.ts index 4f223c4..9c858c5 100644 --- a/web/src/sw/worker.test.ts +++ b/web/src/sw/worker.test.ts @@ -1,8 +1,8 @@ // The service worker itself needs worker globals, so these tests read its // source: they pin the parts routes.ts cannot express — the API cache's // strategy, the font strategy's expiration, the font route's place ahead of -// the precache route, network-only requests left to the browser and stale -// caches deleted on activate. +// the precache route and network-only requests left to the browser. +// src/sw.test.ts runs the worker's activation against mocked Cache Storage. import { describe, expect, it } from 'vitest' import source from '../sw.ts?raw' @@ -38,12 +38,6 @@ describe('service worker source', () => { expect(source).not.toContain('registerRoute(() => true') }) - it('deletes every stale cache on activate', () => { - expect(source).toMatch( - /self\.addEventListener\('activate', \(event\) => \{\s*const current = \[workboxCacheNames\.precache, \.\.\.Object\.values\(cacheNames\)\][\s\S]*?staleCaches\(names, current\)\.map\(\(name\) => caches\.delete\(name\)\)/, - ) - }) - it('activates a waiting worker when the update prompt sends SKIP_WAITING', () => { expect(source).toMatch( /self\.addEventListener\('message', \(event\) => \{\s*if \(event\.data\?\.type === 'SKIP_WAITING'\) void self\.skipWaiting\(\)/, diff --git a/web/tsconfig.json b/web/tsconfig.json index 95c28ed..a11d80b 100644 --- a/web/tsconfig.json +++ b/web/tsconfig.json @@ -19,5 +19,5 @@ "noEmit": true }, "include": ["src", "vite.config.ts", "manifest.json"], - "exclude": ["src/sw.ts"] + "exclude": ["src/sw.ts", "src/sw.test.ts"] } diff --git a/web/tsconfig.sw.json b/web/tsconfig.sw.json index a71dd06..ced4bf6 100644 --- a/web/tsconfig.sw.json +++ b/web/tsconfig.sw.json @@ -4,6 +4,6 @@ "lib": ["ES2022", "WebWorker"], "types": [] }, - "include": ["src/sw.ts"], + "include": ["src/sw.ts", "src/sw.test.ts"], "exclude": [] } -- 2.55.0 From 51bae14b7b798a412cc9a13783ce5211ed84de12 Mon Sep 17 00:00:00 2001 From: dev_agent Date: Tue, 6 Oct 2026 21:16:16 -0400 Subject: [PATCH 5/5] Require the household's name for a Regular added from a link (PR #97 review R4-F1) A pasted link may carry a personal token anywhere in its path, so no part of it can safely name a dinner: the slug heuristic always let some plain words through. POST /api/v1/dinners now takes {name} or {name, recipe_url}; a link without a name is 400 invalid_dinner, and nameFromURL is gone. The link is still kept only for grounding and for the recipe-link duplicate rule. Name-created Regulars, grounding, and the rename rules are unchanged. On the Regulars screen, pasting a link shows a "Dinner name" field, and Add stays disabled until it is filled. The optimistic row shows that name, never the link. Co-Authored-By: Claude Opus 5.5 --- docs/ARCHITECTURE.md | 9 +- internal/dinners/dinners.go | 95 +++-------------- internal/dinners/dinners_test.go | 100 ++++++++++++------ internal/dinners/grounding_test.go | 12 +-- internal/server/dinners.go | 3 +- internal/server/dinners_test.go | 50 +++++---- web/src/dinners/api.ts | 15 +-- .../settings/RegularsSettingsPage.module.css | 7 ++ .../settings/RegularsSettingsPage.test.tsx | 28 +++-- web/src/settings/RegularsSettingsPage.tsx | 63 +++++++---- 10 files changed, 200 insertions(+), 182 deletions(-) diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 9a33eec..4da8dd8 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -179,13 +179,12 @@ automatic** (D-021). response holds the key. `Service.HarnessConfig` reads the settings for each skill run, so a saved change applies to the next run. - Dinners (`dinners.Service`). `POST /api/v1/dinners` adds a Regular from - exactly one of `{name}` or `{recipe_url}` (an https:// link, kept in + `{name}` or `{name, recipe_url}` (an https:// link, kept in `recipe_url` for grounding and never returned in a dinner response, as it may carry a personal token) with status and origin `regular` and - grounding_status `pending`, answering 201 with the dinner. A link's - dinner is named from its last path segment (separators become spaces, - an extension and all-digit words are dropped: `…/12345/beef-stew/` is - "Beef stew"); a link that names nothing is 400 — add it by name. A name + grounding_status `pending`, answering 201 with the dinner. The name is + always the household's: no part of a link is ever used as a dinner's + name, and a link without a name is 400. A name is trimmed, at most 120 characters and must keep a word after `harness.Normalize`, which sets `normalized_name`. A name `harness.Similar` to an unarchived dinner's, or a link one already has diff --git a/internal/dinners/dinners.go b/internal/dinners/dinners.go index 8a72385..ae2a20b 100644 --- a/internal/dinners/dinners.go +++ b/internal/dinners/dinners.go @@ -16,7 +16,6 @@ import ( "slices" "strings" "sync" - "unicode" "unicode/utf8" "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/ai/harness" @@ -79,7 +78,10 @@ type Dinner struct { UpdatedAt string `json:"updated_at"` } -// NewRegular is a Regular to add: exactly one of Name and RecipeURL. +// NewRegular is a Regular to add: its Name, which the household always +// gives, and optionally the RecipeURL to ground it in. The name is never +// taken from the link: a pasted link may carry a personal token anywhere in +// it, and the name is shown in every dinner response. type NewRegular struct { Name *string RecipeURL *string @@ -171,10 +173,9 @@ type Created struct { GroundingJobID *string `json:"grounding_job_id"` } -// CreateRegular adds a Regular named by r.Name, or by the last path segment -// of r.RecipeURL (whose recipe grounding later replaces), with status and -// origin regular and grounding_status pending, recording updatedBy, and -// enqueues the KindGroundDinner job that grounds it. It returns without +// CreateRegular adds a Regular named r.Name, keeping r.RecipeURL for +// grounding when given, with status and origin regular and grounding_status +// pending, recording updatedBy, and enqueues the KindGroundDinner job that grounds it. It returns without // waiting for the job. A name repeating an unarchived dinner is a // *DuplicateError. When the job cannot be enqueued, the stored dinner is // returned with grounding_status failed (ReasonGroundingError) and no job @@ -224,34 +225,24 @@ func (s *Service) createRegular(ctx context.Context, r NewRegular, updatedBy str if updatedBy == "" { return db.Dinner{}, errors.New("dinners: create: no member to record as updated_by") } - var name string + if r.Name == nil { + return db.Dinner{}, invalid("name", "give the dinner's name, with or without a recipe_url") + } + name, err := checkName(*r.Name) + if err != nil { + return db.Dinner{}, err + } var recipeURL sql.NullString - switch { - case r.Name != nil && r.RecipeURL != nil: - return db.Dinner{}, invalid("name", "give a name or a recipe_url, not both") - case r.Name != nil: - n, err := checkName(*r.Name) - if err != nil { - return db.Dinner{}, err - } - name = n - case r.RecipeURL != nil: + if r.RecipeURL != nil { u, err := checkRecipeURL(*r.RecipeURL) if err != nil { return db.Dinner{}, err } - n, ok := nameFromURL(u) - if !ok { - return db.Dinner{}, invalid("recipe_url", "the link does not name a dinner; add it by name instead") - } - name = n recipeURL = sql.NullString{String: u.String(), Valid: true} - default: - return db.Dinner{}, invalid("name", "give a name or a recipe_url") } var row db.Dinner - err := s.st.WithTx(ctx, func(tx *sql.Tx) error { + err = s.st.WithTx(ctx, func(tx *sql.Tx) error { q := db.New(tx) if err := checkUnique(ctx, q, "", name, recipeURL); err != nil { return err @@ -496,60 +487,6 @@ func checkRecipeURL(raw string) (*url.URL, error) { return u, nil } -// credentialWords end the name nameFromURL takes from a slug: a pasted link -// can carry a personal token in its path, as in /stew-access_token-abc123, -// and the name is shown in every dinner response. -var credentialWords = map[string]bool{ - "access": true, "accesstoken": true, "api": true, "apikey": true, "auth": true, - "authtoken": true, "bearer": true, "credential": true, "credentials": true, - "jwt": true, "passcode": true, "password": true, "passwd": true, "secret": true, - "session": true, "sig": true, "signature": true, "token": true, -} - -// maxSlugWordChars bounds a word nameFromURL keeps: a longer run of letters -// is more likely a token than a dish. -const maxSlugWordChars = 20 - -// nameFromURL names a dinner from the last path segment of u: separators -// become spaces, a file extension is dropped, only words of at most -// maxSlugWordChars letters are kept, the name ends at the first of -// credentialWords and the first letter is capitalized, so -// https://example.com/recipes/12345/chicken-tikka-masala/ is -// "Chicken tikka masala". ok is false when no usable name is left. -func nameFromURL(u *url.URL) (name string, ok bool) { - segments := strings.FieldsFunc(u.Path, func(r rune) bool { return r == '/' }) - if len(segments) == 0 { - return "", false - } - slug := segments[len(segments)-1] - if i := strings.LastIndexByte(slug, '.'); i > 0 { - slug = slug[:i] - } - var words []string - for _, w := range strings.FieldsFunc(slug, func(r rune) bool { return r == '-' || r == '_' || r == '+' || unicode.IsSpace(r) }) { - w = strings.ToLower(w) - if credentialWords[w] { - break - } - if utf8.RuneCountInString(w) <= maxSlugWordChars && strings.IndexFunc(w, notLetter) < 0 { - words = append(words, w) - } - } - name = strings.Join(words, " ") - if r, size := utf8.DecodeRuneInString(name); r != utf8.RuneError { - name = string(unicode.ToUpper(r)) + name[size:] - } - if _, err := checkName(name); err != nil { - return "", false - } - return name, true -} - -// notLetter reports whether r is neither a letter nor a combining mark. -func notLetter(r rune) bool { - return !unicode.IsLetter(r) && !unicode.IsMark(r) -} - // checkUnique returns a *DuplicateError when name is harness.Similar to an // unarchived dinner other than self, or recipeURL is one's recipe link: the // source_url of the recipe it is grounded in, else the recipe_url still diff --git a/internal/dinners/dinners_test.go b/internal/dinners/dinners_test.go index 4d4f077..620fdc6 100644 --- a/internal/dinners/dinners_test.go +++ b/internal/dinners/dinners_test.go @@ -1,44 +1,74 @@ -package dinners +package dinners_test import ( - "net/url" + "encoding/json" + "errors" + "strings" "testing" + + "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/dinners" ) -func TestNameFromURL(t *testing.T) { +// TestCreateRegularNeverNamesFromLink: a Regular added with a recipe link +// is named only by the household, so no part of the link — host, path, +// query or fragment — is stored as its name or shown in a dinner or a +// duplicate error, even a token of plain letters in the path. +func TestCreateRegularNeverNamesFromLink(t *testing.T) { t.Parallel() - tests := []struct { - link string - want string // "" means no usable name - }{ - {"https://example.com/recipes/12345/chicken-tikka-masala/", "Chicken tikka masala"}, - {"https://example.com/recipe/beef_stew.html", "Beef stew"}, - {"https://example.com/2024/05/best-ever-chili-123", "Best ever chili"}, - {"https://example.com/crème-brûlée", "Crème brûlée"}, - {"https://example.com/recipes/chili?utm=1#notes", "Chili"}, - {"https://example.com/", ""}, - {"https://example.com/recipe/98765", ""}, - {"https://example.com/the-best-easy-recipe", ""}, - {"https://example.com/best-chili-v2", "Best chili"}, - {"https://example.com/stew-access_token-personaltokenabc123", "Stew"}, - {"https://example.com/stew-token-abcdefghijk", "Stew"}, - {"https://example.com/chili-api-key-zyxwvut", "Chili"}, - {"https://example.com/stew-x7k2p9q4", "Stew"}, - {"https://example.com/recipes/token-abcdefghijk", ""}, - {"https://example.com/key-lime-pie", "Key lime pie"}, - {"https://example.com/stew-t=abcdef", "Stew"}, - {"https://example.com/stew-accesstoken-abcdefghijk", "Stew"}, - {"https://example.com/stew-abcdefghijklmnopqrstuvwxyz", "Stew"}, - {"https://example.com/cre%CC%80me-bru%CC%82le%CC%81e", "Cre\u0300me bru\u0302le\u0301e"}, - } - for _, tt := range tests { - u, err := url.Parse(tt.link) - if err != nil { - t.Fatal(err) - } - got, ok := nameFromURL(u) - if got != tt.want || ok != (tt.want != "") { - t.Errorf("nameFromURL(%s) = %q, %v; want %q", tt.link, got, ok, tt.want) + e := newUnstartedEnv(t) + const link = "https://c.example/stew-key-abcdefghijklmno?sig=zyxwvut#frag" + leaks := []string{"c.example", "key", "abcdefghijklmno", "sig", "zyxwvut", "frag"} + check := func(what, text string) { + t.Helper() + lower := strings.ToLower(text) + for _, leak := range leaks { + if strings.Contains(lower, leak) { + t.Errorf("%s contains %q: %s", what, leak, text) + } } } + + _, err := e.svc.CreateRegular(t.Context(), dinners.NewRegular{RecipeURL: ptr(link)}, member) + var verr *dinners.ValidationError + if !errors.As(err, &verr) { + t.Fatalf("link without a name: err = %v, want a *ValidationError", err) + } + check("validation error", verr.Error()) + + c, err := e.svc.CreateRegular(t.Context(), dinners.NewRegular{Name: ptr(" Sunday stew "), RecipeURL: ptr(link)}, member) + if err != nil { + t.Fatal(err) + } + if c.Name != "Sunday stew" { + t.Errorf("name = %q, want the household's %q", c.Name, "Sunday stew") + } + created, err := json.Marshal(c) + if err != nil { + t.Fatal(err) + } + check("create response", string(created)) + row := e.dinner(t, c.ID) + check("stored name", row.Name+" "+row.NormalizedName) + if row.RecipeUrl.String != link { + t.Errorf("stored recipe_url = %q, want %q kept for grounding", row.RecipeUrl.String, link) + } + + list, err := e.svc.List(t.Context(), dinners.StatusRegular) + if err != nil { + t.Fatal(err) + } + listed, err := json.Marshal(list) + if err != nil { + t.Fatal(err) + } + check("list response", string(listed)) + + // The same link under another name is still a duplicate, and its error + // names the existing dinner by the household's name. + _, err = e.svc.CreateRegular(t.Context(), dinners.NewRegular{Name: ptr("Weeknight chili"), RecipeURL: ptr(link)}, member) + var dup *dinners.DuplicateError + if !errors.As(err, &dup) || dup.Existing.ID != c.ID { + t.Fatalf("same link again: err = %v, want a duplicate of %s", err, c.ID) + } + check("duplicate error", dup.Error()) } diff --git a/internal/dinners/grounding_test.go b/internal/dinners/grounding_test.go index c6ada37..5cff022 100644 --- a/internal/dinners/grounding_test.go +++ b/internal/dinners/grounding_test.go @@ -194,7 +194,7 @@ func TestCreateByRecipeURLGroundsThatURL(t *testing.T) { e := newEnv(t) e.model.QueueGenerate(recipestest.NormalizedReply("oven", 150)) - c, err := e.svc.CreateRegular(t.Context(), dinners.NewRegular{RecipeURL: ptr(stew)}, member) + c, err := e.svc.CreateRegular(t.Context(), dinners.NewRegular{Name: ptr("Beef stew"), RecipeURL: ptr(stew)}, member) if err != nil { t.Fatal(err) } @@ -223,7 +223,7 @@ func TestCreateGroundingFailureRecordsReason(t *testing.T) { reason string }{ {"no candidates", dinners.NewRegular{Name: ptr("Unknown dish")}, nil, dinners.ReasonNoRecipe}, - {"unusable link", dinners.NewRegular{RecipeURL: ptr("https://gone.example/recipes/lost-soup")}, nil, dinners.ReasonRecipeUnusable}, + {"unusable link", dinners.NewRegular{Name: ptr("Lost soup"), RecipeURL: ptr("https://gone.example/recipes/lost-soup")}, nil, dinners.ReasonRecipeUnusable}, {"search fails", dinners.NewRegular{Name: ptr("Chicken curry")}, func(e *env) { e.search.Err = recipes.ErrSearchRateLimited }, dinners.ReasonSearchFailed}, {"model fails", dinners.NewRegular{Name: ptr("Chicken curry")}, func(e *env) { // One provider failure per candidate tried. @@ -375,7 +375,7 @@ func TestRenameLinkWhilePendingGroundsNewName(t *testing.T) { e.search.Results["Beef stew"] = []recipes.Candidate{{URL: stew}} e.model.QueueGenerate(recipestest.NormalizedReply("oven", 150)) - c, err := e.svc.CreateRegular(t.Context(), dinners.NewRegular{RecipeURL: ptr(curryA)}, member) + c, err := e.svc.CreateRegular(t.Context(), dinners.NewRegular{Name: ptr("Chicken curry"), RecipeURL: ptr(curryA)}, member) if err != nil { t.Fatal(err) } @@ -740,7 +740,7 @@ func TestFixReplacesPendingLink(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() e := newEnv(t) - c, err := e.svc.CreateRegular(t.Context(), dinners.NewRegular{RecipeURL: ptr(lost)}, member) + c, err := e.svc.CreateRegular(t.Context(), dinners.NewRegular{Name: ptr("Lamb roast"), RecipeURL: ptr(lost)}, member) if err != nil { t.Fatal(err) } @@ -763,11 +763,11 @@ func TestFixReplacesPendingLink(t *testing.T) { t.Errorf("recipe_url = %q after the fix, want the unusable link dropped", row.RecipeUrl.String) } - if _, err := e.svc.CreateRegular(t.Context(), dinners.NewRegular{RecipeURL: ptr(lost)}, member); err != nil { + if _, err := e.svc.CreateRegular(t.Context(), dinners.NewRegular{Name: ptr("Lentil soup"), RecipeURL: ptr(lost)}, member); err != nil { t.Errorf("adding the dropped link again: %v, want added", err) } if tt.linkTaken != "" { - _, err := e.svc.CreateRegular(t.Context(), dinners.NewRegular{RecipeURL: ptr(tt.linkTaken)}, member) + _, err := e.svc.CreateRegular(t.Context(), dinners.NewRegular{Name: ptr("Curry night"), RecipeURL: ptr(tt.linkTaken)}, member) var dup *dinners.DuplicateError if !errors.As(err, &dup) || dup.Existing.ID != c.ID { t.Errorf("adding the fixed dinner's recipe link: err = %v, want a duplicate of %s", err, c.ID) diff --git a/internal/server/dinners.go b/internal/server/dinners.go index b6c627f..d78cb9d 100644 --- a/internal/server/dinners.go +++ b/internal/server/dinners.go @@ -19,7 +19,8 @@ const maxDinnerBody = 16 << 10 // manual recipe of dinners.MaxManualLines lines of each kind. const maxGroundBody = 512 << 10 -// dinnerCreate is the POST /dinners body: a name or a recipe_url. +// dinnerCreate is the POST /dinners body: a name, with or without a +// recipe_url. type dinnerCreate struct { Name *string `json:"name"` RecipeURL *string `json:"recipe_url"` diff --git a/internal/server/dinners_test.go b/internal/server/dinners_test.go index b956266..49e617c 100644 --- a/internal/server/dinners_test.go +++ b/internal/server/dinners_test.go @@ -186,12 +186,12 @@ func TestCreateDinnerByURL(t *testing.T) { router, _ := newDinnersRouter(t) link := "https://www.example.com/recipes/12345/beef-and-broccoli-stir-fry/" - rec, body := callDinners(t, router, http.MethodPost, dinnersPath, `{"recipe_url":"`+link+`"}`) + rec, body := callDinners(t, router, http.MethodPost, dinnersPath, `{"name":"Stir fry night","recipe_url":"`+link+`"}`) if rec.Code != http.StatusCreated { t.Fatalf("status = %d, want 201: %s", rec.Code, rec.Body) } for k, v := range map[string]any{ - "name": "Beef and broccoli stir fry", + "name": "Stir fry night", "status": "regular", "origin": "regular", "grounding_status": "pending", @@ -211,18 +211,17 @@ func TestCreateDinnerByURL(t *testing.T) { if rec.Code != http.StatusCreated { t.Fatalf("unrelated name = %d: %s", rec.Code, rec.Body) } - rec, body = callDinners(t, router, http.MethodPost, dinnersPath, `{"recipe_url":"`+link+`"}`) + rec, body = callDinners(t, router, http.MethodPost, dinnersPath, `{"name":"Beef and broccoli","recipe_url":"`+link+`"}`) if rec.Code != http.StatusConflict || errorCode(body) != "duplicate_dinner" { t.Errorf("same link again = %d %v, want 409 duplicate_dinner", rec.Code, body) } for _, tc := range []struct{ name, body string }{ - {"plain http", `{"recipe_url":"http://example.com/recipes/chili"}`}, - {"not a URL", `{"recipe_url":"chili"}`}, - {"credentials", `{"recipe_url":"https://me:pw@example.com/recipes/chili"}`}, - {"no name in path", `{"recipe_url":"https://example.com/"}`}, - {"numeric slug", `{"recipe_url":"https://example.com/recipe/98765"}`}, - {"both fields", `{"name":"Chili","recipe_url":"https://example.com/recipes/chili"}`}, + {"plain http", `{"name":"Chili","recipe_url":"http://example.com/recipes/chili"}`}, + {"not a URL", `{"name":"Chili","recipe_url":"chili"}`}, + {"credentials", `{"name":"Chili","recipe_url":"https://me:pw@example.com/recipes/chili"}`}, + {"link without a name", `{"recipe_url":"https://example.com/recipes/chili"}`}, + {"filler name with a link", `{"name":"The Best Easy Recipe","recipe_url":"https://example.com/recipes/chili"}`}, {"neither field", `{}`}, } { rec, body := callDinners(t, router, http.MethodPost, dinnersPath, tc.body) @@ -240,7 +239,7 @@ func TestDinnerResponsesHideRecipeURL(t *testing.T) { router, st := newDinnersRouter(t) const link = "https://c.example/recipes/stew?access_token=personal-token-abc123" - rec, body := callDinners(t, router, http.MethodPost, dinnersPath, `{"recipe_url":"`+link+`"}`) + rec, body := callDinners(t, router, http.MethodPost, dinnersPath, `{"name":"Stew","recipe_url":"`+link+`"}`) if rec.Code != http.StatusCreated { t.Fatalf("status = %d, want 201: %s", rec.Code, rec.Body) } @@ -266,29 +265,36 @@ func TestDinnerResponsesHideRecipeURL(t *testing.T) { } } -// TestDinnerResponsesHideTokenInLinkPath: a personal token can sit in the -// link's last path segment, which names the dinner, so the name stops before -// it and neither the create response nor a later list read shows it. +// TestDinnerResponsesHideTokenInLinkPath: a personal token can sit +// anywhere in a link's path, even as plain letters, so a dinner added from +// a link is named only by the household and neither the create response, a +// later list read nor the duplicate error for the same link shows any of +// the link. func TestDinnerResponsesHideTokenInLinkPath(t *testing.T) { t.Parallel() router, _ := newDinnersRouter(t) - const link = "https://c.example/recipes/stew-access_token-personaltokenabc123" - rec, body := callDinners(t, router, http.MethodPost, dinnersPath, `{"recipe_url":"`+link+`"}`) + const link = "https://c.example/recipes/stew-key-abcdefghijklmno" + rec, body := callDinners(t, router, http.MethodPost, dinnersPath, `{"name":"Sunday stew","recipe_url":"`+link+`"}`) if rec.Code != http.StatusCreated { t.Fatalf("status = %d, want 201: %s", rec.Code, rec.Body) } - if body["name"] != "Stew" { - t.Errorf("name = %v, want Stew", body["name"]) + if body["name"] != "Sunday stew" { + t.Errorf("name = %v, want Sunday stew", body["name"]) } created := rec.Body.String() rec, _ = callDinners(t, router, http.MethodGet, dinnersPath+"?status=regular", "") if rec.Code != http.StatusOK { t.Fatalf("GET status=regular = %d: %s", rec.Code, rec.Body) } - for _, resp := range []struct{ name, body string }{{"create", created}, {"list", rec.Body.String()}} { + listed := rec.Body.String() + rec, _ = callDinners(t, router, http.MethodPost, dinnersPath, `{"name":"Beef stew","recipe_url":"`+link+`"}`) + if rec.Code != http.StatusConflict { + t.Fatalf("same link again = %d, want 409: %s", rec.Code, rec.Body) + } + for _, resp := range []struct{ name, body string }{{"create", created}, {"list", listed}, {"duplicate", rec.Body.String()}} { lower := strings.ToLower(resp.body) - for _, secret := range []string{"personaltokenabc123", "abc123", "access", "token"} { + for _, secret := range []string{"c.example", "recipes", "key", "abcdefghijklmno"} { if strings.Contains(lower, secret) { t.Errorf("%s response contains %q: %s", resp.name, secret, resp.body) } @@ -311,10 +317,10 @@ func TestCreateDinnerDuplicate(t *testing.T) { t.Errorf("%q: %d %v, want 409 duplicate_dinner", name, rec.Code, body) } } - // A slug naming the same dish is a duplicate too. - rec, body := callDinners(t, router, http.MethodPost, dinnersPath, `{"recipe_url":"https://example.com/easy-chicken-tikka-masala.html"}`) + // A name repeating the dish is a duplicate with a link too. + rec, body := callDinners(t, router, http.MethodPost, dinnersPath, `{"name":"Chicken tikka masala","recipe_url":"https://example.com/other-curry.html"}`) if rec.Code != http.StatusConflict { - t.Errorf("URL slug duplicate = %d %v, want 409", rec.Code, body) + t.Errorf("named duplicate with a link = %d %v, want 409", rec.Code, body) } var n int diff --git a/web/src/dinners/api.ts b/web/src/dinners/api.ts index b0a39e5..657d1de 100644 --- a/web/src/dinners/api.ts +++ b/web/src/dinners/api.ts @@ -33,8 +33,10 @@ export type Regular = { pending?: true } -// NewRegular is a POST /api/v1/dinners body: a name or a recipe link. -export type NewRegular = { name: string } | { recipe_url: string } +// NewRegular is a POST /api/v1/dinners body: the household's name for the +// dinner, with the recipe link to ground it in when one was pasted. A +// dinner is never named from its link, which may carry private details. +export type NewRegular = { name: string; recipe_url?: string } // RegularPatch is a PATCH /api/v1/dinners/{id} body; every field is optional. export type RegularPatch = Partial<{ name: string; method: Method; minutes_total: number; difficulty: Difficulty }> @@ -195,10 +197,9 @@ function settle(queryClient: QueryClient) { let pendingIds = 0 -// useAddRegular adds a Regular by name or recipe link (POST -// /api/v1/dinners). It shows in the list at once — a link under the link -// itself until the server names it — and is taken out again if the server -// refuses it. +// useAddRegular adds a Regular by name, with or without a recipe link (POST +// /api/v1/dinners). It shows in the list at once under its name and is +// taken out again if the server refuses it. export function useAddRegular() { const { apiFetch } = useApi() const queryClient = useQueryClient() @@ -210,7 +211,7 @@ export function useAddRegular() { const id = `pending-${++pendingIds}` const optimistic: CachedRegular = { id, - name: 'name' in add ? add.name : add.recipe_url, + name: add.name, method: null, minutes_total: null, difficulty: null, diff --git a/web/src/settings/RegularsSettingsPage.module.css b/web/src/settings/RegularsSettingsPage.module.css index e952791..0954766 100644 --- a/web/src/settings/RegularsSettingsPage.module.css +++ b/web/src/settings/RegularsSettingsPage.module.css @@ -43,6 +43,13 @@ font-weight: 700; } +/* The add form: the add row, then the name field a pasted link needs. */ +.addForm { + display: flex; + flex-direction: column; + gap: 8px; +} + /* The add field: the input fills the row beside the grape + button. */ .add { display: flex; diff --git a/web/src/settings/RegularsSettingsPage.test.tsx b/web/src/settings/RegularsSettingsPage.test.tsx index 990081b..afae963 100644 --- a/web/src/settings/RegularsSettingsPage.test.tsx +++ b/web/src/settings/RegularsSettingsPage.test.tsx @@ -192,21 +192,33 @@ describe('RegularsSettingsPage', () => { expect(screen.getByRole('heading', { level: 2, name: 'On rotation · 2' })).toBeInTheDocument() }) - it('adds a Regular from a pasted recipe link', async () => { - const link = 'https://example.com/recipes/12345/beef-stew/' + it('adds a Regular from a pasted recipe link under the name the household gives it', async () => { + const link = 'https://example.com/recipes/stew-key-abcdefghijklmno' const server = stubServer([], { - 'POST /api/v1/dinners': () => { - const created = dinnerBody('d9', 'Beef stew', { grounding_status: 'pending' }) + 'POST /api/v1/dinners': (body) => { + const created = dinnerBody('d9', (body as { name: string }).name, { grounding_status: 'pending' }) server.regulars = [created] return jsonResponse(201, { ...created, grounding_job_id: 'job-1' }) }, }) await renderPage() - add(link) - expect(await screen.findByText('Beef stew')).toBeInTheDocument() - expect(sent(server.fetchMock)).toEqual([{ request: 'POST /api/v1/dinners', body: { recipe_url: link } }]) - expect(within(row('Beef stew')).getByText('Finding recipe…')).toBeInTheDocument() + fireEvent.change(addField(), { target: { value: ` ${link} ` } }) + // A link alone cannot be added: it never names a dinner. + expect(screen.getByRole('button', { name: 'Add Regular' })).toBeDisabled() + const name = screen.getByRole('textbox', { name: 'Dinner name' }) + expect(name).toHaveAccessibleDescription(/never names a dinner/) + fireEvent.change(name, { target: { value: ' Sunday stew ' } }) + fireEvent.click(screen.getByRole('button', { name: 'Add Regular' })) + + expect(addField()).toHaveValue('') + expect(screen.queryByRole('textbox', { name: 'Dinner name' })).not.toBeInTheDocument() + expect(await screen.findByText('Sunday stew')).toBeInTheDocument() + expect(sent(server.fetchMock)).toEqual([ + { request: 'POST /api/v1/dinners', body: { name: 'Sunday stew', recipe_url: link } }, + ]) + expect(within(row('Sunday stew')).getByText('Finding recipe…')).toBeInTheDocument() + expect(document.body).not.toHaveTextContent(/example\.com|abcdefghijklmno|key/i) }) it('takes back an add the server refuses and says why', async () => { diff --git a/web/src/settings/RegularsSettingsPage.tsx b/web/src/settings/RegularsSettingsPage.tsx index 6464606..c816b49 100644 --- a/web/src/settings/RegularsSettingsPage.tsx +++ b/web/src/settings/RegularsSettingsPage.tsx @@ -49,12 +49,14 @@ export const methodLabels: Record = { const difficultyLabels: Record = { easy: 'Easy', medium: 'Medium', hard: 'Hard' } -// A pasted link is added as a recipe link; anything else is a dinner name. +// A pasted link is added as a recipe link under a name the household gives; +// anything else is a dinner name. const LINK = /^https?:\/\//i // Settings · Regular dinners: the household's Regulars, each with its photo, // method and minutes, favorite heart and whether its recipe was found. The -// field at the top adds one by name or pasted recipe link; tapping one +// field at the top adds one by name, or by pasted recipe link with a name +// the household gives it; tapping one // edits or removes it, or fixes its recipe when none was found. export function RegularsSettingsPage() { const navigate = useNavigate() @@ -85,19 +87,24 @@ function RegularsManager() { const favorite = useToggleFavorite() const { refusals, track } = useRefusals() const [draft, setDraft] = useState('') + const [linkName, setLinkName] = useState('') const [open, setOpen] = useState(null) const headingId = useId() + const linkHintId = useId() + const isLink = LINK.test(draft.trim()) + const ready = draft.trim() !== '' && (!isLink || linkName.trim() !== '') const list = regulars.data ?? [] // A Regular removed meanwhile closes its sheet. const opened = open && list.find((regular) => regular.id === open.id && !regular.pending) function onSubmit(event: FormEvent) { event.preventDefault() + if (!ready) return const text = draft.trim() - if (text === '') return - const body: NewRegular = LINK.test(text) ? { recipe_url: text } : { name: text } - track(`add:${text.toLowerCase()}`, `add ${text}`, add.mutateAsync(body)) + const body: NewRegular = isLink ? { name: linkName.trim(), recipe_url: text } : { name: text } + track(`add:${body.name.toLowerCase()}`, `add ${body.name}`, add.mutateAsync(body)) setDraft('') + setLinkName('') } function onRemove(regular: Regular) { @@ -111,20 +118,38 @@ function RegularsManager() { return ( <> -
- setDraft(event.target.value)} - /> - - - + +
+ setDraft(event.target.value)} + /> + + + +
+ {isLink && ( + <> + setLinkName(event.target.value)} + /> +

+ Name it yourself: a link can carry private details, so it never names a dinner. +

+ + )} {refusals.map((refusal) => (

-- 2.55.0