Skip to content

fix(i18n): drop orphan TerminalPane.minimumContrast entries that broke main static analysis - #19566

Merged
nwparker merged 1 commit into
mainfrom
nwparker/fix-orphan-minimum-contrast-catalog
Sep 8, 2026
Merged

nwparker merged 1 commit into
mainfrom
nwparker/fix-orphan-minimum-contrast-catalog

Conversation

@nwparker

@nwparker nwparker commented Sep 8, 2026 •

Copy link
Copy Markdown
Contributor

ELI5

main has failed the static analysis check for every PR since 07:39Z today. #18126 added seven English strings to en.json that no code reads, and a CI check that decides which English strings must ship in the app's boot bundle treats "no code reads it" as "might be built dynamically, so it must ship." It then noticed those seven were not in the shipped catalog and failed. This PR deletes the seven unused strings — the actual problem — rather than the catalog that correctly flagged them.

Relationship to #19544 / #19337 (read this first)

Two open PRs currently carry a regenerated en-runtime-required.json that adds these 7 orphans to the shipped catalog. If either merges first, main goes green — and this PR becomes the follow-up cleanup, not the fix. Stated plainly and without blame (those PRs were fixing their own red, not making a policy choice): the regenerate variant makes the check pass by adding the dead keys to the list of things that must ship. That is the tool built to prevent boot-bundle dead weight being used to authorise it.

This PR is safe to land in either order:

  • Before theirs: the failing step exits 0 (proven below); their later regeneration then re-adds nothing, because the keys are gone from en.json.
  • After theirs: the two changes touch different files and do not conflict. Deleting the keys from en.json leaves their 7 shipped entries classified by the generator as superfluous — explicitly "harmless; sync to drop them" — not missing or contradicting, so the check stays green. No second break.

Root cause

#18126 (53852c9, "make the contrast floor user-configurable") added two minimumContrast blocks to en.json:

  • auto.components.settings.terminal.search.minimumContrast.* — referenced by terminal-typography-search.ts. Kept.
  • auto.components.settings.TerminalPane.minimumContrast.* (7 entries) — referenced by nothing. The shipped TerminalContrastSetting.tsx reads auto.components.settings.contrast.* instead. These are the orphans this PR removes.

generate-runtime-required-english-catalog.mjs keeps an entry as runtime-required when no call site with a literal default references it (its own doc comment: a dynamic key could still resolve it). So an unreferenced entry is by construction runtime-required — literalFallbacksByKey.get(key) is undefined and it lands in required. The seven orphans therefore became required, en-runtime-required.json was never regenerated in #18126, and verify:localization-runtime-catalog has failed on every merge-ref since.

This failure mode recurs on any rename that leaves leftovers behind — every unreferenced key is silently promoted to runtime-required. Whether that deserves a lint is a separate call; it is worth naming.

Why delete rather than regenerate

Regenerating would also turn the check green — by shipping seven strings nothing will ever read, which is precisely the "dead weight in the boot bundle" the generator's comment says it exists to prevent. Deleting is the root cause.

