fix(Tabs): preserve state when items change#6374
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduces hydration-aware stable keys for Tabs by adding Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/components/Tabs.spec.ts (1)
165-196: Add a true hydration regression for the new refresh path.This test verifies a post-mount item update, but with plain
mount()the hydration branch inTabs.vuedoes not run. Please add a case that hydrates mismatched SSR/client items, or otherwise exerciseshydrationKeyPrefix = 0andshouldRefreshHydratedItems().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/components/Tabs.spec.ts` around lines 165 - 196, Add a new test in Tabs.spec.ts that exercises the SSR/hydration branch: simulate a pre-rendered DOM with a different items list (e.g., render markup or mount with hydrate: true) so the Tabs component enters the hydration path (hydrationKeyPrefix = 0) and then update props to trigger shouldRefreshHydratedItems(); specifically target Tabs.vue's hydrationKeyPrefix and shouldRefreshHydratedItems logic so the test hydrates mismatched SSR/client items and asserts a single active tab after refresh.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/runtime/components/Tabs.vue`:
- Around line 137-142: The current mapper uses the render index in the Vue key
which causes remounts when items shift; change the key generation inside
props.items.map to use a stable identifier derived from the item's explicit
value or label (e.g., get(item, props.valueKey) ?? get(item, props.labelKey))
and only fall back to String(index) for the public tab value, but not for the
Vue key; keep the returned value field semantics (value: get(item,
props.valueKey) ?? String(index)) but set key to
`${hydrationKeyPrefix.value}-${stableId}` where stableId is the non-index
identifier (value or label).
- Around line 183-187: The hydration check is collecting every descendant
[role="tab"] (rootEl.querySelectorAll) so nested Tabs' tabs leak into
tabs/activeIndexes; limit the selector to only the tab elements that belong to
this Tabs instance (e.g. use the tab-list container or a scoped selector like
':scope > [role="tab"]' or select within the specific tabs-list element) so the
tabs variable and activeIndexes only reflect this component's tabs when
comparing against resolvedItems.value and props.modelValue.
---
Nitpick comments:
In `@test/components/Tabs.spec.ts`:
- Around line 165-196: Add a new test in Tabs.spec.ts that exercises the
SSR/hydration branch: simulate a pre-rendered DOM with a different items list
(e.g., render markup or mount with hydrate: true) so the Tabs component enters
the hydration path (hydrationKeyPrefix = 0) and then update props to trigger
shouldRefreshHydratedItems(); specifically target Tabs.vue's hydrationKeyPrefix
and shouldRefreshHydratedItems logic so the test hydrates mismatched SSR/client
items and asserts a single active tab after refresh.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 31f0e06e-0571-46f9-8b5b-21b6690d517c
📒 Files selected for processing (2)
src/runtime/components/Tabs.vuetest/components/Tabs.spec.ts
commit: |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/runtime/components/Tabs.vue (1)
183-187:⚠️ Potential issue | 🟡 MinorLimit the hydration check to this component’s trigger elements.
The selector is scoped to the list, but it still includes any descendant
role="tab"rendered bylist-leading,list-trailing, or nested markup. That can makeactiveIndexes.length !== 1and force an unnecessary post-hydration remount even when this Tabs instance is already correct.Suggested refinement
- const tabs = Array.from(rootEl.querySelectorAll<HTMLElement>(':scope > [data-slot="list"] [role="tab"]')) + const tabs = Array.from(rootEl.querySelectorAll<HTMLElement>(':scope > [data-slot="list"] > [data-slot="trigger"][role="tab"]'))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/components/Tabs.vue` around lines 183 - 187, The hydration check currently queries all descendant role="tab" nodes under the list slot which can capture tabs rendered by list-leading/list-trailing or nested markup; change the selector used in tabs (rootEl.querySelectorAll) to only capture the list slot's immediate tab triggers (e.g. ':scope > [data-slot="list"] > [role="tab"]') so activeIndexes, expectedIndex (resolvedItems, props.modelValue) compare only this component’s actual trigger elements and avoid spurious remounts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/runtime/components/Tabs.vue`:
- Around line 137-145: The current mapping in the render (props.items.map)
builds stableId using get(item, props.valueKey) ?? get(item, props.labelKey)
which can produce duplicate keys when labels repeat; change stableId generation
to prefer props.valueKey, then props.labelKey only if proven unique, and
otherwise assign a per-item stable identifier using a WeakMap (e.g., a
module-scoped WeakMap keyed by the item objects) to generate and persist a
unique id per item; update key construction (using hydrationKeyPrefix.value +
'-' + stableId) so Vue keys are always stable and unique even when labels
duplicate, referencing the existing variables stableId, value,
hydrationKeyPrefix, and props.items to locate where to implement the WeakMap
fallback.
---
Duplicate comments:
In `@src/runtime/components/Tabs.vue`:
- Around line 183-187: The hydration check currently queries all descendant
role="tab" nodes under the list slot which can capture tabs rendered by
list-leading/list-trailing or nested markup; change the selector used in tabs
(rootEl.querySelectorAll) to only capture the list slot's immediate tab triggers
(e.g. ':scope > [data-slot="list"] > [role="tab"]') so activeIndexes,
expectedIndex (resolvedItems, props.modelValue) compare only this component’s
actual trigger elements and avoid spurious remounts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bffcae21-46d6-462b-ba56-2348c7af337b
📒 Files selected for processing (1)
src/runtime/components/Tabs.vue
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/runtime/components/Tabs.vue`:
- Around line 162-171: The current stableId can collide across sources
(explicitValue, label-derived, generated getTabsItemId/String(index)); change
the key generation in the props.items.map block so the returned key namespaces
the id source as well as the value—e.g., prefix explicit values with an
"explicit:" tag, label-derived ids with "label:", and generated ids with "gen:"
(apply this to the stableId used with hydrationKeyPrefix.value when building
key), referencing the existing symbols props.items.map, valueKey, labelKey,
getTabsItemId, stableId and hydrationKeyPrefix to locate and update the logic.
In `@test/components/Tabs.spec.ts`:
- Around line 261-294: The console.warn spy (created as warn = vi.spyOn(console,
'warn').mockImplementation(() => {})) must be restored in a finally block so it
always runs even if wrapper.setProps or the assertion throws; wrap the await
wrapper.setProps(...) and the expectations in try { ... } finally {
warn.mockRestore() } (keeping creation of the spy before and restoration in
finally) to guarantee cleanup for subsequent tests and reference the existing
warn, TestTabs, and wrapper.setProps symbols when making the change.
- Around line 216-247: Wrap the mounting, tick waits and assertions in a
try/finally so cleanup always runs: after creating and app.mount(container) (the
createSSRApp instance referenced as app) and awaiting nextTick(), perform the
DOM queries and expect checks inside the try block, and call app.unmount() and
container.remove() in the finally block to ensure the mounted app and container
are removed even if an assertion throws; ensure you still append the container
to document.body before mounting and preserve the existing nextTick() waits
before querying.
- Line 11: The hydration test gate is inverted: the const hydrationIt currently
sets hydrationIt to it.skip when __NUXT_VITEST_ENVIRONMENT__ is truthy but it
should run in that environment; update the ternary for the declaration of
hydrationIt so that when (globalThis as { __NUXT_VITEST_ENVIRONMENT__?: boolean
}).__NUXT_VITEST_ENVIRONMENT__ is true you assign it and otherwise assign
it.skip (i.e., swap the two branches for the hydrationIt constant).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ab468be0-2044-4b78-a7b8-a009d2caae65
📒 Files selected for processing (2)
src/runtime/components/Tabs.vuetest/components/Tabs.spec.ts
onmax
force-pushed
the
fix/tabs-stable-identity-5841
branch
from
6dcc151 to
6714654
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/components/Tabs.spec.ts (1)
138-166: Consider unmounting the wrapper at end of test.
wrapperis never explicitly unmounted, so theonUnmountedrecording inStatefulContentonly captures removals caused by thesetPropsreconciliation (which is what this test asserts — good). However, leaving the wrapper mounted can leak reactive effects across tests. Addingwrapper.unmount()after the assertions is a cheap safeguard.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/components/Tabs.spec.ts` around lines 138 - 166, Test leaves the Vue test wrapper mounted which can leak reactive effects; unmount the wrapper at the end of the test. After the existing assertions that check getInstances(), unmounted, and activeTabs, call wrapper.unmount() to teardown the mounted TestTabs instance (the component using StatefulContent that populates the unmounted array) so reactive side effects are cleaned up between tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/components/Tabs.spec.ts`:
- Around line 138-166: Test leaves the Vue test wrapper mounted which can leak
reactive effects; unmount the wrapper at the end of the test. After the existing
assertions that check getInstances(), unmounted, and activeTabs, call
wrapper.unmount() to teardown the mounted TestTabs instance (the component using
StatefulContent that populates the unmounted array) so reactive side effects are
cleaned up between tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c66c281f-820f-4f7a-bd19-3107319f3019
📒 Files selected for processing (2)
src/runtime/components/Tabs.vuetest/components/Tabs.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/runtime/components/Tabs.vue
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/components/Tabs.spec.ts (1)
11-11:⚠️ Potential issue | 🟠 MajorRun the hydration test in the Nuxt environment.
hydrationItcurrently skips when__NUXT_VITEST_ENVIRONMENT__is truthy, which leaves the SSR hydration regression untested in the target environment. Swap the ternary branches.Fix
-const hydrationIt = (globalThis as { __NUXT_VITEST_ENVIRONMENT__?: boolean }).__NUXT_VITEST_ENVIRONMENT__ ? it.skip : it +const hydrationIt = (globalThis as { __NUXT_VITEST_ENVIRONMENT__?: boolean }).__NUXT_VITEST_ENVIRONMENT__ ? it : it.skipRun this read-only check to verify the flag’s usage in this repo:
#!/bin/bash # Description: Inspect how the Nuxt Vitest environment flag is used and confirm # the hydration test gate maps the Nuxt environment to `it`, not `it.skip`. sed -n '1,20p' test/components/Tabs.spec.ts rg -n -C3 '__NUXT_VITEST_ENVIRONMENT__|environment:\s*["'\''"]nuxt|@nuxt/test-utils' \ --glob '*.ts' \ --glob '*.vue'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/components/Tabs.spec.ts` at line 11, The hydrationIt definition currently skips tests when the Nuxt env flag is truthy; change the ternary so that hydrationIt resolves to it (not it.skip) when (globalThis as { __NUXT_VITEST_ENVIRONMENT__?: boolean }).__NUXT_VITEST_ENVIRONMENT__ is truthy; locate the hydrationIt declaration in test/components/Tabs.spec.ts and swap the it.skip and it branches so SSR hydration tests run in the Nuxt Vitest environment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/components/Tabs.spec.ts`:
- Line 11: The hydrationIt definition currently skips tests when the Nuxt env
flag is truthy; change the ternary so that hydrationIt resolves to it (not
it.skip) when (globalThis as { __NUXT_VITEST_ENVIRONMENT__?: boolean
}).__NUXT_VITEST_ENVIRONMENT__ is truthy; locate the hydrationIt declaration in
test/components/Tabs.spec.ts and swap the it.skip and it branches so SSR
hydration tests run in the Nuxt Vitest environment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6db9dc94-899b-46d5-8723-1fad0583fd8b
📒 Files selected for processing (1)
test/components/Tabs.spec.ts
|
Closing in favor of #6380. |
2 participants
🔗 Linked issue
Resolves #5841
❓ Type of change
📚 Description
This PR keeps the
UTabsfix focused on the reported issue and the hydration mismatch reproduced in the linked portal case.It fixes the state-loss problem when tab items with stable values are inserted or removed before the active tab by switching trigger/content identity away from render indexes.
It also keeps the hydration-specific repair for the direct-load case where the server renders only
Third, the client hydrates withFirst,Second,Third, andv-modelstaysthird. In that situation, stable keys alone are not enough, so the component performs a one-time post-mount refresh only when the hydrated active tab DOM does not matchmodelValue.Intentionally out of scope for this PR:
Accordionor any other componentThe reduced test coverage now focuses on:
unmountOnHide=false📝 Checklist