You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "alex-poor (via GitHub)" <gi...@apache.org> on 2026/04/15 00:13:35 UTC
[PR] fix(i18n): inject language pack into bootstrap to translate code-split chunks [superset]
alex-poor opened a new pull request, #39357:
URL: https://github.com/apache/superset/pull/39357
### SUMMARY
Fixes #35330. Module-level `const X = t('...')` calls in code-split chunks evaluate before the async `/superset/language_pack/<lang>/` fetch in `preamble.ts` can resolve, capturing the English msgid forever. #36893 fixed the initial-render race by awaiting `initPreamble()` before `ReactDOM.render`, but lazily-loaded chunks (e.g. `DashboardContainer` → `ChartContextMenu` → `useDrillDetailMenuItems`) still evaluate later and hit an unconfigured translator. Result: strings inside component render bodies translate, but strings captured in module-scope constants stay English, even though `bootstrapData.common.locale` is set and the language pack endpoint returns a valid pack. Same symptom reported on 5.0.0 / 6.0.0 for pt_BR, ru, zh in #35330 and linked issues.
**Fix**: inject the full Jed language pack into the HTML bootstrap payload under `common.language_pack`. An inline `<script>` in `spa.html` — which runs before the entry bundle loads — stashes the pack on `window.__SUPERSET_LANGUAGE_PACK__`. `TranslatorSingleton` lazily self-configures from that global on first `getInstance()`, so every chunk-local copy that webpack `splitChunks` may produce ends up configured identically. `preamble.ts` now prefers the bootstrap-injected pack and keeps the async fetch as fallback for entries that don't extend `spa.html` (embedded, legacy).
Net effect: `t()` returns the translated msgstr on first evaluation everywhere, including module-level constants in lazy chunks.
Files changed:
- `superset/views/base.py` — read `translations/<lang>/LC_MESSAGES/messages.json` into `common.language_pack` (skipped for `en`, silently absent if the file isn't present, respects the existing `cached_common_bootstrap_data` memoization).
- `superset/templates/superset/spa.html` — inline script that parses the bootstrap div and sets `window.__SUPERSET_LANGUAGE_PACK__` before `js_bundle(entry)`.
- `superset-frontend/packages/superset-core/src/translation/TranslatorSingleton.ts` — `autoConfigureFromWindow()` called from `getInstance()` to self-configure from the global.
- `superset-frontend/src/preamble.ts` — prefer the injected pack; keep async fetch as fallback.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Tested on a live deployment with `mi` (Māori) locale. Before: navbar translated, chart context menu items ("Drill to detail", etc.) and many other module-scope strings stayed English. After: every string with a msgstr renders translated on first load.
### TESTING INSTRUCTIONS
1. Build with `BUILD_TRANSLATIONS=true` so `messages.json` files exist on disk.
2. Log in, switch locale to any non-English language that has translations (e.g. `pt_BR`, `ru`, `mi`).
3. Open a dashboard, right-click on a chart. Context menu items ("Drill to detail", "Drill to detail by") should render translated.
4. Check browser console: `JSON.parse(document.getElementById('app').dataset.bootstrap).common.language_pack` should return the Jed dict, and `window.__SUPERSET_LANGUAGE_PACK__` should be the same object.
5. Switch back to English — `language_pack` should be `null` and the async fetch path should not run.
### ADDITIONAL INFORMATION
- [x] Has associated issue: #35330
- [ ] Required feature flags:
- [x] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(i18n): inject language pack into bootstrap to translate code-split chunks [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #39357:
URL: https://github.com/apache/superset/pull/39357#discussion_r3190432361
##########
superset/views/base.py:
##########
@@ -436,6 +436,34 @@ def get_default_spinner_svg() -> str | None:
return None
+def _load_language_pack(language: str | None) -> dict[str, Any] | None:
+ """Read the Jed language pack for ``language`` off disk.
+
+ Returns ``None`` for English, when the JSON file is missing, or when
+ it can't be parsed. The result is injected into the bootstrap payload
+ so the frontend can configure the translator synchronously before any
+ code-split chunk evaluates a module-level ``const X = t('...')``
+ (see upstream issue #35330).
+ """
+ if not language or language == "en":
+ return None
+ pack_path = os.path.join(
+ os.path.dirname(__file__),
+ "..",
+ "translations",
+ language,
+ "LC_MESSAGES",
+ "messages.json",
+ )
+ if not os.path.isfile(pack_path):
+ return None
+ try:
+ with open(pack_path, encoding="utf-8") as fp:
+ return cast(dict[str, Any], json.loads(fp.read()))
+ except (OSError, ValueError):
+ return None
Review Comment:
**Suggestion:** This introduces a second language-pack loader instead of using the existing shared utility (`superset.translations.utils.get_language_pack`) that already provides process-level caching and consistent behavior. Because `cached_common_bootstrap_data` is keyed per user, this new direct file read path can repeatedly hit disk for the same locale across many users and diverge from the established translation-loading flow. Reuse the existing utility to avoid repeated I/O and keep bootstrap/endpoint language-pack semantics aligned. [performance]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Extra disk reads per user for non-English locales.
- ⚠️ Bootstrap language loader duplicates translation utils' caching logic.
- ⚠️ Minor latency under heavy localized traffic due to I/O.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Hit any SPA endpoint that uses BaseSupersetView.render_app_template(), such as SQL Lab
(`/superset/sqllab/`), where `render_app_template` in `superset/views/base.py:1–24` calls
`get_spa_template_context`, which in turn calls `get_spa_payload`
(`superset/views/base.py:595–611`) and then `common_bootstrap_payload`
(`superset/views/base.py:159–163`).
2. In `common_bootstrap_payload`, `cached_common_bootstrap_data(utils.get_user_id(),
locale_str)` is invoked (`superset/views/base.py:159–163`), and the function is memoized
with `@cache_manager.cache.memoize(timeout=60)` at `superset/views/base.py:248–256`,
meaning the cache key includes `user_id` and `locale_str`, so each user with the same
locale gets a separate cached entry.
3. For each (user_id, locale) combination with a non-English language,
`cached_common_bootstrap_data` normalizes the locale to `language`
(`superset/views/base.py:78–89`) and then calls `_load_language_pack(language)` at
`superset/views/base.py:126–133`, which constructs a path under
`superset/translations/<language>/LC_MESSAGES/messages.json`
(`superset/views/base.py:231–237`) and reads the file from disk with `open()` when it
exists (`superset/views/base.py:241–243`), performing one disk read per distinct user and
locale every cache window.
4. The existing shared utility `get_language_pack(locale)` in
`superset/translations/utils.py:7–30` already loads the same JSON pack and caches it in
the process-wide `ALL_LANGUAGE_PACKS` dict (`superset/translations/utils.py:2, 15–25`),
avoiding repeat disk I/O per locale; because `_load_language_pack` bypasses this utility
and is invoked on a high-traffic bootstrap path, deployments with multiple users sharing
the same non-English locale will incur redundant file reads and potentially divergent
behavior between the bootstrap path and any future code that relies on
`get_language_pack`.
```
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset%2Fviews%2Fbase.py%0A%2A%2ALine%3A%2A%2A%20439%3A464%0A%2A%2AComment%3A%2A%2A%0A%09%2APerformance%3A%20This%20introduces%20a%20second%20language-pack%20loader%20instead%20of%20using%20the%20existing%20shared%20utility%20%28%60superset.translations.utils.get_language_pack%60%29%20that%20already%20provides%20process-level%20caching%20and%20consistent%20behavior.%20Because%20%60cached_common_bootstrap_data%60%20is%20keyed%20per%20user%2C%20this%20new%20direct%20file%20read%20path%20can%20repeatedly%20hit%20disk%20for%20the%20same%20locale%20across%20many%20users%20and%20diverge%20from%20the%20established%20translation-loading%20flow.%20Reuse%20the%20existing%20utility%20to%20avoid%20repeated%20I%2FO%20and%20keep%20bootstrap%2Fendpoint%20language-pack%20semantics%20aligned.%0A%0AValidate%20the%20correctness%20of%20the%20fl
agged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset%2Fviews%2Fbase.py%0A%2A%2ALine%3A%2A%2A%20439%3A464%0A%2A%2AComment%3A%2A%2A%0A%09%2APerformance%3A%20This%20introduces%20a%20second%20language-pack%20loader%20instead%20of%20using%20the%20existing%20shared%20utility%20%28%60superset.translations.utils.get_language_pack%60%29%20that%20already%20provides%20process-level%20cac
hing%20and%20consistent%20behavior.%20Because%20%60cached_common_bootstrap_data%60%20is%20keyed%20per%20user%2C%20this%20new%20direct%20file%20read%20path%20can%20repeatedly%20hit%20disk%20for%20the%20same%20locale%20across%20many%20users%20and%20diverge%20from%20the%20established%20translation-loading%20flow.%20Reuse%20the%20existing%20utility%20to%20avoid%20repeated%20I%2FO%20and%20keep%20bootstrap%2Fendpoint%20language-pack%20semantics%20aligned.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/views/base.py
**Line:** 439:464
**Comment:**
*Performance: This introduces a second language-pack loader instead of using the existing shared utility (`superset.translations.utils.get_language_pack`) that already provides process-level caching and consistent behavior. Because `cached_common_bootstrap_data` is keyed per user, this new direct file read path can repeatedly hit disk for the same locale across many users and diverge from the established translation-loading flow. Reuse the existing utility to avoid repeated I/O and keep bootstrap/endpoint language-pack semantics aligned.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39357&comment_hash=1a45d5e6f9eef23e5ada11a224a4724fdaefc8840850b37a180272cd1437d743&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39357&comment_hash=1a45d5e6f9eef23e5ada11a224a4724fdaefc8840850b37a180272cd1437d743&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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(i18n): inject language pack into bootstrap to translate code-split chunks [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas merged PR #39357:
URL: https://github.com/apache/superset/pull/39357
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(i18n): inject language pack into bootstrap to translate code-split chunks [superset]
Posted by "alex-poor (via GitHub)" <gi...@apache.org>.
alex-poor commented on code in PR #39357:
URL: https://github.com/apache/superset/pull/39357#discussion_r3440995561
##########
superset/templates/superset/spa.html:
##########
@@ -145,6 +145,29 @@
{% endblock %}
{% block tail_js %}
+ {#
+ Expose the language pack on window BEFORE the entry bundle loads,
+ so module-level `const X = t('...')` calls across webpack-split
+ chunks all find a configured translator on first access. The
+ bootstrap div is the single source of truth; we parse it once and
+ stash the pack on a global that the translator reads lazily.
+ See upstream issue #35330.
+ #}
+ <script nonce="{{ macros.get_nonce() }}">
Review Comment:
Thanks — I've added a comment in the template block ([spa.html#L148](https://github.com/apache/superset/blob/fix/i18n-bootstrap-injection/superset/templates/superset/spa.html#L148)) spelling out the rationale,
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(i18n): inject language pack into bootstrap to translate code-split chunks [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #39357:
URL: https://github.com/apache/superset/pull/39357#issuecomment-4248215022
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/39357?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `11.11111%` with `8 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 55.38%. Comparing base ([`3e25f02`](https://app.codecov.io/gh/apache/superset/commit/3e25f02da981bd6d8e14d024bf77b80c7df071f6?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`29327b6`](https://app.codecov.io/gh/apache/superset/commit/29327b615d3c95ae5752f5cb547a460250b52aee?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/39357?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset/views/base.py](https://app.codecov.io/gh/apache/superset/pull/39357?src=pr&el=tree&filepath=superset%2Fviews%2Fbase.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvdmlld3MvYmFzZS5weQ==) | 11.11% | [7 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/superset/pull/39357?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
:x: Your project check has failed because the head coverage (99.81%) is below the target coverage (100.00%). You can increase the head coverage or adjust the [target](https://docs.codecov.com/docs/commit-status#target) coverage.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #39357 +/- ##
==========================================
- Coverage 64.45% 55.38% -9.07%
==========================================
Files 2555 2555
Lines 132721 132716 -5
Branches 30802 30803 +1
==========================================
- Hits 85539 73501 -12038
- Misses 45696 58563 +12867
+ Partials 1486 652 -834
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/39357/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/39357/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `39.95% <11.11%> (-0.01%)` | :arrow_down: |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/39357/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/39357/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/39357/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `41.74% <11.11%> (-0.01%)` | :arrow_down: |
| [python](https://app.codecov.io/gh/apache/superset/pull/39357/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `41.82% <11.11%> (-20.45%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/39357/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/39357/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/superset/pull/39357?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(i18n): inject language pack into bootstrap to translate code-split chunks [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #39357:
URL: https://github.com/apache/superset/pull/39357#issuecomment-4248194462
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(i18n): inject language pack into bootstrap to translate code-split chunks [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #39357:
URL: https://github.com/apache/superset/pull/39357#discussion_r3440493444
##########
superset/templates/superset/spa.html:
##########
@@ -145,6 +145,29 @@
{% endblock %}
{% block tail_js %}
+ {#
+ Expose the language pack on window BEFORE the entry bundle loads,
+ so module-level `const X = t('...')` calls across webpack-split
+ chunks all find a configured translator on first access. The
+ bootstrap div is the single source of truth; we parse it once and
+ stash the pack on a global that the translator reads lazily.
+ See upstream issue #35330.
+ #}
+ <script nonce="{{ macros.get_nonce() }}">
Review Comment:
One trade-off to call out: the full Jed pack now ships inline in every full-page HTML response (and HTML-attribute-escaped inside `data-bootstrap`, so a touch bigger), instead of the separately-cacheable `/language_pack/` fetch. Probably fine for an SPA where hard reloads are rare, but worth being deliberate about it.
##########
superset-frontend/src/preamble.ts:
##########
@@ -64,34 +64,53 @@ export default function initPreamble(): Promise<void> {
// Setup SupersetClient early so we can fetch language pack
setupClient({ appRoot: applicationRoot() });
- // Load language pack before rendering
- // Use native fetch to avoid race condition with SupersetClient initialization
+ // Load language pack before rendering.
+ // Prefer the bootstrap-injected pack (stashed on window by the inline
+ // script in spa.html, sourced from common.language_pack) so
+ // module-level `const X = t('...')` calls in code-split chunks all
+ // see a configured translator. Fall back to the async fetch only
+ // when the bootstrap payload didn't carry the pack (e.g. embedded
+ // or a legacy entry that doesn't extend spa.html). See issue #35330.
if (lang !== 'en') {
- const abortController = new AbortController();
- const timeoutId = window.setTimeout(() => {
- abortController.abort();
- }, LANGUAGE_PACK_REQUEST_TIMEOUT_MS);
-
- try {
- const languagePackUrl = makeUrl(`/superset/language_pack/${lang}/`);
- const resp = await fetch(languagePackUrl, {
- signal: abortController.signal,
- });
- if (!resp.ok) {
- throw new Error(`Failed to fetch language pack: ${resp.status}`);
- }
- const json = await resp.json();
- configure({ languagePack: json as LanguagePack });
+ const bootstrapPack =
+ (bootstrapData.common as { language_pack?: LanguagePack })
+ .language_pack ??
+ (typeof window !== 'undefined'
+ ? window.__SUPERSET_LANGUAGE_PACK__
+ : undefined);
+ if (bootstrapPack) {
+ configure({ languagePack: bootstrapPack });
dayjs.locale(lang);
- } catch (err) {
- logging.warn(
- 'Failed to fetch language pack, falling back to default.',
- err,
- );
- configure();
- dayjs.locale('en');
- } finally {
- window.clearTimeout(timeoutId);
+ } else {
Review Comment:
Worth flagging as a known limitation: embedded doesn't extend `spa.html`, so it never gets the injected pack and lands here on the async fetch, which still has the same code-split chunk race this PR is fixing. So embedded dashboards would stay partially translated. Fine to leave for a follow-up, just might be worth a note so it's not assumed covered.
##########
superset/views/base.py:
##########
@@ -550,7 +551,22 @@ def common_bootstrap_payload() -> dict[str, Any]:
locale = get_locale()
# Convert locale to string for proper cache key hashing
locale_str = str(locale) if locale else None
- return cached_common_bootstrap_data(utils.get_user_id(), locale_str)
+ payload = dict(cached_common_bootstrap_data(utils.get_user_id(), locale_str))
+ # Inject the Jed language pack outside the per-user memoize so the cached
+ # payload stays small and the pack is shared across users for the same
+ # locale. The frontend uses it to configure the translator synchronously,
+ # before any code-split chunk evaluates a module-level `const X = t('...')`
+ # (upstream issue #35330).
+ language = payload.get("locale")
+ if language and language != "en":
+ # `get_language_pack` falls back to an empty English pack on miss;
+ # treat empty as "no pack" so the frontend can fall through to the
+ # async fetch instead of configuring with an empty translator.
+ pack = get_language_pack(language) or None
+ else:
+ pack = None
+ payload["language_pack"] = pack
Review Comment:
Two small things here. `get_language_pack` returns the empty English pack on a miss (never falsy), so the `or None` never actually fires and the comment's "fall through to the async fetch" doesn't happen. And since `COMMON_BOOTSTRAP_OVERRIDES_FUNC` can set `language_pack` itself (the workaround in #35330 does exactly that), this clobbers it. Both fixable by preferring an existing pack and dropping the dead `or None`:
```suggestion
language = payload.get("locale")
if language and language != "en":
# Respect a pack already provided via COMMON_BOOTSTRAP_OVERRIDES_FUNC,
# otherwise load the shared one. get_language_pack returns the empty
# English pack on a miss, which is the right result (English) when no
# translation file exists.
pack = payload.get("language_pack") or get_language_pack(language)
else:
pack = None
payload["language_pack"] = pack
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(i18n): inject language pack into bootstrap to translate code-split chunks [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #39357:
URL: https://github.com/apache/superset/pull/39357#discussion_r3525686772
##########
tests/unit_tests/views/test_bootstrap_auth.py:
##########
@@ -133,3 +133,73 @@ def test_recaptcha_shown_for_non_federated_auth(
payload = _get_bootstrap()
assert payload["conf"]["RECAPTCHA_PUBLIC_KEY"] == "test-key"
+
+
+# --- language_pack injection --------------------------------------------
+#
+# The Jed pack is injected by `common_bootstrap_payload` (outside the
+# memoized `cached_common_bootstrap_data`) using the shared
+# `superset.translations.utils.get_language_pack`. Tests here cover the
+# wrapper to confirm the pack lands on the payload for non-English
+# locales and is None for English.
+
+
+def test_common_bootstrap_payload_includes_language_pack_for_non_english(
+ app_context: None,
+) -> None:
+ """common.language_pack carries the shared utility's pack for non-en."""
+ fake_pack = {"domain": "superset", "locale_data": {"superset": {}}}
Review Comment:
✅ **Customized review instruction saved!**
**Instruction:**
> Do not flag missing type annotations for local variables in test modules, since test files are covered by the `tests.*` mypy override and may follow a looser typing convention.
**Applied to:**
- `**/test/**`
- `**/tests/**`
- `**/*test*.<ext>`
---
💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)*
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(i18n): inject language pack into bootstrap to translate code-split chunks [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #39357:
URL: https://github.com/apache/superset/pull/39357#issuecomment-4251038551
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) is building environment on [GHA](https://github.com/apache/superset/actions/runs/24448009019) for [254ec95](https://github.com/apache/superset/commit/254ec95)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(i18n): inject language pack into bootstrap to translate code-split chunks [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #39357:
URL: https://github.com/apache/superset/pull/39357#issuecomment-4384572989
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(i18n): inject language pack into bootstrap to translate code-split chunks [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #39357:
URL: https://github.com/apache/superset/pull/39357#issuecomment-4750038009
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(i18n): inject language pack into bootstrap to translate code-split chunks [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #39357:
URL: https://github.com/apache/superset/pull/39357#issuecomment-4248749904
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(i18n): inject language pack into bootstrap to translate code-split chunks [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #39357:
URL: https://github.com/apache/superset/pull/39357#discussion_r3525662657
##########
superset/views/base.py:
##########
@@ -576,7 +577,24 @@ def common_bootstrap_payload() -> dict[str, Any]:
locale = get_locale()
# Convert locale to string for proper cache key hashing
locale_str = str(locale) if locale else None
- return cached_common_bootstrap_data(utils.get_user_id(), locale_str)
+ payload = dict(cached_common_bootstrap_data(utils.get_user_id(), locale_str))
Review Comment:
**Suggestion:** Add an explicit type annotation to this newly introduced local payload object so the modified code complies with the type-hinting rule. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a newly introduced local variable in modified Python code and it is not type annotated, which matches the type-hint requirement for relevant variables that can be annotated.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=6f36185f2d2149a894cf3e20b2101dcb&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=6f36185f2d2149a894cf3e20b2101dcb&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/views/base.py
**Line:** 580:580
**Comment:**
*Custom Rule: Add an explicit type annotation to this newly introduced local payload object so the modified code complies with the type-hinting rule.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39357&comment_hash=64a251d05eb9f33aa29e2fb26dae68ea013d23cffd510c8e488505a02ad1f859&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39357&comment_hash=64a251d05eb9f33aa29e2fb26dae68ea013d23cffd510c8e488505a02ad1f859&reaction=dislike'>👎</a>
##########
superset/views/base.py:
##########
@@ -576,7 +577,24 @@ def common_bootstrap_payload() -> dict[str, Any]:
locale = get_locale()
# Convert locale to string for proper cache key hashing
locale_str = str(locale) if locale else None
- return cached_common_bootstrap_data(utils.get_user_id(), locale_str)
+ payload = dict(cached_common_bootstrap_data(utils.get_user_id(), locale_str))
+ # Inject the Jed language pack outside the per-user memoize so the cached
+ # payload stays small and the pack is shared across users for the same
+ # locale. The frontend uses it to configure the translator synchronously,
+ # before any code-split chunk evaluates a module-level `const X = t('...')`
+ # (upstream issue #35330).
+ language = payload.get("locale")
Review Comment:
**Suggestion:** Add a concrete type hint for this new local locale-derived value to keep modified logic fully typed. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This local variable is newly introduced in the modified function body and lacks an explicit type hint, so it violates the type-hint rule for annotatable variables.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=6ace9c1eb64746b698de467f6f4c7aca&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=6ace9c1eb64746b698de467f6f4c7aca&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/views/base.py
**Line:** 586:586
**Comment:**
*Custom Rule: Add a concrete type hint for this new local locale-derived value to keep modified logic fully typed.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39357&comment_hash=4bc071a8818c30798795b989dfbba1c2fef139810fe6a1216c66fe947b130989&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39357&comment_hash=4bc071a8818c30798795b989dfbba1c2fef139810fe6a1216c66fe947b130989&reaction=dislike'>👎</a>
##########
superset/views/base.py:
##########
@@ -576,7 +577,24 @@ def common_bootstrap_payload() -> dict[str, Any]:
locale = get_locale()
# Convert locale to string for proper cache key hashing
locale_str = str(locale) if locale else None
- return cached_common_bootstrap_data(utils.get_user_id(), locale_str)
+ payload = dict(cached_common_bootstrap_data(utils.get_user_id(), locale_str))
+ # Inject the Jed language pack outside the per-user memoize so the cached
+ # payload stays small and the pack is shared across users for the same
+ # locale. The frontend uses it to configure the translator synchronously,
+ # before any code-split chunk evaluates a module-level `const X = t('...')`
+ # (upstream issue #35330).
+ language = payload.get("locale")
+ if language and language != "en":
+ # Respect a pack already provided via COMMON_BOOTSTRAP_OVERRIDES_FUNC
+ # (the workaround in #35330 does exactly that), otherwise load the
+ # shared one. `get_language_pack` returns the empty English pack on a
+ # miss, which is the right result (English) when no translation file
+ # exists.
+ pack = payload.get("language_pack") or get_language_pack(language)
Review Comment:
**Suggestion:** Type-annotate this new local variable that is assigned conditionally so the added branch logic remains compliant with required annotations. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a newly added local variable assigned in the new branch logic and it is not annotated, which fits the type-hint rule for relevant variables.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d0d51f2929ed4252a196fd66c52bb0ce&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=d0d51f2929ed4252a196fd66c52bb0ce&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/views/base.py
**Line:** 593:593
**Comment:**
*Custom Rule: Type-annotate this new local variable that is assigned conditionally so the added branch logic remains compliant with required annotations.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39357&comment_hash=ed1a10695d7ba49ce324666a58fb5269262b1131be966279f690a238a7bd0ce3&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39357&comment_hash=ed1a10695d7ba49ce324666a58fb5269262b1131be966279f690a238a7bd0ce3&reaction=dislike'>👎</a>
##########
superset-frontend/packages/superset-core/src/translation/TranslatorSingleton.test.ts:
##########
@@ -173,3 +173,82 @@ test('resetTranslation does nothing when not yet configured', () => {
consoleSpy.mockRestore();
});
});
+
+// --- autoConfigureFromWindow ----------------------------------------------
+// These cover the bootstrap-injection path used to dodge the
+// module-level `const X = t(...)` race across code-split chunks
+// (upstream issue #35330).
+
+test('t() self-configures from window.__SUPERSET_LANGUAGE_PACK__ on first call', () => {
+ jest.isolateModules(() => {
+ (window as any).__SUPERSET_LANGUAGE_PACK__ = {
Review Comment:
**Suggestion:** Replace the `any` cast by using a typed global window extension for `__SUPERSET_LANGUAGE_PACK__`. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The code uses a TypeScript `any` cast on `window` when assigning the global `__SUPERSET_LANGUAGE_PACK__` property, which directly violates the no-any-types rule.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=806560c9ddb54597bd09d0c8a1b32b15&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=806560c9ddb54597bd09d0c8a1b32b15&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/packages/superset-core/src/translation/TranslatorSingleton.test.ts
**Line:** 184:184
**Comment:**
*Custom Rule: Replace the `any` cast by using a typed global window extension for `__SUPERSET_LANGUAGE_PACK__`.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39357&comment_hash=1628e5117135aa69e0723e1eb5eab11ac7834e8720bbfc2cb8d7e1d4bda85d82&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39357&comment_hash=1628e5117135aa69e0723e1eb5eab11ac7834e8720bbfc2cb8d7e1d4bda85d82&reaction=dislike'>👎</a>
##########
superset-frontend/packages/superset-core/src/translation/TranslatorSingleton.test.ts:
##########
@@ -173,3 +173,82 @@ test('resetTranslation does nothing when not yet configured', () => {
consoleSpy.mockRestore();
});
});
+
+// --- autoConfigureFromWindow ----------------------------------------------
+// These cover the bootstrap-injection path used to dodge the
+// module-level `const X = t(...)` race across code-split chunks
+// (upstream issue #35330).
+
+test('t() self-configures from window.__SUPERSET_LANGUAGE_PACK__ on first call', () => {
+ jest.isolateModules(() => {
+ (window as any).__SUPERSET_LANGUAGE_PACK__ = {
+ domain: 'superset',
+ locale_data: {
+ superset: {
+ '': {
+ domain: 'superset',
+ lang: 'fr',
+ plural_forms: 'nplurals=2; plural=(n > 1);',
+ },
+ hello: ['bonjour'],
+ },
+ },
+ };
+ const consoleSpy = jest.spyOn(console, 'warn').mockImplementation(() => {});
+ const { t } = require('./TranslatorSingleton');
+ expect(t('hello')).toBe('bonjour');
+ // No "should call configure" warning because we self-configured first.
+ expect(consoleSpy).not.toHaveBeenCalled();
+ consoleSpy.mockRestore();
+ delete (window as any).__SUPERSET_LANGUAGE_PACK__;
+ });
+});
+
+test('t() falls back to msgid when window has no language pack', () => {
+ jest.isolateModules(() => {
+ delete (window as any).__SUPERSET_LANGUAGE_PACK__;
+ const consoleSpy = jest.spyOn(console, 'warn').mockImplementation(() => {});
+ const { t } = require('./TranslatorSingleton');
+ expect(t('hello')).toBe('hello');
+ expect(consoleSpy).toHaveBeenCalledWith(
+ expect.stringMatching(/was called before configure\(\)/),
+ );
+ consoleSpy.mockRestore();
+ });
+});
+
+test('explicit configure() takes precedence over window pack', () => {
+ jest.isolateModules(() => {
+ (window as any).__SUPERSET_LANGUAGE_PACK__ = {
Review Comment:
**Suggestion:** Assign `__SUPERSET_LANGUAGE_PACK__` through a properly typed window interface rather than `any`. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The assignment again uses `(window as any)`, which is exactly the kind of TypeScript `any` usage prohibited by the rule.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=88a2d834608d4aa1a5eb277b0edcf814&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=88a2d834608d4aa1a5eb277b0edcf814&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/packages/superset-core/src/translation/TranslatorSingleton.test.ts
**Line:** 222:222
**Comment:**
*Custom Rule: Assign `__SUPERSET_LANGUAGE_PACK__` through a properly typed window interface rather than `any`.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39357&comment_hash=a3f6ddb86411553c032194e84bebeec6137234d9bc3362a5792558b1ed659cce&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39357&comment_hash=a3f6ddb86411553c032194e84bebeec6137234d9bc3362a5792558b1ed659cce&reaction=dislike'>👎</a>
##########
superset-frontend/packages/superset-core/src/translation/TranslatorSingleton.test.ts:
##########
@@ -173,3 +173,82 @@ test('resetTranslation does nothing when not yet configured', () => {
consoleSpy.mockRestore();
});
});
+
+// --- autoConfigureFromWindow ----------------------------------------------
+// These cover the bootstrap-injection path used to dodge the
+// module-level `const X = t(...)` race across code-split chunks
+// (upstream issue #35330).
+
+test('t() self-configures from window.__SUPERSET_LANGUAGE_PACK__ on first call', () => {
+ jest.isolateModules(() => {
+ (window as any).__SUPERSET_LANGUAGE_PACK__ = {
+ domain: 'superset',
+ locale_data: {
+ superset: {
+ '': {
+ domain: 'superset',
+ lang: 'fr',
+ plural_forms: 'nplurals=2; plural=(n > 1);',
+ },
+ hello: ['bonjour'],
+ },
+ },
+ };
+ const consoleSpy = jest.spyOn(console, 'warn').mockImplementation(() => {});
+ const { t } = require('./TranslatorSingleton');
+ expect(t('hello')).toBe('bonjour');
+ // No "should call configure" warning because we self-configured first.
+ expect(consoleSpy).not.toHaveBeenCalled();
+ consoleSpy.mockRestore();
+ delete (window as any).__SUPERSET_LANGUAGE_PACK__;
Review Comment:
**Suggestion:** Use the same typed window extension when clearing `__SUPERSET_LANGUAGE_PACK__` instead of casting to `any`. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The deletion uses an `any` cast on `window`, so the file still contains a new TypeScript `any` usage that violates the rule.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=c20054e5f5e14e49bea6187f2c71d74f&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=c20054e5f5e14e49bea6187f2c71d74f&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/packages/superset-core/src/translation/TranslatorSingleton.test.ts
**Line:** 203:203
**Comment:**
*Custom Rule: Use the same typed window extension when clearing `__SUPERSET_LANGUAGE_PACK__` instead of casting to `any`.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39357&comment_hash=b60e46e9fd27df24e8ac37a5c04598849f5a0107671f42aa61a4b11ef71dbc09&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39357&comment_hash=b60e46e9fd27df24e8ac37a5c04598849f5a0107671f42aa61a4b11ef71dbc09&reaction=dislike'>👎</a>
##########
superset-frontend/packages/superset-core/src/translation/TranslatorSingleton.test.ts:
##########
@@ -173,3 +173,82 @@ test('resetTranslation does nothing when not yet configured', () => {
consoleSpy.mockRestore();
});
});
+
+// --- autoConfigureFromWindow ----------------------------------------------
+// These cover the bootstrap-injection path used to dodge the
+// module-level `const X = t(...)` race across code-split chunks
+// (upstream issue #35330).
+
+test('t() self-configures from window.__SUPERSET_LANGUAGE_PACK__ on first call', () => {
+ jest.isolateModules(() => {
+ (window as any).__SUPERSET_LANGUAGE_PACK__ = {
+ domain: 'superset',
+ locale_data: {
+ superset: {
+ '': {
+ domain: 'superset',
+ lang: 'fr',
+ plural_forms: 'nplurals=2; plural=(n > 1);',
+ },
+ hello: ['bonjour'],
+ },
+ },
+ };
+ const consoleSpy = jest.spyOn(console, 'warn').mockImplementation(() => {});
+ const { t } = require('./TranslatorSingleton');
+ expect(t('hello')).toBe('bonjour');
+ // No "should call configure" warning because we self-configured first.
+ expect(consoleSpy).not.toHaveBeenCalled();
+ consoleSpy.mockRestore();
+ delete (window as any).__SUPERSET_LANGUAGE_PACK__;
+ });
+});
+
+test('t() falls back to msgid when window has no language pack', () => {
+ jest.isolateModules(() => {
+ delete (window as any).__SUPERSET_LANGUAGE_PACK__;
Review Comment:
**Suggestion:** Replace the `any` cast with a concrete window type when deleting `__SUPERSET_LANGUAGE_PACK__`. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is another occurrence of the same `any` cast on `window`, so the custom no-any rule is genuinely violated here as well.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=925b130ed7374f03ac587e41d6c150b7&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=925b130ed7374f03ac587e41d6c150b7&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/packages/superset-core/src/translation/TranslatorSingleton.test.ts
**Line:** 209:209
**Comment:**
*Custom Rule: Replace the `any` cast with a concrete window type when deleting `__SUPERSET_LANGUAGE_PACK__`.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39357&comment_hash=238d14a399880f214731e465cc218fee1eb354e92f37a8f9cc46db0b08bfbc78&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39357&comment_hash=238d14a399880f214731e465cc218fee1eb354e92f37a8f9cc46db0b08bfbc78&reaction=dislike'>👎</a>
##########
superset-frontend/packages/superset-core/src/translation/TranslatorSingleton.test.ts:
##########
@@ -173,3 +173,82 @@ test('resetTranslation does nothing when not yet configured', () => {
consoleSpy.mockRestore();
});
});
+
+// --- autoConfigureFromWindow ----------------------------------------------
+// These cover the bootstrap-injection path used to dodge the
+// module-level `const X = t(...)` race across code-split chunks
+// (upstream issue #35330).
+
+test('t() self-configures from window.__SUPERSET_LANGUAGE_PACK__ on first call', () => {
+ jest.isolateModules(() => {
+ (window as any).__SUPERSET_LANGUAGE_PACK__ = {
+ domain: 'superset',
+ locale_data: {
+ superset: {
+ '': {
+ domain: 'superset',
+ lang: 'fr',
+ plural_forms: 'nplurals=2; plural=(n > 1);',
+ },
+ hello: ['bonjour'],
+ },
+ },
+ };
+ const consoleSpy = jest.spyOn(console, 'warn').mockImplementation(() => {});
+ const { t } = require('./TranslatorSingleton');
+ expect(t('hello')).toBe('bonjour');
+ // No "should call configure" warning because we self-configured first.
+ expect(consoleSpy).not.toHaveBeenCalled();
+ consoleSpy.mockRestore();
+ delete (window as any).__SUPERSET_LANGUAGE_PACK__;
+ });
+});
+
+test('t() falls back to msgid when window has no language pack', () => {
+ jest.isolateModules(() => {
+ delete (window as any).__SUPERSET_LANGUAGE_PACK__;
+ const consoleSpy = jest.spyOn(console, 'warn').mockImplementation(() => {});
+ const { t } = require('./TranslatorSingleton');
+ expect(t('hello')).toBe('hello');
+ expect(consoleSpy).toHaveBeenCalledWith(
+ expect.stringMatching(/was called before configure\(\)/),
+ );
+ consoleSpy.mockRestore();
+ });
+});
+
+test('explicit configure() takes precedence over window pack', () => {
+ jest.isolateModules(() => {
+ (window as any).__SUPERSET_LANGUAGE_PACK__ = {
+ domain: 'superset',
+ locale_data: {
+ superset: {
+ '': {
+ domain: 'superset',
+ lang: 'fr',
+ plural_forms: 'nplurals=2; plural=(n > 1);',
+ },
+ hello: ['bonjour'],
+ },
+ },
+ };
+ const { configure, t } = require('./TranslatorSingleton');
+ configure({
+ languagePack: {
+ domain: 'superset',
+ locale_data: {
+ superset: {
+ '': {
+ domain: 'superset',
+ lang: 'es',
+ plural_forms: 'nplurals=2; plural=(n != 1);',
+ },
+ hello: ['hola'],
+ },
+ },
+ },
+ });
+ expect(t('hello')).toBe('hola');
+ delete (window as any).__SUPERSET_LANGUAGE_PACK__;
Review Comment:
**Suggestion:** Use a concrete type for `window` when deleting `__SUPERSET_LANGUAGE_PACK__` instead of `any`. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This line also contains the `any` cast on `window`, so it is a real instance of the no-any-types violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=bac02f86ca2a411cab806b258b821421&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=bac02f86ca2a411cab806b258b821421&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/packages/superset-core/src/translation/TranslatorSingleton.test.ts
**Line:** 252:252
**Comment:**
*Custom Rule: Use a concrete type for `window` when deleting `__SUPERSET_LANGUAGE_PACK__` instead of `any`.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39357&comment_hash=d3998dc911ec9a2f89f88eb45e2c8b4ecbdce222fd5fae4ea37d78b93244f9c7&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39357&comment_hash=d3998dc911ec9a2f89f88eb45e2c8b4ecbdce222fd5fae4ea37d78b93244f9c7&reaction=dislike'>👎</a>
##########
tests/unit_tests/views/test_bootstrap_auth.py:
##########
@@ -133,3 +133,73 @@ def test_recaptcha_shown_for_non_federated_auth(
payload = _get_bootstrap()
assert payload["conf"]["RECAPTCHA_PUBLIC_KEY"] == "test-key"
+
+
+# --- language_pack injection --------------------------------------------
+#
+# The Jed pack is injected by `common_bootstrap_payload` (outside the
+# memoized `cached_common_bootstrap_data`) using the shared
+# `superset.translations.utils.get_language_pack`. Tests here cover the
+# wrapper to confirm the pack lands on the payload for non-English
+# locales and is None for English.
+
+
+def test_common_bootstrap_payload_includes_language_pack_for_non_english(
+ app_context: None,
+) -> None:
+ """common.language_pack carries the shared utility's pack for non-en."""
+ fake_pack = {"domain": "superset", "locale_data": {"superset": {}}}
Review Comment:
**Suggestion:** Add an explicit type annotation for the newly introduced local dictionary variable to satisfy the type-hint requirement for relevant annotatable variables. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a newly added local variable with an obvious dictionary type that can be annotated, and the rule requires type hints for relevant annotatable variables in new or modified Python code. The code currently omits that annotation, so the suggestion is a real rule violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=8c069238101b445e902513594c25035f&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=8c069238101b445e902513594c25035f&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/views/test_bootstrap_auth.py
**Line:** 151:151
**Comment:**
*Custom Rule: Add an explicit type annotation for the newly introduced local dictionary variable to satisfy the type-hint requirement for relevant annotatable variables.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39357&comment_hash=a4527955a6da0a4794f2c59f59b3980da0a855accc625ea8d3d2db2dcc75e534&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39357&comment_hash=a4527955a6da0a4794f2c59f59b3980da0a855accc625ea8d3d2db2dcc75e534&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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(i18n): inject language pack into bootstrap to translate code-split chunks [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #39357:
URL: https://github.com/apache/superset/pull/39357#issuecomment-4887657897
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | 72446dc790e3e7d2f4d74126ba01e2ec860d6bba |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6a4acc95bb53f600087f35f0 |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-39357--superset-docs-preview.netlify.app](https://deploy-preview-39357--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">🤖</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#Dcq7DYAgEADQXa6WiEEQ3cAxOD4RoxxBCo1xd-le8V7Ihc5cYQHoAItJdmsO8e7joBNDonrVYjKLafe2RkrtOZ8PelbXpjKjsXaWiFIExTnXUxAycPh-) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-39357--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(i18n): inject language pack into bootstrap to translate code-split chunks [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #39357:
URL: https://github.com/apache/superset/pull/39357#discussion_r3526347611
##########
superset/views/base.py:
##########
@@ -576,7 +577,24 @@ def common_bootstrap_payload() -> dict[str, Any]:
locale = get_locale()
# Convert locale to string for proper cache key hashing
locale_str = str(locale) if locale else None
- return cached_common_bootstrap_data(utils.get_user_id(), locale_str)
+ payload = dict(cached_common_bootstrap_data(utils.get_user_id(), locale_str))
+ # Inject the Jed language pack outside the per-user memoize so the cached
+ # payload stays small and the pack is shared across users for the same
+ # locale. The frontend uses it to configure the translator synchronously,
+ # before any code-split chunk evaluates a module-level `const X = t('...')`
+ # (upstream issue #35330).
+ language = payload.get("locale")
+ if language and language != "en":
+ # Respect a pack already provided via COMMON_BOOTSTRAP_OVERRIDES_FUNC
+ # (the workaround in #35330 does exactly that), otherwise load the
+ # shared one. `get_language_pack` returns the empty English pack on a
+ # miss, which is the right result (English) when no translation file
+ # exists.
+ pack = payload.get("language_pack") or get_language_pack(language)
Review Comment:
I don't think we want this one... an empty `{}` pack isn't a usable override: `Translator`'s constructor reads `locale_data.superset[''].lang`, so honoring `{}` would crash the preamble's synchronous configure path for every non-English user. The truthiness check deliberately degrades a meaningless empty override to the real pack instead. Also `get_language_pack` caches per process (`ALL_LANGUAGE_PACKS`), so there's no repeated disk I/O either way.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(i18n): inject language pack into bootstrap to translate code-split chunks [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #39357:
URL: https://github.com/apache/superset/pull/39357#issuecomment-4886692067
Everything from my pass is addressed, so LGTM. It's gone conflicting with master since, though. After another rebase, hopefully we can merge.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(i18n): inject language pack into bootstrap to translate code-split chunks [superset]
Posted by "alex-poor (via GitHub)" <gi...@apache.org>.
alex-poor commented on code in PR #39357:
URL: https://github.com/apache/superset/pull/39357#discussion_r3197257331
##########
superset/views/base.py:
##########
@@ -436,6 +436,34 @@ def get_default_spinner_svg() -> str | None:
return None
+def _load_language_pack(language: str | None) -> dict[str, Any] | None:
+ """Read the Jed language pack for ``language`` off disk.
+
+ Returns ``None`` for English, when the JSON file is missing, or when
+ it can't be parsed. The result is injected into the bootstrap payload
+ so the frontend can configure the translator synchronously before any
+ code-split chunk evaluates a module-level ``const X = t('...')``
+ (see upstream issue #35330).
+ """
+ if not language or language == "en":
+ return None
+ pack_path = os.path.join(
+ os.path.dirname(__file__),
+ "..",
+ "translations",
+ language,
+ "LC_MESSAGES",
+ "messages.json",
+ )
+ if not os.path.isfile(pack_path):
+ return None
+ try:
+ with open(pack_path, encoding="utf-8") as fp:
+ return cast(dict[str, Any], json.loads(fp.read()))
+ except (OSError, ValueError):
+ return None
Review Comment:
Addressed in 9b2e2c84a5
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(i18n): inject language pack into bootstrap to translate code-split chunks [superset]
Posted by "alex-poor (via GitHub)" <gi...@apache.org>.
alex-poor commented on PR #39357:
URL: https://github.com/apache/superset/pull/39357#issuecomment-4503431870
hi @michael-s-molina @mistercrunch ! I have resolved the copilot review issues and will be grateful for any thoughts or advice on this PR. Thank you.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(i18n): inject language pack into bootstrap to translate code-split chunks [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #39357:
URL: https://github.com/apache/superset/pull/39357#discussion_r3525685987
##########
superset-frontend/packages/superset-core/src/translation/TranslatorSingleton.test.ts:
##########
@@ -173,3 +173,82 @@ test('resetTranslation does nothing when not yet configured', () => {
consoleSpy.mockRestore();
});
});
+
+// --- autoConfigureFromWindow ----------------------------------------------
+// These cover the bootstrap-injection path used to dodge the
+// module-level `const X = t(...)` race across code-split chunks
+// (upstream issue #35330).
+
+test('t() self-configures from window.__SUPERSET_LANGUAGE_PACK__ on first call', () => {
+ jest.isolateModules(() => {
+ (window as any).__SUPERSET_LANGUAGE_PACK__ = {
Review Comment:
Good catch, fixed in 9eb535074e... `TranslatorSingleton.ts` already declares the typed `Window` global, so the casts weren't needed at all.
##########
superset-frontend/packages/superset-core/src/translation/TranslatorSingleton.test.ts:
##########
@@ -173,3 +173,82 @@ test('resetTranslation does nothing when not yet configured', () => {
consoleSpy.mockRestore();
});
});
+
+// --- autoConfigureFromWindow ----------------------------------------------
+// These cover the bootstrap-injection path used to dodge the
+// module-level `const X = t(...)` race across code-split chunks
+// (upstream issue #35330).
+
+test('t() self-configures from window.__SUPERSET_LANGUAGE_PACK__ on first call', () => {
+ jest.isolateModules(() => {
+ (window as any).__SUPERSET_LANGUAGE_PACK__ = {
+ domain: 'superset',
+ locale_data: {
+ superset: {
+ '': {
+ domain: 'superset',
+ lang: 'fr',
+ plural_forms: 'nplurals=2; plural=(n > 1);',
+ },
+ hello: ['bonjour'],
+ },
+ },
+ };
+ const consoleSpy = jest.spyOn(console, 'warn').mockImplementation(() => {});
+ const { t } = require('./TranslatorSingleton');
+ expect(t('hello')).toBe('bonjour');
+ // No "should call configure" warning because we self-configured first.
+ expect(consoleSpy).not.toHaveBeenCalled();
+ consoleSpy.mockRestore();
+ delete (window as any).__SUPERSET_LANGUAGE_PACK__;
Review Comment:
Same fix as the thread above, 9eb535074e.
##########
superset-frontend/packages/superset-core/src/translation/TranslatorSingleton.test.ts:
##########
@@ -173,3 +173,82 @@ test('resetTranslation does nothing when not yet configured', () => {
consoleSpy.mockRestore();
});
});
+
+// --- autoConfigureFromWindow ----------------------------------------------
+// These cover the bootstrap-injection path used to dodge the
+// module-level `const X = t(...)` race across code-split chunks
+// (upstream issue #35330).
+
+test('t() self-configures from window.__SUPERSET_LANGUAGE_PACK__ on first call', () => {
+ jest.isolateModules(() => {
+ (window as any).__SUPERSET_LANGUAGE_PACK__ = {
+ domain: 'superset',
+ locale_data: {
+ superset: {
+ '': {
+ domain: 'superset',
+ lang: 'fr',
+ plural_forms: 'nplurals=2; plural=(n > 1);',
+ },
+ hello: ['bonjour'],
+ },
+ },
+ };
+ const consoleSpy = jest.spyOn(console, 'warn').mockImplementation(() => {});
+ const { t } = require('./TranslatorSingleton');
+ expect(t('hello')).toBe('bonjour');
+ // No "should call configure" warning because we self-configured first.
+ expect(consoleSpy).not.toHaveBeenCalled();
+ consoleSpy.mockRestore();
+ delete (window as any).__SUPERSET_LANGUAGE_PACK__;
+ });
+});
+
+test('t() falls back to msgid when window has no language pack', () => {
+ jest.isolateModules(() => {
+ delete (window as any).__SUPERSET_LANGUAGE_PACK__;
Review Comment:
Same fix as the thread above, 9eb535074e.
##########
superset-frontend/packages/superset-core/src/translation/TranslatorSingleton.test.ts:
##########
@@ -173,3 +173,82 @@ test('resetTranslation does nothing when not yet configured', () => {
consoleSpy.mockRestore();
});
});
+
+// --- autoConfigureFromWindow ----------------------------------------------
+// These cover the bootstrap-injection path used to dodge the
+// module-level `const X = t(...)` race across code-split chunks
+// (upstream issue #35330).
+
+test('t() self-configures from window.__SUPERSET_LANGUAGE_PACK__ on first call', () => {
+ jest.isolateModules(() => {
+ (window as any).__SUPERSET_LANGUAGE_PACK__ = {
+ domain: 'superset',
+ locale_data: {
+ superset: {
+ '': {
+ domain: 'superset',
+ lang: 'fr',
+ plural_forms: 'nplurals=2; plural=(n > 1);',
+ },
+ hello: ['bonjour'],
+ },
+ },
+ };
+ const consoleSpy = jest.spyOn(console, 'warn').mockImplementation(() => {});
+ const { t } = require('./TranslatorSingleton');
+ expect(t('hello')).toBe('bonjour');
+ // No "should call configure" warning because we self-configured first.
+ expect(consoleSpy).not.toHaveBeenCalled();
+ consoleSpy.mockRestore();
+ delete (window as any).__SUPERSET_LANGUAGE_PACK__;
+ });
+});
+
+test('t() falls back to msgid when window has no language pack', () => {
+ jest.isolateModules(() => {
+ delete (window as any).__SUPERSET_LANGUAGE_PACK__;
+ const consoleSpy = jest.spyOn(console, 'warn').mockImplementation(() => {});
+ const { t } = require('./TranslatorSingleton');
+ expect(t('hello')).toBe('hello');
+ expect(consoleSpy).toHaveBeenCalledWith(
+ expect.stringMatching(/was called before configure\(\)/),
+ );
+ consoleSpy.mockRestore();
+ });
+});
+
+test('explicit configure() takes precedence over window pack', () => {
+ jest.isolateModules(() => {
+ (window as any).__SUPERSET_LANGUAGE_PACK__ = {
Review Comment:
Same fix as the thread above, 9eb535074e.
##########
superset-frontend/packages/superset-core/src/translation/TranslatorSingleton.test.ts:
##########
@@ -173,3 +173,82 @@ test('resetTranslation does nothing when not yet configured', () => {
consoleSpy.mockRestore();
});
});
+
+// --- autoConfigureFromWindow ----------------------------------------------
+// These cover the bootstrap-injection path used to dodge the
+// module-level `const X = t(...)` race across code-split chunks
+// (upstream issue #35330).
+
+test('t() self-configures from window.__SUPERSET_LANGUAGE_PACK__ on first call', () => {
+ jest.isolateModules(() => {
+ (window as any).__SUPERSET_LANGUAGE_PACK__ = {
+ domain: 'superset',
+ locale_data: {
+ superset: {
+ '': {
+ domain: 'superset',
+ lang: 'fr',
+ plural_forms: 'nplurals=2; plural=(n > 1);',
+ },
+ hello: ['bonjour'],
+ },
+ },
+ };
+ const consoleSpy = jest.spyOn(console, 'warn').mockImplementation(() => {});
+ const { t } = require('./TranslatorSingleton');
+ expect(t('hello')).toBe('bonjour');
+ // No "should call configure" warning because we self-configured first.
+ expect(consoleSpy).not.toHaveBeenCalled();
+ consoleSpy.mockRestore();
+ delete (window as any).__SUPERSET_LANGUAGE_PACK__;
+ });
+});
+
+test('t() falls back to msgid when window has no language pack', () => {
+ jest.isolateModules(() => {
+ delete (window as any).__SUPERSET_LANGUAGE_PACK__;
+ const consoleSpy = jest.spyOn(console, 'warn').mockImplementation(() => {});
+ const { t } = require('./TranslatorSingleton');
+ expect(t('hello')).toBe('hello');
+ expect(consoleSpy).toHaveBeenCalledWith(
+ expect.stringMatching(/was called before configure\(\)/),
+ );
+ consoleSpy.mockRestore();
+ });
+});
+
+test('explicit configure() takes precedence over window pack', () => {
+ jest.isolateModules(() => {
+ (window as any).__SUPERSET_LANGUAGE_PACK__ = {
+ domain: 'superset',
+ locale_data: {
+ superset: {
+ '': {
+ domain: 'superset',
+ lang: 'fr',
+ plural_forms: 'nplurals=2; plural=(n > 1);',
+ },
+ hello: ['bonjour'],
+ },
+ },
+ };
+ const { configure, t } = require('./TranslatorSingleton');
+ configure({
+ languagePack: {
+ domain: 'superset',
+ locale_data: {
+ superset: {
+ '': {
+ domain: 'superset',
+ lang: 'es',
+ plural_forms: 'nplurals=2; plural=(n != 1);',
+ },
+ hello: ['hola'],
+ },
+ },
+ },
+ });
+ expect(t('hello')).toBe('hola');
+ delete (window as any).__SUPERSET_LANGUAGE_PACK__;
Review Comment:
Same fix as the thread above, 9eb535074e.
##########
superset/views/base.py:
##########
@@ -576,7 +577,24 @@ def common_bootstrap_payload() -> dict[str, Any]:
locale = get_locale()
# Convert locale to string for proper cache key hashing
locale_str = str(locale) if locale else None
- return cached_common_bootstrap_data(utils.get_user_id(), locale_str)
+ payload = dict(cached_common_bootstrap_data(utils.get_user_id(), locale_str))
Review Comment:
mypy infers these locals fine (`dict(...)` and values pulled from a typed payload), and this file doesn't annotate locals where inference suffices. Also heads up: #41780 rewrites this block and these locals go away entirely.
##########
superset/views/base.py:
##########
@@ -576,7 +577,24 @@ def common_bootstrap_payload() -> dict[str, Any]:
locale = get_locale()
# Convert locale to string for proper cache key hashing
locale_str = str(locale) if locale else None
- return cached_common_bootstrap_data(utils.get_user_id(), locale_str)
+ payload = dict(cached_common_bootstrap_data(utils.get_user_id(), locale_str))
+ # Inject the Jed language pack outside the per-user memoize so the cached
+ # payload stays small and the pack is shared across users for the same
+ # locale. The frontend uses it to configure the translator synchronously,
+ # before any code-split chunk evaluates a module-level `const X = t('...')`
+ # (upstream issue #35330).
+ language = payload.get("locale")
Review Comment:
Same as above... inference covers it, and the local disappears in #41780.
##########
superset/views/base.py:
##########
@@ -576,7 +577,24 @@ def common_bootstrap_payload() -> dict[str, Any]:
locale = get_locale()
# Convert locale to string for proper cache key hashing
locale_str = str(locale) if locale else None
- return cached_common_bootstrap_data(utils.get_user_id(), locale_str)
+ payload = dict(cached_common_bootstrap_data(utils.get_user_id(), locale_str))
+ # Inject the Jed language pack outside the per-user memoize so the cached
+ # payload stays small and the pack is shared across users for the same
+ # locale. The frontend uses it to configure the translator synchronously,
+ # before any code-split chunk evaluates a module-level `const X = t('...')`
+ # (upstream issue #35330).
+ language = payload.get("locale")
+ if language and language != "en":
+ # Respect a pack already provided via COMMON_BOOTSTRAP_OVERRIDES_FUNC
+ # (the workaround in #35330 does exactly that), otherwise load the
+ # shared one. `get_language_pack` returns the empty English pack on a
+ # miss, which is the right result (English) when no translation file
+ # exists.
+ pack = payload.get("language_pack") or get_language_pack(language)
Review Comment:
Same as above... inference covers it, and the local disappears in #41780.
##########
tests/unit_tests/views/test_bootstrap_auth.py:
##########
@@ -133,3 +133,73 @@ def test_recaptcha_shown_for_non_federated_auth(
payload = _get_bootstrap()
assert payload["conf"]["RECAPTCHA_PUBLIC_KEY"] == "test-key"
+
+
+# --- language_pack injection --------------------------------------------
+#
+# The Jed pack is injected by `common_bootstrap_payload` (outside the
+# memoized `cached_common_bootstrap_data`) using the shared
+# `superset.translations.utils.get_language_pack`. Tests here cover the
+# wrapper to confirm the pack lands on the payload for non-English
+# locales and is None for English.
+
+
+def test_common_bootstrap_payload_includes_language_pack_for_non_english(
+ app_context: None,
+) -> None:
+ """common.language_pack carries the shared utility's pack for non-en."""
+ fake_pack = {"domain": "superset", "locale_data": {"superset": {}}}
Review Comment:
Test modules are covered by the `tests.*` mypy override (`disallow_untyped_defs` off), and mypy/ruff pass as-is... skipping to match the file's convention.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(i18n): inject language pack into bootstrap to translate code-split chunks [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #39357:
URL: https://github.com/apache/superset/pull/39357#issuecomment-4887654257
Rebased (merged current master) to clear the conflict... master moved the language pack endpoint to `/language_pack/<lang>/`, so the async-fetch fallback here now uses the new URL. Translator, preamble, and bootstrap view tests all pass locally.
I also dug into the payload question since inlining the pack isn't free. Measured with the French catalog: ~237KB of Jed JSON, which is ~67KB gzipped inside the HTML. That cost only applies to non-English locales (en injects null, so default deployments pay nothing), and only on full page loads, which are rare in an SPA. On a cold cache this is actually *faster* than master: the fetch approach serializes HTML → JS bundle → language pack round trip before first render, and the inline pack removes that whole hop. On warm loads it's ~67KB of extra HTML where the fetch would have been free from browser cache.
One more point in its favor: `send_file` serves the pack with our 1-year `SEND_FILE_MAX_AGE_DEFAULT` on an unversioned URL, so today's cached packs can go stale for a year after an upgrade... the inline pack always matches the server version. If we ever want the best of both worlds, a versioned static pack loaded via a synchronous script tag before the entry bundle would be cacheable AND race-free, but that's a build-pipeline project, not this PR.
LGTM once CI comes back green. 🤞
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(i18n): inject language pack into bootstrap to translate code-split chunks [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #39357:
URL: https://github.com/apache/superset/pull/39357#discussion_r3525203560
##########
superset-frontend/src/preamble.ts:
##########
@@ -64,34 +64,53 @@ export default function initPreamble(): Promise<void> {
// Setup SupersetClient early so we can fetch language pack
setupClient({ appRoot: applicationRoot() });
- // Load language pack before rendering
- // Use native fetch to avoid race condition with SupersetClient initialization
+ // Load language pack before rendering.
+ // Prefer the bootstrap-injected pack (stashed on window by the inline
+ // script in spa.html, sourced from common.language_pack) so
+ // module-level `const X = t('...')` calls in code-split chunks all
+ // see a configured translator. Fall back to the async fetch only
+ // when the bootstrap payload didn't carry the pack (e.g. embedded
+ // or a legacy entry that doesn't extend spa.html). See issue #35330.
if (lang !== 'en') {
- const abortController = new AbortController();
- const timeoutId = window.setTimeout(() => {
- abortController.abort();
- }, LANGUAGE_PACK_REQUEST_TIMEOUT_MS);
-
- try {
- const languagePackUrl = makeUrl(`/superset/language_pack/${lang}/`);
- const resp = await fetch(languagePackUrl, {
- signal: abortController.signal,
- });
- if (!resp.ok) {
- throw new Error(`Failed to fetch language pack: ${resp.status}`);
- }
- const json = await resp.json();
- configure({ languagePack: json as LanguagePack });
+ const bootstrapPack =
+ (bootstrapData.common as { language_pack?: LanguagePack })
+ .language_pack ??
+ (typeof window !== 'undefined'
+ ? window.__SUPERSET_LANGUAGE_PACK__
+ : undefined);
+ if (bootstrapPack) {
+ configure({ languagePack: bootstrapPack });
dayjs.locale(lang);
- } catch (err) {
- logging.warn(
- 'Failed to fetch language pack, falling back to default.',
- err,
- );
- configure();
- dayjs.locale('en');
- } finally {
- window.clearTimeout(timeoutId);
+ } else {
Review Comment:
Ahh, you're right about embedded... `EmbeddedView` renders `spa.html` too, so it gets the inline pack.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(i18n): inject language pack into bootstrap to translate code-split chunks [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #39357:
URL: https://github.com/apache/superset/pull/39357#issuecomment-4251153387
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) deployed environment on [GHA](https://github.com/apache/superset/actions/runs/24448009019) for [254ec95](https://github.com/apache/superset/commit/254ec95)
• **Environment:** http://34.215.177.143:8080 (admin/admin)
• **Lifetime:** 48h auto-cleanup
• **Updates:** New commits create fresh environments automatically
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(i18n): inject language pack into bootstrap to translate code-split chunks [superset]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #39357:
URL: https://github.com/apache/superset/pull/39357#discussion_r3190502185
##########
tests/unit_tests/views/test_bootstrap_auth.py:
##########
@@ -133,3 +133,78 @@ def test_recaptcha_shown_for_non_federated_auth(
payload = _get_bootstrap()
assert payload["conf"]["RECAPTCHA_PUBLIC_KEY"] == "test-key"
+
+
+# --- _load_language_pack -------------------------------------------------
+
+
+@pytest.mark.parametrize("language", [None, "", "en"])
+def test_load_language_pack_returns_none_for_english(language: str | None) -> None:
+ """English (and falsy values) should never load a pack."""
+ assert _load_language_pack(language) is None
+
+
+def test_load_language_pack_returns_none_when_file_missing() -> None:
+ """Locales without a messages.json file fall through to the async path."""
+ with patch("superset.views.base.os.path.isfile", return_value=False):
+ assert _load_language_pack("xx_NOPE") is None
+
+
+def test_load_language_pack_returns_dict_when_file_present() -> None:
+ """A valid Jed JSON file is parsed and returned as a dict."""
+ fake_pack = {"domain": "superset", "locale_data": {"superset": {}}}
+ mock_open = patch(
+ "builtins.open",
+ new=MagicMock(),
+ )
+ with (
+ patch("superset.views.base.os.path.isfile", return_value=True),
+ patch(
+ "superset.views.base.json.loads",
+ return_value=fake_pack,
+ ) as mock_loads,
+ mock_open as mock_file,
+ ):
+ mock_file.return_value.__enter__.return_value.read.return_value = "{}"
+ result = _load_language_pack("pt_BR")
+
+ assert result == fake_pack
+ mock_loads.assert_called_once()
+
+
+def test_load_language_pack_returns_none_on_parse_error() -> None:
+ """Malformed JSON is swallowed; caller falls back to the async path."""
+ mock_open = patch("builtins.open", new=MagicMock())
+ with (
+ patch("superset.views.base.os.path.isfile", return_value=True),
+ patch(
+ "superset.views.base.json.loads",
+ side_effect=ValueError("bad json"),
+ ),
+ mock_open as mock_file,
+ ):
+ mock_file.return_value.__enter__.return_value.read.return_value = "garbage"
+ assert _load_language_pack("pt_BR") is None
+
+
+def test_bootstrap_includes_language_pack_when_locale_set(
+ app_context: None,
+) -> None:
+ """The bootstrap payload carries the loaded pack under common.language_pack."""
+ fake_pack = {"domain": "superset", "locale_data": {"superset": {}}}
+ with patch(
+ "superset.views.base._load_language_pack",
+ return_value=fake_pack,
+ ) as mock_load:
+ payload = _get_bootstrap()
+
+ assert payload["language_pack"] == fake_pack
+ mock_load.assert_called_once()
Review Comment:
This test is prone to flakiness because `_get_bootstrap()` calls a `@memoize`d `cached_common_bootstrap_data` with fixed arguments (`user_id=1`, `locale=None`). If any earlier test already populated the memoized value, `_load_language_pack` won’t be invoked and the patch/assertions can fail. Consider either clearing the memoized cache in the test setup, or calling `cached_common_bootstrap_data` with a unique `(user_id, locale)` tuple (and ideally a non-English `locale`, e.g. `'pt_BR'`) so the behavior under test actually matches the docstring.
##########
superset/views/base.py:
##########
@@ -524,11 +552,20 @@ def cached_common_bootstrap_data( # pylint: disable=unused-argument
)
frontend_config["AUTH_PROVIDERS"] = saml_providers
+ # Inject the Jed language pack into the bootstrap payload so the
+ # frontend can configure the translator synchronously, before any
+ # code-split chunk evaluates a module-level `const X = t('...')`.
+ # Without this, strings captured in module-scope constants render in
+ # English even when the locale is set and the async language_pack
+ # endpoint returns a valid pack (upstream issue #35330).
+ language_pack = _load_language_pack(language)
+
bootstrap_data = {
"application_root": app.config["APPLICATION_ROOT"],
"static_assets_prefix": app.config["STATIC_ASSETS_PREFIX"],
"conf": frontend_config,
"locale": language,
+ "language_pack": language_pack,
Review Comment:
Injecting the full Jed pack into `cached_common_bootstrap_data` means the memoized cache entry now includes a potentially large blob per `(user_id, locale)` key. This can significantly increase cache memory/Redis usage and churn (especially with many users/locales) since the language pack itself is locale-scoped, not user-scoped. A more scalable approach is to keep `cached_common_bootstrap_data` small and inject `language_pack` outside the memoized function (or cache `_load_language_pack(language)` separately keyed only by `language`, then insert it into the response without storing it inside the per-user memoized payload).
##########
superset/views/base.py:
##########
@@ -436,6 +436,34 @@ def get_default_spinner_svg() -> str | None:
return None
+def _load_language_pack(language: str | None) -> dict[str, Any] | None:
+ """Read the Jed language pack for ``language`` off disk.
+
+ Returns ``None`` for English, when the JSON file is missing, or when
+ it can't be parsed. The result is injected into the bootstrap payload
+ so the frontend can configure the translator synchronously before any
+ code-split chunk evaluates a module-level ``const X = t('...')``
+ (see upstream issue #35330).
+ """
+ if not language or language == "en":
+ return None
+ pack_path = os.path.join(
+ os.path.dirname(__file__),
+ "..",
+ "translations",
+ language,
+ "LC_MESSAGES",
+ "messages.json",
+ )
+ if not os.path.isfile(pack_path):
+ return None
+ try:
+ with open(pack_path, encoding="utf-8") as fp:
+ return cast(dict[str, Any], json.loads(fp.read()))
+ except (OSError, ValueError):
+ return None
+
+
@cache_manager.cache.memoize(timeout=60)
def cached_common_bootstrap_data( # pylint: disable=unused-argument
user_id: int | None, locale: str | None
Review Comment:
Injecting the full Jed pack into `cached_common_bootstrap_data` means the memoized cache entry now includes a potentially large blob per `(user_id, locale)` key. This can significantly increase cache memory/Redis usage and churn (especially with many users/locales) since the language pack itself is locale-scoped, not user-scoped. A more scalable approach is to keep `cached_common_bootstrap_data` small and inject `language_pack` outside the memoized function (or cache `_load_language_pack(language)` separately keyed only by `language`, then insert it into the response without storing it inside the per-user memoized payload).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(i18n): inject language pack into bootstrap to translate code-split chunks [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #39357:
URL: https://github.com/apache/superset/pull/39357#discussion_r3526326732
##########
superset/views/base.py:
##########
@@ -576,7 +577,24 @@ def common_bootstrap_payload() -> dict[str, Any]:
locale = get_locale()
# Convert locale to string for proper cache key hashing
locale_str = str(locale) if locale else None
- return cached_common_bootstrap_data(utils.get_user_id(), locale_str)
+ payload = dict(cached_common_bootstrap_data(utils.get_user_id(), locale_str))
+ # Inject the Jed language pack outside the per-user memoize so the cached
+ # payload stays small and the pack is shared across users for the same
+ # locale. The frontend uses it to configure the translator synchronously,
+ # before any code-split chunk evaluates a module-level `const X = t('...')`
+ # (upstream issue #35330).
+ language = payload.get("locale")
+ if language and language != "en":
+ # Respect a pack already provided via COMMON_BOOTSTRAP_OVERRIDES_FUNC
+ # (the workaround in #35330 does exactly that), otherwise load the
+ # shared one. `get_language_pack` returns the empty English pack on a
+ # miss, which is the right result (English) when no translation file
+ # exists.
+ pack = payload.get("language_pack") or get_language_pack(language)
Review Comment:
**Suggestion:** Using `or` here treats an explicitly provided but empty `language_pack` (for example `{}` from `COMMON_BOOTSTRAP_OVERRIDES_FUNC`) as missing, so the override is ignored and the code unnecessarily reloads from disk/fallback. This breaks the stated override contract and can cause repeated I/O/error logging; check for `None` explicitly instead of truthiness. [incorrect condition logic]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ COMMON_BOOTSTRAP_OVERRIDES_FUNC cannot enforce empty language pack.
- ⚠️ Non-English SPA payloads ignore explicit empty override packs.
- ⚠️ Extra language-pack I/O despite caller-provided override value.
- ⚠️ Potential repeated translation load errors despite override intent.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure a custom `COMMON_BOOTSTRAP_OVERRIDES_FUNC` using the documented extension
point at `superset/config.py:1001-1004`, in `superset_config.py`, so that for a
non-English locale (e.g. `"fr"`) it returns a dict including `"language_pack": {}`. This
dict is merged into `bootstrap_data` in `superset/views/base.py` where
`cached_common_bootstrap_data` builds the base payload and then applies
`COMMON_BOOTSTRAP_OVERRIDES_FUNC` (see `superset/views/base.py` lines 13-31 in the snippet
around
`bootstrap_data.update(app.config["COMMON_BOOTSTRAP_OVERRIDES_FUNC"](bootstrap_data))`).
2. Start Superset with this configuration and request any SPA page rendered via
`superset/spa.html`, for example a normal application entry that uses
`BaseSupersetView.render_app_template` in `superset/views/base.py:705-707`, which calls
`get_spa_template_context()`, then `get_spa_payload()`, which in turn calls
`common_bootstrap_payload()` at `superset/views/base.py:576-597`. Ensure the user locale
for the request is set to `"fr"` so that `get_locale()` returns `"fr"`.
3. Inside `common_bootstrap_payload()` (`superset/views/base.py:576-597`),
`cached_common_bootstrap_data(utils.get_user_id(), locale_str)` returns a payload that
includes `{"locale": "fr", "language_pack": {}}` because of the override in step 1. The
function then runs:
- `language = payload.get("locale")` → `"fr"`, so the `if language and language !=
"en":` branch is taken.
- `pack = payload.get("language_pack") or get_language_pack(language)` at line 593,
where `payload.get("language_pack")` is `{}` (falsy), so Python evaluates the `or` and
calls `get_language_pack("fr")` from `superset/translations/utils.py:5-29` instead of
using the explicitly overridden `{}`.
4. Observe in the resulting `common` payload sent to the frontend (e.g. by inspecting the
JSON embedded in `superset/spa.html` or by asserting in a unit test similar to
`superset/tests/unit_tests/views/test_bootstrap_auth.py:147-167`) that:
- `payload["language_pack"]` is the non-empty pack returned by
`get_language_pack("fr")`, not the `{}` provided via `COMMON_BOOTSTRAP_OVERRIDES_FUNC`,
so the explicit override is ignored.
- `get_language_pack("fr")` is invoked unnecessarily despite an override being present,
which can cause extra disk I/O and, if the underlying file is missing or malformed,
repeated error logging from `get_language_pack()` even though the intent of the
override was to supply a specific (possibly empty) pack.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=0c18d855fae44927be171a1477330171&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=0c18d855fae44927be171a1477330171&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/views/base.py
**Line:** 593:593
**Comment:**
*Incorrect Condition Logic: Using `or` here treats an explicitly provided but empty `language_pack` (for example `{}` from `COMMON_BOOTSTRAP_OVERRIDES_FUNC`) as missing, so the override is ignored and the code unnecessarily reloads from disk/fallback. This breaks the stated override contract and can cause repeated I/O/error logging; check for `None` explicitly instead of truthiness.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39357&comment_hash=68dab49ef6f1df83515f178bbdd6ab647cf41b01e3ef14065d575a4b5b1b77a4&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39357&comment_hash=68dab49ef6f1df83515f178bbdd6ab647cf41b01e3ef14065d575a4b5b1b77a4&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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(i18n): inject language pack into bootstrap to translate code-split chunks [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #39357:
URL: https://github.com/apache/superset/pull/39357#issuecomment-4888161608
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(i18n): inject language pack into bootstrap to translate code-split chunks [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #39357:
URL: https://github.com/apache/superset/pull/39357#issuecomment-4889908380
**Bito Automatic Review Skipped – PR Already Merged**<br/><br/>Bito scheduled an automatic review for this pull request, but the review was skipped because this PR was merged before the review could be run.<br/>No action is needed if you didn't intend to review it. To get a review, you can type `/review` in a comment and save it
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(i18n): inject language pack into bootstrap to translate code-split chunks [superset]
Posted by "alex-poor (via GitHub)" <gi...@apache.org>.
alex-poor commented on code in PR #39357:
URL: https://github.com/apache/superset/pull/39357#discussion_r3440990713
##########
superset/views/base.py:
##########
@@ -550,7 +551,22 @@ def common_bootstrap_payload() -> dict[str, Any]:
locale = get_locale()
# Convert locale to string for proper cache key hashing
locale_str = str(locale) if locale else None
- return cached_common_bootstrap_data(utils.get_user_id(), locale_str)
+ payload = dict(cached_common_bootstrap_data(utils.get_user_id(), locale_str))
+ # Inject the Jed language pack outside the per-user memoize so the cached
+ # payload stays small and the pack is shared across users for the same
+ # locale. The frontend uses it to configure the translator synchronously,
+ # before any code-split chunk evaluates a module-level `const X = t('...')`
+ # (upstream issue #35330).
+ language = payload.get("locale")
+ if language and language != "en":
+ # `get_language_pack` falls back to an empty English pack on miss;
+ # treat empty as "no pack" so the frontend can fall through to the
+ # async fetch instead of configuring with an empty translator.
+ pack = get_language_pack(language) or None
+ else:
+ pack = None
+ payload["language_pack"] = pack
Review Comment:
Applied, thanks. Kept your point about `COMMON_BOOTSTRAP_OVERRIDES_FUNC: pack = payload.get("language_pack") or get_language_pack(language)`, with the dead `or None` removed and the comment corrected to reflect that `get_language_pack` returns the empty English pack (not a falsy value) on a miss.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(i18n): inject language pack into bootstrap to translate code-split chunks [superset]
Posted by "alex-poor (via GitHub)" <gi...@apache.org>.
alex-poor commented on code in PR #39357:
URL: https://github.com/apache/superset/pull/39357#discussion_r3441050595
##########
superset-frontend/src/preamble.ts:
##########
@@ -64,34 +64,53 @@ export default function initPreamble(): Promise<void> {
// Setup SupersetClient early so we can fetch language pack
setupClient({ appRoot: applicationRoot() });
- // Load language pack before rendering
- // Use native fetch to avoid race condition with SupersetClient initialization
+ // Load language pack before rendering.
+ // Prefer the bootstrap-injected pack (stashed on window by the inline
+ // script in spa.html, sourced from common.language_pack) so
+ // module-level `const X = t('...')` calls in code-split chunks all
+ // see a configured translator. Fall back to the async fetch only
+ // when the bootstrap payload didn't carry the pack (e.g. embedded
+ // or a legacy entry that doesn't extend spa.html). See issue #35330.
if (lang !== 'en') {
- const abortController = new AbortController();
- const timeoutId = window.setTimeout(() => {
- abortController.abort();
- }, LANGUAGE_PACK_REQUEST_TIMEOUT_MS);
-
- try {
- const languagePackUrl = makeUrl(`/superset/language_pack/${lang}/`);
- const resp = await fetch(languagePackUrl, {
- signal: abortController.signal,
- });
- if (!resp.ok) {
- throw new Error(`Failed to fetch language pack: ${resp.status}`);
- }
- const json = await resp.json();
- configure({ languagePack: json as LanguagePack });
+ const bootstrapPack =
+ (bootstrapData.common as { language_pack?: LanguagePack })
+ .language_pack ??
+ (typeof window !== 'undefined'
+ ? window.__SUPERSET_LANGUAGE_PACK__
+ : undefined);
+ if (bootstrapPack) {
+ configure({ languagePack: bootstrapPack });
dayjs.locale(lang);
- } catch (err) {
- logging.warn(
- 'Failed to fetch language pack, falling back to default.',
- err,
- );
- configure();
- dayjs.locale('en');
- } finally {
- window.clearTimeout(timeoutId);
+ } else {
Review Comment:
As far as I can tell embedded _is_ actually covered (I think!).
* `EmbeddedView.embedded` renders `superset/spa.html` with `entry="embedded"` and calls `common_bootstrap_payload()` ([embedded/view.py#L97-L109](https://github.com/apache/superset/blob/master/superset/embedded/view.py#L97)), so the injected `common.language_pack` and the inline `<script>` that sets `window.__SUPERSET_LANGUAGE_PACK__` (before `js_bundle(entry)`) both apply.
* The embedded webpack entry is `addPreamble('src/embedded/index.tsx')`, which prepends `preamble.ts` — so the preferred injected-pack path here runs for embedded too, and `TranslatorSingleton.autoConfigureFromWindow()` self-configures from the global on the first `t()/tn()` regardless.
* `spa.html` is the only template carrying `data-bootstrap/js_bundle(entry)`, so every entry goes through this injection.
whether an embedded load is _actually_ translated still depends on the resolved locale (`?lang=/session/Accept-Language`) being non-English — if it resolves to `en`, the pack is `null` and there's nothing to inject, same as the main app.
Let me know if you're seeing it hit the async fetch path in practice, but as far as I can trace it embedded shouldn't.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org