refactor setup.js to ts #666
Conversation
WalkthroughConverted JavaScript setup logic in 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 |
✅ 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: 4
🧹 Nitpick comments (3)
src/simulator/src/types/setup.types.ts (2)
4-4: Consider a more specific type thananyfor the data property.Using
Record<string, any>weakens type safety. If the structure is unknown, consider usingRecord<string, unknown>which requires type guards before usage, or define a more specific interface based on howwindow.datais actually used in the codebase.- data: Record<string, any>; + data: Record<string, unknown>;
15-18: Strengthen type safety in ProjectData interface.The index signature
[key: string]: anysignificantly weakens type safety. Consider defining specific properties based on the actual project data structure, or useunknowninstead ofanyto enforce type guards at usage sites.export interface ProjectData { timePeriod?: number; - [key: string]: any; + [key: string]: unknown; }src/simulator/src/setup.ts (1)
136-137: Type cast toanydefeats type safety.Line 137 casts to
anywhen calling theloadfunction, which bypasses TypeScript's type checking. From the relevant code snippets,load.jsis still a JavaScript file without type annotations.Consider either:
- Converting
load.jsto TypeScript with proper typing, OR- Creating a proper type definition file for the load function, OR
- At minimum, add a comment explaining why the cast is necessary
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/simulator/src/setup.ts(7 hunks)src/simulator/src/types/setup.types.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/simulator/src/setup.ts (5)
src/simulator/src/backgroundArea.ts (1)
backgroundArea(4-20)src/simulator/src/interface/backgroundArea.ts (1)
BackgroundArea(1-6)src/simulator/src/circuit.ts (1)
newCircuit(210-272)src/simulator/src/types/setup.types.ts (1)
ProjectData(15-18)src/simulator/src/data/load.js (1)
load(207-293)
🔇 Additional comments (2)
src/simulator/src/setup.ts (2)
70-85: Good defensive null checks.These null checks are well-implemented and prevent potential runtime errors. This is the pattern that should be followed consistently throughout the function (see comments on lines 54 and 60).
162-169: Good null safety improvements.The null checks before parsing localStorage items (lines 162-163) prevent potential runtime errors. This is a solid improvement over the previous implementation.
Note: Line 165 has the same
as anycast issue mentioned in the comment for lines 136-137.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/simulator/src/setup.ts (2)
191-197: Use window.embed (bare embed will fail TS and is inconsistent).Access the global via Window augmentation for consistency and to avoid TS compile errors.
- if (!localStorage.getItem('tutorials_tour_done') && !embed) { + if (!localStorage.getItem('tutorials_tour_done') && !window.embed) { @@ - if (!embed) { + if (!window.embed) {Run to confirm no bare globals remain:
#!/bin/bash rg -nP -C1 '(?<!\.)\b(embed|DPR|width|height)\b' src/simulator/src/setup.tsAlso applies to: 205-214
45-61: Good: replaced non-null assertions with explicit null checks.Safer, aligns with defensive patterns and avoids runtime crashes.
🧹 Nitpick comments (2)
src/simulator/src/setup.ts (2)
30-32: Avoid local interface + cast for plotArea; add a .d.ts or convert to TS.The local cast hides API drift. Provide real types so you can call plotArea.setup() without assertions.
- Create src/simulator/src/plotArea.d.ts:
declare const plotArea: { setup(): void reset(): void } export default plotArea
- Then remove the local interface and cast:
-interface PlotArea { - setup: () => void -} @@ - ;(plotArea as PlotArea).setup() + plotArea.setup()Also applies to: 87-89
112-114: Type window.data to ProjectData for clarity.Improves type-safety and self-documentation.
- window.data = {} + window.data = {} as ProjectData
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/simulator/src/setup.ts(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/simulator/src/setup.ts (5)
src/simulator/src/backgroundArea.ts (1)
backgroundArea(4-20)src/simulator/src/canvasApi.js (1)
dots(122-203)src/simulator/src/circuit.ts (1)
newCircuit(210-272)src/simulator/src/types/setup.types.ts (1)
ProjectData(15-18)src/simulator/src/data/load.js (1)
load(207-293)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/simulator/src/setup.ts (1)
174-208: Good error handling; simplify redundant cast.The try-catch blocks appropriately handle errors during data loading and recovery. However, line 190 contains a redundant cast since
JSON.parse()returnsanyby default.Apply this diff to simplify the cast:
- const data = JSON.parse(item) as unknown as ProjectData + const data = JSON.parse(item) as ProjectDataThe
as anycast on line 191 is acceptable technical debt whileload.jsremains untyped.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/simulator/src/setup.ts(7 hunks)src/simulator/src/types/setup.types.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/simulator/src/types/setup.types.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/simulator/src/setup.ts (8)
src/simulator/src/backgroundArea.ts (1)
backgroundArea(4-20)src/simulator/src/canvasApi.js (1)
dots(122-203)src/simulator/src/moduleSetup.js (1)
setupModules(67-135)src/simulator/src/utils.ts (2)
generateId(25-35)showMessage(66-71)src/simulator/src/engine.js (1)
updateSimulationSet(99-101)src/simulator/src/circuit.ts (1)
newCircuit(210-272)src/simulator/src/types/setup.types.ts (1)
ProjectData(15-20)src/simulator/src/data/load.js (1)
load(207-293)
🔇 Additional comments (6)
src/simulator/src/setup.ts (6)
28-32: Good approach for typing untyped dependencies.The local
PlotAreainterface is a clean workaround for typing the imported JavaScriptplotAreamodule during incremental conversion.
40-94: Excellent defensive programming and type safety.The function properly:
- Guards all DOM element accesses with null checks
- Throws clear errors when critical elements are missing
- Guards optional elements before style updates
- Uses window-prefixed global properties consistently
The cast on line 88 is appropriate for interacting with the untyped
plotAreamodule.
107-117: LGTM!The explicit arguments to
newCircuitclearly convey intent and match the expected function signature.
124-167: Well-structured with proper error handling.The function correctly:
- Returns early after redirects to prevent further execution
- Uses strict equality and URL encoding for project names
- Handles errors gracefully with try-catch
The
as anycast on line 156 is acceptable technical debt whileload.jsremains untyped during incremental conversion.
216-222: LGTM!The localStorage check correctly identifies when the tour hasn't been completed, and the condition properly guards against showing the tour in embed mode.
230-239: Solid entry point for simulator initialization.The function properly orchestrates the setup sequence with clear, logical steps. The TypeScript conversion preserves the original functionality while adding type safety.
2 participants
Fixes #661
Describe the changes you have made in this PR -
modified setup.js to typescript while maintaining the logic
Summary by CodeRabbit
Refactor
Bug Fixes