Skip to content

feat: notifications#126

Closed
baptisteArno wants to merge 1 commit intoDimillian:mainfrom
baptisteArno:local/notifications
Closed

feat: notifications#126
baptisteArno wants to merge 1 commit intoDimillian:mainfrom
baptisteArno:local/notifications

Conversation

@baptisteArno
Copy link
Contributor

@baptisteArno baptisteArno commented Jan 19, 2026

Adds a macOS-native notification. Closes #118

Test it manually:

  1. Start a task that will take a bit (e.g. ask the agent to run sleep 10 or a slower command).
  2. Immediately switch to another app so CodexMonitor is unfocused.
  3. Wait for the system notification.
  4. Click the notification.
  5. Expected: CodexMonitor comes to front and opens the workspace + thread that just finished.

Also pops when approval is required

@Dimillian
Copy link
Owner

Dimillian commented Jan 19, 2026

Some notes for one review loop

Findings

  • High: Notification permission is cached for the lifetime of the app. If a user denies permission and later enables it in System Settings, ensureNotificationPermission never re-checks and notifications stay disabled until restart. src/utils/pushNotifications.ts:20. Consider re-checking when permissionGranted === false (or when the toggle flips on), or adding a refresh path.
  • Medium: sendNotification(payload) isn’t awaited, so if it returns a Promise (as in the notification plugin), failures bypass the try/catch and can become unhandled rejections. src/utils/pushNotifications.ts:70. Consider await sendNotification(...) or void sendNotification(...).catch(...).
  • Medium: Completion notifications can fire twice for the same turn. Both onTurnCompleted and onAgentMessageCompleted trigger notifyAgentCompleted, and the 1.5s dedup window may not cover longer gaps. src/features/notifications/hooks/useAgentPushNotifications.ts:155 and src/features/notifications/hooks/useAgentPushNotifications.ts:162. If only one event should notify, drop one or dedup by a stable turn id instead of time.
  • Low: On macOS, WAITING_FOR_CLICK blocks subsequent click-tracked notifications until the first notification is clicked. If a user ignores it, click handling is suppressed for the session. src-tauri/src/notifications.rs:50. Consider a timeout to release the flag.

Questions/Assumptions

  • Are onTurnCompleted and onAgentMessageCompleted both expected to notify, or should one be treated as internal-only?

@baptisteArno
Copy link
Contributor Author

baptisteArno commented Jan 19, 2026

Thanks, will take a look. I'm curious how did you generated such report? /review base main? Mine is not not as complete @Dimillian

@Dimillian
Copy link
Owner

Thanks, will take a look. I'm curious how did you generated such report? /review base main? Mine is not not as complete @Dimillian

I asked for manual review of the current branch not using /review, the problem with /review is that it's very good but ten do work at the first or second reports.

@baptisteArno
Copy link
Contributor Author

Not the best impl. Will rework a new branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat: Native macOS Notifications

2 participants