Repository navigation
Harden workspace-todo CLI targeting, panel moves, caps, and shortcut defaults - #7748
Closed
azooz2003-bit wants to merge 1 commit into
Closed
azooz2003-bit wants to merge 1 commit into
azooz2003-bit wants to merge 1 commit into
Conversation
Blank or valueless --workspace/--window selectors on todo and status commands now fail with usage errors instead of silently targeting the selected workspace; workspace-todo panes refuse cross-workspace and Dock transfers (the panel binds its owning workspace at creation); workspace.todo.set rejects over-cap arrays before parsing on the main actor, sharing the app-side error shape; the new status shortcuts default to ctrl+cmd combos instead of colliding with the macOS spelling shortcuts; toggleChecklistItemComplete is marked non-chordable (its matcher is view-local); expired status overrides now reconcile at the agent-lifecycle and PR/git cache write funnels, closing the pin-revival window; the sidebar checklist section renders independently of the status glyph so None hides only the status. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR adds CLI validation for workspace/window selector options, enforces a checklist item cap in the control socket coordinator with a new error helper and test, restricts cross-workspace transfer of workspace-todo panels across Dock/tab move and detached-surface attach paths, adds task-status reconciliation after lifecycle/metadata updates, and updates default keyboard shortcuts (adding Control modifier) with matching docs and web data. ChangesWorkspace Todo Feature Updates
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ControlCommandCoordinator
participant WorkspaceChecklistItem
Client->>ControlCommandCoordinator: workspace.todo.set(items)
ControlCommandCoordinator->>WorkspaceChecklistItem: compare items.count to maxChecklistItems
alt over cap
ControlCommandCoordinator->>ControlCommandCoordinator: build invalid_params error via workspaceTodoSetTooManyItemsError
ControlCommandCoordinator-->>Client: return invalid_params error
else within cap
ControlCommandCoordinator->>ControlCommandCoordinator: parse and apply items
ControlCommandCoordinator-->>Client: return success
end
Possibly related issues
Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (23 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 hardens workspace-todo targeting and movement behavior. The main changes are:
Confidence Score: 5/5This looks safe to merge after checking the Dock attach edge case.
Sources/DockSplitStore+SurfaceTransfer.swift Important Files Changed
Reviews (1): Last reviewed commit: "Harden todo CLI targeting, panel moves, ..." | Re-trigger Greptile |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/DockSplitStore`+SurfaceTransfer.swift:
- Around line 210-211: The workspace-transfer eligibility rule for workspaceTodo
is duplicated across DockSplitStore, AppDelegate, and Workspace. Move the shared
base check onto a common symbol such as PanelType or Panel (for example a
transferability property/method), then update DockSplitStore+SurfaceTransfer,
AppDelegate.canTransferSurfaceAcrossWorkspaceBoundary(panel:), and
Workspace.attachDetachedSurface to delegate to that single definition, keeping
Workspace’s same-workspace exception separate if needed.
🪄 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: 63e6011a-e91f-4dae-a3db-d74a6c768ff6
📒 Files selected for processing (15)
CLI/CMUXCLI+WorkspaceTodo.swiftPackages/macOS/CmuxControlSocket/Package.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/WorkspaceTodo/ControlCommandCoordinator+WorkspaceTodoSetOpen.swiftPackages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandCoordinatorWorkspaceTodoSetOpenTests.swiftPackages/macOS/CmuxSettings/Sources/CmuxSettings/Values/ShortcutAction+Defaults.swiftPackages/macOS/CmuxSettings/Sources/CmuxSettings/Values/ShortcutAction.swiftSources/AppDelegate+DockSurfaceMove.swiftSources/AppDelegate+MoveTabToNewWorkspace.swiftSources/ContentView.swiftSources/DockSplitStore+SurfaceTransfer.swiftSources/KeyboardShortcutSettings.swiftSources/Workspace+PanelLifecycle.swiftSources/Workspace.swiftdocs/configuration.mdweb/data/cmux-shortcuts.ts
Comment on lines
210
to
+211
| guard bonsplitController.allPaneIds.contains(paneId), panels[detached.panelId] == nil else { return nil } | ||
| guard detached.panel.panelType != .workspaceTodo else { return nil } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Duplicated workspaceTodo eligibility check — consider centralizing.
This inline detached.panel.panelType != .workspaceTodo re-implements the same rule as AppDelegate.canTransferSurfaceAcrossWorkspaceBoundary(panel:) (Sources/AppDelegate+MoveTabToNewWorkspace.swift:15-17), and Workspace.attachDetachedSurface (Sources/Workspace.swift:9407-9409) has yet another inline copy with a same-workspace exception. Since DockSplitStore/Workspace can't call the AppDelegate extension method directly, the rule ends up duplicated three times. If the eligibility rule ever changes (e.g. another panel type becomes non-transferable), it's easy to update one site and miss the others.
Consider moving the base rule onto PanelType/Panel (e.g. panel.isTransferableAcrossWorkspaces) so all three call sites share one definition, with AppDelegate.canTransferSurfaceAcrossWorkspaceBoundary and Workspace's same-workspace exception both delegating to it.
🤖 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/DockSplitStore`+SurfaceTransfer.swift around lines 210 - 211, The
workspace-transfer eligibility rule for workspaceTodo is duplicated across
DockSplitStore, AppDelegate, and Workspace. Move the shared base check onto a
common symbol such as PanelType or Panel (for example a transferability
property/method), then update DockSplitStore+SurfaceTransfer,
AppDelegate.canTransferSurfaceAcrossWorkspaceBoundary(panel:), and
Workspace.attachDetachedSurface to delegate to that single definition, keeping
Workspace’s same-workspace exception separate if needed.
lawrencecchen
added
the
stale-revisit
This branch was previously 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.
Review-hardening for the workspaces-as-todos feature merged in #7216 — these fixes were driven by the structured review loop and pushed while that PR was being merged, so they missed the squash.
Blank or valueless
--workspace/--windowselectors oncmux todoandcmux workspace statuscommands now fail with usage errors instead of silently targeting the selected workspace (destructive commands could mistarget). Workspace-todo panes refuse cross-workspace and Dock transfers: the panel binds its owning workspace at creation, and moving it previously left it displaying and mutating the source workspace's checklist.workspace.todo.setrejects over-cap arrays before parsing on the main actor, sharing the app-side error contract. The new status shortcut defaults move from Cmd+;/Cmd+Shift+; (the standard macOS spelling shortcuts, which they hijacked globally) to Ctrl+Cmd combos.toggleChecklistItemCompleteis marked non-chordable since its matcher is view-local. Expired manual status overrides now reconcile at the agent-lifecycle and PR/git cache write funnels, closing the pin-revival window (first item of #7744). The sidebar checklist section renders independently of the status glyph, so "None (hide status)" no longer hides existing checklist items.Verification: tagged build green;
swift testgreen in CmuxControlSocket (226 tests, incl. new pre-cap coverage) and CmuxWorkspaces; file-length gate green vs main.🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Changes surface-move eligibility, workspace status override timing, and default keyboard bindings; mistakes could block legitimate moves or surprise users on upgrade, but scope is localized to workspace-todo flows.
Overview
Follow-up hardening for workspace todos: CLI and control socket now reject empty
--workspace/--windowvalues oncmux todoandcmux workspace statusinstead of falling back to the ambient workspace.workspace.todo.setenforces the sharedWorkspaceChecklistItem.maxChecklistItemscap before item parsing (with tests), via a new CmuxWorkspaces dependency.Workspace-todo panels are treated as non-transferable:
canTransferSurfaceAcrossWorkspaceBoundaryblocks moves to other workspaces, new workspaces, and the Dock; attach paths only allow todo panels whensourceWorkspaceIdmatches the destination workspace.Shortcuts and docs move mark-done / cycle-status defaults to Ctrl+Cmd+; and Ctrl+Cmd+Shift+; to avoid macOS spelling shortcuts;
toggleChecklistItemCompleteis excluded from chord binding. Sidebar shows the checklist section when items exist (or add-field is active), not only when a status glyph is shown.Status overrides call
reconcileExpiredTaskStatusOverride()when agent lifecycle, git branch, or PR metadata is updated, so manual pins clear when inference changes.Reviewed by Cursor Bugbot for commit a195541. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Hardens workspace-todo targeting and movement, enforces checklist caps, and updates status shortcut defaults to avoid macOS conflicts. Prevents mistargeted CLI actions, cross-workspace todo pane moves, and stale status pins while keeping the checklist visible when status is hidden.
Bug Fixes
cmux todoandcmux workspace statusnow error on blank--workspace/--windowvalues instead of defaulting.workspace.todo.setrejects over-cap item arrays up front with a consistent error; tests added.markWorkspaceDoneandcycleWorkspaceStatusmoved to Ctrl+Cmd combos;toggleChecklistItemCompleteis non-chordable.Dependencies
CmuxWorkspacestoCmuxControlSockettargets and tests.Written for commit a195541. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes