refactor : TS integration : src/simulator/src/hotkey_binder/model/normalize/normalizer.plugin.ts #458
Conversation
WalkthroughThe changes enhance the key binding simulator by introducing explicit TypeScript type annotations throughout the code. The update modernizes variable declarations by replacing Changes
Assessment against linked issues
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (2)
src/simulator/src/hotkey_binder/model/normalize/normalizer.plugin.ts (2)
370-399: Remove commented-out code.The file contains a significant block of commented-out code at the end. This code appears to be unused debugging utilities. Remove it to improve code maintainability.
-// var fields = ['charCode', 'keyCode', 'which', 'type', 'timeStamp', -// 'altKey', 'ctrlKey', 'shiftKey', 'metaKey']; -// var types = ['keydown', 'keypress', 'keyup']; - -// function show_event(type) { ... } -// function show_is_key_down(id, code, ctrl, alt, shift) { ... } -// function update_key_downs() { ... }
370-399: Remove commented-out code.The file contains a significant block of commented-out code that should be removed to improve maintainability. If this code is needed for debugging, consider moving it to a separate debug utility file.
-// var fields = ['charCode', 'keyCode', 'which', 'type', 'timeStamp', -// 'altKey', 'ctrlKey', 'shiftKey', 'metaKey']; -// var types = ['keydown', 'keypress', 'keyup']; - -// function show_event(type) { -// return function(e) { -// var lines = []; -// for(var i = 0; i < fields.length; ++i) { -// lines.push('<b>' + fields[i] + '</b>: ' + e[fields[i]] + '<br />'); -// } -// document.getElementByI(type).innerHTML = lines.join('\n'); -// return false; -// } -// }; - -// function show_is_key_down(id, code, ctrl, alt, shift) { -// document.getElementById(id + '_down').innerHTML = KeyCode.is_down({ -// code: code, -// ctrl: ctrl, -// alt: alt, -// shift: shift -// }); -// }; - -// function update_key_downs() { -// show_is_key_down('x', KeyCode.key('x'), false, false, false); -// show_is_key_down('shift_x', KeyCode.key('x'), false, false, true); -// show_is_key_down('shift_c', KeyCode.key('c'), false, false, true); -// show_is_key_down('ctrl_a', KeyCode.key('a'), true, false, false); -// };
🧹 Nitpick comments (9)
src/simulator/src/hotkey_binder/model/normalize/normalizer.plugin.types.ts (3)
1-14: Add JSDoc comments to document the interface methods.While the interface is well-structured, adding JSDoc comments would improve code maintainability by documenting:
- Purpose and behavior of each method
- Parameter descriptions and constraints
- Return value descriptions
- Example usage where applicable
export interface IKeyCode { + /** Returns a new instance of IKeyCode without any conflicting key bindings */ no_conflict: () => IKeyCode; + /** Generates a function key code from a number between 1 and 12 */ fkey: (num: number) => number; + /** Generates a numeric keypad code from a number between 0 and 9 or special characters */ numkey: (num: number | string) => number; + /** Generates a key code from the ASCII code of the first character of a string */ key: (str: string) => number; + /** Checks if two key objects are equal including their modifiers */ key_equals: (key1: { code: number; ctrl: boolean; alt: boolean; shift: boolean; meta: boolean }, key2: { code: number; ctrl: boolean; alt: boolean; shift: boolean; meta: boolean }) => boolean; + /** Translates a keycode to its normalized value */ translate_key_code: (code: number) => number; + /** Translates a keyDown event to a normalized key event object */ translate_event: (e: KeyboardEvent) => { code: number; shift: boolean; alt: boolean; ctrl: boolean; meta: boolean }; + /** Updates internal state when a key is pressed */ key_down: (e: KeyboardEvent) => void; + /** Updates internal state when a key is released */ key_up: (e: KeyboardEvent) => void; + /** Returns true if the key spec is currently held down */ is_down: (key: { code: number; ctrl: boolean; alt: boolean; shift: boolean; meta: boolean }) => boolean; + /** Returns a string representation of a key event suitable for shortcuts */ hot_key: (key: { code: number; ctrl: boolean; alt: boolean; shift: boolean; meta: boolean }) => string; [key: string]: any; }
1-14: LGTM! Consider these improvements for better type safety.The interface is well-structured, but could benefit from these enhancements:
- Consider making the index signature more specific to known key constants.
- Extract the repeated key object type into a reusable interface.
+interface IKeyObject { + code: number; + ctrl: boolean; + alt: boolean; + shift: boolean; + meta: boolean; +} + export interface IKeyCode { no_conflict: () => IKeyCode; fkey: (num: number) => number; numkey: (num: number | string) => number; key: (str: string) => number; - key_equals: (key1: { code: number; ctrl: boolean; alt: boolean; shift: boolean; meta: boolean }, key2: { code: number; ctrl: boolean; alt: boolean; shift: boolean; meta: boolean }) => boolean; + key_equals: (key1: IKeyObject, key2: IKeyObject) => boolean; translate_key_code: (code: number) => number; - translate_event: (e: KeyboardEvent) => { code: number; shift: boolean; alt: boolean; ctrl: boolean; meta: boolean }; + translate_event: (e: KeyboardEvent) => IKeyObject; key_down: (e: KeyboardEvent) => void; key_up: (e: KeyboardEvent) => void; - is_down: (key: { code: number; ctrl: boolean; alt: boolean; shift: boolean; meta: boolean }) => boolean; - hot_key: (key: { code: number; ctrl: boolean; alt: boolean; shift: boolean; meta: boolean }) => string; - [key: string]: any; + is_down: (key: IKeyObject) => boolean; + hot_key: (key: IKeyObject) => string; + [key: string]: number | Function; }
1-14: Extract repeated types into separate interfaces and add JSDoc documentation.The interface could be improved by:
- Extracting the repeated key type into a separate interface
- Adding JSDoc documentation for better type information
+/** Interface representing a key with its modifiers */ +interface IKey { + code: number; + ctrl: boolean; + alt: boolean; + shift: boolean; + meta: boolean; +} + +/** + * Interface defining keyboard input handling methods + */ export interface IKeyCode { + /** Returns a new instance of KeyCode */ no_conflict: () => IKeyCode; + /** Generates a function key code from a number */ fkey: (num: number) => number; + /** Generates a numeric keypad code */ numkey: (num: number | string) => number; + /** Generates a key code from ASCII code */ key: (str: string) => number; - key_equals: (key1: { code: number; ctrl: boolean; alt: boolean; shift: boolean; meta: boolean }, key2: { code: number; ctrl: boolean; alt: boolean; shift: boolean; meta: boolean }) => boolean; + key_equals: (key1: IKey, key2: IKey) => boolean; translate_key_code: (code: number) => number; - translate_event: (e: KeyboardEvent) => { code: number; shift: boolean; alt: boolean; ctrl: boolean; meta: boolean }; + translate_event: (e: KeyboardEvent) => IKey; key_down: (e: KeyboardEvent) => void; key_up: (e: KeyboardEvent) => void; - is_down: (key: { code: number; ctrl: boolean; alt: boolean; shift: boolean; meta: boolean }) => boolean; - hot_key: (key: { code: number; ctrl: boolean; alt: boolean; shift: boolean; meta: boolean }) => string; + is_down: (key: IKey) => boolean; + hot_key: (key: IKey) => string; [key: string]: any; }src/simulator/src/hotkey_binder/model/normalize/normalizer.plugin.ts (6)
73-96: Improve key mapping maintainability.The key mapping logic is spread across multiple objects and conditionals. Consider consolidating the mappings into a single configuration object.
+const KEY_MAPPINGS = { + default: { + 186: 59, // ;: + 187: 61, // =+ + 188: 44, // ,< + 189: 95, // -_ + 190: 62, // .> + 191: 47, // /? + 192: 126, // `~ + 219: 91, // {[ + 220: 92, // \| + 221: 93, // }] + }, + shifted: { + 58: 59, // : -> ; + 43: 61, // = -> + + // ... rest of shifted_symbols + }, + opera: { + // ... OPERA_KEYMAP entries + } +}; + +function initializeKeyMap(): void { + Object.assign(KEY_MAP, KEY_MAPPINGS.default); + + if (is_opera && is_windows) { + Object.assign(KEY_MAP, KEY_MAPPINGS.opera); + } else if (is_opera || is_konqueror || is_icab) { + Object.assign(KEY_MAP, KEY_MAPPINGS.opera); + Object.assign(KEY_MAP, KEY_MAPPINGS.shifted); + } +}Also applies to: 113-126, 128-146
106-146: Consider using a more modern browser detection approach.The current browser detection relies on user agent strings, which is not recommended for modern web applications.
Consider using feature detection instead:
const is_gecko = typeof document.mozCancelFullScreen !== 'undefined'; const is_ie = typeof (document as any).documentMode !== 'undefined'; const is_opera = typeof (window as any).opera !== 'undefined';
200-212: Consider using readonly properties for immutable state.The current keys state object could benefit from readonly properties to prevent accidental mutations.
const current_keys: { - codes: { [key: number]: number }; - ctrl: boolean; - alt: boolean; - shift: boolean; - meta: boolean; + readonly codes: { [key: number]: number }; + readonly ctrl: boolean; + readonly alt: boolean; + readonly shift: boolean; + readonly meta: boolean; }
350-351: Use type-safe property access.The current approach using
keyof typeof keycould be more specific.-key[modifier as keyof typeof key] +key[modifier as 'ctrl' | 'alt' | 'shift' | 'meta']
366-368: Consider using a type-safe way to add key constants.The current approach of dynamically adding properties might cause type issues.
-for (const code in key_names) { - KeyCode[key_names[code]] = Number(code); +Object.entries(key_names).forEach(([code, name]) => { + (KeyCode as any)[name] = Number(code); +});
73-96: Extract magic numbers into named constants.The shifted symbols mapping contains several magic numbers that could be extracted into named constants for better readability and maintainability.
+const KEY_CODES = { + SEMICOLON: 59, + EQUALS: 61, + COMMA: 44, + MINUS: 45, + PERIOD: 46, + SLASH: 47, + BACKTICK: 192, + BACKSLASH: 92, + QUOTE: 222, + // ... add more constants +} as const; const shifted_symbols: { [key: number]: number } = { - 58: 59, // : -> ; - 43: 61, // = -> + + 58: KEY_CODES.SEMICOLON, // : -> ; + 43: KEY_CODES.EQUALS, // = -> + // ... update remaining entries };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/simulator/src/hotkey_binder/model/normalize/normalizer.plugin.ts(5 hunks)src/simulator/src/hotkey_binder/model/normalize/normalizer.plugin.types.ts(1 hunks)
👮 Files not reviewed due to content moderation or server errors (2)
- src/simulator/src/hotkey_binder/model/normalize/normalizer.plugin.types.ts
- src/simulator/src/hotkey_binder/model/normalize/normalizer.plugin.ts
🔇 Additional comments (4)
src/simulator/src/hotkey_binder/model/normalize/normalizer.plugin.ts (4)
235-363: LGTM! Well-structured implementation of IKeyCode interface.The implementation is thorough and includes proper type annotations for all methods. The code handles various edge cases and provides clear functionality for key code translation and event handling.
71-96: LGTM! Constants are well-typed and organized.The type annotations for constants improve code clarity and maintainability.
230-233: Verify window.KeyCode type definition.The code assumes
window.KeyCodeexists but TypeScript might not recognize it.Add a type declaration:
declare global { interface Window { KeyCode?: IKeyCode; } }✅ Verification successful
🌐 Web query:
What's the recommended way to extend the Window interface in TypeScript?💡 Result:
To properly extend the `Window` interface in TypeScript while maintaining type safety, there are three recommended approaches: ## 1. Global Declaration Merging (`.ts`/`.tsx` files) Use **declaration merging** within a module context: ```typescript declare global { interface Window { customProperty: string; customMethod: () => void; } } // Usage window.customProperty = "TypeScript"; window.customMethod = () => console.log("Method called"); ``` This approach works best when extending types within application code files[1][6]. ## 2. Dedicated Declaration File (`.d.ts`) Create a separate type definition file (e.g., `globals.d.ts`): ```typescript interface Window { gtag: (...args: any[]) => void; dataLayer: Record<string, any>; } ``` No need for `declare global` in `.d.ts` files as they're automatically included in global scope[5][6]. This is considered the cleanest solution for most projects. ## 3. Module-Specific Declaration For localized type extensions within a single module: ```typescript declare const window: typeof globalThis & { temporaryDebugVar: string; }; window.temporaryDebugVar = "debug_value"; ``` This approach avoids modifying global types but requires repetition across files[6]. **Best Practices:** - Use **unique property names** (e.g., app-specific prefixes) to avoid conflicts[1] - Prefer `.d.ts` files for organization and scalability[5][6] - Avoid `any` type assertions (`(window as any)`) to maintain type safety[4] - Consider namespace objects instead of direct window extensions for complex implementations[1] **Example Configuration Pattern:** ```typescript // globals.d.ts interface Window { appConfig: { apiEndpoint: string; analyticsEnabled: boolean; }; } // Implementation window.appConfig = { apiEndpoint: "https://api.example.com", analyticsEnabled: true }; ``` These methods ensure proper TypeScript type checking while enabling safe integration with third-party libraries and global browser APIs[3][6]. For Next.js/Vite projects, place declarations in `globals.d.ts` or similar project-specific type definition files[4][5]. Citations: - 1: https://www.squash.io/tutorial-extending-window-object-in-typescript/ - 2: https://typescriptcenter.com/expanding-the-window-setting-new-properties-on-window-in-typescript-c0f262561ae6?gi=9bdc65d66eea - 3: https://dev.to/emmanuelgautier/extend-window-type-with-typescript-p9d - 4: https://nono.ma/extend-window-interface-typescript-react-vite - 5: https://www.jamestharpe.com/typescript-extend-window/ - 6: https://www.totaltypescript.com/how-to-properly-type-window - 7: https://codemia.io/knowledge-hub/path/how_do_you_explicitly_set_a_new_property_on_window_in_typescript - 8: https://snippets.khromov.se/extending-window-and-document-global-objects-in-typescript/
Action Required: Extend the Window interface to include KeyCode for proper type checking.
The web query confirms that the recommended approach is to utilize global declaration merging or create a dedicated declaration file. For instance, adding the following in a global declaration (e.g., in a .d.ts file) informs TypeScript about the existence and type of KeyCode on the window object:
declare global { interface Window { KeyCode?: IKeyCode; } }This will ensure that TypeScript recognizes window.KeyCode and maintains type safety throughout the codebase.
231-233: Verify window.KeyCode type safety.The code assumes
window.KeyCodeexists but TypeScript might not recognize this property on the Window interface.Add proper type declarations:
+declare global { + interface Window { + KeyCode?: IKeyCode; + } +} let _KeyCode: IKeyCode; if (typeof window.KeyCode !== 'undefined') { _KeyCode = window.KeyCode; }✅ Verification successful
Type Declaration for window.KeyCode Missing – Please Add Global Declaration
Your review comment is valid. The TypeScript compiler does not include a definition for window.KeyCode by default, which means that using it without an appropriate extension to the Window interface can lead to type-safety issues. The suggested change to add a global declaration:
declare global { interface Window { KeyCode?: IKeyCode; } }will correctly inform the TypeScript compiler about the potential existence (or absence) of KeyCode on window, ensuring that your code properly checks for its presence and handles the type correctly.
- File impacted: src/simulator/src/hotkey_binder/model/normalize/normalizer.plugin.ts (Lines 231-233)
- Issue: Missing declaration on the Window interface for KeyCode, leading to type unsafety.
- Resolution: Add the provided global declaration for enhanced type safety.
src/simulator/src/hotkey_binder/model/normalize/normalizer.plugin.ts
Outdated
Show resolved
Hide resolved
Fixes #414
cc @niladrix719 @JoshVarga
Summary by CodeRabbit
New Features
Refactor