Repository navigation
feat(desktop): notify when a background design run finishes - #472
Conversation
Show a system notification when a live run settles as completed or failed, or when the agent asks a question, while the window is unfocused. Clicking it focuses the window and opens the design. Settings > Advanced can turn it off; preferences move to schema version 10 with the setting on by default.
There was a problem hiding this comment.
Findings
- [Minor]
notifyDesignInBackgroundcan reject and silently drop the notification —apps/desktop/src/renderer/src/lib/system-notifications.ts:19awaitsapi.preferences.get()and then constructsnew Notification(...), and both call sites fire it fire-and-forget withvoid(apps/desktop/src/renderer/src/hooks/useAgentStream.ts:514,apps/desktop/src/renderer/src/App.tsx:76). If the preferences IPC rejects or theNotificationconstructor throws (e.g. a restricted environment), the promise rejects unhandled and no notification appears with no signal. Notifications are a best-effort path so this is not a merge blocker, but a.catchkeeps failures diagnosable instead of showing up as an unhandled rejection in the renderer console.
Suggested fix:// at each call site void notifyDesignInBackground(useCodesignStore.getState(), event.designId, kind).catch((err) => { console.warn('[notifications] failed to show system notification', err); });
Questions
- Does
apps/desktopinstall asession.setPermissionRequestHandler(orsetPermissionCheckHandler) in the main process? If one is present and does not allownotifications,new Notification(...)in the renderer can be blocked and the whole feature would silently no-op for packaged users, regardless of the Settings toggle. I could not confirm this from the changed files — worth verifying before merge.
Summary
- Review mode: initial
- No blockers or Major issues found. The change is small, narrowly scoped, and follows the repo's conventions: the new preload method (
focusWindow) is paired with a main-process handler (apps/desktop/src/main/ipc/shell.ts:29), the preference is added through the existingDEFAULTS/readPersistedBoolean/PREFERENCE_UPDATE_FIELDSplumbing, and the schema bump to 10 (apps/desktop/src/main/preferences-ipc.ts:20) preserves the default-on behavior for older files viareadPersistedBooleanreturning the default when the field is absent — which is also asserted by the newpreferences-ipc.test.tscases. - The PR correctly uses
Refs #411(partial work) rather than a closing keyword and explicitly states it does not change the timeout, so there is no completion claim to validate against the linked issue. - Good coverage of the tricky branches: canceled/interrupted runs and
replayevents are excluded from notifying (useAgentStream.ts:514), and the helper tests cover focused-window suppression, the disabled setting, distinct done/failed/ask bodies, and the unknown-design fallback. - Residual risks (not blocking): the author notes the notification has never been exercised in a running desktop app or against a live model, and the macOS case where all windows are closed (no renderer) is a documented limit. The exact notification-delivery path (permission state, packaged vs. dev behavior, click-to-focus while the app is hidden) is therefore unverified in CI.
Testing
- Not run (automation). New coverage looks proportional:
system-notifications.test.ts,useAgentStream.completion.test.ts(completed/failed vs. cancelled/interrupted in the background), and thepreferences-ipc.test.tsround-trip/default/rejection cases. The only gap worth noting is that theApp.tsxwiring for the agentaskpath is not directly unit-tested —system-notifications.test.tscovers the'ask'body but not theask.onRequestsubscription inApp.tsx:76.
Open-CoDesign Bot
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The notification helper can produce foreground notifications and unhandled preference-read rejections, and the new checkbox lacks an accessible name.
Review effort: Balanced
Findings: 2
Open (2)
What changed in this PR
Adds configurable system notifications for background design runs and agent questions.
Changes:
- Adds localized completion, failure, and question notifications.
- Adds an Advanced preference, persisted under schema version 10.
- Focuses the app and opens the relevant design when clicked.
| File | Description |
|---|---|
.changeset/system-notifications.md |
Records the user-visible feature. |
apps/desktop/src/main/ipc/shell.ts |
Adds window-focus IPC. |
apps/desktop/src/main/preferences-ipc.ts |
Persists and validates the notification preference. |
apps/desktop/src/main/preferences-ipc.test.ts |
Tests defaults, validation, and persistence. |
apps/desktop/src/preload/index.ts |
Exposes preference and focus APIs. |
apps/desktop/src/renderer/src/App.tsx |
Handles question notifications. |
apps/desktop/src/renderer/src/components/settings/AdvancedTab.tsx |
Adds the notification setting. |
apps/desktop/src/renderer/src/components/settings/MemoryTab.tsx |
Updates the shared preference defaults. |
apps/desktop/src/renderer/src/hooks/useAgentStream.ts |
Handles run-settlement notifications. |
apps/desktop/src/renderer/src/hooks/useAgentStream.completion.test.ts |
Tests eligible run outcomes. |
apps/desktop/src/renderer/src/lib/system-notifications.ts |
Implements notification display and click behavior. |
apps/desktop/src/renderer/src/lib/system-notifications.test.ts |
Tests notification behavior. |
packages/i18n/src/locales/en.json |
Adds English strings. |
packages/i18n/src/locales/es.json |
Adds Spanish strings. |
packages/i18n/src/locales/pt-BR.json |
Adds Brazilian Portuguese strings. |
packages/i18n/src/locales/zh-CN.json |
Adds Simplified Chinese strings. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| <input | ||
| type="checkbox" | ||
| checked={prefs.systemNotifications} | ||
| onChange={(e) => void updatePref({ systemNotifications: e.target.checked })} | ||
| className="h-4 w-4 accent-[var(--color-accent)]" | ||
| /> |
| const prefs = await api.preferences.get(); | ||
| if (!prefs.systemNotifications) return; |
Read the System notifications preference before checking window focus, so a user who returns while the setting is loading is not notified. Callers log a failed preference read instead of leaving an unhandled rejection, and the Advanced checkbox now has an accessible name.
There was a problem hiding this comment.
Findings
No Blocker or Major issues found. One small robustness nit, listed first so it is not missed.
- [Nit] The notification click handler still fire-and-forgets its IPC calls —
apps/desktop/src/renderer/src/lib/system-notifications.ts:27-28callsvoid api.focusWindow()andvoid state.switchDesign(design.id).focusWindowis aPromise<void>(ipcRenderer.invoke('codesign:v1:focus-window')inapps/desktop/src/preload/index.ts), so a rejection surfaces as an unhandled renderer rejection, and a failedswitchDesignmakes the click look like a no-op with no signal. This is the same class as the previousvoid notifyDesignInBackground(...)finding, which was correctly fixed at both call sites (apps/desktop/src/renderer/src/hooks/useAgentStream.ts:514,apps/desktop/src/renderer/src/App.tsx:77); the handler body was not updated. Non-blocking and rare in practice (thefocus-windowhandler does not throw), so feel free to defer.
Suggested fix: add a.catch((err) => console.warn(...))toapi.focusWindow()(and toswitchDesignif it is promise-returning), mirroring the call-site logging.
Questions
- Carried over from the initial review and still unverified: does the desktop app install a
session.setPermissionRequestHandler/setPermissionCheckHandlerthat could deny the renderernotificationspermission? Electron's default is to allow, so if no custom handler exists this works; if a handler exists and does not allownotifications,new Notification(...)silently no-ops for packaged users regardless of the Settings toggle. Not determinable from the changed files.
Summary
- Review mode: follow-up after new commits
- The one previous Minor is resolved: both call sites now
.catchandconsole.warninstead of using barevoid(useAgentStream.ts:514,App.tsx:77), matching the existing renderer logging pattern. The follow-up also moved the focus check to after the preferences read (system-notifications.ts:19-21) and added a test for "user returns while the setting is being read", plus anaria-labelon the checkbox. All good. - No Blockers or Majors. The schema bump to 10 (
apps/desktop/src/main/preferences-ipc.ts:20), default-on for older files viareadPersistedBooleanreturning the default whensystemNotificationsis absent, the new preloadfocusWindowpaired withcodesign:v1:focus-window(apps/desktop/src/main/ipc/shell.ts:29), and the i18n keys across en/es/pt-BR/zh-CN all follow existing patterns and are covered by tests. - The
Refs #411framing remains correct — this is partial work (notification on settle) and the timeout is unchanged, so there is no completion claim to validate against the issue. - Residual risk (non-blocking, unchanged): the notification has not been exercised in a running app or against a live model; the author documents the macOS "last window closed → no renderer" gap. Default-on for existing installs is intentional and stated in the changeset.
Testing
- Not run (automation). Coverage looks proportional:
system-notifications.test.ts(title/body, click → focus + switch, focused-window suppression, disabled setting, unknown-design fallback, user-returns-during-read),useAgentStream.completion.test.ts(completed/failed notify vs cancelled/interrupted/replay), andpreferences-ipc.test.ts(default-on, round-trip, non-boolean rejection). Remaining gaps: no unit test for theApp.tsxask.onRequestwiring, and no runtime/E2E verification that the OS notification actually appears.
Open-CoDesign Bot
Focusing the window and switching designs are now awaited together, and a failure is logged like the notification call sites instead of becoming an unhandled rejection.
There was a problem hiding this comment.
Findings
No Blocker, Major, or Minor findings. The single previous Nit is resolved by this follow-up.
Summary
- Review mode: follow-up after new commits
- The earlier Nit (the click handler fire-and-forgetting its IPC calls) is fixed.
apps/desktop/src/renderer/src/lib/system-notifications.ts:26-30now wraps both calls inPromise.all([...]).catch(...)and logs withconsole.warn, matching the existing call-site pattern inapps/desktop/src/renderer/src/hooks/useAgentStream.ts:514andapps/desktop/src/renderer/src/App.tsx:77. The new testlogs a failed click instead of leaving an unhandled rejection(apps/desktop/src/renderer/src/lib/system-notifications.test.ts:104) exercises the rejection path and confirmsswitchDesignstill runs whenfocusWindowfails. - Happy-path behavior is unchanged:
focusWindowis invoked first,switchDesignsecond; both still start together, andundefinedin thePromise.allarray for the unknown-design case resolves normally. - No new material issues are introduced by the follow-up diff. The rest of the change (schema bump to 10, default-on for older files via
readPersistedBooleanreturning the default when the key is absent, thecodesign:v1:focus-windowhandler inapps/desktop/src/main/ipc/shell.ts:29, and the i18n keys in en/es/pt-BR/zh-CN) still follows existing patterns and is covered by tests. Refs #411framing remains correct — this is partial work (notify on settle), the timeout is unchanged, so there is no completion claim to validate.
Residual risk (non-blocking, not merge blockers):
- The notification path has not been exercised in a running app or against a live model, and the author documents the macOS "last window closed → no renderer" gap. Default-on for existing installs is intentional and stated in the changeset.
- Carried over from the prior review and still unverifiable from the changed files: if the main process ever installs a
session.setPermissionRequestHandler/setPermissionCheckHandlerthat deniesnotifications,new Notification(...)would silently no-op regardless of the Settings toggle. The author states no handler is installed; worth a one-time manual check before wide rollout, but it does not block this PR.
Testing
- Not run (automation). Coverage looks proportional:
system-notifications.test.ts(title/body, click → focus + switch, focused-window suppression, settings off, unknown-design fallback, user-returns-during-read, and now the click-rejection logging),useAgentStream.completion.test.ts(completed/failed notify vs cancelled/interrupted/replay), andpreferences-ipc.test.ts(default-on, round-trip, non-boolean rejection). Remaining gap: no unit test for theApp.tsxask.onRequestwiring, and no runtime/E2E verification that the OS notification actually appears.
Open-CoDesign Bot
Windows shows toasts only for the AppUserModelID on the installer's Start Menu shortcut, which electron-builder sets to the appId.
There was a problem hiding this comment.
Findings
No Blocker, Major, Minor, or Nit findings. The only change since the prior bot review (18946b6 → b62d768) is the Windows AppUserModelID line added at apps/desktop/src/main/index.ts:227-229.
Questions
- The new line hardcodes
app.setAppUserModelId('ai.opencowork.codesign'), and its comment states the value must match theappIdelectron-builder writes onto the installer's Start Menu shortcut. ThatappIdis not part of this diff and was not surfaced in the public files available to this review, so I could not verify the two match. Can you confirmbuild.appIdin the electron-builder config is exactlyai.opencowork.codesign? If the repo already exposes the app id through a shared constant (e.g.BRANDfrom@open-codesign/shared, already imported atapps/desktop/src/main/index.ts:4), referencing it here would keep the runtime id and the shortcut id from drifting apart.
Summary
- Review mode: follow-up after new commits
- One change since the previous review:
apps/desktop/src/main/index.ts:227-229sets the Windows AppUserModelID to the app id, which is the standard requirement for rendererNotifications to surface as Windows toasts. The call sits inside the!IS_VITESTsingle-instance block at module load, beforecreateWindow(), so it runs early enough and does not affect Vitest (guarded by both!IS_VITESTandprocess.platform === 'win32'). - The rest of the PR is unchanged from the previously reviewed state: schema bump to 10, default-on for older
preferences.jsonviareadPersistedBoolean, thecodesign:v1:focus-windowhandler (apps/desktop/src/main/ipc/shell.ts:29),notifyDesignInBackground(apps/desktop/src/renderer/src/lib/system-notifications.ts), therun_settled/askwiring (useAgentStream.ts:514,App.tsx:77), and the i18n keys in en/es/pt-BR/zh-CN. The earlier click-handler Nit remains resolved (Promise.all([...]).catch(...)), and I did not repeat it. Refs #411is still the correct framing — this is partial work (notify on settle) and the timeout is unchanged, so there is no completion claim to validate.
Residual risk (non-blocking):
- AppUserModelID /
appIdparity is unverified here; if the two differ, Windows toasts would silently not appear even thoughnew Notification(...)succeeds. This only affects the new Windows path; it is not a regression to existing behavior. - Carried over and unchanged: the notification path has not been exercised in a running app or against a live model, and the author documents the macOS "last window closed → no renderer" gap. Default-on for existing installs is intentional and stated in the changeset.
Testing
- Not run (automation). The new Windows line is not unit-testable without an Electron main-process harness; a one-time manual check on a Windows install (background run produces a toast) would close the remaining verification gap.
Open-CoDesign Bot
|
后台通知对长时间生成任务很有帮助。合并前建议在 Windows 已安装的应用中补充实机验证:
|
Installs upgraded from preferences schema 9 or earlier now keep system notifications off until the user turns them on in Settings -> Advanced; new installs still start with them on. The changeset says so and names the switch. The settings hint and the changeset also note that on macOS notifications stop after the last window is closed, until it is opened again.
There was a problem hiding this comment.
Findings
No Blocker, Major, Minor, or Nit findings. The changes since the prior bot review (b62d768 → d3bfe1e) are the notification default flip, the matching test/changeset updates, and the macOS caveat added to the four i18n hint strings.
Summary
- Review mode: follow-up after new commits
- What changed since the last review:
apps/desktop/src/main/preferences-ipc.ts:239now reads the missing field as off for files written before schema 10 and on for fresh/current files:persistedSchema < SYSTEM_NOTIFICATIONS_SCHEMA_VERSION ? false : DEFAULTS.systemNotifications. This matches the revised changeset ("Installs upgraded from an earlier version keep them off until you turn on Settings → Advanced") and the documented intent. New installs still resolvesystemNotifications: trueviaDEFAULTSwhenpreferences.jsonis absent (ENOENT path), which the unchangedreturns defaults when the file does not existtest still asserts.apps/desktop/src/main/preferences-ipc.test.ts:406flips the upgrade expectation tofalseand addskeeps an upgraded install opted out when another preference is saved, which verifies that a schema-9 file read as opted-out is not silently flipped back to on when an unrelated preference (proxyUrl) is saved — the write lands atschemaVersion: 10withsystemNotifications: false. That is the important regression guard for this migration, and it is present..changeset/system-notifications.mdwas corrected to describe opt-out-on-upgrade, opt-in-on-fresh, and the macOS last-window caveat. It now matches the runtime behavior, resolving the earlier mismatch between the changeset and the new default.- i18n hint strings in
en/es/pt-BR/zh-CNadd the macOS caveat.
Refs #411remains the correct framing — this notifies on run settle and does not touch the timeout, so there is no completion claim to validate.
Residual observations (non-blocking, not re-filed as findings):
- Windows AppUserModelID /
appIdparity is still unverified from the public checkout.apps/desktop/src/main/index.ts:227hardcodesai.opencowork.codesign, andapps/desktop/electron-builder.ymlwas not available in the context for this run, so I could not confirm the two match. If they differ, rendererNotifications succeed while Windows silently drops the toast. Only affects the new Windows path; not a regression. The earlier review's suggestion to reference a shared constant (e.g.BRAND) still stands but is optional polish. - Carried over and unchanged: the feature has not been exercised in a running app or against a live model, and the macOS "last window closed → no renderer" gap is documented in both the changeset and the Advanced hint.
Testing
- Not run (automation). Manually confirming a background run produces a Windows toast on an installed build would close the remaining AppUserModelID verification gap.
Open-CoDesign Bot
There was a problem hiding this comment.
Findings
No Blocker, Major, Minor, or Nit findings.
The only diff since the prior review (d3bfe1e → 4641a9e) is a new regression test at apps/desktop/src/renderer/src/hooks/useAgentStream.completion.test.ts:536 asserting that durable run_settled outcomes replayed on remount do not emit OS notifications. That directly covers the !replay guard added in apps/desktop/src/renderer/src/hooks/useAgentStream.ts:514, so the previously noted replay-path coverage gap is now closed.
Summary
- Review mode: follow-up after new commits
- What changed since the last review: the replay-suppression test above. It matches the runtime guard
if (!replay && (event.outcome === 'completed' || event.outcome === 'failed'))and thehandleSettled/forgetCancelledGenerationordering, so behavior and test intent line up. - Carried-forward items from the prior review remain accurate and unchanged:
- Schema migration is sound:
apps/desktop/src/main/preferences-ipc.ts:236reads a missing field asfalsefor files below schema 10 andtrueotherwise, andpreferences-ipc.test.ts:406plus the "keeps an upgraded install opted out when another preference is saved" case guard the upgrade path. New installs still resolvetrueviaDEFAULTS. Refs #411remains the correct framing — this does not touch the timeout and makes no completion claim, so there is nothing to validate against the issue.
- Schema migration is sound:
- The changeset wording ("Installs upgraded from an earlier version keep them off until you turn on Settings → Advanced → System notifications") matches the implemented defaults and the four locale hint strings, including the macOS last-window caveat.
Residual observations (non-blocking, not re-filed as findings):
- Windows AppUserModelID parity is still unverified from the public checkout.
apps/desktop/src/main/index.ts:227hardcodesai.opencowork.codesign; if that differs from the electron-builderappId, rendererNotifications succeed while Windows silently drops the toast. Suggest confirmingapps/desktop/electron-builder.ymland, optionally, sourcing the value from a shared constant. Only affects the new Windows path. - The
asknotification wiring inapps/desktop/src/renderer/src/App.tsx:75useswindow.codesign?.ask?.onRequest?.(...); ifonRequestis not present on the preloadaskAPI the optional chaining makes the question notification a silent no-op. Not verifiable from the provided excerpts, and the PR body already flags this path as unit-untested — worth one manual check. - The feature has not been exercised in a running app or against a live model, and the macOS "last window closed → no renderer" gap is documented in both the changeset and the Advanced hint.
Testing
- Not run (automation). Suggest one manual pass on an installed Windows build: background completed / failed / question notifications, click restoring a minimized window and opening the design, plus confirmation that nothing fires while focused, for cancelled/replayed runs, or with the setting off. That would close the remaining AppUserModelID and
askwiring verification gaps.
Open-CoDesign Bot
|
升级默认关闭、迁移测试和 macOS 限制说明都已补齐,符合之前的建议。 |

Summary
Long runs, especially on local models (see #411), often finish while the user is in another app. This shows a system notification when a live run settles as completed or failed, or when the agent asks a question, while the Open CoDesign window is unfocused. The notification is titled with the design name; clicking it focuses the window and opens that design. Cancelled, interrupted, and replayed runs do not notify. Window focus is checked after the setting is read, so returning to the app meanwhile suppresses the notification; a failed read, or a failed focus or design switch after a click, is logged. The main process installs no permission handler, so Electron allows the renderer
NotificationAPI.Settings → Advanced has a "System notifications" checkbox. It is stored in
preferences.json, which moves to schema version 10. New installs start with it on. Installs upgraded from schema 9 or earlier keep it off until the user turns it on, so an update does not start sending notifications nobody asked for; the changeset (and so the release notes) says this and names the switch, which also turns notifications off.On Windows the main process sets the AppUserModelID to the electron-builder
appId(ai.opencowork.codesign, fromapps/desktop/electron-builder.yml), which the NSIS Start Menu shortcut carries; Windows only shows toasts for that ID. This has not been checked on a Windows machine.Known limit, left for a follow-up: notifications come from the renderer. On macOS, closing the last window keeps the app running without a renderer, so runs that settle after that are not announced until a window is open again. The settings hint and the changeset both state this.
Type of change
Linked issue
Refs #411 (overnight local runs). It does not change the timeout.
Checklist
pnpm lint && pnpm typecheck && pnpm testpasses locallypnpm changeset) if user-visibleDependency additions (if any)
None.
Screenshots / recordings (UI changes)
Not attached. Settings → Advanced gains one checkbox row.
Checks
pnpm -r typecheck && pnpm lint && pnpm testwithCI=1(the repo's two-worker limit): 10 tasks passed; desktop 179 files, 2,622 tests.pnpm lintchecked 680 files.useAgentStream.completion.test.ts: liverun_settledevents notify for completed and failed runs only (this test fails without the hook change); outcomes recovered after a refresh do not notify (fails if the replay check is removed)lib/system-notifications.test.ts: design title and click behaviour, focused window, focus regained during the setting read, a failed click logged, setting off, unknown designpreferences-ipc.test.ts: on for new installs (no file), off for files from schema 9 and still off after another preference is saved (both fail without the migration), update round trip at schema 10, non-boolean values rejectedThe notification has not been exercised in a running desktop app or with a live model. Still to check on an installed Windows build: completed, failed, and question notifications while the app is in the background; a click restoring a minimized window and opening that design; nothing shown while the app is focused, for cancelled runs, for replayed history, or with the setting off. The unit tests above cover these cases in the renderer, except the question notification wiring in
App.tsx; restoring a minimized window is done bywin.restore()in the main-processcodesign:v1:focus-windowhandler, which has no unit test.