fix: surface notification when globalShortcut.register() fails#517
fix: surface notification when globalShortcut.register() fails#517lacymorrow wants to merge 2 commits into
Conversation
When Electron's globalShortcut.register() returns false (silent failure due to OS-level or another app's conflict), CrossOver previously logged nothing and left the shortcut silently inactive. - registerShortcut() in keyboard.js now returns the boolean result and logs a console warning on failure - registerKeyboardShortcuts() in crossover.js collects failed accelerators and fires a user-facing notification with the conflicting combos - Notification is wrapped in try/catch so early startup calls (before renderer is ready) degrade gracefully to log-only Fixes the 'shortcut stops working after relaunch on Windows 11' pattern reported in #516: registration can fail silently at startup when a game or overlay has already claimed the combo. Co-Authored-By: Paperclip <noreply@paperclip.ing>
There was a problem hiding this comment.
Code Review
This pull request introduces tracking and user notifications for failed keyboard shortcut registrations, updating the shortcut registration process to return its success status. The review feedback suggests improving the notification logic by explicitly checking if the window is ready instead of relying on a try/catch block for control flow. Additionally, it is recommended to use the application's custom logging module instead of console.warn to ensure consistent logging behavior.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| // Notify the user — renderer must be ready; guard against early startup calls | ||
| try { | ||
|
|
||
| const notification = require( './notification' ) | ||
| notification( { | ||
| title: 'Keyboard Shortcut Conflict', | ||
| body: `Could not register: ${failed.join( ', ' )}. Another app may be using this combo. Try rebinding in Settings.`, | ||
| } ) | ||
|
|
||
| } catch ( _err ) { | ||
|
|
||
| // Window not yet ready — failure already logged above | ||
| } |
There was a problem hiding this comment.
Relying on a try/catch block to handle expected control flow (such as checking if the window is ready before sending a notification) is an anti-pattern. If windows.win is null or doesn't have webContents loaded yet, notification will throw a TypeError which is caught here. It is much cleaner and safer to explicitly check if windows.win?.webContents is available before attempting to send the notification.
// Notify the user — renderer must be ready; guard against early startup calls
if ( windows.win?.webContents ) {
try {
const notification = require( './notification' )
notification( {
title: 'Keyboard Shortcut Conflict',
body: 'Could not register: ' + failed.join( ', ' ) + '. Another app may be using this combo. Try rebinding in Settings.',
} )
} catch ( err ) {
log.error( 'Failed to send notification: ' + err.message )
}
}| if ( !registered ) { | ||
|
|
||
| // eslint-disable-next-line no-console | ||
| console.warn( `[CrossOver] globalShortcut.register failed for: ${accelerator} (another app may have claimed this combo)` ) | ||
|
|
||
| } |
There was a problem hiding this comment.
Using console.warn bypasses the application's configured logging system (which might write to files, format logs, or handle log levels differently). It is better to import and use the custom log module, which also removes the need for the eslint-disable-next-line comment.
| if ( !registered ) { | |
| // eslint-disable-next-line no-console | |
| console.warn( `[CrossOver] globalShortcut.register failed for: ${accelerator} (another app may have claimed this combo)` ) | |
| } | |
| if ( !registered ) { | |
| const log = require( './log' ) | |
| log.warn( '[CrossOver] globalShortcut.register failed for: ' + accelerator + ' (another app may have claimed this combo)' ) | |
| } |
- Replace try/catch anti-pattern with explicit windows.win && !isDestroyed() check - Import log module in keyboard.js; replace console.warn with log.warn Co-Authored-By: Paperclip <noreply@paperclip.ing>
|
Addressed both review points:
|
Problem
CrossOver uses Electron's
globalShortcut.register()to bind keyboard shortcuts. This function returnsfalsewhen registration fails silently — which happens on Windows when the OS, a game, or an overlay (Discord, GeForce Experience, Xbox Game Bar) has already claimed the key combo. CrossOver ignored the return value, leaving the shortcut inactive with no feedback to the user.This explains the pattern in #516: shortcut works after rebinding (because
unregisterAll()+ re-register runs while no conflict exists), then breaks on next launch (registration fails at startup when the conflict is already present).Changes
src/main/keyboard.jsregisterShortcut()now returns the boolean fromglobalShortcut.register()and logs aconsole.warnwhen it returnsfalsesrc/main/crossover.jsregisterKeyboardShortcuts()collects all failed accelerators and fires a user-facing notification listing the conflicting combosTest plan
Closes #516