Skip to content

[feat] Add property instrumentation for node changes #1136

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

Closed
benceruleanlu wants to merge 15 commits into from
Closed

[feat] Add property instrumentation for node changes #1136

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
3858914
[feat] Add property instrumentation for node changes
benceruleanlu Jul 20, 2025
d982a40
Update test snapshots (?)
benceruleanlu Jul 20, 2025
4671ea3
Add basic unit tests
benceruleanlu Jul 20, 2025
838a185
Change to generic approach
benceruleanlu Jul 20, 2025
ae7577e
Improve property instrumentation to preserve existing object state
benceruleanlu Jul 21, 2025
ddd95da
Move instr to constr
benceruleanlu Jul 22, 2025
c0fe0fa
Refactor property instrumentation: consolidate into LGraphNodePropert…
benceruleanlu Jul 22, 2025
727e926
Fix type errors
benceruleanlu Jul 22, 2025
3524e66
nit
benceruleanlu Jul 22, 2025
8108ecc
Remove default property
benceruleanlu Jul 22, 2025
f35fbb3
nit
benceruleanlu Jul 22, 2025
b59bf7e
Move test file
benceruleanlu Jul 23, 2025
caee207
Rename propertyManager to changeTracker
benceruleanlu Jul 23, 2025
baa4c83
Update test path
benceruleanlu Jul 23, 2025
c9b4861
Revert shotgun tsc changes
benceruleanlu Jul 23, 2025
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
Prev Previous commit
Next Next commit
Improve property instrumentation to preserve existing object state 
Distinguish between existing and non-existing properties when instrumenting nodes. Properties that already exist maintain their enumerable status and values, while tracked properties that don't exist yet are created as non-enumerable with undefined initial values. This prevents unintended initialization side effects while maintaining proper change tracking.

