mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-04-19 22:09:37 +00:00
fix: address CodeRabbit review findings
- Guard deactivate() against inactive tracker to prevent store() from freezing wrong viewport/output data into the old tracker - Update docs: workflowService calls captureCanvasState indirectly via deactivate/prepareForSave, not directly - Add deprecated checkState() to mock utility for extension compat - Update deactivate test: inactive path is now a full no-op
This commit is contained in:
@@ -59,7 +59,7 @@ useWorkflowStore().activeWorkflow?.changeTracker?.captureCanvasState()
|
||||
|
||||
### Existing Manual Call Sites
|
||||
|
||||
These locations already call `captureCanvasState()` explicitly:
|
||||
These locations call `captureCanvasState()` directly:
|
||||
|
||||
- `WidgetSelectDropdown.vue` — After dropdown selection and file upload
|
||||
- `ColorPickerButton.vue` — After changing node colors
|
||||
@@ -71,7 +71,9 @@ These locations already call `captureCanvasState()` explicitly:
|
||||
- `useSubgraphOperations.ts` — After subgraph enter/exit
|
||||
- `useCanvasRefresh.ts` — After canvas refresh
|
||||
- `useCoreCommands.ts` — After metadata/subgraph commands
|
||||
- `workflowService.ts` — After workflow service operations
|
||||
|
||||
`workflowService.ts` calls `captureCanvasState()` indirectly via
|
||||
`deactivate()` and `prepareForSave()` (see Lifecycle Methods above).
|
||||
|
||||
## Lifecycle Methods
|
||||
|
||||
|
||||
@@ -223,14 +223,14 @@ describe('ChangeTracker', () => {
|
||||
expect(mockSubgraphNavigationStore.exportState).toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('still stores viewport when captureCanvasState is no-op (inactive)', () => {
|
||||
it('is a full no-op when called on inactive tracker', () => {
|
||||
const tracker = createTracker()
|
||||
mockWorkflowStore.activeWorkflow = { changeTracker: {} }
|
||||
|
||||
tracker.deactivate()
|
||||
|
||||
expect(app.rootGraph.serialize).not.toHaveBeenCalled()
|
||||
expect(mockNodeOutputStore.snapshotOutputs).toHaveBeenCalled()
|
||||
expect(mockNodeOutputStore.snapshotOutputs).not.toHaveBeenCalled()
|
||||
})
|
||||
})
|
||||
|
||||
|
||||
@@ -97,10 +97,20 @@ export class ChangeTracker {
|
||||
* Called exactly once when switching away from this workflow.
|
||||
*
|
||||
* PRECONDITION: must be called while this workflow is still the active one
|
||||
* (before the activeWorkflow pointer is moved). captureCanvasState() will
|
||||
* no-op if called after the pointer has already moved.
|
||||
* (before the activeWorkflow pointer is moved). If called after the pointer
|
||||
* has already moved, this is a no-op to avoid freezing wrong viewport data.
|
||||
*/
|
||||
deactivate() {
|
||||
const isActive = useWorkflowStore().activeWorkflow?.changeTracker === this
|
||||
if (!isActive) {
|
||||
if (import.meta.env.DEV) {
|
||||
logger.error(
|
||||
'deactivate() called on inactive tracker for:',
|
||||
this.workflow.path
|
||||
)
|
||||
}
|
||||
return
|
||||
}
|
||||
this.captureCanvasState()
|
||||
this.store()
|
||||
}
|
||||
|
||||
@@ -257,6 +257,7 @@ export function createMockChangeTracker(
|
||||
redoQueue: [],
|
||||
changeCount: 0,
|
||||
captureCanvasState: vi.fn(),
|
||||
checkState: vi.fn(),
|
||||
deactivate: vi.fn(),
|
||||
prepareForSave: vi.fn(),
|
||||
reset: vi.fn(),
|
||||
|
||||
Reference in New Issue
Block a user