fix(core): one home for the number-display policy — and percent stops drifting between cell and measure (#4576) - #4589
Merged
Conversation
… drifting between cell and measure (#4576) `formatDisplayNumber`, `shouldGroupDisplayNumber` and `DisplayNumberFormatOptions` move from `@object-ui/i18n` into `@object-ui/core`; `@object-ui/i18n` re-exports all three, so every existing import path keeps compiling and both spellings resolve to the same symbol. `dataset-format`'s duplicate `Intl` implementation is gone. The move is what fixes the bug: `formatMeasure` needed this policy and could not import it across the React boundary, so it carried a parallel implementation, and the two drifted — a German session read `1.234,5 %` in a list cell and `1.234,5%` in a dashboard measure. The measure now renders the locale's own percent convention. No numeral moves in any locale at any magnitude: the value is formatted as percentage POINTS via a new `style: 'percentPoints'` rather than divided by 100 for `Intl`'s `style: 'percent'` to multiply back, a round trip measured to move 27,581 of 1,200,013 ordinary-magnitude en-US forms at rounding ties plus everything from 1e23 up. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Qqyix2QcnpUC9XeYVDzx3
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Contributor
✅ Console Performance Budget
📦 Bundle Size Report
Size Limits
|
…ale convention (#4576) `DatasetDefaultInspector`'s format-sample pin asserted the literal-suffix German form `12,3%`. The measure formatter now renders the locale's own percent convention, so the sample gained the no-break space before the sign, exactly as the two `dataset-format.locale.test.ts` pins did. Third and last authorized pin move for this card. The en sample in the case below it is unmoved, which is what says this is a convention change and not a numeral one. Caught by CI shard 3/4: my local sweep covered core / i18n / fields / plugin-dashboard / plugin-report but not app-shell, which is also a `formatMeasure` consumer. Re-swept the full consumption radius since: app-shell 394 files / 3,725 tests green, and plugin-gantt + plugin-grid + components 231 files / 2,064 tests green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Qqyix2QcnpUC9XeYVDzx3
Contributor
✅ Console Performance Budget
📦 Bundle Size Report
Size Limits
|
yinlianghui
marked this pull request as ready for review
August 13, 2026 15:41
This was referenced Aug 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #4576
Two stages, one PR, per the PM ruling on the card: the
formatDisplayNumberdown-move (PR #4577's open question 1) lands first, and the percent split closes on the moved home.Stage 1 — the down-move
formatDisplayNumber,shouldGroupDisplayNumberandDisplayNumberFormatOptionsmove from@object-ui/i18n(src/utils/number-display.ts) into@object-ui/core(src/utils/number-display.ts).@object-ui/i18nre-exports all three under the same names; nothing published was removed.Cycle check: none.
packages/corecontains zeroimport/export … from '@object-ui/i18n'statements (the only matches were four prose mentions insidedataset-format.ts's own comments, all rewritten here).core's dependencies are@object-ui/types,@objectstack/formula,@objectstack/spec,lodash,zod;@object-ui/typesdepends only on@objectstack/specandzod.@object-ui/coreadded to@object-ui/i18n's dependencies;check:phantom-depsgreen.Importer census — 7 sites, 3 distinct paths, all preserved:
packages/components/.../basic/elements.tsx@object-ui/i18npackages/plugin-dashboard/src/MetricWidget.tsx@object-ui/i18npackages/fields/src/index.tsx@object-ui/i18npackages/fields/src/widgets/CurrencyField.tsx@object-ui/i18npackages/i18n/src/utils/index.ts./number-displaypackages/i18n/src/index.ts./utils/indexpackages/i18n/src/__tests__/number-display.test.ts../utils/number-displayThe relative path is why
packages/i18n/src/utils/number-display.tssurvives as a re-export file rather than being deleted. Identity is pinned, not assumed:number-display.reexport-identity.test.tsassertsfromEntry === fromCoreandfromModule === fromCoreby reference — a second copy would pass every behavioural assertion in the repo and fail only there — plus a compile-time both-directions assignability pin on the option type.dataset-format'sformatNumberInLocaleis now a thin adapter over the movedformatDisplayNumberinstead of a hand-mirroredIntlimplementation. #4577 measured that mapping lossless across 32,760 combinations; re-verified here with a spot-check harness over 7 locales x a value grid x formats x currencies x percentScales — 5,390 combinations, 0 byte-level differences — and the wholedataset-formatsuite is byte-identical apart from the declared pin moves below.Stage 2 — the percent split, and a route the ruling did not anticipate
formatMeasureappended a literal%; it now renders the locale's own percent convention — the same oneformatPercenthas used since #4553 / PR #4565.percentDisplayValue's doc promise that a percent renders identically "as a row value and as an aggregated metric — the two surfaces can never drift" is true again, and the comment now records that it was briefly false and why.Measured convention move (en, ja, zh byte-identical, which is why this was invisible in an English session):
1.234,5%1.234,5+ U+00A0 +%1.234,5%%1.234,5— the sign moves to the FRONT%1,234.5%1,234.5%(unchanged)The ruling named
Intl'sstyle: 'percent'(whatformatPercentuses) and accepted a declared cost: "extreme-magnitude en forms move (the #4577 measurement — all >= ~1e20%)", with "ordinary-magnitude en percent MUST stay byte-identical" and "measure the exact threshold". I measured the threshold first, and there is no magnitude threshold. The divergence is rounding-tie-keyed, not magnitude-keyed, and it reaches all the way down:0.175at 2 decimals,0.18%becoming0.17%.style: 'percent'expects a FRACTION, so percentage points must be divided by 100 forIntlto multiply them straight back.0.175 / 100lands below the tie, so it rounds down. The en moves fix(core): dashboard measures follow the display locale (#4566) #4577 saw at 1e21 /MAX_SAFE_INTEGERare the same artefact at the other end of the range.That is STOP condition 3 of the ruling ("the percent unification measurably moving an ordinary-magnitude en form"), so I did not implement it. Instead I measured whether the locale's convention is reachable without the round trip, and it is:
style: 'unit'/unit: 'percent'/unitDisplay: 'narrow'produces a byte-identical percent affix tostyle: 'percent'across all 171 locale tags tested — 171 identical, 0 different — including the German no-break space, Turkish's prefix position, Arabic's own sign plus U+061C, and the Bengali/Marathi percent patterns that group Western-style where their decimal patterns group by lakh (verifiedA == Dthere, so that is the CLDR percent convention, not a quirk of the route). Against the literal-suffix form it moves 0 of 1,200,013 en-US forms, ordinary and extreme alike.So this PR delivers the ruling's stated goal — one percent convention across both surfaces, locale-correct NBSP where the locale says so — without incurring the declared en cost at all. The extreme-magnitude behaviour change the ruling authorised is not taken:
MAX_SAFE_INTEGERand 1e23 keep every digit, pinned. The mechanism is exposed as a new, self-documentingstyle: 'percentPoints'onDisplayNumberFormatOptions('percent'= value is a fraction, unchanged;'percentPoints'= value is already in percentage points), so the next caller picks the right one by name instead of rediscovering the trap. Please confirm this substitution — it is a deviation from the mechanism the ruling named, made because the ruling's own STOP condition fired on that mechanism.One gap deliberately left open, and pinned honestly
The two surfaces now agree on the CONVENTION but still differ at rounding ties, because
formatPercentkeeps the/100round trip:formatPercent(1.005, 2, 'en-US')is1.00%whileformatMeasure(1.005, '0.00%', undefined, 'whole', 'en-US')is1.01%. Half-up on the authored decimal is1.01, so the MEASURE is the faithful one and the CELL is the artefact. Closing it means editingpackages/fields'formatPercent, which this card's surface excludes. It predates this card, it is narrower than the split closed here, and it is pinned as an explicitly NOT-a-defect case inpercent-cell-vs-measure-4576.test.ts. Filed as #4590.Red-first, predicted in writing
Predictions are in each test file's header. Reverse verification removed the fix by patch +
git checkout --(nevergit stash), keeping the new expectations; restore confirmed byte-identical by sha256 across all 7 files.Verbatim, matching the predictions:
Cannot find module '../number-display'— the wholepercentPointssuite, red at module loadAssertionError: expected '60,8%' to be '60,8 %'(de),expected '1.234,5%' to be '%1.234,5'(tr),Expected: "٪" Received: "٥٠%"(ar),de-DE suffix: expected false to be true(affix parity)expected undefined to be [Function formatDisplayNumber]— the re-export identity pinpackages/fieldsThe 9 green on the red side are the must-not-change pins: every en-US case ordinary and extreme, ja-JP, zh-CN, and the rounding-tie cases. Post-fix: 36 passed (36).
Two of my own cases were mislabelled and the red run caught it — a
MUST NOT CHANGEcase each contained one de-DE assertion that legitimately moves. Both were split out into separately-named moving cases rather than relabelled in place. A third asserted a display magnitude below 1, whichformatPercentcannot reach (it scales stored values under 1 by 100); replaced with1.005, measured reachable on both surfaces.Must-not-change
MUST NOT CHANGEcase, and — the stronger evidence — the en-US assertion sitting beside each of the three moved pins passed untouched through every run.formatPercentsuite untouched and green;packages/fields' implementation was verified, not edited.core+i18n+fields+plugin-dashboard+plugin-report: 281 files, 4,492 tests passed (run twice, identical).app-shell: 394 files, 3,725 passed, 1 skipped.plugin-gantt+plugin-grid+components: 231 files, 2,064 tests passed.2 failed | 4,490 passedand1 failed | 3,724 passed, so the three moved lines are the entire blast radius across nine packages.integers-stay-String(v), the unknown-currency fallthrough, the malformed-tag retry and fix(core): dashboard measures follow the display locale (#4566) #4577's other deliberate preservations are untouched and pinned.Pins moved (3, all declared)
All are #4577 / #4575 pins, all assert the OLD literal-suffix German form, all move only where the percent path moves, and each is annotated in place with
PIN MOVED (objectui#4576):dataset-format.locale.test.ts—formatMeasure(0.608333333, '0.0%', undefined, undefined, 'de-DE'):60,8%becomes60,8+ NBSP +%dataset-format.locale.test.ts—formatMeasure(1, '0.0%', undefined, 'fraction', 'de-DE'):100,0%becomes100,0+ NBSP +%DatasetDefaultInspector.measureLocale.test.tsx(app-shell, a [plugin-report][app-shell] Dataset measures still render in the MACHINE locale — the consumers #4566 could not reach #4575 pin) — the German format sample:12,3%becomes12,3+ NBSP +%The
en-USassertion in each of the three cases is unmoved. No other test in the repo moved.Declared honestly: the third one cost a CI lap. My local sweep covered the five packages the dispatch named and not
app-shell, which is also aformatMeasureconsumer (#4577's own body names it). CI shard 3/4 caught it; the sweep above was then widened to the formatters' full consumption radius rather than to a named package list.Verification
turbo run build43/43 successful, 0error TS.type-check(bothtscpasses) green on@object-ui/coreand@object-ui/i18n.--filter '...@object-ui/i18n'(35 packages, 69 tasks green) and--filter '...@object-ui/core'(72 tasks green), 0error TSin both..d.ts(probe inpackages/fields, which resolves@object-ui/i18nthrough node_modules, not the source-treepaths):style: 'percentPoints'accepted;style: 'percentpoints'rejected withTS2820: Type '"percentpoints"' is not assignable to type '"percent" | "decimal" | "percentPoints" | undefined'. The message quotes the new union, which is what proves tsc read the rebuilt declaration rather than a cached one. Probe removed.origin/mainbaseline: net zero — core 529 to 529, i18n 35 to 35, fields 801 to 801, 0 errors throughout; theapp-shellpin file reports 0 problems on both sides. Baseline measured in a throwawayorigin/mainworktree, since removed.check:control-bytes,check:phantom-deps,check:changeset-presence,check:changeset-no-major,check:changeset-fixed,check:type-check-coverage,check:lint-coverage,check:i18n-call-site-keys,check:i18n-en-drift,check:spec-symbol-derivation,check:doc-links— all green.NBSPconstant, never a raw byte..d.ts, measured both waysBuilt before and after with
dist/andtsconfig.tsbuildinfocleared between runs:@object-ui/core— minor.index.d.tsgainsexport * from './utils/number-display.js';: three new entry-visible names plus the widenedstyleunion. New published surface on an entry export is minor on the fix(core): dashboard measures follow the display locale (#4566) #4577 / PR fix(fields): the date formatter's last three en-US channels thread the display locale (#4272) #4544 precedent.@object-ui/i18n— minor.dist/index.d.tsis byte-identical, butdist/utils/number-display.d.tschanges from three inline declarations toexport { … } from '@object-ui/core'— the declaration MOVED to another package, and the package gains that dependency in a consumer's tree. Per the ruling's rule (declaration-identical implies patch, declaration moved implies minor, on the fix(types): the DataSource contract carries deleteView's per-home outcomes (#4564) #4569 precedent), this is the moved case.fields,components,plugin-dashboard,plugin-report,app-shellhave no source diff (the only consumer-side changes are one new test file infieldsand one pin move inapp-shell) and no declaration change;formatMeasure's signature is unchanged. The behaviour reaches them through core's bump, which changesets propagates.Never major.
Generated by Claude Code