-
Notifications
You must be signed in to change notification settings - Fork 3.8k
[#noissue] Apply memoization to UrlSummaryColumn #13303
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[#noissue] Apply memoization to UrlSummaryColumn #13303
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request applies memoization to the UrlSummaryFetcher component to optimize the creation of table columns. The change wraps the summaryColumns call with React.useMemo to prevent unnecessary recalculation of column definitions on every render.
Changes:
- Wrapped the
summaryColumnscall withReact.useMemoto memoize column definitions based onorderByandisDescdependencies
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const columns = React.useMemo( | ||
| () => | ||
| summaryColumns({ | ||
| orderBy, | ||
| isDesc, | ||
| onClickColumnHeader: (key) => { | ||
| if (orderBy === key) { | ||
| setIsDesc(!isDesc); | ||
| } else { | ||
| setIsDesc(true); | ||
| } | ||
|
|
||
| setOrderBy(key); | ||
| }, | ||
| }); | ||
| setOrderBy(key); | ||
| }, | ||
| }), | ||
| [orderBy, isDesc], | ||
| ); |
Copilot
AI
Jan 19, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The memoization implementation has a stale closure issue. The callback function onClickColumnHeader references isDesc in the expression setIsDesc(!isDesc) on line 33, but the memoization only depends on [orderBy, isDesc]. When the callback is invoked after the memoization has been skipped, it will use a stale value of isDesc. To fix this, use the functional update form: setIsDesc((prev) => !prev) instead of setIsDesc(!isDesc). This ensures the callback always operates on the current state value without requiring isDesc in the closure.
1bb086f to
00c3e3f
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #13303 +/- ##
============================================
- Coverage 33.18% 33.18% -0.01%
+ Complexity 10975 10973 -2
============================================
Files 4070 4070
Lines 94432 94432
Branches 9830 9830
============================================
- Hits 31340 31338 -2
Misses 60420 60420
- Partials 2672 2674 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
No description provided.