Skip to content

fix(desktop): say "Reconnecting" while the client is auto-reconnecting - #5029

Open
sunnnsolar wants to merge 1 commit into
block:mainfrom
sunnnsolar:fix/relay-card-auto-reconnect
Open

fix(desktop): say "Reconnecting" while the client is auto-reconnecting#5029
sunnnsolar wants to merge 1 commit into
block:mainfrom
sunnnsolar:fix/relay-card-auto-reconnect

Conversation

@sunnnsolar

Copy link
Copy Markdown

Problem

The sidebar relay card renders "Can't reach the relay" / "Click to connect" for every degraded connection state — including reconnecting, which is the state the relay client sets while its own backoff loop is actively retrying.

So a transient drop the app is already recovering from without help is presented as a dead connection the user has to go fix by hand. Clicking does not make it recover any faster.

The card had no way to tell the two cases apart. It only receives isReconnectPending, which tracks a manual reconnect the user asked for via useReconnectRelay — that flag is false for the whole duration of the background loop. ConnectionState already distinguishes reconnecting from disconnected/stalled; the card simply never saw it.

What this changes

Threads the existing state through as isAutoReconnecting and uses it only for the no-user-action-needed case:

state before after
reconnecting "Can't reach the relay" / "Click to connect", role=alert "Reconnecting" / "Trying to restore the connection", spinner, role=status
disconnected, stalled "Can't reach the relay" / "Click to connect" unchanged
manual reconnect pending "Connecting" / "Reconnecting" unchanged (manual outranks the background loop)

Two deliberate choices:

  • stalled keeps the alarm copy. The socket is open but no frames are arriving, so the client is not making progress on its own — that one genuinely warrants the user. Only reconnecting is downgraded.
  • The action button stays enabled while auto-reconnecting. Anyone who does not want to wait out the backoff can still force an attempt immediately.

role drops from alert to status for the auto-reconnecting case so screen readers do not announce a self-healing blip as an assertive alarm.

Why it matters

On a hosted community I measured 420 reconnects over 6 days across the agent sessions on one machine. 44% of the gaps between them cluster in a 15m30s–20m band (median 17.1 min) with a hard floor at 15m30s — the shape of a proxy/LB max connection age, not instability. The overwhelming majority recover on the first attempt in under a second.

useRelayConnection already debounces degraded states by 2s, so the sub-second ones never paint. The ones that do paint are still usually recovering unassisted within a few seconds — and for that whole window the sidebar is telling the user the app is broken and they need to intervene.

Test changes

relay-connectivity.spec.ts tests 01, 02 and 06 drive reconnecting and asserted the alarm copy; they now assert the reconnecting copy and not.toContainText("Click to connect"). The stalled assertions in sidebar-relay-card.spec.ts are untouched and still pass.

That file's local settle() helper now delegates to the shared waitForAnimations. The card renders a looping spinner in this state, and a looping animation's .finished never resolves — the old unbounded Promise.allSettled(document.getAnimations()) hung until Playwright aborted the evaluate. The shared helper races the wait against a ceiling for exactly this reason (and its doc comment calls out spinners specifically).

Verification

  • pnpm check — clean
  • pnpm test — 3923 pass, 0 fail
  • pnpm exec playwright test --project=smoke relay-connectivity.spec.ts sidebar-relay-card.spec.ts — 6 passed

I do not have a screenshot in this PR: scripts/post-screenshots.sh pushes to a branch on this repo, which I cannot write to from a fork. Happy to attach one if a maintainer would rather see it.

The sidebar relay card renders "Can't reach the relay" / "Click to
connect" for every degraded state, including `reconnecting` — the state
the relay client sets while its own backoff loop is actively retrying.
So a transient drop that the app is already recovering from unassisted
is presented as a dead connection the user has to go fix by hand.

The card had no way to tell the two apart: it only receives
`isReconnectPending`, which tracks a *manual* reconnect the user asked
for via `useReconnectRelay`, and is false during the background loop.
`ConnectionState` already distinguishes `reconnecting` from
`disconnected`/`stalled`; the card just never saw it.

Thread that through as `isAutoReconnecting` and use it for the
no-user-action-needed case only:

  reconnecting            -> "Reconnecting" + spinner, role=status
  disconnected / stalled  -> unchanged alarm copy, role=alert

The action button stays enabled while auto-reconnecting so anyone who
does not want to wait out the backoff can still force an attempt.

`stalled` deliberately keeps the alarm copy: the socket is open but no
frames are arriving, so the client is not making progress on its own.

Also point `relay-connectivity.spec.ts`'s local `settle()` at the shared
`waitForAnimations` helper. The card now renders a looping spinner in
this state, and a looping animation's `.finished` never resolves, so the
old unbounded `Promise.allSettled(getAnimations())` hung until Playwright
aborted the evaluate. The shared helper races the wait against a ceiling
for exactly this reason.

Verified: desktop `pnpm check`, `pnpm test` (3923 pass), and the
`relay-connectivity` + `sidebar-relay-card` smoke specs (6 pass).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: James Risbey <james@sunnn.co>
@sunnnsolar

Copy link
Copy Markdown
Author

Follow-up on verification: I ran the full smoke project locally.

813 passed, 8 failed, 1 skipped. None of the failures are from this change — I confirmed by checking out main at a5dbdf5e (this PR's base) in the same worktree and re-running the same specs:

failing specs
main (baseline) 8
this branch 7
on this branch but not on main 0

The failures are the same pre-existing set on both sides — composer-selection-formatting (collapsed-caret block formatting), messaging (own-avatar), video-attachment (poster frames), and relay-reconnect ("Relay state seam is not installed"). main actually failed one more than this branch (the Quote starts at a collapsed caret variant), which fits these being load-sensitive rather than deterministic.

Worth flagging separately: relay-reconnect.spec.ts:135 fails under full-suite parallel load but passes 8/8 when its spec is run alone, so that one looks like a seam-install race in the harness rather than a product bug.

The specs this PR actually touches — relay-connectivity and sidebar-relay-card — pass 6/6.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant