feat: configurable advanced keyboard shortcuts system#343
feat: configurable advanced keyboard shortcuts system#343anwarulislam wants to merge 15 commits into
Conversation
👷 Deploy request for appcut pending review.Visit the deploys page to approve it
|
|
@anwarulislam is attempting to deploy a commit to the OpenCut OSS Team on Vercel. A member of the Team first needs to authorize it. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the """ WalkthroughThis change refactors keyboard shortcut handling in the web application by removing the old Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Keyboard
participant KeybindingsListener
participant KeybindingsStore
participant Actions
participant Editor
User->>Keyboard: Presses a key combination
Keyboard->>KeybindingsListener: keydown event
KeybindingsListener->>KeybindingsStore: Get keybinding for event
KeybindingsStore-->>KeybindingsListener: Returns mapped action (if any)
KeybindingsListener->>Actions: invokeAction(action, args)
Actions->>Editor: Editor action handler executes logic
sequenceDiagram
participant User
participant KeyboardShortcutsHelp
participant KeybindingEditor
participant KeybindingsStore
User->>KeyboardShortcutsHelp: Opens shortcuts help
KeyboardShortcutsHelp->>User: Shows list of shortcuts
User->>KeyboardShortcutsHelp: Clicks "Customize"
KeyboardShortcutsHelp->>KeybindingEditor: Renders editor with shortcuts
User->>KeybindingEditor: Edits, adds, removes, imports, or exports shortcuts
KeybindingEditor->>KeybindingsStore: Updates keybindings
KeybindingsStore-->>KeybindingEditor: Persists and validates changes
Possibly related PRs
Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Hey, @mazeincoding! So, why actions? And why complicate things a bit? It's not really complicated; it just makes it super customizable and easy to maintain in the future. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🔭 Outside diff range comments (1)
apps/web/src/components/keyboard-shortcuts-help.tsx (1)
99-167: Refactor to avoid Dialog duplication and improve UXThe current implementation duplicates the Dialog component and could cause the dialog to close/reopen when toggling between views. This creates unnecessary re-renders and poor UX.
Refactor to use a single Dialog wrapper:
- if (showEditor) { - return ( - <Dialog open={open} onOpenChange={setOpen}> - <DialogTrigger asChild> - <Button variant="text" size="sm" className="gap-2"> - <Keyboard className="w-4 h-4" /> - Shortcuts - </Button> - </DialogTrigger> - <DialogContent className="max-w-6xl max-h-[90vh] overflow-y-auto"> - <KeybindingEditor - shortcuts={shortcuts} - onClose={() => setShowEditor(false)} - /> - </DialogContent> - </Dialog> - ); - } return ( <Dialog open={open} onOpenChange={setOpen}> <DialogTrigger asChild> <Button variant="text" size="sm" className="gap-2"> <Keyboard className="w-4 h-4" /> Shortcuts </Button> </DialogTrigger> - <DialogContent className="max-w-2xl max-h-[80vh] overflow-y-auto"> + <DialogContent className={showEditor ? "max-w-6xl max-h-[90vh] overflow-y-auto" : "max-w-2xl max-h-[80vh] overflow-y-auto"}> + {showEditor ? ( + <KeybindingEditor + shortcuts={shortcuts} + onClose={() => setShowEditor(false)} + /> + ) : ( + <> <DialogHeader> <DialogTitle className="flex items-center gap-2"> <Keyboard className="w-5 h-5" /> Keyboard Shortcuts </DialogTitle> <DialogDescription> Speed up your video editing workflow with these keyboard shortcuts. Most shortcuts work when the timeline is focused. </DialogDescription> </DialogHeader> // ... rest of the help content + </> + )} </DialogContent> </Dialog> );
🧹 Nitpick comments (5)
apps/web/src/lib/utils.ts (2)
47-49: Consider simplifying the instanceof checkThe function works correctly but could be simplified since
HTMLElementextendsElement.export function isDOMElement(el: any): el is HTMLElement { - return !!el && (el instanceof Element || el instanceof HTMLElement); + return !!el && el instanceof HTMLElement; }Or if you need to support all DOM elements (not just HTML):
export function isDOMElement(el: any): el is HTMLElement { - return !!el && (el instanceof Element || el instanceof HTMLElement); + return !!el && el instanceof Element; }
51-65: Consider case-insensitive tagName comparisonThe function logic is correct, but
tagNameis always uppercase in the DOM. Consider using uppercase comparisons for consistency.export function isTypableElement(el: HTMLElement): boolean { // If content editable, then it is editable if (el.isContentEditable) return true; // If element is an input and the input is enabled, then it is typable - if (el.tagName === "INPUT") { + if (el.tagName === "INPUT") { return !(el as HTMLInputElement).disabled; } // If element is a textarea and the input is enabled, then it is typable - if (el.tagName === "TEXTAREA") { + if (el.tagName === "TEXTAREA") { return !(el as HTMLTextAreaElement).disabled; } return false; }Actually, the current implementation is fine since
tagNameis always uppercase in browsers. The function correctly identifies typable elements.apps/web/src/hooks/use-keybindings.ts (1)
75-76: Consider proper backward compatibility or removalThe empty
bindingsexport doesn't provide meaningful backward compatibility. Code relying on the old bindings will fail silently.Either implement a proper compatibility layer:
// Provide a compatibility layer that maps to the new system export const bindings = { get: () => { console.warn('bindings.get() is deprecated. Use useKeybindingsStore instead.'); return useKeybindingsStore.getState().keybindings; } };Or remove it entirely to fail fast and help developers migrate:
-// Export the bindings for backward compatibility -export const bindings = {};apps/web/src/components/keybinding-editor.tsx (1)
255-271: Use React ref instead of getElementByIdUsing
getElementByIdis not idiomatic React. Use a ref for better integration and type safety.+ const fileInputRef = useRef<HTMLInputElement>(null); // ... in the JSX: <input type="file" accept=".json" onChange={handleImportKeybindings} className="hidden" - id="import-keybindings" + ref={fileInputRef} /> <Button variant="outline" size="sm" - onClick={() => - document.getElementById("import-keybindings")?.click() - } + onClick={() => fileInputRef.current?.click()} >apps/web/src/constants/actions.ts (1)
14-27: Consider using Set for listener managementThe current array-based implementation works but could be optimized for better performance with many listeners.
class ActionEmitter { - private listeners: Array<(actions: Action[]) => void> = []; + private listeners = new Set<(actions: Action[]) => void>(); subscribe(listener: (actions: Action[]) => void) { - this.listeners.push(listener); + this.listeners.add(listener); return () => { - this.listeners = this.listeners.filter((l) => l !== listener); + this.listeners.delete(listener); }; } emit(actions: Action[]) { - this.listeners.forEach((listener) => listener(actions)); + this.listeners.forEach((listener) => listener(actions)); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
apps/web/src/components/editor-provider.tsx(1 hunks)apps/web/src/components/keybinding-editor.tsx(1 hunks)apps/web/src/components/keyboard-shortcuts-help.tsx(5 hunks)apps/web/src/constants/actions.ts(1 hunks)apps/web/src/hooks/use-editor-actions.ts(1 hunks)apps/web/src/hooks/use-keybinding-conflicts.ts(1 hunks)apps/web/src/hooks/use-keybindings.ts(1 hunks)apps/web/src/hooks/use-keyboard-shortcuts-help.ts(1 hunks)apps/web/src/hooks/use-keyboard-shortcuts.ts(0 hunks)apps/web/src/lib/utils.ts(2 hunks)apps/web/src/stores/keybindings-store.ts(1 hunks)
💤 Files with no reviewable changes (1)
- apps/web/src/hooks/use-keyboard-shortcuts.ts
🧰 Additional context used
🧠 Learnings (1)
apps/web/src/hooks/use-editor-actions.ts (1)
Learnt from: simonorzel26
PR: OpenCut-app/OpenCut#324
File: apps/web/src/components/editor/snap-indicator.tsx:43-43
Timestamp: 2025-07-17T08:26:10.929Z
Learning: In the timeline refactor PR #324, the snap indicator component in apps/web/src/components/editor/snap-indicator.tsx requires the hard-coded `ml-48` class in addition to the calculated `leftPosition` for proper alignment. This is intentional and needed for the new timeline layout.
🧬 Code Graph Analysis (4)
apps/web/src/components/editor-provider.tsx (3)
apps/web/src/stores/editor-store.ts (1)
useEditorStore(67-105)apps/web/src/hooks/use-keybindings.ts (2)
useKeybindingDisabler(60-73)useKeybindingsListener(17-55)apps/web/src/hooks/use-editor-actions.ts (1)
useEditorActions(10-162)
apps/web/src/hooks/use-keybinding-conflicts.ts (2)
apps/web/src/constants/actions.ts (1)
ActionWithOptionalArgs(81-83)apps/web/src/stores/keybindings-store.ts (1)
useKeybindingsStore(65-144)
apps/web/src/stores/keybindings-store.ts (2)
apps/web/src/constants/actions.ts (1)
ActionWithOptionalArgs(81-83)apps/web/src/lib/utils.ts (3)
isDOMElement(47-49)isTypableElement(51-65)isAppleDevice(67-69)
apps/web/src/components/keyboard-shortcuts-help.tsx (4)
apps/web/src/hooks/use-keyboard-shortcuts-help.ts (2)
KeyboardShortcut(7-14)useKeyboardShortcutsHelp(85-123)apps/web/src/components/ui/dialog.tsx (3)
Dialog(123-123)DialogTrigger(126-126)DialogContent(128-128)apps/web/src/components/ui/button.tsx (1)
Button(59-59)apps/web/src/components/keybinding-editor.tsx (1)
KeybindingEditor(116-414)
🔇 Additional comments (22)
apps/web/src/lib/utils.ts (4)
16-19: LGTM - Improved readabilityThe multiline formatting of the condition improves code readability without changing the logic.
32-45: LGTM - Consistent formattingThe reformatted UUID string construction maintains the same logic while using consistent double quotes and clear concatenation structure.
71-77: LGTM - Platform-specific key utilitiesBoth functions correctly return platform-appropriate key symbols for displaying keyboard shortcuts in the UI.
67-69: Consider migrating from deprecated navigator.platformWhile the current implementation correctly detects Apple devices, note that
navigator.platformis deprecated (still supported broadly but discouraged). When support fornavigator.userAgentDataimproves—today available in Chrome, Edge, and Opera but not in Firefox or Safari (including iOS)—update this function to prefernavigator.userAgentData.platformwith a fallback tonavigator.platform.• File: apps/web/src/lib/utils.ts Lines 67–69
Suggested future pattern:
export function isAppleDevice() { if ('userAgentData' in navigator && navigator.userAgentData.platform) { return /^(macOS|iOS)$/i.test(navigator.userAgentData.platform); } return /(Mac|iPhone|iPod|iPad)/i.test(navigator.platform); }apps/web/src/components/editor-provider.tsx (2)
6-24: LGTM - Well-structured modular approachThe refactoring successfully separates concerns by splitting keyboard shortcut handling into distinct responsibilities: action handling, event listening, and enable/disable control. This improves maintainability and testability.
27-33: LGTM - Proper keybinding lifecycle managementThe useEffect correctly manages keybinding state based on editor readiness, preventing keyboard shortcuts from interfering during initialization. The dependency array is complete and the logic aligns with the loading state conditions.
apps/web/src/hooks/use-editor-actions.ts (7)
28-77: LGTM - Comprehensive playback controlsAll playback actions are implemented correctly with proper bounds checking, fallback values, and appropriate state management. The frame stepping logic correctly uses project FPS with a sensible default.
79-101: LGTM - Robust split element implementationThe split element action correctly validates selection count, calculates effective element boundaries considering trim values, and ensures the playhead is positioned within the element before splitting. Error handling provides clear user feedback.
103-114: LGTM - Clean delete implementationThe delete action properly validates selection, removes all selected elements, and clears the selection state. The implementation handles the typical delete workflow correctly.
116-124: LGTM - Correct select all implementationThe select all action properly flattens all track elements and maps them to the required selection format. The use of flatMap is appropriate for this nested structure.
126-148: LGTM - Well-designed duplicate functionalityThe duplicate action correctly validates single selection, calculates appropriate positioning with a small gap to prevent overlap, and properly handles element ID generation for the new element. The implementation follows good practices for element duplication.
150-152: LGTM - Simple toggle implementationThe snapping toggle correctly delegates to the timeline store method.
154-162: LGTM - Proper history action delegationBoth undo and redo actions correctly delegate to the timeline store methods for history management.
apps/web/src/hooks/use-keyboard-shortcuts-help.ts (2)
85-123: LGTM - Well-structured hook implementationThe hook correctly transforms keybindings store data into a user-friendly format with proper memoization and grouping logic. The implementation efficiently handles the conversion from key-to-action mapping to action-to-keys grouping.
17-65: Double-check completeness of keyboard shortcut help mappingThe last automated check didn’t produce any output to confirm exact coverage. Please manually verify that the
actionDescriptionsobject in
•apps/web/src/hooks/use-keyboard-shortcuts-help.ts
includes every action string exported in
•apps/web/src/constants/actions.ts
- Open the
Actionunion inapps/web/src/constants/actions.ts- Confirm each action value appears as a key in
actionDescriptionsapps/web/src/hooks/use-keybinding-conflicts.ts (2)
16-39: LGTM - Efficient conflict detection algorithmThe conflict detection correctly groups actions by key, identifies unique actions using Set deduplication, and properly filters to return only actual conflicts. The logic is sound and efficient.
41-59: LGTM - Well-designed conflict query APIThe helper functions provide intuitive ways to query conflicts by key or action. The return object gives consumers all necessary conflict information with a clean, discoverable API.
apps/web/src/components/keyboard-shortcuts-help.tsx (1)
41-41: Good use of typed interfaceThe change to use the
KeyboardShortcutinterface improves type safety.apps/web/src/components/keybinding-editor.tsx (1)
143-152: Well-implemented search and filter logicThe search and category filtering is cleanly implemented with proper case-insensitive matching.
apps/web/src/constants/actions.ts (2)
61-95: Excellent type safety implementationThe type system effectively handles the complexity of actions with different argument requirements while maintaining strong type safety.
108-116: Clean global state managementThe global state management is well-implemented with proper encapsulation and notification mechanisms.
apps/web/src/stores/keybindings-store.ts (1)
209-223: Good platform-specific key handlingThe modifier key detection correctly handles platform differences between Apple (meta key) and other platforms (ctrl key).
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/web/src/components/keybinding-editor.tsx (1)
52-59: System shortcuts prevention issue persists.The handleKeyDown function still calls
e.preventDefault()unconditionally, which could interfere with critical system or browser shortcuts like Ctrl+R, Ctrl+W, etc.
🧹 Nitpick comments (1)
apps/web/src/components/keybinding-editor.tsx (1)
217-245: Import validation has been properly implemented.The import functionality now includes comprehensive validation for config structure and individual keybindings, addressing previous security concerns.
Consider improving the error message display:
- toast.error(`Failed to import keybindings: ${error}`); + toast.error(`Failed to import keybindings: ${error instanceof Error ? error.message : String(error)}`);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/web/src/components/keybinding-editor.tsx(1 hunks)apps/web/src/constants/actions.ts(1 hunks)apps/web/src/hooks/use-keybindings.ts(1 hunks)apps/web/src/stores/keybindings-store.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/web/src/hooks/use-keybindings.ts
- apps/web/src/constants/actions.ts
🔇 Additional comments (13)
apps/web/src/components/keybinding-editor.tsx (7)
1-42: Well-structured imports and type definitions.The imports are organized logically and the TypeScript interfaces are clean and well-defined.
44-51: Well-implemented key recording functionality.The KeyRecorder component has solid state management and provides clear user feedback during the recording process.
Also applies to: 60-114
116-141: Clean component setup and state management.The component properly integrates with the keybindings store and manages local state effectively.
143-152: Effective filtering and search implementation.The search and category filtering logic is well-implemented with appropriate case-insensitive matching.
154-201: Robust event handling with proper validation.The shortcut editing handlers include appropriate conflict validation, proper state management, and good user feedback through toast notifications.
203-216: Clean export functionality implementation.The export feature properly creates a downloadable JSON file with formatted keybindings data.
247-425: Comprehensive and well-structured UI implementation.The component provides an excellent user experience with intuitive editing controls, proper visual feedback, search/filtering capabilities, and appropriate confirmation dialogs for destructive actions.
apps/web/src/stores/keybindings-store.ts (6)
1-41: Well-defined types and comprehensive default keybindings.The type definitions are clean and the default keybindings provide a solid foundation with commonly expected shortcuts.
43-66: Comprehensive and well-organized store interface.The interface provides all necessary methods for keybinding management with clear categorization and typing.
114-126: Import validation has been properly implemented.The importKeybindings method now includes validation to ensure data integrity, addressing the previous review concern.
68-113: Solid store implementation with proper state management.The Zustand store is well-implemented with immutable updates, proper persistence configuration, and comprehensive method coverage.
Also applies to: 127-163
207-208: Space key handling has been properly implemented.The getPressedKey function now correctly handles the space key, addressing the previous review concern.
165-206: Comprehensive and well-implemented utility functions.The keybinding generation logic properly handles modifier detection, platform differences, and input context awareness. The key detection covers all necessary key types with appropriate validation.
Also applies to: 209-243
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/web/src/stores/keybindings-store.ts (2)
109-121: Validation improvements implemented, but could be more comprehensive.The validation logic addresses the previous review concern by checking key format. However, the validation could be enhanced to also verify that actions are valid.
Consider adding action validation:
importKeybindings: (config: KeybindingConfig) => { // Validate all keys and actions for (const [key, action] of Object.entries(config)) { // Validate the key format if (typeof key !== "string" || key.length === 0) { throw new Error(`Invalid key format: ${key}`); } + + // Validate action is defined + if (!action || typeof action !== "string") { + throw new Error(`Invalid action for key ${key}: ${action}`); + } } set({ keybindings: { ...config }, isCustomized: true, }); },
220-221: Consider expanding special key support.The current implementation handles common keys well, but could benefit from supporting additional special keys that might be useful in a video editor context.
Consider adding support for additional keys:
// Check if slash, period or enter if (key === "/" || key === "." || key === "enter") return key; + + // Add support for additional special keys + if (key === "escape" || key === "pageup" || key === "pagedown") return key; + if (key === "f1" || key === "f2" || key === "f3" || key === "f4" || + key === "f5" || key === "f6" || key === "f7" || key === "f8" || + key === "f9" || key === "f10" || key === "f11" || key === "f12") return key;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/src/stores/keybindings-store.ts(1 hunks)apps/web/src/types/keybinding.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/web/src/types/keybinding.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
apps/web/src/stores/keybindings-store.ts (3)
apps/web/src/types/keybinding.ts (2)
KeybindingConfig(33-35)ShortcutKey(31-31)apps/web/src/constants/actions.ts (1)
ActionWithOptionalArgs(81-83)apps/web/src/lib/utils.ts (3)
isDOMElement(47-49)isTypableElement(51-65)isAppleDevice(67-69)
🔇 Additional comments (5)
apps/web/src/stores/keybindings-store.ts (5)
9-30: Well-structured default keybindings configuration.The default keybindings provide comprehensive coverage for video editing actions with good platform considerations (ctrl+z/ctrl+y for undo/redo). The configuration is clean and follows consistent naming conventions.
32-61: Comprehensive and well-designed interfaces.The interfaces provide a clean separation of concerns with proper TypeScript typing. The KeybindingConflict interface and the categorized methods in KeybindingsState demonstrate good architectural planning.
195-225: Space key handling successfully implemented.The space key handling issue from the previous review has been properly addressed. The function now correctly handles space key events.
227-241: Excellent platform-aware modifier key handling.The modifier key logic correctly handles platform differences between Apple devices (meta key) and other platforms (ctrl key). The combination logic for multiple modifiers is well-implemented.
1-241: Excellent implementation of the keybindings store system.This implementation successfully delivers a comprehensive keyboard shortcuts management system as outlined in the PR objectives. The code demonstrates:
- Proper separation of concerns with validation, conflict detection, and utility methods
- Good TypeScript typing and Zustand patterns
- Platform-aware keyboard event handling
- Proper persistence configuration
- Resolution of previous review concerns
The store provides a solid foundation for the customizable keyboard shortcuts system in OpenCut.
|
Hey @mazeincoding, I'm still making some tweaks. Don’t merge it just yet. I’ll sort out the conflicts! |
…sHelp hook and update KeyBadge and ShortcutItem components
… state and updating related hooks
…and update keybinding handling
|
oh i was actually making a ton of changes, hmm |
|
i had also solved the merge conflicts ^ |
anwarulislam
force-pushed
the
feat/shortcut-actions
branch
from
367a7f9 to
e3d935e
Compare
|
@mazeincoding, good to go from my side. But still I see conflict issues. You can pull latest changes. |
2 participants
Keyboard Shortcuts System
This document describes the configurable keyboard shortcuts system in OpenCut.
Overview
The keyboard shortcuts system is built with the following components:
/src/constants/actions.ts) - Defines all available actions/src/stores/keybindings-store.ts) - Manages keybinding configuration/src/hooks/use-keybindings.ts) - Handles keyboard events/src/hooks/use-editor-actions.ts) - Implements action behaviorArchitecture
Actions System
Actions are the core verbs that can be performed in the editor:
Actions can have optional arguments:
Keybindings Store
The store manages:
Key Features
Usage
Basic Usage
Advanced Features
Export Keybindings
Import Keybindings
Programmatic Updates
Adding New Actions
Actiontype inactions.tsActionArgsMapuseEditorActionshookactionDescriptionsin help hookdefaultKeybindingsin storeExample:
Key Format
Keys are represented as strings with modifiers separated by dashes:
"a"- Simple key"ctrl-a"- Ctrl+A (Cmd+A on macOS)"ctrl-shift-a"- Ctrl+Shift+A"alt-left"- Alt+Left ArrowSupported modifiers:
ctrl- Control key (Command on macOS)alt- Alt key (Option on macOS)shift- Shift keyValidation
The system includes validation to prevent:
Persistence
Keybindings are automatically saved to localStorage with the key
opencut-keybindings.Summary by CodeRabbit
New Features
Bug Fixes
Chores