Comfy-Org / Comfy-Org/ComfyUI_frontend

ChangeTracker.reset() re-baselines initialState without recomputing isModified

Open
#15,122 1 comment 1 reaction 0 assignees View on GitHub
Potential Bug
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.