Skip to content

feat: add dependent adhoc filters - #3585

Merged
kodiakhq[bot] merged 4 commits into
mainfrom
aaron/adhoc-filter-values-condition
Nov 9, 2026
Merged

kodiakhq[bot] merged 4 commits into
mainfrom
aaron/adhoc-filter-values-condition

Conversation

@knudtty

@knudtty knudtty commented Nov 6, 2026

Copy link
Copy Markdown
Contributor

Summary

Adds dependent fields for adhoc filters. This lays the groundwork to support source filters.

Screenshots or video

Notice the only value fetched when scoped to ServiceName = 'hdx-oss-dev-app' is the one scoped to the app service.

export-1793985985388.mp4

References

  • Linear Issue: Related to HDX-5739

@github-actions github-actions Bot added the review/tier-3 Standard — full human review required label Nov 6, 2026
@github-actions

github-actions Bot commented Nov 6, 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: 468 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: 15
  • Production lines changed: 468 (+ 794 in test files, excluded from tier calculation)
  • Branch: aaron/adhoc-filter-values-condition
  • Author: knudtty

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

@github-actions

github-actions Bot commented Nov 6, 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 110 in total (1.21%).
Run yarn dupes locally to reproduce, or yarn dupes:all for the full picture.

Comment thread packages/common-utils/src/types.ts Resolved
@github-actions

github-actions Bot commented Nov 6, 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.

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

2 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 Nov 6, 2026 •

Copy link
Copy Markdown
Contributor

E2E Test Results

✅ All tests passed • 464 passed • 1 skipped • 1443s

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

Tests ran across 4 shards in parallel.

View full report →

@github-actions

github-actions Bot commented Nov 6, 2026 •

Copy link
Copy Markdown
Contributor

Deep Review

✅ No critical issues found. No P0/P1 ship-blockers: the where/conditionSql plumbing reuses the existing authenticated dashboard-authoring SQL trust model (variables expanded before render, injected as UNSAFE_RAW_SQL exactly as chart where clauses already are), condition-scoped cache keys retain connectionId and carry a TTL to avoid pile-up, and the conditioned path falls through to the row-policy-covered source scan rather than the rollup. The notes below are recommended/minor.

🟡 P2 -- recommended

  • packages/app/src/hooks/useMetadata.tsx:309 -- useMultipleAllFields catches each table connection's fetch failure, logs a warning, and returns empty fields/columns, so the query never rejects; the newly threaded keys.error (consumed as keysErrorMessage in the condition editor) stays null and ClickHouse failures from an invalid condition surface as an empty key dropdown with no message, while the values path (useMultipleGetKeyValues) does show its error — an asymmetry that makes the new key-error UI effectively dead.
    • Fix: Propagate the caught per-connection error through useMultipleAllFields so useSqlSourceKeys can expose it, or re-throw once all connections fail so keysErrorMessage renders consistently with the values path.
🔵 P3 nitpicks (3)
  • packages/app/src/components/AdhocDashboardFilter/useAdhocFilterOptions.ts:284 -- useSqlSourceValues reads suggestionsWhere?.where ?? '' without trimming, whereas useSqlSourceKeys only applies the condition when where.trim() is non-empty, so a whitespace-only condition is passed through on the values path but dropped on the keys path.
    • Fix: Trim where in useSqlSourceValues so keys and values apply the condition identically.
  • packages/app/src/components/AdhocDashboardFilter/AdhocDashboardFilter.tsx:215 -- the add popover is gated by opened={isAdding && !isDisabled}, but isAdding is never reset when the filter transitions to disabled (e.g. a referenced required filter is cleared mid-interaction), so re-enabling it can reopen the popover unexpectedly.
    • Fix: Reset isAdding to false when isDisabled becomes true.
  • packages/app/src/hooks/useMetadata.tsx:310 -- condition: conditions?.[i] couples the optional conditions array to tableConnections positionally; the two are built from the same sources.map today, but the implicit index alignment is a latent trap if either list is ever filtered or reordered independently.
    • Fix: Key conditions by table-connection identity, or add an assertion/comment that the arrays must stay index-aligned.

Reviewers: orchestrator direct diff analysis. The full multi-agent fan-out (correctness, security, adversarial, performance, api-contract, testing, maintainability, kieran-typescript, frontend-races, project-standards, agent-native, learnings, previous-comments) was dispatched, but structured results were not available at synthesis time; findings above reflect the orchestrator's own verified reading of the diff and are graded conservatively.

Testing gaps: No test asserts that a backend key-lookup failure surfaces via keysErrorMessage (the swallowed-error path above); consider an integration test covering an invalid condition producing a visible key-input error.

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

@knudtty
knudtty force-pushed the aaron/adhoc-filter-values-condition branch from ef36f29 to cef7c74 Compare November 6, 2026 17:58
Comment thread packages/common-utils/src/types.ts Resolved
Comment thread packages/common-utils/src/types.ts Resolved
@kodiakhq
kodiakhq Bot requested a review from a team as a code owner November 9, 2026 14:20
@vercel

vercel Bot commented Nov 9, 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 Nov 9, 2026 2:25pm UTC
hyperdx-storybook Ready Ready Preview Nov 9, 2026 2:25pm UTC

Request Review

@kodiakhq
kodiakhq Bot merged commit 142c9cc into main Nov 9, 2026
27 of 28 checks passed
@kodiakhq
kodiakhq Bot deleted the aaron/adhoc-filter-values-condition branch November 9, 2026 14:30

This branch was successfully deployed

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

Labels

automerge review/tier-3 Standard — full human review required

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants