Implement and stabilize the paper canvas pane strip#1637
Implement and stabilize the paper canvas pane strip#1637lawrencecchen wants to merge 40 commits into
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
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:
📝 WalkthroughWalkthroughReplaces the Bonsplit submodule with a new local Swift package PaneKit and implements a paper-canvas pane layout plus split-tree support. Adds PaneKit package, large controller/model/view surface for pane/canvas behavior, CLI v2 workspace.viewport.pan and pan-workspace CLI, Open Pane Right shortcut/menu, many tests, Xcode updates, and widespread import/type migrations. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AppDelegate
participant TabManager
participant BonsplitController
participant SplitViewController
participant PaperCanvas
User->>AppDelegate: trigger "Open Pane Right" (menu/shortcut)
AppDelegate->>TabManager: performOpenPaneRightShortcut()
TabManager->>BonsplitController: openPaperCanvasPaneRight(targetPane?)
BonsplitController->>SplitViewController: request openPaperCanvasPaneRight(after: paneId)
SplitViewController->>PaperCanvas: insertPaneRight(newPane, compute frames)
PaperCanvas-->>SplitViewController: return new paneId & new viewport/frame
SplitViewController-->>BonsplitController: created paneId
BonsplitController-->>TabManager: return paneId
TabManager->>AppDelegate: notify geometry change / focus
AppDelegate->>User: UI updates rendered (viewport scrolled)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
20 issues found across 78 files
Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed.
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="PaneKit/Sources/PaneKit/Internal/Views/PaperCanvasViewContainer.swift">
<violation number="1" location="PaneKit/Sources/PaneKit/Internal/Views/PaperCanvasViewContainer.swift:60">
P2: Observe both viewport-origin axes for geometry notifications. Tracking only `viewportOrigin.x` skips callbacks when only Y changes.</violation>
<violation number="2" location="PaneKit/Sources/PaneKit/Internal/Views/PaperCanvasViewContainer.swift:88">
P1: Update the viewport frame on global frame changes (origin + size), not only size changes, so moved windows report correct geometry.</violation>
</file>
<file name="docs/superpowers/plans/2026-03-17-horizontal-pane-strip-stabilization.md">
<violation number="1" location="docs/superpowers/plans/2026-03-17-horizontal-pane-strip-stabilization.md:199">
P2: Task 1’s expected test outcome is incorrect: the provided “minimal strip model” cannot satisfy the tests, so `Expected: PASS` is misleading.</violation>
<violation number="2" location="docs/superpowers/plans/2026-03-17-horizontal-pane-strip-stabilization.md:288">
P1: `addPane` appends unconditionally, which breaks ordered-strip insertion when adding to the middle.</violation>
</file>
<file name="PaneKit/Sources/PaneKit/Internal/Views/PaneContainerView.swift">
<violation number="1" location="PaneKit/Sources/PaneKit/Internal/Views/PaneContainerView.swift:340">
P2: Clamp edge thresholds so left/right and top/bottom hit zones do not overlap in small panes; otherwise center/top/bottom drops become unreachable.</violation>
<violation number="2" location="PaneKit/Sources/PaneKit/Internal/Views/PaneContainerView.swift:401">
P2: Clear drop lifecycle/overlay state regardless of external drop handler result to avoid stale hover UI after rejected drops.</violation>
</file>
<file name="PaneKit/Sources/PaneKit/Internal/Views/SplitContainerView.swift">
<violation number="1" location="PaneKit/Sources/PaneKit/Internal/Views/SplitContainerView.swift:733">
P2: Drag-state hit testing is narrower than the divider’s effective drag region, which can misclassify valid divider drags and snap them back.</violation>
</file>
<file name="PaneKit/Sources/PaneKit/Public/Types/LayoutSnapshot.swift">
<violation number="1" location="PaneKit/Sources/PaneKit/Public/Types/LayoutSnapshot.swift:97">
P2: Validate `orientation` and `dividerPosition` in `ExternalSplitNode` so invalid split states cannot be constructed.</violation>
</file>
<file name="Sources/TabManager.swift">
<violation number="1" location="Sources/TabManager.swift:4182">
P2: `open_pane_right` can pass without capturing any motion samples for the newly created pane, so regressions may be missed.</violation>
</file>
<file name="PaneKit/Sources/PaneKit/Internal/Controllers/SplitViewController.swift">
<violation number="1" location="PaneKit/Sources/PaneKit/Internal/Controllers/SplitViewController.swift:109">
P1: Switching from paper-canvas to split-tree discards paper-canvas structural changes because `paperCanvas` is nulled without reconciling `rootNode`.</violation>
<violation number="2" location="PaneKit/Sources/PaneKit/Internal/Controllers/SplitViewController.swift:140">
P2: `Dictionary(uniqueKeysWithValues:)` can crash on duplicate pane IDs in a layout snapshot; validate or safely coalesce duplicates instead of trapping.</violation>
</file>
<file name="PaneKit/Sources/PaneKit/Internal/Views/SplitViewContainer.swift">
<violation number="1" location="PaneKit/Sources/PaneKit/Internal/Views/SplitViewContainer.swift:22">
P2: `containerFrame` is only refreshed on size changes, so window moves (global origin changes) won’t update `containerFrame.origin`. That leaves layout snapshots/geometry consumers with stale absolute coordinates. Observe the full global frame (origin + size) instead of just `geometry.size`.</violation>
</file>
<file name="PaneKit/Sources/PaneKit/Public/BonsplitController.swift">
<violation number="1" location="PaneKit/Sources/PaneKit/Public/BonsplitController.swift:518">
P1: This removes the source tab before validating split preconditions, so a failed split can drop the tab from the UI/model.</violation>
<violation number="2" location="PaneKit/Sources/PaneKit/Public/BonsplitController.swift:565">
P2: `closePane` can report success for non-existent panes, causing incorrect delegate events and state assumptions.</violation>
<violation number="3" location="PaneKit/Sources/PaneKit/Public/BonsplitController.swift:722">
P2: Respect `allowSplits` here; this API currently bypasses split restrictions and can create panes in single-pane/read-only modes.</violation>
</file>
<file name="docs/superpowers/plans/2026-03-16-horizontal-workspace-pane-strip.md">
<violation number="1" location="docs/superpowers/plans/2026-03-16-horizontal-workspace-pane-strip.md:191">
P2: The planned open-pane width formula (`0.66 * viewport`) contradicts the test expectation (800 at 1200 viewport), so the documented implementation cannot satisfy the documented PASS criteria.</violation>
<violation number="2" location="docs/superpowers/plans/2026-03-16-horizontal-workspace-pane-strip.md:613">
P2: The "re-run regression check" command only builds the project and does not run the added socket regression test, so the verification step is ineffective.</violation>
</file>
<file name="Sources/TerminalController.swift">
<violation number="1" location="Sources/TerminalController.swift:5010">
P2: `pane.resize` can change focused pane in paper-canvas mode, even though it is not a focus-intent command.</violation>
</file>
<file name="Sources/Workspace.swift">
<violation number="1" location="Sources/Workspace.swift:2351">
P1: Set `isProgrammaticSplit = true` before calling `openPaperCanvasPaneRight` to prevent the delegate from auto-creating a duplicate terminal in the new pane.</violation>
</file>
<file name="Sources/TerminalWindowPortal.swift">
<violation number="1" location="Sources/TerminalWindowPortal.swift:1561">
P1: The anchor collapse preservation logic causes an infinite loop because `preserveVisibleRetryCount` is continually reset to its maximum budget. Add a condition so it only starts when the budget is uninitialized.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| func addPane(_ pane: PaneState, after targetPaneId: PaneID, requestedWidth: CGFloat) { | ||
| let insertedPaneId = stripState.openPaneRight(after: targetPaneId, requestedWidth: requestedWidth, minimumPaneWidth: 260) | ||
| panes.append(PaperCanvasPane(pane: pane, frame: .zero)) |
There was a problem hiding this comment.
P1: addPane appends unconditionally, which breaks ordered-strip insertion when adding to the middle.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/superpowers/plans/2026-03-17-horizontal-pane-strip-stabilization.md, line 288:
<comment>`addPane` appends unconditionally, which breaks ordered-strip insertion when adding to the middle.</comment>
<file context>
@@ -0,0 +1,662 @@
+
+ func addPane(_ pane: PaneState, after targetPaneId: PaneID, requestedWidth: CGFloat) {
+ let insertedPaneId = stripState.openPaneRight(after: targetPaneId, requestedWidth: requestedWidth, minimumPaneWidth: 260)
+ panes.append(PaperCanvasPane(pane: pane, frame: .zero))
+ syncPlacementsFromStrip()
+ }
</file context>
| layoutStyle = configuration.layoutStyle | ||
| switch layoutStyle { | ||
| case .splitTree: | ||
| paperCanvas = nil |
There was a problem hiding this comment.
P1: Switching from paper-canvas to split-tree discards paper-canvas structural changes because paperCanvas is nulled without reconciling rootNode.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At PaneKit/Sources/PaneKit/Internal/Controllers/SplitViewController.swift, line 109:
<comment>Switching from paper-canvas to split-tree discards paper-canvas structural changes because `paperCanvas` is nulled without reconciling `rootNode`.</comment>
<file context>
@@ -0,0 +1,813 @@
+ layoutStyle = configuration.layoutStyle
+ switch layoutStyle {
+ case .splitTree:
+ paperCanvas = nil
+ case .paperCanvas:
+ enablePaperCanvasLayout()
</file context>
| } | ||
|
|
||
| // Remove from source first. | ||
| sourcePane.removeTab(tabItem.id) |
There was a problem hiding this comment.
P1: This removes the source tab before validating split preconditions, so a failed split can drop the tab from the UI/model.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At PaneKit/Sources/PaneKit/Public/BonsplitController.swift, line 518:
<comment>This removes the source tab before validating split preconditions, so a failed split can drop the tab from the UI/model.</comment>
<file context>
@@ -0,0 +1,1062 @@
+ }
+
+ // Remove from source first.
+ sourcePane.removeTab(tabItem.id)
+
+ if sourcePane.tabs.isEmpty {
</file context>
| let previousFocusedPanelId = focusedPanelId | ||
| let previousHostedView = focusedTerminalPanel?.hostedView | ||
|
|
||
| guard bonsplitController.openPaperCanvasPaneRight(paneId, withTab: newTab) != nil else { |
There was a problem hiding this comment.
P1: Set isProgrammaticSplit = true before calling openPaperCanvasPaneRight to prevent the delegate from auto-creating a duplicate terminal in the new pane.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/Workspace.swift, line 2351:
<comment>Set `isProgrammaticSplit = true` before calling `openPaperCanvasPaneRight` to prevent the delegate from auto-creating a duplicate terminal in the new pane.</comment>
<file context>
@@ -2134,6 +2309,70 @@ final class Workspace: Identifiable, ObservableObject {
+ let previousFocusedPanelId = focusedPanelId
+ let previousHostedView = focusedTerminalPanel?.hostedView
+
+ guard bonsplitController.openPaperCanvasPaneRight(paneId, withTab: newTab) != nil else {
+ panels.removeValue(forKey: newPanel.id)
+ panelTitles.removeValue(forKey: newPanel.id)
</file context>
| withTab tab: Tab? = nil, | ||
| notify: Bool = true | ||
| ) -> PaneID? { | ||
| guard configuration.layoutStyle == .paperCanvas else { return nil } |
There was a problem hiding this comment.
P2: Respect allowSplits here; this API currently bypasses split restrictions and can create panes in single-pane/read-only modes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At PaneKit/Sources/PaneKit/Public/BonsplitController.swift, line 722:
<comment>Respect `allowSplits` here; this API currently bypasses split restrictions and can create panes in single-pane/read-only modes.</comment>
<file context>
@@ -0,0 +1,1062 @@
+ withTab tab: Tab? = nil,
+ notify: Bool = true
+ ) -> PaneID? {
+ guard configuration.layoutStyle == .paperCanvas else { return nil }
+
+ let targetPaneId = paneId ?? focusedPaneId
</file context>
| guard configuration.layoutStyle == .paperCanvas else { return nil } | |
| guard configuration.layoutStyle == .paperCanvas, configuration.allowSplits else { return nil } |
| return false | ||
| } | ||
|
|
||
| internalController.closePane(PaneID(id: paneId.id)) |
There was a problem hiding this comment.
P2: closePane can report success for non-existent panes, causing incorrect delegate events and state assumptions.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At PaneKit/Sources/PaneKit/Public/BonsplitController.swift, line 565:
<comment>`closePane` can report success for non-existent panes, causing incorrect delegate events and state assumptions.</comment>
<file context>
@@ -0,0 +1,1062 @@
+ return false
+ }
+
+ internalController.closePane(PaneID(id: paneId.id))
+
+ // Notify delegate
</file context>
| internalController.closePane(PaneID(id: paneId.id)) | |
| guard internalController.paneState(paneId) != nil else { return false } | |
| internalController.closePane(PaneID(id: paneId.id)) |
|
|
||
| - [ ] **Step 4: Re-run the regression check** | ||
|
|
||
| Run: `xcodebuild -project GhosttyTabs.xcodeproj -scheme cmux-unit -destination 'platform=macOS' -derivedDataPath /tmp/cmux-pane-strip-unit build` |
There was a problem hiding this comment.
P2: The "re-run regression check" command only builds the project and does not run the added socket regression test, so the verification step is ineffective.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/superpowers/plans/2026-03-16-horizontal-workspace-pane-strip.md, line 613:
<comment>The "re-run regression check" command only builds the project and does not run the added socket regression test, so the verification step is ineffective.</comment>
<file context>
@@ -0,0 +1,683 @@
+
+- [ ] **Step 4: Re-run the regression check**
+
+Run: `xcodebuild -project GhosttyTabs.xcodeproj -scheme cmux-unit -destination 'platform=macOS' -derivedDataPath /tmp/cmux-pane-strip-unit build`
+
+Expected: BUILD SUCCEEDED, and the dedicated regression test passes once added.
</file context>
| Run: `xcodebuild -project GhosttyTabs.xcodeproj -scheme cmux-unit -destination 'platform=macOS' -derivedDataPath /tmp/cmux-pane-strip-unit build` | |
| Run: `python3 Tests/test_ctrl_socket.py` |
| ```swift | ||
| extension PaperCanvasState { | ||
| func openPaneRightPlacement(for targetFrame: CGRect, viewportSize: CGSize) -> CGRect { | ||
| let width = floor(viewportSize.width * 0.66) |
There was a problem hiding this comment.
P2: The planned open-pane width formula (0.66 * viewport) contradicts the test expectation (800 at 1200 viewport), so the documented implementation cannot satisfy the documented PASS criteria.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/superpowers/plans/2026-03-16-horizontal-workspace-pane-strip.md, line 191:
<comment>The planned open-pane width formula (`0.66 * viewport`) contradicts the test expectation (800 at 1200 viewport), so the documented implementation cannot satisfy the documented PASS criteria.</comment>
<file context>
@@ -0,0 +1,683 @@
+```swift
+extension PaperCanvasState {
+ func openPaneRightPlacement(for targetFrame: CGRect, viewportSize: CGSize) -> CGRect {
+ let width = floor(viewportSize.width * 0.66)
+ return CGRect(x: targetFrame.maxX + paneGap, y: targetFrame.minY, width: width, height: targetFrame.height).integral
+ }
</file context>
| navigationDirection = .down | ||
| } | ||
|
|
||
| guard ws.bonsplitController.resizePaperPane( |
There was a problem hiding this comment.
P2: pane.resize can change focused pane in paper-canvas mode, even though it is not a focus-intent command.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/TerminalController.swift, line 5010:
<comment>`pane.resize` can change focused pane in paper-canvas mode, even though it is not a focus-intent command.</comment>
<file context>
@@ -4967,6 +4994,61 @@ class TerminalController {
+ navigationDirection = .down
+ }
+
+ guard ws.bonsplitController.resizePaperPane(
+ PaneID(id: paneUUID),
+ direction: navigationDirection,
</file context>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
GhosttyTabs.xcodeproj/project.pbxproj (1)
503-509:⚠️ Potential issue | 🔴 CriticalLink
PaneKitin the target's Frameworks phase.
GhosttyTabsnow depends onPaneKit, but unlikeMarkdownUI,Sparkle,Sentry, andPostHog, there is no matchingPBXBuildFile/PaneKit in Frameworksentry in theA5001030 /* Frameworks */build phase. The package is referenced as a product dependency but not actually linked to the target, which will fail as soon as app sources importPaneKit.Re-add the package product to the target in Xcode, or mirror the existing
MarkdownUIpattern soPaneKitgets both the product dependency and the Frameworks-phase entry.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@GhosttyTabs.xcodeproj/project.pbxproj` around lines 503 - 509, The target lists PaneKit in packageProductDependencies (A5001261 /* PaneKit */) but you didn't add the matching PBXBuildFile entry in the Frameworks build phase (A5001030 /* Frameworks */), so PaneKit isn't actually linked; fix by adding a Frameworks-phase build file for the PaneKit product similar to the existing MarkdownUI entry (mirror the PBXBuildFile pattern used for MarkdownUI/Sparkle) so the target both has packageProductDependencies including A5001261 and a corresponding PBXBuildFile reference in A5001030 /* Frameworks */.Sources/GhosttyTerminalView.swift (1)
2852-3014:⚠️ Potential issue | 🟠 MajorDisable flags don’t fully disable templated env/working-directory values.
When
CMUX_SURFACE_TEST_DISABLE_ENV=1orCMUX_SURFACE_TEST_DISABLE_WD=1is set, inheritedconfigTemplatevalues can still leak through because only the override paths are skipped. This makes the bootstrap test flags unreliable.💡 Proposed fix
- if !env.isEmpty, !disableEnvVars { + if disableEnvVars { + surfaceConfig.env_vars = nil + surfaceConfig.env_var_count = 0 + } else if !env.isEmpty { envVars.reserveCapacity(env.count) envStorage.reserveCapacity(env.count) for (key, value) in env { guard let keyPtr = strdup(key), let valuePtr = strdup(value) else { continue } envStorage.append((keyPtr, valuePtr)) envVars.append(ghostty_env_var_s(key: keyPtr, value: valuePtr)) } } - if !disableWorkingDirectory, let workingDirectory, !workingDirectory.isEmpty { + if disableWorkingDirectory { + surfaceConfig.working_directory = nil + createSurface() + } else if let workingDirectory, !workingDirectory.isEmpty { workingDirectory.withCString { cWorkingDir in surfaceConfig.working_directory = cWorkingDir createSurface() } } else { createSurface() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/GhosttyTerminalView.swift` around lines 2852 - 3014, The test-disable flags can still leave template data in surfaceConfig; after constructing surfaceConfig (the one assigned to surfaceConfig variable), explicitly clear any templated env and working-directory when disableEnvVars or disableWorkingDirectory are set: if disableEnvVars { set surfaceConfig.env_var_count = 0 and surfaceConfig.env_vars = nil } and if disableWorkingDirectory { set surfaceConfig.working_directory = nil and ensure the later workingDirectory.withCString path is skipped by using a local wd = disableWorkingDirectory ? nil : workingDirectory (or set workingDirectory = nil) so the createSurface path cannot pick up a templated working_directory; reference symbols: surfaceConfig, disableEnvVars, disableWorkingDirectory, env_vars, env_var_count, workingDirectory, createSurface.cmuxTests/CmuxWebViewKeyEquivalentTests.swift (1)
5610-5653:⚠️ Potential issue | 🟡 MinorStrengthen this test so it truly verifies “rightmost pane” routing.
Right now,
newTerminalSplit(...)at Line 5614 typically focuses the new right pane, so this can still pass if the implementation incorrectly routes to the focused pane instead of the rightmost pane. Re-focus left before callingopenBrowser(...)to make this assertion robust.🔧 Suggested test hardening
func testOpenBrowserInWorkspaceSplitRightReusesRightmostPaneWhenAlreadySplit() { let manager = TabManager() guard let workspace = manager.selectedWorkspace, let leftPanelId = workspace.focusedPanelId, let rightPanel = workspace.newTerminalSplit(from: leftPanelId, orientation: .horizontal), let rightPaneId = workspace.paneId(forPanelId: rightPanel.id), let url = URL(string: "https://example.com/pull/456") else { XCTFail("Expected split setup to succeed") return } + // Ensure this validates rightmost-pane behavior (not merely focused-pane behavior). + workspace.focusPanel(leftPanelId) + XCTAssertEqual(workspace.focusedPanelId, leftPanelId) + let initialPaneCount = workspace.bonsplitController.allPaneIds.count guard let browserPanelId = manager.openBrowser( inWorkspace: workspace.id, url: url, preferSplitRight: true, insertAtEnd: true ) else { XCTFail("Expected browser panel to be created") return }As per coding guidelines, tests must verify observable runtime behavior through executable paths (not leave room for implementation-shape false positives).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/CmuxWebViewKeyEquivalentTests.swift` around lines 5610 - 5653, The test currently creates a right split with workspace.newTerminalSplit(from: leftPanelId) which often focuses the new right pane, so before calling manager.openBrowser(...) explicitly refocus the left pane to ensure routing is to the rightmost pane (not the focused pane); e.g., call the workspace API that sets focus back to leftPanelId (workspace.focusPanel(id: leftPanelId) or workspace.focusedPanelId = leftPanelId depending on available API) right after creating the split and before manager.openBrowser(...), then proceed with the existing assertions to verify the browser was placed in rightPaneId.
🟠 Major comments (24)
scripts/ghosttykit-checksums.txt-8-8 (1)
8-8: 🛠️ Refactor suggestion | 🟠 MajorUpdate
docs/ghostty-fork.mdto document this checksum mapping change.The checksum mapping in line 8 is correctly pinned to the current ghostty submodule SHA (
937af727ebf1f53899b0417b91842146333373a9), butdocs/ghostty-fork.mddoes not contain any reference to this fork change or checksum update. Per the fork maintenance guidelines, documentation must be kept in sync with submodule changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/ghosttykit-checksums.txt` at line 8, Update docs/ghostty-fork.md to record the checksum mapping change found in scripts/ghosttykit-checksums.txt: add an entry documenting that the ghostty submodule now pins to SHA 937af727ebf1f53899b0417b91842146333373a9 (previous mapping replaced by 999b980af7c8... entry in the checksums file), include a brief note describing why the fork/update was made and the date/PR that introduced it, and ensure the docs reference the checksums file name (scripts/ghosttykit-checksums.txt) and the exact SHA string so readers can cross-check.cmuxUITests/PaneStripUITests.swift-10-27 (1)
10-27:⚠️ Potential issue | 🟠 MajorAssert the scenario outcome, not just “no failures.”
All four tests currently pass through the same generic helper, and that helper never proves the named scenario actually happened. For example,
open_pane_rightstill passes if the harness no-ops but emitsstatus=okwith zero failure flags. Each case should also assert one scenario-specific observable result, such as pane-count delta, viewport offset change, or focus movement. As per coding guidelines,Tests must verify observable runtime behavior through executable paths.Also applies to: 53-66
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxUITests/PaneStripUITests.swift` around lines 10 - 27, The tests (testInitialTerminalIsVisible, testFocusRevealRightKeepsTerminalsVisibleAndAligned, testViewportPanRightKeepsTerminalsVisibleAndAligned, testOpenPaneRightKeepsTerminalsVisibleAndNonOverlapping) call runPaneStripScenario and then only call assertPassingPaneStripPayload which merely checks for no failures; update each test to assert a scenario-specific observable from the returned payload (e.g., for open_pane_right assert payload.paneCount increased by 1, for focus_reveal_right assert payload.focusedPaneId changed or payload.focusDelta == 1, for pan_viewport_right assert payload.viewportOffset.x increased, for initial_terminal_visible assert payload.visiblePanes contains expectedTerminalId). Alternatively extend assertPassingPaneStripPayload to take an expectedScenario enum/string and perform the corresponding additional assertions (checking payload.paneCount, payload.viewportOffset, payload.focusedPaneId or payload.visiblePanes) so each test verifies the named scenario rather than only status=ok.PaneKit/Sources/PaneKit/Internal/Models/SplitState.swift-17-35 (1)
17-35:⚠️ Potential issue | 🟠 MajorEnforce the
0...1divider invariant inSplitState.
dividerPositionis documented as normalized, but the model currently accepts anyCGFloat. A bad restore payload or overshooting drag calculation can set-0.1/1.1, and downstream split layout code will then compute invalid pane geometry from it. Clamp or reject out-of-range values here instead of relying on every caller to sanitize first.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@PaneKit/Sources/PaneKit/Internal/Models/SplitState.swift` around lines 17 - 35, SplitState currently accepts any CGFloat for dividerPosition which can produce invalid geometry; clamp the value to the valid 0.0...1.0 range inside the SplitState init (reference: the init that takes dividerPosition: CGFloat) so the stored property self.dividerPosition = max(0, min(1, dividerPosition)). Also ensure future assignments are safe by adding a property observer on var dividerPosition (didSet) that clamps out-of-range values, or alternatively expose a setter method that enforces the same clamp, so all construction and updates of dividerPosition are normalized.cmuxUITests/PaneStripUITests.swift-31-50 (1)
31-50:⚠️ Potential issue | 🟠 MajorAlways clean up the launched app on failure paths.
When
waitForJSONKeytimes out (line 45), the function callsXCTFailand returns without terminating the app instance launched on line 42. This leaves the process running, contaminating the next test's state. Add adeferblock to ensure cleanup on both success and failure paths:Suggested cleanup guard
private func runPaneStripScenario(_ scenario: String, frameCount: Int = 24) -> [String: String] { let app = XCUIApplication() let dataPath = "/tmp/cmux-ui-test-pane-strip-\(scenario)-\(UUID().uuidString).json" + defer { + if app.state != .notRunning { + app.terminate() + } + try? FileManager.default.removeItem(atPath: dataPath) + } try? FileManager.default.removeItem(atPath: dataPath)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxUITests/PaneStripUITests.swift` around lines 31 - 50, The test leaves the XCUIApplication running when waitForJSONKey times out; after calling app.launch() in runPaneStripScenario, add a defer block that always terminates the app (e.g., call app.terminate()) and optionally removes the temp dataPath file so cleanup runs on both success and failure; ensure the defer is declared immediately after app.launch() (referencing runPaneStripScenario, app.launch(), dataPath, waitForJSONKey, and XCTFail) so the app is shut down even when XCTFail and early return occur.PaneKit/Sources/PaneKit/Internal/Views/PaneContainerView.swift-338-355 (1)
338-355:⚠️ Potential issue | 🟠 MajorKeep a center insertion region on narrow panes.
Once a pane drops below 160pt in a dimension,
max(80, size * 0.25)lets the edge zones consume the whole surface. That makes strip-sized panes split-only, because a tab can no longer be dropped into the existing pane.📐 Suggested clamp
private func zoneForLocation(_ location: CGPoint) -> DropZone { let edgeRatio: CGFloat = 0.25 - let horizontalEdge = max(80, size.width * edgeRatio) - let verticalEdge = max(80, size.height * edgeRatio) + let minimumCenter: CGFloat = 48 + let horizontalEdge = min( + max(80, size.width * edgeRatio), + max(0, (size.width - minimumCenter) / 2) + ) + let verticalEdge = min( + max(80, size.height * edgeRatio), + max(0, (size.height - minimumCenter) / 2) + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@PaneKit/Sources/PaneKit/Internal/Views/PaneContainerView.swift` around lines 338 - 355, The edge zones in zoneForLocation currently use horizontalEdge/verticalEdge = max(80, size * 0.25) which can exceed half the pane and eliminate the center drop region on narrow panes; change zoneForLocation to clamp horizontalEdge and verticalEdge so they never consume the whole axis by computing a minCenter width (e.g. 40pt) and capping each edge at (size.width - minCenter)/2 and (size.height - minCenter)/2 respectively (ensure you take max(..., 0) to avoid negatives), then use those clamped horizontalEdge/verticalEdge values when returning the DropZone (.left/.right/.top/.bottom/.center).PaneKit/Sources/PaneKit/Internal/Views/PaneContainerView.swift-170-172 (1)
170-172:⚠️ Potential issue | 🟠 MajorDon't key drop hit-testing off the source controller only.
External drops are accepted later from pasteboard payload alone, but
isTabDragActiveonly flips for drags started by thisSplitViewController. On those cross-controller drags,NSViewRepresentableor portal-backed content stays hit-testable and can swallow the drag before the pane drop layer sees it.Also applies to: 247-251, 268-268, 503-512
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@PaneKit/Sources/PaneKit/Internal/Views/PaneContainerView.swift` around lines 170 - 172, isTabDragActive currently only checks controller.draggingTab and controller.activeDragTab, so external drags started by other controllers aren’t detected; change isTabDragActive (and the identical checks at the other noted locations) to also return true when there is an active system dragging session or the dragging pasteboard contains pane-drop types (e.g. inspect NSApp's current NSDraggingSession or NSPasteboard.general for our pane payload UTI) so cross-controller/portal drops are hit-test-blocked by the pane drop layer as well.Sources/TerminalController.swift-3791-3801 (1)
3791-3801:⚠️ Potential issue | 🟠 MajorClose the remaining paper-canvas split bypass.
These guards only cover
surface.splitandpane.create.v2SurfaceDragToSplit(params:)still acceptsup/downwithout the same paper-canvas check, so callers can hit a genericinternal_errorinstead of the newnot_supportedcontract, and may still build the unsupported topology if the lower layer allows it. Please route all split-creation entry points through the same validation.Also applies to: 4824-4834
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TerminalController.swift` around lines 3791 - 3801, The paper-canvas vertical-split guard is only applied in the surface.split and pane.create paths, but v2SurfaceDragToSplit(params:) (and the analogous entry at the other mentioned spot) still permits up/down splits; add the same validation there so any split-creation entrypoint rejects vertical splits when ws.bonsplitController.layoutStyle == .paperCanvas. Concretely, inside v2SurfaceDragToSplit(params:) (and the other split-creation handler referenced), check if ws.bonsplitController.layoutStyle == .paperCanvas && direction.orientation == .vertical and immediately return the same .err(code: "not_supported", message: "Vertical pane splits are not supported in horizontal pane-strip workspaces", data: ["workspace_id": ws.id.uuidString, "direction": direction.debugLabel]) to ensure consistent contract and prevent unsupported topology creation.PaneKit/Sources/PaneKit/Internal/Views/TabItemView.swift-325-380 (1)
325-380:⚠️ Potential issue | 🟠 MajorContext menu strings require localization.
All context menu button titles are bare string literals. As per coding guidelines, user-facing strings must use
String(localized: "key.name", defaultValue: "English text").🌐 Example fix for a few items (apply pattern to all)
`@ViewBuilder` private var contextMenuContent: some View { - contextButton("Rename Tab…", action: .rename) + contextButton(String(localized: "tabContextMenu.renameTab", defaultValue: "Rename Tab…"), action: .rename) if contextMenuState.hasCustomTitle { - contextButton("Remove Custom Tab Name", action: .clearName) + contextButton(String(localized: "tabContextMenu.removeCustomName", defaultValue: "Remove Custom Tab Name"), action: .clearName) } Divider() - contextButton("Close Tabs to Left", action: .closeToLeft) + contextButton(String(localized: "tabContextMenu.closeToLeft", defaultValue: "Close Tabs to Left"), action: .closeToLeft) .disabled(!contextMenuState.canCloseToLeft) - contextButton("Close Tabs to Right", action: .closeToRight) + contextButton(String(localized: "tabContextMenu.closeToRight", defaultValue: "Close Tabs to Right"), action: .closeToRight) .disabled(!contextMenuState.canCloseToRight) // ... apply same pattern to remaining menu items🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@PaneKit/Sources/PaneKit/Internal/Views/TabItemView.swift` around lines 325 - 380, The context menu in the computed property contextMenuContent uses raw string literals for all user-visible button titles; replace each literal with a localized string call (String(localized: "key.name", defaultValue: "English text")) and keep the same text as the defaultValue, e.g. for every contextButton invocation in contextMenuContent (and any labels for Dividers/conditional branches) swap the bare titles like "Rename Tab…" or "Close Tabs to Left" to localized keys, ensuring unique keys per label and preserving existing punctuation/ellipsis; update every occurrence referencing contextButton, contextMenuState, and the action cases (.rename, .closeToLeft, .togglePin, etc.) so all user-facing strings follow the String(localized:..., defaultValue:...) pattern.Sources/cmuxApp.swift-854-859 (1)
854-859:⚠️ Potential issue | 🟠 MajorUse
activeTabManagerin the menu fallback.If
performOpenPaneRightShortcut()declines the action, this path mutates the roottabManagerinstead of the active main window's manager. In multi-window flows that can open the pane in the wrong workspace.Suggested fix
private func performOpenPaneRightFromMenu() { if AppDelegate.shared?.performOpenPaneRightShortcut() == true { return } - _ = tabManager.openPaneRight() + _ = activeTabManager.openPaneRight() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/cmuxApp.swift` around lines 854 - 859, In performOpenPaneRightFromMenu, the fallback currently calls the global tabManager (tabManager.openPaneRight()) which can mutate the wrong window; replace that fallback with the active window's manager by invoking the activeTabManager's openPaneRight (e.g. use AppDelegate.shared?.activeTabManager?.openPaneRight() or the local activeTabManager property) so the pane opens in the active main window; keep the initial AppDelegate.shared?.performOpenPaneRightShortcut() check and only call the activeTabManager fallback when that returns false.Sources/cmuxApp.swift-137-155 (1)
137-155:⚠️ Potential issue | 🟠 MajorPreserve
MANPATH's default-search fallback here.
split(separator: ":")drops empty components, so whenMANPATHis unset or intentionally ends with:, this helper rewrites it to only the bundled Ghostty man dir. That turns an augment into an override, and shells launched from cmux can lose normal system pages.Suggested fix
private static func prependEnvPathIfMissing(_ key: String, path: String, defaultValue: String? = nil) { if path.isEmpty { return } var current = getenv(key).flatMap { String(cString: $0) } ?? "" - if current.isEmpty, let defaultValue { + let trimmedCurrent = current.trimmingCharacters(in: .whitespacesAndNewlines) + if trimmedCurrent.isEmpty, let defaultValue { current = defaultValue + } else if trimmedCurrent.isEmpty { + current = "" } let parts = current - .split(separator: ":") + .split(separator: ":", omittingEmptySubsequences: key != "MANPATH") .map(String.init) if parts.first == path { return } let filtered = parts.filter { $0 != path } - let updated = ([path] + filtered).joined(separator: ":") + var updated = ([path] + filtered).joined(separator: ":") + if key == "MANPATH", current.isEmpty || current.hasSuffix(":") { + updated += ":" + } setenv(key, updated, 1) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/cmuxApp.swift` around lines 137 - 155, The helper prependEnvPathIfMissing currently uses split(separator: ":") which drops empty components and thus removes trailing or empty entries in MANPATH (losing the default-search fallback); modify the splitting to preserve empty components (e.g., use split(separator: ":", omittingEmptySubsequences: false) and map to String) so that empty entries (including a trailing ":") are kept when computing parts and filtered/updated, and then continue to check parts.first == path and setenv as before in prependEnvPathIfMissing.PaneKit/Tests/PaneKitTests/BonsplitTests.swift-1128-1135 (1)
1128-1135:⚠️ Potential issue | 🟠 MajorAssert that the probe actually mounted before using
.zeroas the baseline.A slow CI host can still leave
probeViewat.zeroafter the fixed 50 ms spin, and the rest of the test then passes without ever exercisingPaneDropInteractionContainer. Add a precondition likeXCTAssertNotNil(probeView.superview)/XCTAssertFalse(initialFrame.isEmpty)before togglingmodel.zone.As per coding guidelines, "Tests must verify observable runtime behavior through executable paths."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@PaneKit/Tests/PaneKitTests/BonsplitTests.swift` around lines 1128 - 1135, Before using the probeView frame as a baseline, add an assertion that the probe actually mounted (e.g., assert probeView.superview is non-nil or that initialFrame is not empty) so the test fails on slow CI instead of silently using .zero; locate the block around probeView/frame checks (variables probeView, initialFrame, initialSizeChanges, initialOriginChanges) and insert a precondition like XCTAssertNotNil(probeView.superview) or XCTAssertFalse(initialFrame.isEmpty) immediately after computing initialFrame and before toggling model.zone so PaneDropInteractionContainer is exercised.PaneKit/Sources/PaneKit/Internal/Models/PaneState.swift-11-19 (1)
11-19:⚠️ Potential issue | 🟠 MajorClamp
selectedTabIdto the supplied tabs.If callers pass a stale UUID here, a non-empty pane starts with
selectedTab == nil. Falling back totabs.first?.idwhenever the provided ID isn’t present keeps the pane selection invariant intact.Suggested fix
- self.selectedTabId = selectedTabId ?? tabs.first?.id + if let selectedTabId, tabs.contains(where: { $0.id == selectedTabId }) { + self.selectedTabId = selectedTabId + } else { + self.selectedTabId = tabs.first?.id + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@PaneKit/Sources/PaneKit/Internal/Models/PaneState.swift` around lines 11 - 19, The init for PaneState should clamp the provided selectedTabId to the IDs present in the supplied tabs so a non-empty pane never ends up with selectedTabId == nil; in PaneState.init (the initializer taking id: PaneID, tabs: [TabItem], selectedTabId: UUID?), verify whether the provided selectedTabId exists in tabs' ids and if not set self.selectedTabId = tabs.first?.id (otherwise keep the provided value), then assign self.id and self.tabs as before.PaneKit/Sources/PaneKit/Public/BonsplitView.swift-102-109 (1)
102-109:⚠️ Potential issue | 🟠 MajorLocalize the default empty-pane copy.
Text("No Open Tabs")is shipped UI, so this should go through the repo’s localization path instead of hard-coded English.As per coding guidelines, "All user-facing strings must be localized using
String(localized: "key.name", defaultValue: "English text")... Never use bare string literals in SwiftUIText()."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@PaneKit/Sources/PaneKit/Public/BonsplitView.swift` around lines 102 - 109, Replace the hard-coded user-facing string in BonsplitView's body (the Text("No Open Tabs") instance) with a localized string using String(localized:defaultValue:), e.g. call Text(String(localized: "pane.empty.noOpenTabs", defaultValue: "No Open Tabs")); update the localization key "pane.empty.noOpenTabs" in your .strings or localization catalog as appropriate so the defaultValue remains the English fallback.PaneKit/Sources/PaneKit/Internal/Views/SplitContainerView.swift-840-850 (1)
840-850:⚠️ Potential issue | 🟠 MajorDefer divider writes using captured state, not mutable
self.splitState.The drag callback computes
normalizedPositionfrom the current split view, then defers the write viaTask {@mainactor... }. In that interval,Coordinator.update()can retargetself.splitStateto a differentSplitStateobject (e.g., during split tree restructuring). Writing to the newly-assignedself.splitStatethen applies a stale drag event to the wrong split.Capture both
splitStateandsplitStateIdbefore the task, guard inside the task to confirm the coordinator is still bound to the same node, and write to the captured state object instead ofself.splitState.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@PaneKit/Sources/PaneKit/Internal/Views/SplitContainerView.swift` around lines 840 - 850, The deferred Task in SplitContainerView's drag handler writes to mutable self.splitState and can apply stale drags if Coordinator.update() swaps in a new SplitState; capture the current splitState and its id (e.g., let capturedState = self.splitState and let capturedId = self.splitState.id) before creating Task { `@MainActor` ... }, then inside the Task verify the current self.splitState.id equals capturedId (or guard that self.splitState === capturedState) and only then assign capturedState.dividerPosition and capturedState.lastAppliedPosition and call self.onGeometryChange?(wasDragging); this ensures the write targets the original SplitState instance and avoids applying stale drag updates to a re-targeted split.PaneKit/Sources/PaneKit/Internal/Views/SplitContainerView.swift-294-303 (1)
294-303:⚠️ Potential issue | 🟠 MajorReused split views skip the entry-placement path.
When SwiftUI reuses the same
NSSplitViewinstance for a newSplitState(indicated by a differentsplitState.id),updateNSView()callsCoordinator.update()to resetdidApplyInitialDividerPosition, but the entry-placement logic—which consumesanimationOrigin, hides the new pane, and schedulesapplyInitialDividerPosition()—only exists inmakeNSView(). A new split state withanimationOriginset will never trigger this logic; the pane appears instantly instead of animating in from the origin edge.The entry-placement logic (lines 166–289) must be extracted and called from
updateNSView()wheneversplitState.idchanges andanimationOriginis present.Also applies to lines 500–513 (where the flag is reset without restoring the animation path).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@PaneKit/Sources/PaneKit/Internal/Views/SplitContainerView.swift` around lines 294 - 303, When SwiftUI reuses an NSSplitView for a new SplitState the entry-placement logic that consumes animationOrigin and schedules applyInitialDividerPosition is only in makeNSView, so updateNSView must run it when the splitState.id changes; extract the entry-placement steps (consume splitState.animationOrigin, hide the incoming pane, set up the deferred applyInitialDividerPosition call and set didApplyInitialDividerPosition) into a small helper used by both makeNSView and updateNSView (callable from Coordinator.update path), and update the place where didApplyInitialDividerPosition is being reset (the code that currently clears the flag without restoring the animation path) to invoke the same helper or restore the animation path so the initial divider animation still runs; reference functions/properties: updateNSView, makeNSView, Coordinator.update, didApplyInitialDividerPosition, applyInitialDividerPosition, animationOrigin, and splitState.id.PaneKit/Sources/PaneKit/Internal/Views/SplitViewContainer.swift-22-24 (1)
22-24:⚠️ Potential issue | 🟠 MajorObserve geometry frame changes, not just size changes.
updateContainerFramestoresgeometry.frame(in: .global), but this view only reruns it whengeometry.sizechanges. Window moves and ancestor relayouts can shift the global origin without resizing, leavingcontroller.containerFrame.originstale for downstream pane positioning logic.Suggested fix
- .onChange(of: geometry.size) { _, newSize in + .onChange(of: geometry.frame(in: .global)) { _, _ in updateContainerFrame(geometry: geometry) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@PaneKit/Sources/PaneKit/Internal/Views/SplitViewContainer.swift` around lines 22 - 24, The view currently only calls updateContainerFrame when geometry.size changes, which misses moves/relayouts that change geometry.frame(in: .global) but not size; replace the .onChange(of: geometry.size) observer with an observer on the full geometry.frame(in: .global) (or its origin) so updateContainerFrame(geometry:) runs whenever the global frame changes, ensuring controller.containerFrame is kept up to date for pane positioning.PaneKit/Sources/PaneKit/Internal/Models/TabItem.swift-6-14 (1)
6-14:⚠️ Potential issue | 🟠 MajorAdd missing UTType declarations to Info.plist under
UTExportedTypeDeclarations.The code defines two custom UTTypes (
com.splittabbar.tabitemandcom.splittabbar.tabtransfer) butResources/Info.plisthas incomplete declarations. Currently,com.splittabbar.tabtransferis incorrectly listed underUTImportedTypeDeclarations(for types defined elsewhere), whilecom.splittabbar.tabitemis missing entirely. Both must be declared underUTExportedTypeDeclarationsfor proper system registration of drag-and-drop functionality.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@PaneKit/Sources/PaneKit/Internal/Models/TabItem.swift` around lines 6 - 14, Info.plist currently mis-declares the custom UTTypes used by UTType.tabItem and UTType.tabTransfer: move com.splittabbar.tabtransfer out of UTImportedTypeDeclarations and add both com.splittabbar.tabitem and com.splittabbar.tabtransfer under UTExportedTypeDeclarations; for each exported entry include the UTTypeIdentifier key with the identifier, an appropriate UTTypeConformsTo array (e.g., com.splittabbar.tabtransfer should conform to public.data), and any human-readable description keys so the system registers these types for drag-and-drop to match the UTType extensions in TabItem.swift.Sources/Workspace.swift-254-260 (1)
254-260:⚠️ Potential issue | 🟠 MajorDon't discard split-tree mode during restore.
For
.pane/.split, this returnsSelf.paneLayoutStyle, and that static is hard-coded to.paperCanvas. The restore flow therefore rebuilds split snapshots and then immediately flips the workspace into paper canvas, so saved split layouts do not reopen in the mode they were persisted with.Suggested fix
private func postRestoreLayoutStyle(for layout: SessionWorkspaceLayoutSnapshot) -> PaneLayoutStyle { switch layout { case .canvas: return .paperCanvas case .pane, .split: - return Self.paneLayoutStyle + return .splitTree } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 254 - 260, The postRestoreLayoutStyle(for:) helper currently maps .pane and .split to Self.paneLayoutStyle (which is hard-coded to .paperCanvas) causing restored split layouts to be flipped to paper canvas; change postRestoreLayoutStyle(for:) so that when layout is .split or .pane it returns the split-tree layout style (preserve the split mode) instead of Self.paneLayoutStyle, and keep Self.paneLayoutStyle for any other callers that truly need the legacy default.Sources/Workspace.swift-487-525 (1)
487-525:⚠️ Potential issue | 🟠 MajorAvoid silently dropping the rest of the canvas snapshot after one seed failure.
If any
newTerminalSplit(...)/paneId(forPanelId:)lookup fails, the loop hitsbreak, and the trailingzip(paneIds, layout.panes)truncates every remaining persisted pane. One transient seeding failure should not erase the rest of the saved workspace; bubble the failure up or fall back to another placeholder path instead of breaking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 487 - 525, The loop currently does `break` on failures from newTerminalSplit(...) or paneId(forPanelId:) which silently truncates remaining layout.panes; instead either propagate the failure by making this function throwable and rethrow the error when those calls return nil, or retain the full layout by creating a fallback placeholder for that pane (e.g., call newTerminalSurface(inPane:anchorPaneId, focus:false) or generate a dedicated empty pane id) and append that placeholder to paneIds and continue the loop; update the code paths that consume paneIds/layout.panes pairing so they assume paneIds.count == layout.panes.count and keep references to the symbols newTerminalSplit, paneId(forPanelId:), newTerminalSurface and SessionPaneRestoreEntry to locate and modify the logic.PaneKit/Sources/PaneKit/Internal/Controllers/SplitViewController.swift-471-475 (1)
471-475:⚠️ Potential issue | 🟠 MajorPreserve focus when closing an unfocused split-tree pane.
This path always overwrites
focusedPaneIdwith a sibling/first pane from the removed branch. Closing a background pane will therefore steal focus from the pane the user was actively working in, even when that pane still exists.Also applies to: 526-539
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@PaneKit/Sources/PaneKit/Internal/Controllers/SplitViewController.swift` around lines 471 - 475, The current removal logic always reassigns focusedPaneId to a sibling or first pane which causes focus theft when removing an unfocused pane; change the behavior so you only update focusedPaneId when the pane being removed equals the current focusedPaneId. In other words, in the removal code paths that set focusedPaneId (the block referencing siblingPaneId and rootNode.allPaneIds and the similar block around the 526-539 region), guard the reassignment with a check like `if removedPaneId == focusedPaneId` (or equivalent) before assigning siblingPaneId or firstPane to focusedPaneId so focus is preserved for other active panes. Ensure you update both instances mentioned (the block using siblingPaneId/rootNode.allPaneIds and the separate block at 526-539) to use the same guard and reference the same focusedPaneId and removed pane identifier.PaneKit/Sources/PaneKit/Public/BonsplitController.swift-717-756 (1)
717-756:⚠️ Potential issue | 🟠 MajorApply the same split-policy checks here.
Unlike the other pane-creation APIs, this path skips both
configuration.allowSplitsandshouldSplitPane. That makesopenPaperCanvasPaneRight(...)a bypass for the existing split gating.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@PaneKit/Sources/PaneKit/Public/BonsplitController.swift` around lines 717 - 756, The openPaperCanvasPaneRight method currently bypasses split gating; add the same checks as other pane-creation APIs by verifying configuration.allowSplits and calling shouldSplitPane for the target pane before proceeding. Specifically, after resolving targetPaneId (and before creating internalTab or calling internalController.openPaperCanvasPaneRight), add a guard that returns nil if configuration.allowSplits is false or if shouldSplitPane(PaneID(id: targetPaneId.id)) is false; keep the existing notifyGeometryChange and return behavior unchanged. Ensure the guards reference configuration.allowSplits, shouldSplitPane(_:), focusedPaneId/targetPaneId and internalController.openPaperCanvasPaneRight so the change mirrors the other split-policy enforcement.PaneKit/Sources/PaneKit/Public/BonsplitController.swift-554-572 (1)
554-572:⚠️ Potential issue | 🟠 MajorDon't report a close unless a pane was actually removed.
internalController.closePane(...)can no-op for a stale pane ID, and Line 461 inSplitViewController.swiftstill short-circuits the last-pane case. This wrapper still firesdidClosePaneand returnstrue, so callers get a false success signal andallowCloseLastPanenever really works.PaneKit/Sources/PaneKit/Internal/Controllers/SplitViewController.swift-99-112 (1)
99-112:⚠️ Potential issue | 🟠 MajorSwitching
layoutStylecurrently swaps in stale pane state.
paperCanvasmutations never updaterootNode, andapplyConfiguration(_:)just flips which store is active. After creating/closing panes in paper-canvas mode, switching back to.splitTreewill resurrect stale panes, drop paper-only panes, and can leavefocusedPaneIdpointing at a pane that no longer exists.PaneKit/Sources/PaneKit/Public/BonsplitController.swift-278-286 (1)
278-286:⚠️ Potential issue | 🟠 MajorPropagate pane-focus side effects from
selectTab.Selecting a tab in another pane calls
internalController.focusPane(...), which can both changefocusedPaneIdand scroll the paper viewport. This path only emitsdidSelectTab, so focus observers and geometry consumers stay stale.Possible fix
public func selectTab(_ tabId: TabID) { guard let (pane, tabIndex) = findTabInternal(tabId) else { return } + let previousPaneId = internalController.focusedPaneId + let previousViewportOrigin = internalController.paperViewportOrigin pane.selectTab(tabId.id) internalController.focusPane(pane.id) // Notify delegate let tab = Tab(from: pane.tabs[tabIndex]) + if previousPaneId != internalController.focusedPaneId { + delegate?.splitTabBar(self, didFocusPane: pane.id) + } delegate?.splitTabBar(self, didSelectTab: tab, inPane: pane.id) + if configuration.layoutStyle == .paperCanvas, + internalController.paperViewportOrigin != previousViewportOrigin { + notifyGeometryChange() + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@PaneKit/Sources/PaneKit/Public/BonsplitController.swift` around lines 278 - 286, selectTab calls internalController.focusPane(...) which mutates focus and viewport but does not propagate those side effects to observers; update the controller's focusedPaneId to pane.id and invoke the same focus-change notification path used elsewhere so observers and geometry consumers see the change (for example, call the centralized focus handler or emit the delegate focus callback — e.g. delegate?.splitTabBar(self, didFocusPane: pane.id) or notifyFocusObservers(pane.id)) before/alongside sending delegate?.splitTabBar(... didSelectTab: ...).
🧹 Nitpick comments (10)
Sources/TerminalNotificationStore.swift (1)
4-4: Remove the unused PaneKit import.The
PaneKitmodule is not used anywhere in this file. No types, functions, or extensions from PaneKit are referenced, and the "Pane" references in enum names (NotificationPaneRingSettings,NotificationPaneFlashSettings) are local definitions unrelated to the import.Suggested change
import AppKit import Foundation import UserNotifications -import PaneKit🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TerminalNotificationStore.swift` at line 4, Remove the unused PaneKit import at the top of the file; confirm there are no references to PaneKit types or APIs in this file (the enums NotificationPaneRingSettings and NotificationPaneFlashSettings are local and not dependent on PaneKit) and then delete the line "import PaneKit" so the file only imports modules that are actually used.PaneKit/Sources/PaneKit/Internal/Utilities/SplitAnimator.swift (1)
50-57: Simplify the MainActor dispatch.The nested
DispatchQueue.main.async { Task {@mainactorin ... } }is redundant. ATask {@mainactorin }already schedules execution on the main actor from any thread.♻️ Suggested simplification
let callback: CVDisplayLinkOutputCallback = { _, _, _, _, _, context in - let animator = Unmanaged<SplitAnimator>.fromOpaque(context!).takeUnretainedValue() - DispatchQueue.main.async { - Task { `@MainActor` in - animator.tick() - } + guard let context else { return kCVReturnSuccess } + let animator = Unmanaged<SplitAnimator>.fromOpaque(context).takeUnretainedValue() + Task { `@MainActor` in + animator.tick() } return kCVReturnSuccess }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@PaneKit/Sources/PaneKit/Internal/Utilities/SplitAnimator.swift` around lines 50 - 57, The CVDisplayLink callback in SplitAnimator currently wraps the Task call in DispatchQueue.main.async; remove the redundant DispatchQueue.main.async and directly invoke Task { `@MainActor` in animator.tick() } inside the callback closure (keeping the existing Unmanaged<SplitAnimator>.fromOpaque(context!).takeUnretainedValue() extraction and return kCVReturnSuccess) so the tick() runs on the main actor without the extra dispatch.cmuxTests/AppDelegateShortcutRoutingTests.swift (1)
2444-2460: Optional: dedupe repeated workspace-context lookup logic.The same lookup condition is implemented twice; extracting it into a local closure keeps the helper easier to maintain.
♻️ Suggested simplification
private func waitForFocusedWorkspaceContext( windowId: UUID, timeout: TimeInterval = 1.0 ) -> (window: NSWindow, workspace: Workspace)? { + func resolveContext() -> (window: NSWindow, workspace: Workspace)? { + guard let window = window(withId: windowId), + let manager = AppDelegate.shared?.tabManagerFor(windowId: windowId), + let workspace = manager.selectedWorkspace, + workspace.focusedPanelId != nil else { + return nil + } + return (window, workspace) + } + let deadline = Date().addingTimeInterval(timeout) while Date() < deadline { - if let window = window(withId: windowId), - let manager = AppDelegate.shared?.tabManagerFor(windowId: windowId), - let workspace = manager.selectedWorkspace, - workspace.focusedPanelId != nil { - return (window, workspace) - } + if let context = resolveContext() { return context } RunLoop.main.run(until: Date(timeIntervalSinceNow: 0.01)) } - if let window = window(withId: windowId), - let manager = AppDelegate.shared?.tabManagerFor(windowId: windowId), - let workspace = manager.selectedWorkspace, - workspace.focusedPanelId != nil { - return (window, workspace) - } - - return nil + return resolveContext() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/AppDelegateShortcutRoutingTests.swift` around lines 2444 - 2460, The repeated workspace-context lookup (the while loop body and the final if) should be extracted into a small local closure to avoid duplication: create a closure (e.g., let currentWorkspaceContext = { ... } ) that returns the tuple (window, workspace)? or optional (Window, Workspace) by performing the checks using window(withId:), AppDelegate.shared?.tabManagerFor(windowId:), manager.selectedWorkspace and workspace.focusedPanelId, call that closure inside the loop and after the loop, and return its result when non-nil.PaneKit/Sources/PaneKit/Internal/Views/PaperCanvasViewContainer.swift (1)
13-18: Clarify the intent of the double notification.
scheduleGeometryChangeNotificationinvokes the callback twice: once synchronously and once viaDispatchQueue.main.async. If this is intentional (e.g., to ensure both immediate and post-layout-pass updates), a brief comment would clarify the intent for future maintainers.💡 Suggested clarification
private func scheduleGeometryChangeNotification() { + // Immediate notification for synchronous consumers onGeometryChange?(false) + // Deferred notification after layout pass completes DispatchQueue.main.async { onGeometryChange?(false) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@PaneKit/Sources/PaneKit/Internal/Views/PaperCanvasViewContainer.swift` around lines 13 - 18, The function scheduleGeometryChangeNotification currently calls onGeometryChange?(false) synchronously and again inside DispatchQueue.main.async; add a brief inline comment above or inside scheduleGeometryChangeNotification explaining the intent (e.g., first call for immediate observers and the async call to notify after the next runloop/layout pass), referencing the onGeometryChange callback so future maintainers understand why both immediate and deferred notifications are required; if the double notification was unintentional, remove the redundant synchronous or async call instead and update the comment accordingly.docs/superpowers/specs/2026-03-16-horizontal-workspace-pane-strip-design.md (1)
5-7: Minor: Consider hyphenating compound adjectives for consistency.Per the static analysis hint, "top level" in line 5 and 7 could be hyphenated as "top-level" when used as a compound adjective modifying a noun (e.g., "top-level unit"). This is a minor editorial nit.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/superpowers/specs/2026-03-16-horizontal-workspace-pane-strip-design.md` around lines 5 - 7, Change the instances of the phrase "top level" used as a compound adjective to "top-level" in the spec text (e.g., in the sentence "The top-level unit is a `pane`, not a nested split tree" and the later "This keeps the top level closer to niri...") so both occurrences are hyphenated for consistency and to match the static analysis suggestion.Sources/TabManager.swift (2)
3734-3756: Consider defensive handling for test target creation instead of force unwraps.The force unwraps when building test targets (e.g.,
tab.terminalPanel(for: topLeftId)!) will crash the test if the terminal panel lookup fails, making debugging harder. While this is test-only code, using guard statements with early failure messages would provide clearer diagnostics.♻️ Example: Safer target construction
case "close_bottom": - return [ - ("TL", tab.terminalPanel(for: topLeftId)!.surface.hostedView), - ("TR", topRight.surface.hostedView), - ] + guard let tlPanel = tab.terminalPanel(for: topLeftId) else { + fail("Missing terminal panel for topLeftId in close_bottom scenario") + return [] + } + return [ + ("TL", tlPanel.surface.hostedView), + ("TR", topRight.surface.hostedView), + ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TabManager.swift` around lines 3734 - 3756, Replace the force unwraps used when constructing test targets so failures produce clear diagnostics: for each use of tab.terminalPanel(for: topLeftId)!, topRight.surface.hostedView, and bottomLeft.surface.hostedView in this switch block, perform a guard let (or if let) to safely unwrap the terminalPanel and hostedView and call a single, descriptive early-fail path (e.g., fatalError or a test failure helper) that includes the missing identifier like topLeftId/topRight/bottomLeft; update the switch arms ("close_right_single", "close_bottom", "close_right_lrtd_bottom_first", default) to use the safely-unwrapped values instead of force-unwraps so a clear message is emitted when a panel lookup fails.
766-1005: Large but cohesive frame capture function for motion testing.At 239 lines, this function is lengthy but maintains cohesion - it performs comprehensive per-frame analysis (scheduled actions, reference capture, alignment/visibility/occlusion/overlap detection) in a logical sequence. Given this is DEBUG-only test infrastructure that won't affect production, the length is acceptable. The sequential analysis nature makes decomposition less beneficial than it might seem.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TabManager.swift` around lines 766 - 1005, The reviewer notes that capturePaneStripMotionFrame is large (239 lines) but cohesive and acceptable for DEBUG-only test infrastructure, so no refactor is required; leave the function capturePaneStripMotionFrame as-is and do not decompose it into smaller functions.Sources/Panels/CmuxWebView.swift (1)
209-209: Consider updating stale "bonsplit" references in comments.A few comments still reference "bonsplit" (lines 209, 1151-1158) after the migration to PaneKit. While these are documentation-only and don't affect functionality, updating them would improve consistency.
Also applies to: 1151-1163
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Panels/CmuxWebView.swift` at line 209, Locate any comment text in CmuxWebView.swift that uses the literal token "bonsplit" (for example the comment beginning "bonsplit focus tracks which pane the user clicked in." and the longer comment block that still mentions bonsplit) and update those comments to use the current terminology ("PaneKit" or "pane focus" as appropriate) keeping punctuation/capitalization consistent with surrounding comments; do not change code logic or identifiers—only replace the stale "bonsplit" wording in the comment strings to match the PaneKit migration.PaneKit/Sources/PaneKit/Public/Types/PaneLayoutStyle.swift (1)
3-7: Doc wording should match the API visibility.The comment says “Internal” while
PaneLayoutStyleis public. Consider clarifying to avoid confusion for consumers.✏️ Suggested doc tweak
-/// Internal layout engine selection for pane geometry. +/// Layout engine selection for pane geometry.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@PaneKit/Sources/PaneKit/Public/Types/PaneLayoutStyle.swift` around lines 3 - 7, The documentation header incorrectly calls the API "Internal" even though PaneLayoutStyle is public; update the doc comment for the PaneLayoutStyle enum to reflect its public visibility and purpose (e.g., "Layout engine selection for pane geometry."), and ensure descriptions for cases `splitTree` and `paperCanvas` clearly explain intended behavior for public consumers rather than implying internal use.PaneKit/Sources/PaneKit/Public/Types/PaperCanvasLayoutSnapshot.swift (1)
26-29: ConstrainfocusedPaneIdto panes present in the snapshotLine 28 currently accepts any
focusedPaneId, including stale IDs not inpanes. Normalizing this here keeps snapshot invariants tighter and avoids downstream focus-resolution edge cases.Proposed change
- self.focusedPaneId = focusedPaneId + self.focusedPaneId = focusedPaneId.flatMap { candidate in + panes.contains(where: { $0.paneId == candidate }) ? candidate : nil + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@PaneKit/Sources/PaneKit/Public/Types/PaperCanvasLayoutSnapshot.swift` around lines 26 - 29, The initializer currently assigns focusedPaneId without verifying it exists in panes; update the PaperCanvasLayoutSnapshot init to normalize focusedPaneId by checking membership against panes (e.g., use panes.contains(where: { $0.id == focusedPaneId })) and if the provided focusedPaneId is not found set self.focusedPaneId to nil (or another clear default), ensuring self.panes remains unchanged but self.focusedPaneId is always either nil or an id present in the snapshot.
| // Perform split | ||
| internalController.splitPane( | ||
| PaneID(id: targetPaneId.id), | ||
| orientation: orientation, | ||
| with: internalTab | ||
| ) | ||
|
|
||
| // Find new pane (will be focused after split) | ||
| let newPaneId = focusedPaneId! | ||
|
|
||
| // Notify delegate | ||
| delegate?.splitTabBar(self, didSplitPane: targetPaneId, newPane: newPaneId, orientation: orientation) | ||
|
|
||
| notifyGeometryChange() | ||
|
|
||
| return newPaneId |
There was a problem hiding this comment.
Don't treat focusedPaneId as proof that the split succeeded.
These wrappers assume success after calling internal Void split methods and then reuse focusedPaneId as the "new" pane. Any internal no-op path—stale target pane, or the paper-canvas insertFirst == true branch that splitPaperPane(...) currently rejects—still falls through as success. The movingTab overload is worse: it removes the source tab first, so a no-op can lose the tab entirely.
Also applies to: 463-478, 517-547
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@PaneKit/Sources/PaneKit/Public/BonsplitController.swift` around lines 401 -
416, The current split wrappers call internalController.splitPane (and related
splitPaperPane / movingTab overloads) and immediately assume success by reading
focusedPaneId as the new pane, which can mask no-ops and can lose a tab for the
movingTab path; update these wrappers to detect real success before treating
focusedPaneId as the new pane: have internal split operations return a clear
result (Bool or an optional PaneID) or expose a completion/throwing API, check
that result (or compare pre- and post-operation state) before using
focusedPaneId, only notify the delegate via delegate?.splitTabBar(self,
didSplitPane:..., newPane:..., orientation:...) and call notifyGeometryChange()
when the split actually occurred, and for the movingTab overload ensure the
source tab is not removed until success is confirmed to avoid data loss. Ensure
code references internalController.splitPane, splitPaperPane, focusedPaneId, and
the movingTab path when implementing the checks.
| guard ws.bonsplitController.panPaperCanvasViewport( | ||
| by: CGSize(width: dx, height: dy), | ||
| notify: true | ||
| ), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and get line count
wc -l Sources/TerminalController.swift 2>/dev/null || echo "File not found"Repository: manaflow-ai/cmux
Length of output: 98
🏁 Script executed:
# Read the specific lines mentioned in the review (5140-5143) with context
sed -n '5130,5150p' Sources/TerminalController.swiftRepository: manaflow-ai/cmux
Length of output: 906
🏁 Script executed:
# Look for the function containing these lines to find dx and dy declarations
sed -n '5100,5145p' Sources/TerminalController.swiftRepository: manaflow-ai/cmux
Length of output: 2119
🏁 Script executed:
# Also search backwards to find the function signature and variable declarations
sed -n '5050,5150p' Sources/TerminalController.swiftRepository: manaflow-ai/cmux
Length of output: 4588
🏁 Script executed:
# Find the definition of panPaperCanvasViewport to see its parameter type
rg "func panPaperCanvasViewport" Sources/Repository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
# Also search for v2Int function to understand what type it returns
rg "func v2Int" Sources/Repository: manaflow-ai/cmux
Length of output: 165
🏁 Script executed:
# Search for CGSize usage in the codebase to see how it's typically used with dx/dy
rg "CGSize\(width: .*, height:" Sources/ -A 1 -B 1 | head -50Repository: manaflow-ai/cmux
Length of output: 3563
🏁 Script executed:
# Search for BonsplitController and the panPaperCanvasViewport method
rg "class BonsplitController" Sources/Repository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
# Search for the panPaperCanvasViewport method definition
rg "func panPaperCanvasViewport" -A 3 Sources/Repository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
# Search more broadly for any file containing panPaperCanvasViewport
find . -name "*.swift" -type f | xargs grep -l "panPaperCanvasViewport" 2>/dev/nullRepository: manaflow-ai/cmux
Length of output: 345
🏁 Script executed:
# Check CGSize API - it should accept numeric types in Swift
# Let's also check if dx and dy could be implicitly converted
rg "CGSize\(width: dx" Sources/ -B 5 -A 5Repository: manaflow-ai/cmux
Length of output: 847
🏁 Script executed:
# Look at the actual usage in TabManager.swift to see how CGSize is used with literal numbers
rg "panPaperCanvasViewport" Sources/ -B 2 -A 2Repository: manaflow-ai/cmux
Length of output: 754
🏁 Script executed:
# Look at the BonsplitController definition in PaneKit
cat PaneKit/Sources/PaneKit/Public/BonsplitController.swift | head -100Repository: manaflow-ai/cmux
Length of output: 4017
🏁 Script executed:
# Search for the panPaperCanvasViewport method specifically in BonsplitController
rg "panPaperCanvasViewport" PaneKit/Sources/PaneKit/Public/BonsplitController.swift -A 5Repository: manaflow-ai/cmux
Length of output: 347
🏁 Script executed:
# Let's verify the exact types - check if CGSize initializer accepts Int directly
# by looking at how Swift handles numeric type conversions
python3 << 'EOF'
# In Swift, CGSize(width: CGFloat, height: CGFloat) requires explicit CGFloat types
# Int literals can be implicitly converted (520 becomes CGFloat(520))
# But Int variables cannot be implicitly converted to CGFloat
# This is a compilation error
print("Analysis of Swift type compatibility:")
print("- CGSize initializer signature: init(width: CGFloat, height: CGFloat)")
print("- dx type: Int")
print("- dy type: Int")
print("- Int variables cannot be implicitly converted to CGFloat in Swift")
print("- Therefore: CGSize(width: dx, height: dy) will NOT compile")
print("- Fix required: CGSize(width: CGFloat(dx), height: CGFloat(dy))")
EOFRepository: manaflow-ai/cmux
Length of output: 388
🏁 Script executed:
# Verify by checking if there's any import or extension that might change this behavior
rg "extension CGSize" Sources/Repository: manaflow-ai/cmux
Length of output: 42
Convert the viewport delta to CGFloat before building CGSize.
dx and dy are Int values, but CGSize(width:height:) requires CGFloat parameters, so this code will not compile.
Suggested fix
guard ws.bonsplitController.panPaperCanvasViewport(
- by: CGSize(width: dx, height: dy),
+ by: CGSize(width: CGFloat(dx), height: CGFloat(dy)),
notify: true
),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| guard ws.bonsplitController.panPaperCanvasViewport( | |
| by: CGSize(width: dx, height: dy), | |
| notify: true | |
| ), | |
| guard ws.bonsplitController.panPaperCanvasViewport( | |
| by: CGSize(width: CGFloat(dx), height: CGFloat(dy)), | |
| notify: true | |
| ), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/TerminalController.swift` around lines 5140 - 5143, The call to
ws.bonsplitController.panPaperCanvasViewport uses CGSize(width: dx, height: dy)
but dx and dy are Ints; convert them to CGFloat when constructing the CGSize
(e.g. wrap dx and dy with CGFloat(...) ) so CGSize(width: CGFloat(dx), height:
CGFloat(dy)) is passed to panPaperCanvasViewport; update the CGSize construction
at the call site in TerminalController.swift where
ws.bonsplitController.panPaperCanvasViewport(...) is invoked.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@GhosttyTabs.xcodeproj/project.pbxproj`:
- Line 510: PaneKit is listed in packageProductDependencies but not actually
wired into the GhosttyTabs target (missing Frameworks build phase entries and
PBXBuildFile records); open the GhosttyTabs target in Xcode and re-add the
PaneKit package product to the target’s Frameworks (or Link Binary With
Libraries) phase so Xcode regenerates the missing PBXBuildFile entries and
framework linkage; verify PaneKit appears alongside
Sparkle/Sentry/PostHog/MarkdownUI in the Frameworks build phase and that
corresponding PBXBuildFile and fileReference entries are present in the
project.pbxproj.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d7178913-9a61-4fa5-a177-9ca20f59bbf4
📒 Files selected for processing (1)
GhosttyTabs.xcodeproj/project.pbxproj
| A5001251 /* Sentry */, | ||
| A5001271 /* PostHog */, | ||
| A5001261 /* Bonsplit */, | ||
| A5001261 /* PaneKit */, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
from pathlib import Path
import re
proj = Path("GhosttyTabs.xcodeproj/project.pbxproj")
text = proj.read_text()
target = re.search(
r'A5001050 /\* GhosttyTabs \*/ = \{.*?packageProductDependencies = \((.*?)\);\s*name = GhosttyTabs;',
text,
re.S,
)
frameworks = re.search(
r'A5001030 /\* Frameworks \*/ = \{.*?files = \((.*?)\);\s*runOnlyForDeploymentPostprocessing = 0;\s*\};',
text,
re.S,
)
deps = re.findall(r'/\* ([^*]+) \*/', target.group(1))
linked = re.findall(r'/\* ([^*]+) in Frameworks \*/', frameworks.group(1))
missing = [dep for dep in deps if dep not in linked]
pane_buildfiles = len(re.findall(r'productRef = .*?/\* PaneKit \*/', text))
print("GhosttyTabs packageProductDependencies:", deps)
print("GhosttyTabs Frameworks build phase:", linked)
print("Missing framework links:", missing)
print("PaneKit PBXBuildFile entries:", pane_buildfiles)
PYRepository: manaflow-ai/cmux
Length of output: 335
Fix incomplete PaneKit linking in GhosttyTabs target.
PaneKit is declared in packageProductDependencies but is missing from the Frameworks build phase and has no PBXBuildFile entries. Unlike Sparkle, Sentry, PostHog, and MarkdownUI, the framework is not fully wired up. Re-adding PaneKit to the target in Xcode will regenerate the required build file entries.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@GhosttyTabs.xcodeproj/project.pbxproj` at line 510, PaneKit is listed in
packageProductDependencies but not actually wired into the GhosttyTabs target
(missing Frameworks build phase entries and PBXBuildFile records); open the
GhosttyTabs target in Xcode and re-add the PaneKit package product to the
target’s Frameworks (or Link Binary With Libraries) phase so Xcode regenerates
the missing PBXBuildFile entries and framework linkage; verify PaneKit appears
alongside Sparkle/Sentry/PostHog/MarkdownUI in the Frameworks build phase and
that corresponding PBXBuildFile and fileReference entries are present in the
project.pbxproj.
There was a problem hiding this comment.
2 issues found across 3 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/TabManager.swift">
<violation number="1" location="Sources/TabManager.swift:878">
P2: `nonBlankSampleCounts` can be incremented even when `surfaceSample` is missing, which makes the new visible-content checks report false positives.</violation>
<violation number="2" location="Sources/TabManager.swift:4248">
P3: This branch is unreachable dead code: the action runs before sampling and sets `didPrimeCreatedPane = true`, so the `!didPrimeCreatedPane` path never executes.</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: 1
🧹 Nitpick comments (2)
Sources/TerminalWindowPortal.swift (1)
1722-1727: Consider rate-limiting deferred sync scheduling during continuous motion.The new code schedules
scheduleDeferredFullSynchronizeAll()on every frame change for visible entries. During continuous pane strip animation, this could queue many redundant async tasks. WhilehasDeferredFullSyncScheduledprevents overlapping execution, consider whether this level of polling is necessary or if the presentation layer tracking already handles most cases.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TerminalWindowPortal.swift` around lines 1722 - 1727, The code currently calls scheduleDeferredFullSynchronizeAll() on every frame change which can spam the queue during continuous animation; modify the scheduling in the frame change branch to rate-limit/debounce calls: check hasDeferredFullSyncScheduled and also track a lastDeferredFullSyncScheduledAt timestamp (or use a DispatchWorkItem/DispatchSourceTimer debounce) in TerminalWindowPortal so that scheduleDeferredFullSynchronizeAll() is only enqueued once per short interval (e.g., 50–200ms) during continuous motion, ensuring you still call scheduleDeferredFullSynchronizeAll() when needed but avoid queuing redundant deferred syncs.Sources/Workspace.swift (1)
2312-2374: Extract the shared terminal pane-creation path.
openTerminalPaneRightnow duplicates most ofnewTerminalSplit— panel construction, tab mapping, cleanup, and focus reassertion — and the two paths have already started to drift. A single helper that accepts the controller mutation closure would make future focus/cleanup fixes much less error-prone.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 2312 - 2374, openTerminalPaneRight duplicates most of newTerminalSplit (panel creation, panels/tab mapping, seedTerminalInheritanceFontPoints, cleanup and focus handling); extract that shared path into a helper (e.g., createAndAttachTerminalPanel) that builds the TerminalPanel, registers panels[newPanel.id], panelTitles, surfaceIdToPanelId, calls seedTerminalInheritanceFontPoints(panelId:configTemplate:), and performs the common cleanup on failure, while taking a controller-mutation closure for the bonsplitController call (so callers can call bonsplitController.openPaperCanvasPaneRight or the other split variant). Ensure the helper also centralizes the focus vs preserveFocusAfterNonFocusSplit logic (using focusPanel and previousHostedView handling) so both openTerminalPaneRight and newTerminalSplit call the helper with the appropriate controller mutation closure and only supply the differing arguments.
🤖 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/Workspace.swift`:
- Around line 171-173: The restore currently calls restoreBaseLayoutStyle(for:
snapshot.layout) then setPaneLayoutStyle(...) but postRestoreLayoutStyle returns
Self.paneLayoutStyle (which may be .paperCanvas) causing split/pane snapshots
rebuilt as .splitTree to immediately flip to paperCanvas and lose divider
geometry; change post-restore behaviour to preserve the restored style for
snapshots reconstructed as splitTree by having
restoreBaseLayoutStyle/postRestoreLayoutStyle return and apply the actual
restored layout style (not Self.paneLayoutStyle), i.e., detect when
snapshot.layout was a .pane/.split and keep the computed restoreBaseLayoutStyle
(or store an explicit versioned migration flag) and pass that through to
setPaneLayoutStyle so the layout mode and divider geometry are not overwritten
during restore (update references: restoreBaseLayoutStyle(for:),
setPaneLayoutStyle(_:), postRestoreLayoutStyle).
---
Nitpick comments:
In `@Sources/TerminalWindowPortal.swift`:
- Around line 1722-1727: The code currently calls
scheduleDeferredFullSynchronizeAll() on every frame change which can spam the
queue during continuous animation; modify the scheduling in the frame change
branch to rate-limit/debounce calls: check hasDeferredFullSyncScheduled and also
track a lastDeferredFullSyncScheduledAt timestamp (or use a
DispatchWorkItem/DispatchSourceTimer debounce) in TerminalWindowPortal so that
scheduleDeferredFullSynchronizeAll() is only enqueued once per short interval
(e.g., 50–200ms) during continuous motion, ensuring you still call
scheduleDeferredFullSynchronizeAll() when needed but avoid queuing redundant
deferred syncs.
In `@Sources/Workspace.swift`:
- Around line 2312-2374: openTerminalPaneRight duplicates most of
newTerminalSplit (panel creation, panels/tab mapping,
seedTerminalInheritanceFontPoints, cleanup and focus handling); extract that
shared path into a helper (e.g., createAndAttachTerminalPanel) that builds the
TerminalPanel, registers panels[newPanel.id], panelTitles, surfaceIdToPanelId,
calls seedTerminalInheritanceFontPoints(panelId:configTemplate:), and performs
the common cleanup on failure, while taking a controller-mutation closure for
the bonsplitController call (so callers can call
bonsplitController.openPaperCanvasPaneRight or the other split variant). Ensure
the helper also centralizes the focus vs preserveFocusAfterNonFocusSplit logic
(using focusPanel and previousHostedView handling) so both openTerminalPaneRight
and newTerminalSplit call the helper with the appropriate controller mutation
closure and only supply the differing arguments.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 37a8ea0d-994b-4ac3-85c6-34f536a9ed2b
📒 Files selected for processing (3)
Sources/TabManager.swiftSources/TerminalWindowPortal.swiftSources/Workspace.swift
| let restoreBaseLayoutStyle = restoreBaseLayoutStyle(for: snapshot.layout) | ||
| setPaneLayoutStyle(restoreBaseLayoutStyle) | ||
|
|
There was a problem hiding this comment.
Don’t flip split snapshots to paper-canvas during restore.
.pane / .split snapshots are rebuilt as .splitTree, but postRestoreLayoutStyle immediately returns Self.paneLayoutStyle — which is .paperCanvas now. That changes the restored layout mode and can discard the divider geometry you just reapplied. If a migration is intended, it should be explicit/versioned instead of happening on every restore.
🛠️ Minimal fix
private func postRestoreLayoutStyle(for layout: SessionWorkspaceLayoutSnapshot) -> PaneLayoutStyle {
switch layout {
case .canvas:
return .paperCanvas
- case .pane, .split:
- return Self.paneLayoutStyle
+ case .pane, .split:
+ return .splitTree
}
}Also applies to: 195-198, 245-260
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Workspace.swift` around lines 171 - 173, The restore currently calls
restoreBaseLayoutStyle(for: snapshot.layout) then setPaneLayoutStyle(...) but
postRestoreLayoutStyle returns Self.paneLayoutStyle (which may be .paperCanvas)
causing split/pane snapshots rebuilt as .splitTree to immediately flip to
paperCanvas and lose divider geometry; change post-restore behaviour to preserve
the restored style for snapshots reconstructed as splitTree by having
restoreBaseLayoutStyle/postRestoreLayoutStyle return and apply the actual
restored layout style (not Self.paneLayoutStyle), i.e., detect when
snapshot.layout was a .pane/.split and keep the computed restoreBaseLayoutStyle
(or store an explicit versioned migration flag) and pass that through to
setPaneLayoutStyle so the layout mode and divider geometry are not overwritten
during restore (update references: restoreBaseLayoutStyle(for:),
setPaneLayoutStyle(_:), postRestoreLayoutStyle).
There was a problem hiding this comment.
2 issues found across 3 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/TabManager.swift">
<violation number="1" location="Sources/TabManager.swift:4703">
P3: The source-pane first-nonblank frame is keyed only to `"T"`, so scenarios that use label `"L"` record incorrect diagnostics.</violation>
</file>
<file name="Sources/GhosttyTerminalView.swift">
<violation number="1" location="Sources/GhosttyTerminalView.swift:8104">
P2: Inline motion diagnostics use the host view layer for hosted-layer fields; use `surfaceView.layer` for `hostedLayerClass` and `hostedHasMetalLayer` to avoid incorrect debug telemetry.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| hostedLayerClass: layerClass, | ||
| hostedHasMetalLayer: (layer as? CAMetalLayer) != nil, |
There was a problem hiding this comment.
P2: Inline motion diagnostics use the host view layer for hosted-layer fields; use surfaceView.layer for hostedLayerClass and hostedHasMetalLayer to avoid incorrect debug telemetry.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/GhosttyTerminalView.swift, line 8104:
<comment>Inline motion diagnostics use the host view layer for hosted-layer fields; use `surfaceView.layer` for `hostedLayerClass` and `hostedHasMetalLayer` to avoid incorrect debug telemetry.</comment>
<file context>
@@ -8069,11 +8092,17 @@ final class GhosttySurfaceScrollView: NSView {
hostedHidden: isHidden || window.occlusionState.contains(.visible) == false,
+ hostLayerClass: layerClass,
+ hostWantsLayer: wantsLayer,
+ hostedLayerClass: layerClass,
+ hostedHasMetalLayer: (layer as? CAMetalLayer) != nil,
surfaceSample: debugSampleIOSurface(normalizedCrop: normalizedCrop)
</file context>
| hostedLayerClass: layerClass, | |
| hostedHasMetalLayer: (layer as? CAMetalLayer) != nil, | |
| hostedLayerClass: surfaceView.layer.map { String(describing: type(of: $0)) } ?? "nil", | |
| hostedHasMetalLayer: (surfaceView.layer as? CAMetalLayer) != nil, |
| if result.nonBlankSampleCounts["T", default: 0] == 0 { | ||
| var extra = terminalVisibilityDebugInfo( | ||
| for: sourcePanelId, | ||
| firstNonBlankFrame: result.firstNonBlankFrameByLabel["T"] |
There was a problem hiding this comment.
P3: The source-pane first-nonblank frame is keyed only to "T", so scenarios that use label "L" record incorrect diagnostics.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/TabManager.swift, line 4703:
<comment>The source-pane first-nonblank frame is keyed only to `"T"`, so scenarios that use label `"L"` record incorrect diagnostics.</comment>
<file context>
@@ -4673,7 +4698,10 @@ class TabManager: ObservableObject {
- var extra = terminalVisibilityDebugInfo(for: sourcePanelId)
+ var extra = terminalVisibilityDebugInfo(
+ for: sourcePanelId,
+ firstNonBlankFrame: result.firstNonBlankFrameByLabel["T"]
+ )
extra["sampleCounts"] = debugJSONString(result.sampleCounts)
</file context>
1 participant
Summary
Dependencies
Testing
cd PaneKit && swift test --filter 'PaperCanvasStripStateTests|BonsplitTests'\n-xcodebuild -project GhosttyTabs.xcodeproj -scheme cmux-unit -destination 'platform=macOS' -derivedDataPath /tmp/cmux-pane-strip-stability test -only-testing:cmuxTests/WorkspacePaperCanvasTests -only-testing:cmuxTests/AppDelegateShortcutRoutingTests/testCmdOptionNOpensPaneRightWithoutShrinkingSourcePane -only-testing:cmuxTests/AppDelegateShortcutRoutingTests/testCmdShiftDDoesNotCreateVerticalPaneInPaperCanvasWorkspace -only-testing:cmuxTests/CommandPaletteSearchEngineTests/testPaneLayoutQueriesDifferentiateOpenPaneRightFromSplitRight -only-testing:cmuxTests/CommandPaletteSearchEngineTests/testCommandPaletteShortcutMappingIncludesOpenPaneRight\n-python3 Tests/test_pane_strip_motion.pyequivalent scenarios verified oncmux-sequoia\n- GitHub UI run in progress: https://github.com/manaflow-ai/cmux/actions/runs/23222285091\n\nCloses https://github.com/manaflow-ai/cmux/issues/1221\nSummary by cubic
Implements a horizontal paper‑canvas pane strip as the default workspace layout to keep pane widths stable and keep terminals and browser portals aligned during panning, focusing, splits, and new pane opens. Adds Open Pane Right, workspace viewport panning (CLI/RPC), and further tightens browser/terminal portal alignment during animated pane changes; improves CI with E2E
xcresultuploads (per #1221).New Features
PaneKitwith a default horizontal pane strip, overflow hints, and stable pane widths.cmux pan-workspace --dx/--dyandworkspace.viewport.panRPC; layout, viewport, and focus persist/restore.xcresultbundles; explicit pane‑strip motion diagnostics init.Dependencies
bonsplitsubmodule with localPaneKitSwift package.ghosttysubmodule and pinned the matchingGhosttyKitxcframework for motion/visibility fixes.Written for commit ffa9d54. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation