From 51bae14b7b798a412cc9a13783ce5211ed84de12 Mon Sep 17 00:00:00 2001 From: dev_agent Date: Tue, 6 Oct 2026 21:16:16 -0400 Subject: [PATCH] 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) => (