YuriyKrasilnikov commented on PR #37790:
URL: https://github.com/apache/superset/pull/37790#issuecomment-3919062103
## Response to CodeAnt AI reviews (Feb 17 + Feb 18)
Verified all 16 findings against the actual code. 5 were real bugs and have
been fixed in `90614091ba`. 11 are false positives.
---
### Fixed (5) — commit `90614091ba`
**1. TranslatableTextControl `prevValueRef` broken pattern**
(`index.tsx:201-207`)
Real bug. `prevValueRef.current` is updated on line 204 BEFORE the
`displayValue` check on line 206, so `prevValueRef.current !== value` is always
`false` after the ref update → `displayValue` always equals `localValue` →
external value changes (undo, chart switch) are ignored.
Replaced with `useEffect` sync — the established Superset pattern for
functional components (`BoundsControl.tsx:79-81`).
**2. LocaleSwitcher keyboard toggle bypassing `handleOpenChange`**
(`LocaleSwitcher.tsx:233`)
Real bug. `setDropdownOpen(prev => !prev)` bypasses `handleOpenChange`, so
`onDropdownOpenChange` callback never fires for keyboard users. Two consumers
rely on this callback for focus management: `AdhocMetricEditPopoverTitle` (uses
`dropdownOpenRef` to track state) and `DndColumnSelectPopoverTitle` (calls
`inputRef.current?.focus()` on close).
One-line fix: `handleOpenChange(!dropdownOpen)` — same handler for mouse and
keyboard, matching Superset's `DrillBySubmenu` pattern.
**3. LocaleProvider `isLoading` stuck on success**
(`LocaleProvider.tsx:122-124`)
Real bug. `setIsLoading(true)` at line 118, but only reset in the `catch`
path. When `controller.setLocale(currentLocale)` → controller early returns
(line 132-134 in LocaleController) → `onChange` not fired → `isLoading` stuck
`true`.
Changed `catch` to `finally` — the dominant Superset pattern (3 of 4 loading
state examples: `DrillDetailPane`, `SamplesPane`, `UploadDataModel`).
**4. LocaleController `initializeLocale` race condition**
(`LocaleController.ts:88`)
Real bug. Constructor fires `this.initializeLocale(initialLocale)` without
storing the promise in `pendingLocaleChange`. If `setLocale()` is called during
init, the guard at line 137 (`if (this.pendingLocaleChange)`) does nothing →
two concurrent fetches race → locale can revert to initial.
One-line fix: `this.pendingLocaleChange =
this.initializeLocale(initialLocale)` — leverages the existing dedup mechanism.
Same pattern as `preamble.ts` storing init promises.
**5. `schema_mixin` `translations=null` validation** (`schema_mixin.py:69`)
Real bug (partial). The check `if "translations" not in data` misses the
case where `translations` is explicitly `null` — Marshmallow passes `None` as
the value when `allow_none=True`. The feature flag check at line 72 would block
`null` when the feature is disabled, even though `null` means "no translations."
However, the crash claim is incorrect — `validate_translations(None)`
handles `None` safely (early return at `validation.py:128-129`).
Fixed: `if "translations" not in data or data["translations"] is None:
return`
---
### False positives (11)
**6. `getLocalizedFormDataValue` empty string truthy check**
(`getLocalizedFormDataValue.ts:41`)
Not a bug. Empty string is not a valid translation value — the entire
pipeline enforces this:
- Frontend: `stripEmptyValues()` (`utils.ts:30-44`) removes empty strings
before API call
- Backend: `validate_translations()` (`validation.py:155`) enforces
`min_length=1`
- Backend: `sanitize_translations()` filters empty values after HTML
stripping
The truthy check is intentional and consistent across
`getLocalizedFormDataValue`, `getLocalizedValue`, `countFieldTranslations`, and
`buildLocalizedMetricLabelMap`.
**7 + 13. MixedTimeseries `yAxisTitleSecondary` key casing**
(`transformProps.ts:245`) — flagged twice across two reviews
Not a bug. The suggestion assumes translations are keyed by snake_case
(`y_axis_title_secondary`), but that's incorrect.
The control panel defines `name: 'yAxisTitleSecondary'` (camelCase) at
`controlPanel.tsx:486`. `convertKeysToCamelCase`
(`convertKeysToCamelCase.ts:22-33`) is **shallow** — it uses `mapKeys` on
top-level formData keys only. The `translations` dict is a nested value, so its
internal keys are NOT converted. Both storage (TranslatableTextControl writes
to `translations[control.name]`) and lookup
(`getLocalizedFormDataValue(translations, 'yAxisTitleSecondary')`) use the same
camelCase key from the control's `name` property.
**8. `countFieldTranslations` counting `DEFAULT_LOCALE_KEY`** (`utils.ts:53`)
Not a bug. `DEFAULT_LOCALE_KEY = 'default'` is a UI-only sentinel used in
LocaleSwitcher to represent "editing the default column value." It is never
stored in the `translations` JSON column in the database. The database format
is `{field: {locale_code: value}}` where locale codes are real codes like
`"en"`, `"de"`, `"ru"`. The `default` key cannot appear in the translations map
— `stripEmptyValues()` at save time and `validate_translations()` at API level
both operate on real locale codes only.
**9 + 15. Object spread `undefined`** — FiltersConfigForm
(`FiltersConfigForm.tsx:644`) + DndColumnSelectPopoverTitle
(`DndColumnSelectPopoverTitle.tsx:160`) — flagged twice across two reviews
Not a bug. Object spread with `undefined` is explicitly safe per ECMAScript
spec. **CopyDataProperties** (ECMA-262, §14.18) step 3: if source is
`undefined` or `null`, return target. `({ ...undefined, de: "German" })` → `{
de: "German" }`.
Pre-existing Superset code uses the same pattern:
`useStoredSidebarWidth.ts:30,45` (Jul 2022) — `useRef<Record<string,
number>>()` with no initial value, then `{ ...widthsMapRef.current, [id]:
updatedWidth }` where `.current` can be `undefined`.
The nested level in both components already has an explicit guard (`?.label
?? {}`, `?.name ?? {}`), showing awareness of the undefined case — handled
where the spec does NOT provide safety (property access on undefined).
**10. PropertiesModal `useEffect` stale dependency**
(`PropertiesModal/index.tsx:308`)
Not a bug. The effect at line 294 intentionally loads chart data once when
the modal opens (on `slice.slice_id` change). `userLocale` comes from Redux
`state.common.locale` which is set in `preamble.ts` at app init and does not
change during a session — changing locale in Superset requires a page reload.
Adding `fetchChartProperties` to the deps would cause unnecessary re-fetches
for a value that is effectively constant. The code passes
`react-hooks/exhaustive-deps` in CI.
**11. `dashboards/schemas.py` `del` → `pop` for guest user fields**
(`schemas.py:273-275`)
Pre-existing code — authored in commit `386d4e054` (Dec 2023), not part of
this PR. The fields are always present in the serialized dict:
- `owners`: SQLAlchemy `relationship()` (line 151 in `dashboard.py`) —
always exists
- `changed_by_name`: `@property` (line 260) — always returns `str`
- `changed_by`: inherited from `AuditMixin` — always exists
Marshmallow `@post_dump` includes all declared schema fields when the source
object attributes exist. `KeyError` is impossible. The scenario of
instantiating with `only=("id", "dashboard_title")` doesn't happen — this
schema is used exclusively by the dashboard REST API at full field set.
**12. SaveModal.tsx name overwrite race condition** (`SaveModal.tsx:176-178`)
Not a practical issue. The theoretical race window is ~300-700ms (awaiting
`loadDashboard` + `loadTabs` + translations fetch). The pre-existing code in
the same `componentDidMount` (lines 150-157, commit `386d4e054`, Dec 2023) has
the identical pattern for the `dashboard` field — `setState` after async fetch
without loading gate.
The name field already shows the correct value from `props.sliceName` at
mount. The API response replaces it with the original (non-localized) name from
`include_translations=true` — this is intentional for the editor mode.
`chart.slice_name || this.state.newSliceName` also preserves user input if the
API returns empty.
**14. LocaleSwitcher warning for regional locales**
(`LocaleSwitcher.tsx:83-84`)
Not a bug. The scenario requires a hyphen-format locale like `de-AT`, but
Superset `LANGUAGES` config (`config.py:421-440`) uses base codes and
underscore variants only: `en`, `de`, `pt_BR`, `zh_TW`, etc. The
`session["locale"]` value (`initialization/__init__.py:994`) is always a key
from `LANGUAGES` — never hyphen-separated.
More importantly, the proposed fix would make things **worse**. The
LocaleSwitcher warning correctly reflects what the user actually sees: both
`LocaleSwitcher` (exact match) and `getLocalizedValue` (`split('-')` only)
behave identically for underscore-separated locales — `'pt_BR'.split('-')[0]` =
`'pt_BR'` (no fallback). Adding base-language fallback only to LocaleSwitcher
would make the warning say "translation exists" while the rendered text shows
the default value.
The backend `get_translation()` (`locale_utils.py:123`) handles both
separators (`for sep in ("-", "_")`), but this is a separate backend-frontend
consistency question, not a LocaleSwitcher bug.
**16. `sanitize_translations(None)` crash in `@post_load`**
(`schema_mixin.py:91`)
Not a bug. `sanitize_translations()` explicitly accepts `None`:
1. Type signature (`sanitization.py:64-66`): `translations: dict[str,
dict[str, str]] | None`
2. Guard clause (`sanitization.py:85-86`): `if translations is None: return
None`
3. Docstring example (`sanitization.py:82-83`): `>>>
sanitize_translations(None)` → `None`
4. Dedicated test (`xss_sanitization_test.py:119-121`): `assert
sanitize_translations(None) is None`
The call chain is: Marshmallow deserializes `"translations": null` →
`data["translations"] = None` → `@post_load` calls
`sanitize_translations(None)` → returns `None` → `data["translations"]` stays
`None`. No TypeError possible.
--
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]
