Fix three renderer perf metrics that lied, and the bug they hid

CLAUDE.md

@@ -391,11 +391,73 @@ replaced them:
    tasks, in a different process from the one `perf-monitor.ts` samples, so
    it is invisible to both React profiler callbacks and main-process
    event-loop lag. The `longtask` PerformanceObserver catches those pauses
-   (plus layout thrash and any non-React work), and per-second heap deltas
-   expose the allocate-and-collect sawtooth as `heapReclaimedMB`.
+   (plus layout thrash and any non-React work).
 3. **`[snapshot]` memory was main-only but unlabelled.** Now prefixed, per
    above.
 
+**Two fields lied for months, and both hid the same class of bug. Read this
+before trusting a `[renderer-*]` line.**
+
+- **`reactCommits` / `reactTotalMs` were structurally 0 in every packaged
+  build.** React's production build compiles out `enableProfilerTimer`, so
+  `<Profiler>`'s `onRender` is never called — `react-dom-client.production.js`
+  contains zero occurrences of `onRender`. `"reactCommits":0` appeared in
+  100% of ~1,823 samples across two log files and never once nonzero. The fix
+  — aliasing `react-dom/client` → `react-dom/profiling` in
+  `electron.vite.config.ts` — turned out to cost more than the number was
+  worth: React 19.2's profiling entry emits a `performance.measure()` per
+  component render for the DevTools Performance track, and in a trace of a
+  loaded session `logComponentRender` + `logComponentEffect` + their
+  `performance.now()` calls were **~15% of total renderer CPU**, plus 21k
+  retained `PerformanceMeasure` objects in a heap snapshot taken while nothing
+  was recording. So the alias is now **opt-in via `HARNESS_REACT_PROFILING=1`**.
+  Default builds ship `reactProfiling: false` on every sample and every
+  consumer renders `n/a` — never `0`, which is what caused the original
+  misreading. **Do not add a bare `react-dom` alias** — the profiling build
+  itself does `require("react-dom")` for `ReactDOMSharedInternals`, so that
+  creates a cycle and the app dies at startup on `reading 'd'`.
+
+  The meta-lesson, since this bug's *fix* became the next bug: **profiling
+  builds are not free, and instrumentation added to explain a slowdown can
+  become a measurable share of it.** After enabling any always-on profiler,
+  re-profile and confirm the instrumentation isn't in its own top-10.
+- **`heapUsedMB` and its deltas are quantized and up to ~20 minutes stale.**
+  Chrome caches `performance.memory` on pages that aren't cross-origin
+  isolated. Observed: `heapUsedMB` pinned at exactly 560.8 for 40 minutes
+  across 617 samples (9 distinct values in an entire log) while real RSS swung
+  600MB inside 30 seconds. So `heapGrowthMB` / `heapReclaimedMB` **cannot**
+  show an allocate-and-collect sawtooth — the exact shape they were added to
+  catch — and when the cached value does refresh, the whole 20-minute delta
+  gets misattributed to one 1-second bucket. The trustworthy number is
+  `rendererRssMB` / `rendererCpuPct` in `[snapshot]`, sampled in main via
+  `app.getAppMetrics()` and scoped to `BrowserWindow` webContents (browser
+  tabs are separate renderer processes and are deliberately excluded). The
+  `performance.memory`-derived fields are suffixed `…Quantized` so they can't
+  be misread as live.
+- **`rendererBlockingMsPerSec` was not per second.** It logged the renderer
+  bucket's raw `blockingMs`. That bucket is nominally 1 s but stretches without
+  bound when the renderer's timer is starved — a DevTools heap snapshot
+  produced a single ~104 s bucket, which surfaced as `blocked=103792ms/s`.
+  Reading those totals as rates overstates blocking by 20-100x and makes a
+  mostly-idle renderer look pegged. Now normalized by `elapsedMs`, with
+  `rendererBucketMs` and `rendererBlockingMsTotal` logged alongside so the raw
+  numbers stay recoverable. **Check `rendererBucketMs` before comparing
+  blocking across snapshots** — a long window is itself a signal that the
+  renderer stalled, and it means everything derived from that bucket is
+  averaged over a period long enough to hide the spike.
+
+Two of these three were caught only because someone asked whether a number
+could physically be what it claimed: 100% zeros, a heap that never moved, and
+1399 ms of blocking inside a 1000 ms second. **Sanity-check units and ranges
+against physical limits before drawing conclusions** — a rate that exceeds its
+own denominator is a units bug, not a finding.
+
+The general lesson, since this has now cost three investigations: **a
+telemetry field that reads a constant is not evidence of a quiet system, it is
+evidence of broken instrumentation.** Before optimizing against a metric,
+confirm it has ever moved — `grep -oE '"field":[0-9.]+' perf.log | sort -u`
+takes seconds and would have caught both of these.
+
 The hard constraint when extending this: **the telemetry must not become
 the bottleneck.** `longtask` can fire continuously under load, so buckets
 are aggregated in memory and emitted at most once a second, only when a

electron.vite.config.ts

@@ -21,6 +21,20 @@ function currentGitBranch(): string {
 
 const DEV_BRANCH = currentGitBranch()
 
+// Opt-in, because the profiling build is not free. React 19.2's profiling
+// entry emits a performance.measure() per component render to populate the
+// DevTools Performance track, and in a trace of a loaded session that logging
+// — logComponentRender + logComponentEffect + the performance.now() calls
+// feeding them — was ~15% of total renderer CPU, alongside 21k retained
+// PerformanceMeasure objects in a heap snapshot taken while nothing was
+// recording. That is a permanent tax on every user to populate a counter that
+// only matters while someone is actively debugging. Build with
+// HARNESS_REACT_PROFILING=1 to get reactCommits back; otherwise samples carry
+// reactProfiling:false and consumers render "n/a" rather than a zero that
+// reads as "React is idle" — the exact misreading that cost two prior
+// investigations.
+const REACT_PROFILING = process.env.HARNESS_REACT_PROFILING === '1'
+
 export default defineConfig({
   main: {
     plugins: [externalizeDepsPlugin({ exclude: [] })],
@@ -56,7 +70,33 @@ export default defineConfig({
   renderer: {
     plugins: [react(), tailwindcss()],
     define: {
-      __HARNESS_DEV_BRANCH__: JSON.stringify(DEV_BRANCH)
+      __HARNESS_DEV_BRANCH__: JSON.stringify(DEV_BRANCH),
+      __HARNESS_REACT_PROFILING__: JSON.stringify(REACT_PROFILING)
+    },
+    resolve: {
+      // React's production build compiles out `enableProfilerTimer`, so
+      // <Profiler>'s onRender is never called and rendererPerf's reactCommits
+      // reads 0 in every packaged build — `react-dom-client.production.js`
+      // contains zero occurrences of `onRender`. That is why the alias exists
+      // at all: `react=0c/0ms` was a measurement artifact in 100% of ~1,800
+      // renderer samples and sent two separate perf investigations looking at
+      // the main process. Under HARNESS_REACT_PROFILING=1 the numbers are real;
+      // by default nothing pretends to measure them.
+      //
+      // ONLY the client entry is swapped. Do not add a bare `react-dom` alias:
+      // react-dom-profiling.profiling.js itself does require("react-dom") to
+      // reach ReactDOMSharedInternals, so aliasing the bare specifier points
+      // that lookup back at the profiling build and the cycle leaves the
+      // internals undefined — the app dies at startup on `reading 'd'`.
+      // Bare `react-dom` (createPortal in WorkspaceView) must keep resolving
+      // to the real package; it carries no second copy of the reconciler.
+      //
+      // Anchored regex, not a bare string: alias `find` also matches on a `/`
+      // prefix, so a plain 'react-dom' key would additionally rewrite
+      // `react-dom/server` to `react-dom/profiling/server`.
+      alias: REACT_PROFILING
+        ? [{ find: /^react-dom\/client$/, replacement: 'react-dom/profiling' }]
+        : []
     },
     build: {
       ssr: false

src/main/desktop-shell.ts

@@ -33,7 +33,7 @@ import { join } from 'path'
 import { BrowserManager } from './browser-manager'
 import { ElectronServerTransport } from './transport-electron'
 import type { Store } from './store'
-import type { PerfMonitor } from './perf-monitor'
+import type { PerfMonitor, RendererProcessMetrics } from './perf-monitor'
 import type { CompoundServerTransport } from './transport-compound'
 import type { PtyManager } from './pty-manager'
 import type { WorktreesFSM } from './worktrees-fsm'
@@ -161,6 +161,39 @@ export interface DesktopShellStartDeps {
 export interface DesktopShellStartHandle {
   startAutoUpdateChecks: () => void
   stopAutoUpdateChecks: () => void
+  getRendererProcessMetrics: () => RendererProcessMetrics | null
+}
+
+/** Real RSS/CPU for the app's own renderer process(es), for PerfMonitor.
+ *
+ *  Scoped to BrowserWindow webContents on purpose. Browser tabs are
+ *  WebContentsViews with their own renderer processes, and folding those in
+ *  would make the app's renderer look like it ballooned whenever the user
+ *  opened a heavy page. */
+function getRendererProcessMetrics(): RendererProcessMetrics | null {
+  const pids = new Set<number>()
+  for (const w of BrowserWindow.getAllWindows()) {
+    if (w.isDestroyed()) continue
+    try {
+      pids.add(w.webContents.getOSProcessId())
+    } catch {
+      // webContents torn down mid-iteration; nothing to attribute.
+    }
+  }
+  if (pids.size === 0) return null
+
+  let rssKB = 0
+  let cpuPct = 0
+  let matched = false
+  for (const m of app.getAppMetrics()) {
+    if (!pids.has(m.pid)) continue
+    matched = true
+    // workingSetSize is KB (Electron's ProcessMetric).
+    rssKB += m.memory.workingSetSize
+    cpuPct += m.cpu.percentCPUUsage
+  }
+  if (!matched) return null
+  return { rssMB: Math.round(rssKB / 1024), cpuPct: Math.round(cpuPct) }
 }
 
 /** Second call. After index.ts has wired its mode-agnostic IPC handlers,
@@ -906,6 +939,7 @@ export function startDesktopShell(deps: DesktopShellStartDeps): DesktopShellStar
 
   return {
     startAutoUpdateChecks,
-    stopAutoUpdateChecks
+    stopAutoUpdateChecks,
+    getRendererProcessMetrics
   }
 }

src/main/index.ts

@@ -5110,6 +5110,9 @@ if (desktopShellMod && desktopEarly) {
   })
   desktopHooks.startAutoUpdateChecks = handle.startAutoUpdateChecks
   desktopHooks.stopAutoUpdateChecks = handle.stopAutoUpdateChecks
+  // Real renderer RSS/CPU for `[snapshot]`. Headless has no BrowserWindow, so
+  // no provider is set there and the fields log as null rather than lying.
+  perfMonitor.setRendererProcessMetricsProvider(handle.getRendererProcessMetrics)
 } else {
   // Headless: no app.whenReady to wait for, no menus, no window. Just
   // boot. The WS server already listens (the webHttpServer.listen call

src/main/perf-monitor.test.ts

@@ -18,6 +18,7 @@ function sample(overrides: Partial<RendererPerfSample> = {}): RendererPerfSample
     heapLimitMB: 4096,
     heapGrowthMB: 0,
     heapReclaimedMB: 0,
+    reactProfiling: true,
     reactCommits: 0,
     reactTotalMs: 0,
     reactMaxMs: 0,
@@ -35,6 +36,14 @@ describe('formatRendererSample', () => {
     expect(line).toContain('react=6c/15.5ms')
   })
 
+  // A zero here reads as "React is idle", which sent two perf investigations
+  // to the wrong process. Unmeasured has to look unmeasured.
+  it('reports React as n/a when the build did not enable profiling', () => {
+    const line = formatRendererSample(sample({ reactProfiling: false }))
+    expect(line).toContain('react=n/a')
+    expect(line).not.toContain('0c/')
+  })
+
   it('omits input latency when no slow events occurred', () => {
     expect(formatRendererSample(sample())).not.toContain('input=')
   })

src/main/perf-monitor.ts

@@ -13,7 +13,7 @@ export function formatRendererSample(s: RendererPerfSample): string {
     `longtasks=${s.longTasks}`,
     `blocked=${s.blockingMs}ms`,
     `maxtask=${s.longTaskMaxMs}ms`,
-    `react=${s.reactCommits}c/${s.reactTotalMs}ms`
+    s.reactProfiling ? `react=${s.reactCommits}c/${s.reactTotalMs}ms` : 'react=n/a'
   ]
   if (s.slowEvents > 0) {
     parts.push(`input=${s.slowEventMaxMs}ms(${s.slowEventName ?? '?'})`)
@@ -33,6 +33,13 @@ const SNAPSHOT_INTERVAL_MS = 30000
 const MICROTASK_PROBE_INTERVAL_MS = 50
 const MICROTASK_DRIFT_THRESHOLD_MS = 50
 
+/** Real per-process renderer usage, measured from main via app.getAppMetrics().
+ *  The renderer's own `performance.memory` is quantized and ~20min stale. */
+export interface RendererProcessMetrics {
+  rssMB: number
+  cpuPct: number
+}
+
 function formatBytes(n: number): string {
   if (n < 1024) return `${n}B`
   if (n < 1024 * 1024) return `${(n / 1024).toFixed(1)}KB`
@@ -82,6 +89,21 @@ export class PerfMonitor {
   // 900MB, which is worse than not reporting memory at all.
   private lastRendererSample: RendererPerfSample | null = null
 
+  // Real renderer RSS/CPU, sampled in main. The renderer CANNOT measure its
+  // own memory usefully: `performance.memory` is quantized and Chrome serves a
+  // cached value for ~20 minutes on pages that aren't cross-origin isolated.
+  // Observed in the wild — heapUsedMB sat at exactly 560.8 for 40 minutes
+  // across 617 samples (9 distinct values in an entire log) while the real RSS
+  // swung 600MB inside 30 seconds. So the renderer's heap* fields cannot show
+  // an allocate-and-collect sawtooth, which is precisely the shape this
+  // telemetry exists to catch. Injected rather than imported so the headless
+  // build doesn't pull in electron; null there, and the fields log as null.
+  private rendererProcessMetricsFn: (() => RendererProcessMetrics | null) | null = null
+
+  setRendererProcessMetricsProvider(fn: () => RendererProcessMetrics | null): void {
+    this.rendererProcessMetricsFn = fn
+  }
+
   start(store: Store, getActivePtyCount: () => number): void {
     this.activePtyCountFn = getActivePtyCount
     this.startTime = Date.now()
@@ -172,9 +194,21 @@ export class PerfMonitor {
       .sort((a, b) => b[1] - a[1])
       .slice(0, 5)
     const r = this.lastRendererSample
+    const rp = this.rendererProcessMetricsFn?.() ?? null
+    // rendererRss is the trustworthy number; rendererHeap is quantized and can
+    // be up to ~20 minutes stale (see rendererProcessMetricsFn). Keep the label
+    // explicit so nobody reads the heap figure as a live value again.
+    const rssPart = rp ? ` rendererRss=${rp.rssMB}MB rendererCpu=${rp.cpuPct}%` : ''
+    // The renderer's bucket is nominally 1s but stretches without bound when
+    // its timer is starved — a DevTools heap snapshot produced a single 104s
+    // bucket. So `blockingMs` is a per-bucket total, not a rate, and dividing
+    // by a presumed 1s overstated blocking by 20-100x. Normalize here and log
+    // the window alongside so the raw total stays recoverable.
+    const bucketSec = r ? Math.max(r.elapsedMs, 1) / 1000 : 1
+    const blockedPerSec = r ? Math.round(r.blockingMs / bucketSec) : null
     const rendererPart = r
-      ? ` rendererHeap=${r.heapUsedMB}MB rendererBlocked=${r.blockingMs}ms/s`
-      : ' rendererHeap=n/a'
+      ? `${rssPart} rendererHeapQuantized=${r.heapUsedMB}MB rendererBlocked=${blockedPerSec}ms/s window=${bucketSec.toFixed(1)}s`
+      : `${rssPart} rendererHeapQuantized=n/a`
     perfLog(
       'snapshot',
       `store=${this.storeEventsPerSec}/s ipc=${this.ipcMessagesPerSec}/s gh=${this.githubApiCallsPerSec}/s term=${formatBytes(this.totalTerminalBytesPerSec)}/s lag=${this.eventLoopLagMs}ms mainRss=${rssMB}MB${rendererPart} ptys=${ptys}`,
@@ -186,10 +220,18 @@ export class PerfMonitor {
         eventLoopLagMs: this.eventLoopLagMs,
         mainRssMB: rssMB,
         mainHeapUsedMB: heapMB,
-        rendererHeapUsedMB: r?.heapUsedMB ?? null,
-        rendererHeapTotalMB: r?.heapTotalMB ?? null,
-        rendererBlockingMsPerSec: r?.blockingMs ?? null,
-        rendererLongTasksPerSec: r?.longTasks ?? null,
+        rendererRssMB: rp?.rssMB ?? null,
+        rendererCpuPct: rp?.cpuPct ?? null,
+        // Suffixed, not bare: these come from `performance.memory` and are
+        // quantized + cached for ~20min, so a delta between two adjacent
+        // snapshots is meaningless. Compare rendererRssMB instead.
+        rendererHeapUsedMBQuantized: r?.heapUsedMB ?? null,
+        rendererHeapTotalMBQuantized: r?.heapTotalMB ?? null,
+        rendererBlockingMsPerSec: blockedPerSec,
+        rendererLongTasksPerSec: r ? Math.round(r.longTasks / bucketSec) : null,
+        rendererBlockingMsTotal: r?.blockingMs ?? null,
+        rendererLongTasksTotal: r?.longTasks ?? null,
+        rendererBucketMs: r?.elapsedMs ?? null,
         rendererSampleAgeMs: r ? Date.now() - r.t : null,
         activePtyCount: ptys,
         topEventTypes: Object.fromEntries(top)

src/renderer/App.tsx

@@ -8,7 +8,7 @@ import { useHotkeyHandlers } from './hooks/useHotkeyHandlers'
 import { useWorktreeHandlers } from './hooks/useWorktreeHandlers'
 import { useWorktreeCollapse } from './hooks/useWorktreeCollapse'
 import { useWorktreeListModel, useTabsByWorktree } from './hooks/useWorktreeListModel'
-import type { Worktree, TerminalTab, PtyStatus, QuestStep, PendingWorktree, UpdaterStatus, RepoConfig, PaneNode, ForkSource } from './types'
+import type { Worktree, TerminalTab, PtyStatus, QuestStep, PendingWorktree, UpdaterStatus, RepoConfig, PaneNode, ForkSource, AgentKind } from './types'
 import { getLeaves, findLeaf } from '../shared/state/terminals'
 import { CheckCircle2, FolderOpen } from 'lucide-react'
 import { BUILT_IN_THEMES_BY_MODE } from './themes'
@@ -733,6 +733,21 @@ const setQuestStep = useCallback((next: QuestStep) => {
     setActiveWorktreeId
   })
 
+  // Hoisted out of the WorkspaceView JSX below. Every worktree stays mounted,
+  // so an inline arrow here allocates N new callbacks on every App render and
+  // defeats WorkspaceView's memo before it can compare anything else.
+  const handleFocusPane = useCallback((wtPath: string, paneId: string) => {
+    setActivePaneId((prev) => (prev[wtPath] === paneId ? prev : { ...prev, [wtPath]: paneId }))
+  }, [])
+
+  const effectiveDefaultAgent = defaultAgent ?? 'claude'
+  const handleAddAgentTabWithDefault = useCallback(
+    (wtPath: string, kind: AgentKind | undefined, paneId?: string) => {
+      handleAddAgentTab(wtPath, kind ?? effectiveDefaultAgent, paneId)
+    },
+    [handleAddAgentTab, effectiveDefaultAgent]
+  )
+
   // True when the active worktree's workspace (and its tab bar) is actually on
   // screen — i.e. no full-content view (new-worktree, activity, cleanup, command
   // center, review, report-issue) is replacing it and the active worktree isn't
@@ -1591,16 +1606,14 @@ const setQuestStep = useCallback((next: QuestStep) => {
                   branch={wt.branch}
                   paneTree={paneTree}
                   focusedPaneId={activePaneId[wt.path] || leaves[0]?.id || ''}
-                  statuses={statuses}
-                  shellActivity={shellActivity}
                   visible={isVisible}
                   crashedTabIds={crashedTabIds}
                   nameAgentSessions={nameAgentSessions}
                   onSelectTab={handleSelectTab}
-                  onFocusPane={(wtPath, paneId) => setActivePaneId((prev) => prev[wtPath] === paneId ? prev : { ...prev, [wtPath]: paneId })}
+                  onFocusPane={handleFocusPane}
                   onAddTab={handleAddTerminalTab}
-                  defaultAgent={defaultAgent ?? 'claude'}
-                  onAddAgentTab={(wt, kind, paneId) => handleAddAgentTab(wt, kind ?? defaultAgent ?? 'claude', paneId)}
+                  defaultAgent={effectiveDefaultAgent}
+                  onAddAgentTab={handleAddAgentTabWithDefault}
                   onAddBrowserTab={handleAddBrowserTab}
                   onAddJsonClaudeTab={handleAddJsonClaudeTab}
                   onConvertTabType={handleConvertTabType}