mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-05-20 20:39:30 +00:00
4c35add5bc7cebc63456a3c5aa918f207ce7db2f
5456 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Hunter
|
4c35add5bc |
feat: add civitai.red hostname support (#11349)
*PR Created by the Glary-Bot Agent* --- ## Summary Civitai split its domain — NSFW content moved to `civitai.red` while `civitai.com` stays SFW. The frontend only recognized `civitai.com` URLs, causing the import button to silently reject `.red` links. This was the root cause of 8+ support tickets in 3 days. Companion to backend PR: https://github.com/Comfy-Org/cloud/pull/3259 ## Changes ### Import source recognition - **`civitaiImportSource.ts`**: Added `'civitai.red'` to `hostnames` array — this is the primary fix for "button doesn't recognize the links" ### Missing model auto-download - **`missingModelDownload.ts`**: Added `'https://civitai.red/'` to `ALLOWED_SOURCES` ### URL detection utilities - **`formatUtil.ts`**: `isCivitaiModelUrl()` now accepts `civitai.red` URLs with proper hostname validation - **`assetMetadataUtils.ts`**: `getSourceName()` returns "Civitai" for `.red` URLs ### Tests (4 files) - `useUploadModelWizard.test.ts`: Added civitai.red hostnames and URL test case - `missingModelDownload.test.ts`: Added civitai.red cases for `toBrowsableUrl` and `isModelDownloadable` - `assetMetadataUtils.test.ts`: Added civitai.red case for `getSourceName` - `useMissingModelInteractions.test.ts`: Updated mock hostnames - `formatUtil.test.ts`: Added civitai.red cases for `isCivitaiModelUrl` ## Not changed (intentionally) - `getAssetSourceUrl()` ARN fallback (line 88) — ARNs don't carry domain info, `civitai.com` is correct default - `fetchCivitaiMetadata()` API URL (line 109) — REST API works on both domains, keeping `civitai.com` Resolves BE-353 ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11349-feat-add-civitai-red-hostname-support-3456d73d3650810d9c62ef4ad95ae031) by [Unito](https://www.unito.io) --------- Co-authored-by: Glary-Bot <glary-bot@users.noreply.github.com> Co-authored-by: Christian Byrne <cbyrne@comfy.org> Co-authored-by: Terry Jia <terryjia88@gmail.com> |
||
|
Christian Byrne
|
a3893a593d |
refactor: move select components from input/ to ui/ component library (#11378)
*PR Created by the Glary-Bot Agent* --- ## Summary Reconciles `src/components/input/` (older select components) into `src/components/ui/` (internal component library), eliminating the separate `input/` directory entirely. ## Changes - **Move MultiSelect** → `src/components/ui/multi-select/MultiSelect.vue` - **Move SingleSelect** → `src/components/ui/single-select/SingleSelect.vue` - **Extract shared resources** → `src/components/ui/select/types.ts` (SelectOption type) and `src/components/ui/select/select.variants.ts` (CVA styling variants) - **Update 7 consuming files** to use new import paths - **Update 1 test file** (AssetFilterBar.test.ts mock paths) - **Move stories and tests** alongside their components - **Delete `src/components/input/`** directory ## Context The `input/` directory contained only MultiSelect and SingleSelect — two well-built components that already used the same stack as `ui/` (Reka UI, CVA, Tailwind 4, Composition API). MultiSelect even imported `ui/button/Button.vue`. Moving them into `ui/` removes the split and consolidates all reusable components in one place. No API changes — all component props, slots, events, and behavior are preserved exactly. ## Verification - `pnpm typecheck` ✅ - `pnpm build` ✅ - `pnpm lint` (stylelint + oxlint + eslint) ✅ - All 15 relevant tests pass (MultiSelect: 5, SingleSelect: 2, AssetFilterBar: 8) ✅ - `pnpm knip` — no dead exports ✅ - No stale `@/components/input/` references remain ✅ - Pre-commit hooks pass ✅ - Git detected all moves as renames (97-100% similarity) ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11378-refactor-move-select-components-from-input-to-ui-component-library-3476d73d3650810e99b4c3e0842e67f3) by [Unito](https://www.unito.io) Co-authored-by: Glary-Bot <glary-bot@users.noreply.github.com> |
||
|
Terry Jia
|
deba72e7a0 |
gizmo controls (#11274)
## Summary Add Gizmo transform controls to load3d - Remove automatic model normalization (scale + center) on load; models now appear at their original transform. The previous auto-normalization conflicted with gizmo controls — applying scale/position on load made it impossible to track and reset the user's intentional transform edits vs. the system's normalization - Add a manual Fit to Viewer button that performs the same normalization on demand, giving users explicit control - Add Gizmo Controls (translate/rotate) for interactive model manipulation with full state persistence across node properties, viewer dialog, and model reloads - Gizmo transform state is excluded from scene capture and recording to keep outputs clean ## Motivation The gizmo system is a prerequisite for these potential features: - Custom cameras — user-placed cameras in the scene need transform gizmos for precise positioning and orientation - Custom lights — scene lighting setup requires the ability to interactively position and aim light sources - Multi-object scene composition — positioning multiple models relative to each other requires per-object transform controls - Pose editor — skeletal pose editing depends on the same transform infrastructure to manipulate individual bones/joints Auto-normalization was removed because it silently mutated model transforms on load, making it impossible to distinguish between the original model pose and user edits. This broke gizmo reset (which needs to know the "clean" state) and would corrupt round-trip transform persistence. ## Screenshots (if applicable) https://github.com/user-attachments/assets/621ea559-d7c8-4c5a-a727-98e6a4130b66 ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11274-gizmo-controls-3436d73d365081c38357c2d58e49c558) by [Unito](https://www.unito.io) |
||
|
Rizumu Ayaka
|
3db0eac353 |
perf: textarea widget layer composition (#10804)
## Summary I noticed that nodes using textarea for user input, which contain long user-entered text, require scrolling within a single node. Having 40 such textarea nodes in a test canvas is enough to cause lag (20fps). In contrast, a control group using regular nodes can handle up to 500 nodes without lag (60fps). the numerous scrolling text widgets in test workflows are the main source of performance pressure. Each scrolling text input box imposes independent layout and layering pressure. I initially tried more complex solutions to fix this issue, like virtual scrolling. However, I found that a simple CSS modification was sufficient and effective. Even when I quadrupled the problematic number of nodes on my M5 MacBook Air, it remained smooth. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-10804-perf-textarea-widget-layer-composition-3356d73d3650814da75adec266d7cad9) by [Unito](https://www.unito.io) --------- Co-authored-by: bymyself <cbyrne@comfy.org> |
||
|
Dante
|
4c7729ee0b |
fix: remove hover dimming overlay on image nodes (#11296)
## Summary Remove the black opacity/dimming overlay on image node hover and add shadows to action buttons for visibility against light backgrounds. ## Changes - **What**: Remove `opacity-50` dimming on hover in `DisplayCarousel.vue`, remove `transition-opacity hover:opacity-80` from grid thumbnails in `ImagePreview.vue`, add `shadow-md` to action buttons in `ImagePreview.vue`. Applies to Save Image, Load Image, Preview Image, and all nodes using these shared image components. ## Review Focus Button shadows (`shadow-md`) should provide sufficient contrast against light image backgrounds without needing the dimming overlay. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11296-fix-remove-hover-dimming-overlay-on-image-nodes-3446d73d36508193bb5cc27d431014fd) by [Unito](https://www.unito.io) |
||
|
Dante
|
40083d593b |
test: cover Button, Textarea, Slider components (#11325)
Closes coverage gaps in \`src/components/ui/\` as part of the unit-test
backfill. Uses \`@testing-library/vue\` +
\`@testing-library/user-event\` for user-centric, behavioral assertions.
## Testing focus
Three Reka-UI primitives. The challenge is testing the contract — not
the library internals — given happy-dom's gaps and Reka's
\`useMounted()\`-based async initialization.
### \`Button\` (7 tests)
- Slot rendering + click event propagation.
- \`loading=true\`: three invariants hold **simultaneously** — slot
hidden, \`pi-spin\` spinner present, button is \`toBeDisabled()\`.
- \`disabled=true\` alone: button disabled, no spinner.
- \`as="a"\`: polymorphic root tag (Reka \`Primitive\`'s \`as\` prop
switches the rendered element).
- Variant class pass-through: **one** deliberate style assertion because
the variant-system wiring is part of the component's public contract. No
other styling/class checks (AGENTS.md bans class-based tests).
### \`Textarea\` (6 tests)
- \`v-model\` two-way binding: \`user.type()\` updates the bound ref;
initial value populates the textarea.
- \`disabled\` asserted **behaviorally** — typing is blocked when
disabled, not just the attribute presence.
- Pass-through: \`placeholder\`, \`rows\`, \`class\`.
### \`Slider\` (8 tests)
- Thumb count matches \`modelValue.length\` (range support).
- ARIA: \`aria-valuemin\` / \`aria-valuemax\` / \`aria-valuenow\`.
**Caveat:** Reka's \`SliderRoot\` uses \`useMounted()\`, so
\`aria-valuenow\` is absent on the first render tick. The tests use a
two-tick \`flush()\` helper (\`await nextTick()\` twice) to wait it out
— no mocking of Reka required.
- Keyboard drag: \`user.keyboard('{ArrowRight}')\` / \`'{ArrowLeft}'\`
moves the value; with \`step: 10\` starting from 50, ArrowRight produces
exactly \`[60]\`.
- \`disabled\` → no emit on keyboard events.
### Reka integration limit
Pointer-driven \`slide-start\` / \`slide-end\` gestures in happy-dom
would require faking \`getBoundingClientRect\` and \`setPointerCapture\`
— that crosses into mocking Reka internals. Keyboard-drag paths are
covered instead (the user-facing contract); the \`pressed\` CSS state is
exercised implicitly by surviving a full mount + update cycle.
## Principles applied
- No mocks of Vue, Reka, or \`@vueuse/core\`.
- Queries via \`getByRole\` / \`getByLabelText\`; **no** class-name or
Tailwind-token queries (per AGENTS.md).
- All 21 tests pass; typecheck/lint/format clean. Test-only; no
production code touched.
|
||
|
Dante
|
7089a7d1a0 |
fix: show asset display names in bulk delete confirmation (#11321)
## Summary Bulk-delete confirmation on Comfy Cloud listed raw SHA-256 filenames, making the modal impossible to use to verify what would be deleted. ## Changes - **What**: `useMediaAssetActions.deleteAssets` now maps each asset through `getAssetDisplayName`, so the confirmation's `itemList` matches the user-assigned names shown in the left media panel (`MediaAssetCard`). - **Tests**: Added two regression tests covering `user_metadata.name` / `display_name` resolution and the `asset.name` fallback. ## Review Focus - Parity with `MediaAssetCard` display: we reuse the same `getAssetDisplayName` helper; extension stripping (via `getFilenameDetails`) is not applied in the modal since file extensions are useful context when confirming deletions. Reported in Slack: https://comfy-organization.slack.com/archives/C0A4XMHANP3/p1776383570015289 ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11321-fix-show-asset-display-names-in-bulk-delete-confirmation-3456d73d36508108a3d5f2290ca39e18) by [Unito](https://www.unito.io) |
||
|
jaeone94
|
b756545f59 |
refactor: clean up ChangeTracker logging, guards, and redundant widget wrapper (#11328)
## Summary Follow-ups to PR #10816. Bundles four review items left open after that PR merged — three inside `ChangeTracker` itself and one in the widget composable that wraps it. ### What changed - **Removed all `loglevel` logging from `src/scripts/changeTracker.ts`** — the logger was set to `info`, so every `logger.debug` call was dead code at runtime. `logger.warn` calls were replaced with direct reporting. The only-downstream dead code (`graphDiff` helper) and its sole dependency (`jsondiffpatch`) are also removed. - **Named the `captureCanvasState()` guard conditions** — `isUndoRedoing` and `isInsideChangeTransaction` now carry the intent that the inline `_restoringState` / `changeCount > 0` expressions used to obscure. - **Surfaced lifecycle violations through a single reporting helper** — `reportInactiveTrackerCall()` logs `console.warn` once per method per session and, on Desktop, emits a `Sentry.addBreadcrumb` with the offending workflow path. `deactivate()` and `captureCanvasState()` share this path so the same invariant is reported consistently. - **Inlined `captureWorkflowState` wrapper in `useWidgetSelectActions`** — the private helper forwarded to `changeTracker.captureCanvasState()` with no added logic. Both call sites now invoke the change tracker directly. ### Issues fixed - Fixes #11249 - Fixes #11259 - Fixes #11258 - Fixes #11248 ### Test plan - [x] `pnpm test:unit src/scripts/changeTracker.test.ts` — 16 tests pass - [x] `pnpm test:unit src/renderer/extensions/vueNodes/widgets/composables/useWidgetSelectActions.test.ts` — 6 tests pass - [x] `pnpm typecheck` - [x] `pnpm lint` - [x] `pnpm format` |
||
|
Alexander Brown
|
da91bdc957 |
fix: persist middle-click reroute node setting across reloads (#11362)
*PR Created by the Glary-Bot Agent* --- ## Summary - Remove hardcoded `LiteGraph.middle_click_slot_add_default_node = true` from `slotDefaults` extension `init()` that unconditionally overrode the user's persisted preference on every page load - Add E2E regression test verifying both the setting store value and the LiteGraph runtime flag persist through page reload ## Root Cause The `Comfy.SlotDefaults` extension's `init()` method (in `slotDefaults.ts`) contained a hardcoded `LiteGraph.middle_click_slot_add_default_node = true` from the original JS→TS conversion (July 2024). When `Comfy.Node.MiddleClickRerouteNode` was later made configurable in v1.3.42, this line was never removed. Since extension `init()` runs **after** `useLitegraphSettings()` syncs the stored value, the hardcoded assignment overwrote the user's preference on every reload. ## Changes | File | Change | |------|--------| | `src/extensions/core/slotDefaults.ts` | Remove line 21 (`LiteGraph.middle_click_slot_add_default_node = true`) | | `browser_tests/tests/dialogs/settingsDialog.spec.ts` | Add reload persistence test asserting both store value and LiteGraph global | The setting default (`true`) is already properly managed by `coreSettings.ts` and reactively synced via `useLitegraphSettings.ts`, so removing the hardcoded line preserves existing default behavior while allowing user overrides to persist. ## Screenshots    ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11362-fix-persist-middle-click-reroute-node-setting-across-reloads-3466d73d365081ef8692dbd0619c8594) by [Unito](https://www.unito.io) Co-authored-by: Glary-Bot <glary-bot@users.noreply.github.com> |
||
|
pythongosssss
|
be2d757c47 |
test: add regression test for getCanvasCenter null guard (#8399) (#11271)
## Summary Add a regression test for #8399 (null check in `getCanvasCenter` to prevent crash on asset insert). The fix in `src/services/litegraphService.ts` added optional chaining around `app.canvas?.ds?.visible_area` with a `[0, 0]` fallback so inserting an asset before the canvas finishes initializing no longer crashes. There was no existing unit test for `litegraphService`, so this regression could silently return. ## Changes - **What**: New unit test file `src/services/litegraphService.test.ts` covering `useLitegraphService().getCanvasCenter`. - Mocks `@/scripts/app` so `app.canvas` can be swapped per test via `Reflect.set`. - Null-canvas case (regression for #8399): returns `[0, 0]` instead of throwing. - Missing `ds.visible_area` case: also returns `[0, 0]`. - Initialised case: returns the centre of the visible area. - Verified RED→GREEN locally. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11271-test-add-regression-test-for-getCanvasCenter-null-guard-8399-3436d73d3650815c9925c8fdf9ec4bd3) by [Unito](https://www.unito.io) |
||
|
Kelly Yang
|
6dba67da6b |
refactor: remove @ts-expect-error suppressions in sidebar components (#11338)
…(issue #11092 phase 4a)
## Summary
Part of #11092 — Phase 4a: remove 10 @ts-expect-error suppressions from
three sidebar component files.
## Changes
3 files in the sidebar had `@ts-expect-error` suppressions that all
traced back to the same root cause: **optional properties on generic
interfaces that TypeScript cannot narrow through indirect conditions.**
`TreeExplorerNode<T>` declares `data?: T` — optional by design, since
folder nodes may carry no payload. Every `handleClick`, `handleDrop`,
and `handleDelete` method that accessed `this.data` was relying on the
runtime invariant that leaf nodes always have data, but TypeScript has
no way to derive `data !== undefined` from `this.leaf === true`. The fix
was to make the invariant explicit in the condition (`if (this.leaf &&
this.data)`) or add an early-return guard (`if (!nodeDefToAdd) return`).
The same pattern appeared in a closure in `ModelLibrarySidebarTab.vue`:
`model` was `ComfyModelDef | null` from an outer const, and `if
(this.leaf)` inside a method cannot narrow a captured variable. Widening
the condition to `if (this.leaf && model)` resolved it. Two additional
suppressions in that file covered `addNodeOnGraph`'s nullable return and
its optional `widgets` property, both fixed with optional chaining.
The remaining suppression was an unannotated function parameter inferred
as `any`; adding the explicit type from the `filters` ref removed it.
<!-- CURSOR_SUMMARY -->
---
> [!NOTE]
> **Low Risk**
> Low risk: changes are TypeScript-safety refactors (extra
null/undefined guards) plus new unit tests; runtime behavior should only
differ in edge cases where `data`/`model`/`widgets` are unexpectedly
missing.
>
> **Overview**
> Removes several `@ts-expect-error` suppressions in sidebar library
tabs by making leaf-node invariants explicit (`if (this.leaf &&
this.data/model)`), adding early returns for missing drag-drop payloads,
and using optional chaining for nullable `addNodeOnGraph`/`widgets`
access.
>
> Adds new Vitest coverage for `ModelLibrarySidebarTab`,
`NodeLibrarySidebarTab`, and `NodeBookmarkTreeExplorer` to validate
click-to-add-node behavior, folder expansion toggling, filter add/remove
flow, bookmark drag/drop, and safe no-op paths when required data is
absent.
>
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
|
||
|
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
|
||
|
Benjamin Lu
|
ecb7fd4796 |
feat: add frontend subscription success recovery (#11286)
Improving our subscription detection system. Optimal will have to come after BE team brings personal billing to cloud repo off of comfy api. ## Summary - replace the dialog-local focus poller with a frontend checkout tracker stored in `localStorage` - recover pending subscription checkouts from app boot plus global page lifecycle (`pageshow`, `visibilitychange`) with bounded retries only while an attempt is pending - emit `subscription_success` through GTM with frontend-derived metadata once subscription state reaches the expected target tier/cycle ## Why This is the frontend-only 80/20 path. It fixes the brittle "old tab must regain focus" behavior without adding new backend endpoints or backend event storage. The browser records one pending checkout attempt when checkout is opened, and any returning cloud tab can recover it later by comparing current subscription state against the expected target plan. ## Tradeoffs - browser-scoped, not backend-authoritative - no server transaction id - scheduled downgrades through the billing portal are intentionally not inferred as immediate success events - still best-effort compared with the backend outbox/WebSocket approach ## Validation - `pnpm exec vitest run src/platform/cloud/subscription/composables/useSubscription.test.ts src/platform/telemetry/providers/cloud/GtmTelemetryProvider.test.ts` - `pnpm typecheck` ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11286-feat-add-frontend-subscription-success-recovery-3436d73d3650814d9f74c89e6926aa84) by [Unito](https://www.unito.io) |
||
|
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) |
||
|
Kelly Yang
|
f6f267b46d |
test: add unit tests for slotCalculations (#11302)
# PR Description
#11106
**This PR only focus on `slotCalculations.ts`.**
Add unit tests for `slotCalculations.ts` — the centralized utility that
calculates input/output slot positions in graph coordinates. This file
had zero test coverage despite containing non-trivial branching logic
used by both the litegraph adapter and the Vue renderer layout system.
## What's covered
### `calculateInputSlotPosFromSlot`
- [x] **Collapsed nodes**: Returns the node origin shifted up by half
the title height.
- [x] **Hard-coded offsets**: Slots with specific `pos` offsets return
`nodeOrigin + pos` directly.
- [x] **Default vertical layout**: Covers first slot x/y, multi-slot
vertical ordering, `slotStartY` offset, exclusion of widget input slots,
and exclusion of fixed-position slots from index ordering.
### `getSlotPosition` (Legacy fallback path, `vueNodesMode` disabled)
- [x] **Coordinate derivation**: Input and output slot positions derived
correctly from `node.pos`.
- [x] **Collapsed state**: Collapsed input and output nodes use
`title-height` and `NODE_COLLAPSED_WIDTH` offsets.
- [x] **Boundary handling**: Out-of-range slot index falls back to node
origin.
<!-- CURSOR_SUMMARY -->
---
> [!NOTE]
> **Low Risk**
> Test-only changes that don’t affect runtime behavior; risk is limited
to potential brittleness if slot layout constants or expectations
change.
>
> **Overview**
> Adds a new `slotCalculations.test.ts` suite covering
`calculateInputSlotPosFromSlot` and the legacy (`LiteGraph.vueNodesMode`
disabled) path of `getSlotPosition`.
>
> Tests exercise key branches for collapsed nodes, hard-coded slot `pos`
overrides, default vertical slot ordering (including `slotStartY`), and
filtering of widget/fixed-position inputs, plus boundary behavior for
out-of-range slot indices.
>
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
|
||
|
Dante
|
e8d833bc54 |
test: cover useLazyPagination, useRangeEditor, useCurveEditor (#11326)
Closes coverage gaps in \`src/composables/\` as part of the unit-test backfill. ## Testing focus Three composables, each a different kind of test challenge: reactive pagination state, DOM-track drag math, and SVG pointer interaction. No third-party library is mocked. ### \`useLazyPagination\` (10 tests) - Accepts both \`Ref<T[]>\` and plain \`T[]\` inputs. - \`currentPage\` ceiling at \`totalPages\` (clamp behavior). - Source-array replacement resets internal page state. - \`loadedPages\` (Set) accumulates across navigation. - **Observed source issue.** \`loadNextPage\` is declared \`async\` but contains no \`await\` (the artificial delay is commented out). Consequence: \`isLoading\` is never externally observable as \`true\`, and the concurrent-call dedup in the design doesn't actually fire in practice. Tests cover **observable** behavior only; the finding is noted here as a candidate follow-up fix. ### \`useRangeEditor\` (11 tests) - Drags each of \`min\` / \`max\` / \`midpoint\` handles; respects the \`showMidpoint\` toggle (events on the midpoint are ignored when hidden). - Value clamping within \`[valueMin, valueMax]\`. - \`denormalize\` receives the correct normalized position — verifies the 0–1 mapping math, not just that it was called. - \`trackRef.value === null\` → pointer events are no-ops (null-safety). - **Real lifecycle.** Mounts a tiny \`defineComponent\` via \`@testing-library/vue\`'s \`render\` and exercises cleanup through \`unmount()\`. \`onBeforeUnmount\` only fires inside a component instance — \`effectScope.stop()\` alone is insufficient. ### \`useCurveEditor\` (14 tests) - \`curvePath\` empty when fewer than 2 points. - Linear interpolation: \`M\` + \`L\` command sequence, points sorted by x before drawing. - Non-linear uses \`createInterpolator\` (our module → OK to mock and assert call shape). - Drag: dispatching \`pointermove\` updates \`modelValue\`; after \`pointerup\`, a follow-up \`pointermove\` is a no-op. - **happy-dom gaps polyfilled.** \`Element.setPointerCapture\` is stubbed per-element and \`DOMPoint.prototype.matrixTransform\` is added in \`beforeEach\`. Since the SVG has no CTM, \`DOMMatrix.inverse()\` returns identity — so \`svgCoords\` maps \`clientX\`/\`clientY\` directly into curve space, giving deterministic assertions without brittle coordinate math. ## Principles applied - No mocks of \`vue\`, \`@vueuse/core\`, or \`es-toolkit\`. - Behavioral assertions only — no return-shape checks. - All 35 tests pass; typecheck/lint/format clean. Test-only; no production code touched. |
||
|
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> |
||
|
Jedrzej Kosinski
|
ff4c812d08 |
feat: show sign-in button via server feature flag (#11298)
## Summary Show the sign-in button in the frontend when the `show_signin_button` server feature flag is set, without requiring a special desktop distribution build. ## Changes - Add `SHOW_SIGNIN_BUTTON` to `ServerFeatureFlag` enum - Add `showSignInButton` getter in `useFeatureFlags` composable (returns `boolean | undefined`) - Update `WorkflowTabs.vue` to use `flags.showSignInButton ?? isDesktop` - the server flag takes precedence when set, falls back to compile-time `isDesktop` for legacy desktop support ## Related - Comfy-Org/ComfyUI-Desktop-2.0-Beta#415 - Backend: Comfy-Org/ComfyUI `feature/generic-feature-flag-cli` - Launcher: Comfy-Org/ComfyUI-Desktop-2.0-Beta#418 Co-authored-by: Amp <amp@ampcode.com> |
||
|
Dante
|
7ffaff7e1b |
test: cover useBillingPlans and tierBenefits (#11318)
Closes coverage gaps in `src/platform/cloud/subscription/` as part of the unit-test backfill. ## Testing focus `useBillingPlans` holds **module-scoped refs** (`plans`, `currentPlanSlug`, `isLoading`, `error`). If state leaks between tests, failures get masked as false-green. The suite uses `vi.resetModules()` + dynamic `import()` in every test to get a fresh instance — state isolation is the primary design constraint here. ### `useBillingPlans` (12 tests) - **Concurrent-call dedup.** The \`isLoading\` guard is validated by creating a pending promise, firing a second \`fetchPlans()\` while the first is in-flight, and asserting the mock is called **exactly once**. - **Error branching.** \`Error\` instance → \`.message\` captured. Non-Error rejection → fallback string (\`'Failed to fetch plans'\`). Both paths also verify \`console.error\` logging via a spy. - **Error-reset invariant.** After a failure, a subsequent success must null out \`error.value\` — order-dependent and easy to regress. - **Shared-state invariant.** Two separate \`useBillingPlans()\` calls return refs pointing at the same module-level state. - **Computed filtering.** \`monthlyPlans\` / \`annualPlans\` partition by duration — assertions on distinct output, not input re-assertion. ### \`tierBenefits\` (7 tests) - Table-driven across all \`TierKey\` values for \`maxDuration\`, \`addCredits\`, \`customLoRAs\` branches. - \`monthlyCredits\` free-tier path including the \`remoteConfig.free_tier_credits\` null fallback. - Translator/formatter forwarding verified by spy. ## Principles applied - No mocks of \`vue\`, \`pinia\`, or \`@vueuse/core\` — only our own \`workspaceApi\`. - Behavioral assertions only — no return-shape checks. - All 19 tests pass; typecheck/lint/format clean. Test-only; no production code touched. |
||
|
jaeone94
|
5d04df7b2c |
fix: prevent duplicate prepareForSave and conflicting is_new telemetry on self-overwrite Save As (#11329)
## Summary Follow-up to PR #10816. Fixes a telemetry semantic bug in `saveWorkflowAs` that emitted two conflicting events for a single user action. ### What changed - `saveWorkflowAs` self-overwrite branch now calls `workflowStore.saveWorkflow` directly instead of delegating to the `saveWorkflow()` wrapper. The wrapper would run `prepareForSave` a second time and emit `trackWorkflowSaved({ is_new: false })`, which then conflicted with the outer `saveWorkflowAs`'s `trackWorkflowSaved({ is_new: true })` for the same user action. - Added regression tests asserting a single `trackWorkflowSaved` call with `{ is_new: true }` and a single `prepareForSave` invocation on both the self-overwrite and copy paths. ### Issues fixed - Fixes #10819 ### Why no E2E test The bug and fix are entirely about observability (how many telemetry events are emitted and with what payload). There is no user-visible change — the file is saved correctly in both pre- and post-fix cases, and `is_new` values are never rendered in the UI. Playwright tests cannot directly verify `trackWorkflowSaved` call counts/payloads without intercepting outbound analytics traffic, which is not a pattern used elsewhere in `browser_tests/`. Unit tests at the service boundary are the appropriate level for this contract: they mock `useTelemetry` and can assert exact call count and payload deterministically. ### Test plan - [x] `pnpm test:unit src/platform/workflow/core/services/workflowService.test.ts` — 56 tests pass (including 2 new regression tests + 1 expanded assertion) - [x] `pnpm typecheck` - [x] `pnpm lint` - [x] `pnpm format` ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11329-fix-prevent-duplicate-prepareForSave-and-conflicting-is_new-telemetry-on-self-overwrite-3456d73d36508192875ed5e70ab9c359) by [Unito](https://www.unito.io) |
||
|
Dante
|
2d50cc2d76 |
feat: show success toast after ComfyHub publish (#11316)
## Summary
Adds a success toast in the ComfyHub publish flow so users get explicit
confirmation that the workflow was published before the dialog closes.
## Changes
- **What**: `ComfyHubPublishDialog.handlePublish()` calls `toast.add({
severity: 'success', ... })` after `submitToComfyHub()` resolves and
before `onClose()` runs. Adds two i18n keys (`publishSuccessTitle`,
`publishSuccessDescription`) and an assertion in the existing
success-path test.
## Review Focus
- This is the lightweight stop-gap discussed in [Slack
thread](https://comfy-organization.slack.com/archives/C0AEPRS8N74/p1776370871654139?thread_ts=1776362591.237159&cid=C0AEPRS8N74)
while the larger published-state design is still pending phase-2 work.
Symmetric with the existing `publishFailedTitle/Description` error
toast.
- `submitToComfyHub` is synchronous (asset uploads happen inside it), so
a successful resolve means the workflow is live.
- `<Toast>` is mounted in `GlobalToast.vue`, so it persists after
`onClose()` destroys the dialog.
## Screenshots (if applicable)
<img width="1135" height="634" alt="Screenshot 2026-04-17 at 8 11 34 AM"
src="https://github.com/user-attachments/assets/a71400a7-2055-4c2a-a761-9298cfa24e9a"
/>
n/a — toast text only.
┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-11316-feat-show-success-toast-after-ComfyHub-publish-3446d73d365081a7bbb3ca29ca3bb618)
by [Unito](https://www.unito.io)
|
||
|
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
|
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) |
||
|
Terry Jia
|
6fb90b224d |
fix(load3d): restore missed hover state when viewer init is async (#11265)
## Summary followup https://github.com/Comfy-Org/ComfyUI_frontend/pull/9520 mouseenter fires before load3d is created during async init (getLoad3dAsync), so the STATUS_MOUSE_ON_VIEWER flag is never set. This causes isActive() to return false after INITIAL_RENDER_DONE, stopping the animation loop from calling controlsManager.update() and making OrbitControls unresponsive on first open. Track hover state in the composable and sync it to load3d after creation. |
||
|
pythongosssss
|
a8e1fa8bef |
test: add regression test for WEBP RIFF padding (#8527) (#11267)
## Summary Add a regression test for #8527 (handle RIFF padding for odd-sized WEBP chunks). The fix added + (chunk_length % 2) to the chunk-stride calculation in getWebpMetadata so EXIF chunks following an odd-sized chunk are still located correctly. There was no existing unit test covering getWebpMetadata, so without a regression test the fix could silently break in a future refactor. ## Changes - **What**: - New unit test file src/scripts/pnginfo.test.ts covering getWebpMetadata's RIFF chunk traversal. - Helpers build a minimal in-memory WEBP with one VP8 chunk of configurable length followed by an EXIF chunk encoding workflow:<json>. - Odd-length case (regression for #8527): without the % 2 padding adjustment, the parser walks one byte short and returns {}. - Even-length case: guards against an over-correction that always adds 1. - Verified RED→GREEN locally. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11267-test-add-regression-test-for-WEBP-RIFF-padding-8527-3436d73d36508117a66edf3cb108ded0) by [Unito](https://www.unito.io) |
||
|
pythongosssss
|
83ceef8cb3 |
test: add regression test for non-string serverLogs (#8460) (#11268)
## Summary Add a regression test for #8460 (handle non-string `serverLogs` in error report). The fix added `typeof error.serverLogs === 'string' ? ... : JSON.stringify(...)` in `errorReportUtil.ts` so object-shaped logs no longer render as `[object Object]`. There was no existing unit test for `generateErrorReport`, so this regression could silently return. ## Changes - **What**: New unit test file `src/utils/errorReportUtil.test.ts` covering `generateErrorReport`'s `serverLogs` rendering. - String case: verifies plain-string logs still appear verbatim and `[object Object]` is absent. - Object case (regression for #8460): verifies object logs are JSON-stringified instead of coerced to `[object Object]`. - Verified RED→GREEN locally. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11268-test-add-regression-test-for-non-string-serverLogs-8460-3436d73d36508195a32fc559ab7ce5bb) by [Unito](https://www.unito.io) |
||
|
Christian Byrne
|
873a75d607 |
test: add unit tests for usePainter composable (#11137)
## Summary Add 25 behavioral unit tests for `usePainter` composable, bringing coverage from 0% to ~35% lines / ~57% functions. ## Changes - **What**: New test file `src/composables/painter/usePainter.test.ts` covering widget sync, settings persistence, canvas sizing, brush display scaling, serialization, restore, pointer event guards, and cursor visibility. ## Review Focus - Mock patterns: singleton factory mocks for stores, wrapper component for lifecycle hooks - Test coverage prioritization: focused on mount-time sync, reactive watchers, and computed behavior rather than canvas pixel output ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11137-test-add-unit-tests-for-usePainter-composable-33e6d73d36508147bde7e9c349c743ca) by [Unito](https://www.unito.io) |
||
|
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. |
||
|
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
|
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> |
||
|
AustinMroz
|
25ac047b58 |
Fix node library action buttons (#11232)
The node library migration (#8548) failed to include the 'Edit Blueprint' button, and had incorrect sizing and color on the 'Delete Blueprint' button. - Re-add edit blueprint which was missed in the migration - Fix incorrect sizing on delete blueprint - Fix color (lucide uses background, not text) - Migrate all action buttons use our capital 'B' Button component and use standardized variants where possible Spun out of #11133 ┆Issue is synchronized with this [Notion page](https://app.notion.com/p/PR-11232-Fix-node-library-action-buttons-3426d73d365081339cafc7457c0b5421) by [Unito](https://www.unito.io) |
||
|
Christian Byrne
|
c484c3984f |
fix: remove appendTo override from FormDropdown to restore correct positioning (#11147)
## Problem PR #10338 added `useTransformCompatOverlayProps()` to FormDropdown and FormDropdownMenuActions, which sets `appendTo: 'self'` in graph mode. This breaks PrimeVue Popover positioning inside CSS-transformed containers — the dropdown appears at incorrect Y positions. ## Root Cause PrimeVue Popover with `appendTo: 'self'` renders the overlay inside the component's DOM, inheriting parent CSS transforms. This causes the popover to miscalculate its position when the parent has `transform: scale()` or `translate()`. ## Fix Remove the `appendTo` override from both FormDropdown and FormDropdownMenuActions. PrimeVue defaults to `appendTo: 'body'`, which teleports the popover to `<body>` — correctly positioning it outside any CSS transform context. - **Graph mode**: restores pre-#10338 behavior (`appendTo: 'body'` default) - **App mode**: unaffected — `'body'` is exactly what app mode needs (prevents sidebar overflow clipping) ## Testing - Existing unit tests pass (5/5) - Typecheck clean - Lint clean - **E2E test rationale**: No E2E test added — this is a pure removal of a prop override (reverting to PrimeVue defaults). The positioning bug requires CSS transforms at specific viewport scales which are impractical to assert reliably in Playwright. The existing `subgraph-dom-widget-clipping` perf test exercises dropdown rendering in transformed contexts and shows no regression. Fixes #10499 Supersedes #11001 (temporary hotfix for backport) --------- Co-authored-by: GitHub Action <action@github.com> |
||
|
Dante
|
2524846f5c |
fix: guard progress_text before canvas init (#11174)
## Summary Prevent early `progress_text` websocket events from throwing before the graph canvas is initialized. ## Changes - **What**: Guard `handleProgressText()` until `canvasStore.canvas` exists, and add a regression test for a startup-time `progress_text` event arriving before `GraphCanvas` finishes initialization. ## Review Focus Confirm this is the right guard point for the startup race between `GraphView` websocket binding and `GraphCanvas` async setup, and that progress text behavior is unchanged once the canvas is ready. ## Validation - `pnpm exec eslint src/stores/executionStore.ts src/stores/executionStore.test.ts` - `pnpm exec vitest run src/stores/executionStore.test.ts -t "should ignore progress_text before the canvas is initialized"` - `pnpm test:unit -- --run src/stores/executionStore.test.ts` still reports one unrelated isolated-file failure in `nodeLocatorIdToExecutionId` on current `main` ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11174-fix-guard-progress_text-before-canvas-init-3406d73d3650813dad23d511fb51add5) by [Unito](https://www.unito.io) |
||
|
Comfy Org PR Bot
|
12f578870e |
1.44.4 (#11177)
Patch version increment to 1.44.4 **Base branch:** `main` ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11177-1-44-4-3416d73d365081c0a2e0def7071c1441) by [Unito](https://www.unito.io) --------- Co-authored-by: christian-byrne <72887196+christian-byrne@users.noreply.github.com> Co-authored-by: github-actions <github-actions@github.com> |
||
|
Benjamin Lu
|
719ed16d32 |
fix: track workspace subscription success on immediate subscribe (#11130)
## Summary Track GTM `subscription_success` when a workspace subscription completes synchronously in the dialog. The async billing-operation path already emitted telemetry; the missing gap was the immediate `subscribed` response. ## Changes - **What**: Add the missing GTM success emission to both synchronous workspace subscribe success branches while preserving the existing toast, billing refresh, and dialog close behavior. ## Review Focus Verify the synchronous `response.status === "subscribed"` workspace dialog paths are the only missing frontend success emissions, while the async billing-operation telemetry path remains unchanged. This PR intentionally stays minimal. It does not add new browser coverage yet; the previous component-level unit test was more implementation-coupled than this fix justified, and a better long-term test would be a higher-level workspace billing flow test once we have a cleaner harness. |
||
|
Christian Byrne
|
e39468567a |
fix: check server feature flags for progress_text binary format (#10996)
## Problem API node generation status text (sent via `progress_text` WebSocket binary messages) was not showing on local ComfyUI, but worked on cloud. ## Root Cause The binary decoder for `progress_text` messages (eventType 3) checked `getClientFeatureFlags()?.supports_progress_text_metadata` — the **client's own flags** — to decide whether to parse the new format with `prompt_id`. Since the client always advertises `supports_progress_text_metadata: true`, it always tried to parse the new wire format: ``` [4B event_type][4B prompt_id_len][prompt_id][4B node_id_len][node_id][text] ``` But the backend PR that adds `prompt_id` to the binary message ([ComfyUI#12540](https://github.com/Comfy-Org/ComfyUI/pull/12540)) was **closed without merging**, so local ComfyUI still sends the legacy format: ``` [4B event_type][4B node_id_len][node_id][text] ``` The decoder misinterpreted the `node_id_len` as `prompt_id_len`, consuming the actual node_id bytes as a prompt_id, then producing garbled `nodeId` and `text` — silently dropping all progress text updates via the catch handler. Cloud worked because the cloud backend supports and echoes the feature flag. ## Fix One-line change: check `serverFeatureFlags.value` (what the server echoed back) instead of `getClientFeatureFlags()` (what the client advertises). ## Tests Added 3 tests covering: - Legacy format parsing when server doesn't support the flag - New format parsing when server does support the flag - Corruption regression test: client advertises support but server doesn't ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-10996-fix-check-server-feature-flags-for-progress_text-binary-format-33d6d73d365081449a0dc918358799de) by [Unito](https://www.unito.io) --------- Co-authored-by: GitHub Action <action@github.com> |
||
|
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
|
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
|
||
|
Kelly Yang
|
20255da61f |
feat(load3d): add optional HDRI environment lighting to 3D preview nodes (#10818)
## Summary Adds `HDRIManager` to load `.hdr/.exr` files as equirectangular environment maps via **three.js** `RGBELoader/EXRLoader` - Uploads HDRI files to the server via `/upload/image` API so they persist across page reloads - Restores HDRI state (enabled, **intensity**, **background**) from node properties on reload - Auto-enables "**Show as Background**" on successful upload for immediate visual feedback - Hides standard directional lights when HDRI is active; restores them when disabled - Hides the Light Intensity control while HDRI is active (lights have no effect when HDRI overrides scene lighting) - Limits HDRI availability to PBR-capable formats (.gltf, .glb, .fbx, .obj); automatically disables when switching to an incompatible model - Adds intensity slider and "**Show as Background**" toggle to the HDRI panel ## How to Use HDRI Environment Lighting 1. Load a 3D model using a Load3D or Load3DViewer node (supported formats: .gltf, .glb, .fbx, .obj) 2. Open the control panel → go to the Light tab 3. Click the globe icon to open the **HDRI panel** 4. Click Upload HDRI and select a` .hdr` or `.exr` file 5. The environment lighting applies automatically — the scene background also updates to preview the panorama 6. Use the intensity slider to adjust the strength of the environment lighting 7. Toggle Show as Background to show or hide the HDRI panorama behind the model ## Screenshots https://github.com/user-attachments/assets/1ec56ef0-853e-452f-ae2b-2474c9d0d781 ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-10818-feat-load3d-add-optional-HDRI-environment-lighting-to-3D-preview-nodes-3366d73d365081ea8c7ad9226b8b1e2f) by [Unito](https://www.unito.io) <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Adds new HDRI loading/rendering path and persists new `LightConfig.hdri` state, touching Three.js rendering, file uploads, and node property restoration. Risk is moderate due to new async flows and potential compatibility/performance issues with model switching and renderer settings. > > **Overview** > Adds optional **HDRI environment lighting** to Load3D previews, including a new `HDRIManager` that loads `.hdr`/`.exr` files into Three.js environment/background and exposes controls for enable/disable, background display, and intensity. > > Extends `LightConfig` with an `hdri` block that is persisted on nodes and restored on reload; `useLoad3d` now uploads HDRI files, loads them into `Load3d`, maps scene light intensity to HDRI intensity, and auto-disables HDRI when the current model format doesn’t support it. > > Updates the UI to include embedded HDRI controls under the Light panel (with dismissable overlays and icon updates), adjusts light intensity behavior when HDRI is active, and adds tests/strings/utilities (`getFilenameExtension`, `mapSceneLightIntensityToHdri`, new constants) to support the feature. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit b12c9722dc54a227004bdc383e3bf583504ebdce. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> --------- Co-authored-by: Terry Jia <terryjia88@gmail.com> |
||
|
Christian Byrne
|
c2dba8f4ee |
chore(#11080): consolidate duplicate rgbToHSL — use shared colorUtil (#11134)
## Summary Consolidate duplicate `rgbToHSL` implementation — mask editor now uses the shared `colorUtil.ts` version instead of its own copy. ## Changes - Export `rgbToHsl` from `src/utils/colorUtil.ts` (was private) - Replace 30-line local `rgbToHSL` in `useCanvasTools.ts` with a 2-line wrapper that imports from `colorUtil.ts` and scales the return values from 0-1 to degree/percentage ## Testing ### Automated - All 176 existing tests pass (`colorUtil.test.ts` + `maskeditor/` suite) - No new tests needed — behavior is identical ### E2E Verification Steps 1. Open any image in the mask editor 2. Select the magic wand / color picker tool 3. Use HSL-based color matching — results should be identical to before ## Review Focus The canonical `rgbToHsl` returns normalized 0-1 values while the mask editor needs degree/percentage scale (h: 0-360, s: 0-100, l: 0-100). The local wrapper handles this conversion. Fixes #11080 ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11134-chore-11080-consolidate-duplicate-rgbToHSL-use-shared-colorUtil-33e6d73d36508120bbd8f444f5cc94b6) by [Unito](https://www.unito.io) Co-authored-by: Terry Jia <terryjia88@gmail.com> |
||
|
Dante
|
e729e5edb8 |
fix: place cloned node above original in Vue renderer (#10361)
## Summary Cloned/pasted nodes in Node 2.0 (Vue renderer) mode now appear above the original node instead of behind it. ## Root Cause The legacy LiteGraph canvas renderer uses array ordering for z-ordering: nodes are stored in `graph._nodes` and drawn sequentially, so newly added nodes (appended to the end) are automatically drawn on top. There is no explicit z-index. The Vue renderer (Node 2.0) uses explicit CSS `z-index` for node ordering. New nodes default to `zIndex: 0` in `layoutMutations.ts`. When a node has been interacted with, `bringNodeToFront` raises its z-index. A cloned node at z-index 0 therefore appears behind any previously interacted node. The alt-click clone path in `LGraphNode.vue` already handles this correctly by calling `bringNodeToFront()` after cloning. However, the menu clone and keyboard paste paths go through `_deserializeItems` in `LGraphCanvas.ts`, which does not set z-index for new nodes. | Clone method | Legacy renderer | Vue renderer (before fix) | Vue renderer (after fix) | |---|---|---|---| | Alt-click drag | On top (array order) | On top (`bringNodeToFront` called) | On top | | Right-click menu Clone | On top (array order) | Behind original (z-index 0) | On top | | Ctrl+C / Ctrl+V | On top (array order) | Behind original (z-index 0) | On top | ## Steps to Reproduce 1. Enable Node 2.0 mode (Vue renderer) in settings 2. Add any node to the canvas 3. Click or drag the node (raises its z-index via `bringNodeToFront`) 4. Right-click the node and select "Clone" 5. **Expected**: Cloned node appears above the original, immediately draggable 6. **Actual**: Cloned node appears behind the original; user must move the original to access the clone ## Changes After `batchUpdateNodeBounds` in `_deserializeItems`, calls `bringNodeToFront` for each newly created node so they receive a z-index above all existing nodes. ## Side Effect Analysis Checked all call sites of `_deserializeItems`: 1. **Initial graph load / workflow open**: `loadGraphData` in `app.ts` does NOT call `_deserializeItems`. Workflow loading goes through `LGraph.configure()` which directly adds nodes and links. The layout store is initialized separately via `initializeFromLiteGraph`. No side effect. 2. **Paste from clipboard (Ctrl+V)**: Both `usePaste.ts` (line 52) and `pasteFromClipboard` (line 4080) call `_deserializeItems`. Pasted nodes appearing on top is the correct and desired behavior. No issue. 3. **Undo/Redo**: `ChangeTracker.updateState()` calls `app.loadGraphData()`, which does a full graph reconfigure -- it does NOT go through `_deserializeItems`. No side effect. 4. **Subgraph blueprint addition**: `litegraphService.ts` (line 906) calls `_deserializeItems` when adding subgraph blueprints from the node library. These are freshly placed nodes that should appear on top. Desired behavior. 5. **Alt-click clone in LGraphNode.vue**: This path calls `LGraphCanvas.cloneNodes()` -> `_deserializeItems()`, then separately calls `bringNodeToFront()` again on line 433 of `LGraphNode.vue`. The second call is now redundant (the node is already at max z-index), but harmless -- `bringNodeToFront` finds the current max, adds 1, and sets. The z-index will increment from N to N+1 on the second call. This is a minor redundancy, not a bug. 6. **Performance**: `bringNodeToFront` iterates all nodes in the layout store once per call (O(m)) to find max z-index. For n new nodes, the total cost is O(n*m). In practice, clone/paste operations involve a small number of nodes (typically 1-10), so this is negligible. For extremely large pastes (100+ nodes), each call also increments the max by 1, so z-indices will be sequential (which is actually a reasonable stacking order). 7. **layoutStore availability**: `layoutStore` is a module-level singleton (`new LayoutStoreImpl()`) -- not a Pinia store -- so it is always available. The `useLayoutMutations()` composable is a plain function returning an object of closures over `layoutStore`. It does not require Vue component context. No risk of runtime errors. 8. **Legacy renderer (non-Vue mode)**: When Node 2.0 mode is disabled, the layout store still exists but is not used for rendering. Calling `bringNodeToFront` will update z-index values in the Yjs document that are never read. This is harmless. ## Red-Green Verification | Commit | Result | Description | |---|---|---| | `6894b99` `test:` | RED | Test asserts cloned node z-index > original. Fails with `expected 0 to be greater than 5`. | | `3567469` `fix:` | GREEN | Calls `bringNodeToFront` for each new node in `_deserializeItems`. Test passes. | Fixes #10307 --------- Co-authored-by: Alexander Brown <drjkl@comfy.org> |
||
|
Alexander Brown
|
3043b181d7 |
refactor: extract composables from VTU holdout components, complete VTL migration (#10966)
## Summary Extract internal logic from the 2 remaining VTU holdout components into composables, enabling full VTL migration. ## Changes - **What**: Extract `useProcessedWidgets` from `NodeWidgets.vue` (486→135 LOC) and `useWidgetSelectItems`/`useWidgetSelectActions` from `WidgetSelectDropdown.vue` (563→170 LOC). Rewrite both component test files as composable unit tests + slim behavioral VTL tests. Remove `@vue/test-utils` devDependency. - **Dependencies**: Removes `@vue/test-utils` ## Review Focus - Composable extraction is mechanical — no logic changes, just moving code into testable units - `useProcessedWidgets` handles widget deduplication, promotion border styling, error detection, and identity resolution (~290 LOC) - `useWidgetSelectItems` handles the full computed chain from widget values → dropdown items including cloud asset mode and multi-output job resolution (~350 LOC) - `useWidgetSelectActions` handles selection resolution and file upload (~120 LOC) - 40 new composable-level unit tests replace 13 `wrapper.vm.*` accesses across the 2 holdout files ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-10966-refactor-extract-composables-from-VTU-holdout-components-complete-VTL-migration-33c6d73d36508148a3a4ccf346722d6d) by [Unito](https://www.unito.io) --------- Co-authored-by: Amp <amp@ampcode.com> |
||
|
Christian Byrne
|
577f373cde |
fix: auto fit-to-view on first subgraph entry (#10995)
## Summary Auto-fit viewport to subgraph content on first entry so interior nodes are immediately visible. ## Changes - **What**: On cache miss in `restoreViewport()`, call `fitView()` via `requestAnimationFrame` instead of silently returning. Existing cache-hit path (revisiting a subgraph) is unchanged. ## Review Focus The `anyItemOverlapsRect` guard in `app.ts` (workflow load path) is intentionally **not** touched — it serves a different purpose (respecting `extra.ds` on workflow load). This fix only affects subgraph navigation transitions where there is no saved viewport to respect. Fixes #8173 ## Screenshots (if applicable) N/A — viewport positioning fix; before: empty canvas on subgraph entry, after: nodes visible. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-10995-fix-auto-fit-to-view-on-first-subgraph-entry-33d6d73d365081f3a9b3cc2124979624) by [Unito](https://www.unito.io) |
