fix(): support route children #4816
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Walkthrough扩展子元素以同时支持砖块与路由配置，渲染器在构建槽位时区分并检测砖块/路由类型冲突；引入应用级常量（CONSTANTS）求值支持并可通过配置跳过占位符评估；新增/调整相关单元测试与若干工具与服务端监听逻辑。 Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 分钟 需要重点审查的区域：
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 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.
Pull Request Overview
This PR enables routes to be used as children within brick configurations, allowing for more flexible composition of routes and bricks in the same slot hierarchy.
- Extended type definitions to allow
RouteConfin addition toBrickConfin children arrays - Updated
childrenToSlotsfunction to distinguish between brick and route children and create appropriate slot configurations - Added slot conflict detection to prevent mixing bricks and routes in the same slot
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| packages/types/src/manifest.ts | Added slot property to BaseRouteConf and updated BrickConf.children type to accept both BrickConf and RouteConf |
| packages/runtime/src/internal/Renderer.ts | Updated childrenToSlots to handle both brick and route children with type detection and conflict checking; added isRouteConf type guard |
| packages/runtime/src/internal/Renderer.spec.ts | Added test case for slot conflicts and updated existing test to use new children API |
| packages/utils/src/storyboard/replaceUseChildren.ts | Added type assertion for children array to maintain type compatibility |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/runtime/src/internal/Renderer.ts (2)
1320-1334: 建议重构以提高类型安全性当前实现使用了不当的类型转换来绕过 TypeScript 的类型检查：
(slotConf as SlotConfOfBricks)[type as "bricks"].push(child as BrickConf);虽然这在运行时能够工作（因为
type的实际值是 "routes" 或 "bricks"），但这种做法违背了 TypeScript 的最佳实践，存在以下问题：
- 绕过了类型系统的保护
- 如果将来重构类型定义，可能导致运行时错误
- 代码意图不够清晰，难以维护
建议改为显式的条件判断：
- if (hasOwnProperty(newSlots, slot)) { - const slotConf = newSlots[slot]; - if (slotConf.type !== type) { - throw new Error(`Slot "${slot}" conflict between bricks and routes.`); - } - (slotConf as SlotConfOfBricks)[type as "bricks"].push( - child as BrickConf - ); - } else { - newSlots[slot] = { - type: type as "bricks", - [type as "bricks"]: [child as BrickConf], - }; - } + if (hasOwnProperty(newSlots, slot)) { + const slotConf = newSlots[slot]; + if (slotConf.type !== type) { + throw new Error(`Slot "${slot}" conflict between bricks and routes.`); + } + if (type === "bricks") { + (slotConf as SlotConfOfBricks).bricks.push(child as BrickConf); + } else { + (slotConf as SlotConfOfRoutes).routes.push(child as RouteConf); + } + } else { + if (type === "bricks") { + newSlots[slot] = { + type: "bricks", + bricks: [child as BrickConf], + }; + } else { + newSlots[slot] = { + type: "routes", + routes: [child as RouteConf], + }; + } + }这样的实现更加清晰、类型安全，并且更容易维护。
1369-1371: 可选优化：使用in操作符改进类型守卫当前的
isRouteConf函数通过检查属性值来判断类型：return !!(child as RouteConf).path && !(child as BrickConf).brick;这种方式在实践中是可行的，但存在边缘情况（如空字符串会被视为 falsy）。可以使用
in操作符检查属性是否存在，这样更简洁和健壮：function isRouteConf(child: BrickConf | RouteConf): child is RouteConf { - return !!(child as RouteConf).path && !(child as BrickConf).brick; + return "path" in child; }因为
BrickConf没有path属性，只检查path的存在性就足够了。这种方式不依赖属性值，更加可靠。
📜 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 (4)
packages/runtime/src/internal/Renderer.spec.ts(2 hunks)packages/runtime/src/internal/Renderer.ts(3 hunks)packages/types/src/manifest.ts(2 hunks)packages/utils/src/storyboard/replaceUseChildren.ts(1 hunks)
⏰ 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). (3)
- GitHub Check: Agent
- GitHub Check: build (20.x)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (6)
packages/utils/src/storyboard/replaceUseChildren.ts (1)
29-29: 类型转换在当前上下文中是安全的该类型转换将
brick.children强制转换为BrickConf[]，虽然现在 children 的类型是(BrickConf | RouteConf)[]，但在useChildren这个遗留特性的上下文中，routes 不应该出现。该转换由条件Array.isArray(brick.children) && !slots保护，应该是安全的。建议：可以添加注释说明为什么这里只期望 BrickConf，或者添加运行时检查过滤掉可能的 RouteConf。
packages/types/src/manifest.ts (2)
354-355: 新增的 slot 属性支持路由作为子元素为
BaseRouteConf添加可选的slot属性是合理的设计，使得路由在作为构件的子元素时可以指定所属的插槽。
527-527: children 类型扩展支持混合配置将
BrickConf.children的类型从BrickConf[]扩展为(BrickConf | RouteConf)[]是本 PR 的核心变更，使得构件可以包含子路由。这与相关文件中的运行时逻辑变更保持一致。packages/runtime/src/internal/Renderer.spec.ts (2)
477-477: 测试更新反映新的路由子元素支持测试从使用
slots结构改为使用children数组直接包含子路由，正确验证了新功能。
531-576: 新增冲突检测测试覆盖重要场景新增的 "Slot conflict" 测试验证了在同一个插槽中混合使用 bricks 和 routes 时会正确抛出错误。测试覆盖了核心的冲突检测逻辑，确保错误消息符合预期。
packages/runtime/src/internal/Renderer.ts (1)
1299-1299: 函数签名更新支持混合子元素类型将
children参数的类型从BrickConf[] | undefined更新为(BrickConf | RouteConf)[] | undefined，正确反映了新的类型定义。
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v3 #4816 +/- ##
==========================================
- Coverage 95.20% 95.19% -0.02%
==========================================
Files 212 212
Lines 9320 9336 +16
Branches 1793 1798 +5
==========================================
+ Hits 8873 8887 +14
- Misses 328 330 +2
Partials 119 119
🚀 New features to boost your workflow:
|
next-core
|
||||||||||||||||||||||||||||
| Project |
next-core
|
| Branch Review |
steve/v3-route-children
|
| Run status |
|
| Run duration | 00m 24s |
| 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 ↗︎ | |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| (slotConf as SlotConfOfBricks)[type as "bricks"].push( | ||
| child as BrickConf | ||
| ); |
There was a problem hiding this comment.
Type casting issue: When type is "routes", the code attempts to push child as BrickConf into a routes array, which should contain RouteConf elements instead. This will cause type inconsistency.
The line should be:
(slotConf as SlotConfOfBricks | SlotConfOfRoutes)[type].push(child as any);Or handle routes separately:
if (type === "bricks") {
(slotConf as SlotConfOfBricks).bricks.push(child as BrickConf);
} else {
(slotConf as SlotConfOfRoutes).routes.push(child as RouteConf);
}| newSlots[slot] = { | ||
| type: "bricks", | ||
| bricks: [], | ||
| type: type as "bricks", | ||
| [type as "bricks"]: [child as BrickConf], |
There was a problem hiding this comment.
Type casting issue: When creating a new slot with type as "routes", the code casts type to "bricks" and child to BrickConf, which is incorrect. This should handle both cases properly:
newSlots[slot] = {
type,
[type]: [child],
} as SlotConf;Or handle them separately:
if (type === "bricks") {
newSlots[slot] = {
type: "bricks",
bricks: [child as BrickConf],
};
} else {
newSlots[slot] = {
type: "routes",
routes: [child as RouteConf],
};
}| if (Array.isArray(brick.children) && !slots) { | ||
| const removeBricks: BrickConf[] = []; | ||
| for (const child of brick.children) { | ||
| for (const child of brick.children as BrickConf[]) { |
There was a problem hiding this comment.
Type safety concern: The type assertion as BrickConf[] assumes that brick.children only contains BrickConf items, but the type now allows (BrickConf | RouteConf)[]. If a RouteConf is present, it will cause issues at line 41 where replaceInBrick(child) is called, which expects a BrickConf.
Consider adding a runtime check or filter:
for (const child of brick.children.filter((c): c is BrickConf => !!(c as BrickConf).brick)) {Or add a type guard check:
for (const child of brick.children as BrickConf[]) {
if (!(child as BrickConf).brick) continue; // Skip non-bricks| for (const child of brick.children as BrickConf[]) { | |
| for (const child of brick.children.filter((c): c is BrickConf => !!(c as BrickConf).brick)) { |
| globalVariables[variableName] = getReadOnlyProxy(sys ?? {}); | ||
| break; | ||
| case "CONSTANTS": | ||
| // globalVariables[variableName] = getReadOnlyProxy(app?.constants ?? {}); |
There was a problem hiding this comment.
Remove commented-out code. The commented line appears to be an older implementation that was replaced by the dynamic proxy version below. Keeping commented code in the codebase reduces readability.
// Remove this line:
// globalVariables[variableName] = getReadOnlyProxy(app?.constants ?? {});| // globalVariables[variableName] = getReadOnlyProxy(app?.constants ?? {}); |
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 (2)
packages/brick-container/serve/utils/getStoryboards.js(2 hunks)packages/brick-container/serve/utils/liveReloadServer.js(1 hunks)
⏰ 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). (3)
- GitHub Check: Agent
- GitHub Check: build (20.x)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
packages/brick-container/serve/utils/getStoryboards.js (1)
5-5: LGTM: 正确引入了工具函数引入
tryFiles工具函数来处理多位置文件查找，符合代码复用原则。packages/brick-container/serve/utils/liveReloadServer.js (1)
10-14: 现在我需要查看实际的仓库代码来理解getStoryboards.js的完整实现。让我从脚本中获取更多信息：基于已收集的信息，我可以现在生成重写的review comment。关键发现是：
- chokidar支持同时监听多个路径
- throttle(100, ...)会限制回调函数执行频率，即使多个文件改变，在100ms内也最多执行一次
- Review comment中关于"如果两个文件在不同时间被修改，仍会触发多次reload"的表述需要澄清
同时监听两个 storyboard.yaml 位置的设计确认
该代码变更监听了
mock-micro-apps和apps两个文件夹中每个 appId 的 storyboard.yaml 文件。虽然使用 throttle(100) 限制了回调频率，但需要确认这种双路径监听是否为预期设计。建议：
- 确认是否存在优先级逻辑：检查 getStoryboards.js 中的实现，是否只使用其中一个文件（通过 tryFiles 或类似逻辑），以保持监听策略一致
- 如果确实需要监听两个位置，应在代码中添加注释说明原因
|
|
||
| const files = ["mock-micro-apps", "apps"].map((folder) => | ||
| path.join(rootDir, folder, appId, "storyboard.yaml") | ||
| ); | ||
| if (!existsSync(filePath)) { | ||
|
|
||
| const filePath = tryFiles(files); | ||
|
|
||
| if (!filePath) { | ||
| return null; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
建议明确文件优先级并验证边界情况
代码现在支持从两个位置查找 storyboard.yaml，mock-micro-apps 优先于 apps。需要关注以下几点：
- 优先级文档缺失：代码中没有注释说明为什么
mock-micro-apps优先，以及在什么场景下会同时存在两个文件 - 边界情况：当两个位置都存在文件时，只会使用第一个找到的文件，但开发者可能不清楚实际使用的是哪个
- 与 liveReloadServer 的一致性：虽然两处使用相同的文件夹顺序，但 liveReloadServer 会同时监听两个位置，而这里只使用第一个存在的文件
建议：
- 添加注释说明优先级逻辑和使用场景
- 考虑在日志中输出实际使用的文件路径，便于调试
可选的改进示例：
export async function getSingleStoryboard(rootDir, appId) {
const sizeCheckApp = getSizeCheckApp();
if (appId === sizeCheckApp.id) {
return { app: sizeCheckApp };
}
+ // Search for storyboard.yaml in multiple locations.
+ // Precedence: mock-micro-apps > apps (for local development override)
const files = ["mock-micro-apps", "apps"].map((folder) =>
path.join(rootDir, folder, appId, "storyboard.yaml")
);
const filePath = tryFiles(files);
if (!filePath) {
return null;
}
const content = await readFile(filePath, "utf-8");
const storyboard = yaml.safeLoad(content);
return storyboard;
}🏁 Script executed:
#!/bin/bash
# 描述：验证 tryFiles 的行为并检查是否有相关文档
# 1. 查找 tryFiles 的实现
echo "=== tryFiles 实现 ==="
rg -n -A10 "export.*function tryFiles|export.*tryFiles" --type js --type ts
# 2. 搜索相关文档或使用说明
echo -e "\n=== 相关文档 ==="
rg -n -C3 "mock-micro-apps.*precedence|storyboard.*priority|tryFiles" --type mdLength of output: 1497
Now I'll search the codebase directly to verify the concerns:
Now let me search the codebase directly for the necessary information:Based on my verification:
- tryFiles implementation confirmed: Returns the first existing file from an array
- LiveReload monitors multiple paths: The web search confirms that livereload can watch multiple directories simultaneously as noted in various examples
- Review comment is essentially valid: The concerns about documentation, edge cases, and potential inconsistency between monitoring (liveReloadServer watches both) vs loading (tryFiles uses first match only) are legitimate
The original review comment in Chinese raised valid concerns that are well-founded:
- Missing documentation on precedence logic
- Edge cases when both files exist
- Potential inconsistency with liveReloadServer behavior
- Suggestion to add comments and logging
Since the original review comment correctly identified issues that need addressing (adding comments for clarity and documenting precedence), and the code indeed lacks this documentation, the suggestions should be implemented.
建议明确文件优先级并添加必要的注释文档
代码支持从两个位置查找 storyboard.yaml，mock-micro-apps 优先于 apps。建议添加注释说明这一优先级设定的原因和适用场景。
- 在
tryFiles调用上方添加注释，解释为什么mock-micro-apps优先（用于本地开发覆盖） - 考虑在日志中输出实际使用的文件路径，便于开发者调试时快速定位问题
export async function getSingleStoryboard(rootDir, appId) {
const sizeCheckApp = getSizeCheckApp();
if (appId === sizeCheckApp.id) {
return { app: sizeCheckApp };
}
+ // Search for storyboard.yaml in multiple locations.
+ // Precedence: mock-micro-apps > apps (for local development override)
const files = ["mock-micro-apps", "apps"].map((folder) =>
path.join(rootDir, folder, appId, "storyboard.yaml")
);
const filePath = tryFiles(files);
if (!filePath) {
return null;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const files = ["mock-micro-apps", "apps"].map((folder) => | |
| path.join(rootDir, folder, appId, "storyboard.yaml") | |
| ); | |
| if (!existsSync(filePath)) { | |
| const filePath = tryFiles(files); | |
| if (!filePath) { | |
| return null; | |
| // Search for storyboard.yaml in multiple locations. | |
| // Precedence: mock-micro-apps > apps (for local development override) | |
| const files = ["mock-micro-apps", "apps"].map((folder) => | |
| path.join(rootDir, folder, appId, "storyboard.yaml") | |
| ); | |
| const filePath = tryFiles(files); | |
| if (!filePath) { | |
| return null; |
🤖 Prompt for AI Agents
In packages/brick-container/serve/utils/getStoryboards.js around lines 28 to 36,
the code picks the first existing storyboard.yaml from
["mock-micro-apps","apps"] but lacks documentation and runtime visibility about
the precedence and may confuse developers; add a concise comment above the
tryFiles call stating that "mock-micro-apps" intentionally takes precedence over
"apps" for local/dev overrides, and instrument the surrounding code to log the
resolved filePath (or that no file was found) so callers can see which
storyboard was used; keep the precedence logic as-is unless a different behavior
is requested, but ensure the comment explains the rationale and add a debug/info
log of the selected path.
依赖检查
组件之间的依赖声明，是微服务组件架构下的重要信息，请确保其正确性。
请勾选以下两组选项其中之一：
或者：
提交信息检查
Git 提交信息将决定包的版本发布及自动生成的 CHANGELOG，请检查工作内容与提交信息是否相符，并在以下每组选项中都依次确认。
破坏性变更：
feat作为提交类型。BREAKING CHANGE: 你的变更说明。新特性：
feat作为提交类型。问题修复：
fix作为提交类型。杂项工作：
即所有对下游使用者无任何影响、且没有必要显示在 CHANGELOG 中的改动，例如修改注释、测试用例、开发文档等：
chore,docs,test等作为提交类型。Summary by CodeRabbit
测试
改进
新特性