refactor: typescript integration : src/simulator/src/wire.ts #442
Conversation
|
Warning Rate limit exceeded@ThatDeparted2061 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 19 minutes and 12 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe pull request enhances the Changes
Assessment against linked issues
Poem
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/wire.ts (3)
9-13: Consider improving type safety of the Scope interface.The
rootproperty is typed asany, which defeats TypeScript's type checking benefits. Consider defining a more specific type for therootproperty based on its actual structure and usage.interface Scope { wires: Wire[]; - root: any; // Replace 'any' with the appropriate type if known + root: { [key: string]: unknown }; // Or a more specific interface/type timeStamp: number; }
69-70: Simplify optional chaining in checkConnections method.The optional chaining and nullish coalescing operators could be simplified for better readability.
- !(this.node1.connections?.includes(this.node2)) || - !(this.node2.connections?.includes(this.node1) ?? false); + !this.node1.connections?.includes(this.node2) || + !this.node2.connections?.includes(this.node1);
247-252: Simplify null checks in delete method.The null checks for connections are redundant since the filter operation is safe on undefined/null arrays.
- if (this.node1.connections) { - this.node1.connections = this.node1.connections.filter(x => x !== this.node2); - } - if (this.node2.connections) { - this.node2.connections = this.node2.connections.filter(x => x !== this.node1); - } + this.node1.connections = this.node1.connections?.filter(x => x !== this.node2) ?? []; + this.node2.connections = this.node2.connections?.filter(x => x !== this.node1) ?? [];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/simulator/src/wire.ts(4 hunks)
🔇 Additional comments (2)
src/simulator/src/wire.ts (2)
25-47: Well-structured class properties and constructor implementation!Good job on adding proper type annotations for all class properties and constructor parameters. The initialization logic is clear and type-safe.
196-198: Consider adding type constraints for Node parameters.Methods
checkConvergenceandconvergeaccept Node parameters, but there might be specific requirements for these nodes. Consider adding type constraints or validation.Also applies to: 238-241
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/simulator/src/wire.ts (2)
70-71:⚠️ Potential issueFix reference to undeclared
globalScopevariable
This usage ofglobalScopemirrors a past comment and can cause runtime errors if it's not defined or imported.+import { globalScope } from './globalScope'; // Example import - if ( - this.node1.parent == globalScope.root && - this.node2.parent == globalScope.root - ) { + if ( + this.node1.parent === globalScope.root && + this.node2.parent === globalScope.root + ) {
81-81:⚠️ Potential issueFix reference to undeclared
embedvariable
As withglobalScope, theembedvariable is used but not declared or imported, which can lead to runtime errors.+import { embed } from './embed'; // Example import if (embed) return updated;
🧹 Nitpick comments (3)
src/simulator/src/wire.ts (3)
10-10: Replace 'any' with a known type forrootproperty
If you know the concrete type ofroot, using a strongly-typed approach would improve maintainability and catch type errors at compile-time.- root: any; // Replace 'any' with the appropriate type if known + root: RootObject; // Example placeholder type: replace with the actual type
42-48: Use local variables to avoid repeatedly callingabsX()andabsY()
StoringabsX()andabsY()results in local variables reduces repeated calls, which can marginally improve code clarity and performance.updateData(): void { - this.x1 = this.node1.absX(); - this.y1 = this.node1.absY(); - this.x2 = this.node2.absX(); - this.y2 = this.node2.absY(); - if (this.x1 === this.x2) this.type = 'vertical'; + const node1X = this.node1.absX(); + const node1Y = this.node1.absY(); + const node2X = this.node2.absX(); + const node2Y = this.node2.absY(); + + this.x1 = node1X; + this.y1 = node1Y; + this.x2 = node2X; + this.y2 = node2Y; + if (node1X === node2X) this.type = 'vertical'; }
177-185: Consider support for diagonal wires
Currently, wires only switch between'vertical'or'horizontal'. If diagonal wires (or other angles) are needed in the future, you may need to extend this logic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/simulator/src/wire.ts(1 hunks)
🔇 Additional comments (1)
src/simulator/src/wire.ts (1)
1-265: Overall implementation aligns with PR objectives
The added TypeScript annotations,Scopeinterface introduction, and helper methods significantly improve readability and type safety. Aside from the flagged issues, everything else looks well-structured. Great job!
|
@JoshVarga @niladrix719 having 24 functions really reduces the cognitive complexity wud you still want me to to fix the codeclimate issue ? |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/simulator/src/wire.ts (1)
8-12: Refine therootproperty’s type instead of usingany.
It’s best to replaceanywith a more appropriate type (or a union of types) to benefit from TypeScript’s compile-time checks.interface Scope { wires: Wire[]; - root: any; // Replace 'any' with the appropriate type if known + root: HTMLElement | SomeOtherType; // Or a union type if needed timeStamp: number; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/simulator/src/wire.ts(1 hunks)
🧰 Additional context used
📓 Learnings (1)
src/simulator/src/wire.ts (3)
Learnt from: ThatDeparted2061
PR: CircuitVerse/cv-frontend-vue#442
File: src/simulator/src/wire.ts:81-81
Timestamp: 2025-01-27T17:29:35.080Z
Learning: The `embed` variable in the CircuitVerse simulator is a boolean flag used to determine if the circuit is in embedded mode, defined in `src/simulator/src/embed.ts`.
Learnt from: ThatDeparted2061
PR: CircuitVerse/cv-frontend-vue#442
File: src/simulator/src/wire.ts:0-0
Timestamp: 2025-01-27T17:29:33.929Z
Learning: In the CircuitVerse frontend Vue project, globalScope is a global variable that should be typed by extending the Window interface in a TypeScript declaration file (global.d.ts), as it's initialized on the window object.
Learnt from: ThatDeparted2061
PR: CircuitVerse/cv-frontend-vue#442
File: src/simulator/src/wire.ts:0-0
Timestamp: 2025-01-27T17:29:33.929Z
Learning: In the CircuitVerse frontend Vue project, globalScope should be declared on the window object using TypeScript declaration files (.d.ts) rather than importing it as a module.
🔇 Additional comments (16)
src/simulator/src/wire.ts (16)
14-24: Overall class structure looks good.
The class-level fields and TypeScript conversions are consistent and well-organized.
25-39: Constructor logic is straightforward and well-initialized.
Nicely sets up wire object type, coordinates, and updates data.
41-48: Good practice to centralize coordinate updates inupdateData().
This method ensures wire coordinates remain in sync as the simulation changes.
50-54: Scope update approach is acceptable.
Re-checking connections upon scope switch is logical.
56-65: Connection checks are concise and effective.
Deleting the wire immediately when nodes are invalid is a good approach.
67-76: Fix reference to undeclaredglobalScope.
This usage referencesglobalScope.root, but the variable is not imported or declared in this file. Per your previous note, it should be declared globally via a.d.tsfile (extending theWindowinterface) rather than imported.// Example snippet for globalScope declaration in global.d.ts declare global { interface Window { globalScope?: Scope; } } export {};
78-96: Fix reference to undeclaredembed.
This usage ofembedon line 81 will cause a runtime error unless it is declared or imported. You may need to define and export it from anembed.tsfile, then import it here.// Example snippet in embed.ts export const embed: boolean = false; // Then import it where needed: import { embed } from './embed';
98-111: Drawing logic is nicely encapsulated.
UsingdrawLinefromcanvasApikeeps the wire rendering code concise.
113-137: Wire coordinate checks and geometry helpers look good.
ThecheckWithinandisBetweenfunctions cleanly handle horizontal/vertical wire checks.
139-144: Intermediate node handling.
Converging on a new node before deleting the wire is a clear approach.
146-167: Deletion sequence is correct.
Properly updates the simulation sets, removes mutual connections, and updates timestamps.
169-183: Wire orientation logic is solid.
Automatically recalculating wire type based on node positions is helpful for a dynamic layout.
185-199: Mouse interaction handling is intuitive.
The approach of checking wire selection or drag keeps the code organized.
201-226: Selection and drag detection logic appears robust.
Creating an intermediate node upon drag is a nice user experience improvement.
228-237: Node alignment helpers are well written.
Breaking out alignment along the X or Y axis clarifies the logic.Also applies to: 239-260, 262-279
281-297: Color determination method is well-structured.
Maintains consistent theming for wire states and bitwidths.
|
@JoshVarga I dont think that the current code climate issues should result in refactoring , since the functions are enough in number for code understanding |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/simulator/src/wire.ts (2)
49-54:⚠️ Potential issueFix reference to undeclared global variable.
The
globalScopevariable is used but not imported or declared. This could cause runtime errors.Based on previous discussions, this needs to be declared in a TypeScript declaration file. Create a new file
src/types/global.d.ts:import { Scope } from '../simulator/src/scope' declare global { interface Window { globalScope: Scope } } export {}
56-58:⚠️ Potential issueFix reference to undeclared variable.
The
embedvariable is used but not imported or declared. This could cause runtime errors.Based on the learnings, import the embed variable from the embed module:
+import { embed } from './embed';
🧹 Nitpick comments (4)
src/simulator/src/wire.ts (4)
17-20: Initialize coordinate properties in the constructor instead of using non-null assertions.The non-null assertion operator (!) should be avoided when possible. These properties can be safely initialized in the constructor.
- x1!: number; - y1!: number; - x2!: number; - y2!: number; + x1: number; + y1: number; + x2: number; + y2: number;Then initialize them in the constructor:
constructor(public node1: Node, public node2: Node, public scope: Scope) { this.x1 = this.node1.absX(); this.y1 = this.node1.absY(); this.x2 = this.node2.absX(); this.y2 = this.node2.absY(); this.updateData(); this.scope.wires.push(this); forceResetNodesSet(true); }
88-101: Simplify the checkWithin method using early returns.The method can be simplified by using early returns and avoiding unnecessary variable declarations.
checkWithin(x: number, y: number): boolean { - const isHorizontal = this.type === 'horizontal'; - const isVertical = this.type === 'vertical'; - - if (isHorizontal) { + if (this.type === 'horizontal') { return y === this.node1.absY() && this.isBetween(x, this.node1.absX(), this.node2.absX()); } - if (isVertical) { + if (this.type === 'vertical') { return x === this.node1.absX() && this.isBetween(y, this.node1.absY(), this.node2.absY()); } return false; }
241-256: Improve readability of the getWireColor method.The array access using magic numbers could be replaced with an enum or constant for better maintainability.
+const enum WireValue { + LOOSE = -1, + LOW = 0, + HIGH = 1 +} private getWireColor(): string { if (simulationArea.lastSelected === this) { return colors['color_wire_sel']; } if (this.node1.value === undefined || this.node2.value === undefined) { return colors['color_wire_lose']; } if (this.node1.bitWidth === 1) { return [ colors['color_wire_lose'], colors['color_wire_con'], colors['color_wire_pow'], - ][this.node1.value + 1]; + ][this.node1.value - WireValue.LOOSE]; } return colors['color_wire']; }
222-229: Improve parameter names in checkAndCreateNode method.The parameter names could be more descriptive to better convey their purpose.
private checkAndCreateNode( - current1: number, - expected1: number, + currentPosition1: number, + expectedPosition1: number, createNode1: () => Node, - current2: number, - expected2: number, + currentPosition2: number, + expectedPosition2: number, createNode2: () => Node ): boolean {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/simulator/src/wire.ts(1 hunks)
🧰 Additional context used
📓 Learnings (1)
src/simulator/src/wire.ts (1)
Learnt from: ThatDeparted2061
PR: CircuitVerse/cv-frontend-vue#442
File: src/simulator/src/wire.ts:81-81
Timestamp: 2025-01-27T17:29:35.080Z
Learning: The `embed` variable in the CircuitVerse simulator is a boolean flag used to determine if the circuit is in embedded mode, defined in `src/simulator/src/embed.ts`.
Fixes #414
@JoshVarga @Arnabdaz @niladrix719 @devartstar
Summary by CodeRabbit
Refactor
Chores