-
Notifications
You must be signed in to change notification settings - Fork 21
fix(cli): scope checks list summary bar to account-wide activated state #1291
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
26a7a43
9b2acb0
9c0848a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -61,8 +61,9 @@ export default class ChecksList extends AuthCommand { | |
| const { page, limit } = flags | ||
|
|
||
| try { | ||
| // All filtering is server-side — single paginated API call | ||
| const [paginated, statuses] = await Promise.all([ | ||
| // Paginated + filtered for the table; full account fetch drives the | ||
| // account-wide summary bar, which never reacts to filters. | ||
| const [paginated, allChecks, statuses] = await Promise.all([ | ||
| api.checks.getAllPaginated({ | ||
| limit, | ||
| page, | ||
|
|
@@ -71,6 +72,7 @@ export default class ChecksList extends AuthCommand { | |
| search: flags.search, | ||
| status: flags.status, | ||
| }), | ||
| api.checks.fetchAll(), | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Useful? React with 👍 / 👎. |
||
| api.checkStatuses.fetchAll().catch(() => []), | ||
| ]) | ||
|
|
||
|
|
@@ -112,7 +114,7 @@ export default class ChecksList extends AuthCommand { | |
| // Table output | ||
| const output: string[] = [] | ||
|
|
||
| output.push(formatSummaryBar(statuses, totalChecks)) | ||
| output.push(formatSummaryBar(allChecks, statuses)) | ||
| output.push('') | ||
|
|
||
| if (checks.length === 0) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,8 @@ import { AuthCommand } from '../authCommand' | |
| import { outputFlag } from '../../helpers/flags' | ||
| import * as api from '../../rest/api' | ||
| import { batchQuickRangeValues, type BatchQuickRange } from '../../rest/batch-analytics' | ||
| import type { Check } from '../../rest/checks' | ||
| import type { CheckStatus } from '../../rest/check-statuses' | ||
| import type { CheckWithStatus, PaginationInfo } from '../../formatters/checks' | ||
| import { formatSummaryBar, formatPaginationInfo } from '../../formatters/checks' | ||
| import type { OutputFormat } from '../../formatters/render' | ||
|
|
@@ -72,32 +74,39 @@ export default class ChecksStats extends AuthCommand { | |
|
|
||
| let checksWithStatus: CheckWithStatus[] | ||
| let totalChecks: number | ||
| let allChecks: Check[] | ||
| let allStatuses: CheckStatus[] | ||
|
|
||
| if (explicitIds.length > 0) { | ||
| // Fetch all checks (paginate through all pages), filter to requested IDs | ||
| const [allChecks, statuses] = await Promise.all([ | ||
| ;[allChecks, allStatuses] = await Promise.all([ | ||
| api.checks.fetchAll(), | ||
| api.checkStatuses.fetchAll().catch(() => []), | ||
| ]) | ||
| const statusMap = new Map(statuses.map(s => [s.checkId, s])) | ||
| const statusMap = new Map(allStatuses.map(s => [s.checkId, s])) | ||
| const idSet = new Set(explicitIds) | ||
| checksWithStatus = allChecks | ||
| .filter(c => idSet.has(c.id)) | ||
| .map(c => ({ ...c, status: statusMap.get(c.id) })) | ||
| totalChecks = checksWithStatus.length | ||
| } else { | ||
| // Paginated fetch with filters | ||
| const [paginated, statuses] = await Promise.all([ | ||
| // Paginated fetch with filters for the table; account-wide fetch drives | ||
| // the summary bar, which doesn't react to filters. | ||
| const paginatedResult = await Promise.all([ | ||
| api.checks.getAllPaginated({ | ||
| limit, | ||
| page, | ||
| tag: flags.tag, | ||
| checkType: flags.type, | ||
| search: flags.search, | ||
| }), | ||
| api.checks.fetchAll(), | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
In the non-explicit-ID path, Useful? React with 👍 / 👎. |
||
| api.checkStatuses.fetchAll().catch(() => []), | ||
| ]) | ||
| const statusMap = new Map(statuses.map(s => [s.checkId, s])) | ||
| const paginated = paginatedResult[0] | ||
| allChecks = paginatedResult[1] | ||
| allStatuses = paginatedResult[2] | ||
| const statusMap = new Map(allStatuses.map(s => [s.checkId, s])) | ||
| checksWithStatus = paginated.checks.map(c => ({ ...c, status: statusMap.get(c.id) })) | ||
| totalChecks = paginated.total | ||
| } | ||
|
|
@@ -161,11 +170,7 @@ export default class ChecksStats extends AuthCommand { | |
|
|
||
| // Terminal output | ||
| const output: string[] = [] | ||
| const statuses = checksWithStatus | ||
| .map(c => c.status) | ||
| .filter((s): s is NonNullable<typeof s> => s != null) | ||
| const activeCheckIds = new Set(checksWithStatus.map(c => c.id)) | ||
| output.push(formatSummaryBar(statuses, totalChecks, activeCheckIds)) | ||
| output.push(formatSummaryBar(allChecks, allStatuses)) | ||
| output.push('') | ||
| output.push(formatBatchStats(rows, range, fmt)) | ||
| output.push('') | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,87 @@ | ||
| # API response fixtures | ||
|
|
||
| Sanitized samples of `/v1/checks` and `/v1/check-statuses` responses, drawn | ||
| from a real Checkly production account and scrubbed of account-specific data. | ||
| Use via the typed helpers in `./index.ts`: | ||
|
|
||
| ```ts | ||
| import { scenario } from './' | ||
|
|
||
| const { checks, statuses } = scenario('all-deactivated') | ||
| ``` | ||
|
|
||
| ## Contents | ||
|
|
||
| - `checks.json` — 19 checks covering a mix of active/deactivated states across | ||
| API, BROWSER, HEARTBEAT, MULTI_STEP, PLAYWRIGHT, AGENTIC check types. | ||
| - `check-statuses.json` — 16 statuses (3 of the 19 checks legitimately lack a | ||
| status entry — they're either freshly created or deactivated without a run). | ||
|
|
||
| ## Scenarios (via `scenario(name)`) | ||
|
|
||
| | Name | Description | | ||
| | --------------------- | ------------------------------------------------------ | | ||
| | `mixed` | Full 19-check fixture. The canonical dataset. | | ||
| | `all-passing` | 6 active checks, all passing. Healthy-account baseline.| | ||
| | `all-deactivated` | 5 deactivated checks (3 with stale statuses). | | ||
| | `active-no-statuses` | 1 active check with no status entry. New-check case. | | ||
| | `empty` | `{ checks: [], statuses: [] }`. | | ||
|
|
||
| For edge cases not represented in real data (pure `hasErrors`, | ||
| deactivated+failing), mutate the returned fixture in the test that needs it. | ||
|
|
||
| ## Sanitization | ||
|
|
||
| The following were remapped or stripped: | ||
|
|
||
| - `id` → `check-NNN` (sequential) | ||
| - `name` → `"Check NNN"` | ||
| - `description` → `null` | ||
| - `request.url` → `https://example.com/check-NNN` | ||
| - `tags`, `privateLocations` → sequential synthetic names | ||
| - `groupId` → sequential small integers | ||
| - `created_at` / `updated_at` → synthetic dates starting `2025-01-01` | ||
| - `longestRun` / `shortestRun` → rounded to nearest 100ms | ||
| - `script`, `environmentVariables`, `alertChannelSubscriptions`, | ||
| `alertSettings`, `localSetupScript`, `localTearDownScript`, | ||
| `setupSnippetId`, `tearDownSnippetId`, `sslCheckDomain` → dropped entirely | ||
|
|
||
| Retained as-is (not sensitive): | ||
| - `checkType`, `activated`, `muted` | ||
| - `frequency`, `frequencyOffset` | ||
| - AWS region codes in `locations` | ||
| - Public `runtimeId` version strings | ||
| - `heartbeat` config on HEARTBEAT checks | ||
| - Status booleans (`hasFailures`, `hasErrors`, `isDegraded`) and | ||
| `sslDaysRemaining` | ||
|
|
||
| ## Empirical API quirks verified at capture time | ||
|
|
||
| Documented here because they materially affect how the CLI should treat the | ||
| data — they're not obvious from the type declarations. | ||
|
|
||
| 1. **`/v1/check-statuses` ignores `limit` and `page`** query parameters. It | ||
| always returns all statuses that exist for the account, in one response. | ||
| The `content-range` header can be misleading if those params are passed. | ||
|
|
||
| 2. **`/v1/check-statuses` includes statuses for deactivated checks.** The | ||
| webapp filters these out client-side when rendering its summary bar; the | ||
| CLI must do the same. This fixture reproduces that: `check-015`, `-016`, | ||
| and `-017` are deactivated but appear in `check-statuses.json`. | ||
|
|
||
| 3. **Not every check has a status entry.** Brand-new checks that have never | ||
| run are absent from the status response (e.g. `check-013` here). | ||
| Deactivated checks that never ran are also absent. | ||
|
|
||
| 4. **`/v1/checks?status=X` server filters already exclude deactivated checks** | ||
| — confirmed empirically. No client-side `activated` check is needed when | ||
| applying `status=` to the checks endpoint; only the summary-bar needs | ||
| client-side activated filtering, because the status endpoint isn't | ||
| filter-aware. | ||
|
|
||
| 5. **Per-type shape varies.** HEARTBEAT checks have no `frequency`, | ||
| `locations`, `privateLocations`, or `groupId`; instead they carry a | ||
| `heartbeat` object with `period`/`periodUnit`/`grace`/`graceUnit`. Other | ||
| types carry `frequency` and `locations`. The declared `Check` interface in | ||
| `packages/cli/src/rest/checks.ts` does not currently reflect this | ||
| divergence — something to be aware of if tests run into missing fields. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we fetch all, why also fetching paginated? That looks like quite a waste. I get we're skipping the pagination logic that'd need to be replicated locally, but also 😩
Idk, that feels a bit much.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I need to dig a bit deeper tomorrow
Basically; the status bar requires data that is not efficiently returned via our public api, i'll check tomorrow if there's an easy workaround, extending the public api or simplifying the status bar further