Comfy-Org / Comfy-Org/ComfyUI_frontend

WidgetSelectDropdown stringifies numeric model values in the asset adapter, untested

Open
#14,643 1 comment 1 reaction 1 assignee Claimed by @jaeone94 View on GitHub
area:widgets Potential Bug
Dominant language
TypeScript
Stars
2k
Forks
699
Avg merge
1d 7h
Merged PRs (30d)
490

Description

### Summary

`WidgetSelectDropdown`'s `stringModelValue` adapter stringifies a numeric model value before it reaches `useWidgetSelectItems` and `useWidgetSelectActions`, where the pre-#14460 code forwarded the raw value. This is the one behavior in the adapter that no test covers.

Non-blocking. Surfaced while reviewing #14460 and filed separately rather than held against it, since the adapter is a net improvement over the computed it replaced.

### Detail

#14460 removed `WidgetSelect.vue`'s `assetModelValue` computed, which folded any non-string model value (including `null`) to `undefined` and thereby fired `WidgetSelectDropdown`'s `defineModel` default, fabricating a selection the user never made. That fold is correctly gone.

The replacement widens `WidgetSelectDropdown`'s model to `WidgetValue` and adapts it for the two consumers that still want a string:

```ts
const stringModelValue = computed({
get: () => {
const value = modelValue.value
return value == null ? undefined : String(value)
},
set: (value: string | undefined) => {
modelValue.value = value
}
})
```

For `null` and `undefined` this is exactly right, and `WidgetSelect.asset-mode.test.ts` pins it.

For a number it is a third behavior, distinct from both prior states:

| model value | pre-#14460 (`v-model="modelValue"`) | mid-#14460 (`assetModelValue`) | current |
| --- | --- | --- | --- |
| `null` | `null` forwarded, nothing selected | `undefined`, default fires, first item selected | `undefined`, nothing selected |
| `512` | `512` forwarded | `undefined`, default fires, first item selected | `'512'` forwarded |

The current column is the desired behavior for the `null` row and an untested change for the number row.

### Why this is low priority

The dropdown branch is `isDropdownUIWidget`, which is `isAssetMode || assetKind !== 'unknown'`, and `assetKind` derives from `image_upload` / `animated_image_upload` / `video_upload`. Asset combo values are filenames, so a numeric model value on this branch should not occur in practice. Filing it because "should not occur" is the same assumption that produced the `null` bug this PR fixed, and because the asymmetry between the two consumers and the `set` path is easy to misread later: `set` is typed `string | undefined` and writes strings back, so a numeric value that entered the widget cannot round-trip.

### Suggested resolution

Either confirm the numeric case is unreachable and add a one-line test asserting the dropdown branch is never entered with a non-string value, or preserve the raw value for numbers the way `null` is now preserved. Whichever is chosen, the reasoning is worth a test rather than a comment.

### References

- PR #14460
- `src/renderer/extensions/vueNodes/widgets/components/WidgetSelectDropdown.vue`
- `src/renderer/extensions/vueNodes/widgets/components/WidgetSelect.asset-mode.test.ts`

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.