Copilot commented on code in PR #35208:
URL: https://github.com/apache/superset/pull/35208#discussion_r2535869005
##########
superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx:
##########
@@ -140,6 +140,8 @@ export interface SliceHeaderControlsProps {
supersetCanCSV?: boolean;
crossFiltersEnabled?: boolean;
+
+ ownState?: object;
Review Comment:
The type `object` is too generic for TypeScript. Use `JsonObject` instead
(imported from `@superset-ui/core`) to maintain type consistency with other
parts of the codebase where `ownState` is used (e.g., `Chart.tsx` line 78,
`ExploreChartPanel/index.tsx` line 66). This provides better type safety and
aligns with established patterns.
##########
superset-frontend/src/explore/components/controls/ViewQueryModal.tsx:
##########
@@ -76,7 +78,7 @@ const ViewQueryModal: FC<Props> = ({ latestQueryFormData })
=> {
};
useEffect(() => {
loadChartData('query');
- }, [JSON.stringify(latestQueryFormData)]);
+ }, [JSON.stringify(latestQueryFormData), JSON.stringify(ownState)]);
Review Comment:
Using `JSON.stringify()` in dependency arrays is an anti-pattern that
creates new string references on every render, causing unnecessary
re-executions. Additionally, `loadChartData` is defined inside the component
but not included in the dependency array, which violates the exhaustive-deps
rule.
Consider using `useMemo` to memoize the stringified values, or better yet,
restructure to avoid this pattern:
```typescript
const loadChartData = useCallback((resultType: string) => {
setIsLoading(true);
getChartDataRequest({
formData: latestQueryFormData,
resultFormat: 'json',
resultType,
ownState: ownState || {},
})
// ... rest of the implementation
}, [latestQueryFormData, ownState]);
useEffect(() => {
loadChartData('query');
}, [loadChartData]);
```
This approach properly tracks dependencies without the overhead of JSON
stringification.
##########
superset-frontend/src/dashboard/components/SliceHeader/index.tsx:
##########
@@ -57,6 +57,7 @@ type SliceHeaderProps = SliceHeaderControlsProps & {
filters: object;
handleToggleFullSize: () => void;
formData: object;
+ ownState?: object;
Review Comment:
The type `object` is too generic for TypeScript. Use `JsonObject` instead
(imported from `@superset-ui/core`) to maintain type consistency with other
parts of the codebase where `ownState` is used. This provides better type
safety and aligns with established patterns.
##########
superset-frontend/src/explore/components/controls/ViewQueryModal.tsx:
##########
@@ -31,6 +31,7 @@ import ViewQuery from
'src/explore/components/controls/ViewQuery';
interface Props {
latestQueryFormData: QueryFormData;
+ ownState?: object;
Review Comment:
The type `object` is too generic for TypeScript. Use `JsonObject` instead
(imported from `@superset-ui/core`) to maintain type consistency with other
parts of the codebase where `ownState` is used (e.g., `Chart.tsx` line 78).
This provides better type safety and aligns with established patterns.
```typescript
import {
styled,
ensureIsArray,
t,
getClientErrorObject,
QueryFormData,
JsonObject,
} from '@superset-ui/core';
interface Props {
latestQueryFormData: QueryFormData;
ownState?: JsonObject;
}
```
```suggestion
ownState?: JsonObject;
```
##########
superset-frontend/src/explore/components/useExploreAdditionalActionsMenu/index.jsx:
##########
@@ -497,7 +497,10 @@ export const useExploreAdditionalActionsMenu = (
}
modalTitle={t('View query')}
modalBody={
- <ViewQueryModal latestQueryFormData={latestQueryFormData} />
+ <ViewQueryModal
+ latestQueryFormData={latestQueryFormData}
+ ownState={ownState}
+ />
Review Comment:
The `ownState` dependency is missing from the `useMemo` dependency array.
Since `ownState` is now used in the `ViewQueryModal` component within this
memoized menu, it should be included in the dependency array to ensure the menu
is re-created when `ownState` changes. Add `ownState` to the dependency array
at line 548.
--
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]
