Fix/Replace Temporary removeBugNodes Function with Proper Solution#944
Open
subhamkumarr wants to merge 6 commits intoCircuitVerse:mainfrom
Open
Fix/Replace Temporary removeBugNodes Function with Proper Solution#944subhamkumarr wants to merge 6 commits intoCircuitVerse:mainfrom
removeBugNodes Function with Proper Solution#944subhamkumarr wants to merge 6 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
✅ Deploy Preview for circuitverse ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
WalkthroughAdds debounce handling for clock-related properties in ux.js (DEBOUNCE_DELAY ≈ 300ms) to postpone heavy canvas updates for changeClockTime and changeClockEnable; removes the temporary removeBugNodes helper from simulator load logic and instead loads nodes/connections/modules then performs an in-place reverse-order cleanup of orphaned input/output nodes (parent == scope.root) before wiring; updates InputGroups.vue so increase/decrease update only the DOM input and dispatch a change event (no direct assignment to the public prop); adjusts comments in ProjectProperty.vue. Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ 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 |
Author
|
@Nihal4777 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
- Fix deprecation comment to accurately reflect current code behavior - Remove incorrect statement about 'only intermediate nodes loaded before modules' - Add real-world impact examples explaining consequences of orphaned nodes: * Simulation errors from improperly connected nodes * Memory leaks from nodes not being garbage collected * Circuit loading failures or incorrect behavior * Example: Missing connections when loading saved circuits Addresses CodeRabbit review feedback
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/simulator/src/data/load.js`:
- Around line 126-156: The two-pass approach is broken because node.delete()
mutates/reassigns scope.allNodes (via .filter()), making the collected indices
stale; instead, remove orphaned nodes inline during a single reverse iteration:
loop for (let i = scope.allNodes.length - 1; i >= 0; i--) inspect
scope.allNodes[i] (check node.type !== 2 and node.parent === scope.root) and if
true call scope.allNodes[i].delete() immediately (do not push indices to
nodesToRemove). This keeps higher-index deletions from invalidating lower-index
checks and uses the existing reverse iteration to preserve correctness while
referencing scope.allNodes and node.delete().
🧹 Nitpick comments (4)
src/simulator/src/data/load.js (2)
86-91: Remove the dangling@deprecatedJSDoc comment.This JSDoc block isn't attached to any function — the function it documented (
removeBugNodes) has been deleted. A floating JSDoc comment is confusing and serves no purpose. Remove it entirely, or if you want to leave a historical note, use a plain//comment.Proposed fix
-/** - * `@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. - * The removeBugNodes() workaround is no longer needed. - * `@category` data - */
127-139: Comment block is disproportionately long for 8 lines of code.The 13-line comment (including "real-world impact" examples) reads more like a commit message or issue description than an inline code comment. Consider trimming to 2–3 lines explaining the what and why; the broader context belongs in the commit message or PR description.
v1/src/simulator/src/data/load.js (2)
86-90: Same dangling@deprecatedJSDoc as in the v0 file — remove it here too.See the comment on
src/simulator/src/data/load.jslines 86–91.
125-154: Same cleanup logic and verbose comments as in v0 — same feedback applies.The orphan cleanup block is identical to the v0 version. The same concerns apply: verify
node.delete()behavior, simplify to inline deletion during reverse iteration, and trim the comment block.Additionally, maintaining two near-identical copies of this file (v0 and v1) is a maintenance burden. Consider whether these can share a common implementation.
Critical fix: node.delete() reassigns scope.allNodes with filter(), creating a new array. Collecting indices first makes them stale after the first deletion, causing incorrect nodes to be deleted or skipped. Solution: Delete inline during reverse iteration. Since we iterate backwards, deleting higher indices first keeps lower indices valid across filter operations. Fixes CodeRabbit critical issue: index staling bug in two-pass deletion
Reduce verbose comments to essential information only
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/simulator/src/data/load.js (1)
87-91: Orphaned@deprecatedJSDoc block — consider converting to a plain comment.This JSDoc block with
@deprecatedisn't attached to any function declaration, so documentation generators won't associate it with anything meaningful. A regular//comment would be more appropriate here.Suggested change
-/** - * `@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. - * The removeBugNodes() workaround is no longer needed. - * `@category` data - */ +// Note: The removeBugNodes() workaround has been removed. +// Orphaned nodes are now cleaned up inline in loadScope().
Contributor
|
Hii @subhamkumarr Can you please follow pr template. Thankss :)) |
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
removeBugNodes()function was a temporary workaround that "shouldn't ideally exist" (as documented in the code). It was used to clean up incorrectly created nodes after loading, masking the root cause of the issue.Solution
removeBugNodes()function entirelyChanges Made
Before:
removeBugNodes()function with inefficient loop that reset countertype !== 2 && parent.objectType === 'CircuitElement')After:
scope.rootas parent (wrong) are identified and removedTechnical Details
Root Cause:
loadNode()creates all nodes withscope.rootas parentreplace()swaps them, but some nodes never get replaced (orphaned)scope.rootas parent (wrong for input/output nodes)Fix:
scope.rootas parentscope.root, so they're not affectedFiles Changed
src/simulator/src/data/load.jsv1/src/simulator/src/data/load.jsTesting
Fixes #943
Summary by CodeRabbit