mikebridge commented on code in PR #42539:
URL: https://github.com/apache/superset/pull/42539#discussion_r4020563435
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/GroupByFilterCard.tsx:
##########
@@ -294,17 +285,75 @@ const GroupByFilterCard: FC<GroupByFilterCardProps> = ({
}) => {
const theme = useTheme();
const dataset = customizationItem.targets?.[0]?.datasetId;
+ // Semantic views and regular datasets have independent id sequences —
+ // the persisted type is what disambiguates a colliding numeric id
+ // (sc-111089). Absent type means a regular dataset.
+ const datasourceType = customizationItem.targets?.[0]?.datasourceType;
const [filterTitleRef, , titleElementsTruncated] = useTruncation();
- const [loading, setLoading] = useState(false);
const [isHoverCardVisible, setIsHoverCardVisible] = useState(false);
- const [columnOptions, setColumnOptions] = useState<
- { label: string; value: string }[]
- >([]);
- const [datasetName, setDatasetName] = useState<string | undefined>();
const dispatch = useDispatch();
+ // Legacy persisted targets may carry the id as a number, string, or
+ // {value} object; the card owns this normalization and hands the hook
+ // a clean id.
+ const normalizedDatasetId = useMemo(() => {
+ if (typeof dataset === 'number' || typeof dataset === 'string') {
+ return dataset;
+ }
+ if (typeof dataset === 'object' && dataset !== null && 'value' in dataset)
{
+ return (dataset as { value: string | number }).value;
+ }
+ return undefined;
+ }, [dataset]);
+
+ const {
+ name: datasetName,
+ columns: datasourceColumns,
+ loading,
+ error: datasourceError,
+ } = useDisplayControlDatasource(normalizedDatasetId, datasourceType);
+
+ const columnOptions = useMemo(
+ () =>
+ datasourceColumns
+ .filter(col => col.filterable !== false)
+ .map(col => ({
+ label: col.verbose_name || col.column_name || col.name || '',
+ value: col.column_name || col.name || '',
+ })),
+ [datasourceColumns],
+ );
+
+ // Surface load failures as a toast, once per binding — the hook can
+ // re-render (and StrictMode double-invokes effects) without re-toasting.
+ const toastedBindingRef = useRef<string | undefined>();
+ useEffect(() => {
+ if (!datasourceError || normalizedDatasetId === undefined) {
Review Comment:
Good catch, and this should be covered now. The hook records the failed
binding alongside the error (`useDisplayControlDatasource.ts:90`, set together
in the rejection handler) and scopes the returned error at render time —
`errorBinding === currentBinding ? error : undefined` — so the previous
binding's failure can't be paired with the new datasource in that first commit,
rather than relying on the clearing effect landing in time.
The regression sits at `GroupByFilterCard.test.tsx:325`: fail 310, switch to
311, assert exactly one toast and that none names 311. Does that cover the
sequence you reproduced?
##########
superset-frontend/src/components/Chart/DrillDetail/DrillDetailPane.test.tsx:
##########
@@ -378,3 +378,27 @@ test('should use verbose_map for column headers when
available', async () => {
screen.queryByRole('columnheader', { name: 'na_sales' }),
).not.toBeInTheDocument();
});
+
+/**
+ * sc-111089 T013: a semantic-view drill resource shows the view's name; the
+ * metadata rows the structure payload cannot fill render their explicit
+ * "Not available" state — an accepted, pinned degradation, never another
+ * object's values.
+ */
+test('renders a semantic-view resource with Not available metadata rows',
async () => {
Review Comment:
You were right that it doesn't exercise datasource resolution. We took your
second option: kept the rendering test and corrected its docblock at `:395` to
say it pins only that rendering degradation, with resolution coverage pointed
at `hooks/apiResources/datasets.test.ts` — which does go through
`useDatasetDrillInfo('3__semantic_view', …)` and asserts zero `/drill_info/`
calls.
Does that scope note settle it, or would you still rather it were retargeted?
--
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]
