feat(ui): render Explore quick filters through FilterSelect - #33021
Conversation
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
harsh-vador
added
the
safe to test
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
65e6e90 to
0efa026
Compare
❌ PR checklist incompleteThis PR cannot be merged until the following are addressed on its linked issue:
The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically. Maintainers can bypass this check by adding the |
Jest test CoverageUI tests summary
|
…ws (#33378) * style(ui-core): brand-blue selected state for single-select filter rows Design feedback on the unified filter dropdowns: a single-select row's selected state reads as blue — brand-50 row tint with brand text, icon and count pill — instead of a trailing tick. The tick is removed; multi-select rows are unchanged, where the checkbox alone conveys selection. Verified live in Storybook: the selected row renders one svg (the option's own icon — no tick), rgb(239,248,255) row tint, and rgb(21,112,239) icon and label. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(ui-core): cover the icon and count-pill selected styling paths Review follow-up: the single-select styling test used bare options, so the svg recolor override and the pill's brand border/text had no coverage. The new case selects an icon-and-count option and asserts both selected-state classes, and their absence on an unselected sibling. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
- Staged single-select waits for Apply again (greptile P1): FilterSelect now stages single selections when commitMode='staged' and keeps the popover open (shouldCloseOnSelect is owned by the component), restoring the legacy Update-gated behavior for consumers like Test Definitions. - Staged labels survive option repaints (greptile P2): ExploreQuickFilters records every option it publishes per field (bounded map) and resolves committed keys against it, so a staged value keeps its human-readable label after a later search replaced the shared options list. - Impact map (greptile P2): hand-authored mapping routes filter-select / base dropdown / ExploreQuickFilters changes to the former SearchDropdown consumer spec set, so PR CI selects that coverage again. - Checkstyle: drop the unused raw title attribute from the FilterSelect test mock (the remaining warnings in the summary are pre-existing backlog in files this PR only reflows). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… into fix-33021-v2
A full clear at the 500-entry cap could evict the one entry a still-staged key needs, reintroducing the raw-key-as-label fallback the record exists to prevent. Delete-then-set keeps Map insertion order as recency order, and overflow now drops single oldest entries. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
…nstantly The impact-map entry added for #33021 review feedback now routes the former SearchDropdown consumer specs to this PR — their first run against FilterSelect surfaced two classes of failure: 1. Legacy selectors in specs CI had never selected here: Update-button-by- name is now the update-btn testid ('Apply (N)' label), option rows are menuitemcheckbox rather than menuitem, and the search box's accessible name is no longer the legacy 'Search <label>...' placeholder. Migrated ImpactAnalysis, ColumnBulkOperations, DomainFilterQueryFilter, ExploreTree, and the entity/glossary utils. 2. The popover exit animation kept the closing dropdown visible and hit-testable for ~100ms, so helpers that poll drop-down-menu visibility (ExploreUrlState, ExploreFilterComposition) filled the dying filter's now-disabled search box and hung until their predicate timeout — the trace shows zero tag aggregations fired. FilterSelect popovers now close with no exit animation (hidden + animate-none), so the closing subtree is gone the instant it closes; verified in Storybook (0 menus 30ms after Escape, exactly one menu and one enabled search box mid sibling-swap). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
# Conflicts: # .github/playwright/impact-map.generated.json
- QueryFilterInterface is defined in interface/queryFilter.interface.ts; import it from there instead of through the ExplorePage re-export (no-lower-layer-page-imports in ExploreQuickFilters and ExploreV1). - ExploreV1 read the *global* location, which is not a valid hook dependency — the parsedSearch memo never recomputed on in-app navigation. Use useCustomLocation like its siblings. - Complete the exhaustive-deps arrays where every added dependency is identity-stable (stable useCallback, derived-from-existing-dep values, a prop, i18n t) and memoize getTabsInfo() so it can be one. Remaining two warnings are runtime import cycles through SearchClassBase and AdvanceSearchProvider that predate this PR and need a dependency inversion, not a local edit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
getTabsInfo() bakes t() output into its labels, so memoizing it with no dependencies froze the first language for the component's lifetime (greptile P2 on #33021). Key the memo on i18n.language; the local react-i18next test mock now returns i18n like the global one. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The input-variant trigger was a fixed 176px at ~40px tall, so toolbars that swapped an antd Select for FilterSelect (Collate pipeline observability filters) grew taller, stopped filling their flex items, and wrapped to a second row. Default to 32px height and full container width — matching the replaced control — and let consumers constrain via className. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…elect The wider (push-trigger) Playwright run selects specs the PR-scoped run does not, surfacing four more legacy-renderer assumptions: - Add-Assets drawer matrix asserted the trigger echoes the selected label; FilterSelect shows a count badge — assert the badge instead (applied = '1', discarded = hidden). - ImpactAnalysis owner/domain rows were located by label association, which react-aria menu rows don't have — use role menuitemcheckbox by accessible name. - Pagination's Impact Analysis quick filter still clicked the Update button by visible name. - The glossary asset-modal util used the legacy label-cased checkbox id (Mysql-checkbox); FilterSelect keys rows by option value, and the ES serviceType bucket key is normalized to lowercase (mysql-checkbox). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ctAnalysis The ui-checkstyle gate replays organize-imports → eslint --fix → prettier and fails when the tree moves; the hand-reflowed getByRole chains and import block differed from that pipeline's stable output. This is the sequence's own result, iterated to a fixed point locally. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
# Conflicts: # .github/playwright/impact-map.generated.json
The empty state, helper text, and status-footer count were raw spans/divs
with ad-hoc size and color classes; use the foundations Typography
component. The structural spans inside the trigger, option rows, and
chips stay: Typography wraps content in a block-level prose div, which
would break their inline flex flow, truncation, and the legacy
.locator('span') E2E contract.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… state The FilterSelect option rows kept a hidden real <input> solely so legacy specs could .check()/toBeChecked() a `<value>-checkbox|-radio` id. The design-system checkbox (CheckboxBase via Dropdown.Item showCheckbox) already renders the visuals, and react-aria already exposes the state as aria-checked on the menuitemcheckbox/menuitemradio row that carries the same bare-value testid — so the shim is gone and every spec now asserts aria-checked on the row and clicks the row itself. Central helpers keep their legacy `-checkbox` string surface and strip the suffix (checkCheckboxStatus, applyEntityTypeFilterValue), so most call sites are untouched; direct input references across 14 specs/utils are migrated to row locators. The null row is addressed by its value id (OM_NULL_FIELD) instead of no-option-checkbox. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
🔄 Playwright impact map auto-refreshedThis PR touched specs or UI source that changed the source→spec routing map. I regenerated What is this file? It is the auto-generated half of Playwright's PR planner. It routes "if source X changes, run specs Y" by walking spec imports and cross-referencing What if I want to regenerate locally instead? Run this before pushing your next change to skip the bot commit: python3 .github/scripts/generate_playwright_impact_map.py
git add .github/playwright/impact-map.generated.json
git commit --amend --no-edit # or a separate commit |
|
| Count | Rule |
|---|---|
| 2 | openmetadata-imports/no-circular-imports |
All findings
| Location | Rule | Message | |
|---|---|---|---|
| 🟡 | src/components/Explore/ExploreQuickFilters.tsx:46:1 |
openmetadata-imports/no-circular-imports |
This runtime import participates in a circular dependency. Extract the shared type/constant/utility or invert the dependency. |
| 🟡 | src/components/Explore/ExploreQuickFilters.tsx:49:1 |
openmetadata-imports/no-circular-imports |
This runtime import participates in a circular dependency. Extract the shared type/constant/utility or invert the dependency. |
Fix locally (fast - only checks files changed in this branch):
make ui-checkstyle-changedTypography gains an additive `inline` prop that skips its block-level prose wrapper, making it usable inside buttons, chips, and flex rows where a wrapping div is invalid HTML or breaks truncation — the reason FilterSelect still carried raw text spans. All remaining text (trigger labels, count badge, option titles, count pills, chip labels, placeholder) now renders through Typography; structural/icon spans stay. Also centers the empty state and drops it to text-xs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…er.ts The Data Steward permission flow still clicked the removed table-checkbox input via a string locator, which the row-migration sweep regex missed. Scroll and click the menu row itself. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Code Review ✅ Approved 4 closed / 4 findingsMigrates Explore quick filters to the unified ✅ 4 closed✅ Edge Case: Debounced search isn't cancelled on close, defeating race guard
✅ Bug: Escape in FilterSelect also closes host drawer/modal
✅ Edge Case: Initial-options cache key omits isNLPActive
✅ Edge Case: Full clear of seen-options cache can drop a staged label
OptionsDisplay: compact → Counting what did not apply, without listing it. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source |
|
3 participants
Describe your changes
Stacked on #33011 (FilterSelect primitive) and #33017 (display fixes) — review only the top commits (
feat(ui): render Explore quick filters through FilterSelectandFilterSelect icon accepts rendered nodes); the rest belong to the parent PRs. Draft until both parents merge, then this rebases to a clean diff.Replaces both quick-filter renderers in
ExploreQuickFilterswith the unifiedFilterSelectcore component:SearchDropdownpath, anduntitledDropdownfork (QuickFilterDropdown), which existed only because AntD popups go inert inside SlideoutMenu drawers — react-aria popovers don't, so the fork is deleted.One renderer now covers Explore, Assets tabs (Glossary/Domain/Data Product/Tag/Team/Lineage), the Add Assets drawer, Lineage controls, Domains & Data Products listings, Test Definitions, and Column Bulk Operations — giving all of them the uniform checkbox/typography rows and, on multi-select filters, the Select All row (#29569).
What did NOT change
sourceFieldstop-hits casing, the request-id race guard — untouched. The swap is presentational.SearchDropdownitself stays — its 8 direct consumers (TableQueries, AuditLog, DataInsight, DQ dashboard, …) migrate in follow-up PRs.Details
ExploreQuickFilters— the legacy dropdowns debounced internally, FilterSelect reports each keystroke.onChangemaps selected values back to their existing option objects so hydrated labels survive; the null option maps toOM_NULL_FIELDexactly as before.iconaccepts already-rendered nodes (entity/service icons are pre-rendered elements) and ahelperTextslot (Explore's "pick values to refine").search-dropdown-<key>testid. Menu-internal E2E selectors (search-input, update-btn, checkbox rows) change with the DOM — Playwright specs will be updated based on CI results in this PR.Screen.Recording.2026-09-11.at.5.24.33.PM.mov
Testing
ExploreQuickFiltersunit suite rewritten against the FilterSelect surface: 39/39 pass (fetch behavior, race guards, facet scoping, static options, null option, selection mapping all preserved).🤖 Generated with Claude Code
Greptile Summary
The PR consolidates Explore quick-filter rendering on the shared FilterSelect component and updates its consumers and test coverage.
Confidence Score: 5/5
The PR appears safe to merge because no new actionable defect remains and all previous Greptile threads are resolved.
The changes since the previous review only adjust a Playwright helper to target the migrated FilterSelect row, and the current popover lifecycle does not establish a realistic duplicate-locator failure in that flow. All previous findings were manually resolved, so none remain outstanding.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart LR C[Explore and asset consumers] --> EQF[ExploreQuickFilters] EQF --> FS[Shared FilterSelect] FS --> T[Trigger and selected count] FS --> S[Search and fetched options] FS --> M[Single or multiple selection] M -->|Immediate| U[Update consumer filters] M -->|Staged| A[Apply or cancel] A --> UReviews (17): Last reviewed commit: "test(playwright): migrate the last hidde..." | Re-trigger Greptile