-
Notifications
You must be signed in to change notification settings - Fork 672
fix: keep execution errors scoped to the workflow that produced them #15361
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
d3873c7
9fd8f04
ad4d328
81126fb
3ba549a
fc67551
04b4ce1
329dc10
1dc34bd
bcb4313
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -126,6 +126,7 @@ import { useMissingMediaStore } from '@/platform/missingMedia/missingMediaStore' | |
|
|
||
| import { getWorkflowMode } from '@/utils/appMode' | ||
| import { anyItemOverlapsRect } from '@/utils/mathUtil' | ||
| import { createUuidv4, zeroUuid } from '@/utils/uuid' | ||
| import { | ||
| collectAllNodes, | ||
| forEachNode, | ||
|
|
@@ -2425,7 +2426,7 @@ export class ComfyApp { | |
| const nodeOutputStore = useNodeOutputStore() | ||
| nodeOutputStore.resetAllOutputsAndPreviews() | ||
| const executionErrorStore = useExecutionErrorStore() | ||
| executionErrorStore.clearRunErrors() | ||
| executionErrorStore.setActiveGraph(null) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SHOULD FIX:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 81126fb. Confirmed the exact path you describe:
Store test added ("closes the error overlay when the active graph changes"); verified it fails on the previous commit.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Issue: not every Suggestion: have the standalone Clear paths re-attach (they still hold a root graph with a valid id), or make
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in 3ba549a. |
||
| useMissingNodesErrorStore().setMissingNodeTypes([]) | ||
|
|
||
| useDomWidgetStore().clear() | ||
|
|
@@ -2434,7 +2435,12 @@ export class ComfyApp { | |
| // (`LGraph`) `clear` breaks the subgraph structure. | ||
| if (this.rootGraph && !this.canvas.subgraph) { | ||
| this.rootGraph.clear() | ||
| if (this.rootGraph.id === zeroUuid) { | ||
| this.rootGraph.id = createUuidv4() | ||
| } | ||
| } | ||
|
|
||
| executionErrorStore.setActiveGraph(this.rootGraph?.id ?? null) | ||
|
Comment on lines
2446
to
+2453
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win Extract the zero-id minting rule into one shared helper. Lines 2448-2450 duplicate the rule in ♻️ Proposed shared helperAdd to import type { UUID } from '`@/utils/uuid`'
/** Returns the graph id, minting one when the graph still carries the zero id. */
export function ensureNonZeroGraphId(graph: { id: UUID }): UUID {
if (graph.id === zeroUuid) graph.id = createUuidv4()
return graph.id
}Then in if (this.rootGraph && !this.canvas.subgraph) {
this.rootGraph.clear()
- if (this.rootGraph.id === zeroUuid) {
- this.rootGraph.id = createUuidv4()
- }
+ ensureNonZeroGraphId(this.rootGraph)
}🤖 Prompt for AI Agents |
||
| } | ||
|
|
||
| clientPosToCanvasPos(pos: Vector2): Vector2 { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue: minting only for the zero id leaves a collision for non-zero duplicates.
_configureBasepreserves any serialized non-zero id, and Save-As/Duplicate are the only flows that regenerate one — re-dropping the same saved/exported JSON while its first tab is inactive dedupes the path (file (2).json) but not the id, so two open tabs share onerunErrorsByGraphIdentry: the twin tab shows the other tab's run errors, and clearing via one destroys the other's.subgraphNavigationStorehit the same shape and keys byworkflowPath:graphId(with a same-graph-id/different-workflow test); the same composite key would work here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in fc67551. Run-error buckets now use the composite workflow path + root graph ID key, matching the isolation shape used by subgraph navigation. Two open workflows with the same serialized graph ID no longer share or clear each other’s errors; the store test covers that collision directly.