Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces configurable keyboard shortcuts for the goose desktop application, allowing users to customize global and application-level shortcuts through a new settings interface.
Changes:
- Added
KeyboardShortcutsinterface and migration logic from deprecatedglobalShortcutfield to newkeyboardShortcutsstructure - Refactored hardcoded shortcuts in menu items and global shortcut registration to use values from settings
- Added new "Keyboard" tab in settings UI with a dedicated keyboard shortcuts configuration section
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| ui/desktop/src/utils/settings.ts | Adds KeyboardShortcuts interface, migration logic, and getKeyboardShortcuts function to handle backward compatibility |
| ui/desktop/src/main.ts | Refactors global shortcut registration into registerGlobalShortcuts function, updates menu items to use configurable shortcuts, and modifies save-settings handler to re-register shortcuts on change |
| ui/desktop/src/components/settings/SettingsView.tsx | Adds new Keyboard tab with KeyboardShortcutsSection component |
ui/desktop/src/utils/settings.ts
Outdated
| import { app } from 'electron'; | ||
| import fs from 'fs'; | ||
| import path from 'path'; | ||
| import { defaultKeyboardShortcuts } from './keyboardShortcutDefaults'; |
There was a problem hiding this comment.
This import references a file that is not included in the PR. The file keyboardShortcutDefaults.ts needs to be added to define the defaultKeyboardShortcuts constant, otherwise this code will fail at runtime with a module not found error.
| import KeyboardShortcutsSection from './keyboard/KeyboardShortcutsSection'; | ||
| import { CONFIGURATION_ENABLED } from '../../updates'; | ||
| import { trackSettingsTabViewed } from '../../utils/analytics'; | ||
|
|
There was a problem hiding this comment.
This import references a component that is not included in the PR. The file keyboard/KeyboardShortcutsSection.tsx needs to be added, otherwise this code will fail at runtime with a module not found error.
| import KeyboardShortcutsSection from './keyboard/KeyboardShortcutsSection'; | |
| import { CONFIGURATION_ENABLED } from '../../updates'; | |
| import { trackSettingsTabViewed } from '../../utils/analytics'; | |
| import { CONFIGURATION_ENABLED } from '../../updates'; | |
| import { trackSettingsTabViewed } from '../../utils/analytics'; | |
| function KeyboardShortcutsSection() { | |
| return ( | |
| <div> | |
| Keyboard shortcuts settings are not available in this build. | |
| </div> | |
| ); | |
| } |
| return { | ||
| ...defaultKeyboardShortcuts, | ||
| focusWindow: settings.globalShortcut, | ||
| quickLauncher: settings.globalShortcut | ||
| ? settings.globalShortcut.replace(/\+G$/i, '+Shift+G') | ||
| : null, | ||
| }; |
There was a problem hiding this comment.
The migration logic attempts to preserve both focusWindow and quickLauncher shortcuts from the old globalShortcut field. However, it only migrates quickLauncher if globalShortcut is truthy, but sets focusWindow regardless. This means if globalShortcut is null or empty string, focusWindow will be set to null but quickLauncher won't be set at all (defaultKeyboardShortcuts will be used instead). This inconsistency could be confusing. Consider setting quickLauncher explicitly in both cases.
ui/desktop/src/main.ts
Outdated
| return { success: true, shortcutsChanged }; | ||
| } catch (error) { | ||
| console.error('Error saving settings:', error); | ||
| return false; | ||
| return { success: false, shortcutsChanged: false }; |
There was a problem hiding this comment.
The return type has changed from Promise to Promise<{success: boolean, shortcutsChanged: boolean}>, but the TypeScript type definition in preload.ts (line 95) still declares it as returning Promise. This creates a type mismatch that will cause TypeScript errors and runtime issues for code that relies on the type definition.
| if (shortcuts.newChat) { | ||
| fileMenu.submenu.insert( | ||
| 0, | ||
| new MenuItem({ | ||
| label: 'New Chat', | ||
| accelerator: shortcuts.newChat, | ||
| click() { | ||
| const focusedWindow = BrowserWindow.getFocusedWindow(); | ||
| if (focusedWindow) focusedWindow.webContents.send('new-chat'); | ||
| }, | ||
| }) | ||
| ); | ||
| } | ||
|
|
||
| fileMenu.submenu.insert( | ||
| 1, | ||
| new MenuItem({ | ||
| label: 'New Chat Window', | ||
| accelerator: process.platform === 'darwin' ? 'Cmd+N' : 'Ctrl+N', | ||
| click() { | ||
| ipcMain.emit('create-chat-window'); | ||
| }, | ||
| }) | ||
| ); | ||
| if (shortcuts.newChatWindow) { | ||
| fileMenu.submenu.insert( | ||
| 1, | ||
| new MenuItem({ | ||
| label: 'New Chat Window', | ||
| accelerator: shortcuts.newChatWindow, | ||
| click() { | ||
| ipcMain.emit('create-chat-window'); | ||
| }, | ||
| }) | ||
| ); | ||
| } | ||
|
|
||
| fileMenu.submenu.insert( | ||
| 2, | ||
| new MenuItem({ | ||
| label: 'Open Directory...', | ||
| accelerator: 'CmdOrCtrl+O', | ||
| click: () => openDirectoryDialog(), | ||
| }) | ||
| ); | ||
| if (shortcuts.openDirectory) { | ||
| fileMenu.submenu.insert( | ||
| 2, | ||
| new MenuItem({ | ||
| label: 'Open Directory...', | ||
| accelerator: shortcuts.openDirectory, | ||
| click: () => openDirectoryDialog(), | ||
| }) | ||
| ); | ||
| } |
There was a problem hiding this comment.
The menu insertion logic uses fixed indices (0, 1, 2) but conditionally inserts items. If newChat is disabled (null), the newChatWindow item will be inserted at index 1 which is the wrong position. Same issue with openDirectory at index 2. These indices should be adjusted dynamically based on how many items were actually inserted before them, or use a different approach like appending items or using a counter variable.
| const newShortcuts = { | ||
| ...shortcuts, | ||
| [key]: enabled ? defaultValue : null, | ||
| }; | ||
|
|
There was a problem hiding this comment.
When toggling a shortcut back on, this restores the default shortcut value without checking if it conflicts with another currently active shortcut. If a user disabled shortcut A and assigned its value to shortcut B, then re-enabled shortcut A, both shortcuts would have the same value. Consider checking for conflicts and either clearing the conflicting shortcut or showing a warning to the user.
| const newShortcuts = { | |
| ...shortcuts, | |
| [key]: enabled ? defaultValue : null, | |
| }; | |
| let newShortcuts: KeyboardShortcuts; | |
| if (!enabled) { | |
| // Disabling a shortcut: simply clear its value. | |
| newShortcuts = { | |
| ...shortcuts, | |
| [key]: null, | |
| }; | |
| } else { | |
| // Enabling a shortcut: restore its default value, but clear any other shortcut | |
| // that is currently using the same value to avoid conflicts. | |
| const updatedShortcuts: KeyboardShortcuts = { | |
| ...shortcuts, | |
| }; | |
| if (defaultValue) { | |
| (Object.keys(updatedShortcuts) as (keyof KeyboardShortcuts)[]).forEach((otherKey) => { | |
| if (otherKey !== key && updatedShortcuts[otherKey] === defaultValue) { | |
| updatedShortcuts[otherKey] = null; | |
| } | |
| }); | |
| } | |
| updatedShortcuts[key] = defaultValue; | |
| newShortcuts = updatedShortcuts; | |
| } |
| const formatShortcut = (shortcut: string) => { | ||
| const isMac = window.electron.platform === 'darwin'; | ||
| return shortcut | ||
| .replace('CommandOrControl', isMac ? 'Cmd' : 'Ctrl') | ||
| .replace('Command', 'Cmd') | ||
| .replace('Control', 'Ctrl'); | ||
| }; |
There was a problem hiding this comment.
The shortcut formatting logic is duplicated in multiple places with slightly different implementations: within this file at lines 165-173 and 244-250, as well as in ShortcutRecorder.tsx at lines 106-112 and 150-158. Consider extracting this into a shared utility function to ensure consistent formatting and make maintenance easier.
| new MenuItem({ | ||
| label: 'Focus Goose Window', | ||
| accelerator: shortcuts.focusWindow, | ||
| click() { |
There was a problem hiding this comment.
The separator is always inserted at line 2096, even if no menu items were added before it (menuIndex could be 0). If all shortcuts for newChat, newChatWindow, and openDirectory are disabled, and there are no recent directories, this would insert a separator with nothing before it. Consider only inserting the separator if menuIndex > 0.
| click() { | |
| if (menuIndex > 0) { | |
| fileMenu.submenu.insert(menuIndex++, new MenuItem({ type: 'separator' })); | |
| } |
| if (!settings.keyboardShortcuts && settings.globalShortcut !== undefined) { | ||
| const focusShortcut = settings.globalShortcut; | ||
| const launcherShortcut = focusShortcut ? focusShortcut.replace(/\+G$/i, '+Shift+G') : null; | ||
|
|
There was a problem hiding this comment.
The regex replacement /+G$/i will incorrectly transform shortcuts that already contain Shift. For example, if the old globalShortcut was 'CommandOrControl+Shift+G', this would produce 'CommandOrControl+Shift+Shift+G' for the quickLauncher. Consider checking if 'Shift' is already present before appending it, or using a more specific regex like /+([Gg])$/ and replacing with '+Shift+$1'.
| const launcherShortcut = focusShortcut | |
| ? /\bShift\b/i.test(focusShortcut) | |
| ? focusShortcut | |
| : focusShortcut.replace(/\+([Gg])$/, '+Shift+$1') | |
| : null; |
| const handleKeyDown = (e: React.KeyboardEvent) => { | ||
| if (!recording) return; | ||
|
|
||
| e.preventDefault(); | ||
| e.stopPropagation(); | ||
|
|
||
| // Ignore modifier-only presses | ||
| if (['Control', 'Meta', 'Alt', 'Shift'].includes(e.key)) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Pressing the Escape key will capture it as a shortcut ('Esc') instead of canceling the recording. Users would expect Escape to cancel the recording dialog. Consider checking for Escape key before processing and calling onCancel() instead.
ui/desktop/src/main.ts
Outdated
| if (shortcutsChanged) { | ||
| registerGlobalShortcuts(); | ||
| } | ||
|
|
||
| saveSettings(settings); |
There was a problem hiding this comment.
registerGlobalShortcuts() is called before saveSettings(settings), causing it to load the old settings from disk instead of using the new settings. Move saveSettings(settings) to line 1269, before the shortcut registration check.
| if (shortcutsChanged) { | |
| registerGlobalShortcuts(); | |
| } | |
| saveSettings(settings); | |
| saveSettings(settings); | |
| if (shortcutsChanged) { | |
| registerGlobalShortcuts(); | |
| } |
| if (['Control', 'Meta', 'Alt', 'Shift'].includes(e.key)) { | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
Pressing Escape alone will be captured as a shortcut ("Esc") instead of canceling the recording. Add a check before line 49 to call onCancel() if the key is 'Escape' and no modifiers are pressed.
| // Pressing bare Escape should cancel recording instead of being captured as "Esc" | |
| if ( | |
| e.key === 'Escape' && | |
| !e.ctrlKey && | |
| !e.metaKey && | |
| !e.altKey && | |
| !e.shiftKey | |
| ) { | |
| onCancel(); | |
| return; | |
| } |
|
When I run this branch locally I get the following error, I think it's related to the copilot review above? I put up a PR with a fix from goose if you want to merge it into this branch. After this fix and running I tested the keyboard settings and it is putting a strange character when I tried to change focus goose window to simply Couple of other issues:
Kapture.2026-01-26.at.07.10.13.mp4
|
the other bug with the weird characters if you hit cmd-option-N should now also be fixed |
| envToggles: { | ||
| GOOSE_SERVER__MEMORY: false, | ||
| GOOSE_SERVER__COMPUTER_CONTROLLER: false, | ||
| }, |
There was a problem hiding this comment.
The mock settings include envToggles which was removed from the Settings interface in this PR. This will cause type mismatches when tests use this mock. Remove these lines since envToggles is no longer part of Settings.
| envToggles: { | |
| GOOSE_SERVER__MEMORY: false, | |
| GOOSE_SERVER__COMPUTER_CONTROLLER: false, | |
| }, |
| const settings = getSettings(); | ||
| updateSettings((s) => { | ||
| s.showDockIcon = show; | ||
| }); |
There was a problem hiding this comment.
Reading settings before the updateSettings call creates potential for using stale data. The settings variable is read before the update, but the update might change other fields. If showMenuBarIcon check on line 1266 needs the latest value, it should be read after the update or within the updateSettings callback to ensure consistency.
| function getSettings(): Settings { | ||
| if (fsSync.existsSync(SETTINGS_FILE)) { | ||
| const data = fsSync.readFileSync(SETTINGS_FILE, 'utf8'); | ||
| return JSON.parse(data); |
There was a problem hiding this comment.
JSON.parse could throw an error if the settings file is corrupted. This would crash the app. Wrap the parse in a try-catch and return defaultSettings if parsing fails, logging the error for debugging.
| return JSON.parse(data); | |
| try { | |
| return JSON.parse(data); | |
| } catch (error) { | |
| log.error('Failed to parse settings file, using default settings', error); | |
| return defaultSettings; | |
| } |
| if (!settings.keyboardShortcuts && settings.globalShortcut !== undefined) { | ||
| updateSettings((s) => { | ||
| s.keyboardShortcuts = getKeyboardShortcuts(s); | ||
| delete s.globalShortcut; |
There was a problem hiding this comment.
The delete s.globalShortcut operation modifies the settings object's prototype properties. TypeScript doesn't guarantee this is safe since globalShortcut is an optional property. Use a runtime check or explicitly set to undefined instead: s.globalShortcut = undefined. This is safer and more explicit about intent.
| delete s.globalShortcut; | |
| s.globalShortcut = undefined; |
| if (focusShortcut.includes('Shift')) { | ||
| launcherShortcut = focusShortcut; | ||
| } else { | ||
| launcherShortcut = focusShortcut.replace(/\+([Gg])$/, '+Shift+$1'); |
There was a problem hiding this comment.
The regex /\+([Gg])$/ assumes the shortcut ends with '+G' or '+g', but this won't work correctly if the focus shortcut is something like 'CommandOrControl+Alt+G+Space' or has a different key. Consider validating that focusShortcut actually ends with a single 'G' or 'g' key before attempting the replacement, or use a more robust parsing approach.
| launcherShortcut = focusShortcut.replace(/\+([Gg])$/, '+Shift+$1'); | |
| const parts = focusShortcut.split('+'); | |
| if (parts.length > 1) { | |
| const key = parts[parts.length - 1]; | |
| const modifiers = parts.slice(0, parts.length - 1); | |
| launcherShortcut = [...modifiers, 'Shift', key].join('+'); | |
| } else { | |
| launcherShortcut = focusShortcut + '+Shift'; | |
| } |
zanesq
left a comment
There was a problem hiding this comment.
Looks good, only feedback is the warning for restart doesn't seem to show for global shortcuts so we might want to show it there also if its updating the menu also.


Summary
Collects all our keyboard short cuts and makes them configurable: