Comfy-Org / Comfy-Org/ComfyUI_frontend

Asset sidebar bulk-insertion spec: fixture duplication, route leak, and locator hygiene

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

Description

Follow-up to #14126 (merged). Grouped test-hygiene nitpicks noted in the approving review.

## Problem / Goal

Four small items in `browser_tests/tests/sidebar/assetsSidebarTab.spec.ts` and `src/utils/createAnnotatedPath.test.ts`. None affect correctness today; each is a small durability or drift risk.

1. **`bulkInsertionTest` duplicates mock setup and never unroutes.** It re-implements the four mock calls the `FE-130` describe already performs, making a third copy of that block in one file, and it omits the `try/finally { await page.unroute(...) }` discipline the sibling `jobOutputInsertionFixture.ts` established. A change to `mockViewFiles`/`generatedJobs` semantics now has to be reasoned about in three places, and the routes leak into fixture teardown.
2. **Hardcoded English menu label.** `tab.contextMenuItem('Insert all assets as nodes')` matches a raw string, so an i18n copy edit breaks the test with a locator timeout that reads like a UI regression rather than a copy change. Consistent with the existing `'Delete'` usage in the same file, so this is house style rather than a new problem.
3. **`dispatchEvent('contextmenu')` instead of a real right-click.** This bypasses actionability checks and pointer positioning (`clientX`/`clientY` are 0), so it exercises the handler rather than the interaction and would not catch an overlay intercepting the card. `click({ button: 'right' })` is the Playwright-recommended form — though the synthetic path may have been chosen deliberately for PrimeVue menu-positioning stability, in which case a one-line comment saying so would prevent someone "fixing" it.
4. **`stringCases` lacks the `satisfies` annotation its sibling has.** `resultItemCases` ends with `satisfies { name: string; item: ResultItem; expected: string }[]`; `stringCases` has none, so `options` infers as a union of shapes and a typo'd key (`rootfolder`) would compile silently.

## Proposed Solution

- Extract a single `assetsSidebarPage(page, { jobs, inputFiles, viewFiles })` setup helper consumed by all three describes, wrapping `use(page)` in `try/finally` with `page.unroute` for symmetry with `jobOutputInsertionFixture.ts`
- Source the menu label from the i18n messages rather than a literal
- Either switch to `click({ button: 'right' })` or add a comment explaining why the synthetic event is required
- Add the matching `satisfies` annotation to `stringCases`

## Acceptance Criteria

- [ ] One shared setup helper; no third copy of the mock block in the file
- [ ] Routes are unrouted in a `finally` so they cannot leak into teardown
- [ ] An i18n copy change to the bulk-insert menu item does not break the spec
- [ ] A typo'd key in `stringCases` options fails typecheck

---
Raised in review of #14126.

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.