Skip to content

feat: Support click-to-adhoc-filter on search tiles - #3380

Merged
pulpdrew merged 4 commits into
mainfrom
drew/adhoc-filters-click-search
Oct 19, 2026
Merged

pulpdrew merged 4 commits into
mainfrom
drew/adhoc-filters-click-search

Conversation

@pulpdrew

@pulpdrew pulpdrew commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Support click-to-filter for search tiles on dashboards with adhoc filters.

The existing search table's click-to-filter functionality is re-used. The sidebar add filter button works as well.

Screenshots or video

Screen.Recording.2026-10-16.at.8.40.22.AM.mov

How to test on Vercel preview

  • Create a dashboard
  • Add an adhoc filter and a search tile on the same source
  • Click some cell values to filter!

References

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

@pulpdrew
pulpdrew added this pull request to stack #3342 October 15, 2026 16:37
@vercel

vercel Bot commented Oct 15, 2026 •

Copy link
Copy Markdown

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

Project Deployment Actions Updated
hyperdx-oss Ready Ready Preview Oct 19, 2026 11:47am UTC
hyperdx-storybook Ready Ready Preview Oct 19, 2026 11:47am UTC

Request Review

@greptile-apps

greptile-apps Bot commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerView in GreptileConfidence Score: 5/5

[Medium impact] No new blocking issue was established; the PR appears safe to merge with the accepted chained-alias limitation.

Summary

Adds click-to-filter actions to dashboard search tiles and connects sidebar actions to the matching ad hoc filters.

  • Search tile clicks update the ad hoc filters that reach them.

The previous-review ancestry and merge-base checks did not pass, so this review used the full PR diff. The supplied previous threads have no Comment numbers.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[Search cell or sidebar action] --> B[Find matching ad hoc filters]
  B --> C[Merge and check conditions]
  C --> D[Write filter values to URL]
  D --> E[Apply conditions to matching tiles]
Loading

Reviews (11) · Last reviewed commit: "fix: Fix tile onClick handler stability" · Reviewed by Greptile

Comment thread packages/app/src/components/DBRowTable.tsx Outdated
Comment thread packages/app/src/components/DBRowTable.tsx Outdated
@pulpdrew
pulpdrew force-pushed the drew/adhoc-filters-click-search branch from 342f2df to 36b05ac Compare October 15, 2026 20:00
@changeset-bot

changeset-bot Bot commented Oct 15, 2026 •

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 82fb40b

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

@github-actions

github-actions Bot commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

Duplicate code analysis

No new duplicate code introduced by this PR.


jscpd compared this branch against origin/main
across app, api and common-utils. Only new clones are listed; the codebase
currently holds 125 in total (1.69%).
Run yarn dupes locally to reproduce, or yarn dupes:all for the full picture.

Comment thread packages/app/src/DBDashboardPage.tsx Outdated
@github-actions

github-actions Bot commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

E2E Test Results

✅ All tests passed • 437 passed • 1 skipped • 1333s

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

Tests ran across 4 shards in parallel.

View full report →

Comment thread packages/app/src/components/DBRowTable.tsx Outdated
@github-actions

github-actions Bot commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

🟡 Tier 3 — Standard

Introduces new logic, modifies core functionality, or touches areas with non-trivial risk.

Why this tier:

  • Diff size: 440 production lines changed (Tier 2 max: < 250)
  • Cross-layer change: touches frontend (packages/app) + shared utils (packages/common-utils)

Review process: Full human review — logic, architecture, edge cases.
SLA: First-pass feedback within 1 business day.

Stats
  • Production files changed: 12
  • Production lines changed: 440 (+ 885 in test files, excluded from tier calculation)
  • Branch: drew/adhoc-filters-click-search
  • Author: pulpdrew

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

);

