Repository navigation
fix(next): the sidebar's content hold changes the content's frame once; per-frame trace of both modes - #30064
Conversation
|
Review (subagent claude-opus-5-5 medium): SHIP at 31512f8 The review ran at Findings: the landing width Follow-ups:
|
Red on the base: the slide holds the content at its widest width and rides the sidebar's moving edge, so a show slides the old wide layout across and lays it out narrower at the landing, and a hide lays it out wide at the card's old edge. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Agent-Session: https://cmux.com/deeplink/agent?id=aid-2e370042acb26e63cf31
…ys put Leo: "when i toggle the sidebar on and off I STILL SEE THE ENTIRE OTHER PART OF THE APP SHIFT LAYOUT FOR NO REASON AND SNAP BACK". The content hold (sidebar.slideHoldsContent, opt-in since #29894) kept the widest width of the slide and rode the sidebar's moving edge. It now takes the landing width when the slide starts and is pinned to the card's far edge, so the content's frame changes once per toggle, to where it lands. Still one layout per toggle (GPU audit 2026-10-21). The default (content follows the card every frame, #29894) is unchanged. Sidebar-Look: when i toggle the sidebar on and off I STILL SEE THE ENTIRE OTHER PART OF THE APP SHIFT LAYOUT FOR NO REASON AND SNAP BACK Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Agent-Session: https://cmux.com/deeplink/agent?id=aid-2e370042acb26e63cf31
31512f8 to
c9f9327
Compare
|
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)
Not verified: macOS jobs did not run: compile and app tests were skipped on CI passes on Written by web and size checks: green Default changesThis pull request changes 1 default at
Needs a review. It passes with an independent review receipt on this commit: a comment or review whose first line is Merge receipt: merged as Regenerate the snapshot with Shipped in:
|
|
Review (subagent claude-opus-5-5 medium): SHIP at c9f9327 The geometry holds for a left and a right sidebar, for show, hide and a reversal. The held frame equals Fleet: red Follow-ups:
|
|
Found 12 test failures on Blacksmith runners: Failures
...and 2 more test failures. View all on Blacksmith
|
…, not only wider Review follow-up on #30064: a held show is narrower than the card, so the comments that said the hold keeps content wider than the card, and that the sidebar slides over the content's near edge, now say what both legs do. Comments only. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Agent-Session: https://cmux.com/deeplink/agent?id=aid-2e370042acb26e63cf31
|
Review (subagent claude-opus-5-5 medium): SHIP at 4a11eec The new commit Follow-ups (carried from the review at c9f9327):
|
# Conflicts: # plans/cmux-next/sidebar-owner.md
|
Review (subagent claude-opus-5-5 medium): SHIP at cb34395 This head merges feat-cmux-next into Follow-ups: as listed in the review at c9f9327 (blank strip on an opt-in show, a right-sidebar trace case, reset |
There was a problem hiding this comment.
1 issue found and verified against the latest diff
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/SidebarToggleContentTraceTests.swift">
<violation number="1" location="Packages/macOS/CmuxNext/Tests/CmuxNextAppTests/SidebarToggleContentTraceTests.swift:101">
P2: The hold assertion only counts content-frame changes, so a single snap at landing would pass. Also assert that the change happens no later than the first sliding sample.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| + "before=\(Self.r(before.minX))+\(Self.r(before.width)) landed=\(Self.r(after.minX))+\(Self.r(after.width)) " | ||
| + "frames(sidebar:contentMinX+contentWidth)=\(trace.joined(separator: ","))") | ||
| if holds { | ||
| #expect(frames.count - 1 == 1, |
There was a problem hiding this comment.
P2: The hold assertion only counts content-frame changes, so a single snap at landing would pass. Also assert that the change happens no later than the first sliding sample.
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/SidebarToggleContentTraceTests.swift, line 101:
<comment>The hold assertion only counts content-frame changes, so a single snap at landing would pass. Also assert that the change happens no later than the first sliding sample.</comment>
<file context>
@@ -0,0 +1,127 @@
+ + "before=\(Self.r(before.minX))+\(Self.r(before.width)) landed=\(Self.r(after.minX))+\(Self.r(after.width)) "
+ + "frames(sidebar:contentMinX+contentWidth)=\(trace.joined(separator: ","))")
+ if holds {
+ #expect(frames.count - 1 == 1,
+ "the content's on-screen frame changed \(frames.count - 1) times in one toggle (one is to where it lands): \(frames)")
+ } else {
</file context>
# Conflicts: # plans/cmux-next/sidebar-owner.md
…r-toggle-no-bounce # Conflicts: # plans/cmux-next/sidebar-owner.md
![Fix with [code]smith](https://pr-comments-assets.blacksmith.sh/codesmith/fix-with-codesmith-light-3x.png)
Toggling the sidebar no longer shifts the content and snaps it back in either sidebar mode. This PR fixes the opt-in content hold so it changes the content's frame once and leaves it still. It also adds a per-frame trace that proves both modes reach their landing with no bounce.
Leo:
Cause
The slide's content hold (
MainPaneCardView.contentHold) was added by the GPU audit of 2026-10-21 so panes lay out once per toggle, not on every frame. It kept the content at the widest width of the slide and pinned it to the sidebar's moving edge:#29894 (a carry of #28894), which landed while this was in flight, made the default
sidebar.slideHoldsContent: false. The content now follows the card on every frame, so the default no longer snaps. This PR keeps that default.Fix
The hold, still in Debug Settings, now takes the card's landing width when the slide starts. It pins the content to the card's far edge, which doesn't move (
ContentHold.pinsTrailingEdge: the trailing edge for a left sidebar, the leading edge for a right one). The content is laid out once, on the first frame, at its final size and place, and clearing the hold at the landing changes nothing. That's still one layout per toggle, so the GPU audit's saving holds when it's on. A reversal mid-flight lays out once more, at the new landing width. The rail style, the sidebar's own animation and the default path are unchanged.Performance
Per-frame trace from
SidebarToggleContentTraceTests: real display ticks, a 1100x700 window, a 300 pt left sidebar. Each sample is the content's frame in window coordinates on one display frame. Steps: red8ee6ecb4d5e2d4e91b046149at0a4c827a92f(base plus tests), greend5b6044761f6125b5e255ae5atc9f93278319.6 → 47 → 79 → … → 306at 1088 wide, then snaps to 788)306+788from frame 1)241 → 204 → … → 6at 1088 wide)6+1088from frame 1)6+1088 → 71+1023 → … → 306+788)42+1052 → … → 306+788)249+845 → … → 6+1088)The default is #29894's behaviour on both sides. The trace pins it: the content's minX, maxX and width only move toward where they land, and nothing changes after the landing. It does resize the content on every frame, the cost the GPU audit measured at 6.7–7.4 ms of main thread a frame. The hold avoids that.
Tests
Red first:
0a4c827a92fis the base's production code plus the tests; the fix isc9f93278319.cmux-ci run --class light --script scripts/ci/package-test-lane.sh --display --env CMUX_TEST_REQUIRE_GUI=1 --arg=suite --arg=Packages/macOS/CmuxNext --arg=SidebarSlideHoldsContentTests,SidebarToggleContentTraceTests,SidebarToggleSettlementTestsRed at
0a4c827a92f:SidebarSlideHoldsContentTestsfails 3 of 9:SidebarToggleContentTraceTestsfails for the hold, with 15 and 11 frame changes; its default cases pass.Green at
c9f93278319:SidebarSlideHoldsContentTestspasses 9 of 9,SidebarToggleContentTraceTestspasses all 4 cases (hold and default, show and hide), andSidebarToggleSettlementTests(fix(next): sidebar toggle content follows its frame, no landing snap (carry #28894) #29894) passes.Observed / Not run
c9f93278319returned SHIP.No capture: the change is motion and opt-in; the trace is the evidence.
Prior art: #29894 / #28894 (the default), #24342 (introduced the hold), #18840 (classic sidebar), #21125 (right-sidebar pane widths). Owner note in
plans/cmux-next/sidebar-owner.md.Changelog
Fixed: With "Hold content size in sidebar slide" on (Debug Settings), showing or hiding the sidebar no longer shifts the rest of the window and snaps it back.
🤖 Generated with Claude Code
Agent session: agent session