Skip to content

Review 5303

Cindy Zhang edited this page Aug 27, 2026 · 5 revisions

Review — PR #5303, fix(i18n): harden date locale handling

LOOP VERSION: 1.6.0 AUDIT RUBRIC: 1.13 AUTHOR CAN PROCEED: yes — round 4 resolves the standing public-contract finding

Author: nynexman4464 (internal, .github/ENGOWNERS) · branch fix/date-locale-followup · issue #5074 · carries #5296

Round Head Date Outcome
1 5c2d313e923 2026-08-25 Changes requested — avoid the Gregorian default
2 1ab13812794 2026-08-26 Needs human — one API decision; two conditional public variants drafted
3 1ab13812794 2026-08-27 Request changes — Gregorian contract accepted; public options type still contradicts it
4 a3ab868f7fb 2026-08-27 Approve proposed — explicit calendar is honored; component Gregorian contract is unchanged

Round 1 is reconstructed from GitHub, not from a contemporaneous record. That review was posted before this page existed; the quotes below are taken verbatim from the review body on the PR so round 2 has something to reconcile against (R1e, R31b).


Round 1 — as posted, 2026-08-25 (head 5c2d313e923)

Verdict: changes requested. The review accepted the locale threading and the rerender tests, and objected to one thing:

Ideally I'd like to avoid the Gregorian default. None of our 30 shipped locales move under it, but Locale is any tag: set th-TH and someone reading one screen gets 22 สิงหาคม 2026 in the field and 2569 in the <Timestamp> beside it, because only DateInput goes through plainDateFormat. Thai Buddhist months line up with Gregorian, so that year was right before.

It proposed pinning the three grid headers rather than the formatter, noted that #5296's hydration paragraph belonged in the changeset, and closed by asking whether pinning the headers covered it.

The author replied on 2026-08-25 that Gregorian is intentional and system-wide, and opened #5498 as a draft RFC for alternative calendar support. He then pushed one commit, 1ab13812794, which widened the pin instead of narrowing it.


Round 2 — 2026-08-26 (head 1ab13812794)

Verdict: needs human — held, nothing posted. Everything verifiable is settled; one decision belongs to a maintainer, and each answer produces a different public review. Both are written out below.

LANE: full — unresolved prior changes-requested, 42 files, changed public API on four exported helpers, a new lint rule, an intended visual change, and a related PR.

Problem

Three failures with named victims. The PR states two.

  1. A hydration crash-and-rebuild on any SSR'd date surface. plainDateFormat called Intl.DateTimeFormat(undefined, …), so a server with no LC_ALL emitted 2026 M08 where the browser rendered August 2026 and React discarded the tree. Reproduced in production on a freshly generated app that never mentioned dates.
  2. The touch date picker ignores the provider locale. Measured two ways: the PR's own new lint check flags 7 real call sites on main, six in the touch picker; and at a real coarse pointer the pre-PR sheet reads August 2026 under both a French and a Thai provider.
  3. On main, a Thai user who edits a date silently moves it 543 years. Driven in Chromium: the field renders 22 สิงหาคม 2569, and typing back the year it just showed you — 22/8/2569 — commits 22 สิงหาคม 3112. Display is Buddhist, the parser is Gregorian, and nothing reconciles them. Not stated anywhere in the PR, and it is the strongest argument for the change the PR is making.

Solution — 5 decisions, 3 independent judgements, ~450 runtime lines of 1049

  1. the four date helpers take `locale: Locale = 'en'` instead of an ambient locale  [problem 1]
  2. thread useLocale() through every unthreaded call site — Calendar hooks, DateInput
     pointer + touch, DateRangeInput, DateTimeInput, all of Schedule                   [problem 2]
  3. pin `calendar: 'gregory'` on EVERY date formatter, non-overridable   [NO STATED PROBLEM]
  4. extend the lint rule to require an explicit locale at those helpers               [problem 2]
  5. lab Chart currency/shortDate/monthYear gain a `locale` parameter                  [problem 1]

Decision 3 answers problem 3, which the PR never writes down — an unrecorded rationale, not an out-of-scope change. The remedy is two lines in the body, not a split.

Architecture

Owner is the shared date helpers; behavior unit is a pure utility, which is the right boundary. All seven documented seams driven, none source-derived: pointer surface, touch sheet, Calendar standalone, Calendar in the popover, native picker (n/a — the platform control formats itself), the Schedule view plug-in contract, and the lab Chart tickFormat callback (called with one argument, so the new second parameter cannot collide with a tick index).

Impact

Builders: nothing stops compiling; a two-argument plainDateFormat call changes from host-dependent to English. End users of SSR'd apps: the page stops being discarded on first paint. Phone users in any non-English locale: the sheet stops saying August 2026.

Users in a non-Gregorian-calendar locale get a two-sided change, both measured:

They lose the Buddhist year. On main a th-TH screen reads 2569 on the field, the <Timestamp> and the grid. After this PR all three read 2026 — a year that does not exist in their calendar. fa-IR goes ۳۱ مرداد ۱۴۰۵۲۲ اوت ۲۰۲۶.

