mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-05-14 01:36:14 +00:00
294d90f36ea9137461cfd467daa4ce29ca466607
5556 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Simon Pinfold
|
3a05a37323 |
Fix naming strategy for multi-job asset exports (#11610)
## Summary Use `group_by_job_time` for exports spanning multiple jobs while keeping single-job exports on `preserve`, and add regression coverage for the new naming-strategy behavior. ## Changes - **What**: updated the asset export payload and request typing for the new naming-strategy values, added unit coverage for single-job vs multi-job export requests, added `@cloud` sidebar browser coverage for export payloads, and adjusted the cloud Playwright setup helpers so setup API calls can hit the backend directly and Firebase auth is seeded on the app origin - **Breaking**: none - **Dependencies**: none ## Review Focus Please sanity-check the cloud Playwright harness changes in `ComfyPage` and `CloudAuthHelper`, plus the single-job vs multi-job export naming-strategy assertions in the new browser tests. ## Screenshots (if applicable) N/A ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11610-Fix-naming-strategy-for-multi-job-asset-exports-34c6d73d365081a68a88ea38d897578f) by [Unito](https://www.unito.io) --------- Co-authored-by: Amp <amp@ampcode.com> Co-authored-by: Alexander Brown <drjkl@comfy.org> |
||
|
Dante
|
6cfa5fdb1f |
test: add unit tests for assetsStore (#11672)
## Summary Extends `assetsStore.test.ts` with behavioral coverage for the optimistic-update flows (`updateAssetMetadata`, `updateAssetTags`), the per-asset deletion-state tracker (`setAssetDeleting` / `isAssetDeleting`), the input-name resolution (`getInputName` / `inputAssetsByFilename`), and the cloud-routing branch of `updateInputs`. ## Changes - **What**: Adds 8 Vitest cases across 4 new `describe` blocks (`updateAssetMetadata optimistic cache`, `updateAssetTags diff-based dispatch`, `setAssetDeleting / isAssetDeleting`, `getInputName`, `updateInputs cloud routing`). Extends the shared `assetService` mock with `updateAsset`, `addAssetTags`, and `removeAssetTags` (none of which were previously stubbed). ## Review Focus - Cloud-routed tests flip `mockIsCloud.value` inside a `try/finally` so the existing default (`false`) is restored even if an assertion throws — same pattern the existing Cloud describe block uses. - The optimistic-cache rollback test silences the `console.error` invoked by the catch branch so the test output stays clean. - The `updateAssetTags` tests pin both the no-op short-circuit and the add-only path. Remove-only and combined add+remove are already exercised indirectly through the existing tag-cache invalidation tests. ## Testing \`\`\`bash pnpm exec vitest run src/stores/assetsStore.test.ts pnpm format -- src/stores/assetsStore.test.ts pnpm lint pnpm typecheck pnpm knip \`\`\` 47 tests pass (39 prior + 8 new). ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11672-test-add-unit-tests-for-assetsStore-34f6d73d3650819f8e43e7154541baef) by [Unito](https://www.unito.io) |
||
|
Benjamin Lu
|
3ea75e1c48 |
fix: localize secret date labels (#11524)
## Summary
Localizes secret list date labels through Vue I18n date formatting
instead of the browser default numeric date format, with Storybook
coverage for design review.
## Changes
- **What**: Replaces `toLocaleDateString()` with `useI18n().d(..., {
dateStyle: 'medium' })` for `SecretListItem` dates.
- **What**: Updates `SecretListItem` expectations to match the Vue I18n
date formatter.
- **What**: Adds `SecretListItem` stories for default, never-used,
loading, and disabled states.
- **Dependencies**: None.
## Review Focus
Stacked on #11480 to keep the escaping fix scoped. Please confirm
whether the localized medium date style matches design/product
expectations.
## Screenshots (if applicable)
https://f3ba7229.comfy-storybook.pages.dev/?path=/story/platform-secrets-secretlistitem--default
┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-11524-fix-localize-secret-date-labels-3496d73d3650814bb70bfb3f870a31cd)
by [Unito](https://www.unito.io)
|
||
|
Benjamin Lu
|
7ee667c1d1 |
fix: avoid escaped secret date labels (#11480)
Global i18n config has escapeParameter as true. This explicitly turns it
to false. I opened a Linear ticket to reconsider changing this back to
false as default globally.
## Summary
Fix the Secrets panel so created and last-used dates render as plain
text instead of HTML-escaped slash entities.
## Changes
- **What**: Compute the Secrets date labels with `t(..., {
escapeParameter: false })` after formatting the date, so vue-i18n does
not escape `/` into `/` for plain-text output.
- **What**: Replace the mocked translation setup in
`SecretListItem.test.ts` with a real `vue-i18n` instance and add a
regression test that asserts the rendered dates do not contain escaped
slash entities.
## Review Focus
This intentionally fixes the i18n interpolation issue shown in the bug
screenshot. It does not change the separate RFC3339Nano parsing behavior
discussed in #11358.
┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-11480-fix-avoid-escaped-secret-date-labels-3486d73d365081c890ecd2a6992d7879)
by [Unito](https://www.unito.io)
---------
Co-authored-by: GitHub Action <action@github.com>
|
||
|
Benjamin Lu
|
2b010ac8b3 |
fix: dedupe pending checkout attempt construction (#11622)
## Summary Deduplicates pending subscription checkout attempt construction so storage and fallback paths share the same payload creation. ## Changes - **What**: Build the `PendingSubscriptionCheckoutAttempt` once in `recordPendingSubscriptionCheckoutAttempt()` and reuse it for unavailable-storage, failed-write, and successful-write paths. - **Dependencies**: None. ## Review Focus This is intended as a no-behavior-change cleanup: unavailable storage still returns an attempt, failed `setItem()` still returns that attempt without dispatching, and the pending-checkout event only fires after a successful storage write. Linear: FE-209 ## Screenshots (if applicable) N/A --------- Co-authored-by: GitHub Action <action@github.com> |
||
|
jaeone94
|
b4d209b5f6 |
feat: refresh missing models through pipeline (#11661)
## Summary Follow-up to the closed earlier attempt in #11646. This PR keeps the same user-facing goal, but changes the implementation to reuse the existing missing model pipeline for refresh instead of maintaining a separate candidate-only recheck path. Adds a missing model refresh action in the Errors tab by reusing the existing missing model pipeline, so users can re-check models after downloading or manually placing files without reloading the workflow. ## Changes - **What**: - Adds `app.refreshMissingModels()` as a reusable refresh entry point for the current root graph. - Splits node definition reloading into `app.reloadNodeDefs()` so missing-model refresh can pull fresh `object_info` without showing the generic combo refresh success flow. - Reuses the existing missing model pipeline instead of adding a separate candidate-only checker. The refresh path serializes the current graph, reuses active workflow model metadata when available, falls back to current missing-model metadata, and then reruns the same candidate discovery/enrichment/surfacing flow used during workflow load. - Adds missing model refresh state and error handling to `missingModelStore`. - Adds a Refresh button next to Download all in the missing model card action bar. - Moves Download all from the Errors tab header into the missing model card, so the Download all and Refresh actions render or hide together. - Changes Download all visibility from “more than one downloadable model” to “at least one downloadable model.” - Keeps the action bar hidden when there are no downloadable missing models; Cloud still does not render this action area. - Normalizes active workflow `pendingWarnings` updates so resolved missing model warnings do not get revived by stale empty warning objects. - Adds test IDs and coverage for the new action bar, refresh state, refresh delegation, pending warning sync, and E2E refresh behavior. - **Breaking**: None. - **Dependencies**: None. ## Review Focus The main design choice is intentionally reusing the missing model pipeline for refresh instead of implementing a smaller candidate-only recheck. The earlier candidate-only approach was cheaper, but it created a separate source of truth for missing-model resolution and made edge cases harder to reason about. In particular, it could diverge from the behavior used when a workflow is loaded, and it did not naturally handle the case where a model becomes missing after the workflow is already open. This version pays the cost of refreshing node definitions and rerunning the missing-model scan for the current graph, but keeps the refresh behavior aligned with workflow load semantics. Expected behavior by environment: - OSS browser: - The action bar appears when at least one missing model has a downloadable URL and directory. - Download all uses the existing browser download path. - Refresh reloads `object_info`, refreshes node definitions/combo values, reruns missing-model detection for the current graph, and clears the error if the selected model is now available. - OSS desktop: - The same action bar appears under the same downloadable-model condition. - Download all uses the existing Electron DownloadManager path. - Refresh uses the same missing-model pipeline as browser, so manually placed files or desktop-downloaded files can be rechecked without reloading the workflow. - Cloud: - The action bar remains hidden because model download/import is not supported in this section for Cloud. A few boundaries are intentional: - This PR does not add automatic filesystem watching. Browser OSS cannot reliably observe local model folder changes, so the user-triggered Refresh button remains the cross-environment mechanism. - This PR does not redesign the public `refreshComboInNodes` API beyond extracting `reloadNodeDefs()` for reuse. Further cleanup of toast behavior or a more explicit object-info reload API can be follow-up work. - This PR keeps refresh scoped to missing-model validation; missing media and missing nodes continue to use their existing flows. Linear: FE-417 ## Screenshots (if applicable) https://github.com/user-attachments/assets/2e02799f-1374-4377-b7b3-172241517772 ## Validation - `pnpm format` - `pnpm lint` (passes; existing unrelated warning remains in `src/platform/workspace/composables/useWorkspaceBilling.test.ts`) - `pnpm typecheck` - `pnpm test:unit` - `pnpm test:browser:local -- --project=chromium browser_tests/tests/propertiesPanel/errorsTabMissingModels.spec.ts` - `pnpm build` - `NX_SKIP_NX_CACHE=true DISTRIBUTION=desktop USE_PROD_CONFIG=true NODE_OPTIONS='--max-old-space-size=8192' pnpm exec nx build` - Manual desktop verification through `~/Projects/desktop` after copying the desktop build into `assets/ComfyUI/web_custom_versions/desktop_app`: - confirmed the FE bundle is built with `DISTRIBUTION = "desktop"` - confirmed missing model Download uses the desktop download path instead of browser download - confirmed Refresh can clear the missing model error after the model is available - Push hook: `pnpm knip --cache` ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11661-feat-refresh-missing-models-through-pipeline-34f6d73d3650811488defee54a7a6667) by [Unito](https://www.unito.io) |
||
|
Christian Byrne
|
9ce4c18eb1 |
test: add unit tests for useGLSLRenderer (#11390)
## Summary Adds 44 unit tests for the `useGLSLRenderer` composable (`src/renderer/glsl/useGLSLRenderer.ts`), increasing coverage from ~15% to near-complete line coverage. ## Test Coverage | Describe Block | Tests | What's Covered | |---|---|---| | `init` | 6 | Context creation, FBO setup, `UNPACK_FLIP_Y_WEBGL`, FBO failure cleanup, post-dispose guard | | `compileFragment` | 9 | Shader compile success/failure, program link errors, program re-creation, dispose guard | | `setResolution` | 4 | Viewport + FBO recreation on resize, no-op on same size, dispose guard | | `setFloatUniform` | 2 | Float uniform dispatch, dispose guard | | `setIntUniform` | 2 | Int uniform dispatch, dispose guard | | `bindInputImage` | 4 | Texture creation/binding, out-of-range index, re-bind cleanup, dispose guard | | `render` | 7 | Draw call, `u_resolution`, fallback textures, final pass to default FBO, multi-pass ping-pong, dispose/no-program guards | | `readPixels` | 2 | Returns `ImageData`, throws when uninitialized | | `toBlob` | 2 | Returns `Blob`, throws when uninitialized | | `dispose` | 4 | GL resource cleanup, idempotency, `onScopeDispose`, input texture cleanup | | `config` | 2 | Default vs custom uniform/input counts | ## Approach - Mocks `WebGL2RenderingContext`, `OffscreenCanvas`, and `ImageData` (happy-dom has no WebGL) - Uses Vue `effectScope` to test `onScopeDispose` cleanup - Mocks `detectPassCount` from `glslUtils` for multi-pass tests ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11390-test-add-unit-tests-for-useGLSLRenderer-3476d73d3650815a8787cd9368fd8baf) by [Unito](https://www.unito.io) |
||
|
Christian Byrne
|
56aec1878a |
test: add unit tests for GPUBrushRenderer (#11388)
## Summary Add unit tests for `GPUBrushRenderer`, increasing coverage from ~3.5% to cover constructor initialization, stroke rendering, compositing, preview blitting, readback, and resource cleanup. ## Changes - **What**: 27 unit tests for `GPUBrushRenderer` covering all public methods with comprehensive WebGPU API mocks ## Review Focus Mock factory approach for WebGPU objects — all GPU globals (`GPUBufferUsage`, `GPUTextureUsage`, `GPUShaderStage`) are polyfilled since happy-dom lacks WebGPU support. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11388-test-add-unit-tests-for-GPUBrushRenderer-3476d73d3650814ab0e2c0fb8c424faa) by [Unito](https://www.unito.io) |
||
|
Dante
|
502a02213a |
test: add unit tests for useWorkspaceBilling polling and refresh paths (#11676)
## Summary Extends `useWorkspaceBilling.test.ts` with five behavioral gaps in the existing suite: `initialize` does not double-fetch balance when free tier already has positive balance, `subscribe(planSlug)` forwards `undefined` for return/cancel URLs, `previewSubscribe` does not refresh status or balance after success, `pollCancelStatus` falls back to a default error message when failed status omits `error_message`, and `pollCancelStatus` halts further scheduled polls when a later poll's API call rejects. ## Changes - **What**: Adds 5 Vitest cases across four new `describe` blocks (`initialize free-tier balance refresh`, `subscribe argument forwarding`, `previewSubscribe does not refresh state`, `pollCancelStatus error paths`). Reuses the existing `setupBilling()` factory and `effectScope` lifecycle. ## Review Focus - The mid-poll rejection test silences `unhandledRejection` for its duration: `pollCancelStatus`'s rescheduled poll runs through `void poll()` inside `setTimeout`, so the catch block's rethrow has no awaiter and surfaces as unhandled. The test pins the observable behavior (no further scheduled polls) without claiming `cancelSubscription` propagates the late error. - The `subscribe(planSlug)` test pins the call shape with explicit `undefined` arguments so a future signature change breaks the test rather than silently passing through. - The free-tier branch is targeted: previously only the zero-balance reload was tested; this adds the negative case (positive balance → no second fetch). ## Testing \`\`\`bash pnpm exec vitest run src/platform/workspace/composables/useWorkspaceBilling.test.ts pnpm format -- src/platform/workspace/composables/useWorkspaceBilling.test.ts pnpm lint pnpm typecheck pnpm knip \`\`\` 38 tests pass (33 prior + 5 new). ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11676-test-add-unit-tests-for-useWorkspaceBilling-polling-and-refresh-paths-34f6d73d36508197bc7bc66d54e805e0) by [Unito](https://www.unito.io) |
||
|
Dante
|
f1ea3b02a6 |
test: add unit tests for useNodeReplacement transfer edge cases (#11677)
## Summary Extends `useNodeReplacement.test.ts` with five connection-transfer and graph-mutation edge cases that the existing 23-case suite did not cover: missing-old-input-slot skip, missing-new-output-index resilience, set_value on a non-existent widget, set_value with dot-notation new_id, and the Vue-node refresh path via `nodeGraph.onNodeAdded`. ## Changes - **What**: Adds 5 Vitest cases in a new `transfer edge cases` describe block. Reuses the existing `createPlaceholderNode`, `createNewNode`, `createMockGraph`, `createMockLink`, and `makeMissingNodeType` helpers — no new test infrastructure introduced. ## Review Focus - The "missing new output index" test verifies that `replaceWithMapping` does not throw when `newNode.outputs[newOutputIdx]` is absent, and asserts the original link's `origin_id` is unchanged so the silent-skip behavior is pinned (not a swallowed exception). - The dot-notation `set_value` test pins that the existing dot-notation guard at `useNodeReplacement.ts:203` covers the `set_value` branch (not just the `old_id` connection branch already covered at line 187). - The `onNodeAdded` test asserts the Vue-node sync path that runs after `replaceWithMapping` bypasses `graph.add()` — a future refactor that drops the explicit call would silently break the Vue node renderer otherwise. ## Testing \`\`\`bash pnpm exec vitest run src/platform/nodeReplacement/useNodeReplacement.test.ts pnpm format -- src/platform/nodeReplacement/useNodeReplacement.test.ts pnpm lint pnpm typecheck pnpm knip \`\`\` 28 tests pass (23 prior + 5 new). ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11677-test-add-unit-tests-for-useNodeReplacement-transfer-edge-cases-34f6d73d3650817aa2ffccdb9fb4a947) by [Unito](https://www.unito.io) |
||
|
Christian Byrne
|
b2bba78ce0 |
test: add unit tests for usePanAndZoom composable (#11391)
## Summary Add 29 unit tests for the `usePanAndZoom` composable to improve mask editor test coverage. ## Changes - **What**: New test file `src/composables/maskeditor/usePanAndZoom.test.ts` covering all public API methods ## Review Focus Test coverage spans: - `initializeCanvasPanZoom` — landscape/portrait fit-to-view, panel width accounting, style application - `handlePanStart`/`handlePanMove` — panning state, offset updates, error on missing start - `zoom` — wheel in/out, clamping (0.2–10.0), missing canvas early return, cursor update - `updateCursorPosition` — pan offset application - `invalidatePanZoom` — missing image warning, store container fallback, rgbCanvas dimension sync - Touch handlers — single/two-finger start, double-tap undo, pen blocking, single-touch pan, pinch zoom, touch end states - `addPenPointerId`/`removePenPointerId` — add, dedup, remove, no-op ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11391-test-add-unit-tests-for-usePanAndZoom-composable-3476d73d36508104b447d87471ce021b) by [Unito](https://www.unito.io) --------- Co-authored-by: Terry Jia <terryjia88@gmail.com> Co-authored-by: GitHub Action <action@github.com> |
||
|
Dante
|
3d14bfb09c |
fix: render asset fixtures in AssetBrowserModal stories (#11502)
## Why this fix exists Surfaced as a dead-end during the FE-227 (asset modal scroll breakage in cloud-prod) root-cause investigation, **not** as a standalone Storybook complaint. The natural local repro path for FE-227 is "bump an `AssetBrowserModal` Storybook story to ~120 assets and watch the layout misbehave." When that path was attempted, the stories rendered empty modals regardless of fixture size. A Codex adversarial review confirmed the cause: three stories bind `:assets="..."` to a prop the component never declared, so the binding is silently dropped and the modal falls back to `assetsStore.getAssets(cacheKey)` — which returns an empty array in Storybook. The empty-modal failure mode also silently broke design QA / visual review on this surface: any reviewer opening these stories has been seeing "No assets found" for as long as the bug has existed. Filing this as its own PR so: - FE-227 stays focused on the cloud-prod scroll bug once a DevTools datapoint confirms the hypothesis. - The local repro path for FE-227 (and any future asset-modal layout regression) becomes usable. - Visual review on `AssetBrowserModal` is restored. ## What changed Three `AssetBrowserModal` stories bound `:assets="..."` to a non-existent prop, so the modal silently fell back to `assetsStore.getAssets(cacheKey)` — which returns an empty array in Storybook because the model cache only initializes in cloud distribution builds. Add an optional `assets` prop on `AssetBrowserModal` that, when provided, bypasses the store fetch. Production callers continue to use the store; this is a narrowly scoped Storybook/test seam. - Fixes FE-232 ### Why a prop on the component (Option 1) and not a Storybook decorator (Option 2) `assetsStore`'s model cache (`getModelState`) is gated by `if (isCloud)`, returning an empty stub for desktop/localhost distributions. Storybook's `.storybook/main.ts` does not define `__DISTRIBUTION__`, so `isCloud === false` and the store has no public API to seed assets. Public seed methods (`updateModelsForNodeType` / `updateModelsForTag`) only delegate to `assetService` network calls. Option 2 (decorator-based seeding) would require either patching Storybook's Vite `define` config or building a parallel mock store via `resolve.alias` (the `useJobList` precedent) — significantly more invasive than a +10 / -1 line component change. The new prop is documented as a Storybook/test seam in JSDoc and changes nothing for production callers. ### Bonus `useAssetBrowserDialog.stories.ts:120` had the **same** broken `:assets="mockAssets"` binding. The new prop transparently repairs it without a separate change. ## Before All three stories render an empty modal (`No assets found`) regardless of the fixture data they pass. > Drag-drop the screenshots into the slots below from `/tmp/fe-232-screenshots/`: > - `before-default.png` → Default story > - `before-single-asset-type.png` → Single asset type story > - `before-no-left-panel.png` → No left panel story | Story | Screenshot | |---|---| | Default | | <img width="961" height="821" alt="before-default" src="https://github.com/user-attachments/assets/4a0af0f5-b712-41e2-adbc-c2b4b921045d" /> | Single asset type | | <img width="961" height="821" alt="before-single-asset-type" src="https://github.com/user-attachments/assets/073a8fa8-7bbb-4ec9-a226-156b7141d9b5" /> | No left panel | <img width="961" height="821" alt="before-no-left-panel" src="https://github.com/user-attachments/assets/0d45ff3b-5866-4de9-b7aa-5bd9cb1f3566" /> | ## After All three stories now render their intended fixture data (asset cards visible with mock model names, badges, sort/filter controls populated). > Drag-drop the screenshots into the slots below from `/tmp/fe-232-screenshots/`: > - `after-default.png` → Default story > - `after-single-asset-type.png` → Single asset type story > - `after-no-left-panel.png` → No left panel story | Story | Screenshot | |---|---| <img width="961" height="821" alt="after-default" src="https://github.com/user-attachments/assets/a11b2475-bd18-4c30-aece-cf1bdbcc6ac5" <img width="961" height="821" alt="after-single-asset-type" src="https://github.com/user-attachments/assets/71e11237-006b-43d9-90de-e9d2d8894e34" /> /> | Default | | | Single asset type | | | No left panel | <img width="961" height="821" alt="after-no-left-panel" src="https://github.com/user-attachments/assets/5123db87-2ab9-4359-8e61-ac0d8da9494c" /> | ## Test plan - [x] Storybook stories now render fixture data (manually verified all three via Chrome DevTools MCP) - [x] `pnpm typecheck` passes on touched files - [x] `pnpm lint` passes on touched files - [x] Existing `AssetBrowserModal.test.ts` (14 tests) still passes - [x] `useAssetBrowserDialog.stories.ts` is also functional (same bug pattern, repaired by the new prop) - [ ] No new prop surface added to `AssetBrowserModal` other than the documented Storybook/test seam (`assets?: AssetItem[]`) |
||
|
Dante
|
3340b77908 |
test: add unit tests for value-control widget family (#11440)
## Summary Adds 42 unit tests across 5 files covering the value-control widget family — first batch of a broader effort to raise widget-test coverage. ## Changes - **What**: - `WidgetInputNumber.test.ts` (9) — variant selection by widget.type (int/float/slider/gradientslider), controlWidget wrapping in WidgetWithControl, modelValue forwarding. - `WidgetInputNumberGradientSlider.test.ts` (11) — initial value, min/max/disabled pass-through, default vs custom gradient stops, precision-derived step, WidgetLayoutField wrapping. - `WidgetWithControl.test.ts` (5) — renders passed component with widget/modelValue, initializes ValueControlButton mode from widget.controlWidget.value, calls controlWidget.update on mode change. - `ValueControlButton.test.ts` (11) — i18n aria-label per mode, text vs icon rendering, pointer and keyboard activation, `type="button"` safety. - `ValueControlPopover.test.ts` (6) — BEFORE/AFTER copy from settingStore, four option render, v-model updates on selection. ## Review Focus - Stack follows the existing widget-test pattern (`@testing-library/vue` + PrimeVue + `createI18n` where needed, no `@vue/test-utils`). - `createMockWidget` from `widgetTestUtils.ts` reused; no new helper extracted (YAGNI — per-file `renderComponent` stays ~10 lines). - `WidgetWithControl` watcher test asserts first-arg of `update` since the vue watch callback passes `(newVal, oldVal, onCleanup)`. - No changes to any widget component source — tests-only PR. This is one of several focused PRs in a widget-test-coverage sequence; subsequent PRs cover form-dropdown internals, utility widgets, media/graph/canvas widgets, and e2e value-type specs. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11440-test-add-unit-tests-for-value-control-widget-family-3486d73d3650813891e1fe8d45eaecaf) by [Unito](https://www.unito.io) --------- Co-authored-by: GitHub Action <action@github.com> Co-authored-by: bymyself <cbyrne@comfy.org> |
||
|
Dante
|
0ad85087ea |
test: add unit tests for useMissingModelInteractions (#11675)
## Summary Extends `useMissingModelInteractions.test.ts` with behavioral coverage for the previously untested public surface: `getComboOptions` (both asset-supported and widget-driven branches), `getDownloadStatus`, and the four `handleImport` outcomes (async-pending, async-completed, sync-with-mismatch, error). ## Changes - **What**: Adds 10 Vitest cases across three new `describe` blocks (`getComboOptions`, `getDownloadStatus`, `handleImport`). Extends the existing module-level mocks with `mockUploadAssetAsync`, `mockTrackDownload`, and `mockInvalidateModelsForCategory` so the import flow can be verified at its boundaries. ## Review Focus - The `handleImport` block uses a shared `setupImportableState(key)` helper to seed `urlInputs` + `urlMetadata` and stub `validateSourceUrl` once per test. Each case then asserts a single boundary effect (taskId tracked, cache invalidated, mismatch recorded, error stored). - The `getDownloadStatus` happy path relies on the existing getter-style `mockDownloadList` so the test's mutation lands in the composable without re-stubbing the asset download store. - The `getComboOptions` "asset-supported" test asserts both the call shape (`mockGetAssets` invoked with the candidate's `nodeType`) and the output shape, so a future refactor that swaps the lookup key fails loudly. ## Testing \`\`\`bash pnpm exec vitest run src/platform/missingModel/composables/useMissingModelInteractions.test.ts pnpm format -- src/platform/missingModel/composables/useMissingModelInteractions.test.ts pnpm lint pnpm typecheck pnpm knip \`\`\` 44 tests pass (34 prior + 10 new). ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11675-test-add-unit-tests-for-useMissingModelInteractions-34f6d73d36508112909fe8e49cc68010) by [Unito](https://www.unito.io) |
||
|
pythongosssss
|
4c892341e4 |
feat: Node search UX updates (#9714)
## Summary Addresses feedback from the initial v2 node search implementation for improved UI and UX ## Changes - **What**: - add root filter buttons - remove all extra tree categories leaving only "Most relevant" - replace input/output selection with popover - replace price badge with one from node header - add chevrons and additional styling to category tree - hide empty categories - fix bug with hovering selecting item under mouse automatically - fix tailwind merge with custom sizes removing them - keyboard navigation - general tidy/refactor/test ## Screenshots (if applicable) https://github.com/user-attachments/assets/db798dfa-e248-4b48-bb56-2fa7b6c5f65f ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-9714-feat-Node-search-UX-updates-31f6d73d365081cebd96c4253ad1ca53) by [Unito](https://www.unito.io) --------- Co-authored-by: GitHub Action <action@github.com> |
||
|
Dante
|
c594e30b84 |
test: harden useKeyboard test setup with vi.hoisted and try/finally (#11659)
## Summary - Move `mockCanvasHistory` / `mockStore` into `vi.hoisted()` so the mock state is hoisted before module imports, matching the pattern in `useCanvasTransform.test.ts`. - Wrap the temporary `document.activeElement` override in `try/finally` so the property is restored even if the assertion throws, preventing state leak into subsequent tests. - Fixes #11658 ## Test plan - [x] `pnpm test:unit src/composables/maskeditor/useKeyboard.test.ts` — 17/17 pass - [x] `pnpm typecheck` - [x] `pnpm lint` (no new warnings) ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11659-test-harden-useKeyboard-test-setup-with-vi-hoisted-and-try-finally-34f6d73d36508139be2ddc3095ea6952) by [Unito](https://www.unito.io) |
||
|
Dante
|
2ade779a81 |
fix: use getAssetFilename in asset browser to avoid showing hashes (#11492)
## Root cause
Three surfaces rendered `asset.name` directly even though in Cloud
production `asset.name` often equals `asset.asset_hash`:
1. **Asset browser modal** (`AssetCard.vue`) used `getAssetDisplayName`,
which — when `user_metadata.name` also held the hash — fell through to
the raw hash.
2. **Load Image node widget dropdown** (`useWidgetSelectItems.ts`)
rendered output items as `${asset.name} [output]`.
3. Queue-mapped output assets (`mapTaskOutputToAssetItem`) populate the
human-readable filename only on `asset.display_name`, not on
`user_metadata.filename` / `metadata.filename`. So surfaces that rely
only on `getAssetFilename` still fall through to `asset.name` (the
hash).
Filename / title resolution is now split into three helpers with
distinct responsibilities:
- `getAssetFilename` (unchanged) — canonical filename for serialization
/ identifier use (workflow widget values, schema validation,
missing-model matching). Never substitutes a display-only string.
- `getAssetDisplayFilename` (new) — filename-first label for surfaces
that render a filename. Adds `asset.display_name` as a fallback before
`asset.name`. Used by the Load Image output dropdown.
- `getAssetCardTitle` (new) — card title / delete-dialog label. Prefers
`user_metadata.name` / `metadata.name` when distinct from `asset.name`
(preserves user-renamed model titles from `ModelInfoPanel`), and falls
through to `getAssetDisplayFilename` for the Cloud hash case.
The dropdown item's `name` field (workflow payload value) is still
`${asset.name} [output]`, so Cloud can continue to resolve the asset by
hash.
Fixes FE-228
Source:
https://comfy-organization.slack.com/archives/C0A4XMHANP3/p1776716352588229
## Red / green verification
| Step | SHA | Purpose |
| --- | --- | --- |
| Red (card — metadata.filename) | `20b32e4f0` | `AssetCard.test.ts`
asserts the rendered title is the human-readable filename when
`asset.name === asset_hash`. |
| Green (card) | `ea889b34c` | `AssetCard.vue` switches from
`getAssetDisplayName` to `getAssetFilename`. |
| Red (widget — metadata.filename) | `318feddec` | Asserts the output
dropdown `label` uses `metadata.filename` when `asset.name` is a hash. |
| Green (widget) | `7b19bde15` | `useWidgetSelectItems.ts` derives
`label` from `getAssetFilename`; `name` (serialized) stays
`\${asset.name} [output]`. |
| Red (widget — display_name path) | `b19716e60` | Failing test for the
queue-mapped shape (`display_name` populated, `user_metadata.filename`
absent). |
| Green (util, reverted) | `533e60d6a` | Initial attempt broadened
`getAssetFilename` to include `display_name`; altered filename semantics
for model-asset consumers. |
| Refactor (scope narrowing) | `7c1085f30` | Reverts the util change;
applies `display_name` fallback locally in the output dropdown only. |
| Red (card — display_name path) | `38a9d4828` | Failing test: AssetCard
must fall back to `display_name` when filename metadata is absent. |
| Green (helper split) | `4ca0f620f` | Introduces
`getAssetDisplayFilename`; swaps AssetCard + widget dropdown to use it.
Adds helper unit tests. |
| Red (card — preserves curated name) | `dc2e9231d` | Failing test: a
user-curated `user_metadata.name` distinct from `asset.name` must win
over the filename; plus non-regression guard that
curated-name-equal-to-hash still falls back to filename. |
| Green (card title helper) | `5decf3a2b` | Adds `getAssetCardTitle`
(curated name when distinct, else `getAssetDisplayFilename`). AssetCard
title + delete-dialog swap to it; widget dropdown stays on
`getAssetDisplayFilename`. |
Side-effect audit (`getAssetFilename` still canonical):
- `createModelNodeFromAsset.ts`, `createAssetWidget.ts`,
`missingModelScan.ts`, `useComboWidget.ts`, `useWidgetSelectItems.ts`
asset-mode `name` — all still resolve through `getAssetFilename`, so
model-asset widget serialization and missing-model matching are
unaffected.
## Screenshots
### As is — asset browser card
<img width="1269" height="899" alt="Screenshot 2026-04-21 at 10 49 41
AM"
src="https://github.com/user-attachments/assets/7cffb585-4e64-4037-8bb1-5dd40215597e"
/>
### To be
<img width="1145" height="533" alt="Screenshot 2026-04-25 at 7 25 17 PM"
src="https://github.com/user-attachments/assets/8f12388a-16df-4892-83b4-c8d1f033f190"
/>
_Verified locally via `pnpm dev:cloud`: asset browser cards preserve
user-curated display names, Cloud-hash cases render the filename, and
the Load Image output dropdown also renders the human-readable filename.
The selected value still serializes the hash path._
## Test plan
- [ ] \`pnpm test:unit -- AssetCard.test\` passes (4 cases:
metadata.filename, display_name fallback, curated-name preservation,
curated-name==hash fallback)
- [ ] \`pnpm test:unit -- assetMetadataUtils.test\` passes (53 cases
incl. new helper coverage)
- [ ] \`pnpm test:unit -- useWidgetSelectItems.test\` passes (26 cases)
- [ ] \`pnpm test:unit -- useMissingModelInteractions.test\` passes
(guards against model-asset regressions)
- [ ] Asset browser modal: user-renamed model shows curated name; Cloud
hash outputs show filename
- [ ] Load Image node → widget dropdown → Outputs tab shows original
filenames
- [ ] Delete-confirm dialog references the same curated name as the card
title
- [ ] Model-asset widgets (Checkpoint / LoRA / etc.) still serialize the
model filename
- [ ] Submitting a workflow that references a Cloud output still
executes
┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-11492-fix-use-getAssetFilename-in-asset-browser-to-avoid-showing-hashes-3496d73d36508148b8a3fb0482fa668e)
by [Unito](https://www.unito.io)
---------
Co-authored-by: GitHub Action <action@github.com>
|
||
|
Terry Jia
|
59ef69f355 |
test: add unit tests for useCoordinateTransform mask editor composable (#11640)
## Summary
Add unit tests for `useCoordinateTransform` mask editor composable,
raising coverage from 2.43% to 100% (statements / branches / functions /
lines).
## Changes
- **What**: Add
`src/composables/maskeditor/useCoordinateTransform.test.ts` (14 tests)
covering both `screenToCanvas` and `canvasToScreen`: identity (display
matches bitmap), uniform downscale (bitmap larger than display),
`pointerZone`-vs-`canvasContainer` offset, non-uniform per-axis scaling,
screen↔canvas round-trip, and the three "element missing" branches
(`pointerZone` / `canvasContainer` / `maskCanvas` null) that should warn
and return `{x:0,y:0}`.
## Review Focus
- Mocked `createSharedComposable` to a pass-through so each test gets a
fresh transform reading the latest `mockStore` refs (otherwise the
shared instance captures stale element references between tests).
- DOM rects are stubbed via `vi.spyOn(el, 'getBoundingClientRect')`
rather than constructing fake DOMRects, so `unref(...)` in the
composable still receives a real `HTMLElement` / `HTMLCanvasElement`.
- Round-trip test (`screenToCanvas` → `canvasToScreen`) verifies the two
functions are mathematical inverses under the offset + scale
combination, which is the actual invariant the rest of the editor relies
on.
- Style aligned with sibling tests: `should ...` naming, `describe`
grouped by public method, explicit `MockStore` type alias, helper
factories `createElementWithRect` / `createCanvasWithRect`.
┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-11640-test-add-unit-tests-for-useCoordinateTransform-mask-editor-composable-34e6d73d3650814d95bdef66e36328e8)
by [Unito](https://www.unito.io)
|
||
|
Terry Jia
|
9ad052467d |
test: add unit tests for useKeyboard mask editor composable (#11639)
## Summary
Add unit tests for `useKeyboard` mask editor composable, raising
coverage from 0% to 100% (statements/lines/functions, 95.65% branch).
## Changes
- **What**: Add `src/composables/maskeditor/useKeyboard.test.ts` (17
tests) covering key tracking (`isKeyDown`), space-key
blur/preventDefault, undo/redo shortcuts (Ctrl/Meta+Z, Ctrl+Shift+Z,
Ctrl+Y), modifier-key edge cases (Alt suppression, no-modifier no-op,
Ctrl+Shift+Y ignored), `window blur` clearing keys, and listener
teardown via `removeListeners`.
## Review Focus
- Mock surface is intentionally minimal — only `useMaskEditorStore` is
mocked because the composable only reaches
`store.canvasHistory.{undo,redo}`.
- `afterEach(keyboard.removeListeners)` is required: the composable
attaches listeners to `document` / `window`, so without teardown earlier
test instances leak handlers and inflate mock call counts in later
tests.
- Tests dispatch real `KeyboardEvent`s via `document.dispatchEvent`
rather than calling the internal handlers directly, so they exercise the
actual `addEventListener` wiring.
- Test style aligned with existing mask editor tests: `should ...`
naming, `describe` grouped by public method, explicit `MockStore` /
`MockCanvasHistory` type aliases.
┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-11639-test-add-unit-tests-for-useKeyboard-mask-editor-composable-34e6d73d36508129b437d0270d9424d8)
by [Unito](https://www.unito.io)
|
||
|
Dante
|
aa730c8cb5 |
test(assets): add unit tests and E2E fixture groundwork for sidebar filter & sort (#11632)
## Summary Lays the test foundation for the two open assets-testing issues — Phase 1 (fixtures + page object) and Phase 2 (unit tests). Phase 3/4 (the actual E2E specs) follow in stacked PRs. ## Changes - **What**: 27 new unit tests covering `useMediaAssetFiltering`, `MediaAssetFilterMenu`, and `MediaAssetSettingsMenu`; reusable Playwright fixture factories for diverse media kinds and execution-time specs; new locators + helpers on `AssetsSidebarTab` for the filter menu and longest/fastest sort options. No production code touched. - **Breaking**: none ### New files - `src/platform/assets/composables/useMediaAssetFiltering.test.ts` — 11 cases: single/multi-OR media-type filter, `'3D'` → `'3d'` filename normalization, exclusion of unsupported kinds, all four sort modes, `created_at` fallback when `user_metadata.create_time` is absent, filter+sort composition. - `src/platform/assets/components/MediaAssetFilterMenu.test.ts` — 6 cases: checkbox rendering, prop-driven `aria-checked`, click toggling (add/remove/append), keyboard activation (Enter/Space). - `src/platform/assets/components/MediaAssetSettingsMenu.test.ts` — 10 cases: view-mode v-model, `showSortOptions` and `showGenerationTimeSort` visibility gates, `v-model:sortBy` round-trip for newest/oldest/longest/fastest. ### Extended files - `browser_tests/fixtures/helpers/AssetsHelper.ts` — added `MediaKindFixture` type, optional `mediaKind` shorthand on `createMockJob` (sets both filename extension and `preview_output.mediaType`), plus `createMixedMediaJobs(kinds)` and `createJobsWithExecutionTimes(specs)` factories for unambiguous filter/sort assertions. - `browser_tests/fixtures/components/SidebarTab.ts` — added `filterButton`, per-type checkbox locators (`filterImage/Video/Audio/3DCheckbox`), `sortLongestFirst`, `sortFastestFirst`, plus `openFilterMenu()`, `filterCheckbox(kind)`, `toggleMediaTypeFilter(kind)`, and `getAssetCardOrder()` helpers. ## Review Focus - **Naming of `MediaKindFixture` values** — `'images'` is plural to match existing API conventions emitted by the backend / `useMediaAssetGalleryStore`; `'video' | 'audio' | '3D'` follow the singular `MediaKind` type. Open to renaming if a unified shape is preferred. - **Filter button locator strategy** — `MediaAssetFilterButton` has no `aria-label`, so the page object targets it via the `icon-[lucide--list-filter]` class. Happy to add a `data-testid` or `aria-label` to the source component if reviewers prefer a more durable hook (would be a one-line source change in a follow-up). ## Follow-up PRs - Phase 3 (E2E for media-type filter) → closes #10780 - Phase 4 (E2E for asset sort) → closes #10779 Both are stacked on this branch and can be reviewed/merged in either order once this lands. References #10779, #10780. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11632-test-assets-add-unit-tests-and-E2E-fixture-groundwork-for-sidebar-filter-sort-34e6d73d3650815c9900e5fd7cc7eab0) by [Unito](https://www.unito.io) |
||
|
Terry Jia
|
cc1fe65348 |
test: add unit tests for maskEditorDataStore (#11641)
## Summary
Add unit tests for `maskEditorDataStore` Pinia store, raising coverage
from 0% to 100% (statements / branches / functions / lines).
## Changes
- **What**: Add `src/stores/maskEditorDataStore.test.ts` (13 tests)
covering initial state, the three `computed` predicates
(`hasValidInput`, `hasValidOutput`, `isReady` including the `isLoading`
interaction), the `setLoading(loading, error?)` action across its three
branches (no error arg, truthy error arg, empty-string error arg — empty
string is falsy so it must NOT clobber an existing `loadError`), and
`reset()` clearing every field including derived predicates.
## Review Focus
- Uses `createTestingPinia({ stubActions: false })` so action
implementations actually run, matching the pattern used by other store
tests in `src/stores/` (e.g. `dialogStore.test.ts`).
- The `setLoading('', '')` test guards a real branch in the source — `if
(error)` skips assignment for empty strings, so callers can't
accidentally clear a previous error by passing `''`. Worth keeping if
anyone tightens the guard later.
- `reset()` test asserts both raw refs and the three computed values
flip back to `false` / `null`, so a future regression in either
direction is caught.
- Style aligned with sibling tests: `should ...` naming, `describe`
grouped by exposed property/action, `createImage` / `createCanvas` /
`createOutputData` helpers to keep arrange blocks short.
┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-11641-test-add-unit-tests-for-maskEditorDataStore-34e6d73d36508121b8e5d185178310ab)
by [Unito](https://www.unito.io)
---------
Co-authored-by: GitHub Action <action@github.com>
|
||
|
Terry Jia
|
0f66f76b87 |
test: add unit tests for mask editor control components (#11642)
## Summary
Add unit tests for the three mask editor control components
(`DropdownControl`, `SliderControl`, `ToggleControl`), raising each from
partial (20% / 37.5% / 44.4%) to 100% across all coverage dimensions.
## Changes
- **What**: Add three sibling test files under
`src/components/maskeditor/controls/`:
- `DropdownControl.test.ts` (5 tests): label render, normalization of
`string[]` options into `{label,value}`, pass-through of `{label,value}`
options, `modelValue` reflected as the selected option,
`update:modelValue` emitted on change.
- `SliderControl.test.ts` (4 tests): label render,
`min`/`max`/`step`/`modelValue` exposed on `<input type="range">`,
`step` defaults to `1` when omitted, `update:modelValue` emitted as a
`number` (not string) on input.
- `ToggleControl.test.ts` (5 tests): label render, `modelValue`
reflected as `checked`, `update:modelValue` emitted as `true` / `false`
on toggle.
## Review Focus
- Stack matches the project's prevailing Vue test pattern
(`@testing-library/vue` + `@testing-library/user-event`, e.g.
`Badge.test.ts`, `BatchCountEdit.test.ts`).
- `update:modelValue` is asserted via the `'onUpdate:modelValue'`
callback prop rather than `emitted()` — keeps tests focused on
observable behavior and avoids reaching into the wrapper.
- `SliderControl` uses `fireEvent.input` instead of
`userEvent.type/clear`: `<input type="range">` is non-editable for
`userEvent` (`clear() is only supported on editable elements`). The
single eslint-disable for `testing-library/prefer-user-event` is
annotated with the reason.
- `DropdownControl` test guards both branches of the `string` →
`{label,value}` normalization (string array path and pre-normalized
object array path), since that's the only meaningful logic in the file.
- Style aligned with sibling tests: `should ...` naming, per-file
`renderComponent` helper accepting a `props` override and an `onUpdate`
callback parameter.
┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-11642-test-add-unit-tests-for-mask-editor-control-components-34e6d73d3650812aba2ae34a760489e2)
by [Unito](https://www.unito.io)
|
||
|
Terry Jia
|
bc16865019 |
test: add unit tests for useToolManager mask editor composable (#11643)
## Summary
Add unit tests for `useToolManager` mask editor composable, raising
coverage from 0% to 100% (statements / functions / lines, 97.53%
branch).
## Changes
- **What**: Add `src/composables/maskeditor/useToolManager.test.ts` (35
tests) covering:
- `switchTool`: store update, layer auto-switch via
`newActiveLayerOnSet`, custom-cursor branch, no-cursor (default
`'none'`) branch, missing-`pointerZone` no-throw guard.
- `setActiveLayer`: rgb-while-mask-only-tool → swap to `PaintPen`,
mask-while-`PaintPen` → swap to `MaskPen`, no-swap path.
- `updateCursor`: same custom-cursor / default-cursor split plus
`brushPreviewGradientVisible = false` post-condition.
- `currentTool` watcher: clears `lastColorSelectPoint` only when leaving
`MaskColorFill`.
- `handlePointerDown`: touch-ignore, pen pointer registration,
middle-button pan, space+left pan, `MaskPen`/`PaintPen` left-button
drawing, `PaintPen` continue-drawing branch (`button !== 0 && buttons
=== 1`), `MaskBucket` flood fill (with coord transform),
`MaskColorFill`, alt+right brush adjustment, right-click drawing for
drawing tools, no-op for non-drawing tools.
- `handlePointerMove`: touch-ignore, cursor position update,
middle-button pan, space+left pan, non-drawing-tool ignore, alt+right
brush adjustment while `isAdjustingBrush`, left/right drag drawing.
- `handlePointerUp`: state cleanup (`isPanning` / `brushVisible` /
`isAdjustingBrush`), pen pointer removal, touch-pointer early bail
before `drawEnd`.
## Review Focus
- Mock store is wrapped in `reactive()` so the `watch(() =>
store.currentTool, ...)` actually fires when tests mutate `currentTool`.
Plain object mocks would silently no-op the watcher branch.
- Each `setup()` runs `useToolManager` inside its own `effectScope`,
stopped in `afterEach`. Without scoping, watchers from previous tests
stay attached to the shared reactive store and accumulate (a single
mutation in test N would call `clearLastColorSelectPoint` N times).
- Mocked `app.extensionManager.setting.get` because `useBrushDrawing`
factory reads two settings synchronously at construction time. The mock
returns deterministic defaults so we don't need `useSettingStore`
plumbing.
- Pointer-event factory builds the minimal shape (`button` / `buttons` /
`pointerType` / `offset*` / `client*` / `altKey` / `pointerId`) — no
jsdom `PointerEvent` constructor noise. `preventDefault` is a `vi.fn()`
because the source calls it unconditionally.
- Style aligned with sibling tests: `should ...` naming, `describe`
grouped by exposed function/watcher, typed `MockStore`, helper
`pointerEvent({ ... })` and `setup()`.
┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-11643-test-add-unit-tests-for-useToolManager-mask-editor-composable-34e6d73d36508184b017ebd04626b29d)
by [Unito](https://www.unito.io)
|
||
|
Terry Jia
|
206a367379 |
test: add unit tests for useMaskEditor composable (#11644)
## Summary
Add unit tests for `useMaskEditor` composable, raising coverage from 0%
to 100% (statements / branches / functions / lines).
## Changes
- **What**: Add `src/composables/maskeditor/useMaskEditor.test.ts` (7
tests) covering `openMaskEditor`:
- Happy path: dialog opened once, `node` forwarded as a prop, header /
content components attached.
- Modal dialog config (`modal` / `maximizable` / `closable` flags)
forwarded to PrimeVue dialog props.
- Acceptance path for nodes with no `imgs` but `previewMediaType ===
'image'`.
- Three guard paths that should log and bail: `node` is null, node with
empty `imgs` and no image preview, node with empty `imgs` and a
non-image preview type (e.g. `'video'`).
## Review Focus
- Mocked `useDialogStore` with a single shared `showDialog` spy — the
only contract under test is "we forwarded these props to the store
action", so instantiating Pinia would just add noise.
- `TopBarHeader.vue` and `MaskEditorContent.vue` are stubbed because
they pull in the full mask-editor render tree; we only assert they're
forwarded as `headerComponent` / `component`, not what they render.
- `console.error` is spied per-test so the bail messages are observable
but don't pollute runner output.
- `nodeWithImage` factory uses a structural `NodeShape` (`{ imgs?,
previewMediaType? }`) rather than `Partial<LGraphNode>` because the real
`LGraphNode` type requires a `LGraphNodeConstructor`-shaped
`constructor` field, which would force every test to construct a full
graph node — irrelevant to the contract being tested.
- Style aligned with sibling tests: `should ...` naming, `describe`
grouped by exposed function (`openMaskEditor`), helper factory.
┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-11644-test-add-unit-tests-for-useMaskEditor-composable-34e6d73d365081e98336db0a92c37ccf)
by [Unito](https://www.unito.io)
|
||
|
Terry Jia
|
7e8ede376b |
test: add unit tests for maskEditorStore (#11645)
## Summary
Add unit tests for `maskEditorStore` Pinia store, raising coverage from
0% to 100% (statements / branches / functions / lines).
## Changes
- **What**: Add `src/stores/maskEditorStore.test.ts` (30 tests)
covering:
- Brush setters: `setBrushSize` (1–250), `setBrushOpacity` (0–1),
`setBrushHardness` (0–1), `setBrushStepSize` (1–100) — each tested at
lower bound, upper bound, and in-range.
- `resetBrushToDefault`: documents the exact default brush shape.
- Other clamped setters: `setPaintBucketTolerance` /
`setColorSelectTolerance` / `setMaskTolerance` (0–255), `setFillOpacity`
/ `setSelectionOpacity` (0–100), `setMaskOpacity` (0–1), `setZoomRatio`
(0.1–10).
- `setPanOffset` / `setCursorPoint`: copy-by-value semantics — mutating
the input after the call must not leak into store state.
- `resetZoom` / `triggerClear`: monotonic counter bumps.
- `maskColor` computed: `Black`, `White`, `Negative` blend modes plus
the `default:` fallback for unknown values.
- `canUndo` / `canRedo` proxy through to mocked `useCanvasHistory`.
- Canvas → ctx watchers: setting `maskCanvas` / `rgbCanvas` /
`imgCanvas` derives the corresponding `*Ctx` via `getContext('2d', {
willReadFrequently: true })`. Clearing the canvas leaves the previous
ctx in place (parametrized via `it.each` for all three).
- `resetState`: restores all non-DOM state to documented defaults;
explicitly verifies DOM refs (`maskCanvas` / `pointerZone` / `image`)
are NOT cleared so the editor can reuse mounted elements after a reset.
## Review Focus
- `useCanvasHistory` is mocked via `vi.hoisted` so each test gets the
same exposed `canUndo` / `canRedo` refs while the store's internal
`canvasHistory` reference is untouched. Without this, the store would
call into the real history with `null` canvas refs.
- `setPanOffset` / `setCursorPoint` tests mutate the input *after* the
call — that's the actual behavioral contract (defensive copy via
spread), not a default-value check.
- `resetState` test sets *every* field to a non-default before calling,
so the test fails if `resetState` ever forgets to reset a field. Final
assertions are positive (matches default), not weak negative checks.
- The "DOM refs preserved on reset" assertion is the
surprising-on-purpose part: a future refactor that adds
`maskCanvas.value = null` to `resetState` would break the editor's
ability to reuse mounted canvases after clearing internal state.
- `it.each` for the three canvas/ctx pairs covers the watcher's null
branch without three near-duplicate tests.
- `makeCanvas` overrides `canvas.getContext` directly rather than using
`vi.spyOn` because `HTMLCanvasElement.getContext` has overloads (2d /
webgl / webgpu / bitmaprenderer) and TypeScript picks the GPU overload
by default for spy return type inference.
- Style aligned with sibling `maskEditorDataStore.test.ts`:
`createTestingPinia({ stubActions: false })`, `should ...` naming,
`describe` grouped by exposed property/action, no default-only
change-detector tests.
┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-11645-test-add-unit-tests-for-maskEditorStore-34e6d73d3650818e9855cd9f9f13e62a)
by [Unito](https://www.unito.io)
|
||
|
Terry Jia
|
7bfbd0d7f3 |
test: add unit tests for mask editor settings panels (#11647)
## Summary
Add unit tests for the five mask editor settings panel components,
raising each from 0–35% to ~98–100% across coverage dimensions.
## Changes
- **What**: Add 5 sibling test files under `src/components/maskeditor/`
(47 tests total):
- `PaintBucketSettingsPanel.test.ts` (4): both sliders bind to / write
through to `setPaintBucketTolerance` / `setFillOpacity`. **100%**.
- `ColorSelectSettingsPanel.test.ts` (8): all 7 controls (4 sliders, 2
toggles, 1 dropdown) bind to and write through to the right store
fields/setters; method dropdown casts to `ColorComparisonMethod`.
**100%**.
- `BrushSettingsPanel.test.ts` (13): shape buttons (Arc/Rect), reset to
default, four numeric inputs (size/opacity/hardness/step), logarithmic
size slider including the cached-raw-value path
(`Math.round(Math.pow(250, x))`), color input v-model, color input ref
forwarded to / cleared from store on mount/unmount.
- `ImageLayerSettingsPanel.test.ts` (17): mask opacity slider + canvas
style sync, blend-mode select with all 3 enum values + default fallback,
three layer-visibility checkboxes (with and without canvas refs),
activate-layer button forwarding to `toolManager.setActiveLayer`,
disabled-when-active and "show paint button only when Eraser" branches,
base image preview src binding.
- `SettingsPanelContainer.test.ts` (5): tool → component routing
(`MaskBucket` → bucket panel, `MaskColorFill` → color panel, anything
else → brush panel).
## Review Focus
- Stack matches sibling control tests (`@testing-library/vue` +
`userEvent` + stub child controls). Each panel's child `SliderControl` /
`ToggleControl` / `DropdownControl` is replaced by a minimal `<button>`
stub that emits `update:modelValue` on click, so panel logic is
decoupled from control internals (already covered by their own tests).
- Two panels (`Brush`, `ImageLayer`) need real reactivity
(`brushSettings.size` changes through a setter must propagate to the
slider's modelValue computed; `currentTool` / `activeLayer` mutations
between tests). They use `reactive()` from Vue at top level + a `let
mockStore` reset in `beforeEach`. Plain object mocks would either
silently no-op or leak state between tests.
- The two reactive panels enable file-level `eslint-disable
testing-library/no-container, testing-library/no-node-access` because
the unlabeled DOM (shape divs, unlabeled `<input type="number">`/`<input
type="color">`/`<input type="checkbox">`/`<select>`) genuinely can't be
queried via `screen.getByRole/Label`. Each disable has a comment
explaining why.
- `BrushSettingsPanel` has a non-trivial cached-value branch in
`brushSizeSliderValue` computed: when `rawSliderValue` and `brushSize`
are in sync, the getter returns the raw float instead of recomputing
`log(size)/log(250)`. Test stubs `setBrushSize` to actually write back
so the next read takes the cached path.
- `ImageLayerSettingsPanel` has a `display: 'block' | 'none'` style
binding. happy-dom doesn't populate `el.style.display` from inline style
strings reliably, so the test asserts via `el.getAttribute('style')`
instead.
- `SettingsPanelContainer` uses inline component stubs that render
unique text — assertion is just `textContent.toContain('brush-panel')`
etc. No need for component-instance probing.
- Style aligned with sibling tests: `should ...` naming, `describe`
grouped by feature (button group, slider, etc.), `vi.hoisted` for mock
state where reactivity isn't required.
┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-11647-test-add-unit-tests-for-mask-editor-settings-panels-34e6d73d36508117911bc0850ce085e1)
by [Unito](https://www.unito.io)
|
||
|
Terry Jia
|
32b266f3e9 |
test: add unit tests for SidePanel, MaskEditorButton, and BrushCursor (#11648)
## Summary
Add unit tests for three small mask editor Vue components (`SidePanel`,
`MaskEditorButton`, `BrushCursor`), all reaching **100%** across
coverage dimensions.
## Changes
- **What**: Add 3 sibling test files (17 tests total):
- `SidePanel.test.ts` (3): renders both `SettingsPanelContainer` and
`ImageLayerSettingsPanel`; forwards `toolManager` prop through to
`ImageLayerSettingsPanel`; works with the prop omitted.
- `MaskEditorButton.test.ts` (3): renders with localized `aria-label`
when `isSingleImageNode` is true; hidden via `v-show` (style `display:
none`) when false; click executes `Comfy.MaskEditor.OpenMaskEditor`
command.
- `BrushCursor.test.ts` (11): opacity 1 / 0 from `brushVisible`; size =
2 × effective brush size × zoom (with hardness scaling); border-radius
50% / 0% for Arc / Rect; position from `cursorPoint + panOffset -
radius`, with optional `containerRef.getBoundingClientRect` offset;
gradient preview hidden / shown; flat fill at `effectiveHardness === 1`.
## Review Focus
- `SidePanel` test stubs both child panels to bare `<div data-testid>`
so the test verifies wiring (prop forwarding, child rendering) without
being affected by the children's i18n / store dependencies.
- `MaskEditorButton` mocks `useCommandStore.execute` and
`useSelectionState.isSingleImageNode` (as a Vue ref). Real i18n via
`createI18n` + `globalInjection: true` — the template uses `$t(...)`
which requires global injection in composition mode.
- For the v-show hidden case, `getByLabelText('...', { selector:
'button' })` works where `getByRole('button', { hidden: true })` doesn't
reliably resolve through the `<Button>` wrapper component.
- `BrushCursor` uses `reactive()` mock store and queries the brush /
gradient elements by id (`#maskEditor_brush`,
`#maskEditor_brushPreviewGradient`). File-level eslint-disable for
`testing-library/no-node-access` because the component's anchor elements
are styled divs without ARIA roles or labels.
- The `radial-gradient` (hardness < 1) branch of `gradientBackground` is
intentionally **not** asserted via rendered DOM: the computed returns a
multi-line template literal that happy-dom's CSS parser drops entirely.
The math (`getEffectiveBrushSize` / `getEffectiveHardness`) is covered
by `brushUtils.test.ts`. v8 reports 100% branch coverage because Vue
evaluates the computed regardless of whether the resulting style string
is parsed by the DOM.
- Style aligned with sibling tests: `should ...` naming, `describe`
grouped by feature, `vi.hoisted` for command-store / selection-state
mocks, real i18n via `createI18n`.
┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-11648-test-add-unit-tests-for-SidePanel-MaskEditorButton-and-BrushCursor-34e6d73d365081e38c4afee32ddf2b0b)
by [Unito](https://www.unito.io)
|
||
|
Terry Jia
|
2d4ca9c387 |
test: add unit tests for PointerZone and ToolPanel (#11649)
## Summary
Add unit tests for `PointerZone` and `ToolPanel` mask editor components,
raising each from 0% to **91.89% / 100%** respectively.
## Changes
- **What**: Add 2 sibling test files (24 tests total):
- `PointerZone.test.ts` (13): mount exposes root element to
`store.pointerZone`; pointer events (down/move/up) forward to
`toolManager.handlePointer*`; `pointerleave` hides brush + clears
cursor; `pointerenter` calls `toolManager.updateCursor`; touch events
(start/move/end) forward to `panZoom.handleTouch*`; wheel zooms then
updates cursor with wheel coords; `isPanning` watcher sets cursor to
"grabbing" then back via `updateCursor`; contextmenu's default is
prevented.
- `ToolPanel.test.ts` (11): one container rendered per `allTools`; icon
HTML for each tool; current tool highlighted with
`maskEditor_toolPanelContainerSelected` class while others are not;
clicking a container calls `toolManager.switchTool` with the matching
enum value; zoom indicator rounds `displayZoomRatio * 100`; dimensions
text reflects `image.width × image.height` (or empty when no image);
clicking the zoom indicator calls `store.resetZoom`.
## Review Focus
- `PointerZone` mocks `useToolManager` / `usePanAndZoom` to plain
function bags via factory helpers. The component is mostly forwarding,
so the test's value is in *event mapping* (which event triggers which
handler), not handler internals.
- happy-dom doesn't propagate `clientX` / `clientY` through the
`WheelEvent` constructor; the wheel test sets them via
`Object.defineProperty` after construction. Without this,
`updateCursorPosition` reads `undefined`.
- `PointerZone` ends at 91.89% statements / 70% branch — uncovered lines
are the `onMounted` early-return when `pointerZoneRef.value` is
`undefined` (always set in tests) and the watcher's same guard. Both
unreachable in normal use, intentionally not covered.
- `ToolPanel` mocks `iconsHtml` to `<svg data-testid="icon-${tool}" />`
markers, letting tests assert per-tool rendering via `getByTestId`. Real
i18n via `createI18n` for the reset-zoom tooltip text.
- `getToolContainers` queries by class because tool buttons are
unlabeled `<div>`s with click handlers (no role / aria); a file-level
`eslint-disable testing-library/no-node-access` covers this and the
dimensions-span placeholder query.
- Style aligned with sibling tests: `should ...` naming, `describe`
grouped by feature, `reactive()` mock store + `let mockStore` reset in
`beforeEach`, real i18n where keys are user-visible.
┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-11649-test-add-unit-tests-for-PointerZone-and-ToolPanel-34e6d73d3650817daf85c0d33d16899d)
by [Unito](https://www.unito.io)
|
||
|
Terry Jia
|
6bf75b4cf0 |
refactor(load3d): introduce ModelAdapter abstraction for the loader switch (#11627)
> Prerequisite work for improved PLY / 3D Gaussian Splatting support — the per-format loader logic needs to live behind a stable seam before splat-specific fixes (orientation, async-decoder waits, GPU dispose, custom bounds) and capability-driven UX gating can be added without touching `LoaderManager`'s switch every time. ## Summary Pure refactor. Extracts the per-extension switch inside `LoaderManager` into three `ModelAdapter` implementations and wires the manager to dispatch through them. **No behavior change** — same loader code paths, same outputs, same fallbacks. Sixth in the series splitting up the https://github.com/Comfy-Org/ComfyUI_frontend/pull/11495. ## Changes - **What**: - `ModelAdapter.ts` (new): defines the `ModelAdapter` interface (`kind`, `extensions`, `capabilities`, `load`), a `ModelLoadContext` that exposes only the `SceneModelManager` surface adapters need (`setOriginalModel`, `registerOriginalMaterial`, `standardMaterial`, `materialMode`), and a shared `fetchModelData(path, filename)` helper. - `MeshModelAdapter.ts` (new): owns `stl`, `fbx`, `obj`, `gltf`, `glb`. Each branch is a 1:1 lift of the corresponding `case` from `LoaderManager.loadModelInternal` on `main`. - `PointCloudModelAdapter.ts` (new): owns `ply`. Includes the existing `FastPLYLoader` / `PLYLoader` fallback and the `pointCloud` vs mesh branching logic. - `SplatModelAdapter.ts` (new): owns `spz`, `splat`, `ksplat`. Wraps the `SplatMesh` in a `Group` exactly like the previous `loadSplat` did. - `LoaderManager.ts`: now owns just an adapter array (default = the three above) and a small dispatch path. `pickAdapter` matches by extension and routes PLY → splat when the `Comfy.Load3D.PLYEngine` setting is `sparkjs` (preserving the previous routing). `getCurrentAdapter()` is the new public reader used by `Load3d`. - `Load3d.isSplatModel` / `isPlyModel` now query `loaderManager.getCurrentAdapter()?.kind` instead of doing tree-introspection (`containsSplatMesh`) or `instanceof THREE.BufferGeometry` checks. Same return values, decoupled from the model shape. - `LoaderManagerInterface` no longer exposes the per-format loader fields (`gltfLoader`, `objLoader`, etc.); those are now adapter-internal. - `SceneModelManager` is **unchanged** in this PR. Its existing `containsSplatMesh()` traversal and PLY material-mode rebuild stay put; a follow-up PR refactors them once capability gating is in place. ## Review Focus - **Loader equivalence**: the body of every `case` in `main`'s `LoaderManager.loadModelInternal` is now in the corresponding adapter's `load()` method. Easiest way to verify: diff `main`'s `LoaderManager.loadModelInternal` against the four `load()` bodies and confirm each branch's behavior (file fetch + parse + material wiring + group wrapping) is byte-identical. - **Dispatch parity**: `pickAdapter` produces the same routing as `main` — extension match first, then the PLYEngine === 'sparkjs' override hoisted up from inside the old `loadPLY`. - **Capability fields are dormant**: the `ModelAdapterCapabilities` record (`fitToViewer`, `materialModes`, `fitTargetSize`, …) is declared on every adapter but **not consumed anywhere in this PR**. SceneModelManager / Load3d / Load3DControls still read no capability data. The follow-up PR turns these on. - **`setOriginalModel` / `registerOriginalMaterial`**: adapters now go through the `ModelLoadContext` getter rather than reaching into `modelManager` directly. The context's `standardMaterial` and `materialMode` are exposed via getters so a late-bound `materialMode` is read at the actual call site, not snapshotted at context creation. ## Coverage | File | Stmts | Branch | Funcs | Lines | |---|---|---|---|---| | `ModelAdapter.ts` (new) | **100%** | **100%** | **100%** | **100%** | | `MeshModelAdapter.ts` (new) | **100%** | **100%** | **100%** | **100%** | | `PointCloudModelAdapter.ts` (new) | 97.22% | 61.11% | 75% | 97.22% | | `SplatModelAdapter.ts` (new) | **100%** | **100%** | **100%** | **100%** | | `LoaderManager.ts` (modified) | **100%** | 91.17% | 86.66% | **100%** | | `Load3d.ts` (modified) | 6.63% | 0% | 13.68% | 6.7% | All four new files are at or near 100% via dedicated unit tests for each adapter (load happy path, error propagation, extension declarations, capability shape). `LoaderManager.test.ts` exercises the dispatch logic — extension matching, the `ply → splat` sparkjs override, the stale-load discard, the load-context proxying — across 34 cases. The two changed `Load3d.ts` methods (`isSplatModel`, `isPlyModel`) get dedicated tests verifying they read the current adapter's `kind` and fall back to `false` when none is loaded. `Load3d.ts`'s overall 6.7% number is the pre-existing baseline — the existing `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). PR-F's surface in `Load3d.ts` is two method bodies, both covered by the new adapter-driven kind queries test. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11627-refactor-load3d-introduce-ModelAdapter-abstraction-for-the-loader-switch-34d6d73d3650811b8a1ccc55b45100f2) by [Unito](https://www.unito.io) |
||
|
Terry Jia
|
1b07e82ff7 |
fix: resolve mesh widget thumbnails via asset preview API (#11538)
## Summary The Load3d select-model widget was passing the raw .glb URL as the item preview_url, which browsers can't render as an image, producing the broken-image icon on cloud/local asset-enabled servers. Resolve thumbnails lazily from the asset API using the preview_id link (matching Media3DTop's behavior), look up by basename to stay consistent with the write path in useLoad3d, and fall back to a 3D-box placeholder when no preview exists yet. ## Screenshots before <img width="1112" height="1333" alt="image" src="https://github.com/user-attachments/assets/a8fa88ad-ab82-4951-be03-d28111322e30" /> after with asset-enable on BE https://github.com/user-attachments/assets/34b416af-5729-4ad0-bf17-722461ffc659 without asset-enable on BE <img width="1026" height="1201" alt="image" src="https://github.com/user-attachments/assets/71fd463f-ca77-4d63-85ed-01261d032d53" /> ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11538-fix-resolve-mesh-widget-thumbnails-via-asset-preview-API-34a6d73d365081d2aefac044dab0dfc3) by [Unito](https://www.unito.io) |
||
|
Terry Jia
|
9f4c54eb24 |
refactor: extract Load3d right-click guard to load3dContextMenuGuard (#11625)
## Summary Pull the right-click vs right-drag detection out of `Load3d` into a sibling helper. Mechanical refactor — no behavior change. Third of four small PRs splitting up the [`remove-ply-3dgs-nodes-squashed` mega-commit.](https://github.com/Comfy-Org/ComfyUI_frontend/pull/11495.) ## Changes - **What**: New `load3dContextMenuGuard.ts` exports `attachContextMenuGuard(target, onMenu, { isDisabled, dragThreshold })`. It installs `mousedown` / `mousemove` / `contextmenu` listeners against a single `AbortController` and returns one dispose function. - `Load3d` now calls `attachContextMenuGuard(this.renderer.domElement, (event) => this.onContextMenuCallback?.(event), { isDisabled: () => this.isViewerMode })` and stores the returned disposer in a single field. Drops four private fields (`rightMouseStart`, `rightMouseMoved`, `dragThreshold`, `contextMenuAbortController`) plus the now-redundant `showNodeContextMenu` private method. - The 5px drag threshold and `isViewerMode` gating are preserved. ## Review Focus - The three event handlers (`mousedown`, `mousemove`, `contextmenu`) inside the new helper match the old inline implementations one-for-one — same `e.button === 2` / `e.buttons === 2` checks, same call to `exceedsClickThreshold`, same `preventDefault` + `stopPropagation` ordering. - `isDisabled: () => this.isViewerMode` replaces the inline `if (this.isViewerMode) return` early-out — same gate, just lifted to a callback. - A single `AbortController.abort()` (in the returned disposer) replaces the old four-field teardown in `Load3d.remove()`. - 9 unit tests cover the helper: click vs drag distinction at the threshold, drag-then-click reset, `isDisabled` short-circuit, and the disposer detaching all three listeners. ## Coverage | File | Stmts | Branch | Funcs | Lines | |---|---|---|---|---| | `load3dContextMenuGuard.ts` (new) | **100%** | **93.33%** | **100%** | **100%** | | `Load3d.ts` (modified) | 7.12% | 0% | 13.97% | 7.18% | The single uncovered branch on `load3dContextMenuGuard.ts` (line 22) is the default-parameter fallback for `dragThreshold` when the caller omits it — `Load3d` always passes `{ isDisabled, dragThreshold: 5 }` through `attachContextMenuGuard`'s second-arg destructure, so the default never fires under the production call path. Adding a test that omits `dragThreshold` would push it to 100%; left as-is to avoid a change-detector test for a default value. The `Load3d.ts` numbers are the pre-existing baseline on `main` — `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 `initContextMenu` rewrite and the `remove()` teardown change both sit in those same uninstantiated paths and rely on browser e2e for end-to-end coverage, the same as before. Net: the click-vs-drag logic that previously had no unit test is now ≥93% covered through the extracted helper. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11625-refactor-extract-Load3d-right-click-guard-to-load3dContextMenuGuard-34d6d73d36508162aecef46553a3f50d) by [Unito](https://www.unito.io) |
||
|
Terry Jia
|
6f6fc88b0f |
test: add unit tests for MaskEditorContent main container (#11651)
## Summary
Add unit tests for `MaskEditorContent` (the mask editor's main
orchestration container), raising coverage from 0% to **94.11% / 83.72%
/ 83.33% / 94.11%** (statements / branches / functions / lines).
## Changes
- **What**: Add `src/components/maskeditor/MaskEditorContent.test.ts`
(12 tests) covering:
- **Mount**: keyboard listeners attached, ResizeObserver observes the
container, all 5 canvas refs assigned to the store before init runs.
- **Init flow**: `loader.loadFromNode` → `imageLoader.loadImages` →
`panZoom.initializeCanvasPanZoom` → `canvasHistory.saveInitialState` →
`brushDrawing.initGPUResources` → `initPreviewCanvas` chain runs in
order; child UI (`ToolPanel` / `PointerZone` / `SidePanel` /
`BrushCursor`) only renders after init succeeds; GPU preview canvas
resolution matches the mask canvas.
- **Init errors**: rejection from `loader.loadFromNode` or
`panZoom.initializeCanvasPanZoom` is caught, logged, and triggers
`dialogStore.closeDialog()`.
- **ResizeObserver**: callback invokes `panZoom.invalidatePanZoom()`
(captured the constructor argument to call it manually).
- **Drag**: `Ctrl+drag` is preventDefault'd; plain drag is not.
- **Unmount**: cleanup runs `brushDrawing.saveBrushSettings`,
`keyboard.removeListeners`, `canvasHistory.clearStates`,
`store.resetState`, `dataStore.reset`.
## Review Focus
- Heavy mock surface (10 modules): the 3 stores, 5 composables, plus 4
child Vue components and `LoadingOverlay`. All mocks are `vi.hoisted`
module-level. `mockStore` is `reactive()` because the source mutates
`activeLayer` (visible in template binding), `maskCanvas`, etc.; the
rest are plain function bags.
- Child components are stubbed to bare `<div data-testid>` so init
reveal can be asserted via `screen.findByTestId(...)` without engaging
their real implementations (each has its own test file).
- `MockResizeObserver` captures the constructor callback in module-level
`lastResizeCallback`. The "invalidate on resize" test invokes it
manually with empty args — that's enough to exercise the source's `if
(panZoom) { await panZoom.invalidatePanZoom() }` branch since the
callback only consumes `panZoom` from closure.
- happy-dom doesn't propagate `ctrlKey` through the `DragEvent`
constructor, so the drag tests set it via `Object.defineProperty(event,
'ctrlKey', { value })` (same pattern used in `PointerZone.test.ts` for
wheel `clientX/Y`).
- 94.11% line coverage — the two uncovered blocks (`containerRef`
missing, canvas refs missing) are early-return error paths unreachable
when Vue successfully mounts; not worth constructing a fixture to
trigger.
- Style aligned with sibling tests: `should ...` naming, `describe`
grouped by feature, `vi.hoisted` mocks reset via `beforeEach`,
`screen.findByTestId` for async render assertions.
┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-11651-test-add-unit-tests-for-MaskEditorContent-main-container-34e6d73d365081b38af2e057cb7daf9e)
by [Unito](https://www.unito.io)
|
||
|
Terry Jia
|
492bec28c8 |
test: add unit tests for TopBarHeader (#11650)
## Summary
Add unit tests for `TopBarHeader` mask editor dialog component, raising
coverage from 0% to **100%** across statements, branches, functions, and
lines.
## Changes
- **What**: Add `src/components/maskeditor/dialog/TopBarHeader.test.ts`
(17 tests) covering:
- Localized title rendering.
- Undo / Redo buttons forward to `store.canvasHistory.{undo,redo}`.
- Four transform buttons (rotate left / right, mirror horizontal /
vertical) call the matching `canvasTransform` action — parametrized via
`it.each`.
- All four transform error paths: rejected promise is caught, swallowed,
and logged with the right `[TopBarHeader] ... failed:` prefix.
- Invert calls `canvasTools.invertMask`; Clear calls both
`canvasTools.clearMask` and `store.triggerClear`.
- Save: hides brush, awaits `saver.save()`, closes the dialog on
success; switches button text to "Saving" while in-flight; restores
brush + button label and logs on save failure.
- Cancel: closes the dialog with the `global-mask-editor` key.
## Review Focus
- All five composable / store dependencies are mocked at module level
via `vi.hoisted`: `useMaskEditorStore`, `useDialogStore`,
`useCanvasTools`, `useCanvasTransform`, `useMaskEditorSaver`. Only the
store needs `reactive()` (`brushVisible` flips during save flow); the
rest are plain function bags.
- `Button.vue` is stubbed to a thin `<button :disabled>` so role queries
(`getByRole('button', { name: ... })`) resolve cleanly without dragging
in the real UI button's classes / variants.
- Real i18n via `createI18n`. Icon buttons rely on `:title` for their
accessible name; text buttons rely on slot text — both are reachable
through `getByRole('button', { name: ... })`.
- The "Saving" text test uses an unresolved promise to keep the
in-flight state observable; `waitFor` + `void user.click(...)` lets us
assert without awaiting the click. The dangling promise resolves at the
end so vitest doesn't complain.
- `it.each` parametrizes the four transform success paths and
(separately) the four error paths, keeping the file tight.
- Style aligned with sibling tests: `should ...` naming, `describe`
grouped by feature, `vi.hoisted` for cross-test mocks, real i18n with
`createI18n`.
┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-11650-test-add-unit-tests-for-TopBarHeader-34e6d73d365081adab66e460cf56accb)
by [Unito](https://www.unito.io)
|
||
|
Comfy Org PR Bot
|
13b660a15b |
1.44.10 (#11620)
Patch version increment to 1.44.10 **Base branch:** `main` --------- Co-authored-by: christian-byrne <72887196+christian-byrne@users.noreply.github.com> Co-authored-by: github-actions <github-actions@github.com> Co-authored-by: Alexander Brown <drjkl@comfy.org> |
||
|
Terry Jia
|
b232831441 |
fix: stop duplicate node creation when dropping image on Vue nodes (#11541)
## Summary handleDrop checked `handled === true` to gate stopPropagation, but onDragDrop from useNodeDragAndDrop is async and always returns a Promise, so the check never matched. The drop then bubbled to the document handler in app.ts and spawned a new LoadImage node in addition to the one that accepted the drop. Updated onDragDrop to take an optional claimEvent flag — when true, it calls preventDefault()/stopPropagation() synchronously inside the handler, only after the sync acceptance check passes (valid files / same-origin URI), and before any await. The Vue node now just calls await node.onDragDrop(event, true). Rejected payloads (cross-origin URI, files filtered out, no valid files) skip the claim and bubble to the document fallback as before. The remaining edge case is async URI fetch failures, which we can't sync-detect without speculatively claiming. ## Screenshots (if applicable) before https://github.com/user-attachments/assets/d79a5101-370b-4873-8365-5f9ce188731b after https://github.com/user-attachments/assets/8b787474-eab9-4060-8146-c4d8bb24ff9f ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11541-fix-stop-duplicate-node-creation-when-dropping-image-on-Vue-nodes-34a6d73d36508113b153e31768602933) by [Unito](https://www.unito.io) |
||
|
Dante
|
996e362ba6 |
test: add unit tests for form-dropdown internals (#11441)
## Summary Adds 32 unit tests across 3 files covering the internals of the FormDropdown family (input, filter, menu item). Part of a widget-test-coverage sequence. ## Changes - **What**: - `FormDropdownInput.test.ts` (14) — placeholder vs selected-items display, label preference, multi-item join, select-click emit, file-input rendering/accept/multiple/disabled/upload event. - `FormDropdownMenuFilter.test.ts` (8) — option rendering, v-model update on click, single-option disabled state, import-button gating by \`useModelUpload.isUploadButtonEnabled\` (mocked), \`showUploadDialog\` invocation. - `FormDropdownMenuItem.test.ts` (10) — label vs name preference, img/video rendering by injected \`AssetKindKey\`, placeholder gradient, list-small layout, click emits index, mediaLoad event, selection indicator. ## Review Focus - \`useModelUpload\` mocked at the module boundary with a dynamic import of \`vue\` inside \`vi.mock\` (needed because \`vi.hoisted\` runs before imports). - \`AssetKindKey\` provided via \`global.provide\` using the \`ComputedRef<AssetKind>\` shape. - \`v-tooltip\` registered as a no-op directive to avoid render errors in happy-dom. - No changes to any source component. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11441-test-add-unit-tests-for-form-dropdown-internals-3486d73d3650813cb4a1c6568280ef1a) by [Unito](https://www.unito.io) --------- Co-authored-by: GitHub Action <action@github.com> |
||
|
Terry Jia
|
b0fa179b87 |
refactor: extract Load3d render loop to load3dRenderLoop (#11623)
## Summary
Pull the `requestAnimationFrame` loop and its activity-gated tick body
out of `Load3d` into a small `startRenderLoop({ tick, isActive })`
helper. Pure mechanical refactor — no behavior change. First of four
small PRs splitting up the
https://github.com/Comfy-Org/ComfyUI_frontend/pull/11495.
## Changes
- **What**: New `load3dRenderLoop.ts` exports `startRenderLoop` (returns
a `{ stop }` handle). `Load3d.startAnimation()` now constructs a loop
through it; `Load3d.remove()` calls `stop()` instead of
`cancelAnimationFrame`. Field `animationFrameId: number | null` becomes
`renderLoop: RenderLoopHandle | null`.
## Review Focus
- The tick body inside `startAnimation()` is byte-identical to the
previous inline body — only the rAF scheduling has moved.
- `isActive()` is now invoked through a `() => this.isActive()` closure
instead of a direct call inside the inline `animate` function, so the
activity check still fires once per frame and reads the same fields.
- The new helper has 4 unit tests covering: ticks while active, skip
while inactive, stop halts ticks, stop is idempotent.
┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-11623-refactor-extract-Load3d-render-loop-to-load3dRenderLoop-34d6d73d3650815c9c4ec7713e912e37)
by [Unito](https://www.unito.io)
|
||
|
Terry Jia
|
ba6dd2a09c |
refactor(load3d): extract viewport math to load3dViewport (#11624)
## Summary Pull two pure helpers out of `Load3d` into a sibling module. Mechanical refactor — no behavior change. Second of four small PRs splitting up the https://github.com/Comfy-Org/ComfyUI_frontend/pull/11495. ## Changes - **What**: New `load3dViewport.ts` exports two pure functions: - `computeLetterboxedViewport({ width, height }, targetAspectRatio)` returns `{ offsetX, offsetY, width, height }` — the aspect-ratio fitting math that was duplicated inline in three places (`renderMainScene`, `setBackgroundImage`, `handleResize`). - `isLoad3dActive(flags)` consumes the activity-flag struct used by the rAF tick gate; `Load3d.isActive()` now delegates to it. ## Review Focus - The three call sites in `Load3d.ts` produce byte-identical viewport rectangles for any `(containerWidth, containerHeight, targetAspect)` triple — same branch on aspect-ratio comparison, same offset derivation. Simplest way to verify: diff the math. - `isLoad3dActive` ORs the same six flags as before in the same order. Old implementation read `this.STATUS_MOUSE_ON_NODE` etc. directly; new one reads them through a flags object built at the call site. - 13 unit tests cover the helpers: the "wider" / "taller" / "matching" aspect cases for letterboxing, and each individual flag flipping `isLoad3dActive` on by itself. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11624-refactor-load3d-extract-viewport-math-to-load3dViewport-34d6d73d365081ab9af5fc445bf5bd5a) by [Unito](https://www.unito.io) --------- Co-authored-by: GitHub Action <action@github.com> |
||
|
Kelly Yang
|
4a9001f675 |
test: add E2E tests for image crop widget Levels 4-7 (#11571)
## Summary
Adds 12 Playwright E2E tests for the `ImageCropV2` widget covering
aspect ratio selection, lock/unlock behavior, constrained resize, and
BoundingBox numeric input — all of which had zero test coverage.
## Changes
**Level 4 — Aspect Ratio Selection** (`with source image after
execution`)
- Selecting 16:9 preset adjusts crop height proportionally via
`applyLockedRatio`
- Selecting Custom unlocks the ratio and restores all 8 resize handles
**Level 5 — Lock/Unlock** (`without source image` + `with source image
after execution`)
- Selecting a preset auto-enables the lock (aria-label changes to
"Unlock aspect ratio")
- Unlocking after a preset reverts the dropdown display to "Custom"
- Full lock→unlock round-trip verifies handle count (4 → 8) and
aria-label on both transitions
**Level 6 — Constrained Resize** (`with source image after execution`)
- NW corner drag grows origin (x, y decrease) and dimensions while
maintaining ratio
- SE corner drag beyond image edge clamps to boundary
- NW corner drag beyond (0, 0) clamps x/y to image boundary
- Inward SE corner drag enforces `MIN_CROP_SIZE` (16px minimum)
**Level 7 — BoundingBox Numeric Input** (`with source image after
execution`)
- X increment button increments crop x by 1
- Width increment button increments crop width by 1
- BoundingBox inputs reflect updated position after a drag
No source code was modified.
<!-- CURSOR_SUMMARY -->
---
> [!NOTE]
> **Low Risk**
> Low risk: changes are limited to Playwright E2E coverage plus adding
`data-testid` attributes to BoundingBox inputs, with no behavioral logic
changes expected.
>
> **Overview**
> **Expands E2E coverage for the `ImageCropV2` widget** by adding new
Playwright tests for ratio preset selection, lock/unlock behavior,
constrained resizing (including boundary clamping and min size), and
BoundingBox numeric input updates.
>
> **Improves testability of BoundingBox controls** by adding
`data-testid` attributes to the `WidgetBoundingBox.vue`
`ScrubableNumberInput` fields (`x`, `y`, `width`, `height`) so E2E tests
can target increment/input elements reliably.
>
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
|
||
|
Dante
|
9cf035879f |
test: add WidgetCurve unit tests (#11469)
## Summary Splits the WidgetCurve test coverage out of #11446 so this widget can be reviewed independently. ## Changes - **What**: Adds WidgetCurve unit tests covering point forwarding, interpolation updates, disabled-state behavior, and upstream value handling. ## Review Focus Focused test-only PR extracted from #11446. Validated with `pnpm test:unit -- --run src/components/curve/WidgetCurve.test.ts`. ## Screenshots (if applicable) N/A ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11469-test-add-WidgetCurve-unit-tests-3486d73d365081c2a68bc8403fa0265f) by [Unito](https://www.unito.io) |
||
|
pythongosssss
|
125c11b3d0 |
fix: translate blueprint label (#11573)
## Summary Fixes hardcoded "Blueprint" text and adds e2e coverage to test visibility, resolving #11473 ## Changes - **What**: - add translation - add test ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11573-fix-translate-blueprint-label-34b6d73d365081009215e06be6aa1fa0) by [Unito](https://www.unito.io) |
||
|
pythongosssss
|
40fec7882c |
chore: remove unused glslUtils helpers (#11597)
## Summary These look to have been added as part of the initial phased implementation to align with the backend code, however they are unused: - `detectOutputCount` - The frontend only shows a single output preview which imo is fine, we can add multi display in future if required - `hasVersionDirective` - The backend needs this as it can execute other version shaders, the web browser will automatically validate and throw an error if it is not a valid shader, this gets surfaced to the user already. ## Changes - **What**: - remove unused functions & tests ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11597-chore-remove-unused-glslUtils-helpers-34c6d73d365081eab784d205b8a0053e) by [Unito](https://www.unito.io) |
||
|
jaeone94
|
17d980dbc8 |
feat: add inline-CTA nightly survey for error panel (#11591)
## Summary Adds an inline-CTA Typeform survey for the redesigned error panel, targeting nightly localhost users. Reuses the existing node-search survey infrastructure rather than introducing a parallel stack. ## Changes - **What**: - `surveyRegistry` gains optional `presentation: 'floating' | 'inline-cta'` and a `getFloatingSurveys()` helper; controller filters by it. - `NightlySurveyPopover` accepts `mode` + `v-model:open`. Manual mode skips the eligibility watcher, drops the "Not Now" button, and leaves open/close/markSeen to the parent. - New `ErrorPanelSurveyCta.vue` renders a CTA in the error tab footer once `useExecutionErrorStore.hasAnyError` has transitioned `null → value` at least 3 times. - Popover lives in `NightlySurveyController` (session-persistent). Shared state via module-level singleton (`useErrorSurveyPopoverState`) so the iframe survives error-tab unmounts during workflow switches. - `useTypeformEmbed` centralises script loading (singleton Promise, 10s timeout, explicit `window.tf.load()` on each new container). Necessary because the embed's DOMContentLoaded auto-scan only fires once; late consumers need an explicit re-scan to render. - CTA and feedback-gate strings added under `errorPanelSurvey.*`. ## Review Focus - Manual-mode flow in `NightlySurveyPopover.vue` — CTA click is routed through the module singleton; popover stays mounted after the first open (`hasOpenedOnce` + `v-show`) to preserve the iframe across repeated open/close cycles and workflow switches. - `useTypeformEmbed.ts` — the `window.tf.load()` trick (verified against the CDN `embed.js`) is what lets two surveys coexist; without it Typeform's one-shot DOM scan misses whichever element mounts second. - `NightlySurveyController.vue` guards against double-mount by requiring `presentation === 'inline-cta'` on `errorPanelConfig`. - Scope is nightly+localhost only (`isNightlyLocalhost` in `useSurveyEligibility`); async component gate in `TabErrors.vue` keeps this out of Cloud/Desktop/stable bundles. ## Test plan - [x] `IS_NIGHTLY=true pnpm dev`, clear `Comfy.SurveyState` + `Comfy.FeatureUsage`, trigger 3 failed runs → CTA appears in error tab footer. - [x] Click "Give feedback" → Typeform popover opens and renders the form. - [x] Close popover, switch workflow (error tab unmounts), trigger a new error → CTA reappears and reopening the popover shows the same iframe. - [x] Open error-panel popover, close, then trigger 3 node searches → node-search auto-popup renders its own iframe correctly (two surveys coexist). - [x] CTA × dismisses and persists (`seenSurveys['error-panel']`). - [x] "Don't ask again" inside popover sets `optedOut: true` and hides all nightly surveys. - [x] Cloud/Desktop/stable builds: CTA never renders, controller's manual popover doesn't mount. ## Screenshot https://github.com/user-attachments/assets/91145f23-fd1e-4caf-b6cc-4b97d33ed6b7 ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11591-feat-add-inline-CTA-nightly-survey-for-error-panel-34c6d73d3650817d9f95fddcf64633de) by [Unito](https://www.unito.io) |
||
|
Benjamin Lu
|
04e430e006 |
fix: dedupe keybinding modifier display (#11570)
## Summary Fix keybinding display so pressing a modifier key by itself shows that modifier once instead of duplicated text like `Shift + Shift`. ## Changes - **What**: Centralizes modifier key labels in `KeyComboImpl` and omits the duplicated primary key when the pressed key is itself a modifier. - **Breaking**: None. - **Dependencies**: None. ## Review Focus The keybinding model still includes active modifiers for normal shortcuts like `Ctrl + Shift + k`, while modifier-only input now renders as a single key. Regression coverage includes single modifier presses, combined held modifiers, and a normal non-modifier shortcut. Checks run: `pnpm exec vitest run src/platform/keybindings/keyCombo.test.ts`; `pnpm lint:unstaged`; `pnpm exec oxfmt --check src/platform/keybindings/keyCombo.ts src/platform/keybindings/keyCombo.test.ts`; `pnpm exec vitest run src/platform/keybindings/keyCombo.test.ts src/platform/keybindings/keybindingStore.test.ts src/platform/keybindings/keybindingService.escape.test.ts src/platform/keybindings/keybindingService.canvas.test.ts`; `pnpm typecheck`; pre-commit lint-staged checks; pre-push `pnpm knip --cache`. Linear: FE-240 ## Screenshots (if applicable) Not applicable; covered by keybinding sequence unit tests. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11570-fix-dedupe-keybinding-modifier-display-34b6d73d365081968a88da4465c151de) by [Unito](https://www.unito.io) |
||
|
Comfy Org PR Bot
|
bab122ad6b |
1.44.9 (#11581)
Patch version increment to 1.44.9 **Base branch:** `main` ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11581-1-44-9-34c6d73d3650811a82d9fe7a039a7edc) 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> |
||
|
Kelly Yang
|
bba3c38ae1 |
test: E2E coverage for painter widget (Levels 1–5) (#11551)
## Summary
Adds 7 new E2E tests for the Painter widget covering the remaining
untested items from Levels 1–5 of the widget test plan. All new tests
pass typecheck, lint, and the full pre-commit hook suite.
## Changes
**`browser_tests/tests/painter.spec.ts`**
- **Level 1.2** — assert node size ≥ 450×550 via the graph API to verify
the painter extension enforces its minimum dimensions
- **Level 1.3** — assert `width`, `height`, and `bg_color` widgets have
`options.hidden = true` so they don't appear as standard widget controls
- **Level 2.4** — cursor element becomes visible on pointer enter, its
CSS transform updates as the mouse moves across the canvas, and it hides
on pointer leave; uses `expect.poll` on transform to avoid the Vue
microtask race
- **Level 4.2** — set brush color to `#ff0000` via input event, draw a
stroke, sample a 40×40 region at canvas center and assert red pixels (R
> 200, G < 50, B < 50)
- **Level 4.3** — set opacity to 50%, draw a stroke, sample pixel alpha
values and assert semi-transparency (50 < α < 230)
- **Level 4.4** — focus the hardness slider, press ArrowLeft ×10, assert
the display value changes from `100%` to `90%`
- **Level 5.4** — set background color input to `#ff0000` via input
event, assert the canvas container div has `background-color: rgb(255,
0, 0)`
**`src/components/painter/WidgetPainter.vue`**
- Added `data-testid="painter-canvas-container"` to the inner canvas
wrapper div
- Added `data-testid="painter-cursor"` to the brush cursor div
- Added `data-testid="painter-bg-color-row"` to the background color
control row
- Added `data-testid="painter-hardness-value"` to the hardness display
span (mirrors the existing `painter-size-value` pattern)
<!-- CURSOR_SUMMARY -->
---
> [!NOTE]
> **Low Risk**
> Low risk: changes are limited to Playwright E2E coverage plus a few
`data-testid` attributes to stabilize selectors, with no functional
logic changes to the painter behavior.
>
> **Overview**
> Adds **7 new Playwright E2E tests** for the Painter widget, covering
minimum node sizing/hidden standard widgets, cursor
visibility/positioning behavior, brush hardness display updates,
color/opacity effects on rendered strokes, and background color
application.
>
> Updates `WidgetPainter.vue` to add a handful of **`data-testid`
hooks** (canvas container, cursor, background color row, hardness value)
used by the new tests.
>
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
|
||
|
Dr.Lt.Data
|
bd96bdf4cc |
fix(manager): migrate 4 endpoints GET→POST for CSRF hardening (#11520)
## Summary Align `comfyManagerService` and Manager UI state with CSRF hardening in [Comfy-Org/ComfyUI-Manager#2818](https://github.com/Comfy-Org/ComfyUI-Manager/pull/2818) (4.2.0, Content-Type gate + GET→POST migration) and [Comfy-Org/ComfyUI-Manager#2823](https://github.com/Comfy-Org/ComfyUI-Manager/pull/2823) (4.2.1, `extension.manager.supports_csrf_post` feature flag). ## Changes - **Service layer**: Convert 4 state-mutation endpoints (`START_QUEUE`, `UPDATE_ALL`, `UPDATE_COMFYUI`, `REBOOT`) from GET to POST. `body=null` + axios default `Content-Type: application/json` is allowed by the backend's `reject_simple_form_post` gate (only the three CORS simple-form types are rejected). - **UI/state layer**: Add `ManagerUIState.INCOMPATIBLE` triggered when the backend advertises `supports_manager_v4` but not `supports_csrf_post`. Manager UI is treated as "not installed" — buttons hide via `shouldShowManagerButtons` with zero call-site changes across `TopMenuSection`, `MissingNodeCard`, `MissingPackGroupRow`, `TabErrors`. - **Graceful degraded mode**: One-shot upgrade toast (warn, 15s) dispatched via `watch(immediate:true)` with a module-level guard that survives multiple composable instances. `openManager()` re-emits on explicit user action so stale shortcuts still surface guidance. i18n (en/ko) covering Desktop / standalone pip / Manager UI self-update paths. - **Breaking**: None. Existing policies preserved (`--enable-manager` absent → `DISABLED`; `--enable-manager-legacy-ui` → `LEGACY_UI`; feature flags not yet loaded → `NEW_UI` transient fallback). ## Review Focus - Decision-tree ordering in `useManagerState.ts`: `supports_csrf_post` check evaluates before `NEW_UI`/`LEGACY_UI` branches so stale Manager backends never reach the enabled paths. - Toast guard: module-level `incompatibleToastShown` survives multiple composable instances (tests verify 3× `useManagerState()` = 1 toast call). - `generatedManagerTypes.ts` still declares the 4 endpoints as GET; regeneration follows once Manager 4.2.1 OpenAPI is published. Runtime is unaffected since axios operates on the route string. ## References - [Comfy-Org/ComfyUI-Manager#2818](https://github.com/Comfy-Org/ComfyUI-Manager/pull/2818) — CSRF Content-Type gate + GET→POST migration (4.2.0) - [Comfy-Org/ComfyUI-Manager#2823](https://github.com/Comfy-Org/ComfyUI-Manager/pull/2823) — `supports_csrf_post` feature flag (4.2.1) - [comfyui-manager 4.2.1 on PyPI](https://pypi.org/project/comfyui-manager/4.2.1) — release package |
||
|
Christian Byrne
|
4b7a027946 |
fix: route progress_text feature flag check through getDevOverride (#11384)
## Summary
Route the `progress_text` binary parser's feature-flag check through
`serverSupportsFeature()` so dev overrides via `localStorage` take
effect.
## Changes
- **What**: Replace
`this.getClientFeatureFlags()?.supports_progress_text_metadata` with
`this.serverSupportsFeature('supports_progress_text_metadata')` in the
`case 3` binary message handler, consistent with all other feature-flag
checks in the class.
## Review Focus
Minimal one-line change. The key consideration is that
`serverSupportsFeature()` routes through `getDevOverride()` first,
enabling `localStorage` overrides (`ff:supports_progress_text_metadata`)
for dev testing of the binary wire format.
Fixes #11187
┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-11384-fix-route-progress_text-feature-flag-check-through-getDevOverride-3476d73d36508161bca0d6c2ea7c3c55)
by [Unito](https://www.unito.io)
---------
Co-authored-by: GitHub Action <action@github.com>
|
||
|
Christian Byrne
|
cc1338e51e |
perf: exclude canvas nodes from PostHog session recording (#10494)
## Summary Add `ph-no-capture` class to TransformPane to block PostHog session recording of canvas node DOM mutations, eliminating 226ms CPU overhead in the viewport scenario. ## Changes - **What**: Add `ph-no-capture` CSS class to the TransformPane root div and a unit test guarding it - **Why**: Profiling showed PostHog session recording (via rrweb mutation observer) consuming **226ms CPU** in the viewport scenario — **9× more** than the entire Vue reactivity system (25ms). The 150+ LGraphNode Vue components that mount/unmount during pan/zoom each trigger rrweb to snapshot new DOM subtrees. ### How it works PostHog uses rrweb under the hood. The `ph-no-capture` class maps to rrweb's `blockClass`, which causes: 1. `processMutation` to **early-return** for `childList` mutations when the parent is blocked 2. `genAdds` to **skip child traversal** (`dom.childNodes()`) for blocked elements 3. The element to be replaced with a **same-size placeholder** in replay This means all 150+ node components inside TransformPane produce **zero mutation processing cost**. ### Scope - **TransformPane**: blocked — wraps all Vue-rendered graph nodes - **LinkOverlayCanvas**: evaluated but not blocked — contains only a single `<canvas>` element with no DOM children, so no mutation overhead - **All other UI** (sidebar, menus, dialogs, toolbar, bottom panel): continues recording normally ### Trade-off The graph canvas area appears as a blank placeholder in session replays. This is acceptable because canvas interaction is better captured via workflow JSON, and the performance gain far outweighs the replay fidelity loss. ## Review Focus - Correctness of the `ph-no-capture` placement on TransformPane as the optimal DOM boundary - Whether any other high-mutation DOM subtrees should also be blocked ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-10494-perf-exclude-canvas-nodes-from-PostHog-session-recording-32e6d73d36508169ab07f1b193860fb0) by [Unito](https://www.unito.io) --------- Co-authored-by: Dante <bunggl@naver.com> |
||
|
pythongosssss
|
0052cdadd4 |
test: e2e coverage for node templates (#11564)
## Summary Add E2E tests for node templates ## Changes - **What**: - add tests for save, insert, delete, import export - vue and litegraph - add testid to dialog - update `clickLitegraphMenuItem` to enable clicking children with the same name as parent ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11564-test-e2e-coverage-for-node-templates-34b6d73d365081a39ce5c713f05a2a92) by [Unito](https://www.unito.io) |