refactor Js integration and file restructure : src/simulator/src/Verilog2CV.ts #449
Conversation
WalkthroughThis pull request removes the legacy JavaScript file Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant UI as UI Controller
participant CH as Circuit Handler
participant CM as CodeMirror Editor
U->>UI: Initiate Verilog simulation
UI->>CH: call createVerilogCircuit()
CH-->>UI: Circuit created or error reported
UI->>CM: setupCodeMirrorEnvironment()
CM-->>UI: Editor ready with theme & settings
U->>UI: Toggle Verilog mode (verilogModeSet)
UI-->>U: UI components updated
Assessment against linked issues
Suggested labels
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: 1
🧹 Nitpick comments (12)
src/simulator/src/Verilog2CV.ts (7)
1-10: Consider limiting imports to reduce bundle size.While referencing CodeMirror styles and add-ons is valid, consider selectively importing only the themes and add-ons in use, if possible, to reduce the final bundle size.
47-48: Global variable usage may limit reusability.
editoris declared globally, andverilogModeis used as a global boolean. Consider encapsulating these in a class or module to improve testability and maintainability.
70-74: Possible race condition in saving code and circuit generation.When
saveVerilogCodecallsgenerateVerilogCircuit, the code is saved inglobalScope.verilogMetadata.code. If multiple saves happen quickly, concurrency might cause unexpected overwrites. Consider debouncing or more explicit concurrency control.
89-96: Avoid confusion by renamingverilogModeSet/verilogModeGet.Naming them as
setVerilogMode/isVerilogModeEnabledor similar may help clarify intent and match common TypeScript naming conventions.
110-134: Use progressive enhancement for UI manipulations.The code toggles the visibility of multiple DOM elements in this block. Consider employing a single container or a class toggle for better maintainability and style changes, instead of toggling each element individually.
234-282: Refine error handling ingenerateVerilogCircuit.• Consider using
async/awaitfor better readability.
• The catch block includes custom error logic for status 500, but other status codes are lumped into a single path. More granular handling may be useful.
284-313: Enhance editor configuration customization.
setupCodeMirrorEnvironmentsets default values (e.g.,'// Write Some Verilog Code Here!'). Allowing user-defined or stored settings could improve flexibility.src/simulator/src/verilogInterfaces.ts (4)
1-4: Consider versioning of metadata structure.
VerilogMetadataincludes a code string and subcircuit scope IDs. For future expansions (e.g., user preferences, transformations), consider a version field or expansion pattern to ensure backward compatibility.
34-39: Potential oversight in large nestedYosysJSON.For extensive multi-level subcircuits, confirm performance remains acceptable. Explore lazy loading or partial expansions for extremely nested designs, if needed.
41-43: Add lifestyle hooks or watchers aroundisVerilog.
SimulatorMobileStoremight benefit from watchers to handle scenario changes, e.g., toggling UI or performing code resets. This ensures consistent state transitions.
45-52: Strengthen Node interface.The
Nodeinterface is minimal. If you plan to expand node functionalities, consider capturing additional properties (like direction, data type, etc.) in future revisions. Also, confirm theparentobject is always available.types/codemirror.d.ts (1)
11-31: Extend Editor/Doc interfaces with caution.Adding methods beyond official stable APIs may cause maintenance overhead. If you add new methods or experimental features, segregate them from standard definitions to reduce confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/simulator/src/Verilog2CV.js(0 hunks)src/simulator/src/Verilog2CV.ts(1 hunks)src/simulator/src/verilogInterfaces.ts(1 hunks)tsconfig.json(2 hunks)types/codemirror.d.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/simulator/src/Verilog2CV.js
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (11)
src/simulator/src/Verilog2CV.ts (3)
32-33: Ensure consistent error handling forshowErrorandshowMessage.The code uses
showErrorandshowMessagein other places as well. Confirm that all user-facing errors, warnings, and messages consistently follow your UI/UX guidelines. Some errors might just be logged toconsole.errorinstead.
50-68: Check the promise handling forcreateVerilogCircuit.The function correctly awaits
createNewCircuitScope. However, ensure that potential errors fromcreateNewCircuitScopeor from the UI updates are handled or surfaced consistently. Currently, thetry/catchonly wraps the simulatorMobileStore logic.
138-163: Confirm data validation in VerilogSubCircuit.
VerilogSubCircuitreferencesthis.circuit.data.Input[i].labelandthis.circuit.data.Output[i].labelwithout verifying the existence ofthis.circuit.data.Inputorthis.circuit.data.Output. Consider adding safety checks to prevent runtime errors on malformed data.src/simulator/src/verilogInterfaces.ts (3)
6-11: ValidateGlobalScopeusage ofinitialize().Ensure
initialize()is invoked consistently. If the method is meant to reset or reconfigure, there should be well-defined times for calling it to prevent partial resets or race conditions.
13-17: Confirm updates to Node arrays inCircuitElementare thread-safe.While typical usage may not be concurrent, if multiple updates to
inputNodesoroutputNodesoccur, concurrency or race conditions might appear. If concurrency is not a concern, that’s fine, otherwise consider concurrency measures.
24-28: Double-check optionalcelltypeusage.In
YosysDevice,celltypeis optional. Ensure that code referencingcelltype(e.g., inVerilog2CV.ts) always checks for its existence before usage or logs a clear error if absent.types/codemirror.d.ts (3)
1-10: Ensure alignment with official CodeMirror typings.If the official
@types/codemirrorpackage is installed, compare these declarations to avoid conflicts or missing definitions. Maintaining your own typings is fine but requires staying in sync with library updates.
42-90: KeepEditorConfigurationupdated with CodeMirror changes.CodeMirror updates can add or remove configuration options. Periodically compare your typed definitions with official docs to ensure alignment with new features or deprecations.
92-97: Confirmstickyusage inPosition.
stickyis not standard in all CodeMirror contexts. If it's a custom extension, document its usage or provide fallback logic in the editor usage code.tsconfig.json (2)
24-24: Define the root of source files explicitly.
The addition of"rootDir": "src"clearly designates the base directory for the compiler’s input files, which helps maintain a clean separation between source and output code. Also, having"declarationMap": trueenhances debugging by mapping declaration files back to the TypeScript sources.
33-34: Expand file inclusion for custom type definitions.
Appending"types/**/*.d.ts"to theincludearray ensures that all custom declaration files are recognized by the compiler, improving type safety across the project. Retaining"commitlint.config.js"alongside is acceptable given the current project configuration.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/simulator/src/Verilog2CV.ts (4)
70-79: Return a Promise or handle asynchronous flow insaveVerilogCode().
This function triggersgenerateVerilogCircuit(), which performs a fetch call. MakingsaveVerilogCode()async and awaiting the fetch (or returning a promise) can help ensure that callers properly handle errors or success states.
89-103: Consider avoiding a global variable for Verilog mode.
verilogModeis stored in a file-scoped variable. Storing such state in a more centralized place (e.g., the simulator store) can improve maintainability and consistency.
104-126: Consolidate repetitive DOM toggle logic.
enableVerilogMode()anddisableVerilogMode()switch multiple elements from'none'to'block'(or vice versa). Extracting these toggles into a unified function or data-driven approach can simplify future UI additions.
245-293: Switch to async/await ingenerateVerilogCircuit().
Using async/await can make promise handling cleaner and reduce nested.then/.catch. You can also combine error handling in a single try/catch block for improved readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/simulator/src/Verilog2CV.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (7)
src/simulator/src/Verilog2CV.ts (7)
1-48: Imports and initial declarations look good.
No issues found with the imported modules or global variable declarations in these lines.
81-88: Code reset and change detection are straightforward.
The implementation inresetVerilogCode()andhasVerilogCodeChanges()correctly references and updatesglobalScope.verilogMetadata.code.
127-133: Element display management is handled properly.
setElementDisplay()gracefully checks for element existence and applies the style display. This is a neat utility function.
135-159: Subcircuit port resolution appears correct.
BothVerilogSubCircuitandfindPortInNodes()handle port lookups in a clear, readable way. Good job encapsulating subcircuit logic.
161-193: Validate JSON structure inYosysJSON2CV.
As noted in a previous review comment, consider verifying that required keys (e.g.,json.subcircuits,json.devices, etc.) are present before accessing them to avoid runtime errors on malformed JSON.
195-243: Device and connectors processing is well structured.
processDevices(),processSubCircuitDevice(),processStandardDevice(), andprocessConnectors()cleanly organize the JSON-to-circuit transformations.
295-324: Editor initialization looks robust.
setupCodeMirrorEnvironment()appropriately configures CodeMirror mode, themes, auto-refresh, and default text for a seamless user experience.
|
I have fixed the cognitive complexities the other codeclimate issue dont seem to be so plausibe |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
src/simulator/src/types/verilog.types..ts (4)
6-11: Add return type to initialize() method.The
initialize()method in theGlobalScopeinterface should explicitly declare its return type for better type safety.- initialize(): void; + initialize(): void;
19-22: Consider using more specific types for Input/Output properties.The
CircuitDatainterface could benefit from more specific types to better represent the circuit structure.export interface CircuitData { - Input: { label: string }[]; - Output: { label: string }[]; + Input: Array<{ label: string; type?: 'input' }>; + Output: Array<{ label: string; type?: 'output' }>; }
24-27: Use string literal type for device types.Consider using a string literal type for the
typeproperty to restrict it to valid device types.export interface YosysDevice { - type: string; + type: 'input' | 'output' | 'and' | 'or' | 'not' | 'Subcircuit' | /* add other valid types */; celltype?: string; }
45-52: Consider making parent structure more flexible.The
Nodeinterface has an inconsistent structure where the parent requires averilogLabelbut the node's own label is optional.export interface Node { verilogLabel?: string; parent: { - verilogLabel: string; + verilogLabel?: string; + type?: string; }; label?: string; connect(node: Node): void; }src/simulator/src/Verilog2CV.ts (3)
128-134: Improve type safety in setElementDisplay.The function could be more type-safe by using specific types for selectors and display values.
-function setElementDisplay(selector: string, display: string): void { +type DisplayValue = 'none' | 'block' | ''; +function setElementDisplay(selector: string, display: DisplayValue): void { const element = selector.startsWith('.') || selector.startsWith('#') ? document.querySelector(selector) : document.getElementById(selector); if (element) (element as HTMLElement).style.display = display; }
153-160: Modernize findPortInNodes implementation.The function could be more concise using modern array methods.
-function findPortInNodes(nodes: Node[], data: { label: string }[], portName: string): Node | undefined { - for (let i = 0; i < nodes.length; i++) { - if (data[i].label === portName) { - return nodes[i]; - } - } - return undefined; -} +function findPortInNodes(nodes: Node[], data: { label: string }[], portName: string): Node | undefined { + return nodes[data.findIndex(d => d.label === portName)]; +}
321-325: Modernize editor refresh implementation.Replace setTimeout with a more modern approach using requestAnimationFrame.
editor.setValue('// Write Some Verilog Code Here!'); -setTimeout(function () { +requestAnimationFrame(() => { editor.refresh(); -}, 1); +});
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/simulator/src/Verilog2CV.ts(1 hunks)src/simulator/src/types/verilog.types..ts(1 hunks)
🔇 Additional comments (1)
src/simulator/src/Verilog2CV.ts (1)
50-69: 🛠️ Refactor suggestionEnhance error handling in createVerilogCircuit.
The error handling could be more specific to provide better user feedback.
export async function createVerilogCircuit(): Promise<void> { try { const returned = await createNewCircuitScope( undefined, undefined, true, true ); if (returned) { verilogModeSet(true); const simulatorMobileStore = toRefs(useSimulatorMobileStore()) as SimulatorMobileStore; simulatorMobileStore.isVerilog.value = true; } } catch (error) { - console.error('Failed to create Verilog circuit:', error); - showError('Failed to create Verilog circuit. Please try again.'); + const errorMessage = error instanceof Error ? error.message : 'Unknown error occurred'; + console.error('Failed to create Verilog circuit:', errorMessage); + showError(`Failed to create Verilog circuit: ${errorMessage}`); } }Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
src/simulator/src/types/verilog.types.ts (3)
10-11: Add return type to initialize method.The
initializemethod in theGlobalScopeinterface should explicitly declare its return type for better type safety.- initialize(): void; + initialize(): void;
19-22: Consider using more specific types for label arrays.The
CircuitDatainterface could benefit from more specific typing for the label arrays to better represent their structure and constraints.- Input: { label: string }[]; - Output: { label: string }[]; + Input: Array<{ label: string; id?: string }>; + Output: Array<{ label: string; id?: string }>;
41-43: Add JSDoc comment to explain the purpose of SimulatorMobileStore.The interface's purpose and usage context should be documented for better maintainability.
+/** + * Represents the mobile store state for the simulator. + * Used to track whether the simulator is in Verilog mode on mobile devices. + */ export interface SimulatorMobileStore { isVerilog: { value: boolean }; }src/simulator/src/Verilog2CV.ts (2)
43-45: Improve type safety of global declarations.The global declarations could be more type-safe by using definite assignment assertions or initialization.
-declare var globalScope: GlobalScope; -declare var embed: boolean; -declare var scopeList: { [key: string]: unknown }; +declare var globalScope: GlobalScope | undefined; +declare var embed: boolean | undefined; +declare var scopeList: Record<string, unknown>;
320-324: Replace setTimeout with editor events.Using setTimeout for editor refresh is not reliable. Use CodeMirror's events instead.
- editor.setValue('// Write Some Verilog Code Here!'); - setTimeout(function () { - editor.refresh(); - }, 1); + editor.setValue('// Write Some Verilog Code Here!'); + editor.on('change', () => { + editor.refresh(); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/simulator/src/Verilog2CV.ts(1 hunks)src/simulator/src/types/verilog.types.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Redirect rules - circuitverse
- GitHub Check: Header rules - circuitverse
- GitHub Check: Pages changed - circuitverse
- GitHub Check: Analyze (javascript)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/simulator/src/Verilog2CV.ts (3)
45-47: Consider using more specific types for global declarations.The global declarations could benefit from more specific types instead of using
unknown.-declare var globalScope: GlobalScope; -declare var embed: boolean; -declare var scopeList: { [key: string]: unknown }; +declare var globalScope: GlobalScope; +declare var embed: boolean; +declare var scopeList: Record<string, GlobalScope>;
138-143: Enhance type safety in setElementDisplay function.The function could benefit from stricter typing for display values and element types.
-function setElementDisplay(selector: string, display: string): void { +type DisplayValue = 'none' | 'block' | ''; +function setElementDisplay(selector: string, display: DisplayValue): void { const element = selector.startsWith('.') || selector.startsWith('#') - ? document.querySelector(selector) + ? document.querySelector<HTMLElement>(selector) : document.getElementById(selector); if (element) (element as HTMLElement).style.display = display; }
163-170: Refactor findPortInNodes to use array methods.Consider using Array.find for a more concise and functional implementation.
-function findPortInNodes(nodes: Node[], data: { label: string }[], portName: string): Node | undefined { - for (let i = 0; i < nodes.length; i++) { - if (data[i].label === portName) { - return nodes[i]; - } - } - return undefined; -} +function findPortInNodes(nodes: Node[], data: { label: string }[], portName: string): Node | undefined { + const index = data.findIndex(item => item.label === portName); + return index !== -1 ? nodes[index] : undefined; +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/simulator/src/Verilog2CV.ts(1 hunks)src/simulator/src/types/verilog.types.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/simulator/src/types/verilog.types.ts
🧰 Additional context used
🪛 Biome (1.9.4)
src/simulator/src/Verilog2CV.ts
[error] 261-309: Illegal use of an export declaration not at the top level
move this declaration to the top level
(parse)
[error] 310-344: Illegal use of an export declaration not at the top level
move this declaration to the top level
(parse)
[error] 344-344: expected } but instead the file ends
the file ends here
(parse)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/simulator/src/Verilog2CV.ts (4)
63-66: Enhance error handling with specific error messages.The current error handling is generic. Consider providing more specific error messages based on the error type to help users better understand and resolve issues.
- } catch (error) { - console.error('Failed to create Verilog circuit:', error); - showError('Failed to create Verilog circuit. Please try again.'); + } catch (error: unknown) { + console.error('Failed to create Verilog circuit:', error); + const errorMessage = error instanceof Error + ? `Failed to create Verilog circuit: ${error.message}` + : 'Failed to create Verilog circuit. Please try again.'; + showError(errorMessage); }
143-156: Optimize port lookup performance.The
getPortmethod searches through arrays linearly. Consider using a Map for O(1) port lookups, especially beneficial for circuits with many ports.class VerilogSubCircuit { circuit: CircuitElement; + private portMap: Map<string, Node>; constructor(circuit: CircuitElement) { this.circuit = circuit; + this.portMap = new Map(); + circuit.data.Input.forEach((input, i) => { + this.portMap.set(input.label, circuit.inputNodes[i]); + }); + circuit.data.Output.forEach((output, i) => { + this.portMap.set(output.label, circuit.outputNodes[i]); + }); } getPort(portName: string): Node | undefined { - return ( - findPortInNodes(this.circuit.inputNodes, this.circuit.data.Input, portName) || - findPortInNodes(this.circuit.outputNodes, this.circuit.data.Output, portName) - ); + return this.portMap.get(portName); } }
267-273: Add timeout and retry mechanism for API requests.The current implementation lacks timeout handling and retry mechanism for transient failures, which could lead to poor user experience during network issues.
+const TIMEOUT_MS = 10000; +const MAX_RETRIES = 3; +const RETRY_DELAY_MS = 1000; + +async function fetchWithTimeout(url: string, options: RequestInit, timeout: number): Promise<Response> { + const controller = new AbortController(); + const id = setTimeout(() => controller.abort(), timeout); + try { + const response = await fetch(url, { ...options, signal: controller.signal }); + clearTimeout(id); + return response; + } catch (error) { + clearTimeout(id); + throw error; + } +} + +async function fetchWithRetry(url: string, options: RequestInit): Promise<Response> { + let lastError: Error | null = null; + for (let i = 0; i < MAX_RETRIES; i++) { + try { + return await fetchWithTimeout(url, options, TIMEOUT_MS); + } catch (error) { + lastError = error as Error; + if (error instanceof Error && error.name === 'AbortError') { + throw new Error('Request timed out'); + } + await new Promise(resolve => setTimeout(resolve, RETRY_DELAY_MS)); + } + } + throw lastError; +} - fetch('/api/v1/simulator/verilogcv', { + fetchWithRetry('/api/v1/simulator/verilogcv', { method: 'POST', headers: { 'Content-Type': 'application/json', }, body: JSON.stringify(params), })
337-339: Improve editor refresh mechanism.Using
setTimeoutwith a 1ms delay is an arbitrary solution. Consider using CodeMirror's built-in refresh mechanism or waiting for the DOM to be ready.- setTimeout(function () { - editor.refresh(); - }, 1); + // Option 1: Use requestAnimationFrame for next render cycle + requestAnimationFrame(() => editor.refresh()); + + // Option 2: Wait for DOM content to be loaded + if (document.readyState === 'loading') { + document.addEventListener('DOMContentLoaded', () => editor.refresh()); + } else { + editor.refresh(); + }Also, consider adding a cleanup function to properly dispose of the CodeMirror instance:
export function cleanupCodeMirror(): void { if (editor) { editor.toTextArea(); // Removes the editor and restores the textarea editor = undefined; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/simulator/src/Verilog2CV.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Redirect rules - circuitverse
- GitHub Check: Header rules - circuitverse
- GitHub Check: Pages changed - circuitverse
- GitHub Check: Analyze (javascript)
| 500, // x | ||
| 500, // y | ||
| null, // someParam | ||
| undefined // subCircuitId (explicitly undefined) |
There was a problem hiding this comment.
@JoshVarga
Here I was trying to pass subCircuitScope[subCircuitName] but since the defintion asks for undefined so we cant pass a string,
I could not satisfy myself with the approach, mind giving me some ideas here
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/simulator/src/Verilog2CV.ts (5)
61-64: Enhance error handling with specific error types.The current error handling is generic. Consider adding specific error types and handling different error scenarios.
- } catch (error) { - console.error('Failed to create Verilog circuit:', error); - showError('Failed to create Verilog circuit. Please try again.'); + } catch (error: unknown) { + console.error('Failed to create Verilog circuit:', error); + if (error instanceof Error) { + showError(`Failed to create Verilog circuit: ${error.message}`); + } else { + showError('Failed to create Verilog circuit. Please try again.'); + }
132-137: Improve type safety insetElementDisplay.The function could be more type-safe by using specific element types and CSS display values.
-function setElementDisplay(selector: string, display: string): void { +type CSSDisplayValue = 'block' | 'none' | ''; +function setElementDisplay(selector: string, display: CSSDisplayValue): void { const element = selector.startsWith('.') || selector.startsWith('#') - ? document.querySelector(selector) - : document.getElementById(selector); + ? document.querySelector<HTMLElement>(selector) + : document.getElementById(selector) as HTMLElement | null; - if (element) (element as HTMLElement).style.display = display; + if (element) element.style.display = display;
156-163: SimplifyfindPortInNodesusing array methods.The function can be more concise using array methods.
-function findPortInNodes(nodes: Node[], data: { label: string }[], portName: string): Node | undefined { - for (let i = 0; i < nodes.length; i++) { - if (data[i].label === portName) { - return nodes[i]; - } - } - return undefined; -} +function findPortInNodes(nodes: Node[], data: { label: string }[], portName: string): Node | undefined { + const index = data.findIndex(item => item.label === portName); + return index !== -1 ? nodes[index] : undefined; +}
229-241: Extract grid-based placement logic to a utility class.The grid-based placement logic should be moved to a separate utility class for better reusability and maintainability.
+class GridPositionManager { + private lastX: number = 0; + private lastY: number = 0; + private readonly gridSize: number = 100; + private readonly maxWidth: number = 1500; + + getNextPosition(): { x: number, y: number } { + const position = { x: this.lastX, y: this.lastY }; + this.lastX += this.gridSize; + if (this.lastX > this.maxWidth) { + this.lastX = 0; + this.lastY += this.gridSize; + } + return position; + } +} + +const gridManager = new GridPositionManager();
311-320: Enhance error handling in fetch response.The error handling could be more robust by adding timeout and retry logic for server connectivity issues.
+const FETCH_TIMEOUT = 5000; +const MAX_RETRIES = 3; + +async function fetchWithTimeout(url: string, options: RequestInit, timeout: number): Promise<Response> { + const controller = new AbortController(); + const id = setTimeout(() => controller.abort(), timeout); + try { + const response = await fetch(url, { ...options, signal: controller.signal }); + clearTimeout(id); + return response; + } catch (error) { + clearTimeout(id); + throw error; + } +} + +async function fetchWithRetry(url: string, options: RequestInit, retries: number): Promise<Response> { + try { + return await fetchWithTimeout(url, options, FETCH_TIMEOUT); + } catch (error) { + if (retries > 0) { + return fetchWithRetry(url, options, retries - 1); + } + throw error; + } +} - fetch('/api/v1/simulator/verilogcv', { + fetchWithRetry('/api/v1/simulator/verilogcv', {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/simulator/src/Verilog2CV.ts(1 hunks)
🔇 Additional comments (1)
src/simulator/src/Verilog2CV.ts (1)
26-31: Remove duplicate imports.The following CodeMirror addons are imported multiple times:
codemirror/addon/hint/show-hint.jscodemirror/addon/hint/anyword-hint.js
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/simulator/src/types/verilog.types.ts (1)
47-52: Consider improving initialization type safety.The
initializemethod could benefit from a more specific return type.- initialize: () => void; + initialize: () => Promise<void>;src/simulator/src/Verilog2CV.ts (3)
69-72: Enhance error handling in createVerilogCircuit.The error message could be more specific by including the actual error details.
- console.error('Failed to create Verilog circuit:', error); - showError('Failed to create Verilog circuit. Please try again.'); + const errorMessage = error instanceof Error ? error.message : 'Unknown error'; + console.error('Failed to create Verilog circuit:', errorMessage); + showError(`Failed to create Verilog circuit: ${errorMessage}`);
237-249: Move getNextPosition outside processSubCircuitDevice.The position calculation logic should be moved to a module-level utility function to improve reusability.
+const GRID_SIZE = 100; +let lastX = 0; +let lastY = 0; + +function getNextPosition(): { x: number, y: number } { + const position = { x: lastX, y: lastY }; + lastX += GRID_SIZE; + if (lastX > 1500) { // max width + lastX = 0; + lastY += GRID_SIZE; + } + return position; +} function processSubCircuitDevice( device: string, deviceData: YosysDevice, circuitDevices: { [key: string]: VerilogSubCircuit | unknown }, subCircuitScope: { [key: string]: string } ): void { - const GRID_SIZE = 100; - let lastX = 0; - let lastY = 0; - - function getNextPosition(): { x: number, y: number } { - const position = { x: lastX, y: lastY }; - lastX += GRID_SIZE; - if (lastX > 1500) { // max width - lastX = 0; - lastY += GRID_SIZE; - } - return position; - }
357-359: Replace setTimeout with requestAnimationFrame.Using
setTimeoutfor UI updates is not recommended. UserequestAnimationFramefor better performance.- setTimeout(function () { - editor.refresh(); - }, 1); + requestAnimationFrame(() => { + editor.refresh(); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/simulator/src/Verilog2CV.ts(1 hunks)src/simulator/src/types/verilog.types.ts(1 hunks)
🔇 Additional comments (2)
src/simulator/src/types/verilog.types.ts (1)
33-40: LGTM! Node interface looks good.The interface has been improved with optional verilogLabel fields as suggested in past reviews.
src/simulator/src/Verilog2CV.ts (1)
26-31: Remove duplicate CodeMirror addon imports.The same imports appear multiple times in the file.
Fixes #414
To be merged after #443
@niladrix719 @JoshVarga
Summary by CodeRabbit
New Features
Chores