EnxDev commented on PR #40905:
URL: https://github.com/apache/superset/pull/40905#issuecomment-5102330463
EnxDev's Review Agent β apache/superset#40905 Β· HEAD 374626d5
comment β The registry-driven dependency check is the right shape and well
tested, but the child-side gate silently widens cascade config to two core
filters, and the diff carries unrelated reverts on a 983-commit-stale base.
Context: replaces ALLOW_DEPENDENCIES with registry lookups
(filterCanBeDependencyParent / filterCanHaveDependencies) + adds
supportsCascadeDependencies to ChartMetadata and isColumnSelect to
getControlItemsMap. Refs discussion #26084 (no SIP). CI fully green. Most
existing bot comments are stale β they flag datasetLabel, operatorType and
time_grains removals that are not in the current diff (all three are still
present at FiltersConfigForm.tsx:1062, getControlItemsMap.tsx:317,
FiltersConfigForm.tsx:1305).
π΄ Functional
- hooks/useFilterOperations.ts:46 Β· High β filterCanHaveDependencies gates
only on Behavior.NativeFilter, so Time column and Time grain filters now render
the "Filter is dependent on other filters" section that ALLOW_DEPENDENCIES
(filter_range/filter_select/filter_time) excluded. Not cosmetic: both derive
options from the chart data response (TimeColumnFilterPlugin.tsx:78,
TimeGrainFilterPlugin.tsx:110), so parent extraFormData really does reshape
their option lists. The two capabilities are also asymmetric β a plugin can opt
out of being a cascade parent via supportsCascadeDependencies, but has no way
to opt out of being a child. Confirm this widening is intended, and add a
matching child-side flag. regression test: a FiltersConfigModal test asserting
the dependency section renders for filter_select and not for filter_timecolumn
β useFilterOperations.test.ts only pins the predicate.
- getControlItemsMap.tsx:82 Β· Medium β resolveInitialValue's new ?? null
tail turns "no saved value and no config.default" from undefined into null.
That null is registered as the antd initialValue, so it lands in controlValues
and is persisted (transformers/filterTransformer.ts:124). Plugin code that
destructures with = defaults gets null instead of the default, since = only
fires on undefined. Every core filter control defines a default so core is
unaffected β but this is exactly the third-party path the PR exists to enable.
Drop the ?? null. regression test: assert getControlItemsMap yields checked ===
undefined for a control with no default and no saved value.
π‘ Should-fix
- FiltersConfigModal.tsx:475 β unrelated rewrite of handleValuesChange:
master iterates every changed id (Object.keys(changedValues?.filters ??
{}).forEach), this narrows to Object.keys(changedValues.filters)[0] and deletes
the comment explaining why the hook exists; useCallbackβuseMemo is pure churn.
Revert to master's version.
- FiltersConfigForm.tsx:950,1008 β drops getOptionDataTest and both
optionRender props, removing the data-test="filter-type-option-*" /
customization-type-option-* hooks added by #38922. Unrelated to this PR;
restore.
- getControlItemsMap.tsx:381 β isColumnSelect is a new plugin-facing
control-config field that is never declared; it only compiles because
BaseControlConfig extends AnyDict. Declare it in
superset-ui-chart-controls/src/types.ts so plugin authors get types, the way
supportsCascadeDependencies was properly added to ChartMetadataConfig.
- docs β neither supportsCascadeDependencies nor isColumnSelect appears
anywhere under docs/. Third-party discoverability is the entire premise of the
PR.
- rebase β merge base is 045674ab (2026-06-19), 983 commits behind master.
FiltersConfigForm.tsx, ColumnSelect.tsx, DatasetSelect.tsx and
FiltersConfigForm/utils.ts have all changed substantially since, and
transformers/buildTarget.ts was deleted. The green CI is against a base that no
longer reflects master.
π΅ Nits
- getControlItemsMap.tsx:407 β the column-picker path ignores disabled,
unlike the checkbox path at :347.
- getControlItemsMap.tsx:397 β fallbackLabel={controlItem.name} can surface
a raw untranslated identifier (myPluginColumn) as a user-facing label when a
plugin's label is an arg-taking function.
- getControlItemsMap.tsx:150 β effect deps are [datasetId], but the
!datasetId branch reads value from the closure while both async branches use
valueRef.current; use the ref there too.
π Praise
- getControlItemsMap.tsx:173 β the .catch keeps the current value instead of
wiping it, exactly as @rusackas asked; verified applied in the diff and pinned
by keeps existing value when fetch rejects (getControlItemsMap.test.tsx:485).
- hooks/useFilterOperations.test.ts β clean predicate coverage across
opt-in, opt-out, unset, unknown and undefined.
--
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]
