Mobile push notifications for agent alerts#84
Merged
Merged
Conversation
Adds a daemon->relay->Expo path so agent notifications (needs-input, task-complete) reach mobile, matching desktop gating via notifyAlert. Includes native foreground handler + tap routing and owner-only push filtering. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…imit Move the mobile-push forward into notifyAlert so desktop and mobile alerts fire together by the same gate, instead of being wired at one of two notifyAlert call sites. Add MobilePushThrottle at the daemon funnel: collapse identical alerts within a TTL window and cap pushes per session, preventing chatty agents from flooding the device. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds an end-to-end mobile push pipeline: loopback daemon endpoint → MobilePushThrottle dedupe/rate limit → RelayClient push API → relay delivery via deliverNotificationPush to Expo → device notification taps routed by NativeNotificationRouter; updates docs and bumps OTA version. ChangesMobile Push Notification Pipeline
Sequence DiagramsequenceDiagram
participant Daemon as Daemon
participant Push as POST /internal/push
participant Throttle as MobilePushThrottle
participant Relay as RelayClient
participant RelayWS as Relay WebSocket
participant Device as Device
participant App as NativeNotificationRouter
Daemon->>Push: POST {title, body, kind, sessionId, projectId, projectRoot}
Push->>Throttle: allow({dedupeKey?, title, body, sessionId})
alt Throttled
Throttle-->>Push: false
Push-->>Daemon: {ok: true, suppressed: true}
else Allowed
Throttle-->>Push: true
Push->>Relay: pushNotification({title, body, ...})
Relay->>RelayWS: send notification_push control message to user
RelayWS->>Device: Expo push delivered
Device->>App: notification tapped (cold-start or foreground)
App->>App: extract projectRoot, sessionId
App->>App: setSelectedProject/setSelectedSession and navigate
Push-->>Daemon: {ok: true}
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/notify.test.ts (1)
32-41: ⚡ Quick winUse a fully valid
AlertEventfixture instead of casting.The
as AlertEventcast currently hides missing required fields (projectId), which can mask contract regressions.Proposed refactor
function alert(overrides: Partial<AlertEvent> = {}): AlertEvent { return { type: "alert", kind: "needs_input", + projectId: "project-1", sessionId: "claude-1", title: "claude-1 needs input", message: "waiting for input", ts: "2026-06-06T00:00:00.000Z", ...overrides, - } as AlertEvent; + }; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/notify.test.ts` around lines 32 - 41, The fixture function alert currently returns an object cast with "as AlertEvent", hiding missing required fields (e.g., projectId) and masking contract regressions; change the alert function to construct and return a fully valid AlertEvent object (no "as" cast) by including all required properties from the AlertEvent type—at minimum add projectId and any other required fields—and preserve overrides via the overrides parameter so tests use a true AlertEvent instance.src/mobile-push-bridge.ts (1)
23-23: ⚡ Quick winAvoid fully silent push-forward failures.
Swallowing all errors without even debug logging makes delivery regressions hard to diagnose.
Proposed refactor
import { requestDaemonJson } from "./daemon.js"; import type { AlertEvent } from "./project-events.js"; +import { debug } from "./debug.js"; @@ - }).catch(() => {}); + }).catch((error) => { + debug(`mobile push forward failed: ${error instanceof Error ? error.message : String(error)}`, "notify"); + }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mobile-push-bridge.ts` at line 23, The empty promise rejection handler "}).catch(() => {});" in mobile-push-bridge.ts swallows all errors silently; change it to capture the error (e.g., .catch((err) => { ... })) and log a helpful diagnostic using the existing logger (or console) including context such as the push payload/recipient and a short message (e.g., "Push forward failed"), while still avoiding crashing the process — do not remove error handling, just replace the empty catch with a call to processLogger.error/console.error that includes the error and relevant identifiers.app/components/NativeNotificationRouter.tsx (1)
20-24: ⚡ Quick winReturn trimmed value to prevent whitespace bugs.
The
stringFieldhelper validates thatvalue.trim()is non-empty but returns the original untrimmedvalue. If the notification data contains leading or trailing whitespace (e.g.,"session-123 "), this could cause mismatches when comparing with stored values or route parameters.♻️ Proposed fix
function stringField(data: unknown, key: string): string | undefined { if (!data || typeof data !== "object") return undefined; const value = (data as Record<string, unknown>)[key]; - return typeof value === "string" && value.trim() ? value : undefined; + const trimmed = typeof value === "string" ? value.trim() : ""; + return trimmed ? trimmed : undefined; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/components/NativeNotificationRouter.tsx` around lines 20 - 24, The helper stringField validates by calling value.trim() but returns the original untrimmed value, which can cause mismatches; update stringField to return the trimmed string when the check passes (e.g., compute const trimmed = (value as string).trim() and return trimmed) so callers get a normalized, whitespace‑free string; keep the same type signature and undefined behavior for non-strings or empty trimmed values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@relay/src/relay-object.ts`:
- Around line 254-273: The handleDaemonNotificationPush path currently awaits
deliverNotificationPush which can throw on network/Expo/API failures and bubble
out; wrap the deliverNotificationPush call in a try/catch inside
handleDaemonNotificationPush (after loadSecurityState and before returning) so
errors are caught and do not break daemon message processing—on catch log the
error (include context like ownerUserId and notification) via this.ctx.logger or
equivalent and do not rethrow; keep the existing early-return checks
(ownerUserId, notification.title) and still await loadSecurityState as before.
In `@relay/src/security-delivery.ts`:
- Around line 59-69: sendExpoPush currently assumes any resolved fetch is
success, so non-2xx responses (rate limits/errors) and network exceptions are
ignored; wrap the fetch in try/catch, await the Response and check response.ok,
and if not ok parse the response body (JSON) for error details and either throw
or log a descriptive error including status and body; also handle fetch
exceptions in the catch and surface them to the caller (or implement
retry/backoff for 429 if desired) so callers can react to failures. Use the
sendExpoPush function name to locate where to add the try/catch, response.ok
check, response.json() parsing, and appropriate error propagation/logging.
In `@src/daemon.ts`:
- Around line 731-733: The throttle check is executed before verifying relay
availability, so calling this.pushThrottle.allow(payload) consumes quota even
when the relay is null/disconnected; change the logic to first verify relay
availability (check this.relayClient exists and any connection/ready flag it
exposes) and only if the relay is available call
this.pushThrottle.allow(payload) and then
this.relayClient.pushNotification(payload); if the relay is unavailable return
the same 200 response without calling pushThrottle.allow to avoid burning
dedupe/session quota. Ensure you update the branch that currently calls
this.relayClient?.pushNotification after allow so that pushThrottle.allow and
pushNotification are only invoked when relayClient is present/ready.
---
Nitpick comments:
In `@app/components/NativeNotificationRouter.tsx`:
- Around line 20-24: The helper stringField validates by calling value.trim()
but returns the original untrimmed value, which can cause mismatches; update
stringField to return the trimmed string when the check passes (e.g., compute
const trimmed = (value as string).trim() and return trimmed) so callers get a
normalized, whitespace‑free string; keep the same type signature and undefined
behavior for non-strings or empty trimmed values.
In `@src/mobile-push-bridge.ts`:
- Line 23: The empty promise rejection handler "}).catch(() => {});" in
mobile-push-bridge.ts swallows all errors silently; change it to capture the
error (e.g., .catch((err) => { ... })) and log a helpful diagnostic using the
existing logger (or console) including context such as the push
payload/recipient and a short message (e.g., "Push forward failed"), while still
avoiding crashing the process — do not remove error handling, just replace the
empty catch with a call to processLogger.error/console.error that includes the
error and relevant identifiers.
In `@src/notify.test.ts`:
- Around line 32-41: The fixture function alert currently returns an object cast
with "as AlertEvent", hiding missing required fields (e.g., projectId) and
masking contract regressions; change the alert function to construct and return
a fully valid AlertEvent object (no "as" cast) by including all required
properties from the AlertEvent type—at minimum add projectId and any other
required fields—and preserve overrides via the overrides parameter so tests use
a true AlertEvent instance.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 99be8a93-91a0-4997-a256-a49ab1f787c1
📒 Files selected for processing (18)
AGENTS.mdREADME.mdapp/app.config.jsapp/app/(main)/_layout.tsxapp/components/NativeNotificationRouter.tsxapp/lib/version.tsdocs/deployment.mdrelay/src/relay-object.tsrelay/src/security-delivery.test.tsrelay/src/security-delivery.tsrelay/src/types.tssrc/daemon.tssrc/mobile-push-bridge.tssrc/mobile-push-throttle.test.tssrc/mobile-push-throttle.tssrc/notify.test.tssrc/notify.tssrc/relay-client.ts
- sendExpoPush throws on non-2xx so Expo rejections/rate-limits aren't silently dropped (security path is allSettled, so unaffected). - handleDaemonNotificationPush guards delivery in try/catch so a push failure can't break relay WS message processing. - /internal/push gates on the relay being connected before consuming throttle quota, so dropped pushes don't burn dedupe/rate budget. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
This was referenced Jun 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Bridges agent notifications to mobile push so the same alerts that fire on desktop/CLI also reach the iOS/Android app, end to end (daemon → relay → Expo → APNs).
How
notifyAlert, so desktop and mobile alerts fire together by the exact same gate (config flags + focus suppression). No more "remember to wire the forward at each call site" — the previous approach hooked one of twonotifyAlertcallers and silently skipped the other./internal/push, loopback-only); the relay (deliverNotificationPush) sends to the owner's own iOS/Android tokens via Expo.MobilePushThrottleat the daemon funnel collapses identical alerts within a TTL window and caps pushes per session, so a chatty/looping agent can't flood the device.expo-notificationsconfig plugin (entitlement), a foreground handler, and tap-to-route into the session chat.Criteria
Mobile fires iff
notifyAlertfires: master switch, per-category gates (onPrompt/onComplete/onError), and focus suppression — identical to desktop/CLI.Tests
notify.test.ts— desktop+mobile fire together and respect every gate.mobile-push-throttle.test.ts— dedup window, per-session rate cap, independence.security-delivery.test.ts— owner-only token filtering.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Chores
Tests