Repository navigation
Add workspace pages - #4766
Closed
austinywang wants to merge 51 commits into
Closed
Add workspace pages#4766austinywang wants to merge 51 commits into
austinywang wants to merge 51 commits into
Conversation
|
Complex PR? Review this PR in Change Stack to move by importance, not file order. 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 workspace pages end-to-end: multi-page model and snapshots, full V2 page API and CLI commands, titlebar page-strip UI, menu/shortcut wiring, localization (EN/JA), tests, and a spec document. ChangesWorkspace Page Model and Persistence
V2 API and CLI Surfaces
Keyboard Shortcuts, UI, and Localization
Testing and Documentation
Sequence Diagram (high-level page flow): sequenceDiagram
participant CLI as CLI/cmux
participant Socket as V2 Socket
participant TerminalController as TerminalController
participant Workspace as Workspace
CLI->>Socket: send page.* v2 request (e.g. page.create)
Socket->>TerminalController: processV2Command
TerminalController->>Workspace: perform page lifecycle operation
Workspace-->>TerminalController: updated page state / snapshot
TerminalController-->>Socket: v2 result (ok/result)
Socket-->>CLI: prints v2 result / summary
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (3 errors, 1 warning)
✅ Passed checks (15 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…rface-tab-to-have-splits
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
Greptile SummaryThis PR implements workspace pages — a first-class layout container that lets each workspace hold multiple independent split/pane/surface layouts with fast runtime switching and full session persistence. The change is large (~3,300 net new lines in production Swift + 671 lines of tests) but well-scoped:
Confidence Score: 5/5Safe to merge — page lifecycle, runtime/session capture-restore, actor-isolation boundaries, and closed-panel-history suppression are all correctly handled; previous thread fixes are present in the diff. The feature is large but follows the established v2 socket pattern precisely. Page selection is fully main-thread-gated via v2MainSync, panel detach/reattach is wrapped in withClosedPanelHistorySuppressed, session migration is backward-compatible (optional fields), and all new user-facing strings have full translations. The one finding (missing _ref fields on two reorder anchor errors) is an API cosmetic inconsistency that does not affect correctness or user data. Sources/TerminalController.swift — the v2PageReorder anchor-not-found error responses are missing the _ref companion fields present in every other page-API error. Important Files Changed
Sequence DiagramsequenceDiagram
participant CLI as cmux CLI / socket client
participant TC as TerminalController (nonisolated)
participant Main as MainActor
participant WS as Workspace
CLI->>TC: "page.select {page_id}"
TC->>Main: "v2MainSync { v2LocatePage }"
Main-->>TC: (windowId, tabManager, workspace, pageIndex)
TC->>Main: workspace.selectPage(pageId)
Main->>WS: captureActivePageStoredState(detachPanels:true)
WS-->>Main: StoredPageState (runtime+session)
Main->>WS: "storedPageStates[activePageId] = state"
Main->>WS: restoreStoredPage(pageId)
WS->>WS: restoreRuntimePageState OR restoreSessionPageState
Main->>WS: "activePageId = pageId"
TC-->>CLI: "ok {page_id, page_ref, page_index, page_title}"
CLI->>TC: "surface.list {page_id, workspace_id}"
TC->>TC: v2ActivateRequestedPageContext(params)
TC->>Main: "v2MainSync { locate + selectPage if needed }"
Main-->>TC: nil (no error)
TC->>TC: body() → v2SurfaceList(params)
TC->>Main: "v2MainSync { v2ResolveWorkspace }"
Main-->>TC: surfaces on now-active page
TC-->>CLI: "ok {surfaces: [...]}"
Reviews (12): Last reviewed commit: "Fix page reorder workspace isolation" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/workspace-pages-spec.md (1)
451-451:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winIncomplete line at end of document.
Line 451 appears to end abruptly with just the line number. Please complete this section or remove the orphaned line.
🤖 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 `@docs/workspace-pages-spec.md` at line 451, The document ends with an incomplete/orphaned line at line 451; either remove that stray line or complete the trailing section content so the document ends cleanly—locate the end of the last section (the paragraph or header preceding line 451) and either finish the sentence/section or delete the lone line to prevent the abrupt end.
🤖 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/cmuxApp.swift`:
- Around line 970-989: The pageSelectionMenuShortcut implementation currently
returns shortcuts via KeyboardShortcutSettings.shortcut(for:), which can bypass
menu normalization; update pageSelectionMenuShortcut to use the menu-normalized
resolver on KeyboardShortcutSettings instead (replace the final return that maps
action to KeyboardShortcutSettings.shortcut(for:) with the menu-specific
resolver, e.g. the KeyboardShortcutSettings method that provides menu-normalized
shortcuts) so Select Page items use the same menu key-equivalent normalization
as the rest of the file.
In `@Sources/ContentView.swift`:
- Around line 2339-2406: The ForEach is passing Workspace into
titlebarPageButton causing row views to capture mutable ObservableObject state;
instead create an immutable per-row snapshot and action closures in
titlebarPageStrip (e.g. let row = (id: page.id, title: page.title, isActive:
page.id == workspace.activePageId, canClose: workspace.canClosePage(page.id),
pageCount: workspace.pages.count) and closures newPage/ selectPage/
promptRenamePage/ duplicatePage/ closePage/ closeOtherPages) and change
titlebarPageButton signature to accept only these immutable values plus the
action closures (e.g. selectAction, renameAction, duplicateAction, closeAction,
closeOthersAction) so titlebarPageButton no longer references Workspace or calls
its methods directly.
In `@tests_v2/test_page_cli_socket_parity.py`:
- Around line 176-252: The cached page refs (first_page_ref, second_page_ref,
duplicate_page_ref) are stale after calling c._call("page.reorder", ...); update
the refs before reuse by re-reading current refs from page.list or page.current
(e.g. call _run_cli_json(["list-pages","--workspace", workspace_id]) or
c._call("page.current"/"page.list") and extract fresh refs) and then use those
refreshed refs in subsequent _run_cli_json calls (reorder-page --after and
close-page) or switch to using stable IDs (first_page_id/second_page_id) like
second_page_id to avoid wrong-target CLI operations.
- Line 16: The test uses SOCKET_PATH created from the wrong env var name
("CMUX_SOCKET"), causing silent fallback; update the env lookup in SOCKET_PATH
to use "CMUX_SOCKET_PATH" instead (keep the same default "/tmp/cmux-debug.sock")
so the tests_v2 runner's injected CMUX_SOCKET_PATH is respected; locate the
SOCKET_PATH assignment and replace the env key accordingly.
---
Outside diff comments:
In `@docs/workspace-pages-spec.md`:
- Line 451: The document ends with an incomplete/orphaned line at line 451;
either remove that stray line or complete the trailing section content so the
document ends cleanly—locate the end of the last section (the paragraph or
header preceding line 451) and either finish the sentence/section or delete the
lone line to prevent the abrupt end.
🪄 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: ab494aa8-50cf-42bd-a9d0-cdbcfec36bb6
📒 Files selected for processing (16)
CLI/cmux.swiftResources/Localizable.xcstringsSources/App/TerminalDirectoryOpenSupport.swiftSources/AppDelegate.swiftSources/ContentView.swiftSources/KeyboardShortcutSettings.swiftSources/SessionPersistence.swiftSources/TerminalController.swiftSources/Workspace.swiftSources/cmuxApp.swiftcmuxTests/AppDelegateShortcutRoutingTests.swiftcmuxTests/SessionPersistenceTests.swiftcmuxTests/WorkspaceContentViewVisibilityTests.swiftcmuxTests/WorkspaceUnitTests.swiftdocs/workspace-pages-spec.mdtests_v2/test_page_cli_socket_parity.py
…rface-tab-to-have-splits # Conflicts: # Sources/TerminalController.swift # Sources/Workspace.swift
Contributor
Author
|
Addressed the Greptile maintenance observations in d7b7e39: removed the redundant v2PageDuplicate source-page guard with the misleading impossible-state payload, and added a concise note at StoredPageState.RuntimeState documenting the capture/restore boundary for future runtime-only page state. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmuxTests/WorkspaceContentViewVisibilityTests.swift (1)
161-247: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winMigrate this new suite to Swift Testing.
WorkspacePageLifecycleTestsis a brand new test suite. Per coding guidelines, new unit and integration tests should default to Swift Testing rather than XCTest.♻️ Refactor to Swift Testing
+import Testing + -@MainActor -final class WorkspacePageLifecycleTests: XCTestCase { - func testSwitchingPagesPreservesLivePanelIdentityAcrossDetachAndReattach() throws { +@Suite(.serialized) +@MainActor +struct WorkspacePageLifecycleTests { + `@Test` func switchingPagesPreservesLivePanelIdentityAcrossDetachAndReattach() throws { let workspace = Workspace() let firstPageId = workspace.activePageId - let firstPaneId = try XCTUnwrap(workspace.bonsplitController.allPaneIds.first) + let firstPaneId = try `#require`(workspace.bonsplitController.allPaneIds.first) - XCTAssertNotNil(workspace.newTerminalSurface(inPane: firstPaneId, focus: false)) + `#expect`(workspace.newTerminalSurface(inPane: firstPaneId, focus: false) != nil) let firstPagePanelIds = Set(workspace.panels.keys) - XCTAssertEqual(firstPagePanelIds.count, 2) + `#expect`(firstPagePanelIds.count == 2) let secondPage = workspace.newPage(select: true) - XCTAssertEqual(workspace.activePageId, secondPage.id) + `#expect`(workspace.activePageId == secondPage.id) let secondPagePanelIds = Set(workspace.panels.keys) - XCTAssertEqual( - secondPagePanelIds.count, - 1, - "A fresh page should mount its own placeholder terminal" - ) - XCTAssertNotEqual(firstPagePanelIds, secondPagePanelIds) + `#expect`(secondPagePanelIds.count == 1, "A fresh page should mount its own placeholder terminal") + `#expect`(firstPagePanelIds != secondPagePanelIds) workspace.selectPage(firstPageId) - XCTAssertEqual(workspace.activePageId, firstPageId) - XCTAssertEqual( - Set(workspace.panels.keys), - firstPagePanelIds, - "Returning to the first page should reattach the parked live panels" - ) + `#expect`(workspace.activePageId == firstPageId) + `#expect`(Set(workspace.panels.keys) == firstPagePanelIds, "Returning to the first page should reattach the parked live panels") workspace.selectPage(secondPage.id) - XCTAssertEqual(workspace.activePageId, secondPage.id) - XCTAssertEqual( - Set(workspace.panels.keys), - secondPagePanelIds, - "Returning to the second page should reuse its parked live panel instead of rebuilding a new one" - ) + `#expect`(workspace.activePageId == secondPage.id) + `#expect`(Set(workspace.panels.keys) == secondPagePanelIds, "Returning to the second page should reuse its parked live panel instead of rebuilding a new one") } - func testRuntimePageRestoreReplacesPreviousPagePaneSkeleton() throws { + `@Test` func runtimePageRestoreReplacesPreviousPagePaneSkeleton() throws { let workspace = Workspace() let firstPageId = workspace.activePageId - let firstPanelId = try XCTUnwrap(workspace.focusedPanelId) - XCTAssertNotNil(workspace.newTerminalSplit(from: firstPanelId, orientation: .horizontal)) + let firstPanelId = try `#require`(workspace.focusedPanelId) + `#expect`(workspace.newTerminalSplit(from: firstPanelId, orientation: .horizontal) != nil) let firstPagePanelIds = Set(workspace.panels.keys) let firstPagePaneCount = workspace.bonsplitController.allPaneIds.count - XCTAssertEqual(firstPagePaneCount, 2) + `#expect`(firstPagePaneCount == 2) let secondPage = workspace.newPage(select: true) - let secondPanelId = try XCTUnwrap(workspace.focusedPanelId) - XCTAssertNotNil(workspace.newTerminalSplit(from: secondPanelId, orientation: .vertical)) - XCTAssertEqual(workspace.bonsplitController.allPaneIds.count, 2) + let secondPanelId = try `#require`(workspace.focusedPanelId) + `#expect`(workspace.newTerminalSplit(from: secondPanelId, orientation: .vertical) != nil) + `#expect`(workspace.bonsplitController.allPaneIds.count == 2) workspace.selectPage(firstPageId) - XCTAssertEqual(workspace.activePageId, firstPageId) - XCTAssertEqual(Set(workspace.panels.keys), firstPagePanelIds) - XCTAssertEqual( - workspace.bonsplitController.allPaneIds.count, - firstPagePaneCount, - "Runtime page restore should replace the leaving page's empty pane skeleton" - ) - XCTAssertNotEqual(workspace.activePageId, secondPage.id) + `#expect`(workspace.activePageId == firstPageId) + `#expect`(Set(workspace.panels.keys) == firstPagePanelIds) + `#expect`(workspace.bonsplitController.allPaneIds.count == firstPagePaneCount, "Runtime page restore should replace the leaving page's empty pane skeleton") + `#expect`(workspace.activePageId != secondPage.id) } - func testRuntimePageRestoreDoesNotRecordPlaceholderPanelsAsClosedItems() throws { + `@Test` func runtimePageRestoreDoesNotRecordPlaceholderPanelsAsClosedItems() throws { ClosedItemHistoryStore.shared.removeAll() defer { ClosedItemHistoryStore.shared.removeAll() } let workspace = Workspace() let firstPageId = workspace.activePageId - let firstPanelId = try XCTUnwrap(workspace.focusedPanelId) - XCTAssertNotNil(workspace.newTerminalSplit(from: firstPanelId, orientation: .horizontal)) + let firstPanelId = try `#require`(workspace.focusedPanelId) + `#expect`(workspace.newTerminalSplit(from: firstPanelId, orientation: .horizontal) != nil) let secondPage = workspace.newPage(select: true) - let secondPanelId = try XCTUnwrap(workspace.focusedPanelId) - XCTAssertNotNil(workspace.newTerminalSplit(from: secondPanelId, orientation: .vertical)) + let secondPanelId = try `#require`(workspace.focusedPanelId) + `#expect`(workspace.newTerminalSplit(from: secondPanelId, orientation: .vertical) != nil) workspace.selectPage(firstPageId) workspace.selectPage(secondPage.id) - XCTAssertFalse( - ClosedItemHistoryStore.shared.canReopen, - "Runtime page restore should not expose synthetic placeholder panels in recently closed items" - ) + `#expect`(!ClosedItemHistoryStore.shared.canReopen, "Runtime page restore should not expose synthetic placeholder panels in recently closed items") } }As per coding guidelines: "Swift Testing is the current Apple-supported primitive for tests on this codebase... Default to Swift Testing for all unit and integration tests."
🤖 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 `@cmuxTests/WorkspaceContentViewVisibilityTests.swift` around lines 161 - 247, The new test suite WorkspacePageLifecycleTests is written with XCTest; migrate it to Swift Testing by converting the `@MainActor` final class WorkspacePageLifecycleTests: XCTestCase into the Swift Testing suite style (create the Swift Test suite type and register tests for testSwitchingPagesPreservesLivePanelIdentityAcrossDetachAndReattach, testRuntimePageRestoreReplacesPreviousPagePaneSkeleton, and testRuntimePageRestoreDoesNotRecordPlaceholderPanelsAsClosedItems), replace XCTest assertions (XCTUnwrap, XCTAssertEqual, XCTAssertNotNil, XCTAssertFalse, etc.) with the Swift Testing assertion APIs, preserve the setup/teardown behavior (ClosedItemHistoryStore.shared.removeAll() and defer cleanup) and keep references to workspace, bonsplitController, panels, newTerminalSurface/newTerminalSplit, selectPage, activePageId, focusedPanelId so the logic and intent remain identical while using Swift Testing idioms.Source: Coding guidelines
🤖 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 15190-15259: The parsing helpers currently re-parse tokens after a
"--" terminator; update parsing to split args at the first "--" and only inspect
the pre-terminator slice. Concretely: when implementing page command handling,
compute let pre = args.split(whereSeparator: { $0 == "--" }).first ?? args and
pass that pre slice into parsePageCommandContextOptions,
firstPositionalPageHandle, splitOptionalPositionalPageHandle and any call sites
that call positionalArgsAfterOptionalTerminator so option/handle detection
(workspace/window/page) only looks at pre; leave the post-terminator tail
untouched and forwarded as title/remaining text. Ensure
parsePageCommandContextOptions uses only the pre slice for parseOption and that
positionalArgsAfterOptionalTerminator no longer treats the full args array as
mutable input.
In `@Sources/Workspace.swift`:
- Around line 12025-12046: The code chooses entry.snapshot.selectedPanelId if it
exists in desiredPanelIds even when attachDetachedSurface failed, which leads to
no selected tab; change the selection logic to prefer a selectedPanelId only if
it is present in attachedPanelIds (i.e. actually attached) rather than
desiredPanelIds. Concretely, update the computation that sets selectedPanelId
(currently using desiredPanelIds.contains) to check attachedPanelIds.contains
(or map the original selected id to an attachedPanelId), then proceed to call
surfaceIdFromPanelId(selectedPanelId) and bonsplitController.selectTab only when
that selectedPanelId is one of attachedPanelIds so the pane always selects a
valid attached tab.
---
Outside diff comments:
In `@cmuxTests/WorkspaceContentViewVisibilityTests.swift`:
- Around line 161-247: The new test suite WorkspacePageLifecycleTests is written
with XCTest; migrate it to Swift Testing by converting the `@MainActor` final
class WorkspacePageLifecycleTests: XCTestCase into the Swift Testing suite style
(create the Swift Test suite type and register tests for
testSwitchingPagesPreservesLivePanelIdentityAcrossDetachAndReattach,
testRuntimePageRestoreReplacesPreviousPagePaneSkeleton, and
testRuntimePageRestoreDoesNotRecordPlaceholderPanelsAsClosedItems), replace
XCTest assertions (XCTUnwrap, XCTAssertEqual, XCTAssertNotNil, XCTAssertFalse,
etc.) with the Swift Testing assertion APIs, preserve the setup/teardown
behavior (ClosedItemHistoryStore.shared.removeAll() and defer cleanup) and keep
references to workspace, bonsplitController, panels,
newTerminalSurface/newTerminalSplit, selectPage, activePageId, focusedPanelId so
the logic and intent remain identical while using Swift Testing idioms.
🪄 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: cf1f4e97-a0dd-40bc-99f0-46b536a3a88e
📒 Files selected for processing (7)
CLI/cmux.swiftResources/Localizable.xcstringsSources/TerminalController.swiftSources/Workspace.swiftcmuxTests/WorkspaceContentViewVisibilityTests.swiftdocs/workspace-pages-spec.mdtests_v2/test_page_cli_socket_parity.py
Contributor
Author
|
Addressed the current CodeRabbit findings in the latest push: migrated the new WorkspacePageLifecycleTests suite to Swift Testing, added a regression seam/test for runtime page restore selection fallback, changed restore to select only an actually attached panel, and fixed page CLI parsing so options after -- remain title/positional text instead of being reparsed. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
CLI/cmux.swift (1)
4316-4324:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
--still is not a hard stop for page-command parsing.Line 4316 still feeds the full
commandArgsarray intoparsePageCommandContextOptions, and this helper only treats--specially when it is the first remaining token. Socmux rename-page page:1 -- --window draftcan still have--window draftconsumed as routing instead of title text. Split once at the first--, run all page/workspace/window parsing on the pre-terminator slice only, then forward the tail untouched into the title args.Based on learnings, pre-socket CLI parsing must stop at the first
--and forward everything after it untouched.Also applies to: 15252-15271
🤖 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 `@CLI/cmux.swift` around lines 4316 - 4324, The current parsing passes the entire commandArgs into parsePageCommandContextOptions so `--` is not enforced as a hard stop; change to split commandArgs at the first "--" (keep head = tokens before first "--", tail = tokens after the first "--" untouched), call parsePageCommandContextOptions(head, windowOverride: windowId), then run splitOptionalPositionalPageHandle on the remaining from that parse and finally construct title from the explicit tail plus any titleArgs (do not let parsing consume tokens from the tail). Apply the same pre-terminator split fix for the other occurrence range referenced (lines 15252-15271) using the same functions: parsePageCommandContextOptions and splitOptionalPositionalPageHandle so everything after the first "--" is forwarded as raw title text.Source: Learnings
🤖 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 `@tests_v2/test_page_cli_socket_parity.py`:
- Around line 268-315: The test exercises numeric-title parsing with
out-of-range values ("2024" and "5") so it never detects ambiguity with valid
1-based page handles; update the test cases (the calls to _run_cli_json in the
numeric_title / numeric_title_terminator / numeric_duplicate sections) to use at
least one in-range numeric title (e.g., "2") instead of an out-of-range value
before the subsequent rename to "editor" so the parser’s handling of a valid
page handle vs numeric title is exercised (modify the arguments passed to
["rename-page", ...] and ["duplicate-page", ...] where numeric_title,
numeric_title_terminator, and numeric_duplicate are created).
---
Duplicate comments:
In `@CLI/cmux.swift`:
- Around line 4316-4324: The current parsing passes the entire commandArgs into
parsePageCommandContextOptions so `--` is not enforced as a hard stop; change to
split commandArgs at the first "--" (keep head = tokens before first "--", tail
= tokens after the first "--" untouched), call
parsePageCommandContextOptions(head, windowOverride: windowId), then run
splitOptionalPositionalPageHandle on the remaining from that parse and finally
construct title from the explicit tail plus any titleArgs (do not let parsing
consume tokens from the tail). Apply the same pre-terminator split fix for the
other occurrence range referenced (lines 15252-15271) using the same functions:
parsePageCommandContextOptions and splitOptionalPositionalPageHandle so
everything after the first "--" is forwarded as raw title text.
🪄 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: 8d7cec7f-e1f4-4d2c-9a07-a300a29394d9
📒 Files selected for processing (2)
CLI/cmux.swifttests_v2/test_page_cli_socket_parity.py
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 9a84dda. Configure here.
| ) -> (page: String?, titleArgs: [String]) { | ||
| guard args.first != "--" else { | ||
| return (nil, args) | ||
| } |
There was a problem hiding this comment.
Post-terminator page handles ignored
Medium Severity
splitOptionalPositionalPageHandle bails out when the combined argument list starts with --, so tokens after the terminator are never treated as a page handle. duplicate-page and rename-page use this helper, while select-page, close-page, and reorder-page use firstPositionalPageHandle, which strips -- first. Invocations like duplicate-page -- page:2 or rename-page -- page:2 new title therefore target the current page and fold handle tokens into the title instead of the intended page.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 9a84dda. Configure here.
|
@austinywang The failing this feature would be an awesome addition! |
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
4 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
Closes #3297
Testing
Note
Low Risk
CLI-only additions mirroring existing workspace command patterns; no auth or persistence logic in this diff, though incorrect page targeting could close or reorder the wrong page in automation scripts.
Overview
Adds legacy top-level
cmuxverbs for workspace page lifecycle and navigation, each calling the v2 socketpage.*APIs with shared--workspace/--windowcontext resolution.New commands include
list-pages,new-page,duplicate-page,current-page,select-page,rename-page,close-page(defaults to current page,force: true),reorder-page, andnext-page/previous-page/last-page. Page targets accept UUIDs,page:Nrefs, or 1-based indexes via newnormalizePageHandle;isHandleRefnow recognizespage.cmux treeis updated to nest pages → panes → surfaces under workspaces (with active/selected markers) and falls back to workspace-level panes when no pages exist. Per-command help, the global usage blurb, and the command allowlist document the new verbs andpagehandle syntax.Reviewed by Cursor Bugbot for commit 9a84dda. Bugbot is set up for automated code reviews on this repo. Configure here.
Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Summary by cubic
Adds workspace pages so a workspace can hold multiple full layouts (splits, panes, surfaces) with fast switching. Adds page UI, page-aware menus/shortcuts, per-page session restore, CLI/socket v2 page APIs,
cmuxverbs, andcmux treepage output (closes #3297).New Features
page.*lifecycle (create,duplicate,rename[defaults to current; accepts positional handles and titles after--],select,close[current allowed],reorder,list,next,previous,last);cmuxaddslist-pages,current-page, and page handles (UUID/ref/1-based index). Pane/surface commands accept page refs and route via that page.activePageId; legacy workspaces migrate into a default page. Docs indocs/workspace-pages-spec.md.cmux treeprints pages under each workspace and marks the active page.Bug Fixes
close-pagedefaults to the current workspace and parses--workspacebefore page args; rename/close default to the current page; window-aware routing.--.Written for commit 9a84dda. Summary will update on new commits.
Summary by CodeRabbit
New Features
Documentation
Localization
Tests