Skip to content

fix(ui): SSE connection status indicators for mobile network drops#549

Open
JDis03 wants to merge 1 commit into
NeuralNomadsAI:devfrom
JDis03:fix/sse-connection-status-clean
Open

fix(ui): SSE connection status indicators for mobile network drops#549
JDis03 wants to merge 1 commit into
NeuralNomadsAI:devfrom
JDis03:fix/sse-connection-status-clean

Conversation

@JDis03

@JDis03 JDis03 commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

Problem

Mobile users experience SSE disconnections (wifi drops, Cloudflare idle timeouts) but get no visual feedback. The per-instance status dots remain green even when the SSE transport is down, because instance status updates travel over the broken SSE pipe.

Solution

This PR implements immediate connection status feedback by propagating SSE transport state to all instance status indicators:

Changes

  1. server-events.ts: Add onDisconnect() handler

    • Mirrors existing onOpen() pattern
    • Fire disconnectHandlers in scheduleReconnect() when connection drops
  2. sse-manager.ts: Register SSE lifecycle handlers

    • onDisconnect: set ALL instances to connecting (amber dot)
    • onOpen: clear connecting status (green dot restored by subsequent events)
    • Log transitions for debug visibility
  3. AGENTS.md: Add PR Review Principles

    • Check regressions first, look for better implementations
    • Be the PR gatekeeper, ruthless code quality
    • Test before responding, UI/server version parity

Verification

  • ✅ TypeScript compilation clean (pre-existing SDK errors unrelated)
  • ✅ Vite build successful
  • ✅ Mobile testing: SSE disconnect → immediate amber dots
  • ✅ Reconnect → green dots restored

Edge Cases Handled

  • Transient drop: amber → green on reconnect (no false disconnect modal)
  • Instance dies during outage: amber → green → disconnected event → red
  • 0 instances: Map empty, loop is no-op
  • Rapid reconnect cycles: idempotent (setting connecting on connecting is no-op)

Related

Implements immediate visual feedback when SSE transport disconnects (wifi drops, Cloudflare idle timeouts, mobile network switches).

## Problem
Per-instance status dots remained frozen at 'connected' (green) when SSE transport dropped because:
- Instance status updates travel over SSE (impossible when SSE is down)
- EventSource.onerror doesn't fire immediately on mobile wifi drops
- No user-facing indication of workspace-level SSE state

## Solution
1. server-events.ts: Add onDisconnect() handler (mirrors existing onOpen)
   - Fire disconnectHandlers in scheduleReconnect() when connection drops

2. sse-manager.ts: Register SSE lifecycle handlers
   - onDisconnect: set ALL instances to 'connecting' (amber dot)
   - onOpen: clear 'connecting' status (green dot restored by events)
   - Log transitions for debug visibility

3. AGENTS.md: Add PR Review Principles section
   - Check regressions, look for better implementations
   - Be the PR gatekeeper, ruthless code quality
   - Test before responding, UI/server version parity

## Verification
- TypeScript compilation clean (pre-existing SDK errors unrelated)
- Vite build successful
- Mobile testing: SSE disconnect → immediate amber dots
- Reconnect → green dots restored

## Edge Cases Handled
- Transient drop: amber → green on reconnect (no false modal)
- Instance dies during outage: amber → green → 'disconnected' event → red
- 0 instances: Map empty, loop is no-op
- Rapid reconnect cycles: idempotent (setting 'connecting' on 'connecting' is no-op)

Complements PR NeuralNomadsAI#519 (pong retry with timeout) - merged upstream.
@github-actions

Copy link
Copy Markdown

PR builds are available as GitHub Actions artifacts:

https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/27416160552

Artifacts expire in 7 days.
Artifacts:

  • pr-549-bbe3ebc19e5c4d2021ba0dd2dfb3c86ef4b54af0-electron-macos
  • pr-549-bbe3ebc19e5c4d2021ba0dd2dfb3c86ef4b54af0-tauri-macos
  • pr-549-bbe3ebc19e5c4d2021ba0dd2dfb3c86ef4b54af0-tauri-linux
  • pr-549-bbe3ebc19e5c4d2021ba0dd2dfb3c86ef4b54af0-tauri-windows
  • pr-549-bbe3ebc19e5c4d2021ba0dd2dfb3c86ef4b54af0-tauri-macos-arm64
  • pr-549-bbe3ebc19e5c4d2021ba0dd2dfb3c86ef4b54af0-electron-windows
  • pr-549-bbe3ebc19e5c4d2021ba0dd2dfb3c86ef4b54af0-electron-linux

@pascalandr

Copy link
Copy Markdown
Contributor

@CodeNomadBot

I want you to perform the gatekeeper checks for latest PR state

check for regressions
check for better possible implementations
Be the PR gatekeeper
Be ruthless about code quality

@codenomadbot

codenomadbot Bot commented Jun 13, 2026

Copy link
Copy Markdown
Contributor

Gatekeeper review for latest PR state (bbe3ebc1).

Understood request: perform a strict PR review focused on regressions, implementation quality, and better possible approaches. I treated this as a review-only request and did not change code.

Findings

  1. Blocking: reconnect can leave indicators in the wrong state because the patch overwrites per-instance status and then deletes it on open.

packages/ui/src/lib/sse-manager.ts:124-145 mutates the authoritative connectionStatus map by setting every known instance to connecting, then removes all connecting entries when the transport reopens. The UI does not interpret a missing entry as restored green: compact mobile falls through to disconnected styling at packages/ui/src/components/instance/instance-shell2.tsx:326-331, while the expanded status area only renders explicit connected / connecting / error|disconnected branches at packages/ui/src/components/instance/instance-shell2.tsx:1129-1146.

This is a real regression because packages/server/src/workspaces/instance-events.ts:203-205 only publishes instance status when the instance stream status changes; it does not replay current status to a UI client that merely reconnected its workspace SSE pipe. If an instance was connected before a temporary UI transport drop and no subsequent instance event occurs, the reconnect path can move from amber to unknown/red/no expanded status instead of restoring green. It also loses pre-existing error / disconnected states by temporarily painting them amber.

Better implementation: keep transport reachability as separate overlay state instead of overwriting instance stream status. For display, derive connecting when workspace SSE is down; when it reopens, reveal the previous per-instance status unchanged. If you do want to mutate the map, snapshot previous statuses before the outage and restore them on open, but a derived overlay is cleaner and avoids stale restoration bugs.

  1. Blocking for desktop/Tauri parity: transient native transport disconnects do not trigger the new disconnect lifecycle.

packages/ui/src/lib/server-events.ts:109 fires disconnectHandlers only from scheduleReconnect(). Browser EventSource errors go through that path, but the Rust-native desktop transport handles non-terminal disconnected / error states internally in packages/ui/src/lib/native/desktop-events.ts:74-83 and only calls callbacks.onError when payload.terminal or stopped is true at packages/ui/src/lib/native/desktop-events.ts:90-97. That means native desktop reconnect cycles can continue without ever invoking serverEvents.onDisconnect(), so this PR’s UI feedback path is bypassed for that transport.

Better implementation: extend the transport callback contract with transport status events, or map native disconnected / error / connecting / connected status payloads into the same serverEvents lifecycle callbacks used by browser EventSource. The abstraction should expose transport state consistently regardless of implementation.

  1. Missing regression coverage for the new lifecycle behavior.

I found no tests around serverEvents.onDisconnect, sseManager status overlay behavior, or native transport status propagation. Existing UI tests include packages/ui/src/lib/native/desktop-events.test.ts, but it only covers terminal error notification. Given this change directly manipulates user-visible connection status under outage/reconnect races, add targeted tests for at least these cases: connected instance -> transport disconnect -> display connecting -> transport reopen with no instance events -> display returns to previous connected state; pre-existing disconnected/error instance is not masked permanently; native desktop:event-stream-status disconnected/error/connected transitions notify the same lifecycle path.

Verification Attempted
npm run build:ui failed in this checkout before compiling the PR because vite is not installed/resolved.

npm run typecheck --workspace @codenomad/ui also failed before meaningful PR validation because dependencies/types such as solid-js, @opencode-ai/sdk, and Node test types are missing/resolution-broken in the local environment.

I did not treat those command failures as PR-introduced by themselves, but they mean I could not independently confirm the PR’s claimed clean build/typecheck from this webhook environment.

Recommendation
Do not merge as-is. The intended UX improvement is valid, but the current implementation mixes transport state into per-instance state and misses the native transport path. I would request changes to model workspace SSE reachability separately, derive the amber indicator from that overlay, wire native transport status into the same lifecycle, and add regression tests for reconnect-without-instance-events before approval.

--
Yours,
CodeNomadBot

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.

2 participants