Skip to content

feat: Add source scoping to dashboard filters#2331

Merged
kodiakhq[bot] merged 2 commits into
mainfrom
drew/dashboard-filters-source-scoping
May 26, 2026
Merged

feat: Add source scoping to dashboard filters#2331
kodiakhq[bot] merged 2 commits 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 26, 2026 12:53pm

Request Review

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented May 22, 2026

🦋 Changeset detected

Latest commit: 384bfe0

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 • 189 passed • 3 skipped • 1194s

Status Count
✅ Passed 189
❌ 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

Scope: PR #2331 — 23 files, ~1200 net lines. Adds appliesToSourceIds?: string[] to dashboard filters; touches UI (filter modal, multiselect, dashboard page), import/export name↔ID remapping, MCP schemas/prompts, external API v2, and common-utils.

Intent: Source-scope dashboard filters so a filter (e.g. SpanName) can apply only to tiles using compatible sources, while existing broadcast-to-all remains the default.

✅ No critical issues found. P0/P1 bar (data loss, auth bypass, guaranteed-crash regression on the happy path) not met. The PR's core mechanic is well-covered by unit and e2e tests for the auto-resolve happy path, and the round-trip on the external API is asserted. Findings below are recommended fixes and nits — none should block merge once the author triages.

🟡 P2 -- recommended

  • packages/api/src/routers/external-api/v2/utils/dashboards.ts:777 -- getMissingSources validates filter.sourceId but skips filter.appliesToSourceIds, so the API silently accepts scope IDs that don't belong to the team (cross-team, deleted, or fabricated).
    • Fix: Add filter.appliesToSourceIds ?? [] entries to the sourceIds set in getMissingSources so the same rejection that fires for sourceId fires for scope IDs.
    • api-contract, adversarial, agent-native
  • packages/common-utils/src/core/utils.ts:537 -- when every entry in appliesToSourceIds fails to resolve at export time, the array is collapsed to undefined, so re-importing the template silently broadcasts the filter to every tile — including sources where the expression isn't valid, producing query errors.
    • Fix: Surface a non-empty sentinel (e.g. preserve original stale names or emit []) so the import screen renders the applies-to mapping row and gives the user a chance to remap, instead of silently degrading the scope.
    • correctness, adversarial
  • packages/app/src/hooks/useDashboardFilters.tsx:105 -- when two DashboardFilter rows share an expression, the UI keys their value by expression so both fill on the first selection, and getFilterQueriesForSource unions their scopes — a "Service (Logs only)" filter and a "Service (Traces only)" filter collapse into one shared-value filter that applies to both sources.
    • Fix: Either key filter values by filter.id instead of by expression, or reject duplicate expressions in DashboardFiltersModal, so the per-filter scope UI carries through to the downstream value+scope semantics.
    • adversarial
  • packages/api/openapi.json:2351 -- OpenAPI/JSDoc describes appliesToSourceIds: [] as equivalent to omitted, but the server does not normalize [] → undefined in convertExternalFiltersToInternal, so [] round-trips as [] while undefined round-trips as absent; the MCP schema description further omits the [] equivalence entirely.
    • Fix: Normalize [] to undefined in convertExternalFiltersToInternal, or align all three surfaces (OpenAPI/JSDoc, MCP describe, runtime) on a single story.
    • api-contract
  • packages/app/src/hooks/useDashboardFilters.tsx:97 -- new four-branch getFilterQueriesForSource (broadcast applies, scoped+matching applies, scoped+undefined-sourceId skips, union across same-expression) has no direct unit test; only an indirect mock via usePresetDashboardFilters.test.tsx:113 and one e2e flow.
    • Fix: Add unit tests in useDashboardFilters.test.tsx covering broadcast, scoped match, scoped+undefined-sourceId skip, and the union semantics across two same-expression filters.
    • testing
  • packages/app/src/DBDashboardImportPage.tsx:225 -- resolveAppliesToSources and the splice-based propagation block (case-insensitive matching, resolvedIndexOf, isDirty gating, splice-by-index update) are exercised only by the auto-resolve happy-path e2e; the manual-change and stale-name branches have no test coverage at all.
    • Fix: Extract resolveAppliesToSources into a tested module and add unit coverage for case-insensitive matching, stale-name drop, resolvedIndexOf(-1), and the splice early-return when selectedSourceId is already present.
    • testing
  • packages/app/tests/e2e/features/dashboard.spec.ts:592 -- the "tooltip shows 'Applies to all sources'" e2e only asserts the tooltip text; it never selects a filter value or adds a second-source tile, so the default broadcast behavior isn't actually verified end-to-end.
    • Fix: Add a second tile from a different source, pick a value, and assert both tiles re-query with the filter applied.
    • testing
  • packages/api/src/routers/external-api/__tests__/dashboards.test.ts:855 -- round-trip tests assert valid IDs survive, but there's no negative test for appliesToSourceIds containing IDs that don't belong to the team, malformed ObjectIDs, or duplicates — so the validation gap above is doubly untested.
    • Fix: Add a negative test asserting the documented behavior (reject if validation is added per the related finding, or assert silent acceptance if not).
    • testing
  • packages/app/tests/e2e/features/dashboard.spec.ts:493 -- e2e covers adding a scoped filter, but not editing the scope on an existing filter, clearing the multiselect back to broadcast, persistence across reload, or the tileSourceId === undefined branch (RawSQL/markdown tiles).
    • Fix: Add e2e cases that edit an existing filter's scope, clear it, reload the dashboard, and verify a RawSQL/markdown tile correctly skips scoped filters.
    • testing
