From 0836711fc96531781994994dc60daf8d04c9caa9 Mon Sep 17 00:00:00 2001 From: Ashish Huddar Date: Tue, 19 May 2026 12:44:09 +0530 Subject: [PATCH] Fix remote terminal proxy in dev mode --- docs/FRONTEND_ARCHITECTURE_CHANGE_PLAN.md | 213 +++++++ docs/FRONTEND_STATE_MANAGEMENT_PLAN.md | 739 ++++++++++++++++++++++ docs/session-store-proposal.md | 365 +++++++++++ docs/state-management-improvements.md | 162 +++++ 4 files changed, 1479 insertions(+) create mode 100644 docs/FRONTEND_ARCHITECTURE_CHANGE_PLAN.md create mode 100644 docs/FRONTEND_STATE_MANAGEMENT_PLAN.md create mode 100644 docs/session-store-proposal.md create mode 100644 docs/state-management-improvements.md diff --git a/docs/FRONTEND_ARCHITECTURE_CHANGE_PLAN.md b/docs/FRONTEND_ARCHITECTURE_CHANGE_PLAN.md new file mode 100644 index 000000000..d70bec9f7 --- /dev/null +++ b/docs/FRONTEND_ARCHITECTURE_CHANGE_PLAN.md @@ -0,0 +1,213 @@ +# Frontend Architecture Change Plan + +> Status: draft · Scope: `packages/web` · Last updated: 2026-05-17 + +## Purpose + +This document captures the frontend architecture changes we plan to make to keep the dashboard server-rendered, live-updating, and maintainable as the UI grows. + +The current architecture is directionally correct: Next.js App Router renders an SSR snapshot, the client hydrates into a live dashboard, and the mux WebSocket streams both session updates and terminal data. The changes below are about tightening boundaries, reducing render cascades, and making the implementation easier to extend without rewriting the app. + +## Goals + +- Keep the dashboard **SSR-first** for fast initial load and graceful failure states. +- Keep WebSocket updates **patch-based** and cheap. +- Use REST refreshes only as a **repair/fallback path** when membership changes or data is stale. +- Keep core/session/plugin logic on the server; expose only serialized dashboard DTOs to React. +- Reduce large client components by moving route data loading and state orchestration into focused modules. +- Preserve the existing mux terminal architecture. + +## Non-goals + +- No new frontend state-management dependency. +- No rewrite of the design system or visual language. +- No database or persistent client cache. +- No changes to core plugin interfaces unless a frontend feature explicitly requires it. + +## Current architecture snapshot + +```text +agent-orchestrator.yaml / global config + -> web server getServices() + -> sessionManager.listCached() + -> serialize Session -> DashboardSession + -> Next.js server route renders initial page + -> client Dashboard hydrates + -> MuxProvider connects to WebSocket + -> session patches update UI + -> REST /api/sessions repairs stale or changed membership +``` + +Important files: + +- `packages/web/src/app/layout.tsx` — app shell and providers. +- `packages/web/src/app/page.tsx` — dashboard server route. +- `packages/web/src/lib/dashboard-page-data.ts` — SSR dashboard data loader. +- `packages/web/src/lib/services.ts` — server-only service singleton. +- `packages/web/src/lib/serialize.ts` — core session to dashboard DTO mapping. +- `packages/web/src/components/Dashboard.tsx` — main dashboard client UI. +- `packages/web/src/hooks/useSessionEvents.ts` — live session state reducer. +- `packages/web/src/providers/MuxProvider.tsx` — browser WebSocket provider. +- `packages/web/server/mux-websocket.ts` — terminal/session WebSocket mux server. +- `packages/web/src/components/SessionDetail.tsx` — session detail UI. +- `packages/web/src/app/sessions/[id]/page.tsx` — current session detail route. + +## Planned changes + +### 1. Formalize the data boundary + +Create and consistently use one frontend DTO boundary: + +```text +core Session -> serialize.ts -> DashboardSession -> React components +``` + +Changes: + +- Keep all core-only imports out of client components where possible. +- Centralize dashboard-safe serialization in `src/lib/serialize.ts`. +- Treat `DashboardSession` as the only session shape consumed by UI components. +- Keep server-only service access behind `src/lib/services.ts` and API routes. + +Success criteria: + +- Client components do not need to understand core session internals. +- API route responses and SSR props use the same dashboard DTOs. + +### 2. Tighten live session state + +Improve `useSessionEvents` so live updates do not cause unnecessary render cascades. + +Changes: + +- Dedupe `reset` actions by content before allocating new session arrays. +- Preserve existing references when incoming data is unchanged. +- Dedupe `attentionLevels` in the same way. +- Keep WebSocket patches lightweight: `id`, `status`, `activity`, `attentionLevel`, `lastActivityAt`. +- Use `/api/sessions` only when membership changes or a stale refresh is needed. + +Success criteria: + +- A no-op WebSocket snapshot does not re-render the whole dashboard. +- A 15s fallback refresh with identical data preserves stable references. + +### 3. Split the dashboard into smaller render units + +`Dashboard.tsx` should remain the orchestration component, but list-heavy UI should move into memoized children. + +Changes: + +- Keep `Dashboard.tsx` responsible for page-level state and routing. +- Extract memoized subcomponents for project groups, session rows, and Kanban sections where needed. +- Use stable callbacks for list rows and sidebar actions. +- Avoid inline large JSX blocks for repeated rows. + +Success criteria: + +- Updating one session does not reconcile every row in the sidebar/Kanban. +- Rename inputs, popovers, and mobile menus keep local UI state during live updates. + +### 4. Keep mux WebSocket as the single live transport + +The mux WebSocket should continue to carry both terminal data and session patches. + +Changes: + +- Keep `MuxProvider` as the single browser WebSocket owner. +- Batch session snapshots arriving close together before updating React state. +- On reconnect, trigger one REST refresh to repair any missed membership changes. +- Keep terminal operations isolated behind `openTerminal`, `writeTerminal`, `resizeTerminal`, and `closeTerminal`. + +Success criteria: + +- Multiple rapid session transitions produce one visible UI update batch. +- Reconnects recover to an accurate session list without a manual refresh. +- Terminal behavior remains unchanged. + +### 5. Convert session detail to SSR-first shape + +The session detail route currently contains too much client-side fetch and lifecycle logic. We will reshape it to match the dashboard pattern. + +Target shape: + +```text +app/sessions/[id]/page.tsx server component + -> load initial session/sidebar data + -> render SessionPageClient + +SessionPageClient.tsx client component + -> useSessionEvents / mux updates + -> derive page state + -> render SessionDetail once +``` + +Changes: + +- Move initial route data loading to the server page. +- Add a focused `SessionPageClient.tsx` for live updates and interaction state. +- Reuse the existing live-session update path instead of custom parallel polling logic. +- Render `SessionDetail` through one primary branch instead of duplicate prop branches. + +Success criteria: + +- Session detail has one source of truth for live session data. +- Loading, missing, error, and ready states are explicit. +- The route is easier to test and reason about. + +### 6. Document frontend implementation patterns + +Add a frontend patterns document after the first implementation pass. + +Proposed file: + +- `docs/FRONTEND_PATTERNS.md` + +Topics: + +- SSR snapshot + client live patch pattern. +- DTO boundary rules. +- When to use mux patches vs REST refresh. +- Memoized list rendering guidelines. +- Loading/error state as discriminated unions. +- Avoiding state/ref shadowing in client components. + +## Implementation order + +1. Stabilize `useSessionEvents` references. +2. Memoize/extract sidebar and list rows. +3. Batch mux session updates and refresh after reconnect. +4. Refactor session detail route to SSR-first + client wrapper. +5. Add `docs/FRONTEND_PATTERNS.md` based on the final implemented pattern. + +Each step should ship independently with tests. + +## Test plan + +- Unit-test `useSessionEvents` reducer behavior for no-op resets and snapshots. +- Add component tests proving focused rename/sidebar UI does not lose state during session updates. +- Test mux reconnect behavior with a mocked WebSocket/session refresh. +- Test session detail loading/error/ready states after the route split. +- Run: + +```bash +pnpm --filter @aoagents/ao-web test +pnpm --filter @aoagents/ao-web typecheck +``` + +## Risks and mitigations + +| Risk | Mitigation | +|---|---| +| Over-refactoring large components | Change only files needed for each phase. Keep phases independently shippable. | +| Breaking terminal behavior | Do not change terminal protocol while changing session updates. Keep mux terminal APIs stable. | +| Stale client state after reconnect | Force one REST refresh after reconnect. | +| Divergent server/client session shapes | Keep serialization centralized in `serialize.ts`. | +| Hard-to-test live behavior | Test reducer logic separately from WebSocket provider behavior. | + +## Definition of done + +- Dashboard still renders from SSR data without requiring WebSocket connection. +- Live session patches update the UI without full-list rerenders on no-op data. +- Session detail follows the same SSR + live update pattern as the dashboard. +- Terminal attach/input/resize still works through mux. +- Web tests and typecheck pass. diff --git a/docs/FRONTEND_STATE_MANAGEMENT_PLAN.md b/docs/FRONTEND_STATE_MANAGEMENT_PLAN.md new file mode 100644 index 000000000..fd6200640 --- /dev/null +++ b/docs/FRONTEND_STATE_MANAGEMENT_PLAN.md @@ -0,0 +1,739 @@ +# Frontend State Management Plan + +> Status: **proposal** | Owner: TBD | Scope: `packages/web` +> Last updated: 2026-05-17 +> Cross-model validated: Claude Opus + OpenAI Codex independently converged on the core approach +> Consolidates: `session-store-proposal.md`, `state-management-improvements.md` (both superseded) + +--- + +## Why This Document Exists + +The web dashboard ships recurring UI bugs -- sidebar flicker, rename inputs that lose focus mid-typing, popovers that close unexpectedly, sluggish mobile menu, optimistic-update "flashes," race conditions on the session detail page, and unnecessary re-renders across all components when a single session changes. + +These are not unrelated. They share a single root cause: **all session state lives in one flat array inside a `useReducer` hook, and every component subscribes to the whole array.** + +This document is the plan to fix the root cause (normalized external store with granular selectors), harden the transport layer, and rewrite the session-detail god-component. + +--- + +## Current Architecture + +``` +┌─────────────────────────────────────────────────────────────────────┐ +│ MuxProvider (Context) │ +│ │ +│ WebSocket ──▶ sessions: SessionPatch[] ◀── one context value │ +│ status: "connected" bundles EVERYTHING │ +│ lastError: string | null │ +│ subscribeTerminal(...) │ +│ writeTerminal(...) │ +│ openTerminal(...) │ +│ closeTerminal(...) │ +│ resizeTerminal(...) │ +└─────────────────────┬───────────────────────────────────────────────┘ + │ any field changes = ALL consumers re-render + ▼ +┌─────────────────────────────────────────────────────────────────────┐ +│ useSessionEvents (useReducer → single State) │ +│ │ +│ State = { │ +│ sessions: DashboardSession[] ◀── one big array │ +│ attentionLevels: Record │ +│ liveSessionsResolved: boolean │ +│ loadError: string | null │ +│ } │ +│ │ +│ Actions: │ +│ "snapshot" → patch 3 fields per session, rebuild attention map │ +│ "reset" → REPLACE entire array (HTTP refresh) ← kills memo │ +└─────────────────────┬───────────────────────────────────────────────┘ + │ returns { sessions, attentionLevels, ... } + ▼ +┌─────────────────────────────────────────────────────────────────────┐ +│ DashboardInner (one component) │ +│ │ +│ Derives EVERYTHING from the sessions array: │ +│ │ +│ projectSessions = filter by projectId (useMemo) │ +│ displaySessions = filter by activeSessionId (useMemo) │ +│ grouped = getAttentionLevel() x N (useMemo, O(n)) │ +│ sessionsByProject = group all by projectId (useMemo) │ +│ projectOverviews = counts per project (useMemo) │ +│ │ +│ ALL 5 memos invalidate when sessions array ref changes │ +└──────┬──────────────┬──────────────────┬────────────────────────────┘ + │ │ │ + ▼ ▼ ▼ + AttentionZone ProjectSidebar ProjectOverviewGrid + (memoized) (NOT memoized) (re-renders always) + │ + ▼ + SessionCard + (memoized -- but parent still re-renders to pass new props) +``` + +Transport-wise this is a good design -- push-primary with HTTP fallback. The problems are all downstream of the transport. + +--- + +## Root-Cause Analysis + +### Core Data Layer + +| # | Problem | File / line | Impact | +|---|---------|-------------|--------| +| 1 | MuxProvider bundles session data + terminal functions in one context | `MuxProvider.tsx:6-20` | Consumers needing only `subscribeTerminal()` re-render on session changes | +| 2 | `"reset"` action replaces entire `sessions` array reference even when content is identical | `useSessionEvents.ts:64-72` | Every 15s HTTP fallback invalidates every downstream `useMemo` and every memoized child | +| 3 | `attentionLevels` object replaced with fresh `Object.fromEntries(...)` every refresh | `useSessionEvents.ts:100-107` | Same cascade on a parallel path | +| 4 | No granular subscription -- components get the whole array or nothing | `useSessionEvents.ts:148` | Single session update re-renders the entire dashboard | +| 5 | No backpressure on WS messages | MuxProvider WS handler | 50 simultaneous transitions = 50 dispatches = 50 renders | + +### Sidebar (`ProjectSidebar.tsx`) + +| # | Problem | File / line | Bug it causes | +|---|---------|-------------|---------------| +| 6 | Not wrapped in `React.memo` | `ProjectSidebar.tsx` export | Re-renders on every Dashboard state change (toast, banner, mobile menu, spawn click) | +| 7 | Per-row JSX inline, no `` subcomponent | `ProjectSidebar.tsx ~L780-910` | Entire list reconciles when one session ticks; rename input remounts and loses focus | +| 8 | Inline `onClick` / `onChange` handlers | throughout | New closures every render defeat any future child memoization | +| 9 | `sessionsByProject` Map rebuilds on every parent render | `ProjectSidebar.tsx L277` | Wasted work per render per project | +| 10 | `usePopoverClamp` recomputes whenever the parent re-renders | `ProjectSidebar.tsx L189-190` | Popover flicker, occasional unexpected close | + +### Session-Detail Route (`app/sessions/[id]/page.tsx`) + +This file is 935 lines, marked `"use client"`, and contains: + +- 18 `useState` calls +- 11 `useRef` (many shadowing state -- a known anti-pattern) +- 14 `useEffect` with overlapping deps +- 3 parallel fetch lifecycles (`fetchSession`, `fetchProjectSessions`, `fetchSidebarSessions`), each with its own `AbortController`, in-flight ref, failure counter, and retry timer +- `SessionDetail` rendered in 5 separate branches, each duplicating the prop list + +It does **not** use `useSessionEvents` -- it reimplements the same logic with different dedupe and abort semantics. + +### Connection Health + +Four overlapping "is live data healthy" signals (`liveSessionsResolved`, `loadError`, `muxLastError`, `mux.status`). Consumers branch on different ones, drift over time. + +--- + +## Target Architecture + +``` +┌─────────────────────────────────────────────────────────────────────┐ +│ MuxProvider (SLIMMED DOWN) │ +│ │ +│ Context only holds: │ +│ status: "connected" | "reconnecting" | "disconnected" │ +│ subscribeTerminal(...) │ +│ writeTerminal(...) │ +│ open/close/resizeTerminal(...) │ +│ │ +│ WebSocket data goes DIRECTLY to SessionStore (not React state) │ +└─────────────────────┬───────────────────────────────────────────────┘ + │ only re-renders when status changes + │ + WebSocket pushes │ rAF batching coalesces bursts + ┃ │ + ▼ │ +┌─────────────────────────────────────────────────────────────────────┐ +│ SessionStore (plain class, outside React) │ +│ │ +│ private sessions = new Map() │ +│ private idsByProject = new Map>() │ +│ private idsByZone = new Map>() │ +│ private version = 0 │ +│ private listeners = new Set() │ +│ │ +│ patch(snapshots) ← WS push, only replaces changed objects │ +│ reconcile(full) ← HTTP refresh, structural merge │ +│ subscribe/getSnapshot ← useSyncExternalStore contract │ +│ │ +│ connectionHealth: ConnectionHealth ← unified health signal │ +└─────────────────────────────────────────────────────────────────────┘ + │ + Granular selector hooks (useSyncExternalStore) + │ + ┌──────────┬─────┴─────────┬──────────────┐ + │ │ │ │ +useSession useZoneIds useProjectIds useCounts + (id) (zone) (projectId) () + │ │ │ │ + ▼ ▼ ▼ ▼ +SessionCard AttentionZone ProjectSidebar FaviconBadge +(own data) (zone id list) (project ids) (counts only) +``` + +--- + +## Store Implementation + +### SessionStore Class + +```typescript +// packages/web/src/lib/session-store.ts + +type Listener = () => void; + +class SessionStore { + private sessions = new Map(); + private idsByProject = new Map>(); + private idsByZone = new Map>(); + private version = 0; + private listeners = new Set(); + + /** WebSocket snapshot path -- only updates changed fields */ + patch(patches: SessionPatch[]): void { + let changed = false; + for (const p of patches) { + const existing = this.sessions.get(p.id); + if (existing && !hasChanged(existing, p)) continue; + this.sessions.set(p.id, merge(existing, p)); + this.updateIndexes(p.id); + changed = true; + } + if (changed) this.notify(); + } + + /** HTTP refresh -- structural merge, preserves unchanged refs */ + reconcile(full: DashboardSession[]): void { + const incomingIds = new Set(); + let changed = false; + + for (const session of full) { + incomingIds.add(session.id); + const existing = this.sessions.get(session.id); + if (existing && isDeepEqual(existing, session)) continue; + this.sessions.set(session.id, session); + this.updateIndexes(session.id); + changed = true; + } + + // Remove sessions no longer present server-side + for (const id of this.sessions.keys()) { + if (!incomingIds.has(id)) { + this.sessions.delete(id); + this.removeFromIndexes(id); + changed = true; + } + } + + if (changed) this.notify(); + } + + // --- Selectors --- + + getSession(id: string): DashboardSession | undefined { + return this.sessions.get(id); + } + + getZoneIds(zone: AttentionLevel): readonly string[] { + return [...(this.idsByZone.get(zone) ?? [])]; + } + + getProjectIds(projectId: string): readonly string[] { + return [...(this.idsByProject.get(projectId) ?? [])]; + } + + getAttentionCounts(): Record { + const counts = {} as Record; + for (const [zone, ids] of this.idsByZone) { + counts[zone] = ids.size; + } + return counts; + } + + getAllSessions(): DashboardSession[] { + return [...this.sessions.values()]; + } + + // --- Subscription (useSyncExternalStore contract) --- + + getSnapshot = (): number => this.version; + + subscribe = (listener: Listener): (() => void) => { + this.listeners.add(listener); + return () => this.listeners.delete(listener); + }; + + // --- Internal --- + + private notify(): void { + this.version++; + for (const listener of this.listeners) listener(); + } + + private updateIndexes(id: string): void { /* ... */ } + private removeFromIndexes(id: string): void { /* ... */ } +} + +export const sessionStore = new SessionStore(); +``` + +### Selector Hooks + +```typescript +// packages/web/src/hooks/useSessionStore.ts + +import { useSyncExternalStore, useRef } from "react"; +import { sessionStore } from "@/lib/session-store"; + +/** Subscribe to a single session by ID */ +export function useSession(id: string): DashboardSession | undefined { + const prev = useRef(undefined); + return useSyncExternalStore( + sessionStore.subscribe, + () => { + const next = sessionStore.getSession(id); + if (prev.current && next && !hasChanged(prev.current, next)) { + return prev.current; + } + prev.current = next; + return next; + }, + ); +} + +/** Subscribe to session IDs in an attention zone */ +export function useZoneSessionIds(zone: AttentionLevel): readonly string[] { + const prev = useRef([]); + return useSyncExternalStore( + sessionStore.subscribe, + () => { + const next = sessionStore.getZoneIds(zone); + if (arraysEqual(prev.current, next)) return prev.current; + prev.current = next; + return next; + }, + ); +} + +/** Subscribe to session IDs for a project */ +export function useProjectSessionIds(projectId: string): readonly string[] { + const prev = useRef([]); + return useSyncExternalStore( + sessionStore.subscribe, + () => { + const next = sessionStore.getProjectIds(projectId); + if (arraysEqual(prev.current, next)) return prev.current; + prev.current = next; + return next; + }, + ); +} + +/** Subscribe to zone counts only (for badges, favicon, document title) */ +export function useAttentionCounts(): Record { + const prev = useRef | null>(null); + return useSyncExternalStore( + sessionStore.subscribe, + () => { + const next = sessionStore.getAttentionCounts(); + if (prev.current && countsEqual(prev.current, next)) return prev.current; + prev.current = next; + return next; + }, + ); +} + +/** Subscribe to connection health only */ +export function useConnectionHealth(): ConnectionHealth { + const prev = useRef(null); + return useSyncExternalStore( + sessionStore.subscribe, + () => { + const next = sessionStore.getConnectionHealth(); + if (prev.current && prev.current.state === next.state) return prev.current; + prev.current = next; + return next; + }, + ); +} +``` + +### MuxProvider Changes + +```typescript +// MuxProvider context ONLY holds terminal functions + status +interface MuxContextValue { + subscribeTerminal: (id: string, cb: (data: string) => void) => () => void; + writeTerminal: (id: string, data: string) => void; + openTerminal: (id: string) => void; + closeTerminal: (id: string) => void; + resizeTerminal: (id: string, cols: number, rows: number) => void; + status: "connecting" | "connected" | "reconnecting" | "disconnected"; +} + +// WebSocket message handler writes directly to store: +ws.onmessage = (event) => { + const msg = JSON.parse(event.data); + if (msg.type === "sessions") { + sessionStore.patch(msg.patches); // bypasses React state entirely + } + // terminal data still dispatched to callbacks +}; +``` + +### Connection Health Type + +Replaces 4 overlapping booleans with a single discriminated union: + +```typescript +type ConnectionHealth = + | { state: "connecting" } + | { state: "live" } + | { state: "stale"; reason: string; since: number } + | { state: "offline"; reason: string }; +``` + +All UI components consume this single value. No more branching on inconsistent signals. + +--- + +## Current vs Target Comparison + +| Concern | Current | Target | +|---------|---------|--------| +| Data structure | `DashboardSession[]` (flat array) | `Map` (normalized) | +| Subscription granularity | Whole array or nothing | Per-session, per-zone, per-project, aggregates | +| HTTP refresh | Replaces all refs (memo collapse) | Structural merge, preserves unchanged refs | +| MuxProvider responsibility | Session data + terminal funcs | Terminal funcs + status only | +| Where state lives | React state (useReducer) | External store (plain class) | +| React integration | Context + prop drilling | `useSyncExternalStore` with selectors | +| SessionCard re-renders | When any session changes | Only when ITS session changes | +| ProjectSidebar re-renders | Every update (not memoized) | Only when project membership/counts change | +| Dashboard memo chain | 5 useMemos, all invalidate together | Replaced by targeted selector hooks | +| Health signals | 4 overlapping booleans | 1 discriminated union | +| Session-detail page | 935-line client god-component | SSR server page + ~150-line client wrapper | +| Dependencies added | -- | None (React 19 built-in) | + +--- + +## Execution Plan + +Each phase ships independently, is reversible per commit, and includes regression tests. + +### Phase 1 -- SessionStore foundation (3 days) + +**Goal:** build the normalized store and selector hooks. Additive only -- no existing code changes. + +**Changes:** + +1. **Create `SessionStore` class** -- `packages/web/src/lib/session-store.ts` + - Normalized `Map` storage + - `patch()` for WebSocket snapshots (only replaces changed objects) + - `reconcile()` for HTTP refresh (structural merge, preserves unchanged refs, removes departed sessions) + - Derived indexes: `idsByProject`, `idsByZone` (maintained eagerly on mutation) + - `ConnectionHealth` discriminated union tracking + - `subscribe` / `getSnapshot` contract for `useSyncExternalStore` + +2. **Create selector hooks** -- `packages/web/src/hooks/useSessionStore.ts` + - `useSession(id)` -- single session, stable ref if unchanged + - `useZoneSessionIds(zone)` -- ID list for a zone, stable ref if membership unchanged + - `useProjectSessionIds(projectId)` -- ID list for a project + - `useAttentionCounts()` -- aggregate counts per zone + - `useConnectionHealth()` -- unified health signal + +3. **Unit tests** for `SessionStore` class (pure class, no React test harness needed): + - `patch()` only replaces changed session objects + - `reconcile()` preserves refs for unchanged sessions, removes departed + - Index maintenance (project, zone) stays consistent after mutations + - `version` only increments on actual changes + - Notification fires exactly once per `patch()`/`reconcile()` batch + +**Files created:** +- `packages/web/src/lib/session-store.ts` +- `packages/web/src/lib/__tests__/session-store.test.ts` +- `packages/web/src/hooks/useSessionStore.ts` + +**Outcome:** store and hooks exist, tested, but nothing uses them yet. Zero risk. + +--- + +### Phase 2 -- Wire data sources + transport hardening (3 days) + +**Goal:** make WebSocket and HTTP write to the store instead of React state. Add WS batching and unified health. + +**Changes:** + +4. **MuxProvider writes to store** -- WebSocket `onmessage` calls `sessionStore.patch()` instead of `setSessions()` React state. Remove `sessions` and `lastError` from `MuxContextValue`. + +5. **WS snapshot batching** -- coalesce WebSocket messages arriving within the same animation frame into a single `patch()` call (`requestAnimationFrame` micro-batch). Prevents 50 sessions transitioning = 50 dispatches = 50 renders. + +6. **HTTP reconciliation** -- Replace `dispatch({ type: "reset", sessions })` in `useSessionEvents` with `sessionStore.reconcile(sessions)`. Structural merge preserves object refs for unchanged sessions. + +7. **Unified `ConnectionHealth`** -- collapse `liveSessionsResolved` + `loadError` + `muxLastError` + `mux.status` into the store's `ConnectionHealth` discriminated union. All UI components consume `useConnectionHealth()` instead of four overlapping booleans. + +8. **Reconnect-with-resume** -- on WS reconnect, fire one `/api/sessions` refresh immediately and reconcile membership before resuming live pushes. + +**Files touched:** +- `packages/web/src/providers/MuxProvider.tsx` +- `packages/web/src/hooks/useSessionEvents.ts` +- `packages/web/src/lib/session-store.ts` (add connection health tracking) +- Consumers that read the old four health signals (`Dashboard.tsx`, `ConnectionBar.tsx`, etc.) + +**Outcome:** data flows through the store. Existing components still work (they read from `useSessionEvents` which now reads from the store). WS bursts are coalesced. Health signals are unified. + +--- + +### Phase 3 -- Migrate consumers to selectors (3 days) + +**Goal:** components subscribe to exactly what they need. Kill the render cascade. + +**Changes:** + +9. **SessionCard** -- uses `useSession(id)` instead of receiving session as prop. Only re-renders when ITS session changes. + +10. **AttentionZone** -- uses `useZoneSessionIds(zone)` to get ID list, renders `` for each. Only re-renders when zone membership changes. + +11. **ProjectSidebar** -- uses `useProjectSessionIds(projectId)` + `useAttentionCounts()`. Extract subcomponents: + - `` -- memoized, per-project + - `` -- memoized, per-row, uses `useSession(id)` internally + - Stable callbacks with `useCallback` for `navigate`, `startRename`, `submitRename`, `cancelRename`, `toggleExpand` + +12. **Dashboard** -- remove the 5 `useMemo` derivations (`projectSessions`, `displaySessions`, `grouped`, `sessionsByProject`, `projectOverviews`). Replaced by selector hooks in child components. + +13. **FaviconBadge / document title** -- uses `useAttentionCounts()` instead of computing from full sessions array. + +14. **Extract `useSidebar()` hook** -- consolidate `sidebarCollapsed`, `mobileMenuOpen`, toggle handlers, backdrop click, and responsive breakpoint logic. Currently duplicated identically across Dashboard, SessionDetail, SessionPage, PullRequestsPage (19 occurrences). + +15. **Regression tests:** + - Mount Dashboard with a frozen session list, dispatch a no-op WS snapshot, assert `ProjectSidebar` rendered exactly once and a focused rename input retains focus + - Verify `SessionCard` does not re-render when a different session changes + - Verify zone component re-renders only on membership change, not on field change within existing members + +**Files touched:** +- `packages/web/src/components/SessionCard.tsx` +- `packages/web/src/components/AttentionZone.tsx` +- `packages/web/src/components/ProjectSidebar.tsx` +- `packages/web/src/components/Dashboard.tsx` +- new: `packages/web/src/components/sidebar/SidebarProjectGroup.tsx` +- new: `packages/web/src/components/sidebar/SidebarSessionRow.tsx` +- new: `packages/web/src/hooks/useSidebar.ts` +- new tests in `packages/web/src/components/__tests__/` + +**Outcome:** render cascade eliminated. Sidebar flicker, focus loss, popover flicker, mobile menu lag, and optimistic-rename flash all fixed. + +--- + +### Phase 4 -- Session-detail page rewrite (3-4 days) + +**Goal:** turn the 935-line client god-component into an SSR-first page + thin client wrapper. + +**Target shape:** + +``` +╭─ app/sessions/[id]/page.tsx (server) ────────────────╮ +│ fetch session + projects on the server (SSR) │ +│ pass to │ +╰────────────────────────────────────┬─────────────────╯ + ▼ +╭─ SessionPageClient (~150 lines) ─────────────────────╮ +│ const session = useSession(id) │ +│ const health = useConnectionHealth() │ +│ │ +│ switch (state.kind) { │ +│ case "loading": return │ +│ case "missing": return │ +│ case "error": return │ +│ case "ready": return │ +│ } │ +╰──────────────────────────────────────────────────────╯ +``` + +**Changes:** + +16. **Move SSR initial-data fetch** to a server component in `page.tsx`. + +17. **Create `SessionPageClient.tsx`** with `"use client"`. This is the only client file for the route. + +18. **Replace the three parallel fetchers** with store selectors (`useSession(id)`, `useProjectSessionIds()`, `useConnectionHealth()`). + +19. **Collapse 18 `useState`s** into one `useReducer` with a discriminated-union state. Impossible states become unrepresentable. + +20. **Drop the 5 duplicate `` render branches** -- pick props once at the top, render once at the bottom. + +21. **`React.memo`** on `SessionDetailHeader` and `SessionDetailPRCard`. + +22. **`SessionDetailPRCard`:** replace the 4 `Set` states with one `useReducer` keyed by `commentId`: + + ```ts + type CommentState = "idle" | "sending" | "sent" | "error"; + type CommentMap = Record; + ``` + +**Files touched:** +- `packages/web/src/app/sessions/[id]/page.tsx` (becomes server component) +- new: `packages/web/src/app/sessions/[id]/SessionPageClient.tsx` +- `packages/web/src/components/SessionDetail.tsx` +- `packages/web/src/components/SessionDetailHeader.tsx` +- `packages/web/src/components/SessionDetailPRCard.tsx` + +**Outcome:** 935 lines to ~150. Race conditions structurally impossible. One store, one source of truth, no ref-shadowed state. + +--- + +### Phase 5 -- Cleanup + guardrails (ongoing) + +**Goal:** delete dead code, prevent regression. + +**Changes:** + +23. **Delete `useSessionEvents` hook** -- fully replaced by store selectors. + +24. **Remove session data from MuxProvider context** -- only terminal functions remain. + +25. **Remove unused props** -- `sessions` prop no longer passed down the tree. + +26. **`PullRequestsPage`** -- migrate to store selectors (same pattern as Dashboard). + +27. **Lint rule** to forbid passing fresh array/object literals as props to memoized components (catches regressions at PR time). + +28. **Type a `LiveData` discriminated union** at the hook boundary so every consumer must handle all health states explicitly. + +--- + +## How This Fixes Bugs + +| Bug class today | Root cause | Fixed in phase | +|---|---|---| +| All components re-render when one session changes | Single-array subscription, no granular selectors | 2-3 | +| HTTP refresh breaks all memoization | `"reset"` replaces entire array with new refs | 2 | +| Rename input loses focus mid-typing | Row remounts on refresh / parent re-render | 3 | +| Popover / menu flicker | `usePopoverClamp` reruns on every parent render | 3 | +| Expand / collapse "jumps back" | Effect fires on spurious ref change | 3 | +| Optimistic rename flashes old name | `pendingRenames` cleanup runs on every ref change | 3 | +| Mobile menu close lag | 50 rows reconcile before animation | 3 | +| Sidebar scroll jumps | DOM reconciliation on large lists every 15s | 3 | +| UI burst on startup (50 sessions transitioning) | No WS message batching | 2 | +| Stale UI window after deploy | No reconnect-with-resume | 2 | +| Different components disagree on "is live" | Four overlapping health signals | 2 | +| Session-detail race conditions | Three parallel fetchers | 4 | +| Session-detail "stale data flashes back" | Ref shadowing state | 4 | +| PR card comment-state inconsistency | Four separate `Set`s for one keyed lifecycle | 4 | + +--- + +## Key Design Decisions + +### Why a class, not a hook? + +The store is a **singleton** that outlives any component mount/unmount. WebSocket reconnections, HTTP refreshes, and React concurrent mode transitions all write to the same store. A class with explicit `subscribe`/`getSnapshot` is the correct primitive for `useSyncExternalStore`. + +### Why normalize by ID? + +Arrays require O(n) scans to find a session. Maps give O(1) lookup. More importantly, replacing a single entry in a Map preserves all other object references -- critical for preventing downstream re-renders. + +### Why derived indexes? + +`idsByProject` and `idsByZone` are maintained eagerly on each `patch()`/`reconcile()`. This avoids O(n) filtering on every render. Selector hooks return stable array refs when membership hasn't changed. + +### Why structural merge for HTTP refresh? + +The current `"reset"` action creates new objects for every session, even if nothing changed. Structural merge compares field-by-field and only replaces the object if data actually differs. This preserves memoization across the HTTP fallback path. + +### Selector equality checks + +Each selector hook uses a `useRef` to cache the previous value and returns the cached ref if the new computation is shallowly equal. This prevents consumers from re-rendering when the store version bumps but their specific slice is unchanged. + +--- + +## Pitfalls to Avoid + +1. **Don't return new arrays on every call.** If `useZoneSessionIds("working")` returns `[...set]` every time the store notifies, all zone consumers still re-render. Cache the array and compare membership before returning a new ref. + +2. **Membership vs field changes are different problems.** Lists subscribe to ID arrays (re-render when sessions join/leave a zone). Cards subscribe to individual session objects (re-render when their data changes). Don't conflate them. + +3. **Don't move the array problem elsewhere.** A store that exposes `getAllSessions()` as the primary hook just relocates the re-render cascade. Granular selectors are the point. + +4. **Test the store as a plain class.** No React test harness needed for the core logic. Test `patch()`, `reconcile()`, index maintenance, and notification behavior independently. + +5. **HTTP reconciliation must not create orphan state.** When `reconcile()` receives a full session list, remove sessions from the store that are no longer present server-side. + +6. **Never shadow `useState` with a `useRef`.** Fix the dep array instead. The session-detail page has 11 refs shadowing state -- this is the pattern we're eliminating. + +7. **SSR hydration.** The store is a singleton. Initialize from `initialSessions` in a layout-level effect, gate selectors with a `hydrated` flag to prevent flash. + +--- + +## Estimated Impact + +| Metric | Before | After | +|--------|--------|-------| +| Re-renders per WS push (50 sessions, 1 changed) | ~15 components | ~3 components | +| Re-renders per HTTP refresh (50 sessions) | ~50+ components (all memo broken) | Only components whose data actually changed | +| ProjectSidebar renders per second | 1 per WS push (~0.2/s) | Only on membership changes | +| SessionCard renders for unrelated updates | Yes (parent re-renders) | No (subscribes to own ID) | +| Session-detail page lines | 935 | ~150 | +| Health signal booleans to track | 4 | 1 discriminated union | +| Bundle size added | -- | ~0 (React built-in) | +| External dependencies | -- | None | + +--- + +## Rollout Order and Risk + +| Phase | Effort | Risk | User-visible win | +|---|---|---|---| +| 1 - Store foundation | 3 days | None (additive only) | None yet (infrastructure) | +| 2 - Wire sources + transport | 3 days | Medium (data path change) | No more WS burst renders, unified health | +| 3 - Migrate consumers | 3 days | Medium | Smoother sidebar, no focus loss, no flicker | +| 4 - Session-detail rewrite | 3-4 days | Medium-High | Faster load, no race conditions | +| 5 - Cleanup + guardrails | Ongoing | Low | Prevents regression | + +**Recommendation:** Phase 1 is zero-risk infrastructure. Phase 2+3 should ship together so the store is both wired and consumed in one release. Phase 4 is independent and can follow in the next cycle. + +--- + +## Acceptance Criteria + +A phase is **done** when: + +- All listed changes are merged +- The phase's regression tests exist and pass on CI +- `pnpm typecheck`, `pnpm lint`, and `pnpm --filter @aoagents/ao-web test` all pass +- A manual smoke through the dashboard + session detail confirms the bugs in the table above are gone +- No new `useRef` shadowing `useState` introduced + +--- + +## Out of Scope + +- **TanStack Query.** Not for the session data hot path. Revisit only if significant HTTP-fetched views are added. +- **Zustand / Jotai / Redux.** `useSyncExternalStore` is sufficient and adds no dependencies. +- **Virtualizing the sidebar.** With per-row memoization via store selectors, lists of <500 sessions render fine. Revisit if a user hits that. +- **Replacing the WebSocket transport.** WS-primary with HTTP fallback is the right design; it just needs the hardening in Phase 2. + +--- + +## Open Questions + +1. **SSR hydration** -- The store is a singleton. On SSR, initial session data comes from server props. How do we hydrate the store on first client render without a flash? Likely: initialize from `initialSessions` prop in a layout-level effect, gate selectors with a `hydrated` flag. + +2. **DevTools** -- Without Redux/React Query DevTools, debugging store state requires a custom solution. Consider exposing `sessionStore` on `window` in dev mode. + +3. **Store reset on navigation** -- When navigating between projects, should the store clear? Current behavior keeps all sessions (sidebar shows all projects) -- preserve this. + +--- + +## Superseded Documents + +This plan consolidates and supersedes: + +- `docs/session-store-proposal.md` -- `useSyncExternalStore` implementation spec (merged into Phases 1-3) +- `docs/state-management-improvements.md` -- hook extraction plan (sidebar hook adopted in Phase 3; fetch hooks replaced by store selectors) + +--- + +## References + +- [`packages/web/src/providers/MuxProvider.tsx`](../packages/web/src/providers/MuxProvider.tsx) +- [`packages/web/src/hooks/useSessionEvents.ts`](../packages/web/src/hooks/useSessionEvents.ts) +- [`packages/web/src/components/Dashboard.tsx`](../packages/web/src/components/Dashboard.tsx) +- [`packages/web/src/components/ProjectSidebar.tsx`](../packages/web/src/components/ProjectSidebar.tsx) +- [`packages/web/src/components/SessionCard.tsx`](../packages/web/src/components/SessionCard.tsx) +- [`packages/web/src/components/AttentionZone.tsx`](../packages/web/src/components/AttentionZone.tsx) +- [`packages/web/src/lib/types.ts`](../packages/web/src/lib/types.ts) +- [`packages/web/src/app/sessions/[id]/page.tsx`](../packages/web/src/app/sessions/%5Bid%5D/page.tsx) +- [`packages/web/src/components/SessionDetail.tsx`](../packages/web/src/components/SessionDetail.tsx) +- [`packages/web/src/components/SessionDetailPRCard.tsx`](../packages/web/src/components/SessionDetailPRCard.tsx) diff --git a/docs/session-store-proposal.md b/docs/session-store-proposal.md new file mode 100644 index 000000000..c73b709f5 --- /dev/null +++ b/docs/session-store-proposal.md @@ -0,0 +1,365 @@ +# Session Store Proposal: useSyncExternalStore + +> Status: **proposal** | Scope: `packages/web` | Dependencies: none (React 19 built-in) +> Cross-model validated: Claude Opus + OpenAI Codex independently converged on this approach + +--- + +## Problem Statement + +The dashboard receives real-time session data via WebSocket push (MuxProvider) and periodic HTTP refresh. All session state lives in a single `DashboardSession[]` array inside a `useReducer` hook. This causes: + +1. **MuxProvider bundles unrelated concerns** -- session data, terminal functions, and connection status in one context. Consumers needing only `subscribeTerminal()` re-render when session data changes. +2. **HTTP refresh collapses memoization** -- the `"reset"` action replaces the entire array with new object references, invalidating all downstream `useMemo` and `React.memo` boundaries. +3. **Dashboard recomputes everything** -- 5 `useMemo` derivations (projectSessions, displaySessions, grouped, sessionsByProject, projectOverviews) all invalidate on any single session change. +4. **ProjectSidebar is not memoized** -- receives the full `sessions` array as a prop, re-renders on every parent update. +5. **No granular subscription** -- components cannot subscribe to a single session or a subset without receiving the entire array. + +--- + +## Proposed Architecture + +### SessionStore (plain class, outside React) + +A normalized store that lives outside React's render cycle. WebSocket and HTTP both write into it. React components subscribe to granular slices via `useSyncExternalStore`. + +``` +WebSocket pushes ──▶ SessionStore.patch() +HTTP refresh ──▶ SessionStore.reconcile() + │ + ▼ + ┌─────────────────────┐ + │ SessionStore │ + │ │ + │ Map │ ◀── normalized by ID + │ Map │ ◀── derived index + │ Map │ ◀── derived index + │ version: number │ ◀── change counter + │ listeners: Set │ ◀── subscribers + └─────────┬───────────┘ + │ + useSyncExternalStore selectors + │ + ┌──────────┬───────┴───────┬──────────────┐ + │ │ │ │ +useSession useZoneIds useProjectIds useCounts + (id) (zone) (projectId) () + │ │ │ │ + ▼ ▼ ▼ ▼ +SessionCard AttentionZone ProjectSidebar FaviconBadge +``` + +### Store Implementation + +```typescript +// packages/web/src/lib/session-store.ts + +type Listener = () => void; + +class SessionStore { + private sessions = new Map(); + private idsByProject = new Map>(); + private idsByZone = new Map>(); + private version = 0; + private listeners = new Set(); + + /** WebSocket snapshot path -- only updates changed fields */ + patch(patches: SessionPatch[]): void { + let changed = false; + for (const p of patches) { + const existing = this.sessions.get(p.id); + if (existing && !hasChanged(existing, p)) continue; + this.sessions.set(p.id, merge(existing, p)); + this.updateIndexes(p.id); + changed = true; + } + if (changed) this.notify(); + } + + /** HTTP refresh -- structural merge, preserves unchanged refs */ + reconcile(full: DashboardSession[]): void { + const incomingIds = new Set(); + let changed = false; + + for (const session of full) { + incomingIds.add(session.id); + const existing = this.sessions.get(session.id); + if (existing && isDeepEqual(existing, session)) continue; + this.sessions.set(session.id, session); + this.updateIndexes(session.id); + changed = true; + } + + // Remove sessions no longer present + for (const id of this.sessions.keys()) { + if (!incomingIds.has(id)) { + this.sessions.delete(id); + this.removeFromIndexes(id); + changed = true; + } + } + + if (changed) this.notify(); + } + + // --- Selectors (stable refs when data unchanged) --- + + getSession(id: string): DashboardSession | undefined { + return this.sessions.get(id); + } + + getZoneIds(zone: AttentionLevel): readonly string[] { + return [...(this.idsByZone.get(zone) ?? [])]; + } + + getProjectIds(projectId: string): readonly string[] { + return [...(this.idsByProject.get(projectId) ?? [])]; + } + + getAttentionCounts(): Record { + const counts = {} as Record; + for (const [zone, ids] of this.idsByZone) { + counts[zone] = ids.size; + } + return counts; + } + + getAllSessions(): DashboardSession[] { + return [...this.sessions.values()]; + } + + // --- Subscription --- + + getSnapshot = (): number => this.version; + + subscribe = (listener: Listener): (() => void) => { + this.listeners.add(listener); + return () => this.listeners.delete(listener); + }; + + // --- Internal --- + + private notify(): void { + this.version++; + for (const listener of this.listeners) listener(); + } + + private updateIndexes(id: string): void { /* ... */ } + private removeFromIndexes(id: string): void { /* ... */ } +} + +export const sessionStore = new SessionStore(); +``` + +### Selector Hooks + +```typescript +// packages/web/src/hooks/useSessionStore.ts + +import { useSyncExternalStore, useRef, useCallback } from "react"; +import { sessionStore } from "@/lib/session-store"; + +/** Subscribe to a single session by ID */ +export function useSession(id: string): DashboardSession | undefined { + const prev = useRef(undefined); + + return useSyncExternalStore( + sessionStore.subscribe, + () => { + const next = sessionStore.getSession(id); + // Stable ref: return previous if unchanged + if (prev.current && next && !hasChanged(prev.current, next)) { + return prev.current; + } + prev.current = next; + return next; + }, + ); +} + +/** Subscribe to session IDs in an attention zone */ +export function useZoneSessionIds(zone: AttentionLevel): readonly string[] { + const prev = useRef([]); + + return useSyncExternalStore( + sessionStore.subscribe, + () => { + const next = sessionStore.getZoneIds(zone); + if (arraysEqual(prev.current, next)) return prev.current; + prev.current = next; + return next; + }, + ); +} + +/** Subscribe to session IDs for a project */ +export function useProjectSessionIds(projectId: string): readonly string[] { + const prev = useRef([]); + + return useSyncExternalStore( + sessionStore.subscribe, + () => { + const next = sessionStore.getProjectIds(projectId); + if (arraysEqual(prev.current, next)) return prev.current; + prev.current = next; + return next; + }, + ); +} + +/** Subscribe to zone counts only (for badges, favicon) */ +export function useAttentionCounts(): Record { + const prev = useRef | null>(null); + + return useSyncExternalStore( + sessionStore.subscribe, + () => { + const next = sessionStore.getAttentionCounts(); + if (prev.current && countsEqual(prev.current, next)) return prev.current; + prev.current = next; + return next; + }, + ); +} +``` + +### MuxProvider Changes + +```typescript +// MuxProvider context ONLY holds terminal functions + status +interface MuxContextValue { + subscribeTerminal: (id: string, cb: (data: string) => void) => () => void; + writeTerminal: (id: string, data: string) => void; + openTerminal: (id: string) => void; + closeTerminal: (id: string) => void; + resizeTerminal: (id: string, cols: number, rows: number) => void; + status: "connecting" | "connected" | "reconnecting" | "disconnected"; +} + +// WebSocket message handler writes directly to store: +ws.onmessage = (event) => { + const msg = JSON.parse(event.data); + if (msg.type === "sessions") { + sessionStore.patch(msg.patches); // <-- bypasses React state entirely + } + // terminal data still dispatched to callbacks +}; +``` + +--- + +## Current vs Proposed Comparison + +| Concern | Current | Proposed | +|---------|---------|----------| +| Data structure | `DashboardSession[]` (flat array) | `Map` (normalized) | +| Subscription granularity | Whole array or nothing | Per-session, per-zone, per-project, aggregates | +| HTTP refresh | Replaces all refs (memo collapse) | Structural merge, preserves unchanged refs | +| MuxProvider responsibility | Session data + terminal funcs | Terminal funcs + status only | +| Where state lives | React state (useReducer) | External store (plain class) | +| React integration | Context + prop drilling | `useSyncExternalStore` with selectors | +| SessionCard re-renders | When any session changes | Only when ITS session changes | +| ProjectSidebar re-renders | Every update (not memoized) | Only when project membership/counts change | +| Dashboard memo chain | 5 useMemos, all invalidate together | Replaced by targeted selector hooks | +| Dependencies added | -- | None (React 19 built-in) | + +--- + +## Migration Path + +### Phase 1: Foundation (additive, no breaking changes) + +1. **Create `SessionStore` class** -- `packages/web/src/lib/session-store.ts` + - Normalized Map storage + - `patch()` and `reconcile()` methods + - Derived indexes (by project, by zone) + - Full unit test suite (pure class, no React needed) + +2. **Create selector hooks** -- `packages/web/src/hooks/useSessionStore.ts` + - `useSession(id)` + - `useZoneSessionIds(zone)` + - `useProjectSessionIds(projectId)` + - `useAttentionCounts()` + - `useConnectionStatus()` + +### Phase 2: Wire data sources + +3. **MuxProvider writes to store** -- WebSocket `onmessage` calls `sessionStore.patch()` instead of `setSessions()` +4. **HTTP reconciliation** -- Replace `dispatch({ type: "reset" })` with `sessionStore.reconcile(sessions)` (structural merge preserving refs) + +### Phase 3: Migrate consumers (incremental, per-component) + +5. **SessionCard** -- uses `useSession(id)` instead of receiving session as prop +6. **AttentionZone** -- uses `useZoneSessionIds(zone)` to get ID list, renders `` for each +7. **ProjectSidebar** -- uses `useProjectSessionIds(projectId)` + `useAttentionCounts()` +8. **Dashboard** -- remove the 5 `useMemo` derivations, replaced by selector hooks +9. **FaviconBadge / document title** -- uses `useAttentionCounts()` + +### Phase 4: Cleanup + +10. **Delete `useSessionEvents` hook** -- no longer needed +11. **Remove session data from MuxProvider context** -- only terminal functions remain +12. **Remove unused props** -- `sessions` prop no longer passed down the tree + +--- + +## Key Design Decisions + +### Why a class, not a hook? + +The store is a **singleton** that outlives any component mount/unmount. WebSocket reconnections, HTTP refreshes, and React concurrent mode transitions all write to the same store. A class with explicit `subscribe`/`getSnapshot` is the correct primitive for `useSyncExternalStore`. + +### Why normalize by ID? + +Arrays require O(n) scans to find a session. Maps give O(1) lookup. More importantly, replacing a single entry in a Map preserves all other object references -- critical for preventing downstream re-renders. + +### Why derived indexes? + +`idsByProject` and `idsByZone` are maintained eagerly on each `patch()`/`reconcile()`. This avoids O(n) filtering on every render. Selector hooks return stable array refs when membership hasn't changed. + +### Why structural merge for HTTP refresh? + +The current `"reset"` action creates new objects for every session, even if nothing changed. Structural merge compares field-by-field and only replaces the object if data actually differs. This preserves memoization across the HTTP fallback path. + +### Selector equality checks + +Each selector hook uses a `useRef` to cache the previous value and returns the cached ref if the new computation is shallowly equal. This prevents consumers from re-rendering when the store version bumps but their specific slice is unchanged. + +--- + +## Pitfalls to Avoid + +1. **Don't return new arrays on every call.** If `useZoneSessionIds("working")` returns `[...set]` every time the store notifies, all zone consumers still re-render. Cache the array and compare membership before returning a new ref. + +2. **Membership vs field changes are different problems.** Lists subscribe to ID arrays (re-render when sessions join/leave a zone). Cards subscribe to individual session objects (re-render when their data changes). Don't conflate them. + +3. **Don't move the array problem elsewhere.** A store that exposes `getAllSessions()` as the primary hook just relocates the re-render cascade. Granular selectors are the point. + +4. **Test the store as a plain class.** No React test harness needed for the core logic. Test `patch()`, `reconcile()`, index maintenance, and notification behavior independently. + +5. **HTTP reconciliation must not create orphan state.** When `reconcile()` receives a full session list, remove sessions from the store that are no longer present server-side. + +--- + +## Estimated Impact + +| Metric | Before | After | +|--------|--------|-------| +| Re-renders per WS push (50 sessions, 1 changed) | ~15 components | ~3 components | +| Re-renders per HTTP refresh (50 sessions) | ~50+ components (all memo broken) | Only components whose data actually changed | +| ProjectSidebar renders per second | 1 per WS push (~0.2/s) | Only on membership changes | +| SessionCard renders for unrelated updates | Yes (parent re-renders) | No (subscribes to own ID) | +| Bundle size added | -- | ~0 (React built-in) | +| External dependencies | -- | None | + +--- + +## Open Questions + +1. **Server-side rendering hydration** -- The store is a singleton. On SSR, initial session data comes from server props. How do we hydrate the store on first client render without a flash? Likely: initialize store from `initialSessions` prop in a layout-level effect, gate selectors with a `hydrated` flag. + +2. **Concurrent mode safety** -- `useSyncExternalStore` is concurrent-mode safe by design (it's why it exists). But verify that `getSnapshot` stability guarantees hold when the store mutates between React's render and commit phases. + +3. **DevTools** -- Without Redux DevTools or React Query DevTools, debugging store state requires a custom solution. Consider exposing `sessionStore` on `window` in dev mode, or building a simple `` component. + +4. **Store reset on navigation** -- When navigating between projects in the dashboard, should the store clear and re-initialize? Or keep all sessions and let selectors filter? Current behavior keeps all sessions (sidebar shows all projects) -- preserve this. diff --git a/docs/state-management-improvements.md b/docs/state-management-improvements.md new file mode 100644 index 000000000..5e9e81516 --- /dev/null +++ b/docs/state-management-improvements.md @@ -0,0 +1,162 @@ +# State Management Improvements + +## Current Architecture + +| Concern | Mechanism | Location | +|---|---|---| +| Real-time sessions | MuxProvider (SSE/WebSocket) | `providers/MuxProvider.tsx` | +| REST data (projects, PRs, settings) | Hand-rolled `fetchJsonWithTimeout` + `useState` per page | Scattered across pages | +| UI state (sidebar, modals, filters) | `useState` per component | Every page component | + +No external state library (no Redux, Zustand, TanStack Query, etc.). + +--- + +## Bottlenecks & Issues + +### 1. Session Detail Page — State Explosion + +**File:** `app/sessions/[id]/page.tsx` + +**Problem:** A single component holds 16 `useState` calls and 14 `useRef` calls (lines 400-439). This mixes three concerns: data fetching orchestration, loading/error state management, and UI layout. The component is ~950 lines. + +``` +useState: session, zoneCounts, projectOrchestratorId, projects, projectsLoading, + sidebarSessions, sidebarOrchestrators, loading, routeError, sessionMissing, + sidebarError, prefixByProject, sidebarCollapsed, mobileSidebarOpen + +useRef: sessionProjectIdRef, sessionIsOrchestratorRef, resolvedProjectSessionsKeyRef, + prefixByProjectRef, hasLoadedSessionRef, pendingMuxSessionsRef, + fetchingSessionRef, fetchingProjectSessionsRef, fetchingSidebarRef, + sessionFetchControllerRef, projectSessionsFetchControllerRef, + sidebarFetchControllerRef, pageUnloadingRef, + sessionLoadFailureCountRef, sessionLoadFirstFailureAtRef, sessionLoadRetryTimerRef +``` + +**Impact:** Hard to reason about, hard to test, any fetch change touches a large file with unrelated UI code. + +### 2. Duplicated Fetch Logic — No Shared Cache + +**Files:** `app/sessions/[id]/page.tsx`, Dashboard layout, sidebar components + +**Problem:** Projects (`/api/projects`) are fetched independently in multiple places. Every page navigation re-fetches from scratch with identical loading/error handling boilerplate. There is no shared in-memory cache, so navigating dashboard → session detail → back causes two full project fetch cycles. + +**Impact:** Unnecessary network requests, loading flash on every navigation, ~40 lines of repeated loading/error state per page. + +### 3. Duplicated Sidebar State — 4x Copy-Paste + +**Files:** +- `components/Dashboard.tsx` (lines 199-200) +- `components/SessionDetail.tsx` (line 66) +- `app/sessions/[id]/page.tsx` (line 201) +- `components/PullRequestsPage.tsx` (lines 74-75) + +**Problem:** `sidebarCollapsed` and `mobileMenuOpen` state + toggle handlers are duplicated identically across 4 page components (19 occurrences total). Each file has its own `useState`, its own toggle callback, its own CSS class logic. + +**Impact:** Any sidebar behavior change requires editing 4+ files identically. Bug-prone, easy to drift. + +### 4. Loading/Error State Boilerplate + +**Problem:** Every data-fetching component repeats the same pattern: +```tsx +const [loading, setLoading] = useState(true); +const [error, setError] = useState(null); +const [data, setData] = useState(null); + +useEffect(() => { + (async () => { + try { + setLoading(true); + setError(null); + const result = await fetch(...); + setData(result); + } catch (err) { + setError(err); + } finally { + setLoading(false); + } + })(); +}, []); +``` + +This appears for projects, sessions, PRs, sidebar sessions, project settings — each with its own timeout, abort controller, and error handling. + +**Impact:** ~60% of the state declarations in page components are fetch orchestration boilerplate, not actual UI state. + +### 5. No In-flight Deduplication Across Pages + +**Problem:** `fetchingSessionRef`, `fetchingProjectSessionsRef`, `fetchingSidebarRef` (page.tsx:430-432) guard against concurrent fetches **within a single component instance**. But navigating between pages can trigger the same API call simultaneously from the old and new page. + +**Impact:** Race conditions on fast navigation, wasted requests. + +--- + +## Decision: No New Libraries + +**Why not TanStack Query:** The hot path is WebSocket push via MuxProvider, not HTTP fetch. TanStack Query's core value (HTTP cache + refetch + stale management) would be underutilized. For the session data specifically, you'd end up manually pushing SSE updates into `queryClient.setQueryData()` — which is just a worse event bus. Revisit only if the app grows significant HTTP-fetched pages (audit logs, history, config management). + +**Why not Zustand:** No shared client state problem exists yet. The duplicated sidebar state is a hook extraction, not a store problem. Zustand becomes useful if/when cross-component client state grows (command palette, multi-step wizards, global drag-and-drop). + +--- + +## Plan + +### Change 1: Extract `useSidebar()` Hook + +**Scope:** Shared UI state hook, no new dependency. + +**Create:** `hooks/useSidebar.ts` + +Consolidate `sidebarCollapsed`, `mobileMenuOpen`, toggle handlers, backdrop click, and responsive breakpoint logic. Optionally persist `sidebarCollapsed` to `localStorage`. + +**Replaces:** 4 duplicated state blocks in Dashboard, SessionDetail, SessionPage, PullRequestsPage. + +**Savings:** ~80 lines of duplicated state + handler code, single source of truth for sidebar behavior. + +### Change 2: Extract `useProjects()` Hook + +**Scope:** Shared fetch hook with in-memory cache, no new dependency. + +**Create:** `hooks/useProjects.ts` + +- Fetches `/api/projects` once, caches in module-level variable +- Returns `{ projects, loading, error, refetch }` +- Subsequent calls return cached data immediately +- Optional stale timeout to refetch in background + +**Replaces:** Duplicated project fetch logic in `page.tsx:460-484` and elsewhere. + +**Savings:** ~40 lines of fetch + loading/error boilerplate per consumer, shared cache eliminates re-fetch on navigation. + +### Change 3: Extract `useSession()` and `useSidebarSessions()` Hooks + +**Scope:** Shared fetch hooks for session detail data. + +**Create:** +- `hooks/useSession.ts` — fetches single session by ID, handles loading/error/missing states +- `hooks/useSidebarSessions.ts` — fetches sidebar session list for the sidebar component + +Each hook encapsulates its own fetch, abort controller, loading state, error state, and retry logic. MuxProvider SSE updates can be merged in via a callback/option. + +**Replaces:** ~200 lines of fetch orchestration in `page.tsx` (lines 460-620). + +**Savings:** Session detail page drops from ~950 lines to ~600 lines. Fetch logic is testable in isolation. + +### Change 4: Simplify Session Detail Page + +**Scope:** Refactor `app/sessions/[id]/page.tsx` to use the new hooks. + +After changes 1-3, the component should only contain: +- Route param extraction +- MuxProvider SSE integration (existing) +- UI layout (JSX) + +Target: remove 14 `useRef` calls (in-flight guards move into hooks), remove 8+ `useState` calls (loading/error/data move into hooks). + +--- + +## What Stays the Same + +- **MuxProvider** — no changes. Already correct for real-time SSE/WebSocket data. +- **No new dependencies** — all improvements are plain React hooks. +- **Existing component APIs** — components receive the same props, behavior is identical.