Skip to content

feat: Add source scoping to dashboard filters#2331

Open
pulpdrew wants to merge 1 commit into
mainfrom
drew/dashboard-filters-source-scoping
Open

feat: Add source scoping to dashboard filters#2331
pulpdrew wants to merge 1 commit into
mainfrom
drew/dashboard-filters-source-scoping

Conversation

@pulpdrew
Copy link
Copy Markdown
Contributor

Summary

This PR allows dashboard filters to be scoped to particular sources. All tiles on the dashboard that use one of the selected sources will inherit the filter value. The existing behavior (apply filter to all tiles, regardless of source) remains the default.

This is useful for multi-source dashboards where a filter (eg. SpanName) is compatible with some sources (eg. Traces) and not others (eg. Logs).

This PR includes

  1. Updated UI to allow editing and viewing the filter scope
  2. Updated dashboard export and import to allow for mapping the filter's scope during import
  3. Updated MCP schemas and prompts
  4. Updated external API schemas and specs

Screenshots or video

Screenshot 2026-05-22 at 11 13 17 AM Screenshot 2026-05-22 at 11 13 10 AM

How to test on Vercel preview

This can mostly be tested in the preview environment

  1. Create a dashboard with tiles from different sources
  2. Create some filters, scope some of them to particular sources
  3. Select values for the filters and note that the scoped ones apply only to the appropriate tiles
  4. Try import/export

Locally, you can test the MCP updates and external dashboards updates.

References

  • Linear Issue: Closes HDX-4330
  • Related PRs:

@vercel
Copy link
Copy Markdown

vercel Bot commented May 22, 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 22, 2026 5:38pm

Request Review

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented May 22, 2026

🦋 Changeset detected

Latest commit: 523c74f

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

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

github-actions Bot commented May 22, 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/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: 869 (+ 537 in test files, excluded from tier calculation)
  • Branch: drew/dashboard-filters-source-scoping
  • 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
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 22, 2026

E2E Test Results

All tests passed • 182 passed • 3 skipped • 1238s

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

Tests ran across 4 shards in parallel.

View full report →

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 22, 2026

Deep Review

✅ No critical issues found. Source scoping is plumbed coherently across the React UI, MCP, REST/OpenAPI, and the import/export converter, with new e2e and round-trip tests covering the happy path. The findings below are mostly missing validation, contract gaps that surface as silent UX failures, and untested branches in the new logic.

🟡 P2 — recommended

  • packages/api/src/routers/external-api/v2/utils/dashboards.ts:804getMissingSources iterates filter.sourceId but ignores filter.appliesToSourceIds, so a POST/PUT with bogus or non-existent IDs in the new scope field is accepted, persisted, and the filter then matches no tile because appliesTo.includes(sourceId) is permanently false; the OpenAPI/MCP descriptions promise validation that does not happen.
    • Fix: Inside the if (filters?.length) block, also iterate filter.appliesToSourceIds (when present) and add each entry to sourceIds so the existing-source diff covers scope IDs symmetrically with sourceId.
    • correctness, adversarial, agent-native
  • packages/common-utils/src/core/utils.ts:537 — On export, convertToFilterTemplate remaps each scope ID to a source name and then collapses an empty remap to undefined (remapped.length > 0 ? remapped : undefined), so a filter whose every scoped source was deleted silently becomes a broadcast filter on the next round-trip; the import side mirrors this in DBDashboardImportPage.tsx:674 where appliesTo?.length ? appliesTo : undefined turns "all names unresolved" into broadcast scope.
    • Fix: Distinguish "unscoped" (no field) from "all-stale" (had IDs but none resolved) on export and import — preserve a sentinel/marker or surface a warning in the import Mapping UI when the multiselect renders empty for a filter whose template carried a non-empty appliesToSourceIds, so scope broadening is never silent.
    • correctness, adversarial
  • packages/app/src/hooks/useDashboardFilters.tsx:74filtersByExpression groups filters by expression and the .some() predicate unions their scopes, so two definitions sharing an expression (e.g. service.name scoped to traces + service.name broadcast) erase the scoped one's intent; filterValues is also keyed by expression, so editing either dropdown writes one shared value that immediately appears in both header chips with mismatched option lists.
    • Fix: Either de-duplicate by expression at filter-edit time, key filterValues by filter.id, or document the union/shared-value semantics on the DashboardFilter type and on the modal so users do not author conflicting scopes by accident.
    • correctness, adversarial
  • packages/api/src/mcp/tools/dashboards/schemas.ts:692mcpDashboardFilterSchema.appliesToSourceIds enforces z.array(objectIdSchema) while the REST contract (packages/common-utils/src/types.ts:1063) and the OpenAPI schema (packages/api/openapi.json:2353) only require z.array(z.string().min(1)); a payload that POSTs successfully via REST can be rejected on a subsequent MCP edit (and vice versa), and the React UI has no validation at all.
    • Fix: Pick one shape — either tighten DashboardFilterSchema.appliesToSourceIds to ObjectId everywhere, or loosen the MCP schema to z.string().min(1) — so the three surfaces agree on what they accept.
    • api-contract, adversarial
  • packages/app/src/DBDashboardPage.tsx:1355const filters = dashboard?.filters ?? []; constructs a fresh [] on every render when no filters are declared; that array is the dep of the useMemo inside useDashboardFilters, so the memo and getFilterQueriesForSource re-derive each render and the useCallback(renderTileComponent, [getFilterQueriesForSource]) identity changes per render too, defeating tile-level memoization.
    • Fix: Hoist a module-level EMPTY_FILTERS: DashboardFilter[] = [] constant and use dashboard?.filters ?? EMPTY_FILTERS, or have useDashboardFilters default an undefined input internally to a stable sentinel.
    • kieran-typescript
  • packages/app/src/hooks/useDashboardFilters.tsx:97 — The new getFilterQueriesForSource has at least five distinct branches (unscoped, empty array, scope-includes, scope-excludes, sourceId === undefined for RawSQL tiles, plus the documented union semantics), but no unit test exercises any of them — usePresetDashboardFilters.test.tsx only mocks the return value, and only one e2e happy path runs the real code.
    • Fix: Add unit tests covering each branch in packages/app/src/hooks/__tests__/useDashboardFilters.test.tsx, including sourceId=undefined (RawSQL/markdown), explicit empty appliesToSourceIds, and two-filter same-expression union semantics.
    • correctness, testing, kieran-typescript
  • packages/app/src/DBDashboardImportPage.tsx:229resolveAppliesToSources is a new helper with non-trivial behavior (case-insensitive matching, ordered drop of unresolved names, resolvedIndexOf fallback to -1), and it's exercised only by one e2e happy path; refactors of the resolver, off-by-one in the splice helper, or a regression of the case-insensitivity assumption would not be caught.
    • Fix: Add a focused unit test in packages/common-utils/src/__tests__/ (or co-located) covering undefined inputs, mixed-case matches, partial resolution preserving order, and resolvedIndexOf returning -1 vs a valid index.
    • testing, maintainability
  • packages/api/src/routers/external-api/__tests__/dashboards.test.ts:847 — The OpenAPI text and the runtime (useDashboardFilters.tsx:114 treats [] as broadcast) both promise that appliesToSourceIds: [] is equivalent to omitting the field, but no test POSTs or PUTs an explicit empty array, so a future change that begins to persist [] as a distinct value would silently break the contract.
    • Fix: Add a single POST and a single PUT case sending appliesToSourceIds: [], then assert the read-back filter behaves identically to a filter with the field omitted.
    • api-contract, testing
  • packages/app/src/DashboardFiltersModal.tsx:356 — When appliesToSourceIds is non-empty but every referenced source has been deleted, appliedSourceNames is [] and the row renders as a blank string; the multiselect in the edit modal also cannot render unknown IDs as chips, so a save (which preserves the dead IDs via filter(id => !!id?.length)) silently re-persists an inert filter the user has no way to repair from the UI.
    • Fix: When the resolved-name array is empty but the underlying ID array is non-empty, render "No matching sources" (or the raw IDs as disabled chips) in both the list view and the modal so users can identify and clear stale scope.
    • correctness, adversarial
