Skip to content

fix(api-io): serialize MultiCombo multi_select as object config#13484

Open
Kosinkadink wants to merge 5 commits intomasterfrom
fix/multicombo-multi-select-serialization
Open

fix(api-io): serialize MultiCombo multi_select as object config#13484
Kosinkadink wants to merge 5 commits intomasterfrom
fix/multicombo-multi-select-serialization

Conversation

@Kosinkadink
Copy link
Copy Markdown
Member

@Kosinkadink Kosinkadink commented Apr 20, 2026

Summary

Fix MultiCombo.Input.as_dict() serialization so multi_select is emitted as an object config instead of a boolean, and fix validation so MultiCombo list values are accepted.

Based on #12662 by @Ni-zav with additional fixes:

  • Remove dead if self.multiselect else None branch (MultiCombo always sets self.multiselect = True)
  • Remove redundant top-level placeholder/chip keys (frontend reads them from multi_select object)
  • Fix prompt validation to check each element of a MultiCombo list against options individually
  • Fix custom node skip warning to mention comfy_entrypoint and remove nonexistent NODES_LIST
  • Update tests to match simplified output

Why

Frontend expects multi_select to be an object (z.object({ placeholder, chip })). When serialized as a boolean, MultiCombo is not recognized as multiselect. Additionally, prompt validation rejected MultiCombo values because it compared the entire list against the options instead of checking each selected value individually.

Changes

  • comfy_api/latest/_io.py: Emit multi_select: {} (or with placeholder/chip when provided)
  • execution.py: Handle list values in combo validation for MultiCombo inputs
  • nodes.py: Fix misleading skip warning message
  • tests-unit/comfy_api_test/multicombo_serialization_test.py: Regression tests

Closes #12662

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 20, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 44b16614-1d50-470f-b384-6da7bf2e553d

📥 Commits

Reviewing files that changed from the base of the PR and between fc51d13 and 711907e.

📒 Files selected for processing (2)
  • execution.py
  • tests-unit/comfy_api_test/multicombo_serialization_test.py
🚧 Files skipped from review as they are similar to previous changes (2)
  • execution.py
  • tests-unit/comfy_api_test/multicombo_serialization_test.py

📝 Walkthrough

Walkthrough

MultiCombo.Input.as_dict() now emits a nested multi_select object (containing optional placeholder and chip) while retaining the top-level boolean multiselect via Combo.Input.as_dict(); a stale TODO comment was removed. New unit tests were added to cover Combo and MultiCombo serialization and validation behavior. validate_inputs in execution.py was updated to accept multiselect values (treating inputs as lists when extra_info["multiselect"] is set) and to report invalid list elements individually. A log message in load_custom_node was edited to reference comfy_entrypoint.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Out of Scope Changes check ⚠️ Warning The changes in nodes.py (warning message update) are out of scope relative to #12662 but justified by the PR description as a related fix for misleading messaging. The nodes.py change about comfy_entrypoint warning is not mentioned in issue #12662. Either move it to a separate PR or add it to the linked issue scope.
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: fixing MultiCombo serialization to emit multi_select as an object config instead of a boolean.
Description check ✅ Passed The description is well-related to the changeset, explaining the problem, why it matters, and what changes were made across multiple files.
Linked Issues check ✅ Passed The PR fully addresses issue #12662 requirements: MultiCombo serialization now emits multi_select as an object config, backward compatibility is maintained, validation handles list values, and regression tests are included.

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


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.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@execution.py`:
- Around line 997-1002: The current validation treats any list-valued input
(val) as valid if all elements are in combo_options, which lets {"__value__":
["a","b"]} slip through for single-select combos; change the logic in the
MultiCombo validation to first check the combo's config/multi flag (e.g.,
combo_config.get("multi") or is_multi) and only perform per-element validation
when that flag is true—otherwise treat a list as an invalid whole value (use the
existing scalar-path check for non-multi combos). Ensure you update the branch
that computes invalid_vals (currently using val, combo_options, invalid_vals) to
enforce this multi-check so single-select combos reject list inputs.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 0b0f5c93-50d1-4481-ae68-e72bf9f2f5c2

📥 Commits

Reviewing files that changed from the base of the PR and between 15e0974 and fc51d13.

📒 Files selected for processing (1)
  • execution.py

Comment thread execution.py Outdated
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.

3 participants