changed the embedListeners.js to embedListeners.ts #663
Conversation
✅ Deploy Preview for circuitverse ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
@ThatDeparted2061 pls review |
WalkthroughAdds explicit TypeScript type annotations to functions and event handlers in embedListeners.ts, including parameter types (e: any), return type annotations (void), non-null assertions, and type casts to relax typing while maintaining existing code patterns. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes The changes consist primarily of mechanical type annotation additions and type assertions. However, review should verify that broad Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 1
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/embedListeners.ts (1)
178-196: Fix keyboard zoom detection; current checks are brittle and have typos.
KeyCode/Keycodeare undefined;keyCodeis deprecated. UseKeyboardEvent.key/codeto reliably catch Ctrl/Meta + (+/-) across layouts.Apply this diff:
- window.addEventListener('keydown', (e: any) => { + window.addEventListener('keydown', (e: KeyboardEvent) => { ... - if (e.key == 'Meta' || e.key == 'Control') { + if (e.key === 'Meta' || e.key === 'Control') { simulationArea.controlDown = false; } ... - if (simulationArea.controlDown && (e.keyCode == 187 || e.KeyCode == 171)) { + if (simulationArea.controlDown && (e.key === '+' || e.key === '=' || e.code === 'Equal')) { e.preventDefault(); ZoomIn(); } ... - if (simulationArea.controlDown && (e.keyCode == 189 || e.Keycode == 173)) { + if (simulationArea.controlDown && (e.key === '-' || e.key === '_' || e.code === 'Minus')) { e.preventDefault(); ZoomOut(); }
🧹 Nitpick comments (12)
src/simulator/src/embedListeners.ts (12)
223-228: Handle NaN for prompt input.
Number(input)can beNaN. Guard before callingchangeClockTime.- const input = prompt('Enter Time:'); - if (input !== null) { - simulationArea.changeClockTime?.(Number(input)); - } + const input = prompt('Enter Time:'); + if (input !== null) { + const t = Number(input.trim()); + if (!Number.isNaN(t)) { + simulationArea.changeClockTime?.(t); + } + }
175-176: Prefer standard 'wheel' with non-passive listener; type WheelEvent and use deltaY.Modernize wheel handling and ensure
preventDefaultworks consistently.- document.getElementById('simulationArea')!.addEventListener('mousewheel', MouseScroll); - document.getElementById('simulationArea')!.addEventListener('DOMMouseScroll', MouseScroll); + const simAreaEl = document.getElementById('simulationArea') as HTMLElement | null; + if (simAreaEl) { + simAreaEl.addEventListener('wheel', MouseScroll as EventListener, { passive: false }); + }- function MouseScroll(event: any): void { + function MouseScroll(event: WheelEvent): void { updateCanvasSet(true); event.preventDefault(); - const deltaY = event.wheelDelta ? event.wheelDelta : -event.detail; - const scrolledUp = deltaY < 0; - const scrolledDown = deltaY > 0; + const deltaY = event.deltaY; + const scrolledUp = deltaY < 0; + const scrolledDown = deltaY > 0;Also applies to: 237-245
141-156: Cache #simulationArea element and narrow event types.Avoid repeated DOM queries and use concrete event types.
- document.getElementById('simulationArea')!.addEventListener('mousedown', (e: any) => { + const simAreaEl = document.getElementById('simulationArea') as HTMLElement | null; + if (!simAreaEl) return; + simAreaEl.addEventListener('mousedown', (e: MouseEvent) => { simulationArea.touch = false; embedPanStart(e); }); - document.getElementById('simulationArea')!.addEventListener('mousemove', () => { + simAreaEl.addEventListener('mousemove', () => { simulationArea.touch = false; BlockElementPan(); }); - document.getElementById('simulationArea')!.addEventListener('touchstart', (e: any) => { + simAreaEl.addEventListener('touchstart', (e: TouchEvent) => { simulationArea.touch = true; embedPanStart(e); }); - document.getElementById('simulationArea')!.addEventListener('touchmove', () => { + simAreaEl.addEventListener('touchmove', () => { simulationArea.touch = true; BlockElementPan(); });
36-36: Type event params precisely and avoid unsafe property access on TouchEvent.Use union types and a safe check for
touches.-function embedPanStart(e: any): void { +function embedPanStart(e: MouseEvent | TouchEvent): void {-function embedPanMove(e: any): void { +function embedPanMove(e: MouseEvent | TouchEvent): void { ... - if (!simulationArea.touch || e.touches.length === 1) { + if (!simulationArea.touch || ('touches' in e && e.touches.length === 1)) {Also applies to: 62-66
46-53: Reduce repetitive casts with local aliases; keep logic identical.This improves readability without changing behavior.
- const rect = (simulationArea as any).canvas.getBoundingClientRect(); - (simulationArea as any).mouseDownRawX = (embedCoordinate.x - rect.left) * (DPR as any); - (simulationArea as any).mouseDownRawY = (embedCoordinate.y - rect.top) * (DPR as any); - (simulationArea as any).mouseDownX = Math.round(((( (simulationArea as any).mouseDownRawX - (globalScope as any).ox) / (globalScope as any).scale) / unit) * unit); - (simulationArea as any).mouseDownY = Math.round(((( (simulationArea as any).mouseDownRawY - (globalScope as any).oy) / (globalScope as any).scale) / unit) * unit); - (simulationArea as any).mouseDown = true; - (simulationArea as any).oldx = (globalScope as any).ox; - (simulationArea as any).oldy = (globalScope as any).oy; + const sa = simulationArea as any; + const gs = globalScope as any; + const rect = sa.canvas.getBoundingClientRect(); + sa.mouseDownRawX = (embedCoordinate.x - rect.left) * DPR; + sa.mouseDownRawY = (embedCoordinate.y - rect.top) * DPR; + sa.mouseDownX = Math.round(((sa.mouseDownRawX - gs.ox) / gs.scale) / unit) * unit; + sa.mouseDownY = Math.round(((sa.mouseDownRawY - gs.oy) / gs.scale) / unit) * unit; + sa.mouseDown = true; + sa.oldx = gs.ox; + sa.oldy = gs.oy;- const rect = (simulationArea as any).canvas.getBoundingClientRect(); - (simulationArea as any).mouseRawX = (embedCoordinate.x - rect.left) * (DPR as any); - (simulationArea as any).mouseRawY = (embedCoordinate.y - rect.top) * (DPR as any); - (simulationArea as any).mouseXf = (( (simulationArea as any).mouseRawX - (globalScope as any).ox) / (globalScope as any).scale); - (simulationArea as any).mouseYf = (( (simulationArea as any).mouseRawY - (globalScope as any).oy) / (globalScope as any).scale); - (simulationArea as any).mouseX = Math.round((simulationArea as any).mouseXf / unit) * unit; - (simulationArea as any).mouseY = Math.round((simulationArea as any).mouseYf / unit) * unit; + const sa = simulationArea as any; + const gs = globalScope as any; + const rect = sa.canvas.getBoundingClientRect(); + sa.mouseRawX = (embedCoordinate.x - rect.left) * DPR; + sa.mouseRawY = (embedCoordinate.y - rect.top) * DPR; + sa.mouseXf = (sa.mouseRawX - gs.ox) / gs.scale; + sa.mouseYf = (sa.mouseRawY - gs.oy) / gs.scale; + sa.mouseX = Math.round(sa.mouseXf / unit) * unit; + sa.mouseY = Math.round(sa.mouseYf / unit) * unit;- if ((simulationArea as any).lastSelected == (globalScope as any).root) { + if ((simulationArea as any).lastSelected == (globalScope as any).root) { updateCanvasSet(true); let fn: any; fn = function () { updateSelectionsAndPane(); };Also applies to: 65-72, 73-76
23-26: Stronger global typings (or move to a .d.ts).These globals are numeric in practice; typing them improves safety. Alternatively, declare in
src/types/globals.d.ts.-declare const globalScope: any; -declare const DPR: any; -declare const width: any; -declare const height: any; +declare const globalScope: any; +declare const DPR: number; +declare const width: number; +declare const height: number;Optional ambient declarations:
// src/types/globals.d.ts declare const DPR: number; declare const width: number; declare const height: number; declare const globalScope: any;
28-29: TypeembedCoordinateas a point.Avoid
anywhere trivial.-const unit: number = 10; -let embedCoordinate: any; +const unit = 10; +let embedCoordinate: { x: number; y: number };
166-168: Use arrow functions andwindow.focus()for clearer intent.Removes implicit
thisusage.- window.addEventListener('mousedown', function () { - this.focus(); - }); + window.addEventListener('mousedown', () => { + window.focus(); + }); ... - window.addEventListener('touchstart', function () { - this.focus(); - }); + window.addEventListener('touchstart', () => { + window.focus(); + });Also applies to: 172-174
131-163: Narrow event types on listeners (incrementally).Switch
(e: any)to concrete types to unlock TS help and avoid accidental property access mistakes. For example:(e: KeyboardEvent),(e: MouseEvent),(e: TouchEvent). No behavior change.Also applies to: 178-181
107-125: Defensive access forglobalScope[objectList[i]].If a key is missing,
.lengthwill throw. Optional chaining prevents crashes in odd states.- for (let j = 0; j < globalScope[objectList[i]].length; j++) { + const bucket = globalScope[objectList[i]]; + if (!bucket) continue; + for (let j = 0; j < bucket.length; j++) { - if (globalScope[objectList[i]][j].isHover()) { + if (bucket[j].isHover()) {
130-130: Idempotency guard forstartListeners.If
startListenerscan be called multiple times, add a one-time flag to avoid duplicate listeners.let listenersInitialized = false; export default function startListeners(): void { if (listenersInitialized) return; listenersInitialized = true; // ... }
268-270: Nit:isIeis unused.Safe to remove or gate with a TODO if planned for compatibility checks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/simulator/src/embedListeners.ts(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/simulator/src/embedListeners.ts (3)
src/simulator/src/restrictedElementDiv.js (1)
globalScope(4-4)src/simulator/src/listeners.js (2)
getCoordinate(98-112)fn(221-221)src/simulator/src/engine.js (5)
errorDetectedSet(169-171)updateSimulationSet(99-101)updatePositionSet(83-85)updateCanvasSet(114-116)scheduleUpdate(465-484)
🪛 ast-grep (0.39.6)
src/simulator/src/embedListeners.ts
[warning] 116-116: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: ele!.innerHTML = Subcircuit: ${globalScope.SubCircuit[j].data.name}
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
[warning] 118-118: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: ele!.innerHTML = CircuitElement: ${objectList[i]}
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
[warning] 116-116: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: ele!.innerHTML = Subcircuit: ${globalScope.SubCircuit[j].data.name}
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 118-118: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: ele!.innerHTML = CircuitElement: ${objectList[i]}
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 126-126: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: document.getElementById('elementName')!.innerHTML = ''
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
🔇 Additional comments (3)
src/simulator/src/embedListeners.ts (3)
197-197: Bounds checks rely on globals; confirmwidth/heighttrack canvas size.If these are viewport-sized, mouse outside-canvas suppression may misfire on HiDPI. Consider referencing
simulationArea.canvas.width/heightinstead.Would you like a follow-up patch to use the canvas dimensions?
231-236: LGTM: dblclick handling preserves original behavior.No issues spotted.
141-141: All concerns verified as correct.Both DOM elements (
#simulationAreaand#elementName) exist in the markup and are properly referenced. Listeners are attached to valid elements with appropriate null assertions. ThestartListeners()function is called exactly once in theonMountedhook, with no duplicate calls detected.
| ele!.style.display = 'block'; | ||
| if (objectList[i] === 'SubCircuit') { | ||
| ele.innerHTML = `Subcircuit: ${globalScope.SubCircuit[j].data.name}`; | ||
| ele!.innerHTML = `Subcircuit: ${globalScope.SubCircuit[j].data.name}`; | ||
| } else { | ||
| ele.innerHTML = `CircuitElement: ${objectList[i]}`; | ||
| ele!.innerHTML = `CircuitElement: ${objectList[i]}`; | ||
| } |
There was a problem hiding this comment.
Blocker: Avoid innerHTML to prevent XSS; guard null element.
User-controlled names flow into innerHTML. Switch to textContent and guard ele to avoid NPEs. This preserves behavior while removing the XSS sink. Based on static analysis hints.
Apply this diff:
- const ele = document.getElementById('elementName');
+ const ele = document.getElementById('elementName') as HTMLElement | null;
+ if (!ele) return;
...
- ele!.style.display = 'block';
+ ele.style.display = 'block';
if (objectList[i] === 'SubCircuit') {
- ele!.innerHTML = `Subcircuit: ${globalScope.SubCircuit[j].data.name}`;
+ ele.textContent = `Subcircuit: ${globalScope.SubCircuit[j].data.name}`;
} else {
- ele!.innerHTML = `CircuitElement: ${objectList[i]}`;
+ ele.textContent = `CircuitElement: ${objectList[i]}`;
}
...
- ele!.style.display = 'none';
- document.getElementById('elementName')!.innerHTML = '';
+ ele.style.display = 'none';
+ ele.textContent = '';Also applies to: 126-128, 108-108
🧰 Tools
🪛 ast-grep (0.39.6)
[warning] 116-116: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: ele!.innerHTML = Subcircuit: ${globalScope.SubCircuit[j].data.name}
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
[warning] 118-118: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: ele!.innerHTML = CircuitElement: ${objectList[i]}
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
[warning] 116-116: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: ele!.innerHTML = Subcircuit: ${globalScope.SubCircuit[j].data.name}
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 118-118: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: ele!.innerHTML = CircuitElement: ${objectList[i]}
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
🤖 Prompt for AI Agents
In src/simulator/src/embedListeners.ts around lines 108, 115-120 and 126-128,
the code uses ele!.innerHTML with user-controlled names which creates an XSS
sink and also assumes ele is non-null; change these to safely check that ele is
not null (e.g., if (!ele) return or continue) before modifying it, and replace
innerHTML assignments with textContent (e.g., ele.textContent = ...) so the
displayed names are escaped and no NPE/XSS can occur.
1 participant
Fixes #661
Describe the changes you have made in this PR -
All changes: added missing simulationArea properties (mouseRawX, mouseRawY, mouseDownRawX, mouseDownRawY, mouseXf, mouseYf), declared globalScope, DPR, width, height, typed event parameters/DOM elements, and updated prompt() to check for null and convert input to number with Number(), keeping all original logic intact.
Note: Please check Allow edits from maintainers. if you would like us to assist in the PR.
Summary by CodeRabbit