Refactor: canvas migration from js to ts #662
Conversation
WalkthroughTypeScript migration of canvas API code introducing explicit type definitions and function signatures. A new types file defines Direction, DirectionFix, SimulationObject, and Scope interfaces. The main canvasApi.ts file adds type annotations to 20+ exported functions and public constants without altering underlying logic. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes High homogeneity in changes (consistent pattern of adding type annotations across multiple functions), but substantial scope with 20+ function signatures and new type definitions requiring validation against usage patterns throughout the codebase. 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 |
✅ Deploy Preview for circuitverse ready!
To edit notification comments on pull requests, go to your Netlify project 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
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/canvasApi.ts (1)
86-149: Unsafe type assertions could cause runtime errors.Lines 125-126 use type assertions (
as number) that assumexxandyyare always numbers at that point. However, there are code paths where they might remain'zoomButton'orundefined:
- If
methodis not 1, 2, or 3- AND the conditions at line 99 don't trigger (i.e., xx and yy are not undefined or 'zoomButton')
- OR if they do trigger but there's no valid
lastSelectedand method is not 1 or 2In these cases, the type assertions hide potential runtime errors where non-numeric values would be used in arithmetic operations.
Consider adding explicit validation before the type assertions:
+ if (typeof xx !== 'number' || typeof yy !== 'number') { + console.warn('Invalid zoom coordinates, using screen center'); + xx = (width / 2 - globalScope.ox) / globalScope.scale; + yy = (height / 2 - globalScope.oy) / globalScope.scale; + } + const focusX = xx as number const focusY = yy as numberAlternatively, refactor to ensure all code paths assign numeric values or throw an error for invalid cases.
🧹 Nitpick comments (1)
src/simulator/src/types/canvasApi.types.ts (1)
25-32: Consider usingvarinstead ofletfor global variable declarations.In TypeScript's
declare globalblocks, global variables are typically declared withvarsinceletimplies block scope. For better TypeScript practices, consider either usingvaror extending theWindowinterface.Apply this diff if you want to use the more idiomatic approach:
declare global { - let globalScope: Scope; - let width: number; - let height: number; - let DPR: number; - let embed: boolean; - let lightMode: boolean; + var globalScope: Scope; + var width: number; + var height: number; + var DPR: number; + var embed: boolean; + var lightMode: boolean; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/simulator/src/canvasApi.ts(23 hunks)src/simulator/src/types/canvasApi.types.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/simulator/src/types/canvasApi.types.ts (1)
src/simulator/src/restrictedElementDiv.js (1)
globalScope(4-4)
src/simulator/src/canvasApi.ts (4)
src/simulator/src/types/canvasApi.types.ts (4)
Scope(18-23)SimulationObject(5-16)Direction(2-2)DirectionFix(3-3)src/simulator/src/metadata.ts (1)
updateOrder(67-72)src/simulator/src/minimap.js (2)
updatelastMinimapShown(175-177)removeMiniMap(178-194)src/simulator/src/backgroundArea.ts (1)
backgroundArea(4-20)
🔇 Additional comments (12)
src/simulator/src/types/canvasApi.types.ts (2)
2-3: LGTM! Direction types are well-defined.The
DirectionandDirectionFixtypes provide good type safety for direction handling throughout the canvas API, withDirectionFixaccommodating both upper and lowercase variants.
5-16: SimulationObject interface looks good.The interface appropriately captures the dimensional properties and methods needed for canvas simulation objects. The index signature
[key: string]: anyprovides flexibility for the migration while the explicitly typed properties ensure type safety for critical fields.src/simulator/src/canvasApi.ts (10)
7-14: LGTM! Clean imports and constant declaration.The imports from the new types file are well-organized, and the
unitconstant is properly typed.
20-77: LGTM! Well-typed dimension finding logic.The function signature properly types the
scopeparameter with a default value, and theSimulationObjecttype provides good type safety for the iteration logic. The function correctly preserves the original behavior while adding type safety.
159-239: LGTM! Properly typed with good defensive checks.The function signature is well-typed with appropriate default parameters, and the guard check at line 169 prevents null reference errors. The canvas drawing logic correctly preserves the original behavior.
259-505: LGTM! Drawing helpers are well-typed.The drawing helper functions (
bezierCurveTo,moveTo,lineTo,arc,arc2,drawCircle2) have clean type signatures usingCanvasRenderingContext2DandDirectiontypes. ThebezierCurveTofunction adds a defensive null check at line 285, though other helpers rely on callers to provide valid contexts. The logic is correctly preserved from the JavaScript version.
515-602: LGTM! Rectangle and image drawing functions are well-typed.The
rect,drawImage, andrect2functions use appropriate TypeScript types includingCanvasImageSourcefor the image parameter andDirectionwith a sensible default value of'RIGHT'forrect2. Logic is correctly preserved.
611-659: LGTM! Utility functions with correct tuple return types.The
rotate,correctWidth, androtateAnglefunctions are properly typed with appropriate return types, including tuple types[number, number]and[number, number, boolean]that accurately represent the multi-value returns. The logic is correctly preserved.
671-761: LGTM! Drawing and color utilities are well-typed.The
drawLine,validColor,colorToRGBA, anddrawCirclefunctions are properly typed. Note that line 725 uses a non-null assertion operator (!) when getting the 2D context, which is generally safe for regular canvas elements but assumes the context will never be null.
771-966: LGTM! Text rendering functions are comprehensively typed.The text rendering functions (
canvasMessage,fillText,fillText2,fillText3,fillText4) are well-typed with appropriate use ofCanvasTextAlign,Record<Direction, number>for angle mappings, and sensible default parameters. The functions correctly preserve the original rendering logic while adding type safety.
968-973: LGTM! Opposite direction mapping is correct.The
oppositeDirectionconstant correctly maps eachDirectionto its opposite with proper typing usingRecord<Direction, Direction>.
975-984: Behavior verified as intentional; consider adding clarification comment.The
fixDirectionmapping logic is confirmed to be correct and consistent with the original JavaScript implementation across all versions. The dual behavior—normalizing lowercase directions to uppercase while reversing them—is used during data loading and element initialization to handle mixed-format direction values.The current implementation matches v0 and v1 identically, confirming this is intentional design. Your optional suggestion to add an explanatory comment remains valid for clarity:
export const fixDirection: Record<DirectionFix, Direction> = { // Normalize lowercase directions to uppercase and reverse them (for backward compatibility) right: 'LEFT', left: 'RIGHT', down: 'UP', up: 'DOWN', // Uppercase directions pass through unchanged LEFT: 'LEFT', RIGHT: 'RIGHT', UP: 'UP', DOWN: 'DOWN', }
2 participants
Fixes #661
Describe the changes you have made in this PR -
converting the canvas.js to typescript while maintaining the logic and rendering same
Note: Please check Allow edits from maintainers. if you would like us to assist in the PR.
Summary by CodeRabbit