EnxDev commented on PR #43277:
URL: https://github.com/apache/superset/pull/43277#issuecomment-5339622296
## EnxDev's Review Agent — apache/superset#43277 · HEAD 3c64e55
**request changes** — the `useFilters()` change silently regresses every
existing dashboard (filter-bar dividers disappear, "Clear all" skips canvas
filters, required filters stop gating), and two advertised controls are wired
up but never read.
Context: reviewed the full 3.3k-line diff against `master` @ 097c99b. Linked
issue #41498 is referenced but has no design doc I could read. Note that **CI
has not run the real gates on this PR** — only `labeler` succeeded; frontend
lint, `tsc`, and Jest were never executed, so none of the below is
machine-checked. The prior CodeAnt/Bito comments partly target files that no
longer exist in this diff (`CustomControlsFilterPlugin.tsx`,
`DraggableFilter.tsx`) — I re-derived everything from the current head.
### 🔴 Functional
- **`FilterBar/state.ts:61`** · _High_ — `useFilters()` now drops every
entry with `type !== 'DIVIDER'`. `useFilterControlFactory` builds its list from
`useFilters()` and branches on `isFilterDivider(filter)` to render
`<FilterDivider>` (`NativeFilterType.Divider === 'DIVIDER'`), so **all
native-filter dividers vanish from the filter bar on every dashboard** —
unconditionally, no feature flag, whether or not anyone uses a Filter Card.
Restrict the predicate to `!canvasFilterIds.has(filter.id)` and keep dividers.
**regression test:** render `FilterBar` with a config containing a `DIVIDER`
entry; assert the divider still renders.
- **`FilterBar/index.tsx:512`** · _High_ — `handleClearAll` iterates
`nativeFilterValues`, derived from `useFilters()`. Canvas-bound filters are
excluded there, so **"Clear all" silently leaves them applied** while the UI
reports everything cleared; charts stay filtered. Keep canvas filters in the
lifecycle data and exclude them only from the bar's rendering. **regression
test:** bind a filter to a `FilterHolder`, set a value, click Clear all, assert
its `dataMask` is cleared.
- **`DashboardBuilder/state.ts:99`** · _High_ — same root cause:
`requiredFirstFilter` / `missingInitialFilters` come from `useFilters()`. Once
a `requiredFirst` filter is bound to a Filter Card it drops out of that set, so
**the dashboard renders charts without the mandatory value** instead of showing
the "select a value first" gate. **regression test:** `requiredFirst` filter
bound to a canvas component → `showDashboard` stays false until a value is set.
- **`DateTimeFilter/DateTimeFilterPlugin.tsx:637`** · _High_ — in
`emitFilter`, when `col` is set but `fetchTimeRange` errors or returns fewer
than 2 date matches, `extra` stays `{}` while `filterState` is still published
as `{ value: rangeStr, label: rangeStr }`. The filter renders as applied with
the range label, but **no filtering happens** — a silent no-op the user cannot
distinguish from success. The `catch` has the same gap (it only repairs the
`!col` branch). Set `validateStatus: 'error'` and skip publishing a success
label when resolution fails. **regression test:** mock `fetchTimeRange` to
reject with `col` set; assert no mask claiming an applied range.
- **`CheckboxRadio/controlPanel.ts:87` and
`DateTimeFilter/controlPanel.ts:52`** · _High_ — both plugins expose "Filter
value is required" (`enableEmptyFilter`), and neither ever reads it.
`CheckboxRadio/transformProps.ts` doesn't forward it; `DateTimeFilterPlugin`
never touches `formData.enableEmptyFilter`. Compare
`SelectFilterPlugin.tsx:220`, which computes `emptyFilter` and drives
`validateStatus`. **Marking these filters required does nothing** — empty
selections apply and clear the filter. Implement it or remove the control.
**regression test:** `enableEmptyFilter: true` + empty value → emitted mask
carries `validateStatus: 'error'` and empty `extraFormData`.
- **`FilterHolder.tsx:186`** · _Medium_ — `handleClearStagedFilter`
dispatches `updateDataMask` straight to Redux, but it only renders when
`applyMode === 'manual'`, where ordinary selections stage in `stagedDataMask`
until Apply. **Clear bypasses the manual-apply workflow** and refreshes charts
before the user clicks Apply. Stage the cleared mask and commit it through
Apply. **regression test:** manual mode, set a value, click Clear → Redux
`dataMask` unchanged until Apply.
- **`CheckboxRadio/CheckboxRadioFilterPlugin.tsx:166`** · _Medium_ — the
sync effect guards on `filterState?.value !== undefined`, so an external reset
to `undefined` (Clear all, `getInitialDataMask`) never reaches `localValue`.
**The boxes stay visually checked while the filter is actually cleared.** Sync
unconditionally. **regression test:** rerender with `filterState.value ===
undefined`; assert no option is checked.
### 🟡 Should-fix
- **`packages/superset-core/src/theme/GlobalStyles.tsx:96-125`** — a global
`!important` block on `.ant-select-dropdown`, `.ant-picker-dropdown`,
`.ant-dropdown-menu` forces background, shadow, a new 1px border, and
`font-weight: 600` on selected options for **every dropdown in the app** (SQL
Lab, Explore, list views), and makes them un-overridable by any component that
themes its own popup. Scope this to the filter-card subtree, or fix the
underlying token instead of blanket `!important`.
-
**`packages/superset-ui-core/src/components/PopoverDropdown/index.tsx:96`** —
`theme.zIndexBase` (0) → `theme.zIndexPopupBase` (1000) changes stacking for
every `PopoverDropdown` consumer, not just this feature. The `|| 3100` fallback
is dead — `zIndexPopupBase` is always 1000. Confirm no consumer relied on these
sitting below other layers.
- **No feature flag** — a new dashboard layout element plus two plugins
registered in `MainPreset.ts` ship on by default. Convention is
`DEFAULT_FEATURE_FLAGS` in `superset/config.py` with
`docs/static/feature-flags.json` kept in sync; for a change that alters
`useFilters()` for everyone, gating matters.
- **`DateTimeFilter/index.ts:35`** — `datasourceCount: 1` with no
`buildQuery`. `FilterValue` fires `getChartDataRequest` whenever a dataset is
bound, so the default `buildQuery` runs a real query per DateTime filter on
every dashboard load — and the plugin never reads `queriesData`. Use
`datasourceCount: 0` like `TimeFilterPlugin`, or supply a minimal `buildQuery`.
- **`FilterHolder.test.tsx:142`** — the mock renders `data-test`, but Jest
has no global `testIdAttribute` override (only `playwright.config.ts` sets it),
so `getByTestId` looks for `data-testid` and this test fails.
`NewFilterComponent.test.tsx` sets both attributes; this file doesn't. Confirms
the earlier CodeAnt comment.
- **Test coverage** — ~1,600 new lines across `CheckboxRadio/*` and
`DateTimeFilter/*` ship with zero tests, and the ~140 new lines in
`getControlItemsMap.tsx` (Select/Text rendering, `visibility` evaluation) add
nothing to the existing `getControlItemsMap.test.tsx`. At minimum: `emitFilter`
failure path, `enableEmptyFilter`, and the canvas/bar rendering split.
- **`FilterHolder.tsx:168`** — canvas selections dispatch the raw mask,
keeping `validateStatus`. `FilterBar`'s own handler strips it first ("UI-only
state and shouldn't persist in Redux"); this path leaks it into `dataMask` and
anything derived from it.
- **`getControlItemsMap.tsx:187`** and **`DateTimeFilter/types.ts:42`** —
`as any` casts to synthesize the `visibility()` argument, and `datasource?:
any`. The repo bans `any`; use a typed partial and the core `Datasource` type.
### 🔵 Nits
- `DateTimeFilterPlugin.tsx:443` — `hasTime`'s third branch returns `true`
and so does the fallback; the `includes(':')` check is dead.
- `DateTimeFilterPlugin.tsx:716` — the mount effect re-emits the filter and
re-fetches the time range on every remount, with `exhaustive-deps` suppressed.
- `DateTimeFilterPlugin.tsx:1032` — `t(label)` over a `TAB_CONFIG` variable
won't be picked up by the i18n extractor; wrap the strings at definition
(`:59`).
- Hardcoded px throughout both plugins (`gap: 8px`, `font-size: 11px`,
`min-height: 290px`) instead of `theme.sizeUnit * n` / `theme.fontSizeSM`, plus
many inline `style={{…}}` props (`DateTimeFilterPlugin.tsx:834, 846, 1041`;
`CheckboxRadioFilterPlugin.tsx:224`) and raw `<input className="ant-input">` /
`<button>` where core `Input`/`Button` exist.
- `FilterBar/state.ts:78` — `useAllFilters` is exported and never called;
`DateTimeFilterPlugin.tsx:45` imports `useLocale`/`dayjs` after relative
imports. Also worth running Prettier locally —
`CheckboxRadio/transformProps.ts:87` and `types.ts` look unformatted, and lint
never ran here.
### 🙌 Praise
- `FILTER_TYPE` is threaded consistently through every layout util —
`componentIsResizable`, `getDetailedComponentWidth`, `isValidChild`,
`isDashboardEmpty`, `newComponentFactory` — with tests added for the resizable
and width paths. That's the easy half to miss.
- `FiltersConfigForm/utils.ts` — the `doesColumnMatchFilterType` rewrite
correctly handles temporal-only filter types via `is_dttm`, with four new tests
covering both directions.
<!-- enxdev-review-agent:3c64e55 -->
_Reviewed by EnxDev's Review Agent — @EnxDev · HEAD 3c64e55._
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
