Keyboard shortcut support #234 #454
Conversation
- Implement shortcut definitions with OS-specific display values - Create useShortcuts hook for managing shortcuts and their descriptions - Update IconsSidebar to use dynamic shortcut descriptions - Integrate with electron to detect OS and handle shortcut actions fixes reorproject#234
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This pull request implements keyboard shortcuts for various functionalities in the Reor application, addressing issue #234.
- Added
shortcutDefinitions.tsto define cross-platform keyboard shortcuts for common actions - Implemented
shortcutManager.tsto handle shortcut registration and input matching in Electron - Created
use-shortcut.tscustom React hook for managing shortcut actions and descriptions - Modified
windowManager.tsto return the createdBrowserWindowinstance, enabling shortcut implementation - Updated
index.tsto register global shortcuts when windows load and unregister them on app quit - Integrated shortcuts into
MainPage.tsxandIconsSidebar.tsxcomponents for improved user experience
7 file(s) reviewed, 12 comment(s)
Edit PR Review Bot Settings | Greptile
| @@ -14,6 +14,7 @@ import registerFileHandlers from './filesystem/ipcHandlers' | |||
| import { ollamaService, registerLLMSessionHandlers } from './llm/ipcHandlers' | |||
| import registerPathHandlers from './path/ipcHandlers' | |||
| import { registerDBSessionHandlers } from './vector-database/ipcHandlers' | |||
| import { registerGlobalShortcuts, unregisterGlobalShortcuts } from '../../src/components/shortcuts/shortcutManager' | |||
There was a problem hiding this comment.
style: Consider using an absolute import path instead of a relative one for better maintainability
| app.on('will-quit', () => { | ||
| const windows = BrowserWindow.getAllWindows() | ||
| windows.forEach((window) => unregisterGlobalShortcuts(window)) | ||
| }) |
There was a problem hiding this comment.
style: Consider moving this logic to a separate function for better organization
| @@ -16,6 +16,7 @@ import { FileProvider, useFileContext } from '@/contexts/FileContext' | |||
| import ModalProvider from '@/contexts/ModalContext' | |||
| import CustomContextMenu from './Common/CustomContextMenu' | |||
| import CommonModals from './Common/CommonModals' | |||
| import useShortcuts from './shortcuts/use-shortcut' | |||
There was a problem hiding this comment.
style: Consider adding a comment explaining the purpose of this import and its impact on the component.
| @@ -24,6 +25,7 @@ const MainPageContent: React.FC = () => { | |||
|
|
|||
| const { showChatbot } = useChatContext() | |||
|
|
|||
| useShortcuts() | |||
There was a problem hiding this comment.
style: Consider moving this hook call to a more appropriate lifecycle stage if it's causing any unnecessary re-renders.
There was a problem hiding this comment.
removed this unwanted hook call
| import { BrowserWindow } from 'electron' | ||
| import { shortcuts } from './shortcutDefinitions' | ||
|
|
||
| function isShortcutMatch(input: Electron.Input, shortcutKey: string): boolean { |
There was a problem hiding this comment.
logic: isShortcutMatch function doesn't handle the case where mainKey is undefined
| export function registerGlobalShortcuts(mainWindow: BrowserWindow) { | ||
| mainWindow.webContents.on('before-input-event', (event, input) => { | ||
| shortcuts.forEach((shortcut) => { | ||
| if (input.type === 'keyDown' && isShortcutMatch(input, shortcut.key)) { | ||
| event.preventDefault() | ||
| mainWindow.webContents.send(shortcut.action) | ||
| } | ||
| }) | ||
| }) | ||
| } |
There was a problem hiding this comment.
style: registerGlobalShortcuts doesn't return a way to unregister individual shortcuts
|
thanks for working on this. i like the overall readability of the code. i have to qs on implementation:
|
|
@joseplayero I have attempted to fix the issue as per your comment and have created a new PR. I also encountered some challenges, which I have detailed in the PR. Please review. |
fixes #234
I have added keyboard shortcuts for some functionalities. We can continue to add new shortcuts if the current approach is good and approved.
recording-2024-10-16-013326_R0w2mvyd.mp4