refactor(calculate): replace JSON.parse/stringify anti-pattern with structuredClone() in aggregateCalendars#5025
Conversation
…ucturedClone() in aggregateCalendars
|
Someone is attempting to deploy a commit to the jhasourav07's projects Team on Vercel. A member of the Team first needs to authorize it. |
Aamod-Dev
left a comment
There was a problem hiding this comment.
Excellent refactoring! Replacing the JSON.parse/stringify serialization hack with structuredClone() will definitely improve performance during calendar aggregation. Approving!
Aamod-Dev
left a comment
There was a problem hiding this comment.
Thanks for putting this together. The changes are clean and the logic looks sound. The new additions are clean and integrate nicely with the existing codebase. This is good to go.
Aamod-Dev
left a comment
There was a problem hiding this comment.
Thanks for the contribution. I've taken a look at the changes and everything seems to align with what we need here.
I am adding the quality:exceptional label because the implementation not only solves the core issue but goes beyond by effectively handling edge cases, tests, or reducing technical debt. Great work going above and beyond!
No concerns from my end. Approved.
Aamod-Dev
left a comment
There was a problem hiding this comment.
Thanks for the contribution. I went through the changes and the overall approach looks good, but there are a few issues that should be addressed before this can be merged. Most of the concerns are related to correctness and maintainability.
- There are merge conflicts with the base branch. Please resolve them to ensure existing functionality isn't broken.
Once these issues are addressed, I'll be happy to take another look. Thanks again for the contribution.
|
🚨 Hey @ChetanSenta, the CI Pipeline is failing on this PR and it has been marked as Please fix the issues before this can be reviewed. Here's how: 1. Run checks locally before pushing: npm run format:check # Check Prettier formatting
npm run lint # Run ESLint
npm run typecheck # TypeScript type check
npm run test # Run unit tests (Vitest)
npm run build # Verify production build passes2. Auto-fix common issues: npm run format # Auto-fix formatting with Prettier
npm run lint -- --fix # Auto-fix lint errors where possible3. Check the full failure log here: Once you push a fix and the CI passes, the |
Aamod-Dev
left a comment
There was a problem hiding this comment.
Thanks for the updates! The overall approach looks good, but the CI pipeline (\Format · Lint · Typecheck · Test) is currently failing. Please run
pm run test\ and
pm run lint\ locally to fix the issues. Let me know once it's green!
Aamod-Dev
left a comment
There was a problem hiding this comment.
Thanks for the contribution. The overall approach looks good, but the CI pipeline is currently failing. Please run the checks (like npm run format, npm run lint, and npm run test) locally to identify and fix the issues. Let me know once it's green!
ok |
Aamod-Dev
left a comment
There was a problem hiding this comment.
Thanks for fixing the CI pipeline! Approving.
|
🎉 Congratulations @ChetanSenta! Your PR has been successfully merged. 🚀 Thank you for contributing to CommitPulse. Your work helps us build a better tool for the community.
Keep building! 💻✨ |
Description
Fixes #5019
Pillar
Visual Preview
No visual change. This is a pure internal refactor in lib/calculate.ts.
What this PR does
Replaces the
JSON.parse(JSON.stringify(baseCalendar))deep cloneanti-pattern in
aggregateCalendars()withstructuredClone().structuredClone()is native in Node 18+ (CommitPulse requires Node 18+),faster (no JSON string intermediate), and correctly handles edge cases
that JSON serialization drops silently (undefined values, Date objects).
Before vs After
// Before (anti-pattern — silent data loss for undefined/Date fields):
const aggregatedBase = JSON.parse(
JSON.stringify(baseCalendar)
) as ContributionCalendar;
// After (idiomatic Node 18+ — correct, fast, readable):
const aggregatedBase = structuredClone(baseCalendar);
Changes
lib/calculate.tslib/calculate.test.tsWhy this is safe
Current ContributionCalendar has no Date or undefined fields — so
JSON.parse/stringify was not silently dropping data today. But the
refactor prevents a future silent bug if schema changes add such fields.
All 5 new tests verify the deep clone contract is correctly maintained.
Checklist before requesting a review:
CONTRIBUTING.mdfile.npm run formatandnpm run lintlocally and resolved all errors.README.mdif I added a new theme or URL parameter.