codeant-ai-for-open-source[bot] commented on code in PR #37790: URL: https://github.com/apache/superset/pull/37790#discussion_r2818620004
########## superset-frontend/src/components/TranslationEditor/utils.ts: ########## @@ -0,0 +1,163 @@ +/** + * 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 type { Translations } from 'src/types/Localization'; +import type { QueryFormMetric } from '@superset-ui/core'; +import getBootstrapData from 'src/utils/getBootstrapData'; + +/** + * Sentinel value representing the default (untranslated) column text + * in the locale switcher. Not a real locale code. + */ +export const DEFAULT_LOCALE_KEY = 'default'; + +/** Remove empty-string translation values; drop fields with no translations. */ +export function stripEmptyValues(translations: Translations): Translations { + const cleaned: Translations = {}; + for (const [field, locales] of Object.entries(translations)) { + const filtered: Record<string, string> = {}; + for (const [locale, value] of Object.entries(locales)) { + if (value) { + filtered[locale] = value; + } + } + if (Object.keys(filtered).length > 0) { + cleaned[field] = filtered; + } + } + return cleaned; +} + +/** Count non-empty translations for a specific field. */ +export function countFieldTranslations( + translations: Translations, + fieldName: string, +): number { + const fieldMap = translations[fieldName]; + if (!fieldMap) return 0; + return Object.values(fieldMap).filter(Boolean).length; Review Comment: **Suggestion:** The translation count helper currently includes the sentinel default locale key in its count, so default (untranslated) text will be treated as a translation, inflating the count and potentially breaking any logic that depends on whether there are real translations or not. It should explicitly exclude the `DEFAULT_LOCALE_KEY` when counting. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Translation editor may overreport number of available translations. - ⚠️ UI badges depending on counts may show misleading numbers. - ⚠️ Logic checking "has translations" may behave incorrectly. ``` </details> ```suggestion return Object.entries(fieldMap) .filter( ([locale, value]) => locale !== DEFAULT_LOCALE_KEY && Boolean(value), ) .length; ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. In a test or caller, import `countFieldTranslations` and `DEFAULT_LOCALE_KEY` from `superset-frontend/src/components/TranslationEditor/utils.ts` (function at lines 47–53, constant at line 27). 2. Construct a `translations` object for some field, e.g. `{ label: { [DEFAULT_LOCALE_KEY]: 'Sales', de: 'Verkäufe' } }`, where the `DEFAULT_LOCALE_KEY` entry represents the untranslated base text. 3. Call `countFieldTranslations(translations, 'label')` so execution enters the implementation at `utils.ts:47–53`. 4. Observe the returned value is `2` (both `default` and `de` counted) even though there is only one real translation locale; the current implementation counts all non-empty entries and does not exclude the sentinel `DEFAULT_LOCALE_KEY`. ``` </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/utils.ts **Line:** 53:53 **Comment:** *Logic Error: The translation count helper currently includes the sentinel default locale key in its count, so default (untranslated) text will be treated as a translation, inflating the count and potentially breaking any logic that depends on whether there are real translations or not. It should explicitly exclude the `DEFAULT_LOCALE_KEY` when counting. 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=2df2680831b317d8029a1b29e9711ce2d9123dc7ee243e85df2810576aca7eb7&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37790&comment_hash=2df2680831b317d8029a1b29e9711ce2d9123dc7ee243e85df2810576aca7eb7&reaction=dislike'>👎</a> ########## superset-frontend/src/components/TranslationEditor/LocaleSwitcher.tsx: ########## @@ -0,0 +1,253 @@ +/** + * 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 { useCallback, useMemo, useState } 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; + /** When false, renders as a read-only indicator without dropdown. Default true. */ + interactive?: boolean; + /** Called when the dropdown opens or closes. */ + onDropdownOpenChange?: (open: boolean) => void; +} + +/** + * 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, + interactive = true, + onDropdownOpenChange, +}: 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 [dropdownOpen, setDropdownOpen] = useState(false); + + const handleOpenChange = useCallback( + (open: boolean) => { + setDropdownOpen(open); + onDropdownOpenChange?.(open); + }, + [onDropdownOpenChange], + ); + + const handleMenuClick: MenuProps['onClick'] = ({ key }) => { + onLocaleChange(key); + handleOpenChange(false); + }; + + const suffixColor = isWarning ? theme.colorWarning : theme.colorText; + + const ariaLabel = t( + 'Locale switcher for %s: %s (%s translations)', + fieldLabel, + isDefault ? t('DEFAULT') : activeLocale, + translationCount, + ); + + const iconElement = triggerFlag ? ( + <span + css={css` + font-size: 14px; + `} + > + {triggerFlag} + </span> + ) : ( + <Icons.GlobalOutlined + iconSize="m" + css={css` + color: ${suffixColor}; + `} + /> + ); + + const indicatorCss = css` + display: inline-flex; + align-items: center; + gap: 4px; + line-height: 1; + color: ${suffixColor}; + white-space: nowrap; + user-select: none; + `; + + if (!interactive) { + return ( + <span aria-label={ariaLabel} css={indicatorCss}> + {iconElement} + <Badge count={translationCount} size="small" showZero={false} /> + </span> + ); + } + + return ( + <Dropdown + menu={{ items: menuItems, onClick: handleMenuClick }} + trigger={['click']} + open={dropdownOpen} + onOpenChange={handleOpenChange} + getPopupContainer={() => document.body} + > + <span + role="button" + tabIndex={0} + aria-label={ariaLabel} + onClick={e => e.stopPropagation()} + onKeyDown={e => { + if (e.key === 'Enter' || e.key === ' ') { + e.stopPropagation(); + e.preventDefault(); + setDropdownOpen(prev => !prev); Review Comment: **Suggestion:** When the user toggles the dropdown with keyboard (Enter/Space), the component updates its internal `dropdownOpen` state directly but does not call the `onDropdownOpenChange` callback, so consumers relying on this callback only see open/close changes for mouse clicks and not for keyboard interactions, causing inconsistent external state and making the documented API misleading. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Keyboard toggling doesn't trigger onDropdownOpenChange callback for consumers. - ⚠️ External state synced to dropdown open state becomes inconsistent. - ⚠️ Accessibility keyboard users experience subtly different behavior than mouse. ``` </details> ```suggestion handleOpenChange(!dropdownOpen); ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Render `LocaleSwitcher` (superset-frontend/src/components/TranslationEditor/LocaleSwitcher.tsx) with `interactive` left as default `true` and pass a non-noop `onDropdownOpenChange` callback (e.g., logging to console). 2. Focus the locale switcher trigger `<span>` by tabbing to it in the UI; this uses the element defined at lines 224–240 with `role="button"` and `tabIndex={0}`. 3. Press `Space` or `Enter` so `onKeyDown` at lines 229–235 executes, calling `setDropdownOpen(prev => !prev)` and updating the controlled `open` prop on the surrounding `<Dropdown>` at lines 217–223, causing the menu to open/close. 4. Observe that the `onDropdownOpenChange` callback passed into props is never invoked for this keyboard toggle path because `handleOpenChange` (lines 158–164), which is responsible for calling `onDropdownOpenChange`, is not used in `onKeyDown`, whereas mouse-driven open/close flows through `onOpenChange={handleOpenChange}` on the `<Dropdown>`. ``` </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:** 233:233 **Comment:** *Logic Error: When the user toggles the dropdown with keyboard (Enter/Space), the component updates its internal `dropdownOpen` state directly but does not call the `onDropdownOpenChange` callback, so consumers relying on this callback only see open/close changes for mouse clicks and not for keyboard interactions, causing inconsistent external state and making the documented API misleading. 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=bc0543a53a4cebcafa3e77bf4d4aba7ebfbee564f760779092db99fa109a276e&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37790&comment_hash=bc0543a53a4cebcafa3e77bf4d4aba7ebfbee564f760779092db99fa109a276e&reaction=dislike'>👎</a> ########## superset-frontend/packages/superset-ui-core/src/query/getLocalizedFormDataValue.ts: ########## @@ -0,0 +1,49 @@ +/** + * 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. + */ + +/** + * Resolve a localized value for a formData text field. + * + * Looks up `translations[fieldName][locale]` with base language fallback + * (e.g., "de" from "de-AT"). + * + * @param translations - The translations object from formData (field → locale → value) + * @param fieldName - The control name (e.g., "x_axis_title", "subtitle") + * @param locale - The user's locale (e.g., "de", "de-AT") + * @returns The translated string, or undefined if no translation exists + */ +export default function getLocalizedFormDataValue( + translations: Record<string, Record<string, string>> | undefined, + fieldName: string, + locale?: string, +): string | undefined { + if (!translations || !locale) return undefined; + + const fieldTranslations = translations[fieldName]; + if (!fieldTranslations) return undefined; + + if (fieldTranslations[locale]) return fieldTranslations[locale]; + + const baseLang = locale.split('-')[0]; + if (baseLang !== locale && fieldTranslations[baseLang]) { Review Comment: **Suggestion:** The function treats translation values as truthy when deciding whether a translation exists, so if a valid translation is an empty string (e.g., intentionally blank label), it will be ignored and the function will incorrectly fall back to another locale or return undefined. The check should instead verify the presence of a key in the translations map, not the truthiness of its value, so that empty string translations are respected. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Localized formData fields cannot intentionally be blank. - ⚠️ Fallback locale text appears instead of empty translation. ``` </details> ```suggestion if (locale in fieldTranslations) { return fieldTranslations[locale]; } const baseLang = locale.split('-')[0]; if (baseLang !== locale && baseLang in fieldTranslations) { ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Build or run the frontend including `getLocalizedFormDataValue` at `superset-frontend/packages/superset-ui-core/src/query/getLocalizedFormDataValue.ts:31-49`. 2. In a unit test or caller, invoke `getLocalizedFormDataValue` with `translations = { subtitle: { 'de': '', 'en': 'My subtitle' } }`, `fieldName = 'subtitle'`, and `locale = 'de'`. 3. Inside the function at lines 38-41, `fieldTranslations` is `{ 'de': '', 'en': 'My subtitle' }` and `fieldTranslations[locale]` evaluates to `''` which is falsy, so the `if (fieldTranslations[locale])` check fails and does not return the empty string. 4. Execution proceeds to the fallback logic at lines 43-46; if no base-language key is used or differs, the function returns `undefined` instead of the explicitly configured empty string, so callers cannot distinguish between "no translation" and "intentionally blank". ``` </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/query/getLocalizedFormDataValue.ts **Line:** 41:44 **Comment:** *Logic Error: The function treats translation values as truthy when deciding whether a translation exists, so if a valid translation is an empty string (e.g., intentionally blank label), it will be ignored and the function will incorrectly fall back to another locale or return undefined. The check should instead verify the presence of a key in the translations map, not the truthiness of its value, so that empty string translations are respected. 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=6fd48310d55624c4259d7f9c5ccd54b553c6db86b0d1fdbd0f8b7554ac975461&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37790&comment_hash=6fd48310d55624c4259d7f9c5ccd54b553c6db86b0d1fdbd0f8b7554ac975461&reaction=dislike'>👎</a> ########## superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts: ########## @@ -223,6 +228,24 @@ export default function transformProps( metricsB = [], }: EchartsMixedTimeseriesFormData = { ...DEFAULT_FORM_DATA, ...formData }; + const localizedMetricLabelMap = buildLocalizedMetricLabelMap( + [...metrics, ...metricsB], + locale, + ); + + const localizedXAxisTitle = + getLocalizedFormDataValue(formData.translations, 'x_axis_title', locale) ?? + xAxisTitle; + const localizedYAxisTitle = + getLocalizedFormDataValue(formData.translations, 'y_axis_title', locale) ?? + yAxisTitle; + const localizedYAxisTitleSecondary = + getLocalizedFormDataValue( + formData.translations, + 'yAxisTitleSecondary', + locale, Review Comment: **Suggestion:** The secondary Y-axis title uses a camelCase translation key (`'yAxisTitleSecondary'`) while all other localized form-data keys use snake_case, so any translations stored under the expected key (e.g. `'y_axis_title_secondary'`) will never be found and the axis title will not localize as intended. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Secondary Y-axis title remains untranslated in localized charts. - ⚠️ MixedTimeseries legend/axis localization appears inconsistent to users. ``` </details> ```suggestion 'y_axis_title_secondary', ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Create or edit a Mixed Timeseries chart in Superset so it uses a secondary Y-axis (chart type wired to `transformProps` in `plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts`). 2. In the chart's customize/axis controls, set a secondary Y-axis title (this populates `formData.yAxisTitleSecondary` and, via localization, a translation entry under the control key `y_axis_title_secondary` in `formData.translations`). 3. Configure a non-default UI locale (e.g. `de`) and provide a localized value for the secondary Y-axis title so `formData.translations['y_axis_title_secondary']['de']` is set when the chart is loaded. 4. View the chart in that locale so `transformProps` runs: at lines 243–248 it calls `getLocalizedFormDataValue(formData.translations, 'yAxisTitleSecondary', locale)`, which returns `undefined` because the translations map is keyed by `y_axis_title_secondary`; the code then falls back to `yAxisTitleSecondary`, and at lines ~700–714 assigns `name: localizedYAxisTitleSecondary` to `yAxis[1]`, resulting in the secondary Y-axis title not being localized while other axis titles (looked up via `'x_axis_title'` and `'y_axis_title'`) are localized correctly. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts **Line:** 246:246 **Comment:** *Logic Error: The secondary Y-axis title uses a camelCase translation key (`'yAxisTitleSecondary'`) while all other localized form-data keys use snake_case, so any translations stored under the expected key (e.g. `'y_axis_title_secondary'`) will never be found and the axis title will not localize as intended. 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=1851f7d7422bb459d6ff451d4ac90d9bd8a0bfc404790ba8716281dff6798d11&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37790&comment_hash=1851f7d7422bb459d6ff451d4ac90d9bd8a0bfc404790ba8716281dff6798d11&reaction=dislike'>👎</a> ########## superset-frontend/src/explore/components/SaveModal.tsx: ########## @@ -152,8 +163,30 @@ class SaveModal extends Component<SaveModalProps, SaveModalState> { ); } } + + if ( + isFeatureEnabled(FeatureFlag.EnableContentLocalization) && + this.props.slice?.slice_id + ) { + try { + const response = await SupersetClient.get({ + endpoint: `/api/v1/chart/${this.props.slice.slice_id}?include_translations=true`, + }); + const chart = response.json.result; + this.setState({ + newSliceName: chart.slice_name || this.state.newSliceName, + translations: chart.translations ?? {}, Review Comment: **Suggestion:** After fetching chart translations in componentDidMount, the code unconditionally overwrites the current chart name with the server value whenever it is non-empty; if the user has already started editing the name before the request resolves, their changes will be silently clobbered by the async response. Use a functional setState and only apply the server value when the current name is still empty, while also preserving any existing translations. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Save chart modal discards in-progress chart name edits. - ⚠️ Affects existing charts with localization feature enabled. ``` </details> ```suggestion this.setState(prevState => ({ newSliceName: prevState.newSliceName || chart.slice_name || prevState.newSliceName, translations: chart.translations ?? prevState.translations ?? {}, })); ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open an existing saved chart in Explore and click the "Save" button to open the SaveModal, which renders `SaveModal` in `superset-frontend/src/explore/components/SaveModal.tsx` and triggers its `componentDidMount()` method (around lines 117–186). 2. With `ENABLE_CONTENT_LOCALIZATION` enabled and `this.props.slice.slice_id` present, `componentDidMount()` enters the `if (isFeatureEnabled(FeatureFlag.EnableContentLocalization) && this.props.slice?.slice_id)` block (lines ~167–183) and issues an async GET via `SupersetClient.get()` to `/api/v1/chart/{slice_id}?include_translations=true` (line 172). 3. While that request is in flight (non-trivial network latency), the user edits the "Chart name" field in the modal; this field is rendered as `TranslatableSliceNameField` in `renderSaveChartModal()` (lines ~635–666) with `onChange={this.onSliceNameChange}`, which invokes `onSliceNameChange()` (lines ~195–198) to update `this.state.newSliceName` with the user's typed value. 4. When the GET request resolves, the code at lines 172–178 runs `this.setState({ newSliceName: chart.slice_name || this.state.newSliceName, ... })`; since `chart.slice_name` is non-empty for an existing chart, it wins the `||` expression and overwrites the user-edited `newSliceName` with the server value, silently discarding the in-progress edit in the SaveModal UI. ``` </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/SaveModal.tsx **Line:** 176:178 **Comment:** *Logic Error: After fetching chart translations in componentDidMount, the code unconditionally overwrites the current chart name with the server value whenever it is non-empty; if the user has already started editing the name before the request resolves, their changes will be silently clobbered by the async response. Use a functional setState and only apply the server value when the current name is still empty, while also preserving any existing 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=5ce8a2ce4b842f4ebf349a4a259ca8f99ff94d5dc4e5e24e55d10b190a25971f&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37790&comment_hash=5ce8a2ce4b842f4ebf349a4a259ca8f99ff94d5dc4e5e24e55d10b190a25971f&reaction=dislike'>👎</a> ########## superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx: ########## @@ -569,6 +586,109 @@ 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 => logging.error('Failed to fetch available locales', err), + ); + } + }, [localizationEnabled]); + + // Refs for values read during initialization but not triggering re-runs. + // Without refs, editing a translation would reset the active locale + // because currentTranslations changes on every keystroke. + const currentTranslationsRef = useRef(currentTranslations); + currentTranslationsRef.current = currentTranslations; + const userLocaleRef = useRef(userLocale); + userLocaleRef.current = userLocale; + + // Initialize nameActiveLocale when switching between filters. + useEffect(() => { + if ( + localizationEnabled && + currentTranslationsRef.current.name?.[userLocaleRef.current] + ) { + setNameActiveLocale(userLocaleRef.current); + } else { + setNameActiveLocale(DEFAULT_LOCALE_KEY); + } + }, [localizationEnabled, filterId]); + + const handleNameTranslationChange = useCallback( + (value: string) => { + const updated: Translations = { + ...currentTranslationsRef.current, + name: { ...currentTranslationsRef.current.name, [nameActiveLocale]: value }, + }; Review Comment: **Suggestion:** When there is no existing `name` entry in the translations object, spreading `currentTranslationsRef.current.name` will attempt to spread `undefined`, causing a runtime TypeError ("Cannot convert undefined or null to object") the first time a translation is edited; guard this by falling back to an empty object before spreading. [logic error] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Editing filter name translation crashes dashboard native filter modal. - ⚠️ Prevents adding localized names for new native filters. ``` </details> ```suggestion name: { ...(currentTranslationsRef.current.name ?? {}), [nameActiveLocale]: value, }, ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Enable the content localization feature flag on the backend so `isFeatureEnabled(FeatureFlag.EnableContentLocalization)` returns true, ensuring the localization logic in `FiltersConfigForm` (`superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx`) is active. 2. Open any dashboard and launch the Native Filters configuration modal, which renders the `FiltersConfigForm` component for a given filter (`FiltersConfigForm` is the configuration UI for native filters in this modal). 3. Use a filter that has no `translations` persisted yet (for new filters, `filterToEdit?.translations` and `formFilter?.translations` are `undefined`, so `currentTranslations` and `currentTranslationsRef.current` are `{}` without a `name` property at the time `handleNameTranslationChange` is defined). 4. In the filter configuration tab, enter a non-empty default filter name so `handleNameLocaleChange` allows switching away from `DEFAULT_LOCALE_KEY`, then use the inline `LocaleSwitcher` suffix on the name input to select a non-default locale; this flips `nameActiveLocale` and causes `isEditingFilterTranslation` to become true, rendering the `TranslationInput` whose `onChange` prop calls `handleNameTranslationChange`. 5. Start typing into the translated name field; on the first keystroke, `handleNameTranslationChange` executes and evaluates `name: { ...currentTranslationsRef.current.name, [nameActiveLocale]: value }` at `FiltersConfigForm.tsx:640-646`, where `currentTranslationsRef.current.name` is `undefined`, causing a runtime `TypeError: Cannot convert undefined or null to object` when the spread operator attempts to spread `undefined`, breaking the filter configuration UI for that filter. ``` </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:** 644:644 **Comment:** *Logic Error: When there is no existing `name` entry in the translations object, spreading `currentTranslationsRef.current.name` will attempt to spread `undefined`, causing a runtime TypeError ("Cannot convert undefined or null to object") the first time a translation is edited; guard this by falling back to an empty object before spreading. 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=649f66cc86ac131017d5281c79d29512615fbd126bf92f648d90f0c9362ca8bb&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37790&comment_hash=649f66cc86ac131017d5281c79d29512615fbd126bf92f648d90f0c9362ca8bb&reaction=dislike'>👎</a> ########## superset/localization/schema_mixin.py: ########## @@ -0,0 +1,93 @@ +# 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. +""" +Marshmallow mixin for translations field. + +Provides translations field with feature flag validation and XSS sanitization. +Used by all schemas that accept user-provided translations (Post, Put, Copy). +""" + +from __future__ import annotations + +from typing import Any + +from marshmallow import ValidationError, fields, post_load, validates_schema + +from superset import is_feature_enabled +from superset.localization.sanitization import sanitize_translations +from superset.localization.validation import validate_translations + + +class TranslatableSchemaMixin: + """ + Mixin for Marshmallow schemas that accept a ``translations`` field. + + Adds: + - ``translations`` dict field (optional, nullable) + - ``@validates_schema``: feature flag check + structure validation + - ``@post_load``: XSS sanitization of translation values + + Usage:: + + class MySchema(TranslatableSchemaMixin, Schema): + name = fields.String() + """ + + 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 declares the translations field as nullable, but the schema-level validator still runs and enforces the feature flag and structure checks when translations is explicitly set to null, causing valid requests with `"translations": null` to either fail validation or be passed as None into the structure validator which expects a dict-like object. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Post/Put/Copy schemas using mixin reject null translations. - ⚠️ Dashboard and chart localization APIs may raise server errors. - ⚠️ Optional nullable field behaves inconsistently with schema declaration. ``` </details> ```suggestion translations = data.get("translations") # Allow explicitly null translations when field is nullable 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. Import `TranslatableSchemaMixin` from `superset/localization/schema_mixin.py:35` into a test module or Python shell. 2. Define a Marshmallow schema subclass that mixes in `TranslatableSchemaMixin` (e.g., `class TestSchema(TranslatableSchemaMixin, Schema): name = fields.String()`), so that `validate_translations_data` at lines 55–79 is executed during `.load()`. 3. With feature flag `ENABLE_CONTENT_LOCALIZATION` enabled (so the feature-flag check at line 72 passes), call `TestSchema().load({"name": "foo", "translations": None})` so that `data` passed into `validate_translations_data` contains `"translations": None`. 4. Inside `validate_translations_data` (`schema_mixin.py:69–79`), the condition `if "translations" not in data:` at line 69 is false because the key is present, so the function unconditionally calls `validate_translations(data["translations"])` at line 79 with `None`, turning a field declared `allow_none=True` into a value that must pass the translations feature flag and structure validation, and potentially raising if `validate_translations` expects a mapping. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/localization/schema_mixin.py **Line:** 69:79 **Comment:** *Logic Error: The schema declares the translations field as nullable, but the schema-level validator still runs and enforces the feature flag and structure checks when translations is explicitly set to null, causing valid requests with `"translations": null` to either fail validation or be passed as None into the structure validator which expects a dict-like object. 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=3c78813fcf3495f2571b90d4c65db55ed83f62a779ec9bc9983ef26ab1ffe2dd&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37790&comment_hash=3c78813fcf3495f2571b90d4c65db55ed83f62a779ec9bc9983ef26ab1ffe2dd&reaction=dislike'>👎</a> ########## superset-frontend/src/locale/LocaleController.ts: ########## @@ -0,0 +1,263 @@ +/** + * 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 { configure, t as translate, tn as translateN } from '@apache-superset/core/ui'; +import type { LanguagePack } from '@apache-superset/core/ui'; +import { SupersetClient } from '@superset-ui/core'; +import { extendedDayjs as dayjs } from '@superset-ui/core/utils/dates'; +import { logging } from '@apache-superset/core'; + +const DEFAULT_LANGUAGE_PACK: LanguagePack = { + domain: 'superset', + locale_data: { + superset: { + '': { + domain: 'superset', + lang: 'en', + plural_forms: 'nplurals=2; plural=(n != 1)', + }, + }, + }, +}; + +export interface LocaleControllerOptions { + /** Initial locale code (e.g., 'en', 'ru', 'de') */ + initialLocale?: string; + /** Callback invoked when locale changes */ + onChange?: (locale: string) => void; + /** Skip initial language pack fetch (for cases when it's already loaded) */ + skipInitialFetch?: boolean; +} + +/** + * LocaleController manages application localization state reactively. + * + * This controller follows the same architectural pattern as ThemeController: + * - Owns and manages locale state + * - Notifies subscribers via onChange callbacks + * - Does NOT reload the page + * - Enables reactive updates in React via LocaleProvider + * + * Usage: + * ```typescript + * const controller = new LocaleController({ initialLocale: 'en' }); + * + * // Subscribe to changes + * controller.onChange(locale => console.log('Locale changed:', locale)); + * + * // Change locale (fetches language pack, updates state, notifies listeners) + * await controller.setLocale('ru'); + * ``` + */ +export class LocaleController { + private currentLocale: string; + + private onChangeCallbacks: Set<(locale: string) => void> = new Set(); + + private isInitialized = false; + + private pendingLocaleChange: Promise<void> | null = null; + + constructor(options: LocaleControllerOptions = {}) { + const { initialLocale = 'en', onChange, skipInitialFetch = false } = options; + + this.currentLocale = initialLocale; + + if (onChange) { + this.onChangeCallbacks.add(onChange); + } + + // If not skipping initial fetch and locale is not 'en', fetch language pack + if (!skipInitialFetch && initialLocale !== 'en') { + this.initializeLocale(initialLocale); Review Comment: **Suggestion:** The constructor starts an asynchronous locale initialization via `initializeLocale` without wiring it into `pendingLocaleChange`, so if `setLocale` is called while initialization is still in flight, the two async operations can race and the final active locale may end up being the initial one instead of the last requested locale. To fix this, ensure the initialization promise is stored in `pendingLocaleChange` so that `setLocale` waits for it before running its own update, preserving the "last call wins" behavior. [race condition] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Locale may revert from user-selected to initial language. - ❌ Locale-dependent dashboard and chart text show wrong language. - ⚠️ Embedded SDK dynamic `setLocale` calls become unreliable. - ⚠️ React listeners re-render with stale `currentLocale` value. ``` </details> ```suggestion // Track the initialization promise so subsequent setLocale calls wait for it this.pendingLocaleChange = this.initializeLocale(initialLocale); ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Instantiate `LocaleController` with a non-English initial locale and default options, e.g. `new LocaleController({ initialLocale: 'fr' })`, which triggers `initializeLocale(initialLocale)` in the constructor at `superset-frontend/src/locale/LocaleController.ts:77-89` without setting `pendingLocaleChange`. 2. Before the async `initializeLocale('fr')` call (lines 192-201) resolves its `SupersetClient.get` request (lines 220-227), call `controller.setLocale('de')` on the same instance, invoking `setLocale` at lines 130-148 while `pendingLocaleChange` is still `null`. 3. Inside `setLocale` (lines 130-148), the `if (this.pendingLocaleChange)` guard at lines 136-139 does nothing because `pendingLocaleChange` was never set by the constructor; `this.pendingLocaleChange = this.doSetLocale('de')` is then assigned at line 142, and `doSetLocale('de')` (lines 207-214) starts its own fetch and calls `applyLocale('de', ...)` (lines 236-248), setting `this.currentLocale` to `'de'`. 4. After `setLocale('de')` has completed and the UI has re-rendered in German, the original `initializeLocale('fr')` call from the constructor finally resolves; it calls `applyLocale('fr', ...)` at lines 192-201, which sets `this.currentLocale` back to `'fr'` and notifies listeners at lines 247-248, so the app ends up localized in French (the initial locale) instead of the user-requested German, demonstrating the race condition. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/locale/LocaleController.ts **Line:** 88:88 **Comment:** *Race Condition: The constructor starts an asynchronous locale initialization via `initializeLocale` without wiring it into `pendingLocaleChange`, so if `setLocale` is called while initialization is still in flight, the two async operations can race and the final active locale may end up being the initial one instead of the last requested locale. To fix this, ensure the initialization promise is stored in `pendingLocaleChange` so that `setLocale` waits for it before running its own update, preserving the "last call wins" behavior. 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=5835445543ae710bd08f19ec40d12337287a45b3f71acf39b5455046d3bfd5d9&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37790&comment_hash=5835445543ae710bd08f19ec40d12337287a45b3f71acf39b5455046d3bfd5d9&reaction=dislike'>👎</a> ########## superset/dashboards/schemas.py: ########## @@ -257,8 +273,108 @@ def post_dump(self, serialized: dict[str, Any], **kwargs: Any) -> dict[str, Any] del serialized["owners"] del serialized["changed_by_name"] del serialized["changed_by"] Review Comment: **Suggestion:** In the dashboard response post-dump hook, deleting keys from `serialized` unconditionally will raise `KeyError` if those fields were excluded (for example via `only`/`exclude` when instantiating the schema), so using `pop` with a default makes the hook robust without changing behavior when the keys are present. [possible bug] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Guest dashboard serialization can crash with restricted field sets. - ⚠️ Guest-facing dashboard APIs may intermittently return 500 errors. ``` </details> ```suggestion serialized.pop("owners", None) serialized.pop("changed_by_name", None) serialized.pop("changed_by", None) ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. In a Python shell, import `DashboardGetResponseSchema` from `superset.dashboards.schemas` (class defined at `superset/dashboards/schemas.py:228-260`). 2. Create a minimal dashboard-like object with attributes `id`, `slug`, `url`, `dashboard_title`, etc., but you do not need `owners`, `changed_by_name`, or `changed_by` attributes for this reproduction. 3. Instantiate the schema with a restricted field set so those keys are not serialized, e.g. `schema = DashboardGetResponseSchema(only=("id", "dashboard_title"))`, and ensure `security_manager.is_guest_user()` returns `True` (e.g. by configuring a guest user or monkeypatching during a test). 4. Call `schema.dump(obj)`; the `@post_dump` method at `superset/dashboards/schemas.py:262-281` executes, and because `"owners"`, `"changed_by_name"`, and `"changed_by"` are absent from `serialized`, the `del serialized["owners"]` / `del serialized["changed_by_name"]` / `del serialized["changed_by"]` statements at lines 272-275 raise `KeyError`, causing the dump (and any API using this schema) to fail. ``` </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:** 273:275 **Comment:** *Possible Bug: In the dashboard response post-dump hook, deleting keys from `serialized` unconditionally will raise `KeyError` if those fields were excluded (for example via `only`/`exclude` when instantiating the schema), so using `pop` with a default makes the hook robust without changing behavior when the keys are present. 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=9e98f7f7103ceadcad9a0209b81f8e7114d9976a80feccf47da4b2adc55439ff&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37790&comment_hash=9e98f7f7103ceadcad9a0209b81f8e7114d9976a80feccf47da4b2adc55439ff&reaction=dislike'>👎</a> ########## superset-frontend/src/explore/components/PropertiesModal/index.tsx: ########## @@ -258,6 +293,18 @@ function PropertiesModal({ // get the owners of this slice useEffect(() => { fetchChartProperties(); + if (isFeatureEnabled(FeatureFlag.EnableContentLocalization)) { + SupersetClient.get({ + endpoint: '/api/v1/localization/available_locales', + }).then( + response => { + const { locales, default_locale } = response.json.result; + setAllLocales(locales); + setDefaultLocale(default_locale); + }, + err => logging.error('Failed to fetch available locales', err), + ); + } }, [slice.slice_id]); Review Comment: **Suggestion:** The effect that loads chart properties and available locales only re-runs when the slice ID changes, even though it depends on `fetchChartProperties` (which closes over `userLocale`) and indirectly on `userLocale` itself; if the user's locale changes while the Properties modal is open, the modal will keep showing translations for the old locale and never refetch, which is a stale-state logic bug and will also be flagged by the React hooks linter. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ React hooks linter flags stale dependency in PropertiesModal effect. - ⚠️ Translations not refreshed if user locale changes mid-session. ``` </details> ```suggestion }, [slice.slice_id, fetchChartProperties, userLocale]); ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Run the frontend linting task (or CI) that includes the React hooks ESLint plugin over `superset-frontend/src/explore/components/PropertiesModal/index.tsx`. 2. ESLint inspects the `useEffect` at lines 293–308, which uses the `fetchChartProperties` callback but has a dependency array of `[slice.slice_id]`. 3. Since `fetchChartProperties` is defined with `useCallback` at lines ~167–215 and depends on `userLocale` (via its dependency list `[showError, slice.slice_id, userLocale]`), the `react-hooks/exhaustive-deps` rule reports a missing dependency warning for this effect. 4. At runtime, if anything in the app updates `state.common.locale` (the Redux value read at lines 60–62 via `useSelector`), `fetchChartProperties` is recreated with the new `userLocale`, but the `useEffect` at 293–308 does not re-run (because its dependencies exclude `fetchChartProperties` and `userLocale`), so the modal keeps whatever translations and active locale state were loaded for the previous locale, demonstrating stale state if live locale switching is supported. ``` </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/index.tsx **Line:** 308:308 **Comment:** *Logic Error: The effect that loads chart properties and available locales only re-runs when the slice ID changes, even though it depends on `fetchChartProperties` (which closes over `userLocale`) and indirectly on `userLocale` itself; if the user's locale changes while the Properties modal is open, the modal will keep showing translations for the old locale and never refetch, which is a stale-state logic bug and will also be flagged by the React hooks linter. 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=0d6ff9b5623af59cb69ac714a87628bad5175964caffd95733c9a42eb47d390e&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37790&comment_hash=0d6ff9b5623af59cb69ac714a87628bad5175964caffd95733c9a42eb47d390e&reaction=dislike'>👎</a> ########## superset-frontend/src/explore/components/controls/TranslatableTextControl/index.tsx: ########## @@ -0,0 +1,294 @@ +/** + * 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 { + ChangeEvent, + useCallback, + useEffect, + useMemo, + useRef, + useState, +} from 'react'; +import { useSelector } from 'react-redux'; +import { logging } from '@apache-superset/core'; +import { isFeatureEnabled, FeatureFlag } from '@superset-ui/core'; +import { Constants, Input } from '@superset-ui/core/components'; +import { SupersetClient } from '@superset-ui/core/connection'; +import { debounce } from 'lodash'; +import ControlHeader from '../../ControlHeader'; +import TextControl from '../TextControl'; +import TranslationInput from 'src/components/TranslationInput'; +import { + LocaleSwitcher, + DEFAULT_LOCALE_KEY, +} from 'src/components/TranslationEditor'; +import type { Translations, LocaleInfo } from 'src/types/Localization'; +import type { ExploreActions } from 'src/explore/actions/exploreActions'; +import type { QueryFormData } from '@superset-ui/core'; + +export interface TranslatableTextControlProps { + name: string; + label?: string; + description?: string; + placeholder?: string; + value?: string | null; + disabled?: boolean; + onChange?: (value: string, errors: any[]) => void; + actions?: Partial<ExploreActions> & Pick<ExploreActions, 'setControlValue'>; + formData?: QueryFormData | null; + renderTrigger?: boolean; + [key: string]: unknown; +} + +export default function TranslatableTextControl({ + name, + label, + value, + onChange, + actions, + formData, + placeholder, + disabled, + ...rest +}: TranslatableTextControlProps) { + const localizationEnabled = isFeatureEnabled( + FeatureFlag.EnableContentLocalization, + ); + + // --- Locale state (hooks always called, effects gated by flag) --- + const [allLocales, setAllLocales] = useState<LocaleInfo[]>([]); + const [defaultLocale, setDefaultLocale] = useState(''); + const userLocale: string = useSelector( + (state: { common: { locale: string } }) => state.common.locale, + ); + const [activeLocale, setActiveLocale] = useState(DEFAULT_LOCALE_KEY); + const [localesLoaded, setLocalesLoaded] = useState(false); + + useEffect(() => { + if (!localizationEnabled) return; + SupersetClient.get({ + endpoint: '/api/v1/localization/available_locales', + }).then( + response => { + const { locales, default_locale } = response.json.result; + setAllLocales(locales); + setDefaultLocale(default_locale); + setLocalesLoaded(true); + }, + err => logging.error('Failed to fetch available locales', err), + ); + }, [localizationEnabled]); + + const translations: Translations = (formData as Record<string, unknown>) + ?.translations as Translations ?? {}; + const fieldTranslations = translations[name] ?? {}; + + const translationsRef = useRef(translations); + translationsRef.current = translations; + const fieldTranslationsRef = useRef(fieldTranslations); + fieldTranslationsRef.current = fieldTranslations; + + useEffect(() => { + if (!localizationEnabled || !localesLoaded || !userLocale) return; + if (fieldTranslationsRef.current[userLocale]) { + setActiveLocale(userLocale); + } else { + setActiveLocale(DEFAULT_LOCALE_KEY); + } + }, [localizationEnabled, localesLoaded, userLocale]); + + // --- Feature flag off → plain TextControl --- + if (!localizationEnabled) { + return ( + <TextControl + name={name} + label={label} + value={value} + onChange={onChange} + placeholder={placeholder} + disabled={disabled} + {...rest} + /> + ); + } + + const isLocaleMode = activeLocale !== DEFAULT_LOCALE_KEY; + const defaultValue = String(value ?? ''); + + const localeSwitcher = + allLocales.length > 0 ? ( + <LocaleSwitcher + fieldName={name} + defaultValue={defaultValue} + translations={translations} + allLocales={allLocales} + defaultLocale={defaultLocale} + userLocale={userLocale} + activeLocale={activeLocale} + onLocaleChange={setActiveLocale} + fieldLabel={String(label ?? name)} + /> + ) : null; + + if (isLocaleMode) { + return ( + <TranslatableTextControlLocaleMode + name={name} + label={label} + translationValue={fieldTranslations[activeLocale] ?? ''} + placeholder={defaultValue} + disabled={disabled} + suffix={localeSwitcher} + activeLocale={activeLocale} + translationsRef={translationsRef} + fieldTranslationsRef={fieldTranslationsRef} + setControlValue={actions?.setControlValue} + controlHeaderProps={rest} + /> + ); + } + + return ( + <TranslatableTextControlDefaultMode + name={name} + label={label} + value={defaultValue} + placeholder={placeholder} + disabled={disabled} + suffix={localeSwitcher} + onChange={onChange} + controlHeaderProps={rest} + /> + ); +} + +// --- Default locale mode: Input with debounced onChange --- +function TranslatableTextControlDefaultMode({ + name, + label, + value, + placeholder, + disabled, + suffix, + onChange, + controlHeaderProps, +}: { + name: string; + label?: string; + value: string; + placeholder?: string; + disabled?: boolean; + suffix: React.ReactNode; + onChange?: (value: string, errors: any[]) => void; + controlHeaderProps: Record<string, unknown>; +}) { + const [localValue, setLocalValue] = useState(value); + const prevValueRef = useRef(value); + + if (prevValueRef.current !== value) { + prevValueRef.current = value; + } + const displayValue = + prevValueRef.current !== value ? value : localValue; + + const debouncedOnChange = useMemo( + () => + debounce((val: string) => { + onChange?.(val, []); + }, Constants.FAST_DEBOUNCE), + [onChange], + ); + + const handleChange = useCallback( + (e: ChangeEvent<HTMLInputElement>) => { + setLocalValue(e.target.value); + debouncedOnChange(e.target.value); + }, + [debouncedOnChange], + ); + + return ( + <div> + <ControlHeader name={name} label={label} {...controlHeaderProps} /> + <Input + type="text" + data-test="inline-name" + placeholder={placeholder} + onChange={handleChange} + value={displayValue} Review Comment: **Suggestion:** The default-mode text control keeps its own `localValue` state but never updates it when the `value` prop changes; instead it uses a `prevValueRef`/`displayValue` check that always resolves to `localValue`, so external updates (e.g., reset from parent form) are ignored and the input shows stale text. Replace this with a `useEffect` that synchronizes `localValue` whenever `value` changes and bind the input directly to `localValue`. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Default-mode TranslatableTextControl ignores parent updates to value prop. - ⚠️ Explore form controls can display stale values after programmatic changes. ``` </details> ```suggestion useEffect(() => { setLocalValue(value); }, [value]); const debouncedOnChange = useMemo( () => debounce((val: string) => { onChange?.(val, []); }, Constants.FAST_DEBOUNCE), [onChange], ); const handleChange = useCallback( (e: ChangeEvent<HTMLInputElement>) => { setLocalValue(e.target.value); debouncedOnChange(e.target.value); }, [debouncedOnChange], ); return ( <div> <ControlHeader name={name} label={label} {...controlHeaderProps} /> <Input type="text" data-test="inline-name" placeholder={placeholder} onChange={handleChange} value={localValue} ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Ensure `FeatureFlag.EnableContentLocalization` is enabled so `TranslatableTextControl` follows the localization path and reaches `TranslatableTextControlDefaultMode` when `activeLocale === DEFAULT_LOCALE_KEY` in `superset-frontend/src/explore/components/controls/TranslatableTextControl/index.tsx` (around lines 166–177). 2. Render `TranslatableTextControl` with `value="First"` and `formData` such that `translations[name]` is empty, causing `isLocaleMode` to be false and `TranslatableTextControlDefaultMode` (lines 180–240) to be used; the inner state `localValue` is initialized to `"First"` at line 200. 3. Later, update the parent state so that `TranslatableTextControl` is re-rendered with `value="Second"` while the component instance remains mounted (no unmount/remount), a normal React pattern for programmatic control resets or external updates of form values. 4. On re-render, in `TranslatableTextControlDefaultMode` (lines 200–207), `prevValueRef.current` is immediately set to the new `value` before computing `displayValue`, so `prevValueRef.current !== value` is false and `displayValue` resolves to the stale `localValue` (`"First"`); the `<Input>` at lines 225–237 continues to show `"First"` instead of `"Second"`, demonstrating that external updates to the `value` prop are ignored. ``` </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/controls/TranslatableTextControl/index.tsx **Line:** 201:233 **Comment:** *Logic Error: The default-mode text control keeps its own `localValue` state but never updates it when the `value` prop changes; instead it uses a `prevValueRef`/`displayValue` check that always resolves to `localValue`, so external updates (e.g., reset from parent form) are ignored and the input shows stale text. Replace this with a `useEffect` that synchronizes `localValue` whenever `value` changes and bind the input directly to `localValue`. 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=78f1a478cf2185106b645903e2e15404e0ba931804c684ccd73ea75019c7307f&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37790&comment_hash=78f1a478cf2185106b645903e2e15404e0ba931804c684ccd73ea75019c7307f&reaction=dislike'>👎</a> ########## superset-frontend/src/locale/LocaleProvider.tsx: ########## @@ -0,0 +1,180 @@ +/** + * 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 { + createContext, + useCallback, + useContext, + useEffect, + useMemo, + useState, +} from 'react'; +import { LocaleController } from './LocaleController'; + +/** + * Locale context value provided to consumers. + */ +export interface LocaleContextType { + /** Current locale code (e.g., 'en', 'ru', 'de') */ + locale: string; + /** Whether a locale change is in progress */ + isLoading: boolean; + /** Whether the locale controller has been initialized */ + isReady: boolean; + /** Change the application locale */ + setLocale: (locale: string) => Promise<void>; + /** Translate a string using the current locale */ + t: (key: string, ...args: unknown[]) => string; + /** Translate a string with pluralization */ + tn: (key: string, ...args: unknown[]) => string; +} + +const LocaleContext = createContext<LocaleContextType | null>(null); + +interface LocaleProviderProps { + children: React.ReactNode; + controller: LocaleController; +} + +/** + * LocaleProvider integrates LocaleController with React. + * + * This provider follows the same pattern as SupersetThemeProvider: + * - Subscribes to controller changes via useEffect + * - Triggers re-renders when locale changes + * - Provides context for child components + * + * Usage: + * ```tsx + * const controller = new LocaleController({ initialLocale: 'en' }); + * + * function App() { + * return ( + * <LocaleProvider controller={controller}> + * <MyComponent /> + * </LocaleProvider> + * ); + * } + * + * function MyComponent() { + * const { locale, setLocale, t } = useLocaleContext(); + * return <div>{t('Hello')}</div>; + * } + * ``` + */ +export function LocaleProvider({ + children, + controller, +}: LocaleProviderProps): JSX.Element { + const [locale, setLocaleState] = useState<string>(controller.getLocale()); + const [isLoading, setIsLoading] = useState<boolean>(false); + const [isReady, setIsReady] = useState<boolean>(controller.isReady()); + + // Subscribe to controller changes + useEffect(() => { + const unsubscribe = controller.onChange(newLocale => { + setLocaleState(newLocale); + setIsLoading(false); + setIsReady(controller.isReady()); + }); + + // Update isReady state periodically until controller is initialized + // This handles the case where controller is initializing asynchronously + if (!controller.isReady()) { + const checkReady = setInterval(() => { + if (controller.isReady()) { + setIsReady(true); + clearInterval(checkReady); + } + }, 50); + + return () => { + unsubscribe(); + clearInterval(checkReady); + }; + } + + return unsubscribe; + }, [controller]); + + const setLocale = useCallback( + async (newLocale: string): Promise<void> => { + setIsLoading(true); + try { + await controller.setLocale(newLocale); + } catch (error) { + setIsLoading(false); + throw error; + } Review Comment: **Suggestion:** The `setLocale` callback sets `isLoading` to true before calling `controller.setLocale` but only resets it in the `catch` path, so if `setLocale` resolves without error and does not trigger an `onChange` (for example when called with the current locale), `isLoading` remains stuck as true and the UI will show an infinite loading state. Move the `setIsLoading(false)` into a `finally` block so it always runs regardless of success, failure, or whether callbacks fire. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ LocaleSwitcher UI may show perpetual loading indicator. - ⚠️ Components gating render on isLoading may never settle. - ⚠️ Embedded localization flows can misreport request completion state. ``` </details> ```suggestion } finally { setIsLoading(false); ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Render `LocaleProvider` from `superset-frontend/src/locale/LocaleProvider.tsx:82-155` with a real `LocaleController` instance imported from `./LocaleController`. 2. Inside any child component wrapped by `LocaleProvider`, obtain `setLocale` from `useLocaleContext()` defined at `LocaleProvider.tsx:163-170` and call `setLocale(currentLocale)` where `currentLocale` equals `controller.getLocale()`. 3. The `setLocale` implementation at `LocaleProvider.tsx:117-128` sets `isLoading` to `true` and awaits `controller.setLocale(newLocale)`; since this is a no-op locale change, `LocaleController` typically short-circuits and does not fire the `onChange` callback registered in the `useEffect` subscription at `LocaleProvider.tsx:90-95`. 4. Because the `catch` block at `LocaleProvider.tsx:121-124` is not entered (no error thrown) and the `onChange` subscription is not invoked, there is no code path that calls `setIsLoading(false)`, leaving `isLoading` stuck `true` for any consumers relying on that flag. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/locale/LocaleProvider.tsx **Line:** 122:124 **Comment:** *Logic Error: The `setLocale` callback sets `isLoading` to true before calling `controller.setLocale` but only resets it in the `catch` path, so if `setLocale` resolves without error and does not trigger an `onChange` (for example when called with the current locale), `isLoading` remains stuck as true and the UI will show an infinite loading state. Move the `setIsLoading(false)` into a `finally` block so it always runs regardless of success, failure, or whether callbacks fire. 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=c55b51e5c96f37aafd253f9c7dbf33e77ee7f0d585416908fa9577c18776ec60&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37790&comment_hash=c55b51e5c96f37aafd253f9c7dbf33e77ee7f0d585416908fa9577c18776ec60&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]
