From a75faa82f1e92cd3271cc451d620bdd5b2a5fd49 Mon Sep 17 00:00:00 2001 From: Lawrence Chen <54008264+lawrencecchen@users.noreply.github.com> Date: Thu, 12 Mar 2026 05:24:12 -0700 Subject: [PATCH] Fix SSH review regressions --- Resources/Localizable.xcstrings | 102 +++++++++++++++++++++ Sources/ContentView.swift | 51 +++++++++-- Sources/TabManager.swift | 61 ++++++++---- daemon/remote/cmd/cmuxd-remote/cli.go | 45 ++++++++- daemon/remote/cmd/cmuxd-remote/cli_test.go | 93 +++++++++++++++++++ 5 files changed, 328 insertions(+), 24 deletions(-) diff --git a/Resources/Localizable.xcstrings b/Resources/Localizable.xcstrings index 9d096941..137f9f92 100644 --- a/Resources/Localizable.xcstrings +++ b/Resources/Localizable.xcstrings @@ -61540,6 +61540,108 @@ } } }, + "sidebar.remote.help.connected": { + "extractionState": "manual", + "localizations": { + "en": { + "stringUnit": { + "state": "translated", + "value": "SSH connected to %@" + } + }, + "ja": { + "stringUnit": { + "state": "translated", + "value": "SSH は %@ に接続済み" + } + } + } + }, + "sidebar.remote.help.connecting": { + "extractionState": "manual", + "localizations": { + "en": { + "stringUnit": { + "state": "translated", + "value": "SSH connecting to %@" + } + }, + "ja": { + "stringUnit": { + "state": "translated", + "value": "SSH は %@ に接続中" + } + } + } + }, + "sidebar.remote.help.error": { + "extractionState": "manual", + "localizations": { + "en": { + "stringUnit": { + "state": "translated", + "value": "SSH error for %@" + } + }, + "ja": { + "stringUnit": { + "state": "translated", + "value": "%@ の SSH エラー" + } + } + } + }, + "sidebar.remote.help.errorWithDetail": { + "extractionState": "manual", + "localizations": { + "en": { + "stringUnit": { + "state": "translated", + "value": "SSH error for %@: %@" + } + }, + "ja": { + "stringUnit": { + "state": "translated", + "value": "%@ の SSH エラー: %@" + } + } + } + }, + "sidebar.remote.help.disconnected": { + "extractionState": "manual", + "localizations": { + "en": { + "stringUnit": { + "state": "translated", + "value": "SSH disconnected from %@" + } + }, + "ja": { + "stringUnit": { + "state": "translated", + "value": "SSH は %@ から切断済み" + } + } + } + }, + "sidebar.remote.help.targetFallback": { + "extractionState": "manual", + "localizations": { + "en": { + "stringUnit": { + "state": "translated", + "value": "remote host" + } + }, + "ja": { + "stringUnit": { + "state": "translated", + "value": "リモートホスト" + } + } + } + }, "sidebar.workspace.moveDownAction": { "extractionState": "manual", "localizations": { diff --git a/Sources/ContentView.swift b/Sources/ContentView.swift index de60f501..29fdf434 100644 --- a/Sources/ContentView.swift +++ b/Sources/ContentView.swift @@ -10477,20 +10477,59 @@ private struct TabItemView: View, Equatable { } private var remoteStateHelpText: String { - let target = tab.remoteDisplayTarget ?? "remote host" + let target = tab.remoteDisplayTarget ?? String( + localized: "sidebar.remote.help.targetFallback", + defaultValue: "remote host" + ) let detail = tab.remoteConnectionDetail?.trimmingCharacters(in: .whitespacesAndNewlines) switch tab.remoteConnectionState { case .connected: - return "SSH connected to \(target)" + return String( + format: String( + localized: "sidebar.remote.help.connected", + defaultValue: "SSH connected to %@" + ), + locale: .current, + target + ) case .connecting: - return "SSH connecting to \(target)" + return String( + format: String( + localized: "sidebar.remote.help.connecting", + defaultValue: "SSH connecting to %@" + ), + locale: .current, + target + ) case .error: if let detail, !detail.isEmpty { - return "SSH error for \(target): \(detail)" + return String( + format: String( + localized: "sidebar.remote.help.errorWithDetail", + defaultValue: "SSH error for %@: %@" + ), + locale: .current, + target, + detail + ) } - return "SSH error for \(target)" + return String( + format: String( + localized: "sidebar.remote.help.error", + defaultValue: "SSH error for %@" + ), + locale: .current, + target + ) case .disconnected: - return "SSH disconnected from \(target)" + return String( + format: String( + localized: "sidebar.remote.help.disconnected", + defaultValue: "SSH disconnected from %@" + ), + locale: .current, + target + ) } } private func moveWorkspaces(_ workspaceIds: [UUID], toWindow windowId: UUID) { diff --git a/Sources/TabManager.swift b/Sources/TabManager.swift index 6cfc01d1..2455e8d5 100644 --- a/Sources/TabManager.swift +++ b/Sources/TabManager.swift @@ -19,22 +19,31 @@ enum NewWorkspacePlacement: String, CaseIterable, Identifiable { var displayName: String { switch self { case .top: - return "Top" + return String(localized: "workspace.placement.top", defaultValue: "Top") case .afterCurrent: - return "After current" + return String(localized: "workspace.placement.afterCurrent", defaultValue: "After current") case .end: - return "End" + return String(localized: "workspace.placement.end", defaultValue: "End") } } var description: String { switch self { case .top: - return "Insert new workspaces at the top of the list." + return String( + localized: "workspace.placement.top.description", + defaultValue: "Insert new workspaces at the top of the list." + ) case .afterCurrent: - return "Insert new workspaces directly after the active workspace." + return String( + localized: "workspace.placement.afterCurrent.description", + defaultValue: "Insert new workspaces directly after the active workspace." + ) case .end: - return "Append new workspaces to the bottom of the list." + return String( + localized: "workspace.placement.end.description", + defaultValue: "Append new workspaces to the bottom of the list." + ) } } } @@ -732,7 +741,7 @@ class TabManager: ObservableObject { } var isFindVisible: Bool { - selectedTerminalPanel?.searchState != nil + selectedTerminalPanel?.searchState != nil || focusedBrowserPanel?.searchState != nil } var canUseSelectionForFind: Bool { @@ -740,13 +749,17 @@ class TabManager: ObservableObject { } func startSearch() { - guard let panel = selectedTerminalPanel else { return } - if panel.searchState == nil { - panel.searchState = TerminalSurface.SearchState() + if let panel = selectedTerminalPanel { + if panel.searchState == nil { + panel.searchState = TerminalSurface.SearchState() + } + NSLog("Find: startSearch workspace=%@ panel=%@", panel.workspaceId.uuidString, panel.id.uuidString) + NotificationCenter.default.post(name: .ghosttySearchFocus, object: panel.surface) + _ = panel.performBindingAction("start_search") + return } - NSLog("Find: startSearch workspace=%@ panel=%@", panel.workspaceId.uuidString, panel.id.uuidString) - NotificationCenter.default.post(name: .ghosttySearchFocus, object: panel.surface) - _ = panel.performBindingAction("start_search") + + focusedBrowserPanel?.startFind() } func searchSelection() { @@ -760,11 +773,21 @@ class TabManager: ObservableObject { } func findNext() { - _ = selectedTerminalPanel?.performBindingAction("search:next") + if let panel = selectedTerminalPanel { + _ = panel.performBindingAction("search:next") + return + } + + focusedBrowserPanel?.findNext() } func findPrevious() { - _ = selectedTerminalPanel?.performBindingAction("search:previous") + if let panel = selectedTerminalPanel { + _ = panel.performBindingAction("search:previous") + return + } + + focusedBrowserPanel?.findPrevious() } @discardableResult @@ -774,7 +797,12 @@ class TabManager: ObservableObject { } func hideFind() { - selectedTerminalPanel?.searchState = nil + if let panel = selectedTerminalPanel { + panel.searchState = nil + return + } + + focusedBrowserPanel?.hideFind() } @discardableResult @@ -1234,6 +1262,7 @@ class TabManager: ObservableObject { sentryBreadcrumb("workspace.close", data: ["tabCount": tabs.count - 1]) AppDelegate.shared?.notificationStore?.clearNotifications(forTabId: workspace.id) + workspace.teardownAllPanels() workspace.teardownRemoteConnection() unwireClosedBrowserTracking(for: workspace) diff --git a/daemon/remote/cmd/cmuxd-remote/cli.go b/daemon/remote/cmd/cmuxd-remote/cli.go index 3c667d67..14d69481 100644 --- a/daemon/remote/cmd/cmuxd-remote/cli.go +++ b/daemon/remote/cmd/cmuxd-remote/cli.go @@ -226,7 +226,7 @@ func execV2(socketPath string, spec *commandSpec, args []string, jsonOutput bool if jsonOutput { fmt.Println(resp) } else { - fmt.Println("OK") + fmt.Println(defaultRelayOutput(resp)) } return 0 } @@ -308,11 +308,52 @@ func runBrowserRelay(socketPath string, args []string, jsonOutput bool, refreshA if jsonOutput { fmt.Println(resp) } else { - fmt.Println("OK") + fmt.Println(defaultRelayOutput(resp)) } return 0 } +func defaultRelayOutput(resp string) string { + var result any + if err := json.Unmarshal([]byte(resp), &result); err != nil { + trimmed := strings.TrimSpace(resp) + if trimmed == "" { + return "OK" + } + return trimmed + } + + if relayResultIsEmpty(result) { + return "OK" + } + + switch typed := result.(type) { + case string: + return typed + default: + encoded, err := json.MarshalIndent(typed, "", " ") + if err != nil { + return "OK" + } + return string(encoded) + } +} + +func relayResultIsEmpty(result any) bool { + switch typed := result.(type) { + case nil: + return true + case map[string]any: + return len(typed) == 0 + case []any: + return len(typed) == 0 + case string: + return typed == "" + default: + return false + } +} + // flagToParamKey maps a CLI flag name to its JSON-RPC param key. func flagToParamKey(key string) string { switch key { diff --git a/daemon/remote/cmd/cmuxd-remote/cli_test.go b/daemon/remote/cmd/cmuxd-remote/cli_test.go index fff424db..32d08280 100644 --- a/daemon/remote/cmd/cmuxd-remote/cli_test.go +++ b/daemon/remote/cmd/cmuxd-remote/cli_test.go @@ -16,6 +16,33 @@ import ( "time" ) +func captureStdout(t *testing.T, fn func()) string { + t.Helper() + original := os.Stdout + reader, writer, err := os.Pipe() + if err != nil { + t.Fatalf("pipe stdout: %v", err) + } + os.Stdout = writer + defer func() { + os.Stdout = original + }() + + fn() + + if err := writer.Close(); err != nil { + t.Fatalf("close stdout writer: %v", err) + } + output, err := io.ReadAll(reader) + if err != nil { + t.Fatalf("read stdout: %v", err) + } + if err := reader.Close(); err != nil { + t.Fatalf("close stdout reader: %v", err) + } + return string(output) +} + // startMockSocket creates a Unix socket that accepts one connection, // reads a line, and responds with the given canned response. func startMockSocket(t *testing.T, response string) string { @@ -88,6 +115,46 @@ func startMockV2Socket(t *testing.T) string { return sockPath } +func startMockV2TCPSocketWithResult(t *testing.T, result any) string { + t.Helper() + ln, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatalf("failed to listen on TCP: %v", err) + } + t.Cleanup(func() { ln.Close() }) + + go func() { + for { + conn, err := ln.Accept() + if err != nil { + return + } + go func(conn net.Conn) { + defer conn.Close() + buf := make([]byte, 4096) + n, _ := conn.Read(buf) + if n == 0 { + return + } + var req map[string]any + if err := json.Unmarshal(buf[:n], &req); err != nil { + _, _ = conn.Write([]byte(`{"ok":false,"error":{"code":"parse","message":"bad json"}}` + "\n")) + return + } + resp := map[string]any{ + "id": req["id"], + "ok": true, + "result": result, + } + payload, _ := json.Marshal(resp) + _, _ = conn.Write(append(payload, '\n')) + }(conn) + } + }() + + return ln.Addr().String() +} + // startMockTCPSocket creates a TCP listener that responds with a canned response. func startMockTCPSocket(t *testing.T, response string) string { t.Helper() @@ -391,6 +458,32 @@ func TestCLIListWorkspacesV2(t *testing.T) { } } +func TestCLIListWorkspacesV2DefaultOutputShowsResult(t *testing.T) { + sockPath := startMockV2TCPSocketWithResult(t, map[string]any{"method": "workspace.list", "params": map[string]any{}}) + output := captureStdout(t, func() { + code := runCLI([]string{"--socket", sockPath, "list-workspaces"}) + if code != 0 { + t.Fatalf("list-workspaces should return 0, got %d", code) + } + }) + if !strings.Contains(output, "\"method\": \"workspace.list\"") { + t.Fatalf("expected default output to include result payload, got %q", output) + } +} + +func TestCLINotifyDefaultOutputPrintsOKForEmptyResult(t *testing.T) { + sockPath := startMockV2TCPSocketWithResult(t, map[string]any{}) + output := captureStdout(t, func() { + code := runCLI([]string{"--socket", sockPath, "notify", "--body", "hi"}) + if code != 0 { + t.Fatalf("notify should return 0, got %d", code) + } + }) + if strings.TrimSpace(output) != "OK" { + t.Fatalf("expected empty-result command to print OK, got %q", output) + } +} + func TestCLIRPCPassthrough(t *testing.T) { sockPath := startMockV2Socket(t) code := runCLI([]string{"--socket", sockPath, "rpc", "system.capabilities"})