Skip to content

fix(post-166): close-tab UX, dead RestartAll route, stale endpoints, partial-success - #167

Merged
aterrylu merged 1 commit into
mainfrom
terry/post-166-cleanup
May 8, 2026
Merged

fix(post-166): close-tab UX, dead RestartAll route, stale endpoints, partial-success#167
aterrylu merged 1 commit into
mainfrom
terry/post-166-cleanup

Conversation

@aterrylu

@aterrylu aterrylu commented May 8, 2026

Copy link
Copy Markdown
Owner

Why

Post-merge visual QA of #166 (Agent + Session unification) surfaced four regressions and one critical silent-failure missed by the unification PR's tests. This PR consolidates the cleanup.

What changed

🔴 X button on only tab silently no-op'd

File: packages/dashboard/src/store.ts

closeTabcloseLeafremoveLeaf returns null on the only leaf (layout invariant: ≥1 leaf must exist), and closeLeaf silently returned. Click went nowhere. Fix: fall through to removeTab so the leaf becomes empty and the existing "Create or select an agent to start" placeholder renders.

if (leaf.tabs.length <= 1) {
  const newRoot = removeLeaf(layout, leafId);
  if (newRoot) { /* sibling collapses up */ return; }
  // Only leaf in tree — fall through; removeTab below produces an
  // empty-tabs leaf which renders as the placeholder.
}
const updated = removeTab(layout, leafId, tabId);

Plus a defensive guard for stale tabId (cached event handlers, WS races, missed remap entries) that would otherwise reach the same dead-X UX through a different path.

🟡 RestartAll button posted to a route that no longer exists

Files: packages/server/src/routes/agents.ts, SettingsStatusBarItem.tsx

PR #166 renamed /api/sessions/api/agents but RestartAllButton was still posting to /api/sessions/restart-all. Added POST /api/agents/restart-all (delegates to restartAllAttachments() already exported by runtime.ts) and updated the caller.

🟡 Auth probe pointed at gone endpoint AND misclassified 5xx/404 as authenticated

File: packages/dashboard/src/App.tsx

Mount-time + Retry probes called /api/sessions; the 404 silently landed users on a broken main UI because the ternary res.status === 401 ? "unauthenticated" : "authenticated" mapped 404 → authenticated. Three-state classification: 401 → unauth; 2xx → auth; anything else → error (with logging) so the existing "Cannot connect to server" screen surfaces.

🔴 RestartAll silently dropped per-agent failures

Files: runtime.ts, routes/agents.ts, SettingsStatusBarItem.tsx

restartAllAttachments() returned only the success idMap; respawn exceptions were console.error'd and discarded. The route returned 200 with a partial idMap and the UI showed green "done" — N failed agents would silently not come back. Changed signature to { idMap, failures: Array<{id,name,error}> }; route forwards failures; UI surfaces non-empty failures (e.g. "2 agent(s) failed to restart: foo, bar") via the existing error band. Empty pty.kill() catch now logs the cause.

🟢 Stale endpoint references in comments / docstrings

  • mergeOrgWithSessions.ts, Sidebar.tsx (×3 comments), HierarchyPanel.tsx, Sidebar.mergeOrgWithSessions.test.ts/api/sessions and /api/org/api/agents and /api/agents/tree.

Test plan

  • make check — biome clean, TS clean, 306 server tests pass
  • vitest run src/store.test.ts -t "closeTab" — 4/4 pass (regression + stale-tabId + 2 happy paths)
  • Live dev verification in browser:
    • 0 console errors after page load (was 1: /api/sessions:0 404)
    • X button on only tab → tab disappears + placeholder shows
    • X button on one of two tabs → tab disappears, sibling stays
    • POST /api/agents/restart-all returns 200 with { idMap, failures: [] }
    • RestartAll UI: click → confirm → restart flow runs end-to-end

Reviewed by

  • pr-review-toolkit:code-reviewer — clean (no high-confidence findings)
  • pr-review-toolkit:code-simplifier — clean (no simplification warranted)
  • pr-review-toolkit:silent-failure-hunter — 5 findings, all addressed in this PR

Diff scope

10 files, +250/-38. No new dependencies.

Out of scope (separate follow-up)

  • packages/server/src/channel-server/dist.mjs build artifact still references old endpoints — needs a rebuild of the channel-server bundle (pre-existing on main, not on this branch).
  • 2 pre-existing dashboard test failures in switchPane > Case 1 and movePaneToLeaf > dissolves group — unrelated to this PR; fail on main too.

🤖 Generated with Claude Code

…l-success surfacing

Cleanup PR for regressions discovered during post-merge live QA of #166.

## Bugs fixed

### 1. X button on the only tab silently no-op'd — close-tab UX
`closeTab` → `closeLeaf` → `removeLeaf` returns `null` on the only leaf
(layout invariant: ≥1 leaf must exist), and `closeLeaf` then silently
returned. Fix: when the only leaf would be removed, fall through to
`removeTab` so the leaf becomes empty and the existing
"Create or select an agent to start" placeholder renders.

Adds 4 unit tests covering:
- multi-tab close (leaf intact, sibling tab takes over)
- last tab in one of multiple leaves (sibling leaf collapses up)
- last tab in the only leaf (regression — empty-leaf placeholder)
- stale tabId (defensive — the fall-through must not no-op silently)

### 2. RestartAll button was wired to a route that no longer exists
PR #166 renamed `/api/sessions` → `/api/agents` but the
SettingsStatusBarItem RestartAll button was still posting to
`/api/sessions/restart-all`. Added `POST /api/agents/restart-all` server
route (calls existing `restartAllAttachments()` from runtime.ts) and
updated the dashboard caller.

### 3. App.tsx auth probe still pointed at the gone endpoint
Mount-time and Retry probes were calling `/api/sessions` (404). The
ternary `res.status === 401 ? "unauthenticated" : "authenticated"` was
also misclassifying the 404 (and any future 5xx / 404 / 403) as
authenticated, silently landing the user on a broken main UI. Changed
to a 3-state classification: 401 → unauthenticated; 2xx → authenticated;
anything else → error (with logging) so the existing "Cannot connect to
server" screen surfaces.

### 4. RestartAll silently dropped per-agent failures
`restartAllAttachments()` returned only the success `idMap`; respawn
errors were `console.error`'d and discarded. The route returned 200 with
a partial idMap and the UI showed a green "done" state — N failed
agents would simply not come back.

Changed signature to return `{ idMap, failures: Array<{id, name, error}> }`.
The route forwards `failures` in the response body. RestartAllButton
surfaces non-empty failures via the existing error UI (e.g. "2 agent(s)
failed to restart: foo, bar"). Also logs the previously-empty
`pty.kill()` catch and the "agent record vanished mid-restart" branch.

### 5. Stale endpoint references in comments / docstrings
Carried-over `/api/sessions` and `/api/org` references replaced with
the current `/api/agents` and `/api/agents/tree`:
- mergeOrgWithSessions.ts (docstring)
- Sidebar.tsx (3 comments — fingerprint refresh, fallback notice, error state)
- HierarchyPanel.tsx (DELETE endpoint reference)
- Sidebar.mergeOrgWithSessions.test.ts (header + race-condition comment)

## Verification

- All 306 server tests pass
- 4 new closeTab tests pass (regression + stale-tabId guard + 2 happy paths)
- Live dev verification: 0 console errors, X button closes tab to placeholder,
  POST /api/agents/restart-all returns 200 `{ idMap, failures }`, RestartAll UI
  renders confirmation → restart → success/partial-failure flow correctly.

## Reviewed by
- code-reviewer: clean (no high-confidence findings)
- code-simplifier: clean (no simplification warranted)
- silent-failure-hunter: 5 findings — all addressed in this PR

@nox-0x nox-0x left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving — all four fixes match the symptoms described, the new tests cover the regressions cleanly, and no critical issues in the current code.

Verified:

  • closeTab fall-through (store.ts) — removeLeaf returns null for the only leaf, so the fall-through into removeTab is the correct path to produce an empty leaf that renders the placeholder. The defensive stale-tabId guard prevents the same dead-X UX via WS races. Test coverage is solid (closeTab > REGRESSION + the stale-tabId guard test).
  • POST /api/agents/restart-all — route doesn't shadow any earlier matcher (POST /:id only exists for PATCH; POST /:id/{kill,manager,attach} need two segments). Reachable.
  • Three-state auth probe (App.tsx) — both mount and Retry now route 5xx/404/403 to error, which surfaces the existing "Cannot connect" screen with the Retry button. Catch handlers also log the cause.
  • restartAllAttachments partial-successfailures is plumbed through runtime → route → UI. Empty pty.kill() catch now logs.

Follow-up (not blocking)

When respawnAgent throws inside restartAllAttachments, the agent record is left as status: "running" in the store with no live PTY (zombie). The captured pty.onExit handler returns early because live.get(id)?.pty !== pty — undefined !== oldPty — so it never marks the agent exited. resumeActiveAgents handles the same error by calling markExited(a.id, "crashed") + emitting agent.exited; mirroring that pattern here would keep the data layer consistent with the new UI signal. The PR's UI surfacing is already a clear improvement, so this is a polish follow-up rather than a blocker.

Also pre-existing on main (out of scope here): CreateAgentPanel.handleCreate calls closeTab(leaf.id, "create-agent") passing the pane id where the tab id is expected — currently masked because switchPane Case 4 has already replaced the layout by then, but the new defensive guard will start logging a [layout] closeTab: tabId "create-agent" not found warning if Case 4 ever stops firing for newly-created sessions. Worth fixing alongside the next CreateAgentPanel touch.

@aterrylu
aterrylu merged commit d8efda1 into main May 8, 2026
1 check passed
@aterrylu
aterrylu deleted the terry/post-166-cleanup branch May 8, 2026 09:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants