refactored load.js to load.ts and added a new file loadScope.ts in types #460
Conversation
WalkthroughThis change adds functionality for loading circuit simulation data from JSON files and managing simulation states. A new file implements logic to create circuit elements, handle module loading, clean up problematic nodes, update circuit layouts, and ensure backward compatibility. Additionally, new TypeScript interfaces are introduced to structure circuit elements, node connections, modules, layouts, and overall project metadata. Changes
Sequence Diagram(s)sequenceDiagram
participant Loader as Project Loader
participant LoadFunc as load(data)
participant ScopeFunc as loadScope(scope, data)
participant ModuleFunc as loadModule(data, scope)
participant Cleanup as removeBugNodes(scope)
participant UI as Simulation UI
Loader->>LoadFunc: Provide project JSON data
LoadFunc->>ScopeFunc: For each scope in project data
ScopeFunc->>Cleanup: Clean up problematic nodes
ScopeFunc->>ModuleFunc: Load circuit modules and set properties
ScopeFunc->>UI: Update simulation layout and UI elements
Possibly related PRs
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: 2
🧹 Nitpick comments (3)
src/simulator/src/data/load.ts (2)
47-89: Validate object initialization and potential property mismatches.The “loadModule” function loads modules using dynamic constructor parameters and node replacements. If “data.customData.values” includes properties not found on “obj”, or if type mismatches occur, TypeScript might not catch them at runtime. You may want to refine CustomData’s type definitions or add runtime checks.
208-296: Overall load flow is consistent, but watch for error handling.The “load” function orchestrates the project load process effectively, resetting scopes, focusing circuits, updating the simulation area, etc. However, any malformed or incomplete “ProjectData” object could cause runtime errors (e.g., missing “scopes” property). Consider adding guards or explicit checks to handle these edge cases more safely.
src/simulator/src/types/loadScope.ts (1)
21-22: Clarify 'nodes' types for better TypeScript safety.Currently, “nodes” is declared as “Record<string, NodeConnection | NodeConnection[]>”, which is flexible but could hide mistakes if an unexpected structure is provided. Consider refining types or adding stricter runtime checks for improved confidence.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/simulator/src/data/load.ts(1 hunks)src/simulator/src/types/loadScope.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/simulator/src/data/load.ts
[error] 25-25: Shouldn't redeclare 'TestbenchData'. Consider to delete it or rename it.
'TestbenchData' is defined here:
(lint/suspicious/noRedeclare)
🔇 Additional comments (2)
src/simulator/src/data/load.ts (2)
1-24: Import improvements & potential naming conflict warning.The import structure is logically organized and showcases a clear transition from JavaScript to TypeScript. However, be mindful that “TestbenchData” is imported both from “#/simulator/src/testbench” (line 21) and from '../types/loadScope' (line 25). This can lead to name shadowing or redeclaration issues if both imports are intended to refer to different shapes of data. Verify whether you can consolidate or rename to avoid conflicts.
33-39: Extend or document the mapping for backward compatibility.The “rectifyObjectType” function offers a mechanism to handle older object types (FlipFlop, Ram, etc.). If other components also require renaming, consider extending or documenting it in a mapping table for easier maintenance.
| function removeBugNodes(scope: Scope = globalScope): void { | ||
| let x = scope.allNodes.length; | ||
| for (let i = 0; i < x; i++) { | ||
| if ( | ||
| scope.allNodes[i].type !== 2 && | ||
| scope.allNodes[i].parent.objectType === 'CircuitElement' | ||
| ) { | ||
| scope.allNodes[i].delete?.(); //even if delete doesn't exist, it won't cause any errors | ||
| } | ||
| if (scope.allNodes.length !== x) { | ||
| i = 0; | ||
| x = scope.allNodes.length; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider removing 'removeBugNodes' or clarifying its usage.
The 'removeBugNodes' function is labelled as a “temporary fix” but still persists. If this logic is essential, consider documenting the exact cases it covers or incorporate a more robust long-term solution.
| export interface TestbenchData { | ||
| testData: any; // Define more specific type if you know the structure | ||
| currentGroup: string; | ||
| currentCase: string; | ||
| } |
There was a problem hiding this comment.
Rename or remove the duplicate 'TestbenchData' interface to avoid conflicts.
Static analysis alerts about redeclaring "TestbenchData." If the “TestbenchData” type here is functionally the same as the one imported in "load.ts", remove this version. Otherwise, rename this version to a different name (e.g., "TestbenchDataInterface") to avoid overshadowing or compile-time conflicts.
Proposed fix:
-export interface TestbenchData {
+export interface TestbenchDataInterface {
testData: any;
currentGroup: string;
currentCase: string;
}
|
Load.js has been mentioned in an earlier PR by another contributer, though ut has errors so this can be considered as a better option but please take a look at the files already under PR and then decide the next files to work upon |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| */ | ||
| function loadModule(data: ModuleData, scope: Scope): void { | ||
| // Create circuit element | ||
| const obj = new modules[rectifyObjectType(data.objectType)]( |
Check failure
Code scanning / CodeQL
Unvalidated dynamic method call High
Fixes #
In this pull request, I have converted the existing load.js file to TypeScript to enhance type safety and improved code readability.
Changes Made:
New TypeScript File (loadScope.ts):
I created a new file named loadScope.ts inside the src/types directory to define and manage all the interfaces previously used in load.js.
This new file contains all the relevant TypeScript interfaces, making them reusable across different files and simplifying future updates.
Updated load.ts:
In load.ts, I imported the interfaces from loadScope.ts to provide better type checking for the data being handled.
This modular approach ensures that the interfaces are defined in one central location (loadScope.ts), making them easier to maintain, reuse, and update when necessary.
Improved Code Readability:
The code is now more structured and understandable. By using TypeScript's type checking, it will be easier for future developers to work on and maintain this part of the code.
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
New Features
Bug Fixes