Repository navigation
Right column: measure panes by their final layout width, so an open right after a split is a tab, not a split below - #30010
Conversation
…l widths Review of #29787 at 2986104: LinkOpener.width(of:) reads a pane's view bounds. While a split's spring runs, the source still shows its width from before the split and the new pane a zero-width sliver, so in a window narrower than twice the chat floor a link (or an agent's side open through rightColumn(beside:)) opened right after the split went to a split below the terminal-only right pane instead of a tab in it. The test is the layout's clock (driver.isManual), so the spring stays at its first frame and the race is the same on every host. Red first: fails at this commit.
… path did not compile in the macro) Fleet job cc5d395258652518fd0cb650 at abe70fc: RightColumnAfterSplitTests.swift:55 'call can throw, but it is not marked with try' inside the #require expansion. Same assertion, no behavior change.
…ir view bounds LinkOpener.width(of:), the one width every open on the right reads (links through target(from:), agent side chats, subagents and beside moves through rightColumn(beside:)), read the pane view's bounds. Right after a split the source still shows its width from before the split until the spring lands, and the new pane starts as a zero-width sliver (nil, no floor). In a window narrower than twice the chat floor a link or agent open right after a split went to a split below the terminal-only right pane instead of a tab in it. It now reads LayoutRootView.restingContentWidth(of:): the pane's frame in the geometry its screen's springs head for (the same geometry the right column is read from), less the pane padding. An O(1) lookup into geometry computed when the layout syncs; no layout pass, no wait.
|
cmux status (one comment, edited in place; each line is updated by the job that owns it) Review App of this head (app.zip, kept 1 day)
Default changesThis pull request changes 1 default at
Signed off on this commit by Merge receipt: merged as Regenerate the snapshot with Shipped in:
|
|
Review (independent review subagent, Opus 5.5 claude-opus-5-5, read-only): SHIP at 3af8e0a The reviewer confirmed:
Low notes, not blocking:
Owner: Fleet: red |
There was a problem hiding this comment.
1 issue found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Packages/macOS/CmuxNext/Tests/CmuxNextAppTests/RightColumnAfterSplitTests.swift">
<violation number="1" location="Packages/macOS/CmuxNext/Tests/CmuxNextAppTests/RightColumnAfterSplitTests.swift:66">
P2: This does not verify agent placement, only the measurement snapshot and pure policy decision. Drive one real side-chat or subagent open after `splitRight` and assert its tab lands in `right`; otherwise regressions in `openOnRight` or the agent callbacks can pass this test.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| "the source read \(String(describing: LinkOpener.width(of: pane))) pt after the split, its width before it") | ||
|
|
||
| // Agent opens (AgentSidePanes, AgentSubagentPanes, AgentBesidePlacement) measure through `rightColumn`. | ||
| let measured = services.agentTabs.rightColumn(sourceTab) |
There was a problem hiding this comment.
P2: This does not verify agent placement, only the measurement snapshot and pure policy decision. Drive one real side-chat or subagent open after splitRight and assert its tab lands in right; otherwise regressions in openOnRight or the agent callbacks can pass this test.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At Packages/macOS/CmuxNext/Tests/CmuxNextAppTests/RightColumnAfterSplitTests.swift, line 66:
<comment>This does not verify agent placement, only the measurement snapshot and pure policy decision. Drive one real side-chat or subagent open after `splitRight` and assert its tab lands in `right`; otherwise regressions in `openOnRight` or the agent callbacks can pass this test.</comment>
<file context>
@@ -0,0 +1,106 @@
+ "the source read \(String(describing: LinkOpener.width(of: pane))) pt after the split, its width before it")
+
+ // Agent opens (AgentSidePanes, AgentSubagentPanes, AgentBesidePlacement) measure through `rightColumn`.
+ let measured = services.agentTabs.rightColumn(sourceTab)
+ #expect(RightColumnPlacement.decide(measured) == .tab(right.layoutPaneID.rawValue),
+ "an agent's open right after the split: \(measured)")
</file context>
|
Found 2 test failures on Blacksmith runners: Failures
|
![Fix with [code]smith](https://pr-comments-assets.blacksmith.sh/codesmith/fix-with-codesmith-light-3x.png)
Summary
Right after a split, a link or an agent open beside a pane could land as a new split below the pane on the right. It should be a tab in that pane. This happened in windows narrower than about 960 pt, and only while the split's spring ran, so scripts and agents that split and then open at once hit it most.
Cause:
LinkOpener.width(of:)read the pane view's bounds. While the split's spring runs, the source still shows its width from before the split, because its content is held at that size until the spring lands. The new pane starts as a zero-width sliver, which reads as nil, and nil sets no width limit. SoRightColumnPlacement.decidesaw a wide source and an unmeasured right pane, and pickedsplitBelow. With both panes measured at their final widths, it pickstab. Found by the independent review of #29787.Fix:
width(of:)now readsLayoutRootView.restingContentWidth(of:). This is the pane's frame in the geometry its screen's springs head for (thebaseGeometrythe right column is already read from), less the pane padding. The width now comes from one place, so both entry points get the fix:LinkOpener.target(from:), which coversopenWeb,openFile,openFolderand chat-dock tools throughChatColumnPlacement.openToolLinkOpener.rightColumn(beside:), which coversAgentTabStore.rightColumn(AgentSidePanes, AgentSubagentPanes) andAgentBesidePlacementAction budget: the new read is a dictionary lookup into geometry that is computed when the layout syncs. It adds no layout pass and no wait to a handler. A pane that no screen view lays out (a pane with no workspace, as in
SideOpenStripTests) still reads nil, as before.Owners:
LinkOpenerandRightColumnPlacementhave no OWNERS row, so they go to the Performance Lead. The placement rule is Leo's (#28989, #29684) and is unchanged.LayoutRootView.swiftis in the "Layout motion" row (PL-motion-core); the only change there is one new read-only accessor.Testing
cmux-ci run --class light --display --script scripts/ci/package-test-lane.sh --arg=suite --arg=Packages/macOS/CmuxNext --arg=RightColumnAfterSplitTests --env CMUX_TEST_REQUIRE_GUI=1New test
RightColumnAfterSplitTests.anOpenRightAfterASplitReadsThePanesFinalWidths. It uses a same-tree daemon and a 940 pt window. The test drives the layout clock itself (driver.isManual), so the spring stays at its first frame on every host. It splits right, then at once:services.agentTabs.rightColumnIt expects a tab in the terminal-only right pane and no new split.
Red at
ade9546b00de(test only, merged with feat-cmux-next), job2f3bc17c13cb08a78ffb1630on cmuxs-Mac-mini-4. Three failures:Column(sourceWidth: 720, widths: [:]), decided assplitBelowsplitBelowGreen at
3af8e0a05961(head), joba6ce3d5aa1dfab7b603114dfon cmux13s-Mac-mini. These suites passed:RightColumnAfterSplitTests,RightColumnPlacementTests,LinkOpenPlacementTests,RightColumnTurnTests,SideOpenStripTests,RightColumnScreenTests,RightColumnEntryPointTests.Display proof at the head
3af8e0a05961: job0b3b961d1c808367eb9bf7f5(gui-session-v1, cmux8s-Mac-mini), state done. All 7 suites passed:RightColumnAfterSplitTests,RightColumnPlacementTests,LinkOpenPlacementTests,RightColumnTurnTests,SideOpenStripTests,RightColumnScreenTests,RightColumnEntryPointTests.Observed / Not run
ClickedLinkFocusTestspassed 8 of 10 tests. It now passesaLinksTabInAnotherPane...andaChatChipClickedAgain...on cmux13, the host where base red: ClickedLinkFocusTests' pane on the right holds a browser, so a link lands there on any host #29787 saw them fail. At the base they still fail (job47791f5f3045e15de53ef5b2atade9546b, cmux-austin-mini-0). So this race was a cause of those failures.ade9546b(job47791f5f3045) and at the head:ClickedLinkFocusTests.anAgentsOpenFromALoneChatRefocusesTheChatAsTheApp(:256), the known failure from base red: ClickedLinkFocusTests' pane on the right holds a browser, so a link lands there on any host #29787ClickedLinkFocusTests.theHeadersPreviewBesideThePageTakesTheFocusAndItsCloseGivesItBack(:285)ChatDockToolPlacementTests(openToolreturns false)Changelog
Need help on this PR? Tag
@codesmith-botwith what you need.Summary by cubic
Fixes placement of links and agent panes opened right after a split: instead of landing in a new split below the pane on the right, they now land as a tab in that pane, in narrow windows.
LinkOpener.width(of:)read a pane's view bounds, which are stale while a split's spring runs: the source still shows its width from before the split, and the new pane starts as a zero-width sliver (nil). With a wide source and an unmeasured right pane,RightColumnPlacement.decidechosesplitBelow. The width now comes fromLayoutRootView.restingContentWidth(of:)— the frame the pane rests at in the geometry its screen's springs head for — so links and agent opens both read the final widths. The new read is an O(1) lookup into geometry computed on layout sync, adding no layout pass and no wait.LinkOpener.width(of:)readsrestingContentWidth(of:)instead of the pane view's bounds.LayoutRootView.restingContentWidth(of:)returns nil for a pane no screen view lays out, as before.RightColumnAfterSplitTests.anOpenRightAfterASplitReadsThePanesFinalWidths, which drives the layout clock so the split's spring stays at its first frame on every host.Written for commit 3af8e0a. Summary will update on new commits.