fix(wayland): harden global hotkey lifecycle and defensive handling#79
fix(wayland): harden global hotkey lifecycle and defensive handling#79
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Pull request overview
This PR hardens the Wayland global hotkey lifecycle by introducing an epoch-based mechanism to invalidate stale async registration completions, implementing a queue for in-flight re-registrations, adding D-Bus session cleanup during unregister and no-hotkey states, and adding defensive parsing for portal signal payloads to prevent malformed messages from causing issues.
Changes:
- Introduced epoch mechanism and pending re-registration queue to handle race conditions during async D-Bus registration
- Added D-Bus session cleanup in
unregisterAll()and when all hotkeys are disabled - Added defensive payload parsing (
parseShortcutSignalPayload) for Activated and Deactivated D-Bus signals to gracefully handle malformed messages - Expanded test coverage for malformed payloads, race conditions, and cleanup behavior
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/managers/hotkeyManager.ts | Implemented epoch mechanism for stale registration invalidation, pending re-registration queue, and D-Bus session cleanup in unregisterAll and no-hotkey scenarios |
| src/main/utils/dbusFallback.ts | Added parseShortcutSignalPayload function to defensively parse Activated/Deactivated signal payloads and reject malformed messages |
| tests/unit/main/utils/dbusFallback.test.ts | Added comprehensive tests for malformed signal payloads (null body, non-array body, empty shortcutId, non-string shortcutId) |
| tests/unit/main/hotkeyManager.test.ts | Updated test expectations for queued re-registration behavior and added test for D-Bus session cleanup on unregisterAll |
| tests/coordinated/wayland-hotkey-coordination.coordinated.test.ts | Updated test expectations to reflect queued re-registration behavior and improved cleanup verification |
src/main/managers/hotkeyManager.ts
Outdated
| if (this._hasPendingWaylandReregistration) { | ||
| const latestStatus = getPlatformAdapter().getWaylandStatus(); | ||
| this._hasPendingWaylandReregistration = false; | ||
| this._requestWaylandRegistration(latestStatus); | ||
| } |
There was a problem hiding this comment.
Missing epoch check in the early return path when no hotkeys are enabled. If unregisterAll() is called during the await destroySession() call on line 582, the epoch will be incremented, but the pending re-registration check on lines 589-593 doesn't verify the epoch before potentially triggering a new registration. This could cause a stale registration to proceed after teardown. Add an epoch check similar to the one in the finally block at lines 632-633.
| }); | ||
| expect(hotkeyManager.getPlatformHotkeyStatus().waylandStatus.portalMethod).toBe('none'); | ||
| expect(mockDbusFallback.registerViaDBus).not.toHaveBeenCalled(); | ||
| expect(hotkeyManager.getPlatformHotkeyStatus().globalHotkeysEnabled).toBe(false); |
There was a problem hiding this comment.
This test disables all hotkeys and calls registerShortcuts(), which should trigger the destroySession call on line 582 of hotkeyManager.ts. Consider adding an assertion to verify that mockDbusFallback.destroySession is called to ensure the cleanup behavior is properly tested.
| expect(hotkeyManager.getPlatformHotkeyStatus().globalHotkeysEnabled).toBe(false); | |
| expect(hotkeyManager.getPlatformHotkeyStatus().globalHotkeysEnabled).toBe(false); | |
| expect(mockDbusFallback.destroySession).toHaveBeenCalled(); |
Summary
ActivatedandDeactivatedevents so malformed messages are ignored safely instead of leaking invalid state.Verification