Skip to content

feat(schema): add Timeline display type and TimelineSeriesSchema#2190

Draft
alex-fedotyev wants to merge 10 commits into
mainfrom
claude/dashboard-timeline-tile-schema
Draft

feat(schema): add Timeline display type and TimelineSeriesSchema#2190
alex-fedotyev wants to merge 10 commits into
mainfrom
claude/dashboard-timeline-tile-schema

Conversation

@alex-fedotyev
Copy link
Copy Markdown
Contributor

Add DisplayType.Timeline to the shared enum and the accompanying TimelineSeriesSchema types. This is the first of three chained PRs landing the Timeline dashboard tile; the renderer and builder editor follow separately.

Summary

Registers DisplayType.Timeline = 'timeline' in the shared enum, adds TimelineSeriesSchema and TimelineSeries to SharedChartSettingsSchema, and updates all exhaustive Record<DisplayType, ...> maps so TypeScript compilation stays clean throughout the chain.

External API (packages/api/src/routers/external-api/v2/utils/dashboards.ts): two fall-through cases route Timeline to the existing "unsupported display type" error path, matching what Heatmap does. No Timeline tile is reachable from the external API until a later PR adds full support there.

What's in this PR

  • DisplayType.Timeline enum value in packages/common-utils/src/types.ts
  • TimelineSeriesSchema (Zod) + TimelineSeries (inferred type)
  • timelineSeries: z.array(TimelineSeriesSchema).optional() on SharedChartSettingsSchema
  • QUERY_PARAMS_BY_DISPLAY_TYPE[Timeline] and QUERY_PARAM_EXAMPLES[Timeline] in rawSqlParams.ts
  • SQL_PLACEHOLDERS[Timeline] and DISPLAY_TYPE_INSTRUCTIONS[Timeline] in ChartEditor/constants.tsx
  • External API stub (two switch fall-throughs) in dashboards.ts

What's NOT in this PR

  • The chart renderer (DBTimelineChart, DashboardTimelineChart) - in PR B
  • The builder editor (TimelineEditor, TimelineSeriesRow) - in PR C
  • Dashboard wiring (DBDashboardPage tile render path) - in PR B
  • packages/api/src/utils/zod.ts external API schema mirror - in PR B or C

Why the split

The renderer and editor touch ~1 200 lines across packages/app/. Keeping the schema as its own commit makes the exhaustive-record updates reviewable in isolation and gives PR B a clean base to target.

Test plan

  • TypeScript compilation clean across all packages (yarn nx run-many -t typecheck)
  • No new CI failures (lint, unit, knip)

alex-fedotyev and others added 2 commits May 5, 2026 07:09
Adds `DisplayType.Timeline = 'timeline'` to the shared enum and the
accompanying `TimelineSeriesSchema` + `TimelineSeries` type to
`SharedChartSettingsSchema`.

Updates all exhaustive `Record<DisplayType, ...>` maps:
- `rawSqlParams.ts`: `QUERY_PARAMS_BY_DISPLAY_TYPE` and
  `QUERY_PARAM_EXAMPLES` get Timeline SQL-parameter entries and a
  worked example query.
- `ChartEditor/constants.tsx`: `SQL_PLACEHOLDERS` gets a Timeline
  default query; `DISPLAY_TYPE_INSTRUCTIONS` gets the column-shape
  documentation (Partial Record, but added here for consistency).
- `external-api/v2/utils/dashboards.ts`: two switch fall-throughs
  route Timeline the same way as Heatmap (unsupported for external API
  export, logs an error).

This is PR A in a three-part chain (schema → renderer → builder).
Nothing in the app renders or edits Timeline tiles yet.

Co-Authored-By: Claude Opus <model> <noreply@anthropic.com>
@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented May 5, 2026

🦋 Changeset detected

Latest commit: e962188

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages
Name Type
@hyperdx/common-utils Minor
@hyperdx/api Patch
@hyperdx/app Patch
@hyperdx/otel-collector Patch

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

@vercel
Copy link
Copy Markdown

vercel Bot commented May 5, 2026

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

Project Deployment Actions Updated (UTC)
hyperdx-oss Ready Ready Preview, Comment May 7, 2026 0:06am

Request Review

@github-actions github-actions Bot added the review/tier-4 Critical — deep review + domain expert sign-off label May 5, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 5, 2026

🔴 Tier 4 — Critical

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

Why this tier:

  • Critical-path files (1):
    • packages/api/src/routers/external-api/v2/utils/dashboards.ts
  • Cross-layer change: touches frontend (packages/app) + backend (packages/api) + shared utils (packages/common-utils)

Additional context: agent branch (claude/dashboard-timeline-tile-schema)

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: 5
  • Production lines changed: 57 (+ 95 in test files, excluded from tier calculation)
  • Branch: claude/dashboard-timeline-tile-schema
  • Author: alex-fedotyev

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

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 5, 2026

PR Review

PR Review

✅ No critical issues found.

This is a clean, focused schema-only PR that lays the groundwork for the Timeline tile. Exhaustive Record<DisplayType, ...> maps are all updated, the discriminated union for TimelineSeriesSchema is well-typed, timelineSeries is correctly scoped to the Builder config (not SharedChartSettingsSchema) with a clarifying comment, and the unsupported-type fallthrough in the external API mirrors the existing Heatmap pattern. Schema-level tests cover both modes and rejection cases.

Minor (non-blocking) observations:

  • ⚠️ QUERY_PARAM_EXAMPLES[DisplayType.Timeline] is '' but Timeline registers start/endDateMilliseconds in QUERY_PARAMS_BY_DISPLAY_TYPE → consider DATE_RANGE_WHERE_EXAMPLE_SQL for consistency with Table/Pie/Number when the renderer lands.
  • ⚠️ Placeholder message "Update your client." in DBDashboardPage.tsx:815-820 is slightly misleading (same build), but the path is unreachable until the editor lands in PR C — fine to leave as a defensive shim.
  • ℹ️ External API zod mirror (packages/api/src/utils/zod.ts) is intentionally deferred per the PR description — confirm it lands in PR B/C before any Timeline tile becomes user-creatable.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 5, 2026

E2E Test Results

1 test failed • 163 passed • 3 skipped • 1236s

Status Count
✅ Passed 163
❌ Failed 1
⚠️ Flaky 5
⏭️ Skipped 3

Tests ran across 4 shards in parallel.

View full report →

Timeline has no form-based E2E path yet (renderer and editor land in PR B/C).
Excluding it from TileConfig keeps the same treatment as Heatmap and unblocks
the TypeScript build.

Co-Authored-By: Claude Opus <model> <noreply@anthropic.com>
- Extract Timeline SQL example to TIMELINE_EXAMPLE_SQL constant in
  rawSqlParams.ts and import it in ChartEditor/constants.tsx; removes
  the duplicate inline copy
- Add .superRefine() to TimelineSeriesSchema enforcing that events mode
  requires labelExpression and value_change mode requires trackColumn
- Add 4 unit tests covering both valid and invalid cases for the new
  mode-dependent validation
- Fix two em-dashes in TimelineSeriesSchema JSDoc comments

Co-Authored-By: Claude Opus <noreply@anthropic.com>
@alex-fedotyev
Copy link
Copy Markdown
Contributor Author

Good catch on both items. Applied in 77b24fe:

  • Extracted the Timeline SQL example to TIMELINE_EXAMPLE_SQL in rawSqlParams.ts; constants.tsx now imports it instead of duplicating the string
  • Added .superRefine() to TimelineSeriesSchema enforcing events mode requires labelExpression and value_change mode requires trackColumn; 4 new unit tests cover the happy and rejection paths

Prettier requires the z.object().superRefine() chain to be written as
z\n  .object()\n  .superRefine() when the chain is too long. Reformats
the schema to match.

Co-Authored-By: Claude Opus <model> <noreply@anthropic.com>
@alex-fedotyev
Copy link
Copy Markdown
Contributor Author

PR B (renderer) is open at #2192 and stacked on this branch. If you're reviewing both together, start here.

timelineSeries is a structured-builder concept (each entry has source,
where, whereLanguage, mode, labelExpression/trackColumn). Raw SQL Timeline
tiles emit ts/label/group/severity columns directly and do not use these
series definitions.

Move the field from SharedChartSettingsSchema to _ChartConfigSchema so it
sits next to markdown (the existing precedent for tile-type-specific
complex fields on the Builder side) instead of leaking onto every Raw SQL
chart config. Adds a short comment at the call site explaining the choice.

No consumers read timelineSeries today, so this is a pure schema cleanup.
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 6, 2026

Compound Engineering Review

Schema-only PR adding DisplayType.Timeline and TimelineSeriesSchema. Security-clean. Two structural P1s (silent-broken-tile path; discriminated-union shape) and a few simplicity P1s worth addressing before the renderer PRs land against this contract.

P1 — Block before merge

  • P1 packages/common-utils/src/types.ts:767displayType: z.nativeEnum(DisplayType) immediately accepts "timeline" over the internal /api/dashboards POST/PATCH, but DBDashboardPage.tsx:802-823 has no Timeline branch and no default, so a script-created Timeline tile saves and renders blank. → Either guard the enum (z.enum([...]) excluding Timeline) until the renderer lands, or add default: assertExhaustive(displayType) + a "client update required" placeholder in DBDashboardPage.
  • P1 packages/common-utils/src/types.ts:705-758superRefine mode split exposes labelExpression? and trackColumn? as optional on every value regardless of mode; allows value_change payloads to carry silently-ignored labelExpression/severityExpression; no consumer narrowing. → Refactor to z.discriminatedUnion('mode', [...]) with events/value_change variants, matching existing OnClickSchema pattern just above.
  • P1 packages/common-utils/src/types.ts:744,751!val.labelExpression and !val.trackColumn accept empty strings; populated "" only fails downstream with confusing errors. → Use z.string().min(1) on the mode-required fields (subsumed by the discriminated-union refactor).
  • P1 packages/common-utils/src/types.ts:710-735 — JSDoc on groupExpression/severityExpression/trackColumn describes renderer mechanics ("SQL PARTITION BY for the LAG window function", "FATAL/ERROR/WARN/INFO/DEBUG/TRACE coloring", "renderer emits a marker each time…") for code that doesn't exist in this PR; guaranteed to drift. → Trim each comment to one line stating what the field IS, defer renderer prose to the renderer PR.
  • P1 packages/common-utils/src/rawSqlParams.ts:114-126 and packages/app/src/components/ChartEditor/constants.tsx:50,146-192TIMELINE_EXAMPLE_SQL (13 lines) and DISPLAY_TYPE_INSTRUCTIONS[Timeline] (50 lines) commit to a column contract (ts/label/group/severity) for a display type users can't select yet. → Drop both; ship alongside the renderer PR. (QUERY_PARAM_EXAMPLES[Timeline]: '' matches Search/Heatmap/Markdown.)

P2 — Should fix

  • P2 packages/common-utils/src/__tests__/types.test.ts — Tests only cover the superRefine it was paired with; missing: invalid mode value, empty-string labelExpression/trackColumn, severityExpression/groupExpression round-trip on parsed events, _ChartConfigSchema integration with nested invalid timelineSeries. → Add four small cases.
  • P2 packages/app/tests/e2e/page-objects/DashboardPage.ts:17Exclude<DisplayType, 'heatmap' | 'timeline'> uses enum value-strings, not enum members; silently breaks if a value is renamed. → Exclude<DisplayType, DisplayType.Heatmap | DisplayType.Timeline> (drive-by fix the 'heatmap' form too).
  • P2 packages/api/src/routers/external-api/v2/utils/dashboards.ts:166-174,271-281 and :288-305 — Heatmap/Timeline fall into the "unsupported" logger.error branch and the GET converter then silently coerces unknown tiles into a default Line tile, losing the user's intent on round-trip. Per-tile error log fires on every read. → File a follow-up issue; have convertTileToExternalChart return undefined and filter, rather than coercing-to-Line, before Timeline ships to users.
  • P2 Cross-file (types.ts:705-758 Builder vs rawSqlParams.ts:114-125 raw-SQL ts/label/group/severity convention) — Two divergent expressiveness models for the same display type (Builder has value_change/LAG semantics; raw-SQL has none; severity is severityExpression vs literal column). → Have the renderer PR introduce a single TimelineRenderModel both modes compile to; document the convergence point in a comment on TimelineSeriesSchema now.
  • P2 packages/api/src/routers/external-api/v2/utils/dashboards.ts:166,269 — Add // TODO(#<issue>) next to the unsupported branches so external-API + MCP (mcp/tools/dashboards/schemas.ts, mcp/prompts/dashboards/content.ts, utils/zod.ts) get coordinated updates when the Timeline builder PR lands; otherwise agent-native parity drifts from UI users.

