Commit Graph

14 Commits

Author SHA1 Message Date
Csongor Czezar
7160a9ee3f fix: QPO progress bar now shows node name in subgraphs (#7688)
## Summary

Resolve the queue progress node label from queued prompt metadata so
subgraph execution IDs show the correct node name without depending on
the live canvas.

## Changes

- **What**: Store a prompt-scoped `executionId -> { title, type }`
lookup from `p.output` when queueing a job, and use that lookup for the
active job's executing node label.
- **What**: Reuse the same job-scoped node info for the browser tab
title so it stays aligned with the queue overlay.
- **What**: Add unit coverage for root and subgraph execution IDs, and
merge the branch forward to current `main`.

## Review Focus

This keeps the fix scoped to the existing singular `activeJobId` path.
It fixes subgraph labels and avoids the workflow-switching regression
from resolving against `app.rootGraph`, but it does not redesign
concurrent multi-job selection yet.

Longer term, the cleaner solution is still prompt-scoped execution
metadata from the backend rather than frontend reconstruction.

## Screenshots (if applicable)

N/A

---------

Co-authored-by: Alexander Brown <drjkl@comfy.org>
Co-authored-by: Benjamin Lu <benjaminlu1107@gmail.com>
2026-05-15 14:00:33 -07:00
pythongosssss
15b8771cc2 fix: clear active job on reconnect if no longer in queue (#12067)
## Summary

When a socket disconnects messages can be missed and lead to a stale UI
state, this updates the state on reconnect and clears the active job if
it is no longer running

## Changes

- **What**: 
- add call to update queue on reconnect
- clear active job if job not in queue response
- tests

┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-12067-fix-clear-active-job-on-reconnect-if-no-longer-in-queue-3596d73d365081f79d42d73966420c50)
by [Unito](https://www.unito.io)
2026-05-11 09:28:23 +00:00
Dante
8f61ecd82e test: add unit tests for executionStore WebSocket handlers (#11746)
## Summary

Adds 22 new tests for \`src/stores/executionStore.ts\`, raising coverage
from **48.7% → 82.9%** lines (functions 47% → 72.5%). Tests drive each
WebSocket handler by capturing handlers registered through the mocked
\`api.addEventListener\` and dispatching CustomEvents at them.

## Test Coverage

WebSocket handlers (driven through \`bindExecutionEvents\`):
- \`execution_start\` — sets activeJobId, seeds queued job entry, clears
initializing state for the starting job.
- \`execution_cached\` — marks listed nodes; no-op when no active job.
- \`execution_interrupted\` — clears active job state.
- \`executed\` — marks executed node; no-op when no active job.
- \`execution_success\` — clears active job and progress state.
- \`executing\` — clears \`_executingNodeProgress\` and activeJobId on
null detail.
- \`progress\` — sets \`_executingNodeProgress\`.
- \`status\` — reads clientId once and stops listening.
- \`execution_error\` — service-level (no node_id) routes to
\`lastPromptError\`; runtime errors route to \`lastExecutionError\`.
- \`notification\` — marks job as initializing on "Waiting for a
machine"; ignores empty id and unrelated text.

Other:
- \`unbindExecutionEvents\` removes every listener registered.
- \`storeJob\` populates queuedJobs, jobIdToWorkflowId,
jobIdToSessionWorkflowPath.
- \`registerJobWorkflowIdMapping\` ignores empty inputs.
- \`ensureSessionWorkflowPath\` is idempotent and updates on change.

## Testing

\`\`\`bash
pnpm vitest run src/stores/executionStore.test.ts
pnpm vitest run src/stores/executionStore.test.ts --coverage
--coverage.include='src/stores/executionStore.ts'
\`\`\`

┆Issue is synchronized with this [Notion
page](https://app.notion.com/p/PR-11746-test-add-unit-tests-for-executionStore-WebSocket-handlers-3516d73d3650810aa910f5a022fdc17b)
by [Unito](https://www.unito.io)
2026-04-29 08:29:59 -04:00
Dante
2524846f5c fix: guard progress_text before canvas init (#11174)
## Summary
Prevent early `progress_text` websocket events from throwing before the
graph canvas is initialized.

## Changes
- **What**: Guard `handleProgressText()` until `canvasStore.canvas`
exists, and add a regression test for a startup-time `progress_text`
event arriving before `GraphCanvas` finishes initialization.

## Review Focus
Confirm this is the right guard point for the startup race between
`GraphView` websocket binding and `GraphCanvas` async setup, and that
progress text behavior is unchanged once the canvas is ready.

## Validation
- `pnpm exec eslint src/stores/executionStore.ts
src/stores/executionStore.test.ts`
- `pnpm exec vitest run src/stores/executionStore.test.ts -t "should
ignore progress_text before the canvas is initialized"`
- `pnpm test:unit -- --run src/stores/executionStore.test.ts` still
reports one unrelated isolated-file failure in
`nodeLocatorIdToExecutionId` on current `main`

┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-11174-fix-guard-progress_text-before-canvas-init-3406d73d3650813dad23d511fb51add5)
by [Unito](https://www.unito.io)
2026-04-13 23:47:14 +00:00
jaeone94
66daa6d645 refactor: error system cleanup — store separation, DDD fix, test improvements (#10302)
## Summary

Refactors the error system to improve separation of concerns, fix DDD
layer violations, and address code quality issues.

- Extract `missingNodesErrorStore` from `executionErrorStore`, removing
the delegation pattern that coupled missing-node logic into the
execution error store
- Extract `useNodeErrorFlagSync` composable for node error flag
reconciliation (previously inlined)
- Extract `useErrorClearingHooks` composable with explicit callback
cleanup on node removal
- Extract `useErrorActions` composable to deduplicate telemetry+command
patterns across error card components
- Move `getCnrIdFromNode`/`getCnrIdFromProperties` to
`platform/nodeReplacement` layer (DDD fix)
- Move `missingNodesErrorStore` to `platform/nodeReplacement` (DDD
alignment)
- Add unmount cancellation guard to `useErrorReport` async `onMounted`
- Return watch stop handle from `useNodeErrorFlagSync`
- Add `asyncResolvedIds` eviction on `missingNodesError` reset
- Add `console.warn` to silent catch blocks and empty array guard
- Hoist `useCommandStore` to setup scope, fix floating promises
- Add `data-testid` to error groups, image/video error spans, copy
button
- Update E2E tests to use scoped locators and testids
- Add unit tests for `onNodeRemoved` restoration and double-install
guard

Fixes #9875, Fixes #10027, Fixes #10033, Fixes #10085

## Test plan

- [x] Existing unit tests pass with updated imports and mocks
- [x] New unit tests for `useErrorClearingHooks` (callback restoration,
double-install guard)
- [x] E2E tests updated to use scoped locators and `data-testid`
- [ ] Manual: verify error tab shows runtime errors and missing nodes
correctly
- [ ] Manual: verify "Find on GitHub", "Copy", and "Get Help" buttons
work in error cards

┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-10302-refactor-error-system-cleanup-store-separation-DDD-fix-test-improvements-3286d73d365081838279d045b8dd957a)
by [Unito](https://www.unito.io)

---------

Co-authored-by: GitHub Action <action@github.com>
2026-03-24 16:43:22 +09:00
John Haugeland
21edbd3ee5 fix: cap nodeProgressStatesByJob to prevent unbounded memory growth (#9249)
This has semi-significant performance impact if you use the same
workflow for more than a day. I left Comfy running with a mouse jiggler
and this reduced my instance to a crawl after about an hour.

`nodeProgressStatesByJob` accumulated an entry for every job that ever
executed during a session. In long-running sessions this grew without
bound, since entries were only removed for the active job on
`resetExecutionState`.

Add `MAX_PROGRESS_JOBS` (1000) and `evictOldProgressJobs()`, called
after each `handleProgressState` update. When the map exceeds the limit,
the oldest entries (by ES2015+ insertion order) are pruned — keeping
only the most recent 1000. This mirrors the pattern used by
assetsStore's `MAX_HISTORY_ITEMS`.

Also adds tests for:
- nodeLocationProgressStates computed reactivity (recomputes on
  wholesale replacement, produces new references)
- Eviction behavior (retains below limit, evicts oldest above limit,
  preserves most recent, no-op when updating existing job)
- API event handler wiring via captured apiEventHandlers map

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-9249-fix-cap-nodeProgressStatesByJob-to-prevent-unbounded-memory-growth-3136d73d365081e49b36d8ade0d4dd6e)
by [Unito](https://www.unito.io)

---------

Co-authored-by: GitHub Action <action@github.com>
2026-03-12 17:51:14 -07:00
John Haugeland
5fe31e63ec Cache execution id to node locator id mappings (#9244)
## Summary

nodeLocationProgressStates runs executionIdToNodeLocatorId for every
ancestor prefix of every node's display_node_id on every WebSocket
progress update. Each call traverses the subgraph hierarchy via
getNodeById. For nested subgraphs with many executing nodes, this
results in O(N × D²) graph lookups per progress tick, where N is
the number of executing nodes and D is the nesting depth.

Add a plain Map cache inside the execution store that memoizes
executionIdToNodeLocatorId results by execution ID string. The
cache persists across computed re-evaluations within a single
execution run, reducing subsequent progress updates to O(1)
lookups per execution ID.

Cache invalidation:
- Cleared at execution start (handleExecutionStart) to ensure
  fresh graph state for each new run
- Cleared at execution end (resetExecutionState) to prevent
  stale entries leaking across runs

The cache stores strings only (no graph node references), with
typical size of ~50-200 entries per run, so memory impact is
negligible.

- **What**: Caches a mapping of ids to ids, preventing an exponential
re-scan of subgraphs
- **Breaking**: Nothing
- **Dependencies**: None

You will need to set the feature flag
`ff:expose_executionId_to_node_locator_id_cache_counters` from the
underlying counter PR if you want to measure the impact.

```js
  localStorage.setItem(
    'ff:expose_executionId_to_node_locator_id_cache_counters',
    'true'
  )
```

## Review Focus

Before pulling this PR, pull
https://github.com/Comfy-Org/ComfyUI_frontend/pull/9243 , set the
feature flag, and run a workflow with nested subgraphs, then look in
console to see a cache measurement.

Next, pull this PR and load the same workflow. Note the massive
reduction in visits.

## Screenshots

Login problems due to cross-opener policy are preventing me from taking
screenshots from a local dev build at this time

## Thread

There isn't one.  I don't have access to AmpCode or Unito.

┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-9244-Cache-execution-id-to-node-locator-id-mappings-3136d73d365081e680eae3c891480ee7)
by [Unito](https://www.unito.io)

Co-authored-by: bymyself <cbyrne@comfy.org>
2026-03-12 14:13:27 -07:00
jaeone94
80fe51bb8c feat: show missing node packs in Errors Tab with install support (#9213)
## Summary

Surfaces missing node pack information in the Errors Tab, grouped by
registry pack, with one-click install support via ComfyUI Manager.

## Changes

- **What**: Errors Tab now groups missing nodes by their registry pack
and shows a `MissingPackGroupRow` with pack name, node/pack counts, and
an Install button that triggers Manager installation. A
`MissingNodeCard` shows individual unresolvable nodes that have no
associated pack. `useErrorGroups` was extended to resolve missing node
types to their registry packs using the `/api/workflow/missing_nodes`
endpoint. `executionErrorStore` was refactored to track missing node
types separately from execution errors and expose them reactively.
- **Breaking**: None

## Review Focus

- `useErrorGroups.ts` — the new `resolveMissingNodePacks` logic fetches
pack metadata and maps node types to pack IDs; edge cases around partial
resolution (some nodes have a pack, some don't) produce both
`MissingPackGroupRow` and `MissingNodeCard` entries
- `executionErrorStore.ts` — the store now separates `missingNodeTypes`
state from `errors`; the deferred-warnings path in `app.ts` now calls
`setMissingNodeTypes` so the Errors Tab is populated even when a
workflow loads without executing

## Screenshots (if applicable)


https://github.com/user-attachments/assets/97f8d009-0cac-4739-8740-fd3333b5a85b


┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-9213-feat-show-missing-node-packs-in-Errors-Tab-with-install-support-3126d73d36508197bc4bf8ebfd2125c8)
by [Unito](https://www.unito.io)
2026-02-25 20:25:47 -08:00
jaeone94
8aa4e36fd5 [refactor] Extract executionErrorStore from executionStore (#9060)
## Summary
Extracts error-related state and logic from `executionStore` into a
dedicated `executionErrorStore` for better separation of concerns.

## Changes
- **New store**: `executionErrorStore` with all error state
(`lastNodeErrors`, `lastExecutionError`, `lastPromptError`), computed
properties (`hasAnyError`, `totalErrorCount`,
`activeGraphErrorNodeIds`), and UI state (`isErrorOverlayOpen`,
`showErrorOverlay`, `dismissErrorOverlay`)
- **Moved util**: `executionIdToNodeLocatorId` extracted to
`graphTraversalUtil`, reusing `traverseSubgraphPath` and accepting
`rootGraph` as parameter
- **Updated consumers**: 12 files updated to import from
`executionErrorStore`
- **Backward compat**: Deprecated getters retained in `ComfyApp` for
extension compatibility

## Review Focus
- Deprecated getters in `app.ts` — can be removed in a future
breaking-change PR once extension authors migrate

┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-9060-refactor-Extract-executionErrorStore-from-executionStore-30e6d73d36508101973de835ab6b199f)
by [Unito](https://www.unito.io)
2026-02-21 16:51:22 -08:00
Christian Byrne
473713cf02 refactor: rename internal promptId/PromptId to jobId/JobId (#8730)
## Summary

Rename all internal TypeScript usage of legacy `promptId`/`PromptId`
naming to `jobId`/`JobId` across ~38 files for consistency with the
domain model.

## Changes

- **What**: Renamed internal variable names, type aliases, function
names, class getters, interface fields, and comments from
`promptId`/`PromptId` to `jobId`/`JobId`. Wire-protocol field names
(`prompt_id` in Zod schemas and `e.detail.prompt_id` accesses) are
intentionally preserved since they match the backend API contract.

## Review Focus

- All changes are pure renames with no behavioral changes
- Wire-protocol fields (`prompt_id`) are deliberately unchanged to
maintain backend compatibility
- Test fixtures updated to use consistent `job-id` naming

┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-8730-refactor-rename-internal-promptId-PromptId-to-jobId-JobId-3016d73d3650813ca40ce337f7c5271a)
by [Unito](https://www.unito.io)
2026-02-20 02:10:53 -08:00
Christian Byrne
6c205cbf4c fix: jobs stuck in initializing state when failing before execution_start (#8689)
## Summary

Fix jobs getting permanently stuck in "initializing" state when they
fail before the `execution_start` WebSocket event fires.

## Changes

- **What**: Added `reconcileInitializingPrompts(activeJobIds)` to
`executionStore` that removes orphaned initializing prompt IDs not
present in the active jobs set. Called from `queueStore.update()` after
fetching Running/Pending jobs, ensuring stale initializing states are
cleaned up on every queue poll.

## Review Focus

- The reconciliation delegates to the existing
`clearInitializationByPromptIds` to avoid duplicating Set-diffing logic.
- Only runs during `queueStore.update()` which is already a periodic
poll — no additional network calls.

┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-8689-fix-jobs-stuck-in-initializing-state-when-failing-before-execution_start-2ff6d73d3650814dbeeeda71c8bb7d43)
by [Unito](https://www.unito.io)
2026-02-19 22:10:01 -08:00
Johnpaul Chiwetelu
13311a46ea Road to No explicit any: Group 8 (part 7) test files (#8459)
## Summary

This PR removes unsafe type assertions ("as unknown as Type") from test
files and improves type safety across the codebase.

### Key Changes

#### Type Safety Improvements
- Removed improper `as unknown as Type` patterns from 17 test files in
Group 8 part 7
- Replaced with proper TypeScript patterns using factory functions and
Mock types
- Fixed createTestingPinia usage in test files (was incorrectly using
createPinia)
- Fixed vi.hoisted pattern for mockSetDirty in viewport tests  
- Fixed vi.doMock lint issues with vi.mock and vi.hoisted pattern
- Retained necessary `as unknown as` casts only for complex mock objects
where direct type assertions would fail

### Files Changed

Test files (Group 8 part 7 - services, stores, utils):
- src/services/nodeOrganizationService.test.ts
- src/services/providers/algoliaSearchProvider.test.ts
- src/services/providers/registrySearchProvider.test.ts
- src/stores/comfyRegistryStore.test.ts
- src/stores/domWidgetStore.test.ts
- src/stores/executionStore.test.ts
- src/stores/firebaseAuthStore.test.ts
- src/stores/modelToNodeStore.test.ts
- src/stores/queueStore.test.ts
- src/stores/subgraphNavigationStore.test.ts
- src/stores/subgraphNavigationStore.viewport.test.ts
- src/stores/subgraphStore.test.ts
- src/stores/systemStatsStore.test.ts
- src/stores/workspace/nodeHelpStore.test.ts
- src/utils/colorUtil.test.ts
- src/utils/executableGroupNodeChildDTO.test.ts

Source files:
- src/stores/modelStore.ts - Improved type handling

### Testing
- All TypeScript type checking passes (`pnpm typecheck`)
- All affected test files pass (`pnpm test:unit`)
- Linting passes without errors (`pnpm lint`)
- Code formatting applied (`pnpm format`)

Part of the "Road to No Explicit Any" initiative, cleaning up type
casting issues from branch `fix/remove-any-types-part8`.

### Previous PRs in this series:
- Part 2: #7401
- Part 3: #7935
- Part 4: #7970
- Part 5: #8064
- Part 6: #8083
- Part 7: #8092
- Part 8 Group 1: #8253
- Part 8 Group 2: #8258
- Part 8 Group 3: #8304
- Part 8 Group 4: #8314
- Part 8 Group 5: #8329
- Part 8 Group 6: #8344
- Part 8 Group 7: #8459 (this PR)

┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-8459-Road-to-No-explicit-any-Group-8-part-7-test-files-2f86d73d36508114ad28d82e72a3a5e9)
by [Unito](https://www.unito.io)
2026-01-30 03:38:06 +01:00
Alexander Brown
6ce60a11a4 test: use createTestingPinia instead of createPinia (#8376)
Replace \createPinia\ with \createTestingPinia({ stubActions: false })\
from \@pinia/testing\ across 45 test files for proper test isolation.

┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-8376-test-use-createTestingPinia-instead-of-createPinia-2f66d73d36508137a9f0daffcddc86f7)
by [Unito](https://www.unito.io)

Co-authored-by: Amp <amp@ampcode.com>
2026-01-28 22:21:38 -08:00
Alexander Brown
10feb1fd5b chore: migrate tests from tests-ui/ to colocate with source files (#7811)
## Summary

Migrates all unit tests from `tests-ui/` to colocate with their source
files in `src/`, improving discoverability and maintainability.

## Changes

- **What**: Relocated all unit tests to be adjacent to the code they
test, following the `<source>.test.ts` naming convention
- **Config**: Updated `vitest.config.ts` to remove `tests-ui` include
pattern and `@tests-ui` alias
- **Docs**: Moved testing documentation to `docs/testing/` with updated
paths and patterns

## Review Focus

- Migration patterns documented in
`temp/plans/migrate-tests-ui-to-src.md`
- Tests use `@/` path aliases instead of relative imports
- Shared fixtures placed in `__fixtures__/` directories

┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-7811-chore-migrate-tests-from-tests-ui-to-colocate-with-source-files-2da6d73d36508147a4cce85365dee614)
by [Unito](https://www.unito.io)

---------

Co-authored-by: Amp <amp@ampcode.com>
Co-authored-by: GitHub Action <action@github.com>
2026-01-05 16:32:24 -08:00