codeant-ai-for-open-source[bot] commented on code in PR #34629:
URL: https://github.com/apache/superset/pull/34629#discussion_r3479713551
##########
superset-frontend/packages/superset-core/src/theme/Theme.tsx:
##########
@@ -211,18 +235,26 @@ export class Theme {
const [themeState, setThemeState] = React.useState({
theme: this.theme,
antdConfig: this.antdConfig,
- emotionCache: createCache({ key: 'superset' }),
+ emotionCache: this.emotionCaches,
});
+ const { direction = 'ltr' } = themeState.theme;
Review Comment:
**Suggestion:** Defaulting to `'ltr'` when `themeState.theme.direction` is
missing makes newly created `Theme` instances (like dashboard-scoped themes
created from config only) render in LTR even when the app locale is RTL. This
breaks RTL consistency across providers. Propagate the current global direction
into new theme instances (or into serialized/applied config) so provider-level
direction does not silently fall back to LTR. [incomplete implementation]
<details>
<summary><b>Severity Level:</b> Critical ๐จ</summary>
```mdx
- โ DashboardPage via CrudThemeProvider renders LTR in RTL locale.
- โ ๏ธ Nested dashboard theme ignores global RTL direction setting.
```
</details>
<details>
<summary><b>Steps of Reproduction โ
</b></summary>
```mdx
1. Configure an RTL locale (e.g. Persian `fa`) so `getBootstrapLocale()`
returns an RTL
language code; `src/utils/localeUtils.ts:28-31` maps RTL languages from
`rtlLanguages` in
`src/constants.ts:86-97` to `TextDirection 'rtl'`.
2. Start the app with this RTL locale; in
`src/theme/ThemeController.ts:117-182`, the
constructor applies the initial theme via `this.applyTheme(initialTheme)`
and then calls
`this.initializeDirectionFromLocale()` at line 180, which sets the global
theme direction
using
`this.globalTheme.setDirection(getDirectionFromLocale(getBootstrapLocale()))` at
lines 25-28 (700-729 region).
3. Open a dashboard that has a CRUD theme configured;
`src/dashboard/containers/DashboardPage.tsx:23-35` wraps the dashboard
content in
`<CrudThemeProvider theme={reduxTheme !== undefined ? reduxTheme :
dashboard?.theme}>`.
Inside `src/components/CrudThemeProvider.tsx:52-74`,
`Theme.fromConfig(normalizedConfig,
baseTheme || undefined)` creates a new `dashboardTheme` from JSON config and
bootstrap
base theme, but neither supplies a `direction` token.
4. `CrudThemeProvider` renders `<dashboardTheme.SupersetThemeProvider>` at
`src/components/CrudThemeProvider.tsx:103-107`. In that provider
implementation
(`superset-frontend/packages/superset-core/src/theme/Theme.tsx:229-260`),
`const {
direction = 'ltr' } = themeState.theme;` at line 240 and the subsequent
`<EmotionCacheProvider value={themeState.emotionCache?.[direction] ??
themeState.emotionCache?.ltr}>` and `<ConfigProvider
theme={themeState.antdConfig}
direction={direction}>` at lines 254-257 cause the nested dashboard theme to
use LTR cache
and AntD direction. Because `dashboardTheme.theme.direction` is undefined,
scoped
dashboard content (header, controls, charts) renders LTR even though the
global theme and
document `<html dir>` have been initialized to RTL by `ThemeController`,
breaking RTL
consistency between global and dashboard-scoped providers.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9608ab219f094a16845952467e2418e4&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=9608ab219f094a16845952467e2418e4&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-core/src/theme/Theme.tsx
**Line:** 240:240
**Comment:**
*Incomplete Implementation: Defaulting to `'ltr'` when
`themeState.theme.direction` is missing makes newly created `Theme` instances
(like dashboard-scoped themes created from config only) render in LTR even when
the app locale is RTL. This breaks RTL consistency across providers. Propagate
the current global direction into new theme instances (or into
serialized/applied config) so provider-level direction does not silently fall
back to LTR.
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%2F34629&comment_hash=0777cda17cc7cff2e8d91d9bb81f84e8104cb2d9067dbdb410416274e7848df3&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F34629&comment_hash=0777cda17cc7cff2e8d91d9bb81f84e8104cb2d9067dbdb410416274e7848df3&reaction=dislike'>๐</a>
##########
superset-frontend/packages/superset-ui-core/src/components/PageHeaderWithActions/index.tsx:
##########
@@ -135,21 +128,21 @@ export const PageHeaderWithActions = memo(
const theme = useTheme();
return (
<div css={headerStyles} className="header-with-actions">
- <div className="title-panel">
+ <Flex align="center" gap={theme.sizeUnit * 12}>
<DynamicEditableTitle {...editableTitleProps} />
{showTitlePanelItems && (
- <div css={buttonsStyles}>
+ <div css={buttonsStyles(theme)}>
{certificatiedBadgeProps?.certifiedBy && (
<CertifiedBadge {...certificatiedBadgeProps} />
)}
{showFaveStar && <FaveStar {...faveStarProps} />}
{titlePanelAdditionalItems}
</div>
)}
- </div>
+ </Flex>
<div className="right-button-panel">
{rightPanelAdditionalItems}
- <div css={additionalActionsContainerStyles}>
+ <div css={additionalActionsContainerStyles(theme)}>
Review Comment:
**Suggestion:** The new container with `additionalActionsContainerStyles` is
always rendered, so when `showMenuDropdown` is false it still contributes left
margin and shifts layout spacing despite having no content. Render this wrapper
only when the dropdown is shown (or conditionally remove the margin) to avoid
unintended header alignment gaps. [css layout issue]
<details>
<summary><b>Severity Level:</b> Minor ๐งน</summary>
```mdx
- โ ๏ธ AllEntities header shows extra gap when actions hidden.
- โ ๏ธ Right panel alignment inconsistent across header variants.
```
</details>
<details>
<summary><b>Steps of Reproduction โ
</b></summary>
```mdx
1. Navigate to the All Entities page; its header uses
`PageHeaderWithActions` in
`src/pages/AllEntities/index.tsx:8-38`, passing `showMenuDropdown={false}`
at line 37 and
`rightPanelAdditionalItems` containing an "Edit tag" button.
2. In
`superset-frontend/packages/superset-ui-core/src/components/PageHeaderWithActions/index.tsx:114-172`,
the component renders a right-hand panel: `<div
className="right-button-panel">` at line
143 with `{rightPanelAdditionalItems}` followed by `<div
css={additionalActionsContainerStyles(theme)}>` at line 145, where
`additionalActionsContainerStyles` adds `margin-left: ${theme.sizeUnit *
2}px` (lines
93-95).
3. Inside this container, `{showMenuDropdown && (<Dropdown ...>)}` at lines
146-167
ensures the dropdown trigger is omitted when `showMenuDropdown={false}`, but
the wrapper
`<div css={additionalActionsContainerStyles(theme)}>` remains rendered and
contributes its
left margin even though it has no children.
4. As a result, whenever `PageHeaderWithActions` is used with
`showMenuDropdown={false}`
(verified in `src/pages/AllEntities/index.tsx:34-37`), the All Entities
header right panel
displays an extra horizontal gap to the right of the "Edit tag" button from
`rightPanelAdditionalItems`, caused solely by the always-present empty
container margin.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=101abde26fba49a89e8b72b97d40bf1d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=101abde26fba49a89e8b72b97d40bf1d&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/PageHeaderWithActions/index.tsx
**Line:** 145:145
**Comment:**
*Css Layout Issue: The new container with
`additionalActionsContainerStyles` is always rendered, so when
`showMenuDropdown` is false it still contributes left margin and shifts layout
spacing despite having no content. Render this wrapper only when the dropdown
is shown (or conditionally remove the margin) to avoid unintended header
alignment gaps.
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%2F34629&comment_hash=0a02c7637f20f52685b6562a18db6850733758385a22dc8db1f71a40ec0345e8&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F34629&comment_hash=0a02c7637f20f52685b6562a18db6850733758385a22dc8db1f71a40ec0345e8&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]