They stop losing their data. The read-and-retype round trip goes from 3112 to exact.

API

change public? class verdict
~ plainDateFormat(pd, options, locale: Locale = 'en') yes — core/utils barrel formatSharedDate, formatInstant ok
~ formatSharedDate / parseDateInput / isLocaleDayFirst — same defaulted locale yes same ok
~ all four force calendar: 'gregory' over any caller-supplied calendar yes none — a new contract needs human; BLOCKS in the pin-stays branch
+ lab Chart currency(symbol, locale), shortDate(value, locale), monthYear(value, locale) lab is private: true @ 0.1.9 packages/charts already ships this exact shape ok
~ ScheduleContextValue.locale, and a required locale on the Schedule formatters internal to lab consistent with timezoneID ok

Rubric §3: nothing fires — no props, no rendered element, no booleans, no callbacks.

API Conventions §Principles, first bullet: "Guidance over enforcement. … If a consumer passes a prop value, the component renders it." That settles the override — discarding a caller's calendar is against convention whichever way the calendar question goes. It does not settle whether locale may select a calendar system; the page has no rule on calendar systems, and no row on Rulings mentions calendars, Gregorian, Buddhist or locale-selected calendars.

Ossification. The lab Chart parameters have a class (packages/charts shipped the same spelling). The 'en' defaults agree with useLocale()'s own fallback. The forced calendar has no other member — no second place in Astryx where a public helper silently overrides a caller's Intl option.

Theme targets

No new theme targets, and none removed or moved. ScheduleMonthTitle was rewritten from concatenation to formatToParts, so the emphasised element is now chosen from the parts array — it carries a StyleX class, not a theme target, and the emitted markup at the default locale is unchanged (<span class="_">August</span> 2026, before and after).

Breaking

@astryxdesign/core is published at 0.5.0, so there are consumers; lab and charts are private: true.

  • API — no. Every existing call still compiles.
  • Visualyes, for non-Gregorian-calendar locales only. fr-FR byte-identical. Nothing grew: the Schedule frames are byte-identical, so no reflow.
  • Behavioural, in the user's favour — the round trip goes from 3112 to exact.
  • Theme — no.

Bound tested past its default: calendar: 'gregory' changes isLocaleDayFirst's day/month answer for none of en-US, en-GB, th-TH, fa-IR, ar-SA, ja-JP, hi-IN.

Performance

Zero effects added, kept or moved; zero listeners; no forced reflow; no dependency. One memo dependency added ([][locale]), which is the correctness fix — the old comment claimed the list was constant for the component's life, and that was the bug.

Visual evidence

VISUAL CHECK: manual frames required
WHY: pixels change intentionally for non-Gregorian-calendar locales. The visual job is
     green on the exact head, but the Storybook preview hardcodes locale="en", there is
     no locale global, and the PR's evidence story is new — so no baseline shot renders
     a date under a non-en locale, and none renders the touch surface at a coarse
     pointer. A green gate with no story for the affected state is not coverage.
ENDPOINT COVERAGE: complete — pointer/standalone surfaces and the touch sheet.

Seven frames across three arms, each with a captureWithSensors() receipt. Every state expectation is invariant across the arms of its pair; the varying values are recorded in the difference table, never inside a receipt. Receipts within a pair match on every sensor except Build. One page error is declared rather than filtered silently: a pre-existing nativePicker unknown-prop warning, identical in every arm.

arm delta from head 1ab13812794
before the PR's runtime diff reverted in place — 23 files, keeping story/tests/changeset/lint plugin
after none — the head as proposed
pin-only deletion of exactly the 16 lines matching /^\s*calendar: 'gregory',$/ across 7 files; git diff -U0 is 16 deletions and nothing else
what it shows frames result
A — th-TH + fr-FR: field, Timestamp, standalone Calendar before / after / pin-only th-TH 2569 → 2026; fr-FR unchanged
B — Schedule's rewritten month title at the default locale before / after byte-identical, and the DOM sensor shows why
C — the touch sheet at a real coarse pointer before / after sheet title button named August 2026, Choose month and yearสิงหาคม 2026, Choose month and year

The pin-only arm is byte-identical to before. Identity is the expected contract here, which is what makes a byte-identical pair count as evidence: removing only the pin must reproduce the pre-PR render exactly, and it does, with all the locale threading still in place. That is what makes the split a measured option rather than an inference.

Not this PR's, present identically in both arms: the th-TH standalone Calendar renders English weekday abbreviations, because getStandaloneShortWeekdayNames has no th data. The touch sheet's weekday row is Thai after the change, which localises the gap to Calendar's CLDR table rather than the threading.

A11y and i18n

The axe job is green on the exact head with no new baseline entries. No focusable element is added, removed or reordered; no state, no animation, no CSS. Every date-rendering Intl.DateTimeFormat in shipped source was swept: each now takes the locale, and the remaining ambient ones are correct by design and already were (two machine-shape formatters pinned to en-US, a timezone-validity probe, and a host-timezone read).

An unclaimed improvement: AT-visible date labels now follow the provider locale — per-day aria-labels, every Schedule label, and the touch sheet title's accessible name. Before, a screen-reader user in French heard English day names and an English month from a French calendar.

Judgement

PROBLEM        note — the PR fixes a measured defect it never states
SOLUTION       note — 3 independent judgements; the third one's reason is missing
ARCHITECTURE   clear — all seven seams driven, touch included
IMPACT         note — a two-sided change; the PR describes one side
API            needs human — a new contract, no other member; the caller-`calendar`
               override is a note today and BLOCKS in the pin-stays branch
THEME TARGETS  clear
BREAKING       note — Visual is not "no"
PERFORMANCE    clear
VISUAL         note — intended, sourced, both endpoints covered
A11Y & I18N    clear — and an unclaimed improvement

No slot blocks today. The five notes are one question seen from five angles, not five survivable things stacking; the slot that owns it is API, because the render is the symptom and the frozen contract is the cause.

GOAL: met — measured outcome changes, not "tests pass": the sheet endpoint, all 7 flagged call sites fixed, the round trip, 785 tests across 12 date suites, 19/19 checks green on the exact head.

DISPOSITION, branch-labelled — two findings exist only if the pin stays:

# finding branch disposition
1 the calendar contract is new, unclassed public surface both needs human — the hold
2 the options type promises a calendar the implementation discards pin-stays blocks now · pin-stays
3 the decision's reason is not in the body or changeset pin-stays note · pin-stays
4 one new test asserts constructor arguments rather than an output difference both accepted — harmless
5 nativePicker unknown-prop warning (pre-existing) neither team follow-up
6 no th weekday data in Calendar (pre-existing) neither team follow-up

ADVICE: proven remedy for the split's availability — it was built and driven, not designed. AUTHOR CAN PROCEED: no.

WORST OUTCOME: "After this PR all three read 2026 — a year that does not exist in their calendar." Compatible with needs-human and with either branch's request-changes; not with an unattended approve.

JUDGEMENT NEEDED: API — a new contract, not a new instance. Does `locale` select a
                  calendar system in Astryx, or only language, numbering and field order?

What the review would decide, so it costs one word: take the pin as written. This reverses round 1, on one new measurement — the pin fixes a real defect rather than a preference; the alternative (a locale-aware parser) is #5498's whole scope; and the split, though proven available, leaves the 543-year round trip live. Against it, honestly: a Thai reader loses a year their calendar recognises, and that is being chosen on their behalf.

Reconciliation with round 1

what round 1 said status at 1ab13812794
"Ideally I'd like to avoid the Gregorian default." not taken — reversed, with an argument. On the new evidence the author is more right than round 1 was, and round 2 says so rather than re-asserting the position
"…22 สิงหาคม 2026 in the field and 2569 in the <Timestamp> beside it" fixed, and deleted from the new comment. All three surfaces now agree; the split state existed only at the head round 1 reviewed
"Thai Buddhist months line up with Gregorian, so that year was right before." half stands. The months do line up. "That year was right" is true for a reader and false for an editor — typing it back committed 3112
"Persian is the case that wanted it… plainDateFormat(m, {...DATE_FORMAT_MONTH_YEAR, calendar: 'gregory'}, locale)" not taken, and the suggested mechanism no longer exists — options are spread before calendar, so that exact snippet is now a no-op. Persian is confirmed as the real case
"Everything else lands as it stands" holds — nothing new found against the threading, the tests, Schedule or the lint rule
"#5296's hydration paragraph is worth lifting into the changeset" satisfied
"Does pinning the three grid headers cover it…?" answered — "the formatter, everywhere", in a PR comment rather than the body
inline: "Timestamp doesn't come through here" satisfied
on #5296: "Five call sites in the touch date picker never take a locale" satisfied, and it was seven — six in the touch picker plus one in Schedule's DayView

Pair status — with #5296

Not competing designs — a stack that got flattened, and #5303 is the strict superset. #5303's branch contains rebased copies of #5296's own commits, and the changeset credits their author, so merging #5303 makes #5296 empty. #5296 is CONFLICTING/DIRTY and untouched since 2026-08-25. Its single approval is from an engineering owner, not a design owner. Order: land #5303 (or the split), then close #5296 warmly as carried, with credit. Nothing is posted on #5296 before this hold resolves.

Did main move under it?

One commit since the head touches these files — a height change to the touch field. It does not touch the locale threading or any formatter. No semantic invalidation. The PR is MERGEABLE; it shows blocked only because round 1's changes-requested is still open. A merge with main is wanted before landing.

Exits

item exit who moves next
#5303 held for a ruling the maintainer. Rulings entry; escalation by age — 2 days stale, 3 days the loop applies its own recommendation and posts variant B
#5296 held with #5303 the maintainer, then the loop; closes as carried once #5303 lands
nativePicker warning · no th weekday data team follow-up, unfiled the team — both pre-existing, neither the author's

