Comfy-Org / Comfy-Org/ComfyUI_frontend
Asset sidebar bulk-insertion spec: fixture duplication, route leak, and locator hygiene
- 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
Assessment
This issue has not been assessed yet.