feat(search): edit a filter pill's value in place#2457
Conversation
Make the active filter pills under the search bar editable in place. Clicking an included or excluded pill opens a small action menu to copy the value or flip the filter polarity (include vs exclude), reusing the EventTag popover pattern and setFilterValue's existing include/exclude actions. The one-click remove (x) on the pill is unchanged, and range and unapplied pills keep their remove-only behavior. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Add the changeset for the editable filter pills change, and replace the en-dash in the range pill separator with a hyphen to satisfy the no-en-dash lint on the file.
Add a value picker to the filter-pill action menu so a user can switch a filter to a different value of the same field without removing and re-adding it. The picker is a searchable Select fed by useGetKeyValues (fetched only while the menu is open), and a new replaceFilterValue helper on useSearchPageFilterState swaps the value in one state update so the query runs once. Polarity is preserved (an excluded pill stays excluded). DBSearchPage passes the active source's chart config to the pills for the value query. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
prose-lint flags these on any change to the file; converting to commas.
🦋 Changeset detectedLatest commit: 84bf4b4 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Greptile SummaryThis PR adds an inline value picker to the filter-pill action menu, letting users swap a filter's value without removing and re-adding it. It introduces
Confidence Score: 5/5Safe to merge; the value-picker feature is well-contained and the single-emission guarantee is correctly implemented and tested. The new replaceFilterValue helper, the Select picker wiring, and the chartConfig threading are all implemented cleanly. The single-state-update guarantee (one query run per value swap) is correct, tests cover both polarities and the emission count, and the config stripping that prevents query-scoped value lists is verified. The chartConfig prop change is TypeScript-checked and the only non-mocked caller already supplies the value. packages/app/src/searchFilters.tsx — the replaceFilterValue function was noted in a prior review thread as missing a cleanup of the opposite set when the new value already exists there; worth confirming that concern is addressed before shipping. Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant FilterPill
participant useGetKeyValues
participant ActiveFilterPills
participant replaceFilterValue
participant updateFilterQuery
User->>FilterPill: Click pill to open popover
FilterPill->>useGetKeyValues: fetch field values with where and filters cleared
useGetKeyValues-->>FilterPill: up to 50 values for the field
FilterPill->>FilterPill: prepend current value to options list
User->>FilterPill: Select a different value from picker
FilterPill->>ActiveFilterPills: onReplaceValue with new value
ActiveFilterPills->>replaceFilterValue: field, rawValue, newValue, polarity
replaceFilterValue->>replaceFilterValue: immer delete oldValue then add newValue in target set
replaceFilterValue->>updateFilterQuery: emit onFilterChange exactly once
FilterPill->>FilterPill: close popover
Reviews (2): Last reviewed commit: "fix: show all field values when editing ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| setFilters(prevFilters => { | ||
| const newFilters = produce(prevFilters, draft => { | ||
| if (!draft[property]) { | ||
| draft[property] = { included: new Set(), excluded: new Set() }; | ||
| } | ||
| const set = | ||
| action === 'exclude' | ||
| ? draft[property].excluded | ||
| : draft[property].included; | ||
| set.delete(oldValue); | ||
| set.add(newValue); | ||
| }); | ||
| updateFilterQuery(newFilters); | ||
| return newFilters; | ||
| }); |
There was a problem hiding this comment.
replaceFilterValue only modifies the target set (included or excluded) but never removes newValue from the opposite set. If a user has status = 200 (included) and status != 404 (excluded), and then opens the include pill and picks 404, the result is included: {'404'} and excluded: {'404'} simultaneously. Both setFilterValue call paths (action === 'exclude' / default include) explicitly clean the other set before writing, but this new path does not.
The fix is to add a delete call on the opposite set after adding newValue:
const targetSet = action === 'exclude' ? draft[property].excluded : draft[property].included;
const oppositeSet = action === 'exclude' ? draft[property].included : draft[property].excluded;
targetSet.delete(oldValue);
targetSet.add(newValue);
oppositeSet.delete(newValue); // prevent value from existing in both sets
The value picker fetched its options with the search page's chart config, which carries the active where clause and the filters (the pills themselves). For an included pill the value query was scoped to rows already matching that pill, so it only ever returned the pill's own value and the picker had nothing to switch to. Clear where and filters before the value query so it lists all of the field's values in the time range, matching the sidebar facet list's default behavior. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
alex-fedotyev
force-pushed
the
alex/hdx-4326-editable-filter-pills
branch
from
0993367 to
3467cf2
Compare
|
Folded into #2455, which now carries the full feature (the pill action menu plus in-place value editing) as a single PR. Closing here. |
1 participant
Add a value picker to the filter-pill action menu so a filter can be switched to a different value of the same field without removing and re-adding it.
Builds on #2455 (the pill action menu) and is stacked on that branch, so the diff here is just the value-editing piece. This finishes the second half of HDX-4326.
Summary
Selectabove the copy and polarity actions. Picking a different value swaps the filter in place.useGetKeyValueshook, fetched only while the menu is open (enabled: opened) and scoped to the pill's field.replaceFilterValuehelper onuseSearchPageFilterStatedoes the swap in one state update, so the search query runs once. TwosetFilterValuecalls would each emitonFilterChange, triggering two query runs for one action.DBSearchPagepasses the active source'sfiltersChartConfigto the pills for the value query.Why
Together with #2455 this completes "editable filters" (HDX-4326): from a pill you can now flip include and exclude, copy, remove, and change the value, without rebuilding the filter from the sidebar.
The value swap lives in a dedicated hook helper rather than two
setFilterValuecalls because each call emitsonFilterChange, which would run the query twice for one user action.Test plan
make ci-lint(eslint + tsc) on@hyperdx/appmake ci-unit:searchFilters.test.ts(new) coversreplaceFilterValueswapping an included value, swapping an excluded value, and emittingonFilterChangeexactly once;ActiveFilterPills.test.tsxcovers the value picker populating fromuseGetKeyValuesand selecting a value callingreplaceFilterValuewith the right args and polarity (28 tests total)Note
This PR also converts six pre-existing em-dashes in
searchFilters.tsxcomments to commas. They are unrelated to the feature, but our prose lint flags them on any change to the file.