From 5d8094357de7bd37841bc07a3e5576bae2c9ddba Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 16 Aug 2026 21:16:34 -0400 Subject: [PATCH] fix(tui): surface a disconnect failure independently of connection status MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- clients/tui/__tests__/App.test.tsx | 35 ++++++++++++++++++------------ clients/tui/src/App.tsx | 26 +++++++++++++++++++--- 2 files changed, 44 insertions(+), 17 deletions(-) diff --git a/clients/tui/__tests__/App.test.tsx b/clients/tui/__tests__/App.test.tsx index e4879b02..8a5124f5 100644 --- a/clients/tui/__tests__/App.test.tsx +++ b/clients/tui/__tests__/App.test.tsx @@ -665,17 +665,14 @@ describe("App (foundation)", () => { // '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 // and fails the whole run from somewhere else (#1959). - // There is nothing to assert on screen — the message only renders once the - // client reports `status: "error"`, which a rejected disconnect does not - // do. The assertion is the run itself: vitest fails on an unhandled - // rejection, so this test passing is the proof the catch owns it. + // The banner is deliberately independent of connection status: a rejected + // disconnect leaves the status "connected", so anything gated on + // `status === "error"` would never be seen. h.ctrl.status = "connected"; h.disconnect.mockRejectedValue(new Error("discfail")); - const { stdin } = await mount(oneStdio()); - stdin.write("d"); - await tick(); - await tick(); - expect(h.disconnect).toHaveBeenCalled(); + const r = await mount(oneStdio()); + r.stdin.write("d"); + await expectFrame(r, "Disconnect failed: discfail"); }); 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. h.ctrl.status = "connected"; h.disconnect.mockRejectedValue("plainstring"); - const { stdin } = await mount(oneStdio()); - stdin.write("d"); - await tick(); - await tick(); - expect(h.disconnect).toHaveBeenCalled(); + const r = await mount(oneStdio()); + r.stdin.write("d"); + await expectFrame(r, "Disconnect failed: plainstring"); + }); + + 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 () => { diff --git a/clients/tui/src/App.tsx b/clients/tui/src/App.tsx index 3f0f931d..0c779ab7 100644 --- a/clients/tui/src/App.tsx +++ b/clients/tui/src/App.tsx @@ -170,6 +170,15 @@ function App({ pendingStepUpRef.current = pendingStepUp; }, [pendingStepUp]); const [connectError, setConnectError] = useState(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(null); const oauthInProgressRef = useRef(false); const callbackServerRef = useRef(null); const selectedServerRef = useRef(null); @@ -434,6 +443,9 @@ function App({ useEffect(() => { setOauthStatus("idle"); 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; if (stepUp && selectedServer && stepUp.serverName !== selectedServer) { setPendingStepUp(null); @@ -757,6 +769,8 @@ function App({ if (!selectedServer || !selectedInspectorClient || !selectedServerConfig) { return; } + // A connect attempt supersedes whatever the last disconnect reported. + setDisconnectError(null); const finishConnect = async () => { await connectInspector(); @@ -887,14 +901,15 @@ function App({ // Disconnect handler const handleDisconnect = useCallback(async () => { if (!selectedServer) return; + // Clear first, so a retry that succeeds leaves no stale message behind. + setDisconnectError(null); try { await disconnectInspector(); // InspectorClient will update status automatically, and data is preserved } catch (err) { // Nothing above this catches: the only caller is the key handler, which - // cannot await. Surface the failure the same way handleConnect does - // rather than letting it escape as an unhandled rejection. - setConnectError(err instanceof Error ? err.message : String(err)); + // cannot await, so without this the rejection escapes unhandled. + setDisconnectError(err instanceof Error ? err.message : String(err)); } }, [selectedServer, disconnectInspector]); @@ -1627,6 +1642,11 @@ function App({ OAuth: {oauthMessage} )} + {disconnectError && ( + + Disconnect failed: {disconnectError} + + )}