Repository navigation
Select find text on repeated Cmd+F - #3314
Merged
Merged
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a selectAll-aware focus/selection flow for search fields, centralizes first-responder checks, avoids selection during IME composition, updates notification payloads and callers to include Changes
Sequence DiagramsequenceDiagram
participant User as User
participant TM as TabManager
participant NC as NotificationCenter
participant OV as SearchOverlay
participant ED as Editor
User->>TM: Press Cmd+F (start/focus)
TM->>NC: Post .browser/.ghosttySearchFocus (userInfo: selectAll)
NC->>OV: Deliver focus notification (contains selectAll)
OV->>OV: call cmuxTextFieldIsFirstResponder()
alt selectAll == true
OV->>ED: focusField(selectAll: true)
ED->>ED: if hasMarkedText() then skip selection
ED->>ED: else select all text
else
OV->>ED: focusField(selectAll: false)
ED->>ED: if not already first responder move caret to end
end
ED->>User: Update caret/selection in UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ 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. Review rate limit: 6/8 reviews remaining, refill in 13 minutes and 20 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmuxUITests/BrowserPaneNavigationKeybindUITests.swift`:
- Around line 964-966: After sending the Cmd+D split command (app.typeKey("d",
modifierFlags: [.command])) wait for the split to finish before calling
focusRightPaneForFindScenario(...); add a short synchronization step that polls
for the new pane/split UI (for example using an XCTAssertTrue(...
.waitForExistence(timeout:)) or a small helper like
waitForSplitCreation()/waitForSecondEditorSplit(timeout:)) targeting the right
pane or split divider, then proceed to call focusRightPaneForFindScenario(app,
route: .cmdOptionArrows).
In `@Sources/Find/BrowserSearchOverlay.swift`:
- Around line 225-230: The checks that detect whether a text field is already
focused (found in focusField(_:in:selectAll:), the notification handler around
line ~315, focusNativeTextField(_:in:), and the computed property
alreadyFocused) currently use unsafe casts like ((fr as? NSTextView)?.delegate
as? NSTextField) === field; replace those with the safe field-editor ownership
helper cmuxFieldEditorOwnerView(_:) by resolving the firstResponder as (fr as?
NSTextView).flatMap { cmuxFieldEditorOwnerView($0) } === field so you traverse
the responder chain safely and avoid dereferencing NSTextView.delegate; update
each occurrence accordingly to use cmuxFieldEditorOwnerView.
In `@Sources/TabManager.swift`:
- Around line 1926-1933: When selectAllExistingText is false (first-open path)
the code posts NotificationCenter.default.post(name: .ghosttySearchFocus,
object: panel.surface, userInfo: [FindFocusNotificationKey.selectAll:
selectAllExistingText]) before running
panel.performBindingAction("start_search"), which can cause the overlay to drop
the focus event; move or defer the .ghosttySearchFocus notification so it is
posted only after panel.performBindingAction("start_search") completes in the
branch where selectAllExistingText == false, while preserving the original
behavior when selectAllExistingText == true (i.e., still post immediately with
the same userInfo and object values).
🪄 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: 7cda5193-81c8-409f-98ac-ed75410dc24b
📒 Files selected for processing (6)
Sources/Find/BrowserSearchOverlay.swiftSources/Find/SurfaceSearchOverlay.swiftSources/GhosttyTerminalView.swiftSources/Panels/BrowserPanel.swiftSources/TabManager.swiftcmuxUITests/BrowserPaneNavigationKeybindUITests.swift
Comment on lines
+964
to
+966
| app.typeKey("d", modifierFlags: [.command]) | ||
| focusRightPaneForFindScenario(app, route: .cmdOptionArrows) | ||
|
|
There was a problem hiding this comment.
Stabilize split creation before the first pane-focus hop.
focusRightPaneForFindScenario(...) runs immediately after Cmd+D; adding a short split-complete wait here will reduce CI flake.
Suggested test hardening
app.typeKey("d", modifierFlags: [.command])
+ XCTAssertTrue(
+ waitForDataMatch(timeout: 6.0) { data in
+ guard data["lastSplitDirection"] == "right" else { return false }
+ guard let paneCountAfterSplit = Int(data["paneCountAfterSplit"] ?? "") else { return false }
+ return paneCountAfterSplit >= 2
+ },
+ "Expected Cmd+D split to complete before focusing the right pane. data=\(String(describing: loadData()))"
+ )
focusRightPaneForFindScenario(app, route: .cmdOptionArrows)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmuxUITests/BrowserPaneNavigationKeybindUITests.swift` around lines 964 -
966, After sending the Cmd+D split command (app.typeKey("d", modifierFlags:
[.command])) wait for the split to finish before calling
focusRightPaneForFindScenario(...); add a short synchronization step that polls
for the new pane/split UI (for example using an XCTAssertTrue(...
.waitForExistence(timeout:)) or a small helper like
waitForSplitCreation()/waitForSecondEditorSplit(timeout:)) targeting the right
pane or split divider, then proceed to call focusRightPaneForFindScenario(app,
route: .cmdOptionArrows).
lawrencecchen
force-pushed
the
task-cmd-f-select-find-text
branch
from
April 29, 2026 23:25
9368d56 to
3e2cd3c
Compare
lawrencecchen
force-pushed
the
task-cmd-f-select-find-text
branch
2 times, most recently
from
April 29, 2026 23:29
2fa2706 to
a2e4a12
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Sources/TabManager.swift (1)
1921-1938:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep the first-open focus notification after
start_search.The initial Cmd+F path still posts
.ghosttySearchFocusbefore the field is mounted, so the focus event can be dropped. This is the same race that was already called out on the previous revision.Suggested fix
- NotificationCenter.default.post( - name: .ghosttySearchFocus, - object: panel.surface, - userInfo: [FindFocusNotificationKey.selectAll: selectAllExistingText] - ) if !selectAllExistingText { _ = panel.performBindingAction("start_search") } + NotificationCenter.default.post( + name: .ghosttySearchFocus, + object: panel.surface, + userInfo: [FindFocusNotificationKey.selectAll: selectAllExistingText] + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TabManager.swift` around lines 1921 - 1938, The focus notification is being posted before the search field is mounted, so ensure the first-open focus notification is sent after starting the search; when selectAllExistingText is false call panel.performBindingAction("start_search") (or mount the search field) before posting NotificationCenter.default.post(name: .ghosttySearchFocus, ...), and retain the existing selectAllExistingText flag/logic around panel.searchState and TerminalSurface.SearchState so the notification still carries FindFocusNotificationKey.selectAll for the initial open.
🤖 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/TabManager.swift`:
- Around line 1921-1938: The focus notification is being posted before the
search field is mounted, so ensure the first-open focus notification is sent
after starting the search; when selectAllExistingText is false call
panel.performBindingAction("start_search") (or mount the search field) before
posting NotificationCenter.default.post(name: .ghosttySearchFocus, ...), and
retain the existing selectAllExistingText flag/logic around panel.searchState
and TerminalSurface.SearchState so the notification still carries
FindFocusNotificationKey.selectAll for the initial open.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3b6046e1-fa50-40b0-ac7c-eda1dc2e6302
📒 Files selected for processing (6)
GhosttyTabs.xcodeproj/project.pbxprojSources/Find/BrowserSearchOverlay.swiftSources/Find/SurfaceSearchOverlay.swiftSources/Panels/BrowserPanel.swiftSources/TabManager.swiftcmuxUITests/FindSelectionShortcutUITests.swift
✅ Files skipped from review due to trivial changes (1)
- GhosttyTabs.xcodeproj/project.pbxproj
🚧 Files skipped from review as they are similar to previous changes (2)
- Sources/Find/SurfaceSearchOverlay.swift
- Sources/Find/BrowserSearchOverlay.swift
![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: a2e4a1231d
ℹ️ 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".
Comment on lines
5040
to
+5041
| DispatchQueue.main.asyncAfter(deadline: .now() + 0.05) { [weak self] in | ||
| self?.postBrowserSearchFocusNotification(reason: "async50ms", generation: generation) | ||
| self?.postBrowserSearchFocusNotification(reason: "async50ms", generation: generation, selectAll: false) |
There was a problem hiding this comment.
The async50ms retry is explicitly there to handle cases where earlier focus notifications race with find-bar mount, but this call now hard-codes selectAll: false. In that race path, a repeated Cmd+F (where created == false) will focus the field without selecting existing text, so typing appends instead of replacing. This makes the new behavior flaky on slower/layout-churn paths where only the delayed retry is effective.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmuxUITests/FindSelectionShortcutUITests.swift (1)
8-15: 💤 Low valueConsider adding tearDown for temp file cleanup.
The test creates temp files but doesn't clean them up. While
/tmpfiles are eventually cleared by the OS, explicit cleanup intearDownis better practice and helps when running tests repeatedly during development.♻️ Suggested addition
override func tearDown() { try? FileManager.default.removeItem(atPath: dataPath) try? FileManager.default.removeItem(atPath: socketPath) super.tearDown() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxUITests/FindSelectionShortcutUITests.swift` around lines 8 - 15, Add a tearDown implementation to remove the temporary files created in setUp: in the test class implement override func tearDown() that calls FileManager.default.removeItem for both dataPath and socketPath (using try? to ignore errors) before calling super.tearDown(), ensuring the temporary JSON and socket files created in setUp are cleaned up after each test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmuxUITests/FindSelectionShortcutUITests.swift`:
- Around line 41-48: The test's Pane.needleKey values are wrong and must match
the app's keys; update the computed property needleKey in the Pane enum to
return "focusedTerminalFindNeedle" for .terminal and "focusedBrowserFindNeedle"
for .browser so the UI test reads the same JSON keys the app writes (refer to
Pane and its needleKey property).
---
Nitpick comments:
In `@cmuxUITests/FindSelectionShortcutUITests.swift`:
- Around line 8-15: Add a tearDown implementation to remove the temporary files
created in setUp: in the test class implement override func tearDown() that
calls FileManager.default.removeItem for both dataPath and socketPath (using
try? to ignore errors) before calling super.tearDown(), ensuring the temporary
JSON and socket files created in setUp are cleaned up after each test.
🪄 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: 355ab725-eadf-4a0d-af07-ca4a64d71182
📒 Files selected for processing (6)
GhosttyTabs.xcodeproj/project.pbxprojSources/Find/BrowserSearchOverlay.swiftSources/Find/SurfaceSearchOverlay.swiftSources/Panels/BrowserPanel.swiftSources/TabManager.swiftcmuxUITests/FindSelectionShortcutUITests.swift
✅ Files skipped from review due to trivial changes (1)
- GhosttyTabs.xcodeproj/project.pbxproj
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/Find/BrowserSearchOverlay.swift
Comment on lines
+41
to
+48
| private enum Pane { | ||
| case terminal | ||
| case browser | ||
|
|
||
| var focusKey: String { self == .terminal ? "terminal" : "browser" } | ||
| var needleKey: String { self == .terminal ? "terminalFindNeedle" : "browserFindNeedle" } | ||
| var replacementMessage: String { self == .terminal ? "terminal find text" : "browser find text" } | ||
| } |
There was a problem hiding this comment.
Incorrect JSON keys will cause test to always fail.
The needleKey values don't match the keys written by the app. Per the context snippet from Sources/AppDelegate.swift:8471-8492, the app writes "focusedTerminalFindNeedle" and "focusedBrowserFindNeedle", but the test looks for "terminalFindNeedle" and "browserFindNeedle".
🐛 Proposed fix
private enum Pane {
case terminal
case browser
var focusKey: String { self == .terminal ? "terminal" : "browser" }
- var needleKey: String { self == .terminal ? "terminalFindNeedle" : "browserFindNeedle" }
+ var needleKey: String { self == .terminal ? "focusedTerminalFindNeedle" : "focusedBrowserFindNeedle" }
var replacementMessage: String { self == .terminal ? "terminal find text" : "browser find text" }
}📝 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.
Suggested change
| private enum Pane { | |
| case terminal | |
| case browser | |
| var focusKey: String { self == .terminal ? "terminal" : "browser" } | |
| var needleKey: String { self == .terminal ? "terminalFindNeedle" : "browserFindNeedle" } | |
| var replacementMessage: String { self == .terminal ? "terminal find text" : "browser find text" } | |
| } | |
| private enum Pane { | |
| case terminal | |
| case browser | |
| var focusKey: String { self == .terminal ? "terminal" : "browser" } | |
| var needleKey: String { self == .terminal ? "focusedTerminalFindNeedle" : "focusedBrowserFindNeedle" } | |
| var replacementMessage: String { self == .terminal ? "terminal find text" : "browser find text" } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmuxUITests/FindSelectionShortcutUITests.swift` around lines 41 - 48, The
test's Pane.needleKey values are wrong and must match the app's keys; update the
computed property needleKey in the Pane enum to return
"focusedTerminalFindNeedle" for .terminal and "focusedBrowserFindNeedle" for
.browser so the UI test reads the same JSON keys the app writes (refer to Pane
and its needleKey property).
lawrencecchen
force-pushed
the
task-cmd-f-select-find-text
branch
from
April 29, 2026 23:36
a2e4a12 to
511ad54
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
Sources/TabManager.swift (1)
1923-1929:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFirst-open Cmd+F can still lose focus through
startOrFocusTerminalSearch.This now delegates the initial-open path to
startOrFocusTerminalSearch, but that helper only invokessearchFocusNotifierafterstart_searchifterminalSurface.searchStateis stillnilon the next runloop. Ifstart_searcheagerly creates the search state, the notifier never runs, so the first Cmd+F can open the find UI without focusing the field.Suggested fix in
Sources/App/ShortcutRoutingSupport.swiftif terminalSurface.performBindingAction("start_search") { DispatchQueue.main.async { [weak terminalSurface] in - guard let terminalSurface, terminalSurface.searchState == nil else { return } - terminalSurface.searchState = TerminalSurface.SearchState() + guard let terminalSurface else { return } + if terminalSurface.searchState == nil { + terminalSurface.searchState = TerminalSurface.SearchState() + } searchFocusNotifier(terminalSurface) } return true }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TabManager.swift` around lines 1923 - 1929, startOrFocusTerminalSearch can miss calling the search-focus notifier when start_search eagerly creates terminalSurface.searchState; update startOrFocusTerminalSearch so that when it triggers the initial-open path (i.e., start_search returns/creates a new search state or terminalSurface.searchState becomes non-nil immediately) it posts the .ghosttySearchFocus notification (using the same userInfo [FindFocusNotificationKey.selectAll: hadExistingSearch]) immediately instead of relying on a subsequent runloop check. Reference startOrFocusTerminalSearch, start_search, terminalSurface.searchState, .ghosttySearchFocus and FindFocusNotificationKey.selectAll when making the change.cmuxUITests/FindSelectionShortcutUITests.swift (1)
40-47:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winIncorrect JSON keys will cause test to always fail.
Per the context snippet from
Sources/AppDelegate.swift:8471-8520, the app writes both key sets:
"focusedTerminalFindNeedle"/"focusedBrowserFindNeedle"— for the currently focused panel"terminalFindNeedle"/"browserFindNeedle"— for any panel with find state (first found, regardless of focus)Since
assertFindReplacementfirst verifiesfocusedPanelKind == pane.focusKey(line 65), the test is checking the focused panel's find state. TheneedleKeyshould therefore use the"focused*"variants.🐛 Proposed fix
private enum Pane { case terminal case browser var focusKey: String { self == .terminal ? "terminal" : "browser" } - var needleKey: String { self == .terminal ? "terminalFindNeedle" : "browserFindNeedle" } + var needleKey: String { self == .terminal ? "focusedTerminalFindNeedle" : "focusedBrowserFindNeedle" } var replacementMessage: String { self == .terminal ? "terminal find text" : "browser find text" } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxUITests/FindSelectionShortcutUITests.swift` around lines 40 - 47, The Pane.enum's needleKey currently returns "terminalFindNeedle"/"browserFindNeedle" causing tests to read the non-focused keys; update Pane.needleKey to return the focused variants ("focusedTerminalFindNeedle" for .terminal and "focusedBrowserFindNeedle" for .browser) so assertFindReplacement (which checks focusedPanelKind == pane.focusKey) reads the correct focused find state; locate Pane in FindSelectionShortcutUITests.swift and change the needleKey string values accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cmuxUITests/FindSelectionShortcutUITests.swift`:
- Around line 40-47: The Pane.enum's needleKey currently returns
"terminalFindNeedle"/"browserFindNeedle" causing tests to read the non-focused
keys; update Pane.needleKey to return the focused variants
("focusedTerminalFindNeedle" for .terminal and "focusedBrowserFindNeedle" for
.browser) so assertFindReplacement (which checks focusedPanelKind ==
pane.focusKey) reads the correct focused find state; locate Pane in
FindSelectionShortcutUITests.swift and change the needleKey string values
accordingly.
In `@Sources/TabManager.swift`:
- Around line 1923-1929: startOrFocusTerminalSearch can miss calling the
search-focus notifier when start_search eagerly creates
terminalSurface.searchState; update startOrFocusTerminalSearch so that when it
triggers the initial-open path (i.e., start_search returns/creates a new search
state or terminalSurface.searchState becomes non-nil immediately) it posts the
.ghosttySearchFocus notification (using the same userInfo
[FindFocusNotificationKey.selectAll: hadExistingSearch]) immediately instead of
relying on a subsequent runloop check. Reference startOrFocusTerminalSearch,
start_search, terminalSurface.searchState, .ghosttySearchFocus and
FindFocusNotificationKey.selectAll when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 68b5b140-38bd-46d4-8835-bbf20e0b647f
📒 Files selected for processing (6)
GhosttyTabs.xcodeproj/project.pbxprojSources/Find/BrowserSearchOverlay.swiftSources/Find/SurfaceSearchOverlay.swiftSources/Panels/BrowserPanel.swiftSources/TabManager.swiftcmuxUITests/FindSelectionShortcutUITests.swift
✅ Files skipped from review due to trivial changes (1)
- GhosttyTabs.xcodeproj/project.pbxproj
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/Find/BrowserSearchOverlay.swift
lawrencecchen
force-pushed
the
task-cmd-f-select-find-text
branch
from
April 29, 2026 23:44
511ad54 to
bd983f5
Compare
ShubhamPatilsd
pushed a commit
to emergent-inc/mosaic
that referenced
this pull request
Jul 9, 2026
This branch was successfully deployed
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
Testing
Issues
Summary by cubic
Pressing Cmd+F again now selects the current find text in the terminal and in-app browser so you can replace it immediately. First open focuses the field; repeated Cmd+F selects all.
selectAllfocus signal and selects all even if already focused; first open focuses without selecting. IME composition is respected.selectAll; first open moves the caret to the end. IME-safe.FindSelectionShortcutUITeststo confirm repeated Cmd+F replaces text in both find fields.Written for commit bd983f5. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
Improvements
Tests