Copilot commented on code in PR #36237:
URL: https://github.com/apache/superset/pull/36237#discussion_r2557303398
##########
superset-embedded-sdk/src/index.ts:
##########
@@ -93,6 +93,7 @@ export type EmbeddedDashboard = {
) => void;
getDataMask: () => Promise<Record<string, any>>;
getChartStates: () => Promise<Record<string, any>>;
+ getChartDataPayloads: (chartId?: number) => Promise<Record<string, any>>;
Review Comment:
The type definition should match the intended API design. If the
implementation expects `{ chartId?: number }` as a parameter object, the type
should be:
```typescript
getChartDataPayloads: (params?: { chartId?: number }) =>
Promise<Record<string, any>>;
```
This would be more consistent with `getDashboardPermalink` which accepts a
structured parameter `(anchor: string)` where `anchor` is actually destructured
from an object parameter in the implementation.
```suggestion
getChartDataPayloads: (params?: { chartId?: number }) =>
Promise<Record<string, any>>;
```
##########
superset-embedded-sdk/src/index.ts:
##########
@@ -249,6 +250,8 @@ export async function embedDashboard({
const getActiveTabs = () => ourPort.get<string[]>('getActiveTabs');
const getDataMask = () => ourPort.get<Record<string, any>>('getDataMask');
const getChartStates = () => ourPort.get<Record<string,
any>>('getChartStates');
+ const getChartDataPayloads = (chartId?: number) =>
+ ourPort.get<Record<string, any>>('getChartDataPayloads', { chartId });
Review Comment:
The SDK function signature is inconsistent with the implementation. The SDK
signature accepts `chartId?: number` as a simple parameter, but the
implementation in `api.tsx` expects an object parameter `{ chartId?: number }`.
This mismatch will cause the function to not work correctly.
The SDK should either:
1. Accept an object: `getChartDataPayloads: (params?: { chartId?: number })
=> Promise<Record<string, any>>`
2. Or change line 254 to: `ourPort.get<Record<string,
any>>('getChartDataPayloads', chartId ? { chartId } : undefined)`
Option 1 is more consistent with other methods like `getDashboardPermalink`
which takes a structured parameter.
```suggestion
const getChartDataPayloads = (params?: { chartId?: number }) =>
ourPort.get<Record<string, any>>('getChartDataPayloads', params);
```
##########
superset-frontend/src/embedded/api.tsx:
##########
@@ -80,10 +92,103 @@ const getDataMask = () => store?.getState()?.dataMask ||
{};
const getChartStates = () =>
store?.getState()?.dashboardState?.chartStates || {};
+/**
+ * Get query context payloads for stateful charts (e.g., AG Grid tables).
+ * These payloads include dashboard filters and chart state (sorting, column
order, etc.)
+ * and can be POSTed directly to /api/v1/chart/data for CSV export.
+ *
+ * @param chartId - Optional chart ID to get payload for a specific chart only
+ * @returns Record of chart IDs to their query context payloads
+ */
+const getChartDataPayloads = async ({
+ chartId,
+}: {
+ chartId?: number;
+}): Promise<Record<string, JsonObject>> => {
+ const state = store?.getState();
+ if (!state) return {};
+
+ const charts = state.charts || {};
+ const sliceEntities = state.sliceEntities?.slices || {};
+ const dataMask = state.dataMask || {};
+ const chartStates = state.dashboardState?.chartStates || {};
+ const chartConfiguration =
+ state.dashboardInfo?.metadata?.chart_configuration || {};
+ const nativeFilters = state.nativeFilters?.filters || {};
+ const allSliceIds = state.dashboardState?.sliceIds || [];
+ const colorScheme = state.dashboardState?.colorScheme;
+ const colorNamespace = state.dashboardState?.colorNamespace;
+
+ const payloads: Record<string, JsonObject> = {};
+
+ // Build payloads for each eligible chart
+ const chartEntries = Object.entries(charts).filter(([id]) => {
+ const numericId = Number(id);
+ const slice = sliceEntities[id];
+
+ // Filter: only stateful charts (e.g., AG Grid)
+ if (!slice || !hasChartStateConverter(slice.viz_type)) {
+ return false;
+ }
+
+ // Filter: specific chartId if provided
+ if (chartId !== undefined && numericId !== chartId) {
+ return false;
+ }
+
+ return true;
+ });
+
+ // Process charts sequentially to avoid race conditions
+ for (const [id, chart] of chartEntries) {
+ const numericId = Number(id);
+ const slice = sliceEntities[id];
+
Review Comment:
[nitpick] The type assertion `(chart as JsonObject).form_data` is
potentially unsafe. If `chart` doesn't have a `form_data` property or if
`form_data` is not the expected type, this could lead to runtime errors.
Consider adding a type guard or at least a check to ensure `chart.form_data`
exists before accessing it:
```typescript
const formData = chart && typeof chart === 'object' && 'form_data' in chart
? getFormDataWithExtraFilters({
chart: { id: numericId, form_data: chart.form_data },
// ... rest of params
})
: null;
if (!formData) {
continue; // or handle the error appropriately
}
```
```suggestion
// Type guard: ensure chart has form_data property
if (
!chart ||
typeof chart !== 'object' ||
!('form_data' in chart)
) {
continue;
}
```
##########
superset-frontend/src/embedded/api.tsx:
##########
@@ -80,10 +92,103 @@ const getDataMask = () => store?.getState()?.dataMask ||
{};
const getChartStates = () =>
store?.getState()?.dashboardState?.chartStates || {};
+/**
+ * Get query context payloads for stateful charts (e.g., AG Grid tables).
+ * These payloads include dashboard filters and chart state (sorting, column
order, etc.)
+ * and can be POSTed directly to /api/v1/chart/data for CSV export.
+ *
+ * @param chartId - Optional chart ID to get payload for a specific chart only
+ * @returns Record of chart IDs to their query context payloads
+ */
+const getChartDataPayloads = async ({
+ chartId,
+}: {
+ chartId?: number;
+}): Promise<Record<string, JsonObject>> => {
+ const state = store?.getState();
+ if (!state) return {};
+
+ const charts = state.charts || {};
+ const sliceEntities = state.sliceEntities?.slices || {};
+ const dataMask = state.dataMask || {};
+ const chartStates = state.dashboardState?.chartStates || {};
+ const chartConfiguration =
+ state.dashboardInfo?.metadata?.chart_configuration || {};
+ const nativeFilters = state.nativeFilters?.filters || {};
+ const allSliceIds = state.dashboardState?.sliceIds || [];
+ const colorScheme = state.dashboardState?.colorScheme;
+ const colorNamespace = state.dashboardState?.colorNamespace;
+
+ const payloads: Record<string, JsonObject> = {};
+
+ // Build payloads for each eligible chart
+ const chartEntries = Object.entries(charts).filter(([id]) => {
+ const numericId = Number(id);
+ const slice = sliceEntities[id];
+
+ // Filter: only stateful charts (e.g., AG Grid)
+ if (!slice || !hasChartStateConverter(slice.viz_type)) {
+ return false;
+ }
+
+ // Filter: specific chartId if provided
+ if (chartId !== undefined && numericId !== chartId) {
+ return false;
+ }
+
+ return true;
+ });
+
+ // Process charts sequentially to avoid race conditions
+ for (const [id, chart] of chartEntries) {
+ const numericId = Number(id);
+ const slice = sliceEntities[id];
+
+ // Build enriched form_data with dashboard filters applied
+ const formData = getFormDataWithExtraFilters({
+ chart: { id: numericId, form_data: (chart as JsonObject).form_data },
+ chartConfiguration,
+ filters: getAppliedFilterValues(numericId),
+ colorScheme,
+ colorNamespace,
+ sliceId: numericId,
+ nativeFilters,
+ allSliceIds,
+ dataMask,
+ extraControls: {},
+ });
+
+ const chartState = chartStates[id]?.state;
+ const baseOwnState = dataMask[id]?.ownState || {};
+ const convertedState = chartState
+ ? convertChartStateToOwnState(slice.viz_type, chartState)
+ : {};
+
+ const ownState = {
+ ...baseOwnState,
+ ...convertedState,
+ };
+
+ const payload = await buildV1ChartDataPayload({
+ formData,
+ resultFormat: 'json',
+ resultType: 'results',
+ ownState,
+ setDataMask: null,
+ force: false,
+ });
+
+ payloads[id] = payload;
+ }
Review Comment:
[nitpick] Processing charts sequentially in a for loop could be slow when
there are many stateful charts. The comment mentions "avoid race conditions"
but it's unclear what race condition is being avoided here since each iteration
is independent. Consider using `Promise.all()` to process charts in parallel
for better performance:
```typescript
const payloadPromises = chartEntries.map(async ([id, chart]) => {
// ... existing logic ...
const payload = await buildV1ChartDataPayload({...});
return [id, payload];
});
const results = await Promise.all(payloadPromises);
results.forEach(([id, payload]) => {
payloads[id] = payload;
});
```
This would significantly improve performance when multiple charts need their
payloads generated.
##########
superset-frontend/src/embedded/api.tsx:
##########
@@ -80,10 +92,103 @@ const getDataMask = () => store?.getState()?.dataMask ||
{};
const getChartStates = () =>
store?.getState()?.dashboardState?.chartStates || {};
+/**
+ * Get query context payloads for stateful charts (e.g., AG Grid tables).
+ * These payloads include dashboard filters and chart state (sorting, column
order, etc.)
+ * and can be POSTed directly to /api/v1/chart/data for CSV export.
+ *
+ * @param chartId - Optional chart ID to get payload for a specific chart only
+ * @returns Record of chart IDs to their query context payloads
+ */
+const getChartDataPayloads = async ({
+ chartId,
+}: {
+ chartId?: number;
+}): Promise<Record<string, JsonObject>> => {
+ const state = store?.getState();
+ if (!state) return {};
+
+ const charts = state.charts || {};
+ const sliceEntities = state.sliceEntities?.slices || {};
+ const dataMask = state.dataMask || {};
+ const chartStates = state.dashboardState?.chartStates || {};
+ const chartConfiguration =
+ state.dashboardInfo?.metadata?.chart_configuration || {};
+ const nativeFilters = state.nativeFilters?.filters || {};
+ const allSliceIds = state.dashboardState?.sliceIds || [];
+ const colorScheme = state.dashboardState?.colorScheme;
+ const colorNamespace = state.dashboardState?.colorNamespace;
+
+ const payloads: Record<string, JsonObject> = {};
+
+ // Build payloads for each eligible chart
+ const chartEntries = Object.entries(charts).filter(([id]) => {
+ const numericId = Number(id);
+ const slice = sliceEntities[id];
+
+ // Filter: only stateful charts (e.g., AG Grid)
+ if (!slice || !hasChartStateConverter(slice.viz_type)) {
+ return false;
+ }
+
+ // Filter: specific chartId if provided
+ if (chartId !== undefined && numericId !== chartId) {
+ return false;
+ }
+
+ return true;
+ });
+
+ // Process charts sequentially to avoid race conditions
+ for (const [id, chart] of chartEntries) {
+ const numericId = Number(id);
+ const slice = sliceEntities[id];
+
+ // Build enriched form_data with dashboard filters applied
+ const formData = getFormDataWithExtraFilters({
+ chart: { id: numericId, form_data: (chart as JsonObject).form_data },
+ chartConfiguration,
+ filters: getAppliedFilterValues(numericId),
+ colorScheme,
+ colorNamespace,
+ sliceId: numericId,
+ nativeFilters,
+ allSliceIds,
+ dataMask,
+ extraControls: {},
+ });
+
+ const chartState = chartStates[id]?.state;
+ const baseOwnState = dataMask[id]?.ownState || {};
+ const convertedState = chartState
+ ? convertChartStateToOwnState(slice.viz_type, chartState)
+ : {};
+
+ const ownState = {
+ ...baseOwnState,
+ ...convertedState,
+ };
+
+ const payload = await buildV1ChartDataPayload({
+ formData,
+ resultFormat: 'json',
+ resultType: 'results',
+ ownState,
+ setDataMask: null,
+ force: false,
+ });
+
+ payloads[id] = payload;
Review Comment:
The function silently catches and swallows any errors that might occur
during `buildV1ChartDataPayload` execution. If payload building fails for one
chart, the function will continue processing other charts without any
indication that an error occurred. Consider adding try-catch blocks around the
payload building logic to handle errors gracefully and either:
1. Log errors (if debug mode is available)
2. Include error information in the returned object
3. At minimum, skip the failed chart and continue with others while tracking
which charts failed
This would make debugging issues much easier for SDK users.
```suggestion
try {
const payload = await buildV1ChartDataPayload({
formData,
resultFormat: 'json',
resultType: 'results',
ownState,
setDataMask: null,
force: false,
});
payloads[id] = payload;
} catch (error) {
// Optionally log the error for debugging
// You could check for a debug flag here if available
console.error(`Error building payload for chart ${id}:`, error);
payloads[id] = {
error: true,
message: error instanceof Error ? error.message : String(error),
};
}
```
##########
superset-frontend/src/embedded/api.tsx:
##########
@@ -80,10 +92,103 @@ const getDataMask = () => store?.getState()?.dataMask ||
{};
const getChartStates = () =>
store?.getState()?.dashboardState?.chartStates || {};
+/**
+ * Get query context payloads for stateful charts (e.g., AG Grid tables).
+ * These payloads include dashboard filters and chart state (sorting, column
order, etc.)
+ * and can be POSTed directly to /api/v1/chart/data for CSV export.
+ *
+ * @param chartId - Optional chart ID to get payload for a specific chart only
+ * @returns Record of chart IDs to their query context payloads
+ */
+const getChartDataPayloads = async ({
+ chartId,
+}: {
+ chartId?: number;
+}): Promise<Record<string, JsonObject>> => {
+ const state = store?.getState();
+ if (!state) return {};
+
+ const charts = state.charts || {};
+ const sliceEntities = state.sliceEntities?.slices || {};
+ const dataMask = state.dataMask || {};
+ const chartStates = state.dashboardState?.chartStates || {};
+ const chartConfiguration =
+ state.dashboardInfo?.metadata?.chart_configuration || {};
+ const nativeFilters = state.nativeFilters?.filters || {};
+ const allSliceIds = state.dashboardState?.sliceIds || [];
+ const colorScheme = state.dashboardState?.colorScheme;
+ const colorNamespace = state.dashboardState?.colorNamespace;
+
+ const payloads: Record<string, JsonObject> = {};
+
+ // Build payloads for each eligible chart
+ const chartEntries = Object.entries(charts).filter(([id]) => {
+ const numericId = Number(id);
+ const slice = sliceEntities[id];
+
+ // Filter: only stateful charts (e.g., AG Grid)
+ if (!slice || !hasChartStateConverter(slice.viz_type)) {
+ return false;
+ }
+
+ // Filter: specific chartId if provided
+ if (chartId !== undefined && numericId !== chartId) {
+ return false;
+ }
+
+ return true;
+ });
+
+ // Process charts sequentially to avoid race conditions
+ for (const [id, chart] of chartEntries) {
+ const numericId = Number(id);
+ const slice = sliceEntities[id];
+
+ // Build enriched form_data with dashboard filters applied
+ const formData = getFormDataWithExtraFilters({
+ chart: { id: numericId, form_data: (chart as JsonObject).form_data },
+ chartConfiguration,
+ filters: getAppliedFilterValues(numericId),
+ colorScheme,
+ colorNamespace,
+ sliceId: numericId,
+ nativeFilters,
+ allSliceIds,
+ dataMask,
+ extraControls: {},
+ });
+
+ const chartState = chartStates[id]?.state;
+ const baseOwnState = dataMask[id]?.ownState || {};
+ const convertedState = chartState
+ ? convertChartStateToOwnState(slice.viz_type, chartState)
+ : {};
+
+ const ownState = {
+ ...baseOwnState,
+ ...convertedState,
+ };
+
+ const payload = await buildV1ChartDataPayload({
+ formData,
+ resultFormat: 'json',
+ resultType: 'results',
+ ownState,
+ setDataMask: null,
+ force: false,
+ });
+
+ payloads[id] = payload;
+ }
+
Review Comment:
The function should handle the case when a specific `chartId` is provided
but it doesn't exist in the state. Currently, if a user provides a `chartId`
that doesn't exist, the function will return an empty object without any
indication that the requested chart was not found. Consider adding a check
after the loop to verify that at least one payload was generated when a
specific `chartId` was requested, or return a more informative result that
indicates whether the requested chart was found.
```suggestion
// If a specific chartId was requested but no payload was generated,
return an error object
if (chartId !== undefined && Object.keys(payloads).length === 0) {
return { error: 'Chart not found', chartId };
}
```
##########
superset-frontend/src/embedded/api.tsx:
##########
@@ -80,10 +92,103 @@ const getDataMask = () => store?.getState()?.dataMask ||
{};
const getChartStates = () =>
store?.getState()?.dashboardState?.chartStates || {};
+/**
+ * Get query context payloads for stateful charts (e.g., AG Grid tables).
+ * These payloads include dashboard filters and chart state (sorting, column
order, etc.)
+ * and can be POSTed directly to /api/v1/chart/data for CSV export.
+ *
+ * @param chartId - Optional chart ID to get payload for a specific chart only
+ * @returns Record of chart IDs to their query context payloads
+ */
Review Comment:
The documentation says "These payloads include dashboard filters and chart
state (sorting, column order, etc.)" but it only processes stateful charts
(charts with registered state converters). This creates an inconsistency - the
documentation implies all charts are included, but the implementation filters
to only stateful charts. Consider clarifying this in the documentation, e.g.:
"Get query context payloads for stateful charts only (e.g., AG Grid tables).
Returns payloads only for charts that have registered state converters.
Non-stateful charts will not be included in the result."
--
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]
