-
Notifications
You must be signed in to change notification settings - Fork 16.5k
fix(d3-format): call setupFormatters synchronously to apply D3 format… #35529
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(d3-format): call setupFormatters synchronously to apply D3 format… #35529
Conversation
… when a non‑English language is selected
Code Review Agent Run #df2b36Actionable 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've completed my review and didn't find any issues.
Files scanned
| File Path | Reviewed |
|---|---|
| superset-frontend/src/preamble.ts | ✅ |
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.
mistercrunch
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Claude's analysis:
PR #35529 Review - D3 Format Fix
What Changed
The PR moves the setupFormatters() call outside the async IIFE (immediately invoked function
expression) to execute it synchronously right after getBootstrapData(), instead of inside the async
block where it runs after the language pack is fetched.
Location: superset-frontend/src/preamble.ts:58-93
Why It Matters
This is a regression fix for a bug introduced in PR #34119. Here's the sequence of events:
- Before PR #34119: Everything ran synchronously, D3 formatters worked correctly for all languages
- PR #34119 introduced: Language pack async loading - moved setupFormatters() inside the async
block - Regression: When non-English languages are selected, the language pack loading triggers a second
call to configure() (line 70), which resets D3 formatters to language-specific defaults, overwriting
the custom D3_FORMAT from config.py - This PR fixes: By calling setupFormatters() synchronously first, then again after language pack
loads, the custom formats are properly applied
Behavioral Impact
Without this fix:
- English users: ✅ Custom D3_FORMAT works (no language pack fetch, no configure() reset)
- Non-English users: ❌ Custom D3_FORMAT ignored, reverts to D3 defaults
With this fix:
- All users: ✅ Custom D3_FORMAT respected for number/currency formatting
Is It Safe to Merge?
YES, this is super-safe. Here's why:
- No breaking changes: Only moves existing code, doesn't modify logic
- Small scope: 10-line change (5 additions, 5 deletions) in a single file
- Idempotent operation: setupFormatters() can safely be called multiple times - the second call
(inside async block) simply reinforces the settings - Fixes documented regression: Addresses issue #35524 with clear before/after screenshots
- No dependencies: The function only needs bootstrapData which is already available synchronously
- Tested pattern: This restores the original synchronous behavior that worked in v5.0.0
Technical Details
The setupFormatters() function from superset-frontend/src/setup/setupFormatters.ts configures D3's
number/time formatting globally. It needs to run:
- Early - before any charts/visualizations render
- After configure() calls - to ensure custom formats aren't overwritten
The fix satisfies both requirements by calling it synchronously, then again inside the async block
(defensive redundancy).
Recommendation
✅ SAFE TO MERGE - This is a clean regression fix with no risk and clear user benefit for
internationalization.
… when a non‑English language is selected
SUMMARY
Call
setupFormatterssynchronously so that the custom D3_FORMAT is applied when a non‑English language is selected. See related issue #35524 for more details.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
BEFORE

AFTER

TESTING INSTRUCTIONS
ADDITIONAL INFORMATION