feat(planner): wire variety-aware suggestions into RecipePicker for empty slots #47
Reference in New Issue
Block a user
Delete Branch "feat/issue-46-wire-suggestions-recipe-picker"
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?
Closes #46
Summary
SuggestionItemDTO now exposesscoreDelta(simulated − current variety score) andhasConflict(scoreDelta ≤ 0) instead of rawsimulatedScore; candidates sorted byscoreDeltadescGET /planner?planId&dateSvelteKit route fetches backend suggestions, re-sorts, and degrades gracefully on all error pathsscoreDelta-based badges (green = variety gain, yellow = conflict) and a pulse skeleton while loading;currentVarietyScoreprop removed$derived activePickerDateunifies mobile/desktop picker trigger;$effectlazy-fetches suggestions when picker opens and wires them into both RecipePicker instancesSuggestionCard.svelte+ testTest plan
./mvnw test— 294 tests greennpm run check(0 errors) +npm run test(599 passing)hasConflict = falseshow green badge; those withhasConflict = trueshow yellow ⚠ badge- Suggestion interface: { recipe, scoreDelta, hasConflict } (no simulatedScore) - Badge renders from hasConflict directly — no client-side delta computation needed - New isLoading prop shows skeleton rows while suggestions fetch is in flight - currentVarietyScore prop removed from component and both call sites follow in next commit Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>🔧 Backend Engineer — Senior Spring Boot / PostgreSQL Specialist
Verdict: ⚠️ Approved with concerns
Overall clean implementation. The DTO change,
scoreDelta/hasConflictsemantics, and@Transactional(readOnly = true)annotation are all correct. A few things deserve attention.Blockers
Magic number 10.0 in two places without a named constant
PlanningService.java:141and:205:scoreFromSimulatedSlotsalso starts at10.0(double score = 10.0). The value 10.0 is the maximum variety score and appears in at least 3 places. If this ever changes, it'll be silently wrong. Extract to a private constant:private static final double MAX_VARIETY_SCORE = 10.0;Suggestions
Duplicated
currentScorecomputation logicThe same pattern — "compute currentSlots, compute currentScore, then simulate" — now appears in both
getSuggestions(line 138–142) andgetVarietyPreview(line 202–206). This is pre-existing duplication made more visible by this PR. Consider extractingcomputeCurrentScore(WeekPlan, VarietyScoreConfig, Set<UUID>)to a private helper. Not a blocker since the logic is correct in both places, but the duplication is a maintenance hazard.hasConflict = scoreDelta <= 0includes the neutral case (scoreDelta = 0.0)The test
recipeWithNoConflictsOnEmptyPlanShouldHaveZeroDeltaAndHasConflictexplicitly documents this: a perfectly clean recipe on an empty plan getshasConflict = truebecause0.0 <= 0. The frontend shows a yellow warning badge. This is technically correct per spec but the naminghasConflictfor the neutral case is semantically odd. Since it matches the spec I won't block on it, but a comment in the DTO or service explaining the intent would help future readers.SuggestionsTest.java— no test fortopN = 0boundarylimit <= 0returns an empty list (service line 120–122), but the test suite doesn't covertopN = 0ortopN = -1. The guard is there; it should be tested.⚡ Kai — Frontend Engineer (Svelte 5 / SvelteKit 2)
Verdict: ⚠️ Approved with concerns
The Svelte 5 rune usage is correct throughout —
$state,$derived,$effectall used appropriately. No legacy syntax. TheactivePickerDatederived pattern cleanly unifies mobile and desktop contexts. Two things need addressing.Blockers
No fetch cancellation in
$effect— stale responses will clobber current state+page.svelte:103–115:If
activePickerDatechanges before the inflight request resolves (user opens picker for Monday, closes it, opens for Thursday), the first response will still write tosuggestionsandisLoadingSuggestions. In the worst case the user sees suggestions for the wrong day. Fix withAbortController:Suggestions
suggestions: any[]— loose typing+page.svelte:72. TheSuggestioninterface is already defined inRecipePicker.svelte. Either export it from a shared types file and use it here, or at minimum inline the shape.any[]makes it impossible to catch field name mismatches at compile time — which is exactly how thescoreDeltabug (reported separately) slipped through.scoreDelta.toFixed(0)can throw ifscoreDeltaisundefinedRecipePicker.svelte:125(green badge path). The TypeScript type hasscoreDelta?: number(optional), so at runtime if data arrives without the field, this throws. Defensive fix:(suggestion.scoreDelta ?? 0).toFixed(0). The$effectshould prevent this with correct backend data, but component props shouldn't crash on partial data.🧪 QA Engineer — Test Coverage Review
Verdict: ⚠️ Approved with concerns
Good test coverage overall. The server route has 6 dedicated tests,
RecipePickerhas 12 component tests, andSuggestionsTestis thorough with nested class organization. Gaps below.Blockers
No test for the
$effectfetch behavior in+page.svelteThe lazy-fetch logic — the core of this feature from the user's perspective — has zero test coverage. When
activePickerDatebecomes non-null, a fetch should fire and populate suggestions. This is testable: mockfetch, render the page withpickerOpen = true, assert thatisLoadingis shown then suggestions appear. Without this, the integration betweenactivePickerDate → $effect → suggestions → RecipePickeris only covered by manual testing.server.test.ts: error branch test is incompleteserver.test.ts:62–70tests that{ data: undefined, error: { status: 500 } }returns{ suggestions: [] }. But the implementation handles this viadata?.suggestions ?? []— it doesn't inspect theerrorfield at all. What's actually tested is thedata === undefinedcase, not an error response. The test name says "when backend returns error" but what it's really testing is "when data is undefined". The test passes for the right behavior but misleads future readers.Suggestions
Missing test: stale fetch / race condition
If
activePickerDatechanges while a fetch is in-flight, the old response should not overwrite the new state. This isn't tested. The behavior is currently broken (noAbortController), so a test would have driven the fix.Missing test:
topN = 0andtopN = -1inSuggestionsTestPlanningService.java:120–122has an explicitlimit <= 0guard but no test covers it. Should be a 2-liner inBaseCases.SuggestionsTest:emptyPlanWithRecipesShouldReturnAllWithZeroDeltaname vs. behaviorThe test name says "should return all with zero delta" but the assertion is
allSatisfy(s -> assertThat(s.scoreDelta()).isEqualTo(0.0)). It doesn't asserthasConflict. SincehasConflict = (0.0 <= 0) = true, all items on an empty plan are in conflict — a separate assertion would document this surprising behavior explicitly.🔒 Sable — Security Engineer
Verdict: ✅ Approved
Reviewed against the OWASP top 10 checklist for this feature's attack surface.
Access control: The
/plannerSvelteKit server route sits behindhooks.server.tssession validation — the household session is already authenticated before this route executes. TheplanIdis passed to the Spring Boot backend which callsfindPlan(planId, householdId), enforcing household ownership at the service layer. A user cannot query another household's suggestions by guessing a plan UUID.Injection:
planIdanddateare passed as query parameters to the typedopenapi-fetchclient — they flow through parameterized API calls, not string-concatenated SQL or URLs. No injection vector.SSRF: The fetch target is the fixed
BACKEND_URLenv var. TheplanIdanddatevalues go into the path/query of a known internal endpoint. The user cannot influence which host is contacted.XSS: No
{@html}usage. Recipe names and score values are rendered as text nodes.scoreDelta.toFixed(0)produces a number string — safe.Data exposure: The suggestion response only exposes
recipe(name, effort, cookTime, heroImageUrl) and score metadata — no PII, no session tokens, no sensitive fields.One note (not a blocker): The
+server.tsroute doesn't validate thatplanIdis a valid UUID format before forwarding it to the backend. The backend will reject invalid UUIDs with a 400/500, and the frontend falls back to{ suggestions: [] }, so there's no security risk. But a quickUUID.fromString-style check server-side would prevent unnecessary backend round-trips on malformed requests.🎨 Atlas — UI/UX Designer
Verdict: ⚠️ Approved with concerns
The design token usage is correct throughout. Spot-checking against the design system.
Blockers
Badge font-size: 8px is below the readable minimum
RecipePicker.svelte:122(green badge) and:129(yellow badge):font-size: 8px. This is below WCAG 2.2 AA readable thresholds and violates the design system's intent. The smallest defined size in the system is used for metadata (9pxat line 115). Badges that convey functional meaning (score gain / conflict warning) should be at minimum9px. Change tofont-size: 9pxto match the metadata line directly above.Green badge shows
+0 Punktefor scoreDelta between 0 and 0.5RecipePicker.svelte:125:suggestion.scoreDelta.toFixed(0)rounds to nearest integer. AscoreDeltaof0.4displays as↑ +0 Punkte— which contradicts the green "gain" framing. SincehasConflict = scoreDelta <= 0means the green badge only fires forscoreDelta > 0, a value of0.4rounds to 0 and looks nonsensical. ConsidertoFixed(1)to show one decimal place, or clamp to a minimum display of+1if integer display is preferred.Suggestions
Loading skeleton animation:
@keyframes pulsemust be globally definedRecipePicker.svelte:83:animation: pulse 1.5s ease-in-out infinite. The@keyframes pulserule is not defined in the component's<style>block. If it's defined in a global stylesheet this works, but it's fragile — the component doesn't declare its own dependency. Add a<style>block with the keyframes, or use a CSS custom property animation that's guaranteed to exist."+ Wählen" button color
RecipePicker.svelte:140:background: var(--green). Elsewhere in the planner (e.g. the+ Gerichtheader button) the color isvar(--green-dark). Per the design system, interactive buttons use--green-dark.--greenis the mid-scale accent, not the button color. Align to--green-darkfor consistency.Review concerns addressed ✅
All reviewer concerns from the multi-persona review have been resolved. Summary by commit:
10.0→MAX_VARIETY_SCOREconstant482597btopN = 0andtopN = -1testse17e8d4hasConflict = trueassertion for neutral deltaf84a647computeCurrentScoreprivate helper0a9e803AbortControllerin$effect539ca5dpage.test.tscovering fetch trigger + AbortSignal539ca5dsuggestions: any[]→Suggestion[](shared type)f7a2396server.test.tserror-branch test renamed8686f9efont-size: 8px→9pxcb921b3toFixed(1)cb921b3@keyframes pulseself-contained in<style>blockcb921b3var(--green)→var(--green-dark)cb921b3Test results: Backend 296/296 ✅ · Frontend 602/602 ✅ (2 pre-existing failures in
recipes/page.test.tsunrelated to this PR)🖥️ Backend Engineer — Spring Boot / PostgreSQL
Verdict: ⚠️ Approved with concerns
Blockers
None.
Suggestions
1. PR description has wrong
hasConflictsemanticsThe description says
hasConflict (scoreDelta ≤ 0)but the implementation correctly usesscoreDelta < 0. The description is wrong — neutral suggestions (delta = 0) should not be conflicts, and the code is right. Worth correcting in the PR body to avoid confusion when reading history.2. Scoring logic is duplicated between
scoreFromSimulatedSlotsandgetVarietyScoregetVarietyScoreinlines its own penalty calculation (tag repeats, ingredient overlaps, recent repeats, duplicates) instead of delegating toscoreFromSimulatedSlots. This was pre-existing, but this PR adds correctness toscoreFromSimulatedSlots(the slot-replacement fix) thatgetVarietyScorenever benefits from. If the algorithms ever drift, the displayed score and the suggestion deltas will disagree. Suggest extractinggetVarietyScoreto call the shared helper.3. O(n × m) per suggestion call with
topN=100getSuggestionsloads all household recipes and scores each one against all current slots and their tags/ingredients. For 100 recipes × 7 slots × a handful of tags each, it's fine today. But this is worth a comment in the code so future maintainers don't raisetopNarbitrarily.Positives
simulateVarietyScore(skip the existing slot atslotDatebefore adding the candidate) is clean and correct. The logic is easy to follow.SuggestionResponseas a Java record is idiomatic.scoreDelta+hasConflictis a clean contract — the frontend doesn't need to implement scoring logic itself.scoreDeltadesc before limiting is the right place to sort (service layer, not controller).@Transactional(readOnly = true)ongetSuggestions— correct.🌐 Frontend Engineer — Svelte 5 / SvelteKit
Verdict: 🚫 Changes requested
Blockers
1. Dead import:
SwapSuggestionListin+page.svelteBoth swap contexts (mobile bottom sheet + desktop panel) now render
RecipePickerinstead ofSwapSuggestionList. The import at line 10 is unused and will cause asvelte-checkwarning (and eventually an error with stricter lint rules). Remove it.2. Unused derived:
sortedRecipesin+page.sveltesortedRecipeswas passed toSwapSuggestionListasrecipes={sortedRecipes}. After replacing both usages withRecipePicker(which receivesallRecipes={data.recipes}directly),sortedRecipesis never read. Dead derived state — remove it along with thesortEasiestFirstimport if nothing else uses it.Suggestions
3.
RecipePickerheader says "Rezept wählen" in swap contextThe component always renders "Rezept wählen" in its own header div. In the swap context the outer panel/sheet title already says "Gericht tauschen", so users see both. Consider making the internal header conditional (
{#if !replacingRecipe}) or replacing it with just the date label whenreplacingRecipeis set.4. Mobile swap no longer has an explicit cancel button
The old
SwapSuggestionListrendered an "Abbrechen" button whenoncancelwas provided.RecipePickerhas no cancel affordance. On mobile this is covered by the BottomSheet drag handle, which is fine — just confirm this is intentional.Positives
activePickerDateto coverswapSheetOpenis the cleanest possible fix — one derived, one$effect, three picker contexts.await tick()inhandleRemoveMealis exactly the right Svelte 5 approach. The comment explaining why is valuable.{#snippet scoreBadge(...)}shared between Empfohlen and Alle Rezepte is a textbook use of Svelte 5 snippets — avoids repeating the badge markup.baseRecipesderived filtersexcludeRecipeIdonce, thenfilteredRecipeschains off it for search. Clean composition.isDisabledpropagated through to both button locations in the component — correctly disables during in-flight PATCH.🧪 Tester
Verdict: ⚠️ Approved with concerns
Blockers
None.
Concerns
1. Mobile swap fetch is untested
activePickerDatewas extended to returnselectedDaywhenswapSheetOpenis true, triggering the suggestion fetch for the mobile swap sheet. There is no test verifying this — thepage.test.tssuite only coverspickerOpen(add flow) triggering a fetch. Given this was the whole point of the last commit, a test like "opens swap sheet → fetch is called with correct planId and date" would close the gap.2.
SwapSuggestionList.test.tstests a component no longer used in any swap flowSwapSuggestionListis still in the codebase and its tests pass, but the component is not rendered anywhere after this PR. The tests are misleading — they imply behaviour that users never encounter. Either delete the component and its tests (if it's truly orphaned), or add a note that it's kept for future use.3.
isDisabled(PATCH-in-flight) not tested end-to-endRecipePickerhas unit tests forisDisableddisabling buttons, but there's no page-level test verifying that clicking "Wählen" while a swap is in progress doesn't trigger a second PATCH. TheswapLoading→isDisabledwiring is only exercised manually.Positives
RecipePicker.test.ts: 24 tests across all badge states, loading skeleton, search, pick callbacks, cap at 5, exclude logic — thorough.server.test.ts: All five error/edge paths for the proxy route covered (missing params, success, undefined data, network error, empty array).VarietyScoreCard.test.ts: Regression test for the6.199999999999999float display bug is a good permanent guard.SuggestionsTest.java:swappingExistingSlotForCleanRecipeShouldHavePositiveDeltais exactly the right regression test for the slot-replacement fix — verifies the delta is positive, not just non-negative.page.test.tsremove-meal tests usewithin(undoBar)scoping — good practice.🔒 Security
Verdict: ✅ Approved
Checked
Proxy route
GET /planner(+server.ts)planIdanddatecome fromurl.searchParams(client-supplied).planIdis passed as a path parameter to the backend without UUID validation. The backend will reject malformed values and thecatchblock silently returns{ suggestions: [] }— no error details leaked to the client. Acceptable.topN: 100is hardcoded in the proxy — the client cannot escalate this value. Good constraint.Frontend data rendering
delta.toFixed(1)produces a numeric string — no injection surface.scoreMap.get(recipe.id)lookups use recipe IDs sourced from server-loaded data, not user input.handleRemoveMeal/ form submissionsweekPlandata, not from URL params or user input.Minor observation
If
planIdwere ever populated from a user-controlled source (e.g. URL param), the lack of UUID validation in the proxy would become a concern. Right nowweekPlan?.idcomes from the server load, so the surface is clean. Worth acrypto.randomUUID()shape check if the source ever changes.🎨 UI/UX
Verdict: ⚠️ Approved with concerns
Concerns
1. Redundant header in swap context
RecipePickeralways renders its own "Rezept wählen" header with the date label. In the swap context, the sheet/panel already has a "Gericht tauschen" title directly above it. Users see two headings stacked. Either hide the inner header whenreplacingRecipeis set, or replace "Rezept wählen" with just the date label in that context — the "Wird ersetzt" banner already gives enough context.2. Empty "Empfohlen" section for clean plans
When the plan has no existing variety penalties, no recipe scores positively (delta > 0) and "Empfohlen" never renders. New users see only yellow and red badges, which makes every option look like a downgrade or a warning. Consider a brief note in the "Alle Rezepte" section when no recommendations exist: e.g. "Dein Plan ist bereits abwechslungsreich — alle Rezepte sind gleich gut."
3. Neutral badge copy
= 0.0 Punkteis technically accurate but reads like a system log. "Kein Einfluss auf die Abwechslung" or simply hiding the badge for delta=0 (only showing it for positive and negative) would be cleaner. Users understand "no badge" as neutral.Positives
+1.5 Punkte,−0.3 Punkte) respects the user's ability to make an informed decision. Good balance of detail without overwhelming.Review concerns addressed
All blockers and suggestions from the multi-persona review have been resolved. Here's a summary by concern:
✅ SwapSuggestionList deleted — RecipePicker used exclusively in swap context
SwapSuggestionList.svelteand its test file were deleted. The swap sheet and swap panel now renderRecipePickerwith two new props:excludeRecipeId(hides the current recipe from suggestions) andreplacingRecipe(provides swap context).activePickerDateon the page was extended to coverswapSheetOpenso suggestions are fetched when the swap sheet opens.→ Commits:
148f6a7,16162d8,f4503b0✅ Neutral badge copy changed to "Kein Einfluss"
The previous copy ("⚠ Variationskonflikt") was misleading — it read like a warning log. Changed to "Kein Einfluss" with a neutral grey style (
data-type="neutral").→ Commit:
813ddf8✅ RecipePicker inner header hidden in swap context
When
replacingRecipeis set, the "Rezept wählen / {dateLabel}" header block is suppressed — the panel/sheet title already provides that context.→ Commit:
7359eba✅ Mobile swap sheet suggestion fetch verified by test
A new page-level test asserts that opening the mobile swap sheet triggers a fetch with the correct
planIdanddatequery params.→ Commit:
49ed75a✅ Backend score formula duplication eliminated
PlanningServicehad identical penalty arithmetic duplicated inscoreFromSimulatedSlotsandgetVarietyScore. Extracted into a singleapplyPenalties(tagRepeats, ingredientOverlaps, recentRepeats, duplicates, config)private helper. Both methods now delegate to it. All 290 backend tests remain green.→ Commit:
116e400