feat(theming): make core components fully configurable via theme tokens#40985
feat(theming): make core components fully configurable via theme tokens#40985anamitraadhikari wants to merge 1 commit into
Conversation
dosubot
Bot
added
change:frontend
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| {brand.text && !brand.hide_logo && ( | ||
| <StyledBrandText> | ||
| <span>{brand.text}</span> | ||
| </StyledBrandText> |
There was a problem hiding this comment.
🟠 Architect Review — HIGH
HIDE_NAVBAR_LOGO is wired to brand.hide_logo in a way that also suppresses brand.text, so enabling "hide the navbar logo" hides both the logo image and the right-side branding text, contradicting the config comment and preventing a text-only branding configuration.
Suggestion: Decouple logo and text visibility: keep using hide_logo to skip the logo/tooltip block, but render brand.text independently (e.g., {brand.text && <StyledBrandText>...}) so LOGO_RIGHT_TEXT works even when the logo is hidden.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is an **Architect / Logical Review** comment left during a code review. These reviews are first-class, important findings — not optional suggestions. Do NOT dismiss this as a 'big architectural change' just because the title says architect review; most of these can be resolved with a small, localized fix once the intent is understood.
**Path:** superset-frontend/src/features/home/Menu.tsx
**Line:** 360:363
**Comment:**
*HIGH: `HIDE_NAVBAR_LOGO` is wired to `brand.hide_logo` in a way that also suppresses `brand.text`, so enabling "hide the navbar logo" hides both the logo image and the right-side branding text, contradicting the config comment and preventing a text-only branding configuration.
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.
If a suggested approach is provided above, use it as the authoritative instruction. If no explicit code suggestion is given, you MUST still draft and apply your own minimal, localized fix — do not punt back with 'no suggestion provided, review manually'. Keep the change as small as possible: add a guard clause, gate on a loading state, reorder an await, wrap in a conditional, etc. Do not refactor surrounding code or expand scope beyond the finding.
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
|
The architectural review finding is correct. In the current implementation of To resolve this, you should decouple the visibility of the logo and the text. The logo and its tooltip should be gated by Here is the corrected implementation for {!brand.hide_logo && (
<Tooltip
id="brand-tooltip"
placement="bottomLeft"
title={brand.tooltip}
arrow={{ pointAtCenter: true }}
>
{renderBrand()}
</Tooltip>
)}
{brand.text && (
<StyledBrandText>
<span>{brand.text}</span>
</StyledBrandText>
)}Would you like me to check the other comments on this PR and implement fixes for them as well? superset-frontend/src/features/home/Menu.tsx |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #40985 +/- ##
==========================================
- Coverage 64.30% 64.30% -0.01%
==========================================
Files 2657 2657
Lines 144060 144096 +36
Branches 33216 33248 +32
==========================================
+ Hits 92641 92660 +19
- Misses 49797 49812 +15
- Partials 1622 1624 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| box-shadow: none; | ||
| font-size: ${theme.fontSizeSM}px; | ||
| font-weight: ${theme.fontWeightStrong}; | ||
| height: ${styleConfig?.controlHeight ?? 30}px; |
There was a problem hiding this 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]
Severity Level: Major ⚠️
- ⚠️ Add Dataset footer buttons render at mismatched heights.
- ⚠️ SQL Lab run-dropdown button ignores AntD size contract.
- ⚠️ Theme buttonControlHeight tokens not honored by dropdown.Steps of Reproduction ✅
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.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
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| font-weight: ${theme.fontWeightStrong}; | ||
| height: ${styleConfig?.controlHeight ?? 30}px; | ||
| box-shadow: ${styleConfig?.boxShadow ?? 'none'}; | ||
| font-size: ${styleConfig?.fontSize ?? theme.fontSizeSM}px; |
There was a problem hiding this 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]
Severity Level: Major ⚠️
- ⚠️ DropdownButton text ignores theme.buttonFontSize overrides.
- ⚠️ Adjacent Button and DropdownButton show mismatched typography.
- ⚠️ Theming API for button fonts applied inconsistently across components.Steps of Reproduction ✅
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.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
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
| {renderBrand()} | ||
| </Tooltip> | ||
| )} | ||
| {brand.text && !brand.hide_logo && ( |
There was a problem hiding this 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]
Severity Level: Major ⚠️
- ⚠️ 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.Steps of Reproduction ✅
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.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
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![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #03fc85
Actionable Suggestions - 2
-
superset-frontend/packages/superset-ui-core/src/components/Button/Button.stories.tsx - 1
- Missing borderRadius in description · Line 130-130
-
superset-frontend/packages/superset-ui-core/src/components/DropdownButton/index.tsx - 1
- Missing unit tests for styleConfig · Line 30-30
Additional Suggestions - 3
-
superset-frontend/packages/superset-ui-core/src/components/Button/types.ts - 1
-
Missing unit tests for styleConfig · Line 63-63The new `styleConfig` prop lacks unit test coverage. Existing tests verify secondary button styles and onClick behavior, but there's no test validating that custom style properties (controlHeight, fontSize, etc.) are correctly applied to rendered buttons. Rule [6262] requires tests to verify actual behavior logic.
-
-
superset/config.py - 1
-
Missing test coverage · Line 386-387Consider adding unit tests for the `HIDE_NAVBAR_LOGO` config option. Per BITO.md rule [11730], new features should include tests covering success paths and validation. Existing tests in `tests/unit_tests/views/test_base.py` could be extended to verify `hide_logo` appears in the bootstrap payload.
-
-
superset-frontend/packages/superset-ui-core/src/components/DropdownButton/types.ts - 1
-
Inconsistent style config API · Line 24-29The new `DropdownButtonStyleConfig` type exposes only 4 properties while the analogous `ButtonStyleConfig` (line 43 in Button/types.ts) has 8 properties including `paddingInline`, `borderRadius`, `ctaMinWidth`, `ctaMinHeight`, and `iconGap`. This creates an inconsistent API between related button components. If DropdownButton intentionally supports fewer customization options, consider adding a JSDoc comment explaining the design rationale.
-
Filtered by Review Rules
Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
-
superset-frontend/packages/superset-core/src/theme/types.ts - 1
- Missing JSDoc documentation · Line 242-250
-
superset-frontend/src/features/home/Menu.tsx - 1
- Dead code in renderBrand function · Line 293-295
Review Details
-
Files reviewed - 17 · Commit Range:
adb0eb0..adb0eb0- superset-frontend/packages/superset-core/src/theme/types.ts
- superset-frontend/packages/superset-ui-core/src/components/Button/Button.stories.tsx
- superset-frontend/packages/superset-ui-core/src/components/Button/index.tsx
- superset-frontend/packages/superset-ui-core/src/components/Button/types.ts
- superset-frontend/packages/superset-ui-core/src/components/DropdownButton/DropdownButton.stories.tsx
- superset-frontend/packages/superset-ui-core/src/components/DropdownButton/index.tsx
- superset-frontend/packages/superset-ui-core/src/components/DropdownButton/types.ts
- superset-frontend/packages/superset-ui-core/src/components/Label/index.tsx
- superset-frontend/packages/superset-ui-core/src/components/PageHeaderWithActions/index.tsx
- superset-frontend/packages/superset-ui-core/src/components/Select/styles.tsx
- superset-frontend/src/components/ListView/CardCollection.tsx
- superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx
- superset-frontend/src/dashboard/components/gridComponents/TabsRenderer/TabsRenderer.tsx
- superset-frontend/src/features/home/Menu.tsx
- superset-frontend/src/types/bootstrapTypes.ts
- superset/config.py
- superset/views/base.py
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Eslint (Linter) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| }, | ||
| styleConfig: { | ||
| description: | ||
| 'Optional visual overrides (controlHeight, paddingInline, fontSize, fontWeight, ctaMinWidth, ctaMinHeight, iconGap).', |
There was a problem hiding this comment.
The styleConfig description lists 7 properties but ButtonStyleConfig (types.ts:43-52) includes 8 properties — borderRadius is missing. Users relying on Storybook controls will not know this option exists.
Code suggestion
Check the AI-generated fix before applying
--- superset-frontend/packages/superset-ui-core/src/components/Button/Button.stories.tsx
+++ superset-frontend/packages/superset-ui-core/src/components/Button/Button.stories.tsx
@@ -128,7 +128,7 @@ InteractiveButton.argTypes = {
styleConfig: {
description:
- 'Optional visual overrides (controlHeight, paddingInline, fontSize, fontWeight, ctaMinWidth, ctaMinHeight, iconGap).',
+ 'Optional visual overrides (controlHeight, paddingInline, fontSize, fontWeight, ctaMinWidth, ctaMinHeight, iconGap, borderRadius).',
control: { type: 'object' },
},
Code Review Run #03fc85
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| tooltip, | ||
| tooltipPlacement, | ||
| children, | ||
| styleConfig, |
There was a problem hiding this comment.
The new styleConfig prop lacks unit test coverage. Per rule [6262], tests should verify actual behavior logic — not just rendering. Add tests validating each styleConfig property (controlHeight, fontSize, fontWeight, boxShadow) and their defaults when the prop is omitted.
Code Review Run #03fc85
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
1 participant
SUMMARY
Make Superset UI components fully configurable via backend theme tokens — enabling deployers to customize appearance without rebuilding the frontend.
Components read optional theme tokens and fall back to existing defaults. Zero visual change unless tokens are explicitly set in
superset_config.py.Components updated:
buttonControlHeight,buttonPaddingInline,buttonFontSize,buttonBorderRadiusfrom theme; addedstyleConfigprop for per-instance overrides;buttonStyleMapdeep merge&&), readslabelBorderRadiusfrom themeselectOptionActiveOutlinetoken controls focus ring behaviorbuttonControlHeight+buttonBorderRadiustokenssizeUnittoken mathHIDE_NAVBAR_LOGOconfig support (opt-in, no-op by default)New optional tokens (in
SupersetSpecificTokens, all with safe fallbacks):buttonControlHeight,buttonControlHeightSM,buttonControlHeightXSbuttonPaddingInline,buttonPaddingInlineSM,buttonFontSize,buttonBorderRadiusselectOptionActiveOutlinelabelBorderRadiusBEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
docker compose up -d— verify default appearance unchangedTHEME_DEFAULTindocker/pythonpath_dev/superset_config.pywith custom tokens — verify buttons, labels, selects, tabs, dashboard tiles all respondHIDE_NAVBAR_LOGO = Truehides the logo in navbarnpx jest --testPathPatterns='Button.test|Select.test|Label.test' --no-coverageADDITIONAL INFORMATION