From bbe3ebc19e5c4d2021ba0dd2dfb3c86ef4b54af0 Mon Sep 17 00:00:00 2001 From: JD Date: Fri, 12 Jun 2026 12:38:12 +0000 Subject: [PATCH 1/2] fix(ui): SSE connection status indicators for mobile network drops MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 #519 (pong retry with timeout) - merged upstream. --- AGENTS.md | 8 ++++++++ packages/ui/src/lib/server-events.ts | 8 ++++++++ packages/ui/src/lib/sse-manager.ts | 26 ++++++++++++++++++++++++++ 3 files changed, 42 insertions(+) diff --git a/AGENTS.md b/AGENTS.md index 3a8b08cc9..1cd5c66e4 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -15,6 +15,14 @@ - Prefer composable primitives (signals, hooks, utilities) over deep inheritance or implicit global state. - When adding platform integrations (SSE, IPC, SDK), isolate them in thin adapters that surface typed events/actions. +## PR Review Principles +- **Check for regressions first.** Before approving any change, verify the existing behavior still works — run the test suite, test manually on both mobile and desktop, and confirm no unintended side effects in related subsystems. +- **Look for better possible implementations.** Don't settle for the first working approach. Ask: is there a simpler way? Does the codebase already have a pattern for this? Would a different abstraction reduce future maintenance cost? +- **Be the PR gatekeeper.** Every line merged becomes technical debt someone else will read. If it's unclear, fragile, or lacks tests, push back. The reviewer's job is to protect the codebase, not to be nice. +- **Be ruthless about code quality.** Surface-level "LGTM" is negligence. Inspect: naming, error handling, edge cases, type safety, logging (is it useful or just noise?), performance (any unnecessary allocations or re-renders?), and whether the change respects existing architectural boundaries. +- **Test before responding to review comments.** Never reply "works for me" or "this fixes it" without deploying the exact commit and verifying the behavior. Untested responses waste reviewer time and erode trust. +- **UI and server must be built from the same version.** Version mismatches between UI and server cause subtle bugs (e.g., sessions disappearing). Always build both from the same commit before testing. + ## Multi-Language Support (i18n) The UI uses a small custom i18n layer (no ICU/messageformat). When building features, never hardcode user-visible strings. diff --git a/packages/ui/src/lib/server-events.ts b/packages/ui/src/lib/server-events.ts index 4145367dd..e8704193b 100644 --- a/packages/ui/src/lib/server-events.ts +++ b/packages/ui/src/lib/server-events.ts @@ -21,6 +21,7 @@ function logSse(message: string, context?: Record) { class ServerEvents { private handlers = new Map void>>() private openHandlers = new Set<() => void>() + private disconnectHandlers = new Set<() => void>() private connection: WorkspaceEventConnection | null = null private connectGeneration = 0 private retryDelay = RETRY_BASE_DELAY @@ -105,6 +106,8 @@ class ServerEvents { this.connection = null } + this.disconnectHandlers.forEach((handler) => handler()) + logSse("Events stream disconnected, scheduling reconnect", { delayMs: this.retryDelay }) this.retryTimer = setTimeout(() => { this.retryTimer = null @@ -154,6 +157,11 @@ class ServerEvents { return () => this.openHandlers.delete(handler) } + onDisconnect(handler: () => void): () => void { + this.disconnectHandlers.add(handler) + return () => this.disconnectHandlers.delete(handler) + } + restart(reason = "manual restart"): void { this.retryDelay = RETRY_BASE_DELAY this.clearReconnectTimer() diff --git a/packages/ui/src/lib/sse-manager.ts b/packages/ui/src/lib/sse-manager.ts index 47b9ea802..f0f113af1 100644 --- a/packages/ui/src/lib/sse-manager.ts +++ b/packages/ui/src/lib/sse-manager.ts @@ -101,6 +101,8 @@ const [connectionStatus, setConnectionStatus] = createSignal { const payload = event as InstanceStatusPayload this.updateConnectionStatus(payload.instanceId, payload.status) @@ -118,6 +120,30 @@ class SSEManager { this.updateConnectionStatus(payload.instanceId, "connected") this.handleEvent(payload.instanceId, payload.event as SSEEvent) }) + + serverEvents.onDisconnect(() => { + log.info("SSE transport disconnected → setting all instances to 'connecting'") + setConnectionStatus((prev) => { + const next = new Map(prev) + for (const [id] of next) { + next.set(id, "connecting") + } + return next + }) + }) + + serverEvents.onOpen(() => { + log.info("SSE transport reconnected → clearing 'connecting' status") + setConnectionStatus((prev) => { + const next = new Map(prev) + for (const [id, status] of next) { + if (status === "connecting") { + next.delete(id) + } + } + return next + }) + }) } seedStatus(instanceId: string, status: ConnectionStatus) { From 7d3da501240f79044f55fb2200b7e807c840d2b2 Mon Sep 17 00:00:00 2001 From: Shantur Rathore Date: Mon, 15 Jun 2026 11:44:35 +0100 Subject: [PATCH 2/2] fix(ui): preserve instance status during SSE outages Track workspace event transport reachability separately from per-instance stream status so a temporary SSE outage can show an amber connecting overlay without overwriting connected, disconnected, error, or genuinely connecting instance state. Expose shared transport status from both browser EventSource and the native Tauri event transport, including transient native disconnected and error states, so desktop and browser paths drive the same UI behavior. Remove the unrelated AGENTS.md review-guidance change from this PR and add focused node:test coverage for display-status derivation and native transport status mapping. Validated with UI typecheck, targeted tests, and npm run build:ui. --- AGENTS.md | 8 ----- packages/ui/src/lib/connection-status.test.ts | 25 ++++++++++++++ packages/ui/src/lib/connection-status.ts | 19 +++++++++++ packages/ui/src/lib/event-transport.ts | 14 ++++++-- .../ui/src/lib/native/desktop-events.test.ts | 18 +++++++++- packages/ui/src/lib/native/desktop-events.ts | 17 +++++++++- packages/ui/src/lib/server-events.ts | 26 +++++++++++---- packages/ui/src/lib/sse-manager.ts | 33 ++++--------------- 8 files changed, 116 insertions(+), 44 deletions(-) create mode 100644 packages/ui/src/lib/connection-status.test.ts create mode 100644 packages/ui/src/lib/connection-status.ts diff --git a/AGENTS.md b/AGENTS.md index 1cd5c66e4..3a8b08cc9 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -15,14 +15,6 @@ - Prefer composable primitives (signals, hooks, utilities) over deep inheritance or implicit global state. - When adding platform integrations (SSE, IPC, SDK), isolate them in thin adapters that surface typed events/actions. -## PR Review Principles -- **Check for regressions first.** Before approving any change, verify the existing behavior still works — run the test suite, test manually on both mobile and desktop, and confirm no unintended side effects in related subsystems. -- **Look for better possible implementations.** Don't settle for the first working approach. Ask: is there a simpler way? Does the codebase already have a pattern for this? Would a different abstraction reduce future maintenance cost? -- **Be the PR gatekeeper.** Every line merged becomes technical debt someone else will read. If it's unclear, fragile, or lacks tests, push back. The reviewer's job is to protect the codebase, not to be nice. -- **Be ruthless about code quality.** Surface-level "LGTM" is negligence. Inspect: naming, error handling, edge cases, type safety, logging (is it useful or just noise?), performance (any unnecessary allocations or re-renders?), and whether the change respects existing architectural boundaries. -- **Test before responding to review comments.** Never reply "works for me" or "this fixes it" without deploying the exact commit and verifying the behavior. Untested responses waste reviewer time and erode trust. -- **UI and server must be built from the same version.** Version mismatches between UI and server cause subtle bugs (e.g., sessions disappearing). Always build both from the same commit before testing. - ## Multi-Language Support (i18n) The UI uses a small custom i18n layer (no ICU/messageformat). When building features, never hardcode user-visible strings. diff --git a/packages/ui/src/lib/connection-status.test.ts b/packages/ui/src/lib/connection-status.test.ts new file mode 100644 index 000000000..b61da207a --- /dev/null +++ b/packages/ui/src/lib/connection-status.test.ts @@ -0,0 +1,25 @@ +import assert from "node:assert/strict" +import { describe, it } from "node:test" +import { deriveDisplayConnectionStatus } from "./connection-status.ts" + +describe("deriveDisplayConnectionStatus", () => { + it("overlays connecting while transport is down for connected instances", () => { + assert.equal(deriveDisplayConnectionStatus("connected", "disconnected"), "connecting") + }) + + it("restores previous connected status when transport reconnects", () => { + assert.equal(deriveDisplayConnectionStatus("connected", "connected"), "connected") + }) + + it("preserves disconnected instance status while transport is down", () => { + assert.equal(deriveDisplayConnectionStatus("disconnected", "disconnected"), "disconnected") + }) + + it("preserves error instance status while transport is down", () => { + assert.equal(deriveDisplayConnectionStatus("error", "disconnected"), "error") + }) + + it("does not clear legitimate instance connecting status after transport opens", () => { + assert.equal(deriveDisplayConnectionStatus("connecting", "connected"), "connecting") + }) +}) diff --git a/packages/ui/src/lib/connection-status.ts b/packages/ui/src/lib/connection-status.ts new file mode 100644 index 000000000..c162e550c --- /dev/null +++ b/packages/ui/src/lib/connection-status.ts @@ -0,0 +1,19 @@ +import type { InstanceStreamStatus } from "../../../server/src/api-types" +import type { WorkspaceEventTransportStatus } from "./event-transport" + +export type ConnectionStatus = InstanceStreamStatus + +export function deriveDisplayConnectionStatus( + instanceStatus: ConnectionStatus | null, + workspaceTransportStatus: WorkspaceEventTransportStatus, +): ConnectionStatus | null { + if (instanceStatus === "disconnected" || instanceStatus === "error") { + return instanceStatus + } + + if (workspaceTransportStatus !== "connected") { + return "connecting" + } + + return instanceStatus +} diff --git a/packages/ui/src/lib/event-transport.ts b/packages/ui/src/lib/event-transport.ts index 24b69591d..5de633c37 100644 --- a/packages/ui/src/lib/event-transport.ts +++ b/packages/ui/src/lib/event-transport.ts @@ -15,9 +15,12 @@ export interface WorkspaceEventTransportCallbacks { onBatch: (events: WorkspaceEventPayload[]) => void onError?: () => void onOpen?: () => void + onStatus?: (status: WorkspaceEventTransportStatus) => void onPing?: (payload: { ts?: number }) => void } +export type WorkspaceEventTransportStatus = "connecting" | "connected" | "disconnected" + export interface WorkspaceEventConnection { disconnect: () => void } @@ -25,10 +28,17 @@ export interface WorkspaceEventConnection { async function connectBrowserWorkspaceEvents( callbacks: WorkspaceEventTransportCallbacks, ): Promise { + const notifyDisconnected = () => { + callbacks.onStatus?.("disconnected") + callbacks.onError?.() + } const source = serverApi.connectEvents((event) => { callbacks.onBatch([event]) - }, callbacks.onError, callbacks.onPing) - source.onopen = () => callbacks.onOpen?.() + }, notifyDisconnected, callbacks.onPing) + source.onopen = () => { + callbacks.onStatus?.("connected") + callbacks.onOpen?.() + } return { disconnect() { source.close() diff --git a/packages/ui/src/lib/native/desktop-events.test.ts b/packages/ui/src/lib/native/desktop-events.test.ts index c414b04b4..75c324217 100644 --- a/packages/ui/src/lib/native/desktop-events.test.ts +++ b/packages/ui/src/lib/native/desktop-events.test.ts @@ -1,6 +1,6 @@ import assert from "node:assert/strict" import { describe, it } from "node:test" -import { createTerminalErrorNotifier } from "./desktop-events.ts" +import { createTerminalErrorNotifier, mapDesktopEventTransportStatus } from "./desktop-events.ts" describe("createTerminalErrorNotifier", () => { it("calls onError once for repeated terminal notifications", () => { @@ -17,3 +17,19 @@ describe("createTerminalErrorNotifier", () => { assert.equal(errors, 1) }) }) + +describe("mapDesktopEventTransportStatus", () => { + it("maps native connected state to shared connected state", () => { + assert.equal(mapDesktopEventTransportStatus("connected"), "connected") + }) + + it("maps native connecting state to shared connecting state", () => { + assert.equal(mapDesktopEventTransportStatus("connecting"), "connecting") + }) + + it("maps native transient failures to shared disconnected state", () => { + assert.equal(mapDesktopEventTransportStatus("disconnected"), "disconnected") + assert.equal(mapDesktopEventTransportStatus("error"), "disconnected") + assert.equal(mapDesktopEventTransportStatus("unauthorized"), "disconnected") + }) +}) diff --git a/packages/ui/src/lib/native/desktop-events.ts b/packages/ui/src/lib/native/desktop-events.ts index 01c9af6ea..bd14892b7 100644 --- a/packages/ui/src/lib/native/desktop-events.ts +++ b/packages/ui/src/lib/native/desktop-events.ts @@ -4,9 +4,14 @@ import type { WorkspaceEventPayload } from "../../../../server/src/api-types" import type { DesktopEventsStartResult, DesktopEventTransportStartOptions, + DesktopEventTransportState, DesktopEventTransportStatusPayload, } from "../event-transport-contract" -import type { WorkspaceEventConnection, WorkspaceEventTransportCallbacks } from "../event-transport" +import type { + WorkspaceEventConnection, + WorkspaceEventTransportCallbacks, + WorkspaceEventTransportStatus, +} from "../event-transport" import { getLogger } from "../logger" const log = getLogger("sse") @@ -27,6 +32,14 @@ export function createTerminalErrorNotifier(callbacks: Pick { if (!payload || !matchesGeneration(payload.generation)) return + callbacks.onStatus?.(mapDesktopEventTransportStatus(payload.state)) + if (payload.state === "connected" && !opened) { opened = true callbacks.onOpen?.() diff --git a/packages/ui/src/lib/server-events.ts b/packages/ui/src/lib/server-events.ts index e8704193b..bb747f1a0 100644 --- a/packages/ui/src/lib/server-events.ts +++ b/packages/ui/src/lib/server-events.ts @@ -2,7 +2,11 @@ import { batch as solidBatch } from "solid-js" import type { WorkspaceEventPayload, WorkspaceEventType } from "../../../server/src/api-types" import { serverApi } from "./api-client" import { getClientIdentity } from "./client-identity" -import { connectWorkspaceEvents, type WorkspaceEventConnection } from "./event-transport" +import { + connectWorkspaceEvents, + type WorkspaceEventConnection, + type WorkspaceEventTransportStatus, +} from "./event-transport" import { getLogger } from "./logger" import { retryWithBackoff, isRetryableError } from "./retry-utils" @@ -21,7 +25,7 @@ function logSse(message: string, context?: Record) { class ServerEvents { private handlers = new Map void>>() private openHandlers = new Set<() => void>() - private disconnectHandlers = new Set<() => void>() + private statusHandlers = new Set<(status: WorkspaceEventTransportStatus) => void>() private connection: WorkspaceEventConnection | null = null private connectGeneration = 0 private retryDelay = RETRY_BASE_DELAY @@ -51,6 +55,12 @@ class ServerEvents { } this.scheduleReconnect() }, + onStatus: (status) => { + if (generation !== this.connectGeneration) { + return + } + this.emitTransportStatus(status) + }, onOpen: () => { if (generation !== this.connectGeneration) { return @@ -106,7 +116,7 @@ class ServerEvents { this.connection = null } - this.disconnectHandlers.forEach((handler) => handler()) + this.emitTransportStatus("disconnected") logSse("Events stream disconnected, scheduling reconnect", { delayMs: this.retryDelay }) this.retryTimer = setTimeout(() => { @@ -143,6 +153,10 @@ class ServerEvents { }) } + private emitTransportStatus(status: WorkspaceEventTransportStatus) { + this.statusHandlers.forEach((handler) => handler(status)) + } + on(type: WorkspaceEventType | "*", handler: (event: WorkspaceEventPayload) => void): () => void { if (!this.handlers.has(type)) { this.handlers.set(type, new Set()) @@ -157,9 +171,9 @@ class ServerEvents { return () => this.openHandlers.delete(handler) } - onDisconnect(handler: () => void): () => void { - this.disconnectHandlers.add(handler) - return () => this.disconnectHandlers.delete(handler) + onTransportStatus(handler: (status: WorkspaceEventTransportStatus) => void): () => void { + this.statusHandlers.add(handler) + return () => this.statusHandlers.delete(handler) } restart(reason = "manual restart"): void { diff --git a/packages/ui/src/lib/sse-manager.ts b/packages/ui/src/lib/sse-manager.ts index f0f113af1..359f9d303 100644 --- a/packages/ui/src/lib/sse-manager.ts +++ b/packages/ui/src/lib/sse-manager.ts @@ -24,13 +24,14 @@ import type { } from "@opencode-ai/sdk/v2" import type { LegacyPermissionAskedEvent, LegacyPermissionRepliedEvent } from "../types/permission" import { serverEvents } from "./server-events" +import type { WorkspaceEventTransportStatus } from "./event-transport" import type { BackgroundProcess, InstanceStreamEvent, - InstanceStreamStatus, WorkspaceEventPayload, } from "../../../server/src/api-types" import { getLogger } from "./logger" +import { deriveDisplayConnectionStatus, type ConnectionStatus } from "./connection-status" const log = getLogger("sse") @@ -95,9 +96,8 @@ type SSEEvent = | ServerInstanceDisposedEvent | { type: string; properties?: Record } -type ConnectionStatus = InstanceStreamStatus - const [connectionStatus, setConnectionStatus] = createSignal>(new Map()) +const [transportStatus, setTransportStatus] = createSignal("connecting") class SSEManager { constructor() { @@ -121,28 +121,9 @@ class SSEManager { this.handleEvent(payload.instanceId, payload.event as SSEEvent) }) - serverEvents.onDisconnect(() => { - log.info("SSE transport disconnected → setting all instances to 'connecting'") - setConnectionStatus((prev) => { - const next = new Map(prev) - for (const [id] of next) { - next.set(id, "connecting") - } - return next - }) - }) - - serverEvents.onOpen(() => { - log.info("SSE transport reconnected → clearing 'connecting' status") - setConnectionStatus((prev) => { - const next = new Map(prev) - for (const [id, status] of next) { - if (status === "connecting") { - next.delete(id) - } - } - return next - }) + serverEvents.onTransportStatus((status) => { + log.info("SSE transport status changed", { status }) + setTransportStatus(status) }) } @@ -266,7 +247,7 @@ class SSEManager { onConnectionLost?: (instanceId: string, reason: string) => void | Promise getStatus(instanceId: string): ConnectionStatus | null { - return connectionStatus().get(instanceId) ?? null + return deriveDisplayConnectionStatus(connectionStatus().get(instanceId) ?? null, transportStatus()) } getStatuses() {