Repository navigation
perf: inline codex monitor tailing and sidebar batching - #4128
Closed
austinywang wants to merge 34 commits into
Closed
austinywang wants to merge 34 commits into
austinywang wants to merge 34 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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:
📝 WalkthroughWalkthroughInlines Codex transcript monitoring via V2 socket RPCs, replaces per-chunk sidebar log appends with a ring-buffered debounced pipeline, consolidates readiness observers into a TabManager-scoped system, and throttles agent-driven sidebar updates. ChangesPerformance optimization for high-frequency sidebar updates and process overhead
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (3 errors, 1 warning)
✅ Passed checks (11 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 |
Contributor
Greptile SummaryThis PR inlines Codex transcript monitoring from a CLI sidecar into the app runtime via new V2
Confidence Score: 4/5Safe to merge after adding catalog translations for the two notification strings; all blocking / actor-isolation / completion-event concerns from previous rounds are resolved. The previous blocking concerns (Task.sleep debounce, MainActor.assumeIsolated in timer and sink callbacks, monitor lifetime, completion event clearing the bell badge, raw upstream error text in notification bodies, and missing locale coverage for the four new error-body strings) are all addressed in the current revision. One localization gap remains: Sources/TerminalController.swift — the Important Files Changed
Sequence DiagramsequenceDiagram
participant Agent as Codex Agent (CLI)
participant Socket as V2 Socket RPC
participant TC as TerminalController (@MainActor)
participant Reg as CodexTranscriptMonitorRegistry (utility queue)
participant Mon as CodexTranscriptMonitorSession (utility queue)
participant FS as JSONL Transcript File
participant WS as Workspace (@MainActor)
Agent->>Socket: agent.codex_transcript_monitor.start
Socket->>TC: v2CodexTranscriptMonitorStart(params)
TC->>Reg: start(request)
Reg->>Mon: init + start()
Mon->>FS: open O_EVTONLY (DispatchSourceFileSystemObject)
Mon->>FS: readInitialTail (last 512 KB)
FS-->>Mon: .write / .extend event
Mon->>Mon: readIncremental → processLine
Mon->>TC: "DispatchQueue.main.async { assumeIsolated { handleEvent } }"
TC->>WS: "tab.statusEntries[codex] = SidebarStatusEntry"
TC->>TC: notificationStore.addNotification
Agent->>Socket: agent.codex_transcript_monitor.stop
Socket->>TC: v2CodexTranscriptMonitorStop(params)
TC->>Reg: stop(sessionId:turnId:)
Reg->>Mon: cancel()
Mon->>FS: source.cancel() / close(fd)
Note over TC,WS: Workspace teardown path
TC->>Reg: stopWorkspace(workspaceId)
Reg->>Mon: cancel() for all workspace sessions
Reviews (24): Last reviewed commit: "fix: cancel readiness waiters from deini..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@CLI/cmux.swift`:
- Around line 18012-18084: requestCodexTranscriptMonitorStart currently trims
sessionId/workspaceId only for the emptiness check but stores the raw values
into params, causing a mismatch with requestCodexTranscriptMonitorStop which
uses a trimmed normalizedSessionId; fix by trimming both sessionId and
workspaceId up front (e.g. create normalizedSessionId and normalizedWorkspaceId
via trimming like requestCodexTranscriptMonitorStop), use those normalized
values when building params["session_id"] and params["workspace_id"], and keep
the existing guard and telemetry logic otherwise so the start/stop RPC payloads
are symmetric.
In `@Sources/TerminalController.swift`:
- Around line 23-709: Extract the entire Codex transcript monitor subsystem
(types CodexTranscriptMonitorRequest, CodexTranscriptFailureSummary,
CodexTranscriptMonitorEvent and the CodexTranscriptMonitorRegistry class
including its nested Monitor and all helper/static methods) into a new file
named Sources/CodexTranscriptMonitor.swift, preserving their access level and
Sendable annotations and any required imports; in the original
TerminalController.swift keep only the V2 endpoint handlers, the
stopCodexTranscriptMonitors(forWorkspaceId:) function, and
handleCodexTranscriptMonitorEvent(_:) so callers still route events into
TerminalController.shared; ensure the new file compiles by keeping the onEvent
closure signature and the Task { `@MainActor` in
TerminalController.shared.handleCodexTranscriptMonitorEvent(event) } call site
unchanged, and run a quick build to fix any visibility/import issues (make types
internal/public as needed).
- Around line 174-191: The deadline timer in armDeadline() calls finish() with
the default publishCompletion: false, which leaves the "Codex needs input"
sidebar entry set by handleCodexTranscriptMonitorEvent
(tab.statusEntries["codex"]) stuck when no completion event arrives; change the
timer event handler to call finish(publishCompletion: true) so
onEvent(.completion(request: request)) is published and the existing .completion
handling clears the bell.fill entry (alternatively, if you prefer direct
handling here, explicitly clear the codex status entry via the same mechanism
used in the .completion branch), ensuring you reference armDeadline(),
finish(publishCompletion:), and the .completion event path in
handleCodexTranscriptMonitorEvent.
🪄 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: 16b684b9-36fe-4b79-a20b-d207ed245df3
📒 Files selected for processing (6)
CLI/cmux.swiftSources/BackgroundWorkspacePrimeCoordinator.swiftSources/ContentView.swiftSources/TabManager.swiftSources/TerminalController.swiftSources/Workspace.swift
Contributor
Author
|
Risk/rationale note after the review pass:
|
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
austinywang
dismissed
coderabbitai[bot]’s stale review
May 13, 2026 22:46
Stale bot review: all three actionable threads were fixed in a11ce49, replied to, and resolved; CodeRabbit check is/was passing after fixes.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/CodexTranscriptMonitor.swift`:
- Around line 437-439: The computed callId currently trims rawCallId but treats
whitespace-only strings as non-nil empty strings, defeating deduplication;
replace the trim logic to use the existing normalizedValue helper (or call
Self.normalizedValue(rawCallId)) so that whitespace-only/empty inputs become nil
and the fallback "\(payloadTurnId ?? requestTurnId ?? "session"):\(question ??
"request_user_input")" is used; update the assignment that defines callId
(references: rawCallId, callId, Self.firstString(in:keys:), normalizedValue,
payloadTurnId, requestTurnId, question) to call normalizedValue on rawCallId
before applying the nil-coalescing fallback.
- Around line 277-291: The current logic snapshots endOffset via
handle.seekToEnd() and later sets readOffset = endOffset, which can cause
re-reading appended bytes if the file grows between the snapshot and read;
update the logic in CodexTranscriptMonitor's handleFileEvent() (the block using
endOffset, readOffset, handle.seekToEnd(), handle.seek(toOffset:),
handle.readDataToEndOfFile(), and process(data:)) to advance readOffset by the
actual number of bytes read (data.count) after process(data:) instead of
assigning the earlier endOffset; specifically, after reading data =
handle.readDataToEndOfFile() and processing it, set readOffset +=
UInt64(data.count) (and handle the empty-data case safely), and retain the
existing out-of-range reset (if readOffset > endOffset) and pendingData.clear
behavior.
- Around line 81-85: The current publish(_ event: CodexTranscriptMonitorEvent)
uses Task { `@MainActor` in ... } which does not preserve FIFO ordering; replace
the Task call with a main-queue FIFO dispatch and explicit main-actor isolation
so events are delivered in order: use DispatchQueue.main.async {
MainActor.assumeIsolated {
TerminalController.shared.handleCodexTranscriptMonitorEvent(event) } } inside
publish(_:), ensuring
TerminalController.shared.handleCodexTranscriptMonitorEvent(event) is invoked on
the main actor with FIFO semantics; alternatively implement an AsyncStream
consumed by a single Task { `@MainActor` in for await event in stream {
TerminalController.shared.handleCodexTranscriptMonitorEvent(event) } } if you
prefer stream-based serialization.
In `@Sources/Workspace.swift`:
- Around line 6621-6663: Move the SidebarLogRingBuffer implementation out of
Sources/Workspace.swift into its own new Swift file (e.g.,
SidebarLogRingBuffer.swift) at module scope so it’s lifecycle-independent and
unit-testable; copy the struct and related symbols (SidebarLogRingBuffer,
SidebarLogEntry type references, init(limit:), replaceAll(_:limit:), append(_:),
removeAll(), entries()) into the new file, adjust access level from private to
internal/public as appropriate so tests can access it, remove the original
nested/private definition from Workspace.swift, and update any call sites to use
the relocated type; ensure the new file has no dependencies on AppKit/SwiftUI or
singletons so it compiles in isolation.
- Around line 6652-6655: The removeAll() currently only resets head and count,
leaving old SidebarLogEntry objects referenced in storage; update removeAll() to
also clear or release those references (e.g., nil out elements or replace
storage with an empty array) so stored SidebarLogEntry instances are
deallocated, then reset head and count as before; locate the removeAll() method
and the storage backing (storage, SidebarLogEntry) and perform the storage-clear
before resetting head/count.
🪄 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: 82e1bca6-b093-449d-9e87-41bbfbe9fc04
📒 Files selected for processing (5)
CLI/cmux.swiftGhosttyTabs.xcodeproj/project.pbxprojSources/CodexTranscriptMonitor.swiftSources/TerminalController.swiftSources/Workspace.swift
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 `@Sources/Workspace.swift`:
- Around line 9843-9849: The timer's event handler uses MainActor.assumeIsolated
which unsafely asserts actor isolation; change the handler to perform an
explicit MainActor hop using Task { `@MainActor` in ... } so the call to
flushPendingSidebarLogEntries() runs on the main actor safely: inside the
setEventHandler closure (the one capturing [weak self]) unwrap self and call
Task { `@MainActor` in self.flushPendingSidebarLogEntries() } (or Task {
`@MainActor` in await self.flushPendingSidebarLogEntries() } if the method is
async), replacing the MainActor.assumeIsolated block and preserving the weak
self capture.
🪄 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: 5efb541c-8bec-4c9a-9b0f-3286688dad9f
📒 Files selected for processing (4)
GhosttyTabs.xcodeproj/project.pbxprojSources/CodexTranscriptMonitor.swiftSources/SidebarLogRingBuffer.swiftSources/Workspace.swift
…codex-monitors-sidebar
…codex-monitors-sidebar
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 98c3485. Configure here.
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
cmux hooks codex monitorsidecars and lease coordination with in-process transcript JSONL tailing in the app.Driver 1 choice
I split truly user-driven sidebar fields (
isPinned,customColor,terminalScrollBarHidden) from high-frequency agent-driven fields (title,customDescription,latestConversationMessage) and throttle only the agent stream. That preserves immediate visual feedback for user actions while coalescing Codex title churn.Verification
Closes #4127
Note
Medium Risk
Moves Codex monitoring from an external CLI sidecar to in-app JSONL tailing with new V2 socket RPCs, which changes runtime behavior and file I/O/event handling. Also refactors sidebar observation and log buffering, which could affect UI update timing and persistence snapshots if edge cases are missed.
Overview
Inlines Codex transcript monitoring into the app by removing the CLI
codex monitorsidecar/lease system and replacing it with V2 socket calls (agent.codex_transcript_monitor.start/stop) that start/stop an in-process JSONL tailer and publish user-input/failure/completion events to notifications and sidebar status.Reduces sidebar/UI churn by throttling agent-driven sidebar row updates separately from user-driven fields, and by batching sidebar log appends through a per-workspace ring buffer with a short flush throttle (flushed before snapshots and on
list_log).Consolidates background priming observers so multiple readiness waiters share a single set of
TabManager/notification subscriptions, with explicit cleanup/cancellation on coordinator teardown.Reviewed by Cursor Bugbot for commit 0c3cc11. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Inlines Codex transcript monitoring into the app via V2 RPCs with boundary‑aware JSONL tailing, and batches/throttles sidebar updates to cut UI churn and log I/O. Removes the CLI monitor/leases, confines monitor session state to a single queue, adds per‑turn tracking and localized error bodies, and moves sidebar log flushing to the main run loop. Addresses #4127.
New Features
list_log.cmux hooks codex monitorwith an in‑app monitor + registry; V2agent.codex_transcript_monitor.start/stopvalidate workspace/surface, auto‑resolve transcript path, and publish needs‑input/failure/completion to notifications and the sidebar.Bug Fixes
task_started.Written for commit 0c3cc11. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
New Features
Bug Fixes
Performance