You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
perf(api): collapse stats on issue detail endpoints to save 100-300ms (#551)
Applies the same optimization the Sentry web UI shipped: skip fetching
`stats` (24h/30d time-series from Snuba) on issue detail requests. Saves
100-300ms per request.
## Problem
The `@sentry/api` SDK's `retrieveAnIssue` and `resolveAShortId` both
declare `query?: never`, making it impossible to pass the `collapse`
query parameter through the SDK.
## Solution
- Switch `getIssueInOrg` and `getIssueByShortId` from the SDK to raw
`apiRequestToRegion()` to enable passing `collapse`
- Add `ISSUE_DETAIL_COLLAPSE` constant: `["stats", "lifetime",
"filtered", "unhandled"]`
- Pass it to all 9 issue-fetch call sites in `utils.ts` (view, explain,
plan, selector resolution)
- Add `collapse` to `resolveSelector()` which previously fetched all
fields for a single-issue `@latest`/`@most_frequent` lookup
## What's unaffected
`count`, `userCount`, `firstSeen`, `lastSeen` are top-level fields
outside the collapsed sub-objects — human output is unchanged. `issue
list` already had its own conditional collapse logic and is not touched.
## Follow-up
File upstream issue at https://github.com/getsentry/sentry-api-schema/
to add `collapse` to the OpenAPI spec for `retrieveAnIssue` and
`resolveAShortId`, so the SDK can be used again.
* **cli.sentry.dev is served from gh-pages branch via GitHub Pages**: \`cli.sentry.dev\` is served from gh-pages branch via GitHub Pages. Craft's gh-pages target runs \`git rm -r -f .\` before extracting docs — persist extra files via \`postReleaseCommand\` in \`.craft.yml\`. Install script supports \`--channel nightly\`, downloading from the \`nightly\` release tag directly. version.json is only used by upgrade/version-check flow.
* **npm bundle requires Node.js >= 22 due to node:sqlite polyfill**: The npm package (dist/bin.cjs) requires Node.js >= 22 because the bun:sqlite polyfill uses \`node:sqlite\`. A runtime version guard in the esbuild banner catches this early. When writing esbuild banner strings in TS template literals, double-escape: \`\\\\\\\n\` in TS → \`\\\n\` in output → newline at runtime. Single \`\\\n\` produces a literal newline inside a JS string, causing SyntaxError.
* **withAuthGuard returns discriminated Result type, not fallback+onError**: \`withAuthGuard\<T>(fn)\` in \`src/lib/errors.ts\` returns a discriminated Result: \`{ ok: true, value: T } | { ok: false, error: unknown }\`. AuthErrors always re-throw (triggers bin.ts auto-login). All other errors are captured. Callers inspect \`result.ok\` to degrade gracefully. Used across 12+ files.
* **Sentry SDK switched from @sentry/bun to @sentry/node-core/light**: The CLI switched from \`@sentry/bun\` to \`@sentry/node-core/light\` (PR #474) to eliminate the OpenTelemetry dependency tree (~170ms startup savings). All imports use \`@sentry/node-core/light\` not \`@sentry/bun\`. \`LightNodeClient\` replaces \`BunClient\` in telemetry.ts. \`Span\` type exports from \`@sentry/core\`. The \`@sentry/core\` barrel is patched to remove ~32 unused exports (AI tracing, MCP server, Supabase, feature flags). Light mode is labeled experimental but safe because SDK versions are pinned exactly (not caret). \`makeNodeTransport\` works fine in Bun. The \`cli.runtime\` tag still distinguishes Bun vs Node at runtime.
* **Biome lint: Response.redirect() required, nested ternaries forbidden**: Biome lint rules that frequently trip up this codebase: (1) \`useResponseRedirect\`: use \`Response.redirect(url, status)\` not \`new Response\`. (2) \`noNestedTernary\`: use \`if/else\`. (3) \`noComputedPropertyAccess\`: use \`obj.property\` not \`obj\["property"]\`. (4) Max cognitive complexity 15 per function — extract helpers to stay under.
* **Bugbot flags defensive null-checks as dead code — keep them with JSDoc justification**: Cursor Bugbot and Sentry Seer repeatedly flag two false positives: (1) defensive null-checks as "dead code" — keep them with JSDoc explaining why the guard exists for future safety, especially when removing would require \`!\`assertions banned by \`noNonNullAssertion\`. (2) stderr spinner output during \`--json\`mode — always a false positive since progress goes to stderr, JSON to stdout. Reply explaining the rationale and resolve.
* **Chalk needs chalk.level=3 for ANSI output in tests**: In Bun tests, stdout is piped (not a TTY), so chalk's color level defaults to 0 and produces no ANSI escape codes — even with \`SENTRY\_PLAIN\_OUTPUT=0\`. Setting \`FORCE\_COLOR=1\` env var works but the project convention is to set \`chalk.level = 3\`at the top of test files that need to assert on ANSI output. The project's \`isPlainOutput()\`and chalk's color detection are independent systems: \`isPlainOutput()\` checks \`SENTRY\_PLAIN\_OUTPUT\`/\`NO\_COLOR\`/TTY, while chalk checks \`FORCE\_COLOR\`/TTY separately.
* **Bun mock.module for node:tty requires default export and class stubs**: Bun testing gotchas: (1) \`mock.module()\` for CJS built-ins requires a \`default\`re-export plus all named exports. Missing any causes \`SyntaxError: Export named 'X' not found\`. Always check the real module's full export list. (2) \`Bun.mmap()\`always opens with PROT\_WRITE — macOS SIGKILL on signed Mach-O, Linux ETXTBSY. Fix: use \`new Uint8Array(await Bun.file(path).arrayBuffer())\` in bspatch.ts. (3) Wrap \`Bun.which()\`with optional \`pathEnv\` param for deterministic testing without mocks.
* **issue explain/plan commands never set sentry.org telemetry tag**: The \`issue explain\` and \`issue plan\` commands were missing \`setContext(\[org], \[])\` calls after \`resolveOrgAndIssueId()\`, so \`sentry.org\` and \`sentry.project\`tags were never set on SeerError events. Fixed in commit 816b44b5 on branch \`fix/seer-org-telemetry-tag\` — both commands now destructure \`setContext\` from \`this\`and call it immediately after resolving the org, before any Seer API call. The \`issue view\` and \`issue list\` commands already had this call. Pattern: always call \`setContext()\`right after org resolution, before any API call that might throw, so error events carry the org tag.
* **Use toMatchObject not toEqual when testing resolution results with optional fields**: When \`resolveProjectBySlug()\` or \`resolveOrgProjectTarget()\` adds optional fields (like \`projectData\`) to the return type, tests using \`expect(result).toEqual({ org, project })\` fail because \`toEqual\` requires exact match. Use \`toMatchObject({ org, project })\` instead — it checks the specified subset without failing on extra properties. This affects tests across \`event/view\`, \`log/view\`, \`trace/view\`, and \`trace/list\` test files.
* **LSP auto-organize imports can silently remove new imports**: When editing TypeScript files in this project, the LSP's auto-organize-imports feature can silently remove import lines that were just added if the imported symbols aren't yet referenced in the file. This happens when you add an import in one edit and the usage in a subsequent edit — the LSP runs between edits and strips the "unused" import. Fix: always add the import and its first usage in the same edit operation, or verify imports survived after each edit by grepping the file.
* **project list bare-slug org fallback requires custom handleProjectSearch**: The \`project list\` command has a custom \`handleProjectSearch\` override (not the shared one from \`org-list.ts\`). When a user types \`sentry project list acme-corp\` where \`acme-corp\` is an org slug (not a project), the custom handler must check if the bare slug matches an organization and fall back to listing that org's projects. The shared \`handleProjectSearch\` in \`org-list.ts\` already does this, but custom overrides in individual commands can miss it, causing a confusing \`ResolutionError\` (PR #475 fix).
* **Branch naming and commit message conventions for Sentry CLI**: Branch naming: \`feat/\<short-description>\` or \`fix/\<issue-number>-\<short-description>\` (e.g., \`feat/ghcr-nightly-distribution\`, \`fix/268-limit-auto-pagination\`). Commit message format: \`type(scope): description (#issue)\` (e.g., \`fix(issue-list): auto-paginate --limit beyond 100 (#268)\`, \`feat(nightly): distribute via GHCR instead of GitHub Releases\`). Types seen: fix, refactor, meta, release, feat. PRs are created as drafts via \`gh pr create --draft\`. Implementation plans are attached to commits via \`git notes add\` rather than in PR body or commit message.
* **Zero runtime dependencies enforced by test**: The project has a test in \`test/package.test.ts\` that asserts \`pkg.dependencies\` is empty — all packages must be devDependencies. The CLI is distributed as a compiled Bun binary, so runtime deps are bundled at build time. Adding a package to \`dependencies\` instead of \`devDependencies\` will fail CI. When adding a new package like \`@sentry/sqlish\`, use \`bun add --dev\` not \`bun add\`.
* **Codecov patch coverage only counts test:unit and test:isolated, not E2E**: CI coverage merges \`test:unit\` (\`test/lib test/commands test/types --coverage\`) and \`test:isolated\` (\`test/isolated --coverage\`) into \`coverage/merged.lcov\`. E2E tests (\`test/e2e\`) are NOT included in coverage reports. So func tests that spy on exports (e.g., \`spyOn(apiClient, 'getLogs')\`) give zero coverage to the mocked function's body. To cover \`api-client.ts\` function bodies in unit tests, mock \`globalThis.fetch\` + \`setOrgRegion()\` + \`setAuthToken()\` and call the real function.
* **Pagination contextKey must include all query-varying parameters with escaping**: Pagination \`contextKey\` must encode every query-varying parameter (sort, query, period) with \`escapeContextKeyValue()\` (replaces \`|\` with \`%7C\`). Always provide a fallback before escaping since \`flags.period\` may be \`undefined\` in tests despite having a default: \`flags.period ? escapeContextKeyValue(flags.period) : "90d"\`.
* **PR review workflow: reply, resolve, amend, force-push**: PR review workflow: (1) Read unresolved threads via GraphQL, (2) make code changes, (3) run lint+typecheck+tests, (4) create a SEPARATE commit per review round (not amend) for incremental review, (5) push normally, (6) reply to comments via REST API, (7) resolve threads via GraphQL \`resolveReviewThread\`. Only amend+force-push when user explicitly asks or pre-commit hook modified files.
* **@sentry/api SDK blocks query params on issue detail endpoints**: The \`retrieveAnIssue\` and \`resolveAShortId\` SDK functions have \`query?: never\` in their TypeScript types, preventing callers from passing \`collapse\` or other query parameters. The Sentry backend DOES accept \`collapse\` on these endpoints (same Django view base class as the list endpoint), but the OpenAPI spec omits it. The CLI works around this by using raw \`apiRequestToRegion()\` instead of the SDK for \`getIssueInOrg\` and \`getIssueByShortId\`. Upstream issue filed at https://github.com/getsentry/sentry-api-schema/issues/63. If the schema is fixed, these functions can switch back to the SDK.
* **Stricli optional boolean flags produce tri-state (true/false/undefined)**: Stricli boolean flags with \`optional: true\`(no \`default\`) produce\`boolean | undefined\` in the flags type.\`--flag\` → \`true\`, \`--no-flag\`→ \`false\`, omitted → \`undefined\`. This enables auto-detect patterns: explicit user choice overrides, \`undefined\`triggers heuristic. Used by \`--compact\`on issue list. The flag type must be \`readonly field?: boolean\` (not \`readonly field: boolean\`). This differs from \`default: false\`which always produces a defined boolean.
* **Dashboard widget layout uses 6-column grid with x,y,w,h**: Sentry dashboards use a 6-column grid with \`{x, y, w, h, minH}\`layout. Default big\_number is\`w=2, h=1\`; to center it use\`w=4, x=1\`. Tables default to \`w=6, h=2\`. The \`dashboard widget edit\`command only handles query/display/title — use \`sentry api\` with PUT to \`/api/0/organizations/{org}/dashboards/{id}/\` for layout, default period, environment, and project changes. The PUT payload must include the full \`widgets\`array. Widget queries need both \`fields\`(all columns + aggregates) and \`columns\` (non-aggregate columns only) arrays. Use \`user.display\` instead of \`user.email\`for user columns — it auto-selects the best available identifier.
* **Testing Stricli command func() bodies via spyOn mocking**: Stricli/Bun test patterns: (1) Command func tests: \`const func = await cmd.loader()\`, then \`func.call(mockContext, flags, ...args)\`. \`loader()\` return type union causes LSP errors — false positives that pass \`tsc\`. File naming: \`\*.func.test.ts\`. (2) ESM prevents \`vi.spyOn\` on Node built-in exports. Workaround: test subclass that overrides the method calling the built-in. (3) Follow-mode uses \`setTimeout\`-based scheduling; test with \`interceptSigint()\` helper. \`Bun.sleep()\` has no AbortSignal so \`setTimeout\`/\`clearTimeout\` required.
* **Issue resolution fan-out uses unbounded Promise.all across orgs**: \`resolveProjectSearch()\` in \`src/commands/issue/utils.ts\` fans out \`tryGetIssueByShortId\` across ALL orgs via \`Promise.all\`. Pattern: expand short ID, list all orgs, fire lookups in parallel, collect successes. Exactly one success → use it; multiple → ambiguity error; all fail → fall back to \`resolveProjectSearchFallback()\`. All org fan-out concurrency is controlled by \`ORG\_FANOUT\_CONCURRENCY\` (exported from \`src/lib/api/infrastructure.ts\`, re-exported via \`api-client.ts\`). This single constant is used in \`events.ts\`, \`projects.ts\`, \`issue/utils.ts\`, and any future fan-out sites. Never define a local concurrency limit for org fan-out — always import the shared constant.
0 commit comments