fix([DSTSUP-275]): format file sizes to reflect magnitude and locale - #5764
fix([DSTSUP-275]): format file sizes to reflect magnitude and locale#5764OsamaAbdellateef wants to merge 6 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
🦋 Changeset detectedLatest commit: 1655a44 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Accessibility tests executed. Download the report here. |
Coverage Report for Marigold Code Coverage
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||
OsamaAbdellateef
changed the title
|
Accessibility tests executed. Download the report here. |
jim761
left a comment
jim761
left a comment
There was a problem hiding this comment.
Automated Code Review
Spec match against DSTSUP-275 is good: unit-aware sizes via the ticket's own Math.log(size)/Math.log(1024) approach, plus a top-unit clamp and non-finite/negative guards the ticket didn't ask for. The one deviation — literal unit symbols instead of Intl.NumberFormat style: 'unit' — is documented in the changeset and I agree with it (340 byte next to kB in the same list reads badly).
Also correct and worth noting: useLocale comes from react-aria-components/I18nProvider, not the @react-aria/i18n shell — exactly what check-rac-first-imports.mjs guards — and pure-function cases live in fileUtils.test.ts rather than the component test.
All 28 CI checks pass. Four inline comments: one blocker (visual regression), one decision about the unit symbols, and two minor ones.
Generated with Claude Code review-pr skill
|
Accessibility tests executed. Download the report here. |
|
/run-chromatic |
jim761
left a comment
jim761
left a comment
There was a problem hiding this comment.
Automated Code Review
Reviewed against CLAUDE.md, DSTSUP-275, and the repo's own CI guards. Ran vitest run --project=unit-tests FileField fileUtils (54 passed) and probed formatFileSize directly across 19 boundary values in en-US / de-DE / ar-EG.
Verdict: non-blocking. Careful work — the fix matches the ticket, the rounding carry at unit boundaries is handled, and coverage is layered sensibly across the pure util, the component, and the browser. Seven notes, all nits, left inline on the relevant lines.
Spec match
DSTSUP-275 asked for "a unit-aware size, e.g. 2.35 kB / 1.2 MB / 340 B, picking the unit from the magnitude", localized via Intl.NumberFormat. Delivered exactly that. The ticket's style: 'unit' suggestion was deliberately declined and the changeset says why. The ticket's "Related, possibly a separate issue" (no onSelect on FileField) is correctly left out of scope.
What's good
- The i18n import path dodges a real bug class.
useNumberFormatterfrom@react-aria/i18nwould have been shorter, but it reads the shell'sI18nContext— the DSTSUP-261 / DST-1505 duplicate-context bug thatscripts/check-rac-first-imports.mjsexists to prevent. TakinguseLocalefromreact-aria-components/I18nProviderand buildingIntl.NumberFormatby hand is the right call; worth keeping if this is ever refactored. - The carry at unit boundaries is handled.
1048575renders1 MB, not1024 kB— theMath.round(scaled * 100) / 100 >= FILE_SIZE_STEPcheck, with[1024 ** 2 - 1, '1 MB']pinning it. - Module-level
Map<string, Intl.NumberFormat>is the correct caching shape for a formatter. - No
any, no@ts-ignore,as conston the unit tuple, explicit return types. Hook called unconditionally; no new state or effects; nothing formemo/useMemoto earn here.
One note that has no line to sit on
PR title uses fix([DSTSUP-275]): instead of fix(DSTSUP-275):. The repo's Conventional-Commits scope carries the bare ticket key — fix(DST-1675):, docs(DST-1703): in recent history. The brackets land in main's history verbatim on squash.
Visual regression
Chromatic build #244 ran on head faf191f8 and passed, reporting 3 changes accepted as baselines. Since the item description text is the only thing that changed, three diffs is the expected count — but the "Visual regression tests updated" checkbox is unticked, so worth confirming those three were the FileField stories and that accepting them was intentional.
Generated with Claude Code review-pr skill
| return `${value} ${FILE_SIZE_UNITS[exponent]}`; | ||
| }; | ||
|
|
||
| export const fileKey = (file: File): string => |
There was a problem hiding this comment.
fileKey was dropped.
The new size block was inserted where fileKey's explanation used to live, and took it with it:
-// Identity of a file for de-duplication and removal: two files with the same
-// name, size, and last-modified time are treated as the same file.fileKey is now the only undocumented export in this file, and it's the one whose contract (name + size + lastModified = same file) is least guessable from the body. Worth restoring those two lines.
Putting the size helpers after fileKey / dedupeFiles rather than between the accept-token helpers and fileKey would also keep the accept-matching code contiguous.
There was a problem hiding this comment.
Fixed — restored fileKey's identity comment and moved the size helpers after fileKey/dedupeFiles so the accept-matching code stays contiguous. The reorder landed in 3639c794b; the comment itself got dropped from that push by mistake and is now back in 6db08bfef.
| let scaled = bytes / FILE_SIZE_STEP ** exponent; | ||
| if ( | ||
| exponent < FILE_SIZE_UNITS.length - 1 && | ||
| Math.round(scaled * 100) / 100 >= FILE_SIZE_STEP |
There was a problem hiding this comment.
💡 The magic 100 here silently mirrors maximumFractionDigits: 2 on line 66.
The two have to stay in lockstep, 30 lines apart, with nothing connecting them — change the formatter to 1 or 3 digits and this boundary carry drifts without a single test noticing (the [1024 ** 2 - 1, '1 MB'] case is exactly what would break).
const FILE_SIZE_FRACTION_DIGITS = 2;
const FILE_SIZE_ROUNDING = 10 ** FILE_SIZE_FRACTION_DIGITS;There was a problem hiding this comment.
Fixed — extracted FILE_SIZE_FRACTION_DIGITS and FILE_SIZE_ROUNDING so the formatter's maximumFractionDigits and the boundary-rounding check read from the same constant instead of drifting independently. See 3639c794b.
| bytes === 0 | ||
| ? 0 | ||
| : Math.min( | ||
| Math.floor(Math.log(bytes) / Math.log(FILE_SIZE_STEP)), |
There was a problem hiding this comment.
💡 Optional: make the input guard total.
Line 77 already absorbs -1, NaN and Infinity, and fileUtils.test.ts:172 tests all three. A size in (0, 1) slips past it — Math.floor(Math.log(0.4) / Math.log(1024)) is -1, Math.min keeps it, and FILE_SIZE_UNITS[-1] renders as the literal string undefined:
formatFileSize(0.4, 'en-US') // → "409.6 undefined"
Unreachable today, to be clear: File.size is spec'd as an integer, and formatFileSize isn't re-exported from the package index — one production caller, FileField.tsx:210, passing file.size. So this is only worth doing if you'd rather the guard cover the whole domain than depend on that caller staying the only one.
| Math.floor(Math.log(bytes) / Math.log(FILE_SIZE_STEP)), | |
| Math.max(Math.floor(Math.log(bytes) / Math.log(FILE_SIZE_STEP)), 0), |
There was a problem hiding this comment.
Fixed — added the Math.max(..., 0) guard so the exponent can't go negative for a size in (0, 1), making the domain guard total rather than relying on File.size staying the only caller. See 3639c794b.
|
|
||
| // Identity of a file for de-duplication and removal: two files with the same | ||
| // name, size, and last-modified time are treated as the same file. | ||
| const FILE_SIZE_UNITS = ['B', 'kB', 'MB', 'GB', 'TB'] as const; |
There was a problem hiding this comment.
💡 SI prefixes with a binary step.
2400 bytes renders 2.34 kB, but 2.34 is the kibibyte count — 2400 B is 2.4 kB in SI, or 2.34 KiB in binary.
The changeset is upfront that 1024 was kept so numbers don't shift for existing users, and the ticket's own suggested implementation did the same, so this is a knowingly-taken trade rather than a slip. Flagging it only so it stays a team decision rather than quietly becoming precedent for the next component that formats bytes: either label KiB/MiB, or move the step to 1000 and accept the number change. Fine as-is.
There was a problem hiding this comment.
Agreed, no change made — same trade-off as the kB/binary-step decision above, so leaving it as-is per your note.
| makeFile('report.pdf', 'application/pdf', 2 * 1024 * 1024), | ||
| ]); | ||
|
|
||
| expect(screen.getByText('2.34 kB')).toBeInTheDocument(); |
There was a problem hiding this comment.
💡 This assertion depends on the machine's locale.
There's no I18nProvider here, so 2.34 kB is asserted against whatever useLocale resolves from the environment: green in CI (en-US), red for a developer on a German-locale machine, who gets 2,34 kB. The de-DE test right below is correctly wrapped.
The file already carries this assumption in its older label tests, so it's pre-existing rather than introduced by this PR — but wrapping the two new assertions in <I18nProvider locale="en-US"> would make them say what they mean, and would state the contrast with the de-DE test explicitly.
There was a problem hiding this comment.
Fixed — wrapped that render in <I18nProvider locale="en-US"> so the assertion states its own locale instead of depending on the machine's. See 3639c794b.
| await expect(canvas.getByText('2 MB')).toBeInTheDocument(); | ||
| await expect(canvas.getByText('5 MB')).toBeInTheDocument(); | ||
| await expect(canvas.getByText('512 kB')).toBeInTheDocument(); | ||
| await expect(canvas.getByText('2.34 kB')).toBeInTheDocument(); |
There was a problem hiding this comment.
💡 Same ambient-locale coupling as FileField.test.tsx:207 — 2.34 kB, 512 kB and the two MB assertions all pin en-US number formatting without an I18nProvider to guarantee it.
Slightly more consequential here than in the unit test, since pnpm test:sb runs in a real browser and picks up the developer's actual system locale.
There was a problem hiding this comment.
Fixed — rather than hardcoding a locale into the shared UploadFile story's render (which would have shadowed the de-DE wrapper other tests apply around UploadFile.Component), scoped an en-US decorator to just this .test() call. See 3639c794b.
|
|
||
| The step stays at 1024, which is what the field has always divided by, so a given file keeps the number it had before — only its unit and trailing zeros change (`0.50 MB` is now `512 kB`, `2.00 MB` is now `2 MB`). | ||
|
|
||
| The number is run through `Intl.NumberFormat` for the active locale, so a German consumer gets `2,34 kB` next to the field's already-localized labels. The unit symbol is not localized: `style: 'unit'` spells bytes out in its short form (`340 byte`), which reads inconsistently next to the abbreviated `kB` a row above it in the same list, and `B`/`kB`/`MB` are the same in every locale Marigold ships messages for. |
There was a problem hiding this comment.
💡 This paragraph is implementation rationale, not a changelog entry.
Why style: 'unit' was rejected is genuinely useful and shouldn't be lost — but consumers reading release notes aren't its audience. It belongs in a comment next to fileSizeFormatterFor, where the next person to reach for style: 'unit' will actually find it.
Paragraphs 1–2 plus the "same number, different unit" note are the changelog; four paragraphs is a lot for a patch.
There was a problem hiding this comment.
Fixed — trimmed the changeset to the changelog-relevant paragraphs and moved the style: 'unit' rationale into a comment next to fileSizeFormatterFor in fileUtils.ts. Changeset trim landed in 3639c794b; the comment is now in 6db08bfef.
|
Accessibility tests executed. Download the report here. |
…tionale Both doc comments were dropped from the previous commit before it landed.
|
Accessibility tests executed. Download the report here. |
| }; | ||
|
|
||
| const FILE_SIZE_UNITS = ['B', 'kB', 'MB', 'GB', 'TB'] as const; | ||
| const FILE_SIZE_STEP = 1024; |
There was a problem hiding this comment.
I do not think the rationale holds. The argument for kB + 1024 was that it is "the label every OS file manager and most consumer software already shows". That describes Windows Explorer, which is binary — and which writes KB, not kB. The other places a user would compare against are decimal:
- macOS Finder has been base-1000 since Snow Leopard, and writes
kB - GNOME Files is base-1000 by default, and writes
kB - Chrome and Firefox download UIs are base-1000
So the current pairing does not match either camp. It borrows the SI symbol from the decimal group and the divisor from the binary one, and the result is that a macOS user looking at the same 2,400-byte CSV sees 2.4 kB in Finder and 2.34 kB in this field. That is a worse outcome than either convention on its own, and it is the specific failure mode the "matches what users compare against" argument was trying to avoid.
My preference, in order:
- Move the step to 1000, keep
kB/MB/GB/TB. Correct by SI, matches Finder, GNOME and both browsers, and needs one constant changed. KiB/MiB/GiB/TiBwith the step at 1024. Also correct, less familiar.
The one argument for the status quo is the changeset's "a given file keeps the number it had before" — but that is already not true, and the changeset says so itself two lines later (0.50 MB is now 512 kB). The numbers are changing in this PR regardless; this is the cheapest moment to land on a defensible pair.
Worth settling properly because grep -rn "1024" across packages/ and docs/ finds no other byte formatting anywhere in the repo. Whatever ships here is the precedent, exactly as jim761 predicted.
There was a problem hiding this comment.
Taken option 1 — FILE_SIZE_STEP is now 1000 with kB/MB/GB/TB kept. You're right that the "matches what users compare against" argument pointed at the decimal camp all along, and that the "keeps the number it had before" property was already spent by this PR, so there was nothing left holding the pairing together.
What moved with it:
formatFileSizedocblock and the constants comment now say why the step is 1000 (SI symbols, Finder/GNOME/browser download UIs)fileUtils.test.tscases re-derived on the decimal ladder:2400 → 2.4 kB,999 B/1 kBat the first boundary, and the rounding-boundary case is now[1000 ** 2 - 1, '1 MB']FileField.test.tsxand theUploadFilestory test use decimal fixture sizes (2_000_000,512_000) so the expected strings stay clean; de-DE now asserts2,4 kB- the changeset's "step stays at 1024" paragraph is replaced by the SI rationale, and states plainly that numbers shift against the old output (
0.50 MB→524.29 kB)
See 1655a44ab.
| const FILE_SIZE_FRACTION_DIGITS = 2; | ||
| const FILE_SIZE_ROUNDING = 10 ** FILE_SIZE_FRACTION_DIGITS; | ||
|
|
||
| const fileSizeFormatters = new Map<string, Intl.NumberFormat>(); |
There was a problem hiding this comment.
💡 This came out of jim761's "a new Intl.NumberFormat per file per render", which was a fair catch, but there is a smaller answer than a hand-rolled cache.
@react-aria/i18n exports useNumberFormatter, which memoizes formatters internally and reads the locale straight from context. This repo already uses it, in packages/system/src/components/Formatters/NumericFormat.tsx:39, so it is the established idiom rather than a new dependency.
Passing the formatter in keeps formatFileSize a pure, directly testable function while deleting fileSizeFormatters, fileSizeFormatterFor and the useLocale() import:
export const formatFileSize = (size: number, formatter: Intl.NumberFormat): string => {and in the component:
const sizeFormatter = useNumberFormatter({ maximumFractionDigits: 2 });The fileUtils tests then build a formatter per case instead of passing a locale string, which is barely more code and drops an unbounded module-level Map from the bundle. Entirely optional, the current version is correct.
There was a problem hiding this comment.
Done — useNumberFormatter it is. fileSizeFormatters/fileSizeFormatterFor and the useLocale import are gone; the component does
const sizeFormatter = useNumberFormatter(FILE_SIZE_FORMAT_OPTIONS);and formatFileSize(size, formatter) is now pure. Following the NumericFormat.tsx precedent rather than hand-rolling a cache was the right call.
One wrinkle worth naming: the boundary-rounding check inside formatFileSize derives FILE_SIZE_ROUNDING from maximumFractionDigits, so moving the formatter to the call site would have re-opened exactly the "magic 100 mirrors line 66" coupling from the earlier review, now across two files. So the options object is exported as FILE_SIZE_FORMAT_OPTIONS and the component passes that to useNumberFormatter — one source of truth, and a module-level constant keeps the identity stable for the hook's memo. The tests build their formatter from the same constant.
See 1655a44ab.
`kB`/`MB`/`GB`/`TB` are SI symbols, so pairing them with a 1024 step matched neither convention: a macOS user saw `2.4 kB` in Finder and `2.34 kB` in the field for the same CSV. The step moves to 1000, which also matches GNOME Files and the browser download UIs. `formatFileSize` now takes an `Intl.NumberFormat` instead of a locale string, so the component can hand it `useNumberFormatter(FILE_SIZE_FORMAT_OPTIONS)` — the idiom already used in `NumericFormat` — and the module-level formatter cache plus the `useLocale` import are gone. The options object is exported so `maximumFractionDigits` and the boundary-rounding check stay in lockstep. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Accessibility tests executed. Download the report here. |
3 participants
Description
<FileField>rendered every selected file's size with a fixed megabyte divisor and two decimals ((file.size / 1024 / 1024).toFixed(2)+" MB"), so anything under ~5 kB read0.00 MB. For consumers importing CSVs — routinely 1–50 kB — the item description carried no information at all, and there was no way to override it from the outside since<FileField>owns the file list in internal state and renders the items itself.Sizes now step through
B,kB,MB,GB,TBand pick the unit that fits the file's magnitude, formatted throughIntl.NumberFormatfor the active locale. The step stays at 1024 (unchanged), so a given file keeps the number it had before — only its unit and trailing zeros change.Closes DSTSUP-275
Screenshots / Preview
No visual redesign — only the item description text changes.
0.00 MB2.34 kB0.50 MB512 kB2.00 MB2 MBde-DElocale0.00 MB2,34 kBTest Instructions
FileFieldstory in Storybook and upload a small file (a few KB, e.g. a CSV) alongside a multi-MB file.kBsize instead of0.00 MB, and the large file shows a cleanMB/GBvalue without a fixed two-decimal MB suffix.I18nProviderwithlocale="de-DE"and re-upload; confirm the number uses a comma decimal separator (e.g.2,34 kB) while the unit stayskB.pnpm test:unit -- FileFieldandpnpm test:sb -- FileFieldto run the added/updated tests.Breaking Changes
No
Checklist
component-testtag where applicable)pnpm changeset)