Repository navigation
Update ghostty to latest upstream - #2167
lawrencecchen wants to merge 4 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughUpdates the Ghostty submodule pointer and corresponding host integration: C API additions (IO enums, io_write callback, new fullscreen and action enums, clipboard callback now returns bool, new surface output API) and Swift-side adaptations plus a new DEBUG-only Niri canvas UI, tests, and demo scripts. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as App UI
participant Surface as Ghostty Surface
participant Runtime as Ghostty Runtime
participant Clipboard as System Clipboard
UI->>Surface: provide input / schedule IO (io_mode / io_write_cb)
Surface->>Runtime: ghostty_surface_process_output(data)
Runtime->>Surface: emit action (e.g., COPY_TITLE_TO_CLIPBOARD / SET_TAB_TITLE)
alt COPY_TITLE_TO_CLIPBOARD
Surface->>Runtime: runtime_read_clipboard_cb(... ) -> bool
alt returns true
Runtime->>Clipboard: schedule/read clipboard
Clipboard-->>Runtime: result / completion
Surface-->>UI: acknowledge action handled
else returns false
Surface-->>UI: treat as unhandled
end
else SET_TAB_TITLE
Surface-->>UI: ignore terminal title override (handled elsewhere)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 docstrings
🧪 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 updates the Key concerns:
Confidence Score: 3/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["ghostty-org/ghostty (upstream)"] -->|"1043 new commits merged"| B["manaflow-ai/ghostty\ntask-ghostty-upstream-sync branch"]
B -->|"cmux patches preserved\n(OSC 99, resize fixes, prompt markers,\ntheme picker hooks)"| B
B -->|"⚠️ Should merge to main FIRST"| C["manaflow-ai/ghostty:main"]
C -->|"submodule pointer update\nbc9be90a → f41d302a"| D["manaflow-ai/cmux\nghostty submodule"]
style B fill:#ffcc00,stroke:#cc9900
style C fill:#90EE90,stroke:#228B22
style D fill:#87CEEB,stroke:#4682B4
Reviews (1): Last reviewed commit: "Update ghostty submodule to latest upstr..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @@ -1 +1 @@ | |||
| Subproject commit bc9be90a21997a4e5f06bf15ae2ec0f937c2dc42 | |||
| Subproject commit f41d302a378277d76c3736f9ad5f6d04523bce9e | |||
There was a problem hiding this comment.
manaflow-ai/ghostty:main
The PR description states the Ghostty fork work lives on the task-ghostty-upstream-sync branch (https://github.com/manaflow-ai/ghostty/tree/task-ghostty-upstream-sync), but CLAUDE.md mandates:
Submodule safety: When modifying a submodule (ghostty, vendor/bonsplit, etc.), always push the submodule commit to its remote
mainbranch BEFORE committing the updated pointer in the parent repo. Never commit on a detached HEAD or temporary branch — the commit will be orphaned and lost. Verify with:cd <submodule> && git merge-base --is-ancestor HEAD origin/main.
The new submodule pointer (f41d302a) must be reachable from manaflow-ai/ghostty:main before this parent-repo commit lands. If the task-ghostty-upstream-sync branch is ever deleted or not merged, the pointer will be orphaned and git submodule update will fail for all developers and CI.
Please merge (or push) the fork branch to manaflow-ai/ghostty:main and verify:
cd ghostty
git merge-base --is-ancestor f41d302a378277d76c3736f9ad5f6d04523bce9e origin/mainbefore merging this PR.
Context Used: CLAUDE.md (source)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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: Add a new entry to the ghostty-fork.md sync history documenting the
submodule pointer bump to origin/main: include the exact commit hash that the
submodule now points to, a short bullet-list of upstream changes that were
brought in by that commit (features/bugfixes/other notable diffs), any conflict
resolution steps taken (files changed, manual edits made, or "no conflicts"),
and a brief timestamp/author line; place this entry under the existing "Update
this file with the new change summary + conflict notes" section so the sync
history stays up to date and clearly indicates the new pointer bump to
origin/main.
- Line 1: Push the ghostty submodule commit
f41d302a378277d76c3736f9ad5f6d04523bce9e to the manaflow-ai/ghostty repo and
ensure origin/main contains that commit (so the submodule HEAD is an ancestor of
origin/main); then update docs/ghostty-fork.md by adding a new "Current fork
changes" entry summarizing the sync and any conflicts for this commit; finally
compute the sha256 of the built GhosttyKit artifact for commit f41d302a...
(f41d302a378277d76c3736f9ad5f6d04523bce9e) and append a line with that checksum
and commit id to scripts/ghosttykit-checksums.txt so CI can validate it.
| @@ -1 +1 @@ | |||
| Subproject commit bc9be90a21997a4e5f06bf15ae2ec0f937c2dc42 | |||
| Subproject commit f41d302a378277d76c3736f9ad5f6d04523bce9e | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Verify submodule commit ancestry against remote main"
git -C ghostty fetch origin main
git -C ghostty rev-parse HEAD
git -C ghostty merge-base --is-ancestor HEAD origin/main && echo "OK: HEAD is on origin/main ancestry"
echo
echo "Check whether docs mention this sync commit/context"
rg -n 'f41d302a378277d76c3736f9ad5f6d04523bce9e|upstream sync|conflict' docs/ghostty-fork.md || trueRepository: manaflow-ai/cmux
Length of output: 574
Update docs/ghostty-fork.md with this submodule commit sync details.
The ghostty submodule pointer is correctly positioned on origin/main. Add an entry to the fork documentation documenting this pointer bump, including the commit hash, any upstream changes synced, and any conflict resolution notes (if applicable). Keep the sync history in the doc up to date as specified in the "Update this file with the new change summary + conflict notes" section (line 10 of the doc).
🧰 Tools
🪛 GitHub Actions: CI
[error] 1-1: GhosttyKit checksum missing for ghostty f41d302a378277d76c3736f9ad5f6d04523bce9e in scripts/ghosttykit-checksums.txt; step failed (exit code 1).
🪛 GitHub Actions: macOS Compatibility
[error] 1-1: Missing pinned GhosttyKit checksum for ghostty f41d302a378277d76c3736f9ad5f6d04523bce9e 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, Add a new entry to the ghostty-fork.md sync history
documenting the submodule pointer bump to origin/main: include the exact commit
hash that the submodule now points to, a short bullet-list of upstream changes
that were brought in by that commit (features/bugfixes/other notable diffs), any
conflict resolution steps taken (files changed, manual edits made, or "no
conflicts"), and a brief timestamp/author line; place this entry under the
existing "Update this file with the new change summary + conflict notes" section
so the sync history stays up to date and clearly indicates the new pointer bump
to origin/main.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate checksum file and confirm missing/new entries"
fd ghosttykit-checksums.txt
rg -n 'bc9be90a21997a4e5f06bf15ae2ec0f937c2dc42|f41d302a378277d76c3736f9ad5f6d04523bce9e' scripts/ghosttykit-checksums.txt || true
echo
echo "Show checksum file format (use this to add the new line in correct format)"
sed -n '1,120p' scripts/ghosttykit-checksums.txtRepository: manaflow-ai/cmux
Length of output: 1266
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Verify ghostty submodule commit is on origin/main ==="
cd ghostty 2>/dev/null && {
echo "Current HEAD: $(git rev-parse HEAD)"
echo "Checking ancestry against origin/main..."
if git merge-base --is-ancestor HEAD origin/main 2>/dev/null; then
echo "✓ Current HEAD is ancestor of origin/main (correct)"
else
echo "✗ WARNING: Current HEAD is NOT on the main branch path"
fi
git log --oneline -5
cd - > /dev/null
} || echo "ghostty directory not found or not a git repo"
echo
echo "=== Check docs/ghostty-fork.md for sync notes ==="
if [ -f docs/ghostty-fork.md ]; then
echo "Found docs/ghostty-fork.md. Checking content:"
head -50 docs/ghostty-fork.md
echo "..."
echo "[File exists; checking for any TODOs or outdated notes]"
rg -i 'todo|fixme|outdated|needs update|sync' docs/ghostty-fork.md || echo "No update markers found"
else
echo "docs/ghostty-fork.md not found"
fi
echo
echo "=== Verify no other missing checksum entries exist ==="
echo "Total entries in checksums file:"
wc -l scripts/ghosttykit-checksums.txt
echo "Checking for duplicate or malformed entries:"
rg '^\w{40}\s+\w{64}$' scripts/ghosttykit-checksums.txt | wc -lRepository: manaflow-ai/cmux
Length of output: 2944
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Get the sha256 for the f41d302 commit ==="
# Check if there's any documentation or recent build that might have this checksum
rg -B2 -A2 'f41d302|4ed8698' scripts/ || echo "No cached checksums found for these commits"
echo
echo "=== Check git history of checksums file for format examples ==="
git log --oneline scripts/ghosttykit-checksums.txt | head -10
echo
echo "=== Identify the exact commit being referenced in the PR ==="
cd ghostty 2>/dev/null && git show f41d302a378277d76c3736f9ad5f6d04523bce9e --oneline || echo "Commit not in ghostty repo"Repository: manaflow-ai/cmux
Length of output: 559
Follow the documented submodule update process before committing this pointer.
This PR violates three required steps from the docs/ghostty-fork.md checklist:
-
Submodule not pushed to origin/main: The current ghostty HEAD is not an ancestor of
origin/main. Per the checklist, changes must be committed and pushed tomanaflow-ai/ghosttybefore updating the parent submodule pointer. -
Missing fork change documentation:
docs/ghostty-fork.mdhas not been updated with a new entry documenting this fork sync (no new section under "Current fork changes"). The checklist requires: "Update this file with the new change summary + conflict notes." -
Missing GhosttyKit checksum:
scripts/ghosttykit-checksums.txthas no entry for commitf41d302a378277d76c3736f9ad5f6d04523bce9e. Add the sha256 checksum for the built GhosttyKit artifact on this commit (required for CI).
Resolve all three in order:
- Push the ghostty submodule commit to
manaflow-ai/ghosttymain and verify it is reachable fromorigin/main - Update
docs/ghostty-fork.mdwith the fork change summary - Add the corresponding checksum entry to
scripts/ghosttykit-checksums.txt
🧰 Tools
🪛 GitHub Actions: CI
[error] 1-1: GhosttyKit checksum missing for ghostty f41d302a378277d76c3736f9ad5f6d04523bce9e in scripts/ghosttykit-checksums.txt; step failed (exit code 1).
🪛 GitHub Actions: macOS Compatibility
[error] 1-1: Missing pinned GhosttyKit checksum for ghostty f41d302a378277d76c3736f9ad5f6d04523bce9e 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, Push the ghostty submodule commit
f41d302a378277d76c3736f9ad5f6d04523bce9e to the manaflow-ai/ghostty repo and
ensure origin/main contains that commit (so the submodule HEAD is an ancestor of
origin/main); then update docs/ghostty-fork.md by adding a new "Current fork
changes" entry summarizing the sync and any conflicts for this commit; finally
compute the sha256 of the built GhosttyKit artifact for commit f41d302a...
(f41d302a378277d76c3736f9ad5f6d04523bce9e) and append a line with that checksum
and commit id to scripts/ghosttykit-checksums.txt so CI can validate it.
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).
4ed8698 to
72e846c
Compare
- 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)
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9aa71fd006
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| case GHOSTTY_ACTION_SET_TAB_TITLE: | ||
| // Tab title override from the terminal (e.g. OSC escape). | ||
| // cmux handles tab titles through its own tab manager, so we ignore this. | ||
| return true |
There was a problem hiding this comment.
GHOSTTY_ACTION_SET_TAB_TITLE is now marked handled but ignored, which drops terminal-provided title changes (e.g. OSC title updates) before they reach cmux’s title pipeline. TabManager updates panel/window titles from .ghosttyDidSetTitle, and this branch never posts that notification, so users can end up with stale tab/toolbar titles after the Ghostty upgrade.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ghostty.h`:
- Around line 907-908: Add a new switch case for
GHOSTTY_ACTION_COPY_TITLE_TO_CLIPBOARD in the same Swift action handler switch
that already handles GHOSTTY_ACTION_SET_TAB_TITLE; implement it to read the
provided title payload (same source used by GHOSTTY_ACTION_SET_TAB_TITLE) and
copy that string to the system clipboard using the existing project clipboard
utility or NSPasteboard, matching error handling/logging style of the
surrounding cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e712da17-6269-488e-90c1-327d315d93e6
📒 Files selected for processing (2)
Sources/GhosttyTerminalView.swiftghostty.h
| GHOSTTY_ACTION_SET_TAB_TITLE, | ||
| GHOSTTY_ACTION_PROMPT_TITLE, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if Swift handles the new action tags
rg -n 'SET_TAB_TITLE|COPY_TITLE_TO_CLIPBOARD' --type swift -C3
# Look at action handler implementation
rg -n 'action_cb|ghostty_action_tag_e' --type swift -A10 -B2Repository: manaflow-ai/cmux
Length of output: 1753
🏁 Script executed:
# Search more broadly for any mention of COPY_TITLE or copy_title
rg -n 'COPY_TITLE|copy_title' --type swift
# Get the full action handler switch statement to see all cases
rg -n 'case GHOSTTY_ACTION' --type swift | head -50Repository: manaflow-ai/cmux
Length of output: 2073
🏁 Script executed:
# Check ghostty.h around line 938 to confirm the action is defined
sed -n '930,945p' ghostty.h
# Search entire repo for COPY_TITLE_TO_CLIPBOARD to see if it's defined anywhere
rg -n 'COPY_TITLE_TO_CLIPBOARD' --type c --type hRepository: manaflow-ai/cmux
Length of output: 632
Add Swift case for GHOSTTY_ACTION_COPY_TITLE_TO_CLIPBOARD.
The action is defined in ghostty.h (line 938) but missing from the Swift action handler. GHOSTTY_ACTION_SET_TAB_TITLE is correctly handled at line 2455, but GHOSTTY_ACTION_COPY_TITLE_TO_CLIPBOARD requires a case statement in the same switch block.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ghostty.h` around lines 907 - 908, Add a new switch case for
GHOSTTY_ACTION_COPY_TITLE_TO_CLIPBOARD in the same Swift action handler switch
that already handles GHOSTTY_ACTION_SET_TAB_TITLE; implement it to read the
provided title payload (same source used by GHOSTTY_ACTION_SET_TAB_TITLE) and
copy that string to the system clipboard using the existing project clipboard
utility or NSPasteboard, matching error handling/logging style of the
surrounding cases.
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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4576358bdb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ) | ||
| } | ||
| } | ||
| return true |
There was a problem hiding this comment.
read_clipboard_cb now always returns true after enqueueing work, but the main-queue block can still bail out at callbackContext.runtimeSurface == nil without ever calling ghostty_surface_complete_clipboard_request. In that close-race path (for example, paste requested and surface closes immediately), Ghostty is told the request was handled even though no completion is possible, which can leave the read unresolved. Return false when the request cannot be completed (or guarantee completion even on teardown).
Useful? React with 👍 / 👎.
| styleMask: [.titled, .closable, .resizable, .fullSizeContentView], | ||
| backing: .buffered, defer: false | ||
| ) | ||
| win.title = "Terminal Canvas" |
There was a problem hiding this comment.
The new debug window title is hardcoded as "Terminal Canvas", but this repo’s policy requires all user-visible UI strings to go through String(localized:..., defaultValue:...). Shipping this as-is introduces an untranslated UI string in non-English builds and bypasses the localization pipeline.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
scripts/demo-paperwm.swift (1)
299-312: Consider simplifying the interpolation logic.The current animation applies two consecutive blends per tick (lines 304-305), which is unconventional. Line 305 alone would produce smooth interpolation toward
targetOffset:scrollOffset = scrollOffset * damping + targetOffset * (1 - damping)The current approach works for this demo but could be simplified if this pattern is later adapted for production use.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/demo-paperwm.swift` around lines 299 - 312, The tick() function currently applies two blends to scrollOffset (first adding diff * stiffness then applying a damping blend), which is redundant; simplify by removing the initial stiffness step (the calculation that uses diff * stiffness) and use only the damping blend to interpolate scrollOffset toward targetOffset (i.e., set scrollOffset using the damping/targetOffset blend), keep the proximity snap check (abs(diff) < 0.5) and the updateLayout(animated: false) call intact so behavior remains unchanged aside from the simplified interpolation.Sources/AppDelegate.swift (1)
9086-9091: Log this new debug key event path.Please add a
dlog(...)line when this shortcut fires so the DEBUG key-routing trail is complete.Proposed tweak
if normalizedFlags == [.command, .control] && (chars.lowercased() == "n" || event.keyCode == 45) { + dlog("shortcut.action name=openNiriCanvas keyCode=\(event.keyCode)") openNiriCanvas() return true }Based on learnings: Applies to **/*.swift : All debug events (keys, mouse, focus, splits, tabs) in DEBUG builds go to the debug event log. Use free function
dlog("message")to log events.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 9086 - 9091, Add a debug-log call when the DEBUG-only key shortcut is handled: inside the conditional that checks hasCommand && hasControl && !flags.contains(.shift) && (chars.lowercased() == "n" || event.keyCode == 45), call dlog(...) with a clear message (e.g. "DEBUG key event: openNiriCanvas shortcut triggered") immediately before invoking openNiriCanvas() so the event appears in the debug event log; keep the dlog call inside the same `#if` DEBUG block and include identifying info such as chars or event.keyCode if desired.Sources/NiriCanvasView.swift (1)
34-38: Consider idling and coalescing the display-link ticks.The link starts in
setup()and enqueues a main-threadtick()every refresh for the full window lifetime. With no idle gate or pending-tick coalescing, this keeps relayout running at display refresh even when nothing is moving, and a busy main thread can accumulate stale ticks. A small “is animating?” / “tick already queued?” gate would make this much cheaper.Also applies to: 280-289, 291-340
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/NiriCanvasView.swift` around lines 34 - 38, The display-link created in setup() via startDisplayLink() currently enqueues tick() on every refresh regardless of activity; add a simple boolean gate (e.g., isAnimating) and a "tickQueued"/pendingTick flag to coalesce/skip redundant ticks so that startDisplayLink()/display callback only schedules a tick when isAnimating is true and if no tick is already queued; ensure tick() clears tickQueued when it begins and that stopDisplayLink() (or when isAnimating becomes false) prevents further scheduling. Update references in setup(), startDisplayLink(), the display callback, and tick() to use these flags so idle canvases do not continuously enqueue main-thread work.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/AppDelegate.swift`:
- Around line 9086-9091: The current shortcut condition (using hasCommand &&
hasControl && !flags.contains(.shift)) still matches extra modifiers (e.g.,
Option) and should require an exact modifier set; replace the loose checks with
an exact comparison using the event's modifier flags (e.g., compare
event.modifierFlags.intersection(.deviceIndependentFlagsMask) == [.command,
.control]) so the branch that calls openNiriCanvas() only runs when modifiers
are exactly Command+Control and no other modifiers are present; update the
condition that references hasCommand, hasControl, flags, chars.lowercased(), and
event.keyCode accordingly.
In `@Sources/NiriCanvasView.swift`:
- Around line 177-197: The current ensureVisible implementation only clamps
targetOffset to a minimum of 0, which allows it to exceed the scrollable extent;
compute a maxScrollOffset from the current animated content widths and close
progress (the same value used to render the animated content width) and clamp
both targetOffset and scrollOffset to the range [0, maxScrollOffset]; update the
ensureVisible function (reference: ensureVisible(panelStart:panelWidth:),
variables targetOffset and viewport/maxW) to use maxScrollOffset and also apply
the same [0, maxScrollOffset] clamping in the other places that adjust offsets
(the other offset-update code paths referenced in the review where
scrollOffset/targetOffset are modified) so no offset can go past the animated
content width.
- Line 378: Replace the bare UI string assigned to win.title ("Terminal Canvas")
with a localized lookup using String(localized: "window.terminalCanvas.title",
defaultValue: "Terminal Canvas") and add the key "window.terminalCanvas.title"
with the English value "Terminal Canvas" to Resources/Localizable.xcstrings (and
translations for other languages); update the assignment at the win.title site
in NiriCanvasView (where win.title is set) so the window title uses that
localized string helper.
- Around line 265-273: nearestLiveIndex(forOffset:) currently compares panel
centers to the viewport left edge (offset) which biases selection; compute the
viewport midpoint instead and use that for distance comparisons. Replace uses of
offset in nearestLiveIndex(forOffset:) with a midpoint value (e.g. let
viewportMid = offset + self.bounds.width / 2 or your visibleWidth property) and
compare abs(mid - viewportMid) when computing bestDist; keep the rest of the
loop (pw(for:), panelGap, slots, li) unchanged so you still return the index of
the nearest live panel.
- Around line 213-218: In closeFocusedTerminal(), the code currently checks only
if let s = slots[si].surface.surface before calling
ghostty_surface_request_close(s), which can pass a stale pointer; change the
guard so you check the TerminalSurface live flag first (use the
TerminalSurface.hasLiveSurface / portalLifecycleState check on
slots[si].surface) and only call ghostty_surface_request_close(s) when
hasLiveSurface is true and surface.surface is non-nil (i.e., ensure
slots[si].surface.hasLiveSurface && slots[si].surface.surface != nil before
invoking ghostty_surface_request_close).
---
Nitpick comments:
In `@scripts/demo-paperwm.swift`:
- Around line 299-312: The tick() function currently applies two blends to
scrollOffset (first adding diff * stiffness then applying a damping blend),
which is redundant; simplify by removing the initial stiffness step (the
calculation that uses diff * stiffness) and use only the damping blend to
interpolate scrollOffset toward targetOffset (i.e., set scrollOffset using the
damping/targetOffset blend), keep the proximity snap check (abs(diff) < 0.5) and
the updateLayout(animated: false) call intact so behavior remains unchanged
aside from the simplified interpolation.
In `@Sources/AppDelegate.swift`:
- Around line 9086-9091: Add a debug-log call when the DEBUG-only key shortcut
is handled: inside the conditional that checks hasCommand && hasControl &&
!flags.contains(.shift) && (chars.lowercased() == "n" || event.keyCode == 45),
call dlog(...) with a clear message (e.g. "DEBUG key event: openNiriCanvas
shortcut triggered") immediately before invoking openNiriCanvas() so the event
appears in the debug event log; keep the dlog call inside the same `#if` DEBUG
block and include identifying info such as chars or event.keyCode if desired.
In `@Sources/NiriCanvasView.swift`:
- Around line 34-38: The display-link created in setup() via startDisplayLink()
currently enqueues tick() on every refresh regardless of activity; add a simple
boolean gate (e.g., isAnimating) and a "tickQueued"/pendingTick flag to
coalesce/skip redundant ticks so that startDisplayLink()/display callback only
schedules a tick when isAnimating is true and if no tick is already queued;
ensure tick() clears tickQueued when it begins and that stopDisplayLink() (or
when isAnimating becomes false) prevents further scheduling. Update references
in setup(), startDisplayLink(), the display callback, and tick() to use these
flags so idle canvases do not continuously enqueue main-thread work.
🪄 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: dab98a3c-2bda-4f3e-a861-41af94a0af07
📒 Files selected for processing (4)
GhosttyTabs.xcodeproj/project.pbxprojSources/AppDelegate.swiftSources/NiriCanvasView.swiftscripts/demo-paperwm.swift
| #if DEBUG | ||
| // Cmd+Ctrl+N: Open niri canvas demo | ||
| if hasCommand && hasControl && !flags.contains(.shift) && (chars.lowercased() == "n" || event.keyCode == 45) { | ||
| openNiriCanvas() | ||
| return true | ||
| } |
There was a problem hiding this comment.
Require an exact modifier match for the Cmd+Ctrl+N shortcut.
Line 9088 currently also matches combinations like Cmd+Ctrl+Option+N. That can unintentionally steal unrelated shortcuts.
Proposed fix
- if hasCommand && hasControl && !flags.contains(.shift) && (chars.lowercased() == "n" || event.keyCode == 45) {
+ if normalizedFlags == [.command, .control] && (chars.lowercased() == "n" || event.keyCode == 45) {
openNiriCanvas()
return true
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/AppDelegate.swift` around lines 9086 - 9091, The current shortcut
condition (using hasCommand && hasControl && !flags.contains(.shift)) still
matches extra modifiers (e.g., Option) and should require an exact modifier set;
replace the loose checks with an exact comparison using the event's modifier
flags (e.g., compare
event.modifierFlags.intersection(.deviceIndependentFlagsMask) == [.command,
.control]) so the branch that calls openNiriCanvas() only runs when modifiers
are exactly Command+Control and no other modifiers are present; update the
condition that references hasCommand, hasControl, flags, chars.lowercased(), and
event.keyCode accordingly.
| func closeFocusedTerminal() { | ||
| let live = liveIndices | ||
| guard !live.isEmpty, focusedIndex < live.count else { return } | ||
| let si = live[focusedIndex].slot | ||
| slots[si].closing = true | ||
| if let s = slots[si].surface.surface { ghostty_surface_request_close(s) } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, read the current code at NiriCanvasView.swift around lines 213-218
cat -n Sources/NiriCanvasView.swift | sed -n '210,225p'Repository: manaflow-ai/cmux
Length of output: 659
🏁 Script executed:
# Search for hasLiveSurface in GhosttyTerminalView.swift
rg "hasLiveSurface" Sources/GhosttyTerminalView.swift -B 2 -A 2Repository: manaflow-ai/cmux
Length of output: 348
🏁 Script executed:
# Check the comments around lines 2831-2840 in GhosttyTerminalView.swift
cat -n Sources/GhosttyTerminalView.swift | sed -n '2825,2845p'Repository: manaflow-ai/cmux
Length of output: 1156
🏁 Script executed:
# Search for TerminalSurface class definition and related safety properties
ast-grep --pattern 'class TerminalSurface {
$$$
}'Repository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
# Check the slots property definition in NiriCanvasView.swift
rg "slots.*:" Sources/NiriCanvasView.swift | head -5Repository: manaflow-ai/cmux
Length of output: 330
🏁 Script executed:
# Search for TerminalSurface type definition to confirm it's what we're dealing with
rg "class TerminalSurface" Sources/GhosttyTerminalView.swift -A 5Repository: manaflow-ai/cmux
Length of output: 472
🏁 Script executed:
# Check how ghostty_surface_request_close is used elsewhere in the codebase
rg "ghostty_surface_request_close" --type swift -B 2 -A 2Repository: manaflow-ai/cmux
Length of output: 447
🏁 Script executed:
# Find the Slot definition in NiriCanvasView.swift
rg "struct Slot|class Slot" Sources/NiriCanvasView.swift -A 5Repository: manaflow-ai/cmux
Length of output: 297
Guard ghostty_surface_request_close with hasLiveSurface.
Line 218 passes a stale pointer to a Ghostty C API call. TerminalSurface explicitly documents at GhosttyTerminalView.swift:2834–2839 that hasLiveSurface must be checked before calling C APIs that dereference surface, since a non-nil pointer is insufficient when portal teardown or reparenting is in progress. The current code only checks if let s = slots[si].surface.surface, which omits the required portalLifecycleState check and can race close/reparent paths and crash.
Suggested fix
func closeFocusedTerminal() {
let live = liveIndices
guard !live.isEmpty, focusedIndex < live.count else { return }
let si = live[focusedIndex].slot
- slots[si].closing = true
- if let s = slots[si].surface.surface { ghostty_surface_request_close(s) }
+ let terminalSurface = slots[si].surface
+ slots[si].closing = true
+ if terminalSurface.hasLiveSurface, let s = terminalSurface.surface {
+ ghostty_surface_request_close(s)
+ }
if liveCount > 0 {
focusedIndex = min(focusedIndex, liveCount - 1)
focusCurrentTerminal()
}
}📝 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.
| func closeFocusedTerminal() { | |
| let live = liveIndices | |
| guard !live.isEmpty, focusedIndex < live.count else { return } | |
| let si = live[focusedIndex].slot | |
| slots[si].closing = true | |
| if let s = slots[si].surface.surface { ghostty_surface_request_close(s) } | |
| func closeFocusedTerminal() { | |
| let live = liveIndices | |
| guard !live.isEmpty, focusedIndex < live.count else { return } | |
| let si = live[focusedIndex].slot | |
| let terminalSurface = slots[si].surface | |
| slots[si].closing = true | |
| if terminalSurface.hasLiveSurface, let s = terminalSurface.surface { | |
| ghostty_surface_request_close(s) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/NiriCanvasView.swift` around lines 213 - 218, In
closeFocusedTerminal(), the code currently checks only if let s =
slots[si].surface.surface before calling ghostty_surface_request_close(s), which
can pass a stale pointer; change the guard so you check the TerminalSurface live
flag first (use the TerminalSurface.hasLiveSurface / portalLifecycleState check
on slots[si].surface) and only call ghostty_surface_request_close(s) when
hasLiveSurface is true and surface.surface is non-nil (i.e., ensure
slots[si].surface.hasLiveSurface && slots[si].surface.surface != nil before
invoking ghostty_surface_request_close).
| styleMask: [.titled, .closable, .resizable, .fullSizeContentView], | ||
| backing: .buffered, defer: false | ||
| ) | ||
| win.title = "Terminal Canvas" |
There was a problem hiding this comment.
Localize the window title.
Line 378 introduces a bare UI string. This should go through String(localized:..., defaultValue: ...), and the key should be added to Resources/Localizable.xcstrings.
Suggested fix
- win.title = "Terminal Canvas"
+ win.title = String(localized: "niriCanvas.windowTitle", defaultValue: "Terminal Canvas")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. Keys must go in Resources/Localizable.xcstrings with translations for all supported languages. Never use bare string literals in SwiftUI Text(), Button(), alert titles, or other UI elements".
📝 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.
| win.title = "Terminal Canvas" | |
| win.title = String(localized: "niriCanvas.windowTitle", defaultValue: "Terminal Canvas") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/NiriCanvasView.swift` at line 378, Replace the bare UI string
assigned to win.title ("Terminal Canvas") with a localized lookup using
String(localized: "window.terminalCanvas.title", defaultValue: "Terminal
Canvas") and add the key "window.terminalCanvas.title" with the English value
"Terminal Canvas" to Resources/Localizable.xcstrings (and translations for other
languages); update the assignment at the win.title site in NiriCanvasView (where
win.title is set) so the window title uses that localized string helper.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="scripts/demo-paperwm.swift">
<violation number="1" location="scripts/demo-paperwm.swift:248">
P2: Do not conditionally negate scroll deltas based on `isDirectionInvertedFromDevice`; doing so breaks macOS Natural Scrolling.</violation>
</file>
<file name="Sources/NiriCanvasView.swift">
<violation number="1" location="Sources/NiriCanvasView.swift:196">
P2: Clamp `targetOffset`/`scrollOffset` to the strip’s max scroll extent, not only zero, so repeated wheel input cannot scroll the entire panel strip out of view.</violation>
<violation number="2" location="Sources/NiriCanvasView.swift:281">
P2: Avoid adding an app-level display link; the project guidelines warn that display links can introduce typing lag. Consider driving these animations through Ghostty wakeups or a scoped timer that only runs while resizing/closing instead of a global CVDisplayLink.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| override func scrollWheel(with event: NSEvent) { | ||
| // Horizontal scroll (trackpad or shift+scroll) | ||
| var dx = event.scrollingDeltaX |
There was a problem hiding this comment.
P2: Do not conditionally negate scroll deltas based on isDirectionInvertedFromDevice; doing so breaks macOS Natural Scrolling.
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 248:
<comment>Do not conditionally negate scroll deltas based on `isDirectionInvertedFromDevice`; doing so breaks macOS Natural Scrolling.</comment>
<file context>
@@ -0,0 +1,368 @@
+
+ override func scrollWheel(with event: NSEvent) {
+ // Horizontal scroll (trackpad or shift+scroll)
+ var dx = event.scrollingDeltaX
+ if event.isDirectionInvertedFromDevice { dx = -dx }
+
</file context>
| } | ||
| // else: already fully visible, don't move | ||
| } | ||
| targetOffset = max(0, targetOffset) |
There was a problem hiding this comment.
P2: Clamp targetOffset/scrollOffset to the strip’s max scroll extent, not only zero, so repeated wheel input cannot scroll the entire panel strip out of view.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/NiriCanvasView.swift, line 196:
<comment>Clamp `targetOffset`/`scrollOffset` to the strip’s max scroll extent, not only zero, so repeated wheel input cannot scroll the entire panel strip out of view.</comment>
<file context>
@@ -0,0 +1,415 @@
+ }
+ // else: already fully visible, don't move
+ }
+ targetOffset = max(0, targetOffset)
+ }
+
</file context>
- 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. |
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="cmuxUITests/NiriCanvasUITests.swift">
<violation number="1" location="cmuxUITests/NiriCanvasUITests.swift:158">
P3: This adds substantial duplicate raw-mouse drag helper code that already exists in another UI test file; extract a shared test utility to avoid drift.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| return false | ||
| } | ||
|
|
||
| private struct RawMouseDragSession { |
There was a problem hiding this comment.
P3: This adds substantial duplicate raw-mouse drag helper code that already exists in another UI test file; extract a shared test utility to avoid drift.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cmuxUITests/NiriCanvasUITests.swift, line 158:
<comment>This adds substantial duplicate raw-mouse drag helper code that already exists in another UI test file; extract a shared test utility to avoid drift.</comment>
<file context>
@@ -0,0 +1,236 @@
+ return false
+ }
+
+ private struct RawMouseDragSession {
+ let source: CGEventSource
+ }
</file context>
This branch was successfully deployed
1 participant
Summary
Merges 1043 new commits from ghostty-org/ghostty into the manaflow-ai/ghostty fork, then updates the submodule pointer.
All cmux-specific patches preserved:
Testing
Summary by cubic
Syncs
ghosttyto the latest upstream (1043 commits), preserves all cmux patches, and adapts to upstream API changes. Adds a debug-only Niri/PaperWM-style terminal canvas with per-panel tabs and drag-and-drop.New Features
Upstream/API Changes
ghostty.hto upstream: IO mode/write callback, fullscreen enum updates, renderer health rename, new actions/functions; cmux-specific selection APIs retained.Written for commit 007569f. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Chores