Skip to content

feat: notification_all_screens config + event-driven overlay dismissal#416

Merged
garysheng merged 5 commits intoPeonPing:mainfrom
ctsstc:feat/notification-all-screens-config
Mar 30, 2026
Merged

feat: notification_all_screens config + event-driven overlay dismissal#416
garysheng merged 5 commits intoPeonPing:mainfrom
ctsstc:feat/notification-all-screens-config

Conversation

@ctsstc
Copy link
Copy Markdown
Contributor

@ctsstc ctsstc commented Mar 29, 2026

Summary

Add a new config option to display notifications on all screens or only the active screen.

Reason

Themes only show on the active screen.
The default theme/no theme selected shows the notification on all screens.
Currently there's no parity across the system, this brings it all together and lets the user choose behavior.

Changes

  • Add new config option: notification_all_screens
    • Add new config to migration scripts
      • Keep previous functionality
        • If using the default theme default to: true
        • If using one of the themes default to: false
  • Add IPC via: NSDistributedNotificationCenter for notification dismissal

@vercel
Copy link
Copy Markdown

vercel Bot commented Mar 29, 2026

@ctsstc is attempting to deploy a commit to the Gary Sheng's projects Team on Vercel.

A member of the Team first needs to authorize it.

@ctsstc ctsstc closed this Mar 30, 2026
@ctsstc ctsstc force-pushed the feat/notification-all-screens-config branch from 99bd439 to 0db2652 Compare March 30, 2026 00:03
@ctsstc ctsstc reopened this Mar 30, 2026
@ctsstc ctsstc marked this pull request as ready for review March 30, 2026 00:04
@garysheng
Copy link
Copy Markdown
Collaborator

Thanks for this — the notification_all_screens config and event-driven dismissal via NSDistributedNotificationCenter are solid additions, and the new mac-overlay.bats tests are thorough.

One issue blocking merge: test 537 in peon.bats now fails because the PR appends all_screens as a new last argument to the overlay call. The existing test uses awk '{print $(NF-2)}' to extract ide_pid, expecting notif_position to be the last field — but with the new arg, NF is now all_screens, shifting ide_pid to NF-3.

Fix needed in tests/peon.bats around line 2879:

# Update the comment and offset to account for the new all_screens arg at the end
# Fields from end: ... ide_pid session_tty notif_position notify_type all_screens
# ide_pid is NF-4 (or adjust to use a fixed field index)
ide_pid=$(tail -1 "$TEST_DIR/overlay.log" | awk '{print $(NF-4)}')

The Windows respects custom debug_retention_days from config failure is pre-existing on main and not introduced by this PR.

Once the peon.bats fix lands, this is ready to merge.

@ctsstc ctsstc force-pushed the feat/notification-all-screens-config branch from 9942fbe to 37d76b2 Compare March 30, 2026 16:24
@garysheng
Copy link
Copy Markdown
Collaborator

The Windows CI failure ("respects custom debug_retention_days from config") was a pre-existing flaky test caused by hardcoded dates — not related to your changes. It was just fixed in #420 which merged moments ago.

Could you rebase your branch onto the latest main? That should bring in the fix and get Windows CI to green.

The feature logic itself looks solid — looking forward to getting this merged once CI is clean.

ctsstc and others added 5 commits March 30, 2026 14:42
Add notification_all_screens config option (default: false). When true,
overlay notifications appear on all connected displays simultaneously,
coordinated via NSDistributedNotificationCenter — no polling or temp
files needed. Clicking any instance dismisses all siblings instantly.

- config.json: add notification_all_screens key with auto-migration
- peon.sh: read/export NOTIF_ALL_SCREENS, config migration
- notify.sh: spawn one overlay per screen in all-screens mode
- mac-overlay*.js: replace file-based IPC with distributed notifications

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add BATS tests for the notification_all_screens feature:
- config migration adds the key when missing
- config migration preserves existing value
- overlay receives false by default (single-screen mode)
- overlay receives true when all_screens config is enabled
Default overlay always showed on all screens, while themed overlays
(glass/jarvis/sakura) only showed on the focused screen. The migration
now checks overlay_theme to infer the right default so existing
behavior is preserved on upgrade.
The notification_all_screens arg appended to overlay calls shifted
ide_pid from NF-2 to NF-4 in the overlay.log output.
@ctsstc ctsstc force-pushed the feat/notification-all-screens-config branch from 37d76b2 to 61a1a94 Compare March 30, 2026 21:42
@ctsstc
Copy link
Copy Markdown
Contributor Author

ctsstc commented Mar 30, 2026

@garysheng looks good to go now :shipit:

@garysheng garysheng merged commit fe0da1d into PeonPing:main Mar 30, 2026
2 of 3 checks passed
@ctsstc ctsstc deleted the feat/notification-all-screens-config branch March 31, 2026 00:58
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.

2 participants