// `source` keeps the handler off rows from other sources, e.g. a linked trace
const searchRowContext = useMemo<RowSidePanelContextProps | undefined>(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟠 major — Side panel's surrounding-context table writes unshaped cells (DateTime, Map, aliases) into ad hoc filters

searchRowContext reaches every table under the tile's RowSidePanelContext, not just the tile's own table. DBRowSidePanel passes it down to ContextSubpanel (components/ContextSidePanel.tsx:313), which renders a separate DBSqlRowTable with the source's default select and without isAdhocFilterTarget. That table skips getAdhocCellCondition. Clicking include on its Timestamp cell adds Timestamp = '2026-…Z'. Per isAdhocComparableDataType, that never equals toString(Timestamp), so every tile on the source goes empty. A Map cell or an aliased column also produces a condition that can't match or that refers to a missing column. Fix: put the shaping in the context itself, e.g. a flag on RowSidePanelContextProps that DBRowTableFieldWithPopover reads, instead of a prop on one table. Or drop onPropertyAddClick for nested tables.

Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I tried to reproduce this but couldn't

const searchRowContext = useMemo<RowSidePanelContextProps | undefined>(
() =>
onFilter && source && (isLogSource(source) || isTraceSource(source))
? {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 minor — Side panel "Add to Filters" removes an existing ad hoc condition while the toast says "Added"

DBRowJsonViewer.tsx:553-563 calls onPropertyAddClick with no action and then always shows Added "key = value" to filters. Here, no action is mapped to include. mergeAdhocConditions then toggles the condition off when it already exists, e.g. after a cell click already included ServiceName = frontend. The user sees "Added" while the condition disappears. Fix: map a missing action to a non-toggling add (skip entries already applied), or only toggle when the action comes from the cell popover.

Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond

@github-actions

github-actions Bot commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

PR Review

If you are a coding agent acting for the author, read this first.
Nothing below blocks merge automatically; a maintainer decides. What they will expect:

  • 🔴 critical or 🟠 major in code this PR adds or changes: fix before asking for review.
  • 🔵 minor in code this PR changes: your call. Fix if small, otherwise reply.
  • Anything about surrounding code, or asking you to widen the change (hoist a helper,
    dedupe with another file, fix other call sites): never fix here, whatever the
    severity. Reply with one sentence; if it is critical, say so plainly so a human sees it.

One commit per review round. After two rounds, stop and ask a maintainer to review
scope rather than addressing more automated findings. Full rule: AGENTS.md.

3 finding(s): 🔴 0 critical · 🟠 1 major · 🔵 2 minor

3 posted as inline comment(s) on the changed lines.


Severity is the reviewer's own estimate and is used for ordering, not filtering. No finding blocks merge automatically; a maintainer decides.

@github-actions

github-actions Bot commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

Deep Review

✅ No P0/P1 issues found. The prior P1 review comments on this PR (unquoted physical column names, DateTime value/toString mismatch, structured Array/Map cells) all appear addressed in the current diff via quoteResultColumnNameIfNeeded, isAdhocComparableDataType, and the isJSDataTypeScalar gating in getAdhocCellCondition; the multi-level SQL-alias limitation was explicitly accepted by the author as an out-of-scope, recoverable trade-off.

🟡 P2 -- recommended

  • packages/app/src/hooks/useDashboardFilters.tsx:347 -- applyAdhocAction computes the merged conditions from adhocConditionsByFilterId read out of the render closure and then has setAdhocConditionsByFilterId write that fully-computed list, so two cell clicks that land before a re-render both merge against the same stale base and the second write overwrites the first click's condition rather than stacking on it.
    • Fix: Derive the current conditions inside the setFilterValueEntries functional updater (parse prev) so the merge always reads the latest persisted state instead of the closure snapshot.
🔵 P3 nitpicks (3)
  • packages/app/src/components/DBRowTable.tsx:409 -- columnTypeMap is typed { _type; type?: string } (optional) on the RawLogTable prop while selectColumnMapWithoutAdditionalKeys produces a required type: string; since getAdhocCellCondition falls back to isAdhocComparableDataType('') (which returns true) when type is absent, a future caller that omits type would silently re-enable filtering on DateTime/structured columns.
    • Fix: Make the prop's type required to match the producer, or treat a missing type as non-comparable in getAdhocCellCondition.
  • packages/common-utils/src/filters.ts:956 -- isAdhocComparableDataType excludes only DateTime via /\bDateTime/; plain Date columns remain comparable, and if the table renders a Date cell in a form that differs from ClickHouse toString(Date), include/exclude would mis-match rows the same way DateTime did.
    • Fix: Confirm Date rendering matches toString(Date), and broaden the exclusion to cover Date if it does not.
  • packages/app/src/components/DBTable/adhocCellCondition.ts:10 -- parseDynamicScalar runs clicked Dynamic values through JSON.parse + String(...), which can lose precision for integers beyond Number.MAX_SAFE_INTEGER and coerce booleans, yielding a saved value that no longer equals toString(key).
    • Fix: For Dynamic scalars, strip the JSON quoting without numeric re-parsing (or guard large-integer/boolean cases) so the stored value round-trips to the ClickHouse text form.

Reviewers: correctness, security, previous-comments, maintainability, api-contract (dimensions synthesized from direct diff analysis).

Testing gaps: mergeAdhocConditions has unit coverage for toggle/undo and add, but no test exercises the rapid read-modify-write sequence through applyAdhocAction that the P2 race describes; consider a test asserting two sequential applyAdhocAction calls accumulate both conditions.

No finding blocks merge automatically. A maintainer will expect P0/P1 findings in code this PR changes to be fixed; P2/P3 are your call -- fix or reply. Never fix findings about surrounding code here; reply instead. Do not widen the PR. How to respond

Comment thread packages/app/src/DBDashboardPage.tsx
Comment thread packages/app/src/DBDashboardPage.tsx
Comment thread packages/app/src/DBDashboardPage.tsx Outdated
Comment thread packages/app/src/components/DBTable/adhocCellCondition.ts
Comment thread packages/app/src/hooks/useDashboardFilters.tsx
Comment thread packages/app/src/DBDashboardPage.tsx
Comment thread packages/common-utils/src/dashboardFilterValues.ts
Comment thread packages/app/src/DBDashboardPage.tsx

@karl-power karl-power left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

? {
// Only the cell popover passes an action; the side panel's
// "Add to filters" passes none and shouldn't toggle a condition off.
onPropertyAddClick: (key, value, action) =>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 minor — The side panel's "Add to Filters" skips the DateTime guard that table cells use

Table cells go through getAdhocCellCondition, which never offers a filter on a DateTime cell. The side panel does not. DBRowJsonViewer.tsx:503-512 only blocks fields named exactly Timestamp and TimestampTime, and this handler forwards whatever key and value it receives. So clicking "Add to Filters" on any other DateTime column (for example a custom EventTime or ObservedTimestamp) adds a condition like toString(EventTime) = '2026-10-15T12:00:00Z'. That condition can never match, and it empties every tile the filter reaches. Fix: in the side panel path, check the field's ClickHouse type with isAdhocComparableDataType and drop the action for DateTime fields, the same as getAdhocCellCondition does.

Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond

// Built once per tile list so each tile's `onFilter` keeps its identity
// across renders; a fresh closure would rebuild the tile's row-panel context
// and re-render every cell in its search table.
const tileFilterHandlers = useMemo(() => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 minor — PromQL, raw SQL and multi-entry targeting are built but nothing in this PR can use them

Only the Search tile passes onFilter on (as searchRowContext), and Search tiles are always builder configs on Log, Trace or Session sources. Every call also passes exactly one entry. That leaves several things unreachable in this PR: the PromQL branch and the raw SQL $__filters branch of getAdhocFilterTargetsForTile, the multi-entry "some already apply" path of mergeAdhocConditions, and the handlers tileFilterHandlers builds for every non-Search tile. Fix: either limit tileFilterHandlers to DisplayType.Search tiles and drop the PromQL and raw SQL branches until a chart tile actually uses them, or wire them up in this PR.

Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond

.filter(
field =>
(field.path.length > 1 || isJSDataTypeScalar(field.jsType)) &&
isAdhocComparableDataType(field.type),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 minor — DateTime keys are now missing from the condition editor's key suggestions, though conditions on them still work

The reason given (query output is ISO UTC, so it never equals toString()) applies to values clicked in the table. It does not apply to conditions typed in the editor. The editor offers LIKE and =~, and a condition such as Timestamp LIKE '2026-10-15%', or = with toString's YYYY-MM-DD hh:mm:ss format, matches correctly. With this change, users have to type DateTime keys by hand. Fix: keep the isAdhocComparableDataType filter in the click-to-filter path (getAdhocCellCondition) and remove it from getSqlSourceKeys and getSqlSourceColumnKeys. If DateTime values are the real concern, filter only the =/!= value suggestions.

Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond


const next = new Map<string, AdhocFilterCondition[]>();
for (const filter of targets) {
const merged = mergeAdhocConditions(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟠 major — Each target filter decides toggle-off on its own, so an include click can move a condition instead of removing it

Say ad hoc filters A and B both broadcast to the tile, and A already holds ServiceName = api (added in the editor) while B is empty. Clicking include on api makes A toggle the condition off and B add it, so the dashboard is still filtered on api. Decide once across all targets whether the entries are already applied (for example, applied in every target), then apply the same include/remove result to each filter. Add a hook test where only one of the targets already has the condition.

Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond

continue;
}
next.set(filter.id, merged);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 minor — Side panel "Add to filters" shows a success toast even when the condition was rejected

DBRowJsonViewer.tsx:560 shows a green "Added … to filters" toast as soon as it calls onPropertyAddClick. When applyAdhocAction rejects the condition (notifyAdhocFilterFull or notifyAdhocConditionTooLong), the user sees that yellow warning next to the green success toast. Have the handler report whether anything was applied and show the success toast only then, or skip the generic toast when the context is an ad hoc filter target (isAdhocFilterTarget).

Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond

) {
return [];
}
const isPromql = isPromqlSavedChartConfig(config);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 minor — PromQL, raw SQL and multi-entry paths are built and tested but nothing uses them

onFilter is only used through searchRowContext, which feeds Search tiles (builder config, SQL source kinds) and always passes a single entry. So the PromQL branch, the raw-SQL $__filters branch, multi-entry merging in mergeAdhocConditions, and the handlers built for every non-search tile in tileFilterHandlers (DBDashboardPage.tsx:2390) never run. Either cut them back to the Search case or land them with the chart click-to-filter change that will call them.

Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond

@pulpdrew
pulpdrew merged commit 3e9185d into main Oct 19, 2026
28 of 29 checks passed

This branch was successfully deployed

2 active deployments
Preview – hyperdx-oss — 82fb40b7 Deployed Oct 19, 2026 by vercel[bot]
Preview – hyperdx-storybook — 82fb40b7 Deployed Oct 19, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review/tier-3 Standard — full human review required

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants