Repository navigation
Fix sidebar row-ID layout preference feedback - #5637
austinywang wants to merge 2 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR eliminates a sustained CPU loop in the SwiftUI layout engine by removing workspace row-ID preference accumulation, replacing deferred preference-driven scrolling with direct conditional calls, and adding regression tests with CI detection to prevent reintroduction. ChangesSidebar Workspace Scrolling: Preference Loop Fix
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 17 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (17 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a 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 |
Greptile SummaryThis PR fixes a SwiftUI/AttributeGraph non-converging layout loop (issue #5570) caused by
Confidence Score: 4/5Safe to merge; the layout-loop root cause is cleanly removed and the regression tests lock it in place. The workspace-order-change handler now calls proxy.scrollTo synchronously before the LazyVStack re-renders the new row positions, so the scroll can land on the workspace's old offset rather than its settled new one. This is a narrow edge case limited to programmatic reorders of the selected workspace while it is off-screen, and it does not affect the core layout-loop fix or any data integrity. Sources/ContentView.swift — specifically the onReceive(.workspaceOrderDidChange) closure and the timing of scrollSelectedWorkspaceIfNeeded relative to the subsequent render pass. Important Files Changed
Sequence DiagramsequenceDiagram
participant TM as TabManager
participant SV as VerticalTabsSidebar
participant LV as LazyVStack rows
participant SP as ScrollViewProxy
note over SV,LV: OLD (layout-loop path)
LV->>SV: .preference(SidebarWorkspaceRowIdsPreferenceKey, Set([tab.id]))
SV->>SV: "onPreferenceChange → laidOutWorkspaceRowIds += id"
SV->>SP: flushPendingSelectedWorkspaceScroll → proxy.scrollTo
note over LV,SP: Set.formUnion per row → layout loop (issue 5570)
note over SV,LV: NEW (fixed path)
TM->>SV: selectedTabId changes / workspaceOrderDidChange
SV->>SP: scrollSelectedWorkspaceIfNeeded → proxy.scrollTo(selectedId)
note over LV,SP: No preference emission, no aggregation
Reviews (1): Last reviewed commit: "Remove sidebar row-id layout preference ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| private func shouldScrollSelectedWorkspaceAfterWorkspaceOrderChange(_ notification: Notification) -> Bool { | ||
| guard let manager = notification.object as? TabManager, manager === tabManager else { | ||
| return | ||
| return false | ||
| } | ||
| guard let selectedWorkspaceId = tabManager.selectedTabId else { return } | ||
| guard let selectedWorkspaceId = tabManager.selectedTabId else { return false } | ||
| let movedWorkspaceIds = notification.userInfo?[WorkspaceOrderChangeNotificationKey.movedWorkspaceIds] as? [UUID] ?? [] | ||
| guard movedWorkspaceIds.contains(selectedWorkspaceId) else { return } | ||
| pendingSelectedWorkspaceScrollId = selectedWorkspaceId | ||
| return movedWorkspaceIds.contains(selectedWorkspaceId) |
There was a problem hiding this comment.
shouldScrollSelectedWorkspaceAfterWorkspaceOrderChange returning true causes scrollSelectedWorkspaceIfNeeded to call proxy.scrollTo synchronously during onReceive, before SwiftUI processes the updated tabManager.tabs into a new render. At that point renderContext.workspaceIds still carries the pre-move ordering and the LazyVStack still places the row at its old offset, so the scroll view jumps to the old position. After the subsequent render the row physically moves to its new slot, leaving the scroll position stale. The old preference-based flush path fired post-layout (via onPreferenceChange), so it always scrolled to the settled position. For visible-row drag reorders the effect is imperceptible, but programmatic moves that shift the selected workspace far in the list can leave scroll state stale.
![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 `@cmuxTests/SidebarWorkspaceSnapshotRefreshPolicyTests.swift`:
- Line 2: Remove the unused top-level import by deleting the line "import
CmuxUpdater" from SidebarWorkspaceSnapshotRefreshPolicyTests.swift; all test
types are already available via "`@testable` import cmux", so removing this
redundant import (the lone occurrence of CmuxUpdater in the file) will eliminate
unnecessary module wiring.
🪄 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: 5423ebfb-c3da-44b7-93a2-ded3e554d2b0
📒 Files selected for processing (6)
.github/workflows/ci.ymlSources/ContentView.swiftSources/VerticalTabsSidebar+WorkspaceGroups.swiftcmux.xcodeproj/project.pbxprojcmuxTests/AAASidebarWorkspaceRowIdPreferenceRegressionTests.swiftcmuxTests/SidebarWorkspaceSnapshotRefreshPolicyTests.swift
💤 Files with no reviewable changes (1)
- Sources/VerticalTabsSidebar+WorkspaceGroups.swift
| @@ -1,4 +1,5 @@ | |||
| import AppKit | |||
| import CmuxUpdater | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if tests compile without the CmuxUpdater import
# Remove the import and try to build the tests
cd "$(git rev-parse --show-toplevel)"
# Create a temporary file without the import
sed '2d' cmuxTests/SidebarWorkspaceSnapshotRefreshPolicyTests.swift > /tmp/test_without_import.swift
# Try to compile just this test file
xcodebuild -project cmux.xcodeproj \
-scheme cmux \
-configuration Debug \
-destination 'platform=macOS' \
-derivedDataPath /tmp/cmux-verify-import \
build-for-testing \
2>&1 | grep -i "SidebarWorkspaceSnapshotRefreshPolicyTests"Repository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cd "$(git rev-parse --show-toplevel)"
echo "== command availability =="
command -v xcodebuild || true
echo
echo "== file excerpt =="
nl -ba cmuxTests/SidebarWorkspaceSnapshotRefreshPolicyTests.swift | head -n 200
echo
echo "== direct CmuxUpdater symbol usage in file =="
rg -n "CmuxUpdater|CmuxUpdater\." cmuxTests/SidebarWorkspaceSnapshotRefreshPolicyTests.swift || true
echo
echo "== quick compile check: build-for-testing baseline =="
# Note: this will likely fail in this sandbox if Xcode isn't available.
xcodebuild -project cmux.xcodeproj \
-scheme cmux \
-configuration Debug \
-destination 'platform=macOS' \
-derivedDataPath /tmp/cmux-verify-import-baseline \
build-for-testing 2>&1 | head -n 200 || trueRepository: manaflow-ai/cmux
Length of output: 169
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cd "$(git rev-parse --show-toplevel)"
echo "== tools =="
command -v rg || true
command -v cat || true
command -v sed || true
command -v head || true
command -v xcodebuild || true
echo
echo "== imports in SidebarWorkspaceSnapshotRefreshPolicyTests.swift =="
rg -n "^import " cmuxTests/SidebarWorkspaceSnapshotRefreshPolicyTests.swift || true
echo
echo "== CmuxUpdater symbol usage in file =="
rg -n "CmuxUpdater(\.|\\b)" cmuxTests/SidebarWorkspaceSnapshotRefreshPolicyTests.swift || true
echo
echo "== first ~120 lines with numbers (cat -n) =="
cat -n cmuxTests/SidebarWorkspaceSnapshotRefreshPolicyTests.swift | head -n 160
echo
echo "== whether file imports testable import cmux =="
rg -n "testable import cmux|`@testable` import cmux" cmuxTests/SidebarWorkspaceSnapshotRefreshPolicyTests.swift || true
echo
echo "== sample: other *Sidebar* test files import CmuxUpdater? =="
rg -n "^import CmuxUpdater" cmuxTests | head -n 50 || trueRepository: manaflow-ai/cmux
Length of output: 7964
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cd "$(git rev-parse --show-toplevel)"
echo "== tools =="
command -v rg || true
command -v cat || true
command -v sed || true
command -v head || true
command -v xcodebuild || true
echo
echo "== imports in SidebarWorkspaceSnapshotRefreshPolicyTests.swift =="
rg -n "^import " cmuxTests/SidebarWorkspaceSnapshotRefreshPolicyTests.swift || true
echo
echo "== CmuxUpdater symbol usage in file =="
rg -n "CmuxUpdater(\.|\\b)" cmuxTests/SidebarWorkspaceSnapshotRefreshPolicyTests.swift || true
echo
echo "== first ~160 lines with numbers (cat -n) =="
cat -n cmuxTests/SidebarWorkspaceSnapshotRefreshPolicyTests.swift | head -n 200
echo
echo "== whether file imports testable import cmux =="
rg -n "testable import cmux|`@testable` import cmux" cmuxTests/SidebarWorkspaceSnapshotRefreshPolicyTests.swift || true
echo
echo "== sample: other tests import CmuxUpdater? =="
rg -n "^import CmuxUpdater" cmuxTests | head -n 50 || trueRepository: manaflow-ai/cmux
Length of output: 9598
Remove the unused CmuxUpdater import from cmuxTests/SidebarWorkspaceSnapshotRefreshPolicyTests.swift.
import CmuxUpdater is the only occurrence of CmuxUpdater in this file; all referenced test types come via the existing @testable import cmux. Remove the import to reduce redundant module wiring.
🤖 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/SidebarWorkspaceSnapshotRefreshPolicyTests.swift` at line 2, Remove
the unused top-level import by deleting the line "import CmuxUpdater" from
SidebarWorkspaceSnapshotRefreshPolicyTests.swift; all test types are already
available via "`@testable` import cmux", so removing this redundant import (the
lone occurrence of CmuxUpdater in the file) will eliminate unnecessary module
wiring.
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
3 participants
Fixes #5570.
Summary:
Evidence:
Local tests were not run per repo policy.