Desktop: Fixes #11805: Add global shortcut to show/hide Joplin#15013
Desktop: Fixes #11805: Add global shortcut to show/hide Joplin#15013Ashutoshx7 wants to merge 6 commits intolaurent22:devfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a desktop global-hotkey feature: new setting metadata, a UI control with tests and styles, bridge methods to register/unregister Electron global shortcuts, middleware to apply setting changes, and app shutdown cleanup to unregister the hotkey. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant UI as GlobalHotkeyInput
participant Settings as SettingComponent
participant Middleware as AppMiddleware
participant Bridge as Bridge
participant Electron as Electron API
User->>UI: Click "Record" and press keys
UI->>Settings: props.onChange({ value: accelerator })
Settings->>Middleware: Dispatch SETTING_UPDATE_ONE / SETTING_UPDATE_ALL
Middleware->>Bridge: bridge().updateGlobalHotkey(accelerator)
Bridge->>Bridge: unregisterGlobalHotkey() (if set)
Bridge->>Electron: globalShortcut.register(accelerator, handler)
Electron-->>Bridge: registration result
Bridge->>Bridge: store registeredGlobalHotkey_
sequenceDiagram
participant ElectronProcess as Electron
participant Wrapper as ElectronAppWrapper
participant Bridge as Bridge
participant ElectronAPI as Electron API
ElectronProcess->>Wrapper: before-quit
Wrapper->>Bridge: bridge().unregisterGlobalHotkey()
Bridge->>ElectronAPI: globalShortcut.unregister(accelerator)
ElectronAPI-->>Bridge: unregistered / error
Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderabbitai
bot
added
enhancement
Ashutoshx7
force-pushed
the
fix-11805-global-hotkey
branch
from
f7b98d5 to
d911778
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/app-desktop/gui/ConfigScreen/controls/GlobalHotkeyInput.tsx (1)
6-11: Use a concrete callback type instead ofFunction.
Functionloses type safety for the settings event payload. Please typeonChangeexplicitly (for example{ value: string }) to keep this control contract strict.As per coding guidelines: `**/*.{ts,tsx}`: Use proper TypeScript types and avoid `any`.♻️ Suggested typing refinement
interface Props { - value: unknown; + value: string; themeId: number; - // eslint-disable-next-line `@typescript-eslint/ban-types` -- Matches settingKeyToControl interface - onChange: Function; + onChange: (event: { value: string }) => void; } @@ - const value = (props.value as string) || ''; + const value = props.value || '';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app-desktop/gui/ConfigScreen/controls/GlobalHotkeyInput.tsx` around lines 6 - 11, The Props interface currently uses the unsafe Function type for onChange; replace it with a concrete callback signature such as onChange: (payload: { value: string }) => void (and update value: unknown to value: string if appropriate) in GlobalHotkeyInput.tsx so the control contract is strictly typed; update any call sites to match the new signature and adjust imports/types if needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/app-desktop/gui/ConfigScreen/controls/GlobalHotkeyInput.tsx`:
- Around line 6-11: The Props interface currently uses the unsafe Function type
for onChange; replace it with a concrete callback signature such as onChange:
(payload: { value: string }) => void (and update value: unknown to value: string
if appropriate) in GlobalHotkeyInput.tsx so the control contract is strictly
typed; update any call sites to match the new signature and adjust imports/types
if needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ff5db50e-639c-480f-a3b3-476860ac3d31
📒 Files selected for processing (10)
.eslintignore.gitignorepackages/app-desktop/ElectronAppWrapper.tspackages/app-desktop/app.tspackages/app-desktop/bridge.tspackages/app-desktop/gui/ConfigScreen/controls/GlobalHotkeyInput.test.tsxpackages/app-desktop/gui/ConfigScreen/controls/GlobalHotkeyInput.tsxpackages/app-desktop/gui/ConfigScreen/controls/SettingComponent.tsxpackages/app-desktop/gui/ConfigScreen/styles/global-hotkey-input.scsspackages/app-desktop/gui/ConfigScreen/styles/index.scss
✅ Files skipped from review due to trivial changes (4)
- packages/app-desktop/gui/ConfigScreen/controls/SettingComponent.tsx
- .eslintignore
- packages/app-desktop/gui/ConfigScreen/styles/global-hotkey-input.scss
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/app-desktop/app.ts
- packages/app-desktop/ElectronAppWrapper.ts
- packages/app-desktop/gui/ConfigScreen/controls/GlobalHotkeyInput.test.tsx
| // Should exit recording mode | ||
| expect(screen.getByText('Record shortcut')).toBeTruthy(); | ||
| expect(onChange).not.toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
That's too many tests, please reduce them to something more sensible
| interface Props { | ||
| value: unknown; | ||
| themeId: number; | ||
| // eslint-disable-next-line @typescript-eslint/ban-types -- Matches settingKeyToControl interface |
There was a problem hiding this comment.
Types of new component need to be strict, please give it a proper type
| // eslint-disable-next-line @seiyab/react-hooks/exhaustive-deps -- props.onChange is typed as Function (required by settingKeyToControl interface) | ||
| }, [recording, props.onChange]); | ||
|
|
||
| const onClearClick = useCallback(() => { | ||
| props.onChange({ value: '' }); | ||
| // eslint-disable-next-line @seiyab/react-hooks/exhaustive-deps -- props.onChange is typed as Function (required by settingKeyToControl interface) | ||
| }, [props.onChange]); |
There was a problem hiding this comment.
Please make it work without this
|
How come this doesn't work on Wayland? Which distributions and desktop environments have you tried? I'd expect there's some environments where this could work - https://flatpak.github.io/xdg-desktop-portal/docs/doc-org.freedesktop.portal.GlobalShortcuts.html exists and the Electron documentation implies Electron would make use of it where available. Reasonably this could work on Gnome 48+ for example. |
i am on ubuntu 24 lts ( wayland genome 46) |
|
You'd possibly find this works fine on Ubuntu 25.10, Ubuntu 24.04 is using Gnome 46 which doesn't support the API above; by the time Joplin 3.6 releases the latest Ubuntu LTS will be 26.04 giving people on Ubuntu an avenue to upgrade to get the support. Other distributions like Fedora, Arch, and even the latest Debian would already have this in their most common setups. |
hy i shifted to Gnome50 ( ubuntu 26 ( beta) ) |
…trim tests Addressed review feedback: - Reuse the existing ShortcutRecorder component instead of a custom recorder - Use strict types throughout, removed all eslint-disable comments - Reduced tests from 14 to 3 focused scenarios - Restored Wayland detection to hide the setting since Electron's globalShortcut API does not work under Wayland yet
coderabbitai
bot
added
linux
macOS
windows
and removed
enhancement
coderabbitai
bot
added
enhancement
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/app-desktop/gui/ConfigScreen/controls/GlobalHotkeyInput.tsx (1)
41-45: Comment and behaviour are out of sync inonError.The comment says “log only”, but the handler is intentionally a no-op. Please either log the error or update the comment to reflect current behaviour.
Suggested fix (comment-only)
- // Validation errors aren't critical for global shortcuts — log only. + // Validation errors aren't critical for global shortcuts. const onError = useCallback((_event: { recorderError: Error }) => { - // No-op: ShortcutRecorder validates against the keymap (command + // No-op: ShortcutRecorder validates against the keymap (command // conflicts), which doesn't apply to global hotkeys. }, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app-desktop/gui/ConfigScreen/controls/GlobalHotkeyInput.tsx` around lines 41 - 45, The comment and implementation of onError in GlobalHotkeyInput.tsx are inconsistent: the comment says “log only” but onError is a no-op; update the implementation to match the comment by logging the incoming error (e.g., include recorderError details via your app logger) or change the comment to explicitly state that onError is a deliberate no-op because ShortcutRecorder handles validation and global hotkeys are exempt; modify the onError handler or its comment accordingly and ensure you reference the ShortcutRecorder validation behavior in the comment if you keep the no-op.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/app-desktop/gui/ConfigScreen/controls/GlobalHotkeyInput.tsx`:
- Around line 41-45: The comment and implementation of onError in
GlobalHotkeyInput.tsx are inconsistent: the comment says “log only” but onError
is a no-op; update the implementation to match the comment by logging the
incoming error (e.g., include recorderError details via your app logger) or
change the comment to explicitly state that onError is a deliberate no-op
because ShortcutRecorder handles validation and global hotkeys are exempt;
modify the onError handler or its comment accordingly and ensure you reference
the ShortcutRecorder validation behavior in the comment if you keep the no-op.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0b2c3dad-211f-4608-b11b-3424a58da331
📒 Files selected for processing (2)
packages/app-desktop/gui/ConfigScreen/controls/GlobalHotkeyInput.tsxpackages/app-desktop/gui/ConfigScreen/controls/SettingComponent.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/app-desktop/gui/ConfigScreen/controls/SettingComponent.tsx
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/app-desktop/gui/ConfigScreen/controls/GlobalHotkeyInput.tsx (1)
22-30: Consider renaming the shadowed variable for clarity.The
acceleratorconstant shadowsevent.acceleratorin the same scope. While this works correctly, a more descriptive name likenormalizedAcceleratorwould improve readability.♻️ Suggested change
const onSave = useCallback((event: { commandName: string; accelerator: string }) => { // Normalize platform-specific modifiers to CommandOrControl for // consistent cross-platform storage. - const accelerator = event.accelerator + const normalizedAccelerator = event.accelerator .replace(/\bCmd\b/, 'CommandOrControl') .replace(/\bCtrl\b/, 'CommandOrControl'); - props.onChange({ value: accelerator }); + props.onChange({ value: normalizedAccelerator }); setEditing(false); }, [props.onChange]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app-desktop/gui/ConfigScreen/controls/GlobalHotkeyInput.tsx` around lines 22 - 30, The onSave callback currently declares a local const accelerator that shadows event.accelerator; rename it to a clearer identifier (e.g., normalizedAccelerator) to avoid shadowing and improve readability, then use normalizedAccelerator when calling props.onChange({ value: ... }) and keep the existing normalization logic and setEditing(false) behavior intact in the onSave function.packages/app-desktop/gui/KeymapConfig/ShortcutRecorder.tsx (1)
35-37: Consider addingskipKeymapValidationto the useEffect dependency array.If
skipKeymapValidationwere ever to change during the component's lifecycle, the effect would not re-run and could leave the validation state inconsistent. While currently the prop is stable, adding it to the dependency array would be more robust and satisfy exhaustive-deps expectations.♻️ Suggested change
// eslint-disable-next-line `@seiyab/react-hooks/exhaustive-deps` -- Old code before rule was applied - }, [accelerator]); + }, [accelerator, skipKeymapValidation, commandName]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app-desktop/gui/KeymapConfig/ShortcutRecorder.tsx` around lines 35 - 37, The effect in ShortcutRecorder.tsx calls keymapService.validateKeymap({ accelerator, command: commandName }) guarded by skipKeymapValidation but skipKeymapValidation is not included in the useEffect dependency list; update the useEffect hook to include skipKeymapValidation (alongside accelerator and commandName) so the effect re-runs when that flag changes, keeping validation state consistent and satisfying exhaustive-deps.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/app-desktop/gui/ConfigScreen/controls/GlobalHotkeyInput.tsx`:
- Around line 22-30: The onSave callback currently declares a local const
accelerator that shadows event.accelerator; rename it to a clearer identifier
(e.g., normalizedAccelerator) to avoid shadowing and improve readability, then
use normalizedAccelerator when calling props.onChange({ value: ... }) and keep
the existing normalization logic and setEditing(false) behavior intact in the
onSave function.
In `@packages/app-desktop/gui/KeymapConfig/ShortcutRecorder.tsx`:
- Around line 35-37: The effect in ShortcutRecorder.tsx calls
keymapService.validateKeymap({ accelerator, command: commandName }) guarded by
skipKeymapValidation but skipKeymapValidation is not included in the useEffect
dependency list; update the useEffect hook to include skipKeymapValidation
(alongside accelerator and commandName) so the effect re-runs when that flag
changes, keeping validation state consistent and satisfying exhaustive-deps.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 66d1af16-714d-4034-b4ef-18e5f3cb8570
📒 Files selected for processing (6)
packages/app-desktop/app.tspackages/app-desktop/gui/ConfigScreen/controls/GlobalHotkeyInput.tsxpackages/app-desktop/gui/ConfigScreen/controls/SettingComponent.tsxpackages/app-desktop/gui/ConfigScreen/styles/global-hotkey-input.scsspackages/app-desktop/gui/KeymapConfig/ShortcutRecorder.tsxpackages/lib/models/settings/builtInMetadata.ts
✅ Files skipped from review due to trivial changes (1)
- packages/app-desktop/gui/ConfigScreen/styles/global-hotkey-input.scss
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/app-desktop/app.ts
- packages/app-desktop/gui/ConfigScreen/controls/SettingComponent.tsx
- packages/lib/models/settings/builtInMetadata.ts
Looks like it might be gated behind a feature flag for now
So I guess for now it's worth leaving as is but in the future it sounds like we could expect an Electron version bump to sort out Wayland global shortcuts for people on modern systems |
| return ( | ||
| <div className="global-hotkey-input"> | ||
| <span className="shortcut-display"> | ||
| {value || _('Not set')} | ||
| </span> | ||
| <button | ||
| className="record-btn" | ||
| onClick={() => setEditing(true)} | ||
| type="button" | ||
| > | ||
| {_('Change')} | ||
| </button> | ||
| {value && ( | ||
| <button | ||
| className="clear-btn" | ||
| onClick={() => props.onChange({ value: '' })} | ||
| type="button" | ||
| > | ||
| {_('Clear')} | ||
| </button> | ||
| )} | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
ShortcutRecorded already has these features. Is it not possible to use it for this too?
3 participants
AI disclosure
ai was used to help me in testiing the pr and writing the test
Fixes #11805
This adds a configurable global hotkey to show/hide the Joplin window from anywhere on the desktop. Users can set their preferred shortcut from Settings using a custom key recorder input which Uses Electron's globalShortcut module and includes unit tests.
Screencast.from.2026-04-05.00-15-30.mp4
Reviewe note
This feature works fine on windows /macos / linux ( x11) but on linux( wayland) it not cause wayland doesn't allow global shortcut so on wayland this feature will remain hidden ( wayland detection logic )
Screencast.from.2026-04-05.01-25-17.mp4
recording after switching to wayland )