Skip to content

fix(frontend/dashboard): chart tooltip and axis-tick money formatting does not pin a locale #1733

Description

@cristim

frontend/src/dashboard.ts formats dollar values with 5-6 independent .toLocaleString() calls inside Chart.js tooltip and axis-tick callbacks, none of which pin a locale. This is the same defect #1728 fixed in formatCurrency, in a file that fix did not reach.

Exact line numbers are listed in PR #1732's body.

Why it was scoped out of #1732

Deliberately, and correctly. Those call sites render to canvas, not the DOM; no test exercises them; and none of #1728's eight failures originated there. Widening a one-line locale fix into untested chart-rendering code would have been scope growth into the least verifiable part of the frontend.

The question worth answering first

Should these call formatCurrency instead of formatting money ad hoc?

If they can, that is the better fix: the underlying defect is that money formatting exists in more than one place, which is exactly what allowed the locale divergence. Consolidating removes the class rather than pinning six more instances of it. formatCurrency already pins 'en-US' after #1732 and already hardcodes the $ prefix, so a caller gets both behaviours for free.

If they genuinely cannot — different precision for axis ticks, abbreviated forms like $1.2M, or non-currency numbers mixed in — then pin each explicitly and record why consolidation was rejected, so the next person does not re-litigate it.

Resolve that before writing any code. The answer determines whether this is a 1-line change or a 6-line change.

Verification

Same standard as #1732: run under at least two locales (LANG=en_US.UTF-8 and LANG=de_DE.UTF-8). Since no test covers these paths, a locale run alone proves nothing — render the dashboard and look at the chart under a non-US locale before and after. If that is impractical, say so plainly in the PR rather than implying coverage that does not exist.

Do not fabricate tests for chart rendering to make this look verified. An honest "mechanical change, untested path, here is the visual check" is worth more than a test written to satisfy a checklist.

Context

Found while fixing #1728.

No activity

Activity on this issue will appear here.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions