refactor: TS integration in modules.js #472
Conversation
WalkthroughThe pull request refactors module management within the simulation by removing the JavaScript file and replacing it with a TypeScript implementation. The new TypeScript file introduces helper functions for marking occupied Y positions, finding the next available Y value, and updating element positions when layout changes occur. Additionally, a new TypeScript file provides type declarations for simulation layout, inputs/outputs, modules, and size-changing contexts, ensuring stricter type safety. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant getNextPosition
participant Helper
Caller->>getNextPosition: call getNextPosition(x: number, scope: GlobalScope)
getNextPosition->>Helper: markOccupiedYPositions(elements)
getNextPosition->>Helper: findNextAvailableY(done, startY)
getNextPosition->>getNextPosition: Check if newY exceeds layout.height
alt Height exceeded
getNextPosition->>Helper: updateElementPositions(oldHeight, newHeight)
end
getNextPosition->>Caller: return next available Y
sequenceDiagram
participant Caller
participant changeInputSize
participant ModuleInstance
Caller->>changeInputSize: call changeInputSize(size)
changeInputSize->>changeInputSize: Validate new size (within 2 to 10)
alt Valid & different size
changeInputSize->>ModuleInstance: Create new module instance
ModuleInstance-->>changeInputSize: new instance created
changeInputSize->>Caller: return new ModuleInstance
else Not valid/different
changeInputSize->>Caller: perform no changes
end
Possibly related PRs
Suggested reviewers
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
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. 🪧 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. |
✅ 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 (4)
src/simulator/src/types/modules.types.ts (3)
10-16: Consider extracting layout into its own interfaceThe
Scopeinterface is well-structured, but for better maintainability, consider extracting the layout object into its own interface.+interface Layout { + height: number; +} interface Scope { Input: InputOutputElement[]; Output: InputOutputElement[]; - layout: { - height: number; - }; + layout: Layout; }
22-31: Consider using enum for direction parameterThe
directionparameter is typed as string, but it likely has a limited set of valid values. Consider using a string enum for better type safety.+enum Direction { + LEFT = "LEFT", + RIGHT = "RIGHT", + UP = "UP", + DOWN = "DOWN" +} interface Module { new ( x: number, y: number, scope: GlobalScope, - direction: string, + direction: Direction, size: number, bitWidth: number ): ModuleInstance; }
41-50: Consider consistent direction typing with Module interfaceIf you implement a Direction enum as suggested earlier, ensure it's also used here for consistency.
interface ChangeInputSizeContext { inputSize: number; objectType: string; x: number; y: number; scope: GlobalScope; - direction: string; + direction: Direction; bitWidth: number; delete(): void; }src/simulator/src/modules.ts (1)
61-64: Improve comment for global modules objectThe comment "Global" is not very descriptive. Consider improving it to better explain this object's purpose.
/** - * Global + * Dictionary of available module types that can be instantiated */ export const modules: Modules = {};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/simulator/src/modules.js(0 hunks)src/simulator/src/modules.ts(1 hunks)src/simulator/src/types/modules.types.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/simulator/src/modules.js
🧰 Additional context used
🪛 Biome (1.9.4)
src/simulator/src/modules.ts
[error] 66-66: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
🔇 Additional comments (9)
src/simulator/src/types/modules.types.ts (5)
1-4: Clear and well-structured layout interfaceThe
LayoutPropertiesinterface provides a clean definition for coordinate positioning used throughout the module system.
6-8: Good abstraction for elements with positionsThe
InputOutputElementinterface creates a good abstraction for elements that need positioning in the layout.
18-20: Future-proof GlobalScope extensionUsing extension here provides a clean way to add global-specific properties in the future without modifying the base Scope interface.
37-39: Good dictionary type for modules collectionThe
Modulesinterface provides a clean way to define a dictionary of available module types.
33-35:❓ Verification inconclusive
ModuleInstance interface appears minimal
The
ModuleInstanceinterface only defines a delete method. Consider expanding it to include other properties and methods that module instances should have.
🏁 Script executed:
#!/bin/bash # Check for additional properties/methods used with module instances rg --type ts --type js "\.(?!delete\(\))([\w]+)(?:\s*=|\()" -A 1 -B 1Length of output: 311
ModuleInstance Interface Minimality – Further Verification Required
The current implementation of the
ModuleInstanceinterface atsrc/simulator/src/types/modules.types.ts(lines 33–35) includes only thedeletemethod. An initial search for accesses to additional properties/methods (aside fromdelete) resulted in a regex error due to unsupported look-around.To verify whether module instances are used with additional properties or methods elsewhere in the codebase, please run the following revised shell script using PCRE2 support:
#!/bin/bash # Attempt a re-run using PCRE2 to check for additional properties/method usages on ModuleInstance instances rg --pcre2 --type ts --type js "\.(?!delete\(\))([\w]+)(?:\s*=|\()" -A 1 -B 1Manually verify the output of this command:
- If additional properties or methods are referenced on module instances, consider expanding the
ModuleInstanceinterface accordingly.- If no usage beyond
deleteis found, confirm that the minimal implementation is intentional.src/simulator/src/modules.ts (4)
7-13: Well-implemented helper functionThis helper function efficiently marks occupied positions in the coordinate system.
29-35: Clear element position update logicThis helper function provides clean logic for updating element positions when layout height changes.
37-59: Well-structured position calculation functionThe
getNextPositionfunction effectively uses helper functions to calculate positions and update the layout as needed.
1-3:❓ Verification inconclusive
Address import cycle warning
The ESLint disable comment suggests there's a circular dependency between modules. Circular dependencies can lead to maintenance issues and initialization problems.
🏁 Script executed:
#!/bin/bash # Check for import cycles echo "Checking for import cycles..." rg -l "import.*from.*simulationArea" | xargs -I{} rg -l "import.*from.*modules" {} echo "Checking what simulationArea imports from modules..." rg "import.*from.*modules" ./src/simulator/src/simulationArea.tsLength of output: 3423
Circular Dependency Check – Manual Verification Required
Our investigation shows that while
src/simulator/src/modules.tsimportssimulationAreafrom./simulationArea, a direct cycle isn’t evident—simulationArea.tsdoes not import any module from themodulesdirectory. However, the search results were not conclusive regarding any indirect cycles elsewhere in the dependency graph. Please manually review the broader dependency chain to confirm that no circular dependency exists. If no cycle is found, the ESLint disable comment (/* eslint-disable import/no-cycle */) can be safely removed.
| function findNextAvailableY(done: { [key: number]: number }, startY: number): number { | ||
| let possibleY = startY; | ||
| while (done[possibleY] || done[possibleY + 10] || done[possibleY - 10]) { | ||
| possibleY += 10; | ||
| } | ||
| return possibleY; | ||
| } |
There was a problem hiding this comment.
Add safeguard against potential infinite loop
The findNextAvailableY function lacks a safeguard against infinite loops. If all positions are marked as occupied, this function would never terminate.
function findNextAvailableY(done: { [key: number]: number }, startY: number): number {
let possibleY = startY;
+ const maxIterations = 1000; // Prevent infinite loops
+ let iterations = 0;
while (done[possibleY] || done[possibleY + 10] || done[possibleY - 10]) {
possibleY += 10;
+ iterations++;
+ if (iterations > maxIterations) {
+ console.warn('Maximum iterations reached in findNextAvailableY');
+ break;
+ }
}
return possibleY;
}📝 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.
| function findNextAvailableY(done: { [key: number]: number }, startY: number): number { | |
| let possibleY = startY; | |
| while (done[possibleY] || done[possibleY + 10] || done[possibleY - 10]) { | |
| possibleY += 10; | |
| } | |
| return possibleY; | |
| } | |
| function findNextAvailableY(done: { [key: number]: number }, startY: number): number { | |
| let possibleY = startY; | |
| const maxIterations = 1000; // Prevent infinite loops | |
| let iterations = 0; | |
| while (done[possibleY] || done[possibleY + 10] || done[possibleY - 10]) { | |
| possibleY += 10; | |
| iterations++; | |
| if (iterations > maxIterations) { | |
| console.warn('Maximum iterations reached in findNextAvailableY'); | |
| break; | |
| } | |
| } | |
| return possibleY; | |
| } |
| export function changeInputSize(this: ChangeInputSizeContext, size: number): ModuleInstance | void { | ||
| if (size === undefined || size < 2 || size > 10) return; | ||
| if (this.inputSize === size) return; | ||
|
|
||
| size = parseInt(size.toString(), 10); | ||
|
|
||
| const obj: ModuleInstance = new modules[this.objectType]( | ||
| this.x, | ||
| this.y, | ||
| this.scope, | ||
| this.direction, | ||
| size, | ||
| this.bitWidth | ||
| ); | ||
|
|
||
| this.delete(); | ||
| simulationArea.lastSelected = obj; | ||
| return obj; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix return type and improve parameter handling
The function has a few issues to address:
- Using
voidin a union type is confusing (per static analysis) - Modifying the input parameter directly
- Relying on
thiscontext which can be error-prone
-export function changeInputSize(this: ChangeInputSizeContext, size: number): ModuleInstance | void {
+export function changeInputSize(this: ChangeInputSizeContext, size: number): ModuleInstance | undefined {
if (size === undefined || size < 2 || size > 10) return;
if (this.inputSize === size) return;
- size = parseInt(size.toString(), 10);
+ const parsedSize = parseInt(size.toString(), 10);
const obj: ModuleInstance = new modules[this.objectType](
this.x,
this.y,
this.scope,
this.direction,
- size,
+ parsedSize,
this.bitWidth
);
this.delete();
simulationArea.lastSelected = obj;
return obj;
}📝 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 function changeInputSize(this: ChangeInputSizeContext, size: number): ModuleInstance | void { | |
| if (size === undefined || size < 2 || size > 10) return; | |
| if (this.inputSize === size) return; | |
| size = parseInt(size.toString(), 10); | |
| const obj: ModuleInstance = new modules[this.objectType]( | |
| this.x, | |
| this.y, | |
| this.scope, | |
| this.direction, | |
| size, | |
| this.bitWidth | |
| ); | |
| this.delete(); | |
| simulationArea.lastSelected = obj; | |
| return obj; | |
| } | |
| export function changeInputSize(this: ChangeInputSizeContext, size: number): ModuleInstance | undefined { | |
| if (size === undefined || size < 2 || size > 10) return; | |
| if (this.inputSize === size) return; | |
| const parsedSize = parseInt(size.toString(), 10); | |
| const obj: ModuleInstance = new modules[this.objectType]( | |
| this.x, | |
| this.y, | |
| this.scope, | |
| this.direction, | |
| parsedSize, | |
| this.bitWidth | |
| ); | |
| this.delete(); | |
| simulationArea.lastSelected = obj; | |
| return obj; | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 66-66: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
Fixes #414
@Arnabdaz @JoshVarga
Summary by CodeRabbit
New Features
Refactor