app: Add a setting to disable the system tray icon#5754
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: VlatkoMilisav The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Welcome @VlatkoMilisav! |
|
|
There was a problem hiding this comment.
Pull request overview
This PR adds an Electron-only, persisted preference to enable/disable the system tray icon at runtime (no restart), keeping the default behavior unchanged (tray enabled unless explicitly disabled).
Changes:
- Persist a new
settings.jsonflag (enableSystemTray, defaulting to enabled) and gate tray creation on it. - Add IPC plumbing to query/update the setting from the renderer and create/destroy the tray live.
- Add a desktop-only “Show system tray icon” toggle in General Settings and introduce the new i18n key across locales.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| app/electron/main.ts | Adds IPC handlers and runtime apply logic to create/destroy the tray based on the persisted setting. |
| app/electron/preload.ts | Whitelists new IPC channels for requesting/updating tray icon state. |
| app/electron/tray.ts | Adds persisted getters/setters for the tray preference and skips tray creation when disabled. |
| frontend/src/components/App/Settings/Settings.tsx | Adds an Electron-only toggle wired to IPC to show/hide the tray icon. |
| frontend/src/i18n/locales/en/translation.json | Adds “Show system tray icon” translation key (source string). |
| frontend/src/i18n/locales/de/translation.json | Adds “Show system tray icon” translation key. |
| frontend/src/i18n/locales/es/translation.json | Adds “Show system tray icon” translation key. |
| frontend/src/i18n/locales/fr/translation.json | Adds “Show system tray icon” translation key. |
| frontend/src/i18n/locales/hi/translation.json | Adds “Show system tray icon” translation key. |
| frontend/src/i18n/locales/it/translation.json | Adds “Show system tray icon” translation key. |
| frontend/src/i18n/locales/ja/translation.json | Adds “Show system tray icon” translation key. |
| frontend/src/i18n/locales/ko/translation.json | Adds “Show system tray icon” translation key. |
| frontend/src/i18n/locales/pt/translation.json | Adds “Show system tray icon” translation key. |
| frontend/src/i18n/locales/ru/translation.json | Adds “Show system tray icon” translation key. |
| frontend/src/i18n/locales/ta/translation.json | Adds “Show system tray icon” translation key. |
| frontend/src/i18n/locales/zh/translation.json | Adds “Show system tray icon” translation key. |
| frontend/src/i18n/locales/zh-tw/translation.json | Adds “Show system tray icon” translation key. |
Comments suppressed due to low confidence (1)
app/electron/preload.ts:63
desktopApi.receiveregisters a wrapper function withipcRenderer.on(...), but the renderer later callsdesktopApi.removeListener(channel, originalHandler). Since the wrapper reference is different,removeListenercannot actually unsubscribe, leading to leaked listeners (and duplicated callbacks on remount). Consider storing a mapping from original handler -> wrapped handler (and using it inremoveListener), or havereceivereturn an explicit unsubscribe function.
if (validChannels.includes(channel)) {
// Deliberately strip event as it includes `sender`
ipcRenderer.on(channel, (event, ...args) => func(...args));
}
},
illume
left a comment
There was a problem hiding this comment.
Thanks for working on this.
The commit messages could use some tidying up to match our contribution guidelines. We use Linux kernel style — the contributing guide has the details, and git log shows good examples.
Commits that need attention
Add setting to disable the system tray icon— Missingarea: descriptionprefix — e.g.frontend: HomeButton: Fix so it navigates to homeorbackend: config: Add enable-dynamic-clusters flag.
Commit guidelines
- Use atomic commits focused on a single change.
- Use the title format
<area>: <Description of changes>— description must start with a capital letter. - Keep the title under 72 characters (soft requirement).
- Explain the intention and why the change is needed.
- Make commit titles meaningful and describe what changed.
- Do not add code that a later commit rewrites; squash or reorder commits instead.
- Do not include
Fixes #NNin commit messages.
Good examples:
frontend: HomeButton: Fix so it navigates to homebackend: config: Add enable-dynamic-clusters flag
Would you mind addressing the open Copilot review comments? Please mark each comment as resolved after addressing it.
Looks like the frontend snapshots are out of date — cd frontend && npm run test -- -u will update them.
How to update snapshots
Run cd frontend && npm run test -- -u to regenerate all snapshots. Review the diff to make sure the visual changes are intentional, then commit the updated snapshot files.
90c9153 to
49d7eaa
Compare
|
Thanks for the review! Updated:
On the snapshot check: the failing snapshots in CI are |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
app/electron/preload.ts:71
receiveregisters a wrapped listener withipcRenderer.on(...), butremoveListenerstill forwards the originalfuncdirectly toipcRenderer.removeListener(...), so callers that usewindow.desktopApi.removeListener(channel, originalFunc)will not actually unsubscribe. This can lead to accumulating IPC listeners (e.g. existing frontend code relies onremoveListenerfor theplugin-managerchannel). Consider either (a) tracking a mapping from original handler -> wrapped handler per channel and using it inremoveListener, or (b) removing/deprecatingremoveListenerin favor of the unsubscribe function returned fromreceiveand updating call sites accordingly.
const wrapped = (event: unknown, ...args: unknown[]) => func(...args);
ipcRenderer.on(channel, wrapped);
// Return an unsubscribe function so callers can clean up the exact
// wrapped listener (removeListener with the original func cannot).
return () => ipcRenderer.removeListener(channel, wrapped);
}
},
removeListener: (channel: string, func: (...args: unknown[]) => void) => {
ipcRenderer.removeListener(channel, func);
},
49d7eaa to
2522a09
Compare
|
Addressed Copilot's follow-up round too (both resolved):
Still folded into the single atomic commit. |
2522a09 to
24471be
Compare
|
Rebased onto current main. The |
The system tray icon was created unconditionally on launch with no way to opt out. Add a persisted "Show system tray icon" preference (default on, desktop only) that creates or removes the tray at runtime without requiring a restart.
24471be to
a839014
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
app/electron/preload.ts:71
desktopApi.receivenow registers a wrapped listener (to strip the Electron event) and returns an unsubscribe, butdesktopApi.removeListenerstill callsipcRenderer.removeListener(channel, func)with the original callback. That won’t remove listeners added viareceive, so existing renderer code that pairsreceive(..., cb)withremoveListener(..., cb)(e.g. the plugin-manager flow) will leak listeners and can break request/response logic. Consider tracking original->wrapped callbacks per channel soremoveListenercan remove the wrapped listener, or deprecateremoveListenerand update all call sites to use the unsubscribe returned fromreceive.
// Deliberately strip event as it includes `sender`
const wrapped = (event: unknown, ...args: unknown[]) => func(...args);
ipcRenderer.on(channel, wrapped);
// Return an unsubscribe function so callers can clean up the exact
// wrapped listener (removeListener with the original func cannot).
return () => ipcRenderer.removeListener(channel, wrapped);
}
},
removeListener: (channel: string, func: (...args: unknown[]) => void) => {
ipcRenderer.removeListener(channel, func);
},
illume
left a comment
There was a problem hiding this comment.
Thanks for the contribution.
it looks like there's a merge-main commit in this PR — could you rebase onto main instead?
Why this matters
Merge commits from main make the PR history harder to review. Please rebase your branch on top of the latest main instead, then update the PR with the rebased commits.
| // Deliberately strip event as it includes `sender` | ||
| ipcRenderer.on(channel, (event, ...args) => func(...args)); | ||
| const wrapped = (event: unknown, ...args: unknown[]) => func(...args); | ||
| ipcRenderer.on(channel, wrapped); | ||
| // Return an unsubscribe function so callers can clean up the exact | ||
| // wrapped listener (removeListener with the original func cannot). | ||
| return () => ipcRenderer.removeListener(channel, wrapped); |
Summary
This PR makes the desktop system tray icon optional by adding a persisted "Show system tray icon" setting, addressing the request that the tray (added in #4030 / #4452) is currently created unconditionally with no opt-out.
Related Issue
Fixes #5753
Changes
isTrayIconEnabled/setTrayIconEnabledinapp/electron/tray.ts, persisted via the existingsettings.json(keyenableSystemTray, defaults totrueso current behavior is unchanged)createHeadlampTraynow returns early when the tray is disabledapp/electron/main.ts: extracted tray options into a shared builder and addedapplyTrayIconSetting, which creates or removes the tray at runtime (no restart needed); addedrequest-tray-icon/set-tray-iconIPC handlersapp/electron/preload.ts: whitelisted the new IPC channelsisElectron()), wired through the desktop APInpm run i18nto add the new translation keySteps to Test
make run-appornpm startinapp/) on macOS/Linux/WindowsScreenshots (if applicable)
Notes for the Reviewer
translationkey across locale files) - onlytranslation.jsonwas changed; the parser's incidentalglossary.jsonreformatting was reverted.