Repository navigation
Conversation
The x-axis composed its label by gluing a translated month name onto a day
number, which hardcodes a Latin month-first shape and a space separator. A
lang=zh viewer read "7月 16" for a day bucket, where zh wants 7月16日, and
"7月 26" for a monthly bucket, which reads as the 26th of July rather than
July 2026. The same glue put the month first for sv and id, whose locales
order the day first ("16 juli", "16 Jul").
fmtLabel now builds a UTC Date from the bucket label and formats it with
Intl.DateTimeFormat, the call the tooltip formatter (fmtBucketDate) and the
server-side formatBucketLabel already make. The month branch carries a full
year: a two-digit year is not an option, because zh ICU renders it
"26年7月", which reads as the year 26.
The shape of the label now picks the branch, not the range, which drops the
range list that had to agree with the bucketing in click-repository.ts. The
month.* translation keys are left in place: the client no longer reads them,
but zh.ts is in flight in another change, and dropping keys from one locale
table at a time fails the key-parity test.
New client-chart-labels.test.ts extracts the shipped function from
adminClientScript() output and runs it per language. The chunk is evaluated
with no t() in scope, so a return to the glue throws instead of rendering a
wrong label. Mutating the year option or dropping the day option fails the
suite.
DennisAlund
left a comment
DennisAlund
left a comment
There was a problem hiding this comment.
Code review (xhigh). 10 findings, inline below. The two worst: the hourly axis branch still glues strings and disagrees with the tooltip in en/id, and day/week labels omit the year on axes that cross a year boundary.
Checked by running fmtLabel under Node 22 / ICU 77 for en, id, sv, zh and computing label widths from the chart geometry. vitest and e2e were not run.
| function fmtLabel(label, range) { | ||
| if (range === '24h') { | ||
| // "YYYY-MM-DD HH" → "HH:00" | ||
| return label.slice(11) + ':00'; |
There was a problem hiding this comment.
Hourly axis label still glues strings and keys off range. The branch still builds label.slice(11) + ':00' and is selected by range === '24h', so the PR's claims (no string glue, label shape picks the branch) don't hold here. The axis also disagrees with the tooltip (fmtBucketDate) on the same chart.
Scenario: 24h range, lang=en: axis shows 09:00, tooltip for the same point shows Jul 16, 2026, 09:00 AM (12-hour). lang=id: tooltip 09.00, axis 09:00. If a non-24h range ever returns hourly labels, parseInt("16 09") renders them as the date.
Suggest: detect hourly labels by the space, as fmtBucketDate does, format with toLocaleTimeString(UI_LANG, {hour:'2-digit', minute:'2-digit', timeZone:'UTC'}), and drop range. The only hourly test covers en.
| var day = monthly ? 1 : parseInt(parts[2], 10); | ||
| var dt = new Date(Date.UTC(parseInt(parts[0], 10), parseInt(parts[1], 10) - 1, day)); | ||
| if (monthly) return dt.toLocaleDateString(UI_LANG, { year: 'numeric', month: 'short', timeZone: 'UTC' }); | ||
| return dt.toLocaleDateString(UI_LANG, { month: 'short', day: 'numeric', timeZone: 'UTC' }); |
There was a problem hiding this comment.
Daily/weekly labels omit the year across a year boundary. Same ambiguity the PR fixed for monthly buckets.
Scenario: 1y range on 2026-10-20 (53 weekly buckets, dotInterval 5) shows Oct 20 | Nov 24 | … | Aug 31 | Oct 5 | Today. Oct 20 is 2025, Oct 5 is 2026, and nothing tells them apart. The all range with weekly buckets (91 to 730 days) shows every month name twice.
Suggest: add year when the first and last bucket fall in different years, or at least on the first label and each January label.
| var monthly = parts.length === 2; | ||
| var day = monthly ? 1 : parseInt(parts[2], 10); | ||
| var dt = new Date(Date.UTC(parseInt(parts[0], 10), parseInt(parts[1], 10) - 1, day)); | ||
| if (monthly) return dt.toLocaleDateString(UI_LANG, { year: 'numeric', month: 'short', timeZone: 'UTC' }); |
There was a problem hiding this comment.
Longer monthly labels now collide with the final Today label. Jul 26 became Jul 2026 (zh 2026年7月, sv sep. 2026), and renderTimeline has no collision guard.
Scenario: all range with 26 monthly buckets: dotInterval 3 draws labels at i=24 and i=25 (Today), 756/25 = 30.2 viewBox units apart. At font-size 9 mono (~5.4 units/char), en Sep 2026 is 43.2 wide and Today 27: half-sum 35.1 > 30.2, so they overlap. The old Sep 26 gave 29.7 and fit. zh gives 31.5 and sv sep. 2026/Idag 35.1, both overlapping. sv daily labels also widen, since ICU dec./juli replaces the translators' dec/jul.
| var MONTH_KEYS = ['month.jan','month.feb','month.mar','month.apr','month.may','month.jun','month.jul','month.aug','month.sep','month.oct','month.nov','month.dec']; | ||
| function monthName(m) { return t(MONTH_KEYS[m - 1]); } | ||
|
|
||
| // The chart's x-axis label for one bucket. Bucket labels are canonical and |
There was a problem hiding this comment.
month.* keys are now dead but still ship on every admin page. Nothing reads the 12 keys per locale (en/id/sv/zh) after this change, yet they serialize into var T = …. #71 has merged to main (c0f33a9), so the PR's reason for deferring their removal is gone.
Impact: 48 dead strings per admin page, and translators keep maintaining month names the UI never shows (sv jul vs ICU juli on the axis). Removing the keys from all four locales in this PR keeps the key-parity test in i18n.test.ts green.
| ["2026-07-13", "all"], | ||
| ["2026-07", "all"], | ||
| ]; | ||
| for (const lang of ["en", "id", "sv", "zh"]) { |
There was a problem hiding this comment.
Locale sweep hardcodes ["en", "id", "sv", "zh"]. i18n.test.ts on main now derives its locale lists from SUPPORTED_LANGUAGES so a new locale is covered as soon as it is registered.
Scenario: adding a fifth language (e.g. ja) to SUPPORTED_LANGUAGES leaves its chart labels unchecked, and nothing fails. Iterate SUPPORTED_LANGUAGES from ../../i18n instead.
| // The 1y range and a long all range bucket by week and still emit | ||
| // YYYY-MM-DD, so the range must not pick the shape, only the label can. | ||
| const fmt = loadFmtLabel("en"); | ||
| expect(fmt("2026-07-13", "1y")).toBe(fmt("2026-07-13", "all")); |
There was a problem hiding this comment.
"treats a weekly bucket label like a daily one" cannot fail. fmtLabel ignores range unless it is '24h', so fmt(x,'1y') === fmt(x,'all') always holds, and the test never checks that a weekly label shows the day.
Scenario: a change that renders YYYY-MM-DD labels without the day, or as month+year, keeps this green because both sides change together. Hardening: assert the output contains 13 and Jul, and compare against a known daily label.
| return monthName(parseInt(parts[1], 10)) + ' ' + parseInt(parts[2], 10); | ||
| } | ||
| // "all": label can be YYYY-MM-DD (daily/weekly) or YYYY-MM (monthly) | ||
| var parts = label.split('-'); |
There was a problem hiding this comment.
Bucket-label parsing duplicated. fmtLabel repeats the parsing fmtBucketDate does a few lines below (split on -, parseInt, detect the 2-part monthly shape, Date.UTC), and the server-side formatBucketLabel does it a third time.
Risk: a fix for one label shape lands in the axis or the tooltip but not both. Suggest extracting a bucketDate(label) returning {dt, kind} and having both functions format from it with their own option sets.
| var monthly = parts.length === 2; | ||
| var day = monthly ? 1 : parseInt(parts[2], 10); | ||
| var dt = new Date(Date.UTC(parseInt(parts[0], 10), parseInt(parts[1], 10) - 1, day)); | ||
| if (monthly) return dt.toLocaleDateString(UI_LANG, { year: 'numeric', month: 'short', timeZone: 'UTC' }); |
There was a problem hiding this comment.
Formatter rebuilt per label. toLocaleDateString with options builds a new Intl.DateTimeFormat on every call, on every render and range switch. The script already caches numberFormatter = new Intl.NumberFormat(UI_LANG) at the top; two top-level formatters (day, month-year) built once from UI_LANG would follow that pattern.
| ]; | ||
| for (const lang of ["en", "id", "sv", "zh"]) { | ||
| for (const [label, range] of cases) { | ||
| const out = loadFmtLabel(lang)(label, range); |
There was a problem hiding this comment.
loadFmtLabel(lang) called inside the inner loop. Each call regenerates the full adminClientScript() string, splits it, and compiles it with new Function, so the sweep compiles the client script 32 times (4 langs × 8 cases) where 4 would do. Hoist const fmt = loadFmtLabel(lang) into the outer loop.
| // UTC: "YYYY-MM-DD HH" for hourly, "YYYY-MM-DD" for daily and weekly, and | ||
| // "YYYY-MM" for monthly. | ||
| // | ||
| // The label is composed by Intl rather than by gluing a translated month name |
There was a problem hiding this comment.
Comment breaks the CLAUDE.md writing rules. "Active voice by default. Passive only when actor is unknown or irrelevant." and "Split sentences with more than two clauses." Line 965 reads "The label is composed by Intl rather than by gluing…": passive with Intl named as actor. Lines 965 to 969 form one sentence of four or more clauses, and lines 969 to 971 chain three more.
What
The chart's x-axis built its label by gluing a translated month name onto a day number, which hardcodes a month-first order and a space separator:
That composition is wrong in three ways. A
lang=zhviewer read7月 16for a day bucket where zh wants7月16日, and7月 26for a monthly bucket, which reads as the 26th of July rather than July 2026. Swedish and Indonesian order the day first (16 juli,16 Jul), and got the month first. The month branch also took the year fromparts[0].slice(2), so the same label was ambiguous in every locale.fmtLabelnow builds a UTCDatefrom the bucket label and formats it withIntl.DateTimeFormat, which is the call the tooltip formatter (fmtBucketDate) and the server-sideformatBucketLabelalready make. The month branch carries a full year: a two-digit year is not an option, because zh ICU renders it26年7月, which reads as the year 26.Two consequences worth naming:
click-repository.tsis gone. A weekly bucket label (YYYY-MM-DD) is handled the same as a daily one, which the1yand longallranges emit.MONTH_KEYSandmonthNameare deleted: nothing else read them. Themonth.*translation keys stay in the locale tables for now. The client no longer reads them, but they cannot be dropped one locale at a time, becausesrc/i18n.test.tscompares each locale's key set againsten, and the zh locale is in flight in Add Simplified Chinese to the admin dashboard #71. A follow-up can drop all of them together once that lands.English monthly axis labels grow by two characters (
Jul 26becomesJul 2026). Daily labels are unchanged in English (Jul 16).Tests
New
src/__tests__/unit/client-chart-labels.test.tscuts the shipped function out ofadminClientScript()output and runs it per language, the way the other client.ts tests do. The chunk is evaluated with not()in scope, so a return to the glue throws instead of rendering a wrong label. Assertions name the components that carry meaning (month, day, year) and their order rather than the whole string, followingformatBucketLabelinbig-chart.test.ts. Mutating the year option to2-digit, or dropping the day option, fails the suite.yarn test: 103 files, 1604 tests pass.yarn e2e: 32 tests pass.yarn tsc --noEmit: clean.Context
Raised in the review of #71, which adds Simplified Chinese to the admin. The axis-label defect is independent of that locale: it is the composition that is wrong, and zh is the locale that makes it visible.