sessions: scope the side-pane collapse marker to the collapsed session#323360
Merged
Conversation
Address Copilot code review on #323345: deriving auxiliaryBarHiddenByCollapse from a controller-global _sidePaneToggledClosed flag read at arbitrary capture times was leaky. It required resetting the flag on session switch (breaking the documented invariant) and could still clobber an explicit aux-bar hide on a different session, turning it into a collapse. Record the collapse marker for the active session directly in _onSidePaneToggled (where the collapse actually happens), and have _captureViewState only preserve an existing marker while the aux bar stays hidden — never fabricate one. The global flag is no longer read anywhere, so remove it along with the session-switch and aux-bar-change resets. Add a regression test asserting an explicit aux-bar hide is not turned into a collapse when another session is collapsed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
📬 CODENOTIFYThe following users are being notified based on files changed in this PR: @benibenjMatched files:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refines per-session side-pane state tracking in the Agents window desktop layout controller so the auxiliaryBarHiddenByCollapse marker is recorded only for the session that was actually collapsed, preventing the marker from leaking across sessions. It also adds a regression test and updates the spec to reflect the scoped behavior.
Changes:
- Record collapse/expand markers directly in
_onSidePaneToggledfor the active session, and update_captureViewStateto preserve (not fabricate) an existing collapse marker. - Remove the controller-global
_sidePaneToggledClosedflag from the base controller and its associated resets. - Add a regression unit test and update
desktopSessionLayoutController.mdto document the scoped collapse marker behavior.
Show a summary per file
| File | Description |
|---|---|
| src/vs/sessions/contrib/layout/test/browser/desktopSessionLayoutController.test.ts | Adds regression coverage ensuring a collapse marker does not leak across sessions and convert an explicit aux-bar hide into a collapse. |
| src/vs/sessions/contrib/layout/browser/desktopSessionLayoutController.ts | Moves collapse-marker recording to the toggle hook and changes save-time capture to preserve markers per session. |
| src/vs/sessions/contrib/layout/browser/desktopSessionLayoutController.md | Updates the D9/D9b spec narrative to match the new per-session marker scoping. |
| src/vs/sessions/contrib/layout/browser/baseSessionLayoutController.ts | Removes the no-longer-used global _sidePaneToggledClosed tracking from the base toggle implementation. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Low
… bar Address Copilot code review on #323360: _onSidePaneToggled set the collapse marker whenever the aux bar ended up hidden after a toggle. But re-opening the side pane can restore an editor-only state (aux bar explicitly hidden before the collapse), which wrongly stamped that explicit hide as collapse-driven and made opening Changes re-reveal the aux bar. Pass collapsed + previousAuxiliaryBarVisible to the hook and only mark the collapse when the toggle fully collapsed a previously-visible aux bar; any other outcome captures the resulting state without fabricating a marker, preserving an explicit aux-bar hide. Add a regression test for the collapse + editor-only re-open case. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
vijayupadya
approved these changes
Jun 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Follow-up to #323345 addressing two Copilot Code Review comments. The previous change derived the per-session
auxiliaryBarHiddenByCollapsemarker from a controller-global_sidePaneToggledClosedflag that was read at arbitrary save-time capture points. That was leaky:How
_onSidePaneToggled— at the moment the collapse happens, scoped to the session it happened on. For a created session, a collapse writes its view state withauxiliaryBarHiddenByCollapse: true; an expand writes{ auxiliaryBarVisible: true }._captureViewStatenow only preserves an existing marker while the aux bar stays hidden, and never fabricates one from a global flag. An explicit aux-bar hide (no prior marker) therefore stays explicit._sidePaneToggledClosedflag is no longer read anywhere, so it (and the session-switch / aux-bar-change resets that maintained it) is removed.Net effect is identical user-facing behavior for the original fix (reload + open Changes after collapsing re-reveals the side pane), but the collapse marker can no longer leak across sessions.
Notes for reviewers
[D9]reload tests, plus a new regression test:[D9] does not turn an explicit aux-bar hide into a collapse when another session is collapsed(verified it fails if_captureViewStatefabricates the marker).desktopSessionLayoutController.md(D9/D9b) updated to describe the scoped marker.Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com