[HDX-4997] Alert evaluation event stream: time-range pagination, per-group breakdown, analytics

[wrn14897]

Summary

Stack (3/3): #2797 (persist evaluation errors) ← #2798 (alert detail page) ← this PR. Review/merge bottom-up.
This PR was originally the whole feature; the backend error persistence and the detail-page skeleton were split out into the two PRs below it, leaving the evaluation event stream iterations here.

Makes the alert detail page's evaluation event stream (from #2798) production-grade:

Time-range-driven pagination with hard caps

• The evaluation strip and event stream follow the same time range as the chart instead of always paging from now; older windows load automatically in 200-window pages via an in-viewport sentinel (replacing the load-more button).
• GET /alerts/:id/evaluations accepts startTime/endTime (epoch ms) with the span clamped to the 31d history retention window and limit fixed at 200.
• Each request scans a hard-bounded slice of at most ~(limit+1) × interval of history — group-by alerts can have many rows per window and the $group stage processes every matched row, so the scan is bounded, not just the returned page.
• Pagination is cursor-based via a server-provided nextBefore that always advances past the scanned slice, so paging keeps progressing across gaps with no evaluations (e.g. alerting job downtime) instead of stalling.
• Chart, strip, and event stream all follow the picker's exact range — no interval-based widening.

Per-group breakdown for group-by alerts

• The endpoint returns a per-group breakdown per window: group name, state, breach count, latest value, and whether a notification fired. Sorted firing-first and capped at ALERT_EVALUATION_GROUPS_LIMIT (50, shared const in common-utils) with groupsTotal reporting the pre-cap count — firing groups stay visible and high-cardinality group-bys can't produce unbounded responses. No extra scan cost (rows were already matched; only $group retention changed).
• Parent rows show a k/n groups firing summary and expand on click into indented per-group child rows; the group-cap row states the cap explicitly ("Showing the top 50 of N groups (firing first) — additional groups aren't fetched").

Evaluation analytics

• Every history record now carries a top-level analytics subdoc: queryDurationMs (time-to-failure on query-error rows — ≈ the timeout for QUERY_TIMEOUT), webhookDurationMs (total notification delivery wall time incl. retries), and backfilledBuckets (missed ticks caught up in this run). Optional field, no migration; backfilledBuckets is derived from lastValues for pre-existing rows.
• Surfaced as Backfilled Buckets (with explanatory tooltip), Query Duration, and Webhook Duration columns on the parent rows. Rows no longer auto-expand — group breakdown and error details are click-driven.

Chart/table alignment fix

• The Evaluation Window column now shows the evaluated bucket start (the same x the chart plots the value at) instead of the evaluation time (bucket end), with a tooltip showing the full evaluated span — a row labeled 12:15 no longer holds the value the chart draws at 12:10.

How to test on Vercel preview

N/A — needs the alerting job + seeded alert data. Covered by API integration tests and full-stack Playwright tests instead.

How this was tested

• make ci-lint, make ci-unit — pass across all packages
• Integration: make dev-int FILE=checkAlerts.int, FILE=alertHistory.int, FILE=alerts.int, FILE=singleInvocationAlert.int, FILE=default.int — all pass (evaluations endpoint time-range clamping, bounded scan, nextBefore cursor, per-group breakdown + cap, analytics fields)
• App unit tests for the evaluations table (pagination sentinel, group rows, analytics columns, bucket-start labeling)
• E2E (make dev-e2e FILE=alerts): errored history segment + detail page specs pass

References

• Linear Issue: HDX-4997
• Related: HDX-4977

Note for EE: recordAlertErrors gained an optional analytics param so query-failure ERROR rows carry timing diagnostics (backward compatible).

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
hyperdx-oss	Ready	Preview	Aug 5, 2026 5:44pm
hyperdx-storybook	Ready	Preview	Aug 5, 2026 5:44pm

[changeset-bot]

⚠️ No Changeset found

Latest commit: 9848817

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[github-actions]

🔴 Tier 4 — Critical

Touches auth, data models, config, tasks, OTel pipeline, ClickHouse, or CI/CD.

Why this tier:

• Critical-path files (3):
  • packages/api/src/tasks/checkAlerts/index.ts
  • packages/api/src/tasks/checkAlerts/providers/default.ts
  • packages/api/src/tasks/checkAlerts/providers/index.ts
• Cross-layer change: touches frontend (packages/app) + backend (packages/api) + shared utils (packages/common-utils)

Review process: Deep review from a domain expert. Synchronous walkthrough may be required.
SLA: Schedule synchronous review within 2 business days.

Stats

• Production files changed: 11
• Production lines changed: 940 (+ 797 in test files, excluded from tier calculation)
• Branch: warren/HDX-4997-alert-error-history
• Author: wrn14897

To override this classification, remove the review/tier-4 label and apply a different review/tier-* label. Manual overrides are preserved on subsequent pushes.

[greptile-apps]

Greptile Summary

This PR makes alert evaluation history follow the selected time range, adds bounded cursor pagination across sparse history, and surfaces per-group and evaluation analytics. The new range handling currently skips evaluation fetching for valid sub-minute ranges.

• Adds bounded, gap-safe evaluation pagination using server-provided cursors
• Adds capped firing-first per-group breakdowns and evaluation timing/backfill analytics
• Aligns the chart, evaluation strip, and event table to the selected range
• Adds automatic viewport-driven loading and expandable group/error rows

Confidence Score: 4/5

The PR should not merge until valid sub-minute picker ranges continue to fetch their evaluation history.

Flooring both endpoints to minute boundaries can make a valid exact range collapse to equal request bounds, causing the evaluations query to be disabled while the chart still displays that range.

Files Needing Attention: packages/app/src/api.ts

Important Files Changed

Filename	Overview
packages/api/src/controllers/alertHistory.ts	Implements bounded range pagination that advances across sparse history and returns grouped evaluation details.
packages/api/src/routers/api/alerts.ts	Extends the evaluations endpoint with validated, retention-clamped time bounds and server-provided cursors.
packages/app/src/api.ts	Adds range-keyed infinite evaluation queries, but minute quantization disables requests for valid ranges contained within one minute.
packages/app/src/AlertDetailPage.tsx	Connects the exact selected range and automatic pagination to the alert chart and evaluation stream.
packages/app/src/components/alerts/AlertEvaluationsTable.tsx	Adds analytics columns, expandable per-group/error details, bucket-start labels, and viewport-driven pagination.
packages/api/src/tasks/checkAlerts/index.ts	Records query, notification, and backfill analytics on evaluation history records.
packages/common-utils/src/types.ts	Defines shared evaluation analytics, group breakdown, pagination cursor schemas, and the group response cap.

Sequence Diagram

sequenceDiagram
  participant User
  participant Picker as TimePicker
  participant Detail as AlertDetailPage
  participant Query as useAlertEvaluations
  participant API as GET /alerts/:id/evaluations
  participant Mongo as AlertHistory
  User->>Picker: Select exact time range
  Picker->>Detail: searchedTimeRange
  Detail->>Query: alert id + range
  Query->>API: startTime, endTime, before
  API->>Mongo: bounded window scan
  Mongo-->>API: grouped evaluations
  API-->>Query: data, hasMore, nextBefore
  Query-->>Detail: paginated event stream
  Detail->>Query: fetchNextPage at viewport sentinel

