Repository navigation
Add workspace stress profiling and reduce switch churn - #1218
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a workspace handoff ready-check task and refactors tab UI payloads in ContentView; introduces DEBUG-only staged workspace-switch tracing in TabManager; adds a new end-to-end workspace stress test and wires it into the Xcode project. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 4 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="cmuxTests/WorkspaceStressProfileTests.swift">
<violation number="1" location="cmuxTests/WorkspaceStressProfileTests.swift:206">
P1: A failed surface creation can cause an infinite loop in `populate`, because the loop condition never changes after `XCTAssertNotNil` fails.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/ContentView.swift (1)
9492-9501:⚠️ Potential issue | 🟡 MinorInclude
accessibilityWorkspaceCountinTabItemView.==.Line 9515 now feeds Line 10241, but Lines 9492-9501 never compare it. With
.equatable()on each row, adding/removing a workspace can leave VoiceOver announcing a stale “workspace x of y” count when the other compared fields stay unchanged.Based on learnings: In `ContentView.swift` TabItemView, do not add `EnvironmentObject`, `ObservedObject` (besides `tab`), or `Binding` properties without updating the `==` function and keeping `.equatable()` on ForEach.Proposed fix
nonisolated static func == (lhs: TabItemView, rhs: TabItemView) -> Bool { lhs.tab === rhs.tab && lhs.index == rhs.index && lhs.isActive == rhs.isActive && lhs.workspaceShortcutDigit == rhs.workspaceShortcutDigit && lhs.canCloseWorkspace == rhs.canCloseWorkspace && + lhs.accessibilityWorkspaceCount == rhs.accessibilityWorkspaceCount && lhs.unreadCount == rhs.unreadCount && lhs.latestNotificationText == rhs.latestNotificationText && lhs.rowSpacing == rhs.rowSpacing && lhs.showsModifierShortcutHints == rhs.showsModifierShortcutHints }Also applies to: 9513-9515, 10240-10242
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ContentView.swift` around lines 9492 - 9501, The equality operator for TabItemView is missing a comparison for accessibilityWorkspaceCount which causes VoiceOver to keep stale "workspace x of y" announcements when rows are Equatable; update nonisolated static func == (lhs: TabItemView, rhs: TabItemView) to include lhs.accessibilityWorkspaceCount == rhs.accessibilityWorkspaceCount alongside the other field comparisons, and mirror the same addition wherever TabItemView equality is implemented/used (referenced ranges around the existing == implementation and the other noted locations corresponding to lines 9513-9515 and 10240-10242) so .equatable() on the ForEach correctly detects workspace count changes.
🤖 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/TabManager.swift`:
- Around line 2071-2076: Avoid emitting a fake "prepare" switch when the cycle
is a no-op: check whether the target id equals the current id before calling
debugPrepareWorkspaceSwitch (e.g., in the blocks around
activateWorkspaceCycleHotWindow and where selectedTabId is set). If
nextId/prevId == currentId, skip calling debugPrepareWorkspaceSwitch (and any
related begin switch logging) so that selectedTabId remains unchanged and no
stale debug switch timing is recorded. Apply the same guard to the other similar
blocks referenced (around lines with prevId/nextId and the range 2168-2184).
---
Outside diff comments:
In `@Sources/ContentView.swift`:
- Around line 9492-9501: The equality operator for TabItemView is missing a
comparison for accessibilityWorkspaceCount which causes VoiceOver to keep stale
"workspace x of y" announcements when rows are Equatable; update nonisolated
static func == (lhs: TabItemView, rhs: TabItemView) to include
lhs.accessibilityWorkspaceCount == rhs.accessibilityWorkspaceCount alongside the
other field comparisons, and mirror the same addition wherever TabItemView
equality is implemented/used (referenced ranges around the existing ==
implementation and the other noted locations corresponding to lines 9513-9515
and 10240-10242) so .equatable() on the ForEach correctly detects workspace
count changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1708cf2d-86ca-424c-9b7e-710f54c9a28a
📒 Files selected for processing (4)
GhosttyTabs.xcodeproj/project.pbxprojSources/ContentView.swiftSources/TabManager.swiftcmuxTests/WorkspaceStressProfileTests.swift
Greptile SummaryThis PR adds a workspace stress-profiling unit test, instruments debug workspace-switch logging with real trigger IDs for socket-driven
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Caller as Caller (socket / UI)
participant TM as TabManager
participant CV as ContentView (MainActor)
participant RCT as workspaceHandoffReadyCheckTask
participant FBT as workspaceHandoffFallbackTask
Caller->>TM: selectWorkspace(_:) / selectNextTab() / selectPreviousTab()
TM->>TM: debugPrimeWorkspaceSwitchTrigger / debugPrepareWorkspaceSwitch
TM->>TM: selectedTabId = newId (willSet → debugBeginWorkspaceSwitch)
TM-->>CV: @Published selectedTabId change
CV->>CV: startWorkspaceHandoffIfNeeded(newSelectedId:)
CV->>CV: workspaceHandoffGeneration &+= 1
CV->>CV: cancel previous RCT & FBT
CV->>RCT: Task { for delay in [0, 20ms, 40ms, 60ms] }
CV->>FBT: Task { sleep(150ms) }
loop Each delay checkpoint
RCT->>CV: await MainActor.run { canCompleteWorkspaceHandoffImmediately? }
alt Surface loaded / browser panel focused
CV->>CV: completeWorkspaceHandoff(reason: "ready")
CV->>RCT: cancel (task exits via CancellationError on next sleep)
CV->>FBT: cancel
else Not ready yet
RCT->>RCT: sleep next delay
end
end
alt Fallback fires (150ms, surface still not ready)
FBT->>CV: await MainActor.run { completeWorkspaceHandoff(reason: "timeout") }
end
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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: 976fcee32b
ℹ️ 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".
02d6644 to
4ef62db
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4ef62dbe31
ℹ️ 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".
| dlog("ws.handoff.fastReady id=none selected=\(debugShortWorkspaceId(newSelectedId))") | ||
| } | ||
| #endif | ||
| completeWorkspaceHandoff(reason: "ready") |
There was a problem hiding this comment.
This ready-path completion can fire on the first poll when switching to an already-loaded workspace, but TabManager only creates pendingWorkspaceUnfocusTarget in the async selectedTabId.didSet side-effects (DispatchQueue.main.async → focusSelectedTabPanel). If this runs first, completePendingWorkspaceUnfocus no-ops, fallback is canceled, and later focus notifications skip completion because retiringWorkspaceId is already cleared, leaving the previous workspace unfocus deferred until a later switch. The race is most visible during rapid workspace switches where the target workspace is already warm.
Useful? React with 👍 / 👎.
This branch was successfully deployed
1 participant
Summary
select-workspaceruns show real switch IDs in debug logsTesting
ssh cmux-macmini 'cd /Users/cmux/fun/cmuxterm-hq/worktrees/task-workspace-stress-profile-switching-lag && xcodebuild -project GhosttyTabs.xcodeproj -scheme cmux-unit -destination "platform=macOS" -derivedDataPath /tmp/cmux-workspace-stress-profile-fix6 test -only-testing:cmuxTests/WorkspaceStressProfileTests/testWorkspaceCreationAndSwitchingStressProfile -quiet'ssh cmux-macminiruntime benchmark with tagged debug app andCMUX_DISABLE_SESSION_RESTORE=1: 20 workspaces, 10 surfaces eachcreateMsstayed around190-210ms, explicitselectMsaround240-360ms, andpopulateMsrose from about1.4sto about2.1s/tmp/cmux-debug-wsrtfix7.log:ws.view.selectedChangeavg11.63ms, p9513.48ms;ws.handoff.completeavg94.95ms, p95102.52ms, max122.76msTask
cmux-macminiSummary by cubic
Adds a workspace stress profiling test and speeds up workspace switching with a fast-ready handoff and less sidebar churn. Improves debug logs for consistent switch timing and IDs.
New Features
WorkspaceStressProfileTeststo create many workspaces, populate terminal surfaces, and time create/populate/switch (dispatch/drain1/unfocus/drain2); prints a report and attaches it.Refactors
ws.handoff.fastReady.workspaceShortcutDigit,canCloseWorkspace, andaccessibilityWorkspaceCountintoTabItemViewwith tighterEquatablekeys; close button and accessibility title now use these values.TabManager: prime triggers for “create”, “select”, “focus”, “select_index”, and prepare logs for “next/prev”;selectedTabIdwillSet ensuresws.switch.beginhas consistent IDs, triggers, and start times across all paths.Written for commit 4ef62db. Summary will update on new commits.
Summary by CodeRabbit
New Features
Improvements
Tests