feat(notifications): tell me when a turn finishes - #89
Open
JairEsk wants to merge 10 commits into
Open
Conversation
Roxy went quiet when the agent finished: the only way to know a turn was done was to sit and watch it. Codex solves this with three separate mechanisms (a desktop toast, terminal OSC9/BEL, and a otify hook that shells out with a JSON payload); this takes the parts that make sense for a desktop app. - Condition: never / only-when-unfocused / always, matching Codex's ui.notification_condition. Unfocused is the default -- pinging for a turn you just watched finish is noise. - Sound, played in the renderer because main has no audio output. The bundled chime is generated by script/gen-chime.mjs, deliberately NOT the system alert: the OS toast is posted with silent: true so one event never makes two noises. - A custom sound file. The picked file is COPIED into <userData>/sounds/ and referenced by NAME thereafter, so moving or deleting the original can't break it later, and the read-back handler resolves names against that one directory (path traversal returns null rather than a file). Capped at 2 MB, with a preview button that doubles as the gesture that unlocks Chromium's autoplay policy. - The OS toast is posted from main rather than the renderer's Web Notification API, because only main can focus the window on click. The trigger reads the queue BEFORE draining it: drainQueue awaits the whole chain it starts, so notifying after it would fire once per queued prompt as the recursion unwound. Stopped turns stay silent -- the user is already there.
…ounds
The first pass shipped four controls -- a never/unfocused/always picker,
a sound toggle, a toast toggle, a volume slider -- plus a file picker
that copied a user's audio into <userData>/sounds/. That is a lot of
surface for one question ("tell me when it's done"), and most of the
combinations it allows (toast but no sound, sound only when unfocused)
are decisions nobody wants to make about a chime.
- One `notifyOnComplete` switch. Staying quiet while the window is
focused is not a preference: it is what the feature should always do,
so it lives in `shouldNotify`. Migration v24 carries the old intent
across -- 'never', or sound and toast both off, becomes off -- rather
than silently re-enabling notifications for someone who had turned
them off.
- Volume is a constant at 0.7 (the chime is mastered at -6 dBFS, so it
lands near -9 dBFS). Per-app volume is the OS's job on Windows; if the
number is wrong the fix is to change it, not to hand out a slider.
- Custom sound files are gone, and with them the copy-into-userData
dance, the path-traversal guard, the 2 MB cap and three IPC channels.
The Play button stays: it is the only way to hear what you signed up
for, and the click doubles as the gesture that unlocks Chromium's
autoplay policy.
- The toast now carries the SESSION id: clicking it opens that session
instead of dumping you on whatever was last active, which is exactly
the one you were not notified about. Shown notifications are held in a
Set until clicked or dismissed, otherwise GC can collect the handler
while the toast is still live in the Action Center.
- The toast is titled with the session name (the Windows header already
says Roxy) and renders the app icon through a ToastGeneric template --
Electron's `icon` is ignored for that slot on Windows. The AUMID is
now set directly instead of through @electron-toolkit/utils, which
substitutes process.execPath in dev and printed a full C:\Users\...
path as the toast's header.
…st one `getAllWindows()[0]` is not necessarily the app window. The agent browser (services/browser.ts) opens its own BrowserWindow with the same preload, and the main window can be closed and recreated on macOS `activate`, so the first entry can be a browser window. Clicking a completion toast then raised the browser and sent `notify:activated` to a renderer with no handler for it: the toast did nothing. The rest of main iterates every window for exactly this reason, but a broadcast is wrong here -- this handler FOCUSES what it sends to, and focusing every browser window on a toast click is worse than the bug. So `createWindow` registers the window, the same way it already hands over the icon and the auto-updater, and the ref is cleared on `closed`.
The set that keeps shown Notifications alive (so GC does not take their click handler while the toast is still in the Action Center) was only ever emptied on `click` and `close`. `close` cannot be trusted: a Windows toast that times out into the Action Center and is dismissed from there often never fires it, and macOS is no better. So every completed turn leaked one Notification for the life of the process. Cap it at 20, evicting oldest-first. Windows itself keeps only about 20 toasts per app in the Action Center, so anything past the cap is already gone from the UI and can no longer be clicked -- the eviction drops exactly the references that can no longer do anything.
notify_condition, notify_sound, notify_system_toast and notify_volume only ever existed in the first commit of this branch. No release wrote them, so no database has those rows and the step is a no-op that burns a user_version slot permanently -- the exact cost the CAUTION note at the top of this file warns about. `settings` is key-value, so `notify_on_complete` needs no schema step at all: `getSettings` defaults it to on when the row is absent. The file is now byte-identical to main again.
The notification keys landed as English in ar/de/es/fr/hi/ja/pt/ru/zh. There is precedent for that, but it is precedent for debt, and a toast is the one string a non-English user reads without having opened the app. Translated by hand rather than with `npm run i18n:translate`: that script sweeps every untranslated string in the catalog, so it would have pulled about 400 unrelated ones (cookies.*, codeHosts.*) into this diff. Those deserve their own `chore(i18n)` pass. Also drops `notifications.turnCompleteTitle`, left with no caller once the toast started using the session name as its heading.
The button exists partly so that clicking it satisfies Chromium's autoplay policy -- an app the user has never clicked in cannot play audio, and `playNotificationSound` swallows the rejection, so the first real chime is simply lost. Gating it on the switch removed that gesture from the one person who needs it: someone who just turned notifications on and touched nothing else. Playing a chime is harmless whatever the switch says.
Two macOS-only ways the toast did nothing when clicked. Both are the
same root cause: the Windows assumptions do not hold there.
`win.focus()` raises a window WITHIN the frontmost app. On macOS the
frontmost app is the editor you were in, so the window came forward
behind it and the click looked ignored. `app.focus({ steal: true })` is
the documented way to say "the user asked for this", which a toast click
is -- it is not the app interrupting.
And `window-all-closed` deliberately does not quit on darwin, so Roxy
can sit in the dock with no window at all while its toasts sit in
Notification Center. Clicking one had nothing to raise and returned
early. Now it reopens a window through the same `createWindow` the
`activate` handler uses.
That second path needs the session id to survive the window it does not
have yet. Main cannot push it: the store subscribes partway through
bootstrap, long after `did-finish-load`, so there is no moment main can
know the listener exists. So main HOLDS the id and the preload pulls it
at the instant it subscribes -- race-free by construction, since the
listener is installed on the line above.
On macOS, passing `options.icon` to `new Notification()` does not set the application icon on the left (which macOS always draws from the running `.app` bundle); it maps to `contentImage`, placing the image on the right edge of the banner as an attachment preview. In development that produced an Electron atom on the left and Roxy's avatar on the right. In production it would have shown Roxy's icon twice on the same toast (left squircle badge + right thumbnail). - Suppress `icon` on macOS so no redundant attachment thumbnail is drawn. - In `predev.mjs`, copy `build/icon.icns` into `Electron.app` on darwin so development notifications show Roxy's icon on the left instead of the default Electron atom. Packaged builds already get this from electron-builder.
Co-authored-by: Roxy <299891354+roxy-commits@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Roxy goes quiet when the agent finishes: the only way to know a turn is done is to sit and watch it. This adds a completion notification -- a chime plus a native OS toast that opens the session it fired for.
The setting is one switch
The first pass had four controls -- a never/unfocused/always picker, a sound toggle, a toast toggle, a volume slider -- plus a file picker for a custom sound. That is a lot of surface for one question ("tell me when it's done"), and most of the combinations it allows (toast but no sound, sound only when unfocused) are decisions nobody wants to make about a chime.
So:
notifyOnComplete, one row, defaults on. It is a key in the existing key-valuesettingstable, so there is no migration --getSettingsreads the row as on when absent.Staying quiet while the window is focused is not a preference either -- it is what the feature should always do -- so it lives in
shouldNotify.hasFocus()rather thandocument.hidden, because a window sitting visible behind the editor is still one you are not watching.Volume is a constant (
NOTIFY_VOLUME = 0.7). The chime is mastered at -6 dBFS, so it lands near -9 -- audible over an editor, not startling. Per-app volume is the OS's job; if the number is wrong the fix is to change it, not to hand everyone a slider.There are no custom sound files, and so no copy-into-userData dance, no path-traversal guard, no size cap, no extra IPC channels. The Play button earns its place: it is the only way to hear what you signed up for, and the click doubles as the gesture that unlocks Chromium's autoplay policy -- which is also why it is never disabled, since the person who just switched notifications on and touched nothing else is exactly the one who needs to make that gesture.
Split across the two processes
Audioelement, so rapid completions restart the chime instead of stacking copies. Never throws -- a locked autoplay policy must not take the turn's cleanup down with it.Notificationalso reaches the OS, but gives no way to focus the window on click, which is the entire point.drainQueueawaits the whole chain it starts, so notifying after would fire once per queued prompt as the recursion unwound.Clicking the toast lands on the right session
The toast carries the chat id; main sends
notify:activatedback on click so the renderer selects that session. Raising the window is not enough -- it comes back on whatever was last open, which is exactly the session you were not notified about. If the session was deleted in between, the handler refreshes and bails rather than blanking the view.The window it raises is the one
createWindowregisters, notgetAllWindows()[0]: the agent browser opens its own BrowserWindow with the same preload, so the first entry can be a browser window, and a click would then raise that and send the event to a renderer with no handler for it. The rest of main broadcasts to every window, which is right for a data push and wrong here -- this handler focuses what it sends to.Shown
Notifications are held in aSet, because one that nothing references can be garbage collected along with its click handler while the toast is still live in the Action Center. The set is capped at 20, oldest evicted first:closedoes not reliably fire for a toast dismissed from the Action Center, so an uncapped set would retain one Notification per completed turn forever. Windows keeps roughly 20 toasts per app anyway, so what gets evicted is already unclickable.macOS presentation and behavior
.appbundle. Electron'soptions.iconmaps tocontentImage, which macOS places on the right edge of the banner as an attachment preview. We omiticonon macOS so that only the clean, native app badge on the left is displayed. In development (npm run dev),predev.mjscopiesbuild/icon.icnsintonode_modules/electron/dist/Electron.appso even unbundled dev runs display Roxy's icon instead of the default Electron atom; packaged production builds (Roxy.app) receive this automatically via electron-builder.BrowserWindow.focus()only raises a window within the frontmost app. On macOS the frontmost app is the editor you were working in, so the window came forward behind it and the click looked ignored.app.focus({ steal: true })brings the entire application forward when the user clicks the toast.window-all-closeddeliberately does not quit on darwin, so Roxy can sit in the dock with no window at all while its toasts sit in Notification Center. Clicking one reopens a window throughcreateWindow.notify:takePending) whenonActivatedregisters.Windows presentation
The toast is titled with the session name -- with several sessions running, which one finished is the only thing you need from it, and the Windows header above already says Roxy. The app icon goes through a
ToastGenerictemplate withappLogoOverride hint-crop="circle"; Electron'siconoption is ignored for that slot. Text is XML-escaped, and thefile:///src isencodeURI'd because the app can sit under a path with spaces (a user folder like "Jair Escamilla") and an unencoded space silently drops the image.The AUMID is set with
app.setAppUserModelIddirectly rather than@electron-toolkit/utils, which substitutesprocess.execPathin dev -- Windows prints the AUMID verbatim as the toast header, which is how a fullC:\Users\...path ended up above every notification.silent: truealways, on all platforms: Roxy plays its own chime, and letting the OS add its default alert on top makes two noises for one event.Testing
npm run typecheck(node + web)npm run smoke:app,npm run smoke:shared(1079 checks),npm run smoke:storenpm run smoke:i18n-- 18 checks, all 590 keys resolve inenandes;npm run i18nreports every catalog in syncnpm run format:check