korbit-ai[bot] commented on code in PR #31979:
URL: https://github.com/apache/superset/pull/31979#discussion_r1932238799
##########
superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/ValidatedInputField.tsx:
##########
@@ -52,13 +54,13 @@ export const validatedInputField = ({
id={field}
name={field}
required={required}
- value={db?.parameters?.[field]}
+ value={db?.parameters?.[field as keyof DatabaseParameters]}
validationMethods={{ onBlur: getValidation }}
errorMessage={validationErrors?.[field]}
- placeholder={FIELD_TEXT_MAP[field].placeholder}
- helpText={FIELD_TEXT_MAP[field].helpText}
- label={FIELD_TEXT_MAP[field].label || field}
+ placeholder={FIELD_TEXT_MAP[field as FieldTextMapKey].placeholder}
+ helpText={FIELD_TEXT_MAP[field as 'account']?.helpText}
+ label={FIELD_TEXT_MAP[field as FieldTextMapKey].label || field}
onChange={changeMethods.onParametersChange}
- className={FIELD_TEXT_MAP[field].className || field}
+ className={FIELD_TEXT_MAP[field as 'warehouse' | 'role'].className ||
field}
Review Comment:
### Overly Restrictive Field Type Casting for className <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The field type is incorrectly cast to only 'warehouse' | 'role', which
restricts className access to only these two field types.
###### Why this matters
This will cause className to be incorrectly handled for other field types
that might need specific styling in the future.
###### Suggested change ∙ *Feature Preview*
Change the type casting to use FieldTextMapKey like other properties:
```typescript
className={FIELD_TEXT_MAP[field as FieldTextMapKey]?.className || field}
```
</details>
<sub>💡 Does this comment miss the mark? [Tell us
why](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c2d01f91-2d2c-4616-9ad4-beef78f20518?suggestedFixEnabled=true)
and Korbit will adapt to your team’s feedback.
💬 Chat with Korbit by mentioning @korbit-ai.
</sub>
<!--- korbi internal id:8c3331b4-82bf-4656-9ec3-3d5508dfd70f -->
##########
superset-frontend/src/dashboard/components/ColorSchemeControlWrapper.jsx:
##########
@@ -17,60 +17,35 @@
* under the License.
*/
/* eslint-env browser */
-import PropTypes from 'prop-types';
-import { PureComponent } from 'react';
import { getCategoricalSchemeRegistry, t } from '@superset-ui/core';
-
import ColorSchemeControl from
'src/explore/components/controls/ColorSchemeControl';
-const propTypes = {
- onChange: PropTypes.func,
- labelMargin: PropTypes.number,
- colorScheme: PropTypes.string,
- hasCustomLabelsColor: PropTypes.bool,
-};
-
-const defaultProps = {
- hasCustomLabelsColor: false,
- colorScheme: undefined,
- onChange: () => {},
+const ColorSchemeControlWrapper = ({
+ colorScheme,
+ labelMargin = 0,
+ hasCustomLabelsColor = false,
+ onChange = () => {},
+}) => {
Review Comment:
### Lost Hover State Functionality <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The removal of the `hovered` prop from ColorSchemeControl breaks existing
functionality that was previously implemented using the hover state.
###### Why this matters
The component previously tracked hover state and passed it to
ColorSchemeControl. Removing this functionality might affect user interaction
feedback or visual hover effects that were dependent on this state.
###### Suggested change ∙ *Feature Preview*
Either restore the hover state functionality or verify that it's no longer
needed. If it's still needed, implement it using hooks:
```jsx
const ColorSchemeControlWrapper = ({
colorScheme,
labelMargin = 0,
hasCustomLabelsColor = false,
onChange = () => {},
}) => {
const [hovered, setHovered] = useState(false);
const categoricalSchemeRegistry = getCategoricalSchemeRegistry();
const choices = categoricalSchemeRegistry.keys().map(s => [s, s]);
const schemes = categoricalSchemeRegistry.getMap();
return (
<ColorSchemeControl
description={t(
"Any color palette selected here will override the colors applied to
this dashboard's individual charts",
)}
labelMargin={labelMargin}
name="color_scheme"
onChange={onChange}
value={colorScheme}
choices={choices}
clearable
schemes={schemes}
hovered={hovered}
hasCustomLabelsColor={hasCustomLabelsColor}
/>
);
};
```
</details>
<sub>💡 Does this comment miss the mark? [Tell us
why](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a98b09f7-c3d8-4a5e-a970-9a202bf51d08?suggestedFixEnabled=true)
and Korbit will adapt to your team’s feedback.
💬 Chat with Korbit by mentioning @korbit-ai.
</sub>
<!--- korbi internal id:f3f65747-3ee8-4308-97b8-5e0aefdcefe9 -->
##########
superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/ValidatedInputField.tsx:
##########
@@ -52,13 +54,13 @@
id={field}
name={field}
required={required}
- value={db?.parameters?.[field]}
+ value={db?.parameters?.[field as keyof DatabaseParameters]}
validationMethods={{ onBlur: getValidation }}
errorMessage={validationErrors?.[field]}
- placeholder={FIELD_TEXT_MAP[field].placeholder}
- helpText={FIELD_TEXT_MAP[field].helpText}
- label={FIELD_TEXT_MAP[field].label || field}
+ placeholder={FIELD_TEXT_MAP[field as FieldTextMapKey].placeholder}
+ helpText={FIELD_TEXT_MAP[field as 'account']?.helpText}
Review Comment:
### Incorrect Field Type Casting for helpText <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The field type is incorrectly cast to only 'account', which means helpText
will only be available for account fields even when it might be needed for
other field types.
###### Why this matters
This will cause helpText to be undefined for all fields except 'account',
potentially hiding important help information from users for other field types.
###### Suggested change ∙ *Feature Preview*
Change the type casting to use FieldTextMapKey like other properties:
```typescript
helpText={FIELD_TEXT_MAP[field as FieldTextMapKey]?.helpText}
```
</details>
<sub>💡 Does this comment miss the mark? [Tell us
why](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7b8d57c8-7378-44ab-8e49-63175775f83e?suggestedFixEnabled=true)
and Korbit will adapt to your team’s feedback.
💬 Chat with Korbit by mentioning @korbit-ai.
</sub>
<!--- korbi internal id:d3b2c871-99e7-4e7f-8272-57ed3209e398 -->
##########
superset-frontend/src/SqlLab/utils/reduxStateToLocalStorageHelper.ts:
##########
@@ -97,9 +116,9 @@ export function clearQueryEditors(queryEditors:
QueryEditor[]) {
.reduce(
(accumulator, key) => ({
...accumulator,
- [key]: editor[key],
+ [key]: editor[key as keyof QueryEditor],
}),
- {},
+ {} as Record<string, string>,
Review Comment:
### Incorrect Type Assertion in QueryEditor Accumulator <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The type assertion for the accumulator in clearQueryEditors is incorrect.
QueryEditor values can be of different types, not just strings.
###### Why this matters
This will cause type errors when accessing non-string values from the
QueryEditor, potentially leading to runtime errors or data corruption when the
state is persisted.
###### Suggested change ∙ *Feature Preview*
Change the type assertion to match the correct types of QueryEditor fields:
```typescript
{} as Record<string, string | number | boolean>
```
</details>
<sub>💡 Does this comment miss the mark? [Tell us
why](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2abd8a06-e695-4587-942d-ec6696aef03b?suggestedFixEnabled=true)
and Korbit will adapt to your team’s feedback.
💬 Chat with Korbit by mentioning @korbit-ai.
</sub>
<!--- korbi internal id:fe6f9660-705e-4876-abc2-6a74233694d2 -->
##########
superset-frontend/src/dataMask/reducer.ts:
##########
@@ -43,10 +43,6 @@ import { areObjectsEqual } from '../reduxUtils';
export function getInitialDataMask(
id?: string | number,
- moreProps?: DataMask,
-): DataMask;
-export function getInitialDataMask(
- id: string | number,
moreProps: DataMask = {},
): DataMaskWithId {
Review Comment:
### Incorrect Type Return with Optional ID <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The function signature was simplified by removing an overload, but it still
accepts an optional 'id' while always returning DataMaskWithId which requires
an id.
###### Why this matters
This can lead to runtime errors when the function is called without an id
parameter since DataMaskWithId type requires an id property, but the function
will return an object without an id in this case.
###### Suggested change ∙ *Feature Preview*
The function should either always require an id parameter or return a union
type. Here's the corrected version:
```typescript
export function getInitialDataMask(
id: string | number,
moreProps: DataMask = {},
): DataMaskWithId;
```
Or if the id should be optional:
```typescript
export function getInitialDataMask(
id?: string | number,
moreProps: DataMask = {},
): DataMask | DataMaskWithId;
```
</details>
<sub>💡 Does this comment miss the mark? [Tell us
why](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0bdf53d0-148f-4f7b-bb6f-e1f758477f7c?suggestedFixEnabled=true)
and Korbit will adapt to your team’s feedback.
💬 Chat with Korbit by mentioning @korbit-ai.
</sub>
<!--- korbi internal id:859bf7fc-b4f2-42a6-a5de-0b9f21dea1a8 -->
##########
superset-frontend/src/dashboard/components/SyncDashboardState/index.tsx:
##########
@@ -66,10 +67,13 @@ const selectDashboardContextForExplore = createSelector(
(state: RootState) => state.dataMask,
],
(metadata, dashboardId, colorScheme, filters, dataMask) => {
- const nativeFilters = Object.keys(filters).reduce((acc, key) => {
- acc[key] = pick(filters[key], ['chartsInScope']);
- return acc;
- }, {});
+ const nativeFilters = Object.keys(filters).reduce(
+ (acc, key) => {
+ acc[key] = pick(filters[key], ['chartsInScope']);
+ return acc;
+ },
+ {} as Record<string, Pick<Filter | Divider, 'chartsInScope'>>,
+ );
Review Comment:
### Incorrect Type Union in Native Filters Reduction <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The type assertion allows 'Divider' type which cannot have 'chartsInScope'
property, potentially causing runtime errors.
###### Why this matters
If a Divider type is encountered during runtime, accessing 'chartsInScope'
will fail as this property doesn't exist on Divider objects, leading to
potential undefined behavior or crashes.
###### Suggested change ∙ *Feature Preview*
Remove the Divider type from the type assertion as it's not applicable for
this operation:
```typescript
const nativeFilters = Object.keys(filters).reduce(
(acc, key) => {
acc[key] = pick(filters[key], ['chartsInScope']);
return acc;
},
{} as Record<string, Pick<Filter, 'chartsInScope'>>,
);
```
</details>
<sub>💡 Does this comment miss the mark? [Tell us
why](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/dfe48b19-de41-48d8-aa73-012408193666?suggestedFixEnabled=true)
and Korbit will adapt to your team’s feedback.
💬 Chat with Korbit by mentioning @korbit-ai.
</sub>
<!--- korbi internal id:9ef84510-95bb-4ca8-a04f-90e7c4ee926a -->
##########
superset-frontend/src/dashboard/types.ts:
##########
@@ -183,28 +184,32 @@ export type Charts = { [key: number]: Chart };
type ComponentTypesKeys = keyof typeof componentTypes;
export type ComponentType = (typeof componentTypes)[ComponentTypesKeys];
+export type LayoutItemMeta = {
+ chartId: number;
+ defaultText?: string;
+ height: number;
+ placeholder?: string;
+ sliceName?: string;
+ sliceNameOverride?: string;
+ text?: string;
+ uuid: string;
+ width: number;
+};
+
/** State of dashboardLayout item in redux */
export type LayoutItem = {
children: string[];
parents?: string[];
type: ComponentType;
id: string;
- meta: {
- chartId: number;
- defaultText?: string;
- height: number;
- placeholder?: string;
- sliceName?: string;
- sliceNameOverride?: string;
- text?: string;
- uuid: string;
- width: number;
- };
+ meta: LayoutItemMeta;
};
type ActiveFilter = {
+ filterType?: string;
+ targets: number[] | [Partial<NativeFilterTarget>];
scope: number[];
- values: ExtraFormData;
+ values?: ExtraFormData;
};
Review Comment:
### Potentially Breaking Optional Values Property <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The `values` property in ActiveFilter is now optional (with '?'), but
there's no indication in the developer's intent that this is intended. This
could break existing code that expects values to always be present.
###### Why this matters
If existing code assumes `values` is always present and doesn't check for
undefined, it could lead to runtime errors when accessing the values property.
###### Suggested change ∙ *Feature Preview*
Either revert the change to make values required, or ensure all consuming
code handles the optional case:
```typescript
type ActiveFilter = {
filterType?: string;
targets: number[] | [Partial<NativeFilterTarget>];
scope: number[];
values: ExtraFormData; // Remove the optional '?' if values should always
be present
};
```
</details>
<sub>💡 Does this comment miss the mark? [Tell us
why](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cee9d275-c7cb-4328-92ee-0550a72ce34a?suggestedFixEnabled=true)
and Korbit will adapt to your team’s feedback.
💬 Chat with Korbit by mentioning @korbit-ai.
</sub>
<!--- korbi internal id:a4f11c61-3a72-4000-b226-5fce57cb6555 -->
##########
superset-frontend/src/components/Select/utils.tsx:
##########
@@ -84,10 +84,12 @@ export function hasOption(
* */
export const propertyComparator =
(property: string) => (a: AntdLabeledValue, b: AntdLabeledValue) => {
- if (typeof a[property] === 'string' && typeof b[property] === 'string') {
- return a[property].localeCompare(b[property]);
+ const propertyA = a[property as keyof LabeledValue];
+ const propertyB = b[property as keyof LabeledValue];
+ if (typeof propertyA === 'string' && typeof propertyB === 'string') {
+ return propertyA.localeCompare(propertyB);
}
- return (a[property] as number) - (b[property] as number);
+ return Number(propertyA) - Number(propertyB);
Review Comment:
### Unsafe numeric conversion in comparator <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The propertyComparator function assumes numeric conversion will work for all
non-string properties, which could lead to incorrect sorting behavior with
non-numeric values.
###### Why this matters
Using Number() on non-numeric values returns NaN, and NaN minus NaN equals
NaN. This could cause unexpected sorting results or break the sorting
functionality completely.
###### Suggested change ∙ *Feature Preview*
Add type checking and error handling for non-string, non-numeric values:
```typescript
if (typeof propertyA === 'string' && typeof propertyB === 'string') {
return propertyA.localeCompare(propertyB);
}
if (typeof propertyA === 'number' && typeof propertyB === 'number') {
return propertyA - propertyB;
}
return String(propertyA).localeCompare(String(propertyB)); // fallback to
string comparison
```
</details>
<sub>💡 Does this comment miss the mark? [Tell us
why](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5bad74ed-5f7d-4248-81bd-146ae258fb9b?suggestedFixEnabled=true)
and Korbit will adapt to your team’s feedback.
💬 Chat with Korbit by mentioning @korbit-ai.
</sub>
<!--- korbi internal id:39f52419-0286-46e0-97bb-7d671dee4b64 -->
##########
superset-frontend/plugins/plugin-chart-echarts/src/Bubble/transformProps.ts:
##########
@@ -38,18 +38,37 @@ import { getPadding } from '../Timeseries/transformers';
import { convertInteger } from '../utils/convertInteger';
import { NULL_STRING } from '../constants';
+const isIterable = (obj: any): obj is Iterable<any> =>
+ obj != null && typeof obj[Symbol.iterator] === 'function';
+
function normalizeSymbolSize(
nodes: ScatterSeriesOption[],
maxBubbleValue: number,
) {
- const [bubbleMinValue, bubbleMaxValue] = extent(nodes, x =>
x.data?.[0]?.[2]);
- const nodeSpread = bubbleMaxValue - bubbleMinValue;
- nodes.forEach(node => {
- // eslint-disable-next-line no-param-reassign
- node.symbolSize =
- (((node.data?.[0]?.[2] - bubbleMinValue) / nodeSpread) *
- (maxBubbleValue * 2) || 0) + MINIMUM_BUBBLE_SIZE;
- });
+ const [bubbleMinValue, bubbleMaxValue] = extent<ScatterSeriesOption, number>(
+ nodes,
+ x => {
+ const tmpValue = x.data?.[0];
+ const result = isIterable(tmpValue) ? tmpValue[2] : null;
+ if (typeof result === 'number') {
+ return result;
+ }
+ return null;
+ },
+ );
+ if (bubbleMinValue && bubbleMaxValue) {
Review Comment:
### Zero Value Edge Case in Bubble Size Normalization <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The condition assumes bubbleMinValue is falsy when it's zero, which is a
valid minimum value for bubble sizes.
###### Why this matters
This can cause the bubble size normalization to fail when the minimum value
is 0, leading to incorrectly sized bubbles in the visualization.
###### Suggested change ∙ *Feature Preview*
Replace the condition with an explicit check for null values:
```typescript
if (bubbleMinValue !== null && bubbleMaxValue !== null) {
```
</details>
<sub>💡 Does this comment miss the mark? [Tell us
why](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8ea97f82-1fd9-40ed-b82a-cfe23d0ab239?suggestedFixEnabled=true)
and Korbit will adapt to your team’s feedback.
💬 Chat with Korbit by mentioning @korbit-ai.
</sub>
<!--- korbi internal id:7959251a-992a-4603-b7d6-41ba647e3606 -->
##########
superset-frontend/src/features/tags/tags.ts:
##########
@@ -197,7 +197,7 @@ export function fetchObjectsByTagIds(
{
tagIds = [],
types,
- }: { tagIds: number[] | undefined; types: string | null },
+ }: { tagIds: (number | undefined)[] | string; types: string | null },
Review Comment:
### Invalid tagIds Type Union <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The tagIds parameter allows either an array containing undefined values or a
string, which can lead to runtime errors when accessing the /tag/get_objects/
endpoint.
###### Why this matters
The server endpoint likely expects either a clean array of numbers or a
string representation. Including undefined values or mixing types could cause
API failures or unpredictable behavior.
###### Suggested change ∙ *Feature Preview*
Change the type to be more specific based on the actual API requirements.
For example:
```typescript
tagIds: number[] | string;
```
</details>
<sub>💡 Does this comment miss the mark? [Tell us
why](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ef82ab20-4a3e-4cc9-b030-a9bb00c7f2b5?suggestedFixEnabled=true)
and Korbit will adapt to your team’s feedback.
💬 Chat with Korbit by mentioning @korbit-ai.
</sub>
<!--- korbi internal id:d195038e-a9cf-4498-b14d-cf28013a1749 -->
--
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]
