mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-05-14 01:36:14 +00:00
bb74ec94de38c466c093069a92b3a109022ec44b
7778 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Terry Jia
|
bb74ec94de |
test(load3d): add unit tests for 9 previously-untested controls (#11730)
## Summary Mirror the maskeditor coverage approach for load3d sub-components. Each component gets behavior tests covering rendering, conditional branches, v-model bidirectional sync, and emitted events. - ViewerLightControls: setting-store min/max/step + v-model - ViewerExportControls: format dropdown + click-to-export - ViewerCameraControls: type select, FOV slider visibility - ViewerSceneControls: with/without bg image branches, render mode - PopupSlider: trigger toggle, click-outside dismissal, defaults - CameraControls: switch button, FOV PopupSlider visibility - ExportControls: trigger popup, format selection, click-outside - AnimationControls: empty-list bypass, controls, time formatting - ViewerControls: dialog open routing + onClose wiring ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11730-test-load3d-add-unit-tests-for-9-previously-untested-controls-3506d73d365081eaa9e7c5d0b922fc14) by [Unito](https://www.unito.io) |
||
|
Kelly Yang
|
e7640d414b |
test: add E2E tests for ActionBarButtons toolbar component (#11561)
## Summary
- Add E2E tests for the `ActionBarButtons` toolbar component (FE-111)
- Add `data-testid="action-bar-buttons"` to the container div for stable
test targeting
- Register `TestIds.topbar.actionBarButtons` in `selectors.ts`
## Changes
- `browser_tests/tests/actionBarButtons.spec.ts` — 6 tests across 5
scenarios: empty state, button rendering, icon rendering, multiple
buttons, click handler, mobile label hiding
- `src/components/topbar/ActionBarButtons.vue` — adds `data-testid` to
container
- `browser_tests/fixtures/selectors.ts` — registers new test ID
<!-- CURSOR_SUMMARY -->
---
> [!NOTE]
> **Low Risk**
> Primarily adds Playwright coverage and a `data-testid` attribute;
runtime behavior is unchanged aside from an extra DOM attribute.
>
> **Overview**
> Adds a new Playwright spec (`actionBarButtons.spec.ts`) that verifies
the ActionBarButtons container empty state, rendering (label/icon),
multiple buttons, click handler execution, and mobile label-hiding
behavior by registering buttons via `window.app!.registerExtension`.
>
> Updates the UI and test selector plumbing by adding
`data-testid="action-bar-buttons"` to `ActionBarButtons.vue` and
exposing it as `TestIds.topbar.actionBarButtons` for stable E2E
targeting.
>
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
|
||
|
Alexander Brown
|
c168c37c94 |
chore: update comfyui-ci-container to 0.0.17 (#11569)
Update `comfyui-ci-container` image to `0.0.17` across all CI workflows. | Workflow | Before | After | |---|---|---| | `ci-perf-report.yaml` | `0.0.12` | `0.0.17` | | `ci-tests-e2e.yaml` (×2) | `0.0.16` | `0.0.17` | | `pr-update-playwright-expectations.yaml` | `0.0.16` | `0.0.17` | ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11569-chore-update-comfyui-ci-container-to-0-0-17-34b6d73d365081b8a52ac995855354cb) by [Unito](https://www.unito.io) Co-authored-by: Amp <amp@ampcode.com> |
||
|
pythongosssss
|
089051824c |
test: add tests for link related settings (#11612)
## Summary <!-- One sentence describing what changed and why. --> ## Changes - **What**: <!-- Core functionality added/modified --> - **Breaking**: <!-- Any breaking changes (if none, remove this line) --> - **Dependencies**: <!-- New dependencies (if none, remove this line) --> ## Review Focus <!-- Critical design decisions or edge cases that need attention --> <!-- If this PR fixes an issue, uncomment and update the line below --> <!-- Fixes #ISSUE_NUMBER --> ## Screenshots (if applicable) <!-- Add screenshots or video recording to help explain your changes --> ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11612-test-add-tests-for-link-related-settings-34c6d73d36508145885bd017162e6fae) by [Unito](https://www.unito.io) |
||
|
pythongosssss
|
517da289f6 |
feat: Search - add ghost node following setting and increase opacity (#11365)
## Summary Adds setting to disable the node auto-follow cursor behavior when adding nodes from the search, and increased the visibilty of Vue ghost nodes. ## Changes - **What**: - add setting - increase opacity - add test ## Review Focus <!-- Critical design decisions or edge cases that need attention --> <!-- If this PR fixes an issue, uncomment and update the line below --> <!-- Fixes #ISSUE_NUMBER --> ## Screenshots (if applicable) Before <img width="452" height="517" alt="image" src="https://github.com/user-attachments/assets/369c0d90-5352-482b-a1b3-36180bffb3ee" /> After <img width="440" height="536" alt="image" src="https://github.com/user-attachments/assets/2066fdd4-6eb4-4bfb-ac7c-559fc99de57d" /> ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11365-feat-Search-add-ghost-node-following-setting-and-increase-opacity-3466d73d3650811b9c27ed4cc930816d) by [Unito](https://www.unito.io) |
||
|
Dante
|
98c327b3c6 |
test: add unit tests for colorUtil edge cases (#11671)
## Summary
Extends `colorUtil.test.ts` with boundary tests that the existing suite
did not cover: malformed `parseToRgb` inputs, alpha-hex parsing through
`parseToRgb`, negative hue normalization in `hsbToRgb`, the full
`isTransparent` matrix, HSV-vs-HSB equivalence in `toHexFromFormat`, the
bare-hex prefix path, and a non-primary-color round-trip through
`hexToHsva` / `hsvaToHex`.
## Changes
- **What**: Adds 13 Vitest cases across 5 new `describe` blocks
(`parseToRgb edge cases`, `hsbToRgb normalization`, `isTransparent`,
`toHexFromFormat`, plus a non-primary round-trip in the existing
`hexToHsva / hsvaToHex` block). Uses the existing
`vi.mock('es-toolkit/compat')` memoize stub.
## Review Focus
- The non-primary palette round-trip allows ±1 per RGB channel because
`hsbToRgb` floors while `rgbToHex` rounds; the test asserts the bound
rather than exact equality.
- `parseToRgb` is exercised with alpha-bearing hex (`#f008`,
`#ff000080`); the function returns RGB-only, so the alpha is
intentionally discarded.
- `toHexFromFormat({h, s, v}, 'hsb')` covers the HSV-shaped object path
that wraps `hsbToRgb`.
## Testing
\`\`\`bash
pnpm exec vitest run src/utils/colorUtil.test.ts
pnpm format -- src/utils/colorUtil.test.ts
pnpm lint
pnpm typecheck
pnpm knip
\`\`\`
73 tests pass (60 prior + 13 new).
┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-11671-test-add-unit-tests-for-colorUtil-edge-cases-34f6d73d36508136bac9edfae32815ec)
by [Unito](https://www.unito.io)
|
||
|
Terry Jia
|
fc2a4e82cf |
feat(load3d): bind UI capability gating to ModelAdapterCapabilities (#11711)
> Final piece of the PLY / 3D Gaussian Splatting series. Previous PR made `ModelAdapterCapabilities` load-bearing on the engine side; the UI was still gating off `isSplatModel` / `isPlyModel` proxies. This PR routes the viewer and the viewer-mode dialog through the capability fields directly, so the same source of truth that drives `Load3d` behavior also drives what the user sees. Eighth and last in the series splitting up. ## Summary Snapshot `Load3d.getCurrentModelCapabilities()` into 5 Vue refs on each model load and pipe them through the existing `Load3D` / `Load3DControls` / `Load3dViewerContent` / `ModelControls` / `ViewerModelControls` / `Preview3d` props. Replaces the format-specific `:is-splat-model` / `:is-ply-model` props and the hardcoded "splat → drop light/gizmo/export" subtraction with additive capability gates. No engine behavior changes — capability values are what previous PR already produces; UI now consumes them. ## Changes - **`useLoad3d.ts` / `useLoad3dViewer.ts`**: 5 new refs (`canFitToViewer` / `canUseGizmo` / `canUseLighting` / `canExport` / `materialModes`) refreshed on every load via `load3d.getCurrentModelCapabilities()`. `useLoad3dViewer` extracts the snapshot into a single `captureAdapterFlags(source)` helper because it runs in three places (initializeViewer / initializeStandaloneViewer / loadStandaloneModel). - **`Load3D.vue`**: gate the fit-to-viewer button on `canFitToViewer`; pass capability refs to `Load3DControls` instead of `isSplatModel` / `isPlyModel`. - **`Load3DControls.vue`**: build `availableCategories` additively (`['scene','model','camera']` plus `light` / `gizmo` / `export` if their capability is true) rather than subtracting from a fixed list when `isSplatModel` is true. Forwards `materialModes` to `ModelControls`. - **`Load3dViewerContent.vue`**: gate the light / gizmo / export sidebar sections on the capability refs; pass `materialModes` to `ViewerModelControls`. - **`ModelControls.vue` / `ViewerModelControls.vue`**: drop the local `materialModes` computed (which derived its options from `isPlyModel` and a hardcoded mesh list) and accept `materialModes` as a `readonly MaterialMode[]` prop. An empty array hides the dropdown entirely. - **`Preview3d.vue`** (renderer linearMode): mirror the prop swap on the standalone preview path. ## Review Focus - **Capability prop wiring is the only public-API change for child components**. `ModelControls` and `ViewerModelControls` lost `hideMaterialMode` / `isPlyModel` props. Any extension that imported these components directly will need to migrate, but they're internal `src/components/load3d/controls/**` files and not part of the documented extension surface. - **Empty-`materialModes` semantics**: previously hidden via `:hide-material-mode`; now hidden via `materialModes.length === 0`. `SplatModelAdapter` declares `materialModes: []`, so the splat case keeps the same behavior — the dropdown disappears. PLY adds `'pointCloud'` to the array, so the dropdown picks up that mode automatically without the controls needing an `isPlyModel` branch. - **`captureAdapterFlags` runs after every load completes**, so switching between mesh and splat in the same viewer instance updates the chrome correctly. Verified via the new `Load3D.test.ts` / `Load3dViewerContent.test.ts` cases. - **Capability gating is inclusive of `canFitToViewer`** in this PR even though `Load3DControls` has no fit category — the fit-to-viewer floating button on `Load3D.vue` is what reads it. PLY's `fitToViewer: true` means the button stays visible for PLY users. ## Coverage | File | Stmts | Branch | Funcs | |---|---|---|---| | `Load3D.vue` (modified) | 53.3% | **95.5%** | 83.3% | | `Load3DControls.vue` (modified) | 77.5% | **94.8%** | 86.4% | | `Load3dViewerContent.vue` (modified) | 60.6% | 72.1% | 54.5% | | `controls/ModelControls.vue` (modified) | 16.3% | 0% | 0% | | `controls/viewer/ViewerModelControls.vue` (modified) | **100%** | **100%** | **100%** | | `composables/useLoad3d.ts` (modified) | 78.7% | 64.5% | 71.4% | | `composables/useLoad3dViewer.ts` (modified) | 76.0% | 52.1% | 66.7% | Four new test files (`Load3D.test.ts` / `Load3DControls.test.ts` / `Load3dViewerContent.test.ts` / `controls/viewer/ViewerModelControls.test.ts`) cover the new capability gating directly: each component is rendered with capability flags toggled on/off and the appropriate sidebar / dropdown / button visibility is asserted. Capability prop forwarding from `Load3D.vue` → `Load3DControls.vue` and from `Load3dViewerContent.vue` → `ViewerModelControls.vue` is exercised end-to-end. `controls/ModelControls.vue` is the legacy node-side ModelControls — its existing tests live elsewhere and were not in this PR's scope; the diff line covered (the `v-if="materialModes.length > 0"` swap) is exercised by the new `Load3DControls.test.ts` cases that drive a non-empty / empty `materialModes` through. `Preview3d.vue` (renderer linearMode) has no test file in the project; the prop swap there is the same shape as the `Load3D.vue` swap which is covered. `useLoad3d.ts` / `useLoad3dViewer.ts` percentages are roughly the pre-existing baseline. The diff lines (the 5 new refs and the `captureAdapterFlags` helper) are exercised by the existing composable tests via the mock that now stubs `getCurrentModelCapabilities()`. 73 new component unit tests; 393 total load3d-related tests pass on this branch. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11711-feat-load3d-bind-UI-capability-gating-to-ModelAdapterCapabilities-3506d73d365081b3af68f30e3f728e24) by [Unito](https://www.unito.io) |
||
|
pythongosssss
|
e48d33e4c0 |
test: Canvas grid, ctx menu scaling and group padding settings (#11721)
## Summary Adds tests for canvas snap to grid, context menu scaling and group padding ## Changes - **What**: - add tests for canvas related settings ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11721-test-Canvas-grid-ctx-menu-scaling-and-group-padding-settings-3506d73d36508141868cfa990d903c33) by [Unito](https://www.unito.io) |
||
|
pythongosssss
|
967f1eb562 |
test: extract title editor test component (#11605)
## Summary Extract shared TitleEditor component and update tests to use it ## Changes - **What**: - add title editor helper - update locations that used `TestIds.node.titleInput` ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11605-test-extract-title-editor-test-component-34c6d73d3650811da6b0ec493b190c3f) by [Unito](https://www.unito.io) |
||
|
Kelly Yang
|
8b83559402 |
refactor: extract useBrushPersistence from useBrushDrawing (#11543)
## Summary
PR B of this https://github.com/Comfy-Org/ComfyUI_frontend/pull/11388
Part of the `useBrushDrawing` decomposition plan (PR B).
Extracts brush settings persistence logic into a dedicated
`useBrushPersistence` composable, reducing the responsibility surface of
`useBrushDrawing`.
No runtime behavior is changed — this is a pure structural refactor.
## Changes
- **New** `src/composables/maskeditor/useBrushPersistence.ts` —
encapsulates `loadAndApply` (reads brush settings from localStorage and
applies them to the store on init) and `save` (debounced write of
current brush settings to localStorage)
- **New** `src/composables/maskeditor/useBrushPersistence.test.ts` —
unit tests covering load from empty storage, full restore round-trip,
missing `stepSize` fallback, corrupted data resilience, and
save-to-localStorage behavior
- **Updated** `src/composables/maskeditor/useBrushDrawing.ts` — removes
the inlined persistence functions and delegates to `useBrushPersistence`
## Test Locally
1. Adjust brush size and hardness, close MaskEditor, then reopen it —
brush parameters should restore to the previous settings (verifies
`save` + `loadAndApply`) - pass
2. Draw a few strokes and confirm the marks appear correctly (verifies
the `saveBrushSettings` public interface is not broken) - pass
https://github.com/user-attachments/assets/961155d5-6742-4668-a419-51c29b850edf
<!-- CURSOR_SUMMARY -->
---
> [!NOTE]
> **Low Risk**
> Low risk refactor that moves localStorage read/write logic behind a
new composable and adds unit tests; main risk is accidental behavior
drift in when/what brush settings are persisted/restored.
>
> **Overview**
> Refactors mask editor brush settings persistence by extracting the
localStorage load/save (including debounced writes and `stepSize`
fallback) out of `useBrushDrawing` into a new `useBrushPersistence`
composable, while keeping the `saveBrushSettings` public API wired
through.
>
> Adds `useBrushPersistence` unit tests covering empty storage,
round-trip restore, missing field defaults, corrupted JSON handling, and
save semantics.
>
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
|
||
|
Kelly Yang
|
bc11b5ff5e |
test: add E2E tests for LoginButton toolbar component (#11562)
## Summary
- Add E2E tests for the `LoginButton` toolbar component (FE-109)
- Add `data-testid="login-button"` to `LoginButton.vue` for stable
targeting
- Register `TestIds.topbar.loginButton` in `selectors.ts`
## Changes
- `browser_tests/tests/loginButton.spec.ts` — 7 tests covering:
visibility toggled by `show_signin_button` server feature flag, ARIA
label, click → sign-in dialog, hover popover content, popover dismissal
- `src/components/topbar/LoginButton.vue` — adds `data-testid`
- `browser_tests/fixtures/selectors.ts` — registers new test ID
## Notes
`LoginButton` is rendered in `WorkflowTabs` when `flags.showSignInButton
?? isDesktop` is true. Since `isDesktop = false` in the OSS test build
(`DISTRIBUTION=localhost`), tests enable the button by setting
`window.app.api.serverFeatureFlags.value.show_signin_button = true` —
the established pattern used throughout the test suite (e.g.
`nodeLibraryEssentials.spec.ts`, `shareWorkflowDialog.spec.ts`).
<!-- CURSOR_SUMMARY -->
---
> [!NOTE]
> **Low Risk**
> Low risk: changes are limited to adding stable `data-testid` hooks
plus new Playwright E2E coverage, with only a minor adjustment to a
unit-test performance threshold.
>
> **Overview**
> Adds Playwright E2E coverage for the topbar `LoginButton`, including
feature-flag-driven visibility, ARIA labeling, click-to-open sign-in
dialog, and hover popover behavior (including the *Learn more* link and
dismissal).
>
> To make the tests stable, `LoginButton.vue` now exposes `data-testid`
attributes for the button and popover elements, and `selectors.ts`
registers new `TestIds.topbar.*` entries.
>
> Relaxes the `useModelToNodeStore.getCategoryForNodeType` performance
test threshold (from 10ms to 100ms) while clarifying the intent of the
check.
>
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
|
||
|
Dante
|
8c1ea7ae64 |
test: add unit tests for useNodePricing edge cases (#11673)
## Summary
Extends `useNodePricing.test.ts` with three behavioral gaps the existing
suite did not cover: `getNodePricingConfig` does not leak the compiled
JSONata expression, `pricingRevision` ticks after async evaluation
resolves (with a cache-hit path that does not), and
`formatPricingResult` returns `''` for non-finite numeric inputs across
all four result types.
## Changes
- **What**: Adds 9 Vitest cases across two existing `describe` blocks
(`getNodePricingConfig`, `formatPricingResult`) and one new block
(`reactive revision`). Reuses the existing `priceBadge` and
`createMockNodeWithPriceBadge` helpers.
## Review Focus
- The cache-hit assertion checks that `pricingRevision.value` does not
advance after a second `getNodeDisplayPrice` call with the same
signature, exercising the WeakMap cache hit at `useNodePricing.ts:573`.
- Non-finite coverage spans `type:'usd'`, `type:'range_usd'`,
`type:'list_usd'` (empty + all-non-finite + mixed), and the legacy `{
usd }` shape, matching the four `asFiniteNumber` call sites in
`formatPricingResult`.
- The strip-`_compiled` assertion uses `toHaveProperty` so the test
fails loudly if a future refactor accidentally re-exposes the runtime
JSONata instance to debug consumers.
## Testing
\`\`\`bash
pnpm exec vitest run src/composables/node/useNodePricing.test.ts
pnpm format -- src/composables/node/useNodePricing.test.ts
pnpm lint
pnpm typecheck
pnpm knip
\`\`\`
91 tests pass (82 prior + 9 new).
┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-11673-test-add-unit-tests-for-useNodePricing-edge-cases-34f6d73d365081dab525cdaa71b348a5)
by [Unito](https://www.unito.io)
|
||
|
Dante
|
69e68847d9 |
test: add unit tests for workspaceAuthStore retry/race paths (#11674)
## Summary
Extends `useWorkspaceAuth.test.ts` with the retry, race,
storage-resilience, and Zod-validation paths the existing suite did not
exercise: exponential backoff on `TOKEN_EXCHANGE_FAILED`, immediate
context clearing on permanent errors, stale-refresh abort when the user
switches workspaces mid-flight, and resilience to `sessionStorage` quota
errors.
## Changes
- **What**: Adds 6 Vitest cases across three new `describe` blocks
(`refreshToken retry/race paths`, `persistToSession resilience`, `Zod
validation on token response`). Reuses the existing `mockGetIdToken`,
`mockTeamWorkspacesEnabled`, `vi.useFakeTimers()`, and
`mockTokenResponse` fixtures.
## Review Focus
- The retry test uses `vi.runAllTimersAsync()` so the three backoff
sleeps (1s + 2s + 4s) drain in a single tick instead of slowing the
suite. Both `console.warn` (per-attempt) and `console.error`
(final-failure) are silenced.
- The race test resolves the in-flight refresh fetch with a token tied
to the OLD workspace AFTER `switchWorkspace('workspace-other')` has run,
so the assertion fails loudly if the stale-request guard regresses.
- The sessionStorage spy targets the instance method
(`vi.spyOn(sessionStorage, 'setItem')`); spying
`Storage.prototype.setItem` does not intercept happy-dom's per-instance
method.
## Testing
\`\`\`bash
pnpm exec vitest run
src/platform/workspace/stores/useWorkspaceAuth.test.ts
pnpm format -- src/platform/workspace/stores/useWorkspaceAuth.test.ts
pnpm lint
pnpm typecheck
pnpm knip
\`\`\`
33 tests pass (27 prior + 6 new).
┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-11674-test-add-unit-tests-for-workspaceAuthStore-retry-race-paths-34f6d73d365081e6a8ecce59a156585e)
by [Unito](https://www.unito.io)
|
||
|
comfydesigner
|
fad9cf0db7 |
fix: consolidate --color-coral-red variables into --color-coral (#10374)
Removes the desktop-exclusive `--color-coral-red` CSS variables and replaces their usage with the shared `--color-coral` palette to reduce variable duplication in the design system. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-10374-fix-consolidate-color-coral-red-variables-into-color-coral-32a6d73d365081a4ac88d0ea96aeea02) by [Unito](https://www.unito.io) --------- Co-authored-by: Alex <alex@Mac.lan> Co-authored-by: Amp <amp@ampcode.com> Co-authored-by: Alexander Brown <drjkl@comfy.org> Co-authored-by: DrJKL <DrJKL0424@gmail.com> |
||
|
pythongosssss
|
d532fcf779 |
test: add tests for canvas related settings (#11604)
## Summary Adds test coverage for canvas related settings ## Changes - **What**: - tests canvas info visibility, fps, pointer modes, selection ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11604-test-add-tests-for-canvas-related-settings-34c6d73d365081748b0ec79bc6fdc6ca) by [Unito](https://www.unito.io) --------- Co-authored-by: github-actions <github-actions@github.com> |
||
|
pythongosssss
|
52e73f2697 |
test: Expand node search box V2 e2e coverage (#10620)
## Summary Adds additional browser test coverage to the v2 node search ## Changes - **What**: - extend search v2 fixtures - add additional tests - add data-testid for targeting elements - rework tests to work with new menu UI ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-10620-test-Expand-node-search-box-V2-e2e-coverage-3306d73d365081b6bad3f73daab1194f) by [Unito](https://www.unito.io) --------- Co-authored-by: GitHub Action <action@github.com> |
||
|
Simon Pinfold
|
c4043637d6 |
fix: route context menu Download through downloadMultipleAssets (#11700)
*PR Created by the Glary-Bot Agent* --- ## Summary The asset context menu's Download action called `downloadAsset` directly, so multi-output jobs only downloaded the preview file instead of all outputs. Route through `downloadMultipleAssets`, which detects multi-output jobs and creates a ZIP export in cloud mode and falls back to the single-download path otherwise. ## Changes - **What**: Swap `actions.downloadAsset(asset)` for `actions.downloadMultipleAssets([asset])` in the per-asset context menu Download command, and extend the existing unit test to assert the routing. - **Breaking**: none - **Dependencies**: none ## Review Focus For single-output assets the behavior is unchanged: `downloadMultipleAssets([asset])` falls through to the individual-download path when `hasMultiOutputJobs` is false and `assets.length === 1` (see `useMediaAssetActions.ts:106`). Verified manually — right-clicking a single-output asset and clicking Download still produces one file download to the correct `/api/view` URL. ## Notes This is a focused replacement for the stale #10948. Compared to that branch: - Drops the unrelated `bootstrapStore` API-key auth changes (scope creep). - Drops the new `assets.cloud.spec.ts` Playwright spec — cloud asset-export E2E coverage was added in #11610 (`browser_tests/tests/sidebar/assets.spec.ts`), so a separate cloud spec for this routing change would mostly duplicate it. - Keeps the unit-test change minimal: extends the existing `ContextMenu` stub with a `model` prop watcher and adds one new test, rather than rewriting the whole file from `@testing-library/vue` to `@vue/test-utils`. ## Verification - `pnpm test:unit` (MediaAssetContextMenu.test.ts and useMediaAssetActions.test.ts) - `pnpm typecheck` - `pnpm lint` - `pnpm format` / `oxfmt --check` - `pnpm knip` - Manual: started the OSS dev server, generated a single-output asset via the queue API, opened the assets sidebar, right-clicked the asset, and confirmed the Download menu item triggers a single-file download (screenshot attached). ## Screenshots   ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11700-fix-route-context-menu-Download-through-downloadMultipleAssets-34f6d73d365081eb8135e8b699640d97) by [Unito](https://www.unito.io) --------- Co-authored-by: Glary-Bot <glary-bot@users.noreply.github.com> |
||
|
Alexander Brown
|
9d61b4df06 |
feat: ECS Phase 0b — ID type aliases (FE-166/475/476/477) (#11699)
## Summary
ECS Phase 0b — type-only ID aliases. Builds on FE-165 (centralized
version counter, base of this PR) and adds a tranche of named ID aliases
plus mechanical adoption at known call sites.
This PR now covers four child tickets:
- **FE-166** — adds `GroupId` (in `LGraphGroup.ts`) and `SlotIndex` (in
`interfaces.ts`); re-exported from the litegraph barrel.
- **FE-475** — mechanical adoption of existing aliases (`NodeId`,
`LinkId`, `RerouteId`, `GroupId`, `SlotIndex`, `ExecutionId`) across
litegraph at the audit-listed sites:
`LGraphState.lastGroupId/lastLinkId/lastRerouteId`,
`LGraphExtra.linkExtensions`, `ISerialisedGroup.id`,
`ISerialisedGraph.last_link_id`, `LinkNetwork.removeReroute`,
`INodeOutputSlot.slot_index`, `LGraphNode.{setOutputDataType,
getInputDataType, getOutputPos}` slot params,
`ExecutableNodeDTO.inputs[].linkId` + execution-id locals, and
`RenderLink/MovingLinkBase.fromSlotIndex` (plus subclasses that
redeclare).
- **FE-476** — adds `SubgraphId = UUID` in `LGraph.ts`; adopted at
`_subgraphs` Map, `findUsedSubgraphIds`, `getDirectSubgraphIds`, and
`ExportedSubgraphInstance.type`. Re-exported from the litegraph barrel.
- **FE-477** — adds app-domain entity aliases at their closest
schema/types files: `WorkflowId`, `AssetId`, `PromptId` (propagated as
existing `JobId`), `TaskId`, `UserId`, `WorkspaceId`,
`WorkspaceInviteId`, `NodePackId`. Adopted at primary use sites (entity
id fields, store state, service signatures).
## Entity reference
### ID aliases at a glance
| Alias | Underlying | Defined in | Identifies |
| --- | --- | --- | --- |
| `NodeId` | `number \| string` | `litegraph/LGraphNode.ts` | A node
within a graph |
| `LinkId` | `number` | `litegraph/LLink.ts` | A connection between two
slots |
| `RerouteId` | `number` | `litegraph/Reroute.ts` | A reroute waypoint
on a link |
| `GroupId` *(new)* | `number` | `litegraph/LGraphGroup.ts` | A visual
group of nodes |
| `SlotIndex` *(new)* | `number` | `litegraph/interfaces.ts` | A slot's
position on a node |
| `ExecutionId` | `string` | `litegraph/types/serialisation.ts` | A node
within a subgraph instance |
| `SubgraphId` *(new)* | `UUID` | `litegraph/LGraph.ts` | A subgraph
definition |
| `WorkflowId` *(new)* | `string` |
`platform/workflow/validation/schemas/workflowSchema.ts` | A saved
workflow document |
| `AssetId` *(new)* | `string` |
`platform/assets/schemas/assetSchema.ts` | A binary asset (model, image,
etc.) |
| `JobId` *(reused as `PromptId`)* | `string` | `schemas/apiSchema.ts` |
A queued prompt execution |
| `TaskId` *(new)* | `string` | `platform/tasks/services/taskService.ts`
| A backend background task |
| `UserId` *(new)* | `string` | `types/authTypes.ts` | An authenticated
user |
| `WorkspaceId` *(new)* | `string` |
`platform/workspace/workspaceTypes.ts` | A workspace |
| `WorkspaceInviteId` *(new)* | `string` |
`platform/workspace/workspaceTypes.ts` | A pending workspace invite |
| `NodePackId` *(new)* | `string` |
`workbench/extensions/manager/types/comfyManagerTypes.ts` | A Comfy
Registry / Manager node pack |
### How the entities relate
```mermaid
flowchart TB
subgraph LG["🎨 Litegraph entities"]
direction TB
Graph["LGraph<br/>id: SubgraphId"]
Subgraph["Subgraph<br/>id: SubgraphId"]
Node["LGraphNode<br/>id: NodeId"]
Link["LLink<br/>id: LinkId"]
Reroute["Reroute<br/>id: RerouteId"]
Group["LGraphGroup<br/>id: GroupId"]
Slot["INodeSlot<br/>slot_index: SlotIndex"]
Exec["ExecutableNodeDTO<br/>id: ExecutionId"]
end
subgraph AP["🌐 App-domain entities"]
direction TB
Workflow["ComfyWorkflow<br/>id: WorkflowId"]
Job["Prompt / Job<br/>id: JobId ≡ PromptId"]
Task["Task<br/>id: TaskId"]
Asset["Asset<br/>id: AssetId"]
Workspace["Workspace<br/>id: WorkspaceId"]
Invite["WorkspaceInvite<br/>id: WorkspaceInviteId"]
User["User<br/>id: UserId"]
Pack["NodePack<br/>id: NodePackId"]
end
Subgraph -. extends .-> Graph
Graph --> Node
Graph --> Link
Graph --> Group
Node --> Slot
Link --> Reroute
Link --> Slot
Node -. instantiates .-> Subgraph
Exec -. wraps .-> Node
Workflow -. serializes .-> Graph
Job --> Workflow
Job --> Task
Task --> Asset
Workspace --> Workflow
User --> Workspace
Workspace --> Invite
Pack -. provides .-> Node
```
Solid arrows are containment / direct references; dashed arrows are *“is
a kind of”* (`extends`) or cross-layer relationships (e.g. a
`ComfyWorkflow` *serializes* an `LGraph`; a `NodePack` *provides* node
definitions).
## Explicit non-goals
- `LGraphState.lastNodeId` is intentionally kept as bare `number`
(auto-increment counter; would widen if aliased to `NodeId = number |
string`).
- No new `SubgraphSlotId` alias — verified subsidiary (subgraph IO slots
are addressed via `SUBGRAPH_INPUT_ID/OUTPUT_ID` sentinel + numeric array
index, not by UUID alone).
- No `WidgetName`, `SlotName`, `WorkspaceMemberId` — verified subsidiary
(only meaningful inside a parent or as a relationship).
- No re-typing of `LGraph.id` / `Subgraph.id` — references adopt
`SubgraphId`, but the inherited UUID typing is left intact (minimal
diff).
## Type-only
All changes are structural-equivalent type aliases. No runtime behavior
changes. No new exports beyond the aliases themselves. No generated code
modified.
## Verification
- `pnpm typecheck` ✅
- `pnpm knip` ✅
- Scoped `npx eslint` on changed files ✅
- Lint-staged hooks (oxfmt, oxlint, eslint, typecheck) passed on every
commit
## Notes for reviewers
This branch was rebased onto `main` after FE-165 (`a441364a5`, PR
#11698) merged independently — the auto-skipped FE-165 commit is no
longer part of this PR. Six commits remain (oldest → newest):
| Commit | Maps to | Summary |
| --- | --- | --- |
| `e8e7ff795` | FE-166 | Add `GroupId` and `SlotIndex` aliases + barrel
re-exports |
| `e0bcb75a0` | FE-476 | Add `SubgraphId = UUID` alias |
| `2c136afb9` | FE-477 + FE-475 bulk | Add app-domain aliases;
mechanical adoption of
`NodeId`/`LinkId`/`RerouteId`/`GroupId`/`SlotIndex`/`ExecutionId`/`SubgraphId`
at audit-listed litegraph sites |
| `06d6e6a8b` | FE-477 | Adopt `TaskId` in asset stores |
| `f943e1c2b` | FE-476 | Adopt `SubgraphId` at remaining UUID reference
sites (`LGraphCanvas` clipboard map + paste, `SubgraphNode.type`) |
| `1739d5241` | review feedback | Tighten alias usage:
`linkExtensions.parentId: RerouteId`, drop redundant `String()` wraps in
`executionStore`, type `assetExportStore` map as `Map<TaskId,
AssetExport>` |
FE-475's mechanical adoption is bundled into `2c136afb9` rather than a
dedicated commit (parallel-agent execution on a shared working tree);
the substitutions themselves are complete — see the diff under
`src/lib/litegraph/src/`. PR will be squash-merged, so commit
granularity is informational.
Fixes FE-166
Fixes FE-475
Fixes FE-476
Fixes FE-477
---------
Co-authored-by: Amp <amp@ampcode.com>
|
||
|
Christian Byrne
|
963a7bf178 |
refactor: consolidate browser_tests/helpers/ into fixtures/ (#11411)
*PR Created by the Glary-Bot Agent* --- ## Summary - Eliminates the confusing dual-helpers structure where `browser_tests/helpers/` and `browser_tests/fixtures/helpers/` coexisted one tier apart with overlapping purposes - Routes each file to its natural home based on what it actually *is*: page objects → `components/`, standalone utils → `utils/`, domain helper classes stay in `helpers/` - Adds an ESLint guard (`no-restricted-imports`) to prevent re-creating `browser_tests/helpers/` ## File Moves | File | From | To | Reason | |---|---|---|---| | `actionbar.ts` | `helpers/` | `fixtures/components/Actionbar.ts` | Page object class imported by ComfyPage | | `templates.ts` | `helpers/` | `fixtures/components/Templates.ts` | Page object class imported by ComfyPage | | `boundsUtils.ts` | `fixtures/helpers/` | `fixtures/utils/` | Pure function, not a helper class | | `mimeTypeUtil.ts` | `fixtures/helpers/` | `fixtures/utils/` | Pure function, not a helper class | | `builderTestUtils.ts` | `helpers/` | `fixtures/utils/` | Shared test setup functions | | `clipboardSpy.ts` | `helpers/` | `fixtures/utils/` | Page injection utility | | `fitToView.ts` | `helpers/` | `fixtures/utils/` | Canvas utility function | | `manageGroupNode.ts` | `helpers/` | `fixtures/utils/` | Litegraph interaction helper | | `painter.ts` | `helpers/` | `fixtures/utils/` | Test helper functions | | `perfReporter.ts` | `helpers/` | `fixtures/utils/` | Test infrastructure | | `promotedWidgets.ts` | `helpers/` | `fixtures/utils/` | Query helpers for specs | ## What Changed Beyond File Moves - **28 import statements** updated across test specs, fixtures, and infra files - **AGENTS.md** — directory tree diagram and architectural separation descriptions updated - **README.md** — "Leverage Existing Fixtures and Helpers" section updated - **`.claude/skills/perf-fix-with-proof/SKILL.md`** — perfReporter path reference updated - **`eslint.config.ts`** — added `@e2e/helpers/*` restricted import pattern to both spec and non-spec browser_tests rules ## Verification - `pnpm typecheck` — clean - `pnpm typecheck:browser` — clean - `pnpm lint` — 0 errors, 0 warnings - `pnpm format:check` — all files formatted - `pnpm knip` — clean - Pre-commit hooks passed full pipeline (oxfmt, oxlint, eslint, typecheck, typecheck:browser) ## Config Audit No changes needed to: `tsconfig.json` (`@e2e/*` alias covers all subdirs), `playwright.config.ts`, `vite.config.mts`, `knip.config.ts`, `.oxlintrc.json`, `nx.json` ## Manual Verification Note This is a pure structural refactoring (file moves + import updates) with zero behavioral or visual changes. The typecheck and lint passes confirm all imports resolve correctly. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11411-refactor-consolidate-browser_tests-helpers-into-fixtures-3476d73d3650816cb671ef7fa8433f66) by [Unito](https://www.unito.io) --------- Co-authored-by: glary-bot <glary-bot@comfy.org> Co-authored-by: Glary-Bot <glary-bot@users.noreply.github.com> Co-authored-by: DrJKL <DrJKL0424@gmail.com> Co-authored-by: Amp <amp@ampcode.com> |
||
|
Terry Jia
|
f001bc9af3 |
feat: derive Preview3D camera pose from EXTRINSICS/INTRINSICS matrices (#11626)
> Prerequisite work for improved PLY / 3D Gaussian Splatting support — splat workflows (and PLY pipelines that go through SHARP / COLMAP) emit camera pose as extrinsics + intrinsics matrices, and the viewer needs a way to consume them before that end-to-end story can ship. ## Summary Adds the ability to drive the Preview3D camera from a pair of OpenCV-convention extrinsics + intrinsics matrices (as produced by SHARP / COLMAP / other SfM pipelines), so backend nodes that emit such matrices can position the viewer's camera deterministically. Fifth in the series splitting up https://github.com/Comfy-Org/ComfyUI_frontend/pull/11495. Fully self-contained — adds new API surface only, no existing call paths are modified. ## Changes - **What**: - `cameraFromMatrices.ts` (new): pure function `computeCameraFromMatrices(extrinsics, intrinsics)` returning `{ position, target, fovYDegrees }`. No THREE.js dependency. Shape-validates the inputs (4×4 and 3×3) with a clear error. - `Load3d.setCameraFromMatrices(extrinsics, intrinsics)`: wires the utility into Load3d via `setCameraState` + `setFOV`. Preserves the caller's existing `zoom` and `cameraType`. - `load3d.ts` (extension): Preview3D's `onExecuted` extracts `extrinsics`/`intrinsics` from `result[3]`/`result[4]` (when present) and calls `setCameraFromMatrices`. The output tuple type is widened to include the two optional `Matrix` fields. ## Review Focus - **Convention conversion**: OpenCV is Y-down, Z-forward; three.js is Y-up, Z-back. The function flips Y and Z on both `position` and `target` (equivalent to a 180° X-axis rotation of the whole world). The same rotation is applied elsewhere to splats at load time, so a future splat + camera-pose pair lines up. - **FOV math**: vertical FOV (radians) = `2 * atan(cy / fy)`. We expose it in degrees, matching `cameraManager.setFOV`'s contract. - **Camera-state preservation**: `setCameraFromMatrices` reads the current state to keep `zoom` and `cameraType` intact — only `position`, `target`, and `fov` come from the matrices. - **Backwards compatibility**: backend nodes that don't return matrices simply skip the new branch (the `if (extrinsics && intrinsics)` guard). Existing Preview3D workflows are unaffected. - 7 unit tests for the matrix math (identity, rotation, translation, FOV derivation, shape errors, etc.); 1 unit test for the Load3d wiring (verifies the destructured `position`/`target`/`fovYDegrees` reach `setCameraState` + `setFOV` with `zoom`/`cameraType` preserved). ## Coverage | File | Stmts | Branch | Funcs | Lines | |---|---|---|---|---| | `cameraFromMatrices.ts` (new) | **100%** | **100%** | **100%** | **100%** | | `Load3d.ts` (modified) | ~7.6% | 0% | ~14.6% | ~7.7% | | `load3d.ts` (extension, modified) | 0% | 0% | 0% | 0% | The `Load3d.ts` numbers are the pre-existing baseline — `Load3d.test.ts` covers façade methods via prototype injection rather than instantiating the class (the constructor needs `THREE.WebGLRenderer`, which happy-dom can't provide). The new `setCameraFromMatrices` method body is exercised by a new unit test that asserts `setCameraState` receives the destructured matrix output and `setFOV` receives the computed `fovYDegrees`, with the caller's `zoom`/`cameraType` preserved. `load3d.ts` (the extension registration file, 0% on `main` and after this PR) has no unit-test scaffolding in the project — its `onExecuted` handler runs only after a workflow execution and is exercised end-to-end via browser tests. The new `if (extrinsics && intrinsics) load3d.setCameraFromMatrices(...)` branch sits in that path. Net: the matrix math itself, which previously didn't exist, is now 100% covered. The wiring layer relies on the same e2e safety net the surrounding code has always used. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11626-feat-derive-Preview3D-camera-pose-from-EXTRINSICS-INTRINSICS-matrices-34d6d73d3650817297bdf25a83a4f7a2) by [Unito](https://www.unito.io) |
||
|
Kelly Yang
|
88faaf3d86 |
test: complete remaining Painter widget E2E tests (#11613)
## Summary
Implemented E2E test coverage for Levels 6-12 of the Painter Widget
## Changes
Adds the following coverage to complete the test plan:
Level 6 - Input image connection (3 tests, @slow):
- Width/height/bg-color controls hide when input is linked
- Canvas resizes to match input image dimensions after execution
- Drawing over input image produces canvas content
Level 7 - Clear on empty canvas is harmless
Level 8 - Unchanged canvas does not re-upload on second serialization
Level 9 - Settings persistence:
- Tool selection saved to node.properties.painterTool
- Brush size change saved to node.properties.painterBrushSize
Level 10 - Compact layout collapses to grid-cols-1 when node width <
350px
Level 12 - Rapid drawing accumulates all strokes (checks 3 y-positions)
Supporting changes:
- Add data-testid="painter-controls" to controls grid in
WidgetPainter.vue (needed for compact mode class assertion)
- Add browser_tests/assets/widgets/painter_with_input.json workflow
fixture (LoadImage connected to Painter input slot 0)
<!-- CURSOR_SUMMARY -->
---
> [!NOTE]
> **Low Risk**
> Mostly adds/adjusts Playwright and unit test coverage; the only
runtime change is wrapping pointer-capture calls in `try/catch`, which
is low-risk but touches input-handling paths.
>
> **Overview**
> Completes and expands Painter widget browser test coverage, including
new scenarios for clearing an empty canvas, preventing redundant uploads
when serializing an unchanged canvas, persisting tool/brush-size
settings to node properties, compact layout behavior, multi-stroke
accumulation checks, and an input-image-connected workflow (new
`painter_with_input.json`) with execution/resizing/draw-over-image
assertions.
>
> Hardens `usePainter` pointer handling by tolerating
`setPointerCapture`/`releasePointerCapture` failures (e.g., synthetic
events), with corresponding unit tests updated/added to validate the
behavior and serialization expectations.
>
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
|
||
|
Terry Jia
|
42ff7b6c62 |
refactor(load3d): drive viewer behavior from ModelAdapter capabilities (#11660)
> Final architectural step in the PLY / 3D Gaussian Splatting series. Previous PR introduced `ModelAdapter` with a dormant `capabilities` field; this PR makes those capabilities load-bearing and replaces the remaining `instanceof SplatMesh` / `instanceof BufferGeometry` switches with adapter-driven dispatch. Together with previous one it removes the last of the format-specific branching from `SceneModelManager` / `Load3d`. Seventh in the series splitting up the https://github.com/Comfy-Org/ComfyUI_frontend/pull/11495. ## Summary Drive viewer behavior (fit-to-viewer, default camera pose, world bounds, GPU dispose, material rebuild) from `ModelAdapterCapabilities` + 3 new optional adapter methods, instead of `SceneModelManager` reflecting on model shape. `Load3d` is rewritten to take its 13 managers as injected `Load3dDeps`; a new `createLoad3d` factory assembles them and threads a single `AdapterRef` between `LoaderManager` (writer) and `SceneModelManager` + `Load3d` (readers). Splat orientation + decoder-race + sizing bugs are fixed as a side effect — splats now render upright, fill the grid, and don't lock the OrbitControls target on first frame. ## Changes - **`ModelAdapter.ts`**: add `AdapterRef = { current: ModelAdapter | null }` shared handle and 3 optional adapter methods — `computeBounds(model)`, `disposeModel(model)`, `defaultCameraPose()`. - **`SplatModelAdapter.ts`**: implement all 3 optional methods; `await splatMesh.initialized` so first-frame bounds are populated (fixes a decoder race that collapsed the OrbitControls target onto the camera); `quaternion.set(1, 0, 0, 0)` to convert sparkjs's OpenCV (Y-down, Z-forward) to three.js (Y-up, Z-back); flip `fitToViewer` back to `true` and bump `fitTargetSize` to `20` so splats fill the 20-unit grid instead of shrinking to 1/4 of it. - **`PointCloudModelAdapter.ts`**: extract `buildPointCloudForMaterialMode` so `SceneModelManager` rebuilds via the same code path the initial load uses; `setPath` so PLYs that reference relative assets resolve correctly. - **`LoaderManager.ts`**: accept optional `AdapterRef`; write through it instead of the internal `_currentAdapter` field. `clearModel()` now runs while the old adapter is still current so its `disposeModel()` can release renderer-owned resources. - **`SceneModelManager.ts`**: accept 4 capability lambdas (`getCurrentCapabilities` / `getBoundsFromAdapter` / `disposeModelViaAdapter` / `getDefaultCameraPose`) with `DEFAULT_MODEL_CAPABILITIES` / null fallbacks. `setupModel`, `fitToViewer`, and material-mode rebuild are now capability-driven; `containsSplatMesh` (30 lines of `instanceof` traversal) and `handlePLYModeSwitch` (90 lines of duplicated PLY rebuild) are gone. - **`Load3d.ts`**: ctor switches from manager-creation to deps-injected (`Load3dDeps`); add `getCurrentModelCapabilities()` reader; gate `setGizmoEnabled` / `setGizmoMode` / `resetGizmoTransform` / `applyGizmoTransform` on `capabilities.gizmoTransform`; `isSplatModel` / `isPlyModel` now read `adapterRef.current?.kind` directly. - **`createLoad3d.ts`** (new): single factory that builds the renderer (`createRenderer`), assembles all 13 managers in dependency order, and threads one shared `AdapterRef` through `LoaderManager` and `SceneModelManager`'s 4 capability lambdas. - **`useLoad3d.ts` / `useLoad3dViewer.ts`**: switch from `new Load3d(container, options)` to `createLoad3d(container, options)`. No other call-site changes. ## Review Focus - **Capability dispatch parity**: walk each former hardcoded branch in `SceneModelManager` and confirm it now falls out of the right capability: - `containsSplatMesh()` → `!capabilities.fitToViewer` + `getDefaultCameraPose()` - `handlePLYModeSwitch()` → `capabilities.requiresMaterialRebuild` + `buildPointCloudForMaterialMode()` - `Box3.setFromObject(model)` for sizing → `getBoundsFromAdapter(model) ?? Box3.setFromObject(model)` - Mesh/Points geometry+material disposal in `clearModel` → still happens, plus `disposeModelViaAdapter(obj)` for adapter-owned resources (sparkjs SplatMesh internal GPU state) - **`AdapterRef` lifecycle**: one ref is created in `createLoad3d`, passed to `LoaderManager` (writer) and `SceneModelManager` (read via 4 closures). `LoaderManager.loadModel` clears via the *old* adapter first (so `disposeModel` runs), then null-resets the ref before picking the new one. Test `keeps the old adapter current while clearModel runs` pins this ordering. - **Splat fixes are user-visible, not pure refactor**: - Orientation: `quaternion.set(1, 0, 0, 0)` matches the sparkjs README convention. Without it splats render upside-down and mirrored on Z. Same rotation is applied to the camera-from-matrices output in PR-E so a future splat + camera-pose pair lines up. - Decoder race: `await splatMesh.initialized` ensures `getBoundingBox` returns a non-zero box on the first call. Without it `setupModel`'s bounds → camera pipeline placed the OrbitControls target on the camera origin, locking the view. - Sizing: `fitTargetSize: 20` (vs. the mesh default of 5) means splat geometry spans the full 20-unit grid footprint instead of ~1/4 of it. Mesh assets are unaffected. - **Gizmo gating**: `setGizmoEnabled(true)` early-returns when `capabilities.gizmoTransform` is false. Internal `setGizmoEnabled(false)` still runs (so we can always disable). `setGizmoMode` / `resetGizmoTransform` / `applyGizmoTransform` no-op when the capability is off. - **`createLoad3d` is the single ctor entry**: `new Load3d(...)` is no longer callable from app code (ctor signature changed to `(container, deps, options)`). All call sites use `createLoad3d`. Test scaffolding still uses `Object.create(Load3d.prototype)` + property injection where it needs to bypass renderer creation. - **Backwards compatibility**: `LoaderManager`'s `adapterRef` and `SceneModelManager`'s 4 capability lambdas all have defaults (`createAdapterRef()` and `() => DEFAULT_MODEL_CAPABILITIES` etc.), so the existing test suites that construct these classes with the old signatures still compile and pass without modification beyond what's in this PR. ## Coverage | File | Stmts | Branch | Funcs | Lines | |---|---|---|---|---| | `ModelAdapter.ts` (modified) | **100%** | **100%** | **100%** | **100%** | | `LoaderManager.ts` (modified) | **100%** | 91.7% | 86.7% | **100%** | | `MeshModelAdapter.ts` (unchanged) | **100%** | **100%** | **100%** | **100%** | | `PointCloudModelAdapter.ts` (modified) | **97.9%** | 69.2% | 71.4% | **97.9%** | | `SplatModelAdapter.ts` (modified) | **100%** | **100%** | **100%** | **100%** | | `SceneModelManager.ts` (modified) | 75.4% | 67.2% | 72.2% | 75.4% | | `Load3d.ts` (modified) | 29.5% | 30.6% | 26.7% | 30.1% | | `createLoad3d.ts` (new) | 83.8% | **100%** | 58.3% | 83.8% | | `useLoad3d.ts` (modified) | 78.2% | 65.1% | 71.4% | 82.2% | | `useLoad3dViewer.ts` (modified) | 75.2% | 52.1% | 65.9% | 79.4% | `SplatModelAdapter.ts` jumps to 100% via 6 new tests covering the orientation set, the `await initialized` decoder wait, `computeBounds` (world-space transform + null fallback), `disposeModel` (per-SplatMesh dispose + no-op on non-splat trees), and `defaultCameraPose`. `createLoad3d.ts` hits 100% branch via a new test file with 12 cases — `WebGLRenderer` config, `Load3DOptions` forwarding, `AdapterRef` identity between `LoaderManager` and `SceneModelManager`, and the 4 capability lambdas in both adapter-null and adapter-published states (each delegates correctly to the adapter's optional methods or falls back to defaults). The remaining func% reflects the inline `gizmoTransformChange` callback — not a deliberate skip, just out of scope for the dispatch-wiring tests. `SceneModelManager.ts` and `Load3d.ts` numbers are the pre-existing baseline — the existing `*.test.ts` files cover façade methods via prototype injection rather than instantiating the classes (`Load3d` constructor needs `THREE.WebGLRenderer`, which happy-dom can't provide; `SceneModelManager` covers the new capability paths via its existing `createManager(overrides)` helper). All new branches (capability gating, capability-driven `setupModel` / `fitToViewer` / rebuild, adapter-driven `isSplatModel` / `isPlyModel`) have dedicated tests. Net diff: **+846 / −370** across 16 files (10 production, 6 test). ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11660-refactor-load3d-drive-viewer-behavior-from-ModelAdapter-capabilities-34f6d73d36508130b0ece884add182b9) by [Unito](https://www.unito.io) |
||
|
Dante
|
f404887c96 |
test: add unit tests for linkFixer (#11668)
## Summary Adds unit coverage for `linkFixer` serialized graph repair paths and fixes input-slot repairs being tracked without mutating the serialized workflow data. ## Changes - **What**: Adds 10 Vitest cases for valid links, dry-run reporting, origin/target repair, missing input-slot cleanup, dangling node cleanup, stale link deletion, and silent mode. - **What**: Applies serialized input slot mutations in fix mode and reports `fixed` only after a rerun confirms the graph is clean. - **What**: Adds the design-system package to knip workspace config so the pre-push hook recognizes its exported CSS dependency usage. - **Dependencies**: None. ## Review Focus The workflow validation path calls `fixBadLinks` with serialized workflow JSON, so the new tests stay at that boundary and assert the repaired graph shape directly. ## Test Coverage - CI snapshot for `src/utils/linkFixer.ts`: unit lines 1.8%. - Local targeted coverage: unit lines 83.9%, functions 94.11%, branches 75.15%. ## Testing ```bash pnpm format -- src/utils/linkFixer.ts src/utils/linkFixer.test.ts pnpm test:unit -- src/utils/linkFixer.test.ts pnpm test:unit -- src/utils/linkFixer.test.ts --coverage pnpm typecheck pnpm lint pnpm knip ``` ## screenshoot <img width="1691" height="927" alt="Screenshot 2026-04-28 at 10 09 26 AM" src="https://github.com/user-attachments/assets/ff59888f-bde3-48f5-853a-60df69e84492" /> |
||
|
Comfy Org PR Bot
|
b37c66539b |
1.44.12 (#11705)
Patch version increment to 1.44.12 **Base branch:** `main` ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11705-1-44-12-3506d73d36508121bc39e748c87a6235) by [Unito](https://www.unito.io) --------- Co-authored-by: christian-byrne <72887196+christian-byrne@users.noreply.github.com> Co-authored-by: github-actions <github-actions@github.com>v1.44.12 |
||
|
Alexander Brown
|
95e9a9405b |
test(subgraph): pin #10849 promoted-widget-value corruption with it.fails (#11697)
*PR Created by the Glary-Bot Agent* --- ## Summary #11579 restored *categorical* test coverage for subgraph serialization but didn't reproduce the specific Z-Image-Turbo regression introduced by #10849 — pre-#10849 templates whose `widgets_values` is leftover noise get corrupted on load because the new code applies that array positionally to promoted widget views. This PR adds two **vitest** cases that pin the user-visible symptom directly: after loading a misaligned legacy payload, the promoted widget value should reflect the source default, not the legacy `widgets_values[i]`. Both use `it.fails` so the suite stays green while the bug is present and flips to failing the moment the fix on `fix/subgraph-promoted-widget-inline-state` lands. ## Tests 1. `falls back to source widget value when proxyWidgets is in legacy 2-tuple shape` — configure() with `proxyWidgets: [['-1', 'widget']]` + `widgets_values: [999]` should leave the widget at the source default (42), not 999. 2. `does not corrupt unbound promoted widgets when widgets_values length mismatches view count` — same shape with longer/wrong-length array. ## Verification - All 8 cases in the file pass under `it.fails` (CI green). - Removing `.fails` locally produces the expected failures: `expected 42, received 999` and `expected 42, received 111` — confirming both tests catch the regression. - `pnpm typecheck`, `pnpm exec eslint`, `pnpm exec oxlint` all clean. ## Why `it.fails` and not plain failing tests The actual fix (`fix/subgraph-promoted-widget-inline-state`) is unmerged. Landing genuinely-failing tests on main would break CI for everyone. `it.fails` documents the bug runnably, keeps CI green, and signals when the fix lands so the marker can be dropped. ## Why these assertions, not "widgets_values must be undefined" A first draft asserted that `serialize()` should not write `widgets_values` at all. That conflicts with existing coverage in the same file (`preserves per-instance widget values after configure`, `round-trips per-instance widget values`) which deliberately uses `widgets_values` for round-trip persistence. These rewritten assertions target the load-time corruption symptom directly without contradicting the per-instance contract — feedback from Oracle review. ## Coordination Mirrors the failing tests already on commit `6a982675e` of the unmerged `fix/subgraph-promoted-widget-inline-state` branch, with the addition of `.fails` markers and a clarifying comment so they can land on main first. Follow-up to #11579. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11697-test-subgraph-pin-10849-promoted-widget-value-corruption-with-it-fails-34f6d73d365081d7a04dcf48ebeceafe) by [Unito](https://www.unito.io) --------- Co-authored-by: Glary-Bot <glary-bot@users.noreply.github.com> |
||
|
Dante
|
00974d6339 |
test: add E2E tests for publish flow wizard (#10770)
## Summary - Add Playwright E2E test coverage for the ComfyHub publish workflow dialog - Create `PublishDialog` page object fixture with programmatic dialog opening via Vite dynamic imports - Create `PublishApiHelper` for mocking all publish flow API endpoints (`/hub/profiles/me`, `/hub/labels`, `/hub/workflows`, `/userdata/*/publish`, `/assets/from-workflow`, `/hub/assets/upload-url`) - Add `data-testid` attributes to 6 publish flow components for stable E2E locators - 17 test scenarios across 7 describe blocks covering wizard navigation, form interactions, profile gate, save prompt, and publish submission ## Test plan - [ ] Run `pnpm test:browser:local -- --grep "Publish dialog"` against local dev server - [ ] Verify wizard navigation through Describe → Examples → Finish steps - [ ] Verify profile gate flow (with/without profile) - [ ] Verify save prompt for unsaved workflows - [ ] Verify publish success/failure scenarios Fixes #9079 ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-10770-test-add-E2E-tests-for-publish-flow-wizard-3346d73d3650818094d5fc3a84593402) by [Unito](https://www.unito.io) --------- Co-authored-by: dante <dante@danteui-MacStudio.local> Co-authored-by: GitHub Action <action@github.com> |
||
|
Dante
|
878ffb70cc |
test: add unit tests for assetService (#11670)
## Summary
Extends `assetService.test.ts` (which previously only covered
`shouldUseAssetBrowser`) with behavioral tests for the network-bound
methods: metadata fetch error mapping, base64 upload validation,
async-vs-sync upload routing, delete error propagation, model-folder
filtering, update validation, and tag-filtered list defaults.
## Changes
- **What**: Adds 10 Vitest cases across `getAssetMetadata`,
`uploadAssetFromBase64`, `uploadAssetAsync`, `deleteAsset`,
`getAssetModelFolders`, `updateAsset`, and `getAssetsByTag`. Reuses the
existing hoisted `mockDistributionState` / `mockSettingStoreGet` setup
and the existing `vi.mock('@/scripts/api')` boundary; adds local
`buildResponse` and `validAsset` helpers scoped to this file.
## Review Focus
- The localized error path is covered through public methods
(`getAssetMetadata`) rather than reaching for the internal
`getLocalizedErrorMessage`.
- `getAssetModelFolders` test asserts that the request URL omits
`include_public` (the internal call site passes no `includePublic`),
matching the conditional in `handleAssetRequest`.
- `uploadAssetAsync` tests pin the discriminated-union shape (`type:
'async' | 'sync'`) for both 202 and 200 responses.
## Testing
\`\`\`bash
pnpm exec vitest run src/platform/assets/services/assetService.test.ts
pnpm format -- src/platform/assets/services/assetService.test.ts
pnpm lint
pnpm typecheck
pnpm knip
\`\`\`
All 16 tests pass (6 prior + 10 new).
┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-11670-test-add-unit-tests-for-assetService-34f6d73d36508117b1aafaf463e9c820)
by [Unito](https://www.unito.io)
|
||
|
Alexander Brown
|
a441364a55 |
refactor(litegraph): centralize _version counter via incrementVersion() (#11698)
## Summary
Centralize all `LGraph._version` increments behind a single
`incrementVersion()` method to create the seam for a future
`VersionSystem` (ECS Migration Phase 0a).
## Changes
- **What**: Added `LGraph.incrementVersion()` and replaced all 19 direct
`graph._version++` writes across 7 files. Existing null guards at call
sites are preserved. Zero behavioral change — the counter is still only
read by `LGraphCanvas.renderInfo()` for debug display.
## Review Focus
- The new method is mechanical: `incrementVersion(): void {
this._version++ }`. Look for any sites I missed or null-guard
regressions.
- Files updated: `LGraph.ts`, `LGraphNode.ts`, `LGraphCanvas.ts`,
`widgets/BaseWidget.ts`, `subgraph/SubgraphInput.ts`,
`subgraph/SubgraphInputNode.ts`, `subgraph/SubgraphOutput.ts`.
Part of the [ECS Migration
Plan](https://github.com/Comfy-Org/ComfyUI_frontend/blob/main/docs/architecture/ecs-migration-plan.md).
Linear: FE-165.
┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-11698-refactor-litegraph-centralize-_version-counter-via-incrementVersion-34f6d73d3650810992f8fa3adbae3f38)
by [Unito](https://www.unito.io)
Co-authored-by: Amp <amp@ampcode.com>
|
||
|
Alexander Brown
|
8dcadd6fe1 |
test: restore deleted subgraph serialization E2E tests (#11579)
*PR Created by the Glary-Bot Agent* --- ## Summary #10759 removed ~12 E2E tests from `subgraphSerialization.spec.ts` during a reorganization that shifted semantic coverage to Vitest. Several of the removed tests covered the exact `serialize() → JSON → configure()` round-trip that #10849's positional `_instanceWidgetValues` path later regressed on Main — promoted widget values binding to the wrong slots when loading templates whose `widgets_values` ordering doesn't match current `proxyWidgets`. This restores the pre-reorg E2E coverage so future regressions in promoted-widget serialization are caught at the browser level. ## Restored tests From `subgraphSerialization.spec.ts` (pre-#10759): - **Deterministic proxyWidgets Hydrate** (3 tests) — round-trip stability and compressed `target_slot` resolution. - **Legacy And Round-Trip Coverage** (5 tests) — includes the most directly-relevant restorations: - `Promoted widgets survive serialize -> loadGraphData round-trip` - `Multi-link input representative stays stable through save/reload` - `Cloning a subgraph node keeps promoted widget entries on original and clone` - **Duplicate ID Remapping** (5 tests) — includes `Promoted widget tuples are stable after full page reload boot path`. The 4 tests that already existed on Main (added by #10849 and the Vue-nodes legacy-prefixed block) are kept as-is. ## Adaptations to current APIs - Imports reworked for the post-`@e2e/*` alias layout. - Redundant `comfyPage.nextFrame()` calls dropped — `loadWorkflow` / `loadGraphData` / `serializeAndReload` already wait internally (#11264). - Alt-drag clone block wrapped in `try/finally` around `keyboard.up('Alt')` to match the current `subgraphCrud.spec.ts` pattern. - `PromotedWidgetEntry` is now exported from `browser_tests/helpers/promotedWidgets.ts` so the restored `expectPromotedWidgetsToResolveToInteriorNodes` helper can type its argument. ## Review follow-ups applied - Use `expect.poll()` instead of `expect(async () => …).toPass()` for the single-value snapshot comparison, per `browser_tests/AGENTS.md`. - Capture and call `dispose()` from `SubgraphHelper.collectConsoleWarnings()` inside a `try/finally` so the console listener is unregistered after the test. ## Verification - `pnpm typecheck:browser` — clean. - `pnpm exec eslint` + `pnpm exec oxlint` on changed files — 0 warnings, 0 errors. - `pnpm exec oxfmt` on changed files — applied (no diff). - Ran 2 key restored tests against local ComfyUI + dev server: - `Promoted widgets survive serialize -> loadGraphData round-trip` — PASS (3.9s) - `Multi-link input representative stays stable through save/reload` — PASS (2.9s) - Full-suite runs in this sandbox are blocked by the existing `comfyPage` fixture's `createUser` path failing on repeat runs against persistent backend state — unrelated to this PR. CI will exercise the full suite. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11579-test-restore-deleted-subgraph-serialization-E2E-tests-34b6d73d365081f29b27c1069476ad17) by [Unito](https://www.unito.io) --------- Co-authored-by: Glary-Bot <glary-bot@users.noreply.github.com> |
||
|
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)
|
||
|
Alexander Brown
|
9c0edd0048 |
fix: prevent duplicate website-e2e CI runs on PRs (#11607)
## Summary Fix duplicate `CI: Website E2E` workflow runs on pull requests. ## Problem Two runs were triggered for every PR touching website files: - `website-e2e (pull_request)` — from the PR event - `website-e2e (push)` — from the push to a `website/*` branch The concurrency key used `github.ref`, which evaluates differently for push (`refs/heads/...`) vs pull_request (`refs/pull/N/merge`), so they couldn't cancel each other. ## Changes 1. Scope `push` trigger to `main` only (removes `website/*`) 2. Use `github.head_ref || github.ref` in the concurrency group so push and PR events for the same branch share a group ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11607-fix-prevent-duplicate-website-e2e-CI-runs-on-PRs-34c6d73d3650814c9d24c77b1591e94a) by [Unito](https://www.unito.io) --------- Co-authored-by: Amp <amp@ampcode.com> |
||
|
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
|
9a70676c61 |
[chore] Update Comfy Registry API types from comfy-api@c82adf6 (#11689)
## Automated API Type Update This PR updates the Comfy Registry API types from the latest comfy-api OpenAPI specification. - API commit: c82adf6 - Generated on: 2026-04-25T22:16:06Z These types are automatically generated using openapi-typescript. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11689-chore-Update-Comfy-Registry-API-types-from-comfy-api-c82adf6-34f6d73d3650815881e4dae4bdf05696) by [Unito](https://www.unito.io) Co-authored-by: coderfromthenorth93 <213232275+coderfromthenorth93@users.noreply.github.com> |
||
|
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> |
||
|
Christian Byrne
|
eb8f8b75b5 |
fix: correct zh-CN translations across landing, product, and pricing pages (#11655)
*PR Created by the Glary-Bot Agent* --- ## Summary Applies translation corrections to the Chinese (zh-CN) version of the website, requested in the `#project-new-website-brand-refresh` Slack thread by a native-speaker reviewer. ### Changes (in `apps/website/src/i18n/translations.ts` + `apps/website/src/pages/zh-CN/index.astro`) - **Landing — blog section badges**: `如何` → `了解`, `运作` → `运行方式` - **Landing — hero headline**: `专业控制` → `最强可控性` (also updates the zh-CN page `<title>`) - **Landing — hero subtitle**: `Comfy 是面向视觉专业人士的 AI 创作引擎，让您掌控每个模型、每个参数和每个输出。` → `Comfy 是面向专业视觉人士的 AI 创作引擎。您可以精确掌控每个模型、每个参数和每个输出。` - **Landing — showcase subtitle**: `从社区模板开始，或从零构建。` → `从工作流模板开始，或从零构建。` - **Landing — showcase feature1 title**: `节点式完全控制` → `节点带来的可控性` - **Landing — use case body**: `由 60,000+ 节点、数千个工作流 和一个比任何公司都更快构建的社区驱动。` → `60,000+ 节点，数千条工作流，一个比任何公司速度都更快的社区。` - **All `查看xx特性` CTAs** (local / cloud / API / enterprise product cards): `特性` → `属性` - **All `合作节点` pricing copy**: `合作节点` → `合作伙伴节点` (correct translation for "partner node") ### Verification - `pnpm typecheck` (astro check): 0 errors, 0 warnings, 0 hints - `pnpm build`: completes successfully; rebuilt zh-CN pages contain the new strings and no longer contain the old ones - Manual Playwright verification on `/zh-CN/` (hero, showcase badges, showcase feature card, use-case body, product cards) and `/zh-CN/cloud/pricing/` (`合作伙伴节点` appears 4×, `合作节点` appears 0×) ### Notes on review feedback A review pass flagged two of the swaps (`特性→属性` and `从社区模板→从工作流模板`) as potential style concerns. Both changes are kept as-is because they were specified verbatim by the native-speaker reviewer who requested this pass. ## Screenshots     ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11655-fix-correct-zh-CN-translations-across-landing-product-and-pricing-pages-34e6d73d3650816daddde4a90fb47a01) by [Unito](https://www.unito.io) Co-authored-by: Glary-Bot <glary-bot@users.noreply.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>
|
||
|
Dante
|
25f493bd30 |
test(assets): add E2E spec for sort options (#11634)
## Summary Adds 6 @cloud-tagged Playwright tests covering the assets sidebar sort menu (newest/oldest/longest/fastest). Phase 4 of the assets-test plan. **Stacked on #11632** — base will retarget to \`main\` once the foundation PR merges. Independent of #11633 (filter E2E) and can be reviewed in parallel. ## Changes - **What**: New file `browser_tests/tests/sidebar/assets-sort.spec.ts`. Uses the `createJobsWithExecutionTimes()` factory and the `sortLongestFirst` / `sortFastestFirst` locators added in #11632. - **Coverage**: - Settings menu exposes all four sort options in cloud mode - Default order is newest first (descending `create_time`) - "Oldest first" reverses the order - "Longest first" puts the slowest execution at the top - "Fastest first" puts the quickest execution at the top - Sort persists across search-input edits - **Breaking**: none ## Review Focus - **Misaligned (create_time, duration) axes**: fixture data is deliberately constructed so newest/oldest and longest/fastest produce distinct orderings — no test can false-pass by satisfying a different sort. See the table comment at the top of the spec. - **`@cloud` tag is required**: sort options are gated behind `:show-sort-options="isCloud"`, which depends on the compile-time `__DISTRIBUTION__` flag. Tests run only against the `cloud` Playwright project. - **Local verification needed**: maintainer should verify with `pnpm dev:cloud` + `pnpm test:browser:local --project cloud --grep "sort options"` before merging — I could not run the cloud dev server end-to-end in my environment. Fixes #10779 ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11634-test-assets-add-E2E-spec-for-sort-options-34e6d73d365081a79facde5bde2e18c6) by [Unito](https://www.unito.io) |
