Repository navigation
feat(desktop): native mobile push integration (2/3) - #19935
Conversation
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No blocking issues — two minor rough edges noted below.
Reviewed changes
- Push gateway client + session — challenge/proof handshake, session caching/negative-refusal backoff, redirect-free and deadline-bounded
fetch, register/delete/send with result-vs-throw semantics. - Delivery + registration policy —
DesktopPushServicewith serialized per-device registration, durable unregister outbox (PushUnregisterOutbox), capped retry backoff, seven-day mobile lease with explicit-renewal-only, andPushDispatcherfan-out with per-phone sound/burst filtering and dead-registration pruning. - Shared proof envelope —
host-challenge-envelope.tsextracted fromrelay-host-proof.ts(relay refactored onto it), plus the push-specific transcript validation and cross-workspace proof vector. - RPC surface —
notifications.registerPush/notifications.unregisterPush(mobile-authenticated),includeDesktopSuppressed/deliveredPushesreconciliation on the notification stream, and thenotifications.remote-push.v1capability flag. - Presence & auto-ack —
desktop-away-statethree-minute boundary, renderer auto-ack gated on native idle presence, and the completion-tracking refactor that moves desktop-notification gating into main so mobile delivery stays independent.
ℹ️ Nitpicks
src/shared/mobile-push-contract.ts,src/main/runtime/push/push-host-proof.ts, andsrc/main/runtime/push/push-gateway-client.tsall citedocs/reference/mobile-push-contract.md, but that file doesn't exist in the repo (the cloud half lives undercloud/packages/push-contract/). Either land the doc here or repoint the comments.- The new
notifications.agentStatus.*strings were added only toen.json, whilebuildAgentTaskCompleteNotificationOptionsruns through main-processtranslateMain, which never readsen.json(English comes from the fallback argument) and lazy-loads onlyes/fr/ja/ko/zh. Non-English desktops will showneeds input/working/stopped/finishedin English until those five locale files receive the keys.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughAdds end-to-end mobile push support for desktop and headless runtimes. The change adds authenticated gateway sessions, push registration persistence, durable unregister cleanup, delivery filtering, dismissal reconciliation, desktop-away detection, startup wiring, RPC methods, localized notification states, and renderer auto-ack presence checks. It also adds unit and integration coverage, documentation, reliability-gate metadata, and shared URL and protocol contracts. Priority: ➖ Normal Merge Risk: 🔵 Low · up to The changed localization and auto-acknowledgement behavior are consistent, but a narrow shutdown race may delay push-registration cleanup after shutdown. This is bounded lifecycle risk suitable for owner follow-up. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 15.63% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 64 functions across 59 files. (5 skipped: 5 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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 (2)
src/main/runtime/mobile-notification-dismissal-store.ts (1)
57-72: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winKeep
dismissedThroughmonotonic in the dismiss branch.The dismiss branch removes the entry for the same
notificationIdandnotificationEpoch, then pushes a replacement withdismissedThrough: event.notificationSeq. The prior watermark is lost. If a dismiss record arrives with a lowernotificationSeqthan one already recorded,reconcile()stops reporting the higher sequence as dismissed and the alert reappears.The notification branch already preserves the prior value at line 53. Use the maximum here for the same invariant.
♻️ Proposed fix to preserve the highest dismissed sequence
} else { + const priorDismissedThrough = kept.find(same)?.dismissedThrough ?? -1 next = kept .filter((entry) => !same(entry)) .map((entry) => entry.notificationId === event.notificationId ? { ...entry, dismissedThrough: entry.notificationSeq, expiresAt: now + RETENTION_MS } : entry ) next.push({ notificationId: event.notificationId, notificationEpoch: event.notificationEpoch, notificationSeq: event.notificationSeq, - dismissedThrough: event.notificationSeq, + dismissedThrough: Math.max(priorDismissedThrough, event.notificationSeq), expiresAt: now + RETENTION_MS }) }src/main/runtime/push/desktop-push-service.ts (1)
135-137: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winReturn a retryable reason when the service is stopped.
After the outbox await,
this.stoppedcan be true while the device scope remains'mobile'. The combined condition then returnsnot_mobile, which misclassifies shutdown and can stop client retries. Returngateway_unreachablefor the stopped case.♻️ Proposed split of the two conditions
- if (this.registry.getDevice(input.deviceId)?.scope !== 'mobile' || this.stopped) { + if (this.registry.getDevice(input.deviceId)?.scope !== 'mobile') { return { registered: false, reason: 'not_mobile' } } + if (this.stopped) { + return { registered: false, reason: 'gateway_unreachable' } + }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 92ff2f1d-86cf-492f-987c-b3463f64a015
📒 Files selected for processing (100)
config/reliability-gates.jsoncdocs/reference/headless-linux-server.mdsrc/main/global-fetch-call-site-audit.test.tssrc/main/host/electron-runtime-desktop-surface.tssrc/main/ipc/notification-burst-cooldown.tssrc/main/ipc/notification-options.tssrc/main/ipc/notifications-message-formatting.test.tssrc/main/ipc/notifications-mobile-fanout.test.tssrc/main/ipc/notifications.tssrc/main/notifications/desktop-away-state.test.tssrc/main/notifications/desktop-away-state.tssrc/main/orca-profiles/profile-cloud-auth-config.tssrc/main/orcad/orcad-entry.tssrc/main/orcad/orcad-push-startup.test.tssrc/main/runtime/device-registry.tssrc/main/runtime/host-challenge-envelope.tssrc/main/runtime/mobile-notification-dismissal-store.test.tssrc/main/runtime/mobile-notification-dismissal-store.tssrc/main/runtime/push/desktop-push-service-unreadable-outbox.test.tssrc/main/runtime/push/desktop-push-service.test.tssrc/main/runtime/push/desktop-push-service.tssrc/main/runtime/push/push-agent-state.test.tssrc/main/runtime/push/push-cleanup-auth-expiry.test.tssrc/main/runtime/push/push-delivery-policy.test.tssrc/main/runtime/push/push-device-registration-persistence.test.tssrc/main/runtime/push/push-dispatcher.test-fixture.tssrc/main/runtime/push/push-dispatcher.test.tssrc/main/runtime/push/push-dispatcher.tssrc/main/runtime/push/push-gateway-client.test.tssrc/main/runtime/push/push-gateway-client.tssrc/main/runtime/push/push-gateway-origin.tssrc/main/runtime/push/push-gateway-response.tssrc/main/runtime/push/push-gateway-session.test.tssrc/main/runtime/push/push-gateway-session.tssrc/main/runtime/push/push-host-challenge-fixtures.tssrc/main/runtime/push/push-host-proof-vector.test.tssrc/main/runtime/push/push-host-proof.test.tssrc/main/runtime/push/push-host-proof.tssrc/main/runtime/push/push-outcome-counters.test.tssrc/main/runtime/push/push-outcome-counters.tssrc/main/runtime/push/push-policy-pipeline.integration.test.tssrc/main/runtime/push/push-preferences.test.tssrc/main/runtime/push/push-register-throttle.tssrc/main/runtime/push/push-registration-races.test.tssrc/main/runtime/push/push-registration-rpc.test.tssrc/main/runtime/push/push-unpair-persistence.test.tssrc/main/runtime/push/push-unregister-outbox.test.tssrc/main/runtime/push/push-unregister-outbox.tssrc/main/runtime/relay/relay-host-proof.tssrc/main/runtime/remote-runtime-close-intent.integration.test.tssrc/main/runtime/remote-runtime-request-connection.integration.test.tssrc/main/runtime/rpc/methods/notification-preferences.test.tssrc/main/runtime/rpc/methods/notification-stream-policy.tssrc/main/runtime/rpc/methods/notifications.tssrc/main/runtime/runtime-desktop-surface.tssrc/main/runtime/runtime-mobile-notification-controller.tssrc/main/runtime/runtime-rpc-mobile-method-allowlist-fixtures.tssrc/main/runtime/runtime-rpc-mobile-terminal-streaming.test.tssrc/main/runtime/runtime-rpc-request-authorization.test.tssrc/main/runtime/runtime-rpc-websocket-long-poll-caps.test.tssrc/main/runtime/runtime-rpc/runtime-rpc-mobile-method-allowlist.tssrc/main/runtime/runtime-rpc/runtime-rpc-pairing.tssrc/main/runtime/runtime-rpc/runtime-rpc-state.tssrc/main/runtime/runtime-service-command-surface.tssrc/main/startup/main-process-push-startup.tssrc/main/startup/main-process-quit.tssrc/main/startup/main-process-relay-status.tssrc/main/startup/main-process-runtime-launch.tssrc/main/startup/main-process-state.tssrc/preload/api/notifications-bridge.tssrc/preload/api/os-permission-api.tssrc/renderer/src/components/terminal-pane/agent-task-complete-policy.tssrc/renderer/src/components/terminal-pane/parked-terminal-byte-watcher.test.tssrc/renderer/src/components/terminal-pane/parked-terminal-byte-watcher.tssrc/renderer/src/components/terminal-pane/pty-connection-notification-settings-gating.test.tssrc/renderer/src/components/terminal-pane/pty-connection-types.tssrc/renderer/src/components/terminal-pane/pty-connection/agent-task-complete-notify.tssrc/renderer/src/components/terminal-pane/terminal-pane-lifecycle-types.tssrc/renderer/src/components/terminal-pane/use-notification-dispatch.test.tssrc/renderer/src/components/terminal-pane/use-notification-dispatch.tssrc/renderer/src/hooks/agent-auto-ack-presence.tssrc/renderer/src/hooks/agent-auto-ack-targets.tssrc/renderer/src/hooks/agent-hook-completion-fresh-working.test.tssrc/renderer/src/hooks/agent-hook-completion-notifications.test.tssrc/renderer/src/hooks/agent-hook-completion-notifications.tssrc/renderer/src/hooks/agent-hook-completion-store-sync.test.tssrc/renderer/src/hooks/agent-hook-completion-store-sync.tssrc/renderer/src/hooks/ipc-events-agent-status-store-test-fixtures.tssrc/renderer/src/hooks/ipc-events-agent-status-window-test-fixtures.tssrc/renderer/src/hooks/useAutoAckViewedAgent.away.test.tssrc/renderer/src/hooks/useAutoAckViewedAgent.clock-skew.test.tssrc/renderer/src/hooks/useAutoAckViewedAgent.floating-panel.test.tssrc/renderer/src/hooks/useAutoAckViewedAgent.tssrc/renderer/src/i18n/locales/en.jsonsrc/renderer/src/web/preload-api/web-notifications-api.tssrc/shared/cloud-service-url.test.tssrc/shared/cloud-service-url.tssrc/shared/mobile-push-contract.tssrc/shared/notification-burst-cooldown.tssrc/shared/protocol-version.ts
💤 Files with no reviewable changes (2)
- src/renderer/src/components/terminal-pane/pty-connection-types.ts
- src/renderer/src/components/terminal-pane/terminal-pane-lifecycle-types.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review.
|
Too many files changed for review (109 files, 100 file limit). Bypass the limit by tagging |
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Froze the legacy socket cooldown at dispatch time —
RuntimeMobileNotificationController.dispatchnow computeslegacySocketAllowedonce before recording andcreateNotificationStreamFilterbecame a pure predicate over it, so a reconnect or replay-buffer eviction can no longer resurrect a cooldown-suppressed alert. - Split
DesktopPushService.registerfailure reasons — a stopped service now returns the retryablegateway_unreachableinstead of miscategorizing shutdown asnot_mobile. - Protected unreadable dismissal history —
MobileNotificationDismissalStorelatches anunreadableflag onEIO/EPERM/EACCES/EBUSY/EMFILE/ENFILEand skips writes so a still-intact file is never clobbered, while parse-garbage still self-heals. - Gave the floating pane precedence —
resolveAutoAckTabTargetsacks the floating workspace on colliding tab IDs, since it renders on top. - Rejected malformed persisted registration filters — non-boolean
onlyWhenDesktopAway/soundnow invalidate the row instead of being silently stripped. - Filled locale and doc gaps — added
notifications.agentStatus.*toes/fr/ja/ko/zhand repointed the contract comments fromdocs/reference/mobile-push-contract.mdtocloud/packages/push-contract/src.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 754ea4f0-c74d-4414-9420-8087e01b25b6
📒 Files selected for processing (19)
src/main/runtime/mobile-notification-dismissal-read-failure.test.tssrc/main/runtime/mobile-notification-dismissal-store.tssrc/main/runtime/push/desktop-push-service.tssrc/main/runtime/push/push-gateway-client.tssrc/main/runtime/push/push-host-proof.tssrc/main/runtime/push/push-registration-races.test.tssrc/main/runtime/rpc/methods/notification-preferences.test.tssrc/main/runtime/rpc/methods/notification-reconnect-cooldown.test.tssrc/main/runtime/rpc/methods/notification-stream-policy.tssrc/main/runtime/runtime-mobile-notification-controller.tssrc/renderer/src/hooks/agent-auto-ack-targets.tssrc/renderer/src/hooks/useAutoAckViewedAgent.floating-panel.test.tssrc/renderer/src/i18n/locales/es.jsonsrc/renderer/src/i18n/locales/fr.jsonsrc/renderer/src/i18n/locales/ja.jsonsrc/renderer/src/i18n/locales/ko.jsonsrc/renderer/src/i18n/locales/zh.jsonsrc/shared/mobile-push-contract.test.tssrc/shared/mobile-push-contract.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- src/renderer/src/hooks/useAutoAckViewedAgent.floating-panel.test.ts
- src/main/runtime/mobile-notification-dismissal-store.ts
- src/renderer/src/hooks/agent-auto-ack-targets.ts
- src/main/runtime/push/push-host-proof.ts
- src/main/runtime/push/push-gateway-client.ts
- src/shared/mobile-push-contract.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.
| if (this.stopped) { | ||
| return { registered: false, reason: 'gateway_unreachable' } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🤖 get_repo_knowledge executed:
get_repo_knowledge stablyai/orca /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4/learnings
Length of output: 10674
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- outline ---'
ast-grep outline src/main/runtime/push/desktop-push-service.ts
printf '%s\n' '--- target implementation ---'
sed -n '90,175p' src/main/runtime/push/desktop-push-service.ts
printf '%s\n' '--- relevant symbols and callers ---'
rg -n -C 3 'registerAfterCleanup|flushUnregisterOutbox|scheduleRetry|setMobilePushRegistrar|deleteQueued|stop\(\)' src/main/runtime/push/desktop-push-service.ts src/main/runtimeRepository: stablyai/orca
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- outbox drain and deleteQueued ---'
sed -n '180,285p' src/main/runtime/push/desktop-push-service.ts
printf '%s\n' '--- imports and client binding ---'
sed -n '1,45p' src/main/runtime/push/desktop-push-service.ts
printf '%s\n' '--- registrar binding definitions ---'
rg -n -C 5 'setPushRegistrar|registerPushDevice|MobilePushRegisterInput|MobilePushRegisterResult' src/main/runtime --glob '*.ts' --glob '!*.integration.test.ts'Repository: stablyai/orca
Length of output: 18127
Move the stopped guard before the pending-delete cleanup loop.
registerAfterCleanup checks this.stopped only after the loop. deleteQueued calls this.client.deleteDevice(...) for each pending item. If an in-flight register() reaches this method after stop(), it can send a gateway delete before returning. Check this.stopped before the loop.
🔧 Proposed fix
if (this.outbox.isUnreadable()) {
return { registered: false, reason: 'registration_storage_failed' }
}
+ if (this.stopped) {
+ return { registered: false, reason: 'gateway_unreachable' }
+ }
// A stable gateway ID must not inherit a delete from an earlier registration.
for (const item of this.outbox.pending().filter((entry) => entry.deviceId === input.deviceId)) {
if (!(await this.deleteQueued(item.reqId, item.registrationId))) {
this.scheduleFlushRetry()
return { registered: false, reason: 'gateway_unreachable' }
}
}
if (this.registry.getDevice(input.deviceId)?.scope !== 'mobile') {
return { registered: false, reason: 'not_mobile' }
}
- if (this.stopped) {
- return { registered: false, reason: 'gateway_unreachable' }
- }
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Corrected the
resolveAutoAckTabTargetsassertions — the tests now expect the floating target first, and the colliding-tab-id case returnsFLOATING_TERMINAL_WORKTREE_ID, matching the implementation inagent-auto-ack-targets.ts(floating pane pushed first, main tab skipped on collision). The corrected file passes 24/24. - Moved
notifications.agentStatus.*to the top-levelnotificationsnamespace — the follow-up previously nested these keys under a deeper object (beside asearchsub-object), wheretranslateMain('notifications.agentStatus.*')could not resolve them and silently fell back to English. They now sit at the top level consistent withen.json.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
ELI5
Connects the desktop runtime to Orca's native push gateway. A paired phone can register for notifications, and the desktop forwards eligible events and dismissals while preserving desktop notification settings and the three-minute away policy.
This is PR 2 of the cloud → desktop → mobile split. Cloud landed in #19912; mobile adoption follows separately.
What Changed
Why
The cloud layer needs an authenticated desktop sender and lifecycle owner. Extracts the reviewed desktop implementation from #19204 onto current main, preserving newer mainline changes. No mobile app changes or cloud infrastructure changes are included.
Linked Issue
Split from #19204; follows merged #19912.
Visual Proof
No new controls or layouts. Hidden Electron smoke verified the branch identity, rendered app shell, and real desktop-away IPC. Focus/idle acknowledgement behavior is exercised by deterministic renderer tests; a native-focus before/after recording was not performed on the user's active display. Full device acceptance belongs to the mobile layer.
Testing
Review
Review registration/outbox ownership first, then event policy and presence, then RPC/startup wiring. The shared host challenge envelope reuses the existing Relay proof primitive. The original combined branch remains preserved; mobile source stays for PR 3.
Agent skill upstream boundary
Checklist
Diff size
100 files; +5,457/-349. Tests/fixtures: +3,136/-32; implementation/docs/config: +2,321/-317. Counts include comments and blank lines.
Review fixes
973197ae7dfpreserves the legacy socket eligibility decision at event recording, so reconnect cannot resurrect a cooldown-suppressed alert even after earlier buffer entries are evicted. Removes per-subscription cooldown recalculation. Also protects unreadable dismissal history, rejects malformed persisted boolean preferences, gives floating panes precedence on colliding tab IDs, reports shutdown as retryable, and fills five locale translations.159 focused notification tests passed, all root typechecks and changed-code quality passed. New HEAD hosted CI pending. Existing-client regression runs the real controller and subscription/replay RPC handlers. No new deployment or physical-device test.