Repository navigation
Conversation
|
@PabloLION is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdded per-surface current working directory ( Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip You can customize the tone of the review comments and chat replies.Configure the |
Greptile SummaryThis PR exposes the per-surface working directory (tracked via OSC 7) in the Key changes:
Confidence Score: 2/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant TerminalController
participant Workspace
Note over Client,Workspace: v2 surface.list
Client->>TerminalController: surface.list {workspace_id}
TerminalController->>Workspace: panelDirectories[panel.id]
Workspace-->>TerminalController: String? (OSC 7 path)
TerminalController-->>Client: {surfaces: [{id, cwd: "/path" | null, ...}]}
Note over Client,Workspace: v2 pane.surfaces
Client->>TerminalController: pane.surfaces {pane_id}
TerminalController->>Workspace: panelDirectories[panelId]
Workspace-->>TerminalController: String? (OSC 7 path)
TerminalController-->>Client: {surfaces: [{id, cwd: "/path" | null, ...}]}
Note over Client,Workspace: v1 list_surfaces (⚠️ breaking)
Client->>TerminalController: list_surfaces [tab]
TerminalController->>Workspace: panelDirectories[panel.id]
Workspace-->>TerminalController: String? (OSC 7 path)
TerminalController-->>Client: "* 0: UUID /cwd/path\n 1: UUID"
Note over Client: ⚠️ Parsers split on space and<br/>capture "UUID /cwd/path" as surface_id
Last reviewed commit: 7e3b5d3 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| let cwdSuffix = tab.panelDirectories[panel.id].map { " \($0)" } ?? "" | ||
| return "\(selected) \(index): \(panel.id.uuidString)\(cwdSuffix)" |
There was a problem hiding this comment.
V1 format change breaks existing parsers
Appending the CWD with a space separator breaks two existing parsers that parse the v1 list_surfaces text output:
1. tests/cmux.py lines 344–348:
parts = line.lstrip("* ").split(" ", 1)
if len(parts) >= 2:
index = int(parts[0].rstrip(":"))
surface_id = parts[1] # ← was UUID alone; now "UUID /cwd/path" when cwd is knownAfter this PR, when cwd is available, surface_id will be set to "<UUID> /path/to/cwd" instead of just "<UUID>". Every downstream call that passes this string as a surface/panel ID (e.g. focus_surface, close_surface) will silently receive an invalid UUID and fail or behave unexpectedly.
2. cmuxUITests/MultiWindowNotificationsUITests.swift lines 560–567:
let parts = line.split(separator: ":", maxSplits: 1, ...)
let candidate = String(parts[1]).trimmingCharacters(in: .whitespacesAndNewlines)
if UUID(uuidString: candidate) != nil { return candidate }After this change, candidate becomes "<UUID> /path/to/cwd", which is not a valid UUID string, so UUID(uuidString:) returns nil and firstSurfaceId() will always return nil when CWD is set, silently breaking the UI test helpers.
Both parsers need to be updated to strip the optional cwd suffix. Alternatively, consider using a more unambiguous delimiter (e.g., a tab \t) between the UUID and the cwd to make the format forward-compatible and less fragile for paths that contain spaces:
| let cwdSuffix = tab.panelDirectories[panel.id].map { " \($0)" } ?? "" | |
| return "\(selected) \(index): \(panel.id.uuidString)\(cwdSuffix)" | |
| let cwdSuffix = tab.panelDirectories[panel.id].map { "\t\($0)" } ?? "" | |
| return "\(selected) \(index): \(panel.id.uuidString)\(cwdSuffix)" |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/TerminalController.swift (1)
10663-10664:⚠️ Potential issue | 🟡 MinorEscape
cwdbefore appending it to v1 output.
list_surfacesis a line-oriented text protocol, but this now concatenates the raw directory path. SincepanelDirectoriescan contain decoded OSC 7 paths, a cwd with\n,\r, or other control characters will corrupt the response format and break parsers.[suggested fix]
Escape the suffix before formatting the line
- let cwdSuffix = tab.panelDirectories[panel.id].map { " \($0)" } ?? "" + let cwdSuffix = tab.panelDirectories[panel.id].map { " \(Self.escapeSocketLineValue($0))" } ?? "" return "\(selected) \(index): \(panel.id.uuidString)\(cwdSuffix)"private nonisolated static func escapeSocketLineValue(_ value: String) -> String { value .replacingOccurrences(of: "\\", with: "\\\\") .replacingOccurrences(of: "\n", with: "\\n") .replacingOccurrences(of: "\r", with: "\\r") .replacingOccurrences(of: "\t", with: "\\t") .replacingOccurrences(of: "\u{1B}", with: "\\e") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TerminalController.swift` around lines 10663 - 10664, The listWorkspaces() output currently appends raw cwd values which can include control characters and break the line-oriented list_surfaces protocol; fix by escaping the suffix before formatting each line—add a helper (e.g., nonisolated static func escapeSocketLineValue(_:)) that replaces backslash, newline, carriage return, tab and ESC with safe escape sequences and call this helper when building each line in listWorkspaces() (use escapeSocketLineValue(panelDirectory) or equivalent) so the emitted v1 lines never contain raw control characters.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@Sources/TerminalController.swift`:
- Around line 10663-10664: The listWorkspaces() output currently appends raw cwd
values which can include control characters and break the line-oriented
list_surfaces protocol; fix by escaping the suffix before formatting each
line—add a helper (e.g., nonisolated static func escapeSocketLineValue(_:)) that
replaces backslash, newline, carriage return, tab and ESC with safe escape
sequences and call this helper when building each line in listWorkspaces() (use
escapeSocketLineValue(panelDirectory) or equivalent) so the emitted v1 lines
never contain raw control characters.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 126c00d3-a922-4a8b-a70b-8bde0e0a2dee
📒 Files selected for processing (1)
Sources/TerminalController.swift
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found across 1 file
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
Expose per-surface working directory (tracked via OSC 7) in surface.list and pane.surfaces v2 JSON-RPC responses. Callers can now query CWD of all open terminal surfaces in a single call. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
7e3b5d3 to
ecbdf84
Compare
|
Dropped the v1 Reason: The v1 text format is parsed by The outside-diff comment about escaping control characters in v1 output is no longer applicable. |
teamleaderleo
added
the
area: cli
This branch has not been deployed
2 participants
Summary
surface.listandpane.surfacesv2 JSON-RPC responses as a newcwdfieldChanges
All in
Sources/TerminalController.swift:v2SurfaceList— added"cwd": v2OrNull(ws.panelDirectories[panel.id])v2PaneSurfaces— added"cwd": v2OrNull(panelId.flatMap { ws.panelDirectories[$0] })The v1
list_surfacestext format is intentionally left unchanged to avoid breaking existing parsers intests/cmux.pyandcmuxUITests/MultiWindowNotificationsUITests.swiftthat extract UUIDs by splitting on whitespace.Test plan
surface.listresponse includescwdfield with correct pathpane.surfacesresponse includescwdfieldcwdisnullwhen OSC 7 hasn't reported yetlist_surfacesoutput is unchanged🤖 Generated with Claude Code
Summary by CodeRabbit