Copilot commented on code in PR #37109:
URL: https://github.com/apache/superset/pull/37109#discussion_r2713824615
##########
superset-frontend/src/components/Chart/DrillDetail/DrillDetailModal.tsx:
##########
@@ -117,6 +168,55 @@ export default function DrillDetailModal({
history.push(exploreUrl);
}, [exploreUrl, history]);
+ const handleDownload = useCallback(
+ (exportType: 'csv' | 'xlsx') => {
+ const drillPayload = getDrillPayload(formData, initialFilters);
+
+ if (!drillPayload) {
+ return;
+ }
+
+ const [datasourceId, datasourceType] = formData.datasource.split('__');
+
Review Comment:
No validation that formData.datasource exists or is in the expected format
before calling split. If formData.datasource is undefined or doesn't contain
'__', this will cause a runtime error. Consider adding validation or a
try-catch block to handle malformed datasource identifiers gracefully.
```suggestion
const datasource = formData?.datasource;
if (typeof datasource !== 'string' || !datasource.includes('__')) {
// Malformed or missing datasource; abort download rather than
throwing.
return;
}
const [datasourceId, datasourceType] = datasource.split('__');
```
##########
superset-frontend/src/components/Chart/DrillDetail/DrillDetailModal.tsx:
##########
@@ -117,6 +168,55 @@ export default function DrillDetailModal({
history.push(exploreUrl);
}, [exploreUrl, history]);
+ const handleDownload = useCallback(
+ (exportType: 'csv' | 'xlsx') => {
+ const drillPayload = getDrillPayload(formData, initialFilters);
+
+ if (!drillPayload) {
+ return;
+ }
+
+ const [datasourceId, datasourceType] = formData.datasource.split('__');
+
+ // Build a QueryContext for drill detail (raw samples, not aggregated)
+ // This matches what DrillDetailPane does when fetching data
+ const payload = {
+ datasource: {
+ id: parseInt(datasourceId, 10),
+ type: datasourceType,
+ },
+ queries: [
+ {
+ ...drillPayload,
+ columns: [],
+ metrics: [],
+ orderby: [],
+ row_limit: 10000,
Review Comment:
The hardcoded row limit of 10000 may not align with user expectations or
system configurations. DrillDetailPane uses `SAMPLES_ROW_LIMIT` from the Redux
state configuration. Consider using the same configuration value to ensure
consistency between what users see in the modal and what gets downloaded, or at
minimum use a configurable value rather than a magic number.
##########
superset-frontend/src/components/Chart/DrillDetail/DrillDetailModal.tsx:
##########
@@ -117,6 +168,55 @@ export default function DrillDetailModal({
history.push(exploreUrl);
}, [exploreUrl, history]);
+ const handleDownload = useCallback(
+ (exportType: 'csv' | 'xlsx') => {
+ const drillPayload = getDrillPayload(formData, initialFilters);
+
+ if (!drillPayload) {
+ return;
+ }
+
+ const [datasourceId, datasourceType] = formData.datasource.split('__');
+
+ // Build a QueryContext for drill detail (raw samples, not aggregated)
+ // This matches what DrillDetailPane does when fetching data
+ const payload = {
+ datasource: {
+ id: parseInt(datasourceId, 10),
+ type: datasourceType,
+ },
+ queries: [
+ {
+ ...drillPayload,
+ columns: [],
+ metrics: [],
+ orderby: [],
+ row_limit: 10000,
+ row_offset: 0,
+ },
+ ],
+ result_type: 'drill_detail',
+ result_format: exportType,
+ force: false,
+ };
+
+ // Use postForm to trigger browser download directly (no progress modal)
+ // This matches the behavior of existing chart exports
+ SupersetClient.postForm(ensureAppRoot('/api/v1/chart/data'), {
+ form_data: safeStringify(payload),
+ });
+ },
+ [formData, initialFilters],
+ );
Review Comment:
There's no error handling for failed downloads. If the API call fails or
returns an error, the user will receive no feedback. Consider adding error
handling similar to what DrillDetailPane does with try-catch blocks and error
toasts to notify users when downloads fail.
##########
superset-frontend/src/components/Chart/DrillDetail/DrillDetailModal.tsx:
##########
@@ -65,6 +76,43 @@ const ModalFooter = ({
{t('Edit chart')}
</Button>
)}
+ {canDownload && (
+ <Dropdown
+ trigger={['click']}
+ menu={{
+ onClick: ({ key }) => {
+ if (key === 'csv') {
+ onDownloadCSV();
+ } else if (key === 'xlsx') {
+ onDownloadXLSX();
+ }
+ },
+ items: [
+ {
+ key: 'csv',
+ label: t('Export to CSV'),
+ icon: <Icons.FileOutlined />,
+ },
+ {
+ key: 'xlsx',
+ label: t('Export to Excel'),
+ icon: <Icons.FileOutlined />,
+ },
+ ],
+ }}
+ >
+ <Button
+ buttonStyle="secondary"
+ buttonSize="small"
+ css={css`
+ margin-left: ${theme.sizeUnit * 2}px;
+ `}
+ data-test="drill-detail-download-btn"
+ >
+ {t('Download')} <Icons.DownOutlined />
+ </Button>
+ </Dropdown>
+ )}
Review Comment:
The download button appears between the "Edit chart" button and the "Close"
button in the modal footer. Consider whether the "Close" button should remain
as the primary action (rightmost position) or if the download functionality
warrants primary placement. Current implementation maintains "Close" as primary
which seems appropriate, but verify this matches the intended UX design.
--
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]
