Comfy-Org / Comfy-Org/ComfyUI_frontend
ChangeTracker.reset() re-baselines initialState without recomputing isModified
- Dominant language
- TypeScript
- Stars
- 2k
- Forks
- 704
- Avg merge
- 1d 8h
- Merged PRs (30d)
- 512
Description
## Summary
`ChangeTracker.reset()` is documented as "Save the current state as the initial state", but it re-baselines `initialState` only. `workflow.isModified` is derived from `initialState` by `updateModified()`, and `reset()` never calls it — so after a reset the workflow keeps a stale dirty flag until some unrelated graph change happens to recompute it.
## The code
`src/scripts/changeTracker.ts`:
```ts
reset(state?: ComfyWorkflowJSON) {
if (this._restoringState) return
if (state) this.activeState = clone(state)
this.initialState = clone(this.activeState)
}
```
and the only place `isModified` is derived:
```ts
updateModified(previousState?: ComfyWorkflowJSON) {
const workflow = useWorkflowStore().getWorkflowByPath(this.workflow.path)
if (workflow) {
workflow.isModified = !ChangeTracker.graphEqual(
this.initialState,
this.activeState
)
}
...
}
```
After `reset()`, `initialState` and `activeState` are equal by construction, so `isModified` *should* be `false` — but nothing recomputes it, so it stays whatever it was.
## How it surfaced
`browser_tests/tests/assetDeleteClearsLoadImage.spec.ts` re-baselines the tracker mid-test and then asserts the workflow is clean:
```ts
tracker?.reset?.()
await expect.poll(() => comfyPage.workflow.isCurrentWorkflowModified()).toBe(false)
```
That assertion only ever held because the upload preceding it never marked the workflow modified — which was itself the bug fixed in https://github.com/Comfy-Org/ComfyUI_frontend/pull/15069. Once uploads correctly mark the workflow dirty, the `reset()` stopped clearing the flag and the test failed on the `cloud` project. It is worked around there by calling `updateModified()` after `reset()`; the underlying asymmetry is untouched.
So the test was silently resting on a product bug, and `reset()`'s incomplete semantics hid it.
## Impact
Any caller that re-baselines a tracker and expects a clean workflow gets a stale dirty flag: a spurious unsaved-changes dot, and a spurious "unsaved changes" prompt on close/navigate. `reset()` is called from workflow load/activation paths, so this is not test-only.
## Suggested fix
Have `reset()` recompute the flag it invalidates:
```ts
reset(state?: ComfyWorkflowJSON) {
if (this._restoringState) return
if (state) this.activeState = clone(state)
this.initialState = clone(this.activeState)
this.updateModified()
}
```
Note `updateModified()` also dispatches `graphChanged`, so this needs a check that the extra event is harmless on every `reset()` path (it drives the workflow-persistence debounce), or the flag update should be factored out of `updateModified()` so `reset()` can set it without emitting.
Deliberately not folded into #15069 — it changes shared change-tracker behaviour well outside that PR's intent.
Contributor guide
Research direction
Start in src/scripts/changeTracker.ts by reading reset() and updateModified(), then inspect browser_tests/tests/assetDeleteClearsLoadImage.spec.ts and reset() callers. Check how graphChanged affects workflow persistence on reset paths. Done means re-baselining leaves the workflow unmodified without introducing harmful side effects, and the referenced browser test passes.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- typescript
- Domain
- frontend
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 68/100