Comfy-Org / Comfy-Org/ComfyUI_frontend
migrateWidgetsValues has zero test coverage: neutralising the pre-v1.16 forceInput migration leaves the full suite green
- Dominant language
- TypeScript
- Stars
- 2k
- Forks
- 699
- Avg merge
- 1d 7h
- Merged PRs (30d)
- 490
Description
## Problem / Goal
`migrateWidgetsValues` — the function that strips pre-v1.16 `forceInput` dummy values out of
`widgets_values` on **every** `ComfyNode.configure()` — has **no test coverage at all**. Replacing its
entire body with `return widgetsValues` leaves the full unit suite byte-identically green.
This is a data-correctness path: it decides which on-disk value lands on which widget when an old
workflow is opened. If it silently stops working, every pre-v1.16 workflow with a `forceInput` input
loads with every widget after that input shifted by one, and nothing in CI says a word.
Mutation-verified at `origin/main` `296fc5cd07`, full suite, 16,590 tests, 0 failures in the mutant
arm.
## Provenance
* **Found by:** widget-serialisation audit · re-derived against `origin/main` `296fc5cd07` on
2026-08-23
* **How:** grepped for `migrateWidgetsValues` across `src/**` including every `*.test.ts` — zero test
references; then neutralised the function and ran the full suite twice (baseline and mutant) in a
worktree with all eight `node_modules` present and a passing positive control
* **Confidence:** verified by execution
* **Related but distinct:** #15727 covers the two `serialize` flags. This is the `forceInput`
migration and a different function.
## Evidence
All anchors opened at `origin/main` `296fc5cd07`.
**The function**, `src/utils/litegraphUtil.ts:187-207`:
```ts
export function migrateWidgetsValues(
inputDefs: Record,
widgets: IBaseWidget[],
widgetsValues: TWidgetValue[]
): TWidgetValue[] {
const widgetNames = new Set(widgets.map((w) => w.name)) // :192
const originalWidgetsInputs = Object.values(inputDefs).filter(
(input) => widgetNames.has(input.name) || input.forceInput // :194
)
const widgetIndexHasForceInput = originalWidgetsInputs.flatMap((input) =>
input.control_after_generate
? [!!input.forceInput, false] // :199
: [!!input.forceInput]
)
if (widgetIndexHasForceInput.length !== widgetsValues?.length) // :203
return widgetsValues
return widgetsValues.filter((_, index) => !widgetIndexHasForceInput[index]) // :206
}
```
**Both call sites are on the node-configure hot path**, `src/services/litegraphService.ts:487` and
`:590` — two `configure` overrides, each running for every node of every workflow load:
```ts
data.widgets_values = migrateWidgetsValues(
ComfyNode.nodeData.inputs,
this.widgets ?? [],
data.widgets_values ?? []
)
super.configure(data)
```
**Zero test references.** At `296fc5cd07`:
```
$ grep -rn "migrateWidgetsValues" src/ --include=*.test.ts ; echo "exit=$?"
exit=1
```
The only three hits anywhere under `src/` are the definition (`litegraphUtil.ts:187`) and the two
call sites (`litegraphService.ts:74` import, `:487`, `:590`).
### Mutation, executed at `origin/main` `296fc5cd07`
Replace `:203-206` with `return widgetsValues`, so the migration never happens:
```diff
- if (widgetIndexHasForceInput.length !== widgetsValues?.length)
- return widgetsValues
-
- return widgetsValues.filter((_, index) => !widgetIndexHasForceInput[index])
+ return widgetsValues
```
| Arm | Test Files | Tests |
| --- | --- | --- |
| baseline | `1 failed \| 1204 passed (1205)` | `16570 passed \| 7 expected fail \| 13 skipped (16590)` |
| mutant | `1 failed \| 1204 passed (1205)` | `16570 passed \| 7 expected fail \| 13 skipped (16590)` |
Byte-identical. The one failing **file** in both arms is the pre-existing
`scripts/skills/update-ai-attribution.test.ts` transform error, which contributes **0 tests** and
fails the same way at baseline — it is not related to this change. Counts are identical across arms,
so this is a detection failure and not suite drift.
Positive control before both arms: `vitest run src/lib/litegraph/src/LLink.test.ts` →
`Tests 3 passed (3)`.
### Characterisation measured directly, so you know what to pin
Calling the function directly at `296fc5cd07` with `inputDefs = { a: {forceInput: true}, b: {} }`:
| Arm | `widgets` | `widgetsValues` | Result |
| --- | --- | --- | --- |
| 1 | `[a, b]` | `['DUMMY', 7]` | `[7]` — dummy stripped, correct |
| 2 | `[a, b, preview(serialize:false)]` | `['DUMMY', 7]` | `[7]` — **still correct** |
| 3 | `[a, preview(serialize:false), b]` | `['DUMMY', , 7]` | `['DUMMY', undefined, 7]` — guard at `:203` bails, nothing migrated |
Arm 2 matters because it **retires a plausible-sounding hypothesis**: a trailing non-serialised
widget does *not* break the length guard. `LGraphNode.serialize` writes at the widget's own index
(`src/lib/litegraph/src/LGraphNode.ts:1088`) and skips `serialize === false`, so a trailing skip
simply shortens the array and the count still matches. Only a non-trailing skip leaves a hole
(arm 3), and a `widgets_values` array **with a hole** is not something a pre-v1.16 writer could have
produced — so arm 3 is a property of the function, **not a demonstrated production bug**. Pin all
three as characterisation; do not "fix" arm 3 as part of this issue.
## Proposed Solution
Add a `src/utils/litegraphUtil.test.ts` describe block (or extend the existing file if one is added
by the time you pick this up) covering `migrateWidgetsValues` directly. It is a pure function with no
LiteGraph runtime dependency — `IBaseWidget` only needs `name`, and `InputSpec` only needs `name`,
`type`, `forceInput` and `control_after_generate`, so `fromAny` from `@total-typescript/shoehorn`
(already a dev dependency, used throughout this repo's tests) is enough to build both.
**Test-only. No production behaviour should change.** If a test cannot be made to pass without
editing `litegraphUtil.ts`, that is a finding to report on this issue, not something to fix here.
## Acceptance Criteria
- [ ] Direct unit tests for `migrateWidgetsValues` covering, at minimum:
- a `forceInput` input whose dummy value **is** stripped (arm 1 above)
- a `control_after_generate` input, which contributes **two** entries to the index map
(`:198-200`) — this branch is currently completely unexercised
- an input present in `inputDefs` but **not** among the node's widgets and not `forceInput`,
which `:194` must exclude
- the length-mismatch guard at `:203` returning the input array untouched
- a node with no `forceInput` inputs at all, where the result must equal the input
- [ ] Each test verified to go **red** under the neutralising mutation above. A test that passes both
ways proves nothing — report which of your tests fail under the mutant and with what message
- [ ] At least one test that pins arm 2 (trailing `serialize: false` widget does **not** trip the
guard), because that is the case a reader is most likely to assume is broken
- [ ] Full suite still `16570 passed | 7 expected fail | 13 skipped` plus your new tests, and
`NODE_OPTIONS=--max-old-space-size=8192 vue-tsc --noEmit` exits 0
## Out of scope
* **Changing `migrateWidgetsValues`.** This issue adds detectors, it does not change behaviour. The
arm-3 hole case in particular is characterisation, not a bug to fix here.
* **`widget.serialize` / `widget.options.serialize`.** Covered by #15727. Do not fold them in.
* **`LGraphNode.serialize` / `LGraphNode.configure` index handling**, and anything touching the
compacted-vs-full-index read. There is a separate open PR in that area; leave it alone.
* **`browser_tests/`.** Unit tests only — the e2e suite needs a running ComfyUI backend and cannot be
used to satisfy the fails-before/passes-after gate.
* **Anything on `feature/ecs-migration`.** This issue is about `main` only.
## Working notes for whoever picks this up
Traps in this repo that will otherwise cost you a cycle:
* **This is a pnpm workspace with eight `node_modules` directories** — the root plus
`apps/desktop-ui`, `apps/website`, and
`packages/{design-system,object-info-parser,tailwind-utils,shared-frontend-utils,ingest-types}`.
With only the root linked, `vitest` dies at `Failed to resolve import "clsx"` **at every commit**,
which makes any comparison meaningless.
* **Run a positive control before trusting any red result.**
`vitest run src/lib/litegraph/src/LLink.test.ts` should report `Tests 3 passed (3)`. Note the path:
it is `src/lib/litegraph/src/`, not `src/lib/litegraph/test/` — the latter does not exist and
vitest exits **0** with `No test files found`, which reads as a pass.
* **The one failing test file at baseline is expected.** `scripts/skills/update-ai-attribution.test.ts`
fails to transform at `296fc5cd07` and contributes 0 tests. Do not try to fix it and do not read it
as your change breaking something.
* **Vitest 4 has no `basic` reporter.** `--reporter=basic` exits 1 with
`Failed to load custom Reporter from basic` before running anything. Omit the flag.
* **There is no root `vitest.config.ts`.** The config is `vite.config.mts`, read by default.
* **`vue-tsc --noEmit` OOMs at Node's 2GB default heap and exits 134**, and a grep filtered for
`error TS` then returns zero — byte-identical to a clean run. Always pass
`NODE_OPTIONS=--max-old-space-size=8192` and always report the exit code.
* **`oxlint`'s exit code is ambiguous in both directions.** Most rules here run at warning level, so
exit 0 does not mean no findings; it also exits 1 when every input path is ignored
(`No files found to lint`). Read the output, not just `$?`.
Contributor guide
Assessment
This issue has not been assessed yet.