korbit-ai[bot] commented on code in PR #33962:
URL: https://github.com/apache/superset/pull/33962#discussion_r2173185608
##########
superset-frontend/packages/superset-ui-demo/storybook/stories/superset-ui-theme/Theme.stories.tsx:
##########
@@ -63,32 +60,29 @@ const AntDFunctionalColors = ({ antdTheme }) => {
</tr>
</thead>
<tbody>
- {colorTypes.map(type => {
- const typeKey = `color${type}`;
- return (
- <tr key={type}>
- <td style={{ border: '1px solid #ddd', padding: '8px' }}>
- <strong>{type}</strong>
- </td>
- {variants.map(variant => {
- const color = themeObject.getColorVariants(type)[variant];
- return (
- <td
- key={variant}
- style={{
- border: '1px solid #ddd',
- padding: '8px',
- backgroundColor: color || 'transparent',
- color: [`color${type}${variant}`],
- }}
- >
- {color ? <code>{color}</code> : '-'}
- </td>
- );
- })}
- </tr>
- );
- })}
+ {colorTypes.map(type => (
+ <tr key={type}>
+ <td style={{ border: '1px solid #ddd', padding: '8px' }}>
+ <strong>{type}</strong>
+ </td>
+ {variants.map(variant => {
+ const color = themeObject.getColorVariants(type)[variant];
Review Comment:
### Redundant Color Variant Calculations <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
getColorVariants() is called repeatedly within nested map functions without
memoization, potentially causing unnecessary recalculations.
###### Why this matters
With nested loops over colorTypes and variants, this function is called
multiple times for the same type parameter, causing redundant computations that
could impact rendering performance.
###### Suggested change ∙ *Feature Preview*
Memoize the color variants calculation outside the nested loops using
useMemo or by lifting the calculation:
```typescript
const AntDFunctionalColors = () => {
const colorVariantsMap = useMemo(
() => colorTypes.reduce((acc, type) => ({
...acc,
[type]: themeObject.getColorVariants(type)
}), {}),
[]
);
// Then in the render:
const color = colorVariantsMap[type][variant];
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9732d36c-f4f4-4179-8ef3-ce5d0a55d191/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9732d36c-f4f4-4179-8ef3-ce5d0a55d191?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9732d36c-f4f4-4179-8ef3-ce5d0a55d191?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9732d36c-f4f4-4179-8ef3-ce5d0a55d191?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9732d36c-f4f4-4179-8ef3-ce5d0a55d191)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:22935ccd-66f3-457c-ae9d-1ce003fe6b9e -->
[](22935ccd-66f3-457c-ae9d-1ce003fe6b9e)
##########
superset-frontend/packages/superset-ui-core/src/time-format/TimeFormatter.ts:
##########
@@ -21,6 +21,8 @@ import { isRequired } from '../utils';
import { TimeFormatFunction } from './types';
import stringifyTimeInput from './utils/stringifyTimeInput';
+/* eslint-disable @typescript-eslint/no-unsafe-declaration-merging */
+
export const PREVIEW_TIME = new Date(Date.UTC(2017, 1, 14, 11, 22, 33));
Review Comment:
### Unexplained Date Magic Numbers <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The hardcoded date values in PREVIEW_TIME lack explanation for why these
specific numbers were chosen.
###### Why this matters
Magic numbers in date construction make it difficult to understand the
significance of the chosen preview date.
###### Suggested change ∙ *Feature Preview*
```typescript
// February 14, 2017 11:22:33 UTC - Example date for formatting preview
export const PREVIEW_TIME = new Date(Date.UTC(2017, 1, 14, 11, 22, 33));
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a0b28a62-cc24-4542-bdbb-d40cc962f7af/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a0b28a62-cc24-4542-bdbb-d40cc962f7af?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a0b28a62-cc24-4542-bdbb-d40cc962f7af?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a0b28a62-cc24-4542-bdbb-d40cc962f7af?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a0b28a62-cc24-4542-bdbb-d40cc962f7af)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:b61ad9a8-a5a0-437c-aef3-407bb86e901c -->
[](b61ad9a8-a5a0-437c-aef3-407bb86e901c)
##########
superset-frontend/packages/superset-ui-core/src/number-format/NumberFormatter.ts:
##########
@@ -20,6 +20,8 @@ import { ExtensibleFunction } from '../models';
import { isRequired } from '../utils';
import { NumberFormatFunction } from './types';
+/* eslint-disable @typescript-eslint/no-unsafe-declaration-merging */
Review Comment:
### Unexplained ESLint Disable Comment <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The ESLint disable comment lacks explanation of why this rule needs to be
disabled.
###### Why this matters
Future developers may not understand why this rule was disabled, leading to
confusion or inadvertent reintroduction of issues.
###### Suggested change ∙ *Feature Preview*
```typescript
/* eslint-disable @typescript-eslint/no-unsafe-declaration-merging --
Required for TypeScript function type augmentation pattern */
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e3e92a2f-f407-4d3b-b3f8-1b108b12f9fd/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e3e92a2f-f407-4d3b-b3f8-1b108b12f9fd?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e3e92a2f-f407-4d3b-b3f8-1b108b12f9fd?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e3e92a2f-f407-4d3b-b3f8-1b108b12f9fd?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e3e92a2f-f407-4d3b-b3f8-1b108b12f9fd)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:23551140-a181-4d14-b13a-7d9d83076cca -->
[](23551140-a181-4d14-b13a-7d9d83076cca)
--
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: notifications-unsubscr...@superset.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org
