-
Notifications
You must be signed in to change notification settings - Fork 23
fix: maximum updates Portal.tsx #29
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
Conversation
Fix for the following error:
error-boundary-callbacks.ts:80 Error: Maximum update depth exceeded. This can happen when a component repeatedly calls setState inside componentWillUpdate or componentDidUpdate. React limits the number of nested updates to prevent infinite loops.
at getRootForUpdatedFiber (react-dom-client.development.js:3860:11)
at enqueueConcurrentHookUpdate (react-dom-client.development.js:3820:14)
at dispatchSetStateInternal (react-dom-client.development.js:8121:18)
at dispatchSetState (react-dom-client.development.js:8081:7)
at Portal.useEffect (Portal.js:63:5)
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Walkthrough将 Portal 中对 innerContainer 的状态更新改为函数式更新器:用 Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 诗歌
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 |
Summary of ChangesHello @James-Jager, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a critical React error, 'Maximum update depth exceeded,' by refining the state management within the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request fixes a 'Maximum update depth exceeded' error in the Portal component by preventing unnecessary state updates. The change is correct and resolves the immediate issue. My review points out that the underlying cause is a useEffect hook missing a dependency array, which causes it to run on every render. I've suggested adding the dependency to improve efficiency and prevent similar issues in the future.
Missed dependency
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #29 +/- ##
==========================================
+ Coverage 93.27% 94.28% +1.00%
==========================================
Files 6 7 +1
Lines 119 140 +21
Branches 44 48 +4
==========================================
+ Hits 111 132 +21
Misses 8 8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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 PR addresses a "Maximum update depth exceeded" error in the Portal component by preventing unnecessary state updates when the container hasn't changed. The fix introduces a comparison check using a functional state update and adds a dependency array to the useEffect hook.
Key Changes
- Modified
setInnerContainerto use a functional update with a comparison check to avoid cascading updates - Added dependency array
[getContainer]to ensure the effect re-runs when the container source changes - Prevents infinite update loops by returning the previous state when the container is unchanged
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Anything else you would like me to do? |
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
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/Portal.tsx
Outdated
|
|
||
| return nextContainer; | ||
| }); | ||
| }, [getContainer]); |
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.
wait, this may be break the closure:
const getContainer = useCallback(() => myRef.current)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.
Good catch didn't even think about this. Letting it run on every render is safe now because of the prev === nextContainer preventing unnecessary updates.
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.
Do you think we should also change from useEffect to useLayoutEffect to run synchronously after DOM mutations?
If getContainer is a stable useCallback(() => myRef.current), the effect won't re-run when myRef.current changes because the function reference stays the same.
|
I approve the PR, but I think just return in function call is enough. |
Fix for the following error:
error-boundary-callbacks.ts:80 Error: Maximum update depth exceeded. This can happen when a component repeatedly calls setState inside componentWillUpdate or componentDidUpdate. React limits the number of nested updates to prevent infinite loops.
at getRootForUpdatedFiber (react-dom-client.development.js:3860:11)
at enqueueConcurrentHookUpdate (react-dom-client.development.js:3820:14)
at dispatchSetStateInternal (react-dom-client.development.js:8121:18)
at dispatchSetState (react-dom-client.development.js:8081:7)
at Portal.useEffect (Portal.js:63:5)
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.