feat(recipes): B2 — Recipe detail view with hero, ingredients, steps #37

marcel · 2026-04-03T10:02:33+02:00

marcel commented

2026-04-03 10:02:33 +02:00

Summary

Adds RecipeHero with green-tint (no-image) and image+overlay+white-text variants
Adds IngredientList — read-only ingredient rows with quantity + unit + name
Adds StepList — numbered circles (28px, green-dark) with step instructions sorted by stepNumber
Adds +page.server.ts — fetches GET /v1/recipes/{id}, throws 404 on error
Adds +page.svelte — mobile stacked layout, desktop 2-col ingredients|steps

Closes #24

Test plan

294 unit tests passing
npm run check — 0 errors, 0 warnings
RecipeHero: no-image (green-tint) and with-image (img + overlay) variants tested
IngredientList: quantity+unit displayed, no buttons (read-only)
StepList: steps sorted by stepNumber, numbered circles rendered
404 thrown when backend returns error

🤖 Generated with Claude Code

## Summary - Adds `RecipeHero` with green-tint (no-image) and image+overlay+white-text variants - Adds `IngredientList` — read-only ingredient rows with quantity + unit + name - Adds `StepList` — numbered circles (28px, green-dark) with step instructions sorted by stepNumber - Adds `+page.server.ts` — fetches `GET /v1/recipes/{id}`, throws 404 on error - Adds `+page.svelte` — mobile stacked layout, desktop 2-col ingredients|steps Closes #24 ## Test plan - [ ] 294 unit tests passing - [ ] `npm run check` — 0 errors, 0 warnings - [ ] RecipeHero: no-image (green-tint) and with-image (img + overlay) variants tested - [ ] IngredientList: quantity+unit displayed, no buttons (read-only) - [ ] StepList: steps sorted by stepNumber, numbered circles rendered - [ ] 404 thrown when backend returns error 🤖 Generated with [Claude Code](https://claude.com/claude-code)

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

feat(recipes): add RecipeHero component with image/no-image variants 86a25eb038

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

feat(recipes): add IngredientList component (read-only) c7e56a173d

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

feat(recipes): add StepList component with numbered circles b39d04acce

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

feat(recipes): add recipe detail load function with 404 handling ce860d68e4

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

feat(recipes): implement B2 recipe detail page with mobile/desktop layout 00c48a7c96

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

marcel commented

2026-04-03 10:03:22 +02:00

👨‍💻 Kai — Frontend Engineer

Verdict: 🚫 Changes requested

Blockers

Tags never rendered in RecipeHero.svelte
The recipe.tags prop is accepted and typed, but nothing in the template renders them. The spec explicitly requires tag pills in the hero ("Pills: time, effort, serves, tags" on desktop). All that data is being fetched and discarded. Add tag pills to the pills row — they should render just like effort/serves pills.

IngredientList.svelte doesn't sort by sortOrder
StepList correctly sorts by stepNumber using $derived. IngredientList iterates ingredients directly without sorting. The API returns a sortOrder field on each ingredient — they should be sorted by it. Otherwise ingredient display order is database-insertion-order, which is unpredictable. Apply the same pattern: const sortedIngredients = $derived([...ingredients].sort(...)).

No Edit link on the page
The spec requires an "Edit" button in the desktop topbar: → /recipes/{id}/edit. There's no <a href="/recipes/{recipe.id}/edit"> anywhere on the page or in the hero. This is a missing acceptance criterion.

Suggestions

Local types proliferating again
RecipeHeroProps in RecipeHero.svelte, Ingredient in IngredientList.svelte, Step in StepList.svelte, and RecipeData in +page.svelte all define inline types. We now have $lib/recipes/types.ts — extend it with RecipeDetail, Ingredient, and Step types and import them. Otherwise the same divergence risk we addressed in PR #36 comes back immediately.

Back link has no design system styling

<a href="/recipes" class="text-sm">← Zurück</a>

text-sm is a plain Tailwind class — not a design system token. Should use text-[13px] font-sans font-medium text-[var(--color-text-muted)] or similar. Visually it's probably close enough, but it's inconsistent with how we style navigation elements.

font-[400] is redundant
Fraunces (display font) has a default weight of 400 — specifying font-[400] adds noise without effect. Just omit it.

## 👨‍💻 Kai — Frontend Engineer **Verdict: 🚫 Changes requested** ### Blockers **Tags never rendered in `RecipeHero.svelte`** The `recipe.tags` prop is accepted and typed, but nothing in the template renders them. The spec explicitly requires tag pills in the hero ("Pills: time, effort, serves, tags" on desktop). All that data is being fetched and discarded. Add tag pills to the pills row — they should render just like effort/serves pills. **`IngredientList.svelte` doesn't sort by `sortOrder`** `StepList` correctly sorts by `stepNumber` using `$derived`. `IngredientList` iterates `ingredients` directly without sorting. The API returns a `sortOrder` field on each ingredient — they should be sorted by it. Otherwise ingredient display order is database-insertion-order, which is unpredictable. Apply the same pattern: `const sortedIngredients = $derived([...ingredients].sort(...))`. **No Edit link on the page** The spec requires an "Edit" button in the desktop topbar: `→ /recipes/{id}/edit`. There's no `<a href="/recipes/{recipe.id}/edit">` anywhere on the page or in the hero. This is a missing acceptance criterion. ### Suggestions **Local types proliferating again** `RecipeHeroProps` in `RecipeHero.svelte`, `Ingredient` in `IngredientList.svelte`, `Step` in `StepList.svelte`, and `RecipeData` in `+page.svelte` all define inline types. We now have `$lib/recipes/types.ts` — extend it with `RecipeDetail`, `Ingredient`, and `Step` types and import them. Otherwise the same divergence risk we addressed in PR #36 comes back immediately. **Back link has no design system styling** ```html <a href="/recipes" class="text-sm">← Zurück</a> ``` `text-sm` is a plain Tailwind class — not a design system token. Should use `text-[13px] font-sans font-medium text-[var(--color-text-muted)]` or similar. Visually it's probably close enough, but it's inconsistent with how we style navigation elements. **`font-[400]` is redundant** Fraunces (display font) has a default weight of 400 — specifying `font-[400]` adds noise without effect. Just omit it.

marcel commented

2026-04-03 10:03:33 +02:00

🎨 Atlas — UI/UX Designer

Verdict: 🚫 Changes requested

Blockers

Tag pills missing from hero
The spec says "Pills: time, effort, serves, tags" in both mobile and desktop hero. The tags prop is plumbed through to RecipeHero but the template renders nothing for them. Each tag should render as a pill identical in style to the effort/serves pills. Tags are the filtering metadata the user cares about most after time and effort.

No Edit button in desktop topbar
The spec requires an "Edit" button in the desktop topbar area that links to B3 (/recipes/{id}/edit). The current implementation has no such button anywhere. Style: ghost/outline treatment — border border-[var(--color-border)] text-[var(--color-text)] text-[13px] font-medium font-sans tracking-[0.04em] rounded-[var(--radius-md)] px-[16px] py-[8px].

Suggestions

Hero has no minimum height for image variant
When heroImageUrl is set, the <img> is absolute inset-0 object-cover w-full h-full — but the container has no explicit height. It's sized only by the padding and the inner content. On a recipe with a short name and few pills, the hero could render very small. Recommend min-h-[200px] md:min-h-[240px] on the hero container.

Back link styling
class="text-sm" renders as 14px DM Sans without any token — should use text-[13px] font-sans font-medium text-[var(--color-text-muted)]. Minor but inconsistent with design system.

Step circle accessibility
The numbered step circles (data-testid="step-circle") are presentational div elements rendering a number. The <ol> already provides ordering semantics. Consider aria-hidden="true" on the circle div so screen readers don't announce the step number twice (once from the circle, once from the list position).

## 🎨 Atlas — UI/UX Designer **Verdict: 🚫 Changes requested** ### Blockers **Tag pills missing from hero** The spec says "Pills: time, effort, serves, **tags**" in both mobile and desktop hero. The tags prop is plumbed through to `RecipeHero` but the template renders nothing for them. Each tag should render as a pill identical in style to the effort/serves pills. Tags are the filtering metadata the user cares about most after time and effort. **No Edit button in desktop topbar** The spec requires an "Edit" button in the desktop topbar area that links to B3 (`/recipes/{id}/edit`). The current implementation has no such button anywhere. Style: ghost/outline treatment — `border border-[var(--color-border)] text-[var(--color-text)] text-[13px] font-medium font-sans tracking-[0.04em] rounded-[var(--radius-md)] px-[16px] py-[8px]`. ### Suggestions **Hero has no minimum height for image variant** When `heroImageUrl` is set, the `<img>` is `absolute inset-0 object-cover w-full h-full` — but the container has no explicit height. It's sized only by the padding and the inner content. On a recipe with a short name and few pills, the hero could render very small. Recommend `min-h-[200px] md:min-h-[240px]` on the hero container. **Back link styling** `class="text-sm"` renders as `14px` DM Sans without any token — should use `text-[13px] font-sans font-medium text-[var(--color-text-muted)]`. Minor but inconsistent with design system. **Step circle accessibility** The numbered step circles (`data-testid="step-circle"`) are presentational `div` elements rendering a number. The `<ol>` already provides ordering semantics. Consider `aria-hidden="true"` on the circle div so screen readers don't announce the step number twice (once from the circle, once from the list position).

marcel commented

2026-04-03 10:03:45 +02:00

🧪 QA Engineer

