refactor(simulator): clean up EventQueue implementation and add TS types #678
Conversation
✅ Deploy Preview for circuitverse ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughAdds a cross-platform Node.js multi-version build script ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
build.js (2)
8-9: Defensively validateversion.jsonshape for clearer failures
JSON.parse(fs.readFileSync("version.json", "utf8")).map((v) => v.version)assumesversion.jsonis an array of objects with aversionfield; any deviation will throw with a generic JSON/TypeError. Consider validating the parsed value (e.g., check it’s an array and that each element has a stringversion) and throwing a more descriptive error if the structure is unexpected or the file is missing.
11-22: Optional: capture all failing versions instead of exiting on firstRight now, a failure for one version exits the process immediately. For multi-version builds, you might get better feedback by collecting all failing versions, logging them in a summary, and then exiting with code 1. That said, the current “fail fast” behavior is perfectly acceptable if you prefer simpler control flow.
src/simulator/src/eventQueue.ts (6)
7-14: QueueObject exposes queue internals directly
QueueObjectrequires every event to carryqueueProperties(inQueue, time, index), which leaks internal queue bookkeeping into the domain objects. If you ever want to swap out this implementation (e.g., heap-based or different indexing), this coupling may make refactors harder. Consider, longer term, either wrapping user objects with an internal node type or makingqueuePropertiesinternal toEventQueueinstead of part of the public interface.
29-55: Makedelaytruly optional in theaddsignature
add(obj: QueueObject, delay: number)calculateseventTimeasthis.time + (delay ?? obj.propagationDelay), and the comment mentions using the object’s defaultpropagationDelay. That implies callers should be able to omitdelayentirely, but the current type requires it. To align the type with the behavior and docs, consider:- add(obj: QueueObject, delay: number) { + add(obj: QueueObject, delay?: number) {(or
delay: number | undefined) so callers can cleanly rely on the default delay without passingundefinedexplicitly.
57-71: Consider guarding against double-enqueue inaddImmediate
addImmediateinserts an object at the current time without checkingobj.queueProperties.inQueue, whereasaddexplicitly short‑circuits toreorderwhen the object is already queued. If there’s any chance the sameQueueObjectinstance is passed toaddImmediatewhile still in the queue, it will be enqueued twice. If that’s not an invariant you want to rely on, mirroring theinQueuecheck fromadd(and either rejecting or reordering) would make this safer.
72-106: Ordering helpers are correct but comments are slightly confusing
shiftUp/shiftDownmaintain the array in descendingtimeorder whilepop()removes fromfrontIndex - 1, so the lowest‑time event is returned first. That’s a valid design, but the comments (“Move object earlier in queue if its time increased”, “later if its time decreased”) can be hard to reconcile with the fact that lower array indices correspond to later times in simulation terms. Consider rewording these docstrings to explicitly mention “front/back of the internal array” and that the queue is stored in descending time order, to avoid confusion for future maintainers.
122-137: Docstring forpopdoesn’t match actual orderingThe comment says “Pop the next event (highest time)”, but because the queue is stored in descending time order and you decrement
frontIndexand read fromthis.queue[this.frontIndex],pop()actually returns the lowestqueueProperties.time(earliest event), which is what the simulator likely expects. I’d update the comment to say “lowest time” or “earliest scheduled event” to match the real behavior and avoid incorrect assumptions during future refactors.
139-155: Reset logic is correct; consider optionally clearing indices
reset()correctly clearsinQueue, resetstime, and resetsfrontIndex, so the queue’s observable state is clean. You might optionally also clearqueueProperties.index(and perhapstime) for the active range to avoid stale metadata on objects that have been removed, but functionally it’s safe as-is becauseinQueuegates reordering andaddoverwrites these fields when re-enqueuing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
build.js(1 hunks)package.json(1 hunks)src/simulator/src/eventQueue.ts(1 hunks)
🔇 Additional comments (1)
package.json (1)
7-7: Build script switch tonode build.jslooks consistent with new flowUpdating the
buildscript to callnode build.jsaligns package.json with the new Node-based, cross-platform build entrypoint. Please just confirm CI and any manual build docs no longer referencebuild.shand that Node is available in all build environments.
…e + build script fix
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/simulator/src/eventQueue.ts (2)
18-18: Consider making the time property readonly or private with a getter.The
timeproperty is public and mutable, which means external code could modify it directly and cause inconsistency with the internal heap state. Since time is only updated internally viapop(), consider making it readonly or providing access through a getter method.Apply this diff to make the property readonly:
- time = 0; + readonly time = 0;Or create a private field with a getter:
- time = 0; + private _time = 0; + get time() { return this._time; }Then update Line 69 and Line 86 to use
this._timeinstead ofthis.time.
29-34: Consider optimizing heap repositioning on time update.When updating an existing queue item's time, the code calls both
heapifyUp()andheapifyDown(). While this is correct (since we don't know if the new time is greater or smaller), it's slightly inefficient as at most one direction is needed.Consider storing the old time and only calling the appropriate heapify:
if (obj.queueProperties.inQueue) { + const oldTime = obj.queueProperties.time; + obj.queueProperties.time = eventTime; // Update time + reposition - this.heapifyUp(obj.queueProperties.index); - this.heapifyDown(obj.queueProperties.index); + if (eventTime < oldTime) { + this.heapifyUp(obj.queueProperties.index); + } else if (eventTime > oldTime) { + this.heapifyDown(obj.queueProperties.index); + } return; }Note: Move the time assignment from Line 27 into this block if implementing this optimization.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
build.sh(1 hunks)src/simulator/src/eventQueue.ts(1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.11.0)
build.sh
[error] 1-1: Was this intended as a comment? Use # in sh.
(SC1127)
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
[error] 1-1: '(' is invalid here. Did you forget to escape it?
(SC1036)
[error] 1-1: Parsing stopped here. Invalid use of parentheses?
(SC1088)
🔇 Additional comments (6)
src/simulator/src/eventQueue.ts (6)
49-51: LGTM!Clean delegation to
add()with zero delay. The implementation is simple and correct.
56-71: LGTM! Standard heap pop with correct time management.The implementation correctly:
- Checks for empty queue before proceeding
- Swaps the top with the last element and heapifies down
- Updates the global time to the popped event's time
- Clears the
inQueueflagThe non-null assertion on Line 60 is safe since the
isEmpty()check on Line 57 guarantees at least one element exists.
76-78: LGTM!Simple and correct isEmpty implementation.
83-87: LGTM! Complete state cleanup.The reset correctly:
- Clears
inQueueflags to prevent stale references- Resets the heap array
- Resets time to initial state
93-100: LGTM! Correct swap with index tracking.The swap correctly exchanges heap positions and updates the
indexproperty on both objects to maintain consistency.
102-132: LGTM! Correct binary min-heap operations.Both
heapifyUp()andheapifyDown()are correctly implemented:
- heapifyUp: Properly bubbles elements up while they're smaller than their parent, using the correct parent index formula
Math.floor((i - 1) / 2).- heapifyDown: Correctly sinks elements down by comparing with both children, finding the smallest, and swapping. The bounds checks prevent out-of-range access.
Both methods properly use
swap()to maintain index consistency.
build.sh
Outdated
| // build.js (Windows + cross-platform equivalent of build.sh) | ||
|
|
||
| const { execSync } = require("child_process"); | ||
| const fs = require("fs"); | ||
|
|
||
| versions=($(jq -r '.[].version' version.json)) | ||
| console.log("Reading version.json..."); | ||
|
|
||
| const versions = JSON.parse(fs.readFileSync("version.json", "utf8")) | ||
| .map((v) => v.version); | ||
| for version in "${versions[@]}"; do | ||
| echo "Building for version: $version" | ||
|
|
||
| npx vite build --config vite.config."$version".ts | ||
|
|
||
| #Build status | ||
| if [ $? -ne 0 ]; then | ||
| echo "Build failed for version: $version" | ||
| exit 1 | ||
| fi | ||
| done | ||
| for (const version of versions) { | ||
| console.log(`\nBuilding for version: ${version}`); | ||
| echo "All builds completed successfully" | ||
| try { | ||
| execSync(`npx vite build --config vite.config.${version}.ts`, { | ||
| stdio: "inherit", | ||
| }); | ||
| } catch (err) { | ||
| console.error(`❌ Build failed for version: ${version}`); | ||
| process.exit(1); | ||
| } | ||
| } | ||
| console.log("\n✅ All builds completed successfully!"); |
There was a problem hiding this comment.
CRITICAL: File extension mismatch - JavaScript file named as shell script.
This file contains Node.js JavaScript code but is named build.sh. The .sh extension indicates a shell script, which will cause execution failures and confusion. The inline comment on Line 1 confirms this should be build.js, and the static analysis errors from shellcheck confirm it cannot parse this as shell script.
Please rename this file from build.sh to build.js to match its actual content and the build command referenced in package.json.
🧰 Tools
🪛 Shellcheck (0.11.0)
[error] 1-1: Was this intended as a comment? Use # in sh.
(SC1127)
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
[error] 1-1: '(' is invalid here. Did you forget to escape it?
(SC1036)
[error] 1-1: Parsing stopped here. Invalid use of parentheses?
(SC1088)
🤖 Prompt for AI Agents
In build.sh around lines 1 to 24, the file contains Node.js code but is named
with a .sh extension; rename the file to build.js and update any references
(package.json scripts, CI workflows, docs, or other tooling) that call build.sh
to point to build.js (and invoke it with node or via an npm script) so the
runtime matches the file contents and shellcheck/static analysis failures are
resolved.
1 participant
Fixes #<issue_number_if_any>
Summary of Changes
This PR refactors the EventQueue module inside the simulator to improve clarity, maintainability, performance predictability, and TypeScript safety — while preserving all existing simulator behavior.
✅ 1. Code Cleanup & Structure
Removed redundant paths and duplicate logic.
Simplified event insertion, reordering, and popping.
Modernized function names and aligned method responsibilities.
Clearer internal semantics compared to the previous tightly coupled design.
✅ 2. Strong TypeScript Typing
Added explicit interfaces for queue items and internal metadata.
Removed multiple
anyusages.Stronger type guarantees when scheduling, updating, or popping events.
Improves safety for future simulator features.
✅ 3. Helper Utilities
Introduced internal helper functions to reduce repetition.
Clearer separation between:
queue manipulation, and
simulation event properties.
🧪 4. Added Tests
Introduced initial Vitest suite to validate core queue behavior.
Ensures:
correct ordering,
correct reordering on time update,
proper handling of duplicates/in-queue updates.
⏱️ Time & Space Complexity Analysis
Before Refactor
Key Performance Improvement:
Time updates now use directional repositioning — improving efficiency and reducing unnecessary operations.
Why This Refactor Was Needed
The earlier implementation mixed concerns and lacked clear typing.
Duplicate logic made changes risky and debugging harder.
Performance costs were inconsistent due to linear operations.
This refactor establishes a modern, type-safe, maintainable foundation for future simulator engine enhancements.
Screenshots
No UI changes — this PR modifies internal simulator logic only.
Note
Please ensure Allow edits from maintainers is enabled.