Copilot commented on code in PR #37625: URL: https://github.com/apache/superset/pull/37625#discussion_r2765989428
########## superset-frontend/src/dashboard/actions/dashboardFilters.ts: ########## @@ -0,0 +1,122 @@ +/** + * 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. + */ +/* eslint-disable camelcase */ +import { JsonObject } from '@superset-ui/core'; +import { Dispatch } from 'redux'; +import { DashboardLayout, RootState } from '../types'; + +type GetState = () => RootState; Review Comment: The `GetState` type is defined locally but is likely used across multiple dashboard action files. Consider extracting this to a shared types file (e.g., `dashboard/types.ts`) to maintain consistency and avoid duplication. ########## superset-frontend/eslint-rules/eslint-plugin-theme-colors/index.ts: ########## @@ -0,0 +1,162 @@ +/** + * 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. + */ + +/** + * @fileoverview Rule to warn about literal colors + * @author Apache + */ + +import type { Rule } from 'eslint'; +import type { Node, SourceLocation } from 'estree'; + +const COLOR_KEYWORDS: string[] = require('./colors'); Review Comment: Mixed usage of ES modules (import type) and CommonJS (require). For consistency with the TypeScript migration, consider using `import COLOR_KEYWORDS from './colors'` instead of `require()`, assuming the colors.ts file exports properly. ```suggestion import COLOR_KEYWORDS from './colors'; ``` ########## superset-frontend/src/dashboard/components/Header/types.ts: ########## @@ -17,71 +17,62 @@ * under the License. */ -import { Layout } from 'src/dashboard/types'; +import { type Dispatch, type SetStateAction } from 'react'; +import { DataMaskStateWithId, JsonObject } from '@superset-ui/core'; +import { + DashboardInfo as DashboardInfoType, + Layout, +} from 'src/dashboard/types'; +import type { ReportObject } from 'src/features/reports/types'; import { ChartState } from 'src/explore/types'; -import { AlertObject } from 'src/features/alerts/types'; - -interface DashboardInfo { - id: number; - userId: string | undefined; - dash_edit_perm: boolean; - dash_save_perm: boolean; - dash_export_perm?: boolean; - metadata?: Record<string, any>; - common?: { conf: Record<string, any> }; - theme?: { - id: number; - name: string; - } | null; -} export interface HeaderDropdownProps { addSuccessToast: (msg: string) => void; - addDangerToast: () => void; - customCss: string; + addDangerToast: (msg: string) => void; Review Comment: The `addDangerToast` function signature changed from `() => void` to `(msg: string) => void`, introducing a required parameter. This is a breaking change that will require all call sites to be updated to provide the message argument. Verify that all usages of this function have been updated accordingly. ########## superset-frontend/src/components/Datasource/utils/index.ts: ########## @@ -16,44 +16,125 @@ * specific language governing permissions and limitations * under the License. */ -import { Children, cloneElement } from 'react'; +import { + Children, + cloneElement, + ReactNode, + ReactElement, + isValidElement, +} from 'react'; import { nanoid } from 'nanoid'; import { SupersetClient } from '@superset-ui/core'; import { tn } from '@apache-superset/core/ui'; import rison from 'rison'; -export function recurseReactClone(children, type, propExtender) { +// Type definitions + +interface ColumnMetadata { + id?: string | number; + column_name: string; + is_dttm?: boolean; + type?: string; + groupby?: boolean; + filterable?: boolean; + expression?: string; +} + +interface ColumnChanges { + added: string[]; + modified: string[]; + removed: string[]; + finalColumns: ColumnMetadata[]; +} + +interface DatasourceForSync { + type?: string; + datasource_type?: string; + database?: { + database_name?: string; + name?: string; + }; + catalog?: string; + schema?: string; + table_name?: string; + normalize_columns?: boolean; + always_filter_main_dttm?: boolean; +} + +interface SyncParams { + datasource_type?: string | null; + database_name?: string | null; + catalog_name?: string | null; + schema_name?: string | null; + table_name?: string | null; + normalize_columns?: boolean | null; + always_filter_main_dttm?: boolean | null; + [key: string]: string | boolean | null | undefined; +} + +// React element type to match against in recurseReactClone +interface ComponentType { + name: string; +} + +export function recurseReactClone<T extends Record<string, unknown>>( + children: ReactNode, + type: ComponentType, + propExtender: (child: ReactElement<T>) => Record<string, unknown>, +): ReactNode { Review Comment: The function signature uses `Record<string, unknown>` for both the generic constraint and the return type of `propExtender`, which loses type safety. Consider using a more specific generic constraint or returning `Partial<T>` from `propExtender` to maintain better type relationships between input and output props. ########## superset-frontend/src/dashboard/components/Dashboard.tsx: ########## @@ -36,72 +37,109 @@ import { areObjectsEqual } from '../../reduxUtils'; import getLocationHash from '../util/getLocationHash'; import isDashboardEmpty from '../util/isDashboardEmpty'; +import type { + AppliedCrossFilterType, + AppliedNativeFilterType, + Filter, +} from '@superset-ui/core'; import { getAffectedOwnDataCharts } from '../util/charts/getOwnDataCharts'; import { getRelatedCharts } from '../util/getRelatedCharts'; +import type { + ActiveFilters, + ChartConfiguration, + DashboardLayout, + DatasourcesState, + LayoutItem, +} from '../types'; -const propTypes = { - actions: PropTypes.shape({ - addSliceToDashboard: PropTypes.func.isRequired, - removeSliceFromDashboard: PropTypes.func.isRequired, - triggerQuery: PropTypes.func.isRequired, - logEvent: PropTypes.func.isRequired, - clearDataMaskState: PropTypes.func.isRequired, - }).isRequired, - dashboardId: PropTypes.number.isRequired, - editMode: PropTypes.bool, - isPublished: PropTypes.bool, - hasUnsavedChanges: PropTypes.bool, - slices: PropTypes.objectOf(slicePropShape).isRequired, - activeFilters: PropTypes.object.isRequired, - chartConfiguration: PropTypes.object, - datasources: PropTypes.object.isRequired, - ownDataCharts: PropTypes.object.isRequired, - layout: PropTypes.object.isRequired, - impressionId: PropTypes.string.isRequired, - timeout: PropTypes.number, - userId: PropTypes.string, - children: PropTypes.node, -}; - -const defaultProps = { - timeout: 60, - userId: '', -}; - -class Dashboard extends PureComponent { +type RelatedChartsFilter = + | AppliedNativeFilterType + | AppliedCrossFilterType + | Filter; + +interface DashboardActions { + addSliceToDashboard: (id: number, component: LayoutItem | undefined) => void; + removeSliceFromDashboard: (id: number) => void; + triggerQuery: (value: boolean, id: number | string) => void; + logEvent: (eventName: string, eventData: Record<string, unknown>) => void; + clearDataMaskState: () => void; + clearAllChartStates: () => void; + setDatasources: (datasources: unknown) => void; +} + +interface DashboardProps { + actions: DashboardActions; + dashboardId: number; + editMode?: boolean; + isPublished?: boolean; + hasUnsavedChanges?: boolean; + slices: Record<string, Slice>; + activeFilters: ActiveFilters; + chartConfiguration?: ChartConfiguration; + datasources: DatasourcesState; + ownDataCharts: JsonObject; + layout: DashboardLayout; + impressionId: string; + timeout?: number; + userId?: string; + children?: ReactNode; +} + +interface VisibilityEventData { + start_offset: number; + ts: number; +} + +class Dashboard extends PureComponent<DashboardProps> { static contextType = PluginContext; - static onBeforeUnload(hasChanged) { + // Use type assertion when accessing context instead of declare field + // to avoid babel transformation issues in Jest + + static defaultProps = { + timeout: 60, + userId: '', + }; + + appliedFilters: ActiveFilters; + + appliedOwnDataCharts: JsonObject; + + visibilityEventData: VisibilityEventData; + + static onBeforeUnload(hasChanged: boolean): void { if (hasChanged) { window.addEventListener('beforeunload', Dashboard.unload); } else { window.removeEventListener('beforeunload', Dashboard.unload); } } - static unload() { + static unload(): string { const message = t('You have unsaved changes.'); - window.event.returnValue = message; // Gecko + IE + // Gecko + IE: returnValue is typed as boolean but historically accepts string + (window.event as BeforeUnloadEvent).returnValue = message; return message; // Gecko + Webkit, Safari, Chrome etc. } - constructor(props) { + constructor(props: DashboardProps) { super(props); this.appliedFilters = props.activeFilters ?? {}; this.appliedOwnDataCharts = props.ownDataCharts ?? {}; + this.visibilityEventData = { start_offset: 0, ts: 0 }; this.onVisibilityChange = this.onVisibilityChange.bind(this); } - componentDidMount() { + componentDidMount(): void { const bootstrapData = getBootstrapData(); const { editMode, isPublished, layout } = this.props; - const eventData = { + const eventData: Record<string, unknown> = { is_soft_navigation: Logger.timeOriginOffset > 0, is_edit_mode: editMode, mount_duration: Logger.getTimestamp(), is_empty: isDashboardEmpty(layout), is_published: isPublished, - bootstrap_data_length: bootstrapData.length, + bootstrap_data_length: JSON.stringify(bootstrapData).length, Review Comment: Changed from `bootstrapData.length` to `JSON.stringify(bootstrapData).length`. If `bootstrapData` was previously an array, this changes the semantic meaning of the metric from array length to serialized JSON string length. Verify this change is intentional and that downstream consumers of this metric expect the new value. ########## superset-frontend/.eslintrc.js: ########## @@ -273,6 +273,20 @@ module.exports = { ], }, overrides: [ + // Ban JavaScript files in src/ - all new code must be TypeScript + { + files: ['src/**/*.js', 'src/**/*.jsx'], + rules: { + 'no-restricted-syntax': [ + 'error', + { + selector: 'Program', + message: + 'JavaScript files are not allowed in src/. Please use TypeScript (.ts/.tsx) instead.', + }, + ], + }, Review Comment: Good addition enforcing TypeScript-only code in src/ via ESLint. This replaces the now-deleted GitHub workflow and provides immediate feedback during development. However, this rule will flag all existing .js/.jsx files in src/. Ensure all files have been migrated before merging, or adjust the rule to exclude legacy files temporarily. ########## superset-frontend/src/components/MessageToasts/reducers.test.ts: ########## @@ -18,26 +18,51 @@ */ import { ADD_TOAST, REMOVE_TOAST } from 'src/components/MessageToasts/actions'; import messageToastsReducer from 'src/components/MessageToasts/reducers'; +import { ToastMeta, ToastType } from 'src/components/MessageToasts/types'; // messageToasts reducer test('messageToasts reducer should return initial state', () => { - expect(messageToastsReducer(undefined, {})).toEqual([]); + expect( + messageToastsReducer(undefined, { type: '' } as unknown as Parameters< + typeof messageToastsReducer + >[1]), + ).toEqual([]); }); test('messageToasts reducer should add a toast', () => { expect( messageToastsReducer([], { type: ADD_TOAST, - payload: { text: 'test', id: 'id', type: 'test_type' }, + payload: { + text: 'test', + id: 'id', + toastType: ToastType.Info, + duration: 4000, + }, Review Comment: The test was updated to include required `toastType` and `duration` fields, but the previous test case (line 24-29) tested the behavior with an incomplete payload `{}`. The new test should verify the reducer handles the complete payload correctly, but consider adding a separate test case for handling incomplete/invalid payloads if that's a valid scenario. ########## superset-frontend/src/dashboard/components/DashboardGrid.tsx: ########## @@ -117,22 +130,28 @@ const GridColumnGuide = styled.div` `}; `; -class DashboardGrid extends PureComponent { - constructor(props) { +class DashboardGrid extends PureComponent< + DashboardGridProps, + DashboardGridState +> { + grid: HTMLDivElement | null; + + constructor(props: DashboardGridProps) { super(props); this.state = { isResizing: false, }; - this.theme = this; + this.grid = null; this.handleResizeStart = this.handleResizeStart.bind(this); + this.handleResize = this.handleResize.bind(this); Review Comment: The `handleResize` method is bound in the constructor but its implementation (lines 175-182) is a no-op that only contains a comment. If this method serves no purpose beyond tracking resize position via `getRowGuidePosition`, consider removing both the method and its binding to reduce code clutter. ```suggestion ``` ########## superset-frontend/plugins/legacy-plugin-chart-horizon/src/transformProps.ts: ########## @@ -0,0 +1,38 @@ +/** + * 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 { ChartProps } from '@superset-ui/core'; + +export default function transformProps(chartProps: ChartProps) { + const { height, width, formData, queriesData } = chartProps; + const { + horizon_color_scale: horizonColorScale, + series_height: seriesHeight, + } = formData; + + // Only include colorScale if defined, otherwise let defaultProps apply + return { + ...(horizonColorScale !== undefined && { + colorScale: horizonColorScale as string, + }), + data: queriesData[0].data, + height, + seriesHeight: parseInt(String(seriesHeight ?? 20), 10), + width, + }; Review Comment: The conditional spread operator pattern on line 30-32 correctly handles the undefined case, but the type cast `as string` suggests the type of `horizonColorScale` may not be strictly typed in `formData`. Consider adding proper typing to the formData interface to avoid the need for type assertions. -- 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]
