refactor(core): extract keyboard shortcuts logic from core.ts #930
Conversation
- extracted handleKeyboardShortcuts into it's own module - added tests to verify functionality
✅ Deploy Preview for livecodes ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughKeyboard shortcut handling was moved out of core into a new dependency-injected handler module. A exported Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Window as Window (keydown)
participant Events as EventsManager
participant Core as Core / getActiveEditor
participant Editor as Editor (Monaco)
participant UI as UI (buttons, panes, links)
Note over Window,Events: Global keydown captured via EventsManager
Window->>Events: keydown event
Events->>Core: invoke setupKeyboardShortcuts handler
Core->>Editor: query active editor / execute commands (e.g., Command Palette, Run)
Core->>UI: query buttons (Run, Run Tests, Console, Result, Zoom, Save, etc.)
alt Single-key or modifier shortcut
Core->>UI: click / dblclick / focus / dispatch as needed
else Multi-step sequence (e.g., Esc then other)
Core->>Core: update lastkeys state
Core->>UI: conditional hide/show or focus flows
end
Note over Editor,UI: Handlers respect embed mode and missing elements
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/livecodes/handlers/keyboard-shortcuts.ts (3)
8-18: Consider stronger typing forshowScreenandsplit.Using
anyforshowScreenandsplitreduces type safety. Consider importing or defining the appropriate types.export interface KeyboardShortcutDeps { eventsManager: EventsManager; getActiveEditor: () => CodeEditor; getConfig: () => Config; showEditor: (editorId: EditorId) => void; run: () => Promise<void>; - showScreen: any; + showScreen: (screen: string, options?: any) => Promise<void>; toolsPane?: ToolsPane; - split?: any; + split?: { show: (pane: string, full?: boolean) => void } | null; isEmbed: boolean; }
33-39: Optional chaining onpreventDefaultis unnecessary.
preventDefaultis always defined onKeyboardEvent. The optional chaining suggests uncertainty about the event type.const createPreventBookmarkHandler = () => (e: KeyboardEvent) => { if (ctrl(e) && e.code === 'KeyD') { - e.preventDefault?.(); + e.preventDefault(); return true; } return false; };
230-230: Remove unnecessaryasyncmodifier.The
hotKeysfunction is markedasyncbut doesn't contain anyawaitexpressions. This adds unnecessary overhead.- const hotKeys = async (e: KeyboardEvent) => { + const hotKeys = (e: KeyboardEvent) => {src/livecodes/handlers/__tests__/keyboard-shortcuts.test.ts (1)
400-404: Add test for ArrowLeft wrap-around from first editor.The test validates ArrowRight wrap-around (script → markup), but there's no test for ArrowLeft wrap-around from the first editor (markup → script). Given the bug identified in the implementation, this test would catch the issue.
it('should wrap around when switching beyond available editors', () => { mockConfig.activeEditor = 'script'; simulateKeydown({ ctrlKey: true, altKey: true, key: 'ArrowRight' }); expect(mockDeps.showEditor).toHaveBeenCalledWith('markup'); }); + + it('should wrap around when switching before first editor', () => { + mockConfig.activeEditor = 'markup'; + simulateKeydown({ ctrlKey: true, altKey: true, key: 'ArrowLeft' }); + expect(mockDeps.showEditor).toHaveBeenCalledWith('script'); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/livecodes/core.ts(2 hunks)src/livecodes/handlers/__tests__/keyboard-shortcuts.test.ts(1 hunks)src/livecodes/handlers/index.ts(1 hunks)src/livecodes/handlers/keyboard-shortcuts.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/livecodes/handlers/__tests__/keyboard-shortcuts.test.ts (2)
src/livecodes/handlers/keyboard-shortcuts.ts (2)
KeyboardShortcutDeps(8-18)setupKeyboardShortcuts(227-342)src/livecodes/utils/utils.ts (1)
ctrl(73-73)
src/livecodes/core.ts (1)
src/livecodes/handlers/keyboard-shortcuts.ts (1)
setupKeyboardShortcuts(227-342)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Redirect rules - livecodes
- GitHub Check: Header rules - livecodes
- GitHub Check: tests (24.x, 3)
- GitHub Check: tests (24.x, 4)
- GitHub Check: tests (24.x, 1)
- GitHub Check: tests (24.x, 2)
- GitHub Check: tests (24.x, 5)
- GitHub Check: build (24.x)
- GitHub Check: build
- GitHub Check: Pages changed - livecodes
🔇 Additional comments (10)
src/livecodes/handlers/index.ts (1)
1-2: LGTM! Clean barrel export setup.The barrel export pattern is well-established and the comment clearly indicates the module's purpose. This provides a consolidated import path for the handlers module.
src/livecodes/core.ts (2)
48-48: LGTM! Import correctly placed.The import follows the existing pattern and is properly grouped with other handler-related imports.
4962-4973: No action needed—toolsPaneis properly handled as optional.Confirmed:
toolsPaneis marked as optional in theKeyboardShortcutDepsinterface (toolsPane?: ToolsPane), and the keyboard-shortcuts module safely accesses it using optional chaining (deps.toolsPane?.getStatus()). The logical AND short-circuit evaluation ensures thatgetActiveTool()is only called whentoolsPaneexists.src/livecodes/handlers/keyboard-shortcuts.ts (2)
227-342: Good extraction with clean dependency injection.The overall implementation follows good practices:
- Pure handler factory functions enable testability
- Clear separation of concerns for each shortcut
- Proper use of optional chaining for nullable dependencies
- Consistent pattern across all handlers
144-150: Edge case bug in ArrowLeft navigation when active editor is 'markup'.When
activeEditoris'markup'(index 0), pressing ArrowLeft calculates0 - 1 = -1, then|| 0converts it to0, keeping focus on markup instead of wrapping to the last editor. The|| 0fallback incorrectly handles the-1case since-1is falsy in JavaScript but represents a valid "wrap to end" scenario.const index = ['1', '2', '3'].includes(e.key) ? Number(e.key) - 1 : e.key === 'ArrowLeft' - ? editorIds.findIndex((id) => id === deps.getConfig().activeEditor) - 1 || 0 + ? editorIds.findIndex((id) => id === deps.getConfig().activeEditor) - 1 : e.key === 'ArrowRight' - ? editorIds.findIndex((id) => id === deps.getConfig().activeEditor) + 1 || 0 + ? editorIds.findIndex((id) => id === deps.getConfig().activeEditor) + 1 : 0;The wrap-around logic on lines 152-153 already handles negative indices correctly with
index === -1 ? editorIds.length - 1 : index, so the|| 0is both unnecessary and causes this bug.Likely an incorrect or invalid review comment.
src/livecodes/handlers/__tests__/keyboard-shortcuts.test.ts (5)
1-26: LGTM! Well-structured test mocks.The mocking approach is clean and isolates the unit under test effectively. The mocks for UI selectors and the
ctrlutility enable thorough testing without DOM dependencies.
36-46: Good helper function for simulating keyboard events.The
simulateKeydownhelper provides a clean way to create mock keyboard events with sensible defaults and returns the event for assertions. This pattern reduces boilerplate across tests.
100-125: Solid basic setup tests.The tests verify that the event listener is properly registered with the correct parameters (window, 'keydown', handler, capture=true).
309-365: Comprehensive Escape key sequence tests.The multi-step Escape key handling tests are thorough, covering single, double, and triple Escape presses with appropriate assertions for each state transition.
542-579: Good edge case coverage for missing dependencies.Tests verify graceful handling when UI elements, split pane, or toolsPane are missing. This ensures robustness in different application states.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/livecodes/handlers/keyboard-shortcuts.ts (1)
227-342: Consider exposing a cleanup function.The module registers a global event listener on window but doesn't export a corresponding cleanup/teardown function. If
setupKeyboardShortcutsis called multiple times or needs to be cleaned up during component unmounting or testing, this could lead to memory leaks or duplicate event handlers.Consider returning a cleanup function:
export const setupKeyboardShortcuts = (deps: KeyboardShortcutDeps): void => { +export const setupKeyboardShortcuts = (deps: KeyboardShortcutDeps): (() => void) => { let lastkeys = ''; const hotKeys = (e: KeyboardEvent) => { // ... handler logic ... }; deps.eventsManager.addEventListener(window, 'keydown', hotKeys, true); + + return () => { + deps.eventsManager.removeEventListener(window, 'keydown', hotKeys); + }; };src/livecodes/handlers/__tests__/keyboard-shortcuts.test.ts (1)
117-123: Consider adding a test for lastkeys reset behavior.The current tests don't explicitly verify the behavior of lines 335-338 in the source code, where
lastkeysis updated for non-modifier keys. While this might be intentional behavior, adding a test would document the expected interaction between regular key presses and multi-key sequences.Example test:
it('should reset lastkeys when non-modifier key is pressed', () => { setupKeyboardShortcuts(mockDeps); // Start an Escape sequence simulateKeydown({ code: 'Escape' }); // Press a regular key simulateKeydown({ key: 'a' }); // The next Escape should start fresh (not continue the sequence) const mockFocusButton = { focus: jest.fn() }; (UI.getFocusButton as jest.Mock).mockReturnValue(mockFocusButton); simulateKeydown({ code: 'Escape' }); simulateKeydown({ code: 'Escape' }); // Should focus focus button (second Escape), not logo (third Escape) expect(mockFocusButton.focus).toHaveBeenCalled(); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/livecodes/core.ts(2 hunks)src/livecodes/handlers/__tests__/keyboard-shortcuts.test.ts(1 hunks)src/livecodes/handlers/keyboard-shortcuts.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/livecodes/core.ts
🧰 Additional context used
🧬 Code graph analysis (2)
src/livecodes/handlers/keyboard-shortcuts.ts (3)
src/sdk/models.ts (4)
EventsManager(1976-1989)CodeEditor(1521-1558)EditorId(1173-1173)ToolsPane(1505-1519)src/livecodes/UI/split-panes.ts (1)
createSplitPanes(4-103)src/livecodes/utils/utils.ts (1)
ctrl(73-73)
src/livecodes/handlers/__tests__/keyboard-shortcuts.test.ts (2)
src/livecodes/handlers/keyboard-shortcuts.ts (2)
KeyboardShortcutDeps(9-18)setupKeyboardShortcuts(227-342)src/livecodes/utils/utils.ts (1)
ctrl(73-73)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Redirect rules - livecodes
- GitHub Check: Header rules - livecodes
- GitHub Check: Pages changed - livecodes
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: tests (24.x, 5)
- GitHub Check: tests (24.x, 4)
- GitHub Check: tests (24.x, 2)
- GitHub Check: tests (24.x, 1)
- GitHub Check: tests (24.x, 3)
- GitHub Check: build (24.x)
- GitHub Check: build
🔇 Additional comments (2)
src/livecodes/handlers/keyboard-shortcuts.ts (1)
335-338: Review lastkeys update behavior for potential side effects.The fallback logic that updates
lastkeysfor any non-modifier key might unintentionally reset multi-key sequences. For example, if a user is in the middle of an Escape sequence and presses a regular key, this resets the sequence state. While this might be intentional, it's worth verifying that this doesn't interfere with expected multi-key workflows.Verify this behavior aligns with the intended user experience for multi-key sequences.
src/livecodes/handlers/__tests__/keyboard-shortcuts.test.ts (1)
27-623: Excellent test coverage!The test suite is comprehensive and well-structured, covering:
- All keyboard shortcuts (happy paths and edge cases)
- Multi-step key sequences (Escape, Console maximize)
- Embed mode restrictions
- Editor switching with complex logic (wrapping, hidden editors)
- Graceful handling of missing UI elements and dependencies
- Key combination differentiation
The tests will help catch regressions as the codebase evolves.
| const index = ['1', '2', '3'].includes(e.key) | ||
| ? Number(e.key) - 1 | ||
| : e.key === 'ArrowLeft' | ||
| ? editorIds.findIndex((id) => id === deps.getConfig().activeEditor) - 1 || 0 | ||
| : e.key === 'ArrowRight' | ||
| ? editorIds.findIndex((id) => id === deps.getConfig().activeEditor) + 1 || 0 | ||
| : 0; | ||
|
|
||
| const editorIndex = | ||
| index === editorIds.length ? 0 : index === -1 ? editorIds.length - 1 : index; | ||
|
|
||
| deps.showEditor(editorIds[editorIndex]); | ||
| return true; | ||
| } | ||
| return false; | ||
| }; |
There was a problem hiding this comment.
Fix logic error in fallback handling.
The || 0 fallback operators on Lines 147 and 149 don't work as intended. In JavaScript, -1 || 0 evaluates to -1 (not 0) because -1 is truthy. This means when findIndex returns -1 (not found), the fallback to 0 never occurs, leading to incorrect index calculations.
Additionally, the deeply nested ternary is difficult to read and maintain.
Apply this diff to fix the logic and improve readability:
- const index = ['1', '2', '3'].includes(e.key)
- ? Number(e.key) - 1
- : e.key === 'ArrowLeft'
- ? editorIds.findIndex((id) => id === deps.getConfig().activeEditor) - 1 || 0
- : e.key === 'ArrowRight'
- ? editorIds.findIndex((id) => id === deps.getConfig().activeEditor) + 1 || 0
- : 0;
+ let index = 0;
+ if (['1', '2', '3'].includes(e.key)) {
+ index = Number(e.key) - 1;
+ } else if (e.key === 'ArrowLeft') {
+ const currentIndex = editorIds.findIndex((id) => id === deps.getConfig().activeEditor);
+ index = currentIndex !== -1 ? currentIndex - 1 : 0;
+ } else if (e.key === 'ArrowRight') {
+ const currentIndex = editorIds.findIndex((id) => id === deps.getConfig().activeEditor);
+ index = currentIndex !== -1 ? currentIndex + 1 : 0;
+ }🤖 Prompt for AI Agents
In src/livecodes/handlers/keyboard-shortcuts.ts around lines 144 to 159, the
nested ternary uses `|| 0` fallbacks which fail when findIndex returns -1
(because -1 is truthy) and the logic is hard to read; replace the nested ternary
with explicit steps: get currentIndex = editorIds.findIndex(id => id ===
deps.getConfig().activeEditor), then compute index based on key (if '1'|'2'|'3'
-> Number(e.key)-1; if 'ArrowLeft' -> currentIndex === -1 ? 0 : currentIndex -
1; if 'ArrowRight' -> currentIndex === -1 ? 0 : currentIndex + 1; else 0), then
normalize editorIndex by wrapping boundaries (if index === editorIds.length ->
0; if index < 0 -> editorIds.length - 1; else index), and finally call
deps.showEditor(editorIds[editorIndex]); this removes the faulty `|| 0` and
makes the logic explicit and readable.
| if (createConsoleMaximizeHandler(lastkeys)(e)) { | ||
| lastkeys = 'Ctrl + Alt + C, F'; | ||
| return; |
There was a problem hiding this comment.
Inconsistent lastkeys format.
Line 251 uses the format 'Ctrl + Alt + C, F', which is inconsistent with other lastkeys assignments (e.g., 'Ctrl + Alt + C', 'Shift + Enter'). This inconsistency could cause confusion during debugging or if lastkeys values are used elsewhere.
Consider using a consistent format:
- lastkeys = 'Ctrl + Alt + C, F';
+ lastkeys = 'Ctrl + Alt + F';🤖 Prompt for AI Agents
In src/livecodes/handlers/keyboard-shortcuts.ts around lines 250 to 252, the
assignment lastkeys = 'Ctrl + Alt + C, F' uses a comma which is inconsistent
with other lastkeys values (e.g., 'Ctrl + Alt + C', 'Shift + Enter'); change the
format to use the same separator as other assignments — e.g., replace the comma
with ' + ' so it becomes 'Ctrl + Alt + C + F' (and scan nearby assignments to
ensure all lastkeys use the same ' + ' separator convention).
|
What type of PR is this? (check all applicable)
Description
This PR is to reduce the size of core.ts. It drops the size of core.ts by 194 LOC (although it slightly increases the overall size of the project)
Related Tickets & Documents
Mobile & Desktop Screenshots/Recordings
Added tests?
Added to documentations?
[optional] Are there any post-deployment tasks we need to perform?
[optional] What gif best describes this PR or how it makes you feel?
Summary by CodeRabbit
New Features
Tests
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.