diff --git a/config/scripts/run-typecheck-projects-in-parallel.mjs b/config/scripts/run-typecheck-projects-in-parallel.mjs index 09b0a8e1d1f..eb3b1cd5bef 100644 --- a/config/scripts/run-typecheck-projects-in-parallel.mjs +++ b/config/scripts/run-typecheck-projects-in-parallel.mjs @@ -1,21 +1,61 @@ import { spawn } from 'node:child_process' -import { availableParallelism } from 'node:os' -import { fileURLToPath } from 'node:url' +import { availableParallelism, totalmem } from 'node:os' +import { fileURLToPath, pathToFileURL } from 'node:url' -// These projects overlap heavily in src/shared but have no build dependency on -// each other, so tsc can check them concurrently instead of in a `&&` chain. -const projects = [ - 'tsconfig.node.json', - 'tsconfig.tc.cli.json', - 'tsconfig.tc.web.json', - 'tsconfig.mobile-web.json' +const BYTES_PER_GIB = 1024 ** 3 + +// Peak heap per project, read from `tsc --extendedDiagnostics` and rounded up. node and +// web are the expensive pair: run together they exceed a 16 GB CI runner, and an +// out-of-memory runner is killed mid-check, so the job reports a lost runner instead of a +// type error. Admission is therefore by memory, not by core count alone. +export const TYPECHECK_PROJECTS = [ + { config: 'tsconfig.node.json', heapGib: 7 }, + { config: 'tsconfig.tc.web.json', heapGib: 6 }, + { config: 'tsconfig.tc.cli.json', heapGib: 2 }, + { config: 'tsconfig.mobile-web.json', heapGib: 1 } ] + +// The OS, node itself, and the runner agent need their share; the rest is what tsc may hold. +export function admissibleHeapGib(totalBytes) { + return Math.max(1, (totalBytes / BYTES_PER_GIB) * 0.75) +} + +/** + * Heaviest first, admitting another project only while it fits both the memory budget and + * the core count. A project larger than the whole budget still runs, alone, so a small + * machine makes progress rather than producing an empty batch forever. + */ +export function planTypecheckBatches(projects, { budgetGib, parallelism }) { + const pending = [...projects].sort((left, right) => right.heapGib - left.heapGib) + const batches = [] + + while (pending.length > 0) { + const batch = [] + let claimed = 0 + + for (let index = 0; index < pending.length;) { + const project = pending[index] + const admit = + batch.length === 0 || (batch.length < parallelism && claimed + project.heapGib <= budgetGib) + + if (admit) { + batch.push(project) + claimed += project.heapGib + pending.splice(index, 1) + } else { + index += 1 + } + } + + batches.push(batch) + } + + return batches +} + const repoRoot = fileURLToPath(new URL('../..', import.meta.url)) const tsc = fileURLToPath(new URL('../../node_modules/typescript/bin/tsc', import.meta.url)) -// Why serialize on a single-core runner: three tsc processes there thrash rather than overlap. -const concurrent = availableParallelism() > 1 - function checkProject(project) { return new Promise((resolve, reject) => { const child = spawn(process.execPath, [tsc, '--noEmit', '-p', `config/${project}`], { @@ -36,23 +76,32 @@ function checkProject(project) { }) } -let failures = [] -if (concurrent) { - const results = await Promise.allSettled(projects.map(checkProject)) - failures = results.filter((result) => result.status === 'rejected').map((result) => result.reason) -} else { - for (const project of projects) { - try { - await checkProject(project) - } catch (error) { - failures.push(error) +async function runTypecheckProjects() { + const batches = planTypecheckBatches(TYPECHECK_PROJECTS, { + budgetGib: admissibleHeapGib(totalmem()), + parallelism: availableParallelism() + }) + + // Every batch runs even after one fails, so a single broken project still reports the rest. + const failures = [] + for (const batch of batches) { + const results = await Promise.allSettled(batch.map((project) => checkProject(project.config))) + for (const result of results) { + if (result.status === 'rejected') { + failures.push(result.reason) + } } } + + return failures } -if (failures.length > 0) { - for (const failure of failures) { - console.error(failure.message ?? failure) +if (process.argv[1] && import.meta.url === pathToFileURL(process.argv[1]).href) { + const failures = await runTypecheckProjects() + if (failures.length > 0) { + for (const failure of failures) { + console.error(failure.message ?? failure) + } + process.exit(1) } - process.exit(1) } diff --git a/config/scripts/run-typecheck-projects-in-parallel.test.mjs b/config/scripts/run-typecheck-projects-in-parallel.test.mjs new file mode 100644 index 00000000000..b83ebbec8b4 --- /dev/null +++ b/config/scripts/run-typecheck-projects-in-parallel.test.mjs @@ -0,0 +1,74 @@ +import { describe, expect, it } from 'vitest' +import { + TYPECHECK_PROJECTS, + admissibleHeapGib, + planTypecheckBatches +} from './run-typecheck-projects-in-parallel.mjs' + +const CI_RUNNER = { totalBytes: 16 * 1024 ** 3, parallelism: 4 } +const DEV_LAPTOP = { totalBytes: 64 * 1024 ** 3, parallelism: 18 } + +function planFor({ totalBytes, parallelism }, projects = TYPECHECK_PROJECTS) { + return planTypecheckBatches(projects, { + budgetGib: admissibleHeapGib(totalBytes), + parallelism + }) +} + +function batchOf(batches, config) { + return batches.findIndex((batch) => batch.some((project) => project.config === config)) +} + +describe('typecheck project admission', () => { + it('keeps the two expensive projects off the same CI runner', () => { + const batches = planFor(CI_RUNNER) + expect(batchOf(batches, 'tsconfig.node.json')).not.toBe( + batchOf(batches, 'tsconfig.tc.web.json') + ) + }) + + it('holds every CI batch inside the memory budget', () => { + const budget = admissibleHeapGib(CI_RUNNER.totalBytes) + for (const batch of planFor(CI_RUNNER)) { + const claimed = batch.reduce((total, project) => total + project.heapGib, 0) + expect(claimed).toBeLessThanOrEqual(budget) + } + }) + + it('still fills a CI batch with the cheap projects rather than serializing everything', () => { + // Why: strict serialization measured ~60% slower than pairing the cheap work alongside. + expect(planFor(CI_RUNNER).length).toBeLessThan(TYPECHECK_PROJECTS.length) + }) + + it('leaves a roomy machine fully parallel', () => { + expect(planFor(DEV_LAPTOP)).toHaveLength(1) + }) + + it('serializes on a single core', () => { + const batches = planFor({ totalBytes: 64 * 1024 ** 3, parallelism: 1 }) + expect(batches).toHaveLength(TYPECHECK_PROJECTS.length) + expect(batches.every((batch) => batch.length === 1)).toBe(true) + }) + + it('runs a project that alone exceeds the budget instead of stalling', () => { + const batches = planTypecheckBatches([{ config: 'huge.json', heapGib: 512 }], { + budgetGib: admissibleHeapGib(2 * 1024 ** 3), + parallelism: 4 + }) + expect(batches).toEqual([[{ config: 'huge.json', heapGib: 512 }]]) + }) + + it('schedules every project exactly once', () => { + const scheduled = planFor(CI_RUNNER) + .flat() + .map((project) => project.config) + .sort() + expect(scheduled).toEqual(TYPECHECK_PROJECTS.map((project) => project.config).sort()) + }) + + it('never lets one project outgrow a CI runner on its own', () => { + // A single project over budget runs anyway, so this is the ceiling the pool cannot rescue. + const heaviest = Math.max(...TYPECHECK_PROJECTS.map((project) => project.heapGib)) + expect(heaviest).toBeLessThanOrEqual(admissibleHeapGib(CI_RUNNER.totalBytes)) + }) +}) diff --git a/mobile/app/h/[hostId]/agent-history/[worktreeId].tsx b/mobile/app/h/[hostId]/agent-history/[worktreeId].tsx index bfe3fd9b086..a1421083002 100644 --- a/mobile/app/h/[hostId]/agent-history/[worktreeId].tsx +++ b/mobile/app/h/[hostId]/agent-history/[worktreeId].tsx @@ -1,8 +1,9 @@ import { useLocalSearchParams } from 'expo-router' import { MobileAgentSessionHistoryPanel } from '../../../../src/agent-history/MobileAgentSessionHistoryPanel' import { MobileWebShellScreen } from '../../../../src/mobile-web-shell/MobileWebShellScreen' +import { ShellSwitchPendingScreen } from '../../../../src/mobile-web-shell/ShellSwitchPendingScreen' import { shellScreenRoute } from '../../../../src/mobile-web-shell/shell-screen-route' -import { useMobileWebShellEnabled } from '../../../../src/mobile-web-shell/use-mobile-web-shell-enabled' +import { useShellSwitchDecision } from '../../../../src/mobile-web-shell/shell-switch-decision' import { firstParam } from '../../../../src/navigation/route-param-reader' /** @@ -10,8 +11,7 @@ import { firstParam } from '../../../../src/navigation/route-param-reader' * * The switch is `index.tsx`'s, for its reasons: the shell renders the page only for a route the * bundle lists with grants this app implements, `fallback` is what a negotiation that said no - * falls back to, and a settling flag read renders the native panel because a store build never - * reaches storage at all. + * falls back to, and a flag read still settling paints neither renderer. * * Two dynamic segments rather than one, so both are encoded: `useLocalSearchParams` answers the * decoded value, and a worktree id or a deep-linked host id carrying `/`, `?`, `#` or whitespace @@ -38,21 +38,27 @@ export default function MobileAgentSessionHistoryScreen() { const hostId = firstParam(params.hostId) const worktreeId = firstParam(params.worktreeId) const name = firstParam(params.name) - const enabled = useMobileWebShellEnabled() const panel = ( ) - if (enabled !== true || !hostId || !worktreeId) { - return panel + // Built before the decision rather than after it, as every switch does now: the decision needs + // to know whether the shell is a possible outcome before it can say a neutral frame is owed. + const route = + hostId && worktreeId + ? shellScreenRoute({ + pathname: `/h/${encodeURIComponent(hostId)}/agent-history/${encodeURIComponent(worktreeId)}`, + // Omitted rather than empty: the page reads the label off the search half, and a `name=` + // with nothing after it is a label, where an absent one lets the panel derive its own. + ...(name === '' ? {} : { params: { name } }) + }) + : null + const decision = useShellSwitchDecision(route) + + if (decision.kind === 'pending') { + return } - const route = shellScreenRoute({ - pathname: `/h/${encodeURIComponent(hostId)}/agent-history/${encodeURIComponent(worktreeId)}`, - // Omitted rather than empty: the page reads the label off the search half, and a `name=` - // with nothing after it is a label, where an absent one lets the panel derive its own. - ...(name === '' ? {} : { params: { name } }) - }) - if (route === null) { + if (decision.kind === 'native') { return panel } // Keyed on the route: a host captures the grants its session was opened with, so a screen @@ -60,6 +66,11 @@ export default function MobileAgentSessionHistoryScreen() { // page has left. The key is what makes the change a remount, which disposes that bridge in the // commit, and the new session starts with no grants until its own `init`. return ( - + ) } diff --git a/mobile/app/h/[hostId]/files/[worktreeId].tsx b/mobile/app/h/[hostId]/files/[worktreeId].tsx index 068851facff..8de592f68ee 100644 --- a/mobile/app/h/[hostId]/files/[worktreeId].tsx +++ b/mobile/app/h/[hostId]/files/[worktreeId].tsx @@ -6,15 +6,16 @@ import { shellScreenRouteKey } from '../../../../src/mobile-web-shell/shell-screen-route' import { MobileWebShellScreen } from '../../../../src/mobile-web-shell/MobileWebShellScreen' -import { useMobileWebShellEnabled } from '../../../../src/mobile-web-shell/use-mobile-web-shell-enabled' +import { ShellSwitchPendingScreen } from '../../../../src/mobile-web-shell/ShellSwitchPendingScreen' +import { useShellSwitchDecision } from '../../../../src/mobile-web-shell/shell-switch-decision' /** * The file explorer, from the desktop's bundle or from this app. * * The shell decides, not this switch: it renders the page only for a route the bundle lists with * grants this app implements, and answers `native-route` otherwise, which is what `fallback` is. - * `enabled === null` is the flag read still settling and renders the native screen, which is the - * only frame a store build ever paints here. + * A flag read still settling is a third answer and paints neither renderer; see + * `shell-switch-decision.ts`. * * Encoded, not interpolated raw, for the reason `web.tsx` states: an id carrying `?`, `#` or * whitespace would build a pathname the page refuses and mount nothing. @@ -32,7 +33,6 @@ export default function MobileFileExplorerScreen() { const hostId = firstParam(params.hostId) const worktreeId = firstParam(params.worktreeId) const name = firstParam(params.name) - const enabled = useMobileWebShellEnabled() const native = ( ) @@ -47,7 +47,12 @@ export default function MobileFileExplorerScreen() { }) : null - if (enabled !== true || !hostId || route === null) { + const decision = useShellSwitchDecision(route) + + if (decision.kind === 'pending') { + return + } + if (decision.kind === 'native') { return native } // Keyed on the route: a host captures the grants its session was opened with, so a screen reused @@ -55,9 +60,9 @@ export default function MobileFileExplorerScreen() { // left. The key is what makes the change a remount, which disposes that bridge in the commit. return ( ) diff --git a/mobile/app/h/[hostId]/files/preview/[worktreeId].tsx b/mobile/app/h/[hostId]/files/preview/[worktreeId].tsx index b682011aaf1..a8e247ef18d 100644 --- a/mobile/app/h/[hostId]/files/preview/[worktreeId].tsx +++ b/mobile/app/h/[hostId]/files/preview/[worktreeId].tsx @@ -9,7 +9,8 @@ import { shellScreenRouteKey } from '../../../../../src/mobile-web-shell/shell-screen-route' import { MobileWebShellScreen } from '../../../../../src/mobile-web-shell/MobileWebShellScreen' -import { useMobileWebShellEnabled } from '../../../../../src/mobile-web-shell/use-mobile-web-shell-enabled' +import { ShellSwitchPendingScreen } from '../../../../../src/mobile-web-shell/ShellSwitchPendingScreen' +import { useShellSwitchDecision } from '../../../../../src/mobile-web-shell/shell-switch-decision' /** * The file preview, from the desktop's bundle or from this app. @@ -41,7 +42,6 @@ export default function MobileFilePreviewRoute() { worktreeName?: string | string[] }>() const route = normalizeMobileFilePreviewRouteParams(params) - const enabled = useMobileWebShellEnabled() const native = const shellRoute = route.ok @@ -53,7 +53,14 @@ export default function MobileFilePreviewRoute() { }) : null - if (enabled !== true || !route.ok || shellRoute === null) { + const decision = useShellSwitchDecision(shellRoute) + + if (decision.kind === 'pending') { + return + } + // `route.ok` again for the compiler: `shellRoute` is built only on the ok branch, so a `shell` + // decision already implies it. + if (decision.kind === 'native' || !route.ok) { return native } // Keyed on the whole route, params included, for two reasons. A host captures the grants its @@ -63,9 +70,9 @@ export default function MobileFilePreviewRoute() { // was opened on, with nothing to tell it otherwise. return ( ) diff --git a/mobile/app/h/[hostId]/index.tsx b/mobile/app/h/[hostId]/index.tsx index f14c81a2ca7..05f55248b14 100644 --- a/mobile/app/h/[hostId]/index.tsx +++ b/mobile/app/h/[hostId]/index.tsx @@ -4,8 +4,9 @@ import { firstParam } from '../../../src/navigation/route-param-reader' import { HostScreen } from '../../../src/host-screen/HostScreen' import { useResponsiveLayout } from '../../../src/layout/responsive-layout' import { MobileWebShellScreen } from '../../../src/mobile-web-shell/MobileWebShellScreen' +import { ShellSwitchPendingScreen } from '../../../src/mobile-web-shell/ShellSwitchPendingScreen' import { shellScreenRoute } from '../../../src/mobile-web-shell/shell-screen-route' -import { useMobileWebShellEnabled } from '../../../src/mobile-web-shell/use-mobile-web-shell-enabled' +import { useShellSwitchDecision } from '../../../src/mobile-web-shell/shell-switch-decision' /** * The worktree list, from the desktop's bundle or from this app. @@ -15,8 +16,8 @@ import { useMobileWebShellEnabled } from '../../../src/mobile-web-shell/use-mobi * So the two ways to stay native are a flag that is off and a negotiation that said no, and the * second one covers every host whose desktop is older than the page. * - * `enabled === null` is the flag read still settling, and it renders the native screen: a store - * build never reaches storage at all, so that is the only frame it ever paints here. + * The flag read settling is a third answer, not a fourth spelling of native: see + * `shell-switch-decision.ts`. * * Encoded, not interpolated raw, for the reason `web.tsx` states: a deep-linked host id carrying * `?`, `#` or whitespace would build a pathname the page refuses, and a refusal here is a failure @@ -30,14 +31,17 @@ function HostListScreen() { // truthy, so a bare read also builds `/h/` and hands that over; this answers `''` and stays. const params = useLocalSearchParams<{ hostId?: string | string[] }>() const hostId = firstParam(params.hostId) - const enabled = useMobileWebShellEnabled() // Asked here as every switch asks it: encoding does not save a `.` or `..` host id, which fails // the bridge's segment rule, and handing that over paints the page's failure screen over the // native list this route already has. const route = shellScreenRoute({ pathname: `/h/${encodeURIComponent(hostId)}` }) + const decision = useShellSwitchDecision(hostId === '' ? null : route) - if (enabled !== true || !hostId || route === null) { + if (decision.kind === 'pending') { + return + } + if (decision.kind === 'native') { return } return ( @@ -46,7 +50,7 @@ function HostListScreen() { // so a host id change must be a remount rather than a prop update. key={hostId} hostId={hostId} - route={route} + route={decision.route} fallback={} /> ) diff --git a/mobile/app/h/[hostId]/review/[worktreeId].tsx b/mobile/app/h/[hostId]/review/[worktreeId].tsx index 2a5ee04791a..79d3944d349 100644 --- a/mobile/app/h/[hostId]/review/[worktreeId].tsx +++ b/mobile/app/h/[hostId]/review/[worktreeId].tsx @@ -6,7 +6,8 @@ import { shellScreenRouteKey } from '../../../../src/mobile-web-shell/shell-screen-route' import { MobileWebShellScreen } from '../../../../src/mobile-web-shell/MobileWebShellScreen' -import { useMobileWebShellEnabled } from '../../../../src/mobile-web-shell/use-mobile-web-shell-enabled' +import { ShellSwitchPendingScreen } from '../../../../src/mobile-web-shell/ShellSwitchPendingScreen' +import { useShellSwitchDecision } from '../../../../src/mobile-web-shell/shell-switch-decision' /** * Diff review, from the desktop's bundle or from this app. @@ -33,7 +34,6 @@ export default function MobileDiffReviewScreen() { }>() const hostId = firstReviewParam(params.hostId) const worktreeId = firstReviewParam(params.worktreeId) - const enabled = useMobileWebShellEnabled() const native = // The four query params are read by the screen itself, so they are carried across whole rather @@ -52,7 +52,12 @@ export default function MobileDiffReviewScreen() { }) : null - if (enabled !== true || !hostId || route === null) { + const decision = useShellSwitchDecision(route) + + if (decision.kind === 'pending') { + return + } + if (decision.kind === 'native') { return native } // Keyed on the route: a host captures the grants its session was opened with, so a screen reused @@ -60,9 +65,9 @@ export default function MobileDiffReviewScreen() { // left. A `file` change is the common case here and is a param change, not a path change. return ( ) diff --git a/mobile/app/h/[hostId]/session/[worktreeId].tsx b/mobile/app/h/[hostId]/session/[worktreeId].tsx index 3d72cd9b81e..32b860abd02 100644 --- a/mobile/app/h/[hostId]/session/[worktreeId].tsx +++ b/mobile/app/h/[hostId]/session/[worktreeId].tsx @@ -7,7 +7,8 @@ import { shellScreenRouteKey } from '../../../../src/mobile-web-shell/shell-screen-route' import { MobileWebShellScreen } from '../../../../src/mobile-web-shell/MobileWebShellScreen' -import { useMobileWebShellEnabled } from '../../../../src/mobile-web-shell/use-mobile-web-shell-enabled' +import { ShellSwitchPendingScreen } from '../../../../src/mobile-web-shell/ShellSwitchPendingScreen' +import { useShellSwitchDecision } from '../../../../src/mobile-web-shell/shell-switch-decision' /** * The session screen — terminal and chat — from the desktop's bundle or from this app. @@ -40,7 +41,6 @@ export default function MobileSessionScreen() { }>() const hostId = firstParam(params.hostId) const worktreeId = firstParam(params.worktreeId) - const enabled = useMobileWebShellEnabled() const router = useRouter() const native = const paneKey = firstParam(params.paneKey) ?? '' @@ -77,7 +77,12 @@ export default function MobileSessionScreen() { }) : null - if (enabled !== true || !hostId || route === null) { + const decision = useShellSwitchDecision(route) + + if (decision.kind === 'pending') { + return + } + if (decision.kind === 'native') { return native } // Keyed on the route minus `paneKey`: a host captures the grants its session was opened with, so @@ -90,9 +95,9 @@ export default function MobileSessionScreen() { const { paneKey: _paneKey, ...identity } = routeParams return ( diff --git a/mobile/app/h/[hostId]/source-control/[worktreeId].tsx b/mobile/app/h/[hostId]/source-control/[worktreeId].tsx index 0b4701088aa..783a2c1a5aa 100644 --- a/mobile/app/h/[hostId]/source-control/[worktreeId].tsx +++ b/mobile/app/h/[hostId]/source-control/[worktreeId].tsx @@ -7,14 +7,16 @@ import { shellScreenRouteKey } from '../../../../src/mobile-web-shell/shell-screen-route' import { MobileWebShellScreen } from '../../../../src/mobile-web-shell/MobileWebShellScreen' -import { useMobileWebShellEnabled } from '../../../../src/mobile-web-shell/use-mobile-web-shell-enabled' +import { ShellSwitchPendingScreen } from '../../../../src/mobile-web-shell/ShellSwitchPendingScreen' +import { useShellSwitchDecision } from '../../../../src/mobile-web-shell/shell-switch-decision' /** * The source-control hub, from the desktop's bundle or from this app. * * The files switch's shape, for its reasons: the shell answers `native-route` for a route the * bundle does not list or this app's grants do not cover, and `fallback` is what that renders. - * `enabled === null` is the flag read still settling, which is the only frame a store build paints. + * A flag read still settling is a third answer and paints neither renderer; see + * `shell-switch-decision.ts`. * * `pr` and `history` are not switched and never will be. Both are `Redirect`s into this route, and * a redirect inside the page would leave the session bound to a pathname the page has left; left @@ -37,7 +39,6 @@ export default function MobileSourceControlScreen() { const name = firstParam(params.name) const origin = firstParam(params.origin) const tab = firstParam(params.tab) - const enabled = useMobileWebShellEnabled() const native = ( + } + if (decision.kind === 'native') { return native } // Keyed on the route: a host captures the grants its session was opened with, so a screen reused @@ -73,9 +79,9 @@ export default function MobileSourceControlScreen() { // left. The key is what makes the change a remount, which disposes that bridge in the commit. return ( ) diff --git a/mobile/app/h/[hostId]/tasks.tsx b/mobile/app/h/[hostId]/tasks.tsx index e9b80ccb589..4759bf6168c 100644 --- a/mobile/app/h/[hostId]/tasks.tsx +++ b/mobile/app/h/[hostId]/tasks.tsx @@ -1,7 +1,8 @@ import { useLocalSearchParams } from 'expo-router' import { MobileWebShellScreen } from '../../../src/mobile-web-shell/MobileWebShellScreen' +import { ShellSwitchPendingScreen } from '../../../src/mobile-web-shell/ShellSwitchPendingScreen' import { shellScreenRoute } from '../../../src/mobile-web-shell/shell-screen-route' -import { useMobileWebShellEnabled } from '../../../src/mobile-web-shell/use-mobile-web-shell-enabled' +import { useShellSwitchDecision } from '../../../src/mobile-web-shell/shell-switch-decision' import { firstParam } from '../../../src/navigation/route-param-reader' import { MobileTasksScreen } from '../../../src/tasks/MobileTasksScreen' @@ -21,19 +22,24 @@ export default function MobileTasksRoute() { }>() const hostId = firstParam(params.hostId) const taskSource = firstParam(params.taskSource) - const enabled = useMobileWebShellEnabled() const native = - if (enabled !== true || !hostId) { - return native + // Built before the decision rather than after it, as every switch does now: the decision needs + // to know whether the shell is a possible outcome before it can say a neutral frame is owed. + const route = hostId + ? shellScreenRoute({ + pathname: `/h/${encodeURIComponent(hostId)}/tasks`, + // Omitted rather than empty: an absent provider lets the page pick its own default, where + // `taskSource=` is a provider named nothing. + ...(taskSource === '' ? {} : { params: { taskSource } }) + }) + : null + const decision = useShellSwitchDecision(route) + + if (decision.kind === 'pending') { + return } - const route = shellScreenRoute({ - pathname: `/h/${encodeURIComponent(hostId)}/tasks`, - // Omitted rather than empty: an absent provider lets the page pick its own default, where - // `taskSource=` is a provider named nothing. - ...(taskSource === '' ? {} : { params: { taskSource } }) - }) - if (route === null) { + if (decision.kind === 'native') { return native } return ( @@ -42,7 +48,7 @@ export default function MobileTasksRoute() { // with, so a host id change must be a remount rather than a prop update. key={hostId} hostId={hostId} - route={route} + route={decision.route} fallback={native} /> ) diff --git a/mobile/app/h/[hostId]/web.tsx b/mobile/app/h/[hostId]/web.tsx index 8e3cfccc39c..79bb4beb502 100644 --- a/mobile/app/h/[hostId]/web.tsx +++ b/mobile/app/h/[hostId]/web.tsx @@ -1,33 +1,33 @@ -import { ActivityIndicator, StyleSheet, View } from 'react-native' import { Redirect, useLocalSearchParams } from 'expo-router' import { MobileWebShellScreen } from '../../../src/mobile-web-shell/MobileWebShellScreen' -import { useMobileWebShellEnabled } from '../../../src/mobile-web-shell/use-mobile-web-shell-enabled' -import { colors } from '../../../src/theme/mobile-theme' +import { ShellSwitchPendingScreen } from '../../../src/mobile-web-shell/ShellSwitchPendingScreen' +import { useShellSwitchDecision } from '../../../src/mobile-web-shell/shell-switch-decision' /** * The hybrid shell route, dark behind a development-only flag. * - * One of the two callers of `useMobileWebShellEnabled`. With the flag off — which is every store build, - * since the only writer is the `__DEV__` Troubleshoot toggle — this redirects and the screen is - * never constructed, so nothing is fetched, written or swept. It sits under `app/h/[hostId]` so - * `HostProtocolGate` in that group's layout still owns the `desktop-too-old` wall above it. + * With the flag off — which is every store build, since the only writer is the `__DEV__` + * Troubleshoot toggle — this redirects and the screen is never constructed, so nothing is fetched, + * written or swept. It sits under `app/h/[hostId]` so `HostProtocolGate` in that group's layout + * still owns the `desktop-too-old` wall above it. * * Reachable by deep link and from the developer row only; no screen links here. */ export default function MobileWebShellRoute() { const { hostId } = useLocalSearchParams<{ hostId: string }>() - const enabled = useMobileWebShellEnabled() + // Encoded here rather than in the JSX below so the decision holds the route the shell is handed: + // `hostId` arrives decoded from the URL, so one carrying `?`, `#` or whitespace would build a + // pathname the page refuses and never mount anything. + const decision = useShellSwitchDecision( + hostId ? { pathname: `/h/${encodeURIComponent(hostId)}` } : null + ) - if (enabled === null) { + if (decision.kind === 'pending') { // A redirect fired before the read settles would bounce a flag that is on, and a screen mounted // before it settles would fetch on a flag that is off. Neither, until it is known. - return ( - - - - ) + return } - if (!enabled || !hostId) { + if (decision.kind === 'native') { return } // The screen the page stands in for. The document is served at `/`, which matches no route in @@ -36,27 +36,14 @@ export default function MobileWebShellRoute() { // The fallback is a redirect rather than the native screen: this route exists only to open the // page deliberately, so a bundle that does not list the worktree list has nothing to show here // and the host route is where the list actually lives. - // - // Encoded, not interpolated raw: `hostId` arrives decoded from the URL, so one carrying `?`, `#` - // or whitespace would build a pathname the page refuses and never mount anything. The page - // decodes it back when it matches `[hostId]`, so the screen it opens is the same one. return ( } /> ) } - -const styles = StyleSheet.create({ - pending: { - flex: 1, - alignItems: 'center', - justifyContent: 'center', - backgroundColor: colors.bgBase - } -}) diff --git a/mobile/src/mobile-web-shell/ShellSwitchPendingScreen.tsx b/mobile/src/mobile-web-shell/ShellSwitchPendingScreen.tsx new file mode 100644 index 00000000000..7317476345f --- /dev/null +++ b/mobile/src/mobile-web-shell/ShellSwitchPendingScreen.tsx @@ -0,0 +1,28 @@ +import { ActivityIndicator, StyleSheet, View } from 'react-native' +import { colors } from '../theme/mobile-theme' + +/** + * What a route switch paints while the hybrid shell flag is still being read, which is a + * development build only: a store build cannot have the flag on and never reaches this. + * + * The base background and nothing else placed on it, so the frame before the decision looks like + * the frame after it whichever way the decision goes. Lifted out of the `web` route, which is + * where this exact view already was, rather than written again: it is `HostProtocolGate`'s pending + * state to the pixel, which is the surface directly above every switch that uses this one. + */ +export function ShellSwitchPendingScreen() { + return ( + + + + ) +} + +const styles = StyleSheet.create({ + pending: { + flex: 1, + alignItems: 'center', + justifyContent: 'center', + backgroundColor: colors.bgBase + } +}) diff --git a/mobile/src/mobile-web-shell/catch-all-page-route.test.tsx b/mobile/src/mobile-web-shell/catch-all-page-route.test.tsx index 26555e2c197..0772dfbc1f3 100644 --- a/mobile/src/mobile-web-shell/catch-all-page-route.test.tsx +++ b/mobile/src/mobile-web-shell/catch-all-page-route.test.tsx @@ -33,6 +33,12 @@ vi.mock('@react-native-async-storage/async-storage', () => ({ } })) +vi.mock('react-native', () => ({ + ActivityIndicator: 'ActivityIndicator', + StyleSheet: { create: (styles: unknown) => styles }, + View: 'View' +})) + vi.mock('expo-router', () => ({ useLocalSearchParams: () => dependencies.params })) vi.mock('./PageRouteUnavailableScreen', () => ({ diff --git a/mobile/src/mobile-web-shell/catch-all-page-route.tsx b/mobile/src/mobile-web-shell/catch-all-page-route.tsx index 51dfc01a722..d7eb43b9efe 100644 --- a/mobile/src/mobile-web-shell/catch-all-page-route.tsx +++ b/mobile/src/mobile-web-shell/catch-all-page-route.tsx @@ -3,7 +3,8 @@ import { firstParam } from '../navigation/route-param-reader' import { shellScreenRoute, shellScreenRouteKey } from './shell-screen-route' import { MobileWebShellScreen } from './MobileWebShellScreen' import { PageRouteUnavailableScreen } from './PageRouteUnavailableScreen' -import { useMobileWebShellEnabled } from './use-mobile-web-shell-enabled' +import { ShellSwitchPendingScreen } from './ShellSwitchPendingScreen' +import { useShellSwitchDecision } from './shell-switch-decision' /** * Any host-scoped pathname this app has no route file for, handed to the shell. @@ -32,7 +33,6 @@ export default function MobileWebPageCatchAllScreen() { }>() const hostId = firstParam(params.hostId) const segments = Array.isArray(params.page) ? params.page : params.page ? [params.page] : [] - const enabled = useMobileWebShellEnabled() const refusal = const route = @@ -42,7 +42,12 @@ export default function MobileWebPageCatchAllScreen() { }) : null - if (enabled !== true || !hostId || route === null) { + const decision = useShellSwitchDecision(route) + + if (decision.kind === 'pending') { + return + } + if (decision.kind === 'native') { return refusal } // Keyed on the route, as the other switches are: a host captures the grants its session opened @@ -57,9 +62,9 @@ export default function MobileWebPageCatchAllScreen() { // connection that the screen does not exist. `catch-all-page-route-states.test.tsx` drives them. return ( ) diff --git a/mobile/src/mobile-web-shell/mobile-web-shell-agent-history-route.test.tsx b/mobile/src/mobile-web-shell/mobile-web-shell-agent-history-route.test.tsx index f0da35f1aed..b05bb37f1be 100644 --- a/mobile/src/mobile-web-shell/mobile-web-shell-agent-history-route.test.tsx +++ b/mobile/src/mobile-web-shell/mobile-web-shell-agent-history-route.test.tsx @@ -29,6 +29,12 @@ vi.mock('@react-native-async-storage/async-storage', () => ({ } })) +vi.mock('react-native', () => ({ + ActivityIndicator: 'ActivityIndicator', + StyleSheet: { create: (styles: unknown) => styles }, + View: 'View' +})) + vi.mock('expo-router', () => ({ useLocalSearchParams: () => dependencies.params })) vi.mock('../agent-history/MobileAgentSessionHistoryPanel', () => ({ @@ -92,16 +98,16 @@ describe('the native agent-history route that hands off to the shell', () => { ]) }) - it('renders the native panel while the flag read is still settling', async () => { - // `index.tsx`'s frame, for its reason: the read is async and a store build never reaches - // storage at all, so the native screen is the only thing this route may paint first. - await renderRoute() - expect(dependencies.panels[0]).toEqual({ - hostId: 'host-1', - worktreeId: 'wt-1', - name: 'my worktree' + it('renders neither panel nor shell while the flag read is still settling', async () => { + // `index.tsx`'s frame, for its reason: the read is async, so the native panel used to mount + // here and be replaced by the page the moment a flag-on read landed. No `await` inside `act`, + // which leaves the read's promise pending and catches the route in that window. + act(() => { + create(createElement(MobileAgentSessionHistoryScreen)) }) - expect(dependencies.panels).toHaveLength(1) + expect(dependencies.panels).toEqual([]) + expect(dependencies.routes).toEqual([]) + await act(async () => {}) }) it('names no params when the caller named no worktree', async () => { diff --git a/mobile/src/mobile-web-shell/mobile-web-shell-files-route.test.tsx b/mobile/src/mobile-web-shell/mobile-web-shell-files-route.test.tsx index 19743f36dfd..df09aad4670 100644 --- a/mobile/src/mobile-web-shell/mobile-web-shell-files-route.test.tsx +++ b/mobile/src/mobile-web-shell/mobile-web-shell-files-route.test.tsx @@ -31,6 +31,12 @@ vi.mock('@react-native-async-storage/async-storage', () => ({ } })) +vi.mock('react-native', () => ({ + ActivityIndicator: 'ActivityIndicator', + StyleSheet: { create: (styles: unknown) => styles }, + View: 'View' +})) + vi.mock('expo-router', () => ({ useLocalSearchParams: () => dependencies.params })) vi.mock('../files/MobileFileExplorerPanel', () => ({ @@ -105,14 +111,15 @@ describe('the native file explorer route that hands off to the shell', () => { ]) }) - it('renders the native panel while the flag read is still settling', async () => { - await renderExplorer() - expect(dependencies.panels[0]).toEqual({ - hostId: 'host-1', - worktreeId: 'wt-1', - name: 'my worktree', - embedded: false + it('renders neither panel nor shell while the flag read is still settling', async () => { + // No `await` inside `act`, which leaves the read's promise pending: the native panel used to + // mount in this window and be replaced by the page the moment a flag-on read landed. + act(() => { + create(createElement(MobileFileExplorerScreen)) }) + expect(dependencies.panels).toEqual([]) + expect(dependencies.routes).toEqual([]) + await act(async () => {}) }) /** diff --git a/mobile/src/mobile-web-shell/mobile-web-shell-flag-census.test.ts b/mobile/src/mobile-web-shell/mobile-web-shell-flag-census.test.ts index bb560d88959..9fc1b71603c 100644 --- a/mobile/src/mobile-web-shell/mobile-web-shell-flag-census.test.ts +++ b/mobile/src/mobile-web-shell/mobile-web-shell-flag-census.test.ts @@ -12,8 +12,10 @@ import { censusSourceFiles } from '../test-support/census-source-files' const MOBILE_ROOT = join(import.meta.dirname, '..', '..') const FLAG_KEY = 'orca:mobileWebShellEnabled' const DEFINITION = 'src/storage/preferences.ts' -/** The one product reader. Every route asks it, so the list below stays the whole census. */ +/** The one product reader, which only the shared switch decision asks. */ const FLAG_HOOK = 'src/mobile-web-shell/use-mobile-web-shell-enabled.ts' +/** The one caller of that hook: every route asks this instead, so its list is the whole census. */ +const DECISION = 'src/mobile-web-shell/shell-switch-decision.ts' const ROUTE = 'app/h/[hostId]/web.tsx' const HOST_ROUTE = 'app/h/[hostId]/index.tsx' const AGENT_HISTORY_ROUTE = 'app/h/[hostId]/agent-history/[worktreeId].tsx' @@ -25,6 +27,8 @@ const REVIEW_ROUTE = 'app/h/[hostId]/review/[worktreeId].tsx' const SESSION_ROUTE = 'app/h/[hostId]/session/[worktreeId].tsx' /** The one switch with no native screen behind it; its route file only re-exports this body. */ const CATCH_ALL_ROUTE = 'src/mobile-web-shell/catch-all-page-route.tsx' +/** What a switch paints while the decision is `pending`, and the third thing every switch names. */ +const PENDING_SCREEN = 'src/mobile-web-shell/ShellSwitchPendingScreen.tsx' /** One entry per screen the flag can switch to the page, which is what a review reads. */ const SWITCHED_ROUTES = [ HOST_ROUTE, @@ -62,11 +66,35 @@ function filesContaining(needle: string): string[] { .sort() } +/** + * The same matches with the line each was read off, as the failure message for the rules below. + * + * A census that answers only with paths tells a reader which file is wrong and nothing about what + * in it is: the needles here are identifiers, and a file can name one in an import, a call or a + * comment. The snippet is what turns "this list moved" into the edit that moved it. + */ +function matchesOf(needle: string): string { + // Sorted by path then by line number, not as text: `:59:` sorts before `:4:` as a string, which + // reads as a file whose matches are out of order. + return [...SOURCES] + .sort((left, right) => left.path.localeCompare(right.path)) + .flatMap((file) => + file.text + .split('\n') + .flatMap((line, index) => + line.includes(needle) ? [`${file.path}:${index + 1}: ${line.trim()}`] : [] + ) + ) + .join('\n') +} + describe('who touches the hybrid shell flag', () => { it('reaches every shipped tree, so the absence assertions below cannot pass vacuously', () => { const paths = SOURCES.map((file) => file.path) expect(paths).toContain(DEFINITION) expect(paths).toContain(FLAG_HOOK) + expect(paths).toContain(DECISION) + expect(paths).toContain(PENDING_SCREEN) expect(paths).toContain(ROUTE) for (const route of SWITCHED_ROUTES) { expect(paths).toContain(route) @@ -90,17 +118,51 @@ describe('who touches the hybrid shell flag', () => { ) }) - it('reaches the switched routes through that hook and no others', () => { + it('is read by the shared switch decision and by nothing else', () => { + // The narrowest this has ever been, and the reason the rule below is total: a route cannot + // hold a private opinion about the flag — including about the window where it is still `null` + // — without reading it, and this is the only place that reads it. + expect( + filesContaining('useMobileWebShellEnabled'), + matchesOf('useMobileWebShellEnabled') + ).toEqual([DECISION, FLAG_HOOK].sort()) + }) + + it('reaches the switched routes through that decision and no others', () => { // Each switched route is a screen the flag decides the renderer of, and one more is one more // place a dark feature could turn itself on. The list grows once per domain series, in the PR // that switches the route file to MobileWebShellScreen, and never as a side effect of anything // else. A switched route is inert until MOBILE_WEB_PAGE_ROUTES lists it as well, so an entry // here can land a PR ahead of that one. - expect(filesContaining('useMobileWebShellEnabled')).toEqual( - [FLAG_HOOK, ROUTE, ...SWITCHED_ROUTES].sort() + expect(filesContaining('useShellSwitchDecision'), matchesOf('useShellSwitchDecision')).toEqual( + [DECISION, ROUTE, ...SWITCHED_ROUTES].sort() ) }) + it('gives every one of them the same neutral state to paint while the flag is unresolved', () => { + // The rule a sixth switch would otherwise regress past. Reading the flag through the decision + // is not on its own enough: a switch that ignored `pending` and fell through to its native + // screen would satisfy the rule above and still flash native in front of a flag-on user. This + // one says every switch names the neutral screen, which is existence rather than shape — where + // it names it is the route test's business, and `shell-switch-null-flag.test.tsx` drives all + // nine through the states themselves. + expect( + filesContaining('ShellSwitchPendingScreen'), + matchesOf('ShellSwitchPendingScreen') + ).toEqual([PENDING_SCREEN, ROUTE, ...SWITCHED_ROUTES].sort()) + }) + + it('fences the build kind in one place, which both the read and the hook ask', () => { + // The `__DEV__` test that makes a store build unable to turn the flag on. The hook starts its + // state on it so a release build never reaches the neutral state, which is the same answer + // `loadMobileWebShellEnabled` gives one render later — and two spellings of one build-kind + // test are two things to keep true, where this feature's darkness rests on exactly one. + expect( + filesContaining('mobileWebShellFlagCanBeOn'), + matchesOf('mobileWebShellFlagCanBeOn') + ).toEqual([DEFINITION, FLAG_HOOK].sort()) + }) + it('is written only by the developer row', () => { expect(filesContaining('saveMobileWebShellEnabled')).toEqual([DEFINITION, DEVELOPER_ROW].sort()) }) diff --git a/mobile/src/mobile-web-shell/mobile-web-shell-host-list-route.test.tsx b/mobile/src/mobile-web-shell/mobile-web-shell-host-list-route.test.tsx index 22598b6b382..ffa06894ba2 100644 --- a/mobile/src/mobile-web-shell/mobile-web-shell-host-list-route.test.tsx +++ b/mobile/src/mobile-web-shell/mobile-web-shell-host-list-route.test.tsx @@ -25,6 +25,12 @@ vi.mock('@react-native-async-storage/async-storage', () => ({ } })) +vi.mock('react-native', () => ({ + ActivityIndicator: 'ActivityIndicator', + StyleSheet: { create: (styles: unknown) => styles }, + View: 'View' +})) + vi.mock('expo-router', () => ({ useLocalSearchParams: () => ({ hostId: dependencies.hostId }) })) diff --git a/mobile/src/mobile-web-shell/mobile-web-shell-route.test.tsx b/mobile/src/mobile-web-shell/mobile-web-shell-route.test.tsx index da47619ea9c..b856641a3fc 100644 --- a/mobile/src/mobile-web-shell/mobile-web-shell-route.test.tsx +++ b/mobile/src/mobile-web-shell/mobile-web-shell-route.test.tsx @@ -150,6 +150,9 @@ describe('the hybrid shell route', () => { const tree = rendered.tree expect(tree === null ? [] : byName(tree, 'Redirect')).toEqual([]) expect(dependencies.mounted).toEqual([]) + // The neutral state itself, rendered for real here: `shell-switch-null-flag.test.tsx` mocks it + // to count mounts across all nine switches, so this is where its shape stays pinned. + expect(tree === null ? [] : byName(tree, 'ActivityIndicator')).toHaveLength(1) await act(async () => {}) }) }) diff --git a/mobile/src/mobile-web-shell/mobile-web-shell-session-route.test.tsx b/mobile/src/mobile-web-shell/mobile-web-shell-session-route.test.tsx index b225f486120..cfddb1eeed8 100644 --- a/mobile/src/mobile-web-shell/mobile-web-shell-session-route.test.tsx +++ b/mobile/src/mobile-web-shell/mobile-web-shell-session-route.test.tsx @@ -32,6 +32,12 @@ vi.mock('@react-native-async-storage/async-storage', () => ({ } })) +vi.mock('react-native', () => ({ + ActivityIndicator: 'ActivityIndicator', + StyleSheet: { create: (styles: unknown) => styles }, + View: 'View' +})) + vi.mock('expo-router', () => ({ useLocalSearchParams: () => dependencies.params, useRouter: () => ({ diff --git a/mobile/src/mobile-web-shell/shell-switch-decision.test.ts b/mobile/src/mobile-web-shell/shell-switch-decision.test.ts new file mode 100644 index 00000000000..d9b17ada5ea --- /dev/null +++ b/mobile/src/mobile-web-shell/shell-switch-decision.test.ts @@ -0,0 +1,32 @@ +import { describe, expect, it } from 'vitest' +import { shellSwitchDecision } from './shell-switch-decision' + +const ROUTE = { pathname: '/h/host-1/files/wt-1' } + +/** + * The flag is read asynchronously, so every switch has a window where it holds `null` — the read + * has not settled and neither answer is known yet. Treating that as "off" is what made a flag-on + * user watch the native screen mount and then be replaced by the page. + */ +describe('the hybrid shell switch decision', () => { + it('waits while the flag is unresolved rather than answering native', () => { + expect(shellSwitchDecision(null, ROUTE)).toEqual({ kind: 'pending' }) + }) + + it('answers native with the flag off', () => { + expect(shellSwitchDecision(false, ROUTE)).toEqual({ kind: 'native' }) + }) + + it('answers the shell, carrying the route, with the flag on', () => { + expect(shellSwitchDecision(true, ROUTE)).toEqual({ kind: 'shell', route: ROUTE }) + }) + + it('answers native for a route the shell could never open, without waiting on the flag', () => { + // No neutral frame is owed when the shell is not a possible outcome: a switch whose params + // build no route the bridge would accept has one renderer, and holding it back would paint a + // spinner over a screen that was always going to be the native one. + expect(shellSwitchDecision(null, null)).toEqual({ kind: 'native' }) + expect(shellSwitchDecision(true, null)).toEqual({ kind: 'native' }) + expect(shellSwitchDecision(false, null)).toEqual({ kind: 'native' }) + }) +}) diff --git a/mobile/src/mobile-web-shell/shell-switch-decision.ts b/mobile/src/mobile-web-shell/shell-switch-decision.ts new file mode 100644 index 00000000000..842f405c09e --- /dev/null +++ b/mobile/src/mobile-web-shell/shell-switch-decision.ts @@ -0,0 +1,42 @@ +import type { BridgeInitRoute } from './bridge/bridge-init-route' +import { useMobileWebShellEnabled } from './use-mobile-web-shell-enabled' + +/** + * Which renderer a hybrid-shell route switch mounts, once that is knowable. + * + * `pending` is the third answer every switch was missing. Where the flag can be on it is read from + * storage after the first render, so `null` is a window every switch passes through, and each one + * used to spend it on the native screen: a flag-on user watched the native screen mount, subscribe + * and paint, then be torn down and replaced by the page. One decision here so a switch cannot hold + * a private opinion about `null`, and so the flag keeps exactly one reader — which is what makes + * the census beside this file total rather than a list somebody remembers to extend. + * + * `pending` is unreachable on a build that cannot have the flag on: the hook starts at `false` + * there, so a store build commits its native renderer on frame one and pays nothing for a neutral + * state it could never have used. + * + * A route the shell could never open is answered `native` without waiting: the flag cannot change + * that outcome, and a neutral frame in front of a decided one is a flash this file exists to remove. + */ +export type ShellSwitchDecision = + | { readonly kind: 'pending' } + | { readonly kind: 'native' } + | { readonly kind: 'shell'; readonly route: BridgeInitRoute } + +export function shellSwitchDecision( + enabled: boolean | null, + route: BridgeInitRoute | null +): ShellSwitchDecision { + if (route === null) { + return { kind: 'native' } + } + if (enabled === null) { + return { kind: 'pending' } + } + return enabled ? { kind: 'shell', route } : { kind: 'native' } +} + +/** The route is `null` when this switch's params name no screen the shell could open. */ +export function useShellSwitchDecision(route: BridgeInitRoute | null): ShellSwitchDecision { + return shellSwitchDecision(useMobileWebShellEnabled(), route) +} diff --git a/mobile/src/mobile-web-shell/shell-switch-null-flag.test.tsx b/mobile/src/mobile-web-shell/shell-switch-null-flag.test.tsx new file mode 100644 index 00000000000..49d2030d045 --- /dev/null +++ b/mobile/src/mobile-web-shell/shell-switch-null-flag.test.tsx @@ -0,0 +1,331 @@ +import { createElement, type ComponentType } from 'react' +import { act, create, type ReactTestRenderer } from 'react-test-renderer' +import { beforeEach, describe, expect, it, vi } from 'vitest' + +type SwitchDependencies = { + storage: Map + /** What bounds the neutral window on a released phone; see the last case in this file. */ + reads: number + /** Committed mounts, not renders: React may discard a render, and what this file is about is + * what the user was shown. */ + natives: string[] + shells: string[] + /** Committed mounts of the neutral screen, which is what "never paints a neutral frame" needs: + * a frame committed and replaced inside one `act` leaves nothing in the final tree. */ + neutrals: number + params: Record +} + +const dependencies = vi.hoisted((): SwitchDependencies => ({ + storage: new Map(), + reads: 0, + natives: [], + shells: [], + neutrals: 0, + params: {} +})) + +const nativeScreen = vi.hoisted( + () => + async (name: string): Promise>> => { + const React = await import('react') + return function NativeScreen() { + React.useEffect(() => { + dependencies.natives.push(name) + }, []) + return null + } + } +) + +vi.mock('@react-native-async-storage/async-storage', () => ({ + default: { + getItem: async (key: string) => { + dependencies.reads += 1 + return dependencies.storage.get(key) ?? null + }, + setItem: async (key: string, value: string) => { + dependencies.storage.set(key, value) + } + } +})) + +vi.mock('react-native', () => ({ + ActivityIndicator: 'ActivityIndicator', + StyleSheet: { create: (styles: unknown) => styles }, + View: 'View' +})) + +vi.mock('expo-router', () => ({ + Redirect: 'Redirect', + useLocalSearchParams: () => dependencies.params, + useRouter: () => ({ setParams: () => {} }) +})) + +vi.mock('./MobileWebShellScreen', async () => { + const React = await import('react') + return { + MobileWebShellScreen: (props: { route: { pathname: string } }) => { + const pathname = React.useRef(props.route.pathname) + React.useEffect(() => { + dependencies.shells.push(pathname.current) + }, []) + return null + } + } +}) + +vi.mock('./ShellSwitchPendingScreen', async () => { + const React = await import('react') + return { + ShellSwitchPendingScreen: () => { + React.useEffect(() => { + dependencies.neutrals += 1 + }, []) + return null + } + } +}) + +vi.mock('../host-screen/HostScreen', async () => ({ HostScreen: await nativeScreen('host-list') })) +vi.mock('../components/WorkspaceDetailPlaceholder', async () => ({ + WorkspaceDetailPlaceholder: await nativeScreen('workspace-detail-placeholder') +})) +vi.mock('../layout/responsive-layout', () => ({ + useResponsiveLayout: () => ({ isWideLayout: false }) +})) +vi.mock('../tasks/MobileTasksScreen', async () => ({ + MobileTasksScreen: await nativeScreen('tasks') +})) +vi.mock('../agent-history/MobileAgentSessionHistoryPanel', async () => ({ + MobileAgentSessionHistoryPanel: await nativeScreen('agent-history') +})) +vi.mock('../files/MobileFileExplorerPanel', async () => ({ + MobileFileExplorerPanel: await nativeScreen('files') +})) +vi.mock('../files/MobileFilePreviewScreen', async () => ({ + MobileFilePreviewScreen: await nativeScreen('files-preview') +})) +vi.mock('../source-control/MobileSourceControlPanel', async () => ({ + MobileSourceControlPanel: await nativeScreen('source-control') +})) +vi.mock('../session/MobileDiffReviewRouteScreen', async () => ({ + MobileDiffReviewRouteScreen: await nativeScreen('review') +})) +vi.mock('../session/MobileSessionRouteScreen', async () => ({ + MobileSessionRouteScreen: await nativeScreen('session') +})) +vi.mock('./PageRouteUnavailableScreen', async () => ({ + PageRouteUnavailableScreen: await nativeScreen('catch-all') +})) + +import HostListRoute from '../../app/h/[hostId]/index' +import TasksRoute from '../../app/h/[hostId]/tasks' +import AgentHistoryRoute from '../../app/h/[hostId]/agent-history/[worktreeId]' +import FilesRoute from '../../app/h/[hostId]/files/[worktreeId]' +import FilesPreviewRoute from '../../app/h/[hostId]/files/preview/[worktreeId]' +import SourceControlRoute from '../../app/h/[hostId]/source-control/[worktreeId]' +import ReviewRoute from '../../app/h/[hostId]/review/[worktreeId]' +import SessionRoute from '../../app/h/[hostId]/session/[worktreeId]' +import CatchAllRoute from './catch-all-page-route' + +const FLAG_KEY = 'orca:mobileWebShellEnabled' + +/** + * Every switch the hybrid shell flag decides, with the params each needs to name a route the shell + * could open. `native` is what that switch renders when the flag is off — a panel for most of them + * and the refusal screen for the catch-all, which has no native screen behind it. + */ +type SwitchCase = { + readonly name: string + readonly Route: ComponentType + readonly params: Record + /** What the mocked native screen pushes when it mounts. */ + readonly native: string + readonly pathname: string +} + +const SWITCHES: readonly SwitchCase[] = [ + { + name: 'host list', + Route: HostListRoute, + params: { hostId: 'host-1' }, + native: 'host-list', + pathname: '/h/host-1' + }, + { + name: 'tasks', + Route: TasksRoute, + params: { hostId: 'host-1' }, + native: 'tasks', + pathname: '/h/host-1/tasks' + }, + { + name: 'agent history', + Route: AgentHistoryRoute, + params: { hostId: 'host-1', worktreeId: 'wt-1' }, + native: 'agent-history', + pathname: '/h/host-1/agent-history/wt-1' + }, + { + name: 'files', + Route: FilesRoute, + params: { hostId: 'host-1', worktreeId: 'wt-1' }, + native: 'files', + pathname: '/h/host-1/files/wt-1' + }, + { + name: 'file preview', + Route: FilesPreviewRoute, + params: { hostId: 'host-1', worktreeId: 'wt-1', relativePath: 'src/index.ts' }, + native: 'files-preview', + pathname: '/h/host-1/files/preview/wt-1' + }, + { + name: 'source control', + Route: SourceControlRoute, + params: { hostId: 'host-1', worktreeId: 'wt-1' }, + native: 'source-control', + pathname: '/h/host-1/source-control/wt-1' + }, + { + name: 'review', + Route: ReviewRoute, + params: { hostId: 'host-1', worktreeId: 'wt-1' }, + native: 'review', + pathname: '/h/host-1/review/wt-1' + }, + { + name: 'session', + Route: SessionRoute, + params: { hostId: 'host-1', worktreeId: 'wt-1' }, + native: 'session', + pathname: '/h/host-1/session/wt-1' + }, + { + name: 'catch-all', + Route: CatchAllRoute, + params: { hostId: 'host-1', page: ['settings'] }, + native: 'catch-all', + pathname: '/h/host-1/settings' + } +] + +/** + * `__DEV__` is a React Native global with no value under this runner, so every case pins it rather + * than inheriting one: assigned onto `globalThis` for a build kind that has it and deleted for a + * store build, which is what the app sees when the bundler defined nothing. Which one is in force + * decides whether the neutral state is reachable at all, so an unpinned case would be measuring + * the runner. + */ +function setDevelopmentBuild(isDevelopmentBuild: boolean | undefined): void { + if (isDevelopmentBuild === undefined) { + Reflect.deleteProperty(globalThis, '__DEV__') + return + } + Object.assign(globalThis, { __DEV__: isDevelopmentBuild }) +} + +/** Renders without settling the flag read: no `await` inside `act`, so the effect's promise is + * deliberately left pending and the switch is caught in its unresolved window. */ +function renderUnsettled(Route: ComponentType): ReactTestRenderer { + const rendered: { tree: ReactTestRenderer | null } = { tree: null } + act(() => { + rendered.tree = create(createElement(Route)) + }) + if (rendered.tree === null) { + throw new Error('the switch did not render') + } + return rendered.tree +} + +describe.each(SWITCHES)('the $name switch on a development build', (entry) => { + beforeEach(() => { + dependencies.storage.clear() + dependencies.reads = 0 + dependencies.natives.length = 0 + dependencies.shells.length = 0 + dependencies.neutrals = 0 + dependencies.params = { ...entry.params } + setDevelopmentBuild(true) + }) + + it('mounts neither renderer while the flag is unresolved, and paints the neutral state', async () => { + dependencies.storage.set(FLAG_KEY, 'true') + renderUnsettled(entry.Route) + expect(dependencies.natives).toEqual([]) + expect(dependencies.shells).toEqual([]) + expect(dependencies.neutrals).toBe(1) + await act(async () => {}) + }) + + it('mounts the shell once when the read resolves on, having never mounted native', async () => { + dependencies.storage.set(FLAG_KEY, 'true') + renderUnsettled(entry.Route) + await act(async () => {}) + expect(dependencies.natives).toEqual([]) + expect(dependencies.shells).toEqual([entry.pathname]) + }) + + it('mounts native once when the read resolves off, and nothing else', async () => { + renderUnsettled(entry.Route) + await act(async () => {}) + expect(dependencies.natives).toEqual([entry.native]) + expect(dependencies.shells).toEqual([]) + }) +}) + +/** + * The build every store user is on, where the flag cannot be turned on at all. + * + * A neutral frame is worth a native mount only when the flag could resolve on. Outside `__DEV__` + * it cannot, so the hook holds `false` from its first render and the `pending` branch is + * unreachable here: the switch commits the native renderer on frame one and never revises it. + */ +describe.each(SWITCHES)('the $name switch on a release build', (entry) => { + beforeEach(() => { + dependencies.storage.clear() + dependencies.reads = 0 + dependencies.natives.length = 0 + dependencies.shells.length = 0 + dependencies.neutrals = 0 + dependencies.params = { ...entry.params } + setDevelopmentBuild(false) + }) + + it('commits native on its first frame and never mounts the neutral screen', async () => { + // A flag a development build left in the container, which a store build shares a bundle id + // with: still unreachable, and still no neutral frame in front of it. + dependencies.storage.set(FLAG_KEY, 'true') + renderUnsettled(entry.Route) + expect(dependencies.neutrals).toBe(0) + expect(dependencies.natives).toEqual([entry.native]) + expect(dependencies.shells).toEqual([]) + await act(async () => { + await Promise.resolve() + }) + expect(dependencies.neutrals).toBe(0) + expect(dependencies.natives).toEqual([entry.native]) + expect(dependencies.shells).toEqual([]) + }) + + it('reaches no storage at all, which is what makes the first frame decidable', async () => { + // The same fact the initialiser rests on: `loadMobileWebShellEnabled` answers `false` outside + // `__DEV__` before it looks at the key, so there is nothing to wait for and nothing to read. + dependencies.storage.set(FLAG_KEY, 'true') + renderUnsettled(entry.Route) + await act(async () => { + await Promise.resolve() + }) + expect(dependencies.reads).toBe(0) + }) + + it('does the same when the bundler defined no `__DEV__` at all', async () => { + setDevelopmentBuild(undefined) + dependencies.storage.set(FLAG_KEY, 'true') + renderUnsettled(entry.Route) + expect(dependencies.neutrals).toBe(0) + expect(dependencies.natives).toEqual([entry.native]) + await act(async () => {}) + }) +}) diff --git a/mobile/src/mobile-web-shell/use-mobile-web-shell-enabled.ts b/mobile/src/mobile-web-shell/use-mobile-web-shell-enabled.ts index 6e732a72ced..073e5250be5 100644 --- a/mobile/src/mobile-web-shell/use-mobile-web-shell-enabled.ts +++ b/mobile/src/mobile-web-shell/use-mobile-web-shell-enabled.ts @@ -1,19 +1,23 @@ import { useEffect, useState } from 'react' -import { loadMobileWebShellEnabled } from '../storage/preferences' +import { loadMobileWebShellEnabled, mobileWebShellFlagCanBeOn } from '../storage/preferences' /** * The hybrid shell flag, read once per mount. * * The one product reader of `loadMobileWebShellEnabled`, which is what keeps the flag census * meaningful: the routes ask this and nothing asks the storage key twice. `null` is the read still - * settling, and every caller treats it as off — a route that guessed on would fetch on a flag that - * is off, which is the one thing the flag exists to prevent. + * settling, which `shell-switch-decision.ts` turns into a neutral frame rather than a guess. * - * A release build never reaches storage at all; `loadMobileWebShellEnabled` answers false outside - * `__DEV__` before it looks. + * That frame is worth a native mount only where the flag could resolve on, so a build that cannot + * have it on starts at `false` rather than `null`: `mobileWebShellFlagCanBeOn` is the same fact + * `loadMobileWebShellEnabled` would answer with, one render earlier, and it makes `null` + * unreachable on every store build. The effect still runs there and still answers `false`, because + * the initialiser is a starting point and the read is what decides. */ export function useMobileWebShellEnabled(): boolean | null { - const [enabled, setEnabled] = useState(null) + const [enabled, setEnabled] = useState(() => + mobileWebShellFlagCanBeOn() ? null : false + ) useEffect(() => { let stale = false diff --git a/mobile/src/storage/preferences.ts b/mobile/src/storage/preferences.ts index 092929d2416..93c65672377 100644 --- a/mobile/src/storage/preferences.ts +++ b/mobile/src/storage/preferences.ts @@ -123,11 +123,23 @@ const MOBILE_WEB_SHELL_KEY = 'orca:mobileWebShellEnabled' // Why: the hybrid shell route is dark. Default-off means a store build never fetches, writes or // sweeps a bundle cache, and the only writer is the __DEV__ Troubleshoot toggle — anything but // `'true'`, including an unreadable store, is off. +/** + * Whether this build can have the flag on at all. + * + * A release build never reads the key: it shares its bundle id with the development build and the + * iOS data container survives an install-over, so a flag a developer left on would otherwise + * follow the store build in and mount the shell on a deep link. + * + * Named rather than spelled twice. The hook beside the reader starts its state on this answer so + * a store build is decided on its first render rather than after an effect, and two spellings of + * one `__DEV__` test would be two things to keep true. + */ +export function mobileWebShellFlagCanBeOn(): boolean { + return typeof __DEV__ !== 'undefined' && __DEV__ +} + export async function loadMobileWebShellEnabled(): Promise { - // A release build never reads the key at all: it shares its bundle id with the development build - // and the iOS data container survives an install-over, so a flag a developer left on would - // otherwise follow the store build in and mount the shell on a deep link. - if (typeof __DEV__ === 'undefined' || !__DEV__) { + if (!mobileWebShellFlagCanBeOn()) { return false } try { diff --git a/src/main/codex-usage/codex-model-pricing.ts b/src/main/codex-usage/codex-model-pricing.ts index 23e721e27db..c754974d4c5 100644 --- a/src/main/codex-usage/codex-model-pricing.ts +++ b/src/main/codex-usage/codex-model-pricing.ts @@ -77,6 +77,14 @@ export const MODEL_PRICING: Record = { inputTiers: [{ threshold: LONG_CONTEXT_THRESHOLD_TOKENS, price: 2 }], cachedInputTiers: [{ threshold: LONG_CONTEXT_THRESHOLD_TOKENS, price: 0.2 }], outputTiers: [{ threshold: LONG_CONTEXT_THRESHOLD_TOKENS, price: 9 }] + }, + 'gpt-6-astra': { + input: 10, + cachedInput: 1, + output: 50, + inputTiers: [{ threshold: LONG_CONTEXT_THRESHOLD_TOKENS, price: 20 }], + cachedInputTiers: [{ threshold: LONG_CONTEXT_THRESHOLD_TOKENS, price: 2 }], + outputTiers: [{ threshold: LONG_CONTEXT_THRESHOLD_TOKENS, price: 75 }] } } @@ -171,6 +179,9 @@ export function normalizeModelForPricing(model: string | null): string | null { if (normalized === 'gpt-5.6-luna' || normalized.startsWith('gpt-5.6-luna-')) { return 'gpt-5.6-luna' } + if (normalized === 'gpt-6-astra' || normalized.startsWith('gpt-6-astra-')) { + return 'gpt-6-astra' + } // Why: OpenAI routes the bare `gpt-5.6` alias to Sol. Match it exactly — a // `gpt-5.6-` prefix match would swallow the tier IDs above and any future // cheaper variant. diff --git a/src/main/codex-usage/codex-usage-rollup-projections.ts b/src/main/codex-usage/codex-usage-rollup-projections.ts index 1a947940c92..34bbb912581 100644 --- a/src/main/codex-usage/codex-usage-rollup-projections.ts +++ b/src/main/codex-usage/codex-usage-rollup-projections.ts @@ -31,6 +31,7 @@ export function buildSummary( let events = 0 let estimatedCostUsd = 0 let hasAnyBillableCost = false + let hasUnpricedModels = false const byModel = new Map() const byProject = new Map() @@ -55,6 +56,9 @@ export function buildSummary( if (cost !== null) { hasAnyBillableCost = true estimatedCostUsd += cost + } else if (row.model !== null) { + // A named model with no pricing entry: its tokens silently leave the total. + hasUnpricedModels = true } } @@ -72,6 +76,7 @@ export function buildSummary( reasoningOutputTokens, totalTokens, estimatedCostUsd: hasAnyBillableCost ? estimatedCostUsd : null, + hasUnpricedModels, topModel, topProject, hasAnyCodexData: filteredSessions.length > 0 || filteredDaily.length > 0 diff --git a/src/main/codex-usage/store-model-pricing.test.ts b/src/main/codex-usage/store-model-pricing.test.ts index ee2206684d9..5f88de18088 100644 --- a/src/main/codex-usage/store-model-pricing.test.ts +++ b/src/main/codex-usage/store-model-pricing.test.ts @@ -212,6 +212,145 @@ describe('CodexUsageStore', () => { expect(breakdown.find((row) => row.key === 'gpt-5.6-luna')?.estimatedCostUsd).toBeCloseTo(0.205) }) + it('prices GPT-6 Astra with current OpenAI rates', async () => { + const store = createStoreWithState({ + dailyAggregates: [ + { + day: '2026-04-09', + model: 'gpt-6-astra', + projectKey: 'worktree:repo-1::/workspace/repo', + projectLabel: 'Repo', + repoId: 'repo-1', + worktreeId: 'repo-1::/workspace/repo', + eventCount: 1, + inputTokens: 2_000_000, + cachedInputTokens: 1_000_000, + outputTokens: 1_000_000, + reasoningOutputTokens: 100_000, + totalTokens: 3_000_000, + hasInferredPricing: false + } + ] + }) + + const breakdown = await store.getBreakdown('orca', '30d', 'model') + + expect(breakdown.find((row) => row.key === 'gpt-6-astra')?.estimatedCostUsd).toBeCloseTo(87.208) + }) + + it('normalizes GPT-6 Astra reasoning suffixes and snapshot IDs before pricing', async () => { + const store = createStoreWithState({ + dailyAggregates: ['gpt-6-astra-high', 'gpt-6-astra-2026-09-01'].map((model) => ({ + day: '2026-04-09', + model, + projectKey: 'worktree:repo-1::/workspace/repo', + projectLabel: 'Repo', + repoId: 'repo-1', + worktreeId: 'repo-1::/workspace/repo', + eventCount: 1, + inputTokens: 100_000, + cachedInputTokens: 50_000, + outputTokens: 25_000, + reasoningOutputTokens: 5_000, + totalTokens: 125_000, + hasInferredPricing: false + })) + }) + + const breakdown = await store.getBreakdown('orca', '30d', 'model') + + expect(breakdown.find((row) => row.key === 'gpt-6-astra-high')?.estimatedCostUsd).toBeCloseTo( + 1.8 + ) + expect( + breakdown.find((row) => row.key === 'gpt-6-astra-2026-09-01')?.estimatedCostUsd + ).toBeCloseTo(1.8) + }) + + it('flags a named model with no pricing entry so its missing tokens are declared', async () => { + const store = createStoreWithState({ + dailyAggregates: [ + { + day: '2026-04-09', + model: 'gpt-5', + projectKey: 'worktree:repo-1::/workspace/repo', + projectLabel: 'Repo', + repoId: 'repo-1', + worktreeId: 'repo-1::/workspace/repo', + eventCount: 1, + inputTokens: 1000, + cachedInputTokens: 400, + outputTokens: 250, + reasoningOutputTokens: 100, + totalTokens: 1250, + hasInferredPricing: false + }, + { + day: '2026-04-09', + model: 'gpt-7-unreleased', + projectKey: 'worktree:repo-1::/workspace/repo', + projectLabel: 'Repo', + repoId: 'repo-1', + worktreeId: 'repo-1::/workspace/repo', + eventCount: 1, + inputTokens: 5_000_000, + cachedInputTokens: 0, + outputTokens: 5_000_000, + reasoningOutputTokens: 0, + totalTokens: 10_000_000, + hasInferredPricing: false + } + ] + }) + + const summary = await store.getSummary('orca', '30d') + + expect(summary.hasUnpricedModels).toBe(true) + // The unpriced row's ten million tokens are absent from the total it sits beside. + expect(summary.estimatedCostUsd).toBeCloseTo(0.0033, 6) + }) + + it('keeps the unpriced flag off for priced rows and for rows with no model name', async () => { + const store = createStoreWithState({ + dailyAggregates: [ + { + day: '2026-04-09', + model: 'gpt-6-astra', + projectKey: 'worktree:repo-1::/workspace/repo', + projectLabel: 'Repo', + repoId: 'repo-1', + worktreeId: 'repo-1::/workspace/repo', + eventCount: 1, + inputTokens: 1000, + cachedInputTokens: 400, + outputTokens: 250, + reasoningOutputTokens: 100, + totalTokens: 1250, + hasInferredPricing: false + }, + { + day: '2026-04-09', + model: null, + projectKey: 'worktree:repo-1::/workspace/repo', + projectLabel: 'Repo', + repoId: 'repo-1', + worktreeId: 'repo-1::/workspace/repo', + eventCount: 1, + inputTokens: 1000, + cachedInputTokens: 0, + outputTokens: 500, + reasoningOutputTokens: 0, + totalTokens: 1500, + hasInferredPricing: true + } + ] + }) + + const summary = await store.getSummary('orca', '30d') + + expect(summary.hasUnpricedModels).toBe(false) + }) + it('normalizes Codex model variants and reasoning suffixes before pricing', async () => { const store = createStoreWithState({ dailyAggregates: [ diff --git a/src/main/pty/wsl-orca-env.test.ts b/src/main/pty/wsl-orca-env.test.ts index e9004b7df8d..e56146ea991 100644 --- a/src/main/pty/wsl-orca-env.test.ts +++ b/src/main/pty/wsl-orca-env.test.ts @@ -70,6 +70,7 @@ describe('addOrcaWslInteropEnv', () => { ORCA_TAB_ID: 'tab-1', ORCA_WORKTREE_ID: 'repo::\\\\wsl.localhost\\Ubuntu\\home\\jin\\repo', ORCA_AGENT_LAUNCH_TOKEN: 'launch-secret', + ORCA_OPENCODE_AGENT: 'opencode2', ORCA_AGENT_HOOK_PORT: '4567', ORCA_AGENT_HOOK_TOKEN: 'token', ORCA_AGENT_HOOK_ENV: 'dev', @@ -94,6 +95,7 @@ describe('addOrcaWslInteropEnv', () => { expect(env.WSLENV).toContain('ORCA_TAB_ID/u') expect(env.WSLENV).toContain('ORCA_WORKTREE_ID/u') expect(env.WSLENV).toContain('ORCA_AGENT_LAUNCH_TOKEN/u') + expect(env.WSLENV).toContain('ORCA_OPENCODE_AGENT/u') expect(env.WSLENV).toContain('ORCA_AGENT_HOOK_PORT/u') expect(env.WSLENV).toContain('ORCA_AGENT_HOOK_TOKEN/u') expect(env.WSLENV).toContain('ORCA_AGENT_HOOK_ENV/u') diff --git a/src/main/pty/wsl-orca-env.ts b/src/main/pty/wsl-orca-env.ts index fa4d234bd0d..04035a957ca 100644 --- a/src/main/pty/wsl-orca-env.ts +++ b/src/main/pty/wsl-orca-env.ts @@ -53,6 +53,7 @@ function worktreeSetupWslenvEntries(env: Record): st ] } +/** Adds the host environment values required by a WSL PTY and its guest relay. */ export function addOrcaWslInteropEnv(env: Record): void { // Why set here: every WSL spawn path funnels through this helper, and the // in-guest login script needs the resolved wrapper root. Windows/WSL wrappers @@ -83,6 +84,9 @@ export function addOrcaWslInteropEnv(env: Record): void { 'ORCA_TAB_ID/u', 'ORCA_WORKTREE_ID/u', 'ORCA_AGENT_LAUNCH_TOKEN/u', + // The guest plugin uses this marker to select the OpenCode variant that + // owns the pane when both native and WSL installations are present. + 'ORCA_OPENCODE_AGENT/u', `${SETUP_AGENT_SEQUENCE_STARTUP_COMMAND_ENV}/u`, `${SETUP_AGENT_SEQUENCE_STARTUP_SCRIPT_ENV}/u`, 'ORCA_ORCHESTRATION_COMPATIBILITY_HOST_KIND/u', diff --git a/src/renderer/src/components/artifacts/ArtifactCollection.test.tsx b/src/renderer/src/components/artifacts/ArtifactCollection.test.tsx index 2c9b2cb405a..7a275441885 100644 --- a/src/renderer/src/components/artifacts/ArtifactCollection.test.tsx +++ b/src/renderer/src/components/artifacts/ArtifactCollection.test.tsx @@ -1,8 +1,9 @@ // @vitest-environment happy-dom import '@testing-library/jest-dom/vitest' -import { cleanup, render, screen } from '@testing-library/react' +import { cleanup, render, screen, within } from '@testing-library/react' import userEvent from '@testing-library/user-event' +import { useState } from 'react' import { afterEach, describe, expect, it, vi } from 'vitest' import type { ArtifactListItem } from '../../../../shared/artifacts' @@ -37,12 +38,41 @@ function artifact(slug: string, title: string): ArtifactListItem { } } +/** Selection held in state, so whether a click selected is readable off the row's own wash. */ +function SelectingCollection({ items }: { items: ArtifactListItem[] }): React.JSX.Element { + const [selectedSlug, setSelectedSlug] = useState(null) + return ( + + ) +} + +// `hidden`, because a modal Radix menu marks the rest of the tree aria-hidden while it is open — +// and the row behind that menu is exactly what these assertions have to read. +function rowFor(title: string): HTMLElement { + return screen.getByRole('button', { name: new RegExp(title), hidden: true }) +} + describe('ArtifactCollection', () => { afterEach(cleanup) + // Why: the viewport stub spies on HTMLElement.prototype, so an unrestored one fabricates + // layout for every later test in the run. + afterEach(() => vi.restoreAllMocks()) function renderCollection( items: ArtifactListItem[], - selectArtifact = vi.fn() + selectArtifact = vi.fn(), + hasMore = false ): { container: HTMLElement; selectArtifact: ReturnType } { const { container } = render( @@ -52,7 +82,7 @@ describe('ArtifactCollection', () => { selectedSlug={items[0]?.artifact.slug ?? null} selectArtifact={selectArtifact} deleteArtifact={vi.fn()} - hasMore={false} + hasMore={hasMore} loadingMore={false} loadMore={vi.fn()} onRefresh={vi.fn()} @@ -111,6 +141,84 @@ describe('ArtifactCollection', () => { expect(screen.getByText('No matches')).toBeInTheDocument() }) + /** + * Both of the row's menu escapes at once. The actions trigger is a DOM descendant of the row, and + * Radix portals the open menu out of it but React still bubbles its clicks back through the row — + * so without either guard the detail drawer opens behind every menu interaction. + */ + it('runs a row menu action without selecting the artifact behind the menu', async () => { + const user = userEvent.setup() + const items = [artifact('first', 'First artifact'), artifact('second', 'Second artifact')] + render( + + + + ) + + // Positive control: a click on the row body does select, so the negatives below are not vacuous. + await user.click(rowFor('First artifact')) + expect(rowFor('First artifact')).toHaveAttribute('data-current', 'true') + + const trigger = within(rowFor('Second artifact')).getByRole('button', { + name: 'Artifact actions' + }) + await user.click(trigger) + // Radix mounts menu content only while the menu is open, so a match here is the menu opening. + expect(screen.getByRole('menuitem', { name: 'Copy link' })).toBeInTheDocument() + expect(rowFor('Second artifact')).not.toHaveAttribute('data-current') + + await user.click(screen.getByRole('menuitem', { name: 'Copy link' })) + expect(rowFor('Second artifact')).not.toHaveAttribute('data-current') + expect(rowFor('First artifact')).toHaveAttribute('data-current', 'true') + }) + + // Why past 50: below the windowing threshold every row is in the DOM, so nothing has to be + // announced — the set size only has to be right once the rows are windowed. + const PAGED_ITEM_COUNT = 60 + + function pagedItems(): ArtifactListItem[] { + return Array.from({ length: PAGED_ITEM_COUNT }, (_, index) => + artifact(`slug-${index}`, `Artifact ${index}`) + ) + } + + // Why: happy-dom has no layout, and the virtualizer sizes its window from the scroller's + // offsetHeight — left at 0 it mounts no rows at all and the assertions below would be vacuous. + function stubScrollerViewport(): void { + vi.spyOn(HTMLElement.prototype, 'offsetHeight', 'get').mockImplementation( + function (this: HTMLElement) { + return this.classList.contains('overflow-auto') ? 600 : 53 + } + ) + } + + function announcedSetSizes(container: HTMLElement): string[] { + const wrappers = Array.from(container.querySelectorAll('[data-index]')) + expect(wrappers.length).toBeGreaterThan(0) + // The window is a strict subset, so a set size read off the DOM could not reach the real total. + expect(wrappers.length).toBeLessThan(PAGED_ITEM_COUNT) + return wrappers.map((wrapper) => wrapper.getAttribute('aria-setsize') ?? 'missing') + } + + it('announces an unknown set size while a further page is loadable', () => { + stubScrollerViewport() + const { container } = renderCollection(pagedItems(), vi.fn(), true) + + // The button is the contradiction: a concrete set size here would claim these are all of them. + expect(screen.getByRole('button', { name: /Load more/ })).toBeInTheDocument() + const sizes = announcedSetSizes(container) + expect(sizes).toEqual(sizes.map(() => '-1')) + }) + + it('announces the real row count once the cursor is exhausted', () => { + stubScrollerViewport() + const { container } = renderCollection(pagedItems()) + + expect(screen.queryByRole('button', { name: /Load more/ })).not.toBeInTheDocument() + const sizes = announcedSetSizes(container) + expect(sizes).toEqual(sizes.map(() => String(PAGED_ITEM_COUNT))) + }) + it('shows compact type, size, and expiry in the row', () => { const items = [artifact('first', 'First artifact')] renderCollection(items) diff --git a/src/renderer/src/components/artifacts/ArtifactCollection.tsx b/src/renderer/src/components/artifacts/ArtifactCollection.tsx index 5e1034ce0aa..19c75ee7e46 100644 --- a/src/renderer/src/components/artifacts/ArtifactCollection.tsx +++ b/src/renderer/src/components/artifacts/ArtifactCollection.tsx @@ -37,6 +37,8 @@ export function ArtifactCollection({ // Why: clamp on the way in so a multi-MB paste never reaches state or filtering. const onQueryChange = (next: string): void => setQuery(clampArtifactListSearchQuery(next)) const matches = useMemo(() => filterArtifactsBySearchQuery(artifacts, query), [artifacts, query]) + // Why: state, not a ref — the windowed rows need the scroller on their own mount pass. + const [scrollElement, setScrollElement] = useState(null) return (
@@ -48,19 +50,22 @@ export function ArtifactCollection({ isRefreshing={isRefreshing} />
+ {/* Why no width wrapper around the rows: a `w-fit` band sizes to the full scroll width, + painting every row's hover and selection wash past the viewport edge (376px -> 468px). */} {matches.length > 0 ? ( -
- -
+ ) : (

{translate('auto.components.artifacts.ArtifactCollection.noMatches', 'No matches')} diff --git a/src/renderer/src/components/artifacts/ArtifactListRow.tsx b/src/renderer/src/components/artifacts/ArtifactListRow.tsx new file mode 100644 index 00000000000..29bd79316fc --- /dev/null +++ b/src/renderer/src/components/artifacts/ArtifactListRow.tsx @@ -0,0 +1,193 @@ +import { Fragment } from 'react' +import { Copy, ExternalLink, MoreHorizontal, Trash2 } from 'lucide-react' +import type { LucideIcon } from 'lucide-react' +import type { ArtifactListItem } from '../../../../shared/artifacts' +import { Button } from '@/components/ui/button' +import { + ContextMenu, + ContextMenuContent, + ContextMenuItem, + ContextMenuSeparator, + ContextMenuTrigger +} from '@/components/ui/context-menu' +import { + DropdownMenu, + DropdownMenuContent, + DropdownMenuItem, + DropdownMenuSeparator, + DropdownMenuTrigger +} from '@/components/ui/dropdown-menu' +import { translate } from '@/i18n/i18n' +import { cn } from '@/lib/utils' +import { isPortaledRowMenuClick, isRowActivationKey } from '@/lib/list-row-interaction' +import { + artifactName, + artifactTypeLabel, + formatArtifactExpiryCompact, + formatArtifactUpdatedCompact, + formatByteSize +} from './artifact-display-labels' +import { copyArtifactLink, openArtifactInBrowser } from './artifact-link-actions' +import { ARTIFACTS_TABLE_GRID_CLASS } from './artifacts-table-layout' +import { + LIST_TABLE_ROW_CLASS, + LIST_TABLE_ROW_DIVIDER_CLASS, + LIST_TABLE_ROW_SELECTED_CLASS +} from '@/lib/list-table-layout' + +type ArtifactRowAction = { + key: string + label: string + icon: LucideIcon + onSelect: () => void + /** Rendered after a separator, styled as destructive. */ + destructive?: boolean + disabled?: boolean +} + +// Why: the row dropdown and the row context menu must offer the same actions; one source keeps them from drifting. +function artifactRowActions( + item: ArtifactListItem, + deleting: boolean, + deleteArtifact: (item: ArtifactListItem) => void +): readonly ArtifactRowAction[] { + return [ + { + key: 'copy', + label: translate('auto.components.artifacts.copyLink', 'Copy link'), + icon: Copy, + onSelect: () => void copyArtifactLink(item.shareUrl) + }, + { + key: 'open', + label: translate('auto.components.artifacts.openInBrowser', 'Open in browser'), + icon: ExternalLink, + onSelect: () => openArtifactInBrowser(item.shareUrl) + }, + { + key: 'delete', + label: translate('auto.components.artifacts.ArtifactsPage.deleteArtifact', 'Delete artifact'), + icon: Trash2, + onSelect: () => deleteArtifact(item), + destructive: true, + disabled: deleting + } + ] +} + +export function ArtifactListRow({ + item, + deleting, + isSelected, + showDivider, + selectArtifact, + deleteArtifact +}: { + item: ArtifactListItem + deleting: boolean + isSelected: boolean + /** False on the list-final row only: its bottom edge is the Load more block's top rule, or the + * table container's own border. */ + showDivider: boolean + selectArtifact: (slug: string) => void + deleteArtifact: (item: ArtifactListItem) => void +}): React.JSX.Element { + const name = artifactName(item) + const typeLabel = artifactTypeLabel(item) + const updatedLabel = formatArtifactUpdatedCompact(item.artifact.updatedAt) + const expiryLabel = formatArtifactExpiryCompact(item.artifact.expiresAt) + const sizeLabel = formatByteSize(item.artifact.byteSize) + const rowActions = artifactRowActions(item, deleting, deleteArtifact) + + return ( + // Non-modal, so scrolling is never blocked: windowing unmounting an open menu mid-scroll reads as dismissal. + + +

{ + if (isPortaledRowMenuClick(event)) { + return + } + selectArtifact(item.artifact.slug) + }} + onKeyDown={(event) => { + if (!isRowActivationKey(event)) { + return + } + event.preventDefault() + selectArtifact(item.artifact.slug) + }} + className={cn( + ARTIFACTS_TABLE_GRID_CLASS, + LIST_TABLE_ROW_CLASS, + showDivider && LIST_TABLE_ROW_DIVIDER_CLASS, + isSelected && LIST_TABLE_ROW_SELECTED_CLASS + )} + > + + {name} + + + {typeLabel} + + + {sizeLabel} + + + {updatedLabel} + + + {expiryLabel} + + + + + + + {rowActions.map(({ key, label, icon: Icon, onSelect, destructive, disabled }) => ( + + {destructive ? : null} + + + {label} + + + ))} + + +
+ + + {rowActions.map(({ key, label, icon: Icon, onSelect, destructive, disabled }) => ( + + {destructive ? : null} + + + {label} + + + ))} + + + ) +} diff --git a/src/renderer/src/components/artifacts/ArtifactListRows.tsx b/src/renderer/src/components/artifacts/ArtifactListRows.tsx index 8a1406586e0..a2487a10606 100644 --- a/src/renderer/src/components/artifacts/ArtifactListRows.tsx +++ b/src/renderer/src/components/artifacts/ArtifactListRows.tsx @@ -1,196 +1,58 @@ -import { Fragment } from 'react' -import { Copy, ExternalLink, MoreHorizontal, Trash2 } from 'lucide-react' -import type { LucideIcon } from 'lucide-react' +import { VirtualizedList } from '@/components/virtualized-list' import type { ArtifactListItem } from '../../../../shared/artifacts' -import { Button } from '@/components/ui/button' -import { - ContextMenu, - ContextMenuContent, - ContextMenuItem, - ContextMenuSeparator, - ContextMenuTrigger -} from '@/components/ui/context-menu' -import { - DropdownMenu, - DropdownMenuContent, - DropdownMenuItem, - DropdownMenuSeparator, - DropdownMenuTrigger -} from '@/components/ui/dropdown-menu' -import { translate } from '@/i18n/i18n' -import { cn } from '@/lib/utils' -import { isPortaledRowMenuClick, isRowActivationKey } from '@/lib/list-row-interaction' -import { - artifactName, - artifactTypeLabel, - formatArtifactExpiryCompact, - formatArtifactUpdatedCompact, - formatByteSize -} from './artifact-display-labels' -import { copyArtifactLink, openArtifactInBrowser } from './artifact-link-actions' -import { ARTIFACTS_TABLE_GRID_CLASS } from './artifacts-table-layout' -import { LIST_TABLE_ROW_CLASS, LIST_TABLE_ROW_SELECTED_CLASS } from '@/lib/list-table-layout' - -type ArtifactRowAction = { - key: string - label: string - icon: LucideIcon - onSelect: () => void - /** Rendered after a separator, styled as destructive. */ - destructive?: boolean - disabled?: boolean -} - -// Why: the row dropdown and the row context menu must offer the same actions; one source keeps them from drifting. -function artifactRowActions( - item: ArtifactListItem, - deleting: boolean, - deleteArtifact: (item: ArtifactListItem) => void -): readonly ArtifactRowAction[] { - return [ - { - key: 'copy', - label: translate('auto.components.artifacts.copyLink', 'Copy link'), - icon: Copy, - onSelect: () => void copyArtifactLink(item.shareUrl) - }, - { - key: 'open', - label: translate('auto.components.artifacts.openInBrowser', 'Open in browser'), - icon: ExternalLink, - onSelect: () => openArtifactInBrowser(item.shareUrl) - }, - { - key: 'delete', - label: translate('auto.components.artifacts.ArtifactsPage.deleteArtifact', 'Delete artifact'), - icon: Trash2, - onSelect: () => deleteArtifact(item), - destructive: true, - disabled: deleting - } - ] -} +import { ArtifactListRow } from './ArtifactListRow' +import { ARTIFACTS_TABLE_ROW_HEIGHT_PX } from './artifacts-table-layout' +/** + * The artifacts table body, windowed inside the collection's scroller. Load more only appends, so + * between refreshes the list grows without bound; below the virtualize threshold rows stay in + * natural flow. + */ export function ArtifactListRows({ artifacts, deletingId, selectedSlug, + scrollElement, + hasMore, selectArtifact, deleteArtifact }: { artifacts: readonly ArtifactListItem[] deletingId: string | null selectedSlug: string | null + scrollElement: HTMLDivElement | null + // Why it has to reach the rows: the cursor is what makes the loaded count not the real total, so + // without it every row would announce a set size the Load more button next to it contradicts. + hasMore: boolean selectArtifact: (slug: string) => void deleteArtifact: (item: ArtifactListItem) => void }): React.JSX.Element { - return ( - <> - {artifacts.map((item) => { - const name = artifactName(item) - const typeLabel = artifactTypeLabel(item) - const updatedLabel = formatArtifactUpdatedCompact(item.artifact.updatedAt) - const expiryLabel = formatArtifactExpiryCompact(item.artifact.expiresAt) - const sizeLabel = formatByteSize(item.artifact.byteSize) - const isSelected = selectedSlug === item.artifact.slug - const deleting = deletingId === item.artifact.slug - const rowActions = artifactRowActions(item, deleting, deleteArtifact) + // Why: appended pages are deduped against the slugs already loaded, and the first page's are + // unique per the server, so a slug identifies the last row without an index — which `renderRow` + // does not supply. + const lastSlug = artifacts.at(-1)?.artifact.slug - return ( - - -
{ - if (isPortaledRowMenuClick(event)) { - return - } - selectArtifact(item.artifact.slug) - }} - onKeyDown={(event) => { - if (!isRowActivationKey(event)) { - return - } - event.preventDefault() - selectArtifact(item.artifact.slug) - }} - className={cn( - ARTIFACTS_TABLE_GRID_CLASS, - LIST_TABLE_ROW_CLASS, - isSelected && LIST_TABLE_ROW_SELECTED_CLASS - )} - > - - {name} - - - {typeLabel} - - - {sizeLabel} - - - {updatedLabel} - - - {expiryLabel} - - - - - - - {rowActions.map( - ({ key, label, icon: Icon, onSelect, destructive, disabled }) => ( - - {destructive ? : null} - - - {label} - - - ) - )} - - -
-
- - {rowActions.map(({ key, label, icon: Icon, onSelect, destructive, disabled }) => ( - - {destructive ? : null} - - - {label} - - - ))} - -
- ) - })} - + // Accepted: rows are transform-positioned, so an insert above the viewport slides the list with + // no layout shift for scroll anchoring to correct. + return ( + item.artifact.slug} + renderRow={(item) => ( + + )} + /> ) } diff --git a/src/renderer/src/components/artifacts/artifact-list-windowing.test.tsx b/src/renderer/src/components/artifacts/artifact-list-windowing.test.tsx new file mode 100644 index 00000000000..4723af3d015 --- /dev/null +++ b/src/renderer/src/components/artifacts/artifact-list-windowing.test.tsx @@ -0,0 +1,1099 @@ +// @vitest-environment happy-dom + +import React, { act } from 'react' +import { createRoot, type Root } from 'react-dom/client' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { VIRTUALIZED_LIST_OVERSCAN, VIRTUALIZED_LIST_MIN_ROWS } from '@/components/virtualized-list' +import type { ArtifactListItem } from '../../../../shared/artifacts' +import type * as ArtifactListRowModule from './ArtifactListRow' + +const mountedRows = vi.hoisted(() => ({ + /** Rows mounted right now. */ + count: 0, + /** Slug -> how many times a row for that slug has mounted, across the whole test. */ + mountsBySlug: new Map() +})) + +// Why the real row and not a spy on the virtualizer: a spy would only restate its own +// bookkeeping. Not memoized, because the shipped row is not either. +vi.mock('./ArtifactListRow', async (importOriginal) => { + const actual = await importOriginal() + const react = await import('react') + const Row = actual.ArtifactListRow + function CountingArtifactListRow(props: React.ComponentProps): React.ReactNode { + // Why: the slug this instance first rendered — a later slug on the same instance is reuse, not a mount. + const mountSlug = react.useRef(props.item.artifact.slug) + react.useEffect(() => { + const slug = mountSlug.current + mountedRows.count += 1 + mountedRows.mountsBySlug.set(slug, (mountedRows.mountsBySlug.get(slug) ?? 0) + 1) + return () => { + mountedRows.count -= 1 + } + }, []) + return react.createElement(Row, props) + } + return { ...actual, ArtifactListRow: CountingArtifactListRow } +}) + +const { ArtifactCollection } = await import('./ArtifactCollection') +const { TooltipProvider } = await import('@/components/ui/tooltip') +const { ARTIFACTS_TABLE_ROW_HEIGHT_PX } = await import('./artifacts-table-layout') +const { LIST_TABLE_ROW_DIVIDER_CLASS } = await import('@/lib/list-table-layout') + +const VIEWPORT_HEIGHT_PX = 600 +/** + * Row height the arithmetic below is written against, never assumed: mounted rows measure from + * their own classes (`artifactRowHeightPx`), and the guard test pins that derived height to both + * this number and the virtualizer's estimate for unmounted rows. + */ +const SYNTHETIC_ROW_HEIGHT_PX = 53 +// Synthetic sticky-header height, and so the list's offset inside the scroller. Far larger than +// the real `h-8`, so a lost or wrong scroll margin resolves a visibly different window. +const HEADER_HEIGHT_PX = 300 +const VIRTUAL_SHELL_SELECTOR = '[data-testid="virtualized-list"]' +/** The row box itself: the grid that carries the padding, the divider and the selection wash. */ +const ARTIFACT_ROW_SELECTOR = '[role="button"][tabindex="0"]' +/** + * Literals, not the constants under test: an expectation rebuilt from the same source moves with + * it and can only ever catch a *removed* token. The bug this list shipped with was an added one — + * a `w-fit` band sizing every row's hover and selection wash to the scroll width, not the viewport. + */ +const ARTIFACTS_GRID_CLASS = + 'grid grid-cols-[minmax(0,1.6fr)_minmax(4.5rem,6.5rem)_minmax(4rem,5.5rem)_minmax(6.5rem,9rem)_minmax(6.5rem,9rem)_2.5rem]' +const SCROLLER_CLASS = + 'scrollbar-sleek min-h-0 flex-1 overflow-auto rounded-md border border-border/50 bg-muted/20' +const TABLE_HEADER_CLASS = `${ARTIFACTS_GRID_CLASS} sticky top-0 z-30 h-8 items-center gap-3 border-b border-border/50 bg-[color-mix(in_srgb,var(--muted)_40%,var(--background))] px-3 text-[11px] font-medium uppercase tracking-[0.08em] text-muted-foreground` +/** An unselected row that draws its divider — every windowed row but the list-final one. */ +const ROW_CLASS = `${ARTIFACTS_GRID_CLASS} group/list-table-row w-full min-h-11 scroll-mt-8 cursor-pointer items-center gap-3 px-3 py-3 text-left text-sm transition-colors hover:bg-accent hover:text-accent-foreground focus-visible:outline-none focus-visible:ring-[3px] focus-visible:ring-ring/50 border-b border-border/50` +// Why `data-current` cannot stand in for the wash: it is set from a separate expression, so +// deleting the wash leaves the attribute while the user loses the only visible selection marker. +const SELECTED_ROW_CLASS = `${ROW_CLASS} bg-accent text-accent-foreground` +/** Takes the scroller's visible width, as the old `divide-y` band did; a static block box would even without `w-full`. */ +const VIRTUAL_SHELL_CLASS = 'relative w-full' +/** + * This wrapper, not the shell, sets a windowed row's width: an absolutely positioned box + * shrink-to-fits to the grid's min-content (468px — the template's hard minimums, its gaps and the + * row padding), so without `w-full` it overhangs any narrower scroller's right edge. + */ +const WINDOWED_ROW_WRAPPER_CLASS = 'absolute top-0 left-0 w-full' +/** The list-final row draws no divider, so this rule is its bottom edge. */ +const LOAD_MORE_BLOCK_CLASS = 'border-t border-border/50 p-2' +const DAY_MS = 24 * 60 * 60 * 1000 + +type ResizeObserverBoxSize = { blockSize: number; inlineSize: number } + +const activeResizeObservers = new Set() + +class MockResizeObserver implements ResizeObserver { + readonly elements = new Set() + readonly callback: ResizeObserverCallback + + constructor(callback: ResizeObserverCallback) { + this.callback = callback + activeResizeObservers.add(this) + } + + observe(element: Element): void { + this.elements.add(element) + } + + unobserve(element: Element): void { + this.elements.delete(element) + } + + disconnect(): void { + this.elements.clear() + activeResizeObservers.delete(this) + } +} + +// With a `target`, only observers actually watching that element fire, and only for it — so +// resizing one element proves production observes *that* element, not merely something. +function fireResizeObservers(target?: Element): void { + for (const observer of activeResizeObservers) { + if (target && !observer.elements.has(target)) { + continue + } + const targets = target ? [target] : Array.from(observer.elements) + if (targets.length === 0) { + continue + } + const entries = targets.map((element) => { + const rect = element.getBoundingClientRect() + const size: ResizeObserverBoxSize = { blockSize: rect.height, inlineSize: rect.width } + return { + target: element, + contentRect: rect, + borderBoxSize: [size], + contentBoxSize: [size], + devicePixelContentBoxSize: [size] + } satisfies ResizeObserverEntry + }) + observer.callback(entries, observer) + } +} + +function isObservedByAny(element: Element): boolean { + return Array.from(activeResizeObservers).some((observer) => observer.elements.has(element)) +} + +let host: HTMLDivElement +let root: Root +/** Synthetic layout tops for getBoundingClientRect (happy-dom has no layout). */ +let topsByElement: WeakMap +let scrollTopPatched: WeakSet +/** Current synthetic header height; tests grow it to move the list inside the scroller. */ +let headerHeightPx: number + +/** Tailwind's `text-sm` line box — what a row cell holding only text is tall. */ +const TEXT_SM_LINE_BOX_PX = 20 +/** What a row's `border-b` hairline adds to its border box. */ +const ROW_DIVIDER_HEIGHT_PX = 1 + +// Splits `dark:hover:bg-accent/50` into its variants and the utility they gate. Tracks bracket +// depth because colons inside a variant (`[&_svg:not([class*='size-'])]:size-3`) are not separators. +function splitTailwindToken(token: string): { variants: string[]; utility: string } { + const variants: string[] = [] + let depth = 0 + let start = 0 + for (let index = 0; index < token.length; index += 1) { + const char = token[index] + if (char === '[') { + depth += 1 + } else if (char === ']') { + depth -= 1 + } else if (char === ':' && depth === 0) { + variants.push(token.slice(start, index)) + start = index + 1 + } + } + return { variants, utility: token.slice(start) } +} + +/** `[&_svg…]`, `[&>span…]`: an arbitrary variant that reaches a descendant sizes that child, not this element. */ +function isDescendantScopedVariant(variant: string): boolean { + return /^\[&[_>+~]/.test(variant) +} + +/** + * px for every bare Tailwind spacing token with this prefix (`py-` -> [12] for `py-3`). A variant + * of the same utility (`md:size-9`, `hover:py-4`) throws rather than being skipped: it makes the + * height conditional, and skipping it would quietly resolve to the shorter of the two heights. + */ +function spacingTokensPx(element: Element, prefix: string): number[] { + const steps: number[] = [] + for (const token of element.classList) { + const { variants, utility } = splitTailwindToken(token) + if (!utility.startsWith(prefix)) { + continue + } + if (variants.length > 0 && !variants.every(isDescendantScopedVariant)) { + throw new Error(`variant-prefixed token "${token}" not supported on ${element.className}`) + } + if (variants.length > 0) { + continue + } + const step = Number(utility.slice(prefix.length)) + if (!Number.isFinite(step)) { + throw new Error(`non-numeric ${prefix} token "${token}" on ${element.className}`) + } + steps.push(step * 4) + } + return steps +} + +/** px for the one Tailwind spacing token with this prefix, where exactly one is required. */ +function spacingPx(element: Element, prefix: string): number { + const values = spacingTokensPx(element, prefix) + if (values.length !== 1) { + throw new Error(`expected one ${prefix}N class, got ${values.length} on ${element.className}`) + } + return values[0] ?? 0 +} + +/** Prefixes that can set a box's height. `max-h-` only caps, so it cannot make a row taller. */ +const HEIGHT_PREFIXES = ['size-', 'h-', 'min-h-'] as const + +/** Tallest this element can be, from its own classes: its height tokens, or one text line. */ +function elementBoxHeightPx(element: Element): number { + const explicit = HEIGHT_PREFIXES.flatMap((prefix) => spacingTokensPx(element, prefix)) + return explicit.length > 0 ? Math.max(...explicit) : TEXT_SM_LINE_BOX_PX +} + +/** + * The hairline this row actually draws, or 0. Exact utility match, not a prefix: the colour token + * `border-border/50` beside it also starts with `border-b`, and a variant-gated copy would make + * the height conditional, so it throws rather than resolving to the shorter row. + */ +function rowDividerHeightPx(row: Element): number { + let draws = false + for (const token of row.classList) { + const { variants, utility } = splitTailwindToken(token) + if (utility !== 'border-b') { + continue + } + if (variants.length > 0) { + throw new Error(`variant-prefixed token "${token}" not supported on ${row.className}`) + } + draws = true + } + return draws ? ROW_DIVIDER_HEIGHT_PX : 0 +} + +/** Name | Type | Size | Updated | Expires | Actions — the six tracks of ARTIFACTS_TABLE_GRID_CLASS. */ +const ARTIFACTS_ROW_CELL_COUNT = 6 + +/** Header label -> the value that column must hold, for the `artifact()` fixture at index 3. */ +const LABELLED_ROW_COLUMNS = [ + ['Name', 'Artifact 3'], + ['Type', 'HTML'], + ['Size', '1.2 KB'], + ['Updated', '3 days ago'], + ['Expires', 'in 30 days'] +] as const + +/** + * What the browser would make this row, read off the markup and classes it actually carries: + * padding around the tallest box anywhere inside it, floored by its own min-height, plus whatever + * hairline it draws. Derived, not restated, because `ARTIFACTS_TABLE_ROW_HEIGHT_PX` is the + * virtualizer's height for every unmounted row — at 500 rows an 8px error misreports the scroll + * range by 4000px. Every descendant is scanned, not just the actions button, because any cell that + * outgrows it sets the height, and `LIST_TABLE_ROW_CLASS` is shared with the automations table. + */ +function artifactRowHeightPx(row: Element): number { + const boxes = Array.from(row.querySelectorAll('*')) + // Why the count and not just the heights: a cell over or under the column template wraps the + // grid to a second row, which doubles the height without any box inside it growing. + if (row.children.length !== ARTIFACTS_ROW_CELL_COUNT || boxes.length === 0) { + throw new Error( + `expected ${ARTIFACTS_ROW_CELL_COUNT} artifacts row cells, got ${row.children.length}` + ) + } + const tallestBox = Math.max(...boxes.map(elementBoxHeightPx)) + const padded = 2 * spacingPx(row, 'py-') + tallestBox + return Math.max(padded, spacingPx(row, 'min-h-')) + rowDividerHeightPx(row) +} + +function elementHeight(element: Element): number { + if (element.classList.contains('overflow-auto')) { + return VIEWPORT_HEIGHT_PX + } + if (element.classList.contains('sticky')) { + return headerHeightPx + } + const row = element.matches(ARTIFACT_ROW_SELECTOR) + ? element + : element.querySelector(ARTIFACT_ROW_SELECTOR) + // Why 0 otherwise: nothing else here has a load-bearing height — the Load more block and the + // empty-state paragraph are observed for resizes, never summed into an offset or windowed against. + return row ? artifactRowHeightPx(row) : 0 +} + +beforeEach(() => { + globalThis.IS_REACT_ACT_ENVIRONMENT = true + mountedRows.count = 0 + mountedRows.mountsBySlug.clear() + activeResizeObservers.clear() + topsByElement = new WeakMap() + scrollTopPatched = new WeakSet() + headerHeightPx = HEADER_HEIGHT_PX + host = document.createElement('div') + document.body.appendChild(host) + root = createRoot(host) + + vi.stubGlobal('ResizeObserver', MockResizeObserver) + vi.spyOn(HTMLElement.prototype, 'offsetHeight', 'get').mockImplementation( + function (this: HTMLElement) { + return elementHeight(this) + } + ) + vi.spyOn(Element.prototype, 'getBoundingClientRect').mockImplementation(function (this: Element) { + const top = topsByElement.get(this) ?? 0 + const height = elementHeight(this) + return { + top, + bottom: top + height, + height, + left: 0, + right: 640, + width: 640, + x: 0, + y: top, + toJSON: () => ({}) + } + }) +}) + +afterEach(() => { + act(() => root.unmount()) + host.remove() + activeResizeObservers.clear() + vi.unstubAllGlobals() + vi.restoreAllMocks() +}) + +// Why: relative to now — the row labels are relative times, so fixed dates would rot. The two +// timestamps are far apart so Updated and Expires never render the same string. +function artifact(slug: string, title: string): ArtifactListItem { + const createdAt = new Date(Date.now() - 3 * DAY_MS).toISOString() + return { + artifact: { + version: 1, + slug, + title, + originalFileName: `${slug}.html`, + sourceContentType: 'text/html', + renderedContentType: 'text/html', + createdAt, + updatedAt: createdAt, + expiresAt: new Date(Date.now() + 30 * DAY_MS).toISOString(), + byteSize: 1200, + deletedAt: null + }, + shareUrl: `https://share.onorca.dev/a/${slug}` + } +} + +function artifacts(count: number): ArtifactListItem[] { + return Array.from({ length: count }, (_, index) => artifact(`a-${index}`, `Artifact ${index}`)) +} + +function scroller(): HTMLDivElement { + const node = host.querySelector('.overflow-auto') + if (!node) { + throw new Error('artifacts scroller not rendered') + } + return node +} + +/** The sticky table header — rendered unconditionally, as the scroller's first child. */ +function tableHeader(): HTMLElement { + const header = scroller().querySelector('.sticky') + if (!header) { + throw new Error('artifacts table header not rendered') + } + return header +} + +function virtualShell(): HTMLElement | null { + return host.querySelector(VIRTUAL_SHELL_SELECTOR) +} + +function requireVirtualShell(): HTMLElement { + const shell = virtualShell() + if (!shell) { + throw new Error('virtual shell not rendered') + } + return shell +} + +/** + * Where the browser would put `element` in the scroller's content: the summed heights of the + * in-flow siblings before it. Derived, not assigned — a hardcoded shell top would make every + * scroll-margin assertion restate the number the test chose. + */ +function inFlowTopWithinScroller(element: Element): number { + const scrollerNode = scroller() + let top = 0 + let node: Element | null = element + while (node && node !== scrollerNode) { + for (let prev = node.previousElementSibling; prev; prev = prev.previousElementSibling) { + top += elementHeight(prev) + } + node = node.parentElement + } + return top +} + +function applyShellLayout(): void { + const shell = virtualShell() + if (shell) { + topsByElement.set(shell, inFlowTopWithinScroller(shell) - scroller().scrollTop) + } +} + +/** Real layout the browser would supply: scroller at 0, list below the sticky header. */ +function syncLayout(): void { + const node = scroller() + if (!scrollTopPatched.has(node)) { + scrollTopPatched.add(node) + Object.defineProperty(node, 'scrollTop', { configurable: true, writable: true, value: 0 }) + } + topsByElement.set(node, 0) + applyShellLayout() + act(() => { + fireResizeObservers() + }) +} + +function scrollTo(offset: number): void { + const node = scroller() + node.scrollTop = offset + applyShellLayout() + act(() => { + node.dispatchEvent(new Event('scroll')) + }) +} + +/** Grows the sticky header — the one thing inside the scroller that moves the list's offset. */ +function setHeaderHeight(height: number): void { + headerHeightPx = height + applyShellLayout() + act(() => { + // Only the scroller child that actually changed height reports a resize. + fireResizeObservers(tableHeader()) + }) +} + +// First index the virtualizer must mount at this offset. The sticky header is the shell's only +// in-flow predecessor, so the margin production derives from the DOM comes to `headerHeightPx`; +// dropping or mis-deriving it resolves a window several rows down the list. +function expectedFirstWindowedIndex(scrollOffsetPx: number): number { + const firstVisible = Math.floor((scrollOffsetPx - headerHeightPx) / SYNTHETIC_ROW_HEIGHT_PX) + return Math.max(0, firstVisible - VIRTUALIZED_LIST_OVERSCAN) +} + +function renderCollection({ + items, + selectedSlug = null, + deletingId = null, + hasMore = false, + selectArtifact = vi.fn(), + loadMore = vi.fn() +}: { + items: readonly ArtifactListItem[] + selectedSlug?: string | null + deletingId?: string | null + hasMore?: boolean + selectArtifact?: (slug: string) => void + loadMore?: () => void +}): void { + act(() => { + root.render( + + + + ) + }) + syncLayout() +} + +// React tracks the input's own value, so assigning `input.value` updates that tracker too and the +// change is swallowed as a no-op — the write must go through the setter the tracker delegates to. +function typeSearchQuery(text: string): void { + const input = host.querySelector('input') + const setValue = Object.getOwnPropertyDescriptor(HTMLInputElement.prototype, 'value')?.set + if (!input || !setValue) { + throw new Error('artifacts search field not rendered') + } + act(() => { + setValue.call(input, text) + input.dispatchEvent(new Event('input', { bubbles: true })) + }) + syncLayout() +} + +function windowedIndexes(): number[] { + return Array.from(host.querySelectorAll('[data-index]'), (element) => + Number(element.dataset.index) + ) +} + +function rowAtIndex(index: number): HTMLElement | null { + return host.querySelector(`[data-index="${index}"]`) +} + +/** Clicks Load more and returns it, so a caller can assert it was actually reachable. */ +function clickLoadMore(): HTMLButtonElement { + const button = Array.from(host.querySelectorAll('button')).find((candidate) => + candidate.textContent?.includes('Load more') + ) + if (!button) { + throw new Error('Load more button not rendered') + } + act(() => { + button.dispatchEvent(new MouseEvent('click', { bubbles: true })) + }) + return button +} + +/** + * Whether a windowed row's Delete action is disabled — the row's only rendering of `deleting`. + * Radix mounts the menu only while it is open and portals it out of `host`, so the state cannot be + * read off the row; the menu is toggled back shut so the next row reads its own. + */ +function deleteActionDisabled(index: number): boolean { + const trigger = rowAtIndex(index)?.querySelector('button[aria-label]') + if (!trigger) { + throw new Error(`row ${index} has no actions trigger`) + } + const toggleMenu = (): void => { + act(() => { + trigger.dispatchEvent(new PointerEvent('pointerdown', { bubbles: true, button: 0 })) + }) + } + toggleMenu() + // Radix portals menus to `document.body`, so a leaked open one would leave the lookup below + // free to match the previous row's item. + expect(document.querySelectorAll('[role="menu"]').length).toBe(1) + const items = Array.from(document.querySelectorAll('[role="menuitem"]')) + const remove = items.find((item) => item.textContent?.includes('Delete artifact')) + if (!remove) { + throw new Error(`actions menu for row ${index} has no delete item`) + } + const disabled = remove.getAttribute('aria-disabled') === 'true' + toggleMenu() + return disabled +} + +/** Scroll offset that puts the last of `rowCount` rows at the bottom of the viewport. */ +function bottomScrollOffset(rowCount: number): number { + return HEADER_HEIGHT_PX + rowCount * SYNTHETIC_ROW_HEIGHT_PX - VIEWPORT_HEIGHT_PX +} + +/** + * A row carrying the separator. Both edges: a constant that gained `border-t` would paint two + * hairlines between every pair of rows while every `toBe(true)` here still passed. Colour spelled + * out because Tailwind's default is `currentColor` — a dropped token tints every hairline. + */ +function expectRowDivider(row: Element | null | undefined): void { + expect(row).toBeTruthy() + expect(row?.classList.contains('border-b')).toBe(true) + expect(row?.classList.contains('border-border/50')).toBe(true) + expect(row?.classList.contains('border-t')).toBe(false) +} + +/** The list-final row: no rule on either edge, so nothing doubles the block that follows it. */ +function expectNoRowDivider(row: Element | null | undefined): void { + expect(row).toBeTruthy() + expect(row?.classList.contains('border-b')).toBe(false) + expect(row?.classList.contains('border-t')).toBe(false) +} + +/** + * `text-center` on a direct scroller child centers in the visible width; inside a `w-fit` band or + * with a width utility of its own it would center over the scroll width — partly off screen. + * `min-w-`/`max-w-` floor or cap that box just as `w-` sets it, so all three prefixes are refused. + */ +function expectCenteredEmptyState(): void { + const empty = host.querySelector('p') + expect(empty?.textContent).toContain('No matches') + expect(empty?.parentElement).toBe(scroller()) + expect(empty?.classList.contains('text-center')).toBe(true) + const widthTokens = Array.from(empty?.classList ?? []).filter((token) => + /^(?:min-|max-)?w-/.test(token) + ) + expect(widthTokens).toEqual([]) +} + +function mountCountsSnapshot(): Map { + return new Map(mountedRows.mountsBySlug) +} + +/** Slugs whose row mounted again after `before` — i.e. lost their identity across the update. */ +function slugsRemountedSince(before: ReadonlyMap): string[] { + return Array.from(before) + .filter(([slug, count]) => (mountedRows.mountsBySlug.get(slug) ?? 0) > count) + .map(([slug]) => slug) +} + +describe('artifacts list windowing — below the threshold', () => { + it('renders every row in natural flow with no virtual shell', () => { + const count = VIRTUALIZED_LIST_MIN_ROWS - 10 + renderCollection({ items: artifacts(count) }) + + expect(count).toBe(40) + expect(mountedRows.count).toBe(count) + expect(virtualShell()).toBeNull() + expect(host.querySelectorAll('[data-index]').length).toBe(0) + // Why the structure and not just the count: any band reintroduced around the rows — sized to + // its content, or carrying a rule of its own — would widen the wash or double every hairline, + // and below the threshold nothing else in this file would see it. + const children = Array.from(scroller().children) + expect(children.length).toBe(count + 1) + expect(children[0]).toBe(tableHeader()) + for (const child of children.slice(1)) { + expect(child.matches(ARTIFACT_ROW_SELECTOR)).toBe(true) + } + }) + + it('draws a separator on every row but the last, matching the removed divide-y', () => { + renderCollection({ items: artifacts(VIRTUALIZED_LIST_MIN_ROWS - 10) }) + + const rows = Array.from(host.querySelectorAll('[role="button"][tabindex="0"]')) + expect(rows.length).toBe(40) + for (const row of rows.slice(0, -1)) { + expectRowDivider(row) + } + expectNoRowDivider(rows.at(-1)) + }) + + it('keeps a prepended artifact from remounting the rows already on screen', () => { + const base = artifacts(VIRTUALIZED_LIST_MIN_ROWS - 10) + renderCollection({ items: base }) + const before = mountCountsSnapshot() + expect(before.size).toBe(base.length) + + renderCollection({ items: [artifact('a-new', 'Artifact new'), ...base] }) + + expect(slugsRemountedSince(before)).toEqual([]) + expect(mountedRows.mountsBySlug.get('a-new')).toBe(1) + expect(mountedRows.count).toBe(base.length + 1) + }) +}) + +describe('artifacts list windowing — above the threshold', () => { + // Why not a bare constant comparison: reading the height back off the rendered row's own classes + // fails here on a resized actions button, a `LIST_TABLE_ROW_CLASS` padding change or a dropped + // divider, rather than quietly leaving every unmounted row mis-estimated. + it('pins the production row-height estimate to the geometry the row classes actually describe', () => { + renderCollection({ items: artifacts(500) }) + + const row = host.querySelector(ARTIFACT_ROW_SELECTOR) + expect(row).not.toBeNull() + expect(row && artifactRowHeightPx(row)).toBe(ARTIFACTS_TABLE_ROW_HEIGHT_PX) + expect(ARTIFACTS_TABLE_ROW_HEIGHT_PX).toBe(SYNTHETIC_ROW_HEIGHT_PX) + }) + + // Why the estimate is allowed to be one uniform number: the list-final row draws no divider and + // so is genuinely a pixel shorter, and measurement is what replaces the estimate once that row + // is on screen. Without it the list would run on the estimate forever. + it('hands each windowed row to the virtualizer to measure, and corrects the estimate', () => { + renderCollection({ items: artifacts(500) }) + const shell = requireVirtualShell() + + for (const wrapper of host.querySelectorAll('[data-index]')) { + expect(isObservedByAny(wrapper)).toBe(true) + } + expect(Number.parseInt(shell.style.height, 10)).toBe(500 * SYNTHETIC_ROW_HEIGHT_PX) + + scrollTo(HEADER_HEIGHT_PX + 499 * SYNTHETIC_ROW_HEIGHT_PX) + const lastRow = rowAtIndex(499) + if (!lastRow) { + throw new Error('list-final row not windowed') + } + // Otherwise the correction below would be vacuous: nothing to correct. + expect(elementHeight(lastRow)).toBeLessThan(SYNTHETIC_ROW_HEIGHT_PX) + expect(Number.parseInt(shell.style.height, 10)).toBe(500 * SYNTHETIC_ROW_HEIGHT_PX) + + // Targeted, so only an observer already watching this wrapper can fire: the correction can + // come from nothing but the measurement ref the shared virtual list puts on the wrapper. + act(() => { + fireResizeObservers(lastRow) + }) + expect(Number.parseInt(shell.style.height, 10)).toBe( + 499 * SYNTHETIC_ROW_HEIGHT_PX + elementHeight(lastRow) + ) + }) + + // Why exact: an added token is as damaging as a removed one — `mb-px` alone pushes the real row + // to 54px and invalidates the estimate above, and an opacity token restyles every hairline. + it('pins the row divider to the one bottom hairline', () => { + expect(LIST_TABLE_ROW_DIVIDER_CLASS).toBe('border-b border-border/50') + }) + + it('mounts only a bounded window for a 500-artifact list', () => { + renderCollection({ items: artifacts(500) }) + + const shell = requireVirtualShell() + // 6 visible (600px viewport less the 300px header, over 53px rows) plus 10 overscan. + expect(mountedRows.count).toBe(16) + expect(mountedRows.count).toBeLessThan(VIRTUALIZED_LIST_MIN_ROWS) + expect(host.querySelectorAll('[data-index]').length).toBe(mountedRows.count) + // The scrollbar still represents all 500 rows: mounted rows measure at the height the + // virtualizer estimates unmounted ones at, so the total is exact. + expect(Number.parseInt(shell.style.height, 10)).toBe(500 * SYNTHETIC_ROW_HEIGHT_PX) + + // A screen reader hears all 500 at their true places, not the 16 the window happens to hold. + expect(shell.getAttribute('role')).toBe('list') + scrollTo(HEADER_HEIGHT_PX + 100 * SYNTHETIC_ROW_HEIGHT_PX) + // Deep into the list, so window-relative numbering could not pass as the absolute index. + expect(Math.min(...windowedIndexes())).toBeGreaterThan(mountedRows.count) + expect( + Array.from(host.querySelectorAll('[data-index]'), (wrapper) => [ + wrapper.getAttribute('role'), + wrapper.getAttribute('aria-setsize'), + wrapper.getAttribute('aria-posinset') + ]) + ).toEqual(windowedIndexes().map((index) => ['listitem', '500', String(index + 1)])) + }) + + // Why a literal rather than a shared-token check: it catches an *added* token — `min-w-max` on + // the column template paints the header tint and every row wash out to the full scroll width. + it('keeps the header and the windowed rows on the one shared column template', () => { + renderCollection({ items: artifacts(500) }) + + expect(tableHeader().className).toBe(TABLE_HEADER_CLASS) + const rows = Array.from(host.querySelectorAll(`[data-index] ${ARTIFACT_ROW_SELECTOR}`)) + expect(rows.length).toBe(16) + for (const row of rows) { + expect(row.className).toBe(ROW_CLASS) + } + }) + + // Why the values and not just the tracks: the grid keeps six columns aligned either way, so + // swapping two cells shows the wrong date under the right header — wrong data read as correct. + it('puts each row value under the header that names it', () => { + renderCollection({ items: artifacts(500) }) + + const headers = Array.from(tableHeader().children, (cell) => cell.textContent) + const row = rowAtIndex(3)?.querySelector(ARTIFACT_ROW_SELECTOR) + const values = Array.from(row?.children ?? [], (cell) => cell.textContent).slice(0, -1) + + expect(headers).toEqual([...LABELLED_ROW_COLUMNS.map(([header]) => header), 'Actions']) + expect(values).toEqual(LABELLED_ROW_COLUMNS.map(([, value]) => value)) + // Without distinct values a swapped pair would still satisfy the line above. + expect(new Set(values).size).toBe(LABELLED_ROW_COLUMNS.length) + }) + + it('keeps the header and the virtual shell as the scrollers own children, at the visible width', () => { + renderCollection({ items: artifacts(500) }) + + // Exactly the pre-virtualization structure. A band wrapping the rows and sized to its content + // would widen the row wash and push the scroll range past the viewport edge. + const children = Array.from(scroller().children) + expect(children.length).toBe(2) + expect(children[0]).toBe(tableHeader()) + expect(children[1]).toBe(requireVirtualShell()) + // Pinned, not `toContain`: a presence check cannot see an added width or overflow utility. + expect(scroller().className).toBe(SCROLLER_CLASS) + expect(tableHeader().className).toBe(TABLE_HEADER_CLASS) + expect(requireVirtualShell().className).toBe(VIRTUAL_SHELL_CLASS) + const wrappers = Array.from(host.querySelectorAll('[data-index]')) + expect(wrappers.length).toBe(16) + for (const wrapper of wrappers) { + expect(wrapper.className).toBe(WINDOWED_ROW_WRAPPER_CLASS) + } + }) + + it('separates every windowed row and stops at the list-final row, not the window-final one', () => { + renderCollection({ items: artifacts(500) }) + + const windowedRows = Array.from( + host.querySelectorAll('[data-index] [role="button"]') + ) + expect(windowedRows.length).toBe(16) + for (const row of windowedRows) { + expectRowDivider(row) + } + + // Why: index 499 is nowhere near the first window, so only the slug comparison can find it — + // the removed `divide-y` and any window-relative "is this the last row" check would not. + scrollTo(HEADER_HEIGHT_PX + 499 * SYNTHETIC_ROW_HEIGHT_PX) + expectNoRowDivider(rowAtIndex(499)?.querySelector('[role="button"]')) + for (const index of [497, 498]) { + expectRowDivider(rowAtIndex(index)?.querySelector('[role="button"]')) + } + }) +}) + +describe('artifacts list windowing — scroll margin', () => { + it('windows from the list offset inside the scroller, not from the scroller top', () => { + renderCollection({ items: artifacts(500) }) + + const offset = HEADER_HEIGHT_PX + 200 * SYNTHETIC_ROW_HEIGHT_PX + scrollTo(offset) + + const firstIndex = Math.min(...windowedIndexes()) + expect(firstIndex).toBe(expectedFirstWindowedIndex(offset)) + // Rows position inside the shell, so a windowed row sits at index × row height regardless of offset. + const probe = firstIndex + 3 + expect(rowAtIndex(probe)?.style.transform).toBe( + `translateY(${probe * SYNTHETIC_ROW_HEIGHT_PX}px)` + ) + }) + + it('re-measures and re-windows when the header above the list grows', () => { + renderCollection({ items: artifacts(500) }) + const offset = HEADER_HEIGHT_PX + 200 * SYNTHETIC_ROW_HEIGHT_PX + scrollTo(offset) + expect(Math.min(...windowedIndexes())).toBe(expectedFirstWindowedIndex(offset)) + + const grownHeader = HEADER_HEIGHT_PX + 10 * SYNTHETIC_ROW_HEIGHT_PX + setHeaderHeight(grownHeader) + + // The same scroll offset now lands 10 rows earlier in the list. + expect(Math.min(...windowedIndexes())).toBe(expectedFirstWindowedIndex(offset)) + }) +}) + +describe('artifacts list windowing — row identity', () => { + it('reuses the same virtual shell across insert, remove, and reorder', () => { + const base = artifacts(500) + renderCollection({ items: base }) + const shell = requireVirtualShell() + + renderCollection({ items: [artifact('a-new', 'Artifact new'), ...base] }) + expect(virtualShell()).toBe(shell) + expect(rowAtIndex(0)?.textContent).toContain('Artifact new') + + renderCollection({ items: base.slice(1) }) + expect(virtualShell()).toBe(shell) + expect(rowAtIndex(0)?.textContent).toContain('Artifact 1') + // Still the one window — 6 visible plus 10 overscan — after insert and remove. + expect(mountedRows.count).toBe(16) + + const reordered = [...base.slice(0, 30).toReversed(), ...base.slice(30)] + renderCollection({ items: reordered, selectedSlug: 'a-29' }) + expect(virtualShell()).toBe(shell) + expect(rowAtIndex(0)?.textContent).toContain('Artifact 29') + const current = host.querySelectorAll('[data-current="true"]') + expect(current.length).toBe(1) + expect(rowAtIndex(0)?.contains(current[0] ?? null)).toBe(true) + // The selected class carries no border token, so a selected row keeps its own hairline — and + // this is the only render with a selection, so the only place a `!isSelected` gate would show. + expectRowDivider(current[0]) + }) + + it('keeps every windowed row on its own slug when an artifact is prepended', () => { + // Paired titles, distinct slugs: real artifacts can share a name, and a title key would then + // collide inside the window and hand one artifact's row to the other. + const base = Array.from({ length: 500 }, (_, index) => + artifact(`a-${index}`, `Artifact ${Math.floor(index / 2)}`) + ) + renderCollection({ items: base }) + const before = mountCountsSnapshot() + // More slugs mounted than the 16 on screen: first paint windows before the scroll margin is + // measured, so it briefly fills the whole 600px viewport plus overscan. Bounded, not pinned — + // the claim is only that the transient window stayed far short of the whole list. + expect(before.size).toBeLessThan(VIRTUALIZED_LIST_MIN_ROWS) + expect(before.size).toBeGreaterThanOrEqual(mountedRows.count) + + renderCollection({ items: [artifact('a-new', 'Artifact new'), ...base] }) + + // Slug keys move a row to its new index; index keys would hand its slot to the next artifact. + expect(slugsRemountedSince(before)).toEqual([]) + expect(mountedRows.mountsBySlug.get('a-new')).toBe(1) + expect(rowAtIndex(0)?.textContent).toContain('Artifact new') + }) + + it('gives every windowed row a distinct index', () => { + renderCollection({ items: artifacts(500) }) + + const indexes = windowedIndexes() + // 6 visible plus 10 overscan. + expect(indexes.length).toBe(16) + expect(new Set(indexes).size).toBe(indexes.length) + }) +}) + +describe('artifacts list windowing — surface behaviors', () => { + it('selects from a windowed row', () => { + const selectArtifact = vi.fn() + renderCollection({ items: artifacts(500), selectArtifact }) + + const row = rowAtIndex(3)?.querySelector('[role="button"]') + expect(row).not.toBeNull() + act(() => { + row?.dispatchEvent(new MouseEvent('click', { bubbles: true })) + }) + expect(selectArtifact).toHaveBeenCalledTimes(1) + expect(selectArtifact).toHaveBeenCalledWith('a-3') + }) + + it('disables delete only on the row that is being deleted', () => { + renderCollection({ items: artifacts(500), deletingId: 'a-3' }) + + expect(deleteActionDisabled(3)).toBe(true) + // Conditional, not unconditional: its neighbour in the same window is still deletable. + expect(deleteActionDisabled(4)).toBe(false) + }) + + // The row's second menu, and the only assertion that opens it: its items are mapped from + // `rowActions` separately from the dropdown's, so nothing else here would see them diverge. + it('right-clicking a windowed row offers the same actions as its dropdown', () => { + renderCollection({ items: artifacts(500) }) + scrollTo(HEADER_HEIGHT_PX + 400 * SYNTHETIC_ROW_HEIGHT_PX) + const row = rowAtIndex(400)?.querySelector(ARTIFACT_ROW_SELECTOR) + expect(row).not.toBeNull() + + act(() => { + row?.dispatchEvent(new MouseEvent('contextmenu', { bubbles: true })) + }) + + // Radix mounts menu content only while the menu is open, so a match here is the menu opening. + const labels = Array.from(document.querySelectorAll('[role="menuitem"]'), (i) => i.textContent) + expect(labels).toEqual(['Copy link', 'Open in browser', 'Delete artifact']) + }) + + it('keeps selection while the selected row is outside the window', () => { + renderCollection({ items: artifacts(500), selectedSlug: 'a-400' }) + + expect(host.querySelector('[data-current="true"]')).toBeNull() + expect(rowAtIndex(400)).toBeNull() + + scrollTo(HEADER_HEIGHT_PX + 400 * SYNTHETIC_ROW_HEIGHT_PX) + + const selected = rowAtIndex(400)?.querySelector('[data-current="true"]') + expect(selected).not.toBeNull() + expect(host.querySelectorAll('[data-current="true"]').length).toBe(1) + expect(selected?.className).toBe(SELECTED_ROW_CLASS) + // Conditional, not unconditional: its neighbour in the same window carries no wash. + expect(rowAtIndex(399)?.querySelector(ARTIFACT_ROW_SELECTOR)?.className).toBe(ROW_CLASS) + }) + + it('appends a page without a scroll jump or a shell remount', () => { + const loadMore = vi.fn() + const base = artifacts(500) + renderCollection({ items: base, hasMore: true, loadMore }) + const shell = requireVirtualShell() + + scrollTo(5000) + const indexesBeforeAppend = windowedIndexes() + const topIndex = Math.min(...indexesBeforeAppend) + const topSlugText = rowAtIndex(topIndex)?.textContent + const topTransformBeforeAppend = rowAtIndex(topIndex)?.style.transform + expect(topTransformBeforeAppend).toBeTruthy() + const heightBeforeAppend = Number.parseInt(shell.style.height, 10) + + clickLoadMore() + expect(loadMore).toHaveBeenCalledTimes(1) + + renderCollection({ items: [...base, ...artifacts(600).slice(500)], hasMore: false, loadMore }) + + expect(scroller().scrollTop).toBe(5000) + expect(virtualShell()).toBe(shell) + expect(Number.parseInt(shell.style.height, 10)).toBeGreaterThan(heightBeforeAppend) + expect(rowAtIndex(topIndex)?.textContent).toBe(topSlugText) + // The anti-jump property directly: the same index still sits at the same pixel offset. + expect(rowAtIndex(topIndex)?.style.transform).toBe(topTransformBeforeAppend) + }) + + it('keeps the scroll margin when the Load more block leaves the scroller', () => { + const base = artifacts(500) + renderCollection({ items: base, hasMore: true }) + + // Every scroller child is watched, so any of them resizing re-measures the list's offset. + expect(scroller().children.length).toBe(3) + const loadMoreBlock = scroller().lastElementChild + expect(loadMoreBlock?.textContent).toContain('Load more') + // Why pinned: the list-final row draws no divider of its own, so this top rule is the only + // thing closing the table — and a deleted class is invisible to a containment check. + expect(loadMoreBlock?.className).toBe(LOAD_MORE_BLOCK_CLASS) + expect(isObservedByAny(tableHeader())).toBe(true) + expect(isObservedByAny(requireVirtualShell())).toBe(true) + expect(loadMoreBlock ? isObservedByAny(loadMoreBlock) : false).toBe(true) + + const offset = HEADER_HEIGHT_PX + 200 * SYNTHETIC_ROW_HEIGHT_PX + scrollTo(offset) + expect(Math.min(...windowedIndexes())).toBe(expectedFirstWindowedIndex(offset)) + + renderCollection({ items: base, hasMore: false }) + + expect(scroller().children.length).toBe(2) + expect(scroller().textContent).not.toContain('Load more') + expect(Math.min(...windowedIndexes())).toBe(expectedFirstWindowedIndex(offset)) + expect(mountedRows.count).toBe(windowedIndexes().length) + expect(mountedRows.count).toBeLessThan(VIRTUALIZED_LIST_MIN_ROWS) + }) + + it('shows the empty state with no window and no mounted rows', () => { + renderCollection({ items: [] }) + + expect(host.textContent).toContain('No matches') + // The header renders either way, so the empty state reads as an empty table, not a blank panel. + expect(scroller().children.length).toBe(2) + expect(scroller().firstElementChild).toBe(tableHeader()) + expectCenteredEmptyState() + expect(virtualShell()).toBeNull() + expect(mountedRows.count).toBe(0) + }) + + // Why reachable: `hasMore` tracks the server cursor, `matches` the search-filtered list computed + // inside the collection. A query matching nothing while a next page is pending lands here, so the + // empty text and Load more must coexist — suppressing either leaves a blank panel or a dead end. + it('shows the empty state alongside Load more when a search filters every row away', () => { + renderCollection({ items: [], hasMore: true }) + + const children = Array.from(scroller().children) + expect(children.length).toBe(3) + expect(children[0]).toBe(tableHeader()) + expectCenteredEmptyState() + const loadMoreBlock = children[2] + expect(loadMoreBlock?.textContent).toContain('Load more') + expect(loadMoreBlock?.className).toBe(LOAD_MORE_BLOCK_CLASS) + expect(virtualShell()).toBeNull() + expect(mountedRows.count).toBe(0) + }) +}) + +/** + * Crossing the threshold is the one moment the shared scroller is attached, and on attach the + * virtualizer writes its start offset back to it — a jump to the top unless told where the user + * already is. The 500 -> 600 append above is virtualized on both sides, so it never sees this. + */ +describe('artifacts list windowing — crossing the virtualize threshold', () => { + it('keeps the scroll position when Load more pushes the list over the threshold', () => { + const loadMore = vi.fn() + const base = artifacts(VIRTUALIZED_LIST_MIN_ROWS - 10) + renderCollection({ items: base, hasMore: true, loadMore }) + expect(virtualShell()).toBeNull() + + // Why the bottom and not an arbitrary offset: 40 rows under a 300px header overflow a 600px + // viewport, so Load more is only reachable from here. + const offset = bottomScrollOffset(base.length) + scrollTo(offset) + clickLoadMore() + expect(loadMore).toHaveBeenCalledTimes(1) + + renderCollection({ items: artifacts(80), hasMore: false, loadMore }) + + expect(virtualShell()).not.toBeNull() + expect(scroller().scrollTop).toBe(offset) + // And the user is still looking at the rows they were: the window, not just the raw offset. + expect(Math.min(...windowedIndexes())).toBe(expectedFirstWindowedIndex(offset)) + }) + + it('keeps the scroll position across the exact row count that gates virtualization', () => { + const base = artifacts(VIRTUALIZED_LIST_MIN_ROWS - 1) + renderCollection({ items: base, hasMore: true }) + expect(virtualShell()).toBeNull() + + const offset = bottomScrollOffset(base.length) + scrollTo(offset) + clickLoadMore() + + // One more row is the whole difference: 49 renders plainly, 50 windows. + renderCollection({ items: artifacts(VIRTUALIZED_LIST_MIN_ROWS), hasMore: false }) + + expect(virtualShell()).not.toBeNull() + expect(scroller().scrollTop).toBe(offset) + expect(Math.min(...windowedIndexes())).toBe(expectedFirstWindowedIndex(offset)) + }) + + it('keeps the scroll position when clearing a search restores the list over the threshold', () => { + // 45 of 60 match, so the query takes the list below the threshold and clearing it brings the + // list back over — the same detach and re-attach, reached without loading anything. + const items = Array.from({ length: 60 }, (_, index) => + artifact(`a-${index}`, index < 45 ? `Keep ${index}` : `Drop ${index}`) + ) + renderCollection({ items }) + requireVirtualShell() + + // Inside the filtered list's scroll range too, so the browser would not have clamped it. + const offset = HEADER_HEIGHT_PX + 20 * SYNTHETIC_ROW_HEIGHT_PX + scrollTo(offset) + + typeSearchQuery('keep') + expect(virtualShell()).toBeNull() + expect(mountedRows.count).toBe(45) + + typeSearchQuery('') + + expect(virtualShell()).not.toBeNull() + expect(scroller().scrollTop).toBe(offset) + expect(Math.min(...windowedIndexes())).toBe(expectedFirstWindowedIndex(offset)) + }) +}) diff --git a/src/renderer/src/components/artifacts/artifacts-table-layout.ts b/src/renderer/src/components/artifacts/artifacts-table-layout.ts index d63c00c0602..3a593c03018 100644 --- a/src/renderer/src/components/artifacts/artifacts-table-layout.ts +++ b/src/renderer/src/components/artifacts/artifacts-table-layout.ts @@ -2,3 +2,8 @@ // Name | Type | Size | Updated | Expires | Actions export const ARTIFACTS_TABLE_GRID_CLASS = 'grid grid-cols-[minmax(0,1.6fr)_minmax(4.5rem,6.5rem)_minmax(4rem,5.5rem)_minmax(6.5rem,9rem)_minmax(6.5rem,9rem)_2.5rem]' + +// Why: an `items-center px-3 py-3 text-sm` row whose tallest cell is the `size-7` actions button +// (24px padding + 28px button), plus its own 1px divider. measureElement still corrects, but a +// wrong estimate makes the virtualized list's scrollbar jump on first paint. +export const ARTIFACTS_TABLE_ROW_HEIGHT_PX = 53 diff --git a/src/renderer/src/components/editor/ConflictReviewFileTree.tsx b/src/renderer/src/components/editor/ConflictReviewFileTree.tsx index 0433f9258e9..34031daf829 100644 --- a/src/renderer/src/components/editor/ConflictReviewFileTree.tsx +++ b/src/renderer/src/components/editor/ConflictReviewFileTree.tsx @@ -9,7 +9,7 @@ import { flattenSourceControlTree, type SourceControlTreeNode } from '@/components/right-sidebar/source-control-tree' -import { SourceControlVirtualFileList } from '@/components/right-sidebar/source-control/listing/virtual-file-list' +import { VirtualizedList } from '@/components/virtualized-list' import type { ConflictReviewEntry } from '@/store/slices/editor' import type { GitStatusEntry } from '../../../../shared/git-status-types' import { translate } from '@/i18n/i18n' @@ -103,7 +103,7 @@ export function ConflictReviewFileTree({ )}
) : ( - void }): React.JSX.Element { return ( - { it('mounts every row below the virtualize threshold', () => { - const fileCount = SOURCE_CONTROL_VIRTUALIZE_MIN_ROWS - 10 + const fileCount = VIRTUALIZED_LIST_MIN_ROWS - 10 const directoryCount = 4 renderTree(buildEntries(fileCount, directoryCount)) // `src` plus one directory row per leaf directory, plus one row per file. const totalRows = 1 + directoryCount + fileCount - expect(totalRows).toBeLessThan(SOURCE_CONTROL_VIRTUALIZE_MIN_ROWS) + expect(totalRows).toBeLessThan(VIRTUALIZED_LIST_MIN_ROWS) expect(mountedRows.count).toBe(totalRows) - expect(host.querySelector('[data-testid="source-control-virtual-list"]')).toBeNull() + expect(host.querySelector('[data-testid="virtualized-list"]')).toBeNull() // Natural flow: no absolutely positioned wrappers, exactly the pre-virtualization markup. expect(host.querySelectorAll('[data-index]').length).toBe(0) }) @@ -138,7 +138,7 @@ describe('combined diff file tree row windowing', () => { renderTree(buildEntries(fileCount, directoryCount)) const totalRows = 1 + directoryCount + fileCount - expect(host.querySelector('[data-testid="source-control-virtual-list"]')).not.toBeNull() + expect(host.querySelector('[data-testid="virtualized-list"]')).not.toBeNull() expect(mountedRows.count).toBeGreaterThan(0) // A 600px viewport plus overscan: bounded by the window, not by the review size. expect(mountedRows.count).toBeLessThan(totalRows / 10) diff --git a/src/renderer/src/components/editor/conflict-review-file-tree-windowing.test.tsx b/src/renderer/src/components/editor/conflict-review-file-tree-windowing.test.tsx index 82b2fe05983..e7612a30a39 100644 --- a/src/renderer/src/components/editor/conflict-review-file-tree-windowing.test.tsx +++ b/src/renderer/src/components/editor/conflict-review-file-tree-windowing.test.tsx @@ -3,10 +3,7 @@ import { act } from 'react' import { createRoot, type Root } from 'react-dom/client' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' -import { - SOURCE_CONTROL_FILE_ROW_OVERSCAN, - SOURCE_CONTROL_VIRTUALIZE_MIN_ROWS -} from '@/components/right-sidebar/source-control/listing/virtual-file-list' +import { VIRTUALIZED_LIST_MIN_ROWS, VIRTUALIZED_LIST_OVERSCAN } from '@/components/virtualized-list' import type { GitConflictKind, GitConflictResolutionStatus, @@ -25,7 +22,7 @@ const FILES_PER_DIRECTORY = MERGE_FILE_COUNT / MERGE_DIRECTORY_COUNT // happy-dom has no layout, so the viewport and every row height come from the `offsetHeight` and // `getBoundingClientRect` spies below; `offsetHeight` feeds both virtual-core's viewport rect and // `measureElement`, while `getBoundingClientRect` is read only by -// `measureSourceControlScrollMargin`. The rect spy puts the scroller at `top: 0` and the virtual +// `measureVirtualizedListScrollMargin`. The rect spy puts the scroller at `top: 0` and the virtual // list `MEASURED_SCROLL_MARGIN_PX` into its scrollable content, so the margin this consumer // measures is the real one rather than an accidental 0. const ROWS_PER_VIEWPORT = Math.floor(VIEWPORT_HEIGHT_PX / CONFLICT_REVIEW_ROW_HEIGHT_PX) @@ -80,11 +77,11 @@ afterEach(() => { /** * Synthetic layout top for the rect spy. Everything sits at 0 except the virtual list, which sits * `MEASURED_SCROLL_MARGIN_PX` into the scroller's content: the offset is taken against the - * scroller's current `scrollTop`, which `measureSourceControlScrollMargin` adds back, so a + * scroller's current `scrollTop`, which `measureVirtualizedListScrollMargin` adds back, so a * re-measure mid-scroll reports the same margin as the one at mount. */ function synthesizedTop(element: Element): number { - if (!element.matches('[data-testid="source-control-virtual-list"]')) { + if (!element.matches('[data-testid="virtualized-list"]')) { return 0 } const scroller = element.closest('.overflow-auto') @@ -245,7 +242,7 @@ function expectWindowMatchesProjection(projection: readonly string[]): void { } function getVirtualListContainer(): HTMLElement { - const container = host.querySelector('[data-testid="source-control-virtual-list"]') + const container = host.querySelector('[data-testid="virtualized-list"]') if (!(container instanceof HTMLElement)) { throw new Error('virtual list container not found') } @@ -255,7 +252,7 @@ function getVirtualListContainer(): HTMLElement { /** * The range virtual-core resolves: it ends at the first row whose end reaches or passes the * viewport bottom, so a partly visible last row is still in range; then - * `SOURCE_CONTROL_FILE_ROW_OVERSCAN` extends both edges, clamped to the list. Two + * `VIRTUALIZED_LIST_OVERSCAN` extends both edges, clamped to the list. Two * preconditions: the scroll offset must be row-aligned (a partially scrolled row makes the real * range one row longer), and `CONFLICT_REVIEW_ROW_HEIGHT_PX` must divide `VIEWPORT_HEIGHT_PX` — the * floor in `ROWS_PER_VIEWPORT` keeps the expectation a whole number, but a row height that leaves a @@ -265,10 +262,10 @@ function expectedWindow( startIndex: number, totalRows: number ): { first: number; last: number; count: number } { - const first = Math.max(0, startIndex - SOURCE_CONTROL_FILE_ROW_OVERSCAN) + const first = Math.max(0, startIndex - VIRTUALIZED_LIST_OVERSCAN) const last = Math.min( totalRows - 1, - startIndex + ROWS_PER_VIEWPORT - 1 + SOURCE_CONTROL_FILE_ROW_OVERSCAN + startIndex + ROWS_PER_VIEWPORT - 1 + VIRTUALIZED_LIST_OVERSCAN ) return { first, last, count: last - first + 1 } } @@ -322,9 +319,9 @@ describe('conflict review file tree row windowing', () => { const projection = buildProjection(fileCount, directoryCount) expect(projection.length).toBe(1 + directoryCount + fileCount) - expect(projection.length).toBeLessThan(SOURCE_CONTROL_VIRTUALIZE_MIN_ROWS) + expect(projection.length).toBeLessThan(VIRTUALIZED_LIST_MIN_ROWS) expect(getMountedRows().map(getRowLabel)).toEqual(projection) - expect(host.querySelector('[data-testid="source-control-virtual-list"]')).toBeNull() + expect(host.querySelector('[data-testid="virtualized-list"]')).toBeNull() // Natural flow: no absolutely positioned wrappers, exactly the pre-virtualization markup. expect(host.querySelectorAll('[data-index]')).toHaveLength(0) }) @@ -379,7 +376,7 @@ describe('conflict review file tree row windowing', () => { scroller.dispatchEvent(new Event('scroll')) }) - expect(getMountedWindow()[0]?.index).toBe(250 - SOURCE_CONTROL_FILE_ROW_OVERSCAN - 1) + expect(getMountedWindow()[0]?.index).toBe(250 - VIRTUALIZED_LIST_OVERSCAN - 1) expectWindowMatchesProjection(projection) }) @@ -476,7 +473,7 @@ describe('conflict review file tree row windowing', () => { const collapsed = buildProjection(MERGE_FILE_COUNT, MERGE_DIRECTORY_COUNT, new Set(['dir-04'])) expect(collapsed.length).toBe(461) // Still far above the threshold, so the list cannot have fallen back to natural flow. - expect(collapsed.length).toBeGreaterThanOrEqual(SOURCE_CONTROL_VIRTUALIZE_MIN_ROWS) + expect(collapsed.length).toBeGreaterThanOrEqual(VIRTUALIZED_LIST_MIN_ROWS) expect(getVirtualListContainer().style.height).toBe( `${collapsed.length * CONFLICT_REVIEW_ROW_HEIGHT_PX}px` ) @@ -594,10 +591,10 @@ describe('conflict review file tree row windowing', () => { // Each duplicate is its own row, titled with its full path rather than the shared basename. const projection = ['src', directoryA, pathA, padA, directoryB, pathB, padB] - expect(projection.length + 1).toBeLessThan(SOURCE_CONTROL_VIRTUALIZE_MIN_ROWS) + expect(projection.length + 1).toBeLessThan(VIRTUALIZED_LIST_MIN_ROWS) expect(getMountedRows().map(getRowLabel)).toEqual(projection) // Natural flow has no keyed wrappers, so the row element's own key is all React reconciles by. - expect(host.querySelector('[data-testid="source-control-virtual-list"]')).toBeNull() + expect(host.querySelector('[data-testid="virtualized-list"]')).toBeNull() expect(host.querySelectorAll('[data-index]')).toHaveLength(0) const beforeA = getRow(pathA) @@ -679,6 +676,6 @@ describe('conflict review file tree row windowing', () => { // guard above it the panel would be blank instead of explaining itself. expect(getScroller().textContent).toContain('No conflicts in this snapshot.') expect(getMountedRows()).toHaveLength(0) - expect(host.querySelector('[data-testid="source-control-virtual-list"]')).toBeNull() + expect(host.querySelector('[data-testid="virtualized-list"]')).toBeNull() }) }) diff --git a/src/renderer/src/components/right-sidebar/SourceControl.virtual-file-list.test.tsx b/src/renderer/src/components/right-sidebar/SourceControl.virtual-file-list.test.tsx index ab39363be98..2970693dfe4 100644 --- a/src/renderer/src/components/right-sidebar/SourceControl.virtual-file-list.test.tsx +++ b/src/renderer/src/components/right-sidebar/SourceControl.virtual-file-list.test.tsx @@ -7,10 +7,10 @@ import { TooltipProvider } from '@/components/ui/tooltip' import type { GitStatusEntry } from '../../../../shared/git-status-types' import SourceControl from './SourceControl' import { - SOURCE_CONTROL_FILE_ROW_HEIGHT_PX, - SOURCE_CONTROL_FILE_ROW_OVERSCAN, - SOURCE_CONTROL_VIRTUALIZE_MIN_ROWS -} from './source-control/listing/virtual-file-list' + VIRTUALIZED_LIST_MIN_ROWS, + VIRTUALIZED_LIST_OVERSCAN, + VIRTUALIZED_LIST_ROW_HEIGHT_PX +} from '@/components/virtualized-list' const mocks = vi.hoisted(() => { const activeRepo = { @@ -78,9 +78,7 @@ const VIEWPORT_HEIGHT_PX = 600 // Rows the viewport can show plus overscan on both edges plus the partial // rows clipped at each edge of the window. const MAX_MOUNTED_ROWS = - Math.ceil(VIEWPORT_HEIGHT_PX / SOURCE_CONTROL_FILE_ROW_HEIGHT_PX) + - 2 * SOURCE_CONTROL_FILE_ROW_OVERSCAN + - 2 + Math.ceil(VIEWPORT_HEIGHT_PX / VIRTUALIZED_LIST_ROW_HEIGHT_PX) + 2 * VIRTUALIZED_LIST_OVERSCAN + 2 function gitEntry(overrides: Partial): GitStatusEntry { return { @@ -209,7 +207,7 @@ beforeEach(() => { function (this: HTMLElement) { return this.classList.contains('overflow-auto') ? VIEWPORT_HEIGHT_PX - : SOURCE_CONTROL_FILE_ROW_HEIGHT_PX + : VIRTUALIZED_LIST_ROW_HEIGHT_PX } ) vi.spyOn(Element.prototype, 'getBoundingClientRect').mockImplementation(function (this: Element) { @@ -219,17 +217,7 @@ beforeEach(() => { const isScroller = this.classList.contains('overflow-auto') const scroller = isScroller ? null : this.closest('.overflow-auto') const top = isScroller ? 0 : -(scroller?.scrollTop ?? 0) - return { - top, - bottom: top + SOURCE_CONTROL_FILE_ROW_HEIGHT_PX, - height: SOURCE_CONTROL_FILE_ROW_HEIGHT_PX, - left: 0, - right: 240, - width: 240, - x: 0, - y: top, - toJSON: () => ({}) - } as DOMRect + return new DOMRect(0, top, 240, VIRTUALIZED_LIST_ROW_HEIGHT_PX) }) }) @@ -285,7 +273,7 @@ function row(path: string): HTMLDivElement | null { } function virtualList(): HTMLDivElement | null { - return container.querySelector('[data-testid="source-control-virtual-list"]') + return container.querySelector('[data-testid="virtualized-list"]') } describe('SourceControl virtualized changed-files list', () => { @@ -310,7 +298,7 @@ describe('SourceControl virtualized changed-files list', () => { expect(row('src/file-000.ts')).toBeTruthy() expect(row('src/file-250.ts')).toBeNull() - scrollTo(250 * SOURCE_CONTROL_FILE_ROW_HEIGHT_PX) + scrollTo(250 * VIRTUALIZED_LIST_ROW_HEIGHT_PX) expect(row('src/file-250.ts')).toBeTruthy() expect(row('src/file-000.ts')).toBeNull() @@ -332,7 +320,7 @@ describe('SourceControl virtualized changed-files list', () => { expect(container.textContent).toContain('1 selected') // The selected row scrolls out of the mounted window but stays selected. - scrollTo(240 * SOURCE_CONTROL_FILE_ROW_HEIGHT_PX) + scrollTo(240 * VIRTUALIZED_LIST_ROW_HEIGHT_PX) expect(row('src/file-000.ts')).toBeNull() expect(container.textContent).toContain('1 selected') @@ -371,7 +359,7 @@ describe('SourceControl virtualized changed-files list', () => { }) renderSourceControl() - scrollTo(100 * SOURCE_CONTROL_FILE_ROW_HEIGHT_PX) + scrollTo(100 * VIRTUALIZED_LIST_ROW_HEIGHT_PX) expect(row('src/file-100.ts')).toBeTruthy() const heightBefore = virtualList()?.style.height @@ -384,7 +372,7 @@ describe('SourceControl virtualized changed-files list', () => { } renderSourceControl() - expect(scroller().scrollTop).toBe(100 * SOURCE_CONTROL_FILE_ROW_HEIGHT_PX) + expect(scroller().scrollTop).toBe(100 * VIRTUALIZED_LIST_ROW_HEIGHT_PX) expect(row('src/file-100.ts')).toBeTruthy() expect(virtualList()?.style.height).toBe(heightBefore) }) @@ -398,7 +386,7 @@ describe('SourceControl virtualized changed-files list', () => { }) renderSourceControl() - expect(paths.length).toBeLessThan(SOURCE_CONTROL_VIRTUALIZE_MIN_ROWS) + expect(paths.length).toBeLessThan(VIRTUALIZED_LIST_MIN_ROWS) expect(virtualList()).toBeNull() expect(mountedRows().length).toBe(paths.length) for (const path of paths) { @@ -423,7 +411,7 @@ describe('SourceControl virtualized changed-files list', () => { expect(virtualList()).toBeTruthy() const mounted = container.querySelectorAll( - '[data-testid="source-control-virtual-list"] [data-index]' + '[data-testid="virtualized-list"] [data-index]' ).length expect(mounted).toBeGreaterThan(0) expect(mounted).toBeLessThanOrEqual(MAX_MOUNTED_ROWS) diff --git a/src/renderer/src/components/right-sidebar/source-control/listing/branch-section.tsx b/src/renderer/src/components/right-sidebar/source-control/listing/branch-section.tsx index b96d756a5ef..53e5c0d4ce8 100644 --- a/src/renderer/src/components/right-sidebar/source-control/listing/branch-section.tsx +++ b/src/renderer/src/components/right-sidebar/source-control/listing/branch-section.tsx @@ -12,7 +12,7 @@ import { BranchEntryRow } from './branch-entry-row' import { SectionHeader } from './section-header' import { formatSourceControlRefLabel } from '../panel/branch-context-stats' import { SourceControlBranchTreeDirectoryRow } from './tree-directory-rows' -import { SourceControlVirtualFileList } from './virtual-file-list' +import { VirtualizedList } from '../../../virtualized-list' export function SourceControlBranchSection({ branchSummary, @@ -106,7 +106,7 @@ export function SourceControlBranchSection({ /> {!collapsedSections.has('branch') && (sourceControlViewMode === 'tree' ? ( - node.key} @@ -138,7 +138,7 @@ export function SourceControlBranchSection({ }} /> ) : ( - `branch:${entry.path}`} diff --git a/src/renderer/src/components/right-sidebar/source-control/listing/section-file-list.tsx b/src/renderer/src/components/right-sidebar/source-control/listing/section-file-list.tsx index a958518c7d2..c4efe9091ee 100644 --- a/src/renderer/src/components/right-sidebar/source-control/listing/section-file-list.tsx +++ b/src/renderer/src/components/right-sidebar/source-control/listing/section-file-list.tsx @@ -13,7 +13,7 @@ import { getSourceControlDirectoryActionPaths } from './directory-action-paths' import { SourceControlTreeDirectoryRow } from './tree-directory-rows' import { SubmodulePlaceholderRow } from './submodule-placeholder-row' import { UncommittedEntryRow } from './uncommitted-entry-row' -import { SourceControlVirtualFileList } from './virtual-file-list' +import { VirtualizedList } from '../../../virtualized-list' export function SourceControlSectionFileList({ sourceControlViewMode, @@ -71,7 +71,7 @@ export function SourceControlSectionFileList({ diffCommentCountByPath: Map }): React.JSX.Element { return sourceControlViewMode === 'tree' ? ( - node.key} @@ -134,7 +134,7 @@ export function SourceControlSectionFileList({ }} /> ) : ( - diff --git a/src/renderer/src/components/stats/CodexUsagePane.test.tsx b/src/renderer/src/components/stats/CodexUsagePane.test.tsx new file mode 100644 index 00000000000..0a01673ac63 --- /dev/null +++ b/src/renderer/src/components/stats/CodexUsagePane.test.tsx @@ -0,0 +1,114 @@ +// @vitest-environment happy-dom + +import '@testing-library/jest-dom/vitest' + +import { cleanup, render, screen } from '@testing-library/react' +import { afterEach, describe, expect, it, vi } from 'vitest' +import type { CodexUsageSummary } from '../../../../shared/codex-usage-types' +import type { AppState } from '../../store' +import { CodexUsagePane } from './CodexUsagePane' + +const noop = vi.fn() + +let currentSummary: CodexUsageSummary | null = null + +const mockStoreState = { + codexUsageScanState: { + enabled: true, + isScanning: false, + lastScanStartedAt: 1, + lastScanCompletedAt: 2, + lastScanError: null, + hasAnyCodexData: true + }, + get codexUsageSummary() { + return currentSummary + }, + codexUsageDaily: [], + codexUsageModelBreakdown: [], + codexUsageProjectBreakdown: [], + codexUsageRecentSessions: [], + codexUsageScope: 'orca', + codexUsageRange: '30d', + fetchCodexUsage: noop, + setCodexUsageEnabled: noop, + refreshCodexUsage: noop, + setCodexUsageScope: noop, + setCodexUsageRange: noop, + recordFeatureInteraction: noop +} satisfies Partial + +vi.mock('../../store', () => ({ + useAppStore: (selector: (state: Partial) => unknown) => selector(mockStoreState) +})) + +vi.mock('./CodexUsageDetails', () => ({ + CodexUsageDetails: () =>
details
+})) + +vi.mock('./ShareUsageButton', () => ({ + ShareUsageButton: () => +})) + +vi.mock('@/i18n/i18n', () => ({ + translate: (_key: string, fallback: string) => fallback +})) + +function summaryWithUnpriced( + hasUnpricedModels: boolean, + estimatedCostUsd: number | null = 12.5 +): CodexUsageSummary { + return { + scope: 'orca', + range: '30d', + sessions: 1, + events: 1, + inputTokens: 1000, + cachedInputTokens: 400, + outputTokens: 250, + reasoningOutputTokens: 100, + totalTokens: 1250, + estimatedCostUsd, + hasUnpricedModels, + topModel: 'gpt-6-astra', + topProject: 'Repo', + hasAnyCodexData: true + } +} + +afterEach(() => { + currentSummary = null + cleanup() +}) + +describe('CodexUsagePane estimated cost card', () => { + it('qualifies the total when a named model has no pricing entry', () => { + currentSummary = summaryWithUnpriced(true) + + render() + + expect( + screen.getByText('Est. API-equivalent cost • excludes unpriced models') + ).toBeInTheDocument() + expect(screen.getByText('$12.50')).toBeInTheDocument() + }) + + it('drops the caveat when no model was priced, since there is no remainder to exclude', () => { + currentSummary = summaryWithUnpriced(true, null) + + render() + + expect(screen.getByText('Est. API-equivalent cost')).toBeInTheDocument() + expect(screen.queryByText(/excludes unpriced models/)).not.toBeInTheDocument() + expect(screen.getByText('n/a')).toBeInTheDocument() + }) + + it('leaves the total unqualified when every model is priced', () => { + currentSummary = summaryWithUnpriced(false) + + render() + + expect(screen.getByText('Est. API-equivalent cost')).toBeInTheDocument() + expect(screen.queryByText(/excludes unpriced models/)).not.toBeInTheDocument() + }) +}) diff --git a/src/renderer/src/components/stats/CodexUsagePane.tsx b/src/renderer/src/components/stats/CodexUsagePane.tsx index 1f36402a87c..e2f7c3eeee9 100644 --- a/src/renderer/src/components/stats/CodexUsagePane.tsx +++ b/src/renderer/src/components/stats/CodexUsagePane.tsx @@ -97,6 +97,15 @@ export function CodexUsagePane(): React.JSX.Element { } const hasAnyData = summary?.hasAnyCodexData ?? scanState.hasAnyCodexData + const costLabel = translate( + 'auto.components.stats.CodexUsagePane.1a18fbd56b', + 'Est. API-equivalent cost' + ) + // "Excludes" promises a remainder, so it only qualifies a total that exists. + const costCardLabel = + summary?.hasUnpricedModels && summary.estimatedCostUsd !== null + ? `${costLabel} ${translate('auto.components.stats.CodexUsagePane.costExcludesUnpricedModels', '• excludes unpriced models')}` + : costLabel return ( } /> } /> diff --git a/src/renderer/src/components/stats/usage-overview-model.test.ts b/src/renderer/src/components/stats/usage-overview-model.test.ts index 469340f08b3..39ce675d4b7 100644 --- a/src/renderer/src/components/stats/usage-overview-model.test.ts +++ b/src/renderer/src/components/stats/usage-overview-model.test.ts @@ -79,6 +79,7 @@ describe('usage overview model', () => { reasoningOutputTokens: 300, totalTokens: 3_200, estimatedCostUsd: 0.02, + hasUnpricedModels: false, topModel: 'gpt-5.4', topProject: 'orca-secondary', hasAnyCodexData: true @@ -191,6 +192,37 @@ describe('usage overview model', () => { }) }) + it('marks the overview cost partial when Codex priced some models but not all', () => { + function overviewWithUnpricedCodex(hasUnpricedModels: boolean) { + const codexSummary: CodexUsageSummary = { + scope: 'orca', + range: '30d', + sessions: 1, + events: 3, + inputTokens: 2_000, + cachedInputTokens: 800, + outputTokens: 1_200, + reasoningOutputTokens: 300, + totalTokens: 3_200, + estimatedCostUsd: 0.02, + hasUnpricedModels, + topModel: 'gpt-6-astra', + topProject: 'orca-secondary', + hasAnyCodexData: true + } + return buildUsageOverview({ + claude: { scanState: null, summary: null, daily: [] }, + codex: { scanState: enabledCodexScanState(), summary: codexSummary, daily: [] }, + opencode: { scanState: null, summary: null, daily: [] } + }) + } + + // The provider's own total is a real number, so the null-cost path never fires for it. + expect(overviewWithUnpricedCodex(true).estimatedCostUsd).toBeCloseTo(0.02) + expect(overviewWithUnpricedCodex(true).hasPartialCost).toBe(true) + expect(overviewWithUnpricedCodex(false).hasPartialCost).toBe(false) + }) + it('pads recent usage days with zero-token cells', () => { const recent = getRecentUsageDays( [ diff --git a/src/renderer/src/components/stats/usage-overview-model.ts b/src/renderer/src/components/stats/usage-overview-model.ts index b6a61acabda..9888af073ea 100644 --- a/src/renderer/src/components/stats/usage-overview-model.ts +++ b/src/renderer/src/components/stats/usage-overview-model.ts @@ -34,7 +34,8 @@ export function buildUsageOverview(input: UsageOverviewInput): UsageOverviewMode const knownCost = providers.reduce((sum, provider) => sum + (provider.estimatedCostUsd ?? 0), 0) const hasKnownCost = providers.some((provider) => provider.estimatedCostUsd !== null) const hasPartialCost = providers.some( - (provider) => provider.hasData && provider.estimatedCostUsd === null + (provider) => + provider.hasPartialCost || (provider.hasData && provider.estimatedCostUsd === null) ) const lastUpdatedAt = providers.reduce( diff --git a/src/renderer/src/components/stats/usage-overview-types.ts b/src/renderer/src/components/stats/usage-overview-types.ts index eb04a1f5bfb..eed2659767e 100644 --- a/src/renderer/src/components/stats/usage-overview-types.ts +++ b/src/renderer/src/components/stats/usage-overview-types.ts @@ -33,6 +33,8 @@ export type UsageProviderOverview = { cacheTokens: number reasoningTokens: number estimatedCostUsd: number | null + /** The provider priced some of its tokens but not all, so `estimatedCostUsd` understates the bill. */ + hasPartialCost: boolean topModel: string | null topProject: string | null activeDays: number diff --git a/src/renderer/src/components/stats/usage-provider-normalization.ts b/src/renderer/src/components/stats/usage-provider-normalization.ts index 56c664d4b78..5d8d47da747 100644 --- a/src/renderer/src/components/stats/usage-provider-normalization.ts +++ b/src/renderer/src/components/stats/usage-provider-normalization.ts @@ -45,6 +45,7 @@ export function createClaudeProvider(input: UsageOverviewInput['claude']): Usage cacheTokens: summary ? summary.cacheReadTokens + summary.cacheWriteTokens : 0, reasoningTokens: 0, estimatedCostUsd: summary?.estimatedCostUsd ?? null, + hasPartialCost: false, topModel: summary?.topModel ?? null, topProject: summary?.topProject ?? null, activeDays: countActiveDays(dailyActiveDays) @@ -73,6 +74,7 @@ export function createCodexProvider(input: UsageOverviewInput['codex']): UsagePr cacheTokens: summary?.cachedInputTokens ?? 0, reasoningTokens: summary?.reasoningOutputTokens ?? 0, estimatedCostUsd: summary?.estimatedCostUsd ?? null, + hasPartialCost: summary?.hasUnpricedModels ?? false, topModel: summary?.topModel ?? null, topProject: summary?.topProject ?? null, activeDays: countActiveDays(dailyActiveDays) @@ -103,6 +105,7 @@ export function createOpenCodeProvider( cacheTokens: summary?.cachedInputTokens ?? 0, reasoningTokens: summary?.reasoningOutputTokens ?? 0, estimatedCostUsd: summary?.estimatedCostUsd ?? null, + hasPartialCost: false, topModel: summary?.topModel ?? null, topProject: summary?.topProject ?? null, activeDays: countActiveDays(dailyActiveDays) diff --git a/src/renderer/src/components/right-sidebar/source-control-virtual-file-list.test.tsx b/src/renderer/src/components/virtualized-list.test.tsx similarity index 70% rename from src/renderer/src/components/right-sidebar/source-control-virtual-file-list.test.tsx rename to src/renderer/src/components/virtualized-list.test.tsx index bb84777d984..7320c19ea62 100644 --- a/src/renderer/src/components/right-sidebar/source-control-virtual-file-list.test.tsx +++ b/src/renderer/src/components/virtualized-list.test.tsx @@ -4,12 +4,12 @@ import { act, useState, type ReactElement } from 'react' import { createRoot, type Root } from 'react-dom/client' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { - measureSourceControlScrollMargin, - observeSourceControlScrollMargin, - SOURCE_CONTROL_FILE_ROW_HEIGHT_PX, - SOURCE_CONTROL_VIRTUALIZE_MIN_ROWS, - SourceControlVirtualFileList -} from './source-control/listing/virtual-file-list' + measureVirtualizedListScrollMargin, + observeVirtualizedListScrollMargin, + VIRTUALIZED_LIST_MIN_ROWS, + VIRTUALIZED_LIST_ROW_HEIGHT_PX, + VirtualizedList +} from './virtualized-list' const VIEWPORT_HEIGHT_PX = 600 @@ -18,12 +18,7 @@ type ResizeObserverBoxSize = { inlineSize: number } -type TrackedResizeObserver = { - callback: ResizeObserverCallback - elements: Set -} - -const activeResizeObservers = new Set() +const activeResizeObservers = new Set() class MockResizeObserver implements ResizeObserver { readonly elements = new Set() @@ -72,7 +67,7 @@ function fireResizeObservers(target?: Element): void { devicePixelContentBoxSize: [size] } satisfies ResizeObserverEntry }) - observer.callback(entries, observer as unknown as ResizeObserver) + observer.callback(entries, observer) } } @@ -98,25 +93,15 @@ beforeEach(() => { function (this: HTMLElement) { return this.classList.contains('overflow-auto') ? VIEWPORT_HEIGHT_PX - : SOURCE_CONTROL_FILE_ROW_HEIGHT_PX + : VIRTUALIZED_LIST_ROW_HEIGHT_PX } ) vi.spyOn(Element.prototype, 'getBoundingClientRect').mockImplementation(function (this: Element) { const top = topsByElement.get(this) ?? 0 const height = this.classList.contains('overflow-auto') ? VIEWPORT_HEIGHT_PX - : SOURCE_CONTROL_FILE_ROW_HEIGHT_PX - return { - top, - bottom: top + height, - height, - left: 0, - right: 240, - width: 240, - x: 0, - y: top, - toJSON: () => ({}) - } as DOMRect + : VIRTUALIZED_LIST_ROW_HEIGHT_PX + return new DOMRect(0, top, 240, height) }) }) @@ -135,11 +120,15 @@ function setTop(element: Element, top: number): void { function SharedScrollerHarness({ aboveHeight, rows, - rowTestId = 'virtual-row' + rowTestId = 'virtual-row', + announceListPosition, + hasUnloadedRows }: { aboveHeight: number rows: readonly string[] rowTestId?: string + announceListPosition?: boolean + hasUnloadedRows?: boolean }): ReactElement { const [scroller, setScroller] = useState(null) @@ -177,9 +166,11 @@ function SharedScrollerHarness({ } }} > - row} renderRow={(row) => (
@@ -217,7 +208,7 @@ function MultiSectionHarness({ }} >
- row} @@ -225,7 +216,7 @@ function MultiSectionHarness({ />
- row} @@ -237,7 +228,7 @@ function MultiSectionHarness({ } function syncListTop(aboveHeight: number): HTMLDivElement | null { - const list = host.querySelector('[data-testid="source-control-virtual-list"]') + const list = host.querySelector('[data-testid="virtualized-list"]') if (list) { setTop(list, aboveHeight) } @@ -249,16 +240,16 @@ function syncListTop(aboveHeight: number): HTMLDivElement | null { } function syncMultiSectionListTops(firstRowCount: number): void { - const lists = host.querySelectorAll('[data-testid="source-control-virtual-list"]') + const lists = host.querySelectorAll('[data-testid="virtualized-list"]') if (lists[0]) { setTop(lists[0], 0) } if (lists[1]) { - setTop(lists[1], firstRowCount * SOURCE_CONTROL_FILE_ROW_HEIGHT_PX) + setTop(lists[1], firstRowCount * VIRTUALIZED_LIST_ROW_HEIGHT_PX) } } -describe('measureSourceControlScrollMargin', () => { +describe('measureVirtualizedListScrollMargin', () => { it('returns the list offset inside the scroller independent of scrollTop', () => { const scroller = document.createElement('div') const list = document.createElement('div') @@ -266,16 +257,16 @@ describe('measureSourceControlScrollMargin', () => { setTop(list, 250) Object.defineProperty(scroller, 'scrollTop', { configurable: true, value: 40 }) - expect(measureSourceControlScrollMargin(list, scroller)).toBe(190) + expect(measureVirtualizedListScrollMargin(list, scroller)).toBe(190) Object.defineProperty(scroller, 'scrollTop', { configurable: true, value: 120 }) setTop(list, 170) // list.top dropped by the same amount scrollTop rose → margin unchanged. - expect(measureSourceControlScrollMargin(list, scroller)).toBe(190) + expect(measureVirtualizedListScrollMargin(list, scroller)).toBe(190) }) }) -describe('observeSourceControlScrollMargin', () => { +describe('observeVirtualizedListScrollMargin', () => { it('notifies on resize and disconnects cleanly', () => { const scroller = document.createElement('div') const child = document.createElement('div') @@ -283,7 +274,7 @@ describe('observeSourceControlScrollMargin', () => { scroller.append(child, list) const onLayout = vi.fn() - const cleanup = observeSourceControlScrollMargin(list, scroller, onLayout) + const cleanup = observeVirtualizedListScrollMargin(list, scroller, onLayout) expect(activeResizeObservers.size).toBe(1) fireResizeObservers() @@ -304,7 +295,7 @@ describe('observeSourceControlScrollMargin', () => { scroller.append(removedSibling, list) const onLayout = vi.fn() - const cleanup = observeSourceControlScrollMargin(list, scroller, onLayout) + const cleanup = observeVirtualizedListScrollMargin(list, scroller, onLayout) const observer = Array.from(activeResizeObservers)[0] expect(observer?.elements.has(removedSibling)).toBe(true) onLayout.mockClear() @@ -323,10 +314,10 @@ describe('observeSourceControlScrollMargin', () => { }) }) -describe('SourceControlVirtualFileList scroll-margin lifecycle', () => { +describe('VirtualizedList scroll-margin lifecycle', () => { it('does not read layout during ordinary re-renders after the initial measure', () => { const aboveHeight = 160 - const baseRows = manyRows(SOURCE_CONTROL_VIRTUALIZE_MIN_ROWS) + const baseRows = manyRows(VIRTUALIZED_LIST_MIN_ROWS) act(() => { root.render( @@ -353,7 +344,7 @@ describe('SourceControlVirtualFileList scroll-margin lifecycle', () => { }) it('keeps multi-section windowing correct when content above resizes', () => { - const rows = manyRows(SOURCE_CONTROL_VIRTUALIZE_MIN_ROWS + 20) + const rows = manyRows(VIRTUALIZED_LIST_MIN_ROWS + 20) let aboveHeight = 200 act(() => { @@ -407,10 +398,8 @@ describe('SourceControlVirtualFileList scroll-margin lifecycle', () => { }) it('updates a later virtual section when an earlier virtual section resizes', () => { - let firstRows = manyRows(SOURCE_CONTROL_VIRTUALIZE_MIN_ROWS + 10).map((row) => `first-${row}`) - const secondRows = manyRows(SOURCE_CONTROL_VIRTUALIZE_MIN_ROWS + 20).map( - (row) => `second-${row}` - ) + let firstRows = manyRows(VIRTUALIZED_LIST_MIN_ROWS + 10).map((row) => `first-${row}`) + const secondRows = manyRows(VIRTUALIZED_LIST_MIN_ROWS + 20).map((row) => `second-${row}`) act(() => { root.render() @@ -418,7 +407,7 @@ describe('SourceControlVirtualFileList scroll-margin lifecycle', () => { syncMultiSectionListTops(firstRows.length) act(() => fireResizeObservers()) - firstRows = manyRows(SOURCE_CONTROL_VIRTUALIZE_MIN_ROWS + 80).map((row) => `first-${row}`) + firstRows = manyRows(VIRTUALIZED_LIST_MIN_ROWS + 80).map((row) => `first-${row}`) act(() => { root.render() }) @@ -440,7 +429,7 @@ describe('SourceControlVirtualFileList scroll-margin lifecycle', () => { Object.defineProperty(scroller, 'scrollTop', { configurable: true, writable: true, - value: firstRows.length * SOURCE_CONTROL_FILE_ROW_HEIGHT_PX + value: firstRows.length * VIRTUALIZED_LIST_ROW_HEIGHT_PX }) act(() => scroller.dispatchEvent(new Event('scroll'))) @@ -452,14 +441,97 @@ describe('SourceControlVirtualFileList scroll-margin lifecycle', () => { it('renders small lists without the virtualization shell or observers', () => { const rows = ['a', 'b', 'c'] - expect(rows.length).toBeLessThan(SOURCE_CONTROL_VIRTUALIZE_MIN_ROWS) + expect(rows.length).toBeLessThan(VIRTUALIZED_LIST_MIN_ROWS) act(() => { root.render() }) - expect(host.querySelector('[data-testid="source-control-virtual-list"]')).toBeNull() + expect(host.querySelector('[data-testid="virtualized-list"]')).toBeNull() expect(host.querySelectorAll('[data-testid="plain"]').length).toBe(3) expect(activeResizeObservers.size).toBe(0) }) }) + +// Why not 500 and not a window's worth: a hard-coded set size, or one that happens to equal the +// mounted count, would pass at those lengths. +const ANNOUNCED_ROW_COUNT = VIRTUALIZED_LIST_MIN_ROWS + 23 + +describe('VirtualizedList list-position announcement', () => { + function renderAnnouncing(options: { hasUnloadedRows?: boolean } = {}): HTMLElement[] { + act(() => { + root.render( + + ) + }) + syncListTop(0) + act(() => { + fireResizeObservers() + }) + return Array.from(host.querySelectorAll('[data-index]')) + } + + it('announces the full row count once every row is loaded', () => { + const wrappers = renderAnnouncing() + + expect(wrappers.length).toBeGreaterThan(0) + // The window is a strict subset, so a set size read off the DOM could not reach the real total. + expect(wrappers.length).toBeLessThan(ANNOUNCED_ROW_COUNT) + expect(host.querySelector('[data-testid="virtualized-list"]')?.getAttribute('role')).toBe( + 'list' + ) + expect( + wrappers.map((wrapper) => [ + wrapper.getAttribute('role'), + wrapper.getAttribute('aria-setsize'), + wrapper.getAttribute('aria-posinset') + ]) + ).toEqual( + wrappers.map((wrapper) => [ + 'listitem', + String(ANNOUNCED_ROW_COUNT), + String(Number(wrapper.dataset.index) + 1) + ]) + ) + }) + + it('announces an unknown set size while more rows are still unloaded', () => { + const wrappers = renderAnnouncing({ hasUnloadedRows: true }) + + expect(wrappers.length).toBeGreaterThan(0) + // -1, never the loaded count: a "Load more" control exists precisely because more rows do. + expect(wrappers.map((wrapper) => wrapper.getAttribute('aria-setsize'))).toEqual( + wrappers.map(() => '-1') + ) + // Position is still real — only the total is unknown. + expect(wrappers.map((wrapper) => wrapper.getAttribute('aria-posinset'))).toEqual( + wrappers.map((wrapper) => String(Number(wrapper.dataset.index) + 1)) + ) + }) + + it('leaves the a11y tree untouched for callers that do not opt in', () => { + act(() => { + root.render() + }) + syncListTop(0) + act(() => { + fireResizeObservers() + }) + + const wrappers = Array.from(host.querySelectorAll('[data-index]')) + expect(wrappers.length).toBeGreaterThan(0) + expect(host.querySelector('[data-testid="virtualized-list"]')?.hasAttribute('role')).toBe(false) + for (const wrapper of wrappers) { + expect([ + wrapper.hasAttribute('role'), + wrapper.hasAttribute('aria-setsize'), + wrapper.hasAttribute('aria-posinset') + ]).toEqual([false, false, false]) + } + }) +}) diff --git a/src/renderer/src/components/right-sidebar/source-control/listing/virtual-file-list.tsx b/src/renderer/src/components/virtualized-list.tsx similarity index 60% rename from src/renderer/src/components/right-sidebar/source-control/listing/virtual-file-list.tsx rename to src/renderer/src/components/virtualized-list.tsx index 28634c5630f..2b0acaddd26 100644 --- a/src/renderer/src/components/right-sidebar/source-control/listing/virtual-file-list.tsx +++ b/src/renderer/src/components/virtualized-list.tsx @@ -1,23 +1,16 @@ import { useLayoutEffect, useRef, useState } from 'react' import { useVirtualizer } from '@tanstack/react-virtual' -// Why: below this count plain rows keep the DOM identical to the -// pre-virtualization markup (natural flow, no absolute positioning), so small -// changesets keep exact scrollbar and flicker-free behavior. `STA-351` / -// `STA-1280` jank only appears with hundreds of rows. -export const SOURCE_CONTROL_VIRTUALIZE_MIN_ROWS = 50 -// Why: rows are one py-1 text-xs line, except conflict/submodule rows which -// add a second label line — so estimate the common height and let -// measureElement correct the tall variants. Identical entries measure -// identically, so a git-status refresh cannot move the scroll position. -export const SOURCE_CONTROL_FILE_ROW_HEIGHT_PX = 24 -export const SOURCE_CONTROL_FILE_ROW_OVERSCAN = 10 +// Small lists stay in natural flow; windowing pays off only at larger sizes. +export const VIRTUALIZED_LIST_MIN_ROWS = 50 +export const VIRTUALIZED_LIST_ROW_HEIGHT_PX = 24 +export const VIRTUALIZED_LIST_OVERSCAN = 10 /** * Offset of `container` from the start of `scrollElement`'s scrollable content. * Independent of current scrollTop (relative tops + scrollTop cancel out). */ -export function measureSourceControlScrollMargin( +export function measureVirtualizedListScrollMargin( container: HTMLElement, scrollElement: HTMLElement ): number { @@ -34,7 +27,7 @@ export function measureSourceControlScrollMargin( * Does not observe subtree mutations — virtualized row mount/unmount would * thrash and never change scroll margin. */ -export function observeSourceControlScrollMargin( +export function observeVirtualizedListScrollMargin( container: HTMLElement, scrollElement: HTMLElement, onLayout: () => void @@ -46,7 +39,7 @@ export function observeSourceControlScrollMargin( const observeScrollerChildren = (): void => { const currentChildren = new Set(scrollElement.children) - // Why: child-list churn can detach previously observed sections; pruning + // Why: child-list churn can detach previously observed siblings; pruning // targets prevents the long-lived virtual list from retaining stale DOM. for (const child of observedChildren) { if (!currentChildren.has(child) && child !== container) { @@ -60,7 +53,7 @@ export function observeSourceControlScrollMargin( } observeScrollerChildren() - // Why: sections mount/unmount as direct scroller children; re-observe so a + // Why: list siblings mount/unmount as direct scroller children; re-observe so a // newly inserted sibling can still shift this list's margin when it resizes. const mutationObserver = new MutationObserver(() => { observeScrollerChildren() @@ -75,16 +68,17 @@ export function observeSourceControlScrollMargin( } /** - * Windows one source-control section's rows inside the panel's shared - * scroller. Sections below SOURCE_CONTROL_VIRTUALIZE_MIN_ROWS render plainly; - * larger ones mount only viewport + overscan rows. + * Renders rows in natural flow until the list is large enough to benefit from + * windowing, then mounts only the viewport plus overscan rows. */ -export function SourceControlVirtualFileList({ +export function VirtualizedList({ rows, getRowKey, renderRow, scrollElement, - estimateRowHeightPx = SOURCE_CONTROL_FILE_ROW_HEIGHT_PX + estimateRowHeightPx = VIRTUALIZED_LIST_ROW_HEIGHT_PX, + announceListPosition = false, + hasUnloadedRows }: { rows: readonly TRow[] getRowKey: (row: TRow) => string @@ -96,16 +90,23 @@ export function SourceControlVirtualFileList({ // Why: callers outside source control have their own row paddings; measureElement // still corrects, but a wrong estimate makes the initial scrollbar jump. estimateRowHeightPx?: number + // Why: windowing hides the real row count and each row's place in it from assistive tech, so + // opt in to say both. Virtualized path only — below the threshold this returns a bare fragment, + // with no container to carry the roles, so small lists announce no list at all. Off by default: + // turning it on rewrites every other caller's a11y tree, and list/tree/grid is each surface's call. + announceListPosition?: boolean + // Why: a caller that pages in rows holds fewer than exist, and announcing the loaded count as the + // total is the mis-statement aria-setsize exists to prevent — ARIA spells that unknown total -1. + hasUnloadedRows?: boolean }): React.JSX.Element { const containerRef = useRef(null) const [scrollMargin, setScrollMargin] = useState(0) - const virtualize = rows.length >= SOURCE_CONTROL_VIRTUALIZE_MIN_ROWS + const virtualize = rows.length >= VIRTUALIZED_LIST_MIN_ROWS - // Why: the section shares the panel scroller with the commit area and - // sibling sections, so the virtualizer needs this list's offset inside that - // scroller. Measure on mount / scrollElement attach and when observers - // report layout shifts — never during ordinary React renders (git-status - // polls re-render often and must not force synchronous layout). + // Why: the list shares its scroller with headers and sibling lists, so the + // virtualizer needs this list's offset inside that scroller. Measure on mount + // / scrollElement attach and when observers report layout shifts — never during + // ordinary React renders. useLayoutEffect(() => { if (!virtualize) { return @@ -116,21 +117,26 @@ export function SourceControlVirtualFileList({ } const updateMargin = (): void => { - const nextMargin = measureSourceControlScrollMargin(container, scrollElement) + const nextMargin = measureVirtualizedListScrollMargin(container, scrollElement) setScrollMargin((current) => (current === nextMargin ? current : nextMargin)) } updateMargin() - return observeSourceControlScrollMargin(container, scrollElement, updateMargin) + return observeVirtualizedListScrollMargin(container, scrollElement, updateMargin) }, [scrollElement, virtualize]) const virtualizer = useVirtualizer({ count: rows.length, + // Why the null half: disabled nulls scrollOffset, so initialOffset() cannot latch 0 pre-attach. enabled: virtualize && scrollElement !== null, getScrollElement: () => scrollElement, estimateSize: () => estimateRowHeightPx, - overscan: SOURCE_CONTROL_FILE_ROW_OVERSCAN, + overscan: VIRTUALIZED_LIST_OVERSCAN, scrollMargin, + // Why: crossing the threshold attaches the scroller for the first time and the virtualizer + // writes its start offset back — from 0, scrolling the shared scroller to the top under the + // user, unless told where they already are. + initialOffset: () => scrollElement?.scrollTop ?? 0, // Why: stable row keys let the virtualizer carry item identity across // status refreshes instead of remounting the window each poll. getItemKey: (index) => { @@ -139,6 +145,9 @@ export function SourceControlVirtualFileList({ } }) + // Why not the window's count: the window is what AT must not be able to hear. + const announcedSetSize = hasUnloadedRows ? -1 : rows.length + if (!virtualize) { return <>{rows.map((row) => renderRow(row))} } @@ -146,7 +155,8 @@ export function SourceControlVirtualFileList({ return (
@@ -160,6 +170,9 @@ export function SourceControlVirtualFileList({ key={item.key} ref={virtualizer.measureElement} data-index={item.index} + role={announceListPosition ? 'listitem' : undefined} + aria-setsize={announceListPosition ? announcedSetSize : undefined} + aria-posinset={announceListPosition ? item.index + 1 : undefined} className="absolute top-0 left-0 w-full" // Why: item.start includes scrollMargin (offsets are scroller-wide), // but rows position inside this container, so subtract it back out. diff --git a/src/renderer/src/i18n/locales/en.json b/src/renderer/src/i18n/locales/en.json index 348ed2d300e..7a8eb085539 100644 --- a/src/renderer/src/i18n/locales/en.json +++ b/src/renderer/src/i18n/locales/en.json @@ -4184,7 +4184,8 @@ "rangeLast7Days": "Last 7 days", "rangeLast30Days": "Last 30 days", "rangeLast90Days": "Last 90 days", - "rangeAllTime": "All time" + "rangeAllTime": "All time", + "costExcludesUnpricedModels": "• excludes unpriced models" }, "OpenCodeUsagePane": { "349f7c3f5c": "Total", diff --git a/src/renderer/src/lib/list-table-layout.ts b/src/renderer/src/lib/list-table-layout.ts index 89b85d0aab5..78502fcc604 100644 --- a/src/renderer/src/lib/list-table-layout.ts +++ b/src/renderer/src/lib/list-table-layout.ts @@ -26,3 +26,9 @@ export const LIST_TABLE_STICKY_HEADER_CELL_CLASS = `${LIST_TABLE_STICKY_CELL_BAS // Why: hover/selection ride variants, not props, so the frozen cell tracks the row's own wash. export const LIST_TABLE_STICKY_ROW_CELL_CLASS = `${LIST_TABLE_STICKY_CELL_BASE_CLASS} z-20 bg-[color-mix(in_srgb,var(--muted)_20%,var(--background))] transition-colors group-hover/list-table-row:bg-accent group-data-[current=true]/list-table-row:bg-accent` + +// Why: virtualized rows are absolutely positioned, so a parent `divide-y` would only ever see the +// single virtual container; each row draws its own separator instead. Bottom edge, like `divide-y`: +// the semi-transparent rule composites over the row it belongs to, so a selected row keeps its +// accent wash under its own hairline rather than tinting the one above it. +export const LIST_TABLE_ROW_DIVIDER_CLASS = 'border-b border-border/50' diff --git a/src/renderer/src/store/slices/usage-snapshot-refresh.benchmark.test.ts b/src/renderer/src/store/slices/usage-snapshot-refresh.benchmark.test.ts index 7e2bea1f2ed..edfbf5175fe 100644 --- a/src/renderer/src/store/slices/usage-snapshot-refresh.benchmark.test.ts +++ b/src/renderer/src/store/slices/usage-snapshot-refresh.benchmark.test.ts @@ -56,6 +56,7 @@ function createSummary(totalTokens: number): CodexUsageSummary { reasoningOutputTokens: 0, totalTokens, estimatedCostUsd: 1, + hasUnpricedModels: false, topModel: 'gpt-5', topProject: 'orca', hasAnyCodexData: true diff --git a/src/shared/codex-usage-types.ts b/src/shared/codex-usage-types.ts index 0d4266e93af..2dfece1d228 100644 --- a/src/shared/codex-usage-types.ts +++ b/src/shared/codex-usage-types.ts @@ -22,6 +22,8 @@ export type CodexUsageSummary = { reasoningOutputTokens: number totalTokens: number estimatedCostUsd: number | null + /** A row carried a model name that `MODEL_PRICING` has no entry for, so its tokens are missing from `estimatedCostUsd`. */ + hasUnpricedModels: boolean topModel: string | null topProject: string | null hasAnyCodexData: boolean