Before deleting, I established there is no dynamic path that can produce auto.components.settings.TerminalPane.minimumContrast.* (the exact hazard the check guards against):

  • Literal references: zero across src/, mobile/, tests/ (.ts/.tsx/.json/.snap), excluding en.json itself.
  • Template-literal key builders under auto.components.settings.: exactly one in the whole tree — plugin-chrome-allowlist.test.ts:43, a consumer (path.startsWith(...) over paths read from the catalog), not a producer.
  • String-concat key builders under that namespace: zero.
  • minimumContrast token near any translate/t(/key usage: only the kept terminal.search.minimumContrast.* literals and the unrelated terminalMinimumContrastRatio setting-value reads.
  • Other locales (es/fr/ja/ko/zh) and mobile catalogs: none carry the block; no translation is stranded.
  • The check's own scanner (ts.isStringLiteralLike in verify-localization-catalog.mjs) classifies keys the same way, so what it can't see, neither can i18next via a literal.

git blame attributes the block solely to 53852c9; the sibling TerminalPane.scrollSpeed.* follows the normal pattern (literal refs in TerminalInteractionSection.tsx), which is what the orphans were evidently meant to be before the component was pointed at settings.contrast.*.

The "10 insertions vs 7 orphans" question, closed

A regenerate dry-run reports 10 insertions / 1 deletion. Key-level diff over the flattened JSON: ADDED 7 (exactly the orphans), REMOVED 0, CHANGED 0. The line count is 7 leaves + 2 brace lines + the predecessor line rewritten to add a trailing comma (−1/+1). There are no "other 3." Decisive control: on main + this delete, --fix regenerates a byte-identical en-runtime-required.json (cmp exit 0). Nothing remains to add.

Evidence this is a main break, not a PR-side one

Systemic: three gaps that compound

#18126's own static analysis job was cancelled at 05:48Z and it was merged at 07:39Z anyway. On its head SHA: cancelled:17, failure:1, skipped:4 — zero required checks reached success, and no later run was created for that head. Had the job run, this step would have failed and the break would never have reached main. Twenty-four minutes later a second, independent break landed (#19542 + #19551, 16 seconds apart; fixed in the companion orchestration PR). The full cause:

  1. cancelled ≠ failed — a cancelled required check reads as "not failing."
  2. No up-to-date-base requirement — "green alone" treated as "green together."
  3. No post-merge signal on main — pr.yml is pull_request-only; main gets no CI on push, so neither break was ever red on main itself.

The first two are fixed by branch protection with strict status checks, which main cannot have because it has no branch protection and no rulesets at all. The third needs a push trigger on main.

Verification

On the fixed tree (origin/main d346d6f + this commit), locally and confirmed identically in this PR's own CI (static analysis → success):

Check Result
verify:localization-runtime-catalog (the failing step) exit 0 — "covers en.json: 1680 of 14189"
verify:localization-catalog exit 0 — 14383 references verified
verify:localization-extraction exit 0 — orphan count 1557 → 1550 (exactly the 7)
verify:localization-coverage exit 0
all 9 i18n unit tests that load en.json 123 passed
oxfmt --check en.json clean

Diff is exactly the 9 lines of the orphan block and the trailing comma on its predecessor; en-runtime-required.json is untouched; the referenced terminal.search.minimumContrast block is intact.

This PR's verify is red only because of the second, unrelated main break (shard 3/8), fixed in the companion PR.

…e main

#18126 added a `auto.components.settings.TerminalPane.minimumContrast` block
to en.json alongside the `settings.contrast.*` keys the shipped
TerminalContrastSetting component actually reads. Nothing references the
TerminalPane block: zero call sites, zero tests, zero other locales, and no
dynamic key builder under `auto.components.settings.*` can produce it (the
only template-literal builder in that namespace is a test consumer that reads
paths from the catalog, not one that constructs them).

An unreferenced entry is treated as runtime-required by
generate-runtime-required-english-catalog (possible dynamic key), so the
seven orphans made `verify:localization-runtime-catalog` demand they ship in
en-runtime-required.json. #18126 never regenerated it, and its own
static-analysis gate was cancelled rather than run, so main has failed that
step for every PR since 53852c9 merged.

Delete the orphans rather than regenerate the catalog around them:
regenerating would enshrine seven keys nothing reads.
@coderabbitai

coderabbitai Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 2ef8a9bc-e608-479c-996b-18430eb60de1

📥 Commits

Reviewing files that changed from the base of the PR and between d346d6f and 2292c50.

📒 Files selected for processing (1)
  • src/renderer/src/i18n/locales/en.json
💤 Files with no reviewable changes (1)
  • src/renderer/src/i18n/locales/en.json

Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review.


📝 Walkthrough

Walkthrough

The English locale removes all translation entries for the minimumContrast setting. Removed entries include the title, description, automatic, disabled, pinned, placeholder, and suffix strings. No public declarations change.

Merge Risk: ⚪ Minimal · up to 2292c

This change removes unused English localization entries and has no identified merge-readiness risk.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title is concise, specific, and accurately identifies the removal of orphaned i18n entries that caused static analysis failures.
Description check ✅ Passed The description clearly explains the scope, root cause, rationale, affected keys, evidence, and verification results. It omits some template sections, including an explicit Fixes #... line, visual-p…
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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

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

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

  • Delete orphan TerminalPane.minimumContrast block — removes the 7 unused entries (plus their enclosing braces and the predecessor's trailing comma) from en.json, leaving the file valid JSON.

Confirmed the root-cause claim: the shipped TerminalContrastSetting.tsx reads auto.components.settings.contrast.*, not TerminalPane.minimumContrast.*, so the deleted block had no reader. The sibling terminal.search.minimumContrast.* block (used by terminal-typography-search.ts) and TerminalPane.scrollSpeed.* (used by TerminalInteractionSection.tsx) are untouched, and a repo-wide search turns up no literal or dynamic reference to the deleted keys. Deleting rather than regenerating the runtime catalog is the right call — regenerating would have shipped dead data.

Pullfrog  | View workflow run | Using DeepSeek Pro (free via Pullfrog for OSS) | 𝕏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant