Repository navigation
[HDX-4003] [HDX-4004] feat(cli): dashboard list/export/create/import and saved-searches commands - #2914
Conversation
- hdx dashboards create --file <json>: validates against DashboardWithoutIdSchema locally, auto-generates missing tile ids, and POSTs to /dashboards - hdx saved-searches list/create: list saved searches and create new ones (--name/--source/--where/--where-language/--select/--order-by/--tags), resolving sources by name or id - fix getSavedSearches() to hit /saved-search (the API mounts the singular path; the old /saved-searches always 404'd and was silently swallowed)
🦋 Changeset detectedLatest commit: eb13849 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
🟡 Tier 3 — StandardIntroduces new logic, modifies core functionality, or touches areas with non-trivial risk. Why this tier:
Review process: Full human review — logic, architecture, edge cases. Stats
|
 [Medium impact] The changes affect dashboard and saved-search workflows in the CLI and TUI. The PR needs fixes before merging because dashboard exports lose saved time ranges and child commands ignore parent flags.
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Deep Review✅ No critical issues found. This PR adds 🟡 P2 — recommended
🔵 P3 nitpicks (4)
Reviewers (11): correctness, testing, maintainability, project-standards, api-contract, reliability, kieran-typescript, adversarial, previous-comments, agent-native, learnings-researcher. Testing gaps:
No finding blocks merge automatically. A maintainer will expect P0/P1 findings in code this PR changes to be fixed; P2/P3 are your call -- fix or reply. Never fix findings about surrounding code here; reply instead. Do not widen the PR. How to respond |
E2E Test Results✅ All tests passed • 457 passed • 1 skipped • 1576s
Tests ran across 4 shards in parallel. |
…3, HDX-4004) - hdx dashboards list (default subcommand): --query name filter and --format table|json|csv; --json kept as a back-compat alias - hdx dashboards export --id <id-or-name>: emits a round-trip-safe definition (server-managed fields stripped, re-validated against DashboardWithoutIdSchema so export | import always round-trips) - hdx dashboards create: new inline-chart mode (--name + repeatable --chart / --chart-file with auto grid layout) alongside --file, plus --if-not-exists for idempotent creation - hdx dashboards import --file: recreates exported dashboards with --if-not-exists and --name-override - add 'dashboard' alias for the command group
wrn14897
changed the title
Duplicate code analysisNo new duplicate code introduced by this PR. jscpd compared this branch against |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| const doc: Record<string, unknown> = { | ||
| name: dashboard.name, | ||
| tags: dashboard.tags ?? [], | ||
| tiles: dashboard.tiles, |
There was a problem hiding this comment.
🟠 major — dashboards export keeps each tile's config.alert, so export | import creates a second copy of every tile alert
getDashboards (packages/api/src/controllers/dashboard.ts:160-171) attaches the live alert to every tile.config.alert, and the TileSchema re-parse keeps it. On import, POST /dashboards → syncDashboardAlerts → createOrUpdateDashboardAlerts (controllers/alerts.ts:417) upserts a new alert for the new dashboard id. Every alert on the original then fires twice, and a cross-team import points at webhooks the importing team doesn't own. Remove config.alert from tiles before output, the way the web export does with TileTemplateSchema / *SavedChartConfigWithoutAlertSchema (packages/common-utils/src/types.ts:2208). You could also reuse convertToDashboardTemplate.
Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond
| 'savedQuery', | ||
| 'savedQueryLanguage', | ||
| 'savedFilterValues', | ||
| 'savedSourceFilterValues', |
There was a problem hiding this comment.
🔵 minor — Hand-written export key list drops savedDateRange and includes savedSourceFilterValues, a field that doesn't exist
DashboardSchema (common-utils/src/types.ts:2504) has savedDateRange, but the allowlist leaves it out, so the export → import round trip the PR promises loses the dashboard's saved time range. savedSourceFilterValues isn't defined anywhere in the repo. Drop the manual list: run DashboardWithoutIdSchema.safeParse(dashboard) directly. Zod already strips id, _id, createdAt and other unknown keys.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| const tile: Record<string, unknown> = isTile | ||
| ? { ...(chart as Record<string, unknown>) } | ||
| : { config: chart }; | ||
| tile.x = tile.x ?? (i % 2) * 12; |
There was a problem hiding this comment.
🔵 minor — Auto-layout places tiles by their index among all charts, so bare configs can overlap tiles that set their own position
x/y come from i, which also counts tiles that have explicit layout. Example: --chart '{"x":0,"y":0,"w":24,"h":4,"config":…}' --chart '<bare config>' puts the second tile at x=12, y=0, on top of the first. Lay out only the tiles that have no position, and start below the lowest y + h of the explicit tiles.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| tags: | ||
| typeof opts.tags === 'string' && opts.tags.length > 0 | ||
| ? opts.tags | ||
| .split(',') |
There was a problem hiding this comment.
🔵 minor — Comma-separated tag parsing is written out twice when splitAndTrimCSV already does it
The split/trim/filter pipeline appears in both dashboards create and saved-searches create (line 1695). Use splitAndTrimCSV from @hyperdx/common-utils/dist/core/utils, as the REQUIRED 'Before you add a type, schema, or helper' convention says.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| * Random id for dashboard tiles, matching the web frontend's `makeId()` | ||
| * (packages/app/src/utils/tilePositioning.ts). | ||
| */ | ||
| const makeTileId = () => |
There was a problem hiding this comment.
🔵 minor — New dashboard authoring logic (tile id fill, auto-layout, export field stripping, round trip) has no tests
Move validateDashboardOrExit's id filling, the chart-to-tile layout code and the export document builder into a module under packages/cli/src/ that doesn't depend on Commander. Add tests in src/__tests__/ alongside tileConfig.test.ts. They should check that an exported dashboard re-parses with DashboardWithoutIdSchema with no alert fields and with savedDateRange kept, and that mixed explicit/bare charts don't overlap.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
PR Review
5 finding(s): 🔴 0 critical · 🟠 1 major · 🔵 4 minor 5 posted as inline comment(s) on the changed lines. Severity is the reviewer's own estimate and is used for ordering, not filtering. No finding blocks merge automatically; a maintainer decides. |
| for (const key of [ | ||
| 'filters', | ||
| 'savedQuery', | ||
| 'savedQueryLanguage', | ||
| 'savedFilterValues', | ||
| 'savedSourceFilterValues', | ||
| 'containers', | ||
| ] as const) { |
There was a problem hiding this comment.
export drops savedDateRange, even though the shared schema accepts it and the web page uses it as the dashboard’s default time range. Exporting and importing a dashboard with a saved range silently changes which period its charts show. Include savedDateRange in the exported fields.
| for (const key of [ | |
| 'filters', | |
| 'savedQuery', | |
| 'savedQueryLanguage', | |
| 'savedFilterValues', | |
| 'savedSourceFilterValues', | |
| 'containers', | |
| ] as const) { | |
| for (const key of [ | |
| 'filters', | |
| 'savedQuery', | |
| 'savedQueryLanguage', | |
| 'savedFilterValues', | |
| 'savedDateRange', | |
| 'savedSourceFilterValues', | |
| 'containers', | |
| ] as const) { |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
|
||
| const dashboard = validateDashboardOrExit(parsedJson); | ||
|
|
||
| const client = await ensureSession(opts.appUrl); |
There was a problem hiding this comment.
Options before a dashboard subcommand are stored on dashboardsCmd, but the child actions read only their own opts. For example, hdx dashboards --app-url <other-server> create --file dashboard.json ignores the requested server and falls back to the saved session’s server. This can create the dashboard on the wrong server.
The same placement loses --json, --format, and --query. Merge the parent options into each child’s options while preserving explicit child values.
|
|
||
| /** Quote a value for CSV output (RFC 4180 style). */ | ||
| function csvField(value: string): string { | ||
| return /[",\n]/.test(value) ? `"${value.replaceAll('"', '""')}"` : value; |
There was a problem hiding this comment.
csvField does not quote values containing a carriage return (\r). Dashboard names and tags accept that character, so a value such as team\rwest is emitted as an unquoted field and can split a CSV record. Include \r in the characters that trigger quoting.
| return /[",\n]/.test(value) ? `"${value.replaceAll('"', '""')}"` : value; | |
| return /[",\r\n]/.test(value) ? `"${value.replaceAll('"', '""')}"` : value; |
| const dashboardsCmd = program | ||
| .command('dashboards') | ||
| .description('List dashboards with tile summaries') | ||
| .alias('dashboard') | ||
| .description('List, export, create, or import dashboards') | ||
| .option('-a, --app-url <url>', 'HyperDX app URL') | ||
| .option('--json', 'Output as JSON (for programmatic consumption)') | ||
| .option('--json', 'Output as JSON (shorthand for --format json)') | ||
| .option('--format <fmt>', 'Output format: table, json, or csv', 'table') | ||
| .option('--query <substr>', 'Filter dashboards by name (substring match)') |
There was a problem hiding this comment.
The new dashboard and saved-search commands add hundreds of lines to cli.tsx, which now has 2,416 lines. This violates the repository’s directive, “File Size: Max 300 lines - refactor when approaching limit.” Move the new command groups and helpers into focused modules. This repository requirement must be satisfied before merging.
Context Used: CLAUDE.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Why
The
hdxCLI could list dashboards and render tiles, but had no way to create, export, or import dashboards or saved searches — authoring them required the web UI or raw API calls. This implements the dashboard read/write command surface from HDX-4003 and HDX-4004, making the CLI a complete authoring surface for scripted/agent-driven workflows (author a dashboard, create it, then immediately validate every tile withhdx chart -d).What
Dashboards (
hdx dashboards, aliashdx dashboard)list(default subcommand; barehdx dashboardsunchanged) — adds--query <substr>name filtering and--format table|json|csv(--jsonkept as back-compat alias). CSV emitsid,name,tags,createdAt,updatedAt,tileCount. (HDX-4003)export --id <id-or-name>— prints the full dashboard definition (charts, layout, filters) as JSON with server-managed fields stripped, re-validated againstDashboardWithoutIdSchemasoexport | importis guaranteed round-trip compatible. Legacy dashboards that no longer validate are still exported with a stderr warning. (HDX-4003)create— two modes: (HDX-4004)--file <json>(or-for stdin): fullDashboardWithoutIddefinition, validated locally with readable per-issue zod errors; missing tile ids auto-generated with the web'smakeId()pattern--name <name>+ repeatable--chart '<json>'/--chart-file <path>: each chart is a full tile ({x,y,w,h,config}) or a bare chart config — charts without layout auto-flow onto the 24-column grid (12×4, two per row)--if-not-existsskips creation when the name already exists (prints the existing id/URL, exit 0)import --file <json>— recreates an exported dashboard, with--if-not-existsand--name-override. (HDX-4004)Saved searches (
hdx saved-searches)list(default) — human-readable or--jsoncreate --name --source <name-or-id> --where [--where-language lucene|sql] [--select] [--order-by] [--tags]— resolves sources by name or idBug fix
ApiClient.getSavedSearches()requested/saved-searches, but the API mounts the router at the singular/saved-search(packages/api/src/api-app.ts). The call always 404'd and the failure was silently swallowed in the TUI. Now fixed — saved searches load in the TUI source picker again.Notes
hdx dashboard(singular) and-s <url>; this follows the CLI's established conventions (hdx dashboards,-a/--app-url—-salready means--sourceonchart) and adds adashboardalias for the group.packages/cli/AGENTS.md; changeset included (@hyperdx/climinor).Validation
Verified end-to-end against a live deployment with OTel Demo data:
hdx dashboards create --file— all accepted by the server-side schema; every tile renderedstatus: okviahdx chart -d <dashboard> --json.list --query flow --format csv/--format json/ bare--jsonback-compat all verified.export --id "Shop Fleet Overview"→import --name-override "… (copy)"→ all 9 tiles of the copy renderedok→ re-runningimport --if-not-existsskipped with the existing id.create --name "CLI Smoke Test" --chart '…' --chart '…'auto-laid-out both tiles and both renderedok;--if-not-existson the same name skipped. (Test dashboards deleted afterwards.)hdx saved-searches create(lucene + sql filters, tags) and confirmed withhdx saved-searches list.yarn tsc --noEmit,yarn ci:lint, and prettier pass.