Skip to content

feat: Add filter variable configuration - #2836

Open
pulpdrew wants to merge 2 commits into
mainfrom
drew/filter-variable-modal
Open

feat: Add filter variable configuration#2836
pulpdrew wants to merge 2 commits into
mainfrom
drew/filter-variable-modal

Conversation

@pulpdrew

@pulpdrew pulpdrew commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Summary

This PR updates the dashboard filter configuration modal to support enabling "variable" mode for a filter, which will allow users to refer to the filter value as a variable in queries.

This feature is behind the NEXT_PUBLIC_ENABLE_DASHBOARD_VARIABLES feature toggle, since the actual variable replacement is not yet implemented.

Variable mode is off by default for existing filters and on by default for new filters, when the toggle is on. The user may specify a custom name for the variable. A default variable name is generated from the filter's name. Variable names must be unique in the dashboard, while filter names may not be.

Variable mode and broadcast mode (the current behavior) may both be enabled or disabled independently. When the feature is disabled, broadcast mode cannot be disabled and variable mode cannot be enabled - the checkboxes do not appear.

**Note: Variable substitution functionality is not implemented by this PR. ** Only configuration of variable type filters is implemented.

Screenshots or video

With the toggle enabled

Screen.Recording.2026-08-07.at.2.26.31.PM.mov

Updated tooltips and broadcast-disabled functionality

Screen.Recording.2026-08-10.at.12.33.45.PM.mov

Variable mode is not available on Preset Dashboards, and broadcast mode cannot be disabled

This works on temporary/local dashboards as well

Screen.Recording.2026-08-07.at.2.32.12.PM.mov

Import/Export preserves variable mode

Screen.Recording.2026-08-07.at.2.33.56.PM.mov

With the toggle disabled

Screen.Recording.2026-08-07.at.2.40.45.PM.mov

How to test locally

  1. Run locally so that NEXT_PUBLIC_ENABLE_DASHBOARD_VARIABLES is enabled
  2. Create a dashboard and add some filters. Observe the validation for variable name.
  3. Note that the variables cannot yet be referenced, this change covers only the configuration of variable-mode filters.

References

  • Linear Issue: Closes HDX-5043 HDX-5049
  • Related PRs:

@changeset-bot

changeset-bot Bot commented Aug 7, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 33e693f

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

This PR includes changesets to release 4 packages
Name Type
@hyperdx/app Patch
@hyperdx/api Patch
@hyperdx/common-utils 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 7, 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 10, 2026 4:37pm
hyperdx-storybook Ready Ready Preview Aug 10, 2026 4:37pm

Request Review

@greptile-apps

greptile-apps Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR adds feature-gated broadcast and variable configuration for dashboard filters, including UI controls, shared validation, persistence, import/export support, and test coverage.

  • Adds filter mode and variable-name fields to shared dashboard types.
  • Validates mode combinations and variable-name uniqueness on internal and external dashboard API requests.
  • Adds feature-gated configuration controls and variable metadata to the dashboard UI.
  • Extends unit, integration, and end-to-end coverage for filter variables.

Confidence Score: 4/5

The PR is not yet safe to merge because provisioned dashboards can still persist variable-enabled filters without any usable variable token.

The API request paths now reject enabled variables whose names cannot resolve, but the provisioning task still parses the bare dashboard schema and writes its filters directly, leaving the previously reported invalid persisted state reachable.

Files Needing Attention: packages/common-utils/src/types.ts; packages/api/src/tasks/provisionDashboards/index.ts

Important Files Changed

Filename Overview
packages/common-utils/src/types.ts Adds the variable and broadcast fields, but the schema used by dashboard provisioning still permits variable-enabled filters without a usable token.
packages/common-utils/src/dashboardValidation.ts Adds shared dashboard-level validation for variable-name resolution, uniqueness, and the requirement that at least one filter mode be enabled.
packages/api/src/routers/api/dashboards.ts Applies the shared filter validations to internal dashboard creation and partial updates.
packages/api/src/routers/external-api/v2/utils/dashboards.ts Applies the shared filter validations to external dashboard create and update request schemas.
packages/app/src/DashboardFiltersModal.tsx Adds feature-gated broadcast and variable controls, derived variable names, and client-side validation.

Reviews (3): Last reviewed commit: "feat: Improved validation, implement bro..." | Re-trigger Greptile

Comment thread packages/app/src/DashboardFiltersModal.tsx
Comment thread packages/common-utils/src/types.ts
@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

E2E Test Results

All tests passed • 279 passed • 1 skipped • 1120s

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

Tests ran across 4 shards in parallel.

View full report →

Comment thread packages/common-utils/src/types.ts
@pulpdrew
pulpdrew marked this pull request as ready for review August 7, 2026 19:27
@github-actions github-actions Bot added the review/tier-4 Critical — deep review + domain expert sign-off label Aug 7, 2026
@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

🔴 Tier 4 — Critical

Touches authentication, tenancy data models, the public API or shipped database config — or substantially changes 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/utils/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: 14
  • Production lines changed: 696 (+ 2019 in test files, excluded from tier calculation)
  • Critical-path lines changed: 7
  • Branch: drew/filter-variable-modal
  • 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 7, 2026

Copy link
Copy Markdown
Contributor

Deep Review

✅ No critical issues found. The change is well-guarded: the missing-isBroadcastEnabled-means-enabled default preserves existing filter behavior, the new fields are net-new on DashboardFilterSchema (no legacy stored data can trip the new refinements), the external v2 filter schema inherits the fields via DashboardFilterSchema.omit({source}).extend({sourceId}) so API/UI parity holds, and PATCH partial bodies that omit filters short-circuit cleanly through data.filters ?? []. Form, internal-API, and external-API validation mirror each other.

The items below are recommendations and nits only.

🟡 P2 -- recommended

  • packages/common-utils/src/filters.ts:723 -- deriveVariableName applies no length cap, but both validateVariableName and the variableName schema field enforce DASHBOARD_VARIABLE_NAME_MAX_LENGTH (64); a new filter with a long display name auto-fills an over-64-char variable name that then blocks save with an error on a value the user never typed.
    • Fix: Truncate the derived name to DASHBOARD_VARIABLE_NAME_MAX_LENGTH inside deriveVariableName so the auto-generated default is always savable.
🔵 P3 nitpicks (4)
  • packages/common-utils/src/types.ts:1600 -- the variableName field enforces .regex(...) and .max(...) unconditionally, whereas validateDashboardFilterVariableNames only inspects variable-enabled filters, so an API payload with isVariableEnabled: false plus a malformed variableName is rejected even though the value is inert.
    • Fix: Move the token-grammar/length check into the conditional refinement (or clear variableName when the variable is disabled) so validation strictness matches which filters actually use the field.
  • packages/app/src/hooks/useIsVariablesEnabled.ts:10 -- the hook wraps a compile-time config constant in useMemo and hard-codes isLoading: false for a team-level toggle that does not yet exist, adding indirection ahead of need.
    • Fix: Read IS_DASHBOARD_VARIABLES_ENABLED directly at the call sites until an async source actually exists, or drop the loading field.
  • packages/app/src/DashboardFiltersModal.tsx:418 -- the variable checkbox description reads "Expose the selected value a $variable" (missing "as").
    • Fix: Correct the copy to "Expose the selected value as a $variable."
  • packages/app/src/DashboardFilters.tsx:45 -- getFilterEffect builds sentence casing with a parts.length > 0 ? 'a' : 'A' character-concatenation trick that is fragile to reorder or copy changes.
    • Fix: Compose each clause as a complete string and capitalize the first letter of the joined result instead.

Reviewers (10 dimensions): correctness, testing, maintainability, project-standards, api-contract, kieran-typescript, frontend-races, adversarial, security, agent-native, learnings-research.

Testing gaps: Confirm the new suites cover the backward-compat path (a legacy filter with isBroadcastEnabled === undefined passing validateDashboardFilterModes) and the derive-collision case where two variable-enabled filters with distinct display names resolve to the same variableName; both are new behaviors and are the most likely to regress silently.

Note: the automated sub-reviewer fan-out was truncated before individual agents reported, so this report reflects direct full-diff analysis across the dimensions above rather than merged per-agent findings.

* Whether a filter definition broadcasts its selected value onto a tile
* whose source is `sourceId`.
*/
const definitionAppliesToSource = (

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes in this file prevent filters with broadcast mode disabled from being applied to any dashboard tiles.

onRemoveFilter={handleRemoveFilter}
source={source}
isLoading={isFetchingFilters || isFiltersMutationPending}
showVariableOptions={false}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The user doesn't define tiles on this dashboard, so there is no reason for variable-type filters. The UI will not show the option to enable variable mode or disable broadcast mode.

@pulpdrew
pulpdrew requested review from a team, knudtty and wrn14897 and removed request for a team and knudtty August 10, 2026 16:52
* field broadcasts, and that must not change. Read it through
* `isFilterBroadcastEnabled` rather than defaulting at each call site.
*/
isBroadcastEnabled: z.boolean().optional(),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we want to make these changes on the model side as well?

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.

2 participants