Skip to content

Fix find field arrow keys - #4804

Closed
lawrencecchen wants to merge 3 commits into
mainfrom
issue-cmd-f-arrow-keys
Closed

lawrencecchen wants to merge 3 commits into
mainfrom
issue-cmd-f-arrow-keys

Conversation

@lawrencecchen

@lawrencecchen lawrencecchen commented May 26, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Add XCUITest coverage that Cmd-F find fields keep in-field arrow-key editing while focused.
  • Add XCUITest coverage that a find-field caret at {2, 0} survives Cmd+Option pane navigation away and back, then inserts at the restored cursor position.
  • Route allowed find text-field arrow key equivalents to the focused field editor instead of AppKit menu handling.
  • Preserve find-field selection across focus transitions and map Up/Down to match navigation in terminal and browser find overlays.

Regression proof

Verification

  • git diff --check
  • ./scripts/lint-pbxproj-test-wiring.sh
  • ./scripts/reload.sh --tag findarr

Note

Medium Risk
Touches global key routing and find focus/selection state across terminal and browser; behavior is heavily covered by new UI tests but still affects a sensitive input path.

Overview
Fixes terminal and browser find fields so arrow keys edit the query and navigate matches instead of being swallowed by global shortcut routing.

Keyboard routing: Adds shouldDispatchFindTextFieldArrowViaFirstResponderKeyDown and forwards allowed arrow keyDown events to the find field editor in AppDelegate (with reentry guard), matching omnibar/command-palette behavior.

Find overlays: Terminal and browser search delegates map Up/Down to previous/next match (like Return/Shift+Return). Focus logic prefers stored selection when refocusing.

Selection persistence: FindTextFieldSupport and cmuxApplyFindFocusSelection keep caret/selection across pane switches (Cmd+Option), repeated Cmd+F, and focus transitions—ignoring transient {0,0} resets during editing.

Tests: Expands FindSelectionShortcutUITests for in-field arrow editing, caret/edge ranges after pane navigation, browser Up/Down match navigation, and more stable launch handling.

Reviewed by Cursor Bugbot for commit 9d28b92. Bugbot is set up for automated code reviews on this repo. Configure here.

@vercel

vercel Bot commented May 26, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
cmux Ready Ready Preview, Comment May 27, 2026 11:30am
cmux-staging Building Building Preview, Comment May 27, 2026 11:30am

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

1 similar comment
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

@coderabbitai

coderabbitai Bot commented May 26, 2026 •

Copy link
Copy Markdown

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds a predicate to gate arrow-key dispatch for focused Find text fields, a recursion guard and forwarding in window-level key handling, explicit moveUp/moveDown handlers in find overlays, and UI tests plus helpers for focused-arrow editing and caret/navigation behavior.

Changes

Find Text Field Arrow-Key Navigation

Layer / File(s) Summary
Find text field arrow-key validation predicate
Sources/App/ShortcutRoutingSupport.swift
New shouldDispatchFindTextFieldArrowViaFirstResponderKeyDown predicate validates arrow-key events (keycodes 123–126), checks focus and IME marked-text, and normalizes modifier flags to allow only specific combinations (none, Shift, Option, Option+Shift, Command, Command+Shift).
Arrow-key dispatch routing and loop prevention
Sources/AppDelegate.swift
Adds cmuxFindTextFieldArrowForwardingDepth recursion guard and detects Find-field first responders; when validation passes and depth is zero forwards the event to firstResponder?.keyDown(with:) while preventing re-entrant forwarding.
Find overlay moveUp/moveDown handlers
Sources/Find/BrowserSearchOverlay.swift, Sources/Find/SurfaceSearchOverlay.swift
Both overlays now intercept .moveDown and .moveUp: skip handling during IME marked-text, remember current selection, invoke onReturn(false/true) to navigate matches, and return handled.
UI tests, helpers, and launch config
cmuxUITests/FindSelectionShortcutUITests.swift
Adds tests for arrow-key editing while find fields are focused, caret preservation across Cmd+Option pane navigation, and browser-find Up/Down navigation. Adds helpers: per-run launchTag, configuredApp(), makeBrowserFindMatchesURL, assertFindArrowEditing, assertFindCaretSurvivesCmdOptionNavigation, and refactors navigation helpers.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related issues

  • manaflow-ai/cmux-dev-artifacts#1825: The changes implement guarded arrow-key forwarding and caret/focus restoration behavior referenced by the failing UI test described in that issue.

