Skip to content

feat(dashboards): support constant values and render modes for filters#2383

Open
alex-fedotyev wants to merge 9 commits into
mainfrom
alex/HDX-4404-dashboard-filter-modes
Open

feat(dashboards): support constant values and render modes for filters#2383
alex-fedotyev wants to merge 9 commits into
mainfrom
alex/HDX-4404-dashboard-filter-modes

Conversation

@alex-fedotyev
Copy link
Copy Markdown
Contributor

@alex-fedotyev alex-fedotyev commented May 30, 2026

Linear: HDX-4404.

Summary

  • Adds two fields to DashboardFilterSchema: constant: boolean (locks the saved default; the viewer cannot clear it) and renderMode: 'editable' | 'readonly' | 'hidden' (controls how the chip appears in the filter bar).
  • One dashboard can be cloned and re-pointed by saving a different default value per copy, instead of hand-coding the scope into every tile's WHERE.
  • Filter editor UI presents a single "Visibility" select with three presets (Editable / Read-only / Hidden) that map to the orthogonal (constant, renderMode) pair. The schema still admits all combinations so MCP and external API callers can express future variants.
  • When Visibility is Read-only or Hidden, the editor exposes a "Default value" TagsInput so the author can set the locked value directly. Editable filters keep using the existing chip + Save default flow unchanged.
  • External API FilterInput / Filter OpenAPI schemas and the MCP mcpDashboardFilterSchema carry both new fields; openapi.json regenerated.
  • Hook (useDashboardFilters) accepts savedFilterValues and overlays constant filter values on every read, regardless of URL state; setFilterValue is a no-op for constant expressions. Constant values are kept out of the URL on hydration so the lock doesn't leak into shared links, and handleSaveQuery preserves constant entries when saving so the editable Save default flow doesn't clobber them.
  • Out of scope (tracked as follow-ups in the Linear issue): URL-parameterized constants (?scope=...), inline operator/values on the filter definition, customer-facing docs in clickhouse-docs.
image

Test plan

  • Hook unit tests: constant value injection, setFilterValue no-op, sibling editable filters still work, getFilterQueriesForSource honors appliesToSourceIds for constant filters, hidden + constant resolves the saved value, missing saved value returns nothing.
  • Zod schema tests: accepts constant + renderMode in every valid combination, rejects unknown renderMode, rejects null for either field.
  • External API round-trip integration test: POST + GET + PUT with constant and renderMode set; absent fields stay absent on read; unknown renderMode returns 400.
  • MCP hyperdx_save_dashboard round-trip integration test: same coverage as the external API test.
  • UI smoke (Playwright + manual screenshots, light + dark):
    • Edit a filter, switch Visibility to Read-only, type a value into Default value, save. Verify the chip shows disabled with a lock icon and tiles include the locked value in WHERE.
    • Switch Visibility to Hidden. Verify the chip is removed from the bar; tiles still include the locked value.
    • Clear the Default value (TagsInput empty), save. Verify tiles are no longer scoped on that expression.
    • Hard-reload with an empty URL. Verify the locked value still applies (sourced from savedFilterValues).
    • Open a shared link with a stale value for the locked expression. Verify the locked value still wins.
  • Predicted tier: review/tier-4 (touches packages/api/src/routers/external-api/v2/dashboards.ts, expected per the implementation plan).

Tagging as draft so the UI smoke can be done against this branch's build.

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented May 30, 2026

🦋 Changeset detected

Latest commit: 321a0e9

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 Minor
@hyperdx/app Minor
@hyperdx/otel-collector Minor

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 30, 2026

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

Project Deployment Actions Updated (UTC)
hyperdx-oss Ready Ready Preview, Comment Jun 2, 2026 1:36am
hyperdx-storybook Ready Ready Preview, Comment Jun 2, 2026 1:36am

Request Review

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 30, 2026

E2E Test Results

All tests passed • 192 passed • 3 skipped • 1274s

Status Count
✅ Passed 192
❌ Failed 0
⚠️ Flaky 3
⏭️ Skipped 3

Tests ran across 4 shards in parallel.

View full report →

@alex-fedotyev alex-fedotyev force-pushed the alex/HDX-4404-dashboard-filter-modes branch from 1572113 to 6946b0a Compare May 30, 2026 00:54
@alex-fedotyev alex-fedotyev marked this pull request as ready for review May 30, 2026 01:01
@github-actions github-actions Bot added the review/tier-4 Critical — deep review + domain expert sign-off label May 30, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 30, 2026

🔴 Tier 4 — Critical

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

Why this tier:

  • Critical-path files (2):
    • packages/api/src/routers/external-api/v2/dashboards.ts
    • 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: 17
  • Production lines changed: 1595 (+ 1655 in test files, excluded from tier calculation)
  • Branch: alex/HDX-4404-dashboard-filter-modes
  • 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

<!-- deep-review -->

Deep Review

🔴 P0/P1 -- must fix

  • packages/api/src/mcp/tools/dashboards/saveDashboard.ts:53 -- hyperdx_save_dashboard's MCP inputSchema exposes id/name/tiles/tags/containers/filters but not savedFilterValues, while mcpDashboardFilterSchema tells the agent that constant: true takes its locked value from dashboard.savedFilterValues; the safeParse payloads at :161 and :354 therefore never receive savedFilterValues, so a constant filter created via MCP locks to nothing and scopes no tile.
    • Fix: Add savedFilterValues: z.array(z.object({ type: z.enum(['lucene','sql']), condition: z.string() })).optional() to the tool inputSchema, thread it through to the create/update body schemas, and add an example pairing constant: true with a matching entry to mcpFiltersParam.
    • agent-native

🟡 P2 -- recommended

  • packages/app/src/DBDashboardPage.tsx:1445 -- savedFilterValues is keyed by filter expression, so when two filter definitions share an expression (one constant, one editable) getDefaultValuesForExpression seeds the editable form with the constant's locked value; if the author clears the picker and saves the editable, handleSaveFilter deletes the entry for that expression and silently breaks the sibling constant's lock.

    • Fix: Key savedFilterValues by filter id (and update the hook overlay + URL strip + save preservation to match), or reject sibling definitions where one is constant and one is editable on the same expression.
    • adversarial, correctness
  • packages/app/src/DashboardFiltersModal.tsx:304 -- DashboardFilterEditForm recomputes initialDefaultValues whenever the prop savedFilterValues changes and its useEffect([filter, initialDefaultValues, reset]) calls reset(...) on the change, so a background refetch that lands while the modal is open wipes any in-progress edit to the defaultValues field.

    • Fix: Snapshot initialDefaultValues once via a ref (or gate the reset on !formState.dirtyFields.defaultValues) so subsequent savedFilterValues arrivals don't clobber the author's typed values.
    • reliability, julik-frontend-races
  • packages/app/src/hooks/useDashboardFilters.tsx:102 -- inside the iteration, valuesForExistingFilters[expression] = match is keyed by raw expression, so two filter definitions sharing one expression with mixed constant/editable last-writer-wins: the editable's URL value overwrites the constant's locked value, breaking the lock guarantee for the in-scope tile.

    • Fix: Resolve values per-definition (e.g. key by filter.id and merge in getFilterQueriesForSource), or refuse to overwrite a key already populated by a constant entry.
    • adversarial, correctness
  • packages/api/openapi.json:2366 -- the new property schemas declare "default": false / "default": "editable" (mirrored in the swagger JSDoc at packages/api/src/routers/external-api/v2/dashboards.ts:1382), but the Zod schema is .optional() with no .default() and the new API test explicitly asserts the omitted fields return undefined; generated SDKs will surface a default the wire format never produces.

    • Fix: Drop the default keys from both property schemas in openapi.json and the JSDoc; describe the implied default in the description text instead.
    • api-contract
  • packages/app/src/DBDashboardPage.tsx:1669 -- the new constant-preservation merge in handleSaveQuery, the URL-strip block at :1581, and the savedFilterValues upsert in handleSaveFilter (:1445) are all non-trivial and entirely uncovered; future edits to any of the three inline implementations can silently regress the "Save default doesn't clobber constants" contract.

    • Fix: Extract each merge into a pure helper (mergeConstantFiltersForSave, stripConstantsFromUrl, upsertSavedDefault) and unit-test the matrix: constants present + URL collisions, no constants, constants without saved entries, bracket-notation expressions.
    • testing, maintainability
  • packages/app/src/hooks/__tests__/useDashboardFilters.test.tsx:355 -- every new constant-filter test uses simple dot-notation expressions (environment, service.name); bracket-notation (SpanAttributes['k8s.pod.name']) flows through normalizeKey in the hook overlay, the URL strip, save preservation, and getDefaultValuesForExpression in the modal but has zero coverage.

    • Fix: Add a constant: true test with a bracket-notation expression and assert the locked value resolves through filterValues, blocks setFilterValue, and survives handleSaveQuery.
    • testing, correctness
🔵 P3 nitpicks (12)
  • packages/app/src/DBDashboardPage.tsx:1583 -- constantExpressions is rebuilt inline in the URL-hydration effect, again in handleSaveQuery, and a third time as a memo inside useDashboardFilters, each repeating parseKeyPath(...).join('.').

    • Fix: Promote normalizeExpression(k) and buildConstantExpressionSet(filters) into a shared module and call from all three sites.
    • maintainability, correctness
  • packages/common-utils/src/types.ts:1162 -- the new DashboardFilterRenderMode Zod enum is exported but every consumer in the app package uses the literal strings 'editable' / 'readonly' / 'hidden', including a hand-rolled FilterVisibility union and a getFilterVisibility parameter type.

    • Fix: Import the enum (or z.infer<> it into a type) and replace the magic strings so a future rename breaks the type-checker.
    • maintainability, kieran-typescript
  • packages/app/src/DashboardFiltersModal.tsx:87 -- applyFilterVisibility('editable') returns { constant: undefined, renderMode: undefined }; spreading that into the saved filter leaves the keys present (with undefined values) in memory, so structural equality against a JSON-roundtripped server copy sees a spurious difference even though the wire format is identical.

    • Fix: Return {} for the editable case so the keys are genuinely absent in memory.
    • kieran-typescript
  • packages/app/src/DashboardFiltersModal.tsx:412 -- the defaultsChanged comparison runs sanitizedDefaults.some((v, i) => v !== initialNormalized[i]), so reordering the same set fires a spurious save write.

    • Fix: Replace the positional compare with a set-equality check.
  • packages/app/src/DashboardFiltersModal.tsx:195 -- NEVER_USED_RANGE is wrapped in useMemo with an empty deps array inside the component.

    • Fix: Hoist it to module scope as a plain const.
  • packages/app/src/DashboardFiltersModal.tsx:188 -- the as DashboardFilter cast on filtersForQuery is redundant; the Pick<> type is already structurally assignable because the omitted fields are optional, so the cast just hides whether the assignment is sound.

    • Fix: Drop the cast (or widen the prop type) so the type relationship is explicit.
  • packages/app/src/hooks/usePresetDashboardFilters.tsx -- the preset handleSaveFilter keeps its single-argument signature while the modal's onSaveFilter prop now accepts an optional { defaultValues } second arg; safe today because supportsConstantFilters=false on presets, but it silently drops the arg if that flag is ever flipped.

    • Fix: Widen the signature to (filter, _options?: { defaultValues?: string[] }) => void.
  • packages/app/src/DBDashboardPage.tsx:170 -- the new parseQuery import uses a relative ./searchFilters path while every other call site in the package uses the @/searchFilters alias.

    • Fix: Switch to the alias for consistency.
  • packages/app/src/DBDashboardPage.tsx:1622 -- dashboard?.filters was added to the hydration effect's deps but no body code closes over it; the initializedDashboard.current === dashboard.id guard already protects against re-runs, so the extra dep only widens the re-execution surface.

    • Fix: Drop dashboard?.filters from the dep array and add a one-line eslint-disable explaining why.
    • reliability, julik-frontend-races
  • packages/app/src/DBDashboardPage.tsx:1470 -- handleRemoveFilter deletes a filter from dashboard.filters but leaves any savedFilterValues entry keyed by that filter's expression; recreating a filter on the same expression later silently re-locks to the orphaned value.

    • Fix: Strip the matching savedFilterValues entry alongside the filter delete.
    • correctness
  • packages/common-utils/src/types.ts:1183 -- the Zod schema admits renderMode: 'hidden' (or 'readonly') with constant: false; that combination produces a chip-less filter that never applies (no constant overlay) and is reachable from external API and MCP callers.

    • Fix: Either add a Zod refine rejecting non-editable renderMode without constant: true, or document the combo as a no-op in the field description.
    • correctness
  • packages/api/src/routers/external-api/__tests__/dashboards.test.ts:961 -- the HDX-4404 round-trip test uses toMatchObject on the filter payload, which lets unexpected extra fields slip through silently.

    • Fix: Use toEqual on the filter objects (or assert the full expected key set) for both POST and PUT responses.
    • testing

Reviewers (11): correctness, testing, maintainability, project-standards, security, learnings-researcher, api-contract, reliability, adversarial, kieran-typescript, julik-frontend-races, agent-native.

Testing gaps:

  • Page-level handleSaveQuery, handleSaveFilter, and URL-strip-on-hydration paths in DBDashboardPage.tsx have no direct coverage.
  • Bracket-notation expressions are unverified across the hook overlay, URL strip, and save preservation paths.
  • No component test exercises DashboardFiltersModal's Visibility select or the default-value picker (the filter-visibility-select / filter-default-values-input testids are unused).
  • No test asserts that DashboardFilters actually omits chips for renderMode: 'hidden' from the DOM.
  • No test covers two filter definitions sharing an expression with mixed constant/editable flags.
  • No Playwright/E2E coverage for the locked-filter authoring flow.

alex-fedotyev added a commit that referenced this pull request May 30, 2026
E2E shard 2 was failing on three pre-existing dashboard tests because
`visibleFilters` in DashboardFilters.tsx was a new array reference every
render, churning useDashboardFilterValues' useQueries and leaving the
chip dropdown disabled while the values query never settled. Memoize
the array so a stable reference flows into the hook.

Deep review fixes:

- P0/P1: MCP hyperdx_save_dashboard inputSchema now exposes
  savedFilterValues, paired with a usage example in the filters param
  description so constant: true filters can actually lock to a value
  via MCP. The body schema already accepts it; only the tool input
  shape was missing.

- P2: DashboardFiltersModal snapshots initialDefaultValues via state
  keyed on filter.id, so a background savedFilterValues refetch no
  longer wipes in-progress edits in the default-value picker.

- P2: Schema-level coherence checks moved to common-utils via two
  reusable refinements applied at the external API boundary:
  refineDashboardFilterCoherence rejects readonly/hidden render modes
  without constant: true; refineDashboardFiltersConstantSiblings
  rejects two filter definitions sharing an expression that disagree
  on constant. Internal callers (MCP, future migrations) get the
  same protection through the shared schemas.

- P2: openapi.json + JSDoc no longer declare default: false /
  default: "editable" on the new constant / renderMode property
  schemas. The Zod schema is .optional() with no .default() so
  generated SDKs would otherwise surface a default the wire format
  never produces; the implied default now lives in the description
  text instead.

- P2: page-level merge logic for "Save default" / URL hydration /
  savedFilterValues upsert is extracted into dashboardFilterUtils
  (normalizeExpression, buildConstantExpressionSet,
  stripConstantsFromUrl, mergeConstantFiltersForSave,
  upsertSavedDefault, removeSavedDefaultForExpression). The matrix
  including bracket-notation expressions is covered by a new unit
  test. useDashboardFilters and DBDashboardPage call the helpers.

- P2: bracket-notation constant filter is now covered in the
  hook unit tests (SpanAttributes['k8s.pod.name'] resolves through
  filterValues, blocks setFilterValue, and survives via
  getFilterQueriesForSource).

P3 polish:

- handleRemoveFilter now strips the matching savedFilterValues entry
  alongside the filter delete so deleting + recreating a filter on the
  same expression doesn't silently re-lock to an orphaned value.
- applyFilterVisibility('editable') returns {} so the spread on save
  no longer leaves constant: undefined / renderMode: undefined keys
  in memory.
- defaultsChanged uses set equality so reordering the same values
  doesn't fire a spurious save write.
- NEVER_USED_RANGE hoisted to module scope.
- FilterVisibility uses z.infer<typeof DashboardFilterRenderMode>
  instead of a hand-rolled string union.
- as DashboardFilter cast dropped on filtersForQuery.
- usePresetDashboardFilters.handleSaveFilter widened to accept the
  optional defaultValues arg so a future supportsConstantFilters=true
  flip on presets doesn't silently drop the second arg.
- parseQuery import uses the @/searchFilters alias for consistency
  with sibling files.
- dashboard?.filters dropped from the URL hydration effect's deps
  (the initializedDashboard.current === dashboard.id guard already
  prevents re-runs).
- HDX-4404 API round-trip test uses toEqual instead of toMatchObject
  so unexpected extra fields fail the assertion.

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

Pushed 21880e4 with the deep-review fix-pack plus the e2e shard-2 root cause. Summary:

E2E shard 2 (3 failing tests)

Three pre-existing dashboard tests (should create and populate filters, should scope a filter to a specific source via "Applies to sources", should save and restore query and filter values) failed because visibleFilters in DashboardFilters.tsx was a new array reference every render, so useDashboardFilterValues's useQueries churned and the chip dropdown stayed disabled while the values query never settled. Memoizing the array fixes the regression. The Playwright error-context.md snapshot confirmed the failure mode (both filter chips rendered [disabled] even though neither had constant / renderMode set, and the underlying table tile had loaded its data).

Deep review

  • P0/P1 saveDashboard.ts:53 MCP inputSchema missing savedFilterValues. Fixed in 21880e4 — threaded savedFilterValues through the tool input, the create/update helpers, and added an example pairing constant: true with a savedFilterValues entry in mcpFiltersParam.
  • P2 DashboardFiltersModal.tsx:304 reset-wipes-edits race. Fixed in 21880e4 — snapshot initialDefaultValues via state keyed on filter.id; a background savedFilterValues refetch no longer fires the reset.
  • P2 useDashboardFilters.tsx:102 + DBDashboardPage.tsx:1445 sibling-expression collision. Fixed in 21880e4 — added a Zod refinement at the boundary (refineDashboardFiltersConstantSiblings) that rejects two filter definitions sharing a normalized expression where one has constant: true and another doesn't. The id-keyed wire-format refactor (the other option you suggested) is intentionally out of scope; I'll file a follow-up for that since it's a wire-format change.
  • P2 openapi.json:2366 defaults declaration. Fixed in 21880e4 — dropped "default": false / "default": "editable" from both property schemas and the JSDoc; implied default text moved into the description.
  • P2 DBDashboardPage.tsx:1669 page-level merge helpers uncovered. Fixed in 21880e4 — extracted normalizeExpression, buildConstantExpressionSet, stripConstantsFromUrl, mergeConstantFiltersForSave, upsertSavedDefault, removeSavedDefaultForExpression into dashboardFilterUtils.ts, with the matrix (constants + URL collisions, no constants, constants without saved entries, bracket-notation) covered by a new unit test.
  • P2 useDashboardFilters.test.tsx:355 bracket-notation untested. Fixed in 21880e4 — added a constant: true test with SpanAttributes['k8s.pod.name'] covering hook overlay, setFilterValue no-op, and getFilterQueriesForSource.

P3 nitpicks all addressed in the same commit:

  • DBDashboardPage.tsx:1583 constantExpressions rebuilt three times — now one helper.
  • types.ts:1162 enum unused — FilterVisibility now z.infer<typeof DashboardFilterRenderMode>.
  • DashboardFiltersModal.tsx:87 applyFilterVisibility('editable') returns {} instead of { constant: undefined, renderMode: undefined }.
  • DashboardFiltersModal.tsx:412 set equality instead of positional compare.
  • DashboardFiltersModal.tsx:195 NEVER_USED_RANGE hoisted to module scope.
  • DashboardFiltersModal.tsx:188 as DashboardFilter cast dropped.
  • usePresetDashboardFilters.tsx widened to accept _options?: { defaultValues?: string[] }.
  • DBDashboardPage.tsx:170 parseQuery import uses the @/searchFilters alias.
  • DBDashboardPage.tsx:1622 dashboard?.filters dropped from deps (with an eslint-disable explaining why).
  • DBDashboardPage.tsx:1470 handleRemoveFilter strips the matching savedFilterValues entry.
  • types.ts:1183 renderMode: 'readonly' | 'hidden' without constant: true rejected by refineDashboardFilterCoherence.
  • dashboards.test.ts:961 toMatchObject swapped to toEqual so unexpected fields fail the assertion.

make ci-lint + 45 touched unit tests pass locally. Waiting on CI for the e2e shard-2 retry.

alex-fedotyev added a commit that referenced this pull request May 30, 2026
Two fixes for #2383 CI:

1. DashboardFiltersModal default-value picker no longer issues a query
   when the resolved table name is empty. A metric source with no
   metricType picked yet resolves `getMetricTableName` to
   `source.from.tableName`, which is typically empty on a metric source.
   Firing the autocomplete in that state produces a malformed
   `DESCRIBE {db}.{}` (empty Identifier substitution) that the
   ClickHouse proxy rejects with a 500. The e2e shard 2 trace shows
   exactly this 500 firing during filter editing, which left the
   dashboard filter chip dropdowns disabled long enough to time out the
   click-the-option waits. Gating on `tableName` keeps the picker idle
   until the form is configured enough to name a real table.

2. The external API HDX-4404 round-trip test asserted
   `whereLanguage: 'sql'` on the response for filters whose input payload
   omits `whereLanguage`. The Zod schema is `.optional()` with no
   default, so the wire format is undefined-in / undefined-out. The
   previous expectation made the test fail by asserting a default the
   schema never emits. Dropped the field from those three filter
   expects and from the PUT-flip filter expect; added a comment so the
   omission is intentional.
@alex-fedotyev
Copy link
Copy Markdown
Contributor Author

Pushed 73d370d with two CI fixes after digging into the shard 2 trace:

Integration test: dashboards.test.ts:965 was asserting whereLanguage: 'sql' on filters whose input payload omits whereLanguage. The Zod schema is .optional() with no default, so the wire format is undefined-in / undefined-out. Dropped the field from three filter expects in the round-trip test plus the PUT-flip expect; added a comment so the omission is deliberate.

E2E shard 2: trace.network shows a 500 on clickhouse-proxy at the moment of filter editing:

Code: 457. DB::Exception: Empty Identifier part after parameter `HYPERDX_PARAM_0` substitution.

That HYPERDX_PARAM_0 is the hash of an empty string. The SQL body is DESCRIBE {db}.{} with an empty Identifier substitution. The FilterDefaultValueSelect I added inside the modal fires useDashboardFilterValues while editing, and for a metric source with no metricType picked yet, getMetricTableName(source, undefined) resolves to source.from.tableName, which is empty on metric sources. That's what put the request together and got a 500. The 500 left the chip dropdowns disabled long enough to time out the option clicks.

Gated filterForValueQuery on tableName resolving non-empty so the picker stays idle until the form is configured enough to name a real table.

Local make ci-lint passes, make ci-unit passes (1782 / 1782; one ENOMEM in TraceRedirectPage.test.tsx that's environment, unrelated). Waiting on CI shard 2.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 30, 2026

Deep Review

🔴 P0/P1 -- must fix

  • packages/app/src/searchFilters.tsx:263 -- A constant: true filter paired with a SQL savedFilterValues entry whose condition is not pure IN/NOT IN/BETWEEN (e.g. ServiceName = 'prod') is silently dropped by parseQuery's containsOperatorOutsideQuotes continue branch and is not pushed to passthroughFilters either, so normalizedSaved.get(norm) returns undefined, the overlay resolves to no value, and the dashboard renders the lock chip while tile WHERE clauses run unscoped; the external API accepts this shape because hasAnySqlEntry at v2/utils/dashboards.ts:1188 short-circuits coverage, and any subsequent handleSaveQuery or handleRemoveSavedQuery permanently strips the unparsed SQL value from Mongo via mergeConstantFiltersForSave.
    • Fix: Either reject non-IN SQL savedFilterValues for constant: true filters at the body-schema validator, or preserve unparsed SQL entries through parseQuery into passthroughFilters so the overlay can re-emit them verbatim.
    • adversarial, correctness

🟡 P2 -- recommended

  • packages/api/src/routers/api/dashboards.ts:36 -- The internal POST/PATCH wrapper applies refineDashboardFilterCoherence and refineDashboardFiltersConstantSiblings but omits the constant-has-matching-savedFilterValues rule that the external API enforces at v2/utils/dashboards.ts:1186, so a UI save (or MCP write) with constant: true and no covering saved value persists a locked-but-unscoped filter that later fails to round-trip through the external PUT endpoint.

    • Fix: Hoist the coverage check into common-utils/types.ts next to the other shared refinements and apply it from all three boundaries (internal, MCP, external).
    • api-contract, maintainability, adversarial
  • packages/api/src/routers/external-api/v2/utils/dashboards.ts:1188 -- hasAnySqlEntry = saved.some(s => s.type === 'sql') waives per-constant coverage globally whenever any SQL entry exists, so a payload with filters: [{ constant: true, expression: 'region' }] plus an unrelated savedFilterValues: [{ type: 'sql', condition: "ServiceName IN ('prod')" }] passes validation even though no entry covers the region constant.

    • Fix: Narrow the SQL escape hatch to entries whose condition references the constant's expression (string match on parseKeyPath(expression).join('.')), or drop the global short-circuit and require Lucene entries for coverage.
    • adversarial
  • packages/app/src/hooks/useDashboardFilters.tsx:250 -- hasScrubbedConstantsRef latches true on the first non-empty filterQueries snapshot regardless of whether anything was removed, and the hook's underlying DBDashboardPage instance survives in-SPA dashboard navigations, so navigating from dashboard A to dashboard B with different constant expressions leaves B's stale constant entries in the URL indefinitely (reads stay correct via the overlay but copy/paste of the address bar propagates the leaked tokens).

    • Fix: Reset the ref when the constant-expression set identity changes (or when dashboard?.id changes), so each dashboard gets one scrub pass.
    • correctness, julik-frontend-races, adversarial
  • packages/app/src/DBDashboardPage.tsx:1391 -- dashboardReady = !!dashboard?.id && router.isReady && (isLocalDashboard || !isFetchingDashboard) gates the hook's enabled, and any save mutation flips isFetchingDashboard true on every viewer who has the dashboard open; while that refetch is in flight the hook returns empty filter values, tile render branches around line 1938 are not gated, and tiles refire ClickHouse queries with no constant scope until the refetch resolves.

    • Fix: Gate tile rendering on dashboardReady (reuse the existing skeleton) or have the hook return undefined when disabled and treat undefined as "do not query yet" in Tile.
    • adversarial, julik-frontend-races
  • packages/app/src/dashboardFilterUtils.ts:79 -- When constants are present mergeConstantFiltersForSave concatenates savedPassthrough and urlPassthrough unconditionally, so SQL-type passthrough entries that exist in both the persisted saved values and the URL state appear twice in the saved array (the early-return at line 84 only fires when constantExpressions is empty).

    • Fix: Dedupe passthroughs by serialized condition, or pick a single source (URL state wins for non-constant content) when constants exist.
    • correctness
  • packages/app/src/DashboardFiltersModal.tsx:459 -- The submit handler always destructures constant and renderMode out of the form values, then merges applyFilterVisibility(visibility) (which returns {} when supportsConstantFilters is false); the comment at line 466 claims existing constant/renderMode survive on the preset path, but the destructure has already stripped them, so any preset filter that arrives with those fields (direct DB write, future migration, MCP) loses them on the first editor save.

    • Fix: Skip the destructure when !supportsConstantFilters and let the original keys flow through ...rest, or fix the comment to acknowledge the strip is intentional.
    • kieran-typescript
  • packages/app/src/hooks/useDashboardFilters.tsx:153 -- The constant lock is enforced only in the React client (overlay + setFilterValue no-op); the server-side tile-query path in packages/common-utils/src/core/renderChartConfig.ts composes WHERE from the caller-supplied chartConfig.filters and has no concept of "constant", so any viewer who calls the chart-query endpoint directly (devtools, custom client) can omit the constant filter from the payload and receive unscoped data — the chip presents a lock the API does not enforce.

    • Fix: Either re-apply matching savedFilterValues for constant: true filters server-side in the tile-query handler, or document explicitly that "constant" is a UX lock and not an authorization boundary (the PR's "viewer cannot clear it" framing implies the latter).
    • security
  • packages/common-utils/src/types.ts:645 -- The internal DashboardSchema.savedFilterValues accepts the full FilterSchema union including type: 'sql_ast', while the external/MCP body schemas accept only { type: 'sql'|'lucene' }; an internally-saved sql_ast entry will be returned verbatim by the v2 GET and is not declared in the openapi SavedFilterValue enum, breaking strict-enum generated clients.

    • Fix: Tighten the internal schema to match the external surface, or translate/drop sql_ast in convertToExternalDashboard, or add sql_ast to the openapi enum.
    • api-contract
  • packages/api/src/routers/external-api/v2/utils/dashboards.ts:1166 -- Four as unknown as DashboardFilter[] casts in buildDashboardBodySchema (plus matching casts in mcp/tools/dashboards/schemas.ts and routers/api/dashboards.ts) compile only because both shared refinements happen to read .expression and .constant; tightening the helper signatures to Pick<DashboardFilter, 'expression' | 'constant' | 'renderMode'> would let callers pass data through without the double-cast and catch a future helper that starts reading .source.

    • Fix: Export narrow candidate types from common-utils/types.ts and retype the refinement helpers against them.
    • project-standards, kieran-typescript
  • packages/app/src/hooks/__tests__/useDashboardFilters.test.tsx:251 -- The on-load URL-scrub useEffect at useDashboardFilters.tsx:251 has no dedicated test — the existing test covers the in-flight setFilterValue scrubber but not the one-shot mount-time scrub, and the filterQueries === null / constantExpressions.size === 0 / removed === false branches are uncovered.

    • Fix: Add a test that renders the hook with a non-empty initial filterQueries containing a constant entry and asserts mockSetState is called with the cleaned URL.
    • testing
  • packages/app/src/DBDashboardPage.tsx:1696 -- handleSaveQuery and handleRemoveSavedQuery (around lines 1736-1761) call mergeConstantFiltersForSave to preserve constant entries through Save Default and Remove Default flows, but no integration test pins this; a future refactor that swaps the helper for a direct array push would silently re-introduce the clobbering bug the PR exists to prevent.

    • Fix: Add tests that save and remove defaults on a dashboard containing constant: true filters and assert the matching savedFilterValues entries survive.
    • testing
  • packages/app/src/DBDashboardPage.tsx:1438 -- The new handleSaveFilter rename-cleanup and handleRemoveFilter sibling-preserve branches are uncovered: the cases expression renamed with no sibling, expression renamed with sibling still using the old key, and the normalization-equality short-circuit have no tests, and a regression that always-prunes would silently break the multi-source locked-twin pattern the inline comments describe.

    • Fix: Add tests for each branch, including the multi-source pattern where two filters share the same normalized expression with different appliesToSourceIds.
    • testing
  • packages/app/src/DashboardFilters.tsx:31 -- isLocked = renderMode === 'readonly' || !!constant propagates disabled to VirtualMultiSelect and renders the lock icon, and visibleFilters = filters.filter(f => f.renderMode !== 'hidden') removes hidden chips from the bar, but no component-level test covers either behavior — a regression flipping disabled || isLocked to disabled && isLocked would let viewers edit locked filters without a test catching it.

    • Fix: Add component tests asserting (a) chip is disabled and shows the lock icon for constant: true and renderMode: 'readonly', (b) renderMode: 'hidden' chips do not render, (c) the lock icon does not render for constant: false, renderMode: 'editable'.
    • testing
  • packages/api/src/routers/external-api/__tests__/dashboards.test.ts:920 -- Roundtrip tests cover absent and constant: true, but explicit constant: false (documented as a legal noop in openapi.json) and explicit null for either field at the HTTP boundary are not exercised, so a future strict-coercion change could break the documented contract without a failing test.

    • Fix: Add cases for explicit constant: false, constant: null (expected 400), and renderMode: null (expected 400) at the POST/PUT/GET surface.
    • testing, api-contract
🔵 P3 nitpicks (16)
  • packages/app/src/DBDashboardPage.tsx:1750 -- handleRemoveSavedQuery calls mergeConstantFiltersForSave(..., [], constantExpressions) and that helper always re-emits saved SQL-type passthrough entries, so "Remove default query and filters" silently preserves saved SQL passthroughs even when they are not covering any constant.
    • Fix: Drop saved passthroughs from the preserved set in mergeConstantFiltersForSave when the URL contribution is empty, or include passthroughs only when they actually cover a constant expression.
  • packages/app/src/DashboardFiltersModal.tsx:83 -- A filter shaped { constant: true, renderMode: 'editable' } (schema-legal, MCP-reachable) renders in the editor as the readonly preset because getFilterVisibility collapses any truthy constant to readonly; saving without touching Visibility writes back renderMode: 'readonly', silently mutating the persisted shape.
    • Fix: Either tighten the schema to forbid constant: true with renderMode: 'editable', or preserve the original renderMode when only constant was the trigger for the readonly preset.
  • packages/app/src/DashboardFiltersModal.tsx:219 -- The autocomplete useDashboardFilterValues query fires for renderMode: 'hidden' filters whenever the modal is opened to edit a hidden filter's default value, including the long disableRowLimit: true path; for sources with heavy WHERE clauses this slows editor open on filters that will never render a chip at runtime.
    • Fix: Confirm slow autocomplete is acceptable in hidden-mode authoring or gate the picker on visibility !== 'hidden' and surface a free-text input instead.
  • packages/app/src/hooks/useDashboardFilters.tsx:183 -- filtersByExpression keys by raw expression, so two filters for the same logical field using different notations (Foo['bar'] vs Foo.bar) bucket separately and getFilterQueriesForSource emits redundant WHERE entries; constants exacerbate the issue when only one of the two is constant.
    • Fix: Key filtersByExpression by normalizeExpression(f.expression) with a per-key list of raw expressions, or document mixed notation as unsupported.
  • packages/app/src/DBDashboardPage.tsx:1490 -- handleSaveFilter calls upsertSavedDefault(savedFilterValues, filter.expression, sanitizedDefaults) without guarding against filter.expression === '', so a malformed submit could create a saved entry keyed by the empty string.
    • Fix: Add an early return when filter.expression is empty before calling upsertSavedDefault.
  • packages/common-utils/src/types.ts:1199 -- Schema admits { constant: true, renderMode: 'editable' } with the stated rationale of leaving room for MCP/external API callers, but no consumer distinguishes that combination from { constant: true, renderMode: 'readonly' } anywhere in the diff — it is dead schema surface.
    • Fix: Either tighten the schema to the three supported shapes or land a consumer that distinguishes the fourth combo at the chip or overlay level.
  • packages/app/src/hooks/useDashboardFilters.tsx:88 -- Two scrubbing paths drop constant URL entries: the useEffect keyed on hasScrubbedConstantsRef and the per-write loop inside setFilterValue; once the mount scrub has run, the per-write loop guards against a path that cannot reintroduce constants from parseQuery(prev).
    • Fix: Pick one scrubber and document the invariant — halving the scrub points removes the drift risk if the two implementations diverge.
  • packages/app/src/hooks/usePresetDashboardFilters.tsx:36 -- handleSaveFilter accepts and ignores a _options arg with a long comment, paired with a supportsConstantFilters?: boolean flag on the modal, to keep contract parity with a hypothetical future caller that enables constants on presets.
    • Fix: Wait until presets need constants before keeping the ignored arg and flag; today they add branching for no current behavior delta.
  • packages/app/src/hooks/useDashboardFilterValues.tsx -- The filterIds-out-of-queryFn cache-pollution fix and the constant-filter feature ship in the same PR, making future bisection of either change land on this commit.
    • Fix: Extract the cache-pollution fix into its own commit with a regression test (editor opening with a new filter ID and not blanking the chip dropdown).
  • packages/app/src/dashboardFilterUtils.ts and packages/app/src/hooks/useDashboardFilters.tsx -- Block comments recap PR context and reference ticket IDs (HDX-4404) and test file names; comment-to-code ratio approaches 1:1 in the hook.
    • Fix: Trim to the load-bearing invariant ("constants are sourced from savedFilterValues on every read and not written to URL") and drop change-history narration.
  • packages/app/src/DashboardFiltersModal.tsx and packages/app/src/DBDashboardPage.tsx -- Both files were past the project's 300-line guideline pre-PR (551 and 2799 lines) and this PR grows them further (+460 and +161).
    • Fix: Extract the modal's Visibility section into a sibling component and the page's constant-filter merge handlers into a hook; the helper extraction to dashboardFilterUtils.ts shows the pattern.
  • packages/api/openapi.json:622 -- SavedFilterValue.type widens from ["sql"] to ["sql","lucene"]; strict-enum generated clients pinned to the prior spec will fail to deserialize type: "lucene" responses.
    • Fix: Call the widening out in release notes so consumers regenerate clients before upgrading.
  • packages/api/src/utils/zod.ts:160 -- externalDashboardSavedFilterValueSchema applies .default('sql') for type; legacy saved values stored without type will round-trip as type: 'sql' regardless of their actual condition shape, which is wrong for any pre-existing Lucene-shaped data.
    • Fix: Audit production saved values for the no-type case (likely none exist if savedFilterValues itself is new), or surface a one-time migration.
  • packages/common-utils/src/core/keyPath.ts:45 -- The arrayElement(...) branch reads parts[1].replaceAll("'", '') without checking that inner.split(',') produced two elements; a malformed input throws inside a Zod superRefine rather than returning a normalized form.
    • Fix: Guard with if (parts.length !== 2) return [key]; so malformed inputs degrade gracefully.
  • packages/common-utils/src/types.ts:1264 -- refineDashboardFiltersConstantSiblings records entry.index = i on the first occurrence and uses that index for the addIssue path; when filter[1] is the violating editable sibling of a constant filter[0], the error points at filter[0].
    • Fix: Track each conflicting sibling's index separately and emit issues at every offending position.
  • packages/common-utils/src/types.ts:1294 -- PresetDashboardFilterSchema extends DashboardFilterSchema without re-applying refineDashboardFilterCoherence, so the internal preset routes accept renderMode: 'readonly' without constant: true and constant: true without any saved-value source.
    • Fix: Wrap the preset POST/PUT body schemas with the same coherence refinement the dashboard routes apply.

Reviewers (12): correctness, testing, maintainability, project-standards, ce-agent-native-reviewer, ce-learnings-researcher, api-contract, adversarial, kieran-typescript, julik-frontend-races, security, performance.

Testing gaps:

  • useDashboardFilters on-load URL-scrub useEffect (useDashboardFilters.tsx:251) has no direct test.
  • handleSaveQuery / handleRemoveSavedQuery constant-preservation integration (DBDashboardPage.tsx:1696,1736) is not covered.
  • handleSaveFilter rename-cleanup and handleRemoveFilter sibling-preserve branches (DBDashboardPage.tsx:1438,1494) are uncovered.
  • DashboardFilters lock icon, disabled propagation, and renderMode: 'hidden' chip drop (DashboardFilters.tsx:31,47,84) lack component tests.
  • External API roundtrip lacks explicit constant: false and null-rejection cases.
  • No test exercises a constant: true + non-IN SQL savedFilterValues entry end-to-end through tile WHERE — the highest-severity finding has no failing test pinning the regression.
  • No internal-API integration test covers a constant: true save without a matching savedFilterValues entry — the coverage divergence is silent.
  • parseKeyPath (packages/common-utils/src/core/keyPath.ts) has no malformed-input tests.

alex-fedotyev and others added 6 commits June 1, 2026 15:28
E2E shard 2 was failing on three pre-existing dashboard tests because
`visibleFilters` in DashboardFilters.tsx was a new array reference every
render, churning useDashboardFilterValues' useQueries and leaving the
chip dropdown disabled while the values query never settled. Memoize
the array so a stable reference flows into the hook.

Deep review fixes:

- P0/P1: MCP hyperdx_save_dashboard inputSchema now exposes
  savedFilterValues, paired with a usage example in the filters param
  description so constant: true filters can actually lock to a value
  via MCP. The body schema already accepts it; only the tool input
  shape was missing.

- P2: DashboardFiltersModal snapshots initialDefaultValues via state
  keyed on filter.id, so a background savedFilterValues refetch no
  longer wipes in-progress edits in the default-value picker.

- P2: Schema-level coherence checks moved to common-utils via two
  reusable refinements applied at the external API boundary:
  refineDashboardFilterCoherence rejects readonly/hidden render modes
  without constant: true; refineDashboardFiltersConstantSiblings
  rejects two filter definitions sharing an expression that disagree
  on constant. Internal callers (MCP, future migrations) get the
  same protection through the shared schemas.

- P2: openapi.json + JSDoc no longer declare default: false /
  default: "editable" on the new constant / renderMode property
  schemas. The Zod schema is .optional() with no .default() so
  generated SDKs would otherwise surface a default the wire format
  never produces; the implied default now lives in the description
  text instead.

- P2: page-level merge logic for "Save default" / URL hydration /
  savedFilterValues upsert is extracted into dashboardFilterUtils
  (normalizeExpression, buildConstantExpressionSet,
  stripConstantsFromUrl, mergeConstantFiltersForSave,
  upsertSavedDefault, removeSavedDefaultForExpression). The matrix
  including bracket-notation expressions is covered by a new unit
  test. useDashboardFilters and DBDashboardPage call the helpers.

- P2: bracket-notation constant filter is now covered in the
  hook unit tests (SpanAttributes['k8s.pod.name'] resolves through
  filterValues, blocks setFilterValue, and survives via
  getFilterQueriesForSource).

P3 polish:

- handleRemoveFilter now strips the matching savedFilterValues entry
  alongside the filter delete so deleting + recreating a filter on the
  same expression doesn't silently re-lock to an orphaned value.
- applyFilterVisibility('editable') returns {} so the spread on save
  no longer leaves constant: undefined / renderMode: undefined keys
  in memory.
- defaultsChanged uses set equality so reordering the same values
  doesn't fire a spurious save write.
- NEVER_USED_RANGE hoisted to module scope.
- FilterVisibility uses z.infer<typeof DashboardFilterRenderMode>
  instead of a hand-rolled string union.
- as DashboardFilter cast dropped on filtersForQuery.
- usePresetDashboardFilters.handleSaveFilter widened to accept the
  optional defaultValues arg so a future supportsConstantFilters=true
  flip on presets doesn't silently drop the second arg.
- parseQuery import uses the @/searchFilters alias for consistency
  with sibling files.
- dashboard?.filters dropped from the URL hydration effect's deps
  (the initializedDashboard.current === dashboard.id guard already
  prevents re-runs).
- HDX-4404 API round-trip test uses toEqual instead of toMatchObject
  so unexpected extra fields fail the assertion.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Two fixes for #2383 CI:

1. DashboardFiltersModal default-value picker no longer issues a query
   when the resolved table name is empty. A metric source with no
   metricType picked yet resolves `getMetricTableName` to
   `source.from.tableName`, which is typically empty on a metric source.
   Firing the autocomplete in that state produces a malformed
   `DESCRIBE {db}.{}` (empty Identifier substitution) that the
   ClickHouse proxy rejects with a 500. The e2e shard 2 trace shows
   exactly this 500 firing during filter editing, which left the
   dashboard filter chip dropdowns disabled long enough to time out the
   click-the-option waits. Gating on `tableName` keeps the picker idle
   until the form is configured enough to name a real table.

2. The external API HDX-4404 round-trip test asserted
   `whereLanguage: 'sql'` on the response for filters whose input payload
   omits `whereLanguage`. The Zod schema is `.optional()` with no
   default, so the wire format is undefined-in / undefined-out. The
   previous expectation made the test fail by asserting a default the
   schema never emits. Dropped the field from those three filter
   expects and from the PUT-flip filter expect; added a comment so the
   omission is intentional.
…tion

The DashboardFiltersModal's FilterDefaultValueSelect fires useDashboardFilterValues
queries with a temporary 'new' filter ID. Since filterIds were computed inside
queryFn and cached, the shared React Query cache entry had filterIds: [['new']].
When the chip's useDashboardFilterValues hook (same source/expression/dateRange,
same queryKey) read from cache, filterValuesById.get(realUUID) returned undefined,
leaving the chip's VirtualMultiSelect disabled indefinitely.

Fix: compute filterIds outside queryFn (in a useMemo after useQueries returns) so
they are derived from the current caller's filter list at render time instead of
being frozen in the cache from whoever populated it first.

Also add missing await to DashboardPage.clickFilterOption's serviceFilter.click()
so the click is properly awaited before waiting for the dropdown option to appear.
Round 2 deep-review fix-pack.

**P0/P1**

- `DashboardFiltersModal.tsx`: `applyFilterVisibility('editable')` returns
  `{}`, so flipping a saved readonly/hidden filter back to "Editable"
  let `rest` carry the original `constant: true` and `renderMode`
  through the spread. The persisted filter stayed locked while the
  visibility UI claimed it was editable. Destructure `constant` and
  `renderMode` out of `rest` alongside `visibility` / `defaultValues`
  so the `visibilityFields` spread is authoritative.
- `DashboardFiltersModal.tsx`: the default-value picker's
  `filterForValueQuery` memo depended on raw `useWatch` of CodeMirror-
  backed inputs, firing a ClickHouse autocomplete request per
  keystroke with mid-edit WHERE strings that the proxy rejects. Pipe
  `watchedExpression` / `watchedWhere` / `watchedWhereLanguage`
  through `useDebouncedValue(..., 300)` (matches
  `MetricAttributeHelperPanel`) so the picker only re-queries once
  the author pauses.

**Correctness (P2)**

- `useDashboardFilters.tsx`: scrub stale `constant` expressions out
  of `filterValues` inside `setFilterValue` before
  `filtersToQuery`, AND add a one-shot hydration scrub on first
  non-empty URL snapshot. Without this, a viewer landing with a
  stale shared link that carries a value for an expression now
  locked by `constant: true` would re-publish the locked scope to
  the URL the next time any sibling filter wrote.
- `useDashboardFilters.tsx`: aggregate by normalized expression in
  the overlay loop so legacy data with mixed `constant: true` +
  editable siblings on the same expression resolves
  deterministically (any sibling lock means the saved value wins
  for every sibling).
- `openapi.json` + JSDoc: widen `SavedFilterValue.type` enum to
  `[sql, lucene]`. The UI's "Save default" flow writes
  `type: 'lucene'` and the runtime overlay only matches when the
  caller follows that shape; the previous spec advertised `[sql]`
  only.
- `common-utils/types.ts`: import `parseKeyPath` for
  `refineDashboardFiltersConstantSiblings` so the schema
  normalization matches the runtime overlay
  (`useDashboardFilters -> parseKeyPath`) and the filter helpers
  (`filters.ts -> parseKeyPath`). Move `parseKeyPath` to a new
  `core/keyPath.ts` leaf module to avoid forming a circular import
  (`types.ts -> metadata.ts -> clickhouse -> guards.ts -> types.ts`);
  `core/metadata.ts` re-exports for existing call sites.

**Correctness (P3)**

- `DBDashboardPage.tsx`: `handleRemoveFilter` no longer strips the
  shared `savedFilterValues` entry when another sibling filter
  still references the same normalized expression (two
  `constant: true` siblings scoped to different `appliesToSourceIds`
  are schema-legal and share one saved entry).
- `DashboardFilters.tsx`: rename `isReadOnly` -> `isLocked` and
  document the upstream `visibleFilters` exclusion so the name no
  longer conflates "chip is rendered locked" with "chip is locked
  but possibly hidden upstream".

**API contract**

- `mcp/tools/dashboards/schemas.ts`: apply
  `refineDashboardFilterCoherence` directly on
  `mcpDashboardFilterSchema` and document the sibling-agreement
  rule on the `.describe` block; LLM callers now hit the coherence
  rule at the input boundary instead of via downstream server
  rejection.
- `external-api/v2/dashboards.ts`: document the sibling-agreement
  rule on `FilterInput.constant` and note that `savedFilterValues`
  is overwritten as a whole on update (drop orphan entries).
- `mcp/tools/dashboards/saveDashboard.ts`: matching note on the
  `savedFilterValues` describe.

**Testing**

- `common-utils/dashboardFilter.test.ts`: 14 new tests on the two
  refinement helpers (`refineDashboardFilterCoherence` and
  `refineDashboardFiltersConstantSiblings`), including
  bracket-vs-dot normalization and nested bracket cases.
- `app/hooks/useDashboardFilters.test.tsx`: assert
  `ignoredFilterExpressions === []` on saved-vs-URL collision;
  empty `getFilterQueriesForSource` when a constant has no saved
  value; URL scrubbing on `setFilterValue` writes; aggregated-overlay
  behavior on legacy mixed siblings.
- `api/mcp/__tests__/dashboards.test.ts`: `savedFilterValues`
  round-trip with `toEqual` (catches schema drift in either
  direction); reject mismatched-sibling-constants; reject
  `renderMode: readonly` without `constant: true` on the MCP
  schema.
…xtraction

850ac84 moved parseKeyPath from metadata.ts into a leaf core/keyPath.ts
module to break a metadata -> clickhouse -> guards -> types -> metadata
cycle. External call sites kept working because metadata.ts re-exports the
name, but metadata.ts itself still uses parseKeyPath inline at the
getKeyValues body (line 1353). A re-export does not bind the name in the
local module scope, so the dts build fails with TS2304:
"Cannot find name 'parseKeyPath'" once make ci-build runs. The regular
tsc --noEmit on @hyperdx/app missed it because that workspace consumes
common-utils through its pre-built dist.

Add the matching import at the top and collapse the bottom re-export to
plain export { parseKeyPath } so the file binds and re-exports the same
symbol.

Verified locally with make ci-build, make ci-lint, and the common-utils
metadata test suite (50/50).
**P0/P1 - must fix**

- `DBDashboardPage.tsx`: `handleRemoveSavedQuery` was unconditionally
  wiping `savedFilterValues` to `[]`. With `constant: true` filters
  still active the chip kept the lock icon, `setFilterValue` still
  no-opped, but every tile ran with no scope, exactly the failure
  mode `constant` exists to prevent. Preserve the entries whose
  normalized expression matches a constant filter (mirror
  `mergeConstantFiltersForSave` with an empty URL input); editable
  saved entries fall away as intended.

**P2 - correctness / contract**

- `DBDashboardPage.tsx` `handleSaveFilter`: renaming a filter's
  expression now strips the old expression's `savedFilterValues`
  entry. Without this, the renamed filter resolved to `undefined`
  and a later "lock to default" silently re-bound to the orphaned
  value. The strip skips when another sibling still references the
  old expression (two-source locked filters legitimately share a
  saved entry).

- `external-api/v2/utils/dashboards.ts` `buildDashboardBodySchema`:
  added a `superRefine` that asserts every `constant: true` filter
  has a matching `savedFilterValues` entry on the same expression
  (Lucene-parsed, key normalized via `parseKeyPath`). Without this
  the clone-and-flip template path could silently ship a locked
  dashboard with no WHERE scope. SQL conditions are treated as
  "covers all" (best-effort) since the server doesn't parse SQL
  conditions here.

- `mcp/tools/dashboards/schemas.ts` `mcpFiltersParam`: applied
  `.superRefine(refineDashboardFiltersConstantSiblings)` so the
  array-level sibling rule (no mixing `constant: true` + editable
  on the same expression) fires at the MCP input boundary, instead
  of as a downstream `isError: true` JSON response.

- `routers/api/dashboards.ts`: wrapped the internal POST/PATCH body
  schemas (`DashboardWithoutIdSchema` and `DashboardSchema.partial()`)
  with both refinements. The shared `DashboardSchema` is left as a
  plain `ZodObject` so its `.omit()` / `.partial()` / `.extend()`
  chains keep working, so the rules are applied at the route
  boundary via a tiny pass-through schema. Closes the gap where a
  server-internal or non-MCP / non-external-API caller could persist
  `renderMode: 'readonly'` without `constant: true` or mixed
  constant/editable siblings.

- `DashboardFiltersModal.tsx`: documented the deliberate
  Visibility-preset normalization (`{ constant: true }` with no
  `renderMode` opens as the Read-only preset; saving without
  changing Visibility persists the resolved shape with
  `renderMode: 'readonly'`). Exported `getFilterVisibility` and
  `applyFilterVisibility` and added a round-trip test that pins the
  post-save shape for all four input combinations.

- `useDashboardFilters.tsx`: added an `enabled` option (default
  true) that short-circuits the overlay to empty values/queries
  while the dashboard is still loading. Wired
  `enabled: dashboardReady` from `DBDashboardPage`. Without this
  gate, the hook would treat URL entries as editable (no constants
  known yet) and emit them into tile queries before
  `dashboard.filters` and `savedFilterValues` arrive, letting React
  Query cache a tile result scoped to the stale URL value rather
  than the eventual locked saved value.

**Tests**

- New `packages/app/src/__tests__/DashboardFiltersModal.helpers.test.ts`:
  10 cases pinning `getFilterVisibility`, `applyFilterVisibility`,
  and the documented round-trip normalization.

- `packages/app/src/hooks/__tests__/useDashboardFilters.test.tsx`:
  3 new tests for the `enabled` flag (empty results when false,
  emit on flip to true, default to true when omitted).

- `packages/api/src/routers/external-api/__tests__/dashboards.test.ts`:
  4 new cases - reject renderMode without constant, reject mixed
  siblings, reject constant without savedFilterValues, accept
  constant + matching savedFilterValues round-trip. Updated the
  existing round-trip test to include matching savedFilterValues
  on its constant filters.

- `packages/api/src/mcp/__tests__/dashboards.test.ts`: 1 new case
  for "reject constant without savedFilterValues" via MCP.
  Updated the existing constant + renderMode round-trip test
  (create + update) to include savedFilterValues entries matching
  each constant filter's expression.
@alex-fedotyev
Copy link
Copy Markdown
Contributor Author

Pushed 7b9772d with round-3 deep-review fix-pack.

P0/P1

DBDashboardPage.tsx handleRemoveSavedQuery was wiping savedFilterValues to [] unconditionally, dropping every locked constant filter's value while the chip kept its lock icon and setFilterValue still no-opped. The locked filters then ran with no WHERE scope, which is the failure mode this feature exists to prevent. Preserve entries whose normalized expression matches a constant: true filter (via mergeConstantFiltersForSave with an empty URL input); editable saved entries fall away as intended.

P2 (correctness / contract)

  • handleSaveFilter rename: strip the old expression's savedFilterValues entry on rename, with sibling safety so two-source locked filters that share a saved entry don't get pruned.
  • external-api/v2/utils/dashboards.ts buildDashboardBodySchema: added a refinement that asserts every constant: true filter has a matching savedFilterValues entry on the same expression. Lucene conditions are parsed and keyed via parseKeyPath; SQL conditions are treated as "covers all" (best-effort, the server doesn't parse SQL here). Closes the clone-and-flip template gap.
  • mcp/tools/dashboards/schemas.ts mcpFiltersParam: applied .superRefine(refineDashboardFiltersConstantSiblings) so the array-level sibling rule fires at the input boundary instead of as a downstream isError: true JSON response.
  • routers/api/dashboards.ts: wrapped the internal POST/PATCH body schemas (DashboardWithoutIdSchema and DashboardSchema.partial()) with both refinements at the route boundary, so a server-internal or non-MCP / non-external-API caller can't persist renderMode: 'readonly' without constant: true or mixed siblings.
  • DashboardFiltersModal.tsx: documented the deliberate normalization ({ constant: true } with no renderMode opens as the Read-only preset; saving without changing Visibility persists with renderMode: 'readonly'). Exported getFilterVisibility and applyFilterVisibility and added a round-trip test that pins the post-save shape across all four input combinations.
  • useDashboardFilters.tsx: added an enabled option that short-circuits the overlay to empty values/queries while the dashboard is still loading. Wired enabled: dashboardReady from DBDashboardPage. Closes the race where the hook would emit stale URL values into tile queries before constants hydrate.

Tests

  • New DashboardFiltersModal.helpers.test.ts: 10 cases pinning the Visibility helpers and the round-trip normalization.
  • useDashboardFilters.test.tsx: 3 new tests for the enabled flag (empty results when false, emit on flip, default to true).
  • External-API dashboards.test.ts: 4 new cases (reject renderMode without constant, reject mixed siblings, reject constant without savedFilterValues, accept constant + matching savedFilterValues round-trip). Updated the existing round-trip test to include matching savedFilterValues.
  • MCP dashboards.test.ts: 1 new case for constant without savedFilterValues rejection via MCP. Updated the existing constant + renderMode round-trip test on create AND update to carry matching savedFilterValues.

Local: make ci-lint passes, 22 / 22 common-utils dashboardFilter, 43 / 43 app dashboardFilter + helper + hook tests pass.

Out of scope for this fix-pack (deferred to the follow-up tracking issue):

  • Id-keyed savedFilterValues refactor (Option A from the round-2 P2 findings). Wire-format change; needs its own plan.
  • The remaining P3 nitpicks from the latest deep review (14 of them: usePresetDashboardFilters threading, hydration block gating, mergeConstantFiltersForSave Lucene coercion, normalized-expression dedupe, hasScrubbedConstantsRef keyed by dashboard id, etc.).

Conflict in `packages/common-utils/src/core/metadata.ts`:

Main (PR #2393, "map alias with filters") extended the inline
`parseKeyPath` with the ClickHouse `arrayElement(map, 'key')`
function-call form (the shape ClickHouse renders Map subscript
expressions as in result column names) plus the double-quoted
bracket form.

This branch had moved `parseKeyPath` out of `core/metadata.ts` into
its own leaf module at `core/keyPath.ts` so `types.ts` can
normalize dashboard-filter expressions with the same rules
without forming a circular import through
`metadata.ts -> clickhouse -> guards.ts -> types.ts`.

Resolution: keep the leaf module + re-export structure from this
branch, fold main's `arrayElement` handling AND the
double-quoted form into `core/keyPath.ts`, and let the
re-export in `core/metadata.ts` surface the merged
implementation to call sites that still import from there.
The four new `arrayElement` test cases in
`packages/common-utils/src/__tests__/metadata.test.ts` exercise the
re-exported function unchanged.

Verified:
- common-utils: 1195 / 1195 tests pass (was 1191 + 4 new arrayElement tests from main).
- common-utils lint clean.
- app lint clean (after prettier auto-fix on the helpers test).
- app: 131 / 131 tests pass across dashboardFilter / useDashboardFilters / DashboardFiltersModal.helpers / DBRowTable.
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