Fix: restore the tab strip when a split view is exited - #76
Merged
Merged
Conversation
Exiting a split left the agents that had been in it with no tab at all. Their headers were collapsed — correctly, while they were on screen with their names on the pane headers — and nothing un-collapsed them, so the agents became unreachable until something else happened to refresh the strip. RefreshTabStrip owns that rule, and ExitSplit never called it. Closing a pane looked fine only by accident: RemovePane routes through SelectTab, which refreshes on the way past. Fixed at the choke point rather than at the call site. The strip lists what is NOT on screen, so it is a function of the pane set, and ApplyPaneLayout is the one method every pane-set change goes through. Refreshing there makes the two impossible to leave out of step; fixing only ExitSplit would have left the same trap for the next person to add a pane operation. An audit of all seven places that mutate the pane set found three more relying on an indirect refresh, and one — ValidateSplit, which runs when an agent is closed — with no refresh on either branch. This also re-divides the strip's width. LayoutTabStrip shares the viewport among visible headers only, so without the refresh the restored tabs would have kept widths computed while they were hidden. The bug was introduced with the pane headers and has not shipped, so there is no changelog entry.
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.
Summary
Leaving a split view left the agents that had been in it with no tab, so there was no way back to
them. One line, at the point where every pane-set change already passes through.
What was happening
While an agent is in a pane, its tab is deliberately hidden — its name is on the pane header, so the
strip goes back to being a list of what is not currently visible. That part is correct.
On Exit Split View, nothing put those tabs back.
RefreshTabStripowns that visibility rule andExitSplitnever called it, so the headers stayed collapsed while the panes they belonged to weregone. The agents were still open and still loaded — simply unreachable until something unrelated
happened to refresh the strip.
Closing a pane with its [x] looked fine, which is what made this confusing to place. That path
is not better-written; it just routes through
SelectTab, which happens to refresh on the way past.What changed
ApplyPaneLayout. The strip lists what is not on screen, so it is afunction of the pane set — and
ApplyPaneLayoutis the single method every pane-set change goesthrough. Putting it there makes the two impossible to leave out of step.
ExitSplitwas the obvious smaller change, and is why I did not do it. The sameomission would be waiting for the next person who adds a pane operation. An audit of all seven
places that mutate the pane set found three more relying on an indirect refresh, and one —
ValidateSplit, which runs whenever an agent is closed — with no refresh on either branch.LayoutTabStripdivides the viewport among visible headers only,so restoring visibility alone would have left the returned tabs at widths calculated while they
were hidden.
Scope and risk
Low, with one thing worth a reviewer's eye.
RefreshTabStripnow runs more often — two or three times per tab selection, sinceSelectTabandSwitchPageboth still call it explicitly and both reachApplyPaneLayout. I left those calls inplace rather than pruning them, to keep this diff to the fix. The repeats are cheap and idempotent:
the method sets brushes and visibility on a handful of headers, and
LayoutTabStripis arithmeticplus a
Widthassignment. Checked for recursion —RefreshTabStripdoes not callApplyPaneLayout,SelectTab, orSwitchPage, so there is no cycle.Verification
dotnet build src/MandoCode.Desktop— succeeded, 0 warnings, 0 errors.dotnet test src/MandoCode.Desktop.Tests— 490 passed, 0 failed.RefreshTabStrip.Not covered, and this is the honest part: no automated test catches this. Split view has no UI
coverage —
PaneLayoutTestspins the grid geometry and nothing else. More to the point, a unit testwould not have helped: the rule was always right, it simply was not run, and a test of the rule
passes identically before and after this fix. Making the coupling structural is the mitigation,
not the test suite.
So this needs a manual pass:
both tabs should be correct (this is the
ValidateSplitpath, which had no refresh at all).Changelog
Deliberately none. The bug arrived with the pane headers in #73, which is still in
[Unreleased]and has never been tagged, so it never reached a user — the repo's convention is not to log fixes
for bugs that never shipped, and the feature entry already describes the intended behaviour.