Repository navigation
Conversation
…ction SwiftUI's internal focus system enters a broken state after a browser panel (WKWebView) has been in the responder chain, where its hosting view intercepts key events via performKeyEquivalent, returns true, but never fires the action — silently swallowing arrow keys. - Route non-Command keys directly to CmuxWebView.performKeyEquivalent after app-level shortcuts have been checked, bypassing the broken SwiftUI content view hierarchy walk. - Clear stale browserAddressBarFocusedPanelId when the first responder is inside a WKWebView, not only when it is a terminal surface. - Remove the separate hasMarkedText() branch in the terminal path; GhosttyNSView.performKeyEquivalent already returns false during IME composition, so the unified direct-routing path handles both normal typing and IME candidate navigation correctly. Fixes manaflow-ai#1146
|
@jt-hsiao is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughModified AppDelegate.swift to improve key-event routing and focus detection for web views. Added a helper function to detect when the first responder is inside a WKWebView, and updated key-handling logic to route non-Command keys directly to the appropriate component (terminal or web view) based on responder location. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
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 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 Tip You can customize the tone of the review comments and chat replies.Configure the |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
@codex review |
@jt-hsiao I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
🧠 Learnings used✅ Actions performedReview triggered.
|
![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 `@Sources/AppDelegate.swift`:
- Around line 11220-11226: firstResponderWebView's handling should only
short-circuit when it actually handled the key event; change the logic around
firstResponderWebView and webView.performKeyEquivalent(with:) so you only return
the result when it's true, otherwise allow the method to fall through and let
NSWindow.performKeyEquivalent (and BrowserPopupPanel.performKeyEquivalent) run;
locate the block referencing firstResponderWebView and
WKWebView.performKeyEquivalent(with:) and adjust the conditional to return only
on true so menu/window handlers still get a chance on false.
| if let webView = firstResponderWebView { | ||
| let result = webView.performKeyEquivalent(with: event) | ||
| #if DEBUG | ||
| dlog(" → webview direct: \(result)") | ||
| #endif | ||
| return result | ||
| } |
There was a problem hiding this comment.
Fall through when WebKit declines a Command equivalent.
Line 11225 returns result unconditionally. If WKWebView.performKeyEquivalent(with:) returns false for a Cmd shortcut it does not own, the original NSWindow.performKeyEquivalent(with:) never runs, so window/menu handlers lose their chance. That can bypass repo-specific handlers like browser-popup Cmd+W, and native fallbacks such as window cycling.
💡 Suggested fix
if let webView = firstResponderWebView {
let result = webView.performKeyEquivalent(with: event)
`#if` DEBUG
dlog(" → webview direct: \(result)")
`#endif`
- return result
+ if result || !event.modifierFlags
+ .intersection(.deviceIndependentFlagsMask)
+ .contains(.command) {
+ return result
+ }
}Based on learnings: BrowserPopupPanel.performKeyEquivalent is the primary handler to close popups.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/AppDelegate.swift` around lines 11220 - 11226,
firstResponderWebView's handling should only short-circuit when it actually
handled the key event; change the logic around firstResponderWebView and
webView.performKeyEquivalent(with:) so you only return the result when it's
true, otherwise allow the method to fall through and let
NSWindow.performKeyEquivalent (and BrowserPopupPanel.performKeyEquivalent) run;
locate the block referencing firstResponderWebView and
WKWebView.performKeyEquivalent(with:) and adjust the conditional to return only
on true so menu/window handlers still get a chance on false.
There was a problem hiding this comment.
This is a false positive — CmuxWebView.performKeyEquivalent already handles Command key routing internally:
shouldRouteCommandEquivalentDirectlyToMainMenu→ triesNSApp.mainMenu.performKeyEquivalent(line 155)- Falls back to
handleBrowserSurfaceKeyEquivalent(line 164) - Falls through to
super.performKeyEquivalent(WKWebView, line 171)
All menu items and app shortcuts are resolved inside the CmuxWebView override before it returns. If it still returns false, it means no handler claimed the event — the same outcome the original NSWindow.performKeyEquivalent walk would produce, except the original walk also risks hitting the broken SwiftUI hosting view path (which is exactly the bug this PR fixes).
The only Command key where shouldRouteCommandEquivalentDirectlyToMainMenu returns false is Cmd+` (keyCode 50), which is intentionally excluded (confirmed by existing test testCmdBacktickDoesNotRouteDirectlyToMainMenuWhenWebViewIsFirstResponder). In that case, super.performKeyEquivalent returns false, and our code correctly propagates that — same as the old path.
There was a problem hiding this comment.
🧩 Analysis chain
@coderabbit-user Thanks for the added context. I’ll verify your claim against the repo and get back with a short confirmation or tweak if anything looks off.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Find CmuxWebView and performKeyEquivalent overrides =="
rg -nP --no-ignore-vcs -C3 '(class|extension)\s+CmuxWebView\b' --glob '**/*.swift' || true
rg -n --no-ignore-vcs -C5 'performKeyEquivalent\s*\(\s*with:' --glob '**/*.swift' || true
echo
echo "== Search for handleBrowserSurfaceKeyEquivalent =="
rg -n --no-ignore-vcs -C3 '\bhandleBrowserSurfaceKeyEquivalent\b' --glob '**/*.swift' || true
echo
echo "== Search BrowserPopupPanel and its performKeyEquivalent =="
rg -nP --no-ignore-vcs -C4 '(class|extension)\s+BrowserPopupPanel\b' --glob '**/*.swift' || true
rg -n --no-ignore-vcs -C5 'BrowserPopupPanel.*performKeyEquivalent|extension\s+BrowserPopupPanel|performKeyEquivalent\s*\(\s*with:' Sources/Panels/**/*.swift || true
echo
echo "== Find string identifier used for browser popups to cross-check learning =="
rg -n --no-ignore-vcs -C2 'cmux\.browser-popup' --glob '**/*.swift' || true
Length of output: 26926
@coderabbit-user Agreed — this was a false positive. I verified the routing:
-
In NSWindow.cmux_performKeyEquivalent, we first give the app-level handler a chance: handleBrowserSurfaceKeyEquivalent (Sources/AppDelegate.swift:11208–11213). Only then do we call firstResponderWebView.performKeyEquivalent and return its result (Lines 11220–11226).
-
CmuxWebView.performKeyEquivalent itself handles Command routing:
- If shouldRouteCommandEquivalentDirectlyToMainMenu(event) is true, it tries NSApp.mainMenu.performKeyEquivalent first (Sources/Panels/CmuxWebView.swift:155–160).
- Then falls back to AppDelegate.shared?.handleBrowserSurfaceKeyEquivalent (Lines 164–168).
- Then to super.performKeyEquivalent (Lines 171–175).
- For non-Command, it directly calls super (Lines 139–144).
-
Tests cover this: Cmd+N/W/R are consumed via the menu when WebView is first responder, while Return / Cmd+Return are not, and Cmd+` is explicitly excluded from direct-menu routing (cmuxTests/CmuxWebViewKeyEquivalentTests.swift:184–209, 220–246, 814–820).
Given this, allowing fall-through to the original NSWindow.performKeyEquivalent when the WebView returns false would just re-enter the SwiftUI hierarchy we’re intentionally bypassing for this bug, without adding any valid handler opportunity. Also, there’s no BrowserPopupPanel or “cmux.browser-popup” identifier in this repo, so that prior guideline doesn’t apply here.
I’m retracting the concern; no change needed.
✏️ Learnings added
Learnt from: jt-hsiao
Repo: manaflow-ai/cmux PR: 1423
File: Sources/AppDelegate.swift:11220-11226
Timestamp: 2026-03-14T07:05:52.379Z
Learning: Repo: manaflow-ai/cmux — In Sources/Panels/CmuxWebView.swift, performKeyEquivalent(with:) handles Command-key routing end-to-end: (1) if allowed, route to NSApp.mainMenu.performKeyEquivalent; (2) fall back to AppDelegate.shared?.handleBrowserSurfaceKeyEquivalent(event) for non–menu-backed app shortcuts; (3) fall back to super.performKeyEquivalent. For non-Command keys it calls super directly. Therefore, in NSWindow.cmux_performKeyEquivalent (Sources/AppDelegate.swift), it is correct to call firstResponderWebView.performKeyEquivalent and return its Bool unconditionally to avoid re-entering SwiftUI’s performKeyEquivalent path that can swallow keys after WKWebView focus.
Learnt from: jt-hsiao
Repo: manaflow-ai/cmux PR: 1423
File: Sources/AppDelegate.swift:11220-11226
Timestamp: 2026-03-14T07:05:52.379Z
Learning: Repo: manaflow-ai/cmux — Command-backtick (Cmd+`, keyCode 50) is intentionally excluded from direct menu routing by shouldRouteCommandEquivalentDirectlyToMainMenu; tests assert this behavior. Do not add a window/main-menu bypass for Cmd+`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: SuperManfred
Repo: manaflow-ai/cmux PR: 1150
File: Sources/AppDelegate.swift:7394-7401
Timestamp: 2026-03-10T10:24:14.017Z
Learning: Repo: manaflow-ai/cmux — For Cmd+W behavior with browser popups: BrowserPopupPanel.performKeyEquivalent is the primary handler to close popups. AppDelegate.handleCustomShortcut(_:), in its Cmd+W fallback, must check both NSApp.keyWindow and event.window for identifier "cmux.browser-popup" and close it if found, before routing to workspace/settings close logic.
Learnt from: CR
Repo: manaflow-ai/cmux PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-12T21:54:38.291Z
Learning: Applies to Sources/**/*TerminalWindowPortal.swift : In `WindowTerminalHostView.hitTest()` in `TerminalWindowPortal.swift`: all divider/sidebar/drag routing must be gated to pointer events only. Do not add work outside the `isPointerEvent` guard to avoid typing latency.
Learnt from: SuperManfred
Repo: manaflow-ai/cmux PR: 803
File: Sources/Panels/BrowserPopupWindowController.swift:36-38
Timestamp: 2026-03-04T05:11:56.373Z
Learning: In Sources/Panels/BrowserPopupWindowController.swift and Sources/Panels/BrowserPanel.swift, `webView.isInspectable = true` (guarded by `#available(macOS 13.3, *)`) is intentionally enabled in all builds — not just DEBUG — because cmux is a developer tool and full Web Inspector access is desired in production builds as well. Do not flag this as a security concern.
Learnt from: CR
Repo: manaflow-ai/cmux PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-12T21:54:38.291Z
Learning: Applies to Sources/**/*.swift : `SurfaceSearchOverlay` must be mounted from `GhosttySurfaceScrollView` in `Sources/GhosttyTerminalView.swift` (AppKit portal layer), not from SwiftUI panel containers such as `Sources/Panels/TerminalPanelView.swift`. Portal-hosted terminal views can sit above SwiftUI during split/workspace churn.
Learnt from: moyashin63
Repo: manaflow-ai/cmux PR: 1074
File: Sources/AppDelegate.swift:7523-7545
Timestamp: 2026-03-09T01:38:24.337Z
Learning: When the command palette is visible (as in manaflow-ai/cmux Sources/AppDelegate.swift), ensure the shortcut handling consumes most Command shortcuts to protect the palette's text input. Specifically, do not allow UI zoom shortcuts (Cmd+Shift+= / Cmd+Shift+− / Cmd+Shift+0) to trigger while the palette is open. Do not reorder shortcut handlers (e.g., uiZoomShortcutAction(...)) to bypass this guard; users must close the palette before performing zoom actions. This guideline should apply to Swift source files handling global shortcuts within the app.
Learnt from: CR
Repo: manaflow-ai/cmux PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-05T03:20:48.079Z
Learning: Applies to **/*TerminalView.swift : Terminal find layering contract: SurfaceSearchOverlay must be mounted from GhosttySurfaceScrollView in Sources/GhosttyTerminalView.swift (AppKit portal layer), not from SwiftUI panel containers
Learnt from: CR
Repo: manaflow-ai/cmux PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-05T03:20:48.079Z
Learning: Applies to **/*.swift : Socket/CLI commands must not steal macOS app focus; only explicit focus-intent commands may mutate in-app focus/selection
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 762
File: Sources/ContentView.swift:5784-5788
Timestamp: 2026-03-06T02:00:30.333Z
Learning: In manaflow-ai/cmux (Sources/ContentView.swift), ShortcutHintModifierPolicy.shouldShowHints is Command-only: it returns true only when modifierFlags normalized to [.command]. Control-hold must not show sidebar/titlebar shortcut hints, and close-button suppression in TabItemView must remain tied to showsModifierShortcutHints (Command hold) or the always-show debug flag.
Learnt from: homanp
Repo: manaflow-ai/cmux PR: 883
File: CLI/cmux.swift:1569-1586
Timestamp: 2026-03-04T22:05:07.913Z
Learning: In manaflow-ai/cmux CLI (CLI/cmux.swift), for parity with existing browser commands, markdown.open intentionally normalizes --surface before resolving --workspace/--window, relying on server-side resolution to disambiguate final routing. Avoid one-off reordering in markdown; consider any change only as a coordinated, cross-command refactor.
Learnt from: atani
Repo: manaflow-ai/cmux PR: 819
File: Sources/ContentView.swift:3194-3196
Timestamp: 2026-03-04T14:05:48.668Z
Learning: In manaflow-ai/cmux (PR `#819`), Sources/ContentView.swift: The command palette’s external window labels intentionally use the global window index from the full orderedSummaries (index + 1), matching the Window menu in AppDelegate. Do not reindex after filtering out the current window to avoid mismatches (“Window 2” for an external window is expected).
Learnt from: debgotwired
Repo: manaflow-ai/cmux PR: 1149
File: Sources/ContentView.swift:3977-3978
Timestamp: 2026-03-10T09:33:37.952Z
Learning: In manaflow-ai/cmux (Sources/ContentView.swift), scheduleCommandPaletteResultsRefresh(forceSearchCorpusRefresh:) is shared command‑palette infrastructure across all submenus. Do not change its sync‑seeding behavior within feature‑scoped PRs; treat brief initial flashes as consistent with existing submenus. Any UX improvement (e.g., synchronous seeding on forced corpus refresh) should be implemented and tested globally in a dedicated follow‑up PR.
Learnt from: apollow
Repo: manaflow-ai/cmux PR: 1089
File: Sources/TerminalController.swift:3180-3193
Timestamp: 2026-03-09T02:08:14.574Z
Learning: Repo: manaflow-ai/cmux — In Sources/TerminalController.swift, v2WorkspaceClearTags(params:) must only clear all tags when the "source" key is absent. If "source" is present but blank or non-string (v2String(...) returns nil), the API should return invalid_params. Current implementation uses hasSourceKey = params.keys.contains("source") and guards with if hasSourceKey && source == nil { return .err(...)}.
Learnt from: 0xble
Repo: manaflow-ai/cmux PR: 0
File: :0-0
Timestamp: 2026-03-09T21:53:05.451Z
Learning: Repo: manaflow-ai/cmux — For browser portals, visibility resync is centralized in BrowserWindowPortalRegistry.updateEntryVisibleInUI(for: WKWebView, visibleInUI: Bool), which immediately synchronizes the WKWebView on visibility changes. Call sites (e.g., Workspace.reconcilePanelPortalVisibilityForCurrentLayout()) should use this helper instead of ad‑hoc resync logic.
Learnt from: mdsakalu
Repo: manaflow-ai/cmux PR: 510
File: Sources/GhosttyTerminalView.swift:2276-2336
Timestamp: 2026-03-11T04:48:32.420Z
Learning: In cmux Sources/GhosttyTerminalView.swift, when implementing initial-present recovery for Ghostty terminals, avoid ghostty_surface_draw loops. Re-read self.surface after view.forceRefreshSurface() (since the surface may be torn down/reparented), then use occlusion toggle (ghostty_surface_set_occlusion false/true) plus ghostty_surface_refresh to wake the renderer. Cancel retries once the layer has contents or the retry budget is exhausted.
Learnt from: atani
Repo: manaflow-ai/cmux PR: 819
File: Sources/AppDelegate.swift:3491-3493
Timestamp: 2026-03-04T14:06:12.296Z
Learning: In PRs affecting this repository, limit the scope of localization (i18n) changes to Japanese translations for the file Sources/AppDelegate.swift. Do not include UX enhancements (e.g., preferring workspace.customTitle in workspaceDisplayName() or altering move-target labels) in this PR. Open a separate follow-up issue to address any UX-related changes to avoid scope creep and keep localization review focused.
Learnt from: atani
Repo: manaflow-ai/cmux PR: 819
File: Sources/AppDelegate.swift:0-0
Timestamp: 2026-03-04T14:05:42.574Z
Learning: Guideline: In Swift files (cmux project), when handling pluralized strings, prefer using localization keys with the ICU-style plural forms .one and .other. For example, use keys like statusMenu.unreadCount.one for the singular case (1) and statusMenu.unreadCount.other for all other counts, and similarly for statusMenu.tooltip.unread.one/other. Rationale: ensures correct pluralization across locales and makes localization keys explicit. Review code to ensure any unread count strings and related tooltips follow this .one/.other key pattern and verify the correct value is chosen based on the count.
Learnt from: austinywang
Repo: manaflow-ai/cmux PR: 954
File: Sources/TerminalController.swift:0-0
Timestamp: 2026-03-05T22:04:34.712Z
Learning: Adopt the convention: for health/telemetry tri-state values in Swift, prefer Optionals (Bool?) over sentinel booleans. In TerminalController.swift, socketConnectable is Bool? and only set when socketProbePerformed is true; downstream logic must treat nil as 'not probed'. Ensure downstream code checks for nil before using a value and uses explicit non-nil checks to determine state, improving clarity and avoiding misinterpretation of default false.
Learnt from: zlatkoc
Repo: manaflow-ai/cmux PR: 1368
File: Sources/Panels/BrowserPanel.swift:69-69
Timestamp: 2026-03-13T13:46:01.733Z
Learning: Do not wrap engine/brand name literals (e.g., displayName values such as Google, DuckDuckGo, Bing, Kagi, Startpage) in String(localized: ...). These are brand/product names that are not translatable UI text. Localization should apply to generic UI strings (labels, buttons, error messages, etc.). Apply this guideline across Swift source files under Sources/ (notably in BrowserPanel.swift and similar UI/engine-related strings) and flag only brand-name strings that are part of user-facing UI text appropriately for translation scope.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found across 1 file
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
|
Codex Review: Didn't find any major issues. Hooray! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Does this fix #2653 ? EDIT: is this fixed by #2780 (unreleased)? |
|
Thank you for this! Browser arrow and IME input handling are on main in 296537c :) |
3 participants
Summary
WKWebView has been in the responder chain — its hosting view intercepts
key events via performKeyEquivalent, returns true, but never fires the action
Testing
<textarea>xcodebuild -scheme cmux-unit test— no new failuresBrowserOmnibarCommandNavigationTestsandCJKIMEPerformKeyEquivalentTestspassNo new unit tests added — the bug depends on SwiftUI's hosting view
entering a broken focus state after a WKWebView has been in the responder
chain, which cannot be reproduced in a unit test environment. The fix is
verified through manual testing and covered indirectly by existing tests
for the pure routing logic.
Demo Video
CleanShot.2026-03-14.at.14.26.05-trimmed-converted.3.mp4
Checklist
Fixes #1146
Summary by cubic
Fixes arrow keys (including Cmd+arrow) in browser textareas and restores IME candidate selection in the terminal by bypassing SwiftUI’s broken focus path after a
WKWebViewis focused. App shortcuts still work as expected. Fixes #1146.WKWebViewto keep arrows/Cmd+arrows working in textareas.WKWebView(not just a terminal), preventing swallowed keys.Written for commit f4c2e20. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes