[refactor] Extract executionErrorStore from executionStore#9060
Open
[refactor] Extract executionErrorStore from executionStore#9060
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, 01:02:25 PM UTC 🔗 Links🎉 Your Storybook is ready for review! |
|
Playwright: ✅ 524 passed, 0 failed · 5 flaky 📊 Browser Reports
|
Contributor
📝 WalkthroughWalkthroughThis PR refactors error state management by introducing a new dedicated Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 🔴 +59 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) • 🔴 +189 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
approved these changes
Feb 21, 2026
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
2 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