Fall back to English when ProjectSelector's localized strings are unresolved

[jolierabideau]

What was broken

ProjectSelector rendered raw %projectSelector_*% localization keys at the user instead of
localized text or its English fallback — live on main, introduced by #2673.

Two compounding causes:

1. An unresolved value is a defined string. useLocalizedStrings seeds its state with
   defaultState[key] = key and returns that same state on a platform error, so a lookup hands back
   the literal '%projectSelector_clearAll%'. A typeof value === 'string' test accepts it, and
   ?? 'Default' never fires on it.
2. An explicit undefined beat the default. buildProjectSelectorLocalizedStrings emits a
   property for every field, and a plain { ...DEFAULT_STRINGS, ...partial } treats a
   present-but-undefined property as a real write. Fixing only (1) would have turned "renders a raw
   key" into "renders a blank string".

The fallback has to hold across three windows: before strings resolve, after a platform error, and
after a successful resolve.

The fix

One shared reader, rather than a new copy of the rule. lib/platform-bible-react/src/utils/localization.util.ts
already owned this problem (resolveLocalizedString, used by BookChapterControl and
RecentSearches); it now also treats whitespace-only values as unresolved and exposes two more
shapes, all three exported from platform-bible-react/experimental:

• isResolvedLocalizedValue(value) — the predicate: rejects undefined, a %…% key, and blank text.
• resolveLocalizedString(value, fallback) — one value.
• firstResolvedLocalizedString(...candidates) — an ordered chain, for call sites with more than one
  source to try before reaching a literal they own.

resolveStrings merges field by field against DEFAULT_STRINGS, so the Required<…> return type
catches a missing or misspelled field, and it is memoized rather than re-running per render.

ariaLabel: '' remains a deliberate "no accessible name" opt-out, matching RecentSearches.
Whitespace-only is not that opt-out.

Consumers

Six consumers wire these keys. This PR covers three of them (four pickers):

Consumer	State
checklist (2 pickers)	Fixed — own wording for both text and accessible name
find	Fixed — keeps "No open projects or resources"
checks-side-panel	Fixed — keeps "No project"
manage-books	Not fixed — see below
manage-books-dialog (3 pickers)	Not fixed — see below
settings-tab / SettingsSidebar	Not fixed — overlaps #2796

Deliberately out of scope, and tracked in PT-4673: manage-books-dialog.component.tsx:619
defines t = (key, fallback) => localizedStrings[key] ?? fallback, whose ?? never fires, feeding
the Copy "From", Create "Based on" and sidebar pickers; and settings-tab.component.tsx:305 passes a
bare lookup. Repo-wide there are ~83 localizedStrings[…] ?? fallback sites with the same dead
?? — 35 in platform-bible-react, 22 in platform-scripture-editor, 11 in
src/renderer/components, 7 in platform-scripture, the rest scattered — though not all are live
defects, since not every bag comes from useLocalizedStrings. And
.context/standards/Localization-Guide.md:181-186 still teaches the idiom. That pair is
PT-4673, not this PR.

On #2796

Earlier revisions of this description called #2796 a major conflict. That was wrong: #2796 touches
settings-sidebar*, settings-sidebar-content-search, settings-tab and dist — not
project-selector*. The real overlap is small. #2796 adds its own
Object.entries(...).filter(value !== undefined) at settings-sidebar.component.tsx:117-119, with a
comment at :112-114 saying ProjectSelector "layers this bag … with a plain spread". Once this PR
merges that comment is wrong and the filter is redundant — whichever merges second should remove
both.

Scope note on locales

An earlier revision claimed fr/km/zh-hans/zh-hant were showing raw keys. That is also wrong:
localization.service-host.ts:339 always appends BACKUP_LANGUAGE = 'en' to the fallback chain and
en.json carries all 21 keys, so those locales already got English. The real exposure is the window
before strings load and the platform-error path.

Verification

