Open
Conversation
|
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:
📝 WalkthroughWalkthroughRebases the ghostty fork onto upstream main (Mar 12, 2026), bumps the ghostty submodule and checksum, updates fork docs and conflict notes, restores copy-mode submodule pointer, changes macOS find/shortcut and focus handling to avoid redundant reposts, and adds related unit tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant App as AppDelegate
participant Ghost as GhosttyNSView
participant Menu as MainMenu
User->>App: Press Cmd‑F (key event)
App->>App: terminalFindShortcutAction(flags, chars, layout)
alt App maps & handles shortcut
App->>Ghost: startOrFocusTerminalSearch / trigger binding
Ghost-->>App: binding executed / focus acknowledged
App-->>User: return handled (event consumed)
else App does not handle
App->>Ghost: forward performKeyEquivalent
alt Ghost handles
Ghost-->>User: return handled
else
App->>Menu: route keyEquivalent to main menu
Menu-->>User: perform menu Find action
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Contributor
Greptile SummaryThis PR syncs the
Confidence Score: 5/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Developer runs\ndownload-prebuilt-ghosttykit.sh] --> B{GHOSTTY_SHA\nenv var set?}
B -- Yes --> C[Use provided SHA]
B -- No --> D[Read SHA from\ngit submodule HEAD]
C --> E[Construct TAG:\nxcframework-SHA]
D --> E
E --> F[Look up expected SHA-256\nin ghosttykit-checksums.txt]
F --> G{SHA found\nin checksums?}
G -- No --> H[Exit 1: Missing\npinned checksum]
G -- Yes --> I[Download GhosttyKit.xcframework.tar.gz\nfrom GitHub release TAG]
I --> J[Compute actual SHA-256\nof downloaded archive]
J --> K{Actual == Expected\nSHA-256?}
K -- No --> L[Exit 1: Checksum\nmismatch]
K -- Yes --> M[Extract archive to\nGhosttyKit.xcframework/]
M --> N[Done ✓]
Last reviewed commit: 4552e24 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@scripts/ghosttykit-checksums.txt`:
- Line 6: The parent repo points the ghostty submodule to commit
fbd49738d8f77fb494883747d34c93e7226f6352 but that commit and the corresponding
GhosttyKit release artifact are missing from the fork; push the missing commit
to the manaflow-ai/ghostty main branch, create a release containing
GhosttyKit.xcframework.tar.gz for that commit, upload the artifact and confirm
its checksum matches the entry in scripts/ghosttykit-checksums.txt
(fbd49738d8f77... -> checksum c9840531e5ef3...), then verify the submodule
pointer in the parent repo still references that pushed commit before
updating/merging the PR.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 875765e5-a5ee-4b07-839d-8cb614dfd4d5
📒 Files selected for processing (3)
docs/ghostty-fork.mdghosttyscripts/ghosttykit-checksums.txt
![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: cb65cb1582
ℹ️ 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".
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 `@cmuxTests/CmuxWebViewKeyEquivalentTests.swift`:
- Around line 3586-3592: The test reads NSApp.mainMenu before the shared
application is initialized; move or add initialization of NSApplication.shared
before accessing NSApp.mainMenu in
testCommandFFiresMenuShortcutBeforeGhosttyBinding so the call to NSApp.mainMenu
happens after the app is created (or call the same initialization used by
installMenu(spy:) earlier), ensuring installMenu(spy:) is not relied on to
create the shared app when reading NSApp.mainMenu.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ff6a7977-d5aa-4e27-9a53-cb630a39e469
📒 Files selected for processing (6)
Sources/AppDelegate.swiftSources/ContentView.swiftSources/GhosttyTerminalView.swiftSources/Workspace.swiftcmuxTests/CmuxWebViewKeyEquivalentTests.swiftcmuxTests/GhosttyConfigTests.swift
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a8cb03ae31
ℹ️ 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".
![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 6 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="cmuxTests/CmuxWebViewKeyEquivalentTests.swift">
<violation number="1" location="cmuxTests/CmuxWebViewKeyEquivalentTests.swift:3587">
P3: Initialize NSApplication.shared before accessing NSApp.mainMenu so the test doesn’t crash when NSApp is still nil.</violation>
</file>
<file name="Sources/AppDelegate.swift">
<violation number="1" location="Sources/AppDelegate.swift:1719">
P2: Gate keycode fallbacks to cases where no shortcut characters are available; otherwise non‑QWERTY Cmd shortcuts can be misrouted to Find actions.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
cmuxTests/CmuxWebViewKeyEquivalentTests.swift (1)
3613-3615:⚠️ Potential issue | 🟡 MinorInitialize
NSApplication.sharedbefore snapshottingNSApp.mainMenu.Line 3614 reads
NSApp.mainMenubeforeinstallMenu(spy:)creates the shared app, so this test stays brittle when run in isolation.🐛 Suggested fix
func testCommandFFiresMenuShortcutBeforeGhosttyBinding() { + _ = NSApplication.shared let previousMenu = NSApp.mainMenu defer { NSApp.mainMenu = previousMenu }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/CmuxWebViewKeyEquivalentTests.swift` around lines 3613 - 3615, The test reads NSApp.mainMenu before the shared application is created which makes it flaky; ensure the shared NSApplication is initialized before snapshotting the menu by invoking NSApplication.shared (or otherwise creating the app via installMenu(spy:)) prior to reading NSApp.mainMenu in testCommandFFiresMenuShortcutBeforeGhosttyBinding so the previousMenu capture happens after the app exists; update the test to call/install the shared app (e.g., via installMenu(spy:) or _ = NSApplication.shared) before assigning previousMenu.
🧹 Nitpick comments (2)
cmuxTests/CmuxWebViewKeyEquivalentTests.swift (2)
3672-3673: Assert the Ghostty path stays untouched once the menu consumes Cmd-F.Lines 3672-3673 only prove the menu action fired. A double-dispatch regression that also forwards Cmd-F into Ghostty would still pass. Reusing the terminal-side debug observer already used elsewhere in this file would make the “before Ghostty binding” part explicit.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/CmuxWebViewKeyEquivalentTests.swift` around lines 3672 - 3673, The test currently only asserts the app menu handler ran (XCTAssertTrue(surfaceView.performKeyEquivalent(with: event)) and spy.invocationCount == 1) but doesn't assert the Ghostty path wasn't also invoked; update the test to reuse the existing terminal-side debug observer used elsewhere in this file (the terminal debug observer instance) and assert it did not receive the Cmd-F dispatch (e.g., its invocation count or lastEvent remains unchanged) after calling surfaceView.performKeyEquivalent(with: event), ensuring Ghostty did not get the event once the menu consumed it.
3396-3421: Cover translatedCmd-G/Shift-Cmd-Ghere too.
terminalFindShortcutActioninSources/AppDelegate.swiftalso maps translatedgto.next/.previous, but this class only pins the translatedfpath. A regression in thegbranches would still slip through.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/CmuxWebViewKeyEquivalentTests.swift` around lines 3396 - 3421, Add tests to cover translated Cmd-G and Shift-Cmd-G in TerminalFindShortcutActionTests so regressions in the "g" branches of terminalFindShortcutAction are caught: extend the test class to assert that terminalFindShortcutAction(flags: [.command], chars: "k", keyCode: <use keyCode for cmd-g>, layoutCharacterProvider: { _, modifierFlags in modifierFlags.contains(.command) ? "g" : "k" }) returns .next, and assert that terminalFindShortcutAction(flags: [.command, .shift], chars: "k", keyCode: <use keyCode for shift-cmd-g>, layoutCharacterProvider: { _, modifierFlags in modifierFlags.contains(.command) ? "G" : "k" }) returns .previous; reference the existing TerminalFindShortcutActionTests and terminalFindShortcutAction to add these expectations alongside the existing translated-f test so both f and g translation paths are covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cmuxTests/CmuxWebViewKeyEquivalentTests.swift`:
- Around line 3613-3615: The test reads NSApp.mainMenu before the shared
application is created which makes it flaky; ensure the shared NSApplication is
initialized before snapshotting the menu by invoking NSApplication.shared (or
otherwise creating the app via installMenu(spy:)) prior to reading
NSApp.mainMenu in testCommandFFiresMenuShortcutBeforeGhosttyBinding so the
previousMenu capture happens after the app exists; update the test to
call/install the shared app (e.g., via installMenu(spy:) or _ =
NSApplication.shared) before assigning previousMenu.
---
Nitpick comments:
In `@cmuxTests/CmuxWebViewKeyEquivalentTests.swift`:
- Around line 3672-3673: The test currently only asserts the app menu handler
ran (XCTAssertTrue(surfaceView.performKeyEquivalent(with: event)) and
spy.invocationCount == 1) but doesn't assert the Ghostty path wasn't also
invoked; update the test to reuse the existing terminal-side debug observer used
elsewhere in this file (the terminal debug observer instance) and assert it did
not receive the Cmd-F dispatch (e.g., its invocation count or lastEvent remains
unchanged) after calling surfaceView.performKeyEquivalent(with: event), ensuring
Ghostty did not get the event once the menu consumed it.
- Around line 3396-3421: Add tests to cover translated Cmd-G and Shift-Cmd-G in
TerminalFindShortcutActionTests so regressions in the "g" branches of
terminalFindShortcutAction are caught: extend the test class to assert that
terminalFindShortcutAction(flags: [.command], chars: "k", keyCode: <use keyCode
for cmd-g>, layoutCharacterProvider: { _, modifierFlags in
modifierFlags.contains(.command) ? "g" : "k" }) returns .next, and assert that
terminalFindShortcutAction(flags: [.command, .shift], chars: "k", keyCode: <use
keyCode for shift-cmd-g>, layoutCharacterProvider: { _, modifierFlags in
modifierFlags.contains(.command) ? "G" : "k" }) returns .previous; reference the
existing TerminalFindShortcutActionTests and terminalFindShortcutAction to add
these expectations alongside the existing translated-f test so both f and g
translation paths are covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 866dc741-a3f4-42b3-99fd-3b8bbb2e2372
📒 Files selected for processing (2)
Sources/AppDelegate.swiftcmuxTests/CmuxWebViewKeyEquivalentTests.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/AppDelegate.swift
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cmuxTests/CmuxWebViewKeyEquivalentTests.swift (1)
3681-3683:⚠️ Potential issue | 🟡 MinorInitialize
NSApplication.sharedbefore readingNSApp.mainMenu.Line 3682 reads
NSApp.mainMenubefore this test guarantees the shared app exists, so it can still be order-dependent when run in isolation.Suggested fix
func testCommandFFiresMenuShortcutBeforeGhosttyBinding() { + _ = NSApplication.shared let previousMenu = NSApp.mainMenu defer { NSApp.mainMenu = previousMenu }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/CmuxWebViewKeyEquivalentTests.swift` around lines 3681 - 3683, The test testCommandFFiresMenuShortcutBeforeGhosttyBinding reads NSApp.mainMenu before ensuring the shared application exists; update the test to initialize the shared NSApplication first by referencing NSApplication.shared (or calling NSApplication.shared.activate if appropriate) before accessing NSApp.mainMenu, then store and restore the previous mainMenu as before; ensure you still use defer to reset NSApp.mainMenu and keep the rest of the test logic unchanged.
🤖 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 1692-1694: The current code calls
searchFocusNotifier(terminalSurface) whenever terminalSurface.searchState !=
nil, which reposts a .ghosttySearchFocus event; change the logic so that if
terminalSurface.searchState is already non-nil you do not call
searchFocusNotifier and instead just return true (i.e., make it a no-op), and
only invoke searchFocusNotifier when terminalSurface.searchState is nil (or
invert the condition) so repeated Cmd+F does not re-emit focus.
---
Duplicate comments:
In `@cmuxTests/CmuxWebViewKeyEquivalentTests.swift`:
- Around line 3681-3683: The test
testCommandFFiresMenuShortcutBeforeGhosttyBinding reads NSApp.mainMenu before
ensuring the shared application exists; update the test to initialize the shared
NSApplication first by referencing NSApplication.shared (or calling
NSApplication.shared.activate if appropriate) before accessing NSApp.mainMenu,
then store and restore the previous mainMenu as before; ensure you still use
defer to reset NSApp.mainMenu and keep the rest of the test logic unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2607ac1b-22cc-4583-8ca9-53f0cf29853a
📒 Files selected for processing (3)
Sources/AppDelegate.swiftSources/TabManager.swiftcmuxTests/CmuxWebViewKeyEquivalentTests.swift
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: df42f4ba45
ℹ️ 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".
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ghosttyfroma50579bd5ddec81c6244b9b349d4bf781f667cectofbd49738d8f77fb494883747d34c93e7226f6352, rebasing the cmux Ghostty patch stack onto current upstreammainGhosttyKitrelease checksum and refreshdocs/ghostty-fork.mdfor the new fork stateTesting
cd ghostty && zig build -Demit-xcframework=true -Dxcframework-target=universal -Doptimize=ReleaseFastGHOSTTYKIT_OUTPUT_DIR=/tmp/... GHOSTTYKIT_ARCHIVE_NAME=GhosttyKit.xcframework.tar.gz ./scripts/download-prebuilt-ghosttykit.sh./scripts/setup.sh./scripts/reload.sh --tag issue-824-sync-ghostty-fork-upstreamIssues
Summary by cubic
Sync our
ghosttyfork with upstreammainand pin the matchingGhosttyKitrelease. Fix macOS Find shortcut routing (menu-first with command-aware layouts and direct fallback), refocus the Find field on repeated Cmd-F without dismissing the overlay, and keep window chrome opaque; closes #824.Bug Fixes
Dependencies
ghosttysubmodule to current upstreammain.GhosttyKitxcframework checksum toscripts/ghosttykit-checksums.txt.Written for commit babb656. Summary will update on new commits.
Summary by CodeRabbit
Documentation
New Features
Bug Fixes
UI
Tests
Chores