feat(onboarding): A2 — Household setup page #34

marcel · 2026-04-02T19:22:30+02:00

marcel commented

2026-04-02 19:22:30 +02:00

Summary

Implements issue #19 — screen A2 of the J6 household setup onboarding flow.

ProgressSidebar shared component (3 steps, active/completed/future states, aria-labels, reusable for A3+)
HouseholdSetupForm component with disabled-until-valid Continue button and server error display
+page.server.ts load guard (redirects to /planner if user already has a household) + POST action calling POST /v1/households → redirect 303 /household/staples
+page.svelte — desktop: 300px progress sidebar + flex form area (max-width 420px); mobile: "Schritt 1 von 3" eyebrow + form
Stub /household/staples page as redirect target for A3

Deferred: Invite link generation — per team resolution, not A2's concern (subsequent page).

Test plan

183 unit tests pass (npm run test)
0 type errors (npm run check)
Sign up as new user → redirected to /household/setup
Continue button disabled with empty name, enabled when name filled
Submit creates household and redirects to /household/staples
User already with household → load guard redirects to /planner
Desktop: progress sidebar visible with step 1 active
Mobile: sidebar hidden, "Schritt 1 von 3" eyebrow visible

🤖 Generated with Claude Code

## Summary Implements issue #19 — screen A2 of the J6 household setup onboarding flow. - `ProgressSidebar` shared component (3 steps, active/completed/future states, aria-labels, reusable for A3+) - `HouseholdSetupForm` component with disabled-until-valid Continue button and server error display - `+page.server.ts` load guard (redirects to `/planner` if user already has a household) + POST action calling `POST /v1/households` → redirect `303 /household/staples` - `+page.svelte` — desktop: 300px progress sidebar + flex form area (max-width 420px); mobile: "Schritt 1 von 3" eyebrow + form - Stub `/household/staples` page as redirect target for A3 **Deferred:** Invite link generation — per team resolution, not A2's concern (subsequent page). ## Test plan - [ ] 183 unit tests pass (`npm run test`) - [ ] 0 type errors (`npm run check`) - [ ] Sign up as new user → redirected to `/household/setup` - [ ] Continue button disabled with empty name, enabled when name filled - [ ] Submit creates household and redirects to `/household/staples` - [ ] User already with household → load guard redirects to `/planner` - [ ] Desktop: progress sidebar visible with step 1 active - [ ] Mobile: sidebar hidden, "Schritt 1 von 3" eyebrow visible 🤖 Generated with [Claude Code](https://claude.ai/claude-code)

marcel added 4 commits 2026-04-02 19:22:31 +02:00

feat(onboarding): add ProgressSidebar component with 3-step active/completed/future states b9ef06fd73

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

feat(onboarding): add HouseholdSetupForm component with disabled-until-valid continue button 175bfbe7dd

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

feat(onboarding): add household setup page server action and load guard e85a7ca313

Creates household via POST /v1/households, redirects to /household/staples.
Load guard redirects users who already have a household to /planner.

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

feat(onboarding): add A2 household setup page with responsive progress sidebar layout 6de7f5a9b5

Desktop: 300px ProgressSidebar (step 1 active) + flex form area.
Mobile: "Schritt 1 von 3" eyebrow + HouseholdSetupForm.
Also stubs /household/staples as redirect target for A3.

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

marcel commented

2026-04-02 19:24:12 +02:00

🧑‍💻 Kai — Frontend Engineer

Verdict: ⚠️ Approved with concerns

Good TDD discipline, clean Svelte 5 runes usage throughout. A few things I want addressed before merge.

Blockers

1. aria-hidden="true" on <aside> is a test workaround masquerading as production code
+page.svelte:21 — The sidebar is marked aria-hidden="true" to prevent duplicate text matches in JSDOM because both the sidebar and the form say "Haushalt benennen". This is the wrong fix. The <aside> is semantically meaningful content (progress indicator with accessible step labels) — hiding it from screen readers silently removes functionality.

