From 705899b6cbbf3fc0a5a5b805ea2697991215a783 Mon Sep 17 00:00:00 2001 From: Max Lampert Date: Mon, 24 Aug 2026 17:46:36 -0700 Subject: [PATCH] fix(desktop): guard-gated traces, on-disk log cap, honest projects benchmark MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address the three findings from the 2026-08-24 re-review of #6455, plus one finding from a follow-up adversarial review: - The switch trace now opens inside commitGuardedNavigation, only after the navigation guard accepts — a refused click can no longer leave an orphan trace that a later history navigation would settle with inflated time. The commit flow is extracted and dependency-injected; new unit tests pin guard ordering and the refusal→history-back case. - The perf-log 4 KiB cap now bounds bytes on disk: one byte is reserved for the newline writeln! appends. New boundary test measures the file. - The channel↔Projects benchmark gates readiness on a new data-projects-hydrating marker (driven by the surface's query fan via isLoading, so disabled queries never wedge it) instead of the shell header, verifies member inflation on both inflated channels, uses a prefixed ready-selector for general, and reports each switch direction as its own median so a one-leg regression cannot hide. - A settle wait that hits its 5s deadline while the render is still pending now records with settleWaitTruncated instead of posing as an honest settled paint — the >deadline tail is what the tracer exists to expose. Also removes the dead ChannelSwitchFetchTrace type. Co-Authored-By: Claude Fable 5 Signed-off-by: Max Lampert --- desktop/src-tauri/src/commands/perf_log.rs | 36 +++- .../commitGuardedNavigation.test.mjs | 169 ++++++++++++++++++ .../app/navigation/commitGuardedNavigation.ts | 51 ++++++ .../src/app/navigation/useAppNavigation.ts | 66 ++++--- .../src/features/projects/ui/ProjectsView.tsx | 10 ++ .../src/shared/lib/channelSwitchPerf.test.mjs | 27 +++ desktop/src/shared/lib/channelSwitchPerf.ts | 53 ++++-- desktop/tests/e2e/member-heavy-switch.perf.ts | 114 ++++++++---- 8 files changed, 441 insertions(+), 85 deletions(-) create mode 100644 desktop/src/app/navigation/commitGuardedNavigation.test.mjs create mode 100644 desktop/src/app/navigation/commitGuardedNavigation.ts diff --git a/desktop/src-tauri/src/commands/perf_log.rs b/desktop/src-tauri/src/commands/perf_log.rs index 625e8db88c..5126639f4a 100644 --- a/desktop/src-tauri/src/commands/perf_log.rs +++ b/desktop/src-tauri/src/commands/perf_log.rs @@ -75,8 +75,9 @@ fn shape_perf_log_line( } let line = serde_json::to_string(&value).map_err(|e| e.to_string())?; // The cap must hold for what actually reaches the disk: gitSha and label - // are folded in after the record-size check above. - if line.len() > MAX_RECORD_BYTES { + // are folded in after the record-size check above, and writeln! appends + // a newline terminator — reserve one byte for it. + if line.len() + 1 > MAX_RECORD_BYTES { return Err("perf log line too large".to_string()); } Ok(line) @@ -217,6 +218,37 @@ mod tests { assert!(shape_perf_log_line(&record, Some(&"s".repeat(64)), None).is_err()); } + #[test] + fn the_cap_bounds_bytes_on_disk_including_the_newline() { + // Shaped line = {"pad":"…","gitSha":null} → pad length + 24 bytes. + // The largest accepted line is MAX_RECORD_BYTES - 1: writeln! appends + // a newline, and the cap bounds what reaches the disk. + let fits = format!(r#"{{"pad":"{}"}}"#, "x".repeat(MAX_RECORD_BYTES - 25)); + let line = shape_perf_log_line(&fits, None, None).expect("one byte reserved for newline"); + assert_eq!(line.len(), MAX_RECORD_BYTES - 1); + + let dir = std::env::temp_dir().join(format!( + "perf-log-newline-{}-{:?}", + std::process::id(), + std::thread::current().id() + )); + std::fs::create_dir_all(&dir).expect("tempdir"); + let path = dir.join("switch-perf.jsonl"); + let _ = std::fs::remove_file(&path); + append_line_rotating(&path, &line, MAX_LOG_BYTES).expect("append"); + assert_eq!( + std::fs::metadata(&path).expect("metadata").len(), + MAX_RECORD_BYTES as u64, + "on-disk record must not exceed the cap" + ); + std::fs::remove_dir_all(&dir).ok(); + + // One pad byte more serializes to exactly MAX_RECORD_BYTES, which + // would write MAX_RECORD_BYTES + 1 bytes — rejected. + let over = format!(r#"{{"pad":"{}"}}"#, "x".repeat(MAX_RECORD_BYTES - 24)); + assert!(shape_perf_log_line(&over, None, None).is_err()); + } + #[test] fn append_rotates_once_over_the_cap_and_keeps_one_generation() { let dir = std::env::temp_dir().join(format!("perf-log-test-{}", std::process::id())); diff --git a/desktop/src/app/navigation/commitGuardedNavigation.test.mjs b/desktop/src/app/navigation/commitGuardedNavigation.test.mjs new file mode 100644 index 0000000000..99ab2084bc --- /dev/null +++ b/desktop/src/app/navigation/commitGuardedNavigation.test.mjs @@ -0,0 +1,169 @@ +import assert from "node:assert/strict"; +import test from "node:test"; + +import { commitGuardedNavigation } from "./commitGuardedNavigation.ts"; +import { registerNavigationGuard } from "./navigationGuard.ts"; +import { + CHANNEL_SWITCH_MEASURE, + resetChannelSwitchTrace, + settleChannelSwitchTrace, +} from "../../shared/lib/channelSwitchPerf.ts"; + +const route = (href) => ({ kind: "route", href }); + +test("a refused navigation opens no trace; a later history settle records nothing", async () => { + // Frame-queue stub so the settle's rAF chain can be drained synchronously. + const frames = []; + const originalWindow = globalThis.window; + const originalDocument = globalThis.document; + globalThis.window = { + requestAnimationFrame: (cb) => frames.push(cb) && frames.length, + cancelAnimationFrame: () => {}, + }; + globalThis.document = { querySelector: () => null }; + performance.clearMeasures?.(CHANNEL_SWITCH_MEASURE); + const unregister = registerNavigationGuard(() => false); + try { + let navigated = false; + const committed = await commitGuardedNavigation({ + currentHref: "/channels/aaaa", + nextHref: "/channels/bbbb", + guardedTarget: route("/channels/bbbb"), + traceChannelId: "bbbb", + navigate: async () => { + navigated = true; + }, + }); + assert.equal(committed, false); + assert.equal(navigated, false); + + // Browser Back into the refused channel (history navigation is + // deliberately untraced): its mount settles, and must find NO orphan + // trace from the refused click — otherwise the measure would span the + // refusal and everything the user did in between. + settleChannelSwitchTrace("bbbb"); + for (let i = 0; i < 20 && frames.length > 0; i += 1) { + for (const cb of frames.splice(0, frames.length)) cb(); + } + assert.deepEqual( + performance + .getEntriesByName(CHANNEL_SWITCH_MEASURE) + .map((entry) => entry.detail?.channelId), + [], + ); + } finally { + unregister(); + resetChannelSwitchTrace(); + performance.clearMeasures?.(CHANNEL_SWITCH_MEASURE); + if (originalWindow === undefined) delete globalThis.window; + else globalThis.window = originalWindow; + if (originalDocument === undefined) delete globalThis.document; + else globalThis.document = originalDocument; + } +}); + +test("an accepted navigation opens the trace after the guard, before navigate", async () => { + const order = []; + const committed = await commitGuardedNavigation( + { + currentHref: "/channels/aaaa", + nextHref: "/channels/bbbb", + guardedTarget: route("/channels/bbbb"), + traceChannelId: "bbbb", + navigate: async () => { + order.push("navigate"); + }, + }, + { + allow: () => { + order.push("guard"); + return true; + }, + beginTrace: (channelId) => { + order.push(`begin:${channelId}`); + }, + }, + ); + assert.equal(committed, true); + assert.deepEqual(order, ["guard", "begin:bbbb", "navigate"]); +}); + +test("a same-destination no-op consults neither the guard nor the trace", async () => { + const order = []; + const committed = await commitGuardedNavigation( + { + currentHref: "/channels/aaaa", + nextHref: "/channels/aaaa", + guardedTarget: route("/channels/aaaa"), + traceChannelId: "aaaa", + navigate: async () => { + order.push("navigate"); + }, + }, + { + allow: () => { + order.push("guard"); + return true; + }, + beginTrace: () => { + order.push("begin"); + }, + }, + ); + assert.equal(committed, false); + assert.deepEqual(order, []); +}); + +test("force overrides the same-destination no-op but still runs the guard first", async () => { + const order = []; + const committed = await commitGuardedNavigation( + { + currentHref: "/channels/aaaa", + nextHref: "/channels/aaaa", + force: true, + guardedTarget: route("/channels/aaaa"), + navigate: async () => { + order.push("navigate"); + }, + }, + { + allow: () => { + order.push("guard"); + return true; + }, + beginTrace: () => { + order.push("begin"); + }, + }, + ); + assert.equal(committed, true); + // No traceChannelId: forced re-selection of the active channel stays + // untraced (nothing would settle it). + assert.deepEqual(order, ["guard", "navigate"]); +}); + +test("a same-destination navigation carrying router state still commits", async () => { + const order = []; + const committed = await commitGuardedNavigation( + { + currentHref: "/channels/aaaa", + nextHref: "/channels/aaaa", + guardedTarget: route("/channels/aaaa"), + hasStateUpdate: true, + navigate: async () => { + order.push("navigate"); + }, + }, + { + allow: () => { + order.push("guard"); + return true; + }, + beginTrace: () => { + order.push("begin"); + }, + }, + ); + assert.equal(committed, true); + assert.deepEqual(order, ["guard", "navigate"]); +}); diff --git a/desktop/src/app/navigation/commitGuardedNavigation.ts b/desktop/src/app/navigation/commitGuardedNavigation.ts new file mode 100644 index 0000000000..9b9f69d6a2 --- /dev/null +++ b/desktop/src/app/navigation/commitGuardedNavigation.ts @@ -0,0 +1,51 @@ +import { + allowNavigation, + type GuardedNavigation, +} from "@/app/navigation/navigationGuard"; +import { beginChannelSwitchTrace } from "@/shared/lib/channelSwitchPerf"; + +/** + * commitGuardedNavigation runs the shared commit flow for app navigations: + * skip same-destination no-ops (unless forced or carrying a router-state + * update), consult the navigation guard, then navigate. A same-href + * navigation that writes state — e.g. setting or clearing the search + * highlight — must still commit, or the state never lands. When + * `traceChannelId` is set, the channel-switch + * trace opens only after the guard accepts — a refused click must not leave + * an orphan trace that a later history navigation (deliberately untraced) + * would settle with the refused click's inflated wall time. Returns whether + * the navigation was performed. `deps` exists for unit tests. + */ +export async function commitGuardedNavigation( + input: { + currentHref: string; + nextHref: string; + force?: boolean; + guardedTarget: GuardedNavigation; + hasStateUpdate?: boolean; + traceChannelId?: string; + navigate: () => Promise; + }, + deps: { + allow?: typeof allowNavigation; + beginTrace?: typeof beginChannelSwitchTrace; + } = {}, +): Promise { + const allow = deps.allow ?? allowNavigation; + const beginTrace = deps.beginTrace ?? beginChannelSwitchTrace; + if ( + input.currentHref === input.nextHref && + !input.force && + !input.hasStateUpdate + ) { + return false; + } + if (!allow(input.guardedTarget)) { + return false; + } + if (input.traceChannelId !== undefined) { + beginTrace(input.traceChannelId); + } + await input.navigate(); + return true; +} diff --git a/desktop/src/app/navigation/useAppNavigation.ts b/desktop/src/app/navigation/useAppNavigation.ts index ac2b16a78c..7ad42560d7 100644 --- a/desktop/src/app/navigation/useAppNavigation.ts +++ b/desktop/src/app/navigation/useAppNavigation.ts @@ -6,15 +6,14 @@ import { useRouter, } from "@tanstack/react-router"; +import { commitGuardedNavigation } from "@/app/navigation/commitGuardedNavigation"; import type { SearchHighlightNavigation } from "@/app/navigation/searchHighlightNavigation"; import { openSearchHitWithNavigation } from "@/app/navigation/searchHitNavigation"; import { - allowNavigation, type GuardedNavigation, traverseHistory, } from "@/app/navigation/navigationGuard"; import type { SearchHit } from "@/shared/api/types"; -import { beginChannelSwitchTrace } from "@/shared/lib/channelSwitchPerf"; type NavigationBehavior = { force?: boolean; @@ -42,32 +41,26 @@ export function useAppNavigation() { }, behavior: NavigationBehavior = {}, guardedTarget?: GuardedNavigation, + traceChannelId?: string, ) => { const nextLocation = router.buildLocation(next as never); - const hasStateUpdate = next.state !== undefined; - - if ( - location.href === nextLocation.href && - !behavior.force && - !hasStateUpdate - ) { - return false; - } - - if ( - !allowNavigation( - guardedTarget ?? { kind: "route", href: nextLocation.href }, - ) - ) { - return false; - } - - await navigate({ - ...next, - replace: behavior.replace, - resetScroll: behavior.resetScroll, - } as never); - return true; + return commitGuardedNavigation({ + currentHref: location.href, + force: behavior.force, + guardedTarget: guardedTarget ?? { + kind: "route", + href: nextLocation.href, + }, + hasStateUpdate: next.state !== undefined, + navigate: () => + navigate({ + ...next, + replace: behavior.replace, + resetScroll: behavior.resetScroll, + } as never), + nextHref: nextLocation.href, + traceChannelId, + }); }, [location.href, navigate, router], ); @@ -287,16 +280,6 @@ export function useAppNavigation() { threadRootId?: string | null; }, ) => { - // Every channel navigation entry point funnels through here, so this - // is the single click-time anchor for the switch trace. Re-selecting - // the already-active channel is a no-op navigation: the channel's - // effects never rerun, nothing would settle the trace, and it would - // squat on the singleton until timeout — so don't open one. (History - // back/forward bypasses goChannel entirely and is deliberately - // untraced.) - if (!location.pathname.endsWith(`/channels/${channelId}`)) { - beginChannelSwitchTrace(channelId); - } return commitNavigation( { to: "/channels/$channelId", @@ -336,6 +319,17 @@ export function useAppNavigation() { threadRootId: options.threadRootId ?? null, } : undefined, + // Every channel navigation entry point funnels through here, so this + // is the single click-time anchor for the switch trace; it opens + // inside commitGuardedNavigation only after the navigation guard + // accepts. Re-selecting the already-active channel is a no-op + // navigation: the channel's effects never rerun, nothing would + // settle the trace, and it would squat on the singleton until + // timeout — so don't open one. (History back/forward bypasses + // goChannel entirely and is deliberately untraced.) + location.pathname.endsWith(`/channels/${channelId}`) + ? undefined + : channelId, ); }, [commitNavigation, location.pathname], diff --git a/desktop/src/features/projects/ui/ProjectsView.tsx b/desktop/src/features/projects/ui/ProjectsView.tsx index f33659bcfc..6b85ef46b4 100644 --- a/desktop/src/features/projects/ui/ProjectsView.tsx +++ b/desktop/src/features/projects/ui/ProjectsView.tsx @@ -172,6 +172,15 @@ export function ProjectsView() { snapshotProjects, activeCommunity?.reposDir, ); + // Hydration marker for the switch-perf harness: the shell commits long + // before the query fan loads. isLoading so disabled queries never wedge it. + const projectsHydrating = + projectsQuery.isLoading || + projectsWorkItemsQuery.isLoading || + repoSnapshotsQuery.isLoading || + activitySummariesQuery.isLoading || + repositoryActivitySummariesQuery.isLoading || + localRepositoriesQuery.isLoading; const memberChannelIds = useMemberChannelIds(); const repositoryUnavailableReasonFor = useRepositoryUnavailableReasonFor( repoSnapshotsQuery.data?.unavailable, @@ -722,6 +731,7 @@ export function ProjectsView() { data-project-context-detached={ isNarrowProjectsLayout ? undefined : "true" } + data-projects-hydrating={projectsHydrating ? "true" : undefined} data-testid="projects-overview-layout" > { ); }); +test("the settle wait records truncated — never as an honest settle — at deadline", () => { + // Still pending, before the deadline: keep waiting. + assert.equal(resolveSettleWait(4_999, 5_000, true), "wait"); + // Render caught up: record cleanly. + assert.deepEqual(resolveSettleWait(1_000, 5_000, false), { + settleWaitTruncated: false, + }); + // Deadline expired while still pending: the record must say so — a >5s + // switch reported as an ordinary settle would hide exactly the tail this + // tracer exists to expose. + assert.deepEqual(resolveSettleWait(5_000, 5_000, true), { + settleWaitTruncated: true, + }); +}); + +test("a truncated settle is flagged in the summary and the log record", () => { + const summary = summarizeChannelSwitchTrace(trace(), 1_412, true); + assert.ok(summary.endsWith(" settle=truncated"), summary); + const record = buildSwitchPerfLogRecord(trace(), 1_412, true); + assert.equal(record.settleWaitTruncated, true); + // Clean settles keep the field out of the line entirely. + assert.ok( + !("settleWaitTruncated" in buildSwitchPerfLogRecord(trace(), 1_412)), + ); +}); + test("fetches attribute only when started after the switch began", () => { const active = trace({ channelId: "abcdef1234567890", startedAt: 1_000 }); // Started before the switch (stale A→B→A leg): not attributable. diff --git a/desktop/src/shared/lib/channelSwitchPerf.ts b/desktop/src/shared/lib/channelSwitchPerf.ts index 5007e7a76f..0daa46890e 100644 --- a/desktop/src/shared/lib/channelSwitchPerf.ts +++ b/desktop/src/shared/lib/channelSwitchPerf.ts @@ -19,12 +19,6 @@ * `members=cache` — by design, not omission. */ -export type ChannelSwitchFetchTrace = { - durationMs: number; - eventCount?: number; - memberCount?: number; -}; - import { invoke, isTauri } from "@tauri-apps/api/core"; export type ChannelSwitchTrace = { @@ -48,6 +42,7 @@ let activeTrace: ChannelSwitchTrace | null = null; export function summarizeChannelSwitchTrace( trace: ChannelSwitchTrace, settledAt: number, + settleWaitTruncated = false, ): string { const total = Math.round(settledAt - trace.startedAt); const commit = @@ -64,7 +59,8 @@ export function summarizeChannelSwitchTrace( : `${trace.membersFetch.memberCount} members in ${Math.round(trace.membersFetch.durationMs)}ms`; return ( `[switch-perf] channel=${trace.channelId.slice(0, 8)} total=${total}ms ` + - `commit=${commit} window=${window} members=${members}` + `commit=${commit} window=${window} members=${members}` + + (settleWaitTruncated ? " settle=truncated" : "") ); } @@ -76,6 +72,7 @@ export function summarizeChannelSwitchTrace( export function buildSwitchPerfLogRecord( trace: ChannelSwitchTrace, settledAt: number, + settleWaitTruncated = false, ): { ts: string; channelId: string; @@ -83,8 +80,10 @@ export function buildSwitchPerfLogRecord( commitOffsetMs: number | null; windowFetch: { durationMs: number; eventCount: number } | null; membersFetch: { durationMs: number; memberCount: number } | null; + settleWaitTruncated?: true; } { return { + ...(settleWaitTruncated ? { settleWaitTruncated: true as const } : {}), ts: new Date().toISOString(), channelId: trace.channelId, totalMs: Math.round(settledAt - trace.startedAt), @@ -258,6 +257,23 @@ export function resetChannelSwitchTrace(): void { /** Bound on waiting for the deferred timeline commit before recording. */ const SETTLE_RENDER_WAIT_MS = 5_000; +/** + * Per-frame decision for the bounded settle wait: keep waiting only while + * the deferred render is still pending AND the deadline hasn't passed. When + * the wait ends with the render still pending, the record must say so — the + * >deadline tail is exactly what this tracer exists to expose, so the + * measurement is kept but flagged rather than posing as an honest settled + * paint. Pure for unit testing. + */ +export function resolveSettleWait( + now: number, + waitDeadline: number, + renderPending: boolean, +): "wait" | { settleWaitTruncated: boolean } { + if (renderPending && now < waitDeadline) return "wait"; + return { settleWaitTruncated: renderPending }; +} + /** * Closes the active trace once the settled frame has painted. The timeline * renders rows through a deferred snapshot that exposes @@ -287,7 +303,7 @@ export function settleChannelSwitchTrace(channelId: string): void { // finish inside the measured window still attribute to it. It is released // when the record lands; a newer switch's begin() simply replaces it. const waitDeadline = performance.now() + SETTLE_RENDER_WAIT_MS; - const record = () => { + const record = (settleWaitTruncated: boolean) => { const settledAt = performance.now(); if (activeTrace === trace) activeTrace = null; performance.mark(CHANNEL_SWITCH_SETTLED_MARK, { @@ -299,12 +315,17 @@ export function settleChannelSwitchTrace(channelId: string): void { routeCommitAt: trace.routeCommitAt, windowFetch: trace.windowFetch, membersFetch: trace.membersFetch, + ...(settleWaitTruncated ? { settleWaitTruncated: true } : {}), }, start: trace.startedAt, end: settledAt, }); - console.info(summarizeChannelSwitchTrace(trace, settledAt)); - appendSwitchPerfLogRecord(buildSwitchPerfLogRecord(trace, settledAt)); + console.info( + summarizeChannelSwitchTrace(trace, settledAt, settleWaitTruncated), + ); + appendSwitchPerfLogRecord( + buildSwitchPerfLogRecord(trace, settledAt, settleWaitTruncated), + ); }; const awaitDeferredCommit = () => { if (activeTrace !== trace) { @@ -315,16 +336,18 @@ export function settleChannelSwitchTrace(channelId: string): void { // to diagnose. Better no measurement than a fabricated one. return; } - if ( - performance.now() < waitDeadline && - document.querySelector('[data-render-pending="true"]') !== null - ) { + const decision = resolveSettleWait( + performance.now(), + waitDeadline, + document.querySelector('[data-render-pending="true"]') !== null, + ); + if (decision === "wait") { window.requestAnimationFrame(awaitDeferredCommit); return; } window.requestAnimationFrame(() => { if (activeTrace !== trace) return; - record(); + record(decision.settleWaitTruncated); }); }; window.requestAnimationFrame(awaitDeferredCommit); diff --git a/desktop/tests/e2e/member-heavy-switch.perf.ts b/desktop/tests/e2e/member-heavy-switch.perf.ts index a12720b263..97096f37aa 100644 --- a/desktop/tests/e2e/member-heavy-switch.perf.ts +++ b/desktop/tests/e2e/member-heavy-switch.perf.ts @@ -21,13 +21,20 @@ import { installMockBridge } from "../helpers/bridge"; * (project enumeration, work items, repo snapshots, * activity summaries) on top of the shell, so this * axis captures the cross-surface switch the felt - * 1-2s report singled out. + * 1-2s report singled out. Readiness gates on the + * surface's data-projects-hydrating marker, not just + * the shell header — a sample whose query fan is + * still loading must not count as settled. After the + * untimed warmup both surfaces are cache-warm, so + * measured samples are warm-switch commits. * * Method mirrors warm-switch-markdown.perf.ts: in-page click + rAF polling * (CDP latency never pollutes samples), longtask capture per switch, 4x CPU * throttle, medians over repeated switches, untimed warmup round-trip first. * `deep-history` is pinned to 150 rows so the message-mount cost is fixed - * and comparable across member counts. + * and comparable across member counts. Each direction of a round-trip is + * reported as its own median — the two legs mount different surfaces, and a + * combined median could represent neither and hide a one-leg regression. * * Run it (from desktop/): * pnpm build:e2e @@ -42,6 +49,11 @@ const MEASURED_SWITCHES = 8; const THROTTLE_RATE = 4; const DEEP_HISTORY_ROWS = 150; const MEMBER_COUNTS = [0, 2_000, 10_000] as const; +/** general + deep-history — the two channels inflateChannelMembers targets. */ +const INFLATED_CHANNEL_IDS = [ + "9a1657ac-f7aa-5db0-b632-d8bbeb6dfb50", + "feedf00d-0000-4000-8000-000000000007", +]; type SwitchSample = { ms: number; @@ -69,6 +81,8 @@ async function measureSwitch( targetTitle: string | null; /** Selector that must be present before the switch counts. */ readySelector: string; + /** Selector that must be ABSENT before the switch counts. */ + pendingSelector: string | null; }, ): Promise { return page.evaluate(async (args) => { @@ -92,6 +106,8 @@ async function measureSwitch( const ready = titleReady && document.querySelector(args.readySelector) !== null && + (args.pendingSelector === null || + document.querySelector(args.pendingSelector) === null) && document.querySelector('[data-render-pending="true"]') === null; if (ready) { requestAnimationFrame(() => requestAnimationFrame(() => resolve())); @@ -121,18 +137,23 @@ type SwitchTarget = { targetTestId: string; targetTitle: string | null; readySelector: string; + pendingSelector: string | null; }; const GENERAL_TARGET: SwitchTarget = { targetTestId: "channel-general", targetTitle: "general", - readySelector: "[data-message-id]", + // Prefixed so the previous channel's still-mounted rows can never satisfy + // the gate. + readySelector: '[data-message-id^="mock-general-"]', + pendingSelector: null, }; const DEEP_HISTORY_TARGET: SwitchTarget = { targetTestId: "channel-deep-history", targetTitle: "deep-history", readySelector: '[data-message-id^="mock-deep-history-"]', + pendingSelector: null, }; const PROJECTS_TARGET: SwitchTarget = { @@ -140,25 +161,12 @@ const PROJECTS_TARGET: SwitchTarget = { targetTitle: null, // Rendered by every Projects view mode (Activity intro or section header). readySelector: '[data-testid="projects-page-header"]', + // The header is shell — the query fan (projects, work items, repo + // snapshots, activity summaries) must have settled too. + pendingSelector: '[data-projects-hydrating="true"]', }; -async function runScenario( - page: import("@playwright/test").Page, - label: string, - target: SwitchTarget, - back: SwitchTarget, -): Promise { - // Untimed warmup round-trip: caches both surfaces' queries and jits the - // switch code paths. - await measureSwitch(page, target); - await measureSwitch(page, back); - - const samples: SwitchSample[] = []; - for (let run = 0; run < MEASURED_SWITCHES; run += 1) { - samples.push(await measureSwitch(page, target)); - samples.push(await measureSwitch(page, back)); - } - +function reportDirection(label: string, samples: SwitchSample[]): void { const times = samples.map((sample) => sample.ms); const longtaskTotals = samples.map((sample) => sample.longtaskTotal); /* eslint-disable no-console */ @@ -178,7 +186,37 @@ async function runScenario( `worst single longtask: ${Math.max(...samples.map((sample) => sample.longtaskMax)).toFixed(1)}ms`, ); /* eslint-enable no-console */ - return samples; +} + +async function runScenario( + page: import("@playwright/test").Page, + label: string, + target: SwitchTarget, + back: SwitchTarget, +): Promise<{ toTarget: SwitchSample[]; toBack: SwitchSample[] }> { + // Untimed warmup round-trip: caches both surfaces' queries and jits the + // switch code paths. + await measureSwitch(page, target); + await measureSwitch(page, back); + + // The two legs mount different surfaces; report each as its own median so + // a one-leg regression can never hide in a combined number. + const toTarget: SwitchSample[] = []; + const toBack: SwitchSample[] = []; + for (let run = 0; run < MEASURED_SWITCHES; run += 1) { + toTarget.push(await measureSwitch(page, target)); + toBack.push(await measureSwitch(page, back)); + } + + reportDirection( + `${label}, ${back.targetTestId} -> ${target.targetTestId}`, + toTarget, + ); + reportDirection( + `${label}, ${target.targetTestId} -> ${back.targetTestId}`, + toBack, + ); + return { toTarget, toBack }; } for (const memberCount of MEMBER_COUNTS) { @@ -234,11 +272,13 @@ for (const memberCount of MEMBER_COUNTS) { ), ); - // Verify the inflation actually landed before measuring anything. + // Verify the inflation actually landed — on BOTH inflated channels. The + // bridge silently skips unknown channel names, so a rename/typo would + // otherwise run baseline membership under a "10,000 members" label. if (memberCount > 0) { await expect .poll(() => - page.evaluate(async () => { + page.evaluate(async (channelIds) => { const invoke = ( window as unknown as { __TAURI_INTERNALS__: { @@ -249,11 +289,16 @@ for (const memberCount of MEMBER_COUNTS) { }; } ).__TAURI_INTERNALS__.invoke; - const response = await invoke("get_channel_members", { - channelId: "feedf00d-0000-4000-8000-000000000007", - }); - return response.members.length; - }), + const counts = await Promise.all( + channelIds.map(async (channelId) => { + const response = await invoke("get_channel_members", { + channelId, + }); + return response.members.length; + }), + ); + return Math.min(...counts); + }, INFLATED_CHANNEL_IDS), ) .toBeGreaterThanOrEqual(memberCount); } @@ -282,9 +327,14 @@ for (const memberCount of MEMBER_COUNTS) { await client.send("Emulation.setCPUThrottlingRate", { rate: 1 }); // Instrument, not a gate: assert the harness measured real work. - expect(channelSamples.length).toBe(MEASURED_SWITCHES * 2); - expect(channelSamples.every((sample) => sample.ms > 0)).toBe(true); - expect(projectsSamples.length).toBe(MEASURED_SWITCHES * 2); - expect(projectsSamples.every((sample) => sample.ms > 0)).toBe(true); + for (const samples of [ + channelSamples.toTarget, + channelSamples.toBack, + projectsSamples.toTarget, + projectsSamples.toBack, + ]) { + expect(samples.length).toBe(MEASURED_SWITCHES); + expect(samples.every((sample) => sample.ms > 0)).toBe(true); + } }); }