refactor: remove jQuery from listeners.js#771
refactor: remove jQuery from listeners.js#771pantha704 wants to merge 6 commits intoCircuitVerse:mainfrom
Conversation
✅ Deploy Preview for circuitverse ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughReworks src/simulator/src/listeners.js to replace jQuery usage with native DOM APIs (querySelector/All, addEventListener, classList, dragend, etc.), rewriting event bindings for keyup, dblclick, resize, pan/pinch/zoom, and subcircuit drag handling. Adds native zoom controls including a zoom slider and exported functions 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧠 Learnings (2)📓 Common learnings📚 Learning: 2026-01-13T21:31:27.969ZApplied to files:
🧬 Code graph analysis (1)v1/src/simulator/src/listeners.js (5)
🔇 Additional comments (8)
✏️ Tip: You can disable this entire section by setting 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/simulator/src/listeners.js (2)
332-383: Add null-guards (or early return) around#simulationAreainstartListeners().
You added some null checks (e.g.,projectNameEl), but other calls assumedocument.getElementById('simulationArea')always exists; if it doesn’t (embed/SSR/test), this throws during listener setup.Proposed fix (pattern)
+ const simulationAreaEl = document.getElementById('simulationArea') + if (!simulationAreaEl) return + document - .getElementById('simulationArea') + .getElementById('simulationArea') .addEventListener('mouseup', (e) => {(Then reuse
simulationAreaElfor the other listeners in this function.)
610-631: DeduplicateMouseScroll()and preferwheelover deprecated scroll events.
deltaYis computed twice andpreventDefault()is called twice; alsomousewheel/DOMMouseScrollare legacy and may behave inconsistently.Proposed fix
function MouseScroll(event) { updateCanvasSet(true) event.preventDefault() - var deltaY = event.wheelDelta ? event.wheelDelta : -event.detail - event.preventDefault() - var deltaY = event.wheelDelta ? event.wheelDelta : -event.detail + const deltaY = event.wheelDelta ? event.wheelDelta : -event.detail const direction = deltaY > 0 ? 1 : -1 handleZoom(direction) @@ document .getElementById('simulationArea') - .addEventListener('mousewheel', MouseScroll) - document - .getElementById('simulationArea') - .addEventListener('DOMMouseScroll', MouseScroll) + .addEventListener('wheel', MouseScroll, { passive: false })
🤖 Fix all issues with AI agents
In @src/simulator/src/listeners.js:
- Around line 320-329: The expression setting simulationArea.mouseX uses wrong
operator precedence: "100 + simulationArea.maxWidth || 0" yields NaN when
maxWidth is undefined; change it to add a safe default before the || (or better,
use nullish coalescing). For example update the assignment inside the touch
branch (near toRefs(simulatorMobileStore), findDimensions and getTap calls) to:
simulationArea.mouseX = 100 + (simulationArea.maxWidth ?? 0) so 100 is preserved
when maxWidth is undefined.
- Around line 715-758: The current drag implementation relies only on the
'dragend' handler and getBoundingClientRect() (in the listener attached to
subcircuitMenu), which doesn't provide the true drop coordinates and also
mutates the original object in globalScope; fix it by: ensure draggable
subcircuit DOM nodes (class 'draggableSubcircuitElement') have draggable="true"
and set dataTransfer in a 'dragstart' handler (e.g. store elementName and
elementId and optional offset), add a 'dragover' handler on the drop container
(use guide_1/sideBarWidth logic) that calls event.preventDefault() to allow
drops, and implement a 'drop' handler that reads event.dataTransfer to identify
the element, computes drop coords from event.clientX/Y minus the sidebar width
(instead of getBoundingClientRect()), then create a new copy of the source
object (do not reuse globalScope[elementName][elementId] directly) — set its x/y
and each node.x/node.y appropriately, push the new object into
tempBuffer.subElements, and remove the dragged DOM node from the menu;
update/remove the existing 'dragend' logic to avoid mutating the original
tempElement.
🧹 Nitpick comments (3)
src/simulator/src/listeners.js (3)
778-786:resizeTabs()is fine, but consider guarding#tabsBarif it can be absent.
querySelectorAll('#tabsBar div')will just no-op if missing, so this is optional.
791-806:handleZoom()conditions are redundant (both branches do the same call).
This isn’t wrong, just extra complexity.Proposed simplification
function handleZoom(direction) { - if (globalScope.scale > 0.5 * DPR) { - changeScale(direction * 0.1 * DPR) - } else if (globalScope.scale < 4 * DPR) { - changeScale(direction * 0.1 * DPR) - } + changeScale(direction * 0.1 * DPR) gridUpdateSet(true) scheduleUpdate() }
871-955: Store and call theunlistenfunction returned bylisten()for proper cleanup.Tauri's
event.listen()(v2.3.0) returns aPromise<UnlistenFn>. These calls ignore that return value, preventing cleanup. Whilesetup()runs once at app initialization (limiting practical impact in a desktop context), not capturing these unlisteners violates the API contract and creates a pattern vulnerability if the setup code is ever called multiple times or if listeners are registered dynamically. Await eachlisten()call, store the returned function, and call it when the listener's context is no longer needed (e.g., on app unload or if setup is refactored to support teardown).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/simulator/src/listeners.js
🔇 Additional comments (1)
src/simulator/src/listeners.js (1)
760-770: Restricted hover listeners look good (simple + guarded).
The per-elementgetElementByIdnull-check keeps this safe across DOM variants.
| if (simulationArea.touch) { | ||
| const { isCopy } = toRefs(simulatorMobileStore) | ||
| // small hack so Current circuit element should not spwan above last circuit element | ||
| if (!isCopy.value) { | ||
| findDimensions(globalScope); | ||
| simulationArea.mouseX = 100 + simulationArea.maxWidth || 0; | ||
| simulationArea.mouseY = simulationArea.minHeight || 0; | ||
| getTap(e); | ||
| findDimensions(globalScope) | ||
| simulationArea.mouseX = 100 + simulationArea.maxWidth || 0 | ||
| simulationArea.mouseY = simulationArea.minHeight || 0 | ||
| getTap(e) | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix operator precedence in simulationArea.mouseX = 100 + simulationArea.maxWidth || 0.
If simulationArea.maxWidth is undefined, 100 + undefined becomes NaN, and the || 0 turns the result into 0 (not 100).
Proposed fix
- simulationArea.mouseX = 100 + simulationArea.maxWidth || 0
+ simulationArea.mouseX = 100 + (simulationArea.maxWidth || 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.
| if (simulationArea.touch) { | |
| const { isCopy } = toRefs(simulatorMobileStore) | |
| // small hack so Current circuit element should not spwan above last circuit element | |
| if (!isCopy.value) { | |
| findDimensions(globalScope); | |
| simulationArea.mouseX = 100 + simulationArea.maxWidth || 0; | |
| simulationArea.mouseY = simulationArea.minHeight || 0; | |
| getTap(e); | |
| findDimensions(globalScope) | |
| simulationArea.mouseX = 100 + simulationArea.maxWidth || 0 | |
| simulationArea.mouseY = simulationArea.minHeight || 0 | |
| getTap(e) | |
| } | |
| } | |
| if (simulationArea.touch) { | |
| const { isCopy } = toRefs(simulatorMobileStore) | |
| // small hack so Current circuit element should not spwan above last circuit element | |
| if (!isCopy.value) { | |
| findDimensions(globalScope) | |
| simulationArea.mouseX = 100 + (simulationArea.maxWidth || 0) | |
| simulationArea.mouseY = simulationArea.minHeight || 0 | |
| getTap(e) | |
| } | |
| } |
🤖 Prompt for AI Agents
In @src/simulator/src/listeners.js around lines 320 - 329, The expression
setting simulationArea.mouseX uses wrong operator precedence: "100 +
simulationArea.maxWidth || 0" yields NaN when maxWidth is undefined; change it
to add a safe default before the || (or better, use nullish coalescing). For
example update the assignment inside the touch branch (near
toRefs(simulatorMobileStore), findDimensions and getTap calls) to:
simulationArea.mouseX = 100 + (simulationArea.maxWidth ?? 0) so 100 is preserved
when maxWidth is undefined.
| // 'drag and drop' event listener for subcircuit elements in layout mode | ||
| $('#subcircuitMenu').on('dragstop', '.draggableSubcircuitElement', function (event, ui) { | ||
| const sideBarWidth = $('#guide_1')[0].clientWidth; | ||
| let tempElement; | ||
|
|
||
| if (ui.position.top > 10 && ui.position.left > sideBarWidth) { | ||
| // Make a shallow copy of the element with the new coordinates | ||
| tempElement = globalScope[this.dataset.elementName][this.dataset.elementId]; | ||
| // Changing the coordinate doesn't work yet, nodes get far from element | ||
| tempElement.x = ui.position.left - sideBarWidth; | ||
| tempElement.y = ui.position.top; | ||
| for (const node of tempElement.nodeList) { | ||
| node.x = ui.position.left - sideBarWidth; | ||
| node.y = ui.position.top; | ||
| // Using native HTML5 drag events instead of jQuery UI | ||
| const subcircuitMenu = document.getElementById('subcircuitMenu') | ||
| if (subcircuitMenu) { | ||
| // Use event delegation for dragend events on draggable subcircuit elements | ||
| subcircuitMenu.addEventListener('dragend', function (event) { | ||
| const target = event.target | ||
| if ( | ||
| !(target instanceof HTMLElement) || | ||
| !target.classList.contains('draggableSubcircuitElement') | ||
| ) { | ||
| return | ||
| } | ||
|
|
||
| tempBuffer.subElements.push(tempElement); | ||
| this.parentElement.removeChild(this); | ||
| } | ||
| }); | ||
| const guide1 = document.getElementById('guide_1') | ||
| const sideBarWidth = guide1 ? guide1.clientWidth : 0 | ||
|
|
||
| // Get the final position from the drag event | ||
| const rect = target.getBoundingClientRect() | ||
| const posTop = rect.top | ||
| const posLeft = rect.left | ||
|
|
||
| if (posTop > 10 && posLeft > sideBarWidth) { | ||
| // Make a shallow copy of the element with the new coordinates | ||
| const elementName = target.dataset.elementName | ||
| const elementId = target.dataset.elementId | ||
| if (elementName && elementId) { | ||
| const tempElement = globalScope[elementName][elementId] | ||
| if (tempElement) { | ||
| // Changing the coordinate doesn't work yet, nodes get far from element | ||
| tempElement.x = posLeft - sideBarWidth | ||
| tempElement.y = posTop | ||
| for (const node of tempElement.nodeList) { | ||
| node.x = posLeft - sideBarWidth | ||
| node.y = posTop | ||
| } | ||
|
|
||
| restrictedElements.forEach((element) => { | ||
| $(`#${element}`).mouseover(() => { | ||
| showRestricted() | ||
| tempBuffer.subElements.push(tempElement) | ||
| target.parentElement?.removeChild(target) | ||
| } | ||
| } | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify how subcircuit elements are marked draggable, and whether any drag/dragover/drop handlers exist
rg -n --hidden -S "draggableSubcircuitElement|subcircuitMenu|addEventListener\\('(drag|drop|dragover|dragend)'" -C 3
# Find how tempBuffer.subElements is later consumed (does it expect new instances?)
rg -n --hidden -S "tempBuffer\\.subElements|subElements\\.push" -C 3Repository: CircuitVerse/cv-frontend-vue
Length of output: 6055
🏁 Script executed:
#!/bin/bash
# Search for draggable attribute and any drag/drop handler setup
rg -n --hidden -S "draggable|dragstart|dragover|drop" -C 2
# Also check for any element cloning/copying logic
rg -n --hidden -S "cloneNode|Object\\.assign|spread.*tempElement|\\.slice\\(\\)|JSON\\.parse" -C 2Repository: CircuitVerse/cv-frontend-vue
Length of output: 50385
🏁 Script executed:
#!/bin/bash
# Search for draggable attribute in HTML or element setup
rg -n "draggable\s*=" src/simulator/src/listeners.js -C 2
# Check for drop or dragover handlers in current listeners.js
rg -n "dragover|drop" src/simulator/src/listeners.js -C 2
# Check if cloneNode or any deep copy is used
rg -n "cloneNode|Object\.assign|JSON\.stringify" src/simulator/src/listeners.jsRepository: CircuitVerse/cv-frontend-vue
Length of output: 462
🏁 Script executed:
#!/bin/bash
# Search for where elements are marked as draggable (could be in HTML or CSS)
rg -n "draggable" src/simulator/src/ --type html -C 2
# Check if draggable attribute is set programmatically anywhere
rg -n "setAttribute.*draggable|draggable.*true" src/simulator/src/listeners.js
# Search entire listeners.js for complete drag-drop flow
rg -n "subcircuitMenu|draggableSubcircuitElement" src/simulator/src/listeners.js -C 4Repository: CircuitVerse/cv-frontend-vue
Length of output: 764
HTML5 drag-drop implementation is incomplete and will not function correctly.
The code lacks critical handlers for standard HTML5 drag-drop:
- No
dragoverevent handler to define drop zones - No
dropevent handler to capture the actual drop coordinates - No visible
draggable="true"attribute setup on the elements
Without these handlers, dragend + getBoundingClientRect() cannot determine where the user intended to drop. getBoundingClientRect() returns the element's current DOM position (unchanged by dragging), not the user's target location. The previous jQuery UI code relied on ui.position, which provided this information; the native HTML5 replacement must implement dragover + drop handlers instead.
Additionally, tempElement is taken by reference from globalScope[elementName][elementId], mutated directly, and pushed into tempBuffer.subElements. This aliases the original object rather than creating a new instance, causing unintended side effects.
🤖 Prompt for AI Agents
In @src/simulator/src/listeners.js around lines 715 - 758, The current drag
implementation relies only on the 'dragend' handler and getBoundingClientRect()
(in the listener attached to subcircuitMenu), which doesn't provide the true
drop coordinates and also mutates the original object in globalScope; fix it by:
ensure draggable subcircuit DOM nodes (class 'draggableSubcircuitElement') have
draggable="true" and set dataTransfer in a 'dragstart' handler (e.g. store
elementName and elementId and optional offset), add a 'dragover' handler on the
drop container (use guide_1/sideBarWidth logic) that calls
event.preventDefault() to allow drops, and implement a 'drop' handler that reads
event.dataTransfer to identify the element, computes drop coords from
event.clientX/Y minus the sidebar width (instead of getBoundingClientRect()),
then create a new copy of the source object (do not reuse
globalScope[elementName][elementId] directly) — set its x/y and each
node.x/node.y appropriately, push the new object into tempBuffer.subElements,
and remove the dragged DOM node from the menu; update/remove the existing
'dragend' logic to avoid mutating the original tempElement.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
pantha704
force-pushed
the
jquery-to-vue-listeners#433
branch
from
b74b0df to
ea5edc6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
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.js (1)
609-622: Remove duplicate code in MouseScroll function.Lines 612-614 contain duplicate code:
deltaYis declared twice andevent.preventDefault()is called twice. This appears to be a copy-paste oversight.🐛 Proposed fix
function MouseScroll(event) { 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) updateCanvasSet(true) gridUpdateSet(true) if (layoutModeGet()) layoutUpdate() else update() // Schedule update not working, this is INEFFICIENT }
🧹 Nitpick comments (2)
src/simulator/src/listeners.js (2)
89-91: Consider removing the empty else block.The empty else block with only a comment adds no value and can be removed for cleaner code.
♻️ Suggested cleanup
if (tapLength < 500 && tapLength > 0) { onDoubleClickorTap(e) - } else { - // Single tap }
806-866: Add null checks for DOM element access.The
zoomSliderListenersfunction accesses multiple DOM elements without null checks, which could throw errors if elements don't exist (e.g., in embed mode). This is inconsistent with the defensive pattern used forprojectNameEl(lines 337-344).♻️ Suggested defensive coding
function zoomSliderListeners() { - document.getElementById('customRange1').value = 5 + const customRange1 = document.getElementById('customRange1') + if (!customRange1) return + + customRange1.value = 5 document .getElementById('simulationArea') .addEventListener('DOMMouseScroll', zoomSliderScroll) document .getElementById('simulationArea') .addEventListener('mousewheel', zoomSliderScroll) let curLevel = document.getElementById('customRange1').value - const customRange1 = document.getElementById('customRange1') customRange1.addEventListener('input', function (e) { // ... existing code }) // ... rest of function - document.getElementById('decrement').addEventListener('click', () => { - sliderZoomButton(-1) - }) - document.getElementById('increment').addEventListener('click', () => { - sliderZoomButton(1) - }) + const decrementBtn = document.getElementById('decrement') + const incrementBtn = document.getElementById('increment') + decrementBtn?.addEventListener('click', () => { + sliderZoomButton(-1) + }) + incrementBtn?.addEventListener('click', () => { + sliderZoomButton(1) + }) }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/simulator/src/listeners.js
🔇 Additional comments (5)
src/simulator/src/listeners.js (5)
10-10: LGTM!The consolidated import statements improve readability and maintainability.
Also applies to: 25-31
212-214: LGTM!Good use of native
querySelectorAll().forEach()to replace jQuery's class manipulation for closing open dropdowns.
719-756: Verify dragend position detection works correctly.The native HTML5
dragendevent behaves differently from jQuery UI'sdragstop. In HTML5 drag/drop,getBoundingClientRect()on the target afterdragendmay return the element's original position (since the browser typically snaps the dragged element back), not the drop location.For proper drop position detection, consider using the
dropevent on a drop zone or capturingclientX/clientYfrom thedragendevent directly:const posLeft = event.clientX const posTop = event.clientYPlease verify this interaction works correctly during manual testing.
800-805: LGTM!The new
ZoomIn()andZoomOut()exports provide a clean public API that delegates to the internalhandleZoomfunction.
759-769: LGTM!Good use of
addEventListenerformouseover/mouseoutevents with proper null checks on each element.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
src/simulator/src/listeners.js
Outdated
| .addEventListener('mousewheel', zoomSliderScroll) | ||
| let curLevel = document.getElementById('customRange1').value | ||
| const customRange1 = document.getElementById('customRange1') | ||
| customRange1.addEventListener('input', function (e) { |
Check failure
Code scanning / ESLint
Disallow unused variables
src/simulator/src/listeners.js
Outdated
| gridUpdateSet(true) | ||
| curLevel = newValue | ||
| }) | ||
| customRange1.addEventListener('change', function (e) { |
Check failure
Code scanning / ESLint
Disallow unused variables
|
When making bug fixes, migration or any non feature changes, please make the some change to v1/src in the same PR |
Fixes CircuitVerse#433 - Replaced $(.${outputListNames[i]}) with document.querySelectorAll() - Native DOM API returns NodeList which works the same as jQuery collection
- Changed booleanInputVariables == 0 to booleanInputVariables.length === 0 - This was a pre-existing bug where comparing an array to 0 always returns false - Now properly checks if the array is empty
|
Got it! |
pantha704
force-pushed
the
jquery-to-vue-listeners#433
branch
from
374ca1e to
fd1adbb
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @v1/src/simulator/src/listeners.js:
- Around line 336-344: The click handler for projectNameEl calls
document.getElementById('projname').select() without verifying the element
exists, which can throw; modify the arrow function inside the
projectNameEl.addEventListener to fetch the element into a variable (e.g., const
projInput = document.getElementById('projname')), check if projInput is non-null
before calling projInput.select(), and only call select() inside that
conditional while still setting simulationArea.lastSelected = globalScope.root
and preserving the setTimeout behavior.
- Around line 717-754: The dragend handler for the 'draggableSubcircuitElement'
uses target.getBoundingClientRect() which returns the element's original
position; replace this with the pointer coordinates from the event (use
event.clientX and event.clientY) or move logic to a proper drop handler on the
canvas; compute the element's canvas coordinates by subtracting the sidebar
width (guide_1.clientWidth) and any page scroll (window.scrollX/scrollY) from
event.clientX/event.clientY, then set tempElement.x/y and its node.x/y from
those computed values and push into tempBuffer.subElements (keep using the same
tempElement, tempBuffer, and dataset keys elementName/elementId).
- Around line 789-797: handleZoom currently uses two separate conditions that
both call changeScale, so zooming ignores direction and boundaries; update
handleZoom to check the zoom direction first and only call changeScale when the
new scale would be within bounds — e.g., if direction > 0 (zoom in) only call
changeScale when globalScope.scale < 4 * DPR, and if direction < 0 (zoom out)
only call changeScale when globalScope.scale > 0.5 * DPR; keep the existing
calls to gridUpdateSet(true) and scheduleUpdate() after a successful change (or
always, if desired), and use the existing symbols handleZoom, changeScale,
globalScope.scale, DPR, gridUpdateSet, scheduleUpdate to locate the code.
- Around line 804-864: The zoomSliderListeners function assumes DOM elements
exist (IDs customRange1, simulationArea, decrement, increment) and uses them
directly; add null checks and early returns where these are accessed: in
zoomSliderListeners validate document.getElementById('customRange1') and abort
if missing, guard adding 'DOMMouseScroll'/'mousewheel' listeners on
document.getElementById('simulationArea') only if present, and only attach click
listeners for 'decrement' and 'increment' when those elements exist; also ensure
zoomSliderScroll and sliderZoomButton handle null/invalid slider references
(parseInt only after existence) to avoid runtime exceptions.
- Around line 806-811: The code registers scroll listeners twice on the same
element causing duplicate handling; in particular the MouseScroll handler
(attached earlier) and the zoomSliderScroll handler (attached inside
zoomSliderListeners()) both run for wheel/DOMMouseScroll events. Fix by removing
the duplicate attachment in zoomSliderListeners() — either stop adding
mousewheel/DOMMouseScroll there or replace those calls with a single
consolidated handler that calls both MouseScroll and the slider UI update (or
have MouseScroll invoke the slider update). Ensure only one pair of
addEventListener calls target 'simulationArea' for wheel/DOMMouseScroll and
reference the existing MouseScroll and zoomSliderScroll functions (or a new
unified function) so events are processed once.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/simulator/src/listeners.jsv1/src/simulator/src/listeners.js
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: pantha704
Repo: CircuitVerse/cv-frontend-vue PR: 0
File: :0-0
Timestamp: 2026-01-12T04:23:43.985Z
Learning: Issue #433 requires jQuery removal to be submitted as one file per PR, keeping each pull request focused on a single file's migration from jQuery to Vue/native DOM APIs.
🔇 Additional comments (5)
v1/src/simulator/src/listeners.js (5)
10-31: LGTM!Clean consolidation of imports using native DOM API utilities. The grouped imports from
restrictedElementDivimprove readability.
64-94: LGTM!Clean conversion of double-click/tap detection to native JavaScript. The timeout-based double-tap detection pattern is correctly implemented.
172-215: LGTM!The
panStartfunction is well-migrated. Good use ofquerySelectorAll().forEach()withclassList.remove()at lines 212-214 to replace jQuery's class manipulation.
757-767: LGTM!Good use of event delegation pattern with null checks before attaching
mouseover/mouseoutlisteners.
868-954: LGTM!Desktop app listeners using Tauri's
listenAPI are clean and well-structured. Formatting improvements are consistent.
| document | ||
| .getElementById('simulationArea') | ||
| .addEventListener('DOMMouseScroll', zoomSliderScroll) | ||
| document | ||
| .getElementById('simulationArea') | ||
| .addEventListener('mousewheel', zoomSliderScroll) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
wc -l v1/src/simulator/src/listeners.jsRepository: CircuitVerse/cv-frontend-vue
Length of output: 109
🏁 Script executed:
sed -n '600,630p' v1/src/simulator/src/listeners.jsRepository: CircuitVerse/cv-frontend-vue
Length of output: 971
🏁 Script executed:
sed -n '800,820p' v1/src/simulator/src/listeners.jsRepository: CircuitVerse/cv-frontend-vue
Length of output: 885
🏁 Script executed:
# Search for both handler function definitions
rg -n "function MouseScroll|function zoomSliderScroll" v1/src/simulator/src/listeners.jsRepository: CircuitVerse/cv-frontend-vue
Length of output: 148
🏁 Script executed:
sed -n '830,850p' v1/src/simulator/src/listeners.jsRepository: CircuitVerse/cv-frontend-vue
Length of output: 939
🏁 Script executed:
# Get more context around zoomSliderScroll to understand its full implementation
sed -n '830,860p' v1/src/simulator/src/listeners.jsRepository: CircuitVerse/cv-frontend-vue
Length of output: 1249
🏁 Script executed:
rg -n "zoomSliderListeners" v1/src/simulator/src/listeners.jsRepository: CircuitVerse/cv-frontend-vue
Length of output: 138
🏁 Script executed:
# Check the context around where these listeners are registered - are they in main init or inside functions?
sed -n '595,635p' v1/src/simulator/src/listeners.js | cat -nRepository: CircuitVerse/cv-frontend-vue
Length of output: 1567
🏁 Script executed:
# Check the context of zoomSliderListeners function definition
sed -n '800,825p' v1/src/simulator/src/listeners.js | cat -nRepository: CircuitVerse/cv-frontend-vue
Length of output: 1263
Remove duplicate scroll event listeners to prevent double-processing.
Scroll events (mousewheel and DOMMouseScroll) are registered twice on simulationArea:
- Lines 622–627 attach the
MouseScrollhandler - Lines 806–811 (within
zoomSliderListeners()) attach thezoomSliderScrollhandler
Both handlers execute on the same scroll events, causing redundant processing. The MouseScroll handler updates the zoom state and triggers canvas updates, while zoomSliderScroll updates the slider UI. This duplication can lead to synchronization issues and unnecessary handler invocations. Consider consolidating these into a single handler or clearly separating their responsibilities to avoid conflicts.
🤖 Prompt for AI Agents
In @v1/src/simulator/src/listeners.js around lines 806 - 811, The code registers
scroll listeners twice on the same element causing duplicate handling; in
particular the MouseScroll handler (attached earlier) and the zoomSliderScroll
handler (attached inside zoomSliderListeners()) both run for
wheel/DOMMouseScroll events. Fix by removing the duplicate attachment in
zoomSliderListeners() — either stop adding mousewheel/DOMMouseScroll there or
replace those calls with a single consolidated handler that calls both
MouseScroll and the slider UI update (or have MouseScroll invoke the slider
update). Ensure only one pair of addEventListener calls target 'simulationArea'
for wheel/DOMMouseScroll and reference the existing MouseScroll and
zoomSliderScroll functions (or a new unified function) so events are processed
once.
|
@coderabbitai full review All previously reported issues have been addressed:
Please perform a fresh full review of the current state. |
|
@pantha704, thank you for addressing all the previous feedback! I'll perform a fresh full review of the PR now that you've:
I'll review the complete current state of both 🧠 Learnings used✅ Actions performedFull review triggered. |
v1/src/simulator/src/listeners.js
Outdated
| .addEventListener('mousewheel', zoomSliderScroll) | ||
| let curLevel = document.getElementById('customRange1').value | ||
| const customRange1 = document.getElementById('customRange1') | ||
| customRange1.addEventListener('input', function (e) { |
Check failure
Code scanning / ESLint
Disallow unused variables
v1/src/simulator/src/listeners.js
Outdated
| gridUpdateSet(true) | ||
| curLevel = newValue | ||
| }) | ||
| customRange1.addEventListener('change', function (e) { |
Check failure
Code scanning / ESLint
Disallow unused variables
|
Most of the changes on this PR are formatting changes which are unrelated to jquery, could you revert those changes and focus on jquery ? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 20175 @tachyons is correct - I can see from the diff that a significant portion of the changes are formatting-related rather than jQuery removal. Here are the main types of formatting changes that should be reverted: Formatting changes to revert:
Actual jQuery changes to keep:
@pantha704, to clean this up:
This will make the PR much easier to review and keep it focused on the stated objective of jQuery removal. 🧠 Learnings used |
|
Yeah sure ! |
Replaced jQuery with native DOM methods:
- .removeClass() → document.querySelectorAll()
- .on('keyup') → document.addEventListener()
- .on('click') → getElementById().addEventListener()
- .on('dragstop') → native dragend event delegation
- .mouseover/mouseout → addEventListener('mouseover/mouseout')
- resizeTabs: replaced with native DOM (querySelectorAll, offsetWidth, style)
- zoomSlider: replaced with native getElementById and dispatchEvent
- increment/decrement: replaced .click with addEventListener
No formatting changes made. Applied to both src/ and v1/src/
pantha704
force-pushed
the
jquery-to-vue-listeners#433
branch
from
fd1adbb to
c74924f
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@v1/src/simulator/src/listeners.js`:
- Around line 157-158: The Yf calculation is using the X offset (globalScope.ox)
by mistake; update the Yf formula to subtract the Y offset (globalScope.oy)
instead of globalScope.ox so it mirrors the Xf logic: compute Yf =
Math.round((RawY - globalScope.oy) / globalScope.scale / unit) while keeping
RawY, globalScope.scale and unit unchanged to fix pinch-zoom recentering; locate
and change the line that defines Yf in listeners.js where Xf and Yf are
computed.
- Around line 132-135: The pinch distance calculation incorrectly passes two
arguments to Math.sqrt (so the Y component is ignored); update the calculation
that assigns to distance in listeners.js to compute the Euclidean distance by
summing the squared X and Y differences and passing that single sum to Math.sqrt
(use e.touches[1]/[0] clientX and clientY values), ensuring the expression is
wrapped so Math.sqrt receives one numeric argument.
- Around line 323-327: The assignment to simulationArea.mouseX uses incorrect
operator precedence: "100 + simulationArea.maxWidth || 0" causes 100 + undefined
-> NaN then || 0 returns 0; change it so the default applies only to maxWidth
(e.g., compute 100 + (simulationArea.maxWidth || 0)). Update the expression in
the block guarded by isCopy.value (near findDimensions and getTap) to
parenthesize the maxWidth defaulting; leave mouseY's existing fallback for
minHeight as-is unless you also want the same explicit grouping.
♻️ Duplicate comments (10)
v1/src/simulator/src/listeners.js (4)
337-345: Add null check forprojnameelement.
document.getElementById('projname').select()will throw if the element doesn't exist. This was flagged in a previous review.🐛 Proposed fix
projectNameEl.addEventListener('click', () => { simulationArea.lastSelected = globalScope.root setTimeout(() => { - document.getElementById('projname').select() + const projnameEl = document.getElementById('projname') + if (projnameEl) projnameEl.select() }, 100) })
776-784:handleZoomboundary checks are ineffective — both branches do the same thing.The conditions
scale > 0.5 * DPRandscale < 4 * DPRdon't prevent zooming beyond limits based on direction. This was flagged in a previous review.🐛 Proposed fix to enforce zoom boundaries
function handleZoom(direction) { - if (globalScope.scale > 0.5 * DPR) { - changeScale(direction * 0.1 * DPR) - } else if (globalScope.scale < 4 * DPR) { - changeScale(direction * 0.1 * DPR) - } + const minScale = 0.5 * DPR + const maxScale = 4 * DPR + // Prevent zooming out below min or zooming in above max + if (direction < 0 && globalScope.scale <= minScale) return + if (direction > 0 && globalScope.scale >= maxScale) return + changeScale(direction * 0.1 * DPR) gridUpdateSet(true) scheduleUpdate() }
793-798: Duplicate scroll listeners cause double event processing.
mousewheelandDOMMouseScrollare registered twice onsimulationArea:
- Lines 625-630 attach
MouseScroll- Lines 793-798 attach
zoomSliderScrollBoth execute on every scroll. Consider consolidating into a single handler or having
MouseScrollupdate the slider UI directly.
791-798: Null check needed before accessingcustomRange1at line 792.
document.getElementById('customRange1').value = 5at line 792 will throw if element doesn't exist, but the null check doesn't occur until line 801.🐛 Proposed fix
function zoomSliderListeners() { - document.getElementById('customRange1').value = 5 + const customRange1 = document.getElementById('customRange1') + if (!customRange1) return + customRange1.value = 5 document .getElementById('simulationArea') .addEventListener('DOMMouseScroll', zoomSliderScroll) document .getElementById('simulationArea') .addEventListener('mousewheel', zoomSliderScroll) - let curLevel = document.getElementById('customRange1').value - const customRange1 = document.getElementById('customRange1') - if (customRange1) { + let curLevel = customRange1.valuesrc/simulator/src/listeners.js (6)
337-345: Add null check forprojnameelement.Same issue as v1 counterpart. The
select()call will throw if element doesn't exist.🐛 Proposed fix
projectNameEl.addEventListener('click', () => { simulationArea.lastSelected = globalScope.root setTimeout(() => { - document.getElementById('projname').select() + const projnameEl = document.getElementById('projname') + if (projnameEl) projnameEl.select() }, 100) })
776-784:handleZoomboundary checks ineffective.Same issue as v1 counterpart. Both conditional branches execute identical code, failing to enforce zoom limits.
🐛 Proposed fix
function handleZoom(direction) { - if (globalScope.scale > 0.5 * DPR) { - changeScale(direction * 0.1 * DPR) - } else if (globalScope.scale < 4 * DPR) { - changeScale(direction * 0.1 * DPR) - } + const minScale = 0.5 * DPR + const maxScale = 4 * DPR + if (direction < 0 && globalScope.scale <= minScale) return + if (direction > 0 && globalScope.scale >= maxScale) return + changeScale(direction * 0.1 * DPR) gridUpdateSet(true) scheduleUpdate() }
791-798: Null check needed before accessingcustomRange1at line 792, and duplicate scroll listeners.Same issues as v1 counterpart:
- Line 792 accesses
customRange1before the null check at line 801- Lines 793-798 register scroll handlers that duplicate those at lines 625-630
🐛 Proposed fix for null check
function zoomSliderListeners() { - document.getElementById('customRange1').value = 5 + const customRange1 = document.getElementById('customRange1') + if (!customRange1) return + customRange1.value = 5 document .getElementById('simulationArea') .addEventListener('DOMMouseScroll', zoomSliderScroll) document .getElementById('simulationArea') .addEventListener('mousewheel', zoomSliderScroll) - let curLevel = document.getElementById('customRange1').value - const customRange1 = document.getElementById('customRange1') - if (customRange1) { + let curLevel = customRange1.value
157-158: Bug:YfusesglobalScope.oxinstead ofglobalScope.oy.Same issue as v1 counterpart. Y-coordinate calculation uses wrong offset.
🐛 Proposed fix
const Xf = Math.round((RawX - globalScope.ox) / globalScope.scale / unit) - const Yf = Math.round((RawY - globalScope.ox) / globalScope.scale / unit) + const Yf = Math.round((RawY - globalScope.oy) / globalScope.scale / unit)
132-135: Critical:Math.sqrt()only accepts one argument — Y component is ignored.Same issue as v1 counterpart. The comma creates a second argument which
Math.sqrt()ignores, breaking pinch distance calculation.🐛 Proposed fix
- distance = Math.sqrt( - (e.touches[1].clientX - e.touches[0].clientX) ** 2, - (e.touches[1].clientY - e.touches[0].clientY) ** 2 - ) + distance = Math.sqrt( + (e.touches[1].clientX - e.touches[0].clientX) ** 2 + + (e.touches[1].clientY - e.touches[0].clientY) ** 2 + )
323-327: Operator precedence bug inmouseXcalculation.Same issue as v1 counterpart.
100 + simulationArea.maxWidth || 0yields0whenmaxWidthis undefined.🐛 Proposed fix
- simulationArea.mouseX = 100 + simulationArea.maxWidth || 0 + simulationArea.mouseX = 100 + (simulationArea.maxWidth || 0)
🧹 Nitpick comments (2)
src/simulator/src/combinationalAnalysis.js (1)
1-463: This file appears outside the scope of this PR.This PR's objective is to remove jQuery from
listeners.js. The changes incombinationalAnalysis.jsare primarily formatting/style normalization (import reformatting, multi-line function signatures) with no jQuery removal. Per the learning that issue#433requires one-file-per-PR migrations, consider reverting these changes to keep the PR focused.The static analysis flags (variable redeclarations,
eval()usage at line 445) are pre-existing technical debt unrelated to this PR.v1/src/simulator/src/combinationalAnalysis.js (1)
1-463: Same concern as the non-v1 counterpart — outside PR scope.This file mirrors the formatting changes in
src/simulator/src/combinationalAnalysis.jswith no jQuery removal. Consider reverting to keep the PR focused onlisteners.js. Based on learnings, issue#433requires one-file-per-PR migrations.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/simulator/src/combinationalAnalysis.jssrc/simulator/src/listeners.jsv1/src/simulator/src/combinationalAnalysis.jsv1/src/simulator/src/listeners.js
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: pantha704
Repo: CircuitVerse/cv-frontend-vue PR: 0
File: :0-0
Timestamp: 2026-01-12T04:23:43.985Z
Learning: Issue `#433` requires jQuery removal to be submitted as one file per PR, keeping each pull request focused on a single file's migration from jQuery to Vue/native DOM APIs.
📚 Learning: 2026-01-13T21:31:27.969Z
Learnt from: naman79820
Repo: CircuitVerse/cv-frontend-vue PR: 791
File: src/simulator/src/setup.js:175-187
Timestamp: 2026-01-13T21:31:27.969Z
Learning: In src/simulator/src/setup.js, startMainListeners() is intentionally NOT called directly in setup(). Instead, it's initialized via Vue lifecycle through setupPanelsWhenReady() (called from Extra.vue onMounted), which waits for all panels to be mounted before attaching listeners. This lifecycle-driven approach prevents race conditions on first Tauri launch.
Applied to files:
src/simulator/src/listeners.jsv1/src/simulator/src/listeners.js
🧬 Code graph analysis (3)
src/simulator/src/listeners.js (5)
v1/src/simulator/src/listeners.js (13)
coordinate(45-45)returnCoordinate(46-49)currDistance(51-51)distance(52-52)pinchZ(53-53)centreX(54-54)centreY(55-55)timeout(56-56)lastTap(57-57)pinchZoom(124-165)i(292-292)startListeners(332-760)subcircuitMenu(716-716)src/simulator/src/engine.js (3)
updateCanvasSet(114-116)scheduleUpdate(465-484)update(495-570)src/simulator/src/embedListeners.js (3)
simulationArea(104-104)i(106-106)startListeners(124-255)src/simulator/src/canvasApi.js (1)
findDimensions(10-58)src/simulator/src/data.js (1)
logixFunction(24-24)
v1/src/simulator/src/listeners.js (8)
src/simulator/src/engine.js (1)
updateCanvasSet(114-116)src/simulator/src/restrictedElementDiv.js (3)
globalScope(4-4)showRestricted(35-40)hideRestricted(42-44)src/simulator/src/data/backupCircuit.js (1)
scheduleBackup(72-85)src/store/simulatorMobileStore.ts (1)
useSimulatorMobileStore(6-36)src/simulator/src/minimap.js (2)
updatelastMinimapShown(175-177)removeMiniMap(178-194)src/simulator/src/canvasApi.js (1)
findDimensions(10-58)src/simulator/src/ux.js (2)
exitFullView(405-424)element(484-484)src/simulator/src/data.js (1)
logixFunction(24-24)
v1/src/simulator/src/combinationalAnalysis.js (7)
src/simulator/src/utils.ts (1)
stripTags(38-40)src/simulator/src/quinMcCluskey.ts (1)
BooleanMinimize(10-54)src/simulator/src/canvasApi.js (1)
findDimensions(10-58)src/simulator/src/node.js (1)
Node(150-1034)src/simulator/src/modules/NotGate.js (1)
NotGate(18-96)src/simulator/src/modules/AndGate.js (1)
AndGate(21-136)src/simulator/src/modules/OrGate.ts (1)
OrGate(10-121)
🪛 Biome (2.1.2)
src/simulator/src/combinationalAnalysis.js
[error] 179-179: Shouldn't redeclare 'i'. Consider to delete it or rename it.
'i' is defined here:
(lint/suspicious/noRedeclare)
[error] 231-231: Shouldn't redeclare 'i'. Consider to delete it or rename it.
'i' is defined here:
(lint/suspicious/noRedeclare)
[error] 263-263: Shouldn't redeclare 'k'. Consider to delete it or rename it.
'k' is defined here:
(lint/suspicious/noRedeclare)
[error] 265-265: Shouldn't redeclare 'index'. Consider to delete it or rename it.
'index' is defined here:
(lint/suspicious/noRedeclare)
[error] 272-272: Shouldn't redeclare 'v'. Consider to delete it or rename it.
'v' is defined here:
(lint/suspicious/noRedeclare)
[error] 307-307: Shouldn't redeclare 'j'. Consider to delete it or rename it.
'j' is defined here:
(lint/suspicious/noRedeclare)
[error] 308-308: Shouldn't redeclare 'v'. Consider to delete it or rename it.
'v' is defined here:
(lint/suspicious/noRedeclare)
[error] 315-315: Shouldn't redeclare 'v2'. Consider to delete it or rename it.
'v2' is defined here:
(lint/suspicious/noRedeclare)
[error] 445-445: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
v1/src/simulator/src/combinationalAnalysis.js
[error] 179-179: Shouldn't redeclare 'i'. Consider to delete it or rename it.
'i' is defined here:
(lint/suspicious/noRedeclare)
[error] 231-231: Shouldn't redeclare 'i'. Consider to delete it or rename it.
'i' is defined here:
(lint/suspicious/noRedeclare)
[error] 263-263: Shouldn't redeclare 'k'. Consider to delete it or rename it.
'k' is defined here:
(lint/suspicious/noRedeclare)
[error] 265-265: Shouldn't redeclare 'index'. Consider to delete it or rename it.
'index' is defined here:
(lint/suspicious/noRedeclare)
[error] 272-272: Shouldn't redeclare 'v'. Consider to delete it or rename it.
'v' is defined here:
(lint/suspicious/noRedeclare)
[error] 307-307: Shouldn't redeclare 'j'. Consider to delete it or rename it.
'j' is defined here:
(lint/suspicious/noRedeclare)
[error] 308-308: Shouldn't redeclare 'v'. Consider to delete it or rename it.
'v' is defined here:
(lint/suspicious/noRedeclare)
[error] 315-315: Shouldn't redeclare 'v2'. Consider to delete it or rename it.
'v2' is defined here:
(lint/suspicious/noRedeclare)
[error] 445-445: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
🔇 Additional comments (2)
v1/src/simulator/src/listeners.js (1)
785-790: LGTM —ZoomIn()andZoomOut()exports are clean.The public API additions delegate correctly to
handleZoomwith appropriate direction values.src/simulator/src/listeners.js (1)
785-790: LGTM —ZoomIn()andZoomOut()exports.Clean public API additions for zoom functionality.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| distance = Math.sqrt( | ||
| (e.touches[1].clientX - e.touches[0].clientX) ** 2, | ||
| (e.touches[1].clientY - e.touches[0].clientY) ** 2 | ||
| ) |
There was a problem hiding this comment.
Critical: Math.sqrt() only accepts one argument — Y component is ignored.
The comma creates a second argument which Math.sqrt() ignores, resulting in incorrect pinch distance calculation. This breaks pinch-to-zoom functionality.
🐛 Proposed fix
- distance = Math.sqrt(
- (e.touches[1].clientX - e.touches[0].clientX) ** 2,
- (e.touches[1].clientY - e.touches[0].clientY) ** 2
- )
+ distance = Math.sqrt(
+ (e.touches[1].clientX - e.touches[0].clientX) ** 2 +
+ (e.touches[1].clientY - e.touches[0].clientY) ** 2
+ )📝 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.
| distance = Math.sqrt( | |
| (e.touches[1].clientX - e.touches[0].clientX) ** 2, | |
| (e.touches[1].clientY - e.touches[0].clientY) ** 2 | |
| ) | |
| distance = Math.sqrt( | |
| (e.touches[1].clientX - e.touches[0].clientX) ** 2 + | |
| (e.touches[1].clientY - e.touches[0].clientY) ** 2 | |
| ) |
🤖 Prompt for AI Agents
In `@v1/src/simulator/src/listeners.js` around lines 132 - 135, The pinch distance
calculation incorrectly passes two arguments to Math.sqrt (so the Y component is
ignored); update the calculation that assigns to distance in listeners.js to
compute the Euclidean distance by summing the squared X and Y differences and
passing that single sum to Math.sqrt (use e.touches[1]/[0] clientX and clientY
values), ensuring the expression is wrapped so Math.sqrt receives one numeric
argument.
| const Xf = Math.round((RawX - globalScope.ox) / globalScope.scale / unit) | ||
| const Yf = Math.round((RawY - globalScope.ox) / globalScope.scale / unit) |
There was a problem hiding this comment.
Bug: Yf uses globalScope.ox instead of globalScope.oy.
The Y-coordinate calculation incorrectly uses the X-offset, causing pinch zoom recentering to malfunction.
🐛 Proposed fix
const Xf = Math.round((RawX - globalScope.ox) / globalScope.scale / unit)
- const Yf = Math.round((RawY - globalScope.ox) / globalScope.scale / unit)
+ const Yf = Math.round((RawY - globalScope.oy) / globalScope.scale / unit)🤖 Prompt for AI Agents
In `@v1/src/simulator/src/listeners.js` around lines 157 - 158, The Yf calculation
is using the X offset (globalScope.ox) by mistake; update the Yf formula to
subtract the Y offset (globalScope.oy) instead of globalScope.ox so it mirrors
the Xf logic: compute Yf = Math.round((RawY - globalScope.oy) /
globalScope.scale / unit) while keeping RawY, globalScope.scale and unit
unchanged to fix pinch-zoom recentering; locate and change the line that defines
Yf in listeners.js where Xf and Yf are computed.
| if (!isCopy.value) { | ||
| findDimensions(globalScope); | ||
| simulationArea.mouseX = 100 + simulationArea.maxWidth || 0; | ||
| simulationArea.mouseY = simulationArea.minHeight || 0; | ||
| getTap(e); | ||
| findDimensions(globalScope) | ||
| simulationArea.mouseX = 100 + simulationArea.maxWidth || 0 | ||
| simulationArea.mouseY = simulationArea.minHeight || 0 | ||
| getTap(e) |
There was a problem hiding this comment.
Operator precedence bug: loses base value when maxWidth is undefined.
100 + undefined yields NaN, then NaN || 0 returns 0 instead of the intended 100. The || should apply only to maxWidth.
🐛 Proposed fix
- simulationArea.mouseX = 100 + simulationArea.maxWidth || 0
- simulationArea.mouseY = simulationArea.minHeight || 0
+ simulationArea.mouseX = 100 + (simulationArea.maxWidth || 0)
+ simulationArea.mouseY = simulationArea.minHeight || 0🤖 Prompt for AI Agents
In `@v1/src/simulator/src/listeners.js` around lines 323 - 327, The assignment to
simulationArea.mouseX uses incorrect operator precedence: "100 +
simulationArea.maxWidth || 0" causes 100 + undefined -> NaN then || 0 returns 0;
change it so the default applies only to maxWidth (e.g., compute 100 +
(simulationArea.maxWidth || 0)). Update the expression in the block guarded by
isCopy.value (near findDimensions and getTap) to parenthesize the maxWidth
defaulting; leave mouseY's existing fallback for minHeight as-is unless you also
want the same explicit grouping.
|
I've rebased from main and kept this PR strictly to jQuery changes. No formatting stuff this time. Replaced all the jQuery calls with native DOM methods and made sure v1/src has the same changes. About those bugs CodeRabbit flagged (the Math.sqrt and oy coordinate issues) - those are pre-existing in the codebase, not from my changes. Left them as-is to keep this PR focused, I can fix them in a separate PR if you want. Let me know if anything else needs fixing! |
|
@tachyons should i create a new issue for the formatting bugs by coderabbit, |
2 participants
Fixes #433 (partial - listeners.js file)
Changes
Removed all jQuery from
src/simulator/src/listeners.jsand replaced with native DOM APIs.jQuery → Native DOM Conversions
$('.dropdown.open').removeClass()classList.remove()$('#projectName').on('click')addEventListener('click')$(document).on('keyup')document.addEventListener('keyup')$(document).on('input change')addEventListener('input')+addEventListener('change')$('#subcircuitMenu').on('dragstop')dragendevent$('#el').mouseover/mouseoutaddEventListener('mouseover'/'mouseout')$('#tabsBar div').each().css()querySelectorAll().forEach()+style$('#customRange1').val().change()value+dispatchEvent(new Event())$('#decrement/#increment').click()addEventListener('click')Technical Details
Stats
Testing
bun run build)Code Understanding and AI Usage
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.