feat: support multiple terminal tabs on the agents right panel - #26089

The pending-tab promotion pattern is well-designed: connecting off screen, waiting for painted output or the fallback, then switching. The readiness signaling chain is correct through the useEffectEvent chain and the stale-closure guards on pendingTabId are sound. The invisible-absolute layout strategy correctly solves the canvas refit problem at the root (FitAddon dimensions) rather than patching symptoms. The type guard and storage validation are defensive and well-tested. Solid feature work.

3 P2, 8 P3, 3 Nit.

Hisoka, on what happens when someone opens their fifteenth terminal: "What happens when someone opens their fifteenth terminal and types fast in one of them, triggering re-renders on every keystroke? Shall I show you? ♥"

The tab overflow strategy changed from horizontal scroll (chevron buttons, useTabScroll hook) to flex-wrap. With many open tabs the tab bar now grows vertically, eating into terminal panel space. The previous horizontal scroll kept the tab bar compact at one row. This is a UX tradeoff bundled into the feature without mention in the PR description.

🤖 This review was automatically generated with Coder Agents.

All 14 R1 findings addressed. The hot/warm/detach lifecycle for CRF-3 is a well-calibrated alternative to a hard tab cap: it bounds the expensive resources (xterm, WebSocket, PTY) while keeping lightweight tab descriptors intact for later reattachment. The stale-closure fixes (CRF-6) use correct callback-form setState. The localStorage cleanup (CRF-4) is wired symmetrically with the existing sidebar tab cleanup. The play function on CloseableTabs (CRF-5) verifies close on active and inactive tabs.

1 P2, 1 P3, 1 Nit.

Hisoka traced rapid double-click, close-while-pending, warm-to-cold-to-hot cycles, built-in terminal restore, and useEffectEvent closure chains: "Boring code gets silence. Nothing here broke under inspection. ♠"

Pariston noted a redundant key={agentId} on AgentChatPageView (the parent KeyedAgentChatPage already applies the same key). Harmless but worth knowing.

Hisoka also flagged that isVisible passed to WorkspaceTerminal is a tautology in the new code: the component is conditionally rendered on the same boolean (shouldMountTerminal), so isVisible is always true at mount. Not harmful, not this PR's concern.

site/src/pages/AgentsPage/components/TerminalPanel.stories.tsx:34

P2 [CRF-16] Every TerminalPanel story except AgentUnavailable silently renders an empty terminal container.

The hot/warm lifecycle gate (line 166: {shouldMountTerminal && (<WorkspaceTerminal .../>)}) requires isHot to be truthy, but no story sets isHot in its args. Boolean(undefined) is false, isWarm initializes as useState(false), so shouldMountTerminal is false and WorkspaceTerminal never mounts.

Before this PR, TerminalPanel rendered <WorkspaceTerminal> unconditionally. Now Connected, StartingAgent, StartError, and Disconnected all render a TerminalPanel shell with no terminal inside. Since chromatic: { disableSnapshot: true } is set on the meta, no visual regression catches this.

Fix: add isHot: true to the story meta args.

(Bisky)

🤖

🤖 This review was automatically generated with Coder Agents.

All 17 findings across R1 and R2 resolved. The useTerminalWarmLifecycle extraction (CRF-17) is clean: the hook owns the timer and state, TerminalPanel consumes it, and three tests cover the key transitions. The isHot: true fix for the stories (CRF-16) is correct and minimal. Netero found no mechanical issues.

0 P2, 1 P3, 0 Nit.

Hisoka traced rapid-create, close-while-pending, manual-switch-cancels-pending, warm lifecycle, visibility tautology, key={agentId} boundary, and built-in terminal close vs user-tab close: "Three rounds in, seventeen findings addressed, and the code held up. ♦"

Chopper noted the CloseableTabs story marks the built-in Terminal as non-closeable, diverging from the real component where it is closeable. The close mechanism is exercised on user-created tabs, so the coverage gap is cosmetic.

Multiple reviewers noted isVisible={shouldMountTerminal} is a tautology (always true when WorkspaceTerminal is rendered). Harmless; the prop communicates intent and WorkspaceTerminal uses it independently in other contexts.

site/src/pages/AgentsPage/hooks/useTerminalWarmLifecycle.test.ts:25

P3 [CRF-19] All three tests start with { isHot: true }. The cold-start transition (isHot: false → true) is the primary mount path for terminal tabs restored from localStorage when another tab is active.

The hook initializes isWarm from Boolean(isHot) (line 28), so starting cold means shouldMountTerminal is false until isHot goes true. A change to the initialization or the effect's early-return guard could break first-mount silently.

it("mounts the terminal when a cold tab becomes hot", () => {
  const { result, rerender } = renderHook(
    ({ isHot }) => useTerminalWarmLifecycle({ isHot, detachDelayMs: TERMINAL_IDLE_DETACH_MS }),
    { initialProps: { isHot: false } },
  );
  expect(result.current.shouldMountTerminal).toBe(false);
  rerender({ isHot: true });
  expect(result.current.shouldMountTerminal).toBe(true);
});

(Bisky P3, Meruem P3, Chopper P3)

🤖

🤖 This review was automatically generated with Coder Agents.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a055112e02

The comments are wayyyyy too verbose. Lots of them are just re-iterating the implementation and add little/no value.