[codex] Refactor chat parser and transcript service ownership - #6216
Conversation
|
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:
📝 WalkthroughWalkthroughThis PR replaces shared agent chat transcript service access with injected wiring, adds typed Ghostty title-change helpers used by notification producers and consumers, and converts ChangesAgent chat transcript injection and typed Ghostty title changes
OSC133 parser value-type conversion
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 1 warning, 1 inconclusive)
✅ Passed checks (17 passed)
✨ 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Greptile SummaryThis PR refactors
Confidence Score: 4/5Safe to merge pending resolution of the open review threads on TerminalController.swift and ContentView.swift that were flagged prior to this round and remain unaddressed in the diff. The DI wiring and value-type conversion are mechanically correct and the new localized error key covers both supported locales. Two issues from earlier review rounds are still present in the changed code: Sources/TerminalController.swift and Sources/ContentView.swift both have open findings from prior review rounds that are still visible in the diff and have not been addressed by this PR. Important Files Changed
Reviews (6): 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}
| guard let service = agentChatTranscriptService else { | ||
| return .err(code: "unavailable", message: "Agent chat transcript service is not configured", data: nil) | ||
| } | ||
| var page = await service.history(sessionID: sessionID, beforeSeq: beforeSeq, limit: limit) |
There was a problem hiding this comment.
"Agent chat transcript service is not configured" message is sent verbatim to the iOS mobile client as an API error body. The existing not_found error in this same function routes through String(localized:defaultValue:); these new guards skip localization entirely and expose an internal component name. The custom rules require that API error bodies not contain implementation details and that every user-visible string use a localized API. If the iOS app surfaces this message in the chat error banner, it violates both constraints. Replace with a cmux-terms description and a localized key.
| guard let service = agentChatTranscriptService else { | |
| return .err(code: "unavailable", message: "Agent chat transcript service is not configured", data: nil) | |
| } | |
| var page = await service.history(sessionID: sessionID, beforeSeq: beforeSeq, limit: limit) | |
| guard let service = agentChatTranscriptService else { | |
| return .err(code: "unavailable", message: String( | |
| localized: "mobile.chat.error.serviceUnavailable", | |
| defaultValue: "Chat is not available right now. Please try again shortly." | |
| ), data: nil) | |
| } | |
| var page = await service.history(sessionID: sessionID, beforeSeq: beforeSeq, limit: limit) |
| guard let service = agentChatTranscriptService else { | ||
| return .err(code: "unavailable", message: "Agent chat transcript service is not configured", data: nil) | ||
| } | ||
| // Register coding agents cmux detects by terminal title but that never |
There was a problem hiding this comment.
v2MobileChatSessions: the hardcoded "Agent chat transcript service is not configured" message is returned to the mobile client, exposes an internal class name, and is not localized. The mobile.chat.sessions RPC result is used by the iOS app to render the chat session list.
| guard let service = agentChatTranscriptService else { | |
| return .err(code: "unavailable", message: "Agent chat transcript service is not configured", data: nil) | |
| } | |
| // Register coding agents cmux detects by terminal title but that never | |
| guard let service = agentChatTranscriptService else { | |
| return .err(code: "unavailable", message: String( | |
| localized: "mobile.chat.error.serviceUnavailable", | |
| defaultValue: "Chat is not available right now. Please try again shortly." | |
| ), data: nil) | |
| } | |
| // Register coding agents cmux detects by terminal title but that never |
| /// listener starts. Socket auth commands read these on the main actor. | ||
| @MainActor private(set) var authCoordinator: AuthCoordinator? | ||
| @MainActor private(set) var browserSignInFlow: HostBrowserSignInFlow? | ||
| @MainActor var agentChatTranscriptService: AgentChatTranscriptService? |
There was a problem hiding this comment.
agentChatTranscriptService is exposed as a bare @MainActor var, making it settable (including to nil) by any @MainActor caller. The sibling authCoordinator / browserSignInFlow properties use private(set) and a dedicated attachAuth method to enforce single-writer semantics. Without that guard, accidental reassignment or nilification would silently break all mobile chat without an obvious crash.
| @MainActor var agentChatTranscriptService: AgentChatTranscriptService? | |
| @MainActor private(set) var agentChatTranscriptService: AgentChatTranscriptService? |
![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 `@Sources/Mobile/AgentChat/AgentChatTranscriptService.swift`:
- Around line 56-58: The adoptDetectedAgentSessions callback expects a workspace
ID based on its parameter naming and downstream handler
TerminalController.adoptDetectedAgentSessions(workspaceID:), but at line 81 a
tabId.uuidString is being passed instead. This causes a mismatch where the
surface ID won't correlate with any workspace UUID. Fix this by either: (1)
extracting the actual workspace ID from the title-change notification before
invoking adoptDetectedAgentSessions callback and passing that workspace ID
instead of tabId.uuidString, or (2) renaming the callback parameter from its
current name to reflect surface-ID semantics throughout the chain (the start
method signature, the callback parameter, the
TerminalController.adoptDetectedAgentSessions method parameter, and how it is
used in mobileResolveWorkspaceAndSurface) and ensure proper conversion happens
at the composition root in AppDelegate.swift:1934.
In `@Sources/TerminalController`+MobileChat.swift:
- Around line 46-49: Extract the repeated error message "Agent chat transcript
service is not configured" into a static computed property in
TerminalController+MobileChat.swift following the pattern of
chatTerminalBindingErrorMessage, and ensure it returns a localized string.
Update all three occurrences of this error message in the guard statements that
check for agentChatTranscriptService (at the anchor location in lines 46-49, and
at the sibling locations in lines 56-58 and lines 129-131) to use this new
static property instead of the hardcoded string. Then add the localization key
mobile.chat.error.serviceNotConfigured to Resources/Localizable.xcstrings with
English and Japanese translations to complete the localization.
🪄 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: 59ecd03a-254d-4bd8-ac12-11a11bfb28a0
📒 Files selected for processing (6)
Packages/CmuxAgentChat/Sources/CmuxAgentChat/Parsing/OSC133CommandParser.swiftPackages/CmuxAgentChat/Tests/CmuxAgentChatTests/OSC133CommandParserTests.swiftSources/AppDelegate.swiftSources/Mobile/AgentChat/AgentChatTranscriptService.swiftSources/TerminalController+MobileChat.swiftSources/TerminalController.swift
| /// - Parameter adoptDetectedAgentSessions: Composition-root callback that | ||
| /// adopts a title-detected agent for the workspace whose title changed. | ||
| func start(adoptDetectedAgentSessions: @escaping @MainActor (String) -> Void) { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify parameter semantics at callsites and downstream handlers.
set -euo pipefail
echo "== start(adoptDetectedAgentSessions:) callsites =="
rg -n -C3 '\bstart\s*\(\s*adoptDetectedAgentSessions\s*:' Sources/AppDelegate.swift Sources/TerminalController.swift Sources/TerminalController+MobileChat.swift || true
echo
echo "== closure parameter naming/usage around adoptDetectedAgentSessions =="
rg -n -C3 '\badoptDetectedAgentSessions\b' Sources/AppDelegate.swift Sources/TerminalController.swift Sources/TerminalController+MobileChat.swift || true
echo
echo "== workspace/tab resolution APIs likely used by the closure =="
rg -n -C3 '\bworkspace\b|\btabId\b|workspaceID|tabManagerFor\s*\(' Sources/AppDelegate.swift Sources/TerminalController.swift Sources/TerminalController+MobileChat.swift || trueRepository: manaflow-ai/cmux
Length of output: 50373
🏁 Script executed:
sed -n '50,90p' Sources/Mobile/AgentChat/AgentChatTranscriptService.swiftRepository: manaflow-ai/cmux
Length of output: 1930
Fix tab ID vs workspace ID parameter mismatch in agent adoption callback.
Line 81 passes tabId.uuidString to the adoptDetectedAgentSessions callback, but the method signature (line 56–57) and downstream handler (TerminalController.adoptDetectedAgentSessions(workspaceID:) at AppDelegate.swift:1934) expect a workspace ID. When the callback invokes mobileResolveWorkspaceAndSurface with ["workspace_id": tabId], the surface ID will not match any workspace UUID, causing title-detected agent adoption to fail silently. Either extract the workspace ID from the title-change notification (if available) before invoking the callback, or rename the callback parameter and all downstream handlers to reflect surface-ID semantics and convert appropriately at the composition root.
🤖 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/AgentChat/AgentChatTranscriptService.swift` around lines 56 -
58, The adoptDetectedAgentSessions callback expects a workspace ID based on its
parameter naming and downstream handler
TerminalController.adoptDetectedAgentSessions(workspaceID:), but at line 81 a
tabId.uuidString is being passed instead. This causes a mismatch where the
surface ID won't correlate with any workspace UUID. Fix this by either: (1)
extracting the actual workspace ID from the title-change notification before
invoking adoptDetectedAgentSessions callback and passing that workspace ID
instead of tabId.uuidString, or (2) renaming the callback parameter from its
current name to reflect surface-ID semantics throughout the chain (the start
method signature, the callback parameter, the
TerminalController.adoptDetectedAgentSessions method parameter, and how it is
used in mobileResolveWorkspaceAndSurface) and ensure proper conversion happens
at the composition root in AppDelegate.swift:1934.
| view = AnyView(view.onReceive(NotificationCenter.default.publisher(for: .ghosttyDidSetTitle)) { notification in | ||
| guard let tabId = notification.userInfo?[GhosttyNotificationKey.tabId] as? UUID, | ||
| tabId == tabManager.selectedTabId else { return } | ||
| guard GhosttyTitleChange(notification: notification)?.tabId == tabManager.selectedTabId else { return } | ||
| scheduleTitlebarTextRefresh() | ||
| }) |
There was a problem hiding this comment.
nil == nil case that the original code did not have. tabManager.selectedTabId is UUID?, so when the failable GhosttyTitleChange.init returns nil (malformed notification) and selectedTabId is also nil (no tab selected), the guard passes and scheduleTitlebarTextRefresh() is called incorrectly. The original code used a non-optional tabId: UUID on the left-hand side, making UUID == nil always false. Use a guard let to preserve the original semantics.
| view = AnyView(view.onReceive(NotificationCenter.default.publisher(for: .ghosttyDidSetTitle)) { notification in | |
| guard let tabId = notification.userInfo?[GhosttyNotificationKey.tabId] as? UUID, | |
| tabId == tabManager.selectedTabId else { return } | |
| guard GhosttyTitleChange(notification: notification)?.tabId == tabManager.selectedTabId else { return } | |
| scheduleTitlebarTextRefresh() | |
| }) | |
| view = AnyView(view.onReceive(NotificationCenter.default.publisher(for: .ghosttyDidSetTitle)) { notification in | |
| guard let change = GhosttyTitleChange(notification: notification), | |
| change.tabId == tabManager.selectedTabId else { return } | |
| scheduleTitlebarTextRefresh() | |
| }) |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Sources/Mobile/AgentChat/AgentChatTranscriptService.swift (1)
52-69:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix callback identifier contract mismatch (workspace ID vs tab/surface ID).
At Line 68,
change.tabId.uuidStringis passed into a callback documented and wired as workspace-scoped (adoptDetectedAgentSessions(workspaceID:)in AppDelegate/TerminalController). This can make title-triggered adoption no-op for the intended workspace. Align the contract end-to-end: either pass an actual workspace ID here, or rename/retype the callback as tab/surface-scoped and resolve workspace in the downstream handler before adoption.🤖 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/AgentChat/AgentChatTranscriptService.swift` around lines 52 - 69, The observeAgentTitleChanges function is passing change.tabId.uuidString to the adoptDetectedAgentSessions callback, but the callback is contract-defined to accept a workspace ID, not a tab/surface ID. This mismatch causes title-triggered adoption to fail to work correctly for the intended workspace. Fix this by either: (1) determining the actual workspace ID from the tab ID within observeAgentTitleChanges and passing that workspace ID instead of the tab ID, or (2) if changing the callback contract is preferred, rename the callback parameter and update the downstream handler (adoptDetectedAgentSessions in AppDelegate/TerminalController) to accept a tab/surface ID and resolve the workspace ID there before performing the adoption.
🤖 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 `@cmux.xcodeproj/project.pbxproj`:
- Around line 459-460: The object IDs for GhosttyTitleChange.swift
(A11EAE000000000000000000 and A11EAE000000000000000001) and
GhosttyTitleChangeSubscription.swift (A11EAF000000000000000000 and
A11EAF000000000000000001) collide with IDs already used elsewhere in the
.pbxproj file, which can corrupt project wiring. Regenerate two pairs of fresh,
globally unique IDs to replace these colliding identifiers, then update every
reference to them throughout the project file: the two PBXBuildFile entries for
these files, their corresponding PBXFileReference entries, their references in
the Sources group children, and their entries in the PBXSourcesBuildPhase build
phase. Ensure all four IDs (two new PBXBuildFile IDs and two new
PBXFileReference IDs) are used consistently and uniquely across the entire
project structure at all affected sites.
---
Duplicate comments:
In `@Sources/Mobile/AgentChat/AgentChatTranscriptService.swift`:
- Around line 52-69: The observeAgentTitleChanges function is passing
change.tabId.uuidString to the adoptDetectedAgentSessions callback, but the
callback is contract-defined to accept a workspace ID, not a tab/surface ID.
This mismatch causes title-triggered adoption to fail to work correctly for the
intended workspace. Fix this by either: (1) determining the actual workspace ID
from the tab ID within observeAgentTitleChanges and passing that workspace ID
instead of the tab ID, or (2) if changing the callback contract is preferred,
rename the callback parameter and update the downstream handler
(adoptDetectedAgentSessions in AppDelegate/TerminalController) to accept a
tab/surface ID and resolve the workspace ID there before performing the
adoption.
🪄 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: 1dd59aff-3c7a-44e9-a8c8-3e44cf398e24
📒 Files selected for processing (7)
Sources/ContentView.swiftSources/GhosttyTerminalView.swiftSources/GhosttyTitleChange.swiftSources/GhosttyTitleChangeSubscription.swiftSources/Mobile/AgentChat/AgentChatTranscriptService.swiftSources/TabManager.swiftcmux.xcodeproj/project.pbxproj
This branch was successfully deployed
1 participant
Summary
OSC133CommandParserfrom a reference type to a value type with explicit mutating parse state.AgentChatTranscriptService.sharedand make the app composition root own/inject the transcript service intoTerminalController.Validation
swift test --package-path Packages/CmuxAgentChat --filter OSC133CommandParserTests./ios/scripts/reload.sh --tag tgchat --no-launchNotes
Mac
./scripts/reload.sh --tag refactor-chat-injectionis currently blocked by an existingCmuxTerminal/Ghostty C API mismatch:ghostty_surface_set_renderer_realizedis missing from scope inTerminalSurface+Renderer.swift.Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Refactored the OSC133 chat parser to a value type and moved agent‑chat transcript service ownership to the app root via dependency injection. Typed Ghostty title‑change notifications and localized the “chat service unavailable” error; mobile chat routes now guard cleanly when the service is unset.
Refactors
OSC133CommandParseris now astruct;consume(_:)is mutating. Tests usevar.AgentChatTranscriptService.shared.AppDelegateholds one instance, injects it intoTerminalController, and starts it with an adoption callback; the service seeds once and holds a strongGhosttyTitleChangeSubscription.TerminalControllerexposes an optionalagentChatTranscriptService; mobile/chat routes and event ingestion guard and returnunavailableif unset. The error is localized viamobile.chat.error.serviceUnavailable(EN/JA)..ghosttyDidSetTitle: addedGhosttyTitleChangeandGhosttyTitleChangeSubscription; posting/handling updated inGhosttyTerminalView,ContentView, andTabManager.Migration
AgentChatTranscriptService.sharedusage with the injected service (TerminalController.agentChatTranscriptService).AppDelegatesetsTerminalController.shared.agentChatTranscriptServiceand callsstart { TerminalController.shared.adoptDetectedAgentSessions(workspaceID: $0) }before mobile/chat sockets are reachable.Written for commit eb84e0e. Summary will update on new commits.
Summary by CodeRabbit
Release Notes