Skip to content

fix(directives): Robustness fixes for directives #2906

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
dwgray merged 7 commits into from
Nov 14, 2025
Merged

fix(directives): Robustness fixes for directives #2906

dwgray merged 7 commits into from
Nov 14, 2025

Conversation

@dwgray
Copy link
Member

@dwgray dwgray commented Nov 10, 2025 •
edited by coderabbitai bot
Loading

Describe the PR

I spent some time in GitHub Copilot today exploring whether to keep directives or not. Below is what we came up with along with the rationale.

This code in this PR contains the stability fixes that we landed on as high priority if we intend to support directives moving forward (we probably want them even if we are going to deprecate anytime after our initial official release).

I'm definitely leaning pretty hard on AI for this, so I would appreciate @xvaara and @VividLemon's eyes on it to check for hallucinations. But if this looks reasanbly sane, here's what I'd like to do:

  • Get this PR integrated
  • Do a parity pass on the directives including getting all of the samples working
  • If I don't run into anything blocking, call support for directives in v1 official, but do a pass on the docs to make sure that we're noting that directives are syntactic sugar and continue to nudge people to using components/composables where possible.

Also note that this PR depends on the internal $.uid. While undocumented, it is described in vue's typescript and Vuetify uses it in basically the same way. If everything else looks good, but you find the use of $.uid unnaceptable, I believe there is a reasonable way to use getId() to make this work as well.

Executive Summary

Recommendation: KEEP DIRECTIVES with stability improvements

After comprehensive analysis of 77 GitHub issues, codebase review, comparison with Vuetify's approach, and Vue.js best practices, I recommend continuing to support directives while implementing the stability improvements detailed below.

Key Points

✅ DX Benefits Are Real: v-b-tooltip="text" is significantly cleaner than <BTooltip><template #activator>...</template></BTooltip>

✅ Migration Path: Helps users migrate from Bootstrap-Vue v2

✅ Proven Pattern: Vuetify successfully uses similar directive patterns in production

✅ Issues Are Fixable: Most stability problems stem from implementation details, not the directive pattern itself

⚠️ Vue Best Practice Violation: Vue.js docs state directives should only be for DOM manipulation, but this is more guideline than hard rule

Issue Analysis

Issues Reviewed: 77 total (9 open, 68 closed)

Critical Issues (Fixed in this PR):

  1. Race Conditions in BToggle (BToggle directive producing console error when unmounted quickly #2856)

    • Problem: Async loop checking for targets could continue after unmount
    • Fix: Check dataset.bvtoggle before each iteration and registration

  2. Memory Leaks in Tooltip/Popover

    • Problem: setTimeout(() => div?.remove(), 0) could fire after component destruction
    • Fix: Use queueMicrotask() with destruction flag check

  3. Tooltip Reactivity (vBTooltip: updating tooltip title to an empty string does not update tooltip text #2781, Directive Tooltips disappear when their content is reactive and are updated updated #2454, BTooltip directive not updating content when title changes #2273)

    • Problem: Content updates not always triggering re-render
    • Fix: Improved updated hook logic with proper cleanup when content becomes empty

  4. Multi-Instance Collisions (New)

    • Problem: Multiple components using same directive would overwrite each other's state
    • Fix: Implemented UID namespacing pattern from Vuetify (using binding.instance!.$.uid)

Medium Priority Issues:

  1. Performance with Many Directives (Performance dropped when use many tooltip directives or popovers #2232)

    • Current: Each directive creates a component instance
    • Potential Future Optimization: Shared instance pool

  2. DevTools Pollution (Tooltips and Popovers clutter Vue DevTools with unnamed "App" entries #2700)

    • Tooltips/popovers create unnamed "App" entries
    • Can be improved with proper component naming

  3. Placeholder Span Layout Issues (<span>-element placed by v-b-tooltip directive shifts content in flex container #2225, Ability of adding option to prevent span (_placeholder ) element rendered (tooltip directive/popover) #2228)

    • Directives insert <span> elements that affect flexbox layout
    • Documented limitation; use component form for layout-sensitive cases

Low Priority Issues:

  1. Lazy-loaded Tab Incompatibility (Cannot add tooltip to lazy-loaded BTab #2804)
    • Edge case with dynamic content
    • Can be addressed with better lifecycle handling

Vue.js Best Practices Assessment

Official Guidance

From Vue.js documentation:

"Custom directives should only be used when the desired functionality can only be achieved via direct DOM manipulation."

Our Directive Usage

Directive Purpose Compliant? Assessment
v-b-tooltip Creates tooltip component instance ❌ No Exceeds DOM manipulation scope
v-b-popover Creates popover component instance ❌ No Exceeds DOM manipulation scope
v-b-toggle Registers event listeners + state management ❌ No Beyond simple DOM manipulation
v-b-modal Alias for v-b-toggle ❌ No Same as v-b-toggle
v-b-scrollspy Uses IntersectionObserver for DOM tracking ✅ Yes Appropriate use case
v-b-color-mode Sets data attribute ✅ Yes Appropriate use case

Verdict: While v-b-tooltip, v-b-popover, and v-b-toggle technically violate Vue's stated best practice, this is more of a guideline violation than a hard rule. The pattern is defensible given:

  1. Real DX benefits
  2. Community expectations (migration from BSV v2)
  3. Precedent in major libraries (Vuetify)
  4. Technical feasibility (proven to work when implemented correctly)

Comparison with Vuetify

What Vuetify Does

Vuetify does use directives in similar ways:

// From vuetify/src/directives/tooltip/index.ts
export const Tooltip = useDirectiveComponent<TooltipDirectiveBinding>(VTooltip, (binding) => {
  return {
    activator: 'parent',
    location: binding.arg?.replace('-', ' '),
    text: typeof binding.value === 'boolean' ? undefined : binding.value,
  }
})

Key Differences:

  1. Specialized Helper: useDirectiveComponent handles provides/context properly
  2. UID-based Tracking: Prevents conflicts between directive instances
  3. Explicit Documentation: They acknowledge it's a convenience wrapper
  4. Simpler Transformation: Less complex logic in directive hooks

Lessons Learned from Vuetify

✅ Our code already uses the findProvides pattern from Vuetify
✅ We should adopt their UID-based tracking approach
✅ We should document directives as "convenience wrappers"
⚠️ We have more complex logic that needs better protection against race conditions

Stability Issues Found

1. Race Conditions in BToggle (FIXED)

Before:

while (count < 5 && (el as HTMLElement).dataset.bvtoggle) {
  const showHide = showHideMap?.value.get(targetId)
  if (!showHide) {
    count++
    await new Promise((resolve) => setTimeout(resolve, 100))
    if (count < 4) continue // Only checked count, not mount state!
    console.warn(`[v-b-toggle] Target with ID ${targetId} not found`)
    break
  }
  // ...
}

After:

while (count < maxAttempts) {
  // Check if element is still mounted before each iteration
  if (!(el as HTMLElement).dataset.bvtoggle) {
    return // Element was unmounted, stop trying
  }

  const showHide = showHideMap?.value.get(targetId)
  if (!showHide) {
    count++
    if (count < maxAttempts) {
      await new Promise((resolve) => setTimeout(resolve, delayMs))
      continue
    }
    // Only warn if element is still mounted after all attempts
    if ((el as HTMLElement).dataset.bvtoggle) {
      console.warn(
        `[v-b-toggle] Target with ID ${targetId} not found after ${maxAttempts * delayMs}ms`
      )
    }
    break
  }

  // Final check before registration
  if (!(el as HTMLElement).dataset.bvtoggle) return

  // Register...
}

2. Memory Leaks in Tooltip/Popover (FIXED)

Before:

export const unbind = (el: ElementWithPopper) => {
  const div = el.$__element
  if (div) render(null, div)
  setTimeout(() => {
    div?.remove() // Could fire after element recreated!
  }, 0)
  delete el.$__element
}

After:

export const unbind = (el: ElementWithPopper) => {
  const div = el.$__element
  if (!div) return

  // Mark as being destroyed to prevent race conditions
  el.$__destroying = true

  // Unmount Vue component immediately
  render(null, div)

  // Use microtask instead of setTimeout(0) for more predictable cleanup
  queueMicrotask(() => {
    // Only remove if still marked for destruction (not recreated)
    if (el.$__destroying) {
      div.remove()
      delete el.$__element
      delete el.$__binding
      delete el.$__destroying
    }
  })
}

Benefits:

  • Prevents removal of recreated elements
  • More predictable timing with queueMicrotask()
  • Proper cleanup flags

3. Improved Tooltip/Popover Reactivity (FIXED)

Before:

updated(el, binding, vnode) {
  const isActive = resolveActiveStatus(binding.value)
  if (!isActive) return  // Left zombie tooltip!

  const text = resolveContent(binding.value, el)
  if (!text.body && !text.title) return  // Left zombie tooltip!

  // ...
}

After:

updated(el, binding, vnode) {
  const isActive = resolveActiveStatus(binding.value)

  // If inactive, clean up existing tooltip
  if (!isActive) {
    if (el.$__element) {
      unbind(el)
    }
    return
  }

  const text = resolveContent(binding.value, el)
  if (!text.body && !text.title) {
    // Clean up if no content
    if (el.$__element) {
      unbind(el)
    }
    return
  }

  // Prevent race conditions during update
  if (el.$__destroying) return

  // ...
}

4. Multi-Instance Collisions (FIXED)

Problem:

When multiple component instances used the same directive on the same or different elements, they would overwrite each other's state:

// Before - Global state per element
el.$__binding = JSON.stringify([binding.modifiers, binding.value])
el.$__destroying = false

// Component A and Component B both set el.$__binding
// Component B's value overwrites Component A's!

This caused:

  • False cache hits: Component B skips rendering because it sees Component A's cached binding
  • Race conditions: Shared destroying flag causes cross-component contamination
  • Incorrect behavior: One component's state affects another component's logic

Fix:

Implemented UID namespacing pattern from Vuetify, using binding.instance!.$.uid to isolate state per component instance:

// After - Per-instance state
const uid = binding.instance!.$.uid
el.$__tooltip = el.$__tooltip ?? Object.create(null)
el.$__tooltip![uid] = {
  binding: JSON.stringify([binding.modifiers, binding.value]),
  destroying: false,
}

// Now each component instance has isolated state!
// Component A: el.$__tooltip[123] = { ... }
// Component B: el.$__tooltip[456] = { ... }

Updated TypeScript Types:

export interface ElementWithPopper extends HTMLElement {
  $__element?: HTMLElement
  $__tooltip?: Record<number, {binding: string; destroying: boolean}>
  $__popover?: Record<number, {binding: string; destroying: boolean}>
}

Benefits:

  • Each component instance has isolated state via UID namespace
  • No more false cache hits or cross-instance contamination
  • JSON.stringify optimization still works (per-instance caching)
  • Multiple components can safely use the same directive simultaneously
  • Follows Vuetify's battle-tested pattern exactly

Directives Updated:

  • ✅ BTooltip - Uses el.$__tooltip[uid] namespace
  • ✅ BPopover - Uses el.$__popover[uid] namespace
  • ✅ BScrollspy - Uses el.$__scrollspy[uid] namespace

Implementation Quality

Strengths

✅ Already using Vuetify's findProvides pattern
✅ Good test coverage (1643 tests passing)
✅ Proper cleanup hooks (beforeUnmount)
✅ UID-based instance tracking (prevents multi-instance collisions)
✅ JSON-based change detection (per-instance caching for performance)

Weaknesses (Now Fixed)

✅ Race conditions in async registration loops
✅ Memory leaks from setTimeout cleanup
✅ Incomplete reactivity handling
✅ Multi-instance state collisions

Recommendations

Short-term (Completed in this PR)

✅ Fix race conditions - Added proper mount state checks
✅ Fix memory leaks - Switched to queueMicrotask with destruction flags
✅ Improve reactivity - Clean up when content becomes empty/inactive
✅ Implement UID namespacing - Adopted Vuetify's pattern for multi-instance safety
✅ Add code comments - Document the patterns used

Medium-term (Future Work)

  1. Performance Optimization

    • Consider shared component instance pool for tooltips
    • Reduce overhead when many directives present
    • Monitor performance impact of UID tracking

  2. Documentation

    • Explicitly document directives as "convenience wrappers"
    • Show when to use components vs directives
    • Document layout limitations (span elements)
    • Explain UID namespacing approach for contributors

  3. Testing

    • Add stress tests for rapid mount/unmount
    • Test with many concurrent directives
    • Test race condition scenarios
    • Test multiple instances of same directive

Long-term (Strategic)

  1. Component-First Approach

    • Recommend components as primary API
    • Position directives as convenience feature
    • Clear migration path if directives deprecated later

  2. Alternative Syntax

    • Consider functional components with render functions
    • Explore Vue 3's Composition API patterns
    • Keep directives for simple cases

Decision Matrix

Factor Keep Directives Remove Directives
DX ✅ Better (cleaner syntax) ❌ Worse (verbose)
Migration ✅ Easier from BSV v2 ❌ Breaking change
Vue Best Practices ⚠️ Violates guidance ✅ Compliant
Stability ✅ Fixed with this PR ✅ N/A
Maintenance ⚠️ Requires vigilance ✅ Less code
Community ✅ Matches expectations ❌ Surprising removal
Precedent ✅ Vuetify does same ⚠️ Vue docs say don't

Score: 5 ✅ vs 2 ❌ for keeping directives

Testing Results

All tests pass with the improvements:

Test Files  105 passed | 7 skipped (112)
     Tests  1643 passed | 226 skipped (1869)
  Duration  40.35s

Specific directive test:

✓ src/directives/toggle.spec.ts (1 test) 349ms
  ✓ toggle directive > works with multiple targets, and updates when targets change  347ms

Conclusion

KEEP THE DIRECTIVES but with the implemented stability improvements.

Why Keep Them?

  1. Real DX benefit - Cleaner, more concise syntax
  2. Community expectations - BSV migration path
  3. Proven pattern - Vuetify demonstrates viability
  4. Fixed issues - Stability problems were implementation bugs, not pattern flaws
  5. Flexible approach - Components available for complex cases

Why This Works?

The Vue.js guidance about directives being "only for DOM manipulation" is a best practice, not a hard rule. It's meant to prevent abuse and maintain clarity. In our case:

  • The pattern is well-isolated (only a few directives)
  • The DX benefit is substantial
  • Users have component alternatives for complex cases
  • The implementation is now stable and tested

What Changed?

This PR implements critical stability improvements:

  1. Race condition protection - Proper mount state checks in async loops
  2. Memory leak prevention - queueMicrotask with destruction flags
  3. Improved reactivity - Cleanup when content becomes empty/inactive
  4. UID namespacing - Multi-instance safety following Vuetify's battle-tested pattern
  5. Better error handling - More informative warnings, proper edge case handling

Next Steps

  1. Monitor - Watch for new directive-related issues
  2. Document - Add usage guidelines and limitations
  3. Optimize - Consider performance improvements for many-directive scenarios
  4. Communicate - Be transparent that this is a DX convenience feature

Files Modified

  • packages/bootstrap-vue-next/src/utils/floatingUi.ts - Improved unbind logic, updated TypeScript interfaces for UID namespacing
  • packages/bootstrap-vue-next/src/directives/BTooltip/index.ts - Fixed race conditions, reactivity, and implemented UID namespacing
  • packages/bootstrap-vue-next/src/directives/BPopover/index.ts - Fixed race conditions, reactivity, and implemented UID namespacing
  • packages/bootstrap-vue-next/src/directives/BScrollspy/index.ts - Implemented UID namespacing for multi-instance safety
  • packages/bootstrap-vue-next/src/directives/BToggle/index.ts - Fixed async registration race conditions

Small replication

See bugs referenced above

PR checklist

What kind of change does this PR introduce? (check at least one)

  • Bugfix 🐛 - fix(...)
  • Feature - feat(...)
  • ARIA accessibility - fix(...)
  • Documentation update - docs(...)
  • Other (please describe)

The PR fulfills these requirements:

  • Pull request title and all commits follow the Conventional Commits convention or has an override in this pull request body This is very important, as the CHANGELOG is 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 denied

Summary by CodeRabbit

  • Bug Fixes

    • Improved SSR guards to avoid server-side DOM access and safer cleanup for floating elements.
    • Reduced spurious warnings by checking element mount state before logging.

  • Improvements

    • Standardized directive creation for popovers/tooltips for more consistent behavior.
    • Per-instance UID-scoped directive state to support multiple instances per element.
    • Tighter retry/timing logic and more robust mount/unmount handling.

Copilot AI review requested due to automatic review settings November 10, 2025 22:28
@bolt-new-by-stackblitz
Copy link

bolt-new-by-stackblitz bot commented Nov 10, 2025

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@coderabbitai
Copy link

coderabbitai bot commented Nov 10, 2025 •
edited
Loading

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

Walkthrough

The PR centralizes floating directive behavior via a new createFloatingDirective factory, introduces per-directive UID-scoped instance state, adds SSR guards and safer cleanup for floating UI, updates BPopover/BTooltip to use the factory, namespaces BScrollspy instances per-UID, and hardens BToggle retry/unmount logic. (49 words)

Changes

Cohort / File(s) Summary
Floating Directive Simplification
packages/bootstrap-vue-next/src/directives/BPopover/index.ts, packages/bootstrap-vue-next/src/directives/BTooltip/index.ts		 Replaced bespoke mounted/updated/beforeUnmount implementations with createFloatingDirective(...), delegating lifecycle, prop composition, and binding to the shared factory.
Directive State Infrastructure
packages/bootstrap-vue-next/src/directives/utils.ts		 Added DirectiveInstanceState, per-instance UID helpers (getDirectiveUid, initDirectiveInstance, getDirectiveInstance, cleanupDirectiveInstance, hasBindingChanged, updateBindingCache) and the createFloatingDirective factory to produce UID-scoped directives.
Per-Instance Scrollspy Storage
packages/bootstrap-vue-next/src/directives/BScrollspy/index.ts		 Namespaced scrollspy instances on el.__scrollspy[uid], added SSR guard, cleans up only the instance for current UID, supports string value for content resolution, and adjusted beforeUnmount signature to include binding.
Retry Logic & Mount Guards
packages/bootstrap-vue-next/src/directives/BToggle/index.ts		 Replaced unconditional 400ms wait loop with controlled retry (maxAttempts=5, delayMs=100), added mounted checks before retries/registration, and delayed warning until all retries and mounted checks complete.
Floating UI SSR & Cleanup
packages/bootstrap-vue-next/src/utils/floatingUi.ts		 Added SSR guards around DOM access in resolveContent, guarded bind/unbind flows for server vs client, changed cleanup to microtask-based removal, and expanded ElementWithPopper with UID-scoped __$tooltip / __$popover records and index signature.

Sequence Diagram(s)

sequenceDiagram
    participant App as Vue App
    participant DirectiveFactory as createFloatingDirective
    participant Utils as Directive Utils
    participant Component as Floating Component
    participant DOM as DOM

    rect rgb(220,240,255)
    note over App,DirectiveFactory: Mount
    App->>DirectiveFactory: mounted(el,binding)
    DirectiveFactory->>Utils: getDirectiveUid(binding)
    Utils-->>DirectiveFactory: uid
    DirectiveFactory->>Utils: initDirectiveInstance(el,name,uid,binding)
    DirectiveFactory->>DirectiveFactory: resolveContent & buildProps
    DirectiveFactory->>Component: bind(el, props)
    Component->>DOM: create & mount floating element
    end

    rect rgb(220,255,220)
    note over App,DirectiveFactory: Update
    App->>DirectiveFactory: updated(el,binding)
    DirectiveFactory->>Utils: getDirectiveInstance(el,name,uid)
    DirectiveFactory->>Utils: hasBindingChanged(instance,binding)
    alt changed
        DirectiveFactory->>Component: unbind(el)
        DirectiveFactory->>DirectiveFactory: buildProps & bind(el, props)
        DirectiveFactory->>Utils: updateBindingCache(instance,binding)
    end
    end

    rect rgb(255,230,230)
    note over App,DirectiveFactory: Unmount
    App->>DirectiveFactory: beforeUnmount(el,binding)
    DirectiveFactory->>Component: unbind(el)
    DirectiveFactory->>Utils: cleanupDirectiveInstance(el,name,uid)
    Utils->>DOM: remove instance & mark destroying
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

  • Review focus:
    • createFloatingDirective lifecycle, binding-change detection, and concurrency/race conditions.
    • DirectiveInstanceState and UID scoping to ensure correct isolation and cleanup.
    • SSR guards and server/client fallback behavior in resolveContent and bind/unbind.
    • Interface changes to ElementWithPopper and downstream call sites expecting previous shape.

Possibly related PRs

Suggested reviewers

  • xvaara

Poem

🐇 I hopped in code with eager cheer,
UID pockets kept states near,
Factory binds the floating song,
SSR-safe, tidy and strong,
A little rabbit celebrates this year!

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title 'fix(directives): Robustness fixes for directives' accurately reflects the main objective: implementing stability improvements across multiple directives (BTooltip, BPopover, BToggle, BScrollspy) to fix race conditions, memory leaks, and state management issues.
Description check ✅ Passed The PR description is comprehensive and well-structured, covering the executive summary, issue analysis, stability fixes, recommendations, testing results, and next steps. It addresses the required template sections including a clear description of changes, detailed explanation of why directives are being retained with fixes, and proper checklist completion with conventional commits compliance.
Docstring Coverage ✅ Passed Docstring coverage is 88.89% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a09f28a and 0e66c79.

📒 Files selected for processing (1)
  • packages/bootstrap-vue-next/src/utils/floatingUi.ts (3 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings 
Learnt from: dwgray
Repo: bootstrap-vue-next/bootstrap-vue-next PR: 2777
File: packages/bootstrap-vue-next/CHANGELOG.md:11-11
Timestamp: 2025-08-18T18:20:08.240Z
Learning: For PR #2777 (BSort updates), keep changes scoped to BTable sorting work; unrelated edits like CHANGELOG typos should be deferred to a separate PR/issue.
📚 Learning: 2025-09-12T14:46:49.416Z 
Learnt from: CR
Repo: bootstrap-vue-next/bootstrap-vue-next PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-12T14:46:49.416Z
Learning: Applies to packages/bootstrap-vue-next/src/App.vue : Use packages/bootstrap-vue-next/src/App.vue as a local test area for component changes

Applied to files:

  • packages/bootstrap-vue-next/src/utils/floatingUi.ts
📚 Learning: 2025-05-28T07:01:55.095Z 
Learnt from: xvaara
Repo: bootstrap-vue-next/bootstrap-vue-next PR: 2716
File: packages/bootstrap-vue-next/src/components/BTabs/BTabs.vue:240-261
Timestamp: 2025-05-28T07:01:55.095Z
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.

Applied to files:

  • packages/bootstrap-vue-next/src/utils/floatingUi.ts
📚 Learning: 2025-08-19T23:30:07.062Z 
Learnt from: dwgray
Repo: bootstrap-vue-next/bootstrap-vue-next PR: 2762
File: apps/docs/src/docs/components/tooltip.md:90-94
Timestamp: 2025-08-19T23:30:07.062Z
Learning: In bootstrap-vue-next docs, tooltip.data.ts uses popoverSharedProps('popover') to define props, which includes the 'manual' prop through the shared props structure from popover-shared.ts. The manual prop is available in BTooltip documentation through this prop spreading/inheritance mechanism.

Applied to files:

  • packages/bootstrap-vue-next/src/utils/floatingUi.ts
📚 Learning: 2025-06-26T19:46:19.333Z 
Learnt from: dwgray
Repo: bootstrap-vue-next/bootstrap-vue-next PR: 2762
File: apps/docs/src/docs/components/tooltip.md:0-0
Timestamp: 2025-06-26T19:46:19.333Z
Learning: BTooltip is a very thin wrapper around BPopover in bootstrap-vue-next. There is no separate `useTooltipController` composable - the `usePopoverController` composable can be used to programmatically control both popovers and tooltips.

Applied to files:

  • packages/bootstrap-vue-next/src/utils/floatingUi.ts
📚 Learning: 2025-08-19T23:30:07.062Z 
Learnt from: dwgray
Repo: bootstrap-vue-next/bootstrap-vue-next PR: 2762
File: apps/docs/src/docs/components/tooltip.md:90-94
Timestamp: 2025-08-19T23:30:07.062Z
Learning: In bootstrap-vue-next docs, tooltip.data.ts uses popoverSharedProps('tooltip') to define props, which includes the 'manual' prop through the shared props structure from popover-shared.ts. The manual prop is available in BTooltip documentation through this inheritance mechanism.

Applied to files:

  • packages/bootstrap-vue-next/src/utils/floatingUi.ts
📚 Learning: 2025-04-28T22:48:46.738Z 
Learnt from: xvaara
Repo: bootstrap-vue-next/bootstrap-vue-next PR: 2425
File: packages/bootstrap-vue-next/src/plugins/popoverController/index.ts:138-140
Timestamp: 2025-04-28T22:48:46.738Z
Learning: In bootstrap-vue-next, `PopoverOrchestratorParam` includes an `id` field of type `ControllerKey` through inheritance from base types. This field is declared in the `BvControllerOptions` interface and propagated through the type hierarchy.

Applied to files:

  • packages/bootstrap-vue-next/src/utils/floatingUi.ts
🧬 Code graph analysis (1)
packages/bootstrap-vue-next/src/utils/floatingUi.ts (1)
packages/bootstrap-vue-next/src/types/ComponentProps.ts (1)
  • BPopoverProps (1334-1370)
⏰ 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: build
🔇 Additional comments (4)
packages/bootstrap-vue-next/src/utils/floatingUi.ts (4)

49-66: SSR guards properly prevent DOM access on the server.

The SSR guards correctly prevent getAttribute and setAttribute calls when document is undefined. The control flow ensures that:

  • On the client: title attributes are extracted and processed when no binding value is provided
  • On the server: empty object is returned immediately when no binding value is provided
  • Both paths: binding values are processed normally when provided

110-127: Interface changes properly support UID-namespaced state.

The updated ElementWithPopper interface correctly reflects the per-instance state tracking introduced in this PR:

  • $__tooltip and $__popover are now UID-keyed records tracking per-instance binding state
  • The destroying flag enables guards against operations during cleanup
  • The index signature provides flexibility for dynamic properties

This aligns with the PR's goal of preventing multi-instance collisions by namespacing state per component UID.

134-136: SSR guard correctly prevents DOM manipulation on the server.

The early return when document is undefined prevents calls to createElement, appendChild, and insertBefore that would fail during server-side rendering.

145-170: Cleanup improvements significantly strengthen directive lifecycle.

The refactored unbind implementation introduces several important improvements:

  1. Immediate unmount (line 150): render(null, div) ensures Vue component lifecycle hooks execute synchronously rather than being deferred
  2. Microtask-based cleanup (lines 158-169): Replacing setTimeout(0) with queueMicrotask provides more predictable timing and better performance
  3. Race condition guard (lines 166-168): Only deletes el.$__element if it still references the div being unmounted, preventing issues when bind() is called again before cleanup completes
  4. SSR guard (lines 152-156): Properly skips DOM operations on the server while still cleaning up the reference

These changes align well with the PR's goals of preventing memory leaks and improving robustness.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@pkg-pr-new
Copy link

pkg-pr-new bot commented Nov 10, 2025 •
edited
Loading

bsvn-vite-ts

npm i https://pkg.pr.new/bootstrap-vue-next/bootstrap-vue-next@2906

npm i https://pkg.pr.new/bootstrap-vue-next/bootstrap-vue-next/@bootstrap-vue-next/nuxt@2906

commit: 0e66c79

Copilot AI reviewed Nov 10, 2025
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR implements critical robustness fixes for Vue directives, addressing race conditions, memory leaks, and multi-instance collision issues. The changes introduce UID-based namespacing (following Vuetify's pattern), improved cleanup logic with queueMicrotask, SSR guards, and a refactored architecture using a shared createFloatingDirective factory function.

Key changes:

  • Implemented UID namespacing for directive state management to prevent cross-instance contamination
  • Refactored tooltip/popover directives to use a shared createFloatingDirective factory
  • Added race condition protection in BToggle's async registration loop with mount state checks

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
packages/bootstrap-vue-next/src/utils/floatingUi.ts Added SSR guards, updated unbind to use queueMicrotask, modified TypeScript interfaces for UID namespacing, improved resolveContent SSR handling
packages/bootstrap-vue-next/src/directives/utils.ts Added new UID-based state management utilities, created createFloatingDirective factory function with improved lifecycle handling
packages/bootstrap-vue-next/src/directives/BTooltip/index.ts Refactored to use createFloatingDirective factory, reducing code duplication
packages/bootstrap-vue-next/src/directives/BPopover/index.ts Refactored to use createFloatingDirective factory, reducing code duplication
packages/bootstrap-vue-next/src/directives/BScrollspy/index.ts Implemented UID namespacing for multi-instance safety with SSR guards
packages/bootstrap-vue-next/src/directives/BToggle/index.ts Fixed async registration race conditions with additional mount state checks throughout the loop
Comments suppressed due to low confidence (2)

packages/bootstrap-vue-next/src/directives/BToggle/index.ts:101

  • The async registration loop creates a potential race condition. If handleUpdate is called multiple times rapidly (e.g., during quick re-renders), multiple async loops can run concurrently for the same target, each attempting to register/unregister triggers. This could lead to:
  1. Multiple registrations of the same trigger
  2. Unregistration calls being interleaved unpredictably
  3. The element state (dataset.bvtoggle) being modified by concurrent iterations

Consider tracking pending registrations per element/target pair and canceling previous registration attempts when a new update occurs, or using a debounce/throttle mechanism for rapid updates.

  targets.forEach(async (targetId) => {
    let count = 0
    const maxAttempts = 5
    const delayMs = 100

    // Keep looking until showHide is found, giving up after 500ms or directive is unmounted
    while (count < maxAttempts) {
      // Check if element is still mounted before each iteration
      if (!(el as HTMLElement).dataset.bvtoggle) {
        // Element was unmounted, stop trying
        return
      }

      const showHide = showHideMap?.value.get(targetId)
      if (!showHide) {
        count++
        if (count < maxAttempts) {
          await new Promise((resolve) => setTimeout(resolve, delayMs))
          continue
        }
        // Only warn if element is still mounted after all attempts
        if ((el as HTMLElement).dataset.bvtoggle) {
          // eslint-disable-next-line no-console
          console.warn(
            `[v-b-toggle] Target with ID ${targetId} not found after ${maxAttempts * delayMs}ms`
          )
        }
        break
      }

      // Final check before registration
      if (!(el as HTMLElement).dataset.bvtoggle) return

      // Register the trigger element
      toValue(showHide).unregisterTrigger('click', el, false)
      toValue(showHide).registerTrigger('click', el)
      break
    }
  })

packages/bootstrap-vue-next/src/utils/floatingUi.ts:142

  • The bind function doesn't support multiple component instances using directives on the same element. The el.$__element property (line 142) is shared across all instances, so when multiple component instances try to create tooltips/popovers on the same element:
  1. The first instance creates el.$__element
  2. The second instance overwrites el.$__element, orphaning the first instance's DOM element
  3. When instances unbind, they may remove each other's elements

This breaks the UID namespacing pattern used elsewhere. The $__element should be namespaced per UID like $__tooltip and $__popover, or the architecture should ensure only one visual element exists per HTML element regardless of how many component instances reference it.

Consider either:

  • Making $__element UID-namespaced: el.$__elements[uid] = div
  • Or enforcing a single tooltip/popover per element with proper reference counting
  const div = document.createElement('span')
  if (binding.modifiers.body) document.body.appendChild(div)
  else if (binding.modifiers.child) el.appendChild(div)
  else el.parentNode?.insertBefore(div, el.nextSibling)
  render(h(BPopover, props), div)
  el.$__element = div

coderabbitai[bot]
coderabbitai bot reviewed Nov 10, 2025
View reviewed changes
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (3)
packages/bootstrap-vue-next/src/directives/BToggle/index.ts (2)

63-101: Consider Promise.all for better error handling and testability.

The forEach with an async callback means handleUpdate returns immediately without waiting for any registration attempts to complete. This is a common async antipattern that prevents error aggregation and makes it difficult to test when registrations are complete.

If non-blocking registration is intentional, consider adding a comment to document this behavior. Otherwise, consider refactoring to:

- targets.forEach(async (targetId) => {
+ await Promise.all(targets.map(async (targetId) => {
    // ... retry logic ...
- })
+ }))

This would allow the directive lifecycle to complete registrations before the next update or unmount, reducing race condition windows.

65-66: Consider extracting retry configuration to module level.

The retry constants are defined inside the forEach callback, creating new constants for each target on every update. While the performance impact is negligible, extracting them to the module level would improve clarity and make them easier to adjust:

+const MAX_REGISTRATION_ATTEMPTS = 5
+const REGISTRATION_DELAY_MS = 100
+
 const handleUpdate = (
   // ...
 ) => {
   targets.forEach(async (targetId) => {
     let count = 0
-    const maxAttempts = 5
-    const delayMs = 100
+    const maxAttempts = MAX_REGISTRATION_ATTEMPTS
+    const delayMs = REGISTRATION_DELAY_MS
packages/bootstrap-vue-next/src/directives/utils.ts (1)

24-37: Consider defensive handling for binding.instance access.

Line 36 uses a non-null assertion (binding.instance!.$.uid) which assumes the directive is always used with a component instance. While this is typically safe in Vue 3 directive context, consider adding a defensive check or documenting the assumption.

Apply this diff if defensive handling is desired:

 export function getDirectiveUid(binding: DirectiveBinding): number {
+  if (!binding.instance) {
+    throw new Error('Directive must be used within a component instance')
+  }
   return binding.instance!.$.uid
 }

Alternatively, document the assumption in the JSDoc comment.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f14f049 and eef43f3.

📒 Files selected for processing (6)
  • packages/bootstrap-vue-next/src/directives/BPopover/index.ts (1 hunks)
  • packages/bootstrap-vue-next/src/directives/BScrollspy/index.ts (2 hunks)
  • packages/bootstrap-vue-next/src/directives/BToggle/index.ts (1 hunks)
  • packages/bootstrap-vue-next/src/directives/BTooltip/index.ts (1 hunks)
  • packages/bootstrap-vue-next/src/directives/utils.ts (3 hunks)
  • packages/bootstrap-vue-next/src/utils/floatingUi.ts (3 hunks)
🧰 Additional context used
🧠 Learnings (15)
📓 Common learnings 
Learnt from: dwgray
Repo: bootstrap-vue-next/bootstrap-vue-next PR: 2777
File: packages/bootstrap-vue-next/CHANGELOG.md:11-11
Timestamp: 2025-08-18T18:20:08.240Z
Learning: For PR #2777 (BSort updates), keep changes scoped to BTable sorting work; unrelated edits like CHANGELOG typos should be deferred to a separate PR/issue.

Learnt from: dwgray
Repo: bootstrap-vue-next/bootstrap-vue-next PR: 2762
File: apps/docs/src/docs/components/tooltip.md:90-94
Timestamp: 2025-08-19T23:30:07.062Z
Learning: In bootstrap-vue-next docs, tooltip.data.ts uses popoverSharedProps('popover') to define props, which includes the 'manual' prop through the shared props structure from popover-shared.ts. The manual prop is available in BTooltip documentation through this prop spreading/inheritance mechanism.

Learnt from: xvaara
Repo: bootstrap-vue-next/bootstrap-vue-next PR: 2716
File: packages/bootstrap-vue-next/src/components/BTabs/BTabs.vue:240-261
Timestamp: 2025-05-28T07:01:55.095Z
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: dwgray
Repo: bootstrap-vue-next/bootstrap-vue-next PR: 2762
File: apps/docs/src/docs/components/tooltip.md:90-94
Timestamp: 2025-08-19T23:30:07.062Z
Learning: In bootstrap-vue-next docs, tooltip.data.ts uses popoverSharedProps('tooltip') to define props, which includes the 'manual' prop through the shared props structure from popover-shared.ts. The manual prop is available in BTooltip documentation through this inheritance mechanism.

Learnt from: xvaara
Repo: bootstrap-vue-next/bootstrap-vue-next PR: 2425
File: packages/bootstrap-vue-next/src/plugins/popoverController/index.ts:138-140
Timestamp: 2025-04-28T22:48:46.738Z
Learning: In bootstrap-vue-next, `PopoverOrchestratorParam` includes an `id` field of type `ControllerKey` through inheritance from base types. This field is declared in the `BvControllerOptions` interface and propagated through the type hierarchy.

Learnt from: dwgray
Repo: bootstrap-vue-next/bootstrap-vue-next PR: 2762
File: apps/docs/src/docs/components/tooltip.md:0-0
Timestamp: 2025-06-26T19:46:19.333Z
Learning: BTooltip is a very thin wrapper around BPopover in bootstrap-vue-next. There is no separate `useTooltipController` composable - the `usePopoverController` composable can be used to programmatically control both popovers and tooltips.
📚 Learning: 2025-08-19T23:30:07.062Z 
Learnt from: dwgray
Repo: bootstrap-vue-next/bootstrap-vue-next PR: 2762
File: apps/docs/src/docs/components/tooltip.md:90-94
Timestamp: 2025-08-19T23:30:07.062Z
Learning: In bootstrap-vue-next docs, tooltip.data.ts uses popoverSharedProps('popover') to define props, which includes the 'manual' prop through the shared props structure from popover-shared.ts. The manual prop is available in BTooltip documentation through this prop spreading/inheritance mechanism.

Applied to files:

  • packages/bootstrap-vue-next/src/directives/BTooltip/index.ts
  • packages/bootstrap-vue-next/src/directives/BPopover/index.ts
  • packages/bootstrap-vue-next/src/directives/utils.ts
  • packages/bootstrap-vue-next/src/directives/BToggle/index.ts
  • packages/bootstrap-vue-next/src/utils/floatingUi.ts
📚 Learning: 2025-08-19T23:30:07.062Z 
Learnt from: dwgray
Repo: bootstrap-vue-next/bootstrap-vue-next PR: 2762
File: apps/docs/src/docs/components/tooltip.md:90-94
Timestamp: 2025-08-19T23:30:07.062Z
Learning: In bootstrap-vue-next docs, tooltip.data.ts uses popoverSharedProps('tooltip') to define props, which includes the 'manual' prop through the shared props structure from popover-shared.ts. The manual prop is available in BTooltip documentation through this inheritance mechanism.

Applied to files:

  • packages/bootstrap-vue-next/src/directives/BTooltip/index.ts
  • packages/bootstrap-vue-next/src/directives/BPopover/index.ts
  • packages/bootstrap-vue-next/src/directives/utils.ts
  • packages/bootstrap-vue-next/src/directives/BToggle/index.ts
  • packages/bootstrap-vue-next/src/utils/floatingUi.ts
📚 Learning: 2025-06-26T19:46:19.333Z 
Learnt from: dwgray
Repo: bootstrap-vue-next/bootstrap-vue-next PR: 2762
File: apps/docs/src/docs/components/tooltip.md:0-0
Timestamp: 2025-06-26T19:46:19.333Z
Learning: BTooltip is a very thin wrapper around BPopover in bootstrap-vue-next. There is no separate `useTooltipController` composable - the `usePopoverController` composable can be used to programmatically control both popovers and tooltips.

Applied to files:

  • packages/bootstrap-vue-next/src/directives/BTooltip/index.ts
  • packages/bootstrap-vue-next/src/directives/BPopover/index.ts
  • packages/bootstrap-vue-next/src/directives/BToggle/index.ts
📚 Learning: 2025-09-12T14:46:49.416Z 
Learnt from: CR
Repo: bootstrap-vue-next/bootstrap-vue-next PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-12T14:46:49.416Z
Learning: Applies to packages/bootstrap-vue-next/src/components/index.ts : When adding a new component, update packages/bootstrap-vue-next/src/components/index.ts to export it

Applied to files:

  • packages/bootstrap-vue-next/src/directives/BTooltip/index.ts
  • packages/bootstrap-vue-next/src/directives/BPopover/index.ts
  • packages/bootstrap-vue-next/src/directives/utils.ts
  • packages/bootstrap-vue-next/src/directives/BScrollspy/index.ts
  • packages/bootstrap-vue-next/src/directives/BToggle/index.ts
  • packages/bootstrap-vue-next/src/utils/floatingUi.ts
📚 Learning: 2025-09-12T14:46:49.416Z 
Learnt from: CR
Repo: bootstrap-vue-next/bootstrap-vue-next PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-12T14:46:49.416Z
Learning: Applies to packages/bootstrap-vue-next/src/App.vue : Use packages/bootstrap-vue-next/src/App.vue as a local test area for component changes

Applied to files:

  • packages/bootstrap-vue-next/src/directives/BTooltip/index.ts
  • packages/bootstrap-vue-next/src/directives/BPopover/index.ts
  • packages/bootstrap-vue-next/src/directives/utils.ts
  • packages/bootstrap-vue-next/src/directives/BScrollspy/index.ts
  • packages/bootstrap-vue-next/src/directives/BToggle/index.ts
  • packages/bootstrap-vue-next/src/utils/floatingUi.ts
📚 Learning: 2025-09-30T23:57:21.526Z 
Learnt from: dwgray
Repo: bootstrap-vue-next/bootstrap-vue-next PR: 2866
File: apps/docs/src/docs/components/demo/ModalModel.vue:11-15
Timestamp: 2025-09-30T23:57:21.526Z
Learning: The bootstrap-vue-next documentation project uses unplugin-vue-components to automatically resolve and import Vue components like BModal, BButton, etc. Explicit imports for these components in script sections are not required.

Applied to files:

  • packages/bootstrap-vue-next/src/directives/BTooltip/index.ts
  • packages/bootstrap-vue-next/src/directives/BPopover/index.ts
  • packages/bootstrap-vue-next/src/directives/utils.ts
📚 Learning: 2025-04-28T22:48:46.738Z 
Learnt from: xvaara
Repo: bootstrap-vue-next/bootstrap-vue-next PR: 2425
File: packages/bootstrap-vue-next/src/plugins/popoverController/index.ts:138-140
Timestamp: 2025-04-28T22:48:46.738Z
Learning: In bootstrap-vue-next, `PopoverOrchestratorParam` includes an `id` field of type `ControllerKey` through inheritance from base types. This field is declared in the `BvControllerOptions` interface and propagated through the type hierarchy.

Applied to files:

  • packages/bootstrap-vue-next/src/directives/BTooltip/index.ts
  • packages/bootstrap-vue-next/src/directives/BPopover/index.ts
  • packages/bootstrap-vue-next/src/directives/utils.ts
  • packages/bootstrap-vue-next/src/utils/floatingUi.ts
📚 Learning: 2025-09-12T14:46:49.416Z 
Learnt from: CR
Repo: bootstrap-vue-next/bootstrap-vue-next PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-12T14:46:49.416Z
Learning: Applies to packages/bootstrap-vue-next/src/components/** : Create and modify Vue components only under packages/bootstrap-vue-next/src/components/

Applied to files:

  • packages/bootstrap-vue-next/src/directives/BTooltip/index.ts
  • packages/bootstrap-vue-next/src/directives/BPopover/index.ts
  • packages/bootstrap-vue-next/src/directives/BToggle/index.ts
📚 Learning: 2025-08-19T14:23:46.775Z 
Learnt from: xvaara
Repo: bootstrap-vue-next/bootstrap-vue-next PR: 2732
File: packages/bootstrap-vue-next/CHANGELOG.md:35-41
Timestamp: 2025-08-19T14:23:46.775Z
Learning: In the bootstrap-vue-next repository, CHANGELOG.md files (e.g., packages/bootstrap-vue-next/CHANGELOG.md) are autogenerated and should be ignored in reviews; do not propose edits for them.

Applied to files:

  • packages/bootstrap-vue-next/src/directives/BTooltip/index.ts
  • packages/bootstrap-vue-next/src/directives/BToggle/index.ts
📚 Learning: 2025-05-23T23:58:07.165Z 
Learnt from: dwgray
Repo: bootstrap-vue-next/bootstrap-vue-next PR: 2701
File: apps/docs/src/docs/migration-guide.md:630-632
Timestamp: 2025-05-23T23:58:07.165Z
Learning: The `<NotYetImplemented/>` component in the bootstrap-vue-next documentation automatically renders text indicating "Not Yet Implemented", so additional explanatory text about features not being implemented is redundant when this component is used.

Applied to files:

  • packages/bootstrap-vue-next/src/directives/BTooltip/index.ts
  • packages/bootstrap-vue-next/src/directives/BPopover/index.ts
  • packages/bootstrap-vue-next/src/directives/BToggle/index.ts
📚 Learning: 2025-10-21T19:31:54.113Z 
Learnt from: dwgray
Repo: bootstrap-vue-next/bootstrap-vue-next PR: 2882
File: apps/docs/src/docs/components/demo/PlaceholderWidth.vue:1-22
Timestamp: 2025-10-21T19:31:54.113Z
Learning: The bootstrap-vue-next project uses unplugin to automatically import components, so explicit imports in demo/documentation components are not needed.

Applied to files:

  • packages/bootstrap-vue-next/src/directives/BPopover/index.ts
📚 Learning: 2025-05-01T23:40:56.146Z 
Learnt from: dwgray
Repo: bootstrap-vue-next/bootstrap-vue-next PR: 2679
File: apps/docs/.vitepress/theme/Layout.vue:0-0
Timestamp: 2025-05-01T23:40:56.146Z
Learning: In bootstrap-vue-next, the `useScrollspy` hook returns an object with a `current` property which is the ID string (or null) of the currently active item, not an object containing an ID.

Applied to files:

  • packages/bootstrap-vue-next/src/directives/BScrollspy/index.ts
📚 Learning: 2025-05-28T07:01:55.095Z 
Learnt from: xvaara
Repo: bootstrap-vue-next/bootstrap-vue-next PR: 2716
File: packages/bootstrap-vue-next/src/components/BTabs/BTabs.vue:240-261
Timestamp: 2025-05-28T07:01:55.095Z
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.

Applied to files:

  • packages/bootstrap-vue-next/src/directives/BScrollspy/index.ts
  • packages/bootstrap-vue-next/src/directives/BToggle/index.ts
  • packages/bootstrap-vue-next/src/utils/floatingUi.ts
📚 Learning: 2025-05-28T07:57:19.915Z 
Learnt from: xvaara
Repo: bootstrap-vue-next/bootstrap-vue-next PR: 2716
File: packages/bootstrap-vue-next/src/components/BTabs/BTabs.vue:384-404
Timestamp: 2025-05-28T07:57:19.915Z
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.

Applied to files:

  • packages/bootstrap-vue-next/src/directives/BToggle/index.ts
🧬 Code graph analysis (5)
packages/bootstrap-vue-next/src/directives/BTooltip/index.ts (2)
packages/bootstrap-vue-next/src/utils/floatingUi.ts (2)
  • ElementWithPopper (110-127)
  • resolveDirectiveProps (85-108)
packages/bootstrap-vue-next/src/directives/utils.ts (1)
  • createFloatingDirective (197-296)
packages/bootstrap-vue-next/src/directives/BPopover/index.ts (2)
packages/bootstrap-vue-next/src/utils/floatingUi.ts (2)
  • ElementWithPopper (110-127)
  • resolveDirectiveProps (85-108)
packages/bootstrap-vue-next/src/directives/utils.ts (1)
  • createFloatingDirective (197-296)
packages/bootstrap-vue-next/src/directives/utils.ts (3)
packages/bootstrap-vue-next/src/types/ComponentProps.ts (1)
  • BPopoverProps (1334-1370)
packages/bootstrap-vue-next/src/utils/floatingUi.ts (6)
  • ElementWithPopper (110-127)
  • resolveActiveStatus (35-36)
  • resolveContent (38-83)
  • resolveDirectiveProps (85-108)
  • bind (129-143)
  • unbind (145-166)
packages/bootstrap-vue-next/src/utils/keys.ts (1)
  • defaultsKey (204-205)
packages/bootstrap-vue-next/src/directives/BScrollspy/index.ts (3)
packages/bootstrap-vue-next/src/composables/useScrollspy/index.ts (1)
  • useScrollspy (44-236)
packages/bootstrap-vue-next/src/directives/utils.ts (1)
  • getDirectiveUid (35-37)
packages/bootstrap-vue-next/src/utils/object.ts (1)
  • omit (5-14)
packages/bootstrap-vue-next/src/utils/floatingUi.ts (1)
packages/bootstrap-vue-next/src/types/ComponentProps.ts (1)
  • BPopoverProps (1334-1370)
⏰ 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). (2)
  • GitHub Check: Agent
  • GitHub Check: build
🔇 Additional comments (18)
packages/bootstrap-vue-next/src/directives/BToggle/index.ts (3)

69-74: Good defensive check against registration on unmounted elements.

The mounted state check using dataset.bvtoggle effectively prevents race conditions when elements unmount during retry loops. This is a solid robustness improvement.

79-89: Excellent warning logic that reduces false positives.

The conditional warning (lines 84-89) only fires when the element is still mounted after all retry attempts, preventing noisy warnings when elements are unmounted during the retry window. The warning message clearly communicates the timeout duration.

93-98: Good final safeguard before registration.

The mounted check immediately before registration (line 94) minimizes the race condition window. Combined with the unregister-before-register pattern (line 97), this provides robust handling of edge cases.

packages/bootstrap-vue-next/src/directives/BTooltip/index.ts (1)

1-15: LGTM! Clean refactoring using the centralized factory pattern.

The migration to createFloatingDirective successfully consolidates the tooltip directive logic while maintaining correct behavior. The noninteractive: true default and tooltip: true flag properly distinguish tooltips from popovers, and the title fallback chain (text.title ?? text.body ?? '') ensures content is always available.

packages/bootstrap-vue-next/src/directives/BScrollspy/index.ts (3)

11-12: SSR guard properly placed.

The early return prevents DOM access on the server, which is correct for scrollspy functionality that requires browser APIs.

14-24: Per-instance UID management correctly implemented.

The UID namespacing enables multiple scrollspy instances per element (e.g., when the same element is used in different component instances). The cleanup of existing instances before creating new ones prevents memory leaks during updates.

47-56: Cleanup logic is correct and properly scoped.

The beforeUnmount hook correctly targets only the instance associated with the current UID, preventing interference with other component instances that may be using the same element.

packages/bootstrap-vue-next/src/directives/BPopover/index.ts (1)

1-13: LGTM! Popover directive successfully refactored.

The migration to createFloatingDirective is clean and correct. The prop merging order (defaults → directive props → text) ensures proper precedence, and popovers correctly maintain their interactive behavior by default (unlike tooltips which set noninteractive: true).

packages/bootstrap-vue-next/src/utils/floatingUi.ts (4)

49-66: SSR guards correctly implemented for DOM attribute access.

The guards properly prevent getAttribute/setAttribute/removeAttribute calls on the server while maintaining equivalent behavior. The fallback path returns empty content when no binding value is provided in SSR context, which is appropriate.

134-135: SSR guard prevents DOM manipulation on server.

Correctly skips document.createElement and related DOM operations when running on the server.

145-165: Improved cleanup sequence with better performance characteristics.

The refactored unbind sequence is solid:

  1. Immediate component unmount via render(null, div) prevents lingering reactivity
  2. queueMicrotask provides more predictable timing than setTimeout(0) and better performance
  3. SSR guard prevents DOM operations on server
  4. DOM removal and state cleanup in microtask are appropriately deferred

The comment on Line 162 correctly notes that UID-specific state cleanup happens in the directive's beforeUnmount.

110-126: ElementWithPopper interface properly extended for UID-namespaced state.

The interface changes correctly support the new per-instance architecture:

  • Index signature enables dynamic property assignment for directive state
  • Removed $__binding in favor of per-UID state within $__tooltip/$__popover maps
  • The destroying flag in each instance prevents race conditions during concurrent updates and cleanup
packages/bootstrap-vue-next/src/directives/utils.ts (6)

47-65: State initialization correctly prevents prototype pollution.

Using Object.create(null) for the UID namespace map is a good security practice that prevents prototype pollution attacks and avoids unexpected property inheritance.

85-109: Binding change detection via JSON serialization is pragmatic.

The JSON.stringify approach for deep comparison is straightforward and handles nested objects/arrays correctly. While not the most performant for large binding values, it's acceptable here since:

  1. Directive bindings are typically small objects
  2. The comparison only runs on updates, not renders
  3. The cost is outweighed by preventing unnecessary rebinds

111-130: Cleanup properly coordinates with the destroying flag.

Setting destroying: true before deleting the instance entry ensures any in-flight updates will see the flag and abort, preventing race conditions during concurrent update/unmount scenarios.

208-231: Mounted lifecycle correctly guards against inactive or empty content.

The early returns for inactive bindings (Line 213) and missing content (Line 217) prevent unnecessary DOM operations and state initialization. The order is optimal: check activation → resolve content → check content existence → initialize state → bind.

233-284: Updated lifecycle robustly handles state transitions and race conditions.

The update path correctly handles multiple scenarios:

  1. Instance never mounted or already cleaned up (Line 238)
  2. Inactive binding → cleanup (Lines 246-251)
  3. Empty content → cleanup (Lines 255-261)
  4. No binding change → no-op (Line 265)
  5. Destroying flag → prevent race (Line 268)

The delete binding.oldValue on Line 263 is intentional to prevent Vue's internal change detection from interfering with the custom binding comparison logic.

286-296: BeforeUnmount correctly cleans up per-instance state.

The hook properly unbinds the UI element and removes the UID-specific state entry, ensuring no memory leaks when components are destroyed.

@dwgray dwgray mentioned this pull request Nov 10, 2025
Closed
xvaara
xvaara previously approved these changes Nov 11, 2025
View reviewed changes
Copy link
Contributor

@xvaara xvaara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems a nice update, didn't test it but I presume it works :)

@dwgray
fix: handle directive activation state transitions in updated hook
5f07a4f 
Addresses Copilot feedback on PR bootstrap-vue-next#2906 - when a directive is mounted inactive
(active: false or no content) then later updated to become active, the updated
hook now properly initializes the instance and renders the tooltip/popover.

Also adds proper cleanup calls to cleanupDirectiveInstance when removing due
to inactive state or missing content, preventing potential memory leaks.
coderabbitai[bot]
coderabbitai bot reviewed Nov 12, 2025
View reviewed changes
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
packages/bootstrap-vue-next/src/directives/utils.ts (1)

91-97: Consider edge cases in JSON.stringify-based change detection.

Using JSON.stringify for change detection is simple and reliable for most cases. However, it could fail or throw errors if the binding value contains circular references, functions, or non-serializable objects (e.g., Symbol, BigInt).

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between eef43f3 and 5f07a4f.

📒 Files selected for processing (1)
  • packages/bootstrap-vue-next/src/directives/utils.ts (3 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings 
Learnt from: dwgray
Repo: bootstrap-vue-next/bootstrap-vue-next PR: 2777
File: packages/bootstrap-vue-next/CHANGELOG.md:11-11
Timestamp: 2025-08-18T18:20:08.240Z
Learning: For PR #2777 (BSort updates), keep changes scoped to BTable sorting work; unrelated edits like CHANGELOG typos should be deferred to a separate PR/issue.
📚 Learning: 2025-09-12T14:46:49.416Z 
Learnt from: CR
Repo: bootstrap-vue-next/bootstrap-vue-next PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-12T14:46:49.416Z
Learning: Applies to packages/bootstrap-vue-next/src/components/index.ts : When adding a new component, update packages/bootstrap-vue-next/src/components/index.ts to export it

Applied to files:

  • packages/bootstrap-vue-next/src/directives/utils.ts
📚 Learning: 2025-09-12T14:46:49.416Z 
Learnt from: CR
Repo: bootstrap-vue-next/bootstrap-vue-next PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-12T14:46:49.416Z
Learning: Applies to packages/bootstrap-vue-next/src/App.vue : Use packages/bootstrap-vue-next/src/App.vue as a local test area for component changes

Applied to files:

  • packages/bootstrap-vue-next/src/directives/utils.ts
📚 Learning: 2025-05-28T07:01:55.095Z 
Learnt from: xvaara
Repo: bootstrap-vue-next/bootstrap-vue-next PR: 2716
File: packages/bootstrap-vue-next/src/components/BTabs/BTabs.vue:240-261
Timestamp: 2025-05-28T07:01:55.095Z
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.

Applied to files:

  • packages/bootstrap-vue-next/src/directives/utils.ts
📚 Learning: 2025-04-28T22:48:46.738Z 
Learnt from: xvaara
Repo: bootstrap-vue-next/bootstrap-vue-next PR: 2425
File: packages/bootstrap-vue-next/src/plugins/popoverController/index.ts:138-140
Timestamp: 2025-04-28T22:48:46.738Z
Learning: In bootstrap-vue-next, `PopoverOrchestratorParam` includes an `id` field of type `ControllerKey` through inheritance from base types. This field is declared in the `BvControllerOptions` interface and propagated through the type hierarchy.

Applied to files:

  • packages/bootstrap-vue-next/src/directives/utils.ts
📚 Learning: 2025-08-19T23:30:07.062Z 
Learnt from: dwgray
Repo: bootstrap-vue-next/bootstrap-vue-next PR: 2762
File: apps/docs/src/docs/components/tooltip.md:90-94
Timestamp: 2025-08-19T23:30:07.062Z
Learning: In bootstrap-vue-next docs, tooltip.data.ts uses popoverSharedProps('popover') to define props, which includes the 'manual' prop through the shared props structure from popover-shared.ts. The manual prop is available in BTooltip documentation through this prop spreading/inheritance mechanism.

Applied to files:

  • packages/bootstrap-vue-next/src/directives/utils.ts
📚 Learning: 2025-08-19T23:30:07.062Z 
Learnt from: dwgray
Repo: bootstrap-vue-next/bootstrap-vue-next PR: 2762
File: apps/docs/src/docs/components/tooltip.md:90-94
Timestamp: 2025-08-19T23:30:07.062Z
Learning: In bootstrap-vue-next docs, tooltip.data.ts uses popoverSharedProps('tooltip') to define props, which includes the 'manual' prop through the shared props structure from popover-shared.ts. The manual prop is available in BTooltip documentation through this inheritance mechanism.

Applied to files:

  • packages/bootstrap-vue-next/src/directives/utils.ts
📚 Learning: 2025-09-30T23:57:21.526Z 
Learnt from: dwgray
Repo: bootstrap-vue-next/bootstrap-vue-next PR: 2866
File: apps/docs/src/docs/components/demo/ModalModel.vue:11-15
Timestamp: 2025-09-30T23:57:21.526Z
Learning: The bootstrap-vue-next documentation project uses unplugin-vue-components to automatically resolve and import Vue components like BModal, BButton, etc. Explicit imports for these components in script sections are not required.

Applied to files:

  • packages/bootstrap-vue-next/src/directives/utils.ts
🧬 Code graph analysis (1)
packages/bootstrap-vue-next/src/directives/utils.ts (3)
packages/bootstrap-vue-next/src/types/ComponentProps.ts (1)
  • BPopoverProps (1334-1370)
packages/bootstrap-vue-next/src/utils/floatingUi.ts (6)
  • ElementWithPopper (110-127)
  • resolveActiveStatus (35-36)
  • resolveContent (38-83)
  • resolveDirectiveProps (85-108)
  • bind (129-143)
  • unbind (145-166)
packages/bootstrap-vue-next/src/utils/keys.ts (1)
  • defaultsKey (204-205)
⏰ 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: build
🔇 Additional comments (4)
packages/bootstrap-vue-next/src/directives/utils.ts (4)

24-30: Well-designed state structure for UID-namespaced directives.

The DirectiveInstanceState interface provides a clean solution for managing per-instance directive state. The binding cache enables efficient change detection, and the destroying flag prevents race conditions during cleanup.

117-130: Proper use of destroying flag prevents race conditions.

Setting destroying: true before deleting the instance state effectively guards against race conditions during concurrent cleanup operations.

233-300: The updated hook logic correctly addresses past review concerns.

The implementation now properly handles:

  1. Cleanup with cleanupDirectiveInstance when content becomes empty or inactive (lines 246, 257)
  2. Instance initialization when transitioning from inactive/no-content to active state (lines 262-277)
  3. Race condition prevention via the destroying flag check (line 284)

However, see separate comment about line 279.

302-310: Clean and correct beforeUnmount implementation.

The hook follows the proper cleanup sequence: verify instance existence, unbind the directive, and clean up the UID-namespaced state.

@dwgray
fix: add null safety and remove readonly mutation in directives
a09f28a 
Addresses CodeRabbitAI feedback on PR bootstrap-vue-next#2906:
- Add null check to getDirectiveUid to prevent runtime crashes
- Remove delete binding.oldValue to respect readonly contract
- Both changes improve code safety without changing behavior
coderabbitai[bot]
coderabbitai bot reviewed Nov 12, 2025
View reviewed changes
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5f07a4f and a09f28a.

📒 Files selected for processing (1)
  • packages/bootstrap-vue-next/src/directives/utils.ts (3 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings 
Learnt from: dwgray
Repo: bootstrap-vue-next/bootstrap-vue-next PR: 2777
File: packages/bootstrap-vue-next/CHANGELOG.md:11-11
Timestamp: 2025-08-18T18:20:08.240Z
Learning: For PR #2777 (BSort updates), keep changes scoped to BTable sorting work; unrelated edits like CHANGELOG typos should be deferred to a separate PR/issue.
📚 Learning: 2025-05-28T07:01:55.095Z 
Learnt from: xvaara
Repo: bootstrap-vue-next/bootstrap-vue-next PR: 2716
File: packages/bootstrap-vue-next/src/components/BTabs/BTabs.vue:240-261
Timestamp: 2025-05-28T07:01:55.095Z
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.

Applied to files:

  • packages/bootstrap-vue-next/src/directives/utils.ts
📚 Learning: 2025-04-28T22:48:46.738Z 
Learnt from: xvaara
Repo: bootstrap-vue-next/bootstrap-vue-next PR: 2425
File: packages/bootstrap-vue-next/src/plugins/popoverController/index.ts:138-140
Timestamp: 2025-04-28T22:48:46.738Z
Learning: In bootstrap-vue-next, `PopoverOrchestratorParam` includes an `id` field of type `ControllerKey` through inheritance from base types. This field is declared in the `BvControllerOptions` interface and propagated through the type hierarchy.

Applied to files:

  • packages/bootstrap-vue-next/src/directives/utils.ts
📚 Learning: 2025-08-19T23:30:07.062Z 
Learnt from: dwgray
Repo: bootstrap-vue-next/bootstrap-vue-next PR: 2762
File: apps/docs/src/docs/components/tooltip.md:90-94
Timestamp: 2025-08-19T23:30:07.062Z
Learning: In bootstrap-vue-next docs, tooltip.data.ts uses popoverSharedProps('popover') to define props, which includes the 'manual' prop through the shared props structure from popover-shared.ts. The manual prop is available in BTooltip documentation through this prop spreading/inheritance mechanism.

Applied to files:

  • packages/bootstrap-vue-next/src/directives/utils.ts
📚 Learning: 2025-08-19T23:30:07.062Z 
Learnt from: dwgray
Repo: bootstrap-vue-next/bootstrap-vue-next PR: 2762
File: apps/docs/src/docs/components/tooltip.md:90-94
Timestamp: 2025-08-19T23:30:07.062Z
Learning: In bootstrap-vue-next docs, tooltip.data.ts uses popoverSharedProps('tooltip') to define props, which includes the 'manual' prop through the shared props structure from popover-shared.ts. The manual prop is available in BTooltip documentation through this inheritance mechanism.

Applied to files:

  • packages/bootstrap-vue-next/src/directives/utils.ts
📚 Learning: 2025-09-30T23:57:21.526Z 
Learnt from: dwgray
Repo: bootstrap-vue-next/bootstrap-vue-next PR: 2866
File: apps/docs/src/docs/components/demo/ModalModel.vue:11-15
Timestamp: 2025-09-30T23:57:21.526Z
Learning: The bootstrap-vue-next documentation project uses unplugin-vue-components to automatically resolve and import Vue components like BModal, BButton, etc. Explicit imports for these components in script sections are not required.

Applied to files:

  • packages/bootstrap-vue-next/src/directives/utils.ts
🧬 Code graph analysis (1)
packages/bootstrap-vue-next/src/directives/utils.ts (3)
packages/bootstrap-vue-next/src/types/ComponentProps.ts (1)
  • BPopoverProps (1334-1370)
packages/bootstrap-vue-next/src/utils/floatingUi.ts (6)
  • ElementWithPopper (110-127)
  • resolveActiveStatus (35-36)
  • resolveContent (38-83)
  • resolveDirectiveProps (85-108)
  • bind (129-143)
  • unbind (145-166)
packages/bootstrap-vue-next/src/utils/keys.ts (1)
  • defaultsKey (204-205)
⏰ 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: build

@dwgray
fix: prevent deletion of fresh element reference in rebinding scenario
0e66c79 
When unbind() followed immediately by bind(), the microtask cleanup
could delete the newly created element reference. Now only delete
the reference if it still points to the element we're unmounting.

Fixes race condition identified in CodeRabbitAI review where rapid
rebinding in the updated() hook would leak DOM nodes and lose the
floating UI reference.
@dwgray
Copy link
Member Author

dwgray commented Nov 12, 2025

@xvaara can you please re-approve this - there were a couple of clean-up things from AI codereview that I thought were worth taking care of before merging this.

@dwgray dwgray requested a review from xvaara November 12, 2025 18:19
xvaara
xvaara approved these changes Nov 14, 2025
View reviewed changes
Copy link
Contributor

@xvaara xvaara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did a quick test and read-through. LGTM

@dwgray dwgray merged commit 7b39759 into bootstrap-vue-next:main Nov 14, 2025
5 checks passed
@github-actions github-actions bot mentioned this pull request Nov 14, 2025
Merged
This was referenced Nov 15, 2025
Merged
Merged
xvaara added a commit to xvaara/bootstrap-vue-next that referenced this pull request Nov 30, 2025
@xvaara
Merge remote-tracking branch 'upstream/main'
c67bed4 
* upstream/main: (28 commits)
  fix(BToggle)! Remove redundant attribute cleanup & update docs for accessibility attributes on show/hide components (bootstrap-vue-next#2918)
  chore: release main (bootstrap-vue-next#2912)
  fix: allow custom component props in orchestrator create methods with type safety (bootstrap-vue-next#2922)
  fix(BModal): prevent focus trap error when no tabbable elements exist (bootstrap-vue-next#2864)
  Update package description for clarity (bootstrap-vue-next#2917)
  Update project description for clarity and detail
  test: add event emission tests for BTable and BTableLite (bootstrap-vue-next#2915)
  fix(BTableLite): Use primary key to persist row-details state when items change (bootstrap-vue-next#2871)
  chore: release main (bootstrap-vue-next#2896)
  feat(BTable): add configurable debouncing
  ci: Add publint to the build process (bootstrap-vue-next#2909)
  docs(Offcanvas): Parity pass (bootstrap-vue-next#2900)
  fix(directives): Robustness fixes for directives (bootstrap-vue-next#2906)
  docs(BFormInput): document @wheel.prevent for disabling mousewheel events (bootstrap-vue-next#2903)
  fix(typings): Fix paths to `*.d.mts` files (bootstrap-vue-next#2907)
  feat: add name and form props to BFormRating for form submission (bootstrap-vue-next#2895)
  docs: refactor docs to avoid duplication and boilerplate code (bootstrap-vue-next#2891)
  docs(BToast): Parity (bootstrap-vue-next#2887)
  docs(BModal): fix attribute to hide footer (bootstrap-vue-next#2888)
  docs(BPlaceholder): Parity pass (bootstrap-vue-next#2886)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@coderabbitai coderabbitai[bot] coderabbitai[bot] left review comments

Copilot code review Copilot Copilot left review comments

@xvaara xvaara xvaara approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@dwgray @xvaara