Critic

PASS on the third pass. Pass 1 failed on verdict legality (findings labelled as blocks while no slot blocked) and on an unmeasured claim; pass 2 failed on per-branch verdict labelling, a paraphrased worst-outcome quote, and an unnamed cost in the split variant. What the failures bought: the pin-only arm, the round-trip drive and the coarse-pointer pair all exist because a critic pass asked for them — and the round-trip drive reversed the recommendation.


The review, as it will be posted

Two variants. Exactly one is posted, unedited, after the decision. Both are request-changes; neither carries a Discord clause, because the author is internal.

Variant B — if the pin stays (recommended)

POSTS AS: request changes · cap 150 · actual 117 · 2 inlines, cap 20 each, actual 15 and 16

Thanks — the touch picker was the real gap, and pinning those six call sites is exactly right; the rerender tests prove it.

I asked you to avoid the Gregorian default and I was wrong: on main a th-TH field shows 2569 and typing that back commits 3112, so display and parsing disagree by 543 years. Taking it.

One thing to fix, and one worth adding: plainDateFormat spreads options before calendar, so a caller passing calendar: 'buddhist' silently gets Gregorian. Worth an Omit if that's the contract. The body also never says why the old behaviour was wrong — the round-trip is the reason and it belongs in the changeset.

Does the Omit look right to you?

Full review

[Reviewed by Robohands]

Inlines:

  • packages/core/src/utils/plainDate.ts:326 — options spread before calendar, so a caller's calendar is dropped. Omit<…, 'calendar'> if that's the contract.
  • .changeset/fix-plaindateformat-runtime-locale.md:5 — worth a line on why the old behaviour was wrong; the round trip is the reason.

Variant A — if the pin is split out

POSTS AS: request changes · cap 150 · actual 99 · no inlines

Request-changes rather than comment because the ask is a code change: revert the 16 pin lines and resend. Findings 2 and 3 are omitted deliberately — they exist only if the pin stays, and asking for work on code that is being removed is churn.

Thanks — the touch picker was the real gap, and pinning those six call sites is exactly right; the rerender tests prove it.

I'd like to take the Gregorian pin separately. It's a call about whether locale selects a calendar system at all, and I'd rather settle that on #5498 than inside a fix.

Send the locale threading, the lint rule and the chart formatters on their own and they land today; keep calendar: 'gregory' with the RFC. It reverts cleanly on its own, though the Thai round-trip stays broken until #5498 lands.

Does that split work for you?

Full review

[Reviewed by Robohands]

Do not mix the variants. A must not carry the inlines; B must not carry the split ask.

What changed before posting

Nothing yet — held for the ruling, nothing posted. This slot is filled when one variant goes up.


Round 3 — 2026-08-27 (R1e current-head re-review)

PR

#5303 fix(i18n): harden date locale handling by nynexman4464 (bucket: internal — .github/ENGOWNERS; collaborator permission write)

HEAD REVIEWED

1ab138127940281553bfea743df9e0ddf2a1404b

VERSIONS

LOOP VERSION: 1.5.0 AUDIT RUBRIC: 1.13

LANE

LANE: full WHY: unresolved prior changes-requested, 42 files, public helper semantics, a new lint contract, and intended date-output changes. Current main also moved under four touched component families, so R31c required a merge-tree run.

PROBLEM

WHY 1: Date formatting could use a host locale rather than the provider locale, while locale-selected calendars could disagree with Astryx's Gregorian date model. WHY 2: One screen could show the same date in different languages or years, and a displayed non-Gregorian year could be parsed back as Gregorian. WHY 3: A date control must preserve the selected day across SSR, display, editing, navigation, and adjacent date surfaces; disagreement can replace the server tree or commit a different year.

USER-FACING PROBLEM: A person reading or editing a date can see inconsistent values for one day and can submit a different year from the one the control displayed. PROBLEM SEVERITY: broken task — the old Thai display/parser round trip could move 2569 to Gregorian 2569, and host/provider disagreement could trigger hydration recovery.

VERDICT: clear — the updated body now states the Gregorian and hydration problems rather than only listing mechanisms.

SOLUTION

The provider's locale now reaches every date display instead of falling back to the machine running the code. The date layer treats that locale as language, numbering, and field-order input while keeping Astryx's date model Gregorian. Shared pure helpers own the formatting rules, and a lint check prevents shipped code from silently dropping the provider locale.

SOLUTION (3 runtime decisions · ~450 runtime lines of 1049 added)

  1. Pure date helpers use deterministic English when no locale is supplied, and internal callers must pass provider locale explicitly. [hydration problem]
  2. Calendar, every DateInput surface, Timestamp, PowerSearch, both chart packages, and every Schedule view receive the locale. [host/provider mismatch]
  3. Every date formatter pins Gregorian semantics while retaining localized language, numbering, and field order. [display/parser mismatch]