"🐰
In a hop of keys and tiny paws,
Arrows find their gentle laws,
IME sleeps while matches turn,
Carets return where fingers yearn,
Tests cheer — the search field claws."


Caution

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

  • Ignore

❌ Failed checks (1 error, 1 warning)

Check name Status Explanation Resolution
Cmux Swiftui State Layout ❌ Error PR introduces new @ObservedObject state in main View structs (BrowserSearchOverlay, SurfaceSearchOverlay) instead of modern @Observable pattern per swiftui-state-layout.md rules. Use @Observable plus @State for BrowserSearchState and TerminalSurface.SearchState in the find overlay Views, or pass value snapshots and closures instead.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (15 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Fix find field arrow keys' directly and clearly summarizes the main change: routing find text-field arrow keys to the focused field instead of AppKit menu handling.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Cmux Swift Actor Isolation ✅ Passed PR introduces pure utility functions and UI coordinator methods; no new shared mutable Sendable types or MainActor violations created. Uses pre-existing file-scoped depth counters appropriately.
Cmux Swift Blocking Runtime ✅ Passed No blocking synchronization detected in production code changes. No sleep, semaphores, sync dispatch, or timers. Re-entry guard counter matches existing patterns.
Cmux No Hacky Sleeps ✅ Passed PR adds Resources/feed-tui/index.ts with setTimeout (socket timeout) and setInterval (UI refresh), both legitimate per rule's exceptions for dedicated timeout abstractions and presentation timing.
Cmux Swift Concurrency ✅ Passed PR adds arrow-key routing for find text fields with no legacy async patterns: predicate function, recursion-depth guard, delegate case statements, and tests only.
Cmux Swift @Concurrent ✅ Passed No new async, @concurrent, or nonisolated async declarations. All changes are synchronous predicates and routing; existing DispatchQueue patterns are for required AppKit/SwiftUI boundaries.
Cmux Swift File And Package Boundaries ✅ Passed Focused bug fix adding 21, 17, 11, and 11 lines to existing files for find arrow key routing. Small additions follow established patterns and preserve extraction paths.
Cmux Swift Logging ✅ Passed All production code changes contain no unguarded print/debugPrint/dump/NSLog or inappropriate logging that violates swift-logging.md rules.
Cmux User-Facing Error Privacy ✅ Passed PR adds find field arrow key handling logic without user-facing error messages, alerts, or implementation detail leaks. All production code contains zero user-visible strings violating privacy rules.
Cmux Full Internationalization ✅ Passed PR contains no user-facing strings and no xcstrings modifications; changes are technical implementation (arrow key routing) and test assertions only.
Cmux Architecture Rethink ✅ Passed PR uses depth counter pattern matching pre-existing codebase patterns; centralizes find state via existing NSMapTable; no new state owners; clear re-entry prevention invariant.
Cmux Swift Auxiliary Window Close Shortcuts ✅ Passed PR adds only keyboard routing functions and find field command handling in existing overlays, not new window entities.
Description check ✅ Passed The pull request description comprehensively covers all required template sections: Summary explains what changed and why, Testing describes verification methods, and Checklist items are addressed through CI/script validation.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch issue-cmd-f-arrow-keys

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@greptile-apps

greptile-apps Bot commented May 26, 2026 •

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes arrow-key routing and caret/selection preservation for the terminal and browser find bars. It routes allowed arrow keyDown events directly to the focused find field editor (preventing AppKit menu interception), maps Up/Down to previous/next match navigation, and tightens selection bookkeeping so stored find selections survive Cmd+Option pane switches and repeated Cmd+F without being clobbered by AppKit's spurious {0,0} notifications during focus transitions.

  • Arrow key routing (ShortcutRoutingSupport.swift, AppDelegate.swift): new shouldDispatchFindTextFieldArrowViaFirstResponderKeyDown and corresponding depth-guarded forwarding block follow the identical pattern used for the command palette, browser omnibar, and text-box inputs.
  • Match navigation (BrowserSearchOverlay.swift, SurfaceSearchOverlay.swift): moveDown/moveUp command-selector cases now call onReturn(false/true), making Down/Up step through find matches like Return/Shift+Return.
  • Selection preservation (FindTextFieldSupport.swift): a transient cmuxFocusTransitionRememberedSelection cache suppresses AppKit's spurious {0,0} selection notifications during textDidBeginEditing; preservingStoredFocusTransitionSelection in textDidEndEditing prevents {0,0} from overwriting a valid stored non-zero range.

Confidence Score: 5/5

Safe to merge — changes are localized to find-field key routing and selection bookkeeping, follow established patterns, and are covered by new XCUITests that include a regression proof commit.

All production changes follow the existing depth-counter and selection-tracking patterns in the codebase. The {0,0} guard in cmuxRestoreRememberedSelection relies on AppKit's default refocus behavior, which is exercised directly by the new "caret at beginning" XCUITest case. No actor-isolation mistakes, no blocking primitives, no untranslated strings, and no user-facing error copy are introduced.

FindTextFieldSupport.swift — the {0,0} sentinel assumption spans cmuxRestoreRememberedSelection, cmuxFocusTransitionRememberedSelection, and preservingStoredFocusTransitionSelection; a comment naming the invariant would help future maintainers.

Important Files Changed

Filename Overview
Sources/App/ShortcutRoutingSupport.swift New shouldDispatchFindTextFieldArrowViaFirstResponderKeyDown added — exact structural match to the existing shouldDispatchTextBoxInputArrowViaFirstResponderKeyDown, allowing all four arrow keys with the same modifier allowlist. Clean, well-precedented addition.
Sources/AppDelegate.swift New cmuxFindTextFieldArrowForwardingDepth reentrancy guard and routing block inserted before the browser-omnibar block, following the identical depth-counter pattern used for command palette, text-box, and browser arrow forwarding. No ordering or reentrancy concerns.
Sources/Find/BrowserSearchOverlay.swift Priority order of rememberedRange flipped so stored find selection wins over field's cmuxLastSelectedRange; new moveDown/moveUp command-selector cases correctly route to onReturn(false/true) for next/previous match, mirroring Return/Shift+Return behavior.
Sources/Find/SurfaceSearchOverlay.swift Same two changes as BrowserSearchOverlay: priority-order flip and new moveDown/moveUp delegate cases for match navigation. Changes are symmetric and consistent.
Sources/Find/FindTextFieldSupport.swift Adds cmuxFocusTransitionRememberedSelection transient cache to suppress spurious {0,0} notifications from AppKit during focus transitions, and preservingStoredFocusTransitionSelection flag to cmuxRememberSelectionFromCurrentEditor for the same purpose in textDidEndEditing. The {0,0} range is implicitly treated as a sentinel meaning "no real selection" across multiple guards but this assumption is not documented inline.
cmuxUITests/FindSelectionShortcutUITests.swift Six new XCUITest functions covering arrow editing, caret survival through Cmd+Option navigation, rapid navigation churn, selection-edge ranges, and browser Up/Down match stepping. Test-only scaffolding uses RunLoop.current.run(until:) which is appropriate for XCUITest.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[NSWindow receives keyDown event] --> B{First responder\nis find text field?}
    B -- No --> C[Existing routing:\nbrowser omnibar / cmd palette / text box]
    B -- Yes --> D{Arrow key 123-126\nwith allowed modifiers?\nNo marked text?}
    D -- No --> C
    D -- Yes --> E{cmuxFindTextFieldArrowForwardingDepth > 0?}
    E -- Yes: re-entrant --> F[Return false — bubble normally]
    E -- No --> G[depth += 1\nfirstResponder?.keyDown with event\ndepth -= 1\nReturn true — consumed]
    G --> H{NSTextView delegate\ntextView:doCommandBy:}
    H -- moveDown --> I[rememberSelection\nonReturn false = next match]
    H -- moveUp --> J[rememberSelection\nonReturn true = prev match]
    H -- Left/Right/Cmd+Left/etc --> K[Text field handles\ncaret / selection editing]
    K --> L[cmuxDidObserveSelectionChange fires\nGuard: skip if selection == 0,0\nAND cmuxFocusTransitionRememberedSelection != nil AND != 0,0]
    L --> M[cmuxRememberSelection stored]
Loading

Reviews (15): Last reviewed commit: "test: cover find field selection edge ra..." | Re-trigger Greptile

@lawrencecchen lawrencecchen added the stale-revisit Closed after 30+ days without activity; preserved for possible revisit or reopening. label Sep 23, 2026
@github-project-automation github-project-automation Bot moved this from Todo to Done in cmux backlog Sep 23, 2026

This branch was successfully deployed

1 active deployment
Preview – cmux — 9d28b926 Deployed May 27, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stale-revisit Closed after 30+ days without activity; preserved for possible revisit or reopening.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants