From d4555d45b4cefa04294ba9ebb8f90f84593f29ea Mon Sep 17 00:00:00 2001
From: engineer <engineer@macbookpro.lan>
Date: Thu, 16 Jul 2026 15:53:24 -0700
Subject: [PATCH] fix(feedback): don't count errored sessions; mark review
 asked before requesting
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Review findings on the store-review prompt:

1. SessionStatus has no error variant and session.error never touches
   sessionStatus, so an errored session still ends busy -> idle and was
   counted as a success — potentially burning the once-ever review prompt
   on a failed run. Track an erroredSessions set: mark in the
   session.error handler, clear when the session goes busy again (new
   run) and on disconnect, and skip recordSuccessfulSession() on the
   busy -> idle transition if the session errored.

2. ASKED_KEY was persisted only after requestReview() resolved. On iOS
   requestReview() can throw (MissingCurrentWindowSceneException while
   backgrounded — likely, since sessions often complete in background),
   which would retry the prompt on later successes, violating the
   "at most once, ever" contract. Persist ASKED_KEY before calling
   requestReview(); a failed attempt consumes the one shot.
---
 src/lib/store-review.ts |  7 ++++++-
 src/stores/events.ts    | 21 ++++++++++++++++++---
 2 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/src/lib/store-review.ts b/src/lib/store-review.ts
index 3518ad7..21be248 100644
--- a/src/lib/store-review.ts
+++ b/src/lib/store-review.ts
@@ -59,8 +59,13 @@ async function recordSuccessfulSessionInternal(): Promise<void> {
     const available = await StoreReview.isAvailableAsync()
     if (!available) return
 
-    await StoreReview.requestReview()
+    // Mark as asked BEFORE requesting: on iOS requestReview() can throw
+    // (e.g. MissingCurrentWindowSceneException while backgrounded — likely,
+    // since sessions often complete in the background). A failed attempt
+    // consumes the one shot; that beats retrying and violating the
+    // "at most once, ever" contract.
     await SecureStore.setItemAsync(ASKED_KEY, "true")
+    await StoreReview.requestReview()
   } catch {
     // A review-prompt failure must never affect session handling.
   }
diff --git a/src/stores/events.ts b/src/stores/events.ts
index 3210dce..9164598 100644
--- a/src/stores/events.ts
+++ b/src/stores/events.ts
@@ -52,6 +52,12 @@ interface EventsState {
 let controller: AbortController | null = null
 let reconnectTimer: ReturnType<typeof setTimeout> | null = null
 
+// Sessions that emitted session.error since they last went busy. SessionStatus
+// has no error variant — an errored session still ends with a busy -> idle
+// transition — so without this mark an errored run would count as a success
+// toward the once-ever store review prompt.
+const erroredSessions = new Set<string>()
+
 const RECONNECT_DELAYS_MS = [1000, 2000, 4000, 8000, 15000] as const
 const STABLE_CONNECTION_MS = 10_000
 const PROLONGED_DISCONNECT_MS = 30_000
@@ -161,6 +167,9 @@ export const useEvents = create<EventsState>((set, get) => ({
               const previous = get().sessionStatus[sessionID]
               const completed = previous?.type === "busy" && status.type === "idle"
 
+              // A new run starts — forget any error from the previous one
+              if (status.type === "busy") erroredSessions.delete(sessionID)
+
               set((state) => ({
                 sessionStatus: { ...state.sessionStatus, [sessionID]: status },
                 // Clear status text when idle
@@ -187,9 +196,11 @@ export const useEvents = create<EventsState>((set, get) => ({
                   body: sanitizeBody(match?.title, "Session finished processing"),
                   sessionId: sessionID,
                 })
-                // Genuinely positive moment (never fired on error) — count it
-                // toward the one-time store review prompt.
-                void recordSuccessfulSession()
+                // Genuinely positive moment — count it toward the one-time
+                // store review prompt, but only if this run never errored
+                // (session.error doesn't touch sessionStatus, so an errored
+                // session still lands here via busy -> idle).
+                if (!erroredSessions.has(sessionID)) void recordSuccessfulSession()
               }
               break
             }
@@ -240,6 +251,9 @@ export const useEvents = create<EventsState>((set, get) => ({
               const error = props.error as { message?: string } | undefined
               const sessionID = props.sessionID as string
               if (!sessionID) break
+              // Mark so the eventual busy -> idle transition is not counted
+              // as a success for the store review prompt
+              erroredSessions.add(sessionID)
               // Clear sending state unconditionally — SSE is truth
               useSessions.setState((state) => ({
                 sending: { ...state.sending, [sessionID]: false },
@@ -361,6 +375,7 @@ export const useEvents = create<EventsState>((set, get) => ({
     }
     controller?.abort()
     controller = null
+    erroredSessions.clear()
     set({
       connected: false,
       reconnectAttempts: 0,