🤖 fix: stabilize WorkspaceSidebarState getSnapshot (#1226)

Fix React `useSyncExternalStore` warning by ensuring
`WorkspaceStore.getWorkspaceSidebarState()` returns a
referentially-stable snapshot within a given `WorkspaceState` version
(avoids `Date.now()`-derived instability).

Adds a regression test covering the active-stream + pending-tool case.

---

<details>
<summary>📋 Implementation Plan</summary>

# Fix React `useSyncExternalStore` getSnapshot caching warning
(Workspace sidebar)

## What’s happening / why this warns
React’s `useSyncExternalStore()` calls `getSnapshot()` more than once
during a render/commit. If `getSnapshot()` returns a *different* value
(by `Object.is`) between those reads, React warns:

> “The result of getSnapshot should be cached to avoid an infinite loop”

In our case the call chain is:

- `WorkspaceStatusIndicator` → `useWorkspaceSidebarState(workspaceId)`
- `useWorkspaceSidebarState` uses `useSyncExternalStore(..., () =>
store.getWorkspaceSidebarState(workspaceId))`
- `WorkspaceStore.getWorkspaceSidebarState()` currently computes
`timingStats` using
`StreamingMessageAggregator.getActiveStreamTimingStats()`, which reads
`Date.now()` (pending tool duration + live TPS).

That makes consecutive `getSnapshot()` reads produce *different* objects
even when the store didn’t “change” (no `bump()`), tripping the warning
and risking update loops.

---

## Recommended approach (minimal, correct): cache sidebar snapshot per
workspace-state version
**Net LoC (product code): +15–30**

### Goal
Make `WorkspaceStore.getWorkspaceSidebarState()` return the *exact same
object reference* whenever the underlying workspace state hasn’t changed
(i.e., between consecutive React snapshot reads).

### Implementation steps
1) **Add a per-workspace “source state” cache** in `WorkspaceStore`:
- `sidebarStateCache: Map<string, WorkspaceSidebarState>` already
exists.
- Add `sidebarStateSourceRef: Map<string, WorkspaceState>` (or similar).

2) **Early-return the cached sidebar state when the underlying
`WorkspaceState` reference is unchanged**:
   - At the top of `getWorkspaceSidebarState(workspaceId)`:
     - `const fullState = this.getWorkspaceState(workspaceId);`
- If `cachedSidebar && sidebarStateSourceRef.get(workspaceId) ===
fullState`, return `cachedSidebar`.

This ensures `getSnapshot()` is deterministic for a given store version
and avoids calling any `Date.now()`-dependent code more than once per
version.

3) Keep the existing “field equality” logic for the cross-version case:
- When `fullState` changes because `states.bump(workspaceId)` happened,
recompute the candidate sidebar state.
- If relevant fields are unchanged, keep returning the previously cached
object reference.

4) **Cleanup:** ensure `removeWorkspace()` deletes the new
`sidebarStateSourceRef` entry.

### Verification
- Manually: run the app, start a stream with an in-flight tool call,
open the sidebar; confirm the React warning no longer appears.
- Programmatically: add a unit regression test (below).

---

## Regression test (recommended)
**Net LoC (tests only; not counted in product LoC): +30–60**

Add a focused test in `src/browser/stores/WorkspaceStore.test.ts`:
- Create a workspace + aggregator.
- Drive the aggregator into an “active stream + pending tool” state:
  - `aggregator.handleStreamStart({ type: "stream-start", ... })`
  - `aggregator.handleToolCallStart({ type: "tool-call-start", ... })`
  - (No `store.states.bump()` in between.)
- Call `store.getWorkspaceSidebarState(workspaceId)` twice.
- Assert referential stability:
  - `expect(first).toBe(second)`

This test specifically covers the prior failure mode where `Date.now()`
made `timingStats` differ across consecutive snapshot reads.

---

## Alternatives (if we want a larger cleanup)

### A) Split “status UI” subscriptions from “timing stats” (strong
long-term ergonomics)
**Net LoC (product code): +40–90**

- Introduce a narrower hook for status indicators, e.g.
`useWorkspaceStatusIndicatorState(workspaceId)` that returns only `{
agentStatus, awaitingUserQuestion, canInterrupt, currentModel,
recencyTimestamp }`.
- Move timing-related data into a dedicated hook (or rely exclusively on
`useWorkspaceStatsSnapshot`).

Benefits: avoids coupling sidebar items to high-churn / time-derived
fields.

### B) Make timing stats updates explicit (if we truly want “live”
TPS/tool time)
**Net LoC (product code): +60–140**

- Remove `Date.now()` usage from *snapshot reads*.
- Instead:
  - either compute “live” values in UI with a timer (`useNow(250ms)`),
- or run a store timer that periodically recomputes timing stats and
calls `states.bump(workspaceId)`.

---

<details>
<summary>Notes / follow-ups</summary>

- There’s a similar `useSyncExternalStore` pitfall in
`useAllExperiments()` (`ExperimentsContext.tsx`) which returns a
freshly-created object each snapshot read. It’s currently unused, but
worth fixing if we ever start using it.
- The repo already has a good reference implementation for this exact
React rule in `usePersistedState.ts` (it caches parsed JSON by raw
localStorage string).

</details>

</details>

---
_Generated with `mux` • Model: `openai:gpt-5.2` • Thinking: `xhigh`_

Signed-off-by: Thomas Kosiewski <tk@coder.com>

Author: ThomasK33

src/browser/stores/WorkspaceStore.test.ts

@@ -1,5 +1,6 @@
 import { describe, expect, it, beforeEach, afterEach, mock, type Mock } from "bun:test";
 import type { FrontendWorkspaceMetadata } from "@/common/types/workspace";
+import type { StreamStartEvent, ToolCallStartEvent } from "@/common/types/stream";
 import type { WorkspaceChatMessage } from "@/common/orpc/types";
 import { DEFAULT_RUNTIME_CONFIG } from "@/common/constants/workspace";
 import { WorkspaceStore } from "./WorkspaceStore";
@@ -370,6 +371,61 @@ describe("WorkspaceStore", () => {
       expect(state1).toBe(state2);
     });
 
+    it("getWorkspaceSidebarState() returns same reference when WorkspaceState hasn't changed", () => {
+      const originalNow = Date.now;
+      let now = 1000;
+      Date.now = () => now;
+
+      try {
+        const workspaceId = "test-workspace";
+        createAndAddWorkspace(store, workspaceId);
+
+        const aggregator = store.getAggregator(workspaceId);
+        expect(aggregator).toBeDefined();
+        if (!aggregator) {
+          throw new Error("Expected aggregator to exist");
+        }
+
+        const streamStart: StreamStartEvent = {
+          type: "stream-start",
+          workspaceId,
+          messageId: "msg1",
+          model: "claude-opus-4",
+          historySequence: 1,
+          startTime: 500,
+          mode: "exec",
+        };
+        aggregator.handleStreamStart(streamStart);
+
+        const toolStart: ToolCallStartEvent = {
+          type: "tool-call-start",
+          workspaceId,
+          messageId: "msg1",
+          toolCallId: "tool1",
+          toolName: "test_tool",
+          args: {},
+          tokens: 0,
+          timestamp: 600,
+        };
+        aggregator.handleToolCallStart(toolStart);
+
+        // Simulate store update (MapStore version bump) after handling events.
+        store.bumpState(workspaceId);
+
+        now = 1300;
+        const sidebar1 = store.getWorkspaceSidebarState(workspaceId);
+
+        // Advance time without a store bump. Without snapshot caching, this would
+        // produce a new object due to Date.now()-derived timing stats.
+        now = 1350;
+        const sidebar2 = store.getWorkspaceSidebarState(workspaceId);
+
+        expect(sidebar2).toBe(sidebar1);
+      } finally {
+        Date.now = originalNow;
+      }
+    });
+
     it("syncWorkspaces() does not emit when workspaces unchanged", () => {
       const listener = mock(() => undefined);
       store.subscribe(listener);

src/browser/stores/WorkspaceStore.ts

@@ -755,6 +755,11 @@ export class WorkspaceStore {
 
   // Cache sidebar state objects to return stable references
   private sidebarStateCache = new Map<string, WorkspaceSidebarState>();
+  // Map from workspaceId -> the WorkspaceState reference used to compute sidebarStateCache.
+  // React's useSyncExternalStore may call getSnapshot() multiple times per render; this
+  // ensures getWorkspaceSidebarState() returns a referentially stable snapshot for a given
+  // MapStore version even when timingStats would otherwise change via Date.now().
+  private sidebarStateSourceState = new Map<string, WorkspaceState>();
 
   /**
    * Get sidebar state for a workspace (subset of full state).
@@ -763,6 +768,12 @@ export class WorkspaceStore {
    */
   getWorkspaceSidebarState(workspaceId: string): WorkspaceSidebarState {
     const fullState = this.getWorkspaceState(workspaceId);
+
+    const cached = this.sidebarStateCache.get(workspaceId);
+    if (cached && this.sidebarStateSourceState.get(workspaceId) === fullState) {
+      return cached;
+    }
+
     const aggregator = this.aggregators.get(workspaceId);
 
     // Get timing stats: prefer active stream, fall back to last completed
@@ -787,9 +798,7 @@ export class WorkspaceStore {
     // Get session-level aggregate stats
     const sessionStats = aggregator?.getSessionTimingStats() ?? null;
 
-    const cached = this.sidebarStateCache.get(workspaceId);
-
-    // Return cached if values match (timing stats checked by reference since they change frequently)
+    // Return cached if values match.
     if (
       cached &&
       cached.canInterrupt === fullState.canInterrupt &&
@@ -804,6 +813,9 @@ export class WorkspaceStore {
         (cached.sessionStats?.totalDurationMs === sessionStats?.totalDurationMs &&
           cached.sessionStats?.responseCount === sessionStats?.responseCount))
     ) {
+      // Even if we re-use the cached object, mark it as derived from the current
+      // WorkspaceState so repeated getSnapshot() reads during this render are stable.
+      this.sidebarStateSourceState.set(workspaceId, fullState);
       return cached;
     }
 
@@ -818,6 +830,7 @@ export class WorkspaceStore {
       sessionStats,
     };
     this.sidebarStateCache.set(workspaceId, newState);
+    this.sidebarStateSourceState.set(workspaceId, fullState);
     return newState;
   }
 
@@ -1215,6 +1228,7 @@ export class WorkspaceStore {
     this.recencyCache.delete(workspaceId);
     this.previousSidebarValues.delete(workspaceId);
     this.sidebarStateCache.delete(workspaceId);
+    this.sidebarStateSourceState.delete(workspaceId);
     this.workspaceCreatedAt.delete(workspaceId);
     this.workspaceStats.delete(workspaceId);
     this.statsStore.delete(workspaceId);