fix: consolidate duplicated date formatting into src/utils/date.js - #698
fix: consolidate duplicated date formatting into src/utils/date.js#698bhuvan-somisetty wants to merge 2 commits into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: bhuvan-somisetty The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
✅ Deploy Preview for project-hami ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Date/time formatting was reimplemented independently in EventLanding.js, case-studies.js, and BlogPostItem/Header/index.js instead of using the shared src/utils/date.js utility, which itself had unused exports. This already caused a shipped bug (Project-HAMi#659) that had to be hand-patched across three separate files because there was no single source of truth. - Extend src/utils/date.js with formatShortDate/formatNumericDate presets and give formatDateRange the formatRange feature-detection fallback that only EventLanding.js previously had. - Replace the local formatDate/formatDateRange copies in EventLanding.js, case-studies.js, and BlogPostItem/Header/index.js with imports from the shared utility, preserving each page's existing format granularity as an explicit named preset. - Add unit tests for src/utils/date.js using Node's built-in test runner, including a regression test for the Project-HAMi#659 UTC date-boundary bug. - Wire npm run test into check:all and CI, and document the "date formatting goes through src/utils/date.js" convention in CONTRIBUTING.md. Fixes Project-HAMi#697 Signed-off-by: bhuvan-somisetty <somisettybhuvan5@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughThe change centralizes date formatting in shared UTC-based utilities, updates event, case-study, and blog rendering, adds utility tests, and expands Node test discovery across scripts, CI, and contributor documentation. ChangesDate formatting and validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
77a4489 to
2116467
Compare
|
@rootsongjc @mesutoezdil could one of you take a look when you get a chance? All checks are green, just needs a review/lgtm. Thanks! |
|
Looks like an oversight. I thought I replaced all Eventlanding occurances after creating the date.js helper. But it's nice to see it used for BlogPosts as well. /lgtm |
|
Thanks for taking a look! Yeah, EventLanding had its own copy with the UTC fix baked in already, so it just needed to be swapped over to the shared util like the rest. Glad it's cleaner now for BlogPosts too. |
Resolve conflicts in .github/workflows/docs-health.yml and package.json by consolidating the two independently-added test scripts (mine for src/utils/date.test.js, upstream's for the markdown edge function tests) into a single npm run test that node --test discovers via its default globbing.
|
New changes are detected. LGTM label has been removed. |
|
Conflicts resolved, merged in the latest master and everything's green now. @rootsongjc thanks again for merging my last one, appreciate the guidance - would love your eyes on this when you get a chance! |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Date/time formatting was implemented independently in five places instead of going through the shared
src/utils/date.jsutility that already exists for it.src/utils/date.js's ownformatDate/formatDateRangeexports were unused dead code, whileEventLanding.js,case-studies.js, andBlogPostItem/Header/index.jseach carried their own copy with slightly different locale mapping and format options.This wasn't just cosmetic duplication — it already caused a shipped bug (#659, dates showing a day early west of UTC). The fix for that bug had to be hand-applied to three separate files (commit
a6b7c643) because there was no single place to patch, andEventLanding.js's independent copy wasn't touched by that fix at all (it happened to already havetimeZone: "UTC"by luck).This PR:
src/utils/date.jswithformatShortDate/formatNumericDatepresets, and givesformatDateRangetheformatRangefeature-detection fallback that onlyEventLanding.jspreviously had (so it no longer throws in environments withoutIntl.DateTimeFormat.prototype.formatRange).formatDate/formatDateRangecopies inEventLanding.js,case-studies.js, andBlogPostItem/Header/index.jswith imports from the shared utility, preserving each page's existing format granularity (long/short/numeric month) as an explicit named preset instead of an accidental divergence.src/utils/date.jsusing Node's built-in test runner (node:test), including a regression test for the Blog post and case study dates are one day early west of UTC #659 UTC date-boundary bug across bothenandzhlocales, and a test for theformatRangefallback path. No new dependency:src/utils/package.jsonscopes"type": "module"to that directory so the ESM source runs directly undernode --testwithout affecting the rest of the (CommonJS) repo or the webpack build.npm run test(wired intocheck:alland CI) and documents inCONTRIBUTING.mdthat date/time formatting should go throughsrc/utils/date.js.src/theme/BlogArchivePage/index.jsalready uses Docusaurus'suseDateTimeFormathook correctly and is left as-is — it's a different mechanism, not a duplicate ofsrc/utils/date.js.Verified in the built HTML for all three presets: case studies (short, en)
Jul 2, 2026, case studies (short, zh)2026年7月2日, blog post header (numeric, en)3/19/2026, event landing page (long range, en)July 28 – 30, 2026.Which issue(s) this PR fixes:
Fixes #697
Checklist:
npm run lintandnpm run format:checkpassnpm run buildsucceeds for bothenandzhgit commit -s)Summary by CodeRabbit
Improvements
Documentation
Testing