Uh oh!
There was an error while loading. Please reload this page.
fix: avoid stale async unmount clearing latest React root - #763
fix: avoid stale async unmount clearing latest React root#763li-jia-nan wants to merge 2 commits into
Conversation
The latest updates on your projects. Learn more about Vercel for GitHub.
|
路线图通过添加容器级别的单调递增渲染实例标识符（MARK_ID），防止过期的异步卸载操作误删当前活跃根。render 函数在保存根后递增标识符，unmount 函数捕获并校验标识符匹配才执行卸载。 改动详情异步卸载竞态条件防护
序列图sequenceDiagram
participant App as 应用代码
participant render as render()
participant Container as 容器<br/>(MARK, MARK_ID)
participant unmount as unmount()
participant Microtask as 微任务队列
App->>render: 第一次 render
render->>Container: 存储 Root, MARK_ID=1
App->>unmount: 调用 unmount (捕获 Root, rootId=1)
unmount->>Microtask: 延迟卸载任务
App->>render: 再次 render (容器未卸载)
render->>Container: 存储新 Root, MARK_ID=2
Microtask->>Container: 检查 Root 和 MARK_ID
Container-->>Microtask: MARK_ID=2 ≠ rootId=1
Microtask->>unmount: 校验失败，跳过卸载
代码审查工作量评估🎯 2 (简单) | ⏱️ ~10 分钟 可能相关的 PR
小诗
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/React/render.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-react". (The package "eslint-plugin-react" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-react" was referenced from the config file in ".eslintrc.js » /node_modules/.pnpm/@umijs+fabric@3.0.0/node_modules/@umijs/fabric/dist/eslint.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. 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.
Pull request overview
This PR prevents a delayed (microtask) unmount() call from accidentally unmounting and clearing a React 18 root that has been re-rendered into the same container after unmount() was scheduled.
Changes:
- Add a per-container
MARK_IDcounter to track the “latest” render for a container. - Capture
rootandrootIdatunmount()call time, and only unmount/clear if they still match when the microtask executes.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/React/render.ts (1)
24-35: 💤 Low value竞态条件修复逻辑正确。
通过捕获当前
root和rootId，并在微任务中校验两者是否仍与容器当前值匹配，有效防止了过期的异步卸载操作。逻辑清晰且完整。可选的代码风格改进：既然函数已标记为
async，可以考虑使用await Promise.resolve()替代.then()回调，使代码更简洁：♻️ 可选：使用 async/await 简化代码
export const unmount = async (container: ContainerType) => { const root = container[MARK]; const rootId = container[MARK_ID]; // Delay to unmount to avoid React 18 sync warning - return Promise.resolve().then(() => {- if (container[MARK] === root && container[MARK_ID] === rootId) {- root?.unmount();- delete container[MARK];- delete container[MARK_ID];- }- });+ await Promise.resolve();+ if (container[MARK] === root && container[MARK_ID] === rootId) {+ root?.unmount();+ delete container[MARK];+ delete container[MARK_ID];+ } };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/React/render.ts` around lines 24 - 35, The unmount function uses Promise.resolve().then(...) for a microtask race check; since the function is already async, replace the .then callback with await Promise.resolve() and then perform the same check and cleanup to simplify control flow: inside export const unmount (ContainerType) capture root and rootId as done, await Promise.resolve(), then if (container[MARK] === root && container[MARK_ID] === rootId) call root?.unmount() and delete container[MARK] and container[MARK_ID]; keep the same symbols (unmount, ContainerType, MARK, MARK_ID, root, rootId) and remove the unnecessary explicit return of the Promise chain.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/React/render.ts`:
- Around line 24-35: The unmount function uses Promise.resolve().then(...) for a
microtask race check; since the function is already async, replace the .then
callback with await Promise.resolve() and then perform the same check and
cleanup to simplify control flow: inside export const unmount (ContainerType)
capture root and rootId as done, await Promise.resolve(), then if
(container[MARK] === root && container[MARK_ID] === rootId) call root?.unmount()
and delete container[MARK] and container[MARK_ID]; keep the same symbols
(unmount, ContainerType, MARK, MARK_ID, root, rootId) and remove the unnecessary
explicit return of the Promise chain.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@## master #763 +/- ##
==========================================
+ Coverage 86.42% 86.49% +0.07%
==========================================
Files 39 39 Lines 1068 1074 +6 Branches 388 375 -13 ==========================================
+ Hits 923 929 +6
Misses 143 143 Partials 2 2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Code Review
This pull request introduces a versioning mechanism using MARK_ID to track React roots within containers, ensuring that asynchronous unmounting only occurs if the root has not been replaced by a subsequent render. Feedback suggests optimizing the unmount function by returning early when no root is present to avoid scheduling unnecessary microtasks.
Uh oh!
There was an error while loading. Please reload this page.
| container[MARK]?.unmount(); | ||
| delete container[MARK]; | ||
| if (container[MARK] === root && container[MARK_ID] === rootId) { |
There was a problem hiding this comment.
这个逻辑好像不对。render 多次本来就是可以的，但是 unmount 一次就卸载也是对的。就像 test 里通过 render 多次更新 props，但是清理其实只要跑一次。
增加 MARK_ID，避免 unmount() 的微任务延迟把后续重新 render 的 root 误删。
Summary by CodeRabbit
Bug Fixes