feat: render active filters as inline chips in search where input#2388
feat: render active filters as inline chips in search where input#2388MikeShi42 wants to merge 5 commits into
Conversation
🦋 Changeset detectedLatest commit: 1dbfe55 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.
|
🟡 Tier 3 — StandardIntroduces new logic, modifies core functionality, or touches areas with non-trivial risk. Why this tier:
Review process: Full human review — logic, architecture, edge cases. Stats
|
E2E Test Results✅ All tests passed • 216 passed • 3 skipped • 1391s
Tests ran across 4 shards in parallel. |
Deep Review✅ No critical issues found. 🟡 P2 -- recommended
🔵 P3 nitpicks (7)
Reviewers (9): correctness, testing, maintainability, project-standards, kieran-typescript, julik-frontend-races, adversarial, agent-native, learnings-researcher. Testing gaps:
|
|
@MikeShi42 can you take a look at this PR? #2360 or HDX-4381 Previously, when a user switched the active source on the search page (e.g. Logs → Traces), any filters with fields not present in the new source were silently dropped. A transient yellow toast notified the user, but their selection and the context for switching back was lost. The idea of the PR ##2360 was to keep those filters visible in the ActiveFilterPills bar using a muted, strikethrough, dashed-border style, along with a tooltip explaining why they aren’t applied. The filters are excluded from the generated query to keep it valid, and they automatically reapply if the user switches back to a compatible source. Would be possible to achieve something similar in this PR? Do you think that proposal makes sense? |
|
@elizabetdev I wonder if zooming out - if our source switching mechanic needs a bit of a rethink to solve these issues. It seems like sometimes a user may expect the source to not reset the search page, and sometimes it makes sense to reset the search page. I can't think of any good ideas right now (maybe some sort of "source groups" concept, but that seems overly complex, or "switch without clearing search" option in the dropdown). I'm curious if you have any ideas that might address the issue at a broader level. I think the strike out filters can probably be done here if we'd like, though personally I'm worried it makes the experience more complex/confusing rather than simpler, so I'm hesitant to lean into it unless we think it's really critical to patch right now. |
Replaces the separate ActiveFilterPills row below the search input with inline filter chips rendered directly inside SearchWhereInput. The chips slot is wired through both SQLInlineEditor (CodeMirror) and AutocompleteInput (Lucene), and Backspace at cursor position 0 removes the last chip. - Add InlineFilterChips component and filterPillUtils helpers - Add filterChips + onRemoveLastChip props to SQLInlineEditor, AutocompleteInput, and SearchInputV2 - Wire SearchWhereInput to flatten searchFilters into chips and handle last-chip removal - Remove ActiveFilterPills component and its test - Add unit tests for InlineFilterChips and filterPillUtils, plus an E2E suite (filter-chips.spec.ts)
- SCSS: collapse `:not(.expanded):not([data-has-chips])` into a single complex `:not(.expanded, [data-has-chips])` to satisfy stylelint's `selector-not-notation` rule. - Unit: remove the `@/components/ActiveFilterPills` jest.mock from DBSearchPage.directTrace.test.tsx since the component was deleted. - E2E: the sidebar writes filters in Lucene form (`field:"value"`), not SQL `'value'`, so the URL-sync wait check in `applySidebarFilter` never matched. Switch to a value-substring check and bump the timeout to 10s for slower CI environments.
MikeShi42
force-pushed
the
mikeshi/filter-chips-in-where-input
branch
from
95fa195 to
f9d6e54
Compare
| setSelectedQueryHistoryIndex(-1); | ||
| setIsSearchInputFocused(false); | ||
| }} | ||
| onKeyDown={e => { |
There was a problem hiding this comment.
Can we pull this out instead of inlining?
| if ( | ||
| e.key === 'ArrowDown' && | ||
| e.target instanceof HTMLTextAreaElement | ||
| ) { |
There was a problem hiding this comment.
Hmm arrow up and down on autocomplete is not working for me like it did previously. But it also isn't working on play-clickstack.clickhouse.com.
| } | ||
|
|
||
| .chipExcluded { | ||
| border-color: var(--mantine-color-red-light); |
There was a problem hiding this comment.
This doesn't look very red in light mode
| dateRange={searchedTimeRange} | ||
| sourceId={inputSource} | ||
| size="xs" | ||
| searchFilters={searchFilters} |
There was a problem hiding this comment.
Suuuuper cool that this is the only diff required on this component
| // Inline filter chips | ||
| const filters = searchFilters?.filters; | ||
| const pills = useMemo( | ||
| () => (filters ? flattenFilters(filters) : []), |
There was a problem hiding this comment.
style: flattenFilters could accept FilterState | undefined and just have a guard returning [] if undefined. Then this memo is simplified
- SearchWhereInput: gate `filterChips` slot and `data-has-chips` on
`pills.length > 0`, not on `searchFilters` being wired. Previously
DBSearchPage (which always passes the hook) flipped the attribute on
unconditionally, breaking the SQL Paper's `:not(.expanded,
[data-has-chips]) { max-height: 36px }` cap and the Lucene
collapse-fade gradient for the empty-filter state.
- AutocompleteInput: when Backspace removes a chip, reset
`selectedAutocompleteIndex` and close the suggestion dropdown,
mirroring the Escape branch. Without this, an arrow-key-highlighted
suggestion lingers and the next Enter inserts an unintended token.
- AutocompleteInput + SQLInlineEditor: skip the Backspace-removes-chip
handler during IME composition (CJK input). The textarea value is
empty mid-composition, which previously caused Backspace to eat a
chip instead of editing the composition.
- Tighten `onRemoveLastChip` to `() => boolean` across SearchInputV2,
AutocompleteInput, and SQLInlineEditor so the "consume the keystroke"
contract is enforced by the type system end to end.
- InlineFilterChips: drop the array index from the chip key. Field +
type + value is already unique (the underlying sets dedupe), and
removing a middle chip no longer remounts every subsequent chip's
Tooltip / focus state.
- Re-comment the `.mantine-InputWrapper-root` SCSS rule — Mantine still
emits that wrapper, so the rule is live and load-bearing for Lucene
sizing, not legacy.
- AutocompleteInput: pull the inline onKeyDown handler out into a named `handleKeyDown` useCallback so the JSX body stays readable. Also wrap `onAcceptSuggestion` in useCallback now that it's a dependency of `handleKeyDown`. - InlineFilterChips.module.scss: swap the excluded chip border from `--mantine-color-red-light` (a pink background tint that barely reads as red in light mode) to `--color-text-danger`, which already swings red-3 ↔ red-8 by theme. - filterPillUtils: `flattenFilters` now accepts `FilterState | undefined` and returns `[]` for the undefined case, so callers don't need to guard externally. SearchWhereInput's memo collapses to a single call.
|
@MikeShi42 agreed, let's not overcomplicate this. I'd keep the reset for now. Not resetting causes more headaches than it's worth. Middle ground: people can already pin go-to filters per source. More repeat-use than in-the-moment continuity, but that's probably the common case. Agree the broader mechanic deserves a proper rethink though. Let's revisit it down the line once we've got more signal on what people actually expect. Also what do you twink of removing the WHERE inside the input? We already have it in the placeholder. |
|
@elizabetdev @MikeShi42 Does that mean we can close/cancel Wire up inactive filter preservation on source switch? |
|
@karl-power, we can close it or we can rename the issue and continue the discussion there. |
4 participants
Summary
Replaces the separate
ActiveFilterPillsrow below the search input with inline filter chips rendered directly insideSearchWhereInput. The chips slot is wired through bothSQLInlineEditor(CodeMirror) andAutocompleteInput(Lucene), and Backspace at cursor position 0 removes the last chip — making filter state feel native to the input rather than parked beside it.What changed
InlineFilterChipscomponent +filterPillUtilshelpers (flattenFilters,removePill) that turn aFilterStateinto a list of chips and route removal back through the existing filter-state hook.SQLInlineEditor,AutocompleteInput, andSearchInputV2all gain optionalfilterChipsandonRemoveLastChipprops.SearchWhereInputaccepts a newsearchFilters?: FilterStateHookprop, flattens it into chips, and forwards both the chip nodes and the last-chip removal callback to whichever underlying editor is active.ActiveFilterPills(and its test) — its responsibility is now subsumed bySearchWhereInput.DBSearchPagedrops the<ActiveFilterPills .../>row and passes itssearchFiltershook toSearchWhereInputinstead.Tests
InlineFilterChips.test.tsx(rendering, group container, no artificial limit),filterPillUtils.test.ts(flatten + remove semantics).tests/e2e/features/search/filter-chips.spec.ts+ aFilterChipsComponentpage object.Test plan
/search, apply a couple of sidebar filters (e.g.ServiceName,SeverityText) — verify chips appear inside the where input, not below it.Notes
main(8e52cef). Conflict resolution preserved main'sword-break: break-allfix (fix: use character-level word breaking in Lucene search input #2181) and thee.preventDefault()additions on ArrowDown/ArrowUp from the autocomplete refactor (feat: fast and full autocomplete #2128).ActiveFilterPillsis dropped because the component is removed — let me know if any of that styling should be carried over toInlineFilterChips.