From 3bca43d6b1f6acb2cb54a0d7563b9220e4ce2a36 Mon Sep 17 00:00:00 2001 From: Lawrence Chen <54008264+lawrencecchen@users.noreply.github.com> Date: Tue, 17 Mar 2026 18:59:13 -0700 Subject: [PATCH] fix(command-palette): address follow-up review comments PR: https://github.com/manaflow-ai/cmux/pull/1644 --- Sources/ContentView.swift | 93 ++++++++++++++----- cmuxUITests/SidebarHelpMenuUITests.swift | 108 +++++++++++++++++------ 2 files changed, 155 insertions(+), 46 deletions(-) diff --git a/Sources/ContentView.swift b/Sources/ContentView.swift index 34809815..f7debf31 100644 --- a/Sources/ContentView.swift +++ b/Sources/ContentView.swift @@ -1405,6 +1405,7 @@ struct ContentView: View { @State private var commandPaletteResolvedSearchScope: CommandPaletteListScope? @State private var commandPaletteResolvedSearchFingerprint: Int? @State private var commandPaletteResolvedMatchingQuery = "" + @State private var commandPaletteTerminalOpenTargetAvailability: Set = [] @State private var isCommandPaletteSearchPending = false @State private var commandPalettePendingActivation: CommandPalettePendingActivation? @State private var commandPaletteResultsRevision: UInt64 = 0 @@ -1566,6 +1567,10 @@ struct ContentView: View { } } + private struct CommandPaletteCommandsContext { + let snapshot: CommandPaletteContextSnapshot + } + private enum CommandPaletteContextKeys { static let hasWorkspace = "workspace.hasSelection" static let workspaceName = "workspace.name" @@ -1588,6 +1593,7 @@ struct ContentView: View { static let panelHasUnread = "panel.hasUnread" static let updateHasAvailable = "update.hasAvailable" + static let cliInstalledInPATH = "cli.installedInPATH" static func terminalOpenTargetAvailable(_ target: TerminalDirectoryOpenTarget) -> String { "terminal.openTarget.\(target.rawValue).available" @@ -3753,9 +3759,11 @@ struct ContentView: View { } private var commandPaletteCurrentSearchFingerprint: Int { - commandPaletteEntriesFingerprint( - for: commandPaletteListScope, - includeSurfaces: commandPaletteSwitcherIncludesSurfaceEntries + let scope = commandPaletteListScope + return commandPaletteEntriesFingerprint( + for: scope, + includeSurfaces: commandPaletteSwitcherIncludesSurfaceEntries, + commandsContext: scope == .commands ? commandPaletteCachedCommandsContext() : nil ) } @@ -3859,11 +3867,12 @@ struct ContentView: View { private func commandPaletteEntries( for scope: CommandPaletteListScope, - includeSurfaces: Bool + includeSurfaces: Bool, + commandsContext: CommandPaletteCommandsContext? = nil ) -> [CommandPaletteCommand] { switch scope { case .commands: - return commandPaletteCommands() + return commandPaletteCommands(commandsContext: commandsContext ?? commandPaletteCachedCommandsContext()) case .switcher: return commandPaletteSwitcherEntries(includeSurfaces: includeSurfaces) } @@ -3891,15 +3900,27 @@ struct ContentView: View { searchAllSurfaces: commandPaletteSearchAllSurfaces, query: effectiveQuery ) + let terminalOpenTargets = resolveCommandPaletteTerminalOpenTargets(for: scope) + if commandPaletteTerminalOpenTargetAvailability != terminalOpenTargets { + commandPaletteTerminalOpenTargetAvailability = terminalOpenTargets + } + let commandsContext = scope == .commands + ? commandPaletteCommandsContext(terminalOpenTargets: terminalOpenTargets) + : nil let fingerprint = commandPaletteEntriesFingerprint( for: scope, - includeSurfaces: includeSurfaces + includeSurfaces: includeSurfaces, + commandsContext: commandsContext ) guard force || cachedCommandPaletteScope != scope || cachedCommandPaletteFingerprint != fingerprint else { return } - let entries = commandPaletteEntries(for: scope, includeSurfaces: includeSurfaces) + let entries = commandPaletteEntries( + for: scope, + includeSurfaces: includeSurfaces, + commandsContext: commandsContext + ) commandPaletteSearchCommandsByID = Dictionary(uniqueKeysWithValues: entries.map { ($0.id, $0) }) let searchCorpus = entries.map { entry in CommandPaletteSearchCorpusEntry( @@ -4263,21 +4284,21 @@ struct ContentView: View { private func commandPaletteEntriesFingerprint( for scope: CommandPaletteListScope, - includeSurfaces: Bool + includeSurfaces: Bool, + commandsContext: CommandPaletteCommandsContext? = nil ) -> Int { switch scope { case .commands: - return commandPaletteCommandsFingerprint() + return commandPaletteCommandsFingerprint( + commandsContext: commandsContext ?? commandPaletteCachedCommandsContext() + ) case .switcher: return commandPaletteSwitcherEntriesFingerprint(includeSurfaces: includeSurfaces) } } - private func commandPaletteCommandsFingerprint() -> Int { - var hasher = Hasher() - hasher.combine(commandPaletteContextSnapshot().fingerprint()) - hasher.combine(AppDelegate.shared?.isCmuxCLIInstalledInPATH() ?? false) - return hasher.finalize() + private func commandPaletteCommandsFingerprint(commandsContext: CommandPaletteCommandsContext) -> Int { + commandsContext.snapshot.fingerprint() } private func commandPaletteSwitcherEntriesFingerprint(includeSurfaces: Bool) -> Int { @@ -4629,8 +4650,37 @@ struct ContentView: View { } } - private func commandPaletteCommands() -> [CommandPaletteCommand] { - let context = commandPaletteContextSnapshot() + private func commandPaletteCachedCommandsContext() -> CommandPaletteCommandsContext { + commandPaletteCommandsContext( + terminalOpenTargets: commandPaletteTerminalOpenTargetAvailability + ) + } + + private func resolveCommandPaletteTerminalOpenTargets( + for scope: CommandPaletteListScope + ) -> Set { + guard scope == .commands, + focusedPanelContext?.panel.panelType == .terminal else { + return [] + } + return TerminalDirectoryOpenTarget.availableTargets() + } + + private func commandPaletteCommandsContext( + terminalOpenTargets: Set + ) -> CommandPaletteCommandsContext { + let cliInstalledInPATH = AppDelegate.shared?.isCmuxCLIInstalledInPATH() ?? false + var snapshot = commandPaletteContextSnapshot(terminalOpenTargets: terminalOpenTargets) + snapshot.setBool(CommandPaletteContextKeys.cliInstalledInPATH, cliInstalledInPATH) + return CommandPaletteCommandsContext( + snapshot: snapshot + ) + } + + private func commandPaletteCommands( + commandsContext: CommandPaletteCommandsContext + ) -> [CommandPaletteCommand] { + let context = commandsContext.snapshot let contributions = commandPaletteCommandContributions() var handlerRegistry = CommandPaletteHandlerRegistry() registerCommandPaletteHandlers(&handlerRegistry) @@ -4776,7 +4826,9 @@ struct ContentView: View { } } - private func commandPaletteContextSnapshot() -> CommandPaletteContextSnapshot { + private func commandPaletteContextSnapshot( + terminalOpenTargets: Set? = nil + ) -> CommandPaletteContextSnapshot { var snapshot = CommandPaletteContextSnapshot() if let workspace = tabManager.selectedWorkspace { @@ -4827,7 +4879,7 @@ struct ContentView: View { snapshot.setBool(CommandPaletteContextKeys.panelHasUnread, hasUnread) if panelIsTerminal { - let availableTargets = TerminalDirectoryOpenTarget.availableTargets() + let availableTargets = terminalOpenTargets ?? TerminalDirectoryOpenTarget.availableTargets() for target in TerminalDirectoryOpenTarget.commandPaletteShortcutTargets { snapshot.setBool( CommandPaletteContextKeys.terminalOpenTargetAvailable(target), @@ -4893,7 +4945,7 @@ struct ContentView: View { title: constant(String(localized: "command.installCLI.title", defaultValue: "Shell Command: Install 'cmux' in PATH")), subtitle: constant(String(localized: "command.installCLI.subtitle", defaultValue: "CLI")), keywords: ["install", "cli", "path", "shell", "command", "symlink"], - when: { _ in !(AppDelegate.shared?.isCmuxCLIInstalledInPATH() ?? false) } + when: { !$0.bool(CommandPaletteContextKeys.cliInstalledInPATH) } ) ) contributions.append( @@ -4902,7 +4954,7 @@ struct ContentView: View { title: constant(String(localized: "command.uninstallCLI.title", defaultValue: "Shell Command: Uninstall 'cmux' from PATH")), subtitle: constant(String(localized: "command.uninstallCLI.subtitle", defaultValue: "CLI")), keywords: ["uninstall", "remove", "cli", "path", "shell", "command", "symlink"], - when: { _ in AppDelegate.shared?.isCmuxCLIInstalledInPATH() ?? false } + when: { $0.bool(CommandPaletteContextKeys.cliInstalledInPATH) } ) ) contributions.append( @@ -6393,6 +6445,7 @@ struct ContentView: View { commandPaletteResolvedSearchRequestID = commandPaletteSearchRequestID commandPaletteResolvedSearchScope = nil commandPaletteResolvedSearchFingerprint = nil + commandPaletteTerminalOpenTargetAvailability = [] isCommandPaletteSearchPending = false commandPalettePendingActivation = nil commandPaletteResultsRevision &+= 1 diff --git a/cmuxUITests/SidebarHelpMenuUITests.swift b/cmuxUITests/SidebarHelpMenuUITests.swift index dfd0e3b0..b16fdc33 100644 --- a/cmuxUITests/SidebarHelpMenuUITests.swift +++ b/cmuxUITests/SidebarHelpMenuUITests.swift @@ -313,11 +313,24 @@ final class CommandPaletteAllSurfacesUITests: XCTestCase { super.tearDown() } - func testCmdShiftPBackspaceReturnsToWorkspaceResults() throws { - let app = XCUIApplication() + private func configureSocketControlledLaunch( + _ app: XCUIApplication, + showSettingsWindow: Bool = false + ) { + app.launchArguments += ["-socketControlMode", "allowAll"] app.launchArguments += ["-AppleLanguages", "(en)", "-AppleLocale", "en_US"] app.launchEnvironment["CMUX_UI_TEST_MODE"] = "1" app.launchEnvironment["CMUX_SOCKET_PATH"] = socketPath + app.launchEnvironment["CMUX_SOCKET_ENABLE"] = "1" + app.launchEnvironment["CMUX_SOCKET_MODE"] = "allowAll" + if showSettingsWindow { + app.launchEnvironment["CMUX_UI_TEST_SHOW_SETTINGS"] = "1" + } + } + + func testCmdShiftPBackspaceReturnsToWorkspaceResults() throws { + let app = XCUIApplication() + configureSocketControlledLaunch(app) launchAndActivate(app) XCTAssertTrue( @@ -399,10 +412,7 @@ final class CommandPaletteAllSurfacesUITests: XCTestCase { func testCmdPSearchCanIncludeSurfacesFromOtherWorkspacesWhenEnabled() throws { let app = XCUIApplication() - app.launchArguments += ["-AppleLanguages", "(en)", "-AppleLocale", "en_US"] - app.launchEnvironment["CMUX_UI_TEST_MODE"] = "1" - app.launchEnvironment["CMUX_UI_TEST_SHOW_SETTINGS"] = "1" - app.launchEnvironment["CMUX_SOCKET_PATH"] = socketPath + configureSocketControlledLaunch(app, showSettingsWindow: true) launchAndActivate(app) XCTAssertTrue( @@ -478,9 +488,7 @@ final class CommandPaletteAllSurfacesUITests: XCTestCase { 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 + configureSocketControlledLaunch(app) launchAndActivate(app) XCTAssertTrue( @@ -501,6 +509,25 @@ final class CommandPaletteAllSurfacesUITests: XCTestCase { XCTAssertTrue(searchField.waitForExistence(timeout: 5.0), "Expected command palette search field") searchField.click() + let seededWorkspaceTitlePrefix = "\(noMatchWorkspaceQuery)-" + try debugTypeText(noMatchWorkspaceQuery) + + let seededSnapshot = try XCTUnwrap( + waitForCommandPaletteSnapshot(windowId: mainWindowId, query: noMatchWorkspaceQuery, timeout: 5.0) { snapshot in + self.commandPaletteResultRows(from: snapshot).contains { row in + ((row["title"] as? String) ?? "").hasPrefix(seededWorkspaceTitlePrefix) + } + }, + "Expected seeded workspace titles to be indexed before exercising the no-match path" + ) + XCTAssertTrue( + commandPaletteResultRows(from: seededSnapshot).contains { row in + ((row["title"] as? String) ?? "").hasPrefix(seededWorkspaceTitlePrefix) + }, + "Expected the seeded workspace corpus to be searchable before the no-match assertion. snapshot=\(seededSnapshot)" + ) + + try clearCommandPaletteSearchField(app: app, windowId: mainWindowId) try debugTypeText(String(repeating: "z", count: 8)) let emptyLabel = app.staticTexts["No workspaces match your search."].firstMatch @@ -516,26 +543,35 @@ final class CommandPaletteAllSurfacesUITests: XCTestCase { try debugTypeText("z") - let emptyLabelDisappearedWhileRefining = sidebarHelpPollUntil(timeout: 0.5, pollInterval: 0.01) { - !emptyLabel.exists + let refinedQuery = String(repeating: "z", count: 9) + var refinedSnapshot: [String: Any]? + var emptyLabelDisappearedWhileRefining = false + let refinedQueryResolvedWhileKeepingEmptyStateVisible = sidebarHelpPollUntil( + timeout: 5.0, + pollInterval: 0.01 + ) { + guard emptyLabel.exists else { + emptyLabelDisappearedWhileRefining = true + return false + } + guard let snapshot = commandPaletteSnapshot(windowId: mainWindowId) else { return false } + guard (snapshot["query"] as? String) == refinedQuery else { return false } + guard self.commandPaletteResultRows(from: snapshot).isEmpty else { return false } + refinedSnapshot = snapshot + return true } 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)" + refinedQueryResolvedWhileKeepingEmptyStateVisible, + "Expected the refined no-match query to resolve while keeping the empty-state label visible" + ) + let resolvedRefinedSnapshot = try XCTUnwrap(refinedSnapshot) + XCTAssertTrue( + commandPaletteResultRows(from: resolvedRefinedSnapshot).isEmpty, + "Expected the refined no-match query to stay empty. snapshot=\(resolvedRefinedSnapshot)" ) } @@ -570,7 +606,7 @@ final class CommandPaletteAllSurfacesUITests: XCTestCase { let searchField = app.textFields["CommandPaletteSearchField"] for _ in 0..<2 { app.typeKey(XCUIKeyboardKey.escape.rawValue, modifierFlags: []) - if sidebarHelpPollUntil(timeout: 1.0) { !searchField.exists } { + if sidebarHelpPollUntil(timeout: 1.0, condition: { !searchField.exists }) { return } } @@ -667,6 +703,22 @@ final class CommandPaletteAllSurfacesUITests: XCTestCase { XCTAssertEqual(response["ok"] as? Bool, true, "Expected debug.type to succeed. response=\(response)") } + private func clearCommandPaletteSearchField(app: XCUIApplication, windowId: String) throws { + let searchField = app.textFields["CommandPaletteSearchField"] + searchField.click() + app.typeKey("a", modifierFlags: [.command]) + app.typeKey(XCUIKeyboardKey.delete.rawValue, modifierFlags: []) + let clearedSnapshot = try XCTUnwrap( + waitForCommandPaletteSnapshot(windowId: windowId, query: "", timeout: 5.0), + "Expected the command palette query to clear" + ) + XCTAssertEqual( + clearedSnapshot["query"] as? String, + "", + "Expected the command palette query to clear" + ) + } + private func seedWorkspaceSwitcherCorpus(workspaceCount: Int) throws { guard workspaceCount > 1 else { return } @@ -675,7 +727,7 @@ final class CommandPaletteAllSurfacesUITests: XCTestCase { okUUID(from: socketCommand("new_workspace")), "Expected new_workspace to return a workspace ID" ) - let title = "\(noMatchWorkspaceQuery)-\(index)-" + String(repeating: "workspace-", count: 8) + let title = seededWorkspaceTitle(index: index) let response = try XCTUnwrap( socketJSON( method: "workspace.rename", @@ -696,6 +748,10 @@ final class CommandPaletteAllSurfacesUITests: XCTestCase { XCTAssertEqual(socketCommand("select_workspace 0"), "OK") } + private func seededWorkspaceTitle(index: Int) -> String { + "\(noMatchWorkspaceQuery)-\(index)-" + String(repeating: "workspace-", count: 8) + } + private func socketCommand(_ command: String) -> String? { ControlSocketClient(path: socketPath, responseTimeout: 2.0).sendLine(command) } @@ -720,7 +776,7 @@ final class CommandPaletteAllSurfacesUITests: XCTestCase { guard (snapshot["query"] as? String) == query else { return false } return predicate?(snapshot) ?? true } - return matched ? latest : latest + return matched ? latest : nil } private func commandPaletteSnapshot(windowId: String) -> [String: Any]? {