🔵 P3 nitpicks (12)
  • .changeset/great-papayas-obey.md — The diff against the user-supplied base SHA b94b8eff shows apparent reverts of feat: support text index on lower(Body) with no preprocessor #2326 (lower(Body) text index) and fix: persist column widths in search results table #2327 (column-width persistence) because the PR branched off 8938b05e and was never rebased onto current main; standard 3-way merge / squash / rebase merges will NOT actually revert those features (the PR commit 523c74f5 itself touches none of those files), but the noisy diff makes the PR materially harder to review.
    • Fix: Rebase the branch onto current main and force-push so the reviewer-visible diff contains only source-scoping changes.
  • packages/app/src/DBDashboardImportPage.tsx:486 — The propagation gate uses case-sensitive filter.appliesToSourceIds?.includes(inputSourceName), but resolveAppliesToSources and resolvedIndexOf both use .toLowerCase(); a case-mismatched template name resolves through the helper but is skipped by the gate, leaving the multiselect silently out of sync after a mapping change.
    • Fix: Normalize both sides of the comparison with .toLowerCase() or push the lookup through the same resolvedIndexOf the propagation block already uses.
  • packages/app/src/DBDashboardPage.tsx:1764 — When a RawSQL tile's config.source has been retroactively invalidated, getFilterQueriesForSource silently drops every scoped filter for that tile (appliesTo.includes(stale-id) is false) with no UI signal that the filter was meant to apply.
    • Fix: Either fail validation when a tile's config.source no longer references an existing source, or surface a tile-level warning when scoped filters are dropped for an unrecognized sourceId.
  • packages/app/src/components/sourceSelectUtils.tsx:10SOURCE_KIND_ICONS: Record<string, React.ReactNode> accepts any string key, which lets call sites do SOURCE_KIND_ICONS[map.get(value) ?? ''] and silently fall through; the map is keyed exclusively by SourceKind, so typing it as Record<SourceKind, React.ReactNode> would force callers to narrow.
    • Fix: Tighten to Record<SourceKind, React.ReactNode> and have callers guard with const kind = map.get(value); const icon = kind ? SOURCE_KIND_ICONS[kind] : undefined;.
  • packages/app/src/DashboardFiltersModal.tsx:154values.appliesToSourceIds?.filter(id => !!id?.length) defensively filters for empty/null entries against a z.array(z.string().min(1))-typed array that never contains such values; the same redundant guard repeats in DBDashboardImportPage.tsx:668.
    • Fix: Drop the filter and rely on the schema, or move the contract into the form-state schema instead of guarding at every site.
  • packages/api/src/mcp/tools/dashboards/schemas.ts:695 — The MCP description for appliesToSourceIds says "Omit (or pass undefined)" without documenting that an empty array is equivalent; the OpenAPI description does promise empty-array equivalence, so an LLM client reading only the MCP description will believe empty arrays are disallowed.
    • Fix: Add a sentence to the MCP description noting that an empty array behaves identically to omitting the field.
  • packages/app/src/DBDashboardImportPage.tsx:229resolveAppliesToSources returns { ids, resolvedIndexOf } but the two call sites each use only one half (line 331 reads .ids, line 493 reads .resolvedIndexOf); the closure is rebuilt on every invocation even though the inputs are stable inside the effect.
    • Fix: Split into two narrower helpers (or memoize the lookup map outside the effect) so each call site sees only the surface it actually uses.
  • packages/app/src/DBDashboardImportPage.tsx:393 — The single Mapping propagation effect now spans ~110 lines and conflates "which input changed?" detection with "propagate to which targets?" application across four orthogonal channels (tile/filter/onClick/connection mappings plus per-filter applies-to).
    • Fix: Extract a small detectMappingChange(prevRefs, current, getFieldState) helper returning { source, selectedId } | null, and split applies-to propagation into its own effect.
  • packages/common-utils/src/core/utils.ts:532DashboardFilterSchema.appliesToSourceIds is typed and named as if it always contains IDs, but after convertToDashboardTemplate the same field carries source names; this mirrors the pre-existing overload on filter.source, but a future reader inspecting an exported template will see strings labeled "Ids" that are not IDs.
    • Fix: Document the dual semantics on the schema field's JSDoc, or rename the templated form (e.g. unified appliesToSources).
  • packages/app/src/DBDashboardImportPage.tsx:525 — Adding && getFieldState('connectionMappings.${idx}').isDirty to the change-detection (and the same tightening for tile/filter/onClick/onClickDashboard mappings) means initial cross-fill from the auto-init setValue calls no longer propagates, because setValue defaults to shouldDirty: false.
    • Fix: Either pass { shouldDirty: true } from the auto-init setValue so the propagation chain still fires on initialization, or document the new "manual-edit-only" propagation behavior and add a test that pins it.
  • packages/app/src/DBDashboardImportPage.tsx:673findSource(data.filterSourceMappings?.[idx])!.id throws TypeError when the user submits with a blank mapping (''), surfacing only as the generic "Something went wrong" catch (pre-existing pattern shared with the tile-mapping path).
    • Fix: Tighten MappingFormStateSchema to require z.string().min(1) on per-row source mappings, or replace the non-null assertion with a validation error mentioning the missing row.
  • packages/app/src/DashboardFiltersModal.tsx:1DashboardFiltersModal.tsx (542 LOC) and DBDashboardImportPage.tsx (979 LOC) exceed the project's 300-line target documented in AGENTS.md; both grew further in this PR.
    • Fix: Continue extracting helpers in the spirit of the new sourceSelectUtils.tsx/SourceMultiSelect.tsx split — for example, factor the applies-to row and import mapping resolver into their own files.

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

Testing gaps:

  • No e2e exercising the manual-remap flow for the new "Applies to Sources" multiselect on the import page — only auto-resolution is covered.
  • No MCP integration test creates/updates a dashboard with appliesToSourceIds and asserts round-trip.
  • No test simulates source deletion against an existing dashboard whose filters reference the deleted source via appliesToSourceIds.
  • convertToDashboardTemplate round-trip with all-stale IDs (silent broadening) is uncovered.

@pulpdrew pulpdrew force-pushed the drew/dashboard-filters-source-scoping branch from 1712c1d to 523c74f Compare May 22, 2026 17:35
@pulpdrew pulpdrew requested review from a team and karl-power and removed request for a team May 22, 2026 17:55
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