BURDEN: medium — no state, Effects, listeners, observers, or dependencies; broad cross-package plumbing, one public semantic change, and a focused lint rule/test matrix. BURDEN MATCH: proportionate — the same invariant must hold across every date surface, and each touched surface has focused coverage.

VERDICT: clear

ARCHITECTURE

OWNER: InternationalizationProvider supplies locale; the pure date/instant formatters own interpretation of Gregorian model values. TIER 1: provider locale via useLocale; pure helpers receive it explicitly, matching Architecture Cheat Sheet §Text/i18n/direction. TIER 2: none. SEAMS: pointer DateInput, touch DateInput, Calendar navigation/day labels, Timestamp, PowerSearch, chart callbacks, and all Schedule views. BEHAVIOR UNIT: pure utility — formatting and parsing are deterministic functions with focused tests; components only pass provider context.

seam driven result
DateInput + Calendar + Timestamp exact-head Chromium frame: all three show Thai text with Gregorian year 2026
touch picker rerender/navigation focused test passes in the exact head and current-main merge
PowerSearch / core and lab chart formatters focused output tests pass
Schedule views and AT labels focused monthly-view test passes; locale is carried through the shared context

The implementation lives with the date-formatting owners. No wrapper, portal, slot, render prop, or custom-surface seam owns this behavior.

VERDICT: clear

IMPACT

Existing callers using the 30 shipped locales keep Gregorian years while provider language, numbering, and field order now reach every surface. A Thai user sees 22 สิงหาคม 2026 in both DateInput and Timestamp rather than two years for the same day; the current push resolves the exact mismatch from cixzhang's prior review.

Builders calling the public plainDateFormat have one remaining trap: Intl.DateTimeFormatOptions still advertises calendar, but the implementation discards it. A caller can request Buddhist output, compile cleanly, and receive Gregorian output with no warning.

VERDICT: BLOCKS — the visible mismatch is fixed, but the public helper now accepts an option it intentionally ignores.

API

plainDateFormat(date, {year: 'numeric', calendar: 'buddhist'}, 'en-US')
// compiles; returns "2026", not Buddhist "2569"
change public? class doc'd? verdict
~ plainDateFormat(pd, options: Intl.DateTimeFormatOptions, locale: Locale = 'en') yes — core utils barrel formatSharedDate, formatInstant docblock finding: options type still exposes unsupported calendar
~ formatSharedDate(pd, format, locale: Locale = 'en') yes — core utils barrel same provider-locale fallback as useLocale() docblock ok
~ parseDateInput(input, locale: Locale = 'en') yes — core utils barrel same docblock ok
~ isLocaleDayFirst(locale: Locale = 'en') yes — core utils barrel same docblock ok
+ locale parameters on chart formatters no stable consumer — charts/lab are private: true existing formatter family JSDoc/tests ok
+ ScheduleContextValue.locale: Locale internal to lab existing provider values in context file header/tests ok

OSSIFICATION: The English defaults extend the landed useLocale() fallback and can be changed with a changeset. The Gregorian model is documented in the body and #5498, but plainDateFormat exposes a contradictory option: removing calendar from its accepted type later is breaking, while leaving it makes an unsupported request silently succeed with different output. API Conventions §Principles says, “If a consumer passes a prop value, the component renders it”; this helper currently does the opposite.

VERDICT: BLOCKS — the public type promises a calendar override the implementation intentionally rejects.

THEMING

No theme target, token, CSS variable, style contract, or shipped component style changes. The review harness's only StyleX is untracked test presentation and is banked separately.

VERDICT: clear

BREAKING

BEHAVIOR: yes, intentional — ambient/locale-selected calendars become Gregorian across date outputs; the prior DateInput/Timestamp mismatch is removed. API: yes — an existing public call with options.calendar still compiles but changes meaning because line 326 overrides it. VISUAL: yes, intentional — non-Gregorian locales change year/calendar while retaining locale language and numbering; the exact-head frame shows the intended consistent result. No CSS or geometry changes. THEME: no — no target, token, or override changes.

VERDICT: BLOCKS — the intended semantic break is documented, but the accepted options type gives existing callers no compile-time signal.

PERFORMANCE & RESOURCES

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

RENDER: two memo dependencies now include locale, so month/weekday labels recompute only when the provider locale changes; no extra render pass is added. LISTENERS/OBSERVERS: none added or changed. LAYOUT: no layout read/write or geometry change in the diff. BUNDLE: no dependency; existing formatter calls gain a calendar option.

VERDICT: clear

VISUAL EVIDENCE

VISUAL CHECK: manual frames required WHY: the response commit intentionally changes Thai Timestamp output, and the exact-head visual job's report skipped shot comparison because 376 shots exceeded its 240-shot budget.

The pushed response resolves the prior Thai year mismatch

Before — response pin removed from Timestamp only After — exact PR runtime
Before After

SENSOR RECEIPT: both captures use story review-pr-5303--date-consistency, neutral/light/LTR, 1200×900@1, fine pointer/hover, no forced colors or reduced motion, semantic state {sectionLabel: "th-TH date examples", fieldLabel: "Selected date", selectedDay: "22"}, visible 520×445 subject, loaded fonts, zero animations, and zero page/Storybook errors. Both report head 1ab138127940281553bfea743df9e0ddf2a1404b; the BEFORE arm removes exactly five calendar: 'gregory' lines from Timestamp/formatInstant.ts (arm diff); the common untracked harness is separately banked (harness diff). Receipts match on every sensor.

Difference: before, DateInput and Calendar show 2026 while Timestamp shows 2569; after, all three show 2026. Intentional — the body says Gregorian semantics must remain consistent across Timestamp and every date surface.

VERDICT: clear

A11Y & I18N

Auto-covered: exact-head pr-a11y passed and the PR does not change the baseline. Checked: visible and AT-facing Calendar/DateInput/Schedule dates receive provider locale; no new catalog keys, direction logic, focus path, keyboard path, or live-region mechanism. Browser evidence confirms the selected date and visible Timestamp agree under th-TH.

The English weekday abbreviations under the Thai Calendar are pre-existing missing CLDR data and are not charged to this PR.

VERDICT: clear

JUDGEMENT

slot verdict
PROBLEM clear
SOLUTION clear
ARCHITECTURE clear
IMPACT BLOCKS — public helper silently ignores an accepted option
API BLOCKS — calendar remains in the accepted options type
THEMING clear
BREAKING BLOCKS — existing calendar calls change meaning without a type error
PERFORMANCE clear
VISUAL clear — current push resolves the prior mismatch
A11Y & I18N clear

R1e: cixzhang asked to avoid the Gregorian default because DateInput and Timestamp disagreed. Commit 1ab13812794 fixes that mismatch across Timestamp, PowerSearch, charts, and Schedule and documents the Gregorian model; this review reverses the mechanism objection and extends it with the public-type contradiction introduced by that response.

GOAL: met — the sensor-checked Chromium pair moves Timestamp from 2569 to 2026 beside an unchanged 2026 DateInput, 883 exact-head focused tests pass, and 591 affected tests pass on the clean merge tree with current main. DISPOSITION: accepted — the prior mismatch and Gregorian decision are resolved and documented; blocks now — plainDateFormat accepts and ignores calendar. ADVICE: bounded direction — make unsupported calendar selection fail at compile time; removing calendar from the accepted options type is one direct shape, but the acceptance criterion is the contract rather than that syntax. AUTHOR CAN PROCEED: yes — callers must be unable to pass a calendar option that the helper will discard, and the existing runtime test should remain. WORST OUTCOME: “A caller can request Buddhist output, compile cleanly, and receive Gregorian output with no warning.” → BLOCKS

JUDGEMENT NEEDED: none — the PR and #5498 already establish Gregorian-only semantics; the remaining defect is that the type does not encode that settled contract.

request changes

  1. [BLOCKS] plainDateFormat accepts Intl.DateTimeFormatOptions.calendar and overwrites it → a builder can request Buddhist output, compile cleanly, and silently receive Gregorian output · packages/core/src/utils/plainDate.ts:321-326 Confirmed independently by the public signature and the passing runtime case at plainDate.test.ts:629-637; the whole diff contains no narrowed options type.

REVIEW

Thanks — this fixes the DateInput/Timestamp split I called out: the new head makes every date formatter Gregorian, and the focused browser pair now shows both Thai surfaces on 2026.

One API edge still needs fixing before merge. plainDateFormat accepts calendar inside Intl.DateTimeFormatOptions, then silently overwrites it; the new test proves calendar: 'buddhist' compiles but returns Gregorian. Could we remove calendar from the accepted options type so the Gregorian-only contract fails at compile time?

Full review

[Reviewed by Robohands]

INLINE (0-3, only if genuinely line-anchored)

  • packages/core/src/utils/plainDate.ts:321 — This still accepts calendar, but line 326 ignores it. Could we omit it from the options type?

EVIDENCE I DID NOT SPEND

  • Exact head: 13 focused files, 883 tests passed; 20 checks passed, pr-visual passed, and visual-acceptance remains pending.
  • Current main 8bb01b1f095 merges cleanly; its local merge tree 06ab7d31913b passed 591 affected tests across seven files.
  • #5296 is now a conflicting strict subset; this PR carries its commits and credits @josephfarina in the changeset.

TIME

TIME total 16m setup 4m worktree + fast install + build package + Storybook dev on 6323; warm main reused: yes reading 5m current diff, prior review, response commit, conventions, and current-main movement measuring 4m 883 exact-head tests, 591 merge-tree tests, 3 sensor attempts, 1 deliberate before arm writing 3m presentation + critic self-grade + wiki publication waste 1m first checkout used ambiguous FETCH_HEAD; one merge-test command ran from the parent directory

WHAT I COULD NOT VERIFY

  • Safari and Firefox behavior; managed-Mac policy permits Chromium only.
  • Whether any external consumer currently passes options.calendar; the public type permits it, but repository search cannot count downstream npm users.

What changed before posting

Not posted — read-only review. The clean-gate presentation is recorded as drafted.


Round 4 — 2026-08-27 (R1e delta re-review)

PR

#5303 fix(i18n): harden date locale handling by nynexman4464 (bucket: internal — .github/ENGOWNERS; collaborator permission write)

HEAD REVIEWED

a3ab868f7fbcfe2dbfab109ae1437c8380477b7b

VERSIONS

LOOP VERSION: 1.6.0 AUDIT RUBRIC: 1.13

LANE

LANE: full WHY: unresolved prior changes-requested, an existing public helper changes semantics, and the original PR spans the date/i18n surface. This round is delta-focused but reconciles all prior findings and every affected formatter family.

PROBLEM

WHY 1: The prior head accepted options.calendar but silently replaced it with Gregorian output. WHY 2: A builder could make a valid, typed request and receive a different calendar with no error. WHY 3: A low-level formatting helper must preserve explicit caller intent while components keep their own documented Gregorian contract.

USER-FACING PROBLEM: A builder explicitly requesting Buddhist display could compile successfully and still show Gregorian 2026 instead of 2569. PROBLEM SEVERITY: harmful friction — the task completed with silently wrong localized output.

VERDICT: clear — current head honors the explicit option and documents the limited compatibility boundary.

SOLUTION

The shared low-level formatter uses Gregorian when the caller leaves the calendar unspecified. If the caller explicitly chooses another calendar, that choice wins for display only. Astryx components continue supplying no calendar override, so their date model and visible output remain Gregorian.

DELTA SOLUTION (1 decision · 3 runtime lines plus test/docs)

  1. Preserve an explicit options.calendar; otherwise default to gregory. [prior public-contract finding]

BURDEN: low — no state, Effect, listener, dependency, export, or new type; one pure formatting branch with a focused output test. BURDEN MATCH: proportionate — the branch restores the already-published option contract without widening any component API.

VERDICT: clear

ARCHITECTURE

OWNER: plainDateFormat, the public pure utility that already accepts Intl.DateTimeFormatOptions. TIER 1: InternationalizationProvider continues to supply locale; component formatters keep their own Gregorian option. TIER 2: none. SEAMS: direct utility caller versus Calendar, DateInput, DateRangeInput, DateTimeInput, Timestamp, PowerSearch, both chart formatter packages, and Schedule. BEHAVIOR UNIT: pure utility — one input/output branch covered directly.

seam driven result
direct public helper with calendar: 'buddhist' 2569 BE
locale extension without explicit calendar Gregorian 2026
DateInput + Timestamp + Calendar under th-TH all remain Gregorian 2026 in exact-head Chromium
Schedule, PowerSearch, core charts, lab Chart focused tests retain Gregorian output

The behavior lives at the only public seam that exposes raw formatter options; every component-owned formatter remains explicitly Gregorian.

VERDICT: clear

IMPACT

Builders using the explicit low-level escape hatch again receive the calendar they requested. Everyone using Astryx date components keeps the resolved contract from round 3: provider-localized language, numbering, and field order with Gregorian values across display, parsing, navigation, and adjacent surfaces.

VERDICT: clear

API

plainDateFormat(date, {year: 'numeric', calendar: 'buddhist'}, 'en-US')
// "2569 BE"
change public? class doc'd? verdict
~ plainDateFormat(pd, options: Intl.DateTimeFormatOptions, locale: Locale = 'en') now preserves explicit options.calendar and defaults omitted calendar to gregory (packages/core/src/utils/plainDate.ts:322-330) yes — existing core utils export existing Intl.DateTimeFormatOptions capability docblock + changeset + RFC ok

OSSIFICATION: no surface is added or narrowed. The existing full options type and pre-PR explicit-calendar behavior stay intact; only the omitted-calendar default changes. API Conventions §Principles says, “If a consumer passes a prop value, the component renders it”; current head now does.

VERDICT: clear

THEMING

No theme target, token, variable, style contract, or component styling changes in the delta.

VERDICT: clear

BREAKING

BEHAVIOR: yes, intentionally and compatibly — relative to round 3, an explicit calendar request is honored again; omitted calendar remains Gregorian. All component paths stay unchanged from the reviewed head. API: no — the published signature is unchanged, and the current behavior preserves rather than rejects previously accepted input. VISUAL: direct helper output changes from 2026 to 2569 BE only when a builder explicitly asks for Buddhist display; exact-head component output remains Gregorian. The controlled frame pair proves both halves. THEME: no — no theme surface is touched.

VERDICT: clear

PERFORMANCE & RESOURCES

EFFECTS: zero added, changed, moved, or removed. RENDER: no React path changes; one nullish option selection occurs per direct helper call. LISTENERS/OBSERVERS: none. LAYOUT: none. BUNDLE: no dependency or meaningful code-size change.

VERDICT: clear

VISUAL EVIDENCE

VISUAL CHECK: manual frames required WHY: the delta intentionally changes visible text from the public helper while claiming every component surface remains unchanged.

