fix: memoize CampaignMap to prevent unnecessary re-initializations - #851
fix: memoize CampaignMap to prevent unnecessary re-initializations#851Peolite1 wants to merge 12 commits into
Conversation
|
@Peolite1 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
|
Auto-review failed (API error). Leaving PR for human review. |
|
Auto-review failed (API error). Leaving PR for human review. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
davidmaronio
left a comment
davidmaronio
left a comment
There was a problem hiding this comment.
thanks for picking this up, memoizing CampaignMap is a reasonable idea since the map re-initializing on unrelated dashboard re-renders is a real cost.
the diff as pushed is incomplete though, and the red CI on this PR is genuine, not the stale-base issue other PRs have:
- src/components/CampaignMap.tsx:122 - the only change is the closing
}becoming});plus a newexport default CampaignMap;, but the declaration at the top is stillexport default function CampaignMap(...). that leaves an unmatched)and a duplicate default export, so typecheck, lint, and the production build all fail on this branch. the top of the file needsimport { memo } from "react";and the declaration changed toconst CampaignMap = memo(function CampaignMap(...) {. - once that compiles, note that
memoonly helps if thecampaignsprop is referentially stable between renders. if callers build the array inline the memo is a no-op; worth checking the call sites or noting it in the PR description. - please rebase onto latest main afterwards so the remaining checks run against the fixed base (#875 landed the base fix).
happy to re-review once the wrap is completed and CI is green.
|
Auto-review failed (API error). Leaving PR for human review. |
1 similar comment
|
Auto-review failed (API error). Leaving PR for human review. |
ce20799 to
0d14cbc
Compare
|
Auto-review failed (API error). Leaving PR for human review. |
|
Auto-review failed (API error). Leaving PR for human review. |
|
Auto-review failed (API error). Leaving PR for human review. |
|
Auto-review failed (API error). Leaving PR for human review. |
|
Auto-review failed (API error). Leaving PR for human review. |
|
Auto-review failed (API error). Leaving PR for human review. |
|
Auto-review failed (API error). Leaving PR for human review. |
|
Auto-review failed (API error). Leaving PR for human review. |
1 similar comment
|
Auto-review failed (API error). Leaving PR for human review. |
761c7f2 to
176676f
Compare
|
Auto-review failed (API error). Leaving PR for human review. |
|
this is verified and approved, but today's merge wave put the branch into conflict before it reached the front of the queue (lots of prs touching the same i18n and test files landed at once). please rebase onto current main and push, i'll merge as soon as it's green, no re-review needed. |
4 participants
Summary
Wrapped the
CampaignMapcomponent inReact.memoto prevent expensive LeafletMapContainerre-initializations on every parent re-render. A manual verification note has been added to confirm that re-renders don't reinitialize the map unless thecampaignsprop changes its reference.Closes #830
Type of Change
Contributor Checklist
CONTRIBUTING.mdfor branch, commit, and PR title conventions.Validation
npm run lintnpm run format:checknpm run typechecknpm testnpm run buildReact.memowrapper and comment update; can be verified via manual inspection.Notes for Reviewers
CampaignMapcomponent now usesReact.memoto shallow compare thecampaignsprop. This prevents Leaflet from performing an expensive teardown and rebuild whenever unrelated state (like filter chips) changes in the parent component on/explore.