Verdict: ⚠️ Approved with concerns

The test structure is good — each component has its own test file, behavior is tested from the user's perspective, and the load function has proper 404 coverage. But several paths that exist in the implementation aren't covered.

Missing tests (non-blocking but should be addressed)

RecipeHero.test.ts — no tag pill tests
Tags are typed in the prop but never rendered (a separate blocker). Even once tags are implemented, there's no test that tag names appear as pills. Add a test with recipe.tags = [{ id: 't1', name: 'Pasta' }] and assert screen.getByText('Pasta') is in the document.

IngredientList.test.ts — no sortOrder test
IngredientList currently doesn't sort. Once sorting is added, there should be a test that passes ingredients in the wrong order and asserts the DOM order matches sortOrder ascending.

page.test.ts — no Edit link test
Once the Edit button is added, test: screen.getByRole('link', { name: /bearbeiten/i }) has href /recipes/r1/edit.

page.server.test.ts — only tests 404 error path
The load function throws error(404, ...) for all API failures — 403, 500, and 404 alike. There's only one error test. Consider adding tests for a 403 response (different household) — the expected behavior is the same (404 thrown), which is intentional per security design, but it's worth an explicit test to document that decision.

No test for recipe with image variant in page test
page.test.ts only tests the no-image case (heroImageUrl: undefined). Add a variant with a real heroImageUrl and assert the <img> is rendered.

What's solid

StepList sort test with shuffled input ✅
IngredientList no-remove-button assertion ✅
404 rejection test with rejects.toMatchObject ✅
No CSS class snooping except for data-testid and the bg-[var(--green-tint)] hero test ✅

## 🧪 QA Engineer **Verdict: ⚠️ Approved with concerns** The test structure is good — each component has its own test file, behavior is tested from the user's perspective, and the load function has proper 404 coverage. But several paths that exist in the implementation aren't covered. ### Missing tests (non-blocking but should be addressed) **`RecipeHero.test.ts` — no tag pill tests** Tags are typed in the prop but never rendered (a separate blocker). Even once tags are implemented, there's no test that tag names appear as pills. Add a test with `recipe.tags = [{ id: 't1', name: 'Pasta' }]` and assert `screen.getByText('Pasta')` is in the document. **`IngredientList.test.ts` — no sortOrder test** IngredientList currently doesn't sort. Once sorting is added, there should be a test that passes ingredients in the wrong order and asserts the DOM order matches sortOrder ascending. **`page.test.ts` — no Edit link test** Once the Edit button is added, test: `screen.getByRole('link', { name: /bearbeiten/i })` has href `/recipes/r1/edit`. **`page.server.test.ts` — only tests 404 error path** The load function throws `error(404, ...)` for all API failures — 403, 500, and 404 alike. There's only one error test. Consider adding tests for a 403 response (different household) — the expected behavior is the same (404 thrown), which is intentional per security design, but it's worth an explicit test to document that decision. **No test for recipe with image variant in page test** `page.test.ts` only tests the no-image case (`heroImageUrl: undefined`). Add a variant with a real `heroImageUrl` and assert the `<img>` is rendered. ### What's solid - `StepList` sort test with shuffled input ✅ - `IngredientList` no-remove-button assertion ✅ - 404 rejection test with `rejects.toMatchObject` ✅ - No CSS class snooping except for `data-testid` and the `bg-[var(--green-tint)]` hero test ✅

marcel commented

2026-04-03 10:03:54 +02:00

🔒 Sable — Security Engineer

Verdict: ✅ Approved

B2 is a read-only detail view. Checked against the threat model — nothing alarming.

What I Verified

Access control: +page.server.ts throws error(404, ...) for any API error, including 403 (recipe from a different household). This is the correct behavior per the security model I outlined in the issue — returning 404 instead of 403 avoids revealing to an attacker whether the resource exists. ✅

XSS: No {@html} anywhere in the new components. Recipe name, effort, serves, cookTimeMin, ingredient names, step instructions — all rendered as Svelte text interpolations (auto-escaped). ✅

Image URL: <img src={recipe.heroImageUrl}> in RecipeHero.svelte — same analysis as PR #36. Backend-controlled value; SSRF/injection prevention is the backend's responsibility at write time. ✅

Information leakage: The load function surfaces no error details to the client — just a 404 page. API status codes, error messages, and internal paths stay server-side. ✅

Multi-tenancy: Auth is centralized in hooks.server.ts. The load function uses SvelteKit's server-side fetch which carries session cookies. The backend enforces household isolation at the API level. ✅

No auth checks in +page.server.ts: Correct — per project convention, role guards belong in hooks, not individual load functions. ✅

