mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-05-25 07:05:26 +00:00
c1bfb9cc7a0ea7ed2e5b8065e265ca456173cefa
976 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Johnpaul Chiwetelu
|
beaa269a63 |
feat: polish settings dialog layout and keybinding display (#11241)
Polish keybinding display. Based on #11212 with adjustments: left-aligned content (no centering), key uppercase moved to UI layer. - Reduce settings content font size to 14px - Increase spacing between setting sections with cleaner dividers - Consistent min-height for form items (toggle, slider, dropdown) - Capitalize keybinding badges via CSS `uppercase` instead of mutating data model - Remove '+' separator between keybinding badges - Unbold keybinding badges with fixed min-width Supersedes #11212 ┆Issue is synchronized with this [Notion page](https://app.notion.com/p/PR-11241-feat-polish-settings-dialog-layout-and-keybinding-display-3426d73d3650812a97e4d941a76a4fe9) by [Unito](https://www.unito.io) --------- Co-authored-by: Alex <alex@Mac.localdomain> Co-authored-by: github-actions <github-actions@github.com> |
||
|
Kelly Yang
|
cf98013c18 |
test: expand Image Crop E2E and fix loading overlay deadlock (#11193)
## Summary
Expands Playwright coverage for the **ImageCropV2** widget (Levels 1–3
from the image crop E2E plan), fixes **loading / image mount** behavior
when `imageUrl` changes, adds **stable resize-handle selectors**, and
adds a **small Vitest** for URL→loading transitions.
## Changes
- [x] **Level 1 (E2E)** — Empty state: assert resize handle hidden;
screenshot baseline `image-crop-empty-state.png`; pointer drag on empty
state does not change widget bounds.
- [x] **Level 1 (E2E)** — After run: assert **8 visible** resize handles
with `data-testid` + `filter({ visible: true })`; broken `img.src`
returns to empty state (`crop-empty-state`, no overlay).
- [x] **Level 1 (E2E)** — **Slow `/api/view`** route (delay only
`example.png`) to assert **“Loading…”** then hidden after image loads;
comment clarifies delay is in the route handler, not
`page.waitForTimeout`.
- [x] **Level 2 (E2E)** — Drag clamps to **right/bottom** and
**top-left** image bounds via `setCropBounds` + `expect.poll` on natural
bounds.
- [x] **Level 3 (E2E)** — Free resize: right / left / bottom / top
edges; SE and NW corners; `MIN_CROP_SIZE` (16px); right-edge boundary
clamp; **8 handles** screenshot `image-crop-eight-handles.png`; SE/NW
screenshots (`image-crop-resize-se.png`, `image-crop-resize-nw.png`).
- [x] **E2E helpers** — Shared `getCropValue`, `setCropBounds`,
`dragOnLocator`, `POINTER_OPTS`; drag regression uses **`expect.poll`**
instead of `toPass` where appropriate.
- [x] **`WidgetImageCrop.vue`** — When `imageUrl` is set, **always
render `<img>`**; show loading as an **absolute overlay** (fixes
deadlock where `isLoading` blocked `<img>` so `@load` never ran); add
**`data-testid="crop-resize-{direction}"`** on resize handles.
- [x] **`useImageCrop.ts`** — Watch `imageUrl` and drive `isLoading`;
extract **`imageCropLoadingAfterUrlChange`** (`boolean | null`) for
clear semantics and tests.
- [x] **`useImageCrop.test.ts`** — Vitest coverage for
`imageCropLoadingAfterUrlChange` (null URL, URL change, first URL,
unchanged URL).
## Screenshot / CI notes
- [ ] **Linux screenshot expectations** for new/updated
`toHaveScreenshot(...)` names must be produced on **CI (Linux)** — add
the **`New Browser Test Expectation`** label (or equivalent workflow);
**do not** commit local **Darwin** golden files.
- [x] Existing Linux baselines under `imageCrop.spec.ts-snapshots/` for
prior tests are unchanged where applicable; new baselines are expected
from CI after merge workflow.
## Files
- [x] `browser_tests/tests/vueNodes/widgets/imageCrop.spec.ts`
- [x] `src/components/imagecrop/WidgetImageCrop.vue`
- [x] `src/composables/useImageCrop.ts`
- [x] `src/composables/useImageCrop.test.ts` (new)
<!-- CURSOR_SUMMARY -->
---
> [!NOTE]
> **Medium Risk**
> Touches interactive crop UI rendering and `isLoading` state
transitions, which can affect user-visible behavior and input handling;
changes are mitigated by extensive new E2E and unit tests.
>
> **Overview**
> Improves the `WidgetImageCrop` loading behavior by always rendering
the preview `<img>` when `imageUrl` is set and showing “Loading…” as an
absolute overlay, preventing a deadlock where `isLoading` could block
the `@load` event. Adds stable `data-testid="crop-resize-{direction}"`
selectors for resize handles and hardens pointer-capture handling in
`useImageCrop`.
>
> Greatly expands automated coverage: the Playwright spec now tests
empty-state rendering/screenshot, drag/resize interactions (edge/corner,
min size, and clamping to image bounds), aspect-ratio lock handle
visibility, slow `/api/view` loading overlay behavior, and broken image
fetch recovery. Adds a new Vitest suite for `useImageCrop` (including
`imageCropLoadingAfterUrlChange`) to unit-test URL→loading transitions
and core drag/resize/aspect-ratio logic.
>
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
|
||
|
AustinMroz
|
e28c1e7e43 |
Show multitype slices of shared color (#11250)
A tiny update requested by the backend team. Previously, multitype slot indicators would have inputs that resolve to the same connection color display be combined into a single slice. For example, both `INT` and `FLOAT` have the same color, so an `INT,FLOAT` slot displays as a solid color instead of 2 semi-circles. This was a conscientious decision to improve readability on slots that allow many types, but meant that the more common cases (like `INT,FLOAT`) would have no indicator at all. Since priority is given to types based on order of listing, node authors can still control which types are elided on a slot accepting many types. <img width="430" height="320" alt="image" src="https://github.com/user-attachments/assets/1fc7fb1c-a634-487c-bc03-711637aeef13" /> - I do not believe there are any core nodes affected by this change. - The vue implementation of merging slot colors never functioned properly, but is still removed. - Vue was bugged to incorrectly pass slot types for widgets. This is also fixed. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11250-Show-multitype-slices-of-shared-color-3436d73d365081b6b484ea74423435a1) by [Unito](https://www.unito.io) |
||
|
AustinMroz
|
3fd3c565ae |
Fix dropdown chevron color (#11335)
Updates the the color of the chevron on dropdown widgets to only have the disabled color when the widget is disabled. | Before | After | | ------ | ----- | | <img width="360" alt="before" src="https://github.com/user-attachments/assets/25d35e78-9147-4397-be19-df9d6f87ac72" /> | <img width="360" alt="after" src="https://github.com/user-attachments/assets/3bf3640d-fa14-42cb-afb4-7109eb878d1a" />| ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11335-Fix-dropdown-chevron-color-3456d73d3650819e99c7d15173f11319) by [Unito](https://www.unito.io) --------- Co-authored-by: github-actions <github-actions@github.com> |
||
|
Kelly Yang
|
29d6263fb9 |
test: add Preview3D execution flow E2E tests (#11014)
## Summary
Adds Playwright coverage for `Preview3D execution` and persistence :
real queue execution against a `Load3D → Preview3D` workflow, plus `save
/ full reload / reopen` from the sidebar.
## What these tests do
**Fixture** (every test)
Turns on Vue Nodes, uses the sidebar for workflows, loads a Load3D →
Preview3D workflow, waits for nodes, then clears saved workflows after
the test so runs stay isolated.
**Test 1 — execution updates Preview3D**
Uploads `cube.obj`(the existing test file in the merged version) to
Load3D, runs `Queue Prompt`, then checks that Preview3D’s model_file and
Last Time Model File match and the canvas has non-zero size. No 3D
screenshots (GPU flakiness).
**Test 2 — persistence after reload**
Same upload + queue, then saves the workflow, reloads the page,
re-applies the same UI settings, opens the saved workflow, and checks
the same model path and camera state (with a small numeric tolerance).
<!-- CURSOR_SUMMARY -->
---
> [!NOTE]
> **Medium Risk**
> Adds new slow, WebGL-dependent E2E tests and fixtures, which can
increase CI runtime and introduce flakiness due to timing/graphics
variability, but does not change production logic.
>
> **Overview**
> Adds a new `Load3D → Preview3D` workflow asset and a dedicated
Playwright fixture (`Preview3DPipelineFixture`) to drive real queue
execution, upload a 3D model, and interact with the 3D canvases (orbit
drags) while asserting `model_file`/`Last Time Model File` and camera
state via node properties.
>
> Introduces camera-state comparison helpers with explicit numeric
tolerances, and adds a new `preview3dExecution.spec.ts` suite that
validates (1) Preview3D updates from execution output and (2) model +
camera persistence across save, full page reload, and reopening the
workflow from the sidebar.
>
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
|
||
|
jaeone94
|
a1e6fb36d2 |
refactor: harden ChangeTracker lifecycle with self-defending API (#10816)
## Summary Harden the `ChangeTracker` lifecycle to eliminate the class of bugs where an inactive workflow's tracker silently captures the wrong graph state. Renames `checkState()` to `captureCanvasState()` with a self-defending assertion, introduces `deactivate()` and `prepareForSave()` lifecycle methods, and closes a latent undo-history corruption bug discovered during code review. ## Background ComfyUI supports multiple workflows open as tabs, but only one canvas (`app.rootGraph`) exists at a time. When the user switches tabs, the old workflow's graph is unloaded and the new one is loaded into this shared canvas. The old `checkState()` method serialized `app.rootGraph` into `activeState` to track changes for undo/redo. It had no awareness of *which* workflow it belonged to -- if called on an inactive tab's tracker, it would capture the active tab's graph data and silently overwrite the inactive workflow's state. This caused permanent data loss (fixed in PR #10745 with caller-side `isActive` guards). The caller-side guards were fragile: every new call site had to remember to add the guard, and forgetting would reintroduce the same silent data corruption. Additionally, `beforeLoadNewGraph` only called `store()` (viewport/outputs) without `checkState()`, meaning canvas state could be stale if a tab switch happened without a preceding mouseup event. ### Before (fragile) ``` saveWorkflow(workflow): if (isActive(workflow)) <-- caller must remember this guard workflow.changeTracker.checkState() <-- name implies "read", actually writes ... beforeLoadNewGraph(): activeWorkflow.changeTracker.store() <-- only saves viewport, NOT graph state ``` ### After (self-defending) ``` saveWorkflow(workflow): workflow.changeTracker.prepareForSave() <-- handles active/inactive internally ... beforeLoadNewGraph(): activeWorkflow.changeTracker.deactivate() <-- captures graph + viewport together ``` ## Changes - Rename `checkState` to `captureCanvasState` with active-tracker assertion - Add `deactivate()` and `prepareForSave()` lifecycle methods - Fix undo-history corruption: `captureCanvasState()` guarded by `_restoringState` - Fix viewport regression during undo: `deactivate()` skips `captureCanvasState()` during undo/redo but always calls `store()` to preserve viewport (regression from PR #10247) - Log inactive tracker warnings unconditionally at warn level (not DEV-only) - Deprecated `checkState()` wrapper for extension compatibility - Rename `checkState` to `captureCanvasState` in `useWidgetSelectActions` composable - Add `appModeStore.ts` to manual call sites documentation - Add `checkState()` deprecation note to architecture docs - Add 16 unit tests covering all guard conditions, lifecycle methods, and undo behavior - Add E2E test: "Undo preserves viewport offset" ## New ChangeTracker Public API | Method | Caller | Purpose | |--------|--------|---------| | `captureCanvasState()` | Event handlers, UI interactions | Snapshots canvas into activeState, pushes undo. Asserts active tracker. | | `deactivate()` | `beforeLoadNewGraph` only | `captureCanvasState()` (skipped during undo/redo) + `store()`. Freezes state for tab switch. | | `prepareForSave()` | Save paths only | Active: `captureCanvasState()`. Inactive: no-op. | | `checkState()` | **Deprecated** -- extensions only | Wrapper that delegates to `captureCanvasState()` with deprecation warning. | | `store()` | Internal to `deactivate()` | Saves viewport, outputs, subgraph navigation. | | `restore()` | `afterLoadNewGraph` | Restores viewport, outputs, subgraph navigation. | | `reset()` | `afterLoadNewGraph`, save | Resets initial state (marks as "clean"). | ## Test plan - [x] Unit tests: 16 tests covering all guard conditions, state capture, undo queue behavior - [x] E2E test: "Undo preserves viewport offset" verifies no viewport drift on undo - [x] E2E test: "Prevents captureCanvasState from corrupting workflow state during tab switch" - [x] Existing E2E: "Closing an inactive tab with save preserves its own content" - [ ] Manual: rapidly switch tabs during undo/redo, verify no viewport drift - [ ] Manual: verify extensions calling `checkState()` see deprecation warning in console |
||
|
jaeone94
|
394e36984f |
fix: re-sync collapsed node slot positions after subgraph fitView (#11240)
## Summary Fix collapsed node connection links rendering at wrong positions when entering a subgraph for the first time. `fitView()` (added in #10995) changes canvas scale/offset, invalidating cached slot positions for collapsed nodes. ## Changes - **What**: Schedule `requestSlotLayoutSyncForAllNodes()` on the next frame after `fitView()` in `restoreViewport()` so collapsed node slot positions are re-measured against the updated transform. Inner RAF guarded against mid-frame graph changes. - **Test coverage**: - Unit tests in `subgraphNavigationStore.viewport.test.ts` verify the RAF chain calls `requestSlotLayoutSyncForAllNodes` after `fitView`, and skip the re-sync when the active graph changes between frames. - E2E screenshot test (`@screenshot` tag) validates correct link rendering on first subgraph entry using a new fixture with a pre-collapsed inner node. ## Review Focus The nested `requestAnimationFrame` is intentional: the outer RAF runs `fitView()`, which updates `ds.scale`/`ds.offset` and triggers a CSS transform update on `TransformPane`. The inner RAF ensures the DOM has reflowed with the new transform before `requestSlotLayoutSyncForAllNodes()` measures `getBoundingClientRect()` on slot elements. --------- Co-authored-by: github-actions <github-actions@github.com> |
||
|
Dante
|
19fff29204 |
test: backfill e2e coverage gaps for toolkit widgets, minimap, mask editor, painter (#11183)
## Summary Backfills missing e2e test coverage identified in the [FixIt Burndown](https://www.notion.so/comfy-org/FixIt-Burndown-32e6d73d365080609a81cdc9bc884460) audit. Adds 39 new behavioral tests across 5 spec files with zero test-code overlap. ## Changes - **What**: New e2e specs for Image Crop (6 tests) and Curve Widget (6 tests). Deepened coverage for Minimap (+6), Mask Editor (+10), Painter (+11). - **New fixtures**: `curve_widget.json`, updated `image_crop_widget.json` ## Test Inventory | Spec | New tests | Coverage area | |---|---|---| | `imageCrop.spec.ts` | 6 | Empty state, bounding box inputs, ratio selector/presets, lock toggle, programmatic value update | | `curveWidget.spec.ts` | 6 | SVG render, click-to-add point, drag-to-reshape, Ctrl+click remove, interpolation mode switch, min-2 guard | | `minimap.spec.ts` | +6 | Click-to-pan, drag-to-pan, zoom viewport shrink, node count changes, workflow reload, pan state reflection | | `maskEditor.spec.ts` | +10 | Brush drawing, undo/redo, clear, cancel, invert, Ctrl+Z, tool panel/switching, brush settings, save with mock, eraser | | `painter.spec.ts` | +11 | Clear, eraser, control visibility toggle, brush size slider, stroke width comparison, canvas dimensions, background color, multi-stroke accumulate, color picker, opacity, partial erase | ## Review Focus - Mask editor tests use `.maskEditor_toolPanelContainer` class selectors — may need test-id hardening later - Painter slider interaction tests could be flaky if slider layout changes - All canvas pixel-count assertions use `expect.poll()` with timeouts for reliability ## Test plan - [ ] CI passes all new/modified specs - [ ] No duplicate coverage with existing tests (verified via grep before writing) - [ ] No `waitForTimeout` usage (confirmed) ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11183-test-backfill-e2e-coverage-gaps-for-toolkit-widgets-minimap-mask-editor-painter-3416d73d3650819ca33edd1f27b9651a) by [Unito](https://www.unito.io) --------- Co-authored-by: GitHub Action <action@github.com> |
||
|
Johnpaul Chiwetelu
|
b3b895a2a9 |
refactor(test): use canvasOps.clickEmptySpace in copyPaste spec (#10991)
## Summary
Replace two hardcoded blank-canvas click positions in
`copyPaste.spec.ts` with the existing
`comfyPage.canvasOps.clickEmptySpace()` helper.
## Changes
- **What**: Both `{ x: 50, y: 500 }` click literals in the `Copy paste
node, image paste onto LoadImage, image paste on empty canvas` test now
use `canvasOps.clickEmptySpace()` (which wraps
`DefaultGraphPositions.emptySpaceClick = { x: 35, y: 31 }`). Redundant
`await nextFrame()` calls dropped — the helper already awaits a frame
internally.
## Review Focus
Draft PR — need CI to confirm `(35, 31)` is a valid blank-canvas click
for the `load_image_with_ksampler` workflow used by this test. The
workflow places `LoadImage` at `[50, 50]` and `KSampler` at `[500, 50]`,
so `(35, 31)` should be clear of both. Locally the test was already
failing on `main` (pre-existing, unrelated), so CI is the source of
truth here. If CI fails, the fallback is to add a dedicated named
constant `emptyCanvasClick: { x: 50, y: 500 }` to
`DefaultGraphPositions` as originally proposed in the issue.
Fixes #10330
┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-10991-refactor-test-use-canvasOps-clickEmptySpace-in-copyPaste-spec-33d6d73d3650817aa3ccea44cb48c0ae)
by [Unito](https://www.unito.io)
|
||
|
Dante
|
e5c81488e4 |
fix: include focusMode in splitter refresh key to prevent panel resize (#11295)
## Summary When the properties panel is open, toggling focus mode on then off causes the panel to resize unexpectedly. The root cause is that `splitterRefreshKey` in `LiteGraphCanvasSplitterOverlay.vue` does not include `focusMode`, so the PrimeVue Splitter component instance is reused across focus mode transitions and restores stale panel sizes from localStorage. Fix: add `focusMode` to `splitterRefreshKey` so the Splitter is recreated when focus mode toggles. ## Red-Green Verification | Commit | CI Status | Purpose | |--------|-----------|---------| | `test: add failing test for focus mode toggle resizing properties panel` | 🔴 Red | Proves the test catches the bug | | `fix: include focusMode in splitter refresh key to prevent panel resize` | 🟢 Green | Proves the fix resolves the bug | ## demo ### AS IS https://github.com/user-attachments/assets/95f6a9e3-e4c7-4aba-8e17-0eee11f70491 ### TO BE https://github.com/user-attachments/assets/595eafcd-6a80-443d-a6f3-bb7605ed0758 ## Test Plan - [ ] CI red on test-only commit - [ ] CI green on fix commit - [ ] E2E regression test added in `browser_tests/tests/focusMode.spec.ts` ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11295-fix-include-focusMode-in-splitter-refresh-key-to-prevent-panel-resize-3446d73d365081b7bc3ac65338e17a8f) by [Unito](https://www.unito.io) |
||
|
pythongosssss
|
ecb6fbe8fb |
test: Add waitForWorkflowIdle & remove redundant nextFrame (#11264)
## Summary More cleanup and reliability ## Changes - **What**: - Add wait for idle - Remove redundant nextFrames ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11264-test-Add-waitForWorkflowIdle-remove-redundant-nextFrame-3436d73d3650812c837ac7503ce0947b) by [Unito](https://www.unito.io) Co-authored-by: Alexander Brown <drjkl@comfy.org> |
||
|
Alexander Brown
|
52ccd9ed1a |
refactor: internalize nextFrame() into fixture/helper methods (#11166)
## Summary
Internalize `nextFrame()` calls into fixture/helper methods so spec
authors don't need to remember to call it after common operations.
`nextFrame()` waits for one `requestAnimationFrame` (~16ms) — an extra
call is always safe, making this a low-risk refactor.
## Changes
### Phase 1: `SettingsHelper.setSetting()`
`setSetting()` now calls `nextFrame()` internally. Removed 15 redundant
calls across 7 files.
### Phase 2: `CommandHelper.executeCommand()`
`executeCommand()` now calls `nextFrame()` internally. Removed 15
redundant calls across 7 files, including the now-redundant call in
`AppModeHelper.toggleAppMode()`.
### Phase 3: `WorkflowHelper.loadGraphData()`
New helper wraps `page.evaluate(loadGraphData)` + `nextFrame()`.
Migrated `SubgraphHelper.serializeAndReload()` and `groupNode.spec.ts`.
### Phase 4: `NodeReference` cleanup
Removed redundant `nextFrame()` from `copy()`, `convertToGroupNode()`,
`resizeNode()`, `dragTextEncodeNode2()`, and
`convertDefaultKSamplerToSubgraph()`. Removed 6 spec-level calls after
`node.click('title')`.
### Phase 5: `KeyboardHelper.press()` and `delete()`
New convenience methods that press a key and wait one frame. Converted
40 `canvas.press(key)` + `nextFrame()` pairs across 13 spec files.
### Phase 6: `ComfyPage.expectScreenshot()`
New helper combines `nextFrame()` + `toHaveScreenshot()`. Converted 45
pairs across 12 spec files.
## Total impact
- **~130 redundant `nextFrame()` calls eliminated** across ~35
spec/helper files
- **3 new helper methods** added (`loadGraphData`, `press`/`delete`,
`expectScreenshot`)
- **2 existing methods** enhanced (`setSetting`, `executeCommand`)
## What was NOT changed
- `performance.spec.ts` frame-counting loops (intentional)
- `ComfyMouse.ts` / `CanvasHelper.ts` (already internalized)
- `SubgraphHelper.packAllInteriorNodes()` (deliberate orchestration)
- Builder helpers (already internalized)
┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-11166-refactor-internalize-nextFrame-into-fixture-helper-methods-33f6d73d3650817bb5f6fb46e396085e)
by [Unito](https://www.unito.io)
---------
Co-authored-by: Amp <amp@ampcode.com>
|
||
|
Kelly Yang
|
92ad6fc798 |
test: address review nits for image compare E2E (#11260)
## Summary
A follow-up PR of #11196.
| # | Nit | Action | Reason |
| :--- | :--- | :--- | :--- |
| 1 | Replace `page.on('pageerror')` with request-wait | **Left as-is**
| The `pageErrors` array is an accumulator checked at the end via
`expect(pageErrors).toHaveLength(0)` – the goal is to assert that broken
image URLs don't surface as uncaught JS exceptions during the test run.
A request-wait can't substitute for that behavioral assertion, so the
listener pattern is intentional here. |
| 2 | Move helpers to a `vueNodes.getImageCompareHelper()` subclass |
**Left as-is** | Helpers such as `setImageCompareValue` and
`moveToPercentage` are only used in this file, making local
encapsulation enough. Extracting them to a page object would increase
the file/interface surface area and violate YAGNI; additionally,
`AGENTS.md` clearly states to "minimize the exported values of each
module. |
| 3 | Use `TestIds` enum for test ID strings | **Fixed** – added
`imageCompare` section to `TestIds` in `selectors.ts`; replaced all 8
inline string IDs in `imageCompare.spec.ts` with
`TestIds.imageCompare.*` references | The project already has a
`TestIds` convention for centralizing test IDs. Inline strings create
drift risk between the Vue component and the test file. |
| 4 | Move `expect.poll` bounding box check to helper/page object |
**Left as-is** | This logic already lives inside `moveToPercentage`,
which is a local helper. Moving it further to a page object is the same
refactor as #2 above. |
| 5 | Remove `// ---` style section header comments | **Fixed** –
removed all 8 divider blocks from `imageCompare.spec.ts` | Consistent
with project guidelines and your explicit preference. Test names already
describe what each block does. |
| 6 | Name magic numbers `400` and `350` | **Fixed** – introduced
`minWidth = 400` and `minHeight = 350` constants in the test |
Descriptive names make the constraint self-documenting and easier to
update if the workflow asset changes. |
<!-- CURSOR_SUMMARY -->
---
> [!NOTE]
> **Low Risk**
> Low risk: changes are limited to Playwright E2E test code and shared
selector constants, with no production logic impacted.
>
> **Overview**
> **E2E Image Compare tests now use centralized selectors.** Adds an
`imageCompare` section to `TestIds` and updates `imageCompare.spec.ts`
to reference `TestIds.imageCompare.*` instead of inline `data-testid`
strings.
>
> Cleans up the spec by removing divider comments and naming the minimum
size magic numbers (`minWidth`, `minHeight`) used in the node sizing
assertion.
>
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
|
||
|
pythongosssss
|
06686a1f50 |
test: App mode - additional app mode coverage (#11194)
## Summary Adds additional test coverage for empty state/welcome screen/connect outputs/vue nodes auto switch ## Changes - **What**: - add tests ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11194-test-App-mode-additional-app-mode-coverage-3416d73d365081ca91d0ed61de19f840) by [Unito](https://www.unito.io) |
||
|
jaeone94
|
693b8383d6 |
fix: missing-asset correctness follow-ups from #10856 (#11233)
Follow-up to #10856. Four correctness issues and their regression tests. ## Bugs fixed ### 1. ErrorOverlay model count reflected node selection `useErrorGroups` exposed `filteredMissingModelGroups` under the public name `missingModelGroups`. `ErrorOverlay.vue` read that alias to compute its model count label, so selecting a node shrank the overlay total. The overlay must always show the whole workflow's errors. Exposed both shapes explicitly: `missingModelGroups` / `missingMediaGroups` (unfiltered totals) and `filteredMissingModelGroups` / `filteredMissingMediaGroups` (selection-scoped). `TabErrors.vue` destructures the filtered variant with an alias. Before https://github.com/user-attachments/assets/eb848c5f-d092-4a4f-b86f-d22bb4408003 After https://github.com/user-attachments/assets/75e67819-c9f2-45ec-9241-74023eca6120 ### 2. Bypass → un-bypass dropped url/hash metadata Realtime `scanNodeModelCandidates` only reads widget values, so un-bypass produced a fresh candidate without the url that `enrichWithEmbeddedMetadata` had previously attached from `graphData.models`. `MissingModelRow`'s download/copy-url buttons disappeared after a bypass/un-bypass cycle. Added `enrichCandidateFromNodeProperties` that copies `url`/`hash`/`directory` from the node's own `properties.models` — which persists across mode toggles — into each scanned candidate. Applied to every call site of the per-node scan. A later fix in the same branch also enforces directory agreement to prevent a same-name / different-directory collision from stamping the wrong metadata. Before https://github.com/user-attachments/assets/39039d83-4d55-41a9-9d01-dec40843741b After https://github.com/user-attachments/assets/047a603b-fb52-4320-886d-dfeed457d833 ### 3. Initial full scan surfaced interior errors of a muted/bypassed subgraph container `scanAllModelCandidates`, `scanAllMediaCandidates`, and the JSON-based missing-node scan only check each node's own mode. Interior nodes whose parent container was bypassed passed the filter. Added `isAncestorPathActive(rootGraph, executionId)` to `graphTraversalUtil` and post-filter the three pipelines in `app.ts` after the live rootGraph is configured. The filter uses the execution-ID path (`"65:63"` → check node 65's mode) so it handles both live-scan-produced and JSON-enrichment-produced candidates. Before https://github.com/user-attachments/assets/3032d46b-81cd-420e-ab8e-f58392267602 After https://github.com/user-attachments/assets/02a01931-951d-4a48-986c-06424044fbf8 ### 4. Bypassed subgraph entry re-surfaced interior errors `useGraphNodeManager` replays `graph.onNodeAdded` for each existing interior node when the Vue node manager initializes on subgraph entry. That chain reached `scanSingleNodeErrors` via `installErrorClearingHooks`' `onNodeAdded` override. Each interior node's own mode was active, so the caller guards passed and the scan re-introduced the error that the initial pipeline had correctly suppressed. Added an ancestor-activity gate at the top of `scanSingleNodeErrors`, the single entry point shared by paste, un-bypass, subgraph entry, and subgraph container activation. A later commit also hardens this guard against detached nodes (null execution ID → skip) and applies the same ancestor check to `isCandidateStillActive` in the realtime verification callback. Before https://github.com/user-attachments/assets/fe44862d-f1d6-41ed-982d-614a7e83d441 After https://github.com/user-attachments/assets/497a76ce-3caa-479f-9024-4cd0f7bd20a4 ## Tests - 6 unit tests for `isAncestorPathActive` (root, active, immediate-bypass, deep-nested mute, unresolvable ancestor, null rootGraph) - 4 unit tests for `enrichCandidateFromNodeProperties` (enrichment, no-overwrite, name mismatch, directory mismatch) - 1 unit test for `scanSingleNodeErrors` ancestor guard (subgraph entry replaying onNodeAdded) - 2 unit tests for `useErrorGroups` dual export + ErrorOverlay contract - 4 E2E tests: - ErrorOverlay model count stays constant when a node is selected (new fixture `missing_models_distinct.json`) - Bypass/un-bypass cycle preserves Copy URL button (uses `missing_models_from_node_properties`) - Loading a workflow with bypassed subgraph suppresses interior missing model error (new fixture `missing_models_in_bypassed_subgraph.json`) - Entering a bypassed subgraph does not resurface interior missing model error (shares the above fixture) `pnpm typecheck`, `pnpm lint`, 206 related unit tests passing. ## Follow-up Several items raised by code review are deferred as pre-existing tech debt or scope-avoided refactors. Tracked via comments on #11215 and #11216. --- Follows up on #10856. |
||
|
Christian Byrne
|
033b3dad3a |
feat: add Slack notification workflow for coverage improvements (#10977)
## Summary
Adds a GitHub Actions workflow + TypeScript script that posts to Slack
when a merged PR improves unit or E2E test coverage.
## Changes
- **What**: New `coverage-slack-notify.yaml` workflow triggered on push
to main. Compares current coverage against previous baselines, generates
Slack Block Kit payload with progress bars and milestone celebrations,
posts to `#p-frontend-automated-testing`.
- **Script**: `scripts/coverage-slack-notify.ts` — parses lcov files,
computes deltas, detects milestone crossings (every 5%), builds Slack
payload. Pure functions exported for testability.
- **Tests**: 26 unit tests in `scripts/coverage-slack-notify.test.ts`
covering all pure functions including edge cases (malformed lcov, exact
boundaries, zero coverage).
### Security hardening
- All `${{ }}` expressions moved from `run:` blocks to `env:` variables
- `SLACK_BOT_TOKEN` passed via env var, not inline
- Unique heredoc delimiter (timestamp-based) prevents payload injection
- `parseInt` fallback (`|| 0`) guards against malformed lcov
- PR regex anchored to first line of commit message
### Robustness
- `continue-on-error: true` on Slack post step (outage does not fail the
job)
- Baseline save guarded by `steps.unit-tests.outcome == success`
(prevents corrupt baselines on test failure)
- Channel ID commented for maintainability
- Top-level `text` field added for Slack mobile push notifications
- Author linked to GitHub profile instead of bare `@username`
## Review Focus
- Workflow step ordering and conditional logic
- Security of expression handling and secret management
- Slack payload structure and Block Kit formatting
┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-10977-feat-add-Slack-notification-workflow-for-coverage-improvements-33d6d73d3650819c8950f483c83f297c)
by [Unito](https://www.unito.io)
---------
Co-authored-by: GitHub Action <action@github.com>
|
||
|
Kelly Yang
|
66e8d570e7 |
test: expand Image Compare E2E and stabilize widget selectors (#11196)
## Summary
This PR extends **Image Compare** browser tests, adds **stable
`data-testid` hooks** on `WidgetImageCompare.vue`, and aligns slider
interactions with the **same viewport element** used by
`useMouseInElement`, with **layout polling** to reduce flakes.
## What this PR does
- [x] Adds **`data-testid="image-compare-viewport"`** on the compare
area root (the `containerRef` div) so E2E targets the real slider hit
region instead of a long Tailwind class chain or the `<img>` box alone.
- [x] Adds **`data-testid="image-compare-empty"`** on the no-images
branch so the empty state can be asserted **without hard-coded English**
UI text.
- [x] Adds a **smoke** test that the widget **shows both images and the
drag handle** after value injection, with **`waitForImagesLoaded`** (no
extra full-node screenshot to avoid duplicating the default-50 golden).
- [x] Extends slider coverage to **clamp at both edges** (**0%** and
**~100%**) using the viewport locator and **`expect.poll`** until
**`boundingBox()`** is valid before reading coordinates.
- [x] Updates **`moveToPercentage`** to **`expect.poll`** until the
target locator has a **non-empty layout box** before moving the mouse.
- [x] Routes **hover**, **preserve position**, and **25% / 75%
screenshot** mouse moves through **`image-compare-viewport`**
(consistent with `containerRef`).
- [x] Adds an E2E assertion that the **workflow ImageCompare node** size
is **at least 400×350** (matches the widget workflow asset).
- [x] Hardens the **broken image** case: **`page.on('pageerror')`** /
**`page.off`** in **`finally`**, **`http://127.0.0.1:1/...`** URLs for
fast failure, **`expect.soft`** on key UI invariants, and a hard
assertion that **no page errors** were recorded.
- [x] Extends the **large batch (20×20)** test to **page to the last
index** and assert **counters `20 / 20`**, **previous enabled**, and
**next disabled** on both sides.
- [x] Renames the clamp test title to use an **ASCII hyphen** (`0-100%`)
for easier grepping.
## Out of scope (unchanged in this PR)
- [ ] Replacing **`setImageCompareValue`**’s **`page.evaluate`** setup
with a full UI-driven path (would be a larger follow-up).
## Suggested title
`test: expand Image Compare E2E and stabilize widget selectors`
<!-- CURSOR_SUMMARY -->
---
> [!NOTE]
> **Low Risk**
> Primarily test and selector-hook changes; low production risk, with
only minor DOM attribute additions that could affect external test
tooling if relied upon.
>
> **Overview**
> Improves Image Compare Playwright coverage and reduces flakiness by
driving slider interactions through a new stable
`data-testid="image-compare-viewport"` hook and polling for a valid
layout box before mouse moves.
>
> Updates assertions to avoid localized text (new
`data-testid="image-compare-empty"`), adds smoke coverage that
images/handle render after value injection, validates slider clamping at
both 0% and ~100%, and extends screenshot tests to use the same viewport
target.
>
> Hardens edge-case tests by ensuring broken image loads don’t raise
uncaught `pageerror`s, adds a minimum node size assertion, and extends
large-batch navigation checks through the final index and
disabled/enabled nav states.
>
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
|
||
|
Dante
|
634d57809b |
test: add E2E tests for bottom panel core behaviors (#10814)
## Summary - Add `browser_tests/tests/bottomPanel.spec.ts` with tests for behaviors not covered by existing `bottomPanelLogs` and `bottomPanelShortcuts` specs - Tests cover: close button (X), tab persistence on re-open, resize gutter visibility and drag, canvas interaction when panel is closed, cross-panel switching (terminal <-> shortcuts), and registered tab enumeration - Extend `BottomPanel` fixture with `closeButton` and `resizeGutter` locators ## Test plan - [ ] `pnpm test:browser:local` passes all new tests in `bottomPanel.spec.ts` - [ ] Existing `bottomPanelLogs.spec.ts` and `bottomPanelShortcuts.spec.ts` are unaffected - [ ] `pnpm typecheck:browser` passes - [ ] `pnpm lint` passes ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-10814-test-add-E2E-tests-for-bottom-panel-core-behaviors-3366d73d365081ea9b90c643897845fa) by [Unito](https://www.unito.io) --------- Co-authored-by: dante <dante@danteui-MacStudio.local> Co-authored-by: GitHub Action <action@github.com> Co-authored-by: Alexander Brown <drjkl@comfy.org> |
||
|
Dante
|
5807e03c74 |
test: expand E2E coverage for toolbox actions (#10968)
## Summary - Adds `selectionToolboxMoreActions.spec.ts` with E2E coverage for previously untested toolbox actions - Covers: pin/unpin, minimize/expand, adjust size, copy, duplicate, refresh button, align (top/left), distribute (horizontal/vertical), alignment options hidden for single selection, multi-node bypass toggle - Part of the FixIt Burndown test coverage initiative (toolbox actions) ## Test plan - [ ] All new tests pass in CI - [ ] No regressions in existing selectionToolbox tests ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-10968-test-expand-E2E-coverage-for-toolbox-actions-33c6d73d3650811286cefdd0eb4f5242) by [Unito](https://www.unito.io) --------- Co-authored-by: Alexander Brown <drjkl@comfy.org> |
||
|
Dante
|
6847c7ba2d |
fix: store promoted widget values per SubgraphNode instance (#10849)
## Summary - Multiple SubgraphNode instances of the same blueprint share inner nodes, causing promoted widget values to collide — the last configured instance overwrites all previous values - Add per-instance value storage (`_instanceWidgetValues`) on SubgraphNode so each instance preserves its own promoted widget values independently - Restore `widgets_values` from serialized data into this per-instance map after promoted views are created during configure - Fixes #10146 ## Root Cause When loading a workflow with multiple SubgraphNode instances of the same blueprint: 1. `LGraph.configure()` creates ONE shared Subgraph per blueprint (line 2625) 2. Each SubgraphNode instance calls `configure(instanceData)` sequentially 3. `PromotedWidgetView.value` setter writes to the **shared inner node's widget** (`promotedWidgetView.ts:199`) 4. The last instance's `configure()` overwrites all previous instances' values **Regression**: Introduced by PR #8594 (WidgetValueStore, v1.41.3) which centralized widget state without per-instance scoping for shared blueprints. ## Fix - **SubgraphNode**: Add `_instanceWidgetValues` Map and `_pendingWidgetsValues` for configure-time restoration - **PromotedWidgetView getter**: Check instance map first before falling back to widget store / inner node - **PromotedWidgetView setter**: Write to instance map to avoid shared inner node mutation - **_internalConfigureAfterSlots**: Apply serialized `widgets_values` to per-instance map after promoted views are created ## Red-Green Verification | Commit | CI Status | Purpose | |--------|-----------|---------| | `test: add failing tests for multi-instance subgraph widget value collision` | 🔴 Red | Proves widget values collide across instances | | `fix: store promoted widget values per SubgraphNode instance` | 🟢 Green | Per-instance storage prevents collision | ## Test Plan - [x] CI red on test-only commit - [x] CI green on fix commit - [x] Unit test: `preserves promoted widget values after configure with different widgets_values` - [x] All 253 existing subgraph tests pass - [ ] Manual: load workflow from issue image → verify 3 subgraph instances produce different results ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-10849-fix-store-promoted-widget-values-per-SubgraphNode-instance-3386d73d3650815a8544f54adcc0d504) by [Unito](https://www.unito.io) --------- Co-authored-by: dante <dante@danteui-MacStudio.local> |
||
|
AustinMroz
|
988a546721 |
Add missing dialog tests (#11133)
Leveraging the fancy coverage functionality of #10930, this PR aims to add coverage to missing dialogue models. This has proven quite constructive as many of the dialogues have since been shown to be bugged. - The APINodes sign in dialog that displays when attempting to run a workflow containing Partner nodes while not logged in was intended to display a list of nodes required to execute the workflow. The import for this component was forgotten in the original commit (#3532) and the backing component was later knipped - Error dialogs resulting are intended to display the file responsible for the error, but the prop was accidentally left out during the refactoring of #3265 - ~~The node library migration (#8548) failed to include the 'Edit Blueprint' button, and had incorrect sizing and color on the 'Delete Blueprint' button.~~ - On request, the library button changes were spun out to a separate PR ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11133-Add-missing-dialog-tests-33e6d73d3650812cb142d610461adcd4) by [Unito](https://www.unito.io) --------- Co-authored-by: GitHub Action <action@github.com> |
||
|
pythongosssss
|
165984fe4c |
test: Improve speed of app mode input corruption test (#11236)
## Summary Speeds up test that was timing out https://9b579efd.comfyui-playwright-chromium.pages.dev/#?testId=b97e313f05078cede9be-5e6b75a76880fb6a5d96 ## Changes - **What**: - load prebuilt workflows to reduce test time (17s -> 11s) ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11236-test-Improve-speed-of-app-mode-input-corruption-test-3426d73d3650815b9475ec96dfbd7ad5) by [Unito](https://www.unito.io) |
||
|
pythongosssss
|
34a02a29c9 |
test: Remove unnecessary setup, UseNewMenu and waitForNodes calls (#11237)
## Summary More simplification ## Changes - **What**: - Remove more UseNewMenu settings calls - Remove `await comfyPage.setup()` - Remove `waitForNodes` in vue node tagged tests ┆Issue is synchronized with this [Notion page](https://app.notion.com/p/PR-11237-test-Remove-unnecessary-setup-UseNewMenu-and-waitForNodes-calls-3426d73d36508198a100c218420d479c) by [Unito](https://www.unito.io) |
||
|
pythongosssss
|
a09bb81b98 |
test: Auto wait for nodes after loadWorkflow in vue-node tests (#11238)
## Summary Updates tests to auto wait for vue-nodes when loading a workflow in a test with the vue-nodes tag ## Changes - **What**: - If tag includes vue-nodes, wait - Remove all load->wait calls ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11238-test-Auto-wait-for-nodes-after-loadWorkflow-in-vue-node-tests-3426d73d3650810e8760c5601186fde8) by [Unito](https://www.unito.io) |
||
|
Alexander Brown
|
72eed86cea |
test: remove redundant setup/settings now handled by @vue-nodes fixture (#11195)
## Summary Follow-up cleanup for #11184 — removes redundant test setup calls that the `@vue-nodes` fixture now handles. ## Changes - **What**: Remove 40 lines of redundant `setSetting`, `setup()`, and `waitForNodes()` calls across 11 test files - `UseNewMenu: 'Top'` calls (already fixture default) - `setup()` + `waitForNodes()` on default workflow (fixture already does this for `@vue-nodes`) - Page reload in `subgraphZeroUuid` (fixture applies VueNodes.Enabled server-side before navigation) ## Review Focus Each removal was verified against the fixture's `setupSettings()` defaults (ComfyPage.ts:420-442) and the `@vue-nodes` auto-setup (lines 454-456). Tests that call `setup()`/`waitForNodes()` after `loadWorkflow()` or `page.evaluate()` were intentionally kept. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11195-test-remove-redundant-setup-settings-now-handled-by-vue-nodes-fixture-3416d73d36508154827df116a97e9130) by [Unito](https://www.unito.io) Co-authored-by: Amp <amp@ampcode.com> |
||
|
pythongosssss
|
5899a9392e |
test: Simplify vue node/menu test setup (#11184)
## Summary Simplifies test setup for common settings ## Changes - **What**: - add vue-nodes tag to auto enable nodes 2.0 - remove UseNewMenu Top as this is default ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11184-test-Simplify-vue-node-menu-test-setup-3416d73d3650815487e0c357d28761fe) by [Unito](https://www.unito.io) |
||
|
pythongosssss
|
a373633ab2 |
refactor: fix lint errors in tests (#11182)
## Summary Fix tests failing lint ## Changes - **What**: - Fix relative imports - Fix test not using comfyPage ## Review Focus <!-- Critical design decisions or edge cases that need attention --> <!-- If this PR fixes an issue, uncomment and update the line below --> <!-- Fixes #ISSUE_NUMBER --> ## Screenshots (if applicable) <!-- Add screenshots or video recording to help explain your changes --> ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11182-refactor-fix-lint-errors-in-tests-3416d73d3650812fbf1bc88554c57de2) by [Unito](https://www.unito.io) |
||
|
jaeone94
|
521019d173 |
fix: exclude muted/bypassed nodes from missing asset detection (#10856)
## Summary Muted and bypassed nodes are excluded from execution but were still triggering missing model/media/node warnings. This PR makes the error system mode-aware: muted/bypassed nodes no longer produce missing asset errors, and all error lifecycle events (mode toggle, deletion, paste, undo, tab switch) are handled consistently. - Fixes Comfy-Org/ComfyUI#13256 ## Behavioral notes - **Tab switch overlay suppression (intentional)**: Switching back to a workflow with missing assets no longer re-shows the error overlay. This reverses the behavior introduced in #10190. The error state is still restored silently in the errors tab — users can access it via the properties panel without being interrupted by the overlay on every tab switch. ## Changes ### 1. Scan filtering - `scanAllModelCandidates`, `scanAllMediaCandidates`, `scanMissingNodes`: skip nodes with `mode === NEVER || BYPASS` - `collectMissingNodes` (serialized data): skip error reporting for muted/bypassed nodes while still calling `sanitizeNodeName` for safe `configure()` - `collectEmbeddedModelsWithSource`: skip muted/bypassed nodes; workflow-level `graphData.models` only create candidates when active nodes exist - `enrichWithEmbeddedMetadata`: filter unmatched workflow-level models when all referencing nodes are inactive ### 2. Realtime mode change handling - `useErrorClearingHooks.ts` chains `graph.onTrigger` to detect `node:property:changed` (mode) - Deactivation (active → muted/bypassed): remove missing model/media/node errors for the node - Activation (muted/bypassed → active): scan the node and add confirmed errors, show overlay - Subgraph container deactivation: remove all interior node errors (execution ID prefix match) - Subgraph container activation: scan all active interior nodes recursively - Subgraph interior mode change: resolve node via `localGraph.getNodeById()` then compute execution ID from root graph ### 3. Node deletion - `graph.onNodeRemoved`: remove missing model/media/node errors for the deleted node - Handle `node.graph === null` at callback time by using `String(node.id)` for root-level nodes ### 4. Node paste/duplicate - `graph.onNodeAdded`: scan via `queueMicrotask` (deferred until after `node.configure()` restores widget values) - Guard: skip during `ChangeTracker.isLoadingGraph` (undo/redo/tab switch handled by pipeline) - Guard: skip muted/bypassed nodes ### 5. Workflow tab switch optimization - `skipAssetScans` option in `loadGraphData`: skip full pipeline on tab switch - Cache missing model/media/node state per workflow via `PendingWarnings` - `beforeLoadNewGraph`: save current store state to outgoing workflow's `pendingWarnings` - `showPendingWarnings`: restore cached errors silently (no overlay), always sync missing nodes store (even when null) - Preserve UI state (`fileSizes`, `urlInputs`) on tab switch by using `setMissingModels([])` instead of `clearMissingModels()` - `MissingModelRow.vue`: fetch file size on mount via `fetchModelMetadata` memory cache ### 6. Undo/redo overlay suppression - `silentAssetErrors` option propagated through pipeline → `surfaceMissingModels`/`surfaceMissingMedia` `{ silent }` option - `showPendingWarnings` `{ silent }` option for missing nodes overlay - `changeTracker.ts`: pass `silentAssetErrors: true` on undo/redo ### 7. Error tab node filtering - Selected node filters missing model/media card contents (not just group visibility) - `isAssetErrorInSelection`: resolve execution ID → graph node for selection matching - Missing nodes intentionally unfiltered (pack-level scope) - `hasMissingMediaSelected` added to `RightSidePanel.vue` error tab visibility - Download All button: show only when 2+ downloadable models exist ### 8. New store functions - `missingModelStore`: `addMissingModels`, `removeMissingModelsByNodeId` - `missingMediaStore`: `addMissingMedia`, `removeMissingMediaByNodeId` - `missingNodesErrorStore`: `removeMissingNodesByNodeId` - `missingModelScan`: `scanNodeModelCandidates` (extracted single-node scan) - `missingMediaScan`: `scanNodeMediaCandidates` (extracted single-node scan) ### 9. Test infrastructure improvements - `data-testid` on `RightSidePanel.vue` tabs (`panel-tab-{value}`) - Error-related TestIds moved from `dialogs` to `errorsTab` namespace in `selectors.ts` - Removed unused `TestIdValue` type - Extracted `cleanupFakeModel` to shared `ErrorsTabHelper.ts` - Renamed `openErrorsTabViaSeeErrors` → `loadWorkflowAndOpenErrorsTab` - Added `aria-label` to pencil edit button and subgraph toggle button ## Test plan ### Unit tests (41 new) - Store functions: `addMissing*`, `removeMissing*ByNodeId` - `executionErrorStore`: `surfaceMissing*` silent option - Scan functions: muted/bypassed filtering, `scanNodeModelCandidates`, `scanNodeMediaCandidates` - `workflowService`: `showPendingWarnings` silent, `beforeLoadNewGraph` caching ### E2E tests (17 new in `errorsTabModeAware.spec.ts`) **Missing nodes** - [x] Deleting a missing node removes its error from the errors tab - [x] Undo after bypass restores error without showing overlay **Missing models** - [x] Loading a workflow with all nodes bypassed shows no errors - [x] Bypassing a node hides its error, un-bypassing restores it - [x] Deleting a node with missing model removes its error - [x] Undo after bypass restores error without showing overlay - [x] Pasting a node with missing model increases referencing node count - [x] Pasting a bypassed node does not add a new error - [x] Selecting a node filters errors tab to only that node **Missing media** - [x] Loading a workflow with all nodes bypassed shows no errors - [x] Bypassing a node hides its error, un-bypassing restores it - [x] Pasting a bypassed node does not add a new error - [x] Selecting a node filters errors tab to only that node **Subgraph** - [x] Bypassing a subgraph hides interior errors, un-bypassing restores them - [x] Bypassing a node inside a subgraph hides its error, un-bypassing restores it **Workflow switching** - [x] Does not resurface error overlay when switching back to workflow with missing nodes - [x] Restores missing nodes in errors tab when switching back to workflow # Screenshots https://github.com/user-attachments/assets/e0a5bcb8-69ba-4120-ab7f-5c83e4cfc3c5 ## Follow-up work - Extract error-detection computed properties from `RightSidePanel.vue` into a composable (e.g. `useErrorsTabVisibility`) --------- Co-authored-by: github-actions <github-actions@github.com> Co-authored-by: GitHub Action <action@github.com> |
||
|
Kelly Yang
|
bd82c855e0 |
test: add minimap E2E tests for graph content and click-to-navigate (#10738)
## Summary
Adds Playwright E2E tests verifying that
1. the minimap canvas renders node content
2. clears when the graph is empty
3. correctly navigates the main canvas on click
┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-10738-test-add-minimap-E2E-tests-for-graph-content-and-click-to-navigate-3336d73d365081eb955ce711b3efc57f)
by [Unito](https://www.unito.io)
<!-- CURSOR_SUMMARY -->
---
> [!NOTE]
> **Low Risk**
> Low risk: changes are limited to adding `data-testid` attributes to
the minimap UI and expanding Playwright E2E assertions, with no
production behavior changes expected.
>
> **Overview**
> Strengthens minimap E2E coverage by switching existing assertions from
CSS selectors to new `data-testid`-based selectors and adding helper
utilities for canvas/overlay interactions.
>
> Adds new Playwright tests that verify the minimap canvas renders
content when nodes exist, clears when the graph is emptied, and that
clicking the minimap pans the main canvas (including a
post-`fitViewToSelectionAnimated` tolerance check).
>
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
|
||
|
Kelly Yang
|
5b7ef3fe21 |
test: Painter Widget E2E Test Plan (#10846)
### Summary of Improvements
* **Custom Test Coverage Extension**: Enhanced the Painter widget E2E
test suite by refactoring logic for better maintainability and
robustness.
* **Stable Component Targeting**: Introduced
`data-testid="painter-dimension-text"` to `WidgetPainter.vue`, providing
a reliable, non-CSS-dependent locator for canvas size verification.
* **Improved Test Organization**: Reorganized existing test scenarios
into logical categories using `test.describe` blocks (Drawing, Brush
Settings, Canvas Size Controls, etc.).
* **Asynchronous Helper Integration**: Converted `hasCanvasContent` to
an asynchronous helper and unified its usage across multiple test cases
to eliminate redundant pixel-checking logic.
* **Locator Resilience**: Updated Reka UI slider interaction logic to
use more precise targeting (`:not([data-slot])`), preventing ambiguity
and improving test stability.
* **Scenario Refinement**: Updated the `pointerup` test logic to
accurately reflect pointer capture behavior when interactions occur
outside the canvas boundaries.
* **Enhanced Verification Feedback**: Added descriptive error messages
to `expect.poll` assertions to provide clearer context on potential
failure points.
* **Standardized Tagging**: Restored the original tagging strategy
(including `@smoke` and `@screenshot` tags) to ensure tests are
categorized correctly for CI environments.
### Red-Green Verification
| Commit | CI Status | Purpose |
| :--- | :--- | :--- |
| `test: refactor painter widget e2e tests and address review findings`
| 🟢 Green | Addresses all E2E test quality and stability issues from
review findings. |
### Test Plan
- [x] **Quality Checks**: `pnpm format`, `pnpm lint`, and `pnpm
typecheck` verified as passing.
- [x] **Component Integration**: `WidgetPainter.vue` `data-testid`
correctly applied and used in tests.
- [x] **Helper Reliability**: `hasCanvasContent` correctly identifies
colored pixels and returns a promise for `expect.poll`.
- [x] **Locator Robustness**: Verified Reka slider locators correctly
exclude internal thumb spans.
- [x] **Boundary Interaction**: Verified `pointerup` correctly ends
strokes when triggered outside the viewport.
- [x] **Tagging Consistency**: Verified `@smoke` and `@screenshot` tags
are present in the final test suite.
┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-10846-test-Painter-Widget-E2E-Test-Plan-3386d73d365081deb70fe4afbd417efb)
by [Unito](https://www.unito.io)
<!-- CURSOR_SUMMARY -->
---
> [!NOTE]
> **Low Risk**
> Primarily adds/refactors Playwright E2E tests and stable `data-testid`
hooks, with no changes to Painter drawing logic. Risk is limited to
potential test brittleness or minor UI attribute changes.
>
> **Overview**
> Expands the Painter widget Playwright suite with new grouped scenarios
covering drawing/erasing behavior, tool switching, brush inputs, canvas
resizing (including preserving drawings), clear behavior, and
serialization/upload flows (including failure toast).
>
> Refactors the tests to use a shared `@e2e/helpers/painter` module
(`drawStroke`, `hasCanvasContent`, `triggerSerialization`), improves
stability via role/testid-based locators and clearer `expect.poll`
messaging, and adds `data-testid` attributes (e.g.,
`painter-clear-button`, `painter-*-row`, `painter-dimension-text`) to
`WidgetPainter.vue` to avoid CSS-dependent selectors.
>
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
|
||
|
Kelly Yang
|
85de833776 |
test: add E2E tests for ImageCompare widget (#10767)
## Summary
Add E2E tests for ImageCompare widget
Covers slider interaction, batch navigation, single-image modes, visual
regression screenshots, and edge cases for the ImageCompare Vue node
widget.
┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-10767-test-add-E2E-tests-for-ImageCompare-widget-3346d73d365081c6bfc6fbd97fa04e4d)
by [Unito](https://www.unito.io)
<!-- CURSOR_SUMMARY -->
---
> [!NOTE]
> **Low Risk**
> Adds Playwright E2E coverage and screenshot assertions only; main risk
is increased CI runtime/flakiness due to additional image-loading and
hover/position polling.
>
> **Overview**
> Adds a new Playwright E2E suite for the ImageCompare Vue widget
(tagged `@widget`) that programmatically sets widget values and asserts
rendering for empty, single-image, and dual-image states.
>
> Expands coverage to **slider behavior** (default 50%, hover movement,
clamping, persistence) using polling on inline `clip-path`/handle
position, and adds **batch navigation** tests for multi-image
before/after sets.
>
> Introduces **visual regression screenshots** at default and specific
slider positions, plus edge-case tests for broken URLs, rapid updates
resetting batch index, legacy string values, and custom alt text.
>
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
|
||
|
Kelly Yang
|
cab46567c0 |
test: add E2E tests for ImageCropV2 widget (#10737)
## Summary
Adds Playwright E2E tests for the ImageCropV2 widget covering
1. the empty state (no source image)
2. default control rendering
3. source image display with crop overlay
4. drag-to-reposition behavior.
┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-10737-test-add-E2E-tests-for-ImageCropV2-widget-3336d73d365081b28ed9db63e5df383e)
by [Unito](https://www.unito.io)
<!-- CURSOR_SUMMARY -->
---
> [!NOTE]
> **Low Risk**
> Low risk: primarily adds Playwright E2E coverage and introduces
`data-testid` attributes for more stable selectors, with no changes to
core crop behavior.
>
> **Overview**
> Adds new Playwright E2E coverage for the `ImageCropV2` Vue-node
widget, including workflows/fixtures for a disconnected input and a
`LoadImage -> ImageCropV2 -> PreviewImage` pipeline.
>
> Tests validate the empty state and default controls, verify the crop
overlay renders after execution with screenshot assertions, and exercise
drag-to-reposition by dispatching pointer events and asserting the
widget’s crop value updates.
>
> Updates `WidgetImageCrop.vue` to add `data-testid` hooks (empty
state/icon and crop overlay) to make the E2E selectors stable.
>
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
|
||
|
Alexander Brown
|
6f579c5992 |
fix: enable playwright/no-force-option lint rule (#11164)
## Summary
Enable the previously disabled `playwright/no-force-option` lint rule at
error level and resolve all 29 violations across 10 files.
## Changes
### Lint rule
- `.oxlintrc.json`: `playwright/no-force-option` changed from `off` to
`error`
### Shared utility
- `CanvasHelper.ts`: Add `mouseClickAt()` and `mouseDblclickAt()`
methods that convert canvas-element-relative positions to absolute page
coordinates and use `page.mouse` APIs, avoiding Playwright's locator
actionability checks that fail when Vue DOM overlays sit above the
`<canvas>` element
### Force removal (20 violations)
- `selectionToolboxActions.spec.ts`: Remove `force: true` from 8 toolbox
button clicks (the `pointer-events: none` splitter overlay does not
intercept `elementFromPoint()`)
- `selectionToolboxSubmenus.spec.ts`: Remove `force: true` from 2
popover menu item clicks
- `BuilderSelectHelper.ts`: Remove `force: true` from 2 widget/node
clicks (builder mode does not disable pointer events)
- `linkInteraction.spec.ts`: Remove `force: true` from 3 slot `dragTo()`
calls (`::after` pseudo-elements do not intercept `elementFromPoint()`)
- `SidebarTab.ts`: Remove `force: true` from toast dismissal (`.catch()`
already handles failures)
- `nodeHelp.spec.ts`: Remove `force: true` from info button click
(preceding `toBeVisible()` assertion is sufficient)
### Rewrites (3 violations)
- `integerWidget.spec.ts`: Replace force-clicking disabled buttons with
`toBeDisabled()` assertions
- `Topbar.ts`: Replace force-click with `waitFor({ state: 'visible' })`
after hover
### Canvas coordinate clicks (9 violations)
- `litegraphUtils.ts`: Convert `NodeReference.click()` and
`navigateIntoSubgraph()` to use
`canvasOps.mouseClickAt()`/`mouseDblclickAt()`
- `subgraphPromotion.spec.ts`: Convert 3 right-click canvas calls to
`canvasOps.mouseClickAt()`
- `selectionToolboxSubmenus.spec.ts`: Convert 1 canvas dismiss-click to
`canvasOps.mouseClickAt()`
## Rationale
The original `force: true` usages were added defensively based on
incorrect assumptions about the `z-999 pointer-events: none` splitter
overlay intercepting Playwright's actionability checks. In reality,
`elementFromPoint()` skips elements with `pointer-events: none`, so the
overlay is transparent to Playwright's hit-test.
For canvas coordinate clicks, `force: true` on a locator does not tunnel
through DOM overlays — it only skips Playwright's preflight checks.
`page.mouse.click()` is the correct API for coordinate-based canvas
interactions.
┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-11164-fix-enable-playwright-no-force-option-lint-rule-33f6d73d365081e78601c6114121d272)
by [Unito](https://www.unito.io)
---------
Co-authored-by: Amp <amp@ampcode.com>
|
||
|
Alexander Brown
|
8c9328c1b2 |
feat: add eslint-plugin-playwright via oxlint JS plugins (#11136)
## Summary Add eslint-plugin-playwright as an oxlint JS plugin scoped to browser_tests/, enforcing Playwright best practices at lint time. ## Changes - **What**: Configure eslint-plugin-playwright@2.10.1 via oxlint's alpha `jsPlugins` field (`.oxlintrc.json` override scoped to `browser_tests/**/*.ts`). 18 recommended rules + `prefer-native-locators` + `require-to-pass-timeout` at error severity. All 173 initial violations resolved (config, auto-fix, manual fixes). `no-force-option` set to off — 28 violations need triage (canvas overlay workarounds vs unnecessary force) in a dedicated PR. - **Dependencies**: `eslint-plugin-playwright@^2.10.1` (devDependency, required by oxlint jsPlugins at runtime) ## Review Focus - `.oxlintrc.json` override structure — this is the first use of oxlint's JS plugins alpha feature in this repo - Manual fixes in spec files: `waitForSelector` → `locator.waitFor`, deprecated page methods → locator equivalents, `toPass()` timeout additions - Compound CSS selectors replaced with `.and()` (Playwright native locator composition) to avoid `prefer-native-locators` suppressions - Lint script changes in `package.json` to include `browser_tests/` in oxlint targets --------- Co-authored-by: Amp <amp@ampcode.com> Co-authored-by: GitHub Action <action@github.com> |
||
|
Johnpaul Chiwetelu
|
f0ae91de43 |
test: add e2e coverage for alt+drag duplicate and meta multi-select drag (#10994)
## Summary
Add Playwright coverage for two previously-untested canvas gestures:
Alt+drag to duplicate a regular node, and holding Meta across
click-click-click-drag to move multiple selected Vue nodes together.
## Changes
- **What**:
- `browser_tests/tests/interaction.spec.ts` — new `Node Duplication`
sub-describe with `Can duplicate a regular node via Alt+drag`. Asserts
CLIPTextEncode count goes 2 → 3 via poll, original node still exists.
Exercises the legacy canvas path at
`src/lib/litegraph/src/LGraphCanvas.ts:2434-2458`, which was only tested
for subgraph nodes before.
- `browser_tests/tests/vueNodes/interactions/node/move.spec.ts` — new
`should move all selected nodes together when dragging one with Meta
held`. Holds Meta for the entire sequence (3× `click({ modifiers:
['Meta'] })` + drag), asserts selection stays at 3 and all three nodes
move by the same delta. Exercises
`src/renderer/extensions/vueNodes/layout/useNodeDrag.ts:54-191`.
- Small refactor: `getLoadCheckpointHeaderPos` now delegates to a
reusable `getHeaderPos(comfyPage, title)` helper. Added `deltaBetween`
and `expectSameDelta` utilities (stricter than `expectPosChanged`, which
only checks `> 0`).
┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-10994-test-add-e2e-coverage-for-alt-drag-duplicate-and-meta-multi-select-drag-33d6d73d3650812dbf15c7053f44f0fd)
by [Unito](https://www.unito.io)
---------
Co-authored-by: Alexander Brown <drjkl@comfy.org>
|
||
|
pythongosssss
|
82fb8ce658 |
test: App mode - Execution tests (#10801)
## Summary Adds tests that simulate the execution flow and output feed ## Changes - **What**: - Add ExecutionHelper for mocking network activity - Refactor ws fixture to use Playwright websocket helper instead of patching window - ## Review Focus <!-- Critical design decisions or edge cases that need attention --> <!-- If this PR fixes an issue, uncomment and update the line below --> <!-- Fixes #ISSUE_NUMBER --> ## Screenshots (if applicable) <!-- Add screenshots or video recording to help explain your changes --> ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-10801-test-App-mode-Execution-tests-3356d73d365081e4acf0c34378600031) by [Unito](https://www.unito.io) |
||
|
Alexander Brown
|
0353524e6f |
refactor: standardize Page Object locators as public readonly instead of getters (#11135)
*PR Created by the Glary-Bot Agent* --- ## Summary - Convert ~120 getter-based locators across 18 browser test fixture files to `public readonly` constructor-assigned properties - Removes unnecessary indirection, makes object shape explicit, and improves IDE auto-complete / type inference - Keeps lazy-init getters (`??=`), computed properties, and `private get page()` convenience accessors as getters ## Changes **`browser_tests/fixtures/components/`** (6 files): `ComfyNodeSearchBox`, `ContextMenu`, `SettingDialog`, `SignInDialog`, `SidebarTab` (all 6 classes), `Topbar` **`browser_tests/fixtures/`** (4 files): `ComfyPage` (ComfyMenu.buttons, ComfyPage.visibleToasts), `UserSelectPage`, `ComfyMouse`, `VueNodeHelpers` **`browser_tests/fixtures/helpers/`** (7 files): `AppModeHelper`, `BuilderFooterHelper`, `BuilderSaveAsHelper`, `BuilderSelectHelper`, `BuilderStepsHelper`, `ToastHelper`, `NodeOperationsHelper` **`browser_tests/fixtures/utils/`** (1 file): `vueNodeFixtures` ## Validation - `pnpm typecheck` ✅ - `pnpm typecheck:browser` ✅ - `pnpm exec eslint browser_tests/fixtures/` ✅ - All pre-commit hooks pass (oxfmt, oxlint, eslint, typecheck, typecheck:browser) ✅ - No visual/manual verification needed — changes are test fixture locator declarations only (no UI or runtime behavior change) Fixes #11131 ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11135-refactor-standardize-Page-Object-locators-as-public-readonly-instead-of-getters-33e6d73d3650819690cbc639f3d30daf) by [Unito](https://www.unito.io) --------- Co-authored-by: Glary-Bot <glary-bot@users.noreply.github.com> |
||
|
Alexander Brown
|
f1bb756929 |
fix: remove redundant and counterproductive e2e timeout overrides (#11110)
## Summary Alright, alright, alright. These e2e tests have been runnin' around like they're late for somethin', settin' tight little timeouts like the world's gonna end in 250 milliseconds. Man, you gotta *breathe*. Let the framework do its thing. Go slow to go fast, that's what I always say. ## Changes - **What**: Removed ~120 redundant timeout overrides from auto-retrying Playwright assertions (`toBeVisible`, `toBeHidden`, `toHaveCount`, `toBeEnabled`, `toHaveAttribute`, `toContainText`, `expect.poll`) where 5000ms is already the default. Also removed sub-5s timeouts (1s, 2s, 3s) that were just *begging* for flaky failures — like wearin' a belt and suspenders and also holdin' your pants up with both hands. Raised the absurdly short timeouts in `customMatchers.ts` (250ms `toPass` → 5000ms, 256ms poll → default). Kept `timeout: 5000` on `.toPass()` calls (defaults to 0), `.waitFor()`, `waitForRequest`, `waitForFunction`, intentionally-short timeouts inside retry loops, and conditional `.isVisible()/.catch()` checks — those fellas actually need the help. ## Review Focus Every remaining timeout in the diff is there for a *reason*. The ones on `.toPass()` stay because that API defaults to zero — it won't retry at all without one. The ones on `.waitFor()` and `waitForRequest` stay because those are locator actions, not auto-retrying assertions. The intentionally-short ones inside `toPass` retry loops (`interaction.spec.ts`) and the negative assertions (`actionbar.spec.ts` confirming no response arrives) — those are *supposed* to be tight. The short timeouts on regular assertions were actively *encouragin'* flaky failures. That's like settin' your alarm for 4 AM and then gettin' mad you're tired. Just... don't do that, man. Let things take the time they need. 38 files, net -115 lines. Less code, more chill. That's livin'. --------- Co-authored-by: Amp <amp@ampcode.com> |
||
|
pythongosssss
|
e38dd1efae |
test: App mode - Add test for cross-workflow input corruption (#10944)
## Summary Adds test for the issue where inputs from one workflow would overwrite those on another after sorting ## Changes - **What**: - add test ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-10944-test-App-mode-Add-test-for-cross-workflow-input-corruption-33b6d73d365081deab26d8f23d2318ae) by [Unito](https://www.unito.io) |
||
|
Christian Byrne
|
4cf160d66e |
fix: disable pointer events on non-visible DOM widget overlays (#11063)
## Problem When a node with DOM widget overlays (e.g. CLIPTextEncode) is collapsed, the overlay elements can intercept pointer events intended for the canvas collapse toggler, making click-to-expand unreliable. ## Root Cause `updateWidgets()` runs during `onDrawForeground` (canvas render cycle) and sets `widgetState.visible = false` for collapsed nodes. `v-show` then hides the element with `display: none`. However, there is a timing gap between the canvas state change and Vue's DOM update — during this gap the widget overlay still intercepts pointer events. ## Fix Add `!widgetState.visible` to the `pointerEvents` condition in `composeStyle()`. This immediately sets `pointer-events: none` when the widget becomes invisible, preventing event interception before `v-show` applies `display: none`. Also restores click-to-expand in the E2E test, removing the programmatic `node.collapse()` workaround from PR #10967. - Fixes #11006 ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11063-fix-disable-pointer-events-on-non-visible-DOM-widget-overlays-33e6d73d36508179a83cd47121cf933f) by [Unito](https://www.unito.io) |
||
|
Alexander Brown
|
0132c77c7d |
test: harden 82 Playwright specs for deterministic CI runs (#10967)
## Summary
Harden 98 E2E spec files and 8 fixtures/helpers for deterministic CI
runs by replacing race-prone patterns with retry-safe alternatives.
No source code changes -- only `browser_tests/` is touched.
## Changes
- **E2E spec hardening** (98 spec files, 6 fixtures, 2 helpers):
| Fix class | Sites | Examples |
|-----------|-------|---------:|
| `expect(await ...)` -> `expect.poll()` | ~153 | interaction,
defaultKeybindings, workflows, featureFlags |
| `const x = await loc.count(); expect(x)` -> `toHaveCount()` | ~19 |
menu, linkInteraction, assets, bottomPanelShortcuts |
| `nextFrame()` -> `waitForHidden()` after menu clicks | ~22 |
contextMenu, rightClickMenu, subgraphHelper |
| Redundant `nextFrame()` removed | many | defaultKeybindings, minimap,
builderSaveFlow |
| `expect(async () => { ... }).toPass()` retry blocks | 5 | interaction
(graphdialog dismiss guard) |
| `force:true` removed from `BaseDialog.close()` | 1 | BaseDialog
fixture |
| ContextMenu `waitForHidden` simplified (check-then-act race removed) |
1 | ContextMenu fixture |
| Non-deterministic node order -> proximity-based selection | 1 |
interaction (toggle dom widget) |
| Tight poll timeout (250ms) -> >=2000ms | 2 | templates |
- **Helper improvements**: Exposed locator getters on
`ComfyPage.domWidgets`, `ToastHelper.toastErrors`, and
`WorkflowsSidebarTab.activeWorkflowLabel` so callers can use retrying
assertions (`toHaveCount()`, `toHaveText()`) directly.
- **Flake pattern catalog**: Added section 7 table to
`browser_tests/FLAKE_PREVENTION_RULES.md` documenting 8 pattern classes
for reviewers and future authors.
- **Docs**: Fixed bad examples in `browser_tests/README.md` to use
`expect.poll()`.
- **Breaking**: None
- **Dependencies**: None
## Review Focus
- All fixes follow the rules in
`browser_tests/FLAKE_PREVENTION_RULES.md`
- No behavioral changes to tests -- only timing/retry strategy is
updated
- The `ContextMenu.waitForHidden` simplification removes a
swallowed-error anti-pattern; both locators now use direct `waitFor({
state: 'hidden' })`
---------
Co-authored-by: Amp <amp@ampcode.com>
Co-authored-by: github-actions <github-actions@github.com>
|
||
|
Terry Jia
|
277ee5c32e |
test: add E2E tests for Load3D model upload and drag-drop and basic e2e for 3d viewer (#10957)
## Summary Add tests verifying real model loading: - Upload cube.obj via file chooser button - Drag-and-drop cube.obj onto the 3D canvas - Add data-testid to LoadingOverlay for stable test selectors. Add tests verifying 3d viewer openning: - Open viewer from Load3D node via expand button, verify canvas and controls sidebar - Cancel button closes the viewer dialog ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-10957-test-add-E2E-tests-for-Load3D-model-upload-and-drag-drop-and-basic-e2e-for-3d-viewer-33c6d73d3650810c8ff8ed656a5164a6) by [Unito](https://www.unito.io) --------- Co-authored-by: github-actions <github-actions@github.com> |
||
|
Dante
|
e8787dee9d |
fix: prevent node context menu from overflowing viewport on desktop (#10854)
## Summary The node "More Options" (⋮) context menu had `md:max-h-none md:overflow-y-visible` responsive overrides that removed the height constraint and scrollability on desktop (768px+). When the menu had many items (e.g., KSampler), items below the viewport fold were inaccessible with no scrollbar. Removed the desktop overrides so `max-h-[80vh] overflow-y-auto` applies at all screen sizes. - Fixes #10824 ## Red-Green Verification | Commit | CI Status | Purpose | |--------|-----------|---------| | `test: add failing test for node context menu viewport overflow` | 🔴 Red | Proves the test catches the bug | | `fix: prevent node context menu from overflowing viewport on desktop` | 🟢 Green | Proves the fix resolves the bug | ## Test Plan - [ ] CI red on test-only commit - [ ] CI green on fix commit - [ ] Manual verification: zoom out to 50%, open node More Options menu, verify last item ("Remove") is scrollable ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-10854-fix-prevent-node-context-menu-from-overflowing-viewport-on-desktop-3396d73d365081989403c981847aeda6) by [Unito](https://www.unito.io) |
||
|
Dante
|
ba0bab3e50 |
test: add E2E tests for ManagerDialog (#10970)
## Summary - Add Playwright E2E tests for the ManagerDialog component which had zero test coverage - Covers dialog opening, pack browsing, search filtering, tab navigation, sort controls, search mode switching, error states, install action, info panel, and close behavior - All API calls (Algolia search, Manager installed packs, queue) are mocked with typed responses Part of the FixIt Burndown test coverage initiative. ## Test plan - [ ] CI browser tests pass - [ ] Tests validate core ManagerDialog user flows with mocked APIs - [ ] No regressions in existing tests ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-10970-test-add-E2E-tests-for-ManagerDialog-33c6d73d365081468a4ad8fc1894f05b) by [Unito](https://www.unito.io) |
||
|
Kelly Yang
|
bbb07053c4 |
test: add E2E tests for CanvasModeSelector toolbar component (#10934)
Adds `browser_tests/tests/canvasModeSelector.spec.ts`, covering the canvas toolbar mode-selector component that was introduced with no E2E coverage. Covers: - Trigger button: toolbar visibility, `aria-expanded` state, icon reflects active mode - Popover lifecycle: open on click, close on re-click / item selection / Escape - Mode switching: clicking Hand/Select drives `canvas.state.readOnly`; clicking the active item is a no-op - ARIA state: `aria-checked` and roving `tabindex` track active mode, including state driven by external commands - Keyboard navigation: ArrowDown/Up with wraparound, Escape restores focus to trigger — all using `toBeFocused()` retrying assertions - Focus management: popover auto-focuses the checked item on open - Keybinding integration: `H` / `V` keys update both `canvas.state.readOnly` and the trigger icon - Shortcut hint display: both menu items render non-empty key-sequence hints 22 tests across 7 `describe` groups. All selectors are ARIA-driven. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-10934-test-add-E2E-tests-for-CanvasModeSelector-toolbar-component-33b6d73d3650819cb2cfdca22bf0b9a5) by [Unito](https://www.unito.io) |
||
|
jaeone94
|
809da9c11c |
fix: use cloud assets for asset widget default value (#10983)
## Summary In cloud mode, asset-supported nodes (e.g. CheckpointLoaderSimple) used the server's `object_info` combo options as their default widget value. These options list local files on the backend which may not exist in the user's cloud asset library. When the missing-model pipeline runs (on undo, reload, or tab switch), it checks widget values against cloud assets and correctly flags these local-only files as missing — producing errors that appear to be false positives but are actually valid detections of unusable defaults. This PR changes the default value source from server combo options to the cloud assets store. ## Default Value Behavior (Before → After) ### Cloud + asset-supported widgets (changed) | Condition | Before | After | |-----------|--------|-------| | Assets cached, `inputSpec.default` in assets | `inputSpec.default` | `inputSpec.default` | | Assets cached, `inputSpec.default` not in assets | `inputSpec.default` | `assets[0]` | | Assets cached, no `inputSpec.default`, `options` exist | `options[0]` | `assets[0]` | | Assets not cached, `inputSpec.default` exists | `inputSpec.default` | `undefined` → "Select model" | | Assets not cached, no `inputSpec.default`, `options` exist | `options[0]` | `undefined` → "Select model" | | Assets not cached, no `inputSpec.default`, no `options` | `undefined` → "Select model" | `undefined` → "Select model" | ### Cloud + non-asset widgets (unchanged) | Condition | Behavior | |-----------|----------| | `inputSpec.default` exists | `inputSpec.default` | | `options` exist | `options[0]` | | `remote` input | `"Loading..."` | | None | `undefined` | ### OSS (unchanged) | Condition | Behavior | |-----------|----------| | `inputSpec.default` exists | `inputSpec.default` | | `options` exist | `options[0]` | | `remote` input | `"Loading..."` | | None | `undefined` | ## Root Cause 1. `addComboWidget` called `getDefaultValue(inputSpec)` which returns `inputSpec.options[0]` — a local file from `object_info` 2. In cloud mode, `shouldUseAssetBrowser()` creates an asset widget with this local filename as default 3. The model (e.g. `dynamicrafter/controlnet/dc-sketch_encoder_fp16.safetensors`) exists on the server but not in the user's cloud asset library 4. On undo/reload, `verifyAssetSupportedCandidates()` checks the widget value against cloud assets → not found → marked as missing ## Changes ### Production (`useComboWidget.ts`) - New `resolveCloudDefault(nodeType, specDefault)` function encapsulates cloud default resolution - Default priority: `inputSpec.default` (if found in cloud assets) → first cloud asset → `undefined` (shows "Select model" placeholder) - Edge case guards: `!= null` check for falsy defaults, `|| undefined` for empty `getAssetFilename` return - Server combo options (`object_info`) are no longer used as defaults for asset widgets ### Unit Tests (`useComboWidget.test.ts`) - 6 scenarios covering all default value paths: - Cloud assets loaded, no `inputSpec.default` → `assets[0]` - Cloud assets loaded, `inputSpec.default` in assets → uses default - Cloud assets loaded, `inputSpec.default` not in assets → `assets[0]` - No cloud assets, with `inputSpec.default` → placeholder - No cloud assets, with server options → placeholder - Asset widget creation verification - Test helper refactored: assertions moved from helper to each test for clarity ### E2E Test (`cloud-asset-default.spec.ts`) - New `@cloud` tagged test verifying CheckpointLoaderSimple uses first cloud asset, not server default - Fixture extension stubs `/api/assets` before app loads (local backend returns 503 for this endpoint) - Uses typed mock data from existing `assetFixtures.ts` ## Scope - **Cloud only**: All changes gated behind `isCloud` + `shouldUseAssetBrowser()` - **OSS impact**: None — code path is not entered in non-cloud builds - **Breaking changes**: None — `useComboWidget` export signature unchanged ## Review Focus - Should the `/api/assets` stub in the E2E fixture extension be moved into `ComfyPage` for all `@cloud` tests? ## Record Before https://github.com/user-attachments/assets/994162a0-b56a-4e84-9b1c-d0f0068196d5 After https://github.com/user-attachments/assets/ba299990-9bd3-4565-bd09-bffac3db60a9 |
||
|
Alexander Brown
|
25d1ac7456 |
test: reorganize subgraph test suite into composable domain specs (#10759)
## Summary Reorganize the subgraph test suite so browser tests are thin representative user journeys while lower-level Vitest suites own combinatorics, migration edge cases, and data-shape semantics. ## Changes - **What**: Migrate 17 flat subgraph browser specs into 10 domain-organized specs under `browser_tests/tests/subgraph/`, move redundant semantic coverage down to 8 Vitest owner suites, delete all legacy flat files - **Browser specs** (54 tests): `subgraphSlots`, `subgraphPromotion`, `subgraphPromotionDom`, `subgraphSerialization`, `subgraphNavigation`, `subgraphNested`, `subgraphLifecycle`, `subgraphCrud`, `subgraphSearch`, `subgraphOperations` - **Vitest owners** (230 tests): `SubgraphNode.test.ts` (rename/label propagation), `subgraphNodePromotion.test.ts`, `promotedWidgetView.test.ts`, `SubgraphSerialization.test.ts` (duplicate-ID remap), `SubgraphWidgetPromotion.test.ts` (legacy hydration), `subgraphNavigationStore*.test.ts` (viewport cache, workflow-switch), `subgraphStore.test.ts` (search aliases, description) - **Net effect**: browser suite shrinks from ~96 scattered tests to 54 focused journeys ## Review Focus - Coverage ownership split: each browser test has a unique UI-only failure mode; semantic coverage lives in Vitest - `subgraphPromotionDom.spec.ts` forces LiteGraph mode and uses `canvas.openSubgraph()` instead of `navigateIntoSubgraph()` to avoid a wrapper-specific DOM overlay duplication issue — entry-affordance coverage lives in `subgraphNavigation.spec.ts` - No product code changes — test-only migration ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-10759-test-reorganize-subgraph-test-suite-into-composable-domain-specs-3336d73d365081b0a56bcbf809b1f584) by [Unito](https://www.unito.io) --------- Co-authored-by: Amp <amp@ampcode.com> |
||
|
Christian Byrne
|
9b769656ac |
test: add ShareWorkflow dialog E2E tests (DLG-05) (#10588)
## Summary Adds Playwright E2E tests for the ShareWorkflow dialog component and its various states. ## Tests added - Dialog opens and shows unsaved state for new workflows (save prompt) - Ready state shows create link button for saved but unpublished workflows - Shared state shows copy URL field with share link after publishing - Stale state shows update link button when workflow modified after publishing - Close button dismisses dialog - Create link transitions dialog from ready to shared state - Tab switching between share link and publish to hub (when comfyHubUploadEnabled) - Tab aria-selected states update correctly on switch ## Approach - Share dialog is gated behind `isCloud` (compile-time constant), so tests invoke it directly via `page.evaluate()` importing `useShareDialog` - Share service API calls (`/api/userdata/*/publish`, `/api/assets/from-workflow`) mocked via `page.route()` for deterministic state testing - Dialog state (loading → unsaved → ready → shared → stale) controlled by mock responses - Feature flags set via `serverFeatureFlags.value` for tab visibility testing ## Notes - All pre-existing TS2307 errors are `.vue` module resolution — no new type errors - Tests cover the 5 dialog states: loading, unsaved, ready, shared, stale ## Task Part of Test Coverage Q2 Overhaul (DLG-05). ## Conventions - Uses Vue nodes with new menu enabled - Tests read as user stories - No full-page screenshots - Proper waits, no sleeps - All API calls mocked ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-10588-test-add-ShareWorkflow-dialog-E2E-tests-DLG-05-3306d73d365081a0ab15f333707e493b) by [Unito](https://www.unito.io) --------- Co-authored-by: GitHub Action <action@github.com> |
||
|
Dante
|
537e4bc4f2 |
test: add E2E tests for settings dialog (#10797)
## Summary - Adds Playwright E2E tests for the Settings dialog covering behaviors not tested elsewhere: - About panel displays version badges (navigates to About, verifies content) - Boolean setting toggle through the UI persists the value (uses search + ToggleSwitch click, verifies via API) - Dialog can be closed via the close button (complements existing Escape-key test) ## Test plan - [ ] `pnpm test:browser:local -- --grep "Settings dialog"` passes - [ ] No overlap with existing tests in `dialog.spec.ts` or `useSettingSearch.spec.ts` ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-10797-test-add-E2E-tests-for-settings-dialog-3356d73d365081e4a3adcd3979048444) by [Unito](https://www.unito.io) |
||
|
pythongosssss
|
4b0b8e7240 |
test: App mode - Welcome screen state (#10747)
## Summary Adds tests for validating welcome screen state ## Changes - **What**: - add clear graph util function ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-10747-test-App-mode-Welcome-screen-state-3336d73d365081f0ba27d567c3c81505) by [Unito](https://www.unito.io) |