[smwbev]

Summary

Repurposed from canonicalizing the locale at read time to rejecting it at install, per review.

pluginLocaleIdSchema checks the shape of a locale id, not its grammar, and the shape check is looser than BCP 47 in one direction: en-US-US (duplicate region) and en-aaa (a subtag that is neither a region nor a script) both match /^[A-Za-z]{2,3}(?:-[A-Za-z0-9]{2,8})*$/.

A pack declaring one installs cleanly, loads, and translates every string. Then Intl.DateTimeFormat throws inside getIntlLocale(), the catch at src/renderer/src/i18n/i18n.ts:106-113 swallows it, and dates and relative times fall back to English with nothing logged. The pack author sees a working pack with English dates and no way to tell why.

The fix is one .refine on the schema delegating the grammar to Intl.getCanonicalLocales, so the tag is rejected at install where the author can still act on it. No new module, no cache, no renderer change. The bundled pt-BR pack is unaffected, as is every other well-formed tag.

Screenshots

No visual change.

Testing

• pnpm lint
• pnpm typecheck
• pnpm test — src/shared/plugins green at 147 tests; the 8 unrelated files that fail here fail identically on a clean upstream/main checkout (Node 26 against the required Node 24: node-pty ABI and timing-sensitive watchers)
• pnpm build
• Added tests: two rejection cases (en-US-US, en-aaa) alongside the existing en_US case, and a positive case pinning ru, pt-BR, zh-Hans-CN, en-US so the refine cannot start rejecting real packs

AI Review Report

Reviewed for scope, blast radius, and cross-platform behavior.

• Blast radius. pluginLocaleIdSchema has exactly one consumer, pluginLanguagePackContributionSchema; grep across src/ confirms no other import. Nothing outside manifest parsing changes.
• Regression risk on existing packs. Every locale tag shipped or plausibly shipped was run through the new schema: pt-BR (bundled), ru, en-US, zh-Hans-CN, ja, ko, es. All pass. The added positive test pins four of them so a future tightening cannot silently break them.
• Cross-platform. Intl.getCanonicalLocales is ECMA-402, present in every Node and Chromium build this project targets; no platform branch, no path handling, no shell, no shortcut or label. Verified the behaviour is identical under the repo's Node 24 target and the Node 26 used locally. Nothing Electron-specific is touched.
• Behaviour delta. Brute-forced the tags the regex accepts through old and new logic. The only difference is that tags Intl cannot parse are now rejected at parse time instead of loading and failing silently later. No previously-valid tag becomes invalid.

Security Audit

• Input handling. The change only narrows what the manifest accepts, so it cannot widen the attack surface. Locale ids become i18next bundle keys and filenames; the existing regex already excluded path syntax, and that regex is unchanged — .refine runs after it.
• Error handling. Intl.getCanonicalLocales throws RangeError on malformed input; the helper catches and returns false rather than letting it escape into manifest parsing. It catches everything deliberately — any throw from this call means the tag is unusable.
• No new surface. No command execution, no path resolution, no auth, no secrets, no IPC, no new dependency. Intl is a language built-in.
• Follow-up. None required for this change.

Notes

One measurement worth flagging. I bucketed every tag the current schema accepts by what Intl does with it — all two-letter primary subtags against typical second subtags (regions, scripts, and shape-legal fakes of both):

	count	share	example
Intl throws — caught by this PR	1352	22%	en-US-US, en-aaa
canonical, but no plural rules	3698	61%	xx, aa-US, zz-ZZ
fully working	1034	17%	af, pt-BR

const RE = /^[A-Za-z]{2,3}(?:-[A-Za-z0-9]{2,8})*$/
for (const tag of candidates) {
  if (!RE.test(tag)) continue
  try { Intl.getCanonicalLocales(tag) } catch { throws.push(tag); continue }
  ;(Intl.PluralRules.supportedLocalesOf(tag).length ? works : silent).push(tag)
}

