codeant-ai-for-open-source[bot] commented on code in PR #37790:
URL: https://github.com/apache/superset/pull/37790#discussion_r2779126405
##########
superset-frontend/src/dashboard/components/PropertiesModal/index.tsx:
##########
@@ -672,6 +747,34 @@ const PropertiesModal = ({
<BasicInfoSection
form={form}
validationStatus={validationStatus}
+ activeLocale={
+ isFeatureEnabled(FeatureFlag.EnableContentLocalization)
+ ? titleActiveLocale
+ : undefined
+ }
+ translationValue={
+ titleActiveLocale !== DEFAULT_LOCALE_KEY
+ ? translations.dashboard_title?.[titleActiveLocale] ?? ''
+ : undefined
+ }
+ onTranslationChange={handleTitleTranslationChange}
+ localeSwitcher={
+ isFeatureEnabled(
+ FeatureFlag.EnableContentLocalization,
+ ) ? (
+ <LocaleSwitcher
+ fieldName="dashboard_title"
+ defaultValue={dashboardInfo?.title ?? ''}
Review Comment:
**Suggestion:** The locale switcher is given
`defaultValue={dashboardInfo?.title ?? ''}`, which becomes stale if the user
edits the dashboard title in the form before opening the translation editor;
this causes the translation UI to show an outdated "default" text. It should
instead use the current form field value (falling back to the loaded title) so
the translation editor always reflects the latest default title. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Dashboard title translation editor shows outdated default string.
- ⚠️ Translators may create translations for obsolete dashboard names.
- ⚠️ Minor UX inconsistency in dashboard properties modal.
```
</details>
```suggestion
defaultValue={
form.getFieldValue('title') ??
dashboardInfo?.title ?? ''
}
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Ensure the `EnableContentLocalization` feature flag is enabled so the
locale switcher
is rendered in `PropertiesModal`
(superset-frontend/src/dashboard/components/PropertiesModal/index.tsx:747-777).
2. Open any dashboard and launch the Dashboard Properties modal, which
renders
`PropertiesModal` with `show=true` and initial `dashboardInfo.title` loaded
via
`handleDashboardData` (same file: lines ~184-212, 225-229, 552-585).
3. In the "General information" section rendered by `BasicInfoSection`
(lines 747-778),
edit the dashboard name field bound to the Ant Design `Form` instance
(`form` created at
lines 108-112); this updates the form's `'title'` field but does not update
`dashboardInfo.title`, which is only set in `handleDashboardData`.
4. While the modal is still open and unsaved, open the translation UI for
the dashboard
title via the embedded `<LocaleSwitcher>` passed through the
`localeSwitcher` prop (lines
761-777).
5. Observe that `LocaleSwitcher` receives
`defaultValue={dashboardInfo?.title ?? ''}`
(line 767), which still contains the original title from load time rather
than the updated
`form` value, so any "default text" shown in the translation editor is stale
compared to
the visible title field.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/dashboard/components/PropertiesModal/index.tsx
**Line:** 767:767
**Comment:**
*Logic Error: The locale switcher is given
`defaultValue={dashboardInfo?.title ?? ''}`, which becomes stale if the user
edits the dashboard title in the form before opening the translation editor;
this causes the translation UI to show an outdated "default" text. It should
instead use the current form field value (falling back to the loaded title) so
the translation editor always reflects the latest default title.
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>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37790&comment_hash=46ca29f41fa05f0b33003d8a67fde862dd9cf90aec1b9babbe79b724a1dd8b22&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37790&comment_hash=46ca29f41fa05f0b33003d8a67fde862dd9cf90aec1b9babbe79b724a1dd8b22&reaction=dislike'>👎</a>
##########
superset-frontend/src/dashboard/components/PropertiesModal/sections/BasicInfoSection.tsx:
##########
@@ -58,8 +82,21 @@ const BasicInfoSection = ({
placeholder={t('The display name of your dashboard')}
data-test="dashboard-title-input"
type="text"
+ suffix={!isEditingTranslation ? localeSwitcher : undefined}
/>
</FormItem>
+ {isEditingTranslation && (
+ <Input
+ value={translationValue}
Review Comment:
**Suggestion:** The translation input is controlled via the optional
`translationValue` prop, and passing `undefined` as the `value` can cause React
to treat the field as uncontrolled initially and then controlled after updates,
resulting in warnings and potentially inconsistent input behavior; defaulting
to an empty string avoids this. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- WARNING React logs uncontrolled-to-controlled warning during translation
editing.
- WARNING Dev console noise affects debugging dashboard localization flows.
```
</details>
```suggestion
value={translationValue ?? ''}
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. A parent component renders `BasicInfoSection` from
`superset-frontend/src/dashboard/components/PropertiesModal/sections/BasicInfoSection.tsx`
with `activeLocale` set to a non-default locale (so `isEditingTranslation`
at lines 52–53
evaluates to `true`) and omits an initial translation value, leaving
`translationValue` as
`undefined`.
2. Because `isEditingTranslation === true`, the translation `<Input>` at
lines 88–98 is
rendered with `value={translationValue}` at line 90. With `translationValue`
undefined,
React treats this as an uncontrolled input on initial render.
3. When the user types into the translation field, the `onChange` handler at
lines 91–93
calls `onTranslationChange?.(e.target.value)`, and the parent updates its
state to a
non-empty string, passing a defined `translationValue` back into
`BasicInfoSection` on the
next render.
4. On this subsequent render, the same `<Input>` now has a defined `value`
prop, so React
switches the field from uncontrolled to controlled and logs a console
warning about
changing an uncontrolled input to controlled during dashboard title
localization flows.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/dashboard/components/PropertiesModal/sections/BasicInfoSection.tsx
**Line:** 90:90
**Comment:**
*Possible Bug: The translation input is controlled via the optional
`translationValue` prop, and passing `undefined` as the `value` can cause React
to treat the field as uncontrolled initially and then controlled after updates,
resulting in warnings and potentially inconsistent input behavior; defaulting
to an empty string avoids this.
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>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37790&comment_hash=1a7dedc7ded328ba487acec8a4425ddbfc18353131629ac4bb6a2d341416da9d&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37790&comment_hash=1a7dedc7ded328ba487acec8a4425ddbfc18353131629ac4bb6a2d341416da9d&reaction=dislike'>👎</a>
##########
superset-frontend/src/explore/components/PropertiesModal/PropertiesModal.test.tsx:
##########
@@ -471,3 +493,74 @@ test('Should display only custom tags when tagging system
is enabled', async ()
mockIsFeatureEnabled.mockRestore();
});
+
+test('locale switcher hidden when content localization flag is off', async ()
=> {
+ const props = createProps();
+ renderModal(props);
+ await waitFor(() => {
+ expect(screen.getByRole('dialog')).toBeInTheDocument();
+ });
+ expect(
+ screen.queryByRole('button', { name: /Locale switcher for/i }),
+ ).not.toBeInTheDocument();
+});
+
+test('locale switcher visible when content localization flag is on', async ()
=> {
+ mockedIsFeatureEnabled.mockImplementation(
+ flag => flag === FeatureFlag.EnableContentLocalization,
+ );
+ const props = createProps();
+ renderModal(props);
+ await waitFor(() => {
+ expect(
+ screen.getByRole('button', {
+ name: /Locale switcher for Chart Name/i,
+ }),
+ ).toBeInTheDocument();
+ });
+ mockedIsFeatureEnabled.mockRestore();
Review Comment:
**Suggestion:** `mockedIsFeatureEnabled` is defined as a plain `jest.fn()`
via `jest.mock`, so calling `mockRestore()` on it will throw at runtime because
`mockRestore` only exists on spies created with `jest.spyOn`; use `mockReset()`
to clear the implementation between tests instead. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Locale switcher visibility test fails with Jest runtime error.
- ⚠️ Frontend test suite may fail, blocking PR verification.
```
</details>
```suggestion
mockedIsFeatureEnabled.mockReset();
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run the frontend Jest tests, including
`superset-frontend/src/explore/components/PropertiesModal/PropertiesModal.test.tsx`.
2. In this file, `isFeatureEnabled` from `@superset-ui/core` is mocked at
the top with
`jest.fn()` via `jest.mock('@superset-ui/core', ...)` (lines 30–33 in the PR
hunk), and
`mockedIsFeatureEnabled` is just a typed alias of this mock (lines 35–37).
3. The test `locale switcher visible when content localization flag is on`
(introduced
around line 508 in the PR hunk) calls
`mockedIsFeatureEnabled.mockImplementation(...)`
and, at the end of the test, executes
`mockedIsFeatureEnabled.mockRestore();` at line 521.
4. At runtime, Jest mock functions created with `jest.fn()` do not support
`mockRestore`
(that method only works for spies created via `jest.spyOn`), so calling
`mockedIsFeatureEnabled.mockRestore()` throws an error (e.g. "mockRestore is
not a
function"), causing this test – and the entire `PropertiesModal.test.tsx`
suite – to fail
before completing. Note: there is a pre‑existing identical misuse in the
"tagging system"
test earlier in the same file, so fixing only this line is necessary but not
sufficient to
make the suite pass.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/explore/components/PropertiesModal/PropertiesModal.test.tsx
**Line:** 521:521
**Comment:**
*Logic Error: `mockedIsFeatureEnabled` is defined as a plain
`jest.fn()` via `jest.mock`, so calling `mockRestore()` on it will throw at
runtime because `mockRestore` only exists on spies created with `jest.spyOn`;
use `mockReset()` to clear the implementation between tests 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>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37790&comment_hash=d783300d11c9c2f152e5df4cb42aa55dc4fcc64959cf647db5f3246985513e50&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37790&comment_hash=d783300d11c9c2f152e5df4cb42aa55dc4fcc64959cf647db5f3246985513e50&reaction=dislike'>👎</a>
##########
superset-frontend/src/dashboard/components/PropertiesModal/sections/BasicInfoSection.tsx:
##########
@@ -58,8 +82,21 @@ const BasicInfoSection = ({
placeholder={t('The display name of your dashboard')}
data-test="dashboard-title-input"
type="text"
+ suffix={!isEditingTranslation ? localeSwitcher : undefined}
/>
</FormItem>
+ {isEditingTranslation && (
+ <Input
+ value={translationValue}
+ onChange={(e: ChangeEvent<HTMLInputElement>) =>
+ onTranslationChange?.(e.target.value)
+ }
+ placeholder={t('Translation for %s', activeLocale.toUpperCase())}
+ data-test="dashboard-title-input"
Review Comment:
**Suggestion:** Both the default title input and the translation input share
the same `data-test` attribute, which can cause automated tests to interact
with the wrong field when both exist in the DOM (e.g., in some testing setups),
making tests flaky and assertions ambiguous; giving the translation input a
distinct test id avoids this collision. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- WARNING Automated tests may target hidden dashboard title input.
- WARNING Locale switching E2E tests can become flaky or ambiguous.
```
</details>
```suggestion
data-test="dashboard-title-translation-input"
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Enable the content localization feature flag so the `localeSwitcher`
suffix is passed
to `BasicInfoSection` (file
`superset-frontend/src/dashboard/components/PropertiesModal/sections/BasicInfoSection.tsx`)
and a non-default `activeLocale` is used, making `isEditingTranslation` at
lines 52–53
evaluate to `true`.
2. In this state, the default title `<Input>` inside the `<FormItem
name="title">` at
lines 69–86 remains mounted but hidden, with
`data-test="dashboard-title-input"` at line
83, while the translation `<Input>` at lines 88–98 is also rendered with the
same
`data-test="dashboard-title-input"` at line 95.
3. A Playwright or Cypress test for the dashboard properties modal that
selects
`[data-test="dashboard-title-input"]` during a non-default locale scenario
(as described
in the PR's E2E test plan) may resolve the hidden default input instead of
the visible
translation input.
4. As a result, automated tests attempting to type or assert against the
dashboard title
in a translated locale can become flaky or interact with the wrong input
element whenever
both inputs exist in the DOM with the same test identifier.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/dashboard/components/PropertiesModal/sections/BasicInfoSection.tsx
**Line:** 95:95
**Comment:**
*Possible Bug: Both the default title input and the translation input
share the same `data-test` attribute, which can cause automated tests to
interact with the wrong field when both exist in the DOM (e.g., in some testing
setups), making tests flaky and assertions ambiguous; giving the translation
input a distinct test id avoids this collision.
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>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37790&comment_hash=967548fe6233f0f6835a70624581c553cde7f9ab67fc475ec212a8ae8ef9ffff&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37790&comment_hash=967548fe6233f0f6835a70624581c553cde7f9ab67fc475ec212a8ae8ef9ffff&reaction=dislike'>👎</a>
##########
superset-frontend/src/components/TranslationEditor/LocaleSwitcher.tsx:
##########
@@ -0,0 +1,216 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { useMemo } from 'react';
+import type { MenuProps } from 'antd';
+import { t } from '@apache-superset/core';
+import { css, useTheme } from '@apache-superset/core/ui';
+import {
+ Badge,
+ Dropdown,
+ Icons,
+} from '@superset-ui/core/components';
+import type { Translations, LocaleInfo } from 'src/types/Localization';
+import { DEFAULT_LOCALE_KEY, countFieldTranslations } from './utils';
+
+export interface LocaleSwitcherProps {
+ /** Translation field key (e.g., 'dashboard_title', 'slice_name'). */
+ fieldName: string;
+ /** Current default (column) value — used for ✓ indicator on DEFAULT. */
+ defaultValue: string;
+ /** Full translations object for the entity. */
+ translations: Translations;
+ /** All configured locales from server (including default locale). */
+ allLocales: LocaleInfo[];
+ /** Server's default locale code (e.g., 'en'). */
+ defaultLocale: string;
+ /** Current user's UI locale code from session. */
+ userLocale: string;
+ /** Currently active locale. Controlled by parent. */
+ activeLocale: string;
+ /** Called when user picks a locale from the dropdown. */
+ onLocaleChange: (locale: string) => void;
+ /** Human-readable field label for accessibility (e.g., 'Dashboard Title').
*/
+ fieldLabel: string;
+}
+
+/**
+ * Inline locale dropdown rendered as an Input suffix.
+ *
+ * Displays the currently active locale (DEFAULT or a specific language)
+ * with a badge showing translation count. Click opens a dropdown to
+ * switch between DEFAULT text and per-locale translations.
+ *
+ * Yellow highlight indicates the user's locale has no translation.
+ */
+export default function LocaleSwitcher({
+ fieldName,
+ defaultValue,
+ translations,
+ allLocales,
+ defaultLocale,
+ userLocale,
+ activeLocale,
+ onLocaleChange,
+ fieldLabel,
+}: LocaleSwitcherProps) {
+ const theme = useTheme();
+
+ const fieldTranslations = translations[fieldName] ?? {};
+ const translationCount = countFieldTranslations(translations, fieldName);
+
+ const userLocaleHasTranslation =
+ userLocale === defaultLocale || Boolean(fieldTranslations[userLocale]);
+ const isWarning = !userLocaleHasTranslation;
+
+ const localeMap = useMemo(
+ () => new Map(allLocales.map(loc => [loc.code, loc])),
+ [allLocales],
+ );
+
+ const sortedLocales = useMemo(
+ () => [...allLocales].sort((a, b) => a.code.localeCompare(b.code)),
+ [allLocales],
+ );
+
+ const isDefault = activeLocale === DEFAULT_LOCALE_KEY;
+ const activeLocaleInfo = isDefault ? undefined : localeMap.get(activeLocale);
+ const triggerFlag = activeLocaleInfo?.flag;
+
+ const menuItems: MenuProps['items'] = useMemo(() => {
+ const checkIcon = (
+ <Icons.CheckOutlined
+ iconSize="s"
+ css={css`
+ font-size: 12px;
+ `}
+ />
+ );
+ const noIcon = (
+ <span
+ css={css`
+ display: inline-block;
+ width: 14px;
+ `}
+ />
+ );
+
+ return [
+ {
+ key: DEFAULT_LOCALE_KEY,
+ icon: defaultValue ? checkIcon : noIcon,
+ label: (
+ <span
+ css={css`
+ font-weight: ${activeLocale === DEFAULT_LOCALE_KEY ? 600 : 400};
+ `}
+ >
+ {t('DEFAULT')}
+ </span>
+ ),
+ },
+ { type: 'divider' as const },
+ ...sortedLocales.map(locale => ({
+ key: locale.code,
+ icon: fieldTranslations[locale.code] ? checkIcon : noIcon,
+ label: (
+ <span
+ css={css`
+ font-weight: ${activeLocale === locale.code ? 600 : 400};
+ `}
+ >
+ {locale.flag ? `${locale.flag} ` : ''}
+ {locale.name}
+ </span>
+ ),
+ })),
+ ];
+ }, [
+ sortedLocales,
+ fieldTranslations,
+ defaultValue,
+ activeLocale,
+ ]);
+
+ const handleMenuClick: MenuProps['onClick'] = ({ key }) => {
+ onLocaleChange(key);
+ };
+
+ const suffixColor = isWarning ? theme.colorWarning : theme.colorText;
+
+ return (
+ <Dropdown
+ menu={{ items: menuItems, onClick: handleMenuClick }}
+ trigger={['click']}
+ getPopupContainer={() => document.body}
+ >
+ <span
+ role="button"
+ tabIndex={0}
+ aria-label={t(
+ 'Locale switcher for %s: %s (%s translations)',
+ fieldLabel,
+ isDefault ? t('DEFAULT') : activeLocale,
+ translationCount,
+ )}
+ onClick={e => e.stopPropagation()}
+ onKeyDown={e => {
+ if (e.key === 'Enter' || e.key === ' ') {
+ e.stopPropagation();
+ e.preventDefault();
+ }
Review Comment:
**Suggestion:** The suffix element is focusable and has `role="button"`, but
its keydown handler for Enter/Space only stops propagation and prevents default
without triggering the dropdown, making it impossible to open the locale menu
via keyboard; programmatically invoking a click in this handler restores
keyboard accessibility. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Keyboard users cannot open locale switcher dropdown control.
- ⚠️ Affects translation editing for dashboards, charts, and filters.
```
</details>
```suggestion
(e.currentTarget as HTMLElement).click();
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Render the `LocaleSwitcher` component from
`superset-frontend/src/components/TranslationEditor/LocaleSwitcher.tsx` as
used in the
translation editor, and focus the suffix `<span>` that has `role="button"`
and
`tabIndex={0}` at lines 161-187 by tabbing to it.
2. Confirm the surrounding `Dropdown` from `@superset-ui/core/components` is
configured
with `trigger={['click']}` at lines 157-160, meaning it opens only on click
events.
3. While the suffix `<span>` is focused, press the Enter key (or Space); the
`onKeyDown`
handler at lines 172-176 intercepts the event, calling `e.stopPropagation()`
and
`e.preventDefault()` without performing any additional action.
4. Because the default browser action for Enter/Space (synthesizing a click
on the focused
button-like element) is prevented, no click event is emitted, so the
`Dropdown` never
opens from keyboard interaction, while it still opens correctly when the
user clicks with
a mouse.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/components/TranslationEditor/LocaleSwitcher.tsx
**Line:** 176:176
**Comment:**
*Logic Error: The suffix element is focusable and has `role="button"`,
but its keydown handler for Enter/Space only stops propagation and prevents
default without triggering the dropdown, making it impossible to open the
locale menu via keyboard; programmatically invoking a click in this handler
restores keyboard accessibility.
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>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37790&comment_hash=56e452b35212f0ab06b8d1ce186c1dc11d6d83d7f9dad1ffccdff8b4a35f2c65&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37790&comment_hash=56e452b35212f0ab06b8d1ce186c1dc11d6d83d7f9dad1ffccdff8b4a35f2c65&reaction=dislike'>👎</a>
##########
superset-frontend/src/explore/components/PropertiesModal/PropertiesModal.test.tsx:
##########
@@ -471,3 +493,74 @@ test('Should display only custom tags when tagging system
is enabled', async ()
mockIsFeatureEnabled.mockRestore();
});
+
+test('locale switcher hidden when content localization flag is off', async ()
=> {
+ const props = createProps();
+ renderModal(props);
+ await waitFor(() => {
+ expect(screen.getByRole('dialog')).toBeInTheDocument();
+ });
+ expect(
+ screen.queryByRole('button', { name: /Locale switcher for/i }),
+ ).not.toBeInTheDocument();
+});
+
+test('locale switcher visible when content localization flag is on', async ()
=> {
+ mockedIsFeatureEnabled.mockImplementation(
+ flag => flag === FeatureFlag.EnableContentLocalization,
+ );
+ const props = createProps();
+ renderModal(props);
+ await waitFor(() => {
+ expect(
+ screen.getByRole('button', {
+ name: /Locale switcher for Chart Name/i,
+ }),
+ ).toBeInTheDocument();
+ });
+ mockedIsFeatureEnabled.mockRestore();
+});
+
+test('clicking locale switcher opens dropdown with locales', async () => {
+ mockedIsFeatureEnabled.mockImplementation(
+ flag => flag === FeatureFlag.EnableContentLocalization,
+ );
+ const props = createProps();
+ renderModal(props);
+ await waitFor(() => {
+ expect(
+ screen.getByRole('button', { name: /Locale switcher for Chart Name/i }),
+ ).toBeInTheDocument();
+ });
+ await userEvent.click(
+ screen.getByRole('button', { name: /Locale switcher for Chart Name/i }),
+ );
+ await waitFor(() => {
+ expect(screen.getByText('English')).toBeInTheDocument();
+ expect(screen.getByText('German')).toBeInTheDocument();
+ });
+ mockedIsFeatureEnabled.mockRestore();
Review Comment:
**Suggestion:** The second use of `mockedIsFeatureEnabled.mockRestore()` has
the same issue: it targets a `jest.fn()` mock, not a spy, and will cause a
runtime error; replace it with `mockReset()` to correctly clear the mock state.
[logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Locale switcher dropdown test crashes due to invalid mockRestore.
- ⚠️ Aggregated test failures hinder validating localization UI behavior.
```
</details>
```suggestion
mockedIsFeatureEnabled.mockReset();
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run Jest on
`superset-frontend/src/explore/components/PropertiesModal/PropertiesModal.test.tsx`.
2. At the top of the file, `isFeatureEnabled` is mocked with `jest.fn()` via
`jest.mock('@superset-ui/core', ...)`, and `mockedIsFeatureEnabled` is a
typed alias of
that mock (lines 30–37 in the PR hunk).
3. In the test `clicking locale switcher opens dropdown with locales`
(introduced around
line 524), the code sets `mockedIsFeatureEnabled.mockImplementation(...)`
and later calls
`mockedIsFeatureEnabled.mockRestore();` at line 542.
4. Because `mockedIsFeatureEnabled` is a plain `jest.fn()` mock and not a
spy created with
`jest.spyOn`, invoking `.mockRestore()` at line 542 triggers Jest's runtime
error for
non‑spy mocks, causing this test case to error out; combined with the other
`mockRestore`
usages in this file, this keeps the entire test suite from completing
successfully.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/explore/components/PropertiesModal/PropertiesModal.test.tsx
**Line:** 542:542
**Comment:**
*Logic Error: The second use of `mockedIsFeatureEnabled.mockRestore()`
has the same issue: it targets a `jest.fn()` mock, not a spy, and will cause a
runtime error; replace it with `mockReset()` to correctly clear the mock state.
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>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37790&comment_hash=a97d33637e378beeddbd13e79bcbbe9d246d13f3684999caccc0c8d297c046f9&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37790&comment_hash=a97d33637e378beeddbd13e79bcbbe9d246d13f3684999caccc0c8d297c046f9&reaction=dislike'>👎</a>
##########
superset/localization/locale_utils.py:
##########
@@ -0,0 +1,150 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""
+Locale detection utilities for content localization.
+
+Provides functions to detect and resolve user's preferred locale
+for displaying localized content (dashboard titles, chart names, etc.).
+
+Locale detection priority:
+1. Explicit locale parameter (API override)
+2. Flask session locale (user preference)
+3. Accept-Language HTTP header (browser preference)
+4. Default locale from BABEL_DEFAULT_LOCALE config
+"""
+
+from flask import current_app, has_request_context, request, session
+
+DEFAULT_LOCALE = "en"
+
+
+def parse_accept_language(header: str) -> list[str]:
+ """
+ Parse Accept-Language HTTP header into sorted locale list.
+
+ Parses RFC 7231 Accept-Language header format and returns locales
+ sorted by quality value (q) in descending order.
+
+ Args:
+ header: Accept-Language header value (e.g., "de-DE, de;q=0.9,
en;q=0.8")
+
+ Returns:
+ List of locale codes sorted by quality descending.
+ Empty list if header is empty.
+
+ Examples:
+ >>> parse_accept_language("de-DE, de;q=0.9, en;q=0.8")
+ ["de-DE", "de", "en"]
+ >>> parse_accept_language("fr;q=0.7, de;q=0.9")
+ ["de", "fr"]
+ """
+ if not header or not header.strip():
+ return []
+
+ locales_with_quality: list[tuple[str, float]] = []
+
+ for part in header.split(","):
+ part = part.strip()
+ if not part:
+ continue
+
+ # Parse locale and quality
+ if ";" in part:
+ locale_part, quality_part = part.split(";", 1)
+ locale = locale_part.strip()
+ quality = _parse_quality(quality_part)
+ else:
+ locale = part.strip()
+ quality = 1.0 # Default quality per RFC 7231
+
+ # Skip wildcard
+ if locale == "*":
+ continue
+
+ locales_with_quality.append((locale, quality))
+
+ # Sort by quality descending, preserve order for equal quality
+ locales_with_quality.sort(key=lambda x: x[1], reverse=True)
+
+ return [locale for locale, _ in locales_with_quality]
+
+
+def _parse_quality(quality_part: str) -> float:
+ """
+ Parse quality value from Accept-Language quality parameter.
+
+ Args:
+ quality_part: Quality parameter (e.g., "q=0.9")
+
+ Returns:
+ Quality value as float. Returns 0.0 for invalid values.
+ """
+ quality_part = quality_part.strip()
+ if not quality_part.startswith("q="):
+ return 0.0
+
+ try:
+ return float(quality_part[2:])
+ except ValueError:
+ return 0.0
+
+
+def get_user_locale(locale: str | None = None, validate: bool = False) -> str:
+ """
+ Get user's locale for content localization.
+
+ Resolves locale using priority chain:
+ 1. Explicit locale parameter (if provided)
+ 2. Flask session["locale"] (user preference)
+ 3. Accept-Language header best match
+ 4. BABEL_DEFAULT_LOCALE config value
+
+ Args:
+ locale: Explicit locale override. If provided, takes priority.
+ validate: If True, validates locale against LANGUAGES config.
+ Invalid locales fall back to default.
+
+ Returns:
+ Locale code string (e.g., "de", "fr-FR", "en").
+ """
+ resolved_locale: str | None = None
+
+ # Priority 1: Explicit parameter
+ if locale:
+ resolved_locale = locale
+
+ # Priority 2: Session locale
+ if not resolved_locale:
Review Comment:
**Suggestion:** Accessing the Flask `session` object without first checking
for a request context will raise a runtime "working outside of request context"
error if `get_user_locale` is called from CLI tasks, background jobs, or any
code path without an active request. [possible bug]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ CLI/background uses of get_user_locale crash with RuntimeError.
- ⚠️ Jinja macro current_user_locale() unsafe in scheduled queries.
```
</details>
```suggestion
# Priority 2: Session locale (only if in request context)
if not resolved_locale and has_request_context():
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In a Python shell, create a Superset Flask app context only (no request
context):
`from superset.app import create_app; app = create_app();
app.app_context().push()`.
2. Import and call `get_user_locale()` from
`superset/localization/locale_utils.py:106`
within this app context:
`from superset.localization.locale_utils import get_user_locale;
get_user_locale()`.
3. Inside `get_user_locale` at
`superset/localization/locale_utils.py:130-132`, the code
executes:
`resolved_locale = session.get("locale")` without checking
`has_request_context()`.
4. Because there is no active request context, accessing `session` raises
`RuntimeError:
Working outside of request context`, causing any CLI task, background job,
or Jinja macro
using `get_user_locale` in non-request contexts to crash.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/localization/locale_utils.py
**Line:** 130:131
**Comment:**
*Possible Bug: Accessing the Flask `session` object without first
checking for a request context will raise a runtime "working outside of request
context" error if `get_user_locale` is called from CLI tasks, background jobs,
or any code path without an active request.
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>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37790&comment_hash=5a58b5ef51eac819fdfc9418c59a2f99d0b2354fb90f59d4c6f88779743c8fc9&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37790&comment_hash=5a58b5ef51eac819fdfc9418c59a2f99d0b2354fb90f59d4c6f88779743c8fc9&reaction=dislike'>👎</a>
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.test.tsx:
##########
@@ -157,6 +164,26 @@ function setupFetchMocks() {
});
}
+fetchMock.get('glob:*/api/v1/localization/available_locales', {
+ body: {
+ result: {
+ locales: [
+ { code: 'en', name: 'English' },
+ { code: 'de', name: 'German' },
+ { code: 'fr', name: 'French' },
+ ],
+ default_locale: 'en',
+ },
+ },
+});
+
+const filterTranslations = {
+ name: {
+ de: 'Bundesland',
+ fr: 'État',
+ },
+};
+
const FILTER_TYPE_REGEX = /^filter type$/i;
const FILTER_NAME_REGEX = /^filter name$/i;
Review Comment:
**Suggestion:** The mock for the localization `available_locales` endpoint
is registered once at module load but `fetchMock.removeRoutes()` in `afterEach`
clears all routes, and `setupFetchMocks()` (called in every `beforeEach`) does
not re-register this endpoint, so only the first test has the route mocked
while subsequent tests that trigger the localization fetch will hit an unmocked
URL and fail or behave inconsistently. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Localization-related native filter tests depend on this mock.
- ❌ CI test runs may intermittently fail on localization tests.
- ⚠️ Makes content localization feature tests unreliable and flaky.
- ⚠️ Increases debugging time for frontend test failures.
```
</details>
```suggestion
fetchMock.get('glob:*/api/v1/localization/available_locales', {
body: {
result: {
locales: [
{ code: 'en', name: 'English' },
{ code: 'de', name: 'German' },
{ code: 'fr', name: 'French' },
],
default_locale: 'en',
},
},
});
}
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open
`superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.test.tsx`
and note:
- `setupFetchMocks()` definition at ~lines 120–160.
- The standalone
`fetchMock.get('glob:*/api/v1/localization/available_locales', ...)`
immediately after `setupFetchMocks()` (around lines 167–178).
- The Jest hooks: `beforeEach(() => { setupFetchMocks(); });` and
`afterEach(() => {
...; fetchMock.removeRoutes(); });` near the top of the test file.
2. Observe that `afterEach` calls `fetchMock.removeRoutes()` (same file,
afterEach block
around lines 90–110), which clears *all* registered fetch-mock routes after
every test,
including the `available_locales` route registered once at module load.
3. Observe the localization tests in the same file:
- `test('locale switcher visible when content localization flag is on',
async () => {
... })`
- `test('clicking locale switcher opens dropdown with locales', async ()
=> { ... })`
- Each of these sets `mockedIsFeatureEnabled.mockImplementation(flag =>
flag ===
FeatureFlag.EnableContentLocalization)` so
`FeatureFlag.EnableContentLocalization` is
true during render, causing the FiltersConfigModal / LocaleSwitcher code
to request
`*/api/v1/localization/available_locales` (the test expectations for
labels `'English'`
and `'German'` match the mocked response body).
4. Run Jest for this file (e.g. `npm test -- FiltersConfigModal.test.tsx`):
- The early tests (e.g. `'renders a value filter type'`) run first,
`afterEach`
executes and `fetchMock.removeRoutes()` clears the globally-registered
`available_locales` mock before any localization test uses it.
- For the first localization-enabled test (e.g. `'locale switcher visible
when content
localization flag is on'`), `beforeEach` re-runs `setupFetchMocks()`, but
**does not**
re-register `available_locales`.
- When the component under test fetches
`*/api/v1/localization/available_locales`,
there is no matching fetch-mock route, so the request is unmatched and
will either fall
through to real `fetch` (failing in test environment) or be treated as an
error by
fetch-mock, making these tests fail or flaky.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.test.tsx
**Line:** 188:188
**Comment:**
*Logic Error: The mock for the localization `available_locales`
endpoint is registered once at module load but `fetchMock.removeRoutes()` in
`afterEach` clears all routes, and `setupFetchMocks()` (called in every
`beforeEach`) does not re-register this endpoint, so only the first test has
the route mocked while subsequent tests that trigger the localization fetch
will hit an unmocked URL and fail or behave inconsistently.
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>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37790&comment_hash=611621b133c4e123e1839bd48276afc03a9aaddd7ded6af08b4a6c1784b058c4&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37790&comment_hash=611621b133c4e123e1839bd48276afc03a9aaddd7ded6af08b4a6c1784b058c4&reaction=dislike'>👎</a>
##########
superset/localization/api.py:
##########
@@ -0,0 +1,108 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""REST API for content localization configuration."""
+
+import logging
+
+from flask import current_app, Response
+from flask_appbuilder.api import expose, protect, safe
+
+from superset.constants import MODEL_API_RW_METHOD_PERMISSION_MAP
+from superset.extensions import event_logger
+from superset.localization.schemas import AvailableLocalesResponseSchema
+from superset.views.base_api import BaseSupersetApi, statsd_metrics
+
+logger = logging.getLogger(__name__)
+
+DEFAULT_LOCALE = "en"
+
+
+def get_available_locales_data() -> dict[str, list[dict[str, str]] | str]:
+ """
+ Build available locales response data from Flask config.
+
+ Reads LANGUAGES and BABEL_DEFAULT_LOCALE from current_app.config
+ and transforms to response format.
+
+ Returns:
+ Dict with 'locales' list and 'default_locale' string.
+ """
+ languages: dict[str, dict[str, str]] = current_app.config.get("LANGUAGES",
{})
+ default_locale = current_app.config.get("BABEL_DEFAULT_LOCALE",
DEFAULT_LOCALE)
+
+ # Transform dict to sorted list
+ locales = [
+ {"code": code, "name": meta["name"], "flag": meta["flag"]}
+ for code, meta in sorted(languages.items())
Review Comment:
**Suggestion:** The code assumes every entry in the LANGUAGES config is a
dict with "name" and "flag" keys, so a misconfigured or partially specified
LANGUAGES (missing those keys or using non-dict values) will raise a KeyError
or TypeError and cause the /available_locales endpoint to 500 instead of
failing gracefully; using dict.get with sensible defaults and guarding against
non-dict entries avoids this runtime failure. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ GET /api/v1/localization/available_locales can return HTTP 500.
- ⚠️ Translation editor UI cannot load available locales list.
- ⚠️ Failure triggered only by malformed LANGUAGES configuration.
```
</details>
```suggestion
{
"code": code,
"name": meta.get("name", code) if isinstance(meta, dict) else
code,
"flag": meta.get("flag", "") if isinstance(meta, dict) else "",
}
for code, meta in sorted(languages.items())
if isinstance(meta, dict)
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure Superset with a malformed LANGUAGES config so that at least one
value is not
a dict with "name" and "flag" keys (used via current_app.config["LANGUAGES"]
in
`superset/localization/api.py:44`).
2. Start the Superset web server so that `LocalizationRestApi` (defined in
`superset/localization/api.py:56-72`) is registered.
3. Call the localization endpoint exposed at
`LocalizationRestApi.get_available_locales`
(`@expose("/available_locales", methods=("GET",))` at
`superset/localization/api.py:73-82`), e.g. via HTTP GET
`/api/v1/localization/available_locales`.
4. During the request, `get_available_locales_data()` at
`superset/localization/api.py:34-53` iterates over `languages.items()` and
executes
`meta["name"]` / `meta["flag"]` at lines 48-50; for any misconfigured entry
(missing keys
or non-dict value), this raises `KeyError` or `TypeError`, propagating up
and causing an
HTTP 500 response instead of a graceful, schema-conforming payload.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/localization/api.py
**Line:** 49:50
**Comment:**
*Possible Bug: The code assumes every entry in the LANGUAGES config is
a dict with "name" and "flag" keys, so a misconfigured or partially specified
LANGUAGES (missing those keys or using non-dict values) will raise a KeyError
or TypeError and cause the /available_locales endpoint to 500 instead of
failing gracefully; using dict.get with sensible defaults and guarding against
non-dict entries avoids this runtime failure.
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>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37790&comment_hash=8df02b5eeb08e2f5efd9310bfc031fdbf8da73d29dcc21b251ede1ec1a1da303&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37790&comment_hash=8df02b5eeb08e2f5efd9310bfc031fdbf8da73d29dcc21b251ede1ec1a1da303&reaction=dislike'>👎</a>
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx:
##########
@@ -569,6 +583,96 @@ const FiltersConfigForm = (
[filterId, form, formChanged],
);
+ const localizationEnabled = isFeatureEnabled(
+ FeatureFlag.EnableContentLocalization,
+ );
+
+ const currentTranslations: Translations =
+ formFilter?.translations ?? filterToEdit?.translations ?? {};
+
+ useEffect(() => {
+ if (filterToEdit?.translations) {
+ setNativeFilterFieldValues(form, filterId, {
+ translations: filterToEdit.translations,
+ });
+ }
+ }, [filterId, filterToEdit?.translations, form]);
+
+ useEffect(() => {
+ if (localizationEnabled) {
+ SupersetClient.get({
+ endpoint: '/api/v1/localization/available_locales',
+ }).then(
+ response => {
+ const { locales, default_locale } = response.json.result;
+ setAllLocales(locales);
+ setDefaultLocale(default_locale);
+ },
+ err => getClientErrorObject(err).then(setError),
Review Comment:
**Suggestion:** The localization locales fetch reuses the generic `error`
state used for default-value query failures, so a failure to load available
locales will incorrectly surface as a "Cannot load filter" error in the default
value section and may hide the default-value UI until another refresh clears
it; handle localization errors separately (e.g., with a toast) instead of
setting this shared error. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Default value editor hidden after locales API failure.
- ⚠️ Misleading "Cannot load filter" error shown to users.
- ⚠️ Users blocked from configuring default filter values.
```
</details>
```suggestion
err =>
getClientErrorObject(err).then(clientError =>
addDangerToast(
clientError.error || t('Failed to load available locales'),
),
),
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Enable the content localization feature flag so `localizationEnabled` is
true in
`FiltersConfigForm` (this component is defined in
`superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx`
and mounted when opening the Native Filters configuration modal for a
dashboard filter).
2. Cause `/api/v1/localization/available_locales` to fail, e.g. by
misconfiguring the
backend or temporarily returning HTTP 500, so that the `SupersetClient.get`
call in the
`useEffect` at lines 601–614 rejects.
3. Open the Native Filters configuration modal for any filter so that
`FiltersConfigForm`
mounts and the localization `useEffect` runs; on request failure the `err`
handler calls
`getClientErrorObject(err).then(setError)` which sets the shared `error`
state declared
near the top of this file (`const [error, setError] =
useState<ClientErrorObject>();`).
4. In the same modal, scroll to the "Default Value" section where `error` is
consumed
around the `DefaultValue` rendering (`error ? <ErrorMessageWithStackTrace
... /> :
<DefaultValue ... />` inside the `DefaultValueContainer`), and observe that
the UI shows a
"Cannot load filter" error and hides the default-value picker even though
the dataset
query itself is not failing; this incorrect error state persists until a
successful
default-value refresh clears it.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx
**Line:** 611:611
**Comment:**
*Logic Error: The localization locales fetch reuses the generic `error`
state used for default-value query failures, so a failure to load available
locales will incorrectly surface as a "Cannot load filter" error in the default
value section and may hide the default-value UI until another refresh clears
it; handle localization errors separately (e.g., with a toast) instead of
setting this shared error.
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>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37790&comment_hash=27392454ebd6be844980940d5f08f29b1816feec481895200719a8ee7c3c8efc&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37790&comment_hash=27392454ebd6be844980940d5f08f29b1816feec481895200719a8ee7c3c8efc&reaction=dislike'>👎</a>
##########
superset-frontend/src/explore/components/PropertiesModal/PropertiesModal.test.tsx:
##########
@@ -471,3 +493,74 @@ test('Should display only custom tags when tagging system
is enabled', async ()
mockIsFeatureEnabled.mockRestore();
});
+
+test('locale switcher hidden when content localization flag is off', async ()
=> {
+ const props = createProps();
+ renderModal(props);
+ await waitFor(() => {
+ expect(screen.getByRole('dialog')).toBeInTheDocument();
+ });
+ expect(
+ screen.queryByRole('button', { name: /Locale switcher for/i }),
+ ).not.toBeInTheDocument();
+});
+
+test('locale switcher visible when content localization flag is on', async ()
=> {
+ mockedIsFeatureEnabled.mockImplementation(
+ flag => flag === FeatureFlag.EnableContentLocalization,
+ );
+ const props = createProps();
+ renderModal(props);
+ await waitFor(() => {
+ expect(
+ screen.getByRole('button', {
+ name: /Locale switcher for Chart Name/i,
+ }),
+ ).toBeInTheDocument();
+ });
+ mockedIsFeatureEnabled.mockRestore();
+});
+
+test('clicking locale switcher opens dropdown with locales', async () => {
+ mockedIsFeatureEnabled.mockImplementation(
+ flag => flag === FeatureFlag.EnableContentLocalization,
+ );
+ const props = createProps();
+ renderModal(props);
+ await waitFor(() => {
+ expect(
+ screen.getByRole('button', { name: /Locale switcher for Chart Name/i }),
+ ).toBeInTheDocument();
+ });
+ await userEvent.click(
+ screen.getByRole('button', { name: /Locale switcher for Chart Name/i }),
+ );
+ await waitFor(() => {
+ expect(screen.getByText('English')).toBeInTheDocument();
+ expect(screen.getByText('German')).toBeInTheDocument();
+ });
+ mockedIsFeatureEnabled.mockRestore();
+});
+
+test('save includes translations in PUT payload', async () => {
+ const put = jest.spyOn(SupersetClient, 'put');
+ put.mockResolvedValue({ json: { result: {} } } as unknown as ReturnType<
+ typeof SupersetClient.put
+ >);
+ mockedIsFeatureEnabled.mockImplementation(
+ flag => flag === FeatureFlag.EnableContentLocalization,
+ );
+ const props = createProps();
+ renderModal(props);
+ await waitFor(() => {
+ expect(screen.getByRole('button', { name: 'Save' })).toBeEnabled();
+ });
+ await userEvent.click(screen.getByRole('button', { name: 'Save' }));
+ await waitFor(() => {
+ expect(put).toHaveBeenCalledTimes(1);
+ });
+ const putBody = JSON.parse(put.mock.calls[0][0].body as string);
+ expect(putBody.translations).toEqual(chartTranslations);
+ put.mockRestore();
+ mockedIsFeatureEnabled.mockRestore();
Review Comment:
**Suggestion:** The final `mockedIsFeatureEnabled.mockRestore()` in the save
test suffers from the same problem as above—`mockedIsFeatureEnabled` is a
module-level `jest.fn()` mock, not a spy, so calling `mockRestore()` will fail;
use `mockReset()` to restore its default behavior after the test. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Save-with-translations test fails due to misuse of mockRestore.
- ⚠️ Makes it harder to validate PUT payload for translations.
```
</details>
```suggestion
mockedIsFeatureEnabled.mockReset();
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Execute the Jest suite for
`superset-frontend/src/explore/components/PropertiesModal/PropertiesModal.test.tsx`.
2. As above, note that `isFeatureEnabled` is mocked with `jest.fn()` via
`jest.mock('@superset-ui/core', ...)`, and `mockedIsFeatureEnabled` simply
references that
mock (lines 30–37 in the PR hunk).
3. In the test `save includes translations in PUT payload` (introduced
around lines
545–566), the code sets `mockedIsFeatureEnabled.mockImplementation(...)` to
enable the
content-localization feature flag and, at the end of the test, invokes
`mockedIsFeatureEnabled.mockRestore();` at line 565.
4. Since `mockedIsFeatureEnabled` is not a spy created with `jest.spyOn` but
a standalone
`jest.fn()` mock, calling `.mockRestore()` at line 565 throws a Jest runtime
error,
preventing this save/PUT-payload test from completing and contributing to
overall frontend
test failures.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/explore/components/PropertiesModal/PropertiesModal.test.tsx
**Line:** 565:565
**Comment:**
*Logic Error: The final `mockedIsFeatureEnabled.mockRestore()` in the
save test suffers from the same problem as above—`mockedIsFeatureEnabled` is a
module-level `jest.fn()` mock, not a spy, so calling `mockRestore()` will fail;
use `mockReset()` to restore its default behavior after the test.
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>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37790&comment_hash=6169320341e2ed360f6f560dd56341d8d828d1a5b68dbf8c07170023feefe738&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37790&comment_hash=6169320341e2ed360f6f560dd56341d8d828d1a5b68dbf8c07170023feefe738&reaction=dislike'>👎</a>
##########
superset-frontend/playwright/pages/DashboardPage.ts:
##########
@@ -125,4 +125,22 @@ export class DashboardPage {
await menu.selectSubmenuItem('Download', optionText);
return downloadPromise;
}
+
+ /**
+ * Enter edit mode by clicking "Edit dashboard" button.
+ * Required before accessing edit-only menu items like "Edit properties".
+ */
+ async enterEditMode(): Promise<void> {
+ await this.page.click('[data-test="edit-dashboard-button"]');
+ }
+
+ /**
+ * Click "Edit properties" in the header actions menu.
+ * Requires edit mode — call enterEditMode() first.
+ */
+ async clickEditProperties(): Promise<void> {
+ await this.openHeaderActionsMenu();
+ const menu =
this.page.locator(DashboardPage.SELECTORS.HEADER_ACTIONS_MENU);
+ await menu.getByText('Edit properties', { exact: true }).click();
Review Comment:
**Suggestion:** The Playwright selector for the "Edit properties" menu item
relies on the visible text "Edit properties", which will change under content
localization and cause tests using this helper to fail or hang when the UI is
not in English; use a stable, non-localized selector (e.g., a data-test
attribute) instead so tests remain robust across locales. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Dashboard edit E2E tests fail under non-English UI locale.
- ⚠️ Blocks reliable automation for locale-switching dashboard scenarios.
```
</details>
```suggestion
await menu.locator('[data-test="edit-properties-menu-item"]').click();
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Start the Superset frontend with the PR code and configure the UI to use
a non-English
locale so that header menu items are translated (existing Superset i18n
behavior; this
affects the label for the edit-properties action).
2. In a Playwright test, construct a `DashboardPage` instance from
`superset-frontend/playwright/pages/DashboardPage.ts` and navigate to a
dashboard via
`gotoById()` or `gotoBySlug()`, then call `waitForLoad()` to ensure the
header is
rendered.
3. Call `enterEditMode()` (lines 133–135 in `DashboardPage.ts`) so that the
edit-only
header actions, including the properties menu item, become available in the
header actions
menu.
4. Invoke `clickEditProperties()` (lines 141–145 in `DashboardPage.ts`);
this calls
`openHeaderActionsMenu()` and then `menu.getByText('Edit properties', {
exact: true
}).click()`. Because the menu item text is now translated and no longer
exactly `'Edit
properties'`, Playwright fails to find the element and times out, causing
the E2E test
using this helper to fail or hang whenever it runs under a non-English
locale.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/playwright/pages/DashboardPage.ts
**Line:** 144:144
**Comment:**
*Logic Error: The Playwright selector for the "Edit properties" menu
item relies on the visible text "Edit properties", which will change under
content localization and cause tests using this helper to fail or hang when the
UI is not in English; use a stable, non-localized selector (e.g., a data-test
attribute) instead so tests remain robust across locales.
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>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37790&comment_hash=69bda1faf96e4cd9b62581fb75682ed8e982f45e247650702bf3193c39f002bd&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37790&comment_hash=69bda1faf96e4cd9b62581fb75682ed8e982f45e247650702bf3193c39f002bd&reaction=dislike'>👎</a>
##########
superset/dashboards/schemas.py:
##########
@@ -445,6 +574,50 @@ class DashboardPutSchema(BaseDashboardSchema):
fields.Integer(metadata={"description": tags_description},
allow_none=True)
)
uuid = fields.UUID(allow_none=True)
+ translations = fields.Dict(
+ metadata={"description": "Translations dict for content localization"},
+ allow_none=True,
+ )
+
+ @validates_schema
+ def validate_translations_data(
+ self, data: dict[str, Any], **kwargs: Any
+ ) -> None:
+ """
+ Validate translations field: feature flag and structure.
+
+ Checks:
+ 1. Feature flag ENABLE_CONTENT_LOCALIZATION must be enabled
+ 2. Structure must be valid: {field: {locale: value}}
+
+ Raises:
+ ValidationError: If feature disabled or structure invalid.
+ """
+ if "translations" not in data:
+ return
+
+ if not is_feature_enabled("ENABLE_CONTENT_LOCALIZATION"):
+ raise ValidationError(
+ "Content localization is not enabled. "
+ "Set ENABLE_CONTENT_LOCALIZATION=True to use translations.",
+ field_name="translations",
+ )
+
+ validate_translations(data["translations"])
Review Comment:
**Suggestion:** The schema allows `translations` to be `None`
(`allow_none=True`), but the schema-level validator unconditionally passes this
value into `validate_translations`, which is likely to expect a dictionary and
may raise a non-ValidationError (e.g. TypeError), resulting in a 500 instead of
a clean validation error or acceptance of `null` as "no translations". [type
error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ PUT /api/v1/dashboard fails when translations explicitly null.
- ⚠️ External clients cannot clear translations using null payload.
- ⚠️ Risk of 500 error from schema validator.
```
</details>
```suggestion
translations = data.get("translations")
# Allow explicit null (treated the same as absence of translations)
if translations is None:
return
if not is_feature_enabled("ENABLE_CONTENT_LOCALIZATION"):
raise ValidationError(
"Content localization is not enabled. "
"Set ENABLE_CONTENT_LOCALIZATION=True to use translations.",
field_name="translations",
)
validate_translations(translations)
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Call the dashboard update endpoint `PUT /api/v1/dashboard/<id>` (handled
by the
Dashboard REST API that uses `DashboardPutSchema` for request validation;
schema defined
in `superset/dashboards/schemas.py:610-676` in the final file).
2. In the JSON payload, include `"translations": null` while
`ENABLE_CONTENT_LOCALIZATION`
is enabled so that the request reaches `DashboardPutSchema.translations`
(field declared
at `superset/dashboards/schemas.py:672-676`).
3. Marshmallow deserializes the payload: because `translations` is declared
as
`fields.Dict(..., allow_none=True)`, the deserialized `data` passed into
`validate_translations_data` at `superset/dashboards/schemas.py:582-606`
contains the key
`"translations"` with value `None`.
4. Inside `validate_translations_data`, the `"translations" in data` check
passes and
`validate_translations(data["translations"])` is invoked with `None` at line
606; since
`validate_translations` (from `superset.localization`) is designed to
validate a mapping
of translations, passing `None` can result in a non-`ValidationError`
exception (e.g.
`TypeError`) or an unexpected validation failure, producing a 500 or
rejecting `null`
despite `allow_none=True`.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/dashboards/schemas.py
**Line:** 599:606
**Comment:**
*Type Error: The schema allows `translations` to be `None`
(`allow_none=True`), but the schema-level validator unconditionally passes this
value into `validate_translations`, which is likely to expect a dictionary and
may raise a non-ValidationError (e.g. TypeError), resulting in a 500 instead of
a clean validation error or acceptance of `null` as "no translations".
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>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37790&comment_hash=4d879dab4a9a8665436dfc3a5252648eac6a7269ba2ca0717c726b8dcfbb06a1&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37790&comment_hash=4d879dab4a9a8665436dfc3a5252648eac6a7269ba2ca0717c726b8dcfbb06a1&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]
