Repository navigation
fix(frontend): pin formatCurrency to en-US instead of the host locale - #1732
Conversation
cristim
added
triaged
📝 WalkthroughWalkthroughThe shared ChangesCurrency formatting consistency
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/dashboard.ts (1)
965-979: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftSplit the chart code into bounded frontend modules.
frontend/src/dashboard.tsis at least 1,167 lines. This violates the TypeScript rule that files stay under 500 lines. Move the savings chart renderers and their callbacks into the appropriate dashboard chart bounded context before extending this module further.As per coding guidelines, TypeScript files must follow bounded contexts and stay under 500 lines.
Also applies to: 997-997, 1134-1150
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/dashboard.ts` around lines 965 - 979, Split the savings chart renderers and their callbacks out of the oversized dashboard module into an appropriate bounded dashboard chart module. Move the logic surrounding the savings breakdown, including the code near the current/committed renderer and the referenced sections around lines 997 and 1134–1150, while preserving existing behavior and keeping dashboard.ts under 500 lines.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@frontend/src/dashboard.ts`:
- Around line 965-979: Split the savings chart renderers and their callbacks out
of the oversized dashboard module into an appropriate bounded dashboard chart
module. Move the logic surrounding the savings breakdown, including the code
near the current/committed renderer and the referenced sections around lines 997
and 1134–1150, while preserving existing behavior and keeping dashboard.ts under
500 lines.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bcc2ac27-c741-4bd8-ac84-6789868cc8e8
📒 Files selected for processing (2)
frontend/src/dashboard.tsfrontend/src/utils.ts
formatCurrency() passed undefined as the locale to toLocaleString, so its digit grouping followed whichever locale the runtime (browser or, for tests, the machine running jest) happened to default to. On a locale that uses '.' as the thousands separator, $1,000 rendered as $1.000. Eight tests across utils.test.ts, riexchange.test.ts and approval-details.test.ts asserted the en-US grouping directly, so they failed locally on such a machine and passed in CI purely by accident of environment -- a genuine formatting regression under CI's own locale would have stayed green too. Pins the locale to 'en-US', matching the convention formatDate() and formatDateTime() already use in this same file for the identical ambiguity reason. formatCurrency's currency-symbol prefix is already locale-invariant (a fixed "$"/"€"/etc, not Intl currency-style formatting), so the digits after it should be too: CUDly's money model is USD-only end to end, and two admins should see the same digits for the same dollar figure regardless of their browser's locale. This is a production behavior change for real users on a non-US-locale browser, not only a test fix. Verified the 8 tests fail under LANG=de_DE.UTF-8 and pass under LANG=en_US.UTF-8 before this change, and pass under both locales after. Full suite (2781 tests) also passes under both locales post-fix. dashboard.ts has several independent toLocaleString() calls in Chart.js tooltip/axis-tick callbacks with the same unpinned-locale pattern; left untouched since no test currently exercises them and they are out of this issue's scope.
dashboard.ts's Chart.js tooltip and axis-tick callbacks built dollar
strings by hand with `$${x.toLocaleString()}`, six occurrences across
two charts. Same unpinned-locale defect as formatCurrency() before
the previous commit -- these fell back to the host locale too -- but
the deeper issue was duplication: dashboard.ts already imports
formatCurrency from utils.ts and uses it elsewhere in the same file,
so these six were reinventing it inline instead of calling it.
Routes all six through the shared helper. Mechanical: each site
formats a guaranteed-finite number (never null/undefined) with no
fraction digits and a literal '$' prefix, exactly formatCurrency's
default signature, so the change is a straight substitution with no
behavior difference beyond the locale fix itself.
These call sites render into a Chart.js <canvas>, not the DOM, so no
existing test exercises them and none are being added here -- the
change is verified by tsc, eslint, and the full suite passing under
both locales, not by new coverage of previously-untested chart code.
ab2b845 to
5529dd6
Compare
|
Rebased onto current Zero failures under de_DE confirms the fix (both the formatCurrency pin and the dashboard.ts DRY follow-up) survived the rebase intact. |
Full review, head
|
| build | locale | result |
|---|---|---|
| head | en_US.UTF-8 |
91/91 pass |
| head | de_DE.UTF-8 |
91/91 pass |
utils.ts reverted to origin/main |
en_US.UTF-8 |
91/91 pass |
utils.ts reverted to origin/main |
de_DE.UTF-8 |
2 failed, 89 passed |
✕ formats positive numbers correctly Expected: "$1,000" Received: "$1.000"
✕ supports custom currency symbol Expected: "€1,000" Received: "€1.000"
Node here is built with full ICU and LC_ALL genuinely drives Intl's default locale, so these are real host-locale runs, not an explicit-locale substitute.
F1 The pin is partial, and the gap is in the same file and the same view
This is the one I would want fixed. frontend/src/utils.ts:167:
month: d.toLocaleString('default', { month: 'short' })'default' is not a pin. It is an explicit request for the host locale, six lines below the formatCurrency this PR just pinned. Verified live:
HOST-default "Mär" en-US "Mar" de-DE "Mär" fr-FR "mars"
And it is not in some unrelated corner. dashboard.ts:8 imports formatCurrency and getDateParts from the same module, and dashboard.ts:443 calls getDateParts to render the month badge on the upcoming-purchases cards. So after this PR the dashboard shows en-US money and en-US dates everywhere except one month abbreviation that still follows the browser. That is exactly the failure mode where a partial pin is worse than none: the remaining inconsistency is now much harder to notice, because everything around it looks pinned.
It is invisible to the suite by construction: utils.test.ts:139 asserts only expect(result.month).toBeTruthy(), and both dashboard.test.ts and dashboard-ownership-950.test.ts mock getDateParts to a fixed { day: 15, month: 'Jan' }.
One-line fix ('default' to 'en-US'), plus an assertion that the month is 'Mar' rather than merely truthy.
F2 The dashboard.ts half of this PR has zero test coverage
dashboard.test.ts:104 does jest.mock('../utils', ...) and replaces formatCurrency with a stub returning `$${n}` — no separators, no locale. The dashboard tests therefore cannot observe the difference between the old toLocaleString() call sites and the new formatCurrency ones, in any locale. Every one of the changed dashboard call sites is reading-verified only.
That does not make the change wrong, and routing them through the shared helper is clearly the right direction. It just means the PR's verification evidence covers utils.ts and not the file where most of the diff lives.
F3 Nothing guards the pin, and the existing tests were victims rather than guards
The two tests that fail under de_DE are pre-existing; this PR adds no tests at all. And note the third row of the table above: under en_US, the pre-PR code passes 91/91. CI runs en-US. So the suite was green in CI with the bug present for its entire life, and it will be green in CI whether or not the pin survives a future refactor.
This is the same shape as the vacuous-assertion problem: tests that pass for a reason unrelated to the property you believe they protect. utils.test.ts:60 already shows the right pattern for the sibling concern (asserting formatDate's output shape regardless of host). One test asserting formatCurrency(1000) is '$1,000' with the host locale forced to de-DE would turn the existing accidental coverage into a real guard, and would be the natural place to also pin F1.
No parse-side exposure. Checked specifically; there is none.
Looked for the format to store/edit to parse cycle that would turn 1,234.56 versus 1.234,56 into a factor-of-1000 money error. It does not exist:
- No input is ever populated by a formatter. A sweep for
.value = ...formatCurrency/toLocaleString/toFixed/formatDate,setAttribute('value', anddefaultValue =returns zero hits. - The one money round-trip is symmetric and locale-independent:
groupModals.ts:172writes the purchase-cap input with rawString(...)(always.decimals, never grouped) andgroupModals.ts:227,235reads it back withparseFloat. - The numeric column filters (
lib/column-filters.ts:44-64) parse user-typed expressions against strict regexes and apply them to raw numeric cell values, never to rendered strings.
Minor
'en-US'is a bare literal repeated at 8 sites with no shared constant, even thoughutils.ts:14exportsCURRENCY_DEFAULT_DIGITSfor precisely this reason ("stay in lock-step rather than hard-coding0and silently drifting"). F1 is that drift, in the one dimension that never got the constant. Aconst UI_LOCALE = 'en-US'would make the next'default'obvious.- Two now-false comments that this PR makes stale:
recommendations.test.ts:1223still says "formatCurrency usestoLocaleString(undefined, ...)" and keeps its separator optional (/\$1,?100/) to tolerate a bug that no longer exists;recommendations.test.ts:7328still strips "any locale group separators". - Out of scope, flagging only:
modules/savings-history.ts:332defines a privateformatCurrencythat shadows the shared one (the module never imports it), rendering$${v.toFixed(2)}at seven call sites. Not a locale bug (toFixedis invariant), but it is the same divergence-from-the-shared-helper this PR is otherwise fixing, on a chart adjacent to the ones it touched. - Out of scope, unrelated to locale, but a money guard:
groupModals.ts:172usesmax_amount || '', so a stored cap of0renders as an empty input andgroupModals.ts:234'sif (maxAmount)then drops the constraint on save, widening a "$0 cap" to no cap. I did not verify whether0is reachable from the backend.
Verdict
The change is right, matches the existing convention, and genuinely fixes a real bug that I reproduced. Nothing here is a correctness risk on the money path, because there is no parse side to desynchronise.
I would not merge it as the complete fix its title implies, though. F1 leaves a host-locale formatter six lines away, feeding the same dashboard view — one line to fix, and fixing it here is much cheaper than discovering it later as "we already pinned the locale, so it can't be that". F2 and F3 are about evidence rather than code, but together they mean nothing in CI would notice if the pin regressed.
Execution-verified: the four-cell locale matrix above (my own runs, utils.ts reverted from origin/main and restored), the getDateParts locale divergence, and the getDateParts to dashboard.ts:443 call path. Reading-derived: the formatter/parser sweeps and the minor items.
getDateParts() called toLocaleString('default', {month: 'short'}) --
an explicit request for the host locale, the exact thing PR #1732
exists to eliminate from formatCurrency/formatDate/formatDateTime.
dashboard.ts imports both formatCurrency and getDateParts and renders
them side by side on the upcoming-purchases cards, so the dashboard
was showing en-US money and en-US dates except one month abbreviation
that still followed the browser (e.g. "Mär" under de-DE while
everything else read "Mar").
Pins it to 'en-US', matching the other helpers in this file.
Also adds regression coverage the original pin lacked: the pre-PR
suite passed 91/91 under en-US (CI's own locale), so nothing actually
exercised the locale-dependent branch. utils.test.ts now simulates a
de-DE host default (by intercepting the "no explicit locale" call
shape used by unpinned toLocaleString calls) and asserts formatCurrency
and getDateParts still render en-US output. Verified both assertions
fail pre-fix and pass post-fix:
- reverting getDateParts to 'default': expected "Mar", got "Mär"
- reverting formatCurrency to toLocaleString(undefined, ...):
expected "$1,000", got "$1.000"
Corrects two now-stale comments in recommendations.test.ts that
described formatCurrency as locale-dependent (toLocaleString(undefined,
...)); it's been pinned to en-US since #1732.
Filed #1747 for a separate, out-of-scope duplication issue found in
review: modules/savings-history.ts has its own private formatCurrency
shadowing this one (different, toFixed-based implementation, not
locale-dependent).
|
Addressed two findings from an independent adversarial review of this PR (both confirmed by execution): F1 — partial pin. F2 — the pre-PR tests never covered this. The suite passed 91/91 under CI's own en-US locale before this PR, so it was green with the bug present and would stay green whether or not the pin survives a future refactor. Added Verification (reverted each fix individually, ran the new assertions, captured actual output):
Also corrected two now-stale comments in Out of scope, filed separately: Gate results (frontend, CI-pinned tool versions where available — Node 22 locally vs CI's Node 24, no version manager available in this environment; TS/eslint/jest all installed from the repo's pinned
|
1 participant
Summary
formatCurrency()(frontend/src/utils.ts) calledvalue.toLocaleString(undefined, {...}), so its digit grouping followed whichever locale the runtime happened to default to. On a locale using.as the thousands separator,$1,000rendered as$1.000.utils.test.tsx2,riexchange.test.tsx4,approval-details.test.tsx2) trace to this single call site:riexchange.tsandapproval-details.tsboth format money exclusively through the sharedformatCurrency()helper, no separatetoLocaleStringcalls of their own.dashboard.tshad six more occurrences of the identical unpinned-locale pattern, hand-building$${x.toLocaleString()}in Chart.js tooltip/axis-tick callbacks instead of calling theformatCurrencyit already imports. Routed through the shared helper too, in a separate commit (see below).Deliberate vs. accidental
Established this before touching production code, since the fix differs materially depending on the answer:
formatCurrencyalready hardcodes a fixed currency-symbol prefix ($,€, ...) rather than usingIntl's currency-style formatting that would adapt symbol placement per locale — the formatting is already meant to be a fixed convention, just with the number grouping left accidentally unpinned.formatDate()/formatDateTime()both explicitly pin'en-US', withformatDate's doc comment reading "The en-US locale +month: 'short'removes ambiguity from pure numeric forms ('3/25/2026' vs '25.3.2026')... readable for non-technical users." That is the identical ambiguity argument for money, already decided once in this codebase, just not applied consistently toformatCurrency.formatCurrency's own doc comment discusses only fraction-digit precision, with no mention of locale intent — consistent with an oversight rather than a decision.Conclusion: production code change, pinning to
'en-US', matching the sibling date formatters in the same file. This is a real behavior change for actual users viewing the app on a non-US-locale browser, not only a test fix — worth stating plainly rather than downplaying. Concretely: a user on ade-DEbrowser currently sees$1.200for a value the backend/CI treats as $1,200; after this change they see$1,200, matching every other user regardless of their browser's locale.Commit 1: pin formatCurrency to en-US
One line, plus a comment explaining the reasoning above and referencing this issue.
Commit 2: route dashboard.ts's chart callbacks through formatCurrency
Before fixing the six
dashboard.tssites individually, checked whether they should instead call the sharedformatCurrency()helper —dashboard.tsalready imports it and uses it elsewhere in the same file. All six format a guaranteed-finite number (never null/undefined, always whole dollars, always prefixed with a literal$), which is exactlyformatCurrency's default signature. This was pure duplication, not a case with different precision or non-currency semantics, so all six were replaced withformatCurrency(x)calls:dashboard.ts:965,976-979— Chart.js tooltip callback strings (Current / Committed,Total,Lowest option,Upside)dashboard.ts:997,1150— Chart.js axis-tick callbacks (two charts)dashboard.ts:1134— cumulative-savings tooltip callbackThis fixes the locale bug and removes the duplication that let the two formatters drift in the first place. These call sites render into a Chart.js
<canvas>, not the DOM, so no existing test exercises them and none are being added here — the change is mechanical (verified bytsc,eslint, and the full suite passing under both locales), not by new coverage of previously-untested chart-rendering code.Verification (both locales, as required)
Pre-fix baseline:
Post-fix (commit 1, before the dashboard.ts commit):
Full suite (2781 tests), after both commits, both locales:
A fix verified under only one locale wasn't accepted as verification here — both were run and both are clean, before and after the second commit.
Test plan
npx tsc --noEmit— cleannpx eslint src/utils.ts src/dashboard.ts— cleanLANG=de_DE.UTF-8pre-fix, pass under both locales post-fix (evidence above)en_US.UTF-8andde_DE.UTF-8post-fix, after both commitsnpm run build— clean production buildCloses #1728
Summary by CodeRabbit