feat(recipes): B1 — Recipe Library page with search and effort filtering #36
Reference in New Issue
Block a user
Delete Branch "feat/issue-22-recipe-library"
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
RecipeCardcomponent with hero image/placeholder, name, cookTime, effortFilterChipRowfor effort filtering (Alle/Leicht/Mittel/Schwer) with aria-pressed accessibilityRecipeGridwith 2-col mobile / 4-col desktop layout and empty state+page.server.tsfetching recipes fromGET /v1/recipes+page.sveltewith client-side search + effort filtering, link to add recipeCloses #22
Test plan
npm run check— 0 errors, 0 warnings🤖 Generated with Claude Code
👨💻 Kai — Frontend Engineer
Verdict: 🚫 Changes requested
Blockers
RecipeSummarytype defined in three places (RecipeCard.svelte,RecipeGrid.svelte,+page.svelte)These three files each independently define an identical local type:
This will diverge the moment any field changes. Extract it to a shared module (
$lib/recipes/types.ts) or derive it from the OpenAPI-generated types. Keeping three copies is a maintenance time-bomb.+page.svelte"Rezept hinzufügen" link uses Skeleton UI classesbtnandvariant-filled-primaryare Skeleton UI utility classes — we don't use Skeleton. This will render as an unstyled text link. TheRecipeGrid.svelteempty-state link shows the correct pattern:font-sans text-[13px] font-medium tracking-[0.04em] rounded-[var(--radius-md)] bg-[var(--green-dark)] px-[24px] py-[12px] text-white. Use that.Suggestions
Unused imports in
page.test.ts—vi,beforeEach,afterEachare imported but not used. Clean these up.effortMapin+page.svelte— the mapping lives only in the page, whileFilterChipRowjust emits raw label strings. If effort labels ever change, the mapping breaks silently. Consider co-locating the chips array with their API values insideFilterChipRow, or exporting the map from a shared constants file so the page imports it instead of defining it inline.🎨 Atlas — UI/UX Designer
Verdict: 🚫 Changes requested
Blockers
+page.svelteheader link broken stylingThe "Rezept hinzufügen" link in the page header uses
class="btn variant-filled-primary"— not design system tokens. This renders as an unstyled link. The empty-state link inRecipeGrid.sveltegets it right:Apply the same treatment to the header link.
FilterChipRow.sveltefont size violates design systemChips use
text-[11px]:The design system rule is non-negotiable: all button-role elements use
text-[13px]. These chips are<button>elements — the rule applies. Change totext-[13px].Suggestions
<h1>in+page.sveltedoesn't use display fontPage headings should use
font-[var(--font-display)](Fraunces). Recommend:font-[var(--font-display)] text-[28px] font-medium text-[var(--color-text)]. Thetext-2xlTailwind utility equals 24px which is close but doesn't use the correct font family.RecipeCard metadata text
text-[11px]cookTimeMin · effortis rendered at 11px inRecipeCard.svelte. This is secondary metadata, not a button, so the 13px button rule doesn't strictly apply — but 11px is below 12px which is generally considered the minimum for readable body text. Considertext-[12px].🧪 QA Engineer
Verdict: ⚠️ Approved with concerns
Coverage Summary
Overall test coverage is solid — all five new components have dedicated test files following the right patterns (query by role/text/label,
userEventfor interactions). The TDD structure is evident. Good work.Concerns (non-blocking but should be addressed)
Missing: combined search + effort filter test
There's a test for search-only and a test for effort-chip-only, but no test for applying both simultaneously. This is a real user interaction (filter by "Leicht" then search within results). The
$derivedchain handles it, but it's an untested code path. Example:Missing: test for clearing effort filter back to "Alle"
There's no test that clicking "Alle" after an active filter resets to showing all recipes. This validates the toggle/reset UX.
page.test.tsunused importsvi,beforeEach, andafterEachare imported but not used. Minor, but test files should be kept clean — they serve as documentation.page.server.test.tsusesload: anyType assertion
let load: anysuppresses TypeScript safety on the test. This is a known pattern for dynamic imports in Vitest but worth calling out — if the load function signature changes, the test won't catch it through types.What's Good
userEvent.setup()used throughout (notfireEvent) ✅page.server.test.tsis clean:vi.resetModules()+ dynamic import ensures isolation ✅🔒 Sable — Security Engineer
Verdict: ✅ Approved
Checked this PR against the OWASP Top 10 and project-specific threat model. No blockers.
What I Verified
Access control: The recipe library page is read-only and serves data for authenticated household members. Auth is centralized in
hooks.server.ts— the load function correctly does not duplicate auth logic. ✅Injection / XSS: No
{@html}usage anywhere in the new components. Recipe name, cookTimeMin, and effort values are rendered as text interpolations (auto-escaped by Svelte). ✅heroImageUrlin RecipeCard: The<img src={recipe.heroImageUrl}>renders an image from a backend-provided URL. The backend controls this value (stored via API, not directly from user input at render time), so SSRF andjavascript:scheme injection are the backend's responsibility to prevent at write time. Acceptable. ✅Information leakage: The load function returns
{ recipes: [] }on any API error — no status codes, stack traces, or internal error messages leak to the client. ✅No client-side filtering secrets: The
effortMapand search logic in+page.svelteoperate entirely on data already delivered to the client. No server-side bypass concern. ✅Multi-tenancy: The
GET /v1/recipescall uses SvelteKit's server-sidefetchwhich carries the session cookie. The backend enforces household isolation — the frontend is not responsible for that here. ✅🖥️ Backend Engineer
Verdict: ✅ Approved
This PR is frontend-only and I have limited concerns from the backend perspective. Here's what I checked:
What Looks Good
+page.server.tsis clean and idiomaticThe load function is thin — it fetches, maps, and returns. No business logic, no leaking internal API shapes. The defensive fallback to
{ recipes: [] }on error is correct for a read-only page. ✅Server-side data fetching: All recipe data is loaded server-side via
+page.server.tsusing the authenticatedfetch. No client-side API calls, no exposed API keys. ✅Field mapping is explicit: Only the fields actually needed by the UI are mapped —
id,name,cookTimeMin,effort,heroImageUrl. Internal fields from the API response (if any) are stripped. ✅Observation
Client-side filtering vs server-side filtering
GET /v1/recipesis called with no query params ({}), and search/effort filtering is done client-side in$derived. This is fine for small datasets. If the recipe count grows significantly, the backend already supportssearchandeffortquery params — migrating filtering server-side later would be a clean lift. No action needed now, just noting the tradeoff is understood.