Steve/v3-control-node-bug #4743
Conversation
Walkthrough本次变更重构了 runtime 渲染树的节点结构，将原有的 PLACEHOLDER 节点统一为 ABSTRACT 节点，简化了类型体系，并移除了渲染节点的跟踪与缓存机制。渲染逻辑和挂载流程同步调整以适配 ABSTRACT 节点。相关测试、类型定义和部分工具函数也进行了相应修改，并补充了 E2E 场景和 Storyboard 路由配置。 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/brick-container/scripts/start.jsOops! 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/react-runtime/src/ReactUseBrick.tsxOops! 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/Renderer.spec.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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v3 #4743 +/- ##
==========================================
- Coverage 95.26% 95.23% -0.03%
==========================================
Files 209 209
Lines 9060 9066 +6
Branches 1744 1738 -6
==========================================
+ Hits 8631 8634 +3
- Misses 319 321 +2
- Partials 110 111 +1
🚀 New features to boost your workflow:
|
next-core
|
||||||||||||||||||||||||||||
| Project |
next-core
|
| Branch Review |
steve/v3-control-node-bug
|
| Run status |
|
| Run duration | 00m 22s |
| 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 ↗︎ | |
weareoutman
force-pushed
the
steve/v3-control-node-bug
branch
4 times, most recently
from
c76b40a to
84558a4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
weareoutman
force-pushed
the
steve/v3-control-node-bug
branch
2 times, most recently
from
01bf1d1 to
58b37b3
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
weareoutman
force-pushed
the
steve/v3-control-node-bug
branch
from
58b37b3 to
af760cf
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/runtime/src/internal/RendererContext.ts (1)
803-809: getEntityReturnNode 函数存在栈安全性问题这个递归函数在深度嵌套的抽象节点中可能导致栈溢出。
建议改为迭代实现以避免栈溢出：
-function getEntityReturnNode(node: RenderReturnNode): RenderRoot | RenderBrick { - let current = node; - while (current.tag === RenderTag.ABSTRACT) { - current = current.return; - } - return current; -} +function getEntityReturnNode(node: RenderReturnNode): RenderRoot | RenderBrick { + let current = node; + while (current.tag === RenderTag.ABSTRACT && current.return) { + current = current.return; + } + return current as RenderRoot | RenderBrick; +}
🧹 Nitpick comments (2)
packages/runtime/src/internal/Renderer.ts (1)
274-276: 使用 forEach 替代传统 for 循环代码更简洁，但在性能敏感的渲染路径中，传统 for 循环通常更快。
考虑在性能敏感的渲染代码中保持传统 for 循环：
- rendered.forEach((item) => { - mergeRenderOutput(output, item); - }); + for (const item of rendered) { + mergeRenderOutput(output, item); + }packages/runtime/src/internal/RendererContext.ts (1)
680-727: findLastChildNodes 函数实现合理该函数正确处理了抽象节点的遍历，区分普通节点和 portal 节点。逻辑复杂但看起来正确。
考虑添加更详细的注释说明复杂的遍历逻辑：
+ // 遍历给定节点及其子节点，查找最后的普通和 portal 节点 + // level 用于跟踪抽象节点的嵌套深度，确保不会超出给定节点的范围 let current: RenderChildNode | undefined = node; let level = 0;
📜 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 (12)
mock-micro-apps/e2e/storyboard.yaml(1 hunks)packages/brick-container/scripts/start.js(1 hunks)packages/react-runtime/src/ReactUseBrick.tsx(1 hunks)packages/runtime/src/internal/Renderer.spec.ts(5 hunks)packages/runtime/src/internal/Renderer.ts(17 hunks)packages/runtime/src/internal/RendererContext.ts(7 hunks)packages/runtime/src/internal/Runtime.spec.ts(1 hunks)packages/runtime/src/internal/enums.ts(1 hunks)packages/runtime/src/internal/interfaces.ts(2 hunks)packages/runtime/src/internal/mount.ts(3 hunks)packages/runtime/src/internal/secret_internals.spec.ts(1 hunks)packages/runtime/src/internal/secret_internals.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- packages/brick-container/scripts/start.js
- packages/runtime/src/internal/enums.ts
- packages/runtime/src/internal/Runtime.spec.ts
- packages/runtime/src/internal/secret_internals.spec.ts
- packages/react-runtime/src/ReactUseBrick.tsx
- packages/runtime/src/internal/mount.ts
- packages/runtime/src/internal/secret_internals.ts
- packages/runtime/src/internal/interfaces.ts
- packages/runtime/src/internal/Renderer.spec.ts
- mock-micro-apps/e2e/storyboard.yaml
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/runtime/src/internal/RendererContext.ts (1)
packages/runtime/src/internal/interfaces.ts (5)
RenderReturnNode(81-81)RenderChildNode(80-80)RenderBrick(61-64)RenderNode(79-79)RenderRoot(52-59)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build (20.x)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (15)
packages/runtime/src/internal/Renderer.ts (8)
51-51: 导入类型更新正确将
RenderPlaceholder更新为RenderAbstract与整体重构保持一致。
259-272: 移除 keyPath 参数简化了函数调用参数简化有助于降低复杂性，与移除记忆化缓存的重构目标一致。
465-470: 控制节点包装逻辑实现正确新的
RenderAbstract节点包装机制正确实现了控制节点的统一处理。
619-624: reRender 调用签名更新正确新的参数顺序
(returnNode, newNode, oldNode)更直观，与 RendererContext 中的实现保持一致。
902-906: 增量渲染中的抽象节点创建在增量渲染场景中正确创建了
RenderAbstract节点，保持了架构一致性。Also applies to: 954-960
1017-1019: 增量渲染的重新渲染调用更新后的
reRender调用签名在增量渲染场景中使用正确。
1125-1143: forEach 渲染逻辑保持一致
renderForEach函数中的更新与整体架构变更保持一致，forEach 循环改为更简洁的形式。Also applies to: 1148-1150
473-474: 请手动验证复杂嵌套场景下控制节点渲染逻辑的测试覆盖情况自动扫描未发现针对嵌套
<if>/<for>/<switch>场景的测试用例，建议在以下位置确认或补充测试：
- packages/runtime/src/internal/Renderer.ts
- Lines 473–474
- Lines 481
- Lines 483–495
- Lines 496
- Lines 499–510
- Lines 513–518
可使用以下脚本快速定位现有测试文件并检查是否覆盖到嵌套场景：
#!/bin/bash # 查找测试文件中涉及嵌套控制节点的测试用例 rg -l -e '<if' -e '<for' -e '<switch' --glob '*.{spec,test}.ts'packages/runtime/src/internal/RendererContext.ts (7)
378-382: reRender 方法签名简化合理新的签名
(returnNode, node, oldNode)比原来的(slotId, keyPath, node, returnNode)更直观，移除了复杂的 keyPath 跟踪。
408-414: 前置兄弟节点查找逻辑正确直接遍历查找
oldNode的前置兄弟节点，逻辑简单清晰。
433-434: 使用新的 getContainedBricks 函数替换了原有的
getBrickRange函数，新函数逻辑更清晰。Also applies to: 489-490
444-444: 简化兄弟节点链接直接赋值
node.sibling = oldNode.sibling比之前的复杂逻辑更简洁。
454-459: 抽象节点的实体返回节点解析使用
getEntityReturnNode跳过抽象节点找到实际的容器节点，逻辑正确。
729-769: findNextSiblingNode 函数逻辑正确该函数正确实现了在 DOM 中查找下一个兄弟节点的逻辑，恰当处理了抽象节点的跳过。
771-801: getContainedBricks 函数实现正确递归收集节点内包含的所有 brick，终止条件设置合理。
weareoutman
force-pushed
the
steve/v3-control-node-bug
branch
from
af760cf to
c2d3e93
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/runtime/src/internal/RendererContext.ts (2)
383-403: 新的树遍历逻辑替代记忆化缓存使用直接树遍历替代记忆化缓存是重大架构变更。逻辑看起来正确，但在复杂的 DOM 结构中需要验证性能影响。
803-809: getEntityReturnNode 递归函数的栈安全性这是一个递归函数，在深度嵌套的抽象节点中可能导致栈溢出。
🧹 Nitpick comments (3)
packages/runtime/src/internal/RendererContext.ts (3)
680-727: findLastChildNodes 函数复杂度较高此函数实现了复杂的树遍历逻辑来查找最后的子节点。逻辑正确但相当复杂，需要确保边界情况处理得当。
考虑为复杂的遍历逻辑添加详细的单元测试，特别是嵌套抽象节点的场景：
// 建议添加测试用例 describe('findLastChildNodes', () => { it('should handle deeply nested abstract nodes', () => { // 测试深度嵌套的抽象节点 }); it('should correctly distinguish normal and portal nodes', () => { // 测试正常节点和门户节点的区分 }); it('should handle mixed node types', () => { // 测试混合节点类型 }); });
729-769: findNextSiblingNode 函数逻辑复杂此函数在查找下一个兄弟节点时需要处理抽象节点的遍历。实现正确但逻辑复杂，建议加强测试覆盖。
建议添加边界情况的单元测试，确保在各种节点结构下都能正确工作。
771-801: getContainedBricks 函数需要优化边界条件函数在遍历包含的 brick 节点时有多个终止条件。逻辑看起来正确，但需要确保所有边界情况都被正确处理。
建议添加断言或日志来帮助调试，特别是在复杂的节点层次结构中：
function getContainedBricks(node: RenderChildNode) { const range = new Set<RenderBrick>(); let current: RenderNode | undefined = node; let depth = 0; // 添加深度跟踪用于调试 while (current) { // 添加深度限制防止无限循环 if (depth > 1000) { console.warn('getContainedBricks: Maximum depth exceeded'); break; } // ... 现有逻辑 } return range; }
📜 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 (12)
mock-micro-apps/e2e/storyboard.yaml(1 hunks)packages/brick-container/scripts/start.js(1 hunks)packages/react-runtime/src/ReactUseBrick.tsx(1 hunks)packages/runtime/src/internal/Renderer.spec.ts(5 hunks)packages/runtime/src/internal/Renderer.ts(17 hunks)packages/runtime/src/internal/RendererContext.ts(7 hunks)packages/runtime/src/internal/Runtime.spec.ts(1 hunks)packages/runtime/src/internal/enums.ts(1 hunks)packages/runtime/src/internal/interfaces.ts(2 hunks)packages/runtime/src/internal/mount.ts(2 hunks)packages/runtime/src/internal/secret_internals.spec.ts(1 hunks)packages/runtime/src/internal/secret_internals.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- packages/react-runtime/src/ReactUseBrick.tsx
- packages/runtime/src/internal/Runtime.spec.ts
- packages/brick-container/scripts/start.js
- packages/runtime/src/internal/enums.ts
- packages/runtime/src/internal/mount.ts
- packages/runtime/src/internal/secret_internals.spec.ts
- packages/runtime/src/internal/secret_internals.ts
- mock-micro-apps/e2e/storyboard.yaml
- packages/runtime/src/internal/Renderer.spec.ts
- packages/runtime/src/internal/interfaces.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/runtime/src/internal/RendererContext.ts (1)
packages/runtime/src/internal/interfaces.ts (5)
RenderReturnNode(81-81)RenderChildNode(80-80)RenderBrick(61-64)RenderNode(79-79)RenderRoot(52-59)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build (20.x)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (7)
packages/runtime/src/internal/Renderer.ts (6)
51-51: 导入更新符合重构需求从
RenderPlaceholder更新为RenderAbstract的导入与整体重构保持一致。
259-272: 简化渲染函数参数，移除跟踪逻辑移除
keyPath参数和相关跟踪逻辑，简化了函数签名。更改与整体架构重构目标一致。
619-624: reRender 调用参数顺序更新
reRender方法的参数顺序已从(slotId, keyPath, node, returnNode)更改为(returnNode, node, oldNode)。更改与 RendererContext 中的新签名一致。
901-906: 路由渲染中的抽象节点创建在增量子路由渲染中创建
RenderAbstract控制节点是正确的。这确保了路由渲染与控制节点渲染使用相同的抽象节点模式。Also applies to: 954-959
1125-1148: renderForEach 函数简化移除
keyPath参数并简化结果合并逻辑。更改与整体简化目标一致。
466-470: 确认 RenderAbstract 节点处理一致性经验证，核心遍历和挂载逻辑在以下位置均正确跳过并处理了 RenderTag.ABSTRACT 节点，相关接口与测试覆盖也已到位，新增的 controlNode 构建逻辑符合预期，无需额外调整。
需要关注的文件：
- packages/runtime/src/internal/RendererContext.ts：遍历时正确下钻并跳过抽象节点
- packages/runtime/src/internal/mount.ts：跳过 RenderTag.ABSTRACT 后，将子元素挂载到 ROOT 或对应元素
- packages/runtime/src/internal/interfaces.ts：RenderAbstract 接口已正确定义
- packages/runtime/src/internal/Renderer.ts：controlNode 构建逻辑清晰
- packages/runtime/src/internal/Renderer.spec.ts：测试已覆盖抽象节点场景
packages/runtime/src/internal/RendererContext.ts (1)
378-382: reRender 方法签名重大更改方法签名从基于记忆化的参数更改为直接的节点参数。这是架构重构的核心部分，简化了接口但需要新的树遍历逻辑。
There was a problem hiding this comment.
Pull Request Overview
This pull request fixes a bug related to control node handling and improves the rendering logic for nested and dynamic nodes in the runtime. Key changes include modifications to node type handling (e.g. replacing "PLACEHOLDER" with "ABSTRACT"), refactoring of the re-render and memoization logic in the renderer context, and adjustments in test cases and mounting behaviors to support the new control node structure.
Reviewed Changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/runtime/src/internal/secret_internals.ts | Revamped the brick node detection logic in renderUseBrick |
| packages/runtime/src/internal/RendererContext.ts | Refactored reRender parameter order and child node traversal logic |
| packages/react-runtime/src/ReactUseBrick.tsx | Modified type cast for tagName in the React useBrick component |
| Other files | Various updates to enums, interfaces, tests, mounts, and scripts |
Comments suppressed due to low confidence (2)
packages/runtime/src/internal/RendererContext.ts:382
- The parameter order for reRender has been changed compared to the previous implementation; please verify all invocations throughout the codebase are updated accordingly to avoid parameter mismatches.
) {
packages/runtime/src/internal/secret_internals.ts:112
- When traversing to determine the brick node, if no node with tag BRICK is found then tagName becomes null while renderRoot.child remains the original node, possibly leading to unexpected behavior. Confirm that this fallback behavior is intended and does not inadvertently bypass required node validations.
let brickNode: RenderBrick | undefined;
| export interface RenderPlaceholder extends BaseRenderNode { | ||
| tag: RenderTag.PLACEHOLDER; | ||
| export interface RenderAbstract extends BaseRenderNode { | ||
| tag: RenderTag.ABSTRACT; |
There was a problem hiding this comment.
Now, we will always generate abstract nodes for control nodes and routes nodes in bricks
依赖检查
组件之间的依赖声明，是微服务组件架构下的重要信息，请确保其正确性。
请勾选以下两组选项其中之一：
或者：
提交信息检查
Git 提交信息将决定包的版本发布及自动生成的 CHANGELOG，请检查工作内容与提交信息是否相符，并在以下每组选项中都依次确认。
破坏性变更：
feat作为提交类型。BREAKING CHANGE: 你的变更说明。新特性：
feat作为提交类型。问题修复：
fix作为提交类型。杂项工作：
即所有对下游使用者无任何影响、且没有必要显示在 CHANGELOG 中的改动，例如修改注释、测试用例、开发文档等：
chore,docs,test等作为提交类型。Summary by CodeRabbit
新功能
修复
测试
其他