This PR closes the 22%. The 61% is well-formed-but-unknown tags: they pass getCanonicalLocales, the pack loads, and Intl.PluralRules.supportedLocalesOf returns empty — same silent English fallback, different cause. .refine structurally cannot catch those, because the grammar is valid.

That is the half #12505 addresses, which is why I don't think it rests on the same premise. Its core claim — the synthetic plugin<hex> language never gets plural rules regardless of how valid the declared locale is — holds after this change. It currently imports canonicalizePackLocale, which this PR removes, so I'll rebase it off this branch once the direction here is settled.

Separately and out of scope: the regex is also narrower than BCP 47 in places. en-US-u-ca-buddhist and en-a-bbb are legal but rejected, because a single-letter extension singleton doesn't fit {2,8}. Happy to open that separately if it's worth having.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

• Last reviewed commit: 8993614
• Latest commit, not reviewed: 591731a

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

No actionable comments were generated in the recent review.

Recent review info

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Repository UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 3cc8ebf3-f606-40bd-b03f-9bc62da2b3a7

Commits

Reviewing files that changed from the base of the PR and between 0c23cac and 8993614.

Files selected for processing: 2

• src/shared/plugins/plugin-content-pack-contributions.test.ts
• src/shared/plugins/plugin-content-pack-contributions.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

Walkthrough

Plugin locale schema validation now calls Intl.getCanonicalLocales to reject malformed locale identifiers that match the existing pattern. Tests cover invalid duplicate-region and unknown-subtag values. Tests also cover valid locales with language, region, and script subtags.

Priority: Normal

Merge Risk: Minimal · previous review 89936

No actionable merge-blocking risk was established.

Pre-merge checks: 5

Check name	Status	Explanation
Title check		The title "fix(plugins): reject locale ids Intl cannot parse" matches the main change. The diff adds an Intl.getCanonicalLocales check to pluginLocaleIdSchema, so locale ids that Intl cannot parse are…
Description check		The description follows the template closely. It has a Summary that explains the before and after, the mechanism (a single .refine delegating to Intl.getCanonicalLocales), and the regression risk. It …
Docstring Coverage		Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Full details: Title check

Explanation

The title "fix(plugins): reject locale ids Intl cannot parse" matches the main change. The diff adds an Intl.getCanonicalLocales check to pluginLocaleIdSchema, so locale ids that Intl cannot parse are rejected at install. The title is short, specific, and uses the conventional prefix. It does not cover the added tests, but that is expected.

Full details: Description check

Explanation

The description follows the template closely. It has a Summary that explains the before and after, the mechanism (a single .refine delegating to Intl.getCanonicalLocales), and the regression risk. It includes a Testing section with the four checklist commands marked and the added test cases listed. It adds Security Audit and Notes sections beyond the template. The template's ELI5, Linked Issue, Visual Proof, and AI Disclosure sections are not clearly filled in. The Linked Issue section is a required item for outside contributors and has no 'Fixes #' issue number. The Visual Proof section says 'No visual change' under Screenshots, which does not follow the template's exact N/A wording. The ELI5 and Why headings are not labelled as such in the body, although the Summary and Notes cover the reasoning. The description is mostly complete, so the check passes, but the missing linked issue and ELI5 should be added before merge.

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[brennanb2025]

Thanks for this — the writeup, the benchmark table, and the security notes are unusually careful, and I dug in properly because of it. Heads up first: I have not pushed anything to your branch.

