From 9d5c47137fe8746410aee3df3a016ea51708d427 Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Mon, 3 Aug 2026 19:49:56 -0700 Subject: [PATCH] temp checkin of files --- .../claude-opus-ssh-reconnect-judgment.md | 180 ++++++++++ .../ssh-reconnect-sidebar-control-design.md | 329 ++++++++++++++++++ 2 files changed, 509 insertions(+) create mode 100644 artifacts/claude-opus-ssh-reconnect-judgment.md create 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 new file mode 100644 index 00000000000..0890d762162 --- /dev/null +++ b/artifacts/claude-opus-ssh-reconnect-judgment.md @@ -0,0 +1,180 @@ +# 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 `