EnxDev commented on PR #40905: URL: https://github.com/apache/superset/pull/40905#issuecomment-4961526931
## EnxDev's Review Agent — apache/superset#40905 · HEAD 8e50b80 **comment** — Core registry refactor is sound; the prettier CI blocker from my last pass is now green, and the remaining items are scope + minor guards, not blockers. Supersedes my earlier review at [c506e5d](https://github.com/apache/superset/pull/40905#pullrequestreview-4572661501). Since then, new commits landed and **CI is now fully green** — `pre-commit (current)` passes, so the prettier failure I flagged before is resolved. No new regressions in the delta. Note on the automated reviews above: several Copilot/codeant/bito findings target an earlier, larger force-pushed revision. Verified against the diff at HEAD 8e50b80, the following are **not present here** and should not gate merge — broken `handleValuesChange` divider tracking (it *does* call `handleModifyItem` via `isDivider`), `String(description)`→`[object Object]` (now passes the ReactNode through), the stale-response race (guarded by the `cancelled` flag), missing saved-value restore (injected via `Form.Item` `initialValue`), and the removed `operatorType`/`time_grains`/`datasetLabel` UI (none of those are in this diff). ### 🟡 Should-fix - **`getControlItemsMap.tsx:294`** — the `renderTrigger` checkbox loop still doesn't exclude `isColumnSelect` controls. A plugin declaring both `renderTrigger: true` and `isColumnSelect: true` renders twice — once as a checkbox (`controlItems`) and once as the column picker (`mainControlItems`). Add `&& !item.config?.isColumnSelect` to the filter as a cheap guard. **regression test:** a control config with both flags should yield exactly one element. - **Scope** — reinforcing @rusackas: the titled feature is the registry-driven dependency gate (`ChartMetadata` flag + `filterSupportsDependencies` + the 5 plugin flags). The `isColumnSelect`/`DatasetColumnSelect` column-picker API, the `ControlLabel`/`resolveInitialValue` refactors, and the `handleValuesChange` divider refactor are unrelated riders that widen the surface — and `isColumnSelect` ships a plugin-facing control API with zero in-tree consumers (exercised only via mocks). Worth splitting into its own PR. - **`useFilterOperations.ts:21`** — the behavior fallback makes *every* already-installed third-party `Behavior.NativeFilter` plugin cascade-capable by default, where before only `filter_range/select/time` qualified. That's the intended extensibility, but it's a silent default change for existing deployments — state it explicitly in the PR body/docs so the opt-out (`supportsCascadeDependencies: false`) is deliberate, not a surprise. ### 🔵 Nits - **`getControlItemsMap.tsx:159`** — `DatasetColumnSelect`'s `.catch` sets empty options with no user feedback; a failed dataset fetch leaves the picker silently empty. Keeping the saved value on a transient failure is right, but a `addDangerToast` (as the groupby `ColumnSelect` path does) would surface the failure. - **`FiltersConfigModal.tsx:474`** — `useCallback(fn, deps)` → `useMemo(() => fn, deps)` is churn; identical result, keep `useCallback`. It also now reads only `Object.keys(changedValues.filters)[0]` where master iterated all changed ids — safe because antd reports one changed leaf per event, but the single-key assumption is implicit; a one-line comment would help. - **`getControlItemsMap.tsx:210`** — `notifyChange` reorders the checkbox path (`formChanged(); forceUpdate()` → `forceUpdate(); formChanged()`). Harmless, but an unflagged behavior reorder inside a "refactor." - **`FiltersConfigForm.tsx:950`** — dropping `optionRender`/`getOptionDataTest` removes `data-test` hooks from the filter-type/customization-type options. Confirmed nothing else references those strings (safe), but it's unrelated scope. ### 🙌 Praise - **`TimeColumn`/`TimeGrain` `index.ts`** — explicitly setting `supportsCascadeDependencies: false` preserves the prior exclusion while the `undefined`→NativeFilter fallback keeps third-party plugins extensible. That's the subtle part that makes the refactor behavior-preserving — correct call. - **`getControlItemsMap.tsx:143`** — `DatasetColumnSelect` guards both the stale-response race (`cancelled` flag keyed on `datasetId`) and fetch errors, and the new tests cover stale-value reset, fetch-reject fallback, and dataset-cleared reset. Good async hygiene and coverage. <!-- enxdev-review-agent:8e50b80 --> _Reviewed by EnxDev's Review Agent — @EnxDev · HEAD 8e50b80._ -- 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]
