fix(translation): loading translations in menu#35640
fix(translation): loading translations in menu#35640jakubhruby wants to merge 21 commits intoapache:masterfrom
Conversation
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Status |
|---|---|---|
 |
Blocking main thread with top-level await ▹ view | |
 |
Top-level await without module configuration ▹ view |
Files scanned
| File Path | Reviewed |
|---|---|
| superset-frontend/src/views/index.tsx | ✅ |
| superset-frontend/src/views/menu.tsx | ✅ |
| superset-frontend/src/preamble.ts | ✅ |
| superset-frontend/src/embedded/index.tsx | ✅ |
| superset-frontend/src/features/home/Menu.tsx | ✅ |
| superset-frontend/webpack.config.js | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
| await i18nLoadJob; | ||
|
|
||
| ReactDOM.render(<App />, document.getElementById('app')); |
There was a problem hiding this comment.
Blocking main thread with top-level await 
Tell me more
What is the issue?
Top-level await blocks the main thread during i18n loading, preventing any UI rendering or user interaction until translation loading completes.
Why this matters
This creates a poor user experience with a blank screen during network delays or slow translation loading, and prevents showing loading indicators or fallback content to users.
Suggested change ∙ Feature Preview
Replace top-level await with asynchronous initialization that shows a loading state while translations load, then renders the app:
import { i18nLoadJob } from '../preamble';
const initializeApp = async () => {
// Show loading indicator
ReactDOM.render(<LoadingSpinner />, document.getElementById('app'));
// Load translations
await i18nLoadJob;
// Render main app
ReactDOM.render(<App />, document.getElementById('app'));
};
initializeApp();Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
jakubhruby
changed the title
jakubhruby
requested review from
betodealmeida,
craig-rueda,
dpgaspar,
eschutho,
geido,
kgabryje,
nytai,
rusackas,
sadpandajoe and
villebro
as code owners
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #35640 +/- ##
=======================================
Coverage 64.36% 64.36%
=======================================
Files 2569 2569
Lines 134745 134747 +2
Branches 31278 31278
=======================================
+ Hits 86731 86733 +2
Misses 46516 46516
Partials 1498 1498
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@dosu didn't someone recently file an issue about loading french translations in their deployment? Maybe this is relevant? |
|
I applied these changes to my slightly modified 6.0 branch and this long standing issue is finally fixed. Good work! |
|
I was celebrating too early. With fix these feature flags do not work anymore "ENABLE_DASHBOARD_SCREENSHOT_ENDPOINTS": True, |
|
@jakubhruby thanks for working on this — wanted to flag a couple of things before we can move forward:
Once those are addressed I'll take another look. Appreciate the patch — the screenshots in the description make the bug very clear. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| let i18nLoadResolve: (value?: unknown) => void; | ||
|
|
||
| export const I18nLoadJob = new Promise(resolve => { | ||
| i18nLoadResolve = resolve; | ||
| }); | ||
|
|
||
| export default function initPreamble(): Promise<void> { | ||
| if (initPromise) { | ||
| return initPromise; | ||
| return initPromise.then(() => i18nLoadResolve()); | ||
| } |
There was a problem hiding this comment.
🔴 Architect Review — CRITICAL
I18nLoadJob is never resolved during the initial initPreamble execution: the resolver is only invoked in the if (initPromise) branch, but the first call (triggered at module load) sets initPromise and returns without ever calling i18nLoadResolve. Because the SPA, menu, and embedded entrypoints now await I18nLoadJob at top level, their module evaluation can block indefinitely and prevent the UI from rendering.
Suggestion: Resolve I18nLoadJob from the same async flow that loads translations in initPreamble (success and fallback paths), ensuring the resolver is called on the initial run and not just on subsequent calls. For example, call i18nLoadResolve() in the finally of the language-pack-loading logic before returning the resolved initPromise.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is an **Architect / Logical Review** comment left during a code review. These reviews are first-class, important findings — not optional suggestions. Do NOT dismiss this as a 'big architectural change' just because the title says architect review; most of these can be resolved with a small, localized fix once the intent is understood.
**Path:** superset-frontend/src/preamble.ts
**Line:** 46:55
**Comment:**
*CRITICAL: I18nLoadJob is never resolved during the initial initPreamble execution: the resolver is only invoked in the `if (initPromise)` branch, but the first call (triggered at module load) sets initPromise and returns without ever calling `i18nLoadResolve`. Because the SPA, menu, and embedded entrypoints now `await I18nLoadJob` at top level, their module evaluation can block indefinitely and prevent the UI from rendering.
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.
If a suggested approach is provided above, use it as the authoritative instruction. If no explicit code suggestion is given, you MUST still draft and apply your own minimal, localized fix — do not punt back with 'no suggestion provided, review manually'. Keep the change as small as possible: add a guard clause, gate on a loading state, reorder an await, wrap in a conditional, etc. Do not refactor surrounding code or expand scope beyond the finding.
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
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
| let i18nLoadResolve: (value?: unknown) => void; | ||
|
|
||
| export const I18nLoadJob = new Promise(resolve => { | ||
| i18nLoadResolve = resolve; | ||
| }); | ||
|
|
||
| export default function initPreamble(): Promise<void> { | ||
| if (initPromise) { | ||
| return initPromise; | ||
| return initPromise.then(() => i18nLoadResolve()); |
There was a problem hiding this comment.
Suggestion: I18nLoadJob is only resolved in the if (initPromise) branch, but the first initialization path never calls i18nLoadResolve, so the promise can stay pending forever. Resolve (and ideally reject) I18nLoadJob from the primary async initialization completion path, not only on subsequent calls. [logic error]
Severity Level: Critical 🚨
- ❌ Main app entry `views/index.tsx` hangs before render.
- ❌ Menu entry `views/menu.tsx` blocks on unresolved I18nLoadJob.
- ❌ Embedded dashboards `embedded/index.tsx` never start authentication.
- ⚠️ Translations and bootstrap flow cannot complete successfully.Steps of Reproduction ✅
1. Build and load the main web UI entrypoint `superset-frontend/src/views/index.tsx`,
which imports `{ default as initPreamble, I18nLoadJob }` from `src/preamble` at line 9 and
then performs a top-level `await I18nLoadJob` at line 12.
2. During module loading of `views/index.tsx`, the imported module
`superset-frontend/src/preamble.ts` is evaluated: it defines `I18nLoadJob` as `new
Promise(resolve => { i18nLoadResolve = resolve; })` at lines 48–50 and then immediately
calls `initPreamble().catch(...)` at lines 139–141, which sets `initPromise` at line 57
and starts the async initialization, but does not call `i18nLoadResolve` anywhere in that
first execution path.
3. After `preamble.ts` finishes evaluating, control returns to `views/index.tsx` and
execution reaches the top-level `await I18nLoadJob` at line 12; at this point
`I18nLoadJob` is still pending because its resolver `i18nLoadResolve` (declared at line
46) has not been invoked.
4. The only place `i18nLoadResolve` is ever called is inside `initPreamble()` when
`initPromise` is already non-null, via `return initPromise.then(() => i18nLoadResolve());`
at line 54, but this branch is only executed on subsequent calls to `initPreamble()`;
because `views/index.tsx` awaits `I18nLoadJob` before it ever calls `initPreamble()`
itself (lines 16–22), that second call never happens, `I18nLoadJob` never resolves, and
the main app (and similarly `views/menu.tsx` awaiting `I18nLoadJob` at lines 11–12, and
`embedded/index.tsx` chaining `I18nLoadJob.then(...)` at lines 139–145) hangs indefinitely
waiting for a promise that is never settled.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/preamble.ts
**Line:** 46:54
**Comment:**
*Logic Error: `I18nLoadJob` is only resolved in the `if (initPromise)` branch, but the first initialization path never calls `i18nLoadResolve`, so the promise can stay pending forever. Resolve (and ideally reject) `I18nLoadJob` from the primary async initialization completion path, not only on subsequent calls.
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| import { logging } from '@apache-superset/core/utils'; | ||
| import initPreamble from 'src/preamble'; | ||
|
|
||
| await I18nLoadJob; |
There was a problem hiding this comment.
Suggestion: Awaiting I18nLoadJob at module top level blocks app boot before initPreamble() is invoked in this file, so when that job is not resolved yet the SPA never mounts. Gate rendering by awaiting initPreamble() directly (or ensure the job is guaranteed to settle before this await). [possible bug]
Severity Level: Critical 🚨
- ❌ SQL Lab /sqllab/ page never renders React application.
- ❌ All spa.html-based views render blank, no UI.Steps of Reproduction ✅
1. Build and serve the frontend with this PR so that webpack uses the `spa` entry defined
in `superset-frontend/webpack.config.js:7-18`, where `experiments.topLevelAwait` is
enabled (`webpack.config.js:7-10`) and `spa` is configured as
`addPreamble('src/views/index.tsx')` (`webpack.config.js:15-17`), which prepends
`src/preamble.ts` via `PREAMBLE` (`webpack.config.js:259-265`).
2. In a browser, navigate to SQL Lab at `/sqllab/`, which is handled by `SqllabView.root`
in `superset/views/sqllab.py:33-40`; this method calls `self.render_app_template(payload)`
at `sqllab.py:40`, which delegates to `BaseSupersetView.render_app_template` in
`superset/views/base.py:11-35` to render `superset/spa.html` using the `spa` webpack entry
by default (`get_spa_template_context` in `base.py:568-587`).
3. When the browser loads the `spa` bundle, the prepended module
`superset-frontend/src/preamble.ts` executes first: it defines `I18nLoadJob` as a new
Promise with resolver `i18nLoadResolve` at `preamble.ts:46-50` and defines
`initPreamble()` at `preamble.ts:52-133`; at the bottom it immediately calls
`initPreamble()` at `preamble.ts:136-139` to start initialization, but `i18nLoadResolve`
is only wired in the `if (initPromise)` branch of `initPreamble()` (`preamble.ts:52-55`),
which is not taken on this first call, so `I18nLoadJob` remains pending and is never
resolved.
4. After `preamble.ts` finishes its initial call, webpack evaluates the entry module
`superset-frontend/src/views/index.tsx`; module execution reaches the top-level `await
I18nLoadJob;` at `index.tsx:26` before any code in this file can call `initPreamble()`,
and because `I18nLoadJob` never resolves (per step 3), the module evaluation is suspended
indefinitely and the code path that imports `./App` and calls `ReactDOM.render(<App />,
appMountPoint);` at `index.tsx:35-36` is never executed, so the SQL Lab page (and any
other view using the `spa` entry) never mounts the React app and appears as a blank
spinner/empty page.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/views/index.tsx
**Line:** 26:26
**Comment:**
*Possible Bug: Awaiting `I18nLoadJob` at module top level blocks app boot before `initPreamble()` is invoked in this file, so when that job is not resolved yet the SPA never mounts. Gate rendering by awaiting `initPreamble()` directly (or ensure the job is guaranteed to settle before this await).
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| import { I18nLoadJob } from '../preamble'; | ||
| import querystring from 'query-string'; | ||
|
|
||
| await I18nLoadJob; |
There was a problem hiding this comment.
Suggestion: This top-level await can block the menu bundle forever if I18nLoadJob is still pending, preventing the menu app from initializing. Use initPreamble()-based startup flow (or a guaranteed-settling synchronization promise) before rendering. [possible bug]
Severity Level: Critical 🚨
- ❌ Menu React app never renders on backend-rendered pages.
- ❌ Backend views fail to show navigation menu content.
- ⚠️ I18n translation loading blocks entire menu entrypoint.Steps of Reproduction ✅
1. The `menu` webpack entrypoint is configured in `superset-frontend/webpack.config.js`
via `menu: addPreamble('src/views/menu.tsx')`, which prepends `src/preamble.ts` before
`src/views/menu.tsx` (verified by `addPreamble` definition in `webpack.config.js:12–14`
from the Read output).
2. When the menu bundle loads in the browser, module `superset-frontend/src/preamble.ts`
runs first: it defines `I18nLoadJob` at lines 48–50 as `new Promise(resolve => {
i18nLoadResolve = resolve; })`, and calls `initPreamble()` once at lines 136–141 to kick
off initialization.
3. `initPreamble()` in `src/preamble.ts:52–55` only calls `i18nLoadResolve()` in the
branch `if (initPromise) { return initPromise.then(() => i18nLoadResolve()); }`; on the
first (module-internal) call `initPromise` is `null`, so that branch is skipped and the
asynchronous initialization body at lines 57–133 never invokes `i18nLoadResolve`, leaving
`I18nLoadJob` pending (confirmed by `Grep` showing `i18nLoadResolve` used only on lines
46–54).
4. After `preamble.ts` finishes evaluating, module `superset-frontend/src/views/menu.tsx`
executes and hits the top-level `await I18nLoadJob;` at line 38, which waits on a promise
that never settles; as a result the code that sets up the Redux store and renders `<Menu
/>` into `#app-menu` at lines 42–45 and 50–71 never runs, so any page that loads the
`menu` bundle (per step 1) will see the menu React app fail to initialize and the header
menu never render.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/views/menu.tsx
**Line:** 38:38
**Comment:**
*Possible Bug: This top-level await can block the menu bundle forever if `I18nLoadJob` is still pending, preventing the menu app from initializing. Use `initPreamble()`-based startup flow (or a guaranteed-settling synchronization promise) before rendering.
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
jakubhruby
changed the title
jakubhruby
changed the title
|
Some parts of this PR have been already fixed with implementing async function initPreamble. So I have removed i18nLoad job from this PR code. |
|
/review |
|
Bito In-Progress Reviews Aborted |
|
/abort |
|
/review |
Code Review Agent Run #139410Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Code Review Agent Run #9f17dbActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
@rusackas babel-extract chekc is running for 3+ hours, but the others are green |
5 participants
SUMMARY
Translations need to be fully loaded before app is started - it's insuperset-frontend/src/preamble.tswhere there even is a comment sayingLoad language pack before anything else, but in fact there is no wait for async translation load.Some menu items are not translated.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before - translated and non-translated mixed up
After - all strings are loaded
TESTING INSTRUCTIONS
Reoad page and watch all strings are translated (requires fully translated localization, e.g. CS added in #35639)
ADDITIONAL INFORMATION