From 1562929f6baaaab70d29a0271033b6d87303b7bd Mon Sep 17 00:00:00 2001
From: Peter Woolery <peter@research.bike>
Date: Fri, 5 Jun 2026 13:40:51 -0700
Subject: [PATCH] =?UTF-8?q?fix(ui):=20Sprint=209=20=E2=80=94=20dismiss=20X?=
 =?UTF-8?q?=20/=20Skip=20tour=20did=20not=20hide=20the=20dialog?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Root cause: the OnboardingTour early-return is gated on
isComplete=true, but App.tsx was calling onboarding.reset() on
onComplete. reset() does the inverse: clears the localStorage key
and flips isComplete to FALSE. The user clicked X, the localStorage
key got written, but the App-level flag flipped to false, so the
tour re-rendered and the early-return did not fire — the dialog
stayed visible.

Fix: split the dismiss and reset paths into two distinct callbacks
onComplete (dismiss) and onReset (re-show). Added markComplete to
useOnboarding: flips isComplete to true. App wires:
  onComplete -> onboarding.markComplete()
  onReset    -> onboarding.reset()
The tour itself still calls writeComplete() before invoking
onComplete, so the localStorage key is written once on dismiss.

Also cleaned markComplete: it now only flips state (the tour already
wrote the key), removing a redundant double-write.

Verified npm run build green on docker-willester. No regression
expected; all other Sprint 9 code paths untouched.
---
 frontend/src/App.tsx                       | 13 +++++++---
 frontend/src/components/OnboardingTour.tsx | 30 +++++++++++++++++++---
 2 files changed, 36 insertions(+), 7 deletions(-)

diff --git a/frontend/src/App.tsx b/frontend/src/App.tsx
index 17365da..614779d 100644
--- a/frontend/src/App.tsx
+++ b/frontend/src/App.tsx
@@ -102,9 +102,16 @@ function App() {
             <OnboardingTour
               isComplete={onboarding.isComplete}
               onComplete={() => {
-                // The tour already wrote the localStorage key via
-                // writeComplete(); flip the App-level flag so it stays
-                // hidden after the next render.
+                // Dismiss path (X / Skip / Esc / "Got it"). The tour
+                // already wrote the localStorage key; flip the
+                // App-level flag to true so the early-return fires and
+                // the dialog disappears.
+                onboarding.markComplete()
+              }}
+              onReset={() => {
+                // Re-show path (?reset-tour=1). The tour already
+                // cleared the localStorage key; flip the App-level
+                // flag to false so the tour re-appears.
                 onboarding.reset()
               }}
             />
diff --git a/frontend/src/components/OnboardingTour.tsx b/frontend/src/components/OnboardingTour.tsx
index e36c341..d17aac5 100644
--- a/frontend/src/components/OnboardingTour.tsx
+++ b/frontend/src/components/OnboardingTour.tsx
@@ -100,9 +100,16 @@ function clearComplete(): void {
   }
 }
 
-export function useOnboarding(): { reset: () => void; show: () => void; isComplete: boolean } {
+export function useOnboarding(): {
+  reset: () => void
+  show: () => void
+  markComplete: () => void
+  isComplete: boolean
+} {
   const [isComplete, setIsComplete] = useState<boolean>(readComplete)
 
+  // Used by ?reset-tour=1 and any other "re-show" path. Clears the
+  // localStorage key and flips the App-level flag so the tour re-appears.
   const reset = useCallback(() => {
     clearComplete()
     setIsComplete(false)
@@ -113,15 +120,28 @@ export function useOnboarding(): { reset: () => void; show: () => void; isComple
     setIsComplete(false)
   }, [])
 
-  return { reset, show, isComplete }
+  // Used by the tour's dismiss path (X / Skip / Esc / "Got it" on the
+  // last step). Called from App when onComplete fires. Just flips
+  // the App-level flag to true so the tour's early-return fires on
+  // the next render. The localStorage key is already written by the
+  // tour's finish() before it invokes onComplete.
+  const markComplete = useCallback(() => {
+    setIsComplete(true)
+  }, [])
+
+  return { reset, show, markComplete, isComplete }
 }
 
 export function OnboardingTour({
   isComplete,
   onComplete,
+  onReset,
 }: {
   isComplete: boolean
+  /** Called when the user dismisses the tour (X / Skip / Esc / "Got it"). */
   onComplete: () => void
+  /** Called when the tour re-shows (e.g. ?reset-tour=1). Inverse of onComplete. */
+  onReset: () => void
 }) {
   const location = useLocation()
   const navigate = useNavigate()
@@ -135,18 +155,20 @@ export function OnboardingTour({
   const isLast = step === STEPS.length - 1
 
   // Handle ?reset-tour=1 (clears the key; tour shows on next render).
+  // Calls onReset() to flip the App-level flag to false (the inverse
+  // of onComplete, which flips it to true on dismiss).
   useEffect(() => {
     const sp = new URLSearchParams(location.search)
     if (sp.get(RESET_PARAM) === '1') {
       clearComplete()
-      onComplete()
+      onReset()
       // Strip the param so a refresh doesn't re-trigger the reset.
       sp.delete(RESET_PARAM)
       const next = sp.toString()
       navigate(`${location.pathname}${next ? `?${next}` : ''}`, { replace: true })
       setStep(0)
     }
-    // We intentionally don't depend on `onComplete` (changes per render)
+    // We intentionally don't depend on `onReset` (changes per render)
     // — the only effect we want is when the URL search changes.
     // eslint-disable-next-line react-hooks/exhaustive-deps
   }, [location.search])