Tabs/Accordion, modelValue is id now and we have v-model:index. #2716
Conversation
… the old value fix(BTabs): fix initial value and some ssr errors fix(BTab): @click event is now preventable. fix(BTabs): reactivity in v-for fixed fix(BTabs): fix focus on keyboard navigation feat(BTab): expose activate and deactivate
feat((BAccordion): modelValue is array id prop free is true feat(BAccordion): add model index to control the index of open item/items. If free is true it is an array of open items fix(BAccordion): reactivity working in v-for fix(BAccordion): ssr fixes.
|
|
WalkthroughThis change refactors and extends the APIs and internal logic of the Changes
Sequence Diagram(s)sequenceDiagram
participant Parent as Parent Component
participant BAccordion as BAccordion
participant BAccordionItem as BAccordionItem
Parent->>BAccordion: Bind v-model and/or v-model:index
BAccordion->>BAccordionItem: Provide openItem, setOpenItem, setCloseItem, registerAccordionItem
BAccordionItem->>BAccordion: registerAccordionItem(id, el)
BAccordionItem->>BAccordion: setOpenItem(id) / setCloseItem(id)
BAccordion-->>Parent: Emit update:model-value / update:index
BAccordionItem->>BAccordion: unregisterAccordionItem(id)
sequenceDiagram
participant Parent as Parent Component
participant BTabs as BTabs
participant BTab as BTab
Parent->>BTabs: Bind v-model:index and/or v-model
BTabs->>BTab: Provide activeIndex, activeId, registerTab, activateTab
BTab->>BTabs: registerTab(tab)
BTab->>BTabs: activateTab(internalId)
BTabs-->>Parent: Emit activate-tab(newTabId, prevTabId, newTabIndex, prevTabIndex, bvEvent)
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
✨ 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 (
|
commit: |
There was a problem hiding this comment.
Pull Request Overview
This pull request updates the Tabs and Accordion components to use IDs (modelValue) instead of indices for active items and introduces the new v-model:index syntax throughout the codebase. Key changes include:
- Updating TypeScript types and component APIs so that modelValue represents an id (or an array of ids) rather than an index.
- Refactoring internal tab registration to use a new internalId and adapting event signatures, particularly for the activate-tab event.
- Updating documentation, tests, and demo examples to reflect the new v-model:index usage.
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/bootstrap-vue-next/src/utils/keys.ts | Added ShallowRef import to support new functionality. |
| packages/bootstrap-vue-next/src/types/Tab.ts | Introduced internalId and updated onClick, titleItemClass, titleLinkAttrs, and titleLinkClass definitions. |
| packages/bootstrap-vue-next/src/types/ComponentProps.ts | Updated props types for BTabs and accordion components, switching modelValue from a numeric index to a string id. |
| packages/bootstrap-vue-next/src/components/BTabs/* | Refactored tab registration, activation, and watcher logic to support the new v-model:index API, including changes in key binding and event signature. |
| packages/bootstrap-vue-next/src/components/BAccordion/* | Adjusted accordion item handling for free mode and synchronized index/modelValue, plus updated injection functions. |
| apps/playground/src/components/Comps/TTabs.vue, apps/docs/* | Updated examples and documentation to use the new v-model:index syntax. |
Comments suppressed due to low confidence (2)
packages/bootstrap-vue-next/src/components/BTabs/BTabs.vue:384
- [nitpick] The watcher on activeId contains logic that adjusts activeIndex based on tab order. It may benefit from being refactored into a helper function for improved clarity and maintainability.
watch(activeId, (newValue, oldValue) => {
packages/bootstrap-vue-next/src/components/BAccordion/BAccordion.vue:227
- [nitpick] The branching logic in setOpenItem based on the free prop and modelValue type is somewhat convoluted; consider simplifying or adding detailed comments to clarify how modelValue is expected to behave in free vs. non-free modes.
setOpenItem: (id: string) => {
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (2)
apps/docs/src/docs/components/tabs.md (1)
239-246: 💡 Verification agent🧩 Analysis chain
Verify consistency of the BTabs API migration across the codebase.
The documentation update looks correct, but ensure all BTabs component usages have been migrated to the new API.
🏁 Script executed:
#!/bin/bash # Description: Verify all BTabs components use the new v-model:index syntax # Expected: All BTabs should use v-model:index instead of plain v-model echo "=== Checking for old v-model syntax on BTabs ===" rg -A 2 '<BTabs.*v-model="' --type vue || echo "No old syntax found (good!)" echo -e "\n=== Checking for new v-model:index syntax ===" rg -A 2 '<BTabs.*v-model:index=' --type vue echo -e "\n=== Checking for any BTabs without explicit model binding ===" rg -A 2 '<BTabs(?![^>]*v-model)' --type vueLength of output: 838
I’ll recheck all BTabs usages in
.vuefiles with more reliable patterns:
🏁 Script executed:
#!/bin/bash echo "=== All BTabs usages in .vue files ===" rg -n -g '*.vue' '<BTabs' echo -e "\n=== BTabs with old plain v-model syntax ===" rg -n -g '*.vue' '<BTabs[^>]*v-model="' || echo "None found" echo -e "\n=== BTabs with new v-model:index syntax ===" rg -n -g '*.vue' '<BTabs[^>]*v-model:index=' || echo "None found" echo -e "\n=== BTabs without any v-model binding ===" rg -n -g '*.vue' '<BTabs' | grep -v 'v-model' | sed 's/$/ <-- missing binding/' \ || echo "All BTabs have explicit binding"Length of output: 6050
Ensure BTabs usages are updated to the new v-model API
Several demo and playground components still instantiate
<BTabs>without anyv-model:index(orv-model:active-id) binding. Please update each of these to use the new API—for example:<BTabs v-model:index="tabIndex" …>and define the corresponding
tabIndex(e.g.const tabIndex = ref(0)) in their<script setup>blocks.Files requiring fixes:
- apps/docs/src/docs/components/demo/TabsJustified.vue
- apps/docs/src/docs/components/demo/TabsBottom.vue
- apps/docs/src/docs/components/demo/TabsDynamic.vue
- apps/docs/src/docs/components/demo/TabsVerticalWidth.vue
- apps/docs/src/docs/components/demo/TabsVerticalEnd.vue
- apps/docs/src/docs/components/demo/TabsVertical.vue
- apps/docs/src/docs/components/demo/TabsPills.vue
- apps/docs/src/docs/components/demo/TabsPictures.vue
- apps/docs/src/docs/components/demo/TabsOverview.vue
- apps/docs/src/docs/components/demo/TabsLazyAll.vue
- apps/docs/src/docs/components/demo/TabsLazy.vue
- apps/docs/src/docs/components/demo/TabsFill.vue
- apps/docs/src/docs/components/demo/TabsEmpty.vue
- apps/docs/src/docs/components/demo/TabsCustom.vue
- apps/docs/src/docs/components/demo/TabsAlignment.vue
- apps/docs/src/docs/components/demo/TabsActive.vue
- apps/playground/src/components/Comps/TTabs.vue (lines 50 and 118)
🧰 Tools
🪛 LanguageTool
[uncategorized] ~241-~241: Loose punctuation mark.
Context: ...itted with five arguments: -newTabId: The id of the tab that is going to be a...(UNLIKELY_OPENING_PUNCTUATION)
[style] ~241-~241: Use ‘will’ instead of ‘going to’ if the following action is certain.
Context: ...: -newTabId: The id of the tab that is going to be activated -preTabId: The id of th...(GOING_TO_WILL)
[style] ~243-~243: Use ‘will’ instead of ‘going to’ if the following action is certain.
Context: ...newTabIndex: The index of the tab that is going to be activated -prevTabIndex`: The inde...(GOING_TO_WILL)
packages/bootstrap-vue-next/src/types/ComponentProps.ts (1)
116-125: 💡 Verification agent🧩 Analysis chain
Consider documenting the breaking changes and migration path.
The changes to
BAccordionPropsintroduce multi-selection support which is a nice enhancement. However, changingmodelValuefromstringtostring | string[]is a breaking change that will affect existing consumers.Ensure that the migration guide documents these breaking changes. Run the following script to check if there's documentation for this change:
🏁 Script executed:
#!/bin/bash # Search for migration guide or changelog entries about these breaking changes fd -e md -e mdx | xargs rg -l "BAccordion.*modelValue.*breaking|migration.*accordion" -A 5Length of output: 92
🏁 Script executed:
#!/bin/bash # Search all markdown files for references to Accordion or migration guidance fd -e md -e mdx | xargs rg -n "Accordion" -A2 fd -e md -e mdx | xargs rg -n "migration" -A2Length of output: 20933
Document BAccordion API Breaking Changes
The migration guide and changelog don’t yet mention that:
- The
modelValueprop onBAccordionhas changed fromstringtostring | string[](enabling multiple selection).- A new
index?: number | number[]prop is available to control panels by index.Please update:
• CHANGELOG.md (e.g. under “### Breaking Changes”):
- **BAccordionProps**: - `modelValue` now accepts `string | string[]` (was `string`). - Added `index?: number | number[]` for index-based single or multiple panels.• apps/docs/src/docs/migration-guide.md under the “### BAccordion” section:
#### BAccordion - `modelValue` now accepts both `string` and `string[]`. If you upgrade from single to multi-select, update your `v-model` usage to handle arrays. - Added `index?: number | number[]` to programmatically control active items by their index(es).These additions will ensure consumers see the required steps when migrating.
🧹 Nitpick comments (7)
apps/docs/src/docs/components/tabs.md (1)
239-246: Documentation correctly updated, but address minor style issues.The documentation properly reflects the expanded
activate-tabevent signature with both ID and index parameters. However, there are minor style improvements needed:Apply this diff to address the style issues:
-tab from activating. The `activate-tab` event is emitted with five arguments: +tab from activating. The `activate-tab` event is emitted with five arguments: -- `newTabId`: The id of the tab that is going to be activated -- `preTabId`: The id of the currently active tab +- `newTabId`: The id of the tab that will be activated +- `preTabId`: The id of the currently active tab🧰 Tools
🪛 LanguageTool
[uncategorized] ~241-~241: Loose punctuation mark.
Context: ...itted with five arguments: -newTabId: The id of the tab that is going to be a...(UNLIKELY_OPENING_PUNCTUATION)
[style] ~241-~241: Use ‘will’ instead of ‘going to’ if the following action is certain.
Context: ...: -newTabId: The id of the tab that is going to be activated -preTabId: The id of th...(GOING_TO_WILL)
[style] ~243-~243: Use ‘will’ instead of ‘going to’ if the following action is certain.
Context: ...newTabIndex: The index of the tab that is going to be activated -prevTabIndex`: The inde...(GOING_TO_WILL)
packages/bootstrap-vue-next/src/components/BAccordion/BAccordionItem.vue (1)
141-147: Consider simplifying the mounted hook logic.The current logic in onMounted checks for a specific condition that might already be handled by the watchers. Since the
openInParentcomputed property and watchers handle synchronization, this mounted hook might be redundant.-onMounted(() => { - if (!modelValue.value && parentData?.openItem.value === computedId.value) { - nextTick(() => { - modelValue.value = true - }) - } -}) +// The watchers handle synchronization, so explicit mounted logic may not be neededpackages/bootstrap-vue-next/src/types/ComponentProps.ts (1)
714-756: Breaking API changes in tab components need clear documentation.The changes to
BTabPropsandBTabsPropsrepresent significant breaking changes:
BTabProps:lazyOnce→unmountLazy(semantic change)BTabsProps:activeIdremoved,modelValuetype changed fromnumbertostringThese changes will require consumers to update their code.
Would you like me to help generate TypeScript migration examples or a codemod to help users migrate from the old API to the new one?
packages/bootstrap-vue-next/src/components/BTabs/BTab.vue (2)
99-103: Remove commented code or implement the watcher if needed.This commented watcher suggests incomplete implementation. If prop changes need to trigger tab updates, implement the watcher. Otherwise, remove the commented code.
-// watch(() => props, () => { -// if (!parentData) return -// updateTab() -// })
69-90: Consider optimizing the tab registration approach.The current implementation creates a new computed ref on each
updateTab()call. SinceupdateTab()is only called once during component initialization, this is acceptable. However, if you plan to make props reactive (as suggested by the commented watcher), consider creating the computed ref once and reusing it.If you need reactive prop updates, consider this optimization:
+const tab = computed( + () => + ({ + internalId: internalId.value, + id: computedId.value, + active: activeModel.value, + buttonId: computedButtonId.value, + disabled: props.disabled, + title: props.title, + titleComponent: slots.title, + titleItemClass: props.titleItemClass, + titleLinkAttrs: props.titleLinkAttrs, + titleLinkClass: props.titleLinkClass, + onClick: attrs.onClick, + el, + }) as TabType +) + function updateTab() { if (!parentData) return - parentData.registerTab( - computed( - () => - ({ - internalId: internalId.value, - id: computedId.value, - active: activeModel.value, - buttonId: computedButtonId.value, - disabled: props.disabled, - title: props.title, - titleComponent: slots.title, - titleItemClass: props.titleItemClass, - titleLinkAttrs: props.titleLinkAttrs, - titleLinkClass: props.titleLinkClass, - onClick: attrs.onClick, - el, - }) as TabType - ) - ) + parentData.registerTab(tab) }packages/bootstrap-vue-next/src/components/BAccordion/BAccordion.vue (1)
117-206: Consider extracting synchronization logic for better maintainability.The watchers for
indexandmodelValuecontain complex logic that could be extracted into helper functions for better readability and testability.Would you like me to help refactor this synchronization logic into smaller, more focused helper functions? This would improve code maintainability and make unit testing easier.
🧰 Tools
🪛 GitHub Check: test-lint
[warning] 156-156:
Unexpected console statementpackages/bootstrap-vue-next/src/components/BTabs/BTabs.vue (1)
290-292: Optional chaining is redundant with typeof checkThe optional chaining on line 292 is unnecessary since you already check that
onClickis a function on line 290.- tabs.value[index].onClick?.(event) + tabs.value[index].onClick(event)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
apps/docs/src/data/components/accordion.data.ts(3 hunks)apps/docs/src/data/components/tabs.data.ts(4 hunks)apps/docs/src/docs/components/demo/TabsCustomLinks.vue(1 hunks)apps/docs/src/docs/components/demo/TabsDynamic.vue(1 hunks)apps/docs/src/docs/components/demo/TabsExternalControls.vue(1 hunks)apps/docs/src/docs/components/tabs.md(1 hunks)apps/playground/src/components/Comps/TTabs.vue(3 hunks)packages/bootstrap-vue-next/src/components/BAccordion/BAccordion.vue(3 hunks)packages/bootstrap-vue-next/src/components/BAccordion/BAccordionItem.vue(4 hunks)packages/bootstrap-vue-next/src/components/BAccordion/accordion.spec.ts(2 hunks)packages/bootstrap-vue-next/src/components/BTabs/BTab.vue(6 hunks)packages/bootstrap-vue-next/src/components/BTabs/BTabs.vue(10 hunks)packages/bootstrap-vue-next/src/components/BTabs/tabs.spec.ts(1 hunks)packages/bootstrap-vue-next/src/types/ComponentProps.ts(3 hunks)packages/bootstrap-vue-next/src/types/Tab.ts(1 hunks)packages/bootstrap-vue-next/src/utils/keys.ts(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
packages/bootstrap-vue-next/src/types/Tab.ts (2)
packages/bootstrap-vue-next/src/types/index.ts (1)
TabType(62-62)packages/bootstrap-vue-next/src/types/AnyValuedAttributes.ts (1)
ClassValue(2-2)
packages/bootstrap-vue-next/src/utils/keys.ts (1)
packages/bootstrap-vue-next/src/types/Tab.ts (1)
TabType(4-18)
packages/bootstrap-vue-next/src/types/ComponentProps.ts (1)
packages/bootstrap-vue-next/src/types/AnyValuedAttributes.ts (1)
ClassValue(2-2)
🪛 LanguageTool
apps/docs/src/docs/components/tabs.md
[uncategorized] ~241-~241: Loose punctuation mark.
Context: ...itted with five arguments: - newTabId: The id of the tab that is going to be a...
(UNLIKELY_OPENING_PUNCTUATION)
[style] ~241-~241: Use ‘will’ instead of ‘going to’ if the following action is certain.
Context: ...: - newTabId: The id of the tab that is going to be activated - preTabId: The id of th...
(GOING_TO_WILL)
🪛 GitHub Check: test-lint
packages/bootstrap-vue-next/src/components/BAccordion/BAccordion.vue
[warning] 213-213:
Use array destructuring
[warning] 156-156:
Unexpected console statement
[warning] 90-90:
Unexpected console statement
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (26)
apps/docs/src/docs/components/demo/TabsExternalControls.vue (1)
5-5: LGTM! API migration correctly implemented.The change from
v-model="tabIndex"tov-model:index="tabIndex"properly aligns with the new BTabs component API that separates index and ID models for better control.apps/docs/src/docs/components/demo/TabsCustomLinks.vue (1)
3-3: LGTM! Consistent API migration.The change to
v-model:index="tabIndex"maintains consistency with the new BTabs API across demo components.apps/playground/src/components/Comps/TTabs.vue (2)
5-5: LGTM! API migration correctly implemented.The change to
v-model:index="tabIndex"follows the consistent pattern for the new BTabs API.
92-92: Excellent demonstration of enhanced dual binding capability.These lines showcase the new BTabs API's ability to simultaneously track both the active tab index and ID through dual binding (
v-model:indexandv-model:active-id). This provides more granular control over tab state management.Also applies to: 126-126
packages/bootstrap-vue-next/src/components/BTabs/tabs.spec.ts (1)
531-531: Test binding syntax correctly updated for new BTabs API.The v-model binding syntax has been properly updated to reflect the new BTabs component API:
v-model:index="index"for explicit index bindingv-model="id"for the default model (active tab ID)This change aligns with the broader refactor where
modelValuenow represents the active tab's ID as a string rather than an index number.apps/docs/src/docs/components/demo/TabsDynamic.vue (2)
7-17: Improved UX by moving close button to tab title.The close button placement in the tab title is more intuitive and follows common UI patterns. The styling with negative margin and link classes provides appropriate visual integration.
23-31: Enhanced new tab button styling and accessibility.The change from
BNavItemtoBButtonwith proper Bootstrap utility classes improves the button appearance. Therole="presentation"attribute is correctly applied for accessibility.packages/bootstrap-vue-next/src/components/BAccordion/accordion.spec.ts (4)
4-4: Correct import addition for new test functionality.The import of
BAccordionItemis properly added to support the new test cases.
42-47: Good test coverage for the new free prop.Simple test verifies the
freeprop is correctly set and accessible.
49-72: Well-structured complex test component for multi-selection testing.The test component properly demonstrates the new dual v-model binding pattern with both
modelValueandindexprops, which aligns with the new accordion API.
105-133: Comprehensive test for multi-selection behavior.The test thoroughly verifies the array-based state management when
free=true, including:
- Initial state verification
- Adding selections
- Removing selections
- Proper async handling with timeouts and
$nextTickThe test logic correctly validates that both
modelValueandindexarrays stay synchronized.packages/bootstrap-vue-next/src/utils/keys.ts (4)
1-1: Appropriate import addition for enhanced type safety.The
ComputedRefimport is correctly added to support the updated injection key interfaces.
58-58: Enhanced type safety for tab registration.The change from
Readonly<Ref<TabType>>toComputedRef<TabType>provides better type safety and aligns with the component refactoring where tabs use computed properties for registration.
98-98: Proper type expansion for multi-selection support.The
openItemtype correctly expanded fromstring | undefinedtostring | string[] | undefinedto support multiple open accordion items whenfree=true.
104-106: Well-designed new methods for accordion item management.The three new methods provide comprehensive accordion item lifecycle management:
setCloseItem: For closing specific itemsregisterAccordionItem: For registering items with DOM element referenceunregisterAccordionItem: For cleanup when items are removedThe method signatures are appropriate and include necessary type constraints.
packages/bootstrap-vue-next/src/types/Tab.ts (1)
1-18: Breaking changes are properly aligned with the tabs refactoring goals.The interface changes are well-structured and improve type safety. The addition of
internalIdfor robust internal identification and the simplification from function-returned values to direct values make the API cleaner.Note for migration guide: These are breaking changes that will require updates to existing code:
- New required
internalIdproperty- Properties changed from functions to direct values
onClickhandler signature changeelproperty now uses Vue'sRefwrapperapps/docs/src/data/components/accordion.data.ts (1)
23-82: Well-designed API for multi-selection accordion support.The addition of
indexandmodelValueprops with support for both single and array values provides excellent flexibility. The corresponding events are properly documented and align with Vue's v-model conventions.apps/docs/src/data/components/tabs.data.ts (2)
19-24: Good prop rename for clarity.The rename from
lazyOncetounmountLazybetter describes the behavior - content is unmounted when inactive rather than cached.
68-73:❓ Verification inconclusive
Verify consistency between index and modelValue handling.
The switch from
activeIdtoindexprop and changingmodelValuefrom number to string represents a significant API change. The description notes that "Id (modelValue) takes priority over index" which is good for clarity.Ensure examples and tests are updated to reflect these breaking changes:
Also applies to: 134-137
🏁 Script executed:
#!/bin/bash # Description: Check for outdated usage patterns that need updating # Search for activeId prop usage that needs to be updated rg -A 3 'activeId\s*[:=]' --type vue --type ts # Search for v-model usage on BTabs that might expect numbers ast-grep --pattern 'BTabs v-model=$_'Length of output: 116
🏁 Script executed:
#!/bin/bash # Retry searching for outdated activeId usage in .vue and .ts files rg -A 3 'activeId\s*[:=]' -g '*.vue' -g '*.ts' # Search for BTabs v-model usage in .vue files rg -A 3 '<BTabs[^>]*v-model' -g '*.vue'Length of output: 4331
Update BTabs examples and tests for the new index/modelValue API
We still have live usages of the old
activeIdbinding alongsidev-model:index. Please verify and update these examples and any tests to align with the switch fromactiveId→index(number) and the change ofmodelValueto a string ID:• apps/playground/src/components/Comps/TTabs.vue
– Remove or adjustv-model:active-id="activeId"if you no longer intend to demonstrate ID-based activation, or confirm it now binds to a string.
– Ensurev-model:index="tabIndex"continues to work with numeric indexes.• apps/docs/src/docs/components/demo/TabsExternalControls.vue
– Already usesv-model:index; confirm examples reflect the TypeScript change (index: number).• apps/docs/src/docs/components/demo/TabsCustomLinks.vue
– Same as above: verifyv-model:indexexamples and descriptions mention the index-based prop.• packages/bootstrap-vue-next/src/components/BTabs
– Check any unit or integration tests under this folder to ensure they expectmodelValue(string ID) and/orindex(number) rather than the oldactiveIdprop.You can re-run these searches to catch any residual patterns:
rg -A3 'activeId' -g '*.vue' -g '*.ts' rg -A3 '<BTabs[^>]*v-model' -g '*.vue'packages/bootstrap-vue-next/src/components/BAccordion/BAccordionItem.vue (2)
125-132: Excellent handling of single and multi-selection modes.The logic correctly checks whether the parent's
openItemis an array to support both single and multiple open accordion items. Good defensive programming with the Array.isArray check.
134-139: Good lifecycle management for accordion item registration.Registering and unregistering the item with the parent ensures proper state management and prevents memory leaks.
packages/bootstrap-vue-next/src/components/BTabs/BTab.vue (2)
138-153: Good defensive programming with disabled state check!Excellent addition of the disabled state check to prevent activating disabled tabs. This improves the component's robustness.
166-173: Well-designed public API exposure.The
activate()anddeactivate()methods provide a clean programmatic interface for external tab control, which enhances the component's flexibility.packages/bootstrap-vue-next/src/components/BAccordion/BAccordion.vue (2)
213-213: Use array destructuring for cleaner code.Minor style improvement as suggested by the linter.
- modelValue.value = modelValue.value[0] + modelValue.value = [modelValue.value][0]Wait, this suggestion doesn't make sense. The current code is correct. The linter might be suggesting:
- modelValue.value = modelValue.value[0] + const [firstValue] = modelValue.value + modelValue.value = firstValueBut the original code is cleaner. You can safely ignore this linting suggestion or disable it for this line.
🧰 Tools
🪛 GitHub Check: test-lint
[warning] 213-213:
Use array destructuring
110-206: Well-implemented bidirectional synchronization logic.The synchronization between
indexandmodelValueis comprehensive and handles edge cases well:
- Validates indices before updating
- Supports both single and multi-selection modes
- Uses
nextTickappropriately for invalid values- Implements
areEqualfor proper array comparisonThis is a solid implementation of complex state management.
🧰 Tools
🪛 GitHub Check: test-lint
[warning] 156-156:
Unexpected console statementpackages/bootstrap-vue-next/src/components/BTabs/BTabs.vue (1)
200-200: Temporary IDs could cause issues with external referencesThe
premount-${index}temporary IDs might cause issues if external code tries to reference tabs before they're fully mounted.Consider using a more robust ID generation strategy or document this behavior clearly.
packages/bootstrap-vue-next/src/components/BAccordion/BAccordion.vue
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
I believe this PR addresses the issues I raised in #2383. There is some cleanup remaining in the migration-guide and docs files, I am happy to handle that after this PR goes through.
Aside from the minor comments I added, it looks like the AI is raising flags on race conditions in a couple of places, I didn't take the time to get my head wrapped around them, but let me know if you'd like a second opinion on them and I'll try to grab some time to do that.
@dwgray @VividLemon I'd like your input on the race conditions. Especially I got some hangs with infinite loops when I prevented inside the activate-tab emit, but they should work in the committed version. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/bootstrap-vue-next/src/components/BAccordion/BAccordion.vue (1)
90-93: Console statements should be replaced with proper error handling.This matches a previous review comment that hasn't been addressed. Console statements should not be left in production code.
Apply the suggested fix from the previous review:
- if (next.length !== modelValue.value.length) { - if (import.meta.env.DEV) { - console.warn('[BAccordion] Unknown item id in v-model:', modelValue.value) - } - } + if (next.length !== modelValue.value.length) { + if (import.meta.env.DEV) { + console.warn('[BAccordion] Unknown item id in v-model:', modelValue.value) + } + // Optionally emit an event for production error tracking + // emit('error', { type: 'unknown-item-id', value: modelValue.value }) + }🧰 Tools
🪛 GitHub Check: test-lint
[warning] 91-91:
Unexpected console statement
🧹 Nitpick comments (2)
apps/docs/src/docs/components/tabs.md (1)
241-243: Consider style improvements for better readability.Based on static analysis feedback, consider these minor style improvements:
- - `newTabId`: The id of the tab that is going to be activated - - `prevTabId`: The id of the currently active tab + - `newTabId`: The id of the tab that will be activated + - `prevTabId`: The id of the currently active tab🧰 Tools
🪛 LanguageTool
[uncategorized] ~241-~241: Loose punctuation mark.
Context: ...itted with five arguments: -newTabId: The id of the tab that is going to be a...(UNLIKELY_OPENING_PUNCTUATION)
[style] ~241-~241: Use ‘will’ instead of ‘going to’ if the following action is certain.
Context: ...: -newTabId: The id of the tab that is going to be activated -prevTabId: The id of t...(GOING_TO_WILL)
[style] ~243-~243: Use ‘will’ instead of ‘going to’ if the following action is certain.
Context: ...newTabIndex: The index of the tab that is going to be activated -prevTabIndex`: The inde...(GOING_TO_WILL)
packages/bootstrap-vue-next/src/components/BTabs/BTabs.vue (1)
342-395: Consider minor optimization in nextIndex calculation.The activeIndex watcher logic is well-structured for handling complex activation scenarios. However, there's a small redundant calculation:
const index = nextIndex(newValue, newValue > oldValue ? 1 : -1) if (index !== newValue) { previousIndex = oldValue activeIndex.value = index return } const tabEvent = new BvEvent('activate-tab', {cancelable: true}) emit( 'activate-tab', tabs.value[index]?.id, tabs.value[previousIndex ?? oldValue]?.id, index, previousIndex ?? oldValue, tabEvent )The logic is correct, but consider storing
previousIndex ?? oldValuein a variable to avoid repetition.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/docs/src/data/components/tabs.data.ts(4 hunks)apps/docs/src/docs/components/tabs.md(1 hunks)packages/bootstrap-vue-next/src/components/BAccordion/BAccordion.vue(3 hunks)packages/bootstrap-vue-next/src/components/BTabs/BTab.vue(6 hunks)packages/bootstrap-vue-next/src/components/BTabs/BTabs.vue(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/bootstrap-vue-next/src/components/BTabs/BTab.vue
🧰 Additional context used
🧠 Learnings (2)
apps/docs/src/data/components/tabs.data.ts (1)
Learnt from: xvaara
PR: bootstrap-vue-next/bootstrap-vue-next#2716
File: packages/bootstrap-vue-next/src/components/BTabs/BTabs.vue:384-404
Timestamp: 2025-05-28T07:57:19.884Z
Learning: In BTabs component (packages/bootstrap-vue-next/src/components/BTabs/BTabs.vue), the activeIndex and activeId watchers are intentionally designed with separation of concerns: activeIndex watcher handles activate-tab event emission and complex validation logic, while activeId watcher is kept simple for synchronization between activeId and activeIndex values only.
packages/bootstrap-vue-next/src/components/BTabs/BTabs.vue (3)
Learnt from: xvaara
PR: bootstrap-vue-next/bootstrap-vue-next#2716
File: packages/bootstrap-vue-next/src/components/BTabs/BTabs.vue:384-404
Timestamp: 2025-05-28T07:57:19.884Z
Learning: In BTabs component (packages/bootstrap-vue-next/src/components/BTabs/BTabs.vue), the activeIndex and activeId watchers are intentionally designed with separation of concerns: activeIndex watcher handles activate-tab event emission and complex validation logic, while activeId watcher is kept simple for synchronization between activeId and activeIndex values only.
Learnt from: xvaara
PR: bootstrap-vue-next/bootstrap-vue-next#2716
File: packages/bootstrap-vue-next/src/components/BTabs/BTabs.vue:240-261
Timestamp: 2025-05-28T07:01:55.082Z
Learning: In BTabs component (packages/bootstrap-vue-next/src/components/BTabs/BTabs.vue), the complex initialization logic with updateInitialActiveIndex and updateInitialActiveId flags is necessary for SSR compatibility. Tab initialization cannot be deferred to mounted lifecycle because tabs must be available for server-side rendering of the initial HTML state.
Learnt from: xvaara
PR: bootstrap-vue-next/bootstrap-vue-next#2716
File: packages/bootstrap-vue-next/src/components/BTabs/BTabs.vue:425-430
Timestamp: 2025-05-28T07:32:45.637Z
Learning: In BTabs component (packages/bootstrap-vue-next/src/components/BTabs/BTabs.vue), the activeIndex synchronization within sortTabs() is necessary and correct. When sorting tabs by DOM position, indices change, so activeIndex must be updated immediately to maintain consistency with activeId. This is not an unwanted side effect but a required consequence of the sorting operation.
🪛 GitHub Check: test-lint
packages/bootstrap-vue-next/src/components/BAccordion/BAccordion.vue
[warning] 214-214:
Use array destructuring
[warning] 91-91:
Unexpected console statement
🪛 LanguageTool
apps/docs/src/docs/components/tabs.md
[uncategorized] ~241-~241: Loose punctuation mark.
Context: ...itted with five arguments: - newTabId: The id of the tab that is going to be a...
(UNLIKELY_OPENING_PUNCTUATION)
[style] ~241-~241: Use ‘will’ instead of ‘going to’ if the following action is certain.
Context: ...: - newTabId: The id of the tab that is going to be activated - prevTabId: The id of t...
(GOING_TO_WILL)
[style] ~243-~243: Use ‘will’ instead of ‘going to’ if the following action is certain.
Context: ...newTabIndex: The index of the tab that is going to be activated - prevTabIndex`: The inde...
(GOING_TO_WILL)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (20)
packages/bootstrap-vue-next/src/components/BAccordion/BAccordion.vue (5)
52-70: VNode tracking implementation looks solid.The slot watching and VNode filtering logic is well-structured. Using
nextTickfor sorting after slot updates ensures DOM is ready.
119-220: Review synchronization logic for potential race conditions.The dual-watcher setup between
indexandmodelValuecreates potential for infinite loops or race conditions, as mentioned in the PR comments. While theareEqualchecks help prevent some redundant updates, the complex conditional logic could still cause issues.Consider these improvements:
- Use a single source of truth with computed values instead of dual watchers
- Add debugging flags to track watcher execution order
- Consider using
watchSyncEffector a state machine patternThe array destructuring on line 214 should also be updated per the linter warning:
- modelValue.value = modelValue.value[0] + [modelValue.value] = [modelValue.value[0]]Would you like me to generate a verification script to test for race conditions or help refactor this synchronization logic?
🧰 Tools
🪛 GitHub Check: test-lint
[warning] 214-214:
Use array destructuring
206-206: Deep watching is appropriate here.The deep watch on
modelValueis necessary to detect changes in array values, and since the data is typically simple (strings/arrays of strings), the performance impact should be minimal.
222-272: Injection implementation handles edge cases well.The
setOpenItemandsetCloseItemmethods properly handle all combinations of free mode, array/single values, and state transitions. The registration system with automatic sorting when all items are registered is well-designed.
112-117: Simple and effective equality utility.The
areEqualfunction appropriately handles both array and primitive comparisons for preventing unnecessary watcher updates.apps/docs/src/docs/components/tabs.md (1)
239-246: Documentation accurately reflects the enhanced event signature.The updated documentation correctly describes the expanded
activate-tabevent with five arguments, providing clear descriptions for each parameter.🧰 Tools
🪛 LanguageTool
[uncategorized] ~241-~241: Loose punctuation mark.
Context: ...itted with five arguments: -newTabId: The id of the tab that is going to be a...(UNLIKELY_OPENING_PUNCTUATION)
[style] ~241-~241: Use ‘will’ instead of ‘going to’ if the following action is certain.
Context: ...: -newTabId: The id of the tab that is going to be activated -prevTabId: The id of t...(GOING_TO_WILL)
[style] ~243-~243: Use ‘will’ instead of ‘going to’ if the following action is certain.
Context: ...newTabIndex: The index of the tab that is going to be activated -prevTabIndex`: The inde...(GOING_TO_WILL)
apps/docs/src/data/components/tabs.data.ts (5)
19-24: LGTM! Property rename aligns with component implementation.The
lazyOncetounmountLazyrename with updated description correctly reflects the new lazy rendering behavior where content is unmounted when inactive.
68-73: LGTM! Index prop introduction supports dual-model architecture.The new
indexprop with number type and -1 default provides explicit control over tab selection by index, complementing the ID-basedmodelValue.
135-138: LGTM! ModelValue type change supports ID-based selection.Changing
modelValuefrom number to string enables ID-based tab selection, which is more semantically meaningful and stable than index-based selection.
209-220: LGTM! New index event enables dual binding support.The
update:indexevent properly supportsv-model:indexbinding for index-based control, complementing the ID-basedmodelValue.
226-251: Enhanced event signature provides comprehensive context.The expanded
activate-tabevent with both ID and index parameters for new and previous tabs offers developers complete context for handling tab activation.packages/bootstrap-vue-next/src/components/BTabs/BTabs.vue (9)
33-46: LGTM! Template updates align with component refactor.The template changes correctly use
internalIdfor keys and remove function calls from prop bindings, improving performance by eliminating unnecessary computations on each render.
87-142: Well-structured dual-model implementation.The separation of
activeIndexandactiveIdmodels with proper defaults provides a clean API that supports both index-based and ID-based tab control.
147-165: Reactive tab elements tracking improves state management.The
tabElementsArraytracking with proper slot watching ensures the component responds correctly to dynamic tab changes, which is essential for proper DOM synchronization.
225-264: Complex initialization logic necessary for SSR compatibility.Based on previous discussions, this initialization complexity with flags is required for SSR compatibility where tabs must be available before the mounted lifecycle. The logic properly handles the various initialization states.
290-304: Simplified click handler improves maintainability.The updated click handler correctly processes the onClick callback and sets
activeIndexdirectly, eliminating the need for the previousactivateTabfunction complexity.
397-422: ActiveId watcher maintains clean separation of concerns.Based on retrieved learnings, this watcher is intentionally kept simple for synchronization purposes while the activeIndex watcher handles complex event logic. The implementation correctly maintains this architectural separation.
424-441: Tab registration handles dynamic updates effectively.The registration logic properly handles both initial registration and updates, with appropriate sorting triggered only when needed. The initialization flag prevents premature sorting.
443-448: ActiveIndex synchronization in sortTabs is architecturally sound.Based on retrieved learnings, this synchronization is necessary and correct. When sorting changes tab positions, activeIndex must be updated immediately to maintain consistency with activeId.
464-471: ActivateTab method provides clean programmatic interface.The method correctly uses
internalIdfor tab lookup and delegates to the activeIndex model, maintaining consistency with the component's state management approach.
I just spent some time getting my head wrapped around the watchers for I'm good with this going through. |
fixes #2497
fixes #2705
Describe the PR
A clear and concise description of what the pull request does.
Small replication
A small replication or video walkthrough can help demonstrate the changes made. This is optional, but can help observe the intended changes. A mentioned issue that contains a replication also works.
PR checklist
What kind of change does this PR introduce? (check at least one)
fix(...)feat(...)fix(...)docs(...)The PR fulfills these requirements:
CHANGELOGis generated from these messages, and determines the next version type. Pull requests that do not follow conventional commits or do not have an override will be deniedSummary by CodeRabbit
New Features
Enhancements
unmountLazyprop.v-model:indexbindings for clearer state management.Bug Fixes
Documentation
Tests