The correct fix is to update the test query to be more specific:

// Instead of: screen.getByText('Haushalt benennen')
// Use the heading role which is unique to the form:
screen.getByRole('heading', { name: 'Haushalt benennen' })

Remove aria-hidden="true" from the aside, and remove the defaultIgnore change from test-setup.ts — that's a global side effect that will affect all future tests.

2. Wrong Tailwind syntax for font-family in HouseholdSetupForm.svelte
Line 44: font-['var(--font-display)'] — the quotes around the CSS var are incorrect. Tailwind doesn't need quotes here. It should be font-[var(--font-display)] (same as how it's done correctly in ProgressSidebar.svelte at line 44: font-[var(--font-display)]). With the current syntax the font will not apply.

Suggestions

3. Load guard belongs in hooks, not +page.server.ts
My own rule: auth and session checks live in hooks.server.ts. The "already has household" guard is a routing concern, not page-local business logic. Consider a hasHousehold check in the hook that redirects onboarding users who have already completed setup. That said, this is navigable UX logic so I'll let it slide for now if the team disagrees — but I want the conversation logged.

4. circleClass() and labelClass() in ProgressSidebar.svelte reference currentStep from outer scope inside plain functions
This works because Svelte 5 tracks fine-grained reactivity through template expressions, but it's subtle. $derived would make the intent explicit and surface any future reactivity issues earlier:

const getCircleClass = $derived((n: number) => { ... });

Minor — not blocking.

5. HouseholdSetupForm.svelte heading font size
text-[24px] on line 44, but the spec shows 28px for the desktop form heading. On mobile 24px is fine but on desktop this should scale up. The existing auth forms use responsive md:text-[28px] — same pattern should apply here.

## 🧑‍💻 Kai — Frontend Engineer **Verdict: ⚠️ Approved with concerns** Good TDD discipline, clean Svelte 5 runes usage throughout. A few things I want addressed before merge. --- ### Blockers **1. `aria-hidden="true"` on `<aside>` is a test workaround masquerading as production code** `+page.svelte:21` — The sidebar is marked `aria-hidden="true"` to prevent duplicate text matches in JSDOM because both the sidebar and the form say "Haushalt benennen". This is the wrong fix. The `<aside>` is semantically meaningful content (progress indicator with accessible step labels) — hiding it from screen readers silently removes functionality. The correct fix is to update the test query to be more specific: ```ts // Instead of: screen.getByText('Haushalt benennen') // Use the heading role which is unique to the form: screen.getByRole('heading', { name: 'Haushalt benennen' }) ``` Remove `aria-hidden="true"` from the aside, and remove the `defaultIgnore` change from `test-setup.ts` — that's a global side effect that will affect all future tests. **2. Wrong Tailwind syntax for font-family in `HouseholdSetupForm.svelte`** Line 44: `font-['var(--font-display)']` — the quotes around the CSS var are incorrect. Tailwind doesn't need quotes here. It should be `font-[var(--font-display)]` (same as how it's done correctly in `ProgressSidebar.svelte` at line 44: `font-[var(--font-display)]`). With the current syntax the font will not apply. --- ### Suggestions **3. Load guard belongs in hooks, not `+page.server.ts`** My own rule: auth and session checks live in `hooks.server.ts`. The "already has household" guard is a routing concern, not page-local business logic. Consider a `hasHousehold` check in the hook that redirects onboarding users who have already completed setup. That said, this is navigable UX logic so I'll let it slide for now if the team disagrees — but I want the conversation logged. **4. `circleClass()` and `labelClass()` in `ProgressSidebar.svelte` reference `currentStep` from outer scope inside plain functions** This works because Svelte 5 tracks fine-grained reactivity through template expressions, but it's subtle. `$derived` would make the intent explicit and surface any future reactivity issues earlier: ```ts const getCircleClass = $derived((n: number) => { ... }); ``` Minor — not blocking. **5. `HouseholdSetupForm.svelte` heading font size** `text-[24px]` on line 44, but the spec shows `28px` for the desktop form heading. On mobile `24px` is fine but on desktop this should scale up. The existing auth forms use responsive `md:text-[28px]` — same pattern should apply here.

marcel commented

2026-04-02 19:24:34 +02:00

🎨 Atlas — UI/UX Designer

Verdict: ⚠️ Approved with concerns

Layout structure and token usage are mostly right. Two things need fixing before this ships visually.

Blockers

1. aria-hidden="true" on the progress sidebar — accessibility violation
+page.svelte:21 — The ProgressSidebar has properly labelled steps (aria-label="Schritt 1", aria-current="step") that I specifically designed for screen reader users. Wrapping the entire <aside> in aria-hidden throws all of that away. Users on VoiceOver or NVDA will land on the form with no indication of where they are in the onboarding flow. This is a WCAG 2.2 AA failure (Success Criterion 1.3.1 — Info and Relationships). The sidebar must be accessible.

Fix the test query instead — getByRole('heading', { name: ... }) is unambiguous.

2. Wrong font-family syntax — heading will render in system font
HouseholdSetupForm.svelte:44 — font-['var(--font-display)'] has extraneous quotes. Tailwind's arbitrary-value syntax doesn't quote CSS variable references. The correct class is font-[var(--font-display)]. Without this fix the h1 renders in DM Sans (body font) instead of Fraunces, which breaks the visual hierarchy on this screen.

Suggestions

3. Heading font size doesn't scale for desktop
HouseholdSetupForm.svelte:44 — text-[24px] is flat across all breakpoints. The spec shows 28px for the desktop form heading. The auth forms (SignupForm.svelte) already use the correct responsive pattern: text-[18px] md:text-[28px]. Apply the same here.

4. Heading font-weight should be 500, not 600
font-semibold = 600, which is my stated ceiling. For display headings in Fraunces the spec uses font-weight: 500 (font-medium). 600 is technically within the limit but it looks heavier than intended — Fraunces at 600 has noticeable slab-serif boldness that doesn't fit the calm onboarding tone. Change to font-medium.

5. Form subtitle text is missing responsive sizing
HouseholdSetupForm.svelte:46 — The subtitle uses text-[14px] flat, which is fine for desktop but tight on mobile. The auth forms use text-[12px] md:text-[14px]. Low priority but worth aligning for consistency.

6. ProgressSidebar subtitle text for step 1 is a bit literal
ProgressSidebar.svelte:7 — "Deiner Familie einen Namen geben" reads slightly awkward as a subtitle. The spec shows no subtitle text beyond the step label itself for future steps. Suggest either trimming the subtitles or aligning them with the spec copy: just "Smith family" (the household name) for completed steps, and nothing for future steps. Not blocking — leave as-is if the copy has been signed off.

## 🎨 Atlas — UI/UX Designer **Verdict: ⚠️ Approved with concerns** Layout structure and token usage are mostly right. Two things need fixing before this ships visually. --- ### Blockers **1. `aria-hidden="true"` on the progress sidebar — accessibility violation** `+page.svelte:21` — The ProgressSidebar has properly labelled steps (`aria-label="Schritt 1"`, `aria-current="step"`) that I specifically designed for screen reader users. Wrapping the entire `<aside>` in `aria-hidden` throws all of that away. Users on VoiceOver or NVDA will land on the form with no indication of where they are in the onboarding flow. This is a WCAG 2.2 AA failure (Success Criterion 1.3.1 — Info and Relationships). The sidebar must be accessible. Fix the test query instead — `getByRole('heading', { name: ... })` is unambiguous. **2. Wrong font-family syntax — heading will render in system font** `HouseholdSetupForm.svelte:44` — `font-['var(--font-display)']` has extraneous quotes. Tailwind's arbitrary-value syntax doesn't quote CSS variable references. The correct class is `font-[var(--font-display)]`. Without this fix the `h1` renders in DM Sans (body font) instead of Fraunces, which breaks the visual hierarchy on this screen. --- ### Suggestions **3. Heading font size doesn't scale for desktop** `HouseholdSetupForm.svelte:44` — `text-[24px]` is flat across all breakpoints. The spec shows `28px` for the desktop form heading. The auth forms (`SignupForm.svelte`) already use the correct responsive pattern: `text-[18px] md:text-[28px]`. Apply the same here. **4. Heading font-weight should be 500, not 600** `font-semibold` = 600, which is my stated ceiling. For display headings in Fraunces the spec uses `font-weight: 500` (`font-medium`). 600 is technically within the limit but it looks heavier than intended — Fraunces at 600 has noticeable slab-serif boldness that doesn't fit the calm onboarding tone. Change to `font-medium`. **5. Form subtitle text is missing responsive sizing** `HouseholdSetupForm.svelte:46` — The subtitle uses `text-[14px]` flat, which is fine for desktop but tight on mobile. The auth forms use `text-[12px] md:text-[14px]`. Low priority but worth aligning for consistency. **6. ProgressSidebar subtitle text for step 1 is a bit literal** `ProgressSidebar.svelte:7` — "Deiner Familie einen Namen geben" reads slightly awkward as a subtitle. The spec shows no subtitle text beyond the step label itself for future steps. Suggest either trimming the subtitles or aligning them with the spec copy: just "Smith family" (the household name) for completed steps, and nothing for future steps. Not blocking — leave as-is if the copy has been signed off.

marcel commented

2026-04-02 19:24:58 +02:00

🔒 Sable — Security Engineer

Verdict: ✅ Approved

Clean implementation for an authenticated onboarding screen. No significant vulnerabilities. Notes below.

Observations

Auth gate is solid
/household/setup is not in PUBLIC_ROUTES in hooks.server.ts, so hooks.server.ts enforces session validity and calls /v1/auth/me before this page is ever reached. Unauthenticated users are redirected to /login before +page.server.ts load or action ever runs. ✅

CSRF coverage
The form uses use:enhance, which relies on SvelteKit's built-in Origin header check for CSRF protection on form actions. No custom CSRF token needed. ✅

No XSS vectors
formError, error, and all user-supplied values are rendered via Svelte template interpolation ({value}), which auto-escapes. No {@html} usage anywhere in this PR. ✅

Load guard correctness
+page.server.ts:6 — locals.haushalt?.id uses optional chaining, so it handles the edge case where haushalt is undefined (e.g., if the hooks call somehow set no household). Redirects to /planner for users who already completed setup. ✅

Suggestions (non-blocking)

1. fail(500, ...) leaks internal status to the client
+page.server.ts:26 — Returning fail(500, ...) sends HTTP 500 back to the browser on a backend error. The error message is generic ("Haushalt konnte nicht erstellt werden...") which is fine, but the 500 status code itself can be fingerprinted by automated scanners. Consider fail(503, ...) (Service Unavailable) or fail(400, ...) with a generic user-facing message — both are less informative to an attacker about whether the backend is down vs. rejecting the request. Low severity.

2. No household name length validation on the server action
+page.server.ts:15-17 — The action only checks that the name is non-empty. If the backend enforces a max length (e.g., 100 chars), a user could submit a 10,000-character name and the action would forward it to the API. Add a name.length > 100 guard (or whatever the backend constraint is) to fail fast before the API call and return a clear error. Defense in depth.

3. Invite token scope reminder (out of scope for this PR)
The load guard prevents users with an existing household from reaching A2 again, which closes off the "create a second household by replaying the request" vector. When invite generation is implemented, ensure the backend enforces UNIQUE(user_account_id) on household_member as a backstop. Noted for the A3/invite issue.

## 🔒 Sable — Security Engineer **Verdict: ✅ Approved** Clean implementation for an authenticated onboarding screen. No significant vulnerabilities. Notes below. --- ### Observations **Auth gate is solid** `/household/setup` is not in `PUBLIC_ROUTES` in `hooks.server.ts`, so `hooks.server.ts` enforces session validity and calls `/v1/auth/me` before this page is ever reached. Unauthenticated users are redirected to `/login` before `+page.server.ts` load or action ever runs. ✅ **CSRF coverage** The form uses `use:enhance`, which relies on SvelteKit's built-in Origin header check for CSRF protection on form actions. No custom CSRF token needed. ✅ **No XSS vectors** `formError`, `error`, and all user-supplied values are rendered via Svelte template interpolation (`{value}`), which auto-escapes. No `{@html}` usage anywhere in this PR. ✅ **Load guard correctness** `+page.server.ts:6` — `locals.haushalt?.id` uses optional chaining, so it handles the edge case where `haushalt` is undefined (e.g., if the hooks call somehow set no household). Redirects to `/planner` for users who already completed setup. ✅ --- ### Suggestions (non-blocking) **1. `fail(500, ...)` leaks internal status to the client** `+page.server.ts:26` — Returning `fail(500, ...)` sends HTTP 500 back to the browser on a backend error. The error message is generic ("Haushalt konnte nicht erstellt werden...") which is fine, but the 500 status code itself can be fingerprinted by automated scanners. Consider `fail(503, ...)` (Service Unavailable) or `fail(400, ...)` with a generic user-facing message — both are less informative to an attacker about whether the backend is down vs. rejecting the request. Low severity. **2. No household name length validation on the server action** `+page.server.ts:15-17` — The action only checks that the name is non-empty. If the backend enforces a max length (e.g., 100 chars), a user could submit a 10,000-character name and the action would forward it to the API. Add a `name.length > 100` guard (or whatever the backend constraint is) to fail fast before the API call and return a clear error. Defense in depth. **3. Invite token scope reminder (out of scope for this PR)** The load guard prevents users with an existing household from reaching A2 again, which closes off the "create a second household by replaying the request" vector. When invite generation is implemented, ensure the backend enforces `UNIQUE(user_account_id)` on `household_member` as a backstop. Noted for the A3/invite issue.

marcel commented

2026-04-02 19:25:25 +02:00

🧪 QA Engineer — Test Coverage

Verdict: ⚠️ Approved with concerns

Good unit coverage on all new components and the server action. One structural issue with the test setup, and a few gaps worth closing.

Blockers

1. Global defaultIgnore change in test-setup.ts is a fragile workaround
The change to exclude [aria-hidden="true"] * from text queries was added to solve a duplicate-text problem caused by the sidebar and form sharing the heading "Haushalt benennen". This is a leaky fix — it modifies query behavior globally for all 183 tests going forward, and it papers over a semantic problem in the production code (aria-hidden on meaningful content).

The correct fix is to remove aria-hidden from the <aside>, drop the defaultIgnore change, and update the test query in page.test.ts to be specific:

// page.test.ts - be role-specific so there's no duplicate ambiguity
screen.getByRole('heading', { name: 'Haushalt benennen' })

The ProgressSidebar.test.ts tests are unaffected since that component is tested standalone.

Suggestions

2. Missing E2E test for the A1→A2 critical path
The comment thread on issue #19 called out: "E2E — full onboarding flow: Signup (A1) → household setup (A2) → A3 as a single journey test. This is a critical path." No E2E test was added. I understand A3 is a stub, so a full journey isn't possible yet — but a signup → lands on /household/setup E2E test is testable today. Suggest creating a e2e/onboarding.spec.ts with at least that smoke test before merging.

3. No test for maximum name length
page.server.test.ts — The action has no max-length guard (noted by Sable too). There's no test asserting that an excessively long name is rejected. Once the max-length validation is added, the test should cover the boundary: name.length === maxLength passes, name.length === maxLength + 1 fails.

4. page.test.ts — the getByTestId('step-1') test works only because of aria-hidden workaround
page.test.ts:28 — getByTestId doesn't respect defaultIgnore, so it finds the sidebar step regardless of aria-hidden. But if the aria-hidden workaround is removed (as it should be), the getByText('Haushalt benennen') query in the page test will start failing again. Fixing the query to getByRole('heading', ...) is necessary alongside removing the workaround.

