fix: auto-close files setting cannot be unchecked — always reverts to checked#668
Conversation
The autoCloseZooOpenedFiles, autoCloseZooOpenedFilesAfterUserEdited, and autoCloseZooOpenedNewFiles settings were saved to global state but never included in the state posted back to the webview (getStateToPostToWebview) or returned by getState. This caused the checkbox to always revert to checked after saving, and the DiffViewProvider to always use the default value regardless of the user's preference. Also adds a changeset instruction to AGENTS.md. Closes #667
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThree auto-close settings ( ChangesAuto-close Settings State Serialization Fix
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
| await provider.contextProxy.setValue("autoCloseZooOpenedFiles", false) | ||
|
|
||
| const state = await provider.getState() | ||
|
|
||
| // DiffViewProvider reads from getState(); the field must be present. | ||
| expect(state.autoCloseZooOpenedFiles).toBe(false) |
There was a problem hiding this comment.
Does this test need assertions for autoCloseZooOpenedFilesAfterUserEdited and autoCloseZooOpenedNewFiles too? Since the bug was about all three fields being missing from getState(), a regression that drops those two would pass this test as-is.
| await provider.contextProxy.setValue("autoCloseZooOpenedFiles", false) | |
| const state = await provider.getState() | |
| // DiffViewProvider reads from getState(); the field must be present. | |
| expect(state.autoCloseZooOpenedFiles).toBe(false) | |
| await provider.contextProxy.setValue("autoCloseZooOpenedFiles", false) | |
| await provider.contextProxy.setValue("autoCloseZooOpenedFilesAfterUserEdited", true) | |
| await provider.contextProxy.setValue("autoCloseZooOpenedNewFiles", true) | |
| const state = await provider.getState() | |
| // DiffViewProvider reads from getState(); the field must be present. | |
| expect(state.autoCloseZooOpenedFiles).toBe(false) | |
| expect(state.autoCloseZooOpenedFilesAfterUserEdited).toBe(true) | |
| expect(state.autoCloseZooOpenedNewFiles).toBe(true) |
| }) | ||
| }) | ||
|
|
||
| describe("auto-close settings are included in posted state", () => { |
There was a problem hiding this comment.
Is there a reason these tests live in ClineProvider.taskHistory.spec.ts rather than ClineProvider.spec.ts? The existing getState settings coverage (e.g. autoCondenseContext, writeDelayMs) is in the latter, so a contributor looking for where settings persistence is tested would miss these.
…ert all three fields Address review comments on PR #668: - Move the auto-close settings test block from ClineProvider.taskHistory.spec.ts to ClineProvider.spec.ts, where existing getState settings coverage (autoCondenseContext, writeDelayMs) already lives. - Expand the getState regression test to assert autoCloseZooOpenedFiles, autoCloseZooOpenedFilesAfterUserEdited, and autoCloseZooOpenedNewFiles so a regression dropping any of the three fields is caught.
Related GitHub Issue
Closes: #667
Description
The three auto-close settings (
autoCloseZooOpenedFiles,autoCloseZooOpenedFilesAfterUserEdited,autoCloseZooOpenedNewFiles) were saved to global state viacontextProxy.setValue()but never included in the state posted back to the webview or returned bygetState(). This had two effects:UI symptom:
getStateToPostToWebview()omitted these fields, so the webview always receivedundefined. When the SettingsView re-mounted,cachedStatewas initialized fromextensionState(withundefined), and the checkbox renderedchecked={undefined ?? true}→ always checked. The user could uncheck and save, but the value was never sent back, so it appeared unsaved on reopen.Functional symptom:
getState()omitted these fields, so theDiffViewProvider(which readssaveState?.autoCloseZooOpenedFiles ?? true) always gotundefinedand fell back to the defaulttrue— the auto-close behavior ignored the user's saved preference entirely.Fix: Added the three fields to both
getState()(reading fromstateValues) andgetStateToPostToWebview()(destructuring fromgetState()and including in the return object with their documented defaults:true,false,false).Also added a changeset instruction to
AGENTS.mdper maintainer request.Test Procedure
Unit tests (added to
src/core/webview/__tests__/ClineProvider.taskHistory.spec.ts):getStateToPostToWebview returns saved autoCloseZooOpenedFiles value— Sets values viacontextProxy.setValue(), callsgetStateToPostToWebview(), and asserts the saved values are present in the returned state.getStateToPostToWebview defaults autoCloseZooOpenedFiles to true when unset— Verifies unset values default totrue/false/falsein posted state.getState returns saved autoCloseZooOpenedFiles value for DiffViewProvider— VerifiesgetState()returns the saved value soDiffViewProvidercan read it.Run tests:
All 188 tests pass (18 + 170). Lint and type-check also pass.
Pre-Submission Checklist
Screenshots / Videos
N/A — no visual UI changes; the fix corrects data flow so the existing checkbox behaves correctly.
Documentation Updates
Additional Notes
The root cause was a straightforward omission: the fields were added to the
ExtensionStatetype andglobalSettingsSchema, the webview correctly sends them inupdateSettings, and theDiffViewProvidercorrectly reads them fromgetState()— but thegetState()andgetStateToPostToWebview()return objects were never updated to include them. Since all fields areoptionalin the type, TypeScript did not flag the missing properties.Get in Touch
N/A
Summary by CodeRabbit
Release Notes
New Features
Tests