Repository navigation
Add beta Workspace Tasks - #6708
Closed
azooz2003-bit wants to merge 29 commits into
Closed
azooz2003-bit wants to merge 29 commits into
azooz2003-bit wants to merge 29 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
Greptile SummaryThis PR adds beta-gated Workspace Tasks and Workspace Controls — per-workspace open/archive task lists with a new
Confidence Score: 4/5Safe to merge after adding The ControlCommandExecutionPolicy.swift and its test file: Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant CLI as cmux CLI / Socket client
participant Policy as ControlCommandExecutionPolicy
participant Worker as Socket Worker Lane
participant TC as TerminalController (nonisolated)
participant Main as MainActor (v2MainSync)
participant WS as Workspace
CLI->>Policy: "workspace.tasks.* command"
Policy-->>Worker: socketWorkerMethods allowlist match
Worker->>TC: "v2WorkspaceTasks{List,Add,Archive,Unarchive,Remove,Move,Open}(params)"
TC->>Main: "v2MainSync { resolve workspace }"
Main->>WS: addWorkspaceTask / archiveWorkspaceTask / etc.
WS-->>Main: mutated WorkspaceTask
Main-->>TC: V2CallResult (.ok / .err)
TC-->>Worker: result
Worker-->>CLI: JSON response
Note over Policy,Worker: workspace.tasks.unarchive is missing from socketWorkerMethods — routes incorrectly
%%{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"}}}%%
sequenceDiagram
participant CLI as cmux CLI / Socket client
participant Policy as ControlCommandExecutionPolicy
participant Worker as Socket Worker Lane
participant TC as TerminalController (nonisolated)
participant Main as MainActor (v2MainSync)
participant WS as Workspace
CLI->>Policy: "workspace.tasks.* command"
Policy-->>Worker: socketWorkerMethods allowlist match
Worker->>TC: "v2WorkspaceTasks{List,Add,Archive,Unarchive,Remove,Move,Open}(params)"
TC->>Main: "v2MainSync { resolve workspace }"
Main->>WS: addWorkspaceTask / archiveWorkspaceTask / etc.
WS-->>Main: mutated WorkspaceTask
Main-->>TC: V2CallResult (.ok / .err)
TC-->>Worker: result
Worker-->>CLI: JSON response
Note over Policy,Worker: workspace.tasks.unarchive is missing from socketWorkerMethods — routes incorrectly
Reviews (21): 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}
Comment on lines
+183
to
+188
| private nonisolated static func workspaceTaskDateString(_ date: Date) -> String { | ||
| let formatter = ISO8601DateFormatter() | ||
| formatter.formatOptions = [.withInternetDateTime, .withFractionalSeconds] | ||
| formatter.timeZone = TimeZone(secondsFromGMT: 0) | ||
| return formatter.string(from: date) | ||
| } |
Contributor
There was a problem hiding this comment.
ISO8601DateFormatter is allocated fresh on every call to workspaceTaskDateString, and this method is called per task inside the map(v2WorkspaceTaskPayload) closures (up to twice per task — once for createdAt, once for archivedAt). ISO8601DateFormatter is an expensive Objective-C bridge object. A static cached instance avoids the per-element allocation while remaining thread-safe since the formatter is fully configured before the first use and never mutated after.
Suggested change
| private nonisolated static func workspaceTaskDateString(_ date: Date) -> String { | |
| let formatter = ISO8601DateFormatter() | |
| formatter.formatOptions = [.withInternetDateTime, .withFractionalSeconds] | |
| formatter.timeZone = TimeZone(secondsFromGMT: 0) | |
| return formatter.string(from: date) | |
| } | |
| private nonisolated static let workspaceTaskDateFormatter: ISO8601DateFormatter = { | |
| let formatter = ISO8601DateFormatter() | |
| formatter.formatOptions = [.withInternetDateTime, .withFractionalSeconds] | |
| formatter.timeZone = TimeZone(secondsFromGMT: 0) | |
| return formatter | |
| }() | |
| private nonisolated static func workspaceTaskDateString(_ date: Date) -> String { | |
| workspaceTaskDateFormatter.string(from: date) | |
| } |
Rule Used: Flag per-call allocating formatting on hot or conc... (source)
Comment on lines
+150
to
+165
| ) -> [String: Any] { | ||
| let windowId = v2ResolveWindowId(tabManager: tabManager) | ||
| var payload: [String: Any] = [ | ||
| "window_id": v2OrNull(windowId?.uuidString), | ||
| "window_ref": v2Ref(kind: .window, uuid: windowId), | ||
| "workspace_id": workspace.id.uuidString, | ||
| "workspace_ref": v2Ref(kind: .workspace, uuid: workspace.id), | ||
| "tasks": workspace.workspaceTasks.map(v2WorkspaceTaskPayload), | ||
| "open": workspace.openWorkspaceTasks.map(v2WorkspaceTaskPayload), | ||
| "archived": workspace.archivedWorkspaceTasks.map(v2WorkspaceTaskPayload), | ||
| "open_count": workspace.openWorkspaceTasks.count, | ||
| "archived_count": workspace.archivedWorkspaceTasks.count | ||
| ] | ||
| if let changedTask { | ||
| payload["task"] = v2WorkspaceTaskPayload(changedTask) | ||
| } |
Contributor
There was a problem hiding this comment.
workspaceTasks in every socket response
v2WorkspaceTasksPayload iterates workspace.workspaceTasks three times: once directly for "tasks", once via workspace.openWorkspaceTasks (which filters the whole list for isOpen), and once via workspace.archivedWorkspaceTasks (which filters for isArchived). That is 3n iterations and 2n intermediate filter passes on a user-owned collection with no configured bound. A single partition loop would produce all three arrays in O(n) and avoid allocating two intermediate filtered arrays per socket response. Since every mutation command calls this payload builder, the overhead compounds with task-list size.
Rule Used: Flag production code that adds nested full-collect... (source)
|
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:
📝 WalkthroughWalkthroughIntroduces a complete beta-gated per-workspace task system across data model, workspace state, v2 RPC methods, CLI command namespace, configurable sidebar hover controls, SwiftUI panel UI, settings management, and session persistence. Additionally, implements mobile live terminal font-size updates via streaming events and socket dispatch. ChangesWorkspace Tasks and Sidebar Controls
Mobile Terminal Font Updates
Sequence Diagram(s)sequenceDiagram
participant User
participant SidebarTabItemView as TabItemView
participant WorkspaceTasksPopover
participant Workspace
participant v2RPC as v2 RPC Handler
participant CLI as CLI / Socket Client
rect rgba(100, 149, 237, 0.5)
Note over User, Workspace: Sidebar control interaction flow
User->>SidebarTabItemView: hover workspace row
SidebarTabItemView->>SidebarTabItemView: compute WorkspaceRowControlsSnapshot.resolved()
SidebarTabItemView->>SidebarTabItemView: render close/tasks control buttons
User->>SidebarTabItemView: click tasks button
SidebarTabItemView->>WorkspaceTasksPopover: present popover
User->>WorkspaceTasksPopover: click "open as tab"
WorkspaceTasksPopover->>Workspace: openOrFocusWorkspaceTasksSurface()
Workspace->>Workspace: newWorkspaceTasksSurface or reattach panel
Workspace-->>SidebarTabItemView: panel surface created
end
rect rgba(60, 179, 113, 0.5)
Note over CLI, Workspace: Socket RPC task mutation path
CLI->>v2RPC: workspace.tasks.add {title, before_task_id}
v2RPC->>v2RPC: validate beta enabled, UUIDs, resolve workspace
v2RPC->>Workspace: addWorkspaceTask(title:before:)
Workspace->>Workspace: sanitizedWorkspaceTasks, insert, `@Published` notify
Workspace-->>v2RPC: inserted WorkspaceTask
v2RPC->>v2RPC: construct v2WorkspaceTasksPayload(task, open[], archived[])
v2RPC-->>CLI: {task, open, archived, counts}
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 2 warnings)
✅ Passed checks (21 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 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 7585-8065: Extract the workspace-tasks command implementation into
a separate scoped type or file to reduce the size of the monolithic CLI file.
Create a new type (such as WorkspaceTasksCommandHandler) and move all the
implementation functions including runWorkspaceTasksListCommand,
runWorkspaceTasksAddCommand, runWorkspaceTasksTaskMutationCommand,
runWorkspaceTasksMoveCommand, runWorkspaceTasksOpenCommand,
workspaceTasksTargetParams, applyWorkspaceTaskPlacement,
rejectUnknownWorkspaceTasksFlags, positionalArguments, and all the summary/usage
helper functions into this new type. Keep only the runWorkspaceTasksCommand
function as a thin dispatch shim in the original file that delegates to the new
type, reducing the addition from 400+ lines to under 250 lines in the main CLI
file.
- Around line 7705-7721: When assigning the title using the expression titleOpt
?? positionalArguments(rem5).joined(separator: " "), add validation to reject
extra positional arguments if titleOpt is provided. After parsing the title,
check that if titleOpt is not nil, the positionalArguments(rem5) array must be
empty. If positional arguments exist when --title is explicitly used, throw a
CLIError to prevent silent dropping of unexpected arguments.
In
`@Packages/macOS/CmuxSettingsUI/Tests/CmuxSettingsUITests/SettingsRowAnchorResolutionTests.swift`:
- Around line 147-149: The new sidebar row path
"setting:sidebarAppearance:workspace-controls" has been added to the code but is
missing from the rowConfigPaths list in the SettingsRowAnchorResolutionTests
test. Add this row path to the rowConfigPaths array alongside the other existing
sidebar appearance paths to maintain the test contract that every standalone row
path resolves properly.
In `@skills/cmux-workspace-tasks/SKILL.md`:
- Around line 1-74: The Commands section of the SKILL.md documentation is
incomplete and does not show all supported flags and argument patterns for
workspace task operations. Update the "Add a task" examples to document that the
add command supports `--before` and `--index` flags in addition to `--after` for
positioning control. Update the "Insert or reorder" examples to show that the
move command accepts `--task`, `--task-id`, or `--id` as alternative flag forms
for specifying the task UUID instead of only the positional argument.
Additionally, clarify in the "Open the native Workspace Tasks surface" example
that the `--focus` flag requires a boolean string value formatted as `"true"` or
`"false"` to explicitly show users the expected value format.
In `@Sources/CmuxSettingsJSONPathSupport.swift`:
- Around line 245-246: Replace the hardcoded string literal
"sidebar.workspaceControls" at line 389 with a reference to the constant
SidebarSettingsFileMapping.workspaceControlsPath that is already defined at line
245. This ensures the settings path key is maintained in a single place and
prevents drift if the key needs to be updated in the future.
In `@Sources/ContentView.swift`:
- Around line 1245-1252: The minimum sidebar width calculation in this function
currently uses GhosttyConfig.defaultSidebarFontSize when calling
SidebarTabItemFontScale.scale, but it should use the actual configured
sidebarFontScale value instead (the same way SidebarTabItemSettingsSnapshot
resolves row controls at lines 9632-9637). Replace the hardcoded
GhosttyConfig.defaultSidebarFontSize argument with the active/configured
sidebarFontScale so that the computed minimumSidebarWidth accounts for the
user's actual font size setting, preventing under-calculation that can cause
clipping or overlap of row controls.
In `@Sources/Panels/WorkspaceTasksPanelView.swift`:
- Around line 4-5: Replace the `@ObservedObject` property wrappers for the panel
and workspace properties with the newer `@Observable` pattern. Mark the
WorkspaceTasksPanel and Workspace model classes with the `@Observable` macro
instead of conforming to ObservableObject, remove any `@Published` property
wrappers from their properties, and convert the state access to use `@State` or
immutable value snapshots with action closures as required by the repository's
SwiftUI state layout guidelines for new cmux-owned code.
In `@Sources/TerminalController`+WorkspaceTasks.swift:
- Around line 25-27: The current implementation uses nil-coalescing operators on
the result of v2UUID and v2Int calls, which silently collapses
malformed-but-present values to nil instead of detecting them as invalid
parameters. Refactor the parameter extraction logic for beforeTaskId,
afterTaskId, and index (and their fallback alternatives on the referenced lines)
to first validate that if a parameter key is present in params, it is actually
valid. If a parameter key exists but contains a malformed value, return an
invalid_params error response instead of treating it as an absent parameter. Use
explicit validation checks on the raw parameter values before attempting type
conversion, rather than relying on nil-coalescing to handle the distinction
between absent and malformed parameters.
In `@Sources/Workspace.swift`:
- Around line 2207-2210: The workspaceTasks property is currently using the
`@Published` property wrapper, which violates the project's SwiftUI state-layout
rules for cmux-owned state. Remove the `@Published` decorator from the
workspaceTasks property and refactor this workspace state to use the `@Observable`
macro pattern instead, implementing the state management through `@State`
snapshots or value closures as required by the project guidelines for new state
surfaces.
- Around line 1525-1533: The `.workspaceTasks` case in the
`createPanel(from:...)` method restores workspace tasks panels without verifying
the workspace-tasks beta feature gate is enabled. Add a guard statement at the
beginning of the `.workspaceTasks` case to check if the workspace-tasks feature
is enabled (using the appropriate beta gate check), and return nil if the
feature is disabled. This ensures that session restore respects the feature flag
and cannot bypass it.
In `@web/data/cmux.schema.json`:
- Line 767: The description text added on line 767 for the workspace sidebar
controls setting is hardcoded without a corresponding descriptionKey property
for i18n support. Add a descriptionKey field alongside the description that
references a message key following your i18n naming conventions, then create
matching entries in all locale message files listed in web/i18n/routing.ts with
the appropriate translations for each locale to ensure full internationalization
coverage.
🪄 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: d5d7ea43-cb49-47e3-9883-fca4ce7adda9
📒 Files selected for processing (48)
CLI/cmux.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Wire/ControlCommandExecutionPolicy.swiftPackages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandExecutionPolicyTests.swiftPackages/macOS/CmuxSettings/Sources/CmuxSettings/Keys/BetaFeaturesCatalogSection.swiftPackages/macOS/CmuxSettings/Sources/CmuxSettings/Keys/SidebarCatalogSection.swiftPackages/macOS/CmuxSettings/Sources/CmuxSettings/Values/WorkspaceRowControlOption.swiftPackages/macOS/CmuxSettings/Tests/CmuxSettingsTests/WorkspaceRowControlOptionTests.swiftPackages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Navigation/CuratedSettingEntry+Default.swiftPackages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/BetaFeaturesSection.swiftPackages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/SidebarSection.swiftPackages/macOS/CmuxSettingsUI/Tests/CmuxSettingsUITests/SettingsRowAnchorResolutionTests.swiftPackages/macOS/CmuxWorkspaces/Sources/CmuxWorkspaces/Core/Values/SurfaceKind.swiftPackages/macOS/CmuxWorkspaces/Tests/CmuxWorkspacesTests/Core/WorkspaceCoreValueTests.swiftResources/Localizable.xcstringsSources/Canvas/WorkspaceCanvasHostView.swiftSources/ClosedItemHistory.swiftSources/CmuxLifecycleEventPublishing.swiftSources/CmuxSettingsJSONPathSupport.swiftSources/ContentView.swiftSources/KeyboardShortcutSettingsFileStore.swiftSources/Panels/Panel.swiftSources/Panels/PanelContentView.swiftSources/Panels/WorkspaceTasksPanel.swiftSources/Panels/WorkspaceTasksPanelView.swiftSources/Search/GlobalSearchDocuments.swiftSources/SessionPersistence.swiftSources/SettingsNavigation.swiftSources/Sidebar/SidebarWorkspaceSnapshotRefreshPolicy.swiftSources/Sidebar/WorkspaceRowControlButton.swiftSources/Sidebar/WorkspaceRowControlsLayout.swiftSources/Sidebar/WorkspaceRowControlsSnapshot.swiftSources/SidebarWorkspaceGroupHeaderView.swiftSources/TabManager.swiftSources/TerminalController+ControlPaneContext.swiftSources/TerminalController+ControlSurfaceContext2.swiftSources/TerminalController+MoveTabToNewWorkspace.swiftSources/TerminalController+WorkspaceTasks.swiftSources/TerminalController.swiftSources/TerminalControllerV2ParamParsingSupport.swiftSources/TerminalPaneDropTargetView.swiftSources/Workspace+Tasks.swiftSources/Workspace.swiftSources/WorkspaceTask.swiftcmux.xcodeproj/project.pbxprojcmuxTests/WorkspaceTasksTests.swiftdocs/cli-contract.mdskills/cmux-workspace-tasks/SKILL.mdweb/data/cmux.schema.json
Comment on lines
+88
to
+100
| return v2WorkspaceTasksCommand(params: params) { workspace, tabManager in | ||
| guard let task = workspace.archiveWorkspaceTask(id: taskId) else { | ||
| return .err(code: "not_found", message: String(localized: "socket.workspaceTasks.notFound", defaultValue: "Task not found"), data: ["task_id": taskId.uuidString]) | ||
| } | ||
| return .ok(v2WorkspaceTasksPayload(workspace: workspace, tabManager: tabManager, changedTask: task)) | ||
| } | ||
| } | ||
|
|
||
| nonisolated func v2WorkspaceTasksRemove(params: [String: Any]) -> V2CallResult { | ||
| guard let taskId = v2UUID(params, "task_id") ?? v2UUID(params, "id") else { | ||
| return .err( | ||
| code: "invalid_params", | ||
| message: String(localized: "socket.workspaceTasks.taskIdRequired", defaultValue: "workspace task command requires task_id"), |
Contributor
There was a problem hiding this comment.
not_found returned when archive is full
archiveWorkspaceTask returns nil for two distinct reasons: the task ID was not found, and the task exists but the archive cap (200) has been reached. Because v2WorkspaceTasksArchive maps both nil returns to the same not_found / "Task not found" response, a caller that supplies a valid task_id of an open task will receive {"code":"not_found"} when the archive bucket is full — indistinguishable from a genuinely missing task. An agent or script that retries on not_found will loop indefinitely; one that surfaces the message to the user will show an incorrect error. The add command avoids this by pre-checking openCount < maximumOpenTaskCount before calling addWorkspaceTask, then mapping the residual nil to an anchor-not-found error. The same separation is needed here: pre-check the archive count inside the v2WorkspaceTasksCommand closure and return limit_exceeded before calling the mutating method.
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/Panels/WorkspaceTasksPanelView.swift`:
- Around line 19-20: The computed properties openWorkspaceTasks and
archivedWorkspaceTasks in the Workspace model are filtering the entire
workspaceTasks collection on every access, causing redundant O(n) scans whenever
the body property renders. Instead of leaving these as computed properties that
filter repeatedly, refactor the Workspace model to cache the filtered task
arrays as stored properties that update only when workspaceTasks changes (using
didSet or by observing the `@Published` property). Alternatively, compute the
filtered snapshots once in a single location before passing them to
WorkspaceTasksPanelView and WorkspaceTasksPopoverView to ensure filtering
happens only once per state change rather than on every render cycle.
🪄 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: 1020ef2b-fb89-4616-a11e-b7d78484f6ea
📒 Files selected for processing (2)
Resources/Localizable.xcstringsSources/Panels/WorkspaceTasksPanelView.swift
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+WorkspaceTasks.swift (1)
81-81: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winValidate malformed task_id instead of treating it as absent.
Lines 81, 97, and 113 extract
task_id/idwith nil-coalescing, then fail with "requires task_id" when nil. Iftask_idis present but malformed (e.g., invalid UUID format),v2UUIDreturns nil and the error message is misleading—it should report "Unresolved task_id" not "requires task_id."This is the same issue the past review comment flagged for placement params (lines 42-44), which was correctly fixed by introducing
v2WorkspaceTasksPlacementValidationError(lines 271-299). Apply the same validation pattern here.🛡️ Proposed fix pattern
Before extracting
task_idin each method, validate it explicitly:nonisolated func v2WorkspaceTasksArchive(params: [String: Any]) -> V2CallResult { - guard let taskId = v2UUID(params, "task_id") ?? v2UUID(params, "id") else { + for key in ["task_id", "id"] { + if v2HasNonNullParam(params, key), v2UUID(params, key) == nil { + return .err( + code: "invalid_params", + message: String( + format: String(localized: "socket.workspaceTasks.unresolvedIdentifier", defaultValue: "Unresolved %@"), + locale: .current, + key + ), + data: nil + ) + } + } + guard let taskId = v2UUID(params, "task_id") ?? v2UUID(params, "id") else { return .err( code: "invalid_params", message: String(localized: "socket.workspaceTasks.taskIdRequired", defaultValue: "workspace task command requires task_id"), data: nil ) }Apply the same pattern to
v2WorkspaceTasksRemove(line 97) andv2WorkspaceTasksMove(line 113).Also applies to: 97-97, 113-113
🤖 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/TerminalController`+WorkspaceTasks.swift at line 81, The issue is that task_id/id parameter extraction at lines 81, 97, and 113 uses nil-coalescing which treats malformed UUIDs (invalid format) the same as missing parameters, resulting in a misleading "requires task_id" error instead of "Unresolved task_id". Apply the same validation error pattern that was implemented for placement params using v2WorkspaceTasksPlacementValidationError (lines 271-299) to each of the three methods. Before extracting task_id in each method (the unnamed method containing line 81, v2WorkspaceTasksRemove at line 97, and v2WorkspaceTasksMove at line 113), validate the parameter explicitly to distinguish between missing and malformed values, then throw the appropriate error that distinguishes these two cases just as the placement validation error does.
🤖 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.
Outside diff comments:
In `@Sources/TerminalController`+WorkspaceTasks.swift:
- Line 81: The issue is that task_id/id parameter extraction at lines 81, 97,
and 113 uses nil-coalescing which treats malformed UUIDs (invalid format) the
same as missing parameters, resulting in a misleading "requires task_id" error
instead of "Unresolved task_id". Apply the same validation error pattern that
was implemented for placement params using
v2WorkspaceTasksPlacementValidationError (lines 271-299) to each of the three
methods. Before extracting task_id in each method (the unnamed method containing
line 81, v2WorkspaceTasksRemove at line 97, and v2WorkspaceTasksMove at line
113), validate the parameter explicitly to distinguish between missing and
malformed values, then throw the appropriate error that distinguishes these two
cases just as the placement validation error does.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 65dab73f-3606-4755-b470-6508a8d4ca41
📒 Files selected for processing (8)
CLI/cmux.swiftResources/Localizable.xcstringsSources/Panels/WorkspaceTasksPanelView.swiftSources/TerminalController+WorkspaceTasks.swiftSources/Workspace+Tasks.swiftSources/WorkspaceTask.swiftcmuxTests/CMUXCLIErrorOutputRegressionTests.swiftcmuxTests/WorkspaceTasksTests.swift
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 `@CLI/cmux.swift`:
- Around line 3855-3865: The set-font command parsing is too permissive and
could misparse arguments when flags are positioned early or ignore extra
positional arguments and unknown flags. Replace the simple first(where:) logic
that finds the first non-flag token as sizeArg with a proper argument parser
that walks through the entire rest array once, explicitly skips recognized
option values for --surface and --workspace, rejects any unknown flags with a
CLIError, and ensures exactly one positional size argument is provided before
the optional flags.
In `@Sources/TerminalController.swift`:
- Around line 12928-12962: The error messages in the v2MobileTerminalSetFont
function ("Missing or invalid font_size" and "font_size must be a positive
number of points") are plain string literals but must be localized since they
are user-facing. Replace both error message strings with the
String(localized:defaultValue:) pattern, using descriptive keys like
"error.terminal.font_size.invalid_params" and
"error.terminal.font_size.invalid_value", then add corresponding entries to the
Resources/Localizable.xcstrings file with translations for all supported
locales.
🪄 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: 174652b0-bbfc-4ac0-a0e2-f40f11d47538
📒 Files selected for processing (12)
CLI/cmux.swiftPackages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileTerminalSetFontEvent.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/GhosttySurfaceRepresentable.swiftPackages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Wire/ControlCommandExecutionPolicy.swiftPackages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandExecutionPolicyTests.swiftResources/Localizable.xcstringsSources/TerminalController.swiftcmuxTests/RightSidebarRemoteCommandTests.swiftcmuxTests/TerminalControllerSocketSecurityTests.swiftios/cmuxPackage/Tests/cmuxFeatureTests/cmuxFeatureTests.swift
Comment on lines
12925
to
12971
| } | ||
| } | ||
|
|
||
| /// Publish a `terminal.set_font` event to connected iOS device(s) so the | ||
| /// mirrored terminal live-zooms its font. This drives the same iOS apply path | ||
| /// as a pinch/zoom step, but originates from Mac automation | ||
| /// (`cmux mobile set-font <size>`). | ||
| nonisolated func v2MobileTerminalSetFont(params: [String: Any]) -> V2CallResult { | ||
| guard let fontSize = v2Double(params, "font_size") else { | ||
| return .err( | ||
| code: "invalid_params", | ||
| message: String( | ||
| localized: "socket.mobile.setFont.error.missingOrInvalidFontSize", | ||
| defaultValue: "Missing or invalid font_size" | ||
| ), | ||
| data: nil | ||
| ) | ||
| } | ||
| guard fontSize.isFinite, fontSize > 0 else { | ||
| return .err( | ||
| code: "invalid_params", | ||
| message: String( | ||
| localized: "socket.mobile.setFont.error.positiveFontSizeRequired", | ||
| defaultValue: "font_size must be a positive number of points" | ||
| ), | ||
| data: ["font_size": fontSize] | ||
| ) | ||
| } | ||
| var payload: [String: Any] = ["font_size": fontSize] | ||
| if let surfaceID = v2RawString(params, "surface_id") { | ||
| payload["surface_id"] = surfaceID | ||
| } | ||
| if let workspaceID = v2RawString(params, "workspace_id") { | ||
| payload["workspace_id"] = workspaceID | ||
| } | ||
| let hasSubscribers = MobileHostService.hasEventSubscribers(topic: "terminal.set_font") | ||
| MobileHostService.emitEvent(topic: "terminal.set_font", payload: payload) | ||
| return .ok([ | ||
| "ok": true, | ||
| "font_size": fontSize, | ||
| "delivered": hasSubscribers, | ||
| ]) | ||
| } | ||
|
|
||
| /// Mobile-gated wrapper over ``v2WorkspaceAction(params:)``. | ||
| func v2MobileWorkspaceAction(params: [String: Any]) -> V2CallResult { | ||
| let rawAction = v2RawString(params, "action") |
Contributor
There was a problem hiding this comment.
surface_id and workspace_id are read with v2RawString instead of v2UUID. Every other command in this file that accepts these params calls v2UUID, which returns invalid_params for malformed values. Here, any non-null string — including garbage like "not-a-uuid" — is forwarded verbatim in the terminal.set_font push payload, and the response still returns {"ok": true, "delivered": true}. The iOS subscriber receives the bad identifier, can't match any real surface/workspace, and silently mis-scopes (or drops) the font change. The caller has no way to detect the scope failure from the response.
Comment on lines
+151
to
+163
| message: String(localized: "socket.workspaceTasks.openFailed", defaultValue: "Workspace Tasks surface could not be opened"), | ||
| data: nil | ||
| ) | ||
| } | ||
| return .ok(v2WorkspaceTasksPayload(workspace: workspace, tabManager: tabManager, changedSurfaceId: panel.id)) | ||
| } | ||
| } | ||
|
|
||
| nonisolated private func v2WorkspaceTasksCommand( | ||
| params: [String: Any], | ||
| body: @MainActor (_ workspace: Workspace, _ tabManager: TabManager) -> V2CallResult | ||
| ) -> V2CallResult { | ||
| for key in ["workspace_id", "surface_id", "terminal_id", "tab_id", "pane_id", "window_id"] { |
Contributor
There was a problem hiding this comment.
When moveWorkspaceTask returns nil because before_task_id or after_task_id was not found in the task's bucket, the handler returns {"code":"not_found","data":{"task_id":"<SUBJECT>"}}. The task_id in the data refers to the subject task — which IS present — so the error falsely reports that task as missing. A caller that retries on not_found will loop indefinitely with a valid task ID. The add command avoids this by calling v2WorkspaceTasksAnchorErrorData(beforeTaskId:afterTaskId:) to surface the actual anchor ID. The move handler should do the same: check whether the subject task exists first (if it doesn't, return not_found with the task ID), and if the anchor lookup then fails, return not_found with the anchor ID and a distinct message.
# Conflicts: # CLI/cmux.swift # Packages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileTerminalSetFontEvent.swift # Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift # Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/GhosttySurfaceRepresentable.swift # Packages/iOS/CmuxMobileTerminal/Sources/CmuxMobileTerminal/GhosttySurfaceView.swift # Packages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Wire/ControlCommandExecutionPolicy.swift # Resources/Localizable.xcstrings # Sources/TerminalController.swift
# Conflicts: # .github/swift-file-length-budget.tsv
# Conflicts: # .github/swift-file-length-budget.tsv # Sources/SidebarWorkspaceGroupHeaderView.swift
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
Testing
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Adds beta Workspace Tasks with a native panel and popover, per‑workspace persistence, sidebar status, configurable hover controls, and safer open/index handling that focuses the tasks panel and switches to the right workspace. Also restores and hardens the mobile terminal set‑font path with typed payloads, scoped updates, localized errors, and live font streaming that survives reconnects and output‑tracking resets.
New Features
workspaceTaskswith a restyled panel and popover (header counts, accent stripe, drag handles); sidebar rows show task status; session restore + global search; hover add and between‑task insert dividers; row actions for archive/unarchive/remove/move; open viapanelType=workspacetasks(splits supported); rejects stale pane targets; clamps insert/move indices; beta‑gated; opening from the popover oropenfocuses the tasks panel and switches to the target workspace.cmux workspace tasks <list|add|archive|unarchive|remove|move|open>andworkspace.tasks.{list,add,archive,unarchive,remove,move,open}; resolves workspace refs; validates required flags and rejects conflicting workspace targets; bounded inputs (280‑char titles; max 200 open/200 archived; empty titles rejected;moverequires a destination; index params clamped); execution policy allowlist + tests; localized help/errors;openfocuses the target workspace.sidebar.workspaceControls(max 3, deduped;closealways preserved); the Tasks control shows whenever the Workspace Tasks beta is enabled, and the Workspace Controls beta unlocks chooser UI; settings UI, settings file mapping, andweb/data/cmux.schema.jsonupdated.cmux mobile set-font <points> [--surface <id>] [--workspace <id>]andmobile.terminal.set_font; Mac emitsterminal.set_font; iOS decodes typed payloads and applies per‑surface or per‑workspace; validates scope flags and positive font size with localized errors; live font streams persist across reconnects and terminal output‑tracking resets.Migration
cmux workspace tasks list,cmux workspace tasks add "Title",cmux workspace tasks unarchive <id>,cmux workspace tasks open. From the sidebar Tasks popover, click “Open as tab” to open and focus the tasks panel. For iOS live‑resize: pair a device, then runcmux mobile set-font 12(optionally add--surfaceor--workspace).Written for commit f235782. Summary will update on new commits.
Summary by CodeRabbit
Release Notes
cmux mobile set-fontto update live terminal font size (optionally scoped to a surface/workspace).cmux workspace taskssubcommands with updated help/usage.workspaceControlssidebar configuration.