fix: #232 auto generate tabbar list item #234
Conversation
|
Warning Rate limit exceeded@edwinhuish has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 41 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughAdds per-page tabBar metadata and merges per-page tabBar entries into generated pages JSON; refactors Page to use meta caching with new uri/changed/read/getTabBar APIs; introduces UserTabBarItem type; renames playground option tabbar→tabBar; updates snapshots/tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer
participant Ctx as Context
participant Page as Page (n)
participant FS as File System
rect rgba(235,245,255,0.6)
note right of Dev: Build/pages generation starts
Dev->>Ctx: generatePagesJson()
Ctx->>Page: getPageMeta(force=false) *
alt meta not cached or force
Page->>FS: readPageMetaFromFile()
FS-->>Page: SFC/meta content
Page->>Page: parseSFC/derive meta
Page->>Page: cache meta, raw JSON, changed flag
end
Page-->>Ctx: PageMetaDatum
end
rect rgba(240,255,240,0.6)
note over Ctx,Page: Collect per-page tab bars
loop pages
Ctx->>Page: getTabBar(force=false)
Page-->>Ctx: TabBarItem + index or undefined
end
Ctx->>Ctx: sort by index, dedupe by pagePath, strip index
Ctx-->>Dev: pages.json { pages, tabBar, ... }
end
note over Dev: * getPageMeta() is now Promise-based
sequenceDiagram
autonumber
participant Page as Page
participant FS as File System
rect rgba(255,248,230,0.6)
Page->>Page: read()
Page->>FS: readPageMetaFromFile()
FS-->>Page: raw SFC/meta
Page->>Page: update this.meta and this.rawJson
Page->>Page: set this.changed accordingly
end
Page->>Page: getTabBar(force?)
alt meta missing and force
Page->>Page: read()
end
Page-->>Page: TabBarItem + index | undefined
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
✨ Finishing touches
🧪 Generate unit tests
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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/core/src/context.ts (2)
307-324: Stale-change check can skip real updates.
hasChanged()is consulted before any read, so it uses the previous snapshot and may early-return on genuine changes.Apply this to ensure the latest meta is loaded before deciding:
- if (page && !await page.hasChanged()) { - debug.cache(`The route block on page ${filepath} did not send any changes, skipping`) - return false - } + if (page) { + await page.read() + if (!await page.hasChanged()) { + debug.cache(`The route block on page ${filepath} did not send any changes, skipping`) + return false + } + }
355-360: Only emittabBarwhen it has items; guard invalid empty lists.Writing
{ tabBar: { list: [] } }may violate consumers that require 2–5 items; safer to omit the key if empty.- const data = { - ...this.pagesGlobConfig, - pages: this.pageMetaData, - subPackages: this.subPageMetaData, - tabBar: await this.getTabBarMerged(), - } + const mergedTabBar = await this.getTabBarMerged() + const data = { + ...this.pagesGlobConfig, + pages: this.pageMetaData, + subPackages: this.subPageMetaData, + ...(mergedTabBar.list.length ? { tabBar: mergedTabBar } : {}), + }
🧹 Nitpick comments (4)
packages/playground/src/pages/define-page/object.vue (1)
9-9: Use a non-empty example for clarity (index/text).An empty
{}works due to defaults, but adding minimal fields demonstrates intent and ordering.- tabBar: {}, + tabBar: { + // shown text and ordering example + text: 'object', + index: 20, + },test/generate.spec.ts (1)
24-33: Add tests for merged tabBar output.We should cover:
- auto-generated tab items from per-page
tabBar- merge order vs. global
tabBar.list- dedupe by
pagePath- respect of
indexsortWould you like me to add a spec like below?
it('merges tabBar from pages and global config', async () => { const ctx = new PageContext({ dir: 'packages/playground/src/pages', homePage: 'pages/index' }) await ctx.scanPages() await ctx.mergePageMetaData() const merged = (ctx as any).getTabBarMerged ? await (ctx as any).getTabBarMerged() : undefined expect(merged?.list.length).toBeGreaterThan(0) // add finer-grained assertions for order and dedupe })packages/core/src/context.ts (1)
283-305: Parallelize per-page tabBar reads.Gather promises first to avoid N serial awaits.
- const tabBarItems: (TabBarItem & { index: number })[] = [] - for (const [_, page] of this.pages) { - const tabbar = await page.getTabBar() - if (tabbar) { - tabBarItems.push(tabbar) - } - } + const results = await Promise.all( + [...this.pages.values()].map(p => p.getTabBar()) + ) + const tabBarItems = results.filter(Boolean) as (TabBarItem & { index: number })[]packages/core/src/page.ts (1)
63-72: Consider makinghasChanged()self-refreshing.If you prefer the check to be self-contained, have it read before comparison. Optional, since the context-side fix also works.
- public async hasChanged() { - return this.changed - } + public async hasChanged() { + await this.read() + return this.changed + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/core/src/context.ts(4 hunks)packages/core/src/page.ts(2 hunks)packages/core/src/types.ts(2 hunks)packages/playground/src/pages/define-page/object.vue(1 hunks)test/generate.spec.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-28T03:00:09.715Z
Learnt from: xiaohe0601
PR: uni-helper/vite-plugin-uni-pages#229
File: packages/core/client.d.ts:9-11
Timestamp: 2025-08-28T03:00:09.715Z
Learning: In uni-helper/vite-plugin-uni-pages, global type declarations like `definePage` are kept in a separate `client.d.ts` file and exposed via the `./client` export. Users need to manually add `uni-helper/vite-plugin-uni-pages/client` to their tsconfig.json to access these global declarations, which is documented as the intended usage pattern.
Applied to files:
packages/core/src/page.tspackages/core/src/context.ts
🧬 Code graph analysis (5)
packages/playground/src/pages/define-page/object.vue (2)
packages/core/src/config/types/tabBar.ts (3)
TabBarItem(32-76)TabBar(125-261)TabBarMidButton(78-123)packages/core/src/config/types/index.ts (1)
PagesConfig(23-145)
test/generate.spec.ts (1)
test/parser.spec.ts (3)
it(17-88)path(41-64)path(66-87)
packages/core/src/types.ts (2)
packages/core/src/config/types/tabBar.ts (3)
TabBarItem(32-76)TabBar(125-261)TabBarIconFont(3-30)packages/core/src/config/types/index.ts (2)
UserPagesConfig(147-147)PagesConfig(23-145)
packages/core/src/page.ts (3)
packages/core/src/types.ts (3)
UserPageMeta(160-171)PagePath(120-123)PageMetaDatum(125-140)packages/core/src/config/types/tabBar.ts (1)
TabBarItem(32-76)packages/core/src/files.ts (1)
readFileSync(34-41)
packages/core/src/context.ts (2)
packages/core/src/config/types/tabBar.ts (3)
TabBar(125-261)TabBarItem(32-76)TabBarIconFont(3-30)packages/core/src/config/types/index.ts (1)
PagesConfig(23-145)
🔇 Additional comments (6)
test/generate.spec.ts (2)
141-149: Snapshots updated correctly after removing per-page tabbar from routes.The changes reflect that
tabBaris not part of route output anymore. LGTM.
304-312: Second snapshot adjustment looks good.Consistent with the new behavior; no
tabbar: {}in routes.packages/core/src/types.ts (3)
3-3: Types import looks correct.Brings
TabBarIteminto scope for downstream typing.
147-158:UserTabBarItemshape is reasonable.
pagePath?(deprecated) andindex?are well-scoped; extension viaPartial<TabBarItem>is appropriate.
167-171:UserPageMeta.tabBaraddition aligns with the new feature.No issues.
packages/core/src/page.ts (1)
32-43: Meta projection LGTM.Excluding
tabBarfrom page meta avoids leaking it into routes. Thepath ?? urifallback is correct.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/context.ts (1)
358-363: Include tabBar conditionally at the call site.Mirror the optional return and omit the property when undefined to avoid emitting invalid configs.
- const data = { - ...this.pagesGlobConfig, - pages: this.pageMetaData, - subPackages: this.subPageMetaData, - tabBar: await this.getTabBarMerged(), - } + const mergedTabBar = await this.getTabBarMerged() + const data = { + ...this.pagesGlobConfig, + pages: this.pageMetaData, + subPackages: this.subPageMetaData, + ...(mergedTabBar ? { tabBar: mergedTabBar } : {}), + }
♻️ Duplicate comments (1)
packages/core/src/page.ts (1)
56-60: Typo breaks user-specified pagePath in tab items.Property should be
pagePath, notpathPath, otherwise overrides are ignored.return { ...tabBar, - pagePath: tabBar.pathPath || this.uri, + pagePath: tabBar.pagePath || this.uri, index: tabBar.index || 0, }Run to confirm no lingering occurrences:
#!/bin/bash rg -n "pathPath" -C2 --type ts
🧹 Nitpick comments (3)
packages/core/src/context.ts (1)
296-302: Dedup per-page tab items as well, not just against global list.If two pages set the same pagePath, both get appended. Consider set-based dedup during merge.
- const newTabbarItems = tabBarItems.sort((a, b) => a.index - b.index) - .filter(tabbar => tabBar.list.findIndex(item => item.pagePath === tabbar.pagePath) === -1) - .map((tabbar) => { - const { index: _, ...others } = tabbar - return others - }) + const seen = new Set(tabBar.list.map(i => i.pagePath)) + const ordered = tabBarItems.sort((a, b) => a.index - b.index) + const deduped: TabBarItem[] = [] + for (const { index: _i, ...item } of ordered) { + if (!seen.has(item.pagePath)) { + seen.add(item.pagePath) + deduped.push(item) + } + } + const newTabbarItems = dedupedpackages/core/src/page.ts (2)
45-61: Validate index input; prefer nullish coalescing.Guard against non-integer/negative indices and avoid truthiness pitfalls by using
??and sanitizing.- index: tabBar.index || 0, + index: Number.isInteger(tabBar.index) && tabBar.index! >= 0 ? tabBar.index! : 0,
74-89: Preserve original error for better diagnostics.Wrap with error cause to keep stack/context.
- catch (err: any) { - throw new Error(`Read page meta fail in ${this.path.relativePath}\n${err.message}`) - } + catch (err: any) { + throw new Error( + `Read page meta fail in ${this.path.relativePath}\n${err?.message ?? String(err)}`, + { cause: err }, + ) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/core/src/context.ts(5 hunks)packages/core/src/page.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-28T03:00:09.715Z
Learnt from: xiaohe0601
PR: uni-helper/vite-plugin-uni-pages#229
File: packages/core/client.d.ts:9-11
Timestamp: 2025-08-28T03:00:09.715Z
Learning: In uni-helper/vite-plugin-uni-pages, global type declarations like `definePage` are kept in a separate `client.d.ts` file and exposed via the `./client` export. Users need to manually add `uni-helper/vite-plugin-uni-pages/client` to their tsconfig.json to access these global declarations, which is documented as the intended usage pattern.
Applied to files:
packages/core/src/page.tspackages/core/src/context.ts
🧬 Code graph analysis (2)
packages/core/src/page.ts (3)
packages/core/src/types.ts (4)
UserPageMeta(160-171)PagePath(120-123)PageMetaDatum(125-140)UserPageMeta(147-153)packages/core/src/config/types/tabBar.ts (1)
TabBarItem(32-76)packages/core/src/files.ts (1)
readFileSync(34-41)
packages/core/src/context.ts (2)
packages/core/src/config/types/tabBar.ts (3)
TabBar(125-261)TabBarItem(32-76)TabBarMidButton(78-123)packages/core/src/config/types/index.ts (1)
PagesConfig(23-145)
⏰ 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). (1)
- GitHub Check: test (windows-latest, 20)
🔇 Additional comments (2)
packages/core/src/context.ts (1)
320-326: Efficient change-skip looks good.Reading once and skipping when the route block/meta hasn’t changed avoids unnecessary regeneration.
packages/core/src/page.ts (1)
29-30: URI derivation and meta projection look correct.Computing
urifrom relative path and excludingtabBarfrom returned page meta is appropriate.Also applies to: 32-43
| private async getTabBarMerged(): Promise<TabBar> { | ||
| const tabBar = { | ||
| ...this.pagesGlobConfig?.tabBar, | ||
| list: this.pagesGlobConfig?.tabBar?.list || [], | ||
| } | ||
|
|
||
| const tabBarItems: (TabBarItem & { index: number })[] = [] | ||
| for (const [_, page] of this.pages) { | ||
| const tabbar = await page.getTabBar() | ||
| if (tabbar) { | ||
| tabBarItems.push(tabbar) | ||
| } | ||
| } | ||
| const newTabbarItems = tabBarItems.sort((a, b) => a.index - b.index) | ||
| .filter(tabbar => tabBar.list.findIndex(item => item.pagePath === tabbar.pagePath) === -1) | ||
| .map((tabbar) => { | ||
| const { index: _, ...others } = tabbar | ||
| return others | ||
| }) | ||
| tabBar.list = [...tabBar.list, ...newTabbarItems] | ||
|
|
||
| return tabBar | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don't emit tabBar when list < 2; make it optional.
Always writing { list: [] } (or 1 item) is invalid per uni-app spec and may break existing apps that don't configure tabBar. Return undefined when the merged list has fewer than 2 items, and omit tabBar from pages.json.
- private async getTabBarMerged(): Promise<TabBar> {
+ private async getTabBarMerged(): Promise<TabBar | undefined> {
@@
- tabBar.list = [...tabBar.list, ...newTabbarItems]
-
- return tabBar
+ tabBar.list = [...tabBar.list, ...newTabbarItems]
+ // Only emit when valid (min 2 items)
+ if ((tabBar.list?.length || 0) < 2)
+ return undefined
+ return tabBar📝 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.
| private async getTabBarMerged(): Promise<TabBar> { | |
| const tabBar = { | |
| ...this.pagesGlobConfig?.tabBar, | |
| list: this.pagesGlobConfig?.tabBar?.list || [], | |
| } | |
| const tabBarItems: (TabBarItem & { index: number })[] = [] | |
| for (const [_, page] of this.pages) { | |
| const tabbar = await page.getTabBar() | |
| if (tabbar) { | |
| tabBarItems.push(tabbar) | |
| } | |
| } | |
| const newTabbarItems = tabBarItems.sort((a, b) => a.index - b.index) | |
| .filter(tabbar => tabBar.list.findIndex(item => item.pagePath === tabbar.pagePath) === -1) | |
| .map((tabbar) => { | |
| const { index: _, ...others } = tabbar | |
| return others | |
| }) | |
| tabBar.list = [...tabBar.list, ...newTabbarItems] | |
| return tabBar | |
| } | |
| private async getTabBarMerged(): Promise<TabBar | undefined> { | |
| const tabBar = { | |
| ...this.pagesGlobConfig?.tabBar, | |
| list: this.pagesGlobConfig?.tabBar?.list || [], | |
| } | |
| const tabBarItems: (TabBarItem & { index: number })[] = [] | |
| for (const [_, page] of this.pages) { | |
| const tabbar = await page.getTabBar() | |
| if (tabbar) { | |
| tabBarItems.push(tabbar) | |
| } | |
| } | |
| const newTabbarItems = tabBarItems.sort((a, b) => a.index - b.index) | |
| .filter(tabbar => tabBar.list.findIndex(item => item.pagePath === tabbar.pagePath) === -1) | |
| .map((tabbar) => { | |
| const { index: _, ...others } = tabbar | |
| return others | |
| }) | |
| tabBar.list = [...tabBar.list, ...newTabbarItems] | |
| // Only emit when valid (min 2 items) | |
| if ((tabBar.list?.length || 0) < 2) | |
| return undefined | |
| return tabBar | |
| } |
🤖 Prompt for AI Agents
In packages/core/src/context.ts around lines 283 to 305, the merged tabBar is
always emitted with at least { list: [] } which violates uni-app spec; change
the logic to compute the merged list first, and if the final list has fewer than
2 items return undefined (and omit tabBar from pages.json); update the method
signature/return type to Promise<TabBar | undefined> (and update any callers to
handle undefined) so tabBar is only produced when list.length >= 2.
2 participants
Description 描述
related: #232
使用方式：
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests