refactor:minimap.js to ts #664
Conversation
✅ Deploy Preview for circuitverse ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughConverts the minimap module to a strongly typed TypeScript implementation: adds a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 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: 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/minimap.ts (1)
29-36: Guard against missing canvas element before use.
document.getElementById('miniMapArea')may be null or non-canvas; subsequentwidth/height/getContextcalls would throw.Apply:
- this.canvas = document.getElementById( - 'miniMapArea' - ) as HTMLCanvasElement + const el = document.getElementById('miniMapArea') + if (!(el instanceof HTMLCanvasElement)) { + // Element missing or not a canvas; skip setup safely + return + } + this.canvas = elAlso guard before sizing and context:
- this.ctx = this.canvas.getContext('2d') + this.ctx = this.canvas.getContext('2d') + if (!this.ctx) returnAlso applies to: 70-84, 85-93
🧹 Nitpick comments (8)
src/simulator/src/minimap.ts (8)
70-80: Handle degenerate bounds (h or w ≤ 0) to avoid division by zero/Infinity.
If all content lies on a line/point,ratioand size math can explode.Apply:
- const h = this.maxY - this.minY - const w = this.maxX - this.minX - const ratio = Math.min(250 / h, 250 / w) + const h = this.maxY - this.minY + const w = this.maxX - this.minX + const safeH = Math.max(h, 1) + const safeW = Math.max(w, 1) + const ratio = Math.min(250 / safeH, 250 / safeW) - if (h > w) { + if (h > w) { this.canvas.height = 250.0 - this.canvas.width = (250.0 * w) / h + this.canvas.width = (250.0 * w) / safeH } else { this.canvas.width = 250.0 - this.canvas.height = (250.0 * h) / w + this.canvas.height = (250.0 * h) / safeW }Also applies to: 73-73
95-103: Begin a new path before the background rect.
Avoids path accumulation side‑effects if code changes later.- this.ctx.fillStyle = '#bbb' - this.ctx.rect(0, 0, this.canvas!.width, this.canvas!.height) + this.ctx.fillStyle = '#bbb' + this.ctx.beginPath() + this.ctx.rect(0, 0, this.canvas!.width, this.canvas!.height)
153-156: Strict equality for string comparison.
Use!==for consistency with typical TS lint rules.- } else if (lst[i] != 'nodes') { + } else if (lst[i] !== 'nodes') {
131-153: Loop readability: prefer for..of and local names.
Reduces indexing noise and repeated lookups; no behavior change.- for (let i = 0; i < lst.length; i++) { - if (lst[i] === 'wires') { - for (let j = 0; j < globalScope[lst[i]].length; j++) { + for (const key of lst) { + if (key === 'wires') { + for (const wire of globalScope[key]) { this.ctx.beginPath() - this.ctx.moveTo( + this.ctx.moveTo( 2.5 + - (globalScope[lst[i]][j].node1.absX() - this.minX) * + (wire.node1.absX() - this.minX) * ratio, 2.5 + - (globalScope[lst[i]][j].node1.absY() - this.minY) * + (wire.node1.absY() - this.minY) * ratio ) this.ctx.lineTo( 2.5 + - (globalScope[lst[i]][j].node2.absX() - this.minX) * + (wire.node2.absX() - this.minX) * ratio, 2.5 + - (globalScope[lst[i]][j].node2.absY() - this.minY) * + (wire.node2.absY() - this.minY) * ratio ) this.ctx.stroke() } - } else if (lst[i] != 'nodes') { + } else if (key !== 'nodes') { ... - for (let j = 0; j < globalScope[lst[i]].length; j++) { - const obj = globalScope[lst[i]][j] + for (const obj of globalScope[key]) {Also applies to: 164-177
155-163: LED type check: consider Set for O(1) membership.
Minor readability/intent improvement.- let ledY = 0 - if ( - lst[i] == 'DigitalLed' || - lst[i] == 'VariableLed' || - lst[i] == 'RGBLed' - ) { - ledY = 20 - } + const ledTypes = new Set(['DigitalLed', 'VariableLed', 'RGBLed']) + const ledY = ledTypes.has(key) ? 20 : 0
211-216: Use Date.now() for clarity and fewer allocations.
No behavior change; micro‑cleanup.- if (lastMiniMapShown && lastMiniMapShown + 2000 >= new Date().getTime()) { + if (lastMiniMapShown && lastMiniMapShown + 2000 >= Date.now()) { setTimeout( removeMiniMap, - lastMiniMapShown + 2000 - new Date().getTime() + lastMiniMapShown + 2000 - Date.now() )
190-197: Function naming needs refactoring for consistency.The function
updatelastMinimapShownhas inconsistent camelCase casing. Rename toupdateLastMinimapShownas the primary export and add a back-compat alias to avoid breaking the existing call sites insrc/simulator/src/canvasApi.js(line 113) andsrc/simulator/src/listeners.js(line 252). Also modernize toDate.now():-export function updatelastMinimapShown(): void { - lastMiniMapShown = new Date().getTime() -} +export function updateLastMinimapShown(): void { + lastMiniMapShown = Date.now() +} +// Back-compat alias +export const updatelastMinimapShown = updateLastMinimapShown
7-9: Remove manual jQuery declaration and rely on @types/jquery globals.With
@types/jquery@^3.5.14installed and tsconfig configured to auto-include all@types/*packages (notypesrestriction, no customtypeRoots), thedeclare const $: JQueryStaticdeclaration in minimap.ts is redundant. TypeScript will automatically provide jQuery globals. Also, the comment claiming "Global variables are already declared in app.types.ts" is inaccurate—that file contains only domain types (Device, Connector, JsConfig) and no jQuery declarations.Delete the manual declaration and comment to align with the @types/jquery configuration and prevent type drift.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/simulator/src/minimap.ts(7 hunks)src/simulator/src/types/minimap.types.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/simulator/src/minimap.ts (4)
src/simulator/src/types/minimap.types.ts (1)
MiniMapAreaType(7-37)src/simulator/src/layoutMode.ts (1)
layoutModeGet(40-42)src/simulator/src/metadata.ts (1)
updateOrder(67-72)src/simulator/src/themer/themer.ts (1)
colors(47-47)
🔇 Additional comments (2)
src/simulator/src/types/minimap.types.ts (1)
7-37: Interface looks solid; matches usage in minimap.ts.
Fields and method signatures align with the implementation; nullable canvas/ctx are appropriate.src/simulator/src/minimap.ts (1)
10-10: No action needed. DOM lib is already configured in tsconfig.Verification confirms that
compilerOptions.libin tsconfig.json already includes'dom', which is required forHTMLCanvasElementandCanvasRenderingContext2Dtypes used at lines 10 and 91 in minimap.ts. The configuration is correct.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/simulator/src/minimap.ts (2)
80-84: Guard against zero/negative extents to avoid divide‑by‑zero and NaN sizes.
ratio = Math.min(250 / h, 250 / w)explodes whenhorwis 0. Add a simple guard.- const ratio = Math.min(250 / h, 250 / w) + if (h <= 0 || w <= 0) return + const ratio = Math.min(250 / h, 250 / w)Please confirm whether
minX..maxXorminY..maxYcan be equal in practice; if so, this guard prevents runtime issues while preserving behavior.
200-217: Avoidas numberon possibly undefined timestamp; add explicit guard.If
lastMiniMapShownis undefined, the math yieldsNaN, andsetTimeoutreceivesNaN(coerces to 0). Make it explicit.-let lastMiniMapShown: number | undefined +let lastMiniMapShown: number | undefined @@ - if ((lastMiniMapShown as number) + 2000 >= new Date().getTime()) { - setTimeout( - removeMiniMap, - (lastMiniMapShown as number) + 2000 - new Date().getTime() - ) + if (!lastMiniMapShown) { + $('#miniMap').fadeOut('fast') + return + } + const now = Date.now() + const target = lastMiniMapShown + 2000 + if (target >= now) { + setTimeout(removeMiniMap, target - now) return }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/simulator/src/minimap.ts(7 hunks)src/simulator/src/types/minimap.types.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/simulator/src/minimap.ts (4)
src/simulator/src/types/minimap.types.ts (1)
MiniMapAreaType(1-17)src/simulator/src/layoutMode.ts (1)
layoutModeGet(40-42)src/simulator/src/metadata.ts (1)
updateOrder(67-72)src/simulator/src/themer/themer.ts (1)
colors(47-47)
🔇 Additional comments (1)
src/simulator/src/minimap.ts (1)
224-224: Review comment is incorrect; no breaking change exists.The code at line 224 already exports
export default miniMapArea, and all importers across the codebase use the compatible default import syntax (import miniMapArea from './minimap'). No files use named import syntax forminiMapArea, so the stated concern about breakingimport { miniMapArea }does not apply. The codebase is in a consistent state.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/simulator/src/minimap.ts (1)
95-100: Add CSS units and guard missing container element.Unitless values (e.g., "500") are ignored by CSS; append "px" and avoid non‑null assertion on missing #miniMap.
- document.getElementById('miniMap')!.style.height = String( - this.canvas.height - ) - document.getElementById('miniMap')!.style.width = String( - this.canvas.width - ) + const miniMapEl = document.getElementById('miniMap') + if (miniMapEl) { + miniMapEl.style.height = `${this.canvas.height}px` + miniMapEl.style.width = `${this.canvas.width}px` + }
🧹 Nitpick comments (4)
src/simulator/src/minimap.ts (4)
81-91: Guard zero/invalid extents to avoid NaN/Infinity canvas sizes.When h or w ≤ 0, ratio or derived dims can become NaN/Infinity.
- const ratio = Math.min(250 / h, 250 / w) - if (h > w) { - this.canvas.height = 250.0 - this.canvas.width = (250.0 * w) / h - } else { - this.canvas.width = 250.0 - this.canvas.height = (250.0 * h) / w - } + const ratio = Math.min(250 / h, 250 / w) + if (!isFinite(ratio) || h <= 0 || w <= 0) { + // fallback to minimal size + this.canvas.height = 5 + this.canvas.width = 5 + } else if (h > w) { + this.canvas.height = 250.0 + this.canvas.width = (250.0 * w) / h + } else { + this.canvas.width = 250.0 + this.canvas.height = (250.0 * h) / w + }
108-116: Avoid non-null assertion on canvas in play(); use ctx.canvas.Slightly safer and clearer, given ctx is already truthy.
- if (!this.ctx) return - - this.ctx.fillStyle = '#bbb' - this.ctx.beginPath() - this.ctx.rect(0, 0, this.canvas!.width, this.canvas!.height) - this.ctx.fill() + const ctx = this.ctx + if (!ctx) return + const canvas = ctx.canvas as HTMLCanvasElement + ctx.fillStyle = '#bbb' + ctx.beginPath() + ctx.rect(0, 0, canvas.width, canvas.height) + ctx.fill()
167-170: Prefer strict equality.Use
!==for predictable comparisons.- } else if (lst[i] != 'nodes') { + } else if (lst[i] !== 'nodes') {
205-222: Removeas numbercasts; handle undefined timestamp cleanly.Small readability and safety tweak; no behavior change.
-export function updatelastMinimapShown(): void { - lastMiniMapShown = new Date().getTime() -} +export function updatelastMinimapShown(): void { + lastMiniMapShown = Date.now() +} @@ - if ((lastMiniMapShown as number) + 2000 >= new Date().getTime()) { - setTimeout( - removeMiniMap, - (lastMiniMapShown as number) + 2000 - new Date().getTime() - ) + const last = lastMiniMapShown + const now = Date.now() + if (last && last + 2000 >= now) { + setTimeout(removeMiniMap, last + 2000 - now) return }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/simulator/src/minimap.ts(7 hunks)src/simulator/src/types/minimap.types.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/simulator/src/minimap.ts (4)
src/simulator/src/types/minimap.types.ts (1)
MiniMapAreaType(1-16)src/simulator/src/layoutMode.ts (1)
layoutModeGet(40-42)src/simulator/src/metadata.ts (1)
updateOrder(67-72)src/simulator/src/themer/themer.ts (1)
colors(47-47)
🔇 Additional comments (3)
src/simulator/src/types/minimap.types.ts (1)
1-16: Type surface looks correct and consistent with usage.Clean interface, correct nullability; no redundant
contextfield. Good.src/simulator/src/minimap.ts (2)
195-201: Nice fix on clear().Using local
ctx/canvaswith guards removes the prior crash risk. LGTM.
7-11: No action required—jQuery typings are properly configured.The verification confirms that
@types/jquery(version 3.5.14) is present inpackage.jsonand installed in the lockfile at version 3.5.30. The declarationdeclare const $: JQueryStaticwill have full TypeScript support.
1 participant
Fixes #661
Describe the changes you have made in this PR -
converting the minimap.js to ts while maintaining the logic
Screenshots of the changes (If any) -
Note: Please check Allow edits from maintainers. if you would like us to assist in the PR.
Summary by CodeRabbit