fix(tabs): Optimize the rendering logic of multi terminal tabs #3707
Conversation
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
chenxi-20
force-pushed
the
fix/tabs-20250915
branch
from
2fdd0d5 to
d3cd31d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (16)
packages/renderless/src/tabs-mf/vue.ts (1)
42-45: Make currentItem sync deterministic; avoid key churn before mountcurrentItem is null until onMounted; key falls back to random(), which can cause an extra remount and cache misses. Prefer a stable key while currentItem is null (e.g., cacheCurrentItem?.name) and ensure non‑swipeable flows keep currentItem in sync with selection.
- currentItem: null, - cacheCurrentItem: computed(() => state.items.find((item) => item.selected)), - key: computed(() => (state.currentItem ? state.currentItem.name : random())), + currentItem: null, + cacheCurrentItem: computed(() => state.items.find((item) => item.selected)), + key: computed(() => state.cacheCurrentItem?.name || random()),Additionally, to keep non‑swipeable usage in sync without relying on slider transitions:
watch( () => props.modelValue, (name) => name && api.setActive(name) ) + + // Keep content in sync when not using slider transitions + watch( + () => state.cacheCurrentItem, + (val) => { + if (!state.swipeable) state.currentItem = val + } + )packages/vue/src/tabs/src/mobile-first.vue (2)
51-53: Short-circuit when no currentItem to avoid per-item checksMinor perf nit: bail out before iterating rather than checking inside each loop.
- state.items.forEach((item) => { - if (!state.currentItem || item.name !== state.currentItem.name) { - return - } + if (!state.currentItem) { + // nothing to render in optimized mode until selection resolves + } else { + state.items.forEach((item) => { + if (item.name !== state.currentItem.name) return … - }) + }) + }
51-64: Use name-based equality consistently across both render pathsOptimized path uses name equality; the non‑optimized path (Line 76) still uses identity (item === state.currentItem). For consistency and to avoid edge cases when item references change, consider switching that to name equality too.
packages/vue/src/tabs/src/mobile-first/tab-nav.vue (1)
31-36: Defer slider render until measurements exist to prevent flickerRender the slider bar only when we have a nav and non‑zero width/position; otherwise hide it.
- h(tabSliderBar, { - props: { - currentWidth: state.currentWidth, - currentPosition: state.currentPosition, - currentNav: state.currentNav - } - }) + (state.currentNav && (state.currentWidth > 0 || state.currentPosition > 0)) + ? h(tabSliderBar, { + props: { + currentWidth: state.currentWidth, + currentPosition: state.currentPosition, + currentNav: state.currentNav + } + }) + : nullpackages/vue/src/tabs/src/mobile-first/tab-bar.vue (1)
12-14: Prop passthrough is fine; consider explicit typing for DXNon-blocking: add an interface for forwarded props to improve TS hints.
packages/renderless/src/tabs-mf/vue-slider-bar.ts (4)
12-15: Null-safety for injected tabsIf used outside the provider, this will throw. Add a guard.
- const handleTransitionEnd = () => { - tabs.state.currentItem = tabs.state.cacheCurrentItem - } + const handleTransitionEnd = () => { + if (tabs && tabs.state) { + tabs.state.currentItem = tabs.state.cacheCurrentItem + } + }
16-22: Lifecycle: prefer onBeforeUnmount; also guard vm.$elDetach the listener before unmount and avoid accessing a null el.
-export const renderless = (props, { reactive, inject, computed, onMounted, onUnmounted, onUpdated }, { vm, emit }) => { +export const renderless = (props, { reactive, inject, computed, onMounted, onBeforeUnmount }, { vm }) => { … - onMounted(() => { - vm.$el.addEventListener('transitionend', handleTransitionEnd) - }) + onMounted(() => { + vm?.$el && vm.$el.addEventListener('transitionend', handleTransitionEnd) + }) - onUnmounted(() => { - vm.$el.removeEventListener('transitionend', handleTransitionEnd) - }) + onBeforeUnmount(() => { + vm?.$el && vm.$el.removeEventListener('transitionend', handleTransitionEnd) + })
24-25: Align return shape with other renderless modulesOthers return an api object with state attached. Keep it consistent.
- return { state } + const apiObj = {} + Object.assign(apiObj, { state }) + return apiObj
1-4: API list OK; remove unused params/importsonUpdated and emit are unused after the change.
-export const api = ['state'] +export const api = ['state']Follow-up: drop onUpdated and emit from the signature (see previous diff).
packages/vue/src/tabs/src/mobile-first/tab-slider-bar.vue (3)
28-33: Verify use of m(...) with utility classes.If m is a BEM helper, passing raw utility classes may produce incorrect class names. Consider using a plain class string/array for utilities and reserve m for namespaced classes.
- class: m( - 'absolute bg-color-brand transition-all duration-100 z-20 rounded-full bottom-px h-px py-px box-content', - { - hidden: !nav - } - ), + class: ['absolute bg-color-brand transition-all duration-100 z-20 rounded-full bottom-px h-px py-px box-content', { hidden: !nav }],If m is required for namespacing, append it: class: [m(), 'absolute ...', { hidden: !nav }]
27-35: Optional: animate with transform for smoother perf.left/width cause layout; translateX is often smoother for slider bars. Keep width for size, but move with transform.
- style: { width, left } + style: { width, transform: `translateX(${left}px)` }
27-35: Optional a11y: add aria-hidden when hidden.Expose visibility to AT when the slider is not rendered.
- return h('div', { + return h('div', { + 'aria-hidden': !nav,packages/renderless/src/tabs-mf/vue-nav.ts (4)
25-26: Scope observation narrowly.Observing vm.$el subtree for all attribute changes is broad. If possible, observe only currentNav.$el and rebind when currentNav changes to reduce callback pressure.
7-9: Null-guard inject('tabs').If tabs injection ever fails, tabs.state.* will throw. Add a guard or a clear error.
- const tabs = inject('tabs', null) + const tabs = inject('tabs', null) + if (!tabs || !tabs.state) { + throw new Error('[tabs-mf/vue-nav] missing tabs provider') + }
21-22: Position calculation robustness.offsetLeft is relative to offsetParent; inside transformed/scrolling containers this can drift. Consider using getBoundingClientRect relative to container rect.
3-3: Type the renderless signature.Add typings for props/context/vm for better DX and tree-shaking safety.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/renderless/src/tabs-mf/vue-nav.ts(1 hunks)packages/renderless/src/tabs-mf/vue-slider-bar.ts(1 hunks)packages/renderless/src/tabs-mf/vue.ts(2 hunks)packages/vue/src/tabs/src/mobile-first.vue(1 hunks)packages/vue/src/tabs/src/mobile-first/tab-bar.vue(2 hunks)packages/vue/src/tabs/src/mobile-first/tab-nav.vue(2 hunks)packages/vue/src/tabs/src/mobile-first/tab-slider-bar.vue(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/renderless/src/tabs-mf/vue-nav.ts (2)
packages/renderless/src/tabs-mf/vue-slider-bar.ts (1)
api(1-1)packages/renderless/src/tabs-mf/vue.ts (1)
api(21-32)
packages/renderless/src/tabs-mf/vue-slider-bar.ts (2)
packages/renderless/src/tabs-mf/vue-nav.ts (2)
api(1-1)renderless(3-38)packages/renderless/src/tabs-mf/vue.ts (2)
api(21-32)renderless(36-111)
⏰ 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: PR E2E Test (pnpm test:e2e3)
🔇 Additional comments (2)
packages/vue/src/tabs/src/mobile-first/tab-bar.vue (1)
3-3: ```shell
#!/bin/bash
set -euo pipefailecho "PWD: $(pwd)"
echo
echo "=== Searching for literal occurrences of $props ==="
rg -n --hidden --no-ignore -S '$props' || true
echo
echo "=== Searching for spread patterns '...props' (anywhere) ==="
rg -nP --hidden --no-ignore -S '...\sprops' || true
echo
echo "=== Searching for 'props:' array/object defs that include a spread ==="
rg -nP --hidden --no-ignore -S 'props\s:\s*([|{)[^\}\]].{3}' || true
echo
echo "=== Searching for imports that mention $props ==="
rg -n --hidden --no-ignore -S "import\s{[^}]*$props" || true
echo
echo "=== Searching for mobile-first and tab-bar files ==="
rg -n --hidden --no-ignore -S 'mobile-first|tab-bar.vue' || true</blockquote></details> <details> <summary>packages/renderless/src/tabs-mf/vue-nav.ts (1)</summary><blockquote> `9-11`: **Switch to cacheCurrentItem: confirm semantics.** cacheCurrentItem is a computed selection (packages/renderless/src/tabs-mf/vue.ts:43) while state.currentItem is assigned from it later (vue.ts:97 and vue-slider-bar.ts:13). vue-nav computes currentIndex from cacheCurrentItem (packages/renderless/src/tabs-mf/vue-nav.ts:10), so the nav can update earlier than the slider/content (which follow state.currentItem) during fast programmatic switches — verify with rapid programmatic/user tab switches. If you see visual desync, either use state.currentItem for the nav or update state.currentItem earlier. Locations to check: packages/renderless/src/tabs-mf/vue.ts (lines ~42–44, 97), vue-slider-bar.ts (line ~13), vue-nav.ts (line ~10), index.ts (uses state.currentItem at ~lines 9–11 and 311–315). </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
New Features
Improvements
Bug Fixes