fix(badges): sort a copy in BadgesRow instead of mutating the prop#2435
fix(badges): sort a copy in BadgesRow instead of mutating the prop#2435innolope-dev wants to merge 1 commit into
Conversation
Array.prototype.sort is in-place, so the sortedBadges useMemo mutated the caller-owned badges array during render. Sort a copy so the memo callback is pure and safe under concurrent/StrictMode double-invocation. No user-visible change today: the only caller passes locally-owned state that is repopulated per mount, and the resulting order is the desired one anyway.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
ChangesBadge rendering
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed: dependency version conflict. Check your lock file or package.json. 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 |
Code-analysis diffPainscore total: 6206.68 → 6206.82 (+0.14) 🆕 New findings (1)
✅ Resolved (1)
|
🧪 UI test report — ✅ all greenSuites
📊 Coverage (unit)
⏱ 10 slowest test cases
|
kushagrasarathe
left a comment
There was a problem hiding this comment.
Approved. Correct fix for the impure useMemo (in-place Array.sort mutating a prop during render). Copy-then-sort is exactly right; in-place-sort audit spot-checked and clean. Regression test verified meaningful (fails pre-fix).
What
BadgesRow'ssortedBadgesuseMemocalledbadges.sort(...).Array.prototype.sortsorts in place, so the memo mutated the caller-ownedbadgesarray during render and returned the same reference. Sort a copy instead.Severity — deliberately not oversold
There is most likely no user-visible symptom today. The only caller is
PublicProfile.tsx, which passesprofileBadges— a localuseStatearray populated fresh fromusersApi.getByUsername(...)on mount. The mutation hits a locally-owned array that no other consumer reads, and the resulting order is the desired one anyway.It's worth fixing as correctness/robustness, not as a user-facing bug fix:
useMemocallback must be pure. Mutating during render is what React's concurrent rendering rules forbid.user.badgesfrom the auth context), this silently reorders shared state.Test
Added
src/components/Badges/__tests__/BadgesRow.test.tsx, asserting the input array is not mutated. It fails before the fix (the caller's array comes back reordered toNEWEST, MIDDLE, OLDEST) and passes after — not a regression test that would pass either way.Audit of other in-place sorts
Swept
srcfor.sort(on arrays the caller doesn't own.BadgesRowwas the only real offender. Everything else either already copies via[...arr]or sorts a fresh array returned from a.map/.filterchain:lib/blog.ts:53,lib/content.ts:298,badgeCelebration.utils.ts:101,dev/leaderboard/page.tsx:53,cardUnlock.types.ts:69map/filterchain, or a locally-built array — safeHomeHistory.tsx:260,history/page.tsx:202entries.sort(...)looks in-place, butentriesis built via[...spread]locally (:158/:166) — safeuseGeoFilteredPaymentOptions,CountryList,InvitesGraph,TokenSelector,shareAssetLayout,useContributePotFlow[...arr].sort(...)Left them as-is rather than churn the diff.
Verification
pnpm typecheck— cleannpx jest src/components/Badges— 27 passed, 5 suitesnpx eslint src/components/Badges— no new errors (the one error,badge.types.ts:11no-explicit-any, is pre-existing; the new test's<img>warning matches the identical existing mock inBadgeEarnToast.test.tsx)prettier --writeon both touched filesFound incidentally during localization work on
feat/app-localization; unrelated to i18n, so branched offmainand kept separate.Summary by CodeRabbit
Bug Fixes
Tests