fix(BTabs): corrent classes on ssr #2664
Conversation
|
|
|
""" WalkthroughThe changes update the initialization and tab registration logic in the Changes
Sequence Diagram(s)sequenceDiagram
participant Parent as BTabs
participant Child as BTab
Note over Child: Setup phase
Child->>Parent: Register and activate tab synchronously (if parentData exists)
Child->>Parent: onMounted calls parentData.sortTabs() (if parentData exists)
Note over Parent: Compute tabs
alt Internal tabs registered
Parent->>Parent: Use tabsInternal.value for tabs
else No internal tabs
Parent->>Parent: Extract tab info from slot content fallback
end
Parent->>Child: Set active tab state
Assessment against linked issues
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
There was a problem hiding this comment.
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
packages/bootstrap-vue-next/src/components/BTabs/BTab.vue:84
- Moving the tab registration out of the onMounted hook may lead to a timing issue if parentData isn't fully initialized. Consider reverting to using onMounted to ensure the parent's data is available before registration.
if (parentData) {
|
@VividLemon the way we register the tabs, they arent there on the first pass so this is kinda a hack to get them working in the setup context for ssr. |
commit: |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
packages/bootstrap-vue-next/src/components/BTabs/BTabs.vue (2)
156-178: Consider in‑place function references for class helpers
titleItemClass,titleLinkAttrs,titleLinkClassare wrapped in arrow functions so the consuming template still calls them with().
If these props are already functions you could pass them through unchanged to save an indirection:- titleItemClass: () => tab.props?.titleItemClass, - titleLinkAttrs: () => tab.props?.titleLinkAttrs, - titleLinkClass: () => tab.props?.titleLinkClass, + titleItemClass: tab.props?.titleItemClass, + titleLinkAttrs: tab.props?.titleLinkAttrs, + titleLinkClass: tab.props?.titleLinkClass,Not critical, but it reduces one level of call‑stack noise.
367-368: Injection key updated — document the new contractThe additional
sortTabsmethod is now part of the public injections API.
Please add a brief JSDoc block (or update any existing type definition) so downstream component authors know it exists.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/bootstrap-vue-next/src/components/BTabs/BTabs.vue(6 hunks)packages/bootstrap-vue-next/src/utils/flattenFragments.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/bootstrap-vue-next/src/utils/flattenFragments.ts
🧰 Additional context used
🪛 GitHub Check: test-lint
packages/bootstrap-vue-next/src/components/BTabs/BTabs.vue
[warning] 147-147:
Unexpected console statement
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (2)
packages/bootstrap-vue-next/src/components/BTabs/BTabs.vue (2)
84-86: 👍 Necessary imports added
flattenFragmentsandBTabare now brought into scope which makes the SSR‑fallback logic possible. No issues spotted here.
314-315: 👍 CallingfindActive()after registration keeps state in syncTriggering
findActive()right after the first sort ensures the correct tab is marked active even during rapid dynamic inserts.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
packages/bootstrap-vue-next/src/components/BTabs/BTabs.vue (1)
239-249: 🛠️ Refactor suggestionAvoid calling
onClick()twice & make the flow safer
handleClick()evaluatesonClick()once in the guard and a second time for execution.
If the wrapper allocates fresh closures or has side‑effects, this double invocation is undesirable.
Capturing the result once also simplifies the checks:const handleClick = (event: Readonly<MouseEvent>, index: number) => { activateTab(index) - if ( - index >= 0 && - !tabs.value[index].disabled && - tabs.value[index]?.onClick && - typeof tabs.value[index].onClick === 'function' && - typeof tabs.value[index].onClick() === 'function' - ) { - tabs.value[index].onClick?.()(event) - } + if (index < 0) return + const tab = tabs.value[index] + if (tab.disabled || typeof tab.onClick !== 'function') return + const clickHandler = tab.onClick() + if (typeof clickHandler === 'function') { + clickHandler(event) + } }
♻️ Duplicate comments (1)
packages/bootstrap-vue-next/src/components/BTabs/BTabs.vue (1)
142-146:⚠️ Potential issue
filterpredicate is still not null‑safe – same crash vector as beforePast feedback already highlighted that
flattenFragmentscan yieldundefinedentries, yet the predicate still dereferencestab.typeunguarded.
A single empty node in the slot will throw at render time (TypeError: Cannot read properties of undefined (reading 'type')).- const tabElementsArray = (Array.isArray(tabElements) ? tabElements : [tabElements]).filter( - (tab) => tab.type === BTab - ) + const tabElementsArray = (Array.isArray(tabElements) ? tabElements : [tabElements]).filter( + (tab) => tab && tab.type === BTab + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/bootstrap-vue-next/src/components/BTabs/BTab.vue(1 hunks)packages/bootstrap-vue-next/src/components/BTabs/BTabs.vue(7 hunks)packages/bootstrap-vue-next/src/types/Tab.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/bootstrap-vue-next/src/components/BTabs/BTab.vue
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
|
@VividLemon do you see any other way how we could get the ssr context working? I'm not a fan of using the VNode structure, but I couldn't figure another way to fix this. |
Due to the immense overhead of this component, it would seem better to introduce another library to handle this, like Radix-vue. |
I don't really see how that would make any sense. They have a totally different way of doing tabs, totally different api. And we'd have the same problems with it if we don't change the api (ie. use another component for the tab button, and another for tab content) and leave the problems to the user. |
* upstream/main: chore: release main (bootstrap-vue-next#2690) docs(BProgress): Parity pass (bootstrap-vue-next#2689) fix(BTableSimple): fixed and nobordercollapse to work fixes bootstrap-vue-next#2685 docs: fix incorrect references and missed script sections (bootstrap-vue-next#2686) docs: implement on this page expand/collapse with useScrollspy (bootstrap-vue-next#2679) chore: release main (bootstrap-vue-next#2683) feat(BTable): implement 'fixed' and 'noBorderCollapse' props (bootstrap-vue-next#2681) chore: release main (bootstrap-vue-next#2678) Update package.json fix(BFormSelect): prevent options with label from being treated as groups (bootstrap-vue-next#2666) fix: patch regression issue in bootstrap-vue-next#2665 (bootstrap-vue-next#2670) Update release-main.yaml chore: release main (bootstrap-vue-next#2660) chore: update depencies fix(BTabs): corrent classes on ssr (bootstrap-vue-next#2664) Changes to public composables (bootstrap-vue-next#2425) docs(BTable): parity pass (bootstrap-vue-next#2669)
fix(BTabs): corrent classes on ssr fix(BTabs): fix another recursion error
commit 2a9e30b Author: Jukka Raimovaara <roska@mentalhouse.fi> Date: Thu May 15 18:24:07 2025 +0300 doc data commit 08c89fd Author: Jukka Raimovaara <roska@mentalhouse.fi> Date: Thu May 15 17:57:29 2025 +0300 feat(BPopover): add titleClass and bodyClass, remove unneeded customClass prop since class is inherited to the same place commit 90b578d Author: Jukka Raimovaara <roska@mentalhouse.fi> Date: Wed May 14 11:39:42 2025 +0300 feat(BToast): add noProgress prop, make progress show as default if modelValue is number. fix(useToastController): if using the deprecated show method the countdown didn't start. commit dc85d94 Author: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Date: Sun May 11 09:53:25 2025 -0500 chore: release main (bootstrap-vue-next#2690) Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> commit 070cb8c Author: David W. Gray <dwgray67@hotmail.com> Date: Sun May 11 07:52:30 2025 -0700 docs(BProgress): Parity pass (bootstrap-vue-next#2689) commit c61f532 Author: Thierry Blind <tbl0605@gmail.com> Date: Sun May 11 16:52:14 2025 +0200 fix(BTableSimple): fixed and nobordercollapse to work fixes bootstrap-vue-next#2685 commit beae36f Author: David W. Gray <dwgray67@hotmail.com> Date: Sun May 11 07:43:58 2025 -0700 docs: fix incorrect references and missed script sections (bootstrap-vue-next#2686) commit 34432d9 Author: David W. Gray <dwgray67@hotmail.com> Date: Sun May 11 07:42:02 2025 -0700 docs: implement on this page expand/collapse with useScrollspy (bootstrap-vue-next#2679) commit ce869f0 Author: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Date: Wed May 7 11:16:08 2025 -0500 chore: release main (bootstrap-vue-next#2683) Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> commit 9486276 Author: Mohamed Nasri <51300752+mhn147@users.noreply.github.com> Date: Wed May 7 09:44:38 2025 -0600 feat(BTable): implement 'fixed' and 'noBorderCollapse' props (bootstrap-vue-next#2681) commit a4a9294 Author: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Date: Mon May 5 09:16:09 2025 -0500 chore: release main (bootstrap-vue-next#2678) Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> commit 0165e94 Author: Jukka Raimovaara <roska@mentalhouse.fi> Date: Mon May 5 16:24:04 2025 +0300 Update package.json commit c1645a9 Author: Rajitha <rajithaeye@gmail.com> Date: Wed Apr 30 23:49:23 2025 +0530 fix(BFormSelect): prevent options with label from being treated as groups (bootstrap-vue-next#2666) commit 59ddc39 Author: Thierry Blind <tbl0605@gmail.com> Date: Wed Apr 30 20:17:34 2025 +0200 fix: patch regression issue in bootstrap-vue-next#2665 (bootstrap-vue-next#2670) commit d82091b Author: Jukka Raimovaara <roska@mentalhouse.fi> Date: Wed Apr 30 06:01:10 2025 +0300 Update release-main.yaml commit 31cb4bf Author: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Date: Wed Apr 30 04:51:31 2025 +0300 chore: release main (bootstrap-vue-next#2660) Co-Authored-By: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> commit 6733770 Author: Jukka Raimovaara <roska@mentalhouse.fi> Date: Wed Apr 30 05:15:02 2025 +0300 chore: update depencies commit 2b37c18 Author: Jukka Raimovaara <roska@mentalhouse.fi> Date: Wed Apr 30 04:36:46 2025 +0300 fix(BTabs): corrent classes on ssr (bootstrap-vue-next#2664) fix(BTabs): corrent classes on ssr fix(BTabs): fix another recursion error commit 99718eb Author: Jukka Raimovaara <roska@mentalhouse.fi> Date: Wed Apr 30 04:20:00 2025 +0300 Changes to public composables (bootstrap-vue-next#2425) feat(BAlert)!: make act like toast, useShowHide. feat(useShowHide): create triggerRegistry for adding external triggers (like in vBToggle) fix: type popoverController fix(useShowHide): focustrap off at the begining of leave, pass down the trigger to other hide emits. fix(vBToggle): keep track of targets fix(BPopover)!: change prop content to body to align with other components fix(BTooltip)!: change prop content to body to align with other components feat(usePopoverController): allow more options fix(vBToggle): find late components, ie. inside ClientOnly fix(useModalController)!: move props to main level, add slots feat(usePopoverController): add slots feat(useToastController)!: remove props obj, the parameters are flat now. Add slots, rename pos -> position feat(useShowHide): show returns a promise, resolve on show or hide. feat(useToggle): toggle any show/hide component feat!: controller composables functions return promise, with chainable functions feat(useModalController): add support for using syntax in ts feat(BModal): add okClass and cancelClass to add classes to the buttons. feat(useModalController)!: change of api, check the docs fix: inline functional style to show toast,modal and dropdown feat(useToggle): add trigger to promise resolve on hide. fix(BCarousel): fix v-for updates commit 340edfd Author: David W. Gray <dwgray67@hotmail.com> Date: Mon Apr 28 18:39:44 2025 -0700 docs(BTable): parity pass (bootstrap-vue-next#2669) commit 4dd6c89 Author: Jukka Raimovaara <roska@mentalhouse.fi> Date: Mon Apr 28 22:46:31 2025 +0300 fix(BDropdown): don't calulcate the position when dropdown is not shown.
fixes #2652
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