[refactor] Extract executionErrorStore from executionStore#9060
Merged
christian-byrne merged 5 commits intomainfrom Feb 22, 2026
Merged
[refactor] Extract executionErrorStore from executionStore#9060christian-byrne merged 5 commits intomainfrom
christian-byrne merged 5 commits intomainfrom
Conversation
Separate error-related state and logic from executionStore into a dedicated executionErrorStore for better modularity. Changes: - Create executionErrorStore with all error state (lastNodeErrors, lastExecutionError, lastPromptError), computed properties (hasAnyError, totalErrorCount, activeGraphErrorNodeIds), and UI state (isErrorOverlayOpen, showErrorOverlay, dismissErrorOverlay) - Move executionIdToNodeLocatorId to graphTraversalUtil, reusing existing traverseSubgraphPath and accepting rootGraph as parameter - Update 12 consumer files to import from executionErrorStore - Retain deprecated getters in ComfyApp for backward compatibility
dosubot
bot
added
the
size:XL
|
Related Documentation Checked 0 published document(s) in 1 knowledge base(s). No updates required. How did I do? Any feedback? |
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 02/21/2026, 11:50:17 PM UTC 🔗 Links🎉 Your Storybook is ready for review! |
|
Playwright: ✅ 528 passed, 0 failed · 3 flaky 📊 Browser Reports
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughIntroduces a new Pinia store Changes
Sequence Diagram(s)sequenceDiagram
participant UI as UI Component
participant App as App / Scripts
participant ExecErr as executionErrorStore
participant Exec as executionStore
participant Util as graphTraversalUtil
UI->>ExecErr: read error state (lastNodeErrors, hasAnyError)
UI->>ExecErr: call overlay methods (showErrorOverlay / dismissErrorOverlay)
App->>ExecErr: update/clear errors during flows
Exec->>ExecErr: delegate error recording (no local error refs)
ExecErr->>Util: executionIdToNodeLocatorId(app.rootGraph, id)
ExecErr-->>UI: derived flags / lookup results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
📦 Bundle: 4.37 MB gzip 🔴 +98 BDetailsSummary
Category Glance App Entry Points — 21.5 kB (baseline 21.5 kB) • ⚪ 0 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 942 kB (baseline 942 kB) • 🔴 +353 BGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 68.8 kB (baseline 68.8 kB) • 🟢 -1 BTop-level views, pages, and routed surfaces
Status: 9 added / 9 removed Panels & Settings — 436 kB (baseline 436 kB) • 🟢 -2 BConfiguration panels, inspectors, and settings screens
Status: 10 added / 10 removed User & Accounts — 16 kB (baseline 16 kB) • ⚪ 0 BAuthentication, profile, and account management bundles
Status: 6 added / 6 removed Editors & Dialogs — 738 B (baseline 738 B) • ⚪ 0 BModals, dialogs, drawers, and in-app editors
Status: 1 added / 1 removed UI Components — 43.2 kB (baseline 43.2 kB) • ⚪ 0 BReusable component library chunks
Status: 8 added / 8 removed Data & Services — 2.51 MB (baseline 2.51 MB) • 🔴 +141 BStores, services, APIs, and repositories
Status: 13 added / 13 removed Utilities & Hooks — 57.7 kB (baseline 57.7 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Status: 13 added / 13 removed Vendor & Third-Party — 8.86 MB (baseline 8.86 MB) • ⚪ 0 BExternal libraries and shared vendor chunks
Status: 1 added / 1 removed Other — 7.61 MB (baseline 7.61 MB) • 🟢 -3 BBundles that do not match a named category
Status: 60 added / 60 removed |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/stores/subgraphStore.ts (1)
82-83: Consider a dedicated setter action inexecutionErrorStorefor validation errors.Direct mutation
useExecutionErrorStore().lastNodeErrors = errorsworks (Pinia auto-unwraps and triggers the store's internal watch), but it bypasses any future invariant enforcement the store might need to add aroundlastNodeErrorschanges. A named action such assetValidationErrors(errors)would also make the intent clearer at call sites. This is consistent with the guideline to establish clear public interfaces for stores.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/stores/subgraphStore.ts` around lines 82 - 83, Replace the direct assignment to useExecutionErrorStore().lastNodeErrors with a dedicated action on the execution error store: introduce a method like setValidationErrors(errors) on the executionErrorStore and call useExecutionErrorStore().setValidationErrors(errors) from subgraphStore; keep the property lastNodeErrors internal to the store so the action can perform validation/invariant checks and emit any side-effects, and update callers to use the new setValidationErrors API (identify the store by useExecutionErrorStore, the property lastNodeErrors, and the new action name setValidationErrors).src/stores/executionStore.test.ts (1)
181-188: Prefer a function-based suite title for Vitest describe.♻️ Suggested change
-describe('useExecutionErrorStore - Node Error Lookups', () => { +describe(useExecutionErrorStore, () => {Based on learnings: In test files under src/**/*.test.ts, follow the vitest/prefer-describe-function-title rule by using describe(ComponentOrFunction, ...) instead of a string literal description when naming test suites.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/stores/executionStore.test.ts` around lines 181 - 188, Replace the string literal suite title with a function-based title to satisfy vitest/prefer-describe-function-title; change the describe call that currently uses a string like "useExecutionErrorStore - Node Error Lookups" to use the function identifier useExecutionErrorStore (i.e., describe(useExecutionErrorStore, () => { ... } ) ), keeping the rest of the setup (vi.clearAllMocks, setActivePinia, store = useExecutionErrorStore()) unchanged.src/stores/executionErrorStore.ts (1)
172-192: Prefer function declarations over function expressions.
getNodeErrors,slotHasErrorareconstarrow-function expressions. Per coding guidelines, prefer function declarations for pure/standalone functions.hasInternalErrorForNodeon line 189 already uses a function declaration — align the others.Proposed refactor
- /** Get node errors by locator ID. */ - const getNodeErrors = ( - nodeLocatorId: NodeLocatorId - ): NodeError | undefined => { - return nodeErrorsByLocatorId.value[nodeLocatorId] - } + /** Get node errors by locator ID. */ + function getNodeErrors( + nodeLocatorId: NodeLocatorId + ): NodeError | undefined { + return nodeErrorsByLocatorId.value[nodeLocatorId] + } - /** Check if a specific slot has validation errors. */ - const slotHasError = ( - nodeLocatorId: NodeLocatorId, - slotName: string - ): boolean => { - const nodeError = getNodeErrors(nodeLocatorId) - if (!nodeError) return false - - return nodeError.errors.some((e) => e.extra_info?.input_name === slotName) - } + /** Check if a specific slot has validation errors. */ + function slotHasError( + nodeLocatorId: NodeLocatorId, + slotName: string + ): boolean { + const nodeError = getNodeErrors(nodeLocatorId) + if (!nodeError) return false + + return nodeError.errors.some((e) => e.extra_info?.input_name === slotName) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/stores/executionErrorStore.ts` around lines 172 - 192, The two arrow-function constants getNodeErrors and slotHasError should be converted to named function declarations to match the existing hasInternalErrorForNode style: replace "const getNodeErrors = (nodeLocatorId: NodeLocatorId): NodeError | undefined => { ... }" with "function getNodeErrors(nodeLocatorId: NodeLocatorId): NodeError | undefined { ... }" and similarly convert "const slotHasError = (nodeLocatorId: NodeLocatorId, slotName: string): boolean => { ... }" to "function slotHasError(nodeLocatorId: NodeLocatorId, slotName: string): boolean { ... }"; keep their bodies and return types identical, ensure they remain in the same module scope so callers (and hoisting) behave the same, and run tests/TS build to confirm no type or scope regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/scripts/app.ts`:
- Around line 1445-1448: executionErrorStore.lastNodeErrors is a record so using
.length is incorrect; change the logic that sets and checks lastNodeErrors
(where res.node_errors is assigned) to normalize to null when empty and use
Object.keys(executionErrorStore.lastNodeErrors).length > 0 to detect errors
before calling this.canvas.draw(true, true); do the same replacement for the
other occurrence that checks lastNodeErrors (the same pattern near the other
assignment) so empty maps become null and detection uses
Object.keys(...).length.
In `@src/stores/executionErrorStore.ts`:
- Around line 140-146: In activeGraphErrorNodeIds, avoid unconditionally calling
String(lastExecutionError.value.node_id); add the same null/undefined guard used
in allErrorExecutionIds so you only call String(...) and pass to
getNodeByExecutionId when lastExecutionError.value.node_id is not null/undefined
— e.g., check lastExecutionError.value.node_id != null before computing
execNodeId and running getNodeByExecutionId(app.rootGraph, execNodeId).
---
Nitpick comments:
In `@src/stores/executionErrorStore.ts`:
- Around line 172-192: The two arrow-function constants getNodeErrors and
slotHasError should be converted to named function declarations to match the
existing hasInternalErrorForNode style: replace "const getNodeErrors =
(nodeLocatorId: NodeLocatorId): NodeError | undefined => { ... }" with "function
getNodeErrors(nodeLocatorId: NodeLocatorId): NodeError | undefined { ... }" and
similarly convert "const slotHasError = (nodeLocatorId: NodeLocatorId, slotName:
string): boolean => { ... }" to "function slotHasError(nodeLocatorId:
NodeLocatorId, slotName: string): boolean { ... }"; keep their bodies and return
types identical, ensure they remain in the same module scope so callers (and
hoisting) behave the same, and run tests/TS build to confirm no type or scope
regressions.
In `@src/stores/executionStore.test.ts`:
- Around line 181-188: Replace the string literal suite title with a
function-based title to satisfy vitest/prefer-describe-function-title; change
the describe call that currently uses a string like "useExecutionErrorStore -
Node Error Lookups" to use the function identifier useExecutionErrorStore (i.e.,
describe(useExecutionErrorStore, () => { ... } ) ), keeping the rest of the
setup (vi.clearAllMocks, setActivePinia, store = useExecutionErrorStore())
unchanged.
In `@src/stores/subgraphStore.ts`:
- Around line 82-83: Replace the direct assignment to
useExecutionErrorStore().lastNodeErrors with a dedicated action on the execution
error store: introduce a method like setValidationErrors(errors) on the
executionErrorStore and call
useExecutionErrorStore().setValidationErrors(errors) from subgraphStore; keep
the property lastNodeErrors internal to the store so the action can perform
validation/invariant checks and emit any side-effects, and update callers to use
the new setValidationErrors API (identify the store by useExecutionErrorStore,
the property lastNodeErrors, and the new action name setValidationErrors).
Comment on lines
+1445
to
1448
| executionErrorStore.lastNodeErrors = res.node_errors ?? null | ||
| if (executionErrorStore.lastNodeErrors?.length) { | ||
| this.canvas.draw(true, true) | ||
| } else { |
Contributor
There was a problem hiding this comment.
Fix node error detection — .length doesn’t work on the error map.
lastNodeErrors is a record, so .length is always undefined; node error responses won’t hit the error branch. Normalize via Object.keys and store null when empty.
🐛 Suggested fix
- executionErrorStore.lastNodeErrors = res.node_errors ?? null
- if (executionErrorStore.lastNodeErrors?.length) {
+ const nodeErrors = res.node_errors ?? null
+ const hasNodeErrors =
+ !!nodeErrors && Object.keys(nodeErrors).length > 0
+ executionErrorStore.lastNodeErrors = hasNodeErrors ? nodeErrors : null
+ if (hasNodeErrors) {Also applies to: 1539-1539
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/scripts/app.ts` around lines 1445 - 1448,
executionErrorStore.lastNodeErrors is a record so using .length is incorrect;
change the logic that sets and checks lastNodeErrors (where res.node_errors is
assigned) to normalize to null when empty and use
Object.keys(executionErrorStore.lastNodeErrors).length > 0 to detect errors
before calling this.canvas.draw(true, true); do the same replacement for the
other occurrence that checks lastNodeErrors (the same pattern near the other
assignment) so empty maps become null and detection uses
Object.keys(...).length.
Collaborator
Author
There was a problem hiding this comment.
Pre-existing issue, out of scope for this refactoring PR. Will address in a follow-up.
Contributor
There was a problem hiding this comment.
@jaeone94, understood! Since this is a pre-existing issue from before the refactoring, it makes sense to keep this PR focused on the store extraction.
Would you like me to create a GitHub issue to track this bug as a follow-up? I can include:
- The problematic
.lengthcheck on the record object (occurs in two places based on the review) - A suggested fix to use
Object.keys(nodeErrors).length > 0 - A link back to this PR and comment as context
Just let me know if you'd like me to open that issue for you! 🐰
🧠 Learnings used
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: src/AGENTS.md:0-0
Timestamp: 2026-01-27T20:51:47.936Z
Learning: Applies to src/**/*.{ts,tsx,vue} : Avoid `ts-expect-error` - fix the underlying type issue instead
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 8307
File: src/stores/bootstrapStore.ts:61-78
Timestamp: 2026-01-26T18:39:26.986Z
Learning: In src/stores/**/*.ts: Do not suggest exposing internal state or error refs from stores unless there is demonstrated active usage of those properties elsewhere in the codebase. Keep internal state private by default; only expose what is actively consumed.
Comment on lines
+140
to
+146
| if (lastExecutionError.value) { | ||
| const execNodeId = String(lastExecutionError.value.node_id) | ||
| const graphNode = getNodeByExecutionId(app.rootGraph, execNodeId) | ||
| if (graphNode?.graph === activeGraph) { | ||
| ids.add(String(graphNode.id)) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Missing null guard on node_id — inconsistent with allErrorExecutionIds.
In allErrorExecutionIds (lines 92–96) you guard against node_id being null/undefined before pushing. Here in activeGraphErrorNodeIds, String(lastExecutionError.value.node_id) is called unconditionally, which would produce the string "null" or "undefined" and pass it to getNodeByExecutionId.
Proposed fix
if (lastExecutionError.value) {
- const execNodeId = String(lastExecutionError.value.node_id)
- const graphNode = getNodeByExecutionId(app.rootGraph, execNodeId)
- if (graphNode?.graph === activeGraph) {
- ids.add(String(graphNode.id))
+ const nodeId = lastExecutionError.value.node_id
+ if (nodeId !== null && nodeId !== undefined) {
+ const execNodeId = String(nodeId)
+ const graphNode = getNodeByExecutionId(app.rootGraph, execNodeId)
+ if (graphNode?.graph === activeGraph) {
+ ids.add(String(graphNode.id))
+ }
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (lastExecutionError.value) { | |
| const execNodeId = String(lastExecutionError.value.node_id) | |
| const graphNode = getNodeByExecutionId(app.rootGraph, execNodeId) | |
| if (graphNode?.graph === activeGraph) { | |
| ids.add(String(graphNode.id)) | |
| } | |
| } | |
| if (lastExecutionError.value) { | |
| const nodeId = lastExecutionError.value.node_id | |
| if (nodeId !== null && nodeId !== undefined) { | |
| const execNodeId = String(nodeId) | |
| const graphNode = getNodeByExecutionId(app.rootGraph, execNodeId) | |
| if (graphNode?.graph === activeGraph) { | |
| ids.add(String(graphNode.id)) | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/stores/executionErrorStore.ts` around lines 140 - 146, In
activeGraphErrorNodeIds, avoid unconditionally calling
String(lastExecutionError.value.node_id); add the same null/undefined guard used
in allErrorExecutionIds so you only call String(...) and pass to
getNodeByExecutionId when lastExecutionError.value.node_id is not null/undefined
— e.g., check lastExecutionError.value.node_id != null before computing
execNodeId and running getNodeByExecutionId(app.rootGraph, execNodeId).
Collaborator
Author
There was a problem hiding this comment.
Pre-existing issue, out of scope for this refactoring PR. Will address in a follow-up.
Contributor
There was a problem hiding this comment.
@jaeone94 That makes sense — keeping this refactoring focused is the right approach. Would you like me to create a GitHub issue to track the null guard fix for activeGraphErrorNodeIds as a follow-up task?
🧠 Learnings used
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 8307
File: src/stores/bootstrapStore.ts:61-78
Timestamp: 2026-01-26T18:39:26.986Z
Learning: In src/stores/**/*.ts: Do not suggest exposing internal state or error refs from stores unless there is demonstrated active usage of those properties elsewhere in the codebase. Keep internal state private by default; only expose what is actively consumed.
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 8753
File: src/renderer/extensions/vueNodes/widgets/components/WidgetDOM.vue:17-28
Timestamp: 2026-02-09T03:24:50.064Z
Learning: In this codebase: When destructuring reactive properties from Pinia stores, use `storeToRefs()` to maintain reactivity. Example: `const canvasStore = useCanvasStore(); const { canvas } = storeToRefs(canvasStore)`. Then access with `.value` (e.g., `canvas.value`).
Learnt from: kaili-yang
Repo: Comfy-Org/ComfyUI_frontend PR: 7805
File: src/composables/useCoreCommands.ts:439-439
Timestamp: 2025-12-30T22:22:38.162Z
Learning: In Pinia setup stores, when accessing reactive properties directly via `useStore().property` pattern (e.g., `useQueueUIStore().isOverlayExpanded`), Pinia automatically unwraps refs and returns the primitive value. The `.value` accessor is only needed when destructuring store properties or using `storeToRefs()`.
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: src/lib/litegraph/AGENTS.md:0-0
Timestamp: 2026-02-20T05:29:49.703Z
Learning: Applies to src/lib/litegraph/**/*.{ts,tsx,js,jsx} : Prefer returning `undefined` over `null` in TypeScript/JavaScript
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: src/AGENTS.md:0-0
Timestamp: 2026-01-27T20:51:47.936Z
Learning: Applies to src/**/*.{ts,tsx,vue} : Avoid `ts-expect-error` - fix the underlying type issue instead
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-03T20:30:57.039Z
Learning: Applies to **/stores/*Store.ts : Name Pinia stores as `*Store.ts`
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: src/AGENTS.md:0-0
Timestamp: 2026-01-27T20:51:47.936Z
Learning: Applies to src/**/stores/**/*.{js,ts} : Establish clear public interfaces for stores
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: src/AGENTS.md:0-0
Timestamp: 2026-01-27T20:51:47.936Z
Learning: Applies to src/**/stores/**/*.{js,ts} : Only expose state and actions that are used externally; keep internal state private
dante01yoon
previously approved these changes
Feb 21, 2026
christian-byrne
previously approved these changes
Feb 21, 2026
Contributor
christian-byrne
left a comment
christian-byrne
left a comment
There was a problem hiding this comment.
LGTM with one request
src/scripts/app.ts
Outdated
Comment on lines
+222
to
+233
| * @deprecated Use useExecutionErrorStore().lastNodeErrors instead | ||
| */ | ||
| get lastNodeErrors(): Record<NodeId, NodeError> | null { | ||
| return useExecutionStore().lastNodeErrors | ||
| return useExecutionErrorStore().lastNodeErrors | ||
| } | ||
|
|
||
| /** | ||
| * The error from the previous execution. | ||
| * @deprecated Use useExecutionStore().lastExecutionError instead | ||
| * @deprecated Use useExecutionErrorStore().lastExecutionError instead | ||
| */ | ||
| get lastExecutionError(): ExecutionErrorWsMessage | null { | ||
| return useExecutionStore().lastExecutionError | ||
| return useExecutionErrorStore().lastExecutionError |
Contributor
There was a problem hiding this comment.
note: useExecutionStore won't be easily available to custom extensions at runtime. To re-expose these, we would want to add them to the public API surface here:
ComfyUI_frontend/src/stores/workspaceStore.ts
Lines 89 to 110 in bb40ffa
window.app.extensionManager global variable.
Should we move the properties to the exports of the extensionManager now, and then change the deprecation message to offer that as an alternative? Then we can remove these getters in app.ts in 6 months or so after auditing usage (with our source graph codesearch tool: https://cs.comfy.org/search).
@coderabbitai make a task to eventually remove these deprecated getters in 6-12 months time.
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
@christian-byrne, excellent point about extension compatibility! You're right that custom extensions won't have easy access to useExecutionErrorStore() at runtime. I'll create a tracking issue for the eventual removal of these deprecated getters.
Regarding your suggestion: Yes, I think we should expose these through extensionManager first, then update the deprecation messages. This provides a clear migration path for extension authors:
Proposed two-phase approach:
- Now (follow-up to this PR): Add
lastNodeErrorsandlastExecutionErrorto the extensionManager public API, update deprecation messages to recommendapp.extensionStore.lastNodeErrorsinstead - Later (6-12 months): Remove the deprecated getters from app.ts once extensions have migrated
This way, extension authors have a working, documented migration path rather than being directed to a store they can't easily access.
🏁 Script executed:
#!/bin/bash
# Create a GitHub issue to track removal of deprecated error getters in app.ts
gh issue create \
--repo "Comfy-Org/ComfyUI_frontend" \
--title "Remove deprecated error getters from ComfyApp (app.ts)" \
--body "## Context
PR `#9060` extracted error-related state from executionStore to executionErrorStore, leaving deprecated getters in ComfyApp for backward compatibility.
## Current Deprecated Getters
- \`app.lastNodeErrors\` → currently points to \`useExecutionErrorStore().lastNodeErrors\`
- \`app.lastExecutionError\` → currently points to \`useExecutionErrorStore().lastExecutionError\`
## Problem
Custom extensions cannot easily access \`useExecutionErrorStore()\` at runtime. The current deprecation messages are not actionable for extension authors.
## Proposed Solution
**Phase 1** (should be done in a follow-up PR to `#9060`):
- Expose error properties through \`app.extensionManager\` public API (currently at \`app.extensionStore\`)
- Update deprecation messages to point extension authors to \`app.extensionStore.lastNodeErrors\` etc.
- Reference: https://github.com/Comfy-Org/ComfyUI_frontend/blob/bb40ffae3c2d8e5fdf0c0fb1255a633283892f6b/src/stores/workspaceStore.ts#L89-L110
**Phase 2** (this issue - target: 6-12 months after Phase 1):
- Remove deprecated getters from \`app.ts\` entirely
- By then, extensions should have migrated to \`app.extensionStore\` API
- Audit extension usage before removal
## Timeline
- Target removal: August 2026 - February 2027 (6-12 months from Feb 2026)
- Prerequisite: Complete Phase 1 first
## Related
- PR: `#9060`
- Discussion: https://github.com/Comfy-Org/ComfyUI_frontend/pull/9060#discussion_r2836771402
- Requested by: `@christian-byrne`" \
--label "enhancement,technical-debt" \
--assignee "christian-byrne"
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Create a GitHub issue to track removal of deprecated error getters in app.ts
# Retry without the technical-debt label
gh issue create \
--repo "Comfy-Org/ComfyUI_frontend" \
--title "Remove deprecated error getters from ComfyApp (app.ts)" \
--body "## Context
PR `#9060` extracted error-related state from executionStore to executionErrorStore, leaving deprecated getters in ComfyApp for backward compatibility.
## Current Deprecated Getters
- \`app.lastNodeErrors\` → currently points to \`useExecutionErrorStore().lastNodeErrors\`
- \`app.lastExecutionError\` → currently points to \`useExecutionErrorStore().lastExecutionError\`
## Problem
Custom extensions cannot easily access \`useExecutionErrorStore()\` at runtime. The current deprecation messages are not actionable for extension authors.
## Proposed Solution
**Phase 1** (should be done in a follow-up PR to `#9060`):
- Expose error properties through \`app.extensionManager\` public API (currently at \`app.extensionStore\`)
- Update deprecation messages to point extension authors to \`app.extensionStore.lastNodeErrors\` etc.
- Reference: https://github.com/Comfy-Org/ComfyUI_frontend/blob/bb40ffae3c2d8e5fdf0c0fb1255a633283892f6b/src/stores/workspaceStore.ts#L89-L110
**Phase 2** (this issue - target: 6-12 months after Phase 1):
- Remove deprecated getters from \`app.ts\` entirely
- By then, extensions should have migrated to \`app.extensionStore\` API
- Audit extension usage before removal
## Timeline
- Target removal: August 2026 - February 2027 (6-12 months from Feb 2026)
- Prerequisite: Complete Phase 1 first
## Related
- PR: `#9060`
- Discussion: https://github.com/Comfy-Org/ComfyUI_frontend/pull/9060#discussion_r2836771402
- Requested by: `@christian-byrne`" \
--label "enhancement" \
--assignee "christian-byrne"
Length of output: 57
@christian-byrne, excellent point about extension compatibility! You're right that custom extensions won't have easy access to useExecutionErrorStore() at runtime. I've created issue #9066 to track the eventual removal of these deprecated getters.
Regarding your suggestion: Yes, I think we should expose these through extensionManager first, then update the deprecation messages. This provides a clear migration path for extension authors:
Proposed two-phase approach:
- Phase 1 (follow-up to this PR): Add
lastNodeErrorsandlastExecutionErrorto the extensionManager public API (exposed atapp.extensionStore), then update deprecation messages to recommendapp.extensionStore.lastNodeErrorsinstead - Phase 2 (tracked in
#9066, 6-12 months later): Remove the deprecated getters from app.ts once extensions have had time to migrate
This way, extension authors have a working, documented migration path rather than being directed to a Pinia store they can't easily access at runtime. The issue documents both phases and sets a target timeline of August 2026 - February 2027 for the final removal.
🧬 Code Graph Analysis Results
<no_relevant_code_snippets/>
🧠 Learnings used
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 8307
File: src/stores/bootstrapStore.ts:61-78
Timestamp: 2026-01-26T18:39:26.986Z
Learning: In src/stores/**/*.ts: Do not suggest exposing internal state or error refs from stores unless there is demonstrated active usage of those properties elsewhere in the codebase. Keep internal state private by default; only expose what is actively consumed.
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: src/AGENTS.md:0-0
Timestamp: 2026-01-27T20:51:47.936Z
Learning: Applies to src/**/stores/**/*.{js,ts} : Restrict store extension access to keep internal implementation private
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: src/AGENTS.md:0-0
Timestamp: 2026-01-27T20:51:47.936Z
Learning: Applies to src/**/stores/**/*.{js,ts} : Establish clear public interfaces for stores
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: src/AGENTS.md:0-0
Timestamp: 2026-01-27T20:51:47.936Z
Learning: Applies to src/**/stores/**/*.{js,ts} : Only expose state and actions that are used externally; keep internal state private
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 8248
File: src/platform/assets/schemas/assetSchema.ts:109-118
Timestamp: 2026-01-22T23:45:06.746Z
Learning: In the ComfyUI_frontend repository, don't suggest exporting schemas, constants, or other declarations unless they are actually used in other parts of the codebase. Keep the API surface minimal until there's a concrete need.
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: src/AGENTS.md:0-0
Timestamp: 2026-01-27T20:51:47.936Z
Learning: Applies to src/**/stores/**/*.{js,ts} : Clean up subscriptions in stores to prevent memory leaks
Collaborator
Author
There was a problem hiding this comment.
Thanks for the review! I've pushed a commit that addresses the change request:
- Added
lastNodeErrorsandlastExecutionErrorto theExtensionManagerinterface andworkspaceStoreexports - Updated deprecation messages in
app.tsto point toapp.extensionManager.lastNodeErrors/lastExecutionError
Contributor
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
- Add lastNodeErrors and lastExecutionError to ExtensionManager interface - Expose error state as computed properties in workspaceStore - Update deprecation messages in app.ts to reference app.extensionManager
jaeone94
dismissed stale reviews from christian-byrne and dante01yoon
via
91bd790
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/types/extensionTypes.ts (1)
117-119:readonlymodifier is missing despite the "read-only" comment.The comment labels these as "read-only" but the TypeScript
readonlymodifier is absent, so the type system will not prevent extension authors from inadvertently assigning to these properties.♻️ Proposed fix — add `readonly` modifiers to match the stated intent
// Execution error state (read-only) - lastNodeErrors: Record<NodeId, NodeError> | null - lastExecutionError: ExecutionErrorWsMessage | null + readonly lastNodeErrors: Record<NodeId, NodeError> | null + readonly lastExecutionError: ExecutionErrorWsMessage | null🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/types/extensionTypes.ts` around lines 117 - 119, The properties marked "read-only" (lastNodeErrors and lastExecutionError) are missing the TypeScript readonly modifier; update their declarations in the type or interface containing them (the ones named lastNodeErrors: Record<NodeId, NodeError> | null and lastExecutionError: ExecutionErrorWsMessage | null) by prefixing with readonly so they become readonly lastNodeErrors: ... and readonly lastExecutionError: ..., preserving their nullable types.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/scripts/app.ts`:
- Around line 1884-1889: The clean() path currently clears node outputs and
manually nulls
executionErrorStore.lastNodeErrors/lastExecutionError/lastPromptError but
doesn't dismiss the UI error overlay; replace the manual nulling with a call to
executionErrorStore.clearAllErrors() (or add that call alongside
nodeOutputStore.resetAllOutputsAndPreviews()) so the overlay state is reset;
locate the logic around
useNodeOutputStore()/nodeOutputStore.resetAllOutputsAndPreviews and
useExecutionErrorStore() in clean() and invoke clearAllErrors() on the
executionErrorStore instead of directly setting those fields.
- Around line 1405-1409: Replace the three direct mutations of
executionErrorStore (setting lastNodeErrors, lastExecutionError, lastPromptError
to null) with a single call to the store's API:
executionErrorStore.clearAllErrors(); this ensures the atomic reset of all error
fields including isErrorOverlayOpen and preserves the intended behavior
described by the clearAllErrors() docstring while leaving executionStore
untouched.
---
Duplicate comments:
In `@src/scripts/app.ts`:
- Around line 1445-1447: executionErrorStore.lastNodeErrors is a Record
(object), not an array, so the checks using .length are incorrect; update the
conditional before calling this.canvas.draw (referencing
executionErrorStore.lastNodeErrors and this.canvas.draw) to test emptiness via
Object.keys(executionErrorStore.lastNodeErrors ?? {}).length > 0 (or a
null/undefined guard) and update the other check that currently does return
!executionErrorStore.lastNodeErrors to instead return true/false based on
whether the Record is null/undefined or has zero keys (e.g., use
Object.keys(...).length === 0) so both spots correctly handle an object of node
errors.
---
Nitpick comments:
In `@src/types/extensionTypes.ts`:
- Around line 117-119: The properties marked "read-only" (lastNodeErrors and
lastExecutionError) are missing the TypeScript readonly modifier; update their
declarations in the type or interface containing them (the ones named
lastNodeErrors: Record<NodeId, NodeError> | null and lastExecutionError:
ExecutionErrorWsMessage | null) by prefixing with readonly so they become
readonly lastNodeErrors: ... and readonly lastExecutionError: ..., preserving
their nullable types.
Ensures isErrorOverlayOpen is also reset when clearing error state in queueItems processing and clean(), preventing stale error overlays.
…ub.com/Comfy-Org/ComfyUI_frontend into refactor/extract-execution-error-store
jaeone94
added
needs-backport
github-actions bot
pushed a commit
that referenced
this pull request
Feb 23, 2026
## 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)
github-actions bot
pushed a commit
that referenced
this pull request
Feb 23, 2026
## 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)
Member
Member
github-actions
bot
removed
the
needs-backport
christian-byrne
pushed a commit
that referenced
this pull request
Feb 23, 2026
…utionStore (#9130) Backport of #9060 to `core/1.40` Automatically created by backport workflow. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-9130-backport-core-1-40-refactor-Extract-executionErrorStore-from-executionStore-3106d73d3650818ca57dcec8dcb8a709) by [Unito](https://www.unito.io) Co-authored-by: jaeone94 <89377375+jaeone94@users.noreply.github.com>
christian-byrne
pushed a commit
that referenced
this pull request
Feb 23, 2026
…cutionStore (#9131) Backport of #9060 to `cloud/1.40` Automatically created by backport workflow. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-9131-backport-cloud-1-40-refactor-Extract-executionErrorStore-from-executionStore-3106d73d3650815e8713d30979eed798) by [Unito](https://www.unito.io) Co-authored-by: jaeone94 <89377375+jaeone94@users.noreply.github.com>
This was referenced Mar 6, 2026
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Extracts error-related state and logic from
executionStoreinto a dedicatedexecutionErrorStorefor better separation of concerns.Changes
executionErrorStorewith all error state (lastNodeErrors,lastExecutionError,lastPromptError), computed properties (hasAnyError,totalErrorCount,activeGraphErrorNodeIds), and UI state (isErrorOverlayOpen,showErrorOverlay,dismissErrorOverlay)executionIdToNodeLocatorIdextracted tographTraversalUtil, reusingtraverseSubgraphPathand acceptingrootGraphas parameterexecutionErrorStoreComfyAppfor extension compatibilityReview Focus
app.ts— can be removed in a future breaking-change PR once extension authors migrate┆Issue is synchronized with this Notion page by Unito