🔵 P3 nitpicks (6)
  • packages/app/src/DBDashboardImportPage.tsx:486 -- filter.appliesToSourceIds?.includes(inputSourceName) is case-sensitive while resolveAppliesToSources (line 234) is case-insensitive, so a hand-edited template with mismatched case resolves on init but won't auto-propagate when its tile-source mapping changes.
    • Fix: Compare lowered names in the guard so both sides of the resolution agree.
    • correctness, kieran-typescript
  • packages/api/src/mcp/tools/dashboards/schemas.ts:691 -- MCP uses z.array(objectIdSchema) while common-utils uses z.array(z.string().min(1)); both surfaces accept the field but only MCP rejects non-ObjectID strings, creating a class of dashboards that the external API persists but MCP can't read back.
    • Fix: Align by either tightening DashboardFilterSchema to an ObjectID-shaped string or loosening the MCP schema to z.string().min(1).
    • api-contract, adversarial
  • packages/app/src/components/SourceMultiSelect.tsx:19 -- the component declares allowedSourceKinds, connectionId, and size props but the two call sites (DashboardFiltersModal.tsx:197, DBDashboardImportPage.tsx:871) pass none of them — dead surface area that widens the type contract for no current consumer.
    • Fix: Drop the three unused props until a caller actually needs filtering; add them back when needed.
  • packages/app/src/DBDashboardImportPage.tsx:489 -- when the user changes a tile-source mapping to a workspace source that's already in the applies-to list at a different index, the propagation early-returns before splicing, leaving the stale ID at indexToUpdate instead of replacing it; on submit the filter is persisted with both the stale and the new ID.
    • Fix: Drop the includes() early return for the splice path, perform the replacement, then dedupe the array before setValue.
  • packages/app/src/DBDashboardImportPage.tsx:265 -- filterAppliesToSourceMappings: z.array(z.array(z.string())).optional() is looser than the persisted z.array(z.string().min(1)).optional(); the empty-string filter at submit covers this today but a future caller could persist '' IDs that fail re-parse.
    • Fix: Tighten the inner element to z.string().min(1) so the form schema matches the persisted schema.
  • packages/common-utils/src/types.ts:1072 -- PresetDashboardFilterSchema extends DashboardFilterSchema and so now accepts appliesToSourceIds at the API boundary, but the mongoose model in packages/api/src/models/presetDashboardFilter.ts doesn't declare the field, so Mongoose strict mode silently drops it on save to the preset endpoint.
    • Fix: Either add appliesToSourceIds to the mongoose schema, or omit it from PresetDashboardFilterSchema so the contract reflects what is actually persisted.

Pre-existing (not introduced by this PR, surface only)

  • The init useEffect in DBDashboardImportPage.tsx:297 already overwrites user-edited mappings when react-query refetches sources / connections / dashboards in the background; this PR widens the blast radius to the new filterAppliesToSourceMappings field but does not introduce the underlying issue.
  • DBDashboardPage.tsx (~2667 lines) and DashboardFiltersModal.tsx (~542 lines) are already past the repo's 300-line guideline; this PR's contributions are bounded (+9 and +61 respectively).

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

Testing gaps:

  • getFilterQueriesForSource union-across-same-expression branch and sourceId === undefined branch have no direct test.
  • resolveAppliesToSources and the splice-based propagation in DBDashboardImportPage are exercised only by the auto-resolve happy-path e2e.
  • External API has no negative test for invalid / cross-team / malformed entries in appliesToSourceIds.
  • No e2e for editing or clearing an existing filter's scope, persistence across reload, or the RawSQL/markdown (no-source) tile path.
  • convertToDashboardTemplate tests don't cover duplicate IDs in appliesToSourceIds or two-sources-same-name collisions.

@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
@kodiakhq kodiakhq Bot merged commit 04a5a92 into main May 26, 2026
28 of 29 checks passed
@kodiakhq kodiakhq Bot deleted the drew/dashboard-filters-source-scoping branch May 26, 2026 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

automerge 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