feat(suggestions): C2 — Meal suggestions (variety-aware) #40
Reference in New Issue
Block a user
Delete Branch "feat/issue-27-meal-suggestions"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Implements issue #27: variety-aware meal suggestions screen (C2).
SuggestionContextBannerwith week-so-far context, ranked suggestion list, browse library link--color-surface) with week-so-far mini cards + filter reasons + library link; right (flex,--color-page) with ranked suggestion cards--radius-full), pick form$state), shows planned days + filter criterion bulletssimulatedScoredescending/v1/week-plans/{id}/slots?selectFor={day}&week={weekStart}to B1 (selection mode integration point)Test plan
Closes #27
🤖 Generated with Claude Code
Two-panel desktop layout (280px context panel + flex suggestions panel) and mobile layout (sticky header + collapsible context banner + ranked list). - SuggestionCard: rank number, recipe metadata, green/yellow reasoning badge, pick form - SuggestionContextBanner: collapsible week-so-far list + filter reasons - Server load: fetches week plan + variety-aware suggestions sorted by simulatedScore - pickSuggestion action: role guard, date validation, POST to /v1/week-plans/{id}/slots - Browse full library link passes selectFor+week params to recipe library (B1 integration point) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>👨💻 Kai — Frontend Engineer
Verdict: ⚠️ Approved with concerns
Good structure — Svelte 5 runes used correctly,
$derivedfor computed values, clean component decomposition. TDD is evident. A few things need addressing.Blockers
1. Reasoning badge derived from score threshold (7.5) — not from API data
In
+page.svelte,rankedSuggestionshardcodes the reasoning label as either "Passt gut zur Woche" or "Wiederholung möglich" based onsimulatedScore >= 7.5. This throws away the backend's actual reasoning. The issue spec says the badge text is server-generated (e.g., "✓ Fresh protein · effort balance", "⚠ Chicken again — 2 days apart"). The API response should include badge type and label — but looking atSuggestionItemin the schema, neitherreasoningTypenorreasoningLabelexists. Either: (a) the schema needs extending, or (b) the frontend derives minimal labels until the schema is updated. The current approach is a workaround — it should be documented with a// TODO: replace with API-provided reasoning when availablecomment, not silently shipped as the intended behavior.2.
planIdcan be empty string in suggestion cards whenweekPlan?.idis nullishWhen
weekPlan?.id ?? ''evaluates to'', the form submitsplanId: ""to the action. The action doesn't validateplanId— it passes""to the API which will either 400 or 404. Add aplanIdvalidation check in the action (like the existingslotDatevalidation). IfplanIdis missing, return{ success: false, error: 'Fehlende Plan-ID.' }.3. Missing redirect after successful pick
The
pickSuggestionaction returns{ success: true }but doesn't redirect to C1. The user stays on C2 after picking a suggestion. The issue spec says "Pick a suggestion → writes week_plan_slot → returns to C1." Useredirect(303,/planner?week=${weekStart})(import from@sveltejs/kit) after the successful POST.Suggestions
SuggestionContextBannertoggle button text "Filter ausblenden ▲ / Filter einblenden ▼" — chevron arrows are text characters, which may render inconsistently. A simple Tailwind rotate transform on an SVG chevron is more reliable._localsparam in+page.server.tsload function — the underscore prefix is correct to indicate unused param. Consider whether to destructure only what's needed fromlocalsin the future.🧪 QA Engineer
Verdict: 🚫 Changes requested
Good coverage on the server side (11 tests). Component tests cover the card and banner adequately. But there are critical gaps.
Blockers
1. No test for the missing redirect after successful pick
After a successful
pickSuggestion, the action should redirect to C1. There is no test asserting that aredirect()is thrown/returned. Once the redirect is implemented (it's currently missing), it needs a test:shouldRedirectToC1AfterSuccessfulPick().2. No test for
planIdvalidation in the actionThe
planIdfield isn't validated in the action (can be empty string""). When validation is added, it needs a test:shouldReturnErrorForMissingPlanId().3. No test for suggestions returned in reversed order (already sorted)
The test "returns suggestions list sorted by simulatedScore descending" covers the sort — but only tests with
9.2 > 6.1. There's no test for the edge case where suggestions arrive already sorted (no-op sort), or all with equal scores (stable sort), or a single suggestion. Add at minimum a test for empty suggestions list returning[].4.
SuggestionContextBannertoggle test is fragileThe test asserts
detail.hasAttribute('hidden') || detail.getAttribute('aria-hidden') === 'true'equalsinitiallyVisible. This is checking two different attributes depending on implementation and can produce confusing results. Simplify: after toggle, check that the hidden attribute state is opposite of the initial state. The test currently passes by coincidence because both attributes are set — but if one is removed, the test still passes.5. No test for
SuggestionCardwith no recipeWhat happens if
suggestion.recipeisundefined? The component falls back to "Unbekanntes Rezept" — but there's no test for this case. The existing "card without reasoning" test doesn't cover missingrecipe.Suggestions
selectForparameter in itshref.🔒 Sable — Security Engineer
Verdict: ⚠️ Approved with concerns
Role guard on
pickSuggestionis present. Date validation exists. No{@html}usage. However, two blockers.Blockers
1.
planIdfrom user-supplied form data passes unvalidated to the API path parameter+page.server.tsline 51:params: { path: { id: planId } }—planIdcomes from a hidden form field. A planner can manipulate the form'splanIdvalue in the browser to inject another household's plan ID. The backend should enforce household scoping (and should be the authoritative guard), but the frontend action doesn't validate thatplanIdis non-empty. At minimum, add:if (!planId) return { success: false, error: 'Fehlende Plan-ID.' }. The deeper fix is server-side validation, but a blank/garbageplanIdshould be caught early.2.
recipeIdfrom form data has no format validationrecipeIdis submitted as a form field from the hidden input. No UUID format validation — a crafted request could submit arbitrary strings. The backend validates, but defense-in-depth suggests:if (!recipeId || !/^[0-9a-f-]{36}$/.test(recipeId)) return { success: false, error: '...' }. Low risk (backend controls authorization), but consistent with theslotDatepattern already in place.3.
selectForparameter in "Browse full library" link is not validated on the recipes pageThe URL
/recipes?selectFor={selectedDay}&week={weekStart}passes untrustedselectedDayfrom page data. When B1 reads this, it must validateselectForserver-side (correct household, correct plan slot). This is an implementation note for when B1's selection mode is built — flag it clearly with a code comment or issue.Observations
fetch— household scoping is at the API layer. Correct._localsunderscore correctly documents that locals aren't used in the load function.🎨 Atlas — UI/UX Designer
Verdict: ⚠️ Approved with concerns
Token usage is mostly correct. The two-panel desktop structure matches the spec. Minor issues.
Blockers
1.
SuggestionCardrecipe name is truncated withtruncate— single line onlyThe spec doesn't explicitly require truncation for recipe names, but the Atlas spec consistently uses
line-clamp-2rather than single-line truncation for recipe titles in list contexts. A recipe name like "Hähnchenbrust mit Zitronensauce und Kartoffelgratin" will truncate badly. Suggestline-clamp-2overtruncatefor the recipe name<p>.2. Mobile context banner defaults to expanded — should default collapsed
The spec says the banner is "collapsible" — meaning it should default collapsed on mobile to minimize vertical space.
let expanded = $state(true)means it defaults open. Change to$state(false)so the banner defaults collapsed, saving screen space for the suggestion list.3. Rank number should be
self-startto top-align with multi-line recipe namesThe rank column uses
w-10 flex-shrink-0 text-right. Withoutself-start, it will stretch to match the full card height when recipe names wrap. Addself-startto the rank<div>.Suggestions
--shadow-card— correct for at-rest interactive surfaces.🔧 Backend Engineer
Verdict: ✅ Approved
Frontend-only PR. API contract usage is correct.
Checked
GET /v1/week-planswith{ query: { weekStart } }— correct for fetching the week plan for context.GET /v1/week-plans/{id}/suggestionswith{ path: { id }, query: { slotDate } }— matchesgetSuggestionsoperation withslotDateas required query param. Fixed correctly after the type error.POST /v1/week-plans/{id}/slotswith{ path: { id: planId }, body: { slotDate, recipeId } }— matchesaddSlot/CreateSlotRequest. Correct.suggestionsData?.suggestionsaccesses theSuggestionResponse.suggestionsarray correctly.simulatedScoreis adoublein the schema — sorting by it numerically is correct.One note
The load function ignores the
slotDatequery param when calling suggestions — it usesselectedDayfrom the URL which is the same value. This is fine. But worth noting that ifselectedDaydefaults toweekStart(nodayparam), the suggestions will be for Monday, which may not be the intended slot. The backend handles this correctly (returns suggestions for whatever date is passed), so this is a UX concern, not a bug.