Repository navigation
fix: goto_split:previous/next cycle through all panes with wrapping - #2639
Conversation
Add tests verifying that goto_split:previous and goto_split:next cycle through all panes regardless of split direction (horizontal and vertical) and wrap at the ends. Uses Ghostty's default keybinds (Cmd+]/[). Extends the goto_split test infrastructure with a three_pane_terminal layout mode (CMUX_UI_TEST_GOTO_SPLIT_LAYOUT=three_pane_terminal) and a cycle navigation recorder for test observability. These tests are expected to FAIL without the accompanying fix, because goto_split:previous/next currently map to directional left/right navigation which skips vertically-split panes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Previously, goto_split:previous and goto_split:next were mapped to directional left/right navigation in Bonsplit, which only found spatially adjacent panes and skipped vertically-split panes entirely. This adds cycle-based navigation that traverses all panes in tree order (using Bonsplit's allPaneIds) and wraps around at the ends, matching Ghostty's intended behavior for these actions. Changes: - Workspace.cycleFocus(forward:) traverses allPaneIds with wrapping - TabManager.cycleSplitFocus delegates to Workspace.cycleFocus - GhosttyTerminalView.handleAction routes PREVIOUS/NEXT through cycle navigation instead of mapping to directional .left/.right - focusDirection() no longer handles PREVIOUS/NEXT cases Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@mykmelez is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
|
This review could not be run because your cubic account has exceeded the monthly review limit. If you need help restoring access, please contact contact@cubic.dev. |
|
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 deterministic cycle-based pane-focus navigation routed via tab-specific managers, implements Workspace/TabManager cycle APIs and test-handshake recording, and introduces a three-pane UI test suite that validates next/previous cycling and wraparound. Changes
Sequence DiagramsequenceDiagram
actor User
participant GhosttyTerminalView as "GhosttyTerminalView"
participant TabManager as "TabManager"
participant Workspace as "Workspace"
participant Bonsplit as "bonsplitController"
participant AppDelegate as "AppDelegate"
User->>GhosttyTerminalView: GOTO_SPLIT_NEXT / PREVIOUS
GhosttyTerminalView->>TabManager: cycleSplitFocus(tabId, forward)
TabManager->>Workspace: cycleFocus(forward)
Workspace->>Bonsplit: unfocus(currentPane)
Workspace->>Bonsplit: focusPane(targetPane)
Workspace->>Workspace: applyTabSelection(tabId:inPane:)
Workspace-->>TabManager: return success
TabManager-->>GhosttyTerminalView: return Bool
GhosttyTerminalView->>AppDelegate: recordGotoSplitCycleMoveIfNeeded(tabId, forward) (DEBUG)
AppDelegate->>AppDelegate: write snapshot JSON for UI tests
GhosttyTerminalView-->>User: focus updated
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
Sources/Workspace.swift (1)
10098-10114: Use tree traversal instead ofallPaneIdsfor the cycle order.This method promises tree-order traversal, but it currently builds the sequence from
bonsplitController.allPaneIds. The same file already usesSidebarBranchOrdering.orderedPaneIds(tree:)when pane order matters. IfallPaneIdsever diverges from tree order after split/move churn,goto_split:previous/nextwill walk panes in the wrong sequence. Please either derive the list fromtreeSnapshot()here or verify that Bonsplit guaranteesallPaneIdsis tree-ordered.♻️ Suggested change
- let allPaneIds = bonsplitController.allPaneIds + let orderedPaneIdStrings = SidebarBranchOrdering.orderedPaneIds( + tree: bonsplitController.treeSnapshot() + ) + let panesById = Dictionary( + uniqueKeysWithValues: bonsplitController.allPaneIds.map { ($0.id.uuidString, $0) } + ) + let allPaneIds = orderedPaneIdStrings.compactMap { panesById[$0] }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 10098 - 10114, The current navigation uses bonsplitController.allPaneIds which may not reflect tree-order; change the sequence source to a tree-ordered list by calling treeSnapshot() and passing it to SidebarBranchOrdering.orderedPaneIds(tree:) (or otherwise derive ordered pane ids from the tree) instead of allPaneIds; keep the rest of the logic (unfocusing via focusedPanelId/panels and computing targetIndex) identical but replace references to allPaneIds with the ordered list so goto_split previous/next walks panes in true tree order.cmuxUITests/GotoSplitCycleUITests.swift (4)
12-17: Add tearDown to remove the test data file.The test creates a JSON file at
dataPathbut never cleans it up. Adding atearDownmethod ensures test artifacts don't accumulate on disk, especially during repeated local test runs.♻️ Proposed fix
override func setUp() { super.setUp() continueAfterFailure = false dataPath = "/tmp/cmux-ui-test-goto-split-cycle-\(UUID().uuidString).json" try? FileManager.default.removeItem(atPath: dataPath) } + + override func tearDown() { + try? FileManager.default.removeItem(atPath: dataPath) + super.tearDown() + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxUITests/GotoSplitCycleUITests.swift` around lines 12 - 17, Add a tearDown method to remove the temporary JSON created in setUp: implement override func tearDown() { try? FileManager.default.removeItem(atPath: dataPath); super.tearDown() } so the file referenced by dataPath is deleted after each test; ensure you call super.tearDown() and use the same FileManager removal logic as in setUp to avoid leftover artifacts.
37-41: Guard against missing dictionary keys to avoid test crashes.The
waitForDatacall confirms keys exist, butloadData()is called again afterward. If the file changes between calls or parsing differs, these force unwraps will crash without a useful diagnostic. Usingguard letprovides clearer failure messages.♻️ Proposed fix
- let allPaneIds = Set(setup["allPaneIds"]!.split(separator: ",").map(String.init)) + guard let allPaneIdsRaw = setup["allPaneIds"] else { + XCTFail("Missing allPaneIds in setup data") + return + } + let allPaneIds = Set(allPaneIdsRaw.split(separator: ",").map(String.init)) XCTAssertEqual(allPaneIds.count, 3, "Expected 3 distinct pane IDs") - let startPane = setup["focusedPaneId"]! + guard let startPane = setup["focusedPaneId"] else { + XCTFail("Missing focusedPaneId in setup data") + return + } XCTAssertTrue(allPaneIds.contains(startPane), "Start pane should be in allPaneIds")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxUITests/GotoSplitCycleUITests.swift` around lines 37 - 41, Replace the force-unwrapped dictionary accesses after loadData() with safe guards: instead of using setup["allPaneIds"]! and setup["focusedPaneId"]! directly (used when computing allPaneIds and startPane), add guard let statements to unwrap setup["allPaneIds"] and setup["focusedPaneId"] (and ensure the split/map result exists) and call XCTFail with a clear diagnostic and return if missing; this change should be made in the test function that calls loadData()/waitForData() so the failure is descriptive rather than crashing (reference symbols: loadData(), waitForData(), allPaneIds, startPane).
85-88: Same force unwrap issue as thenexttest.Apply the same guard-let pattern here for consistency and better failure diagnostics.
♻️ Proposed fix
- let allPaneIds = Set(setup["allPaneIds"]!.split(separator: ",").map(String.init)) + guard let allPaneIdsRaw = setup["allPaneIds"] else { + XCTFail("Missing allPaneIds in setup data") + return + } + let allPaneIds = Set(allPaneIdsRaw.split(separator: ",").map(String.init)) XCTAssertEqual(allPaneIds.count, 3, "Expected 3 distinct pane IDs") - let startPane = setup["focusedPaneId"]! + guard let startPane = setup["focusedPaneId"] else { + XCTFail("Missing focusedPaneId in setup data") + return + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxUITests/GotoSplitCycleUITests.swift` around lines 85 - 88, The test currently force-unwraps values from setup (allPaneIds and focusedPaneId); change to a guard-let pattern to safely unwrap setup["allPaneIds"] and setup["focusedPaneId"], call XCTFail with a clear message and return if either is missing, then proceed to build allPaneIds (split/map) and assert count and use startPane; update references to use the unwrapped variables (e.g., use the local allPaneIdsString and startPane variables instead of force-unwrapped setup accesses).
69-112: Consider extracting shared test logic into a parameterized helper.Both tests share ~90% identical code, differing only in the key (
"]"vs"[") and direction name in assertions. A helper likeverifyCycleNavigation(key:directionName:)would reduce duplication and make adding edge-case tests easier.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxUITests/GotoSplitCycleUITests.swift` around lines 69 - 112, The two nearly identical tests should be consolidated by extracting the shared logic into a parameterized helper (e.g., verifyCycleNavigation(key: String, directionName: String)); move the setup/teardown, launchWithThreePaneLayout() call, the waitForData checks, loadData usage, the loop that types app.typeKey(...), waitForDataMatch, and the final assertions into that helper, and then replace each testGotoSplitPreviousCyclesAllPanes and its counterpart with a single one-line call to verifyCycleNavigation(key: "[", directionName: "previous") (and the other test calling with key: "]" and directionName: "next"); ensure helper references functions/locals like launchWithThreePaneLayout(), waitForData(...), waitForDataMatch(...), loadData(), and asserted variables such as allPaneIds, focusedPaneId and visited so the behavior and assertion text remain the same.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmuxUITests/GotoSplitCycleUITests.swift`:
- Around line 193-203: The helper launchAndEnsureForeground currently returns
success when app.state == .runningBackground, which contradicts its name and can
cause flaky input failures; update it to either (A) rename the function to
launchAndEnsureRunning (or similar) if background is an acceptable end state, or
(B) ensure the app is actually foreground by calling app.activate() (or another
activation attempt) when app.state == .runningBackground and re-checking
app.state before returning, keeping the existing XCTExpectFailure wrapper around
app.launch() and preserving the failure path that calls XCTFail with state info.
In `@Sources/GhosttyTerminalView.swift`:
- Around line 2839-2841: The code uses AppDelegate.shared?.tabManager which
biases to the active window; instead resolve the TabManager that actually owns
the given tabId before calling cycleSplitFocus(tabId:forward:). Replace the
AppDelegate.shared?.tabManager lookup with the cross-window resolver used
elsewhere (e.g. the same lookup pattern as in Sources/TerminalController.swift —
a TabManager lookup by tabId like tabManagerForTab(tabId:) or similar), then
call the found TabManager.cycleSplitFocus(tabId: forward:); if no owning manager
is found, fall back gracefully (no-op or the existing behavior).
---
Nitpick comments:
In `@cmuxUITests/GotoSplitCycleUITests.swift`:
- Around line 12-17: Add a tearDown method to remove the temporary JSON created
in setUp: implement override func tearDown() { try?
FileManager.default.removeItem(atPath: dataPath); super.tearDown() } so the file
referenced by dataPath is deleted after each test; ensure you call
super.tearDown() and use the same FileManager removal logic as in setUp to avoid
leftover artifacts.
- Around line 37-41: Replace the force-unwrapped dictionary accesses after
loadData() with safe guards: instead of using setup["allPaneIds"]! and
setup["focusedPaneId"]! directly (used when computing allPaneIds and startPane),
add guard let statements to unwrap setup["allPaneIds"] and
setup["focusedPaneId"] (and ensure the split/map result exists) and call XCTFail
with a clear diagnostic and return if missing; this change should be made in the
test function that calls loadData()/waitForData() so the failure is descriptive
rather than crashing (reference symbols: loadData(), waitForData(), allPaneIds,
startPane).
- Around line 85-88: The test currently force-unwraps values from setup
(allPaneIds and focusedPaneId); change to a guard-let pattern to safely unwrap
setup["allPaneIds"] and setup["focusedPaneId"], call XCTFail with a clear
message and return if either is missing, then proceed to build allPaneIds
(split/map) and assert count and use startPane; update references to use the
unwrapped variables (e.g., use the local allPaneIdsString and startPane
variables instead of force-unwrapped setup accesses).
- Around line 69-112: The two nearly identical tests should be consolidated by
extracting the shared logic into a parameterized helper (e.g.,
verifyCycleNavigation(key: String, directionName: String)); move the
setup/teardown, launchWithThreePaneLayout() call, the waitForData checks,
loadData usage, the loop that types app.typeKey(...), waitForDataMatch, and the
final assertions into that helper, and then replace each
testGotoSplitPreviousCyclesAllPanes and its counterpart with a single one-line
call to verifyCycleNavigation(key: "[", directionName: "previous") (and the
other test calling with key: "]" and directionName: "next"); ensure helper
references functions/locals like launchWithThreePaneLayout(), waitForData(...),
waitForDataMatch(...), loadData(), and asserted variables such as allPaneIds,
focusedPaneId and visited so the behavior and assertion text remain the same.
In `@Sources/Workspace.swift`:
- Around line 10098-10114: The current navigation uses
bonsplitController.allPaneIds which may not reflect tree-order; change the
sequence source to a tree-ordered list by calling treeSnapshot() and passing it
to SidebarBranchOrdering.orderedPaneIds(tree:) (or otherwise derive ordered pane
ids from the tree) instead of allPaneIds; keep the rest of the logic (unfocusing
via focusedPanelId/panels and computing targetIndex) identical but replace
references to allPaneIds with the ordered list so goto_split previous/next walks
panes in true tree order.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: c787c334-ac1d-44d4-92b4-fa1ea9a4dd44
📒 Files selected for processing (6)
GhosttyTabs.xcodeproj/project.pbxprojSources/AppDelegate.swiftSources/GhosttyTerminalView.swiftSources/TabManager.swiftSources/Workspace.swiftcmuxUITests/GotoSplitCycleUITests.swift
Greptile SummaryReplaces the stub
Confidence Score: 4/5The cycle navigation logic is correct and multi-window tab manager resolution is properly handled; the one area needing attention is the asyncAfter polling loop in setupThreePaneTerminalLayout, which ships in the production binary gated only by an env-var check and is captured in the existing review comment. The production Workspace.cycleFocus, TabManager forwarders, and GhosttyTerminalView action dispatch are all clean. The setupThreePaneTerminalLayout function introduced in AppDelegate.swift contains an asyncAfter polling loop (up to 60 iterations over 6 s) compiled into the production binary; the existing review comment covers this. No other blocking issues were found. Sources/AppDelegate.swift — specifically the setupThreePaneTerminalLayout function and its DispatchQueue.main.asyncAfter retry loop. Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant NSWindow
participant AppDelegate
participant GhosttyNSView
participant GhosttyTerminalView
participant TabManager
participant Workspace
User->>NSWindow: keyDown (Cmd+] / Cmd+[)
NSWindow->>AppDelegate: shouldRouteGhosttyGotoSplitCycleShortcutToTerminal(event)
AppDelegate-->>NSWindow: true (shortcut matches)
NSWindow->>GhosttyNSView: performKeyEquivalentAfterMenuMiss(event)
GhosttyNSView->>GhosttyTerminalView: handleAction(GHOSTTY_ACTION_GOTO_SPLIT, previous/next)
GhosttyTerminalView->>AppDelegate: tabManagerFor(tabId)
AppDelegate-->>GhosttyTerminalView: TabManager (multi-window resolved)
GhosttyTerminalView->>TabManager: cycleSplitFocus(tabId:, forward:)
TabManager->>Workspace: cycleFocus(forward:)
Workspace->>Workspace: unfocus() current panel
Workspace->>Workspace: compute targetIndex (wrapping)
Workspace->>Workspace: bonsplitController.focusPane(targetPaneId)
Workspace->>Workspace: applyTabSelection(tabId:, inPane:)
GhosttyNSView-->>NSWindow: true (consumed)
Note over AppDelegate,TabManager: Non-terminal path (browser pane focused)
User->>AppDelegate: performShortcut(event)
AppDelegate->>AppDelegate: matchesGhosttyGotoSplitNext/PreviousShortcut
AppDelegate->>AppDelegate: cmuxRememberFindSelectionBeforePanelFocusMove
AppDelegate->>TabManager: cyclePaneFocus(forward:)
TabManager->>Workspace: cycleFocus(forward:)
Reviews (3): Last reviewed commit: "Merge remote-tracking branch 'manaflow/m..." | Re-trigger Greptile |
The setupThreePaneTerminalLayout helper was writing setupComplete immediately after creating splits, before a terminal surface became first responder. Ghostty keybinds only fire when GhosttyNSView has focus, so early keystrokes could miss. Now waits for .ghosttyDidFocusSurface and verifies a terminal panel is focused before signaling readiness, matching the pattern used by the existing browser split setup. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Use tabManagerFor(tabId:) instead of AppDelegate.shared?.tabManager so that goto_split:previous/next routes to the correct window's TabManager in multi-window scenarios, rather than biasing toward the active window. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Addressed two of the CodeRabbit review comments in follow-up commits:
The |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Sources/AppDelegate.swift (1)
8588-8625:⚠️ Potential issue | 🟠 MajorStill gate readiness on the terminal’s actual keyboard target.
window.firstResponder is NSViewis too weak here. The poll can still writesetupCompletewhile some unrelated view owns focus, and a stale scheduled poll can keep running after a successful notification path. That reintroduces the same flake this helper is meant to remove. Resolve the window from the focused terminal panel and use its hosted view’s preferred-focus check, with aresolvedguard so pending polls stop once setup succeeds.Suggested fix
- var observer: NSObjectProtocol? + var observer: NSObjectProtocol? + var resolved = false let deadline = Date().addingTimeInterval(6.0) func checkAndSignal() { + guard !resolved else { return } guard Date() < deadline else { if let observer { NotificationCenter.default.removeObserver(observer) } + resolved = true self.writeGotoSplitTestData(["setupError": "Timed out waiting for terminal focus"]) return } guard let focusedPanelId = tab.focusedPanelId, - tab.terminalPanel(for: focusedPanelId) != nil, - let window = NSApp.mainWindow ?? NSApp.keyWindow, - window.firstResponder is NSView else { + let terminalPanel = tab.terminalPanel(for: focusedPanelId), + let window = terminalPanel.hostedView.window, + terminalPanel.hostedView.responderMatchesPreferredKeyboardFocus(window.firstResponder) else { DispatchQueue.main.asyncAfter(deadline: .now() + 0.1) { checkAndSignal() } return } if let observer { NotificationCenter.default.removeObserver(observer) } + resolved = true let allPaneIds = tab.bonsplitController.allPaneIds.map(\.description)Based on learnings: In
Sources/AppDelegate.swift, determine whether to repair focus by asking the focused terminal’s hosted view if the current first responder already matches that panel’s preferred keyboard target.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 8588 - 8625, The poll currently uses a weak check (window.firstResponder is NSView) and can report setupComplete while an unrelated view is focused and leave scheduled polls running; update checkAndSignal to resolve the focused terminal panel via tab.focusedPanelId and tab.terminalPanel(for:), obtain that panel’s hosted view / preferred keyboard target (e.g. the hosted view’s preferredFocus or preferredKeyboardTarget API), and only consider readiness true when window.firstResponder matches that specific preferred target; when readiness succeeds remove the observer and set a local resolved flag so any pending DispatchQueue.asyncAfter callbacks return early and do not re-send setupComplete or keep polling. Ensure observer removal happens in both success and timeout paths and reference checkAndSignal, observer, tab.focusedPanelId, tab.terminalPanel(for:), and bonsplitController.focusedPaneId in your changes.
🤖 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/AppDelegate.swift`:
- Around line 8588-8625: The poll currently uses a weak check
(window.firstResponder is NSView) and can report setupComplete while an
unrelated view is focused and leave scheduled polls running; update
checkAndSignal to resolve the focused terminal panel via tab.focusedPanelId and
tab.terminalPanel(for:), obtain that panel’s hosted view / preferred keyboard
target (e.g. the hosted view’s preferredFocus or preferredKeyboardTarget API),
and only consider readiness true when window.firstResponder matches that
specific preferred target; when readiness succeeds remove the observer and set a
local resolved flag so any pending DispatchQueue.asyncAfter callbacks return
early and do not re-send setupComplete or keep polling. Ensure observer removal
happens in both success and timeout paths and reference checkAndSignal,
observer, tab.focusedPanelId, tab.terminalPanel(for:), and
bonsplitController.focusedPaneId in your changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 08d7ce14-6eb5-4093-b58d-d662d203bac4
📒 Files selected for processing (2)
Sources/AppDelegate.swiftSources/GhosttyTerminalView.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/GhosttyTerminalView.swift
The checkAndSignal poll and .ghosttyDidFocusSurface observer could both fire and write setupComplete twice. Add a resolved flag so the first successful path short-circuits subsequent invocations. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
Sources/AppDelegate.swift (1)
8655-8668:⚠️ Potential issue | 🟠 MajorStill gate
setupCompleteon the target terminal actually owning first responder.
window.firstResponder is NSViewis too weak here. Any view in the key/main window satisfies it, so this can still writesetupCompletebefore the selectedTerminalPanelowns keyboard focus, which putsCmd+]/Cmd+[back into the same flaky race with AppKit.Proposed fix
func checkAndSignal() { guard !resolved else { return } guard Date() < deadline else { if let observer { NotificationCenter.default.removeObserver(observer) } resolved = true self.writeGotoSplitTestData(["setupError": "Timed out waiting for terminal focus"]) return } - guard let focusedPanelId = tab.focusedPanelId, - tab.terminalPanel(for: focusedPanelId) != nil, - let window = NSApp.mainWindow ?? NSApp.keyWindow, - window.firstResponder is NSView else { + guard let focusedPanelId = tab.focusedPanelId, + let terminalPanel = tab.terminalPanel(for: focusedPanelId), + let window = terminalPanel.hostedView.window, + let firstResponder = window.firstResponder, + terminalPanel.hostedView.responderMatchesPreferredKeyboardFocus(firstResponder) else { DispatchQueue.main.asyncAfter(deadline: .now() + 0.1) { checkAndSignal() } return }Based on learnings: In
AppDelegate.repairFocusedTerminalKeyboardRoutingIfNeeded(window:event:), determine whether to repair focus by asking the focused terminal’s hosted view if the current first responder already matches that panel’s preferred keyboard target.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 8655 - 8668, The current first-responder check in checkAndSignal() is too permissive; replace the window.firstResponder is NSView guard with a check that the focused TerminalPanel actually owns the keyboard focus — e.g. get the panel via tab.terminalPanel(for: focusedPanelId) and verify window.firstResponder === terminalPanel.preferredKeyboardTarget (or that terminalPanel.hostedView.isFirstResponder / the panel’s hosted view reports itself as the preferred keyboard responder) before proceeding to write setupComplete; if it isn’t, continue the async retry path. Also follow the same decision logic used in AppDelegate.repairFocusedTerminalKeyboardRoutingIfNeeded(window:event:) when determining the panel’s preferred keyboard target.Sources/GhosttyTerminalView.swift (1)
2982-2990:⚠️ Potential issue | 🟠 MajorResolve directional split focus through the owning
TabManagertoo.Lines 2988-2990 still route
up/down/left/rightthrough the active window’stabManager, so directional split navigation can still hit the wrong window in multi-window sessions.Suggested fix
return performOnMain { - guard let tabManager = AppDelegate.shared?.tabManager else { return false } + guard let app = AppDelegate.shared, + let tabManager = app.tabManagerFor(tabId: tabId) ?? app.tabManager else { + return false + } return tabManager.moveSplitFocus(tabId: tabId, surfaceId: surfaceId, direction: direction) }Based on learnings:
sendPickedElementToTerminal(workspaceId:summary:)resolves targets across all mainWindowContexts instead of using the active window manager, because the activetabManageris window-biased.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/GhosttyTerminalView.swift` around lines 2982 - 2990, The code currently always uses AppDelegate.shared?.tabManager (window-biased) which can target the wrong window; instead resolve the owning TabManager for the surface before calling moveSplitFocus. In the performOnMain block, obtain the TabManager from the surfaceView/terminalSurface owner (e.g. the surface's owning window or terminalSurface.owningTabManager / surfaceView.window/windowController that exposes a tabManager) and call tabManager.moveSplitFocus(tabId: tabId, surfaceId: surfaceId, direction: direction) on that resolved manager rather than AppDelegate.shared?.tabManager so directional split focus is routed to the correct window.
🤖 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/AppDelegate.swift`:
- Around line 9620-9627: recordGotoSplitCycleMoveIfNeeded currently uses
AppDelegate.tabManager.selectedWorkspace which can snapshot the wrong workspace
after multi-window routing; change the function to accept (or resolve) the
triggering tabId and use that tabId to find the correct TabManager and workspace
before calling gotoSplitFindStateSnapshot and writeGotoSplitTestData (preserve
the isGotoSplitUITestRecordingEnabled guard). Also update the
GhosttyTerminalView.swift call site to pass the triggering tabId into
recordGotoSplitCycleMoveIfNeeded so diagnostics reflect the actual
cycleSplitFocus target.
---
Duplicate comments:
In `@Sources/AppDelegate.swift`:
- Around line 8655-8668: The current first-responder check in checkAndSignal()
is too permissive; replace the window.firstResponder is NSView guard with a
check that the focused TerminalPanel actually owns the keyboard focus — e.g. get
the panel via tab.terminalPanel(for: focusedPanelId) and verify
window.firstResponder === terminalPanel.preferredKeyboardTarget (or that
terminalPanel.hostedView.isFirstResponder / the panel’s hosted view reports
itself as the preferred keyboard responder) before proceeding to write
setupComplete; if it isn’t, continue the async retry path. Also follow the same
decision logic used in
AppDelegate.repairFocusedTerminalKeyboardRoutingIfNeeded(window:event:) when
determining the panel’s preferred keyboard target.
In `@Sources/GhosttyTerminalView.swift`:
- Around line 2982-2990: The code currently always uses
AppDelegate.shared?.tabManager (window-biased) which can target the wrong
window; instead resolve the owning TabManager for the surface before calling
moveSplitFocus. In the performOnMain block, obtain the TabManager from the
surfaceView/terminalSurface owner (e.g. the surface's owning window or
terminalSurface.owningTabManager / surfaceView.window/windowController that
exposes a tabManager) and call tabManager.moveSplitFocus(tabId: tabId,
surfaceId: surfaceId, direction: direction) on that resolved manager rather than
AppDelegate.shared?.tabManager so directional split focus is routed to the
correct window.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: d4f8205d-f192-4d54-96aa-301ea3e30be1
📒 Files selected for processing (4)
Sources/AppDelegate.swiftSources/GhosttyTerminalView.swiftSources/TabManager.swiftSources/Workspace.swift
🚧 Files skipped from review as they are similar to previous changes (2)
- Sources/TabManager.swift
- Sources/Workspace.swift
recordGotoSplitCycleMoveIfNeeded now accepts tabId and resolves the workspace via tabManagerFor(tabId:), consistent with how cycleSplitFocus itself is routed. Previously it used the active window's tabManager, which could snapshot the wrong workspace in multi-window scenarios. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Sources/AppDelegate.swift (1)
8649-8689:⚠️ Potential issue | 🟠 MajorTighten setup-complete gating to the target terminal responder.
checkAndSignal()still treats any.ghosttyDidFocusSurfacepluswindow.firstResponder is NSViewas success. During XCTest startup/fallback-window churn, an unrelated Ghostty focus event can satisfy that and marksetupCompletebefore this three-pane tab actually owns keyboard focus, which makes the firstCmd+]/Cmd+[flaky again.Proposed fix
- func checkAndSignal() { + func checkAndSignal(notification: Notification? = nil) { guard !resolved else { return } guard Date() < deadline else { if let observer { NotificationCenter.default.removeObserver(observer) } resolved = true self.writeGotoSplitTestData(["setupError": "Timed out waiting for terminal focus"]) return } - guard let focusedPanelId = tab.focusedPanelId, - tab.terminalPanel(for: focusedPanelId) != nil, - let window = NSApp.mainWindow ?? NSApp.keyWindow, - window.firstResponder is NSView else { - DispatchQueue.main.asyncAfter(deadline: .now() + 0.1) { checkAndSignal() } + if let notifiedTabId = notification?.userInfo?[GhosttyNotificationKey.tabId] as? UUID, + notifiedTabId != tab.id { + return + } + guard let focusedPanelId = tab.focusedPanelId, + let terminalPanel = tab.terminalPanel(for: focusedPanelId), + let window = tabManager.window ?? self.windowId(for: tabManager).flatMap(self.mainWindow(for:)), + terminalPanel.hostedView.window === window, + terminalPanel.hostedView.responderMatchesPreferredKeyboardFocus(window.firstResponder) else { + DispatchQueue.main.asyncAfter(deadline: .now() + 0.1) { checkAndSignal(notification: nil) } return } if let observer { NotificationCenter.default.removeObserver(observer) } resolved = true @@ observer = NotificationCenter.default.addObserver( forName: .ghosttyDidFocusSurface, object: nil, queue: .main - ) { _ in checkAndSignal() } + ) { note in checkAndSignal(notification: note) } // Also poll in case the notification already fired before we observed. - DispatchQueue.main.asyncAfter(deadline: .now() + 0.2) { checkAndSignal() } + DispatchQueue.main.asyncAfter(deadline: .now() + 0.2) { checkAndSignal(notification: nil) }Based on learnings: In
Sources/AppDelegate.swift, keyboard focus checks should usehostedView.responderMatchesPreferredKeyboardFocus(responder)so the responder is verified against the intended terminal surface.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 8649 - 8689, The gating in checkAndSignal is too permissive: replace the generic "window.firstResponder is NSView" check with a verification that the terminal surface actually owns keyboard focus by locating the terminal's hostedView (from tab.terminalPanel(for: focusedPanelId) or similar) and calling hostedView.responderMatchesPreferredKeyboardFocus(responder) against window.firstResponder; keep the existing timeout/observer removal logic and only treat the check as successful when responderMatchesPreferredKeyboardFocus returns true so the writeGotoSplitTestData("setupComplete": "true") is emitted only for the target terminal responder.
🤖 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/AppDelegate.swift`:
- Around line 8649-8689: The gating in checkAndSignal is too permissive: replace
the generic "window.firstResponder is NSView" check with a verification that the
terminal surface actually owns keyboard focus by locating the terminal's
hostedView (from tab.terminalPanel(for: focusedPanelId) or similar) and calling
hostedView.responderMatchesPreferredKeyboardFocus(responder) against
window.firstResponder; keep the existing timeout/observer removal logic and only
treat the check as successful when responderMatchesPreferredKeyboardFocus
returns true so the writeGotoSplitTestData("setupComplete": "true") is emitted
only for the target terminal responder.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9ad817bb-0737-431e-93ae-76ab0d77c40b
📒 Files selected for processing (2)
Sources/AppDelegate.swiftSources/GhosttyTerminalView.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/GhosttyTerminalView.swift
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
Sources/AppDelegate.swift (1)
7079-7083:⚠️ Potential issue | 🟡 MinorRequire the actual terminal surface to be first responder before signaling ready.
This still passes when any
NSViewis first responder, sosetupCompletecan be written before Ghostty receivesCmd+]/Cmd+[keybinds. Gate on the focused terminal hosted view instead.Proposed fix
- guard let focusedPanelId = tab.focusedPanelId, - tab.terminalPanel(for: focusedPanelId) != nil, - let window = NSApp.mainWindow ?? NSApp.keyWindow, - window.firstResponder is NSView else { + guard let focusedPanelId = tab.focusedPanelId, + let terminalPanel = tab.terminalPanel(for: focusedPanelId), + terminalPanel.hostedView.isSurfaceViewFirstResponder() else { DispatchQueue.main.asyncAfter(deadline: .now() + 0.1) { checkAndSignal() } return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 7079 - 7083, The current guard only checks window.firstResponder is any NSView; instead fetch the terminal panel via tab.terminalPanel(for: focusedPanelId) and verify the actual hosted terminal view is the first responder (e.g. compare window.firstResponder === terminalPanel.hostedView or test that the first responder is a descendant of terminalPanel.hostedView) before calling checkAndSignal(), so setupComplete is only signaled when the focused terminal surface truly has focus.
🤖 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/GhosttyTerminalView.swift`:
- Around line 3121-3124: The DEBUG call to app.recordGotoSplitCycleMoveIfNeeded
runs even when tabManager.cycleSplitFocus(...) returns false, causing tests to
record a move that didn't happen; fix by capturing the Bool result from
cycleSplitFocus(tabId:forward:) and only call
recordGotoSplitCycleMoveIfNeeded(tabId:forward:) when that result is true (i.e.,
wrap the DEBUG call in an if result { ... } using the local result variable).
In `@Sources/TabManager.swift`:
- Around line 5391-5396: Add a DEBUG-only unified debug log call in
cycleSplitFocus to mirror existing focus/split instrumentation: after locating
the Tab and before/after calling tab.cycleFocus(forward:), emit a dlog(...)
wrapped in `#if` DEBUG / `#endif` that records the tab id (tab.id or tabId) and the
direction (forward/backward) and a short event name like "split-focus-cycle" so
this shortcut is traceable in the unified debug log; reference the
cycleSplitFocus function and the tab.cycleFocus(forward:) call when making the
change.
---
Duplicate comments:
In `@Sources/AppDelegate.swift`:
- Around line 7079-7083: The current guard only checks window.firstResponder is
any NSView; instead fetch the terminal panel via tab.terminalPanel(for:
focusedPanelId) and verify the actual hosted terminal view is the first
responder (e.g. compare window.firstResponder === terminalPanel.hostedView or
test that the first responder is a descendant of terminalPanel.hostedView)
before calling checkAndSignal(), so setupComplete is only signaled when the
focused terminal surface truly has focus.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 80ea86c1-2e70-4507-862a-38e690a1a0b0
📒 Files selected for processing (5)
GhosttyTabs.xcodeproj/project.pbxprojSources/AppDelegate.swiftSources/GhosttyTerminalView.swiftSources/TabManager.swiftSources/Workspace.swift
✅ Files skipped from review due to trivial changes (1)
- GhosttyTabs.xcodeproj/project.pbxproj
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/Workspace.swift
|
Coming from iTerm2 I really missed this feature. Hope it can get merged 🤞 |
…e-navigation # Conflicts: # Sources/TabManager.swift # cmux.xcodeproj/project.pbxproj
|
I'm excited about this! Also coming from iTerm, and missing this functionality. |
|
Another data point for merging this: I run cmux as a multi-agent workspace — currently 7 panes in one workspace, each holding a different coding agent. Directional ⌥⌘+arrow navigation degrades badly past ~4 panes: you have to hold a mental map of the split tree to know which arrow lands where. The only non-directional fallback today is focus-history ⌘[ / ⌘], which is MRU rather than positional and collides with browser back/forward in browser surfaces. This PR's wrapping cycle is exactly the missing primitive. Happy to test it on macOS if that helps unstick review. |
…e-navigation # Conflicts: # Sources/AppDelegate.swift # Sources/Workspace.swift # cmux.xcodeproj/project.pbxproj
…e-navigation # Conflicts: # cmux.xcodeproj/project.pbxproj
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
Hi @1vecera , thanks for the offer! I'm not sure if it'll help, as I'm unsure what is sticking review at this point. I've asked @austinywang to review it several times, including in a Discord thread where he asked people to recommend PRs to review (https://discord.com/channels/1324643092963266570/1324643772272742473/threads/1515100799355850832); but he still hasn't done so. @austinywang , any chance you can prioritize this PR the next time you spend some time focusing on reviews? Multiple commenters eagerly await it! |
|
I opened follow-up PR #9046 to complete/supersede this fork PR, with credit to @mykmelez. I built and launched
That visits every pane in stable order and wraps at both ends. Full verification details are in #9046: #9046 (comment) I also replied to and resolved the four previously unresolved review threads on this PR. |
5 participants
Summary
goto_split:previousandgoto_split:nextnow cycle through all panes in tree order regardless of split direction (horizontal/vertical) and wrap at the endsGhosttyTerminalView.swiftacknowledged this: "Bonsplit doesn't have cycle-based navigation")allPaneIdsandfocusPaneAPIs to implement proper cycle traversal without requiring Bonsplit changesTest plan
GotoSplitCycleUITests— creates 3-pane layout (horizontal + vertical splits), verifiesCmd+]andCmd+[visit all panes and wrapgoto_split:nextandgoto_split:previouscycle through all panes in mixed split layoutsBrowserPaneNavigationKeybindUITestsunaffected (directional navigation unchanged)Note: per regression test commit policy, the test commit is first (expected to fail) and the fix commit is second.
🤖 Generated with Claude Code
Summary by cubic
Fixes goto_split:previous/next to cycle through all panes in tree order with wrap-around, and routes these shortcuts to the terminal so Ghostty keybinds work reliably. Directional up/down/left/right stays the same; multi-window routing targets the correct
TabManager.BonsplitallPaneIds/focusPane; addWorkspace.cycleFocusandTabManager.cycleSplitFocus/cyclePaneFocus; remove previous/next fromfocusDirection.TabManagerbytabIdin the action handler so previous/next operate in the correct window; record cycle state from the routed workspace.goto_split:previous/nextkey equivalents to the terminal view before the main menu; load shortcuts from Ghostty config.GotoSplitCycleUITestswith a 3‑pane terminal layout; wait for terminal focus and avoid duplicate setupComplete writes.Written for commit 235ead8. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
New Features
Tests