refactor: migrate canvasApi.js to TypeScript #694
Conversation
✅ Deploy Preview for circuitverse ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughConverts the canvasApi drawing utilities module from JavaScript to TypeScript, adding type annotations and type safety improvements. Introduces Direction type, function parameter types, return type declarations, and type aliases. Logic and functionality remain unchanged; this is a pure migration with type layer additions. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (5 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: 2
🧹 Nitpick comments (4)
src/simulator/src/canvasApi.ts (4)
36-37: ExportCanvasContextif it’s intended as part of the public API.
Right nowCanvasContextis file-local (type CanvasContext = ...). If other modules are expected to import it, change toexport type CanvasContext = CanvasRenderingContext2D;.
193-198: Narrowxx/yytypes to prevent invalid strings being cast to numbers.
xx?: number | string+focalX = xx as numberwill happily accept any string at runtime. If the only sentinel is'zoomButton', prefer:xx?: number | 'zoomButton'(same foryy).Also applies to: 209-238
250-260: Minor: duplicatedminiMap.style.display = 'block'.
Line 255 and Line 257 both set the same value; one can be removed.
982-1011: Simplify/verify the baseline offset math infillText2(currently a no-op).
Math.round(4 * globalScope.scale) * (1 - 0 * +(dir === 'DOWN'))always evaluates toMath.round(4 * globalScope.scale). If DOWN-specific behavior is intended, make it explicit; otherwise simplify.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/simulator/src/canvasApi.js(0 hunks)src/simulator/src/canvasApi.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/simulator/src/canvasApi.js
| export function findDimensions(scope: any = globalScope): void { | ||
| let totalObjects = 0; | ||
| simulationArea.minWidth = undefined as unknown as number; | ||
| simulationArea.maxWidth = undefined as unknown as number; | ||
| simulationArea.minHeight = undefined as unknown as number; | ||
| simulationArea.maxHeight = undefined as unknown as number; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid undefined as unknown as number for min/max bounds—this defeats type safety.
This pattern can propagate invalid numeric states. Prefer typing simulationArea.minWidth/etc as number | undefined (or initialize to Infinity/-Infinity and handle the “no objects” case explicitly).
Also applies to: 171-172
🤖 Prompt for AI Agents
In src/simulator/src/canvasApi.ts around lines 122-128 (and also apply to
171-172), do not use "undefined as unknown as number" to set min/max bounds;
instead change the types of simulationArea.minWidth/minHeight/maxWidth/maxHeight
to number | undefined or initialize them to safe numeric sentinels (e.g.,
Infinity for mins and -Infinity for maxes) and update the logic that computes
and checks these values to handle the "no objects" case explicitly; ensure
assignments and comparisons use the chosen approach consistently and remove the
unsafe type cast.
| export const fixDirection: Readonly<Record<string, Direction>> = { | ||
| right: 'LEFT', | ||
| left: 'RIGHT', | ||
| down: 'UP', | ||
| up: 'DOWN', | ||
| LEFT: 'LEFT', | ||
| RIGHT: 'RIGHT', | ||
| UP: 'UP', | ||
| DOWN: 'DOWN', | ||
| } as const; |
There was a problem hiding this comment.
fixDirection appears inverted (likely breaks rotation/orientation).
The mapping claims to “normalize direction values”, but currently maps right -> 'LEFT', down -> 'UP', etc. That’s the opposite of normalization and is very likely a functional regression.
Proposed fix:
export const fixDirection: Readonly<Record<string, Direction>> = {
- right: 'LEFT',
- left: 'RIGHT',
- down: 'UP',
- up: 'DOWN',
+ right: 'RIGHT',
+ left: 'LEFT',
+ down: 'DOWN',
+ up: 'UP',
LEFT: 'LEFT',
RIGHT: 'RIGHT',
UP: 'UP',
DOWN: 'DOWN',
} as const;📝 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.
| export const fixDirection: Readonly<Record<string, Direction>> = { | |
| right: 'LEFT', | |
| left: 'RIGHT', | |
| down: 'UP', | |
| up: 'DOWN', | |
| LEFT: 'LEFT', | |
| RIGHT: 'RIGHT', | |
| UP: 'UP', | |
| DOWN: 'DOWN', | |
| } as const; | |
| export const fixDirection: Readonly<Record<string, Direction>> = { | |
| right: 'RIGHT', | |
| left: 'LEFT', | |
| down: 'DOWN', | |
| up: 'UP', | |
| LEFT: 'LEFT', | |
| RIGHT: 'RIGHT', | |
| UP: 'UP', | |
| DOWN: 'DOWN', | |
| } as const; |
🤖 Prompt for AI Agents
In src/simulator/src/canvasApi.ts around lines 1114 to 1123, the fixDirection
mapping is inverted (e.g., right -> 'LEFT', down -> 'UP'), which reverses
intended normalization; update the object so each lowercase key maps to the
corresponding uppercase direction (right -> 'RIGHT', left -> 'LEFT', up -> 'UP',
down -> 'DOWN') while keeping the existing uppercase entries unchanged, ensuring
the mapping returns the canonical Direction for each input.
1 participant
Description
Migrates canvasApi.js to TypeScript (canvasApi.ts) as part of the ongoing JS-to-TS migration in
src/simulator/src/.Fixes #661
Changes Made
Type Definitions
Directiontype ('RIGHT' | 'LEFT' | 'UP' | 'DOWN') for element orientationCanvasContexttype alias forCanvasRenderingContext2DMiniMapAreainterface with TODO for unmigratedminimap.jsdependencyDirectionAngleMapinterface for text rotation anglesDirectiontype for use by other modulesImprovements
varwithconst/letthroughoutmoveTo,lineTo,bezierCurveToarc,arc2,drawCircle,drawCircle2rect,rect2fillText,fillText2,fillText3,fillText4,canvasMessagerotate,rotateAngle,correctWidth,validColor,colorToRGBAfindDimensions,changeScale,dots,drawLine,drawImageReadonly<Record<...>>andas const:oppositeDirection: Maps directions to their oppositesfixDirection: Normalizes direction input strings@param,@returns,@example, and@categorytagsglobalScope,MiniMapArea)Testing
tsc --noEmit)Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.