Repository navigation
Fix clipboard manager paste failing during focus transitions - #2435
alexander-pecheny wants to merge 2 commits into
Conversation
When a clipboard manager overlay (Raycast, Tuna) dismisses and sends a synthetic Cmd+V, the window regains key status before the terminal view becomes first responder. The first responder is briefly the window itself (AppKitWindow), so the main menu bypass doesn't fire (no terminal in the responder chain to validate paste:), and the event falls through to the SwiftUI view hierarchy which swallows it. Fix: detect when the first responder is the window during a Command key equivalent and restore the focused terminal's GhosttyNSView as first responder before dispatching to the main menu. Fixes manaflow-ai#2415, related to manaflow-ai#700 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@alexander-pecheny is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughWindow-level Command-key routing now recovers missing Ghostty first responders by searching subviews for a Changes
Sequence DiagramsequenceDiagram
actor User
participant AppDelegate
participant NSWindow
participant ViewHierarchy as "Subview Tree"
participant GhosttyNSView
User->>AppDelegate: Press Cmd+<key>
AppDelegate->>NSWindow: check firstResponder / ghostty owner
alt firstResponderGhosttyView == nil && firstResponder is NSWindow
AppDelegate->>ViewHierarchy: findGhosttyNSView(in: window.contentView)
ViewHierarchy->>ViewHierarchy: recursively traverse subviews
ViewHierarchy->>GhosttyNSView: locate GhosttyNSView
AppDelegate->>NSWindow: makeFirstResponder(GhosttyNSView)
NSWindow->>GhosttyNSView: focus restored
end
AppDelegate->>NSWindow: performKeyEquivalent -> menu check
alt consumedByMenu == true
NSWindow->>User: execute menu action
else
NSWindow->>GhosttyNSView: deliver key-equivalent to terminal
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
⚔️ Resolve merge conflicts
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![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 12949-12953: Ensure the menu bypass is only set when we
successfully recovered a responder in the same window: when locating the
terminal panel via
AppDelegate.shared?.tabManager?.selectedWorkspace?.focusedTerminalPanel and its
Ghostty view found by Self.findGhosttyNSView(in:), first verify the
terminalPanel.hostedView.window === self (same NSWindow) and then call
self.makeFirstResponder(ghosttyNSView) and only assign menuBypassGhosttyView =
ghosttyNSView if makeFirstResponder returned true; apply the same checks before
enabling the menu bypass in the later block that sets the bypass (lines
~12960-12961).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Greptile SummaryThis PR fixes Cmd+V paste failures that occur when a clipboard manager overlay (Raycast, Tuna, etc.) dismisses and sends a synthetic key event during an AppKit focus transition. When the overlay closes, the window regains key status before any view claims Changes:
Minor issue: The return value of Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant CM as Clipboard Manager<br/>(Raycast/Tuna)
participant OS as macOS
participant Win as NSWindow
participant PKE as cmux_performKeyEquivalent
participant FR as firstResponder<br/>(NSWindow self)
participant TP as focusedTerminalPanel
participant GNV as GhosttyNSView
participant Menu as NSApp.mainMenu
CM->>OS: Dismiss overlay + send synthetic Cmd+V
OS->>Win: Window becomes key (focus transition)
Note over Win,FR: firstResponder = NSWindow itself<br/>(terminal not yet first responder)
OS->>PKE: performKeyEquivalent(Cmd+V)
PKE->>PKE: firstResponderGhosttyView = nil<br/>(window is FR, not a GhosttyNSView)
PKE->>PKE: menuBypassGhosttyView = nil
PKE->>PKE: self.firstResponder is NSWindow? ✓
PKE->>TP: AppDelegate.shared?.tabManager?<br/>.selectedWorkspace?.focusedTerminalPanel
TP-->>PKE: terminalPanel
PKE->>GNV: findGhosttyNSView(in: terminalPanel.hostedView)
GNV-->>PKE: ghosttyNSView
PKE->>Win: makeFirstResponder(ghosttyNSView)
Win->>GNV: becomeFirstResponder()
GNV-->>Win: true
PKE->>PKE: menuBypassGhosttyView = ghosttyNSView
PKE->>Menu: mainMenu.performKeyEquivalent(Cmd+V)
Menu->>GNV: paste: (via responder chain)
GNV-->>Menu: handled ✓
Menu-->>PKE: consumedByMenu = true
PKE-->>OS: return true
Reviews (1): Last reviewed commit: "Fix clipboard manager paste failing duri..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| self.makeFirstResponder(ghosttyNSView) | ||
| menuBypassGhosttyView = ghosttyNSView |
There was a problem hiding this comment.
makeFirstResponder return value unchecked
makeFirstResponder can return false when the view is not currently in the window's view hierarchy (e.g., the hosted view is detached during a workspace transition). In that case, menuBypassGhosttyView is still set to ghosttyNSView, so the code proceeds with mainMenu.performKeyEquivalent(with: event) even though the first responder was not actually restored. paste: validation then runs against the window's own responder chain (which has no terminal), consumedByMenu is false, and the code falls through to cmux_performKeyEquivalent — so the paste still fails, but the bypass fires unnecessarily.
Gating the assignment on the return value makes the intent explicit and avoids the spurious menu invocation:
| self.makeFirstResponder(ghosttyNSView) | |
| menuBypassGhosttyView = ghosttyNSView | |
| if self.makeFirstResponder(ghosttyNSView) { | |
| menuBypassGhosttyView = ghosttyNSView |
You'd also need a closing } after the #if DEBUG block at line 12957.
… success Check that the recovered GhosttyNSView belongs to the current window and that makeFirstResponder actually succeeds before enabling the menu bypass. Without these guards, the bypass could fire with no valid terminal responder if the view is detached or belongs to a different window. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Sources/AppDelegate.swift (1)
12948-12958: Add a regression test for the recovered-responder path.An
AppDelegateShortcutRoutingTestscase that forcesfirstResponder === windowfor synthetic Cmd+V—and a Cmd+` variant—would lock in this recovery path and guard the native window-cycling behavior while this responder swap is in place.Based on learnings, "Cmd+
(command-backtick, 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+."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 12948 - 12958, Add a regression test in AppDelegateShortcutRoutingTests that simulates the recovered-responder path by forcing AppDelegate.firstResponder to be the NSWindow (so menuBypassGhosttyView starts nil) and then exercising the synthetic Cmd+V and Cmd+` paths; verify that when the code path in AppDelegate that uses findGhosttyNSView(in:), checks selectedWorkspace?.focusedTerminalPanel.hostedView, and calls makeFirstResponder(ghosttyNSView) it restores menuBypassGhosttyView and allows Cmd+V menu routing, but do not change behavior for keyCode 50 (Cmd+`)—assert shouldRouteCommandEquivalentDirectlyToMainMenu still excludes Cmd+` and that no window/main-menu bypass is added for that key.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Sources/AppDelegate.swift`:
- Around line 12948-12958: Add a regression test in
AppDelegateShortcutRoutingTests that simulates the recovered-responder path by
forcing AppDelegate.firstResponder to be the NSWindow (so menuBypassGhosttyView
starts nil) and then exercising the synthetic Cmd+V and Cmd+` paths; verify that
when the code path in AppDelegate that uses findGhosttyNSView(in:), checks
selectedWorkspace?.focusedTerminalPanel.hostedView, and calls
makeFirstResponder(ghosttyNSView) it restores menuBypassGhosttyView and allows
Cmd+V menu routing, but do not change behavior for keyCode 50 (Cmd+`)—assert
shouldRouteCommandEquivalentDirectlyToMainMenu still excludes Cmd+` and that no
window/main-menu bypass is added for that key.
|
This review could not be run because your cubic account has exceeded the monthly review limit. If you need help restoring access, please contact contact@cubic.dev. |
da2e4ba to
99eeec8
Compare
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 `@Sources/Update/UpdateController.swift`:
- Around line 104-107: The updater-disabled bundle check
(Bundle.main.bundleIdentifier != "com.cmuxterm.app") is currently used only at
startup but manual flows still enter readiness-retry and can produce the
misleading "Updater is still starting…" timeout; modify
UpdateController.checkForUpdates and UpdateController.checkForUpdatesWhenReady
to short-circuit when the bundle is updater-disabled by checking the same
Bundle.main.bundleIdentifier condition up-front and returning early (no-op) or
appending a clear disabled message via UpdateLogStore.shared.append (e.g.,
"updater disabled (non-release bundle)"), so the readiness-retry path is never
entered for disabled bundles.
In `@Sources/Workspace.swift`:
- Around line 7492-7500: The change sets workingDirectory: nil when constructing
TerminalPanel (in the TerminalPanel(...) call using workspaceId: id, context:
GHOSTTY_SURFACE_CONTEXT_SPLIT, configTemplate: inheritedConfig) which
unintentionally alters split-terminal CWD behavior; revert this by restoring the
prior cmux fallback resolution (i.e., pass through the previous workingDirectory
value or restore the logic that computed the fallback CWD instead of forcing
nil) or remove the workingDirectory change from this PR and move it to a
dedicated split-CWD PR with tests for split terminal cwd behavior.
🪄 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: e4ba707f-3825-4e63-831f-057a1fb4f511
📒 Files selected for processing (5)
Sources/GhosttyTerminalView.swiftSources/SocketControlSettings.swiftSources/TabManager.swiftSources/Update/UpdateController.swiftSources/Workspace.swift
💤 Files with no reviewable changes (1)
- Sources/GhosttyTerminalView.swift
| if Bundle.main.bundleIdentifier != "com.cmuxterm.app" { | ||
| UpdateLogStore.shared.append("updater skipped (non-release bundle: \(Bundle.main.bundleIdentifier ?? "nil"))") | ||
| return | ||
| } |
There was a problem hiding this comment.
Handle the updater-disabled path explicitly in update-check flows.
Line 104 intentionally skips updater startup for non-release bundles, but the manual check path can still run readiness retries and end in the misleading timeout error (“Updater is still starting…”). Please short-circuit checkForUpdates / checkForUpdatesWhenReady when this bundle is updater-disabled (no-op or dedicated disabled message), instead of entering startup-retry state.
Suggested fix sketch
class UpdateController {
+ private var isUpdaterDisabledForCurrentBundle: Bool {
+ Bundle.main.bundleIdentifier != "com.cmuxterm.app"
+ }
func startUpdaterIfNeeded() {
guard !didStartUpdater else { return }
- if Bundle.main.bundleIdentifier != "com.cmuxterm.app" {
+ if isUpdaterDisabledForCurrentBundle {
UpdateLogStore.shared.append("updater skipped (non-release bundle: \(Bundle.main.bundleIdentifier ?? "nil"))")
return
}
...
}
func checkForUpdatesWhenReady(retries: Int = 10) {
+ guard !isUpdaterDisabledForCurrentBundle else {
+ UpdateLogStore.shared.append("checkForUpdates skipped (updater disabled for this bundle)")
+ return
+ }
...
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Update/UpdateController.swift` around lines 104 - 107, The
updater-disabled bundle check (Bundle.main.bundleIdentifier !=
"com.cmuxterm.app") is currently used only at startup but manual flows still
enter readiness-retry and can produce the misleading "Updater is still
starting…" timeout; modify UpdateController.checkForUpdates and
UpdateController.checkForUpdatesWhenReady to short-circuit when the bundle is
updater-disabled by checking the same Bundle.main.bundleIdentifier condition
up-front and returning early (no-op) or appending a clear disabled message via
UpdateLogStore.shared.append (e.g., "updater disabled (non-release bundle)"), so
the readiness-retry path is never entered for disabled bundles.
| // Let Ghostty handle working directory via its own | ||
| // working-directory / split-inherit-working-directory config. | ||
|
|
||
| // Create the new terminal panel. | ||
| let newPanel = TerminalPanel( | ||
| workspaceId: id, | ||
| context: GHOSTTY_SURFACE_CONTEXT_SPLIT, | ||
| configTemplate: inheritedConfig, | ||
| workingDirectory: splitWorkingDirectory, | ||
| workingDirectory: nil, |
There was a problem hiding this comment.
This introduces an out-of-scope split-CWD behavior change in a paste-fix PR.
Line 7500 now forces workingDirectory: nil, which changes split terminal cwd behavior and is unrelated to the clipboard-routing objective. Please either move this to a dedicated PR with split-cwd regression coverage, or keep prior cmux fallback resolution in this PR.
Suggested revert (if this PR should stay scoped to clipboard paste)
- // Let Ghostty handle working directory via its own
- // working-directory / split-inherit-working-directory config.
+ let splitWorkingDirectory = panelDirectories[panelId]
+ ?? terminalPanel(for: panelId)?.requestedWorkingDirectory
+ ?? currentDirectory
// Create the new terminal panel.
let newPanel = TerminalPanel(
workspaceId: id,
context: GHOSTTY_SURFACE_CONTEXT_SPLIT,
configTemplate: inheritedConfig,
- workingDirectory: nil,
+ workingDirectory: splitWorkingDirectory,
portOrdinal: portOrdinal,
initialCommand: remoteTerminalStartupCommand
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Workspace.swift` around lines 7492 - 7500, The change sets
workingDirectory: nil when constructing TerminalPanel (in the TerminalPanel(...)
call using workspaceId: id, context: GHOSTTY_SURFACE_CONTEXT_SPLIT,
configTemplate: inheritedConfig) which unintentionally alters split-terminal CWD
behavior; revert this by restoring the prior cmux fallback resolution (i.e.,
pass through the previous workingDirectory value or restore the logic that
computed the fallback CWD instead of forcing nil) or remove the workingDirectory
change from this PR and move it to a dedicated split-CWD PR with tests for split
terminal cwd behavior.
|
Thanks for this! The Raycast paste focus fix landed on main in #2768. You opened this first, so you got there first. Closing since main covers it now. |
2 participants
Human Summary
Text below the horizontal line is written by Claude. This is my human summary: I've had issues with raycast clipboard history not working ( #2415 ). This PR was made by Claude, but I've verified that it actually fixes the issue, AND that
cmd-+ / − / 0shortcuts to change zoom still work (commit making them work was the one that broke pasting). Feel free to reject / rewrite that, it is only as a proof of concept how this might be fixed.Summary
Problem
When a clipboard manager overlay dismisses and sends a synthetic Cmd+V, the window regains key status before the terminal view becomes first responder. The first responder is briefly the
NSWindowitself, so:paste:)Fix
Detect when
firstResponderis the window during a Command key equivalent, look up the focused terminal panel'sGhosttyNSView, restore it as first responder, then dispatch to the main menu as usual.Fixes #2415, related to #700
Test plan
🤖 Posted by Claude Code
Summary by CodeRabbit
Bug Fixes
Behavior Changes