Repository navigation
fix(notifications): stop clearing phone alerts that were never sent - #37095
Conversation
Reading an agent on the desktop rebuilt notification ids for turns that never alerted (every chat pane, silenced workers) and forwarded each one to the phone as a silent dismiss push plus a dismissal-store record. Only forward a clear when the store holds a live delivery for that id.
Findings
 |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Description check |  |
The description clearly explains the user impact, implementation, rationale, testing, and compatibility considerations. However, the required Linked Issue section states "None" and does not provide an… | Add an actual linked issue in the Linked Issue section, such as an applicable pre-existing issue. If the contributor is an internal maintainer, attach the relevant existing issue instead of leaving the section as "None". |
| Docstring Coverage | |
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly and concisely describes the main change: preventing dismissal events for phone alerts that were never sent. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Description check
-
Explanation
The description clearly explains the user impact, implementation, rationale, testing, and compatibility considerations. However, the required Linked Issue section states "None" and does not provide an actual issue link.
Resolution
Add an actual linked issue in the Linked Issue section, such as an applicable pre-existing issue. If the contributor is an internal maintainer, attach the relevant existing issue instead of leaving the section as "None".
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 1 functions across 3 files.
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
Finishing Touches
-
Generate docstrings
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
-
-
- Source mode: Auto
- Repository: Unavailable
- Branch: Unavailable
- File: No configuration file from this repository applied
- Commit: Unavailable
- Layers: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
0d31dd34-f9aa-4ae0-8499-8e7ca1328d0d
-
src/main/ipc/notifications-subject-retirement.test.tssrc/main/runtime/mobile-notification-dismissal-store.tssrc/main/runtime/runtime-mobile-notification-controller.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 2 remain after this review.
-
| hasLiveDelivery(notificationId: string): boolean { | ||
| const now = Date.now() | ||
| return ( | ||
| this.unreadable || |
There was a problem hiding this comment.
Treat malformed history as uncertain.
If the saved JSON is malformed, JSON.parse throws, but isUnreadableError returns false for the parse error. The store then reports no live deliveries and suppresses clears for pre-restart alerts. Treat an invalid snapshot as unknown history and forward those clears. (raw.githubusercontent.com)
| } | ||
|
|
||
| function isLive(entry: RecordEntry, now: number): boolean { | ||
| return entry.expiresAt > now && entry.dismissedThrough < entry.notificationSeq |
There was a problem hiding this comment.
Do not expire undismissed deliveries after seven days.
A phone can still show an alert after its store entry expires. A desktop read then skips its clear, even when the entry was not evicted. Keep undismissed entries until they are retired or reach the accepted capacity limit. Also update the pruning in record. (developer.apple.com)
…tablyai#37095) Reading an agent on the desktop rebuilt notification ids for turns that never alerted (every chat pane, silenced workers) and forwarded each one to the phone as a silent dismiss push plus a dismissal-store record. Only forward a clear when the store holds a live delivery for that id.
…tablyai#37095) Reading an agent on the desktop rebuilt notification ids for turns that never alerted (every chat pane, silenced workers) and forwarded each one to the phone as a silent dismiss push plus a dismissal-store record. Only forward a clear when the store holds a live delivery for that id.
…tablyai#37095) Reading an agent on the desktop rebuilt notification ids for turns that never alerted (every chat pane, silenced workers) and forwarded each one to the phone as a silent dismiss push plus a dismissal-store record. Only forward a clear when the store holds a live delivery for that id.
ELI5
When you read an agent on the desktop, Orca also tells your phone to clear that agent's notification. It did this even when the phone never got a notification, so the phone kept receiving silent "clear" pushes for nothing. Now Orca only sends the clear when it actually sent the notification.
What Changed
Before: marking an agent read rebuilds a notification id from the pane and turn start, and main forwarded every one of those ids to the phone. Each forward was a silent dismiss push and a tombstone in
mobile-notification-dismissals.json. Chat panes never deliver under those ids (their alerts useagent-attention:ids), and since #36968 dispatched workers don't alert at all, so most of these were for alerts that never existed.After:
RuntimeMobileNotificationController.dismissforwards a clear only when the dismissal store holds a live delivery for that id. The store is persisted, so alerts sent before a restart still clear. If the store couldn't be read at startup it answers "maybe", and the clear goes out as before.On my install, 2,897 of the 4,096 store slots (the cap) were tombstones for ids with no delivery record: 1,097 from chat panes alone.
Why
The store already records every phone delivery for reconnect reconciliation, so it's the one place that knows whether a clear has anything to clear. Stopping the renderer from rebuilding ids would miss the worker case and any other pane that was read without alerting, and the rebuild is still needed for alerts sent before a restart.
Linked Issue
None. Found while auditing notification noise for #36968 / #36992.
Visual Proof
N/A: nothing visible changes. The phone just stops receiving silent clear pushes for notifications it never showed.
Testing
Added a test in
notifications-subject-retirement.test.ts: an alert delivered before a restart still clears once, and a read for an id that was never sent forwards nothing. It fails on main. All controller and dismissal tests pass (11 files), pluspnpm tc:nodeandpnpm lint.AI Disclosure
Review
Agent skill upstream boundary
Notes
Mobile: no protocol change; phones of any version just see fewer dismiss events. Remote/serve hosts gate on their own store the same way. A delivery evicted from the 4,096-entry store would no longer clear; with tombstones no longer filling it, a week of real alerts sits well under the cap.
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)