Comfy-Org / Comfy-Org/ComfyUI_frontend
WidgetSelectDropdown stringifies numeric model values in the asset adapter, untested
- 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
Assessment
This issue has not been assessed yet.