Loading

_{Reviews (9): Last reviewed commit: "fix(api): unexport internal alert evalua..." | Re-trigger Greptile}

[github-actions]

E2E Test Results

✅ All tests passed • 271 passed • 1 skipped • 1108s

Status	Count
✅ Passed	271
❌ Failed	0
⚠️ Flaky	1
⏭️ Skipped	1

Tests ran across 4 shards in parallel.

View full report →

[github-actions]

<!-- deep-review -->

Deep Review

🔴 P0/P1 — must fix

• packages/api/src/tasks/checkAlerts/index.ts:1553 — The new state: { $ne: AlertState.ERROR } filters in getPreviousAlertHistories and getConsecutiveWindowHistories are the only thing keeping ERROR rows from counting as evaluated windows, so a code rollback leaves those rows in place while removing the filters: shouldSkipAlertCheck then matches on createdAt alone and getAlertEvaluationDateRange advances previousCreatedAt past the failed window, which is never re-queried, and any ERROR row inside the numConsecutiveWindows lookback breaks the every(state === ALERT || PENDING) check so multi-window alerts silently stop firing.
  • Fix: Gate the ERROR-row writes behind a flag that can be disabled independently of the read-side filters, or state in the deploy runbook that rollback is unsafe once ERROR rows exist and require draining them first.
• packages/api/src/tasks/checkAlerts/errors.ts:67 — isQueryTimeoutError inspects e.type/e.code on the thrown value directly, but BaseClickhouseClient.query() re-throws every failure as new ClickHouseQueryError(message, debugSql) with the original attached as cause, so isClickHouseError fails both its instanceof and constructor-name checks and the server-side TIMEOUT_EXCEEDED/159 and ETIMEDOUT branches never match in production — the timeout is persisted as QUERY_ERROR and the counter is labelled error_type: 'error'.
  • Fix: Walk the cause chain in isQueryTimeoutError and isClickHouseError, and add a case to errors.test.ts built from the wrapped shape BaseClickhouseClient.query() actually throws rather than a bare ClickHouseError.
• packages/api/src/tasks/checkAlerts/index.ts:1038 — The recordAlertErrors call in the query-failure catch is unguarded, and DefaultAlertProvider.recordAlertErrors commits Alert.updateOne before awaiting upsertErrorHistory, so a throw from the second write escapes into processAlert's outer catch, which reclassifies the failure as AlertErrorType.UNKNOWN and overwrites the just-persisted QUERY_TIMEOUT/QUERY_ERROR with a hardcoded generic message; the same unguarded upsertErrorHistory inside updateAlertState additionally flips evalOutcome to error for an evaluation that succeeded.
  • Fix: Wrap the upsertErrorHistory calls in providers/default.ts in their own try/catch that logs and returns, so a history-write failure cannot reach the outer catch and clobber the authoritative executionErrors.
  • _{reliability, adversarial}
• packages/app/src/components/alerts/AlertEvaluationsTable.tsx:139 — The sentinel effect's only guard is !isFetchingNextPage, and hasNextPage is derived from the last successful page, so a failing /alerts/:id/evaluations request re-fires onLoadMore() on every settle with no error surfaced; the same unbounded loop runs on the success path because the server returns hasMore: true with empty data when a scan slice has no rows, so a 1m-interval alert over a 30-day picker range walks the range in roughly 215 back-to-back aggregation requests from a single page view.
  • Fix: Thread the infinite query's error state into LoadMoreSentinel and skip onLoadMore() once a page fetch has failed, and cap auto-pagination at N pages behind an explicit "load older" click.
  • _{julik-frontend-races, reliability, correctness, adversarial, api-contract}

🟡 P2 — recommended

• packages/api/src/controllers/alertHistory.ts:301 — getAlertTransitionsInRange has no $limit or scan-count bound, unlike the sibling evaluations pipeline this PR explicitly bounds, and the new detail chart calls it unconditionally with a user-selectable range up to MAX_HISTORY_SPAN_MS, so a 1m-interval group-by alert over 30 days materializes roughly 43k windows times per-window group count into one $group.
  • Fix: Apply the same (limit + 1) * intervalMs scan bound used by getAlertEvaluations, or add a document-count cap before the $group stage.
• packages/api/src/routers/api/alerts.ts:107 — GET /alerts fetches 20 history windows per alert and formatAlertResponse passes errors through verbatim, while QUERY_ERROR/QUERY_TIMEOUT are absent from the hardcoded-message map so raw ClickHouse text is persisted at up to 10,000 chars per entry — one unreachable connection failing every alert in a team turns the alerts landing page into a multi-megabyte response.
  • Fix: Truncate persisted messages far more aggressively in makeAlertError, and return only errors[].type plus a short preview from the list endpoint, keeping full text behind GET /alerts/:id/evaluations.
• packages/api/src/tasks/checkAlerts/providers/default.ts:461 — upsertErrorHistory uses $set: { errors }, so a later tick in the same evaluation window replaces rather than merges the stored array: a 1d alert that times out at 00:01 and then fails its webhook at 00:02 keeps only WEBHOOK_ERROR and loses the timeout permanently, even though the read path's dedupeErrors exists to merge distinct errors per window.
  • Fix: Accumulate with $addToSet/$push capped by $slice and let dedupeErrors collapse duplicates on read, or document that only the latest tick's errors are retained per window.
• packages/api/src/tasks/checkAlerts/providers/default.ts:454 — The upsert filter { alert, createdAt, state: ERROR } is not backed by a unique index (models/alertHistory.ts declares only the createdAt TTL plus two non-unique compound indexes), so updateOne(..., { upsert: true }) is not atomic against a concurrent insert and two overlapping evaluations of the same window can each insert a row, contradicting the function's own one-row-per-window docstring.
  • Fix: Add a partial unique index on { alert: 1, createdAt: 1, state: 1 } filtered to state: 'ERROR' and retry an E11000 from the insert race as an update.
  • _{data-migrations, reliability, adversarial}
• packages/app/src/components/alerts/AlertHistoryCards.tsx:128 — groupStateToOverallState ranks ALERT above ERROR, so a window that fired and whose webhook failed is grouped as ALERT with errors populated, but the click target and error modal are gated on history.state === AlertState.ERROR — the "fired but never notified" case renders as an ordinary solid segment with no way to reach the stored error.
  • Fix: Gate the clickable error affordance on (history.errors?.length ?? 0) > 0 while leaving the striped styling driven by state.
  • _{correctness, adversarial}
• packages/app/src/AlertDetailPage.tsx:129 — AlertDetailBody never destructures isError from api.useAlertEvaluations, so a failed first page leaves isLoading false and data undefined, and AlertEvaluationsTable's evaluations.length === 0 && !hasNextPage branch renders "No evaluations in the selected time range" — a backend failure is presented as healthy-but-empty on the page whose purpose is diagnosing failures.
  • Fix: Pass isError/error through to AlertEvaluationsTable and render a distinct error state with a retry affordance.
  • _{reliability, julik-frontend-races}
• packages/api/src/controllers/alertHistory.ts:92 — fetchGroupedWindows bounds the $match by interval count but places $group before $limit, so document volume scales with group-by cardinality: a 200-window page on a 1m-interval alert with hundreds of groups pulls every matched row into $group on each page, and the code's own comment acknowledges the cost without mitigating it.
  • Fix: Add a document-count cap inside the pipeline or an early exit once limit distinct createdAt values have been seen, independent of the time bound.
• packages/api/src/tasks/checkAlerts/providers/default.ts:411 — executionErrors accumulates one entry per failing group and is written unbounded via $set, mirrored onto the ERROR history row; only per-message text is truncated, so a high-cardinality group-by alert with a persistently failing webhook can grow the document toward the BSON limit and fail an Alert.updateOne that also carries state: finalState.
  • Fix: Dedupe by type and message and cap the array to a fixed entry count before persisting to either Alert.executionErrors or the ERROR row.
• packages/api/src/models/alert.ts:149 — state is declared enum: AlertState, which now includes ERROR, so the "AlertHistory rows only" invariant rests entirely on a comment and current call-site discipline; an ERROR value on Alert.state would be schema-valid and would match none of AlertsPage.tsx's ALERT/PENDING/OK buckets, silently dropping the alert from the list, and is absent from the v2 OpenAPI AlertState enum.
  • Fix: Split the enum so Alert.state uses a variant without ERROR and only AlertHistory.state/AlertTransition.state accept the widened set.
  • _{maintainability, data-migrations}
• packages/api/src/controllers/alertHistory.ts:47 — dedupeErrors here and dedupeAlertErrors in AlertHistoryCards.tsx implement the same ${type}||${message} key and same keep-newest rule twice and have already diverged, with only the backend copy sorting newest-first, so error ordering differs between the alerts-list and evaluations paths.
  • Fix: Move one implementation into packages/common-utils and import it from both the controller and the component.
• packages/app/src/components/alerts/AlertDetailChart.tsx:135 — TileAlertChart's config useMemo rebuilds the dashboard tile's ChartConfig assembly field-for-field from DBDashboardPage.tsx, so any change to the real tile's config (a new source field, a change to getMetricTableName) leaves this preview charting something different from what the tile and the alert task actually query.
  • Fix: Extract the builder-tile and raw-SQL-tile config assembly into a shared helper and call it from both Tile and TileAlertChart.
• packages/app/src/utils/alerts.ts:50 — extendDateRangeToInterval is eight near-identical branches of threshold-and-unit boundary math with no test coverage; utils/__tests__/alerts.test.ts only exercises normalizeNoOpAlertScheduleFields, so a wrong magnitude or comparator in any branch silently mis-scopes the detail chart and evaluations range.
  • Fix: Add unit tests covering each interval's threshold, an already-wide range that must pass through unchanged, the exact-threshold boundary, and an unmatched interval.
• packages/app/src/api.ts:233 — useAlertEvaluations is the cursor engine behind the entire load-more UX and has no test coverage at all, and getNextPageParam trusts lastPage.nextBefore whenever hasMore is true even though nextBefore is only .optional() in the schema.
  • Fix: Add a test with a mocked server asserting getNextPageParam returns the cursor when hasMore is true, undefined when false, and terminates rather than looping when hasMore is true with nextBefore absent.
• packages/api/src/routers/api/alerts.ts:174 — The new evaluation-history capability is mounted only on the session-authenticated internal router; the API-key-authenticated external-api/v2/alerts.ts registers no equivalent route and the MCP clickstack_get_alert tool is fixed at getRecentAlertHistories({ limit: 20 }), so an agent cannot reproduce the time-ranged failure investigation a human can now perform.
  • Fix: Add a v2 route that calls getAlertEvaluations under validateUserAccessKey, and extend the MCP tool with startTime/endTime/before parameters.

🔵 P3 nitpicks (7)

• packages/common-utils/src/types.ts:2185 — AlertEvaluationsApiResponseSchema declares nextBefore optional independently of hasMore, so the "cursor present whenever more pages exist" contract is enforced by comment rather than by a discriminated union.
  • Fix: Model the response as a union of { hasMore: true, nextBefore: Date } and { hasMore: false }.
• packages/common-utils/src/types.ts:621 — AlertErrorSchema.timestamp is z.union([z.string(), z.date()]), but the value is always a string once serialized, so every client consumer must handle a Date branch that cannot occur.
  • Fix: Type timestamp as z.string() and rely on the existing server-side pre-serialization wrapper for the Date form.
• packages/app/src/components/alerts/AlertHistoryCards.tsx:221 — showErrorIndicator and history are independent optional props with only one valid pairing, so a caller can pass an explicit window array alongside an indicator summarizing alert.executionErrors, which describes a different set of windows.
  • Fix: Split out a presentational strip component and keep the executionErrors indicator wired up only in the alerts-list caller.
• packages/api/src/tasks/checkAlerts/index.ts:228 — makeAlertError, HARDCODED_ALERT_ERROR_MESSAGES, and makeQueryAlertError were added to the already-1860-line index.ts even though errors.ts is the dedicated home for the classification predicates they call, so adding an AlertErrorType now requires edits in two files.
  • Fix: Move the three error-construction helpers into errors.ts alongside isQueryTimeoutError.
• packages/app/src/components/alerts/AlertHistoryCards.tsx:199 — AlertErrorsIndicator hardcodes color: 'var(--mantine-color-red-6)', which agent_docs/code_style.md prohibits in favour of the semantic danger token.
  • Fix: Replace the raw Mantine color variable with the documented semantic danger token.
• packages/app/src/components/alerts/AlertEvaluationsTable.tsx:95 — EvaluationRow uses <Text c="red"> where agent_docs/code_style.md specifies <Text variant="danger"> for inline status text.
  • Fix: Switch to variant="danger".
• packages/app/src/components/alerts/__tests__/AlertEvaluationsTable.test.tsx:85 — Both sentinel tests replace useInViewport with a hardcoded boolean, so they only re-assert the component's own if (inViewport && !isFetchingNextPage) guard and would still pass if the ref were never attached to the sentinel element.
  • Fix: Keep the guard tests and add one that renders the sentinel against a polyfilled IntersectionObserver to prove the ref is wired.

---

Reviewers (13): correctness, adversarial, security, reliability, api-contract, performance, testing, maintainability, kieran-typescript, data-migrations, julik-frontend-races, project-standards, agent-native.

Testing gaps:

• No fixture places an AlertState.ERROR row where removing the $ne: AlertState.ERROR filter from getPreviousAlertHistories or getConsecutiveWindowHistories would change the result, so a regression on the PR's load-bearing exclusion invariant would not fail a test.
• No test drives the error path with an error shaped like what BaseClickhouseClient.query() actually throws, which is why the QUERY_TIMEOUT classification gap passes CI.
• No test forces recordAlertErrors to reject and asserts the specific classification survives instead of being overwritten with UNKNOWN.
• No test covers concurrent upsertErrorHistory calls for the same { alert, createdAt, state } key — the existing dedupe test only exercises sequential ticks and passes with or without a unique index.
• getAlertEvaluations pagination has no test isolating truncatedByCount from truncatedByScanBound, no tie-on-identical-createdAt cursor test, and no test for an empty page returned with hasMore: true.
• AlertHistoryCards truncation and padding math is never exercised above maxItems, though the detail page passes 60 windows.
• AlertDetailPage, AlertDetailChart, and useAlertAnnotations have no tests; the recordAlertErrors branch where evaluationWindowStart is undefined is uncovered.

Coverage limitations: Bash, Grep, and Glob were unavailable in this environment (bwrap failed on every invocation, including with the sandbox disabled), so no git diff was computed — reviewers read the checked-out head and separated feature code from pre-existing code by inspection, which means added-vs-existing attribution on the large modified files is best-effort. ce-learnings-researcher was not run (docs/solutions/ could not be enumerated without Glob), and the root AGENTS.md changeset requirement could not be verified for the same reason — confirm a changeset entry exists before merge.

[github-actions]

<!-- deep-review -->

Deep Review

🔴 P0/P1 — must fix

• packages/app/src/components/alerts/AlertEvaluationsTable.tsx:209 — The scroll sentinel re-arms on every isFetchingNextPage false-transition, and because a scan-bound-truncated page legitimately returns {data: [], hasMore: true, nextBefore}, an empty page appends no rows, never scrolls the sentinel out of view, and fires the next fetch immediately — a wide picked range or an alert with sparse history turns one page view into hundreds of back-to-back aggregation requests with no user interaction.
  • Fix: Track consecutive pages that appended zero rows and stop auto-firing onLoadMore after a small threshold, falling back to an explicit "Load more" button.
  • _{julik-frontend-races, adversarial, correctness, testing}

🟡 P2 — recommended

• packages/api/src/controllers/alertHistory.ts:290 — fetchStructuredWindows $pushes a full sub-document for every matched row into one array per window and applies $limit only after $group, while ALERT_EVALUATION_GROUPS_LIMIT is applied in JS after materialization, so the scan is bounded in time but not in rows and a high-cardinality group-by alert can exceed the 100MB $group limit with no allowDiskUse fallback.
  • Fix: Cap rows per window inside the pipeline before the final $group and pass allowDiskUse: true on the aggregate.
  • _{performance, adversarial, correctness}
• packages/api/src/controllers/alertHistory.ts:455 — getAlertTransitionsInRange matches the full caller-supplied range (clamped only to 31 days) with no per-window scan bound and no allowDiskUse, unlike the sibling getAlertEvaluations that was deliberately bounded.
  • Fix: Apply the same (limit + 1) * interval scan bound (or a window-count cap) to this aggregation and set allowDiskUse: true.
• packages/api/src/tasks/checkAlerts/index.ts:1038 — The recordAlertErrors call in the query-failure branch is unwrapped inside the outer try, so a Mongo write failure propagates to the catch at line 1461, which reclassifies the error as UNKNOWN and overwrites the QUERY_TIMEOUT/QUERY_ERROR diagnostic with a hardcoded message — the identical call at line 1479 already has its own guard.
  • Fix: Wrap the line 1038 call in its own try/catch that logs and returns, mirroring the guard at line 1479.
  • _{adversarial, reliability}
• packages/api/src/tasks/checkAlerts/index.ts:827 — evaluationWindowStart is declared at line 827 but only assigned at line 869, after ms(alert.interval), normalizeScheduleStartAt, normalizeScheduleOffsetMinutes, and getScheduledWindowStart; a throw anywhere in that span reaches the catch with the value undefined, and recordAlertErrors then skips upsertErrorHistory entirely, silently dropping exactly the configuration-level failures from the evaluation history.
  • Fix: Derive a best-effort window start in the catch block when the hoisted value is still undefined so every failure is attributable to a window.
  • _{reliability, correctness, testing, adversarial}
• packages/api/src/tasks/checkAlerts/providers/default.ts:461 — upsertErrorHistory uses $set: { errors }, replacing the whole array, so when a window fails twice with different error classes across ticks (a query timeout, then a webhook failure on a later successful evaluation) the first diagnostic is destroyed even though the read path deduplicates by type and message specifically to display several.
  • Fix: Merge with $addToSet: { errors: { $each: errors } } plus a length cap, and add a test covering two distinct error types within one window.
  • _{correctness, testing}
• packages/api/src/tasks/checkAlerts/index.ts:920 — The chartConfig == null early return and the !meta early return around line 1200 only log and return, writing no AlertHistory row of any state, so a permanently broken alert renders as "No evaluations in the selected time range." on the new detail page — the page actively conceals the failure it was added to surface.
  • Fix: Call recordAlertErrors with an INVALID_ALERT error and the current window before returning from both branches, and cover each with a test.
  • _{testing, adversarial, correctness}
• packages/app/src/AlertDetailPage.tsx:129 — isError and error are never destructured from useAlertEvaluations and AlertEvaluationsTable accepts no error prop, so a failed page fetch leaves hasNextPage true from the last successful page, keeps the sentinel mounted, and re-arms indefinitely behind a permanent "Loading older evaluations…" spinner; a first-page failure instead renders the empty-state message, presenting a server error as no data.
  • Fix: Thread isError into the table, stop auto-fetching in the error state, and render a retry affordance instead of the spinner.
  • _{julik-frontend-races, adversarial, testing}
• packages/api/src/tasks/checkAlerts/providers/default.ts:422 — executionErrors accumulates one entry per failing group per evaluation, and that whole array is now copied into a 30-day-retained per-window AlertHistory row with no dedupe or element cap before the write, while the read path collapses every byte-identical WEBHOOK_ERROR entry to a single line.
  • Fix: Dedupe by type and message and cap the array length before persisting, rather than only on read.
  • _{adversarial, security}
• packages/api/src/controllers/alertHistory.ts:410 — getRecentAlertHistoriesBatch wraps each per-alert aggregation in Promise.all with no per-item catch, so one alert's query failure rejects the whole batch and the alerts list route returns 500 instead of degrading to an empty history for that one alert; the equivalent prefetch helpers in the alert task share the pattern.
  • Fix: Catch inside the queue.add callback, log, and return an empty history for the failing alert.
• packages/app/src/utils/alerts.ts:50 — extendDateRangeToInterval is new, drives the detail page's chart and evaluation range through eight interval-specific branches plus a fallback, and has no tests; the colocated test file only covers normalizeNoOpAlertScheduleFields.
  • Fix: Add unit tests for each interval branch, the already-wide no-op case, and the exact-boundary case.
  • _{testing, maintainability}
• packages/api/src/models/alert.ts:12 — AlertState is redeclared verbatim, doc comment included, alongside the definition in packages/common-utils/src/types.ts; this change had to add ERROR to both copies by hand, and the same file already re-exports AlertThresholdType from common-utils, so the correct pattern was available.
  • Fix: Import and re-export AlertState from @hyperdx/common-utils/dist/types instead of maintaining a second copy.
  • _{project-standards, maintainability, kieran-typescript}
