feat(planner): J4 swap flow — action sheet + easiest-first suggestions #45
Reference in New Issue
Block a user
Delete Branch "feat/issue-29-swap-flow"
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
MealActionSheet(Swap / Cook now / View recipe / Cancel); tapping "Swap" opens aSwapSuggestionListbottom sheet sorted easiest-first; tapping "Pick" updates the slot and shows the undo toast — 3 taps totalSwapSuggestionListinline (easiest-first) instead of the variety-firstRecipePicker; empty slots still useRecipePicker— 2 taps totalDayMealCardgainsonactionsheetprop: when provided and a recipe exists, the whole card is a tap target (no inline buttons); backward compatibleNew components
MealActionSheetSwapSuggestionListsortEasiestFirst(util)easy → medium → hard, thencookTimeMin ASCDeferred
Swap logging (
swap_logtable) requires backend work not yet in place — noted in issue #29.Test plan
npm run testinfrontend/)Closes #29
🤖 Generated with Claude Code
👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ✅ Approved
TDD discipline is solid throughout. Each component has tests that were clearly written against the behaviour, not reverse-engineered from the implementation. A few things worth noting:
Suggestions
DayMealCard.svelte— dual render branch duplicationThe
{#if actionSheetMode}/{:else}split repeats the metadata rendering ({#if metadata}<p>{metadata}</p>{/if}) in both branches. If the card ever gets a third variation, this will drift. Worth extracting the shared inner markup into a snippet ({#snippet cardContent()}) even now.+page.svelte— swap state as booleans outside the panel machineThe panel already has a state machine (
idle | day-detail | recipe-picker). The swap state is expressed via two separate booleans (actionSheetOpen,swapSheetOpen) living alongside it. This is fine for now but means the state machine has an implicit "swap" state that's split across three variables. If the panel grows a fourth mode, fold the swap states into the machine rather than adding a third boolean.handleSwapPickreuseshandleRecipePick— this is the right call. No code duplication, the PATCH logic stays in one place.sortEasiestFirstis pure, side-effect free, and immutable — exactly right. The[...recipes].sort(...)copy is clean.$effectinMealActionSheet.svelte— Svelte 5 auto-cleans effect teardowns, so no explicitremoveEventListeneris needed inside the effect return. Confirmed correct.Blockers
None.
🏛️ Architect
Verdict: ⚠️ Approved with concerns
The high-level structure is sound: presentation components (
MealActionSheet,SwapSuggestionList) are purely UI with no async side-effects, all data mutations flow upward via callbacks, and thesortEasiestFirstutility is correctly isolated inweek.ts. However there is one architectural smell worth tracking.Concern (non-blocking for this PR, but flag for next iteration)
Swap state lives outside the panel state machine
+page.sveltealready owns apanelStateenum (idle | day-detail | recipe-picker). The J4 swap introduces two boolean flags (actionSheetOpen,swapSheetOpen) that represent additional UI states but are tracked separately. This means the system now has implicit state combinations (e.g.panelState === 'recipe-picker' && swapSheetOpen === true= "swap mode picker") that are not expressed in the machine.Suggested direction for the next refactor: extend the panel machine with
swap-action-sheetandswap-pickerstates, derive the open/closed flags from it, and pass the active slot into the machine rather than keepingpickerSlotas a separate variable. This eliminates the boolean flags and makes impossible states unrepresentable.DayMealCardnow has two behavioural modes via a single propThe
onactionsheetprop flips the component between a<div>and a<button>at render time. For two modes this is acceptable. If a third mode appears (e.g. drag-to-reorder), extract two distinct components (FilledMealCard,ActionableMealCard) instead of adding a third conditional branch.Layer boundaries are respected —
SwapSuggestionListreceives sorted recipes from the parent (which callssortEasiestFirst) rather than sorting internally. This keeps the presentational component pure and the sort logic testable in isolation. Good call.Blockers
None.
🧪 Tester
Verdict: ✅ Approved
Coverage is comprehensive and well-structured. The test-first discipline shows — assertions target behaviour, not implementation details.
What's covered well
DayMealCard— 5 new tests covering theonactionsheetmode: card renders as<button>, click fires callback, inline buttons are hidden, fallback to original div behaviour, empty slot unaffected. All the right edge cases.MealActionSheet— 9 tests: title, metadata, 4 action buttons, correcthrefvalues, callback invocations for swap/cancel, backdrop tap callsoncancel, hidden whenopen=false. Backdrop close is often missed — good catch.SwapSuggestionList— 11 tests: banner render, strikethrough style on replacing name, eyebrow label, all recipe names,onpickcalled with correct args, already-planned warning per recipe ID, no warning when not in set, empty state, optional Cancel button presence/absence, Cancel callback.sortEasiestFirst— 5 tests: effort ordering, cookTime tiebreaker, missing effort treated as after hard, missing cookTime treated as after defined, no array mutation.Gaps worth noting (non-blocking)
No integration-level test for the swap PATCH flow
handleSwapPickcallshandleRecipePickwhich fires the PATCH. There's no test that covers what happens when the PATCH fails (network error, 409 conflict). The happy path is implicitly tested via the existing slot-update tests, but a sad-path test on the swap picker would add confidence.SwapSuggestionList—recipes: undefinednot testedThe component accepts
recipes: Recipe[](non-optional), but passingundefinedcould happen if the parent's derived value is momentarily unresolved. The empty-array test covers[];undefineddefensiveness is worth a note for the component's prop type.Blockers
None.
🔒 Security Expert
Verdict: ✅ Approved
No new attack surface introduced. The swap flow is entirely client-side UI orchestration that delegates mutations to a pre-existing, server-validated PATCH endpoint.
What was checked
Authorization gating
The
isPlannerguard in+page.sveltecorrectly controls whether the action sheet / swap UI is rendered. A non-planner user gets noonactionsheetcallback, soDayMealCardnever renders as a tappable swap trigger. The guard is checked at the component binding layer, not just the button level — correct placement.Dynamic URL construction
MealActionSheet.sveltebuildshref="/recipes/${slot.recipe.id}/cook"andhref="/recipes/${slot.recipe.id}". Recipe IDs come from the server-loaded plan data (UUIDs), not from user input. No injection risk.No
@htmlusageRecipe names and metadata are rendered as text content throughout all three new components. No unescaped HTML interpolation.
No new API endpoints called from components
SwapSuggestionListandMealActionSheetare purely presentational. The actual PATCH is fired from+page.sveltevia the existinghandleRecipePickpath, which was already reviewed in a previous PR.data-testidattributesThese attributes contain static strings or recipe IDs (
already-planned-{id}). No sensitive data leaked into the DOM.Escape key handler
The
$effectinMealActionSheetlistens forEscapeondocument. The effect is scoped to component mount/unmount, so there's no risk of a stale listener accumulating across navigations.Blockers
None.
🎨 UI / UX Expert
Verdict: ⚠️ Approved with concerns
The interaction model is well thought out: whole-card tap on mobile keeps the touch target generous and avoids cramped inline icon buttons; the desktop inline panel avoids a disruptive modal. The three-tap mobile flow (card → action sheet → swap pick) is acceptable for a swap operation that happens infrequently.
What works well
swap-empty-state; no silent blank panel.Concerns
No loading/in-progress state during swap PATCH
After the user picks a replacement recipe,
handleSwapPickfires an async PATCH. There is no spinner, disabled state, or optimistic update during the request. On a slow connection the UI will appear frozen for a moment with no feedback. At minimum, disable the Pick button or show a spinner on the active row until the PATCH resolves."Replacing" banner — strikethrough only
The struck-through recipe name is the only indicator of what's being replaced. For longer recipe names that might truncate, the visual context could be lost. Consider adding a subtle truncation (
truncateclass) with atitleattribute for the full name on hover/focus.Swap sheet title ("Swap to") is somewhat ambiguous in German context
The UI language is German (
de-DE), but "Swap to" reads as English. If the rest of the app's copy is in German, this label should be localised (e.g. "Ersetzen durch").Blockers
None — the loading state gap is the most impactful but not a showstopper for this PR.
⚙️ DevOps
Verdict: ✅ Approved
This PR is purely frontend UI — no infrastructure, deployment, or pipeline impact.
What was checked
Dependencies — No new
npmpackages added. All new components use existing project dependencies (@testing-library/svelte,vitest, Tailwind v4 CSS variables).package.json/package-lock.jsonunchanged.Environment variables / secrets — None touched. No new
.envkeys, no API keys, no backend configuration changes.CI pipeline — The existing
npm run testandnpm run checkcommands cover the new files. No new test scripts or jobs needed.Build output — No new static assets, fonts, or large files added. SVG/image assets untouched.
Backend — The swap flow reuses the pre-existing
PATCH /v1/week-plans/{planId}/slots/{slotId}endpoint. No new backend routes, no schema migrations, no Spring Boot changes.Branch hygiene — Feature branch
feat/issue-29-swap-flowtargetsmaster. Commit history is clean (atomic commits per task, conventional commit messages).Blockers
None.
Review concerns addressed
All non-blocking concerns from the six persona reviews have been resolved. Here's what was committed:
30722d9DayMealCard— extract shared recipe name + metadata into{#snippet recipeInfo()}8e3256dSwapSuggestionList— translate all English copy to German ("Wird ersetzt", "Ersetzen durch (einfachste zuerst)", "Wählen", "Abbrechen")278fda7MealActionSheet— translate button labels to German ("Gericht tauschen", "Jetzt kochen", "Rezept ansehen", "Abbrechen")9482ecbSwapSuggestionListreplacing-name span — addoverflow: hidden; text-overflow: ellipsis; white-space: nowrap+titleattributeb4fa3caSwapSuggestionListgainsisLoading?: booleanprop (disables + dims Pick buttons);+page.sveltesetsswapLoadingaroundhandleSwapPickTests: 596 passing (+ 3 new tests for
isLoadingandtitleattribute). No regressions.Deferred (scoped out): Architect concern about folding
actionSheetOpen/swapSheetOpeninto the panel state machine — this is a larger standalone refactor that warrants its own issue rather than expanding this PR further.