fix(scripts): pin the catalog generation locale so assets are reproducible - #1129
Open
sukvvon wants to merge 1 commit into
Open
fix(scripts): pin the catalog generation locale so assets are reproducible#1129sukvvon wants to merge 1 commit into
sukvvon wants to merge 1 commit into
Conversation
Copy link
Copy Markdown
Member
Sorry, something went wrong.
Copy link
Copy Markdown
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
tanstack-com | c1160d3 | Commit Preview URL Branch Preview URL |
Aug 08 2026, 03:09 AM |
Sorry, something went wrong.
Copy link
Copy Markdown
WalkthroughThe chart catalog and asset generation scripts now load a shared locale shim. The shim defaults omitted date and number formatting locales to en-US while preserving explicit locale arguments. ChangesCatalog locale determinism
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. ❤️ ShareComment @coderabbitai help to get the list of available commands. |
Sorry, something went wrong.
Copy link
Copy Markdown
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agentsVerify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. Inline comments: In `@scripts/pin-catalog-locale.ts`: - Around line 25-29: Update all three locale wrapper methods in scripts/pin-catalog-locale.ts to default only when locales is undefined, preserving explicit null and other invalid values for native validation. Replace the nullish fallback in each originalToLocaleDateString call with an undefined-only conditional while keeping the existing GENERATION_LOCALE default and call flow unchanged.
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info ⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a07e3ab6-8fdf-40e5-9b53-808ec6b3aa48
📥 CommitsReviewing files that changed from the base of the PR and between ece8227 and c1160d3.
📒 Files selected for processing (3)- scripts/generate-charts-landing-assets.ts
- scripts/generate-charts-landing-catalog.ts
- scripts/pin-catalog-locale.ts
Sorry, something went wrong.
Comment thread
scripts/pin-catalog-locale.ts
Comment on lines
+25
to
+29
| return originalToLocaleDateString.call( | ||
| this, | ||
| locales ?? GENERATION_LOCALE, | ||
| options, | ||
| ) |
Copy link
Copy Markdown
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- file outline ---\n'
ast-grep outline scripts/pin-catalog-locale.ts --view expanded || true
printf '\n--- relevant file contents ---\n'
cat -n scripts/pin-catalog-locale.ts
printf '\n--- native behavioral probe ---\n'
node - <<'JS'
const dates = [
new Date('2026-08-20T12:34:56Z'),
new Date('invalid date')
];
const options = {};
for (const date of dates) {
for (const fn of ['toLocaleDateString','toLocaleTimeString','toLocaleString']) {
const original = Date.prototype[fn] || Object.getPrototypeOf(new Date())?.[fn];
console.log(original, fn, date.toISOString());
for (const input of [undefined, null, 'en-US', 'invalid-locale']) {
try {
const result = original.call(date, input, options);
console.log(`${fn}(${JSON.stringify(input)}) -> ${result}`);
} catch (error) {
console.log(`${fn}(${JSON.stringify(input)}) throws ${error.name}: ${error.message}`);
}
}
}
}
JS
printf '\n--- search for generated default / helpers usages ---\n'
rg -n "GENERATION_LOCALE|pin-catalog|Catalog|locale" scripts package.json -S || trueRepository: TanStack/tanstack.com
Length of output: 250
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- file outline ---'
ast-grep outline scripts/pin-catalog-locale.ts --view expanded || true
printf '%s\n' ''
printf '%s\n' '--- relevant file contents ---'
cat -n scripts/pin-catalog-locale.ts
printf '%s\n' ''
printf '%s\n' '--- native behavioral probe ---'
node - <<'JS'
const dates = [new Date('2026-08-20T12:34:56Z'), new Date('invalid date')];
const options = {};
for (const date of dates) {
for (const fn of ['toLocaleDateString','toLocaleTimeString','toLocaleString']) {
const dateProto = Object.getPrototypeOf(new Date());
const original = dateProto[fn];
console.log(dateProto, fn, date.toISOString());
for (const input of [undefined, null, 'en-US', 'invalid-locale']) {
try {
const result = original.call(date, input, options);
console.log(`${fn}(${JSON.stringify(input)}) -> ${result}`);
} catch (error) {
console.log(`${fn}(${JSON.stringify(input)}) throws ${error.name}: ${error.message}`);
}
}
}
}
JS
printf '%s\n' ''
printf '%s\n' '--- search for generated default / helpers usages ---'
rg -n "GENERATION_LOCALE|pin-catalog|Catalog|locale" scripts package.json package-lock.json bun.lockb pnpm-lock.yaml yarn.lock -S || trueRepository: TanStack/tanstack.com
Length of output: 4352
Preserve explicit invalid locale values.
locales ?? GENERATION_LOCALE treats null as omitted and formats it as en-US, while the native methods throw. Use locales === undefined ? GENERATION_LOCALE : locales in all three wrappers so only omitted locale arguments receive the generation default.
🤖 Prompt for AI AgentsVerify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/pin-catalog-locale.ts` around lines 25 - 29, Update all three locale wrapper methods in scripts/pin-catalog-locale.ts to default only when locales is undefined, preserving explicit null and other invalid values for native validation. Replace the nullish fallback in each originalToLocaleDateString call with an undefined-only conditional while keeping the existing GENERATION_LOCALE default and call flow unchanged.
Sorry, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Reviewers
At least 1 approving review is required to merge this pull request.
Assignees
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
pnpm test fails for anyone whose machine is not set to an English locale. The pretest hook runs charts:check-landing-catalog, which regenerates the catalog assets in memory and compares them against what is committed — and the regenerated SVGs differ, so the check reports the committed assets as stale.
The difference is the x-axis date labels. Three cases render Oct 6, Oct 8, Oct 10, Oct 12 on an English machine and 10월 6일, 10월 8일, … on a Korean one: 86-streaming-window-preservation, 91-timeline-playback-scrubber, 92-editable-event-range. That text is part of the SVG, so it changes the content hash, which changes chartsLandingCatalogAssetRevision, which fails the check.
Where it comes from
The formatting happens inside @tanstack/react-charts-catalog (0.7.2), not in this repo. Its catalog cases pin en-US in 96 places and leave 29 toLocaleDateString / toLocaleString calls without a locale, so those fall back to the machine's. The cases already fix their data and pass timeZone: 'UTC', so the missing locale reads as an oversight rather than a decision.
The real fix is TanStack/charts#68, which pins those 29 call sites. This PR is the guard until that lands, is published, and the version is bumped here — three separate events, and the check stays broken locally in the meantime. Once the bump happens, drop the two import './pin-catalog-locale' lines and re-run the check under a non-English locale; if it passes, scripts/pin-catalog-locale.ts can be deleted.
The change
scripts/pin-catalog-locale.ts defaults Date.prototype.toLocaleDateString, Date.prototype.toLocaleString, and Number.prototype.toLocaleString to en-US, and both generation scripts import it for side effects before the catalog components load. Calls that pass a locale explicitly keep it — the wrapper is locales ?? 'en-US', so the 32 already-pinned call sites are untouched.
Setting process.env.LANG inside the script does not work: Node resolves ICU's default locale at process start, before any module code runs. Pinning in only one script is also not enough, since the pre-commit chain is generate-charts-landing-catalog --check && generate-charts-landing-assets --check and both load the case components.
Verification
Under LANG=ko_KR.UTF-8, regenerating now produces zero changed files — previously three — and chartsLandingCatalogAssetRevision stays at 487bba6af954. The generated assets are byte-identical to what is committed, so nothing about the rendered charts changes.
Both --check scripts pass under ko_KR.UTF-8 and under en_US.UTF-8. This commit was made with the pre-commit hook enabled on a Korean-locale machine, which previously required --no-verify.
Explicit locales still work: with the patch loaded, toLocaleDateString('ko-KR', …) returns 10월 6일, ('ja-JP', …) returns 10月6日, and (1234.5).toLocaleString('de-DE') returns 1.234,5.
Scope is limited to asset generation. The scripts run as separate tsx processes and nothing in the app bundle imports this module, so runtime rendering — including charts.catalog_.embed.$caseId.tsx, which renders the same case components live — is unaffected and still follows the visitor's locale.
Summary by CodeRabbit