refactor: TS integration Minimap.js #474
Conversation
WalkthroughThe pull request removes the legacy JavaScript implementation of the minimap in Changes
Sequence Diagram(s)sequenceDiagram
participant S as Simulator
participant M as MiniMapArea
participant C as Canvas
S->>M: setup(simulationArea)
M->>C: Configure dimensions and update DOM
S->>M: play()
M->>C: Render background and simulation elements
M-->>M: resolve() - Draw wires and circuit elements
S->>M: clear() (if in light mode or as needed)
S->>M: updatelastMinimapShown() / removeMiniMap() on user actions
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for circuitverse ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
src/simulator/src/minimap.ts (5)
3-3: Avoid declaring global variables in TypeScript if possible.
DeclaringlightModeglobally can introduce maintainability and debugging challenges. Consider using an explicit import or a shared config approach instead.
9-10: Consider removing or properly importingglobalScope.
Usingdeclare const globalScopemay obscure dependencies. A better practice is to import it from a typed module or pass it as a parameter for improved clarity and testability.
23-69: Consider decomposing thesetupmethod and clarifying the '5px' adjustments.
Collecting the dimension logic and the final canvas adjustments into smaller helper methods may enhance readability and testing. The extra 5px might merit a well-documented constant or a configurable parameter.
71-129: Renameplay()to a more self-descriptive term (e.g.,render()).
The function is primarily responsible for rendering the minimap rather than “playing” an animation or media. A more descriptive name can improve clarity.
131-136: Unify DOM interaction approach.
Here, jQuery is used to update the z-index, but elsewhere vanilla DOM methods are used. Aligning on a single approach can improve consistency and limit dependency issues.src/simulator/src/types/minimap.types.ts (2)
1-4: Consider factoring out aNodeinterface.
Currently, each wire node is an inline object withabsXandabsY. A separate interface (e.g.,Node { absX(): number; absY(): number }) can improve clarity and potential reuse.
15-22: Index signature might undermine type safety.
[key: string]: Wire[] | CircuitElement[] | number | unknown;can mask potential type mismatches. Consider specifying additional properties or using more targeted type definitions to maintain stricter type safety.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/simulator/src/minimap.js(0 hunks)src/simulator/src/minimap.ts(1 hunks)src/simulator/src/types/minimap.types.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/simulator/src/minimap.js
🔇 Additional comments (9)
src/simulator/src/minimap.ts (7)
1-2: Looks good.
No issues spotted with these import statements.
4-7: No immediate concerns.
Import statements align well with the usage seen below.
11-22: Verify element existence before casting.
document.getElementById('miniMapArea')could benull, causing runtime errors when cast toHTMLCanvasElement. Guard against this with a null check or optional chaining.- canvas: document.getElementById('miniMapArea') as HTMLCanvasElement, + const elem = document.getElementById('miniMapArea'); + if (!elem) { + console.warn("miniMapArea element not found!"); + } + canvas: elem as HTMLCanvasElement,
138-140: No issues found.
ThelastMiniMapShownobject is straightforward.
142-145: Logic appears correct.
This function cleanly updates the timestamp for last known usage.
146-156: No critical issues.
removeMiniMapeffectively manages the minimap’s fade-out, respecting interaction checks.
157-157: Export statement is consistent.
No issues found with exportingminiMapArea.src/simulator/src/types/minimap.types.ts (2)
6-13: Looks good.
Well-defined interface for circuit elements.
24-39: Clear and coherent interface.
These definitions for the minimap area align well with its implementation inminimap.ts.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/simulator/src/types/minimap.types.ts (2)
15-22: Consider making the interface more type-safe.The interface could be improved in two ways:
- The index signature is very permissive, allowing any string key with various types. Consider using a discriminated union or specific property names.
- The
rootproperty is typed asunknown. Consider using a more specific type if possible.Example of a more type-safe approach:
export interface GlobalScope { oy: number; ox: number; scale: number; wires: Wire[]; root: RootType; // Replace with actual type circuitElements: Record<string, CircuitElement[]>; // Add other specific properties as needed }
25-49: Add return type annotations to methods.The interface methods should have explicit return type annotations for better type safety and documentation.
Example improvements:
export interface MiniMapArea { // ... existing properties ... setup(): void; initializeCanvas(): void; calculateBounds(): void; calculateBound(simulationBound: number | undefined, defaultBound: number): number; adjustCanvasSize(): void; initializeContext(): void; play(ratio: number): void; resolve(ratio: number): void; drawBackground(ratio: number): void; drawCircuitElements(ratio: number): void; drawWires(ratio: number): void; drawComponents(item: string, ratio: number): void; clear(): void; }src/simulator/src/minimap.ts (3)
52-73: Extract magic numbers into named constants.The method uses magic numbers (250.0, 5) which should be extracted into named constants for better maintainability.
+ const MAX_MINIMAP_DIMENSION = 250.0; + const PADDING = 5; adjustCanvasSize() { const h = this.maxY - this.minY; const w = this.maxX - this.minX; - const ratio = Math.min(250 / h, 250 / w); + const ratio = Math.min(MAX_MINIMAP_DIMENSION / h, MAX_MINIMAP_DIMENSION / w); if (h > w) { - this.canvas.height = 250.0; + this.canvas.height = MAX_MINIMAP_DIMENSION; this.canvas.width = (250.0 * w) / h; } else { - this.canvas.width = 250.0; + this.canvas.width = MAX_MINIMAP_DIMENSION; this.canvas.height = (250.0 * h) / w; } - this.canvas.height += 5; - this.canvas.width += 5; + this.canvas.height += PADDING; + this.canvas.width += PADDING;
156-161: Consider using native DOM API instead of jQuery.The
clearmethod uses jQuery for DOM manipulation. Consider using native DOM API for better performance and reduced dependencies.clear() { if (lightMode) return; - $('#miniMapArea').css('z-index', '-1'); + this.canvas.style.zIndex = '-1'; this.ctx.clearRect(0, 0, this.canvas.width, this.canvas.height); }
171-180: Simplify time comparison and consider using native DOM API.The
removeMiniMapfunction could be improved by:
- Using constants for the timeout duration
- Simplifying the time comparison logic
- Using native DOM API instead of jQuery
+ const MINIMAP_TIMEOUT_MS = 2000; export function removeMiniMap(): void { if (lightMode) return; if (simulationArea.lastSelected === globalScope.root && simulationArea.mouseDown) return; - if (lastMiniMapShown.value + 2000 >= new Date().getTime()) { - setTimeout(removeMiniMap, lastMiniMapShown.value + 2000 - new Date().getTime()); + const timeElapsed = new Date().getTime() - lastMiniMapShown.value; + if (timeElapsed < MINIMAP_TIMEOUT_MS) { + setTimeout(removeMiniMap, MINIMAP_TIMEOUT_MS - timeElapsed); return; } - $('#miniMap').fadeOut('fast'); + const miniMap = document.getElementById('miniMap'); + if (miniMap) { + miniMap.style.opacity = '0'; + miniMap.style.transition = 'opacity 200ms'; + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/simulator/src/minimap.ts(1 hunks)src/simulator/src/types/minimap.types.ts(1 hunks)
🔇 Additional comments (3)
src/simulator/src/types/minimap.types.ts (2)
1-4: LGTM!The
Wireinterface correctly defines the structure for wire nodes with appropriate method signatures.
6-13: LGTM!The
CircuitElementinterface provides a clear and well-structured definition for circuit elements with appropriate property types.src/simulator/src/minimap.ts (1)
1-10: LGTM!The imports and declarations are well-organized and properly typed.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/simulator/src/minimap.ts (4)
3-3: Consider moving global variables into a TypeScript module.Using
declarefor global variables likelightModeandglobalScopeworks but isn't ideal for TypeScript projects. Consider moving these into a proper TypeScript module for better type safety and maintainability.-declare const lightMode: boolean; -declare const globalScope: GlobalScope; +import { lightMode, globalScope } from './globals';Also applies to: 9-9
50-54: Simplify the calculateBound method.The current implementation is hard to read. Consider simplifying it for better maintainability.
- calculateBound(simulationBound: number | undefined, defaultBound: number): number { - return simulationBound !== undefined - ? Math[simulationBound === simulationArea.minHeight || simulationBound === simulationArea.minWidth ? 'min' : 'max'](simulationBound, defaultBound) - : defaultBound; + calculateBound(simulationBound: number | undefined, defaultBound: number): number { + if (simulationBound === undefined) return defaultBound; + + const isMinBound = simulationBound === simulationArea.minHeight || + simulationBound === simulationArea.minWidth; + return isMinBound + ? Math.min(simulationBound, defaultBound) + : Math.max(simulationBound, defaultBound); },
160-164: Replace jQuery with native DOM API.Consider using native DOM API instead of jQuery for better performance and reduced dependencies.
clear() { if (lightMode) return; - $('#miniMapArea').css('z-index', '-1'); + this.canvas.style.zIndex = '-1'; this.ctx.clearRect(0, 0, this.canvas.width, this.canvas.height); },
175-184: Improve timeout handling in removeMiniMap.The timeout logic uses a magic number and could be simplified.
+const MINIMAP_TIMEOUT_MS = 2000; + export function removeMiniMap(): void { if (lightMode) return; if (simulationArea.lastSelected === globalScope.root && simulationArea.mouseDown) return; - if (lastMiniMapShown.value + 2000 >= new Date().getTime()) { - setTimeout(removeMiniMap, lastMiniMapShown.value + 2000 - new Date().getTime()); + + const currentTime = new Date().getTime(); + const timeoutEnd = lastMiniMapShown.value + MINIMAP_TIMEOUT_MS; + + if (currentTime < timeoutEnd) { + setTimeout(removeMiniMap, timeoutEnd - currentTime); return; } - $('#miniMap').fadeOut('fast'); + document.getElementById('miniMap')?.animate( + [{ opacity: 1 }, { opacity: 0 }], + { duration: 200, fill: 'forwards' } + ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/simulator/src/minimap.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Redirect rules - circuitverse
- GitHub Check: Header rules - circuitverse
- GitHub Check: Pages changed - circuitverse
🔇 Additional comments (1)
src/simulator/src/minimap.ts (1)
31-41: LGTM! Null check implemented correctly.The canvas element null check has been implemented as suggested in the previous review.
| drawComponents(item: string, ratio: number) { | ||
| const ledY = ['DigitalLed', 'VariableLed', 'RGBLed'].includes(item) ? 20 : 0; | ||
|
|
||
| for (const obj of globalScope[item] as CircuitElement[]) { | ||
| this.ctx.beginPath(); | ||
| this.ctx.rect( | ||
| 2.5 + (obj.x - obj.leftDimensionX - this.minX) * ratio, | ||
| 2.5 + (obj.y - obj.upDimensionY - this.minY) * ratio, | ||
| (obj.rightDimensionX + obj.leftDimensionX) * ratio, | ||
| (obj.downDimensionY + obj.upDimensionY + ledY) * ratio | ||
| ); | ||
| this.ctx.fill(); | ||
| this.ctx.stroke(); | ||
| } | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety and maintainability in drawComponents.
Several improvements can be made:
- Unsafe type casting with
as CircuitElement[] - Magic number
20for LED height - LED type check could be extracted
+ private readonly LED_TYPES = ['DigitalLed', 'VariableLed', 'RGBLed'];
+ private readonly LED_HEIGHT = 20;
+
drawComponents(item: string, ratio: number) {
- const ledY = ['DigitalLed', 'VariableLed', 'RGBLed'].includes(item) ? 20 : 0;
+ const ledY = this.LED_TYPES.includes(item) ? this.LED_HEIGHT : 0;
- for (const obj of globalScope[item] as CircuitElement[]) {
+ const components = globalScope[item];
+ if (!Array.isArray(components)) {
+ console.warn(`Invalid component array for ${item}`);
+ return;
+ }
+
+ for (const obj of components as CircuitElement[]) {📝 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.
| drawComponents(item: string, ratio: number) { | |
| const ledY = ['DigitalLed', 'VariableLed', 'RGBLed'].includes(item) ? 20 : 0; | |
| for (const obj of globalScope[item] as CircuitElement[]) { | |
| this.ctx.beginPath(); | |
| this.ctx.rect( | |
| 2.5 + (obj.x - obj.leftDimensionX - this.minX) * ratio, | |
| 2.5 + (obj.y - obj.upDimensionY - this.minY) * ratio, | |
| (obj.rightDimensionX + obj.leftDimensionX) * ratio, | |
| (obj.downDimensionY + obj.upDimensionY + ledY) * ratio | |
| ); | |
| this.ctx.fill(); | |
| this.ctx.stroke(); | |
| } | |
| }, | |
| private readonly LED_TYPES = ['DigitalLed', 'VariableLed', 'RGBLed']; | |
| private readonly LED_HEIGHT = 20; | |
| drawComponents(item: string, ratio: number) { | |
| const ledY = this.LED_TYPES.includes(item) ? this.LED_HEIGHT : 0; | |
| const components = globalScope[item]; | |
| if (!Array.isArray(components)) { | |
| console.warn(`Invalid component array for ${item}`); | |
| return; | |
| } | |
| for (const obj of components as CircuitElement[]) { | |
| this.ctx.beginPath(); | |
| this.ctx.rect( | |
| 2.5 + (obj.x - obj.leftDimensionX - this.minX) * ratio, | |
| 2.5 + (obj.y - obj.upDimensionY - this.minY) * ratio, | |
| (obj.rightDimensionX + obj.leftDimensionX) * ratio, | |
| (obj.downDimensionY + obj.upDimensionY + ledY) * ratio | |
| ); | |
| this.ctx.fill(); | |
| this.ctx.stroke(); | |
| } | |
| }, |
| const miniMapArea: MiniMapArea = { | ||
| canvas: document.getElementById('miniMapArea') as HTMLCanvasElement, | ||
| pageY: 0, | ||
| pageX: 0, | ||
| pageHeight: 0, | ||
| pageWidth: 0, | ||
| minY: 0, | ||
| maxY: 0, | ||
| minX: 0, | ||
| maxX: 0, | ||
| ctx: null as unknown as CanvasRenderingContext2D, | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve object initialization safety.
The current initialization has potential issues:
- Direct DOM access during object initialization could fail if the element doesn't exist.
- Type casting
null as unknown as CanvasRenderingContext2Dis unsafe.
const miniMapArea: MiniMapArea = {
- canvas: document.getElementById('miniMapArea') as HTMLCanvasElement,
+ canvas: null as unknown as HTMLCanvasElement,
pageY: 0,
pageX: 0,
pageHeight: 0,
pageWidth: 0,
minY: 0,
maxY: 0,
minX: 0,
maxX: 0,
- ctx: null as unknown as CanvasRenderingContext2D,
+ ctx: null as CanvasRenderingContext2D | null,📝 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 miniMapArea: MiniMapArea = { | |
| canvas: document.getElementById('miniMapArea') as HTMLCanvasElement, | |
| pageY: 0, | |
| pageX: 0, | |
| pageHeight: 0, | |
| pageWidth: 0, | |
| minY: 0, | |
| maxY: 0, | |
| minX: 0, | |
| maxX: 0, | |
| ctx: null as unknown as CanvasRenderingContext2D, | |
| const miniMapArea: MiniMapArea = { | |
| canvas: null as unknown as HTMLCanvasElement, | |
| pageY: 0, | |
| pageX: 0, | |
| pageHeight: 0, | |
| pageWidth: 0, | |
| minY: 0, | |
| maxY: 0, | |
| minX: 0, | |
| maxX: 0, | |
| ctx: null as CanvasRenderingContext2D | null, | |
| // ... any further properties or methods | |
| }; |
Fixes #414
@JoshVarga @Arnabdaz
Summary by CodeRabbit
New Features
Refactor
Bug Fixes