Fix sidebar reopen cutoff render by austinywang · Pull Request #8626 · manaflow-ai/cmux

austinywang · 2026-07-22T03:05:02Z

Summary

Preserve the built-in AppKit sidebar table subtree across hide/show so reopening does not expose a cold, deferred row-layout pass.
Treat visibility as presentation geometry instead of native view identity: collapse and clip the retained shell while hidden, disable hit testing/accessibility, then reuse the same table container on reopen.
Quiesce retained hidden UI: suspend observers, event monitors, animations, row actions and payloads, staged applies, viewport work, and eager table reloads; reconcile the current workspace model when presentation resumes.
Commit active inline rename/checklist edits before detaching their callbacks, preserve installed row heights, and defer view-mutating commits/reloads past the current AppKit update.
Return focus to the selected main panel only when the retained left sidebar owned first-responder focus.
Scope retention to the built-in default provider with appKitSidebarList enabled. Custom and ExtensionKit providers retain their existing unmount behavior. The legacy SwiftUI sidebar path is unchanged.

Root cause and scope

Sidebar visibility was encoded as view lifetime for the AppKit implementation. Closing the sidebar destroyed SidebarWorkspaceTableContainerView; reopening cold-started its estimated/deferred row layout, briefly exposing rows at an incomplete clipped geometry before the table settled.

The architectural invariant is now: visibility may change presentation geometry, but it must not change the identity of the built-in native AppKit sidebar. Retaining the native hierarchy also requires quiescing hidden work so the fix does not trade the flash for off-screen observers, animations, payload retention, or mutation churn.

This is related to #8412 only at the native-row rendering-settle layer. Its settings-triggered repaint path differs from this visibility-triggered remount, so this fix does not reuse or assume the same trigger.

Regression coverage

The initial red/green history remains split:

cdf4531d15 — failing real-window behavior test only.
ab76282633 — production identity-preservation fix.

Follow-up tests and fixes cover lifecycle details discovered during review: provider scoping, focus ownership, hidden payload release, animation suspension, checklist/rename teardown, row-height preservation, scheduler deferral, and hidden reload reconciliation.

Final coverage is split across files that remain below 500 lines:

SidebarHiddenPresentationTests.swift — controller/payload/animation behavior plus real-window hide/reopen identity and provider scope.
SidebarWorkspaceRowSuspensionTests.swift — row-action release and inline edit/checklist suspension.
SidebarWorkspaceTableTests.swift — table controller, height-cache, scheduler, and reload behavior.
WorkspaceContentViewVisibilityTests.swift — layout/focus helpers and deterministic resizer scheduling.

The real-window regression mounts ContentView in an NSWindow, selects the AppKit flag through a test-owned feature-flag store, toggles the window-scoped SidebarState, verifies the same native table container remains mounted while hidden and after reopen, confirms hidden workspace updates are deferred then reconciled, and exercises focus ownership.

Validation

Passed on final rebased HEAD:

Swift frontend parse for every changed Swift file.
python3 scripts/lint-feature-flags.py
./scripts/check-pbxproj.sh
python3 scripts/check-test-determinism.py --strict
PYTHONUNBUFFERED=1 python3 tests/test_ci_sidebar_lazy_layout_guard.py
CI workflow guard suite, including the exact pbxproj test-wiring lint, project normalization, workspace package grouping, SwiftPM lockfile policy, warning budget guard, sidebar lazy-layout guard, feature-flag lint, and determinism gate.
git diff --check origin/main

The repository revision no longer contains .github/swift-file-length-budget.tsv or scripts/swift_file_length_budget.py, so the requested script cannot be run. Neither Swift budget file was introduced or changed; .github/swift-warning-budget.tsv is untouched. New and newly split Swift files remain under 500 lines.

Per task instructions, no local xcodebuild, XCUITest, tagged app build, app launch, or reload.sh was run on this host.

Final-head manual CI run: https://github.com/manaflow-ai/cmux/actions/runs/29968346069

All workflow guards and the independent web/database/React/Rust jobs passed.
The unrelated daemon/remote test TestTerminateProcessesRunsOnlyOnce failed three clean attempts before macOS routing.
This is a deterministic current-main failure: the once-only guard never landed. Existing PR remote-daemon: tear a PTY session down once #8643 is the scoped upstream fix and explicitly documents that this failure blocks all manually dispatched CI from reaching the macOS matrix: remote-daemon: tear a PTY session down once #8643
This sidebar PR does not touch daemon/remote, and the unrelated daemon fix was not copied into this branch.
All GitHub-required PR checks and latest-head review checks are green; the manually dispatched all-routes workflow remains blocked until remote-daemon: tear a PTY session down once #8643 lands.

A previous exact-head Depot run auto-canceled at the workflow's fixed 20-minute job timeout while unit tests were still running; it did not report a code/test failure: https://github.com/manaflow-ai/cmux/actions/runs/29958396576.

Localization audit: the production diff adds no user-facing strings or message keys. Added-line inspection found no new labels, buttons, alerts, tooltips, titles, or descriptions, so no locale catalog changes are required.

Demo video

A clean cloud-Mac capture was attempted but infrastructure prevented a usable session:

Loader run: https://github.com/manaflow-ai/cmux-loader/actions/runs/29886387305
The VM reached its GUI-ready hold, but its Tailscale device never appeared and this host has no macfleet Admin token.
Existing fallbacks were blocked by a full disk on cmux-aws-m4pro and missing Screen Recording permission on cmux13s-mac-mini.
No shared-host TCC settings or SSH keys were changed.

Video URL or attachment: unavailable due to those infrastructure blockers.

Checklist

Local app/Xcode build — intentionally not run; prohibited for this task on this host.
Behavior regression coverage added with red/green commit history.
AppKit/default-provider scope verified; legacy SwiftUI and custom/ExtensionKit paths unchanged.
Project wiring, parser, determinism, feature-flag, lazy-layout, and diff checks passed.
Localization audit completed; no user-facing copy changed.
Budget files untouched; new/split files remain below 500 lines.
Latest-head required checks, bot reviews, and review threads complete.
Manual all-routes CI — blocked before macOS by the deterministic current-main daemon failure tracked in remote-daemon: tear a PTY session down once #8643.

coderabbitai · 2026-07-22T03:05:55Z

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

▶️ Resume reviews
🔍 Trigger review

📝 Walkthrough

Walkthrough

ContentView now receives feature flags explicitly, uses the injected AppKit sidebar flag to select layout and sidebar implementations, retains the default sidebar while hidden, restores focus, and verifies table-container identity across visibility toggles.

Changes

AppKit sidebar identity preservation

Layer / File(s)	Summary
Injected sidebar flags and layout path `Sources/FeatureFlags.swift`, `Sources/ContentView.swift`	Adds a named AppKit sidebar flag and propagates injected feature flags through `ContentView`, sidebar construction, gating, and state comparison.
Provider retention and focus handling `Sources/ContentView.swift`	Resolves effective providers, retains the default AppKit sidebar in a clipped layout when hidden, and restores main-panel focus after hiding it.
Sidebar visibility reuse coverage `cmuxTests/WorkspaceContentViewVisibilityTests.swift`	Mounts `ContentView`, toggles sidebar visibility, verifies table-container reuse, and checks persistence is limited to the default provider.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant ContentView
  participant CmuxFeatureFlags
  participant VerticalTabsSidebar
  participant NSWindow
  participant SidebarState

  ContentView->>CmuxFeatureFlags: Read injected AppKit sidebar flag
  ContentView->>VerticalTabsSidebar: Pass feature flags and select implementation
  ContentView->>NSWindow: Mount persistent sidebar layout
  SidebarState->>ContentView: Toggle sidebar visibility
  ContentView->>ContentView: Retain table container and restore focus

Suggested reviewers: lawrencecchen, azooz2003-bit

🚥 Pre-merge checks | ✅ 25

