From 4c85c929d353f916d31520884f113ea705b98628 Mon Sep 17 00:00:00 2001 From: Peter Woolery Date: Fri, 5 Jun 2026 14:09:10 -0700 Subject: [PATCH] docs: record Sprint 9 post-deploy dismiss-bug fix across all 6 running docs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sprint 9 (commit 6e386ba) shipped a working OnboardingTour but a broken dismiss path: clicking X / Skip / Esc / "Got it" did nothing. Root cause: useOnboarding().reset() was wired to the dismiss handler at App.tsx, but reset() does the inverse of dismiss — it clears the localStorage key and flips isComplete to FALSE, so the tour re-rendered, the early-return did not fire, and the dialog stayed visible. Fix: commit 1562929 split the dismiss and reset paths into two distinct callbacks (onComplete and onReset). User confirmed browser smoke passes. This commit updates the 6 running docs that track Sprint 9: - .agent/plan.md — S9.4.1 sub-task (post-deploy fix) added. - .agent/context.md — D9 (root cause + fix) + Q4 (Vitest?) added. - Review/sprint9-verification.md — full post-deploy fix section appended (root cause, fix, post-fix verification, lessons). - Review/handoff-ui-audit.md — Sprint 9 status banner + Last updated footer updated to reference the fix commit. - fix-ui-audit.md — T3.4.1 sub-task added under the T3.4 verification gate. - docs/HANDOFF.md — post-deploy fix paragraph added to the Sprint 9 section. All 6 docs now reflect the post-deploy reality. No code changes. --- .agent/context.md | 2 ++ .agent/plan.md | 18 +++++++++-- Review/handoff-ui-audit.md | 4 +-- Review/sprint9-verification.md | 55 ++++++++++++++++++++++++++++++++++ docs/HANDOFF.md | 2 ++ fix-ui-audit.md | 8 +++-- 6 files changed, 83 insertions(+), 6 deletions(-) diff --git a/.agent/context.md b/.agent/context.md index 554a752..85ba684 100644 --- a/.agent/context.md +++ b/.agent/context.md @@ -194,12 +194,14 @@ User direction 2026-06-05: "Proceed with the next phase in the redesign." §Futu - **D6. rAF polling for the anchor's `getBoundingClientRect`.** Runs only while the tour is open. Cancellable. One DOM read per frame; well under 1% CPU on a 60Hz display. - **D7. Focus captured on open (primary action), restored on close.** Uses `previouslyFocused.current = document.activeElement` on mount; restores on unmount. Standard focus-trap pattern, minus the trap (the dialog is intentionally non-modal — the user can interact with the page below). - **D8. The 4 anchor points are stable elements that already exist in the DOM.** The Dashboard's `` wrapping the Weekly Overview, the Pantry's page header, the Recipes Filters button, the Shopping List page header. Each gets `data-tour=""`. The anchor also has an off-route fallback (centered card + "Open " CTA) so a first-time user who lands on `/pantry` can still see the Dashboard step (with a one-click nav). +- **D9. Sprint 9 post-deploy bug fix (2026-06-05).** The dismiss path (X / Skip / Esc / "Got it") was wired to `useOnboarding().reset()` via `onComplete`, but `reset()` does the *inverse* of dismiss — clears the localStorage key AND flips `isComplete` to `false`. So clicking X wrote the key, but the App-level flag flipped in the wrong direction, the tour's `if (isComplete || !currentStep) return null` early-return never fired, and the dialog stayed visible. **Fix (`1562929`)**: split the dismiss and reset paths into two distinct callbacks. `useOnboarding` now exposes `markComplete()` (state flip to `true`) in addition to `reset()` (state flip to `false`). `OnboardingTour` takes two props: `onComplete` (dismiss) and `onReset` (re-show). `App.tsx` wires `onComplete → onboarding.markComplete()` and `onReset → onboarding.reset()`. The tour's `finish()` still calls `writeComplete()` + `onComplete()`; `markComplete` is the matching App-side state setter. Cleaned up: `markComplete` no longer double-writes localStorage. The bug was missed in initial verification because `npm run build` was green and no browser smoke was run before deploy. ## Open questions to surface to the user, not to assume - **Q1. Should the tour show on every page or only `/`?** Default: `/` only. Other pages need `?reset-tour=1`. If the user lands on a non-root page first, the tour does NOT auto-show. Documented in `Review/sprint9-verification.md` smoke step 2. - **Q2. Should the tour re-show on logout / new device?** Default: no. The localStorage key is per-browser, not per-family-profile. If the user has multiple devices or shares a device, the tour shows once per browser. A future migration could move the key to the family profile, but that's a Sprint 11+. - **Q3. Should the tour re-show on a recipe update / catalog change?** Default: no. The tour is a one-shot. New users see it; existing users don't. +- **Q4. Should we add a Vitest unit test for `useOnboarding` to lock the dismiss/reset/show state transitions?** Default: not now (would require adding `vitest` + `happy-dom` to frontend dev-deps; violates "no new npm deps"). Trade-off: relying on browser smoke for the dismiss path means the same class of bug can re-appear if a future change mis-wires the callbacks. Worth lifting the "no new npm deps" rule *for testing only* in a future sprint. ## Sprint 9 verification gate diff --git a/.agent/plan.md b/.agent/plan.md index a2c43fd..b53c572 100644 --- a/.agent/plan.md +++ b/.agent/plan.md @@ -160,8 +160,22 @@ Goal: bring implementation back into alignment with `Review/reviewconcensus.md`. ### S9.4 — Verify - [x] `npm run build` green (tsc 0 errors, vite 0 errors). -- [ ] Browser smoke (8 steps) on `http://100.108.208.56:8082/` per `Review/sprint9-verification.md`. -- [ ] No regression in Sprints 1–8 (keyboard shortcuts, error toast, 3-button vote row, WeekRangeNav, bulk pantry add). +- [x] Browser smoke (8 steps) on `http://100.108.208.56:8082/` per `Review/sprint9-verification.md`. +- [x] No regression in Sprints 1–8 (keyboard shortcuts, error toast, 3-button vote row, WeekRangeNav, bulk pantry add). + +#### S9.4.1 — Post-deploy fix (2026-06-05) + +User reported post-deploy: "The tour window looks great, but Clicking the X nor skip tour do anything. I cannot exit the tour." Build was green but the dismiss path was broken. + +- [x] **Root cause identified** (systematic-debugging Phase 4): `useOnboarding().reset()` was wired to the dismiss handler at `App.tsx:104-109`. `reset()` does the *inverse* of dismiss — it clears the localStorage key AND flips `isComplete` to `false`. So clicking X wrote the key, but the App-level flag flipped in the wrong direction, the tour's `if (isComplete || !currentStep) return null` early-return never fired, and the dialog stayed visible. +- [x] **Fix committed** (`1562929`): split the dismiss and reset paths into two distinct callbacks. + - `useOnboarding` now exposes `markComplete()` (state flip to `true`) in addition to `reset()` (state flip to `false`). + - `OnboardingTour` takes two props: `onComplete` (dismiss) and `onReset` (re-show). + - `App.tsx` wires `onComplete → onboarding.markComplete()` and `onReset → onboarding.reset()`. + - Cleaned up: `markComplete` no longer double-writes localStorage (the tour's `finish()` already does that). +- [x] `npm run build` green on `docker-willester` after the fix (495.64 kB, no size change). +- [x] User confirmed post-deploy smoke test passes (2026-06-05). +- [x] Anchors + URL effect re-verified: 5/5 `data-tour` anchors present at `Dashboard.tsx:602`, `Pantry.tsx:185, 208`, `Recipes.tsx:131`, `ShoppingList.tsx:231`; `?reset-tour=1` effect calls `onReset()` correctly. ### S9.5 — Docs (all 6 running docs updated) diff --git a/Review/handoff-ui-audit.md b/Review/handoff-ui-audit.md index fb4b6af..2817486 100644 --- a/Review/handoff-ui-audit.md +++ b/Review/handoff-ui-audit.md @@ -56,7 +56,7 @@ If you are a new agent continuing this work, do this **in order**: ### Sprint 9 — F1 Onboarding Tour (H10) -**Status: COMMITTED on 2026-06-05. Build green. Frontend-only.** Awaiting user to `git pull` + `docker compose up -d --build frontend` (no migration, no backend rebuild). +**Status: COMMITTED + POST-DEPLOY FIX on 2026-06-05. Build green. Frontend-only.** Awaiting user to `git pull` + `docker compose up -d --build frontend` (no migration, no backend rebuild). The original commit (`6e386ba`) had a broken dismiss path (X / Skip / Esc / "Got it" did nothing — `onComplete` was wired to `useOnboarding().reset()` which is the inverse operation). Post-deploy fix is `1562929`: split into two callbacks `onComplete` (dismiss → `markComplete()`) and `onReset` (re-show → `reset()`). User confirmed browser smoke passes after the fix. **Root cause (one-liner):** new users land on the Dashboard with no orientation. The audit's F1 §Future item ("Onboarding hints / tour") was the natural next phase. @@ -362,4 +362,4 @@ cd frontend && npm run build Trust the build output. Trust the smoke checklist. Don't trust the deployment host's UI until the user confirms. The verification model is "I shipped, you verified, you reported, I fixed" — the agent in this role never sees the live UI directly. -**Last updated: 2026-06-05** — Sprint 1 deployed; Sprints 2-6 awaiting user deploy; **Sprint 7 (`09c7525`), Sprint 8 (`efd1fc6`), Sprint 9 (F1 Onboarding Tour), and Sprint 10 (Deny Forever on Recipes) committed on 2026-06-05, awaiting user deploy**. See the "How to take over" and "Pending user deploy" sections at the top of this file. +**Last updated: 2026-06-05** — Sprint 1 deployed; Sprints 2-6 awaiting user deploy; **Sprint 7 (`09c7525`), Sprint 8 (`efd1fc6`), Sprint 9 (F1 Onboarding Tour) + post-deploy fix (`1562929`), and Sprint 10 (Deny Forever on Recipes) committed on 2026-06-05, awaiting user deploy**. See the "How to take over" and "Pending user deploy" sections at the top of this file. diff --git a/Review/sprint9-verification.md b/Review/sprint9-verification.md index 3fcc772..f1957ba 100644 --- a/Review/sprint9-verification.md +++ b/Review/sprint9-verification.md @@ -117,3 +117,58 @@ One commit: `feat(ui): Sprint 9 — F1 onboarding tour (4-step welcome)`. Files: - `frontend/src/pages/Pantry.tsx` (2 anchors) - `frontend/src/pages/Recipes.tsx` (anchor on Filters button) - `frontend/src/pages/ShoppingList.tsx` (anchor on header) + +--- + +## Post-deploy fix (2026-06-05) — tour dismiss did nothing + +User reported after first deploy: clicking the X button, "Skip tour" text, "Got it" final-step button, OR pressing Esc on the dialog did nothing — the tour stayed visible and could not be exited. + +### Root cause + +The OnboardingTour dialog's early-return is gated on `isComplete === true`: + +```ts +if (isComplete || !currentStep) return null +``` + +But `App.tsx` was wiring the dismiss handler (`onComplete`) to `useOnboarding().reset()`. `reset()` does the **inverse** of dismiss: it clears the localStorage key AND flips `isComplete` to `false`. So when the user clicked X: + +1. `finish()` ran — `writeComplete()` wrote `"1"` to `localStorage.mealplanner:onboarding-complete` ✓ +2. `onComplete()` ran — `onboarding.reset()` cleared the key AND set `isComplete = false` ✗ +3. Tour re-rendered — `isComplete` was now `false`, so the early-return did NOT fire +4. The dialog stayed visible. Forever (until `?reset-tour=1` was visited). + +All four dismiss paths (X button, "Skip tour" text, Esc keyboard, "Got it" final-step button) shared the same broken path through `finish()` → `onComplete()`. None of them worked. + +The `?reset-tour=1` effect was working "accidentally" — it called `clearComplete()` (which was the right thing for the reset path) and `onComplete()` (which was the wrong thing for that path too, but the `clearComplete()` had already done the work, and the dialog re-appearing was the correct user-visible behavior). + +### Fix (commit `1562929`) + +Split the dismiss and reset paths into two distinct callbacks: + +1. Added `markComplete()` to `useOnboarding`: flips `isComplete` to `true` (matches the early-return's expected state). `reset()` and `show()` are unchanged. +2. `OnboardingTour` now takes two props: `onComplete` (dismiss) and `onReset` (re-show). +3. `App.tsx` wires: + - `onComplete → onboarding.markComplete()` (X / Skip / Esc / "Got it" all hide the dialog) + - `onReset → onboarding.reset()` (`?reset-tour=1` re-shows) +4. The tour's `finish()` still calls `writeComplete()` + `onComplete()`. Cleaned up: `markComplete` no longer double-writes localStorage (the tour's `finish()` already did that). + +### Post-fix verification + +- `npm run build` green on `docker-willester` (495.64 kB, no size change from Sprint 9 build). +- User confirmed browser smoke on `http://100.108.208.56:8082/`: + - X button → dialog disappears, `localStorage.mealplanner:onboarding-complete === "1"` + - "Skip tour" → dialog disappears, key set + - Esc → dialog disappears, key set + - "Got it" on last step → dialog disappears, key set + - Refresh → dialog does NOT re-show (key persists) + - `?reset-tour=1` → dialog re-appears, then URL strips the param on its own +- Re-verified all 5 `data-tour` anchors still resolve: `Dashboard.tsx:602`, `Pantry.tsx:185, 208`, `Recipes.tsx:131`, `ShoppingList.tsx:231`. +- Re-verified the URL effect calls `onReset()` (not the old `onComplete()`) for the `?reset-tour=1` path. + +### Lessons learned + +- **Lesson 1: A green build is not verification.** The bug was missed in initial verification because `tsc 0 errors + vite 0 errors` does not exercise the dismiss path. A 4-step browser smoke (open `/`, click X, check `localStorage`, refresh) would have caught it. Future sprints: when the deliverable is user-visible interaction (not just data rendering), browser smoke is part of the verification gate, not optional. +- **Lesson 2: Inverse paths deserve inverse APIs.** `reset()` and `markComplete()` are inverses, and they were collapsed onto a single `onComplete` callback. The collapse worked for `reset` and broke for `markComplete`. A two-callback API (`onComplete` + `onReset`) would have caught this at code-review time. +- **Lesson 3: Unit tests for state hooks are cheap insurance.** A 10-line Vitest test for `useOnboarding` would have caught this in CI without browser smoke. Worth lifting the "no new npm deps" rule for testing-only deps in a future sprint (see `.agent/context.md` Q4). diff --git a/docs/HANDOFF.md b/docs/HANDOFF.md index f7a593e..7108ab8 100644 --- a/docs/HANDOFF.md +++ b/docs/HANDOFF.md @@ -338,6 +338,8 @@ Trust the tests. Trust the live runs. Don't trust prose claims that something is **No regression expected:** Sprint 9 does not touch Sprints 1-8. The anchor `data-tour` attributes are additive; the page components still render the same. The KeyboardShortcuts hook (Sprint 5) is mounted in `App.tsx` and unaffected. The react-query error handler (Sprint 4) is unaffected. +**Post-deploy fix (2026-06-05, commit `1562929`):** user reported the X / Skip / Esc / "Got it" buttons did not dismiss the tour. Root cause: `App.tsx` wired the dismiss handler to `useOnboarding().reset()`, which is the *inverse* of dismiss (clears the localStorage key AND flips `isComplete` to `false`). Fix: split into two distinct callbacks — `onComplete` (dismiss) and `onReset` (re-show). User confirmed browser smoke passes after the fix. Full root-cause + lessons in `Review/sprint9-verification.md` (Post-deploy fix section). + ### Sprint 10 — "Deny Forever" on Recipes (user-driven) — COMMITTED 2026-06-05 **User direction (2026-06-05):** "Proceed with the next phase in the redesign. Also add a phase to include a 'Deny Forever' button in the Recipes endpoint." diff --git a/fix-ui-audit.md b/fix-ui-audit.md index 9379fba..a96ed6d 100644 --- a/fix-ui-audit.md +++ b/fix-ui-audit.md @@ -538,10 +538,14 @@ User direction 2026-06-05: "Proceed with the next phase in the redesign." F1 was ### T3.4 · Sprint 9 verification gate - [x] `npm run build` green for Sprint 9 (tsc 0 errors, vite 0 errors). -- [ ] Browser smoke (8 steps) on `http://100.108.208.56:8082/` per `Review/sprint9-verification.md`. -- [ ] No regression in Sprints 1-8. +- [x] Browser smoke (8 steps) on `http://100.108.208.56:8082/` per `Review/sprint9-verification.md`. +- [x] No regression in Sprints 1-8. - [x] `Review/sprint9-verification.md` written. +#### T3.4.1 · Post-deploy fix (2026-06-05, commit `1562929`) + +User reported post-deploy that X / Skip / Esc / "Got it" did not dismiss the tour. Root cause: `App.tsx` wired the dismiss handler to `useOnboarding().reset()`, which is the inverse of dismiss (clears the localStorage key AND flips `isComplete` to `false`). Fix: split the dismiss and reset paths into two distinct callbacks (`onComplete` → `markComplete()` and `onReset` → `reset()`). The tour's `finish()` still calls `writeComplete()` + `onComplete()`. Full root-cause + fix + lessons in `Review/sprint9-verification.md` (Post-deploy fix section). + --- ## Sprint 10 — "Deny Forever" on Recipes — ✅ COMPLETE, awaiting deploy