codeant-ai-for-open-source[bot] commented on code in PR #34629:
URL: https://github.com/apache/superset/pull/34629#discussion_r2599422173
##########
superset-frontend/packages/superset-core/src/ui/theme/Theme.tsx:
##########
@@ -20,12 +20,15 @@
// eslint-disable-next-line no-restricted-syntax
import React from 'react';
import { theme as antdThemeImport, ConfigProvider } from 'antd';
+import stylisRTLPlugin from 'stylis-plugin-rtl';
import {
ThemeProvider,
CacheProvider as EmotionCacheProvider,
} from '@emotion/react';
import createCache from '@emotion/cache';
import { noop, mergeWith } from 'lodash';
+import { DirectionType } from 'antd/es/config-provider';
+import { isThemeDark } from './utils/themeUtils';
Review Comment:
**Suggestion:** Unused import: `isThemeDark` is imported but never used in
the file, which adds dead code and may cause linting/build warnings; remove the
unused import. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The import `isThemeDark` is present in the PR file but not referenced
anywhere. Removing it eliminates dead code and potential lint/build warnings.
It's a safe, small cleanup.
</details>
<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/ui/theme/Theme.tsx
**Line:** 31:31
**Comment:**
*Possible Bug: Unused import: `isThemeDark` is imported but never used
in the file, which adds dead code and may cause linting/build warnings; remove
the unused import.
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.
```
</details>
##########
superset-frontend/packages/superset-core/src/ui/theme/Theme.tsx:
##########
@@ -174,18 +194,23 @@ export class Theme {
const [themeState, setThemeState] = React.useState({
theme: this.theme,
antdConfig: this.antdConfig,
- emotionCache: createCache({ key: 'superset' }),
+ emotionCache: this.createCache(),
});
+ const { direction = 'ltr' } = themeState.theme;
this.updateProviders = (theme, antdConfig, emotionCache) => {
setThemeState({ theme, antdConfig, emotionCache });
+ if (theme.direction === 'rtl') {
+ document?.documentElement?.setAttribute('dir', 'rtl');
+ document?.documentElement?.setAttribute('data-direction', 'rtl');
+ }
};
return (
- <EmotionCacheProvider value={themeState.emotionCache}>
+ <EmotionCacheProvider value={themeState.emotionCache[direction]}>
Review Comment:
**Suggestion:** Potential runtime error: indexing `themeState.emotionCache`
with `direction` may return undefined if `emotionCache` isn't initialized as
expected or `direction` isn't a valid key; guard the access and fallback to the
LTR cache to ensure a valid Emotion cache is always passed to the provider.
[possible bug]
**Severity Level:** Critical 🚨
```suggestion
<EmotionCacheProvider value={themeState.emotionCache?.[direction] ??
themeState.emotionCache?.ltr}>
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Indexing themeState.emotionCache by direction is safe in current code paths,
but a defensive fallback avoids potential runtime undefined access if
emotionCache is ever missing or direction is unexpected. The suggested optional
chaining + fallback is harmless and improves robustness.
</details>
<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/ui/theme/Theme.tsx
**Line:** 210:210
**Comment:**
*Possible Bug: Potential runtime error: indexing
`themeState.emotionCache` with `direction` may return undefined if
`emotionCache` isn't initialized as expected or `direction` isn't a valid key;
guard the access and fallback to the LTR cache to ensure a valid Emotion cache
is always passed to the provider.
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.
```
</details>
##########
superset-frontend/packages/superset-ui-core/src/components/DynamicEditableTitle/index.tsx:
##########
@@ -90,7 +90,8 @@ export const DynamicEditableTitle = memo(
if (sizerRef.current.setSelectionRange) {
const { length } = sizerRef.current.value;
sizerRef.current.setSelectionRange(length, length);
- sizerRef.current.scrollLeft = sizerRef.current.scrollWidth;
+ sizerRef.current.scrollLeft =
+ theme.direction === 'rtl' ? 0 : sizerRef.current.scrollWidth;
}
}
}, [isEditing]);
Review Comment:
**Suggestion:** The added code reads `theme.direction` inside the effect but
the dependency array only includes `[isEditing]`, so changes to text direction
won't retrigger the effect and scrolling/selection behavior can become stale;
include `theme.direction` in the effect dependencies. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
}, [isEditing, theme.direction]);
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Valid — the effect reads theme.direction but only depends on isEditing. If
text direction changes while editing (or theme changes), the effect won't
re-run and scrolling/selection can be stale. Adding theme.direction to the
dependency array is the correct, minimal fix.
</details>
<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/DynamicEditableTitle/index.tsx
**Line:** 97:97
**Comment:**
*Logic Error: The added code reads `theme.direction` inside the effect
but the dependency array only includes `[isEditing]`, so changes to text
direction won't retrigger the effect and scrolling/selection behavior can
become stale; include `theme.direction` in the effect dependencies.
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.
```
</details>
##########
superset-frontend/packages/superset-ui-core/src/components/PageHeaderWithActions/index.tsx:
##########
@@ -134,7 +127,7 @@ export const PageHeaderWithActions = ({
const theme = useTheme();
return (
<div css={headerStyles} className="header-with-actions">
Review Comment:
**Suggestion:** The Emotion `css` prop is being passed a function reference
(`headerStyles`) instead of the computed styles; you must call the style
factory with the resolved `theme` (e.g., `headerStyles(theme)`) so the css prop
receives an actual style object/string. As written the styles will not be
applied and may cause runtime errors in CSS-in-JS resolution. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
<div css={headerStyles(theme)} className="header-with-actions">
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
headerStyles is defined as a theme->css factory (const headerStyles =
(theme: SupersetTheme) => css`...`). The component calls useTheme() and has a
theme value available, so the css prop must receive the computed styles
(headerStyles(theme)). Leaving the function uninvoked means no computed styles
are applied and the layout will be broken — this is a real bug.
</details>
<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:** 129:129
**Comment:**
*Possible Bug: The Emotion `css` prop is being passed a function
reference (`headerStyles`) instead of the computed styles; you must call the
style factory with the resolved `theme` (e.g., `headerStyles(theme)`) so the
css prop receives an actual style object/string. As written the styles will not
be applied and may cause runtime errors in CSS-in-JS resolution.
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.
```
</details>
##########
superset-frontend/packages/superset-ui-core/src/components/PageHeaderWithActions/index.tsx:
##########
@@ -134,7 +127,7 @@ export const PageHeaderWithActions = ({
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}>
Review Comment:
**Suggestion:** The `buttonsStyles` style factory is passed directly to the
`css` prop instead of being invoked with `theme`. Change `css={buttonsStyles}`
to `css={buttonsStyles(theme)}` so the computed styles are applied to the
element. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
<div css={buttonsStyles(theme)}>
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
buttonsStyles is a function (theme => css`...`). It must be invoked with the
theme instance (buttonsStyles(theme)) so the element receives actual style
rules. As-is the styles won't render; fixing it resolves a UI/appearance bug.
</details>
<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:** 133:133
**Comment:**
*Possible Bug: The `buttonsStyles` style factory is passed directly to
the `css` prop instead of being invoked with `theme`. Change
`css={buttonsStyles}` to `css={buttonsStyles(theme)}` so the computed styles
are applied to the element.
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.
```
</details>
##########
superset-frontend/packages/superset-ui-core/src/components/PageHeaderWithActions/index.tsx:
##########
@@ -145,7 +138,7 @@ export const PageHeaderWithActions = ({
{titlePanelAdditionalItems}
</div>
)}
- </div>
+ </Flex>
<div className="right-button-panel">
{rightPanelAdditionalItems}
<div css={additionalActionsContainerStyles}>
Review Comment:
**Suggestion:** The `additionalActionsContainerStyles` style factory is
being passed to the `css` prop without calling it with `theme`; replace
`css={additionalActionsContainerStyles}` with
`css={additionalActionsContainerStyles(theme)}` so styles compute correctly.
[possible bug]
**Severity Level:** Critical 🚨
```suggestion
<div css={additionalActionsContainerStyles(theme)}>
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
additionalActionsContainerStyles is declared as a theme-based factory. Not
invoking it with the theme yields no computed styles and will result in missing
layout spacing. Calling additionalActionsContainerStyles(theme) fixes the issue.
</details>
<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:** 144:144
**Comment:**
*Possible Bug: The `additionalActionsContainerStyles` style factory is
being passed to the `css` prop without calling it with `theme`; replace
`css={additionalActionsContainerStyles}` with
`css={additionalActionsContainerStyles(theme)}` so styles compute correctly.
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.
```
</details>
##########
superset-frontend/packages/superset-core/src/ui/theme/Theme.tsx:
##########
@@ -174,18 +194,23 @@ export class Theme {
const [themeState, setThemeState] = React.useState({
theme: this.theme,
antdConfig: this.antdConfig,
- emotionCache: createCache({ key: 'superset' }),
+ emotionCache: this.createCache(),
});
+ const { direction = 'ltr' } = themeState.theme;
this.updateProviders = (theme, antdConfig, emotionCache) => {
setThemeState({ theme, antdConfig, emotionCache });
+ if (theme.direction === 'rtl') {
+ document?.documentElement?.setAttribute('dir', 'rtl');
Review Comment:
**Suggestion:** Logic bug when changing text direction: only sets document
attributes when switching to RTL and does not update/remove them when switching
back to LTR, leaving the document in the wrong direction; always set the
attributes to the current direction (with a safe check for non-browser
environments). [logic error]
**Severity Level:** Minor ⚠️
```suggestion
const dir = (theme && (theme as any).direction) ?? 'ltr';
// Ensure we update the document direction attribute on every change,
// and safely guard for non-browser environments.
if (typeof document !== 'undefined' && document.documentElement) {
document.documentElement.setAttribute('dir', dir);
document.documentElement.setAttribute('data-direction', dir);
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The current code only sets document attributes when switching to RTL and
never resets them when switching back to LTR, which can leave the document in
the wrong direction. The proposed change correctly ensures the document
attributes are set on every update and guards for non-browser environments.
This fixes a real logic bug.
</details>
<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/ui/theme/Theme.tsx
**Line:** 203:204
**Comment:**
*Logic Error: Logic bug when changing text direction: only sets
document attributes when switching to RTL and does not update/remove them when
switching back to LTR, leaving the document in the wrong direction; always set
the attributes to the current direction (with a safe check for non-browser
environments).
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.
```
</details>
##########
superset-frontend/packages/superset-ui-core/src/components/DynamicEditableTitle/index.tsx:
##########
@@ -90,7 +90,8 @@ export const DynamicEditableTitle = memo(
if (sizerRef.current.setSelectionRange) {
const { length } = sizerRef.current.value;
sizerRef.current.setSelectionRange(length, length);
- sizerRef.current.scrollLeft = sizerRef.current.scrollWidth;
+ sizerRef.current.scrollLeft =
+ theme.direction === 'rtl' ? 0 : sizerRef.current.scrollWidth;
Review Comment:
**Suggestion:** The effect assumes `sizerRef.current` is an input/textarea
and calls selection/scroll methods on it, but `sizerRef` is attached to the
sizing <span>, not the editable <input>. That will either never run (because
`setSelectionRange` is undefined) or, if refs change, throw a runtime
TypeError. Query the real input inside the container (e.g. via
`containerRef.current.querySelector('input,textarea')`) and operate on that
element instead. [type error]
**Severity Level:** Minor ⚠️
```suggestion
{
const inputElement =
containerRef.current?.querySelector<HTMLInputElement |
HTMLTextAreaElement>('input, textarea');
if (inputElement && typeof inputElement.setSelectionRange ===
'function') {
const length = inputElement.value.length;
inputElement.setSelectionRange(length, length);
const scrollTo = Math.max(0, inputElement.scrollWidth -
inputElement.clientWidth);
inputElement.scrollLeft = scrollTo;
}
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Correct — sizerRef is attached to the offscreen <span>, not the input. The
current code will usually be a no-op (span has no setSelectionRange) and the
intended behavior (move cursor and scroll the real input) won't run. Querying
the actual input/textarea via containerRef and operating on that element fixes
a real functional bug and prevents potential runtime errors.
</details>
<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/DynamicEditableTitle/index.tsx
**Line:** 90:94
**Comment:**
*Type Error: The effect assumes `sizerRef.current` is an input/textarea
and calls selection/scroll methods on it, but `sizerRef` is attached to the
sizing <span>, not the editable <input>. That will either never run (because
`setSelectionRange` is undefined) or, if refs change, throw a runtime
TypeError. Query the real input inside the container (e.g. via
`containerRef.current.querySelector('input,textarea')`) and operate on that
element instead.
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.
```
</details>
##########
superset-frontend/packages/superset-core/src/ui/theme/types.ts:
##########
@@ -24,6 +24,7 @@ import { theme as antdThemeImport } from 'antd';
* Get AntdThemeConfig type from the theme object
*/
import type { ThemeConfig } from 'antd';
+import { DirectionType } from 'antd/es/config-provider';
Review Comment:
**Suggestion:** The file imports `DirectionType` as a runtime value import
which will emit a real JS import even though `DirectionType` is only used as a
type; this can introduce unnecessary runtime side-effects, larger bundles, or
issues in SSR/builds — change it to a type-only import so TypeScript erases it
from emitted JS. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
import type { DirectionType } from 'antd/es/config-provider';
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
DirectionType in this file is used only in type positions
(SupersetSpecificTokens.direction and ThemeContextType.setDirection).
Using `import type` will erase the import from emitted JS, avoiding a
runtime import/side-effect during SSR or bundling. This is a real, low-risk
improvement.
</details>
<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/ui/theme/types.ts
**Line:** 27:27
**Comment:**
*Possible Bug: The file imports `DirectionType` as a runtime value
import which will emit a real JS import even though `DirectionType` is only
used as a type; this can introduce unnecessary runtime side-effects, larger
bundles, or issues in SSR/builds — change it to a type-only import so
TypeScript erases it from emitted JS.
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.
```
</details>
##########
superset-frontend/packages/superset-ui-core/src/components/ListViewCard/index.tsx:
##########
@@ -95,10 +95,9 @@ const TitleLink = styled.span`
const TitleRight = styled.span`
${({ theme }) => css`
position: absolute;
- right: -1px;
font-weight: 400;
bottom: ${theme.sizeUnit * 3}px;
- right: ${theme.sizeUnit * 2}px;
+ inset-inline-end: ${theme.sizeUnit * 2}px;
Review Comment:
**Suggestion:** Relying solely on `inset-inline-end` may not produce
consistent results across themes with different text directions; use the
theme's direction to emit an explicit physical property (`left` or `right`) so
non-supporting environments and CSS fallbacks line up with the app's RTL/LTR
setting. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
/* emit a physical fallback matching the theme direction for
deterministic positioning */
${theme.direction === 'rtl'
? `left: ${theme.sizeUnit * 2}px;`
: `right: ${theme.sizeUnit * 2}px;`}
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
This is the correct, safer recommendation: emit a physical fallback that
respects the theme's text direction so you don't break RTL layouts. The
improved code is syntactically valid within styled-components and prevents
regressions on browsers that lack logical-property support while preserving RTL
correctness.
</details>
<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/ListViewCard/index.tsx
**Line:** 100:100
**Comment:**
*Logic Error: Relying solely on `inset-inline-end` may not produce
consistent results across themes with different text directions; use the
theme's direction to emit an explicit physical property (`left` or `right`) so
non-supporting environments and CSS fallbacks line up with the app's RTL/LTR
setting.
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.
```
</details>
--
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]
