Skip to content

feat: Accept variable-keyed dashboard filter values - #2963

Open
pulpdrew wants to merge 1 commit into
mainfrom
drew/name-based-var-state
Open

feat: Accept variable-keyed dashboard filter values#2963
pulpdrew wants to merge 1 commit into
mainfrom
drew/name-based-var-state

Conversation

@pulpdrew

@pulpdrew pulpdrew commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Summary

Part 1/2 in enabling variable-name-based dashboard filter state. This part covers the types/schema changes and the MCP + API + Import paths.

Why

Historically, dashboard filter selection state has been persisted (in the URL, dashboard documents, and exports) as keyed by the filter's expression, in the same format used by search-page filters. This has two problems:

  1. Filters that share an expression (eg. ServiceName from two different sources) must necessarily share a selection state despite being two dropdowns in the UI.
  2. Future "static custom values" filters will have no expression (since they're not queried) and thus would have no way to key their state

What

This change will persist dashboard filter selection state (that is, what values are selected in the filter's drop-down) based on the variableName of the filter, when it has one.

  • Existing expression-keyed filter state will be migrated on write. Existing URLs and saved filter states continue working.
  • Filters that are not variable-enabled continue being written in expression-keyed format
  • Variable-keyed state takes precedence over expression-keyed state, when both may apply to a single filter.

Screenshots or video

How to test

This will be easier to test on top of PR 2/2

  • Create a dashboard and add some filters. Make some of them variable enabled and some of them not
  • Select values for the filters
  • Try saving the default filter values, importing/exporting the dashboard, sharing the link, etc
  • Try editing the saved values through MCP + External API

References

  • Linear Issue:
  • Related PRs:

@changeset-bot

changeset-bot Bot commented Aug 21, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: dd0c178

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 Patch
@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

vercel Bot commented Aug 21, 2026

Copy link
Copy Markdown

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

Project Deployment Actions Updated (UTC)
hyperdx-oss Ready Ready Preview Aug 24, 2026 2:38pm
hyperdx-storybook Ready Ready Preview Aug 24, 2026 2:38pm

Request Review

@greptile-apps

greptile-apps Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR extends dashboard filter persistence and external API schemas to accept selections keyed by dashboard variable names while retaining legacy expression-keyed entries.

  • Adds shared variable-value schemas, parsing, serialization, and selection-resolution helpers.
  • Updates dashboard app types and URL parsing to accept the expanded value union.
  • Updates external API validation, OpenAPI documentation, and integration coverage for mixed saved-filter values.

Confidence Score: 3/5

The PR does not appear safe to merge because variable-keyed selections can still be ignored on load and removed when another dashboard filter changes.

The earlier read-path issue remains because variable entries are filtered out before active variables are derived, despite the reply stating that it is addressed by the second PR; separately, the mutation path rebuilds URL state from expression entries alone and the save path persists that rewritten state. The earlier reply from greptile conceded that the mutation issue was not reachable in this PR, but the current API and initialization path provide the concrete counterexample by accepting variable-keyed savedFilterValues and copying them into the URL before setFilterValue runs.

Files Needing Attention: packages/app/src/hooks/useDashboardFilters.tsx and packages/app/src/DBDashboardPage.tsx

Important Files Changed

Filename Overview
packages/app/src/hooks/useDashboardFilters.tsx Expands URL state typing but still strips variable-keyed entries from both the mutation and active-state derivation paths.
packages/app/src/DBDashboardPage.tsx Uses the expanded parser and round-trips raw filter URL state between saved dashboards and subsequent saves.
packages/common-utils/src/dashboardFilterValues.ts Adds shared parsing, serialization, key selection, and precedence helpers for mixed expression- and variable-keyed state.
packages/common-utils/src/types.ts Extends the shared dashboard schema with bounded variable-keyed filter values.
packages/api/src/utils/zod.ts Extends external dashboard validation to accept strict variable-keyed saved-filter entries.
packages/api/src/routers/external-api/v2/dashboards.ts Documents the new saved-filter union and its limits in the external API annotations.
packages/api/openapi.json Publishes the matching SQL-or-variable saved-filter union in the generated API contract.

Reviews (6): Last reviewed commit: "feat: Accept variable-keyed dashboard fi..." | Re-trigger Greptile

Comment thread packages/app/src/hooks/useDashboardFilters.tsx
Comment thread packages/common-utils/src/__tests__/dashboardFilterValues.test.ts
@github-actions

github-actions Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

E2E Test Results

All tests passed • 308 passed • 1 skipped • 1055s

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

Tests ran across 4 shards in parallel.

View full report →

@pulpdrew
pulpdrew marked this pull request as ready for review August 21, 2026 14:32
@github-actions github-actions Bot added the review/tier-4 Critical — deep review + domain expert sign-off label Aug 21, 2026
@github-actions

Copy link
Copy Markdown
Contributor

🔴 Tier 4 — Critical

Touches authentication, tenancy data models, the public API or shipped database config — or substantially changes the query rendering engine, background tasks, the OTel pipeline, image build, or release CI.

Why this tier:

  • Critical-path files (1) — tenancy, public API, or shipped database config:
    • packages/api/src/routers/external-api/v2/dashboards.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: 9
  • Production lines changed: 341 (+ 661 in test files, excluded from tier calculation)
  • Critical-path lines changed: 36
  • Branch: drew/name-based-var-state
  • Author: pulpdrew

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

github-actions Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Deep Review

No critical issues found. This is well-scoped Part 1/2 infrastructure: it widens schemas and read paths to accept variable-keyed dashboard filter values without wiring any write path, and it lands with thorough unit and integration coverage. Existing SQL-expression-keyed dashboards parse and behave identically (expressionKeyedEntries retains every non-variable entry before parseQuery), the external-API union stays backward-compatible with existing sql values, and the GET→PUT echo round-trip is covered by an integration test. The two prior P1 review comments are legitimately deferred to PR 2/2: no code in this diff writes type: 'variable' entries, so neither the setFilterValue drop nor the read-path ignore is reachable here.

🔵 P3 nitpicks (2)
  • packages/common-utils/src/dashboardFilterValues.ts:47parseDashboardFilterValues, serializeDashboardFilterValues, resolveFilterSelection, and filterSelectionKey are exported but have no production caller in this diff; they are consumed only by tests until PR 2/2 wires them in.
    • Fix: Confirm the follow-up PR consumes these helpers, or the exported surface remains unreferenced scaffolding.
  • packages/common-utils/src/dashboardFilterValues.ts:73parseDashboardFilterValues routes lucene/sql_ast entries to passthrough (not applied as active selections), whereas the live reader in packages/app/src/hooks/useDashboardFilters.tsx:56 feeds all non-variable entries into parseQuery; the two readers diverge on those types.
    • Fix: When PR 2/2 switches production to parseDashboardFilterValues, verify lucene/sql_ast saved filter values remain applied so the swap does not silently drop them.

Reviewers (8): correctness, adversarial, api-contract, testing, kieran-typescript, maintainability, previous-comments, project-standards.

Testing gaps:

  • The setFilterValue variable-entry merge and the variable read path are unexercised because no write path produces variable-keyed state in this PR; ensure PR 2/2 adds coverage for merging variable entries back through setFilterValue and for resolving them on initial URL load.
  • The divergent lucene/sql_ast passthrough handling in parseDashboardFilterValues vs. the live useDashboardFilters reader has no test asserting equivalent active-filter behavior across the two paths.

@pulpdrew

Copy link
Copy Markdown
Contributor Author

packages/app/src/hooks/useDashboardFilters.tsx:52 -- setFilterValue rebuilds the URL filters= state from expressionKeyedEntries(prev) and returns only filtersToQuery(...), so any type:'variable' entry present in prev is never merged back and is silently dropped whenever an unrelated expression filter changes.

Fixed in the next PR in the stack, I will merge them together. This PR doesn't write variable-keyed state so this isn't hit in practice even if this does merge.

Comment thread packages/app/src/hooks/useDashboardFilters.tsx
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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