feat(staples): A3/D3 — Pantry staples toggle UI #35

marcel · 2026-04-02T20:17:44+02:00

marcel commented

2026-04-02 20:17:44 +02:00

Summary

StapleChip: toggle button with aria-pressed, selected/unselected visual states, focus ring for keyboard accessibility
CategorySection: renders an eyebrow heading + row of chips; bubbles onToggle(id, value) up
StaplesManager: holds $state for all chip selections; optimistic updates with debounced PATCH (300 ms) to /household/staples; reverts chip and shows inline error on failure; 2-col grid in onboarding context, 3-col in settings
+server.ts: PATCH proxy — validates id, calls PATCH /v1/ingredients/{id} via typed API client, returns 204 or 500
+page.server.ts: load function fetches categories and ingredients in parallel, groups ingredients by category id
+page.svelte: context-aware layout — ?ctx=onboarding renders ProgressSidebar (step 2 active) + mobile "Schritt 2 von 3" indicator + Continue/Skip footer; no ctx renders plain settings heading
household/setup redirect: updated to /household/staples?ctx=onboarding
household/invite stub: landing page for the Continue button

Test plan

All 221 unit tests pass (npm run test)
No type errors (npm run check)
Chip toggle works optimistically (immediate visual feedback)
Chip reverts on failed PATCH + error message appears
Debounce: rapid clicks send only one PATCH after 300 ms
Onboarding flow: household setup → staples (step 2 active) → invite
Settings flow: /household/staples without ?ctx shows plain heading, 3-col grid

Closes #20

🤖 Generated with Claude Code

## Summary - **StapleChip**: toggle button with `aria-pressed`, selected/unselected visual states, focus ring for keyboard accessibility - **CategorySection**: renders an eyebrow heading + row of chips; bubbles `onToggle(id, value)` up - **StaplesManager**: holds `$state` for all chip selections; optimistic updates with debounced PATCH (300 ms) to `/household/staples`; reverts chip and shows inline error on failure; 2-col grid in onboarding context, 3-col in settings - **`+server.ts`**: PATCH proxy — validates `id`, calls `PATCH /v1/ingredients/{id}` via typed API client, returns 204 or 500 - **`+page.server.ts`**: load function fetches categories and ingredients in parallel, groups ingredients by category id - **`+page.svelte`**: context-aware layout — `?ctx=onboarding` renders ProgressSidebar (step 2 active) + mobile "Schritt 2 von 3" indicator + Continue/Skip footer; no `ctx` renders plain settings heading - **`household/setup` redirect**: updated to `/household/staples?ctx=onboarding` - **`household/invite` stub**: landing page for the Continue button ## Test plan - [ ] All 221 unit tests pass (`npm run test`) - [ ] No type errors (`npm run check`) - [ ] Chip toggle works optimistically (immediate visual feedback) - [ ] Chip reverts on failed PATCH + error message appears - [ ] Debounce: rapid clicks send only one PATCH after 300 ms - [ ] Onboarding flow: household setup → staples (step 2 active) → invite - [ ] Settings flow: `/household/staples` without `?ctx` shows plain heading, 3-col grid Closes #20 🤖 Generated with [Claude Code](https://claude.com/claude-code)

marcel added 8 commits 2026-04-02 20:17:44 +02:00

feat(staples): add StapleChip component with aria-pressed toggle and focus ring 7bdc049461

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

feat(staples): add CategorySection component with eyebrow heading and chip row 376dc03646

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

feat(staples): add StaplesManager with optimistic toggle and debounced PATCH d577e0231c

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

feat(staples): add PATCH proxy server route for ingredient staple toggle 54df70a442

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

feat(staples): load categories and ingredients, group by category 3550d681dc

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

feat(staples): add staples page with onboarding and settings layouts 97175e7d9d

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

refactor(setup): redirect to /household/staples?ctx=onboarding after household creation d68a9d9312

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

feat(invite): stub household invite page as onboarding Continue target 7979076f5e

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

marcel referenced this pull request

