Skip to content

fix(workflow): preserve record widget values - #1016

Open
allenflux wants to merge 4 commits into
Comfy-Org:mainfrom
allenflux:codex/fix-record-widget-values
Open

allenflux wants to merge 4 commits into
Comfy-Org:mainfrom
allenflux:codex/fix-record-widget-values

Conversation

@allenflux

Copy link
Copy Markdown

UI-to-API conversion routes any dictionary in widgets_values through the self-naming custom-widget layout. For ImageCropV2, that drops a saved 1669×2688 crop_region and replaces it with the 512×512 schema default.

Use the existing schema widget classification before that fallback so declared record-valued slots follow the positional schema walk. Preserve the legacy layout for nodes without declared widget slots, with regressions for UI trailers, opaque records, linked inputs, and PowerLoRA controls.

Validation:

  • ruff check . and ruff format --check . pass (ruff 0.15.15).
  • Complete pytest . -q: 10,112 passed, 39 skipped on Linux arm64 / uv-managed Python 3.10.20, using locked project dependencies and prepared pip/fixture caches for offline execution.
  • Both converter test files pass (151 tests); the 16 new cases on the original implementation produce 8 record failures and 8 passing legacy controls.
  • Actual ComfyUI CPU execution using real node schemas and synthetic UI workflows: baseline PNG 512×512, fixed PNG 1669×2688.

An earlier macOS/Python 3.12 full run was not clean: global telemetry opt-out settings interfered with tracking tests (204 pass when corrected), a CA assertion also fails on the unchanged base, and a pretty-output assertion did not reproduce in isolation.

AI-assisted implementation, testing, and review.

Fixes #1011.

@coderabbitai

coderabbitai Bot commented Oct 16, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: fed8ef96-8c45-4245-b43b-b5e2e7ec77b2
📥 Commits

Reviewing files that changed from the base of the PR and between 1da7e7b and 3e69829.

📒 Files selected for processing: 3
  • CHANGELOG.md
  • comfy_cli/workflow_to_api.py
  • tests/comfy_cli/test_workflow_to_api.py

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.

📝 Walkthrough

UI-to-API conversion now applies self-naming handling to dictionary-valued widgets only when the schema declares no widget inputs. Tests cover record-valued widgets, positional mapping, linked inputs, and Power Lora Loader layouts.

Changes: Widget conversion

Layer / File(s) Summary
Schema-based widget routing
comfy_cli/workflow_to_api.py, tests/comfy_cli/test_workflow_to_api.py, CHANGELOG.md
Dictionary-valued widgets use self-naming handling only when the schema declares no widget inputs. Tests cover saved records, positional alignment, linked inputs, and Power Lora Loader layouts. The changelog documents record-valued widget preservation.

Priority: ➖ Normal

Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to 3e698

The change preserves saved record-widget values while retaining the legacy layout for schemas without widget inputs. No actionable merge-blocking risk remains after normal checks.

Pre-merge checks: Passed 2
Check name Status Explanation
Linked Issues check Passed Issue #1011 requires preservation of record-valued widget data during UI-to-API conversion. workflow_to_api.py now checks declared schema widget inputs before applying dictionary self-naming. Declar…
Out of Scope Changes check Passed The code change is limited to widget classification in workflow_to_api.py. The tests verify the issue fix and preserve legacy layouts. The changelog documents the same behavior. No unrelated change …
Full details: Linked Issues check

Explanation

Issue #1011 requires preservation of record-valued widget data during UI-to-API conversion. workflow_to_api.py now checks declared schema widget inputs before applying dictionary self-naming. Declared records use positional schema pairing, including ImageCropV2.crop_region, so saved values are not replaced by schema defaults. Tests cover trailing UI values, opaque records, linked records, declared positions, and PowerLoRA compatibility.

Full details: Out of Scope Changes check

Explanation

The code change is limited to widget classification in workflow_to_api.py. The tests verify the issue fix and preserve legacy layouts. The changelog documents the same behavior. No unrelated change is established by the supplied whole-PR evidence.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
✨ Simplify code
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

