Niri-style terminal canvas with per-panel tabs and drag-drop#2220
Niri-style terminal canvas with per-panel tabs and drag-drop#2220lawrencecchen wants to merge 16 commits intomainfrom
Conversation
Merges ghostty-org/ghostty main into manaflow-ai/ghostty fork, preserving all cmux-specific patches (OSC 99, cursor-click-to-move, resize fixes, Pure prompt markers, theme picker).
- read_clipboard_cb now returns Bool (was Void) - Handle new GHOSTTY_ACTION_SET_TAB_TITLE action - Update ghostty.h to match new upstream header, preserving cmux-specific declarations (select_cursor_cell, clear_selection)
Horizontal strip layout with real ghostty terminal surfaces. Each terminal renders independently via ghostty's Metal renderer. Features: - Cmd+Opt+Arrow / Cmd+Ctrl+H/L to navigate between panels - Cmd+Ctrl+R to cycle per-panel resize presets (33%/67%/100%) - Cmd+W / Ctrl+D to close, Cmd+T to add new terminal - Smooth 120fps animation via CVDisplayLink - Smart scroll: only moves when panel is off-screen - Animated resize with left-edge pinning and visibility clamping Triggered with Cmd+Ctrl+N in debug builds.
- Per-panel tab system: each panel has its own tab bar with tabs - Cmd+T adds tab, Cmd+N adds panel, Ctrl+1-9 switches tabs - Cross-panel tab drag with floating drag image and blue drop indicators - Drop onto another panel's tab bar moves the tab there - Drop in gap between panels creates a new panel (inherits source width) - Escape cancels drag - Auto-scroll: hovering over a panel during drag scrolls it into view - Click on terminal/tab focuses that panel and scrolls it into view - Fixed focus-after-drop: recomputes live index after panel changes - Fixed hitTest focus stealing: removed side-effect from hitTest, focus-on-click handled in NiriCanvasWindow.sendEvent instead - All operations logged via dlog for debugging
|
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.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a debug-only multi-panel "Niri Canvas" UI (tab bars, cross-panel drag/reorder, keyboard controls, window/controller), registers the new source in the Xcode project, adds a debug AppDelegate shortcut to open it, adds UI tests and demo scripts, updates ghostty C API and submodule, and adjusts clipboard/tab-title handling in GhosttyTerminalView. Changes
Sequence DiagramsequenceDiagram
participant User
participant TabBar as NiriTabBarView
participant Canvas as NiriCanvasView
participant Window as NiriCanvasWindow
participant Display as CVDisplayLink
User->>TabBar: mouseDown & drag
TabBar->>Canvas: onDragStart(tabIndex, event)
Canvas->>Canvas: create floating text layer, start drag loop
loop while dragging
User->>Canvas: mouse move
Canvas->>Canvas: compute drop target, auto-scroll
Canvas->>TabBar: showDropIndicator(at:index?)
Canvas->>Window: suppress hit-test / route events
Display->>Canvas: tick -> request visual update
end
User->>Canvas: mouseUp
Canvas->>Canvas: finalize drop (reorder/move/new panel), restore state
Display->>Canvas: tick -> animate layout updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR introduces a Niri/PaperWM-style horizontal terminal canvas prototype (Cmd+Ctrl+N, Key changes:
Issues found:
Confidence Score: 3/5Safe to merge as a debug-only prototype, but three correctness bugs in NiriCanvasView.swift should be addressed before the feature is used in regular development sessions. All new code is guarded by #if DEBUG so there is zero production risk. However, NSSetUncaughtExceptionHandler clobbering Sentry's handler and the debugNoGhostty resource leak are genuine bugs that will affect anyone exercising the canvas during normal development, and the CVDisplayLink unretained-pointer pattern is a latent use-after-free. The scroll direction inversion makes the feature feel broken on machines with natural scrolling. These four issues together warrant addressing before the branch is merged, even as a prototype. Sources/NiriCanvasView.swift requires the most attention — all four bugs are concentrated there. Important Files Changed
Reviews (1): Last reviewed commit: "Add GhosttyKit checksum for ghostty 2c53..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| contentRect: NSRect(x: scr.midX - 700, y: scr.midY - 350, width: 1400, height: 700), | ||
| styleMask: [.titled, .closable, .resizable], | ||
| backing: .buffered, defer: false | ||
| ) |
There was a problem hiding this comment.
NSSetUncaughtExceptionHandler is a process-wide singleton. Setting it inside NiriCanvasWindowController.init() silently replaces any handler installed by Sentry (or any other crash reporter already wired up in the app), leaving only a bare NSLog behind. This means:
- After the user first opens the canvas with Cmd+Ctrl+N, the Sentry crash reporter loses its exception hook for the remainder of the app's lifetime.
- Any ObjC exception thrown after that point (in any part of the app, not just the canvas) will be logged to Console but not sent to Sentry.
Because this is guarded by #if DEBUG, it won't ship to users — but it will silently break crash telemetry in developer debug builds the moment someone tries the canvas, which is exactly when you want Sentry working.
Consider removing this entirely (the stack symbols will still appear in Xcode's crash log), or at least storing the previous handler and chaining to it:
// Store existing handler first, then chain to it
let prev = NSGetUncaughtExceptionHandler()
NSSetUncaughtExceptionHandler { exception in
NSLog("niri.UNCAUGHT: \(exception.name) reason=\(exception.reason ?? "nil")")
NSLog("niri.UNCAUGHT.stack: \(exception.callStackSymbols.joined(separator: "\n"))")
prev?(exception)
}| }) | ||
| } else { | ||
| guard GhosttyApp.shared.app != nil else { return } | ||
| canvasView.setSurfaces((0..<terminalCount).map { _ in | ||
| TerminalSurface(tabId: UUID(), context: GHOSTTY_SURFACE_CONTEXT_SPLIT, configTemplate: nil) | ||
| }) | ||
| } | ||
| showWindow(nil); window?.makeKeyAndOrderFront(nil) | ||
| if !debugNoGhostty { | ||
| DispatchQueue.main.asyncAfter(deadline: .now() + 0.5) { [weak self] in | ||
| self?.canvasView.focusCurrentTerminal() | ||
| } | ||
| } |
There was a problem hiding this comment.
debugNoGhostty branch still creates real terminal surfaces
Both branches of the if debugNoGhostty / else block call the identical TerminalSurface(tabId:context:configTemplate:) constructor. TerminalSurface.init initialises a real ghostty surface — so even in debug-placeholder mode, live terminal processes are spawned (and their hostedView is never added to the view hierarchy, so they are orphaned).
The if debugNoGhostty branch should use lightweight placeholder objects or skip surface creation entirely, otherwise debugNoGhostty only suppresses the view but not the underlying ghostty process.
| return kCVReturnSuccess | ||
| } | ||
| CVDisplayLinkSetOutputCallback(displayLink, cb, Unmanaged.passUnretained(self).toOpaque()) | ||
| CVDisplayLinkStart(displayLink) | ||
| } | ||
|
|
||
| private func tick() { | ||
| var anyResizing = false | ||
| for i in 0..<panels.count { | ||
| let d = panels[i].targetWidth - panels[i].currentWidth |
There was a problem hiding this comment.
self reference in CVDisplayLink callback
Unmanaged.passUnretained(self).toOpaque() stores a raw pointer to self in the display link callback context. CVDisplayLinkStop is called in deinit, but the CVDisplayLink framework can fire one final callback on a background thread after CVDisplayLinkStop returns (the stop is asynchronous). If that trailing callback fires after self is deallocated, takeUnretainedValue() dereferences freed memory — even though the async dispatch uses [weak v], the takeUnretainedValue() call itself happens before the weak reference check.
| if nf != focusedIndex { focusedIndex = nf; focusCurrentTerminal() } | ||
| } | ||
|
|
||
| private func nearestLive(forOffset off: CGFloat) -> Int { | ||
| var best = 0; var bestD = CGFloat.infinity; var x: CGFloat = 0; var li = 0 | ||
| for p in panels where !p.closing { | ||
| let d = abs(x + pw(for: p) / 2 - off) |
There was a problem hiding this comment.
When "natural scrolling" is enabled, isDirectionInvertedFromDevice is true. The code negates both deltas in that case, which moves the strip in the opposite direction from what the user expects. With natural scrolling on, scrollingDeltaX is already in the "content moves with your fingers" direction — no inversion is needed. Consider flipping the condition:
if !event.isDirectionInvertedFromDevice { dx = -dx; dy = -dy }
let delta = abs(dx) > abs(dy) ? dx : dy
targetOffset += delta * 2.0| dl.shadowColor = CGColor(gray: 0, alpha: 1) | ||
| dl.shadowOpacity = 0.5 | ||
| dl.shadowRadius = 8 | ||
| dl.contentsScale = window?.backingScaleFactor ?? 2 | ||
| dl.zPosition = 1000 | ||
| dl.anchorPoint = CGPoint(x: 0.5, y: 0.5) | ||
| layer?.addSublayer(dl) | ||
| dragLayer = dl | ||
|
|
There was a problem hiding this comment.
NSLog fires on every mouse movement over tab bar
PanelContainerView.hitTest logs "niri.hitTest -> TAB BAR" every time hitTest is triggered for a tab bar region hit. On macOS, hitTest is called on every mouse-moved event — at 120 Hz with the cursor over a tab bar, this produces ~120 log lines per second to the system log. Suggest wrapping it behind a #if VERBOSE_LOGGING flag or removing it before the PR merges, keeping only the "MISS" log which fires much less frequently.
Sources/NiriCanvasView.swift
Outdated
| panels[pi].closing = true | ||
| if let s = panels[pi].activeSurface?.surface { ghostty_surface_request_close(s) } | ||
| if liveCount > 0 { | ||
| focusedIndex = min(focusedIndex, liveCount - 1) | ||
| scrollToReveal() | ||
| focusCurrentTerminal() | ||
| } | ||
| } else { | ||
| let idx = panels[pi].activeTab | ||
| let surface = panels[pi].tabs[idx] | ||
| surface.hostedView.removeFromSuperview() | ||
| if let s = surface.surface { ghostty_surface_request_close(s) } | ||
| panels[pi].tabs.remove(at: idx) | ||
| panels[pi].activeTab = min(idx, panels[pi].tabs.count - 1) | ||
| panels[pi].syncTabBar() | ||
| if let a = panels[pi].activeSurface { | ||
| a.hostedView.removeFromSuperview() | ||
| panels[pi].containerView.addSubview(a.hostedView) | ||
| } | ||
| layoutStrip(); scrollToReveal(); focusCurrentTerminal() | ||
| } | ||
| } | ||
|
|
||
| func addNewTab() { | ||
| nlog("addNewTab") | ||
| guard GhosttyApp.shared.app != nil else { return } | ||
| let live = liveIndices | ||
| guard focusedIndex < live.count else { addNewPanel(); return } |
There was a problem hiding this comment.
closeActiveTab() panel-close path skips layoutStrip()
When a panel's last tab is closed, closing is set to true but layoutStrip() is never called synchronously. The multi-tab close path explicitly calls layoutStrip(). The tick() display-link callback will eventually call it, but until that tick fires there can be a stale-layout frame where the focused index has changed but the container frames still reflect the old layout.
panels[pi].closing = true
if let s = panels[pi].activeSurface?.surface { ghostty_surface_request_close(s) }
if liveCount > 0 {
focusedIndex = min(focusedIndex, liveCount - 1)
scrollToReveal()
layoutStrip() // ← add to match multi-tab path
focusCurrentTerminal()
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/GhosttyTerminalView.swift (1)
1102-1208:⚠️ Potential issue | 🟠 MajorMove the
runtimeSurfacecheck before returningtrueto prevent broken clipboard acknowledgements.The callback returns
trueat line 1208 before verifying a live surface exists. SinceruntimeSurfaceis only checked inside the dispatched async block (line 1105), the surface can be torn down between the return statement and async execution. If teardown completes (which setssurface = nilbefore deferred free), the dispatched block exits early without callingghostty_surface_complete_clipboard_request, leaving Ghostty's callback incomplete. This violates the callback protocol contract.Capture the live surface before returning
true, or fail fast:Suggested fix
runtimeConfig.read_clipboard_cb = { userdata, location, state in - guard let callbackContext = GhosttyApp.callbackContext(from: userdata) else { return false } + guard let callbackContext = GhosttyApp.callbackContext(from: userdata), + let requestSurface = callbackContext.runtimeSurface else { return false } DispatchQueue.main.async { - guard let requestSurface = callbackContext.runtimeSurface else { return } func completeClipboardRequest(with text: String) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/GhosttyTerminalView.swift` around lines 1102 - 1208, The callback currently returns true before ensuring a live runtimeSurface exists, risking that the async block will early-return and never call ghostty_surface_complete_clipboard_request; fix by capturing/validating callbackContext.runtimeSurface synchronously (e.g., bind a local requestSurface or guard runtimeSurface != nil) before scheduling DispatchQueue.main.async and only return true when you have a valid captured surface, otherwise return false; ensure completeClipboardRequest still uses that captured requestSurface and that callbackContext/runtimeSurface checks inside the async block are updated to use the captured value where appropriate.
🧹 Nitpick comments (1)
cmuxUITests/NiriCanvasUITests.swift (1)
129-142: Please assert the reorder result.This currently only proves the window survives the drag. A missed hit target, ignored drag, or no-op reorder still passes, so the test will not catch regressions in the feature it is named after. Expose one stable observable for tab order (for example the same kind of debug/accessibility hook used by
BonsplitTabDragUITests) and assert that the order actually changes.Based on learnings, tests must verify observable runtime behavior through executable paths, not just exercise a path without checking the feature result.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxUITests/NiriCanvasUITests.swift` around lines 129 - 142, The test currently only checks canvasWindow.exists and does not verify tab reorder; update NiriCanvasUITests to assert the actual tab order by exposing a stable observable (e.g. add accessibilityIdentifiers or a debug/access API on NiriTabBarView that returns tab order) similar to the mechanism used in BonsplitTabDragUITests, perform the drag (Cmd+T then drag) and then query that observable to assert the order changed (compare before/after order) so the test fails when the reorder is a no-op or missed hit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ghostty`:
- Line 1: Update the docs/ghostty-fork.md file to reflect the new submodule sync
to commit 2c530daf (submodule HEAD db6df246d...), replacing the old "as of March
12, 2026" timestamp with the current sync date; add a short "Sync to 2c530daf"
section that lists the primary changes pulled in from that commit, explicitly
document any conflict resolutions applied during the merge (file names and brief
resolution notes), and confirm you followed the fork update checklist present in
the document (e.g., tests run, build verified, and changes reviewed).
- Line 1: The ghostty submodule is in a detached HEAD at commit db6df246... and
references a commit 2c530daf0647... that isn’t present on the
manaflow-ai/ghostty fork and its remote is incorrectly set to manaflow-ai/cmux;
fix this by entering the ghostty submodule, re-pointing its remote to the
correct repo (manaflow-ai/ghostty), fetching remotes, checking out or creating
the intended branch (e.g., main) so HEAD is not detached, ensure the local
commit 2c530daf0647... is present on that branch (apply, cherry-pick, or
recreate the commit if needed) and push the branch to the manaflow-ai/ghostty
fork, then update the parent repo’s submodule pointer to the pushed commit and
commit that change in the parent repository.
In `@scripts/demo-paperwm.swift`:
- Around line 213-226: The z ordering is inverted because zOffset is computed
with a leading negative: change the zOffset calculation in the transform block
(variable zOffset, which currently uses -abs(distFromCenter) * perspectiveZ) so
it does not negate the product; compute zOffset as abs(distFromCenter) *
perspectiveZ (or otherwise remove the unary '-' ) so that given perspectiveZ is
negative (−120) side panels receive a negative zOffset and correctly recede from
the camera.
- Around line 10-12: The file is missing the CoreVideo import required for
CVDisplayLink and related types; add an import CoreVideo statement near the top
alongside existing imports (so the CVDisplayLink, CVDisplayLinkOutputCallback,
CVDisplayLinkStart/Stop and teardown code in the display link setup and teardown
functions will compile).
In `@scripts/test-tabbar.swift`:
- Around line 106-127: Add an NSApplicationDelegate that returns true from
applicationShouldTerminateAfterLastWindowClosed and assign it to the existing
app (let app = NSApplication.shared) so the helper quits when the last window
closes; specifically, create a small AppDelegate class implementing
applicationShouldTerminateAfterLastWindowClosed(_:)->Bool { return true } and
set app.delegate = AppDelegate() before calling app.run(), leaving TestTabBar
and the window setup unchanged.
In `@Sources/AppDelegate.swift`:
- Around line 9091-9095: The Cmd+Ctrl+N handler (the if block that checks
hasCommand && hasControl && !flags.contains(.shift) && (chars.lowercased() ==
"n" || event.keyCode == 45)) runs before the command-palette consumption guard
and can open the demo while the palette is active; either move this
openNiriCanvas() block to after the existing palette-consumption guard or add an
explicit early return when the command palette is visible (i.e., check the same
"palette is visible/consumes commands" condition used elsewhere) so that
openNiriCanvas() is not invoked while the palette has focus.
- Around line 9087-9090: Update the shortcut-handling guard to resolve the
actual window using the event's windowNumber or NSApp.keyWindow when
event.window is nil (instead of only checking event.window). In the method that
contains the current check (the block using "if event.window is NiriCanvasWindow
{ return false }" inside handleCustomShortcut / AppDelegate), obtain a
resolvedWindow from event.window ?? NSApp.window(withWindowNumber:
event.windowNumber) ?? NSApp.keyWindow and use that to test "is
NiriCanvasWindow" so key-equivalent events delivered with event.window == nil
are correctly treated as coming from the canvas.
In `@Sources/GhosttyTerminalView.swift`:
- Around line 2455-2458: The handler is currently swallowing
GHOSTTY_ACTION_SET_TAB_TITLE by returning true; instead, mirror the SET_TITLE
handling: construct and post the same notification used for terminal-driven
title updates (the ghosttyDidSetTitle notification and its payload) but
mark/apply it as the workspace/tab title update so the workspace tab manager
updates the tab label; update the switch case for GHOSTTY_ACTION_SET_TAB_TITLE
to build the same userInfo as SET_TITLE and post the notification rather than
returning true.
In `@Sources/NiriCanvasView.swift`:
- Around line 1005-1012: The window-level shortcut handler is swallowing
unhandled Command combos (the final `if cmd { return }`) which prevents a
focused Ghostty surface from receiving terminal input; update the logic around
the canvas key handling so that after trying canvas.performKeyEquivalent(with:)
you only consume Cmd combos that are truly handled and otherwise forward
unhandled Cmd+Shift (and other unbound Cmd combos) to the canvas/terminal.
Concretely, modify the block containing handleCtrlD(), switchToTabPublic(...),
and the call to canvas.performKeyEquivalent(with:) so that you do not
unconditionally `return` on `if cmd` — instead detect when a real canvas
(GhosttyNSView) is focused and let its performKeyEquivalent take precedence or
explicitly forward unhandled Cmd+Shift combos to the canvas before swallowing
the event.
- Around line 547-560: When removing a tab from a panel (the block manipulating
panels[srcPi].tabs, panels[srcPi].activeTab, panels[srcPi].activeSurface, and
calling panels[srcPi].syncTabBar), preserve the currently displayed surface if
the removed tab is left of the current activeTab: instead of always setting
activeTab = min(...), if srcTab < panels[srcPi].activeTab decrement
panels[srcPi].activeTab by 1; otherwise set it to min(...). Apply the identical
change in the other helper that performs tab removal (the other code path that
also updates panels[...].tabs, activeTab, activeSurface and syncTabBar) so the
activeSurface does not silently jump when a left-side tab is dragged out.
- Around line 503-511: When handling case .inTabBar you must adjust insertIdx
for same-panel rightward moves because removing the source first shifts targets
left; compute an adjusted index (e.g. let adjustedInsert = insertIdx -
(sourceTab < insertIdx ? 1 : 0)), clamp it against panels[sourcePi].tabs.count -
1, and pass adjustedInsert to reorderTab(from:to:inPanel:) instead of the raw
insertIdx; keep the rest of the liveIndices/targetPi/sourcePi logic intact.
- Around line 238-240: The setSurfaces implementation is dropping
TerminalSurface instances by replacing panels without calling their teardown
path; update setSurfaces (and the similar code at the other occurrences) to
iterate the existing panels' TerminalSurface(s) and call teardownSurface() for
each discarded surface before removing container views, then update panels =
surfaces.map { makePanel(with: [$0]) }; ensure you reference the existing panels
array, the makePanel(with:) helper, and the teardownSurface() method so the
portal sealing lifecycle used in GhosttyTerminalView's teardown is executed for
each surface being replaced.
- Line 127: Replace hard-coded UI strings with localized lookups: in
NiriCanvasView where you set let title = tab.title.isEmpty ? "Shell" :
String(tab.title.prefix(20)) and at the other noted locations (the "Terminal
Canvas" uses and other literal strings around lines referenced), change the
literals to use String(localized: "key.name", defaultValue: "Shell") or
appropriate keys (e.g., "terminal.canvas.title") and add corresponding entries
in Resources/Localizable.xcstrings; ensure you use descriptive key names, keep
the same default English text in defaultValue, and update all occurrences
mentioned ("Shell", "Terminal Canvas", and the other two literals) to follow
this pattern so user-facing strings are localized.
---
Outside diff comments:
In `@Sources/GhosttyTerminalView.swift`:
- Around line 1102-1208: The callback currently returns true before ensuring a
live runtimeSurface exists, risking that the async block will early-return and
never call ghostty_surface_complete_clipboard_request; fix by
capturing/validating callbackContext.runtimeSurface synchronously (e.g., bind a
local requestSurface or guard runtimeSurface != nil) before scheduling
DispatchQueue.main.async and only return true when you have a valid captured
surface, otherwise return false; ensure completeClipboardRequest still uses that
captured requestSurface and that callbackContext/runtimeSurface checks inside
the async block are updated to use the captured value where appropriate.
---
Nitpick comments:
In `@cmuxUITests/NiriCanvasUITests.swift`:
- Around line 129-142: The test currently only checks canvasWindow.exists and
does not verify tab reorder; update NiriCanvasUITests to assert the actual tab
order by exposing a stable observable (e.g. add accessibilityIdentifiers or a
debug/access API on NiriTabBarView that returns tab order) similar to the
mechanism used in BonsplitTabDragUITests, perform the drag (Cmd+T then drag) and
then query that observable to assert the order changed (compare before/after
order) so the test fails when the reorder is a no-op or missed hit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8995934f-8d6a-4385-9d22-bcae013ac2c3
📒 Files selected for processing (9)
GhosttyTabs.xcodeproj/project.pbxprojSources/AppDelegate.swiftSources/GhosttyTerminalView.swiftSources/NiriCanvasView.swiftcmuxUITests/NiriCanvasUITests.swiftghosttyghostty.hscripts/demo-paperwm.swiftscripts/test-tabbar.swift
| @@ -1 +1 @@ | |||
| Subproject commit bc9be90a21997a4e5f06bf15ae2ec0f937c2dc42 | |||
| Subproject commit 2c530daf06473067af537db209a0f3c598881281 | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if docs/ghostty-fork.md was updated alongside the submodule
echo "=== Checking if docs/ghostty-fork.md exists and was recently modified ==="
if [ -f docs/ghostty-fork.md ]; then
echo "File exists. Recent content:"
echo ""
head -50 docs/ghostty-fork.md
echo ""
echo "=== Checking for mentions of the new commit or recent sync ==="
rg -i "2c530daf|1043.*commit|sync.*2026-03" docs/ghostty-fork.md || echo "No mention of recent sync found"
else
echo "✗ docs/ghostty-fork.md not found!"
fiRepository: manaflow-ai/cmux
Length of output: 2229
🏁 Script executed:
#!/bin/bash
echo "=== Full content of docs/ghostty-fork.md ==="
cat docs/ghostty-fork.md
echo ""
echo "=== Current ghostty submodule commit ==="
cd ghostty && git rev-parse HEAD && cd -
echo ""
echo "=== Checking git log for docs/ghostty-fork.md in recent commits ==="
git log --oneline -n 20 -- docs/ghostty-fork.mdRepository: manaflow-ai/cmux
Length of output: 6089
Update docs/ghostty-fork.md to document the ghostty sync to commit 2c530daf.
The file exists and contains comprehensive fork documentation, but it was not updated in this PR. The file still references "as of March 12, 2026" and lacks any mention of the new commit 2c530daf (submodule HEAD: db6df246d...). Add a section documenting what changed in this sync, which conflict resolutions were applied (if any), and update the timestamp. Per the fork update checklist in the file itself, this documentation must be updated alongside submodule changes.
🧰 Tools
🪛 GitHub Actions: CI
[error] 1-1: Missing pinned GhosttyKit checksum for ghostty 2c530daf06473067af537db209a0f3c598881281 in scripts/ghosttykit-checksums.txt
[error] 1-1: Command failed with exit code 1 (process completed with exit code 1) due to missing pinned GhosttyKit checksum
🪛 GitHub Actions: macOS Compatibility
[error] 1-1: Missing pinned GhosttyKit checksum for ghostty 2c530daf06473067af537db209a0f3c598881281 in /Users/runner/work/cmux/cmux/scripts/ghosttykit-checksums.txt
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ghostty` at line 1, Update the docs/ghostty-fork.md file to reflect the new
submodule sync to commit 2c530daf (submodule HEAD db6df246d...), replacing the
old "as of March 12, 2026" timestamp with the current sync date; add a short
"Sync to 2c530daf" section that lists the primary changes pulled in from that
commit, explicitly document any conflict resolutions applied during the merge
(file names and brief resolution notes), and confirm you followed the fork
update checklist present in the document (e.g., tests run, build verified, and
changes reviewed).
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify ghostty submodule commit exists in manaflow-ai/ghostty fork
COMMIT="2c530daf06473067af537db209a0f3c598881281"
echo "=== Checking if commit exists in manaflow-ai/ghostty fork ==="
gh api repos/manaflow-ai/ghostty/commits/$COMMIT --jq '.sha // "COMMIT NOT FOUND"' 2>&1
echo ""
echo "=== Checking if commit is on main branch ==="
cd ghostty
git fetch origin main 2>/dev/null || echo "Could not fetch origin/main"
if git merge-base --is-ancestor $COMMIT origin/main 2>/dev/null; then
echo "✓ Commit $COMMIT is on origin/main"
else
echo "✗ Commit $COMMIT is NOT on origin/main (detached HEAD or temp branch?)"
fi
cd ..Repository: manaflow-ai/cmux
Length of output: 414
🏁 Script executed:
#!/bin/bash
# Get detailed info about ghostty submodule state
echo "=== Current ghostty submodule state ==="
cd ghostty
echo "Current HEAD:"
git rev-parse HEAD
echo ""
echo "=== Is HEAD on a branch? ==="
git rev-parse --abbrev-ref HEAD
echo ""
echo "=== Check if commit is reachable from any branch ==="
git branch -r --contains 2c530daf06473067af537db209a0f3c598881281 || echo "Commit not found in any remote branch"
echo ""
echo "=== Local branches ==="
git branch -a | head -20
echo ""
echo "=== Recent commits ==="
git log --oneline -10
echo ""
echo "=== Git remote ==="
git remote -vRepository: manaflow-ai/cmux
Length of output: 807
Submodule configuration is broken and must be fixed before merging.
The ghostty submodule has multiple critical issues:
- Detached HEAD: Submodule is at detached HEAD
db6df246d...instead of being on a branch - Wrong remote: Submodule remote incorrectly points to
manaflow-ai/cmuxinstead ofmanaflow-ai/ghostty - Unpushed commit: The commit
2c530daf06473067af537db209a0f3c598881281referenced in the parent repo does not exist in any branch of the manaflow-ai/ghostty fork
The submodule must be fixed by:
- Checking out the correct branch in the ghostty submodule
- Pushing any local changes to the manaflow-ai/ghostty fork's main branch
- Updating the submodule remote configuration if necessary
- Only then updating the parent repo's submodule pointer
🧰 Tools
🪛 GitHub Actions: CI
[error] 1-1: Missing pinned GhosttyKit checksum for ghostty 2c530daf06473067af537db209a0f3c598881281 in scripts/ghosttykit-checksums.txt
[error] 1-1: Command failed with exit code 1 (process completed with exit code 1) due to missing pinned GhosttyKit checksum
🪛 GitHub Actions: macOS Compatibility
[error] 1-1: Missing pinned GhosttyKit checksum for ghostty 2c530daf06473067af537db209a0f3c598881281 in /Users/runner/work/cmux/cmux/scripts/ghosttykit-checksums.txt
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ghostty` at line 1, The ghostty submodule is in a detached HEAD at commit
db6df246... and references a commit 2c530daf0647... that isn’t present on the
manaflow-ai/ghostty fork and its remote is incorrectly set to manaflow-ai/cmux;
fix this by entering the ghostty submodule, re-pointing its remote to the
correct repo (manaflow-ai/ghostty), fetching remotes, checking out or creating
the intended branch (e.g., main) so HEAD is not detached, ensure the local
commit 2c530daf0647... is present on that branch (apply, cherry-pick, or
recreate the commit if needed) and push the branch to the manaflow-ai/ghostty
fork, then update the parent repo’s submodule pointer to the pushed commit and
commit that change in the parent repository.
| import AppKit | ||
| import QuartzCore | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "demo-paperwm.swift" -type fRepository: manaflow-ai/cmux
Length of output: 88
🌐 Web query:
CVDisplayLink CoreVideo framework Swift import
💡 Result:
To use CVDisplayLink from the CoreVideo framework in Swift on macOS, import the CoreVideo module with import CoreVideo. CVDisplayLink is a type and functions within this framework for creating high-priority display synchronization threads. Example basic setup in a Swift class (e.g., NSOpenGLView subclass): import CoreVideo // CoreVideo framework import for CVDisplayLink class MyView: NSOpenGLView { private var displayLink: CVDisplayLink? override func viewDidMoveToWindow { super.viewDidMoveToWindow if let window = window, window.isVisible { createDisplayLink } } private func createDisplayLink { var link: CVDisplayLink? let result = CVDisplayLinkCreateWithActiveCGDisplays(&link) guard result == kCVReturnSuccess, let displayLink = link else { return } self.displayLink = displayLink // Modern Swift handler (macOS 10.11+) CVDisplayLinkSetOutputHandler(displayLink, { (displayLink, now, outputTime, flagsIn, flagsOut) -> CVReturn in // Render frame here, e.g., self.renderFrame(at: outputTime.pointee) return kCVReturnSuccess }) CVDisplayLinkStart(displayLink) } deinit { if let displayLink = displayLink { CVDisplayLinkStop(displayLink) } } } Link CoreVideo.framework in your Xcode project target Build Phases > Link Binary With Libraries. This synchronizes rendering with display refresh rates, ideal for games or animations.
Citations:
- 1: https://developer.apple.com/documentation/CoreVideo/cvdisplaylink-k0k
- 2: https://stackoverflow.com/questions/25981553/cvdisplaylink-with-swift
- 3: https://gist.github.com/CanTheAlmighty/ee76fbf701a61651fe439fcd6d25f41d?permalink_comment_id=2725094
- 4: https://developer.apple.com/documentation/CoreVideo/CVDisplayLink
- 5: https://developer.apple.com/documentation/corevideo
🏁 Script executed:
cat -n ./scripts/demo-paperwm.swift | head -100Repository: manaflow-ai/cmux
Length of output: 4732
🏁 Script executed:
cat -n ./scripts/demo-paperwm.swift | sed -n '1,300p' | tail -200Repository: manaflow-ai/cmux
Length of output: 9715
🏁 Script executed:
cat -n ./scripts/demo-paperwm.swift | tail -100Repository: manaflow-ai/cmux
Length of output: 3888
Add the missing CoreVideo import.
CVDisplayLink, CVDisplayLinkOutputCallback, and related CoreVideo functions are used throughout the display link setup (lines 285–296) and teardown (line 317) but CoreVideo is not imported. This will fail to compile.
💡 Suggested fix
import AppKit
+import CoreVideo
import QuartzCore📝 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.
| import AppKit | |
| import QuartzCore | |
| import AppKit | |
| import CoreVideo | |
| import QuartzCore | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/demo-paperwm.swift` around lines 10 - 12, The file is missing the
CoreVideo import required for CVDisplayLink and related types; add an import
CoreVideo statement near the top alongside existing imports (so the
CVDisplayLink, CVDisplayLinkOutputCallback, CVDisplayLinkStart/Stop and teardown
code in the display link setup and teardown functions will compile).
| let distFromCenter = abs(relativeX) / (panelWidth + panelGap) | ||
| let zOffset = -abs(distFromCenter) * perspectiveZ | ||
| let scale = max(0.75, 1.0 - distFromCenter * 0.08) | ||
|
|
||
| // Subtle rotation for panels going off-screen | ||
| let rotation = -relativeX / (bounds.width * 1.5) * edgeRotation | ||
|
|
||
| // Opacity falloff | ||
| let opacity = max(0.3, 1.0 - distFromCenter * 0.25) | ||
|
|
||
| var transform = CATransform3DIdentity | ||
| transform = CATransform3DTranslate(transform, screenX + panelWidth / 2 - centerX, 0, zOffset) | ||
| transform = CATransform3DRotate(transform, rotation, 0, 1, 0) | ||
| transform = CATransform3DScale(transform, scale, scale, 1) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n scripts/demo-paperwm.swift | grep -A 30 -B 5 "perspectiveZ"Repository: manaflow-ai/cmux
Length of output: 3568
Remove the unary - operator from zOffset calculation to correctly push side panels away from the camera.
perspectiveZ is -120 (negative). The current code multiplies it by -abs(distFromCenter), resulting in a positive zOffset that pushes panels toward the camera, contradicting the intended depth recession. The fix makes zOffset negative so panels correctly recede.
Suggested fix
- let zOffset = -abs(distFromCenter) * perspectiveZ
+ let zOffset = abs(distFromCenter) * perspectiveZ📝 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.
| let distFromCenter = abs(relativeX) / (panelWidth + panelGap) | |
| let zOffset = -abs(distFromCenter) * perspectiveZ | |
| let scale = max(0.75, 1.0 - distFromCenter * 0.08) | |
| // Subtle rotation for panels going off-screen | |
| let rotation = -relativeX / (bounds.width * 1.5) * edgeRotation | |
| // Opacity falloff | |
| let opacity = max(0.3, 1.0 - distFromCenter * 0.25) | |
| var transform = CATransform3DIdentity | |
| transform = CATransform3DTranslate(transform, screenX + panelWidth / 2 - centerX, 0, zOffset) | |
| transform = CATransform3DRotate(transform, rotation, 0, 1, 0) | |
| transform = CATransform3DScale(transform, scale, scale, 1) | |
| let distFromCenter = abs(relativeX) / (panelWidth + panelGap) | |
| let zOffset = abs(distFromCenter) * perspectiveZ | |
| let scale = max(0.75, 1.0 - distFromCenter * 0.08) | |
| // Subtle rotation for panels going off-screen | |
| let rotation = -relativeX / (bounds.width * 1.5) * edgeRotation | |
| // Opacity falloff | |
| let opacity = max(0.3, 1.0 - distFromCenter * 0.25) | |
| var transform = CATransform3DIdentity | |
| transform = CATransform3DTranslate(transform, screenX + panelWidth / 2 - centerX, 0, zOffset) | |
| transform = CATransform3DRotate(transform, rotation, 0, 1, 0) | |
| transform = CATransform3DScale(transform, scale, scale, 1) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/demo-paperwm.swift` around lines 213 - 226, The z ordering is
inverted because zOffset is computed with a leading negative: change the zOffset
calculation in the transform block (variable zOffset, which currently uses
-abs(distFromCenter) * perspectiveZ) so it does not negate the product; compute
zOffset as abs(distFromCenter) * perspectiveZ (or otherwise remove the unary '-'
) so that given perspectiveZ is negative (−120) side panels receive a negative
zOffset and correctly recede from the camera.
| let app = NSApplication.shared | ||
| app.setActivationPolicy(.regular) | ||
|
|
||
| let w = NSWindow(contentRect: NSRect(x: 400, y: 400, width: 600, height: 400), | ||
| styleMask: [.titled, .closable, .resizable], backing: .buffered, defer: false) | ||
| w.title = "Tab Bar Test" | ||
|
|
||
| let content = NSView(frame: w.contentView!.bounds) | ||
| content.autoresizingMask = [.width, .height] | ||
| content.wantsLayer = true | ||
| content.layer?.backgroundColor = CGColor(gray: 0.1, alpha: 1) | ||
|
|
||
| let tabBar = TestTabBar(frame: NSRect(x: 0, y: content.bounds.height - 30, width: content.bounds.width, height: 30)) | ||
| tabBar.autoresizingMask = [.width, .minYMargin] | ||
| content.addSubview(tabBar) | ||
|
|
||
| w.contentView = content | ||
| w.makeKeyAndOrderFront(nil) | ||
| app.activate(ignoringOtherApps: true) | ||
|
|
||
| print("Tab bar test running. Click and drag tabs to reorder.") | ||
| app.run() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the test-tabbar.swift file
fd test-tabbar.swift --type fRepository: manaflow-ai/cmux
Length of output: 85
🏁 Script executed:
# Check demo-paperwm.swift for comparison
fd demo-paperwm.swift --type fRepository: manaflow-ai/cmux
Length of output: 86
🏁 Script executed:
cat -n scripts/test-tabbar.swiftRepository: manaflow-ai/cmux
Length of output: 5841
🏁 Script executed:
cat -n scripts/demo-paperwm.swift | head -150Repository: manaflow-ai/cmux
Length of output: 7556
🏁 Script executed:
cat -n scripts/demo-paperwm.swift | grep -A 10 -B 5 "ApplicationDelegate\|applicationShouldTerminateAfterLastWindowClosed\|NSApplication.shared"Repository: manaflow-ai/cmux
Length of output: 1033
Exit the helper app when the last window closes.
Closing the window currently leaves this script running in the Dock because nothing opts into last-window termination. scripts/demo-paperwm.swift already handles that, so this helper should match.
💡 Suggested fix
+final class AppDelegate: NSObject, NSApplicationDelegate {
+ func applicationShouldTerminateAfterLastWindowClosed(_ sender: NSApplication) -> Bool {
+ true
+ }
+}
+
let app = NSApplication.shared
+let delegate = AppDelegate()
+app.delegate = delegate
app.setActivationPolicy(.regular)📝 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.
| let app = NSApplication.shared | |
| app.setActivationPolicy(.regular) | |
| let w = NSWindow(contentRect: NSRect(x: 400, y: 400, width: 600, height: 400), | |
| styleMask: [.titled, .closable, .resizable], backing: .buffered, defer: false) | |
| w.title = "Tab Bar Test" | |
| let content = NSView(frame: w.contentView!.bounds) | |
| content.autoresizingMask = [.width, .height] | |
| content.wantsLayer = true | |
| content.layer?.backgroundColor = CGColor(gray: 0.1, alpha: 1) | |
| let tabBar = TestTabBar(frame: NSRect(x: 0, y: content.bounds.height - 30, width: content.bounds.width, height: 30)) | |
| tabBar.autoresizingMask = [.width, .minYMargin] | |
| content.addSubview(tabBar) | |
| w.contentView = content | |
| w.makeKeyAndOrderFront(nil) | |
| app.activate(ignoringOtherApps: true) | |
| print("Tab bar test running. Click and drag tabs to reorder.") | |
| app.run() | |
| final class AppDelegate: NSObject, NSApplicationDelegate { | |
| func applicationShouldTerminateAfterLastWindowClosed(_ sender: NSApplication) -> Bool { | |
| true | |
| } | |
| } | |
| let app = NSApplication.shared | |
| let delegate = AppDelegate() | |
| app.delegate = delegate | |
| app.setActivationPolicy(.regular) | |
| let w = NSWindow(contentRect: NSRect(x: 400, y: 400, width: 600, height: 400), | |
| styleMask: [.titled, .closable, .resizable], backing: .buffered, defer: false) | |
| w.title = "Tab Bar Test" | |
| let content = NSView(frame: w.contentView!.bounds) | |
| content.autoresizingMask = [.width, .height] | |
| content.wantsLayer = true | |
| content.layer?.backgroundColor = CGColor(gray: 0.1, alpha: 1) | |
| let tabBar = TestTabBar(frame: NSRect(x: 0, y: content.bounds.height - 30, width: content.bounds.width, height: 30)) | |
| tabBar.autoresizingMask = [.width, .minYMargin] | |
| content.addSubview(tabBar) | |
| w.contentView = content | |
| w.makeKeyAndOrderFront(nil) | |
| app.activate(ignoringOtherApps: true) | |
| print("Tab bar test running. Click and drag tabs to reorder.") | |
| app.run() |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/test-tabbar.swift` around lines 106 - 127, Add an
NSApplicationDelegate that returns true from
applicationShouldTerminateAfterLastWindowClosed and assign it to the existing
app (let app = NSApplication.shared) so the helper quits when the last window
closes; specifically, create a small AppDelegate class implementing
applicationShouldTerminateAfterLastWindowClosed(_:)->Bool { return true } and
set app.delegate = AppDelegate() before calling app.run(), leaving TestTabBar
and the window setup unchanged.
| // Skip all shortcut handling when the niri canvas window is key. | ||
| // It handles its own shortcuts via sendEvent/performKeyEquivalent. | ||
| if event.window is NiriCanvasWindow { return false } | ||
|
|
There was a problem hiding this comment.
Resolve the canvas window from windowNumber/NSApp.keyWindow, not just event.window.
This guard misses the key-equivalent paths where AppKit delivers a key event with event.window == nil. In those cases the canvas is still key, but handleCustomShortcut keeps processing global app shortcuts against the last main window.
🛠️ Suggested fix
- if event.window is NiriCanvasWindow { return false }
+ let shortcutWindow = event.window
+ ?? (event.windowNumber > 0 ? NSApp.window(withWindowNumber: event.windowNumber) : nil)
+ ?? NSApp.keyWindow
+ if shortcutWindow is NiriCanvasWindow { return false }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/AppDelegate.swift` around lines 9087 - 9090, Update the
shortcut-handling guard to resolve the actual window using the event's
windowNumber or NSApp.keyWindow when event.window is nil (instead of only
checking event.window). In the method that contains the current check (the block
using "if event.window is NiriCanvasWindow { return false }" inside
handleCustomShortcut / AppDelegate), obtain a resolvedWindow from event.window
?? NSApp.window(withWindowNumber: event.windowNumber) ?? NSApp.keyWindow and use
that to test "is NiriCanvasWindow" so key-equivalent events delivered with
event.window == nil are correctly treated as coming from the canvas.
| } | ||
|
|
||
| // Title | ||
| let title = tab.title.isEmpty ? "Shell" : String(tab.title.prefix(20)) |
There was a problem hiding this comment.
Localize the visible canvas labels.
"Shell" and "Terminal Canvas" are user-visible UI strings, so they should come from String(localized: ..., defaultValue: ...) keys in Resources/Localizable.xcstrings, not bare English literals.
As per coding guidelines, "All user-facing strings must be localized using String(localized: "key.name", defaultValue: "English text") for every string shown in the UI."
Also applies to: 199-199, 367-370, 1034-1034
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/NiriCanvasView.swift` at line 127, Replace hard-coded UI strings with
localized lookups: in NiriCanvasView where you set let title = tab.title.isEmpty
? "Shell" : String(tab.title.prefix(20)) and at the other noted locations (the
"Terminal Canvas" uses and other literal strings around lines referenced),
change the literals to use String(localized: "key.name", defaultValue: "Shell")
or appropriate keys (e.g., "terminal.canvas.title") and add corresponding
entries in Resources/Localizable.xcstrings; ensure you use descriptive key
names, keep the same default English text in defaultValue, and update all
occurrences mentioned ("Shell", "Terminal Canvas", and the other two literals)
to follow this pattern so user-facing strings are localized.
| func setSurfaces(_ surfaces: [TerminalSurface]) { | ||
| for p in panels { p.containerView.removeFromSuperview() } | ||
| panels = surfaces.map { makePanel(with: [$0]) } |
There was a problem hiding this comment.
Discarded TerminalSurfaces need explicit teardown.
These paths let TerminalSurface instances fall out of panels without calling teardownSurface(). That bypasses the lifecycle transition used in Sources/GhosttyTerminalView.swift:3259-3290 / Sources/GhosttyTerminalView.swift:4080-4125 to seal the portal before the C surface is freed, so cleanup is left to deinit/request-close timing instead of the normal shutdown path.
Also applies to: 832-843, 961-965
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/NiriCanvasView.swift` around lines 238 - 240, The setSurfaces
implementation is dropping TerminalSurface instances by replacing panels without
calling their teardown path; update setSurfaces (and the similar code at the
other occurrences) to iterate the existing panels' TerminalSurface(s) and call
teardownSurface() for each discarded surface before removing container views,
then update panels = surfaces.map { makePanel(with: [$0]) }; ensure you
reference the existing panels array, the makePanel(with:) helper, and the
teardownSurface() method so the portal sealing lifecycle used in
GhosttyTerminalView's teardown is executed for each surface being replaced.
| case .inTabBar(let targetLive, let insertIdx): | ||
| let live = liveIndices | ||
| guard targetLive < live.count else { break } | ||
| let targetPi = live[targetLive].panel | ||
|
|
||
| if targetPi == sourcePi { | ||
| reorderTab(from: sourceTab, | ||
| to: min(insertIdx, panels[sourcePi].tabs.count - 1), | ||
| inPanel: ObjectIdentifier(panels[sourcePi].containerView)) |
There was a problem hiding this comment.
Rightward same-panel drops land one slot too far.
insertIdx here is an insertion boundary in the pre-move array. When the source tab moves right within the same panel, removing it first shifts the target left by one, so this path currently drops after the intended slot.
💡 Proposed fix
if targetPi == sourcePi {
+ let adjustedInsert = insertIdx > sourceTab ? insertIdx - 1 : insertIdx
reorderTab(from: sourceTab,
- to: min(insertIdx, panels[sourcePi].tabs.count - 1),
+ to: min(adjustedInsert, panels[sourcePi].tabs.count - 1),
inPanel: ObjectIdentifier(panels[sourcePi].containerView))
} else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/NiriCanvasView.swift` around lines 503 - 511, When handling case
.inTabBar you must adjust insertIdx for same-panel rightward moves because
removing the source first shifts targets left; compute an adjusted index (e.g.
let adjustedInsert = insertIdx - (sourceTab < insertIdx ? 1 : 0)), clamp it
against panels[sourcePi].tabs.count - 1, and pass adjustedInsert to
reorderTab(from:to:inPanel:) instead of the raw insertIdx; keep the rest of the
liveIndices/targetPi/sourcePi logic intact.
| // Remove from source | ||
| panels[srcPi].activeSurface?.hostedView.removeFromSuperview() | ||
| panels[srcPi].tabs.remove(at: srcTab) | ||
| if panels[srcPi].tabs.isEmpty { | ||
| panels[srcPi].closing = true | ||
| } else { | ||
| panels[srcPi].activeTab = min(panels[srcPi].activeTab, panels[srcPi].tabs.count - 1) | ||
| panels[srcPi].syncTabBar() | ||
| // Show new active in source | ||
| if let s = panels[srcPi].activeSurface { | ||
| s.hostedView.removeFromSuperview() | ||
| panels[srcPi].containerView.addSubview(s.hostedView) | ||
| } | ||
| } |
There was a problem hiding this comment.
Keep the same active surface when removing a tab to its left.
Both source-panel removal paths recompute activeTab with min(...) only. If a background tab is dragged out from the left of the active tab, the active index should shift left by one to keep the same surface selected; today it silently jumps to the next tab instead.
💡 Proposed fix
if panels[srcPi].tabs.isEmpty {
panels[srcPi].closing = true
} else {
- panels[srcPi].activeTab = min(panels[srcPi].activeTab, panels[srcPi].tabs.count - 1)
+ if srcTab < panels[srcPi].activeTab {
+ panels[srcPi].activeTab -= 1
+ } else {
+ panels[srcPi].activeTab = min(panels[srcPi].activeTab, panels[srcPi].tabs.count - 1)
+ }
panels[srcPi].syncTabBar()Apply the same adjustment in both helpers.
Also applies to: 582-594
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/NiriCanvasView.swift` around lines 547 - 560, When removing a tab
from a panel (the block manipulating panels[srcPi].tabs,
panels[srcPi].activeTab, panels[srcPi].activeSurface, and calling
panels[srcPi].syncTabBar), preserve the currently displayed surface if the
removed tab is left of the current activeTab: instead of always setting
activeTab = min(...), if srcTab < panels[srcPi].activeTab decrement
panels[srcPi].activeTab by 1; otherwise set it to min(...). Apply the identical
change in the other helper that performs tab removal (the other code path that
also updates panels[...].tabs, activeTab, activeSurface and syncTabBar) so the
activeSurface does not silently jump when a left-side tab is dragged out.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 10 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/NiriCanvasView.swift">
<violation number="1" location="Sources/NiriCanvasView.swift:931">
P1: Avoid adding an app-level CVDisplayLink; the repo’s Ghostty performance guidance warns this can introduce typing latency. Drive animations via Ghostty wakeups or a scoped timer only while animating instead of a global display link.</violation>
</file>
<file name="scripts/demo-paperwm.swift">
<violation number="1" location="scripts/demo-paperwm.swift:294">
P1: Passing `self` as an unretained pointer to `CVDisplayLink` and stopping it in `deinit` creates a use-after-free race condition. The display link callback can fire on a background thread while or after `self` is deallocated.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| return kCVReturnSuccess | ||
| } | ||
|
|
||
| CVDisplayLinkSetOutputCallback(displayLink, callback, |
There was a problem hiding this comment.
P1: Passing self as an unretained pointer to CVDisplayLink and stopping it in deinit creates a use-after-free race condition. The display link callback can fire on a background thread while or after self is deallocated.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/demo-paperwm.swift, line 294:
<comment>Passing `self` as an unretained pointer to `CVDisplayLink` and stopping it in `deinit` creates a use-after-free race condition. The display link callback can fire on a background thread while or after `self` is deallocated.</comment>
<file context>
@@ -0,0 +1,368 @@
+ return kCVReturnSuccess
+ }
+
+ CVDisplayLinkSetOutputCallback(displayLink, callback,
+ Unmanaged.passUnretained(self).toOpaque())
+ CVDisplayLinkStart(displayLink)
</file context>
…eys, instant close Restores ~840 lines of uncommitted work that was accidentally reverted. Tab titles: Listen for ghosttyDidSetTitle notification (registered in setSurfaces, not setup, to avoid main window crash). Display last path component in tab bar. Tab bar colors: Use GhosttyApp.shared.defaultBackgroundColor for selected tab bg. Bar bg = terminal bg darkened 15%. Remove bottom border. Reduce tab bar height from 30px to 24px. Scroll fixes: Use fractional visibility (99% threshold) instead of pixel tolerance. Check against scrollOffset (current) not targetOffset. Add 100px peek when scrolling to reveal. Subtract one panelGap in pw() to fix gap overflow. Ctrl+Tab/Ctrl+Shift+Tab: Cycle tabs in focused panel. Handle in NiriCanvasWindow.performKeyEquivalent (keyCode 48) before ghostty can swallow it. Ctrl+9: Jump to last tab in focused panel. Instant column close: Remove panel from array immediately (no close animation). Absorb width into split partner via splitPartnerId/ preSplitWidth tracking. Focus neighbor with least scroll cost. Less vertical padding: viewH - 4 instead of viewH - 20. No layout() override: Detect window resize in tick() by checking bounds.width change, avoiding Auto Layout constraint loops. Reduce peekWidth from 60px to 20px. Min tab width sized to content.
…sition logging - Fix startup crash: defer startDisplayLink to setSurfaces, guard tick() for empty panels - Remove NSSetUncaughtExceptionHandler (conflicts with Sentry) - Tab bar: per-tab widths based on content, scrollable when overflowing, scroll consumed - Font: use .AppleSystemUIFont (SF Pro) matching bonsplit tabs - Fix pw() to subtract panelGap (fixes position shift on split/unsplit) - Add Cmd+D splitRight with gap-aware half-width calculation - Add position logging for debugging split/close position drift
There was a problem hiding this comment.
♻️ Duplicate comments (5)
Sources/NiriCanvasView.swift (5)
636-639:⚠️ Potential issue | 🟠 MajorAdjust same-panel rightward drop index before reorder.
When source and destination are the same panel, insertion index is computed pre-removal. Rightward moves currently land one slot too far.
💡 Proposed fix
if targetPi == sourcePi { + let adjustedInsert = insertIdx > sourceTab ? insertIdx - 1 : insertIdx reorderTab(from: sourceTab, - to: min(insertIdx, panels[sourcePi].tabs.count - 1), + to: min(adjustedInsert, panels[sourcePi].tabs.count - 1), inPanel: ObjectIdentifier(panels[sourcePi].containerView)) } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/NiriCanvasView.swift` around lines 636 - 639, When sourcePi == targetPi the insertIdx is computed before removing sourceTab so rightward moves land one slot too far; detect this case in the same-panel branch and if insertIdx is greater than the source tab's current index decrement insertIdx by 1 before calling reorderTab (ensure the adjusted index is clamped within 0...panels[sourcePi].tabs.count-1). Update the call site that uses sourceTab, insertIdx, panels[sourcePi].tabs.count and ObjectIdentifier(panels[sourcePi].containerView) so reorderTab receives the corrected destination index.
675-683:⚠️ Potential issue | 🟠 MajorPreserve active surface identity when removing a tab to the left.
Both source-panel removal paths clamp with
min(...)only. If a left-side background tab is removed,activeTabshould decrement to keep the same active surface.💡 Proposed fix
- panels[srcPi].activeTab = min(panels[srcPi].activeTab, panels[srcPi].tabs.count - 1) + if srcTab < panels[srcPi].activeTab { + panels[srcPi].activeTab -= 1 + } else { + panels[srcPi].activeTab = min(panels[srcPi].activeTab, panels[srcPi].tabs.count - 1) + }Also applies to: 710-717
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/NiriCanvasView.swift` around lines 675 - 683, When removing a tab from panels[srcPi] we currently only clamp activeTab with min(...), which fails to preserve the same active surface when a left-side tab (index < activeTab) is removed; update the logic in the removal paths (the block handling panels[srcPi].tabs.remove(at: srcTab) and the duplicate block around the other removal, e.g. the code invoking panels[srcPi].syncTabBar()) to adjust activeTab like: if srcTab < panels[srcPi].activeTab then decrement panels[srcPi].activeTab by 1, otherwise set panels[srcPi].activeTab = min(panels[srcPi].activeTab, panels[srcPi].tabs.count - 1); ensure you guard against negative values when tabs becomes empty and keep calling panels[srcPi].syncTabBar() after updating activeTab.
47-47:⚠️ Potential issue | 🟠 MajorLocalize visible canvas labels and fallback tab titles.
"Shell"and"Terminal Canvas"are user-visible strings and should not be hard-coded in Swift UI/event-visible surfaces.💡 Proposed fix
- let title = tab.title.isEmpty ? "Shell" : String(tab.title.prefix(20)) + let title = tab.title.isEmpty + ? String(localized: "niri.canvas.tab.shell", defaultValue: "Shell") + : String(tab.title.prefix(20)) - let title = tabTitles[s.id] ?? "Shell" + let title = tabTitles[s.id] ?? String(localized: "niri.canvas.tab.shell", defaultValue: "Shell") - ? (panels[srcPi].tabBar.tabs[tabIndex].title.isEmpty ? "Shell" : panels[srcPi].tabBar.tabs[tabIndex].title) - : "Shell" + ? (panels[srcPi].tabBar.tabs[tabIndex].title.isEmpty + ? String(localized: "niri.canvas.tab.shell", defaultValue: "Shell") + : panels[srcPi].tabBar.tabs[tabIndex].title) + : String(localized: "niri.canvas.tab.shell", defaultValue: "Shell") - win.title = "Terminal Canvas" + win.title = String(localized: "niri.canvas.window.title", defaultValue: "Terminal Canvas")Also add these keys to
Resources/Localizable.xcstrings.As per coding guidelines, "All user-facing strings must be localized using
String(localized: "key.name", defaultValue: "English text")for every string shown in the UI."Also applies to: 193-193, 268-268, 497-499, 1315-1315
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/NiriCanvasView.swift` at line 47, Replace hard-coded user-facing strings in NiriCanvasView.swift (e.g., the fallback in let title = tab.title.isEmpty ? "Shell" : String(tab.title.prefix(20)) and other occurrences like "Terminal Canvas") with localized lookups using String(localized: "key.name", defaultValue: "English text"); update each unique key to reflect context (e.g., "canvas.tab.fallback.shell", "canvas.title.terminal") and add those keys with the English defaults to Resources/Localizable.xcstrings so all UI-visible strings use String(localized:..., defaultValue:...) instead of raw literals.
1296-1298:⚠️ Potential issue | 🟠 MajorDon’t swallow unhandled Command key events at window level.
The unconditional
if cmd { return }consumes unhandledCmdcombinations and can block terminal-owned key paths.💡 Proposed fix
if (cmd || ctrl), let canvas = canvasView, canvas.performKeyEquivalent(with: event) { return } - if cmd { return } // consume unhandled Cmd combos to prevent cmux crash + // Let unhandled command equivalents continue through normal responder routing.Based on learnings,
GhosttyNSView.performKeyEquivalent(with:)should receive unbound Command combinations rather than having them swallowed upstream.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/NiriCanvasView.swift` around lines 1296 - 1298, Remove the unconditional swallowing of Cmd events so unhandled Command-key combos can propagate to the terminal view; in NiriCanvasView.swift, delete or avoid the `if cmd { return }` branch after the existing `if (cmd || ctrl), let canvas = canvasView, canvas.performKeyEquivalent(with: event) { return }` check, ensuring you only return when `canvas.performKeyEquivalent(with:)` actually handled the event and otherwise let the event continue so `GhosttyNSView.performKeyEquivalent(with:)` can receive unbound Command combinations.
315-317:⚠️ Potential issue | 🟠 MajorTeardown discarded
TerminalSurfaces before replacing panels.Line 315–317 removes old panel views and replaces
panels, but dropped surfaces are not explicitly torn down first.💡 Proposed fix
func setSurfaces(_ surfaces: [TerminalSurface]) { - for p in panels { p.containerView.removeFromSuperview() } + let incomingIds = Set(surfaces.map(\.id)) + for p in panels { + for tab in p.tabs where !incomingIds.contains(tab.id) { + tab.teardownSurface() + } + p.containerView.removeFromSuperview() + } panels = surfaces.map { makePanel(with: [$0]) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/NiriCanvasView.swift` around lines 315 - 317, The current setSurfaces(_:) replaces panel views but never tears down the discarded TerminalSurface instances; before removing old panels call the teardown method on each surface associated with the existing panels (e.g. iterate panels.flatMap { $0.surfaces } and invoke their teardown/tearDown/close method), then remove their containerView and finally replace panels using makePanel(with:). Ensure you reference the existing panels and makePanel(with:) when implementing the teardown loop.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@Sources/NiriCanvasView.swift`:
- Around line 636-639: When sourcePi == targetPi the insertIdx is computed
before removing sourceTab so rightward moves land one slot too far; detect this
case in the same-panel branch and if insertIdx is greater than the source tab's
current index decrement insertIdx by 1 before calling reorderTab (ensure the
adjusted index is clamped within 0...panels[sourcePi].tabs.count-1). Update the
call site that uses sourceTab, insertIdx, panels[sourcePi].tabs.count and
ObjectIdentifier(panels[sourcePi].containerView) so reorderTab receives the
corrected destination index.
- Around line 675-683: When removing a tab from panels[srcPi] we currently only
clamp activeTab with min(...), which fails to preserve the same active surface
when a left-side tab (index < activeTab) is removed; update the logic in the
removal paths (the block handling panels[srcPi].tabs.remove(at: srcTab) and the
duplicate block around the other removal, e.g. the code invoking
panels[srcPi].syncTabBar()) to adjust activeTab like: if srcTab <
panels[srcPi].activeTab then decrement panels[srcPi].activeTab by 1, otherwise
set panels[srcPi].activeTab = min(panels[srcPi].activeTab,
panels[srcPi].tabs.count - 1); ensure you guard against negative values when
tabs becomes empty and keep calling panels[srcPi].syncTabBar() after updating
activeTab.
- Line 47: Replace hard-coded user-facing strings in NiriCanvasView.swift (e.g.,
the fallback in let title = tab.title.isEmpty ? "Shell" :
String(tab.title.prefix(20)) and other occurrences like "Terminal Canvas") with
localized lookups using String(localized: "key.name", defaultValue: "English
text"); update each unique key to reflect context (e.g.,
"canvas.tab.fallback.shell", "canvas.title.terminal") and add those keys with
the English defaults to Resources/Localizable.xcstrings so all UI-visible
strings use String(localized:..., defaultValue:...) instead of raw literals.
- Around line 1296-1298: Remove the unconditional swallowing of Cmd events so
unhandled Command-key combos can propagate to the terminal view; in
NiriCanvasView.swift, delete or avoid the `if cmd { return }` branch after the
existing `if (cmd || ctrl), let canvas = canvasView,
canvas.performKeyEquivalent(with: event) { return }` check, ensuring you only
return when `canvas.performKeyEquivalent(with:)` actually handled the event and
otherwise let the event continue so `GhosttyNSView.performKeyEquivalent(with:)`
can receive unbound Command combinations.
- Around line 315-317: The current setSurfaces(_:) replaces panel views but
never tears down the discarded TerminalSurface instances; before removing old
panels call the teardown method on each surface associated with the existing
panels (e.g. iterate panels.flatMap { $0.surfaces } and invoke their
teardown/tearDown/close method), then remove their containerView and finally
replace panels using makePanel(with:). Ensure you reference the existing panels
and makePanel(with:) when implementing the teardown loop.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c08b07a1-e939-4898-bda2-d31c848396da
📒 Files selected for processing (1)
Sources/NiriCanvasView.swift
There was a problem hiding this comment.
2 issues found across 1 file (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/NiriCanvasView.swift">
<violation number="1" location="Sources/NiriCanvasView.swift:160">
P2: The draw-time auto-scroll forces the selected tab into view every frame, which overrides manual tab-bar scrolling and can make overflow tabs unreachable.</violation>
<violation number="2" location="Sources/NiriCanvasView.swift:268">
P2: Tab titles are keyed per panel, but tab move paths don’t transfer the title mapping, so dragging a tab to another panel resets its label to "Shell".</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| mutating func syncTabBar() { | ||
| tabBar.tabs = tabs.map { s in | ||
| let title = tabTitles[s.id] ?? "Shell" |
There was a problem hiding this comment.
P2: Tab titles are keyed per panel, but tab move paths don’t transfer the title mapping, so dragging a tab to another panel resets its label to "Shell".
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/NiriCanvasView.swift, line 268:
<comment>Tab titles are keyed per panel, but tab move paths don’t transfer the title mapping, so dragging a tab to another panel resets its label to "Shell".</comment>
<file context>
@@ -189,14 +249,25 @@ final class NiriCanvasView: NSView {
mutating func syncTabBar() {
- tabBar.tabs = tabs.map { NiriTabBarView.Tab(id: $0.id, title: "Shell") }
+ tabBar.tabs = tabs.map { s in
+ let title = tabTitles[s.id] ?? "Shell"
+ return NiriTabBarView.Tab(id: s.id, title: title)
+ }
</file context>
Blue overlay shows drop zone during drag: - Left/right 25% edge: creates new column (halves target width) - Top/bottom 25% edge: creates new column (same width, for now) - Center: adds tab to existing panel
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
Sources/NiriCanvasView.swift (5)
731-734:⚠️ Potential issue | 🟠 MajorSame-panel rightward drag is still off by one.
Line 733 uses pre-removal insertion index directly; rightward moves in the same panel land one slot too far.
Proposed fix
if targetPi == sourcePi { + let adjustedInsert = insertIdx > sourceTab ? insertIdx - 1 : insertIdx reorderTab(from: sourceTab, - to: min(insertIdx, panels[sourcePi].tabs.count - 1), + to: min(adjustedInsert, panels[sourcePi].tabs.count - 1), inPanel: ObjectIdentifier(panels[sourcePi].containerView)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/NiriCanvasView.swift` around lines 731 - 734, The same-panel rightward drag lands one slot too far because you use the pre-removal insertIdx directly; in the if targetPi == sourcePi branch adjust the insertion position when moving right by checking if insertIdx > sourceTab and, if so, pass min(insertIdx - 1, panels[sourcePi].tabs.count - 1) into reorderTab (instead of min(insertIdx, ...)); update the call site that currently uses reorderTab(from: sourceTab, to: min(insertIdx, panels[sourcePi].tabs.count - 1), inPanel: ObjectIdentifier(panels[sourcePi].containerView)) to subtract one for rightward same-panel moves.
1449-1459:⚠️ Potential issue | 🟠 MajorWindow-level key handling is still swallowing terminal input.
Line 1449 binds
Ctrl+D(terminal EOF) to close-tab, and Line 1459 consumes any unhandled Command combo, preventing focused Ghostty from receiving unbound shortcuts.Based on learnings, `GhosttyNSView.performKeyEquivalent(with:)` must forward unbound Command combinations to terminal handling rather than being swallowed by higher-level shortcut code.Proposed fix
- if ctrl && !cmd && !opt && (ch == "d" || event.characters == "\u{04}") { - canvasView?.handleCtrlD(); return - } if ctrl && !cmd && !opt && ch == "9" { canvasView?.switchToLastTabPublic(); return } if ctrl && !cmd && !opt, let n = Int(ch), n >= 1, n <= 8 { canvasView?.switchToTabPublic(n - 1); return } if (cmd || ctrl), let canvas = canvasView, canvas.performKeyEquivalent(with: event) { return } - if cmd { return } // consume unhandled Cmd combos to prevent cmux crash🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/NiriCanvasView.swift` around lines 1449 - 1459, The window-level key handler in NiriCanvasView is swallowing terminal input (e.g., Ctrl+D EOF and unbound Cmd combos) instead of letting focused GhosttyNSView handle them; update the logic so Ctrl+D and numeric Ctrl shortcuts only act when the canvas/tab UI is the active responder (e.g., not when the terminal/Ghostty is first responder), and do not unconditionally consume unhandled Command combinations — call canvas.performKeyEquivalent(with:) (or GhosttyNSView.performKeyEquivalent) and if it returns false, allow the responder chain to receive the event rather than returning/consuming it; adjust checks around handleCtrlD(), switchToLastTabPublic(), switchToTabPublic(_:) and the final cmd-consume branch so focused GhosttyNSView gets unbound shortcuts and terminal EOF.
315-318:⚠️ Potential issue | 🟠 MajorDiscarded
TerminalSurfaces still skip explicit teardown insetSurfaces.Replacing
panelsdirectly can drop live surfaces without running the expected shutdown path.Proposed fix
func setSurfaces(_ surfaces: [TerminalSurface]) { - for p in panels { p.containerView.removeFromSuperview() } + for p in panels { + for surface in p.tabs { + surface.teardownSurface() + } + p.containerView.removeFromSuperview() + } panels = surfaces.map { makePanel(with: [$0]) } for p in panels { addSubview(p.containerView) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/NiriCanvasView.swift` around lines 315 - 318, setSurfaces currently replaces panels wholesale which can drop live TerminalSurface instances without running their shutdown/teardown path; before you assign panels = surfaces.map { ... } capture the old panels, for each old panel call its explicit teardown/cleanup method on the panel or the underlying TerminalSurface (the codepath that shuts down connections/resources), then removeFromSuperview(), and only after all old panels are properly torn down create new panels via makePanel(with:) and add their containerView; ensure you reference panels, setSurfaces(_:) and makePanel(with:) when making the change.
47-47:⚠️ Potential issue | 🟠 MajorLocalize visible canvas strings (
Shell,Terminal Canvas).These are UI-visible and should not be hardcoded literals.
As per coding guidelines, "All user-facing strings must be localized using `String(localized: "key.name", defaultValue: "English text")` for every string shown in the UI."Proposed fix
- let title = tab.title.isEmpty ? "Shell" : String(tab.title.prefix(20)) + let title = tab.title.isEmpty + ? String(localized: "niri.canvas.tab.untitled", defaultValue: "Shell") + : String(tab.title.prefix(20)) - let title = tabTitles[s.id] ?? "Shell" + let title = tabTitles[s.id] + ?? String(localized: "niri.canvas.tab.untitled", defaultValue: "Shell") - ? (panels[srcPi].tabBar.tabs[tabIndex].title.isEmpty ? "Shell" : panels[srcPi].tabBar.tabs[tabIndex].title) - : "Shell" + ? (panels[srcPi].tabBar.tabs[tabIndex].title.isEmpty + ? String(localized: "niri.canvas.tab.untitled", defaultValue: "Shell") + : panels[srcPi].tabBar.tabs[tabIndex].title) + : String(localized: "niri.canvas.tab.untitled", defaultValue: "Shell") - win.title = "Terminal Canvas" + win.title = String(localized: "niri.canvas.window.title", defaultValue: "Terminal Canvas")Also applies to: 193-193, 268-269, 507-508, 1477-1477
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/NiriCanvasView.swift` at line 47, Replace hardcoded UI strings with localized variants: where you build the tab title (the assignment using tab.title and the literal "Shell") use String(localized: "niri.canvas.shell", defaultValue: "Shell") and apply String(localized: "niri.canvas.terminalCanvas", defaultValue: "Terminal Canvas") for any visible "Terminal Canvas" labels; find other hardcoded UI strings (the other occurrences of "Shell" and "Terminal Canvas" noted in the review) and replace them similarly using String(localized: ..., defaultValue: ...), keeping the original literal as the defaultValue and using clear localization keys (e.g., "niri.canvas.shell", "niri.canvas.terminalCanvas") so all user-facing text in NiriCanvasView is localized.
768-769:⚠️ Potential issue | 🟠 MajorPreserve active surface when removing a tab to its left.
These paths only clamp with
min(...). If a tab left of the active tab is removed, active index must shift left by one to keep the same surface selected.Proposed fix pattern (apply at all listed ranges)
- panels[srcPi].activeTab = min(panels[srcPi].activeTab, panels[srcPi].tabs.count - 1) + if srcTab < panels[srcPi].activeTab { + panels[srcPi].activeTab -= 1 + } else { + panels[srcPi].activeTab = min(panels[srcPi].activeTab, panels[srcPi].tabs.count - 1) + }Also applies to: 843-844, 878-879
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/NiriCanvasView.swift` around lines 768 - 769, When removing a tab, adjust panels[sourcePi].activeTab so it shifts left if the removed tab was to the left of the active one: compute newActive = panels[sourcePi].activeTab - (removedIndex < panels[sourcePi].activeTab ? 1 : 0), then clamp to valid range e.g. panels[sourcePi].activeTab = max(0, min(newActive, panels[sourcePi].tabs.count - 1)); finally call panels[sourcePi].syncTabBar(). Use the actual removed tab index variable from the surrounding code in place of removedIndex; apply this change where panels[sourcePi].activeTab is currently only clamped with min(...).
🧹 Nitpick comments (1)
Sources/NiriCanvasView.swift (1)
386-387: Usedloginstead ofNSLogfor debug event logging consistency.This file is DEBUG-only and these are debug-event traces; keep them on the unified debug event log path.
Based on learnings, all DEBUG key/mouse/focus/split/tab debug events should be routed via `dlog(...)`.Proposed refactor
- NSLog("niri.hitTest -> TAB BAR local=\(localPt) tbFrame=\(tb.frame)") + dlog("niri.hitTest -> TAB BAR local=\(localPt) tbFrame=\(tb.frame)") - NSLog("niri.hitTest MISS near tabBar local=\(localPt) tbFrame=\(tabBar?.frame ?? .zero) result=\(type(of: result).self)") + dlog("niri.hitTest MISS near tabBar local=\(localPt) tbFrame=\(tabBar?.frame ?? .zero) result=\(type(of: result).self)")Also applies to: 393-394
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/NiriCanvasView.swift` around lines 386 - 387, Replace the debug NSLog calls in the hit-testing debug block with dlog(...) to keep debug-event logs consistent: change the NSLog("niri.hitTest -> TAB BAR local=\(localPt) tbFrame=\(tb.frame)") (and the similar NSLog at the later occurrence around the same hitTest logic) to dlog("niri.hitTest -> TAB BAR local=\(localPt) tbFrame=\(tb.frame)") so the message, interpolated values (localPt, tb.frame) and surrounding control flow (return tb) remain unchanged but use the unified dlog debug-event path.
🤖 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/NiriCanvasView.swift`:
- Around line 755-767: The terminalEdge branch can crash when removing the only
tab from a panel that is also the target (sourcePi == targetPi) because panels
can become empty and subsequent accesses to panels[targetPi] go out of bounds;
change the logic in the case .terminalEdge handling (around variables sourcePi,
sourceTab, targetLive, liveIndices, panels) to detect the scenario where
removing the source tab will remove its panel (panels[sourcePi].tabs.count == 1)
and the target refers to that same panel, and handle it before any further
panels[...] indexing—either by early-returning, remapping targetPi to a safe
value, or recomputing liveIndices/targetPi after the removal and verifying guard
targetLive < live.count again; ensure all later reads of panels[targetPi] are
protected by the new guard so no out-of-bounds access occurs.
---
Duplicate comments:
In `@Sources/NiriCanvasView.swift`:
- Around line 731-734: The same-panel rightward drag lands one slot too far
because you use the pre-removal insertIdx directly; in the if targetPi ==
sourcePi branch adjust the insertion position when moving right by checking if
insertIdx > sourceTab and, if so, pass min(insertIdx - 1,
panels[sourcePi].tabs.count - 1) into reorderTab (instead of min(insertIdx,
...)); update the call site that currently uses reorderTab(from: sourceTab, to:
min(insertIdx, panels[sourcePi].tabs.count - 1), inPanel:
ObjectIdentifier(panels[sourcePi].containerView)) to subtract one for rightward
same-panel moves.
- Around line 1449-1459: The window-level key handler in NiriCanvasView is
swallowing terminal input (e.g., Ctrl+D EOF and unbound Cmd combos) instead of
letting focused GhosttyNSView handle them; update the logic so Ctrl+D and
numeric Ctrl shortcuts only act when the canvas/tab UI is the active responder
(e.g., not when the terminal/Ghostty is first responder), and do not
unconditionally consume unhandled Command combinations — call
canvas.performKeyEquivalent(with:) (or GhosttyNSView.performKeyEquivalent) and
if it returns false, allow the responder chain to receive the event rather than
returning/consuming it; adjust checks around handleCtrlD(),
switchToLastTabPublic(), switchToTabPublic(_:) and the final cmd-consume branch
so focused GhosttyNSView gets unbound shortcuts and terminal EOF.
- Around line 315-318: setSurfaces currently replaces panels wholesale which can
drop live TerminalSurface instances without running their shutdown/teardown
path; before you assign panels = surfaces.map { ... } capture the old panels,
for each old panel call its explicit teardown/cleanup method on the panel or the
underlying TerminalSurface (the codepath that shuts down connections/resources),
then removeFromSuperview(), and only after all old panels are properly torn down
create new panels via makePanel(with:) and add their containerView; ensure you
reference panels, setSurfaces(_:) and makePanel(with:) when making the change.
- Line 47: Replace hardcoded UI strings with localized variants: where you build
the tab title (the assignment using tab.title and the literal "Shell") use
String(localized: "niri.canvas.shell", defaultValue: "Shell") and apply
String(localized: "niri.canvas.terminalCanvas", defaultValue: "Terminal Canvas")
for any visible "Terminal Canvas" labels; find other hardcoded UI strings (the
other occurrences of "Shell" and "Terminal Canvas" noted in the review) and
replace them similarly using String(localized: ..., defaultValue: ...), keeping
the original literal as the defaultValue and using clear localization keys
(e.g., "niri.canvas.shell", "niri.canvas.terminalCanvas") so all user-facing
text in NiriCanvasView is localized.
- Around line 768-769: When removing a tab, adjust panels[sourcePi].activeTab so
it shifts left if the removed tab was to the left of the active one: compute
newActive = panels[sourcePi].activeTab - (removedIndex <
panels[sourcePi].activeTab ? 1 : 0), then clamp to valid range e.g.
panels[sourcePi].activeTab = max(0, min(newActive, panels[sourcePi].tabs.count -
1)); finally call panels[sourcePi].syncTabBar(). Use the actual removed tab
index variable from the surrounding code in place of removedIndex; apply this
change where panels[sourcePi].activeTab is currently only clamped with min(...).
---
Nitpick comments:
In `@Sources/NiriCanvasView.swift`:
- Around line 386-387: Replace the debug NSLog calls in the hit-testing debug
block with dlog(...) to keep debug-event logs consistent: change the
NSLog("niri.hitTest -> TAB BAR local=\(localPt) tbFrame=\(tb.frame)") (and the
similar NSLog at the later occurrence around the same hitTest logic) to
dlog("niri.hitTest -> TAB BAR local=\(localPt) tbFrame=\(tb.frame)") so the
message, interpolated values (localPt, tb.frame) and surrounding control flow
(return tb) remain unchanged but use the unified dlog debug-event path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8dbdf620-6cb8-4752-8f8d-206c991d18f9
📒 Files selected for processing (1)
Sources/NiriCanvasView.swift
There was a problem hiding this comment.
4 issues found across 1 file (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/NiriCanvasView.swift">
<violation number="1" location="Sources/NiriCanvasView.swift:557">
P2: Auto-scroll bounds should be based on viewport edges, not first/last panel frames; current checks can disable drag auto-scroll when panels overflow.</violation>
<violation number="2" location="Sources/NiriCanvasView.swift:750">
P1: Handle self-edge drops of a single-tab panel as a no-op before removing the source tab. Otherwise the source removal can empty `panels`, and subsequent edge-layout indexing can crash with out-of-bounds access.</violation>
<violation number="3" location="Sources/NiriCanvasView.swift:751">
P1: Guard against self-target center drops before calling `moveTab`; otherwise a single-tab panel can be marked closing and disappear.</violation>
<violation number="4" location="Sources/NiriCanvasView.swift:777">
P0: Handle the `panels.isEmpty` case after removing the source tab in `.terminalEdge`; current indexing can crash when dropping the last tab.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| } | ||
|
|
||
| // Recompute targetPi after potential removal | ||
| let newLive2 = liveIndices |
There was a problem hiding this comment.
P0: Handle the panels.isEmpty case after removing the source tab in .terminalEdge; current indexing can crash when dropping the last tab.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/NiriCanvasView.swift, line 777:
<comment>Handle the `panels.isEmpty` case after removing the source tab in `.terminalEdge`; current indexing can crash when dropping the last tab.</comment>
<file context>
@@ -647,16 +742,83 @@ final class NiriCanvasView: NSView {
+ }
+
+ // Recompute targetPi after potential removal
+ let newLive2 = liveIndices
+ switch edge {
+ case .left:
</file context>
| let newLive2 = liveIndices | |
| let newLive2 = liveIndices | |
| if panels.isEmpty { | |
| var newCol = makePanel(with: [surface]) | |
| newCol.targetWidth = 1.0 | |
| newCol.currentWidth = 1.0 | |
| newCol.presetIndex = -1 | |
| panels.insert(newCol, at: 0) | |
| addSubview(newCol.containerView) | |
| focusedIndex = 0 | |
| break | |
| } |
…ingle-tab own panel
There was a problem hiding this comment.
1 issue found across 1 file (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/NiriCanvasView.swift">
<violation number="1" location="Sources/NiriCanvasView.swift:664">
P1: The same-panel single-tab edge-drop guard is incomplete: it excludes only left/right, so top/bottom drops can still reach a crash path when the source panel is removed.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| if let edge { | ||
| // Skip edge drops on own panel with single tab (would just recreate same layout) | ||
| if isSamePanel && isSingleTab && (edge == .left || edge == .right) { return } |
There was a problem hiding this comment.
P1: The same-panel single-tab edge-drop guard is incomplete: it excludes only left/right, so top/bottom drops can still reach a crash path when the source panel is removed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/NiriCanvasView.swift, line 664:
<comment>The same-panel single-tab edge-drop guard is incomplete: it excludes only left/right, so top/bottom drops can still reach a crash path when the source panel is removed.</comment>
<file context>
@@ -657,10 +657,16 @@ final class NiriCanvasView: NSView {
+
if let edge {
+ // Skip edge drops on own panel with single tab (would just recreate same layout)
+ if isSamePanel && isSingleTab && (edge == .left || edge == .right) { return }
currentDropTarget = .init(kind: .terminalEdge(livePanel: li, edge: edge))
showDropZoneOverlay(in: termRect, edge: edge)
</file context>
| if isSamePanel && isSingleTab && (edge == .left || edge == .right) { return } | |
| if isSamePanel && isSingleTab { return } |
There was a problem hiding this comment.
♻️ Duplicate comments (6)
Sources/NiriCanvasView.swift (6)
315-317:⚠️ Potential issue | 🟠 Major
setSurfaces(_:)drops oldTerminalSurfaces without explicit teardown.Replacing
panelshere removes UI containers but skips the explicit surface teardown path for discarded tabs.Suggested fix
func setSurfaces(_ surfaces: [TerminalSurface]) { - for p in panels { p.containerView.removeFromSuperview() } + for p in panels { + for tab in p.tabs { + tab.teardownSurface() + } + p.containerView.removeFromSuperview() + } panels = surfaces.map { makePanel(with: [$0]) } for p in panels { addSubview(p.containerView) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/NiriCanvasView.swift` around lines 315 - 317, setSurfaces(_:) currently replaces panels and removes container views but never invokes the TerminalSurface teardown path, leaking/skip-cleanup discarded tabs; before removing or reassigning panels, iterate the existing panels and call the appropriate teardown/cleanup on their TerminalSurface instances (e.g. a teardown/close/destroy method exposed by TerminalSurface) and only then removeFromSuperview and replace panels via makePanel(with:). Ensure you reference panels, setSurfaces(_:), TerminalSurface, and makePanel(with:) when locating where to insert the teardown calls.
737-740:⚠️ Potential issue | 🟠 MajorSame-panel rightward tab drops are still off by one.
insertIdxis computed pre-removal; when moving right within the same panel, it must be adjusted before callingreorderTab.Suggested fix
if targetPi == sourcePi { + let adjustedInsert = insertIdx > sourceTab ? insertIdx - 1 : insertIdx reorderTab(from: sourceTab, - to: min(insertIdx, panels[sourcePi].tabs.count - 1), + to: min(adjustedInsert, panels[sourcePi].tabs.count - 1), inPanel: ObjectIdentifier(panels[sourcePi].containerView)) } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/NiriCanvasView.swift` around lines 737 - 740, When moving a tab within the same panel (targetPi == sourcePi) the pre-removal insertIdx must be adjusted for rightward moves; detect if targetPi == sourcePi and insertIdx > sourceTab.index (or the current tab index in panels[sourcePi].tabs) and decrement insertIdx by 1 before calling reorderTab; then call reorderTab(from: sourceTab, to: min(adjustedInsertIdx, panels[sourcePi].tabs.count - 1), inPanel: ObjectIdentifier(panels[sourcePi].containerView)) so the destination index accounts for the removal.
664-665:⚠️ Potential issue | 🔴 CriticalSelf-edge drop of a single-tab panel can still crash (
.top/.bottom).The guard only blocks
.left/.right;.top/.bottomon the same single-tab panel can remove the source panel and then indexpanels[...]when empty.Suggested fix
if let edge { - // Skip edge drops on own panel with single tab (would just recreate same layout) - if isSamePanel && isSingleTab && (edge == .left || edge == .right) { return } + // Skip any edge drop on own single-tab panel (no-op and avoids empty-array indexing later) + if isSamePanel && isSingleTab { return } currentDropTarget = .init(kind: .terminalEdge(livePanel: li, edge: edge)) showDropZoneOverlay(in: termRect, edge: edge)Also applies to: 761-766, 808-809
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/NiriCanvasView.swift` around lines 664 - 665, The guard that prevents self-edge drops only checks left/right and misses top/bottom, so a drag to .top/.bottom of the same single-tab panel can remove the source and later index an empty panels array; change the early-return condition used before assigning currentDropTarget = .init(kind: .terminalEdge(livePanel: li, edge: edge)) to return whenever isSamePanel && isSingleTab (i.e., treat all edges the same) or explicitly include .top and .bottom in the check; apply the same fix to the other identical blocks that set currentDropTarget (the other occurrences using isSamePanel, isSingleTab, edge and .terminalEdge(livePanel:...)).
774-775:⚠️ Potential issue | 🟠 MajorActive tab should shift left when removing a tab to its left.
All three removal paths clamp with
min(...)only, which can switch the visible surface unexpectedly after drag/move.Suggested fix pattern (apply in all three blocks)
- panels[srcPi].activeTab = min(panels[srcPi].activeTab, panels[srcPi].tabs.count - 1) + if srcTab < panels[srcPi].activeTab { + panels[srcPi].activeTab -= 1 + } else { + panels[srcPi].activeTab = min(panels[srcPi].activeTab, panels[srcPi].tabs.count - 1) + }Also applies to: 849-850, 884-885
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/NiriCanvasView.swift` around lines 774 - 775, The activeTab adjustment currently only clamps down with min(...) which can jump the visible tab when a tab left of the active one is removed; instead update panels[sourcePi].activeTab by decrementing when the removed tab index is less than the current activeTab, otherwise clamp to the last index. Locate the three blocks that set panels[sourcePi].activeTab and syncTabBar() and replace the single min(...) assignment with logic: if removedIndex < panels[sourcePi].activeTab then panels[sourcePi].activeTab -= 1 else panels[sourcePi].activeTab = min(panels[sourcePi].activeTab, panels[sourcePi].tabs.count - 1); then call panels[sourcePi].syncTabBar(); apply this change for the occurrences around the existing panels[sourcePi].activeTab / tabs.count updates.
47-47:⚠️ Potential issue | 🟡 MinorLocalize visible canvas strings (
Shell, window title).These are user-facing labels/titles and should use
String(localized:..., defaultValue:...)keys instead of bare literals.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 ... UI elements."Also applies to: 193-193, 268-268, 507-508, 1483-1483
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/NiriCanvasView.swift` at line 47, Replace bare user-facing string literals with the localization API: update the title assignment (let title = ...) to use String(localized:..., defaultValue:...) instead of the literal "Shell", and similarly replace any other visible labels/window titles in this file (the other occurrences noted) to use localization keys (e.g. "canvas.shell.title", "canvas.window.title", etc.) with appropriate defaultValue text; ensure you use descriptive localization keys and keep the same logic (e.g., tab.title.isEmpty ? String(localized:..., defaultValue:...) : String(tab.title.prefix(20))).
1455-1466:⚠️ Potential issue | 🟠 MajorWindow-level key handling still steals terminal-owned input.
Ctrl+Dis consumed here, and unhandledCmdcombos are swallowed (if cmd { return }), so terminal delivery is still blocked for unbound shortcuts.Based on learnings,
GhosttyNSView.performKeyEquivalent(with:)should forward unbound Command combinations (not swallow them) so terminal input remains intact.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/NiriCanvasView.swift` around lines 1455 - 1466, The window-level key handler is swallowing terminal-owned input; change it so only truly-handled shortcuts stop propagation: make canvasView?.handleCtrlD() return a Bool (or add a boolean predicate) and only return from the key handler when that call returns true, similarly use the Boolean result of canvas.performKeyEquivalent(with:) to decide whether to return, and remove the unconditional swallow on Cmd combos (replace the `if cmd { return }` with letting the event fall through or calling super/GhosttyNSView.performKeyEquivalent(with:) so unbound Command combinations are forwarded to the terminal). Ensure references: handleCtrlD(), switchToLastTabPublic(), switchToTabPublic(), performKeyEquivalent(with:), and GhosttyNSView.performKeyEquivalent(with:) are updated accordingly.
🧹 Nitpick comments (1)
Sources/NiriCanvasView.swift (1)
386-387: Usedlog(...)instead ofNSLog(...)for DEBUG event traces.This file already centralizes debug logging via
nlog/dlog; switching these hit-test traces keeps logging consistent.Based on learnings, all DEBUG key/mouse/focus/split/tab events should use
dlog("message").Also applies to: 393-394
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/NiriCanvasView.swift` around lines 386 - 387, Replace debug traces that call NSLog(...) inside the NiriCanvasView hit-test logic with the project's debug logger dlog(...); specifically update the NSLog call that prints "niri.hitTest -> TAB BAR local=\(localPt) tbFrame=\(tb.frame)" to use dlog and change the other two NSLog calls around the same hitTest code path (the ones at the nearby locations referenced in the comment) to dlog as well so all DEBUG key/mouse/focus/split/tab events use the centralized dlog logging; locate these in the hitTest implementation where localPt and tb.frame are logged (and the two adjacent NSLog lines at 393-394) and swap them to dlog("...") with the same message content.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@Sources/NiriCanvasView.swift`:
- Around line 315-317: setSurfaces(_:) currently replaces panels and removes
container views but never invokes the TerminalSurface teardown path,
leaking/skip-cleanup discarded tabs; before removing or reassigning panels,
iterate the existing panels and call the appropriate teardown/cleanup on their
TerminalSurface instances (e.g. a teardown/close/destroy method exposed by
TerminalSurface) and only then removeFromSuperview and replace panels via
makePanel(with:). Ensure you reference panels, setSurfaces(_:), TerminalSurface,
and makePanel(with:) when locating where to insert the teardown calls.
- Around line 737-740: When moving a tab within the same panel (targetPi ==
sourcePi) the pre-removal insertIdx must be adjusted for rightward moves; detect
if targetPi == sourcePi and insertIdx > sourceTab.index (or the current tab
index in panels[sourcePi].tabs) and decrement insertIdx by 1 before calling
reorderTab; then call reorderTab(from: sourceTab, to: min(adjustedInsertIdx,
panels[sourcePi].tabs.count - 1), inPanel:
ObjectIdentifier(panels[sourcePi].containerView)) so the destination index
accounts for the removal.
- Around line 664-665: The guard that prevents self-edge drops only checks
left/right and misses top/bottom, so a drag to .top/.bottom of the same
single-tab panel can remove the source and later index an empty panels array;
change the early-return condition used before assigning currentDropTarget =
.init(kind: .terminalEdge(livePanel: li, edge: edge)) to return whenever
isSamePanel && isSingleTab (i.e., treat all edges the same) or explicitly
include .top and .bottom in the check; apply the same fix to the other identical
blocks that set currentDropTarget (the other occurrences using isSamePanel,
isSingleTab, edge and .terminalEdge(livePanel:...)).
- Around line 774-775: The activeTab adjustment currently only clamps down with
min(...) which can jump the visible tab when a tab left of the active one is
removed; instead update panels[sourcePi].activeTab by decrementing when the
removed tab index is less than the current activeTab, otherwise clamp to the
last index. Locate the three blocks that set panels[sourcePi].activeTab and
syncTabBar() and replace the single min(...) assignment with logic: if
removedIndex < panels[sourcePi].activeTab then panels[sourcePi].activeTab -= 1
else panels[sourcePi].activeTab = min(panels[sourcePi].activeTab,
panels[sourcePi].tabs.count - 1); then call panels[sourcePi].syncTabBar(); apply
this change for the occurrences around the existing panels[sourcePi].activeTab /
tabs.count updates.
- Line 47: Replace bare user-facing string literals with the localization API:
update the title assignment (let title = ...) to use String(localized:...,
defaultValue:...) instead of the literal "Shell", and similarly replace any
other visible labels/window titles in this file (the other occurrences noted) to
use localization keys (e.g. "canvas.shell.title", "canvas.window.title", etc.)
with appropriate defaultValue text; ensure you use descriptive localization keys
and keep the same logic (e.g., tab.title.isEmpty ? String(localized:...,
defaultValue:...) : String(tab.title.prefix(20))).
- Around line 1455-1466: The window-level key handler is swallowing
terminal-owned input; change it so only truly-handled shortcuts stop
propagation: make canvasView?.handleCtrlD() return a Bool (or add a boolean
predicate) and only return from the key handler when that call returns true,
similarly use the Boolean result of canvas.performKeyEquivalent(with:) to decide
whether to return, and remove the unconditional swallow on Cmd combos (replace
the `if cmd { return }` with letting the event fall through or calling
super/GhosttyNSView.performKeyEquivalent(with:) so unbound Command combinations
are forwarded to the terminal). Ensure references: handleCtrlD(),
switchToLastTabPublic(), switchToTabPublic(), performKeyEquivalent(with:), and
GhosttyNSView.performKeyEquivalent(with:) are updated accordingly.
---
Nitpick comments:
In `@Sources/NiriCanvasView.swift`:
- Around line 386-387: Replace debug traces that call NSLog(...) inside the
NiriCanvasView hit-test logic with the project's debug logger dlog(...);
specifically update the NSLog call that prints "niri.hitTest -> TAB BAR
local=\(localPt) tbFrame=\(tb.frame)" to use dlog and change the other two NSLog
calls around the same hitTest code path (the ones at the nearby locations
referenced in the comment) to dlog as well so all DEBUG
key/mouse/focus/split/tab events use the centralized dlog logging; locate these
in the hitTest implementation where localPt and tb.frame are logged (and the two
adjacent NSLog lines at 393-394) and swap them to dlog("...") with the same
message content.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 23b6f825-06ab-46f5-9b7c-7bb542e4e42d
📒 Files selected for processing (1)
Sources/NiriCanvasView.swift
…onary in edge zone
There was a problem hiding this comment.
1 issue found across 1 file (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/NiriCanvasView.swift">
<violation number="1" location="Sources/NiriCanvasView.swift:541">
P2: Refresh the drop target before handling `.leftMouseUp`; otherwise per-frame drag auto-scroll can leave `currentDropTarget` stale and apply the drop to the wrong destination.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
- Scroll speed ramps up over 0.4s (quadratic ease-in): starts slow, accelerates - Speed also scales with proximity to edge (closer = faster) - Drop overlay and target update during tick-driven scroll (follows content) - Dwell timer resets when cursor leaves the scroll zone
- Clamp scroll to max (last panel right edge) - After drop, restore scroll position and only adjust if focused panel is off-screen - Drop target stays visually stable instead of jumping
1 participant
Summary
Niri/PaperWM-style horizontal terminal strip layout prototype (debug only, Cmd+Ctrl+N).
Testing
Related
Summary by CodeRabbit
Summary by cubic
Experimental Niri/PaperWM-style terminal canvas with per-panel tabs, cross-panel drag-drop, edge drop zones for splitting (larger 40% zones on outer edges), continuous drag auto-scroll, and stable drop behavior; rendered at 120 fps. Debug-only: open with Cmd+Ctrl+N; includes scrollable tab bars, terminal-driven titles, Ctrl+Tab cycling, Cmd+D split-right, and fixes for self-drop edge cases.
New Features
ghosttyterminals in a horizontal strip with per-panel tab bars; content-sized, scrollable tabs; titles synced from terminal; bar colors follow theme.Dependencies
ghosttyto latest upstream and syncedghostty.h(IO mode, write callback, renderer/selection symbols); added newGhosttyKitchecksum.read_clipboard_cbnow returns Bool; handledGHOSTTY_ACTION_SET_TAB_TITLE(canvas manages titles).Written for commit bb4efca. Summary will update on new commits.