From 27362cee98b2cba638a403319c0e10a5c7791de2 Mon Sep 17 00:00:00 2001 From: Den <2119348+dzianisv@users.noreply.github.com> Date: Sat, 18 Jul 2026 06:04:15 -0700 Subject: [PATCH] fix: request notification permission; resync session on focus; notif + reply bugs (#121) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five correctness bugs from an adversarial review of the notification and permission/approval paths (each verified against the code): 1. Notifications never worked for most users (HIGH): OS permission was only requested when a user manually toggled a Settings switch off→on. Since categories default on, that path never fired, permission stayed 'undetermined', and send() silently no-op'd every notification. Now request it once on first live connection (in-context). (app/_layout.tsx) 2. Wrong-session data after back-navigation (HIGH): session screen reads a global store and its resync ran only on mount; the native stack keeps screens mounted underneath a pushed one, so returning to a session could show another session's messages and permission prompts — approving the wrong session's tool call. Re-select on focus via useFocusEffect. (app/session/[id].tsx) 3. 'Task completed' fired on aborted/errored runs (misleading, and a duplicate push alongside 'Session error'). Gate the notify by !aborted && !errored. (src/stores/events.ts) 4. Tapping a connection-drop notification (no sessionId) navigated to an empty '/session/' dead-end. Route to home instead. (app/_layout.tsx) 5. Double-tap on a single-select question sent two replies; the second hit an already-resolved request and popped a spurious 'Reply failed' alert. One-shot guard on reply/reject. (src/components/chat/QuestionPrompt.tsx) Verified but intentionally NOT changed: 'completed' notifications default off (a defensible anti-spam choice — the app still notifies when the agent needs input). typecheck clean, 187/187 tests. Claude-Session: https://claude.ai/code/session_01T12AhSnQVrSxNnvwfCx2z6 Co-authored-by: engineer Co-authored-by: Claude Fable 5 --- app/_layout.tsx | 20 +++++++++++++++-- app/session/[id].tsx | 31 +++++++++++++++++--------- src/components/chat/QuestionPrompt.tsx | 26 ++++++++++++++++----- src/stores/events.ts | 22 ++++++++++++------ 4 files changed, 74 insertions(+), 25 deletions(-) diff --git a/app/_layout.tsx b/app/_layout.tsx index aa0d981..b4ffe33 100644 --- a/app/_layout.tsx +++ b/app/_layout.tsx @@ -30,6 +30,7 @@ function RootLayout() { const { initialize: initAuth, isLoading: authLoading } = useAuth() const { loadConnections, isLoading: connectionsLoading, client } = useConnections() const sseStarted = useRef(false) + const notifPermissionRequested = useRef(false) // Telemetry consent state: null = loading, 'unknown' = show modal, else decided const [consentState, setConsentState] = useState<"loading" | "unknown" | "decided">("loading") @@ -42,9 +43,12 @@ function RootLayout() { // Connect notification preferences to the notification module notifications.configure(() => useSettings.getState().notifications) - // Navigate to session when user taps a notification + // Navigate to session when user taps a notification. Connection-drop + // notifications carry no sessionId (they aren't about a session) — route + // to the home tab instead of "/session/" (an empty, dead-end route). const unsubNotifications = notifications.onTap((data) => { - router.push(`/session/${data.sessionId}`) + if (data.sessionId) router.push(`/session/${data.sessionId}`) + else router.push("/") }) // Load telemetry consent — initialise Sentry only if previously granted @@ -79,6 +83,18 @@ function RootLayout() { sseStarted.current = true useEvents.getState().connect() useCatalog.getState().load() + // Request OS notification permission once we have a live connection — + // the in-context moment the user will start running agent tasks they'll + // want to be pinged about. Previously this was only ever requested when + // a user manually toggled a notification switch off→on in Settings; since + // most categories default on, that path never fired for typical users + // and send() silently no-op'd on every notification (permission stayed + // "undetermined"). setup() is idempotent — it won't re-prompt once the + // OS has a decision — so the ref just avoids redundant calls per session. + if (!notifPermissionRequested.current) { + notifPermissionRequested.current = true + void notifications.setup() + } } else if (!client && sseStarted.current) { sseStarted.current = false useEvents.getState().disconnect() diff --git a/app/session/[id].tsx b/app/session/[id].tsx index f9c0bc7..553c3e9 100644 --- a/app/session/[id].tsx +++ b/app/session/[id].tsx @@ -12,7 +12,7 @@ import { ActivityIndicator, Alert, } from "react-native" -import { useLocalSearchParams, Stack, useRouter } from "expo-router" +import { useLocalSearchParams, Stack, useRouter, useFocusEffect } from "expo-router" import { Ionicons } from "@expo/vector-icons" import { useSafeAreaInsets } from "react-native-safe-area-context" import { useTranslation } from "react-i18next" @@ -247,16 +247,25 @@ export default function SessionScreen() { flatListRef.current?.scrollToOffset({ offset: 0, animated }) }, []) - useEffect(() => { - if (!id) return - selectSession(id, directory).then(() => { - // Re-fetch pending permissions/questions from the server to recover from - // missed SSE events or failed optimistic removals - const connState = useConnections.getState() - const c = directory ? (connState.clientForDirectory(directory) ?? connState.client) : connState.client - if (c) refreshPending(c, id) - }) - }, [id, directory]) + // Re-select on every focus, not just mount. currentSession/messages/ + // permissions are a single global store, and the native stack keeps screens + // underneath a pushed one mounted. Without re-selecting on focus, navigating + // to another session and back would leave this screen bound to the *other* + // session's data (and its permission/question prompts) — so a user could + // approve the wrong session's tool call. useFocusEffect re-binds this screen + // to its own session whenever it becomes visible again. + useFocusEffect( + useCallback(() => { + if (!id) return + selectSession(id, directory).then(() => { + // Re-fetch pending permissions/questions from the server to recover from + // missed SSE events or failed optimistic removals + const connState = useConnections.getState() + const c = directory ? (connState.clientForDirectory(directory) ?? connState.client) : connState.client + if (c) refreshPending(c, id) + }) + }, [id, directory]), + ) // Sync model chip from latest assistant message useEffect(() => { diff --git a/src/components/chat/QuestionPrompt.tsx b/src/components/chat/QuestionPrompt.tsx index 1f8973a..d6aaeea 100644 --- a/src/components/chat/QuestionPrompt.tsx +++ b/src/components/chat/QuestionPrompt.tsx @@ -1,4 +1,4 @@ -import { useState } from "react" +import { useRef, useState } from "react" import { View, Text, TextInput, TouchableOpacity, StyleSheet } from "react-native" import { Ionicons } from "@expo/vector-icons" import { useTranslation } from "react-i18next" @@ -33,6 +33,22 @@ export function QuestionPrompt({ request, isDark, onReply, onReject }: Props) { const [showCustom, setShowCustom] = useState(false) const [current, setCurrent] = useState(0) + // A question is answered exactly once. Without this guard, a double-tap on a + // single-select option schedules two `onReply` timers; the second reply hits + // an already-resolved request server-side and surfaces a spurious + // "Reply failed" alert even though the answer went through. + const replied = useRef(false) + const reply = (a: string[][]) => { + if (replied.current) return + replied.current = true + onReply(a) + } + const reject = () => { + if (replied.current) return + replied.current = true + onReject() + } + const q = request.questions[current] if (!q) return null @@ -45,7 +61,7 @@ export function QuestionPrompt({ request, isDark, onReply, onReject }: Props) { } else { copy[current] = [label] if (request.questions.length === 1) { - setTimeout(() => onReply(copy), 100) + setTimeout(() => reply(copy), 100) } } return copy @@ -60,7 +76,7 @@ export function QuestionPrompt({ request, isDark, onReply, onReject }: Props) { setCustom("") setShowCustom(false) if (request.questions.length === 1) { - onReply(copy) + reply(copy) } } @@ -116,7 +132,7 @@ export function QuestionPrompt({ request, isDark, onReply, onReject }: Props) { - + {t("chat.questionPrompt.dismiss")} {(request.questions.length > 1 || q.multiple) && ( @@ -126,7 +142,7 @@ export function QuestionPrompt({ request, isDark, onReply, onReject }: Props) { if (current < request.questions.length - 1) { setCurrent(current + 1) } else { - onReply(answers) + reply(answers) } }} > diff --git a/src/stores/events.ts b/src/stores/events.ts index d972d2d..9256eeb 100644 --- a/src/stores/events.ts +++ b/src/stores/events.ts @@ -206,13 +206,21 @@ export const useEvents = create((set, get) => ({ // as a received response or a review-worthy success. const aborted = abortedSessions.has(sessionID) if (!aborted) track(AnalyticsEvent.ResponseReceived) - const match = useSessions.getState().sessions.find((s) => s.id === sessionID) - notify({ - category: "completed", - title: "Task completed", - body: sanitizeBody(match?.title, "Session finished processing"), - sessionId: sessionID, - }) + // Only notify "Task completed" for a genuine completion — a + // user-cancelled run didn't complete, and an errored run + // already fired its own "Session error" notification (session.error + // doesn't touch sessionStatus, so an errored session still lands + // here via busy→idle). Without this guard the user gets a + // misleading — or duplicate, contradictory — completion push. + if (!aborted && !erroredSessions.has(sessionID)) { + const match = useSessions.getState().sessions.find((s) => s.id === sessionID) + notify({ + category: "completed", + title: "Task completed", + body: sanitizeBody(match?.title, "Session finished processing"), + sessionId: sessionID, + }) + } // 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