refactor(core): extract keyboard shortcuts logic from core.ts#930
refactor(core): extract keyboard shortcuts logic from core.ts#930hatemhosny merged 4 commits intolive-codes:developfrom
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. |
BassemHalim
changed the title
WalkthroughKeyboard shortcut handling was moved from core into a new dependency-injected handler exported as Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Window as Window (keydown)
participant Events as EventsManager
participant Core as Core (wires deps)
participant Handler as KeyboardShortcuts Handler
participant Editor as Active Editor
participant UI as UI (buttons/panes)
Note over Window,Events: Global keydown captured and routed
Window->>Events: keydown event
Events->>Core: dispatch registered listener
Core->>Handler: handleKeyboardShortcuts invoked (on init) -> handler receives deps
Handler->>Handler: maintain lastkeys / match shortcut patterns
alt Shortcut requires editor action
Handler->>Editor: query/execute commands (focus, run, command palette)
else Shortcut manipulates UI
Handler->>UI: click/toggle/hide/show panes (Console, Result, Tools)
else Project or navigation action
Handler->>Core: call run/showEditor/split/etc via deps
end
Note right of Handler: respects embed mode & missing UI elements
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
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]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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).
|
Thanks a lot @BassemHalim I have a few suggestions:
const createPreventBookmarkHandler = () => (e: KeyboardEvent) => {
if (ctrl(e) && e.code === 'KeyD') {
e.preventDefault();
return true;
}
return false;
};
export const setupKeyboardShortcuts = (deps: KeyboardShortcutDeps): void => {
let lastkeys = '';
const hotKeys = (e: KeyboardEvent) => {
// ...
if (createPreventBookmarkHandler()(e)) {
lastkeys = 'Ctrl + D';
return;
}
// ...
}
}I suggest moving let lastkeys = '';
const createPreventBookmarkHandler = () => (e: KeyboardEvent) => {
if (ctrl(e) && e.code === 'KeyD') {
e.preventDefault();
lastkeys = 'Ctrl + D';
return true;
}
return false;
};
export const setupKeyboardShortcuts = (deps: KeyboardShortcutDeps): void => {
const hotKeys = (e: KeyboardEvent) => {
// ...
if (createPreventBookmarkHandler()(e)) return;
// ...
}
}
Thanks a lot. I really appreciate you effort and approach. |
- Move lastkeys state outside handlers and assign inside each handler - Rename setupKeyboardShortcuts to handleKeyboardShortcuts for consistency - Simplify hotKeys function by removing redundant lastkeys assignments
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/livecodes/handlers/keyboard-shortcuts.ts (2)
160-166: Logic error with|| 0fallback remains unaddressed.This was flagged in a previous review:
-1 || 0evaluates to-1because-1is truthy in JavaScript. WhenfindIndexreturns-1(editor not found), the fallback to0never occurs, causing incorrect navigation.
62-62: Inconsistentlastkeysformat remains unaddressed.This was flagged in a previous review:
'Ctrl + Alt + C, F'uses a comma separator, which is inconsistent with other assignments like'Ctrl + Alt + C'.
🧹 Nitpick comments (1)
src/livecodes/handlers/keyboard-shortcuts.ts (1)
251-306: Handler factories are recreated on every keydown event.Each keydown invokes all factory functions (e.g.,
createCommandPaletteHandler(deps)), allocating new function objects even when the shortcut doesn't match. While the current approach is readable, you could memoize handlers once at registration time for a minor performance improvement.This is not urgent—keyboard events are infrequent compared to render loops—but worth noting if you revisit this module.
🔎 Optional optimization sketch
export const handleKeyboardShortcuts = (deps: KeyboardShortcutDeps): void => { + // Create handlers once + const handlers = [ + createCommandPaletteHandler(deps), + createPreventBookmarkHandler(), + createConsoleToggleHandler(), + createConsoleMaximizeHandler(), + createRunTestsHandler(), + createRunHandler(), + createResultToggleHandler(), + createZoomToggleHandler(), + createFocusEditorHandler(deps), + createEscapeHandler(deps), + createEditorSwitchHandler(deps), + createNewProjectHandler(deps), + createOpenProjectHandler(deps), + createImportHandler(deps), + createShareHandler(deps), + createForkHandler(deps), + createSaveHandler(deps), + createFocusModeHandler(deps), + ]; + const hotKeys = (e: KeyboardEvent) => { - if (createCommandPaletteHandler(deps)(e)) return; - if (createPreventBookmarkHandler()(e)) return; - // ... etc + for (const handler of handlers) { + if (handler(e)) return; + } + // Update lastkeys for non-modifier keys + if (!ctrl(e) && !e.altKey && !e.shiftKey) { + lastkeys = e.key; + } }; deps.eventsManager.addEventListener(window, 'keydown', hotKeys, true); };
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/livecodes/core.tssrc/livecodes/handlers/__tests__/keyboard-shortcuts.test.tssrc/livecodes/handlers/keyboard-shortcuts.ts
🚧 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 (3)
EventsManager(1978-1991)CodeEditor(1523-1560)EditorId(1175-1175)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)handleKeyboardShortcuts(251-309)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: build
- GitHub Check: build (24.x)
- GitHub Check: tests (24.x, 2)
- GitHub Check: tests (24.x, 4)
- GitHub Check: tests (24.x, 3)
- GitHub Check: tests (24.x, 1)
- GitHub Check: tests (24.x, 5)
🔇 Additional comments (3)
src/livecodes/handlers/__tests__/keyboard-shortcuts.test.ts (1)
27-97: Comprehensive test setup with sensible defaults.The mock structure provides good isolation for unit testing the keyboard shortcuts module. The
simulateKeydownhelper andbeforeEachsetup create a consistent testing environment.Note: As the reviewer mentioned, the heavy reliance on mocks couples tests to implementation details (e.g., clicking UI buttons, dispatching events). This is an acceptable trade-off for unit tests, but consider complementing with integration or e2e tests for higher confidence.
src/livecodes/handlers/keyboard-shortcuts.ts (2)
9-23: Clean interface design with module-level state as suggested.The
KeyboardShortcutDepsinterface provides good dependency injection, and optional properties (toolsPane,split) are correctly typed. The module-levellastkeysstate follows the reviewer's suggestion to keep state near key-check logic.
118-147: Well-structured escape key state machine.The three-level escape sequence (hide menus → focus console/focus button → focus logo) is clearly documented and handles the toolsPane status checks gracefully. The optional chaining on UI element access prevents runtime errors when elements are missing.
| it('should return false when lastkeys is not Ctrl+Alt+C', () => { | ||
| const mockConsoleButton = { dispatchEvent: jest.fn() }; | ||
| (UI.getConsoleButton as jest.Mock).mockReturnValue(mockConsoleButton); | ||
| const mockFocusButton = { click: jest.fn() }; | ||
| (UI.getFocusButton as jest.Mock).mockReturnValue(mockFocusButton); | ||
|
|
||
| // Press Ctrl+Alt+F without prior Ctrl+Alt+C (in non-embed mode, this triggers focus mode) | ||
| mockDeps.isEmbed = false; | ||
| handleKeyboardShortcuts(mockDeps); | ||
| simulateKeydown({ ctrlKey: true, altKey: true, code: 'KeyF' }); | ||
|
|
||
| // Console maximize should not be triggered (dblclick event) | ||
| const dblclickCalls = mockConsoleButton.dispatchEvent.mock.calls.filter( | ||
| (call: any) => call[0]?.type === 'dblclick', | ||
| ); | ||
| expect(dblclickCalls.length).toBe(0); | ||
| }); |
There was a problem hiding this comment.
Double handler registration may produce unreliable results.
Line 214 calls handleKeyboardShortcuts(mockDeps) a second time after beforeEach already registered the handler. This adds a second keydown listener (the first still references the old lastkeys state). The test may pass for the wrong reason or become flaky.
Consider removing the duplicate registration:
🔎 Proposed fix
it('should return false when lastkeys is not Ctrl+Alt+C', () => {
const mockConsoleButton = { dispatchEvent: jest.fn() };
(UI.getConsoleButton as jest.Mock).mockReturnValue(mockConsoleButton);
const mockFocusButton = { click: jest.fn() };
(UI.getFocusButton as jest.Mock).mockReturnValue(mockFocusButton);
// Press Ctrl+Alt+F without prior Ctrl+Alt+C (in non-embed mode, this triggers focus mode)
mockDeps.isEmbed = false;
- handleKeyboardShortcuts(mockDeps);
simulateKeydown({ ctrlKey: true, altKey: true, code: 'KeyF' });
// Console maximize should not be triggered (dblclick event)
const dblclickCalls = mockConsoleButton.dispatchEvent.mock.calls.filter(
(call: any) => call[0]?.type === 'dblclick',
);
expect(dblclickCalls.length).toBe(0);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('should return false when lastkeys is not Ctrl+Alt+C', () => { | |
| const mockConsoleButton = { dispatchEvent: jest.fn() }; | |
| (UI.getConsoleButton as jest.Mock).mockReturnValue(mockConsoleButton); | |
| const mockFocusButton = { click: jest.fn() }; | |
| (UI.getFocusButton as jest.Mock).mockReturnValue(mockFocusButton); | |
| // Press Ctrl+Alt+F without prior Ctrl+Alt+C (in non-embed mode, this triggers focus mode) | |
| mockDeps.isEmbed = false; | |
| handleKeyboardShortcuts(mockDeps); | |
| simulateKeydown({ ctrlKey: true, altKey: true, code: 'KeyF' }); | |
| // Console maximize should not be triggered (dblclick event) | |
| const dblclickCalls = mockConsoleButton.dispatchEvent.mock.calls.filter( | |
| (call: any) => call[0]?.type === 'dblclick', | |
| ); | |
| expect(dblclickCalls.length).toBe(0); | |
| }); | |
| it('should return false when lastkeys is not Ctrl+Alt+C', () => { | |
| const mockConsoleButton = { dispatchEvent: jest.fn() }; | |
| (UI.getConsoleButton as jest.Mock).mockReturnValue(mockConsoleButton); | |
| const mockFocusButton = { click: jest.fn() }; | |
| (UI.getFocusButton as jest.Mock).mockReturnValue(mockFocusButton); | |
| // Press Ctrl+Alt+F without prior Ctrl+Alt+C (in non-embed mode, this triggers focus mode) | |
| mockDeps.isEmbed = false; | |
| simulateKeydown({ ctrlKey: true, altKey: true, code: 'KeyF' }); | |
| // Console maximize should not be triggered (dblclick event) | |
| const dblclickCalls = mockConsoleButton.dispatchEvent.mock.calls.filter( | |
| (call: any) => call[0]?.type === 'dblclick', | |
| ); | |
| expect(dblclickCalls.length).toBe(0); | |
| }); |
🤖 Prompt for AI Agents
In src/livecodes/handlers/__tests__/keyboard-shortcuts.test.ts around lines 206
to 222, the test calls handleKeyboardShortcuts(mockDeps) again (line ~214) even
though beforeEach already registered the keydown handler, causing a duplicate
listener that can reference stale lastkeys and make the test flaky; remove the
extra handleKeyboardShortcuts(mockDeps) call from this test (or replace it with
logic to remove existing listener before re-registering) so the keydown handler
is only registered once per test lifecycle and the assertions reliably reflect
the intended behavior.
|
Hi Dr @hatemhosny
Happy New Year! 🎆 Wishing you a great year ahead! |
|
Thank you @BassemHalim ❤️ |
2 participants
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.