feat: propagate errors up subgraphs and show slot errors in subgraphs #6963
Conversation
📝 WalkthroughWalkthroughRefactors node error handling by removing direct red stroke styling from the litegraph service and introducing a reactive error lookup and mapping system in the execution store. The new system provides helper functions to query node and slot error states while propagating errors up the subgraph hierarchy. Changes
Sequence Diagram(s)sequenceDiagram
participant lastNodeErrors as lastNodeErrors<br/>(reactive)
participant watch as Watch Handler
participant store as Execution Store
participant mapping as nodeErrorsByLocatorId<br/>(computed)
participant nodes as Nodes & Slots
participant parents as Parent Subgraphs
lastNodeErrors->>watch: Error state changes
watch->>nodes: Clear all error flags
watch->>store: Process new errors
store->>mapping: Translate errors by locator ID
mapping->>nodes: Mark nodes with errors
mapping->>nodes: Mark input slots with errors
nodes->>parents: Propagate error flags<br/>up parent chain
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
dosubot
bot
added
the
size:L
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/stores/executionStore.ts (1)
289-336: Error flag watcher is conceptually solid; verify theapp.graph.nodesguard and propagationThe watcher correctly resets all node/slot error flags, reapplies them from
lastNodeErrors, and propagateshas_errorsup the subgraph chain via sliced execution IDs, which matches the execution ID semantics. Two things to verify:
- The guard
if (!app.graph || !app.graph.nodes) returnassumes a.nodescollection on the graph, while other utilities tend to work off internal structures (e.g._nodesviaforEachNode). If the realLGraphdoesn’t expose.nodes, this watcher will never run in production. It may be safer to guard only onapp.graph(and letforEachNodehandle empty graphs).- For nested subgraphs, confirm via manual testing or an additional unit test that slicing
executionId.split(':')and walking parents (1..parts.length-1) lines up with howgetNodeByExecutionIdinterprets execution IDs in deeply nested graphs.If either of these assumptions doesn’t hold, you may need a small adjustment to the guard or an extra test around the watcher behavior.
tests-ui/tests/store/executionStore.test.ts (1)
109-276: Good coverage for node error lookups; avoid newas anyin mocksThe new tests do a nice job exercising
getNodeErrorsandslotHasErrorfor root vs subgraph nodes, multiple errors per slot, and missingextra_info, which gives good confidence in the new store helpers.One thing to align with the TS guidelines: the
mockNodein the subgraph test is declared withas any. You can avoidanyby typing it as a minimal structural type instead of casting toany, for example:- const mockNode = { - id: 123, - isSubgraphNode: () => true, - subgraph: mockSubgraph - } as any + const mockNode: { + id: number + isSubgraphNode: () => boolean + subgraph: typeof mockSubgraph + } = { + id: 123, + isSubgraphNode: () => true, + subgraph: mockSubgraph + }This keeps the test strongly typed without changing its behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/services/litegraphService.ts(0 hunks)src/stores/executionStore.ts(4 hunks)tests-ui/tests/store/executionStore.test.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/services/litegraphService.ts
🧰 Additional context used
📓 Path-based instructions (15)
**/*.{vue,ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{vue,ts,tsx}: Leverage VueUse functions for performance-enhancing utilities
Use vue-i18n in Composition API for any string literals and place new translation entries in src/locales/en/main.json
Files:
src/stores/executionStore.tstests-ui/tests/store/executionStore.test.ts
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursorrules)
Use es-toolkit for utility functions
Files:
src/stores/executionStore.tstests-ui/tests/store/executionStore.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
Use TypeScript for type safety
**/*.{ts,tsx}: Never useanytype - use proper TypeScript types
Never useas anytype assertions - fix the underlying type issue
Files:
src/stores/executionStore.tstests-ui/tests/store/executionStore.test.ts
**/*.{ts,tsx,js,vue}
📄 CodeRabbit inference engine (.cursorrules)
Implement proper error handling in components and services
**/*.{ts,tsx,js,vue}: Use 2-space indentation, single quotes, no semicolons, and maintain 80-character line width as configured in.prettierrc
Organize imports by sorting and grouping by plugin, and runpnpm formatbefore committing
Files:
src/stores/executionStore.tstests-ui/tests/store/executionStore.test.ts
src/**/*.{vue,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.{vue,ts}: Leverage VueUse functions for performance-enhancing styles
Implement proper error handling
Use vue-i18n in composition API for any string literals. Place new translation entries in src/locales/en/main.json
Files:
src/stores/executionStore.ts
src/**/*.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.ts: Use es-toolkit for utility functions
Use TypeScript for type safety
Files:
src/stores/executionStore.ts
**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit inference engine (CLAUDE.md)
Use camelCase for variable and setting names in TypeScript/Vue files
Files:
src/stores/executionStore.tstests-ui/tests/store/executionStore.test.ts
**/*.{ts,tsx,vue}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,vue}: Useconst settingStore = useSettingStore()andsettingStore.get('Comfy.SomeSetting')to retrieve settings in TypeScript/Vue files
Useawait settingStore.set('Comfy.SomeSetting', newValue)to update settings in TypeScript/Vue files
Check server capabilities usingapi.serverSupportsFeature('feature_name')before using enhanced features
Useapi.getServerFeature('config_name', defaultValue)to retrieve server feature configurationEnforce ESLint rules for Vue + TypeScript including: no floating promises, no unused imports, and i18n raw text restrictions in templates
Files:
src/stores/executionStore.tstests-ui/tests/store/executionStore.test.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Define dynamic setting defaults using runtime context with functions in settings configuration
UsedefaultsByInstallVersionproperty for gradual feature rollout based on version in settings configuration
Files:
src/stores/executionStore.tstests-ui/tests/store/executionStore.test.ts
src/**/*.{ts,tsx,vue}
📄 CodeRabbit inference engine (src/CLAUDE.md)
src/**/*.{ts,tsx,vue}: Sanitize HTML with DOMPurify to prevent XSS attacks
Avoid using @ts-expect-error; use proper TypeScript types instead
Use es-toolkit for utility functions instead of other utility libraries
Implement proper TypeScript types throughout the codebase
Files:
src/stores/executionStore.ts
src/**/stores/**/*.{ts,tsx}
📄 CodeRabbit inference engine (src/CLAUDE.md)
src/**/stores/**/*.{ts,tsx}: Maintain clear public interfaces and restrict extension access in stores
Use TypeScript for type safety in state management stores
Files:
src/stores/executionStore.ts
src/**/*.{vue,ts,tsx}
📄 CodeRabbit inference engine (src/CLAUDE.md)
Follow Vue 3 composition API style guide
Files:
src/stores/executionStore.ts
**/stores/*Store.ts
📄 CodeRabbit inference engine (AGENTS.md)
Name Pinia stores with the
*Store.tssuffix
Files:
src/stores/executionStore.ts
tests-ui/**/*.test.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (tests-ui/CLAUDE.md)
tests-ui/**/*.test.{js,ts,jsx,tsx}: Write tests for new features
Follow existing test patterns in the codebase
Use existing test utilities rather than writing custom utilities
Mock external dependencies in tests
Always prefer vitest mock functions over writing verbose manual mocks
Files:
tests-ui/tests/store/executionStore.test.ts
**/*.{test,spec}.{ts,tsx,js}
📄 CodeRabbit inference engine (AGENTS.md)
Unit and component tests should be located in
tests-ui/or co-located with components assrc/components/**/*.{test,spec}.ts; E2E tests should be inbrowser_tests/
Files:
tests-ui/tests/store/executionStore.test.ts
🧠 Learnings (12)
📓 Common learnings
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: src/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:34.324Z
Learning: Proper error propagation in service layer and state management
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: src/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:34.324Z
Learning: Implement proper error handling in stores
📚 Learning: 2025-11-24T19:47:45.616Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: src/components/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:45.616Z
Learning: Applies to src/components/**/*.vue : Use ref/reactive for state management in Vue 3 Composition API
Applied to files:
src/stores/executionStore.ts
📚 Learning: 2025-11-24T19:47:14.779Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:14.779Z
Learning: Applies to **/*.{ts,tsx,vue} : Use `const settingStore = useSettingStore()` and `settingStore.get('Comfy.SomeSetting')` to retrieve settings in TypeScript/Vue files
Applied to files:
src/stores/executionStore.ts
📚 Learning: 2025-11-24T19:47:45.616Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: src/components/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:45.616Z
Learning: Applies to src/components/**/*.vue : Implement computed() for derived state in Vue 3 Composition API
Applied to files:
src/stores/executionStore.ts
📚 Learning: 2025-11-24T19:47:02.860Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T19:47:02.860Z
Learning: Applies to src/**/*.vue : Utilize ref and reactive for reactive state
Applied to files:
src/stores/executionStore.ts
📚 Learning: 2025-11-24T19:46:52.279Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: .cursorrules:0-0
Timestamp: 2025-11-24T19:46:52.279Z
Learning: Applies to **/*.vue : Utilize ref and reactive for reactive state in Vue 3
Applied to files:
src/stores/executionStore.ts
📚 Learning: 2025-11-24T19:47:56.371Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: src/lib/litegraph/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:56.371Z
Learning: Applies to src/lib/litegraph/**/*.{test,spec}.{ts,tsx} : Use provided test helpers `createTestSubgraph` and `createTestSubgraphNode` from `./fixtures/subgraphHelpers` for consistent subgraph test setup
Applied to files:
tests-ui/tests/store/executionStore.test.ts
📚 Learning: 2025-11-24T19:48:03.270Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: tests-ui/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:48:03.270Z
Learning: Applies to tests-ui/**/*.test.{js,ts,jsx,tsx} : Write tests for new features
Applied to files:
tests-ui/tests/store/executionStore.test.ts
📚 Learning: 2025-11-24T19:47:56.371Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: src/lib/litegraph/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:56.371Z
Learning: Applies to src/lib/litegraph/**/*.{test,spec}.{ts,tsx} : When writing tests for subgraph-related code, always import from the barrel export at `@/lib/litegraph/src/litegraph` to avoid circular dependency issues
Applied to files:
tests-ui/tests/store/executionStore.test.ts
📚 Learning: 2025-11-24T19:47:22.909Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: browser_tests/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:22.909Z
Learning: Applies to browser_tests/**/*.{e2e,spec}.{ts,tsx,js,jsx} : Test user workflows in browser tests
Applied to files:
tests-ui/tests/store/executionStore.test.ts
📚 Learning: 2025-11-24T19:47:56.371Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: src/lib/litegraph/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:56.371Z
Learning: Applies to src/lib/litegraph/**/*.{test,spec}.{js,ts,jsx,tsx} : When adding features, always write vitest unit tests using cursor rules in @.cursor
Applied to files:
tests-ui/tests/store/executionStore.test.ts
📚 Learning: 2025-11-24T19:47:34.324Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: src/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:34.324Z
Learning: Applies to src/**/stores/**/*.{ts,tsx} : Maintain clear public interfaces and restrict extension access in stores
Applied to files:
tests-ui/tests/store/executionStore.test.ts
🧬 Code graph analysis (1)
src/stores/executionStore.ts (4)
src/types/nodeIdentification.ts (1)
NodeLocatorId(18-18)src/types/index.ts (2)
NodeLocatorId(34-34)NodeError(48-48)src/scripts/app.ts (2)
lastNodeErrors(204-206)app(1678-1678)src/utils/graphTraversalUtil.ts (2)
forEachNode(156-169)getNodeByExecutionId(285-307)
🪛 ESLint
src/stores/executionStore.ts
[error] 2-2: Unable to resolve path to module 'vue'.
(import-x/no-unresolved)
[error] 35-35: Unable to resolve path to module '@/utils/graphTraversalUtil'.
(import-x/no-unresolved)
🔇 Additional comments (2)
src/stores/executionStore.ts (2)
247-283: Node error mapping and slot helper look correct; watch for potential ID collisionsThe
nodeErrorsByLocatorId,getNodeErrors, andslotHasErrorlogic is clean and matches the expectedNodeErrorshape, including safe handling of missingextra_info. One thing to double‑check: if multiple execution IDs can legitimately map to the sameNodeLocatorId(e.g. multiple instances of the same subgraph), this implementation will silently let the last one win in themap[locatorId] = nodeErrorassignment. If that scenario is possible, you may want to either aggregate errors or make that overwrite explicit in a comment so the behavior is intentional.
579-582: Public store API additions are cohesiveExposing
nodeLocatorIdToExecutionId,getNodeErrors, andslotHasErrorhere makes the execution store a clear single source of truth for execution↔locator conversions and error lookups, which fits the “clear public interfaces” guidance for stores. No issues from an API surface perspective.
| getNodeErrors, | ||
| slotHasError |
There was a problem hiding this comment.
Are these just exposed for the test?
If so, is there a way to test the behavior through the public API?
There was a problem hiding this comment.
DrJKL
left a comment
DrJKL
left a comment
There was a problem hiding this comment.
Testing internal logic piece can (and should) be fixed later.
2025-11-13.17-59-01_processed_20251113_175953.mp4
┆Issue is synchronized with this Notion page by Unito