mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-05-11 08:20:53 +00:00
v1.44.14
7804 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Comfy Org PR Bot
|
036c79259b |
1.44.14 (#11769)
Patch version increment to 1.44.14 **Base branch:** `main` ┆Issue is synchronized with this [Notion page](https://app.notion.com/p/PR-11769-1-44-14-3526d73d3650816ba3a9cd87afeef035) by [Unito](https://www.unito.io) --------- Co-authored-by: christian-byrne <72887196+christian-byrne@users.noreply.github.com> Co-authored-by: Alexander Brown <drjkl@comfy.org> Co-authored-by: github-actions <github-actions@github.com>v1.44.14 |
||
|
Benjamin Lu
|
17c18b0707 |
fix: embed HubSpot contact form (#11723)
## Summary Embed the HubSpot-hosted contact sales form on `/contact` and `/zh-CN/contact` so HubSpot owns submission handling, validation, anti-spam/security updates, tracking context, and form configuration. ## Changes - **What**: Replaces the custom local contact form stub with a HubSpot embed component using the HubSpot-hosted developer/raw HTML script and `hs-form-html` container. - **Localization**: Uses the existing English form ID by default and switches to the zh-CN form ID for `/zh-CN/contact`. - **Styling**: Applies HubSpot form CSS custom properties to match the Comfy contact page colors and `PP Formula` font more closely. - **Docs**: Updates the website README with the developer embed snippet and the zh-CN form ID. - **Dependencies**: None. ## Why Embedded Form - HubSpot docs say forms should be loaded with the HubSpot-hosted JavaScript file, and that security, anti-spam, accessibility, and performance improvements will not propagate if the embed runtime is copied or self-hosted: https://developers.hubspot.com/docs/cms/start-building/building-blocks/modules/forms - The direct form submission endpoint is documented under `v3 legacy`: https://developers.hubspot.com/docs/api-reference/legacy/marketing/forms/v3-legacy/submit-data-unauthenticated - HubSpot’s legacy API overview says numeric-versioned APIs are legacy and will be deprecated in a future release: https://developers.hubspot.com/docs/api-reference/legacy/overview ## Review Focus - Confirm the portal ID and form IDs are correct: - `en`: `94e05eab-1373-47f7-ab5e-d84f9e6aa262` - `zh-CN`: `6885750c-02ef-4aa2-ba0d-213be9cccf93` - Check visual fit on `/contact` and `/zh-CN/contact`, especially font, background, inputs, radio controls, and submit button. - Confirm the developer/raw HTML embed remains the preferred integration over a custom Forms API POST. ## Local Checks - `pnpm exec oxfmt --check apps/website/src/components/contact/HubspotFormEmbed.vue apps/website/README.md` - `pnpm exec eslint apps/website/src/components/contact/HubspotFormEmbed.vue` - `pnpm --filter @comfyorg/website typecheck` - `pnpm --filter @comfyorg/website test:unit` - `pnpm --filter @comfyorg/website build` - Commit hooks: stylelint, oxfmt, oxlint, eslint, `pnpm typecheck`, `pnpm typecheck:website` - Push hook: `pnpm knip --cache` Build completed with existing non-fatal environment warnings: Node `v25.8.1` vs requested `24.x`, unresolved website font paths deferred to runtime, `transformWithEsbuild` deprecation, and missing Ashby env falling back to the committed snapshot. Incredibly, during the taking of these screenshots, I discovered a bug in macos, where despite the snapshot/record bar not existing, from me esc'ing out, some of the tooltips persist. Closing and reopening the lid did not fix this. I didn't see the process in activity monitor. <img width="1512" height="862" alt="Screenshot 2026-04-29 at 7 09 55 PM" src="https://github.com/user-attachments/assets/92518795-6845-4b34-8da3-54048b440eb1" /> <img width="1512" height="862" alt="Screenshot 2026-04-29 at 7 13 18 PM" src="https://github.com/user-attachments/assets/f7609e58-898d-413c-975c-b02b70b84e73" /> ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11723-fix-embed-HubSpot-contact-form-3506d73d365081528bfbe4b024c2a21f) by [Unito](https://www.unito.io) --------- Co-authored-by: github-actions <github-actions@github.com> |
||
|
Alexander Brown
|
ca8407218b |
chore(husky): skip pre-push knip hook in CI (#11772)
*PR Created by the Glary-Bot Agent* --- ## Summary Fixes a false-positive knip failure in the `i18n: Update Core` workflow that's been blocking version-bump PRs (e.g. #11769, run [25141091787](https://github.com/Comfy-Org/ComfyUI_frontend/actions/runs/25141091787/job/73690747401)). ## Root cause 1. The `update-locales` job uses `setup-comfyui-server`, which copies `tools/devtools/*` → `ComfyUI/custom_nodes/ComfyUI_devtools/*` at the workspace root. 2. The job's final `git push` triggers the husky `.husky/pre-push` hook, which runs `pnpm knip`. 3. Knip's `project` glob (`**/*.{js,ts,vue}` in `knip.config.ts`) sweeps the runtime `ComfyUI/` directory; the existing `tools/devtools/web/**` ignore doesn't cover the copied path, so `ComfyUI/custom_nodes/ComfyUI_devtools/web/legacyWidget.js` (added in #11574) is reported as unused → exit 1 → push aborted. ## Why fix the hook (not knip config or workflows) - **`knip.config.ts` ignore** (`'ComfyUI/**'`): tried first; `treatConfigHintsAsErrors: true` makes the unused-pattern hint fatal in normal CI runs where `ComfyUI/` doesn't exist, breaking `ci-lint-format` everywhere. - **`HUSKY=0` per workflow step**: works, but bots can't push workflow file changes without `workflows` scope, and it would need to be repeated on every CI workflow that pushes. - **Skip the hook in CI**: the canonical `pnpm knip` check runs in `.github/actions/lint-format-verify/action.yml` on every PR, so the pre-push duplicate is pure dev-side guardrail. Gating it on `$CI` (set to `true` by all GitHub Actions runners) cleanly removes it from every CI push without affecting local developer workflow. ## Verification Reproduced the failure locally by mirroring the CI sequence (`cp -r tools/devtools/* ComfyUI/custom_nodes/ComfyUI_devtools/` then `pnpm knip:no-cache`): - Without fix: `[log] Unused files (1) ComfyUI/custom_nodes/ComfyUI_devtools/web/legacyWidget.js` → exit 1 (matches CI log) - With `CI=true`, the hook short-circuits at `exit 0` before invoking knip - `pnpm knip` on a clean checkout (no `ComfyUI/`) still passes ┆Issue is synchronized with this [Notion page](https://app.notion.com/p/PR-11772-chore-husky-skip-pre-push-knip-hook-in-CI-3526d73d3650819f9a01d549d122b69c) by [Unito](https://www.unito.io) Co-authored-by: Glary-Bot <glary-bot@users.noreply.github.com> |
||
|
Benjamin Lu
|
26ac1eece1 |
Allow website screenshot workflow to remove its label (#11725)
## Summary Allows the website screenshot update workflow to remove its own trigger label when a label-triggered run completes. ## Changes - **What**: Grants the screenshot update job `issues: write`, which is required for `issues.removeLabel`, and keeps the cleanup scoped to `Update Website Screenshots`. - **Dependencies**: None. ## Review Focus Confirm the workflow permission scope is appropriate and that unexpected label cleanup failures should fail the workflow instead of being silently swallowed. ## Validation - `/Users/ben/go/bin/actionlint .github/workflows/pr-update-website-screenshots.yaml` - `/Users/ben/Library/Python/3.9/bin/yamllint --config-file .yamllint .github/workflows/pr-update-website-screenshots.yaml` - `git diff --check HEAD~1 HEAD` No browser e2e regression was added because this change only adjusts GitHub Actions token permissions and label cleanup behavior; it does not change shipped app/runtime behavior. Fixes FE-487 |
||
|
Benjamin Lu
|
810381ab63 |
Stabilize website GitHub stars in visual snapshots (#11771)
## Summary Stabilize the website nav GitHub star count in visual-test builds so snapshot comparisons do not drift as the live GitHub count changes. ## Changes - **What**: Added `WEBSITE_GITHUB_STARS_OVERRIDE` for build-time star-count overrides and set it to `111000` in the website E2E and screenshot-update workflows. - **Dependencies**: None. ## Review Focus Confirm the deterministic build-time override is preferable to screenshot masking, since Playwright masks draw a colored rectangle whose geometry can also drift when masked content changes size. ## Screenshots (if applicable) Not included; this keeps visual-test input data stable rather than changing the UI. |
||
|
Christian Byrne
|
cc1a737291 |
test: add unit tests for useImagePreviewWidget (#11394)
## Summary Add 23 unit tests for `useImagePreviewWidget` composable, improving coverage from ~52% to significantly higher. ## Test Coverage - Widget construction (factory return, name, type, serialization, options) - `computeLayoutSize` returns correct dimensions - `createCopyForNode` creates properly bound copy - Upload spinner rendering (animation, stroke color) - Single image rendering (draws image, handles missing node.size) - Image size text overlay (enabled/disabled via setting) - Multi-image thumbnail grid (compact vs non-compact mode, cell borders) - Pointer interaction (pointerDown cleanup, overIndex reset) - `previewImages` override from DrawWidgetOptions - `onPointerDown` drag handler setup - `onClick` no-op behavior ## Testing ```bash pnpm test:unit -- src/renderer/extensions/vueNodes/widgets/composables/useImagePreviewWidget.test.ts ``` All 23 tests pass. No source changes. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11394-test-add-unit-tests-for-useImagePreviewWidget-3476d73d365081a69c78e87329e75d9f) by [Unito](https://www.unito.io) --------- Co-authored-by: Alexander Brown <drjkl@comfy.org> Co-authored-by: DrJKL <DrJKL0424@gmail.com> Co-authored-by: Amp <amp@ampcode.com> |
||
|
Kelly Yang
|
c74e08e244 |
refactor: extract useBrushAdjustment from useBrushDrawing (#11544)
## Summary
Part of the `useBrushDrawing` decomposition plan (PR C). Extracts brush
size/hardness adjustment logic (Alt+drag interaction) into a dedicated
`useBrushAdjustment` composable. No runtime behavior is changed — pure
structural refactor.
## Changes
- **New** `src/composables/maskeditor/useBrushAdjustment.ts` —
encapsulates `startBrushAdjustment` and `handleBrushAdjustment`,
including dead zone filtering, dominant axis suppression, and
size/hardness clamping
- **New** `src/composables/maskeditor/useBrushAdjustment.test.ts` — unit
tests covering: no-op before start, dead zone suppression, size increase
on drag, size/hardness clamping, dominant axis lock
- **Updated** `src/composables/maskeditor/useBrushDrawing.ts` — removes
inlined adjustment state and functions, delegates to
`useBrushAdjustment(initialSettings)`
## Test Functionality
Open ComfyUI and enter the MaskEditor of any image node. On the canvas,
Alt + Right-click Drag:
- Drag Right → Increase brush size - pass
- Drag Left → Decrease brush size - pass
- Drag Up → Increase hardness - pass
- Drag Down → Decrease hardness - pass
https://github.com/user-attachments/assets/273e8383-dab5-4c82-ac7b-0a1534dfd770
<!-- CURSOR_SUMMARY -->
---
> [!NOTE]
> **Medium Risk**
> Touches core pointer-interaction logic for brush tuning and changes
adjustment behavior (removes delta saturation and uses initial values),
which could subtly affect UX even though scope is localized to the mask
editor.
>
> **Overview**
> Extracts the Alt-drag brush size/hardness adjustment logic out of
`useBrushDrawing` into a new `useBrushAdjustment` composable, and wires
`useBrushDrawing` to delegate to it.
>
> The extracted logic now bases adjustments off the captured initial
brush size/hardness and removes prior delta capping (no ±100px
saturation), which changes how large/continuous drags affect the final
values. Adds a Vitest suite covering dead-zone behavior, dominant-axis
suppression, clamping, and the no-op-before-start contract.
>
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
|
||
|
Robin Huang
|
8f9f452c86 |
fix: enable Chrome password autofill on signup form (#11636)
## Summary Add `name` attributes to the signup form's email, password, and confirm-password inputs so Chrome's password manager recognizes the form and offers autofill/save. ## Changes - **What**: Pass `name` through to the underlying `<input>` on the email field (via `pt:root:name`) and on both password fields (via `pt:pc-input-text:root:name`). Without `name`, Chrome can't pair the email with the password and won't surface the save-password / suggest-strong-password prompts. ## Review Focus - The PrimeVue passthrough syntax (`pt:root:*` for `InputText`, `pt:pc-input-text:root:*` for `Password`) lands the attribute on the actual `<input>` element — verified in DevTools. - `confirm-password` is not a standard `autocomplete` token; we keep `autocomplete="new-password"` on both password fields and only differentiate via `name`. ## Screenshots (if applicable) https://github.com/user-attachments/assets/e1e25ab5-8496-4c84-b5f1-630f31956c80 ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11636-fix-enable-Chrome-password-autofill-on-signup-form-34e6d73d36508180882cc9ebafb58417) by [Unito](https://www.unito.io) |
||
|
Benjamin Lu
|
9e16390c33 |
test: assert core command help urls (#11768)
## Summary - Tighten the new `useCoreCommands` help command tests to assert the exact external URL opened for GitHub issues and Discord. ## Testing ```bash pnpm test:unit -- src/composables/useCoreCommands.test.ts pnpm format:check src/composables/useCoreCommands.test.ts ``` Also passed pre-commit `pnpm typecheck` and push hook `pnpm knip`. Stacked on #11748. ┆Issue is synchronized with this [Notion page](https://app.notion.com/p/PR-11768-test-assert-core-command-help-urls-3516d73d365081de99d0c71f707d0fb4) by [Unito](https://www.unito.io) --------- Co-authored-by: dante01yoon <bunggl@naver.com> |
||
|
Terry Jia
|
c88275b2a4 |
test(load3d): add unit tests for SceneManager (#11762)
## Summary Add 35 unit tests for `SceneManager`, the largest remaining gap in the load3d core (452 LOC). Targets only the logic-bearing methods (background mode dispatch, render-mode switching, aspect-ratio scaling, capture pipeline, dispose). Renderer-passthrough internals are intentionally left to E2E. Follow-up to Tier 1 (#11733), Tier 2, Tier 3a, and Tier 3b. ## Changes - **What**: 35 new tests covering construction (main scene + background scene + grid + tiled mesh + default color mode), `toggleGrid`, `setBackgroundColor` (color update, scene-bg cleanup, panorama-demote, prior-texture dispose), `setBackgroundImage` (empty-path fallback, loading-start emit, temp/output subfolder rewrite, /api prefix, tiled-mesh material swap, panorama scene-background promotion, prior-texture dispose, error-path fallback), `removeBackgroundImage`, `setBackgroundRenderMode` (no-op same-mode, color-only emit, image panorama-promote, image tiled-demote), `updateBackgroundSize` (no-texture/no-mesh/no-map guards, wide vs. tall image scaling), `handleResize` (image-bg active vs. color-only), `getCurrentBackgroundInfo`, `captureScene` (returns 3 data URLs + restores renderer state, restores grid visibility, propagates errors), and `dispose` (resource cleanup + scene-background null). ## Review Focus - **Coverage**: `SceneManager.ts` 89.5% lines / 74.2% branches / 89.5% funcs. Uncovered lines are concentrated in `renderBackground` and the deep mesh-traversal loop inside `captureScene` — exactly the renderer-passthrough territory deferred per the Tier 3c plan. - **`THREE.Material.needsUpdate` is a write-only setter** in THREE.js — reading returns `undefined`. The "demote panorama → tiled" test asserts `mat.map === texture` instead of `mat.needsUpdate === true`, with a comment explaining why. - **happy-dom canvas `clientWidth`/`clientHeight` default to 0** — `makeRenderer()` overrides them via `Object.defineProperty` so production code reading `renderer.domElement.clientWidth` gets the test value. - **`THREE.TextureLoader` is mocked via `vi.mock('three', ...)` with `importOriginal`**, matching the pattern in `RecordingManager.test.ts` and `HDRIManager.test.ts`. `mockTextureLoad` is hoisted so each test can resolve/reject the load callback independently. - **`vi.spyOn(manager, 'setBackgroundColor')` in three places** to assert internal delegation (empty-path fallback, error fallback, `removeBackgroundImage`). Defensible because the delegation IS the documented contract for these methods. ┆Issue is synchronized with this [Notion page](https://app.notion.com/p/PR-11762-test-load3d-add-unit-tests-for-SceneManager-3516d73d365081628ff4c146defebac1) by [Unito](https://www.unito.io) |
||
|
pythongosssss
|
23e48b2140 |
feat: Node search - Improve category tree on mobile with collapse (#11687)
## Summary Improves the search experience on mobile by collapsing the category menu & reogranises the filer buttons ## Changes - **What**: - add toggle button to collapse category selection - auto collapse on mobile - floating panel on mobile - re-order filter buttons - tests ## Screenshots (if applicable) Closed: <img width="415" height="373" alt="image" src="https://github.com/user-attachments/assets/c99cd6cd-eb92-4ce3-9844-591dd1e80769" /> Desktop open: <img width="455" height="328" alt="image" src="https://github.com/user-attachments/assets/df15bdda-f77a-4c12-90e1-8608d67c55b4" /> Mobile open: <img width="427" height="600" alt="image" src="https://github.com/user-attachments/assets/a2b115ad-bce0-4ed1-9d30-126a35263259" /> ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11687-feat-Node-search-Improve-category-tree-on-mobile-with-collapse-34f6d73d365081729075e8b0071a3bc1) by [Unito](https://www.unito.io) |
||
|
Terry Jia
|
af43619ae1 |
test(load3d): add unit tests for copyLoad3dState in load3dService (#11761)
## Summary Add 19 unit tests for `Load3dService.copyLoad3dState` (the one method intentionally deferred from Tier 1). Brings `load3dService.ts` from 54.5% to 100% line coverage. Follow-up to Tier 1 (#11733), Tier 2, and Tier 3a. ## Changes - **What**: 19 new tests covering every branch of `copyLoad3dState`: no-source-model fast path, splat fast path (with and without `originalURL`), mesh path (existing-target-model removal, SkeletonUtils clone, originalModel/material/upDirection/texture copy, initial transform on clone, gizmo transform application, gizmo enable/disable across both source and target prior states, animation copy when present/absent), background-image vs. background-color dispatch, light-intensity falsy fallback, perspective-vs-orthographic FOV gating, and the always-detach + setupForModel gizmo contract. ## Review Focus - **Coverage**: `load3dService.ts` lines 54.5% → **100%**, branches 50% → **90.9%**, funcs 88.9% → **100%**. Remaining uncovered lines are minor (`loadSkeletonUtils` cache-hit path, a couple of null-map early returns). - **Test fixtures use real `THREE.Object3D` and `THREE.Scene`** so production code's `.position.set(...)`, `.rotation.set(...)`, `scene.add/remove` calls work without further stubbing. - **`makeTarget` memoizes the gizmo manager** (`getGizmoManager: () => gizmoManager` rather than returning a fresh literal each call). Production code calls `getGizmoManager()` multiple times; without memoization, the `detach` and `setupForModel` mocks would be unobservable from tests. - **`state` return on `makeTarget`** exposes mutable `modelManager`, captured `gizmoManager`/`animationManager`, and `sceneAdded`/`sceneRemoved` arrays so tests can assert post-state directly without casts through the production-typed `Load3d` interface. - **Background-image test uses `createMockLGraphNode({ id, properties })` overrides** rather than mid-test property mutation. - **Destructuring-default gotcha**: `const { lightsIntensity = 0.8 } = overrides` applies the default even when `undefined` is passed explicitly. The "fallback to setLightIntensity(1)" test passes `0` instead — production code's `intensity || 1` short-circuits the same way. ┆Issue is synchronized with this [Notion page](https://app.notion.com/p/PR-11761-test-load3d-add-unit-tests-for-copyLoad3dState-in-load3dService-3516d73d36508142bc72d97b27b0a36b) by [Unito](https://www.unito.io) |
||
|
Terry Jia
|
d2e88011aa |
test(load3d): add unit tests for EventManager, ViewHelperManager, and load3dLazy (#11760)
## Summary Add unit tests for the three remaining small/wrapper modules in the load3d domain (`EventManager` pub/sub, `ViewHelperManager` ViewHelper wrapper, and `load3dLazy` lazy-extension loader). Follow-up to Tier 1 (#11733) and Tier 2. ## Changes - **What**: 28 new unit tests across 3 files covering EventManager add/remove/emit semantics, ViewHelperManager container DOM creation + pointer-event interception + animation-finished camera-state emission + perspective/orthographic zoom snapshotting + dispose ordering, and load3dLazy extension registration + 3D-node-type recognition + Load3D-specific `mesh_upload` injection + `beforeRegisterNodeDef` hook replay for newly registered extensions. ## Review Focus - **Coverage** (lines/branches/funcs): EventManager 100% / 100% / 100%, ViewHelperManager 100% / 83.3% / 72.7%, load3dLazy 95.8% / 80% / 100%. Aggregate: **98.6% / 85.3% / 85.7%**. - **`vi.mock` factory side-effects only fire once per test file** — `load3dLazy.test.ts` originally tried to count dynamic imports of `./load3d` and `./saveMesh` via spies inside the mock factory, but factories aren't re-invoked across `vi.resetModules()`. Switched to verifying observable side effects (`enabledExtensions` getter call counts, `beforeRegisterNodeDef` replay invocations). - **Snapshot-vs-diff `enabledExtensions` queue** in `load3dLazy.test.ts`: production code does `before = new Set(enabledExtensions); await imports; diff = enabledExtensions.filter(!before.has)`. To exercise the replay branch, the mock returns `[]` first (for `before`) and `[newExtension]` second (for the post-import snapshot) via `mockReturnValueOnce` queueing. - **`MockViewHelper` is defined inside the `vi.mock()` factory** rather than `vi.hoisted()`, matching the `GizmoManager.test.ts:15-41` convention (hoisted handles only, classes inside the factory). - **PointerEvent propagation tests** require the production code's `event.stopPropagation()` to actually keep events from bubbling to the parent in happy-dom; the parent listener gets attached and asserted-not-called. ┆Issue is synchronized with this [Notion page](https://app.notion.com/p/PR-11760-test-load3d-add-unit-tests-for-EventManager-ViewHelperManager-and-load3dLazy-3516d73d3650814bb2dac3360ab0d2a1) by [Unito](https://www.unito.io) |
||
|
Terry Jia
|
180a0001e8 |
test(load3d): add unit tests for LightingManager, ControlsManager, exportMenuHelper, and ModelExporter (#11758)
## Summary Add unit tests for the four Tier 2 untested logic modules in the load3d domain (`LightingManager`, `ControlsManager`, `exportMenuHelper`, and `ModelExporter`). Follow-up to the Tier 1 PR (#11733). ## Changes - **What**: 43 new unit tests across 4 files covering light setup/intensity scaling/HDRI mode/disposal, OrbitControls construction (including DOM-parent fallback) and camera-state event emission, the export submenu builder (item structure, submenu opening, format dispatch, success/error toasts), and the static `ModelExporter` (URL parsing, direct-URL fast paths for matching extensions, GLB/OBJ/STL serialization branches, error toast paths). ## Review Focus - **Coverage** (lines/branches/funcs): LightingManager 100% / 50% / 90.9%, ControlsManager 100% / 100% / 87.5%, exportMenuHelper 100% / 100% / 100%, ModelExporter 98.4% / 95.7% / 100%. Aggregate: **99.2% / 93.5% / 95.1%**. - **`vi.mock(import('@/lib/litegraph/src/litegraph'), ...)` in `exportMenuHelper.test.ts`** uses the dynamic-import form so `importOriginal()` is auto-typed. Required because `apiSchema.ts` transitively imports `LinkMarkerShape` from the same module — replacing the whole module breaks the build. The mock replaces only `LiteGraph.ContextMenu` in-place on the real singleton. - **`MockContextMenu` must be a class**, not an arrow function — production code does `new LiteGraph.ContextMenu(...)`. Initial arrow-function mock failed with "is not a constructor". - **Fake-timer rejection pattern in `ModelExporter.test.ts`**: rejection assertions are attached *before* `vi.runAllTimersAsync()` (`const assertion = expect(p).rejects.toThrow(...); await drain; await assertion`) to avoid unhandled-rejection warnings. - **Surprising `detectFormatFromURL` behavior**: `detectFormatFromURL('?filename=cube')` returns `'cube'`, not `null`, because `'cube'.split('.').pop()` returns the whole basename when no dot is present. Test documents this rather than asserting an incorrect expectation. - **Two unreachable lines left uncovered**: `LightingManager:65` (`?? 1` fallback in the `setLightIntensity` multiplier lookup — every light is registered in the map at construction, so the fallback is dead) and `ModelExporter:21` (a `try/catch` around `URLSearchParams` whose constructor cannot throw on the inputs the production code passes). ┆Issue is synchronized with this [Notion page](https://app.notion.com/p/PR-11758-test-load3d-add-unit-tests-for-LightingManager-ControlsManager-exportMenuHelper-and-3516d73d365081cb96fff33672503822) by [Unito](https://www.unito.io) |
||
|
Dante
|
8f011225bf |
test: add unit tests for useCoreCommands canvas/help commands (#11748)
## Summary Adds 8 tests across three new describe blocks for \`src/composables/useCoreCommands.ts\`: - **Canvas view**: \`Comfy.Canvas.ResetView\`, \`Comfy.Canvas.ZoomIn\`, \`Comfy.Canvas.ZoomOut\`. - **Workflow lifecycle**: \`Comfy.OpenClipspace\`, \`Comfy.RefreshNodeDefinitions\`. - **Help**: \`Comfy.Help.OpenComfyUIIssues\`, \`Comfy.Help.OpenComfyOrgDiscord\`, \`Comfy.Help.AboutComfyUI\`. Adds \`vi.hoisted\` mocks for \`useTelemetry\`, \`useSettingsDialog\`, and \`useLitegraphService.resetView\` so they remain isolated from the existing 15-test suite. ## Why this slice \`useCoreCommands.ts\` exports 118 distinct command callbacks (1356 LOC). A single coverage-backfill PR for the whole file would be unwieldy and risk merge conflicts (this file is touched frequently). This PR covers a coherent slice — view/lifecycle/help commands — and follow-up PRs can pick off remaining clusters. ## Testing \`\`\`bash pnpm vitest run src/composables/useCoreCommands.test.ts \`\`\` ┆Issue is synchronized with this [Notion page](https://app.notion.com/p/PR-11748-test-add-unit-tests-for-useCoreCommands-canvas-help-commands-3516d73d365081c384ffcc72c15dfd47) by [Unito](https://www.unito.io) |
||
|
pythongosssss
|
3c50487c18 |
test: add test for MaxHistoryItems setting (#11750)
## Summary Adds test to ensure MaxHistoryItems limits both API query & client rendering ## Changes - **What**: - ensure limit is added to url - ensure virtual grid is capped to limit ┆Issue is synchronized with this [Notion page](https://app.notion.com/p/PR-11750-test-add-test-for-MaxHistoryItems-setting-3516d73d365081daa973ccfd8a7f479e) by [Unito](https://www.unito.io) |
||
|
pythongosssss
|
818e549e8e |
fix: hide blueprint node id in search (#11759)
There is a setting that enables Node IDs to display on the search results. Subgraphs have long non-user friendly IDs which cause this to render badly for the built in blueprints (and user published). This update hides the IDs for blueprint nodes. ## Changes - **What**: - hide if blueprint - add test ## Screenshots (if applicable) Before: <img width="910" height="504" alt="image" src="https://github.com/user-attachments/assets/9eea9fd7-8f72-4e1b-9522-46efba0ef71a" /> After: <img width="797" height="552" alt="image" src="https://github.com/user-attachments/assets/43d6fc62-4102-41c3-b9bb-a3efd244580d" /> ┆Issue is synchronized with this [Notion page](https://app.notion.com/p/PR-11759-fix-hide-blueprint-node-id-in-search-3516d73d365081baa055d12c6a31fadd) by [Unito](https://www.unito.io) |
||
|
AustinMroz
|
fd1a8e9432 |
Fix legacy widget width in app mode (#11574)
When in app mode, widgets can be drawn with size different from the size of the parent node. Mouse events on legacy canvas widgets require that the client code query the current state of the node and widget to determine if any elements are being interacted with. This PR sets the `widget.width` property when a legacy canvas widget draw operation occurs so that custom nodes can properly resolve subsequent mouse events. At current, no core nodes exist that utilize legacy widgets. As a result the setup code to test this bug fix is slightly involved. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11574-Fix-legacy-widget-width-in-app-mode-34b6d73d365081caaa34c6204f8361f6) by [Unito](https://www.unito.io) |
||
|
Dante
|
8f61ecd82e |
test: add unit tests for executionStore WebSocket handlers (#11746)
## Summary Adds 22 new tests for \`src/stores/executionStore.ts\`, raising coverage from **48.7% → 82.9%** lines (functions 47% → 72.5%). Tests drive each WebSocket handler by capturing handlers registered through the mocked \`api.addEventListener\` and dispatching CustomEvents at them. ## Test Coverage WebSocket handlers (driven through \`bindExecutionEvents\`): - \`execution_start\` — sets activeJobId, seeds queued job entry, clears initializing state for the starting job. - \`execution_cached\` — marks listed nodes; no-op when no active job. - \`execution_interrupted\` — clears active job state. - \`executed\` — marks executed node; no-op when no active job. - \`execution_success\` — clears active job and progress state. - \`executing\` — clears \`_executingNodeProgress\` and activeJobId on null detail. - \`progress\` — sets \`_executingNodeProgress\`. - \`status\` — reads clientId once and stops listening. - \`execution_error\` — service-level (no node_id) routes to \`lastPromptError\`; runtime errors route to \`lastExecutionError\`. - \`notification\` — marks job as initializing on "Waiting for a machine"; ignores empty id and unrelated text. Other: - \`unbindExecutionEvents\` removes every listener registered. - \`storeJob\` populates queuedJobs, jobIdToWorkflowId, jobIdToSessionWorkflowPath. - \`registerJobWorkflowIdMapping\` ignores empty inputs. - \`ensureSessionWorkflowPath\` is idempotent and updates on change. ## Testing \`\`\`bash pnpm vitest run src/stores/executionStore.test.ts pnpm vitest run src/stores/executionStore.test.ts --coverage --coverage.include='src/stores/executionStore.ts' \`\`\` ┆Issue is synchronized with this [Notion page](https://app.notion.com/p/PR-11746-test-add-unit-tests-for-executionStore-WebSocket-handlers-3516d73d3650810aa910f5a022fdc17b) by [Unito](https://www.unito.io) |
||
|
Dante
|
8fe0385a57 |
test: add unit tests for pnginfo wrappers and getLatentMetadata (#11745)
## Summary Extends \`src/scripts/pnginfo.test.ts\` with 5 new tests covering the format-specific delegating wrappers and the safetensors metadata reader. Lifts pnginfo.ts coverage from **17.6% → 23.2%** lines (the remaining gap is \`importA1111\`, which needs a refactor before it can be tested cleanly — left to a follow-up). ## Test Coverage - \`getPngMetadata\`, \`getFlacMetadata\`, \`getAvifMetadata\` delegate to their respective \`metadata/*\` modules (mocked). - \`getLatentMetadata\` returns the \`__metadata__\` object from a hand-built safetensors header. - \`getLatentMetadata\` resolves \`undefined\` when the header has no \`__metadata__\` entry. ## Out of scope \`importA1111\` (lines 176-542) is a 270-line A1111-prompt → ComfyUI-graph builder. Testing it requires either heavy LiteGraph mocks or a refactor that extracts pure parsing helpers from the graph-mutation code. Tracking separately. ## Testing \`\`\`bash pnpm vitest run src/scripts/pnginfo.test.ts pnpm vitest run src/scripts/pnginfo.test.ts --coverage --coverage.include='src/scripts/pnginfo.ts' \`\`\` ┆Issue is synchronized with this [Notion page](https://app.notion.com/p/PR-11745-test-add-unit-tests-for-pnginfo-wrappers-and-getLatentMetadata-3516d73d365081c080a6c8146aa1bee8) by [Unito](https://www.unito.io) |
||
|
Dante
|
d078af3a79 |
test: add unit tests for avif metadata parser (#11744)
## Summary
Adds 12 tests for `src/scripts/metadata/avif.ts`, raising line coverage
from **2.3% → 90.4%** (statements 88.5%, functions 93.3%).
## Test Coverage
Happy paths:
- Workflow JSON extracted from EXIF Exif item (LE)
- Prompt JSON extracted
- Big-endian (MM) EXIF parsing
- Both prompt and workflow present in separate EXIF entries
Negative paths (each yields `{}` without throwing):
- AVIF major brand is not "avif"
- Meta box missing
- iinf has no Exif item
- EXIF entry uses an unrecognized key
- EXIF entry has malformed JSON
- infe version is unsupported (1)
- iloc box missing while iinf has Exif
- Buffer too short for valid header
## Testing
\`\`\`bash
pnpm vitest run src/scripts/metadata/avif.test.ts
pnpm vitest run src/scripts/metadata/avif.test.ts --coverage
--coverage.include='src/scripts/metadata/avif.ts'
\`\`\`
┆Issue is synchronized with this [Notion
page](https://app.notion.com/p/PR-11744-test-add-unit-tests-for-avif-metadata-parser-3516d73d365081c5b29adf7a2b9eff62)
by [Unito](https://www.unito.io)
|
||
|
Terry Jia
|
57d708767a |
test(load3d): add unit tests for AnimationManager, CameraManager, RecordingManager, and load3dService (#11733)
## Summary
Add unit tests for the four largest untested logic-heavy modules in the
load3d domain (`AnimationManager`, `CameraManager`, `RecordingManager`,
and the `load3dService` façade).
## Changes
- **What**: 78 new unit tests across 4 files covering animation
lifecycle (mixer setup, clip switching, play/pause/seek, dispose),
camera state (perspective↔orthographic toggling, FOV gating, state
round-trip, controls rebinding, resize math, model fitting), recording
lifecycle (MediaRecorder wiring, indicator visibility, chunk handling,
export/clear paths, dispose ordering), and the service singleton
(sync/async map access, viewer cache, `handleViewerClose` apply-changes
flow, `handleViewportRefresh` camera-toggle dance).
## Review Focus
- **Coverage**: AnimationManager 100%, CameraManager 88.8%,
RecordingManager 89.1%, load3dService 54.5% lines / 88.9% functions. The
service gap is concentrated in one method — `copyLoad3dState` (lines
217-333) — which is entangled with 3-4 subsystems and is intentionally
deferred to a follow-up PR.
- **happy-dom shims** in `RecordingManager.test.ts`: `MediaRecorder`,
`HTMLCanvasElement.prototype.captureStream`, and `getContext('2d')` are
all stubbed because happy-dom doesn't provide them.
`THREE.TextureLoader` is also mocked because the constructor eagerly
loads an SVG indicator.
- **Singleton state reset** in `load3dService.test.ts`: the service
holds a module-level `viewerInstances` Map that can't be reached from
outside. Tests track every node they create in a `Set` and drain via
`removeViewer` in `beforeEach`. Cleaner than `vi.resetModules()` (which
would re-import the service and break the singleton identity).
- **One subtle THREE behavior**: `AnimationManager.setAnimationTime`
clamps to `duration`, but `AnimationMixer.setTime(duration)` with
`LoopRepeat` wraps `action.time` back to 0. The clamping is therefore
only observable through the emitted progress event, not via
`action.time` directly — the test asserts via the event.
┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-11733-test-load3d-add-unit-tests-for-AnimationManager-CameraManager-RecordingManager-and--3516d73d3650812485a0c91065e161f0)
by [Unito](https://www.unito.io)
|
||
|
Benjamin Lu
|
b3f5f82216 |
Remove unused queue job components (#11621)
## Summary Remove the unused legacy queue job row implementation before changing the live queue popover behavior. ## Changes - Deleted `JobGroupsList` and its test. - Deleted `QueueJobItem` and its Storybook story. - Deleted `QueueAssetPreview`, which was only used by `QueueJobItem`. ## Review Focus - This PR is deletion-only. - `rg` found no remaining references to these components. - `knip` and `typecheck` pass. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11621-Remove-unused-queue-job-components-34d6d73d36508164bf32cb581594cd9f) by [Unito](https://www.unito.io) --------- Co-authored-by: GitHub Action <action@github.com> Co-authored-by: Alexander Brown <drjkl@comfy.org> |
||
|
Dante
|
9df4e02189 |
refactor: unify media asset downloads (#11717)
## Summary Unifies media asset download actions behind a single `downloadAssets(assets?)` API to avoid single and multi asset download path drift. ## Changes - **What**: Replaces `downloadAsset` and `downloadMultipleAssets` with `downloadAssets`, preserving no-arg media context fallback and explicit asset arrays. - **Dependencies**: None. ## Review Focus Download behavior for single-card, context-menu, and sidebar bulk actions should continue to use the same ZIP-export path for cloud multi-output jobs. Fixes #11715 ## Screenshots (if applicable) N/A ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11717-refactor-unify-media-asset-downloads-3506d73d3650810d8bcec9c0194e743d) by [Unito](https://www.unito.io) --------- Co-authored-by: Alexander Brown <drjkl@comfy.org> |
||
|
AustinMroz
|
1c541d8577 |
Short circuit asset reuploads, simplify node dnd (#11691)
When an output is dragged from the assets panel onto a node, outputs were being reuploaded. This logic has been simplified to instead reference the existing asset by resolving the annotated path. As part of this change, async drop handlers on nodes are also fixed. Rather than placing obligation of event handling on client code, not respecting async handlers, or completely ignoring return types, the vue drop handler will now simply set `app.dragOverNode` and allow the `document` drop handler to resolve node drag/drop operations without any of the difficulty from propagation. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11691-Short-circuit-asset-reuploads-simplify-node-dnd-34f6d73d36508157af86e6cf09229781) by [Unito](https://www.unito.io) --------- Co-authored-by: Alexander Brown <drjkl@comfy.org> |
||
|
Comfy Org PR Bot
|
389ff8ba49 |
1.44.13 (#11732)
Patch version increment to 1.44.13 **Base branch:** `main` ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11732-1-44-13-3516d73d36508119acabf5f86256f2aa) 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>v1.44.13 |
||
|
Terry Jia
|
bb74ec94de |
test(load3d): add unit tests for 9 previously-untested controls (#11730)
## Summary Mirror the maskeditor coverage approach for load3d sub-components. Each component gets behavior tests covering rendering, conditional branches, v-model bidirectional sync, and emitted events. - ViewerLightControls: setting-store min/max/step + v-model - ViewerExportControls: format dropdown + click-to-export - ViewerCameraControls: type select, FOV slider visibility - ViewerSceneControls: with/without bg image branches, render mode - PopupSlider: trigger toggle, click-outside dismissal, defaults - CameraControls: switch button, FOV PopupSlider visibility - ExportControls: trigger popup, format selection, click-outside - AnimationControls: empty-list bypass, controls, time formatting - ViewerControls: dialog open routing + onClose wiring ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11730-test-load3d-add-unit-tests-for-9-previously-untested-controls-3506d73d365081eaa9e7c5d0b922fc14) by [Unito](https://www.unito.io) |
||
|
Kelly Yang
|
e7640d414b |
test: add E2E tests for ActionBarButtons toolbar component (#11561)
## Summary
- Add E2E tests for the `ActionBarButtons` toolbar component (FE-111)
- Add `data-testid="action-bar-buttons"` to the container div for stable
test targeting
- Register `TestIds.topbar.actionBarButtons` in `selectors.ts`
## Changes
- `browser_tests/tests/actionBarButtons.spec.ts` — 6 tests across 5
scenarios: empty state, button rendering, icon rendering, multiple
buttons, click handler, mobile label hiding
- `src/components/topbar/ActionBarButtons.vue` — adds `data-testid` to
container
- `browser_tests/fixtures/selectors.ts` — registers new test ID
<!-- CURSOR_SUMMARY -->
---
> [!NOTE]
> **Low Risk**
> Primarily adds Playwright coverage and a `data-testid` attribute;
runtime behavior is unchanged aside from an extra DOM attribute.
>
> **Overview**
> Adds a new Playwright spec (`actionBarButtons.spec.ts`) that verifies
the ActionBarButtons container empty state, rendering (label/icon),
multiple buttons, click handler execution, and mobile label-hiding
behavior by registering buttons via `window.app!.registerExtension`.
>
> Updates the UI and test selector plumbing by adding
`data-testid="action-bar-buttons"` to `ActionBarButtons.vue` and
exposing it as `TestIds.topbar.actionBarButtons` for stable E2E
targeting.
>
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
|
||
|
Alexander Brown
|
c168c37c94 |
chore: update comfyui-ci-container to 0.0.17 (#11569)
Update `comfyui-ci-container` image to `0.0.17` across all CI workflows. | Workflow | Before | After | |---|---|---| | `ci-perf-report.yaml` | `0.0.12` | `0.0.17` | | `ci-tests-e2e.yaml` (×2) | `0.0.16` | `0.0.17` | | `pr-update-playwright-expectations.yaml` | `0.0.16` | `0.0.17` | ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11569-chore-update-comfyui-ci-container-to-0-0-17-34b6d73d365081b8a52ac995855354cb) by [Unito](https://www.unito.io) Co-authored-by: Amp <amp@ampcode.com> |
||
|
pythongosssss
|
089051824c |
test: add tests for link related settings (#11612)
## Summary <!-- One sentence describing what changed and why. --> ## Changes - **What**: <!-- Core functionality added/modified --> - **Breaking**: <!-- Any breaking changes (if none, remove this line) --> - **Dependencies**: <!-- New dependencies (if none, remove this line) --> ## Review Focus <!-- Critical design decisions or edge cases that need attention --> <!-- If this PR fixes an issue, uncomment and update the line below --> <!-- Fixes #ISSUE_NUMBER --> ## Screenshots (if applicable) <!-- Add screenshots or video recording to help explain your changes --> ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11612-test-add-tests-for-link-related-settings-34c6d73d36508145885bd017162e6fae) by [Unito](https://www.unito.io) |
||
|
pythongosssss
|
517da289f6 |
feat: Search - add ghost node following setting and increase opacity (#11365)
## Summary Adds setting to disable the node auto-follow cursor behavior when adding nodes from the search, and increased the visibilty of Vue ghost nodes. ## Changes - **What**: - add setting - increase opacity - add test ## Review Focus <!-- Critical design decisions or edge cases that need attention --> <!-- If this PR fixes an issue, uncomment and update the line below --> <!-- Fixes #ISSUE_NUMBER --> ## Screenshots (if applicable) Before <img width="452" height="517" alt="image" src="https://github.com/user-attachments/assets/369c0d90-5352-482b-a1b3-36180bffb3ee" /> After <img width="440" height="536" alt="image" src="https://github.com/user-attachments/assets/2066fdd4-6eb4-4bfb-ac7c-559fc99de57d" /> ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11365-feat-Search-add-ghost-node-following-setting-and-increase-opacity-3466d73d3650811b9c27ed4cc930816d) by [Unito](https://www.unito.io) |
||
|
Dante
|
98c327b3c6 |
test: add unit tests for colorUtil edge cases (#11671)
## Summary
Extends `colorUtil.test.ts` with boundary tests that the existing suite
did not cover: malformed `parseToRgb` inputs, alpha-hex parsing through
`parseToRgb`, negative hue normalization in `hsbToRgb`, the full
`isTransparent` matrix, HSV-vs-HSB equivalence in `toHexFromFormat`, the
bare-hex prefix path, and a non-primary-color round-trip through
`hexToHsva` / `hsvaToHex`.
## Changes
- **What**: Adds 13 Vitest cases across 5 new `describe` blocks
(`parseToRgb edge cases`, `hsbToRgb normalization`, `isTransparent`,
`toHexFromFormat`, plus a non-primary round-trip in the existing
`hexToHsva / hsvaToHex` block). Uses the existing
`vi.mock('es-toolkit/compat')` memoize stub.
## Review Focus
- The non-primary palette round-trip allows ±1 per RGB channel because
`hsbToRgb` floors while `rgbToHex` rounds; the test asserts the bound
rather than exact equality.
- `parseToRgb` is exercised with alpha-bearing hex (`#f008`,
`#ff000080`); the function returns RGB-only, so the alpha is
intentionally discarded.
- `toHexFromFormat({h, s, v}, 'hsb')` covers the HSV-shaped object path
that wraps `hsbToRgb`.
## Testing
\`\`\`bash
pnpm exec vitest run src/utils/colorUtil.test.ts
pnpm format -- src/utils/colorUtil.test.ts
pnpm lint
pnpm typecheck
pnpm knip
\`\`\`
73 tests pass (60 prior + 13 new).
┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-11671-test-add-unit-tests-for-colorUtil-edge-cases-34f6d73d36508136bac9edfae32815ec)
by [Unito](https://www.unito.io)
|
||
|
Terry Jia
|
fc2a4e82cf |
feat(load3d): bind UI capability gating to ModelAdapterCapabilities (#11711)
> Final piece of the PLY / 3D Gaussian Splatting series. Previous PR made `ModelAdapterCapabilities` load-bearing on the engine side; the UI was still gating off `isSplatModel` / `isPlyModel` proxies. This PR routes the viewer and the viewer-mode dialog through the capability fields directly, so the same source of truth that drives `Load3d` behavior also drives what the user sees. Eighth and last in the series splitting up. ## Summary Snapshot `Load3d.getCurrentModelCapabilities()` into 5 Vue refs on each model load and pipe them through the existing `Load3D` / `Load3DControls` / `Load3dViewerContent` / `ModelControls` / `ViewerModelControls` / `Preview3d` props. Replaces the format-specific `:is-splat-model` / `:is-ply-model` props and the hardcoded "splat → drop light/gizmo/export" subtraction with additive capability gates. No engine behavior changes — capability values are what previous PR already produces; UI now consumes them. ## Changes - **`useLoad3d.ts` / `useLoad3dViewer.ts`**: 5 new refs (`canFitToViewer` / `canUseGizmo` / `canUseLighting` / `canExport` / `materialModes`) refreshed on every load via `load3d.getCurrentModelCapabilities()`. `useLoad3dViewer` extracts the snapshot into a single `captureAdapterFlags(source)` helper because it runs in three places (initializeViewer / initializeStandaloneViewer / loadStandaloneModel). - **`Load3D.vue`**: gate the fit-to-viewer button on `canFitToViewer`; pass capability refs to `Load3DControls` instead of `isSplatModel` / `isPlyModel`. - **`Load3DControls.vue`**: build `availableCategories` additively (`['scene','model','camera']` plus `light` / `gizmo` / `export` if their capability is true) rather than subtracting from a fixed list when `isSplatModel` is true. Forwards `materialModes` to `ModelControls`. - **`Load3dViewerContent.vue`**: gate the light / gizmo / export sidebar sections on the capability refs; pass `materialModes` to `ViewerModelControls`. - **`ModelControls.vue` / `ViewerModelControls.vue`**: drop the local `materialModes` computed (which derived its options from `isPlyModel` and a hardcoded mesh list) and accept `materialModes` as a `readonly MaterialMode[]` prop. An empty array hides the dropdown entirely. - **`Preview3d.vue`** (renderer linearMode): mirror the prop swap on the standalone preview path. ## Review Focus - **Capability prop wiring is the only public-API change for child components**. `ModelControls` and `ViewerModelControls` lost `hideMaterialMode` / `isPlyModel` props. Any extension that imported these components directly will need to migrate, but they're internal `src/components/load3d/controls/**` files and not part of the documented extension surface. - **Empty-`materialModes` semantics**: previously hidden via `:hide-material-mode`; now hidden via `materialModes.length === 0`. `SplatModelAdapter` declares `materialModes: []`, so the splat case keeps the same behavior — the dropdown disappears. PLY adds `'pointCloud'` to the array, so the dropdown picks up that mode automatically without the controls needing an `isPlyModel` branch. - **`captureAdapterFlags` runs after every load completes**, so switching between mesh and splat in the same viewer instance updates the chrome correctly. Verified via the new `Load3D.test.ts` / `Load3dViewerContent.test.ts` cases. - **Capability gating is inclusive of `canFitToViewer`** in this PR even though `Load3DControls` has no fit category — the fit-to-viewer floating button on `Load3D.vue` is what reads it. PLY's `fitToViewer: true` means the button stays visible for PLY users. ## Coverage | File | Stmts | Branch | Funcs | |---|---|---|---| | `Load3D.vue` (modified) | 53.3% | **95.5%** | 83.3% | | `Load3DControls.vue` (modified) | 77.5% | **94.8%** | 86.4% | | `Load3dViewerContent.vue` (modified) | 60.6% | 72.1% | 54.5% | | `controls/ModelControls.vue` (modified) | 16.3% | 0% | 0% | | `controls/viewer/ViewerModelControls.vue` (modified) | **100%** | **100%** | **100%** | | `composables/useLoad3d.ts` (modified) | 78.7% | 64.5% | 71.4% | | `composables/useLoad3dViewer.ts` (modified) | 76.0% | 52.1% | 66.7% | Four new test files (`Load3D.test.ts` / `Load3DControls.test.ts` / `Load3dViewerContent.test.ts` / `controls/viewer/ViewerModelControls.test.ts`) cover the new capability gating directly: each component is rendered with capability flags toggled on/off and the appropriate sidebar / dropdown / button visibility is asserted. Capability prop forwarding from `Load3D.vue` → `Load3DControls.vue` and from `Load3dViewerContent.vue` → `ViewerModelControls.vue` is exercised end-to-end. `controls/ModelControls.vue` is the legacy node-side ModelControls — its existing tests live elsewhere and were not in this PR's scope; the diff line covered (the `v-if="materialModes.length > 0"` swap) is exercised by the new `Load3DControls.test.ts` cases that drive a non-empty / empty `materialModes` through. `Preview3d.vue` (renderer linearMode) has no test file in the project; the prop swap there is the same shape as the `Load3D.vue` swap which is covered. `useLoad3d.ts` / `useLoad3dViewer.ts` percentages are roughly the pre-existing baseline. The diff lines (the 5 new refs and the `captureAdapterFlags` helper) are exercised by the existing composable tests via the mock that now stubs `getCurrentModelCapabilities()`. 73 new component unit tests; 393 total load3d-related tests pass on this branch. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11711-feat-load3d-bind-UI-capability-gating-to-ModelAdapterCapabilities-3506d73d365081b3af68f30e3f728e24) by [Unito](https://www.unito.io) |
||
|
pythongosssss
|
e48d33e4c0 |
test: Canvas grid, ctx menu scaling and group padding settings (#11721)
## Summary Adds tests for canvas snap to grid, context menu scaling and group padding ## Changes - **What**: - add tests for canvas related settings ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11721-test-Canvas-grid-ctx-menu-scaling-and-group-padding-settings-3506d73d36508141868cfa990d903c33) by [Unito](https://www.unito.io) |
||
|
pythongosssss
|
967f1eb562 |
test: extract title editor test component (#11605)
## Summary Extract shared TitleEditor component and update tests to use it ## Changes - **What**: - add title editor helper - update locations that used `TestIds.node.titleInput` ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11605-test-extract-title-editor-test-component-34c6d73d3650811da6b0ec493b190c3f) by [Unito](https://www.unito.io) |
||
|
Kelly Yang
|
8b83559402 |
refactor: extract useBrushPersistence from useBrushDrawing (#11543)
## Summary
PR B of this https://github.com/Comfy-Org/ComfyUI_frontend/pull/11388
Part of the `useBrushDrawing` decomposition plan (PR B).
Extracts brush settings persistence logic into a dedicated
`useBrushPersistence` composable, reducing the responsibility surface of
`useBrushDrawing`.
No runtime behavior is changed — this is a pure structural refactor.
## Changes
- **New** `src/composables/maskeditor/useBrushPersistence.ts` —
encapsulates `loadAndApply` (reads brush settings from localStorage and
applies them to the store on init) and `save` (debounced write of
current brush settings to localStorage)
- **New** `src/composables/maskeditor/useBrushPersistence.test.ts` —
unit tests covering load from empty storage, full restore round-trip,
missing `stepSize` fallback, corrupted data resilience, and
save-to-localStorage behavior
- **Updated** `src/composables/maskeditor/useBrushDrawing.ts` — removes
the inlined persistence functions and delegates to `useBrushPersistence`
## Test Locally
1. Adjust brush size and hardness, close MaskEditor, then reopen it —
brush parameters should restore to the previous settings (verifies
`save` + `loadAndApply`) - pass
2. Draw a few strokes and confirm the marks appear correctly (verifies
the `saveBrushSettings` public interface is not broken) - pass
https://github.com/user-attachments/assets/961155d5-6742-4668-a419-51c29b850edf
<!-- CURSOR_SUMMARY -->
---
> [!NOTE]
> **Low Risk**
> Low risk refactor that moves localStorage read/write logic behind a
new composable and adds unit tests; main risk is accidental behavior
drift in when/what brush settings are persisted/restored.
>
> **Overview**
> Refactors mask editor brush settings persistence by extracting the
localStorage load/save (including debounced writes and `stepSize`
fallback) out of `useBrushDrawing` into a new `useBrushPersistence`
composable, while keeping the `saveBrushSettings` public API wired
through.
>
> Adds `useBrushPersistence` unit tests covering empty storage,
round-trip restore, missing field defaults, corrupted JSON handling, and
save semantics.
>
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
|
||
|
Kelly Yang
|
bc11b5ff5e |
test: add E2E tests for LoginButton toolbar component (#11562)
## Summary
- Add E2E tests for the `LoginButton` toolbar component (FE-109)
- Add `data-testid="login-button"` to `LoginButton.vue` for stable
targeting
- Register `TestIds.topbar.loginButton` in `selectors.ts`
## Changes
- `browser_tests/tests/loginButton.spec.ts` — 7 tests covering:
visibility toggled by `show_signin_button` server feature flag, ARIA
label, click → sign-in dialog, hover popover content, popover dismissal
- `src/components/topbar/LoginButton.vue` — adds `data-testid`
- `browser_tests/fixtures/selectors.ts` — registers new test ID
## Notes
`LoginButton` is rendered in `WorkflowTabs` when `flags.showSignInButton
?? isDesktop` is true. Since `isDesktop = false` in the OSS test build
(`DISTRIBUTION=localhost`), tests enable the button by setting
`window.app.api.serverFeatureFlags.value.show_signin_button = true` —
the established pattern used throughout the test suite (e.g.
`nodeLibraryEssentials.spec.ts`, `shareWorkflowDialog.spec.ts`).
<!-- CURSOR_SUMMARY -->
---
> [!NOTE]
> **Low Risk**
> Low risk: changes are limited to adding stable `data-testid` hooks
plus new Playwright E2E coverage, with only a minor adjustment to a
unit-test performance threshold.
>
> **Overview**
> Adds Playwright E2E coverage for the topbar `LoginButton`, including
feature-flag-driven visibility, ARIA labeling, click-to-open sign-in
dialog, and hover popover behavior (including the *Learn more* link and
dismissal).
>
> To make the tests stable, `LoginButton.vue` now exposes `data-testid`
attributes for the button and popover elements, and `selectors.ts`
registers new `TestIds.topbar.*` entries.
>
> Relaxes the `useModelToNodeStore.getCategoryForNodeType` performance
test threshold (from 10ms to 100ms) while clarifying the intent of the
check.
>
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
|
||
|
Dante
|
8c1ea7ae64 |
test: add unit tests for useNodePricing edge cases (#11673)
## Summary
Extends `useNodePricing.test.ts` with three behavioral gaps the existing
suite did not cover: `getNodePricingConfig` does not leak the compiled
JSONata expression, `pricingRevision` ticks after async evaluation
resolves (with a cache-hit path that does not), and
`formatPricingResult` returns `''` for non-finite numeric inputs across
all four result types.
## Changes
- **What**: Adds 9 Vitest cases across two existing `describe` blocks
(`getNodePricingConfig`, `formatPricingResult`) and one new block
(`reactive revision`). Reuses the existing `priceBadge` and
`createMockNodeWithPriceBadge` helpers.
## Review Focus
- The cache-hit assertion checks that `pricingRevision.value` does not
advance after a second `getNodeDisplayPrice` call with the same
signature, exercising the WeakMap cache hit at `useNodePricing.ts:573`.
- Non-finite coverage spans `type:'usd'`, `type:'range_usd'`,
`type:'list_usd'` (empty + all-non-finite + mixed), and the legacy `{
usd }` shape, matching the four `asFiniteNumber` call sites in
`formatPricingResult`.
- The strip-`_compiled` assertion uses `toHaveProperty` so the test
fails loudly if a future refactor accidentally re-exposes the runtime
JSONata instance to debug consumers.
## Testing
\`\`\`bash
pnpm exec vitest run src/composables/node/useNodePricing.test.ts
pnpm format -- src/composables/node/useNodePricing.test.ts
pnpm lint
pnpm typecheck
pnpm knip
\`\`\`
91 tests pass (82 prior + 9 new).
┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-11673-test-add-unit-tests-for-useNodePricing-edge-cases-34f6d73d365081dab525cdaa71b348a5)
by [Unito](https://www.unito.io)
|
||
|
Dante
|
69e68847d9 |
test: add unit tests for workspaceAuthStore retry/race paths (#11674)
## Summary
Extends `useWorkspaceAuth.test.ts` with the retry, race,
storage-resilience, and Zod-validation paths the existing suite did not
exercise: exponential backoff on `TOKEN_EXCHANGE_FAILED`, immediate
context clearing on permanent errors, stale-refresh abort when the user
switches workspaces mid-flight, and resilience to `sessionStorage` quota
errors.
## Changes
- **What**: Adds 6 Vitest cases across three new `describe` blocks
(`refreshToken retry/race paths`, `persistToSession resilience`, `Zod
validation on token response`). Reuses the existing `mockGetIdToken`,
`mockTeamWorkspacesEnabled`, `vi.useFakeTimers()`, and
`mockTokenResponse` fixtures.
## Review Focus
- The retry test uses `vi.runAllTimersAsync()` so the three backoff
sleeps (1s + 2s + 4s) drain in a single tick instead of slowing the
suite. Both `console.warn` (per-attempt) and `console.error`
(final-failure) are silenced.
- The race test resolves the in-flight refresh fetch with a token tied
to the OLD workspace AFTER `switchWorkspace('workspace-other')` has run,
so the assertion fails loudly if the stale-request guard regresses.
- The sessionStorage spy targets the instance method
(`vi.spyOn(sessionStorage, 'setItem')`); spying
`Storage.prototype.setItem` does not intercept happy-dom's per-instance
method.
## Testing
\`\`\`bash
pnpm exec vitest run
src/platform/workspace/stores/useWorkspaceAuth.test.ts
pnpm format -- src/platform/workspace/stores/useWorkspaceAuth.test.ts
pnpm lint
pnpm typecheck
pnpm knip
\`\`\`
33 tests pass (27 prior + 6 new).
┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-11674-test-add-unit-tests-for-workspaceAuthStore-retry-race-paths-34f6d73d365081e6a8ecce59a156585e)
by [Unito](https://www.unito.io)
|
||
|
comfydesigner
|
fad9cf0db7 |
fix: consolidate --color-coral-red variables into --color-coral (#10374)
Removes the desktop-exclusive `--color-coral-red` CSS variables and replaces their usage with the shared `--color-coral` palette to reduce variable duplication in the design system. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-10374-fix-consolidate-color-coral-red-variables-into-color-coral-32a6d73d365081a4ac88d0ea96aeea02) by [Unito](https://www.unito.io) --------- Co-authored-by: Alex <alex@Mac.lan> Co-authored-by: Amp <amp@ampcode.com> Co-authored-by: Alexander Brown <drjkl@comfy.org> Co-authored-by: DrJKL <DrJKL0424@gmail.com> |
||
|
pythongosssss
|
d532fcf779 |
test: add tests for canvas related settings (#11604)
## Summary Adds test coverage for canvas related settings ## Changes - **What**: - tests canvas info visibility, fps, pointer modes, selection ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11604-test-add-tests-for-canvas-related-settings-34c6d73d365081748b0ec79bc6fdc6ca) by [Unito](https://www.unito.io) --------- Co-authored-by: github-actions <github-actions@github.com> |
||
|
pythongosssss
|
52e73f2697 |
test: Expand node search box V2 e2e coverage (#10620)
## Summary Adds additional browser test coverage to the v2 node search ## Changes - **What**: - extend search v2 fixtures - add additional tests - add data-testid for targeting elements - rework tests to work with new menu UI ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-10620-test-Expand-node-search-box-V2-e2e-coverage-3306d73d365081b6bad3f73daab1194f) by [Unito](https://www.unito.io) --------- Co-authored-by: GitHub Action <action@github.com> |
||
|
Simon Pinfold
|
c4043637d6 |
fix: route context menu Download through downloadMultipleAssets (#11700)
*PR Created by the Glary-Bot Agent* --- ## Summary The asset context menu's Download action called `downloadAsset` directly, so multi-output jobs only downloaded the preview file instead of all outputs. Route through `downloadMultipleAssets`, which detects multi-output jobs and creates a ZIP export in cloud mode and falls back to the single-download path otherwise. ## Changes - **What**: Swap `actions.downloadAsset(asset)` for `actions.downloadMultipleAssets([asset])` in the per-asset context menu Download command, and extend the existing unit test to assert the routing. - **Breaking**: none - **Dependencies**: none ## Review Focus For single-output assets the behavior is unchanged: `downloadMultipleAssets([asset])` falls through to the individual-download path when `hasMultiOutputJobs` is false and `assets.length === 1` (see `useMediaAssetActions.ts:106`). Verified manually — right-clicking a single-output asset and clicking Download still produces one file download to the correct `/api/view` URL. ## Notes This is a focused replacement for the stale #10948. Compared to that branch: - Drops the unrelated `bootstrapStore` API-key auth changes (scope creep). - Drops the new `assets.cloud.spec.ts` Playwright spec — cloud asset-export E2E coverage was added in #11610 (`browser_tests/tests/sidebar/assets.spec.ts`), so a separate cloud spec for this routing change would mostly duplicate it. - Keeps the unit-test change minimal: extends the existing `ContextMenu` stub with a `model` prop watcher and adds one new test, rather than rewriting the whole file from `@testing-library/vue` to `@vue/test-utils`. ## Verification - `pnpm test:unit` (MediaAssetContextMenu.test.ts and useMediaAssetActions.test.ts) - `pnpm typecheck` - `pnpm lint` - `pnpm format` / `oxfmt --check` - `pnpm knip` - Manual: started the OSS dev server, generated a single-output asset via the queue API, opened the assets sidebar, right-clicked the asset, and confirmed the Download menu item triggers a single-file download (screenshot attached). ## Screenshots   ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11700-fix-route-context-menu-Download-through-downloadMultipleAssets-34f6d73d365081eb8135e8b699640d97) by [Unito](https://www.unito.io) --------- Co-authored-by: Glary-Bot <glary-bot@users.noreply.github.com> |
||
|
Alexander Brown
|
9d61b4df06 |
feat: ECS Phase 0b — ID type aliases (FE-166/475/476/477) (#11699)
## Summary
ECS Phase 0b — type-only ID aliases. Builds on FE-165 (centralized
version counter, base of this PR) and adds a tranche of named ID aliases
plus mechanical adoption at known call sites.
This PR now covers four child tickets:
- **FE-166** — adds `GroupId` (in `LGraphGroup.ts`) and `SlotIndex` (in
`interfaces.ts`); re-exported from the litegraph barrel.
- **FE-475** — mechanical adoption of existing aliases (`NodeId`,
`LinkId`, `RerouteId`, `GroupId`, `SlotIndex`, `ExecutionId`) across
litegraph at the audit-listed sites:
`LGraphState.lastGroupId/lastLinkId/lastRerouteId`,
`LGraphExtra.linkExtensions`, `ISerialisedGroup.id`,
`ISerialisedGraph.last_link_id`, `LinkNetwork.removeReroute`,
`INodeOutputSlot.slot_index`, `LGraphNode.{setOutputDataType,
getInputDataType, getOutputPos}` slot params,
`ExecutableNodeDTO.inputs[].linkId` + execution-id locals, and
`RenderLink/MovingLinkBase.fromSlotIndex` (plus subclasses that
redeclare).
- **FE-476** — adds `SubgraphId = UUID` in `LGraph.ts`; adopted at
`_subgraphs` Map, `findUsedSubgraphIds`, `getDirectSubgraphIds`, and
`ExportedSubgraphInstance.type`. Re-exported from the litegraph barrel.
- **FE-477** — adds app-domain entity aliases at their closest
schema/types files: `WorkflowId`, `AssetId`, `PromptId` (propagated as
existing `JobId`), `TaskId`, `UserId`, `WorkspaceId`,
`WorkspaceInviteId`, `NodePackId`. Adopted at primary use sites (entity
id fields, store state, service signatures).
## Entity reference
### ID aliases at a glance
| Alias | Underlying | Defined in | Identifies |
| --- | --- | --- | --- |
| `NodeId` | `number \| string` | `litegraph/LGraphNode.ts` | A node
within a graph |
| `LinkId` | `number` | `litegraph/LLink.ts` | A connection between two
slots |
| `RerouteId` | `number` | `litegraph/Reroute.ts` | A reroute waypoint
on a link |
| `GroupId` *(new)* | `number` | `litegraph/LGraphGroup.ts` | A visual
group of nodes |
| `SlotIndex` *(new)* | `number` | `litegraph/interfaces.ts` | A slot's
position on a node |
| `ExecutionId` | `string` | `litegraph/types/serialisation.ts` | A node
within a subgraph instance |
| `SubgraphId` *(new)* | `UUID` | `litegraph/LGraph.ts` | A subgraph
definition |
| `WorkflowId` *(new)* | `string` |
`platform/workflow/validation/schemas/workflowSchema.ts` | A saved
workflow document |
| `AssetId` *(new)* | `string` |
`platform/assets/schemas/assetSchema.ts` | A binary asset (model, image,
etc.) |
| `JobId` *(reused as `PromptId`)* | `string` | `schemas/apiSchema.ts` |
A queued prompt execution |
| `TaskId` *(new)* | `string` | `platform/tasks/services/taskService.ts`
| A backend background task |
| `UserId` *(new)* | `string` | `types/authTypes.ts` | An authenticated
user |
| `WorkspaceId` *(new)* | `string` |
`platform/workspace/workspaceTypes.ts` | A workspace |
| `WorkspaceInviteId` *(new)* | `string` |
`platform/workspace/workspaceTypes.ts` | A pending workspace invite |
| `NodePackId` *(new)* | `string` |
`workbench/extensions/manager/types/comfyManagerTypes.ts` | A Comfy
Registry / Manager node pack |
### How the entities relate
```mermaid
flowchart TB
subgraph LG["🎨 Litegraph entities"]
direction TB
Graph["LGraph<br/>id: SubgraphId"]
Subgraph["Subgraph<br/>id: SubgraphId"]
Node["LGraphNode<br/>id: NodeId"]
Link["LLink<br/>id: LinkId"]
Reroute["Reroute<br/>id: RerouteId"]
Group["LGraphGroup<br/>id: GroupId"]
Slot["INodeSlot<br/>slot_index: SlotIndex"]
Exec["ExecutableNodeDTO<br/>id: ExecutionId"]
end
subgraph AP["🌐 App-domain entities"]
direction TB
Workflow["ComfyWorkflow<br/>id: WorkflowId"]
Job["Prompt / Job<br/>id: JobId ≡ PromptId"]
Task["Task<br/>id: TaskId"]
Asset["Asset<br/>id: AssetId"]
Workspace["Workspace<br/>id: WorkspaceId"]
Invite["WorkspaceInvite<br/>id: WorkspaceInviteId"]
User["User<br/>id: UserId"]
Pack["NodePack<br/>id: NodePackId"]
end
Subgraph -. extends .-> Graph
Graph --> Node
Graph --> Link
Graph --> Group
Node --> Slot
Link --> Reroute
Link --> Slot
Node -. instantiates .-> Subgraph
Exec -. wraps .-> Node
Workflow -. serializes .-> Graph
Job --> Workflow
Job --> Task
Task --> Asset
Workspace --> Workflow
User --> Workspace
Workspace --> Invite
Pack -. provides .-> Node
```
Solid arrows are containment / direct references; dashed arrows are *“is
a kind of”* (`extends`) or cross-layer relationships (e.g. a
`ComfyWorkflow` *serializes* an `LGraph`; a `NodePack` *provides* node
definitions).
## Explicit non-goals
- `LGraphState.lastNodeId` is intentionally kept as bare `number`
(auto-increment counter; would widen if aliased to `NodeId = number |
string`).
- No new `SubgraphSlotId` alias — verified subsidiary (subgraph IO slots
are addressed via `SUBGRAPH_INPUT_ID/OUTPUT_ID` sentinel + numeric array
index, not by UUID alone).
- No `WidgetName`, `SlotName`, `WorkspaceMemberId` — verified subsidiary
(only meaningful inside a parent or as a relationship).
- No re-typing of `LGraph.id` / `Subgraph.id` — references adopt
`SubgraphId`, but the inherited UUID typing is left intact (minimal
diff).
## Type-only
All changes are structural-equivalent type aliases. No runtime behavior
changes. No new exports beyond the aliases themselves. No generated code
modified.
## Verification
- `pnpm typecheck` ✅
- `pnpm knip` ✅
- Scoped `npx eslint` on changed files ✅
- Lint-staged hooks (oxfmt, oxlint, eslint, typecheck) passed on every
commit
## Notes for reviewers
This branch was rebased onto `main` after FE-165 (`a441364a5`, PR
#11698) merged independently — the auto-skipped FE-165 commit is no
longer part of this PR. Six commits remain (oldest → newest):
| Commit | Maps to | Summary |
| --- | --- | --- |
| `e8e7ff795` | FE-166 | Add `GroupId` and `SlotIndex` aliases + barrel
re-exports |
| `e0bcb75a0` | FE-476 | Add `SubgraphId = UUID` alias |
| `2c136afb9` | FE-477 + FE-475 bulk | Add app-domain aliases;
mechanical adoption of
`NodeId`/`LinkId`/`RerouteId`/`GroupId`/`SlotIndex`/`ExecutionId`/`SubgraphId`
at audit-listed litegraph sites |
| `06d6e6a8b` | FE-477 | Adopt `TaskId` in asset stores |
| `f943e1c2b` | FE-476 | Adopt `SubgraphId` at remaining UUID reference
sites (`LGraphCanvas` clipboard map + paste, `SubgraphNode.type`) |
| `1739d5241` | review feedback | Tighten alias usage:
`linkExtensions.parentId: RerouteId`, drop redundant `String()` wraps in
`executionStore`, type `assetExportStore` map as `Map<TaskId,
AssetExport>` |
FE-475's mechanical adoption is bundled into `2c136afb9` rather than a
dedicated commit (parallel-agent execution on a shared working tree);
the substitutions themselves are complete — see the diff under
`src/lib/litegraph/src/`. PR will be squash-merged, so commit
granularity is informational.
Fixes FE-166
Fixes FE-475
Fixes FE-476
Fixes FE-477
---------
Co-authored-by: Amp <amp@ampcode.com>
|
||
|
Christian Byrne
|
963a7bf178 |
refactor: consolidate browser_tests/helpers/ into fixtures/ (#11411)
*PR Created by the Glary-Bot Agent* --- ## Summary - Eliminates the confusing dual-helpers structure where `browser_tests/helpers/` and `browser_tests/fixtures/helpers/` coexisted one tier apart with overlapping purposes - Routes each file to its natural home based on what it actually *is*: page objects → `components/`, standalone utils → `utils/`, domain helper classes stay in `helpers/` - Adds an ESLint guard (`no-restricted-imports`) to prevent re-creating `browser_tests/helpers/` ## File Moves | File | From | To | Reason | |---|---|---|---| | `actionbar.ts` | `helpers/` | `fixtures/components/Actionbar.ts` | Page object class imported by ComfyPage | | `templates.ts` | `helpers/` | `fixtures/components/Templates.ts` | Page object class imported by ComfyPage | | `boundsUtils.ts` | `fixtures/helpers/` | `fixtures/utils/` | Pure function, not a helper class | | `mimeTypeUtil.ts` | `fixtures/helpers/` | `fixtures/utils/` | Pure function, not a helper class | | `builderTestUtils.ts` | `helpers/` | `fixtures/utils/` | Shared test setup functions | | `clipboardSpy.ts` | `helpers/` | `fixtures/utils/` | Page injection utility | | `fitToView.ts` | `helpers/` | `fixtures/utils/` | Canvas utility function | | `manageGroupNode.ts` | `helpers/` | `fixtures/utils/` | Litegraph interaction helper | | `painter.ts` | `helpers/` | `fixtures/utils/` | Test helper functions | | `perfReporter.ts` | `helpers/` | `fixtures/utils/` | Test infrastructure | | `promotedWidgets.ts` | `helpers/` | `fixtures/utils/` | Query helpers for specs | ## What Changed Beyond File Moves - **28 import statements** updated across test specs, fixtures, and infra files - **AGENTS.md** — directory tree diagram and architectural separation descriptions updated - **README.md** — "Leverage Existing Fixtures and Helpers" section updated - **`.claude/skills/perf-fix-with-proof/SKILL.md`** — perfReporter path reference updated - **`eslint.config.ts`** — added `@e2e/helpers/*` restricted import pattern to both spec and non-spec browser_tests rules ## Verification - `pnpm typecheck` — clean - `pnpm typecheck:browser` — clean - `pnpm lint` — 0 errors, 0 warnings - `pnpm format:check` — all files formatted - `pnpm knip` — clean - Pre-commit hooks passed full pipeline (oxfmt, oxlint, eslint, typecheck, typecheck:browser) ## Config Audit No changes needed to: `tsconfig.json` (`@e2e/*` alias covers all subdirs), `playwright.config.ts`, `vite.config.mts`, `knip.config.ts`, `.oxlintrc.json`, `nx.json` ## Manual Verification Note This is a pure structural refactoring (file moves + import updates) with zero behavioral or visual changes. The typecheck and lint passes confirm all imports resolve correctly. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11411-refactor-consolidate-browser_tests-helpers-into-fixtures-3476d73d3650816cb671ef7fa8433f66) by [Unito](https://www.unito.io) --------- Co-authored-by: glary-bot <glary-bot@comfy.org> Co-authored-by: Glary-Bot <glary-bot@users.noreply.github.com> Co-authored-by: DrJKL <DrJKL0424@gmail.com> Co-authored-by: Amp <amp@ampcode.com> |
||
|
Terry Jia
|
f001bc9af3 |
feat: derive Preview3D camera pose from EXTRINSICS/INTRINSICS matrices (#11626)
> Prerequisite work for improved PLY / 3D Gaussian Splatting support — splat workflows (and PLY pipelines that go through SHARP / COLMAP) emit camera pose as extrinsics + intrinsics matrices, and the viewer needs a way to consume them before that end-to-end story can ship. ## Summary Adds the ability to drive the Preview3D camera from a pair of OpenCV-convention extrinsics + intrinsics matrices (as produced by SHARP / COLMAP / other SfM pipelines), so backend nodes that emit such matrices can position the viewer's camera deterministically. Fifth in the series splitting up https://github.com/Comfy-Org/ComfyUI_frontend/pull/11495. Fully self-contained — adds new API surface only, no existing call paths are modified. ## Changes - **What**: - `cameraFromMatrices.ts` (new): pure function `computeCameraFromMatrices(extrinsics, intrinsics)` returning `{ position, target, fovYDegrees }`. No THREE.js dependency. Shape-validates the inputs (4×4 and 3×3) with a clear error. - `Load3d.setCameraFromMatrices(extrinsics, intrinsics)`: wires the utility into Load3d via `setCameraState` + `setFOV`. Preserves the caller's existing `zoom` and `cameraType`. - `load3d.ts` (extension): Preview3D's `onExecuted` extracts `extrinsics`/`intrinsics` from `result[3]`/`result[4]` (when present) and calls `setCameraFromMatrices`. The output tuple type is widened to include the two optional `Matrix` fields. ## Review Focus - **Convention conversion**: OpenCV is Y-down, Z-forward; three.js is Y-up, Z-back. The function flips Y and Z on both `position` and `target` (equivalent to a 180° X-axis rotation of the whole world). The same rotation is applied elsewhere to splats at load time, so a future splat + camera-pose pair lines up. - **FOV math**: vertical FOV (radians) = `2 * atan(cy / fy)`. We expose it in degrees, matching `cameraManager.setFOV`'s contract. - **Camera-state preservation**: `setCameraFromMatrices` reads the current state to keep `zoom` and `cameraType` intact — only `position`, `target`, and `fov` come from the matrices. - **Backwards compatibility**: backend nodes that don't return matrices simply skip the new branch (the `if (extrinsics && intrinsics)` guard). Existing Preview3D workflows are unaffected. - 7 unit tests for the matrix math (identity, rotation, translation, FOV derivation, shape errors, etc.); 1 unit test for the Load3d wiring (verifies the destructured `position`/`target`/`fovYDegrees` reach `setCameraState` + `setFOV` with `zoom`/`cameraType` preserved). ## Coverage | File | Stmts | Branch | Funcs | Lines | |---|---|---|---|---| | `cameraFromMatrices.ts` (new) | **100%** | **100%** | **100%** | **100%** | | `Load3d.ts` (modified) | ~7.6% | 0% | ~14.6% | ~7.7% | | `load3d.ts` (extension, modified) | 0% | 0% | 0% | 0% | The `Load3d.ts` numbers are the pre-existing baseline — `Load3d.test.ts` covers façade methods via prototype injection rather than instantiating the class (the constructor needs `THREE.WebGLRenderer`, which happy-dom can't provide). The new `setCameraFromMatrices` method body is exercised by a new unit test that asserts `setCameraState` receives the destructured matrix output and `setFOV` receives the computed `fovYDegrees`, with the caller's `zoom`/`cameraType` preserved. `load3d.ts` (the extension registration file, 0% on `main` and after this PR) has no unit-test scaffolding in the project — its `onExecuted` handler runs only after a workflow execution and is exercised end-to-end via browser tests. The new `if (extrinsics && intrinsics) load3d.setCameraFromMatrices(...)` branch sits in that path. Net: the matrix math itself, which previously didn't exist, is now 100% covered. The wiring layer relies on the same e2e safety net the surrounding code has always used. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11626-feat-derive-Preview3D-camera-pose-from-EXTRINSICS-INTRINSICS-matrices-34d6d73d3650817297bdf25a83a4f7a2) by [Unito](https://www.unito.io) |
||
|
Kelly Yang
|
88faaf3d86 |
test: complete remaining Painter widget E2E tests (#11613)
## Summary
Implemented E2E test coverage for Levels 6-12 of the Painter Widget
## Changes
Adds the following coverage to complete the test plan:
Level 6 - Input image connection (3 tests, @slow):
- Width/height/bg-color controls hide when input is linked
- Canvas resizes to match input image dimensions after execution
- Drawing over input image produces canvas content
Level 7 - Clear on empty canvas is harmless
Level 8 - Unchanged canvas does not re-upload on second serialization
Level 9 - Settings persistence:
- Tool selection saved to node.properties.painterTool
- Brush size change saved to node.properties.painterBrushSize
Level 10 - Compact layout collapses to grid-cols-1 when node width <
350px
Level 12 - Rapid drawing accumulates all strokes (checks 3 y-positions)
Supporting changes:
- Add data-testid="painter-controls" to controls grid in
WidgetPainter.vue (needed for compact mode class assertion)
- Add browser_tests/assets/widgets/painter_with_input.json workflow
fixture (LoadImage connected to Painter input slot 0)
<!-- CURSOR_SUMMARY -->
---
> [!NOTE]
> **Low Risk**
> Mostly adds/adjusts Playwright and unit test coverage; the only
runtime change is wrapping pointer-capture calls in `try/catch`, which
is low-risk but touches input-handling paths.
>
> **Overview**
> Completes and expands Painter widget browser test coverage, including
new scenarios for clearing an empty canvas, preventing redundant uploads
when serializing an unchanged canvas, persisting tool/brush-size
settings to node properties, compact layout behavior, multi-stroke
accumulation checks, and an input-image-connected workflow (new
`painter_with_input.json`) with execution/resizing/draw-over-image
assertions.
>
> Hardens `usePainter` pointer handling by tolerating
`setPointerCapture`/`releasePointerCapture` failures (e.g., synthetic
events), with corresponding unit tests updated/added to validate the
behavior and serialization expectations.
>
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
|
||
|
Terry Jia
|
42ff7b6c62 |
refactor(load3d): drive viewer behavior from ModelAdapter capabilities (#11660)
> Final architectural step in the PLY / 3D Gaussian Splatting series. Previous PR introduced `ModelAdapter` with a dormant `capabilities` field; this PR makes those capabilities load-bearing and replaces the remaining `instanceof SplatMesh` / `instanceof BufferGeometry` switches with adapter-driven dispatch. Together with previous one it removes the last of the format-specific branching from `SceneModelManager` / `Load3d`. Seventh in the series splitting up the https://github.com/Comfy-Org/ComfyUI_frontend/pull/11495. ## Summary Drive viewer behavior (fit-to-viewer, default camera pose, world bounds, GPU dispose, material rebuild) from `ModelAdapterCapabilities` + 3 new optional adapter methods, instead of `SceneModelManager` reflecting on model shape. `Load3d` is rewritten to take its 13 managers as injected `Load3dDeps`; a new `createLoad3d` factory assembles them and threads a single `AdapterRef` between `LoaderManager` (writer) and `SceneModelManager` + `Load3d` (readers). Splat orientation + decoder-race + sizing bugs are fixed as a side effect — splats now render upright, fill the grid, and don't lock the OrbitControls target on first frame. ## Changes - **`ModelAdapter.ts`**: add `AdapterRef = { current: ModelAdapter | null }` shared handle and 3 optional adapter methods — `computeBounds(model)`, `disposeModel(model)`, `defaultCameraPose()`. - **`SplatModelAdapter.ts`**: implement all 3 optional methods; `await splatMesh.initialized` so first-frame bounds are populated (fixes a decoder race that collapsed the OrbitControls target onto the camera); `quaternion.set(1, 0, 0, 0)` to convert sparkjs's OpenCV (Y-down, Z-forward) to three.js (Y-up, Z-back); flip `fitToViewer` back to `true` and bump `fitTargetSize` to `20` so splats fill the 20-unit grid instead of shrinking to 1/4 of it. - **`PointCloudModelAdapter.ts`**: extract `buildPointCloudForMaterialMode` so `SceneModelManager` rebuilds via the same code path the initial load uses; `setPath` so PLYs that reference relative assets resolve correctly. - **`LoaderManager.ts`**: accept optional `AdapterRef`; write through it instead of the internal `_currentAdapter` field. `clearModel()` now runs while the old adapter is still current so its `disposeModel()` can release renderer-owned resources. - **`SceneModelManager.ts`**: accept 4 capability lambdas (`getCurrentCapabilities` / `getBoundsFromAdapter` / `disposeModelViaAdapter` / `getDefaultCameraPose`) with `DEFAULT_MODEL_CAPABILITIES` / null fallbacks. `setupModel`, `fitToViewer`, and material-mode rebuild are now capability-driven; `containsSplatMesh` (30 lines of `instanceof` traversal) and `handlePLYModeSwitch` (90 lines of duplicated PLY rebuild) are gone. - **`Load3d.ts`**: ctor switches from manager-creation to deps-injected (`Load3dDeps`); add `getCurrentModelCapabilities()` reader; gate `setGizmoEnabled` / `setGizmoMode` / `resetGizmoTransform` / `applyGizmoTransform` on `capabilities.gizmoTransform`; `isSplatModel` / `isPlyModel` now read `adapterRef.current?.kind` directly. - **`createLoad3d.ts`** (new): single factory that builds the renderer (`createRenderer`), assembles all 13 managers in dependency order, and threads one shared `AdapterRef` through `LoaderManager` and `SceneModelManager`'s 4 capability lambdas. - **`useLoad3d.ts` / `useLoad3dViewer.ts`**: switch from `new Load3d(container, options)` to `createLoad3d(container, options)`. No other call-site changes. ## Review Focus - **Capability dispatch parity**: walk each former hardcoded branch in `SceneModelManager` and confirm it now falls out of the right capability: - `containsSplatMesh()` → `!capabilities.fitToViewer` + `getDefaultCameraPose()` - `handlePLYModeSwitch()` → `capabilities.requiresMaterialRebuild` + `buildPointCloudForMaterialMode()` - `Box3.setFromObject(model)` for sizing → `getBoundsFromAdapter(model) ?? Box3.setFromObject(model)` - Mesh/Points geometry+material disposal in `clearModel` → still happens, plus `disposeModelViaAdapter(obj)` for adapter-owned resources (sparkjs SplatMesh internal GPU state) - **`AdapterRef` lifecycle**: one ref is created in `createLoad3d`, passed to `LoaderManager` (writer) and `SceneModelManager` (read via 4 closures). `LoaderManager.loadModel` clears via the *old* adapter first (so `disposeModel` runs), then null-resets the ref before picking the new one. Test `keeps the old adapter current while clearModel runs` pins this ordering. - **Splat fixes are user-visible, not pure refactor**: - Orientation: `quaternion.set(1, 0, 0, 0)` matches the sparkjs README convention. Without it splats render upside-down and mirrored on Z. Same rotation is applied to the camera-from-matrices output in PR-E so a future splat + camera-pose pair lines up. - Decoder race: `await splatMesh.initialized` ensures `getBoundingBox` returns a non-zero box on the first call. Without it `setupModel`'s bounds → camera pipeline placed the OrbitControls target on the camera origin, locking the view. - Sizing: `fitTargetSize: 20` (vs. the mesh default of 5) means splat geometry spans the full 20-unit grid footprint instead of ~1/4 of it. Mesh assets are unaffected. - **Gizmo gating**: `setGizmoEnabled(true)` early-returns when `capabilities.gizmoTransform` is false. Internal `setGizmoEnabled(false)` still runs (so we can always disable). `setGizmoMode` / `resetGizmoTransform` / `applyGizmoTransform` no-op when the capability is off. - **`createLoad3d` is the single ctor entry**: `new Load3d(...)` is no longer callable from app code (ctor signature changed to `(container, deps, options)`). All call sites use `createLoad3d`. Test scaffolding still uses `Object.create(Load3d.prototype)` + property injection where it needs to bypass renderer creation. - **Backwards compatibility**: `LoaderManager`'s `adapterRef` and `SceneModelManager`'s 4 capability lambdas all have defaults (`createAdapterRef()` and `() => DEFAULT_MODEL_CAPABILITIES` etc.), so the existing test suites that construct these classes with the old signatures still compile and pass without modification beyond what's in this PR. ## Coverage | File | Stmts | Branch | Funcs | Lines | |---|---|---|---|---| | `ModelAdapter.ts` (modified) | **100%** | **100%** | **100%** | **100%** | | `LoaderManager.ts` (modified) | **100%** | 91.7% | 86.7% | **100%** | | `MeshModelAdapter.ts` (unchanged) | **100%** | **100%** | **100%** | **100%** | | `PointCloudModelAdapter.ts` (modified) | **97.9%** | 69.2% | 71.4% | **97.9%** | | `SplatModelAdapter.ts` (modified) | **100%** | **100%** | **100%** | **100%** | | `SceneModelManager.ts` (modified) | 75.4% | 67.2% | 72.2% | 75.4% | | `Load3d.ts` (modified) | 29.5% | 30.6% | 26.7% | 30.1% | | `createLoad3d.ts` (new) | 83.8% | **100%** | 58.3% | 83.8% | | `useLoad3d.ts` (modified) | 78.2% | 65.1% | 71.4% | 82.2% | | `useLoad3dViewer.ts` (modified) | 75.2% | 52.1% | 65.9% | 79.4% | `SplatModelAdapter.ts` jumps to 100% via 6 new tests covering the orientation set, the `await initialized` decoder wait, `computeBounds` (world-space transform + null fallback), `disposeModel` (per-SplatMesh dispose + no-op on non-splat trees), and `defaultCameraPose`. `createLoad3d.ts` hits 100% branch via a new test file with 12 cases — `WebGLRenderer` config, `Load3DOptions` forwarding, `AdapterRef` identity between `LoaderManager` and `SceneModelManager`, and the 4 capability lambdas in both adapter-null and adapter-published states (each delegates correctly to the adapter's optional methods or falls back to defaults). The remaining func% reflects the inline `gizmoTransformChange` callback — not a deliberate skip, just out of scope for the dispatch-wiring tests. `SceneModelManager.ts` and `Load3d.ts` numbers are the pre-existing baseline — the existing `*.test.ts` files cover façade methods via prototype injection rather than instantiating the classes (`Load3d` constructor needs `THREE.WebGLRenderer`, which happy-dom can't provide; `SceneModelManager` covers the new capability paths via its existing `createManager(overrides)` helper). All new branches (capability gating, capability-driven `setupModel` / `fitToViewer` / rebuild, adapter-driven `isSplatModel` / `isPlyModel`) have dedicated tests. Net diff: **+846 / −370** across 16 files (10 production, 6 test). ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11660-refactor-load3d-drive-viewer-behavior-from-ModelAdapter-capabilities-34f6d73d36508130b0ece884add182b9) by [Unito](https://www.unito.io) |
||
|
Dante
|
f404887c96 |
test: add unit tests for linkFixer (#11668)
## Summary Adds unit coverage for `linkFixer` serialized graph repair paths and fixes input-slot repairs being tracked without mutating the serialized workflow data. ## Changes - **What**: Adds 10 Vitest cases for valid links, dry-run reporting, origin/target repair, missing input-slot cleanup, dangling node cleanup, stale link deletion, and silent mode. - **What**: Applies serialized input slot mutations in fix mode and reports `fixed` only after a rerun confirms the graph is clean. - **What**: Adds the design-system package to knip workspace config so the pre-push hook recognizes its exported CSS dependency usage. - **Dependencies**: None. ## Review Focus The workflow validation path calls `fixBadLinks` with serialized workflow JSON, so the new tests stay at that boundary and assert the repaired graph shape directly. ## Test Coverage - CI snapshot for `src/utils/linkFixer.ts`: unit lines 1.8%. - Local targeted coverage: unit lines 83.9%, functions 94.11%, branches 75.15%. ## Testing ```bash pnpm format -- src/utils/linkFixer.ts src/utils/linkFixer.test.ts pnpm test:unit -- src/utils/linkFixer.test.ts pnpm test:unit -- src/utils/linkFixer.test.ts --coverage pnpm typecheck pnpm lint pnpm knip ``` ## screenshoot <img width="1691" height="927" alt="Screenshot 2026-04-28 at 10 09 26 AM" src="https://github.com/user-attachments/assets/ff59888f-bde3-48f5-853a-60df69e84492" /> |
||
|
Comfy Org PR Bot
|
b37c66539b |
1.44.12 (#11705)
Patch version increment to 1.44.12 **Base branch:** `main` ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11705-1-44-12-3506d73d36508121bc39e748c87a6235) 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>v1.44.12 |
