Repository navigation
Reduce title-driven workspace list fanout - #6564
Eridanus117 wants to merge 1 commit into
Conversation
|
Someone is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughAdds a feature-flagged setting ChangesPanel Title Workspace List Fanout Feature Flag
Sequence Diagram(s)sequenceDiagram
rect rgba(173, 216, 230, 0.5)
Note over TabManager,Workspace: Panel title received from terminal
end
participant TabManager
participant PanelTitleWorkspaceListFanoutSettings
participant UserDefaultsSettingsClient
participant Workspace
participant MobileWorkspaceListObserver
TabManager->>PanelTitleWorkspaceListFanoutSettings: isEnabled(settings:)
PanelTitleWorkspaceListFanoutSettings->>UserDefaultsSettingsClient: value(for: titleUpdateWorkspaceListFanoutEnabled)
UserDefaultsSettingsClient-->>PanelTitleWorkspaceListFanoutSettings: Bool
PanelTitleWorkspaceListFanoutSettings-->>TabManager: updatesWorkspaceTitle: Bool
TabManager->>Workspace: updatePanelTitle(panelId:title:updatesWorkspaceTitle:)
alt updatesWorkspaceTitle == true
Workspace-->>Workspace: self.title = panelTitle
else updatesWorkspaceTitle == false
Workspace-->>Workspace: skip self.title update
end
rect rgba(255, 228, 181, 0.5)
Note over MobileWorkspaceListObserver: Hash computation at emit time
end
MobileWorkspaceListObserver->>MobileWorkspaceListObserver: summaryHash(includesAutomaticPanelTitles:)
MobileWorkspaceListObserver->>MobileWorkspaceListObserver: workspaceListPanelTitle(workspace:panelId:includesAutomaticPanelTitles:)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 1 warning)
✅ Passed checks (20 passed)
✨ Finishing Touches🧪 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 |
11dc5a3 to
31d0959
Compare
Greptile SummaryThis PR adds a
Confidence Score: 3/5The fanout-disabled path works correctly end-to-end for the default case, but the subscription system and hash computation diverge when the setting is toggled from disabled back to enabled at runtime without a process restart. The core logic in Workspace and TabManager is clean and the false → mobile-list-quiesce direction works correctly. The issue is in refreshPerWorkspaceSubscriptions: it captures the setting once at workspace-subscription time, so existing multi-panel workspaces built while the setting was false never acquire a $panelTitles subscription. After the setting flips to true, emitIfNeeded starts hashing automatic titles again, but those workspaces produce no wakeup signal for automatic title changes — the mobile list silently goes stale for them. Because workspace.title is not mutated for multi-panel panels, even the $title fallback does not rescue the case. Sources/Mobile/MobileWorkspaceListObserver.swift — specifically the refreshPerWorkspaceSubscriptions guard that skips re-subscription for already-tracked workspaces when the setting changes. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[ghosttyDidSetTitle notification] --> B[TabManager.updatePanelTitle]
B --> C{Read\nfanout setting}
C -->|enabled = true| D[tab.updatePanelTitle\nupdatesWorkspaceTitle: true]
C -->|enabled = false| E[tab.updatePanelTitle\nupdatesWorkspaceTitle: false]
D --> F[panelTitles updated]
E --> F
F --> G{Single panel\n& no customTitle?}
G -->|enabled = true| H[workspace.title updated\n→ $title fires]
G -->|enabled = false\nor multi-panel| I[workspace.title unchanged]
H --> J[MobileWorkspaceListObserver\nemitIfNeeded]
I --> K{$panelTitles\nsubscribed?}
K -->|Subscription built while\nenabled=true| L[emitIfNeeded triggered]
K -->|Subscription built while\nenabled=false| M[⚠️ No wakeup for\nmulti-panel workspaces]
L --> J
J --> N{summaryHash\nchanged?}
N -->|yes| O[emit workspace.updated\nto mobile clients]
N -->|no| P[suppressed by hash diff]
style M fill:#ffcccc,stroke:#cc0000
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[ghosttyDidSetTitle notification] --> B[TabManager.updatePanelTitle]
B --> C{Read\nfanout setting}
C -->|enabled = true| D[tab.updatePanelTitle\nupdatesWorkspaceTitle: true]
C -->|enabled = false| E[tab.updatePanelTitle\nupdatesWorkspaceTitle: false]
D --> F[panelTitles updated]
E --> F
F --> G{Single panel\n& no customTitle?}
G -->|enabled = true| H[workspace.title updated\n→ $title fires]
G -->|enabled = false\nor multi-panel| I[workspace.title unchanged]
H --> J[MobileWorkspaceListObserver\nemitIfNeeded]
I --> K{$panelTitles\nsubscribed?}
K -->|Subscription built while\nenabled=true| L[emitIfNeeded triggered]
K -->|Subscription built while\nenabled=false| M[⚠️ No wakeup for\nmulti-panel workspaces]
L --> J
J --> N{summaryHash\nchanged?}
N -->|yes| O[emit workspace.updated\nto mobile clients]
N -->|no| P[suppressed by hash diff]
style M fill:#ffcccc,stroke:#cc0000
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| import CmuxSettings | ||
|
|
||
| enum PanelTitleWorkspaceListFanoutSettings { | ||
| static func isEnabled(settings: any SettingsReading) -> Bool { | ||
| settings.value(for: SettingCatalog().terminal.titleUpdateWorkspaceListFanoutEnabled) | ||
| } | ||
| } |
There was a problem hiding this comment.
SettingCatalog() constructed on every title-update call
isEnabled allocates a fresh SettingCatalog() on every invocation. This is called in TabManager.updatePanelTitle and TabManager.updateWorkspaceTitle — both of which fire on every incoming Ghostty ghosttyDidSetTitle notification, which can arrive many times per second during heavy shell activity. While individual DefaultsKey allocations are cheap, constructing the full catalog just to touch one key on a hot notification path is unnecessary overhead. Storing the key directly (e.g. SettingCatalog().terminal.titleUpdateWorkspaceListFanoutEnabled captured once at call-site construction time, or caching the resolved Bool per observer) would avoid the repeated allocation.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| @discardableResult | ||
| private func waitForTitleFanoutCondition( | ||
| timeout: TimeInterval = 3.0, | ||
| pollInterval: TimeInterval = 0.05, | ||
| file: StaticString = #filePath, | ||
| line: UInt = #line, | ||
| _ condition: @escaping () -> Bool | ||
| ) -> Bool { | ||
| if condition() { | ||
| return true | ||
| } | ||
|
|
||
| let expectation = XCTestExpectation(description: "wait for title fanout condition") | ||
| let deadline = Date().addingTimeInterval(timeout) | ||
|
|
||
| func poll() { | ||
| if condition() { | ||
| expectation.fulfill() | ||
| return | ||
| } | ||
| guard Date() < deadline else { return } | ||
| DispatchQueue.main.asyncAfter(deadline: .now() + pollInterval) { | ||
| poll() | ||
| } | ||
| } | ||
|
|
||
| DispatchQueue.main.async { | ||
| poll() | ||
| } | ||
|
|
||
| let result = XCTWaiter().wait(for: [expectation], timeout: timeout + pollInterval + 0.1) | ||
| if result != .completed { | ||
| XCTFail("Timed out waiting for condition", file: file, line: line) | ||
| return false | ||
| } | ||
| return true | ||
| } |
There was a problem hiding this comment.
asyncAfter on @MainActor test
waitForTitleFanoutCondition relies on DispatchQueue.main.asyncAfter scheduling follow-up polls while the calling test (marked @MainActor) blocks on XCTWaiter().wait(...). This works today because XCTWaiter spins the run loop while waiting, draining queued main-thread work. That run-loop-spinning behavior is an implementation detail of the XCTest runtime and is not guaranteed under Swift Testing or if the test runner changes its wait strategy. An AsyncStream/continuation-based approach driven by the same @Published sources under test would give a real-signal-based wait without relying on run-loop spin, and would integrate naturally with Swift concurrency's structured async/await test style already used in MobileWorkspaceListFidelityTests.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/TabManagerTitleWorkspaceListFanoutTests.swift`:
- Around line 48-83: Migrate the test file from XCTest framework to Swift
Testing for consistency with the sibling test file in this PR. Convert the
TabManagerTitleWorkspaceListFanoutTests class to a struct decorated with `@Suite`,
change the testTitleWorkspaceListFanoutCanBeDisabled method to be decorated with
`@Test`, replace XCTUnwrap calls with `#require`, and replace all XCTAssertTrue,
XCTAssertEqual, and XCTAssertNil assertions with `#expect` calls. Update the
imports to include the Testing framework and ensure the `@MainActor` decorator is
retained on the struct.
In `@Sources/Mobile/MobileWorkspaceListObserver.swift`:
- Around line 30-48: Convert the computed property
includesAutomaticPanelTitlesInWorkspaceList from a computed property that reads
the setting on every access to a stored property that captures the value once at
initialization time. In the init method, assign the result of
PanelTitleWorkspaceListFanoutSettings.isEnabled(settings: settings) to this
property so the setting value is captured once when the observer is created
rather than being re-read on every access.
🪄 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: d194dded-6529-46df-8bd0-ab4041828266
📒 Files selected for processing (9)
Packages/macOS/CmuxSettings/Sources/CmuxSettings/Keys/TerminalCatalogSection.swiftPackages/macOS/CmuxSettings/Tests/CmuxSettingsTests/UserDefaultsSettingsClientTests.swiftSources/Mobile/MobileWorkspaceListObserver.swiftSources/PanelTitleWorkspaceListFanoutSettings.swiftSources/TabManager.swiftSources/Workspace.swiftcmux.xcodeproj/project.pbxprojcmuxTests/MobileWorkspaceListFidelityTests.swiftcmuxTests/TabManagerTitleWorkspaceListFanoutTests.swift
| @MainActor | ||
| final class TabManagerTitleWorkspaceListFanoutTests: XCTestCase { | ||
| func testTitleWorkspaceListFanoutCanBeDisabled() throws { | ||
| let suiteName = "cmux-title-workspace-list-fanout-\(UUID().uuidString)" | ||
| let defaults = try XCTUnwrap(UserDefaults(suiteName: suiteName)) | ||
| defaults.removePersistentDomain(forName: suiteName) | ||
| defer { defaults.removePersistentDomain(forName: suiteName) } | ||
|
|
||
| let settings = UserDefaultsSettingsClient(defaults: defaults) | ||
| settings.set(false, for: SettingCatalog().terminal.titleUpdateWorkspaceListFanoutEnabled) | ||
|
|
||
| let manager = TabManager(settings: settings) | ||
| let workspace = try XCTUnwrap(manager.selectedWorkspace) | ||
| let panelId = try XCTUnwrap(workspace.focusedPanelId) | ||
| let originalTitle = workspace.title | ||
|
|
||
| NotificationCenter.default.post( | ||
| name: .ghosttyDidSetTitle, | ||
| object: nil, | ||
| userInfo: [ | ||
| GhosttyNotificationKey.tabId: workspace.id, | ||
| GhosttyNotificationKey.surfaceId: panelId, | ||
| GhosttyNotificationKey.title: "Runtime title" | ||
| ] | ||
| ) | ||
|
|
||
| XCTAssertTrue( | ||
| waitForTitleFanoutCondition(timeout: 1.0) { | ||
| workspace.panelTitles[panelId] == "Runtime title" && | ||
| workspace.sessionSnapshot(includeScrollback: false).processTitle == "Runtime title" | ||
| } | ||
| ) | ||
| XCTAssertEqual(workspace.title, originalTitle) | ||
| XCTAssertNil(workspace.customTitle) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Consider using Swift Testing for consistency.
The test logic is correct and properly validates the fanout disable behavior. However, the sibling test file MobileWorkspaceListFidelityTests.swift (also part of this PR) uses Swift Testing (@Test, #expect), while this test uses XCTest.
For consistency and modern testing practices, consider migrating this test to Swift Testing:
import Testing
import CmuxSettings
`#if` canImport(cmux_DEV)
`@testable` import cmux_DEV
`#elseif` canImport(cmux)
`@testable` import cmux
`#endif`
`@MainActor`
`@Suite`(.serialized)
struct TabManagerTitleWorkspaceListFanoutTests {
`@Test` func titleWorkspaceListFanoutCanBeDisabled() throws {
let suiteName = "cmux-title-workspace-list-fanout-\(UUID().uuidString)"
let defaults = try `#require`(UserDefaults(suiteName: suiteName))
defaults.removePersistentDomain(forName: suiteName)
defer { defaults.removePersistentDomain(forName: suiteName) }
let settings = UserDefaultsSettingsClient(defaults: defaults)
settings.set(false, for: SettingCatalog().terminal.titleUpdateWorkspaceListFanoutEnabled)
let manager = TabManager(settings: settings)
let workspace = try `#require`(manager.selectedWorkspace)
let panelId = try `#require`(workspace.focusedPanelId)
let originalTitle = workspace.title
NotificationCenter.default.post(
name: .ghosttyDidSetTitle,
object: nil,
userInfo: [
GhosttyNotificationKey.tabId: workspace.id,
GhosttyNotificationKey.surfaceId: panelId,
GhosttyNotificationKey.title: "Runtime title"
]
)
`#expect`(
waitForTitleFanoutCondition(timeout: 1.0) {
workspace.panelTitles[panelId] == "Runtime title" &&
workspace.sessionSnapshot(includeScrollback: false).processTitle == "Runtime title"
}
)
`#expect`(workspace.title == originalTitle)
`#expect`(workspace.customTitle == nil)
}
}Note: The waitForTitleFanoutCondition helper would need adjustment to return Bool and use #expect for the timeout failure case, or you could use Swift Testing's built-in async test support if the underlying notification/update mechanism can be awaited.
🧰 Tools
🪛 SwiftLint (0.63.3)
[Warning] 49-49: Classes should have an explicit deinit method
(required_deinit)
🤖 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/TabManagerTitleWorkspaceListFanoutTests.swift` around lines 48 -
83, Migrate the test file from XCTest framework to Swift Testing for consistency
with the sibling test file in this PR. Convert the
TabManagerTitleWorkspaceListFanoutTests class to a struct decorated with `@Suite`,
change the testTitleWorkspaceListFanoutCanBeDisabled method to be decorated with
`@Test`, replace XCTUnwrap calls with `#require`, and replace all XCTAssertTrue,
XCTAssertEqual, and XCTAssertNil assertions with `#expect` calls. Update the
imports to include the Testing framework and ensure the `@MainActor` decorator is
retained on the struct.
| private let settings: any SettingsReading | ||
| /// Throttle window with `latest: true`. First event in a burst emits | ||
| /// immediately (iPhone gets the change in milliseconds), subsequent | ||
| /// events within the window collapse to one trailing emit carrying the | ||
| /// final state. So a single action is instant; a burst caps at ~1 emit | ||
| /// per 80 ms. Hash-diff suppresses no-op rebroadcasts. | ||
| private let throttleMilliseconds: Int = 80 | ||
| private var includesAutomaticPanelTitlesInWorkspaceList: Bool { | ||
| PanelTitleWorkspaceListFanoutSettings.isEnabled(settings: settings) | ||
| } | ||
|
|
||
| init(tabManager: TabManager, notificationStore: TerminalNotificationStore? = nil) { | ||
| init( | ||
| tabManager: TabManager, | ||
| notificationStore: TerminalNotificationStore? = nil, | ||
| settings: any SettingsReading = UserDefaultsSettingsClient(defaults: .standard) | ||
| ) { | ||
| self.tabManager = tabManager | ||
| self.notificationStore = notificationStore | ||
| self.settings = settings |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider capturing the setting value at initialization time.
The computed property includesAutomaticPanelTitlesInWorkspaceList reads the setting on every access. If the setting changes at runtime while the workspace set remains stable, refreshPerWorkspaceSubscriptions won't re-run, so:
- Changing from true→false leaves the observer subscribed to
$panelTitles(unnecessary but harmless). - Changing from false→true means the observer won't receive
$panelTitlesupdates (mobile client misses automatic title changes).
If this setting is intended as launch-time configuration (not a runtime toggle), consider capturing the value once at initialization:
private let includesAutomaticPanelTitlesInWorkspaceList: Bool
init(...) {
...
self.includesAutomaticPanelTitlesInWorkspaceList =
PanelTitleWorkspaceListFanoutSettings.isEnabled(settings: settings)
...
}If runtime toggles are intended, subscribe to setting changes and call refreshPerWorkspaceSubscriptions when it changes.
🤖 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/Mobile/MobileWorkspaceListObserver.swift` around lines 30 - 48,
Convert the computed property includesAutomaticPanelTitlesInWorkspaceList from a
computed property that reads the setting on every access to a stored property
that captures the value once at initialization time. In the init method, assign
the result of PanelTitleWorkspaceListFanoutSettings.isEnabled(settings:
settings) to this property so the setting value is captured once when the
observer is created rather than being re-read on every access.
1 participant
Summary
terminal.titleUpdates.workspaceListFanout.enabledsetting so existing title behavior stays unchanged.Workspace.title.panelTitleschanges and exclude automatic panel titles from its hash while still tracking manual terminal renames.Root cause
Automatic terminal title updates can fan out through the workspace-list path: a runtime title update mutates
Workspace.panelTitles, the focused/single-panel path mutatesWorkspace.title, and the mobile workspace-list observer hashes/subscribes to those automatic title changes.This PR targets that fanout path directly. The setting defaults to
true, so default behavior is unchanged. Users who set it tofalsetrade live automatic runtime-title freshness in the workspace/mobile list for lower title-driven list churn.Relationship to other PRs
This PR is now independent from #6552 and is based on
main.Behavior
With
terminal.titleUpdates.workspaceListFanout.enabled = false:Workspace.panelTitlesstill records the automatic terminal runtime title.Workspace.titleremains stable for automatic runtime-title churn.Validation
git diff --checkpython3 scripts/swift_file_length_budget.py --repo-root . --budget .github/swift-file-length-budget.tsvDEVELOPER_DIR=/Applications/Xcode-26.5.0.app/Contents/Developer swift test --package-path Packages/macOS/CmuxSettings --filter UserDefaultsSettingsClientTestsDEVELOPER_DIR=/Applications/Xcode-26.5.0.app/Contents/Developer CMUX_ZIG=/opt/homebrew/opt/zig@0.15/bin/zig PATH="/opt/homebrew/opt/zig@0.15/bin:$PATH" xcodebuild -project cmux.xcodeproj -scheme cmux-unit -configuration Debug -destination 'platform=macOS' -only-testing:cmuxTests/TabManagerTitleWorkspaceListFanoutTests -only-testing:cmuxTests/MobileWorkspaceListFidelityTests testSummary by CodeRabbit
New Features
Tests