Add top snapshots and Task Manager window - #3290
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:
📝 WalkthroughWalkthroughAdds a new Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI Parser
participant RPC as TerminalController (v2)
participant Hier as Hierarchy Builder
participant Snap as CmuxTopProcessSnapshot
participant OS as OS (sysctl / proc APIs)
participant UI as Task Manager UI
CLI->>RPC: invoke v2 system.top(flags, workspace, include_processes)
RPC->>Hier: resolve windows → workspaces → panes → surfaces → webviews
Hier-->>RPC: hierarchical structure (with webview refs)
RPC->>Snap: capture(includeProcessDetails?)
Snap->>OS: sysctl KERN_PROC_ALL / proc_pidinfo
OS-->>Snap: processes, metrics, tty ids
Snap-->>RPC: sample, totals, pid indices, process trees
RPC->>Hier: annotate nodes with resource summaries and optional process trees
RPC-->>CLI: return payload (JSON or text)
UI->>RPC: call taskManagerTopPayload(includeProcesses:)
RPC-->>UI: annotated payload for UI rendering
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 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 |
Greptile SummaryThis PR adds Confidence Score: 4/5Safe to merge; all findings are P2 (missing comment, edge-case surface drop, dead code). No P0 or P1 issues found. Three P2 findings: a missing threading-policy comment required by CLAUDE.md, a silent no-op when a surface lacks a pane mapping, and an unreachable code path in processTreePayload's explicitRootPIDs branch. None affect correctness in the normal case. Sources/TerminalController.swift (threading comment + surface-drop), Sources/CmuxTopSnapshot.swift (dead explicitRootPIDs branch) Important Files Changed
Sequence DiagramsequenceDiagram
participant CLI as cmux top (CLI)
participant Socket as SocketClient
participant TC as TerminalController
participant Main as Main Thread (v2MainSync)
participant Snap as CmuxTopProcessSnapshot
CLI->>Socket: system.top {all_windows, workspace_id, caller}
Socket->>TC: v2SystemTop(params)
TC->>Main: v2MainSync — build window/workspace/pane/surface/tag nodes
Main-->>TC: windowNodes []
TC->>Snap: CmuxTopProcessSnapshot.capture() — sysctl + proc_pidinfo
Snap-->>TC: processSnapshot
TC->>TC: v2TopBrowserPIDOccurrences(windowNodes)
TC->>TC: v2AnnotateTopWindows — attach resources & process trees
TC-->>Socket: {totals, windows[...resources, panes[...surfaces[...webviews]]]}
Socket-->>CLI: payload
CLI->>CLI: renderTopText or JSON print
|
![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: 143f41afd2
ℹ️ 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".
![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
🧹 Nitpick comments (2)
Sources/CmuxTopSnapshot.swift (1)
85-175: Add focused tests for root selection and cycle handling.The
explicitRootPIDs/orphaned/visitedinteraction is subtle enough that a small change here could silently reshape thesystem.toptree. A few unit tests around overlapping roots, missing roots, and parent cycles would make this much safer to evolve.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/CmuxTopSnapshot.swift` around lines 85 - 175, Add unit tests covering processTreePayload and processTreeNode to validate root selection and cycle handling: create tests for (1) overlapping explicitRootPIDs where some roots are ancestors of others, (2) missing/explicit roots that are not in allowedPIDs (ensure orphaned logic filters correctly), and (3) parent cycles (ensure visited prevents infinite recursion and that cycle nodes appear exactly once). Use the public interfaces processTreePayload(for:rootPIDs:) and the behavior of explicitRootPIDs/orphaned/visited to assert tree shape, ordering (processSortKey), presence of tty_device payloads, and that summary(for:) resources aggregate correctly for single-node and multi-node subtrees.Sources/TerminalController.swift (1)
3425-3427: Gate process-tree serialization behind an opt-in flag.
processTreePayload(...)is generated for every surface, webview, and tag on everysystem.topcall. Given this PR already defines an optional--processesmode, skipping those subtrees unless the caller asks for them would cut both traversal cost and response size.Also applies to: 3446-3449, 3466-3468
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TerminalController.swift` around lines 3425 - 3427, The code currently always populates surface["processes"] by calling processSnapshot.processTreePayload(...) for every surface; change this to only generate and assign that subtree when the caller has opted in (the new --processes mode). In TerminalController where system.top responses are built (the block assigning surface["root_pids"], surface["resources"], surface["processes"]), wrap the processSnapshot.processTreePayload(...) call behind a conditional that checks the request/flag (the --processes option) and omit surface["processes"] when not set; apply the same pattern to the other two sites mentioned (near the assignments at the ranges around lines 3446-3449 and 3466-3468) so processTreePayload is only invoked on demand.
🤖 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/CmuxTopSnapshot.swift`:
- Around line 275-285: Replace the current open/fstat sequence that uses
Darwin.open, fd, fstat and Darwin.close with a direct stat call: call stat(path,
&statInfo) (using the existing statInfo variable) and guard its return == 0
before returning Int64(statInfo.st_rdev); remove the Darwin.open/Darwin.close
and fstat usage and keep the same nil-on-failure behavior so TTY device
attribution works for paths that are stat-able but not openable.
In `@Sources/TerminalController.swift`:
- Around line 3255-3280: The current tag entries build a non-unique, unescaped
"ref" like "tag:\(entry.key)" in the loops that populate tags (see
workspace.sidebarStatusEntriesInDisplayOrder() loop and the subsequent
workspace.agentPIDs.keys loop), which can collide across workspaces or break
when keys contain ":" or whitespace; update both places that set "ref" to either
(a) namespace the ref with the workspace id and escape the raw key (e.g. "ref":
"workspace:\(workspace.id.uuidString):tag:\(escapedKey)") where escapedKey is a
safe percent-encoding or other deterministic escape of entry.key/key, or (b)
remove the "ref" field until a resolvable workspace-scoped tag handle exists —
ensure the same approach is applied in both occurrences so refs are unique and
safe.
---
Nitpick comments:
In `@Sources/CmuxTopSnapshot.swift`:
- Around line 85-175: Add unit tests covering processTreePayload and
processTreeNode to validate root selection and cycle handling: create tests for
(1) overlapping explicitRootPIDs where some roots are ancestors of others, (2)
missing/explicit roots that are not in allowedPIDs (ensure orphaned logic
filters correctly), and (3) parent cycles (ensure visited prevents infinite
recursion and that cycle nodes appear exactly once). Use the public interfaces
processTreePayload(for:rootPIDs:) and the behavior of
explicitRootPIDs/orphaned/visited to assert tree shape, ordering
(processSortKey), presence of tty_device payloads, and that summary(for:)
resources aggregate correctly for single-node and multi-node subtrees.
In `@Sources/TerminalController.swift`:
- Around line 3425-3427: The code currently always populates
surface["processes"] by calling processSnapshot.processTreePayload(...) for
every surface; change this to only generate and assign that subtree when the
caller has opted in (the new --processes mode). In TerminalController where
system.top responses are built (the block assigning surface["root_pids"],
surface["resources"], surface["processes"]), wrap the
processSnapshot.processTreePayload(...) call behind a conditional that checks
the request/flag (the --processes option) and omit surface["processes"] when not
set; apply the same pattern to the other two sites mentioned (near the
assignments at the ranges around lines 3446-3449 and 3466-3468) so
processTreePayload is only invoked on demand.
🪄 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: 2795a3ff-6fab-48ca-8270-c2bacc2b1ab4
📒 Files selected for processing (4)
CLI/cmux.swiftGhosttyTabs.xcodeproj/project.pbxprojSources/CmuxTopSnapshot.swiftSources/TerminalController.swift
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 543866385c
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
Sources/TerminalController.swift (1)
3261-3263:⚠️ Potential issue | 🟠 MajorMake tag refs workspace-scoped and escaped.
refis still emitted as rawtag:\(key), so it remains ambiguous across workspaces and can break for keys containing:or whitespace. Either namespace and escape it the same way as the rest of this payload, or omitrefuntil there is a resolvable tag handle.Also applies to: 3281-3282
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TerminalController.swift` around lines 3261 - 3263, The emitted "ref" currently uses raw tag:\(entry.key) which is ambiguous and breaks for keys with ":" or whitespace; change the ref to be workspace-scoped and escaped the same way as the "id" field (which uses "\(workspace.id.uuidString):tag:\(entry.key)"): either set ref to the namespaced-and-escaped form "\(workspace.id.uuidString):tag:\(escapedEntryKey)" using the same escape helper used elsewhere in this payload (or create one if missing), or remove/refactor the ref emission until a resolvable tag handle is available; update the occurrences where ref is set (the lines that build "ref": "tag:\(entry.key)") to use the namespaced escaped value.
🧹 Nitpick comments (1)
Sources/CmuxTopSnapshot.swift (1)
317-329: Add a comment explaining the private WebKit API dependency.The
_webProcessIdentifierselector is a private, undocumented WebKit API. While the implementation safely handles selector absence (returningnil), this dependency is fragile and could break silently in future macOS or WebKit updates. Since cmux is a developer tool that intentionally uses private APIs, add a comment documenting:
- Why this private API is needed (web content PID attribution for debugging/monitoring)
- The fallback behavior (returns
nilif selector is unavailable)- The risk of breakage across OS versions
This helps maintainers understand the tradeoff when future breakage occurs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/CmuxTopSnapshot.swift` around lines 317 - 329, Add a clarifying comment above the CmuxWebContentProcessIdentifier enum/pid(for:) explaining that it relies on the private WebKit selector "_webProcessIdentifier" (a private/undocumented API) to obtain the web content process PID for debugging/monitoring, that the implementation already guards for absence of the selector and will return nil as a safe fallback, and that this is a fragile dependency which may break on future macOS/WebKit updates so maintainers should be aware of the tradeoff and consider alternatives if/when it breaks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLI/cmux.swift`:
- Around line 10243-10260: The live watch loop currently swallows all errors
from buildTopPayload and keeps repainting failures; change the catch to detect
permanent/unsupported failures (e.g., CLIError.method_not_found or whatever
CLIError case represents an unrecoverable system.top failure) and break out of
the while true watch loop and propagate/return a failure instead of continuing;
for non-permanent errors keep the existing behavior (set body and continue).
Update the same logic for the other identical catch block around lines
10376-10380 so both live top loops treat permanent CLIError cases the same.
- Around line 10979-10981: The code appends raw strings like title (from
workspace["title"]) directly into parts which allows ANSI sequences or newlines
to corrupt the terminal; create or use a helper named e.g. sanitizeLabel /
escapeControlCharacters that strips/escapes ANSI control sequences and newlines
(remove characters < 0x20 except permitted whitespace, and strip CSI/OSC
sequences) and call it wherever labels are written (the title variable before
parts.append("\"\(title)\""), and the other places flagged for page titles/URLs,
tag values, and process names in the ranges referenced) so only sanitized text
is concatenated into the text view.
- Around line 10914-10935: The surface-level processes are skipped when webviews
exist because the current branch only calls
appendTopProcessLines(surface["processes"] ...) in the else path; update the
logic in the loop that handles webviews (involving webviews, webviewIndex,
webviewBranch, webviewIndent, topWebViewLabel) to always call
appendTopProcessLines for the surface itself when showProcesses is true — e.g.,
after emitting all webview entries (or before/after the webview loop) invoke
appendTopProcessLines with surface["processes"] as? [[String: Any]] ?? [] and
the appropriate indent (workspaceIndent + paneIndent + surfaceIndent) so both
the surface and per-webview processes are rendered.
In `@Sources/TerminalController.swift`:
- Around line 3023-3024: Currently, invalid present booleans for "all_windows"
and "include_processes" are treated as false; change the handler that reads
these (the assignments using v2Bool(params, "all_windows") and v2Bool(params,
"include_processes")) to reject present-but-malformed values: check if params
contains the key and v2Bool returned nil, and if so return an "invalid_params"
error response instead of falling back to false; keep the existing behavior
where absent keys still default to false. Ensure the error path uses the same
invalid_params error format used elsewhere in TerminalController.swift so
callers see the parameter validation failure.
---
Duplicate comments:
In `@Sources/TerminalController.swift`:
- Around line 3261-3263: The emitted "ref" currently uses raw tag:\(entry.key)
which is ambiguous and breaks for keys with ":" or whitespace; change the ref to
be workspace-scoped and escaped the same way as the "id" field (which uses
"\(workspace.id.uuidString):tag:\(entry.key)"): either set ref to the
namespaced-and-escaped form "\(workspace.id.uuidString):tag:\(escapedEntryKey)"
using the same escape helper used elsewhere in this payload (or create one if
missing), or remove/refactor the ref emission until a resolvable tag handle is
available; update the occurrences where ref is set (the lines that build "ref":
"tag:\(entry.key)") to use the namespaced escaped value.
---
Nitpick comments:
In `@Sources/CmuxTopSnapshot.swift`:
- Around line 317-329: Add a clarifying comment above the
CmuxWebContentProcessIdentifier enum/pid(for:) explaining that it relies on the
private WebKit selector "_webProcessIdentifier" (a private/undocumented API) to
obtain the web content process PID for debugging/monitoring, that the
implementation already guards for absence of the selector and will return nil as
a safe fallback, and that this is a fragile dependency which may break on future
macOS/WebKit updates so maintainers should be aware of the tradeoff and consider
alternatives if/when it breaks.
🪄 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: 911112e2-138e-4bb5-b590-e8aecf626f3e
📒 Files selected for processing (3)
CLI/cmux.swiftSources/CmuxTopSnapshot.swiftSources/TerminalController.swift
lawrencecchen
changed the title
5bd03f0 to
bfbf5b5
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (3)
CLI/cmux.swift (2)
10738-10827:⚠️ Potential issue | 🟠 MajorSanitize titles/URLs/tag/process labels before terminal rendering.
Line 10740+, 10765+, 10812+, and 10824+ still concatenate raw strings into terminal output. ANSI/control/newline content can corrupt layout or inject terminal control behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLI/cmux.swift` around lines 10738 - 10827, The terminal labels are constructed from raw strings (titles, URLs, tags, pids, names, tty) and must be sanitized to strip control/ANSI sequences and newlines before rendering; add a helper like sanitizeForTerminal(_ s: String) -> String that trims, replaces newlines with spaces, and removes ANSI/control bytes (e.g. strip \u001B[...] sequences and other non-printables), then call it where raw values are used: topWorkspaceLabel (title), topSurfaceLabel (title, tty, url), topTagLabel (key, value), topWebViewLabel (title, url), and topProcessLabel (name) so all concatenated strings are passed through the sanitizer before being appended to parts.
10679-10700:⚠️ Potential issue | 🟠 MajorSurface process trees are still skipped when webviews exist.
Because of the
else if showProcessesbranch (Line 10694),surface["processes"]is rendered only whenwebviewsis empty. That still drops surface-level processes on browser surfaces.Suggested fix
- if !webviews.isEmpty { + if !webviews.isEmpty { for (webviewIndex, webview) in webviews.enumerated() { let webviewIsLast = webviewIndex == webviews.count - 1 let webviewBranch = webviewIsLast ? "└── " : "├── " let webviewIndent = webviewIsLast ? " " : "│ " lines.append("\(topResourceColumns(node: webview))\(workspaceIndent)\(paneIndent)\(surfaceIndent)\(webviewBranch)\(topWebViewLabel(webview))") if showProcesses { appendTopProcessLines( webview["processes"] as? [[String: Any]] ?? [], to: &lines, indent: workspaceIndent + paneIndent + surfaceIndent + webviewIndent ) } } - } else if showProcesses { + } + if showProcesses { appendTopProcessLines( surface["processes"] as? [[String: Any]] ?? [], to: &lines, indent: workspaceIndent + paneIndent + surfaceIndent ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLI/cmux.swift` around lines 10679 - 10700, The code currently only appends surface["processes"] in the else branch when webviews is empty, which omits surface-level processes whenever webviews exist; change the logic so that after handling webviews (if any) you always conditionally append surface processes when showProcesses is true. Specifically, keep the existing webview loop that calls appendTopProcessLines(webview["processes"]...), and then outside that if-block (not in the else) call appendTopProcessLines(surface["processes"] as? [[String: Any]] ?? [], to: &lines, indent: workspaceIndent + paneIndent + surfaceIndent) guarded by if showProcesses; this preserves webview process output and also renders surface-level processes.Sources/CmuxTopSnapshot.swift (1)
292-302:⚠️ Potential issue | 🟡 MinorUse
statinstead of opening the TTY device.Opening the device with
Darwin.opencan fail for devices that are stat-able but not openable (e.g., TTYs owned by other users), causing TTY-based process attribution to silently fail even when the path is valid.Suggested fix
- let fd = Darwin.open(path, O_RDONLY | O_NOCTTY | O_NONBLOCK) - guard fd >= 0 else { - return nil - } - defer { Darwin.close(fd) } - var statInfo = stat() - guard fstat(fd, &statInfo) == 0 else { + guard Darwin.stat(path, &statInfo) == 0 else { return nil } return Int64(statInfo.st_rdev)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/CmuxTopSnapshot.swift` around lines 292 - 302, The code opens the TTY path with Darwin.open and uses fstat, which fails for stat-able but non-openable devices; replace the open/fstat logic with a direct stat on the path (call stat(path, &statInfo)) and return Int64(statInfo.st_rdev), removing the fd, open, close, and fstat usage; update the function that currently uses Darwin.open/fstat (references: Darwin.open, fstat, statInfo, st_rdev) to perform stat on the path and handle stat failure by returning nil.
🧹 Nitpick comments (5)
Sources/AppDelegate.swift (1)
6556-6558: Route Task Manager opening through the AppDelegate entry pointThis callback should call
openTaskManagerWindow()instead of duplicatingTaskManagerWindowController.shared.show()inline, so behavior stays centralized.♻️ Proposed refactor
- onOpenTaskManager: { - TaskManagerWindowController.shared.show() - }, + onOpenTaskManager: { [weak self] in + self?.openTaskManagerWindow() + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 6556 - 6558, The onOpenTaskManager callback currently calls TaskManagerWindowController.shared.show() inline; change it to call the centralized AppDelegate entry point openTaskManagerWindow() instead so behavior is routed through the single method. Replace the inline TaskManagerWindowController.shared.show() invocation inside the onOpenTaskManager closure with a call to openTaskManagerWindow() to keep Task Manager opening logic centralized.Sources/TaskManagerWindowController.swift (2)
511-522: UUID fallback creates unstable row identity.When a payload node lacks
id,pid, andref, a newUUID().uuidStringis generated on each refresh. This causes SwiftUI'sForEachto treat the row as a new item every refresh cycle, potentially causing visual flicker or losing scroll position for affected rows.Consider generating a deterministic fallback ID from available payload data (e.g., kind + parent index + child index) or logging a warning when falling back to UUID so the data source can be fixed.
Example deterministic fallback
private static func rowID(_ payload: [String: Any], kind: CmuxTaskManagerRow.Kind) -> String { if let id = nonEmptyString(payload["id"]) { return "\(kind.rawValue):\(id)" } if let pid = int(payload["pid"]) { return "\(kind.rawValue):pid:\(pid)" } if let ref = nonEmptyString(payload["ref"]) { return "\(kind.rawValue):\(ref)" } - return "\(kind.rawValue):\(UUID().uuidString)" + // Fallback: use hash of available keys for stability + let hash = (payload["key"] as? String ?? "") + (payload["title"] as? String ?? "") + return "\(kind.rawValue):hash:\(hash.hashValue)" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TaskManagerWindowController.swift` around lines 511 - 522, The rowID(_ payload:kind:) function currently falls back to a random UUID when payload lacks "id", "pid", and "ref", creating an unstable identity; change the fallback to produce a deterministic stable ID (for example derive a stable hash/string from other payload fields or combine kind.rawValue with contextual indices such as parentIndex/childIndex if available, or build a canonical JSON-sorted string of the payload and hash it) and emit a warning via the existing logging facility when the UUID path would have been used so callers can be fixed; update references to CmuxTaskManagerRow.Kind and the rowID function accordingly so the ForEach keys remain stable across refreshes.
128-133: Redundantstart()call fromonAppear.
show()at line 38 already callsmodel.start()before making the window visible. When the SwiftUI view then appears,onAppearcallsstart()again. Whilestart()is guarded to avoid creating duplicate timers, it still triggers an extrarefresh(force: true)causing two rapid refreshes in succession.Consider removing the
onAppearcall sinceshow()handles initialization, or removing thestart()fromshow()if you prefer the view to own the lifecycle.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TaskManagerWindowController.swift` around lines 128 - 133, The view currently calls model.start() in both show() and in the SwiftUI .onAppear handler, which causes an unnecessary duplicate refresh via start() -> refresh(force: true); remove one of them to prevent double refreshes: either delete the model.start() call inside the .onAppear block in TaskManagerWindowController.swift, leaving .onDisappear { model.stop() } intact, or remove the model.start() call from the show() method and let the SwiftUI lifecycle (onAppear) manage start()/stop(); ensure start(), stop(), show(), and the refresh(force:) behavior remain consistent after the change.Sources/CmuxTopSnapshot.swift (2)
79-87: Cache theISO8601DateFormatterinstance.A new
ISO8601DateFormatter()is created on every call tosamplePayload(). Since this method is called frequently (every 3 seconds via the timer), the repeated allocations are wasteful.Suggested fix
final class CmuxTopProcessSnapshot: `@unchecked` Sendable { private static let cpuScale = 2048.0 private static let pidPathBufferSize = 4096 + private static let isoFormatter = ISO8601DateFormatter() // ... func samplePayload() -> [String: Any] { [ - "sampled_at": ISO8601DateFormatter().string(from: sampledAt), + "sampled_at": Self.isoFormatter.string(from: sampledAt), "source": "sysctl+proc_pidinfo", "cpu_source": "kinfo_proc.p_pctcpu", "memory_source": "proc_pidinfo.PROC_PIDTASKINFO", "process_details": includesProcessDetails ] }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/CmuxTopSnapshot.swift` around lines 79 - 87, samplePayload() creates a new ISO8601DateFormatter on every call; cache a single formatter instance instead (e.g. as a static/private property on CmuxTopSnapshot or a shared static member) and replace ISO8601DateFormatter() in samplePayload() with the cached formatter (use the cached formatter to call string(from: sampledAt)). This avoids repeated allocations while keeping the formatting logic in the samplePayload() method (refer to samplePayload and CmuxTopSnapshot).
317-329: No public API exists for retrieving web process PID; graceful nil fallback is adequate.
_webProcessIdentifieris indeed a private undocumented API with no public alternative. The gracefulnilfallback already mitigates the impact of potential breakage on future macOS versions, returningnilif the selector cannot be found. For a developer tool like cmux, this approach is acceptable.Consider adding a comment (optional) explaining why this private API is necessary and that the fallback handles future incompatibility gracefully:
// Private API: WKWebView._webProcessIdentifier has no public alternative for retrieving // the web content process PID on macOS. Graceful nil fallback handles future API changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/CmuxTopSnapshot.swift` around lines 317 - 329, Add an explanatory comment above the CmuxWebContentProcessIdentifier.pid(for:) implementation stating that it relies on the private selector "_webProcessIdentifier" because there is no public API to retrieve a WKWebView web process PID on macOS and that the guard returning nil gracefully handles future API changes; keep the existing implementation (selector lookup, class_getInstanceMethod, unsafeBitCast to WebProcessIdentifierFn, and nil return when pid <= 0) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@CLI/cmux.swift`:
- Around line 10738-10827: The terminal labels are constructed from raw strings
(titles, URLs, tags, pids, names, tty) and must be sanitized to strip
control/ANSI sequences and newlines before rendering; add a helper like
sanitizeForTerminal(_ s: String) -> String that trims, replaces newlines with
spaces, and removes ANSI/control bytes (e.g. strip \u001B[...] sequences and
other non-printables), then call it where raw values are used: topWorkspaceLabel
(title), topSurfaceLabel (title, tty, url), topTagLabel (key, value),
topWebViewLabel (title, url), and topProcessLabel (name) so all concatenated
strings are passed through the sanitizer before being appended to parts.
- Around line 10679-10700: The code currently only appends surface["processes"]
in the else branch when webviews is empty, which omits surface-level processes
whenever webviews exist; change the logic so that after handling webviews (if
any) you always conditionally append surface processes when showProcesses is
true. Specifically, keep the existing webview loop that calls
appendTopProcessLines(webview["processes"]...), and then outside that if-block
(not in the else) call appendTopProcessLines(surface["processes"] as? [[String:
Any]] ?? [], to: &lines, indent: workspaceIndent + paneIndent + surfaceIndent)
guarded by if showProcesses; this preserves webview process output and also
renders surface-level processes.
In `@Sources/CmuxTopSnapshot.swift`:
- Around line 292-302: The code opens the TTY path with Darwin.open and uses
fstat, which fails for stat-able but non-openable devices; replace the
open/fstat logic with a direct stat on the path (call stat(path, &statInfo)) and
return Int64(statInfo.st_rdev), removing the fd, open, close, and fstat usage;
update the function that currently uses Darwin.open/fstat (references:
Darwin.open, fstat, statInfo, st_rdev) to perform stat on the path and handle
stat failure by returning nil.
---
Nitpick comments:
In `@Sources/AppDelegate.swift`:
- Around line 6556-6558: The onOpenTaskManager callback currently calls
TaskManagerWindowController.shared.show() inline; change it to call the
centralized AppDelegate entry point openTaskManagerWindow() instead so behavior
is routed through the single method. Replace the inline
TaskManagerWindowController.shared.show() invocation inside the
onOpenTaskManager closure with a call to openTaskManagerWindow() to keep Task
Manager opening logic centralized.
In `@Sources/CmuxTopSnapshot.swift`:
- Around line 79-87: samplePayload() creates a new ISO8601DateFormatter on every
call; cache a single formatter instance instead (e.g. as a static/private
property on CmuxTopSnapshot or a shared static member) and replace
ISO8601DateFormatter() in samplePayload() with the cached formatter (use the
cached formatter to call string(from: sampledAt)). This avoids repeated
allocations while keeping the formatting logic in the samplePayload() method
(refer to samplePayload and CmuxTopSnapshot).
- Around line 317-329: Add an explanatory comment above the
CmuxWebContentProcessIdentifier.pid(for:) implementation stating that it relies
on the private selector "_webProcessIdentifier" because there is no public API
to retrieve a WKWebView web process PID on macOS and that the guard returning
nil gracefully handles future API changes; keep the existing implementation
(selector lookup, class_getInstanceMethod, unsafeBitCast to
WebProcessIdentifierFn, and nil return when pid <= 0) unchanged.
In `@Sources/TaskManagerWindowController.swift`:
- Around line 511-522: The rowID(_ payload:kind:) function currently falls back
to a random UUID when payload lacks "id", "pid", and "ref", creating an unstable
identity; change the fallback to produce a deterministic stable ID (for example
derive a stable hash/string from other payload fields or combine kind.rawValue
with contextual indices such as parentIndex/childIndex if available, or build a
canonical JSON-sorted string of the payload and hash it) and emit a warning via
the existing logging facility when the UUID path would have been used so callers
can be fixed; update references to CmuxTaskManagerRow.Kind and the rowID
function accordingly so the ForEach keys remain stable across refreshes.
- Around line 128-133: The view currently calls model.start() in both show() and
in the SwiftUI .onAppear handler, which causes an unnecessary duplicate refresh
via start() -> refresh(force: true); remove one of them to prevent double
refreshes: either delete the model.start() call inside the .onAppear block in
TaskManagerWindowController.swift, leaving .onDisappear { model.stop() } intact,
or remove the model.start() call from the show() method and let the SwiftUI
lifecycle (onAppear) manage start()/stop(); ensure start(), stop(), show(), and
the refresh(force:) behavior remain consistent after the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a75b61bc-8c2b-4321-9160-e774f0f26653
📒 Files selected for processing (9)
CLI/cmux.swiftGhosttyTabs.xcodeproj/project.pbxprojResources/Localizable.xcstringsSources/App/MenuBarExtraController.swiftSources/AppDelegate.swiftSources/CmuxTopSnapshot.swiftSources/TaskManagerWindowController.swiftSources/TerminalController.swiftSources/cmuxApp.swift
✅ Files skipped from review due to trivial changes (2)
- GhosttyTabs.xcodeproj/project.pbxproj
- Resources/Localizable.xcstrings
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bfbf5b5d95
ℹ️ 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".
There was a problem hiding this comment.
🧹 Nitpick comments (2)
Sources/TaskManagerSnapshot.swift (2)
212-223: UUID fallback inrowIDcould cause identity instability.When a payload item lacks
id,pid, andref, a new UUID is generated on each parse (line 222). This could cause SwiftUIForEachto treat the same logical row as a new item on refresh, leading to unnecessary view recreation.In practice, the payload from
TerminalController.taskManagerTopPayloadalways includesidorreffor windows/workspaces/panes/surfaces andpidfor processes, so this fallback should rarely trigger. The current implementation is acceptable given the data contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TaskManagerSnapshot.swift` around lines 212 - 223, The rowID(_:kind:) fallback generates a new UUID each parse causing unstable identities; update rowID to avoid ephemeral UUIDs by deriving a deterministic fallback from available immutable payload data (e.g., hash or concatenation of stable fields) instead of UUID().uuidString so ForEach won't treat the same logical row as new; locate the rowID function and replace the final UUID-based return with a deterministic stable identifier computed from payload (using nonEmptyString(_:), int(_:), and CmuxTaskManagerRow.Kind to build the key).
248-261: Consider consolidating parsing utilities withCmuxTaskManagerResources.The
bool()andint()helpers duplicate similar logic inTaskManagerTypes.swift(lines 91-98). Since both files are part of the same module, these could be consolidated into a shared internal helper to reduce duplication.This is a minor suggestion for future cleanup rather than a blocking concern.
♻️ Potential consolidation approach
Move the shared parsing utilities to
CmuxTaskManagerResourcesor a separate internal helper:// In TaskManagerTypes.swift, make helpers internal extension CmuxTaskManagerResources { static func bool(_ raw: Any?) -> Bool { if let value = raw as? Bool { return value } if let value = raw as? NSNumber { return value.boolValue } return false } // int() already exists, could be made internal }Then in
TaskManagerSnapshot.swift, useCmuxTaskManagerResources.bool(_:)instead of the local helper.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TaskManagerSnapshot.swift` around lines 248 - 261, The bool(_:) and int(_:) helpers in TaskManagerSnapshot.swift duplicate parsing logic already present in TaskManagerTypes.swift; consolidate by removing the local helpers and calling the shared internal helpers on CmuxTaskManagerResources (or move the existing implementations into CmuxTaskManagerResources as internal/static methods) so TaskManagerSnapshot uses CmuxTaskManagerResources.bool(_:) and CmuxTaskManagerResources.int(_:) instead; update access levels in TaskManagerTypes.swift if necessary to make the helpers accessible within the module and remove the duplicate functions from TaskManagerSnapshot.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Sources/TaskManagerSnapshot.swift`:
- Around line 212-223: The rowID(_:kind:) fallback generates a new UUID each
parse causing unstable identities; update rowID to avoid ephemeral UUIDs by
deriving a deterministic fallback from available immutable payload data (e.g.,
hash or concatenation of stable fields) instead of UUID().uuidString so ForEach
won't treat the same logical row as new; locate the rowID function and replace
the final UUID-based return with a deterministic stable identifier computed from
payload (using nonEmptyString(_:), int(_:), and CmuxTaskManagerRow.Kind to build
the key).
- Around line 248-261: The bool(_:) and int(_:) helpers in
TaskManagerSnapshot.swift duplicate parsing logic already present in
TaskManagerTypes.swift; consolidate by removing the local helpers and calling
the shared internal helpers on CmuxTaskManagerResources (or move the existing
implementations into CmuxTaskManagerResources as internal/static methods) so
TaskManagerSnapshot uses CmuxTaskManagerResources.bool(_:) and
CmuxTaskManagerResources.int(_:) instead; update access levels in
TaskManagerTypes.swift if necessary to make the helpers accessible within the
module and remove the duplicate functions from TaskManagerSnapshot.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 41210775-d9de-454a-9992-1058d18f7f77
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (4)
GhosttyTabs.xcodeproj/project.pbxprojSources/TaskManagerSnapshot.swiftSources/TaskManagerTypes.swiftSources/TaskManagerWindowController.swift
✅ Files skipped from review due to trivial changes (1)
- GhosttyTabs.xcodeproj/project.pbxproj
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Sources/TaskManagerWindowController.swift (1)
128-133: 💤 Low valueRedundant lifecycle calls provide safety net.
Both the controller (
show()/windowWillClose) and the view (onAppear/onDisappear) manage model lifecycle. This is redundant but benign—start()guards against duplicate timers andstop()is idempotent. The duplication provides robustness if one path doesn't fire as expected.If you prefer a single source of truth, consider removing these and relying solely on the controller's window delegate callbacks.
,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TaskManagerWindowController.swift` around lines 128 - 133, The view currently calls model.start() / model.stop() via .onAppear and .onDisappear while TaskManagerWindowController also manages lifecycle in show() and windowWillClose, causing redundant calls; to centralize lifecycle management, remove the .onAppear { model.start() } and .onDisappear { model.stop() } from the SwiftUI view and rely solely on the controller's show() and windowWillClose to call model.start() and model.stop(); if you prefer the opposite, instead remove the controller calls and keep the view modifiers, but ensure model.start() and model.stop() remain present and idempotent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Sources/TaskManagerWindowController.swift`:
- Around line 128-133: The view currently calls model.start() / model.stop() via
.onAppear and .onDisappear while TaskManagerWindowController also manages
lifecycle in show() and windowWillClose, causing redundant calls; to centralize
lifecycle management, remove the .onAppear { model.start() } and .onDisappear {
model.stop() } from the SwiftUI view and rely solely on the controller's show()
and windowWillClose to call model.start() and model.stop(); if you prefer the
opposite, instead remove the controller calls and keep the view modifiers, but
ensure model.start() and model.stop() remain present and idempotent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 68a27603-cad5-4173-a9c7-17feb3eb0b79
📒 Files selected for processing (1)
Sources/TaskManagerWindowController.swift
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c9df53906
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 89b544e7dd
ℹ️ 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".
This branch was successfully deployed
1 participant
Summary
system.topsocket snapshots for window, workspace, pane, surface, status tag, and browser webview resource trees.cmux topas a one-shot CLI with--all,--workspace,--processes, and--json.Verification
./scripts/reload.sh --tag cli-topcmux top --helpgit diff --checkjq empty Resources/Localizable.xcstringspython3 scripts/swift_file_length_budget.py --budget .github/swift-file-length-budget.tsv./tests/test_ci_swift_file_length_budget.sh./tests/test_ci_swift_warning_budget.shSummary by CodeRabbit
topCLI command with --all, --workspace, --processes and --json options; strict flag validation and user-facing message when remote support is missing.