codeant-ai-for-open-source[bot] commented on code in PR #40985:
URL: https://github.com/apache/superset/pull/40985#discussion_r3399909107
##########
superset-frontend/packages/superset-ui-core/src/components/DropdownButton/index.tsx:
##########
@@ -57,10 +58,10 @@ export const DropdownButton = ({
defaultBtnCss,
css`
.ant-btn {
- height: 30px;
- box-shadow: none;
- font-size: ${theme.fontSizeSM}px;
- font-weight: ${theme.fontWeightStrong};
+ height: ${styleConfig?.controlHeight ?? 30}px;
Review Comment:
**Suggestion:** This hardcoded fallback height ignores the component's
`size` prop and also diverges from the shared button token defaults, so default
dropdown buttons render at 30px while regular default buttons render at 32px.
That creates visible misalignment in mixed button rows (for example, the
dataset footer) and breaks the expected AntD size contract. Use
theme/token-driven defaults (and/or size-aware logic) instead of a fixed `30`.
[api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Add Dataset footer buttons render at mismatched heights.
- ⚠️ SQL Lab run-dropdown button ignores AntD size contract.
- ⚠️ Theme buttonControlHeight tokens not honored by dropdown.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open
`superset-frontend/packages/superset-ui-core/src/components/Button/index.tsx`
and
note the default button height logic at lines 137-145, where `defaultHeight`
is computed
as `theme.buttonControlHeight ?? 32` for normal buttons and
`theme.buttonControlHeightSM
?? 30` only when `buttonSize === 'small'`.
2. Open
`superset-frontend/packages/superset-ui-core/src/components/DropdownButton/index.tsx`
and
observe that at line 61 the wrapper forces `.ant-btn { height:
${styleConfig?.controlHeight ?? 30}px; }`, ignoring any AntD `size` prop and
the shared
`buttonControlHeight*` tokens.
3. Open
`superset-frontend/src/features/datasets/AddDataset/Footer/index.tsx` and see
the
footer layout at lines 144-148 where a `Button` (Cancel) and a
`DropdownButton` (Create
dataset) are rendered side by side with no `styleConfig` overrides passed to
`DropdownButton`.
4. Run the UI and navigate to the "Add dataset" flow so that `Footer`
renders; the
`Button` uses the 32px default from `Button/index.tsx` while
`DropdownButton` uses the
hardcoded 30px height from `DropdownButton/index.tsx:61`, causing visible
vertical
misalignment between the two buttons and, for any future caller that sets
`size="small" |
"large"` on `DropdownButton` (supported via `DropdownButtonProps` in
`types.ts:31`), the
forced 30px height will continue to ignore that `size` prop.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=57a7f5b7500048d08d7e4552a099dc3f&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=57a7f5b7500048d08d7e4552a099dc3f&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/packages/superset-ui-core/src/components/DropdownButton/index.tsx
**Line:** 61:61
**Comment:**
*Api Mismatch: This hardcoded fallback height ignores the component's
`size` prop and also diverges from the shared button token defaults, so default
dropdown buttons render at 30px while regular default buttons render at 32px.
That creates visible misalignment in mixed button rows (for example, the
dataset footer) and breaks the expected AntD size contract. Use
theme/token-driven defaults (and/or size-aware logic) instead of a fixed `30`.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40985&comment_hash=95f5de352a01d28d736b7ef7799c9b8258b457ce583e8010e3235e5de516dd3c&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40985&comment_hash=95f5de352a01d28d736b7ef7799c9b8258b457ce583e8010e3235e5de516dd3c&reaction=dislike'>👎</a>
##########
superset-frontend/packages/superset-ui-core/src/components/DropdownButton/index.tsx:
##########
@@ -57,10 +58,10 @@ export const DropdownButton = ({
defaultBtnCss,
css`
.ant-btn {
- height: 30px;
- box-shadow: none;
- font-size: ${theme.fontSizeSM}px;
- font-weight: ${theme.fontWeightStrong};
+ height: ${styleConfig?.controlHeight ?? 30}px;
+ box-shadow: ${styleConfig?.boxShadow ?? 'none'};
+ font-size: ${styleConfig?.fontSize ?? theme.fontSizeSM}px;
Review Comment:
**Suggestion:** The fallback font size is wired to `fontSizeSM` instead of
the shared button font token, so global button typography customization does
not propagate to `DropdownButton` and this component drifts from `Button`
styling. Align this fallback with the same button token path used by the main
button component. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ DropdownButton text ignores theme.buttonFontSize overrides.
- ⚠️ Adjacent Button and DropdownButton show mismatched typography.
- ⚠️ Theming API for button fonts applied inconsistently across components.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open `superset-frontend/packages/superset-core/src/theme/types.ts` and
note that
`SupersetTheme` exposes a `buttonFontSize?: number` token at lines 244-251
as part of the
public theming surface.
2. Open
`superset-frontend/packages/superset-ui-core/src/components/Button/index.tsx`
and
see at line 122 that `btnFontSize` is computed as `theme.buttonFontSize ??
theme.fontSizeSM`, and at line 153 this `btnFontSize` is applied to
`resolvedStyleConfig.fontSize`, which is then used as `fontSize` in the
button's `css`
block at line 211—so any theme overriding `buttonFontSize` changes all core
`Button` text
sizes.
3. Open
`superset-frontend/packages/superset-ui-core/src/components/DropdownButton/index.tsx`
and
observe at line 63 that the dropdown button text uses `font-size:
${styleConfig?.fontSize
?? theme.fontSizeSM}px;`, bypassing `theme.buttonFontSize` entirely when
`styleConfig` is
not provided.
4. In a deployment that sets `buttonFontSize` to a value different from
`fontSizeSM` via
the Superset theme (as allowed by `SupersetTheme`), render any
`DropdownButton` usage such
as the Add Dataset footer
(`src/features/datasets/AddDataset/Footer/index.tsx:144-148`) or
the SQL Lab run menu (`src/SqlLab/components/RunQueryActionButton/index.tsx`
when
`overlayCreateAsMenu` is truthy); the standard `Button` text will reflect
the customized
`buttonFontSize` while `DropdownButton` text remains at `fontSizeSM`,
causing inconsistent
typography between adjacent buttons.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=582154bf7cec4fce95383dffc1960ecd&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=582154bf7cec4fce95383dffc1960ecd&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/packages/superset-ui-core/src/components/DropdownButton/index.tsx
**Line:** 63:63
**Comment:**
*Logic Error: The fallback font size is wired to `fontSizeSM` instead
of the shared button font token, so global button typography customization does
not propagate to `DropdownButton` and this component drifts from `Button`
styling. Align this fallback with the same button token path used by the main
button component.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40985&comment_hash=34c01f88e404b098e7bcab663fff4761f693f0593ce3b9059605184697dff424&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40985&comment_hash=34c01f88e404b098e7bcab663fff4761f693f0593ce3b9059605184697dff424&reaction=dislike'>👎</a>
##########
superset-frontend/src/features/home/Menu.tsx:
##########
@@ -344,15 +347,17 @@ export function Menu({
>
<StyledRow>
<StyledCol md={16} xs={24}>
- <Tooltip
- id="brand-tooltip"
- placement="bottomLeft"
- title={brand.tooltip}
- arrow={{ pointAtCenter: true }}
- >
- {renderBrand()}
- </Tooltip>
- {brand.text && (
+ {!brand.hide_logo && (
+ <Tooltip
+ id="brand-tooltip"
+ placement="bottomLeft"
+ title={brand.tooltip}
+ arrow={{ pointAtCenter: true }}
+ >
+ {renderBrand()}
+ </Tooltip>
+ )}
+ {brand.text && !brand.hide_logo && (
Review Comment:
**Suggestion:** The new condition ties brand text visibility to the logo
visibility flag, but `HIDE_NAVBAR_LOGO` is defined to hide only the logo. This
change suppresses the configured right-side brand text whenever the logo is
hidden, which breaks existing navbar behavior/config. Remove the
`!brand.hide_logo` part from the text-rendering condition so text can still
appear when only the logo is disabled. [incorrect condition logic]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Navbar brand text disappears when hiding only logo.
- ⚠️ Breaks documented semantics of HIDE_NAVBAR_LOGO flag.
- ⚠️ Affects main SPA navbar via App.tsx menu.
- ⚠️ Affects backend-rendered views using views/menu.tsx.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure the Superset instance to hide only the navbar logo but still
show right-side
text by setting `HIDE_NAVBAR_LOGO = True` and `LOGO_RIGHT_TEXT = "My Brand
Text"` in the
Python config, whose defaults and semantics are defined in
`superset/config.py:386-394`
(`# When True, hide the navbar logo.` and `# Specify any text that should
appear to the
right of the logo`).
2. The backend view `menu_data` in `superset/views/base.py:14-35` constructs
the `brand`
payload sent to the frontend, setting `brand["text"]` from `LOGO_RIGHT_TEXT`
and
`brand["hide_logo"]` from `HIDE_NAVBAR_LOGO`, as shown by the Grep result at
`superset/views/base.py:20-35`.
3. The React app entry in `superset-frontend/src/views/App.tsx:62-69` (and
similarly
`src/views/menu.tsx:21-23,41`) reads `bootstrapData.common.menu_data` from
the bootstrap
payload and passes it into the `Menu` component from
`src/features/home/Menu.tsx` as the
`data` prop, so `brand.text` and `brand.hide_logo` reach the `Menu`
component.
4. In `superset-frontend/src/features/home/Menu.tsx:350-361` (Grep result),
the header
renders the brand area with `{!brand.hide_logo &&
<Tooltip>{renderBrand()}</Tooltip>}` and
`{brand.text && !brand.hide_logo && (<StyledBrandText>...)}`; when
`HIDE_NAVBAR_LOGO` is
`True`, `brand.hide_logo` is `True`, so both conditions evaluate to `false`
and neither
the logo nor the configured `LOGO_RIGHT_TEXT` are rendered, contrary to the
config
contract that only the logo should be hidden—removing `!brand.hide_logo`
from the text
condition would restore the ability to show text when the logo is hidden.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=932b8b2c9e9e4db3b501c5e067705bbc&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=932b8b2c9e9e4db3b501c5e067705bbc&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/features/home/Menu.tsx
**Line:** 360:360
**Comment:**
*Incorrect Condition Logic: The new condition ties brand text
visibility to the logo visibility flag, but `HIDE_NAVBAR_LOGO` is defined to
hide only the logo. This change suppresses the configured right-side brand text
whenever the logo is hidden, which breaks existing navbar behavior/config.
Remove the `!brand.hide_logo` part from the text-rendering condition so text
can still appear when only the logo is disabled.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40985&comment_hash=2917176e58455a5b547fce167e785c3a11fc7490bbe61700b3c2784f778db0d5&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40985&comment_hash=2917176e58455a5b547fce167e785c3a11fc7490bbe61700b3c2784f778db0d5&reaction=dislike'>👎</a>
--
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]
