fix: request notification permission; resync session on focus; notif + reply bugs (#121)
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 <engineer@macbookpro.lan> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
@@ -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()
|
||||
|
||||
@@ -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(() => {
|
||||
|
||||
Reference in New Issue
Block a user