## 🔒 Sable — Security Engineer **Verdict: ✅ Approved** B2 is a read-only detail view. Checked against the threat model — nothing alarming. ### What I Verified **Access control**: `+page.server.ts` throws `error(404, ...)` for any API error, including 403 (recipe from a different household). This is the correct behavior per the security model I outlined in the issue — returning 404 instead of 403 avoids revealing to an attacker whether the resource exists. ✅ **XSS**: No `{@html}` anywhere in the new components. Recipe name, effort, serves, cookTimeMin, ingredient names, step instructions — all rendered as Svelte text interpolations (auto-escaped). ✅ **Image URL**: `<img src={recipe.heroImageUrl}>` in `RecipeHero.svelte` — same analysis as PR #36. Backend-controlled value; SSRF/injection prevention is the backend's responsibility at write time. ✅ **Information leakage**: The load function surfaces no error details to the client — just a 404 page. API status codes, error messages, and internal paths stay server-side. ✅ **Multi-tenancy**: Auth is centralized in `hooks.server.ts`. The load function uses SvelteKit's server-side `fetch` which carries session cookies. The backend enforces household isolation at the API level. ✅ **No auth checks in +page.server.ts**: Correct — per project convention, role guards belong in hooks, not individual load functions. ✅

marcel commented

2026-04-03 10:04:02 +02:00

🖥️ Backend Engineer

Verdict: ✅ Approved

Frontend-only PR. Checked the server-side load function and data mapping.

What Looks Good

+page.server.ts is correct

const { data, error: apiError } = await api.GET('/v1/recipes/{id}', {
    params: { path: { id: params.id } }
});
if (apiError || !data) { error(404, 'Recipe not found'); }

Clean variable naming (error: apiError avoids collision with the imported error from @sveltejs/kit). The 404 for all API errors is intentional and consistent with the security design. ✅

Data mapping is complete and explicit
All fields from RecipeDetailResponse that the UI needs are mapped: id, name, serves, cookTimeMin, effort, heroImageUrl, ingredients (with ingredientId, name, quantity, unit, sortOrder), steps (with stepNumber, instruction), tags (with id, name, tagType). ✅

Non-null assertions are justified
data.id! and data.name! — these fields are marked optional in the OpenAPI schema (a common codegen quirk for required fields) but are logically required by the recipe entity. The assertion is acceptable here. ✅

Observation

sortOrder is mapped but not used
The sortOrder field is correctly mapped from the API response in +page.server.ts. However, IngredientList.svelte doesn't actually sort by it (noted by Kai as a blocker). The backend data contract is correctly honored — the UI just needs to use the field.

## 🖥️ Backend Engineer **Verdict: ✅ Approved** Frontend-only PR. Checked the server-side load function and data mapping. ### What Looks Good **`+page.server.ts` is correct** ```ts const { data, error: apiError } = await api.GET('/v1/recipes/{id}', { params: { path: { id: params.id } } }); if (apiError || !data) { error(404, 'Recipe not found'); } ``` Clean variable naming (`error: apiError` avoids collision with the imported `error` from `@sveltejs/kit`). The 404 for all API errors is intentional and consistent with the security design. ✅ **Data mapping is complete and explicit** All fields from `RecipeDetailResponse` that the UI needs are mapped: id, name, serves, cookTimeMin, effort, heroImageUrl, ingredients (with ingredientId, name, quantity, unit, sortOrder), steps (with stepNumber, instruction), tags (with id, name, tagType). ✅ **Non-null assertions are justified** `data.id!` and `data.name!` — these fields are marked optional in the OpenAPI schema (a common codegen quirk for required fields) but are logically required by the recipe entity. The assertion is acceptable here. ✅ ### Observation **`sortOrder` is mapped but not used** The `sortOrder` field is correctly mapped from the API response in `+page.server.ts`. However, `IngredientList.svelte` doesn't actually sort by it (noted by Kai as a blocker). The backend data contract is correctly honored — the UI just needs to use the field.

marcel added 1 commit 2026-04-03 10:07:24 +02:00

fix(recipes): address B2 review — tags, sort, edit link, types, a11y, tests 0256b4360b

- RecipeHero: render tag pills, min-h-[200px/240px], fix back link styling, remove font-[400]
- IngredientList: sort by sortOrder ascending
- StepList: aria-hidden on step circles
- types.ts: add Tag, Ingredient, Step, RecipeDetail shared types
- +page.svelte: add Edit link → /recipes/[id]/edit (desktop topbar)
- Tests: tag pills, sortOrder sort, edit link, image variant, 403-as-404 documented

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

marcel merged commit fcf0f297bb into master

2026-04-03 10:07:27 +02:00

marcel referenced this issue from a commit

2026-04-03 10:07:29 +02:00

Merge pull request 'feat(recipes): B2 — Recipe detail view with hero, ingredients, steps' (#37) from feat/issue-24-recipe-detail into master

marcel deleted branch feat/issue-24-recipe-detail

2026-04-03 10:07:29 +02:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/mealprep#37