codeant-ai-for-open-source[bot] commented on code in PR #36893:
URL: https://github.com/apache/superset/pull/36893#discussion_r2769634594
##########
superset-frontend/src/preamble.ts:
##########
@@ -38,67 +39,102 @@ import 'dayjs/plugin/duration';
import 'dayjs/plugin/updateLocale';
import 'dayjs/plugin/localizedFormat';
-configure();
+let initPromise: Promise<void> | null = null;
-// Set hot reloader config
-if (process.env.WEBPACK_MODE === 'development') {
- setHotLoaderConfig({ logLevel: 'debug', trackTailUpdates: false });
-}
+const LANGUAGE_PACK_REQUEST_TIMEOUT_MS = 5000;
-// Grab initial bootstrap data
-const bootstrapData = getBootstrapData();
-
-setupFormatters(
- bootstrapData.common.d3_format,
- bootstrapData.common.d3_time_format,
-);
-
-// Setup SupersetClient early so we can fetch language pack
-setupClient({ appRoot: applicationRoot() });
-
-// Load language pack before anything else
-(async () => {
- const lang = bootstrapData.common.locale || 'en';
- if (lang !== 'en') {
- try {
- // Second call to configure to set the language pack
- const { json } = await SupersetClient.get({
- endpoint: `/superset/language_pack/${lang}/`,
- });
- configure({ languagePack: json as LanguagePack });
- dayjs.locale(lang);
- } catch (err) {
- console.warn(
- 'Failed to fetch language pack, falling back to default.',
- err,
- );
- configure();
- dayjs.locale('en');
- }
+export default function initPreamble(): Promise<void> {
+ if (initPromise) {
+ return initPromise;
}
- // Continue with rest of setup
- initFeatureFlags(bootstrapData.common.feature_flags);
+ initPromise = (async () => {
+ configure();
- setupColors(
- bootstrapData.common.extra_categorical_color_schemes,
- bootstrapData.common.extra_sequential_color_schemes,
- );
+ // Set hot reloader config
+ if (process.env.WEBPACK_MODE === 'development') {
+ setHotLoaderConfig({ logLevel: 'debug', trackTailUpdates: false });
+ }
- setupDashboardComponents();
+ // Grab initial bootstrap data
+ const bootstrapData = getBootstrapData();
- const getMe = makeApi<void, User>({
- method: 'GET',
- endpoint: '/api/v1/me/',
- });
+ setupFormatters(
+ bootstrapData.common.d3_format,
+ bootstrapData.common.d3_time_format,
+ );
- if (bootstrapData.user?.isActive) {
- document.addEventListener('visibilitychange', () => {
- if (document.visibilityState === 'visible') {
- getMe().catch(() => {
- // SupersetClient will redirect to login on 401
+ // 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
+ const lang = bootstrapData.common.locale || 'en';
+ 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}/`);
Review Comment:
**Suggestion:** The language pack URL is built with `makeUrl` and already
includes the `/superset` prefix, which will cause a double-prefix (e.g.
`/superset/superset/...`) in deployments where `makeUrl` prepends the
application root or URL prefix, leading to 404s for the language pack and
silent fallback to English translations. [logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Login page translations revert to English unexpectedly.
- ❌ Any pre-render pages use fallback locale.
- ⚠️ Console shows language pack fetch failure warnings.
```
</details>
```suggestion
const languagePackUrl = makeUrl(`/language_pack/${lang}/`);
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Ensure the frontend is served with a non-English default locale encoded
in bootstrap
data. The bootstrap consumer reads the data-bootstrap attribute in
src/utils/getBootstrapData.ts (lines 24-34). Set that attribute (server-side
or by
starting the app with BABEL_DEFAULT_LOCALE) so bootstrapData.common.locale
!== 'en'.
2. Load a page that includes the preamble module (this module is prepended
to multiple
webpack entrypoints). The preamble's init function is defined at
superset-frontend/src/preamble.ts:46 and is invoked eagerly at the bottom of
the same file
(see lines ~136-140 where initPreamble().catch(...) is called). This triggers
initPreamble() during page load.
3. Inside initPreamble(), the selected lang is computed at preamble.ts:72
(const lang =
bootstrapData.common.locale || 'en';). For a non-en locale the code enters
the fetch
branch and constructs the language pack URL at preamble.ts:80 (const
languagePackUrl =
makeUrl(`/superset/language_pack/${lang}/`);). The code then calls
fetch(languagePackUrl)
at preamble.ts:81-83.
4. If makeUrl already prefixes the application root (for example, adding
'/superset' or a
URL prefix), passing a path that already contains '/superset' yields a
doubled prefix
(e.g. '/superset/superset/language_pack/…') so the network request performed
at
preamble.ts:81 results in a 404. The fetch's non-ok response (or network
404) causes the
code path at preamble.ts:84-89 to throw and the catch at preamble.ts:90-97
to run, which
calls configure() fallback and dayjs.locale('en'), producing English
translations despite
the configured non-English locale. Verify by observing the failed network
request URL in
browser DevTools Network tab and the console warning printed at
preamble.ts:91-94 ("Failed
to fetch language pack, falling back to default.").
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/preamble.ts
**Line:** 80:80
**Comment:**
*Logic Error: The language pack URL is built with `makeUrl` and already
includes the `/superset` prefix, which will cause a double-prefix (e.g.
`/superset/superset/...`) in deployments where `makeUrl` prepends the
application root or URL prefix, leading to 404s for the language pack and
silent fallback to English translations.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset-frontend/src/views/index.tsx:
##########
@@ -19,6 +19,20 @@
import 'src/public-path';
import ReactDOM from 'react-dom';
-import App from './App';
+import initPreamble from 'src/preamble';
-ReactDOM.render(<App />, document.getElementById('app'));
+const appMountPoint = document.getElementById('app');
+
+if (appMountPoint) {
+ (async () => {
+ try {
+ await initPreamble();
+ } finally {
+ const { default: App } = await import(/* webpackMode: "eager" */
'./App');
+ ReactDOM.render(<App />, appMountPoint);
+ }
+ })().catch(err => {
+ // eslint-disable-next-line no-console
+ console.error('Unhandled error during app initialization', err);
+ });
+}
Review Comment:
**Suggestion:** If the `#app` mount element is missing from the DOM (for
example due to an HTML/template regression), the current code does nothing and
fails silently, leaving users with a blank page and no diagnostics; adding an
explicit error log makes this failure mode visible and easier to debug. [logic
error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Login and main UI render produce a blank page.
- ⚠️ No diagnostic message appears in console.
- ⚠️ Template regressions harder to debug without logs.
```
</details>
```suggestion
} else {
// eslint-disable-next-line no-console
console.error('Failed to initialize app: mount point element with id "app"
was not found.');
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Inspect the mount logic in src/views/index.tsx and confirm the guard uses
document.getElementById('app') at line 24 and conditional at
src/views/index.tsx:26-38.
2. Reproduce missing-mount by serving an HTML page without the <div
id="app"> element (or
by removing it in browser DevTools) so document.getElementById('app')
returns null at page
load.
3. Reload the page; code at src/views/index.tsx:26 evaluates false and the
if-block is
skipped, causing no further logs or rendering to occur.
4. Observe a blank page with no console diagnostic from this entrypoint; the
suggested
else branch would log an explicit error to aid debugging.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/views/index.tsx
**Line:** 38:38
**Comment:**
*Logic Error: If the `#app` mount element is missing from the DOM (for
example due to an HTML/template regression), the current code does nothing and
fails silently, leaving users with a blank page and no diagnostics; adding an
explicit error log makes this failure mode visible and easier to debug.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
--
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]
