korbit-ai[bot] commented on code in PR #35601:
URL: https://github.com/apache/superset/pull/35601#discussion_r2418773774
##########
superset-frontend/src/components/ListView/ActionsBar.tsx:
##########
@@ -59,49 +55,14 @@ interface ActionsBarProps {
const StyledActions = styled.span`
white-space: nowrap;
min-width: 100px;
- .action-button {
- cursor: pointer;
- color: ${({ theme }) => theme.colorIcon};
- margin-right: ${({ theme }) => theme.sizeUnit}px;
- &:hover {
- path {
- fill: ${({ theme }) => theme.colorPrimary};
- }
- }
- }
`;
export function ActionsBar({ actions }: ActionsBarProps) {
return (
<StyledActions className="actions">
- {actions.map((action, index) => {
- const ActionIcon = Icons[action.icon as IconNameType];
- const actionButton = (
- <span
- role="button"
- tabIndex={0}
- style={{ cursor: 'pointer' }}
- className="action-button"
- data-test={action.label}
- onClick={action.onClick}
- key={action.tooltip ? undefined : index}
- >
- <ActionIcon iconSize="l" />
- </span>
- );
- return action.tooltip ? (
- <Tooltip
- id={`${action.label}-tooltip`}
- title={action.tooltip}
- placement={action.placement}
- key={index}
- >
- {actionButton}
- </Tooltip>
- ) : (
- actionButton
- );
- })}
+ {actions.map((action, index) => (
+ <ActionButton key={action.tooltip ? undefined : index} {...action} />
Review Comment:
### Unstable React Key Implementation <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The key prop is using a conditional that can result in undefined, which
defeats the purpose of having a stable key for React's reconciliation process.
###### Why this matters
Using unstable or undefined keys can lead to rendering issues and
performance degradation as React won't be able to properly track and update
components.
###### Suggested change ∙ *Feature Preview*
Use a more stable and unique key. Consider using a combination of the action
properties or require an id property in the ActionProps type:
```typescript
type ActionProps = {
id: string; // Add this
label: string;
// ...
};
// Then use it as:
<ActionButton key={action.id} {...action} />
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d3b03122-7c04-46f4-af0b-a586fc8b54a0/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d3b03122-7c04-46f4-af0b-a586fc8b54a0?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d3b03122-7c04-46f4-af0b-a586fc8b54a0?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d3b03122-7c04-46f4-af0b-a586fc8b54a0?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d3b03122-7c04-46f4-af0b-a586fc8b54a0)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:96cfe84f-60e9-4f35-9eef-47ee23c9cf75 -->
[](96cfe84f-60e9-4f35-9eef-47ee23c9cf75)
##########
superset-frontend/packages/superset-ui-core/src/components/Tabs/Tabs.tsx:
##########
@@ -46,6 +49,7 @@ const StyledTabs = ({
.ant-tabs-content-holder {
overflow: ${allowOverflow ? 'visible' : 'auto'};
+ ${contentStyle}
Review Comment:
### Missing null check for contentStyle CSS interpolation <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The contentStyle prop is being interpolated directly into CSS without
null/undefined checks, which could cause rendering issues if the prop is not
provided.
###### Why this matters
When contentStyle is undefined or null, the CSS interpolation will render
'undefined' or 'null' as literal text in the CSS, potentially breaking styles
or causing console warnings.
###### Suggested change ∙ *Feature Preview*
Add a null check before interpolating contentStyle:
```typescript
${contentStyle || ''}
```
Or use optional chaining with nullish coalescing:
```typescript
${contentStyle ?? ''}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/edd0b27f-de50-4604-8e97-d6a726fff33c/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/edd0b27f-de50-4604-8e97-d6a726fff33c?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/edd0b27f-de50-4604-8e97-d6a726fff33c?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/edd0b27f-de50-4604-8e97-d6a726fff33c?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/edd0b27f-de50-4604-8e97-d6a726fff33c)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:ef435de6-1d1e-4878-ba1c-c8e359072e6d -->
[](ef435de6-1d1e-4878-ba1c-c8e359072e6d)
##########
superset-frontend/src/components/ActionButton/index.tsx:
##########
@@ -0,0 +1,75 @@
+/**
+ * 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 type { ReactElement } from 'react';
+import {
+ Icons,
+ type IconNameType,
+ Tooltip,
+ type TooltipPlacement,
+} from '@superset-ui/core/components';
+import { css, useTheme } from '@superset-ui/core';
+
+export interface ActionProps {
+ label: string;
+ tooltip?: string | ReactElement;
+ placement?: TooltipPlacement;
+ icon: string;
+ onClick: () => void;
+}
Review Comment:
### Interface-Implementation Type Mismatch <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The 'icon' prop is typed as string but is used as IconNameType in the
component, creating a potential type mismatch.
###### Why this matters
This design creates a hidden coupling between the interface and
implementation. Consumers of the component might pass any string value, leading
to runtime errors when the string is not a valid IconNameType.
###### Suggested change ∙ *Feature Preview*
```typescript
export interface ActionProps {
label: string;
tooltip?: string | ReactElement;
placement?: TooltipPlacement;
icon: IconNameType;
onClick: () => void;
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/10b49a6a-2f51-4058-9a65-4719620c8017/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/10b49a6a-2f51-4058-9a65-4719620c8017?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/10b49a6a-2f51-4058-9a65-4719620c8017?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/10b49a6a-2f51-4058-9a65-4719620c8017?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/10b49a6a-2f51-4058-9a65-4719620c8017)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:ce16ddc5-1163-4d0e-9068-6c0cfaf0da8b -->
[](ce16ddc5-1163-4d0e-9068-6c0cfaf0da8b)
##########
superset-frontend/src/SqlLab/components/TablePreview/index.tsx:
##########
@@ -240,16 +226,37 @@ const TablePreview: FC<Props> = ({ dbId, catalog, schema,
tableName }) => {
],
);
- const dropdownMenu = useMemo(() => {
- let menus = [...MENUS];
- if (!tableData.selectStar) {
- menus = menus.filter(({ key }) => key !== 'copy-select-statement');
- }
- if (!tableData.view) {
- menus = menus.filter(({ key }) => key !== 'show-create-view-statement');
- }
- return menus;
- }, [tableData.view, tableData.selectStar]);
+ const titleActions = () => (
+ <Flex
+ align="center"
+ css={css`
+ padding-left: ${theme.sizeUnit * 4}px;
+ `}
+ >
+ <ActionButton
+ label="Refresh table schema"
+ tooltip={t('Refresh table schema')}
+ icon="SyncOutlined"
+ onClick={refreshTableMetadata}
+ />
+ {tableData.selectStar && (
+ <ActionButton
+ label="Copy SELECT statement"
+ icon="CopyOutlined"
+ tooltip={t('Copy SELECT statement')}
+ onClick={() => copyStatementActionRef.current?.click()}
+ />
+ )}
+ {tableData.view && (
+ <ActionButton
+ label="Show CREATE VIEW statement"
+ icon="EyeOutlined"
+ tooltip={t('Show CREATE VIEW statement')}
+ onClick={() => showViewStatementActionRef.current?.click()}
+ />
+ )}
+ </Flex>
+ );
Review Comment:
### Unmemoized Component Function <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The titleActions function is defined inside the component, causing it to be
recreated on every render. It should be memoized or moved outside the component
since it depends on stable props and callbacks.
###### Why this matters
Recreating the function on every render can lead to unnecessary re-renders
of child components and impact performance, especially in larger applications.
###### Suggested change ∙ *Feature Preview*
Use useMemo to memoize the titleActions function:
```typescript
const titleActions = useMemo(
() => (
<Flex>...
</Flex>
),
[theme, tableData.selectStar, tableData.view, refreshTableMetadata]
);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0fd0b2bb-cefb-42fc-9e9b-16fe5ed17849/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0fd0b2bb-cefb-42fc-9e9b-16fe5ed17849?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0fd0b2bb-cefb-42fc-9e9b-16fe5ed17849?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0fd0b2bb-cefb-42fc-9e9b-16fe5ed17849?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0fd0b2bb-cefb-42fc-9e9b-16fe5ed17849)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:ae40d57b-e9d7-4d74-99d5-87432ee2ff99 -->
[](ae40d57b-e9d7-4d74-99d5-87432ee2ff99)
##########
superset-frontend/src/components/ActionButton/index.tsx:
##########
@@ -0,0 +1,75 @@
+/**
+ * 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 type { ReactElement } from 'react';
+import {
+ Icons,
+ type IconNameType,
+ Tooltip,
+ type TooltipPlacement,
+} from '@superset-ui/core/components';
+import { css, useTheme } from '@superset-ui/core';
+
+export interface ActionProps {
+ label: string;
+ tooltip?: string | ReactElement;
+ placement?: TooltipPlacement;
+ icon: string;
+ onClick: () => void;
+}
+
+export const ActionButton = ({
+ label,
+ tooltip,
+ placement,
+ icon,
+ onClick,
+}: ActionProps) => {
+ const theme = useTheme();
+ const ActionIcon = Icons[icon as IconNameType];
+ const actionButton = (
+ <span
+ role="button"
+ tabIndex={0}
+ css={css`
+ cursor: pointer;
+ color: ${theme.colorIcon};
+ margin-right: ${theme.sizeUnit}px;
+ &:hover {
+ path {
+ fill: ${theme.colorPrimary};
+ }
+ }
+ `}
+ className="action-button"
+ data-test={label}
+ onClick={onClick}
+ >
+ <ActionIcon iconSize="l" />
+ </span>
Review Comment:
### Non-semantic button with no keyboard activation <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Non-semantic clickable element used as a button without native button
semantics or keyboard activation handling.
###### Why this matters
Without native button semantics, keyboard users (e.g., pressing Enter/Space)
may not activate the control, reducing accessibility and potentially breaking
expected functionality.
###### Suggested change ∙ *Feature Preview*
Use a native button element (type="button") for proper semantics and
built-in keyboard accessibility. Example:
```tsx
<button
type="button"
className="action-button"
data-test={label}
onClick={onClick}
css={css`
cursor: pointer;
color: ${theme.colorIcon};
margin-right: ${theme.sizeUnit}px;
background: transparent;
border: none;
padding: 0;
&:hover {
path {
fill: ${theme.colorPrimary};
}
}
`}
aria-label={label}
>
<ActionIcon iconSize="l" />
</button>
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b84004fa-8be4-4a77-9a85-ff8645d28790/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b84004fa-8be4-4a77-9a85-ff8645d28790?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b84004fa-8be4-4a77-9a85-ff8645d28790?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b84004fa-8be4-4a77-9a85-ff8645d28790?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b84004fa-8be4-4a77-9a85-ff8645d28790)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:f12fcddc-92a3-41b5-b1b5-d529a9f937ae -->
[](f12fcddc-92a3-41b5-b1b5-d529a9f937ae)
--
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]
