Add connection status indicator in UI #260
Conversation
WalkthroughRefactors connection status handling from an imperative getter to a context state. Adds ConnectionState type, updates APIProvider to expose connectionStatus, moves document.title updates into ThemeProvider, and propagates connection state/title via ThemeContext. App uses useAPI and ThemeProvider setters. ConnectionStatus component simplified to read connectionStatus. Entry point wraps App with APIProvider. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant App
participant ThemeProvider
participant APIProvider
participant ConnectionStatusIcon
Browser->>APIProvider: Initialize (EventSource setup)
APIProvider-->>APIProvider: connectionStatus = "connecting"
APIProvider-->>APIProvider: onopen -> connectionStatus = "connected"
APIProvider-->>APIProvider: onerror -> connectionStatus = "disconnected" (retry/backoff)
Browser->>ThemeProvider: Initialize
ThemeProvider-->>Browser: document.title = [emoji(connectionState)] + appTitle
Browser->>App: Render
App->>APIProvider: useAPI() -> connectionStatus
App->>ThemeProvider: setConnectionState(connectionStatus)
App->>ThemeProvider: get/set appTitle (on edit)
ThemeProvider-->>Browser: Update document.title on state change
App->>ConnectionStatusIcon: Render
ConnectionStatusIcon->>APIProvider: useAPI() -> connectionStatus
ConnectionStatusIcon-->>App: Show status dot/title
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ui/src/contexts/APIProvider.tsx (2)
203-216: Context consumers won’t re-render on connectionStatus changes (missing dependency).connectionStatus is included in the value but omitted from the useMemo dependency list, freezing the context value for subscribers.
Apply this diff:
const value = useMemo( () => ({ models, listModels, unloadAllModels, loadModel, enableAPIEvents, proxyLogs, upstreamLogs, metrics, connectionStatus, }), - [models, listModels, unloadAllModels, loadModel, enableAPIEvents, proxyLogs, upstreamLogs, metrics] + [models, listModels, unloadAllModels, loadModel, enableAPIEvents, proxyLogs, upstreamLogs, metrics, connectionStatus] );
69-76: Guard reconnection after disable/unmount and clear scheduled retries.As written, onerror schedules reconnects that can outlive enableAPIEvents(false) or unmount, causing stray EventSource instances and network churn.
Apply this diff to track “enabled” state and clear pending timeouts:
@@ export function APIProvider({ children, autoStartAPIEvents = true }: APIProviderProps) { @@ - const apiEventSource = useRef<EventSource | null>(null); + const apiEventSource = useRef<EventSource | null>(null); + const eventsEnabledRef = useRef(false); + const reconnectTimeoutRef = useRef<number | null>(null); @@ - const enableAPIEvents = useCallback((enabled: boolean) => { + const enableAPIEvents = useCallback((enabled: boolean) => { + eventsEnabledRef.current = enabled; if (!enabled) { apiEventSource.current?.close(); apiEventSource.current = null; setMetrics([]); + // clear any scheduled reconnect + if (reconnectTimeoutRef.current !== null) { + clearTimeout(reconnectTimeoutRef.current); + reconnectTimeoutRef.current = null; + } + // reflect state as disconnected when explicitly disabled + // (optional: remove if you prefer to keep last-known status) + // setConnectionStatus("disconnected"); return; } @@ - const connect = () => { + const connect = () => { + if (!eventsEnabledRef.current) return; apiEventSource.current = null; const eventSource = new EventSource("/api/events"); - setConnectionState("connecting"); + setConnectionStatus("connecting"); @@ eventSource.onopen = () => { @@ - apiEventSource.current = eventSource; - retryCount = 0; - setConnectionState("connected"); + apiEventSource.current = eventSource; + retryCount = 0; + setConnectionStatus("connected"); }; @@ - eventSource.onerror = () => { + eventSource.onerror = () => { eventSource.close(); retryCount++; const delay = Math.min(initialDelay * Math.pow(2, retryCount - 1), 5000); - setConnectionState("disconnected"); - setTimeout(connect, delay); + setConnectionStatus("disconnected"); + if (eventsEnabledRef.current) { + reconnectTimeoutRef.current = window.setTimeout(connect, delay); + } }; };Notes:
- If you keep the optional setConnectionStatus on disable, ensure the setter rename (below) is applied.
- Using window.setTimeout keeps TS int return type in the browser; adjust if your tsconfig targets Node types.
Also applies to: 80-86, 139-145 </blockquote></details> </blockquote></details>🧹 Nitpick comments (11)
ui/src/lib/types.ts (1)
1-1: Consider exporting a runtime list of valid states alongside the type.This helps with validation, dropdowns, and guards without duplicating string literals across the app.
Apply this diff to provide both the type and a runtime constant:
-export type ConnectionState = "connected" | "connecting" | "disconnected"; +export const CONNECTION_STATES = ["connected", "connecting", "disconnected"] as const; +export type ConnectionState = typeof CONNECTION_STATES[number];ui/src/components/ConnectionStatus.tsx (1)
20-22: Add basic a11y semantics for screen readers.Expose the status as a live region and label it; keep the title for hover.
Apply this diff:
- <div className="flex items-center" title={`event stream: ${connectionStatus}`}> - <span className={`inline-block w-3 h-3 rounded-full ${eventStatusColor} mr-2`}></span> + <div + className="flex items-center" + title={`API connection: ${connectionStatus}`} + role="status" + aria-live="polite" + aria-label={`API connection ${connectionStatus}`} + > + <span className={`inline-block w-3 h-3 rounded-full ${eventStatusColor} mr-2`}></span> + <span className="sr-only">{`API connection ${connectionStatus}`}</span>ui/src/contexts/ThemeProvider.tsx (2)
6-16: Broaden setter typings to standard React setState signatures.Using Dispatch<SetStateAction<...>> allows both direct values and functional updates, matching the actual setters returned by useState/usePersistentState.
Apply this diff:
-import { createContext, useContext, useEffect, type ReactNode, useMemo, useState } from "react"; +import { + createContext, + useContext, + useEffect, + type ReactNode, + useMemo, + useState, + type Dispatch, + type SetStateAction, +} from "react"; @@ type ThemeContextType = { @@ - appTitle: string; - setAppTitle: (title: string) => void; - setConnectionState: (state: ConnectionState) => void; + appTitle: string; + setAppTitle: Dispatch<SetStateAction<string>>; + setConnectionState: Dispatch<SetStateAction<ConnectionState>>;
31-34: Minor readability: avoid nested ternary for the icon.Not critical, but a small map reads clearer and scales if more states are added.
For example:
const iconByState: Record<ConnectionState, string> = { connecting: "🟡", connected: "🟢", disconnected: "🔴", }; document.title = `${iconByState[connectionState]} ${appTitle}`;ui/src/contexts/APIProvider.tsx (2)
57-58: Rename setter to match its state for clarity.Minor naming mismatch (connectionStatus vs setConnectionState) can be confusing during maintenance.
Apply this diff:
- const [connectionStatus, setConnectionState] = useState<ConnectionState>("disconnected"); + const [connectionStatus, setConnectionStatus] = useState<ConnectionState>("disconnected"); @@ - setConnectionState("connecting"); + setConnectionStatus("connecting"); @@ - setConnectionState("connected"); + setConnectionStatus("connected"); @@ - setConnectionState("disconnected"); + setConnectionStatus("disconnected");Also applies to: 83-84, 92-94, 143-144
69-76: Optional: include appendLog in enableAPIEvents dependencies for completeness.appendLog is stable here, but adding it clarifies intent if its implementation ever changes.
- const enableAPIEvents = useCallback((enabled: boolean) => { + const enableAPIEvents = useCallback((enabled: boolean) => { // ... - }, []); + }, [appendLog]);ui/src/App.tsx (5)
8-8: Align file/component naming for clarity.
You import the default export asConnectionStatusIconfrom./components/ConnectionStatus. Consider renaming the file toConnectionStatusIcon.tsxand importing from./components/ConnectionStatusIconto avoid confusion between filename and exported component.
15-15: Factor out title sanitization and make it reusable.
The inline sanitization works but is easy to duplicate or drift. Consider a small helper and constants for the default and max length. Also collapsing runs of whitespace improves UX when users paste titles.Apply within this line:
- setAppTitle(newTitle.replace(/\n/g, "").trim().substring(0, 64) || "llama-swap"); + setAppTitle(sanitizeTitle(newTitle));And add these helpers (outside this range, e.g., near imports or before
App):const DEFAULT_APP_TITLE = "llama-swap"; const MAX_TITLE_LENGTH = 64; function sanitizeTitle(raw: string): string { // collapse whitespace, strip newlines, trim, and cap length const s = raw.replace(/\s+/g, " ").replace(/\n/g, "").trim().slice(0, MAX_TITLE_LENGTH); return s || DEFAULT_APP_TITLE; }
22-26: Tighten effect deps and fix comment wording.
- Comment nit: “connections” → “connection”.
- Include
setConnectionStatein deps for future-proofing. Even if stable, this avoids potential lint noise and signals intent.Apply:
- // Synchronize the window.title connections state with the actual connection state + // Synchronize ThemeProvider's connection state with the API connection status (drives document.title) useEffect(() => { setConnectionState(connectionStatus); - }, [connectionStatus]); + }, [connectionStatus, setConnectionState]);
32-48: contentEditable: avoid persisting the placeholder and add basic a11y.
- Passing "(set title)" to
handleTitleChangewill set that literal as the title. Use empty string to fall back to the default ("llama-swap").- Add
role="textbox"and anaria-labelfor screen readers.Apply:
- <h1 + <h1 contentEditable suppressContentEditableWarning + role="textbox" + aria-label="Edit application title" className="flex items-center p-0 outline-none hover:bg-gray-100 dark:hover:bg-gray-700 rounded px-1" - onBlur={(e) => handleTitleChange(e.currentTarget.textContent || "(set title)")} + onBlur={(e) => handleTitleChange(e.currentTarget.textContent || "")} onKeyDown={(e) => { if (e.key === "Enter") { e.preventDefault(); - handleTitleChange(e.currentTarget.textContent || "(set title)"); + handleTitleChange(e.currentTarget.textContent || ""); e.currentTarget.blur(); } }} > {appTitle} </h1>Optional: handle Escape to cancel edits and revert visible content (not committing changes).
onKeyDown={(e) => { if (e.key === "Enter") { /* as above */ } if (e.key === "Escape") { e.preventDefault(); e.currentTarget.textContent = appTitle; e.currentTarget.blur(); } }}
60-62: Add accessible labels to the theme toggle.
Provide an aria-label/title and explicit type to avoid accidental form submissions if this gets moved inside a form later.Apply:
- <button className="" onClick={toggleTheme}> + <button + className="" + type="button" + onClick={toggleTheme} + aria-label={isDarkMode ? "Switch to light mode" : "Switch to dark mode"} + title={isDarkMode ? "Switch to light mode" : "Switch to dark mode"} + > {isDarkMode ? <RiMoonFill /> : <RiSunFill />} </button>📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
ui/src/App.tsx(1 hunks)ui/src/components/ConnectionStatus.tsx(2 hunks)ui/src/contexts/APIProvider.tsx(6 hunks)ui/src/contexts/ThemeProvider.tsx(3 hunks)ui/src/lib/types.ts(1 hunks)ui/src/main.tsx(1 hunks)🧰 Additional context used
🧬 Code Graph Analysis (5)
ui/src/main.tsx (2)
ui/src/contexts/ThemeProvider.tsx (1)
ThemeProvider(24-89)ui/src/contexts/APIProvider.tsx (1)
APIProvider(53-219)ui/src/contexts/ThemeProvider.tsx (2)
ui/src/lib/types.ts (1)
ConnectionState(1-1)ui/src/hooks/usePersistentState.ts (1)
usePersistentState(3-39)ui/src/contexts/APIProvider.tsx (1)
ui/src/lib/types.ts (1)
ConnectionState(1-1)ui/src/components/ConnectionStatus.tsx (1)
ui/src/contexts/APIProvider.tsx (1)
useAPI(221-227)ui/src/App.tsx (2)
ui/src/contexts/ThemeProvider.tsx (1)
useTheme(91-97)ui/src/contexts/APIProvider.tsx (1)
useAPI(221-227)🔇 Additional comments (7)
ui/src/main.tsx (2)
10-14: Provider wiring LGTM.Wrapping App with APIProvider inside ThemeProvider is consistent with the new flow where App bridges API connectionStatus into Theme via setConnectionState.
1-16: No legacy getConnectionStatus or eventStreamStatus usage detectedI’ve searched all
.tsand.tsxfiles for bothgetConnectionStatus(andeventStreamStatusand found no remaining references. The old imperative getter and its polling artifacts appear fully removed—no further action needed here.ui/src/App.tsx (5)
12-12: LGTM: Centralizing title and connection state in ThemeProvider.
Good move exposingappTitle,setAppTitle, andsetConnectionStateviauseTheme(). This simplifies App and keeps document.title concerns in one place.
20-21: LGTM: Reading connectionStatus from API context.
Minimal exposure of API state at the edge of App is clean and aligns with the new context-driven design.
63-63: LGTM: Connection status indicator placement.
Rendering the status icon in the top-right cluster is intuitive and keeps state concerns encapsulated in the component.
67-74: LGTM: Routes unaffected by the refactor.
Page wiring remains intact; the refactor doesn’t alter navigation.
4-4: App is correctly wrapped with APIProvider and ThemeProviderConfirmed that in
ui/src/main.tsx,<App />is nested inside<APIProvider>within<ThemeProvider>. No changes needed.
Show connection status in the title. Refactor connections status checking to be event driven instead of polling.
Summary by CodeRabbit
New Features
Enhancements
Refactor