Fix tooltip tracking lifetime and shortcut lag#1043
Merged
austinywang merged 2 commits intomainfrom Mar 7, 2026
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
To use Codex here, create a Codex account and connect to github. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughMigrates Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
Contributor
Greptile SummaryThis PR addresses two distinct bugs: orphaned Tooltip tracking lifetime fix:
Shortcut simulation lag fix:
Key changes by file:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant ST as Socket Thread
participant Main as Main Thread (DispatchQueue.main.sync)
participant App as NSApp
participant Win as NSWindow
Note over ST,Win: simulateShortcut / simulateType
ST->>Main: DispatchQueue.main.sync { ... }
Main->>Main: Resolve targetWindow
Main->>Main: prepareWindowForSyntheticInput(targetWindow)
alt App not active
Main->>App: NSApp.activate(ignoringOtherApps: true)
end
alt Window not key OR not visible
Main->>Win: window.makeKeyAndOrderFront(nil)
end
Main->>App: NSApp.sendEvent(keyDownEvent)
Main->>App: NSApp.sendEvent(keyUpEvent)
ST-->>ST: result = "OK"
Note over ST,Win: Before this PR: activate + makeKeyAndOrderFront called unconditionally every time
Note over ST,Win: After this PR: called only when truly needed → reduced lag
Last reviewed commit: 9e8a401 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/Find/BrowserSearchOverlay.swift (1)
76-98:⚠️ Potential issue | 🟡 MinorLocalize the new tooltip strings.
Lines 76, 87, and 98 add user-visible help text as bare literals. Please wrap these in
String(localized:..., defaultValue:...)before passing them to.safeHelp(...).Suggested fix
- .safeHelp("Next match (Return)") + .safeHelp(String(localized: "browserSearchOverlay.nextMatch.help", defaultValue: "Next match (Return)")) … - .safeHelp("Previous match (Shift+Return)") + .safeHelp(String(localized: "browserSearchOverlay.previousMatch.help", defaultValue: "Previous match (Shift+Return)")) … - .safeHelp("Close (Esc)") + .safeHelp(String(localized: "browserSearchOverlay.close.help", defaultValue: "Close (Esc)"))As per coding guidelines "All user-facing strings must be localized using
String(localized: "key.name", defaultValue: "English text")for every string shown in the UI" and "Never use bare string literals in SwiftUIText(),Button(), alert titles, or other user-visible UI elements; all must use theString(localized:)API".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Find/BrowserSearchOverlay.swift` around lines 76 - 98, Replace the three bare help-string literals passed to .safeHelp(...) with localized variants using String(localized:..., defaultValue:...), e.g. for the .safeHelp("Next match (Return)"), .safeHelp("Previous match (Shift+Return)"), and .safeHelp("Close (Esc)") calls, call String(localized: "find.nextMatch.help", defaultValue: "Next match (Return)"), String(localized: "find.previousMatch.help", defaultValue: "Previous match (Shift+Return)"), and String(localized: "find.close.help", defaultValue: "Close (Esc)") respectively so the help text shown by Button / .safeHelp uses the String(localized:..., defaultValue:...) API.
🤖 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/Panels/BrowserPanelView.swift`:
- Line 627: Replace the hard-coded tooltip string passed to .safeHelp(...) with
a localized string key; e.g. use String(localized: "browser.theme.tooltip",
defaultValue: "Browser Theme: %@").formatted(browserThemeMode.displayName) so
the wrapper text is localized while still including
browserThemeMode.displayName. Update the .safeHelp call to use that
localized-formatted string (referencing browserThemeMode.displayName and the
.safeHelp modifier in BrowserPanelView) and add the "browser.theme.tooltip" key
to localization files.
In `@Sources/TerminalController.swift`:
- Around line 10254-10325: Add a DEBUG-only dlog() call in simulateShortcut
after preparing the targetWindow and creating keyDownEvent/keyUpEvent (and
before returning) to record the synthetic shortcut activity: include
parsed.characters, parsed.charactersIgnoringModifiers, parsed.keyCode,
parsed.modifierFlags, requestTimestamp, windowNumber
(targetWindow?.windowNumber), and whether
AppDelegate.shared?.debugHandleCustomShortcut handled the event; place the dlog
invocation near the block that checks AppDelegate.debugHandleCustomShortcut and
before NSApp.sendEvent so both handled and dispatched cases are logged; use the
existing symbols simulateShortcut, prepareWindowForSyntheticInput, keyDownEvent,
keyUpEvent, and AppDelegate.debugHandleCustomShortcut to locate where to insert
the call.
- Around line 10241-10252: prepareWindowForSyntheticInput currently activates
the app and raises the window which steals macOS focus; change it to not mutate
focus: in prepareWindowForSyntheticInput(_ window: NSWindow?) (the helper used
by simulate_shortcut and simulate_type) remove the
NSApp.activate(ignoringOtherApps:) and window.makeKeyAndOrderFront(nil) calls
and instead simply check guard let window else { return } plus require the app
to already be active and the window to be key and visible—if not, return/fail
early (or log/throw) so synthetic input does not force activation; ensure
callers (simulate_shortcut/simulate_type) rely on an explicit activate_app
command when focus is intended.
---
Outside diff comments:
In `@Sources/Find/BrowserSearchOverlay.swift`:
- Around line 76-98: Replace the three bare help-string literals passed to
.safeHelp(...) with localized variants using String(localized:...,
defaultValue:...), e.g. for the .safeHelp("Next match (Return)"),
.safeHelp("Previous match (Shift+Return)"), and .safeHelp("Close (Esc)") calls,
call String(localized: "find.nextMatch.help", defaultValue: "Next match
(Return)"), String(localized: "find.previousMatch.help", defaultValue: "Previous
match (Shift+Return)"), and String(localized: "find.close.help", defaultValue:
"Close (Esc)") respectively so the help text shown by Button / .safeHelp uses
the String(localized:..., defaultValue:...) API.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 66ae2e88-205d-4551-91d3-d737e4f0bc60
📒 Files selected for processing (12)
Sources/BrowserWindowPortal.swiftSources/ContentView.swiftSources/Find/BrowserSearchOverlay.swiftSources/Find/SurfaceSearchOverlay.swiftSources/GhosttyTerminalView.swiftSources/NotificationsPage.swiftSources/Panels/BrowserPanelView.swiftSources/TerminalController.swiftSources/TerminalWindowPortal.swiftSources/Update/UpdatePill.swiftSources/Update/UpdateTitlebarAccessory.swiftvendor/bonsplit
| browserThemeModePopover | ||
| } | ||
| .help("Browser Theme: \(browserThemeMode.displayName)") | ||
| .safeHelp("Browser Theme: \(browserThemeMode.displayName)") |
There was a problem hiding this comment.
Localize the browser theme tooltip wrapper string.
"Browser Theme: ..." is still a user-visible literal, so this tooltip stays English-only even if browserThemeMode.displayName is localized.
Suggested fix
- .safeHelp("Browser Theme: \(browserThemeMode.displayName)")
+ .safeHelp(
+ String(
+ localized: "browser.theme.tooltip",
+ defaultValue: "Browser Theme: \(browserThemeMode.displayName)"
+ )
+ )As per coding guidelines, "All user-facing strings must be localized using String(localized: "key.name", defaultValue: "English text")" and "Never use bare string literals in SwiftUI Text(), Button(), alert titles, or other user-visible UI elements; all must use the String(localized:) API".
📝 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
| .safeHelp("Browser Theme: \(browserThemeMode.displayName)") | |
| .safeHelp( | |
| String( | |
| localized: "browser.theme.tooltip", | |
| defaultValue: "Browser Theme: \(browserThemeMode.displayName)" | |
| ) | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Panels/BrowserPanelView.swift` at line 627, Replace the hard-coded
tooltip string passed to .safeHelp(...) with a localized string key; e.g. use
String(localized: "browser.theme.tooltip", defaultValue: "Browser Theme:
%@").formatted(browserThemeMode.displayName) so the wrapper text is localized
while still including browserThemeMode.displayName. Update the .safeHelp call to
use that localized-formatted string (referencing browserThemeMode.displayName
and the .safeHelp modifier in BrowserPanelView) and add the
"browser.theme.tooltip" key to localization files.
Comment on lines
+10241
to
+10252
| private func prepareWindowForSyntheticInput(_ window: NSWindow?) { | ||
| guard let window else { return } | ||
|
|
||
| // Stamp at socket-handler arrival so event.timestamp includes any wait | ||
| // before the main-thread event dispatch. | ||
| let requestTimestamp = ProcessInfo.processInfo.systemUptime | ||
|
|
||
| var result = "ERROR: Failed to create event" | ||
| DispatchQueue.main.sync { | ||
| // Prefer the current active-tab-manager window so shortcut simulation stays | ||
| // scoped to the intended window even when NSApp.keyWindow is stale. | ||
| let targetWindow: NSWindow? = { | ||
| if let activeTabManager = self.tabManager, | ||
| let windowId = AppDelegate.shared?.windowId(for: activeTabManager), | ||
| let window = AppDelegate.shared?.mainWindow(for: windowId) { | ||
| return window | ||
| } | ||
| return NSApp.keyWindow | ||
| ?? NSApp.mainWindow | ||
| ?? NSApp.windows.first(where: { $0.isVisible }) | ||
| ?? NSApp.windows.first | ||
| }() | ||
| if let targetWindow { | ||
| NSApp.activate(ignoringOtherApps: true) | ||
| targetWindow.makeKeyAndOrderFront(nil) | ||
| } | ||
| let windowNumber = targetWindow?.windowNumber ?? 0 | ||
| guard let keyDownEvent = NSEvent.keyEvent( | ||
| with: .keyDown, | ||
| location: .zero, | ||
| modifierFlags: parsed.modifierFlags, | ||
| timestamp: requestTimestamp, | ||
| windowNumber: windowNumber, | ||
| context: nil, | ||
| characters: parsed.characters, | ||
| charactersIgnoringModifiers: parsed.charactersIgnoringModifiers, | ||
| isARepeat: false, | ||
| keyCode: parsed.keyCode | ||
| ) else { | ||
| result = "ERROR: NSEvent.keyEvent returned nil" | ||
| return | ||
| } | ||
| let keyUpEvent = NSEvent.keyEvent( | ||
| with: .keyUp, | ||
| location: .zero, | ||
| modifierFlags: parsed.modifierFlags, | ||
| timestamp: requestTimestamp + 0.0001, | ||
| windowNumber: windowNumber, | ||
| context: nil, | ||
| characters: parsed.characters, | ||
| charactersIgnoringModifiers: parsed.charactersIgnoringModifiers, | ||
| isARepeat: false, | ||
| keyCode: parsed.keyCode | ||
| ) | ||
| // Socket-driven shortcut simulation should reuse the exact same matching logic as the | ||
| // app-level shortcut monitor (so tests are hermetic), while still falling back to the | ||
| // normal responder chain for plain typing. | ||
| if let delegate = AppDelegate.shared, delegate.debugHandleCustomShortcut(event: keyDownEvent) { | ||
| result = "OK" | ||
| return | ||
| } | ||
| NSApp.sendEvent(keyDownEvent) | ||
| if let keyUpEvent { | ||
| NSApp.sendEvent(keyUpEvent) | ||
| } | ||
| result = "OK" | ||
| } | ||
| return result | ||
| } | ||
| // Keep socket-driven input simulation focused on the intended window without | ||
| // paying repeated activation/order-front costs for every synthetic key event. | ||
| if !NSApp.isActive { | ||
| NSApp.activate(ignoringOtherApps: true) | ||
| } | ||
| if !window.isKeyWindow || !window.isVisible { | ||
| window.makeKeyAndOrderFront(nil) | ||
| } | ||
| } |
There was a problem hiding this comment.
Don't activate or raise the app from this helper.
prepareWindowForSyntheticInput now calls NSApp.activate and window.makeKeyAndOrderFront, so both simulate_shortcut and the Line 10370 simulate_type path will steal macOS/app focus even though these are not explicit focus-intent socket commands. Prefer failing when the target window is not already ready, or require tests to call the explicit activate_app command first.
As per coding guidelines, "Socket/CLI commands must not steal macOS app focus (no app activation/window raising side effects)" and "Only explicit focus-intent commands may mutate in-app focus/selection (window.focus, workspace.select/next/previous/last, surface.focus, pane.focus/last, browser focus commands, and v1 focus equivalents)`."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/TerminalController.swift` around lines 10241 - 10252,
prepareWindowForSyntheticInput currently activates the app and raises the window
which steals macOS focus; change it to not mutate focus: in
prepareWindowForSyntheticInput(_ window: NSWindow?) (the helper used by
simulate_shortcut and simulate_type) remove the
NSApp.activate(ignoringOtherApps:) and window.makeKeyAndOrderFront(nil) calls
and instead simply check guard let window else { return } plus require the app
to already be active and the window to be key and visible—if not, return/fail
early (or log/throw) so synthetic input does not force activation; ensure
callers (simulate_shortcut/simulate_type) rely on an explicit activate_app
command when focus is intended.
Comment on lines
+10254
to
+10325
| private func simulateShortcut(_ args: String) -> String { | ||
| let combo = args.trimmingCharacters(in: .whitespacesAndNewlines) | ||
| guard !combo.isEmpty else { | ||
| return "ERROR: Usage: simulate_shortcut <combo>" | ||
| } | ||
| guard let parsed = parseShortcutCombo(combo) else { | ||
| return "ERROR: Invalid combo. Example: cmd+ctrl+h" | ||
| } | ||
|
|
||
| // Stamp at socket-handler arrival so event.timestamp includes any wait | ||
| // before the main-thread event dispatch. | ||
| let requestTimestamp = ProcessInfo.processInfo.systemUptime | ||
|
|
||
| var result = "ERROR: Failed to create event" | ||
| DispatchQueue.main.sync { | ||
| // Prefer the current active-tab-manager window so shortcut simulation stays | ||
| // scoped to the intended window even when NSApp.keyWindow is stale. | ||
| let targetWindow: NSWindow? = { | ||
| if let activeTabManager = self.tabManager, | ||
| let windowId = AppDelegate.shared?.windowId(for: activeTabManager), | ||
| let window = AppDelegate.shared?.mainWindow(for: windowId) { | ||
| return window | ||
| } | ||
| return NSApp.keyWindow | ||
| ?? NSApp.mainWindow | ||
| ?? NSApp.windows.first(where: { $0.isVisible }) | ||
| ?? NSApp.windows.first | ||
| }() | ||
| prepareWindowForSyntheticInput(targetWindow) | ||
| let windowNumber = targetWindow?.windowNumber ?? 0 | ||
| guard let keyDownEvent = NSEvent.keyEvent( | ||
| with: .keyDown, | ||
| location: .zero, | ||
| modifierFlags: parsed.modifierFlags, | ||
| timestamp: requestTimestamp, | ||
| windowNumber: windowNumber, | ||
| context: nil, | ||
| characters: parsed.characters, | ||
| charactersIgnoringModifiers: parsed.charactersIgnoringModifiers, | ||
| isARepeat: false, | ||
| keyCode: parsed.keyCode | ||
| ) else { | ||
| result = "ERROR: NSEvent.keyEvent returned nil" | ||
| return | ||
| } | ||
| let keyUpEvent = NSEvent.keyEvent( | ||
| with: .keyUp, | ||
| location: .zero, | ||
| modifierFlags: parsed.modifierFlags, | ||
| timestamp: requestTimestamp + 0.0001, | ||
| windowNumber: windowNumber, | ||
| context: nil, | ||
| characters: parsed.characters, | ||
| charactersIgnoringModifiers: parsed.charactersIgnoringModifiers, | ||
| isARepeat: false, | ||
| keyCode: parsed.keyCode | ||
| ) | ||
| // Socket-driven shortcut simulation should reuse the exact same matching logic as the | ||
| // app-level shortcut monitor (so tests are hermetic), while still falling back to the | ||
| // normal responder chain for plain typing. | ||
| if let delegate = AppDelegate.shared, delegate.debugHandleCustomShortcut(event: keyDownEvent) { | ||
| result = "OK" | ||
| return | ||
| } | ||
| NSApp.sendEvent(keyDownEvent) | ||
| if let keyUpEvent { | ||
| NSApp.sendEvent(keyUpEvent) | ||
| } | ||
| result = "OK" | ||
| } | ||
| return result | ||
| } |
There was a problem hiding this comment.
Add a dlog() entry for the synthetic shortcut path.
This code now prepares window focus state and injects synthetic keyDown/keyUp events, but it still emits no unified debug-event log entry for the actual key/focus activity. That makes shortcut-lag regressions much harder to trace in DEBUG builds.
As per coding guidelines, "All debug events (keys, mouse, focus, splits, tabs) must be logged to the unified debug event log in DEBUG builds using the dlog() free function."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/TerminalController.swift` around lines 10254 - 10325, Add a
DEBUG-only dlog() call in simulateShortcut after preparing the targetWindow and
creating keyDownEvent/keyUpEvent (and before returning) to record the synthetic
shortcut activity: include parsed.characters,
parsed.charactersIgnoringModifiers, parsed.keyCode, parsed.modifierFlags,
requestTimestamp, windowNumber (targetWindow?.windowNumber), and whether
AppDelegate.shared?.debugHandleCustomShortcut handled the event; place the dlog
invocation near the block that checks AppDelegate.debugHandleCustomShortcut and
before NSApp.sendEvent so both handled and dispatched cases are logged; use the
existing symbols simulateShortcut, prepareWindowForSyntheticInput, keyDownEvent,
keyUpEvent, and AppDelegate.debugHandleCustomShortcut to locate where to insert
the call.
![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 12 files
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="Sources/Find/BrowserSearchOverlay.swift">
<violation number="1" location="Sources/Find/BrowserSearchOverlay.swift:76">
P3: Localize this tooltip string instead of using a raw literal so it can be translated in non-English locales.</violation>
</file>
<file name="Sources/Panels/BrowserPanelView.swift">
<violation number="1" location="Sources/Panels/BrowserPanelView.swift:627">
P3: Localize this tooltip string instead of hardcoding English text.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| } | ||
| .buttonStyle(SearchButtonStyle()) | ||
| .help("Next match (Return)") | ||
| .safeHelp("Next match (Return)") |
There was a problem hiding this comment.
P3: Localize this tooltip string instead of using a raw literal so it can be translated in non-English locales.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/Find/BrowserSearchOverlay.swift, line 76:
<comment>Localize this tooltip string instead of using a raw literal so it can be translated in non-English locales.</comment>
<file context>
@@ -73,7 +73,7 @@ struct BrowserSearchOverlay: View {
}
.buttonStyle(SearchButtonStyle())
- .help("Next match (Return)")
+ .safeHelp("Next match (Return)")
Button(action: {
</file context>
Suggested change
| .safeHelp("Next match (Return)") | |
| .safeHelp(String(localized: "search.nextMatch.help", defaultValue: "Next match (Return)")) |
| browserThemeModePopover | ||
| } | ||
| .help("Browser Theme: \(browserThemeMode.displayName)") | ||
| .safeHelp("Browser Theme: \(browserThemeMode.displayName)") |
There was a problem hiding this comment.
P3: Localize this tooltip string instead of hardcoding English text.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/Panels/BrowserPanelView.swift, line 627:
<comment>Localize this tooltip string instead of hardcoding English text.</comment>
<file context>
@@ -624,7 +624,7 @@ struct BrowserPanelView: View {
browserThemeModePopover
}
- .help("Browser Theme: \(browserThemeMode.displayName)")
+ .safeHelp("Browser Theme: \(browserThemeMode.displayName)")
.accessibilityIdentifier("BrowserThemeModeButton")
}
</file context>
…acking-area-crash # Conflicts: # Sources/ContentView.swift
This was referenced Mar 9, 2026
This was referenced Mar 27, 2026
bn-l
pushed a commit
to bn-l/cmux
that referenced
this pull request
Apr 3, 2026
…ip-tracking-area-crash Fix tooltip tracking lifetime and shortcut lag
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
Demo Video
For UI or behavior changes, include a short demo video (GitHub upload, Loom, or other direct link).
Review Trigger (Copy/Paste as PR comment)
Checklist
Summary by cubic
Fixes tooltip crashes by cleaning up tracking areas and switching all tooltips to safeHelp. Also reduces shortcut lag by focusing the right window only when needed.
Bug Fixes
Performance
Written for commit 01e9a05. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Chores