JS to TS: src/simulator/src/hotkey_binder/model/shortcuts.plugin.ts #425
Conversation
|
Warning Rate limit exceeded@ThatDeparted2061 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 22 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThe changes involve migrating the keyboard shortcut management functionality from a JavaScript implementation to a TypeScript implementation. The new version introduces type definitions and interfaces to enhance type safety and provide more structured shortcut management. The core functionality remains similar, with methods for adding, removing, and managing keyboard shortcuts, but now with improved type annotations and more explicit type checking. Additionally, a new TypeScript file defines several interfaces relevant to the hotkey binding functionality. Changes
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
❌ Deploy Preview for circuitverse failed.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
src/simulator/src/hotkey_binder/model/shortcuts.plugin.ts (5)
1-18: Add TypeScript-specific documentation.While the existing documentation covers usage restrictions well, consider adding:
- Type information for key parameters
- Examples of valid shortcut combinations using TypeScript
- Return type documentation
19-25: Improve type safety for target property.Instead of using
stringfor the target property, consider using a more specific type likeHTMLElement['id']to ensure only valid element IDs are passed.- target?: Document | string + target?: Document | HTMLElement['id']
52-55: Replace type assertion with type guard.Instead of using type assertion, implement a type guard for better type safety.
-let ele: Document | HTMLElement = options.target as Document | HTMLElement -if (typeof options.target === 'string') { - ele = document.getElementById(options.target) || document -} +const isDocumentTarget = (target: ShortcutOptions['target']): target is Document => + target instanceof Document + +let ele: Document | HTMLElement +if (typeof options.target === 'string') { + ele = document.getElementById(options.target) || document +} else { + ele = isDocumentTarget(options.target) ? options.target : document +}
164-170: Modernize event listener attachment.The code uses older event attachment methods for compatibility. Consider modernizing this with a proper fallback strategy.
- if (ele.addEventListener) { - ele.addEventListener(options.type || 'keydown', func, false) - } else if ((ele as any).attachEvent) { - (ele as any).attachEvent('on' + (options.type || 'keydown'), func) - } else { - (ele as any)['on' + (options.type || 'keydown')] = func - } + const eventType = options.type || 'keydown' + try { + ele.addEventListener(eventType, func, false) + } catch (error) { + console.warn(`Failed to attach event listener: ${error}`) + // Fallback for older browsers + try { + (ele as any).attachEvent?.('on' + eventType, func) + } catch (fallbackError) { + console.error(`Event listener attachment failed: ${fallbackError}`) + } + }
38-40: Consider adding cleanup method for memory management.The
all_shortcutsobject could grow indefinitely. Consider adding a method to clean up unused shortcuts and prevent memory leaks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/simulator/src/hotkey_binder/model/shortcuts.plugin.js(0 hunks)src/simulator/src/hotkey_binder/model/shortcuts.plugin.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/simulator/src/hotkey_binder/model/shortcuts.plugin.js
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/simulator/src/hotkey_binder/model/shortcuts.plugin.ts (5)
17-17: Enhance modification documentation.The comment "*! This plugin has been modified" should include details about what modifications were made, especially since this is a TypeScript migration.
-//*! This plugin has been modified +//*! This plugin has been modified to: +// - Migrate from JavaScript to TypeScript +// - Add type safety through interfaces +// - Improve error handling
19-25: Improve type definitions for better type safety.The
ShortcutOptionsinterface could benefit from more specific types:interface ShortcutOptions { - type?: string + type?: 'keydown' | 'keyup' | 'keypress' - target?: Document | string + target?: Document | HTMLElement | string propagate?: boolean disable_in_input?: boolean keycode?: number | false }
52-55: Improve target element resolution.The target element resolution could be more robust by:
- Adding type guard
- Handling case when element is not found
-let ele: Document | HTMLElement = options.target as Document | HTMLElement -if (typeof options.target === 'string') { - ele = document.getElementById(options.target) || document -} +let ele: Document | HTMLElement +if (typeof options.target === 'string') { + const targetElement = document.getElementById(options.target) + if (!targetElement) { + console.warn(`Target element "${options.target}" not found, falling back to document`) + ele = document + } else { + ele = targetElement + } +} else { + ele = options.target as Document | HTMLElement +}
84-105: Move key mappings to constants.The shift numbers and special keys mappings should be moved outside the function to avoid recreation on each call.
+const SHIFT_NUMS: Readonly<Record<string, string>> = { + '`': '~', '1': '!', '2': '@', '3': '#', '4': '$', '5': '%', + '6': '^', '7': '&', '8': '*', '9': '(', '0': ')', + '-': '_', '=': '+', ';': ':', "'": '"', + ',': '<', '.': '>', '/': '?', '\\': '|' +} as const + +const SPECIAL_KEYS: Readonly<Record<string, number>> = { + esc: 27, escape: 27, tab: 9, space: 32, + // ... rest of the special keys +} as const + const func: EventListener = (evt: Event): void => { // ... - const shift_nums: Record<string, string> = { - '`': '~', 1: '!', 2: '@', // ... - } - const special_keys: Record<string, number> = { - esc: 27, escape: 27, // ... - } + const shift_nums = SHIFT_NUMS + const special_keys = SPECIAL_KEYS
168-174: Consider modernizing event binding.The code includes legacy event binding methods that might not be necessary for modern browsers.
- if (ele.addEventListener) { - ele.addEventListener(options.type || 'keydown', func, false) - } else if ((ele as any).attachEvent) { - (ele as any).attachEvent('on' + (options.type || 'keydown'), func) - } else { - (ele as any)['on' + (options.type || 'keydown')] = func - } + ele.addEventListener(options.type || 'keydown', func, false)If legacy browser support is still required, consider adding a comment explaining why:
// Note: attachEvent and on* event handlers are kept for legacy browser support
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/simulator/src/hotkey_binder/model/shortcuts.plugin.ts(1 hunks)
🔇 Additional comments (1)
src/simulator/src/hotkey_binder/model/shortcuts.plugin.ts (1)
177-210: LGTM! Error handling looks good.The error handling in both
removeandremoveAllmethods is well implemented, following the suggestions from previous reviews.
|
My code takes almost same lines as the previous code, so adhering to code climate doesn't seem plausible |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/simulator/src/hotkey_binder/model/shortcuts.plugin.ts (5)
1-19: Add JSDoc documentation and improve type definitions.The interfaces would benefit from documentation and some type improvements:
+/** Options for configuring a keyboard shortcut */ interface ShortcutOptions { - type?: string + type?: 'keydown' | 'keyup' | 'keypress' propagate?: boolean disable_in_input?: boolean target?: Document | string keycode?: number | false } +/** Represents a registered shortcut binding */ interface ShortcutBinding { callback: EventListener target: Document | HTMLElement - event: string + event: 'keydown' | 'keyup' | 'keypress' } +/** Represents the state of a modifier key */ interface ModifierState { wanted: boolean pressed: boolean }
23-57: Add input validation for shortcut combinations.The
addmethod should validate the shortcut combination format before processing.add: function (shortcut_combination: string, callback: (e: KeyboardEvent) => void, opt?: ShortcutOptions): void { + if (!shortcut_combination || typeof shortcut_combination !== 'string') { + throw new Error('Invalid shortcut combination'); + } + if (typeof callback !== 'function') { + throw new Error('Callback must be a function'); + } const options = this.getOptions(opt); const ele = this.getTargetElement(options.target); shortcut_combination = shortcut_combination.toLowerCase();
151-163: Move key mappings to constants.The special keys and shift numbers mappings should be moved to constants at the module level for better maintainability.
+const SPECIAL_KEYS: Record<string, number> = { + esc: 27, escape: 27, tab: 9, /* ... */ +}; + +const SHIFT_NUMS: Record<string, string> = { + '`': '~', 1: '!', /* ... */ +}; + checkKeysMatch: function (keys: string[], character: string, e: KeyboardEvent, options: ShortcutOptions): boolean { - const special_keys: Record<string, number> = { - esc: 27, escape: 27, /* ... */ - }; - - const shift_nums: Record<string, string> = { - '`': '~', 1: '!', /* ... */ - }; + const special_keys = SPECIAL_KEYS; + const shift_nums = SHIFT_NUMS;
140-148: Enhance character mapping for special keys.The
getCharacterFromKeyCodemethod only handles comma and period special cases. Consider adding more special character mappings.getCharacterFromKeyCode: function (e: KeyboardEvent): string { const code = e.keyCode || e.which; let character = String.fromCharCode(code).toLowerCase(); if (code === 188) character = ','; if (code === 190) character = '.'; + if (code === 189) character = '-'; + if (code === 187) character = '='; + if (code === 191) character = '/'; + if (code === 220) character = '\\'; + if (code === 222) character = "'"; return character; }
200-206: Document browser compatibility requirements.The code includes fallbacks for legacy event handling. Consider adding documentation about supported browsers and environments.
+/** + * Registers an event listener with fallbacks for legacy browsers. + * Supports: + * - Modern browsers: addEventListener + * - IE8 and earlier: attachEvent + * - Legacy browsers: direct event handler assignment + */ if (ele.addEventListener) { ele.addEventListener(event, func, false); } else if ((ele as any).attachEvent) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/simulator/src/hotkey_binder/model/shortcuts.plugin.ts(1 hunks)
🔇 Additional comments (1)
src/simulator/src/hotkey_binder/model/shortcuts.plugin.ts (1)
104-111: LGTM! Null check for parentNode implemented.The null check for element.parentNode has been properly implemented as suggested in the previous review.
|
@JoshVarga I have fixed the cognitive complexity of the code and rest of codeclimate. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/simulator/src/hotkey_binder/model/shortcuts.plugin.ts (4)
1-7: Add JSDoc documentation for better type clarity.The
ShortcutOptionsinterface would benefit from documentation explaining each option's purpose and possible values.Add documentation like this:
+/** + * Configuration options for keyboard shortcuts + */ interface ShortcutOptions { + /** Event type to listen for (default: 'keydown') */ type?: string; + /** Whether to propagate the event (default: false) */ propagate?: boolean; + /** Whether to disable shortcuts in input fields (default: true) */ disable_in_input?: boolean; + /** Target element to attach the shortcut to (default: document) */ target?: Document | string; + /** Specific keycode to match (default: false) */ keycode?: number | false; }
77-89: Improve event type safety and error handling.The event type casting could be more robust with additional checks.
Consider this improvement:
createEventListener: function (shortcut_combination: string, callback: (e: KeyboardEvent) => void, options: ShortcutOptions): EventListener { return (evt: Event): void => { + if (!(evt instanceof KeyboardEvent)) { + console.warn('Expected KeyboardEvent but received:', evt.type); + return; + } - const e = evt as KeyboardEvent; + const e = evt; if (this.shouldIgnoreEvent(e, options)) { return; }
173-180: Extract special keys mapping to a constant.The special keys mapping should be moved outside the function to improve maintainability and reduce memory allocations.
Move the mapping to a constant:
+const SPECIAL_KEYS: Readonly<Record<string, number>> = { + esc: 27, escape: 27, tab: 9, space: 32, return: 13, enter: 13, backspace: 8, + scrolllock: 145, scroll_lock: 145, scroll: 145, capslock: 20, caps_lock: 20, caps: 20, + numlock: 144, num_lock: 144, num: 144, pause: 19, break: 19, insert: 45, home: 36, + delete: 46, end: 35, pageup: 33, page_up: 33, pu: 33, pagedown: 34, page_down: 34, pd: 34, + left: 37, up: 38, right: 39, down: 40, f1: 112, f2: 113, f3: 114, f4: 115, f5: 116, + f6: 117, f7: 118, f8: 119, f9: 120, f10: 121, f11: 122, f12: 123 +} as const; + isSpecialKeyMatch: function (key: string, e: KeyboardEvent): boolean { - const special_keys: Record<string, number> = { - esc: 27, escape: 27, tab: 9, space: 32, return: 13, enter: 13, backspace: 8, - scrolllock: 145, scroll_lock: 145, scroll: 145, capslock: 20, caps_lock: 20, caps: 20, - numlock: 144, num_lock: 144, num: 144, pause: 19, break: 19, insert: 45, home: 36, - delete: 46, end: 35, pageup: 33, page_up: 33, pu: 33, pagedown: 34, page_down: 34, pd: 34, - left: 37, up: 38, right: 39, down: 40, f1: 112, f2: 113, f3: 114, f4: 115, f5: 116, - f6: 117, f7: 118, f8: 119, f9: 120, f10: 121, f11: 122, f12: 123 - }; - return special_keys[key] === (e.keyCode || e.which); + return SPECIAL_KEYS[key] === (e.keyCode || e.which);
190-193: Extract shift number mappings to a constant.Similar to special keys, the shift number mappings should be moved outside the function.
Move the mapping to a constant:
+const SHIFT_NUMS: Readonly<Record<string, string>> = { + '`': '~', '1': '!', '2': '@', '3': '#', '4': '$', '5': '%', '6': '^', '7': '&', + '8': '*', '9': '(', '0': ')', '-': '_', '=': '+', ';': ':', "'": '"', + ',': '<', '.': '>', '/': '?', '\\': '|' +} as const; + isCharacterMatch: function (key: string, character: string, e: KeyboardEvent): boolean { - const shift_nums: Record<string, string> = { - '`': '~', 1: '!', 2: '@', 3: '#', 4: '$', 5: '%', 6: '^', 7: '&', 8: '*', - 9: '(', 0: ')', '-': '_', '=': '+', ';': ':', "'": '"', ',': '<', '.': '>', - '/': '?', '\\': '|' - };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/simulator/src/hotkey_binder/model/shortcuts.plugin.ts(1 hunks)
🔇 Additional comments (1)
src/simulator/src/hotkey_binder/model/shortcuts.plugin.ts (1)
211-225: Add browser compatibility check.The event listener registration uses legacy methods without checking browser support.
Add browser support detection:
Consider modernizing the code:
registerShortcut: function (shortcut_combination: string, func: EventListener, ele: Document | HTMLElement, event: string): void { + const isModernBrowser = 'addEventListener' in window; + if (!isModernBrowser) { + console.warn('Browser uses legacy event handling methods'); + } + this.all_shortcuts[shortcut_combination] = { callback: func, target: ele, event: event }; - if (ele.addEventListener) { + if (isModernBrowser) { ele.addEventListener(event, func, false); - } else if ((ele as any).attachEvent) { - (ele as any).attachEvent('on' + event, func); } else { - (ele as any)['on' + event] = func; + const target = ele as any; + if (target.attachEvent) { + target.attachEvent('on' + event, func); + } else { + target['on' + event] = func; + } } }
|
Merge this PR before #423 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
src/simulator/src/hotkey_binder/model/modelInterfaces.ts (2)
4-10: Add JSDoc documentation for ShortcutOptions interface.Consider adding JSDoc comments to document the purpose and usage of each option:
+/** + * Configuration options for keyboard shortcuts + */ export interface ShortcutOptions { + /** Event type to listen for (e.g., 'keydown', 'keyup') */ type?: string + /** Whether to allow event propagation */ propagate?: boolean + /** Whether to disable shortcuts when focus is in input elements */ disable_in_input?: boolean + /** Target element for the shortcut. Can be Document or element ID */ target?: Document | string + /** Specific keycode to match, or false to match any */ keycode?: number | false }
23-26: Enhance type safety for Storage interface.Consider adding constraints to ensure type consistency between set and get operations:
-export interface Storage { - set<T>(key: string, obj: T): void; - get<T>(key: string): T | null; -} +export interface Storage<T = unknown> { + set(key: string, obj: T): void; + get(key: string): T | null; +}src/simulator/src/hotkey_binder/model/shortcuts.plugin.ts (4)
1-3: Consolidate imports from modelInterfaces.Combine the three imports from the same file into a single import statement:
-import { ShortcutOptions } from './modelInterfaces' -import { ShortcutBinding } from './modelInterfaces' -import { ModifierState } from './modelInterfaces'; +import { ShortcutOptions, ShortcutBinding, ModifierState } from './modelInterfaces';
10-15: Fix indentation in validation checks.The indentation is inconsistent in the validation checks:
- if (!shortcut_combination || typeof shortcut_combination !== 'string') { - throw new Error('Shortcut combination must be a non-empty string'); - } - if (!callback || typeof callback !== 'function') { - throw new Error('Callback must be a function'); - } + if (!shortcut_combination || typeof shortcut_combination !== 'string') { + throw new Error('Shortcut combination must be a non-empty string'); + } + if (!callback || typeof callback !== 'function') { + throw new Error('Callback must be a function'); + }
166-173: Consider using an enum for special keys.The special keys mapping would be better represented as an enum for better type safety and maintainability:
+enum SpecialKeys { + ESC = 27, + ESCAPE = 27, + TAB = 9, + // ... other keys +} + - const special_keys: Record<string, number> = { - esc: 27, escape: 27, tab: 9, space: 32, return: 13, enter: 13, backspace: 8, - // ... other keys - }; + const special_keys: Record<string, SpecialKeys> = { + esc: SpecialKeys.ESC, + escape: SpecialKeys.ESCAPE, + tab: SpecialKeys.TAB, + // ... other keys + };
211-217: Add deprecation warning for legacy event handling.The code supports legacy event handling methods. Consider adding warnings:
if (ele.addEventListener) { ele.addEventListener(event, func, false); } else if ((ele as any).attachEvent) { + console.warn('Using deprecated attachEvent method. Consider upgrading the browser.'); (ele as any).attachEvent('on' + event, func); } else { + console.warn('Using deprecated event handler assignment. Consider upgrading the browser.'); (ele as any)['on' + event] = func; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/simulator/src/hotkey_binder/model/modelInterfaces.ts(1 hunks)src/simulator/src/hotkey_binder/model/shortcuts.plugin.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
src/simulator/src/hotkey_binder/model/shortcuts.plugin.ts (1)
98-102: Previous null check fix has been implemented correctly.The code now properly handles the case where parentNode is null.
|
#423 to be merged with this since actions.js requires shortcuts.plugin.js which would break if we have shortcuts.plugin.ts |
Fixes #414
@niladrix719 @JoshVarga @Arnabdaz @devartstar
Summary by CodeRabbit
Summary by CodeRabbit
Refactor
New Features
Bug Fixes
Documentation