• packages/api/src/controllers/alertHistory.ts:94 — fetchGroupedWindows/mapGroupedHistories and fetchStructuredWindows/mapStructuredWindow are near-identical $match/$sort/$group/$sort/$limit pipelines differing only in whether group identity survives the $push, and getAlertTransitionsInRange adds a third, so the ERROR-exclusion rule is now hand-written in three places that must stay consistent while the names give a reader no way to tell them apart.
  • Fix: Derive the alerts-page shape from fetchStructuredWindows, or rename to encode the real distinction such as fetchCollapsedWindows versus fetchPerGroupWindows.
• packages/api/src/tasks/checkAlerts/providers/index.ts:106 — evaluationWindowStart is optional on recordAlertErrors, so an out-of-tree AlertProvider keeps satisfying the interface with a two-argument method while silently recording zero ERROR rows, and the new detail page presents that provider's evaluation history as complete rather than incomplete.
  • Fix: Document at the interface that omitting the parameter opts out of evaluation-error history, and add a contract test asserting registered providers produce an ERROR row.
  • _{api-contract, adversarial}

🔵 P3 nitpicks (10)

• packages/api/src/models/alertHistory.ts:84 — The one-ERROR-row-per-window invariant rests only on the upsert filter; the collection has no unique index on {alert, createdAt, state}, so two concurrent ticks or replicas can both miss and both insert.
  • Fix: Add a partial unique index filtered to state: 'ERROR', or document that duplicates are expected and rely on read-side dedupe.
• packages/api/src/controllers/alertHistory.ts:491 — When the alert was firing before startTime and the first in-range window lands exactly on startTime in a non-firing state, pinCarryInIfFiring pushes an ALERT marker and the next branch immediately pushes an OK marker at the identical timestamp; minute-quantized bounds make this alignment routine.
  • Fix: Only pin the carry-in marker when the first in-range window is strictly after startTime or is itself firing.
• packages/app/src/api.ts:238 — useAlertEvaluations sets no staleTime or refetchOnWindowFocus, so a focus refetch replays each page at its original cursor and a boundary shift can yield two entries sharing a createdAt, which is used directly as the React key for rows holding local expand state.
  • Fix: Disable focus refetching for this historical query, or dedupe by createdAt when flattening pages.
• packages/api/src/controllers/alertHistory.ts:362 — The Mongo date filter is typed Record<string, Date>, so a mistyped operator key compiles and fails silently at the database layer instead of at the type checker.
  • Fix: Type it as { $gte?: Date; $lt?: Date; $lte?: Date } or FilterQuery<IAlertHistory>['createdAt'].
• packages/api/src/controllers/alertHistory.ts:243 — r.group as string papers over a non-narrowing .filter, so editing the predicate would keep compiling after the runtime guarantee the cast depends on is gone.
  • Fix: Make the filter a type predicate returning r is EvaluationWindowRow & { group: string } and drop the cast.
• packages/app/src/components/alerts/AlertHistoryCards.tsx:75 — dedupeAlertErrors reimplements the server's dedupeErrors but omits the newest-first sort, so the two have already diverged on ordering while re-deduplicating data the server already deduplicated.
  • Fix: Share one implementation from common-utils, or at minimum match the server's sort order.
• packages/app/src/components/alerts/AlertEvaluationsTable.tsx:32 — stateBadge is a third hand-written AlertState-to-presentation mapping alongside stateToBgColorClass and the inline badge switch in AlertsPage.tsx, with no shared source of truth.
  • Fix: Centralize label and color in one ALERT_STATE_PRESENTATION map that all three render paths read.
• packages/api/src/routers/api/alerts.ts:217 — The MAX_HISTORY_SPAN_MS clamp is re-derived independently in the /evaluations and /history routes with different null handling for startTime.
  • Fix: Extract a single clampStartTime(startTime, endTime, maxSpanMs) helper used by both routes.
• packages/app/src/utils/alerts.ts:50 — The interval-to-window-size ladder is written out a second time here, duplicating the pairs already encoded in intervalToDateRange a few lines above.
  • Fix: Extract one Record<AlertInterval, Duration> map and have both functions read from it.
• packages/common-utils/src/types.ts:606 — AlertState.ERROR now appears in history[].state on the pre-existing /alerts and /alerts/:id responses, widening the realized value set for consumers written against the old three states with no changelog or schema note.
  • Fix: Note the new value in the alert API docs and confirm external consumers have an unknown-state fallback.

---

Reviewers (11): correctness, adversarial, security, reliability, api-contract, performance, testing, maintainability, kieran-typescript, julik-frontend-races, project-standards.

Testing gaps:

• No test fails if state: { $ne: ERROR } is dropped from getPreviousAlertHistories or getConsecutiveWindowHistories — the retry/backfill invariant this change rests on would break silently.
• No coverage of getAlertEvaluations over a range that is entirely a gap (the empty-page-with-hasMore chain) or of a before cursor at or below startTime.
• AlertEvaluationsTable accepts no error input, so failed-page behavior is untestable without a signature change.
• No coverage of a group-by alert exceeding ALERT_EVALUATION_GROUPS_LIMIT, or of high group cardinality against the $group stage.
• AlertHistoryCardList's new history, maxItems, and showErrorIndicator props — the exact usage the detail page introduces — are exercised by no test.

Reviewer note: Bash, Grep, and Glob were unavailable in this environment, so reviewers worked from the checked-out files rather than a computed diff; findings were confirmed by reading the cited code, but new-versus-pre-existing attribution is weaker than usual, and the repo's changeset requirement could not be verified. Security review found no exploitable issues and confirmed the new endpoint resolves the alert via getAlertById(id, teamId) before reading history.

[github-actions]

Scope note: Bash was unavailable in this environment (every invocation failed at sandbox setup, with and without the override), so no git diff could be produced. Reviewers worked from the files at HEAD with the changed surface identified by symbol. Findings were verified against the actual code, but new-vs-pre-existing attribution is best-effort; where a finding sits in code that predates this work but is newly reached by it, that is stated inline.

🔴 P0/P1 — must fix

• packages/app/src/components/alerts/AlertEvaluationsTable.tsx:276 — The load-more sentinel's effect depends on isFetchingNextPage, so it re-fires every time a fetch settles, including a failed one, while hasNextPage stays true and no rows are appended to push the sentinel out of view.
  • Fix: Thread isError into the table and gate the effect on !isError, render a terminal error row with an explicit retry instead of the perpetual spinner, and re-arm auto-fetch only after the loaded page count increases.
  • _{adversarial, performance, reliability, correctness}
• packages/api/src/controllers/alertHistory.ts:323 — $group pushes one subdocument per matched AlertHistory row before $limit runs, and the 50-group cap is applied in Node at line 298, so a group-by alert's cost scales with group cardinality; allowDiskUse is unset, so the post-$group blocking sort can exceed the 100 MB stage limit and a single fat window can exceed the 16 MB BSON limit, 500-ing the new page.
  • Fix: Bound the accumulator inside the pipeline — narrow to the target createdAt values with $group/$sort/$limit first, then use $topN capped at ALERT_EVALUATION_GROUPS_LIMIT with a separate $sum for the total — and pass allowDiskUse on both aggregate calls.
  • _{security, adversarial, performance, correctness}

🟡 P2 — recommended

