From 5376266b5fbf1aad79ebf14bbe9dec6242e65723 Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Mon, 3 Aug 2026 21:45:46 -0700 Subject: [PATCH] fix(ssh): hold connect lock for backend request, not UI timeout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace manual begin/end pairs with trackSshConnect wrapper that holds the lock for the full backend request duration. Previously, the lock was released after the UI timeout fired, even though the backend was still dialing — a second click on any surface for this host would trigger a second connect and a duplicate credential prompt on passphrase-gated targets. The wrapper survives unmount, unlike a finally block in a component handler. --- .../claude-opus-ssh-reconnect-judgment.md | 180 ---------- .../ssh-reconnect-sidebar-control-design.md | 329 ------------------ .../components/NewWorkspaceComposerCard.tsx | 23 +- .../sidebar/WorktreeCardSshHostControl.tsx | 17 +- .../TerminalSshReconnectOverlay.tsx | 17 +- .../src/ssh/ssh-connect-in-flight.test.ts | 59 +++- src/renderer/src/ssh/ssh-connect-in-flight.ts | 16 + 7 files changed, 100 insertions(+), 541 deletions(-) delete mode 100644 artifacts/claude-opus-ssh-reconnect-judgment.md delete mode 100644 artifacts/ssh-reconnect-sidebar-control-design.md diff --git a/artifacts/claude-opus-ssh-reconnect-judgment.md b/artifacts/claude-opus-ssh-reconnect-judgment.md deleted file mode 100644 index 0890d762162..00000000000 --- a/artifacts/claude-opus-ssh-reconnect-judgment.md +++ /dev/null @@ -1,180 +0,0 @@ -# Judgment — Sidebar SSH reconnect affordance (Grok vs Codex) - -**Judge:** Claude Opus 5 (independent UI design judge) -**Date:** 2026-08-03 -**Artifacts reviewed:** both standalone HTMLs (read in full) and both PNG renders (viewed). -**Grounding:** `docs/STYLEGUIDE.md`, `src/renderer/src/assets/main.css`, `src/renderer/src/components/sidebar/WorktreeCard.tsx`, `src/renderer/src/components/sidebar/SshDisconnectedDialog.tsx`, `src/renderer/src/components/terminal-pane/TerminalSshReconnectOverlay.tsx`, `src/renderer/src/components/sidebar/WorktreeCardMetadataControls.tsx`, `src/renderer/src/components/ui/button.tsx`, `src/shared/ssh-types.ts`. -**No production code was modified.** - -> Note on the renders: both PNGs are single-viewport captures that cut off at ~1000px, so the state-matrix / mapping / a11y sections below the fold are visible only in the HTML. Judgment below is based on the HTML for content and on the PNGs for actual rendered visual weight — which is where the most decisive difference showed up. - ---- - -## 1. Verified ground truth (what the code actually does today) - -| Fact | Evidence | -| --- | --- | -| The sidebar cue is a passive, non-focusable `` wrapping `ServerOff className="size-3 text-red-400"` with tooltip "SSH disconnected". | `WorktreeCard.tsx:1430–1452` | -| A *second*, near-identical block exists for runtime ("Orca server") hosts, also `ServerOff text-red-400`, also passive. | `WorktreeCard.tsx:1455–1490` | -| Card click opens the blocking dialog **only** when the active view is not a terminal. | `WorktreeCard.tsx:880–883`, comment at `:365` and `:393` | -| Whole card gets `opacity-60` when SSH **or** runtime is disconnected. | `WorktreeCard.tsx:1892` | -| `sshStatus` is `null` for runtime-owned SSH targets (deliberate — suppresses false "disconnected"). | `WorktreeCard.tsx:352–358` | -| `sshOwnerEnvironmentId` is already computed in the card, so runtime-routed connect is reachable from the sidebar today. | `WorktreeCard.tsx:349–351` | -| `SshConnectionStatus` has **8** members: `disconnected, connecting, auth-failed, deploying-relay, connected, reconnecting, reconnection-failed, error`. | `src/shared/ssh-types.ts:102–110` | -| Reconnectable set is `['disconnected','reconnection-failed','error','auth-failed']`, duplicated verbatim in the dialog and the overlay. | `SshDisconnectedDialog.tsx:59–61` vs `TerminalSshReconnectOverlay.tsx:35–37` | -| The overlay's connect is **not** a bare `ssh.connect`: it branches on `sshOwnerEnvironmentId`, mirrors `setSshConnectionState` (because `ssh.connect` can resolve before the state IPC lands), and on failure resyncs target metadata + removed labels (STA-1468). | `TerminalSshReconnectOverlay.tsx:91–137` | -| A removed target must never be offered Connect; `targetRemoved` is a real prop and `removedSshTargetLabels` lives in the renderer store, so the sidebar can derive it. | `TerminalSshReconnectOverlay.tsx:19–21, 88–89`; `store/slices/ssh.ts:39`; `store/slices/runtime-environment-ssh.ts:300, 321` | -| There is an existing test contract that the dialog must not auto-open for a restored active disconnected worktree. | `WorktreeCard.ssh-reconnect-prompt.test.tsx:147` | -| `WorktreeCard.tsx` is the **only** non-test consumer of `SshDisconnectedDialog`. | grep across `src` | -| `Button` has both `xs` (h-6, 12px svg) and `icon-xs` (size-6) — both proposals' primitive claims are valid. | `ui/button.tsx:23, 27` | -| **Precedent A — icon-only ghost action in the card:** `MetadataActionIcon` = `Button variant="ghost" size="icon-xs" className="size-6"` + `stopPropagation` + Tooltip. | `WorktreeCardMetadataControls.tsx:38–60` | -| **Precedent B — compact *labeled* destructive action in the title row:** the "rename failed" control is a `Button variant="ghost"` at `h-4`, `text-[10px]`, `text-destructive border-destructive/40 bg-destructive/10`, icon `size-2.5`, **plus** a tooltip even though it has a visible label. | `WorktreeCard.tsx:1523–1551` | -| Sidebar surface tokens in this pane are the `worktree-sidebar` family (`--worktree-sidebar: #2a2a2a` in dark), not the generic `--sidebar` (#171717). | `main.css:287–288`; `WorktreeList.tsx:4086, 4227, 4272` | - -Precedent B matters a lot: it means a short *labeled* pill in the title row is house style, not an invention — and that a tooltip alongside a visible label is already accepted here, so the STYLEGUIDE line "don't tooltip a control that has a visible label" is not a live objection against Codex. - ---- - -## 2. Scorecard - -Scale 1–5. Weighting is mine, stated so it can be argued with. - -| Criterion | Weight | Grok | Codex | -| --- | --- | --- | --- | -| Clarity of the affordance | 1.0 | 3 | **5** | -| Discoverability (the actual brief) | 1.5 | 2 | **5** | -| Consistency with Orca style / components / tokens | 1.25 | **5** | 3 | -| Accessibility | 1.25 | 3 | **4** | -| State handling: connected / connecting / error / retry / removed | 1.5 | **5** | 2 | -| Minimal UI disruption | 1.25 | **5** | 2 | -| Implementation fit (does the wiring survive contact with the code) | 1.25 | 3 | **5** | -| Correctness of stated assumptions about the codebase | 1.0 | **5** | 3 | -| **Weighted total (max 50)** | | **38.75** | **35.5** | - ---- - -## 3. Grok — assessment - -### What it gets right - -1. **Complete state model.** All 8 `SshConnectionStatus` members are enumerated and mapped, including `deploying-relay` and `error`, which Codex silently drops. Its `isConnectingStatus` / `canConnectStatus` predicates are copied correctly from the real source. -2. **It is the only proposal that handles `targetRemoved`.** The ghost state ("SSH host removed — reconnect unavailable", no Connect, overlay offers Remove workspace) is a faithful port of `TerminalSshReconnectOverlay.tsx:88–89` and of the STA-1468 lesson: never render a Connect that can only fail. This is the single largest correctness gap between the two. -3. **Token fidelity.** It mirrors the `worktree-sidebar` family (`#2a2a2a` fill, `#353535` accent, `worktree-sidebar-ring`), keeps `red-400` / `yellow-500` exactly as the existing dialog and card use them, invents no tokens, and keeps the existing `opacity-60`. The render visibly *is* the Orca sidebar. -4. **Accurate, checkable citations.** `~1430–1452`, `~880–883`, the `WorktreeCardMetadataControls` pattern, the focus-steal rationale on `showDisconnectedDialog` — every one verified true. It also correctly identifies that the reconnectable-status predicate is duplicated between dialog and overlay and proposes extracting it. -5. **Respects the runtime-host boundary.** It explicitly says: do not mint a chip when `sshStatus` is `null` (runtime-owned targets), and treat the runtime-host `ServerOff` block at `:1455+` as a follow-up gated on a runtime reconnect API existing. Codex never mentions that second block at all, even though it renders the identical red icon. -6. **Additive, not subtractive.** Chip is a new parallel path; the tested dialog behavior and the overlay stay as-is. Lowest-risk landing. - -### Where it is wrong or weak - -1. **It under-delivers the brief.** The whole premise is "the red icon looks like a status badge, not a control" — and the fix is… the same 12px red icon, in the same place, at the same weight. Compare the two PNGs: Grok's after-state is visually indistinguishable from its own before-state except for a tooltip. Hover-to-discover does not solve discoverability for a user who never suspects the glyph is clickable. Grok's own non-goal ("no permanent Reconnect text button — too heavy for a dense sidebar") is asserted, not argued, and Precedent B (`rename failed`) refutes it: the card already carries a labeled 10px destructive pill in that exact row. -2. **Real a11y defect: `disabled` for the connected and removed states.** Grok renders the chip as a `