diff --git a/src/commands/issue/list.ts b/src/commands/issue/list.ts index 13831ecbe..f97fccb1b 100644 --- a/src/commands/issue/list.ts +++ b/src/commands/issue/list.ts @@ -175,17 +175,45 @@ function shouldCollapseStats(json: boolean): boolean { return !willShowTrend(); } +/** + * Fields that depend on the `lifetime` API data. When `collapse=lifetime` + * is sent, the server omits these from the list response. See #969. + */ +const LIFETIME_FIELDS = new Set([ + "count", + "userCount", + "firstSeen", + "lastSeen", +]); + /** * Build the collapse and groupStatsPeriod options for issue list API calls. * * When stats are collapsed, groupStatsPeriod is omitted (undefined) since * the server won't compute stats anyway. This avoids wasted server-side * processing and makes the request intent explicit. + * + * Lifetime is only collapsed in JSON mode when explicit `--fields` are + * provided and none of them are lifetime-dependent (`count`, `userCount`, + * `firstSeen`, `lastSeen`). Human output always needs these for the + * EVENTS, USERS, SEEN, and AGE columns. */ -function buildListApiOptions(json: boolean): ListApiOptions { +function buildListApiOptions(json: boolean, fields?: string[]): ListApiOptions { const collapseStats = shouldCollapseStats(json); + // Collapse lifetime only when in JSON mode with explicit --fields that + // don't include any lifetime-dependent field. Human output always needs + // these (EVENTS, USERS, SEEN, AGE columns), and JSON without --fields + // returns all fields. + const collapseLifetime = + json && + fields !== undefined && + fields.length > 0 && + !fields.some((f) => LIFETIME_FIELDS.has(f)); return { - collapse: buildIssueListCollapse({ shouldCollapseStats: collapseStats }), + collapse: buildIssueListCollapse({ + shouldCollapseStats: collapseStats, + shouldCollapseLifetime: collapseLifetime, + }), groupStatsPeriod: collapseStats ? undefined : "auto", }; } @@ -870,14 +898,14 @@ function prevPageHint(org: string, flags: ListFlags): string { */ async function fetchOrgAllIssues( org: string, - flags: Pick, + flags: Pick, timeRange: TimeRange, options: { cursor?: string; onPage?: (fetched: number, limit: number) => void; } ): Promise { - const apiOpts = buildListApiOptions(flags.json); + const apiOpts = buildListApiOptions(flags.json, flags.fields); const timeParams = timeRangeToApiParams(timeRange); const { cursor, onPage } = options; @@ -1257,7 +1285,7 @@ async function handleResolvedTargets( ? `Fetching issues from ${targetCount} projects` : "Fetching issues"; - const apiOpts = buildListApiOptions(flags.json); + const apiOpts = buildListApiOptions(flags.json, flags.fields); const { results, hasMore } = await withProgress( { message: `${baseMessage} (up to ${flags.limit})...`, json: flags.json }, diff --git a/src/lib/api/issues.ts b/src/lib/api/issues.ts index 66155a917..cd985da00 100644 --- a/src/lib/api/issues.ts +++ b/src/lib/api/issues.ts @@ -47,7 +47,7 @@ export type IssueSort = NonNullable< * expensive Snuba/ClickHouse queries on the backend. * * - `'stats'` — time-series event counts (sparkline data) - * - `'lifetime'` — lifetime aggregate counts (count, userCount, firstSeen) + * - `'lifetime'` — lifetime aggregate sub-object AND top-level count/userCount/firstSeen/lastSeen on list endpoints * - `'filtered'` — filtered aggregate counts * - `'unhandled'` — unhandled event flag computation * - `'base'` — base group fields (rarely useful to collapse) @@ -59,9 +59,17 @@ export type IssueCollapseField = NonNullable< /** * Build the `collapse` parameter for issue list API calls. * - * Always collapses fields the CLI never consumes in issue list: - * `filtered`, `lifetime`, `unhandled`. Conditionally collapses `stats` - * when sparklines won't be rendered (narrow terminal, non-TTY, or JSON). + * Always collapses `filtered` and `unhandled` — the CLI never consumes + * these in issue list views. Conditionally collapses `stats` when + * sparklines won't be rendered (narrow terminal, non-TTY, or JSON), + * and `lifetime` when the caller confirms the lifetime-dependent + * top-level fields (`count`, `userCount`, `firstSeen`, `lastSeen`) + * aren't needed. + * + * **Important:** Despite being documented as removing only the `lifetime` + * sub-object, `collapse=lifetime` also strips the top-level `count`, + * `userCount`, `firstSeen`, and `lastSeen` fields from list responses. + * Only collapse it when those fields are confirmed unnecessary. See #969. * * Matches the Sentry web UI's optimization: the initial page load sends * `collapse=stats,unhandled` to skip expensive Snuba queries, fetching @@ -70,12 +78,20 @@ export type IssueCollapseField = NonNullable< * @param options - Context for determining what to collapse * @param options.shouldCollapseStats - Whether stats data can be skipped * (true when sparklines won't be shown: narrow terminal, non-TTY, --json) + * @param options.shouldCollapseLifetime - Whether lifetime data can be skipped. + * Defaults to `false` because most output paths need `count`/`userCount`/ + * `firstSeen`/`lastSeen`. Only set to `true` when `--json --fields` omits + * all lifetime-dependent fields. * @returns Array of fields to collapse */ export function buildIssueListCollapse(options: { shouldCollapseStats: boolean; + shouldCollapseLifetime?: boolean; }): IssueCollapseField[] { - const collapse: IssueCollapseField[] = ["filtered", "lifetime", "unhandled"]; + const collapse: IssueCollapseField[] = ["filtered", "unhandled"]; + if (options.shouldCollapseLifetime) { + collapse.push("lifetime"); + } if (options.shouldCollapseStats) { collapse.push("stats"); } @@ -90,8 +106,10 @@ export function buildIssueListCollapse(options: { * in detail views (`issue view`, `issue explain`, `issue plan`). * Collapsing these skips expensive Snuba queries, saving 100-300ms per request. * - * Note: `count`, `userCount`, `firstSeen`, `lastSeen` are top-level fields - * and remain unaffected by collapsing. + * Note: On the **list** endpoint, `collapse=lifetime` strips top-level + * `count`, `userCount`, `firstSeen`, `lastSeen` (see #969). The detail + * endpoint preserves these fields regardless of collapse — safe to include + * `lifetime` here. */ export const ISSUE_DETAIL_COLLAPSE: IssueCollapseField[] = [ "stats", diff --git a/test/commands/issue/list.test.ts b/test/commands/issue/list.test.ts index bc26aee05..b991b51c3 100644 --- a/test/commands/issue/list.test.ts +++ b/test/commands/issue/list.test.ts @@ -41,6 +41,9 @@ type ListFlags = { readonly period: TimeRange; readonly json: boolean; readonly cursor?: string; + readonly fields?: string[]; + readonly fresh?: boolean; + readonly compact?: boolean; }; /** Command function type extracted from loader result */ @@ -1110,7 +1113,7 @@ describe("issue list: collapse parameter optimization", () => { advancePaginationStateSpy.mockRestore(); }); - test("always collapses filtered, lifetime, unhandled in org-all mode", async () => { + test("always collapses filtered and unhandled in org-all mode", async () => { listIssuesPaginatedSpy.mockResolvedValue({ data: [sampleIssue], nextCursor: undefined, @@ -1134,10 +1137,125 @@ describe("issue list: collapse parameter optimization", () => { const options = callArgs?.[2] as Record | undefined; const collapse = options?.collapse as string[]; expect(collapse).toContain("filtered"); - expect(collapse).toContain("lifetime"); expect(collapse).toContain("unhandled"); }); + test("does not collapse lifetime in human mode (needed for EVENTS/USERS/SEEN/AGE)", async () => { + listIssuesPaginatedSpy.mockResolvedValue({ + data: [sampleIssue], + nextCursor: undefined, + }); + + const orgAllFunc = (await listCommand.loader()) as unknown as ( + this: unknown, + flags: Record, + target?: string + ) => Promise; + + const { context } = createOrgAllContext(); + await orgAllFunc.call( + context, + { limit: 10, sort: "date", period: parsePeriod("90d"), json: false }, + "my-org/" + ); + + expect(listIssuesPaginatedSpy).toHaveBeenCalled(); + const callArgs = listIssuesPaginatedSpy.mock.calls[0]; + const options = callArgs?.[2] as Record | undefined; + const collapse = options?.collapse as string[]; + expect(collapse).not.toContain("lifetime"); + }); + + test("does not collapse lifetime in JSON mode without --fields", async () => { + listIssuesPaginatedSpy.mockResolvedValue({ + data: [sampleIssue], + nextCursor: undefined, + }); + + const orgAllFunc = (await listCommand.loader()) as unknown as ( + this: unknown, + flags: Record, + target?: string + ) => Promise; + + const { context } = createOrgAllContext(); + await orgAllFunc.call( + context, + { limit: 10, sort: "date", period: parsePeriod("90d"), json: true }, + "my-org/" + ); + + expect(listIssuesPaginatedSpy).toHaveBeenCalled(); + const callArgs = listIssuesPaginatedSpy.mock.calls[0]; + const options = callArgs?.[2] as Record | undefined; + const collapse = options?.collapse as string[]; + expect(collapse).not.toContain("lifetime"); + }); + + test("does not collapse lifetime in JSON mode when --fields includes lifetime-dependent field", async () => { + listIssuesPaginatedSpy.mockResolvedValue({ + data: [sampleIssue], + nextCursor: undefined, + }); + + const orgAllFunc = (await listCommand.loader()) as unknown as ( + this: unknown, + flags: Record, + target?: string + ) => Promise; + + const { context } = createOrgAllContext(); + await orgAllFunc.call( + context, + { + limit: 10, + sort: "date", + period: parsePeriod("90d"), + json: true, + fields: ["shortId", "title", "count"], + }, + "my-org/" + ); + + expect(listIssuesPaginatedSpy).toHaveBeenCalled(); + const callArgs = listIssuesPaginatedSpy.mock.calls[0]; + const options = callArgs?.[2] as Record | undefined; + const collapse = options?.collapse as string[]; + expect(collapse).not.toContain("lifetime"); + }); + + test("collapses lifetime in JSON mode when --fields omits all lifetime-dependent fields", async () => { + listIssuesPaginatedSpy.mockResolvedValue({ + data: [sampleIssue], + nextCursor: undefined, + }); + + const orgAllFunc = (await listCommand.loader()) as unknown as ( + this: unknown, + flags: Record, + target?: string + ) => Promise; + + const { context } = createOrgAllContext(); + await orgAllFunc.call( + context, + { + limit: 10, + sort: "date", + period: parsePeriod("90d"), + json: true, + fields: ["shortId", "title"], + }, + "my-org/" + ); + + expect(listIssuesPaginatedSpy).toHaveBeenCalled(); + const callArgs = listIssuesPaginatedSpy.mock.calls[0]; + const options = callArgs?.[2] as Record | undefined; + const collapse = options?.collapse as string[]; + expect(collapse).toContain("lifetime"); + }); + test("collapses stats in JSON mode", async () => { listIssuesPaginatedSpy.mockResolvedValue({ data: [sampleIssue], diff --git a/test/lib/issue-collapse.property.test.ts b/test/lib/issue-collapse.property.test.ts index 4938d29ae..ce8945dfe 100644 --- a/test/lib/issue-collapse.property.test.ts +++ b/test/lib/issue-collapse.property.test.ts @@ -2,11 +2,11 @@ * Property-based tests for buildIssueListCollapse and ISSUE_DETAIL_COLLAPSE. * * Verifies invariants that must hold for any configuration of the collapse - * parameter: always-collapsed fields, stats control, and safety constraints. + * parameter: always-collapsed fields, stats/lifetime control, and safety constraints. */ import { describe, expect, test } from "bun:test"; -import { boolean, assert as fcAssert, property } from "fast-check"; +import { boolean, assert as fcAssert, property, tuple } from "fast-check"; import { buildIssueListCollapse, @@ -15,27 +15,62 @@ import { import { DEFAULT_NUM_RUNS } from "../model-based/helpers.js"; describe("property: buildIssueListCollapse", () => { - test("always collapses filtered, lifetime, unhandled regardless of stats flag", () => { + test("always collapses filtered and unhandled regardless of optional flags", () => { fcAssert( - property(boolean(), (collapseStats) => { - const result = buildIssueListCollapse({ - shouldCollapseStats: collapseStats, - }); - expect(result).toContain("filtered"); - expect(result).toContain("lifetime"); - expect(result).toContain("unhandled"); - }), + property( + tuple(boolean(), boolean()), + ([collapseStats, collapseLifetime]) => { + const result = buildIssueListCollapse({ + shouldCollapseStats: collapseStats, + shouldCollapseLifetime: collapseLifetime, + }); + expect(result).toContain("filtered"); + expect(result).toContain("unhandled"); + } + ), { numRuns: DEFAULT_NUM_RUNS } ); }); test("stats presence is exactly controlled by shouldCollapseStats", () => { + fcAssert( + property( + tuple(boolean(), boolean()), + ([collapseStats, collapseLifetime]) => { + const result = buildIssueListCollapse({ + shouldCollapseStats: collapseStats, + shouldCollapseLifetime: collapseLifetime, + }); + expect(result.includes("stats")).toBe(collapseStats); + } + ), + { numRuns: DEFAULT_NUM_RUNS } + ); + }); + + test("lifetime presence is exactly controlled by shouldCollapseLifetime", () => { + fcAssert( + property( + tuple(boolean(), boolean()), + ([collapseStats, collapseLifetime]) => { + const result = buildIssueListCollapse({ + shouldCollapseStats: collapseStats, + shouldCollapseLifetime: collapseLifetime, + }); + expect(result.includes("lifetime")).toBe(collapseLifetime); + } + ), + { numRuns: DEFAULT_NUM_RUNS } + ); + }); + + test("defaults to not collapsing lifetime when option omitted", () => { fcAssert( property(boolean(), (collapseStats) => { const result = buildIssueListCollapse({ shouldCollapseStats: collapseStats, }); - expect(result.includes("stats")).toBe(collapseStats); + expect(result).not.toContain("lifetime"); }), { numRuns: DEFAULT_NUM_RUNS } ); @@ -43,36 +78,50 @@ describe("property: buildIssueListCollapse", () => { test("never collapses base (would break all rendering)", () => { fcAssert( - property(boolean(), (collapseStats) => { - const result = buildIssueListCollapse({ - shouldCollapseStats: collapseStats, - }); - expect(result).not.toContain("base"); - }), + property( + tuple(boolean(), boolean()), + ([collapseStats, collapseLifetime]) => { + const result = buildIssueListCollapse({ + shouldCollapseStats: collapseStats, + shouldCollapseLifetime: collapseLifetime, + }); + expect(result).not.toContain("base"); + } + ), { numRuns: DEFAULT_NUM_RUNS } ); }); test("returns no duplicates", () => { fcAssert( - property(boolean(), (collapseStats) => { - const result = buildIssueListCollapse({ - shouldCollapseStats: collapseStats, - }); - expect(new Set(result).size).toBe(result.length); - }), + property( + tuple(boolean(), boolean()), + ([collapseStats, collapseLifetime]) => { + const result = buildIssueListCollapse({ + shouldCollapseStats: collapseStats, + shouldCollapseLifetime: collapseLifetime, + }); + expect(new Set(result).size).toBe(result.length); + } + ), { numRuns: DEFAULT_NUM_RUNS } ); }); - test("length is 3 without stats, 4 with stats", () => { + test("length equals 2 + number of optional flags enabled", () => { fcAssert( - property(boolean(), (collapseStats) => { - const result = buildIssueListCollapse({ - shouldCollapseStats: collapseStats, - }); - expect(result.length).toBe(collapseStats ? 4 : 3); - }), + property( + tuple(boolean(), boolean()), + ([collapseStats, collapseLifetime]) => { + const result = buildIssueListCollapse({ + shouldCollapseStats: collapseStats, + shouldCollapseLifetime: collapseLifetime, + }); + const expected = + 2 + (collapseStats ? 1 : 0) + (collapseLifetime ? 1 : 0); + expect(result.length).toBe(expected); + } + ), { numRuns: DEFAULT_NUM_RUNS } ); }); @@ -96,8 +145,11 @@ describe("ISSUE_DETAIL_COLLAPSE", () => { expect(ISSUE_DETAIL_COLLAPSE).toContain("stats"); }); - test("is a superset of buildIssueListCollapse with stats collapsed", () => { - const listCollapse = buildIssueListCollapse({ shouldCollapseStats: true }); + test("is a superset of buildIssueListCollapse with all options enabled", () => { + const listCollapse = buildIssueListCollapse({ + shouldCollapseStats: true, + shouldCollapseLifetime: true, + }); for (const field of listCollapse) { expect(ISSUE_DETAIL_COLLAPSE).toContain(field); }