fix(tui): surface a disconnect failure independently of connection status

Review catch (Copilot, PR #2039): folding the rejected disconnect into
`connectError` meant nobody ever saw it. InfoTab renders that value only
when `serverState.status === "error"`, and a failed disconnect leaves the
status "connected" — so the message existed in state and never on screen.
Worse, it fed `connectError ?? inspectorLastError`, where a stale value
would go on masking a later, real connection error.

Give it its own `disconnectError` state and a header banner that renders
regardless of status, cleared on the next connect or disconnect attempt
and on a server switch, so it can neither linger nor be attributed to
the wrong server. The tests now assert the rendered banner rather than
noting that nothing is visible.

Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-08-16 21:16:34 -04:00
commit 5d8094357d
2 changed files with 44 additions and 17 deletions

View file

@ -665,17 +665,14 @@ describe("App (foundation)", () => {
// 'd' is a key handler, so it cannot await handleDisconnect — the handler // 'd' is a key handler, so it cannot await handleDisconnect — the handler
// has to own the failure itself or it escapes as an unhandled rejection // has to own the failure itself or it escapes as an unhandled rejection
// and fails the whole run from somewhere else (#1959). // and fails the whole run from somewhere else (#1959).
// There is nothing to assert on screen — the message only renders once the // The banner is deliberately independent of connection status: a rejected
// client reports `status: "error"`, which a rejected disconnect does not // disconnect leaves the status "connected", so anything gated on
// do. The assertion is the run itself: vitest fails on an unhandled // `status === "error"` would never be seen.
// rejection, so this test passing is the proof the catch owns it.
h.ctrl.status = "connected"; h.ctrl.status = "connected";
h.disconnect.mockRejectedValue(new Error("discfail")); h.disconnect.mockRejectedValue(new Error("discfail"));
const { stdin } = await mount(oneStdio()); const r = await mount(oneStdio());
stdin.write("d"); r.stdin.write("d");
await tick(); await expectFrame(r, "Disconnect failed: discfail");
await tick();
expect(h.disconnect).toHaveBeenCalled();
}); });
it("owns a non-Error disconnect rejection too", async () => { it("owns a non-Error disconnect rejection too", async () => {
@ -683,11 +680,21 @@ describe("App (foundation)", () => {
// `.message` off it; a throw here would escape the same way. // `.message` off it; a throw here would escape the same way.
h.ctrl.status = "connected"; h.ctrl.status = "connected";
h.disconnect.mockRejectedValue("plainstring"); h.disconnect.mockRejectedValue("plainstring");
const { stdin } = await mount(oneStdio()); const r = await mount(oneStdio());
stdin.write("d"); r.stdin.write("d");
await tick(); await expectFrame(r, "Disconnect failed: plainstring");
await tick(); });
expect(h.disconnect).toHaveBeenCalled();
it("clears a disconnect failure once a retry succeeds", async () => {
// A stale banner would keep reporting a failure the user has since fixed.
h.ctrl.status = "connected";
h.disconnect.mockRejectedValueOnce(new Error("discfail"));
const r = await mount(oneStdio());
r.stdin.write("d");
await expectFrame(r, "Disconnect failed: discfail");
r.stdin.write("d");
await waitUntil(() => !(r.lastFrame() ?? "").includes("Disconnect failed"));
expect(r.lastFrame() ?? "").not.toContain("Disconnect failed");
}); });
it("switches tabs via accelerator keys", async () => { it("switches tabs via accelerator keys", async () => {

View file

@ -170,6 +170,15 @@ function App({
pendingStepUpRef.current = pendingStepUp; pendingStepUpRef.current = pendingStepUp;
}, [pendingStepUp]); }, [pendingStepUp]);
const [connectError, setConnectError] = useState<string | null>(null); const [connectError, setConnectError] = useState<string | null>(null);
// A failed disconnect is deliberately NOT folded into `connectError`. That
// one is only rendered by InfoTab when the status is "error", which a
// rejected disconnect leaves untouched (the status stays "connected"), so
// the message would never be seen — and it feeds
// `connectError ?? inspectorLastError`, where a stale value would go on
// masking a later, real connection error. This renders in the header
// regardless of status and is cleared on the next connect/disconnect
// attempt and on a server switch.
const [disconnectError, setDisconnectError] = useState<string | null>(null);
const oauthInProgressRef = useRef(false); const oauthInProgressRef = useRef(false);
const callbackServerRef = useRef<OAuthCallbackServer | null>(null); const callbackServerRef = useRef<OAuthCallbackServer | null>(null);
const selectedServerRef = useRef<string | null>(null); const selectedServerRef = useRef<string | null>(null);
@ -434,6 +443,9 @@ function App({
useEffect(() => { useEffect(() => {
setOauthStatus("idle"); setOauthStatus("idle");
setOauthMessage(null); setOauthMessage(null);
// The header banner is server-scoped, so a failure from the server we just
// left must not be read as this one's.
setDisconnectError(null);
const stepUp = pendingStepUpRef.current; const stepUp = pendingStepUpRef.current;
if (stepUp && selectedServer && stepUp.serverName !== selectedServer) { if (stepUp && selectedServer && stepUp.serverName !== selectedServer) {
setPendingStepUp(null); setPendingStepUp(null);
@ -757,6 +769,8 @@ function App({
if (!selectedServer || !selectedInspectorClient || !selectedServerConfig) { if (!selectedServer || !selectedInspectorClient || !selectedServerConfig) {
return; return;
} }
// A connect attempt supersedes whatever the last disconnect reported.
setDisconnectError(null);
const finishConnect = async () => { const finishConnect = async () => {
await connectInspector(); await connectInspector();
@ -887,14 +901,15 @@ function App({
// Disconnect handler // Disconnect handler
const handleDisconnect = useCallback(async () => { const handleDisconnect = useCallback(async () => {
if (!selectedServer) return; if (!selectedServer) return;
// Clear first, so a retry that succeeds leaves no stale message behind.
setDisconnectError(null);
try { try {
await disconnectInspector(); await disconnectInspector();
// InspectorClient will update status automatically, and data is preserved // InspectorClient will update status automatically, and data is preserved
} catch (err) { } catch (err) {
// Nothing above this catches: the only caller is the key handler, which // Nothing above this catches: the only caller is the key handler, which
// cannot await. Surface the failure the same way handleConnect does // cannot await, so without this the rejection escapes unhandled.
// rather than letting it escape as an unhandled rejection. setDisconnectError(err instanceof Error ? err.message : String(err));
setConnectError(err instanceof Error ? err.message : String(err));
} }
}, [selectedServer, disconnectInspector]); }, [selectedServer, disconnectInspector]);
@ -1627,6 +1642,11 @@ function App({
<Text color="red">OAuth: {oauthMessage}</Text> <Text color="red">OAuth: {oauthMessage}</Text>
</Box> </Box>
)} )}
{disconnectError && (
<Box marginTop={1}>
<Text color="red">Disconnect failed: {disconnectError}</Text>
</Box>
)}
</Box> </Box>
</Box> </Box>