Security-sentinel and agent-native-reviewer cleared the PR (no P0/P1). The SQL-string-as-z.string() pattern matches every other expression field; parameterized {startDateMilliseconds:Int64}/{endDateMilliseconds:Int64} is the canonical safe form; alert pipeline (displayTypeSupportsRawSqlAlerts / isTimeSeriesDisplayType) is an allowlist so Timeline can't accidentally enable alerting.

P1 fixes:
- Refactor TimelineSeriesSchema to z.discriminatedUnion('mode', [...])
  matching OnClickSchema pattern; removes superRefine and makes
  mode-required fields (labelExpression, trackColumn) non-optional with
  z.string().min(1), rejecting empty strings
- Trim JSDoc on schema fields to one-line descriptions; remove
  renderer-mechanics prose that doesn't belong in this PR
- Drop TIMELINE_EXAMPLE_SQL and DISPLAY_TYPE_INSTRUCTIONS[Timeline] —
  both committed to a raw-SQL column contract for a tile users can't
  select yet; replaced with empty string / omitted from the partial map
- Add Timeline placeholder in DBDashboardPage so API-created tiles
  render a message instead of blank

P2 fixes:
- Fix Exclude<DisplayType, 'heatmap' | 'timeline'> to use enum members
  so renames don't silently break the exclusion
- Add TODO(#2226) comments next to the unsupported Timeline branches in
  external-api dashboards.ts for coordinated follow-up
- Add missing test cases: invalid mode, empty-string labelExpression /
  trackColumn, optional-fields round-trip on events series

Follow-up issue for external-API Timeline support: #2226
@alex-fedotyev
Copy link
Copy Markdown
Contributor Author

All P1s and P2s addressed in c9f2b58.

P1s:

  • Discriminated union: refactored TimelineSeriesSchema to z.discriminatedUnion('mode', [...]) matching OnClickSchema. Mode-required fields are now z.string().min(1) in their respective variants, not optional on the base type.
  • JSDoc: trimmed all field descriptions to single-line; renderer-mechanics prose removed.
  • TIMELINE_EXAMPLE_SQL / DISPLAY_TYPE_INSTRUCTIONS[Timeline]: dropped both. Raw SQL example is ''; instructions entry omitted from the partial map.
  • DBDashboardPage placeholder: Timeline tiles now render a 'Timeline renderer not yet available. Update your client.' message instead of blank.

P2s:

… errors

ESLint's no-fallthrough rule treats comments between stacked case labels as
a break in the fallthrough chain, requiring an explicit break before each
subsequent case. Moving the TODO comments above the case group restores the
valid empty-fallthrough pattern.
@alex-fedotyev
Copy link
Copy Markdown
Contributor Author

Converting to draft. PR B (#2192, the renderer) needs to be reshaped before this chain has anywhere to land, so there's no point holding a tier-4 reviewer slot in the meantime. Shard-3 e2e failure is the pre-existing custom-time-range flake, not this PR. Will mark ready again once the renderer side is in shape.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

compound-review review/tier-4 Critical — deep review + domain expert sign-off

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant