refactor: convert listeners.js to listeners.ts #883
Conversation
✅ Deploy Preview for circuitverse ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughThis pull request makes type safety improvements and extends mouse tracking capabilities across the simulator. The SimulationArea interface adds raw and floating-point mouse coordinate properties and narrows the copyList type. The listeners.ts file undergoes significant type-related changes, including new type definitions (Coordinate, Direction, PointerEvent), updated function signatures with stricter event typing, addition of type guards and safer DOM access patterns, import path adjustments, and refinements to zoom and clipboard handling logic. The logixFunction.newProject call is also updated to pass a boolean argument. 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/simulator/src/listeners.ts (1)
598-611: DuplicatedeltaYvariable declaration.Line 601 and 603 contain identical declarations of
deltaY. The second one is redundant and should be removed.Proposed fix
function MouseScroll(event: WheelEvent & { wheelDelta?: number; detail?: number }) { updateCanvasSet(true) event.preventDefault() var deltaY = event.wheelDelta ? event.wheelDelta : -event.detail - event.preventDefault() - var deltaY = event.wheelDelta ? event.wheelDelta : -event.detail const direction = deltaY > 0 ? 1 : -1 handleZoom(direction)
🤖 Fix all issues with AI agents
In `@src/simulator/src/listeners.ts`:
- Around line 634-644: The code parses the JSON string returned by
copy(simulationArea.copyList, true) into an object (textToPutOnClipboard) but
then passes it to localStorage.setItem and clipboardData.setData which expect
strings; remove the JSON.parse and keep the string returned by copy(), e.g., set
textToPutOnClipboard = copy(simulationArea.copyList, true), validate it (if
undefined/null return early), then call localStorage.setItem('clipboardData',
textToPutOnClipboard) and clipboard APIs (window.clipboardData.setData /
e.clipboardData.setData) with that string; adjust references in the surrounding
block including updateRestrictedElementsInScope(), the isIe branch, and the
undefined check to reflect the string variable.
- Around line 665-675: The handler uses
JSON.parse(copy(simulationArea.copyList)) which produces an object but clipboard
and localStorage expect a string; remove the JSON.parse and use the raw string
returned by copy(simulationArea.copyList) (or JSON.stringify(...) if copy
returns an object) so that textToPutOnClipboard is a string, then call
localStorage.setItem('clipboardData', textToPutOnClipboard) and
e.clipboardData.setData('text/plain', textToPutOnClipboard) accordingly;
updateRestrictedElementsInScope and references to isIe and
(window).clipboardData remain unchanged.
🧹 Nitpick comments (2)
src/simulator/src/listeners.ts (2)
53-58: CustomPointerEventtype shadows the native DOMPointerEventinterface.The native DOM already has a
PointerEventinterface that extendsUIEvent. Using the same name for a custom union type can cause confusion and potential type conflicts if nativePointerEventis needed elsewhere.Suggested rename to avoid shadowing
type Coordinate = { x: number; y: number }; type Direction = 1 | -1; -type PointerEvent = MouseEvent | TouchEvent; +type InputEvent = MouseEvent | TouchEvent;Then update all function signatures accordingly (e.g.,
onDoubleClickorTap(e: InputEvent)).
43-51: Consider moving global declarations to a TypeScript declaration file.These inline
declarestatements work but could be consolidated with other global declarations. Based on learnings from this repository,globalScopeshould be declared on the window object using TypeScript declaration files (.d.ts) rather than inline declarations, as it's initialized on the window object.
| const textToPutOnClipboard = JSON.parse(copy(simulationArea.copyList, true)) | ||
|
|
||
| // Updated restricted elements | ||
| updateRestrictedElementsInScope() | ||
| localStorage.setItem('clipboardData', textToPutOnClipboard) | ||
| e.preventDefault() | ||
| if (textToPutOnClipboard == undefined) return | ||
| if (isIe) { | ||
| window.clipboardData.setData('Text', textToPutOnClipboard) | ||
| (window as any).clipboardData.setData('Text', textToPutOnClipboard) | ||
| } else { | ||
| e.clipboardData.setData('text/plain', textToPutOnClipboard) | ||
| e?.clipboardData?.setData('text/plain', textToPutOnClipboard) |
There was a problem hiding this comment.
JSON.parse() returns an object, but setData() and localStorage.setItem() expect strings.
The copy() function returns a JSON string. Calling JSON.parse() on it produces an object. This object is then passed to clipboardData.setData('text/plain', ...) and localStorage.setItem(), both of which expect string arguments. This will result in "[object Object]" being stored instead of the actual circuit data.
🐛 Proposed fix: remove JSON.parse
- const textToPutOnClipboard = JSON.parse(copy(simulationArea.copyList, true))
+ const textToPutOnClipboard = copy(simulationArea.copyList, true)
// Updated restricted elements
updateRestrictedElementsInScope()
localStorage.setItem('clipboardData', textToPutOnClipboard)
e.preventDefault()
if (textToPutOnClipboard == undefined) return
if (isIe) {
(window as any).clipboardData.setData('Text', textToPutOnClipboard)
} else {
e?.clipboardData?.setData('text/plain', textToPutOnClipboard)
}📝 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.
| const textToPutOnClipboard = JSON.parse(copy(simulationArea.copyList, true)) | |
| // Updated restricted elements | |
| updateRestrictedElementsInScope() | |
| localStorage.setItem('clipboardData', textToPutOnClipboard) | |
| e.preventDefault() | |
| if (textToPutOnClipboard == undefined) return | |
| if (isIe) { | |
| window.clipboardData.setData('Text', textToPutOnClipboard) | |
| (window as any).clipboardData.setData('Text', textToPutOnClipboard) | |
| } else { | |
| e.clipboardData.setData('text/plain', textToPutOnClipboard) | |
| e?.clipboardData?.setData('text/plain', textToPutOnClipboard) | |
| const textToPutOnClipboard = copy(simulationArea.copyList, true) | |
| // Updated restricted elements | |
| updateRestrictedElementsInScope() | |
| localStorage.setItem('clipboardData', textToPutOnClipboard) | |
| e.preventDefault() | |
| if (textToPutOnClipboard == undefined) return | |
| if (isIe) { | |
| (window as any).clipboardData.setData('Text', textToPutOnClipboard) | |
| } else { | |
| e?.clipboardData?.setData('text/plain', textToPutOnClipboard) | |
| } |
🤖 Prompt for AI Agents
In `@src/simulator/src/listeners.ts` around lines 634 - 644, The code parses the
JSON string returned by copy(simulationArea.copyList, true) into an object
(textToPutOnClipboard) but then passes it to localStorage.setItem and
clipboardData.setData which expect strings; remove the JSON.parse and keep the
string returned by copy(), e.g., set textToPutOnClipboard =
copy(simulationArea.copyList, true), validate it (if undefined/null return
early), then call localStorage.setItem('clipboardData', textToPutOnClipboard)
and clipboard APIs (window.clipboardData.setData / e.clipboardData.setData) with
that string; adjust references in the surrounding block including
updateRestrictedElementsInScope(), the isIe branch, and the undefined check to
reflect the string variable.
| const textToPutOnClipboard = JSON.parse(copy(simulationArea.copyList)) | ||
|
|
||
| // Updated restricted elements | ||
| updateRestrictedElementsInScope() | ||
| localStorage.setItem('clipboardData', textToPutOnClipboard) | ||
| e.preventDefault() | ||
| if (textToPutOnClipboard == undefined) return | ||
| if (isIe) { | ||
| window.clipboardData.setData('Text', textToPutOnClipboard) | ||
| (window as any).clipboardData.setData('Text', textToPutOnClipboard) | ||
| } else { | ||
| e.clipboardData.setData('text/plain', textToPutOnClipboard) | ||
| e?.clipboardData?.setData('text/plain', textToPutOnClipboard) |
There was a problem hiding this comment.
Same JSON.parse() issue in the copy handler.
This has the same problem as the cut handler - JSON.parse() returns an object but string is expected.
🐛 Proposed fix: remove JSON.parse
- const textToPutOnClipboard = JSON.parse(copy(simulationArea.copyList))
+ const textToPutOnClipboard = copy(simulationArea.copyList)📝 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.
| const textToPutOnClipboard = JSON.parse(copy(simulationArea.copyList)) | |
| // Updated restricted elements | |
| updateRestrictedElementsInScope() | |
| localStorage.setItem('clipboardData', textToPutOnClipboard) | |
| e.preventDefault() | |
| if (textToPutOnClipboard == undefined) return | |
| if (isIe) { | |
| window.clipboardData.setData('Text', textToPutOnClipboard) | |
| (window as any).clipboardData.setData('Text', textToPutOnClipboard) | |
| } else { | |
| e.clipboardData.setData('text/plain', textToPutOnClipboard) | |
| e?.clipboardData?.setData('text/plain', textToPutOnClipboard) | |
| const textToPutOnClipboard = copy(simulationArea.copyList) | |
| // Updated restricted elements | |
| updateRestrictedElementsInScope() | |
| localStorage.setItem('clipboardData', textToPutOnClipboard) | |
| e.preventDefault() | |
| if (textToPutOnClipboard == undefined) return | |
| if (isIe) { | |
| (window as any).clipboardData.setData('Text', textToPutOnClipboard) | |
| } else { | |
| e?.clipboardData?.setData('text/plain', textToPutOnClipboard) |
🤖 Prompt for AI Agents
In `@src/simulator/src/listeners.ts` around lines 665 - 675, The handler uses
JSON.parse(copy(simulationArea.copyList)) which produces an object but clipboard
and localStorage expect a string; remove the JSON.parse and use the raw string
returned by copy(simulationArea.copyList) (or JSON.stringify(...) if copy
returns an object) so that textToPutOnClipboard is a string, then call
localStorage.setItem('clipboardData', textToPutOnClipboard) and
e.clipboardData.setData('text/plain', textToPutOnClipboard) accordingly;
updateRestrictedElementsInScope and references to isIe and
(window).clipboardData remain unchanged.
|
Hey @Nihal4777, can you please review the changes. |
1 participant
Part of #661
Describe the changes you have made in this PR -
changed listeners.js to listeners.ts as a part of javascript to typescript conversion
Code Understanding and AI Usage
Did you use AI assistance (ChatGPT, Claude, Copilot, etc.) to write any part of this code?
If you used AI assistance:
Explain your implementation approach:
Checklist before requesting a review
Note: Please check Allow edits from maintainers if you would like us to assist in the PR.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.