Copilot commented on code in PR #37412: URL: https://github.com/apache/superset/pull/37412#discussion_r2728780488
########## superset-frontend/packages/superset-ui-core/src/utils/addLabelMapToVerboseMap.test.ts: ########## @@ -0,0 +1,61 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import addLabelMapToVerboseMap from './addLabelMapToVerboseMap'; + +describe('echarts forecast', () => { Review Comment: The test suite description `"echarts forecast"` is misleading for this file, which specifically exercises `addLabelMapToVerboseMap`. Renaming the `describe` block to reference `addLabelMapToVerboseMap` (or this utility module) will make failures easier to interpret and keep test naming consistent with the code under test. ```suggestion describe('addLabelMapToVerboseMap', () => { ``` ########## superset-frontend/src/explore/components/DataTablesPane/components/useResultsPane.tsx: ########## @@ -85,11 +88,13 @@ export const useResultsPane = ({ formData: queryFormData, force: queryForce, resultFormat: 'json', - resultType: 'results', + resultType: 'full', ownState, }) .then(({ json }) => { - setResultResp(ensureIsArray(json.result)); + const responseArray = ensureIsArray(json.result); + const transformedResponseArray = transformTableData(responseArray); + setResultResp(transformedResponseArray); setResponseError(''); cache.set(queryFormData, json.result); Review Comment: On cache hits you call `setResultResp(ensureIsArray(cache.get(queryFormData)))`, but you never reapply `transformTableData` to that cached payload, while on cache misses you transform the data before putting it into state and then store the **untransformed** `json.result` in the cache. This means the first load shows correctly labeled columns/rows, but subsequent loads that use the cache will revert to the raw server column names. To keep behavior consistent, either cache the already-transformed response (and read it back as-is) or always run `transformTableData` on the cached value before calling `setResultResp`. ```suggestion cache.set(queryFormData, transformedResponseArray); ``` ########## superset-frontend/src/explore/components/DataTablesPane/components/useResultsPane.tsx: ########## @@ -45,17 +46,19 @@ const StyledDiv = styled.div` const cache = new WeakMap(); -export const useResultsPane = ({ - isRequest, - queryFormData, - queryForce, - ownState, - errorMessage, - setForceQuery, - isVisible, - dataSize = 50, - canDownload, -}: ResultsPaneProps): ReactElement[] => { +export const useResultsPane = (args: ResultsPaneProps): ReactElement[] => { + const { + isRequest, + queryFormData, + queryForce, + ownState, + errorMessage, + setForceQuery, + isVisible, + dataSize = 50, + canDownload, + } = args; + console.log('useResultsPane args', { args }); Review Comment: This `console.log` introduces noisy debug output in production whenever `useResultsPane` runs, and it doesn’t appear to be guarded by any dev flag or feature flag. Please remove this log (or gate it behind a debug flag) to avoid unnecessary console noise and potential performance overhead in hot paths. ```suggestion ``` -- 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]
