mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-05-25 07:05:26 +00:00
9ef78eea0c4a2f1fa3cbeaa6f25928d7669c586b
606 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
bymyself
|
9ef78eea0c |
Consolidate image upload implementations into shared service
Creates core uploadService to eliminate ~60-70 LOC duplication across multiple implementations. Changes: - Add src/platform/assets/services/uploadService.ts with uploadMedia() and uploadMediaBatch() - Refactor Load3dUtils to use uploadService (eliminates 50+ LOC) - Refactor WidgetSelectDropdown to use uploadService (eliminates 20+ LOC) - Add comprehensive unit tests for uploadService - Maintain backward compatibility for all existing APIs Benefits: - Single source of truth for upload logic - Consistent error handling - Type-safe interfaces - Easier to test and maintain |
||
|
jaeone94
|
04918360eb |
Use hash lookup for missing asset detection (#11873)
## Summary
Use exact BLAKE3 hash lookups first for missing model/media detection,
and add a separate public-inclusive input asset cache so public input
assets are considered missing-detection candidates without changing the
user-only input assets shown in the UI.
## Changes
- **What**:
- Added `assetService.checkAssetHash()` for `HEAD
/api/assets/hash/{hash}` status-only existence checks.
- Added strict BLAKE3 hash helpers so only `blake3:<64 hex>` media
values and raw 64-hex BLAKE3 model metadata are sent to the hash
endpoint.
- Updated missing media detection to group BLAKE3 candidates by hash,
resolve them through the hash endpoint, and fall back to the legacy
asset list path for invalid/unverifiable/non-hash values.
- Updated missing model detection to use hash lookup for BLAKE3-backed
asset-supported candidates before falling back to the existing node-type
asset matching path.
- Added `assetService.getInputAssetsIncludingPublic()` backed by a
dedicated cache that fetches input assets with `include_public=true` for
missing media fallback checks.
- Kept `assetsStore.inputAssets` user-only for widget/UI display, while
invalidating the public-inclusive missing-detection cache when input
assets may change.
- Added abort handling for paginated asset fetches and shared
public-input cache callers so one aborted caller does not cancel the
shared fetch for other callers.
- Added regression coverage for hash lookup, fallback behavior, abort
paths, public input fallback detection, and cache invalidation.
- **Dependencies**: None.
- **Change size**:
- Production code: 4 files, 400 insertions, 24 deletions, net +376.
- Test code: 4 files, 806 insertions, 59 deletions, net +747.
- Total: 8 files, 1206 insertions, 83 deletions, net +1123.
## Review Focus
- The public-inclusive input asset cache is intentionally separate from
`assetsStore.inputAssets`. The existing store data is user-only and
drives the asset widgets/sidebar, so using it for missing input
detection misses public assets. Making that store public-inclusive would
change UI data semantics; this PR instead keeps the UI dataset unchanged
and adds a missing-detection-specific cache in `assetService`.
- Hash lookup is only used when the workflow exposes a valid BLAKE3
hash. Filename-like values and invalid hash values still use the legacy
fallback path.
- Missing model detection keeps the existing fallback behavior for
non-hash candidates and for hash checks that are invalid or fail
transiently.
- Async model download cache refresh behavior is left unchanged; this PR
avoids coupling model download completion to input asset cache
invalidation.
- No browser/e2e test was added because this changes the missing asset
detection data path, not UI interaction or rendering. The behavioral
coverage is in unit tests for the asset service and the missing
media/model scanners.
## Follow-up Items
- Fix `assetsStore.updateAssetTags()` partial-failure recovery. If
`removeAssetTags()` succeeds and `addAssetTags()` fails, the local model
asset cache can roll back to tags that the backend has already removed;
this should be handled in a focused model asset cache PR.
- Consider extracting shared hash-verification flow used by missing
media and missing model scans after this behavior stabilizes.
- Consider adding a concurrency cap or short-lived request cache for
large workflows with many unique hash lookups.
- Consider splitting `assetService.ts` further, e.g. hash helpers, abort
utilities, and the public-inclusive input asset cache.
- Consider tightening the asset hash service API shape so callers do not
directly depend on HTTP-oriented statuses such as `invalid`.
- Consider adding broader mutation-path coverage for public-inclusive
input cache invalidation once the cache has more consumers.
Linear: FE-534
## Screenshots (if applicable)
Before <false positive / missing image / public asset>
https://github.com/user-attachments/assets/db7ce2a9-b169-4fae-bf9f-98bb93d3ee6d
After
https://github.com/user-attachments/assets/29af9f9e-b536-4fcd-a426-3add40bcb165
┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-11873-Use-hash-lookup-for-missing-asset-detection-3556d73d36508165babafb16614be0d8)
by [Unito](https://www.unito.io)
|
||
|
Christian Byrne
|
5cad2c952b |
refactor+test: extract useSubscriptionCheckout composable, rewrite tests (#11396)
## Summary Adds 20 component tests for `SubscriptionRequiredDialogContentWorkspace.vue` covering: - **Initial rendering**: pricing table display, close/back button visibility, out_of_credits reason message - **Close button**: calls onClose callback - **Subscribe click flow**: pricing→preview transitions (new subscription & upgrade), error toasts for disallowed/missing/failed previews, monthly billing cycle - **Back button**: returns from preview to pricing step - **Add credit card**: handles subscribed status (success toast + close), needs_payment_method (opens Stripe URL), error state - **Confirm transition**: success path with close emit, error toast on failure - **Resubscribe**: success path with toast + close, error toast on failure ## Testing ```bash pnpm test:unit -- src/platform/workspace/components/SubscriptionRequiredDialogContentWorkspace.test.ts ``` All 20 tests pass. Quality gates (typecheck, lint, format, knip) pass. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11396-test-add-component-tests-for-SubscriptionRequiredDialogContentWorkspace-3476d73d36508156a218dcb67a2a334e) by [Unito](https://www.unito.io) |
||
|
Robin Huang
|
96575fcec9 |
feat: redesign cloud onboarding survey for ICP and persona signal (#11628)
## Summary Replaces the 4-step Cloud onboarding survey with a 7-step flow that captures both ICP attributes and user persona dimensions. The survey questions are now populated dynamically from remoteConfig. ## Changes - **What**: New survey questions — Usage, Familiarity, Role, Team size, Industry, Making, Source. Role / Team size / Industry are gated to "Work" usage; Education users see a Student / Educator short list for Role. Most option lists are randomized per visit (familiarity and team size stay ordered as ordinals). \`SurveyResponses\` extended with optional \`usage\`, \`role\`, \`teamSize\`, \`source\` fields. - **Breaking**: None — \`useCase\` and \`workflowRelationship\` remain optional in the type and existing telemetry normalization keeps working unchanged. ## Review Focus - The \`role\` step has a function-form \`options\` so the list can swap based on \`usage\`. \`steps\` is a computed that filters by \`showWhen()\` and resolves the option function — verify reactivity when \`usage\` changes. - Changing \`usage\` clears the previously-picked \`role\` via a watcher to prevent a stale value from carrying over between Work / Education modes. - Per-visit shuffle is stable: option lists are passed through \`randomize()\` once at module load, not on every render. ## Screenshots https://github.com/user-attachments/assets/3602a388-50dc-401e-ada9-ea9016c5052d ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11628-feat-redesign-cloud-onboarding-survey-for-ICP-and-persona-signal-34d6d73d365081f4a792cfe76a987ffb) by [Unito](https://www.unito.io) --------- Co-authored-by: Dante <bunggl@naver.com> |
||
|
Dante
|
4fff0c4b49 |
fix: report total file count, not job count, in ZIP export toast (#11737)
## Summary ZIP export toast now reports the total number of files instead of the number of selected jobs when any selected job has multiple outputs. ## Changes - **What**: In `downloadMultipleAssetsAsZip` (`src/platform/assets/composables/useMediaAssetActions.ts`), compute `fileCount` by summing each asset's `outputCount` metadata (fallback 1) and pass it to `mediaAsset.selection.exportStarted` instead of `assets.length`. The existing i18n string already handles `file`/`files` plural. - **Tests**: 3 new unit tests in `useMediaAssetActions.test.ts` covering multi-output, single-output fallback, and mixed selections. The `useToast` and `useI18n` mocks were lifted to hoisted refs so toast call args are assertable. ## Review Focus - Reduce uses `count > 1 ? count : 1`, mirroring the `hasMultiOutputJobs` gate above so a known `outputCount === 1` is still counted as 1 file (no double-counting). - Only `downloadMultipleAssetsAsZip` is touched; OSS individual-download path and direct-download path are unchanged. Fixes #11736 ┆Issue is synchronized with this [Notion page](https://app.notion.com/p/PR-11737-fix-report-total-file-count-not-job-count-in-ZIP-export-toast-3516d73d3650811ba78dfdb0a0ae8ea1) by [Unito](https://www.unito.io) --------- Co-authored-by: Alexander Brown <drjkl@comfy.org> |
||
|
jaeone94
|
11432f7d0e |
refactor: extract missing model refresh pipeline (#11751)
## Summary Extracts the missing-model pipeline orchestration out of `ComfyApp` and into an app-independent platform module, while tightening the workflow-flattening type boundary that refresh needs when rescanning the live LiteGraph graph. This PR is intentionally refactor-heavy. It is the follow-up to the earlier missing-model refresh work: instead of keeping refresh-specific candidate recheck logic beside the UI, this change makes the refresh path reuse the existing missing-model pipeline and removes the direct dependency on private `ComfyApp` pipeline methods. Linear: FE-499 Issues covered by this PR: - Fixes #11678 - Fixes #11680 - Partially addresses #11679 by removing the missing-model refresh path's unsafe `graph.serialize() as unknown as ComfyWorkflowJSON` cast and replacing it with the narrower flattenable workflow contract. Broader workflow serialization/type-boundary cleanup outside this missing-model refresh path remains deferred. ## Changes - **What**: - Added `src/platform/missingModel/missingModelPipeline.ts` as the orchestration module for missing-model detection/verification. - `runMissingModelPipeline(...)` now owns the pipeline previously embedded in `ComfyApp`: - candidate scan and enrichment - active ancestor filtering for muted/bypassed subgraph containers - pending warning cache updates - OSS folder path and file-size follow-up work - cloud asset verification follow-up work - surfaced missing-model errors via the existing execution error store - `refreshMissingModelPipeline(...)` handles the refresh-specific flow: - calls the injected `reloadNodeDefs()` first - serializes the current live graph - preserves model metadata by preferring active workflow `models`, then falling back to current missing-model candidate metadata - delegates back into the same pipeline used during workflow load - Kept `ComfyApp` as the compatibility caller instead of the owner of the pipeline. - `loadGraphData(...)` now calls `runMissingModelPipeline(...)` with `graph`, `graphData`, `missingNodeTypes`, and `silent` options. - `refreshMissingModels(...)` is now a thin wrapper around `refreshMissingModelPipeline(...)` and keeps the existing default `silent: true` refresh behavior. - The new pipeline module does not import `@/scripts/app`; app-owned data/actions are passed in as inputs. - Moved the workflow node-flattening helpers out of `workflowSchema.ts` and into `src/platform/workflow/core/utils/workflowFlattening.ts`. - This includes `flattenWorkflowNodes`, `buildSubgraphExecutionPaths`, and `isSubgraphDefinition`. - The move is intentional: these helpers are not zod schema definitions or workflow validation logic. They are core workflow traversal utilities used to flatten root workflow nodes plus nested subgraph definition nodes into the execution-shaped node list needed by missing-model scanning. - The refresh path receives data from `LGraph.serialize()`, whose return type is serialized LiteGraph data rather than validated `ComfyWorkflowJSON`. Previously this forced unsafe typing like `graph.serialize() as unknown as ComfyWorkflowJSON`. - The new `FlattenableWorkflowGraph` / `FlattenableWorkflowNode` structural contract describes only what flattening actually needs: `nodes`, `definitions.subgraphs`, node `id`, `type`, `mode`, `widgets_values`, and `properties`. - This lets both normal workflow-load data (`ComfyWorkflowJSON`) and refresh-time live graph serialization (`LGraph.serialize()`) flow into the same scan/enrichment path without pretending serialized LiteGraph output is a fully validated workflow schema document. - Updated `missingModelScan.ts` to consume that minimal flattenable workflow shape via `MissingModelWorkflowData`. - `MissingModelWorkflowData` extends the flattenable workflow contract with optional workflow-level `models` metadata. - Removed now-unnecessary casts around execution IDs, flattened nodes, and `widgets_values` object access. - Updated `getSelectedModelsMetadata(...)` to accept readonly widget value arrays so flattened workflow data can stay read-only. - Reduced the exported surface of the new pipeline module after `knip` flagged unused exported internal option/store interfaces. - Kept `workflowSchema.ts` focused on validation schemas. The flattening helpers are not re-exported from the schema module because they are internal workflow core utilities, not public schema API. - **Breaking**: None intended. - Internal imports were updated to the new core utility path. - This repo is not exposing these flattening helpers as a public package API, so the old schema-local helper location is treated as an internal implementation detail. - **Dependencies**: None. ## Review Focus - **Pipeline extraction / dependency direction**: - Please verify that `missingModelPipeline.ts` stays independent from `@/scripts/app`. - `ComfyApp` should remain the caller/adapter, not the owner of missing-model pipeline orchestration. - **Workflow flattening type boundary**: - The main type-cleanup goal is removing the refresh-time `graph.serialize() as unknown as ComfyWorkflowJSON` lie. - `LGraph.serialize()` and validated workflow JSON are not the same contract. The new flattenable workflow contract is deliberately smaller and structural because the missing-model enrichment path only needs enough data to flatten nodes and read embedded model metadata. - This is why the flattening helpers moved from `workflowSchema.ts` to `workflow/core/utils`: the logic is reusable workflow traversal, not validation schema. - **Behavior preservation**: - The PR is intended to preserve existing user-facing missing-model behavior while moving ownership out of `app.ts`. - Existing async follow-up behavior remains intentionally fire-and-forget: - cloud asset verification still surfaces after verification completes - OSS folder paths still update asynchronously before surfacing confirmed missing models - file-size metadata fetching remains asynchronous - More invasive behavior changes, such as adding non-cloud post-fetch `isMissingCandidateActive(...)` re-verification or redesigning the fire-and-forget result contract, are intentionally left for follow-up work because they are not pure extraction. - **Downloadable model metadata**: - `missingModels` returned for download metadata now requires both `url` and `directory`. - Candidates without a directory still remain in `confirmedCandidates`, but they are not exposed as downloadable model metadata. This keeps the returned downloadable list aligned with what the download flow can actually use. - **Test ownership**: - Complex missing-model pipeline behavior tests moved out of `src/scripts/app.test.ts` and into `src/platform/missingModel/missingModelPipeline.test.ts`. - `app.test.ts` now only covers thin delegation for `app.refreshMissingModels(...)`. - Workflow flattening tests moved with the helper from schema tests into `src/platform/workflow/core/utils/workflowFlattening.test.ts`. - **Deferred follow-ups**: - Broader function decomposition for cognitive complexity. - Wider dependency-injection/port cleanup for stores and services beyond the app boundary. - Cloud-specific pipeline unit tests, which need a separate `isCloud` mocking strategy. - Additional E2E coverage expansion beyond the existing OSS refresh path. - More general workflow serialization/type-boundary cleanup outside the missing-model refresh path. ## Validation - `pnpm format` - `pnpm lint` - Passed. Existing lint output included a pre-existing `no-misused-spread` warning and icon-name logs, but the command exited successfully. - `pnpm typecheck` - `pnpm test:unit` - `714 passed`, `9514 passed | 8 skipped` - Pre-push `pnpm knip` - Passed after reducing the exported surface of the new pipeline module. ## Screenshots (if applicable) Not applicable. This PR is a pipeline/type-boundary refactor with no UI changes. ┆Issue is synchronized with this [Notion page](https://app.notion.com/p/PR-11751-refactor-extract-missing-model-refresh-pipeline-3516d73d3650816d9245d4b1324b71c9) by [Unito](https://www.unito.io) --------- Co-authored-by: DrJKL <DrJKL0424@gmail.com> Co-authored-by: Alexander Brown <drjkl@comfy.org> |
||
|
Dante
|
9df4e02189 |
refactor: unify media asset downloads (#11717)
## Summary Unifies media asset download actions behind a single `downloadAssets(assets?)` API to avoid single and multi asset download path drift. ## Changes - **What**: Replaces `downloadAsset` and `downloadMultipleAssets` with `downloadAssets`, preserving no-arg media context fallback and explicit asset arrays. - **Dependencies**: None. ## Review Focus Download behavior for single-card, context-menu, and sidebar bulk actions should continue to use the same ZIP-export path for cloud multi-output jobs. Fixes #11715 ## Screenshots (if applicable) N/A ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11717-refactor-unify-media-asset-downloads-3506d73d3650810d8bcec9c0194e743d) by [Unito](https://www.unito.io) --------- Co-authored-by: Alexander Brown <drjkl@comfy.org> |
||
|
AustinMroz
|
1c541d8577 |
Short circuit asset reuploads, simplify node dnd (#11691)
When an output is dragged from the assets panel onto a node, outputs were being reuploaded. This logic has been simplified to instead reference the existing asset by resolving the annotated path. As part of this change, async drop handlers on nodes are also fixed. Rather than placing obligation of event handling on client code, not respecting async handlers, or completely ignoring return types, the vue drop handler will now simply set `app.dragOverNode` and allow the `document` drop handler to resolve node drag/drop operations without any of the difficulty from propagation. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11691-Short-circuit-asset-reuploads-simplify-node-dnd-34f6d73d36508157af86e6cf09229781) by [Unito](https://www.unito.io) --------- Co-authored-by: Alexander Brown <drjkl@comfy.org> |
||
|
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
|
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)
|
||
|
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>
|
||
|
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)
|
||
|
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> |
||
|
Benjamin Lu
|
3ea75e1c48 |
fix: localize secret date labels (#11524)
## Summary
Localizes secret list date labels through Vue I18n date formatting
instead of the browser default numeric date format, with Storybook
coverage for design review.
## Changes
- **What**: Replaces `toLocaleDateString()` with `useI18n().d(..., {
dateStyle: 'medium' })` for `SecretListItem` dates.
- **What**: Updates `SecretListItem` expectations to match the Vue I18n
date formatter.
- **What**: Adds `SecretListItem` stories for default, never-used,
loading, and disabled states.
- **Dependencies**: None.
## Review Focus
Stacked on #11480 to keep the escaping fix scoped. Please confirm
whether the localized medium date style matches design/product
expectations.
## Screenshots (if applicable)
https://f3ba7229.comfy-storybook.pages.dev/?path=/story/platform-secrets-secretlistitem--default
┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-11524-fix-localize-secret-date-labels-3496d73d3650814bb70bfb3f870a31cd)
by [Unito](https://www.unito.io)
|
||
|
Benjamin Lu
|
7ee667c1d1 |
fix: avoid escaped secret date labels (#11480)
Global i18n config has escapeParameter as true. This explicitly turns it
to false. I opened a Linear ticket to reconsider changing this back to
false as default globally.
## Summary
Fix the Secrets panel so created and last-used dates render as plain
text instead of HTML-escaped slash entities.
## Changes
- **What**: Compute the Secrets date labels with `t(..., {
escapeParameter: false })` after formatting the date, so vue-i18n does
not escape `/` into `/` for plain-text output.
- **What**: Replace the mocked translation setup in
`SecretListItem.test.ts` with a real `vue-i18n` instance and add a
regression test that asserts the rendered dates do not contain escaped
slash entities.
## Review Focus
This intentionally fixes the i18n interpolation issue shown in the bug
screenshot. It does not change the separate RFC3339Nano parsing behavior
discussed in #11358.
┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-11480-fix-avoid-escaped-secret-date-labels-3486d73d365081c890ecd2a6992d7879)
by [Unito](https://www.unito.io)
---------
Co-authored-by: GitHub Action <action@github.com>
|
||
|
Benjamin Lu
|
2b010ac8b3 |
fix: dedupe pending checkout attempt construction (#11622)
## Summary Deduplicates pending subscription checkout attempt construction so storage and fallback paths share the same payload creation. ## Changes - **What**: Build the `PendingSubscriptionCheckoutAttempt` once in `recordPendingSubscriptionCheckoutAttempt()` and reuse it for unavailable-storage, failed-write, and successful-write paths. - **Dependencies**: None. ## Review Focus This is intended as a no-behavior-change cleanup: unavailable storage still returns an attempt, failed `setItem()` still returns that attempt without dispatching, and the pending-checkout event only fires after a successful storage write. Linear: FE-209 ## Screenshots (if applicable) N/A --------- Co-authored-by: GitHub Action <action@github.com> |
||
|
jaeone94
|
b4d209b5f6 |
feat: refresh missing models through pipeline (#11661)
## Summary Follow-up to the closed earlier attempt in #11646. This PR keeps the same user-facing goal, but changes the implementation to reuse the existing missing model pipeline for refresh instead of maintaining a separate candidate-only recheck path. Adds a missing model refresh action in the Errors tab by reusing the existing missing model pipeline, so users can re-check models after downloading or manually placing files without reloading the workflow. ## Changes - **What**: - Adds `app.refreshMissingModels()` as a reusable refresh entry point for the current root graph. - Splits node definition reloading into `app.reloadNodeDefs()` so missing-model refresh can pull fresh `object_info` without showing the generic combo refresh success flow. - Reuses the existing missing model pipeline instead of adding a separate candidate-only checker. The refresh path serializes the current graph, reuses active workflow model metadata when available, falls back to current missing-model metadata, and then reruns the same candidate discovery/enrichment/surfacing flow used during workflow load. - Adds missing model refresh state and error handling to `missingModelStore`. - Adds a Refresh button next to Download all in the missing model card action bar. - Moves Download all from the Errors tab header into the missing model card, so the Download all and Refresh actions render or hide together. - Changes Download all visibility from “more than one downloadable model” to “at least one downloadable model.” - Keeps the action bar hidden when there are no downloadable missing models; Cloud still does not render this action area. - Normalizes active workflow `pendingWarnings` updates so resolved missing model warnings do not get revived by stale empty warning objects. - Adds test IDs and coverage for the new action bar, refresh state, refresh delegation, pending warning sync, and E2E refresh behavior. - **Breaking**: None. - **Dependencies**: None. ## Review Focus The main design choice is intentionally reusing the missing model pipeline for refresh instead of implementing a smaller candidate-only recheck. The earlier candidate-only approach was cheaper, but it created a separate source of truth for missing-model resolution and made edge cases harder to reason about. In particular, it could diverge from the behavior used when a workflow is loaded, and it did not naturally handle the case where a model becomes missing after the workflow is already open. This version pays the cost of refreshing node definitions and rerunning the missing-model scan for the current graph, but keeps the refresh behavior aligned with workflow load semantics. Expected behavior by environment: - OSS browser: - The action bar appears when at least one missing model has a downloadable URL and directory. - Download all uses the existing browser download path. - Refresh reloads `object_info`, refreshes node definitions/combo values, reruns missing-model detection for the current graph, and clears the error if the selected model is now available. - OSS desktop: - The same action bar appears under the same downloadable-model condition. - Download all uses the existing Electron DownloadManager path. - Refresh uses the same missing-model pipeline as browser, so manually placed files or desktop-downloaded files can be rechecked without reloading the workflow. - Cloud: - The action bar remains hidden because model download/import is not supported in this section for Cloud. A few boundaries are intentional: - This PR does not add automatic filesystem watching. Browser OSS cannot reliably observe local model folder changes, so the user-triggered Refresh button remains the cross-environment mechanism. - This PR does not redesign the public `refreshComboInNodes` API beyond extracting `reloadNodeDefs()` for reuse. Further cleanup of toast behavior or a more explicit object-info reload API can be follow-up work. - This PR keeps refresh scoped to missing-model validation; missing media and missing nodes continue to use their existing flows. Linear: FE-417 ## Screenshots (if applicable) https://github.com/user-attachments/assets/2e02799f-1374-4377-b7b3-172241517772 ## Validation - `pnpm format` - `pnpm lint` (passes; existing unrelated warning remains in `src/platform/workspace/composables/useWorkspaceBilling.test.ts`) - `pnpm typecheck` - `pnpm test:unit` - `pnpm test:browser:local -- --project=chromium browser_tests/tests/propertiesPanel/errorsTabMissingModels.spec.ts` - `pnpm build` - `NX_SKIP_NX_CACHE=true DISTRIBUTION=desktop USE_PROD_CONFIG=true NODE_OPTIONS='--max-old-space-size=8192' pnpm exec nx build` - Manual desktop verification through `~/Projects/desktop` after copying the desktop build into `assets/ComfyUI/web_custom_versions/desktop_app`: - confirmed the FE bundle is built with `DISTRIBUTION = "desktop"` - confirmed missing model Download uses the desktop download path instead of browser download - confirmed Refresh can clear the missing model error after the model is available - Push hook: `pnpm knip --cache` ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11661-feat-refresh-missing-models-through-pipeline-34f6d73d3650811488defee54a7a6667) by [Unito](https://www.unito.io) |
||
|
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) |
||
|
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
|
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) |
||
|
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
|
aa730c8cb5 |
test(assets): add unit tests and E2E fixture groundwork for sidebar filter & sort (#11632)
## Summary Lays the test foundation for the two open assets-testing issues — Phase 1 (fixtures + page object) and Phase 2 (unit tests). Phase 3/4 (the actual E2E specs) follow in stacked PRs. ## Changes - **What**: 27 new unit tests covering `useMediaAssetFiltering`, `MediaAssetFilterMenu`, and `MediaAssetSettingsMenu`; reusable Playwright fixture factories for diverse media kinds and execution-time specs; new locators + helpers on `AssetsSidebarTab` for the filter menu and longest/fastest sort options. No production code touched. - **Breaking**: none ### New files - `src/platform/assets/composables/useMediaAssetFiltering.test.ts` — 11 cases: single/multi-OR media-type filter, `'3D'` → `'3d'` filename normalization, exclusion of unsupported kinds, all four sort modes, `created_at` fallback when `user_metadata.create_time` is absent, filter+sort composition. - `src/platform/assets/components/MediaAssetFilterMenu.test.ts` — 6 cases: checkbox rendering, prop-driven `aria-checked`, click toggling (add/remove/append), keyboard activation (Enter/Space). - `src/platform/assets/components/MediaAssetSettingsMenu.test.ts` — 10 cases: view-mode v-model, `showSortOptions` and `showGenerationTimeSort` visibility gates, `v-model:sortBy` round-trip for newest/oldest/longest/fastest. ### Extended files - `browser_tests/fixtures/helpers/AssetsHelper.ts` — added `MediaKindFixture` type, optional `mediaKind` shorthand on `createMockJob` (sets both filename extension and `preview_output.mediaType`), plus `createMixedMediaJobs(kinds)` and `createJobsWithExecutionTimes(specs)` factories for unambiguous filter/sort assertions. - `browser_tests/fixtures/components/SidebarTab.ts` — added `filterButton`, per-type checkbox locators (`filterImage/Video/Audio/3DCheckbox`), `sortLongestFirst`, `sortFastestFirst`, plus `openFilterMenu()`, `filterCheckbox(kind)`, `toggleMediaTypeFilter(kind)`, and `getAssetCardOrder()` helpers. ## Review Focus - **Naming of `MediaKindFixture` values** — `'images'` is plural to match existing API conventions emitted by the backend / `useMediaAssetGalleryStore`; `'video' | 'audio' | '3D'` follow the singular `MediaKind` type. Open to renaming if a unified shape is preferred. - **Filter button locator strategy** — `MediaAssetFilterButton` has no `aria-label`, so the page object targets it via the `icon-[lucide--list-filter]` class. Happy to add a `data-testid` or `aria-label` to the source component if reviewers prefer a more durable hook (would be a one-line source change in a follow-up). ## Follow-up PRs - Phase 3 (E2E for media-type filter) → closes #10780 - Phase 4 (E2E for asset sort) → closes #10779 Both are stacked on this branch and can be reviewed/merged in either order once this lands. References #10779, #10780. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11632-test-assets-add-unit-tests-and-E2E-fixture-groundwork-for-sidebar-filter-sort-34e6d73d3650815c9900e5fd7cc7eab0) by [Unito](https://www.unito.io) |
||
|
jaeone94
|
17d980dbc8 |
feat: add inline-CTA nightly survey for error panel (#11591)
## Summary Adds an inline-CTA Typeform survey for the redesigned error panel, targeting nightly localhost users. Reuses the existing node-search survey infrastructure rather than introducing a parallel stack. ## Changes - **What**: - `surveyRegistry` gains optional `presentation: 'floating' | 'inline-cta'` and a `getFloatingSurveys()` helper; controller filters by it. - `NightlySurveyPopover` accepts `mode` + `v-model:open`. Manual mode skips the eligibility watcher, drops the "Not Now" button, and leaves open/close/markSeen to the parent. - New `ErrorPanelSurveyCta.vue` renders a CTA in the error tab footer once `useExecutionErrorStore.hasAnyError` has transitioned `null → value` at least 3 times. - Popover lives in `NightlySurveyController` (session-persistent). Shared state via module-level singleton (`useErrorSurveyPopoverState`) so the iframe survives error-tab unmounts during workflow switches. - `useTypeformEmbed` centralises script loading (singleton Promise, 10s timeout, explicit `window.tf.load()` on each new container). Necessary because the embed's DOMContentLoaded auto-scan only fires once; late consumers need an explicit re-scan to render. - CTA and feedback-gate strings added under `errorPanelSurvey.*`. ## Review Focus - Manual-mode flow in `NightlySurveyPopover.vue` — CTA click is routed through the module singleton; popover stays mounted after the first open (`hasOpenedOnce` + `v-show`) to preserve the iframe across repeated open/close cycles and workflow switches. - `useTypeformEmbed.ts` — the `window.tf.load()` trick (verified against the CDN `embed.js`) is what lets two surveys coexist; without it Typeform's one-shot DOM scan misses whichever element mounts second. - `NightlySurveyController.vue` guards against double-mount by requiring `presentation === 'inline-cta'` on `errorPanelConfig`. - Scope is nightly+localhost only (`isNightlyLocalhost` in `useSurveyEligibility`); async component gate in `TabErrors.vue` keeps this out of Cloud/Desktop/stable bundles. ## Test plan - [x] `IS_NIGHTLY=true pnpm dev`, clear `Comfy.SurveyState` + `Comfy.FeatureUsage`, trigger 3 failed runs → CTA appears in error tab footer. - [x] Click "Give feedback" → Typeform popover opens and renders the form. - [x] Close popover, switch workflow (error tab unmounts), trigger a new error → CTA reappears and reopening the popover shows the same iframe. - [x] Open error-panel popover, close, then trigger 3 node searches → node-search auto-popup renders its own iframe correctly (two surveys coexist). - [x] CTA × dismisses and persists (`seenSurveys['error-panel']`). - [x] "Don't ask again" inside popover sets `optedOut: true` and hides all nightly surveys. - [x] Cloud/Desktop/stable builds: CTA never renders, controller's manual popover doesn't mount. ## Screenshot https://github.com/user-attachments/assets/91145f23-fd1e-4caf-b6cc-4b97d33ed6b7 ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11591-feat-add-inline-CTA-nightly-survey-for-error-panel-34c6d73d3650817d9f95fddcf64633de) by [Unito](https://www.unito.io) |
||
|
Benjamin Lu
|
04e430e006 |
fix: dedupe keybinding modifier display (#11570)
## Summary Fix keybinding display so pressing a modifier key by itself shows that modifier once instead of duplicated text like `Shift + Shift`. ## Changes - **What**: Centralizes modifier key labels in `KeyComboImpl` and omits the duplicated primary key when the pressed key is itself a modifier. - **Breaking**: None. - **Dependencies**: None. ## Review Focus The keybinding model still includes active modifiers for normal shortcuts like `Ctrl + Shift + k`, while modifier-only input now renders as a single key. Regression coverage includes single modifier presses, combined held modifiers, and a normal non-modifier shortcut. Checks run: `pnpm exec vitest run src/platform/keybindings/keyCombo.test.ts`; `pnpm lint:unstaged`; `pnpm exec oxfmt --check src/platform/keybindings/keyCombo.ts src/platform/keybindings/keyCombo.test.ts`; `pnpm exec vitest run src/platform/keybindings/keyCombo.test.ts src/platform/keybindings/keybindingStore.test.ts src/platform/keybindings/keybindingService.escape.test.ts src/platform/keybindings/keybindingService.canvas.test.ts`; `pnpm typecheck`; pre-commit lint-staged checks; pre-push `pnpm knip --cache`. Linear: FE-240 ## Screenshots (if applicable) Not applicable; covered by keybinding sequence unit tests. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11570-fix-dedupe-keybinding-modifier-display-34b6d73d365081968a88da4465c151de) by [Unito](https://www.unito.io) |
||
|
Christian Byrne
|
0f0210e482 |
test: add unit tests for workspaceApi (#11393)
## Summary Adds 27 unit tests for `src/platform/workspace/api/workspaceApi.ts`, increasing line coverage from 2.9% to 83.2%. ## Test Coverage - Authentication: auth header null checks (workspace auth + firebase auth) - Error handling: axios error wrapping, fallback messages, non-axios rethrow - Workspace CRUD: list, create, update, delete, leave - Member management: listMembers, removeMember - Invite management: listInvites, createInvite, revokeInvite, acceptInvite - Billing: getBillingStatus, getBillingBalance, getBillingPlans - Subscription: previewSubscribe, subscribe, cancelSubscription, resubscribe - Payment: getPaymentPortalUrl, createTopup - Billing events: getBillingEvents, getBillingOpStatus ## Testing ```bash pnpm vitest run src/platform/workspace/api/workspaceApi.test.ts # 27 tests pass ``` ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11393-test-add-unit-tests-for-workspaceApi-3476d73d36508192b8f6d1af6aa543f4) by [Unito](https://www.unito.io) --------- Co-authored-by: GitHub Action <action@github.com> |
||
|
Christian Byrne
|
ef59f46495 |
refactor: migrate cn imports from @/utils/tailwindUtil shim to @comfyorg/tailwind-utils directly (#11453)
*PR Created by the Glary-Bot Agent* --- ## Summary - Replace all `cn` / `ClassValue` imports from the `@/utils/tailwindUtil` re-export shim with direct imports from `@comfyorg/tailwind-utils` across 198 source files in `src/` and 3 in `apps/desktop-ui/` - Delete both shim files (`src/utils/tailwindUtil.ts` and `apps/desktop-ui/src/utils/tailwindUtil.ts`) - Add explicit `@comfyorg/tailwind-utils` dependency to `apps/desktop-ui/package.json` - Update documentation references in `AGENTS.md`, `docs/guidance/design-standards.md`, and `docs/guidance/vue-components.md` Fixes #11288 ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11453-refactor-migrate-cn-imports-from-utils-tailwindUtil-shim-to-comfyorg-tailwind-utils--3486d73d365081ec92cce91fbf88e6e4) by [Unito](https://www.unito.io) --------- Co-authored-by: Glary-Bot <glary-bot@users.noreply.github.com> Co-authored-by: GitHub Action <action@github.com> Co-authored-by: Alexander Brown <drjkl@comfy.org> |
||
|
Dante
|
bea72410fd |
test: add unit tests for utility widgets (#11442)
## Summary Adds 26 unit tests across 3 files covering BatchNavigation, FormSearchInput, and WidgetLayoutField. Part of a widget-test-coverage sequence. ## Changes - **What**: - \`BatchNavigation.test.ts\` (10) — hidden when count ≤ 1, counter formatted as 1-based \`current / total\`, prev/next navigation, disabled states at range boundaries. - \`FormSearchInput.test.ts\` (8) — v-model binding as the user types, clear-button visibility based on trimmed-query, debounced searcher invocation with fake timers (250ms debounce, 1000ms maxWait). - \`WidgetLayoutField.test.ts\` (8) — widget.name vs widget.label preference, empty-name suppression, \`HideLayoutFieldKey\` injection hides label but preserves slot, slot receives \`borderStyle\` scoped prop. ## Review Focus - Fake timers used in FormSearchInput tests for \`refDebounced\` — the debounce assertion depends on the 250ms/1000ms window in the component staying unchanged. - \`HideLayoutFieldKey\` provided via \`global.provide\` using the Symbol key. - No changes to any source component. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11442-test-add-unit-tests-for-utility-widgets-3486d73d365081a891cafe21b09b91c0) by [Unito](https://www.unito.io) --------- Co-authored-by: GitHub Action <action@github.com> Co-authored-by: bymyself <cbyrne@comfy.org> |
||
|
Dante
|
65b8a5652c |
fix: render dates in Secrets panel for timestamps with >3 fractional-second digits (#11358)
## Summary
Cloud Prod renders "Invalid Date" in Settings → Secrets on strict JS
Date parsers (older Safari, some WebViews) because the backend emits
timestamps with variable fractional-second precision (e.g.
`"2026-04-18T10:04:55.6513Z"` — 4 digits), which falls outside the
3-digit-only ECMA-262 grammar.
## Changes
- **What**:
- Add `parseIsoDateSafe()` in `src/utils/dateTimeUtil.ts` — trims the
fractional portion to millisecond precision before `new Date(...)` and
returns `null` for missing or unparseable input.
- `SecretListItem.vue` uses the helper and hides the Created / Last Used
line when the timestamp is invalid instead of rendering the literal
string "Invalid Date".
- Unit tests for the parser (8) and for the component (4-digit
fractional seconds, garbage input).
## Review Focus
- The backend (Go `time.RFC3339Nano`) strips trailing zeros from
fractional seconds, producing 0–9 digits depending on the value. Modern
V8 parses this leniently; older Safari does not. A durable fix is
server-side — emit exactly 3 fractional digits — and should be filed
separately. This PR is a defensive frontend guard that also protects ~10
other `toLocaleDateString` callsites if they migrate to the helper.
- Regex `(\.\d{3})\d+(?=Z|[+-]\d{2}:?\d{2}|$)` trims only when there are
**more than** 3 digits; shorter fractions and zero-fraction timestamps
are unchanged.
## Screenshots (if applicable)
Reported in Slack:
https://comfy-organization.slack.com/archives/C0A4XMHANP3/p1776443594202969
┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-11358-fix-render-dates-in-Secrets-panel-for-timestamps-with-3-fractional-second-digits-3466d73d3650813cb855cfbd50b3650b)
by [Unito](https://www.unito.io)
---------
Co-authored-by: Terry Jia <terryjia88@gmail.com>
|
||
|
Dante
|
2c772077e0 |
test: add E2E tests for billing dialogs (CancelSubscription, TopUpCredits) (#10969)
## Summary - Add Playwright E2E tests for `CancelSubscriptionDialogContent` and `TopUpCreditsDialogContentLegacy` - CancelSubscription tests: dialog display with date formatting, keep subscription dismiss, confirm cancel with mocked API, error handling on API failure - TopUpCredits tests: dialog display with preset amounts, insufficient credits variant, preset selection, close button dismiss, pricing link visibility Part of the FixIt Burndown test coverage initiative (Untested Dialogs). ## Test plan - [ ] Verify tests pass in CI against OSS build - [ ] `pnpm test:browser:local -- browser_tests/tests/dialogs/cancelSubscriptionDialog.spec.ts` - [ ] `pnpm test:browser:local -- browser_tests/tests/dialogs/topUpCreditsDialog.spec.ts` ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-10969-test-add-E2E-tests-for-billing-dialogs-CancelSubscription-TopUpCredits-33c6d73d36508164b268c08c99464ca1) by [Unito](https://www.unito.io) |
||
|
Kelly Yang
|
983789753e |
refactor: remove @ts-expect-error suppressions in test files (#11337)
## Summary
Part of #11092 — Phase 3: remove @ts-expect-error suppressions from test
files.
This phase targets 22 suppressions across two test files:
- `src/utils/nodeDefUtil.test.ts` (18)
- `src/platform/workflow/validation/schemas/workflowSchema.test.ts` (4)
## Changes
`nodeDefUtil.test.ts`: Each test already constrains the inputs to a
known subtype (`IntInputSpec`, `FloatInputSpec`, `ComboInputSpecV2`), so
casting result to the expected subtype at the declaration site is both
correct and self-documenting. For the one test that uses the base
`InputSpec` type, the options object is extracted with an inline
structural cast.
`workflowSchema.test.ts`: validateComfyWorkflow returns
ComfyWorkflowJSON | null. The tests were accessing .nodes[0].pos without
narrowing, causing "object is possibly null" errors. Fixed with explicit
expect(validatedWorkflow).not.toBeNull() assertions before each property
access, which also improves failure messages — previously a null result
would throw a TypeError rather than a readable assertion failure.
<!-- CURSOR_SUMMARY -->
---
> [!NOTE]
> **Low Risk**
> Test-only type-safety refactor with no runtime code changes; primary
risk is minor test assertion behavior changes if a helper unexpectedly
returns `null`.
>
> **Overview**
> Removes `@ts-expect-error` suppressions from two test suites by making
nullability and return-type expectations explicit.
>
> `workflowSchema.test.ts` now asserts `validateComfyWorkflow` results
are non-null before accessing `nodes[0]` fields, and
`nodeDefUtil.test.ts` casts `mergeInputSpec` results to the expected
spec subtype (or extracts typed options) so property assertions compile
cleanly under stricter TS settings.
>
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
|
||
|
AustinMroz
|
91ed6a37e2 |
Fix nodeReplacement not triggering onRemoved (#11509)
Node Replacement failed to call onRemoved on the old node. This would cause domWidgets to persist after a node is replaced. <img width="474" height="257" alt="image" src="https://github.com/user-attachments/assets/51641de7-81e9-4355-88d9-d1605f397076" /> ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11509-Fix-nodeReplacement-not-triggering-onRemoved-3496d73d365081e19a4ae252aa87172d) by [Unito](https://www.unito.io) |
||
|
Christian Byrne
|
63d0e3ae5d |
test: achieve 100% coverage on keybinding presetService (#11399)
## Summary Add 7 new unit tests to achieve 100% statement/branch/function/line coverage on `src/platform/keybindings/presetService.ts`. ## Changes - **What**: 7 new tests in `presetService.test.ts` covering previously-uncovered paths: importPreset JSON parse error, deletePreset cancel/non-active preset, applyPreset with unset bindings, switchPreset save-as-new flow (success and cancel), switchPreset to default after unsaved changes dialog. Cherry-picked source files from |
||
|
Christian Byrne
|
5d98e11ba1 |
feat: enable queue panel v2 by default on nightly builds (#11376)
*PR Created by the Glary-Bot Agent* --- ## Summary - Changes the `Comfy.Queue.QPOV2` setting's `defaultValue` from `false` to `isNightly` - On nightly builds, users get the docked job history/queue panel (v2) by default - On stable builds, behavior is unchanged (v1 floating overlay remains default) - Users can still toggle the setting manually regardless of build type ## Pattern Follows the existing pattern used by `Comfy.VueNodes.Enabled` which uses `isCloud || isDesktop` as its version-conditional default. This is a compile-time constant from `@/platform/distribution/types`. ## Context Part of a dual-variant audit to graduate experimental features. QPO v2 has 0 extension ecosystem dependencies (confirmed via GitHub codesearch), making nightly default-on safe for gathering feedback before promoting to all users. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11376-feat-enable-queue-panel-v2-by-default-on-nightly-builds-3466d73d36508140b814d1d684acacba) by [Unito](https://www.unito.io) Co-authored-by: Glary-Bot <glary-bot@users.noreply.github.com> |
||
|
Christian Byrne
|
60c7471818 |
feat: enable node replacement by default (#11439)
*PR Created by the Glary-Bot Agent* --- ## Summary Enable node replacement suggestions by default so users see Quick Fix options for deprecated/renamed nodes without toggling an experimental setting. - Change `Comfy.NodeReplacement.Enabled` default from `false` to `true` and remove `experimental` flag - Add `versionModified` metadata for release tracking - No breaking change — users who previously disabled this setting keep their preference ## Safety gates This is an intentional global rollout, gated by two additional server-side checks: 1. Server must provide `node_replacements` feature flag as true (PostHog controlled) 2. `GET /api/node_replacements` must return data (cloud PR Comfy-Org/cloud#2686) Without both, changing this default alone has no effect. The three gates ensure safe rollout. ## Companion PRs - Comfy-Org/cloud#2686 — backend `GET /api/node_replacements` endpoint + server-side validation bypass Replicate of #11246, retargeted to `main` for backport automation. Labels: `needs-backport`, `cloud/1.42`, `cloud/1.43`, `core/1.42`, `core/1.43` ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11439-feat-enable-node-replacement-by-default-3486d73d36508192b77aea9640986106) by [Unito](https://www.unito.io) Co-authored-by: Glary-Bot <glary-bot@users.noreply.github.com> |
||
|
Dante
|
799ffcf4b6 |
test: cover useWorkspaceUI and useWorkspaceBilling (#11319)
Closes coverage gaps in \`src/platform/workspace/composables/\` as part of the unit-test backfill. ## Testing focus \`useWorkspaceUI\` is wrapped in \`createSharedComposable\` (shared instance across all callers). \`useWorkspaceBilling\` is a large stateful composable: parallel API calls, exponential-backoff polling, computed mappers, lifecycle cleanup. Both need careful state isolation and real lifecycle behavior — not faked hooks. ### \`useWorkspaceUI\` (8 tests) - **Permission / UI-config matrix.** Three role/type combinations — (personal × any), (team × owner), (team × member) — plus the no-active-workspace default. Assertions target concrete flags that differ per role (the table itself is the contract), not the return shape. - **\`createSharedComposable\` identity invariant.** Multiple calls return the same instance. - **Isolation.** Each test uses \`vi.resetModules()\` to get a fresh shared instance so the memoization doesn't leak between cases. ### \`useWorkspaceBilling\` (23 tests) - **Parallel init.** \`initialize\` runs \`Promise.all([status, balance, plans])\` concurrently, then re-fetches balance when free-tier shows a zero amount (lazy credit grant path). - **Polling with fake timers.** \`cancelSubscription\`'s exponential backoff (\`1000 * 2^attempt\`, max 5000ms) driven by \`vi.useFakeTimers()\` + \`advanceTimersByTimeAsync()\`. Covers success, failure, and the unmount-stops-polling case. - **Real lifecycle.** \`onBeforeUnmount\` only fires inside a component instance — not inside a raw \`effectScope\`. The unmount test mounts a minimal Vue app via \`createApp\` / \`app.unmount\` so the production cleanup path actually runs. - **Computed getter mapping.** \`subscription\`, \`balance\`, \`isActiveSubscription\`, \`isFreeTier\` assert the snake_case API shape is remapped to the camelCase UI shape correctly. - **\`window\` effects.** \`window.open\` stubbed via \`vi.spyOn\`, \`window.location.href\` via \`vi.stubGlobal\`. Restored in \`afterEach\`. ## Principles applied - No mocks of \`vue\` or \`@vueuse/core\` — only our own workspace API, stores, and sibling composables. - Behavioral assertions only. - All 31 tests pass; typecheck/lint/format clean. Test-only; no production code touched. |
||
|
Hunter
|
4c35add5bc |
feat: add civitai.red hostname support (#11349)
*PR Created by the Glary-Bot Agent* --- ## Summary Civitai split its domain — NSFW content moved to `civitai.red` while `civitai.com` stays SFW. The frontend only recognized `civitai.com` URLs, causing the import button to silently reject `.red` links. This was the root cause of 8+ support tickets in 3 days. Companion to backend PR: https://github.com/Comfy-Org/cloud/pull/3259 ## Changes ### Import source recognition - **`civitaiImportSource.ts`**: Added `'civitai.red'` to `hostnames` array — this is the primary fix for "button doesn't recognize the links" ### Missing model auto-download - **`missingModelDownload.ts`**: Added `'https://civitai.red/'` to `ALLOWED_SOURCES` ### URL detection utilities - **`formatUtil.ts`**: `isCivitaiModelUrl()` now accepts `civitai.red` URLs with proper hostname validation - **`assetMetadataUtils.ts`**: `getSourceName()` returns "Civitai" for `.red` URLs ### Tests (4 files) - `useUploadModelWizard.test.ts`: Added civitai.red hostnames and URL test case - `missingModelDownload.test.ts`: Added civitai.red cases for `toBrowsableUrl` and `isModelDownloadable` - `assetMetadataUtils.test.ts`: Added civitai.red case for `getSourceName` - `useMissingModelInteractions.test.ts`: Updated mock hostnames - `formatUtil.test.ts`: Added civitai.red cases for `isCivitaiModelUrl` ## Not changed (intentionally) - `getAssetSourceUrl()` ARN fallback (line 88) — ARNs don't carry domain info, `civitai.com` is correct default - `fetchCivitaiMetadata()` API URL (line 109) — REST API works on both domains, keeping `civitai.com` Resolves BE-353 ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11349-feat-add-civitai-red-hostname-support-3456d73d3650810d9c62ef4ad95ae031) by [Unito](https://www.unito.io) --------- Co-authored-by: Glary-Bot <glary-bot@users.noreply.github.com> Co-authored-by: Christian Byrne <cbyrne@comfy.org> Co-authored-by: Terry Jia <terryjia88@gmail.com> |
||
|
Christian Byrne
|
a3893a593d |
refactor: move select components from input/ to ui/ component library (#11378)
*PR Created by the Glary-Bot Agent* --- ## Summary Reconciles `src/components/input/` (older select components) into `src/components/ui/` (internal component library), eliminating the separate `input/` directory entirely. ## Changes - **Move MultiSelect** → `src/components/ui/multi-select/MultiSelect.vue` - **Move SingleSelect** → `src/components/ui/single-select/SingleSelect.vue` - **Extract shared resources** → `src/components/ui/select/types.ts` (SelectOption type) and `src/components/ui/select/select.variants.ts` (CVA styling variants) - **Update 7 consuming files** to use new import paths - **Update 1 test file** (AssetFilterBar.test.ts mock paths) - **Move stories and tests** alongside their components - **Delete `src/components/input/`** directory ## Context The `input/` directory contained only MultiSelect and SingleSelect — two well-built components that already used the same stack as `ui/` (Reka UI, CVA, Tailwind 4, Composition API). MultiSelect even imported `ui/button/Button.vue`. Moving them into `ui/` removes the split and consolidates all reusable components in one place. No API changes — all component props, slots, events, and behavior are preserved exactly. ## Verification - `pnpm typecheck` ✅ - `pnpm build` ✅ - `pnpm lint` (stylelint + oxlint + eslint) ✅ - All 15 relevant tests pass (MultiSelect: 5, SingleSelect: 2, AssetFilterBar: 8) ✅ - `pnpm knip` — no dead exports ✅ - No stale `@/components/input/` references remain ✅ - Pre-commit hooks pass ✅ - Git detected all moves as renames (97-100% similarity) ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11378-refactor-move-select-components-from-input-to-ui-component-library-3476d73d3650810e99b4c3e0842e67f3) by [Unito](https://www.unito.io) Co-authored-by: Glary-Bot <glary-bot@users.noreply.github.com> |
||
|
Dante
|
7089a7d1a0 |
fix: show asset display names in bulk delete confirmation (#11321)
## Summary Bulk-delete confirmation on Comfy Cloud listed raw SHA-256 filenames, making the modal impossible to use to verify what would be deleted. ## Changes - **What**: `useMediaAssetActions.deleteAssets` now maps each asset through `getAssetDisplayName`, so the confirmation's `itemList` matches the user-assigned names shown in the left media panel (`MediaAssetCard`). - **Tests**: Added two regression tests covering `user_metadata.name` / `display_name` resolution and the `asset.name` fallback. ## Review Focus - Parity with `MediaAssetCard` display: we reuse the same `getAssetDisplayName` helper; extension stripping (via `getFilenameDetails`) is not applied in the modal since file extensions are useful context when confirming deletions. Reported in Slack: https://comfy-organization.slack.com/archives/C0A4XMHANP3/p1776383570015289 ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11321-fix-show-asset-display-names-in-bulk-delete-confirmation-3456d73d36508108a3d5f2290ca39e18) by [Unito](https://www.unito.io) |
||
|
Johnpaul Chiwetelu
|
beaa269a63 |
feat: polish settings dialog layout and keybinding display (#11241)
Polish keybinding display. Based on #11212 with adjustments: left-aligned content (no centering), key uppercase moved to UI layer. - Reduce settings content font size to 14px - Increase spacing between setting sections with cleaner dividers - Consistent min-height for form items (toggle, slider, dropdown) - Capitalize keybinding badges via CSS `uppercase` instead of mutating data model - Remove '+' separator between keybinding badges - Unbold keybinding badges with fixed min-width Supersedes #11212 ┆Issue is synchronized with this [Notion page](https://app.notion.com/p/PR-11241-feat-polish-settings-dialog-layout-and-keybinding-display-3426d73d3650812a97e4d941a76a4fe9) by [Unito](https://www.unito.io) --------- Co-authored-by: Alex <alex@Mac.localdomain> Co-authored-by: github-actions <github-actions@github.com> |
||
|
Benjamin Lu
|
ecb7fd4796 |
feat: add frontend subscription success recovery (#11286)
Improving our subscription detection system. Optimal will have to come after BE team brings personal billing to cloud repo off of comfy api. ## Summary - replace the dialog-local focus poller with a frontend checkout tracker stored in `localStorage` - recover pending subscription checkouts from app boot plus global page lifecycle (`pageshow`, `visibilitychange`) with bounded retries only while an attempt is pending - emit `subscription_success` through GTM with frontend-derived metadata once subscription state reaches the expected target tier/cycle ## Why This is the frontend-only 80/20 path. It fixes the brittle "old tab must regain focus" behavior without adding new backend endpoints or backend event storage. The browser records one pending checkout attempt when checkout is opened, and any returning cloud tab can recover it later by comparing current subscription state against the expected target plan. ## Tradeoffs - browser-scoped, not backend-authoritative - no server transaction id - scheduled downgrades through the billing portal are intentionally not inferred as immediate success events - still best-effort compared with the backend outbox/WebSocket approach ## Validation - `pnpm exec vitest run src/platform/cloud/subscription/composables/useSubscription.test.ts src/platform/telemetry/providers/cloud/GtmTelemetryProvider.test.ts` - `pnpm typecheck` ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11286-feat-add-frontend-subscription-success-recovery-3436d73d3650814d9f74c89e6926aa84) by [Unito](https://www.unito.io) |
||
|
Dante
|
7ffaff7e1b |
test: cover useBillingPlans and tierBenefits (#11318)
Closes coverage gaps in `src/platform/cloud/subscription/` as part of the unit-test backfill. ## Testing focus `useBillingPlans` holds **module-scoped refs** (`plans`, `currentPlanSlug`, `isLoading`, `error`). If state leaks between tests, failures get masked as false-green. The suite uses `vi.resetModules()` + dynamic `import()` in every test to get a fresh instance — state isolation is the primary design constraint here. ### `useBillingPlans` (12 tests) - **Concurrent-call dedup.** The \`isLoading\` guard is validated by creating a pending promise, firing a second \`fetchPlans()\` while the first is in-flight, and asserting the mock is called **exactly once**. - **Error branching.** \`Error\` instance → \`.message\` captured. Non-Error rejection → fallback string (\`'Failed to fetch plans'\`). Both paths also verify \`console.error\` logging via a spy. - **Error-reset invariant.** After a failure, a subsequent success must null out \`error.value\` — order-dependent and easy to regress. - **Shared-state invariant.** Two separate \`useBillingPlans()\` calls return refs pointing at the same module-level state. - **Computed filtering.** \`monthlyPlans\` / \`annualPlans\` partition by duration — assertions on distinct output, not input re-assertion. ### \`tierBenefits\` (7 tests) - Table-driven across all \`TierKey\` values for \`maxDuration\`, \`addCredits\`, \`customLoRAs\` branches. - \`monthlyCredits\` free-tier path including the \`remoteConfig.free_tier_credits\` null fallback. - Translator/formatter forwarding verified by spy. ## Principles applied - No mocks of \`vue\`, \`pinia\`, or \`@vueuse/core\` — only our own \`workspaceApi\`. - Behavioral assertions only — no return-shape checks. - All 19 tests pass; typecheck/lint/format clean. Test-only; no production code touched. |
||
|
jaeone94
|
5d04df7b2c |
fix: prevent duplicate prepareForSave and conflicting is_new telemetry on self-overwrite Save As (#11329)
## Summary Follow-up to PR #10816. Fixes a telemetry semantic bug in `saveWorkflowAs` that emitted two conflicting events for a single user action. ### What changed - `saveWorkflowAs` self-overwrite branch now calls `workflowStore.saveWorkflow` directly instead of delegating to the `saveWorkflow()` wrapper. The wrapper would run `prepareForSave` a second time and emit `trackWorkflowSaved({ is_new: false })`, which then conflicted with the outer `saveWorkflowAs`'s `trackWorkflowSaved({ is_new: true })` for the same user action. - Added regression tests asserting a single `trackWorkflowSaved` call with `{ is_new: true }` and a single `prepareForSave` invocation on both the self-overwrite and copy paths. ### Issues fixed - Fixes #10819 ### Why no E2E test The bug and fix are entirely about observability (how many telemetry events are emitted and with what payload). There is no user-visible change — the file is saved correctly in both pre- and post-fix cases, and `is_new` values are never rendered in the UI. Playwright tests cannot directly verify `trackWorkflowSaved` call counts/payloads without intercepting outbound analytics traffic, which is not a pattern used elsewhere in `browser_tests/`. Unit tests at the service boundary are the appropriate level for this contract: they mock `useTelemetry` and can assert exact call count and payload deterministically. ### Test plan - [x] `pnpm test:unit src/platform/workflow/core/services/workflowService.test.ts` — 56 tests pass (including 2 new regression tests + 1 expanded assertion) - [x] `pnpm typecheck` - [x] `pnpm lint` - [x] `pnpm format` ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11329-fix-prevent-duplicate-prepareForSave-and-conflicting-is_new-telemetry-on-self-overwrite-3456d73d36508192875ed5e70ab9c359) by [Unito](https://www.unito.io) |
||
|
Dante
|
2d50cc2d76 |
feat: show success toast after ComfyHub publish (#11316)
## Summary
Adds a success toast in the ComfyHub publish flow so users get explicit
confirmation that the workflow was published before the dialog closes.
## Changes
- **What**: `ComfyHubPublishDialog.handlePublish()` calls `toast.add({
severity: 'success', ... })` after `submitToComfyHub()` resolves and
before `onClose()` runs. Adds two i18n keys (`publishSuccessTitle`,
`publishSuccessDescription`) and an assertion in the existing
success-path test.
## Review Focus
- This is the lightweight stop-gap discussed in [Slack
thread](https://comfy-organization.slack.com/archives/C0AEPRS8N74/p1776370871654139?thread_ts=1776362591.237159&cid=C0AEPRS8N74)
while the larger published-state design is still pending phase-2 work.
Symmetric with the existing `publishFailedTitle/Description` error
toast.
- `submitToComfyHub` is synchronous (asset uploads happen inside it), so
a successful resolve means the workflow is live.
- `<Toast>` is mounted in `GlobalToast.vue`, so it persists after
`onClose()` destroys the dialog.
## Screenshots (if applicable)
<img width="1135" height="634" alt="Screenshot 2026-04-17 at 8 11 34 AM"
src="https://github.com/user-attachments/assets/a71400a7-2055-4c2a-a761-9298cfa24e9a"
/>
n/a — toast text only.
┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-11316-feat-show-success-toast-after-ComfyHub-publish-3446d73d365081a7bbb3ca29ca3bb618)
by [Unito](https://www.unito.io)
|
||
|
jaeone94
|
a1e6fb36d2 |
refactor: harden ChangeTracker lifecycle with self-defending API (#10816)
## Summary Harden the `ChangeTracker` lifecycle to eliminate the class of bugs where an inactive workflow's tracker silently captures the wrong graph state. Renames `checkState()` to `captureCanvasState()` with a self-defending assertion, introduces `deactivate()` and `prepareForSave()` lifecycle methods, and closes a latent undo-history corruption bug discovered during code review. ## Background ComfyUI supports multiple workflows open as tabs, but only one canvas (`app.rootGraph`) exists at a time. When the user switches tabs, the old workflow's graph is unloaded and the new one is loaded into this shared canvas. The old `checkState()` method serialized `app.rootGraph` into `activeState` to track changes for undo/redo. It had no awareness of *which* workflow it belonged to -- if called on an inactive tab's tracker, it would capture the active tab's graph data and silently overwrite the inactive workflow's state. This caused permanent data loss (fixed in PR #10745 with caller-side `isActive` guards). The caller-side guards were fragile: every new call site had to remember to add the guard, and forgetting would reintroduce the same silent data corruption. Additionally, `beforeLoadNewGraph` only called `store()` (viewport/outputs) without `checkState()`, meaning canvas state could be stale if a tab switch happened without a preceding mouseup event. ### Before (fragile) ``` saveWorkflow(workflow): if (isActive(workflow)) <-- caller must remember this guard workflow.changeTracker.checkState() <-- name implies "read", actually writes ... beforeLoadNewGraph(): activeWorkflow.changeTracker.store() <-- only saves viewport, NOT graph state ``` ### After (self-defending) ``` saveWorkflow(workflow): workflow.changeTracker.prepareForSave() <-- handles active/inactive internally ... beforeLoadNewGraph(): activeWorkflow.changeTracker.deactivate() <-- captures graph + viewport together ``` ## Changes - Rename `checkState` to `captureCanvasState` with active-tracker assertion - Add `deactivate()` and `prepareForSave()` lifecycle methods - Fix undo-history corruption: `captureCanvasState()` guarded by `_restoringState` - Fix viewport regression during undo: `deactivate()` skips `captureCanvasState()` during undo/redo but always calls `store()` to preserve viewport (regression from PR #10247) - Log inactive tracker warnings unconditionally at warn level (not DEV-only) - Deprecated `checkState()` wrapper for extension compatibility - Rename `checkState` to `captureCanvasState` in `useWidgetSelectActions` composable - Add `appModeStore.ts` to manual call sites documentation - Add `checkState()` deprecation note to architecture docs - Add 16 unit tests covering all guard conditions, lifecycle methods, and undo behavior - Add E2E test: "Undo preserves viewport offset" ## New ChangeTracker Public API | Method | Caller | Purpose | |--------|--------|---------| | `captureCanvasState()` | Event handlers, UI interactions | Snapshots canvas into activeState, pushes undo. Asserts active tracker. | | `deactivate()` | `beforeLoadNewGraph` only | `captureCanvasState()` (skipped during undo/redo) + `store()`. Freezes state for tab switch. | | `prepareForSave()` | Save paths only | Active: `captureCanvasState()`. Inactive: no-op. | | `checkState()` | **Deprecated** -- extensions only | Wrapper that delegates to `captureCanvasState()` with deprecation warning. | | `store()` | Internal to `deactivate()` | Saves viewport, outputs, subgraph navigation. | | `restore()` | `afterLoadNewGraph` | Restores viewport, outputs, subgraph navigation. | | `reset()` | `afterLoadNewGraph`, save | Resets initial state (marks as "clean"). | ## Test plan - [x] Unit tests: 16 tests covering all guard conditions, state capture, undo queue behavior - [x] E2E test: "Undo preserves viewport offset" verifies no viewport drift on undo - [x] E2E test: "Prevents captureCanvasState from corrupting workflow state during tab switch" - [x] Existing E2E: "Closing an inactive tab with save preserves its own content" - [ ] Manual: rapidly switch tabs during undo/redo, verify no viewport drift - [ ] Manual: verify extensions calling `checkState()` see deprecation warning in console |
||
|
jaeone94
|
693b8383d6 |
fix: missing-asset correctness follow-ups from #10856 (#11233)
Follow-up to #10856. Four correctness issues and their regression tests. ## Bugs fixed ### 1. ErrorOverlay model count reflected node selection `useErrorGroups` exposed `filteredMissingModelGroups` under the public name `missingModelGroups`. `ErrorOverlay.vue` read that alias to compute its model count label, so selecting a node shrank the overlay total. The overlay must always show the whole workflow's errors. Exposed both shapes explicitly: `missingModelGroups` / `missingMediaGroups` (unfiltered totals) and `filteredMissingModelGroups` / `filteredMissingMediaGroups` (selection-scoped). `TabErrors.vue` destructures the filtered variant with an alias. Before https://github.com/user-attachments/assets/eb848c5f-d092-4a4f-b86f-d22bb4408003 After https://github.com/user-attachments/assets/75e67819-c9f2-45ec-9241-74023eca6120 ### 2. Bypass → un-bypass dropped url/hash metadata Realtime `scanNodeModelCandidates` only reads widget values, so un-bypass produced a fresh candidate without the url that `enrichWithEmbeddedMetadata` had previously attached from `graphData.models`. `MissingModelRow`'s download/copy-url buttons disappeared after a bypass/un-bypass cycle. Added `enrichCandidateFromNodeProperties` that copies `url`/`hash`/`directory` from the node's own `properties.models` — which persists across mode toggles — into each scanned candidate. Applied to every call site of the per-node scan. A later fix in the same branch also enforces directory agreement to prevent a same-name / different-directory collision from stamping the wrong metadata. Before https://github.com/user-attachments/assets/39039d83-4d55-41a9-9d01-dec40843741b After https://github.com/user-attachments/assets/047a603b-fb52-4320-886d-dfeed457d833 ### 3. Initial full scan surfaced interior errors of a muted/bypassed subgraph container `scanAllModelCandidates`, `scanAllMediaCandidates`, and the JSON-based missing-node scan only check each node's own mode. Interior nodes whose parent container was bypassed passed the filter. Added `isAncestorPathActive(rootGraph, executionId)` to `graphTraversalUtil` and post-filter the three pipelines in `app.ts` after the live rootGraph is configured. The filter uses the execution-ID path (`"65:63"` → check node 65's mode) so it handles both live-scan-produced and JSON-enrichment-produced candidates. Before https://github.com/user-attachments/assets/3032d46b-81cd-420e-ab8e-f58392267602 After https://github.com/user-attachments/assets/02a01931-951d-4a48-986c-06424044fbf8 ### 4. Bypassed subgraph entry re-surfaced interior errors `useGraphNodeManager` replays `graph.onNodeAdded` for each existing interior node when the Vue node manager initializes on subgraph entry. That chain reached `scanSingleNodeErrors` via `installErrorClearingHooks`' `onNodeAdded` override. Each interior node's own mode was active, so the caller guards passed and the scan re-introduced the error that the initial pipeline had correctly suppressed. Added an ancestor-activity gate at the top of `scanSingleNodeErrors`, the single entry point shared by paste, un-bypass, subgraph entry, and subgraph container activation. A later commit also hardens this guard against detached nodes (null execution ID → skip) and applies the same ancestor check to `isCandidateStillActive` in the realtime verification callback. Before https://github.com/user-attachments/assets/fe44862d-f1d6-41ed-982d-614a7e83d441 After https://github.com/user-attachments/assets/497a76ce-3caa-479f-9024-4cd0f7bd20a4 ## Tests - 6 unit tests for `isAncestorPathActive` (root, active, immediate-bypass, deep-nested mute, unresolvable ancestor, null rootGraph) - 4 unit tests for `enrichCandidateFromNodeProperties` (enrichment, no-overwrite, name mismatch, directory mismatch) - 1 unit test for `scanSingleNodeErrors` ancestor guard (subgraph entry replaying onNodeAdded) - 2 unit tests for `useErrorGroups` dual export + ErrorOverlay contract - 4 E2E tests: - ErrorOverlay model count stays constant when a node is selected (new fixture `missing_models_distinct.json`) - Bypass/un-bypass cycle preserves Copy URL button (uses `missing_models_from_node_properties`) - Loading a workflow with bypassed subgraph suppresses interior missing model error (new fixture `missing_models_in_bypassed_subgraph.json`) - Entering a bypassed subgraph does not resurface interior missing model error (shares the above fixture) `pnpm typecheck`, `pnpm lint`, 206 related unit tests passing. ## Follow-up Several items raised by code review are deferred as pre-existing tech debt or scope-avoided refactors. Tracked via comments on #11215 and #11216. --- Follows up on #10856. |
||
|
AustinMroz
|
988a546721 |
Add missing dialog tests (#11133)
Leveraging the fancy coverage functionality of #10930, this PR aims to add coverage to missing dialogue models. This has proven quite constructive as many of the dialogues have since been shown to be bugged. - The APINodes sign in dialog that displays when attempting to run a workflow containing Partner nodes while not logged in was intended to display a list of nodes required to execute the workflow. The import for this component was forgotten in the original commit (#3532) and the backing component was later knipped - Error dialogs resulting are intended to display the file responsible for the error, but the prop was accidentally left out during the refactoring of #3265 - ~~The node library migration (#8548) failed to include the 'Edit Blueprint' button, and had incorrect sizing and color on the 'Delete Blueprint' button.~~ - On request, the library button changes were spun out to a separate PR ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11133-Add-missing-dialog-tests-33e6d73d3650812cb142d610461adcd4) by [Unito](https://www.unito.io) --------- Co-authored-by: GitHub Action <action@github.com> |
||
|
Benjamin Lu
|
719ed16d32 |
fix: track workspace subscription success on immediate subscribe (#11130)
## Summary Track GTM `subscription_success` when a workspace subscription completes synchronously in the dialog. The async billing-operation path already emitted telemetry; the missing gap was the immediate `subscribed` response. ## Changes - **What**: Add the missing GTM success emission to both synchronous workspace subscribe success branches while preserving the existing toast, billing refresh, and dialog close behavior. ## Review Focus Verify the synchronous `response.status === "subscribed"` workspace dialog paths are the only missing frontend success emissions, while the async billing-operation telemetry path remains unchanged. This PR intentionally stays minimal. It does not add new browser coverage yet; the previous component-level unit test was more implementation-coupled than this fix justified, and a better long-term test would be a higher-level workspace billing flow test once we have a cleaner harness. |
