diff --git a/CLAUDE.md b/CLAUDE.md index c2a6381d8..e9f852412 100644 --- a/CLAUDE.md +++ b/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 + ``'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 diff --git a/electron.vite.config.ts b/electron.vite.config.ts index 5dfe03a6c..f51830d2d 100644 --- a/electron.vite.config.ts +++ b/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 + // '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 diff --git a/src/main/desktop-shell.ts b/src/main/desktop-shell.ts index 691ebc895..525b636f9 100644 --- a/src/main/desktop-shell.ts +++ b/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() + 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 } } diff --git a/src/main/index.ts b/src/main/index.ts index 69b367dc4..99ee14834 100644 --- a/src/main/index.ts +++ b/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 diff --git a/src/main/perf-monitor.test.ts b/src/main/perf-monitor.test.ts index feff32d81..819e8ed1c 100644 --- a/src/main/perf-monitor.test.ts +++ b/src/main/perf-monitor.test.ts @@ -18,6 +18,7 @@ function sample(overrides: Partial = {}): 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=') }) diff --git a/src/main/perf-monitor.ts b/src/main/perf-monitor.ts index 3c1f4f6a3..746d77577 100644 --- a/src/main/perf-monitor.ts +++ b/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) diff --git a/src/renderer/App.tsx b/src/renderer/App.tsx index 0bd1c7bef..5776040b3 100644 --- a/src/renderer/App.tsx +++ b/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} diff --git a/src/renderer/components/PerfMonitorHUD.tsx b/src/renderer/components/PerfMonitorHUD.tsx index e2e417ce1..2dc6d4f85 100644 --- a/src/renderer/components/PerfMonitorHUD.tsx +++ b/src/renderer/components/PerfMonitorHUD.tsx @@ -31,6 +31,9 @@ interface CombinedSample extends PerfSample { rendererHeap: number } +/** Neither good nor bad — the number simply wasn't collected. */ +const NOT_MEASURED_CLASS = 'text-faint' + function statusClass(value: number, green: number, amber: number): string { if (value < green) return 'text-success' if (value < amber) return 'text-warning' @@ -325,21 +328,23 @@ export function PerfMonitorHUD({ onClose }: Props): JSX.Element { ) const [fps, setFps] = useState(60) const [windowMode, setWindowMode] = useState('1s') - const [enabledMetrics, setEnabledMetrics] = useState>( - () => - new Set([ - 'storeEvents', - 'ipcMessages', - 'githubApi', - 'terminalBytes', - 'eventLoopLag', - 'memory', - 'reactCommits', - 'reactRenderMs', - 'blockingMs', - 'rendererHeap', - ]) - ) + // Only true under HARNESS_REACT_PROFILING=1; otherwise onRender never fires + // and the react* fields are zeros, not measurements. + const reactProfiling = renderLatest.reactProfiling + const [enabledMetrics, setEnabledMetrics] = useState>(() => { + const keys: MetricKey[] = [ + 'storeEvents', + 'ipcMessages', + 'githubApi', + 'terminalBytes', + 'eventLoopLag', + 'memory', + 'blockingMs', + 'rendererHeap', + ] + if (rendererPerf.getLatest().reactProfiling) keys.push('reactCommits', 'reactRenderMs') + return new Set(keys) + }) const frameCountRef = useRef(0) const lastFpsTimeRef = useRef(performance.now()) const rafRef = useRef(0) @@ -462,14 +467,18 @@ export function PerfMonitorHUD({ onClose }: Props): JSX.Element { { key: 'reactCommits', label: 'React commits', - value: `${renderLatest.reactCommits}/s`, - valueClassName: statusClass(renderLatest.reactCommits, 20, 60), + value: reactProfiling ? `${renderLatest.reactCommits}/s` : 'n/a', + valueClassName: reactProfiling + ? statusClass(renderLatest.reactCommits, 20, 60) + : NOT_MEASURED_CLASS, }, { key: 'reactRenderMs', label: 'React time', - value: `${renderLatest.reactTotalMs.toFixed(1)}ms/s`, - valueClassName: statusClass(renderLatest.reactTotalMs, 16, 50), + value: reactProfiling ? `${renderLatest.reactTotalMs.toFixed(1)}ms/s` : 'n/a', + valueClassName: reactProfiling + ? statusClass(renderLatest.reactTotalMs, 16, 50) + : NOT_MEASURED_CLASS, }, { key: 'blockingMs', @@ -543,8 +552,10 @@ export function PerfMonitorHUD({ onClose }: Props): JSX.Element { /> - shellActivity: Record repoLabel: string branch: string registerSlot: (paneId: string, el: HTMLDivElement | null) => void @@ -110,8 +115,6 @@ const TAB_STATUS_DOT: Record = { interface SortableTabProps { tab: TerminalTab isActive: boolean - status: PtyStatus - shellActivity?: { active: boolean; processName?: string } showClose: boolean onSelect: () => void onClose: () => void @@ -163,7 +166,13 @@ function TabProgressBar({ terminalId }: { terminalId: string }): JSX.Element | n ) } -function SortableTab({ tab, isActive, status, shellActivity, showClose, onSelect, onClose, onConvertTabType, onSleepTab, onRename }: SortableTabProps): JSX.Element { +function SortableTab({ tab, isActive, showClose, onSelect, onClose, onConvertTabType, onSleepTab, onRename }: SortableTabProps): JSX.Element { + // Read per-tab rather than taking the whole statuses/shellActivity maps as + // props: a single terminal's status change used to allocate a new map and + // re-render every mounted worktree's subtree. Same pattern as + // TabProgressBar / SpectatorChip above. + const status = useTerminalStatus(tab.id) + const shellActivity = useShellActivity(tab.id) ?? undefined const { attributes, listeners, setNodeRef, transform, transition, isDragging } = useSortable({ id: tab.id }) @@ -530,8 +539,6 @@ export function TerminalPanel({ worktreePath, pane, paneCount, - statuses, - shellActivity, repoLabel, branch, registerSlot, @@ -677,8 +684,6 @@ export function TerminalPanel({ key={tab.id} tab={tab} isActive={tab.id === pane.activeTabId} - status={statuses[tab.id] || 'idle'} - shellActivity={shellActivity[tab.id]} showClose={pane.tabs.length > 1 || paneCount > 1} onSelect={() => onSelectTab(tab.id)} onClose={() => onCloseTab(tab.id)} diff --git a/src/renderer/components/WorkspaceView.tsx b/src/renderer/components/WorkspaceView.tsx index d87098c2c..ba1fdff57 100644 --- a/src/renderer/components/WorkspaceView.tsx +++ b/src/renderer/components/WorkspaceView.tsx @@ -1,4 +1,4 @@ -import { useCallback, useRef, useEffect } from 'react' +import { memo, useCallback, useRef, useEffect } from 'react' import { createPortal } from 'react-dom' import { DndContext, @@ -11,7 +11,7 @@ import { type DragOverEvent, type CollisionDetection } from '@dnd-kit/core' -import type { PaneNode, PaneLeaf, PaneSplit, PtyStatus, AgentKind } from '../types' +import type { PaneNode, PaneLeaf, PaneSplit, AgentKind } from '../types' import { getLeaves, findLeafByTabId } from '../../shared/state/terminals' import { TerminalPanel } from './TerminalPanel' import { XTerminal } from './XTerminal' @@ -27,8 +27,6 @@ interface WorkspaceViewProps { worktreePath: string paneTree: PaneNode focusedPaneId: string - statuses: Record - shellActivity: Record visible: boolean nameAgentSessions: boolean repoLabel: string @@ -138,8 +136,6 @@ function SplitRenderer({ node, worktreePath, focusedPaneId, - statuses, - shellActivity, repoLabel, branch, nameAgentSessions, @@ -168,8 +164,6 @@ function SplitRenderer({ node: PaneNode worktreePath: string focusedPaneId: string - statuses: Record - shellActivity: Record repoLabel: string branch: string nameAgentSessions: boolean @@ -210,8 +204,6 @@ function SplitRenderer({ pane={node} isFocused={node.id === focusedPaneId} paneCount={leafCount} - statuses={statuses} - shellActivity={shellActivity} repoLabel={showLabel ? repoLabel : ''} branch={showLabel ? branch : ''} registerSlot={registerSlot} @@ -254,8 +246,6 @@ function SplitRenderer({ node={split.children[0]} worktreePath={worktreePath} focusedPaneId={focusedPaneId} - statuses={statuses} - shellActivity={shellActivity} repoLabel={repoLabel} branch={branch} nameAgentSessions={nameAgentSessions} @@ -296,8 +286,6 @@ function SplitRenderer({ node={split.children[1]} worktreePath={worktreePath} focusedPaneId={focusedPaneId} - statuses={statuses} - shellActivity={shellActivity} repoLabel={repoLabel} branch={branch} nameAgentSessions={nameAgentSessions} @@ -328,12 +316,10 @@ function SplitRenderer({ ) } -export function WorkspaceView({ +function WorkspaceViewInner({ worktreePath, paneTree, focusedPaneId, - statuses, - shellActivity, visible, nameAgentSessions, onSelectTab, @@ -515,8 +501,6 @@ export function WorkspaceView({ node={paneTree} worktreePath={worktreePath} focusedPaneId={focusedPaneId} - statuses={statuses} - shellActivity={shellActivity} repoLabel={repoLabel} branch={branch} nameAgentSessions={nameAgentSessions} @@ -659,6 +643,12 @@ export function WorkspaceView({ ) } +// App keeps every worktree mounted, so this component exists ~N times over and +// every App render re-rendered all of them. Memo is only sound now that the +// whole `statuses` / `shellActivity` maps are no longer props — they changed +// identity on every streamed token, which would have made this a no-op. +export const WorkspaceView = memo(WorkspaceViewInner) + function findTopLeftLeaf(node: PaneNode): PaneLeaf { if (node.type === 'leaf') return node return findTopLeftLeaf(node.children[0]) diff --git a/src/renderer/main.tsx b/src/renderer/main.tsx index 27f9fc7fe..76912a96f 100644 --- a/src/renderer/main.tsx +++ b/src/renderer/main.tsx @@ -1,5 +1,5 @@ import './styles.css' -import { Profiler, type ProfilerOnRenderCallback } from 'react' +import { Profiler, type ProfilerOnRenderCallback, type ReactNode } from 'react' import { createRoot } from 'react-dom/client' import App from './App' import { initStore } from './store' @@ -17,15 +17,29 @@ const onRender: ProfilerOnRenderCallback = (_id, _phase, actualDuration) => { rendererPerf.recordCommit(actualDuration) } +// Without the react-dom/profiling alias onRender is never called, so mounting +// would only add tree depth for a counter that stays 0. +// See electron.vite.config.ts for why the alias is opt-in. +const reactProfiling = + typeof __HARNESS_REACT_PROFILING__ !== 'undefined' && __HARNESS_REACT_PROFILING__ +if (reactProfiling) rendererPerf.markReactProfilingEnabled() + +function withProfiler(children: ReactNode): ReactNode { + if (!reactProfiling) return children + return ( + + {children} + + ) +} + initStore() .then(() => { defineHarnessTheme() rendererPerf.start((sample) => getBackend().perfReportRendererSample(sample)) createRoot(document.getElementById('root')!).render( - - - + {withProfiler()} ) diff --git a/src/renderer/renderer-perf.test.ts b/src/renderer/renderer-perf.test.ts index d808db377..349360d48 100644 --- a/src/renderer/renderer-perf.test.ts +++ b/src/renderer/renderer-perf.test.ts @@ -18,6 +18,7 @@ function bucket(overrides: Partial = {}): Omit { expect(computeFlags(bucket({ reactCommits: 1, reactTotalMs: 3, reactMaxMs: 3 }))).toEqual([]) }) + it('never flags react when profiling is off, whatever the counters say', () => { + const notMeasured = bucket({ reactProfiling: false, reactCommits: 40, reactTotalMs: 180 }) + expect(computeFlags(notMeasured)).not.toContain('react') + }) + // Chromium throttles timers in hidden windows, so a backgrounded renderer // hands back one enormous bucket. Flagging its raw totals would fire every // time the user switched apps. diff --git a/src/renderer/renderer-perf.ts b/src/renderer/renderer-perf.ts index 8d4667cc7..119be45d3 100644 --- a/src/renderer/renderer-perf.ts +++ b/src/renderer/renderer-perf.ts @@ -60,7 +60,8 @@ export function computeFlags(sample: Omit): string[ const flags: string[] = [] if (sample.blockingMs / seconds >= THRESHOLDS.blockingMs) flags.push('blocking') if (sample.longTaskMaxMs >= THRESHOLDS.longTaskMaxMs) flags.push('longtask') - if (sample.reactTotalMs / seconds >= THRESHOLDS.reactTotalMs) flags.push('react') + if (sample.reactProfiling && sample.reactTotalMs / seconds >= THRESHOLDS.reactTotalMs) + flags.push('react') if (sample.slowEventMaxMs >= THRESHOLDS.slowEventMaxMs) flags.push('input') if (sample.heapReclaimedMB >= THRESHOLDS.heapReclaimedMB) flags.push('gc') return flags @@ -99,6 +100,7 @@ class RendererPerf { private slowEventMaxMs = 0 private slowEventName: string | null = null + private reactProfiling = false private reactCommits = 0 private reactTotalMs = 0 private reactMaxMs = 0 @@ -128,6 +130,13 @@ class RendererPerf { this.timer = setInterval(() => this.tick(), BUCKET_MS) } + /** Declared by whichever entry mounted the root . Without it every + * react* field stays 0, and 0 is indistinguishable from an idle app — the + * flag is what lets consumers say "not measured" instead. */ + markReactProfilingEnabled(): void { + this.reactProfiling = true + } + /** Called from the root 's onRender for every React commit. * Counter bumps only — no threshold check, no IPC, nothing that could make * the measurement part of what it measures. */ @@ -204,6 +213,7 @@ class RendererPerf { heapLimitMB: round(heap?.limitMB ?? 0), heapGrowthMB: round(Math.max(0, delta)), heapReclaimedMB: round(Math.max(0, -delta)), + reactProfiling: this.reactProfiling, reactCommits: this.reactCommits, reactTotalMs: round(this.reactTotalMs), reactMaxMs: round(this.reactMaxMs), @@ -281,6 +291,7 @@ function emptySample(): RendererPerfSample { heapLimitMB: 0, heapGrowthMB: 0, heapReclaimedMB: 0, + reactProfiling: false, reactCommits: 0, reactTotalMs: 0, reactMaxMs: 0, diff --git a/src/renderer/store.ts b/src/renderer/store.ts index 212353c7e..8ec4d1570 100644 --- a/src/renderer/store.ts +++ b/src/renderer/store.ts @@ -33,6 +33,7 @@ import { type StateEvent, type WireSnapshotState } from '../shared/state' +import type { PtyStatus } from '../shared/state/terminals' import type { RemoteServerVersion } from '../shared/state/ssh-bootstrap' import type { LocalTransportHandle, BackendConnection } from './types' import type { BootstrapProgress, SshBootstrapState } from '../shared/state/ssh-bootstrap' @@ -880,6 +881,23 @@ export function useTerminalProgress(terminalId: string) { return useAppState((s) => s.terminals.progress[terminalId] ?? null) } +/** Per-terminal status. Same narrowing rationale as useTerminalProgress, + * but this one was load-bearing for a different reason: `statuses` used to + * be threaded whole from App through WorkspaceView → SplitRenderer → + * LeafPane → TerminalPanel, so one terminal changing status allocated a new + * map and re-rendered every mounted worktree's entire subtree. With ~22 + * worktrees mounted at once and a status dispatch per streamed token, that + * was the renderer's dominant commit-phase cost. Read it here, per tab. */ +export function useTerminalStatus(terminalId: string): PtyStatus { + return useAppState((s) => s.terminals.statuses[terminalId] ?? 'idle') +} + +/** Per-terminal shell busy/process-name. Narrowed for the same reason as + * useTerminalStatus — see that comment. */ +export function useShellActivity(terminalId: string) { + return useAppState((s) => s.terminals.shellActivity[terminalId] ?? null) +} + export function useJsonClaude() { return useAppState((s) => s.jsonClaude) } diff --git a/src/renderer/vite-env.d.ts b/src/renderer/vite-env.d.ts index 42f7cd779..60603f6ef 100644 --- a/src/renderer/vite-env.d.ts +++ b/src/renderer/vite-env.d.ts @@ -1,3 +1,4 @@ /// declare const __HARNESS_DEV_BRANCH__: string +declare const __HARNESS_REACT_PROFILING__: boolean diff --git a/src/shared/perf-types.ts b/src/shared/perf-types.ts index 34fec2bc9..d5fee2365 100644 --- a/src/shared/perf-types.ts +++ b/src/shared/perf-types.ts @@ -38,6 +38,10 @@ export interface RendererPerfSample { * value is the only direct evidence of a major GC we can observe from JS. */ heapGrowthMB: number heapReclaimedMB: number + /** False unless the build aliased react-dom/client to react-dom/profiling + * (HARNESS_REACT_PROFILING=1). When false the three react* fields below are + * not measurements — render them as "n/a", never as 0. */ + reactProfiling: boolean reactCommits: number reactTotalMs: number reactMaxMs: number diff --git a/src/web-client/main.tsx b/src/web-client/main.tsx index af79f6d0a..4cab36b9b 100644 --- a/src/web-client/main.tsx +++ b/src/web-client/main.tsx @@ -16,7 +16,6 @@ // transport sits behind the local-backend handle. import '../renderer/styles.css' -import type { ProfilerOnRenderCallback } from 'react' import { WebSocketClientTransport } from '../shared/transport/transport-websocket' declare global { @@ -85,9 +84,8 @@ async function boot(): Promise { // fires on first mount) so the import-order constraint is softer // than it used to be, but we keep dynamic imports here for symmetry // with how the Electron renderer awaits initStore before mount. - const [react, reactDom, appMod, storeMod, monacoMod, metricsMod, errorBoundaryMod] = + const [reactDom, appMod, storeMod, monacoMod, metricsMod, errorBoundaryMod] = await Promise.all([ - import('react'), import('react-dom/client'), import('../renderer/App'), import('../renderer/store'), @@ -103,10 +101,8 @@ async function boot(): Promise { const backendMod = await import('../renderer/backend') rendererPerf.start((sample) => backendMod.getBackend().perfReportRendererSample(sample)) - const onRender: ProfilerOnRenderCallback = (_id, _phase, actualDuration) => { - rendererPerf.recordCommit(actualDuration) - } - + // No here: vite.web.config.ts has no react-dom/profiling alias, so + // onRender could never fire. Samples report reactProfiling:false. const App = appMod.default const ErrorBoundary = errorBoundaryMod.ErrorBoundary @@ -114,9 +110,7 @@ async function boot(): Promise { .createRoot(document.getElementById('root')!) .render( - - - + ) }