• packages/api/src/controllers/alertHistory.ts:255 — Each window's lastValues is the uncapped union across every group row and is returned verbatim, while the client reads only the first and last entry and skips latestValue() entirely for grouped rows.
  • Fix: Return only the earliest and latest bucket entries per window, computing the distinct-bucket count needed by resolveWindowAnalytics during the flatten instead of from the retained array.
  • _{security, adversarial, performance}
• packages/api/src/controllers/alertHistory.ts:412 — hasMore is set from the scan bound alone, so every empty scan slice returns data: [] with an advancing cursor, making the client walk a wide range one slice at a time (about 220 round trips for a 1-minute alert over the clamped span).
  • Fix: Loop the scan slice server-side, advancing pageEndMs to scanFloorMs until limit windows are collected or startTime is reached, capped by a fixed slice budget per request.
  • _{adversarial, performance, reliability, correctness, api-contract}
• packages/api/src/tasks/checkAlerts/providers/default.ts:477 — $set: { errors } replaces the whole array on the window's single ERROR row, so a query timeout recorded on one tick is erased when a later tick for the same window records a webhook failure.
  • Fix: Merge into the existing array with $addToSet/$each plus a bounded $slice, keeping one entry per distinct error type.
  • _{correctness, adversarial, reliability}
• packages/api/src/tasks/checkAlerts/index.ts:1051 — This recordAlertErrors call is not individually guarded, unlike the identical call at line 1515, so a failed Mongo write propagates to the outer catch, which reclassifies the failure as UNKNOWN and overwrites the already-persisted timeout diagnostic with the hardcoded generic message.
  • Fix: Wrap this call in its own try/catch that logs and swallows, mirroring the guarded call in the outer catch.
  • _{reliability, adversarial}
• packages/app/src/components/alerts/AlertEvaluationsTable.tsx:145 — firingGroups counts the server-capped groups array while groupsTotal is the uncapped count, so a window with 500 groups of which 300 are firing renders as 50/500 groups firing.
  • Fix: Return a server-computed firing-group total alongside groupsTotal and render that instead of counting the truncated array.
• packages/api/src/tasks/checkAlerts/index.ts:1136 — The webhookDurationMs bracket wraps all of fireChannelEvent, which for saved-search alerts runs a ClickHouse query for sample log lines before any delivery attempt, so the value surfaced as the Webhook Duration column attributes ClickHouse latency to the webhook destination.
  • Fix: Move the measurement inside the delivery path so it covers only notification I/O, or rename the field, column, and doc comment to reflect total notification time.
• packages/api/src/tasks/checkAlerts/index.ts:978 — tryOptimizeConfigWithMaterializedView performs ClickHouse I/O outside the try block that starts at line 1001, so on materialized-view-backed sources a ClickHouse outage lands in the outer catch as UNKNOWN with no queryDurationMs, bypassing the new timeout classification entirely.
  • Fix: Wrap the call so it either degrades to the unoptimized config like the adjacent alias-clause block or routes through makeQueryAlertError.
• packages/api/src/tasks/checkAlerts/index.ts:1221 — The meta == null early return, and the chartConfig == null return at line 927, set the error outcome and log but persist nothing, leaving those failed windows with no error record and no row in the new evaluation history.
  • Fix: Call recordAlertErrors with an appropriate error type and the evaluation window start on both early-return paths.
• packages/api/src/models/alertHistory.ts:114 — Nothing backs the {alert, createdAt, state} upsert key with a unique constraint, and in production the task runs as a one-shot process with no lock, so two overlapping ticks can each insert an ERROR row for the same window and inflate the per-window counts and groupsTotal read back by the endpoint.
  • Fix: Add a partial unique index on {alert, createdAt, state} filtered to state: 'ERROR' and treat a duplicate-key error in the upsert as success.
  • _{adversarial, reliability, performance, learnings-researcher}
• packages/app/src/api.ts:238 — useAlertEvaluations sets no staleTime, refetchOnWindowFocus, or maxPages, and the QueryClient has no defaultOptions, so a tab blur/focus makes React Query re-fetch every accumulated page sequentially against the heavy per-window aggregation.
  • Fix: Set a non-zero staleTime, disable refetchOnWindowFocus/refetchOnMount for this query, and cap retained pages with maxPages.
• packages/common-utils/src/types.ts:832 — analytics is declared on AlertHistorySchema, which types AlertsPageItemSchema.history, but the producer for GET /alerts and GET /alerts/:id never projects or emits it, so the same declared type carries different real field sets depending on the endpoint.
  • Fix: Move analytics onto AlertEvaluationSchema, the only response shape whose producer populates it.
• packages/api/src/controllers/alertHistory.ts:173 — AlertEvaluationEntry is hand-written alongside the zod AlertEvaluationSchema for the same payload, and because the router assigns data: page.data from a variable rather than an object literal, excess-property checking never fires, so a field added on one side is silently dropped on the other.
  • Fix: Derive the controller types from the zod-inferred types, or add a compile-time assignability assertion pinning the two together.
  • _{kieran-typescript, maintainability}
• packages/api/src/controllers/alertHistory.ts:489 — This aggregation has no $limit and pushes every matched row's state into a per-window array, bounded only by the 31-day span clamp; the new detail page fires it unconditionally for the picked range, so a 1-minute alert on a wide range scans tens of thousands of windows times group count.
  • Fix: Replace states: { $push: '$state' } with boolean $max accumulators for the firing and pending cases and bound the returned window count. (Pre-existing aggregation, newly reached at user-controlled scale by the detail page.)
• packages/api/src/tasks/checkAlerts/providers/__tests__/default.int.test.ts:1 — The provider's dedicated test file covers only getAlertTasks and the link builders, so the new updateAlertState, recordAlertErrors, and upsertErrorHistory write paths have no direct coverage; the read-side tests insert ERROR rows manually and never exercise the upsert.
  • Fix: Add tests asserting first-tick ERROR-row creation, same-window upsert into a single row across two ticks, and that $setOnInsert defaults apply only on insert.

🔵 P3 nitpicks (15)

• packages/api/src/controllers/alertHistory.ts:396 — Record<string, Date> for the Mongo filter accepts any key, so a mistyped range operator compiles and silently changes query semantics.
  • Fix: Introduce a named type with explicit $gte/$lt/$lte fields and use it for both fetch helpers.
• packages/api/src/controllers/alertHistory.ts:275 — r.group as string relies on a preceding .filter() that TypeScript cannot narrow through, so the cast keeps compiling if that condition is ever edited.
  • Fix: Make the filter a type predicate narrowing group to string and drop the cast.
  • _{kieran-typescript, project-standards}
• packages/app/src/components/alerts/AlertEvaluationsTable.tsx:223 — The error label uses a raw palette color where the repo's documented style guide calls for the semantic variant.
  • Fix: Replace c="red" with variant="danger".
• packages/api/src/routers/api/alerts.ts:188 — endTime and before are unbounded positive integers, so a value past the maximum representable date yields Invalid Date bounds that reach the aggregation, and the .refine only compares the two when both are supplied.
  • Fix: Add a max bound to the epoch-ms fields and extend the refinement to compare against the default end time when endTime is omitted.
  • _{adversarial, security}
