Skip to content

feat: Add filter templating to custom dashboard on-click#2146

Merged
kodiakhq[bot] merged 11 commits into
mainfrom
drew/dashboard-filter-linking
May 11, 2026
Merged

feat: Add filter templating to custom dashboard on-click#2146
kodiakhq[bot] merged 11 commits into
mainfrom
drew/dashboard-filter-linking

Conversation

@pulpdrew

@pulpdrew pulpdrew commented Apr 22, 2026

Copy link
Copy Markdown
Contributor

Summary

This is the third in a series of PRs adding customizable on-click / linking behaviors to dashboard tables. This PR extends the custom dashboard table onClick behavior from #2140.

This feature is behind the NEXT_PUBLIC_IS_DASHBOARD_LINKING_ENABLED, which has been enabled in the preview environment and for local development.

Scope

In this PR:

  • Links can now include templated filter values, to set filters on the target dashboard or search page.
  • The dashboard page now has a warning banner when the URL contains a filter expression which is ignored because there is no corresponding filter defined on the dashboard.

Not included yet

Future PRs will add:

  • Update dashboard import to support link source and dashboard ID mappings
  • Updates to the external API to support the new onClick fields
  • Updates to the MCP prompts to support generating dashboards with custom links
  • Updates to the import flow to support importing bundles of linked dashboards at once
  • Support for linking on other visualization types

Screenshots or video

Screen.Recording.2026-04-23.at.4.58.18.PM.mov

How to test locally or on Vercel

This can be tested in the preview environment

References

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

@vercel

vercel Bot commented Apr 22, 2026

Copy link
Copy Markdown

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

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
hyperdx-oss Ignored Ignored Preview May 11, 2026 5:33pm

Request Review

@changeset-bot

changeset-bot Bot commented Apr 22, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: f4a954c

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/app Patch
@hyperdx/api 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

enableMapSet();

describe('searchFilters', () => {
describe('filtersToQuery', () => {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

filtersToQuery and its tests have been moved to common-utils

@pulpdrew pulpdrew force-pushed the drew/dashboard-dashboard-linking branch 3 times, most recently from 8902ef4 to 7f746b7 Compare April 23, 2026 18:23
@pulpdrew pulpdrew force-pushed the drew/dashboard-filter-linking branch from ca2be83 to de0c501 Compare April 23, 2026 20:40
@pulpdrew pulpdrew changed the base branch from drew/dashboard-dashboard-linking to main April 27, 2026 02:34
@pulpdrew pulpdrew force-pushed the drew/dashboard-filter-linking branch from de0c501 to 226d4ff Compare April 27, 2026 02:48
@pulpdrew pulpdrew marked this pull request as ready for review April 27, 2026 02:48
@github-actions github-actions Bot added the review/tier-3 Standard — full human review required label Apr 27, 2026
@github-actions

github-actions Bot commented Apr 27, 2026

Copy link
Copy Markdown
Contributor

🔴 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/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: 13
  • Production lines changed: 518 (+ 1232 in test files, excluded from tier calculation)
  • Branch: drew/dashboard-filter-linking
  • Author: pulpdrew

To override this classification, remove the review/tier-4 label and apply a different review/tier-* label. Manual overrides are preserved on subsequent pushes.

@github-actions

github-actions Bot commented Apr 27, 2026

Copy link
Copy Markdown
Contributor

PR Review

✅ No critical issues found.

The feature is well-scoped, well-tested, and the encoding/escaping concerns are handled correctly. A few minor, non-blocking observations:

  • ⚠️ packages/api/src/routers/external-api/v2/utils/dashboards.ts:488 — the // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion was removed but as SavedChartConfig was kept. The rule is still active (warn). It's filtered by lint --quiet, but the removal looks unintentional / unrelated to this PR — consider restoring it or removing the cast.
  • ⚠️ DBDashboardPage.tsx — the banner useEffect intentionally omits ignoredFilterExpressions from deps (eslint-disabled). The trade-off (banner doesn't reappear after dashboard-save refetch, but also won't react to URL filter changes for the current dashboard) is documented in the comment and verified by an e2e step; acceptable as a known limitation.
  • 💡 OnClickDrawer.tsxsetValue/getValues are prop-drilled through ModeFieldsDashboardOnClickFields. Since the useForm instance is local to OnClickDrawer, a FormProvider + useFormContext would avoid the drilling, but this is purely cosmetic.
  • 💡 renderFilterTemplates only emits IN (...) clauses (no NOT IN, BETWEEN, or boolean handling) — matches the PR's scope, but worth a TODO/comment so a future contributor doesn't expect parity with FilterState.
  • 💡 validateOnClickTemplate switches on filter.kind === 'expressionTemplate', which is the only variant today. Good forward-compatible shape; just confirm there's no path where an unknown kind would silently skip validation.

Nice work — the escape/round-trip tests (backslashes + O\'Malley) and the e2e coverage for both the rendered filter and the ignored-filter banner are particularly thorough.

@github-actions

github-actions Bot commented Apr 27, 2026

Copy link
Copy Markdown
Contributor

E2E Test Results

All tests passed • 169 passed • 3 skipped • 1187s

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

Tests ran across 4 shards in parallel.

View full report →

@github-actions

github-actions Bot commented May 11, 2026

Copy link
Copy Markdown
Contributor

Deep Review

Scope: PR #2146 — dashboard onClick filter templates + URL filters= param wiring, ~1500 added lines across 22 files.
Intent: When a dashboard table row is clicked, render configured handlebars templates against the row, package them as SQL Filter[], JSON-stringify into a filters= URL param consumed by the destination dashboard/search page. Also moves filtersToQuery/FilterState from app → common-utils, adds backslash escaping, and adds an "ignored URL filters" warning banner.

🔴 P0/P1 -- must fix

  • packages/common-utils/src/core/linkUrlBuilder.ts:248 -- validateOnClickTemplate's new onClick.filters loop (calls validateTemplate(filter.template) on every expressionTemplate) has zero test coverage — invalid Handlebars in a filter template will not throw at save time, only at render.
    • Fix: Add validateOnClickTemplate test cases covering a valid filter template, an invalid filter template (must throw), and filters: undefined/[] (no-op).
    • testing
  • packages/app/src/utils/queryParsers.ts:43 -- parseAsJsonEncoded<Filter[]>() does JSON.parse and casts to T with no runtime validation, so any URL filters= value that parses as JSON is typed as Filter[] regardless of actual shape; useDashboardFilters and DBSearchPage then iterate it without guarding {type, condition}.
    • Fix: Validate parsed JSON against a Zod FilterSchema.array() inside the consumer (or extend parseAsJsonEncoded with an optional validator) and treat invalid as null so the existing nullish handling kicks in.
    • kieran-typescript
  • packages/app/src/DBSearchPage.tsx:791 (pre-existing) -- filters: parseAsJsonEncoded<Filter[]>() reads attacker-controllable SQL fragments from the URL and forwards them through buildSearchChartConfigrenderWhereExpressionchSql\${UNSAFE_RAW_SQL: filter.condition}`; the /searchpage has no expression-allowlist gate the way/dashboards/[id]` does, and this PR adds a new convenient producer of these URLs.
    • Fix: Reject any URL-supplied condition that parseQuery cannot reconstruct (i.e. require simple <col> IN (...) shapes) before it can reach UNSAFE_RAW_SQL, or HMAC-sign pre-populated filter URLs.
    • security, kieran-typescript

🟡 P2 -- recommended

  • packages/app/src/components/DBEditTimeChartForm/OnClickForm/OnClickDrawer.tsx:90 -- handleTargetChange calls setValue('onClick.filters', ...) on a path managed by useFieldArray in FilterTemplateList; allTemplatesEmpty is also true for arrays where the user has typed an expression but no template, so picking a target silently discards in-progress rows, clearing the Select wipes all filter rows, and the visible field-array may remount with fresh ids and lose focus.
    • Fix: Lift useFieldArray (or pass replace down) and call fieldArray.replace(newRows); gate on target.mode === 'id' && target.id !== '' and on currentFilters.length === 0 rather than per-row template emptiness.
    • correctness, maintainability, kieran-typescript, julik-frontend-races, adversarial
  • packages/app/src/DBDashboardPage.tsx:1268 -- the banner-latch useEffect deliberately omits ignoredFilterExpressions from its deps and sets lastLoadedIdForBannerRef.current = dashboard?.id unconditionally; adding/removing a declared dashboard filter does not re-evaluate the banner, mutating the URL via nuqs on the same dashboard does not either, and the in-code "Known limitation" comment matches the PR's primary user story (clicking an onClick link that lands on the same dashboard the user is already on).
    • Fix: Either key the latch on (dashboard.id, ignoredFilterExpressions.join('|')) and surface staleness via a dismissed-id Set ref, or compute the banner as derived state gated by a per-id dismissal record so the eslint-disable goes away.
    • correctness, maintainability, adversarial, julik-frontend-races
  • packages/app/src/components/DBEditTimeChartForm/OnClickForm/OnClickDrawer.tsx:80 -- handleTargetChange reads dashboards from useDashboards(), which can be undefined on first render; selecting a target before the list resolves auto-populates zero rows and the logic never retries when dashboards arrives.
    • Fix: Disable the target Select while dashboards is loading, or run an effect that re-applies the auto-populate when dashboards becomes available and onClick.target.id already points at a known target with declared filters.
    • julik-frontend-races
  • packages/app/src/searchFilters.tsx:313 (pre-existing, expanded surface) -- the BETWEEN parser is regex-based, runs against the whole condition (not per-AND-part), and is not quote-aware: a hostile URL with condition: "x BETWEEN 'a' AND 'b'" produces {min:NaN, max:NaN} that is rebuilt as BETWEEN NaN AND NaN and breaks every tile, and "y IN ('a') AND x BETWEEN 1 AND 2" silently drops the IN half.
    • Fix: Split on AND-outside-quotes first (splitOnFirstOutsideQuotes), apply the BETWEEN regex per part, and reject the clause when either bound is not Number.isFinite.
    • security, adversarial
  • packages/common-utils/src/core/linkUrlBuilder.ts:30 -- renderFilterTemplates relies on Handlebars-stringification of row values, so object cells render [object Object], array cells produce comma-joined strings that get IN-quoted as a single value, and numeric/boolean row values reach an integer ClickHouse column as '42' / 'true' (type mismatch → empty results); none of these surface as a render error.
    • Fix: Reject (renderOrError → {ok:false}) when the toString result is '[object Object]', when the source value is an array, or when it is a non-string and the schema expects a literal, with a clear "Link error" message.
    • adversarial
  • packages/common-utils/src/core/linkUrlBuilder.ts:30 -- a template that renders to '' (missing/null row column with {{default ...}} fallback, or a null field) is added to the included Set and emitted as IN (''), which filters for empty-string rows rather than "no filter" — indistinguishable from a real no-results page.
    • Fix: Skip entries whose rendered value is '' (or treat empty render as a render error so the user sees a clear "Link error" toast).
    • adversarial
  • packages/common-utils/src/filters.ts:11 -- the new escapeString doubles \ before escaping ', but the previous in-app filtersToQuery did not escape backslashes at all; any pre-existing URL/bookmark/saved chart that contains a literal backslash now round-trips differently ('C:\foo' produces 'C:\\foo' on serialize, and legacy \\ is now decoded as a single \ by the updated parser).
    • Fix: Call this out in the changeset as a deliberate breaking change, and add an explicit "legacy single-backslash" round-trip test alongside the new ones so the contract is pinned.
    • correctness, api-contract
  • packages/common-utils/src/filters.ts:11 -- escapeString lives here, but its inverse (getBooleanOrUnquotedString + isQuoteBoundary) sits in packages/app/src/searchFilters.tsx, and the inverse's correctness depends on the exact escape order via a prose comment; round-trip tests live only in the app package.
    • Fix: Either move the inverse parser to common-utils/filters.ts next to escapeString, or export escapeString and add round-trip coverage inside packages/common-utils/src/__tests__/filters.test.ts so a future common-utils-only consumer can't silently break the contract.
    • maintainability, kieran-typescript
  • packages/common-utils/src/core/linkUrlBuilder.ts:11 -- the file declares LinkBuildResult = {ok:true; url:string} | {ok:false; error:string} and then reinvents the same shape twice more (renderOrError's {ok:true; value:string} and renderFilterTemplates's {ok:true; filters: Filter[]}).
    • Fix: Introduce a local Result<T> = {ok:true; value:T} | {ok:false; error:string} and let callers rename value to url/filters at the call site.
    • kieran-typescript
  • packages/app/src/components/DBEditTimeChartForm/OnClickForm/OnClickDrawer.tsx:185 -- setValue and getValues are prop-drilled through ModeFields (which doesn't use them) into DashboardOnClickFields; the typed surface (UseFormSetValue<DrawerFormValues>) is also wider than the single field it actually touches.
    • Fix: Wrap the drawer body in FormProvider and have DashboardOnClickFields call useFormContext<DrawerFormValues>() directly, or hoist the field-array and pass a narrow onTargetDashboardChange callback.
    • maintainability, kieran-typescript
  • packages/app/src/hooks/useDashboardFilters.tsx:62 -- when useDashboardFilters re-emits filters to the URL it wraps keys via filtersToQuery(..., {stringifyKeys: true}) so the URL ends up with toString(ServiceName) IN ('x'); renderFilterTemplates emits bare keys; copying or refreshing a URL that has been through the dashboard once mismatches knownExpressions and the value lands in ignoredFilterExpressions while the dashboard renders unfiltered.
    • Fix: Strip a leading toString(...) wrapper from URL keys before matching against declared expressions, or stop emitting toString(...) wrappers in URL serialization (the onClick path's bare-key emission is the canonical contract).
    • adversarial
  • packages/app/src/hooks/useDashboardFilters.tsx:88 -- the hook returns an inline object literal, so ignoredFilterExpressions is a structural-only addition; the usePresetDashboardFilters mock had to be updated by hand, and other consumers that destructure-and-spread would silently drop new fields.
    • Fix: Export type UseDashboardFiltersResult = ReturnType<typeof useDashboardFilters> and update existing mockReturnValue(... as any) casts to use the named type so test mocks fail closed on shape drift.
    • kieran-typescript, api-contract
  • packages/common-utils/src/core/linkUrlBuilder.ts:53 -- when multiple templates share the same expression and render to the same value, the Set silently de-dupes; whitespace- and case-different renders ({{x}} vs {{ x }}, 'A' vs 'a') do not de-dupe.
    • Fix: Document the de-dupe semantics in the drawer help text, or normalize values (trim()) before adding to the Set so accidental whitespace doesn't produce duplicate IN entries.
    • correctness, testing, adversarial
🔵 P3 nitpicks (10)
  • packages/common-utils/src/types.ts:673 -- OnClickFilterTemplateSchema is a discriminated union of one variant; every construction site pays kind: 'expressionTemplate' as const ceremony and the narrowing in validateOnClickTemplate is tautological.
    • Fix: Drop the kind field until a second variant exists, or document the planned second variant in a one-line comment.
    • maintainability, kieran-typescript, api-contract
  • packages/common-utils/src/core/linkUrlBuilder.ts:41 -- the JSDoc on renderFilterTemplates says "Entries that share the same filter expression are merged", but the field is named expression.
    • Fix: Rename filterexpression in the doc comment.
  • packages/app/src/DBDashboardPage.tsx:1257 -- dashboardReady = !!dashboard?.id && ..., but local dashboards have dashboard.id === '', so the banner-latch effect never runs for the local builder path even if a shared link arrives with bad URL filters.
    • Fix: Treat isLocalDashboard && router.isReady as ready (with a sentinel id like 'local') so the banner can surface for local dashboards too.
    • julik-frontend-races
  • packages/app/src/DBDashboardPage.tsx:1277 -- banner dismissal lives in a per-component ref, so browser back/forward, route remounts, and A→B→A navigations re-show the same banner the user already dismissed.
    • Fix: Track dismissed dashboard ids in a stable client store (sessionStorage keyed by dashboard id, or a top-level Zustand slice) instead of the per-mount ref.
    • julik-frontend-races
  • packages/app/src/components/DBEditTimeChartForm/OnClickForm/OnClickTargetInputControlled.tsx:80 -- the Select's clear path calls field.onChange({mode:'id', id:''}) and onTargetChange?.(...) with id === ''; the empty value passes the TS type but violates OnClickTargetSchema's id: z.string().min(1), and downstream consumers silently no-op.
    • Fix: Return early when value is null/empty, or change the callback signature to (target: OnClickTarget | null) => void and let consumers handle the cleared case.
    • kieran-typescript
  • packages/app/src/components/DBEditTimeChartForm/OnClickForm/OnClickDrawer.tsx:242 -- validation failures in handleSubmit show inline TextInputControlled errors but no top-level "Apply rejected" notification, and toggling the segmented mode control via emptyDashboardOnClick() resets onClick.filters without warning.
    • Fix: Surface a Mantine notification when handleSubmit validation fails, and preserve the filters array when toggling between Search/Dashboard modes (or confirm before clearing).
    • adversarial
  • packages/app/src/DBDashboardPage.tsx:1267 -- the inline comment mixes a real invariant ("latched to prevent flash during nuqs/router race") with a TODO-shaped "Known limitation" that will rot the moment the limitation is fixed.
    • Fix: Keep the invariant text, move the limitation into a tracked issue or remove it once the P2 finding above lands.
    • maintainability
  • packages/app/src/searchFilters.tsx:332 -- URL filter conditions that don't match any IN/NOT IN/BETWEEN shape are silently dropped by parseQuery and never appear in ignoredFilterExpressions either, so the banner under-reports what "wouldn't apply".
    • Fix: Collect unparseable conditions and surface them in the same banner ("N URL filters could not be applied").
    • correctness
  • packages/common-utils/src/core/linkUrlBuilder.ts:135 -- where: encodeURIComponent(where) plus the URLSearchParams second encode pass produces double-encoded URLs; receivers handle both shapes via parseAsStringEncoded fallback, but any old client tab still loaded in a user's browser pre-parseAsStringEncoded will display literal %xx in the where input.
    • Fix: Note the deploy ordering (receiver must ship before producer) in the changeset; harmless once the rollout completes.
    • api-contract
  • packages/common-utils/src/core/linkUrlBuilder.ts:53 -- renderFilterTemplates calls filtersToQuery(state) without passing {stringifyKeys: false} explicitly; useDashboardFilters does pass {stringifyKeys: true}. The asymmetric call sites obscure which mode renderFilterTemplates chose.
    • Fix: Pass {stringifyKeys: false} explicitly so the two call sites read symmetrically.
    • kieran-typescript

Reviewers (8): correctness, security, maintainability, testing, kieran-typescript, adversarial, api-contract, julik-frontend-races.

Testing gaps:

  • validateOnClickTemplate with onClick.filters containing a valid/invalid/empty filter-template branch (P1 above).
  • renderFilterTemplates with non-string row values (object, array, number, boolean, null) — currently only happy-path strings are covered.
  • renderFilterTemplates Set de-dupe behavior when two templates share an expression and render the same value, and the whereTemplate + filters set-simultaneously interaction.
  • useDashboardFilters mixed scenario: some declared-with-value, some declared-but-unset, some ignored, all in the same URL.
  • parseQuery regression coverage for BETWEEN with non-numeric bounds, BETWEEN combined with IN ... AND ..., and unparseable conditions that should land in ignoredFilterExpressions.
  • Banner latch contract: "same dashboard id, new URL filters" stays-as-is is comment-only; dismissal durability across remounts and A→B→A navigation has no test pinning either intended or accidental behavior.
  • Round-trip coverage in packages/app/src/__tests__/searchFilters.test.ts for: empty string, trailing backslash, 3-5 consecutive backslashes, backslash-quote-backslash, Windows path with embedded quote (C:\Users\O'Brien\file), and a legacy single-backslash value that pre-PR URLs may have stored verbatim.
  • Type-level test (expectType<OnClickFilterTemplate[]>(...)) on FilterTemplateList's useFieldArray result so future schema changes break compilation instead of silently widening.

@github-actions github-actions Bot added review/tier-4 Critical — deep review + domain expert sign-off and removed review/tier-3 Standard — full human review required labels May 11, 2026
@pulpdrew

Copy link
Copy Markdown
Contributor Author

packages/app/src/components/DBEditTimeChartForm/OnClickForm/OnClickDrawer.tsx:115 — handleTargetChange's allTemplatesEmpty predicate only inspects template === '', so switching the target dashboard silently overwrites any row where the user has typed an expression but not yet a template.

This is intentional to ensure the expressions will change when changing between selected dashboards.

packages/app/src/components/DBEditTimeChartForm/OnClickForm/OnClickDrawer.tsx:118 — DashboardOnClickFields calls setValue('onClick.filters', newArray) on a path that FilterTemplateList owns via useFieldArray; RHF v7's internal fields[].id keys are not regenerated by bare setValue, so the keyed row list can desync, lose focus, or render stale Controller state after a target switch.

This has essentially no functional impact that I can find.

packages/app/src/components/DBEditTimeChartForm/OnClickForm/OnClickDrawer.tsx:111 — selected?.filters ?? [] conflates "target dashboard not yet in useDashboards cache" with "target has no filters", so picking an id before the query resolves wipes any rows the form already had.

The query will have loaded on app load, so the react query cache will always be hot - no user impact.

packages/app/src/components/DBEditTimeChartForm/OnClickForm/OnClickDrawer.tsx:242 — applyChanges wraps handleSubmit(onValid)() with no onInvalid handler; a row with an empty expression / template (or a template-mode target with template: '') fails the zod min(1) checks silently — Apply does nothing, no toast, drawer stays open, and per-field errors are easy to miss in a scrollable drawer.

There are per-field errors. Sure the drawer is scrollable but (a) is unlikely to be longer than screen height without a ton of filters and (b) the user will just scroll to find the error if there is one

packages/app/src/components/DBEditTimeChartForm/OnClickForm/OnClickTargetInputControlled.tsx:85 — When the target Select fires onChange(null), the controller writes { mode: 'id', id: '' }, which fails OnClickTargetSchema.id.min(1); combined with the silent-no-op Apply path the drawer appears dead.

Yeah, the user will have to select a new target or the form will be invalid.

packages/app/src/hooks/useOnClickLinkBuilder.ts:54 — If a user clicks a row before useDashboards / useSources resolves, the empty Set/Map produces a misleading Could not find dashboard with ID xxx notification rather than a "still loading" signal; users assume the configured dashboard was deleted.

The dashboard will only render if useDashboards has already loaded, no impact, and not related to these changes.

packages/common-utils/src/types.ts:680 — onClick (and the new filters array) is absent from the external dashboard tile schemas (packages/api/src/utils/zod.ts per-display externalDashboardChartConfigSchema plus the .transform that strips unknown fields) and from every mcpTileSchema in packages/api/src/mcp/tools/dashboards/schemas.ts; convertToExternalTileChartConfig / convertExternalTilesToInternal likewise drop the field both directions, so an agent calling hyperdx_save_dashboard cannot author on-click filter templates and hyperdx_get_dashboard cannot read them back.

MCP support is a followup, read the PR description, clanker

packages/app/src/DBDashboardPage.tsx:1277 — Banner-latch effect deps are [dashboard?.id, dashboardReady] with react-hooks/exhaustive-deps suppressed; if filterQueries hydrates after dashboardReady flips (cached dashboard + nuqs URL-parse lag, or transient parseAsJsonEncoded returning null), the ref latches against an empty ignoredFilterExpressions and the banner is permanently suppressed for the session even when the URL actually contained ignored filters.

Hallucination. The ref isn't latched based on the ignoredFilterExpressions, just when the user actually dismisses the banner.

packages/common-utils/src/filters.ts:11 — escapeString now doubles backslashes (\ → \\) and quotes (' → ''), but the downstream parser getBooleanOrUnquotedString in packages/app/src/searchFilters.tsx:47 only unescapes ''; useDashboardFilters round-trips through parseQuery → filtersToQuery (line 65) so any filter value containing a literal backslash (Windows path, regex literal) doubles its backslashes on every cycle until it stops matching the underlying rows.

Fixed.

packages/app/src/hooks/tests/useDashboardFilters.test.tsx — The new ignoredFilterExpressions derivation (the source of truth for the banner) has no direct unit test; the only mention is a mocked [] in usePresetDashboardFilters.test.tsx:112, so a regression in the Set classification passes the suite.
packages/app/tests/e2e/features/dashboard-table-linking.spec.ts:673 — The "ignored-filter warning banner is dismissable" E2E dismisses the banner and ends; it never triggers a dashboardReady toggle (dashboard save / refocus / refetch) after dismiss, so the very invariant lastLoadedIdForBannerRef exists to enforce is uncovered.

Added tests

@pulpdrew

Copy link
Copy Markdown
Contributor Author

packages/common-utils/src/core/linkUrlBuilder.ts:248 -- validateOnClickTemplate's new onClick.filters loop (calls validateTemplate(filter.template) on every expressionTemplate) has zero test coverage — invalid Handlebars in a filter template will not throw at save time, only at render.

Incorrect, invalid handlebars template on a filter will be validated while submitting the form.

packages/app/src/utils/queryParsers.ts:43 -- parseAsJsonEncoded<Filter[]>() does JSON.parse and casts to T with no runtime validation, so any URL filters= value that parses as JSON is typed as Filter[] regardless of actual shape; useDashboardFilters and DBSearchPage then iterate it without guarding {type, condition}.

Existing, not related to these changes.

packages/app/src/DBSearchPage.tsx:791 (pre-existing) -- filters: parseAsJsonEncoded<Filter[]>() reads attacker-controllable SQL fragments from the URL and forwards them through buildSearchChartConfig → renderWhereExpression → chSql${UNSAFE_RAW_SQL: filter.condition}; the /searchpage has no expression-allowlist gate the way/dashboards/[id] does, and this PR adds a new convenient producer of these URLs.

Existing, not related to these changes.

@kodiakhq kodiakhq Bot merged commit a36c5b1 into main May 11, 2026
18 of 19 checks passed
kodiakhq Bot pushed a commit that referenced this pull request May 28, 2026
…tile row click (#2321)

## Summary

When a dashboard table tile is configured with an `onClick` action ([#2140](#2140), [#2141](#2141), [#2146](#2146), [#2148](#2148)), the row click destination is now discoverable before the user commits to the click.

**What's new for users:**

- **Hover hint.** After ~250ms of hover, a small card above the row reads `Search HyperDX Logs` or `Open dashboard "API Latency Drilldown"` (or generic `Open in search` / `Open dashboard` when the target is templated or the named source / dashboard no longer exists).
- **Native browser behaviors.** The cell wrapper is now a real `<a href>`. Cmd-click opens the destination in a new tab, middle-click opens it in a new tab, right-click shows the browser context menu with "Open in New Tab" and "Copy Link Address", and the destination URL appears in the status bar on hover.
- **Keyboard navigation.** Tab focuses the first clickable cell with a visible focus ring; Enter activates it.

### Screenshots

Search action: hovering a row reveals the resolved source name.

| Light | Dark |
|---|---|
| ![Search onClick hover hint, light theme](https://raw.githubusercontent.com/hyperdxio/hyperdx/assets/pr-2321/onclick-search-hover.png) | ![Search onClick hover hint, dark theme](https://raw.githubusercontent.com/hyperdxio/hyperdx/assets/pr-2321/onclick-search-hover-dark.png) |

Dashboard action: hovering a row reveals the resolved dashboard name.

| Light | Dark |
|---|---|
| ![Dashboard onClick hover hint, light theme](https://raw.githubusercontent.com/hyperdxio/hyperdx/assets/pr-2321/onclick-dashboard-hover.png) | ![Dashboard onClick hover hint, dark theme](https://raw.githubusercontent.com/hyperdxio/hyperdx/assets/pr-2321/onclick-dashboard-hover-dark.png) |

**How the implementation changes:**

- `packages/app/src/HDXMultiSeriesTableChart.tsx`: cell wrapper goes from `<div role="link" tabIndex={0}>` plus manual `onClick` / `onAuxClick` / `onMouseDown` / `onKeyDown` handlers to a Next.js `<Link href>` with `prefetch={false}`. The HoverCard wraps the whole `<tr>` at the row body level so the hint position stays stable as the cursor moves across cells; cell-level mounting would flicker per column. Rows whose templates fail to resolve render as a real `<button type="button">` (not `<a href="#">`) so cmd-click / middle-click / right-click "Open in New Tab" can't silently open a meaningless new tab against a `#` fragment.
- `packages/app/src/components/DBTableChart.tsx`: drops the parent `onRowClick` cmd/middle-click branching; threads new `getRowAction` (or legacy `getRowSearchLink`) into Table.
- `packages/common-utils/src/core/linkUrlBuilder.ts`: new `describeOnClick({ onClick, sourceNamesById, dashboardNamesById }): string` helper returns the one-line hint, with an exhaustiveness check on the OnClick discriminator.
- `packages/app/src/hooks/useOnClickLinkBuilder.ts`: also builds `Map<id, name>` for sources and dashboards, computes the row-independent description once, and returns a per-row resolver of shape `{ url, description, onClickError? }`. Per-row results are memoized internally (WeakMap) so cells sharing a row don't rerun handlebars rendering. When a row's templates fail to resolve, `url` is `null` and `onClickError` fires the existing Mantine "Link error" toast on click.
- Focus ring uses the shared `styles/focus.module.scss` `focusRing` style.
- Cell wrappers carry `data-testid="dashboard-table-row-action"` on both branches; the e2e page object resolves by testid so future inline column links don't steal the click.
- The legacy `getRowSearchLink` drilldown path (used outside dashboards, e.g. the services dashboard) renders as a plain `<Link href>` without a HoverCard so behavior there is unchanged.

## Test plan

- [x] `make ci-lint` clean across all 3 workspaces.
- [x] `make ci-unit` clean. New `HDXMultiSeriesTableChart` component test covers both success and failure branches end to end: anchor / button shape, `onClickError` wiring, row-level HoverCard hint visibility, legacy `getRowSearchLink` fallback.
- [x] `describeOnClick` unit tests cover all 6 OnClickSchema discriminator + name-lookup combinations.
- [x] `useOnClickLinkBuilder` hook tests cover: no onClick configured returns null, resolved source name in description, resolved dashboard name in description, row resolution failure encodes as `url: null` + click handler fires notification, WeakMap caching shares results across cells of the same row.
- [x] `knip` clean (no new unused exports).
- [x] UI verified in the dev stack (screenshots above). Status bar shows the destination URL on the success branch; failure branch (a `<button>`) shows no status bar URL and clicking fires the existing red `Link error` toast. Right-click on a success anchor shows the native context menu; cmd-click and middle-click open in a new tab.
- [x] Both light and dark theme verified.
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