🤖 Generated with [opencode](https://opencode.ai)

Co-Authored-By: opencode <[email protected]>
  • Loading branch information
@benceruleanlu
benceruleanlu and opencode committed Jul 23, 2025
commit ae7577ed4faa6be8a73ccd0dc3f2271d6eae8928
41 changes: 30 additions & 11 deletions src/nodePropertyInstrumentation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ interface PropertyConfig {
*/
const DEFAULT_TRACKED_PROPERTIES: PropertyConfig[] = [
{ path: "title", type: "string" },
{ path: "flags.collapsed", defaultValue: false, type: "boolean" },
{ path: "flags.collapsed", type: "boolean" },
]

/**
Expand Down Expand Up @@ -99,18 +99,37 @@ export function instrumentNodeProperties(
propertyName = parts.at(-1)!
}

// Get initial value
const hasProperty = Object.prototype.hasOwnProperty.call(targetObject, propertyName)
const currentValue = targetObject[propertyName]
const initialValue = currentValue !== undefined
? currentValue
: config.defaultValue

// Create and apply the property descriptor
Object.defineProperty(
targetObject,
propertyName,
createInstrumentedProperty(node, config.path, initialValue),
)
if (!hasProperty) {
let value: any = undefined
Object.defineProperty(targetObject, propertyName, {
get() {
return value
},
set(newValue: any) {
const oldValue = value
value = newValue
if (oldValue !== newValue && node.graph) {
node.graph.trigger("node:property:changed", {
nodeId: node.id,
property: config.path,
oldValue,
newValue,
})
}
},
enumerable: false,
configurable: true,
})
} else {
Object.defineProperty(
targetObject,
propertyName,
createInstrumentedProperty(node, config.path, currentValue),
)
}
}
}

Expand Down
12 changes: 3 additions & 9 deletions test/__snapshots__/ConfigureGraph.test.ts.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,9 +68,7 @@ LGraph {
"console": undefined,
"exec_version": undefined,
"execute_triggered": undefined,
"flags": {
"collapsed": false,
},
"flags": {},
"freeWidgetSpace": undefined,
"gotFocusAt": undefined,
"graph": [Circular],
Expand Down Expand Up @@ -138,9 +136,7 @@ LGraph {
"console": undefined,
"exec_version": undefined,
"execute_triggered": undefined,
"flags": {
"collapsed": false,
},
"flags": {},
"freeWidgetSpace": undefined,
"gotFocusAt": undefined,
"graph": [Circular],
Expand Down Expand Up @@ -209,9 +205,7 @@ LGraph {
"console": undefined,
"exec_version": undefined,
"execute_triggered": undefined,
"flags": {
"collapsed": false,
},
"flags": {},
"freeWidgetSpace": undefined,
"gotFocusAt": undefined,
"graph": [Circular],
Expand Down
12 changes: 3 additions & 9 deletions test/__snapshots__/LGraph_constructor.test.ts.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,9 +68,7 @@ LGraph {
"console": undefined,
"exec_version": undefined,
"execute_triggered": undefined,
"flags": {
"collapsed": false,
},
"flags": {},
"freeWidgetSpace": undefined,
"gotFocusAt": undefined,
"graph": [Circular],
Expand Down Expand Up @@ -138,9 +136,7 @@ LGraph {
"console": undefined,
"exec_version": undefined,
"execute_triggered": undefined,
"flags": {
"collapsed": false,
},
"flags": {},
"freeWidgetSpace": undefined,
"gotFocusAt": undefined,
"graph": [Circular],
Expand Down Expand Up @@ -209,9 +205,7 @@ LGraph {
"console": undefined,
"exec_version": undefined,
"execute_triggered": undefined,
"flags": {
"collapsed": false,
},
"flags": {},
"freeWidgetSpace": undefined,
"gotFocusAt": undefined,
"graph": [Circular],
Expand Down
21 changes: 9 additions & 12 deletions test/nodePropertyInstrumentation.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ describe("Node Property Instrumentation", () => {
expect(mockTrigger).toHaveBeenCalledWith("node:property:changed", {
nodeId: node!.id,
property: "flags.collapsed",
oldValue: false,
oldValue: undefined,
newValue: true,
})
})
Expand All @@ -63,9 +63,6 @@ describe("Node Property Instrumentation", () => {
const currentTitle = node!.title
node!.title = currentTitle

// Set same collapsed state
node!.flags.collapsed = false

expect(mockTrigger).not.toHaveBeenCalled()
})

Expand Down Expand Up @@ -95,7 +92,7 @@ describe("Node Property Instrumentation", () => {
expect(mockTrigger).toHaveBeenNthCalledWith(2, "node:property:changed", {
nodeId: node!.id,
property: "flags.collapsed",
oldValue: false,
oldValue: undefined,
newValue: true,
})
expect(mockTrigger).toHaveBeenNthCalledWith(3, "node:property:changed", {
Expand Down Expand Up @@ -144,7 +141,7 @@ describe("Node Property Instrumentation", () => {

// Verify properties are enumerable
expect(Object.keys(node!)).toContain("title")
expect(Object.keys(node!.flags)).toContain("collapsed")
expect(node!.flags.collapsed).toBe(true)
})

describe("Generic property tracking", () => {
Expand Down Expand Up @@ -182,15 +179,15 @@ describe("Node Property Instrumentation", () => {
expect(mockTrigger).toHaveBeenCalledWith("node:property:changed", {
nodeId: node!.id,
property: "priority",
oldValue: 0,
oldValue: undefined,
newValue: 5,
})

node!.flags.readonly = true
expect(mockTrigger).toHaveBeenCalledWith("node:property:changed", {
nodeId: node!.id,
property: "flags.readonly",
oldValue: false,
oldValue: undefined,
newValue: true,
})
})
Expand Down Expand Up @@ -219,7 +216,7 @@ describe("Node Property Instrumentation", () => {
expect(mockTrigger).toHaveBeenCalledWith("node:property:changed", {
nodeId: node!.id,
property: "config.ui.theme.color",
oldValue: "light",
oldValue: undefined,
newValue: "dark",
})

Expand Down Expand Up @@ -262,19 +259,19 @@ describe("Node Property Instrumentation", () => {
expect(mockTrigger).toHaveBeenNthCalledWith(1, "node:property:changed", {
nodeId: node!.id,
property: "category",
oldValue: "default",
oldValue: undefined,
newValue: "advanced",
})
expect(mockTrigger).toHaveBeenNthCalledWith(2, "node:property:changed", {
nodeId: node!.id,
property: "metadata",
oldValue: {},
oldValue: undefined,
newValue: { version: 1, author: "test" },
})
expect(mockTrigger).toHaveBeenNthCalledWith(3, "node:property:changed", {
nodeId: node!.id,
property: "flags.pinned",
oldValue: false,
oldValue: undefined,
newValue: true,
})
})
Expand Down