Review 4404

PR

#4404 feat(core): wire the translator adapter into i18n by AKnassa (bucket: external contributor)

HEAD REVIEWED

dd8aa87790921aaacbcccd63b31483e994586321

VERSIONS

LOOP VERSION: 1.6.0 AUDIT RUBRIC: 1.13

LANE

LANE: full WHY: New public provider prop and public context field in core; community-authored API surface; more than one decisive check required.

PROBLEM

WHY 1: Translator was already exported, but InternationalizationProvider could not accept one and resolve() always used the bundled formatter. WHY 2: An app already using react-intl/i18next/Lingui had to run Astryx strings through a second provider/catalog path. WHY 3: That duplicates locale synchronization and makes one app-level localization runtime impossible for Astryx-owned UI strings. USER-FACING PROBLEM: Builders with an existing i18n runtime cannot route Astryx component labels through the same runtime they use for product copy. PROBLEM SEVERITY: missing capability — #4029 asks for this adapter, and the old docs said it was roadmap-only.

VERDICT: clear

SOLUTION

The provider accepts an optional adapter and carries it through the i18n context. String lookup stays in Astryx: overrides, custom messages, parent locale, then shipped English. The adapter formats the resolved ICU message, including value-less messages; if it returns a non-string, the PR tries to warn and fall back.

SOLUTION (3 decisions · ~128 runtime lines of 1144 total)

1. Public translator?: Translator on InternationalizationProvider, plus translator?: Translator on the exported context value.
2. resolve() delegates every resolved message to the adapter before the no-values short-circuit.
3. Runtime guard for non-string adapter output.

BURDEN: medium — public API surface plus an opt-in call on every resolved Astryx string; zero Effects, listeners, timers, observers, or global state. BURDEN MATCH: proportionate — the capability is explicitly requested and the existing Translator type already points at this split, but the fallback branch below does not yet meet its own safety promise.

VERDICT: BLOCKS — non-string fallback returns the raw ICU template for interpolated messages

ARCHITECTURE

OWNER: i18n provider/context plus resolve() own lookup and formatting. TIER 1: Existing MessagesByLocale, Overrides, locale fallback, and warnOnce; no new subsystem. TIER 2: none. SEAMS: nested provider; provider absence; runtime locale swap; useTranslator() captured callback. BEHAVIOR UNIT: inline in resolve() — small pure branch, focused by i18n unit tests.

seam	driven result
omitted translator	focused tests keep existing bundled output and formatter cache
value-less messages	focused tests prove they reach the adapter
nested provider	focused tests prove child provider replaces the parent adapter
bad adapter output	confirmed broken for interpolated messages: object return yields Go to page {page, number}

VERDICT: clear

IMPACT

Default consumers see no change unless they pass translator. Opted-in builders get one runtime for Astryx strings, but a misconfigured runtime that returns a non-string for an interpolated label still puts the ICU placeholder text into the string path. The issue is user-facing because resolve() feeds labels, titles, and visible text.

VERDICT: BLOCKS — bad adapter return can expose raw ICU placeholders

API

Smallest call site:

<InternationalizationProvider locale={intl.locale} translator={translator}>
  <App />
</InternationalizationProvider>

	change	public?	class	doc'd?	verdict
+	InternationalizationProviderProps.translator?: Translator	@astryxdesign/core/i18n	existing adapter issue/type	en docs	ok after runtime fix
+	InternationalizationContextValue.translator?: Translator	exported context type	provider context extension	docblock only	ok after runtime fix

OSSIFICATION: This is new core API surface, but it implements the already-exported Translator type and the exact capability requested in #4029. The risk of being wrong is permanent: apps can key their i18n catalogs on the message text shape this PR documents.

VERDICT: note — public API surface still needs maintainer review after the runtime blocker is fixed

THEMING

No theme targets, tokens, CSS variables, StyleX, or rendered class contracts change.

VERDICT: clear

BREAKING

BEHAVIOR: no default-path behavior change; opted-in adapter path is new and currently has a bad fallback branch. API: no existing call site stops compiling; two optional public fields are added. VISUAL: no — no geometry, color, DOM structure, or CSS changed. THEME: no — no theme target/token/override contract changed.

VERDICT: BLOCKS — opted-in fallback behavior is wrong for interpolated messages

PERFORMANCE & RESOURCES

EFFECTS: zero added, changed, moved, or deleted.

Effect + deps	external system	why render/handler cannot do it	measured render cost	lifetime + cleanup	focused test
none	n/a	n/a	n/a	n/a	n/a

RENDER: no default-path render work beyond an extra optional-argument branch; opted-in apps pay one adapter call per Astryx string by choice. LISTENERS/OBSERVERS: none. LAYOUT: no layout reads or writes. BUNDLE: no dependency change.

VERDICT: clear

VISUAL EVIDENCE

VISUAL CHECK: not applicable WHY: changed files are i18n source, i18n tests, docs, and a changeset; no CSS, DOM structure, geometry, color, or intended pixel change. CI pr-a11y and pr-rtl skipped because no changed component was detected; no manual frames are required for a pure string-resolution path.

VERDICT: clear

REMEDY SEARCH

REMEDY SEARCH: not triggered — no proven visual defect

A11Y & I18N

Focused i18n tests passed at exact head: 8 files, 94 tests. The adapter receives resolved messages rather than keys, value-less strings reach it, locale swaps propagate, and omitted translator keeps existing output. The bad-return guard is the failing path: a translator.format() object return for @astryx.pagination.goToPage with {page: 5} warns, then returns Go to page {page, number}; the bundled path returns Go to page 5. That is still a string, but it is not the safe user/AT-facing fallback the PR claims.

VERDICT: BLOCKS — interpolated label fallback is unformatted

JUDGEMENT

slot	verdict
PROBLEM	clear
SOLUTION	BLOCKS — non-string fallback returns raw ICU template
ARCHITECTURE	clear
IMPACT	BLOCKS — bad adapter return can expose raw ICU placeholders
API	note — public API still needs maintainer review
THEMING	clear
BREAKING	BLOCKS — opted-in fallback behavior is wrong
PERFORMANCE	clear
VISUAL	clear
A11Y & I18N	BLOCKS — interpolated label fallback is unformatted

GOAL: partly met — adapter wiring works in focused tests, but the promised safe non-string fallback fails for interpolated strings. DISPOSITION: non-string fallback exposes raw ICU placeholders → blocks now. ADVICE: bounded outcome criteria — when adapter output is not a string, return the bundled formatted result for the resolved message, values, and locale; confirmed by resolve('@astryx.pagination.goToPage', {page: 5}, 'en', {}, undefined, badTranslator) producing Go to page {page, number} while the bundled path produces Go to page 5. AUTHOR CAN PROCEED: yes — fix the fallback so interpolated labels/titles receive formatted text, and update the existing bad-translator test to expect that. WORST OUTCOME: “a Pagination button can expose Go to page {page, number} instead of Go to page 5” → request changes.

JUDGEMENT NEEDED: none for the blocker; maintainer API review remains after the runtime defect is fixed.

request changes

1. Non-string adapter fallback returns the raw ICU template → an opted-in app with a bad rich-text/null adapter path can expose placeholder text in labels or titles · packages/core/src/i18n/resolve.ts:170

REVIEW

Thanks, this is close. One fallback still needs tightening before merge: a bad adapter return on an interpolated Astryx string falls back to the raw ICU template, so a Pagination button can expose Go to page {page, number} instead of Go to page 5. Could we route that through Astryx's formatter first? If you'd rather talk it through with someone, we're in Discord.

[Reviewed by Robohands]

INLINE (0–3, only when genuinely line-anchored)

• packages/core/src/i18n/resolve.ts:170 — Might need formatted fallback here; {page} can still reach the label as text.

EVIDENCE I DID NOT SPEND

• No prior human review or line comments existed on the PR.
• Exact-head CI was still running during review; lint, docs/check-components, build-storybook, dependency-check, and Meta CLA were green; Vercel was red.
• Public fork-wiki Critic-Rules.md contains the ratified #4022 executable-removal line not present in the local kit; it was not overwritten.

TIME

TIME total 7m to first clean gate setup/rules 2m local kit, public fork wiki, loop version, public critic delta install/build/server 1m fast-install seed only; no build/server needed browser/a11y 0m not applicable; no rendered visual path, CI a11y/RTL skipped focused tests 1m 8 i18n files / 94 tests green code/history 2m issue #4029, touched i18n files, API/rubric extracts, current-main history critique/wiki 1m presentation plus critic pass; wiki not yet pushed in this gate CI wait 0m sampled only; checks still in progress waste 1m first metadata command used an unsupported gh pr diff --name-status

WHAT I COULD NOT VERIFY

• Exact-head full CI had not completed when the review verdict was written.

What changed before posting

Not posted; read-only review draft.