-
Notifications
You must be signed in to change notification settings - Fork 16k
fix(SqlLab): South pane visual changes #35601
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
Category | Issue | Status |
---|---|---|
Interface-Implementation Type Mismatch ▹ view | ||
Non-semantic button with no keyboard activation ▹ view | ✅ Fix detected | |
Missing null check for contentStyle CSS interpolation ▹ view | ||
Unstable React Key Implementation ▹ view | ||
Unmemoized Component Function ▹ view |
Files scanned
File Path | Reviewed |
---|---|
superset-frontend/src/components/ActionButton/index.tsx | ✅ |
superset-frontend/src/components/ListView/ActionsBar.tsx | ✅ |
superset-frontend/packages/superset-ui-core/src/components/Tabs/Tabs.tsx | ✅ |
superset-frontend/src/SqlLab/components/QueryHistory/index.tsx | ✅ |
superset-frontend/src/SqlLab/components/TablePreview/index.tsx | ✅ |
superset-frontend/src/SqlLab/components/ResultSet/index.tsx | ✅ |
superset-frontend/src/SqlLab/components/SqlEditor/index.tsx | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
export interface ActionProps { | ||
label: string; | ||
tooltip?: string | ReactElement; | ||
placement?: TooltipPlacement; | ||
icon: string; | ||
onClick: () => void; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interface-Implementation Type Mismatch 
Tell me more
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
export interface ActionProps {
label: string;
tooltip?: string | ReactElement;
placement?: TooltipPlacement;
icon: IconNameType;
onClick: () => void;
}
Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
<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> |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
.ant-tabs-content-holder { | ||
overflow: ${allowOverflow ? 'visible' : 'auto'}; | ||
${contentStyle} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing null check for contentStyle CSS interpolation 
Tell me more
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:
${contentStyle || ''}
Or use optional chaining with nullish coalescing:
${contentStyle ?? ''}
Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
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> | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unmemoized Component Function 
Tell me more
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:
const titleActions = useMemo(
() => (
<Flex>...
</Flex>
),
[theme, tableData.selectStar, tableData.view, refreshTableMetadata]
);
Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
); | ||
})} | ||
{actions.map((action, index) => ( | ||
<ActionButton key={action.tooltip ? undefined : index} {...action} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unstable React Key Implementation 
Tell me more
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:
type ActionProps = {
id: string; // Add this
label: string;
// ...
};
// Then use it as:
<ActionButton key={action.id} {...action} />
Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
); | ||
|
||
return tooltip ? ( | ||
<Tooltip id={`${label}-tooltip`} title={tooltip} placement={placement}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can labels being passed in currently contain spaces? If so, would we want to sanitize this first to remove spaces?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch, fixed!
SUMMARY
This pr improves the design on SouthPane on SQLLab according to design input. In the process i also extracted ActionButton to a seperate component so that it can be used in other places without duplicate code.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before:



After:



TESTING INSTRUCTIONS
Visual testing of sqllab southpane
ADDITIONAL INFORMATION