codeant-ai-for-open-source[bot] commented on code in PR #41780:
URL: https://github.com/apache/superset/pull/41780#discussion_r3526403992
##########
superset/views/base.py:
##########
@@ -578,25 +578,50 @@ def common_bootstrap_payload() -> dict[str, Any]:
# Convert locale to string for proper cache key hashing
locale_str = str(locale) if locale else None
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)
- else:
- pack = None
- payload["language_pack"] = pack
+ # The language pack itself is NOT embedded in the payload: spa.html loads
+ # it through the content-addressed
/language_pack/<lang>/<version>/script.js
+ # tag before the entry bundle, keeping HTML small while still configuring
+ # the translator synchronously (upstream issue #35330). A pack provided via
+ # COMMON_BOOTSTRAP_OVERRIDES_FUNC (the historical workaround) is respected
+ # and takes precedence over the script tag.
+ payload.setdefault("language_pack", None)
return payload
+def get_language_pack_template_context(common: dict[str, Any]) -> dict[str,
Any]:
+ """Template vars controlling how spa.html delivers the language pack.
+
+ ``common`` is the already-built common bootstrap payload for the request
+ (passed in rather than re-derived, so callers that assembled or mocked
+ their own payload stay consistent with what the template sees).
+
+ Three mutually exclusive outcomes:
+ - English (or no locale): no script tag, nothing to stash.
+ - Operator supplied a pack via COMMON_BOOTSTRAP_OVERRIDES_FUNC: no script
+ tag; spa.html stashes the bootstrap pack on window instead.
+ - Otherwise: emit the content-addressed script URL so the browser can
+ cache the pack as immutable and cache-bust on translation changes.
+ """
+ language = common.get("locale")
+ if not language or language == "en":
+ return {"language_pack_src": None, "language_pack_inline": False}
+ if common.get("language_pack"):
+ return {"language_pack_src": None, "language_pack_inline": True}
Review Comment:
**Suggestion:** The override-presence check uses truthiness instead of key
presence, so an operator-provided empty language pack (for example `{}`) is
treated as “not provided” and the versioned script tag is emitted anyway. This
breaks the documented precedence of `COMMON_BOOTSTRAP_OVERRIDES_FUNC`; check
whether the `language_pack` key is present/non-None rather than truthy.
[incorrect condition logic]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Operator override packs ignored when empty dictionary.
- ⚠️ Language pack precedence inconsistent with configuration contract.
- ⚠️ Unnecessary script request despite configured override pack.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure `COMMON_BOOTSTRAP_OVERRIDES_FUNC` in
`superset/config.py:1001-1014` so that
for a non-English locale (for example `locale == "fr"`), the upstream common
payload
includes `{"language_pack": {}}` (an empty dict) in addition to the `locale`
key; this
matches the documented use in the config comments for overriding
`bootstrap_data.common`.
2. Make a request that renders `spa.html` via
`SupersetModelView.render_app_template` in
`superset/views/base.py:3-26`, with `get_locale()` returning `"fr"` so that
`common_bootstrap_payload()` (defined in `superset/views/base.py:17-29`)
builds a `common`
dict containing `{"locale": "fr", "language_pack": {}}`.
3. During template assembly, `get_spa_template_context` in
`superset/views/base.py:644-648` calls
`get_language_pack_template_context(payload.get("common") or {})`, and
inside that
function at `superset/views/base.py:605-609`, `language =
common.get("locale")` is `"fr"`
so the English short-circuit (`if not language or language == "en"`) is
skipped.
4. At `superset/views/base.py:608`, the condition `if
common.get("language_pack"):`
evaluates to `False` for the empty dict `{}`, so the function proceeds to
compute a
versioned script URL using `get_language_pack_version`
(`superset/translations/utils.py:44-60`) and `url_for`, returning a context
with
`{"language_pack_src": "/language_pack/fr/<version>/script.js",
"language_pack_inline":
False}`; this causes `spa.html` to emit the script tag and ignore the
operator-supplied
override pack, contrary to the intended precedence documented in the
docstring and tested
for non-empty packs in `tests/unit_tests/views/test_bootstrap_auth.py:94-99`.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=68cf823b7bc74d1da92db4bf19395ea8&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=68cf823b7bc74d1da92db4bf19395ea8&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:** 608:609
**Comment:**
*Incorrect Condition Logic: The override-presence check uses truthiness
instead of key presence, so an operator-provided empty language pack (for
example `{}`) is treated as “not provided” and the versioned script tag is
emitted anyway. This breaks the documented precedence of
`COMMON_BOOTSTRAP_OVERRIDES_FUNC`; check whether the `language_pack` key is
present/non-None rather than truthy.
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%2F41780&comment_hash=3241dd5f618148048b77ae333732a533a0236dfa1f0816d22d8c48be4bd9e60e&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41780&comment_hash=3241dd5f618148048b77ae333732a533a0236dfa1f0816d22d8c48be4bd9e60e&reaction=dislike'>👎</a>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
