Copilot commented on code in PR #36281:
URL: https://github.com/apache/superset/pull/36281#discussion_r2578304589
##########
superset-frontend/src/explore/components/useExploreAdditionalActionsMenu/index.jsx:
##########
@@ -619,6 +873,7 @@ export const useExploreAdditionalActionsMenu = (
showDashboardSearch,
slice,
theme.sizeUnit,
+ ownState,
Review Comment:
The `hasExportCurrentView` variable is used in the menu construction (line
749) but is not included in the `useMemo` dependency array. This could cause
the menu to show incorrect export options when the viz_type changes. Add
`hasExportCurrentView` to the dependency array at line 854.
```suggestion
ownState,
hasExportCurrentView,
```
##########
superset-frontend/plugins/plugin-chart-table/src/index.ts:
##########
@@ -39,6 +39,7 @@ const metadata = new ChartMetadata({
Behavior.InteractiveChart,
Behavior.DrillToDetail,
Behavior.DrillBy,
+ 'EXPORT_CURRENT_VIEW' as any,
],
Review Comment:
[nitpick] Using `as any` to add a custom behavior bypasses TypeScript's type
safety. Consider either:
1. Adding `EXPORT_CURRENT_VIEW` to the `Behavior` enum in
`@superset-ui/core` if this is intended to be a standard behavior, or
2. Extending the behaviors array type to support string literals for custom
behaviors.
This would make the implementation more type-safe and maintainable.
```suggestion
'EXPORT_CURRENT_VIEW',
] as (Behavior | 'EXPORT_CURRENT_VIEW')[],
```
##########
superset-frontend/plugins/plugin-chart-table/src/DataTable/DataTable.tsx:
##########
@@ -424,6 +429,45 @@ export default typedMemo(function DataTable<D extends
object>({
onServerPaginationChange(pageNumber, serverPageSize);
}
+ // Emit filtered rows to parent in client-side mode (debounced via RAF)
+ const isMountedRef = useRef(true);
+ useEffect(() => {
+ isMountedRef.current = true;
+ return () => {
+ isMountedRef.current = false;
+ };
+ }, []);
+
+ const rafRef = useRef<number | null>(null);
+ const lastSigRef = useRef<string>('');
+
+ useEffect(() => {
+ if (serverPagination || typeof onFilteredRowsChange !== 'function') {
+ return;
+ }
+
+ const filtered = rows.map(r => r.original as D);
+ const len = filtered.length;
+ const first = len ? Object.values(filtered[0] as any)[0] : '';
+ const last = len ? Object.values(filtered[len - 1] as any)[0] : '';
+ const sig = `${len}|${String(first)}|${String(last)}`;
Review Comment:
[nitpick] The signature generation logic uses `Object.values(filtered[0] as
any)[0]` to get the first value of the first row. This approach is fragile
because:
1. If the first column value is `null`, `undefined`, or an empty string, the
signature might incorrectly indicate no change
2. Column order changes would not be detected if the first value remains the
same
3. The `as any` type assertion bypasses type safety
Consider using a more robust approach, such as computing a hash of the
entire filtered dataset or using a combination of row count and a stable
identifier (e.g., first row's ID field if available).
##########
superset-frontend/src/explore/components/useExploreAdditionalActionsMenu/index.jsx:
##########
@@ -462,7 +633,7 @@ export const useExploreAdditionalActionsMenu = (
},
{
key: MENU_KEYS.DOWNLOAD_AS_IMAGE,
- label: t('Download as image'),
+ label: t('Export screenshot (jpeg)'),
Review Comment:
[nitpick] The label text was changed from "Download as image" to "Export
screenshot (jpeg)", which is more descriptive. However, this menu item remains
in the `currentViewChildren` array (used for "Export Current View") even though
screenshots capture the entire visible chart and are not filtered by the
current data view. Consider moving this item to a separate menu section or
clarifying that it captures the visual representation rather than being
data-dependent.
##########
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:
##########
@@ -1320,6 +1321,49 @@ export default function TableChart<D extends DataRecord
= DataRecord>(
}
};
+ // collect client-side filtered rows for export & push snapshot to ownState
(guarded)
+ const [clientViewRows, setClientViewRows] = useState<DataRecord[]>([]);
+
+ const exportColumns = useMemo(
+ () =>
+ visibleColumnsMeta.map(col => ({
+ key: col.key,
+ label: col.config?.customColumnName || col.originalLabel || col.key,
+ })),
+ [visibleColumnsMeta],
+ );
+
+ const clientViewSigRef = useRef<string>('');
+ useEffect(() => {
+ if (serverPagination) return; // only for client-side mode
+
+ const len = clientViewRows.length;
+ const first = len ? clientViewRows[0]?.[exportColumns[0]?.key ?? ''] : '';
+ const last = len
+ ? clientViewRows[len - 1]?.[exportColumns[0]?.key ?? '']
+ : '';
+ const colSig = exportColumns.map(c => c.key).join(',');
+ const sig = `${len}|${String(first)}|${String(last)}|${colSig}`;
+
+ if (sig !== clientViewSigRef.current) {
+ clientViewSigRef.current = sig;
+ updateTableOwnState(setDataMask, {
+ ...(serverPaginationData || {}),
+ clientView: {
+ rows: clientViewRows,
+ columns: exportColumns,
+ count: len,
Review Comment:
[nitpick] Similar to the issue in DataTable.tsx, the signature generation in
TableChart uses a simple concatenation of first/last values and column keys.
This approach has similar fragility issues:
1. Changes to middle rows won't be detected
2. Column value changes in non-first/non-last rows won't trigger updates
3. The signature could have false positives if different datasets happen to
have the same first/last values
While this optimization helps avoid unnecessary state updates, consider
documenting these limitations in a comment or using a more comprehensive
approach (e.g., shallow equality check on the arrays themselves).
```suggestion
// Use a ref to store previous clientViewRows and exportColumns for robust
change detection
const prevClientViewRef = useRef<{ rows: DataRecord[]; columns: typeof
exportColumns } | null>(null);
useEffect(() => {
if (serverPagination) return; // only for client-side mode
const prev = prevClientViewRef.current;
const rowsChanged = !prev || !isEqual(prev.rows, clientViewRows);
const columnsChanged = !prev || !isEqual(prev.columns, exportColumns);
if (rowsChanged || columnsChanged) {
prevClientViewRef.current = { rows: clientViewRows, columns:
exportColumns };
updateTableOwnState(setDataMask, {
...(serverPaginationData || {}),
clientView: {
rows: clientViewRows,
columns: exportColumns,
count: clientViewRows.length,
```
##########
superset-frontend/plugins/plugin-chart-table/src/DataTable/DataTable.tsx:
##########
@@ -424,6 +429,45 @@ export default typedMemo(function DataTable<D extends
object>({
onServerPaginationChange(pageNumber, serverPageSize);
}
+ // Emit filtered rows to parent in client-side mode (debounced via RAF)
+ const isMountedRef = useRef(true);
+ useEffect(() => {
+ isMountedRef.current = true;
+ return () => {
+ isMountedRef.current = false;
+ };
+ }, []);
+
+ const rafRef = useRef<number | null>(null);
+ const lastSigRef = useRef<string>('');
+
+ useEffect(() => {
+ if (serverPagination || typeof onFilteredRowsChange !== 'function') {
+ return;
+ }
+
+ const filtered = rows.map(r => r.original as D);
+ const len = filtered.length;
+ const first = len ? Object.values(filtered[0] as any)[0] : '';
+ const last = len ? Object.values(filtered[len - 1] as any)[0] : '';
+ const sig = `${len}|${String(first)}|${String(last)}`;
+
+ if (sig !== lastSigRef.current) {
+ lastSigRef.current = sig;
+
+ rafRef.current = requestAnimationFrame(() => {
+ if (isMountedRef.current) onFilteredRowsChange(filtered);
+ });
+ }
Review Comment:
There's a potential race condition with the `requestAnimationFrame`
scheduling. When the signature changes (line 455), a new RAF is scheduled
without canceling the previous one first. This could result in multiple pending
RAF callbacks. Consider canceling any existing `rafRef.current` before
scheduling a new one:
```javascript
if (sig !== lastSigRef.current) {
lastSigRef.current = sig;
if (rafRef.current != null) {
cancelAnimationFrame(rafRef.current);
}
rafRef.current = requestAnimationFrame(() => {
if (isMountedRef.current) onFilteredRowsChange(filtered);
});
}
```
##########
superset-frontend/src/explore/components/useExploreAdditionalActionsMenu/index.jsx:
##########
@@ -307,6 +319,116 @@ export const useExploreAdditionalActionsMenu = (
}
}, [addDangerToast, addSuccessToast, latestQueryFormData]);
+ // Minimal client-side CSV builder used for "Current View" when pagination
is disabled
+ const downloadClientCSV = (rows, columns, filename) => {
+ if (!rows?.length || !columns?.length) return;
+ const esc = v => {
+ if (v === null || v === undefined) return '';
+ const s = String(v);
+ const wrapped = /[",\n]/.test(s) ? `"${s.replace(/"/g, '""')}"` : s;
+ return wrapped;
+ };
+ const header = columns.map(c => esc(c.label ?? c.key ?? '')).join(',');
+ const body = rows
+ .map(r => columns.map(c => esc(r[c.key])).join(','))
+ .join('\n');
+ const csv = `${header}\n${body}`;
+ const blob = new Blob([csv], { type: 'text/csv;charset=utf-8;' });
+ const link = document.createElement('a');
+ link.href = URL.createObjectURL(blob);
+ link.download = `${filename || 'current_view'}.csv`;
+ document.body.appendChild(link);
+ link.click();
+ document.body.removeChild(link);
+ URL.revokeObjectURL(link.href);
+ };
+
+ // Robust client-side JSON for "Current View"
+ const downloadClientJSON = (rows, columns, filename) => {
+ if (!rows?.length || !columns?.length) return;
+
+ const norm = v => {
+ if (v instanceof Date) return v.toISOString();
+ if (v && typeof v === 'object' && 'input' in v && 'formatter' in v) {
+ const dv = v.input ?? v.value ?? v.toString?.() ?? '';
+ return dv instanceof Date ? dv.toISOString() : dv;
+ }
+ return v;
+ };
+
+ const data = rows.map(r => {
+ const out = {};
+ columns.forEach(c => {
+ out[c.key] = norm(r[c.key]);
+ });
+ return out;
+ });
+
+ const meta = {
+ columns: columns.map(c => ({
+ key: c.key,
+ label: c.label ?? c.key,
+ })),
+ count: rows.length,
+ };
+
+ const payload = { meta, data };
+ const blob = new Blob([JSON.stringify(payload, null, 2)], {
+ type: 'application/json;charset=utf-8;',
+ });
+ const link = document.createElement('a');
+ link.href = URL.createObjectURL(blob);
+ link.download = `${filename || 'current_view'}.json`;
+ document.body.appendChild(link);
+ link.click();
+ document.body.removeChild(link);
+ URL.revokeObjectURL(link.href);
+ };
+
+ // NEW: Client-side XLSX for "Current View" (uses 'xlsx' already in deps)
+ const downloadClientXLSX = async (rows, columns, filename) => {
+ if (!rows?.length || !columns?.length) return;
+ try {
+ const XLSX = (await import(/* webpackChunkName: "xlsx" */ 'xlsx'))
+ .default;
+
+ // Build a flat array of objects keyed by backend column key
+ const data = rows.map(r => {
+ const o = {};
+ columns.forEach(c => {
+ const v = r[c.key];
+ o[c.label ?? c.key] =
+ v && typeof v === 'object' && 'input' in v && 'formatter' in v
+ ? v.input instanceof Date
+ ? v.input.toISOString()
+ : (v.input ?? v.value ?? '')
+ : v instanceof Date
+ ? v.toISOString()
+ : v;
+ });
+ return o;
+ });
+
+ const ws = XLSX.utils.json_to_sheet(data, { skipHeader: false });
+ const wb = XLSX.utils.book_new();
+ XLSX.utils.book_append_sheet(wb, ws, 'Current View');
+
+ // Autosize columns (roughly) by header length
+ const colWidths = Object.keys(data[0] || {}).map(h => ({
+ wch: Math.max(10, String(h).length + 2),
+ }));
+ ws['!cols'] = colWidths;
+
+ XLSX.writeFile(wb, `${filename || 'current_view'}.xlsx`);
+ } catch (e) {
+ // If xlsx isn’t available for some reason, fall back to CSV
+ downloadClientCSV(rows, columns, filename || 'current_view');
+ addDangerToast?.(
+ t('Falling back to CSV; Excel export library not available.'),
+ );
+ }
+ };
Review Comment:
[nitpick] The functions `downloadClientCSV`, `downloadClientJSON`, and
`downloadClientXLSX` are defined inside the hook component but are not wrapped
in `useCallback`. While they're not included in the `useMemo` dependencies,
they're used directly in the menu item onClick handlers (lines 583, 617, 667).
Consider wrapping these functions in `useCallback` to prevent unnecessary
re-creation on every render and ensure stable function references.
--
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]
