fix: cancel stale scroll retry sync#1498
Conversation
|
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: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
Walkthrough在 ChangesforceScroll 重试定时器修复
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
React Doctor found 9 issues in 2 files · 9 warnings · score 77 / 100 (Needs work) · vs 9 warnings
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| const [setScrollTarget, getScrollTarget] = useTimeoutLock(null); | ||
| const scrollRetryTimeoutRef = React.useRef( | ||
| new Map<HTMLDivElement, ReturnType<typeof setTimeout>>(), |
There was a problem hiding this comment.
React Doctor · react-doctor/rerender-lazy-ref-init (warning)
useRef(new Map()) rebuilds this value on every render & throws it away.
Fix → Initialize the ref lazily so expensive values are not rebuilt and discarded on every render.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to cancel stale scrollLeft retry timeouts when the target is already synced, preventing unnecessary scroll updates. It also adds a corresponding unit test to verify this behavior. The review feedback suggests optimizing the initialization of scrollRetryTimeoutRef by using lazy initialization to avoid creating a new Map instance on every render.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const scrollRetryTimeoutRef = React.useRef( | ||
| new Map<HTMLDivElement, ReturnType<typeof setTimeout>>(), | ||
| ); |
There was a problem hiding this comment.
Initializing useRef with new Map() directly in the render path causes a new Map instance to be created on every single render of the Table component, even though useRef only uses the initial value on the first render. Since Table can render frequently (e.g., during scrolling, resizing, or hovering), this leads to unnecessary object allocation and garbage collection overhead.
We can optimize this by using lazy initialization so that the Map is only created once on the initial render.
| const scrollRetryTimeoutRef = React.useRef( | |
| new Map<HTMLDivElement, ReturnType<typeof setTimeout>>(), | |
| ); | |
| const scrollRetryTimeoutRef = React.useRef<Map<HTMLDivElement, ReturnType<typeof setTimeout>>>(null); | |
| if (!scrollRetryTimeoutRef.current) { | |
| scrollRetryTimeoutRef.current = new Map(); | |
| } |
❌ Deploy failed
📋 Build log (last lines)🤖 Powered by surge-preview |
|||||||||
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1498 +/- ##
=======================================
Coverage 99.04% 99.05%
=======================================
Files 45 45
Lines 1362 1369 +7
Branches 409 410 +1
=======================================
+ Hits 1349 1356 +7
Misses 13 13 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
zombieJ
force-pushed
the
agent/clear-scroll-retry-timeout
branch
from
7023930 to
8226a2d
Compare
1 participant
Summary
Related
Follow-up to #1495.
Verification
Summary by CodeRabbit
Bug Fixes
Tests