From ff13aeeff3fe02667d36d98555e563337b1677f3 Mon Sep 17 00:00:00 2001 From: Joan Leon Date: Mon, 9 Nov 2026 18:11:01 +0100 Subject: [PATCH 1/3] Reject --time-range where unsupported and validate numeric options `--time-range` and `--absolute-timestamps` were global options, so `insights`, `navigations`, `diff` and `scan` accepted them and returned results for the full trace without any warning. Numeric options had no validation: `--top -5`, `--top abc` and `--bucket 0` returned empty results with exit code 0. - Declare `--time-range` and `--absolute-timestamps` only on the commands that filter by time. `diff` keeps `--absolute-timestamps` for its window values. Other commands reject both options through yargs strict mode. - Validate `--top`/`--limit`/`--slowest` (integer >= 1), `--min-duration` (number >= 0) and `--bucket` (duration > 0) with a clear error message. - Extract `loadTrace` and `loadTraceWithTimeRange` helpers to replace the load and time-range block repeated in each command handler. - Document the time range options in the README. Refs #9 --- README.md | 11 +- src/cli.ts | 303 ++++++++++++++++++++++++----------------------- test/cli.test.ts | 197 ++++++++++++++++++++++++++++++ 3 files changed, 362 insertions(+), 149 deletions(-) diff --git a/README.md b/README.md index 6f3242a..caaa897 100644 --- a/README.md +++ b/README.md @@ -250,11 +250,18 @@ trace-engine-cli --help | Option | Type | Default | Description | | :--- | :--- | :--- | :--- | | `-f, --format` | `'toon' \| 'md'` | `'toon'` | Output serialization format (`toon` or `md`) | -| `--time-range` | `string` | — | Filter window relative to trace start (e.g. `"3500,4200"`, `"3.5s,4.2s"`, `"100ms-500ms"`) | -| `--absolute-timestamps` | `boolean` | `false` | Treat time ranges as raw absolute trace timestamps rather than relative to trace start | | `--trace-engine`| `string` | — | Path to a custom DevTools trace engine bundle | | `--help` | `boolean` | — | Show help and synopsis | +### Time Range Options + +These options apply to `overview`, `tasks`, `find`, `profile`, `scheduling`, `network`, `interactions`, `shifts` and `events`. `diff` accepts `--absolute-timestamps` for its `--window-a` and `--window-b` values. Other commands reject both options. + +| Option | Type | Default | Description | +| :--- | :--- | :--- | :--- | +| `--time-range` | `string` | — | Filter window relative to trace start (e.g. `"3500,4200"`, `"3.5s,4.2s"`, `"100ms-500ms"`) | +| `--absolute-timestamps` | `boolean` | `false` | Treat time ranges as raw absolute trace timestamps rather than relative to trace start | + --- ## Relative vs. Absolute Timestamps & Units diff --git a/src/cli.ts b/src/cli.ts index 712cce4..03b454d 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -27,9 +27,14 @@ import { scanTraces } from './engine/extractors/scan.js'; import { extractScheduling } from './engine/extractors/scheduling.js'; import { extractShifts } from './engine/extractors/shifts.js'; import { extractTasks } from './engine/extractors/tasks.js'; -import { loadTraceFromFile } from './engine/loader.js'; +import { type LoadedTrace, loadTraceFromFile } from './engine/loader.js'; import { formatResult } from './formatters/index.js'; -import { parseTimeRange, parseTimeValue, resolveTimeRange } from './utils/filter.js'; +import { + parseTimeRange, + parseTimeValue, + resolveTimeRange, + type TimeRange, +} from './utils/filter.js'; /** * Splits a comma-separated argument into trimmed, non-empty tokens. @@ -45,6 +50,80 @@ function parseCommaList(val?: unknown): string[] | undefined { return items.length > 0 ? items : undefined; } +/** + * Builds a yargs coerce function that accepts only integers >= 1. + */ +function positiveInteger(flag: string): (value: unknown) => number { + return (value) => { + const num = typeof value === 'number' ? value : Number(value); + if (!Number.isInteger(num) || num < 1) { + throw new Error(`--${flag} must be a positive integer. Received: ${value}`); + } + return num; + }; +} + +/** + * Builds a yargs coerce function that accepts only finite numbers >= 0. + */ +function nonNegativeNumber(flag: string): (value: unknown) => number { + return (value) => { + const num = typeof value === 'number' ? value : Number(value); + if (!Number.isFinite(num) || num < 0) { + throw new Error(`--${flag} must be a number >= 0. Received: ${value}`); + } + return num; + }; +} + +/** + * Builds a yargs coerce function that parses a duration with optional unit and accepts only values > 0. + */ +function positiveDuration(flag: string): (value: unknown) => number { + return (value) => { + const ms = parseTimeValue(String(value)); + if (ms <= 0) { + throw new Error(`--${flag} must be a duration > 0. Received: ${value}`); + } + return ms; + }; +} + +// Declared per command, so commands that do not filter by time reject these options. +const TIME_RANGE_OPTION = { + type: 'string', + describe: + 'Time window (relative to trace start by default, e.g. "3500,4200", "3.5s,4.2s", "100ms-500ms")', +} as const; + +const ABSOLUTE_TIMESTAMPS_OPTION = { + type: 'boolean', + default: false, + describe: + 'Treat time ranges as raw absolute trace timestamps rather than relative to trace start (default: false)', +} as const; + +type CliArgs = Record; + +function loadTrace(args: CliArgs): Promise { + return loadTraceFromFile(args.file as string, args.traceEngine as string | undefined); +} + +/** + * Loads the trace and resolves `--time-range` against its bounds. + */ +async function loadTraceWithTimeRange( + args: CliArgs, +): Promise<{ loaded: LoadedTrace; timeRange: TimeRange | undefined }> { + const loaded = await loadTrace(args); + const timeRange = resolveTimeRange( + parseTimeRange(args.timeRange as string | undefined), + loaded.parsedTrace.data.Meta.traceBounds, + args.absoluteTimestamps as boolean, + ); + return { loaded, timeRange }; +} + export async function runCli(argv: string[]): Promise { const cli = yargs(hideBin(argv)) .scriptName('trace-engine-cli') @@ -60,37 +139,20 @@ export async function runCli(argv: string[]): Promise { type: 'string', describe: 'Path to custom trace engine bundle', }) - .option('time-range', { - type: 'string', - describe: - 'Time window (relative to trace start by default, e.g. "3500,4200", "3.5s,4.2s", "100ms-500ms")', - }) - .option('absolute-timestamps', { - type: 'boolean', - default: false, - describe: - 'Treat time ranges as raw absolute trace timestamps rather than relative to trace start (default: false)', - }) .command( ['overview ', '$0 '], 'High-level trace metadata, Core Web Vitals, and main thread category breakdown (default command)', (y) => - y.positional('file', { - describe: 'Path to trace file (.json or .json.gz)', - type: 'string', - demandOption: true, - }), + y + .positional('file', { + describe: 'Path to trace file (.json or .json.gz)', + type: 'string', + demandOption: true, + }) + .option('time-range', TIME_RANGE_OPTION) + .option('absolute-timestamps', ABSOLUTE_TIMESTAMPS_OPTION), async (args) => { - const loaded = await loadTraceFromFile( - args.file as string, - args.traceEngine as string | undefined, - ); - const rawRange = parseTimeRange(args.timeRange as string | undefined); - const timeRange = resolveTimeRange( - rawRange, - loaded.parsedTrace.data.Meta.traceBounds, - args.absoluteTimestamps as boolean, - ); + const { loaded, timeRange } = await loadTraceWithTimeRange(args); const result = extractOverview(loaded, { timeRange }); const isDefaultCmd = !args._.includes('overview'); console.log( @@ -123,10 +185,7 @@ export async function runCli(argv: string[]): Promise { describe: 'Navigation ID to inspect insights for', }), async (args) => { - const loaded = await loadTraceFromFile( - args.file as string, - args.traceEngine as string | undefined, - ); + const loaded = await loadTrace(args); const result = extractInsights(loaded, { category: args.category as | 'lcp' @@ -156,7 +215,8 @@ export async function runCli(argv: string[]): Promise { demandOption: true, }) .option('min-duration', { - type: 'number', + requiresArg: true, + coerce: nonNegativeNumber('min-duration'), default: 50, describe: 'Minimum task duration threshold in milliseconds (default: 50ms)', }) @@ -168,21 +228,15 @@ export async function runCli(argv: string[]): Promise { }) .option('top', { alias: ['limit'], - type: 'number', + requiresArg: true, + coerce: positiveInteger('top'), default: 20, describe: 'Limit output to top N tasks (default: 20)', - }), + }) + .option('time-range', TIME_RANGE_OPTION) + .option('absolute-timestamps', ABSOLUTE_TIMESTAMPS_OPTION), async (args) => { - const loaded = await loadTraceFromFile( - args.file as string, - args.traceEngine as string | undefined, - ); - const rawRange = parseTimeRange(args.timeRange as string | undefined); - const timeRange = resolveTimeRange( - rawRange, - loaded.parsedTrace.data.Meta.traceBounds, - args.absoluteTimestamps as boolean, - ); + const { loaded, timeRange } = await loadTraceWithTimeRange(args); const result = extractTasks(loaded, { minDurationMs: args.minDuration as number, tree: args.tree as 'bottom-up' | 'top-down' | undefined, @@ -215,21 +269,15 @@ export async function runCli(argv: string[]): Promise { }) .option('top', { alias: ['limit'], - type: 'number', + requiresArg: true, + coerce: positiveInteger('top'), default: 50, describe: 'Maximum number of invocation entries to list (default: 50)', - }), + }) + .option('time-range', TIME_RANGE_OPTION) + .option('absolute-timestamps', ABSOLUTE_TIMESTAMPS_OPTION), async (args) => { - const loaded = await loadTraceFromFile( - args.file as string, - args.traceEngine as string | undefined, - ); - const rawRange = parseTimeRange(args.timeRange as string | undefined); - const timeRange = resolveTimeRange( - rawRange, - loaded.parsedTrace.data.Meta.traceBounds, - args.absoluteTimestamps as boolean, - ); + const { loaded, timeRange } = await loadTraceWithTimeRange(args); const result = extractFind(loaded, { functionName: args.function as string, timeRange, @@ -255,30 +303,25 @@ export async function runCli(argv: string[]): Promise { }) .option('bucket', { type: 'string', + requiresArg: true, + coerce: positiveDuration('bucket'), describe: 'Bucket slice duration with optional unit (e.g. "50ms", "0.1s", "50", default: dynamic ~50ms)', }) .option('top', { alias: ['limit'], - type: 'number', + requiresArg: true, + coerce: positiveInteger('top'), default: 3, describe: 'Top N functions to include per time bucket (default: 3)', - }), + }) + .option('time-range', TIME_RANGE_OPTION) + .option('absolute-timestamps', ABSOLUTE_TIMESTAMPS_OPTION), async (args) => { - const loaded = await loadTraceFromFile( - args.file as string, - args.traceEngine as string | undefined, - ); - const rawRange = parseTimeRange(args.timeRange as string | undefined); - const timeRange = resolveTimeRange( - rawRange, - loaded.parsedTrace.data.Meta.traceBounds, - args.absoluteTimestamps as boolean, - ); - const bucketMs = args.bucket ? parseTimeValue(String(args.bucket)) : undefined; + const { loaded, timeRange } = await loadTraceWithTimeRange(args); const result = extractProfile(loaded, { timeRange, - bucketMs, + bucketMs: args.bucket, top: args.top as number, }); console.log( @@ -313,15 +356,14 @@ export async function runCli(argv: string[]): Promise { }) .option('top', { alias: ['limit'], - type: 'number', + requiresArg: true, + coerce: positiveInteger('top'), default: 20, describe: 'Maximum differential entries to return (default: 20)', - }), + }) + .option('absolute-timestamps', ABSOLUTE_TIMESTAMPS_OPTION), async (args) => { - const loaded = await loadTraceFromFile( - args.file as string, - args.traceEngine as string | undefined, - ); + const loaded = await loadTrace(args); const rawRangeA = parseTimeRange(args['window-a'] as string); const rawRangeB = parseTimeRange(args['window-b'] as string); const bounds = loaded.parsedTrace.data.Meta.traceBounds; @@ -351,22 +393,16 @@ export async function runCli(argv: string[]): Promise { 'scheduling ', 'Async task and animation frame scheduling, cancellation, and churn metrics', (y) => - y.positional('file', { - describe: 'Path to trace file (.json or .json.gz)', - type: 'string', - demandOption: true, - }), + y + .positional('file', { + describe: 'Path to trace file (.json or .json.gz)', + type: 'string', + demandOption: true, + }) + .option('time-range', TIME_RANGE_OPTION) + .option('absolute-timestamps', ABSOLUTE_TIMESTAMPS_OPTION), async (args) => { - const loaded = await loadTraceFromFile( - args.file as string, - args.traceEngine as string | undefined, - ); - const rawRange = parseTimeRange(args.timeRange as string | undefined); - const timeRange = resolveTimeRange( - rawRange, - loaded.parsedTrace.data.Meta.traceBounds, - args.absoluteTimestamps as boolean, - ); + const { loaded, timeRange } = await loadTraceWithTimeRange(args); const result = extractScheduling(loaded, { timeRange }); console.log( formatResult(result, { @@ -393,7 +429,8 @@ export async function runCli(argv: string[]): Promise { }) .option('slowest', { alias: ['top', 'limit'], - type: 'number', + requiresArg: true, + coerce: positiveInteger('slowest'), default: 20, describe: 'Limit output to N slowest requests (default: 20)', }) @@ -401,18 +438,11 @@ export async function runCli(argv: string[]): Promise { type: 'string', describe: 'Trace the upstream request initiator chain matching a requestId or URL substring', - }), + }) + .option('time-range', TIME_RANGE_OPTION) + .option('absolute-timestamps', ABSOLUTE_TIMESTAMPS_OPTION), async (args) => { - const loaded = await loadTraceFromFile( - args.file as string, - args.traceEngine as string | undefined, - ); - const rawRange = parseTimeRange(args.timeRange as string | undefined); - const timeRange = resolveTimeRange( - rawRange, - loaded.parsedTrace.data.Meta.traceBounds, - args.absoluteTimestamps as boolean, - ); + const { loaded, timeRange } = await loadTraceWithTimeRange(args); const result = extractNetwork(loaded, { renderBlocking: args.renderBlocking as boolean, slowest: args.slowest as number, @@ -431,22 +461,16 @@ export async function runCli(argv: string[]): Promise { 'interactions ', 'User interaction latencies and target DOM elements', (y) => - y.positional('file', { - describe: 'Path to trace file (.json or .json.gz)', - type: 'string', - demandOption: true, - }), + y + .positional('file', { + describe: 'Path to trace file (.json or .json.gz)', + type: 'string', + demandOption: true, + }) + .option('time-range', TIME_RANGE_OPTION) + .option('absolute-timestamps', ABSOLUTE_TIMESTAMPS_OPTION), async (args) => { - const loaded = await loadTraceFromFile( - args.file as string, - args.traceEngine as string | undefined, - ); - const rawRange = parseTimeRange(args.timeRange as string | undefined); - const timeRange = resolveTimeRange( - rawRange, - loaded.parsedTrace.data.Meta.traceBounds, - args.absoluteTimestamps as boolean, - ); + const { loaded, timeRange } = await loadTraceWithTimeRange(args); const result = extractInteractions(loaded, { timeRange }); console.log( formatResult(result, { @@ -470,18 +494,11 @@ export async function runCli(argv: string[]): Promise { type: 'boolean', default: false, describe: 'Include individual shift events within each cluster', - }), + }) + .option('time-range', TIME_RANGE_OPTION) + .option('absolute-timestamps', ABSOLUTE_TIMESTAMPS_OPTION), async (args) => { - const loaded = await loadTraceFromFile( - args.file as string, - args.traceEngine as string | undefined, - ); - const rawRange = parseTimeRange(args.timeRange as string | undefined); - const timeRange = resolveTimeRange( - rawRange, - loaded.parsedTrace.data.Meta.traceBounds, - args.absoluteTimestamps as boolean, - ); + const { loaded, timeRange } = await loadTraceWithTimeRange(args); const result = extractShifts(loaded, { timeRange, includeEvents: args.events as boolean, @@ -527,21 +544,15 @@ export async function runCli(argv: string[]): Promise { }) .option('top', { alias: ['limit'], - type: 'number', + requiresArg: true, + coerce: positiveInteger('top'), default: 50, describe: 'Maximum number of matching events to return (default: 50)', - }), + }) + .option('time-range', TIME_RANGE_OPTION) + .option('absolute-timestamps', ABSOLUTE_TIMESTAMPS_OPTION), async (args) => { - const loaded = await loadTraceFromFile( - args.file as string, - args.traceEngine as string | undefined, - ); - const rawRange = parseTimeRange(args.timeRange as string | undefined); - const timeRange = resolveTimeRange( - rawRange, - loaded.parsedTrace.data.Meta.traceBounds, - args.absoluteTimestamps as boolean, - ); + const { loaded, timeRange } = await loadTraceWithTimeRange(args); const names = parseCommaList(args.name); const fields = parseCommaList(args.fields); const result = extractEvents(loaded, { @@ -570,10 +581,7 @@ export async function runCli(argv: string[]): Promise { demandOption: true, }), async (args) => { - const loaded = await loadTraceFromFile( - args.file as string, - args.traceEngine as string | undefined, - ); + const loaded = await loadTrace(args); const result = extractNavigations(loaded); console.log( formatResult(result, { @@ -621,7 +629,8 @@ export async function runCli(argv: string[]): Promise { }) .option('top', { alias: ['limit'], - type: 'number', + requiresArg: true, + coerce: positiveInteger('top'), default: 50, describe: 'Maximum number of matched files to return (default: 50)', }), diff --git a/test/cli.test.ts b/test/cli.test.ts index cdcded2..2147da5 100644 --- a/test/cli.test.ts +++ b/test/cli.test.ts @@ -312,3 +312,200 @@ describe('CLI Dispatcher', () => { assert.ok(output.includes('https://example.com/styles.css')); }); }); + +describe('CLI option validation', () => { + const tracePath = path.resolve(import.meta.dirname, 'fixtures/basic-trace.json'); + const navigationTracePath = path.resolve( + import.meta.dirname, + 'fixtures/navigation-cwv-trace.json', + ); + const fixturesDir = path.resolve(import.meta.dirname, 'fixtures'); + + test('rejects --time-range on insights', async () => { + await assert.rejects( + captureCliOutput(['node', 'trace-engine-cli', 'insights', tracePath, '--time-range', '0,10']), + /time-range/, + ); + }); + + test('rejects --time-range on navigations', async () => { + await assert.rejects( + captureCliOutput([ + 'node', + 'trace-engine-cli', + 'navigations', + tracePath, + '--time-range', + '0,10', + ]), + /time-range/, + ); + }); + + test('rejects --time-range on diff', async () => { + await assert.rejects( + captureCliOutput([ + 'node', + 'trace-engine-cli', + 'diff', + tracePath, + '--window-a', + '0,50', + '--window-b', + '50,100', + '--time-range', + '0,10', + ]), + /time-range/, + ); + }); + + test('rejects --time-range on scan', async () => { + await assert.rejects( + captureCliOutput([ + 'node', + 'trace-engine-cli', + 'scan', + fixturesDir, + '--name', + 'RunTask', + '--time-range', + '0,10', + ]), + /time-range/, + ); + }); + + test('rejects --absolute-timestamps on insights', async () => { + await assert.rejects( + captureCliOutput([ + 'node', + 'trace-engine-cli', + 'insights', + tracePath, + '--absolute-timestamps', + ]), + /absolute-timestamps/, + ); + }); + + test('keeps --absolute-timestamps on diff', async () => { + const output = await captureCliOutput([ + 'node', + 'trace-engine-cli', + 'diff', + tracePath, + '--window-a', + '1000,1050', + '--window-b', + '1050,1100', + '--absolute-timestamps', + ]); + assert.ok(output.includes('windowA:')); + }); + + test('rejects --top 0 on tasks', async () => { + await assert.rejects( + captureCliOutput(['node', 'trace-engine-cli', 'tasks', tracePath, '--top', '0']), + /--top must be a positive integer\. Received: 0/, + ); + }); + + test('rejects negative --top on tasks', async () => { + await assert.rejects( + captureCliOutput(['node', 'trace-engine-cli', 'tasks', tracePath, '--top', '-5']), + /--top must be a positive integer\. Received: -5/, + ); + }); + + test('rejects fractional --top on tasks', async () => { + await assert.rejects( + captureCliOutput(['node', 'trace-engine-cli', 'tasks', tracePath, '--top', '1.5']), + /--top must be a positive integer\. Received: 1\.5/, + ); + }); + + test('rejects non-numeric --top on tasks', async () => { + await assert.rejects( + captureCliOutput(['node', 'trace-engine-cli', 'tasks', tracePath, '--top', 'abc']), + /--top must be a positive integer\. Received: abc/, + ); + }); + + test('rejects --top 0 on profile', async () => { + await assert.rejects( + captureCliOutput(['node', 'trace-engine-cli', 'profile', tracePath, '--top', '0']), + /--top must be a positive integer\. Received: 0/, + ); + }); + + test('rejects negative --limit alias on events', async () => { + await assert.rejects( + captureCliOutput(['node', 'trace-engine-cli', 'events', tracePath, '--limit', '-1']), + /--top must be a positive integer\. Received: -1/, + ); + }); + + test('rejects --slowest 0 on network', async () => { + await assert.rejects( + captureCliOutput([ + 'node', + 'trace-engine-cli', + 'network', + navigationTracePath, + '--slowest', + '0', + ]), + /--slowest must be a positive integer\. Received: 0/, + ); + }); + + test('rejects --top 0 on scan', async () => { + await assert.rejects( + captureCliOutput([ + 'node', + 'trace-engine-cli', + 'scan', + fixturesDir, + '--name', + 'RunTask', + '--top', + '0', + ]), + /--top must be a positive integer\. Received: 0/, + ); + }); + + test('rejects negative --min-duration on tasks', async () => { + await assert.rejects( + captureCliOutput(['node', 'trace-engine-cli', 'tasks', tracePath, '--min-duration', '-1']), + /--min-duration must be a number >= 0\. Received: -1/, + ); + }); + + test('accepts --min-duration 0 on tasks', async () => { + const output = await captureCliOutput([ + 'node', + 'trace-engine-cli', + 'tasks', + tracePath, + '--min-duration', + '0', + ]); + assert.ok(output.includes('mode: list')); + }); + + test('rejects --bucket 0 on profile', async () => { + await assert.rejects( + captureCliOutput(['node', 'trace-engine-cli', 'profile', tracePath, '--bucket', '0']), + /--bucket must be a duration > 0\. Received: 0/, + ); + }); + + test('rejects negative --bucket on profile', async () => { + await assert.rejects( + captureCliOutput(['node', 'trace-engine-cli', 'profile', tracePath, '--bucket', '-5ms']), + /--bucket must be a duration > 0\. Received: -5ms/, + ); + }); +}); From 74249886c2b3807d06db7f4d5e7a5db05f9241f7 Mon Sep 17 00:00:00 2001 From: Joan Leon Date: Mon, 9 Nov 2026 22:38:57 +0100 Subject: [PATCH 2/3] Remove redundant comments from CLI helpers --- src/cli.ts | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/src/cli.ts b/src/cli.ts index 03b454d..ec659f9 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -50,9 +50,6 @@ function parseCommaList(val?: unknown): string[] | undefined { return items.length > 0 ? items : undefined; } -/** - * Builds a yargs coerce function that accepts only integers >= 1. - */ function positiveInteger(flag: string): (value: unknown) => number { return (value) => { const num = typeof value === 'number' ? value : Number(value); @@ -63,9 +60,6 @@ function positiveInteger(flag: string): (value: unknown) => number { }; } -/** - * Builds a yargs coerce function that accepts only finite numbers >= 0. - */ function nonNegativeNumber(flag: string): (value: unknown) => number { return (value) => { const num = typeof value === 'number' ? value : Number(value); @@ -76,9 +70,6 @@ function nonNegativeNumber(flag: string): (value: unknown) => number { }; } -/** - * Builds a yargs coerce function that parses a duration with optional unit and accepts only values > 0. - */ function positiveDuration(flag: string): (value: unknown) => number { return (value) => { const ms = parseTimeValue(String(value)); @@ -109,9 +100,6 @@ function loadTrace(args: CliArgs): Promise { return loadTraceFromFile(args.file as string, args.traceEngine as string | undefined); } -/** - * Loads the trace and resolves `--time-range` against its bounds. - */ async function loadTraceWithTimeRange( args: CliArgs, ): Promise<{ loaded: LoadedTrace; timeRange: TimeRange | undefined }> { From 2e829a02088085486c5b09041f8d600e3479b217 Mon Sep 17 00:00:00 2001 From: Joan Leon Date: Sat, 14 Nov 2026 00:18:08 +0100 Subject: [PATCH 3/3] Reject profile buckets below 0.01ms or above 1000 buckets Bucket bounds are rounded to 2 decimals, so a bucket below 0.01ms returned an empty profile with exit 0. Reject it in the --bucket coercion. Also fail when the time range needs more than 1000 buckets, and suggest the smallest valid bucket size in the error. --- README.md | 2 ++ src/cli.ts | 18 ++++++++---------- src/engine/extractors/profile.ts | 13 +++++++++++++ test/cli.test.ts | 18 ++++++++++++++++-- test/extractors/profile.test.ts | 26 ++++++++++++++++++++++++++ 5 files changed, 65 insertions(+), 12 deletions(-) diff --git a/README.md b/README.md index caaa897..40077bc 100644 --- a/README.md +++ b/README.md @@ -396,6 +396,8 @@ trace-engine-cli find trace.json.gz --function "drawEventTiles" --time-range "3. ### 5. `profile` Time-bucketed CPU profiling progression across discrete slices (e.g. 50ms buckets) to analyze temporal shifts in CPU consumption during gestures. +`--bucket` must be at least `0.01ms`. A profile can have a maximum of 1000 buckets. If the time range divided by the bucket size is more than 1000, the command fails and shows the smallest valid bucket size. + ```bash # Profile with automatic bucket sizing trace-engine-cli profile trace.json.gz --time-range "3.5s,4.1s" diff --git a/src/cli.ts b/src/cli.ts index ec659f9..5c43c6c 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -22,7 +22,7 @@ import { extractInteractions } from './engine/extractors/interactions.js'; import { extractNavigations } from './engine/extractors/navigations.js'; import { extractNetwork } from './engine/extractors/network.js'; import { extractOverview } from './engine/extractors/overview.js'; -import { extractProfile } from './engine/extractors/profile.js'; +import { extractProfile, MIN_BUCKET_MS } from './engine/extractors/profile.js'; import { scanTraces } from './engine/extractors/scan.js'; import { extractScheduling } from './engine/extractors/scheduling.js'; import { extractShifts } from './engine/extractors/shifts.js'; @@ -70,14 +70,12 @@ function nonNegativeNumber(flag: string): (value: unknown) => number { }; } -function positiveDuration(flag: string): (value: unknown) => number { - return (value) => { - const ms = parseTimeValue(String(value)); - if (ms <= 0) { - throw new Error(`--${flag} must be a duration > 0. Received: ${value}`); - } - return ms; - }; +function bucketDuration(value: unknown): number { + const ms = parseTimeValue(String(value)); + if (ms < MIN_BUCKET_MS) { + throw new Error(`--bucket must be a duration >= ${MIN_BUCKET_MS}ms. Received: ${value}`); + } + return ms; } // Declared per command, so commands that do not filter by time reject these options. @@ -292,7 +290,7 @@ export async function runCli(argv: string[]): Promise { .option('bucket', { type: 'string', requiresArg: true, - coerce: positiveDuration('bucket'), + coerce: bucketDuration, describe: 'Bucket slice duration with optional unit (e.g. "50ms", "0.1s", "50", default: dynamic ~50ms)', }) diff --git a/src/engine/extractors/profile.ts b/src/engine/extractors/profile.ts index 03180a8..4e9a9fc 100644 --- a/src/engine/extractors/profile.ts +++ b/src/engine/extractors/profile.ts @@ -18,6 +18,10 @@ import { getMainRendererThread } from '../helpers.js'; import type { LoadedTrace } from '../loader.js'; import type { ProfileResult } from '../types.js'; +// Bucket bounds are rounded to 2 decimals, so smaller buckets produce no slices. +export const MIN_BUCKET_MS = 0.01; +export const MAX_BUCKETS = 1000; + export interface ExtractProfileOptions { timeRange?: TimeRange; bucketMs?: number; @@ -42,6 +46,15 @@ export function extractProfile( const bucketSizeMs = options?.bucketMs ?? Math.max(10, round(totalRangeMs / 20)); const topCount = options?.top ?? 3; + const rangeMs = endMs - startMs; + if (bucketSizeMs < rangeMs / MAX_BUCKETS) { + const bucketCount = Math.ceil(rangeMs / bucketSizeMs); + const minBucketMs = Math.ceil((rangeMs / MAX_BUCKETS) * 100) / 100; + throw new Error( + `Bucket size ${bucketSizeMs}ms creates ${bucketCount} buckets over ${round(rangeMs)}ms. The maximum is ${MAX_BUCKETS}. Use a bucket size of at least ${minBucketMs}ms.`, + ); + } + const result: ProfileResult = { timeRange: { startMs, endMs }, bucketSizeMs, diff --git a/test/cli.test.ts b/test/cli.test.ts index 2147da5..8a78be5 100644 --- a/test/cli.test.ts +++ b/test/cli.test.ts @@ -498,14 +498,28 @@ describe('CLI option validation', () => { test('rejects --bucket 0 on profile', async () => { await assert.rejects( captureCliOutput(['node', 'trace-engine-cli', 'profile', tracePath, '--bucket', '0']), - /--bucket must be a duration > 0\. Received: 0/, + /--bucket must be a duration >= 0\.01ms\. Received: 0/, ); }); test('rejects negative --bucket on profile', async () => { await assert.rejects( captureCliOutput(['node', 'trace-engine-cli', 'profile', tracePath, '--bucket', '-5ms']), - /--bucket must be a duration > 0\. Received: -5ms/, + /--bucket must be a duration >= 0\.01ms\. Received: -5ms/, + ); + }); + + test('rejects --bucket below 0.01ms on profile', async () => { + await assert.rejects( + captureCliOutput(['node', 'trace-engine-cli', 'profile', tracePath, '--bucket', '1us']), + /--bucket must be a duration >= 0\.01ms\. Received: 1us/, + ); + }); + + test('rejects --bucket that creates more than 1000 buckets on profile', async () => { + await assert.rejects( + captureCliOutput(['node', 'trace-engine-cli', 'profile', tracePath, '--bucket', '10us']), + /Bucket size 0\.01ms creates 16000 buckets over 160ms\. The maximum is 1000\./, ); }); }); diff --git a/test/extractors/profile.test.ts b/test/extractors/profile.test.ts index 3f941e5..6d585f7 100644 --- a/test/extractors/profile.test.ts +++ b/test/extractors/profile.test.ts @@ -80,4 +80,30 @@ describe('Profile Extractor', () => { assert.ok(functionNames.includes('Paint')); assert.ok(functionNames.includes('RunTask')); }); + + test('rejects bucket sizes that create more than 1000 buckets', async () => { + const fixturePath = path.resolve(import.meta.dirname, '../fixtures/basic-trace.json'); + const loaded = await loadTraceFromFile(fixturePath); + + assert.throws( + () => + extractProfile(loaded, { + timeRange: { startMs: 1000, endMs: 1300 }, + bucketMs: 0.29, + }), + /Bucket size 0\.29ms creates 1035 buckets over 300ms\. The maximum is 1000\. Use a bucket size of at least 0\.3ms\./, + ); + }); + + test('accepts a bucket size that creates exactly 1000 buckets', async () => { + const fixturePath = path.resolve(import.meta.dirname, '../fixtures/basic-trace.json'); + const loaded = await loadTraceFromFile(fixturePath); + + const result = extractProfile(loaded, { + timeRange: { startMs: 1000, endMs: 1300 }, + bucketMs: 0.3, + }); + + assert.equal(result.buckets.length, 1000); + }); });