feat(mobile): notify by oversight category with deep links, quiet hours and a stuck threshold - #1653
Conversation
janicduplessis
left a comment
There was a problem hiding this comment.
Fresh review of the phone half (diff vs feat/1644-oversight-notifications). tsc --noEmit, npm test (158 passing), npm run lint and npm run format:check pass in apps/mobile.
-
bug
apps/mobile/src/hooks/notifications.tsx:328-330- taps are deduplicated byrequest.identifieralone (HANDLED_KEY). Before this PR every local notification had a fresh identifier; now they are stable on purpose (<mac>:looping-ios:<path>,summary), and pushes carry an APNscollapseId, which iOS uses as the request identifier. So after the user taps a loop notification, the next episode's loop notification for the same workspace (or the nextsummary) replaces it under the same identifier, and tapping it does nothing:storage.getString(HANDLED_KEY) === idreturns early until some other notification is tapped. Fix: key the marker by identifier plus delivery time, e.g.const id = `${response.notification.request.identifier}:${response.notification.date}`. -
should-fix
apps/mobile/src/hooks/notifications.tsx:308-312- against a stim-server from before #1648,push.registerwith the new event names is refused (bad-request), and the app then marks the Mac as not pushing and notifies locally. The Mac's registry still holds the registration the phone made before the update (old events:build-failed,log-errors,disk,app-stopped,slow-build), and a refused register does not replace it. So that Mac keeps pushing the raw events this PR removes, and low disk arrives twice: once as the olddiskpush and once as the localmachinenotification. The PR's Risk section says this is the existing fallback path, but that path assumed the old registration was gone. Fix: when the refusal isbad-request, sendpush.unregisterso the old server stops pushing. The other option is to retry with the legacy names, which the new server also accepts (LEGACY_PUSH_EVENTS). -
nit
apps/mobile/src/screens/settings.tsx:145/settings.ios.tsx:187- Stuck after shows even when Agent looks stuck is off, where it has no effect. Show it only whenprefs.categories.includes('stuck'). -
nit
apps/mobile/src/hooks/notifications.tsx:243-timeZoneis read on each render, so a phone that changes time zone re-registers only after some unrelated re-render (a connection change). This is harmless in practice. I'm mentioning it only because the PR says it "re-registers when any of those change".
Checked with no finding:
- Per-machine
link:/status:scopes. The link scope always runs withstatus: null, and the status scope withlink: null. - A pushed machine's status scope runs with no categories, so
lastingmarks problems settled andeventdrops them. No backlog if pushing stops. - The summary's quiet flag.
- Quiet hours via
inQuietHours, with the 30 s tick picking up their end. parsePrefsmigration and ranges, which match the server schema (1-240).- The
urlallowlist: the regex anchors the host, and a non-matching url falls back to the workspace. device/buildroutes and their params matchsrc/app/mac/[id]/device.tsxandbuild.tsx.- The foreground handler. iOS serializes
interruptionLevel. On Android, localsound: falseserializes to null, and the LOW channel prevents heads-up. - App
PushRegisterParamsagainstpackages/server/src/protocol.ts. - README and footer, against
oversight.tsbehavior. - Tests assert notifier output and routes at the right boundary, with no constant or pass-through tests. No test covers finding 1, which sits in the hook.
|
Review follow-up:
|
janicduplessis
left a comment
There was a problem hiding this comment.
Fresh review of b79d8c8 and of the phone diff vs feat/1644-oversight-notifications. No actionable findings.
typecheck, format:check, lint and npm test (158 passing) pass in apps/mobile.
What I checked in the fix commit:
- Tap marker: it is now
<identifier>@<date>. A later notification that reuses an identifier (a stable local id,summary, or an APNscollapseId) has a new delivery date, so a tap on it opens its route. A re-render with the same response still has the same key and is still ignored. bad-requestunregister: the pre-#1648registerPush(a24deffserver.ts) refuses unknown events withbad-request, and itspush.unregisterhandler takes{}, so the old registration is removed. The unregister runs only whilecurrent()holds. A newer registration sent in the meantime therefore can't be undone by a stale refusal, and on one socket the unregister is ordered before any later register. A new server that refuses a register for another reason also gets unregistered. That matchessetPushed(false)and local notifying, so the phone doesn't get the same event twice.- Stuck after: now shown only while
stuckis among the categories, on both settings screens.
The rest of hooks/notifications.tsx and lib/notifications.ts had no new issue. One note that needs no change: on the first launch after the update against an old server, the stored pushed: flag keeps the status scope silent until the refusal arrives. A problem already present in that window is marked settled and isn't notified locally. This happens once, and the flag is cleared on refusal.
b79d8c8 to
cfcc7f6
Compare
cfcc7f6 to
3bcdf89
Compare
janicduplessis
left a comment
There was a problem hiding this comment.
Fresh review of 3bcdf89 (web in notification routing and the attention strip).
No actionable findings.
notificationRouteacceptsplatform: 'web'for device targets.app/mac/[id]/device.tsxfrom #1650 accepts web, so a webstartedorstucknotification opens the page in the device viewer. Abuildtarget with web falls through to the workspace, which is right because web has no builds.NotificationData.platformand the device route params useDevicePlatform, which matches the server's widenedPushTarget.- The attention strip's
page-webitem no longer carries the removed push-event fields, and still opens the workspace. The existing attention test for a failed web page covers it. pnpm exec tsc --noEmitpasses in apps/mobile,npm test -- src/libpasses (159 tests), andoversight-agreement.test.tspasses.
…rs and a stuck threshold Local notifications come from the rules stim-server pushes with, per category, updated in place under a stable identifier, quiet for work started, and open the device viewer, build details, a pull request, the workspace or the machine sheet. Settings gets a switch per category, the stuck threshold and quiet hours. Fixes #1645.
Handle a tap on a later notification that reuses an identifier, unregister from a stim-server that refuses the new events so it stops pushing the old ones, and show the stuck threshold only while stuck notifications are on.
…he web platform landed
3bcdf89 to
85798a4
Compare
Description
Stacked on #1648, which adds the shared rule module (
apps/mobile/src/lib/oversight.ts, identical to the server's) and makes stim-server push by oversight category. The diff shown here is only the phone side.The app still notified locally on raw attention events: a failed build, new log errors, low disk, offline, a stopped app, a slow build. Settings had one switch per raw event plus an "only workspaces an agent drives" filter. A tap could open only home, the machine sheet, a workspace or its errors. Every notification played a sound, and each one stacked under a new identifier.
Solution
Local notifications come from the shared rules.
localNotificationsrunsoverseeper machine. Each machine gets two state scopes, as before:link:<id>for the connection (offline / refused / unpaired, which only the phone sees) andstatus:<id>for everything the status carries. Local notifications cover foreground use and servers that don't push. For a machine that acceptedpush.register, the status scope still runs but with no categories, so it records state without notifying. That way the phone never repeats a push, and doesn't dump a backlog if pushing stops. Without push there are no PR lookups or control conflicts; git'smergedIntostands in for "merged".Delivery. Each local notification is scheduled under a stable identifier (
<mac>:<category>:<path>), so a later episode replaces the earlier notification instead of stacking. Work started is quiet:interruptionLevel: 'passive'and no sound on iOS, and a new low-importance "Work started" channel on Android. It usesthreadIdentifier: started:<Mac>. The foreground handler now follows each notification, local or pushed: no banner or sound for passive ones, and sound only when the notification has one.Deep links. Notification data gains the
device,buildandurltargets the server sends.urlopens in the browser only when it matcheshttps://github.com/<owner>/<repo>/pull/<n>. Anything else falls back to the workspace, so a push can't make the phone open an arbitrary link.Settings. Settings gets a switch per category, Stuck after (5 to 60 min, default 15), Quiet hours (off or three night ranges) and a rewritten footer.
push.registernow sends the categories,stuckMinutesandquietHourswith the phone's IANA time zone. It re-registers when any of them change or the connection reopens. Stored preferences from before this change keepenabledand get every category.Attention strip. The strip keeps showing failed builds, log errors, stopped apps and slow builds.
HomeAttentionItemloses the fields that existed only for notifications (event,occurrence,reason,driven,count), andsrc/lib/notify.tsgoes away.Risk
push.registerwith the new event names is refused asbad-request. The app then treats that Mac as not pushing, sendspush.unregisterso the old registration stops pushing the raw events, and notifies locally.expo-notificationsalready ships, so this is OTA-compatible with the current TestFlight build. The Android channels are created whenever the notifier starts, so a phone that turned notifications on before gets the quiet channel too.Test plan
src/lib/notifications.test.tscovers:urlfalling back to the workspaceSTIM_HOME. I drove it with agent-device against the mock server on port 7797, whose--overlayI changed step by step:janic/wide-insets/Mock Mac/Same Swift error 3x at AppDelegate.swift:71. A fourth failure did not notify again.No agent activity for 5 min; iPhone 18 Pro 27.0 still uparrived at the threshold.Agent stopped after a green iOS build.3.0 GB free, below Stim's floor.AppDelegate.swift:99:9diagnostic. Tapping the disk banner opened the machine sheet.stim-serverfrom this stack on port 7911.push.registerstored all six categories,stuckMinutes: 5and a real Expo token.control.begin { takeOver: true }. The phone showed "Control ended. Test iPad took over this device." and the server sent the control push to Expo without a ticket error.activity.recent.Fixes #1645