feat(app): add faceted filters to the sessions page - #2992
Conversation
🦋 Changeset detectedLatest commit: 2f51ae2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
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 |
|
@vinzee is attempting to deploy a commit to the HyperDX Team on Vercel. A member of the Team first needs to authorize it. |
97076ea to
fa84b72
Compare
Greptile SummaryThe PR adds URL-persisted faceted filters to the sessions page and applies them to correlated RUM trace aggregation.
Confidence Score: 3/5The PR is not yet safe to merge because service scoping can omit valid sessions and source changes can execute stale facet filters against a different trace schema. A nonempty but incomplete session-derived service list remains a hard trace scope, while source changes retain and resubmit SQL filters created for the previous correlated trace source. Files Needing Attention: packages/app/src/sessions.ts and packages/app/src/SessionsPage.tsx
|
| Filename | Overview |
|---|---|
| packages/app/src/SessionsPage.tsx | Adds URL-backed filter state, the sidebar UI, and facet configuration, but source switches still submit filters bound to the previous trace source. |
| packages/app/src/sessions.ts | Applies facet filters to session aggregation and adds service scoping, but a successful incomplete service enumeration still excludes valid sessions. |
| packages/app/src/components/DBSearchPageFilters.tsx | Makes analysis controls optional and supports forcing exact facet mode for the sessions wrapper. |
| packages/app/src/components/SessionFilters.tsx | Provides a thin sessions-specific configuration of the shared faceted-filter sidebar. |
| packages/app/src/tests/sessions.test.ts | Covers filter URL validation and service-filter serialization, but not incomplete service enumeration or source-switch behavior. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
U[Session query and facet filters] --> T[Correlated trace source]
S[Session source] --> E[Enumerate service names]
E --> I[ServiceName IN scope]
I --> T
T --> A[Aggregate sessions]
A --> L[Session list]
Reviews (5): Last reviewed commit: "feat(app): add faceted filters to the se..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Deep ReviewFaceted-filter sidebar added to 🔴 P0/P1 — must fix
🟡 P2 — recommended
🔵 P3 nitpicks (5)
Reviewers (10): correctness, security, adversarial, performance, frontend-races, typescript, testing, maintainability, api-contract, previous-comments. Testing gaps:
|
fa84b72 to
d81257b
Compare
2b5bdd1 to
1901232
Compare
…gate (#3024) ## Summary `claude-code-review.yml` has been disabled since **2026-05-20**, so this is a rebuild rather than a tweak. It replaces the prompt, moves it into a versioned file, and changes how findings reach the PR. **The prompt was selected by measurement, not taste.** I built an offline harness that scores reviewer prompts against a frozen dataset of **122 hyperdx PRs where a human left a substantive inline review comment**, using those comments as ground truth. On the held-out test split (Opus, 49 PRs / 87 gold items): | prompt | recall | cost/PR | agents | | --- | --- | --- | --- | | lifted-budget variant of the old prompt | 31% | $2.86 | 1 | | **this prompt** | **40%** | **$3.11** | **1** | | `deep-review` (currently in production) | ~22–33% | — | 6–13 | On the larger shared slice this prompt and `deep-review` are within noise of each other, so **the claim is parity-or-better at one agent instead of six-to-thirteen — not a 4× improvement.** I'd rather understate that than have it quoted back later. ### What changes for reviewers - **Findings post as inline comments on the changed lines.** 82% of findings anchor to a diff line (measured); the rest reference files the diff never touches and fall back to the sticky summary rather than being dropped — that class is a large share of the useful output. - **One sticky summary**, updated in place, with counts and anything unanchored. - **Repeat comments are suppressed across pushes** via a hidden per-finding fingerprint keyed on path + normalized title, so a reworded body doesn't repost. - **Minor findings are folded behind `<details>`, not filtered.** On the dataset, minor-severity findings carry **29–57% of everything a human independently flagged** — filtering them would cost roughly half the recall. Severity is used for ordering only. ### Cost and correctness controls - **Review gate.** Hashes the effective diff (`merge-base..HEAD`) and the prompt; skips when both are unchanged. `synchronize` fires on every "Update branch" merge, which advances the merge-base while leaving the diff byte-identical — without this, a large share of runs are pure waste at ~$3–5/PR. Same idea as `deep-review.yml`. - **Fail-open.** The state marker is only stamped when the run produced parseable output; otherwise the next push retries instead of the gate caching a zero-finding review against that diff forever. - **`concurrency` + cancel-in-progress**, so two quick pushes can't race the sticky comment or pay twice. - **Model pinned to `opus`.** This prompt gained ~13 points from Opus where the old one gained ~5 — the extra passes only pay off at that tier, and an action-default change shouldn't silently swap the reviewer. ### Tool grants Follows the pattern already set in `deep-review.yml`: a read-only `git`/`gh` prefix allowlist. Withheld deliberately, since this job checks out fork-authored code and holds secrets: - **`gh api`** — accepts `--method POST`, and prefix allowlists can't constrain flags (your own note in `deep-review.yml`). Prior review threads are materialized in trusted shell instead, fenced and capped, and the prompt instructs the reviewer not to re-report them. - **bare `Bash`** — would be arbitrary code execution on PR-author-controlled build scripts next to `ANTHROPIC_API_KEY`. - **`WebFetch`/`WebSearch`/`curl`/`wget`** — data egress from a job holding secrets. ### Testing `.github/scripts/review-comments.cjs` holds the comment-routing logic so it's testable without triggering a PR event. 11 tests, run by this workflow before the review step (same pattern as `pr-triage.yml`). They pin the two silent failure modes — a wrong line map, and a fingerprint that reposts — plus the fail-open contract, so nobody "tidies" the marker format and quietly disables the gate. ### Dry run on live PRs Ran against #2992, #2981, #2980 (none in the eval set) without posting: 17 findings, 14 inline, 3 in summary, $10.86. Full output on request. The standout was a `critical` on #2981 that neither `greptile` nor `deep-review` reported — quoting every builder column makes `ORDER BY "count()"` unresolvable for unaliased projections. ## Merging this does not turn it on The workflow is `disabled_manually`; merging the file doesn't change that. Enabling is a separate, deliberate step: ``` gh api -X PUT repos/hyperdxio/hyperdx/actions/workflows/claude-code-review.yml/enable ``` I'd suggest enabling it **alongside** `deep-review` for a couple of weeks (two comments per PR, distinct markers) and comparing on live PRs before deciding whether this replaces it. If it does, expect a net spend *decrease* — one agent instead of 6–13. ## Known gaps, stated plainly - **Two changes here are unevaluated.** The harness measured the prompt with the diff *inlined* and with `Read/Grep/Glob` only. CI reads the diff from a file, and grants read-only `git`/`gh` plus materialized prior comments. Those should help, but the 40% figure doesn't cover them. Prior comments in particular are *unevaluable* in the harness — on the eval PRs those comments **are** the gold set. - **Cost may run above $3.11/PR.** The dry run averaged $3.62 on mid-sized PRs. If the bill looks wrong, the natural lever is gating on the existing `review/tier-N` labels and only running this on tier-3/4. - **One measured gap.** On the dry-run PRs, both `greptile` and `deep-review` independently caught failures involving *data persisted before the change* (stale saved sort strings) and this prompt caught neither. It has no pass for that class. That's the next thing I'd add. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Bring the search page's faceted filter sidebar to /sessions and wire the selected filters into the session aggregation query. - Add a SessionFilters wrapper around DBSearchPageFilters that hides the analysis-mode tabs, denoise, and root-spans-only toggles that don't apply to sessions. - Persist sidebar filters in the URL and thread them through useSessions so they constrain the trace aggregation alongside the free-text query. Validate the ?filters= param against FilterSchema with a [] default so a stale/hand-edited param resolves to the default instead of white-screening. - Speed up the aggregation: resolve the RUM service name(s) from the session source and constrain the otel_traces scan by ServiceName, turning a ~100k-mark full scan into a small primary-key range. Enumerate those services over a window wider than the selected range so sessions that started just before the range aren't dropped, and on any failure or when no services resolve, fall back to the unscoped (still correct) scan. - Harden the ServiceName scope: escape ingested names with escapeSqlString (backslash then quote) in a dedicated buildServiceScopeFilters helper and build it inside the try/catch, so a crafted/backslash name can't inject into or break the aggregation. - Fix the empty filter sidebar: scope facet queries to RUM session spans via a rum.sessionId indexHint and force exact facet mode, so the 'show all values' path can't strip the scope and sample the whole trace table (which timed out and left the sidebar empty). - Portal the source-select dropdown so it renders above the filter sidebar instead of being clipped by its stacking context. - Add unit tests: SessionFilters prop forwarding, buildServiceScopeFilters escaping/fallback, and the ?filters= param validator. Co-authored-by: Cursor <cursoragent@cursor.com>
1901232 to
2f51ae2
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| // Applying a facet filter re-runs the query immediately (against the current | ||
| // time range), mirroring the search page's debounced auto-submit. | ||
| const handleSetFilters = useCallback( |
There was a problem hiding this comment.
🟠 major — Clicking a sidebar facet resets the form and discards a typed-but-unsubmitted query
useForm({ values: {...} }) at SessionsPage.tsx:272 has no resetOptions, so RHF does a full reset whenever the values object changes. handleSetFilters calls setAppliedConfig({ filters }), which changes appliedConfig → changes values → resets where back to appliedConfig.where. Concretely: type error in the search box (don't press Run), click a ServiceName checkbox — the search box visibly reverts to empty and the query never runs. The comment claims this mirrors the search page, but DBSearchPage.tsx:1122 sets resetOptions: { keepDirtyValues: true } and its handleSetFilters (DBSearchPage.tsx:1296) calls debouncedSubmit() so the whole form is applied. Do the same here: add resetOptions: { keepDirtyValues: true } to the useForm call and make handleSetFilters call onSubmit() instead of writing only filters.
| ]; | ||
| const hasSearchQuery = searchFilters.length > 0; | ||
|
|
||
| // Scope the trace aggregation to just the service(s) that emit RUM |
There was a problem hiding this comment.
🟠 major — ServiceName scope derived from the session source can silently drop whole services' sessions
The scope list is built from a DISTINCT ServiceName scan of sessionSource.from (the rrweb table), then applied as a hard ServiceName IN (...) predicate on the trace aggregation. A service that emits RUM spans (rum.sessionId, component:"user-interaction") but writes no rrweb rows — session recording disabled in the browser SDK, recording not yet started, or a shorter TTL on the sessions table — contributes zero rows to that DISTINCT, so its sessions disappear from /sessions entirely, with no error and no UI signal. The try/catch only covers query failure, not a successfully-returned-but-incomplete list. Either derive the scope from the trace source itself (a cheap DISTINCT serviceNameExpression with the rum.sessionId predicate over the same range), or gate the whole optimization behind an explicit source/feature flag so correctness isn't traded away by default.
| // which times out and leaves the sidebar empty. The `indexHint` lets the | ||
| // `rum.sessionId` skip index prune to just the session rows. This is applied | ||
| // as an always-on SQL filter alongside the user's search + facet selections. | ||
| const rumSessionIdKey = `${traceTrace.resourceAttributesExpression}['rum.sessionId']`; |
There was a problem hiding this comment.
🟠 major — rumScopeFilter hardcodes Map access and mapContains, breaking JSON-typed ResourceAttributes
${traceTrace.resourceAttributesExpression}['rum.sessionId'] and mapContains(...) assume the column is a Map. The repo already has useFieldExpressionGenerator (packages/app/src/hooks/useFieldExpressionGenerator.tsx) for exactly this — sessions.ts:132 uses it to build traceSessionIdExpression for the same key, emitting toString(col.key) for JSON columns. On a JSON-typed ResourceAttributes the sidebar's facet query is invalid SQL, so every facet fetch fails and the user gets a red notification plus an empty sidebar. resourceAttributesExpression is also z.string().optional() (common-utils/src/types.ts:2279), so it can render as the literal undefined. Build the expression with useFieldExpressionGenerator(traceTrace) and apply the ?? 'ResourceAttributes' fallback used elsewhere in sessions.ts.
| </> | ||
| )} | ||
| </> | ||
| <ActiveFilterPills |
There was a problem hiding this comment.
🟠 major — In-pill value picker strips the RUM scope that forceExactFacetMode exists to preserve
ActiveFilterPills is handed filtersChartConfig, and FilterPill builds its value-picker config as { ...chartConfig, where: '', filters: [] } (packages/app/src/components/ActiveFilterPills.tsx:160). That clears rumScopeFilter, so clicking a pill on /sessions runs groupUniqArray over the entire trace table for the selected range — the exact unscoped query the PR added forceExactFacetMode to avoid. Carry the always-on scope outside filters (e.g. fold it into the config's where, or add a prop for a non-strippable base filter honoured by both useFacets' mode === 'all' branch and valueChartConfig) so the pill picker stays scoped.
| sourceId: sourceId ?? null, | ||
| dateRange, | ||
| mode: showAllValues ? 'all' : 'exact', | ||
| mode: forceExactFacetMode || !showAllValues ? 'exact' : 'all', |
There was a problem hiding this comment.
🔵 minor — "Show all values" setting is inert on /sessions but still writes the shared localStorage key
With forceExactFacetMode, mode is always 'exact', yet FilterSettingsPanel (line 1661) still renders the showAllValues switch, wired to the shared hdx-show-all-filter-values key. On /sessions the toggle does nothing locally while silently changing the search page's facet behaviour. Add a prop to hide (or disable with an explanatory tooltip) the "Show all values" control when forceExactFacetMode is set.
| @@ -1622,9 +1633,11 @@ const DBSearchPageFiltersComponent = ({ | |||
| > | |||
| <Stack gap="sm" p="xs"> | |||
| <Flex align="center" justify="space-between"> | |||
There was a problem hiding this comment.
🔵 minor — Sidebar toolbar icons jump to the left edge when hideAnalysisMode is set
The header <Flex align="center" justify="space-between"> relies on the "Analysis Mode" Text as its first item. With hideAnalysisMode the Group holding the settings gear and the "Hide filters" collapse button is the only flex child, so space-between places it at flex-start — the controls render at the top-left of the sessions sidebar instead of the top-right where they sit on the search page. Use justify={hideAnalysisMode ? 'flex-end' : 'space-between'}.
| style={{ flex: 1, minWidth: 0, minHeight: 0 }} | ||
| > | ||
| <Group gap={4} align="center" wrap="nowrap"> | ||
| {isFilterSidebarCollapsed && ( |
There was a problem hiding this comment.
🔵 minor — Show-filters button duplicates DBSearchPage's ExpandFiltersButton
The Tooltip + subtle ActionIcon + IconArrowBarToRight + aria-label="Show filters" block is a copy of ExpandFiltersButton in DBSearchPage.tsx:312 (differing only in icon/ActionIcon size). Export that component (or move it next to DBSearchPageFilters) and use it here so the two collapse affordances can't drift.
| // failure — query or serialization — falls back to the unscoped (correct) | ||
| // scan rather than emitting broken/injectable SQL into the aggregation. | ||
| let serviceScopeFilters: Filter[] = []; | ||
| try { |
There was a problem hiding this comment.
🔵 minor — Service-name enumeration re-runs serially inside every sessions fetch
The DISTINCT ServiceName query is issued inside queryFn and awaited before the main Promise.all, so every time range change, facet toggle, or manual refresh pays an extra serialized round trip for a 30-day scan whose answer changes very rarely. Lift it into its own useQuery (keyed on sessionSource.id + a coarse window) with a long staleTime, and pass the resolved names into useSessions; the fallback-to-unscoped behaviour on error is unchanged.
PR Review10 finding(s): 🔴 0 critical · 🟠 4 major · 🔵 6 minor 8 posted as inline comment(s) on the changed lines. 2 listed below. Findings outside the changed lines2 minor
Severity is the reviewer's own estimate and is used for ordering, not filtering. |
| <ActiveFilterPills | ||
| searchFilters={searchFilters} | ||
| chartConfig={filtersChartConfig} | ||
| dateTimeColumns={dateTimeColumns} | ||
| style={{ flex: 1 }} | ||
| /> |
There was a problem hiding this comment.
I think we could use some bottom margin here, and maybe some alignment of the left margin between these pills and the session card list
| {...props} | ||
| isLive={false} | ||
| hideAnalysisMode | ||
| forceExactFacetMode |
There was a problem hiding this comment.
If we're forcing exact mode, we should avoid showing the option to toggle between exact and all mode
| { | ||
| databaseName: traceTrace?.from?.databaseName ?? '', | ||
| tableName: traceTrace?.from?.tableName ?? '', | ||
| connectionId: traceTrace?.connection ?? '', | ||
| }, |
There was a problem hiding this comment.
| { | |
| databaseName: traceTrace?.from?.databaseName ?? '', | |
| tableName: traceTrace?.from?.tableName ?? '', | |
| connectionId: traceTrace?.connection ?? '', | |
| }, | |
| tcFromSource(traceTrace) |
| const rumScopeFilter: Filter = { | ||
| type: 'sql', | ||
| condition: `notEmpty(${rumSessionIdKey}) AND indexHint(mapContains(${traceTrace.resourceAttributesExpression}, 'rum.sessionId'))`, | ||
| }; |
There was a problem hiding this comment.
I believe this will probably break when the trace source is the JSON schema (where resource attributes is not a map but rather a JSON column) due to the mapContains.
Same thing above for the rumSessionIdKey using bracket syntax
| .query({ | ||
| query: serviceNamesQuery.sql, | ||
| query_params: serviceNamesQuery.params, | ||
| connectionId: sessionSource.connection, |
There was a problem hiding this comment.
| connectionId: sessionSource.connection, | |
| connectionId: sessionSource.connection, | |
| format: 'JSON', |
| {...props} | ||
| isLive={false} | ||
| hideAnalysisMode | ||
| forceExactFacetMode |
There was a problem hiding this comment.
Two other reachable paths wipe chartConfig.filters too: DBSearchPageFilters/hooks.ts:225 (loadMoreFacetsForKey) and ActiveFilterPills.tsx:161 (in-pill value picker). "Show more values" and pill-editing therefore scan the whole trace table unscoped
| // Scope the trace aggregation to just the service(s) that emit RUM | ||
| // sessions. `otel_traces` is sorted by (ServiceName, SpanName, Timestamp), | ||
| // so with no ServiceName predicate the time filter can't prune via the | ||
| // primary index — the query scans the whole table's marks (~100k) to | ||
| // evaluate the `rum.sessionId` skip index. RUM traffic comes from a tiny | ||
| // set of services, which we read cheaply from the session source (sorted | ||
| // by time), turning the trace scan into a primary-key range (~50 marks, | ||
| // ~20x faster cold). | ||
| // | ||
| // Correctness assumption: the session and trace sources report the same | ||
| // `ServiceName` for RUM spans (the session source is the cheap, | ||
| // time-sorted projection of the same RUM traffic, which is why we read the | ||
| // list from there instead of re-scanning the trace table). If they diverge | ||
| // — a service present in trace RUM spans but absent from the session-source | ||
| // scan, or a differing service-name column — that service's sessions would | ||
| // be dropped. On any query failure or an empty result we fall back to the | ||
| // unscoped (correct) scan. | ||
| const serviceNameExpression = | ||
| traceSource.serviceNameExpression || 'ServiceName'; | ||
| // Enumerate RUM services over a window wider than the selected range. A | ||
| // session's row in the session source is timestamped at (near) its start, | ||
| // which can fall *before* `dateRange` even though its trace spans land | ||
| // inside it. Scoping to only services seen within the exact range would | ||
| // then drop those sessions. Over-inclusion is safe — an extra ServiceName | ||
| // only widens the primary-key range scanned; the `rum.sessionId` | ||
| // predicate + HAVING still filter the rows — so we look back a generous | ||
| // margin. Under-inclusion is the correctness bug we must avoid. | ||
| const SERVICE_SCOPE_LOOKBACK_MS = 30 * 24 * 60 * 60 * 1000; // 30 days | ||
| const serviceScopeDateRange: [Date, Date] = [ | ||
| new Date(dateRange[0].getTime() - SERVICE_SCOPE_LOOKBACK_MS), | ||
| dateRange[1], | ||
| ]; |
There was a problem hiding this comment.
This optimization seems like a separate change, with separate risks - could we split it out into it's own PR for discussion?
| // only widens the primary-key range scanned; the `rum.sessionId` | ||
| // predicate + HAVING still filter the rows — so we look back a generous | ||
| // margin. Under-inclusion is the correctness bug we must avoid. | ||
| const SERVICE_SCOPE_LOOKBACK_MS = 30 * 24 * 60 * 60 * 1000; // 30 days |
There was a problem hiding this comment.
I'm curious why this would be such a long lookback - it seems excessive to scan 30 days back before each sessions query. And since this is tied to the selected date range, if won't necessarily be cached when changing date ranges.
2 participants
Summary
Bring the search page's faceted filter sidebar to /sessions and wire the selected filters into the session aggregation query.
Screenshots or video
The Sessions Tab now has a sidebar with filters:
How to test on Vercel preview
Preview routes: /sessions
Steps (/sessions):
References