-
Notifications
You must be signed in to change notification settings - Fork 0
Review 5303
LOOP VERSION: 1.4.0 AUDIT RUBRIC: 1.12 AUTHOR CAN PROCEED: no — one decision is a maintainer's
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 |
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).
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
Localeis any tag: setth-THand someone reading one screen gets 22 สิงหาคม 2026 in the field and 2569 in the<Timestamp>beside it, because only DateInput goes throughplainDateFormat. 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.
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.
Three failures with named victims. The PR states two.
-
A hydration crash-and-rebuild on any SSR'd date surface.
plainDateFormatcalledIntl.DateTimeFormat(undefined, …), so a server with noLC_ALLemitted2026 M08where the browser renderedAugust 2026and React discarded the tree. Reproduced in production on a freshly generated app that never mentioned dates. -
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 readsAugust 2026under both a French and a Thai provider. -
On
main, a Thai user who edits a date silently moves it 543 years. Driven in Chromium: the field renders22 สิงหาคม 2569, and typing back the year it just showed you —22/8/2569— commits22 สิงหาคม 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.
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.
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).
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
mainath-THscreen 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-IRgoes۳۱ مرداد ۱۴۰۵→۲۲ اوت ۲۰۲۶.They stop losing their data. The read-and-retype round trip goes from
3112to exact.
| 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.
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).
@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.
-
Visual — yes, for non-Gregorian-calendar locales only.
fr-FRbyte-identical. Nothing grew: the Schedule frames are byte-identical, so no reflow. -
Behavioural, in the user's favour — the round trip goes from
3112to 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.
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 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.
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.
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.
| 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.
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.
| 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 |
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.
Two variants. Exactly one is posted, unedited, after the decision. Both are request-changes; neither carries a Discord clause, because the author is internal.
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:
plainDateFormatspreads options beforecalendar, so a caller passingcalendar: '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?
[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.
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?
[Reviewed by Robohands]
Do not mix the variants. A must not carry the inlines; B must not carry the split ask.
Nothing yet — held for the ruling, nothing posted. This slot is filled when one variant goes up.
#5303 fix(i18n): harden date locale handling by nynexman4464 (bucket: internal — .github/ENGOWNERS; collaborator permission write)
1ab138127940281553bfea743df9e0ddf2a1404b
LOOP VERSION: 1.5.0 AUDIT RUBRIC: 1.13
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.
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.
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)
- Pure date helpers use deterministic English when no locale is supplied, and internal callers must pass provider locale explicitly. [hydration problem]
- Calendar, every DateInput surface, Timestamp, PowerSearch, both chart packages, and every Schedule view receive the locale. [host/provider mismatch]
- 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
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
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.
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.
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
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.
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 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.
| Before — response pin removed from Timestamp only | After — exact PR runtime |
|---|---|
![]() |
![]() |
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
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
| 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
- [BLOCKS]
plainDateFormatacceptsIntl.DateTimeFormatOptions.calendarand overwrites it → a builder can request Buddhist output, compile cleanly, and silently receive Gregorian output ·packages/core/src/utils/plainDate.ts:321-326Confirmed independently by the public signature and the passing runtime case atplainDate.test.ts:629-637; the whole diff contains no narrowed options type.
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.
plainDateFormatacceptscalendarinsideIntl.DateTimeFormatOptions, then silently overwrites it; the new test provescalendar: 'buddhist'compiles but returns Gregorian. Could we removecalendarfrom the accepted options type so the Gregorian-only contract fails at compile time?[Reviewed by Robohands]
-
packages/core/src/utils/plainDate.ts:321— This still acceptscalendar, but line 326 ignores it. Could we omit it from the options type?
- Exact head: 13 focused files, 883 tests passed; 20 checks passed,
pr-visualpassed, andvisual-acceptanceremains pending. - Current main
8bb01b1f095merges cleanly; its local merge tree06ab7d31913bpassed 591 affected tests across seven files. -
#5296 is now a conflicting strict subset; this PR carries its commits and credits
@josephfarinain the changeset.
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
- 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.
Not posted — read-only review. The clean-gate presentation is recorded as drafted.