• packages/api/src/tasks/checkAlerts/providers/default.ts:426 — Nothing removes a window's ERROR row when a later attempt succeeds cleanly, and groupStateToOverallState ranks ERROR above OK, so a recovered window reads as an error for the full retention period and its history-strip card renders as a modal button instead of the source-search link.
  • Fix: Either delete the ERROR row when a later attempt for the window records no errors, or rank ERROR below OK so the row contributes only error detail.
• packages/api/src/tasks/checkAlerts/errors.ts:34 — The constructor-name fallback asserts the full ClickHouseError shape while checking only a name string, so any class with that name is treated as a classified ClickHouse error.
  • Fix: Narrow the predicate's return type to what is actually verified, or add a structural check on type/code.
• packages/api/src/tasks/checkAlerts/providers/index.ts:108 — The ERROR-row upsert lives entirely in the default provider behind two optional trailing params, so an out-of-tree provider registered through the extension point keeps type-checking while silently producing no evaluation-error history.
  • Fix: State the persistence obligation in the interface docs, or move the upsert into shared code every provider path calls.
  • _{api-contract, reliability, maintainability}
• packages/api/src/routers/external-api/v2/alerts.ts:353 — The new per-window evaluation history, error records, and analytics have no API-key-authenticated route, and the MCP single-alert tool still reads the pipeline that never projects analytics or per-group rows, so the diagnostics are browser-session-only.
  • Fix: Add an external-API evaluations route reusing getAlertEvaluations, and point the MCP tool at the structured-window path.
• packages/api/src/models/alert.ts:151 — The Alert document's state enum now accepts ERROR, which the published v2 OpenAPI AlertState enum does not list, leaving a comment as the only guard on the history-only invariant.
  • Fix: Narrow the document's enum to the non-ERROR members via a shared constant used by the OpenAPI schema too.
• packages/api/src/controllers/alertHistory.ts:262 — Group rows are fully mapped and sorted, with a localeCompare tiebreak, before being sliced to 50, so per-request CPU scales with group cardinality on the single event-loop thread.
  • Fix: Do a bounded top-K selection instead of a full sort and use relational string comparison for the tiebreak.
• packages/api/src/controllers/alertHistory.ts:95 — fetchGroupedWindows/mapGroupedHistories and fetchStructuredWindows/mapStructuredWindow independently reimplement the same pipeline shape, lastValues comparator, and error dedupe.
  • Fix: Extract the shared comparator and dedupe, and comment why the two pipelines cannot be unified.
• packages/api/src/controllers/alertHistory.ts:1 — At 548 lines the file is well past the 300-line ceiling in the repo's documented code-style guide, with most of the growth from the new evaluation-window logic.
  • Fix: Split the structured-window and pagination functions into their own module.
• packages/api/src/tasks/checkAlerts/providers/default.ts:508 — A configured timeout of 0 passes the non-negative schema check, is not replaced by the ?? default, and is accepted by the client, so the timeout message renders as a 0-second limit while the client treats it as unlimited.
  • Fix: Require a positive value in the task-args schema, or coerce falsy values to the default in getClickHouseClient.
• packages/app/src/components/alerts/AlertEvaluationsTable.tsx:345 — Rows are neither memoized nor virtualized and pages accumulate without bound, with a tooltip instance mounted per row.
  • Fix: Wrap the row component in React.memo and cap retained pages or virtualize the table body.
• packages/api/src/controllers/alertHistory.ts:321 — The $sort immediately before a $group on the same key imposes no order any consumer relies on, since every downstream array is re-sorted, but it makes index selection load-bearing for avoiding a blocking sort.
  • Fix: Drop the pre-$group sort, or comment that it exists only to steer the query plan.

---

Reviewers (12): correctness, adversarial, security, reliability, performance, api-contract, kieran-typescript, testing, maintainability, project-standards, agent-native, learnings-researcher.

Verified as sound (no findings): team scoping on the new endpoint via getAlertById; pagination cursor progress in every branch; ERROR-row exclusion from the due-ness gate, retry range, consecutive-window counting, and transition annotations; per-alert isolation in the evaluation loop; no NoSQL-operator injection or raw-HTML sink.

Testing gaps:

• No coverage of the same-window error sequence (query failure then webhook failure) asserting what survives on the single ERROR row.
• No concurrency test asserting exactly one ERROR row exists after two parallel upserts for the same window.
• No high-cardinality group-by fixture bounding the evaluations aggregation's document count or serialized response size.
• No client test that the load-more sentinel stops after a failed page fetch, or that empty-but-hasMore pages terminate in a bounded number of fetches.
• useAlertAnnotations/alertTransitionsToAnnotations and the detail page itself have no test files.
• No test for a ClickHouse failure raised from the materialized-view optimization path, where classification currently falls through to UNKNOWN.

[github-actions]

<!-- deep-review -->

Deep Review

🔴 P0/P1 — must fix

• packages/app/src/components/alerts/AlertEvaluationsTable.tsx:276 — The sentinel effect re-runs every time isFetchingNextPage falls back to false while the sentinel is still in the viewport, so a page that errors is retried forever (hasNextPage stays true off the last successful page and no error state is surfaced) and a range whose older portion has no evaluations chains one request per scan slice — roughly 215 sequential fetches for a 1-minute alert over 30 days, since empty pages add no rows and the sentinel never scrolls out of view.
  • Fix: Stop auto-firing once a page fetch fails or returns zero rows, cap consecutive automatic fetches (plus maxPages), and fall back to an explicit "load older" control.
  • _{adversarial, correctness, performance, reliability}
• packages/api/src/controllers/alertHistory.ts:326 — The 50-group cap is applied in JavaScript in mapStructuredWindow only after $group has already $pushed all seven fields of every matched row into one array per window, and $limit runs after $group, so a group-by alert with a few thousand groups materializes on the order of 201 × group-count row objects per request and can exceed the 100MB per-stage limit (no allowDiskUse, and DocumentDB is a supported target) or the 16MB per-document ceiling, turning the endpoint into a persistent 500 for that alert; lastValues is then returned uncapped even though the UI reads only its first and last entry.
  • Fix: Bound rows per window inside the pipeline (sort/slice within $group, or a $slice projection), reduce lastValues to what the response needs, and pass allowDiskUse.
  • _{adversarial, performance, security}
• packages/api/src/routers/api/alerts.ts:217 — startTime is validated only as a positive integer, so a value above the maximum representable Date (for example 1e16) yields an Invalid Date, making scanFloorMs NaN; the $gte bound then serializes to epoch 0 while $lte stays at now, and truncatedByScanBound silently evaluates false — defeating both the 31-day span clamp and the per-request scan bound, which is the guarantee the whole pagination design rests on.
  • Fix: Add .max() upper bounds to startTime/endTime/before, and assert the resolved bounds are finite and startTime < endTime after the clamp rather than relying on the both-present refine.
  • _{security, adversarial, correctness}

🟡 P2 — recommended

