Skip to content

feat(i18n): internationalization across the API and both React apps - #1344

Open
marcelo-maciel wants to merge 76 commits into
fullstackhero:mainfrom
marcelo-maciel:feat/i18n
Open

feat(i18n): internationalization across the API and both React apps#1344
marcelo-maciel wants to merge 76 commits into
fullstackhero:mainfrom
marcelo-maciel:feat/i18n

Conversation

@marcelo-maciel

@marcelo-maciel marcelo-maciel commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Implements internationalization (i18n) with multi-language support across the backend API and both React front-ends, following up on the discussion in #1301 (branched off v10 GA as suggested).

On the split request. Everything below is the review being closed on this branch first, so the three PRs are cut from a tree that is already correct rather than each carrying its own version of these fixes. Several of them do not respect the split boundary — the impersonation locale fix spans both apps, and the pt-BR catalog convention spans the framework resx and both front-end catalogs — so landing them once here is strictly less work to review than landing them three times. Say the word and the framework PR goes up next.

⚠️ Touches protected src/BuildingBlocks (Golden Rule #4)

The previous description omitted this, which was wrong. Eighteen files, needing maintainer sign-off:

CoreCore.csproj; Exceptions/ (CustomException, ForbiddenException, UnauthorizedException, and the new ILocalizableMessage, LocalizedKeyNotFoundException, LocalizedUnauthorizedAccessException); Localization/ (new SharedResources marker + SupportedCultures + the two shared catalogs).

WebExtensions.cs (registers and orders the localization middleware, +6 lines); Exceptions/GlobalExceptionHandler.cs; Validation/PagedQueryValidator.cs; new Localization/ (LocalizationExtensions, UserLocaleRequestCultureProvider).

JobsExtensions.cs, one exception message. StorageQuotaMeteredStorageService.cs, one exception message.

No existing behaviour of other building blocks is altered. src/Directory.Packages.props is not touched: the pin this branch carried is already on main, so after the merge the file has a zero-line diff.

UseRequestLocalization sets the UI culture only

You asked whether UI-culture-only was considered. It is now what ships.

main has no request localization at all, so pinning the formatting culture is less change than negotiating it: CultureInfo.CurrentCulture behaves exactly as it does on main today, and only resource lookup follows the request. For an API whose output is JSON that is the safer default, and it makes the CA1305 question moot rather than merely bounded.

It is not one switch. RequestLocalizationMiddleware.SetCurrentThreadCulture assigns both cultures unconditionally, so the culture half has to be pinned:

  • DefaultRequestCulture carries (InvariantCulture, configured default). The middleware resolves the culture half as cultureInfo ??= DefaultRequestCulture.Culture, making invariant the only reachable value.
  • SupportedCultures is null, so the middleware skips culture filtering entirely. A one-element [InvariantCulture] list behaves identically but logs UnsupportedCultures on every request — the middleware's parent-culture walk bails at the empty culture name, so invariant is unmatchable by design.

With formatting out of the negotiation, the neutral pt/en entries bought nothing and are gone; SupportedCultures.Tags is the single, specific-only list. A request asking for a bare pt or an unsupported variant now resolves to the configured default. Both React apps canonicalise variants onto supported tags before calling the API, so app traffic is unaffected; a hand-rolled client sending bare pt gets the default.

Message arguments are culture-insensitive too. The localizer formats with string.Format under CurrentCulture, so a double or DateTime in a message would render with an invariant separator. Every MessageArgs site and every localizer["…", …] call site was enumerated: all int, long, string or enum, except MaxWindow.TotalDays in the two audit-window validators, which is now an int at the source.

Catalogs are named for specific cultures

SharedResources.pt.resx and the ten module catalogs are now *.pt-BR.resx, matching the front-end. Pure renames, no string changed.

The asymmetry you flagged is gone, and so is the trap behind it: a future pt-PT is no longer served Brazilian strings by parent fallback. The documented consequence is that a bare pt or an unsupported variant lands on the neutral English catalog rather than on Portuguese. Adding a language is now: add the specific tag to SupportedCultures.Tags, add a *.{tag}.resx per catalog, add the JSON catalogs to both apps, and drop it from the front-end CANON map if it was being folded into another tag. .agents/rules/localization.md records all of this.

The Locale column

The old summary was wrong: there is no DB default. The column is nullable with en-US as a code-level fallback, and it is now character varying(10) rather than unbounded text — 10 covers language-script-region (zh-Hant-TW). AddUserLocale was edited in place rather than stacked with an ALTER, since it has never shipped in a release.

Confirmed as you asked: Validation.UnsupportedLocale is wired at the write boundary. UpdateUserCommandValidator restricts Locale to SupportedCultures.Tags, on PUT /identity/profile, via the Mediator ValidationBehavior. The column constraint is the storage-level backstop, not the validation.

Known behaviour (documented, not bugs)

  • The locale claim lags a language switch by one token. The provider reads the JWT claim, so a switch reaches the API at the next token issue. The front-end persists to the profile and re-mints, so it converges; in between, the shell can be in the new language while an API error is still in the old one. The alternative is a per-request DB read on every authenticated call.
  • SignalR does not carry the app locale. The hub client builds its own requests instead of going through apiFetch, so Accept-Language on the negotiate is the browser's. Applies to every session, not just impersonation. Named explicitly in handoff-locale.spec.ts so any other channel that stops carrying the locale fails the test.

Fixed since the last review

Three reviewer bots' P2s, plus what a five-lens adversarial pass turned up. Each was verified at the source before being treated as real, and each fix has a mutation gate — the fix is reverted, the pinning test must go red, and the file is restored byte-exact.

  • The operator's language now survives cross-app impersonation. StartImpersonation strips the target's locale claim on purpose, and the two apps normally sit on different origins, so the handoff URL had no way to convey it and the API fell through to the dashboard's own browser detection. The handoff now carries locale; the dashboard adopts it before createRoot, which fixes the shell and Accept-Language at once. Chosen over an actor-locale token claim: it leaves the token contract and the framework culture provider untouched.
  • Unmapped status codes are no longer titled "an unexpected error occurred". TitleKeyFor sent everything outside four statuses to Error.Unexpected; the type-name fallback beside it only fires on ResourceNotFound, and that key resolves, so it never fired. Every one of the 41 Conflict throw sites across Billing and Catalog answered Status: 409 with a title contradicting its own detail. This branch regressed that — before it, Title was the exception type name. Unmapped statuses now fall back to the type name again, and Conflict gets a real localized title.
  • A stale persisted locale no longer reverts the chosen language. updateMyProfile is a read-modify-write with no concurrency token, and Settings › Profile invalidates the same ["identity","me"] key the topbar reads, so a save whose read preceded the language PUT could echo the old locale back and win. The topbar's hydration effect then changed the UI language to it. Hydration now stops once the user chooses a language in-session; a locale set on another device still carries over on a fresh mount.
  • The document element declares the active locale. Both apps shipped a static lang="en" that nothing updated, so a Portuguese UI announced itself as English to screen readers and browser translation.
  • Test gaps the audit found. A built-in-validation test asserted only the absence of the English string (satisfied by a blank message or a leaked resource key) and now pins the Portuguese text. ExceptionSeverityClassifier was never exercised with the Localized* subclasses, even though those subclass the BCL types precisely to keep audit severity classification working — changing a base type would have silently reclassified every unauthorized access with the suite green.
  • Catalog parity is now enforced generically. Notifications shipped a catalog with no parity test and has no test project to hold one. CatalogParityTests discovers every catalog by reflection and compares each culture's own key set with tryParents: false, plus the placeholder-index set per key — {1} present in one culture and not the other throws FormatException at render time, in that culture only. New module catalogs are covered without a new test.

Known: CI is red on a pre-existing dependency advisory

NU1903 / GHSA-q939-rpr3-3284 on SSH.NET 2025.1.0, pulled transitively by Testcontainers. It fails restore for the whole solution under TreatWarningsAsErrors, on main too — dotnet restore src/FSH.Starter.slnx at 3f2959e6 fails identically. Not introduced here and not fixed here; the pin lives in #1333. Verified locally with the audit disabled on the command line only, never committed.

Testing

  • Backend: 15 test assemblies, 1891 passed / 0 failed / 1 skipped, including Integration (Testcontainers/Postgres, Docker) at 746 passed / 1 skipped.
  • Front-end: build, test-suite typecheck, lint and the full Playwright suite green in both apps — admin 135 passed, dashboard 183 passed, no flaky, none skipped.
  • Every fix carries a mutation gate with a byte-exact restore.

Not pinned, and stated rather than glossed: the hydration guard has no regression test. Reproducing it needs an in-mount profile refetch driven through the Settings form, and the click races the language-change re-render (element detached from the DOM). Three distinct approaches, then stopped rather than paper over it with retries or a longer timeout.

Notes

  • en-US and pt-BR are held at strict key and placeholder parity, enforced by tests, so a missing or mis-arged translation fails the build instead of shipping English.
  • Documentation and a changelog entry land with the framework PR (Golden Rule Localization with JSON files #10), since code on ProblemDetails and the new config section are public contract.

Add SupportedCultures constant (Default/Tags/RequestMatch) in
BuildingBlocks/Core/Localization and reject unsupported locale tags in
UpdateUserCommandValidator; null/empty locale still passes.
Inject IStringLocalizer<SharedResources> and swap hardcoded ProblemDetails
titles (Validation/Unauthorized/NotFound/BadRequest/Unexpected + the 500
Detail) for resx keys. Exception-supplied Detail messages stay raw.

Docker-free handler-level tests exercise the localized 404 and 500 branches
under pt-BR and en-US.
Inject IStringLocalizer<SharedResources> into UpdateUserCommandValidator and
resolve the three custom WithMessage literals lazily (Func overload, so the
lookup runs per validation under the request culture, not at construction).
Add the keys to both resx catalogs.

Built-in FluentValidation messages localize automatically via CurrentUICulture
(FV ships a pt catalog) — no LanguageManager wiring needed. Adds a resx
key-parity test (neutral vs .pt) and updates the Task 2 validator test to
supply a localizer.
Add a Language section to the profile dropdown, mirroring the Theme
section: one item per SUPPORTED locale with an active-locale check.

Selecting a locale calls i18n.changeLanguage and persists it via a new
updateMyProfile mutation (PUT /identity/profile). The locale travels
through the mutate argument (frontend rule fullstackhero#9). Current name/phone are
echoed to avoid the backend wiping FirstName/LastName on a locale-only
save. onSuccess triggers a best-effort token refresh so the new locale
JWT claim is minted.
Mirror the admin Task 10 switcher on the tenant dashboard: a Language
section in the profile dropdown that switches the UI locale in place,
persists it, and re-mints the JWT locale claim.

- topbar: LanguageMenuItem + Language section (preventDefault keeps the
  menu open so the section label re-localizes visibly); onSelectLanguage
  calls i18n.changeLanguage, persists via updateMyProfile (locale by
  mutate arg), and refreshes the token best-effort onSuccess. Profile
  query is not invalidated so a refetch cannot revert the switch.
- api/identity: UpdateProfileInput gains locale; the PUT body echoes it
  alongside the profile-read name/phone so a locale-only save cannot wipe
  FirstName/LastName (backend sets them unconditionally).
- test: switcher spec asserts PUT {locale: pt-BR} with names preserved,
  in-place localization, and the token refresh firing.
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

Both Playwright configs raise actionTimeout to 10s and navigationTimeout to 15s
but leave `expect` at its 5s default, so the assertion every test ends on had the
tightest deadline in the suite. Under CPU contention the first paint of a lazy
route lands past 5s while staying well inside the budgets the config already
chose, and the run fails on toBeVisible.

Measured on the dashboard suite at 16 workers, same machine, three runs:

  no expect timeout   13 failures, mostly `Timeout: 5000ms` on toBeVisible
  expect 10s           5 failures, none of them an assertion
  reverted            17 failures, 14 of them assertions

CI runs 2 workers with 2 retries, which is why this stayed hidden there: the
retry absorbs it and the job still reports green.

What remains under deliberate oversubscription is action and navigation
timeouts, i.e. a saturated machine rather than a budget mismatch. Not chased
further — inflating those would hide real regressions.

@iammukeshm iammukeshm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The engineering here is strong and I want to be explicit about that before I ask for anything, because what I'm asking for is not a criticism of the work.

Several decisions in this are better than what I'd have specified:

  • MessageKey / MessageArgs / ResourceSource on CustomException, with Message staying English. Localising the response body while keeping logs culture-independent is the correct split, and getting it right at the exception type means call sites don't each have to remember it.
  • Surfacing MessageKey as a code extension on ProblemDetails. This is the detail that makes the whole thing usable. Localised detail is prose that changes with Accept-Language; without a stable key, every client that needs to branch on a specific error ends up matching translated text. Adding code turns a localisation change into an API improvement. The tests pinning "unknown key still travels as the code" and "no key → no code property at all" show the contract was thought through rather than stumbled into.
  • ILocalizableMessage subclasses that preserve UnauthorizedAccessException / KeyNotFoundException as base types because audit severity classification keys off the runtime type. That's a non-obvious coupling and most contributors would have swapped the types and quietly broken audit classification.
  • Scoping LogContext.PushProperty in using blocks. That fixes a pre-existing AsyncLocal leak that was contaminating every subsequent log entry in the request. It's unrelated to i18n and it's a genuine bug fix.
  • Dropping CookieRequestCultureProvider by type rather than by index, with the comment explaining that a framework reshuffle of the defaults would otherwise remove the wrong provider.

What I'm asking for: split this into three PRs

519 files and ~20k lines, touching BuildingBlocks/Core, Web, Jobs and Storage, every module, both React apps, and an Identity migration — landing weeks after the 10.0.0 GA.

I can't review this to the standard the change deserves, and neither can anyone else. The parts I've examined closely are good; that's precisely the problem, because it means the risk sits in the parts nobody will read carefully. A 22-resx, 519-file diff gets skimmed, and skimming is how a wrong translation on an authorization message or a subtly changed exception type gets in.

Please split:

  1. FrameworkBuildingBlocks localization, SharedResources, the exception plumbing, GlobalExceptionHandler, the culture-provider chain, the User.Locale column and migration. This is the one that needs real scrutiny and it's small enough to get it.
  2. clients/admin — react-i18next wiring, switcher, catalogs, parity tests.
  3. clients/dashboard — same.

(1) is the hard review and unblocks the other two. (2) and (3) are then mostly mechanical and can be reviewed quickly and in parallel. Same total work for you, dramatically better review for the repo. I'll prioritise them as they arrive so this doesn't stall.

Issues to carry into the split

Golden Rule #4 — undeclared BuildingBlocks changes. This touches Core (exceptions, localization, csproj), Web (Extensions.cs, GlobalExceptionHandler, validation), Jobs and Storage. The description doesn't mention it. Protected code needs explicit sign-off and the description has to say so plainly — see #1323 and #1334 for the form.

UseRequestLocalization sets CurrentCulture, not just CurrentUICulture. Every ToString(), Parse and interpolation in the request pipeline shifts to the negotiated culture. In a codebase not written culture-aware, that's how you get 1,5 in a JSON number or a date that round-trips wrong.

I checked before raising it: AnalysisMode=AllEnabledByDefault with CodeAnalysisTreatWarningsAsErrors=true means CA1305 is enforced, so first-party code already passes explicit format providers — the exposure is much smaller than it first looks. Residual risk is third-party libraries and any [SuppressMessage]. Please state this explicitly in the framework PR rather than leaving it implicit, and say whether you considered setting only CurrentUICulture and leaving CurrentCulture invariant. For an API whose output is JSON, UI-culture-only is arguably the safer default, with formatting handled at the presentation layer where both React apps are already doing it.

The Locale column doesn't match its description. The summary says "a new nullable User.Locale column (default en-US)" but the migration is type: "text", nullable: true with no default — the en-US is a code-level fallback. That's a fine design, but say what it is. Also constrain the column: a BCP-47 tag is varchar(10), not unbounded text. Validation should reject anything outside SupportedCultures.Tags at the write boundary (I see Validation.UnsupportedLocale exists — confirm it's wired to the profile update).

SharedResources.pt.resx is neutral pt; the front-end catalogs are pt-BR. Resource fallback makes this work today (pt-BR → parent pt), and using the neutral is defensible. But the asymmetry with the front-end will confuse the next contributor, and adding pt-PT later silently serves Brazilian strings. Pick one convention and document the fallback intent either way.

RequestMatch includes bare pt and en. Combined with CurrentCulture being set, a request can resolve to a neutral culture, whose formatting comes from a representative culture rather than a specified one. Harmless in practice, but if you go UI-culture-only above, it stops mattering at all.

JWT-claim-carried locale. A language switch doesn't reach the API until the next token issue. The front-end persists to the profile so it converges, but there's a window where the UI is in one language and API errors come back in another. That's an acceptable trade for not adding a per-request DB read — it just needs to be documented as known behaviour rather than discovered.

Docs (Golden Rule #10). Noted that these are prepared as a companion docs PR; they should land with the framework PR, since code on ProblemDetails and the new config section are both public contract.

On #1301

Branching this off v10 GA as discussed was the right call. The foundation is one I'm happy to build on — I'd just like to merge it in pieces I can actually stand behind. Please open the framework PR first and I'll turn it around quickly.

Brings in the 24 commits since the branch point (outbox redesign, wallet
currency match, SQLite/crypto package pins).

Conflict: src/Directory.Packages.props. main added the same
System.Security.Cryptography.Xml 10.0.10 pin this branch carried, with a
more accurate comment (five advisories, not four, and the real transitive
source). Took main's block verbatim, so the branch no longer changes the
central package file at all.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 304e8f777b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

HttpStatusCode.Unauthorized => "Error.Unauthorized",
HttpStatusCode.Forbidden => "Error.Forbidden",
HttpStatusCode.BadRequest => "Error.BadRequest",
_ => "Error.Unexpected",

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Map non-standard custom statuses before falling back

When a CustomException uses statuses that are already common in this repo, such as HttpStatusCode.Conflict for duplicate brands/categories/products or InsufficientStorage for quota failures, this default makes the response title Error.Unexpected even though the HTTP status and detail describe an expected business error. That regresses these localized ProblemDetails from status-specific/type-specific errors into an “unexpected error” banner; please add mappings for the business statuses in use or fall back to the exception type/title instead of Error.Unexpected. .agents/rules/localization.mdL36-L36

Useful? React with 👍 / 👎.

…rals

The backend catalogs were neutral `*.pt.resx` holding Brazilian Portuguese while
the front-end catalogs are `pt-BR`. Resource fallback made that work (pt-BR ->
parent pt), but it left two problems: the asymmetry with the front-end is a trap
for the next contributor, and a future pt-PT would have been served Brazilian
strings silently by the same parent fallback.

Rename all eleven catalogs to `*.pt-BR.resx` so every catalog is named for the
specific culture it actually contains. A bare `pt`, or an unsupported variant
like pt-PT, now walks its parent chain, finds no catalog and lands on the neutral
English one instead of being handed Portuguese it was never translated into.
SharedResourcesLocalizationTests pins that consequence directly.

Adding a language is now: add its specific tag to SupportedCultures.Tags plus a
`*.{tag}.resx` per catalog. Pure rename, no string changed (all eleven are R100).
UseRequestLocalization was configured with both AddSupportedCultures and
AddSupportedUICultures, so every negotiated request also moved
CultureInfo.CurrentCulture. In a codebase that is not written culture-aware that
is how a JSON number arrives as "1,5" or a date round-trips wrong. Localizing the
response body is the goal; shifting formatting for the whole request is not.

Note that main has no request localization at all, so pinning the formatting
culture is LESS change than negotiating it: CurrentCulture now behaves exactly as
it does today on main, and only resource lookup follows the request.

Getting there is not a single switch. RequestLocalizationMiddleware's
SetCurrentThreadCulture assigns both CurrentCulture and CurrentUICulture
unconditionally, so the culture half has to be pinned rather than left alone:

  - DefaultRequestCulture now carries (InvariantCulture, configured default). The
    middleware resolves the culture half as `cultureInfo ??=
    DefaultRequestCulture.Culture`, which makes invariant the only reachable
    value.
  - SupportedCultures is null so the middleware skips culture filtering. A
    one-element [InvariantCulture] list behaves the same but logs
    UnsupportedCultures on every request: the middleware's parent-culture walk
    bails at the empty culture name, so invariant is unmatchable by design.

With formatting out of the negotiation, the neutral `pt`/`en` entries that existed
only to widen Accept-Language matching no longer buy anything, and they made a
request resolvable to a neutral culture whose formatting comes from a
representative culture rather than a specified one. SupportedCultures.RequestMatch
is therefore gone; Tags is the single list, specific tags only, matching the
renamed catalogs.

Behaviour change worth stating plainly: a request asking only for a bare `pt` or
an unsupported variant now resolves to the configured default rather than
Portuguese. Both React apps canonicalize variants onto supported tags before
calling the API, so app traffic is unaffected; a hand-rolled client sending bare
`pt` is.

The new test drives the real middleware and pre-sets CurrentCulture to pt-BR
before invoking it, so it proves the middleware actively resets the formatting
culture rather than merely leaving an already-invariant ambient value alone.
The column shipped as unbounded `text`. A BCP-47 tag is short and bounded, so
there is no reason to accept arbitrary input at the storage layer: 10 characters
covers language-script-region (zh-Hant-TW), which is the longest form the
platform could ever offer.

Writes were already constrained to SupportedCultures.Tags by
UpdateUserCommandValidator on PUT /identity/profile, so this is the storage-level
backstop, not the validation.

AddUserLocale is edited in place rather than stacked with an ALTER: it has never
shipped in a release, it only exists on this branch.

Verified with `dotnet ef migrations has-pending-model-changes` (red before the
snapshot edit, green after) and by reading the generated DDL:
`ALTER TABLE identity."Users" ADD "Locale" character varying(10);`
Closes the third Codex P2, the one still open on fullstackhero#1344.

StartImpersonation strips the target's `locale` claim on purpose so the operator
keeps reading in their own language. But the dashboard normally runs on a separate
origin from admin, so it cannot read admin's persisted `i18nextLng`, and the
handoff URL carried only token/tenant/expiresAt. With no claim to negotiate from
and nothing in the handoff, the API culture fell through to the dashboard's own
browser detection: the operator picked Português in admin and then read English
error details in the impersonated session.

The handoff now carries `locale`, and the dashboard adopts it before createRoot.
That fixes both halves at once: the shell renders in the operator's language, and
apiFetch derives Accept-Language from i18n.language so the API localizes to it
too. Unsupported or absent tags are ignored, leaving normal detection in place.

Chosen over adding an actor-locale claim to the token, which Codex offered as the
alternative: this stays inside the two React apps and leaves the token contract
and the framework culture provider untouched.

`installImpersonationFromHash` becomes async for the changeLanguage await. The
token install stays synchronous at the top, and main.tsx awaits the whole thing
before createRoot, so the "installed before AuthProvider's first render" guarantee
is unchanged.

Known gap, deliberately not addressed here and named in the spec: the SignalR
client builds its own requests instead of going through apiFetch, so the hub
negotiate carries the browser's Accept-Language rather than the app's locale. That
applies to every session, not just impersonation. The spec asserts every OTHER
path carries the operator's locale, so a new channel that stops carrying it fails
the test instead of slipping through.
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

The varchar(10) constraint had no test pinning it — reverting HasMaxLength(10)
left the whole suite green, so the fix could regress silently. Asserts the
model-level max length directly, following the EventingDbContextModelTests
pattern for building a module DbContext without a database.
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

…tive

The two window validators interpolated `MaxWindow.TotalDays` — a double — into a
localized message. `ResourceManagerStringLocalizer`'s indexer formats arguments
with `string.Format` under `CurrentCulture`, which is now pinned invariant, so a
non-integral value would render with an invariant decimal separator regardless of
the reader's language.

Today the window is exactly 90 days, so both cultures render "90" and no output
changes. The point is that the type made it culture-sensitive by construction, and
this is the only place in any catalog where a placeholder argument was not an int,
long, string or enum — verified by enumerating every `MessageArgs` and every
`localizer["…", …]` call site.

Fixed at the source rather than at the call site: `MaxWindowDays` is the int the
message wants and `MaxWindow` derives from it, so the two can never disagree and no
cast can truncate.
Ten modules have a hand-written parity test. Notifications does not — it has no
test project at all, so the catalog this branch added to it shipped with no parity
guard. Parity is exactly the invariant that fails silently: a key missing from a
translated catalog falls back to the neutral English string and ships looking
translated.

Discovers catalogs by reflection (a type whose full name matches an embedded
`.resources` manifest, i.e. the co-located `ResourcesPath = ""` convention) across
every `FSH.Modules.*.dll` in the test output plus Core, then compares each
culture's OWN key set with `tryParents: false` — with parent fallback on, a missing
key would be answered by the neutral catalog and parity would always look perfect.

Covers new modules with no new test. Asserts a floor on the number of catalogs
discovered so a reflection regression fails instead of silently guarding nothing.

Verified by mutation: dropping `Notifications.NotificationNotFound` from
NotificationsResources.pt-BR.resx turns it red naming both the catalog and the key.
The rule still described the design this branch replaced: a negotiated
CurrentCulture, neutral `.pt` catalogs, and neutrals in the supported-tag list. A
contributor reading it would have re-introduced exactly what the review asked to
remove.

Records the UI-culture-only split and why the culture half has to be pinned rather
than left alone, the specific-tags-only convention and what adding a language now
involves, the requirement that placeholder arguments be culture-insensitive, and
the generic parity guard. Adds a "Known behaviour" section for the three things
that are deliberate and were previously only discoverable by reading code: the
one-token lag of the `locale` claim, impersonation carrying the operator's
language, and SignalR not carrying the app locale at all.
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

The dashboard side of the handoff was pinned; the producer side was not. Dropping
`params.set("locale", i18n.language)` from the dialog left every suite green, so
half the fix had no gate.

Drives Re-open (which pre-fills the user and skips the picker step) through to
Start, with `window.open` stubbed — the URL is the thing under test and the real
dashboard origin is not served in this suite. Asserts the operator's language in
both directions (pt-BR and en-US, selected via the `?culture=` detection hook) and
re-asserts token/tenant/expiresAt so the added parameter cannot quietly displace
the pre-existing contract.
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

Matching keys are not enough. The caller passes ONE argument list for every culture,
so a translation that consumes a different set of `{n}` placeholders than the
neutral string is broken in that culture only — never on the reviewer's machine.

`{1}` present in Portuguese but not English is the dangerous direction:
string.Format throws FormatException when the index is out of range, turning a
localized 404 into a 500 for Portuguese readers. The opposite direction silently
drops an argument the message was supposed to show.

Compares the placeholder index SETS per key, tolerating reordering (which
translation legitimately needs) and format specifiers, and stripping escaped braces
so a literal brace is not read as a placeholder.

All eleven catalog pairs currently match. Verified by mutation: adding a `{1}` to
Catalog.BrandNotFound in pt-BR fails with
"neutral uses {0} but pt-BR uses {0,1}".
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

index.html ships a static `lang="en"` in both apps and nothing ever updated it, so a
Portuguese UI kept declaring itself English to screen readers, browser translation
offers and hyphenation. The whole point of the feature is that the page is in the
reader's language; the attribute that tells assistive tech so was left behind.

An i18next `languageChanged` listener registered before init, so it covers the
initial detected language as well as every switch.

Also the only language signal in these apps that a stale render cannot satisfy,
which makes it the right assertion target for locale specs.
… language

Audit finding (concurrency lens), verified at the source. `updateMyProfile` is a
GET-then-PUT with no concurrency token, and Settings > Profile invalidates the SAME
["identity","me"] key the topbar reads. So a Settings save whose read preceded the
language PUT echoes the pre-switch locale back and wins if it lands second. The
topbar's hydration effect then saw `persistedLocale` change and called
`changeLanguage` on it — the user watched the UI revert with no error and nothing to
act on. Admin has the same shape via two rapid switches landing out of network order.

Hydration now stops once the user picks a language in this session. A locale chosen
on another device still carries over, because that is a fresh mount with no
in-session choice — pinned by hydration-guard.spec.ts.

This does NOT fix the lost update itself; the server can still end up holding the old
locale, which is what the `ponytail:` notes in both topbars record. The damage is
bounded from "the app changed language while I was using it" to "my language did not
stick across a reload". The real fix is an ETag / RowVersion with If-Match on
PUT /identity/profile — a contract change to an existing endpoint, its own PR.

Not pinned by a regression test: reproducing it needs an in-mount profile refetch
driven through the Settings form, and the click races the language-change re-render
(element detached from the DOM). Three distinct attempts, then stopped rather than
paper over it with retries. Recorded as unpinned rather than presented as verified.
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

Both verified at the source before changing anything.

UpdateUserCommandValidatorTests: the built-in-message test asserted only
ShouldNotContain("is not a valid email address"). A blank message, a raw resource key
leaking through, or any wrong-but-non-English string all satisfied that — the exact
failures it existed to catch. Now pins the actual Portuguese text, matching the
sibling test one method up. The literal came from the runtime, not from guessing.

ExceptionSeverityClassifierTests: LocalizedUnauthorizedAccessException subclasses the
BCL type precisely so the audit severity classifier keeps mapping unauthorized access
to Warning, and that intent existed only as a code comment. Changing the base type
would have silently reclassified every unauthorized access as Error with the whole
suite green. Added for both Localized subclasses.
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

Audit finding (data-contract lens), verified at the source.

TitleKeyFor mapped four statuses and sent everything else to "Error.Unexpected".
The type-name fallback next to it looked like it covered the rest, but it only fires
on ResourceNotFound — and Error.Unexpected resolves, so it never fired. Every status
outside those four reported a title contradicting its own status code and its own
detail. The 41 Conflict throw sites across Billing and Catalog answered
Status 409 with "An unexpected error occurred" and a detail describing an ordinary
business-rule conflict.

Before this branch, Title was `e.GetType().Name` — imperfect, but at least
status-consistent. This regressed that.

TitleKeyFor now returns null for a status with no title of its own, which routes
back to the exception type name, and Conflict gets a real localized title
(Error.Conflict, added to both shared catalogs). Any RFC7807-aware client branching
on `title` for a conflict sees a coherent value again.

Pinned in both directions: Conflict renders "Conflict"/"Conflito", and an
untranslated status (Locked) falls back to the type name rather than claiming the
error was unexpected.
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

Both parity specs iterate a hand-maintained namespace array. A namespace file added
without a matching entry escaped every parity assertion in the file — it could ship
half-translated with the suite green, which is precisely the failure these specs
exist to prevent. The backend closed the equivalent gap generically via reflection;
these two did not.

Verified by mutation: dropping a new JSON catalog into src/locales/en-US fails the
check naming the uncovered namespace.
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@marcelo-maciel

Copy link
Copy Markdown
Contributor Author

Thanks — and the split is the right call. I'm not arguing with it, but I've closed the review on this branch first, and I want to explain the order rather than just do it.

Three of these don't respect the split boundary. The impersonation locale fix spans both apps and only makes sense as one change. The pt-BR catalog convention spans the framework resx and both front-end catalogs. Fixing them once here means the three PRs get cut from a tree that's already correct, instead of each carrying its own half of the same fix and you reviewing the same decision three times. The framework PR goes up as soon as you want it; nothing below needs to be re-litigated when it does.

Everything below was verified at the source, and every fix has a mutation gate: I revert the fix, require the pinning test to go red, and restore the file byte-exact. Where something isn't pinned I say so.


Golden Rule #4. You're right and the omission was mine. The description now lists all eighteen BuildingBlocks files by project and says what each one is for, in the form used in #1323 and #1334. Worth noting one thing in the other direction: src/Directory.Packages.props now has a zero-line diff — the pin this branch carried had already landed on main, so the merge dropped it.

UseRequestLocalization and CurrentCulture. Went with UI-culture-only. The argument that decided it: main has no request localization at all, so pinning the formatting culture is less change than negotiating it — CurrentCulture now behaves exactly as it does on main, and only resource lookup follows the request. That makes CA1305 moot rather than merely bounded, which is a better place to be than relying on an analyzer staying enabled.

It wasn't a single switch, and the reason is worth recording: RequestLocalizationMiddleware.SetCurrentThreadCulture assigns both cultures unconditionally. So DefaultRequestCulture carries (InvariantCulture, configured default) — the middleware resolves the culture half as cultureInfo ??= DefaultRequestCulture.Culture — and SupportedCultures is null so culture filtering is skipped entirely. A one-element [InvariantCulture] list behaves the same but logs UnsupportedCultures on every request: the parent-culture walk bails at the empty culture name, so invariant is unmatchable by design. Pinned by a test that pre-sets CurrentCulture to pt-BR before invoking the real middleware, so it proves the middleware actively resets it rather than finding it already invariant.

RequestMatch with bare pt/en. Gone, exactly as you predicted — with formatting out of the negotiation those entries bought nothing. SupportedCultures.Tags is now the single specific-only list.

Argument formatting. Since the localizer formats with string.Format under CurrentCulture, I enumerated every MessageArgs site and every localizer["…", …] call site rather than assert it from inspection. All int/long/string/enum, with one exception: MaxWindow.TotalDays is a double. It renders "90" in both cultures today, so nothing changed on the wire, but the type made it culture-sensitive by construction — it's an int at the source now.

The Locale column. The summary was wrong and is corrected: nullable, no DB default, en-US as a code-level fallback. Now character varying(10); AddUserLocale edited in place since it has never shipped. And confirmed as you asked — Validation.UnsupportedLocale is wired: UpdateUserCommandValidator restricts Locale to SupportedCultures.Tags on PUT /identity/profile through the Mediator ValidationBehavior, with the column length as the storage-level backstop rather than the validation.

pt vs pt-BR. Went with your second option and made the convention specific-only: all eleven catalogs renamed to *.pt-BR.resx, pure renames. The asymmetry is gone and so is the pt-PT trap. The consequence is documented rather than implicit — a bare pt or an unsupported variant now lands on the neutral English catalog instead of Portuguese, which a test pins directly. .agents/rules/localization.md records the convention and what adding a language involves; it was describing the design I'd just replaced, which would have led the next contributor straight back into it.

JWT-carried locale. Documented as known behaviour, in the description and in the rules file, together with the impersonation and SignalR caveats.

Docs. Will land with the framework PR.


Changed after your review, and not reviewed by anyone. 0933b857 is what you read. Twelve commits since, from closing the third bot P2 and from a five-lens adversarial pass over the whole diff:

  • The operator's language now survives cross-app impersonation (the third Codex P2, still open when you reviewed).
  • A regression this branch introduced, which your review didn't cover and I'd want a second look at: TitleKeyFor sent every status outside four to Error.Unexpected. The type-name fallback beside it only fires on ResourceNotFound, and that key resolves — so it never fired, and all 41 Conflict throw sites across Billing and Catalog started answering Status: 409 with "An unexpected error occurred". Before this branch, Title was the exception type name. Unmapped statuses fall back to that again, and Conflict gets a real localized title.
  • A lost update on the profile: updateMyProfile is a read-modify-write with no concurrency token, and Settings › Profile invalidates the same query key the topbar reads, so a save could echo a pre-switch locale back and win — after which the topbar's hydration effect changed the UI language to it. Hydration now stops after an in-session choice. The underlying lost update is a ponytail: in both topbars: fixing it properly means If-Match on PUT /identity/profile, which is a contract change to an existing endpoint and shouldn't ride along here.
  • Both apps shipped a static <html lang="en"> that nothing updated.
  • Catalog parity is now enforced generically, including placeholder-index parity per key. Notifications had a catalog with no parity test and no test project to hold one; {1} in one culture and not the other is a FormatException at render time, in that culture only.
  • Two weak tests: one asserted only the absence of the English string, and ExceptionSeverityClassifier was never exercised with the Localized* subclasses that exist precisely to keep it working.

Not pinned, stated plainly: the hydration guard has no regression test. Reproducing it needs an in-mount profile refetch through the Settings form and the click races the language-change re-render. Three distinct approaches, then I stopped rather than reach for retries or a longer timeout.

CI is red on NU1903 (SSH.NET 2025.1.0 via Testcontainers), which fails restore on main too — dotnet restore at 3f2959e6 fails identically. The pin is in #1333. Verified locally with the audit disabled on the command line only, never committed.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants