Reopen workspace pages in the titlebar - #1039
Closed
lawrencecchen wants to merge 7 commits into
Closed
lawrencecchen wants to merge 7 commits into
lawrencecchen wants to merge 7 commits into
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:
📝 WalkthroughWalkthroughCentralizes CLI positional/trailing-text parsing; adds per-page titlebar width tracking and width-aware drag/drop; introduces a V2 workspace/page resolver and richer pane/surface payloads; changes new/duplicate page defaults to not auto-select; changes page-selection shortcuts to require Command; standardizes stored-shortcut decoding and updates tests/docs. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User (Drag)
participant UI as Titlebar UI
participant Drop as TitlebarPageDropDelegate
participant Width as Titlebar Width Tracker
participant Store as Workspace/Page Store
User->>UI: Start dragging page
UI->>Drop: dropEntered / dropUpdated
Drop->>Width: effectiveTargetWidth()
Width-->>Drop: target width (from titlebarPageWidths)
Drop->>Drop: compute indicator position (width-aware)
Note over Drop: debugIndicator logged
User->>UI: Drop completes
Drop->>Store: performDrop -> reorder pages
Store-->>UI: UI updates ordering
sequenceDiagram
participant Client as V2 Client
participant Controller as TerminalController
participant Resolver as v2ResolveWorkspaceForPage
participant TabMgr as TabManager
participant Workspace as Workspace
Client->>Controller: V2 page operation (rename/close/duplicate/reorder)
Controller->>Resolver: resolve workspace/page (params, pageId)
Resolver->>TabMgr: lookup workspace (scoped or default)
TabMgr-->>Resolver: tabManager or nil
alt workspace found
Resolver->>Workspace: locate page
Workspace-->>Resolver: page found / not found
Resolver-->>Controller: resolved(tabManager, workspace, page?)
else workspace not found
Resolver-->>Controller: workspaceNotFound
end
Controller->>Controller: build V2 payload (include surface_ids / surface_refs)
Controller-->>Client: reply with resolved payload or error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 11 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/TerminalController.swift">
<violation number="1" location="Sources/TerminalController.swift:4260">
P2: Explicit `workspace_id` scoping can be bypassed when `workspace_id` is malformed, causing page operations to run against the selected workspace.</violation>
</file>
<file name="Sources/Workspace.swift">
<violation number="1" location="Sources/Workspace.swift:1758">
P2: The strict detached-surface equality check can incorrectly reject valid runtime page state and fall back to capped session restore, dropping panels in large workspaces.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
Greptile SummaryThis PR completes the titlebar workspace-pages feature by addressing remaining review feedback from issue #569. It fixes keyboard shortcut defaults (all page-select shortcuts gain the Key changes and notable items:
Confidence Score: 3/5
Last reviewed commit: 22e85ed |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?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)
Sources/Workspace.swift (1)
1750-1762:⚠️ Potential issue | 🟠 MajorTear down detached runtime panels before snapshot fallback.
On Line 1751 this code removes the stored page state from
storedPageStates, but if the new parity check fails on Lines 1757-1758 the fallback path on Line 1761 restores fromsessionStatewithout closing the detached panels inruntimeState. Those panels are then orphaned permanently, which can leak Ghostty/WebKit resources and keep terminal processes alive.Proposed fix
private func restoreStoredPage(_ pageId: UUID) { let storedState = storedPageStates.removeValue(forKey: pageId) ?? StoredPageState( sessionState: emptyPageSessionStateSnapshot(currentDirectory: currentDirectory), runtimeState: nil ) let expectedPanelIds = Set(storedState.sessionState.panels.map(\.id)) - if let runtimeState = storedState.runtimeState, - Set(runtimeState.detachedSurfaces.keys) == expectedPanelIds { - restoreRuntimePageState(runtimeState) - } else { - restoreSessionPageState(storedState.sessionState) + if let runtimeState = storedState.runtimeState { + if Set(runtimeState.detachedSurfaces.keys) == expectedPanelIds { + restoreRuntimePageState(runtimeState) + return + } + + teardownStoredPageState(storedState) } + + restoreSessionPageState(storedState.sessionState) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 1750 - 1762, When restoring a stored page in restoreStoredPage you currently drop storedPageStates and, if the runtime/session parity check fails, fall back to restoreSessionPageState without cleaning up runtimeState.detachedSurfaces; iterate over storedState.runtimeState?.detachedSurfaces (if present) and explicitly tear down each detached surface before calling restoreSessionPageState — use the same teardown/remove API used elsewhere in the codebase (e.g. the method used by restoreRuntimePageState or any existing removeDetachedSurface/teardownSurface helper) so detached WebKit/terminal resources are closed instead of orphaned.
🧹 Nitpick comments (2)
Sources/cmuxApp.swift (1)
933-939: Consider extractingdecodeShortcutto a shared utility.This helper is duplicated identically in
ContentView.swift,NotificationsPage.swift, andWorkspaceContentView.swift. Extracting it to a shared location (e.g., aStoredShortcutextension or utility file) would reduce duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/cmuxApp.swift` around lines 933 - 939, The decodeShortcut(from:fallback:) helper is duplicated across cmuxApp.swift, ContentView.swift, NotificationsPage.swift, and WorkspaceContentView.swift; extract it into one shared location by moving its logic into a single utility (e.g., an extension on StoredShortcut or a new ShortcutsUtils.swift) and update callers to use StoredShortcut.decode(from:fallback:) or ShortcutsUtils.decodeShortcut(...) instead of the local private function; ensure the new shared method signature matches the original (accepts Data and a fallback StoredShortcut and returns StoredShortcut) and remove the duplicate private functions from the three files.Sources/ContentView.swift (1)
1535-1537: GateClose Pagefrom the active page policy, not just page count.
workspaceHasMultiplePagesis broader than the per-page close rule already used by the titlebar viaworkspace.canClosePage(page.id). A dedicatedpageCanCloseflag from the active page would keep the command palette aligned with the rest of the UI and avoid future drift if close eligibility changes again.Also applies to: 4488-4495, 4828-4836
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ContentView.swift` around lines 1535 - 1537, Replace the broad workspaceHasMultiplePages gate with a per-page flag derived from the active page: add a pageCanClose boolean (computed from workspace.canClosePage(activePage.id) or the equivalent active page identifier) and use that flag where the "Close Page" command palette entry is gated (references: workspaceHasMultiplePages -> replace with pageCanClose; use workspace.canClosePage(page.id) to compute it). Update all occurrences mentioned (the main block and the other ranges) so the command palette aligns with the titlebar's per-page close policy and avoid future drift.
🤖 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 1144-1149: When parsing commands like "new-page",
"duplicate-page", and "close-page" the code currently only validates leftover
args when options like title/page are absent (see variables titleOpt/pageOpt and
the call to trailingTextArgument(rem1)), which lets valueless or misspelled
flags remain in trailing and silently fallback; fix by validating trailing after
parseOption even when titleOpt/pageOpt is present: after parsing options
(variables titleOpt, pageOpt and arrays like trailing or rem1), ensure
trailing.isEmpty (or trailing.count <= 1 as appropriate) and throw an error if
extra tokens exist so commands like duplicate-page and close-page fail fast
instead of using defaults.
In `@Sources/ContentView.swift`:
- Around line 1320-1322: The titlebar hint monitor currently follows page
shortcut modifiers via ContentView.titlebarPageShortcutHintModifierFlags(),
causing titlebar hints to appear when users hold page shortcuts (e.g. ⌘⌥…) even
if they disabled Command-hold hints; change the requiredModifierFlagsProvider
used by titlebarPageShortcutHintMonitor to return a Command-only modifier
(normalized to [.command]) so ShortcutHintModifierPolicy.shouldShowHints treats
it as the Command-hold case, and ensure any logic that suppresses the
TabItemView close button continues to consult showsModifierShortcutHints (or the
always-show debug flag) rather than the page-shortcut provider; refer to
titlebarPageShortcutHintMonitor, ShortcutHintModifierMonitor,
ContentView.titlebarPageShortcutHintModifierFlags(),
ShortcutHintModifierPolicy.shouldShowHints, showsModifierShortcutHints, and
TabItemView in your change.
In `@Sources/TerminalController.swift`:
- Around line 3865-3869: The failure branch after calling
resolved.workspace.movePage(pageId:toIndex:) currently leaves result as the
previous not_found value; change it so when movePage returns false because the
workspace has a single page (the guard in movePage(pageId:toIndex:) in
Workspace), you set result to the proper invalid_state response instead of
leaving it unchanged. Update the block in TerminalController where movePage is
called (the code that builds payload with v2PageResultPayload and sets result)
to detect the false return and assign result = .invalid_state (or the project's
equivalent enum/case for invalid_state) so the client receives the correct error
for single-page reorders.
- Around line 4248-4280: The code conflates "parameter omitted" with "parameter
present but malformed"; update V2PageWorkspaceResolution to include a new case
(.invalidParams) and modify v2ResolveWorkspaceForPage to detect when params
contains "page_id" or "workspace_id" but the converted UUIDs are nil (i.e.
malformed) and return .invalidParams in those branches instead of treating them
as omitted; specifically, keep using params["workspace_id"] != nil and
params["page_id"] != nil to detect explicit presence, return .invalidParams when
an explicit key is present but v2ResolveWorkspace(params:) or the pageId
argument is nil/malformed, and only fall back to v2LocatePage when the keys are
genuinely absent; update any callers to handle the new .invalidParams case.
---
Outside diff comments:
In `@Sources/Workspace.swift`:
- Around line 1750-1762: When restoring a stored page in restoreStoredPage you
currently drop storedPageStates and, if the runtime/session parity check fails,
fall back to restoreSessionPageState without cleaning up
runtimeState.detachedSurfaces; iterate over
storedState.runtimeState?.detachedSurfaces (if present) and explicitly tear down
each detached surface before calling restoreSessionPageState — use the same
teardown/remove API used elsewhere in the codebase (e.g. the method used by
restoreRuntimePageState or any existing removeDetachedSurface/teardownSurface
helper) so detached WebKit/terminal resources are closed instead of orphaned.
---
Nitpick comments:
In `@Sources/cmuxApp.swift`:
- Around line 933-939: The decodeShortcut(from:fallback:) helper is duplicated
across cmuxApp.swift, ContentView.swift, NotificationsPage.swift, and
WorkspaceContentView.swift; extract it into one shared location by moving its
logic into a single utility (e.g., an extension on StoredShortcut or a new
ShortcutsUtils.swift) and update callers to use
StoredShortcut.decode(from:fallback:) or ShortcutsUtils.decodeShortcut(...)
instead of the local private function; ensure the new shared method signature
matches the original (accepts Data and a fallback StoredShortcut and returns
StoredShortcut) and remove the duplicate private functions from the three files.
In `@Sources/ContentView.swift`:
- Around line 1535-1537: Replace the broad workspaceHasMultiplePages gate with a
per-page flag derived from the active page: add a pageCanClose boolean (computed
from workspace.canClosePage(activePage.id) or the equivalent active page
identifier) and use that flag where the "Close Page" command palette entry is
gated (references: workspaceHasMultiplePages -> replace with pageCanClose; use
workspace.canClosePage(page.id) to compute it). Update all occurrences mentioned
(the main block and the other ranges) so the command palette aligns with the
titlebar's per-page close policy and avoid future drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5cc575c8-b442-4fa4-91c2-131248938e0c
📒 Files selected for processing (11)
CLI/cmux.swiftResources/Info.plistSources/AppDelegate.swiftSources/ContentView.swiftSources/KeyboardShortcutSettings.swiftSources/TerminalController.swiftSources/Workspace.swiftSources/cmuxApp.swiftcmuxTests/AppDelegateShortcutRoutingTests.swiftdocs/workspace-pages-spec.mdtests_v2/test_page_cli_socket_parity.py
![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: 22e85edfa0
ℹ️ 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.
1 issue found across 11 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/ContentView.swift">
<violation number="1" location="Sources/ContentView.swift:1320">
P2: The titlebar page-hint monitor is now locked to ⌘ only, so hints no longer track the real page shortcut modifiers (default ⌘⌥ and custom bindings).</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
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 (2)
Sources/ContentView.swift (2)
2330-2343:⚠️ Potential issue | 🟡 MinorHide the invisible close button from accessibility.
opacity(0)andallowsHitTesting(false)don't remove this control from the accessibility tree, so VoiceOver can still encounter an invisible “Close Page” button. Add accessibility hiding when the button is not shown, or render it conditionally.♿ Minimal fix
.buttonStyle(.plain) .disabled(!canClose) .accessibilityLabel(String(localized: "workspace.page.context.close", defaultValue: "Close Page")) + .accessibilityHidden(!showCloseButton) .opacity(showCloseButton ? 1 : 0) .allowsHitTesting(showCloseButton) .accessibilityIdentifier(titlebarPageCloseButtonAccessibilityIdentifier(pageId: page.id))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ContentView.swift` around lines 2330 - 2343, The invisible close button remains in the accessibility tree; update the Button with either conditional rendering or add accessibility hiding when hidden: wrap the Button in an if showCloseButton { ... } so it is not created at all when showCloseButton is false, or keep the Button but add .accessibilityHidden(!showCloseButton) alongside the existing modifiers; ensure you preserve .accessibilityLabel and .accessibilityIdentifier (titlebarPageCloseButtonAccessibilityIdentifier(pageId: page.id)) and keep .disabled(!canClose) behavior.
2232-2249:⚠️ Potential issue | 🟡 MinorDon't make the titlebar “New Page” action hover-only.
The new accessibility label helps, but the button still only exists while the pointer is hovering the titlebar, so keyboard and VoiceOver users still can't reach it. Please keep an always-focusable affordance here, or expose an equivalent non-hover-only control.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ContentView.swift` around lines 2232 - 2249, The button is only present when isTitlebarHovered so keyboard/VoiceOver users can't reach it; instead always render the Button that calls workspace.newPage(select:), and toggle its visual state based on isTitlebarHovered (e.g., change .opacity, .background, or .foregroundColor) rather than conditionally including it or using .transition(.opacity) to remove it; keep the .accessibilityLabel and .accessibilityIdentifier ("titlebarPageNewButton") and ensure it is not accessibilityHidden so it remains focusable by keyboard/VoiceOver even when not hovered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/workspace-pages-spec.md`:
- Around line 66-67: The docs incorrectly describe the chord as "Command+Option"
but the implementation uses only "Command" via
ShortcutHintModifierPolicy.shouldShowHints; update the text in the specified
locations (the three occurrences) to read "Command" instead of "Command+Option"
and ensure the description of page shortcut hints and references to
KeyboardShortcutSettings/default bindings match the implementation; search for
the phrase "Command+Option" in this document and replace it with "Command" where
it refers to page shortcut hint chord so the docs align with
ShortcutHintModifierPolicy.shouldShowHints.
In `@Sources/TerminalController.swift`:
- Line 3584: The code currently uses v2Bool(params, "select") ?? false which
treats malformed explicit values as if the key were absent; update the
validation so that if params contains the "select" key but v2Bool returns nil
you return an invalid_params error instead of defaulting to false. Concretely,
before calling v2FocusAllowed(requested: ...), check
params.keys.contains("select"); if true and v2Bool(params, "select") == nil
produce an invalid_params response, otherwise pass the unwrapped Bool (or
default false only when the key is absent) into v2FocusAllowed. Apply the same
pattern for other boolean params mentioned (e.g., "force" in page.close) and the
other call sites around v2FocusAllowed/v2Bool noted in the comment (lines
3606-3607, 3707-3719).
- Around line 4230-4338: Add distinct failure cases to V2PageWorkspaceResolution
(e.g., missingWindow, missingSurface, missingTab, missingWorkspaceRef or
noActiveWorkspace) and update v2ResolveWorkspaceForPage to return those specific
cases instead of collapsing to .workspaceNotFound: replace the current early
UUID-nil checks to return the new missing* cases (for params["window_id"],
["surface_id"], ["tab_id"], ["workspace_id"] and when pageId present but page
not found in an explicitly scoped workspace return
.pageNotFoundInScopedWorkspace as already done), and when no explicit scope and
no active workspace inferable return the new
noActiveWorkspace/missingWorkspaceRef case. Also update downstream handling (
callers of V2PageWorkspaceResolution and v2PageResolutionInvalidParamsResult )
to map the new enum cases to the correct error codes/messages so missing-scope
objects produce not_found for the specific scope and truly no active workspace
produces the existing workspace-not-found path.
In `@Sources/Workspace.swift`:
- Around line 1756-1762: The current check builds expectedPanelIds from
sessionState.panels (capped by SessionPersistencePolicy.maxPanelsPerWorkspace)
which can miss panel IDs present in runtimeState.layout; change the validation
to collect all panel UUIDs referenced by runtimeState.layout (e.g., add a helper
panelIds(in:) that traverses SessionWorkspaceLayoutSnapshot cases) and ensure
expectedPanelIds (or a new set derived from the layout) is a subset of
runtimeState.detachedSurfaces.keys before calling
restoreRuntimePageState(runtimeState); if any layout panel ID is missing, fall
back to teardownDetachedSurfaces(storedState.runtimeState?.detachedSurfaces) and
restoreSessionPageState(storedState.sessionState) instead, keeping existing
teardownDetachedSurfaces and restoreSessionPageState calls.
---
Outside diff comments:
In `@Sources/ContentView.swift`:
- Around line 2330-2343: The invisible close button remains in the accessibility
tree; update the Button with either conditional rendering or add accessibility
hiding when hidden: wrap the Button in an if showCloseButton { ... } so it is
not created at all when showCloseButton is false, or keep the Button but add
.accessibilityHidden(!showCloseButton) alongside the existing modifiers; ensure
you preserve .accessibilityLabel and .accessibilityIdentifier
(titlebarPageCloseButtonAccessibilityIdentifier(pageId: page.id)) and keep
.disabled(!canClose) behavior.
- Around line 2232-2249: The button is only present when isTitlebarHovered so
keyboard/VoiceOver users can't reach it; instead always render the Button that
calls workspace.newPage(select:), and toggle its visual state based on
isTitlebarHovered (e.g., change .opacity, .background, or .foregroundColor)
rather than conditionally including it or using .transition(.opacity) to remove
it; keep the .accessibilityLabel and .accessibilityIdentifier
("titlebarPageNewButton") and ensure it is not accessibilityHidden so it remains
focusable by keyboard/VoiceOver even when not hovered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cd96d9c2-7c36-4115-8028-cd8e5c2fa655
📒 Files selected for processing (11)
CLI/cmux.swiftSources/ContentView.swiftSources/KeyboardShortcutSettings.swiftSources/NotificationsPage.swiftSources/TerminalController.swiftSources/Workspace.swiftSources/WorkspaceContentView.swiftSources/cmuxApp.swiftcmuxTests/AppDelegateShortcutRoutingTests.swiftdocs/workspace-pages-spec.mdtests_v2/test_page_cli_socket_parity.py
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/KeyboardShortcutSettings.swift
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4503d5d701
ℹ️ 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: 2
🧹 Nitpick comments (1)
cmuxTests/AppDelegateShortcutRoutingTests.swift (1)
1389-1396: Assert exactsurface_refsmapping, not just counts.These assertions still pass if pane-level refs are duplicated or reordered. Since
agentsSurfacesis already available here, compare the exact ref list instead of only checking lengths.Suggested assertion tightening
- XCTAssertEqual((agentsPane?["surface_refs"] as? [String])?.count, expectedAgentSurfaceIds.count) - XCTAssertEqual(agentsSurfaces?.compactMap { $0["ref"] as? String }.count, expectedAgentSurfaceIds.count) + let expectedAgentSurfaceRefs = agentsSurfaces?.compactMap { $0["ref"] as? String } + XCTAssertEqual(agentsPane?["surface_refs"] as? [String], expectedAgentSurfaceRefs)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/AppDelegateShortcutRoutingTests.swift` around lines 1389 - 1396, The pane-level ref assertions only checked counts and can miss duplicates/reordering; replace them with exact comparisons by building the expected ref list from agentsSurfaces using expectedAgentSurfaceIds order and then assert equality against agentsPane["surface_refs"]. Specifically, compute expectedRefs = expectedAgentSurfaceIds.compactMap { id in agentsSurfaces?.first(where: { ($0["id"] as? String) == id })?["ref"] as? String } and then XCTAssertEqual(agentsPane?["surface_refs"] as? [String], expectedRefs); remove or replace the two count-based assertions that reference (agentsPane?["surface_refs"] as? [String])?.count and agentsSurfaces?.compactMap { $0["ref"] as? String }.count with this exact-list assertion (keep the selected_surface_ref assertion as-is which already compares the selected ref).
🤖 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/TerminalController.swift`:
- Around line 2674-2685: The surface summary fields are using raw pane.panelIds
instead of the filtered surfaces, causing surface_ids/selected_surface_id to
mismatch when panelSnapshotsById dropped entries; update the construction to
derive surface_ids and surface_refs (and
selected_surface_id/selected_surface_ref) from the already-computed surfaces
array (which was filtered via panelSnapshotsById) rather than pane.panelIds,
using the same v2Ref(kind:.surface, uuid: ...) and v2OrNull(...) helpers and
preserving paneIndex and focused logic so the exported keys
("surface_ids","surface_refs","selected_surface_id","selected_surface_ref")
exactly mirror the entries in surfaces and selected panel presence.
- Around line 4311-4395: v2ResolveWorkspaceForPage currently routes to a
TabManager (e.g. when window_id is provided) but then calls v2ResolveWorkspace
which only checks the selected workspace, causing pages in other workspaces of
the same TabManager to be missed; change the logic after selecting
routedTabManager in v2ResolveWorkspaceForPage so that if pageId is present you
first search all workspaces owned by that routedTabManager for the page (via an
existing helper or add a small helper like locatePage(in: TabManager, pageId:
UUID) that mirrors v2LocatePage but scoped to a TabManager), and if found return
.resolved(tabManager: routedTabManager, workspace: foundWorkspace); if not found
and hasExplicitScope return .pageNotFoundInScopedWorkspace, otherwise fall
through to the existing global locate logic. Ensure you update references in
v2ResolveWorkspaceForPage (the window_id branch and subsequent resolution block)
and reuse v2LocatePage behavior but limited to the routedTabManager.
---
Nitpick comments:
In `@cmuxTests/AppDelegateShortcutRoutingTests.swift`:
- Around line 1389-1396: The pane-level ref assertions only checked counts and
can miss duplicates/reordering; replace them with exact comparisons by building
the expected ref list from agentsSurfaces using expectedAgentSurfaceIds order
and then assert equality against agentsPane["surface_refs"]. Specifically,
compute expectedRefs = expectedAgentSurfaceIds.compactMap { id in
agentsSurfaces?.first(where: { ($0["id"] as? String) == id })?["ref"] as? String
} and then XCTAssertEqual(agentsPane?["surface_refs"] as? [String],
expectedRefs); remove or replace the two count-based assertions that reference
(agentsPane?["surface_refs"] as? [String])?.count and agentsSurfaces?.compactMap
{ $0["ref"] as? String }.count with this exact-list assertion (keep the
selected_surface_ref assertion as-is which already compares the selected ref).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 280e80b3-e76b-4226-bdc8-43d7d7a49ea9
📒 Files selected for processing (4)
Sources/TerminalController.swiftSources/Workspace.swiftcmuxTests/AppDelegateShortcutRoutingTests.swiftcmuxTests/WorkspaceContentViewVisibilityTests.swift
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Sources/TerminalController.swift (1)
2675-2685:⚠️ Potential issue | 🟡 MinorKeep stored pane
surface_*fields aligned with the filteredsurfacesarray.Lines 2675-2685 still derive
surface_ids,surface_refs, andselected_surface_*from rawpane.panelIds. IfpanelSnapshotsByIddropped a missing snapshot,system.treecan return IDs/refs that do not exist insurfaces.Suggested fix
let focused = surfaces.contains { ($0["focused"] as? Bool) == true } - let surfaceIds = pane.panelIds.map(\.uuidString) - let surfaceRefs = pane.panelIds.map { v2Ref(kind: .surface, uuid: $0) } + let presentPanelIds = pane.panelIds.filter { panelSnapshotsById[$0] != nil } + let selectedPanelId = pane.selectedPanelId.flatMap { presentPanelIds.contains($0) ? $0 : nil } + let surfaceIds = presentPanelIds.map(\.uuidString) + let surfaceRefs = presentPanelIds.map { v2Ref(kind: .surface, uuid: $0) } return [ "id": NSNull(), "ref": NSNull(), "index": paneIndex, "focused": focused, "surface_ids": surfaceIds, "surface_refs": surfaceRefs, - "selected_surface_id": v2OrNull(pane.selectedPanelId?.uuidString), - "selected_surface_ref": v2Ref(kind: .surface, uuid: pane.selectedPanelId), + "selected_surface_id": v2OrNull(selectedPanelId?.uuidString), + "selected_surface_ref": v2Ref(kind: .surface, uuid: selectedPanelId), "surface_count": surfaces.count, "surfaces": surfaces ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TerminalController.swift` around lines 2675 - 2685, The stored pane "surface_*" fields are built from raw pane.panelIds which can include IDs dropped by panelSnapshotsById; update the construction so surface_ids and surface_refs are derived from the filtered surfaces array (not pane.panelIds), and compute selected_surface_id/selected_surface_ref only when pane.selectedPanelId is present in that filtered surfaces set—use the same v2OrNull/v2Ref helpers as before but source values from the surfaces collection used for the UI tree (and reference panelSnapshotsById/system.tree filtering) so stored IDs/refs remain aligned with the surfaces array.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@Sources/TerminalController.swift`:
- Around line 2675-2685: The stored pane "surface_*" fields are built from raw
pane.panelIds which can include IDs dropped by panelSnapshotsById; update the
construction so surface_ids and surface_refs are derived from the filtered
surfaces array (not pane.panelIds), and compute
selected_surface_id/selected_surface_ref only when pane.selectedPanelId is
present in that filtered surfaces set—use the same v2OrNull/v2Ref helpers as
before but source values from the surfaces collection used for the UI tree (and
reference panelSnapshotsById/system.tree filtering) so stored IDs/refs remain
aligned with the surfaces array.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6106176c-dca0-463d-a96d-415a5dc79f43
📒 Files selected for processing (2)
Sources/TerminalController.swiftcmuxTests/AppDelegateShortcutRoutingTests.swift
Closed
|
Is this feature abandoned? |
|
Is this meant to be closed in favor of #4766? |
Collaborator
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.
Replacement for #1030.
Addresses the remaining review comments on #569.
What changed:
Verification:
xcodebuild -project GhosttyTabs.xcodeproj -scheme cmux-unit -configuration Debug -destination 'platform=macOS' -derivedDataPath /tmp/cmux-issue-569-titlebar-top-level-tabs-reopen buildpython3 -m py_compile tests_v2/test_page_cli_socket_parity.py./scripts/reload.sh --tag issue-569-titlebar-top-level-tabs-reopengh workflow run test-e2e.yml --repo manaflow-ai/cmux -f ref=issue-569-titlebar-top-level-tabs-reopen -f test_filter=WorkspacePagesUITests -f record_video=trueSummary by cubic
Reopens workspace pages as top-level tabs in the titlebar with precise drag-and-drop and cleaner restore. Tightens v2 page socket validation, scopes page lookups to the caller’s window, and keeps stored page tree surfaces aligned with runtime state.
New Features
Migration
Written for commit 897a578. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests