Skip to content

fix: skip the container update when it is unchanged - #46

Open
coldsmirk wants to merge 1 commit into
react-component:masterfrom
coldsmirk:fix/skip-unchanged-container-update
Open

coldsmirk wants to merge 1 commit into
react-component:masterfrom
coldsmirk:fix/skip-unchanged-container-update

Conversation

@coldsmirk

@coldsmirk coldsmirk commented Sep 28, 2026 •

Copy link
Copy Markdown

Fixes #27 — "Maximum update depth exceeded" still reproduces on 2.2.1.

Problem

Portal re-resolves its container in an effect that runs after every render and calls setInnerContainer unconditionally. #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 === NoLanes in dispatchSetState). 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 / Drawer and 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 throw Maximum update depth exceeded from Portal.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 antd Modal, after a confirm dialog had been opened over it.

Change

The effect still re-resolves the container after every render — a stable getContainer may close over a ref whose element changes, the reason given in #27 for keeping it dependency-less — but it now calls setInnerContainer only 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 render counts the portal's render function executions (via the forwardRef object's render) 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.
  • tsc and eslint report nothing new; the existing react-hooks/exhaustive-deps warning on this effect predates the change.
  • ref-able › not support fragment fails on current master with and without this change (React 19.2's experimental fragment refs); it is unrelated.

Summary by CodeRabbit

  • 修复
    • Portal 在容器目标切换时会将内容更新到新容器,避免不必要的重复渲染。

Copilot AI lite review requested due to automatic review settings September 28, 2026 10:40
@coderabbitai

coderabbitai Bot commented Sep 28, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

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 configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 3c3f3197-14de-4347-b06b-4f652b29631b

📥 Commits

Reviewing files that changed from the base of the PR and between 4f29120 and 13e4682.

📒 Files selected for processing (2)
  • src/Portal.tsx
  • tests/index.test.tsx

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.


Walkthrough

Portal 现在只在解析出的容器与当前容器不同时更新状态。新增测试验证容器切换后内容位于新容器中,且 Portal 共渲染两次。

Changes

Portal 容器更新

Layer / File(s) Summary
条件式容器更新
src/Portal.tsx, tests/index.test.tsx
effect 将空容器规范为 null,并仅在解析结果与当前容器不同时更新状态。测试验证目标容器切换后内容出现在新节点中,且 Portal 共渲染两次。

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to 13e46

The change avoids redundant Portal updates while preserving container switching. No actionable merge-blocking risk remains.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed 标题准确概括了主要变更:当容器未变化时跳过状态更新。标题简洁、明确,并与 PR 内容一致。
Linked Issues check ✅ Passed PR 实现了 Issue #27 的编码目标。src/Portal.tsx 在每次 effect 中重新解析 getContainer,但仅在解析结果与 innerContainer 不同时调用 setInnerContainer,因此相同容器不会触发状态更新。tests/index.test.tsx 新增容器切换测试。测试验证内容移动到新容器,并验证切换仅产生两次 Porta…
Out of Scope Changes check ✅ Passed 变更范围与 Issue #27 一致。src/Portal.tsx 修改容器状态更新逻辑,tests/index.test.tsx 增加对应回归测试。未发现与该问题无关的功能、公共 API 或其他模块变更。
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2…
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

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.

❤️ Share

我是小兔,蹦到新容器旁,
看 Portal 把内容安稳搬迁。
目标未变,状态不再更新,
目标切换,内容随之出现。
两次渲染,测试记得清晰,
我竖起耳朵,为这改动欢喜。

Comment @coderabbitai help to get the list of available commands.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Uncaught Error: Maximum update depth exceeded.

2 participants