Closed
Conversation
Contributor
📝 WalkthroughWalkthroughAdds grouped handling for dynamic-combo widgets across subgraphs, extends proxy overlays with visibility/re-resolution metadata, updates UI to render grouped widgets and enable group reordering, adds Playwright helpers/tests and litegraph/devtools support for dynamic-combo scenarios. Changes
Sequence Diagram(s)mermaid User->>UI: request promote widget Possibly related PRs
Suggested reviewers
✨ Finishing touches
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 |
🎭 Playwright Tests: ✅ PassedResults: 514 passed, 0 failed, 0 flaky, 8 skipped (Total: 522) 📊 Browser Reports
|
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 01/28/2026, 04:31:36 AM UTC 🔗 Links🎉 Your Storybook is ready for review! |
Bundle Size ReportSummary
Category Glance Per-category breakdownApp Entry Points — 25.9 kB (baseline 25.9 kB) • ⚪ 0 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 963 kB (baseline 960 kB) • 🔴 +3.47 kBGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 80.7 kB (baseline 80.7 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 9 added / 9 removed Panels & Settings — 471 kB (baseline 471 kB) • 🟢 -8 BConfiguration panels, inspectors, and settings screens
Status: 12 added / 12 removed User & Accounts — 3.94 kB (baseline 3.94 kB) • ⚪ 0 BAuthentication, profile, and account management bundles
Status: 3 added / 3 removed Editors & Dialogs — 2.86 kB (baseline 2.86 kB) • ⚪ 0 BModals, dialogs, drawers, and in-app editors
Status: 2 added / 2 removed UI Components — 33.7 kB (baseline 33.7 kB) • ⚪ 0 BReusable component library chunks
Status: 4 added / 4 removed Data & Services — 2.71 MB (baseline 2.71 MB) • 🔴 +3.35 kBStores, services, APIs, and repositories
Status: 8 added / 8 removed Utilities & Hooks — 25.3 kB (baseline 25.3 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Status: 7 added / 7 removed Vendor & Third-Party — 10.7 MB (baseline 10.7 MB) • ⚪ 0 BExternal libraries and shared vendor chunks
Other — 7.04 MB (baseline 7.04 MB) • 🟢 -195 BBundles that do not match a named category
Status: 34 added / 34 removed |
dosubot
bot
added
the
size:XXL
coderabbitai
bot
requested review from
KarryCharon,
Myestery,
Yorha4D,
christian-byrne and
shinshin86
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@browser_tests/fixtures/components/PropertiesPanel.ts`:
- Around line 30-56: promoteWidget currently performs blind clicks to expand
Advanced Inputs and to refresh the panel (clicking the Toggle properties panel
twice); replace those blind toggles with deterministic checks: reuse
ensureOpen() at the start (already called) and use the same expansion-check
pattern as demoteWidget to only click advancedInputsButton when it is not
already expanded, and after promoting the widget call the class's close() and
ensureOpen() (or ensureOpen() only if you need it open) instead of clicking the
Toggle properties panel twice to deterministically refresh state; update
promoteWidget to reference ensureOpen(), close(), demoteWidget's expansion-check
logic, and the advancedInputsButton locator when making these changes.
- Around line 61-79: The code builds unescaped RegExp from widgetName to find
elements (see inputsContent and widgetRow) which breaks for widget names with
regex metacharacters; replace those locator.filter({ hasText: new
RegExp(`^${widgetName}$`) }) usages with a safe exact-text matcher such as
getByText(widgetName, { exact: true }) or escape widgetName before creating the
RegExp, and update both occurrences (the inputsContent check and the widgetRow
lookup) to use the safe matcher so matching is exact and not treated as a regex.
In `@browser_tests/tests/dynamicWidgetsSubgraph.spec.ts`:
- Line 288: Replace the unconditional sleep at
comfyPage.page.waitForTimeout(500) with a retrying assertion (e.g., expect.poll
or Playwright locator-based waits) that checks the concrete condition indicating
subgraph conversion is finished (for example a specific element becomes
visible/has text, or a status attribute changes); update the test to poll that
condition via expect.poll or await expect(locator).toHaveText()/toBeVisible() so
the test retries until completion instead of using a fixed timeout.
- Line 269: Replace the explicit sleep comfyPage.page.waitForTimeout(500) with a
retrying Playwright assertion that waits for the expected state (for example use
await expect(comfyPage.locator('<selector>')).toHaveText('<expected>') or await
comfyPage.locator('<selector>').waitFor({ state: 'visible' })) and remove the
duplicate assertion further down so only the poll-based assertion remains;
target the comfyPage.page.waitForTimeout call in the test and use Playwright's
expect/locator.waitFor APIs instead.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@browser_tests/tests/dynamicWidgetsSubgraph.spec.ts`:
- Around line 311-314: Replace the immediate assertion after pressing Escape
with a retrying Playwright assertion: after calling
comfyPage.page.keyboard.press('Escape') (and keeping comfyPage.nextFrame() if
needed), change expect(await comfyPage.isInSubgraph()).toBe(false) to a
poll-style assertion such as await expect.poll(async () => await
comfyPage.isInSubgraph()).toBe(false) so the test retries until the subgraph
exit completes; reference comfyPage.page.keyboard.press, comfyPage.nextFrame,
and comfyPage.isInSubgraph when locating the change.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@browser_tests/tests/dynamicWidgetsSubgraph.spec.ts`:
- Around line 161-242: The assertions that check widget state immediately after
calling comfyPage.menu.topbar.switchToTab(0) can be flaky; update both tests to
wait/retry for the subgraph and its widgets to be ready before asserting by
polling getSubgraphNode() and reloadedSubgraph.getWidgets() (or using
comfyPage.waitForFunction) with a short timeout/retry loop after switchToTab,
then perform the expect on the stable result (references:
comfyPage.menu.topbar.switchToTab, getSubgraphNode, reloadedSubgraph.getWidgets,
the two test blocks "Promoted combo maintains state after workflow reload" and
"Hidden children remain hidden after workflow reload when combo is none").
- Around line 96-159: Replace immediate synchronous assertions after
comboWidget.setValue with retrying assertions using Playwright's expect.poll():
after each call to comboWidget.setValue(...) call expect.poll(() =>
subgraphNode.getWidgets()).toEqual(...) so the check retries until the UI
updates. Update each occurrence where comboWidget.setValue is followed by
expect(await subgraphNode.getWidgets()) — e.g., the sequences around
comboWidget.setValue in this test — and keep the same expected arrays built with
widget(...) and subgraphWidgetName(...) so the assertion logic and helpers
remain unchanged.
AustinMroz
reviewed
Jan 28, 2026
Collaborator
AustinMroz
left a comment
AustinMroz
left a comment
There was a problem hiding this comment.
Will need to spend some time poking on the actual code, but figure it's best to report on the issues I've seen so far.
Looks like there's rough edges with nested dynamicCombos
- There's some concerning (but potentially intentional?) mixing of
indexOf('.')andlastIndexOf('.')in the code - Promoting a group will sometimes fail to pick all children.
- These are correctly grabbed when the value of the parent is changed
- Some widgets which should not be grouped are (positive and negative prompt of default workflow)
And then attempting to promote a dynamicCombo across nested subgraphs is even more rough.
| */ | ||
| function getBaseWidgetName(widget: IBaseWidget): string { | ||
| // Check if it's a proxy widget with _overlay | ||
| const overlay = (widget as { _overlay?: { widgetName?: string } })._overlay |
Collaborator
There was a problem hiding this comment.
Suggested change
| const overlay = (widget as { _overlay?: { widgetName?: string } })._overlay | |
| const overlay = isProxyWidget(widget) ? widget._overlay : undefined |
Super nit, but probably for the best in case the isProxyWidget guard gets tightened in the future.
| overlay.hidden = shouldHide | ||
| subgraphNode.setDirtyCanvas(true, true) | ||
| } | ||
| } |
Collaborator
There was a problem hiding this comment.
Hiding in vue mode currently uses widget.options.hidden instead of widget.hidden. Probably better fixed in useGraphNodeManager.
There's also some potential for shenanigans if a promoted widget already has the hidden flag. Might cause issues with nested promotion of dynamicCombos? 😅
auto-merge was automatically disabled
January 29, 2026 00:27
Pull request was converted to draft
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.
Summary
Fixes for dynamic widgets on subgraphs.
Changes
Review Focus
.- this pattern already exists in the dynamic widgets codeFuture update pass to disable/hide the disconencted dynamic widgets in the side panel
We should look at refactoring the proxy widget system, or serialize more data about them to potentially reduce need for the
.parsingScreenshots
dynamic.widget.subgraph.mp4
┆Issue is synchronized with this Notion page by Unito