Fix browser omnibar typing lag with many workspaces - #3422
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
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:
📝 WalkthroughWalkthroughAdds an in-memory omnibar open-tab suggestion index, moves open-tab matching into TabManager, refactors omnibar focus/select-all into deterministic request IDs, wires command-palette visibility notifications into focus flow, and switches UI-test browser-history seeding to an environment-based path. ChangesBrowser Omnibar Suggestions & Focus Management
Sequence Diagram(s)sequenceDiagram
participant Omnibar as BrowserPanelView / Omnibar
participant App as AppDelegate
participant Workspace
participant TabMgr as TabManager
participant Index as BrowserOpenTabSuggestionIndex
App->>Workspace: setCommandPaletteVisible -> postCommandPaletteVisibilityDidChangeIfNeeded
App-->>Omnibar: commandPaletteVisibilityDidChange notification
Omnibar->>Workspace: publishBrowserOpenTabSuggestion(for: panel)
Workspace->>TabMgr: upsertBrowserOpenTabSuggestion(snapshot)
TabMgr->>Index: upsert(panelId, snapshot)
Omnibar->>TabMgr: matchingOpenBrowserTabSuggestions(query...)
TabMgr->>Index: matching(query) --> matches
TabMgr-->>Omnibar: returns OmnibarOpenTabMatch[]
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. Comment |
Greptile SummaryThis PR fixes omnibar typing lag with many workspaces by moving the O(n·panels) scan out of the hot keystroke path: a Confidence Score: 4/5Safe to merge; only P2 findings present — one cross-window notification filter suggestion and one loop-exit micro-optimization. All findings are P2: the cross-window notification trigger is guarded by per-window state checks so no incorrect state transitions occur, and the continue-vs-break loop issue only affects performance marginally beyond the limit. Core caching logic, cache invalidation callsite coverage, and the select-all state machine are all correct. Sources/Panels/BrowserPanelView.swift (cross-window notification filter) and Sources/TabManager.swift (loop exit condition) Important Files Changed
Sequence DiagramsequenceDiagram
participant BP as BrowserPanelView
participant WS as Workspace
participant TM as TabManager (cache)
participant AD as AppDelegate
Note over WS,TM: Panel lifecycle (install / close)
WS->>TM: upsertBrowserOpenTabSuggestion(snapshot)
WS->>TM: removeBrowserOpenTabSuggestion(panelId)
Note over BP,TM: Omnibar keystroke path (hot)
BP->>TM: matchingOpenBrowserTabSuggestions(query, ...)
TM-->>BP: [OmnibarOpenTabMatch]
Note over WS,TM: URL / title change (Combine publisher)
WS->>TM: upsertBrowserOpenTabSuggestion(updatedSnapshot)
Note over AD,BP: Command palette state change
AD->>AD: post commandPaletteVisibilityDidChange
AD-->>BP: NotificationCenter (all windows)
BP->>BP: applyPendingAddressBarFocusRequestIfNeeded()
Note over BP: Select-all (state-driven)
BP->>BP: omnibarSelectAllRequestId &+= 1
BP->>BP: queueSelectAllRequest / applyPendingSelectAllIfPossible
Reviews (1): Last reviewed commit: "Fix browser omnibar typing lag with many..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| .onChange(of: panel.pendingAddressBarFocusRequestId) { _ in | ||
| applyPendingAddressBarFocusRequestIfNeeded() | ||
| } | ||
| .onReceive(NotificationCenter.default.publisher(for: .commandPaletteVisibilityDidChange)) { _ in |
There was a problem hiding this comment.
NotificationCenter.default.publisher(for: .commandPaletteVisibilityDidChange) is not filtered to the owning window, so closing a command palette in any window fires applyPendingAddressBarFocusRequestIfNeeded() on every browser panel across all windows. In a multi-window setup a panel in Window B could attempt to re-focus its address bar when Window A's palette closes — isCommandPaletteVisibleForPanelWindow() guards against advancing past the command-palette block, but a panel with a stale pending request in Window B would pass that guard because Window B's palette is not visible, potentially stealing focus from an unrelated window.
Filtering to the panel's own window avoids the spurious cross-window triggers:
.onReceive(
NotificationCenter.default.publisher(
for: .commandPaletteVisibilityDidChange,
object: panel.panelWindow // or however the owning NSWindow is accessed
)
) { _ in
applyPendingAddressBarFocusRequestIfNeeded()
}![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/Workspace.swift (1)
13539-13555:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReorder pane-close cleanup to avoid potential stale suggestion re-upsert.
At Line 13540, suggestion removal happens before
panel.close()while the subscription is still alive. Ifclose()emits a synchronous browser state publish, it can re-upsert a stale snapshot. Move removal afterclose()(or cancel subscription first).🔧 Suggested reorder
- let panel = panels[panelId] - if panel is BrowserPanel { - removeBrowserOpenTabSuggestion(panelId: panelId) - } - panel?.close() + let panel = panels[panelId] + panel?.close() + if panel is BrowserPanel { + removeBrowserOpenTabSuggestion(panelId: panelId) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 13539 - 13555, The cleanup calls removeBrowserOpenTabSuggestion(panelId:) run before panel?.close(), which can let a synchronous publish during BrowserPanel.close() re-create a stale suggestion; move the removeBrowserOpenTabSuggestion(panelId:) call to after panel?.close() or explicitly cancel the panel's subscription first (use panelSubscriptions[panelId] to cancel/remove) so that close() cannot re-upsert the suggestion while the cleanup continues.
🤖 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/TabManager.swift`:
- Around line 5722-5731: In append(_ snapshot: BrowserOpenTabSuggestionSnapshot,
isKnownOpenTab: Bool) the dedupe key (built from snapshot.workspaceId,
snapshot.panelId, snapshot.lowercasedURL) is being inserted into seenKeys before
calling snapshotMatches, which can cause a non-matching snapshot to block a
later matching one; fix by computing the key as now but move the seenKeys.insert
call to after the guard snapshotMatches(snapshot) check and perform the
seenKeys.insert(...).inserted guard immediately before appending to matches
(i.e., ensure you call snapshotMatches(snapshot) first, then guard
seenKeys.insert(key).inserted else { return }, then append to matches).
---
Outside diff comments:
In `@Sources/Workspace.swift`:
- Around line 13539-13555: The cleanup calls
removeBrowserOpenTabSuggestion(panelId:) run before panel?.close(), which can
let a synchronous publish during BrowserPanel.close() re-create a stale
suggestion; move the removeBrowserOpenTabSuggestion(panelId:) call to after
panel?.close() or explicitly cancel the panel's subscription first (use
panelSubscriptions[panelId] to cancel/remove) so that close() cannot re-upsert
the suggestion while the cleanup continues.
🪄 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: dabe2b09-4e47-4a7f-965c-a362d8a4da8c
📒 Files selected for processing (4)
Sources/AppDelegate.swiftSources/Panels/BrowserPanelView.swiftSources/TabManager.swiftSources/Workspace.swift
| func append(_ snapshot: BrowserOpenTabSuggestionSnapshot, isKnownOpenTab: Bool) { | ||
| guard matches.count < limit else { return } | ||
| let key = [ | ||
| snapshot.workspaceId.uuidString.lowercased(), | ||
| snapshot.panelId.uuidString.lowercased(), | ||
| snapshot.lowercasedURL, | ||
| ].joined(separator: "|") | ||
| guard seenKeys.insert(key).inserted else { return } | ||
| guard snapshotMatches(snapshot) else { return } | ||
| matches.append( |
There was a problem hiding this comment.
Move dedupe insertion after match evaluation.
At Line 5729, the key is marked as seen before Line 5730 checks snapshotMatches. A non-matching snapshot can block a later matching snapshot with the same key, causing missed suggestions.
Suggested fix
func append(_ snapshot: BrowserOpenTabSuggestionSnapshot, isKnownOpenTab: Bool) {
guard matches.count < limit else { return }
let key = [
snapshot.workspaceId.uuidString.lowercased(),
snapshot.panelId.uuidString.lowercased(),
snapshot.lowercasedURL,
].joined(separator: "|")
- guard seenKeys.insert(key).inserted else { return }
guard snapshotMatches(snapshot) else { return }
+ guard seenKeys.insert(key).inserted else { return }
matches.append(
OmnibarOpenTabMatch(
tabId: snapshot.workspaceId,
panelId: snapshot.panelId,
url: snapshot.url,📝 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.
| func append(_ snapshot: BrowserOpenTabSuggestionSnapshot, isKnownOpenTab: Bool) { | |
| guard matches.count < limit else { return } | |
| let key = [ | |
| snapshot.workspaceId.uuidString.lowercased(), | |
| snapshot.panelId.uuidString.lowercased(), | |
| snapshot.lowercasedURL, | |
| ].joined(separator: "|") | |
| guard seenKeys.insert(key).inserted else { return } | |
| guard snapshotMatches(snapshot) else { return } | |
| matches.append( | |
| func append(_ snapshot: BrowserOpenTabSuggestionSnapshot, isKnownOpenTab: Bool) { | |
| guard matches.count < limit else { return } | |
| let key = [ | |
| snapshot.workspaceId.uuidString.lowercased(), | |
| snapshot.panelId.uuidString.lowercased(), | |
| snapshot.lowercasedURL, | |
| ].joined(separator: "|") | |
| guard snapshotMatches(snapshot) else { return } | |
| guard seenKeys.insert(key).inserted else { return } | |
| matches.append( |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/TabManager.swift` around lines 5722 - 5731, In append(_ snapshot:
BrowserOpenTabSuggestionSnapshot, isKnownOpenTab: Bool) the dedupe key (built
from snapshot.workspaceId, snapshot.panelId, snapshot.lowercasedURL) is being
inserted into seenKeys before calling snapshotMatches, which can cause a
non-matching snapshot to block a later matching one; fix by computing the key as
now but move the seenKeys.insert call to after the guard
snapshotMatches(snapshot) check and perform the seenKeys.insert(...).inserted
guard immediately before appending to matches (i.e., ensure you call
snapshotMatches(snapshot) first, then guard seenKeys.insert(key).inserted else {
return }, then append to matches).
09543da to
20e7a06
Compare
20e7a06 to
0b78345
Compare
0b78345 to
580e04e
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/BrowserOmnibarPerformanceSupport.swift`:
- Around line 92-100: The code currently inserts the dedupe key into seenKeys
before verifying the snapshot actually matches, which suppresses later valid
matches; in append(_:isKnownOpenTab:) move the seenKeys.insert(key) (and the
early return on duplicate) to after guard snapshotMatches(snapshot) so
deduplication only happens for snapshots that pass snapshotMatches(_:), and
apply the same change to the other similar block (the later append/handling
between lines 112-125) so both paths only mark keys as seen after
snapshotMatches returns true; keep existing checks for matches.count < limit and
isKnownOpenTab logic intact.
- Around line 143-153: Add cleanup in TabManager.deinit to remove its entry from
the global browserOpenTabSuggestionIndexesByManagerId map: in the TabManager
type implement a deinit that computes let managerId = ObjectIdentifier(self) and
calls browserOpenTabSuggestionIndexesByManagerId.removeValue(forKey: managerId)
so closed managers don't leak BrowserOpenTabSuggestionIndex instances or yield
stale snapshots; place the deinit alongside the browserOpenTabSuggestionIndex
computed property to ensure symmetry.
- Around line 67-72: seedSnapshots() is being eagerly evaluated because you call
seedIfNeeded(seedSnapshots()), so browserOpenTabSuggestionSeedSnapshots() still
scans every workspace on each lookup; instead pass the closure itself to avoid
evaluation until needed: change the call to seedIfNeeded(seedSnapshots) and, if
seedIfNeeded currently takes an array, update its signature to accept a closure
(e.g., seedIfNeeded(_ snapshots: `@autoclosure` `@escaping` () ->
[BrowserOpenTabSuggestionSnapshot] or a regular () ->
[BrowserOpenTabSuggestionSnapshot]) and ensure seedIfNeeded invokes the closure
only when seeding is required.
In `@Sources/Panels/BrowserPanelView.swift`:
- Around line 716-718: Scope the .commandPaletteVisibilityDidChange subscriber
to this panel's window by inspecting the Notification's payload (use the
provided window or windowId) before calling
applyPendingAddressBarFocusRequestIfNeeded(); update the .onReceive closure that
currently calls applyPendingAddressBarFocusRequestIfNeeded() unconditionally to
first extract the notification's window/windowId and compare it against this
panel's owning window or windowId (the same identifier used by the panel), and
only call applyPendingAddressBarFocusRequestIfNeeded() when they match so
off-window BrowserPanelView instances cannot consume the shared
pendingAddressBarFocusRequestId.
- Around line 3698-3725: The applyPendingSelectAllIfPossible method must not
synchronously re-enter editing by calling field.selectText(nil); change it so
startEditingIfNeeded is ignored (or removed) and only apply selection when
field.currentEditor() already exists: if currentEditor() is nil, do not call
selectText, do not clear pendingSelectAllRequestId, and return false so the
async focus path or controlTextDidBeginEditing can consume the pending request;
update the caller in updateNSView that currently passes startEditingIfNeeded:
isFocused to stop requesting synchronous start-editing and rely on the existing
async makeFirstResponder/controlTextDidBeginEditing flow. Ensure
appliedSelectAllRequestId and publishSelectionState only update when selection
was actually applied to the editor.
In `@Sources/Workspace.swift`:
- Around line 8130-8135: The publishBrowserOpenTabSuggestion call is happening
before confirming the Bonsplit tab still exists, allowing stale suggestions to
be inserted after a panel is closed; move or guard the publish call so it only
runs when the tab is confirmed live: inside the sink closure, first resolve
tabId via surfaceIdFromPanelId(browserPanel.id) and then check
bonsplitController.tab(tabId) (the existing variable `existing`) before calling
publishBrowserOpenTabSuggestion(for: browserPanel), ensuring the publish happens
after the `bonsplitController.tab(tabId)` existence check (or re-check
immediately before publishing) so closed/detached tabs can't reinsert
suggestions.
🪄 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: addd2721-009f-4ee2-9cd8-ddc0701f83de
📒 Files selected for processing (6)
GhosttyTabs.xcodeproj/project.pbxprojSources/AppDelegate.swiftSources/Panels/BrowserOmnibarPerformanceSupport.swiftSources/Panels/BrowserPanelView.swiftSources/TabManager.swiftSources/Workspace.swift
💤 Files with no reviewable changes (1)
- Sources/TabManager.swift
✅ Files skipped from review due to trivial changes (1)
- GhosttyTabs.xcodeproj/project.pbxproj
| private var browserOpenTabSuggestionIndexesByManagerId: [ObjectIdentifier: BrowserOpenTabSuggestionIndex] = [:] | ||
|
|
||
| extension TabManager { | ||
| private var browserOpenTabSuggestionIndex: BrowserOpenTabSuggestionIndex { | ||
| let managerId = ObjectIdentifier(self) | ||
| if let index = browserOpenTabSuggestionIndexesByManagerId[managerId] { | ||
| return index | ||
| } | ||
| let index = BrowserOpenTabSuggestionIndex() | ||
| browserOpenTabSuggestionIndexesByManagerId[managerId] = index | ||
| return index |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Swift, is ObjectIdentifier guaranteed unique only for an object's lifetime, and can the same identifier be reused after the object is deallocated?
💡 Result:
Yes, ObjectIdentifier is guaranteed to be unique only during the lifetime of the object. The same identifier can be reused after the object is deallocated, as it is based on the object's memory address, which can be recycled by the runtime.
Citations:
- 1: https://developer.apple.com/documentation/swift/objectidentifier
- 2: https://github.com/apple/swift/blob/main/stdlib/public/core/ObjectIdentifier.swift
- 3: [SR-13564] ObjectIdentifier's equal has an unexpected result swiftlang/swift#56002
🏁 Script executed:
find . -type f -name "BrowserOmnibarPerformanceSupport.swift" | head -5Repository: manaflow-ai/cmux
Length of output: 116
🏁 Script executed:
wc -l Sources/Panels/BrowserOmnibarPerformanceSupport.swiftRepository: manaflow-ai/cmux
Length of output: 118
🏁 Script executed:
cat -n Sources/Panels/BrowserOmnibarPerformanceSupport.swiftRepository: manaflow-ai/cmux
Length of output: 10563
🏁 Script executed:
find . -type f -name "*.swift" | xargs rg "class TabManager" -A 5 | head -30Repository: manaflow-ai/cmux
Length of output: 2540
🏁 Script executed:
rg "browserOpenTabSuggestionIndexesByManagerId" --type swiftRepository: manaflow-ai/cmux
Length of output: 483
🏁 Script executed:
find . -type f -name "*.swift" | xargs rg "^class TabManager" -A 30 | head -50Repository: manaflow-ai/cmux
Length of output: 1790
🏁 Script executed:
rg "deinit" Sources -A 5 | grep -A 5 "TabManager\|browserOpenTab" | head -30Repository: manaflow-ai/cmux
Length of output: 679
🏁 Script executed:
sed -n '1,200p' Sources/TabManager.swift | tail -100Repository: manaflow-ai/cmux
Length of output: 3163
🏁 Script executed:
rg "deinit" Sources/TabManager.swift -A 20Repository: manaflow-ai/cmux
Length of output: 1010
🏁 Script executed:
rg "browserOpenTabSuggestionIndexesByManagerId\[" Sources -B 3 -A 3Repository: manaflow-ai/cmux
Length of output: 1067
🏁 Script executed:
rg "removeValue\|removeAll\|removeValue.*browserOpenTab" Sources -B 5 -A 5Repository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
rg "ObjectIdentifier" Sources -B 2 -A 2Repository: manaflow-ai/cmux
Length of output: 50372
🏁 Script executed:
grep -r "browserOpenTabSuggestion" Sources --include="*.swift" | grep -v "BrowserOmnibarPerformanceSupport"Repository: manaflow-ai/cmux
Length of output: 42
Add cleanup in TabManager.deinit to remove closed manager's suggestion index.
The global browserOpenTabSuggestionIndexesByManagerId dictionary keeps BrowserOpenTabSuggestionIndex entries for closed TabManager instances indefinitely—this is a memory leak. Additionally, since ObjectIdentifier is based on object memory address and can be reused after deallocation, a future TabManager may acquire the same identifier as a deallocated one and receive stale suggestion snapshots.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Panels/BrowserOmnibarPerformanceSupport.swift` around lines 143 -
153, Add cleanup in TabManager.deinit to remove its entry from the global
browserOpenTabSuggestionIndexesByManagerId map: in the TabManager type implement
a deinit that computes let managerId = ObjectIdentifier(self) and calls
browserOpenTabSuggestionIndexesByManagerId.removeValue(forKey: managerId) so
closed managers don't leak BrowserOpenTabSuggestionIndex instances or yield
stale snapshots; place the deinit alongside the browserOpenTabSuggestionIndex
computed property to ensure symmetry.
580e04e to
df5e529
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/Workspace.swift (1)
13302-13350:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCancel the browser panel subscription before
panel.close().In both close paths,
panel.close()runs while the browser subscription is still active. IfBrowserPanelemits a final title/URL change during shutdown, the sink can re-upsert the open-tab suggestion after you remove it. Tear downpanelSubscriptionsfirst, mirroringteardownAllPanels.Also applies to: 13505-13530
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 13302 - 13350, The panel's subscription may still be active when calling panel.close(), allowing BrowserPanel sinks to emit and re-insert suggestions; before calling panel.close() in both branches (the isDetaching branch where you create DetachedSurfaceTransfer and the else branch that may call onClosedBrowserPanel and then panel?.close()), first cancel/ remove the panel's subscription from panelSubscriptions (e.g., call panelSubscriptions.removeValue(forKey: panelId) or explicitly tear down the subscription for panelId) mirroring teardownAllPanels' ordering, then call panel.close(); ensure this change is applied to both close paths and any duplicate close logic around lines noted (also where removeBrowserOpenTabSuggestionIfNeeded is called) so subscriptions are always removed before closing BrowserPanel.
♻️ Duplicate comments (1)
Sources/Workspace.swift (1)
8125-8154:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMove the browser-suggestion upsert behind the live-tab guard.
The sink still publishes before confirming Bonsplit still owns the tab, and the eager seed here can also run while a detached panel is being reattached. That lets a closing panel repopulate the omnibar index.
Suggested reorder
.sink { [weak self, weak browserPanel] _, _, isLoading, favicon in guard let self = self, let browserPanel = browserPanel, let tabId = self.surfaceIdFromPanelId(browserPanel.id) else { return } - self.publishBrowserOpenTabSuggestion(for: browserPanel) guard let existing = self.bonsplitController.tab(tabId) else { return } + self.publishBrowserOpenTabSuggestion(for: browserPanel)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 8125 - 8154, The sink currently calls publishBrowserOpenTabSuggestion(for: browserPanel) before verifying Bonsplit still owns the tab; move that call to after the guard that checks let existing = self.bonsplitController.tab(tabId) so the suggestion is only upserted when the tab is known to be owned by Bonsplit and not for detached/reattaching panels. Specifically, inside the Combine sink closure (the subscription stored in panelSubscriptions[browserPanel.id]) remove the early publishBrowserOpenTabSuggestion(for: browserPanel) and instead call publishBrowserOpenTabSuggestion(for: browserPanel) immediately after the guard let existing = self.bonsplitController.tab(tabId) check (but before computing title/favicon/loading updates).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@Sources/Workspace.swift`:
- Around line 13302-13350: The panel's subscription may still be active when
calling panel.close(), allowing BrowserPanel sinks to emit and re-insert
suggestions; before calling panel.close() in both branches (the isDetaching
branch where you create DetachedSurfaceTransfer and the else branch that may
call onClosedBrowserPanel and then panel?.close()), first cancel/ remove the
panel's subscription from panelSubscriptions (e.g., call
panelSubscriptions.removeValue(forKey: panelId) or explicitly tear down the
subscription for panelId) mirroring teardownAllPanels' ordering, then call
panel.close(); ensure this change is applied to both close paths and any
duplicate close logic around lines noted (also where
removeBrowserOpenTabSuggestionIfNeeded is called) so subscriptions are always
removed before closing BrowserPanel.
---
Duplicate comments:
In `@Sources/Workspace.swift`:
- Around line 8125-8154: The sink currently calls
publishBrowserOpenTabSuggestion(for: browserPanel) before verifying Bonsplit
still owns the tab; move that call to after the guard that checks let existing =
self.bonsplitController.tab(tabId) so the suggestion is only upserted when the
tab is known to be owned by Bonsplit and not for detached/reattaching panels.
Specifically, inside the Combine sink closure (the subscription stored in
panelSubscriptions[browserPanel.id]) remove the early
publishBrowserOpenTabSuggestion(for: browserPanel) and instead call
publishBrowserOpenTabSuggestion(for: browserPanel) immediately after the guard
let existing = self.bonsplitController.tab(tabId) check (but before computing
title/favicon/loading updates).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c7371a5c-dba0-482e-ae6e-e7403eb0569c
📒 Files selected for processing (8)
GhosttyTabs.xcodeproj/project.pbxprojSources/AppDelegate.swiftSources/Panels/BrowserOmnibarPerformanceSupport.swiftSources/Panels/BrowserPanelView.swiftSources/TabManager.swiftSources/Workspace.swiftcmuxUITests/BrowserOmnibarSuggestionsTestSupport.swiftcmuxUITests/BrowserOmnibarSuggestionsUITests.swift
💤 Files with no reviewable changes (1)
- Sources/TabManager.swift
✅ Files skipped from review due to trivial changes (2)
- Sources/AppDelegate.swift
- Sources/Panels/BrowserPanelView.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/Panels/BrowserOmnibarPerformanceSupport.swift
df5e529 to
59ff346
Compare
59ff346 to
df4453e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cmuxUITests/BrowserOmnibarSuggestionsUITests.swift (2)
15-16:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStale comment still references the removed
browser_history.jsonfile.The comment says "history-save doesn't overwrite the seeded
browser_history.json", but seeding is now entirely in-memory viaCMUX_UI_TEST_BROWSER_HISTORY_JSON. The rationale for terminating a lingering app (debounced writes) may still be valid if the app writes other state, but the comment should reflect the new mechanism.✏️ Suggested update
- // Terminate any lingering app from a prior test so its debounced - // history-save doesn't overwrite the seeded browser_history.json. + // Terminate any lingering app from a prior test to avoid + // state bleed before the next launch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxUITests/BrowserOmnibarSuggestionsUITests.swift` around lines 15 - 16, Update the stale comment that references the removed browser_history.json: replace the line mentioning "seeded browser_history.json" with a concise note that the test seeds history in-memory via the CMUX_UI_TEST_BROWSER_HISTORY_JSON environment variable and that we terminate any lingering app to avoid its debounced background/state writes interfering with the in-memory seed; locate the comment immediately above the termination call in BrowserOmnibarSuggestionsUITests.swift near the app termination logic and reference CMUX_UI_TEST_BROWSER_HISTORY_JSON and "debounced writes" in the new comment.
586-612:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUnsafe JSON construction via string interpolation — malformed JSON silently drops the seed.
entry.urlandentry.titleare embedded in the JSON string without escaping. Any"or\in those values produces invalid JSON;JSONDecoderinBrowserHistoryStore.uiTestSeedEntriesIfConfigured()returnsnilon a decode error, meaning the seed is silently ignored. The test proceeds with no history, and the first assertion failure will be far downstream with no indication of why the seed was lost.All current call sites happen to use safe literals, but the function signature accepts arbitrary
SeedEntryvalues, making this a latent trap.🛡️ Proposed fix — JSON-encode values via `JSONSerialization`
Replace the manual string-building block with a proper serialisation approach:
- let entriesJSON = resolved.enumerated().reversed().map { index, entry in - let recencyOffset = index * 120 - var json = """ - { - "id": "\(UUID().uuidString)", - "url": "\(entry.url)", - "title": "\(entry.title)", - "lastVisited": \(now - Double(recencyOffset)), - "visitCount": \(entry.visitCount) - """ - if entry.typedCount > 0 { - json += """ - , - "typedCount": \(entry.typedCount), - "lastTypedAt": \(now - Double(recencyOffset)) - """ - } - json += "\n }" - return json - }.joined(separator: ",\n") - - let json = """ - [ - \(entriesJSON) - ] - """ - browserHistorySeedJSON = json + let dicts: [[String: Any]] = resolved.enumerated().reversed().map { index, entry in + let recencyOffset = Double(index * 120) + var d: [String: Any] = [ + "id": UUID().uuidString, + "url": entry.url, + "title": entry.title, + "lastVisited": now - recencyOffset, + "visitCount": entry.visitCount, + ] + if entry.typedCount > 0 { + d["typedCount"] = entry.typedCount + d["lastTypedAt"] = now - recencyOffset + } + return d + } + if let data = try? JSONSerialization.data(withJSONObject: dicts), + let json = String(data: data, encoding: .utf8) { + browserHistorySeedJSON = json + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxUITests/BrowserOmnibarSuggestionsUITests.swift` around lines 586 - 612, The test currently builds JSON by string-interpolating entry.url and entry.title (in the entriesJSON block), which can produce invalid JSON and silently drop the seed in BrowserHistoryStore.uiTestSeedEntriesIfConfigured(); replace the manual string construction with proper serialization: map resolved.enumerated() to an array of [String: Any] dictionaries (including id, url, title, lastVisited, visitCount and conditionally typedCount/lastTypedAt), then use JSONSerialization.data(withJSONObject:options:) (or JSONEncoder with a Codable struct) to create the JSON data/string and assign it to browserHistorySeedJSON; update any references to entriesJSON and ensure UUID().uuidString and numeric values are stored as native types so JSONSerialization correctly escapes strings and encodes numbers.
🤖 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`:
- Around line 2083-2088: The snapshot construction for
BrowserOpenTabSuggestionSnapshot is passing an optional by using
panel.preferredURLStringForOmnibar() which returns String? while
BrowserOpenTabSuggestionSnapshot.url is non-optional; update the url argument to
coalesce the optional (e.g., panel.preferredURLStringForOmnibar() ?? "")
following the same pattern used elsewhere in this file to eliminate the type
mismatch.
---
Outside diff comments:
In `@cmuxUITests/BrowserOmnibarSuggestionsUITests.swift`:
- Around line 15-16: Update the stale comment that references the removed
browser_history.json: replace the line mentioning "seeded browser_history.json"
with a concise note that the test seeds history in-memory via the
CMUX_UI_TEST_BROWSER_HISTORY_JSON environment variable and that we terminate any
lingering app to avoid its debounced background/state writes interfering with
the in-memory seed; locate the comment immediately above the termination call in
BrowserOmnibarSuggestionsUITests.swift near the app termination logic and
reference CMUX_UI_TEST_BROWSER_HISTORY_JSON and "debounced writes" in the new
comment.
- Around line 586-612: The test currently builds JSON by string-interpolating
entry.url and entry.title (in the entriesJSON block), which can produce invalid
JSON and silently drop the seed in
BrowserHistoryStore.uiTestSeedEntriesIfConfigured(); replace the manual string
construction with proper serialization: map resolved.enumerated() to an array of
[String: Any] dictionaries (including id, url, title, lastVisited, visitCount
and conditionally typedCount/lastTypedAt), then use
JSONSerialization.data(withJSONObject:options:) (or JSONEncoder with a Codable
struct) to create the JSON data/string and assign it to browserHistorySeedJSON;
update any references to entriesJSON and ensure UUID().uuidString and numeric
values are stored as native types so JSONSerialization correctly escapes strings
and encodes numbers.
🪄 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: 2423c613-1e71-4cf5-b9d8-1885a1d1b0c1
📒 Files selected for processing (8)
GhosttyTabs.xcodeproj/project.pbxprojSources/AppDelegate.swiftSources/Panels/BrowserOmnibarPerformanceSupport.swiftSources/Panels/BrowserPanel.swiftSources/Panels/BrowserPanelView.swiftSources/TabManager.swiftSources/Workspace.swiftcmuxUITests/BrowserOmnibarSuggestionsUITests.swift
💤 Files with no reviewable changes (1)
- Sources/TabManager.swift
✅ Files skipped from review due to trivial changes (2)
- GhosttyTabs.xcodeproj/project.pbxproj
- Sources/Workspace.swift
🚧 Files skipped from review as they are similar to previous changes (2)
- Sources/AppDelegate.swift
- Sources/Panels/BrowserOmnibarPerformanceSupport.swift
df4453e to
be550da
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmuxUITests/BrowserOmnibarSuggestionsUITests.swift (1)
570-612: ⚡ Quick winUse a real encoder for the seed payload.
Building JSON with string interpolation here is brittle: any seeded title or URL containing quotes, backslashes, or newlines will produce invalid launch data and the app-side decoder will silently fail. Encoding the array instead would make this test helper much safer.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxUITests/BrowserOmnibarSuggestionsUITests.swift` around lines 570 - 612, seedBrowserHistoryForTest builds JSON via string interpolation which breaks on quotes/backslashes/newlines; instead create an explicit payload array (e.g., map resolved.enumerated().reversed() into a struct/dictionary that includes id: UUID().uuidString, url, title, lastVisited: now - Double(recencyOffset), visitCount, and conditionally typedCount/lastTypedAt) and use JSONEncoder to encode that array to Data and then to String; replace the string-concatenation logic in seedBrowserHistoryForTest with this encoder-based flow so browserHistorySeedJSON is a safe, valid JSON string (reference SeedEntry, seedBrowserHistoryForTest, and browserHistorySeedJSON).
🤖 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/BrowserOmnibarSuggestionsUITests.swift`:
- Around line 541-545: The test testOmnibarSingleRowPopupUsesMinimumHeight must
explicitly seed browser history before app launch so it doesn't rely on on-disk
state; call seedBrowserHistoryForTest() at the start of
testOmnibarSingleRowPopupUsesMinimumHeight (before invoking
launchAndEnsureForeground(_:)) to ensure CMUX_UI_TEST_BROWSER_HISTORY_JSON is
set and stable for assertions, or alternatively make launchAndEnsureForeground(_
app: XCUIApplication, ...) always set CMUX_UI_TEST_BROWSER_HISTORY_JSON from
browserHistorySeedJSON (ensure browserHistorySeedJSON is non-optional when
used); update the test to call seedBrowserHistoryForTest() unless you choose the
helper change.
---
Nitpick comments:
In `@cmuxUITests/BrowserOmnibarSuggestionsUITests.swift`:
- Around line 570-612: seedBrowserHistoryForTest builds JSON via string
interpolation which breaks on quotes/backslashes/newlines; instead create an
explicit payload array (e.g., map resolved.enumerated().reversed() into a
struct/dictionary that includes id: UUID().uuidString, url, title, lastVisited:
now - Double(recencyOffset), visitCount, and conditionally
typedCount/lastTypedAt) and use JSONEncoder to encode that array to Data and
then to String; replace the string-concatenation logic in
seedBrowserHistoryForTest with this encoder-based flow so browserHistorySeedJSON
is a safe, valid JSON string (reference SeedEntry, seedBrowserHistoryForTest,
and browserHistorySeedJSON).
🪄 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: e5533dbb-dd0c-405a-897e-d81ac25d2e2f
📒 Files selected for processing (8)
GhosttyTabs.xcodeproj/project.pbxprojSources/AppDelegate.swiftSources/Panels/BrowserOmnibarPerformanceSupport.swiftSources/Panels/BrowserPanel.swiftSources/Panels/BrowserPanelView.swiftSources/TabManager.swiftSources/Workspace.swiftcmuxUITests/BrowserOmnibarSuggestionsUITests.swift
💤 Files with no reviewable changes (1)
- Sources/TabManager.swift
✅ Files skipped from review due to trivial changes (3)
- Sources/Panels/BrowserPanel.swift
- Sources/AppDelegate.swift
- Sources/Panels/BrowserPanelView.swift
🚧 Files skipped from review as they are similar to previous changes (3)
- Sources/Workspace.swift
- Sources/Panels/BrowserOmnibarPerformanceSupport.swift
- GhosttyTabs.xcodeproj/project.pbxproj
This branch was successfully deployed
1 participant
Summary
TabManagerinstead of scanning every workspace panel on each omnibar edit.Verification
/tmp/cmux-debug-scrollfast.sock: 140 workspaces,debug.typeinto the omnibar peaked at 137.26 ms./tmp/cmux-debug-omlag.sock: 81 workspaces, peak 48.02 ms, cleanup closed all generated workspaces../scripts/reload.sh --tag omlagSummary by cubic
Fixes browser omnibar typing lag by caching open‑tab suggestions per window and making focus/select‑all event‑driven. Matching stays fast with many workspaces, seeds once per window, and no longer scans all workspaces on each keystroke.
TabManager(window‑scoped) open‑tab suggestion index with lazy, one‑time seeding; query viamatchingOpenBrowserTabSuggestionswith single‑character prefix handling, deduping, stable order, and limits. Uses the correct window viatabManagerFor(tabId:)when needed.Workspacepublishes/upserts suggestion snapshots oncurrentURL/title changes (and on subscription install) and removes them on panel close, detach, and workspace teardown.AppDelegatepostscommandPaletteVisibilityDidChangeonly on real changes,BrowserPanelViewfilters by its window and applies pending focus on that notification, and the omnibar text field uses aselectAllRequestIdto select once the editor is ready. UI tests inject history viaCMUX_UI_TEST_BROWSER_HISTORY_JSON, and tests ensure the index seeds only once.Written for commit 7a09c17. Summary will update on new commits.
Summary by CodeRabbit
New Features
Improvements
Tests