Repository navigation
Fix Files sidebar flicker on pane close - #3747
Closed
austinywang wants to merge 39 commits into
Closed
austinywang wants to merge 39 commits into
austinywang wants to merge 39 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for 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:
📝 WalkthroughWalkthroughFileExplorerStore gains an ChangesOutline Revision Tracking & Flicker Mitigation
Sequence Diagram(s)sequenceDiagram
participant View
participant Coordinator
participant Store
participant OutlineView
View->>Coordinator: updateBindings(store,...)
Coordinator->>Coordinator: reset caches if store changed
Store->>Coordinator: objectWillChange (outlineRevision++)
Coordinator->>Coordinator: reloadIfNeeded() (compare lastAppliedOutlineRevision)
alt revision unchanged
Coordinator-->>OutlineView: (no reload)
else revision changed
Coordinator->>OutlineView: reloadData / reloadItem
Coordinator->>Coordinator: update lastAppliedOutlineRevision
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
Greptile SummaryFixes the Files sidebar flicker on pane close by introducing a
Confidence Score: 4/5Safe to merge once the missing locale translations are addressed; the outline-refresh gating and SSH search additions are correct and well-tested. The Resources/Localizable.xcstrings — the two new/changed Important Files Changed
Sequence DiagramsequenceDiagram
participant SwiftUI as SwiftUI Transaction
participant Coord as Coordinator
participant Store as FileExplorerStore
participant OV as NSOutlineView
SwiftUI->>Coord: updateNSView (pane close / layout)
Coord->>Coord: updateBindings(store:state:...)
Coord->>Coord: reloadIfNeeded()
Coord->>Store: outlineRevision
Note over Coord: lastAppliedOutlineRevision == outlineRevision?
Coord-->>OV: skip — no repaint
SwiftUI->>Coord: updateNSView (Files mutation)
Coord->>Coord: reloadIfNeeded()
Coord->>Store: outlineRevision (incremented by mutateOutline)
Note over Coord: revision changed → proceed
Coord->>Coord: "lastAppliedOutlineRevision = new revision"
alt rootNodes.count changed
Coord->>OV: reloadData()
Coord->>OV: restoreExpansionState(expandedPaths)
else same root count
Coord->>OV: refreshLoadedNodes()
end
Coord->>OV: applyStoredSelection()
Reviews (34): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
austinywang
force-pushed
the
issue-3742-files-sidebar-flicker
branch
from
May 8, 2026 20:47
4762eff to
6b39b8a
Compare
The Files sidebar repaint path needs coverage at the coordinator boundary: a parent SwiftUI update with unchanged file-store state must not walk loaded directories and refresh their outline rows. The companion positive assertion also proves Files-owned disclosure changes still refresh the outline when the store revision changes. Constraint: Repro shows pane close mutates the shared ContentView tree while the file store itself does not change. Confidence: high Scope-risk: narrow Directive: Keep file-tree outline refreshes gated by file-store revisions, not by pane geometry or parent SwiftUI transactions. Tested: git diff --check Not-tested: Local XCTest not run; direct xcodebuild is prohibited for this task and CI will prove the red test.
austinywang
force-pushed
the
issue-3742-files-sidebar-flicker
branch
from
May 8, 2026 20:51
6b39b8a to
8b43f0e
Compare
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/FileExplorerStore.swift`:
- Line 703: The call to markOutlineDirty() unconditionally increments
outlineRevision and triggers refreshLoadedNodes() even for silent
(hover-prefetch) successes; update the success path in the prefetch completion
handler so markOutlineDirty() is only invoked when silent == false (i.e., move
the markOutlineDirty() call inside the existing non-silent branch), ensuring
silent prefetches do not bump outlineRevision or cause
reloadIfNeeded()/refreshLoadedNodes() to walk the tree unnecessarily.
🪄 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: f8a1472b-aaaf-4d37-833f-85c47ec78cc4
📒 Files selected for processing (3)
Sources/FileExplorerStore.swiftSources/FileExplorerView.swiftcmuxTests/FileExplorerStoreTests.swift
austinywang
force-pushed
the
issue-3742-files-sidebar-flicker
branch
from
May 8, 2026 20:58
8b43f0e to
0da73c6
Compare
austinywang
force-pushed
the
issue-3742-files-sidebar-flicker
branch
from
May 8, 2026 21:01
0da73c6 to
9d98a6c
Compare
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/FileExplorerStore.swift`:
- Around line 721-742: The closure applyLoadedChildren currently references many
stored properties without explicitly capturing self; add an explicit capture
list [self] on the applyLoadedChildren closure to satisfy Swift's
non-escaping/escaping rules (e.g. `let applyLoadedChildren = { [self] in ...
}`), leaving the body unchanged; also rename the inner local variable that
shadows the function parameter (the let path inside the
pendingDescendIntoFirstChildPath branch) to something like targetPath to avoid
shadowing the outer path used later when calling loadingPaths.remove(path) and
loadTasks.removeValue(forKey: path). Ensure references to selectedPath,
selectedPaths, rootNodes, isRootLoading, parentNode,
pendingDescendIntoFirstChildPath, loadingPaths and loadTasks remain as-is but
are now allowed via the explicit [self] capture.
🪄 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: 95f466a3-a1da-43bb-8619-61d53f00e639
📒 Files selected for processing (3)
Sources/FileExplorerStore.swiftSources/FileExplorerView.swiftcmuxTests/FileExplorerStoreTests.swift
Pane close re-evaluates the shared SwiftUI ContentView, but the Files outline should only repaint when FileExplorerStore changes. The store now owns an outline revision and the AppKit coordinator ignores unchanged revisions, so parent layout transactions no longer walk loaded directories. Constraint: Direct xcodebuild is prohibited; final tagged reload must happen only after CI passes. Rejected: Rendering masks such as drawingGroup or pre-rendering | they hide repaint instead of removing the spurious invalidation. Confidence: high Scope-risk: narrow Directive: Do not call outline row refreshes from parent SwiftUI transactions without a FileExplorerStore revision change. Tested: git diff --check Not-tested: Local XCTest/build; CI and final reload own executable verification.
austinywang
force-pushed
the
issue-3742-files-sidebar-flicker
branch
from
May 8, 2026 21:09
9d98a6c to
022f0a8
Compare
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
The settled screenshot regression was not proving the flicker path. This adds DEBUG-only Files sidebar counters plus a socket repro that closes a split while the Files right sidebar is visible and asserts that the Files subtree does not dirty layout or refresh outline data. Constraint: The flicker is transient, so screenshots after the window settles can pass while the visible redraw still happens. Rejected: Keep relying on visual screenshot comparison | it misses the AppKit layout invalidation that happens during close. Confidence: high Scope-risk: narrow Directive: Use the counter-based repro for this flicker class; do not replace it with a settled screenshot-only check. Tested: python3 -m py_compile tests/cmux.py tests/test_files_sidebar_no_redraw_on_close_surface.py Not-tested: Intermediate commit is expected to fail the new repro against the old layout behavior.
Closing a split can still cause SwiftUI to call the Files NSViewRepresentable update path, even when no Files-owned state changed. The container now treats visibility and search layout as applied state, updates AppKit properties only when values change, and skips header work when the displayed path is unchanged. Constraint: Parent workspace updates are expected during split close; the Files subtree must tolerate those calls without dirtying layout. Rejected: Suppress updateNSView itself | SwiftUI legitimately re-enters that bridge during parent host churn. Confidence: high Scope-risk: narrow Directive: Future Files sidebar changes should preserve idempotent AppKit updates during unrelated workspace/split updates. Tested: CMUX_SOCKET_PATH=/tmp/cmux-debug-issue-3742-files-loop.sock python3 tests/test_files_sidebar_no_redraw_on_close_surface.py Tested: ./scripts/reload.sh --tag issue-3742-files-loop --launch Tested: git diff --check Tested: python3 -m py_compile tests/cmux.py tests/test_files_sidebar_no_redraw_on_close_surface.py
The PR branch needed the latest main before CI iteration. The only conflict was in the Files sidebar search layout path; the resolution preserves main's search height changes while keeping this branch's idempotent AppKit layout guard. Constraint: origin/main advanced with right-sidebar Find typing-lag changes that touched the same Files search layout method. Rejected: Rebase the branch | keeping the PR history stable avoids rewriting already published commits. Confidence: high Scope-risk: moderate Directive: Preserve searchBarVisibleHeight from main when editing the Files search layout path. Tested: ./scripts/reload.sh --tag issue-3742-files-sidebar-flicker Tested: git diff --check
Merging main brought the current window-drag API, where temporary movability is covered by withTemporaryWindowMovableEnabled and suppression depth is covered separately. The old disable/restore helper tests referenced functions that no longer exist, so this removes the stale assertions while keeping the current lifecycle coverage. Constraint: CircleCI macos-unit-tests failed at compile time before running the Files tests. Rejected: Reintroduce temporarilyDisableWindowDragging helpers | the production API has moved to scoped temporary movability plus suppression depth. Confidence: high Scope-risk: narrow Tested: xcodebuild -project GhosttyTabs.xcodeproj -scheme cmux-unit -destination 'platform=macOS' -derivedDataPath /Users/austinwang/Library/Developer/Xcode/DerivedData/cmux-issue-3742-files-sidebar-flicker-unit -only-testing:cmuxTests/FolderWindowMoveSuppressionTests test Tested: xcodebuild -project GhosttyTabs.xcodeproj -scheme cmux-unit -destination 'platform=macOS' -derivedDataPath /Users/austinwang/Library/Developer/Xcode/DerivedData/cmux-issue-3742-files-sidebar-flicker-unit -only-testing:cmuxTests/FileExplorerStoreTests -only-testing:cmuxTests/FolderWindowMoveSuppressionTests test
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/FileExplorerStore.swift (1)
515-535:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDrop stale async git-status results when root changes.
The callbacks at Lines 527 and 534 apply status for the captured
pathwithout verifying the current root. If the user switches folders before the async fetch returns, stale status can overwrite the active tree decoration and bumpoutlineRevisionincorrectly.🛡️ Proposed fix
DispatchQueue.main.async { [weak self] in - self?.applyGitStatus(status) + guard let self, self.rootPath == path else { return } + self.applyGitStatus(status) }Apply the same guard in both SSH and local branches.
🤖 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 `@Sources/FileExplorerStore.swift` around lines 515 - 535, The async callbacks use a captured `path` and unconditionally call `self?.applyGitStatus(status)`, which can overwrite the active tree if the root changed; in both the SSH branch (GitStatusProvider.fetchStatusSSH) and the local branch (GitStatusProvider.fetchStatus) add a guard inside the DispatchQueue.main.async closure that verifies the store's current `rootPath` still equals the captured `path` (e.g. guard let self = self, self.rootPath == path else { return }) before calling `applyGitStatus(status)` so stale results don't update decorations or bump `outlineRevision`.
🤖 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/FileExplorerStore.swift`:
- Around line 692-695: applyGitStatus currently sets gitStatusByPath inside
mutateOutline unconditionally which increments outlineRevision every git
refresh; change applyGitStatus to compare the incoming status with the existing
gitStatusByPath and only call mutateOutline when they differ. Specifically, in
applyGitStatus (the function name) perform an equality check between the new
status and the store's gitStatusByPath (or use a deep/element-wise compare), and
only then update gitStatusByPath inside mutateOutline so outlineRevision is
incremented only on real changes.
In `@tests/test_files_sidebar_no_redraw_on_close_surface.py`:
- Around line 93-98: The except block handling cmuxError currently raises
SystemExit(1) without preserving the original exception context; change the
raise to use exception chaining so the cmuxError remains the __cause__ (i.e.,
use "raise SystemExit(1) from exc" in the except cmuxError as exc block) and
keep the existing print/fail message and the try/raise SystemExit(main()) around
main() unchanged.
---
Outside diff comments:
In `@Sources/FileExplorerStore.swift`:
- Around line 515-535: The async callbacks use a captured `path` and
unconditionally call `self?.applyGitStatus(status)`, which can overwrite the
active tree if the root changed; in both the SSH branch
(GitStatusProvider.fetchStatusSSH) and the local branch
(GitStatusProvider.fetchStatus) add a guard inside the DispatchQueue.main.async
closure that verifies the store's current `rootPath` still equals the captured
`path` (e.g. guard let self = self, self.rootPath == path else { return })
before calling `applyGitStatus(status)` so stale results don't update
decorations or bump `outlineRevision`.
🪄 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: 24ef93e9-0a6e-4546-acf5-070073aed525
📒 Files selected for processing (6)
Sources/FileExplorerStore.swiftSources/FileExplorerView.swiftSources/TerminalController.swiftcmuxTests/FileExplorerStoreTests.swifttests/cmux.pytests/test_files_sidebar_no_redraw_on_close_surface.py
The redraw gate depends on outlineRevision representing real Files-owned visible changes. Git status refreshes now ignore stale async results after the root changes and avoid bumping outlineRevision when the status map is unchanged. Constraint: Git status fetches run off-main and can complete after the Files root has changed. Rejected: Let every git poll bump outlineRevision | it can still create unnecessary row walks under the new redraw gate. Confidence: high Scope-risk: narrow Directive: Keep async Files metadata callbacks root-checked before applying decorations. Tested: xcodebuild -project GhosttyTabs.xcodeproj -scheme cmux-unit -destination 'platform=macOS' -derivedDataPath /Users/austinwang/Library/Developer/Xcode/DerivedData/cmux-issue-3742-files-sidebar-flicker-unit -only-testing:cmuxTests/FileExplorerStoreTests test Tested: python3 -m py_compile tests/cmux.py tests/test_files_sidebar_no_redraw_on_close_surface.py Tested: git diff --check
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 9 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/FileExplorerSearchController.swift">
<violation number="1" location="Sources/FileExplorerSearchController.swift:417">
P2: Avoid showing absolute executable paths in user-facing error text; this exposes local environment details.</violation>
</file>
<file name="Sources/FileExplorerStore.swift">
<violation number="1" location="Sources/FileExplorerStore.swift:732">
P2: Path-only staleness checks can apply git status from a previous SSH connection when the new workspace has the same root path. Include provider/connection identity in the callback guard.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Re-trigger cubic
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/FileExplorerSearchController.swift">
<violation number="1" location="Sources/FileExplorerSearchController.swift:422">
P3: Remove the `.unsupported` switch arm here; it is unreachable after the earlier `guard scope != .unsupported` return.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Re-trigger cubic
…-3742-files-sidebar-flicker
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 68d3626. Configure here.
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 #3742.
Repro:
Root cause:
Fix:
Regression test:
Verification so far:
Not run locally:
Note
Medium Risk
Medium risk because it changes Files sidebar state invalidation and search process execution (including SSH command construction), which could affect UI refresh correctness and search behavior across local/remote providers.
Overview
Fixes Files sidebar flicker by gating AppKit outline reload/refresh on a store-owned
outlineRevision, so unrelated SwiftUI updates (like closing a split) no longer repaint the file tree.Refactors Files state mutations to be idempotent and revision-scoped (expand/collapse no-ops, silent prefetch, git status updates deduped and stale-result guarded), and adds
FileSearchScopeto run Find searches locally viargor remotely via/usr/bin/sshwith shared shell quoting and clearer localized error messaging.Adds DEBUG-only redraw counters and terminal/Python plumbing plus new unit/integration tests to assert no outline/layout invalidation during split close and to cover the new revision/search behaviors.
Reviewed by Cursor Bugbot for commit 590a71e. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Fixes #3742 by removing the Files sidebar flicker when closing a split. Redraws are now gated by a Files-owned
outlineRevision, UI layout updates are idempotent, and Find adds SSH support with clearer errors.Bug Fixes
outlineRevision; ignore unrelated parent SwiftUI updates.FileSearchScope(local vs remote over/usr/bin/ssh); improved messages: “Search unavailable” and “Search executable is missing: %@”.ShellCommandQuoting.singleQuotedfor consistent command quoting.Tests
file_explorer_debug_counts/reset_file_explorer_debug_counts; new Python test asserts no outline reload/refresh and no search-layout invalidations on split close.Written for commit 590a71e. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
Performance & Stability
Bug Fixes
Tests
New Features
Closes #3742.