Skip to content

Fix global search lifecycle after reopening - #9562

Closed
austinywang wants to merge 34 commits into
mainfrom
issue-7445-global-search-palette-enter-never-activa
Closed

austinywang wants to merge 34 commits into
mainfrom
issue-7445-global-search-palette-enter-never-activa

Conversation

@austinywang

@austinywang austinywang commented Aug 4, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #7445

Summary

The global-search popover retains its SwiftUI content view, so SwiftUI appearance callbacks were not a reliable per-presentation lifecycle. The popover delegate now drives a dedicated presentation owner that clears retained query/results, reinstalls Return and navigation routing, cancels stale work, restores focus, and refreshes the live index on every open.

Live search indexing is incremental and cancellation-safe: title updates are deduplicated and resolved in one workspace pass, panel captures follow replacements within one bounded presentation deadline, closed panels are reconciled, cancelled SQLite reads terminate through the progress handler, and discarded browser refreshes preserve the last authoritative indexed content.

Testing

  • Red → green lifecycle regression on a remote macOS fleet runner:
    • Test-only commit 3c2e7b7fa2: reopeningSearchClearsRetainedQuery() and reopeningSearchRefreshesLiveIndex() failed against the old behavior.
    • Fix commit b7c9340290: the same tests passed, together with Return activation after reopen.
  • Additional red → green capture regression:
    • Test-only commit 894e992b0c: discardedBrowserRefreshPreservesPreviouslyIndexedContent() failed against the old behavior.
    • Fix commit b74f340d38: the regression passed.
  • Final HEAD eca3285f50 on the leased Xcode 26.3 fleet host:
    • cmux-unit build-for-testing: succeeded with isolated DerivedData.
    • 12 selected GlobalSearchShortcutBehaviorTests passed, covering the reopen lifecycle, Return activation, batched local/remote title resolution, cancelled index operations and active SQLite progress cancellation, deterministic Markdown/browser replacement ordering, and presentation-owned/late/reused capture invalidation.
    • ./scripts/lint-pbxproj-test-wiring.sh: passed.
  • Canonical branch autoreview on final HEAD: clean with zero findings; merge-conflict and cmux policy gates also clean.
  • Tagged dev build used the primary cloud path with no local fallback: ./scripts/reload-cloud.sh --tag sym7445 --launch (run 30948727541, blacksmith-6vcpu-macos-26, BUILD_OK).
  • Isolated socket dogfood on /tmp/cmux-debug-sym7445.sock:
    • Clean first presentation: queried inactive workspace zz7445eca8041404; Return changed the active workspace from 38C34E38-3263-4D41-B9B9-2810349F4BC2 to F6C9853B-19D3-4EF2-85FB-3DE1E3BD59F1.
    • Created zz7445eca8041405 only after that first palette session, reopened Search without clearing the field, typed only the new title, and Return changed the active workspace from F6C9853B-19D3-4EF2-85FB-3DE1E3BD59F1 to 5E4A0760-7E89-43E6-B062-87368C3DC9C6.
    • This verifies first-open Return activation, query reset, and live re-indexing on reopen.
    • Quit with pkill -f "DerivedData/cmux-sym7445"; confirmed no tagged process remained and removed the stale tagged socket.
  • Localization audit: no user-facing copy, localization catalogs, shortcuts, schema, or help text changed.

@coderabbitai

coderabbitai Bot commented Aug 4, 2026 •

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds a presentation controller for Global Search. It manages popover lifecycle, keyboard events, query execution, selection, dismissal, and cancellation. It also synchronizes panel titles and content with the live search index. Tests cover lifecycle, cancellation, refresh, capture races, deadlines, and title behavior.

Changes

Global Search

