Repository navigation
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. WalkthroughPortal 现在只在解析出的容器与当前容器不同时更新状态。新增测试验证容器切换后内容位于新容器中,且 Portal 共渲染两次。 ChangesPortal 容器更新
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The change avoids redundant Portal updates while preserving container switching. No actionable merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
No unresolved blocking issues were identified.
Review effort: Lite
Findings: None
What changed in this PR
Updates Portal to skip unchanged container state updates, preventing redundant renders and nested update errors.
Changes:
- Guard container updates with an identity comparison.
- Add regression coverage for container switching render counts.
| File | Description |
|---|---|
src/Portal.tsx |
Skips unchanged container state updates. |
tests/index.test.tsx |
Verifies container switching avoids redundant renders. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Fixes #27 — "Maximum update depth exceeded" still reproduces on 2.2.1.
Problem
Portalre-resolves its container in an effect that runs after every render and callssetInnerContainerunconditionally. #29 switched that call to a functional updater, expecting React to compare the value and skip the update. It does not: whether React can skip a same-value update without scheduling a render depends on the fiber, not on the updater form. React bails out eagerly only while neither the fiber nor its alternate has a pending lane (fiber.lanes === NoLanes && alternate.lanes === NoLanesindispatchSetState). Right after any real update to the portal, the alternate still carries the lane that update was processed in, so the next "no-op" call schedules a render anyway.In isolation that costs one redundant render. In practice it compounds: a component that renders an antd
Modal/Drawerand holds a controlled input's state re-renders the portal on every keystroke, and once the portal fiber has a lingering lane, every keystroke's passive effect schedules another update. React counts these as nested updates, so ~50 fast keystrokes throwMaximum update depth exceededfromPortal.js(the stack in #27). We hit this in production code on@rc-component/portal@2.2.1+ React 19.2: typing quickly into an input inside an antdModal, after a confirm dialog had been opened over it.Change
The effect still re-resolves the container after every render — a stable
getContainermay close over a ref whose element changes, the reason given in #27 for keeping it dependency-less — but it now callssetInnerContaineronly when the resolved container differs from the current state. An unchanged container never schedules an update, so there is nothing left for React to fail to bail out.Test
getContainer › switch container without a redundant rendercounts the portal's render function executions (via theforwardRefobject'srender) while the container switches from one element to another. It asserts the content moves to the new container and that the switch costs exactly two portal renders — the parent's, plus the one storing the new container. Before this change it takes three: the extra one is the redundant update described above (Expected: 2, Received: 3).Verification
rc-test tests/index.test.tsx: the new test passes with the change and fails without it.tscandeslintreport nothing new; the existingreact-hooks/exhaustive-depswarning on this effect predates the change.ref-able › not support fragmentfails on currentmasterwith and without this change (React 19.2's experimental fragment refs); it is unrelated.Summary by CodeRabbit