2026-04-02 20:17:55 +02:00

Frontend: A3/D3 — Pantry staples component (onboarding + settings) #20

marcel commented

2026-04-02 20:25:14 +02:00

🧑‍💻 Kai — Frontend Engineer

Verdict: 🚫 Changes requested

Good TDD discipline throughout, clean component decomposition, debounce pattern is solid. But there's one production bug that will silently break the entire onboarding flow.

Blockers

1. ctx is never passed as a prop by SvelteKit — onboarding layout will never render in production

+page.svelte declares ctx as a $props() field:

let { data, ctx }: { data: { categories: Category[] }; ctx?: string } = $props();

SvelteKit only passes data (from load), form (from actions), and children to page components. URL query parameters are not passed as props. In production ctx will always be undefined — the {#if isOnboarding} branch will never be entered, the ProgressSidebar will never show, and the onboarding flow is silently broken.

The test works only because @testing-library/svelte lets you inject arbitrary props directly. The mock for $app/state is set up but the component never actually reads from it.

Fix option A — handle in load function (cleaner, SSR-safe):

// +page.server.ts
export const load: PageServerLoad = async ({ fetch, url }) => {
  // ... existing fetch logic ...
  return { categories, ctx: url.searchParams.get('ctx') };
};

Then in the page: let { data } = $props(); const isOnboarding = $derived(data.ctx === 'onboarding');

Fix option B — read from $app/state in the component:

import { page } from '$app/state';
const isOnboarding = $derived(page.url.searchParams.get('ctx') === 'onboarding');

(Also update the test to not pass ctx as a prop at all — mock page.url.searchParams.get to return 'onboarding'.)

Either works; A is more testable.

2. <svelte:head> before <script> in +page.svelte

The original stub had <svelte:head> at the top, and the new <script> block was appended after it, leaving the non-conventional order. Svelte compiles it, but script should come first — it's the established pattern in every other page in the codebase (see household/setup/+page.svelte).

Suggestions

StaplesManager.svelte: The debounce helper defined inline is fine for now. If it's needed elsewhere (shopping list real-time interactions come to mind), extract it to $lib/utils/debounce.ts.
The debouncedPatchers record is initialised lazily via getPatcher(). Clean pattern, no issues.
$effect re-initialising stapleState on every categories change is intentional but means any in-flight optimistic updates get reset if the parent re-renders with new props. Low risk for v1, worth a comment.

## 🧑‍💻 Kai — Frontend Engineer **Verdict: 🚫 Changes requested** Good TDD discipline throughout, clean component decomposition, debounce pattern is solid. But there's one production bug that will silently break the entire onboarding flow. --- ### Blockers **1. `ctx` is never passed as a prop by SvelteKit — onboarding layout will never render in production** `+page.svelte` declares `ctx` as a `$props()` field: ```ts let { data, ctx }: { data: { categories: Category[] }; ctx?: string } = $props(); ``` SvelteKit only passes `data` (from load), `form` (from actions), and `children` to page components. URL query parameters are **not** passed as props. In production `ctx` will always be `undefined` — the `{#if isOnboarding}` branch will never be entered, the ProgressSidebar will never show, and the onboarding flow is silently broken. The test works only because `@testing-library/svelte` lets you inject arbitrary props directly. The mock for `$app/state` is set up but the component never actually reads from it. Fix option A — handle in load function (cleaner, SSR-safe): ```ts // +page.server.ts export const load: PageServerLoad = async ({ fetch, url }) => { // ... existing fetch logic ... return { categories, ctx: url.searchParams.get('ctx') }; }; ``` Then in the page: `let { data } = $props(); const isOnboarding = $derived(data.ctx === 'onboarding');` Fix option B — read from `$app/state` in the component: ```ts import { page } from '$app/state'; const isOnboarding = $derived(page.url.searchParams.get('ctx') === 'onboarding'); ``` (Also update the test to not pass `ctx` as a prop at all — mock `page.url.searchParams.get` to return `'onboarding'`.) Either works; A is more testable. --- **2. `<svelte:head>` before `<script>` in `+page.svelte`** The original stub had `<svelte:head>` at the top, and the new `<script>` block was appended after it, leaving the non-conventional order. Svelte compiles it, but script should come first — it's the established pattern in every other page in the codebase (see `household/setup/+page.svelte`). --- ### Suggestions - `StaplesManager.svelte`: The `debounce` helper defined inline is fine for now. If it's needed elsewhere (shopping list real-time interactions come to mind), extract it to `$lib/utils/debounce.ts`. - The `debouncedPatchers` record is initialised lazily via `getPatcher()`. Clean pattern, no issues. - `$effect` re-initialising `stapleState` on every `categories` change is intentional but means any in-flight optimistic updates get reset if the parent re-renders with new props. Low risk for v1, worth a comment.

marcel commented

2026-04-02 20:25:32 +02:00

🖥️ Backend Engineer — API Proxy & Server Logic

Verdict: ⚠️ Approved with concerns

This is a frontend PR — I'm focusing exclusively on +server.ts and +page.server.ts, since that's where backend concerns touch.

Concerns

1. isStaple is forwarded to the backend without type validation

+server.ts:

const { id, isStaple } = body;
// isStaple is passed directly — could be undefined, string, anything
const { error } = await api.PATCH('/v1/ingredients/{id}', {
  params: { path: { id } },
  body: { isStaple }
});

id is validated (returns 400 if missing), but isStaple has no check. A caller could send { id: 'ing-1' } with no isStaple — the schema says it's optional so the backend would accept a no-op PATCH. Worse, { id: 'ing-1', isStaple: 'yes' } or { id: 'ing-1', isStaple: null } would reach the backend and the TypeScript types would be coerced at runtime. Add:

if (typeof isStaple !== 'boolean') {
  return json({ error: 'isStaple must be a boolean' }, { status: 400 });
}

2. Backend error status code is swallowed — always returns 500

If the backend returns a 403 (user not authorized to patch this ingredient) or 404 (ingredient doesn't exist), the proxy collapses it to a generic 500. The client gets no actionable information and the error handling in StaplesManager shows the same generic message regardless.

At a minimum, forward the backend's status code:

if (error) {
  const status = (error as any).status ?? 500;
  return json({ error: 'Failed to update ingredient' }, { status });
}

LGTM

+page.server.ts load function: Promise.all for parallel fetches is the right call. Grouping by category.id is clean. Non-null assertions (cat.id!) are acceptable since the schema types these as optional but the API will always return them.
The apiClient(fetch) pattern correctly threads SvelteKit's fetch through so session cookies are forwarded to the backend. Good.

## 🖥️ Backend Engineer — API Proxy & Server Logic **Verdict: ⚠️ Approved with concerns** This is a frontend PR — I'm focusing exclusively on `+server.ts` and `+page.server.ts`, since that's where backend concerns touch. --- ### Concerns **1. `isStaple` is forwarded to the backend without type validation** `+server.ts`: ```ts const { id, isStaple } = body; // isStaple is passed directly — could be undefined, string, anything const { error } = await api.PATCH('/v1/ingredients/{id}', { params: { path: { id } }, body: { isStaple } }); ``` `id` is validated (returns 400 if missing), but `isStaple` has no check. A caller could send `{ id: 'ing-1' }` with no `isStaple` — the schema says it's optional so the backend would accept a no-op PATCH. Worse, `{ id: 'ing-1', isStaple: 'yes' }` or `{ id: 'ing-1', isStaple: null }` would reach the backend and the TypeScript types would be coerced at runtime. Add: ```ts if (typeof isStaple !== 'boolean') { return json({ error: 'isStaple must be a boolean' }, { status: 400 }); } ``` **2. Backend error status code is swallowed — always returns 500** If the backend returns a 403 (user not authorized to patch this ingredient) or 404 (ingredient doesn't exist), the proxy collapses it to a generic 500. The client gets no actionable information and the error handling in `StaplesManager` shows the same generic message regardless. At a minimum, forward the backend's status code: ```ts if (error) { const status = (error as any).status ?? 500; return json({ error: 'Failed to update ingredient' }, { status }); } ``` --- ### LGTM - `+page.server.ts` load function: `Promise.all` for parallel fetches is the right call. Grouping by `category.id` is clean. Non-null assertions (`cat.id!`) are acceptable since the schema types these as optional but the API will always return them. - The `apiClient(fetch)` pattern correctly threads SvelteKit's `fetch` through so session cookies are forwarded to the backend. Good.

marcel commented

2026-04-02 20:25:54 +02:00

🔬 QA Engineer — Test Coverage Review

Verdict: ⚠️ Approved with concerns

Strong unit test foundation — 221 tests green, good use of fake timers for debounce, optimistic revert is covered. A few coverage gaps worth addressing.

Missing Test Cases

1. StaplesManager — no test for empty categories array

The component renders {#each categories as category} — if an empty array is passed, nothing renders. This is probably fine (it won't crash) but the edge case is untested. Given that the load function returns [] when the API fails (via ?? []), this is reachable in prod.

it('renders without crashing when categories is empty', () => {
  render(StaplesManager, { props: { categories: [], context: 'onboarding' } });
  expect(screen.queryByRole('button')).not.toBeInTheDocument();
});

2. page.server.ts — no test for API failure during load

The load function uses ?? [] as fallback when either API call fails (categoriesResult.data ?? []), but this path is completely untested. If /v1/ingredient-categories returns an error, categories silently becomes empty — the user sees a blank page with no feedback.

it('returns empty categories when API fails', async () => {
  mockGet.mockResolvedValue({ data: undefined, error: { status: 500 } });
  const result = await load({ fetch: vi.fn() } as any);
  expect(result.categories).toEqual([]);
});

3. +server.ts — no test for missing isStaple in body

The handler validates id but not isStaple. There's no test asserting what happens when isStaple is absent. Given the backend engineer flagged this as a gap, there's a corresponding missing test:

it('returns 400 when isStaple is missing', async () => {
  const response = await PATCH(createRequest({ id: 'ing-1' }));
  expect(response.status).toBe(400);
  expect(mockPatch).not.toHaveBeenCalled();
});

4. page.test.ts — the ctx bug means these tests don't verify real SvelteKit behaviour

The onboarding tests pass ctx as a direct prop, which works in jsdom but is not how SvelteKit delivers this value in production. All 6 onboarding tests are testing a prop-passing mechanism that doesn't exist at runtime. Once the ctx prop is fixed (either via load function or $app/state), the tests need updating to test the actual mechanism.

Minor Issues

5. Dynamic import in CategorySection.test.ts:37

const { userEvent } = await import('@testing-library/user-event');

All other test files use the static top-level import (import { userEvent } from '@testing-library/user-event'). This inconsistency is a style issue but can confuse readers about whether there's a deliberate reason for the dynamic import.

LGTM

Debounce test using vi.useFakeTimers() + vi.advanceTimersByTime(300) + vi.runAllTimersAsync() is exactly right. Covers both "no fetch before debounce window" and "exactly one fetch after" in one clean test.
Optimistic revert test is solid — checks the intermediate state (pressed=true) then the reverted state (pressed=false) after the timer fires.
aria-pressed state used as the assertion target rather than CSS class — correct approach.

## 🔬 QA Engineer — Test Coverage Review **Verdict: ⚠️ Approved with concerns** Strong unit test foundation — 221 tests green, good use of fake timers for debounce, optimistic revert is covered. A few coverage gaps worth addressing. --- ### Missing Test Cases **1. `StaplesManager` — no test for empty `categories` array** The component renders `{#each categories as category}` — if an empty array is passed, nothing renders. This is probably fine (it won't crash) but the edge case is untested. Given that the load function returns `[]` when the API fails (via `?? []`), this is reachable in prod. ```ts it('renders without crashing when categories is empty', () => { render(StaplesManager, { props: { categories: [], context: 'onboarding' } }); expect(screen.queryByRole('button')).not.toBeInTheDocument(); }); ``` **2. `page.server.ts` — no test for API failure during load** The load function uses `?? []` as fallback when either API call fails (`categoriesResult.data ?? []`), but this path is completely untested. If `/v1/ingredient-categories` returns an error, categories silently becomes empty — the user sees a blank page with no feedback. ```ts it('returns empty categories when API fails', async () => { mockGet.mockResolvedValue({ data: undefined, error: { status: 500 } }); const result = await load({ fetch: vi.fn() } as any); expect(result.categories).toEqual([]); }); ``` **3. `+server.ts` — no test for missing `isStaple` in body** The handler validates `id` but not `isStaple`. There's no test asserting what happens when `isStaple` is absent. Given the backend engineer flagged this as a gap, there's a corresponding missing test: ```ts it('returns 400 when isStaple is missing', async () => { const response = await PATCH(createRequest({ id: 'ing-1' })); expect(response.status).toBe(400); expect(mockPatch).not.toHaveBeenCalled(); }); ``` **4. `page.test.ts` — the `ctx` bug means these tests don't verify real SvelteKit behaviour** The onboarding tests pass `ctx` as a direct prop, which works in jsdom but is not how SvelteKit delivers this value in production. All 6 onboarding tests are testing a prop-passing mechanism that doesn't exist at runtime. Once the `ctx` prop is fixed (either via load function or `$app/state`), the tests need updating to test the actual mechanism. --- ### Minor Issues **5. Dynamic import in `CategorySection.test.ts:37`** ```ts const { userEvent } = await import('@testing-library/user-event'); ``` All other test files use the static top-level import (`import { userEvent } from '@testing-library/user-event'`). This inconsistency is a style issue but can confuse readers about whether there's a deliberate reason for the dynamic import. --- ### LGTM - Debounce test using `vi.useFakeTimers()` + `vi.advanceTimersByTime(300)` + `vi.runAllTimersAsync()` is exactly right. Covers both "no fetch before debounce window" and "exactly one fetch after" in one clean test. - Optimistic revert test is solid — checks the intermediate state (pressed=true) then the reverted state (pressed=false) after the timer fires. - `aria-pressed` state used as the assertion target rather than CSS class — correct approach.

marcel commented

2026-04-02 20:26:18 +02:00

🔒 Sable — Security Engineer

Verdict: ⚠️ Approved with concerns

No XSS, no CSRF gaps I can see, no secrets in code. One authorization concern that needs clarification before merge.

Concerns

1. Authorization on PATCH /household/staples — who can flip isStaple on any ingredient?

+server.ts proxies to PATCH /v1/ingredients/{id}. Ingredients appear to be global in this schema (not household-scoped — IngredientCategoryResponse has no household ID, and searchIngredients has no household filter). That means:

Any authenticated user who knows an ingredient UUID can flip isStaple for every household in the system, not just their own.
A read-only household member (role: member) can call this endpoint and mutate global data.

This needs a backend confirmation: does PATCH /v1/ingredients/{id} enforce that only users with admin or planner roles can call it? If not, this is a broken access control issue (OWASP A01).

The SvelteKit hooks.server.ts validates that a session exists, but that's authentication only — role-based authorization for this specific mutation needs to be enforced at the backend.

Action required: Confirm backend enforces role check on PATCH /v1/ingredients/{id}. If it doesn't, add a role guard in +server.ts:

if (locals.user?.rolle !== 'planer') {
  return json({ error: 'Forbidden' }, { status: 403 });
}

2. isStaple not type-checked before forwarding

body.isStaple is passed directly to the backend without validation. A crafted request with isStaple: "__proto__" or isStaple: null won't cause injection in this context (it's a typed API client with a JSON body), but it will produce unexpected backend behaviour. Add typeof isStaple !== 'boolean' check (see backend review).

LGTM

No {@html} usage anywhere — XSS surface is zero in this PR.
apiClient(fetch) threads the SvelteKit fetch so the session cookie is forwarded — the backend session validation will fire on every proxied request. ✓
The PATCH endpoint uses request.json() not request.text() / eval() — no injection vector there.
No secrets, credentials, or internal paths in any of the new files. ✓
SvelteKit's built-in CSRF protection covers +server.ts routes for same-origin requests. The Content-Type: application/json header sent by StaplesManager also bypasses the old CSRF-via-form-encoding vector. ✓

## 🔒 Sable — Security Engineer **Verdict: ⚠️ Approved with concerns** No XSS, no CSRF gaps I can see, no secrets in code. One authorization concern that needs clarification before merge. --- ### Concerns **1. Authorization on `PATCH /household/staples` — who can flip `isStaple` on any ingredient?** `+server.ts` proxies to `PATCH /v1/ingredients/{id}`. Ingredients appear to be **global** in this schema (not household-scoped — `IngredientCategoryResponse` has no household ID, and `searchIngredients` has no household filter). That means: - Any authenticated user who knows an ingredient UUID can flip `isStaple` for **every household in the system**, not just their own. - A read-only household member (role: `member`) can call this endpoint and mutate global data. This needs a backend confirmation: does `PATCH /v1/ingredients/{id}` enforce that only users with admin or planner roles can call it? If not, this is a broken access control issue (OWASP A01). The SvelteKit `hooks.server.ts` validates that a session exists, but that's authentication only — role-based authorization for this specific mutation needs to be enforced at the backend. **Action required:** Confirm backend enforces role check on `PATCH /v1/ingredients/{id}`. If it doesn't, add a role guard in `+server.ts`: ```ts if (locals.user?.rolle !== 'planer') { return json({ error: 'Forbidden' }, { status: 403 }); } ``` --- **2. `isStaple` not type-checked before forwarding** `body.isStaple` is passed directly to the backend without validation. A crafted request with `isStaple: "__proto__"` or `isStaple: null` won't cause injection in this context (it's a typed API client with a JSON body), but it will produce unexpected backend behaviour. Add `typeof isStaple !== 'boolean'` check (see backend review). --- ### LGTM - No `{@html}` usage anywhere — XSS surface is zero in this PR. - `apiClient(fetch)` threads the SvelteKit `fetch` so the session cookie is forwarded — the backend session validation will fire on every proxied request. ✓ - The PATCH endpoint uses `request.json()` not `request.text()` / `eval()` — no injection vector there. - No secrets, credentials, or internal paths in any of the new files. ✓ - SvelteKit's built-in CSRF protection covers `+server.ts` routes for same-origin requests. The `Content-Type: application/json` header sent by `StaplesManager` also bypasses the old CSRF-via-form-encoding vector. ✓

marcel commented

2026-04-02 20:26:43 +02:00

🎨 Atlas — UI/UX Designer

Verdict: 🚫 Changes requested

The chip selected state colours are correct and the category eyebrow pattern matches the design system. But there are three violations of the design system spec that need fixing before this ships.

Blockers

1. StapleChip — chip text size is 12px, spec requires 13px

class="inline-flex text-[12px] font-medium px-[12px] py-[6px] ..."

Design system constraint: "Buttons: 13px, weight 500, tracking 0.04em, font-sans — always." A chip that toggles state is functionally a toggle button — this rule applies. Fix:

text-[12px] → text-[13px]
Add tracking-[0.04em]
Add font-sans (currently relying on inheritance, which will break if a parent sets a display font)

Corrected class string:

text-[13px] font-medium tracking-[0.04em] font-sans px-[12px] py-[6px] rounded-full border cursor-pointer ...

2. +page.svelte — "Weiter" and "Überspringen" navigation links are completely unstyled

<a href="/planner">Überspringen</a>
<a href="/household/invite">Weiter</a>

These are raw <a> tags with zero Tailwind classes. They will render with browser-default link styling (blue underline, system font). This is not acceptable for any element the user interacts with in the onboarding flow.

At minimum these need:

font-sans text-[13px] font-medium tracking-[0.04em] (button text spec)
Appropriate colour tokens:
- "Weiter" (primary action): text-[var(--green-dark)] bg-[var(--green-tint)] with padding and border-radius
- "Überspringen" (secondary/ghost): text-[var(--color-text-muted)]

The exact visual treatment depends on the A3 spec — if one doesn't exist yet, match the pattern used for the "Weiter" button on the household setup page (A2).

3. +page.svelte settings branch — <h1> has no typography styling

<h1>Vorräte</h1>

This will render with browser-default h1 sizing (2em, Times New Roman on some browsers). Add the display heading pattern from the rest of the app. Based on the household setup form heading: font-[var(--font-display)] text-[18px] md:text-[28px] font-medium text-[var(--color-text)].

Suggestions

The chip selected state (--green-tint bg, --green-light border, --green-dark text) matches the established active state pattern. ✓
The category eyebrow at text-[10px] font-medium tracking-[.08em] uppercase text-[var(--color-text-muted)] is consistent with the pattern used in other eyebrow labels across the app. ✓
rounded-full on chips is the right choice for pill-shaped ingredients. ✓
The focus-visible:outline-2 focus-visible:outline-offset-2 focus ring is correct WCAG 2.2 pattern. ✓

## 🎨 Atlas — UI/UX Designer **Verdict: 🚫 Changes requested** The chip selected state colours are correct and the category eyebrow pattern matches the design system. But there are three violations of the design system spec that need fixing before this ships. --- ### Blockers **1. `StapleChip` — chip text size is 12px, spec requires 13px** ```svelte class="inline-flex text-[12px] font-medium px-[12px] py-[6px] ..." ``` Design system constraint: *"Buttons: 13px, weight 500, tracking 0.04em, font-sans — always."* A chip that toggles state is functionally a toggle button — this rule applies. Fix: - `text-[12px]` → `text-[13px]` - Add `tracking-[0.04em]` - Add `font-sans` (currently relying on inheritance, which will break if a parent sets a display font) Corrected class string: ``` text-[13px] font-medium tracking-[0.04em] font-sans px-[12px] py-[6px] rounded-full border cursor-pointer ... ``` --- **2. `+page.svelte` — "Weiter" and "Überspringen" navigation links are completely unstyled** ```svelte <a href="/planner">Überspringen</a> <a href="/household/invite">Weiter</a> ``` These are raw `<a>` tags with zero Tailwind classes. They will render with browser-default link styling (blue underline, system font). This is not acceptable for any element the user interacts with in the onboarding flow. At minimum these need: - `font-sans text-[13px] font-medium tracking-[0.04em]` (button text spec) - Appropriate colour tokens: - "Weiter" (primary action): `text-[var(--green-dark)] bg-[var(--green-tint)]` with padding and border-radius - "Überspringen" (secondary/ghost): `text-[var(--color-text-muted)]` The exact visual treatment depends on the A3 spec — if one doesn't exist yet, match the pattern used for the "Weiter" button on the household setup page (A2). --- **3. `+page.svelte` settings branch — `<h1>` has no typography styling** ```svelte <h1>Vorräte</h1> ``` This will render with browser-default h1 sizing (2em, Times New Roman on some browsers). Add the display heading pattern from the rest of the app. Based on the household setup form heading: `font-[var(--font-display)] text-[18px] md:text-[28px] font-medium text-[var(--color-text)]`. --- ### Suggestions - The chip selected state (`--green-tint` bg, `--green-light` border, `--green-dark` text) matches the established active state pattern. ✓ - The category eyebrow at `text-[10px] font-medium tracking-[.08em] uppercase text-[var(--color-text-muted)]` is consistent with the pattern used in other eyebrow labels across the app. ✓ - `rounded-full` on chips is the right choice for pill-shaped ingredients. ✓ - The `focus-visible:outline-2 focus-visible:outline-offset-2` focus ring is correct WCAG 2.2 pattern. ✓

marcel added 9 commits 2026-04-03 09:31:00 +02:00

test(staples): add empty categories edge case to StaplesManager 7b497be1c1

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

test(staples): cover API failure fallback in page load 65f18cfb43

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

fix(staples): validate isStaple is boolean before forwarding to backend 21b873b85b

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

fix(staples): forward backend error status code instead of always 500 3581af2bf9

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

fix(staples): add role guard — only planer role can toggle staples 45b7e7b003

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

fix(staples): pass ctx from URL through load function; fix script order in page 8daaa0e21d

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

fix(staples): apply design-system button spec to StapleChip (13px, tracking, font-sans) 73b33ee956

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

fix(staples): apply design-system styles to nav links and settings heading 2d6ddf0e48

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

refactor(staples): convert dynamic userEvent import to static in CategorySection test df95462094

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

marcel commented

2026-04-03 09:31:24 +02:00

Review feedback addressed

All reviewer concerns resolved in 9 commits on this branch. 230 tests green, 0 type errors.

#	Concern	Fix	Commit
1	QA: no test for empty categories	Added edge-case test to `StaplesManager.test.ts`	`7b497be`
2	QA: no test for API failure in load	Added test covering `?? []` fallback path	`65f18cf`
3	Backend + QA + Sable: `isStaple` not validated	Added `typeof isStaple !== 'boolean'` check → 400; added two new tests	`21b873b`
4	Backend: backend error status always 500	Forward backend status via `(error as { status? }).status ?? 500`; added 404-forwarding test	`3581af2`
5	Sable: no role guard on PATCH proxy	Added `locals.benutzer?.rolle !== 'planer'` → 403; added test	`45b7e7b`
6	Kai: `ctx` prop never set by SvelteKit; `<svelte:head>` before `<script>`	Load function now reads `url.searchParams.get('ctx')` and returns it in data; page reads `data.ctx`; script moved first; page + load tests updated	`8daaa0e`
7	Atlas: StapleChip 12px text, missing tracking + font-sans	`text-[13px]`, `tracking-[0.04em]`, `font-sans` added; test added	`73b33ee`
8	Atlas: unstyled nav links and `<h1>`	Weiter → green-dark button; Überspringen → muted ghost link; h1 → display font tokens	`2d6ddf0`
9	QA: dynamic import in CategorySection.test.ts	Converted to static top-level import	`df95462`

🤖 Generated with Claude Code

## Review feedback addressed All reviewer concerns resolved in 9 commits on this branch. 230 tests green, 0 type errors. | # | Concern | Fix | Commit | |---|---------|-----|--------| | 1 | QA: no test for empty categories | Added edge-case test to `StaplesManager.test.ts` | `7b497be` | | 2 | QA: no test for API failure in load | Added test covering `?? []` fallback path | `65f18cf` | | 3 | Backend + QA + Sable: `isStaple` not validated | Added `typeof isStaple !== 'boolean'` check → 400; added two new tests | `21b873b` | | 4 | Backend: backend error status always 500 | Forward backend status via `(error as { status? }).status ?? 500`; added 404-forwarding test | `3581af2` | | 5 | Sable: no role guard on PATCH proxy | Added `locals.benutzer?.rolle !== 'planer'` → 403; added test | `45b7e7b` | | 6 | Kai: `ctx` prop never set by SvelteKit; `<svelte:head>` before `<script>` | Load function now reads `url.searchParams.get('ctx')` and returns it in data; page reads `data.ctx`; script moved first; page + load tests updated | `8daaa0e` | | 7 | Atlas: StapleChip 12px text, missing tracking + font-sans | `text-[13px]`, `tracking-[0.04em]`, `font-sans` added; test added | `73b33ee` | | 8 | Atlas: unstyled nav links and `<h1>` | Weiter → green-dark button; Überspringen → muted ghost link; h1 → display font tokens | `2d6ddf0` | | 9 | QA: dynamic import in CategorySection.test.ts | Converted to static top-level import | `df95462` | 🤖 Generated with [Claude Code](https://claude.com/claude-code)

marcel merged commit 021d308a71 into master

2026-04-03 09:35:03 +02:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/mealprep#35