codeant-ai-for-open-source[bot] commented on code in PR #36893:
URL: https://github.com/apache/superset/pull/36893#discussion_r2663215412
##########
superset-frontend/src/preamble.ts:
##########
@@ -44,67 +44,84 @@ 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 });
-}
-
-// 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
+ 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}/`,
});
Review Comment:
**Suggestion:** Race condition: the code uses `SupersetClient.get` to fetch
the language pack immediately after calling `setupClient(...)`, but
`setupClient` initializes the client asynchronously and does not expose a
promise to await; this can cause `SupersetClient.get` to fail if the client
isn't fully initialized. Use the browser `fetch` API to load the language pack
here (independent of SupersetClient initialization) and handle non-OK
responses. [race condition]
**Severity Level:** Minor ⚠️
```suggestion
// Second call to configure to set the language pack (use fetch so
we don't depend on SupersetClient being initialized)
const resp = await fetch(`/superset/language_pack/${lang}/`);
if (!resp.ok) {
throw new Error(`Failed to fetch language pack: ${resp.status}`);
}
const json = await resp.json();
```
<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:** 77:80
**Comment:**
*Race Condition: Race condition: the code uses `SupersetClient.get` to
fetch the language pack immediately after calling `setupClient(...)`, but
`setupClient` initializes the client asynchronously and does not expose a
promise to await; this can cause `SupersetClient.get` to fail if the client
isn't fully initialized. Use the browser `fetch` API to load the language pack
here (independent of SupersetClient initialization) and handle non-OK responses.
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,19 @@
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);
+ }
+ })();
Review Comment:
**Suggestion:** The top-level async IIFE returns a promise that isn't
handled; if it rejects it will create an unhandled promise rejection — append a
`.catch(...)` to the IIFE invocation to log unexpected bootstrap failures.
[possible bug]
**Severity Level:** Critical 🚨
```suggestion
})().catch(err => {
// Ensure any unhandled rejections from the bootstrap sequence are
logged.
// eslint-disable-next-line no-console
console.error('Unhandled error during app initialization', err);
});
```
<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:** 36:36
**Comment:**
*Possible Bug: The top-level async IIFE returns a promise that isn't
handled; if it rejects it will create an unhandled promise rejection — append a
`.catch(...)` to the IIFE invocation to log unexpected bootstrap failures.
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/views/base.py:
##########
@@ -475,7 +475,9 @@ def cached_common_bootstrap_data( # pylint:
disable=unused-argument
and bool(available_specs[GSheetsEngineSpec])
)
- language = locale.language if locale else "en"
+ language = (
+ locale.language if locale else app.config.get("BABEL_DEFAULT_LOCALE",
"en")
+ )
Review Comment:
**Suggestion:** The code assumes `locale` has a `.language` attribute; if
callers pass a plain string (or another truthy non-Locale type), accessing
`locale.language` will raise AttributeError. Detect `Locale` vs `str` and
handle both to avoid runtime errors. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
if isinstance(locale, Locale):
language = locale.language
elif isinstance(locale, str):
language = locale
else:
language = app.config.get("BABEL_DEFAULT_LOCALE", "en")
```
<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:** 478:480
**Comment:**
*Possible Bug: The code assumes `locale` has a `.language` attribute;
if callers pass a plain string (or another truthy non-Locale type), accessing
`locale.language` will raise AttributeError. Detect `Locale` vs `str` and
handle both to avoid runtime errors.
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/preamble.ts:
##########
@@ -44,67 +44,84 @@ 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 });
-}
-
-// 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
+ 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');
}
+ }
+
+ // Continue with rest of setup
+ initFeatureFlags(bootstrapData.common.feature_flags);
+
+ setupColors(
+ bootstrapData.common.extra_categorical_color_schemes,
+ bootstrapData.common.extra_sequential_color_schemes,
+ );
+
+ setupDashboardComponents();
+
+ const getMe = makeApi<void, User>({
+ method: 'GET',
+ endpoint: '/api/v1/me/',
});
- }
-})();
+
+ if (bootstrapData.user?.isActive) {
+ document.addEventListener('visibilitychange', () => {
+ if (document.visibilityState === 'visible') {
+ getMe().catch(() => {
+ // SupersetClient will redirect to login on 401
+ });
+ }
+ });
+ }
+ })();
+
+ return initPromise;
+}
+
+// This module is prepended to multiple webpack entrypoints (see
`webpack.config.js`).
+// Kick off initialization eagerly, while still allowing entrypoints to
`await` it
+// before rendering when needed (e.g. the login page).
+initPreamble().catch(err => {
+ console.warn('Preamble initialization failed.', err);
+});
Review Comment:
**Suggestion:** If the eager initialization fails, `initPromise` is left as
a rejected promise and subsequent callers of `initPreamble()` will immediately
receive the same rejected promise (no retry). Reset `initPromise` to `null`
when initialization fails so callers can retry initialization. [state &
lifecycle]
**Severity Level:** Minor ⚠️
```suggestion
// Allow retries by clearing the cached promise on failure
initPromise = null;
```
<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:** 127:127
**Comment:**
*State Lifecycle: If the eager initialization fails, `initPromise` is
left as a rejected promise and subsequent callers of `initPreamble()` will
immediately receive the same rejected promise (no retry). Reset `initPromise`
to `null` when initialization fails so callers can retry initialization.
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]
