fix: address code review issues in promoted widget system

- Use ctx.translate instead of mutating concrete widget y/last_y in drawWidget

- Sync interior node input label in PromotedWidgetSlot.label setter

- Extract syncPromotedWidgets from side-effecting property setter

- Add idempotency guard to registerPromotedWidgetSlots

- Add tests for PromotedWidgetSlot, promotedWidgetRegistration, widgetUtil

Amp-Thread-ID: https://ampcode.com/threads/T-019c537f-f0da-77bc-a401-51c05a4e2ebb
Co-authored-by: Amp <amp@ampcode.com>

src/core/graph/subgraph/PromotedWidgetSlot.test.ts

@@ -221,6 +221,48 @@ describe('PromotedWidgetSlot', () => {
       expect(interiorWidget.label).toBeUndefined()
     })
 
+    it('updates the interior node input label when setting label', () => {
+      const interiorWidget = createMockWidget()
+      const interiorInput = {
+        name: 'seed',
+        widget: { name: 'seed' },
+        label: undefined
+      }
+      const interiorNode = {
+        id: '5',
+        widgets: [interiorWidget],
+        inputs: [interiorInput]
+      } as unknown as LGraphNode
+
+      const subNode = createMockSubgraphNode({ '5': interiorNode })
+      const slot = new PromotedWidgetSlot(subNode, '5', 'seed')
+      slot.label = 'Renamed'
+
+      expect(interiorWidget.label).toBe('Renamed')
+      expect(interiorInput.label).toBe('Renamed')
+    })
+
+    it('clears the interior node input label when label is set to undefined', () => {
+      const interiorWidget = createMockWidget()
+      const interiorInput = {
+        name: 'seed',
+        widget: { name: 'seed' },
+        label: 'Old'
+      }
+      const interiorNode = {
+        id: '5',
+        widgets: [interiorWidget],
+        inputs: [interiorInput]
+      } as unknown as LGraphNode
+
+      const subNode = createMockSubgraphNode({ '5': interiorNode })
+      const slot = new PromotedWidgetSlot(subNode, '5', 'seed')
+      slot.label = undefined
+
+      expect(interiorWidget.label).toBeUndefined()
+      expect(interiorInput.label).toBeUndefined()
+    })
+
     it('does not throw when setting label while disconnected', () => {
       const subNode = createMockSubgraphNode()
       const slot = new PromotedWidgetSlot(subNode, '5', 'seed')
@@ -240,6 +282,23 @@ describe('PromotedWidgetSlot', () => {
       expect(descriptor!.get).toBeDefined()
     })
 
+    it('type accessor returns resolved value even if BaseWidget data property existed', () => {
+      const interiorWidget = createMockWidget({ type: 'slider' })
+      const interiorNode = {
+        id: '5',
+        widgets: [interiorWidget]
+      } as unknown as LGraphNode
+
+      const subNode = createMockSubgraphNode({ '5': interiorNode })
+      const slot = new PromotedWidgetSlot(subNode, '5', 'seed')
+
+      // Verify no own data property for 'type' exists (only accessor)
+      const descriptor = Object.getOwnPropertyDescriptor(slot, 'type')
+      expect(descriptor?.value).toBeUndefined()
+      expect(descriptor?.get).toBeDefined()
+      expect(slot.type).toBe('slider')
+    })
+
     it('defines options as an accessor on the instance, not the prototype', () => {
       const subNode = createMockSubgraphNode()
       const slot = new PromotedWidgetSlot(subNode, '5', 'seed')
@@ -291,7 +350,8 @@ describe('PromotedWidgetSlot', () => {
         fillStyle: '',
         strokeStyle: '',
         font: '',
-        globalAlpha: 1
+        globalAlpha: 1,
+        translate: vi.fn()
       } as unknown as CanvasRenderingContext2D
     }
 
@@ -309,7 +369,42 @@ describe('PromotedWidgetSlot', () => {
       expect(spy).toHaveBeenCalled()
     })
 
-    it('restores concrete widget y/last_y even when drawWidget throws', () => {
+    it('does not mutate concrete widget y/last_y during rendering', () => {
+      const interiorWidget = createMockWidget({ type: 'number' })
+      const interiorNode = {
+        id: '5',
+        widgets: [interiorWidget]
+      } as unknown as LGraphNode
+      const subNode = createMockSubgraphNode({ '5': interiorNode })
+      const slot = new PromotedWidgetSlot(subNode, '5', 'seed')
+      slot.y = 100
+      slot.last_y = 90
+
+      const originalY = 10
+      const originalLastY = 5
+
+      const concreteWidget = {
+        y: originalY,
+        last_y: originalLastY,
+        drawWidget: vi.fn()
+      } as unknown as BaseWidget<IBaseWidget>
+
+      vi.mocked(toConcreteWidget).mockReturnValueOnce(concreteWidget)
+
+      const ctx = createMockCtx()
+      slot.drawWidget(ctx, { width: 200, showText: true })
+
+      // y/last_y should never have been mutated
+      expect(concreteWidget.y).toBe(originalY)
+      expect(concreteWidget.last_y).toBe(originalLastY)
+
+      // ctx.translate should be used instead of mutating widget state
+      expect(ctx.save).toHaveBeenCalled()
+      expect(ctx.translate).toHaveBeenCalledWith(0, slot.y - originalY)
+      expect(ctx.restore).toHaveBeenCalled()
+    })
+
+    it('does not mutate concrete widget y/last_y even when drawWidget throws', () => {
       const interiorWidget = createMockWidget({ type: 'number' })
       const interiorNode = {
         id: '5',
@@ -335,6 +430,7 @@ describe('PromotedWidgetSlot', () => {
         slot.drawWidget(ctx, { width: 200, showText: true })
       ).toThrow('render failure')
 
+      // Widget state was never mutated — ctx.translate is used instead
       expect(concreteWidget.y).toBe(10)
       expect(concreteWidget.last_y).toBe(5)
     })

src/core/graph/subgraph/PromotedWidgetSlot.ts

@@ -127,8 +127,14 @@ export class PromotedWidgetSlot
 
   override set label(v: string | undefined) {
     const resolved = this.resolve()
-    if (resolved) {
-      resolved.widget.label = v
+    if (!resolved) return
+
+    resolved.widget.label = v
+    const input = resolved.node.inputs?.find(
+      (inp) => inp.widget?.name === this.sourceWidgetName
+    )
+    if (input) {
+      input.label = v
     }
   }
 
@@ -156,16 +162,10 @@ export class PromotedWidgetSlot
 
     const concrete = toConcreteWidget(resolved.widget, resolved.node, false)
     if (concrete) {
-      const origY = concrete.y
-      const origLastY = concrete.last_y
-      concrete.y = this.y
-      concrete.last_y = this.last_y
-      try {
-        concrete.drawWidget(ctx, options)
-      } finally {
-        concrete.y = origY
-        concrete.last_y = origLastY
-      }
+      ctx.save()
+      ctx.translate(0, this.y - concrete.y)
+      concrete.drawWidget(ctx, options)
+      ctx.restore()
     } else {
       this.drawWidgetShape(ctx, options)
       if (options.showText !== false) {

src/core/graph/subgraph/promotedWidgetRegistration.test.ts

@@ -0,0 +1,149 @@
+import { createPinia, setActivePinia } from 'pinia'
+import { beforeEach, describe, expect, it, vi } from 'vitest'
+
+import { PromotedWidgetSlot } from '@/core/graph/subgraph/PromotedWidgetSlot'
+import type { LGraphCanvas } from '@/lib/litegraph/src/litegraph'
+import { SubgraphNode } from '@/lib/litegraph/src/subgraph/SubgraphNode'
+import type { ISerialisedNode } from '@/lib/litegraph/src/types/serialisation'
+import type { IBaseWidget } from '@/lib/litegraph/src/types/widgets'
+
+import { registerPromotedWidgetSlots } from './promotedWidgetRegistration'
+
+vi.mock('@/core/graph/subgraph/proxyWidgetUtils', () => ({
+  promoteRecommendedWidgets: vi.fn()
+}))
+
+function createMockCanvas() {
+  const listeners = new Map<string, EventListener[]>()
+  const canvasElement = {
+    addEventListener: vi.fn((type: string, handler: EventListener) => {
+      if (!listeners.has(type)) listeners.set(type, [])
+      listeners.get(type)!.push(handler)
+    })
+  }
+  return {
+    canvas: canvasElement,
+    setDirty: vi.fn(),
+    _listeners: listeners
+  } as unknown as LGraphCanvas
+}
+
+function createMockSubgraphNode(
+  widgets: IBaseWidget[] = []
+): SubgraphNode & { properties: Record<string, unknown> } {
+  return {
+    isSubgraphNode: () => true,
+    widgets,
+    properties: { proxyWidgets: [] },
+    _setConcreteSlots: vi.fn(),
+    arrange: vi.fn()
+  } as unknown as SubgraphNode & { properties: Record<string, unknown> }
+}
+
+describe('registerPromotedWidgetSlots', () => {
+  beforeEach(() => {
+    setActivePinia(createPinia())
+  })
+
+  describe('onConfigure – syncPromotedWidgets', () => {
+    it('assigning to properties.proxyWidgets triggers widget reconstruction', () => {
+      const canvas = createMockCanvas()
+      registerPromotedWidgetSlots(canvas)
+
+      const nativeWidget = {
+        name: 'steps',
+        type: 'number',
+        value: 20,
+        options: {},
+        y: 0
+      } as unknown as IBaseWidget
+      const node = createMockSubgraphNode([nativeWidget])
+
+      const serialisedNode = {
+        properties: {}
+      } as ISerialisedNode
+
+      SubgraphNode.prototype.onConfigure!.call(node, serialisedNode)
+
+      // After onConfigure, proxyWidgets is a getter/setter property
+      const descriptor = Object.getOwnPropertyDescriptor(
+        node.properties,
+        'proxyWidgets'
+      )
+      expect(descriptor?.set).toBeDefined()
+      expect(descriptor?.get).toBeDefined()
+
+      // Assign promoted widgets via the setter
+      node.properties.proxyWidgets = [['42', 'seed']]
+
+      // The setter should have created a PromotedWidgetSlot
+      const promotedSlots = node.widgets.filter(
+        (w) => w instanceof PromotedWidgetSlot
+      )
+      expect(promotedSlots).toHaveLength(1)
+      expect(promotedSlots[0].sourceNodeId).toBe('42')
+      expect(promotedSlots[0].sourceWidgetName).toBe('seed')
+
+      // The setter should have called _setConcreteSlots and arrange
+      expect(node._setConcreteSlots).toHaveBeenCalled()
+      expect(node.arrange).toHaveBeenCalled()
+    })
+
+    it('preserves native widgets not in the proxy list', () => {
+      const canvas = createMockCanvas()
+      registerPromotedWidgetSlots(canvas)
+
+      const nativeWidget = {
+        name: 'steps',
+        type: 'number',
+        value: 20,
+        options: {},
+        y: 0
+      } as unknown as IBaseWidget
+
+      const node = createMockSubgraphNode([nativeWidget])
+      const serialisedNode = {
+        properties: {}
+      } as ISerialisedNode
+
+      SubgraphNode.prototype.onConfigure!.call(node, serialisedNode)
+
+      // Promote a different widget; native 'steps' should remain
+      node.properties.proxyWidgets = [['42', 'seed']]
+
+      const nativeWidgets = node.widgets.filter(
+        (w) => !(w instanceof PromotedWidgetSlot)
+      )
+      expect(nativeWidgets).toHaveLength(1)
+      expect(nativeWidgets[0].name).toBe('steps')
+    })
+
+    it('re-orders native widgets listed in the proxy list with id -1', () => {
+      const canvas = createMockCanvas()
+      registerPromotedWidgetSlots(canvas)
+
+      const nativeWidget = {
+        name: 'steps',
+        type: 'number',
+        value: 20,
+        options: {},
+        y: 0
+      } as unknown as IBaseWidget
+
+      const node = createMockSubgraphNode([nativeWidget])
+      const serialisedNode = {
+        properties: {}
+      } as ISerialisedNode
+
+      SubgraphNode.prototype.onConfigure!.call(node, serialisedNode)
+
+      // Use -1 to reference native widgets
+      node.properties.proxyWidgets = [['-1', 'steps']]
+
+      // Native widget should be placed via the proxy list ordering
+      expect(node.widgets).toHaveLength(1)
+      expect(node.widgets[0].name).toBe('steps')
+      expect(node.widgets[0]).toBe(nativeWidget)
+    })
+  })
+})

src/core/graph/subgraph/promotedWidgetRegistration.ts

@@ -8,14 +8,22 @@ import type { ISerialisedNode } from '@/lib/litegraph/src/types/serialisation'
 import type { IBaseWidget } from '@/lib/litegraph/src/types/widgets'
 import { useCanvasStore } from '@/renderer/core/canvas/canvasStore'
 
+let registered = false
+
 /**
  * Registers the promoted widget system using PromotedWidgetSlot instances.
  * Sets up:
  * - `subgraph-opened` event: syncs `promoted` flags on interior widgets
  * - `subgraph-converted` event: auto-promotes recommended widgets
  * - `onConfigure` override: creates PromotedWidgetSlot instances in widgets[]
+ *
+ * Prototype patching is necessary because `onConfigure` must be set before
+ * SubgraphNode construction (called during `configure()` in the constructor).
  */
 export function registerPromotedWidgetSlots(canvas: LGraphCanvas) {
+  if (registered) return
+  registered = true
+
   canvas.canvas.addEventListener<'subgraph-opened'>('subgraph-opened', (e) => {
     const { subgraph, fromNode } = e.detail
     const proxyWidgets = parseProxyWidgets(fromNode.properties.proxyWidgets)
@@ -34,6 +42,49 @@ export function registerPromotedWidgetSlots(canvas: LGraphCanvas) {
   SubgraphNode.prototype.onConfigure = onConfigure
 }
 
+/**
+ * Reconstructs the promoted widget slots on a subgraph node based on
+ * a serialized proxy widgets list.
+ *
+ * This replaces the previous side-effecting property setter pattern where
+ * assigning to `properties.proxyWidgets` would trigger widget reconstruction.
+ */
+function syncPromotedWidgets(
+  node: LGraphNode & { isSubgraphNode(): boolean },
+  property: NodeProperty
+): void {
+  const canvasStore = useCanvasStore()
+  const parsed = parseProxyWidgets(property)
+
+  // Snapshot native widgets before filtering so we can restore them
+  const widgets = node.widgets ?? []
+
+  const nativeWidgets = widgets.filter(
+    (w) => !(w instanceof PromotedWidgetSlot)
+  )
+
+  // Remove existing PromotedWidgetSlot instances and native widgets
+  // that will be re-ordered by the parsed list
+  node.widgets = widgets.filter((w) => {
+    if (w instanceof PromotedWidgetSlot) return false
+    return !parsed.some(([, name]) => w.name === name)
+  })
+
+  // Create new PromotedWidgetSlot for each promoted entry
+  const newSlots: IBaseWidget[] = parsed.flatMap(([nodeId, widgetName]) => {
+    if (nodeId === '-1') {
+      const widget = nativeWidgets.find((w) => w.name === widgetName)
+      return widget ? [widget] : []
+    }
+    return [new PromotedWidgetSlot(node as SubgraphNode, nodeId, widgetName)]
+  })
+  node.widgets.push(...newSlots)
+
+  canvasStore.canvas?.setDirty(true, true)
+  node._setConcreteSlots()
+  node.arrange()
+}
+
 const originalOnConfigure = SubgraphNode.prototype.onConfigure
 const onConfigure = function (
   this: LGraphNode,
@@ -42,7 +93,6 @@ const onConfigure = function (
   if (!this.isSubgraphNode())
     throw new Error("Can't add promoted widgets to non-subgraphNode")
 
-  const canvasStore = useCanvasStore()
   this.properties.proxyWidgets ??= []
 
   originalOnConfigure?.call(this, serialisedNode)
@@ -54,41 +104,11 @@ const onConfigure = function (
           ? [w.sourceNodeId, w.sourceWidgetName]
           : ['-1', w.name]
       ),
-    set: (property: NodeProperty) => {
-      const parsed = parseProxyWidgets(property)
-
-      // Snapshot native widgets before filtering so we can restore them
-      const nativeWidgets = this.widgets.filter(
-        (w) => !(w instanceof PromotedWidgetSlot)
-      )
-
-      // Remove existing PromotedWidgetSlot instances and native widgets
-      // that will be re-ordered by the parsed list
-      this.widgets = this.widgets.filter((w) => {
-        if (w instanceof PromotedWidgetSlot) return false
-        return !parsed.some(([, name]) => w.name === name)
-      })
-
-      // Create new PromotedWidgetSlot for each promoted entry
-      const newSlots: IBaseWidget[] = parsed.flatMap(([nodeId, widgetName]) => {
-        if (nodeId === '-1') {
-          const widget = nativeWidgets.find((w) => w.name === widgetName)
-          return widget ? [widget] : []
-        }
-        return [
-          new PromotedWidgetSlot(this as SubgraphNode, nodeId, widgetName)
-        ]
-      })
-      this.widgets.push(...newSlots)
-
-      canvasStore.canvas?.setDirty(true, true)
-      this._setConcreteSlots()
-      this.arrange()
-    }
+    set: (value: NodeProperty) => syncPromotedWidgets(this, value)
   })
 
   if (serialisedNode.properties?.proxyWidgets) {
-    this.properties.proxyWidgets = serialisedNode.properties.proxyWidgets
+    syncPromotedWidgets(this, serialisedNode.properties.proxyWidgets)
     const parsed = parseProxyWidgets(serialisedNode.properties.proxyWidgets)
     serialisedNode.widgets_values?.forEach((v, index) => {
       if (parsed[index]?.[0] !== '-1') return

src/utils/widgetUtil.test.ts

@@ -0,0 +1,75 @@
+import { createPinia, setActivePinia } from 'pinia'
+import { beforeEach, describe, expect, it } from 'vitest'
+
+import type { LGraphNode } from '@/lib/litegraph/src/LGraphNode'
+import type { IBaseWidget } from '@/lib/litegraph/src/types/widgets'
+
+import { renameWidget } from './widgetUtil'
+
+function createMockWidget(overrides: Partial<IBaseWidget> = {}): IBaseWidget {
+  return {
+    name: 'seed',
+    type: 'number',
+    value: 42,
+    options: {},
+    y: 0,
+    ...overrides
+  } satisfies Partial<IBaseWidget> as IBaseWidget
+}
+
+function createMockNode(
+  widgets: IBaseWidget[] = [],
+  inputs: Array<{
+    name: string
+    widget?: { name: string }
+    label?: string
+  }> = []
+): LGraphNode {
+  return {
+    widgets,
+    inputs
+  } as unknown as LGraphNode
+}
+
+describe('renameWidget', () => {
+  beforeEach(() => {
+    setActivePinia(createPinia())
+  })
+
+  it('sets widget label to new name', () => {
+    const widget = createMockWidget()
+    const node = createMockNode([widget])
+
+    renameWidget(widget, node, 'New Name')
+    expect(widget.label).toBe('New Name')
+  })
+
+  it('clears widget label when given empty string', () => {
+    const widget = createMockWidget()
+    widget.label = 'Old'
+    const node = createMockNode([widget])
+
+    renameWidget(widget, node, '')
+    expect(widget.label).toBeUndefined()
+  })
+
+  it('updates matching input label', () => {
+    const widget = createMockWidget()
+    const input = {
+      name: 'seed',
+      widget: { name: 'seed' },
+      label: undefined as string | undefined
+    }
+    const node = createMockNode([widget], [input])
+
+    renameWidget(widget, node, 'Renamed')
+    expect(input.label).toBe('Renamed')
+  })
+
+  it('returns true on success', () => {
+    const widget = createMockWidget()
+    const node = createMockNode([widget])
+
+    expect(renameWidget(widget, node, 'New')).toBe(true)
+  })
+})