Repository navigation
Premium Analytics: format dates with the site locale instead of hardcoded US patterns - #50888
Conversation
chihsuan
added
Bug
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! |
Code Coverage SummaryCoverage changed in 6 files. Only the first 5 are listed here.
3 files are newly checked for coverage.
|
…e ranges on the calendar day
3baf253 to
e5398ba
Compare
chihsuan
added
[Status] Needs Review
| * `from`/`to` (e.g. a hand-edited or under-encoded deep link where the `+` | ||
| * offset decoded to a space) must not become an invalid Date — `formatDate` | ||
| * throws "Invalid time value" on one and would white-screen the page. | ||
| * offset decoded to a space) must not become an invalid Date and leak an |
There was a problem hiding this comment.
I think this comment is now wrong, and wrong in the direction that gets the guard deleted.
formatDate no longer throws; correct: dateI18n on an invalid Date returns the string "Invalid date".
But the other consumer of this range does throw. DateRangeInput stopped calling formatDate in this PR and now calls formatToTimezoneNaiveString, which does throw new Error( 'Invalid date provided' ) at packages/datetime/src/tz.ts:86.
So the white-screen path is intact; it just moved packages. Downgrading the stated consequence to a cosmetic label sets up someone reading this in six months to conclude that the isValid filter is decoration.
| * offset decoded to a space) must not become an invalid Date and leak an | |
| * offset decoded to a space) must not become an invalid Date: the picker's | |
| * `formatToTimezoneNaiveString` throws on one and would white-screen the page, | |
| * and the trigger label would read "Invalid date". |
| * Adjusts output based on whether dates share the same day, month, or year. | ||
| * | ||
| * Both ends are spelled out because WordPress provides no locale-specific | ||
| * range-elision rules. |
There was a problem hiding this comment.
NonNullable< Parameters< typeof dateI18n >[ 1 ] > resolves to Moment | Date | string | number, so formatDate( '2026-01-01' ) compiles and returns the previous day for every visitor ahead of the site.
The PR removes the custom-pattern escape hatch to stop callers from freezing the day/month order by hand. That hatch produced a wrong order. This one produces a wrong day, and it's the failure the docblock right above spends three lines describing in prose.
Date | number closes it in the compiler with no caller loss: the only non-Date call site is the chart at line 302, which passes a timestamp.
type DateInput = Date | number;parseSiteDateTime already exists to get from a string to a Date, so the narrow type routes people to it instead of documenting the trap.
There was a problem hiding this comment.
Thanks, narrowed to Date | number.
| axis: { | ||
| x: { | ||
| // Use the chart library's default behavior for 'custom' presets | ||
| tickFormat: xTickFormatType ? xTickFormat : undefined, |
There was a problem hiding this comment.
Worth a note on this line now that it carries more weight than it used to.
With ?? 'short' gone from line 302, this ternary is the only thing keeping the chart's own tick formatter.
formatDate's default is 'medium', not 'short', so if the guard ever collapses to tickFormat: xTickFormat, the X axis silently goes from the library default to the site's full date format on every tick.
The existing comment explains the 'custom' preset case, not this one. Something like "must stay conditional: formatDate defaults to medium, so an unconditional xTickFormat would put full site-format dates on every tick" would survive the next refactor.
There was a problem hiding this comment.
Agreed. Replaced it with the reason the ternary has to stay conditional.
| [] | ||
| ); | ||
|
|
||
| const renderTooltip = useCallback( |
There was a problem hiding this comment.
minor:
The description lists post summary, video summary, and latest-post as consumers left on their own date-fns formatting.
There's a third group worth naming: call sites that have already gone through formatDate and therefore change their output without appearing in the diff.
This tooltip is one. On a default F j, Y site, it goes from Jun 21, 2025 to June 21, 2025.
Same for metric-tabs-chart.tsx:85 and the post-views widget legend, which move to full both-ends ranges, and for the Emails Sent column, which additionally stops following the browser locale.
None of these are wrong 👍. They're just cells and tooltips that get wider, and none of them are covered by the testing steps.
| "@wordpress/compose": "8.4.0", | ||
| "@wordpress/data": "10.51.0", | ||
| "@wordpress/dataviews": "17.1.0", | ||
| "@wordpress/date": "5.51.0", |
There was a problem hiding this comment.
@wordpress/date is test-only in this package.
The single import is in src/helpers/__tests__/format-legend-labels.test.ts; the helper itself reaches parseSiteDateTime through @jetpack-premium-analytics/datetime.
The package has a devDependencies block. No bundle impact either way, but the manifest currently claims a runtime dependency that doesn't exist.
There was a problem hiding this comment.
Right, moved to devDependencies.
| const formatToString = ( date: Date | undefined, timeZone: string ) => | ||
| date ? formatToTimezoneNaiveString( date, timeZone ).slice( 0, 10 ) : ''; |
There was a problem hiding this comment.
.slice( 0, 10 ) is getDatePart from packages/datetime/src/date.ts:20, which is exported from the package this PR just added to routes/reports' dependencies.
Switching to the picker's own timezone here is the right call; that part isn't in question. something like...
| const formatToString = ( date: Date | undefined, timeZone: string ) => | |
| date ? formatToTimezoneNaiveString( date, timeZone ).slice( 0, 10 ) : ''; | |
| const formatToString = ( date: Date | undefined, timeZone: string ) => | |
| date ? getDatePart( formatToTimezoneNaiveString( date, timeZone ) ) ?? '' : ''; |
Nested ternary btw 😬
retrofox
left a comment
retrofox
left a comment
There was a problem hiding this comment.
Thanks, Chi. Nice enhancement. Approving ✅
The tokenizer holds up, and the two things the diff doesn't make visible check out.
I ran withoutYear against the date_format strings the core language packs ship, not just the ones in the test file. 36 formats, all correct, including the ones that break naive
tokenizers: Y \m. F j \d. (lt_LT), Y. \g\a\d\a j. F (lv_LV), Y\k\o F\r\e\n j\a (eu), Y. F j. (hu_HU), j. F Y. (hr). The dot-retention rule is what makes fi/cs/de work without
dragging in punctuation from formats where the dot floats.
The parseSiteDateTime to formatDate round-trip is DST-safe. getDate() anchors to the synthetic WP zone while dateI18n() renders through the IANA string, so a fixed-offset WP zone would shift near-midnight timestamps by a day in the opposite half of the year. setupWPTimezone clones the real IANA zone data (GB #48083).
Checked against WP 7.0.2 and Gutenberg's wp/6.9, our declared minimum.
@wordpress/date matches wp-build's /^@wordpress\// externals pattern, so it lands on core's wp-date handle and core's inline setSettings() applies. Moment stays out of the bundle.
Suite green under TZ=Asia/Taipei and TZ=Pacific/Midway: 200 suites, 1369 tests.
Notes inline. Two are worth acting on; the rest are follow-up.
Narrow `formatDate` to `Date | number` so a date-only string cannot compile: `dateI18n` reads one as browser-local midnight and renders the previous day for anyone ahead of the site. Also restore the white-screen consequence in the picker range docblock, note why the chart's tick-format ternary must stay conditional, use `getDatePart` instead of a magic slice, and move the toolkit's test-only `@wordpress/date` to devDependencies.
|
Thanks for the review! @retrofox 🙌 |
github-actions
Bot
added
[Status] UI Changes
2 participants
Fixes WOOA7S-1766
Proposed changes
Premium Analytics currently formats dates with hardcoded US-English patterns or the browser locale. As a result, dates can disagree with the language, date format, and timezone used elsewhere in wp-admin.
This change makes the affected dashboard dates follow WordPress's date settings:
@wordpress/date, using WordPress's translated month and weekday names and the site'sdate_format.medium: the site's complete date format.short: the site format without its year, falling back to the complete format when nothing remains.year: the year only.iso: the non-localizedYYYY-MM-DDformat used by query parameters.YYYY-MM-DDvalues round-trip without changing days.Reviewer focus
The
shortformat is derived from an arbitrary PHP date format by removing its year. The tokenizer must preserve escaped literals: for example, the Spanish formatj \d\e F \d\e Yuses\d\eto spell "de", even thoughdandeare also PHP date tokens. It must also retain the ordinal dot used by formats such as Finnishj.n.Y.The display formatter treats its input as an instant and renders that instant in the site timezone. Its parameter type accepts only a
Dateor a timestamp, so a site-local string cannot be passed by accident — those go throughparseSiteDateTime— while the date input uses its explicit picker timezone for both parsing and serialization.The X-axis
tickFormatprop is now limited to named date formats, but no current production caller opts into it. When omitted, the chart library's default tick labels remain in use. WOOA7S-1827 tracks localizing those ticks with separate visual review.This PR updates the consumers required for WOOA7S-1766. The post summary, video summary, and latest-post widget still have independent date-fns formatting and should be migrated separately.
A third group of consumers already went through
formatDate, so their output changes without appearing in the diff: the comparative-line tooltip and themetric-tabs-chartsubtitle move fromJun 21, 2025toJune 21, 2025(the oldmediumpreset wasMMM d, yyyy, the new one is the site'sdate_format), themetric-tabs-chartsubtitle and the post-views legend spell out both ends of a range, and the Emails Sent column stops following the browser locale. None of these are wrong, but the cells and tooltips get wider and the steps below do not cover them. #50893 reintroduces range elision where the locale allows it, so the wider ranges are temporary; the long-form single dates are not.The data layer keeps its own site-timezone accessor,
getSiteTimezone(), which resolves from the core-dataroot/siteentity rather than from the settings the formatters render in, and falls back to the browser zone until that entity loads. Only its zero-offset guard is fixed here; collapsing the two accessors touches query construction across the data layer and needs its own testing pass, so WOOA7S-1820 tracks it along withlocalTZDate()reading date-only values as UTC.Related product discussion/links
tickFormatcallerDoes this pull request change what data or activity we track or use?
No.
Testing instructions
Set the browser and the site to timezones that disagree, so an accidental conversion becomes visible. These steps assume an
Asia/Taipeibrowser; any zone works as long as it differs from the site's.One invariant runs through all of it: the calendar day shown for a given record must never move when the site timezone changes. The Stats API returns timezone-less, site-local timestamps, so the rendered day must follow the site's zone setting and must not be re-derived from the browser's.
Each group changes one setting at a time, so a failure points at a single cause.
Baseline
Europe/Amsterdamand Date Format toF j, Y.July 9, 2026. Keep it; the timezone group below checks against this exact value.Jul 9, 2026form.Date format
Leave the timezone at
Europe/Amsterdamand change only Settings → General → Date Format.d/m/Y— the Sent column reads09/07/2026: day-first, taken from the site format.j \d\e F \d\e Y, the format Spanish sites ship — the column reads9 de July de 2026. The month name stays English because the site language has not changed. What this checks is that the day-first ordering comes from the site format, and that the escaped\dand\erender as the word "de" rather than as the day and timezone tokens.j.n.Y, the Finnish format — on Jetpack → Analytics → Insights, the Most popular day widget keeps its trailing month dot, for example9.7., instead of losing it along with the year.Y— the same widget shows a readable year rather than an ISO timestamp.Steps 5 and 6 have an objective oracle, so they need not be eyeballed. Ask WordPress itself what the format should produce:
The dashboard should print exactly what PHP prints.
Ranges
F j, Y. Open the dashboard date-range picker and select a comparison preset. Compare to: should spell out both ends, for exampleMay 30, 2026 – June 28, 2026.d/m/Yand confirm the same range follows it, for example30/05/2026 – 28/06/2026. This exercises the range formatter together with the tokenizer, which the single-date steps above do not.Timezone
Return Date Format to
F j, Yand change only Settings → General → Timezone. The Sent value recorded in step 2 must stay on the same calendar day for every zone below.UTC+0— reopen the date-range picker, select a date such as July 9, and confirm the field stays2026-07-09even though the browser is in Asia/Taipei.Pacific/Midway— the Sent column still shows the day from step 2. This is the load-bearing case: at UTC−11 it is the only zone here on the far side of UTC, 19 hours fromUTC+8, so a timestamp parsed as UTC or as browser-local lands on the previous day here and nowhere else in this list.UTC-11— dates still render instead of falling back to an em dash. This is a different path from step 12 even at the same offset: choosing a manual offset leavestimezone_stringempty, so the zone has to come fromgmt_offsetalone.Prefer a negative manual offset to
UTC+8here.UTC+8is the same as the suggested browser timezone, so it cannot distinguish a correctly anchored value from one parsed as browser-local.Changing Site Language to a non-English locale additionally shows translated month names, for example
15 de julio de 2026 – 21 de julio de 2026. It is not required to review this PR, and the dashboard currently stalls for a long time on non-English locales for unrelated reasons — see WOOA7S-1822.Locally verified with the site set to
Europe/Amsterdam,UTC+0,Pacific/Midway, and a manual positive offset, while the browser usedAsia/Taipei. Regression tests also run withTZ=Asia/Taipei.