fix(i18n): sanitize browser locales like en-US@posix#10219
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
I have read the CLA Document and I hereby sign the CLA |
|
|
||
| export function safeLocale(locale: string | null | undefined): string { | ||
| if (locale && isValidLocale(locale)) { | ||
| return locale; |
There was a problem hiding this comment.
isValidLocale normalizes underscores only for validation, but this returns the raw value. A configured en_US passes the check and still reaches I18nProvider as an invalid locale; should this return normalizeBrowserLocale(locale)?
There was a problem hiding this comment.
Good catch — fixed in the latest commit.
isValidLocale was rewriting _ → - only inside the check, then safeLocale returned the original string, so a configured en_US could pass validation and still reach I18nProvider raw.
Valid config locales now go through normalizeBrowserLocale before handoff (same path as browser tags). Added a regression test: safeLocale("en_US") === "en-US".
There was a problem hiding this comment.
Good catch — fixed in 0a4f6aa.
safeLocale now always runs valid config through normalizeBrowserLocale, so underscore forms like en_US are canonicalized to en-US before they reach I18nProvider. Added a unit test for that path.
isValidLocale accepts underscore forms (en_US) for validation only. safeLocale now always returns a BCP 47 tag so I18nProvider never gets raw underscore locales. Addresses review on marimo-team#10219.
|
Thanks @Sanjays2402 — addressed your review:
Ready for another look. |
|
Addressed @Sanjays2402's review: |
|
@Light2Dark since you attempted a fix previously can you please evaluate this PR? |
There was a problem hiding this comment.
Pull request overview
Fixes a frontend crash caused by non‑BCP47 browser locale tags (e.g. en-US@posix) being passed into Intl/react-aria, which can throw RangeError and prevent notebooks (including the intro tutorial) from rendering.
Changes:
- Add locale tag normalization/validation and route
LocaleProviderlocale selection through it with anen-USfallback. - Add regression tests covering Playwright/Chromium-style locale modifiers and other malformed tags.
- Apply the same normalization to the data-table column-formatting default locale.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| frontend/src/core/i18n/locale-provider.tsx | Introduces locale normalization/validation helpers and uses them to provide a safe locale to I18nProvider. |
| frontend/src/core/i18n/tests/locale-provider.test.tsx | Adds/updates tests to cover normalized browser locales and fallback behavior. |
| frontend/src/components/data-table/column-formatting/feature.ts | Uses the normalization helper for the data-table’s default formatting locale. |
Comments suppressed due to low confidence (1)
frontend/src/core/i18n/locale-provider.tsx:72
safeLocalevalidates the raw configured locale before normalization, so config values likeen-US@posixoren_US.UTF-8(both mentioned as inputs to normalize) are treated as invalid and ignored. Normalize/strip common modifiers for the configured value before deciding to fall back tonavigator.language.
if (locale && isValidLocale(locale)) {
return normalizeBrowserLocale(locale);
}
| if (Intl.NumberFormat.supportedLocalesOf([normalized]).length === 0) { | ||
| return false; | ||
| } | ||
| new Intl.Locale(normalized); | ||
| return true; |
| import type { DataType } from "@/core/kernel/messages"; | ||
| import { normalizeBrowserLocale } from "@/core/i18n/locale-provider"; | ||
| import { logNever } from "@/utils/assertNever"; |
Chromium/Playwright can report navigator.language as en-US@posix, which throws RangeError in Intl/react-aria and crashes the app before render. Normalize browser tags (strip @/charset modifiers), validate with supportedLocalesOf, and fall back to en-US. Reuse normalization for the data table default locale. Closes marimo-team#9938
for more information, see https://pre-commit.ci
isValidLocale accepts underscore forms (en_US) for validation only. safeLocale now always returns a BCP 47 tag so I18nProvider never gets raw underscore locales. Addresses review on marimo-team#10219.
Address review feedback while Light2Dark evaluates: - Move normalize/isValid/safeLocale into core/i18n/locale.ts so the data-table feature does not import the React LocaleProvider module - Guard missing Intl.Locale (do not force en-US when only NumberFormat is available) - Normalize config locales with @posix / .UTF-8 before navigator fallback - Prefer vi.stubGlobal for navigator mocks (no leaked navigator object) - Tests: 17 passing including de-DE@posix and config-modifier cases Refs marimo-team#9938
0a4f6aa to
ff3c6da
Compare
Evaluation notes for @Light2Dark (and anyone reviewing)@kirangadhave asked for your eyes since you previously attempted this in #9945 — thank you. Here's a concise self-eval of this PR vs that attempt, plus what I just pushed to make review easier. Problem (issue #9938)Chromium/Playwright can report How this differs from closed #9945
Neither approach is wrong; this one is a bit more aggressive about always supplying a concrete locale so we never depend on react-aria’s internal default path for the crash case. Latest push (
|
for more information, see https://pre-commit.ci
This pull request was AI-assisted; human-reviewed line by line.
Summary
Some Chromium/Playwright environments report navigator.language as en-US@posix. Passing that tag straight into react-aria / Intl throws RangeError and the notebook never renders (including the stock intro tutorial).
Changes
Closes #9938
I have read the CLA Document and I hereby sign the CLA