Copilot commented on code in PR #38165:
URL: https://github.com/apache/superset/pull/38165#discussion_r3471113524
##########
superset-frontend/playwright/pages/ExplorePage.ts:
##########
@@ -30,6 +32,11 @@ export class ExplorePage {
DATASOURCE_CONTROL: '[data-test="datasource-control"]',
VIZ_SWITCHER: '[data-test="fast-viz-switcher"]',
CHART_CONTAINER: '[data-test="chart-container"]',
+ // The bottom data panel (DataTablesPane / SouthPane) with Results/Samples
tabs
+ SOUTH_PANE: '[data-test="some-purposeful-instance"]',
+ EXPAND_DATA_PANEL: '[aria-label="Expand data panel"]',
+ RESULTS_TAB: '[data-node-key="results"]',
+ ACTIVE_TABPANE: '.ant-tabs-tabpane-active',
Review Comment:
`SOUTH_PANE` is keyed off `data-test="some-purposeful-instance"`, which
reads like a placeholder and makes the E2E selectors harder to
understand/maintain. Prefer a descriptive `data-test` value (and update the
component that renders it) or avoid relying on a custom test id if a stable
accessible selector exists.
##########
superset-frontend/src/explore/components/DataTablesPane/components/useResultsPane.tsx:
##########
@@ -89,7 +91,42 @@ export const useResultsPane = ({
useEffect(() => {
// it's an invalid formData when gets a errorMessage
if (errorMessage) return;
- if (isRequest && cache.has(cappedFormData)) {
+ if (!isRequest) return;
+
+ // Reuse chart data from Redux when available. The chart visualization
+ // query and the results query produce identical SQL on the backend,
+ // so there is no need for a separate network request.
+ //
+ // The Results tab can request fewer rows than the chart via the row-limit
+ // dropdown. The chart query always ran with the chart's own row_limit
+ // (>= effectiveRowLimit), so the first `effectiveRowLimit` rows are
exactly
+ // what a dedicated results query (LIMIT effectiveRowLimit) would return.
+ // Slice locally so the dropdown stays functional without a duplicate
query.
+ if (queriesResponse?.length && 'colnames' in queriesResponse[0]) {
+ const mapped = queriesResponse.map(q => {
+ const result = q as ChartDataResponseResult;
+ const limitedData = ensureIsArray(result.data).slice(
+ 0,
+ effectiveRowLimit,
+ );
+ return {
+ colnames: result.colnames,
+ coltypes: result.coltypes,
+ data: limitedData,
+ rowcount: limitedData.length,
+ };
+ }) as unknown as QueryResultInterface[];
Review Comment:
The reuse-path guard `queriesResponse?.length && 'colnames' in
queriesResponse[0]` is too loose for `QueryData` (it can be an arbitrary legacy
object). If a legacy payload happens to include a `colnames` key but not the
full v1 shape (`coltypes`, `data` arrays), this will incorrectly take the reuse
path and can break the Results pane. Consider a stricter runtime type guard
(verify `colnames`, `coltypes`, and `data` are arrays for every query) and fall
back to the API request otherwise.
--
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]