• packages/api/src/controllers/alertHistory.ts:262 — The per-group filter drops non-ERROR rows whose group is null/empty, but groupStateToOverallState and the counts sum still include them, so a group-by alert with a zero-is-breach threshold and a backfilled empty bucket renders a red Alert badge and a nonzero breach count next to a breakdown in which no listed group is firing.
  • Fix: Surface the ungrouped row in the breakdown as an explicit synthetic entry counted in groupsTotal so the window's state is always explained by a visible row.
  • _{correctness, adversarial}
• packages/app/src/components/alerts/AlertEvaluationsTable.tsx:145 — firingGroups is counted from the already-capped groups array but rendered against the pre-cap groupsTotal, so once more than 50 groups fire the row reads "50/200 groups firing" instead of the true count — understating breadth during exactly the incident the breakdown exists to explain.
  • Fix: Return a pre-cap firing count from the server alongside groupsTotal and render that instead of counting the capped array.
• packages/api/src/controllers/alertHistory.ts:234 — resolveWindowAnalytics prefers a non-ERROR row's analytics but falls back to any row's, so a window holding both a failed attempt and a successful retry can show a Query Duration belonging to a different attempt than the error named in the Errors column — hiding a timeout's time-to-failure in one direction and attributing the failed attempt's timing to a successful window in the other.
  • Fix: Report the timing for the attempt the row's error describes, or expose both attempts' durations as separate fields.
  • _{reliability, adversarial}
• packages/api/src/tasks/checkAlerts/providers/default.ts:439 — Stale ERROR-row cleanup matches only createdAt === evaluationWindowStart, but a 1-minute alert's retry lands in the next rounded window, so the failed window's ERROR row survives until the 30-day TTL and the new table renders it as a permanent red row labelled with a bucket that was in fact later evaluated successfully.
  • Fix: Delete stale ERROR rows for every window covered by the evaluation's expected buckets, not just the current window start.
• packages/app/src/components/alerts/__tests__/AlertEvaluationsTable.test.tsx:154 — useInViewport is mocked to a static boolean, so the sentinel is only asserted for a single fire and a single already-fetching no-op; neither the chained-fetch loop across empty pages nor the re-fire after a failed page is exercised, leaving the P1 above with no regression guard.
  • Fix: Add a test that toggles isFetchingNextPage true→false across re-renders while inViewport and hasNextPage stay true, asserting a bounded onLoadMore call count.
  • _{testing, correctness, reliability, adversarial, maintainability}

🔵 P3 nitpicks (9)

• packages/app/src/components/alerts/AlertEvaluationsTable.tsx:136 — For raw-SQL Number (single_value) alerts lastValues[0].startTime equals createdAt, so the relabelled column still shows the evaluation time and the tooltip renders an identical start and end.
  • Fix: Derive the label as min(lastValue.startTime, createdAt − interval) so single-value alerts still show a bucket start and a non-degenerate span.
• packages/api/src/controllers/alertHistory.ts:241 — The legacy backfilledBuckets derivation (distinct(lastValues.startTime) − 1) undercounts when a bucket's rows all parse to null, yet it is returned in the same shape as a measured value and rendered with an authoritative tooltip.
  • Fix: Return derived counts in a distinct field or soften the tooltip wording so estimates are not presented as recorded measurements.
• packages/app/src/components/alerts/AlertEvaluationsTable.tsx:33 — TABLE_COLUMNS = 8 must be kept in manual sync with the eight header cells and with GroupRow's eight <Table.Td> (four intentionally empty), so adding a column silently breaks colSpan alignment.
  • Fix: Derive both the column count and GroupRow's empty-cell count from one column-descriptor array.
  • _{kieran-typescript, maintainability}
• packages/api/src/controllers/alertHistory.ts:396 — The createdAt filter is typed Record<string, Date>, so a mistyped Mongo range operator key is a silent no-op rather than a compile error.
  • Fix: Type it as { $gte?: Date; $lt?: Date; $lte?: Date } (or a driver Filter helper) in all three call sites.
• packages/api/src/controllers/alertHistory.ts:18 — Re-exporting ALERT_EVALUATION_GROUPS_LIMIT creates two valid import paths for one constant, and no consumer of the controller-side path was confirmed.
  • Fix: Drop the re-export and import the constant directly from common-utils everywhere.
  • _{maintainability, kieran-typescript, api-contract}
• packages/api/src/routers/api/alerts.ts:180 — limit is a caller-tunable 1-200 that also sets the scan-slice width, so a client sending limit=1 changes what nextBefore/hasMore mean without that coupling being documented.
  • Fix: Either document limit and its scan-bound coupling, or drop the parameter and fix the page size server-side.
• packages/api/src/routers/api/alerts.ts:163 — MAX_HISTORY_SPAN_MS is a hand-written 31 days coupled by convention to the model's ms('30d') TTL, so the oldest day of a max-span request is always empty and a TTL change silently desynchronises the clamp.
  • Fix: Derive the span cap from the TTL constant exported by the history model.
  • _{maintainability, adversarial}
• packages/app/src/api.ts:208 — The BUCKET_MS minute-quantization constant and its Math.floor expression are duplicated verbatim in useAlertHistory and useAlertEvaluations.
  • Fix: Extract one shared quantization helper used by both hooks.
• packages/api/src/routers/external-api/v2/alerts.ts:289 — The evaluation stream and its new diagnostics (queryDurationMs, webhookDurationMs, backfilledBuckets, per-group fired) exist only on the session-authenticated internal route, with no API-key/OpenAPI surface, so programmatic clients cannot retrieve what the table shows.
  • Fix: File a follow-up to expose an external v2 evaluations route reusing getAlertEvaluations, documenting the cursor and group-cap semantics.

---

Reviewers (10): correctness, adversarial, security, testing, performance, api-contract, reliability, kieran-typescript, maintainability, agent-native.

Testing gaps:

• No test covers the $gte/$lt handoff at exactly scanFloorMs, so a window on the page boundary could be duplicated or dropped without failing CI.
• Scan-bound truncation (hasMore: true with fewer than limit windows) is not asserted separately from count truncation, and no test proves nextBefore is present on every hasMore branch.
• The group cap is exercised well above the limit but not at exactly 50/51 groups, and firing-first ordering is untested when state and value tie.
• webhookDurationMs accumulation across multiple groups/resolves in one evaluation is unasserted; only the query-failure queryDurationMs path is covered.
• The legacy backfilledBuckets derivation is tested only for a single ungrouped row — not for group-by windows (where lastValues is concatenated across groups) or ERROR+legacy-row mixes.
• No test covers a persistent 500 from the evaluations endpoint to prove pagination stops retrying.

Coverage note: The environment had no working shell, grep/glob, or network, so review was performed by reading files and following the import graph; git diff was unavailable, meaning findings were scoped to the evaluation-stream feature surface rather than to literal changed lines. The project-standards reviewer was dispatched but returned no results before synthesis, so CLAUDE.md/AGENTS.md compliance and changeset policy were not audited.

[greptile-apps]

Sub-minute ranges disable history

When a valid custom range starts and ends within the same clock minute, flooring both bounds makes startTime equal endTime, so this guard disables the evaluations query and leaves the event stream empty while the chart renders the exact selected range.

Knowledge Base Used: HyperDX App Frontend (packages/app)