✅ Passed checks (25 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The change keeps the AppKit sidebar subtree mounted when hidden, which directly addresses `#8624`'s brief cut-off reopen render.
Out of Scope Changes check	✅ Passed	The changes stay focused on sidebar visibility/rendering, feature flags, and regression tests with no clear unrelated additions.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Cmux Swift Actor Isolation	✅ Passed	New feature-flag plumbing stays on `@MainActor` UI types; CmuxFeatureFlags is main-actor isolated, and the added test is `@MainActor`, so no new background access or isolation debt appears.
Cmux Swift Blocking Runtime	✅ Passed	HEAD’s production diff adds only synchronous sidebar state helpers; no new waits, sleeps, asyncAfter, main.sync, semaphores, or locks appear in production hunks.
Cmux Browser Automation Off-Main	✅ Passed	PR only changes sidebar UI/tests; browser automation routing files stayed untouched, so the off-main WebKit wait rule is not implicated.
Cmux Expensive Synchronous Load	✅ Passed	PASS: the touched ContentView/FeatureFlags hunks add only sidebar/flag logic; no new production call to RestorableAgentSessionIndex.load() or agent-history parsing appears on MainActor.
Cmux Cache Substitution Correctness	✅ Passed	UI-only layout gate uses direct UserDefaults reads plus fallback docs; no persistence/history/undo/snapshot path swapped to a stale cache.
Cmux No Hacky Sleeps	✅ Passed	Only Swift files changed; no covered non-Swift runtime scripts were modified, and the diff adds no new fixed sleeps or wall-clock waits.
Cmux Algorithmic Complexity	✅ Passed	New body-path lookup is bounded to a 6-item static provider list with fileExists probes; the recursive descendants walk is test-only.
Cmux Swift Concurrency	✅ Passed	Diff adds AppKit/SwiftUI/XCTest-boundary changes only; no new DispatchQueue/Task fire-and-forget, Combine state, or completion-handler async APIs in touched lines.
Cmux Swift `@Concurrent`	✅ Passed	No changed Swift function adds `@concurrent` or nonisolated async; the only new async helper is `@MainActor` drainMainRunLoop in a UI test and is intentionally UI-bound.
Cmux Swift Package Boundaries	✅ Passed	The diff only adds app-target sidebar layout/focus glue and feature-flag wiring; no reusable domain logic was kept in the app target.
Cmux Swiftpm Lockfiles	✅ Passed	Diff only touches Sources/ContentView.swift and the test file; no .gitignore, Package.swift, Package.resolved, project, or workflow changes are present.
Cmux Swift Logging	✅ Passed	No added or changed print/debugPrint/dump/NSLog/Logger-based production logging appeared in the Swift diff; only sidebar logic and tests changed.
Cmux User-Facing Error Privacy	✅ Passed	PASS: the diff only changes sidebar layout/feature-flag plumbing and tests; it adds no user-facing errors, alerts, API bodies, or recovery copy.
Cmux Full Internationalization	✅ Passed	New production strings in Sources/FeatureFlags.swift are localized and the keys exist in Resources/Localizable.xcstrings with translated en/ja entries.
Cmux Swiftui State Layout	✅ Passed	The diff only adds plain feature-flag plumbing and AppKit-bridge sidebar retention; it adds no new ObservableObject/@published state, GeometryReader measurement, or store-holding lazy rows.
Cmux Architecture Rethink	✅ Passed	PASS: The diff uses an explicit featureFlags owner and a clear mounted-sidebar invariant; it adds no sleeps, polling, locks, or duplicate lifecycle ownership.
Cmux Swift Auxiliary Window Close Shortcuts	✅ Passed	The only new NSWindow is a test-only fixture; no production auxiliary window, identifier, or cmuxAuxiliaryWindowIdentifiers change was introduced.
Cmux Source Artifacts	✅ Passed	Only hand-written source/test files changed; no logs, caches, build output, temp dirs, or other artifact paths were added.
Cmux No Test Or Debug Seam In Production Source	✅ Passed	PASS: the new static helper is used by production layout logic, and the diff adds no `#if` DEBUG/test-only members or test-named seams under Sources/.
Cmux No Ambient Global State	✅ Passed	No new ambient global state: `featureFlags` is injected, `appKitSidebarListFlag` is a static let, and the new helpers are private or instance-scoped.
Title check	✅ Passed	The title clearly matches the main change: preventing sidebar reopen cutoff rendering.
Description check	✅ Passed	The description is mostly complete, covering summary, root cause, testing, demo video, and checklist; the review-trigger block is the main omission.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch issue-8624-sidebar-toggle-cutoff-render

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

greptile-apps · 2026-07-22T03:06:30Z

Greptile Summary

This PR keeps the built-in AppKit sidebar mounted while it is hidden. The main changes are:

Collapses and clips the retained sidebar during hiding.
Suspends hidden observers, interactions, animations, edits, and table updates.
Reconciles current workspace state when the sidebar reopens.
Preserves focus, row heights, and native table identity.
Limits retention to the built-in AppKit provider.
Adds lifecycle, provider, focus, editing, and table tests.

Confidence Score: 5/5

This looks safe to merge.

The production table-container test hook was removed.
The sidebar behavior test now uses an isolated injected feature-flag store.
No blocking issue remains in the reviewed fixes.

Important Files Changed

Filename	Overview
Sources/ContentView.swift	Adds provider-scoped sidebar retention, hidden presentation behavior, focus transfer, and injected feature flags.
Sources/FeatureFlags.swift	Exposes a stable AppKit sidebar flag definition for consistent resolution by injected stores.
Sources/Sidebar/AppKitList/SidebarWorkspaceTableController.swift	Coordinates table suspension, deferred mutations, row retirement, and reload reconciliation.
cmuxTests/SidebarHiddenPresentationTests.swift	Covers retained native identity, provider scope, hidden updates, and isolated feature-flag selection.

_{Reviews (38): Last reviewed commit: "Fix AppKit row retirement view typing" | Re-trigger Greptile}

coderabbitai

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 `@cmuxTests/WorkspaceContentViewVisibilityTests.swift`:
- Around line 124-127: Replace the positional CmuxFeatureFlags.allFlags[6]
lookup in WorkspaceContentViewVisibilityTests with the stable named identifier
or enum case for the AppKit sidebar feature flag. Keep the existing override
setup and deferred restoration using the resolved flag unchanged.

🪄 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: e878c3d7-3f61-4dec-84f6-534d1efeb9d8

📥 Commits

Reviewing files that changed from the base of the PR and between 3e66d61 and b4ab99d.

📒 Files selected for processing (3)

Sources/Debug/SidebarLazyContractProbe.swift
Sources/Sidebar/AppKitList/SidebarWorkspaceTableView.swift
cmuxTests/WorkspaceContentViewVisibilityTests.swift

austinywang · 2026-07-22T03:42:35Z

@codex review
@coderabbitai review
@greptile-apps review
@cubic-dev-ai review

cubic-dev-ai · 2026-07-22T03:42:43Z

@codex review
@coderabbitai review
@greptile-apps review
@cubic-dev-ai review

@austinywang I can't start this review because your workspace has reached its free monthly review limit. cubic has reviewed 241,631 of the 240,000 allowed lines of code this month. Reviews resume on 1 August 2026 (in 10 days). Paid plans include much higher monthly review limits. Upgrade now to resume reviews.

To help optimise your usage, you can tune cubic to get the most out of your usage limits:

Stop reviewing test files
Turn off incremental reviews
Use the cubic CLI locally
Skip non-default base branches

Learn more →

chatgpt-codex-connector · 2026-07-22T03:42:43Z

To use Codex here, create a Codex account and connect to github.

coderabbitai · 2026-07-22T03:42:44Z

@austinywang: I’ll review the changes in #8626.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

coderabbitai

Actionable comments posted: 3

🤖 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 `@cmuxTests/WorkspaceContentViewVisibilityTests.swift`:
- Around line 159-200: Replace the single-result firstDescendant checks in the
sidebar toggle test with collection of all SidebarWorkspaceTableContainerView
descendants after each toggle. Assert each collection contains exactly one
container and that its identity is initialContainer; update or add a helper near
firstDescendant to gather all matching descendants while preserving the existing
traversal.
- Around line 111-156: In the AppKit sidebar test setup, capture the existing
WindowTerminalPortal.usesCoalescedAnchorFailsafe value before changing it, then
restore that value in the existing defer teardown alongside the defaults and
window cleanup. Ensure the restoration runs regardless of test outcome so later
tests do not inherit this test’s configuration.

In `@Sources/FeatureFlags.swift`:
- Around line 176-177: Update appKitSidebarListFlag and the corresponding
definition at the additional location to resolve the flag by its stable key
rather than the positional allFlags[6] index. Preserve the existing flag
identity and ensure the definition remains included in allFlags without
depending on ordering.

🪄 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: 1c543000-6d85-4a70-8000-b31db522b789

📥 Commits

Reviewing files that changed from the base of the PR and between b4ab99d and 634be0d.

📒 Files selected for processing (3)

Sources/ContentView.swift
Sources/FeatureFlags.swift
cmuxTests/WorkspaceContentViewVisibilityTests.swift

coderabbitai

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/ContentView.swift`:
- Around line 2596-2613: Add test coverage in
cmuxTests/WorkspaceContentViewVisibilityTests.swift:170-174 by adding a second
scenario that makes the sidebar’s in-place rename NSTextView/field editor the
window’s first responder before toggling sidebarState, then assert focus is
restored to the main panel. Sources/ContentView.swift:2596-2613 requires no
direct change; the existing appKitSidebarContainer(owning:) delegate-lookup path
should be exercised by the new test.

🪄 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: b7887140-c98a-4f77-8b3b-1ca4f894df5d

📥 Commits

Reviewing files that changed from the base of the PR and between 06dab30 and d5ff67f.

📒 Files selected for processing (2)

Sources/ContentView.swift
cmuxTests/WorkspaceContentViewVisibilityTests.swift

cursor · 2026-07-22T20:43:18Z