Skip to content

fix: improve node deletion logic reliability #946

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
subhamkumarr wants to merge 5 commits into
base: main
Choose a base branch
from
Open

fix: improve node deletion logic reliability #946

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a79a5d4
fix: prevent clock timer and clock enable crash with debouncing
subhamkumarr Feb 15, 2026
190d3da
fix: remove prop mutation in InputGroups component
subhamkumarr Feb 15, 2026
3c0bf73
fix: replace temporary removeBugNodes function with proper solution
subhamkumarr Feb 15, 2026
d196d59
fix: improve node deletion logic reliability
subhamkumarr Feb 15, 2026
f3b651f
docs: add real-world impact examples for node deletion bug
subhamkumarr Feb 16, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/components/Panels/PropertiesPanel/ModuleProperty/ProjectProperty/ProjectProperty.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,4 +259,5 @@ type SimulatorStateType = {
}
</style>

<!-- TODO: add type to scopeList and fix clock timer and clock enable and lite mode (crashing with continuous resetup() calls) -->
<!-- TODO: add type to scopeList -->
<!-- Note: Clock timer and clock enable crash issue fixed with debouncing in ux.js -->
2 changes: 0 additions & 2 deletions src/components/Panels/Shared/InputGroups.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,6 @@ function increaseValue() {
step = isNaN(step) ? 1 : step
if (value + step <= props.valueMax) value = value + step
else return
props.propertyValue = value
ele.value = value
// manually triggering on change event
const e = new Event('change')
Expand All @@ -72,7 +71,6 @@ function decreaseValue() {
step = isNaN(step) ? 1 : step
if (value - step >= props.valueMin) value = value - step
else return
props.propertyValue = value
ele.value = value
// manually triggering on change event
const e = new Event('change')
Expand Down
48 changes: 29 additions & 19 deletions src/simulator/src/data/load.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,25 +84,11 @@ function loadModule(data, scope) {
}

/**
* This function shouldn't ideally exist. But temporary fix
* for some issues while loading nodes.
* @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.
* @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
}
}
}

/**
* Function to load a full circuit
Expand All @@ -114,7 +100,7 @@ export function loadScope(scope, data) {
const ML = moduleList.slice() // Module List copy
scope.restrictedCircuitElementsUsed = data.restrictedCircuitElementsUsed

// Load all nodes
// Load all nodes first (required for replace() to work with correct indices)
data.allNodes.map((x) => loadNode(x, scope))

// Make all connections
Expand All @@ -137,11 +123,35 @@ export function loadScope(scope, data) {
}
}
}

// Clean up orphaned nodes that weren't properly replaced
// These are input/output nodes (type 0, 1) that still have scope.root as parent
// instead of their correct CircuitElement parent. This happens when replace() fails
// or when a node's circuit element wasn't loaded.
// Input/output nodes should belong to circuit elements, not scope.root
// Intermediate nodes (type 2) correctly belong to scope.root
const nodesToRemove = []
for (let i = scope.allNodes.length - 1; i >= 0; i--) {
const node = scope.allNodes[i]
// Check if it's an input/output node (type 0 or 1) with scope.root as parent
// These should have been replaced with nodes that have proper CircuitElement parents
if (
node.type !== 2 && // Not an intermediate node (input/output nodes)
node.parent === scope.root // Has scope.root as parent (wrong - should have CircuitElement parent)
) {
// This node should have been replaced but wasn't - it's orphaned
nodesToRemove.push(i)
}
}
// Remove orphaned nodes in reverse order to maintain indices
for (const index of nodesToRemove) {
scope.allNodes[index].delete()
}

// Update wires according
scope.wires.map((x) => {
x.updateData(scope)
})
removeBugNodes(scope) // To be deprecated

// If Verilog Circuit Metadata exists, then restore
if (data.verilogMetadata) {
Expand Down
37 changes: 36 additions & 1 deletion src/simulator/src/node.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -930,14 +930,49 @@ export default class Node {

/**
* function delete a node
*
* Real-world impact if deletion fails:
* - Memory leaks: Orphaned nodes remain in nodeList, preventing garbage collection
* - Simulation errors: Deleted nodes still referenced, causing "node not found" errors
* - UI glitches: Deleted nodes may still appear in properties panel or cause crashes
* - Example scenario: User deletes a gate's input node, but it remains in parent.nodeList.
* Later, when the circuit simulates, it tries to access the deleted node, causing:
* * Simulation to fail or produce incorrect results
* * Browser console errors
* * Circuit becoming unresponsive
* * Need to reload the page to fix
*/
delete() {
updateSimulationSet(true)
this.deleted = true
this.parent.scope.allNodes = this.parent.scope.allNodes.filter(x => x !== this)
this.parent.scope.nodes = this.parent.scope.nodes.filter(x => x !== this)

this.parent.scope.root.nodeList = this.parent.scope.root.nodeList.filter(x => x !== this) // Hope this works! - Can cause bugs
// 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
//
// Previous bug: Code tried to remove from root.nodeList, but nodes are in parent.nodeList
// This mismatch meant nodes weren't properly removed, causing the issues above
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)
}
}

if (simulationArea.lastSelected == this)
simulationArea.lastSelected = undefined
Expand Down
58 changes: 57 additions & 1 deletion src/simulator/src/ux.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,11 @@ var ctxPos = {
}
let isFullViewActive = false
let prevMobileState = null

// Debounce timers for clock properties to prevent excessive updates
let clockTimeDebounceTimer = null
let clockEnableDebounceTimer = null
const DEBOUNCE_DELAY = 300 // milliseconds
// FUNCTION TO SHOW AND HIDE CONTEXT MENU
function hideContextMenu() {
var el = document.getElementById('contextMenu')
Expand Down Expand Up @@ -184,6 +189,33 @@ function checkValidBitWidth() {
}

export function objectPropertyAttributeUpdate() {
const propertyName = this.name

// Special handling for clock time - debounce and skip heavy updates
// Clock time changes don't require canvas resets, only interval updates
if (propertyName === 'changeClockTime') {
if (clockTimeDebounceTimer) {
clearTimeout(clockTimeDebounceTimer)
}

clockTimeDebounceTimer = setTimeout(() => {
checkValidBitWidth()
let { value } = this
if (this.type === 'number') {
value = parseFloat(value)
}
// Only update clock time, skip canvas updates to prevent crashes
if (simulationArea.lastSelected && simulationArea.lastSelected[propertyName]) {
simulationArea.lastSelected[propertyName](value)
} else {
circuitProperty[propertyName](value)
}
clockTimeDebounceTimer = null
}, DEBOUNCE_DELAY)
return
}

// Default behavior for other properties
checkValidBitWidth()
scheduleUpdate()
updateCanvasSet(true)
Expand All @@ -200,7 +232,31 @@ export function objectPropertyAttributeUpdate() {
}

export function objectPropertyAttributeCheckedUpdate() {
if (this.name === 'toggleLabelInLayoutMode') return // Hack to prevent toggleLabelInLayoutMode from toggling twice
const propertyName = this.name

// Hack to prevent toggleLabelInLayoutMode from toggling twice
if (propertyName === 'toggleLabelInLayoutMode') return

// Special handling for clock enable - debounce and skip heavy updates
// Clock enable changes don't require canvas resets, only state updates
if (propertyName === 'changeClockEnable') {
if (clockEnableDebounceTimer) {
clearTimeout(clockEnableDebounceTimer)
}

clockEnableDebounceTimer = setTimeout(() => {
// Only update clock enable, skip canvas updates to prevent crashes
if (simulationArea.lastSelected && simulationArea.lastSelected[propertyName]) {
simulationArea.lastSelected[propertyName](this.checked)
} else {
circuitProperty[propertyName](this.checked)
}
clockEnableDebounceTimer = null
}, DEBOUNCE_DELAY)
return
}

// Default behavior for other properties
scheduleUpdate()
updateCanvasSet(true)
wireToBeCheckedSet(1)
Expand Down
3 changes: 2 additions & 1 deletion v1/src/components/Panels/PropertiesPanel/ModuleProperty/ProjectProperty/ProjectProperty.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,4 +259,5 @@ type SimulatorStateType = {
}
</style>

<!-- TODO: add type to scopeList and fix clock timer and clock enable and lite mode (crashing with continuous resetup() calls) -->
<!-- TODO: add type to scopeList -->
<!-- Note: Clock timer and clock enable crash issue fixed with debouncing in ux.js -->
2 changes: 0 additions & 2 deletions v1/src/components/Panels/Shared/InputGroups.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,6 @@ function increaseValue() {
step = isNaN(step) ? 1 : step
if (value + step <= props.valueMax) value = value + step
else return
props.propertyValue = value
ele.value = value
// manually triggering on change event
const e = new Event('change')
Expand All @@ -72,7 +71,6 @@ function decreaseValue() {
step = isNaN(step) ? 1 : step
if (value - step >= props.valueMin) value = value - step
else return
props.propertyValue = value
ele.value = value
// manually triggering on change event
const e = new Event('change')
Expand Down
44 changes: 26 additions & 18 deletions v1/src/simulator/src/data/load.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,25 +84,10 @@ function loadModule(data, scope) {
}

/**
* This function shouldn't ideally exist. But temporary fix
* for some issues while loading nodes.
* @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
}
}
}

Comment on lines +87 to 91
Copy link
Contributor

@coderabbitai coderabbitai bot Feb 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

/**
* Function to load a full circuit
Expand Down Expand Up @@ -137,11 +122,34 @@ export function loadScope(scope, data) {
}
}
}
// Clean up orphaned nodes that weren't properly replaced
// These are input/output nodes (type 0, 1) that still have scope.root as parent
// instead of their correct CircuitElement parent. This happens when replace() fails
// or when a node's circuit element wasn't loaded.
// Input/output nodes should belong to circuit elements, not scope.root
// Intermediate nodes (type 2) correctly belong to scope.root
const nodesToRemove = []
for (let i = scope.allNodes.length - 1; i >= 0; i--) {
const node = scope.allNodes[i]
// Check if it's an input/output node (type 0 or 1) with scope.root as parent
// These should have been replaced with nodes that have proper CircuitElement parents
if (
node.type !== 2 && // Not an intermediate node (input/output nodes)
node.parent === scope.root // Has scope.root as parent (wrong - should have CircuitElement parent)
) {
// This node should have been replaced but wasn't - it's orphaned
nodesToRemove.push(i)
}
}
// Remove orphaned nodes in reverse order to maintain indices
for (const index of nodesToRemove) {
scope.allNodes[index].delete()
}

// Update wires according
scope.wires.map((x) => {
x.updateData(scope)
})
removeBugNodes(scope) // To be deprecated

// If Verilog Circuit Metadata exists, then restore
if (data.verilogMetadata) {
Expand Down
37 changes: 36 additions & 1 deletion v1/src/simulator/src/node.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -930,14 +930,49 @@ export default class Node {

/**
* function delete a node
*
* Real-world impact if deletion fails:
* - Memory leaks: Orphaned nodes remain in nodeList, preventing garbage collection
* - Simulation errors: Deleted nodes still referenced, causing "node not found" errors
* - UI glitches: Deleted nodes may still appear in properties panel or cause crashes
* - Example scenario: User deletes a gate's input node, but it remains in parent.nodeList.
* Later, when the circuit simulates, it tries to access the deleted node, causing:
* * Simulation to fail or produce incorrect results
* * Browser console errors
* * Circuit becoming unresponsive
* * Need to reload the page to fix
*/
delete() {
updateSimulationSet(true)
this.deleted = true
this.parent.scope.allNodes = this.parent.scope.allNodes.filter(x => x !== this)
this.parent.scope.nodes = this.parent.scope.nodes.filter(x => x !== this)

this.parent.scope.root.nodeList = this.parent.scope.root.nodeList.filter(x => x !== this) // Hope this works! - Can cause bugs
// 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
//
// Previous bug: Code tried to remove from root.nodeList, but nodes are in parent.nodeList
// This mismatch meant nodes weren't properly removed, causing the issues above
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)
}
}

if (simulationArea.lastSelected == this)
simulationArea.lastSelected = undefined
Expand Down
Loading