Copilot commented on code in PR #39684:
URL: https://github.com/apache/superset/pull/39684#discussion_r3236277561
##########
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:
##########
@@ -739,13 +753,263 @@ export default function TableChart<D extends DataRecord
= DataRecord>(
);
};
- // Compute visible columns before groupHeaderColumns to ensure index
consistency.
- // This filters out columns with config.visible === false.
- const visibleColumnsMeta = useMemo(
+ const selectableColumnsMeta = useMemo(
() => filteredColumnsMeta.filter(col => col.config?.visible !== false),
[filteredColumnsMeta],
);
+ const selectableColumnKeys = useMemo(
+ () => selectableColumnsMeta.map(col => String(col.key)),
+ [selectableColumnsMeta],
+ );
+
+ const visibleColumnsStorageKey = useMemo(() => {
+ if (typeof window === 'undefined') {
+ return null;
+ }
+ const pathname = window.location?.pathname || '';
+ if (!pathname.includes(DASHBOARD_PATH_SEGMENT)) {
+ return null;
+ }
+ return `superset.table.visibleColumns:${pathname}:${slice_id}`;
+ }, [slice_id]);
+
+ const storedVisibleColumnKeys = useMemo(() => {
+ if (!visibleColumnsStorageKey || typeof window === 'undefined') {
+ return [];
+ }
+ try {
+ const raw = window.localStorage.getItem(visibleColumnsStorageKey);
+ if (!raw) {
+ return [];
+ }
+ const parsed = JSON.parse(raw);
+ return sanitizeVisibleColumnSelection(
+ parsed,
+ new Set(selectableColumnKeys),
+ );
+ } catch {
+ return [];
+ }
+ }, [selectableColumnKeys, visibleColumnsStorageKey]);
+
+ const [selectedVisibleColumnKeys, setSelectedVisibleColumnKeys] = useState<
+ string[]
+ >(() => {
+ const availableKeys = new Set(selectableColumnKeys);
+ const selectedFromOwnState = sanitizeVisibleColumnSelection(
+ serverPaginationData?.visibleColumns,
+ availableKeys,
+ );
+ if (selectedFromOwnState.length > 0) {
+ return selectedFromOwnState;
+ }
+ if (storedVisibleColumnKeys.length > 0) {
+ return storedVisibleColumnKeys;
+ }
+ return selectableColumnKeys;
+ });
+
+ useEffect(() => {
+ setSelectedVisibleColumnKeys(prevSelection => {
+ const availableKeys = new Set(selectableColumnKeys);
+ const sanitizedCurrent = prevSelection.filter(key =>
+ availableKeys.has(key),
+ );
+
+ const selectedFromOwnState = sanitizeVisibleColumnSelection(
+ serverPaginationData?.visibleColumns,
+ availableKeys,
+ );
+ if (selectedFromOwnState.length > 0) {
+ return isEqual(sanitizedCurrent, selectedFromOwnState)
+ ? sanitizedCurrent
+ : selectedFromOwnState;
+ }
+
+ if (storedVisibleColumnKeys.length > 0) {
+ return isEqual(sanitizedCurrent, storedVisibleColumnKeys)
+ ? sanitizedCurrent
+ : storedVisibleColumnKeys;
+ }
+
+ if (sanitizedCurrent.length > 0) {
+ return sanitizedCurrent;
+ }
+
+ return selectableColumnKeys;
+ });
+ }, [
+ selectableColumnKeys,
+ serverPaginationData?.visibleColumns,
+ storedVisibleColumnKeys,
+ ]);
+
+ const persistTableOwnState = useCallback(
+ (updates: Record<string, unknown>) => {
+ const selectedKeys =
+ (updates.visibleColumns as string[] | undefined) ??
+ selectedVisibleColumnKeys;
+ if (visibleColumnsStorageKey && typeof window !== 'undefined') {
+ try {
+ if (selectedKeys?.length) {
+ window.localStorage.setItem(
+ visibleColumnsStorageKey,
+ JSON.stringify(selectedKeys),
+ );
+ } else {
+ window.localStorage.removeItem(visibleColumnsStorageKey);
+ }
+ } catch {
+ // no-op: storage write failures should not block table interactions
+ }
+ }
+ updateTableOwnState(setDataMask, {
+ ...serverPaginationData,
+ ...updates,
+ ...(selectedKeys?.length ? { visibleColumns: selectedKeys } : {}),
+ });
+ },
+ [
+ selectedVisibleColumnKeys,
+ serverPaginationData,
+ setDataMask,
+ visibleColumnsStorageKey,
+ ],
+ );
+
+ useEffect(() => {
+ if (storedVisibleColumnKeys.length === 0) {
+ return;
+ }
+ const selectedFromOwnState = sanitizeVisibleColumnSelection(
+ serverPaginationData?.visibleColumns,
+ new Set(selectableColumnKeys),
+ );
+ if (
+ selectedFromOwnState.length === 0 &&
+ selectedVisibleColumnKeys.length > 0
+ ) {
+ persistTableOwnState({ visibleColumns: selectedVisibleColumnKeys });
+ }
+ }, [
+ persistTableOwnState,
+ selectableColumnKeys,
+ selectedVisibleColumnKeys,
+ serverPaginationData?.visibleColumns,
+ storedVisibleColumnKeys.length,
+ ]);
+
+ const handleVisibleColumnsChange = useCallback(
+ (nextVisibleColumns: string[]) => {
+ if (isEqual(nextVisibleColumns, selectedVisibleColumnKeys)) {
+ return;
+ }
+ setSelectedVisibleColumnKeys(nextVisibleColumns);
+ persistTableOwnState({ visibleColumns: nextVisibleColumns });
+ },
+ [persistTableOwnState, selectedVisibleColumnKeys],
+ );
+
+ const renderColumnSelectDropdown = (): JSX.Element | null => {
+ if (!selectableColumnsMeta.length) {
+ return null;
+ }
Review Comment:
The PR description says the Columns selector is **only shown/active on
dashboards**, but `renderColumnSelectDropdown` currently renders whenever there
are selectable columns (regardless of route/context). Since
`visibleColumnsStorageKey` is already `null` when not on a dashboard path,
consider additionally gating the dropdown itself (e.g., return `null` when
`visibleColumnsStorageKey` is null) so Explore / chart view don’t get this
control.
##########
superset-frontend/plugins/plugin-chart-table/src/buildQuery.ts:
##########
@@ -228,6 +248,34 @@ const buildQuery: BuildQuery<TableChartFormData> = (
moreProps.row_offset = 0;
}
+ const visibleColumnKeys = Array.isArray(ownState?.visibleColumns)
+ ? ownState.visibleColumns.map(String)
+ : [];
+ const visibleColumnSet = new Set(visibleColumnKeys);
+
+ if (isDownloadQuery && visibleColumnSet.size > 0) {
+ columns = columns.filter(column =>
+ visibleColumnSet.has(
+ getColumnSelectionKey(column as string | AdhocColumn),
+ ),
+ );
+
+ if (Array.isArray(metrics) && metrics.length > 0) {
+ metrics = metrics.filter(metric =>
+ visibleColumnSet.has(getMetricLabel(metric)),
+ );
+ }
+
+ if (Array.isArray(orderby) && orderby.length > 0) {
+ orderby = orderby.filter(([orderValue]) => {
+ if (typeof orderValue === 'string') {
+ return visibleColumnSet.has(orderValue);
+ }
+ return visibleColumnSet.has(getMetricLabel(orderValue));
+ });
+ }
+ }
+
Review Comment:
Filtering `queryObject.columns` / `metrics` for **download queries** can
change the *semantic* result, not just the exported projection—especially in
`QueryMode.Aggregate`, where `columns` represent GROUP BY fields. If a viewer
hides a group-by column, this code will remove it from the query and change
aggregation granularity (potentially collapsing rows and changing metric
values). To keep exports consistent with the on-screen table (same rows, fewer
visible fields), avoid removing group-by columns from the query; instead,
project/hide columns at a post-processing/export step (or apply selection only
after the aggregation has occurred). Also, ensure the selection key used here
matches exactly what the UI stores (TableChart stores `String(col.key)`),
otherwise downloads may unexpectedly drop all columns/metrics.
##########
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:
##########
@@ -739,13 +753,263 @@ export default function TableChart<D extends DataRecord
= DataRecord>(
);
};
- // Compute visible columns before groupHeaderColumns to ensure index
consistency.
- // This filters out columns with config.visible === false.
- const visibleColumnsMeta = useMemo(
+ const selectableColumnsMeta = useMemo(
() => filteredColumnsMeta.filter(col => col.config?.visible !== false),
[filteredColumnsMeta],
);
+ const selectableColumnKeys = useMemo(
+ () => selectableColumnsMeta.map(col => String(col.key)),
+ [selectableColumnsMeta],
+ );
+
+ const visibleColumnsStorageKey = useMemo(() => {
+ if (typeof window === 'undefined') {
+ return null;
+ }
+ const pathname = window.location?.pathname || '';
+ if (!pathname.includes(DASHBOARD_PATH_SEGMENT)) {
+ return null;
+ }
+ return `superset.table.visibleColumns:${pathname}:${slice_id}`;
Review Comment:
The persistence key includes the full `pathname`, which can differ for the
same dashboard (e.g., `/edit`, embedded paths, or other dashboard sub-routes).
That can lead to the same dashboard+chart having multiple independent stored
selections, which conflicts with the stated goal of persisting per dashboard +
chart. Consider normalizing the key to a stable dashboard identifier (dashboard
id/uuid) rather than the entire pathname.
--
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]