Explicit override changes; component contract does not

Prior reviewed semantics applied to current tree Exact current head
Before After

SENSOR RECEIPT: both captures use current worktree SHA a3ab868f7fbcfe2dbfab109ae1437c8380477b7b, story review-pr-5303-noon--french-and-thai-gregorian, neutral/light/LTR, 1200×900@1, fine pointer/hover, no forced colors or reduced motion, semantic state {sectionLabel: "th-TH date examples", fieldLabel: "Selected date", selectedDay: "22"}, visible 552px-wide subject, loaded fonts, zero animations, and zero page/Storybook errors. Receipts match on every sensor. The BEFORE arm applies only the previously reviewed helper semantics; the common review story is banked separately.

The explicit helper line moves 20262569 BE; DateInput, Timestamp, and Calendar stay on Thai-localized Gregorian 2026 in both frames.

VERDICT: clear

REMEDY SEARCH

REMEDY SEARCH: not triggered — no proven visual defect

A11Y & I18N

Exact-head pr-a11y and pr-rtl passed with no baseline change. The production call-site sweep found no Astryx component passing a non-Gregorian calendar: every built-in date surface stays Gregorian, while only the documented public helper escape hatch honors an explicit calendar. The browser frame confirms DateInput and Timestamp agree under th-TH; the focused suite also covers Calendar navigation, touch input, DateRangeInput, DateTimeInput, PowerSearch, charts, and Schedule.

No focus, keyboard, direction, announcement, or translated-string path changes in the delta.

VERDICT: clear

JUDGEMENT

slot verdict
PROBLEM clear
SOLUTION clear
ARCHITECTURE clear
IMPACT clear
API clear
THEMING clear
BREAKING clear
PERFORMANCE clear
VISUAL clear
A11Y & I18N clear

R1e: cixzhang's round-3 review said, “plainDateFormat accepts calendar inside Intl.DateTimeFormatOptions, then silently overwrites it.” Current head resolves that exact finding by honoring explicit calendar, rather than narrowing the published type. Round 1's DateInput/Timestamp mismatch remains fixed, round 2's Gregorian component contract remains documented, and the changeset now records the compatibility exception.

GOAL: met — exact-head tests return 2569 BE for the explicit helper, Chromium keeps DateInput/Timestamp/Calendar on 2026, all 790 focused tests plus the lint-rule test pass, and all relevant remote checks are green. DISPOSITION: prior round-3 block resolved; all earlier findings remain resolved. ADVICE: omitted — no defect remains. AUTHOR CAN PROCEED: yes — no further change requested. WORST OUTCOME: none found → approve

JUDGEMENT NEEDED: none — this preserves an existing public option while keeping the already-settled component contract.

approve

PROPOSED EVENT

APPROVE

REVIEW

Thanks — honoring an explicit calendar closes the public API edge, while every Astryx date surface stays Gregorian. This is good now.

Full review

[Reviewed by Robohands]

INLINE

None.

EVIDENCE I DID NOT SPEND

  • The exact-head visual acceptance, stable visual regression, pr-a11y, pr-rtl, build, lint, test, sandbox, docsite, and dependency checks are green.
  • The author response is a single-parent commit; current main has not touched the PR's files since the round-3 review.
  • #5296 remains the carried strict subset and is not acted on in this review.

TIME

TIME total 14m

  • setup/rules 3m — safety guard, current loop 1.6.0, rubric 1.13, prior rounds
  • install/build/server 2m — fast-install seed, one config build, guarded Storybook
  • browser/a11y 2m — controlled pair, strict receipts, remote a11y/RTL results
  • focused tests 2m — 790 Vitest tests plus lint-rule test
  • code/history analysis 2m — semantic delta, API contract, full formatter radius, main movement
  • critique/wiki 3m — one critic pass and publication
  • CI wait 0m — all relevant checks had completed before this review started
  • waste 1m — first capture hit the known nativePicker warning; clean harness replaced it

WHAT I COULD NOT VERIFY

  • Safari and Firefox rendering; the managed-Mac review policy permits Chromium only.
  • Whether any downstream npm consumer passes options.calendar; the public contract permits it, so preserving it is the compatible choice.

CRITIC

PASS on the first pass: prior findings are explicitly reconciled; the approve body is 22 words; every claim is tied to current-head source, focused output, or exact-head browser evidence; no new finding was manufactured.

What changed before posting

Posted unchanged as APPROVE at exact head a3ab868f7fbcfe2dbfab109ae1437c8380477b7b on 2026-08-27 at 19:24:30 UTC: review 5044829206.

Delivery receipt

  • Event: APPROVE
  • Review ID: 5044829206
  • Delivered: 2026-08-27 at 19:24:30 UTC
  • Body: posted unchanged from this round's REVIEW block
  • Head: a3ab868f7fbcfe2dbfab109ae1437c8380477b7b
  • Exit: approved and open; pipeline 4, awaiting the maintainer's merge double-check. No auto-merge armed.

Clone this wiki locally