The short version is that the ru_RU case can't reach getIntlLocale(). pluginLocaleIdSchema in src/shared/plugins/plugin-content-pack-contributions.ts has always matched /^[A-Za-z]{2,3}(?:-[A-Za-z0-9]{2,8})*$/, which forbids underscores, so a manifest declaring ru_RU fails to parse and the plugin never loads at all. main already pins this — src/shared/plugins/plugin-content-pack-contributions.test.ts:122 asserts a en_US language pack is rejected. I brute-forced every string that schema accepts (~24k tags) through the before/after logic and found zero behaviour differences, apart from three cosmetic ones on deprecated Chinese variant tags (zh-latn2-guoyu and friends) that no real manifest ships. That's the same reason Intl.getCanonicalLocales in front of supportedLocalesOf is a no-op: supportedLocalesOf runs that canonicalization itself.

You were circling something real, though. That regex is looser than BCP-47, and that's the direction that bites: shape-legal typos like en-US-US (duplicate region) or en-aaa (a subtag that is neither a region nor a script) pass the manifest, the pack loads, every string translates — and then dates and relative times silently fall back to English with nothing logged. That is exactly the failure you described, just with a different trigger.

The fix for it is small and sits one layer up: a single .refine on pluginLocaleIdSchema delegating the grammar to Intl.getCanonicalLocales, so the tag is rejected at install where the pack author can see it and act on it. Around 18 lines with tests, no new module, no cache, no renderer change. The bundled pt-BR pack is unaffected.

Would you like to repurpose this PR to that, or would you rather we carry it? Genuinely your call — you found the gap.

One more thing worth a look: #12505 is stacked on this branch, so it may rest on the same premise.

[coderabbitai]

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

[smwbev]

Repurposed as you suggested — taking it, since you offered. The PR body has the full writeup; short version below.

.refine(isWellFormedLocaleId) on pluginLocaleIdSchema, delegating the grammar to Intl.getCanonicalLocales. 28 lines with tests, no new module, no cache, no renderer change. pt-BR unaffected, src/shared/plugins green at 147 tests. pack-locale.ts is gone — you were right that it was solving a case that can't occur.

One measurement I'd flag before you review. I bucketed every tag the current schema accepts by what Intl does with it, and the split is lopsided:

	count	share	example
Intl throws — caught here	1352	22%	en-US-US, en-aaa
canonical, but no plural rules	3698	61%	xx, aa-US, zz-ZZ
fully working	1034	17%	af, pt-BR

This PR closes the 22%. The 61% is well-formed-but-unknown tags — they pass getCanonicalLocales, the pack loads, PluralRules.supportedLocalesOf comes back empty, and dates fall back to English just as silently. .refine structurally cannot catch those: the grammar is valid.

That's the half #12505 covers, so I don't think it rests on the same premise — its claim is that the synthetic plugin<hex> language never gets plural rules no matter how valid the declared locale is, and that survives this change. It currently imports canonicalizePackLocale, so I'll rebase it off this branch once you're happy with the direction here.

Also out of scope but noted in the body: the regex is narrower than BCP 47 in places too — en-US-u-ca-buddhist is legal but rejected, because a single-letter extension singleton doesn't fit {2,8}. Separate PR if you want it.

[smwbev]

Gentle nudge — no rush intended, I know the queue is deep.

This has been sitting green since 8 Aug: repurposed to the .refine you outlined, CodeRabbit closed its review with no comments, mergeable, and the four checklist commands are run and recorded in the description. Nothing is waiting on me here.

Two things downstream of your call on it, whenever you get to it:

#12505 needs a rebase either way, since it imports canonicalizePackLocale and this PR removes it. I've held off because rebasing before the direction is settled risks doing it twice. Its core claim is unchanged — the synthetic plugin<hex> language gets no plural rules regardless of how valid the declared locale is — and the measurement in this PR's Notes is why I think it addresses a different half of the problem rather than the same premise.

A second narrow widening of the plugin-chrome exemption is written and tested locally — 23 paths, the install form and the log controls, following the same rule as #12455. I'm holding it rather than opening a third PR into the same area while these two are open. Happy to keep holding it, or to open it, whichever is less noise for you.

And if the direction here has changed or this dropped down the list, just say so — I'd rather know than keep it warm.