feat: D1 — Shopping list (Issue #30) #43
Reference in New Issue
Block a user
Delete Branch "feat/issue-30-shopping-list"
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
Full-stack implementation of the D1 Shopping List screen (closes #30).
GET /v1/shopping-list?weekStart=endpoint, merge-based regeneration strategy (preserves custom items + check states), reusable@RequiresHouseholdRoleannotation,filteredStaplesCountandRecipeRefin responseBackend changes
generated_atcolumn onshopping_listForbiddenException+ 403 handler@RequiresHouseholdRoleannotation +HouseholdRoleInterceptor(reusable across controllers)ShoppingListResponseextended withgeneratedAt,filteredStaplesCount,List<RecipeRef>getByWeekStart()— week-based lookup defaulting to current MondaygenerateFromPlan()refactored to merge strategyGET /v1/shopping-listendpoint, planner guard on generateFrontend changes
+page.server.ts— load (shopping list + week plan) + form actions (check, addItem, generate)ShoppingHeader.svelte— title, eyebrow counts, timestamp, generate buttonChecklistItem.svelte— checkbox with strikethrough, recipe source label, quantityAddCustomItem.svelte— expandable inline formRecipeReferencePanel.svelte— desktop sidebar with recipe cards + filtered staples+page.svelte— responsive layout with 3 empty statesTest plan
npm run check— 0 errors🤖 Generated with Claude Code
Renamed endpoint to /v1/shopping-list to avoid Springdoc path conflict. Added @RequiresHouseholdRole("planner") on generate. Regenerated frontend OpenAPI schema with all new shopping list endpoints. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>👨💻 Backend Engineer — Senior Spring Boot / PostgreSQL Specialist
Verdict: ⚠️ Approved with concerns
What I checked
Layer boundaries (controller → service → repository), domain modeling, SQL/migration quality, DTO design, transaction management, API design, error handling.
Blockers
None.
Suggestions
ShoppingServiceis getting large (~330 lines, 7 dependencies). ThegenerateFromPlan()merge logic is the core complexity — consider extracting it into a dedicatedShoppingListGeneratoror a domain method onShoppingListitself. TheMergedIngredientinner class is a sign this logic wants its own home. Not blocking for v1, but worth tracking.HouseholdRoleInterceptorthrowsForbiddenException("Not authenticated")whenauth == null(line 33). This is arguably a 401, not a 403. An unauthenticated user isn't forbidden — they haven't identified themselves yet. Spring Security should already handle this before the interceptor runs, so it's likely unreachable, but the semantics are worth being precise about.countFilteredStaples()(line 253–261) iterates all slots and all recipe ingredients on every response. For a read-heavy endpoint likeGET /v1/shopping-list, this is an N+1 waiting to happen if the JPA associations aren't eagerly fetched. Consider caching the count on theShoppingListentity at generation time, or at least verify the fetch plan with a SQL log check.Merge key construction is duplicated — the string
ingredient.getId() + "|" + unitappears in three places (lines 93, 106, 116). A small helpermergeKey(UUID ingredientId, String unit)would prevent subtle drift if one instance is changed but not the others.@Transactionalon the class level (line 28) meansgenerateFromPlanis read-write transactional, which is correct. ButgetByWeekStartoverrides with@Transactional(readOnly = true)— good. Just noting thatgetShoppingListalso hasreadOnly = true— consistent. 👍RequiresHouseholdRoleannotation uses a plainString value(). A type-safe enum (HouseholdRole.PLANNER,HouseholdRole.MEMBER) would prevent typos like"Planner"vs"planner". Low priority but worth considering as more endpoints use this.Positives
ShoppingListResponse,ShoppingListItemResponse, nestedRecipeRefandCategoryRef.findByHouseholdIdAndWeekPlanWeekStartis a well-named derived query.findList()helper.IF NOT EXISTS— safe for re-runs.🎨 Kai — Senior Frontend Engineer (Svelte 5 / SvelteKit 2)
Verdict: ⚠️ Approved with concerns
What I checked
Svelte 5 runes usage, SvelteKit 2 patterns (+page.server.ts / form actions / use:enhance), component architecture, Tailwind usage, reactivity model, TypeScript safety.
Blockers
None.
Suggestions
+page.svelteduplicates the entire layout for mobile and desktop. The mobile block (lines 27–117) and desktop block (lines 120–207) repeat the same empty states, the same checklist rendering, and the sameChecklistItemiteration. This is ~180 lines of near-identical markup. Extract the checklist body (unchecked items + add form + checked items) into aShoppingChecklist.sveltecomponent and render it in both layouts. The responsive chrome differs, but the content is identical.No
use:enhanceon the check form inChecklistItem.svelte. Theuse:enhanceis there (line 37), which is good — but it doesn't prevent the default full-page reload behavior. Without a customenhancecallback that callsupdate({ reset: false }), the form will re-render the page and scroll to top on each check/uncheck. For a checklist where users tap items rapidly, this UX is jarring. Consider:data.benutzer?.rolleaccess in+page.svelteline 11. Thebenutzerobject comes from the layout load function. Since this is cast as(data as any).benutzerin the planner page but accessed directly here, verify the type flows correctly from+layout.server.ts. IfPageDatadoesn't includebenutzer, this will type-check asanysilently. Consider adding the type toApp.PageDataor use$page.data.benutzer.ChecklistItem.svelteusesquantity: number | nullprop but the schema type isquantity?: number. The?? nullcoercion in+page.sveltehandles this, but it means the component's interface doesn't match the API type. Minor, but could cause confusion.AddCustomItem.svelteresets state after successful submission (line 34–38) but doesn't handle errors. If theaddItemaction returns{ success: false }, the form still collapses. Consider checking the action result before resetting.No loading/pending state on form submissions. When checking an item or generating a list, there's no visual feedback that the action is in progress. For generate (which may take a moment), consider disabling the button during submission.
Positives
$props(),$derived(),$state(). No legacy syntax.ShoppingHeader,ChecklistItem,AddCustomItem,RecipeReferencePanel.+page.server.tsfollows the established pattern from the planner page —apiClient(fetch), parallel fetches inPromise.all, form actions with role guard.{#if}chains.🧪 QA Engineer — Backend & Frontend Test Specialist
Verdict: ⚠️ Approved with concerns
What I checked
Test coverage, path analysis (happy/bad/edge), test quality and naming, missing test scenarios, TDD adherence.
Blockers
None.
Missing test coverage — Backend
No controller test for
@RequiresHouseholdRoleenforcement. TheHouseholdRoleInterceptorTesttests the interceptor in isolation, which is good. But there's no integration or controller test that verifiesPOST /v1/week-plans/{id}/shopping-listactually returns 403 when called by a non-planner. TheShoppingListControllerTestusesMockMvcin standalone mode (no interceptors registered), so the role guard is untested at the HTTP level.No test for
generateFromPlanremoving stale generated items. The merge test (generateFromPlanShouldMergeWhenListAlreadyExists) tests updating existing items and preserving custom items, but doesn't verify that a generated item is removed when its ingredient is no longer in the plan. Add a test where the existing list has tomatoes + onions, the new plan only has tomatoes, and verify onions are removed.No test for duplicate recipe IDs in
sourceRecipes. If the same recipe contributes an ingredient via two slots (e.g., a recipe used twice in the week),sourceRecipesshould deduplicate. The.distinct()call is there, but no test exercises it.checkItem— no test for wrong household. There's a household mismatch test forgetShoppingListandgenerateFromPlan, but not forcheckItem,addItem, ordeleteItem. These all usefindList()which enforces it, but the test gap means a regression could go unnoticed.Missing test coverage — Frontend
No frontend component tests for any of the new Svelte components. Zero test files created for
ShoppingHeader,ChecklistItem,AddCustomItem,RecipeReferencePanel, or the page itself. The persona spec says TDD — red/green/refactor — but the frontend was built without tests. This is the biggest gap.At minimum, I'd expect:
ShoppingHeader.test.ts— renders counts, shows/hides generate button based onisPlannerChecklistItem.test.ts— renders name, quantity, strikethrough when checked, recipe labelAddCustomItem.test.ts— expand/collapse, form submission+page.sveltetest — empty states render correctlySuggestions
ShoppingServiceTestuses reflection to set IDs (setId). This works, but it's fragile — if the field name changes, the test silently breaks. Consider a test factory or builder pattern. Not blocking.Test names are descriptive and follow conventions. Good:
generateFromPlanShouldMergeWhenListAlreadyExists,checkItemShouldUncheckAndClearUser. 👍Summary
Backend test coverage is solid (20 tests covering most paths) with a few gaps in cross-cutting concerns and edge cases. Frontend has zero component tests, which is a significant gap for a feature of this complexity. I'd recommend adding at least the
ShoppingHeaderandChecklistItemtests before merge.🔒 Sable — Security Engineer
Verdict: ⚠️ Approved with concerns
What I checked
OWASP Top 10 against all changed code: access control, injection, authentication/authorization, data exposure, multi-tenant isolation, CSRF, input validation.
Blockers
None.
Concerns
ForbiddenExceptionmessage leaks required role name (line 38 of interceptor):"Requires household role: planner"— this tells an attacker exactly which role they need. In theGlobalExceptionHandler, this message is returned verbatim in the API response body. Recommend a generic message like"Insufficient permissions"in the response, while keeping the specific role in server-side logs.No input validation on
AddItemRequestin the controller. TheaddItemaction acceptscustomName,quantity, andunitdirectly from user input. There's no@Validannotation on the request body, no length constraint oncustomName, and no range check onquantity. An attacker could submit a 10MBcustomNamestring or a negative/extreme quantity. Add@NotBlank @Size(max=255)oncustomNameand@Positiveonquantityat minimum.CheckItemRequest.isCheckedis not validated as non-null. If the JSON body omitsisChecked, the boolean defaults tofalsesilently. This isn't a security risk per se, but it means any malformed request body will uncheck items — potentially a griefing vector in a shared household.+page.server.tsform actions don't validatelistId/itemIdas UUID format. The form data values are passed directly to the API as path parameters. While the backend will likely reject malformed UUIDs, validating format server-side in the SvelteKit action prevents garbage from reaching the backend. This is defense in depth.Role check in
+page.server.tsgenerateaction (line 67) uses frontend-side role.locals.benutzer?.rolle !== 'planer'is a UI guard only — the backend interceptor is the real enforcement. This is correct architecture (defense in depth), but note the role string mismatch: frontend uses'planer'(German), backend uses'planner'(English). If these ever diverge, the frontend guard breaks silently. Worth documenting.Positives
householdIdmatch before returning data. IDOR is well-mitigated.generateFromPlanverifies household ownership of the week plan before proceeding.@RequiresHouseholdRoleis clean and auditable — single enforcement point.{@html}usage in any Svelte component — XSS risk from template injection is zero.use:enhancewith SvelteKit's built-in CSRF protection.🎨 Atlas — UI/UX Designer
Verdict: ⚠️ Approved with concerns
What I checked
Design system compliance (tokens, fonts, spacing, radii), responsive layout, accessibility (WCAG 2.2 AA), component consistency with existing screens, German localization.
Blockers
None.
Concerns
Checkbox in
ChecklistItem.svelteis a<button>, not an<input type="checkbox">. This works visually but breaks native checkbox semantics for screen readers. Thearia-labelis present, which helps, but assistive tech won't announce it as a checkbox — it'll announce it as a button. Either use a real<input type="checkbox">(hidden, with a styled label) or addrole="checkbox"andaria-checked={isChecked}to the button."Alles erledigt!" empty state (when all items are checked) has no visual weight. It's a single
<p>centered in the page. Consider adding a subtle icon or illustration consistent with the other empty states. The "no plan" and "no list" states both have links/actions — the "all done" state should feel like a positive completion, not a missing-data state.ShoppingHeader.sveltegenerate button has two visual variants (line 44–47) using a ternary in the class. The filled greenbg-[var(--green-dark)]for "Liste generieren" vs bordered for "Neu generieren" is a good distinction. However, the button text size/weight/tracking should match the spec:13px, weight 500, tracking 0.04em, font-sans. Thefont-mediumclass gives500— correct. 👍RecipeReferencePanel.svelterecipe cards usetext-[13px] font-medium. Good — matches the spec. But the day abbreviation usestext-[11px]which is below our minimum readable size for UI labels. Considertext-[12px]to match the eyebrow pattern used elsewhere.Mobile filtered staples info (line 97–103 of
+page.svelte) usesbg-[var(--color-surface)]but the desktop version inRecipeReferencePanelusesbg-[var(--color-page)]. These should be consistent — the panel sits on--color-surfacebg, so its children should use--color-pagefor contrast. The mobile version sits on--color-pagebg, so its children should use--color-surface. Currently they're swapped.No focus ring/indicator on the
ChecklistItembutton. When navigating via keyboard, users won't see which item is focused. Add afocus-visible:ring-2 focus-visible:ring-[var(--green)]or similar.Positives
--color-text,--color-text-muted,--color-border,--font-sans,--font-display,--radius-md.📐 Architecture & DevOps Review
Verdict: ✅ Approved
What I checked
Layer boundaries, cross-cutting concern design, migration safety, Docker/deployment readiness, API design consistency.
Notes
@RequiresHouseholdRole+HouseholdRoleInterceptoris a clean cross-cutting pattern. Interceptor-based enforcement is the right choice for Spring Boot 4.x without AOP starter. TheWebMvcConfigregistration is straightforward. This pattern will scale well to other controller methods.application-docker.ymladdsspring.flyway.out-of-order: true. This is safe for development but should not be enabled in production — out-of-order migrations can mask dependency issues. The Docker profile is dev-only, so this is fine.API path design:
GET /v1/shopping-list(singular, week-based lookup) vsGET /v1/shopping-lists/{id}(plural, ID-based) is slightly inconsistent in convention but functional. The singular form avoids the Springdoc collision that was discovered during implementation — pragmatic choice.Migration V022 is safe:
ADD COLUMN IF NOT EXISTSis idempotent.timestamptz NOT NULL DEFAULT now()is correct for PostgreSQL — uses server time, avoids timezone ambiguity.No deployment or CI concerns. No new dependencies, no infrastructure changes, no environment variables needed. The feature is purely additive.
LGTM from architecture and devops perspective.
Review concerns addressed ✅
All open reviewer concerns resolved across 8 commits on
feat/issue-30-shopping-list.Backend (commits
40ee4da–e3afe1b)mergeKey(UUID, String)private helper40ee4da"Insufficient permissions"40a6a0eAddItemRequest@Valid+@NotBlank @Size(max=255)oncustomName,@Positiveonquantity9d210begenerateFromPlanShouldRemoveStaleGeneratedItemseb5ee1asourceRecipesdeduplicationgenerateFromPlanShouldDeduplicateSourceRecipesWhenSameRecipeInTwoSlotseb5ee1acheckItemcheckItemShouldThrowWhenHouseholdMismatcheb5ee1aHouseholdRoleInterceptorwired into standalone MockMvce3afe1bFrontend (commits
be43fe9–f6265ef)use:enhancedoesn't prevent scroll-to-top on checkupdate({ reset: false })callbackbe43fe9role="checkbox"+aria-checkedChecklistItembe43fe9focus-visible:ring-2 focus-visible:ring-[var(--green)]be43fe9text-[11px]below minimumtext-[12px]inRecipeReferencePanelbe43fe9generatingstate; button shows…and is disabled during submitbe43fe9AddCustomItemcollapses even on action failureresult.type === 'success'before collapsingbe43fe9ShoppingChecklist.svelte; used in both layouts3cd9154ShoppingHeader.test.ts,ChecklistItem.test.ts,AddCustomItem.test.tsf6265efFinal state
npm run check: 0 errors