From 8354f7ba024b98044d8ea33c7ce63a6fd7cb60d0 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Tue, 28 Jul 2026 23:45:05 -0700 Subject: [PATCH] test(vault): guard the workspace-switch regression #11303 fixed (#11311) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #11303 removed the O(sessions x roots) path normalization from the session worktree map, but nothing fails if that shape comes back. Adds the two checks that were missing, plus the tool that would have caught it. - timing guard: 1200 worktrees x 400 sessions must build in <150ms. Verified fail-first — restoring the pre-#11303 per-session buildWorktreeCandidates call takes it to 238ms; it is ~15ms as merged. - path boundary: '/repo/alpha-sibling' must not be attributed to '/repo/alpha'. Hoisting the root normalization out of the loop must not degrade containment into a bare startsWith. Also adds tools/benchmarks/workspace-switch-paint-latency.mjs, which attaches over CDP and measures first-paint-after-click and max frame gap. The existing worktree-switch-responsiveness.spec.ts only times the synchronous click task, which stays ~1ms because the highlight is a direct DOM mutation — that is why a ~1s stall could ship without tripping a budget. On the affected build it read maxFrameGap p50=973ms. (cherry picked from commit c5102e1262ca2550fba84c807a6e452484a700ce) --- .../ai-vault-session-worktree-map.test.tsx | 49 +++ .../workspace-switch-paint-latency.mjs | 398 ++++++++++++++++++ 2 files changed, 447 insertions(+) create mode 100644 tools/benchmarks/workspace-switch-paint-latency.mjs diff --git a/src/renderer/src/components/right-sidebar/ai-vault-session-worktree-map.test.tsx b/src/renderer/src/components/right-sidebar/ai-vault-session-worktree-map.test.tsx index aae0ad8d32a..90ea7cd900c 100644 --- a/src/renderer/src/components/right-sidebar/ai-vault-session-worktree-map.test.tsx +++ b/src/renderer/src/components/right-sidebar/ai-vault-session-worktree-map.test.tsx @@ -184,4 +184,53 @@ describe('useAiVaultSessionWorktreeMap', () => { worktreeId: cjk.id }) }) + + it('does not attribute a session to a workspace that merely shares a path prefix', () => { + // Guards the candidate matcher's boundary: hoisting the root normalization + // out of the loop must not degrade containment into a bare startsWith, or + // '/repo/alpha-sibling' would be swallowed by '/repo/alpha'. + const sibling = makeWorktree({ + id: 'repo-1::/repo/alpha-sibling', + path: '/repo/alpha-sibling' + }) + const session = makeSession({ id: 'codex:sibling', cwd: '/repo/alpha-sibling/src' }) + + const { result } = renderHook(() => + useAiVaultSessionWorktreeMap({ + sessions: [session], + repos, + worktrees: [worktreeA, sibling] + }) + ) + + expect(result.current.get(session.id)).toMatchObject({ worktreeId: sibling.id }) + }) + + it('builds the map at scale without re-normalizing paths per session', () => { + // Regression for the ~1s workspace-switch stall (#10841/#11303): the map + // used to rebuild and re-normalize every candidate root per session, making + // it O(sessions x roots) on path normalization. Times the real fan-out + // rather than counting calls, so it fails on any return to that shape. + const manyWorktrees = Array.from({ length: 1200 }, (_, i) => + makeWorktree({ id: `repo-1::/repo/w${i}`, path: `/repo/w${i}` }) + ) + const manySessions = Array.from({ length: 400 }, (_, i) => + makeSession({ id: `codex:s${i}`, cwd: `/repo/w${i % manyWorktrees.length}/src` }) + ) + + const startedAt = performance.now() + const { result } = renderHook(() => + useAiVaultSessionWorktreeMap({ + sessions: manySessions, + repos, + worktrees: manyWorktrees + }) + ) + const elapsedMs = performance.now() - startedAt + + expect(result.current.size).toBe(manySessions.length) + // Was ~350ms+ before the hoist and ~15ms after; the bound is loose enough + // to stay quiet on a slow CI box while still catching the quadratic shape. + expect(elapsedMs).toBeLessThan(150) + }) }) diff --git a/tools/benchmarks/workspace-switch-paint-latency.mjs b/tools/benchmarks/workspace-switch-paint-latency.mjs new file mode 100644 index 00000000000..200c0053b8c --- /dev/null +++ b/tools/benchmarks/workspace-switch-paint-latency.mjs @@ -0,0 +1,398 @@ +#!/usr/bin/env node +/** + * Measures how long a workspace-card click takes to become VISIBLE. + * + * The existing e2e spec (tests/e2e/worktree-switch-responsiveness.spec.ts) only + * measures the synchronous click task. That stays fast even when the symptom is + * present: `markSidebarWorktreeActiveImmediately` mutates the DOM in the click + * handler, so the attribute flips in ~1ms. What the user sees is the next + * painted FRAME, and that frame is blocked when the async activation path jams + * the main thread. So the number that matters is first-paint-after-click. + * + * Attaches over CDP to an already-running dev app: + * + * ORCA_DEV_USER_DATA_PATH=... REMOTE_DEBUGGING_PORT=9455 pn dev + * node tools/benchmarks/workspace-switch-paint-latency.mjs --port 9455 + * + * Exits non-zero when the median first-paint exceeds --budget (default 200ms), + * which makes it usable directly as a `git bisect run` predicate. + */ + +import { writeFileSync, mkdirSync } from 'node:fs' +import path from 'node:path' +import { chromium } from '@stablyai/playwright-test' + +function parseArgs(argv) { + const args = { + port: 9455, + switches: 6, + budgetMs: 200, + settleMs: 2500, + out: null, + json: false + } + for (let i = 0; i < argv.length; i += 1) { + const [flag, inlineValue] = argv[i].split('=') + const value = inlineValue ?? argv[i + 1] + const consume = () => { + if (inlineValue === undefined) { + i += 1 + } + } + switch (flag) { + case '--port': + args.port = Number(value) + consume() + break + case '--switches': + args.switches = Number(value) + consume() + break + case '--budget': + args.budgetMs = Number(value) + consume() + break + case '--settle': + args.settleMs = Number(value) + consume() + break + case '--out': + args.out = value + consume() + break + case '--json': + args.json = true + break + case '--help': + console.log( + 'Usage: workspace-switch-paint-latency.mjs [--port 9455] [--switches 6] [--budget 200] [--settle 2500] [--out file.json] [--json]' + ) + process.exit(0) + break + default: + break + } + } + return args +} + +const args = parseArgs(process.argv.slice(2)) + +function quantile(sorted, q) { + if (sorted.length === 0) { + return null + } + const pos = (sorted.length - 1) * q + const lo = Math.floor(pos) + const hi = Math.ceil(pos) + if (lo === hi) { + return sorted[lo] + } + return sorted[lo] + (sorted[hi] - sorted[lo]) * (pos - lo) +} + +function summarize(values) { + const clean = values.filter((v) => typeof v === 'number' && Number.isFinite(v)) + if (clean.length === 0) { + return null + } + const sorted = [...clean].sort((a, b) => a - b) + return { + n: sorted.length, + min: +sorted[0].toFixed(1), + p50: +quantile(sorted, 0.5).toFixed(1), + p90: +quantile(sorted, 0.9).toFixed(1), + max: +sorted.at(-1).toFixed(1) + } +} + +/** + * Installed in the renderer before each click. Records, from the real + * pointerdown, every animation frame plus long tasks, so we can tell a jammed + * main thread (no frames for ~1s) apart from slow-but-smooth work. + */ +const PROBE_SOURCE = `(targetId) => { + const probe = { + t0: null, + targetId, + frames: [], + longTasks: [], + attrFlipMs: null, + storeCommitMs: null, + renderedCommitMs: null, + done: false + } + window.__switchProbe = probe + + const surfaceOf = (id) => { + const row = [...document.querySelectorAll('[data-worktree-id]')].find( + (el) => el.dataset.worktreeId === id + ) + return row ? row.querySelector('[data-worktree-card-surface]') : null + } + const isActive = (id) => { + const s = surfaceOf(id) + return Boolean(s && s.getAttribute('data-worktree-card-active')) + } + const renderedId = () => + document + .querySelector('[data-rendered-active-worktree-id]') + ?.getAttribute('data-rendered-active-worktree-id') ?? null + + try { + const po = new PerformanceObserver((list) => { + for (const e of list.getEntries()) { + if (probe.t0 === null) continue + probe.longTasks.push({ + startMs: +(e.startTime - probe.t0).toFixed(1), + durationMs: +e.duration.toFixed(1) + }) + } + }) + po.observe({ entryTypes: ['longtask'] }) + probe.observer = po + } catch {} + + // Why: pointerdown is the earliest thing the user's click produces, so it is + // the honest zero point for "time until I saw something happen". + const onPointerDown = () => { + if (probe.t0 !== null) return + probe.t0 = performance.now() + const tick = () => { + const now = performance.now() - probe.t0 + probe.frames.push(+now.toFixed(1)) + if (probe.attrFlipMs === null && isActive(probe.targetId)) probe.attrFlipMs = +now.toFixed(1) + if (probe.storeCommitMs === null) { + const st = window.__store?.getState?.() + if (st && st.activeWorktreeId === probe.targetId) probe.storeCommitMs = +now.toFixed(1) + } + if (probe.renderedCommitMs === null && renderedId() === probe.targetId) { + probe.renderedCommitMs = +now.toFixed(1) + } + if (!probe.done) requestAnimationFrame(tick) + } + requestAnimationFrame(tick) + } + window.addEventListener('pointerdown', onPointerDown, { capture: true, once: true }) + probe.cleanup = () => { + probe.done = true + try { probe.observer?.disconnect() } catch {} + window.removeEventListener('pointerdown', onPointerDown, { capture: true }) + } + return true +}` + +/** Press Escape while any full-screen overlay is intercepting pointer events. */ +async function dismissOverlays(page) { + for (let attempt = 0; attempt < 3; attempt += 1) { + const blocked = await page.evaluate(() => + [...document.querySelectorAll('body *')].some((el) => { + const r = el.getBoundingClientRect() + if (r.width < window.innerWidth * 0.9 || r.height < window.innerHeight * 0.9) { + return false + } + const s = getComputedStyle(el) + return s.position === 'fixed' && s.pointerEvents !== 'none' && Number(s.zIndex) >= 40 + }) + ) + if (!blocked) { + return + } + await page.keyboard.press('Escape') + await page.waitForTimeout(200) + } +} + +async function main() { + const browser = await chromium.connectOverCDP(`http://127.0.0.1:${args.port}`) + const contexts = browser.contexts() + const pages = contexts.flatMap((c) => c.pages()) + let page = null + for (const candidate of pages) { + const hasStore = await candidate + .evaluate( + () => Boolean(window.__store) && Boolean(document.querySelector('[data-worktree-id]')) + ) + .catch(() => false) + if (hasStore) { + page = candidate + break + } + } + if (!page) { + throw new Error( + 'No renderer page with window.__store + a mounted sidebar. Is the app past startup, and is the sidebar open?' + ) + } + + // Normalize sidebar state so the same cards are reachable across commits. + // Why pin the sort: 'recent' reorders the list after every switch, which + // would move the card out from under the cursor mid-run. + const worktreeIds = await page.evaluate(() => { + const state = window.__store.getState() + state.setActiveView('terminal') + state.setSidebarOpen(true) + state.setGroupBy?.('none') + state.setSortBy?.('name') + state.setShowActiveOnly?.(false) + state.setFilterRepoIds?.([]) + const viewportH = window.innerHeight + // Only cards fully inside the viewport are clickable: the list is + // virtualized, so off-screen rows are unmounted or positioned outside. + return [...document.querySelectorAll('[data-worktree-id]')] + .filter((el) => { + const surface = el.querySelector('[data-worktree-card-surface]') + if (!surface) { + return false + } + const r = surface.getBoundingClientRect() + return r.height > 0 && r.top >= 0 && r.bottom <= viewportH + }) + .map((el) => el.dataset.worktreeId) + .filter((id, i, all) => id && all.indexOf(id) === i) + }) + + if (worktreeIds.length < 2) { + throw new Error( + `Need >=2 workspace cards visible in the viewport, found ${worktreeIds.length}. Is the sidebar open?` + ) + } + + // Alternate between two cards, neither of which is the one already active at + // start — clicking the active card short-circuits and measures nothing. + const activeAtStart = await page.evaluate(() => window.__store.getState().activeWorktreeId) + const pair = worktreeIds.filter((id) => id !== activeAtStart).slice(0, 2) + if (pair.length < 2) { + throw new Error('Need two visible non-active cards to alternate between') + } + console.error(`alternating between:\n ${pair[0]}\n ${pair[1]}`) + + const samples = [] + for (let i = 0; i < args.switches; i += 1) { + const targetId = pair[i % 2] + + // Why: page.evaluate() treats a string as an expression to evaluate and + // drops the argument, so the probe has to be applied inline. + const surface = page + .locator(`[data-worktree-id="${targetId}"] [data-worktree-card-surface]`) + .first() + + // Why: a transient popover (`fixed inset-0 z-50`) swallows the click, and + // the run would then measure a click that never reached the card. + await dismissOverlays(page) + + await page.evaluate(`(${PROBE_SOURCE})(${JSON.stringify(targetId)})`) + + // Real CDP input events, not element.click(): the click path has + // pointer-drag suppression that a synthetic click would bypass. Going + // through the locator also re-resolves position at click time, which + // matters because the virtualized list reorders between switches. + try { + await surface.click({ timeout: 10_000 }) + } catch (error) { + console.error(`skip switch ${i + 1}: ${String(error).split('\n')[0]}`) + continue + } + + await page.waitForTimeout(args.settleMs) + + const sample = await page.evaluate(() => { + const p = window.__switchProbe + p.cleanup?.() + const frames = p.frames + let maxGap = 0 + let prev = 0 + for (const f of frames) { + maxGap = Math.max(maxGap, f - prev) + prev = f + } + return { + firstPaintMs: frames.length ? frames[0] : null, + maxFrameGapMs: +maxGap.toFixed(1), + frameCount: frames.length, + attrFlipMs: p.attrFlipMs, + storeCommitMs: p.storeCommitMs, + renderedCommitMs: p.renderedCommitMs, + longTaskCount: p.longTasks.length, + longTaskTotalMs: +p.longTasks.reduce((a, t) => a + t.durationMs, 0).toFixed(1), + worstLongTaskMs: p.longTasks.reduce((a, t) => Math.max(a, t.durationMs), 0) + } + }) + sample.targetId = targetId + // Why: a click that never activated would report a fast "first paint" and + // silently make every commit look good during a bisect. + if (sample.attrFlipMs === null && sample.renderedCommitMs === null) { + throw new Error( + `switch ${i + 1} clicked card ${targetId} but it never became active — the measurement is not valid` + ) + } + samples.push(sample) + console.error( + `switch ${i + 1}/${args.switches} -> firstPaint=${sample.firstPaintMs}ms ` + + `maxFrameGap=${sample.maxFrameGapMs}ms attrFlip=${sample.attrFlipMs}ms ` + + `rendered=${sample.renderedCommitMs}ms longTasks=${sample.longTaskCount}/${sample.longTaskTotalMs}ms` + ) + } + + // Why: the first switch after attach pays one-off lazy work (chunk loads, + // cold caches) that a warm app never pays again. Keep it in the raw samples + // but exclude it from the verdict so bisect steps compare like with like. + const scored = samples.length > 1 ? samples.slice(1) : samples + + const report = { + port: args.port, + switches: samples.length, + budgetMs: args.budgetMs, + firstPaintMs: summarize(scored.map((s) => s.firstPaintMs)), + maxFrameGapMs: summarize(scored.map((s) => s.maxFrameGapMs)), + attrFlipMs: summarize(scored.map((s) => s.attrFlipMs)), + renderedCommitMs: summarize(scored.map((s) => s.renderedCommitMs)), + worstLongTaskMs: summarize(scored.map((s) => s.worstLongTaskMs)), + longTaskTotalMs: summarize(scored.map((s) => s.longTaskTotalMs)), + samples + } + + const medianFirstPaint = report.firstPaintMs?.p50 ?? Infinity + const medianFrameGap = report.maxFrameGapMs?.p50 ?? Infinity + // The visible symptom is a stall, which shows up either as a late first frame + // or as a long gap between frames right after the click. + const verdictMs = Math.max(medianFirstPaint, medianFrameGap) + report.verdictMs = +verdictMs.toFixed(1) + report.pass = verdictMs <= args.budgetMs + + if (args.out) { + mkdirSync(path.dirname(args.out), { recursive: true }) + writeFileSync(args.out, JSON.stringify(report, null, 2)) + } + if (args.json) { + console.log(JSON.stringify(report, null, 2)) + } else { + console.log('') + console.log( + `first paint after click p50=${report.firstPaintMs?.p50}ms max=${report.firstPaintMs?.max}ms` + ) + console.log( + `max frame gap p50=${report.maxFrameGapMs?.p50}ms max=${report.maxFrameGapMs?.max}ms` + ) + console.log(`attribute flip (JS) p50=${report.attrFlipMs?.p50}ms`) + console.log( + `main pane rendered p50=${report.renderedCommitMs?.p50}ms max=${report.renderedCommitMs?.max}ms` + ) + console.log( + `worst long task p50=${report.worstLongTaskMs?.p50}ms max=${report.worstLongTaskMs?.max}ms` + ) + console.log('') + console.log( + `${report.pass ? 'PASS' : 'FAIL'} verdict=${report.verdictMs}ms budget=${args.budgetMs}ms` + ) + } + + await browser.close() + process.exit(report.pass ? 0 : 1) +} + +main().catch((error) => { + console.error(error?.stack ?? String(error)) + // 125 tells `git bisect run` to skip rather than mark the commit bad. + process.exit(125) +})