Frontend: design system, navigation, auth guard, signup screen #33
Reference in New Issue
Block a user
Delete Branch "feat/issue-16-design-system"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
@themetokens (colors, spacing, radii, typography, elevation), WCAG AA contrast testshooks.server.tswith session validation, public route allowlist, redirect preservation, household fallback/v1/auth/signup, no-nav-chrome regression testTest plan
npm run test)npm run check)/signupon mobile and desktop viewportsCloses #18
🤖 Generated with Claude Code
🧑💻 Kai — Frontend Engineer
Verdict: ⚠️ Approved with concerns
Blockers
None.
Concerns
SignupFormusesbg-[var(--green)]for the submit button, but the design system comment inapp.css:21says--green(#3D8C4A) gives 4.16:1 contrast against white text — fails AA. The button should use--green-dark(#2E6E39) which the comment explicitly notes is for "button backgrounds with white text". Same issue as the spec'sbtn-primarytoken. (SignupForm.svelte:142)No
use:enhanceon the form. The form currently submits as a full-page navigation. The issue discussion (Kai's comment) explicitly resolved thatuse:enhanceshould be used for progressive enhancement. Without it: full-page reload on submit, no way to show server-side errors inline, no loading state on the button. The+page.server.tsreturnsfail(400, { error: ... })butSignupFormnever readsformfrom$pageoruse:enhance. This means server errors are silently lost. (SignupForm.svelte:42,+page.server.ts:18)Login link styling diverges from spec. The spec preview shows the link text in
--green/font-weight: 500. The implementation usestext-[var(--color-text)] underline— darker and underlined instead of green and bold. (SignupForm.svelte:152)Suggestions
<script lang="ts">— fine per project convention, but consider removing it since the root layout and login page both skip it when there's no logic. Consistency would say pick one pattern. Low priority.{#each}inline array inBrandPanel.svelte:23is readable but could be extracted to aconstat the top of the script block for clarity if more items are added later. Fine for now.autocompleteattributes to the form inputs (autocomplete="name",autocomplete="email",autocomplete="new-password") for better browser autofill behavior.🧪 QA Engineer — Test Coverage Review
Verdict: ⚠️ Approved with concerns
What's well covered
/signupand/signup/added to parameterized public route test. Good boundary coverage.Missing test paths
Server-side validation is absent. The
+page.server.tsaction trustsformData.get()blindly — no null check, no validation. If someone submits with JavaScript disabled and an empty form,displayName,email, andpasswordwill benull(cast to'null'string viaas string). There should be a test (and implementation) for server-side validation rejecting empty/invalid fields. (+page.server.ts:7-9)No test for server error display in the UI. The action returns
fail(400, { error: '...' })but no component test verifies that this error message is ever shown to the user. This is related to the missinguse:enhance— but even once that's added, the error display path needs a test.No test for multiple validation errors simultaneously. What happens when all three fields are invalid? The current tests only check one error at a time. A test submitting a completely empty form should verify all three errors appear.
Email validation edge cases. The regex
/^[^\s@]+@[^\s@]+\.[^\s@]+$/acceptsa@b.candfoo@bar..baz. Consider a parameterized test with known valid/invalid emails to document the boundary.Password boundary test. There's a test for "short" (5 chars) but not for exactly 7 chars (just under) and exactly 8 chars (just at minimum). Boundary tests would lock down the rule.
Suggestions
page.server.test.tsusesas anyfor the event mock. Consider extracting a typed factory similar tocreateEvent()inhooks.server.test.tsfor consistency.unused vi importcleanup inSignupForm.test.ts:3—viis imported but never used.🔒 Sable — Security Engineer
Verdict: 🚫 Changes requested
Blockers
No server-side input validation. The form action in
+page.server.ts:7-9doesformData.get('displayName') as stringwithout any validation. This sends whatever the user (or an attacker) submits directly to the backend API. While the backend should validate too, defense in depth requires the BFF layer to reject obviously malformed input before forwarding it. An attacker can bypass client-side JS validation trivially (curl, disabled JS, intercepting proxy). At minimum: reject empty strings, validate email format, enforce password minimum length server-side.Password sent in form data without
use:enhance. Withoutuse:enhance, the form submits as a traditional HTML form POST. This means the password travels in the request body asapplication/x-www-form-urlencoded— which is fine over HTTPS. But the concern is that SvelteKit's default form action handling does a full navigation, and on validation failure thefail()response may re-render the page without the password field populated (acceptable) — but there's no mechanism to show the error to the user at all (see Kai's review). The user gets a blank form with no feedback. This is a UX issue that becomes a security issue: users will retry, possibly with weaker passwords or by giving up and using a simpler form elsewhere.formData.get()returnsFormDataEntryValue | null, notstring. Theas stringcast on null returns the string"null", not an empty string. If someone submits the form with a missing field, the backend receivesdisplayName: "null". This could create an account with the literal name "null". (+page.server.ts:7-9)Concerns
hooks.server.tsdoesn't disable it. Currently looks fine, but worth a comment in the code or review notes.Suggestions
autocomplete="new-password"to the password input to help password managers generate strong passwords.<meta name="robots" content="noindex">to the signup page to prevent search engine indexing of the auth flow.🎨 Atlas — UI/UX Designer
Verdict: ⚠️ Approved with concerns
Blockers
None.
Concerns
Submit button uses
--greeninstead of--green-dark. Per the design system,--green(#3D8C4A) gives 4.16:1 contrast against white — fails WCAG AA for normal text. The button token rule is:--green-darkbg with white text. This is documented inapp.css:21. (SignupForm.svelte:142)Login link styling doesn't match spec. The spec shows "Log in" in
color: var(--green)withfont-weight: 500— a green, medium-weight text link with no underline. The implementation usestext-[var(--color-text)] underline— dark text with underline. Should be:text-[var(--green)] font-medium no-underline hover:underline. (SignupForm.svelte:152)Desktop form area not vertically centered on the
max-widthcontainer. The page layout hasjustify-centeron the right column, which centers the form vertically — good. But themax-width: 380pxdiv is not horizontally centered within the flex container. On very wide screens, the form will hug the left edge of the right column. The spec shows the form content area centered within the right panel. Consider addingmx-autoor wrapping alignment. (+page.svelte:10)Missing
<svelte:head>with page title. The signup page should set<title>Konto erstellen — Mealprep</title>for browser tab display and accessibility. Every page needs a meaningful<title>.Suggestions
max-width: 280pxandline-height: 1.6. Consider matching the spec's desktop tagline for richer brand storytelling.bg-white/10which is correct. Therounded-lg(10px) is close but the spec showsborder-radius: 8px— considerrounded-[8px]for pixel-perfect match.py-7(28px) which matches the spec's 28px padding. Good.🗄️ Backend Engineer — Spring Boot / PostgreSQL
Verdict: ✅ Approved
Observations
This PR is frontend-only from my perspective. The backend integration points look correct:
POST /v1/auth/signupwith{ displayName, email, password }matches theSignupRequestschema inopenapi.json. Good.apiClient(fetch)passes SvelteKit's fetch. This forwards cookies correctly for session establishment after signup. Good pattern — consistent withhooks.server.ts.app→backendis a welcome clarification. TheBACKEND_URL: http://backend:8080reference is correct.Concerns
POST /v1/auth/signup, the backend should set aJSESSIONIDcookie to establish the session. Theredirect(303, '/household/setup')will then land in an authenticated context. This works if the backend's signup endpoint returnsSet-CookieandapiClient(fetch)propagates it — but there's no test verifying this. Worth confirming the backend actually logs the user in on signup (not just creates the account). If signup and login are separate operations, the redirect to/household/setupwill bounce to/loginbecause the auth guard won't find a session.Suggestions
fail()error body with more structure — e.g.,fail(400, { error: message, field: 'email' })— so the frontend can highlight the specific field when the backend returns a 409 for duplicate email. The current generic message is fine for v1 but will need refinement.Validates displayName, email, password server-side before calling the backend API. Handles null from formData.get() safely. Returns structured field errors via fail(400, { errors }). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>Review Fixes Applied
All reviewer concerns addressed. 122 tests pass, 0 type errors.
--green→--green-darkb71c98675a13d9afcea65<svelte:head>with "Konto erstellen — Mealprep"845e669md:items-centeron parent82840bbbd9e133use:enhance+ form prop + error banner6d0f00c7de1874b3607ca🧑💻 Kai — Frontend Engineer
Verdict: ⚠️ Approved with concerns
The auth fixes are correct and the approach is clean. A few things worth flagging.
Blockers
None.
Suggestions
hooks.server.ts— TypeScript narrowing afterloginRedirect: After theif (error || !data?.data)block,data.datais accessed without narrowing. TypeScript should handle this correctly sinceloginRedirectis typed asnever, but ifopenapi-fetchtypesdataasT | undefined, TypeScript might not fully narrow it. The PR description says 0 type errors, so it's passing — but worth a second look to confirm the narrowing works correctly and isn't relying on a!assertion somewhere downstream.Cookie
secureflag: Bothsignupandloginactions set theJSESSIONIDcookie withoutsecure: true. In development this is fine, but in production over HTTPS this must besecure: true. Suggest adding an env-based flag or settingsecure: process.env.NODE_ENV === 'production'now so it doesn't get forgotten.Set-Cookiemulti-header edge case:response.headers.get('set-cookie')returns a comma-joined string when multipleSet-Cookieheaders are present. The regexJSESSIONID=([^;]+)still works correctly in that case since JSESSIONID values don't contain commas, but it's worth a comment to explain the pattern for future readers.No tests for the cookie forwarding path: The new session forwarding logic in both
+page.server.tsfiles has no component or integration test coverage. Worth adding a test that confirms a successful signup/login sets theJSESSIONIDcookie on the response.🗄️ Backend Engineer — Spring Boot / PostgreSQL
Verdict: ⚠️ Approved with concerns
The auth fixes are architecturally sound and the session management approach is correct. A few things to clean up.
Blockers
None.
Suggestions
authenticateInSession— hardcoded"user"for signup: The role"user"is hardcoded when called fromsignup. This is correct today since all new users aresystem_role = 'user', but it's an implicit assumption. A brief comment explaining why this is safe (new users always getsystem_role = 'user'per DB default) would help future readers.SecurityContextHolder.setContext()in a request-scoped context: Setting the thread-localSecurityContextHolderin the controller is fine — Spring Security clears it at the end of the request. The session attribute (SPRING_SECURITY_CONTEXT_KEY) is what persists. This is correct, but the pattern is non-obvious. Consider a short inline comment explaining that the holder is temporary and the session attribute is what matters across requests.logoutdoesn't clearSecurityContextHolder: Thelogoutendpoint invalidates the session (which destroys theSPRING_SECURITY_CONTEXTattribute) but doesn't callSecurityContextHolder.clearContext(). On a long-running Tomcat thread with keep-alive, there's a brief window where the thread-local context could be stale. Low-risk but worth addingSecurityContextHolder.clearContext()aftersession.invalidate()for correctness.No updated tests:
AuthControllerTestandAuthServiceTestexist but are not updated for the newauthenticateInSessionbehavior. At minimum, the login and signup tests should assert that the response includes aSet-Cookie: JSESSIONIDheader and that a subsequent call to/v1/auth/mewith that session returns 200.🧪 QA Engineer — Test Coverage
Verdict: ⚠️ Approved with concerns
The auth flow was completely broken before this PR — no tests caught it. That gap needs to be addressed.
Blockers
None for merge, but the missing test coverage represents real risk.
Missing test coverage (post-merge backlog)
Backend integration tests (highest priority — these would have caught the original bugs):
POST /v1/auth/signup→201+JSESSIONIDcookie set in responsePOST /v1/auth/signup→ subsequentGET /v1/auth/mewith returned session →200(end-to-end session establishment)POST /v1/auth/login→200+ newJSESSIONIDset, old session invalidatedPOST /v1/auth/loginwith wrong password →4xx(not a 500)GET /v1/auth/mewithout session →401GET /v1/auth/mewith invalid session →401Frontend action tests (Vitest):
JSESSIONIDcookie set + redirect to/household/setupJSESSIONIDcookie set + redirect to?redirector/plannerJSESSIONIDin backend response → redirect still fires (graceful degradation)E2E test (Playwright — critical path):
/household/setupwith valid session/plannerwith valid session/planner→ redirected to/login?redirect=%2FplannerObservation
The original broken
redirect()withoutthrowand the broken session setup survived 110 passing unit tests. Those tests weren't covering the right things. The integration test gap is the systemic issue here.🔒 Sable — Security Engineer
Verdict: 🚫 Changes requested
One blocker. The auth foundation is now correct — CSRF handling is appropriate for this architecture, session fixation protection is in place, and
httpOnlyon the cookie is good. But there's an open redirect that must be fixed before merge.Blockers
login/+page.server.ts: The?redirect=parameter is used without validation:https://yourapp.com/login?redirect=https://evil.comand after a successful login the user is sent off-site. Fix: validate thatredirectTois a relative path before using it:Suggestions (non-blocking)
secureflag onJSESSIONIDcookie: Both actions set the cookie withoutsecure: true. In production over HTTPS this means the session cookie can be transmitted over plain HTTP, exposing it to network interception. Addsecure: process.env.NODE_ENV === 'production'or an equivalent env flag.CSRF disabled — document the decision:
SecurityConfignow has.csrf(csrf -> csrf.disable())with no comment explaining why. A one-line comment (// CSRF handled by SvelteKit BFF; browser never calls backend directly) prevents future developers from re-enabling it without understanding the architecture dependency.authenticateInSessionrole parameter is a raw string: The method accepts anyString roleand uppercases it into aROLE_authority. There's no validation thatroleis a known value. Add an internal-only note or use the existingsystem_roleCHECK constraint values ("user","admin") as the expected inputs. Low risk since it's private, but a defensive check prevents accidental privilege escalation if the call site ever changes.🎨 Atlas — UI/UX Designer
Verdict: ✅ Approved
The auth changes are backend/server plumbing — no visual impact. One small layout fix in
signup/+page.sveltecaught my eye.Observations
signup/+page.sveltealignment fix:items-start→items-centeron the form container div on mobile is correct. The form was left-aligned on mobile instead of centered, which would have been visually off. Good catch.No visual regressions from the auth fixes: The session handling, cookie forwarding, and redirect logic are all server-side. The rendered components (BrandPanel, SignupForm, LoginForm) are unchanged. The design system tokens, navigation components, and layout splits from the original PR are unaffected.
Nothing to flag from my end.
✅ All reviewer concerns addressed
Here's a summary of every open concern resolved in this round, with the relevant commits:
Sable — Security Engineer
BLOCKER: Open redirect in
login/+page.server.ts— fixed in16f0feb?redirect=parameter now validated: must start with/and not//https://evil.com) and protocol-relative URLs (//evil.com) fall back to/plannerMissing
secureflag on JSESSIONID cookie — fixed in61249afsecure: trueadded tocookies.set()in both login and signup actionslocalhost(browsers treat localhost as a secure context even over HTTP)CSRF comment missing — fixed in
93ce1eaSecurityConfig.javaexplaining why CSRF is disabledKai — Frontend Engineer
Missing
secureflag — same fix as above (61249af)Backend Engineer
SecurityContextHolder.clearContext()missing from logout — fixed in93ce1eaauthenticateInSession()needs explanatory comment — fixed in93ce1eaNo tests for session establishment — fixed in
09333ccsignupShouldStoreSecurityContextInSessionandloginShouldStoreSecurityContextInSessionadded toAuthControllerTestSPRING_SECURITY_CONTEXT_KEYsession attribute is set after successful authAdditional fix (found during implementation)
3 existing tests were broken by our auth changes from the previous round — fixed in
16f0febresponseobject, causingresponse.headers.get(...)to throw a TypeErrormockSuccess()helper with a properresponse: { headers: { get: vi.fn().mockReturnValue(null) } }Final test counts: 150 frontend tests ✅ · 267 backend tests ✅ · 0 type errors ✅