korbit-ai[bot] commented on code in PR #35506: URL: https://github.com/apache/superset/pull/35506#discussion_r2423477085
########## superset-frontend/scripts/oxlint-metrics-uploader.js: ########## @@ -0,0 +1,245 @@ +/** + * 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. + */ +const { execSync } = require('child_process'); +const { google } = require('googleapis'); + +const { SPREADSHEET_ID } = process.env; +const SERVICE_ACCOUNT_KEY = JSON.parse(process.env.SERVICE_ACCOUNT_KEY || '{}'); Review Comment: ### Potential exposure of service account credentials <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The code is parsing a service account key from an environment variable, which could potentially expose sensitive credentials if not properly secured. ###### Why this matters If the environment variable is not set or is compromised, it could lead to unauthorized access to Google Sheets API. ###### Suggested change ∙ *Feature Preview* Ensure that the SERVICE_ACCOUNT_KEY environment variable is securely set and not exposed in any logs or error messages. Consider using a secure secret management system for storing and accessing sensitive credentials. ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b6f0ee49-2421-41f9-8761-801068c3943c/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b6f0ee49-2421-41f9-8761-801068c3943c?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b6f0ee49-2421-41f9-8761-801068c3943c?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b6f0ee49-2421-41f9-8761-801068c3943c?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b6f0ee49-2421-41f9-8761-801068c3943c) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:48090b87-9509-4967-8049-42f6acee8fe6 --> [](48090b87-9509-4967-8049-42f6acee8fe6) ########## superset-frontend/src/explore/components/controls/ComparisonRangeLabel.tsx: ########## @@ -144,7 +144,8 @@ export const ComparisonRangeLabel = ({ ); const startDateDayjs = extendedDayjs(parseDttmToDate(startDate)); if ( - startDateDayjs.isSameOrBefore(parsedDateDayjs) || + startDateDayjs.isBefore(parsedDateDayjs) || + startDateDayjs.isSame(parsedDateDayjs) || Review Comment: ### Redundant date comparison methods <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? Two separate method calls are used to check if a date is before or same as another date, when a single method call could achieve the same result. ###### Why this matters This creates unnecessary method invocations and reduces code readability. The dayjs library provides `isSameOrBefore()` method that combines both checks in a single operation. ###### Suggested change ∙ *Feature Preview* Replace the two separate calls with the single `isSameOrBefore()` method: ```typescript startDateDayjs.isSameOrBefore(parsedDateDayjs) || ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e929255e-42ec-418e-8d5c-270107e4d286/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e929255e-42ec-418e-8d5c-270107e4d286?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e929255e-42ec-418e-8d5c-270107e4d286?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e929255e-42ec-418e-8d5c-270107e4d286?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e929255e-42ec-418e-8d5c-270107e4d286) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:8beea09a-42c7-45f6-a06f-e0326ebdeecf --> [](8beea09a-42c7-45f6-a06f-e0326ebdeecf) ########## superset-frontend/src/dashboard/components/Header/types.ts: ########## @@ -77,7 +77,7 @@ export interface HeaderProps { charts: ChartState | {}; colorScheme?: string; customCss: string; - user: Object | undefined; + user: object | undefined; Review Comment: ### Weak user prop typing <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? Prop 'user' is typed as 'object | undefined' which is too generic for a user object. ###### Why this matters Using a generic 'object' type reduces type safety when consuming user properties and can lead to runtime errors or extra casting/guards when accessing user fields. ###### Suggested change ∙ *Feature Preview* Introduce a concrete User interface and use it for the prop: ``` // new or existing: define a User type export interface User { id?: string; // add other fields used by Header component } ``` and update the prop type: ``` - user: object | undefined; + user: User | undefined; ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2d9d18fc-2945-44be-bcd5-c8995855df0f/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2d9d18fc-2945-44be-bcd5-c8995855df0f?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2d9d18fc-2945-44be-bcd5-c8995855df0f?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2d9d18fc-2945-44be-bcd5-c8995855df0f?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2d9d18fc-2945-44be-bcd5-c8995855df0f) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:a803cbd3-a78b-4740-878e-38e6d8d9693f --> [](a803cbd3-a78b-4740-878e-38e6d8d9693f) ########## superset-frontend/packages/superset-ui-core/src/components/Modal/types.ts: ########## @@ -67,8 +67,8 @@ export interface StyledModalProps { export type { ModalFuncProps }; export interface FormModalProps extends ModalProps { - initialValues?: Object; - formSubmitHandler: (values: Object) => Promise<void>; + initialValues?: object; + formSubmitHandler: (values: object) => Promise<void>; Review Comment: ### Overly generic object types lack type safety <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? Using the generic 'object' type instead of a more specific type provides no type safety and defeats the purpose of TypeScript's type system. ###### Why this matters This allows any object structure to be passed, making it impossible to catch type-related errors at compile time and providing no IntelliSense support for developers using these interfaces. ###### Suggested change ∙ *Feature Preview* Replace with generic types or more specific interfaces: ```typescript export interface FormModalProps<T = Record<string, any>> extends ModalProps { initialValues?: T; formSubmitHandler: (values: T) => Promise<void>; onSave: () => void; requiredFields: string[]; } ``` Or use a more specific type like `Record<string, any>` if the structure is truly unknown. ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fa31917c-01b3-4408-a5c5-6d4f72b1bea3/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fa31917c-01b3-4408-a5c5-6d4f72b1bea3?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fa31917c-01b3-4408-a5c5-6d4f72b1bea3?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fa31917c-01b3-4408-a5c5-6d4f72b1bea3?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fa31917c-01b3-4408-a5c5-6d4f72b1bea3) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:da567225-f277-461b-b147-8c3a57c3fa1c --> [](da567225-f277-461b-b147-8c3a57c3fa1c) ########## superset-frontend/plugins/plugin-chart-ag-grid-table/src/renderers/NumericCellRenderer.tsx: ########## @@ -109,13 +109,15 @@ function cellBackground({ value, colorPositiveNegative = false, isDarkTheme = false, + theme, }: { value: number; colorPositiveNegative: boolean; isDarkTheme: boolean; + theme: any; Review Comment: ### Weak typing with 'any' for theme parameter <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The theme parameter is typed as 'any' which removes type safety and prevents proper IntelliSense support. ###### Why this matters Using 'any' type defeats the purpose of TypeScript's type system, making the code prone to runtime errors and reducing developer productivity through lack of autocomplete and compile-time checks. ###### Suggested change ∙ *Feature Preview* Import the proper theme type from @superset-ui/core and use it instead of 'any': ```typescript import { styled, useTheme, SupersetTheme } from '@superset-ui/core'; // Then update the parameter type: theme: SupersetTheme; ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ac26cf72-d2e4-4732-8bd6-d0b19096274b/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ac26cf72-d2e4-4732-8bd6-d0b19096274b?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ac26cf72-d2e4-4732-8bd6-d0b19096274b?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ac26cf72-d2e4-4732-8bd6-d0b19096274b?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ac26cf72-d2e4-4732-8bd6-d0b19096274b) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:420d21b3-517b-4410-a29d-706f36485fde --> [](420d21b3-517b-4410-a29d-706f36485fde) ########## superset-frontend/src/utils/downloadAsImage.tsx: ########## @@ -97,7 +102,7 @@ const copyAllComputedStyles = (original: Element, clone: Element) => { styleCache.set(origNode, computed); } - for (const property of CRITICAL_STYLE_PROPERTIES) { + for (const property of Array.from(CRITICAL_STYLE_PROPERTIES)) { Review Comment: ### Unnecessary Set to Array conversion in hot path <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? Converting a Set to Array on every iteration creates unnecessary overhead when iterating over CRITICAL_STYLE_PROPERTIES. ###### Why this matters This conversion happens for every element being processed in the DOM tree, creating multiple temporary arrays and adding O(n) time complexity for each iteration where n is the size of the Set. ###### Suggested change ∙ *Feature Preview* Iterate directly over the Set using `for (const property of CRITICAL_STYLE_PROPERTIES)` since Sets are iterable in JavaScript. ```typescript for (const property of CRITICAL_STYLE_PROPERTIES) { ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4f14d7d8-2f62-4f22-8ede-27cb18d1ff0d/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4f14d7d8-2f62-4f22-8ede-27cb18d1ff0d?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4f14d7d8-2f62-4f22-8ede-27cb18d1ff0d?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4f14d7d8-2f62-4f22-8ede-27cb18d1ff0d?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4f14d7d8-2f62-4f22-8ede-27cb18d1ff0d) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:d6b1cd4a-f6f7-4721-a72b-0c7170b71b53 --> [](d6b1cd4a-f6f7-4721-a72b-0c7170b71b53) ########## superset-frontend/packages/superset-ui-demo/storybook/stories/superset-ui-theme/Theme.stories.tsx: ########## @@ -89,7 +91,7 @@ const AntDFunctionalColors = () => { }; export const ThemeColors = () => { - const { colors } = supersetTheme; + const { colors } = supersetTheme as any; Review Comment: ### Missing Theme Type Definition <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? Another instance of using 'as any' type assertion, bypassing TypeScript's type system. ###### Why this matters This creates the same issues as the first problem, making the code less maintainable and more error-prone. ###### Suggested change ∙ *Feature Preview* Define a proper interface for the supersetTheme: ```typescript interface SupersetTheme { colors: Record<string, Record<string, string>>; } const { colors } = supersetTheme as SupersetTheme; ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d4493615-32e0-43a1-8fa3-c41587628e8e/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d4493615-32e0-43a1-8fa3-c41587628e8e?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d4493615-32e0-43a1-8fa3-c41587628e8e?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d4493615-32e0-43a1-8fa3-c41587628e8e?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d4493615-32e0-43a1-8fa3-c41587628e8e) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:454361cc-2770-4c0f-be00-b0b5a746219e --> [](454361cc-2770-4c0f-be00-b0b5a746219e) ########## scripts/check-custom-rules.sh: ########## @@ -0,0 +1,48 @@ +#!/usr/bin/env bash + +# 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. + +set -e + +script_dir="$(dirname "$(realpath "$0")")" +root_dir="$(dirname "$script_dir")" +frontend_dir=superset-frontend + +if [[ ! -d "$root_dir/$frontend_dir" ]]; then + echo "Error: $frontend_dir directory not found in $root_dir" >&2 + exit 1 +fi + +cd "$root_dir/$frontend_dir" + +# Filter files to only include JS/TS files and remove the frontend dir prefix +js_ts_files=() +for file in "$@"; do + # Remove superset-frontend/ prefix if present + cleaned_file="${file#$frontend_dir/}" + + # Only include JS/TS files + if [[ "$cleaned_file" =~ \.(js|jsx|ts|tsx)$ ]]; then + js_ts_files+=("$cleaned_file") + fi +done Review Comment: ### Inefficient sequential file processing <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The script processes all input files sequentially in a loop, performing string operations and regex matching for each file individually. ###### Why this matters This approach has O(n) time complexity for each file operation and creates performance bottlenecks when processing large numbers of files, as each file requires separate string manipulation and pattern matching operations. ###### Suggested change ∙ *Feature Preview* Use shell parameter expansion and globbing patterns to filter files more efficiently, or batch the filtering operations. Consider using `find` with `-name` patterns or shell arrays with pattern matching to reduce the number of individual operations: ```bash # More efficient approach using shell globbing shopt -s nullglob js_ts_files=() for pattern in "*.js" "*.jsx" "*.ts" "*.tsx"; do for file in "$@"; do [[ "$file" == *"/$pattern" || "$file" == "$pattern" ]] && js_ts_files+=("${file#$frontend_dir/}") done done ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a89dc7ce-2e54-4e67-ba25-18252f196a15/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a89dc7ce-2e54-4e67-ba25-18252f196a15?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a89dc7ce-2e54-4e67-ba25-18252f196a15?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a89dc7ce-2e54-4e67-ba25-18252f196a15?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a89dc7ce-2e54-4e67-ba25-18252f196a15) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:deb0fbac-a542-4c81-a521-3baf7cccf513 --> [](deb0fbac-a542-4c81-a521-3baf7cccf513) ########## superset-frontend/packages/superset-ui-core/src/components/Modal/types.ts: ########## @@ -67,8 +67,8 @@ export interface StyledModalProps { export type { ModalFuncProps }; export interface FormModalProps extends ModalProps { - initialValues?: Object; - formSubmitHandler: (values: Object) => Promise<void>; + initialValues?: object; + formSubmitHandler: (values: object) => Promise<void>; Review Comment: ### Overly Generic Form Values Type <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? Using the generic 'object' type for form values lacks type safety and makes the interface too permissive. ###### Why this matters Without proper typing, consumers of this interface won't know what fields are available in the form values, leading to potential runtime errors and making the code harder to maintain. ###### Suggested change ∙ *Feature Preview* ```typescript // Define a generic type parameter for the form values export interface FormModalProps<T extends object = any> extends ModalProps { initialValues?: T; formSubmitHandler: (values: T) => Promise<void>; // ...rest of the interface } ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bf47bb01-3d55-4803-9305-b37d1004dab9/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bf47bb01-3d55-4803-9305-b37d1004dab9?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bf47bb01-3d55-4803-9305-b37d1004dab9?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bf47bb01-3d55-4803-9305-b37d1004dab9?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bf47bb01-3d55-4803-9305-b37d1004dab9) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:4d195d81-c841-4b74-8759-f1cc99f72f12 --> [](4d195d81-c841-4b74-8759-f1cc99f72f12) ########## superset-frontend/src/dashboard/components/Header/types.ts: ########## @@ -77,7 +77,7 @@ export interface HeaderProps { charts: ChartState | {}; colorScheme?: string; customCss: string; - user: Object | undefined; + user: object | undefined; Review Comment: ### Overly Generic User Type <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The user property type is defined as a generic 'object', which is too broad and provides no type safety or documentation of the expected structure. ###### Why this matters Using the generic 'object' type defeats TypeScript's type checking capabilities and makes it harder for developers to understand what properties are available on the user object. ###### Suggested change ∙ *Feature Preview* Create a specific interface for the user object that defines its expected properties: ```typescript interface User { id: number; username: string; // ... other relevant user properties } user: User | undefined; ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b71f43ac-3c13-4039-91b5-8e157d2c38aa/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b71f43ac-3c13-4039-91b5-8e157d2c38aa?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b71f43ac-3c13-4039-91b5-8e157d2c38aa?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b71f43ac-3c13-4039-91b5-8e157d2c38aa?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b71f43ac-3c13-4039-91b5-8e157d2c38aa) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:641ffada-e918-4759-9bc4-e4ab81aab52b --> [](641ffada-e918-4759-9bc4-e4ab81aab52b) -- 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]
