Repository navigation
[Maintainer] i18n: a new locale no longer breaks release-check - #1235
Merged
Merged
Conversation
releaseBase only accepted a base whose tree had a catalog for every TARGET_LOCALES entry. A PR that bundles a new locale (tr in #1184, it in #1200) passes PR CI, but after it merges no release tag, origin/main fallback or first-catalog commit has that locale's catalog, so the next release fails with "no base with bundled PO catalogs", and `--base <tag>` is rejected too. A base now qualifies when it has any target catalog, which still rejects a base from before the catalogs. A locale without a catalog at the base has nothing reviewed there to keep; release-check names it, and its compiled pack is still scanned for drafts. Tests cover a locale bundled after the latest release tag: the release passes, a reviewed-to-draft regression in an existing locale still fails against that tag, a draft compiled into the new locale's pack still fails, and an explicit --base without catalogs is still rejected.
The fixture repositories behind the release-check tests committed and tagged with the developer's global git config. With commit signing on, every fixture commit went through the signer; a throttled signer took up to 15 s per commit, so these tests hit bun's 5 s timeout. With tag.gpgSign on, `git tag <name>` asks for an annotated tag message and fails. The fixtures now turn off commit and tag signing.
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
The default lookup and --base never compare with a base that has no catalogs, but a --base-dir with none (a typo, or the wrong level) read as an empty base: release-check passed, and now also listed every locale as new. It fails like --base instead, so the README's "never compares with a base that has no catalogs" holds for every base. The README now says it is the release job that lists a new locale; i18n:check in CI does not.
Every catalog fixture and release repository stayed in the temp directory, a few megabytes each, and the new-locale tests added three more per run. They are removed after each test, as the other script tests already do.
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
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.
What does this PR do?
The release workflow's translation gate,
bun scripts/i18n.ts release-check, could not pick a base once a new locale was bundled.hasCatalogsrequired a catalog for everyTARGET_LOCALESentry, and no earlier commit has the new locale's catalog. So after #1184 (Turkish) or #1200 (Italian) merges, the latest release tag, theorigin/mainfallback and the catalogs' first commit are all rejected, and the next release fails:--base @pascal-app/editor@1.0.7is rejected the same way. PR CI doesn't see this:i18n:checkcompares with the PR's base commit, where a missing locale is already allowed.every→someinhasCatalogs). A base from before the catalogs is still rejected, by the default lookup and by--base.New locales since @pascal-app/editor@1.0.7, with no reviewed entries to keep: tr), and its compiled pack is still scanned for drafts.--base-dir <dir>with no target catalog now fails like--base(--base-dir <dir> has no bundled PO catalogs to compare with). Before, it compared with an empty base and passed.i18n/README.md: one sentence on new locales in the release-check paragraph.tag.gpgSign=true,git tag <name>fails for want of a message. Fixture directories are now removed after each test, as in the other script tests.No catalog, translation or review marker changes here, and nothing in this PR reviews or approves a
troritentry.How to test
bun test ./scripts/i18n-literals.test.ts: 51 pass. The new tests cover:--basepredating a locale but not the catalogs;--base-dirwithout catalogs failing.The first three fail on
mainwith the error above; the--base-dirone fails without its guard (exit 0 instead of 1).In a throwaway clone, merge a locale PR and point
origin/mainat the merge, as the release run after that merge sees it:git fetch origin pull/1184/head:pr-1184 git switch --detach origin/main && git merge --no-edit pr-1184 git update-ref refs/remotes/origin/main HEAD bun scripts/i18n.ts release-checkOn
mainthis fails with the error above. Aftergit merge --no-edit origin/cursor/i18n-release-check-new-locale-b052, it printsComparing with @pascal-app/editor@1.0.7andNew locales since @pascal-app/editor@1.0.7, with no reviewed entries to keep: tr, then passes. Same with Add Italian (it) as a bundled locale (unreviewed AI drafts) #1200 (it), and with both merged (tr, it).In that tree, mark fr's reviewed
editor.buildPanel.advancedas#, fuzzywithReview: required: release-check fails withfr: "editor.buildPanel.advanced" is reviewed on the base branch with the same English and must stay reviewed. Compiling a draft into thefr,troritpack fails with<locale>: compiled pack ships unreviewed "…".bun run cipassed locally onbe3f503: biome,i18n:check, skills validation, typecheck, package and root tests, build, and the production smoke test. On the final head, the root i18n suite (80 pass),i18n:checkandbun run checkpass locally, and CI passes.Screenshots / screen recording
N/A — non-visual change.
Checklist
bun devbun checkto verify)mainbranchLocale PRs
This unblocks the release after #1184 (tr) or #1200 (it) merges. It doesn't make either one ready:
main,i18n:checkfails for both until their catalogs are refreshed. tr is missing 1,192 messages and has 37 obsolete; it is missing 129 and has 24 obsolete.TARGET_LOCALESline and the same locale lists inscripts/i18n-literals.test.ts, so the second to land has to merge those lines.Review
be3f503with no findings. It hasn't re-run on the two follow-up commits.be3f503. Its P2 (a--base-dirwithout catalogs passed silently) and two nits are addressed in1e2ba69and8f97c54, which it then approved with no P0–P2 findings.8f97c54with no comments.Next owner: Maintainer
Note
Medium Risk
Changes release workflow gating and base-selection logic for translation regressions; mistakes could let bad packs ship or block valid releases.
Reviewed by Cursor Bugbot for commit 8f97c54. Bugbot is set up for automated code reviews on this repo. Configure here.