Repository navigation
Minimal mode: stop the toggle from re-evaluating the window root, sidebar, and Bonsplit trees - #5932
Closed
austinywang wants to merge 13 commits into
Closed
austinywang wants to merge 13 commits into
austinywang wants to merge 13 commits into
Conversation
…5732) Test-only socket verb that flips workspacePresentationMode through the same UserDefaults path as the Settings toggle and the palette commands, then logs how long the main thread stays blocked before the run loop turns again (mainBlockedMs), the synchronous defaults-observer cost (setMs), and which view bodies re-evaluated during the swap (PresentationModeToggleDiagnostics). External samplers cannot attach on this macOS 26 generation (sample/xctrace fail to read thread state even with get-task-allow), so this probe is the measurement instrument for #5732. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
Author
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughLocalizes minimal-mode and dotted-key debug subscriptions into bridge/host views, makes WorkspaceContentView equatable to gate Bonsplit re-evaluation, centralizes presentation-mode side effects, controls tmux snapshot publishing, and adds tests plus a debug terminal probe. ChangesMinimal Mode Toggle Re-evaluation Localization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (19 passed)
✨ Finishing Touches🧪 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 |
austinywang
force-pushed
the
issue-5732-minimal-mode-toggle-hang
branch
from
June 12, 2026 01:42
46a4143 to
ccaca32
Compare
Contributor
Greptile SummaryThis PR surgically fixes the minimal-mode toggle hang by moving every presentation-mode
Confidence Score: 5/5The change is safe to merge; all new code paths are well-scoped leaf views or DEBUG-only probes, behavior is verified by the new equatable contract tests, and the architectural invariants are clearly documented. The mode-subscription refactor is narrow and mechanical — each new leaf view owns exactly one @AppStorage key, the Equatable conformance has a safe fallback for the off-main case, and the tmuxLayoutSnapshot change is gated behind a feature flag check. The only non-trivial concern is that the nonisolated == operator relies on a runtime thread check rather than compile-time isolation guarantees, but it cannot cause stale renders (the fallback is an extra render, not a skipped one), and the contract tests cover every render-relevant input. Sources/WorkspaceContentView+Equatable.swift — the nonisolated == pattern is the one place where actor isolation is enforced at runtime rather than compile time; worth revisiting if the stored properties can be declared nonisolated. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
subgraph After["After this PR"]
X[UserDefaults modeKey write] --> L1[MinimalModeTitlebarBandHost]
X --> L2[MinimalModeContentTopPaddingBridge]
X --> L3[MinimalModeSafeAreaBridge]
X --> L4[MinimalModeTitlebarEventSurfaceHost]
X --> L5[SidebarMinimalModeTitlebarControlsOverlay]
L1 -->|onModeChange| AppKit[AppKit side effects]
L2 -->|re-layout only| Skip1[Content subtree: no body re-eval]
L3 -->|re-layout only| Skip2[Bonsplit subtree: no body re-eval]
DotKey[dotted-key defaults write] --> Sentinel[TitlebarDebugChromeSentinel leaf]
DotKey --> Model[ExtensionSidebarProviderSelectionModel]
Model -->|only on real change| Sidebar[VerticalTabsSidebar re-renders]
end
subgraph Before["Before #5732"]
A[UserDefaults modeKey write] --> B[ContentView full re-eval]
A --> C[VerticalTabsSidebar full re-eval]
A --> D[WorkspaceContentView re-eval x N]
B --> E[Bonsplit subtree re-eval per window]
C --> F[O-N sidebar render context]
D --> G[Full Bonsplit re-eval per workspace]
end
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
subgraph After["After this PR"]
X[UserDefaults modeKey write] --> L1[MinimalModeTitlebarBandHost]
X --> L2[MinimalModeContentTopPaddingBridge]
X --> L3[MinimalModeSafeAreaBridge]
X --> L4[MinimalModeTitlebarEventSurfaceHost]
X --> L5[SidebarMinimalModeTitlebarControlsOverlay]
L1 -->|onModeChange| AppKit[AppKit side effects]
L2 -->|re-layout only| Skip1[Content subtree: no body re-eval]
L3 -->|re-layout only| Skip2[Bonsplit subtree: no body re-eval]
DotKey[dotted-key defaults write] --> Sentinel[TitlebarDebugChromeSentinel leaf]
DotKey --> Model[ExtensionSidebarProviderSelectionModel]
Model -->|only on real change| Sidebar[VerticalTabsSidebar re-renders]
end
subgraph Before["Before #5732"]
A[UserDefaults modeKey write] --> B[ContentView full re-eval]
A --> C[VerticalTabsSidebar full re-eval]
A --> D[WorkspaceContentView re-eval x N]
B --> E[Bonsplit subtree re-eval per window]
C --> F[O-N sidebar render context]
D --> G[Full Bonsplit re-eval per workspace]
end
Reviews (12): Last reviewed commit: "fix: use presentation mode snapshot in f..." | Re-trigger Greptile |
austinywang
force-pushed
the
issue-5732-minimal-mode-toggle-hang
branch
from
June 12, 2026 01:59
ccaca32 to
1ac08d0
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/Workspace.swift`:
- Around line 19962-19971: Extract the tmux snapshot publish logic from
Sources/Workspace.swift into a new extension file named
Workspace+TmuxLayoutSnapshot.swift: move the private func
storeTmuxLayoutSnapshot(_ snapshot: LayoutSnapshot?) implementation (including
the comment) into an extension Workspace { ... } in that file, retaining the
same access level and references to tmuxLayoutSnapshot,
TmuxOverlayExperimentSettings, and objectWillChange so compilation and behavior
are unchanged; remove the original method from Sources/Workspace.swift and add
the new file to the target so the project builds.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 052199ba-51d4-4a87-bd5c-38e296155e2a
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (6)
Sources/ContentView.swiftSources/MinimalModeChromeSwap.swiftSources/Workspace.swiftSources/WorkspaceContentView.swiftcmux.xcodeproj/project.pbxprojcmuxTests/WorkspaceContentViewEquatableTests.swift
Toggling minimal mode hung the main thread in one giant synchronous AttributeGraph transaction: the mode was observed via @AppStorage on three large bodies (ContentView, VerticalTabsSidebar, every mounted WorkspaceContentView), and the re-evaluated parents rebuilt closure-carrying children, so the whole Bonsplit tree and the O(N) sidebar re-rendered for a chrome swap that only changes a titlebar band, a top inset, and a small sidebar controls strip. Verified with the set_presentation_mode probe (35 workspaces, 3-pane selected workspace): per-toggle body counts go from {contentView=1 sidebar=1 workspaceContent=2 + full Bonsplit re-eval} to {contentView=0 sidebar=0 workspaceContent=0} with only leaf hosts re-running; the remaining main-thread block is the genuine per-pane terminal relayout. - MinimalModeChromeSwap.swift: leaf views that own the mode subscription — MinimalModeTitlebarBandHost (band mount/unmount + AppKit side effects), MinimalModeContentTopPaddingBridge (content top inset), MinimalModeTitlebarEventSurfaceHost, MinimalModeSafeAreaBridge (per- workspace safe-area cancellation), SidebarMinimalModeTitlebarControlsOverlay (one shared construction path for both sidebar variants). - WorkspaceContentView is Equatable and mounted with .equatable(): window- root re-renders no longer re-evaluate the Bonsplit subtree; workspace and notification stores keep their own subscriptions. - Dotted-key @AppStorage amplifiers removed (dots break per-key KVO, so SwiftUI invalidated the holder on EVERY UserDefaults write, empirically confirmed via Self._logChanges): the four titlebarDebug.* keys move off ContentView into TitlebarDebugChromeSentinel, and the sidebar's cmuxExtensionSidebar.providerId moves behind ExtensionSidebarProviderSelectionModel, which publishes only on real provider changes. - Workspace.tmuxLayoutSnapshot is no longer @published: bonsplit reports geometry on every divider drag/resize/chrome relayout and each publish re-evaluated the whole mounted Bonsplit body mid-layout. Writes route through storeTmuxLayoutSnapshot, which still fires objectWillChange while a tmux pane-overlay experiment target is active (the only reactive consumer). cmuxTests/WorkspaceContentViewEquatableTests.swift pins the diffing contracts: closure-identity churn compares equal (so .equatable() can skip the Bonsplit subtree), every render-relevant input change compares unequal, and the provider-selection model publishes only on real changes. Fixes #5732 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
austinywang
force-pushed
the
issue-5732-minimal-mode-toggle-hang
branch
from
June 12, 2026 02:03
1ac08d0 to
5d3ad50
Compare
Take main's entries and re-sync the four files this branch touches (TerminalController, Workspace, ContentView, WorkspaceContentView) to the merged tree's actual line counts. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
SwiftUI re-exports Observation on macOS 14+, but the explicit import keeps the @observable model and withObservationTracking call sites self-contained (autoreview P1). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmuxTests/WorkspaceContentViewEquatableTests.swift (1)
1-131:⚠️ Potential issue | 🔴 CriticalWire
WorkspaceContentViewEquatableTests.swiftinto thecmuxTeststarget sources
cmux.xcodeproj/project.pbxprojcontains aPBXFileReferenceforWorkspaceContentViewEquatableTests.swift(A5732006), but no correspondingPBXBuildFileentry was found—so it isn’t added to thecmuxTeststarget’sPBXSourcesBuildPhase. This will cause the test to be silently ignored by Xcode/CI; add the missingPBXBuildFileand include it in thecmuxTestssources phase.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmuxTests/WorkspaceContentViewEquatableTests.swift` around lines 1 - 131, The test file WorkspaceContentViewEquatableTests.swift (PBXFileReference A5732006) is present in the project but not wired into the cmuxTests target sources; add a PBXBuildFile entry for A5732006 in the project.pbxproj and include that PBXBuildFile in the cmuxTests PBXSourcesBuildPhase so the file is compiled into the cmuxTests target (also verify target membership in Xcode UI if needed). Ensure the new PBXBuildFile references the existing PBXFileReference A5732006 and that the cmuxTests target’s sources phase contains that build file entry.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@cmuxTests/WorkspaceContentViewEquatableTests.swift`:
- Around line 1-131: The test file WorkspaceContentViewEquatableTests.swift
(PBXFileReference A5732006) is present in the project but not wired into the
cmuxTests target sources; add a PBXBuildFile entry for A5732006 in the
project.pbxproj and include that PBXBuildFile in the cmuxTests
PBXSourcesBuildPhase so the file is compiled into the cmuxTests target (also
verify target membership in Xcode UI if needed). Ensure the new PBXBuildFile
references the existing PBXFileReference A5732006 and that the cmuxTests
target’s sources phase contains that build file entry.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 194b4cb7-7834-46fc-a86a-da11825a8fa8
📒 Files selected for processing (2)
Sources/MinimalModeChromeSwap.swiftcmuxTests/WorkspaceContentViewEquatableTests.swift
…elivery synchronous on main; Swift Testing Addresses autoreview findings: - MinimalModeChromeSwap.swift bundled several major types; split into single-type files (safe-area bridge, band host, padding bridge, event surface host, debug sentinel, provider-selection model, sidebar overlay, WorkspaceContentView+Equatable) per the one-major-type-per-file rule. - ExtensionSidebarProviderSelectionModel now observes defaults with queue: nil, so main-thread writes (every in-app mutation path) refresh synchronously in the same turn — and the model test is deterministic instead of racing an operation-queue delivery. - New non-UI coverage converted from XCTest to Swift Testing per the cmux testing policy. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
austinywang
force-pushed
the
issue-5732-minimal-mode-toggle-hang
branch
from
June 12, 2026 02:53
998f71a to
5aede8e
Compare
…ontroller split landed) Take main's entries and re-sync this branch's touched files to the merged tree's actual line counts. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…de-toggle-hang # Conflicts: # .github/swift-file-length-budget.tsv # Sources/WorkspaceContentView.swift # cmux.xcodeproj/project.pbxproj
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
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
3 participants
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.
Fixes #5732
Root cause
Toggling minimal mode ran one giant synchronous AttributeGraph transaction on the main thread. Three compounding causes, all empirically confirmed with a DEBUG probe and
Self._logChanges()(external samplers cannot attach on this macOS 26 generation):workspacePresentationModewas an@AppStorageonContentView(window root),VerticalTabsSidebar, and every mountedWorkspaceContentView. A toggle invalidated all of them, and because the re-evaluated parents rebuild closure-carrying children (which defeat SwiftUI's implicit diffing), the entire Bonsplit subtree (TabBarView.splitButtons/AGGraphSetOutputValueSentry frames) and the O(N) sidebar render context (VerticalTabsSidebar.workspaceScrollContentframes) re-evaluated — per window — for a swap that only changes a titlebar band, a top inset, and a small sidebar controls strip.@AppStorageamplifiers. Keys containing dots (titlebarDebug.…onContentView,cmuxExtensionSidebar.providerIdon the sidebar) break per-key KVO, so SwiftUI falls back to invalidating the holder on everyUserDefaultswrite of any key._logChangesshowedContentViewre-running with the fourtitlebarDebugproperties "changed" when only the mode key was written. This is why the window root and sidebar re-rendered on every defaults write app-wide, not just on toggles.Workspace.tmuxLayoutSnapshotwas@Publishedand assigned on every bonsplit geometry change. The chrome swap resizes the content, bonsplit reports geometry, the publish re-evaluated the whole mounted workspace's Bonsplit body mid-layout (the "NSHostingView is being laid out reentrantly" warning), 1–2 extra times per toggle — and the same storm fired on every divider drag and window resize. This silently defeated the carefulpaneLayoutVersiongating right below it whose comment says geometry events must not fireobjectWillChange.Fix — observe the mode (and the dotted keys) on leaf chrome only
Sources/MinimalModeChromeSwap.swift(new): small leaf views own the subscriptions and store the heavy subtrees as already-built view values, so a toggle re-runs only the leaves and re-layouts the stored content:MinimalModeTitlebarBandHost(band mount/unmount + the AppKit side effects that used to live inContentView.onChange),MinimalModeContentTopPaddingBridge(mode-dependent top inset),MinimalModeTitlebarEventSurfaceHost,MinimalModeSafeAreaBridge(per-workspace top safe-area cancellation), andSidebarMinimalModeTitlebarControlsOverlay(one shared construction path for both sidebar variants, replacing two duplicated inline blocks).WorkspaceContentViewisEquatableand mounted with.equatable(): window-root re-renders (titlebar text churn, selection changes, appearance) no longer re-evaluate the Bonsplit subtree.==compares workspace identity plus the five render-relevant value inputs;workspace/notificationStorekeep invalidating through their own subscriptions.titlebarDebug.…keys move offContentViewintoTitlebarDebugChromeSentinel(a zero-size background leaf that reapplies window decorations when the debug insets actually change); the sidebar's provider selection moves behindExtensionSidebarProviderSelectionModel, which watchesUserDefaults.didChangeNotificationand publishes only on real provider changes.tmuxLayoutSnapshotis no longer@Published: writes route throughstoreTmuxLayoutSnapshot, which firesobjectWillChangeonly while a tmux pane-overlay experiment target is active (the only reactive consumer; default off). Divider drags / resizes / chrome swaps no longer re-evaluate mounted Bonsplit bodies.ContentViewno longer observes the mode at all; imperative readers (syncTrafficLightInset, palette context, titlebar-controls hiding) readWorkspacePresentationModeSettings/MinimalModeTitlebarDebugSettingsfrom defaults at call time, and the palette enable/disable commands writeUserDefaultsdirectly (same single mutation path as the Settings toggle).Numbers (same workload, same build pipeline, before/after)
Tagged Debug dev build (cloud builder), M-series MacBook Pro (macOS 26.4.1), single window, 35 workspaces, selected workspace split into 3 panes / 6 surfaces, sidebar visible. Toggles driven through the shared UserDefaults path by the DEBUG
set_presentation_modeverb, which logs the main-thread block until the run loop turns again. Raw before-series and methodology: #5732 (comment)ContentView,VerticalTabsSidebar(O(N) render context),WorkspaceContentView×2 + full Bonsplit subtree re-evalcontentView=0 sidebar=0 workspaceContent=0— leaf hosts only (bridge=1 chromeHost=3 tabItem=1)The wall-clock residual is the genuine relayout: it halves when the selected workspace has 1 pane instead of 3 (~75ms vs ~145ms on a local build), i.e. it is the per-pane Ghostty resize + AppKit constraint pass — the same cost class as any window resize, and it does not scale with workspace/window count anymore. The part the fix removes is exactly the part that scaled with app state (whole-window AttributeGraph recompute per window, O(N) sidebar, full Bonsplit re-eval, and the every-defaults-write amplifiers), which is what turns into the multi-second
AGGraphSetOutputValuehang family at production workloads (37 workspaces / 65 surfaces / 30 live agents / 3 windows). The dotted-key andtmuxLayoutSnapshotfixes also remove body-churn storms far beyond the toggle path (every defaults write; every divider drag/resize).Behavior verified on the tagged build via socket-driven screenshots: standard band/traffic lights, minimal full-bleed content with pane tab bar on top, minimal+hidden-sidebar traffic-light inset on the pane tab bar, sidebar controls strip, splits/workspace switching/surface create+close.
Measurement probe (kept, DEBUG-only)
set_presentation_mode <minimal|standard|toggle>logsmainBlockedMsandsetMs(synchronous defaults-observer cost) to the debug event log, so this hang class stays measurable — external samplers (sample,xctrace) cannot read thread state on this macOS generation even withget-task-allow. The per-toggle body counts quoted above came from temporary render-time counters used during development; they were removed before merge per the swiftui-state-layout rule (no state writes frombody), so the shipped probe is timing-only.Coverage
cmuxTests/WorkspaceContentViewEquatableTests.swiftpins the contracts the fix depends on: closure-identity churn must compare equal (otherwise.equatable()cannot skip the Bonsplit subtree and the hang returns), every render-relevant input change must compare unequal (otherwise workspaces render stale), andExtensionSidebarProviderSelectionModelmust publish only on real provider changes (otherwise the sidebar re-renders on every defaults write again). This mirrorsTaskManagerViewSnapshotBoundaryTests. A two-commit red/green structure is not applicable: the "red" state is a ~120 ms main-thread stall in a live app under load, which unit-test CI cannot observe, and the contract tests cannot compile before the fix because the conformances they pin are part of the fix.Localization audit
No user-facing strings added or changed: the new views render the existing localized
HiddenTitlebarSidebarControlsView/titlebar band unchanged; the only new text is the debug sockethelpline in the existing unlocalized DEBUG help block. NoResources/Localizable.xcstringsorweb/messages/*changes required.Summary by CodeRabbit
Performance Improvements
Stability / UX
Tests