Skip to content

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

Open
pulpdrew wants to merge 4 commits into
mainfrom
drew/adhoc-filters-click-search
Open

pulpdrew wants to merge 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 16, 2026 6:59pm UTC
hyperdx-storybook Ready Ready Preview Oct 16, 2026 6:59pm 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] The PR appears safe to merge.

Summary

Search-tile cells and row side panels can update the ad hoc filters that apply to their source.

  • Search tile clicks update the ad hoc filters that reach them.
  • Include and exclude clicks update conditions in each target filter.
  • SQL ad hoc filter suggestions leave out DateTime fields.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[Click search cell] --> B[Check cell type and resolve expression]
  B --> C[Find filters for tile source]
  C --> D[Add or toggle conditions]
  D --> E[Save conditions in URL]
  E --> F[Refresh matching tiles]
Loading

Reviews (9) · 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: daf6b09

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 • 434 passed • 1 skipped • 1312s

Status Count
✅ Passed 434
❌ Failed 0
⚠️ Flaky 3
⏭️ 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

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 · 🟠 0 major · 🔵 3 minor

1 posted as inline comment(s) on the changed lines. 2 listed below.

Findings outside the changed lines

2 minor
  • 🔵 packages/app/src/hooks/useDashboardFilters.tsx:849 — Side panel says 'Added … to filters' even when applyAdhocAction rejects the value → DBRowJsonViewer.tsx:560 always shows a green "Added … to filters" toast after onPropertyAddClick. On a dashboard, that call now goes to applyAdhocAction, which can refuse the value: it shows notifyAdhocFilterFull or notifyAdhocConditionTooLong and returns without writing anything. So with a full filter, or a Body over the length limit, the user sees a success toast and a warning toast for the same click. Fix: have the handler report whether it applied (e.g. applyAdhocAction returns a boolean, passed through onPropertyAddClick), or have the dashboard path own all of its notifications so the viewer skips its success toast.
  • 🔵 packages/app/src/hooks/useDashboardFilters.tsx:855 — Merge reads conditions from render time, so every filter change rebuilds every tile handler → applyAdhocAction merges into the adhocConditionsByFilterId captured at render, then writes the result as a full replacement. Two writes inside one render can lose the first one. It also puts adhocConditionsByFilterId in the deps, so tileFilterHandlers (DBDashboardPage.tsx:2375) and every tile's searchRowContext get new identities on any filter value change, which contradicts the comment that says they stay stable. Fix: do the mergeAdhocConditions and limit check inside the setFilterValueEntries(prev => …) updater, reading the current conditions from parseDashboardFilterValues(prev), and drop adhocConditionsByFilterId from the deps.

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 critical issues found. No P0/P1 defects are introduced by this diff. All five prior Greptile P1s (DateTime, structured-cell, alias-quoting, and session-source value/format mismatches) are verified resolved in the current code, and the clicked-value path is confirmed SQL-injection-safe (values flow through escapeSqlString; keys are config/alias-derived, never cell data).

🟡 P2 -- recommended

  • packages/app/src/DBDashboardPage.tsx:2389 -- getTileFilterHandler returns a fresh closure on every call and is invoked inline in renderTileComponent, so onFilter gets a new identity each render; this invalidates the searchRowContext useMemo (deps [onFilter, source]) and the Tile content useMemo that now lists searchRowContext, defeating memoization for every search tile with an ad hoc filter target.
    • Fix: Stabilize the per-tile handler identity, e.g. memoize handlers keyed by tile/source or move targetIds resolution into applyAdhocAction so the closure can be hoisted.
🔵 P3 nitpicks (4)
  • packages/app/src/components/DBTable/adhocCellCondition.ts:38 -- columnTypeMap types type as optional while getAdhocCellCondition does isAdhocComparableDataType(columnType?.type ?? ''), and isAdhocComparableDataType('') returns true, so a missing CH type silently defaults to the comparable (DateTime-unsafe) branch; safe today only because the sole ad-hoc-target producer always populates type.
    • Fix: Return undefined when columnType?.type is absent (treat unknown type as non-comparable), or require type: string on the ad-hoc-target path.
    • api-contract, kieran-typescript
  • packages/app/src/hooks/useDashboardFilters.tsx:45 -- the generic filter-state hook imports notifyAdhocConditionTooLong/notifyAdhocFilterFull from a specific component folder (@/components/AdhocDashboardFilter/notifications), coupling core state logic to that component's location and adding a UI side-effect to the hook.
    • Fix: Move the notification helpers to a neutral utils module, or have applyAdhocAction return a result and let the component surface the toast.
  • packages/app/src/components/DBTable/adhocCellCondition.ts:11 -- parseDynamicScalar runs JSON.parse then String(parsed), losing precision for Dynamic/JSON integer scalars beyond Number.MAX_SAFE_INTEGER (e.g. large UInt64), so the produced filter value can differ from the query-returned string and never match.
    • Fix: Preserve the raw scalar text for large-integer Dynamic values instead of round-tripping through a JS number.
  • packages/app/src/DBDashboardPage.tsx:679 -- the searchRowContext bridge maps onPropertyAddClick's 'only' | 'include' | 'exclude' action vocabulary onto AdhocConditionAction's 'include' | 'exclude' | 'add', collapsing both 'only' and undefined to 'add'; harmless today but a latent coherence trap if the popover ever emits 'only' for an ad-hoc tile.
    • Fix: Handle 'only' explicitly or document that it is intentionally treated as 'add'.

Pre-existing (not introduced by this diff)

  • packages/common-utils/src/filters.ts:972 -- ad hoc condition key is rendered verbatim as toString(${key}) with only length validation, so a crafted filters= URL param can place arbitrary SQL in key; confirmed present in base commit and not reachable via the new click path (which only feeds config-derived keys).
  • packages/app/src/components/DBRowTable.tsx:1297 -- (cell.column.columnDef.meta as any)?.column erases type checking at the spot that now determines the ad hoc filter key; pre-existing pattern reused, but this feature widens its blast radius.

Agent-Native Gap (advisory)

  • The click-to-filter merge primitives (mergeAdhocConditions, getAdhocFilterTargetsForTile) correctly live in shared common-utils, but the apply/persist path (applyAdhocAction) is React-UI-only and the external/MCP schema (externalDashboardSavedFilterValueSchema) omits the adhoc filter-value variant, so an agent cannot persist or query-under ad hoc conditions the way a user click does. Pre-existing scope; a follow-up could add the adhoc branch to the external schema.

Reviewers (9): security, testing, maintainability, project-standards, api-contract, kieran-typescript, previous-comments, agent-native, learnings-researcher. The correctness and adversarial reviewers did not return before synthesis; their scope (value/format round-tripping, toggle/undo logic, filter targeting, alias/column resolution) was independently verified during this review and cross-covered by the previous-comments reviewer, which re-checked and confirmed resolution of the prior correctness bugs.

Testing gaps:

  • applyAdhocAction limit/too-long branches assert only unchanged state, not that notifyAdhocConditionTooLong/notifyAdhocFilterFull fire.
  • No test exercises getAdhocCellCondition with an undefined type (the ?? ''-to-comparable path) or getCellFilter's non-ad-hoc fallback preserving existing include/exclude behavior.
  • applyAdhocAction is only tested with action: 'include'; the 'add' (no-undo) path is covered only indirectly via mergeAdhocConditions unit tests.

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/hooks/useDashboardFilters.tsx
Comment thread packages/app/src/DBDashboardPage.tsx
Comment thread packages/app/src/DBDashboardPage.tsx
Base automatically changed from drew/adhoc-filter-broadcast-sql to main October 16, 2026 17:22
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

This branch was successfully deployed

2 active deployments
Preview – hyperdx-oss — daf6b09a Deployed Oct 16, 2026 by vercel[bot]
Preview – hyperdx-storybook — daf6b09a Deployed Oct 16, 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.

1 participant