sadpandajoe commented on code in PR #39684:
URL: https://github.com/apache/superset/pull/39684#discussion_r4031372828
##########
superset-frontend/plugins/plugin-chart-table/src/buildQuery.ts:
##########
@@ -239,6 +243,19 @@ export const buildQuery: BuildQuery<TableChartFormData> = (
moreProps.row_offset = 0;
}
+ const visibleColumnKeys = Array.isArray(ownState?.visibleColumns)
+ ? ownState.visibleColumns.map(String)
+ : [];
+
+ if (isDownloadQuery && visibleColumnKeys.length > 0) {
+ postProcessing.push({
+ operation: 'select',
+ options: {
+ columns: visibleColumnKeys,
Review Comment:
With time comparison enabled, this sends UI-only keys such as `Main
<metric>` and `△ <metric>` to the backend `select` operator, but those names
are only created later in `transformProps`; the DataFrame still has the raw
metric/offset columns, so the download is rejected as referencing unavailable
columns. Could the export projection translate the selected display keys to the
actual post-processing column names, with a regression covering a
time-comparison download?
##########
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:
##########
@@ -843,13 +857,265 @@ 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;
+ }
+ const match = pathname.match(/\/superset\/dashboard\/(?:p\/)?([^/?]+)/);
+ const dashboardId = match ? match[1] : 'unknown';
+ return `superset.table.visibleColumns:${dashboardId}:${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') {
Review Comment:
This writes the current visible-column snapshot on every own-state update,
including the mount-time `clientView` effect, so merely viewing a chart records
today's full column list as an explicit preference. When an editor later adds a
column, returning viewers keep the old non-empty snapshot and silently never
see or export the new field; could storage be updated only after an actual
selector change, with a way to return to the default?
##########
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:
##########
@@ -843,13 +857,265 @@ 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;
+ }
+ const match = pathname.match(/\/superset\/dashboard\/(?:p\/)?([^/?]+)/);
+ const dashboardId = match ? match[1] : 'unknown';
+ return `superset.table.visibleColumns:${dashboardId}:${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 || !visibleColumnsStorageKey) {
+ return null;
+ }
+
+ const handleOnClick = ({ key }: { key: string }) => {
+ const targetKey = String(key);
+ const isSelected = selectedVisibleColumnKeys.includes(targetKey);
+ const nextVisibleColumns = isSelected
+ ? selectedVisibleColumnKeys.filter(columnKey => columnKey !==
targetKey)
+ : [...selectedVisibleColumnKeys, targetKey];
Review Comment:
Re-enabling a middle column appends its key here, while the table still
renders columns in metadata order and backend `select` preserves this array
order. After hiding and restoring `B` in `A, B, C`, the dashboard shows `A, B,
C` but the export becomes `A, C, B`; could the persisted selection be
normalized back to metadata order?
##########
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:
##########
@@ -843,13 +857,265 @@ 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;
+ }
+ const match = pathname.match(/\/superset\/dashboard\/(?:p\/)?([^/?]+)/);
+ const dashboardId = match ? match[1] : 'unknown';
+ return `superset.table.visibleColumns:${dashboardId}:${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 || !visibleColumnsStorageKey) {
+ return null;
+ }
+
+ const handleOnClick = ({ key }: { key: string }) => {
+ const targetKey = String(key);
+ const isSelected = selectedVisibleColumnKeys.includes(targetKey);
+ const nextVisibleColumns = isSelected
+ ? selectedVisibleColumnKeys.filter(columnKey => columnKey !==
targetKey)
+ : [...selectedVisibleColumnKeys, targetKey];
+
+ // Keep at least one visible column so table structure stays valid.
+ if (!nextVisibleColumns.length) {
+ return;
+ }
+
+ handleVisibleColumnsChange(nextVisibleColumns);
+ };
+
+ return (
+ <Dropdown
+ placement="bottomRight"
+ open={showColumnSelectorDropdown}
+ onOpenChange={(flag: boolean) => {
+ setShowColumnSelectorDropdown(flag);
+ }}
+ menu={{
+ selectable: true,
+ multiple: true,
+ onClick: handleOnClick,
+ selectedKeys: selectedVisibleColumnKeys,
+ items: [
+ {
+ key: 'columns-group',
+ label: (
+ <div
+ css={css`
+ max-width: 260px;
+ padding: 0 ${theme.sizeUnit * 2}px;
+ color: ${theme.colorText};
+ font-size: ${theme.fontSizeSM}px;
+ `}
+ >
+ {t(
+ 'Choose which columns should be visible in this table on
the dashboard.',
+ )}
+ </div>
+ ),
+ type: 'group',
+ children: selectableColumnsMeta.map(column => ({
+ key: String(column.key),
+ label: (
+ <>
+ <span
+ css={css`
+ color: ${theme.colorText};
+ `}
+ >
+ {column.config?.customColumnName ||
+ column.originalLabel ||
+ column.label ||
+ column.key}
+ </span>
+ <span
+ css={css`
+ float: right;
+ font-size: ${theme.fontSizeSM}px;
+ `}
+ >
+ {selectedVisibleColumnKeys.includes(
+ String(column.key),
+ ) && <CheckOutlined />}
+ </span>
+ </>
+ ),
+ })),
+ },
+ ],
+ }}
+ trigger={['click']}
+ >
+ <span>
Review Comment:
The dropdown trigger is a plain `span`, so it has no keyboard focus or
button semantics and keyboard-only dashboard users cannot reach the new Columns
control. Could this use the project's focusable button control, with keyboard
coverage for opening the menu?
##########
superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx:
##########
@@ -473,6 +473,38 @@ describe('plugin-chart-table', () => {
expect(cells[4]).toHaveTextContent('2.47k');
});
+ test('render columns dropdown when visibleColumnsStorageKey is
provided', async () => {
+ const props = transformProps(testData.advanced);
+ const persistTableOwnState = jest.fn();
+ render(
+ ProviderWrapper({
+ children: (
+ <TableChart
+ {...props}
+ visibleColumnsStorageKey="table_col_key"
+ persistTableOwnState={persistTableOwnState}
+ sticky={false}
Review Comment:
Agreed—the test injects props that `TableChart` neither declares nor reads,
while the component derives its dashboard gate from `window.location`, so this
case cannot exercise the selector. Could it set a dashboard pathname and assert
the real `setDataMask` or local-storage behavior instead?
--
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]