Layer / File(s) Summary
Presentation and key routing
Sources/Search/GlobalSearchPopoverPresentation.swift, Sources/Search/GlobalSearchPopoverSelectionUpdate.swift
Adds observable state, debounced searches, stale-task protection, keyboard monitoring, shortcut routing, navigation, submission, dismissal, and result activation.
Popover lifecycle integration
Sources/Search/MenubarSearchPopover.swift, cmux.xcodeproj/project.pbxproj
Connects the popover and palette to the presentation object. Adds the new search sources to the target.
Live index title synchronization
Sources/Search/GlobalSearchCoordinator.swift, Sources/Search/AppDelegate+GlobalSearch.swift, Sources/Search/GlobalSearchTitleSnapshot.swift
Tracks panel title snapshots, updates changed titles, removes closed-panel snapshots, and resolves workspace-provided panel titles.
Cancellation-safe content indexing
Sources/Search/GlobalSearchPanelCaptureManager.swift, Sources/Search/GlobalSearchPanelCaptureCompletion.swift, Sources/Search/GlobalSearchPanelCaptureDeadline.swift, Sources/Search/SearchIndex.swift
Adds deadline coordination, generation and revision checks, indexed-panel tracking, live-panel reconciliation, and cancellation checks for capture and deletion operations.
Reopening behavior validation
cmuxTests/GlobalSearchShortcutBehaviorTests.swift
Tests lifecycle reset, result activation, cancellation, live refresh, selection behavior, title resolution, capture races, deadlines, preserved indexed content, and asynchronous coordination helpers.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant MenubarSearchPopover
  participant GlobalSearchPopoverPresentation
  participant GlobalSearchCoordinator
  participant GlobalSearchPanelCaptureManager
  participant SearchIndex
  MenubarSearchPopover->>GlobalSearchPopoverPresentation: begin presentation
  GlobalSearchPopoverPresentation->>GlobalSearchCoordinator: refresh browse results
  GlobalSearchCoordinator->>GlobalSearchPanelCaptureManager: refresh panel content
  GlobalSearchPanelCaptureManager->>SearchIndex: upsert captured content
  GlobalSearchPopoverPresentation->>SearchIndex: execute debounced query
  SearchIndex-->>GlobalSearchPopoverPresentation: return search hits
  GlobalSearchPopoverPresentation->>MenubarSearchPopover: publish results and selection
  MenubarSearchPopover->>GlobalSearchPopoverPresentation: submit selected result
  GlobalSearchPopoverPresentation->>GlobalSearchCoordinator: activate selected result
  MenubarSearchPopover->>GlobalSearchPopoverPresentation: end presentation
Loading

Possibly related issues

Possibly related PRs

  • manaflow-ai/cmux#8699: Modifies Global Search shortcut handling and popover lifecycle code in the same integration area.

Suggested reviewers: lawrencecchen


Important

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

❌ Failed checks (3 errors, 1 warning)

Check name Status Explanation Resolution
Cmux Swift Blocking Runtime ❌ Error Production adds GlobalSearchPanelCaptureDeadline with a scheduled DispatchSourceTimer to coordinate refresh expiration; new timer-based synchronization is not an allowed UI delay. Replace the raw production deadline timer with an approved cancellation-aware completion/scheduler abstraction, or document a specific allowed exception for this synchronization.
Cmux Algorithmic Complexity ❌ Error AppDelegate+GlobalSearch.swift:52 calls Workspace.panelTitle for every panel; that scans remoteTmuxWindowMirrors and each mirror's controlPanes, creating an unbounded nested scan in search refresh. Build one per-workspace panel-ID-to-title map, or use direct indexed title data, before iterating panels; add a measurement for large remote workspaces.
Cmux Swift Package Boundaries ❌ Error The PR adds an independently testable capture state machine in app Sources/Search: Foundation-only Deadline/Completion helpers plus generation/revision logic in a 593-line manager, with no search S... Create Packages/macOS/CmuxGlobalSearchCore and expose GlobalSearchPanelCaptureCoordinator for generation, revision, deadline, and completion state; keep AppKit/panel adapters and popover composition in the app target.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (21 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Cmux Swift Actor Isolation ✅ Passed New mutable search coordinators are explicitly @MainActor, SearchIndex remains an actor, and new value types are nonisolated; async tasks use explicit @MainActor boundaries.
Cmux Browser Automation Off-Main ✅ Passed The PR changes only global-search implementation, project wiring, and tests; it does not modify browser socket automation routing or worker-lane policy.
Cmux Expensive Synchronous Load ✅ Passed Production diff adds no agent-history loader or agent-owned file parsing; SearchIndex.open uses Task.detached, and refresh uses async actor APIs. New synchronous work only builds in-memory panel/ti...
Cmux Cache Substitution Correctness ✅ Passed Live contexts are rebuilt before title upserts; title snapshots only suppress unchanged writes, while Markdown cache starts cold, resets/reconciles, and is invalidated by edit and file-watcher call...
Cmux No Hacky Sleeps ✅ Passed The PR diff contains only Swift files and an Xcode project file; it has no covered TypeScript, JavaScript, shell, or runtime-script changes. Swift timing is out of scope.
Cmux Swift Concurrency ✅ Passed No new background queues or Combine. Production Tasks are stored and cancelled, while main-actor timer/AppKit callback bridges and test continuations are allowed boundaries.
Cmux Swift @Concurrent ✅ Passed Changed search async work is @MainActor UI coordination; SearchIndex actor methods and SearchIndex.open's Task.detached provide non-UI boundaries, and the diff adds no nonisolated async or invalid...
Cmux Swiftpm Lockfiles ✅ Passed The PR changes only Xcode source-file entries; it adds no SwiftPM package references, Package.swift dependencies, or .gitignore rules, so no Package.resolved diff is required.
Cmux Swift Logging ✅ Passed Changed production Swift adds no print, debugPrint, dump, NSLog, or ad hoc output; the only new diagnostics are #if DEBUG cmuxDebugLog calls, which the rule allows.
Cmux User-Facing Error Privacy ✅ Passed The production diff adds no user-facing error or alert text; new failure messages are DEBUG-only cmuxDebugLog diagnostics, while palette copy remains generic search UI.
Cmux Full Internationalization ✅ Passed Production diff adds no new user-facing copy or catalog changes; existing UI strings use String(localized:defaultValue:), and existing locale gaps are unchanged.
Cmux Swiftui State Layout ✅ Passed The PR uses @Observable with @Bindable, has no legacy ObservableObject state or GeometryReader, and passes immutable row snapshots plus closures below LazyVStack.
Cmux Architecture Rethink ✅ Passed The presentation object owns UI state and generations; NSPopoverDelegate is a documented bridge. Production refresh waits on completion callbacks and a bounded deadline, while polling/sleeps are te...
Cmux Swift Auxiliary Window Close Shortcuts ✅ Passed The PR changes an existing NSPopover, not a standalone window; no new NSWindow/NSPanel/NSWindowController/WindowGroup appears, and the auxiliary-window lint passes.
Cmux Source Artifacts ✅ Passed All 12 changed paths are Swift source, a Swift test, or the Xcode project file; no logs, media, temp/cache/build directories, downloads, or scratch artifact paths were added.
Cmux No Test Or Debug Seam In Production Source ✅ Passed Production diff adds no test/debug seam names or test-only accessors; new DEBUG blocks only log errors, and existing SearchIndex.clearForTesting is unchanged.
Cmux No Ambient Global State ✅ Passed The production diff adds constructable owning types with injected dependencies; it adds no free file-scope functions, top-level mutable vars, static helper namespaces, or new singletons.
Title check ✅ Passed The title clearly describes the primary change: fixing global search lifecycle behavior when the popover reopens.
Description check ✅ Passed The description clearly explains the lifecycle and indexing changes and provides extensive regression, build, and dogfood testing details.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch issue-7445-global-search-palette-enter-never-activa

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@austinywang
austinywang force-pushed the issue-7445-global-search-palette-enter-never-activa branch from 162862d to 19c0cb6 Compare August 4, 2026 09:25
@cursor

cursor Bot commented Aug 4, 2026

Copy link
Copy Markdown

Bugbot is paused — on-demand spend limit reached

Bugbot uses usage-based billing for this team and has hit its on-demand spend limit.

A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue.

@austinywang
austinywang force-pushed the issue-7445-global-search-palette-enter-never-activa branch from 19c0cb6 to b7c9340 Compare August 4, 2026 10:03
@cursor

cursor Bot commented Aug 4, 2026

Copy link
Copy Markdown

Bugbot is paused — on-demand spend limit reached

Bugbot uses usage-based billing for this team and has hit its on-demand spend limit.

A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 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 `@cmuxTests/GlobalSearchShortcutBehaviorTests.swift`:
- Line 422: Remove the fixed-duration RunLoop delay on line 422 that calls
`RunLoop.main.run(until: Date.now.addingTimeInterval(0.05))`. Move the reopen
operation to execute immediately after `waitUntilGlobalSearchCloses()`
completes. If coordinator visibility is insufficient to verify the popover
close, add an explicit completion predicate wait instead of relying on a
fixed-duration delay to detect the close event.

In `@Sources/Search/MenubarSearchPopover.swift`:
- Around line 151-154: Update cancelSearchWork() to increment searchGeneration
before clearing searchDebounceTimer, invalidating already-queued debounce
handlers; have scheduleSearch() capture the incremented generation as its
current generation. Add a deterministic regression test covering cleanup racing
with queued scheduling, avoiding fixed delays and verifying stale work cannot
update results after cleanup.
🪄 Autofix

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 Plus

Run ID: 2da57111-0641-4c9b-a8ff-3b10721450b3

📥 Commits

Reviewing files that changed from the base of the PR and between 99f7e1b and e7fe3aa.

📒 Files selected for processing (4)
  • Sources/Search/GlobalSearchPopoverPresentation.swift
  • Sources/Search/MenubarSearchPopover.swift
  • cmux.xcodeproj/project.pbxproj
  • cmuxTests/GlobalSearchShortcutBehaviorTests.swift

Comment thread cmuxTests/GlobalSearchShortcutBehaviorTests.swift Outdated
Comment thread Sources/Search/MenubarSearchPopover.swift Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
Sources/Search/MenubarSearchPopover.swift (1)

38-54: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Gate popoverDidClose on the live presentation generation before tearing down.

dismiss() only schedules performanceClose; popoverDidClose later calls endPresentation() and increments presentationGeneration unconditionally. refreshTask/scheduleSearch use the generation that beginPresentation() set, while removeKeyMonitor() clears input handling from the displayed popover. Keep a copy of the presentation generation in popoverDidClose and only perform teardown when it matches the current presentation.

🤖 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/MenubarSearchPopover.swift` around lines 38 - 54, Update
popoverDidClose to capture the presentation generation associated with the
closing popover and compare it with the current generation before calling
presentation.endPresentation(), clearing dismissalHandler, or invoking the
handler. Skip teardown for stale close notifications so the active presentation
and its refreshTask/scheduleSearch generation remain intact.
🤖 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 `@cmuxTests/GlobalSearchShortcutBehaviorTests.swift`:
- Around line 477-480: Update the test around GlobalSearchPopoverPresentation so
it waits for the scheduled search task to finish, not merely
debounce.waitUntilFinished(). Add an appropriate completion signal,
deadline-bounded predicate, or owned-task wait before asserting
presentation.results == browseResults, while preserving the existing
presentation closure and cancellation behavior.

In `@Sources/Search/GlobalSearchCoordinator.swift`:
- Around line 164-173: The title indexing flow in GlobalSearchCoordinator must
prevent a stale deletePanel mutation from completing after a newer upsert.
Serialize delete and upsert operations per panel, or track generations so
indexedTitleSnapshots[context.panelID] is committed only when that upsert is
still current; update the logic around cancelPanelPurge and index.upsert while
preserving cancellation behavior.
- Around line 168-173: Update the construction of GlobalSearchPanelContext so
its panelTitle is resolved through workspace.panelTitle(panelId:) rather than
panel.displayTitle, ensuring GlobalSearchTitleSnapshot compares workspace-owned
remote and custom titles and refreshes stale global-search documents.

In `@Sources/Search/GlobalSearchPopoverPresentation.swift`:
- Around line 130-133: Update the query-results handling near
GlobalSearchResultRow creation to reset selectedIndex to 0 whenever new results
replace results, matching reloadBrowseResults(). Remove the existing clamping
behavior so stale selection positions cannot carry over between queries.

---

Outside diff comments:
In `@Sources/Search/MenubarSearchPopover.swift`:
- Around line 38-54: Update popoverDidClose to capture the presentation
generation associated with the closing popover and compare it with the current
generation before calling presentation.endPresentation(), clearing
dismissalHandler, or invoking the handler. Skip teardown for stale close
notifications so the active presentation and its refreshTask/scheduleSearch
generation remain intact.
🪄 Autofix

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 Plus

Run ID: b9133561-6901-4c02-9e8a-a1ffb6c38e03

📥 Commits

Reviewing files that changed from the base of the PR and between e7fe3aa and a3d74fb.

📒 Files selected for processing (7)
  • Sources/Search/AppDelegate+GlobalSearch.swift
  • Sources/Search/GlobalSearchCoordinator.swift
  • Sources/Search/GlobalSearchPopoverPresentation.swift
  • Sources/Search/GlobalSearchTitleSnapshot.swift
  • Sources/Search/MenubarSearchPopover.swift
  • cmux.xcodeproj/project.pbxproj
  • cmuxTests/GlobalSearchShortcutBehaviorTests.swift

Comment thread cmuxTests/GlobalSearchShortcutBehaviorTests.swift
Comment thread Sources/Search/GlobalSearchCoordinator.swift
Comment thread Sources/Search/GlobalSearchCoordinator.swift
Comment thread Sources/Search/GlobalSearchPopoverPresentation.swift Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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 `@cmuxTests/GlobalSearchShortcutBehaviorTests.swift`:
- Around line 750-763: Update the private waitUntil polling helper to use the
project’s injected test clock or monotonic deadline helper instead of Date.now
for both deadline creation and expiration checks. Preserve the existing
predicate polling and timeout behavior while avoiding wall-clock APIs in this
test.
🪄 Autofix

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 Plus

Run ID: 74afc497-54fe-455e-bfad-2c5e1594e232

📥 Commits

Reviewing files that changed from the base of the PR and between a3d74fb and f319d72.

📒 Files selected for processing (7)
  • Sources/Search/AppDelegate+GlobalSearch.swift
  • Sources/Search/GlobalSearchCoordinator.swift
  • Sources/Search/GlobalSearchPanelCaptureManager.swift
  • Sources/Search/GlobalSearchPopoverPresentation.swift
  • Sources/Search/MenubarSearchPopover.swift
  • Sources/Search/SearchIndex.swift
  • cmuxTests/GlobalSearchShortcutBehaviorTests.swift

Comment thread cmuxTests/GlobalSearchShortcutBehaviorTests.swift

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 (3)
cmuxTests/GlobalSearchShortcutBehaviorTests.swift (3)

937-948: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Complete popover dismissal during cleanup.

Line 938 starts dismissal, then the helper immediately resets shortcut routing and closes the main window. At Line 391, Line 424, and Line 669, the tests do not treat the same dismissal as synchronous. If a test exits with the palette open, pending delegate work can affect the next test through GlobalSearchCoordinator.shared. Wait for waitUntilGlobalSearchCloses() before resetting shared routing state and closing the window.

As per coding guidelines, “Isolate shared static, global, UserDefaults, file, and related state per test, resetting it in setUp and tearDown as appropriate.”

🤖 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 `@cmuxTests/GlobalSearchShortcutBehaviorTests.swift` around lines 937 - 948,
The closeWindow method begins dismissing the palette with
GlobalSearchCoordinator.shared.dismissPalette() but proceeds immediately to
reset state and close the window without waiting for that dismissal to complete.
This causes the palette to remain open across test boundaries, polluting the
shared GlobalSearchCoordinator state. Add a call to
waitUntilGlobalSearchCloses() immediately after the dismissPalette call to
ensure the popover fully closes before the DEBUG block executes the
debugResetShortcutRoutingStateForTesting and subsequent window closing logic.

Source: Coding guidelines


795-808: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Use a monotonic deadline in waitUntil.

Line 800 derives the timeout from Date.now. A system-clock change can make the predicate poll expire early or run longer. Use the project test clock or a monotonic deadline helper.

As per coding guidelines, “Test code must avoid real wall-clock dependencies: use injected virtual clocks and advance them manually for time-driven behavior.”

🤖 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 `@cmuxTests/GlobalSearchShortcutBehaviorTests.swift` around lines 795 - 808,
Update the waitUntil function to use a monotonic deadline instead of wall-clock
time. Replace the deadline initialization that currently uses
Date.now.addingTimeInterval(timeout) with a monotonic deadline approach such as
the project's test clock or a monotonic deadline helper. Update the while
condition that checks the deadline to use the same monotonic time source,
ensuring the timeout logic is immune to system clock changes.

Source: Coding guidelines


440-443: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Assert activation of the selected result.

Line 440 only proves that Return closes the palette. A handler that closes the palette without activating the selected browse result also passes. Capture the expected browse target before reopening, then assert its focused panel, selected workspace, or activation callback after the key event.

As per coding guidelines, “When a user reports that tests missed a bug, add behavior-level coverage for the exact reproduction path before claiming the fix is complete.”

🤖 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 `@cmuxTests/GlobalSearchShortcutBehaviorTests.swift` around lines 440 - 443,
The current test only validates that pressing Return closes the global search
palette via waitUntilGlobalSearchCloses, but does not verify that the selected
result was actually activated. Capture the expected browse target before the
popover reopens and the key event is triggered. After the
waitUntilGlobalSearchCloses call succeeds, add assertions to verify the target
was activated by checking its focused panel property, selected workspace state,
or activation callback to ensure the selected result was properly activated and
not just closed without selection.

Source: Coding guidelines

🤖 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 `@cmuxTests/GlobalSearchShortcutBehaviorTests.swift`:
- Around line 731-738: Update the initial-refresh test to exercise
GlobalSearchCoordinator.refreshLiveIndex() and await its completion before
asserting searchability, or otherwise explicitly await the capture completion
produced by captureManager.refreshPanelContent(for:). Ensure the assertion that
the newly discovered Markdown panel is searchable runs only after
coordinator-completed Markdown capture work.

---

Outside diff comments:
In `@cmuxTests/GlobalSearchShortcutBehaviorTests.swift`:
- Around line 937-948: The closeWindow method begins dismissing the palette with
GlobalSearchCoordinator.shared.dismissPalette() but proceeds immediately to
reset state and close the window without waiting for that dismissal to complete.
This causes the palette to remain open across test boundaries, polluting the
shared GlobalSearchCoordinator state. Add a call to
waitUntilGlobalSearchCloses() immediately after the dismissPalette call to
ensure the popover fully closes before the DEBUG block executes the
debugResetShortcutRoutingStateForTesting and subsequent window closing logic.
- Around line 795-808: Update the waitUntil function to use a monotonic deadline
instead of wall-clock time. Replace the deadline initialization that currently
uses Date.now.addingTimeInterval(timeout) with a monotonic deadline approach
such as the project's test clock or a monotonic deadline helper. Update the
while condition that checks the deadline to use the same monotonic time source,
ensuring the timeout logic is immune to system clock changes.
- Around line 440-443: The current test only validates that pressing Return
closes the global search palette via waitUntilGlobalSearchCloses, but does not
verify that the selected result was actually activated. Capture the expected
browse target before the popover reopens and the key event is triggered. After
the waitUntilGlobalSearchCloses call succeeds, add assertions to verify the
target was activated by checking its focused panel property, selected workspace
state, or activation callback to ensure the selected result was properly
activated and not just closed without selection.
🪄 Autofix

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 Plus

Run ID: ca7c1acb-9926-43bd-a7f5-dbab3701d587

📥 Commits

Reviewing files that changed from the base of the PR and between f319d72 and 2751c1d.

📒 Files selected for processing (1)
  • cmuxTests/GlobalSearchShortcutBehaviorTests.swift

Comment thread cmuxTests/GlobalSearchShortcutBehaviorTests.swift

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/Search/GlobalSearchCoordinator.swift (1)

118-151: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

A purge already awaiting deletePanel can still erase a newer title upsert.

cancelPanelPurge (Line 153) only cancels the task and clears panelPurgeTaskIDs. A purge task that has already passed the guard at Line 141 and is suspended inside try await index.deletePanel(panelID) continues to completion. refreshLivePanelTitles calls cancelPanelPurge at Line 175 and then upserts the title at Line 181, so this ordering is possible:

  1. purgePanel(id:) schedules the delete and reaches await index.deletePanel(panelID).
  2. The panel becomes live again; cancelPanelPurge clears the bookkeeping.
  3. refreshLivePanelTitles upserts the title document.
  4. The suspended deletePanel completes and removes every document for that panel, including the new title document.
  5. Line 186 commits indexedTitleSnapshots[context.panelID] = snapshot, so later refreshes skip the upsert and the panel stays missing from search results until its title changes.

titleIndexGeneration does not cover this case, because a purge does not change the generation. Serialize per-panel index mutations, or record a purge revision that the title upsert must re-check before committing the snapshot.

🤖 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/GlobalSearchCoordinator.swift` around lines 118 - 151, Prevent
an in-flight purge from deleting a title re-upserted by refreshLivePanelTitles.
Update purgePanel and the related cancelPanelPurge/refreshLivePanelTitles flow
to serialize per-panel index mutations or track and revalidate a purge revision
before committing indexedTitleSnapshots, ensuring a completed delete cannot
overwrite a newer upsert or leave its snapshot marked as indexed.
🤖 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 `@Sources/Search/GlobalSearchCoordinator.swift`:
- Around line 96-99: The loop calling await
captureManager.refreshPanelContent(for: context) blocks the palette open because
refreshPanelContent awaits Markdown panel initialization. Wrap the
refreshPanelContent calls in a non-awaited background Task so the palette can
open without waiting for panel content to load, allowing the index build to
proceed independently after presentation.

---

Outside diff comments:
In `@Sources/Search/GlobalSearchCoordinator.swift`:
- Around line 118-151: Prevent an in-flight purge from deleting a title
re-upserted by refreshLivePanelTitles. Update purgePanel and the related
cancelPanelPurge/refreshLivePanelTitles flow to serialize per-panel index
mutations or track and revalidate a purge revision before committing
indexedTitleSnapshots, ensuring a completed delete cannot overwrite a newer
upsert or leave its snapshot marked as indexed.
🪄 Autofix

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 Plus

Run ID: 65d947c2-a326-457c-8263-f7f3c9ecc986

📥 Commits

Reviewing files that changed from the base of the PR and between 2751c1d and cc882c3.

📒 Files selected for processing (2)
  • Sources/Search/GlobalSearchCoordinator.swift
  • Sources/Search/GlobalSearchPanelCaptureManager.swift

Comment thread Sources/Search/GlobalSearchCoordinator.swift Outdated
@austinywang

Copy link
Copy Markdown
Contributor Author

Follow-up on the CodeRabbit outside-diff findings:

  • Stale popover close: no code change. MenubarSearchPopover keeps presentation.isPresented true until this popover instance delivers popoverDidClose, and show() is guarded by that state. A replacement presentation therefore cannot begin before the closing presentation has completed teardown, so there is no newer generation for the old close callback to invalidate.
  • Test cleanup and Return activation: addressed in e6ef912. Cleanup now waits for the shared popover to finish closing, and the Return regression test keyboard-selects an inactive workspace result and asserts that its workspace and panel actually become active.
  • Monotonic polling: addressed in c8be85a with ContinuousClock.
  • In-flight panel purge: no code change. SearchIndex is an actor and deletePanel is a synchronous actor-isolated method with no suspension point; once it starts, the delete finishes before a later upsert can run. If cancellation wins before actor execution, Task.checkCancellation prevents the delete. The proposed delete-after-upsert ordering cannot occur.

@austinywang
austinywang force-pushed the issue-7445-global-search-palette-enter-never-activa branch from fda7d37 to 4f616a1 Compare August 4, 2026 14:30
@cursor

cursor Bot commented Aug 4, 2026

Copy link
Copy Markdown

Bugbot is paused — on-demand spend limit reached

Bugbot uses usage-based billing for this team and has hit its on-demand spend limit.

A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue.

@austinywang
austinywang force-pushed the issue-7445-global-search-palette-enter-never-activa branch from bb8801a to 3b6e714 Compare August 4, 2026 15:07
@cursor

cursor Bot commented Aug 4, 2026

Copy link
Copy Markdown

Bugbot is paused — on-demand spend limit reached

Bugbot uses usage-based billing for this team and has hit its on-demand spend limit.

A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue.

@cursor

cursor Bot commented Aug 4, 2026

Copy link
Copy Markdown

Bugbot is paused — on-demand spend limit reached

Bugbot uses usage-based billing for this team and has hit its on-demand spend limit.

A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue.

@austinywang

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 4, 2026 •

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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 `@cmuxTests/GlobalSearchShortcutBehaviorTests.swift`:
- Around line 1122-1128: Remove the negative fixed-duration wait that polls
refreshFinishedCount after thirdIndexRequestStarted.wait() in the relevant test.
Rely on thirdIndexRequestStarted as the deterministic proof that the latest
capture was followed, and retain the existing positive completion assertion that
follows.
🪄 Autofix

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 Plus

Run ID: 78fe26df-7dd9-44fa-a295-67d442331e41

📥 Commits

Reviewing files that changed from the base of the PR and between 2751c1d and ad6ca22.

📒 Files selected for processing (8)
  • Sources/Search/GlobalSearchCoordinator.swift
  • Sources/Search/GlobalSearchPanelCaptureCompletion.swift
  • Sources/Search/GlobalSearchPanelCaptureDeadline.swift
  • Sources/Search/GlobalSearchPanelCaptureManager.swift
  • Sources/Search/GlobalSearchPopoverPresentation.swift
  • Sources/Search/GlobalSearchPopoverSelectionUpdate.swift
  • cmux.xcodeproj/project.pbxproj
  • cmuxTests/GlobalSearchShortcutBehaviorTests.swift

Comment thread cmuxTests/GlobalSearchShortcutBehaviorTests.swift
@austinywang

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 4, 2026 •

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@austinywang

Copy link
Copy Markdown
Contributor Author

Follow-up on the automated pre-merge prompts:

  • Actor isolation: GlobalSearchTitleSnapshot is created only from GlobalSearchCoordinator, which is @MainActor; the final Swift 6/Xcode 26.3 cmux-unit build-for-testing passes this boundary.
  • Timing primitives: query debounce is an injected scheduler that returns explicit cancellation, and the capture deadline is a one-shot, owner-cancelled deadline. Neither polls. Deterministic tests inject/control the debounce and drive capture-completion signals directly.
  • Package boundary: capture adapters depend on app-owned BrowserPanel, MarkdownPanel, panel context, and SearchIndex types. Moving this narrow integration into a package would invert or expand those dependencies; the lifecycle helpers are instead separated into focused app-owned files.
  • Lifecycle ownership: MenubarSearchPopover is now the thin NSPopoverDelegate adapter. GlobalSearchPopoverPresentation owns query, selection, monitor, task, cancellation, and begin/end state. The coordinator’s weak reference only routes index-change notifications to the currently presented owner and cannot retain or replace its lifecycle.
  • Documentation: the new types are internal app integration, not public package APIs; the non-obvious shared-deadline and refresh invariants are documented at their ownership points.

The final branch-wide canonical review and cmux policy check both report no actionable findings.

@coderabbitai

coderabbitai Bot commented Aug 4, 2026 •

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
cmuxTests/GlobalSearchShortcutBehaviorTests.swift (1)

1029-1041: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Make the replacement capture observable.

Both the initial refresh and captureBrowserPanel(browserPanel) are validated with the same token. An implementation that commits only the first capture can still pass the final search assertion. Change the browser content between captures, or use generation-specific markers, and assert that the replacement marker is indexed.

🤖 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 `@cmuxTests/GlobalSearchShortcutBehaviorTests.swift` around lines 1029 - 1041,
Update the test around refreshTask and
captureManager.captureBrowserPanel(browserPanel) so the replacement capture
produces distinct observable content from the initial capture, using changed
browser content or generation-specific markers. Assert that index.search(token)
contains the replacement marker for the browser panel, ensuring the superseding
capture—not merely the initial capture—is committed.
🤖 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.

Outside diff comments:
In `@cmuxTests/GlobalSearchShortcutBehaviorTests.swift`:
- Around line 1029-1041: Update the test around refreshTask and
captureManager.captureBrowserPanel(browserPanel) so the replacement capture
produces distinct observable content from the initial capture, using changed
browser content or generation-specific markers. Assert that index.search(token)
contains the replacement marker for the browser panel, ensuring the superseding
capture—not merely the initial capture—is committed.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 8f09e414-6f9d-40f6-9140-e5ecf9ee855d

📥 Commits

Reviewing files that changed from the base of the PR and between ad6ca22 and 29e3de9.

📒 Files selected for processing (1)
  • cmuxTests/GlobalSearchShortcutBehaviorTests.swift

@cursor

cursor Bot commented Aug 4, 2026

Copy link
Copy Markdown

Bugbot is paused — on-demand spend limit reached

Bugbot uses usage-based billing for this team and has hit its on-demand spend limit.

A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue.

@lawrencecchen lawrencecchen added the stale-revisit Closed after 30+ days without activity; preserved for possible revisit or reopening. label Sep 23, 2026
@github-project-automation github-project-automation Bot moved this from Todo to Done in cmux backlog Sep 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stale-revisit Closed after 30+ days without activity; preserved for possible revisit or reopening.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Global search palette: Enter never activates the selected result; stale query and no re-index on reopen (retained popover view)

3 participants