@github-actions

github-actions Bot commented Oct 16, 2026 •

Copy link
Copy Markdown

✅ All contributors have signed the CLA. Thank you! This PR is ready to be merged.
Posted by the CLA Assistant Lite bot.

@allenflux
allenflux marked this pull request as ready for review October 17, 2026 00:53
@allenflux

Copy link
Copy Markdown
Author

I have read and agree to the Contributor License Agreement

@coderabbitai
coderabbitai Bot requested a review from jaeone94 October 17, 2026 00:55
comfy-legal added a commit to Comfy-Org/comfy-cla that referenced this pull request Oct 17, 2026

@jaeone94 jaeone94 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the clear writeup. I reproduced the bug locally: the new TestRecordWidgetValues cases fail on main (8 failures) and pass on this branch, and the full CI matrix is green after I approved the runs. The fix itself is small and correct for record slots like ImageCropV2.crop_region, and Power Lora Loader still takes the self-naming path since its schema declares no widgets.

One behavior worth a decision, plus a few small cleanups inline.

Mixed nodes (declared widgets + self-named dicts). The new gate checks "does the schema declare any widget", not "does this dict sit in a declared slot". So a custom node that declares scalar widgets and also saves frontend-only dicts now takes the positional walk:

widgets_values (schema declares seed, text) main this PR
[5, "hi", {"type": "Hdr"}] seed/text fall back to defaults, Hdr kept seed=5, text="hi", Hdr dropped
[{"type": "Hdr"}, 5, "hi"] seed/text fall back to defaults, Hdr kept seed={"type": "Hdr"}, text=5 (shifted; fails server validation)

Main was already wrong for these nodes, and I couldn't find a real node with this layout, so I don't think this blocks. If you want to cover it, one option is to run the schema walk first and only hand the values left over after the declared slots to _absorb_dict_widget_values. Either way, a test pinning whichever behavior you pick for the mixed case would be nice.

Comment thread comfy_cli/workflow_to_api.py Outdated

if any(isinstance(v, dict) for v in widget_values):
schema_widgets = any(_is_widget_input(spec)[0] for spec in input_specs) if schema else False
# Declared widget slots can hold records; keep the self-naming layout for nodes without those slots.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: the comment reads as if the gate looks for slots that accept records, but it triggers on any declared widget. Maybe something like:

Suggested change
# Declared widget slots can hold records; keep the self-naming layout for nodes without those slots.
# Any schema-declared widget means widgets_values is positional; only schema-less layouts self-name via dicts.

Comment thread comfy_cli/workflow_to_api.py Outdated
@@ -1683,7 +1685,6 @@ def emit(name: str, value: Any) -> Any:
# (e.g. ``COMFY_AUTOGROW_V3`` images) so they never consume a value slot.
schema = _schema_for(node_type, node, object_info)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: schema is already resolved at the top of the function with the same arguments, and schema_widgets now relies on that earlier value. This second lookup can go so nobody wonders whether the two could differ.

Comment thread CHANGELOG.md Outdated
Comment on lines +124 to +126
- Preserve record-valued widgets such as `ImageCropV2.crop_region` during
UI-to-API workflow conversion instead of replacing them with schema defaults.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: the other entries in this section have no blank line between bullets and describe the user-visible outcome. Something like:

Suggested change
- Preserve record-valued widgets such as `ImageCropV2.crop_region` during
UI-to-API workflow conversion instead of replacing them with schema defaults.
- Converting a UI workflow to API format keeps a record widget's saved value,
such as `ImageCropV2.crop_region`, instead of replacing it with the schema
default.

@coderabbitai
coderabbitai Bot requested a review from bigcat88 October 18, 2026 01:06

@jaeone94 jaeone94 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick follow-up, LGTM.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

UI→API conversion drops a record-valued widget (ImageCropV2.crop_region) and runs the node with its default

2 participants