chore(web): extract App.tsx's connection lifecycle into useConnectionLifecycle

[cliffhall]

Closes #2154

Phase 2 step 3 of the App.tsx decomposition — see #2129 for the phase, #2126 for the whole effort. Step 2 (#2153, useOAuthRecovery) has merged, so this is cut from v2/main rather than stacked.

What moved

Everything about bringing a session up and taking it down now lives in clients/web/src/hooks/useConnectionLifecycle.ts:

• setupClientForServer (the heaviest single function in the file — the one place a connection is configured) and the layout effect that publishes it to the /oauth/callback rebuild
• onToggleConnection, onDisconnect, onReauthenticateFromBanner
• the deep-link auto-connect effect and its three phase latches
• the connected-server, client-teardown and disconnect-event effects
• connectErrorMessage / recordConnectError
• resetSessionScopedUiState
• sandboxUrlRef

useHandshakeTelemetry (connectStartRef + latencyMs) is exported from the same file but called separately. It is listed in step 3's scope and belongs here, but it cannot live inside the hook: useOAuthRecovery stamps the same ref on its /oauth/callback reconnect and runs first, so the ref has to exist earlier than the hook that otherwise owns it. Keeping both halves in the connection file was the better of the two options — the alternative was leaving them loose in App.tsx.

No behavior change. App.tsx goes 3,039 → 2,412 lines.

The reset() surface #2129 asked for

resetSessionScopedUiState reaches into most of phase 1's UI state on disconnect. Rather than handing the hook every setter — which would have made it depend on everything phase 1 extracted, decoupling nothing — it takes a single SessionResetSurface: one reset() per owner (clearResultPanels, resetTabUiState, resetTaskProgress, resetOAuthRecoveryState, resetLogLevels, closeConnectionInfoModal).

The two log-level operations stay App-owned deliberately: currentLogLevel / modernLogLevel are App state that the connect path also seeds, so the hook gets resetLogLevels() for the disconnect edge and seedModernLogLevel(settings) for the construction edge, and neither reaches a raw setter.

Coverage

useConnectionLifecycle.test.tsx — 52 tests, 100% statements / lines / functions, 99.34% branches.

InspectorClient is deliberately not mocked. setupClientForServer exists to translate a server entry into that constructor's options, so a stand-in would leave the whole translation unverified; the network-touching methods are spied on the prototype instead, and the assertions read back through the client's own getters (getServerSettings, getRoots, getSessionId).

One 401 detail worth noting for review: isUnauthorizedError keys off status/code, not the message, so the fixture is Object.assign(new Error("Unauthorized"), { status: 401 }). A plain Error("HTTP 401") takes the non-auth arm and every OAuth test would have passed for the wrong reason — it did, on the first run.

Verification

• npm run ci green from the repo root, all three web smokes included (plus the Firefox pass).
• The web coverage gate passes: 385 files / 6,941 tests.

No UI change, so no screenshots — the moves are inert and the three web smokes (smoke:web:browser, smoke:web:app, smoke:web:elicit) are the end-to-end backstop for that.

[Copilot]

Pull request overview

Extracts web connection lifecycle logic from App.tsx into a dedicated, tested hook without intended behavior changes.

Changes:

• Adds connection lifecycle and handshake telemetry hooks.
• Wires lifecycle dependencies through narrow reset and OAuth surfaces.
• Adds comprehensive hook tests.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
clients/web/src/hooks/useConnectionLifecycle.ts	Implements connection setup, teardown, deep-linking, and reauthentication.
clients/web/src/hooks/useConnectionLifecycle.test.tsx	Tests lifecycle, OAuth, telemetry, and deep-link behavior.
clients/web/src/App.tsx	Replaces inline lifecycle logic with the new hooks.

Suppressed comments (1)

clients/web/src/hooks/useConnectionLifecycle.ts:947

• Discarding this promise can also create an unhandled rejection: onToggleConnection may throw during client/store construction before reaching its internal try. Terminate it with a catch and expose the failure instead of leaving the banner retry silent.

    void onToggleConnection(serverId);

---

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

[cliffhall]

Copilot round 1 — all three addressed in 005e4c6. Mirroring here since inline threads go outdated once the fix is pushed.

1. useConnectionLifecycle.ts:582 — the config race was untested. Fair; the harness always handed the hook an already-resolved gate, so the await and sandboxUrlRef were both dead weight as far as the suite was concerned. Added waits for the config gate, then reads the sandbox URL as of then: the harness takes a configSettled promise, the test starts a connect with sandboxUrl: undefined, asserts nothing is constructed or connected while it is pending, rerenders with the settled URL, then releases the gate. The assertion is that newAppElicitationSession was called — construction read the URL as of the release, not the undefined in scope when the toggle was called.

2. useConnectionLifecycle.test.tsx:106 — misleading harness doc. Correct; both session inputs read p.client ?? null and setInspectorClient is only a spy, so there was never a last-constructed fallback. Doc rewritten to the actual contract.

3. useConnectionLifecycle.ts:904 / :947 — unhandled rejection on the banner retry. Real, and reachable: the deep-link test in this PR demonstrates the shape via destroyStores. Both sites now go through a retryConnect helper that awaits and catches.

One deliberate call on #3: it records the failure on connectErrorMessage rather than toasting. onToggleConnection already toasts everything it handles, so a toast here would be new user-visible behavior in what #2129 requires to be an inert relocation — and connectErrorMessage is what the deep-link phases own catch already does with the same class of failure. Unhandled rejection gone, diagnostic on the machine-readable surface, no UI change.

Coverage after the round: 55 tests, 100% statements / lines / functions, 99.35% branches. Gate green — all four client validates, the coverage gate, verify:build-gate, verify:bundle-externals, every smoke including the Firefox pass, and Storybook (119 files / 502 tests).

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

clients/web/src/hooks/useConnectionLifecycle.ts:283

• connectedServerId is completely derived from connectionStatus and activeServerId, so synchronizing it in an effect renders one stale frame and requires suppressing react-hooks/set-state-in-effect. Remove this state/effect pair and derive the returned value during render (connectionStatus === "connected" ? activeServerId : undefined).

  useEffect(() => {
    // eslint-disable-next-line react-hooks/set-state-in-effect -- pre-existing status-driven effect, unmasked when this component dropped below the React Compiler's bail-out (#2161)
    setConnectedServerId(
      connectionStatus === "connected" ? activeServerId : undefined,
    );

clients/web/src/hooks/useConnectionLifecycle.test.tsx:920

• This test never hydrates the server row or asserts a connection, so it does not verify the “still connects” behavior in its name. Rerender with the matching row after the rejected add and wait for connectSpy; otherwise a regression that permanently stops after the 409 remains green.

      await waitFor(() => expect(addServerImpl).toHaveBeenCalled());
      expect(h.api().connectErrorMessage).toBeUndefined();
    });

[cliffhall]

Copilot round 2 — no new comments on the changed code; two previously-missed findings, both answered.

useConnectionLifecycle.test.tsx:920 — the 409 test did not verify its own name. Fair hit, and on a test I wrote in this PR. It asserted only that addServer was called and nothing was recorded, so a deep link that swallowed the 409 and then stopped forever would have stayed green. Fixed in c37e795: it now rerenders with the hydrated row and waits for connectSpy, which is the behavior the name claims.

useConnectionLifecycle.ts:283 — connectedServerId should be derived, not effect-synced. Correct on the merits, and deliberately not fixed here.

That effect is pre-existing and moved inert; the eslint-disable on it is one of the nine suppressions #2153 added, and #2161 is the tracked issue for resolving them. It names this exact site: "connectedServerId is a candidate for plain derived state rather than a status-driven effect." Deriving it during render drops a frame of state, which is a real (if small) behavior change — and #2129 is explicit that a behavior change buried in a relocation is unreviewable, which is the same reason #2095 was left to reproduce through its own 135-line move. It belongs in #2161, where the change is the whole diff and the suppression comes off with it.

Gate green on c37e795, and test:scripts came back 222 pass / 0 fail this run — the red I saw on earlier runs was scripts/lib/render-smoke.test.mjss 400ms real-timer assertion losing its margin under a load average of 60–91 from other work on this machine, not anything in this diff.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

clients/web/src/hooks/useConnectionLifecycle.ts:85

• This synchronizes local state from the connectionStatus prop in a passive effect, so the connected frame can render the previous session's latency before the effect corrects it. The web state convention requires render-time adjustment via useValueChange (or restructuring telemetry around the connection callbacks); keep any ref mutation out of the useValueChange callback because it runs during render.

This issue also appears on line 279 of the same file.

  useEffect(() => {
    if (
      connectionStatus === "connected" &&
      connectStartRef.current !== undefined
    ) {
      setLatencyMs(Date.now() - connectStartRef.current);
      connectStartRef.current = undefined;
    } else if (connectionStatus !== "connected") {
      setLatencyMs(undefined);
    }

clients/web/src/hooks/useConnectionLifecycle.ts:284

• connectedServerId is exactly derived from connectionStatus and activeServerId, but this effect stores that derivation after paint and suppresses the rule that detects the stale frame. Remove the state/effect and derive the returned value directly (connectionStatus === "connected" ? activeServerId : undefined); this also removes the lint suppression.

  useEffect(() => {
    // eslint-disable-next-line react-hooks/set-state-in-effect -- pre-existing status-driven effect, unmasked when this component dropped below the React Compiler's bail-out (#2161)
    setConnectedServerId(
      connectionStatus === "connected" ? activeServerId : undefined,
    );
  }, [connectionStatus, activeServerId]);

[cliffhall]

Copilot round 3 — no new comments again. Two suppressed items: one repeat, one new. No code change this round.

useConnectionLifecycle.ts:284 — connectedServerId. Repeat of round 2; answered above. Tracked in #2161, which names this exact site.

useConnectionLifecycle.ts:85 — the latency effect should use useValueChange. Declining, on two grounds.

The stated failure does not occur. The claim is that "the connected frame can render the previous session's latency before the effect corrects it". It cannot: the else if (connectionStatus !== "connected") arm clears latencyMs on every non-connected status, and a reconnect or server switch always passes through connecting. So by the time a connected frame renders, the value is already undefined. The worst case is one frame of undefined before the measurement lands — which is correct, because at that instant the measurement does not exist yet.

And useValueChange cannot express this. latencyMs is not derived from connectionStatus; it is a measurement taken at an edge. Computing it reads the wall clock (Date.now()) and consumes a one-shot ref stamp (connectStartRef.current = undefined). Per AGENTS.md the useValueChange callback "must be pure — setState calls and nothing else. No fetches, DOM writes, logging, ref mutation" — the comment above rightly flags the ref mutation, but the clock read is equally disqualifying, and dropping the consume would make a second render re-measure against a stale stamp. AGENTS.md draws exactly this line: "An effect is still the right tool for genuine synchronization with an external system... The rule is about deriving React state from React props." A clock is an external system.

Worth noting this effect carries no eslint-disable: it does not trip react-hooks/set-state-in-effect, which is consistent with it not being a prop→state derivation, and it is therefore not among #2161's nine sites either.

Both remaining items are answered — one tracked, one declined on the merits — and there are no actionable comments outstanding.