Repository navigation
Add Cmd+P all-surface search option - #1382
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdds surface-aware command-palette support: new fingerprint types and surface entries, a Settings toggle to include surfaces in searches, expanded localization entries, unit and UI tests (socket-driven) validating cross-workspace surface search and switcher behavior. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant UI as App UI
participant Settings as SettingsStore
participant Switcher as CommandPalette Switcher
participant Search as Search Engine
participant Windows as WindowContexts
User->>UI: open command palette / type query
UI->>Settings: read commandPaletteSearchAllSurfaces
UI->>Switcher: request entries (includeSurfaces?)
Switcher->>Windows: enumerate workspaces & surfaces
Windows-->>Switcher: workspace + surfaces metadata
Switcher->>Search: compute fingerprint & search scope
Search-->>Switcher: results (workspace/surface entries)
Switcher-->>UI: render results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3135b4b13e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 5 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="cmuxUITests/SidebarHelpMenuUITests.swift">
<violation number="1" location="cmuxUITests/SidebarHelpMenuUITests.swift:527">
P2: Use the `matched` result when returning the snapshot; otherwise timeouts can return stale/unmatched data and make this UI test pass for the wrong state.</violation>
<violation number="2" location="cmuxUITests/SidebarHelpMenuUITests.swift:551">
P3: This adds another large duplicate `ControlSocketClient`; extract/reuse a shared test helper to avoid divergence in socket behavior across UI tests.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
Sources/ContentView.swift (1)
4101-4110: Consider locale-aware aliases in surface search keywords.
commandPaletteSurfaceKeywords(for:)is English-only; type-based queries in Japanese may miss expected matches. Consider adding localized aliases (or injecting the localized kind label into searchable keywords) to improve non-English discoverability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ContentView.swift` around lines 4101 - 4110, The surface keyword list is English-only so non-English users miss matches; update commandPaletteSurfaceKeywords(for:) to include locale-aware aliases by adding localized labels for each PanelType (use NSLocalizedString or a localization helper) and/or append a localizedKindLabel(from: PanelType) to the returned array; ensure PanelType (enum) provides a stable localization key (e.g., "panel.terminal", "panel.browser", "panel.markdown") and use Locale.current when generating the localized string so searches work in other languages.cmuxUITests/SidebarHelpMenuUITests.swift (1)
395-398: Prefer a predicate wait over the fixed 400 ms sleep.
RunLoop.current.run(until:)makes this flow timing-sensitive under CI load. You already havesidebarHelpPollUntil(...), so it would be safer to wait for the workspace/window handoff you need before opening the palette.Based on learnings,
cmuxUITestsshould run on the UTM macOS VM, so fixed sleeps here are more likely to flake than state-based waits.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxUITests/SidebarHelpMenuUITests.swift` around lines 395 - 398, Replace the fixed RunLoop sleep with a predicate-based wait using the existing sidebarHelpPollUntil helper: instead of calling RunLoop.current.run(until: Date().addingTimeInterval(0.4)) after socketCommand("select_workspace 0") and socketCommand("focus_window \(mainWindowId)"), call sidebarHelpPollUntil(...) to wait for the workspace/window handoff or the expected UI state before opening the palette; this ensures the test observes the actual state transition (use the same condition/sidebarHelpPollUntil predicate that indicates the palette or focus is ready).
🤖 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/SidebarHelpMenuUITests.swift`:
- Around line 569-586: The function waitForCommandPaletteSnapshot currently
returns `latest` even when `matched` is false, allowing callers (e.g., tests
using XCTUnwrap) to proceed with a stale snapshot; update the return to return
`nil` on timeout by changing the final line to return `matched ? latest : nil`
(leave the polling logic using `sidebarHelpPollUntil` and
`commandPaletteSnapshot` intact so callers get a valid snapshot only when the
predicate matched).
- Around line 486-506: The test currently calls throw XCTSkip(...) in
requireSearchAllSurfacesToggle which hides regressions; change it to fail the
test instead by replacing the throw XCTSkip call with an assertion failure (e.g.
XCTFail("Could not find the command palette all-surfaces toggle")) and then
throw a simple test Error to stop execution (create a small private Error type
like enum TestFailure: Error { case elementMissing } and throw
TestFailure.elementMissing) so requireSearchAllSurfacesToggle still exits with
an error but marks the test as failed rather than skipped.
- Around line 303-313: The tearDown() currently only removes socketPath and
leaves the AppStorage-backed searchAllSurfacesToggle enabled across tests;
update tearDown() to locate the UI toggle via
requireSearchAllSurfacesToggle(app: app), check its state with
toggleIsOn(toggle), and if it's on call toggle.click() to turn it off before
cleaning socketPath and calling super. Also apply the same reset in the other
tearDown-equivalent block referenced (lines ~409-419) so tests are not
order-dependent.
---
Nitpick comments:
In `@cmuxUITests/SidebarHelpMenuUITests.swift`:
- Around line 395-398: Replace the fixed RunLoop sleep with a predicate-based
wait using the existing sidebarHelpPollUntil helper: instead of calling
RunLoop.current.run(until: Date().addingTimeInterval(0.4)) after
socketCommand("select_workspace 0") and socketCommand("focus_window
\(mainWindowId)"), call sidebarHelpPollUntil(...) to wait for the
workspace/window handoff or the expected UI state before opening the palette;
this ensures the test observes the actual state transition (use the same
condition/sidebarHelpPollUntil predicate that indicates the palette or focus is
ready).
In `@Sources/ContentView.swift`:
- Around line 4101-4110: The surface keyword list is English-only so non-English
users miss matches; update commandPaletteSurfaceKeywords(for:) to include
locale-aware aliases by adding localized labels for each PanelType (use
NSLocalizedString or a localization helper) and/or append a
localizedKindLabel(from: PanelType) to the returned array; ensure PanelType
(enum) provides a stable localization key (e.g., "panel.terminal",
"panel.browser", "panel.markdown") and use Locale.current when generating the
localized string so searches work in other languages.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f76929fc-cd30-475e-8659-fd6ea496d772
📒 Files selected for processing (5)
Resources/Localizable.xcstringsSources/ContentView.swiftSources/cmuxApp.swiftcmuxTests/CommandPaletteSearchEngineTests.swiftcmuxUITests/SidebarHelpMenuUITests.swift
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmuxTests/CommandPaletteSearchEngineTests.swift (1)
619-711: Add surfacedisplayNameandiddelta assertions to complete fingerprint contract coverage.This test already checks
metadataandkindLabel; addingdisplayNameandidmutations would fully cover the stated fingerprint dimensions.♻️ Suggested test extension
@@ func testSwitcherFingerprintTracksSurfaceValuesAtSameCardinality() { @@ let changedSurfaceKind = ContentView.commandPaletteSwitcherFingerprint( @@ ) + let changedSurfaceDisplayName = ContentView.commandPaletteSwitcherFingerprint( + windowContexts: [ + ContentView.CommandPaletteSwitcherFingerprintContext( + windowId: windowID, + windowLabel: nil, + selectedWorkspaceId: workspaceID, + workspaces: [ + ContentView.CommandPaletteSwitcherFingerprintWorkspace( + id: workspaceID, + displayName: "Workspace Alpha", + metadata: CommandPaletteSwitcherSearchMetadata(), + surfaces: [ + ContentView.CommandPaletteSwitcherFingerprintSurface( + id: surfaceID, + displayName: "Terminal 2", + kindLabel: "Terminal", + metadata: CommandPaletteSwitcherSearchMetadata( + directories: ["/tmp/search-alpha"], + branches: ["feature/a"], + ports: [3000] + ) + ) + ] + ) + ] + ) + ] + ) + let changedSurfaceID = ContentView.commandPaletteSwitcherFingerprint( + windowContexts: [ + ContentView.CommandPaletteSwitcherFingerprintContext( + windowId: windowID, + windowLabel: nil, + selectedWorkspaceId: workspaceID, + workspaces: [ + ContentView.CommandPaletteSwitcherFingerprintWorkspace( + id: workspaceID, + displayName: "Workspace Alpha", + metadata: CommandPaletteSwitcherSearchMetadata(), + surfaces: [ + ContentView.CommandPaletteSwitcherFingerprintSurface( + id: UUID(), + displayName: "Terminal", + kindLabel: "Terminal", + metadata: CommandPaletteSwitcherSearchMetadata( + directories: ["/tmp/search-alpha"], + branches: ["feature/a"], + ports: [3000] + ) + ) + ] + ) + ] + ) + ] + ) XCTAssertNotEqual(base, changedSurfaceMetadata) XCTAssertNotEqual(base, changedSurfaceKind) + XCTAssertNotEqual(base, changedSurfaceDisplayName) + XCTAssertNotEqual(base, changedSurfaceID) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/CommandPaletteSearchEngineTests.swift` around lines 619 - 711, Extend testSwitcherFingerprintTracksSurfaceValuesAtSameCardinality to also assert that changing a surface's displayName and changing a surface's id produce different fingerprints: create two additional variants using ContentView.commandPaletteSwitcherFingerprint with the same structure but one where ContentView.CommandPaletteSwitcherFingerprintSurface has a different displayName and another where it has a different id, then add XCTAssertNotEqual(base, changedDisplayName) and XCTAssertNotEqual(base, changedId) (referencing ContentView.CommandPaletteSwitcherFingerprintSurface.displayName and .id to locate the fields).
🤖 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/ContentView.swift`:
- Around line 4129-4140: The helper commandPaletteOrderedSwitcherPanels
currently returns orderedPanelIds early based only on count, allowing stale or
duplicate IDs to slip through; update it to normalize orderedPanelIds first by
filtering out IDs that are not present in workspace.panels and removing
duplicates (preserving first occurrence), then build the final list by appending
any missing workspace.panel IDs (sorted by uuidString) not already seen; this
removes stale/duplicate entries that can create missing surfaces or duplicate
command IDs when later building dictionaries.
---
Nitpick comments:
In `@cmuxTests/CommandPaletteSearchEngineTests.swift`:
- Around line 619-711: Extend
testSwitcherFingerprintTracksSurfaceValuesAtSameCardinality to also assert that
changing a surface's displayName and changing a surface's id produce different
fingerprints: create two additional variants using
ContentView.commandPaletteSwitcherFingerprint with the same structure but one
where ContentView.CommandPaletteSwitcherFingerprintSurface has a different
displayName and another where it has a different id, then add
XCTAssertNotEqual(base, changedDisplayName) and XCTAssertNotEqual(base,
changedId) (referencing
ContentView.CommandPaletteSwitcherFingerprintSurface.displayName and .id to
locate the fields).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 216d0a39-9d53-49f1-83a5-a96752dc233c
📒 Files selected for processing (2)
Sources/ContentView.swiftcmuxTests/CommandPaletteSearchEngineTests.swift
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9ee6e0ca6e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
This was referenced Mar 14, 2026
This was referenced Mar 24, 2026
bn-l
pushed a commit
to bn-l/cmux
that referenced
this pull request
Apr 3, 2026
…h-all-surfaces Add Cmd+P all-surface search option
This was referenced May 13, 2026
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Testing
Task
Summary by cubic
Adds a Settings toggle to let Cmd+P search all surfaces (Terminal, Browser, Markdown) across workspaces. Also hardens command palette refresh to avoid stale or wrong results when switching between command and switcher modes.
New Features
@AppStorage("commandPalette.switcherSearchAllSurfaces"); updated placeholder/empty messages (EN/JA).switcher.surface.<id>and focus the target; stable panel order; index includes directories, branches, and ports.Bug Fixes
Written for commit 9ee6e0c. Summary will update on new commits.
Summary by CodeRabbit
New Features
Localization
Tests