feat(notifications): notify on sandbox results and daemon health - #366
feat(notifications): notify on sandbox results and daemon health#366AprilNEA wants to merge 1 commit into
Conversation
The app keeps running with its window closed, so a sandbox execution that finishes and a daemon that dies were both invisible until the window was reopened. Adds the delivery layer plus the two triggers worth having first: sandbox executions (every failure, successes past 30s) and daemon health (fatal setup error, unreachable after the reconnect grace). Rules are pure values so the trigger conditions are testable without a notification centre; UserNotificationService is the only thing that touches UNUserNotificationCenter, suppresses banners for what is already on screen, and routes clicks back through the deep link router.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Greptile SummaryThe PR adds native macOS notification delivery for sandbox execution results and daemon-health transitions, including deep-link navigation and foreground suppression.
Confidence Score: 3/5The PR should not merge until notification identity and live authorization handling are corrected, because both can silently drop expected notifications. Rapid same-sandbox completions can replace one another due to whole-second identifiers, and enabling notifications after an initial denial remains ineffective until ArcBox restarts. Files Needing Attention: ArcBox/Services/Notifications/SandboxNotificationRules.swift, ArcBox/Services/Notifications/UserNotificationService.swift
|
| Filename | Overview |
|---|---|
| ArcBox/App/ApplicationCoordinator.swift | Wires notification decisions, delivery, foreground suppression, click routing, and daemon-state triggers into coordinator startup. |
| ArcBox/Services/Notifications/UserNotificationService.swift | Adds UserNotifications delivery and navigation callbacks, but permanently cached authorization can suppress delivery after a runtime permission change. |
| ArcBox/Services/Notifications/SandboxNotificationRules.swift | Implements sandbox result decisions and duration tracking, but whole-second request identifiers can collide for rapid completions. |
| ArcBox/Services/Notifications/DaemonNotificationRules.swift | Implements daemon failure-transition decisions correctly, with a repository import-order violation. |
| ArcBox/Services/SandboxEventMonitor.swift | Adds a typed per-event callback before the existing debounced list refresh. |
| ArcBox/App/DeepLink.swift | Adds canonical URL serialization for notification destinations with round-trip test coverage. |
Sequence Diagram
sequenceDiagram
participant Event as Sandbox/Daemon Event
participant Rules as Notification Rules
participant Service as UserNotificationService
participant Center as UNUserNotificationCenter
participant Router as DeepLinkRouter
Event->>Rules: state transition or completion
Rules-->>Event: AppNotification?
Event->>Service: post(notification)
Service->>Center: authorize and add request
Center-->>Service: default-action response
Service->>Router: handle(destination)
Reviews (1): Last reviewed commit: "feat(notifications): notify on sandbox r..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| AppNotification( | ||
| // Distinct per event: two executions finishing are two results, and | ||
| // the second must not silently replace the first. | ||
| identifier: "sandbox.\(event.sandboxID).\(Int(event.timestamp.timeIntervalSince1970))", |
There was a problem hiding this comment.
If two executions of the same sandbox complete within one wall-clock second, converting the event timestamp to Int gives both notification requests the same identifier, causing Notification Center to replace the first result and hide one completion notification.
| identifier: "sandbox.\(event.sandboxID).\(Int(event.timestamp.timeIntervalSince1970))", | |
| identifier: "sandbox.\(event.sandboxID).\(event.timestamp.timeIntervalSince1970)", |
Knowledge Base Used:
| case .granted: return true | ||
| case .denied: return false | ||
| case .unrequested: break | ||
| } |
There was a problem hiding this comment.
When a user initially denies authorization and later enables notifications in System Settings while ArcBox remains running, isAuthorized() continues returning the cached denial, causing every subsequent sandbox and daemon notification to be silently dropped until the app restarts.
Knowledge Base Used: App Startup Flow
| import ArcBoxClient | ||
| import Foundation |
There was a problem hiding this comment.
The new file imports ArcBoxClient before Foundation, contrary to the repository requirement that Foundation or SwiftUI imports precede local packages.
| import ArcBoxClient | |
| import Foundation | |
| import Foundation | |
| import ArcBoxClient |
Context Used: AGENTS.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1 participant
Closes ABXD-132.
The app keeps running with its window closed, so a sandbox execution that finishes and a daemon that dies were both invisible until the window was reopened. There was no
UNUserNotificationusage anywhere.This adds the delivery layer plus the two triggers worth having first. The remaining candidates (container
die, long image operations, memory pressure, disk usage) are listed in ABXD-132 as follow-ups; the layer is shaped for them but they are not implemented here.What notifies
Sandbox executions — every failure, regardless of duration or whether the app saw the run start: non-zero
exit_code, asignal, or anerrorattribute onidle(the session broke before an exit was observed). Successes notify only when therunningevent was observed and the run lasted at least 30s — a quick success finished while the user was still looking at it, and an unknown duration stays silent rather than guessing.Daemon health — a fatal setup error (
.error), and.running → .registered, whichDaemonManageronly reaches after its ~3s reconnect grace window, so transient GOAWAY/stream drops do not fire it..stopping/.stoppedare deliberately excluded: those are the states of a shutdown the user asked for.Shape
AppNotification— what to say, as a plain value.SandboxNotificationRules/DaemonNotificationRules— the decisions, pure and covered by tests. No notification centre involved.UserNotificationService— the only thing that touchesUNUserNotificationCenter. Requests authorization lazily on the first real event rather than at launch, suppresses a banner inwillPresentwhen the user is already looking at what it would announce, and routes clicks back throughDeepLinkRouter.Supporting changes:
SandboxEventMonitorgains a typedonEventhook (.sandboxChangedcarries no payload, so a subscriber cannot see the event);DeepLinkgains aurlso a destination can travel in a notification'suserInfo;SandboxEventRecordgains a direct initializer so the rules' tests do not go through protobuf.Verification
make build,make lint(0 violations) andmake testpass. 26 new cases — 14 sandbox, 10 daemon, 2 deep-link round-trip — each confirmed to actually execute inArcBoxTests, not inferred fromTEST SUCCEEDED.Notes for review
UNNotificationRequest. It belongs with the Dockerdiefollow-up, wherecompose upproduces dozens of events at once.