• npm run typecheck, npm run lint (0 errors), npm test — all green, 11 projects.
• The four platform-scripture suites run separately (80 passing): the root vitest config's
  include excludes extensions/src/**, so npm test from the root never runs them. Worth
  confirming CI covers them somewhere.
• Every new guard confirmed falsifiable by reverting it and watching the intended test fail.
• lib/platform-bible-react/dist/ rebuilt and committed — the extensions/ suites resolve the
  library through its committed bundle, so this is load-bearing, not precautionary.

Note for @Sebastian-ubs

This is in the %projectSelector_*% localization work from #2673 — flagging it rather than leaving
it to be found in review.

🤖 Generated with Claude Code

---

This change is 

[katherinejensen00]

PR #2829 review — Fall back to English when ProjectSelector's localized strings are unresolved

PR: #2829
Author: @jolierabideau · Branch: fix/project-selector-unresolved-localization-keys
Reviewed at: 7b60770eace against merge-base e7d19304ad7 (lib/platform-bible-react/dist/ skipped; I checked that it matches src)
CI at review time: CodeQL and Analyze passed; the three Build jobs were still pending.

How this was reviewed: separate passes for correctness and tests, architecture, and comments and docs. Then an adversarial pass tried to knock those findings down, and a /code-review max pass (10 finder angles) ran on top. Every finding below was checked against the PR's version of the file. The adversarial pass dropped findings that were wrong, a matter of taste, or repeats. Line numbers are in the PR's version of each file, and every comment is anchored to a line the PR changes.

---

Summary table

#	Severity	File:line to attach	Finding
1	Important	lib/platform-bible-react/src/components/advanced/project-selector/project-selector.component.tsx:214	Reuse the existing resolveLocalizedString instead of a new copy of the rule
2	Important	lib/platform-bible-react/src/components/advanced/project-selector/project-selector.component.tsx:216	The blank check removes the ariaLabel: '' opt-out and leaves || undefined at :1062 as dead code
3	Important	extensions/src/platform-scripture/src/find/find.component.test.tsx:66	Consumer wording is replaced with generic English that can be wrong ("Select a project" when none are open)
4	Minor	extensions/src/platform-scripture/src/checklist.web-view.tsx:831	ariaLabel skips firstUsableLabel, so both checklist pickers get the same accessible name
5	Minor	lib/platform-bible-react/src/components/advanced/project-selector/project-selector.component.tsx:216	resolveStrings now splits every field into graphemes on every render, and it isn't memoized
6	Minor	lib/platform-bible-react/src/components/advanced/project-selector/project-selector.groupings.ts:87	Two different definitions of "unresolved", and grouping labels only go through the weaker one
7	Minor	extensions/src/platform-scripture/src/checklist.web-view.tsx:964	New primaryProjectLabel fallback can show a GUID, and the label changes once strings load
8	Minor	lib/platform-bible-react/src/components/advanced/project-selector/project-selector.component.test.tsx:587	New tests: one assertion can never fail, one test doesn't reach the branch it names, one key doesn't exist
9	Minor	lib/platform-bible-react/src/components/advanced/project-selector/project-selector.component.tsx:196	The same explanation appears at 6 new places; keep one, point to it from the rest
10	Minor	PR description	The #2796 warning is out of date, the fr/km/zh claim is wrong, and a pasted review summary buries the important notes
11	Nit	extensions/src/platform-scripture/src/checklist.web-view.tsx:965	New hard-coded English label duplicates localizedStrings.json
12	Nit	lib/platform-bible-react/src/components/advanced/project-selector/project-selector.component.tsx:214	Object.fromEntries round-trip loses field types (no risk today)
13	Nit	extensions/src/platform-scripture/src/checklist.web-view.test.tsx:109	Hoisted vi.mock factory uses a top-level import, against the rule stated at :124
14	Nit	lib/platform-bible-react/src/components/advanced/project-selector/project-selector.groupings.ts:62	"So they cannot drift" is untrue; firstUsableLabel does its own trim() check
15	Nit	lib/platform-bible-react/src/components/advanced/project-selector/project-selector.groupings.test.ts:76	Comment names searchPlaceholder but the test uses clearAll; a checklist test comment sits above the wrong constant
16	Follow-up (not blocking)	extensions/src/platform-scripture/src/checklist.web-view.tsx:128	7 more localizedStrings[…] ?? fallback sites have the same bug, and Localization-Guide.md still teaches that pattern

---

Comments for the PR owner

Each comment below is written so it can be pasted into the PR as is.

1. Important — reuse resolveLocalizedString

Attach to: lib/platform-bible-react/src/components/advanced/project-selector/project-selector.component.tsx:214

This package already has a tested helper for this exact problem, resolveLocalizedString in lib/platform-bible-react/src/utils/localization.util.ts:20, so this PR adds another copy of the rule instead of using it.

Details:

• resolveLocalizedString(value, fallback) exists for this bug. Its doc says useLocalizedStrings seeds { [key]: key }, so ?? 'Default' never falls back. It has its own test file, and BookChapterControl and RecentSearches already use it (15 call sites).
• After this PR, the "is this value usable?" rule exists in several places that don't quite agree: resolveLocalizedString, readProjectSelectorString (project-selector.groupings.ts:87), resolveStrings (here), firstUsableLabel (checklist.web-view.tsx:128), and older ones in the renderer (localizedOrEnglish, createCrashedViewLocalizer).
• It isn't a drop-in swap, and the differences matter:
  • resolveLocalizedString checks !value, so it catches '' but not whitespace-only strings. This PR treats whitespace-only as unresolved on purpose.
  • Its key test is /^%[^%]*%$/. isLocalizeKey is looser (startsWith('%') && endsWith('%')): a bare '%' or '%d%' counts as a key. No shipped translation looks like that today. But firstUsableLabel also runs this check on primaryProjectName, which a user chooses, so a project named like %ABC% would be skipped.
• Suggestion: extend resolveLocalizedString to cover whitespace-only values, then write resolveStrings as a typed loop over Object.keys(DEFAULT_STRINGS) that calls it for each field. That removes the third copy of the rule and also fixes #5 and #12. The firstUsableLabel candidate chain can then sit on the same helper.

2. Important — ariaLabel: '' can no longer turn the label off

Attach to: lib/platform-bible-react/src/components/advanced/project-selector/project-selector.component.tsx:216

Passing ariaLabel: '' used to leave the trigger with no aria-label. The new blank check swaps '' for "Projects & resources", which quietly removes that option and leaves the code built to support it unreachable.

Details:

• The trigger renders aria-label={strings.ariaLabel || undefined} at :1062. || undefined exists so that '' means "no aria-label here; the visible text or a parent element names the control."
• Now isBlankLocalizedValue drops '' before it reaches :1062, so strings.ariaLabel is never empty and || undefined never runs.
• The sibling component in this library handles the same case the other way, on purpose: recent-searches.component.tsx:95-97 says "An empty string is a deliberate 'no label'…, so only undefined and a raw localization key fall back", and only sends non-empty values through resolveLocalizedString.
• The new test at project-selector.component.test.tsx:593 (falls back to English for a blank localized value, with ariaLabel: '') makes this change official. Anyone who later tries to restore the opt-out will see that test fail.
• Suggestion: either keep the opt-out the way recent-searches does (ariaLabel === '' ? '' : …), or, if dropping it is intended, remove || undefined and say in the prop's TSDoc that '' falls back.

3. Important — generic English replaces the consumer's own wording, and can say the wrong thing

Attach to: extensions/src/platform-scripture/src/find/find.component.test.tsx:66

When a consumer's own override is unresolved, the new check drops it and the picker shows the library's generic English. For Find, that turns "No open projects or resources" into "Select a project", which asks the user to pick a project at the very moment there are none open.

Details:

• find.component.tsx:975-977 sets buttonPlaceholder to %webView_find_projectFilter_noOpenProjectsOrResources% ("No open projects or resources"). checks-side-panel.component.tsx:226-230 does the same with its "No project" key. Before strings resolve, or after a platform error, both now show DEFAULT_STRINGS.buttonPlaceholder = "Select a project".
• The PR fixed this for the checklist with firstUsableLabel, but no other consumer got the same fix. The same thing happens in:
  • Manage Books dialog: manage-books-dialog.component.tsx:619 defines t = (key, fallback) => localizedStrings[key] ?? fallback, and that ?? never runs. So the Copy "From" picker (:2351), the Create "Based on" picker (:2647), and the sidebar picker (manage-books-sidebar.component.tsx:310) lose "Select project" / "Select reference project" / "Project". All three end up announcing "Projects & resources".
  • Settings sidebar: settings-tab.component.tsx:305 passes a raw lookup for projectsSidebarGroupLabel. Unresolved, the heading shows the raw %settings_sidebar_projectSettingsLabel% key while the picker right under it says "Projects & resources".
• The tests hide this. PICKER_BOUND_FIND_KEYS (here) and PICKER_BOUND_CHECKS_SIDE_PANEL_KEYS (checks-side-panel.component.test.tsx:62) now make those keys resolve in the stub, so no consumer test covers the unresolved state anymore.
• Suggestion: at minimum, give Find and Checks the same consumer-side fallback the checklist got, and add one test per consumer for the unresolved state. If the Manage Books and Settings pickers are out of scope, list them in the PR body so they aren't lost.

4. Minor — the checklist's ariaLabel skips firstUsableLabel

Attach to: extensions/src/platform-scripture/src/checklist.web-view.tsx:831

In the same object literal, buttonPlaceholder goes through firstUsableLabel but ariaLabel doesn't. While strings are unresolved, both checklist pickers get the same accessible name, "Projects & resources".

Details:

• :827-831 and :962-967: the visible text becomes "Select comparative projects" / "Select primary Scripture text", but the raw-key ariaLabel is dropped and replaced with DEFAULT_STRINGS.ariaLabel. Before this PR the two pickers had different (though ugly) raw-key names. Now a screen reader user hears two identical toolbar comboboxes.
• .eslintrc.ai.js enforces paranext/require-localized-aria as an error for *.web-view.tsx, so this is a pattern the repo cares about.
• The new checklist tests only check toHaveTextContent, which is why nothing caught this.
• Suggestion: one firstUsableLabel(...) call at each of the two sites, plus a getByRole('combobox', { name: … }) assertion in the new tests.

5. Minor — resolveStrings is slower on every render and isn't memoized

Attach to: lib/platform-bible-react/src/components/advanced/project-selector/project-selector.component.tsx:216

resolveStrings runs on every render (:728). Each field now goes through isLocalizeKey, which creates a GraphemeString, and that constructor splits the whole string into graphemes.

Details:

• grapheme-string.ts:175: this.graphemes = string === '' ? [] : Array.from(splitGraphemes(string)) runs in the constructor. The early exit on startsWith('%') only skips building the offsets array, not the grapheme split.
• The picker re-renders on every keystroke in its search box (setQuery), and the checklist shows two pickers. The /code-review pass measured roughly 78× the old spread's cost, and more in the all-raw-keys error state this PR targets. I didn't re-measure, but the constructor code confirms the extra work happens.
• Suggestion: useMemo(() => resolveStrings(props.localizedStrings), [props.localizedStrings]). Using the regex-based resolveLocalizedString from #1 would also avoid the grapheme split entirely.

6. Minor — two definitions of "unresolved", and grouping labels only get the weaker one

Attach to: lib/platform-bible-react/src/components/advanced/project-selector/project-selector.groupings.ts:87

readProjectSelectorString only rejects a value that equals its own key, while resolveStrings rejects anything shaped like a key. Grouping labels and section headings never go through resolveStrings, so they only get the weaker check.

Details:

• {grouping.label} (project-selector.component.tsx:681) and grouping section headings (:1194) read the availableGroupings objects directly.
• groupings.test.ts:120 (does not treat a DIFFERENT key as unresolved) locks in the weaker behavior. For chrome fields that result is never visible, because resolveStrings drops it afterwards. So that test describes a contract the rest of the code doesn't follow.
• It isn't a live bug for the shared keys, since the real failure modes return value === key. But it is live for consumer-built groupings: manage-books.web-view.tsx:919 does if (localizedName) names.set(...), and a raw key is truthy, so it ends up as a Type section heading. manage-books-dialog.component.tsx:855/903 does the same for versification headings.
• Suggestion: use one predicate (the shared helper from #1) in both places, and update the "DIFFERENT key" test to match.

7. Minor — new primaryProjectLabel fallback can show a GUID, and the label changes once strings load

Attach to: extensions/src/platform-scripture/src/checklist.web-view.tsx:964

Before this PR, the primaryProjectLabel candidate was never used. Now it is, and it can hold a raw project ID.

Details:

• primaryProjectName is (await pdp.getSetting('platform.name')) ?? projectId, and the error path calls setPrimaryProjectName(projectId). manage-books-sidebar.component.tsx:311 explicitly avoids this: "projectIds are GUIDs and would render as a 32-char hex string in the trigger".
• Ordering: while unresolved, the trigger shows the project name ("ESVUS16"). Once strings resolve, candidate 1 wins and it changes to the generic "Select primary Scripture text". In mode="project" the placeholder slot means "nothing selected", so the two states disagree about whether something is selected. The new test at checklist.web-view.test.tsx:271 treats the mid-load state as correct.
• Suggestion: drop primaryProjectLabel from the candidate chain, or skip it when it equals projectId.

8. Minor — some of the new tests can't catch the regressions they describe

Attach to: lib/platform-bible-react/src/components/advanced/project-selector/project-selector.component.test.tsx:587

A few of the new assertions can't fail, or pass without reaching the code they are named for.

Details:

• :587 — expect(trigger).not.toHaveAccessibleName(expect.stringContaining('%')) can never fail. trigger came from getByRole('combobox', { name: 'Projects & resources' }) a few lines earlier, which is an exact match, so its name is already known.
• :531-553 — the headline test never reaches the isLocalizeKey branch. buildProjectSelectorLocalizedStrings(UNRESOLVED_STRINGS) returns all undefined, because readProjectSelectorString already rejects value === key. Only the !== undefined branch runs. (The second test does cover isLocalizeKey, so that branch is tested; the first test's name and comment just claim more than it checks.)
• :569 — %webView_find_projectFilter_selectProject% doesn't exist anywhere in the repo. Find's real key is %webView_find_projectFilter_noOpenProjectsOrResources%. The behavior is still right, but the test presents a made-up key as a real consumer override.
• :586 — /%\w+_/ can't match a key without an underscore (for example %clearAll%), and queryByText throws "Found multiple elements" instead of failing clearly if more than one key leaks. queryAllByText(/%[^%\s]+%/) with .toHaveLength(0) is broader.
• Both tests render with openTabs={[]} and no availableGroupings, so the group-by menu, section headings, and bound-but-closed row never appear. The "nothing may render a raw key" checks therefore can't see most of the 21 strings.

9. Minor — the same explanation is written out at 6 new places

Attach to: lib/platform-bible-react/src/components/advanced/project-selector/project-selector.component.tsx:196

This PR adds six near-copies of the paragraph "useLocalizedStrings seeds its state with defaultState[key] = key and returns that same state on a platform error…". At that volume, readers stop reading them.

Details:

• The copies added here are at project-selector.component.tsx:203, project-selector.groupings.ts:75, project-selector.groupings.test.ts:74, project-selector.component.test.tsx:523, checklist.web-view.tsx:121, and project-selector.test-utils.ts:7. Several share the exact same clause.
• The comment in resolveStrings (:196-213) is 18 lines, split into three labeled sections, inside a function with 5 lines of code.
• project-selector.test-utils.ts: 19 of its 29 lines are comment, for a one-line helper and a one-line predicate.
• The comments are forward-facing, which is good; there are no ticket references or change history. The only problem is volume.
• Suggestion: keep the full explanation once, on readProjectSelectorString (the exported function that owns the idea), and make the other copies one-line pointers. For resolveStrings, something like:

  // Filter rather than spread: the builder emits `undefined` for unresolved fields, which a spread
  // would write over the English default. Localize-key and blank values are dropped for the same
  // reason — see `readProjectSelectorString`.

10. Minor — the PR description has errors and hides its key notes

Attach to: PR description (a general PR comment)

Parts of the description are out of date or wrong, and the notes a reviewer needs sit under about 200 lines of pasted review output.

Details:

• The top "Suggested Review Focus" item calls the #2796 conflict "the single most important thing to settle" and says #2796 "reworks this same file". That's no longer true: gh pr view 2796 --json files shows it only touches settings-sidebar*, settings-sidebar-content-search, settings-tab, and dist, not project-selector*. The real overlap is small. #2796 adds its own Object.entries(...).filter(value !== undefined) at settings-sidebar.component.tsx:117-119, with a comment at :112-114 saying ProjectSelector "layers this bag … with a plain spread". Once this PR merges, that comment is wrong and the filter is redundant. Whichever PR merges second should remove them.
• "fr, km, zh-hans and zh-hant were showing raw keys throughout the picker and now show English" is wrong. localization.service-host.ts:339 always adds BACKUP_LANGUAGE = 'en' as a fallback, and en.json has all 21 keys, so those locales already got English. The real exposure is the window before strings load and the platform-error path.
• "Four consumers wire these keys" leaves out SettingsSidebar and the three Manage Books dialog pickers (see #3).
• The <!-- review-paratext:summary --> block (checkboxes, "Author response", "Interview Notes", exit codes) is useful review history, but as the PR description it pushes the Exposure note and the note for @Sebastian-ubs to the very bottom. Consider moving it to a PR comment and keeping the description short: what was broken, the two causes, and the widened check.

11. Nit — new hard-coded English label

Attach to: extensions/src/platform-scripture/src/checklist.web-view.tsx:965

'Select primary Scripture text' is a new user-facing English string in a web view, and it duplicates extensions/src/platform-scripture/contributions/localizedStrings.json:723 word for word.

Details: Localization-Guide.md:53 says "Never hardcode English text in code that faces users." The library exception (DEFAULT_STRINGS) is for platform-bible-react components that can't call PAPI, not for a web view that already calls useLocalizedStrings. The string is now in three places (the JSON, this literal, and checklist.stories.tsx), and a typo fix in the JSON, which the immutable-strings rule allows in place, would leave the fallback out of sync. If a hard-coded fallback is still the right choice here, a short comment pointing to the JSON key would make the duplication intentional and easy to find.

12. Nit — Object.fromEntries loses field types

Attach to: lib/platform-bible-react/src/components/advanced/project-selector/project-selector.component.tsx:214

The Object.entries → filter → Object.fromEntries round-trip turns the spread into { [k: string]: string }, so Required<ProjectSelectorLocalizedStrings> no longer checks the values.

Details: I confirmed this with tsc --strict: { ...D, c: 'oops' } fails with TS2322 when c is a number, but { ...D, ...Object.fromEntries([['c', 'oops']]) } compiles. There's no risk today, because every field of ProjectSelectorLocalizedStrings is string? and partial is typed. It would start to matter if a non-string field is ever added. The typed loop suggested in #1 fixes this for free, and the PR's own summary lists it as an open question.

13. Nit — the vi.mock factory breaks the file's own rule

Attach to: extensions/src/platform-scripture/src/checklist.web-view.test.tsx:109

The hoisted vi.mock('@papi/frontend/react') factory now uses isProjectSelectorSharedKey / localizedValueFor, which are top-level imports. The comment at :124 in the same file says a hoisted factory "must not close over the file's top-level import bindings".

Details: It works only because ./project-selector.test-utils is imported before ./checklist.web-view. If an import sorter or a manual edit reorders them, collection fails with a temporal-dead-zone ReferenceError. manage-books.web-view.test.tsx:71 has the same shape. Either import inside the factory the way :124 describes, or update the rule comment to explain why static imports are safe here.

14. Nit — "so they cannot drift" isn't true

Attach to: lib/platform-bible-react/src/components/advanced/project-selector/project-selector.groupings.ts:62

isBlankLocalizedValue's doc says the two owners of "unresolved" share it "so they cannot drift on what counts as blank". But it isn't exported from experimental.ts, so checklist.web-view.tsx:130 writes its own candidate.trim() !== '' as a third check, and the two owners already disagree on what counts as a key (#6).

Suggestion: shorten the JSDoc to one line ("Whether a localized value has no visible text; whitespace-only counts as blank."). The shared helper from #1 is what would actually stop the drift.

15. Nit — two comments don't match the code next to them

Attach to: lib/platform-bible-react/src/components/advanced/project-selector/project-selector.groupings.test.ts:76

• :76 — the comment says the picker would render "%projectSelector_searchPlaceholder%", but the test below uses %projectSelector_clearAll%. Use the same key in both so a reader checking them against each other isn't thrown off.
• extensions/src/platform-scripture/src/checklist.web-view.test.tsx:239 — this comment describes the useLocalizedStrings mock but sits right above const OTHER_PROJECTS (:243), which it has nothing to do with. Move it above the describe.

16. Follow-up (not blocking) — the same bug elsewhere, and a standard that teaches it

Attach to: extensions/src/platform-scripture/src/checklist.web-view.tsx:128

firstUsableLabel fixes ?? fallback for two lines, but the same dead-?? pattern is still used elsewhere, and the repo's localization standard tells people to write it.

Details:

• A grep for localizedStrings\[[^]]*\] *\?\? in extensions/src/platform-scripture/src (tests excluded) finds 7 sites. They include manage-books.web-view.tsx:507 (the dock tab title), checklist.web-view.tsx:577 in this same file, find.component.tsx:882 and :1449, and manage-books-dialog.component.tsx:619. TypeScript can't flag these because noUncheckedIndexedAccess is off.
• .context/standards/Localization-Guide.md:181-186 still tells you to write localizedStrings?.['%…%'] ?? 'Select Chapter', and its example (book-chapter-control.component.tsx) has already moved to resolveLocalizedString.
• This is outside the diff, and the PR body already leaves the sweep across components out of scope, so I'm not asking for it here. A Jira ticket to (a) export one shared reader and (b) fix the guide would stop this bug from being written again.

---

Appendix — /code-review max final report (numbered, 30-item cap)

This is the raw output of the /code-review max pass (16 candidates per subagent, 30 in the final report). I checked it before merging it into the comments above. Where I disagree with the raw report, the note says so.

1. project-selector.component.tsx:216 — the check uses the looser of two "is this a key?" tests in the package (isLocalizeKey vs /^%[^%]*%$/). Merged into comment 1. Correction: the report's example '%s of %s' is wrong. That string doesn't end in %, so isLocalizeKey returns false. Real differences are values like '%' or '%d%'.
2. project-selector.component.tsx:216 — resolveStrings has no typeof value === 'string' check, so a non-string value crashes isLocalizeKey. Not raised: partial is typed ProjectSelectorLocalizedStrings with only string? fields, so this can only happen through an untyped caller. It's defensive hardening, not a live bug.
3. checklist.web-view.tsx:831/:967 — ariaLabel bare lookup; both pickers share one accessible name. Comment 4.
4. project-selector.component.tsx:728 — unmemoized resolveStrings now splits graphemes per render (78–232× measured). Comment 5. Confirmed the constructor splits eagerly; the multiplier wasn't re-measured.
5. manage-books-dialog.component.tsx:619 — dead ?? feeds three more pickers, and their wording is dropped. Merged into comment 3.
6. settings-sidebar.component.tsx:135, :176-177 — a fifth consumer, half fixed. Merged into comment 3.
7. project-selector.component.tsx:681, :1194 — grouping labels skip resolveStrings. Merged into comment 6.
8. checklist.web-view.tsx:962 — primaryProjectLabel can be a GUID. Comment 7.
9. find.component.tsx:975 / checks-side-panel.component.tsx:226 — fallback turns an informational placeholder into a wrong instruction. Comment 3 (confirmed en text "No open projects or resources").
10. project-selector.component.tsx:1062 — || undefined is dead, and the '' opt-out is gone. Comment 2.
11. checklist.web-view.tsx:962 — candidate order causes a label change once strings load. Merged into comment 7.
12. Seven localizedStrings[…] ?? fallback sites remain. Comment 16.
13. manage-books.web-view.tsx:919-920, manage-books-dialog.component.tsx:855/903 — raw keys reach section headings. Merged into comment 6.
14. groupings.ts:87 vs component.tsx:216 — the two layers disagree, and a test pins the difference. Merged into comment 6.
15. groupings.ts:62-65 — "cannot drift" is false; .trim() also differs from isWhiteSpace. Comment 14 (the isWhiteSpace point was left out as too minor).
16. component.test.tsx:551, :585 — the raw-key sweeps run against a DOM that doesn't contain most of the strings. Merged into comment 8.
17. component.test.tsx:527/541 — the headline test never reaches isLocalizeKey. Merged into comment 8 (noted that the second test does cover that branch).
18. component.test.tsx:587 — not.toHaveAccessibleName can never fail. Comment 8.
19. component.test.tsx:617-621 — the padded-whitespace test can't detect trimming (the groupings.test.ts:104 version does). Not raised separately: the lower-level test already pins this behavior.
20. component.test.tsx:586 — /%\w+_/ is too narrow, and queryByText throws on multiple matches. Merged into comment 8.
21. find.component.test.tsx:66, checks-side-panel.component.test.tsx:62 — PICKER_BOUND_*_KEYS are hand-maintained. Not raised as its own finding: there's no exported list to derive them from. The real problem (they hide the unresolved state) is in comment 3.
22. The test-utils stub change means no consumer suite tests the unresolved state for picker keys. Merged into comment 3.
23. checklist.web-view.test.tsx:111, manage-books.web-view.test.tsx:73 — hoisted vi.mock uses top-level imports. Comment 13.
24. checklist.web-view.test.tsx:273 — 'P1' comes from an incidental mock default, and afterEach doesn't reset mockProjects. Not raised: fragile but correct today. Mention it only if the author touches that suite again.
25. component.test.tsx:569, :638 — %webView_find_projectFilter_selectProject% doesn't exist. Comment 8 (confirmed by grep).
26. localization.util.ts:20 — resolveLocalizedString already exists. Comment 1.
27. checklist.web-view.tsx:965 — hard-coded English duplicates localizedStrings.json:723. Comment 11. The adversarial pass disputed this, since a hard-coded fallback is how this whole PR works. I kept it as a Nit because Localization-Guide.md:53 applies to web views.
28. component.tsx:214-218 — Object.fromEntries loses types. Comment 12, marked Nit after checking with tsc (no risk today).
29. Three wrong claims in the PR body (#2796, fr/km/zh, consumer count). Comment 10 (#2796's file list and BACKUP_LANGUAGE behavior both confirmed).
30. No update to the standards or the architecture decisions log; test stubs duplicated (stubLocalizedStrings, the useLocalizedStrings mock, localizedValueFor ×2). Standards part → comment 16. ADR request not raised: too heavy for a ~20-line bug fix. Duplicated stubs: not raised, since the cross-workspace localizedValueFor copy can't be avoided without exporting test helpers from platform-bible-react.

Findings from the earlier passes that the adversarial pass dropped

• Performance "no regression" (the adversarial pass's own conclusion): overruled. It relied on the PERF note in offsetAt, but the constructor at grapheme-string.ts:175 splits eagerly. Kept as comment 5.
• "metadata.json fallbackKey values disprove the PR's 'no %-wrapped values' claim": dropped. Those are keys by definition, not localized values; the PR's claim holds.
• "|| on undefined is not a nullish test" (on the firstUsableLabel JSDoc): dropped. The comment explicitly limits it to undefined.
• "Every suite here stubs…" is wrong: dropped. The mock is module-level and does apply to every suite. Only the comment's position is off (comment 15).
• The test name at checklist.web-view.test.tsx:274 "has its meaning backwards": dropped. Line number wrong, and the reading was reversed.
• Moving project-selector.test-utils.ts into platform-bible-react: dropped. That would ship test utilities in the library's public API.

@katherinejensen00 partially reviewed 20 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on jolierabideau).

---

lib/platform-bible-react/src/components/advanced/project-selector/project-selector.component.tsx line 214 at r1 (raw file):

  // the trigger with no text and no accessible name, which reads as a broken control rather than a
  // wrong label — `isBlankLocalizedValue` keeps that on the fallback path too.
  const resolved = Object.entries(partial).filter(

1. Important — reuse resolveLocalizedString

Attach to: lib/platform-bible-react/src/components/advanced/project-selector/project-selector.component.tsx:214

This package already has a tested helper for this exact problem, resolveLocalizedString in lib/platform-bible-react/src/utils/localization.util.ts:20, so this PR adds another copy of the rule instead of using it.

Details:

• resolveLocalizedString(value, fallback) exists for this bug. Its doc says useLocalizedStrings seeds { [key]: key }, so ?? 'Default' never falls back. It has its own test file, and BookChapterControl and RecentSearches already use it (15 call sites).
• After this PR, the "is this value usable?" rule exists in several places that don't quite agree: resolveLocalizedString, readProjectSelectorString (project-selector.groupings.ts:87), resolveStrings (here), firstUsableLabel (checklist.web-view.tsx:128), and older ones in the renderer (localizedOrEnglish, createCrashedViewLocalizer).
• It isn't a drop-in swap, and the differences matter:
  • resolveLocalizedString checks !value, so it catches '' but not whitespace-only strings. This PR treats whitespace-only as unresolved on purpose.
  • Its key test is /^%[^%]*%$/. isLocalizeKey is looser (startsWith('%') && endsWith('%')): a bare '%' or '%d%' counts as a key. No shipped translation looks like that today. But firstUsableLabel also runs this check on primaryProjectName, which a user chooses, so a project named like %ABC% would be skipped.
• Suggestion: extend resolveLocalizedString to cover whitespace-only values, then write resolveStrings as a typed loop over Object.keys(DEFAULT_STRINGS) that calls it for each field. That removes the third copy of the rule and also fixes #5 and #12. The firstUsableLabel candidate chain can then sit on the same helper.

[jolierabideau]

Rebased onto latest main (4 commits, clean — no overlap with this PR's files) and worked through all 16. Fixed 14, deferred 1 to a follow-up, pushed back on 0. Summary, then the two worth discussing.

1 — reuse resolveLocalizedString ✅

Taken, including the suggested shape. resolveLocalizedString now treats whitespace-only as unresolved, and backs two new shapes in the same file: isResolvedLocalizedValue (the predicate) and firstResolvedLocalizedString (the candidate chain). All three are exported from platform-bible-react/experimental, which is what let the consumer-side fixes in 3 and 4 use the same rule instead of a fourth copy. readProjectSelectorString, resolveStrings and the former firstUsableLabel now all route through it; isBlankLocalizedValue is gone.

Two notes on the differences you flagged. The regex (/^%[^%]*%$/) is now the only key test in play, so the isLocalizeKey looseness — '%', '%d%' — is moot, and so is the %ABC%-named-project hazard, since the project name is no longer a candidate (see 7). Widening resolveLocalizedString does change behavior at the 15 existing BookChapterControl/RecentSearches call sites: a whitespace-only translation there now falls back to English instead of rendering invisibly. I took that as strictly better and confirmed the recent-searches suite still passes.

2 — ariaLabel: '' opt-out ✅

Kept the opt-out, the RecentSearches way. resolveStrings special-cases ariaLabel === '', || undefined at :1062 is reachable again, and the test that had made the removal official is split in two: one asserting '' produces no aria-label attribute, one asserting whitespace-only still falls back. Reverting the special case fails the first and not the second.

3 — generic English replacing consumer wording ✅ (partly deferred)

Fixed for find and checks-side-panel: each override now falls back to its own English rather than the picker's, verified word-for-word against localizedStrings.json. Two tests each, and the wording test does fail if I revert the fallback — I checked.

Manage Books (3 pickers) and Settings are not fixed here, and are now listed in the PR body as you asked. manage-books-dialog.component.tsx:619's t() helper is the interesting one, since it's a single fix that covers all three of its pickers. It's in the follow-up (16).

4, 5, 6, 7 ✅

• 4 — both checklist pickers derive visible text and accessible name from one value, plus a getByRole('combobox', { name }) assertion per picker.
• 5 — useMemo, and the field-by-field merge means no GraphemeString construction at all now.
• 6 — one predicate in both layers. The does not treat a DIFFERENT key as unresolved test is inverted to treats a DIFFERENT key as unresolved too, with a comment saying why grouping labels need it: they're read straight off the grouping objects and never see the merge.
• 7 — dropped primaryProjectLabel from the chain, which restores exactly the pre-PR behavior since it was never reachable before. That left the platform.name read with no consumer, so the primaryProjectName state and its async effect went too (~25 lines). Flagging that explicitly since it's pre-existing code from feat(project-selector): grouping options (open tabs, last used, language, versification, type) #2673, not something this PR added — say the word if you'd rather keep the read for a future use.

8 ✅

All five. :587 removed (the getByRole name match already proves it). The headline test renamed to falls back to English when the shared block resolved nothing with a comment saying the builder rejects key-as-value itself, so it pins the undefined arm and the second test is what reaches the key arm. %webView_find_projectFilter_selectProject% → the real …_noOpenProjectsOrResources%. Sweeps are now one RAW_KEY = /%[^%\s]+%/ with queryAll…().toHaveLength(0). And both tests now render with availableGroupings and open the group-by menu, so the sweep sees the menu strings rather than only the popover's.

9, 11, 13, 14, 15 ✅

Six copies down to one. The full explanation lives on isResolvedLocalizedValue — its doc enumerates the three unresolved states — and everything else points at it. resolveStrings' 18-line block is ~8, project-selector.test-utils.ts is down to two short blocks, groupings.test.ts:76 names clearAll to match its test, and the checklist comment moved above its describe. On 13: the rule comment was the thing that was wrong, so it now says what actually makes the difference — react is in the mocked module's own graph, ./project-selector.test-utils isn't, so the latter is fully initialized before the hoisted factory runs. On 11: the literal survives (it's the last-resort candidate, same as the rest of this PR) but now sits under a comment naming the JSON key it mirrors.

12 ✅ — but not the typed loop

Object.keys(DEFAULT_STRINGS).forEach needs a keyof assertion, which this package's no-type-assertion rule rejects, and suppressing it seemed like the wrong trade for a typing fix. So resolveStrings spells all 16 fields out explicitly. Verbose, but it's strictly stronger than the loop: with the Required<…> return type, a missing field is now also a compile error, not just a mistyped one.

10 ✅

Description rewritten. All three errors confirmed before correcting — gh pr view 2796 --json files shows no project-selector*, and localization.service-host.ts:339 does always append BACKUP_LANGUAGE. The fr/km/zh claim was wrong and is now called out as wrong rather than quietly dropped. The review-summary block is gone from the description; the #2796 note now says plainly that whichever of us merges second removes the redundant filter and its comment at settings-sidebar.component.tsx:112-119.

16 — deferred, written up

Not done here, per your "not asking for it" and my own out-of-scope note. Written up as a follow-up rather than lost: ~74 sites repo-wide match the call shape (7 in platform-scripture, of which 3 are benign — they fall back to the key itself or '', so the dead ?? costs nothing). The write-up leads with fixing Localization-Guide.md:181-186, since the guide is what keeps regenerating this, and flags a paranext/ lint rule as probably the highest-leverage piece. Happy to file it in Jira if you'd rather it be on the board.

Verification

npm run typecheck, npm run lint (0 errors — the one remaining warning is the pre-existing unsubscriber-async-list.test.ts one), npm test all green across 11 projects. The four platform-scripture suites (80 passing) were run from extensions/src/platform-scripture — your point about the root config's include excluding extensions/src/** still stands and still needs confirming against CI. dist/ rebuilt and committed.

[jolierabideau]

Round-1 review addressed across 91c6dfd and 9112253. 15 fixed, 1 deferred to a ticket.

Fixed

1 — reuse resolveLocalizedString. Done, and it collapsed 5, 6, 12 and 14 with it. localization.util.ts now treats whitespace-only as unresolved and exposes the rule three ways — isResolvedLocalizedValue, resolveLocalizedString, firstResolvedLocalizedString — exported from platform-bible-react/experimental so consumers judge their own %webView_…% lookups the same way the picker does. readProjectSelectorString, the picker's merge and the checklist's candidate chain all call it; the third copy is gone. isLocalizeKey is no longer on this path at all, so the %ABC%-shaped project name you flagged is no longer skipped.

2 — ariaLabel: ''. Kept as the opt-out, matching recent-searches.component.tsx:95. || undefined at the trigger is reachable again, the prop's TSDoc says so, and the test is split: whitespace-only falls back, '' asserts no aria-label attribute.

3 — consumer wording. Find and the checks side panel carry their own English now, so "No open projects or resources" and "No project" survive an unresolved lookup instead of becoming "Select a project". Each gained an unresolved-state test — the case their PICKER_BOUND_*_KEYS stubs had hidden. Manage Books (3 pickers) and Settings are listed in the PR body as known-remaining and are in PT-4673's scope; say the word if you'd rather they land here.

4 — checklist ariaLabel. Both pickers derive visible text and accessible name from one value, with a test asserting the two toolbar comboboxes have distinct accessible names.

5 — per-render grapheme split. Gone twice over: the regex path replaces isLocalizeKey, so no GraphemeString is constructed, and resolveStrings is memoized on props.localizedStrings.

6 — two definitions of "unresolved". One predicate. readProjectSelectorString drops value === key for the shared one, so grouping labels — read straight off the grouping objects — get the same guard as everything else. The DIFFERENT key test is inverted to match.

7 — primaryProjectLabel. Dropped, and the platform.name read behind it with it, since nothing else consumed it. That removes both halves: the raw GUID on the error path, and the label changing once strings load. In mode="project" the picker already renders the selected project, so the read was redundant with the component.

8 — tests that can't catch their regression. The not.toHaveAccessibleName assertion is gone; the raw-key sweeps broadened to any %…% key with queryAll + a length assertion, run against a DOM with the group-by menu open; the headline test named for what it actually covers. %webView_find_projectFilter_selectProject% — which exists nowhere in the repo — is replaced by Find's real %webView_find_projectFilter_noOpenProjectsOrResources% (9112253). Every new guard was confirmed falsifiable by reverting it and watching the intended test fail.

9 — six copies of the explanation. Down to three, one per package where it's load-bearing for that file's reader. resolveStrings's 18-line block is a short pointer now; project-selector.test-utils.ts is 25 lines with 8 of comment.

11 — hard-coded English. Kept — it's how the whole fallback works — with a comment naming the localizedStrings.json key, so the duplication is deliberate and greppable.

12 — Object.fromEntries loses types. Gone; resolveStrings merges field by field against DEFAULT_STRINGS, so a missing or misspelled field is a compile error again.

13 — vi.mock factory. Fixed in 9112253. Both factories (checklist and manage-books) import inside the factory now, aliased so the in-factory bindings don't shadow the top-level ones. You were right that it only worked by import order.

14 — "so they cannot drift". isBlankLocalizedValue is gone entirely. The shared exported predicate is what actually stops the drift, as you said.

15 — comments beside the wrong code. Both fixed: the groupings.test.ts comment names the key its test uses, and the checklist mock comment sits above the describe.

10 — PR description. Rewritten. The #2796 and fr/km/zh corrections are in; the consumer count is three (four pickers); the ?? fallback figure is re-counted at ~83 with a per-area breakdown and the caveat that not every site is a live defect; the removed platform.name read is called out; PT-4673 is linked.

Deferred

16 — the same bug elsewhere, and the standard that teaches it. Filed as PT-4673 under PT-4530 — export one shared reader, rewrite Localization-Guide.md:181-186, triage the ~83 sites (your named ones listed individually), and decide on a lint rule so the idiom can't be reintroduced. Not in this PR.

Verification

Branch is up to date with origin/main. npm run typecheck, npm run lint (0 errors) and npm test green; platform-bible-react unit 965 tests, platform-scripture 631 tests, prettier --check clean across both src/ trees.

One thing worth knowing if you re-run locally: the three Storybook-project failures on project-selector.stories.tsx and the two settings-sidebar stories are a stale Vite dep-cache for platform-bible-utils, not a regression — they reproduce with all changes stashed, and PROJECT_SELECTOR_CUSTOM_DATA_KEYS is present in the committed dist.

[katherinejensen00]

PR Review Report: PR #2829 (round 2)

Repository: paranext/paranext-core
PR: #2829 — "Fall back to English when ProjectSelector's localized strings are unresolved"
Author: @jolierabideau · Branch: fix/project-selector-unresolved-localization-keys
Reviewed at: 9112253459a against merge-base e8ffa3e7488
Date: 2026-09-18
Mode: dry-run (not posted — drafted for the user to post)
Reviewers: security, scope, ux, tests, clarity (panel auto-narrowed: no C# files → contracts/architecture dropped)
CI: all green — 3 Build jobs, CodeQL, Analyze (csharp + javascript). Reviewable shows 1 discussion left.

lib/platform-bible-react/dist/ was not reviewed (generated). It IS rebuilt and in sync: the dist diff
adds all three new declarations to dist/experimental.d.ts, and CI's "no files changed after build"
check passes.

Round-1 verification

Round 1 raised 16 findings. 15 are fixed and verified; 1 (#16) is deliberately deferred.

#	Round-1 finding	Verdict
1	Reuse resolveLocalizedString instead of a 3rd copy of the rule	Fixed — one rule in localization.util.ts; isBlankLocalizedValue/firstUsableLabel gone. See new finding F/G on the shape of the result
2	ariaLabel: '' opt-out removed; || undefined dead	Fixed — carve-out at project-selector.component.tsx:209-212, || undefined reachable at :1108, test split in two
3	Generic English replaced consumer wording	Fixed for find + checks-side-panel, English verbatim vs localizedStrings.json. Manage Books + Settings deferred → see B
4	Both checklist pickers got the same accessible name	Fixed — distinct in every state, incl. unresolved
5	Grapheme split per render, not memoized	Fixed on the expensive half (no GraphemeString at all now). The useMemo itself is a no-op → see E
6	Two definitions of "unresolved"; grouping labels on the weaker one	Fixed — one predicate, DIFFERENT key test inverted
7	Fallback could show a GUID; label changed once strings load	Fixed — primaryProjectName state + platform.name read deleted outright
8	Vacuous assertion / test not reaching its branch / nonexistent key	Mostly fixed — not.toHaveAccessibleName gone, key replaced with a real one. One new sweep is vacuous → see C
9	Same explanation at 6 places	Fixed in source (:199, groupings.ts:63 now point at the predicate). Recurs in 7 test files → see L
10	PR description: stale #2796 warning, wrong fr/km/zh claim, pasted review summary	All three fixed. New body issues → see D
11	Hard-coded English duplicating localizedStrings.json	Fixed — all 8 fallbacks verbatim-correct, duplication justified at each site
12	Object.fromEntries round-trip loses field types	Fixed — field-by-field merge
13	Hoisted vi.mock factory closed over a top-level import	Fixed (commit 9112253). Justifying comment now self-contradictory → see I
14	"So they cannot drift" was untrue	Fixed — comment gone
15	Comment named searchPlaceholder but test used clearAll	Fixed
16	7 more sites with the same bug; Localization-Guide still teaches it	Deferred to PT-4673 — but the ticket appears nowhere in the tree → see A

Summary table

Perspective	Blocking	Warning	Suggestion
Security	0	0	0
Scope	0	3	2
Tests	0	1	3
Clarity	0	0	5
UX	0	1	1
Total	0	5	11

No blocking findings. Recommended event: COMMENT.

New findings (round 2)

A — Important. The PT-4673 deferral is recorded nowhere in the tree, and the guide still teaches the bug

grep -rn PT-4673 over the whole worktree (*.ts, *.tsx, *.md) returns zero hits. The deferral
lives only in the PR body and in Jira, and the PR body does not survive as a searchable pointer after
squash-merge. Raised independently by the scope and UX passes.

.claude/rules/code-quality/forward-facing-comments.md explicitly keeps "an open PT-XXXX for
follow-up work deliberately not done in this PR — best written as a TODO naming the open ticket".

Two places to add one:

1. lib/platform-bible-react/src/utils/localization.util.ts:16 — the canonical doc block describing
   the broken idiom is the natural home for a TODO(PT-4673) noting the remaining sites.
2. .context/standards/Localization-Guide.md:181-186 — still teaches the exact dead form
   (localizedStrings?.['%webView_bookChapterControl_selectChapter%'] ?? 'Select Chapter'). This is
   the thing that regenerates the defect: an author writing a new component from that guide today
   reproduces it. Root CLAUDE.md ("Promote settled conventions") asks that a hardened rule be
   folded into the relevant standard in the same change, not left to a follow-up.

B — Important, decision needed. Should Manage Books land here?

File: extensions/src/platform-scripture/src/manage-books.web-view.test.tsx:67

The author asked: "say the word if you'd rather they land here." The scope and UX passes converged on
the same answer: split the deferral — land Manage Books, keep Settings deferred.

• The two halves are not alike. settings-tab.component.tsx:305 sits in files #2796 is actively
  editing, so deferring it avoids a real conflict, and the body says so.
  manage-books-dialog.component.tsx:617-620's
  t = (key, fallback) => localizedStrings[key] ?? fallback has no such excuse — by the author's own
  account it is a single function body covering all three of its pickers, in the same extension and
  picker family as the three consumers this PR does fix. The fix is mechanical:
  localizedStrings[key] → firstResolvedLocalizedString(localizedStrings[key], fallback), reusing
  the helper this PR already exports; every call site already passes a sensible English fallback, so
  no new strings.
• This PR is already inside Manage Books. The diff reworks manage-books.web-view.test.tsx's
  useLocalizedStrings stub so the shared %projectSelector_*% block resolves. That leaves Manage
  Books' suite passing through the fixed path while the component keeps the bug — the least
  defensible resting place, because the suite now looks like it covers Manage Books' localization
  and does not.
• Leaving it also makes the unresolved state actively worse than before in one respect: the Copy
  "From" picker (:2351), the Create "Based on" picker (:2647) and the sidebar picker
  (manage-books-sidebar.component.tsx:310) all now resolve to the library's identical generic
  'Projects & resources' / 'Select a project' — three comboboxes in one dialog with the same
  accessible name, where before they at least had distinct (ugly) raw-key names.

If the PR stays as-is instead, the Manage Books test-stub change should not land here either.

C — Important. One of the new raw-key sweeps cannot fail

File: lib/platform-bible-react/src/components/advanced/project-selector/project-selector.component.test.tsx:566-567

In the first test of the new unresolved localized strings block, both RAW_KEY sweeps are vacuous,
and the comment at :564 calls them "the load-bearing half" — the opposite of what they are. The test's
own comment at :534 explains why: buildProjectSelectorLocalizedStrings(UNRESOLVED_STRINGS) rejects
key-as-value itself, so the bag reaches the merge as all-undefined. Every value the render can
produce is then undefined, an English DEFAULT_STRINGS entry, or the literal ariaLabel: 'Project';
availableGroupings are literals and SAMPLE_PROJECTS (:63-67) contain no %. No raw key can enter
this DOM under any behavior of resolveStrings, so reverting the production fix cannot trip these two
lines — the English assertions at :561-562 are what actually fail.

Same class as round-1 #8. In the second test (:600-601) the identical two lines ARE load-bearing,
because the consumer overrides there are real raw keys.

Fix: drop the sweeps from the first test and let the English assertions carry it, or feed that test a
bag that actually contains raw keys at the merge.

D — Warning. PR body: undocumented blast radius, inconsistent counts, review-process residue

File: PR description

1. The body documents the widening ("it now also treats whitespace-only values as unresolved") but
   never says it changes behavior at call sites this PR is not about. resolveLocalizedString has 15
   pre-existing callers outside the picker — 9 in book-chapter-control.component.tsx, 4 in
   book-chapter-control.navigation.ts, 2 in recent-searches.component.tsx. That fact is in the
   author's round-2 comment, not the body, and the body is what a reader of the squash-merge commit
   sees. Add one line to "The fix".
2. Site count is inconsistent across artifacts: the body says "~83" with a breakdown summing to 75;
   the first round-2 comment says "~74". Pick one number and one method.
3. "On #2796" and "Scope note on locales" are written as corrections of earlier revisions of the
   description ("Earlier revisions of this description called #2796 a major conflict. That was
   wrong"), and "Worth confirming CI covers them somewhere." is a reviewer-directed ask. Round-1
   #10 was about exactly this class of residue surviving into the body: state the current facts
   directly and move the asks to a PR comment.

E — Warning. The useMemo on resolveStrings is a no-op in exactly the state this PR targets

File: lib/platform-bible-react/src/components/advanced/project-selector/project-selector.component.tsx:774

Verified directly: src/renderer/hooks/papi-hooks/use-localized-strings-hook.ts:45-48 builds
const defaultState: { [key: string]: string } = {} in the render body, and :54 returns
isPlatformError(localizedStrings) ? defaultState : localizedStrings — a fresh object every render on
the error path, which is the permanent raw-key state this PR exists for. So localizedStrings changes
identity every render, the consumer's own useMemo recomputes, the prop object is fresh, and this memo
misses on every keystroke in the search box.

Not worth blocking now that the work is ~16 cheap regex tests — round-1 #5's expensive half (the
per-render grapheme split) is genuinely gone. But the memo does not do what the PR body claims. Either
give it a value-based dependency (or make useLocalizedStrings return a stable defaultState), or
drop it so no one reads it as a guarantee.

Consumer call sites are otherwise fine: checklist.web-view.tsx:744, find.component.tsx:974 and
checks-side-panel.component.tsx:225 all wrap in useMemo keyed on localizedStrings.

F — Warning. Three new exports on experimental, one external consumer

File: lib/platform-bible-react/src/experimental.ts:40

Every extension import in the diff is firstResolvedLocalizedString (checklist.web-view.tsx:18,
checks-side-panel.component.tsx:21, find.component.tsx:49). isResolvedLocalizedValue and
resolveLocalizedString are used only inside lib/platform-bible-react/src, where the
@/utils/localization.util import already reaches them — nothing outside the library imports either.
So two of three new public exports are speculative surface, and the comment at :35-38 ("shared with
consumers so a web view merging its own %webView_…% lookups judges them the same way") is
aspirational rather than a description of the code.

Also a name collision worth avoiding:
extensions/src/platform-scripture-editor/src/scripture-text-grid/view-options-notice.utils.ts:29
exports its own resolveLocalizedString(localizedStrings, key) — different signature, different
contract, untouched by this PR. Exporting this one under the same name from experimental sets up a
genuinely confusing choice for the next person who reaches for it there. Either fold that copy onto
the shared helper or note in the body that it was left out.

Suggestion: export firstResolvedLocalizedString now, add the other two when a consumer needs them —
or, if the trio is deliberate (a defensible argument: the predicate is the definition the docs point
at), say so in the export comment so the unused pair doesn't read as an oversight.

G — Suggestion. firstResolvedLocalizedString is variadic but never called with more than 2 args

File: lib/platform-bible-react/src/utils/localization.util.ts:51

All 8 call sites pass exactly two arguments, the second a literal English string that always resolves:
checklist.web-view.tsx:777, :781; checks-side-panel.component.tsx:229, :233, :237;
find.component.tsx:979, :983, :987. In that shape it is resolveLocalizedString(value, fallback)
with a worse return type — string | undefined, so every call site's result is nullable when it never
can be. The TSDoc's motivating case (a consumer override, then a setting, then English) has no caller.

Consider collapsing those 8 sites onto resolveLocalizedString and adding the variadic form back when
a third candidate actually appears. That also shrinks the newly exported triple to the one function
consumers need, resolving F.

H — Suggestion. Two new tests restate the previous assertion instead of pinning a changed line

Files: extensions/src/platform-scripture/src/find/find.component.test.tsx:328,
extensions/src/platform-scripture/src/checks/checks-side-panel/checks-side-panel.component.test.tsx:187

With projects: [] the trigger's only text is buttonPlaceholder, which the test immediately above
already asserts exactly ('No open projects or resources' / 'No project'). If that regressed to a
raw key, the first test fails too — so neither of these can fail independently.

Meanwhile the other new fallbacks on the changed production lines are unpinned: neither new test opens
the popover, so commandEmptyMessage → 'No projects found' (find.component.tsx:981-984,
checks-side-panel.component.tsx:231-240) is never exercised on the unresolved path, and the
pre-existing suites can't cover it because stubLocalizedStrings now hands the picker-bound keys a
resolved value. Better spent opening the picker under UNRESOLVED_STRINGS, typing a non-matching
query, and asserting 'No projects found' — as the library test at
project-selector.component.test.tsx:590-596 does.

I — Suggestion. Two comments 25 lines apart give contradictory vi.mock hoisting rules

File: extensions/src/platform-scripture/src/checklist.web-view.test.tsx:131

The round-2 code fix is correct — the @papi/frontend/react factory at :105 is async and imports the
helpers inside itself, which is unconditionally safe under vitest hoisting. But :106-107 states flatly
that "a hoisted vi.mock factory must not close over the file's top-level import bindings", while
:131-132 carves out an exception saying bindings from ./project-selector.test-utils are "safe by
contrast". The stated reason for the carve-out is also not the reason it holds: what matters is whether
the helper module is evaluated before the factory runs (it is, because checklist.web-view is loaded
lazily inside getChecklistWebView()), not whether "the mocked module graph imports it". A reader
following :131 could move the helpers back into a top-level closure and reintroduce the
temporal-dead-zone failure :106 warns about.

Keep one rule — import inside the factory, full stop — and drop the carve-out.
manage-books.web-view.test.tsx:67-71 applies the same fix without it.

J — Suggestion. The renderer's two older copies stay on the weaker rule

File: lib/platform-bible-react/src/utils/localization.util.ts:25

localizedOrEnglish (src/renderer/components/overlays/overlay-connection-lost.component.tsx:88-94)
and createCrashedViewLocalizer (src/renderer/components/crashed-view.util.ts:102-111) both test
!value || value === key — identity against the specific key only, no blank/whitespace check. So a
value that is a different %…% key, or whitespace-only, still renders at the user on the
crash/connection-lost screens, which is exactly the state those screens exist for. Out of scope to
change here; worth a one-line pointer or a body note so the next reader knows they were left on the
weaker rule deliberately.

K — Suggestion. An Architecture-Decisions entry looks warranted

File: lib/platform-bible-react/src/experimental.ts:35

Nothing under .context/ is touched, and no existing slug covers the ground (adr-library-string-keys-ship-in-shell-assets
is about where string values ship, not how an unresolved one is detected). Two decisions here meet
root CLAUDE.md's bar:

1. One shared definition of "unresolved localized value", exported from experimental so consumers
   judge their own %webView_…% lookups by the component's rule — a new pattern plus new top-level
   public surface, and exactly what the next component author will otherwise re-derive as a fourth
   local copy.
2. A consumer owns its English fallback rather than inheriting the component's — visible in find
   keeping "No open projects or resources" and checks-side-panel keeping "No project".

One entry covering both would also be the right home for the PT-4673 deferral rationale and the
whitespace widening. Insert in byte-order slug position (LC_ALL=C sort), not at the end: a slug
like adr-unresolved-localized-value-is-one-predicate lands between adr-tour-offered-in-both-modes
and adr-unresolvable-spdx-operators-drop-the-dependency.

L — Suggestion. The seeding explanation is now repeated in 7 test files

File: extensions/src/platform-scripture/src/find/find.component.test.tsx:311

Round-1 #9 is fixed on the source side, but the useLocalizedStrings key-as-value seeding is
re-explained in near-identical prose at checklist.web-view.test.tsx:112 and :246,
checks-side-panel.component.test.tsx:169, find.component.test.tsx:311,
manage-books.web-view.test.tsx:74, project-selector.component.test.tsx:523,
project-selector.groupings.test.ts:24. Now that project-selector.test-utils.ts is the shared home
for these stubs, the explanation could live once in its TSDoc with the rest saying "see
localizedValueFor". PR-level note, not per-site.

Adjudicated down (raised by a pass, dropped by me on verification)

Two UX findings described real problems but attributed them to this PR. Both are pre-existing; I
verified against the merge-base and am not raising them against #2829.

1. "aria-label replaces the trigger's content, so a screen-reader user never hears the selected
   project's short name." Real a11y issue, but not introduced here: at merge-base the trigger
   already had aria-label={strings.ariaLabel || undefined} on the role="combobox" Button
   (:1037), DEFAULT_STRINGS.ariaLabel already existed (:172), and the checklist already passed
   an ariaLabel for both pickers (:813, :946). The PR changed which string, not the mechanism.
   Worth its own ticket for the library.
2. "New wrong state: the primary-project trigger reads 'Select primary Scripture text' while the
   project list loads, and no isLoading is passed." Also pre-existing and textually identical to
   before: %markersChecklist_toolbar_primaryProject% is verbatim "Select primary Scripture text"
   (localizedStrings.json:723), so merge-base rendered the same text in that window once strings
   loaded. The isLoading={isLoading} at :1040 belongs to ChecklistTool, not the picker — a
   pre-existing gap the PR neither creates nor worsens.

Also noted and not raised: the whitespace widening's scope. The scope pass's verdict, which I agree
with, is keep it here. The old body was if (!value || PATTERN.test(value)) return fallback, so ''
already went to the fallback; the only newly diverted input is a translation that is non-empty but
trim()s to empty, which in shipped locale JSON is a data defect and never an intentional opt-out.
The two intentional-blank sites (recent-searches.component.tsx:97 and now resolveStrings) both
guard on ariaLabel === '' before calling the helper, so the widening can't swallow them. The
obligation is documentation (finding D), not separation.

Security found nothing: no secrets, no widened sandbox surface, no ReDoS in /^%[^%]*%$/ (anchored,
single negated class, no nesting), and the GUID leak path was deleted rather than patched.

Line validation

All findings anchored to lines in the PR's version of each file and spot-checked against the files in
the PR-head worktree. No corrections needed. D anchors to the PR description (no line).

@katherinejensen00 partially reviewed 28 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on jolierabideau).