feat(search): Cmd+F-style global all-tabs search palette - #5591
Supersynergy wants to merge 1 commit into
Conversation
|
@Supersynergy is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThis PR implements a comprehensive global cross-window search feature for the cmux application. It introduces a persistent SQLite FTS5 search index, a multi-component ranking pipeline (combining BM25, recency, and click-history priors), dual UI entry points (titlebar inline field and menubar status-item popover), global hotkey activation, and content capture hooks for browser and markdown panels. All components are wired together via AppDelegate, NotificationCenter, and FeedCoordinator focus events. ChangesGlobal Search Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (7 errors, 1 warning, 2 inconclusive)
✅ Passed checks (9 passed)
✨ Finishing Touches🧪 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 extracts the global search palette from
Confidence Score: 3/5Not safe to merge without addressing the missing translations and the preview/activate conflation. Three concrete defects need resolution: (1) four new string-catalog keys ship without translations for 16+ already-supported locales, so every non-English/non-Japanese user sees raw English text in the new scope toggles and navigation buttons; (2)
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant GlobalSearchPaletteView
participant GlobalSearchCoordinator
participant AppDelegate
User->>GlobalSearchPaletteView: Open (⌥⌘F / menubar / titlebar)
GlobalSearchPaletteView->>GlobalSearchCoordinator: refreshLiveIndex()
GlobalSearchCoordinator-->>GlobalSearchPaletteView: done
User->>GlobalSearchPaletteView: Type query
GlobalSearchPaletteView->>GlobalSearchPaletteView: scheduleSearch (DispatchSourceTimer 80ms)
GlobalSearchPaletteView->>GlobalSearchCoordinator: search(query, limit)
GlobalSearchCoordinator-->>GlobalSearchPaletteView: [SearchIndexHit]
GlobalSearchPaletteView->>GlobalSearchPaletteView: applyHits (filter by activeKindIDs)
User->>GlobalSearchPaletteView: ↑/↓ arrow key
GlobalSearchPaletteView->>GlobalSearchCoordinator: preview(hit, query)
GlobalSearchCoordinator->>AppDelegate: openGlobalSearchHit(hit, query)
Note over GlobalSearchCoordinator,AppDelegate: ⚠️ Same call as activate — hard navigation
User->>GlobalSearchPaletteView: ↵ Enter
GlobalSearchPaletteView->>GlobalSearchCoordinator: activate(hit, query)
GlobalSearchCoordinator->>AppDelegate: openGlobalSearchHit(hit, query)
AppDelegate-->>User: Focus window → workspace → panel
Reviews (2): Last reviewed commit: "feat(search): polish global search palet..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| debounce?.cancel() | ||
| debounce = Task { [weak self] in | ||
| try? await Task.sleep(nanoseconds: 600_000_000) | ||
| guard !Task.isCancelled, let self else { return } | ||
| await self.capture() | ||
| } | ||
| } |
There was a problem hiding this comment.
Task.sleep for debounce synchronization in production
Task.sleep is not an allowed debounce mechanism in shipped app code per cmux's blocking-runtime rule — it holds a task alive for a fixed wall-clock interval with no backpressure, is not cancellation-aware in the error path (only via Task.isCancelled), and does not compose with the scheduler. The same pattern appears in TitlebarSearchField.preview (120 ms). A Clock-based debouncer, an AsyncStream-backed pipeline, or an explicit DispatchSourceTimer with a real cancellation path should replace both instances.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| text: $query) | ||
| .textFieldStyle(.plain) | ||
| .font(.system(size: 16)) | ||
| .onChange(of: query) { _, new in Task { await refresh(new) } } |
There was a problem hiding this comment.
Task for refresh causes stale results
Task { await refresh(new) } is never stored or cancelled. Every keystroke while the palette is open creates an independent concurrent task; if an earlier (slower) search for a longer query completes after a shorter one, hits is overwritten with stale results. The same pattern appears in the onChange(of: scopes) handler on line 187 and in TitlebarSearchField line 49. The fix is to store the refresh task handle and cancel the previous one before scheduling a new one, matching the pattern already used for previewTask in TitlebarSearchField.
| private func save() { | ||
| try? FileManager.default.createDirectory( | ||
| at: storeURL.deletingLastPathComponent(), | ||
| withIntermediateDirectories: true) | ||
| if let data = try? JSONEncoder().encode(stats) { | ||
| try? data.write(to: storeURL, options: .atomic) | ||
| } | ||
| } |
There was a problem hiding this comment.
@MainActor on every impression
save() does a synchronous data.write(to:options:.atomic) directly on the main actor. It is called by recordImpressions (once per visible result set) and reward (on every accept). On a busy session with dozens of results, this writes to disk on the UI thread each time the search palette refreshes, which can cause measurable frame drops. The stats should be persisted asynchronously — either with a stored Task.detached that coalesces dirty-flushes, or by moving save to a background actor.
| public func upsert( | ||
| windowID: UUID, workspaceID: UUID, panelID: UUID, | ||
| kind: Kind, anchor: String, text: String | ||
| ) { | ||
| let sql = """ | ||
| INSERT INTO chunks(window_id, workspace_id, panel_id, kind, ts, anchor, text) | ||
| VALUES(?, ?, ?, ?, ?, ?, ?) | ||
| """ | ||
| var stmt: OpaquePointer? | ||
| guard sqlite3_prepare_v2(db, sql, -1, &stmt, nil) == SQLITE_OK else { return } | ||
| defer { sqlite3_finalize(stmt) } | ||
| sqlite3_bind_text(stmt, 1, windowID.uuidString, -1, nil) | ||
| sqlite3_bind_text(stmt, 2, workspaceID.uuidString, -1, nil) | ||
| sqlite3_bind_text(stmt, 3, panelID.uuidString, -1, nil) | ||
| sqlite3_bind_text(stmt, 4, kind.rawValue, -1, nil) | ||
| sqlite3_bind_double(stmt, 5, Date().timeIntervalSince1970) | ||
| sqlite3_bind_text(stmt, 6, anchor, -1, nil) | ||
| sqlite3_bind_text(stmt, 7, text, -1, nil) | ||
| sqlite3_step(stmt) | ||
| } |
There was a problem hiding this comment.
upsert always inserts — FTS5 index grows unboundedly
The function is named upsert but executes a bare INSERT, not DELETE … INSERT or any form of row replacement. FTS5 virtual tables do not support INSERT OR REPLACE. Every call from CmuxBrowserCaptureHook (once per finished navigation) and CmuxMarkdownCaptureHook (once per save) appends a new row for the same (panel_id, anchor) pair. Between panel closes (the only purge trigger), the same content is indexed repeatedly, inflating BM25 scores for frequently-visited panels and growing the on-disk database without bound. The correct shape is: DELETE FROM chunks WHERE panel_id = ? AND anchor = ? before the INSERT, or a MERGE/UPDATE OR INSERT strategy keyed on (panel_id, anchor).
| deinit { | ||
| if db != nil { sqlite3_close(db) } |
There was a problem hiding this comment.
deinit accesses isolated state without isolation
In Swift 6, deinit of an actor is implicitly nonisolated, so reading db (an actor-isolated var) from deinit is technically unsound — the compiler emits a warning under strict concurrency. The standard fix is to hoist the pointer into a local nonisolated let at the point where isolation is still valid (e.g., in a close() method called explicitly before deallocation), or to capture the raw pointer in a nonisolated stored property.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/menubar-global-search.md`:
- Around line 73-83: Add the required blank lines around fenced code blocks and
after the headings "## Storage", "## Localization", and "## Non-goals" to
satisfy MD031/MD022: ensure there's a blank line above and below each ```fenced
block``` (including the Swift snippet with TitlebarSearchField and the
AppDelegate snippet) and add a single blank line immediately after each of the
three headings so the markdown linter stops reporting spacing violations; edit
the docs/menubar-global-search.md heading and code-block regions to insert these
blank lines.
- Around line 12-16: Reword the conflicting lines to state that the menubar-only
plan was dropped but dual entry points remain: update the paragraph mentioning
`NSStatusItem`/`MenubarSearchPopover` so it reads something like “menubar-only
plan dropped; dual entry points (titlebar + menubar) retained,” and keep the
`magnifyingglass` menubar item and `MenubarSearchPopover` wiring references to
avoid confusion about the menubar still being present alongside the titlebar
entry.
- Around line 61-67: The doc still references a Synapse-backed path and
components (e.g. SynapseBridge, ~/.synapse) even though the design is
local-only; remove all references to SynapseBridge and any ~/.synapse storage
path and update the P1/P3/Storage sections to state the shipped FTS5-only, local
architecture. Specifically: delete SynapseBridge from the P1 scaffold list,
remove any mentions of Synapse, SynapseBridge, or Unix-socket clients in P3 and
Storage paragraphs, and replace storage details with the local FTS5 storage
description (FTS5 actor / SearchIndex and local file locations) so the doc
strictly reflects the FTS5-only design.
In `@Sources/AppDelegate.swift`:
- Around line 1017-1043: AppDelegate contains bootstrapping for global search
(GlobalSearchHotkey.shared.install(),
MenubarSearchPopover.shared.installStatusItem(index:), and the two
NotificationCenter observers handling .cmuxJumpToSearchHit and
.cmuxPreviewSearchHit that call FeedCoordinator.focus(workspaceId:surfaceId:)),
so extract that logic into a new integration type (e.g., GlobalSearchIntegration
or GlobalSearchBootstrap) that encapsulates installing the hotkey, installing
the menubar popover, and registering the two notification observers; expose a
simple install() or start() method on that type and replace the inline setup in
AppDelegate with a single call to the new integration to keep AppDelegate
limited to composition wiring and remove feature orchestration from the
app-target root.
In `@Sources/Search/BrowserCaptureHook.swift`:
- Around line 47-52: CmuxBrowserCaptureHook.install currently replaces
webView.navigationDelegate with the hook but only forwards two methods, breaking
other optional WKNavigationDelegate callbacks; fix this by making
CmuxBrowserCaptureHook inherit from NSObject and implement robust selector
forwarding: add a weak var forwardDelegate: WKNavigationDelegate? (already used)
and override responds(to:) to return true if super or forwardDelegate responds
to the selector, and implement forwardingTarget(for:) to return forwardDelegate
for selectors the hook doesn’t implement, so all optional WKNavigationDelegate
methods (policy, auth, navigation, etc.) are forwarded transparently; ensure
these overrides live on the CmuxBrowserCaptureHook type so setting
webView.navigationDelegate = hook and objc_setAssociatedObject(webView,
&keyHolder, hook, .OBJC_ASSOCIATION_RETAIN) preserves previously implemented
delegate behavior.
In `@Sources/Search/GlobalSearchHotkey.swift`:
- Around line 29-33: The defaultChords array in GlobalSearchHotkey.swift
hard-codes cmux-owned chords (Chord instances) instead of wiring them through
the existing KeyboardShortcutSettings/configurable path; replace the hard-coded
defaultChords usage with a retrieval from KeyboardShortcutSettings (falling back
to the proper defaults defined in settings.json), register the chords from
KeyboardShortcutSettings (or its helper API) so they become editable in Settings
and persisted to ~/.config/cmux/cmux.json, and ensure the Chord id values and
keyCode/modifier mapping match the settings schema so documentation and settings
export/import work correctly.
- Around line 50-58: Reviewer notes that the DispatchQueue.main.async inside the
InstallEventHandler legacy callback is an allowed minimal UI hop; either leave
it as-is or modernize by replacing the GCD hop with a main-actor Task to avoid
explicit GCD usage. Specifically, in the InstallEventHandler callback that
references NSApp.keyWindow, NotificationCenter.default.post(name:
.cmuxFocusTitlebarSearch, ...) and MenubarSearchPopover.shared.toggle(), you can
remove DispatchQueue.main.async and wrap the UI work in Task { `@MainActor` in ...
} so the checks and calls run on the main actor instead of using DispatchQueue.
In `@Sources/Search/MarkdownCaptureHook.swift`:
- Around line 15-31: Replace the no-case namespace enum CmuxMarkdownCaptureHook
with a value-typed struct to allow behavior composition and DI: change
CmuxMarkdownCaptureHook to a public struct and convert the static `@MainActor`
feed(text:windowID:workspaceID:panelID:anchor:index:) into an instance
`@MainActor` method (or provide an instance method and a small static convenience
initializer if needed), keep the same parameter list and internal logic (guard
let index, truncate payload, Task.detached calling index.upsert), and update
callers to obtain/inject an instance of CmuxMarkdownCaptureHook instead of
calling a static method; this preserves behavior while enabling
testing/injection.
In `@Sources/Search/MenubarSearchPopover.swift`:
- Around line 36-38: The accessibilityDescription and any user-facing literals
passed into NSImage(systemSymbolName:accessibilityDescription:) (in
MenubarSearchPopover) must be replaced with localized string lookups (e.g.
NSLocalizedString or a Localization helper) rather than hardcoded English;
update the two occurrences that set accessibilityDescription and the
tooltip/title strings referenced in this file to use localized keys and add
matching entries to the app’s Localizable.strings (and comment strings for
translators). Specifically, change the literals used in the NSImage initializers
and any tooltip/accessibility text in MenubarSearchPopover to
NSLocalizedString("MENU_SEARCH_TOOLTIP", comment: "Search cmux windows and
tabs") (and a separate key for VoiceOver if needed), and ensure corresponding
keys are added to the translation catalog for all supported locales.
- Around line 174-175: The async refresh invocations are racy: store the
in-flight Task in a property (e.g., refreshTask: Task<Void, Never>?), cancel any
existing task before starting a new one, then assign refreshTask = Task { await
refresh(newQuery) } from both onChange(of: query) and the other caller (the
block that sets hits at refresh -> hits assignment), and ensure refresh respects
cancellation (check Task.isCancelled or return early) so a slower previous query
cannot overwrite hits; update references to onChange(of: query) and the
refresh(new)/hits update flow accordingly.
- Around line 84-220: This file defines multiple major types — extension
SearchIndex.Kind, ScopeToggleBar, SearchPaletteView, and HitRow — which must be
split into one major type per file; move each top-level declaration into its own
Swift file named after the type (ScopeToggleBar.swift, SearchPaletteView.swift,
HitRow.swift, and if needed SearchIndex+Kind.swift or MenubarSearchPopover.swift
for the popover type), preserve all property signatures, bindings, and
initializer signatures (e.g. ScopeToggleBar(enabled:),
SearchPaletteView(index:onPick:), HitRow(hit:hotkey:)), keep any modifiers and
imports, update any internal references or access levels if necessary, and
ensure the original file only contains the single remaining major type (or just
a small coordinating file) so compilation and symbol references stay unchanged.
In `@Sources/Search/SearchIndex.swift`:
- Around line 54-61: The upsert function currently uses a plain INSERT that
always appends duplicates; modify the SQL in the upsert function (the sql
variable in upsert) to perform a real upsert — either use SQLite syntax "INSERT
OR REPLACE INTO chunks(...)" or use "INSERT INTO chunks(...) VALUES(...) ON
CONFLICT(<unique key columns>) DO UPDATE SET kind=excluded.kind, ts=excluded.ts,
anchor=excluded.anchor, text=excluded.text" and ensure the conflict target
matches the unique constraint you want (e.g., window_id, workspace_id, panel_id,
anchor) so panel content is replaced instead of duplicated.
In `@Sources/Search/SmartRanker.swift`:
- Around line 55-62: The code is synchronously calling save() inside
recordImpressions (and similarly at the refresh path around the stats
persistence), causing JSON encode + atomic file writes on `@MainActor`; remove the
direct save() call from recordImpressions and instead batch/debounce
persistence: keep updating the in-memory stats (stats, key(_:)), and schedule an
async non‑blocking persistence task that performs encoding and atomic write off
the main actor (e.g., a single shared Task/DispatchQueue or a coalescing timer
that calls saveAsync/persistStats), and ensure save/saveAsync runs off the main
actor and is safe for concurrent updates.
In `@Sources/Search/TitlebarSearchField.swift`:
- Around line 91-94: The selector currently accepts any modifier set that
includes .command (press.modifiers.contains(.command)), which lets chords like
⌘⌥1 or ⌘⇧1 match; change the modifier check in the .onKeyPress closure to
require exact command-only modifiers (e.g., replace contains(.command) with a
strict equality check like press.modifiers == .command or an exact OptionSet
comparison) so that only pure Cmd+digit triggers the hits selection logic; keep
the rest of the guard (Int(press.characters), hits.indices.contains(n - 1))
unchanged.
- Line 188: Replace the direct user-facing string usage in TitlebarSearchField
(Text(hit.kind.rawValue.uppercased())) with a localized catalog-backed key: add
localized keys for each Hit.Kind (e.g. "result_kind_file" / "result_kind_folder"
etc.) in Localizable.strings, add a computed property on the Hit.Kind enum (e.g.
var localizedKey: LocalizedStringKey {
LocalizedStringKey("result_kind_\(rawValue)") } or var localizedString: String {
NSLocalizedString("result_kind_\(rawValue)", comment: "") }), then update the
view to use Text(kind.localizedKey) (or
Text(kind.localizedString).textCase(.uppercase) if you need uppercase) so the
badge label is served from the localization catalog rather than rawValue.
- Line 76: Cancel the pending preview debounce task when the search is closed or
a hit is accepted: locate the debounce scheduling code that queues the preview
post (the preview debounce Task/DispatchWorkItem used around Line 170 that
ultimately posts .cmuxPreviewSearchHit) and add logic to cancel that scheduled
work from the .onKeyPress(.escape) handler (the block that sets fieldFocus =
false) and from the code path that accepts a hit / calls pick (the dismiss logic
around Lines 156–162 and 166–175). Specifically, call cancel() on the stored
preview task (e.g., previewDebounceTask.cancel() or previewTask?.cancel()) and
clear the reference (set to nil) before closing/dismissing so no late
.cmuxPreviewSearchHit is posted.
- Around line 49-50: The refresh tasks launched from the onChange and other
paths can overlap and let older tasks later call recordImpressions and set hits
for a newer query; to fix, add a stored Task? property (e.g. refreshTask) and
cancel any existing refreshTask before creating a new one when calling
refresh(query) (references: refresh, query, onChange handler), assign the newly
created Task to refreshTask, and inside the async refresh function check
Task.isCancelled and/or verify the query still matches the current query before
calling recordImpressions or assigning hits (references: recordImpressions,
hits) so stale tasks cannot mutate state or pollute ranking feedback.
In `@Sources/Update/UpdateTitlebarAccessory.swift`:
- Around line 582-584: The shortcut hint X positions are computed in
titlebarButtonRightEdge(for:) but it still assumes three adjacent fixed-size
buttons and doesn't account for the inserted TitlebarSearchField; update
titlebarButtonRightEdge(for:) (and any helper that computes the
.newTab/.newWorkspace offsets) to include the TitlebarSearchField’s width
(TitlebarSearchField / its frame width 220 or config.buttonSize where
appropriate) when calculating the right-edge X offsets so the shortcut pill
offsets (e.g., for .newTab/.newWorkspace) shift right by the search field width;
ensure the same correction is applied to the other affected call site(s) noted
around the later block (also around the 681-684 region).
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: dfbcb9c4-d362-44f3-a9c1-6e5e5ad6eeab
📒 Files selected for processing (11)
GhosttyTabs.xcodeproj/project.pbxprojSources/AppDelegate.swiftSources/Search/BrowserCaptureHook.swiftSources/Search/GlobalSearchHotkey.swiftSources/Search/MarkdownCaptureHook.swiftSources/Search/MenubarSearchPopover.swiftSources/Search/SearchIndex.swiftSources/Search/SmartRanker.swiftSources/Search/TitlebarSearchField.swiftSources/Update/UpdateTitlebarAccessory.swiftdocs/menubar-global-search.md
| (`NSStatusItem`) plan dropped — inline is one click closer, per-window, | ||
| and shares focus with the rest of the titlebar controls. | ||
|
|
||
| **Menubar status item is now wired** (`MenubarSearchPopover`): a | ||
| `magnifyingglass` item in the system menubar opens the same palette, |
There was a problem hiding this comment.
Clarify the menubar statement to avoid a self-contradiction.
Line 12 says the menubar plan was dropped, while Line 15 says the menubar item is now wired. Reword this as “menubar-only plan dropped; dual entry points (titlebar + menubar) retained.”
🧰 Tools
🪛 LanguageTool
[grammar] ~13-~13: Ensure spelling is correct
Context: ..., and shares focus with the rest of the titlebar controls. **Menubar status item is now...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/menubar-global-search.md` around lines 12 - 16, Reword the conflicting
lines to state that the menubar-only plan was dropped but dual entry points
remain: update the paragraph mentioning `NSStatusItem`/`MenubarSearchPopover` so
it reads something like “menubar-only plan dropped; dual entry points (titlebar
+ menubar) retained,” and keep the `magnifyingglass` menubar item and
`MenubarSearchPopover` wiring references to avoid confusion about the menubar
still being present alongside the titlebar entry.
| - **P1 (this PR)** — scaffold: | ||
| - `SearchIndex` (FTS5 actor) | ||
| - `SynapseBridge` (Unix-socket client, graceful no-op on miss) | ||
| - `SmartRanker` (BM25 + recency + Thompson click-history) | ||
| - `TitlebarSearchField` (inline SwiftUI field, popover results) | ||
| - `MenubarSearchPopover` (opt-in fallback) | ||
| - `GlobalSearchHotkey` (default `⌥⌘F` → focus the inline field) |
There was a problem hiding this comment.
Remove stale SynapseBridge references from the phase and storage plan.
This doc still describes a Synapse-backed path (SynapseBridge, ~/.synapse/) even though this PR’s architecture is explicitly local-only and notes Synapse was dropped. Please align P1/P3/Storage text with the shipped FTS5-only design to avoid misleading follow-up work.
Also applies to: 88-90, 101-105
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/menubar-global-search.md` around lines 61 - 67, The doc still references
a Synapse-backed path and components (e.g. SynapseBridge, ~/.synapse) even
though the design is local-only; remove all references to SynapseBridge and any
~/.synapse storage path and update the P1/P3/Storage sections to state the
shipped FTS5-only, local architecture. Specifically: delete SynapseBridge from
the P1 scaffold list, remove any mentions of Synapse, SynapseBridge, or
Unix-socket clients in P3 and Storage paragraphs, and replace storage details
with the local FTS5 storage description (FTS5 actor / SearchIndex and local file
locations) so the doc strictly reflects the FTS5-only design.
| ```swift | ||
| TitlebarSearchField(index: AppDelegate.shared?.searchIndex) | ||
| .frame(width: 220) | ||
| ``` | ||
| - In `AppDelegate`, lazily create the shared index: | ||
| ```swift | ||
| let url = URL.applicationSupportDirectory | ||
| .appending(path: "cmux/search.db") | ||
| self.searchIndex = try? SearchIndex(url: url) | ||
| GlobalSearchHotkey.shared.install() | ||
| ``` |
There was a problem hiding this comment.
Fix markdownlint spacing violations around fenced blocks and headings.
Please add required blank lines around the fenced code blocks and after ## Storage, ## Localization, and ## Non-goals to satisfy MD031/MD022.
Also applies to: 101-111
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 73-73: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 76-76: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 78-78: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 83-83: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/menubar-global-search.md` around lines 73 - 83, Add the required blank
lines around fenced code blocks and after the headings "## Storage", "##
Localization", and "## Non-goals" to satisfy MD031/MD022: ensure there's a blank
line above and below each ```fenced block``` (including the Swift snippet with
TitlebarSearchField and the AppDelegate snippet) and add a single blank line
immediately after each of the three headings so the markdown linter stops
reporting spacing violations; edit the docs/menubar-global-search.md heading and
code-block regions to insert these blank lines.
Source: Linters/SAST tools
| // Global search: ⇧⌘F / ⌥⌘F focus the inline titlebar field (key | ||
| // window) or toggle the menubar palette. Tracking #3865. | ||
| GlobalSearchHotkey.shared.install() | ||
|
|
||
| // Menubar status item → cross-window/cross-tab search palette. | ||
| MenubarSearchPopover.shared.installStatusItem(index: searchIndex) | ||
|
|
||
| // Bridge: a picked search hit → focus its workspace+surface via | ||
| // the existing Feed focus pathway (so the user lands in the | ||
| // correct window/workspace/panel without inventing a new route). | ||
| NotificationCenter.default.addObserver( | ||
| forName: .cmuxJumpToSearchHit, object: nil, queue: .main | ||
| ) { note in | ||
| guard let hit = note.object as? SearchIndex.Hit else { return } | ||
| FeedCoordinator.focus( | ||
| workspaceId: hit.workspaceID.uuidString, | ||
| surfaceId: hit.panelID.uuidString) | ||
| } | ||
| // Soft-focus while arrowing — same focus path, no reward/dismiss. | ||
| NotificationCenter.default.addObserver( | ||
| forName: .cmuxPreviewSearchHit, object: nil, queue: .main | ||
| ) { note in | ||
| guard let hit = note.object as? SearchIndex.Hit else { return } | ||
| FeedCoordinator.focus( | ||
| workspaceId: hit.workspaceID.uuidString, | ||
| surfaceId: hit.panelID.uuidString) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Extract global-search bootstrap from AppDelegate into a dedicated integration type.
This adds more feature orchestration to an already oversized app-target file. Move hotkey/popover/notification bridge setup into a focused integration unit and keep AppDelegate to composition wiring.
As per coding guidelines, production Swift files under Sources/ should avoid accumulating independent feature logic in the app-target root and should enforce file-size/single-responsibility discipline.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Sources/AppDelegate.swift` around lines 1017 - 1043, AppDelegate contains
bootstrapping for global search (GlobalSearchHotkey.shared.install(),
MenubarSearchPopover.shared.installStatusItem(index:), and the two
NotificationCenter observers handling .cmuxJumpToSearchHit and
.cmuxPreviewSearchHit that call FeedCoordinator.focus(workspaceId:surfaceId:)),
so extract that logic into a new integration type (e.g., GlobalSearchIntegration
or GlobalSearchBootstrap) that encapsulates installing the hotkey, installing
the menubar popover, and registering the two notification observers; expose a
simple install() or start() method on that type and replace the inline setup in
AppDelegate with a single call to the new integration to keep AppDelegate
limited to composition wiring and remove feature orchestration from the
app-target root.
Source: Coding guidelines
| // Retain via objc associated object; chain the previous delegate. | ||
| hook.forwardDelegate = webView.navigationDelegate | ||
| webView.navigationDelegate = hook | ||
| objc_setAssociatedObject( | ||
| webView, &keyHolder, hook, .OBJC_ASSOCIATION_RETAIN) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="Sources/Search/BrowserCaptureHook.swift"
# Show the relevant region around the reported lines
sed -n '1,140p' "$FILE" | cat -n
# Also capture any delegate methods further down (just in case Lines 64-73 are offset)
rg -n "WKNavigationDelegate|didFinish|didFailProvisionalNavigation|install\(" "$FILE" || true
rg -n "forwardDelegate|objc_setAssociatedObject|keyHolder" "$FILE" || trueRepository: manaflow-ai/cmux
Length of output: 5623
Critical: WKNavigationDelegate forwarding is incomplete after swapping delegates in CmuxBrowserCaptureHook.install.
Sources/Search/BrowserCaptureHook.swift replaces webView.navigationDelegate with CmuxBrowserCaptureHook (around lines 47-52) and only forwards webView(_:didFinish:) + webView(_:didFailProvisionalNavigation:withError:) (around lines 64-73). Any other optional WKNavigationDelegate methods implemented by the previous delegate will no longer be called, breaking navigation/policy/auth flows that rely on them.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Sources/Search/BrowserCaptureHook.swift` around lines 47 - 52,
CmuxBrowserCaptureHook.install currently replaces webView.navigationDelegate
with the hook but only forwards two methods, breaking other optional
WKNavigationDelegate callbacks; fix this by making CmuxBrowserCaptureHook
inherit from NSObject and implement robust selector forwarding: add a weak var
forwardDelegate: WKNavigationDelegate? (already used) and override responds(to:)
to return true if super or forwardDelegate responds to the selector, and
implement forwardingTarget(for:) to return forwardDelegate for selectors the
hook doesn’t implement, so all optional WKNavigationDelegate methods (policy,
auth, navigation, etc.) are forwarded transparently; ensure these overrides live
on the CmuxBrowserCaptureHook type so setting webView.navigationDelegate = hook
and objc_setAssociatedObject(webView, &keyHolder, hook,
.OBJC_ASSOCIATION_RETAIN) preserves previously implemented delegate behavior.
| .onChange(of: query) { _, new in Task { await refresh(new) } } | ||
| .onSubmit { acceptSelected() } |
There was a problem hiding this comment.
Uncancelled refresh tasks can both show stale hits and corrupt ranking feedback.
Lines 49 and 75 launch overlapping refresh tasks; an older task can later run Line 132 (recordImpressions) and Line 135 (hits = ...) after a newer query, polluting SmartRanker stats and UI state.
Also applies to: 75-75, 114-137
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Sources/Search/TitlebarSearchField.swift` around lines 49 - 50, The refresh
tasks launched from the onChange and other paths can overlap and let older tasks
later call recordImpressions and set hits for a newer query; to fix, add a
stored Task? property (e.g. refreshTask) and cancel any existing refreshTask
before creating a new one when calling refresh(query) (references: refresh,
query, onChange handler), assign the newly created Task to refreshTask, and
inside the async refresh function check Task.isCancelled and/or verify the query
still matches the current query before calling recordImpressions or assigning
hits (references: recordImpressions, hits) so stale tasks cannot mutate state or
pollute ranking feedback.
| .frame(width: 480, height: 354) | ||
| } | ||
| .onChange(of: scopes) { _, _ in Task { await refresh(query) } } | ||
| .onKeyPress(.escape) { fieldFocus = false; return .handled } |
There was a problem hiding this comment.
Cancel pending preview debounce when search closes or a hit is accepted.
A queued preview task from Line 170 can still fire after Escape (Line 76) or after pick dismisses (Lines 160–162), causing unexpected late .cmuxPreviewSearchHit posts.
Also applies to: 156-162, 166-175
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Sources/Search/TitlebarSearchField.swift` at line 76, Cancel the pending
preview debounce task when the search is closed or a hit is accepted: locate the
debounce scheduling code that queues the preview post (the preview debounce
Task/DispatchWorkItem used around Line 170 that ultimately posts
.cmuxPreviewSearchHit) and add logic to cancel that scheduled work from the
.onKeyPress(.escape) handler (the block that sets fieldFocus = false) and from
the code path that accepts a hit / calls pick (the dismiss logic around Lines
156–162 and 166–175). Specifically, call cancel() on the stored preview task
(e.g., previewDebounceTask.cancel() or previewTask?.cancel()) and clear the
reference (set to nil) before closing/dismissing so no late
.cmuxPreviewSearchHit is posted.
| .onKeyPress(keys: Set("123456789".map { KeyEquivalent($0) })) { press in | ||
| guard press.modifiers.contains(.command), | ||
| let n = Int(press.characters), hits.indices.contains(n - 1) else { | ||
| return .ignored |
There was a problem hiding this comment.
Cmd+digit should require exact command-only modifiers.
Line 92 uses contains(.command), so chords like ⌘⌥1/⌘⇧1 can be consumed unintentionally by search selection logic.
Proposed fix
- guard press.modifiers.contains(.command),
+ guard press.modifiers == [.command],
let n = Int(press.characters), hits.indices.contains(n - 1) else {
return .ignored
}Based on learnings, keyboard handlers in this repo should treat command-navigation chords as exact modifier sets (not broad contains checks) to avoid hijacking other shortcuts.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Sources/Search/TitlebarSearchField.swift` around lines 91 - 94, The selector
currently accepts any modifier set that includes .command
(press.modifiers.contains(.command)), which lets chords like ⌘⌥1 or ⌘⇧1 match;
change the modifier check in the .onKeyPress closure to require exact
command-only modifiers (e.g., replace contains(.command) with a strict equality
check like press.modifiers == .command or an exact OptionSet comparison) so that
only pure Cmd+digit triggers the hits selection logic; keep the rest of the
guard (Int(press.characters), hits.indices.contains(n - 1)) unchanged.
Source: Learnings
| ScrollViewReader { proxy in | ||
| List(Array(hits.enumerated()), id: \.offset, selection: $selection) { idx, hit in | ||
| HStack(spacing: 8) { | ||
| Text(hit.kind.rawValue.uppercased()) |
There was a problem hiding this comment.
Localize the result-kind badge label.
Line 188 renders hit.kind.rawValue.uppercased() directly, which is user-facing text and not localizable.
As per coding guidelines, production user-facing Swift text must come from localized APIs with catalog-backed entries for supported locales.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Sources/Search/TitlebarSearchField.swift` at line 188, Replace the direct
user-facing string usage in TitlebarSearchField
(Text(hit.kind.rawValue.uppercased())) with a localized catalog-backed key: add
localized keys for each Hit.Kind (e.g. "result_kind_file" / "result_kind_folder"
etc.) in Localizable.strings, add a computed property on the Hit.Kind enum (e.g.
var localizedKey: LocalizedStringKey {
LocalizedStringKey("result_kind_\(rawValue)") } or var localizedString: String {
NSLocalizedString("result_kind_\(rawValue)", comment: "") }), then update the
view to use Text(kind.localizedKey) (or
Text(kind.localizedString).textCase(.uppercase) if you need uppercase) so the
badge label is served from the localization catalog rather than rawValue.
Source: Coding guidelines
| TitlebarSearchField(index: AppDelegate.shared?.searchIndex) | ||
| .frame(width: 220, height: config.buttonSize) | ||
|
|
There was a problem hiding this comment.
Shortcut-hint geometry is now incorrect for the New Workspace button.
After inserting TitlebarSearchField before the plus button, titlebarButtonRightEdge(for:) still assumes three adjacent fixed-size buttons. The .newTab hint offset no longer includes the search field width, so its shortcut pill is rendered in the wrong X position.
Suggested fix
+private let titlebarSearchFieldWidth: CGFloat = 220
+
private func titlebarButtonRightEdge(for slot: HintSlot, config: TitlebarControlsStyleConfig) -> CGFloat {
- let index = CGFloat(slot.rawValue)
- return (index + 1) * config.buttonSize + index * config.spacing
+ switch slot {
+ case .toggleSidebar:
+ return config.buttonSize
+ case .showNotifications:
+ return (2 * config.buttonSize) + config.spacing
+ case .newTab:
+ return (3 * config.buttonSize)
+ + (3 * config.spacing)
+ + titlebarSearchFieldWidth
+ }
}Also applies to: 681-684
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Sources/Update/UpdateTitlebarAccessory.swift` around lines 582 - 584, The
shortcut hint X positions are computed in titlebarButtonRightEdge(for:) but it
still assumes three adjacent fixed-size buttons and doesn't account for the
inserted TitlebarSearchField; update titlebarButtonRightEdge(for:) (and any
helper that computes the .newTab/.newWorkspace offsets) to include the
TitlebarSearchField’s width (TitlebarSearchField / its frame width 220 or
config.buttonSize where appropriate) when calculating the right-edge X offsets
so the shortcut pill offsets (e.g., for .newTab/.newWorkspace) shift right by
the search field width; ensure the same correction is applied to the other
affected call site(s) noted around the later block (also around the 681-684
region).
UX audit: Cmd+F-style global findbarThis PR already covers the core user goal: local-only search across windows/workspaces/tabs with a shared index, scope icons, result focus, and Supersynergy attribution on the PR. Audit against existing cmux find behavior:
Proposed UX direction from the compact Find bar mockup:
Target result: one recognizable Find UX with two extra icons for scope, fewer commands, less tab/project confusion, and direct keyboard traversal through every matching word across open cmux state. |
ef996db to
7ce45de
Compare
Supersynergy
changed the title
|
Re-cut update:
Remaining non-code blocker: Vercel deployment checks still require Manaflow team authorization. |
teamleaderleo
added
area: command-palette
|
Reopening: current main has the global-search foundation, but the scope controls and Cmd-G / Option-Cmd-G traversal described here are still missing. The earlier superseded classification was premature; this remains a live product call. |
|
I have read the CLA Document v2.2 and I hereby sign the CLA You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
teamleaderleo
added
the
needs a call
This branch has not been deployed
2 participants
What
Re-cuts this PR on current main after the menubar global search base landed upstream. Keeps the existing local FTS5 architecture and adds a Cmd+F-style global palette: scope icons, result counter, previous/next/close controls, and keyboard traversal across all open windows/tabs/panels.
Shortcut contract
Review fixes
Verification
Blocked locally: full xcodebuild/reload.sh cannot run because this machine only has CommandLineTools selected and no Xcode.app installed. reload.sh reaches xcodebuild and fails with: active developer directory /Library/Developer/CommandLineTools is a command line tools instance.
Expected external noise: Vercel checks require Manaflow team authorization and are not code failures.