Subgraph widget promotion - Part 2 #5617

AustinMroz · 2025-09-17T03:07:01Z

Implements proxyWidget support on subgraph nodes.
This registers a special proxyWidgets property on subgraph nodes which is directly mapped to the proxyWidgets displayed on the node ~~with no duplication of state~~. Each proxyWidget directly maps to a real widget inside the subgraph.

Areas of concern

The proxyWidgets property is serialized as a string ~~on every access~~. Litegraph fully supports leaving this as a list, but this is safer than risking breakage by changing types, is 3-4 orders of magnitude less important than the performance of the proxying code, and provides a chance to slot in validation
The code is applied by patching SubgraphNode._internalConfigureAfterSlots. SubgraphNodes are a little jank because they sometimes are configured twice on initialization and this call deletes all widgets on the node each time. As there is no duplication of state, if the code is applied after the first call, all widgets are cleared from the node on the second. setTimeout was previously utilized, but makes for a much uglier workaround.
- Code now uses the standardised onConfigured callback.
The proxying code itself. ~~Don't be fooled by the any cast.~~ Typescript doesn't seem to bother checking types on reflection and the degree of wrapping is rather heavy handed.
The location where this code is stored. Placing code in scripts isn't great, but it puts it adjacent to the similar DOMWidget code and is consistent with simple act of importing the code applying side effects. Placing the code inside litegraph itself was originally attempted, but non-viable. The code requires access to multiple stores, including the domWidgetstore in order to manage state.

┆Issue is synchronized with this Notion page by Unito

github-actions · 2025-09-17T03:07:33Z

🎭 Playwright Test Results

⚠️ Tests passed with flaky tests

⏰ Completed at: 09/25/2025, 01:50:55 AM UTC

📈 Summary

Total Tests: 458
Passed: 426 ✅
Failed: 0
Flaky: 3 ⚠️
Skipped: 29 ⏭️

📊 Test Reports by Browser

✅ chromium: View Report • ✅ 419 / ❌ 0 / ⚠️ 3 / ⏭️ 29
✅ chromium-2x: View Report • ✅ 2 / ❌ 0 / ⚠️ 0 / ⏭️ 0
✅ chromium-0.5x: View Report • ✅ 1 / ❌ 0 / ⚠️ 0 / ⏭️ 0
✅ mobile-chrome: View Report • ✅ 4 / ❌ 0 / ⚠️ 0 / ⏭️ 0

🎉 Click on the links above to view detailed test results for each browser configuration.

src/scripts/proxyWidget.ts

christian-byrne

Could we move all the files to src/core/graph/subgraph

src/scripts/proxyWidget.ts

tests-ui/tests/widgets/proxyWidget.test.ts

christian-byrne · 2025-09-18T22:59:20Z

Sorry Claude review went haywire.

christian-byrne · 2025-09-24T19:52:56Z

The lint staged should pass if you rebase, since the knip config file was added to tsconfig include last week in #5662

Wrap onConfigure instead of _internalConfigureAfterSlots Use previously set proxyWidgets property instead of calculating Fix canvas preview image support Split proxy handlers into individual functions. Add additional tests

The code to reset display of proxied widgets before adding or removing wasn't being filtered to just proxy widgets. This would cause other promoted DOMWidgets to be hidden Resolves a playwright test

github-actions · 2025-09-24T20:05:01Z

🔧 Auto-fixes Applied

This PR has been automatically updated to fix linting and formatting issues.

⚠️ Important: Your local branch is now behind. Run git pull before making additional changes to avoid conflicts.

Changes made:

ESLint auto-fixes
Prettier formatting

src/core/graph/subgraph/proxyWidget.ts

christian-byrne · 2025-09-24T20:08:26Z

src/core/graph/subgraph/proxyWidget.ts

+        const w = addProxyWidget(this, `${nodeId}`, widgetName)
+        if (w instanceof DOMWidgetImpl) {
+          const widgetState = widgetStates.get(w.id)
+          if (!widgetState) continue


question (non-blocking): What would it mean if the condition if (!widgetState) is true?

At this point in the code, we have already called addProxyWidget, so if it turns out w is a DOM widget but isn't in the widgetStates map, what does that mean for us? Do we need some sort of transaction logic or to move the check for this before we create the proxy?

While the check was added to satisfy type guards and this shouldn't ever come up, it does make a good question. Particularly for the subsequent "set active" code.

A DOMWidget that somehow doesn't exist in the store can't be displayed. Ignoring a widget was fine previously when the getter was calculating from the actual widgets, but now it causes a desync of state. 🤔

Will think on it a bit more, but I'm ever-so-slightly leaning towards throwing an error. If this ever comes up in the future, we'd want to be notified either when testing or through sentry.

src/core/graph/subgraph/proxyWidget.ts

src/core/schemas/proxyWidget.ts

Adds additional documentation Moves modifications to widget state into functions in the domWidgetStore Moves types to the start of the file Throw an error on failed validation of a proxyWidget property

arjansingh

Only got a nit.

src/core/graph/subgraph/proxyWidget.ts

christian-byrne

LGTM

Implements proxyWidget support on subgraph nodes. This registers a special proxyWidgets property on subgraph nodes which is directly mapped to the proxyWidgets displayed on the node. Each proxyWidget directly maps to a real widget inside the subgraph. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-5617-Subgraph-widget-promotion-Part-2-2716d73d3650813d8621fefdce6ae518) by [Unito](https://www.unito.io) --------- Co-authored-by: GitHub Action <[email protected]>

AustinMroz requested review from a team as code owners September 17, 2025 03:07

dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Sep 17, 2025

christian-byrne added the area:subgraph label Sep 18, 2025

This comment was marked as spam.

Sign in to view

christian-byrne reviewed Sep 18, 2025

View reviewed changes

src/scripts/proxyWidget.ts Outdated Show resolved Hide resolved

christian-byrne reviewed Sep 18, 2025

View reviewed changes

src/scripts/proxyWidget.ts Outdated Show resolved Hide resolved

src/scripts/proxyWidget.ts Show resolved Hide resolved

tests-ui/tests/widgets/proxyWidget.test.ts Show resolved Hide resolved

AustinMroz added 6 commits September 24, 2025 14:57

Implement proxyWidgets on subgraphNodes

e0357c6

Merge partial changes from working branch

7b3baef

Wrap onConfigure instead of _internalConfigureAfterSlots Use previously set proxyWidgets property instead of calculating Fix canvas preview image support Split proxy handlers into individual functions. Add additional tests

Remove test for reading proxy widgets

b49958b

File movement and import fixes

ef3699a

Fix linting.

ea955fd

Fix hiding of non-proxied DOMWidgets

49bb39d

The code to reset display of proxied widgets before adding or removing wasn't being filtered to just proxy widgets. This would cause other promoted DOMWidgets to be hidden Resolves a playwright test

AustinMroz force-pushed the austin/widget-promotion-impl branch from 286832e to 49bb39d Compare September 24, 2025 20:01

[auto-fix] Apply ESLint and Prettier fixes

27037c8

christian-byrne reviewed Sep 24, 2025

View reviewed changes

Add feedback

03856c9

Adds additional documentation Moves modifications to widget state into functions in the domWidgetStore Moves types to the start of the file Throw an error on failed validation of a proxyWidget property

arjansingh reviewed Sep 25, 2025

View reviewed changes

src/core/graph/subgraph/proxyWidget.ts Outdated Show resolved Hide resolved

Reduce nesting when proxying node.imgs

4e5bf80

christian-byrne approved these changes Sep 25, 2025

View reviewed changes

AustinMroz merged commit 9678a87 into main Sep 26, 2025
21 checks passed

AustinMroz deleted the austin/widget-promotion-impl branch September 26, 2025 15:24

Subgraph widget promotion - Part 2 #5617

Subgraph widget promotion - Part 2 #5617

Conversation

AustinMroz commented Sep 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Sep 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎭 Playwright Test Results

📈 Summary

📊 Test Reports by Browser

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

Uh oh!

christian-byrne left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

christian-byrne commented Sep 18, 2025

Uh oh!

christian-byrne commented Sep 24, 2025

Uh oh!

github-actions bot commented Sep 24, 2025

🔧 Auto-fixes Applied

Changes made:

Uh oh!

Uh oh!

christian-byrne Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AustinMroz Sep 24, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

arjansingh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

christian-byrne left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

AustinMroz commented Sep 17, 2025 •

edited

Loading

github-actions bot commented Sep 17, 2025 •

edited

Loading

christian-byrne Sep 24, 2025 •

edited

Loading