Skip to content

feat: preserve compatible filters when switching sources#2314

Merged
kodiakhq[bot] merged 2 commits into
mainfrom
karl/preserve-compatible-filters-when-switching-sources
May 27, 2026
Merged

feat: preserve compatible filters when switching sources#2314
kodiakhq[bot] merged 2 commits into
mainfrom
karl/preserve-compatible-filters-when-switching-sources

Conversation

@karl-power
Copy link
Copy Markdown
Contributor

@karl-power karl-power commented May 20, 2026

Summary

Switching the active source on the /search page used to clear all sidebar filters unconditionally. This was added (HDX-1176, HDX-1258, HDX-2545) to prevent broken queries when columns from the old source didn't exist on the new one, but users keep asking for filters to survive across compatible sources.

This PR revisits that tradeoff with a per-filter rule: when the source changes, keep filters whose root column exists on the new source's schema, drop the rest, and surface a yellow toast naming the count of dropped filters.

  • searchFilters.tsx — new retainFiltersByColumns(allowed: Set<string>) helper on useSearchPageFilterState. Returns the keys of dropped filters and syncs URL state via updateFilterQuery only when something actually changed. Root-column extraction handles both top-level columns (ServiceName) and nested JSON/Map keys (LogAttributes.user.id → root LogAttributes), with an exact-match fallback for the rare case of a column whose name contains dots.
  • DBSearchPage.tsx — replaced the unconditional clearAllFilters() call in the source-change effect with a pendingFilterReconcileRef. A follow-up effect runs once useColumns for the new source resolves and calls retainFiltersByColumns. The deep-link "open this trace by ID" path (onDirectTraceSourceChange) is intentionally untouched — it builds a fresh SQL where clause from scratch and is a different flow.
  • Toastnotifications.show({ color: 'yellow', message: 'N filter(s) didn't apply to this source and were removed.' }) fires only when at least one filter was dropped.

Coverage: 6 unit tests for retainFiltersByColumns (empty, all-kept, nested keys, all-dropped, mixed, passthrough preservation) and 3 Playwright E2E tests (compatible → preserved + no toast, mixed → toast + URL keeps shared filter, all-dropped → toast). Existing mocks in ActiveFilterPills.test.tsx and DBSearchPage.directTrace.test.tsx were updated for the new hook return shape.

Screenshots or video

Screen.Recording.2026-05-20.at.16.14.59.mov

How to test on Vercel preview

Preview routes: /search

Steps:

  1. Open /search and select a logs source from the source dropdown.
  2. In the sidebar, open the ServiceName filter group and check any value.
  3. Open the SeverityText filter group and check info.
  4. Switch the source dropdown to a traces source.
  5. Verify a yellow toast appears reading 1 filter didn't apply to this source and was removed. (the SeverityText filter is dropped because traces lack that column).
  6. Re-open the ServiceName filter group and confirm the URL's filters= param still contains the ServiceName filter.
  7. Switch back to the logs source. Verify no toast appears (the ServiceName filter is still compatible) and the URL retains the filter.

References

  • Linear Issue: Closes HDX-4254
  • Related PRs: —

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented May 20, 2026

🦋 Changeset detected

Latest commit: 8466601

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@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

@vercel
Copy link
Copy Markdown

vercel Bot commented May 20, 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 27, 2026 8:01am

Request Review

@karl-power karl-power requested review from a team and knudtty and removed request for a team May 20, 2026 14:21
@github-actions github-actions Bot added the review/tier-2 Low risk — AI review + quick human skim label May 20, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 20, 2026

🔵 Tier 2 — Low Risk

Small, isolated change with no API route or data model modifications.

Why this tier:

  • Standard feature/fix — introduces new logic or modifies core functionality

Review process: AI review + quick human skim (target: 5–15 min). Reviewer validates AI assessment and checks for domain-specific concerns.
SLA: Resolve within 4 business hours.

Stats
  • Production files changed: 2
  • Production lines changed: 95 (+ 280 in test files, excluded from tier calculation)
  • Branch: karl/preserve-compatible-filters-when-switching-sources
  • Author: karl-power

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

@github-actions
Copy link
Copy Markdown
Contributor

PR Review

✅ No critical issues found.

A few minor observations (non-blocking):

  • ℹ️ Reconcile effect depends on the entire searchFilters object → effect re-runs on every render of useSearchPageFilterState (the hook returns a fresh object each render and retainFiltersByColumns is recreated whenever filters changes). The pendingFilterReconcileRef/!watchedSourceColumns guard makes this a cheap no-op, but you could depend on searchFilters.retainFiltersByColumns directly to make the intent clearer.
  • ℹ️ If useColumns errors or never resolves for the new source, pendingFilterReconcileRef is never cleared and filters are silently left in place (mild behavior regression vs. the old unconditional clear). Probably acceptable since the worst case is a stale filter the user can clear manually, but worth noting — a fallback timeout or "clear on error" path would close the gap.
  • ℹ️ Empty watchedSourceColumns ([] returned successfully) would drop every filter and show "N filters didn't apply" — unlikely in practice but a corner case the tests don't cover. Consider guarding on watchedSourceColumns.length > 0 if you want to be defensive.
  • ℹ️ Root-column extraction uses key.indexOf('.') with an exact-match fallback. This is correct for the current parseKeyPath output (top-level columns never contain dots; bracketed keys join as Root.nested.path). Worth a brief comment pointing back to parseKeyPath so the invariant is discoverable from searchFilters.tsx.
  • ℹ️ Toast message names a count but not which filters were dropped — fine per the PR description, just flagging that returning dropped from retainFiltersByColumns is currently used only for the count; a future iteration could surface the names.

Implementation looks well-scoped, with thorough unit coverage (including passthrough preservation) and three E2E scenarios. Direct-trace flow correctly left untouched.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 20, 2026

E2E Test Results

All tests passed • 191 passed • 3 skipped • 1288s

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

Tests ran across 4 shards in parallel.

View full report →

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 20, 2026

Deep Review

✅ No critical issues found.

🟡 P2 -- recommended

  • packages/app/src/DBSearchPage.tsx:1158 -- Reconcile gate checks watchedSourceColumns for truthiness but not .length > 0, so a successful-but-empty metadata response silently drops every filter and fires a misleading "didn't apply to this source" toast.
    • Fix: Tighten the gate to watchedSourceColumns && watchedSourceColumns.length > 0 so a degraded metadata fetch doesn't wipe user-authored filter state.
    • correctness, testing, maintainability, project-standards, adversarial
  • packages/app/src/hooks/useMetadata.tsx:91 -- useColumns queryKey is ['useMetadata.useColumns', { databaseName, tableName }] and omits connectionId; two sources sharing databaseName/tableName on different connections collide in the cache and the reconcile path consumes the wrong source's columns, dropping legitimate filters or retaining incompatible ones.
    • Fix: Include connectionId in the queryKey so cross-connection switches don't reconcile against stale schema; audit useTableMetadata for the same shape.
    • correctness, testing, kieran-typescript, julik-frontend-races, adversarial
  • packages/app/src/DBSearchPage.tsx:1129 -- debouncedSubmit() schedules a chart query 1s after source change, so when useColumns cold-fetches in over a second the chart fires with the old filter list against the new source and surfaces a ClickHouse "unknown column" error before reconcile lands.
    • Fix: Skip debouncedSubmit() while pendingFilterReconcileRef is set and let the reconcile path's own filter-change submit commit source + trimmed filters atomically.
    • correctness, julik-frontend-races, adversarial
  • packages/app/src/searchFilters.tsx:549 -- updateFilterQuery(kept) re-emits parsedQuery.passthroughFilters unchanged, so unparseable Lucene and raw SQL filters referencing columns missing from the new schema survive the reconcile while the toast claims filters were cleaned.
    • Fix: Either drop passthrough entries whose referenced columns are absent from allowedColumnNames, or explicitly note in the toast that raw SQL filters were not analyzed.
    • correctness, julik-frontend-races, adversarial
  • packages/app/src/DBSearchPage.tsx:1101 -- The source-change effect fires on any watchedSource transition, including the initial form default settling from undefined to defaultSourceId once sources loads, so deep links carrying filters= but no source= get reconciled + toasted on first paint even though the user never switched sources.
    • Fix: Defer prevSourceRef initialization until inputSourceObjs has loaded, or gate pendingFilterReconcileRef assignment on a real user-intent signal (the SourceSelect onChange, not any useWatch transition).
    • adversarial
  • packages/app/src/DBSearchPage.tsx:1157 -- The reconcile orchestration (pendingFilterReconcileRefuseColumnsretainCompatibleFilters ↔ toast) has no unit or integration test; coverage stops at the pure retainFiltersByColumns helper.
    • Fix: Add a React Testing Library test that drives a source change, waits for the mocked useColumns to resolve, and asserts the ref-gated effect runs exactly once with the correct toast count and resulting filter state.
    • testing, maintainability, project-standards
  • packages/app/tests/e2e/features/search/search-filters.spec.ts:149 -- expect(page).toHaveURL(/filters=.*ServiceName/i) matches any URL whose filters= param contains ServiceName as a substring anywhere, so a residual filter or stale URL state would also pass.
    • Fix: Assert against the decoded filters payload (or interpolate the chosen serviceName value into the regex) so the assertion actually pins the preserved filter.
    • testing
  • packages/app/src/__tests__/DBSearchPage.directTrace.test.tsx:131 -- The new useColumns mock returns data: undefined, which suppresses the reconcile effect entirely in this suite, so the direct-trace + reconcile interaction is untested and a future regression coupling those paths won't be caught.
    • Fix: Add a sibling test that mocks useColumns with a realistic ColumnMeta[] and asserts reconcile does not misfire during direct-trace navigation.
    • testing
  • packages/app/src/DBSearchPage.tsx:1142 -- Two useEffects coordinate via pendingFilterReconcileRef to defer reconcile until columns load, and retainCompatibleFilters is wrapped in useStableCallback only to mask a missing memoization in useSearchPageFilterState's returned hook shape; the dependency graph is hard to follow.
    • Fix: Encapsulate the source-switch + reconcile lifecycle in a single custom hook with explicit state, or memoize the retainFiltersByColumns reference upstream so the wrapper isn't load-bearing.
    • maintainability, kieran-typescript
  • packages/app/src/hooks/useMetadata.tsx:99 -- The ...options spread overrides the inner enabled: !!databaseName && !!tableName && !!connectionId default; the new caller passes enabled: !!watchedSourceObj, so a source with empty from.databaseName/tableName still triggers getColumns with empty strings.
    • Fix: AND-combine the guards (enabled: !!databaseName && !!tableName && !!connectionId && (options?.enabled ?? true)) or tighten the DBSearchPage caller to gate on the actual from fields.
    • kieran-typescript
🔵 P3 nitpicks (8)
  • packages/app/src/DBSearchPage.tsx:1086 -- pendingFilterReconcileRef reads like a boolean but stores string | null.
    • Fix: Rename to pendingReconcileSourceIdRef (or similar) so name conveys both type and intent.
  • packages/app/src/DBSearchPage.tsx:1119 -- Ref is set to watchedSource ?? null but the reconcile gate compares directly against watchedSource (typed string | undefined); a null === undefined mismatch would silently skip reconcile.
    • Fix: Normalize both sides ((ref.current ?? null) === (watchedSource ?? null)) or pick a single nullable shape end-to-end.
  • packages/app/src/DBSearchPage.tsx:790 -- formatDroppedFiltersMessage is a single-call helper that hides the toast text from its only call site.
    • Fix: Inline the ternaries at the call site.
  • packages/app/src/DBSearchPage.tsx:1142 -- retainCompatibleFilters (wrapper) and retainFiltersByColumns (hook method) have near-identical names for two layers.
    • Fix: Rename the wrapper to something like applyPendingFilterReconcile, or inline it into the effect since it has one caller.
  • packages/app/src/searchFilters.tsx:540 -- dotIdx > 0 treats a key starting with . as having no dot and returns the full leading-dot key as the "root column".
    • Fix: Use dotIdx > 0 deliberately only if leading-dot keys are impossible; otherwise switch to dotIdx >= 0 (paired with slice(0, dotIdx)).
  • packages/app/src/searchFilters.tsx:536 -- Inline comment frames the exact-match branch as "the rare case of a column whose name contains dots", but that branch also routinely handles every top-level column.
    • Fix: Rewrite the comment so both branches are described accurately.
  • packages/app/src/DBSearchPage.tsx:1150 -- Toast color is the literal 'yellow' rather than a theme/warning token, if one is established elsewhere in the app.
    • Fix: Verify whether the codebase uses a warning color token for Mantine notifications and follow that convention.
  • packages/app/src/__tests__/searchFilters.test.ts:1023 -- Tests assert onFilterChange is not called on the no-drop path but never assert that setFilters itself isn't invoked.
    • Fix: Add a spy or referential-equality check so a regression that re-renders subscribers on a no-op reconcile is caught.

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

Testing gaps:

  • No test for useColumns returning [] (silent wipe path).
  • No test for two sources sharing databaseName/tableName across different connectionIds (cache-collision path).
  • No test for useColumns cold-fetch >1s exposing the debouncedSubmit broken-query window.
  • No test for rapid A → B → C source switches before columns resolve (ref-guard correctness).
  • No test that switching back to the original source does not restore previously dropped filters.
  • No test for passthrough filters surviving the reconcile against the new schema.

knudtty
knudtty previously approved these changes May 22, 2026
Copy link
Copy Markdown
Contributor

@knudtty knudtty left a comment

Choose a reason for hiding this comment

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

nothing blocking

Comment thread packages/app/src/DBSearchPage.tsx Outdated
@karl-power karl-power force-pushed the karl/preserve-compatible-filters-when-switching-sources branch from 27f5bd8 to ce02044 Compare May 26, 2026 07:46
@kodiakhq kodiakhq Bot merged commit 923b544 into main May 27, 2026
17 of 18 checks passed
@kodiakhq kodiakhq Bot deleted the karl/preserve-compatible-filters-when-switching-sources branch May 27, 2026 08:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

automerge review/tier-2 Low risk — AI review + quick human skim

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants