Skip to content

Add last-visited tab quick switch (Ctrl+Tab) and surface.last API#1106

Open
maxbeizer wants to merge 2 commits intomanaflow-ai:mainfrom
maxbeizer:feat/last-visited-tab
Open

Add last-visited tab quick switch (Ctrl+Tab) and surface.last API#1106
maxbeizer wants to merge 2 commits intomanaflow-ai:mainfrom
maxbeizer:feat/last-visited-tab

Conversation

@maxbeizer
Copy link

@maxbeizer maxbeizer commented Mar 9, 2026

Summary

  • Implement per-pane MRU tab tracking so quick-switch targets the previously visited surface
  • Map legacy Ctrl+Tab/Ctrl+Shift+Tab to last-visited surface switching
  • Add new v2 socket method surface.last and expose it in capabilities
  • Add tmux-compat CLI command last-tab that calls surface.last
  • Extend tests_v2 helpers and tmux compatibility matrix coverage for surface.last and last-tab

Validation

  • ./scripts/setup.sh
  • ./scripts/reload.sh --tag fix-issue-474
  • xcodebuild -project GhosttyTabs.xcodeproj -scheme cmux -configuration Debug -destination 'platform=macOS' build

Fixes #474


Summary by cubic

Add a quick switch to the previously visited tab per pane. Ctrl+Tab/Ctrl+Shift+Tab now toggle MRU in-pane, with a new surface.last API and cmux last-tab command.

  • New Features
    • Per-pane MRU tracking; Ctrl+Tab/Ctrl+Shift+Tab toggle to the last visited surface in the focused pane.
    • v2 surface.last socket method (in capabilities) with optional pane_id to target a specific pane.
    • cmux command last-tab [--workspace <id|ref|index>] that calls surface.last and returns formatted tab/pane/workspace handles.
    • MRU history prunes on pane close to avoid stale state.

Written for commit e3f7fc3. Summary will update on new commits.

Summary by CodeRabbit

  • New Features

    • Added "last-tab" subcommand to switch to the previously visited surface in the current pane.
    • MRU-based per-pane surface history so "last-tab" reliably returns to the most recently used surface.
    • Ctrl+Tab now toggles between the current and last-visited surface for faster switching.
  • Tests

    • Added tests covering the "last-tab" behavior and presence of the new capability.

Implement per-pane MRU tab tracking so Ctrl+Tab toggles to the
previously visited surface instead of cycling through tabs.

- Add currentSelectedPanelByPaneId/lastVisitedPanelByPaneId tracking
  to Workspace with pruning for closed surfaces
- Add selectLastVisitedSurface() to Workspace and TabManager
- Record surface selections in BonsplitDelegate didSelectTab
- Map Ctrl+Tab and Ctrl+Shift+Tab to last-visited surface switching
- Add surface.last v2 socket method and expose in capabilities
- Add last-tab CLI command (tmux compat)
- Add last_surface() test helper and compat matrix coverage

Fixes manaflow-ai#474

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@vercel
Copy link

vercel bot commented Mar 9, 2026

@maxbeizer is attempting to deploy a commit to the Manaflow Team on Vercel.

A member of the Team first needs to authorize it.

@coderabbitai
Copy link

coderabbitai bot commented Mar 9, 2026

📝 Walkthrough

Walkthrough

Adds last-visited surface (tab) navigation: new "last-tab" CLI command, Ctrl+Tab mapped to select last-visited surface, per-pane MRU tracking in Workspace, and V1/V2 API handlers for surface.last.

Changes

Cohort / File(s) Summary
CLI Command Integration
CLI/cmux.swift
Added last-tab subcommand, help text, v2 payload summary construction, and tmux-compat dispatch entry.
UI Navigation Binding
Sources/AppDelegate.swift
Changed Ctrl+Tab handling to call selectLastVisitedSurface() (surface quick switch). Minor comment update.
Core Surface Selection API
Sources/TabManager.swift
Added selectLastVisitedSurface() that forwards to the focused workspace.
MRU Tracking & History Management
Sources/Workspace.swift
Introduced per-pane MRU storage (currentSelectedPanelByPaneId, lastVisitedPanelByPaneId), selection recording helpers, and selectLastVisitedSurface(inPane:) logic; wired into tab/pane events.
V1/V2 Command Handler Support
Sources/TerminalController.swift
Registered surface.last in V1 dispatch and V2 capabilities; implemented v2SurfaceLast(...) handler, and added surface.last to focusIntentV2Methods.
Tests / Test Helpers
tests_v2/cmux.py, tests_v2/test_tmux_compat_matrix.py
Added last_surface() to test cmux client; extended tmux compatibility matrix with surface.last capability and end-to-end last-tab toggle test sequence.

Sequence Diagram(s)

sequenceDiagram
    actor User
    participant AppDelegate
    participant TabManager
    participant Workspace
    participant BonsplitDelegate

    User->>AppDelegate: Press Ctrl+Tab / run last-tab
    AppDelegate->>TabManager: selectLastVisitedSurface()
    TabManager->>Workspace: selectLastVisitedSurface(inPane?)
    Workspace->>Workspace: Lookup current & last panels for pane
    Workspace->>BonsplitDelegate: instruct selection change
    BonsplitDelegate->>Workspace: recordSurfaceSelection(panelId)
    Workspace-->>User: Focus changes to last-visited surface
Loading

Estimated Code Review Effort

🎯 3 (Moderate) | ⏱️ ~28 minutes

Poem

🐰 I hopped through panes and toggled fast,
A shortcut found from first to last.
With MRU crumbs I trace the track,
Ctrl+Tab bounces my steps back.
Happy rabbit, tab-hopping at last!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 29.63% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed Title clearly summarizes the main changes: adding last-visited tab quick switch functionality and a new surface.last API.
Description check ✅ Passed Description includes Summary and Validation sections, detailing changes and testing steps, but lacks Testing and Demo Video sections from the template.
Linked Issues check ✅ Passed All objectives from issue #474 are met: per-pane MRU tracking for Ctrl+Tab, last-visited surface switching via surface.last API, and CLI command support.
Out of Scope Changes check ✅ Passed All changes are within scope of issue #474: MRU tracking, surface.last API, CLI command, and supporting infrastructure are directly tied to implementing last-visited tab/surface functionality.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (2)
CLI/cmux.swift (1)

5985-5992: Prefer tab-formatted output for last-tab.

Text mode currently falls back to v2OKSummary(...), so the new tab command prints surface: handles. This is functional, but it breaks the tab-oriented terminology already used by tab-action and rename-tab in this file. Reusing formatTabHandle(...) here would keep the tmux-compat surface consistent.

🔧 Suggested refactor
         case "last-tab":
             let workspaceArg = workspaceFromArgsOrEnv(commandArgs, windowOverride: windowOverride)
             var params: [String: Any] = [:]
             let wsId = try normalizeWorkspaceHandle(workspaceArg, client: client)
             if let wsId { params["workspace_id"] = wsId }
             let payload = try client.sendV2(method: "surface.last", params: params)
-            printV2Payload(payload, jsonOutput: jsonOutput, idFormat: idFormat, fallbackText: v2OKSummary(payload, idFormat: idFormat, kinds: ["surface", "pane", "workspace"]))
+            var summaryParts = ["OK"]
+            if let tabHandle = formatTabHandle(payload, idFormat: idFormat) {
+                summaryParts.append("tab=\(tabHandle)")
+            }
+            if let paneHandle = formatHandle(payload, kind: "pane", idFormat: idFormat) {
+                summaryParts.append("pane=\(paneHandle)")
+            }
+            if let workspaceHandle = formatHandle(payload, kind: "workspace", idFormat: idFormat) {
+                summaryParts.append("workspace=\(workspaceHandle)")
+            }
+            printV2Payload(
+                payload,
+                jsonOutput: jsonOutput,
+                idFormat: idFormat,
+                fallbackText: summaryParts.joined(separator: " ")
+            )
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@CLI/cmux.swift` around lines 5985 - 5992, The "last-tab" branch currently
uses v2OKSummary as the text fallback which yields "surface:" handles and breaks
tab-oriented terminology; update the fallback text passed to printV2Payload (in
the case "last-tab" block that calls workspaceFromArgsOrEnv,
normalizeWorkspaceHandle, client.sendV2 and printV2Payload) to use the tab
formatter instead—call formatTabHandle (or derive a tab-friendly string from the
payload) as the fallbackText so the printed output matches the tab terminology
used by tab-action and rename-tab rather than the generic v2OKSummary.
Sources/Workspace.swift (1)

3895-3919: Consider cleaning up MRU history when panes are closed.

When a pane is closed via splitTabBar(_:didClosePane:), the entries in currentSelectedPanelByPaneId and lastVisitedPanelByPaneId for that pane's UUID persist indefinitely. While this won't cause functional issues (pane UUIDs are unique and won't be reused), it creates a minor memory leak.

♻️ Proposed cleanup in didClosePane

Add cleanup in splitTabBar(_:didClosePane:) around line 4271:

 func splitTabBar(_ controller: BonsplitController, didClosePane paneId: PaneID) {
     let closedPanelIds = pendingPaneClosePanelIds.removeValue(forKey: paneId.id) ?? []
     let shouldScheduleFocusReconcile = !isDetachingCloseTransaction
+    
+    // Clean up MRU history for the closed pane
+    currentSelectedPanelByPaneId.removeValue(forKey: paneId.id)
+    lastVisitedPanelByPaneId.removeValue(forKey: paneId.id)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Sources/Workspace.swift` around lines 3895 - 3919, The MRU maps
currentSelectedPanelByPaneId and lastVisitedPanelByPaneId are not cleaned when a
pane is closed; update the SplitTabBar delegate method
splitTabBar(_:didClosePane:) to remove the pane's UUID key from both
dictionaries (use pane.id to identify the key) so entries for a closed pane are
removed, e.g. call currentSelectedPanelByPaneId.removeValue(forKey: pane.id) and
lastVisitedPanelByPaneId.removeValue(forKey: pane.id) (or invoke a shared
cleanup helper) inside splitTabBar(_:didClosePane:).
🤖 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 4631-4639: Update the help text for the "last-tab" command to
document that the --workspace flag also accepts numeric workspace indexes (since
the implementation uses normalizeWorkspaceHandle(...)); locate the "case
\"last-tab\"" help string in CLI/cmux.swift and add an "index" (or show a
numeric selector) to the --workspace description (e.g., change "<id|ref>" to
"<id|ref|index>" or similar) so the CLI docs match the
parser/normalizeWorkspaceHandle behavior.

---

Nitpick comments:
In `@CLI/cmux.swift`:
- Around line 5985-5992: The "last-tab" branch currently uses v2OKSummary as the
text fallback which yields "surface:" handles and breaks tab-oriented
terminology; update the fallback text passed to printV2Payload (in the case
"last-tab" block that calls workspaceFromArgsOrEnv, normalizeWorkspaceHandle,
client.sendV2 and printV2Payload) to use the tab formatter instead—call
formatTabHandle (or derive a tab-friendly string from the payload) as the
fallbackText so the printed output matches the tab terminology used by
tab-action and rename-tab rather than the generic v2OKSummary.

In `@Sources/Workspace.swift`:
- Around line 3895-3919: The MRU maps currentSelectedPanelByPaneId and
lastVisitedPanelByPaneId are not cleaned when a pane is closed; update the
SplitTabBar delegate method splitTabBar(_:didClosePane:) to remove the pane's
UUID key from both dictionaries (use pane.id to identify the key) so entries for
a closed pane are removed, e.g. call
currentSelectedPanelByPaneId.removeValue(forKey: pane.id) and
lastVisitedPanelByPaneId.removeValue(forKey: pane.id) (or invoke a shared
cleanup helper) inside splitTabBar(_:didClosePane:).

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 96cb62eb-2cc0-4f32-b2bf-55700fc52f02

📥 Commits

Reviewing files that changed from the base of the PR and between c447bee and d1c06dd.

📒 Files selected for processing (7)
  • CLI/cmux.swift
  • Sources/AppDelegate.swift
  • Sources/TabManager.swift
  • Sources/TerminalController.swift
  • Sources/Workspace.swift
  • tests_v2/cmux.py
  • tests_v2/test_tmux_compat_matrix.py

Copy link

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 7 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/Workspace.swift">

<violation number="1" location="Sources/Workspace.swift:4194">
P2: MRU tab history is only recorded in `didSelectTab`, but this callback is known to be skipped in some selection flows, so Ctrl+Tab can use stale targets.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.


func splitTabBar(_ controller: BonsplitController, didSelectTab tab: Bonsplit.Tab, inPane pane: PaneID) {
if let panelId = panelIdFromSurfaceId(tab.id) {
recordSurfaceSelection(panelId: panelId, in: pane)
Copy link

@cubic-dev-ai cubic-dev-ai bot Mar 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: MRU tab history is only recorded in didSelectTab, but this callback is known to be skipped in some selection flows, so Ctrl+Tab can use stale targets.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/Workspace.swift, line 4194:

<comment>MRU tab history is only recorded in `didSelectTab`, but this callback is known to be skipped in some selection flows, so Ctrl+Tab can use stale targets.</comment>

<file context>
@@ -4139,6 +4190,9 @@ extension Workspace: BonsplitDelegate {
 
     func splitTabBar(_ controller: BonsplitController, didSelectTab tab: Bonsplit.Tab, inPane pane: PaneID) {
+        if let panelId = panelIdFromSurfaceId(tab.id) {
+            recordSurfaceSelection(panelId: panelId, in: pane)
+        }
         applyTabSelection(tabId: tab.id, inPane: pane)
</file context>
Fix with Cubic

@greptile-apps
Copy link

greptile-apps bot commented Mar 9, 2026

Greptile Summary

This PR implements a per-pane MRU ("most-recently-used") tab toggle so that Ctrl+Tab, Ctrl+Shift+Tab, and the new surface.last socket method all switch to the previously visited surface rather than cycling through surfaces in order. It also adds the last-tab CLI command as a tmux-compat alias, extends the v2 capabilities list, and adds matrix test coverage.

The overall design is well-structured and consistent with existing patterns in the codebase. Key concerns:

  • Memory leak in Workspace.swift: splitTabBar(_:didClosePane:) and teardownAllPanels() do not remove entries from the two new per-pane tracking dictionaries when a pane is closed, causing unbounded dictionary growth in long-running sessions with frequent pane creation/destruction.
  • Silent error in v2SurfaceLast: Success is detected by diffing ws.focusedPanelId before and after the call. If applyTabSelection posts focus changes asynchronously, the method may return a "not_found" error even though the tab switch was triggered — diverging from the unconditional ok pattern used by v2SurfaceFocus and siblings.
  • Ctrl+Shift+Tab behavior change: Both Ctrl+Tab and Ctrl+Shift+Tab now call selectLastVisitedSurface() identically, removing the backward-cycling behavior that Ctrl+Shift+Tab previously provided. This appears intentional per the PR description but is a silent breaking change for users relying on the old shortcut.
  • Redundant prune: selectLastVisitedSurface() calls pruneSurfaceSelectionHistory explicitly at the top, then calls recordSurfaceSelection which immediately calls it again — the first call is unnecessary.

Confidence Score: 3/5

  • Safe to merge after addressing the pane-close memory leak and the v2SurfaceLast success-detection issue.
  • The feature logic and test coverage are solid, but there is a confirmed memory leak (pane UUID entries never cleaned up on close) and a potential silent-failure path in the socket API handler that should be fixed before shipping.
  • Sources/Workspace.swift (pane-close cleanup gap) and Sources/TerminalController.swift (v2SurfaceLast success detection)

Important Files Changed

Filename Overview
Sources/Workspace.swift Adds per-pane MRU tracking via two new UUID-keyed dictionaries and the selectLastVisitedSurface() method; the pane-close delegate and teardownAllPanels() do not clean up these dictionaries, causing unbounded growth. Also contains a redundant pruneSurfaceSelectionHistory call in the new function.
Sources/TerminalController.swift Adds surface.last socket method and exposes it in capabilities; success detection in v2SurfaceLast via focusedPanelId diff may silently return an error if applyTabSelection updates focus asynchronously, diverging from the pattern used by sibling methods like v2SurfaceFocus.
Sources/AppDelegate.swift Remaps both Ctrl+Tab and Ctrl+Shift+Tab to selectLastVisitedSurface(); this intentionally removes the previous forward/backward cycling distinction for these shortcuts.
Sources/TabManager.swift Adds a thin selectLastVisitedSurface() delegate method that forwards to the selected workspace; straightforward and consistent with sibling methods.
CLI/cmux.swift Adds last-tab CLI command including help text, usage string, and dispatch to surface.last; consistent with other command implementations in the file.
tests_v2/cmux.py Adds last_surface() helper method wrapping surface.last; straightforward and consistent with the existing helper pattern.
tests_v2/test_tmux_compat_matrix.py Adds capability check for surface.last and a toggle-round-trip test for last-tab; test coverage is good and correctly validates both directions of the MRU toggle.

Sequence Diagram

sequenceDiagram
    participant User
    participant AppDelegate
    participant TabManager
    participant Workspace
    participant BonsplitController
    participant TerminalController

    Note over User,TerminalController: Keyboard shortcut path (Ctrl+Tab)
    User->>AppDelegate: Ctrl+Tab / Ctrl+Shift+Tab
    AppDelegate->>TabManager: selectLastVisitedSurface()
    TabManager->>Workspace: selectLastVisitedSurface()
    Workspace->>Workspace: pruneSurfaceSelectionHistory(pane)
    Workspace->>Workspace: recordSurfaceSelection(currentPanel, pane)
    Note right of Workspace: Updates currentSelectedPanelByPaneId\nand lastVisitedPanelByPaneId
    Workspace->>BonsplitController: selectTab(targetTabId)
    Workspace->>Workspace: applyTabSelection(tabId, pane)

    Note over User,TerminalController: Socket API path (surface.last)
    User->>TerminalController: surface.last {workspace_id?}
    TerminalController->>Workspace: selectLastVisitedSurface()
    Workspace->>BonsplitController: selectTab(targetTabId)
    Workspace->>Workspace: applyTabSelection(tabId, pane)
    TerminalController-->>User: {surface_id, pane_id, workspace_id, window_id}

    Note over User,BonsplitController: MRU recording path (tab bar click)
    User->>BonsplitController: click tab
    BonsplitController->>Workspace: splitTabBar(_:didSelectTab:inPane:)
    Workspace->>Workspace: recordSurfaceSelection(panelId, pane)
    Workspace->>Workspace: applyTabSelection(tabId, pane)
Loading

Comments Outside Diff (1)

  1. Sources/Workspace.swift, line 4270-4302 (link)

    MRU dictionaries not cleaned up when a pane closes

    splitTabBar(_:didClosePane:) cleans up all panel-level metadata (panels, panelDirectories, panelGitBranches, panelTitles, panelSubscriptions, etc.) but the two new per-pane MRU tracking dictionaries added in this PR are not cleared for the closed pane. Because they are keyed by pane UUID (not panel UUID), the pruneSurfaceSelectionHistory helper — which only prunes stale panel IDs within an existing pane — will never reach these entries after the pane itself is gone.

    The result is that every closed pane leaves a permanently stale entry in both dictionaries. In long-running sessions with frequent pane creation/destruction this accumulates without bound.

    teardownAllPanels() has the same gap: it explicitly clears many dictionaries but omits both new ones.

    The fix is to remove the closed pane's entries from both tracking dictionaries inside splitTabBar(_:didClosePane:), and to call removeAll on both inside teardownAllPanels().

Last reviewed commit: d1c06dd

Comment on lines +7408 to 7416
// Surface quick switch (legacy Ctrl+Tab support)
if matchTabShortcut(event: event, shortcut: StoredShortcut(key: "\t", command: false, shift: false, option: false, control: true)) {
tabManager?.selectNextSurface()
tabManager?.selectLastVisitedSurface()
return true
}
if matchTabShortcut(event: event, shortcut: StoredShortcut(key: "\t", command: false, shift: true, option: false, control: true)) {
tabManager?.selectPreviousSurface()
tabManager?.selectLastVisitedSurface()
return true
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ctrl+Shift+Tab now has identical behavior to Ctrl+Tab

Previously these two shortcuts provided directional tab cycling — forward with Ctrl+Tab and backward with Ctrl+Shift+Tab. After this change both shortcuts call selectLastVisitedSurface(), making Ctrl+Shift+Tab redundant.

The PR description explicitly calls these "legacy" shortcuts, but conventionally Ctrl+Shift+Tab is expected to reverse the direction of Ctrl+Tab. Users who relied on Ctrl+Shift+Tab to cycle backward through surfaces will silently get MRU-toggle behavior instead, with no alternative key binding to cycle backward via the keyboard (the selectPreviousSurface() path is still accessible via Cmd+Shift+[ and the palette, but not via a ctrl-tab equivalent).

Worth confirming this is the intended end-state, or consider mapping Ctrl+Shift+Tab to selectPreviousSurface() to preserve backward compat.

Comment on lines +3208 to +3215
/// Select the previously visited surface in the currently focused pane (MRU toggle).
func selectLastVisitedSurface() {
guard let focusedPaneId = bonsplitController.focusedPaneId else { return }
pruneSurfaceSelectionHistory(for: focusedPaneId)

guard let currentTabId = bonsplitController.selectedTab(inPane: focusedPaneId)?.id,
let currentPanelId = panelIdFromSurfaceId(currentTabId) else { return }
recordSurfaceSelection(panelId: currentPanelId, in: focusedPaneId)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant pruneSurfaceSelectionHistory call

selectLastVisitedSurface() calls pruneSurfaceSelectionHistory(for: focusedPaneId) at line 3211, but then immediately calls recordSurfaceSelection(panelId:in:) at line 3215, which opens with another call to pruneSurfaceSelectionHistory. The initial explicit prune is therefore a no-op — the history will be pruned again inside recordSurfaceSelection before any mutation happens.

Suggested change
/// Select the previously visited surface in the currently focused pane (MRU toggle).
func selectLastVisitedSurface() {
guard let focusedPaneId = bonsplitController.focusedPaneId else { return }
pruneSurfaceSelectionHistory(for: focusedPaneId)
guard let currentTabId = bonsplitController.selectedTab(inPane: focusedPaneId)?.id,
let currentPanelId = panelIdFromSurfaceId(currentTabId) else { return }
recordSurfaceSelection(panelId: currentPanelId, in: focusedPaneId)
func selectLastVisitedSurface() {
guard let focusedPaneId = bonsplitController.focusedPaneId else { return }
guard let currentTabId = bonsplitController.selectedTab(inPane: focusedPaneId)?.id,
let currentPanelId = panelIdFromSurfaceId(currentTabId) else { return }
recordSurfaceSelection(panelId: currentPanelId, in: focusedPaneId)

Comment on lines +3761 to +3763
let before = ws.focusedPanelId
ws.selectLastVisitedSurface()
guard let after = ws.focusedPanelId, after != before else { return }
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Success detection via focusedPanelId diff may silently return an error on the first-ever invocation

let before = ws.focusedPanelId
ws.selectLastVisitedSurface()
guard let after = ws.focusedPanelId, after != before else { return }

selectLastVisitedSurface() early-returns (without switching) if lastVisitedPanelByPaneId[pane] is absent or equals the current panel. The guard here catches that case correctly.

However, there is a subtle scenario: if selectLastVisitedSurface() does switch tabs but applyTabSelection posts an async UI update that sets focusedPanelId on the next run-loop cycle rather than synchronously, ws.focusedPanelId will still equal before immediately after the call. The function would then return the default "not_found" error even though the tab switch was successfully triggered.

Other v2Surface* methods (e.g. v2SurfaceFocus) avoid this by setting result = .ok(...) unconditionally after the action, not by diffing state. Aligning with that pattern would be safer:

ws.selectLastVisitedSurface()
guard let after = ws.focusedPanelId else { return }
let paneId = ws.paneId(forPanelId: after)?.id
let windowId = v2ResolveWindowId(tabManager: tabManager)
result = .ok([...])

(This still returns "not_found" if focusedPanelId is nil, and returns ok with the now-focused surface otherwise — consistent with how v2SurfaceFocus behaves.)

- Honor pane_id param in surface.last to target a specific pane
  instead of always using the focused pane
- Add selectLastVisitedSurface(inPane:) optional parameter
- Clean up MRU history in didClosePane to prevent memory leaks
- Use formatTabHandle for tab-oriented CLI output in last-tab
- Document workspace index support in last-tab help text

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 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 5985-6002: The "last-tab" case must reject unexpected positional
tokens or unknown flags instead of silently proceeding; before calling
workspaceFromArgsOrEnv/normalizeWorkspaceHandle/sendV2, validate commandArgs
contains only the allowed options (e.g., the workspace flag) and no stray
positional tokens—if any unexpected args are present, print a clear error and
exit non-zero (or return a failure) so the command fails fast; implement this
check at the top of the "last-tab" case using the existing commandArgs variable
and short-circuit before invoking workspaceFromArgsOrEnv,
normalizeWorkspaceHandle, client.sendV2, or printV2Payload.

In `@Sources/TerminalController.swift`:
- Around line 3771-3773: The code incorrectly checks ws.focusedPanelId to
determine if selectLastVisitedSurface(inPane:) switched surfaces; instead, read
the selected surface for the target pane before and after calling
ws.selectLastVisitedSurface(inPane: targetPaneId) and compare those values (e.g.
obtain ws.selectedSurface(forPane: targetPaneId) or equivalent) so you only
report success when the pane's selected surface actually changed; do not rely on
ws.focusedPanelId because selectLastVisitedSurface(inPane:) may change focus
even when no alternate surface exists.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 8f4fb054-b7d7-4568-b212-40b06592aedb

📥 Commits

Reviewing files that changed from the base of the PR and between d1c06dd and e3f7fc3.

📒 Files selected for processing (3)
  • CLI/cmux.swift
  • Sources/TerminalController.swift
  • Sources/Workspace.swift

Comment on lines +5985 to +6002
case "last-tab":
let workspaceArg = workspaceFromArgsOrEnv(commandArgs, windowOverride: windowOverride)
var params: [String: Any] = [:]
let wsId = try normalizeWorkspaceHandle(workspaceArg, client: client)
if let wsId { params["workspace_id"] = wsId }
let payload = try client.sendV2(method: "surface.last", params: params)
var summaryParts = ["OK"]
if let tabHandle = formatTabHandle(payload, idFormat: idFormat) {
summaryParts.append("tab=\(tabHandle)")
}
if let paneHandle = formatHandle(payload, kind: "pane", idFormat: idFormat) {
summaryParts.append("pane=\(paneHandle)")
}
if let workspaceHandle = formatHandle(payload, kind: "workspace", idFormat: idFormat) {
summaryParts.append("workspace=\(workspaceHandle)")
}
printV2Payload(payload, jsonOutput: jsonOutput, idFormat: idFormat, fallbackText: summaryParts.joined(separator: " "))

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Reject unexpected args for last-tab.

Right now stray positional tokens and misspelled flags are ignored, and the command still switches the current workspace's tab. That makes typos silently do the wrong thing instead of failing fast.

🛠️ Suggested fix
         case "last-tab":
-            let workspaceArg = workspaceFromArgsOrEnv(commandArgs, windowOverride: windowOverride)
+            let (workspaceOpt, remaining) = parseOption(commandArgs, name: "--workspace")
+            if let unknown = remaining.first(where: { $0.hasPrefix("--") }) {
+                throw CLIError(message: "last-tab: unknown flag '\(unknown)'. Known flags: --workspace <id|ref|index>")
+            }
+            if let extra = remaining.first {
+                throw CLIError(message: "last-tab: unexpected argument '\(extra)'")
+            }
+            let workspaceArg = workspaceOpt ?? (windowOverride == nil ? ProcessInfo.processInfo.environment["CMUX_WORKSPACE_ID"] : nil)
             var params: [String: Any] = [:]
             let wsId = try normalizeWorkspaceHandle(workspaceArg, client: client)
             if let wsId { params["workspace_id"] = wsId }
             let payload = try client.sendV2(method: "surface.last", params: params)
             var summaryParts = ["OK"]
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@CLI/cmux.swift` around lines 5985 - 6002, The "last-tab" case must reject
unexpected positional tokens or unknown flags instead of silently proceeding;
before calling workspaceFromArgsOrEnv/normalizeWorkspaceHandle/sendV2, validate
commandArgs contains only the allowed options (e.g., the workspace flag) and no
stray positional tokens—if any unexpected args are present, print a clear error
and exit non-zero (or return a failure) so the command fails fast; implement
this check at the top of the "last-tab" case using the existing commandArgs
variable and short-circuit before invoking workspaceFromArgsOrEnv,
normalizeWorkspaceHandle, client.sendV2, or printV2Payload.

Comment on lines +3771 to +3773
let before = ws.focusedPanelId
ws.selectLastVisitedSurface(inPane: targetPaneId)
guard let after = ws.focusedPanelId, after != before else { return }
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Compare the target pane’s selected surface, not ws.focusedPanelId.

When pane_id points at a different pane with no alternate surface, Workspace.selectLastVisitedSurface(inPane:) still focuses that pane before it returns. This code then sees ws.focusedPanelId change and reports success even though no last-visited switch happened. The result is a false OK plus an unintended pane-focus mutation.

🛠️ Suggested fix
-            let before = ws.focusedPanelId
+            let before = (targetPaneId ?? ws.bonsplitController.focusedPaneId)
+                .flatMap { ws.bonsplitController.selectedTab(inPane: $0) }
+                .flatMap { ws.panelIdFromSurfaceId($0.id) }
             ws.selectLastVisitedSurface(inPane: targetPaneId)
-            guard let after = ws.focusedPanelId, after != before else { return }
+            guard let resolvedPaneId = targetPaneId ?? ws.bonsplitController.focusedPaneId,
+                  let after = ws.bonsplitController.selectedTab(inPane: resolvedPaneId)
+                    .flatMap({ ws.panelIdFromSurfaceId($0.id) }),
+                  after != before else { return }

Cross-file note: Sources/Workspace.swift focuses the target pane before it checks whether that pane actually has an MRU alternate surface.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Sources/TerminalController.swift` around lines 3771 - 3773, The code
incorrectly checks ws.focusedPanelId to determine if
selectLastVisitedSurface(inPane:) switched surfaces; instead, read the selected
surface for the target pane before and after calling
ws.selectLastVisitedSurface(inPane: targetPaneId) and compare those values (e.g.
obtain ws.selectedSurface(forPane: targetPaneId) or equivalent) so you only
report success when the pane's selected surface actually changed; do not rely on
ws.focusedPanelId because selectLastVisitedSurface(inPane:) may change focus
even when no alternate surface exists.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature] Last visited workspace/tab

1 participant