Comfy-Org / Comfy-Org/ComfyUI_frontend

Badges always render top-right is the stated compat contract and has no test anywhere in the repo

Open
#15,619 0 comments 0 reactions 1 assignee Claimed by @DrJKL View on GitHub
area:testing documentation
Dominant language
TypeScript
Stars
2k
Forks
699
Avg merge
1d 7h
Merged PRs (30d)
490

Description

`ecs-extension-compatibility-audit.md:52` and `:122` make "badges always render top-right" the compatibility contract that replaced configurable badge placement. `ecs-verification-audit.md:256` cites `src/lib/litegraph/src/nodeBadgeDraw.test.ts` as the evidence for it, under an **Extension behavior: Good** grade.

That file asserts nothing about placement, and neither does anything else in the repository.

## The cited file

`src/lib/litegraph/src/nodeBadgeDraw.test.ts` at `5002fae1b12d44831a21367afa7c0f798f7e7a2c` is 66 lines, one `describe`, four tests, 14 `expect`s:

- `:13` core parts join in id, lifecycle, source order
- `:26` joined core text truncates to 31 chars
- `:36` credits rows draw separately with their icon
- `:54` draw objects are reused until the rows array is replaced

No position, `x`, `y`, corner, or offset assertion. No `node.badges` extension-row input.

Note the audit cites it as `src/lib/litegraph/src/canvas/nodeBadgeDraw.test.ts`; the real path has no `canvas/` segment.

## Control

The grep is live: the same file returns 14 hits for `expect(`.

Repo-wide, `grep -inE "top.?right|badgePosition|TOP_RIGHT"` over `src/` returns only `src/components/card/Card.stories.ts` (a `showTopRight` slot prop on an unrelated component). So the property is untested everywhere, not merely miscited.

## Why this one is worth a test rather than a doc edit

Configurable badge placement was a real public capability. This branch removes it: `badgePosition` becomes an ignored accessor, and `BadgePosition` (the enum) is dropped from the public barrel entirely (#15587). The replacement promise to extension authors is "always top-right".

That promise currently has no detector. If a future change reintroduces a position offset, or moves derived rows relative to extension rows, nothing goes red and the regression reaches extension authors as a visual defect.

## Suggested

A unit test in `src/lib/litegraph/src/nodeBadgeDraw.test.ts` asserting the computed draw position is the node's top-right corner for both a derived row and an extension row, and that assignment to `node.badgePosition` does not move it. Mutation-verify it: perturb the offset, confirm exactly that test fails, revert, confirm green.

The second half of the same audit line, "retained extension badge rows", **is** covered, at `src/systems/badgeSystem.usePartitionedBadges.test.ts:127` (`'appends non-empty node.badges extension badges after derived rows'`). The audit should cite that file for it.

## Related citation errors in the same document

Found in the same pass, all misattribution rather than absence, and all cheap to fix:

- `:67` — `useNodeLayout.test.ts` is cited for "workflow replacement releases the originally retained graph". It has one test (`:60`) that asserts a rendered x goes 200 then 300 and says nothing about release. There is no retain/release concept in `useNodeLayout.ts`. Cited under **Lifecycle teardown: Strong**.
- `:182` — `workflowService.insertWorkflow.test.ts` is cited for "direct insertion preserves topology and subgraph definitions". Its fixture is `links: []`, its only `subgraph` token is a `vi.mock`, and its third test asserts a mock call argument rather than post-insertion state.
- `:137` — graph isolation is in `widgetValueStore.test.ts:301`, not `widgetValueStore.graphReactivity.test.ts`.
- `:246` — "unknown keys" is in `NodeInputSlot.test.ts:135` / `NodeOutputSlot.test.ts:128`, not `slotEcosystemPatterns.test.ts`.
- `:254` — prototype enumeration and deprecated `type` mutation are in `LGraphNode.nodeState.test.ts:62,160`, not `LGraphNode.test.ts`.

Also worth stating in the doc: `performance.spec.ts` has no budget assertions. Every perf test ends in `recordMeasurement(m)` plus a `console.log`, and the file's single `expect` (`:203`) is a functional drag check. That includes `:160` "large graph legacy node drag", cited as Performance evidence. Consistent with the doc's own `Partial` grade, but the doc should say the measurement exists and the budget does not.

## Provenance

Denominators, so this is not overstated: **73 claims checked in `ecs-verification-audit.md`, 68 confirmed, 5 false.** All 65 cited test files exist and none contain `it.skip` / `it.todo` / `test.fixme`. The document is largely sound; this is the one claim where the property is absent from the repo rather than merely miscited.

Detail: https://github.com/Comfy-Org/ComfyUI_frontend/pull/14246#pullrequestreview-4998972476

Related: #15587 (`BadgePosition` barrel removal), #15618 (slot-accessor contract wrong in seven docs).

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.