mirror of
https://github.com/NeuralNomadsAI/CodeNomad.git
synced 2026-08-11 01:13:39 +00:00
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.
This commit is contained in:
parent
bbe3ebc19e
commit
7d3da50124
8 changed files with 116 additions and 44 deletions
|
|
@ -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.
|
||||
|
|
|
|||
25
packages/ui/src/lib/connection-status.test.ts
Normal file
25
packages/ui/src/lib/connection-status.test.ts
Normal file
|
|
@ -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")
|
||||
})
|
||||
})
|
||||
19
packages/ui/src/lib/connection-status.ts
Normal file
19
packages/ui/src/lib/connection-status.ts
Normal file
|
|
@ -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
|
||||
}
|
||||
|
|
@ -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<WorkspaceEventConnection> {
|
||||
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()
|
||||
|
|
|
|||
|
|
@ -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")
|
||||
})
|
||||
})
|
||||
|
|
|
|||
|
|
@ -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<WorkspaceEventTransp
|
|||
}
|
||||
}
|
||||
|
||||
export function mapDesktopEventTransportStatus(
|
||||
state: DesktopEventTransportState,
|
||||
): WorkspaceEventTransportStatus {
|
||||
if (state === "connected") return "connected"
|
||||
if (state === "connecting") return "connecting"
|
||||
return "disconnected"
|
||||
}
|
||||
|
||||
export async function connectTauriWorkspaceEvents(
|
||||
callbacks: WorkspaceEventTransportCallbacks,
|
||||
options: DesktopEventTransportStartOptions,
|
||||
|
|
@ -59,6 +72,8 @@ export async function connectTauriWorkspaceEvents(
|
|||
const handleStatusPayload = (payload: DesktopEventTransportStatusPayload) => {
|
||||
if (!payload || !matchesGeneration(payload.generation)) return
|
||||
|
||||
callbacks.onStatus?.(mapDesktopEventTransportStatus(payload.state))
|
||||
|
||||
if (payload.state === "connected" && !opened) {
|
||||
opened = true
|
||||
callbacks.onOpen?.()
|
||||
|
|
|
|||
|
|
@ -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<string, unknown>) {
|
|||
class ServerEvents {
|
||||
private handlers = new Map<WorkspaceEventType | "*", Set<(event: WorkspaceEventPayload) => 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 {
|
||||
|
|
|
|||
|
|
@ -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<string, unknown> }
|
||||
|
||||
type ConnectionStatus = InstanceStreamStatus
|
||||
|
||||
const [connectionStatus, setConnectionStatus] = createSignal<Map<string, ConnectionStatus>>(new Map())
|
||||
const [transportStatus, setTransportStatus] = createSignal<WorkspaceEventTransportStatus>("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<void>
|
||||
|
||||
getStatus(instanceId: string): ConnectionStatus | null {
|
||||
return connectionStatus().get(instanceId) ?? null
|
||||
return deriveDisplayConnectionStatus(connectionStatus().get(instanceId) ?? null, transportStatus())
|
||||
}
|
||||
|
||||
getStatuses() {
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue