fix(suite-desktop): clean up accumulated Electron listeners on re-init#28241
Draft
mroz22 wants to merge 2 commits into
Draft
fix(suite-desktop): clean up accumulated Electron listeners on re-init#28241mroz22 wants to merge 2 commits into
mroz22 wants to merge 2 commits into
Conversation
Stop accumulation of `powerMonitor` listeners in `suite-desktop-core/modules/power-monitor.ts`. Document findings from the desktop and connect packages audit.
Fix two IPC listener leaks: DesktopUpdater.tsx leaked 8 update channels without cleanup when `desktopUpdate.enabled` changed, and Protocol.tsx was missing cleanup for `protocol/open`. Document new false positives from the suite/suite-desktop-ui audit.
2 tasks
mroz22
added
code
There was a problem hiding this comment.
Pull request overview
This PR fixes Electron IPC/listener leaks in the desktop app by ensuring listeners are not accumulated when parts of the app are re-initialized (e.g., window recreation or settings toggles), preventing duplicated event handling and potential memory growth.
Changes:
- Register
powerMonitorlisteners only once (module init), and ensuresuspendtargets the current window instance. - Add missing
desktopApilistener cleanup inDesktopUpdaterwhen the effect re-runs/unmounts. - Add missing
protocol/openlistener cleanup inProtocol.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/suite-desktop-core/src/modules/power-monitor.ts | Moves powerMonitor listener registration out of window init to avoid stacking across window recreation; targets current window on suspend. |
| packages/suite-desktop-ui/src/support/DesktopUpdater.tsx | Ensures all updater-related desktopApi listeners are removed in effect cleanup to prevent leaks when desktopUpdate.enabled toggles. |
| packages/suite/src/support/suite/Protocol.tsx | Adds cleanup to remove protocol/open listeners on unmount/re-run to prevent listener accumulation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Summary
Two isolated Electron listener-cleanup fixes, split out of the broader audit in #27978 so they can be reviewed independently of the transport/connect changes. Both are single-concern and touch only desktop packages.
Fixes
@trezor/suite-desktop-core—power-monitorListeners (
lock-screen/unlock-screen/suspend/resume) were registered insidemainWindowProxy.on('init', …), which fires again on everyreactivateWindow()after the window is destroyed — so each window recreation stacked another full set ofpowerMonitorlisteners on the global Electron singleton. Moved registration out of theinitcallback (runs once per module init) and switched thesuspendhandler tomainWindowProxy.getInstance()?.webContents.send(...)so it always targets the current window.@trezor/suite-desktop-ui—DesktopUpdater/ProtocolDesktopUpdater.tsxregistered 8desktopApiupdate-channel listeners but only cleaned up the interval; togglingdesktopUpdate.enabledre-ran the effect and leaked the previous listeners. Now the effect removes all update listeners on cleanup.Protocol.tsxadds the missingremoveAllListeners('protocol/open')cleanup.Why split out
These are in packages no other PR in the leak-audit stream touches, each is a self-contained "add the missing cleanup" change, and each is independently revertible — so they don't need to wait on the transport/connect review.
Related PRs (listener-leak audit stream)
fix: audit and patch event listener leaks across transport/connect/desktop packages(origin of these commits)feat(utils): introduce listener disposer helper for Connect/transport sitesTest plan
lint:jsclean on all three changed files🤖 Generated with Claude Code
🤖 LLM Test Recommendations
Summary: The three changed files have very limited E2E test relevance.
power-monitor.ts(desktop power monitoring) andDesktopUpdater.tsx(desktop auto-updater UI) are desktop-core/desktop-UI modules with no static test coverage and no E2E tests that exercise power monitoring or update flows.Protocol.tsxis a broadly imported support component (URI/deep-link protocol handler) that maps to 121 tests — but none of the 142 analyzed tests specifically exercise protocol/deep-link handling behavior, meaning the static mapping is purely transitive. Since no test directly exercises the changed functionality (power monitor behavior, desktop updater UI, or protocol URI handling), no E2E tests are recommended. The risk is low for user-facing regressions detectable by these E2E tests; manual or unit-level testing of these modules would be more appropriate.Changed files (3)
packages/suite-desktop-core/src/modules/power-monitor.tspackages/suite-desktop-ui/src/support/DesktopUpdater.tsxpackages/suite/src/support/suite/Protocol.tsxNo test recommendations found.
packages/suite-desktop-core/src/modules/power-monitor.tspackages/suite-desktop-ui/src/support/DesktopUpdater.tsxpackages/suite/src/support/suite/Protocol.tsxUpdated: 2026-06-01T15:02:08.157Z
🔍 Currents Test Results
🔍 Suite desktop test results: View in Currents
🔍 Suite web test results: View in Currents
🔒 Quarantined E2E Tests
Trezor Suite (web) — 10 test(s)
Updated: 2026-06-01T15:08:40.352Z • 10 test(s) total
Trezor Suite (desktop) — 6 test(s)
Updated: 2026-06-01T15:05:23.654Z • 6 test(s) total
🌐 Preview deployments
🌐 Suite Web preview: https://dev.suite.sldev.cz/suite-web/mroz22/desktop-listener-cleanup/web/