Repository navigation
Fix missing command palette open-in targets - #1621
Merged
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![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.
📝 WalkthroughWalkthroughAdds a LaunchServices-backed application-name lookup via Changes
Sequence Diagram(s)sequenceDiagram
participant CV as ContentView
participant TOT as TerminalDirectoryOpenTarget
participant DE as DetectionEnvironment
participant WS as NSWorkspace
CV->>TOT: availableTargets()
TOT->>TOT: resolve candidates for each target
TOT->>DE: fileExistsAtPath / isExecutableFileAtPath
alt candidate path resolved
DE-->>TOT: bundle path
else fallback to name lookup
TOT->>TOT: compute applicationSearchNames
loop per appName
TOT->>DE: applicationPathForName(appName)
DE->>WS: fullPath(forApplication:)
WS-->>DE: app path or nil
DE-->>TOT: path or nil
end
end
TOT-->>CV: available targets list
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![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
🤖 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 519-535: The test currently uses sidebarHelpPollUntil(timeout:
0.5, ...) to assert the emptyLabel remains visible but then waits up to 5s for
waitForCommandPaletteSnapshot(...) to return refinedSnapshot, allowing the label
to disappear after 0.5s on slow CI; change the logic so the no-blink check runs
until the refined snapshot is observed: remove the fixed 0.5s timeout and
instead poll sidebarHelpPollUntil(...) continuously while
waitForCommandPaletteSnapshot(windowId: mainWindowId, query: String(repeating:
"z", count: 9), ...) is pending (or combine into a single poll condition that
returns true only when the refined snapshot is seen and the emptyLabel still
exists). Update references: emptyLabel, sidebarHelpPollUntil,
waitForCommandPaletteSnapshot, refinedSnapshot, and commandPaletteResultRows so
the empty-state visibility is asserted for the full duration of the refined
snapshot wait.
- Around line 497-498: After calling seedWorkspaceSwitcherCorpus(workspaceCount:
96) ensure the seeded entries are actually indexed by polling the switcher
search until at least one known seeded workspace title appears (or until the
expected count is returned) before proceeding to the no-match assertion;
implement this by performing the same switcher search used later (e.g., call the
switcher search helper or getSwitcherResults/searchInSwitcher) in a retry loop
with a timeout (or use an XCTest expectation/XCTWaiter) and assert the presence
of a seeded workspace title or expected result count; update the same pattern
for the other occurrences (lines ~670-698) so the test fails if indexing didn’t
complete rather than silently using the default workspace set.
- Around line 479-540: The test
testSwitcherEmptyStateDoesNotBlinkWhileRefiningNoMatchQuery asserts a global "no
blink while refining" UX guarantee that touches shared infrastructure
(scheduleCommandPaletteResultsRefresh(forceSearchCorpusRefresh:)); remove or
relax the specific visibility assertion that enforces emptyLabel never
disappearing (the sidebarHelpPollUntil/XCTAssertFalse block checking
emptyLabelDisappearedWhileRefining) so this PR only verifies the
open-target/scoped behavior: keep the setup, ensure the refined query still
yields no results (the waitForCommandPaletteSnapshot/assert that resultRows are
empty) but do not assert on the empty-state label visibility or modify
scheduleCommandPaletteResultsRefresh(forceSearchCorpusRefresh:)'s seeding
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f06457cf-5c40-4694-a1d5-a800e97a5a33
📒 Files selected for processing (1)
cmuxUITests/SidebarHelpMenuUITests.swift
Comment on lines
+479
to
+540
| func testSwitcherEmptyStateDoesNotBlinkWhileRefiningNoMatchQuery() throws { | ||
| let app = XCUIApplication() | ||
| app.launchArguments += ["-AppleLanguages", "(en)", "-AppleLocale", "en_US"] | ||
| app.launchEnvironment["CMUX_UI_TEST_MODE"] = "1" | ||
| app.launchEnvironment["CMUX_SOCKET_PATH"] = socketPath | ||
| launchAndActivate(app) | ||
|
|
||
| XCTAssertTrue( | ||
| sidebarHelpPollUntil(timeout: 8.0) { | ||
| app.windows.count >= 1 | ||
| }, | ||
| "Expected the main window to be visible" | ||
| ) | ||
| XCTAssertTrue(waitForSocketPong(timeout: 12.0), "Expected control socket at \(socketPath)") | ||
|
|
||
| let mainWindowId = try XCTUnwrap( | ||
| socketCommand("current_window")?.trimmingCharacters(in: .whitespacesAndNewlines) | ||
| ) | ||
| try seedWorkspaceSwitcherCorpus(workspaceCount: 96) | ||
|
|
||
| let searchField = app.textFields["CommandPaletteSearchField"] | ||
| app.typeKey("p", modifierFlags: [.command]) | ||
| XCTAssertTrue(searchField.waitForExistence(timeout: 5.0), "Expected command palette search field") | ||
| searchField.click() | ||
|
|
||
| try debugTypeText(String(repeating: "z", count: 8)) | ||
|
|
||
| let emptyLabel = app.staticTexts["No workspaces match your search."].firstMatch | ||
| XCTAssertTrue( | ||
| sidebarHelpPollUntil(timeout: 5.0) { | ||
| guard emptyLabel.exists else { return false } | ||
| guard let snapshot = commandPaletteSnapshot(windowId: mainWindowId) else { return false } | ||
| return (snapshot["query"] as? String) == String(repeating: "z", count: 8) | ||
| && self.commandPaletteResultRows(from: snapshot).isEmpty | ||
| }, | ||
| "Expected the switcher to reach a visible no-results state before refining the query" | ||
| ) | ||
|
|
||
| try debugTypeText("z") | ||
|
|
||
| let emptyLabelDisappearedWhileRefining = sidebarHelpPollUntil(timeout: 0.5, pollInterval: 0.01) { | ||
| !emptyLabel.exists | ||
| } | ||
| XCTAssertFalse( | ||
| emptyLabelDisappearedWhileRefining, | ||
| "Expected refining an already-empty switcher query to keep the empty-state label visible" | ||
| ) | ||
|
|
||
| let refinedSnapshot = try XCTUnwrap( | ||
| waitForCommandPaletteSnapshot( | ||
| windowId: mainWindowId, | ||
| query: String(repeating: "z", count: 9), | ||
| timeout: 5.0 | ||
| ) { snapshot in | ||
| self.commandPaletteResultRows(from: snapshot).isEmpty | ||
| } | ||
| ) | ||
| XCTAssertTrue( | ||
| commandPaletteResultRows(from: refinedSnapshot).isEmpty, | ||
| "Expected the refined no-match query to stay empty. snapshot=\(refinedSnapshot)" | ||
| ) | ||
| } |
There was a problem hiding this comment.
This regression test is scoped to shared command-palette refresh behavior, not the open-target fix.
It locks a “no blink while refining” guarantee onto shared switcher refresh infrastructure, so this PR now depends on a broader UX behavior that maintainers have explicitly asked to keep out of feature-scoped changes.
Based on learnings, "scheduleCommandPaletteResultsRefresh(forceSearchCorpusRefresh:) is shared command‑palette infrastructure across all submenus. Do not change its sync‑seeding behavior within feature‑scoped PRs; treat brief initial flashes as consistent with existing submenus. Any UX improvement ... should be implemented and tested globally in a dedicated follow‑up PR."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmuxUITests/SidebarHelpMenuUITests.swift` around lines 479 - 540, The test
testSwitcherEmptyStateDoesNotBlinkWhileRefiningNoMatchQuery asserts a global "no
blink while refining" UX guarantee that touches shared infrastructure
(scheduleCommandPaletteResultsRefresh(forceSearchCorpusRefresh:)); remove or
relax the specific visibility assertion that enforces emptyLabel never
disappearing (the sidebarHelpPollUntil/XCTAssertFalse block checking
emptyLabelDisappearedWhileRefining) so this PR only verifies the
open-target/scoped behavior: keep the setup, ensure the refined query still
yields no results (the waitForCommandPaletteSnapshot/assert that resultRows are
empty) but do not assert on the empty-state label visibility or modify
scheduleCommandPaletteResultsRefresh(forceSearchCorpusRefresh:)'s seeding
behavior.
Comment on lines
+497
to
+498
| try seedWorkspaceSwitcherCorpus(workspaceCount: 96) | ||
|
|
There was a problem hiding this comment.
Assert that the seeded switcher corpus is actually searchable before the no-match check.
seedWorkspaceSwitcherCorpus only sends new_workspace/workspace.rename; it never waits for those titles to land in switcher results. If indexing lags, this test can pass against the default workspace set and stop exercising the large-corpus path it is supposed to cover.
Also applies to: 670-698
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmuxUITests/SidebarHelpMenuUITests.swift` around lines 497 - 498, After
calling seedWorkspaceSwitcherCorpus(workspaceCount: 96) ensure the seeded
entries are actually indexed by polling the switcher search until at least one
known seeded workspace title appears (or until the expected count is returned)
before proceeding to the no-match assertion; implement this by performing the
same switcher search used later (e.g., call the switcher search helper or
getSwitcherResults/searchInSwitcher) in a retry loop with a timeout (or use an
XCTest expectation/XCTWaiter) and assert the presence of a seeded workspace
title or expected result count; update the same pattern for the other
occurrences (lines ~670-698) so the test fails if indexing didn’t complete
rather than silently using the default workspace set.
Comment on lines
+519
to
+535
| let emptyLabelDisappearedWhileRefining = sidebarHelpPollUntil(timeout: 0.5, pollInterval: 0.01) { | ||
| !emptyLabel.exists | ||
| } | ||
| XCTAssertFalse( | ||
| emptyLabelDisappearedWhileRefining, | ||
| "Expected refining an already-empty switcher query to keep the empty-state label visible" | ||
| ) | ||
|
|
||
| let refinedSnapshot = try XCTUnwrap( | ||
| waitForCommandPaletteSnapshot( | ||
| windowId: mainWindowId, | ||
| query: String(repeating: "z", count: 9), | ||
| timeout: 5.0 | ||
| ) { snapshot in | ||
| self.commandPaletteResultRows(from: snapshot).isEmpty | ||
| } | ||
| ) |
There was a problem hiding this comment.
The no-blink assertion stops well before the refined query is allowed to settle.
You only watch the empty-state label for 0.5s, but the refined snapshot is allowed 5s to arrive. On slower CI, the label can disappear after that 0.5s window and this still passes. Keep the visibility check running until the 9-character snapshot is observed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmuxUITests/SidebarHelpMenuUITests.swift` around lines 519 - 535, The test
currently uses sidebarHelpPollUntil(timeout: 0.5, ...) to assert the emptyLabel
remains visible but then waits up to 5s for waitForCommandPaletteSnapshot(...)
to return refinedSnapshot, allowing the label to disappear after 0.5s on slow
CI; change the logic so the no-blink check runs until the refined snapshot is
observed: remove the fixed 0.5s timeout and instead poll
sidebarHelpPollUntil(...) continuously while
waitForCommandPaletteSnapshot(windowId: mainWindowId, query: String(repeating:
"z", count: 9), ...) is pending (or combine into a single poll condition that
returns true only when the refined snapshot is seen and the emptyLabel still
exists). Update references: emptyLabel, sidebarHelpPollUntil,
waitForCommandPaletteSnapshot, refinedSnapshot, and commandPaletteResultRows so
the empty-state visibility is asserted for the full duration of the refined
snapshot wait.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/ContentView.swift (1)
4830-4835:⚠️ Potential issue | 🟠 MajorAvoid live app discovery in the commands refresh hot path.
Line 4830 now runs
TerminalDirectoryOpenTarget.availableTargets()insidecommandPaletteContextSnapshot(). That snapshot is read by bothcommandPaletteCommandsFingerprint()andcommandPaletteCommands(), so each commands-mode query update can do the filesystem/LaunchServices lookup more than once before the background search even starts. This is a typing-sensitive path; please compute the set once per refresh or palette session and thread it through instead of doing live discovery from the snapshot.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ContentView.swift` around lines 4830 - 4835, The code calls TerminalDirectoryOpenTarget.availableTargets() inside commandPaletteContextSnapshot(), causing repeated live discovery during each commands refresh; instead compute availableTargets once per refresh/session and thread that result through the command-palette flow (e.g. call TerminalDirectoryOpenTarget.availableTargets() once before creating the snapshot in the refresh path and pass the resulting Set into commandPaletteContextSnapshot() or include it as an explicit parameter used by commandPaletteContextSnapshot(), commandPaletteCommandsFingerprint(), and commandPaletteCommands()); update uses of CommandPaletteContextKeys.terminalOpenTargetAvailable to read from the precomputed set rather than invoking availableTargets() again.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@Sources/ContentView.swift`:
- Around line 4830-4835: The code calls
TerminalDirectoryOpenTarget.availableTargets() inside
commandPaletteContextSnapshot(), causing repeated live discovery during each
commands refresh; instead compute availableTargets once per refresh/session and
thread that result through the command-palette flow (e.g. call
TerminalDirectoryOpenTarget.availableTargets() once before creating the snapshot
in the refresh path and pass the resulting Set into
commandPaletteContextSnapshot() or include it as an explicit parameter used by
commandPaletteContextSnapshot(), commandPaletteCommandsFingerprint(), and
commandPaletteCommands()); update uses of
CommandPaletteContextKeys.terminalOpenTargetAvailable to read from the
precomputed set rather than invoking availableTargets() again.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e7adafee-8ec5-4fa4-8146-109328bd2235
📒 Files selected for processing (2)
Sources/ContentView.swiftcmuxTests/CommandPaletteSearchEngineTests.swift
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
findon tagged build, confirmedOpen Current Directory in Finderstill appears in terminal-scoped resultsSummary by cubic
Fixes missing “Open Current Directory in …” actions by resolving apps via LaunchServices and recalculating availability at runtime; all open‑in commands now show in terminal panels even when apps are in non‑standard locations. Keeps the command palette empty state stable while refining a no‑match query to avoid blinking.
NSWorkspace.fullPath(forApplication:)) using names derived from bundle candidates.vscodeavailable when the app is found;vscodeInlinestill requirescode-tunnel./Applications, plus unit and UI tests for the no‑match refinement behavior.Written for commit b65c794. Summary will update on new commits.
Summary by CodeRabbit