Pending re-auth/step-up slots reset in an effect, painting the previous server's OAuth UI for one frame

[cliffhall]

Found in the v2.5.0 milestone-merge review (#2215), in code that shipped on v2/main during the milestone (#2153 / #2162). Filed here rather than fixed in the merge PR, whose tree is byte-identical to origin/v2/main.

The bug

clients/web/src/hooks/useOAuthRecovery.ts resets the two server-scoped pending-OAuth slots in an effect:

useEffect(() => {
  const pending = sessionRef.current.pendingReauth;
  if (pending && pending.serverId !== activeServerId) {
    setPendingReauth(null);
  }
  const stepUp = sessionRef.current.pendingStepUp;
  if (stepUp && stepUp.serverId !== activeServerId) {
    setPendingStepUp(null);
  }
}, [sessionRef, activeServerId]);

This is state derived from activeServerId, reset in an effect. AGENTS.md is unambiguous:

NEVER reset or re-sync local state from a prop inside a useEffect. useEffect(() => setX(prop), [prop]) renders once with the stale value, paints it, and only then corrects itself — the user sees the wrong frame and React renders twice.

So on a server switch, one frame commits carrying the previous server's pending re-auth or step-up UI before the reset lands. That UI is a modal-grade authorization affordance attached to the wrong server, which is a worse thing to flash than a stale list.

Why the lint gate did not catch it

react-hooks/set-state-in-effect is enabled at error for the web client and, since #2192 / #2194, for core/react/ too. It does not fire here because the effect reads the previous value from sessionRef.current rather than from a prop or a state variable — the rule sees a setState guarded by a ref read, not a prop-to-state sync.

That is worth recording as its own point: the ref indirection is what makes this invisible to the gate, so the class can recur anywhere the same mirror-into-a-ref pattern is used. The rule cannot be tightened to catch it; a reviewer has to.

Fix

Reset both slots during render with useValueChange, the pattern the rest of the decomposition uses:

useValueChange(activeServerId, () => {
  setPendingReauth((current) =>
    current && current.serverId !== activeServerId ? null : current,
  );
  setPendingStepUp((current) =>
    current && current.serverId !== activeServerId ? null : current,
  );
});

Two constraints from AGENTS.md that this shape satisfies and a naive port would not:

• activeServerId is a primitive, so it is referentially stable across no-change renders — exactly what useValueChange's Object.is comparison requires.
• The onChange runs during render and so must be pure. Reading the previous value from the updater argument rather than from sessionRef.current keeps it that way: a ref read during render is precisely the kind of external state that makes a replayed or abandoned render inconsistent.

useValueChange does not fire on first render, which is correct here — there is nothing pending to clear before a switch has happened.

Keep the separate mirror-into-sessionRef effect above it as is: that one writes a ref from state, which is legitimate synchronization and not a prop-to-state sync.

A test should assert the reset is visible in the first committed render after the switch, not merely eventually — otherwise it passes against the effect version too and proves nothing.

Reported by Copilot on #2215.