Skip to content

fix(widgets): collapse duplicate COLOR widget rendering on Color to RGB Int (FE-842)#12447

Open
dante01yoon wants to merge 2 commits into
mainfrom
jaewon/core-256-color-widget-dedupe
Open

fix(widgets): collapse duplicate COLOR widget rendering on Color to RGB Int (FE-842)#12447
dante01yoon wants to merge 2 commits into
mainfrom
jaewon/core-256-color-widget-dedupe

Conversation

@dante01yoon
Copy link
Copy Markdown
Collaborator

@dante01yoon dante01yoon commented May 24, 2026

Summary

Fix the duplicate `` rendering on the `Color to RGB Int` node (and any other COLOR-using V3 node that the runtime double-registers a widget for).
after-fix-dedupe-proof

Changes

  • What:
    • `useProcessedWidgets.getWidgetIdentity`: fall back to the host `nodeId` parameter for the dedupe identity root when neither `storeNodeId/widget.nodeId` nor `sourceExecutionId` is set. Normal root-graph widgets now dedupe identically to promoted/execution-scoped widgets, so any duplicate same-name+same-type widget collapses to one render. `sourceExecutionId` precedence is preserved.
    • `useColorWidget`: read top-level `default` from the V2 spec (falls back to nested `options.default` for hand-authored V2 specs), and short-circuit if a same-name color widget already exists on `node.widgets` so a second `addWidget('color', …)` call from upstream hooks (or a `configure` round-trip) no longer duplicates the row.
  • Tests:
    • New `useColorWidget.test.ts` covers top-level default, nested-options fallback, no-default fallback, and the idempotency guard.
    • `useProcessedWidgets.test.ts` gets a regression case for two identical color widgets on the same node collapsing to one render, plus an updated `getWidgetIdentity` case for the host-nodeId fallback.

Review Focus

  • `getWidgetIdentity` precedence change. The fallback only fires when none of `storeNodeId`, `widget.nodeId`, or `sourceExecutionId` are present, so promoted/exec-scoped widgets (incl. the "unresolved same-name promoted entries distinct by source execution identity" `NodeWidgets` test) are unaffected.
  • `useColorWidget` idempotency guard is defensive — the root cause of the second `addWidget` call (cloud-only hook or persisted `info.widgets` configure round-trip) is not in this diff; that's tracked separately.

Fixes FE-842

…GB Int

Two color widgets render side-by-side for nodes whose backend declares a
single `Color.Input`. The visible cause is that `node.widgets` ends up
with two same-name color widgets at runtime, but `useProcessedWidgets`
only dedupes promoted / execution-scoped widgets — for regular root-graph
widgets `dedupeIdentity` was always `undefined`, so duplicates fell
through to two `<WidgetColorPicker>` instances.

Two defensive fixes:

1. `getWidgetIdentity` now falls back to the host `nodeId` parameter
   when neither `storeNodeId/widget.nodeId` nor `sourceExecutionId` are
   available, so identical normal widgets on the same node collapse to
   one render. `sourceExecutionId` precedence is preserved.
2. `useColorWidget` honors the V2 top-level `default` and short-circuits
   if a same-name color widget is already on the node, preventing a
   second `addWidget` call from upstream hooks.

Fixes CORE-256
@dante01yoon dante01yoon added needs-backport Fix/change that needs to be cherry-picked to the current feature freeze branch cloud/1.44 Backport PRs for cloud 1.44 core/1.44 Backport PRs for core 1.44 cloud/1.45 Backport PRs for cloud 1.45 core/1.45 Backport PRs for core 1.45 labels May 24, 2026
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 24, 2026

Review Change Stack

📝 Walkthrough

Walkthrough

Adds a host-derived fallback for widget identity used in deduplication when stable IDs are missing, introduces tests asserting the new transient/dedupe behaviors, and updates color widget logic to prefer inputSpec.default, fallback to options?.default then '#000000', reusing existing color widgets to avoid duplicates.

Changes

Widget Improvements

Layer / File(s) Summary
Widget identity fallback and deduplication
src/renderer/extensions/vueNodes/composables/useProcessedWidgets.ts, src/renderer/extensions/vueNodes/composables/useProcessedWidgets.test.ts
getWidgetIdentity derives a hostNodeIdRoot from the provided nodeId as a fallback base for stableIdentityRoot when rawWidgetId and sourceExecutionId are absent, altering dedupeIdentity. Tests updated/added to assert dedupe fallback behavior and that computeProcessedWidgets collapses duplicate normal widgets on the same node.
Color widget defaults and widget reuse
src/renderer/extensions/vueNodes/widgets/composables/useColorWidget.ts, src/renderer/extensions/vueNodes/widgets/composables/useColorWidget.test.ts
useColorWidget resolves defaultValue by preferring inputSpec.defaultoptions?.default'#000000' (nullish-coalescing). It looks up existing node widgets matching name and type === 'color' and returns the existing widget instead of creating a duplicate. New tests cover default precedence and reuse behavior.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested labels

size:M

Suggested reviewers

  • christian-byrne

Poem

🐰 A rabbit hopped in with a careful cheer,
Identity fallbacks now hold widgets near.
Colors find defaults in the right array,
And duplicate widgets hop away.
Hooray for tidy nodes — a joyful gear!

🚥 Pre-merge checks | ✅ 6 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (6 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
End-To-End Regression Coverage For Fixes ✅ Passed PR uses "fix" language and includes browser_tests/ changes (widgetReactivity.spec.ts), satisfying the condition for e2e regression test coverage.
Adr Compliance For Entity/Litegraph Changes ✅ Passed No changed files are under src/lib/litegraph/ or src/ecs/. Changes are in Vue renderer composables in src/renderer/extensions/vueNodes/, not core graph entity definitions.
Title check ✅ Passed The title accurately identifies the main change: fixing duplicate COLOR widget rendering on the 'Color to RGB Int' node, with a clear reference to the issue (FE-842).
Description check ✅ Passed The PR description includes all required sections: Summary explains the fix, Changes details the modifications, and Review Focus highlights critical design decisions. Includes issue reference and screenshot evidence.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch jaewon/core-256-color-widget-dedupe

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 24, 2026

🎨 Storybook: ✅ Built — View Storybook

Details

⏰ Completed at: 05/24/2026, 02:23:23 PM UTC

Links

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 24, 2026

🎭 Playwright: ✅ 1637 passed, 0 failed · 2 flaky

📊 Browser Reports
  • chromium: View Report (✅ 1618 / ❌ 0 / ⚠️ 1 / ⏭️ 5)
  • chromium-2x: View Report (✅ 2 / ❌ 0 / ⚠️ 0 / ⏭️ 0)
  • chromium-0.5x: View Report (✅ 1 / ❌ 0 / ⚠️ 0 / ⏭️ 0)
  • mobile-chrome: View Report (✅ 16 / ❌ 0 / ⚠️ 1 / ⏭️ 0)

@codecov
Copy link
Copy Markdown

codecov Bot commented May 24, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

@@             Coverage Diff             @@
##             main   #12447       +/-   ##
===========================================
- Coverage   74.42%   60.18%   -14.24%     
===========================================
  Files        1526     1416      -110     
  Lines       86018    72557    -13461     
  Branches    23699    20193     -3506     
===========================================
- Hits        64016    43670    -20346     
- Misses      21179    28413     +7234     
+ Partials      823      474      -349     
Flag Coverage Δ
e2e ?
unit 60.18% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...nsions/vueNodes/composables/useProcessedWidgets.ts 89.40% <100.00%> (-6.85%) ⬇️
...ons/vueNodes/widgets/composables/useColorWidget.ts 100.00% <100.00%> (+22.22%) ⬆️

... and 1014 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@dante01yoon
Copy link
Copy Markdown
Collaborator Author

Manual visual verification (cloud distribution)

Verified locally against pnpm dev:cloud-prod (DISTRIBUTION=cloud + cloud.comfy.org backend) on the patched branch.

1. Baseline — Color to RGB Int node added via LiteGraph.createNode:

  • node.widgets.length === 1
  • Rendered color value: #ffffff (V2 top-level default from backend Color.Input("color", default="#ffffff") is now honored — previously fell back to #000000).

2. Dedupe layer proof — force-pushed a second identical color widget into node.widgets:

node.widgets.push(Object.assign(Object.create(Object.getPrototypeOf(existing)), existing))
// node.widgets.length === 2
  • Vue rendered output: still 1 <WidgetColorPicker> row, not 2.
  • getWidgetIdentity now produces dedupeIdentity = 'node:1:color:color:color' for both entries via the host-nodeId fallback, so computeProcessedWidgets collapses them to a single render.

Screenshots saved locally (gitignored under temp/screenshots/):

  • core-256-baseline-single-widget.png — node with one color widget, value #ffffff
  • core-256-after-fix-dedupe.png — same node after forcing widgets.length === 2, still one row in the DOM

Confirms the fix lands at the rendering layer regardless of where the second node.widgets entry originates (cloud-only hook, configure round-trip, or any future composable that double-creates).

Quality gates: pnpm typecheck ✅, pnpm lint ✅, full pnpm test:unit 10,900 ✅. Browser_tests colors.spec.ts is gated on a local backend (createUser 500) — not a regression from this change.

@dante01yoon dante01yoon marked this pull request as ready for review May 24, 2026 14:17
@dante01yoon dante01yoon requested a review from a team May 24, 2026 14:17
@dosubot dosubot Bot added the size:S This PR changes 10-29 lines, ignoring generated files. label May 24, 2026
@dante01yoon
Copy link
Copy Markdown
Collaborator Author

dante01yoon commented May 24, 2026

Visual verification — before/after, with forced widgets.length === 2

Reproduced locally against pnpm dev:cloud-prod (DISTRIBUTION=cloud + cloud.comfy.org backend) on the patched branch.

# Phase Code state node.widgets.length DOM rows What it shows Screenshot
1 Before-fix · baseline Unpatched (other worktree on :5175) 1 1 (#000000) useColorWidget read options?.default (always undefined on V2 specs) and fell back to #000000, ignoring the backend Color.Input("color", default="#ffffff"). Single widget — bug not yet visible. before-fix-baseline
2 Before-fix · force duplicate Unpatched 2 via node.widgets.push(dup) 2 (#000000 × 2) getWidgetIdentity returned dedupeIdentity = undefined for normal root-graph widgets, so computeProcessedWidgets passed both entries through. The duplicate-render bug from the Slack thread reproduces here. before-fix-dedupe-bug
3 After-fix · baseline Patched (this PR, :5180) 1 1 (#ffffff) Top-level default honored — useColorWidget now reads colorSpec.default, so the backend-declared #ffffff flows through. Idempotency guard in place but only one widget present yet. after-fix-baseline
4 After-fix · force duplicate Patched 2 via node.widgets.push(dup) 1 (#ffffff) Same forced-duplication scenario as row 2, but now getWidgetIdentity falls back to the host nodeId and produces dedupeIdentity = 'node:1:color:color:color' for both entries, so computeProcessedWidgets collapses them to a single row. Bug fixed at the rendering layer regardless of where the second node.widgets entry originates. after-fix-dedupe-proof

Key comparisons

  • Row 2 → Row 4: with node.widgets.length === 2 forced identically in both, the unpatched build renders two <WidgetColorPicker> instances; the patched build renders one. Proves the dedupe fallback in getWidgetIdentity is what collapses the duplicate.
  • Row 1 → Row 3: declared default color changes from #000000 (lost) to #ffffff (honored). Proves the useColorWidget top-level-default read is working.

Quality gates: pnpm typecheck ✅, pnpm lint ✅, full pnpm test:unit 10,900 ✅ (incl. 4 new cases on useColorWidget + 1 regression case on computeProcessedWidgets collapsing duplicate normal widgets).

`Should display added widgets` was pushing the same widget reference into
`node.widgets` and expecting N rendered rows. After the CORE-256 dedupe
fix in `getWidgetIdentity` (which now derives a stable identity from the
host nodeId for normal widgets), identical name+type entries on the same
node correctly collapse to one render — exactly the bug behavior the
spec was inadvertently encoding as a contract.

Push shallow clones with unique names instead. This preserves the test's
intent (mutating `node.widgets` updates the rendered rows) without
relying on the bug we just fixed.
@dante01yoon dante01yoon changed the title fix(widgets): collapse duplicate COLOR widget rendering on Color to RGB Int fix(widgets): collapse duplicate COLOR widget rendering on Color to RGB Int (FE-842) May 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cloud/1.44 Backport PRs for cloud 1.44 cloud/1.45 Backport PRs for cloud 1.45 core/1.44 Backport PRs for core 1.44 core/1.45 Backport PRs for core 1.45 needs-backport Fix/change that needs to be cherry-picked to the current feature freeze branch size:S This PR changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants