fix: improve node deletion logic reliability#946
Open
subhamkumarr wants to merge 5 commits intoCircuitVerse:mainfrom
Open
fix: improve node deletion logic reliability#946subhamkumarr wants to merge 5 commits intoCircuitVerse:mainfrom
subhamkumarr wants to merge 5 commits intoCircuitVerse:mainfrom
Conversation
- Add debouncing (300ms) for changeClockTime and changeClockEnable - Skip heavy canvas updates (scheduleUpdate, updateCanvasSet, wireToBeCheckedSet) for clock properties - Clock changes now only update clock interval/state, not entire canvas - Fixes continuous resetup() calls that caused crashes - Applied to both v0 and v1 versions Fixes: Clock timer and clock enable crash issue in Project Properties Panel
- Remove direct prop mutation (props.propertyValue = value) - Props are readonly in Vue 3, causing console warnings - Input value is already updated and change event is triggered - Fixes Vue warnings: 'Set operation on key propertyValue failed: target is readonly'
- Remove removeBugNodes() function that was a temporary workaround - Replace with targeted cleanup that only removes orphaned nodes - Fix root cause: input/output nodes with scope.root parent are now properly identified and removed - Use reverse iteration for safer array modification - Applied to both v0 and v1 versions The new approach: - Only removes input/output nodes (type 0, 1) that have scope.root as parent - These nodes should have been replaced with nodes having proper CircuitElement parents - More efficient and clearer than the original loop-based approach Fixes CircuitVerse#943
- Fix node deletion from nodeList to use correct parent reference - Remove from this.parent.nodeList (where node was added) instead of always root.nodeList - Add validation to ensure nodeList exists and is an array before filtering - Use indexOf + splice for more reliable removal with fallback to filter - Also check and remove from root.nodeList for safety/backward compatibility - Remove uncertain comment 'Hope this works! - Can cause bugs' The previous code tried to remove from root.nodeList, but nodes are actually added to this.parent.nodeList in the constructor. This mismatch could cause nodes to not be properly removed, leading to memory leaks or orphaned references. Fixes #[ISSUE_NUMBER]
✅ Deploy Preview for circuitverse ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
WalkthroughUpdates across simulator and UI: Node.delete() now removes nodes from the node’s parent.nodeList with a safe fallback and still cleans root.nodeList; ux.js adds debounce timers and special-case handling to debounce clock time and clock enable property updates; load.js removes the removeBugNodes helper and instead explicitly scans and deletes orphaned input/output nodes after loading; InputGroups.vue no longer mutates the public 🚥 Pre-merge checks | ✅ 3 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@v1/src/simulator/src/data/load.js`:
- Around line 87-91: Update the misleading JSDoc/deprecation comment to reflect
current behavior: remove or replace the phrase "only intermediate nodes are
loaded before modules" and state that all nodes are loaded first (via
data.allNodes.map((x) => loadNode(x, scope))) and orphaned nodes are cleaned up;
reference loadNode and data.allNodes.map to clarify where this loading happens
and keep the category/data deprecation note accurate.
🧹 Nitpick comments (3)
src/simulator/src/node.js (1)
940-961: Redundant double-removal whenthis.parent === scope.root.When a node's parent is
scope.root(e.g., intermediate nodes converted viaconverToIntermediate), lines 943–950 already remove it fromthis.parent.nodeList(which isroot.nodeList). Then lines 955–961 attempt to remove the same node fromroot.nodeListagain. The secondindexOfwill return-1, so it's safe, but the duplication is unnecessary.Also, the
elsefallback at line 948–950 is effectively dead code —Array.prototype.indexOfalways works correctly for object identity checks in JS. The comment "if indexOf doesn't work" is misleading.Consider simplifying:
♻️ Suggested simplification
- // Remove node from parent's nodeList (where it was originally added) - // Input/output nodes (type 0, 1) are added to parent.nodeList in constructor (line 167) - // We need to remove from the correct parent's nodeList, not always from root.nodeList - if (this.parent && this.parent.nodeList && Array.isArray(this.parent.nodeList)) { - const index = this.parent.nodeList.indexOf(this) - if (index !== -1) { - this.parent.nodeList.splice(index, 1) - } else { - // Fallback: use filter if indexOf doesn't work (shouldn't happen, but safer) - this.parent.nodeList = this.parent.nodeList.filter(x => x !== this) - } - } - - // Also remove from root.nodeList if present (for safety and backward compatibility) - // Some edge cases might have nodes in root.nodeList - if (this.parent && this.parent.scope && this.parent.scope.root && - this.parent.scope.root.nodeList && Array.isArray(this.parent.scope.root.nodeList)) { - const rootIndex = this.parent.scope.root.nodeList.indexOf(this) - if (rootIndex !== -1) { - this.parent.scope.root.nodeList.splice(rootIndex, 1) - } - } + // Remove node from parent's nodeList (where it was added in constructor) + if (this.parent.nodeList && Array.isArray(this.parent.nodeList)) { + const index = this.parent.nodeList.indexOf(this) + if (index !== -1) { + this.parent.nodeList.splice(index, 1) + } + } + + // Also remove from root.nodeList if parent differs from root (backward compatibility) + const root = this.parent.scope.root + if (root !== this.parent && root.nodeList && Array.isArray(root.nodeList)) { + const rootIndex = root.nodeList.indexOf(this) + if (rootIndex !== -1) { + root.nodeList.splice(rootIndex, 1) + } + }v1/src/simulator/src/data/load.js (1)
125-149: Orphan cleanup logic is correct but relies on a subtle ordering invariant.The reverse-order index collection (line 132) combined with forward iteration of
nodesToRemove(line 145) ensures thatnode.delete()— which usesfilterto shrinkallNodes— doesn't invalidate subsequent indices. This works because higher indices are processed first.This is correct but fragile. If anyone later changes the iteration order or the way
delete()modifiesallNodes, it could silently break. A brief inline comment would help future maintainers.📝 Suggested clarifying comment
// Remove orphaned nodes in reverse order to maintain indices + // (nodesToRemove is already sorted descending because the scan loop runs from high to low) for (const index of nodesToRemove) { scope.allNodes[index].delete() }v1/src/simulator/src/ux.js (1)
191-216: DebouncedchangeClockTimehandler — verifythiscontext survives the delay.The arrow function inside
setTimeoutcorrectly capturesthis(the DOM element) from the enclosing jQuery handler. Readingthis.valueat timeout-fire time returns the latest input value, which is ideal for debounce. This looks correct.One minor concern: if the input element is removed from the DOM during the debounce window (e.g., user switches circuits), the deferred call will still fire against stale references. Consider guarding:
🛡️ Optional guard
clockTimeDebounceTimer = setTimeout(() => { + if (!document.contains(this)) { + clockTimeDebounceTimer = null + return + } checkValidBitWidth()
Comment on lines
+87
to
91
| * @deprecated This function has been removed as the root cause has been fixed. | ||
| * Nodes are now loaded correctly and orphaned nodes are cleaned up properly. | ||
| * @category data | ||
| */ | ||
| function removeBugNodes(scope = globalScope) { | ||
| 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() | ||
| } | ||
| if (scope.allNodes.length !== x) { | ||
| i = 0 | ||
| x = scope.allNodes.length | ||
| } | ||
| } | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
Misleading deprecation comment.
Line 88 states "only intermediate nodes are loaded before modules" but line 103–104 loads all nodes first via data.allNodes.map((x) => loadNode(x, scope)). The comment should accurately describe the current behavior.
📝 Suggested fix
/**
- * `@deprecated` This function has been removed as the root cause has been fixed.
- * Nodes are now loaded correctly: only intermediate nodes are loaded before modules,
- * and input/output nodes are created as part of their circuit elements.
+ * `@deprecated` removeBugNodes has been removed. Orphaned input/output nodes
+ * are now cleaned up inline in loadScope after module loading.
* `@category` data
*/🤖 Prompt for AI Agents
In `@v1/src/simulator/src/data/load.js` around lines 87 - 91, Update the
misleading JSDoc/deprecation comment to reflect current behavior: remove or
replace the phrase "only intermediate nodes are loaded before modules" and state
that all nodes are loaded first (via data.allNodes.map((x) => loadNode(x,
scope))) and orphaned nodes are cleaned up; reference loadNode and
data.allNodes.map to clarify where this loading happens and keep the
category/data deprecation note accurate.
Member
|
Could you add a real world impact of this bug with example ? |
- Add detailed JSDoc comments explaining consequences of failed node deletion - Document memory leaks, simulation errors, and UI glitches - Include concrete example scenario: deleting gate input node that remains in parent.nodeList - Explain how this causes simulation failures, console errors, and circuit unresponsiveness - Clarify the root cause: mismatch between where nodes are added vs where they're removed Addresses reviewer feedback requesting real-world impact examples
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/simulator/src/node.js (3)
957-965: Redundantfilterfallback —indexOfalways works for identity checks.
Array.prototype.indexOfuses strict equality (===), which is the correct way to find object references in an array. It will never "not work" for this use case. Theelsebranch (line 962-963) is dead code.♻️ Simplify to just indexOf + splice
if (this.parent && this.parent.nodeList && Array.isArray(this.parent.nodeList)) { const index = this.parent.nodeList.indexOf(this) if (index !== -1) { this.parent.nodeList.splice(index, 1) - } else { - // Fallback: use filter if indexOf doesn't work (shouldn't happen, but safer) - this.parent.nodeList = this.parent.nodeList.filter(x => x !== this) } }
951-975: Double removal fromparent.nodeListwhen called viareplace().The
replace()function (line 45) already filtersparent.nodeListto remove the node before callingdelete(). Thendelete()attempts the same removal. This is harmless (indexOf returns -1, splice is skipped), but worth being aware of for clarity.Additionally, when
this.parent === this.parent.scope.root(e.g. intermediate nodes whose parent is root), both the parent-block and root-block operate on the samenodeListarray. The second lookup will simply find nothing since the first already removed it — no bug, but the comment "Some edge cases might have nodes in root.nodeList" is misleading for this scenario.
933-943: JSDoc comment is overly verbose for an implementation detail.This 10-line impact narrative reads more like a commit message or issue description than a code comment. Consider condensing to 2-3 lines explaining what the code does and why, and leave the detailed impact analysis in the PR/issue.
Author
@tachyons I've added real-world impact examples in the JSDoc comments for the delete() method. The documentation now includes:
The changes are in the JSDoc comments at lines 934-943 in src/simulator/src/node.js (and the v1 version). This explains the root cause and the consequences if node deletion fails. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The node deletion logic had an uncertain comment
// Hope this works! - Can cause bugsindicating potential issues. The code was trying to remove nodes fromroot.nodeList, but nodes are actually added tothis.parent.nodeListin the constructor, creating a mismatch.Solution
nodeListnodeListexists and is an arrayindexOf + splicefor reliable removal with filter fallbackroot.nodeListfor safety and backward compatibilityRoot Cause
this.parent.nodeListin constructor (line 167)this.parent.scope.root.nodeList(line 940)Changes Made
Before:
this.parent.scope.root.nodeList = this.parent.scope.root.nodeList.filter(x => x !== this) // Hope this works! - Can cause bugs
After:
Also check root.nodeList as safety measure
Files Changed
src/simulator/src/node.jsv1/src/simulator/src/node.jsTesting
✅ Applied fix to both v0 and v1 versions
✅ No linting errors
✅ More reliable node deletion with proper validation
Fixes #945
Summary by CodeRabbit