Repository navigation
Reduce sidebar CPU by splitting summary and detail invalidation - #2527
lawrencecchen wants to merge 2 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe changes split the monolithic Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d4b9916515
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| lhs.detailVisibility == rhs.detailVisibility && | ||
| lhs.sidebarBranchVerticalLayout == rhs.sidebarBranchVerticalLayout && | ||
| lhs.sidebarShowGitBranch == rhs.sidebarShowGitBranch && | ||
| lhs.sidebarShowGitBranchIcon == rhs.sidebarShowGitBranchIcon && | ||
| lhs.isActive == rhs.isActive |
There was a problem hiding this comment.
The new Equatable check for SidebarWorkspaceStructuredDetailsSection omits any value tied to onOpenPullRequest, so changing the openPullRequestLinksInCmuxBrowser setting can leave this subtree treated as unchanged and keep the old action closure alive. In that case, clicking a pull-request row right after toggling the setting can still use the previous open target (external browser vs in-app browser) until an unrelated detail invalidation (e.g., branch/PR data change) forces a rebuild.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR reduces sidebar CPU usage by splitting the single Confidence Score: 5/5Safe to merge — all remaining findings are P2 style suggestions with no correctness or reliability impact. The publisher split is architecturally sound: summary and detail channels are correctly partitioned, the Equatable + @State generation mechanism works as intended, and three targeted unit tests validate the boundary. Both open comments are low-priority clean-ups (duplicate remoteConfiguration slot, computed debounce chain), neither of which affects correctness. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Workspace state change] --> B{Which field changed?}
B -->|title / isPinned / customColor\nstatus / metadata / log\nprogress / ports / remote state| C[sidebarSummaryObservationPublisher]
B -->|currentDirectory / panels\npanelDirectories / gitBranch\npanelGitBranches / pullRequest\npanelPullRequests / remoteConfiguration| D[sidebarDetailObservationPublisher]
B -->|remoteConfiguration| C
C -->|debounce| E[TabItemView\nworkspaceObservationGeneration ++]
D -->|debounce| F[SidebarWorkspaceStructuredDetailsSection\ndetailObservationGeneration ++]
E --> G[TabItemView.body re-evaluates\nsummary row redraws]
F --> H[Detail subtree body re-evaluates\nbranch / directory / PR rows redrawn]
G -->|.equatable check| I{6 primitive props equal?}
I -->|yes - skip body| J[Detail subtree unchanged]
I -->|no| H
Reviews (1): Last reviewed commit: "Split sidebar summary and detail invalid..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| sidebarObservationSignal($panelGitBranches), | ||
| sidebarObservationSignal($pullRequest), | ||
| sidebarObservationSignal($panelPullRequests), | ||
| sidebarObservationSignal($remoteConfiguration), |
There was a problem hiding this comment.
$remoteConfiguration appears in both publishers
$remoteConfiguration is already included in sidebarSummaryObservationPublisher (line 5622) for the remote-connection status row. Including it here as well means every remote config change fires the detail publisher too, causing the branch/PR subtree to re-render even when neither the branches nor the PRs themselves changed. If the detail section only needs remoteConfiguration to determine which remote to query (and that result is already reflected in $gitBranch/$pullRequest), this slot could be dropped to keep the split clean.
| private var detailObservationPublisher: AnyPublisher<Void, Never> { | ||
| guard showsAnyDetails else { | ||
| return Empty(completeImmediately: false).eraseToAnyPublisher() | ||
| } | ||
|
|
||
| return tab.sidebarDetailObservationPublisher | ||
| .receive(on: RunLoop.main) | ||
| // Prompt-time sidebar telemetry can arrive as a short burst | ||
| // (pwd, branch, PR, shell state). Coalesce that burst so the | ||
| // details subtree redraws once with the settled state. | ||
| .debounce(for: TabItemView.workspaceObservationCoalesceInterval, scheduler: RunLoop.main) | ||
| .eraseToAnyPublisher() | ||
| } |
There was a problem hiding this comment.
detailObservationPublisher is a computed property, so each time SwiftUI re-evaluates body (e.g., on every detailObservationGeneration increment) it produces a brand-new .debounce operator chain. SwiftUI's onReceive detects the changed publisher identity and re-subscribes, resetting the in-flight debounce timer. In practice the timer has already fired before the next render so no events are dropped, and the pattern is consistent with TabItemView's existing inline chain — this is a low-priority clean-up only.
![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
🧹 Nitpick comments (1)
cmuxTests/WorkspaceUnitTests.swift (1)
2286-2321: Add an explicit pre-focus baseline assertion for stronger causality.You can make this test more deterministic by asserting
publishCount == 0right beforefocusPanel, so the final> 0is clearly attributable to the focus-triggered PR detail change.Suggested test hardening
var publishCount = 0 let cancellable = workspace.sidebarDetailObservationPublisher.sink { publishCount += 1 } defer { cancellable.cancel() } + XCTAssertEqual( + publishCount, + 0, + "Expected no detail invalidation before focused panel change" + ) + workspace.focusPanel(secondPanel.id) XCTAssertGreaterThan( publishCount, 0,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/WorkspaceUnitTests.swift` around lines 2286 - 2321, Add an explicit pre-focus baseline assertion so the publishCount change is clearly caused by focusing the panel: after creating the subscriber to workspace.sidebarDetailObservationPublisher and before calling workspace.focusPanel(secondPanel.id), assert publishCount == 0 (using XCTAssertEqual or XCTAssert) to ensure no prior publishes occurred; refer to the subscriber variable publishCount, the publisher sidebarDetailObservationPublisher, and the focus action workspace.focusPanel(secondPanel.id).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/ContentView.swift`:
- Around line 12254-12279: The Equatable implementation for
SidebarWorkspaceStructuredDetailsSection omits the onOpenPullRequest closure, so
changes to the “open pull request links in cmux” setting don’t cause the view to
update; update the nonisolated static func == to also compare
lhs.onOpenPullRequest and rhs.onOpenPullRequest (e.g., by comparing function
identity or wrapping the closure in an Equatable container) so the .equatable()
wrapper invalidates the view when the routing handler changes, touching the
SidebarWorkspaceStructuredDetailsSection type and its == implementation.
---
Nitpick comments:
In `@cmuxTests/WorkspaceUnitTests.swift`:
- Around line 2286-2321: Add an explicit pre-focus baseline assertion so the
publishCount change is clearly caused by focusing the panel: after creating the
subscriber to workspace.sidebarDetailObservationPublisher and before calling
workspace.focusPanel(secondPanel.id), assert publishCount == 0 (using
XCTAssertEqual or XCTAssert) to ensure no prior publishes occurred; refer to the
subscriber variable publishCount, the publisher
sidebarDetailObservationPublisher, and the focus action
workspace.focusPanel(secondPanel.id).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: be3435c8-b1ea-4471-bef7-85a6d881a73a
📒 Files selected for processing (3)
Sources/ContentView.swiftSources/Workspace.swiftcmuxTests/WorkspaceUnitTests.swift
| private struct SidebarWorkspaceStructuredDetailsSection: View, Equatable { | ||
| nonisolated static func == ( | ||
| lhs: SidebarWorkspaceStructuredDetailsSection, | ||
| rhs: SidebarWorkspaceStructuredDetailsSection | ||
| ) -> Bool { | ||
| lhs.tab === rhs.tab && | ||
| lhs.detailVisibility == rhs.detailVisibility && | ||
| lhs.sidebarBranchVerticalLayout == rhs.sidebarBranchVerticalLayout && | ||
| lhs.sidebarShowGitBranch == rhs.sidebarShowGitBranch && | ||
| lhs.sidebarShowGitBranchIcon == rhs.sidebarShowGitBranchIcon && | ||
| lhs.isActive == rhs.isActive | ||
| } | ||
|
|
||
| let tab: Tab | ||
| let detailVisibility: SidebarWorkspaceAuxiliaryDetailVisibility | ||
| let sidebarBranchVerticalLayout: Bool | ||
| let sidebarShowGitBranch: Bool | ||
| let sidebarShowGitBranchIcon: Bool | ||
| let isActive: Bool | ||
| let branchDirectoryRow: (_ gitSummary: String?, _ directorySummary: String?) -> String? | ||
| let gitBranchSummaryText: ([UUID]) -> String? | ||
| let directorySummaryText: ([UUID]) -> String? | ||
| let verticalBranchDirectoryLines: ([UUID]) -> [VerticalBranchDirectoryLine] | ||
| let pullRequestDisplays: ([UUID]) -> [PullRequestDisplay] | ||
| let pullRequestStatusLabel: (SidebarPullRequestStatus, SidebarPullRequestChecksStatus?) -> String | ||
| let onOpenPullRequest: (URL) -> Void |
There was a problem hiding this comment.
Include pull-request link routing in this view’s equality surface.
onOpenPullRequest changes when the “open pull request links in cmux” setting flips, but this Equatable view never sees that input. With the .equatable() wrapper on Line 11634, the pull-request buttons can keep the old open-in-cmux/open-external behavior until some unrelated detail invalidation happens.
💡 Minimal fix
SidebarWorkspaceStructuredDetailsSection(
tab: tab,
detailVisibility: detailVisibility,
sidebarBranchVerticalLayout: sidebarBranchVerticalLayout,
sidebarShowGitBranch: sidebarShowGitBranch,
sidebarShowGitBranchIcon: sidebarShowGitBranchIcon,
isActive: usesInvertedActiveForeground,
+ openPullRequestLinksInCmuxBrowser: openSidebarPullRequestLinksInCmuxBrowser,
branchDirectoryRow: branchDirectoryRow,
gitBranchSummaryText: gitBranchSummaryText,
directorySummaryText: directorySummaryText,
verticalBranchDirectoryLines: verticalBranchDirectoryLines,
pullRequestDisplays: pullRequestDisplays,
pullRequestStatusLabel: pullRequestStatusLabel,
onOpenPullRequest: openPullRequestLink
)
.equatable()
@@
lhs.detailVisibility == rhs.detailVisibility &&
lhs.sidebarBranchVerticalLayout == rhs.sidebarBranchVerticalLayout &&
lhs.sidebarShowGitBranch == rhs.sidebarShowGitBranch &&
lhs.sidebarShowGitBranchIcon == rhs.sidebarShowGitBranchIcon &&
- lhs.isActive == rhs.isActive
+ lhs.isActive == rhs.isActive &&
+ lhs.openPullRequestLinksInCmuxBrowser == rhs.openPullRequestLinksInCmuxBrowser
@@
let sidebarShowGitBranch: Bool
let sidebarShowGitBranchIcon: Bool
let isActive: Bool
+ let openPullRequestLinksInCmuxBrowser: Bool
let branchDirectoryRow: (_ gitSummary: String?, _ directorySummary: String?) -> String?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/ContentView.swift` around lines 12254 - 12279, The Equatable
implementation for SidebarWorkspaceStructuredDetailsSection omits the
onOpenPullRequest closure, so changes to the “open pull request links in cmux”
setting don’t cause the view to update; update the nonisolated static func == to
also compare lhs.onOpenPullRequest and rhs.onOpenPullRequest (e.g., by comparing
function identity or wrapping the closure in an Equatable container) so the
.equatable() wrapper invalidates the view when the routing handler changes,
touching the SidebarWorkspaceStructuredDetailsSection type and its ==
implementation.
![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 3 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/Workspace.swift">
<violation number="1" location="Sources/Workspace.swift:5641">
P2: `remoteConfiguration` is wired into both split publishers, so the merged `sidebarObservationPublisher` can publish twice for one state change.</violation>
</file>
<file name="Sources/ContentView.swift">
<violation number="1" location="Sources/ContentView.swift:12264">
P2: The `Equatable` conformance doesn't account for changes to the pull-request open mode. When the user toggles "open PR links in cmux browser", the `onOpenPullRequest` closure captures the new behavior, but `.equatable()` will skip `body` re-evaluation because none of the compared properties changed. PR button taps will keep using the stale closure until an unrelated detail change forces a rebuild.
Add an `openPullRequestLinksInCmuxBrowser: Bool` property to this struct and include it in the `==` check so the view invalidates when the setting flips.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| sidebarObservationSignal($panelGitBranches), | ||
| sidebarObservationSignal($pullRequest), | ||
| sidebarObservationSignal($panelPullRequests), | ||
| sidebarObservationSignal($remoteConfiguration), |
There was a problem hiding this comment.
P2: remoteConfiguration is wired into both split publishers, so the merged sidebarObservationPublisher can publish twice for one state change.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/Workspace.swift, line 5641:
<comment>`remoteConfiguration` is wired into both split publishers, so the merged `sidebarObservationPublisher` can publish twice for one state change.</comment>
<file context>
@@ -5632,6 +5629,26 @@ final class Workspace: Identifiable, ObservableObject {
+ sidebarObservationSignal($panelGitBranches),
+ sidebarObservationSignal($pullRequest),
+ sidebarObservationSignal($panelPullRequests),
+ sidebarObservationSignal($remoteConfiguration),
+ ]
+
</file context>
| lhs.sidebarBranchVerticalLayout == rhs.sidebarBranchVerticalLayout && | ||
| lhs.sidebarShowGitBranch == rhs.sidebarShowGitBranch && | ||
| lhs.sidebarShowGitBranchIcon == rhs.sidebarShowGitBranchIcon && | ||
| lhs.isActive == rhs.isActive |
There was a problem hiding this comment.
P2: The Equatable conformance doesn't account for changes to the pull-request open mode. When the user toggles "open PR links in cmux browser", the onOpenPullRequest closure captures the new behavior, but .equatable() will skip body re-evaluation because none of the compared properties changed. PR button taps will keep using the stale closure until an unrelated detail change forces a rebuild.
Add an openPullRequestLinksInCmuxBrowser: Bool property to this struct and include it in the == check so the view invalidates when the setting flips.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/ContentView.swift, line 12264:
<comment>The `Equatable` conformance doesn't account for changes to the pull-request open mode. When the user toggles "open PR links in cmux browser", the `onOpenPullRequest` closure captures the new behavior, but `.equatable()` will skip `body` re-evaluation because none of the compared properties changed. PR button taps will keep using the stale closure until an unrelated detail change forces a rebuild.
Add an `openPullRequestLinksInCmuxBrowser: Bool` property to this struct and include it in the `==` check so the view invalidates when the setting flips.</comment>
<file context>
@@ -12354,6 +12251,191 @@ private struct TabItemView: View, Equatable {
+ lhs.sidebarBranchVerticalLayout == rhs.sidebarBranchVerticalLayout &&
+ lhs.sidebarShowGitBranch == rhs.sidebarShowGitBranch &&
+ lhs.sidebarShowGitBranchIcon == rhs.sidebarShowGitBranchIcon &&
+ lhs.isActive == rhs.isActive
+ }
+
</file context>
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
2 participants
Summary
Testing
ssh cmux-macmini 'set -e; cd /Users/cmux/fun/cmux-worktrees/issue-2487-sidebar-high-cpu-test; rm -rf /tmp/cmux-issue-2487-sidebar-high-cpu-test; CMUX_SKIP_ZIG_BUILD=1 xcodebuild -project GhosttyTabs.xcodeproj -scheme cmux-unit -destination "platform=macOS" -derivedDataPath /tmp/cmux-issue-2487-sidebar-high-cpu-test test -only-testing:cmuxTests/WorkspacePanelGitBranchTests/testUpdatingFocusedPanelGitBranchWithSameStateDoesNotRepublishWorkspace -only-testing:cmuxTests/WorkspacePanelGitBranchTests/testUpdatingFocusedPanelPullRequestWithSameStateDoesNotRepublishWorkspace -only-testing:cmuxTests/WorkspacePanelGitBranchTests/testSidebarObservationPublisherEmitsForFocusedGitBranchChangesOnlyOncePerState -only-testing:cmuxTests/WorkspacePanelGitBranchTests/testSidebarObservationPublisherIgnoresRemoteHeartbeatOnlyChanges -only-testing:cmuxTests/WorkspacePanelGitBranchTests/testSidebarSummaryObservationPublisherIgnoresDetailOnlyGitUpdates -only-testing:cmuxTests/WorkspacePanelGitBranchTests/testSidebarDetailObservationPublisherIgnoresSummaryOnlyStatusUpdates -only-testing:cmuxTests/WorkspacePanelGitBranchTests/testSidebarDetailObservationPublisherEmitsWhenFocusedPullRequestChanges'Closes #2487
Summary by cubic
Split sidebar invalidation into a lightweight summary stream and a detail-only stream to reduce redraws and CPU. Branch, directory, and PR UI now update separately from status/metadata/log/ports.
sidebarSummaryObservationPublisher(status, metadata, log, progress, ports) andsidebarDetailObservationPublisher(directory, branch, PR);sidebarObservationPublishernow merges both.SidebarWorkspaceStructuredDetailsSectionand moved branch/directory/PR rendering into this detail subtree;TabItemViewnow listens to summary updates, details section to detail updates, both debounced.sidebarPanelsObservationSignal()to consolidate panel change observation and reduce duplicate work.Written for commit d4b9916. Summary will update on new commits.
Summary by CodeRabbit
Refactor
Tests