Conversation
Talmaj
requested review from
Kosinkadink,
alexisrolland,
comfyanonymous,
guill,
kijai and
rattus128
as code owners
📝 WalkthroughWalkthroughAdds 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@comfy_api/latest/_io.py`:
- Line 1366: The row_count calculation at the line with max(min_rows,
present_rows) does not enforce the max parameter that was serialized in
as_dict(). Read the max parameter from the List.Input object and modify the
row_count assignment to cap it so that row_count respects both the minimum
constraint (min_rows) and the maximum constraint (max parameter). This ensures
that row_count falls within the allowed range and prevents the backend from
accepting row indices beyond the specified maximum.
🪄 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: cd605340-c975-4ad9-8610-e97f9f52618f
📒 Files selected for processing (2)
comfy_api/latest/_io.pytests-unit/comfy_api_test/io_list_test.py
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests-unit/comfy_api_test/io_dynamic_group_test.py (1)
198-204: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression coverage for
maxoverflow.The implementation now rejects live rows beyond
max, but this error path is not covered. A small test locks the backend cap contract.Proposed test
def test_no_leftover_flat_keys(self): """Flat keys must be consumed; only the reconstructed list remains.""" inp = DynamicGroup.Input("rows", template=[Float.Input("x", default=0.0)], min=0, max=5) result = _run(inp, {"rows.0.x": 1.0, "rows.1.x": 2.0}) assert "rows.0.x" not in result assert "rows.1.x" not in result assert isinstance(result["rows"], list) + + def test_rows_beyond_max_raise(self): + inp = DynamicGroup.Input("rows", template=[Float.Input("x", default=0.0)], min=0, max=2) + + with pytest.raises(ValueError): + get_finalized_class_inputs(_make_class_inputs(inp), {"rows.2.x": 1.0})🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests-unit/comfy_api_test/io_dynamic_group_test.py` around lines 198 - 204, Add a regression test in io_dynamic_group_test around DynamicGroup.Input/_run that covers the max overflow path: create a group with a small max and submit more indexed rows than allowed, then assert the backend rejects the extra row(s) instead of reconstructing them. Reuse the existing test helpers and symbols like test_no_leftover_flat_keys, DynamicGroup.Input, and _run so the new case sits alongside the current flat-key coverage and verifies the cap contract explicitly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@comfy_api/latest/_io.py`:
- Around line 1305-1308: The DynamicGroup ID validation currently checks only
for uniqueness, but it also needs to reject any “.” characters in both group IDs
and template field IDs because slot_id encoding and path.split(".") decoding
will rebuild the wrong nested row shape. Update the DynamicGroup validation
logic around field_ids and the related DynamicGroup ID handling paths to assert
that neither the group id nor any template field id contains ".", and surface a
clear validation error before rows are encoded or decoded.
---
Nitpick comments:
In `@tests-unit/comfy_api_test/io_dynamic_group_test.py`:
- Around line 198-204: Add a regression test in io_dynamic_group_test around
DynamicGroup.Input/_run that covers the max overflow path: create a group with a
small max and submit more indexed rows than allowed, then assert the backend
rejects the extra row(s) instead of reconstructing them. Reuse the existing test
helpers and symbols like test_no_leftover_flat_keys, DynamicGroup.Input, and
_run so the new case sits alongside the current flat-key coverage and verifies
the cap contract explicitly.
🪄 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 Plus
Run ID: 7cdfa785-547c-4784-a1bd-881bbd413d05
📒 Files selected for processing (2)
comfy_api/latest/_io.pytests-unit/comfy_api_test/io_dynamic_group_test.py
Comment on lines
+1305
to
+1308
| field_ids = [t.id for t in template] | ||
| assert len(field_ids) == len(set(field_ids)), ( | ||
| f"DynamicGroup template field ids must be unique within a row. Got: {field_ids}" | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Reject . in DynamicGroup IDs and template field IDs.
slot_id is encoded with "." and later decoded with path.split("."); a group or field id containing . reconstructs into a different nested shape than the documented list[dict] row contract.
Proposed validation
super().__init__(id, display_name, optional, tooltip, lazy, extra_dict)
+ assert "." not in id, "DynamicGroup id must not contain '.'."
# Validate template entries: only WidgetInput subclasses, no nesting
assert len(template) > 0, "DynamicGroup template must have at least one field."
for t in template:
assert isinstance(t, WidgetInput), (
f"DynamicGroup template field '{t.id}' must be a WidgetInput subclass "
f"(Combo, Float, Int, String, Boolean, Color). Got {type(t).__name__}."
)
+ assert "." not in t.id, f"DynamicGroup template field id '{t.id}' must not contain '.'."
assert not isinstance(t, DynamicInput), (
f"DynamicGroup template field '{t.id}' must not be a DynamicInput. "
"Nesting dynamic inputs inside DynamicGroup is not supported."
)Also applies to: 1375-1382, 1942-1962
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@comfy_api/latest/_io.py` around lines 1305 - 1308, The DynamicGroup ID
validation currently checks only for uniqueness, but it also needs to reject any
“.” characters in both group IDs and template field IDs because slot_id encoding
and path.split(".") decoding will rebuild the wrong nested row shape. Update the
DynamicGroup validation logic around field_ids and the related DynamicGroup ID
handling paths to assert that neither the group id nor any template field id
contains ".", and surface a clear validation error before rows are encoded or
decoded.
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
1 participant
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.
io.DynamicGroup– repeatable widget-group inputWhat this adds
A new dynamic input type
io.DynamicGroup(COMFY_DYNAMICGROUP_V3) that lets a node declare a repeatable group of widget fields. The frontend can render N rows of those fields; the backend reconstructs them into alist[dict]before callingexecute.Previously,
Autogrowcould repeat a single widget N times andDynamicCombocould show a different sub-schema per combo value, but nothing could repeat a group of widgets.io.DynamicGroupfills that gap.Example
How it works
DynamicGroup.Inputserializes atemplate(standard V1 input dict),min, andmaxinto/object_info.COMFY_DYNAMICGROUP_V3, renders stackable rows, submits flat keys:loras.0.lora_name,loras.0.strength,loras.1.lora_name, …DynamicGroup._expand_schema_for_dynamicscanslive_inputsto determine the current row count, registers each{groupId}.{row}.{fieldId}slot inrequired/optional, and records the group root inlist_paths.build_nested_inputscollects the flat values into{0: {…}, 1: {…}}, then a post-pass converts everylist_pathsentry to a sortedlist[dict].Parameters
templatelist[WidgetInput]– the fields in each rowmin0max50Limitations
comfyui-frontend-package. Until then the schema is served correctly but rows won't stack in the UI.WidgetInputsubclasses (Combo,Float,Int,String,Boolean,Color). Plain socket inputs and nested dynamic types (DynamicGroup,Autogrow,DynamicCombo) are not allowed.io.DynamicGroupcannot be nested inside anotherio.DynamicGrouporAutogrow.Files changed
comfy_api/latest/_io.py–DynamicGroupclass,DynamicPathsDefaultValue.EMPTY_LIST,list_pathsinV3Data/get_finalized_class_inputs, list post-pass inbuild_nested_inputs, registered insetup_dynamic_input_funcs, exported in__all__tests-unit/comfy_api_test/io_dynamic_group_test.py– 18 unit tests covering schema construction, 0-row, N-row, sort order,minenforcement, partial values, and no-leftover-flat-keys