fix(): dot not re-render controlled nodes before mounted #4749
Conversation
Walkthrough本次更改在渲染节点的生命周期管理中引入了 Changes
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
packages/runtime/src/internal/Renderer.tsOops! Something went wrong! :( ESLint: 9.29.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find package '@next-core/eslint-config-next' imported from /eslint.config.mjs packages/runtime/src/internal/interfaces.tsOops! Something went wrong! :( ESLint: 9.29.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find package '@next-core/eslint-config-next' imported from /eslint.config.mjs packages/runtime/src/internal/mount.tsOops! Something went wrong! :( ESLint: 9.29.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find package '@next-core/eslint-config-next' imported from /eslint.config.mjs ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
This PR fixes an issue where controlled nodes could be re-rendered before they were marked as mounted by:
- Introducing a
mountedflag onBaseRenderNodeand setting it on the root and each child inmountTree. - Moving the
disposedflag toBaseRenderNodeand updating the interface accordingly. - Updating the listener in
legacyRenderBrickto skip re-renders if a node is not yet mounted or has been disposed.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/runtime/src/internal/mount.ts | Set mounted flag on root and each current node in mountTree |
| packages/runtime/src/internal/interfaces.ts | Moved disposed into BaseRenderNode and added mounted |
| packages/runtime/src/internal/Renderer.ts | Guarded re-render listener with returnNode.mounted and !disposed |
Comments suppressed due to low confidence (3)
packages/runtime/src/internal/mount.ts:20
- Add unit tests to verify that
mountTreecorrectly sets themountedflag on the root and child nodes, and that controlled nodes are not re-rendered before mounting.
root.mounted = true;
packages/runtime/src/internal/interfaces.ts:79
- [nitpick] Consider renaming boolean properties
disposedandmountedtoisDisposedandisMountedto make their boolean nature explicit and improve readability.
disposed?: boolean;
packages/runtime/src/internal/Renderer.ts:601
- Requiring
returnNode.mountedfor the root or other nodes may prevent intended re-renders beforemountTreeruns. If this gating is intentional only for non-root nodes, consider excludingRenderTag.ROOThere or documenting why the root also needsmounted.
if (!returnNode.mounted || returnNode.disposed) {
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
packages/runtime/src/internal/Renderer.ts(2 hunks)packages/runtime/src/internal/interfaces.ts(1 hunks)packages/runtime/src/internal/mount.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/runtime/src/internal/mount.ts (1)
packages/runtime/src/internal/interfaces.ts (1)
RuntimeBrickElement(111-122)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build (20.x)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
packages/runtime/src/internal/mount.ts (2)
20-20: 正确设置根节点的挂载状态在
mountTree函数开始时设置root.mounted = true是正确的做法，确保根节点的挂载状态得到正确跟踪。
25-25: 正确设置节点挂载状态在遍历过程中为每个节点设置
current.mounted = true能确保所有节点在挂载期间都有正确的生命周期状态标记。packages/runtime/src/internal/Renderer.ts (1)
619-620: 改进了过期渲染的检查逻辑将过期渲染检查条件更新为
returnNode.mounted && !returnNode.disposed确保只有在节点已挂载且未销毁的情况下才继续处理，这比之前的条件更加严格和准确。packages/runtime/src/internal/interfaces.ts (1)
79-80: 正确地将生命周期属性集中到基础接口将
disposed和mounted属性添加到BaseRenderNode接口是一个很好的设计决策，这样所有继承自该接口的渲染节点类型都能统一管理生命周期状态。将属性定义为可选的（
?:）是正确的，因为节点在初始状态下是未挂载的。
| // TODO(steve): start listeners when mounting, and handle changes between rendering and mounting. | ||
| if (!returnNode.mounted || returnNode.disposed) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
添加了必要的生命周期保护逻辑
在监听器函数中添加的保护条件 !returnNode.mounted || returnNode.disposed 能正确防止对未挂载或已销毁的节点进行处理，这是一个重要的防护措施。
TODO 注释正确识别了渲染和挂载之间可能存在的时序问题。
考虑实现更精细的生命周期管理来处理渲染和挂载之间的状态变化，可以通过以下方式验证当前实现是否存在竞态条件：
🏁 Script executed:
#!/bin/bash
# 搜索 mounted 属性的使用模式，检查是否存在其他需要同步处理的地方
rg -A 3 -B 3 "\.mounted" --type tsLength of output: 1888
改进渲染监听器的启动时机以消除渲染与挂载之间的竞态
当前在 Renderer.ts 中直接启动监听器（listener），即便加了 guard 条件，仍可能与 mount.ts 的挂载流程产生时序冲突。建议将 listener 的注册／启动延后到所有节点的 mounted 标志设置完毕之后，或在挂载流程中统一启动。
需要关注的代码位置：
- packages/runtime/src/internal/Renderer.ts（约第596–603行）：listener 定义与立即启动处
- packages/runtime/src/internal/mount.ts（约第3–7行）：
root.mounted = true及current.mounted = true赋值处
示例改进思路：
--- Renderer.ts
- listener(); // 立即启动会与挂载流程竞态
+ // 不在这里直接启动 listener，而在挂载完成后统一注册// mount.ts 挂载流程末尾
root.mounted = true;
for (let node of allNodes) {
node.mounted = true;
}
// 挂载完成后再启动渲染监听器
renderer.registerListener(listener);请根据项目整体生命周期管理方案，将 listener 启动逻辑移至挂载完成位置。
🤖 Prompt for AI Agents
In packages/runtime/src/internal/Renderer.ts around lines 596 to 603, the
listener is currently started immediately during rendering, which can cause race
conditions with the mounting process in mount.ts where nodes' mounted flags are
set around lines 3 to 7. To fix this, remove the listener startup from
Renderer.ts and instead move the listener registration and startup to the end of
the mounting process in mount.ts, after all nodes have their mounted flags set
to true, ensuring listeners start only after mounting is fully complete.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v3 #4749 +/- ##
==========================================
- Coverage 95.24% 95.22% -0.02%
==========================================
Files 209 209
Lines 9140 9144 +4
Branches 1759 1760 +1
==========================================
+ Hits 8705 8707 +2
- Misses 322 323 +1
- Partials 113 114 +1
🚀 New features to boost your workflow:
|
next-core
|
||||||||||||||||||||||||||||
| Project |
next-core
|
| Branch Review |
steve/v3-fix-control-bug
|
| Run status |
|
| Run duration | 00m 23s |
| Commit |
|
| Committer | Shenwei Wang |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
17
|
| View all changes introduced in this branch ↗︎ | |
依赖检查
组件之间的依赖声明，是微服务组件架构下的重要信息，请确保其正确性。
请勾选以下两组选项其中之一：
或者：
提交信息检查
Git 提交信息将决定包的版本发布及自动生成的 CHANGELOG，请检查工作内容与提交信息是否相符，并在以下每组选项中都依次确认。
破坏性变更：
feat作为提交类型。BREAKING CHANGE: 你的变更说明。新特性：
feat作为提交类型。问题修复：
fix作为提交类型。杂项工作：
即所有对下游使用者无任何影响、且没有必要显示在 CHANGELOG 中的改动，例如修改注释、测试用例、开发文档等：
chore,docs,test等作为提交类型。Summary by CodeRabbit
修复问题
其他改进