Onboarding goals step: ask about the selected language, not hardcoded English - #338
Chevron7Locked wants to merge 3 commits into
Conversation
The goals step asked "What do you want to use English for?" no matter
which language the learner picked: the sentence was hardcoded in all 10
message files. Parameterize it as {language} and pass the selected
language's display name from the page (native name via
getLanguageByCode, capitalized per formatLanguageName).
Add an i18n regression test that fails if any locale stops templating
the subtitle.
Three defects in the first pass, found in audit:
- The body claimed the codebase carries no per-locale translations of
language names. False: every message file ships a full
targetLanguages catalog (de: Spanisch, es: francés...). Use it —
the subtitle now names the language exactly as the selector button
does, via useTranslations('targetLanguages'), in every UI locale.
The native-name detour (getLanguageByCode/formatLanguageName) is
removed.
- The body claimed omitting CHANGELOG.md matches repo practice.
False: AGENTS.md rule 4 requires changelog entries for user-visible
changes and every comparable recent change has one. Entry added.
- The claimed verification covered only string content, not wiring.
The new page-level test renders the real onboarding flow with real
next-intl interpolation and asserts the subtitle names the selected
language ('What do you want to use Spanish for?...'). Proven to fail
against the first-pass code and pass against this one.
…folding - The changelog entry still gave the native-name example from the abandoned first pass; it now describes the localized-name behavior that actually ships. - Drop the unused mockReplace handle and tighten two comments in the page test.
|
Edit: disregard the follow-up PR references below — those PRs have been closed and withdrawn at the author's request. #338 stands on its own. |
artcc
left a comment
There was a problem hiding this comment.
Thanks for this — the underlying bug is real and worth fixing: the goals step did ask about English no matter which language was chosen. Parameterizing onboarding.goals.subtitle, updating all ten catalogs consistently, adding a regression test and a changelog entry is exactly the right shape of fix, and it's appreciated.
I'd like to flag one thing before this lands: which value fills {language}.
tLang(targetLanguage) uses the full-code entries in targetLanguages, i.e. the selector-button labels for variants (en-GB → "British English", es-ES → "Español"). Those read well on a button, but not always in a sentence:
en-GB(the default) → "What do you want to use British English for?" — names the variant rather than the language ("English").- es → "¿Para qué quieres usar Español?" — language names are lowercase in Spanish, and the natural phrasing is "usar el español".
- fr → "Pour quoi voulez-vous utiliser Espagnol ?" — needs the article and lowercase: "utiliser l'espagnol".
- it → "usare Spagnolo" — "usare lo spagnolo".
- pt, ro, ru → same class of issue (article and/or capitalization).
- pl → "używać Hiszpański" — also needs the genitive: "używać hiszpańskiego".
- de, nl, en read fine, because those locales capitalize language names and don't need an article here.
The repo already has a pattern for this kind of interpolation: BeginnerGate receives activeLanguage?.iso639 (frontend/src/app/(app)/assessment/page.tsx:466) and passes it to tLang(...) (frontend/src/components/assessment/BeginnerGate.tsx:31). The bare-code entries in the catalogs are the lowercase, language-level names ("español", "espagnol", "hiszpański"), and formatLanguageName() (frontend/src/lib/target-languages.ts:177) exists for the same purpose, used by the lesson page. Using the bare code plus formatLanguageName would name the language instead of the variant and fix the capitalization in the locales that don't capitalize language names. Article/grammatical-case wording in fr/it/pl/ro may still need per-locale phrasing, but that is true for the existing {language} messages too.
It would also be worth asserting the assembled sentence in a non-English UI locale (es or fr) in the tests: the i18n test only checks that {language} is present, and the page test renders the English UI, so the current tests can't catch this.
One small edge case, low priority: tLang(targetLanguage) now runs with the raw state, which can come from ?language=. If that parameter is invalid and someone reaches step 2 before fetchLanguages resolves — or it fails, leaving availableLanguageCodes empty — next-intl can throw MISSING_MESSAGE, whereas the subtitle was static before. Resolving through getLanguageByCode() with a fallback would avoid it.
Happy to help rework this if useful.
|
Thanks again, @Chevron7Locked, for spotting this bug and putting together the original fix and regression tests. Your contribution helped identify a real inconsistency in the onboarding experience. We've opened a maintainer follow-up in #343 to carry the fix forward and address the remaining points from the review:
The follow-up has passed the full local pre-push checks: backend tests, frontend lint/typecheck, and frontend tests. It is now awaiting review and merge; this PR can be closed once that replacement is integrated. Thank you for taking the time to report and work on this! |
What
On the goals step of onboarding, the question always said "What do you want to use English for?" — no matter which language the learner had just picked. Pick Spanish, get asked about English. The sentence was hardcoded, with the language name baked in, in all 10 message files (
messages/*.json).Fix
Parameterize it as
{language}and supply the value from the page:messages/*.json: theonboarding.goals.subtitlestring now uses a{language}placeholder in all 10 locales (one line each; each locale's wording is otherwise untouched).frontend/src/app/(auth)/onboarding/page.tsx: passes the selected language's localized name from thetargetLanguagescatalog (useTranslations('targetLanguages')), the same names the selector buttons on the previous step render — "Spanish" in the English UI, "Spanisch" in the German one.The subtitle now reads "What do you want to use Spanish for? Select all that apply." (English UI, Spanish selected), and the equivalent in whichever UI locale the learner is using.
CHANGELOG.mdhas an entry under 1.9.15 Fixed.Tests
frontend/tests/i18n/onboarding-messages.test.ts(new): every locale's subtitle must contain{language}and must not name a language outright. Fails ondevelop, passes here.frontend/tests/app/onboarding-goals-subtitle.test.tsx(new): renders the real onboarding flow with real next-intl strings and interpolation — the repo's usual next-intl mock echoes keys back and would not catch this bug — and asserts the goals step shows "What do you want to use Spanish for? Select all that apply." when Spanish is selected. Also verified it fails against the unfixed page and passes with this change.Verification
On this branch:
CI ran green on an earlier head of this branch and will re-run on the current one.
Docs
No spec changes needed — the specs describe the selected target language driving the experience, which is what this restores. Changelog entry included as above.