feat(i18n): add per-instance language selection mechanism - #1442
Conversation
30bab68 to
87ab4f0
Compare
gdevenyi
left a comment
gdevenyi
left a comment
There was a problem hiding this comment.
Review — per-instance language selection
The mechanism is the right shape: store the active set on SetupState, derive the toggle options from it, hide the toggle when only one language is active. Hiding rather than disabling the toggle is a nice touch, and disabling the last remaining checkbox is the correct guard.
Two things make this hard to accept as-is: the perimeter isn't validated, and the type widening was absorbed by casting at ~8 call sites rather than fixed once.
Blocking
-
activeLanguages: z.array(z.string())validates nothing.PATCH /v1/setup {"activeLanguages": ["klingon"]}is accepted and persisted.Object.entries(ALL_LANGUAGES).filter(([key]) => activeLanguages.includes(key))then yields{},Object.keys(...).length > 1is false, and the language toggle disappears from the navbar and sidebar for every user on the instance, with no way to restore it through the UI. AGENTS.md is explicit here — strict data validation at the boundary, and fail loudly on an undeclared policy. This should bez.array($Language).min(1)with$Language = z.enum(['en', 'es', 'fr'])exported as the single source of truth. -
The casts move the problem instead of solving it. Widening
Languageto includeesbroke indexing into the{ en, fr }objects in$BrandingConfigandGroupSettings, and the fix wasas undefined | { [key: string]: string }at four spots inLoginBrandingPanel, one inlogin.tsx, one inStartSessionForm, plusresolvedLanguage === 'en' || resolvedLanguage === 'fr' ? resolvedLanguage : 'en'repeated five times. AGENTS.md: "no casting at call sites", "shape is never repeated", "concentrate the cost". The actual defect is that those schema fields are typed{ en: string; fr: string }instead of being keyed byLanguage. Fixing them once inpackages/schemasmakes every one of these call sites type-check unmodified. (#1439 introduces a$LocalizedStringthat is close to what's needed — worth converging on one, inschemas/core.) -
No tests. AGENTS.md requires unit tests and e2e tests for new changes; this has neither, and both manual boxes in the test plan are unchecked. Minimum viable set: a
$UpdateSetupStateDataschema test for the accepted/rejected language arrays, aSetupServicetest thatactiveLanguagesround-trips (the pattern is right there in #1441'ssetup.service.spec.ts), and an e2e that deactivates a language and asserts the toggle disappears.
Should fix
-
A failed autosave still reports "All changes saved" —
onSettledruns on error. Same issue as #1441; see inline. -
Deactivating a language strands users who had it selected.
i18n.changeLanguageis called only for the admin flipping the checkbox. Every other user whose persisted language was just removed keeps rendering in it, and the toggle no longer offers a way out. Needs a reconciliation on app load: if the resolved language isn't inactiveLanguages, switch to the first active one. -
['en', 'fr']is hardcoded as the default in four places here (setup.service.ts,Navbar,Sidebar,settings.tsx) and five more in #1439. IfSetupServiceguarantees a non-empty array — which it already tries to — no client needs a fallback at all. -
The gateway carries a fifth hardcoded language list and doesn't consult
activeLanguages, so?lang=esworks there even when Spanish is deactivated instance-wide. -
ALL_LANGUAGES: { [key: string]: string }— a loose record over a closed key set, which AGENTS.md rules out and which is what forces the?? codefallbacks in #1439's consumers. -
The libui bump isn't here. The PR notes this depends on libui#107 for libui's own Spanish strings. Until that lands and is released, selecting Español leaves every libui-owned string (form submit/reset buttons, search placeholder, date pickers, notification titles) in English. Should this merge before the bump, or wait?
Coordination with the other open PRs
| Conflict | Files |
|---|---|
| #1442 × #1441 | apps/web/src/routes/_app/admin/settings.tsx (both replace the staged-Save block with incompatible autosave layouts — #1441 adds a Settings section and SettingSection wrappers, this adds a Languages card) and apps/web/src/components/SaveStatus.tsx (add/add) |
| #1442 × #1439 | apps/web/src/components/SaveStatus.tsx (add/add); apps/web/src/utils/languages.ts, activeLanguages in schema.prisma / setup.service.ts / $SetupState are duplicated verbatim between the two |
Where the two autosave implementations differ, #1441's is correct: it destructures mutate (referentially stable) and depends on [mutate], whereas this one depends on [updateSetupStateMutation], which React Query returns fresh every render.
| }); | ||
|
|
||
| const $SetupState = z.object({ | ||
| activeLanguages: z.array(z.string()).optional(), |
There was a problem hiding this comment.
z.array(z.string()) accepts any string, and this is the perimeter — $UpdateSetupStateData is what UpdateSetupStateDto validates the PATCH body against.
PATCH /v1/setup {"activeLanguages": ["klingon"]} is stored as-is. Navbar/Sidebar then compute Object.entries(ALL_LANGUAGES).filter(([key]) => activeLanguages.includes(key)) → {} → Object.keys(...).length > 1 is false → the language toggle vanishes for every user, and the admin settings page shows all three boxes unchecked with no way back through the UI. [] is likewise accepted here and only papered over by a ?.length check in the service.
AGENTS.md: "Strict data validation at the boundary, trusting inside" and "Correctness is structural, not vigilant".
const LANGUAGES = ['en', 'es', 'fr'] as const;
const $Language = z.enum(LANGUAGES);
const $ActiveLanguages = z.array($Language).min(1);Exporting LANGUAGES/$Language also gives ALL_LANGUAGES, the gateway's VALID_LANGUAGES, and #1439's MAIL_LANGUAGE one thing to derive from instead of four independent copies.
| (data: Parameters<typeof updateSetupStateMutation.mutate>[0]) => { | ||
| setSaveState('saving'); | ||
| updateSetupStateMutation.mutate(data, { | ||
| onSettled: () => { |
There was a problem hiding this comment.
onSettled runs on failure too, so a rejected save still shows the green check and "All changes saved". Because useUpdateSetupStateMutation doesn't disable the default error notification, the user gets a red error toast and a green "saved" pill simultaneously — and for the language checkboxes specifically, the box stays flipped even though nothing was stored.
onSuccess for the saved state, onError for a visible failure (and revert the optimistic checkbox).
Same issue in #1441, which rewrites this same block.
| }; | ||
| }); | ||
| }, | ||
| [updateSetupStateMutation] |
There was a problem hiding this comment.
React Query returns a new mutation result object on every render — only mutate/mutateAsync are referentially stable. Depending on [updateSetupStateMutation] means this useCallback re-creates autosave every render, so the memo does nothing, and it becomes a real bug the moment autosave is used in an effect dependency array.
#1441 gets this right in the same file:
// `mutate` is referentially stable across renders, so callbacks that depend on `autosave` stay stable too.
const { mutate } = updateSetupStateMutation;
const autosave = useCallback((data) => { ... }, [mutate]);Worth adopting that version wholesale when these two branches are reconciled.
| : activeLanguages.filter((l) => l !== code); | ||
| if (updated.length > 0) { | ||
| autosave({ activeLanguages: updated }); | ||
| if (!updated.includes(i18n.resolvedLanguage)) { |
There was a problem hiding this comment.
Two problems with switching the language here.
It only moves the admin. Every other user whose persisted language was just deactivated keeps rendering in it — t() still resolves their strings, and the toggle no longer lists it, so they can't switch away either. There's no reconciliation anywhere on app load. Something like this in the _app route (or wherever setupStateQuery is first read) would close it:
if (!activeLanguages.includes(i18n.resolvedLanguage)) i18n.changeLanguage(activeLanguages[0]);updated[0] is arbitrary. [...activeLanguages, code] appends, so the array's order reflects the sequence of toggles, not any canonical order. Deactivating English can drop the admin into Spanish or French depending on click history. Sorting against the canonical LANGUAGES tuple (or just preferring 'en' when present) makes it deterministic.
Minor: the if (updated.length > 0) guard silently does nothing when it fails. The disabled={isLastActive} prop already prevents that path, so the guard is either dead or masking a case where it isn't — worth deciding which.
| // older $BrandingConfig will silently drop newer branding fields on read. | ||
| const branding = $BrandingConfig.nullable().safeParse(savedOptions?.branding ?? null); | ||
| return { | ||
| activeLanguages: savedOptions?.activeLanguages?.length ? savedOptions.activeLanguages : ['en', 'fr'], |
There was a problem hiding this comment.
This is the first of four copies of ['en', 'fr'] in this PR (also Navbar, Sidebar, settings.tsx) and #1439 adds five more. AGENTS.md: "One source of truth; everything derived."
Export DEFAULT_ACTIVE_LANGUAGES from schemas/setup and use it here. Then, since this method already guarantees a non-empty array, $SetupState.activeLanguages can drop .optional() and every client-side ?? ['en', 'fr'] disappears — the clients stop needing to know the default exists.
| createdAt DateTime @default(now()) @db.Date | ||
| updatedAt DateTime @updatedAt @db.Date | ||
| id String @id @default(auto()) @map("_id") @db.ObjectId | ||
| activeLanguages String[] |
There was a problem hiding this comment.
A required scalar list added to a model with existing documents. #1441 adds its SetupState field as Int?; this one has neither ? (not permitted on lists) nor @default([]).
Prisma's MongoDB connector should return [] for a list field absent from a stored document, so this probably works — but "probably" is doing real work in a field that gates the language toggle for the whole instance. @default([]) costs nothing and removes the question for every instance created before this deploys.
Also worth noting initApp creates SetupState without this field, so the very first document an instance writes exercises exactly that path.
| } | ||
| } | ||
|
|
||
| const VALID_LANGUAGES = ['en', 'es', 'fr']; |
There was a problem hiding this comment.
Third hardcoded language list in this PR (after ALL_LANGUAGES and LanguageOptions), and #1439 adds a fourth (MAIL_LANGUAGE). Import the tuple from schemas and derive.
Two behavioural notes:
-
The gateway ignores
activeLanguages.?lang=esrenders Spanish even on an instance where Spanish has been deactivated. Since the emailed assignment links in feat(mail): add outgoing email, group email templates, and assignment emails #1439 are built as${assignment.url}?lang=${language}, that's reachable in practice. -
SSR/hydration.
detectLanguage()readswindow.location.searchat module scope; the guard makes the server always resolve'en'while the client can resolve'es'. Worth confirming the first paint doesn't flash English or trip a hydration mismatch — a?lang=link is precisely the case where the two disagree.
Minor: the two /* eslint-disable */ lines at the top disable those rules for the whole file rather than the declare module block that needs them.
There was a problem hiding this comment.
Do not change the eslint-disable though
| setInstrument( | ||
| translateInstrument( | ||
| instrument, | ||
| resolvedLanguage === 'en' || resolvedLanguage === 'fr' ? resolvedLanguage : 'en' |
There was a problem hiding this comment.
resolvedLanguage === 'en' || resolvedLanguage === 'fr' ? resolvedLanguage : 'en' appears five times in this PR — here, useInstrumentInfoQuery, group/manage.tsx, InteractiveContent, and useInterpretedInstrument. AGENTS.md: "Shape is never repeated."
One named helper says what it means and gives you a single place to change the policy:
/** Instruments are authored in en/fr only; other UI languages fall back to English. */
export const toInstrumentLanguage = (language: Language): 'en' | 'fr' =>
language === 'fr' ? 'fr' : 'en';The policy itself is also worth surfacing: a user who has selected Español gets instruments silently rendered in English with no indication that a translation is missing. That's defensible, but right now it's an implicit consequence of five scattered ternaries rather than a stated decision.
There was a problem hiding this comment.
No. Translate instrument should handle this already downstream. I think it already does
| const instanceDetails = branding?.instanceDetails?.[lang]?.trim() || null; | ||
| /* eslint-disable @typescript-eslint/prefer-nullish-coalescing -- blank strings must fall back, so `||` not `??` */ | ||
| const instanceName = | ||
| (branding?.instanceName as undefined | { [key: string]: string })?.[lang]?.trim() || DEFAULT_INSTANCE_NAME; |
There was a problem hiding this comment.
These casts (four in this file, plus login.tsx and StartSessionForm.tsx) are the symptom, not the fix. AGENTS.md: "no casting at call sites" and "Concentrate the cost: complex type machinery belongs in a small number of utilities."
The root cause is that $BrandingConfig types instanceName/instanceTagline/instanceDetails and resourceLinks[].label as literal { en, fr } objects, so they can't be indexed once Language widens. Keying them by Language in packages/schemas fixes all six sites without touching them:
const $LocalizedString = z.partialRecord($Language, z.string().nullish());That also makes tl = (obj) => obj[lang] ?? obj.en ?? '' unnecessary and — importantly — makes adding a fourth language a compile-time task rather than a runtime fallback to ''. Today the cast means a missing language silently renders as an empty instance name.
Note #1439 adds a $LocalizedString in schemas/mail for the same reason; one shared definition in schemas/core would serve both PRs.
|
|
||
| const handleChangeLanguageEvent = useCallback( | ||
| (event: CustomEvent<Language>) => { | ||
| (event: CustomEvent<string>) => { |
There was a problem hiding this comment.
Widening a react-core event contract from CustomEvent<Language> to CustomEvent<string> to work around a downstream type error loses the checking for every other consumer of the changeLanguage event — including instrument authors, since this is the interactive-task bridge.
The runtime guard on the next line already narrows to 'en' | 'fr', so the type can stay Language and the guard keeps doing its job. If the incompatibility is with document.addEventListener's global CustomEventMap (which is where the widening pressure comes from), that map is the thing to update — not this handler's signature.
While here: console.error on an unsupported language is invisible to the participant, who just sees the language not change. Given AGENTS.md's "fail loudly on an undeclared policy", a user-visible signal (or not offering the option at all) would be better.
There was a problem hiding this comment.
Absolutely, this is not okay
joshunrau
left a comment
joshunrau
left a comment
There was a problem hiding this comment.
Thanks for this. The instance-level activeLanguages idea is the right shape, and the settings page is a sensible place for it.
Two questions on this PR need a maintainer decision — whether an admin-selectable language should ship before its translations exist, and how the new required Prisma field behaves against already-deployed instances. I'll follow up separately on both. The items below stand either way.
Gateway — this is the most serious part
-
apps/gateway/src/services/i18n.ts:19-31—detectLanguage()readswindow.location.searchat module scope, in a file imported bysrc/Root.tsx.apps/gateway/AGENTS.md("SSR traps") forbids exactly this. It also diverges SSR from hydration:renderToStringresolvesenwhile the client resolveses/fr. Read the param server-side in the root router and pass it throughRootPropsinstead. -
i18n.ts:30—?lang=esrenders a gateway page with blank labels and buttons. Passing the detected language asdefaultLanguagedestroys libui's fallback:Translator.tresolvesobj[resolvedLanguage] ?? obj[defaultLanguage], so when both areesand no Spanish string exists it logsFailed to extract translation…and returns''. KeepdefaultLanguageaten. -
apps/gateway/src/Root.tsx:82-88— the gateway'sLanguageTogglestill hard-codes{ en, fr }and nothing there readsactiveLanguages, so the per-instance setting never reaches the patient-facing app at all.
Types and single-source-of-truth
-
packages/schemas/src/setup/setup.ts:180,191—z.array(z.string()).optional()accepts[]and arbitrary language codes. The "at least one language" invariant is enforced only in the settings UI (settings.tsx:171). Make it a non-empty array of a language enum so the perimeter rejects bad input. -
apps/web/src/utils/languages.ts:1— typeALL_LANGUAGESasRecord<Language, string>rather than{ [key: string]: string }, so adding a language toLanguageOptionsfails to compile until it has a display name. -
The
['en', 'fr']default is written four times:apps/api/src/setup/setup.service.ts:54,Navbar.tsx:30,Sidebar.tsx:36,settings.tsx:67. One source of truth. -
resolvedLanguage === 'en' || resolvedLanguage === 'fr' ? resolvedLanguage : 'en'is copy-pasted five times:useInstrument.ts:25,useInstrumentInfoQuery.ts:29,group/manage.tsx:382,InteractiveContent.tsx:184,useInterpretedInstrument.ts:50. Extract one helper mapping a UI language to an instrument language. -
Casting at call sites is banned by
CLAUDE.md—LoginBrandingPanel.tsx:149,237,239,241,auth/login.tsx:40,StartSessionForm.tsx:215,settings.tsx:174. The root cause is$BrandingText(packages/schemas/src/setup/setup.ts:42) being fixed at{ en, fr }; key it byLanguageand the casts disappear rather than needing to be written. -
InteractiveContent.tsx:66widens the handler toCustomEvent<string>whilepackages/react-core/src/types.ts:11still declareschangeLanguage: CustomEvent<Language>. The two now disagree.
The autosave rewrite
-
apps/web/src/routes/_app/admin/settings.tsx:51-63is unrelated to i18n and regresses the page — please split it out. The Save button and success notification are gone;onSettledreports "All changes saved" even when the PATCH failed; and both controls now derive straight from server state with no optimistic update, so each click waits for a PATCH plus a query invalidation before the checkbox visibly moves. -
settings.tsx:18— the deleted comment explaining thatToggleis hand-rolled because libui ships no Switch is exactly the non-obvious kindCLAUDE.mdsays to keep. Please restore it. -
SaveStatus.tsx:12— rawslate/whitescales;apps/web/AGENTS.mdasks for semantic libui tokens (bg-muted,text-muted-foreground).
Tests
- No tests at all —
CLAUDE.mdrequires a unit test and an e2e test per change. There's no e2e page object for/admin/settingsintesting/src/pages/_app/, and the new checkboxes atsettings.tsx:164carry nodata-testid, whichapps/web/AGENTS.mdasks for on new UI an e2e test will drive.
PR description
- The body says
eswas added to runtime-core'sLanguage. It wasn't —packages/runtime-core/src/types/core.ts:2is still'en' | 'fr', which is precisely why the five-way narrowing in item 7 exists. Worth fixing so reviewers aren't looking for a change that isn't there.
Overlap with #1439. The two PRs share seven files and both add activeLanguages to the Prisma schema, setup.ts and setup.service.ts, plus SaveStatus.tsx and languages.ts, with divergent contents. They can't both merge as written — we'll sort out the ordering and let you know which one rebases onto the other.
|
Following up on the overlap with #1439 — that's now settled in this PR's favour. This PR owns the shared files. That makes the review items I left earlier the whole of what's outstanding, with the gateway ones (1–3) being the ones I'd start on — Still with the maintainer: whether Español ships before its translations exist, and whether this waits on the libui release. I'll follow up. If the answer on Spanish is to gate it behind real translations, this PR gets smaller rather than larger — so nothing you'd do now is at risk from that decision. |
|
Both remaining questions on this PR are now settled. Español ships. Add it as a selectable language even though the translations don't exist yet — users who pick it get an English interface via the fallback, and that's accepted. Translators can then work against a real setting. libui #107 is merged and released as 6.10.0. Please bump the catalog pin in One item is now more urgent than when I first wrote it. Since Español becomes genuinely selectable, review item 2 is no longer theoretical: Items 1 and 3 on the gateway matter for the same reason — the SSR/hydration split, and the fact that the gateway's Everything else from my review stands unchanged. |
|
Correcting my earlier guidance on the gateway — I said "keep
The reason it looked like the right knob is that libui's init({ defaultLanguage, translations }: TranslatorConfig) {
this.#config = {
defaultLanguage: defaultLanguage ?? 'en', // ← the fallback used by t()
...
};
this.changeLanguage(this.#config.defaultLanguage); // ← also seeds resolvedLanguage
}You wanted the second effect — start the session in the detected language — and got the first as collateral. Use i18n.init({ translations: {} }); // defaultLanguage stays 'en' — don't pass it
i18n.changeLanguage(detectedLanguage); // start in the detected language, fallback intactThat also composes better with the SSR fix in item 1: read Worth noting |
|
Suggested model: Fable 5 Sizing the remaining work on this PR for whoever picks it up, human or agent. It's an automatic escalation on |
joshunrau
left a comment
joshunrau
left a comment
There was a problem hiding this comment.
Total rewrite required to meet the standards. Tell Opus 5 Max Thinking to revise the entire PR in accordance with the standards and documentation now in the app (see all the new AGENTS.md files)
| createdAt DateTime @default(now()) @db.Date | ||
| updatedAt DateTime @updatedAt @db.Date | ||
| id String @id @default(auto()) @map("_id") @db.ObjectId | ||
| activeLanguages String[] |
| // older $BrandingConfig will silently drop newer branding fields on read. | ||
| const branding = $BrandingConfig.nullable().safeParse(savedOptions?.branding ?? null); | ||
| return { | ||
| activeLanguages: savedOptions?.activeLanguages?.length ? savedOptions.activeLanguages : ['en', 'fr'], |
| } | ||
| } | ||
|
|
||
| const VALID_LANGUAGES = ['en', 'es', 'fr']; |
| } | ||
| } | ||
|
|
||
| const VALID_LANGUAGES = ['en', 'es', 'fr']; |
There was a problem hiding this comment.
Do not change the eslint-disable though
| boldResourceLinks: boolean; | ||
| fontSize: null | number | undefined; | ||
| lang: 'en' | 'fr'; | ||
| lang: string; |
There was a problem hiding this comment.
Unacceptable. We do not use magic strings in this codebase. Language needs to be adjusted and used here.
| setInstrument( | ||
| translateInstrument( | ||
| instrument, | ||
| resolvedLanguage === 'en' || resolvedLanguage === 'fr' ? resolvedLanguage : 'en' |
There was a problem hiding this comment.
No. Translate instrument should handle this already downstream. I think it already does
| }); | ||
| const infos = await $InstrumentInfo.array().parseAsync(response.data); | ||
| return infos.map((instrument) => translateInstrumentInfo(instrument, resolvedLanguage ?? 'en')); | ||
| const instrumentLang = resolvedLanguage === 'en' || resolvedLanguage === 'fr' ? resolvedLanguage : 'en'; |
There was a problem hiding this comment.
this should be handled downstream
| (data: Parameters<typeof updateSetupStateMutation.mutate>[0]) => { | ||
| setSaveState('saving'); | ||
| updateSetupStateMutation.mutate(data, { | ||
| onSettled: () => { |
|
|
||
| const handleChangeLanguageEvent = useCallback( | ||
| (event: CustomEvent<Language>) => { | ||
| (event: CustomEvent<string>) => { |
There was a problem hiding this comment.
Absolutely, this is not okay
| {ALL_LANGUAGES[language][resolvedLanguage]} | ||
| { | ||
| ALL_LANGUAGES[language][ | ||
| resolvedLanguage === 'en' || resolvedLanguage === 'fr' ? resolvedLanguage : 'en' |
joshunrau
left a comment
joshunrau
left a comment
There was a problem hiding this comment.
This branch now conflicts with main — GitHub reports the merge as CONFLICTING, and the CI lint-and-test job has not run against this commit (only the CodeQL jobs did).
Please rebase or merge main into the branch and resolve the conflicts, so that CI can run and the merge result can be reviewed. Review resumes automatically once the branch is mergeable and the checks are green.
Reviewed at commit 094c92b.
Implements the maintainer's settled decisions and the standing items from
both review rounds. Net -553 lines.
SMTP only (decision 1, removal half). Drops the four-provider HTTP client,
the hand-rolled AWS SigV4 signer, `apiKey`, and the provider/region/domain
fields. Every provider implemented here exposes SMTP, so nodemailer already
reached them. Consuming libnest's lazy transporter is left until it ships.
No more emailed password (decision 2, partial). `{{password}}` is gone from
the default templates and the welcome email no longer carries a credential.
The invite-token flow is a follow-up.
Drops the files #1442 owns (decision 4): `activeLanguages`, `SaveStatus`,
`languages.ts` — and `SegmentedControl`, which nothing used once the
transport switch and category tabs went.
Correctness:
- `isMailEnabled` is one function over the raw stored value, so the public
flag and the server's behaviour cannot disagree.
- `POST /v1/mail/test` no longer authenticates to a caller-supplied host
with the stored password; changing the server requires re-entering it
rather than silently blanking a working credential.
- `emailTemplates` updates carry `expectedUpdatedAt`, which constrains the
write itself, so a concurrent edit conflicts instead of vanishing.
- `AssignmentEmailForm` resolves templates from the assignment's own group,
matching the server, and clamps the language to what is on offer.
- Mail settings save explicitly; autosave was persisting half-typed secrets
and reporting failures as success.
- Assignment emails are audit-logged and rate-limited; the expiry renders
with a time and zone instead of a bare UTC date.
- Only `NotFoundException` is swallowed when resolving group names.
- `$LocalizedString` moves to `core` and derives from `Language`.
- `remote-assignment.tsx` keeps the libui `Form` (reverted; unrelated).
- The `INFORMATION` template category is removed — nothing sent one.
Structure: `SectionCard`, `FormField` and `EmailTemplateEditor` extracted;
`useGroupQuery` replaces two inline fetches; `useUpdateGroupMutation` gets
its success toast back behind an option.
Tests: schemas coverage for the mail contract, web tests for the hook and
the two form bugs, and e2e specs plus page objects for `/admin/mail` and
`/group/email-templates`.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Reconciles the SMTP-only refactor with the two mail fixes pushed to the branch and with everything main gained since (#1441, #1444, #1477). The branch had patched the HTTP transport rather than removed it, so the conflicts were resolved toward the maintainer's decision 1: the HTTP client and SigV4 signer go, which makes both patches unnecessary rather than wrong. `isMailEnabled` no longer needs an HTTP branch, and the provider names come back out of `eslint.config.js` since no JSX mentions them any more. Kept from the other side: - #1441's `getDefaultAssignmentExpiry` replaces the hardcoded one-year default in the create-assignment form. - #1444's `$Email`/`$PhoneNumber`/`omittedIfBlank` validation helpers in the user-create form, replacing the old `PHONE_REGEX`. - The deduplicated welcome-email notification in `useCreateUserMutation`, now also opted out of the router error boundary so the call site's own password-error handling is the only path. - #1477's page objects and specs, unioned with the two added here. `SaveStatus.tsx` is restored from main: #1442 has landed and `admin/settings` now consumes it, so decision 4's "pick it back up once #1442 lands" applies. `activeLanguages` and `utils/languages.ts` are still absent from main, so those stay dropped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Spanish reached the frontends only through libui's type, while this repo kept `Language` at en|fr, so every site that indexed localized content needed a cast or a narrowing ternary. Separate the two concepts instead. `schemas/core` now owns the interface language (en|es|fr) and `runtime-core` keeps the instrument-authoring language (en|fr), with `$InstrumentAuthoringLanguage` naming the narrower set. react-core derives libui's `LanguageOptions` from the contract, so enabling a language is one edit rather than one per app. Widening `translateInstrument` to accept an interface language removes the need to narrow at any call site: `getTargetLanguage` already falls back to the language an instrument was written in. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adds activeLanguages to the setup state, validated at the perimeter as a non-empty tuple of interface languages, with an admin card to toggle them. The set is rebuilt by filtering the canonical language order rather than appending, so the fallback language does not depend on click order. Deactivating a language previously stranded everyone already reading in it, so the shared LanguageToggle reconciles on load and hides itself when only one language is active. The gateway has its own database and cannot read the setup state, so the languages travel with the assignment. `?lang=` is read server-side and applied with `changeLanguage`, never `defaultLanguage` — that is the fallback `t()` uses for an untranslated string, and pointing it at the patient's language renders those strings as nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Unit tests for the perimeter ($ActiveLanguages rejecting an empty or unknown set), the SetupService round-trip and its fallback for a document written before the setting existed, and the toggle's reconciliation of a user left on a language the instance no longer offers. The e2e test drives the admin card and asserts the sidebar toggle disappears, restoring the language it deactivated since the setup state is instance-wide. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
094c92b to
8eb257c
Compare
| createdAt DateTime @default(now()) @db.Date | ||
| updatedAt DateTime @updatedAt @db.Date | ||
| id String @id @default(auto()) @map("_id") @db.ObjectId | ||
| activeLanguages String[] @default([]) |
There was a problem hiding this comment.
would suggest adding a Language enum here
| createdAt DateTime @default(now()) @db.Date | ||
| updatedAt DateTime @updatedAt @db.Date | ||
| id String @id @default(auto()) @map("_id") @db.ObjectId | ||
| activeLanguages String[] @default([]) |
| // Parsed rather than read through: the column is a bare `String[]` and predates this setting, | ||
| // so a document written by an older release holds `[]` and every client would be left with no | ||
| // language to offer. | ||
| const activeLanguages = $ActiveLanguages.safeParse(savedOptions?.activeLanguages); |
There was a problem hiding this comment.
once enum is added at the db level, this can be removed
| id String @id | ||
| // SQLite has no scalar list, so this is JSON, read back through $ActiveLanguages. The default | ||
| // covers assignments created before an instance could choose which languages to offer. | ||
| activeLanguagesStringified String @default("[\"en\",\"fr\"]") |
There was a problem hiding this comment.
Wrong. We can use JSON type in sqlite now with the same generator as in api
|
|
||
| // Read server-side rather than from `window.location` so the SSR pass and the hydration pass | ||
| // resolve the same language; anything else renders the page in English and then swaps it. | ||
| const activeLanguages = $ActiveLanguages.safeParse(JSON.parse(assignment.activeLanguagesStringified)); |
There was a problem hiding this comment.
can be removed once db is done properly
| (data: Parameters<typeof updateSetupStateMutation.mutate>[0]) => { | ||
| setSaveState('saving'); | ||
| updateSetupStateMutation.mutate(data, { | ||
| onSettled: () => { |
Addresses the review on #1442. `activeLanguages` on `SetupState` is a Prisma `Language` enum rather than `String[]`, so an unknown code cannot be stored and `getState` no longer parses what it reads back. Its `@default` is `[en, fr]` — the same set `DEFAULT_ACTIVE_LANGUAGES` names, so an instance upgrading onto this keeps the toggle it has today rather than silently dropping French. The gateway stores the languages an assignment offers as a `Json` column typed through `prisma-json-types-generator`, the generator apps/api already uses. SQLite supports `Json` as of the pinned Prisma, so the stringify / JSON.parse / re-validate round trip in the router is gone and the column reads back as the non-empty tuple directly. A rejected autosave on /admin/settings reported "All changes saved": `onSettled` runs on failure too. It reports the failure instead, and the mutation opts out of the app-wide `throwOnError` so the status is reachable rather than replaced by the router error boundary. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Pushed as 2a9894a. All six comments addressed; one of them I answered differently than suggested, flagged below.
|
Deactivating the language a user is reading left the sidebar in it. The reconciliation added in #1442 runs from `useLanguageOptions`, inside `LanguageToggle`. It corrects `i18n.resolvedLanguage` and emits `languageChange` — but libui's `useTranslation` subscribes in an effect, and React runs effects child-first, so a tree that mounts already stranded fires the correction before the toggle's ancestors have subscribed. They never hear it and keep rendering the deactivated language. The sidebar is the casualty precisely because it renders the toggle. `Layout` mounts the navbar first, so the navbar subscribes in time and updates while the sidebar does not — which is why the symptom names one and not the other. Fixes it where the ordering cannot matter: `_app`'s `beforeLoad` reconciles before any component renders, so every `useTranslation` initialises from the corrected language instead of waiting to be told. The policy itself moves to `resolveActiveLanguage` in schemas/core, next to the authoring-language policy, so the pre-render pass and the live in-session effect decide the same thing rather than each carrying a copy. `useLanguageOptions` keeps its effect: it covers an admin deactivating a language mid-session, where every component is already subscribed and it demonstrably works. Its doc now says which case it can and cannot cover. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
#1442 made Español selectable and translated the namespace JSON files, but left 572 inline `t()` calls naming only `en` and `fr`. libui resolves `obj[resolvedLanguage] ?? obj.en`, so those rendered in English with nothing reporting it — the admin panel and its submenu among them. Adds the missing `es` entry at all 572 sites across apps/web, packages/react-core and apps/gateway, reusing the terminology the completed namespace files already established (Panel de control, Centro de datos, Sujeto, Tarea remota) rather than inventing a second vocabulary. Five strings that carry no words — the `{}: {}` format strings whose `fr` exists only for French's space-before-colon — get an entry identical to English, so the set is complete rather than partly deliberate and partly forgotten. A unit test scans all three frontends and fails on an inline call that names `en` but not `es`, which is what keeps this from decaying; the four AGENTS.md files that taught the `{ en, fr }` shape now teach `{ en, es, fr }` and point at it. apps/playground is untouched: it hard-codes its LanguageToggle options as { en, fr }, so Spanish cannot be selected there at all. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
3 participants
Summary
Rewritten from scratch against current
mainper review. Adds Spanish as a selectable interface language, and lets each instance choose which languages it offers.The underlying defect
Spanish reached the frontends only through libui's
Languagetype, while this repo kept its ownLanguageaten | fr. Two types that had to agree didn't, and the seam was papered over withascasts at 8 call sites and aresolvedLanguage === 'en' || === 'fr'ternary repeated 5 times.This separates the two concepts rather than casting between them:
schemas/coreowns the interface language (en | es | fr) — what a user reads the application in.runtime-corekeeps the instrument-authoring language (en | fr) — what clinical content exists in.$InstrumentAuthoringLanguagenames the narrower set, andtoInstrumentAuthoringLanguageis the one place the policy for content typed in a third language lives.packages/react-corederives libui'sLanguageOptionsfrom the contract (extends Record<Language, true>), so enabling a language is one edit, not one per app.Widening
translateInstrumentto accept an interface language removed the need to narrow at any call site:getTargetLanguagealready falls back to the language an instrument was written in. All 5 ternaries and theCustomEvent<string>widening deleted themselves.$LocalizedStringand$BrandingTextare now keyed byLanguagewith asatisfiesguard, so all 8 casts went with them.Per-instance language selection
activeLanguagesonSetupState, validated at the perimeter asz.tuple([$Language], $Language)— a tuple rather than.nonempty()so the non-empty guarantee reaches the type and consumers readactiveLanguages[0]with no assertion.DEFAULT_ACTIVE_LANGUAGES, and a Prisma@default([])so documents predating the field are covered.LanguageToggleinreact-coreoffering the active set, hidden when only one is active, that reconciles a user stranded on a deactivated language on load rather than only moving the admin who flipped it.Gateway
The gateway has its own SQLite database and cannot read the setup state, so the languages travel with the assignment as a snapshot taken at creation.
?lang=is read server-side in the root router and passed throughRootProps, so SSR and hydration resolve the same language — no module-scopewindowaccess.defaultLanguageis never passed toi18n.init. It is the fallbackt()uses for a string with no entry in the resolved language, so pointing it at the patient's language would have rendered every untranslated string as''. The entry points callchangeLanguageinstead.Also
apps/webnamespace files (271 strings, complete).idValidationRegexErrorMessageand the PrismaErrorMessagetype converged on$LocalizedString.react-core, serving the toggle, the email-template selector and the instrument dropdown.Test plan
pnpm lint— 33/33 workspaces cleanpnpm test— 642 passed, 1 skipped (29 new)pnpm test:e2e— could not run locally (ports held by another process); needs CIUnit tests cover the perimeter rejecting empty and unknown language sets, the
SetupServiceround-trip and its fallback for a pre-existing document, and the toggle's reconciliation. The e2e test drives the admin card and asserts the sidebar toggle disappears.Notes for review
esis offered with complete translations for the namespace JSON files, but 229 inlinet({ en, fr })calls inapps/weband 15 inreact-corehave no Spanish entry and will fall back to English. Happy to do those in this PR or a follow-up — flagging rather than silently shipping a half-translated interface.🤖 Generated with Claude Code