feat(user-testing): tell a tester when a server didn't connect - #4062
Open
olartgabo wants to merge 3 commits into
Open
feat(user-testing): tell a tester when a server didn't connect#4062olartgabo wants to merge 3 commits into
olartgabo wants to merge 3 commits into
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
olartgabo
temporarily deployed
to
preview-pr-4062
August 17, 2026 12:09 — with 
GitHub Actions
Inactive
GitHub Actions
Inactive
dosubot
Bot
added
size:L
Contributor
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Contributor
Internal previewPreview URL: https://mcp-inspector-pr-4062.up.railway.app |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 4 files
You’re at about 95% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
olartgabo
force-pushed
the
olartgabo/user-testing-connection-truth
branch
from
August 17, 2026 23:35
51eb159 to
3dff7ff
Compare
olartgabo
temporarily deployed
to
preview-pr-4062
August 17, 2026 23:35 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
dosubot
Bot
added
size:XL
A scenario built on an Excalidraw server ran a whole session without it. Nothing in the guest runtime ever connects before the first message — `/api/web/chatboxes/redeem` is a config fetch that never touches the servers it lists — and `ChatboxChatPage` then hard-coded `connectionStatus: "connected"` for every row in the payload. So the composer showed a green dot for a server nothing had reached, the turn quietly included it, and the tester was left asking a model with no tools why it couldn't draw. Probe each server the OAuth gate doesn't already own, up front, over the same /validate hop the chat turn would make. A server that doesn't answer reads `failed` instead of `connected` — which is what both the composer's dot and the turn's server selection read — and a banner names it to the tester in terms they can act on. The composer holds until the probes answer: sending early withholds a still-unknown server from the turn, which is the same silent failure wearing a different hat. Transport detail stays in the console. Someone who followed a link can't act on an SSE status code, and every other visitor-facing string on this route is curated for the same reason.
… bound its probes
Three defects in the reachability probe, all reachable from a normal tester
session.
A verdict outlived the session that produced it. `ScenarioChatPage` is rendered
without a `key` and swaps sessions in place, so `probedServerIdsRef` survived a
scenario switch: a server id listed by both scenarios kept the first session's
verdict and was never re-probed, showing an answer the new session never got.
The hook now takes a `sessionKey` and resets probe state when it changes, and a
probe that resolves after the switch is discarded. Keyed on `scenarioId`, not
`accessVersion` — a mid-session re-redeem bumps the version without changing
which servers the tester is exercising, and re-probing there would shut the
composer on every access recovery.
A timed-out probe kept running. `withProbeTimeout` rejected at the 15s deadline
but left the request in flight, so the retry opened a second connection to the
same server and both outlived unmount. `validateHostedServer` now accepts an
`AbortSignal` (threaded through `webPost`); the deadline aborts as well as
rejects, and unmount aborts whatever is still in flight.
An unprobed server did not hold the composer. `reachabilityByServerId` is
populated from an effect, so the first render after a session resolved had an
empty map and `=== "checking"` read false — a send could include a server
nothing had contacted. A probe-eligible server with no entry now reads as
checking. `reachableSessionServerIds` and `unreachableServerNames` still key on
a proven `unreachable` verdict: excluding unprobed servers from the turn would
drop them for no reason, and the banner must only name servers that failed.
Covered by "holds the composer on the very first frame, before the probe
registers", "abandons a probe that outlives the page", and "re-probes a shared
server when the tester opens another scenario". The first and third fail on the
old code with `true`/`false` and `"connected"`/`"failed"` respectively.
Not changed: the reviewer's claim that a `{ success: false }` body is treated as
a successful probe. `/api/web/servers/validate` returns 200 only via
`buildConnectSuccessEnvelope`, whose type is `success: true`, and `webPost`
throws `WebApiError` on any non-ok response, so that branch is unreachable.
Two ways the reachability probe told a tester a healthy server was unreachable. Losing the bootstrap race branded the server. `BOOTSTRAP_ATTEMPTS` exists because `/redeem` resolves before `useApiContext` has published this scenario's ids, so the first probes throw `BootstrapNotReadyError` without a round trip. Running out of those six attempts — 1.8s total — fell through to the same `unreachable` initializer as a refused connection, so a slow bootstrap showed the banner, painted the dot red, and dropped the server from `selectedServerIds`, with nothing left to re-probe it for the rest of the session. Nothing had reached the wire, so there was no evidence either way: exhausting the bootstrap attempts now leaves the server reachable and lets the turn make its own attempt and report its own failure. The sibling `useHostedOAuthRequirements` already resolves its version of this race to the benign default. A lost response cost two deadlines. `PROBE_TIMEOUT_MS` is 15s and the route's own connect timeout is 10s (`WEB_CONNECT_TIMEOUT_MS`), so reaching the deadline means the response was lost, not that the server was slow — but the timeout rejected with a plain `Error` and was counted as an ordinary wire failure, so it was retried for another 15s. The composer is shut for that whole time behind "Connecting to this session's tools…", with no way for the tester to send. `ProbeTimeoutError` now marks the attempt terminal, capping the hold at one deadline; the retry stays for the blips it was added for, which reject fast. Also stops counting the teardown abort as an attempt. Unmount aborts whatever is in flight, which lands in the same catch as a real failure; the verdict was then discarded anyway, after an unnecessary retry delay. It returns immediately now. Covered by "keeps a server reported connected when the probe never got to run" and "does not wait out a second deadline after the first response is lost". Both fail on the old code — the first with the banner present, the second with two `validateHostedServer` calls instead of one. Not changed: reading `result.success` before believing the probe. The reviewer raised it and the previous commit answered it — `/api/web/servers/validate` returns 200 only through `buildConnectSuccessEnvelope`, typed `success: true`, and `webPost` throws on any non-ok response. A test for that branch could only assert its own mock.
olartgabo
force-pushed
the
olartgabo/user-testing-connection-truth
branch
from
August 21, 2026 15:04
b916784 to
28e937e
Compare
dosubot
Bot
added
size:L
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The team ran a User Testing scenario built on an Excalidraw MCP server. It never
connected, the session said nothing, and the tester spent the whole session
asking a model with no tools why it couldn't draw.
Two things caused that:
/api/web/scenarios/redeemis a config fetch that never touches the servers itlists; the first real connection attempt happens inside a chat turn.
ScenarioChatPagethen hard-codedconnectionStatus: "connected"for everyserver in the payload. That map is what the composer's server list reads, so a
server nothing had ever reached showed a green dot.
What this does
Probes each server the OAuth gate doesn't already own, up front, over the same
/api/web/servers/validatehop the chat turn would make — so a pass here meansthe turn's connection works for the same reasons, and a failure here is the
failure the turn would have hit.
failed, notconnected.to the console: someone who followed a link can't act on an SSE status code,
and every other visitor-facing string on this route is curated for the same
reason.
still-unknown server from the turn, which is the same silent failure wearing a
different hat.
hostedContext.selectedServerIds. Thatfield wins over the status-filtered names inside
ChatTabV2, so it has to befiltered here or the turn still ships the dead server — and one server failing
listToolsrejects thePromise.allbehind the tool set, killing the wholeturn.
Scoping
Only servers with
useOAuth: falseare probed. EveryuseOAuthrow — includinga discover-mode one, which the mirror also reports as true — belongs to the OAuth
gate, which verifies them against this same endpoint. Probing those here would
double-connect and report a server merely awaiting consent as broken. So a
discover-mode server that is simply down stays silent for now; that's deliberate,
not an oversight.
A wire failure gets one retry, and the probe has a client-side deadline. Both
matter: the verdict is final for the session, so a 502 or a dropped connection
must not permanently brand a healthy server unreachable, and an unbounded probe
would leave the composer shut forever.
Testing
Three new cases in
ScenarioChatPage.test.tsx: the unreachable server is namedand reported
failedand dropped from the turn; a transient blip is retriedrather than believed; the composer holds until the probe answers.
ScenarioChatPage.test.tsx43/43.npm run typecheckandnpm run typecheck:clientboth exit 0.build:inspectorall pass.Still open, deliberately out of scope: nothing records connection outcome
server-side, so a scenario's session count still reads clean when every tester
got a dead server.
Summary by cubic
Probes non‑OAuth scenario servers on load and tells testers when a server didn’t connect. Previously all servers showed “connected” until the first turn; now we pre‑validate, show a failure banner, block sending until probes finish, and exclude failed servers from the turn.
Written for commit 28e937e. Summary will update on new commits.