codeant-ai-for-open-source[bot] commented on code in PR #34629:
URL: https://github.com/apache/superset/pull/34629#discussion_r3482820939
##########
superset-frontend/src/components/CrudThemeProvider.tsx:
##########
@@ -68,14 +68,18 @@ export default function CrudThemeProvider({
normalizedConfig,
baseTheme || undefined,
);
+ const parentDirection = themeContext?.theme.theme.direction;
+ if (parentDirection) {
+ createdTheme.setDirection(parentDirection);
+ }
const rawUrls = themeConfig?.token?.fontUrls;
const urls = Array.isArray(rawUrls) ? (rawUrls as string[]) : undefined;
return { dashboardTheme: createdTheme, fontUrls: urls };
} catch (error) {
logging.warn('Failed to load dashboard theme:', error);
return { dashboardTheme: null, fontUrls: undefined };
}
- }, [theme?.json_data, hasThemeConfigOverride]);
+ }, [theme?.json_data, hasThemeConfigOverride, themeContext?.theme]);
Review Comment:
**Suggestion:** The memoized dashboard theme does not re-compute when only
text direction changes, because the dependency tracks `themeContext?.theme`
object identity instead of the nested direction value. `setDirection` mutates
the existing theme instance, so this keeps a stale LTR/RTL value in the nested
dashboard theme until some unrelated prop changes. Add the actual direction
field to the memo dependencies (or derive direction from `useTheme`) so
dashboard theming updates immediately on language direction changes. [stale
reference]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Dashboard theme direction stays LTR after global RTL switch.
- ⚠️ RTL users see inconsistent layout between chrome and dashboards.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. The app bootstraps `ThemeController` in
`src/views/RootContextProviders.tsx:34` and
wraps the React tree in `SupersetThemeProvider` from
`src/theme/ThemeProvider.tsx:43`,
exposing `ThemeContext` with `theme` and `setDirection` (lines 87–90,
145–163).
2. A dashboard page mounts `CrudThemeProvider` around `DashboardContainer` in
`src/dashboard/containers/DashboardPage.tsx:23–35`, so `CrudThemeProvider`
reads
`themeContext` via `ThemeContext` and computes `dashboardTheme` in its
`useMemo` (lines
13–43 in `CrudThemeProvider.tsx`).
3. At runtime, some caller (e.g. a future integration in `RightMenu.tsx`
using
`useThemeContext()` from `ThemeProvider.tsx:196`) calls
`setDirection('rtl')` without
changing `theme?.json_data` or `hasThemeConfigOverride`, triggering
`ThemeController.setDirection` in `src/theme/ThemeController.ts:391–394`.
4. `ThemeController.setDirection` mutates `globalTheme` in place
(`this.globalTheme.setDirection(direction)`) and calls `notifyListeners()`,
so
`SupersetThemeProvider`'s `updateState` sets `currentTheme` to the same
`Theme` instance
(object identity unchanged). Because `CrudThemeProvider`'s `useMemo` depends
on
`themeContext?.theme` (line 43), which does not change identity when only
`theme.theme.direction` is mutated, the memo does not recompute and
`createdTheme.setDirection(parentDirection)` (lines 71–74) is never
re-applied. The nested
`dashboardTheme` therefore keeps a stale LTR/RTL direction until
`theme?.json_data` or
`hasThemeConfigOverride` changes.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=dcf8f5aad77b40fbb8f4a14e64729848&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=dcf8f5aad77b40fbb8f4a14e64729848&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/components/CrudThemeProvider.tsx
**Line:** 71:82
**Comment:**
*Stale Reference: The memoized dashboard theme does not re-compute when
only text direction changes, because the dependency tracks
`themeContext?.theme` object identity instead of the nested direction value.
`setDirection` mutates the existing theme instance, so this keeps a stale
LTR/RTL value in the nested dashboard theme until some unrelated prop changes.
Add the actual direction field to the memo dependencies (or derive direction
from `useTheme`) so dashboard theming updates immediately on language direction
changes.
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=bd7c7d7806c48b19af2a77f5bf011cee53dd5eed441c461ed53c5cc57daa148e&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F34629&comment_hash=bd7c7d7806c48b19af2a77f5bf011cee53dd5eed441c461ed53c5cc57daa148e&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]
