JS to TS simulator/src/hotkey_binder/model/actions.ts #423
Conversation
WalkthroughThe pull request introduces extensive modifications to the Changes
Assessment against linked issues
Poem
✨ Finishing Touches
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
🧹 Nitpick comments (4)
src/simulator/src/hotkey_binder/model/actions.ts (4)
15-17: Consider adding validation for key format in KeyMap.The
KeyMapinterface allows any string value, which could lead to invalid key combinations being stored. Consider adding a more specific type or validation for the key format.interface KeyMap { [key: string]: `${string}${' + ' | ''}${string}` // Ensures format like "Ctrl + A" or "Meta" }
19-28: Consider splitting SimulationArea interface.The
SimulationAreainterface mixes positional properties with behavioral ones. Consider splitting it into smaller, focused interfaces for better maintainability.interface Position { x: number y: number } interface LabelProperties { labelDirection: string labelDirectionFixed?: boolean } interface SimulationObject extends Position, LabelProperties { newDirection: (direction: string) => void helplink?: string } interface SimulationArea { lastSelected?: SimulationObject }
Line range hint
194-213: Consider using an enum for DirectionType.The direction handling could be more maintainable using an enum with associated values.
enum Direction { UP = 'up', DOWN = 'down', LEFT = 'left', RIGHT = 'right' } interface DirectionOffset { x: number y: number } const DIRECTION_OFFSETS: Record<Direction, DirectionOffset> = { [Direction.UP]: { x: 0, y: -10 }, [Direction.DOWN]: { x: 0, y: 10 }, [Direction.LEFT]: { x: -10, y: 0 }, [Direction.RIGHT]: { x: 10, y: 0 } } export const moveElement = (direct: Direction) => (): void => { if (simulationArea.lastSelected) { const offset = DIRECTION_OFFSETS[direct] simulationArea.lastSelected.x += offset.x simulationArea.lastSelected.y += offset.y updateSystem() } }
221-230: Add type guard for simulationArea.lastSelected.The null checks could be more TypeScript-idiomatic using type guards.
export const openDocumentation = (): void => { const selected = simulationArea.lastSelected const defaultDocs = 'https://docs.circuitverse.org/' window.open( selected?.helplink ?? defaultDocs, '_blank' ) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/simulator/src/hotkey_binder/model/actions.ts(2 hunks)
🔇 Additional comments (2)
src/simulator/src/hotkey_binder/model/actions.ts (2)
Line range hint
1-236: Overall good TypeScript conversion with room for improvement.The conversion to TypeScript adds basic type safety and improves code maintainability. However, consider:
- Using more TypeScript features (enums, type guards, utility types)
- Adding proper type declarations for external functions
- Improving error handling for DOM and localStorage operations
- Breaking down complex interfaces into smaller, focused ones
The changes are a good foundation for further TypeScript enhancements.
232-236: Consider adding return types for imported functions.The
updateSystemfunction calls imported functions without clear type information. Consider adding type declarations for these imported functions.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/simulator/src/hotkey_binder/model/actions.ts (1)
56-56:⚠️ Potential issueUse localStorage methods correctly ().
CheckinglocalStorage.userKeysis invalid. Local storage only supports the standardgetItem(),setItem(), andremoveItem()methods. Replace:-if (localStorage.userKeys) localStorage.removeItem('userKeys') +if (localStorage.getItem('userKeys')) localStorage.removeItem('userKeys')Also applies to: 131-131
🧹 Nitpick comments (3)
src/simulator/src/hotkey_binder/model/actions.ts (3)
49-49: Handle potential JSON parsing errors.
Directly usingJSON.parse(...)on localStorage data can throw a runtime error if the stored string is not valid JSON. Consider adding a try/catch block or a guard to prevent the entire function from breaking if the data is malformed.Also applies to: 57-57, 75-75
190-196: Avoid triple function nesting for clarity.
Returning a function that returns another function may complicate readability and type inference. Instead, consider a single function or partial application patterns that are clearer to maintain. For example:-export const elementDirection = (direct: string) => (): void => { +export const elementDirection = (direct: string): (() => void) => { return () => { // ... } }Also applies to: 198-207, 239-247
109-122: Consider unifying repeated DOM traversal logic.
BothgetUserKeysFromUIandwarnOverridetraversedocument.getElementById('preference')children. Extracting this repetitive logic into a helper function helps DRY up the code and improves readability.Also applies to: 153-175
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/simulator/src/hotkey_binder/model/actions.ts(1 hunks)
🔇 Additional comments (2)
src/simulator/src/hotkey_binder/model/actions.ts (2)
48-49: Question the removal of default keys when fetching user keys.
Currently, thegetUserKeysfunction removes thedefaultKeysfrom localStorage every time user keys are fetched. If this is unintended, consider removing theremoveItem('defaultKeys')call or clarifying why default keys must be cleared whenever user keys are requested.
252-259: Include a default or exhaustive check in the switch statement.
Becausedirectis a union type, ensure all cases are handled or use adefault:clause for unforeseen values. Without it, TypeScript might not warn if new directions are added in the future but not handled.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
src/simulator/src/hotkey_binder/model/actions.ts (6)
39-42: Reconsider removing all shortcuts on every key load
Removing all shortcuts (line 39) obliterates any other registered shortcuts, even those unrelated touserKeysordefaultKeys. Confirm if this blanket removal is intentional or if selective cleanup is preferable.
98-103: Watch out for concurrency with localStorage
Multiple browser tabs or windows could overwrite each other’s stored keys. If concurrent usage is expected, consider a more robust synchronization approach (e.g., broadcasting updates or using a storage event listener).
109-120: Ensure DOM structure consistency
The logic here assumes a specific DOM structure:child?.children[1]?.children[...]. If this structure changes, the code will break. Consider more defensive or flexible lookups (e.g. querySelector).
153-168: Clarify override flow
The code warns about conflicts but leaves the actual override handling to user intervention. Consider implementing a follow-up step to confirm/deny override, preventing user confusion.
218-224: Simplify nested arrow functions
Usingexport const elementDirection = (direct: string) => (): void => { ... }can be less readable. A single-level function returningvoidmight suffice unless you need a factory of factories.-export const elementDirection = (direct: string) => (): void => { +export const elementDirection = (direct: string): void => {
306-308: Add fallback or checks for blocked popups
When opening documentation in a new tab/window, user browser settings or popup blockers may prevent it. Consider providing a fallback or a user message if the window fails to open.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/simulator/src/hotkey_binder/model/actions.ts(1 hunks)
🔇 Additional comments (4)
src/simulator/src/hotkey_binder/model/actions.ts (4)
48-49: Validate the unconditional removal ofdefaultKeys
Unconditionally removingdefaultKeysmight cause scenarios where a user cannot revert to default keys if they switch prematurely. Consider preserving the default keys for a more flexible workflow.
65-66: Evaluate the implication of stripping spaces
Joining all segments into a single string (line 65) can lead to collisions if multiple shortcuts differ only by spacing. Ensure this is intentional or consider a more robust normalization approach.
298-300: Handle potential missing elements
Callingdocument.getElementById('customShortcut')?.click()assumes the element is present. Confirm that a missing element is not a possible scenario or handle it gracefully.
313-317: Ensure update steps are robust
updateSystemcalls broad updates but lacks error handling. If any step fails (e.g.,scheduleUpdateorupdateCanvasSet), the system state might remain partially updated.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/simulator/src/hotkey_binder/model/actions.ts (1)
224-230: Clarify higher-order function signature.The function returns another function which returns yet another function of type
(): void. This can be confusing to maintain. Consider whether a single function or a simpler function type would be more readable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/simulator/src/hotkey_binder/model/actions.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/simulator/src/hotkey_binder/model/actions.ts
[error] 69-69: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
[error] 77-77: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
🔇 Additional comments (4)
src/simulator/src/hotkey_binder/model/actions.ts (4)
34-37: Verify removal of defaultKeys when retrieving userKeys.Currently,
getUserKeys()removesdefaultKeysfrom localStorage unconditionally. Double-check whether this is intentional. If you need to preserve default keys for future reference or fallback, consider removing this call or making it conditional.Would you like a script to search other parts of the codebase to confirm no features rely on persisting
defaultKeysduring user key retrieval?
43-44: UsegetIteminstead of direct property access on localStorage.This is consistent with a past review comment. Accessing
localStorage.userKeysis non-standard and may fail. Prefer:-if (localStorage.userKeys) localStorage.removeItem('userKeys') +if (localStorage.getItem('userKeys')) localStorage.removeItem('userKeys')
52-52: Clarify the removal of spaces from keys.By invoking
key.split(' ').join(''), all spaces in the key are stripped. Ensure this behavior is deliberate. If your application expects multi-word shortcuts, consider preserving them.Do you want a script to scan the codebase for multispaced key definitions to confirm no unintended collisions will occur?
106-106: UsegetItemrather than direct property access on localStorage.Past comment also applies here. Replace:
-if (localStorage.defaultKeys) localStorage.removeItem('defaultKeys') +if (localStorage.getItem('defaultKeys')) localStorage.removeItem('defaultKeys')
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/simulator/src/hotkey_binder/model/actions.ts (3)
195-225: Consider simplifying iteration with a direct exit loop.Using
Array.from(...)+.reduce(...)to find a single match may be less intuitive. A simple for-of loop or anArray.find(...)approach could convey the intent more clearly:- return Array.from(preferenceChildren).reduce<string | undefined>((acc, child) => { - ... - }, undefined) + for (const child of preferenceChildren) { + const result = getAssigneeFromPreference(child, combo, target) + if (result) return result + } + return undefined
240-246: Clarify or simplify nested arrow function patterns.Both
elementDirectionandlabelDirectionreturn double arrow functions. While this can be useful for higher-order bindings, consider making them single arrow functions returning void for clearer readability if there is no requirement for partial application.Also applies to: 251-257
309-316: Avoid magic numbers in movement logic.Using the hardcoded step of
10for the x/y offsets can cause difficulty with future changes or customization. Consider extracting it into a named constant, e.g.const MOVE_STEP = 10;.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/simulator/src/hotkey_binder/model/actions.ts(1 hunks)
🔇 Additional comments (3)
src/simulator/src/hotkey_binder/model/actions.ts (3)
43-43: UsegetItemover direct property access onlocalStorage.Referencing
localStorage.userKeysorlocalStorage.defaultKeysis non-standard. For consistency, prefer the canonicalgetItem/removeItemmethods:-if (localStorage.userKeys) localStorage.removeItem('userKeys') +if (localStorage.getItem('userKeys')) localStorage.removeItem('userKeys') -if (localStorage.defaultKeys) localStorage.removeItem('defaultKeys') +if (localStorage.getItem('defaultKeys')) localStorage.removeItem('defaultKeys')Also applies to: 122-122
72-103: Sync logic thoroughly addresses key management.Your implementation of
syncKeys,addOrUpdateKeys, andremoveObsoleteKeysis an apt solution for seamlessly handling changes in the default key map. This admirably addresses past feedback on cleaning up outdated user keys.
131-145: Good job with null checks and type guards for DOM elements.Using optional chaining and verifying instance types for
HTMLElementensures robust DOM interaction. This prevents potential runtime errors caused by uninitialized elements.
Fixes #414
Js to TS
@niladrix719 @JoshVarga @Arnabdaz @devartstar
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Refactor
Style