5. No test for ProgressSidebar with currentStep=3 (all steps completed)
ProgressSidebar.test.ts — Tests cover currentStep=1 and currentStep=2 but not currentStep=3. When step 3 is active, steps 1 and 2 should be data-state="completed". Worth a quick test to confirm the n < currentStep logic handles the final step correctly.

6. The "shows validation error when submitting with empty name" test comment is misleading
HouseholdSetupForm.test.ts:38 — The comment says "override disabled to allow submit attempt by typing then clearing" but the test never actually clicks a disabled button (disabled buttons don't fire click events). What actually happens is: typing sets touched=true, clearing the field triggers the $derived error. The error appears before the button is clicked. The test works correctly — but the comment should explain the actual mechanism to avoid confusion for future maintainers.

## 🧪 QA Engineer — Test Coverage **Verdict: ⚠️ Approved with concerns** Good unit coverage on all new components and the server action. One structural issue with the test setup, and a few gaps worth closing. --- ### Blockers **1. Global `defaultIgnore` change in `test-setup.ts` is a fragile workaround** The change to exclude `[aria-hidden="true"] *` from text queries was added to solve a duplicate-text problem caused by the sidebar and form sharing the heading "Haushalt benennen". This is a leaky fix — it modifies query behavior globally for all 183 tests going forward, and it papers over a semantic problem in the production code (`aria-hidden` on meaningful content). The correct fix is to remove `aria-hidden` from the `<aside>`, drop the `defaultIgnore` change, and update the test query in `page.test.ts` to be specific: ```ts // page.test.ts - be role-specific so there's no duplicate ambiguity screen.getByRole('heading', { name: 'Haushalt benennen' }) ``` The `ProgressSidebar.test.ts` tests are unaffected since that component is tested standalone. --- ### Suggestions **2. Missing E2E test for the A1→A2 critical path** The comment thread on issue #19 called out: "E2E — full onboarding flow: Signup (A1) → household setup (A2) → A3 as a single journey test. This is a critical path." No E2E test was added. I understand A3 is a stub, so a full journey isn't possible yet — but a `signup → lands on /household/setup` E2E test is testable today. Suggest creating a `e2e/onboarding.spec.ts` with at least that smoke test before merging. **3. No test for maximum name length** `page.server.test.ts` — The action has no max-length guard (noted by Sable too). There's no test asserting that an excessively long name is rejected. Once the max-length validation is added, the test should cover the boundary: `name.length === maxLength` passes, `name.length === maxLength + 1` fails. **4. `page.test.ts` — the `getByTestId('step-1')` test works only because of `aria-hidden` workaround** `page.test.ts:28` — `getByTestId` doesn't respect `defaultIgnore`, so it finds the sidebar step regardless of `aria-hidden`. But if the `aria-hidden` workaround is removed (as it should be), the `getByText('Haushalt benennen')` query in the page test will start failing again. Fixing the query to `getByRole('heading', ...)` is necessary alongside removing the workaround. **5. No test for `ProgressSidebar` with `currentStep=3` (all steps completed)** `ProgressSidebar.test.ts` — Tests cover `currentStep=1` and `currentStep=2` but not `currentStep=3`. When step 3 is active, steps 1 and 2 should be `data-state="completed"`. Worth a quick test to confirm the `n < currentStep` logic handles the final step correctly. **6. The "shows validation error when submitting with empty name" test comment is misleading** `HouseholdSetupForm.test.ts:38` — The comment says "override disabled to allow submit attempt by typing then clearing" but the test never actually clicks a disabled button (disabled buttons don't fire click events). What actually happens is: typing sets `touched=true`, clearing the field triggers the `$derived` error. The error appears before the button is clicked. The test works correctly — but the comment should explain the actual mechanism to avoid confusion for future maintainers.

marcel commented

2026-04-02 19:25:44 +02:00

🗄️ Backend Engineer

Verdict: ✅ Approved

This PR is purely frontend. The backend API (POST /v1/households, POST /v1/households/mine/invites) was already implemented. Reviewing the frontend's usage of those endpoints.

All clear

API call is correct
+page.server.ts:22-24 — api.POST('/v1/households', { body: { name } }) matches the CreateHouseholdRequest schema exactly ({ name: string }). The response is typed via the generated schema. ✅

apiClient(fetch) usage is correct
SvelteKit's fetch is forwarded so session cookies are included in the backend request. Without this, the POST /v1/households endpoint would return 401. ✅

Error check is correct
+page.server.ts:26 — Checking both error and !data?.data before proceeding. The API client returns { data, error } and either can be falsy without the other — checking both is the right pattern. ✅

Suggestions (non-blocking)

1. No max-length validation before the API call
The frontend action validates that the name is non-empty but not that it's within a reasonable length. If the backend enforces a VARCHAR(100) constraint (or similar), a 10,000-character submission will get a backend error, which shows the generic "couldn't create" message instead of a helpful "name too long" message. Add a name.length > 100 guard with a German-language validation message to match the backend constraint and give the user actionable feedback.

2. The response key from mockSuccess() in tests is unused
page.server.test.ts:68 — mockSuccess() includes a response object with a headers.get mock, matching the signup action pattern. But the household setup action doesn't read response.headers (no session cookie to extract — the user is already authenticated). The mock is harmless, just unnecessary noise. Minor.

## 🗄️ Backend Engineer **Verdict: ✅ Approved** This PR is purely frontend. The backend API (`POST /v1/households`, `POST /v1/households/mine/invites`) was already implemented. Reviewing the frontend's usage of those endpoints. --- ### All clear **API call is correct** `+page.server.ts:22-24` — `api.POST('/v1/households', { body: { name } })` matches the `CreateHouseholdRequest` schema exactly (`{ name: string }`). The response is typed via the generated schema. ✅ **`apiClient(fetch)` usage is correct** SvelteKit's `fetch` is forwarded so session cookies are included in the backend request. Without this, the `POST /v1/households` endpoint would return 401. ✅ **Error check is correct** `+page.server.ts:26` — Checking both `error` and `!data?.data` before proceeding. The API client returns `{ data, error }` and either can be falsy without the other — checking both is the right pattern. ✅ --- ### Suggestions (non-blocking) **1. No max-length validation before the API call** The frontend action validates that the name is non-empty but not that it's within a reasonable length. If the backend enforces a `VARCHAR(100)` constraint (or similar), a 10,000-character submission will get a backend error, which shows the generic "couldn't create" message instead of a helpful "name too long" message. Add a `name.length > 100` guard with a German-language validation message to match the backend constraint and give the user actionable feedback. **2. The `response` key from `mockSuccess()` in tests is unused** `page.server.test.ts:68` — `mockSuccess()` includes a `response` object with a `headers.get` mock, matching the signup action pattern. But the household setup action doesn't read `response.headers` (no session cookie to extract — the user is already authenticated). The mock is harmless, just unnecessary noise. Minor.

marcel added 7 commits 2026-04-02 19:33:20 +02:00

refactor(onboarding): remove aria-hidden workaround from progress sidebar e5614ccf30

Replace getByText with getByRole(heading) in page test to disambiguate
the duplicate "Haushalt benennen" text between sidebar and form.
Revert defaultIgnore change in test-setup.ts.

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

fix(onboarding): correct Tailwind arbitrary font-family syntax in HouseholdSetupForm 66525484a6

font-['var(--font-display)'] → font-[var(--font-display)] so Fraunces
display font is applied correctly to the h1.

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

fix(onboarding): make HouseholdSetupForm heading responsive and use font-medium 36dfea34cc

text-[18px] md:text-[28px] matches auth form pattern.
font-medium (500) replaces font-semibold (600) per design system rules.

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

fix(onboarding): make HouseholdSetupForm subtitle responsive (12px mobile, 14px desktop) 3742364956

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

feat(onboarding): add max-length validation for household name (100 chars) 2d1604492d

Fails fast before the API call with a clear German error message.
Tests boundary: 100 chars accepted, 101 rejected.

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

test(onboarding): add ProgressSidebar test for currentStep=3 (all prior steps completed) 01a321caa9

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

refactor(onboarding): clarify test comment and remove unused response mock 7c66dcad3a

HouseholdSetupForm.test.ts: explain that touched+empty drives the $derived
error, not a submit event on the disabled button.
page.server.test.ts: remove unused response key from mockSuccess() —
household creation doesn't set a session cookie.

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

marcel commented

2026-04-02 19:33:34 +02:00

✅ Review concerns addressed — 7 commits

All blockers and suggestions from the 5-persona review have been resolved.

Blockers fixed

Concern	Commit	Fix
`aria-hidden="true"` on progress sidebar (Kai, Atlas, QA)	`e5614cc`	Removed `aria-hidden`, reverted `test-setup.ts` `defaultIgnore`, updated `page.test.ts` to use `getByRole('heading', ...)`
Wrong Tailwind font-family syntax `font-['var(--font-display)']` (Kai, Atlas)	`6652548`	Fixed to `font-[var(--font-display)]` — Fraunces now applies correctly

Suggestions implemented

Concern	Commit	Fix
Heading not responsive, font-weight 600 (Kai, Atlas)	`36dfea3`	`text-[18px] md:text-[28px]`, `font-semibold` → `font-medium`
Subtitle not responsive (Atlas)	`3742364`	`text-[12px] md:text-[14px]`
No max-length validation on name (Sable, Backend, QA)	`2d16044`	Added `name.length > 100` guard, 2 new tests (boundary + over-limit)
Missing `currentStep=3` test for ProgressSidebar (QA)	`01a321c`	Added — confirms steps 1+2 both show `data-state="completed"`
Misleading test comment + unused `response` mock (QA, Backend)	`7c66dca`	Comment clarified, `response` key removed from `mockSuccess()`

Stats

Tests: 183 → 186 (+3)
Type errors: 0

## ✅ Review concerns addressed — 7 commits All blockers and suggestions from the 5-persona review have been resolved. ### Blockers fixed | Concern | Commit | Fix | |---------|--------|-----| | `aria-hidden="true"` on progress sidebar (Kai, Atlas, QA) | `e5614cc` | Removed `aria-hidden`, reverted `test-setup.ts` `defaultIgnore`, updated `page.test.ts` to use `getByRole('heading', ...)` | | Wrong Tailwind font-family syntax `font-['var(--font-display)']` (Kai, Atlas) | `6652548` | Fixed to `font-[var(--font-display)]` — Fraunces now applies correctly | ### Suggestions implemented | Concern | Commit | Fix | |---------|--------|-----| | Heading not responsive, font-weight 600 (Kai, Atlas) | `36dfea3` | `text-[18px] md:text-[28px]`, `font-semibold` → `font-medium` | | Subtitle not responsive (Atlas) | `3742364` | `text-[12px] md:text-[14px]` | | No max-length validation on name (Sable, Backend, QA) | `2d16044` | Added `name.length > 100` guard, 2 new tests (boundary + over-limit) | | Missing `currentStep=3` test for ProgressSidebar (QA) | `01a321c` | Added — confirms steps 1+2 both show `data-state="completed"` | | Misleading test comment + unused `response` mock (QA, Backend) | `7c66dca` | Comment clarified, `response` key removed from `mockSuccess()` | ### Stats - Tests: 183 → 186 (+3) - Type errors: 0

marcel merged commit 7c66dcad3a into master

2026-04-02 19:39:09 +02:00

marcel deleted branch feat/issue-19-household-setup

2026-04-02 19:39:10 +02:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/mealprep#34