🌐 AI搜索 & 代理 主页
Skip to content

Conversation

@dwgray
Copy link
Member

@dwgray dwgray commented Nov 15, 2025

Describe the PR

This PR handles race conditions when show/hide registry components with the same id are mounted/unmounted in unexpected orders by using vue's uid to disambiguate.

Fixes #2897

  • Replace the simple registry of ids with a registry of arrays of id/uids
  • Add tests for the new logic
  • Add a couple of helper functions to make the code more readable/reduce duplicate code

Small replication

See #2897

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

  • Refactor

    • Registry redesigned to support multiple component instances per ID and expose helpers for resolving the active instance; toggle/directive code now uses these helpers for consistent show/hide behavior.
  • Tests

    • Added extensive unit tests for registry operations, active-instance selection, race conditions, ID updates, and the new registry helpers.
  • Documentation

    • Minor formatting and example tweaks in component docs.
  • Chores

    • Changelog formatting normalized and duplicate entries adjusted.

✏️ Tip: You can customize this high-level summary in your review settings.

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

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

@coderabbitai
Copy link

coderabbitai bot commented Nov 15, 2025

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

Replaces single-instance-per-ID registry entries with per-ID holders containing multiple instances and an active getter; adds registryAccess helpers (getActiveShowHide, getShowHideValue); updates consumers (BToggle directive, useToggle, BNavbarToggle) to use those helpers; adjusts register/unregister/updateId logic and adds tests for instance tracking and races.

Changes

Cohort / File(s) Summary
Registry core
packages/bootstrap-vue-next/src/composables/useRegistry/index.ts, packages/bootstrap-vue-next/src/utils/keys.ts
Switch storage to per-ID RegisterShowHideInstances holders (instances array + getActive()), update register, unregister, and updateId to track instances by UID, migrate instances between holders, and remove empty holders. Update public storage type to map to RegisterShowHideInstances.
Registry access utilities
packages/bootstrap-vue-next/src/utils/registryAccess.ts
Add getActiveShowHide(registry, id) to return the active instance for an ID (with null/undefined handling) and getShowHideValue(registry, id) to read the active instance's boolean value safely.
Consumers (directive, composables, component)
packages/bootstrap-vue-next/src/directives/BToggle/index.ts, packages/bootstrap-vue-next/src/composables/useToggle/index.ts, packages/bootstrap-vue-next/src/components/BNavbar/BNavbarToggle.vue
Replace direct registry map and toValue usage with getActiveShowHide / getShowHideValue; operate on active instance methods/properties directly; add early-return paths when targets or show/hide data are absent.
Tests
packages/bootstrap-vue-next/tests/composables/useRegistry.spec.ts, packages/bootstrap-vue-next/tests/utils/registryAccess.spec.ts
Add comprehensive tests covering register/getActive/unregister/updateId behaviors, multi-instance holders, race conditions, and registryAccess helper edge cases (null/falsy inputs).
Docs / Changelogs / Minor
apps/docs/src/docs/components/table.md, packages/bootstrap-vue-next/CHANGELOG.md, packages/nuxt/CHANGELOG.md
Formatting and example formatting changes in docs and changelogs; duplicated changelog entries adjusted/presentational edits only.

Sequence Diagram(s)

sequenceDiagram
    participant Consumer as Consumer (BToggle / useToggle / BNavbarToggle)
    participant RegistryRef as RegistryRef (Ref<Map<id, Holder>>)
    participant Holder as RegisterShowHideInstances
    participant Instance as ShowHideInstance

    Note over Consumer,RegistryRef: Lookup now resolves per-ID holder then active instance
    Consumer->>RegistryRef: getActiveShowHide(registry, id)
    RegistryRef->>Holder: holder = registry.get(id)
    alt holder exists
        Holder->>Holder: getActive() (select most recent instance)
        Holder-->>Consumer: active Instance
        Consumer->>Instance: call show/hide/toggle or read value (getShowHideValue)
        Instance-->>Consumer: result / side-effect
    else no holder or falsy id
        RegistryRef-->>Consumer: null / undefined
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

  • Pay special attention to:
    • useRegistry/index.ts — UID ordering, instance removal, updateId migration, and cleanup edge cases.
    • registryAccess.ts — precise return semantics (null vs undefined) and default boolean behavior.
    • Directive and composable changes — ensure no behavioral regressions when multiple instances exist and early-return logic is correct.
    • New tests — verify race-condition expectations align with implementation.

Possibly related PRs

Suggested reviewers

  • xvaara

Poem

🐰 A hop through maps and holders new,
many instances now share one view.
getActive finds the freshest sprite,
races settle, toggles work right.
🎉 — From a rabbit, light and true

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Fix/show hide registry race condition' directly and clearly summarizes the main change of the PR, which is addressing race conditions in the show/hide registry.
Description check ✅ Passed The PR description includes the required template sections: clear description of changes, reference to linked issue #2897, proper PR checklist completion with bugfix selected, and conventional commits confirmation. All essential elements are present.
Linked Issues check ✅ Passed The PR successfully addresses the race condition bug in issue #2897 by implementing a registry of arrays with per-ID instance holders using Vue uids for disambiguation, allowing components to be found reliably after mount/unmount cycles.
Out of Scope Changes check ✅ Passed The PR contains minimal out-of-scope changes. Most modifications directly address the race condition fix. The only tangential changes are minor formatting updates in documentation (table.md) and changelog files, which are acceptable housekeeping.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ 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 1b28dbc and 6904759.

📒 Files selected for processing (4)
  • apps/docs/src/docs/components/table.md (3 hunks)
  • packages/bootstrap-vue-next/CHANGELOG.md (1 hunks)
  • packages/bootstrap-vue-next/src/directives/BToggle/index.ts (5 hunks)
  • packages/nuxt/CHANGELOG.md (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • apps/docs/src/docs/components/table.md
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/bootstrap-vue-next/src/directives/BToggle/index.ts
🧰 Additional context used
🧠 Learnings (8)
📚 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/nuxt/CHANGELOG.md
  • packages/bootstrap-vue-next/CHANGELOG.md
📚 Learning: 2025-11-30T17:09:16.276Z
Learnt from: CR
Repo: bootstrap-vue-next/bootstrap-vue-next PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-30T17:09:16.276Z
Learning: Applies to packages/bootstrap-vue-next/src/components/**/*.vue : Edit component files in `packages/bootstrap-vue-next/src/components/` and test using `packages/bootstrap-vue-next/src/App.vue` with hot-reload via `pnpm --filter bootstrap-vue-next run dev`

Applied to files:

  • packages/nuxt/CHANGELOG.md
📚 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/nuxt/CHANGELOG.md
  • packages/bootstrap-vue-next/CHANGELOG.md
📚 Learning: 2025-11-30T17:09:16.276Z
Learnt from: CR
Repo: bootstrap-vue-next/bootstrap-vue-next PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-30T17:09:16.276Z
Learning: Applies to packages/bootstrap-vue-next/src/styles/styles.scss : Main library styles are defined in `packages/bootstrap-vue-next/src/styles/styles.scss`, with component-specific styles typically in component `.vue` files, using Bootstrap 5.3.x as the base CSS framework

Applied to files:

  • packages/nuxt/CHANGELOG.md
📚 Learning: 2025-11-30T17:09:16.276Z
Learnt from: CR
Repo: bootstrap-vue-next/bootstrap-vue-next PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-30T17:09:16.276Z
Learning: Test component changes using the core package dev server at `http://localhost:5174` with `packages/bootstrap-vue-next/src/App.vue` as a test area, and test real-world usage with the playground app at `http://localhost:5173`

Applied to files:

  • packages/nuxt/CHANGELOG.md
📚 Learning: 2025-11-30T17:09:16.276Z
Learnt from: CR
Repo: bootstrap-vue-next/bootstrap-vue-next PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-30T17:09:16.276Z
Learning: Applies to packages/bootstrap-vue-next/src/components/index.ts : When adding new components, add them to the components/index.ts export file

Applied to files:

  • packages/bootstrap-vue-next/CHANGELOG.md
📚 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/CHANGELOG.md
📚 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/CHANGELOG.md
🪛 markdownlint-cli2 (0.18.1)
packages/nuxt/CHANGELOG.md

19-19: Multiple headings with the same content

(MD024, no-duplicate-heading)


23-23: Multiple headings with the same content

(MD024, no-duplicate-heading)


33-33: Multiple headings with the same content

(MD024, no-duplicate-heading)


37-37: Multiple headings with the same content

(MD024, no-duplicate-heading)

packages/bootstrap-vue-next/CHANGELOG.md

19-19: Multiple headings with the same content

(MD024, no-duplicate-heading)


26-26: Multiple headings with the same content

(MD024, no-duplicate-heading)

⏰ 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

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.

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 fixes race conditions in the show/hide registry by using Vue's component UID to disambiguate between multiple instances with the same ID. The solution converts the simple ID-to-instance map into an ID-to-array-of-instances map, where the most recently mounted instance is considered "active."

Key changes:

  • Introduced RegisterShowHideInstances interface to store arrays of component instances per ID
  • Created helper functions getActiveShowHide and getShowHideValue in new registryAccess.ts utility file
  • Updated registry logic to handle UID-based instance tracking for proper race condition handling

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
packages/bootstrap-vue-next/src/utils/registryAccess.ts New utility file with helper functions for accessing the show/hide registry
packages/bootstrap-vue-next/src/utils/keys.ts Added RegisterShowHideInstances interface and updated RegisterShowHideValue types
packages/bootstrap-vue-next/src/composables/useRegistry/index.ts Refactored registry to use arrays of instances with UID-based tracking
packages/bootstrap-vue-next/src/directives/BToggle/index.ts Updated to use new helper functions instead of direct map access
packages/bootstrap-vue-next/src/composables/useToggle/index.ts Simplified logic using new helper functions
packages/bootstrap-vue-next/src/components/BNavbar/BNavbarToggle.vue Updated to use new helper functions for cleaner code
packages/bootstrap-vue-next/tests/utils/registryAccess.spec.ts New test file for helper functions
packages/bootstrap-vue-next/tests/composables/useRegistry.spec.ts Comprehensive tests for registry race condition scenarios

@pkg-pr-new
Copy link

pkg-pr-new bot commented Nov 15, 2025

  • bsvn-vite-ts

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

commit: 7a1db93

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
packages/bootstrap-vue-next/src/composables/useRegistry/index.ts (1)

100-201: Fix unregister behavior when a component’s ID is updated via updateId

In _newShowHideRegistry, register() captures the initial id in its closure and unregister() looks up the holder using that original key:

const register = ({id, component, ...}: RegisterShowHideFnInput) => {
  // ...
  return {
    unregister() {
      const holder = values.value.get(id)
      // ...
    },
    updateId(newId: string, oldId: string) {
      const holder = values.value.get(oldId)
      // ... move instance to newId holder ...
    },
  }
}

After updateId(newId, oldId) runs, the instance is moved to the newId holder, and the old oldId holder may even be deleted. However, unregister() still uses the original id from registration, so it won’t find the instance anymore and won’t clean it up. This can leave stale entries in the registry for components that changed their ID before unmounting.

You can keep unregister() in sync with ID changes by tracking a mutable currentId inside the closure and updating it at the end of updateId:

   const register = ({
     id,
     component,
@@
     registerTrigger,
     unregisterTrigger,
   }: RegisterShowHideFnInput) => {
-    const instanceValue: RegisterShowHideMapValue = {
+    let currentId = id
+    const instanceValue: RegisterShowHideMapValue = {
       id,
       component,
       value: readonly(value),
@@
-    // Get or create the instances array for this ID
-    let instancesHolder = values.value.get(id)
+    // Get or create the instances array for this ID
+    let instancesHolder = values.value.get(currentId)
@@
     return {
       unregister() {
-        const holder = values.value.get(id)
+        const holder = values.value.get(currentId)
         if (!holder) return
@@
       updateId(newId: string, oldId: string) {
         const holder = values.value.get(oldId)
         if (!holder) return
@@
         // Clean up old ID if no instances remain
         if (holder.instances.length === 0) {
           values.value.delete(oldId)
         }
+        // Keep local id in sync so unregister() uses the latest key
+        currentId = newId
       },
     }
   }

With this change, components that update their ID and later unmount will correctly remove their instance from the registry, preventing leaks or unexpected lookups under stale IDs.

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

46-120: Normalize registry value to null before passing to getActiveShowHide

showHideMap can be undefined due to optional chaining, while getActiveShowHide is typed to accept Ref<...> | null. At runtime this works (the helper checks !registry), but it’s slightly at odds with the declared type and makes the call sites less explicit.

You can tighten this and avoid any nullability mismatches by coalescing to null where showHideMap is computed:

-  const provides = findProvides(binding, vnode)
-  const showHideMap = (provides as Record<symbol, RegisterShowHideValue>)[showHideRegistryKey]
-    ?.values
+  const provides = findProvides(binding, vnode)
+  const showHideMap =
+    (provides as Record<symbol, RegisterShowHideValue>)[showHideRegistryKey]?.values ?? null

and similarly in handleUnmount:

-  const provides = findProvides(binding, vnode)
-  const showHideMap = (provides as Record<symbol, RegisterShowHideValue>)[showHideRegistryKey]
-    ?.values
+  const provides = findProvides(binding, vnode)
+  const showHideMap =
+    (provides as Record<symbol, RegisterShowHideValue>)[showHideRegistryKey]?.values ?? null

This keeps the runtime behavior the same but makes the contract with getActiveShowHide explicit and type-safe.

packages/bootstrap-vue-next/tests/composables/useRegistry.spec.ts (1)

269-284: LGTM! Consider adding getActive verification for completeness.

The test correctly verifies that an instance moved to an existing ID is appended to that ID's instances array. While the ordering is validated, you could optionally add an assertion to verify that getActive() returns the moved instance (uid 1), making the test more complete:

 expect(holder?.instances[0].component.uid).toBe(2)
 expect(holder?.instances[1].component.uid).toBe(1)
+expect(holder?.getActive()?.component.uid).toBe(1)

This is optional since getActive behavior is already thoroughly tested elsewhere.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ca26be4 and da5f8ec.

📒 Files selected for processing (8)
  • packages/bootstrap-vue-next/src/components/BNavbar/BNavbarToggle.vue (2 hunks)
  • packages/bootstrap-vue-next/src/composables/useRegistry/index.ts (4 hunks)
  • packages/bootstrap-vue-next/src/composables/useToggle/index.ts (2 hunks)
  • packages/bootstrap-vue-next/src/directives/BToggle/index.ts (5 hunks)
  • packages/bootstrap-vue-next/src/utils/keys.ts (1 hunks)
  • packages/bootstrap-vue-next/src/utils/registryAccess.ts (1 hunks)
  • packages/bootstrap-vue-next/tests/composables/useRegistry.spec.ts (1 hunks)
  • packages/bootstrap-vue-next/tests/utils/registryAccess.spec.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.spec.ts

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Write tests following existing patterns in *.spec.ts files

Files:

  • packages/bootstrap-vue-next/tests/utils/registryAccess.spec.ts
  • packages/bootstrap-vue-next/tests/composables/useRegistry.spec.ts
packages/bootstrap-vue-next/src/components/**

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Create and modify Vue components only under packages/bootstrap-vue-next/src/components/

Files:

  • packages/bootstrap-vue-next/src/components/BNavbar/BNavbarToggle.vue
packages/bootstrap-vue-next/src/components/**/*.vue

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Keep component-specific styles within their respective .vue single-file components

Files:

  • packages/bootstrap-vue-next/src/components/BNavbar/BNavbarToggle.vue
🧠 Learnings (13)
📚 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/tests/utils/registryAccess.spec.ts
  • packages/bootstrap-vue-next/src/composables/useToggle/index.ts
  • packages/bootstrap-vue-next/src/composables/useRegistry/index.ts
  • packages/bootstrap-vue-next/src/components/BNavbar/BNavbarToggle.vue
  • packages/bootstrap-vue-next/src/directives/BToggle/index.ts
  • packages/bootstrap-vue-next/tests/composables/useRegistry.spec.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/utils/registryAccess.ts
  • packages/bootstrap-vue-next/src/composables/useToggle/index.ts
  • packages/bootstrap-vue-next/src/utils/keys.ts
  • packages/bootstrap-vue-next/src/composables/useRegistry/index.ts
  • packages/bootstrap-vue-next/src/directives/BToggle/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/composables/useToggle/index.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/composables/useToggle/index.ts
  • packages/bootstrap-vue-next/src/components/BNavbar/BNavbarToggle.vue
  • packages/bootstrap-vue-next/src/directives/BToggle/index.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/keys.ts
📚 Learning: 2025-05-28T07:32:45.658Z
Learnt from: xvaara
Repo: bootstrap-vue-next/bootstrap-vue-next PR: 2716
File: packages/bootstrap-vue-next/src/components/BTabs/BTabs.vue:425-430
Timestamp: 2025-05-28T07:32:45.658Z
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.

Applied to files:

  • packages/bootstrap-vue-next/src/components/BNavbar/BNavbarToggle.vue
  • packages/bootstrap-vue-next/src/directives/BToggle/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/components/BNavbar/BNavbarToggle.vue
  • packages/bootstrap-vue-next/src/directives/BToggle/index.ts
📚 Learning: 2025-05-26T17:28:35.902Z
Learnt from: VividLemon
Repo: bootstrap-vue-next/bootstrap-vue-next PR: 2691
File: packages/bootstrap-vue-next/src/composables/useBLinkHelper.ts:85-86
Timestamp: 2025-05-26T17:28:35.902Z
Learning: In the `useBLinkTagResolver` function in `packages/bootstrap-vue-next/src/composables/useBLinkHelper.ts`, the `routerComponentName` parameter is required and typed as `MaybeRefOrGetter<string>`, ensuring it cannot be undefined.

Applied to files:

  • packages/bootstrap-vue-next/src/components/BNavbar/BNavbarToggle.vue
📚 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/BToggle/index.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/BToggle/index.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/BToggle/index.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/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/** : Create and modify Vue components only under packages/bootstrap-vue-next/src/components/

Applied to files:

  • packages/bootstrap-vue-next/src/directives/BToggle/index.ts
🧬 Code graph analysis (6)
packages/bootstrap-vue-next/tests/utils/registryAccess.spec.ts (2)
packages/bootstrap-vue-next/src/utils/keys.ts (2)
  • RegisterShowHideMapValue (148-157)
  • RegisterShowHideInstances (163-166)
packages/bootstrap-vue-next/src/utils/registryAccess.ts (2)
  • getActiveShowHide (12-20)
  • getShowHideValue (29-35)
packages/bootstrap-vue-next/src/utils/registryAccess.ts (1)
packages/bootstrap-vue-next/src/utils/keys.ts (2)
  • RegisterShowHideInstances (163-166)
  • RegisterShowHideMapValue (148-157)
packages/bootstrap-vue-next/src/composables/useToggle/index.ts (1)
packages/bootstrap-vue-next/src/utils/registryAccess.ts (1)
  • getActiveShowHide (12-20)
packages/bootstrap-vue-next/src/composables/useRegistry/index.ts (1)
packages/bootstrap-vue-next/src/utils/keys.ts (2)
  • RegisterShowHideInstances (163-166)
  • RegisterShowHideMapValue (148-157)
packages/bootstrap-vue-next/src/directives/BToggle/index.ts (1)
packages/bootstrap-vue-next/src/utils/registryAccess.ts (1)
  • getActiveShowHide (12-20)
packages/bootstrap-vue-next/tests/composables/useRegistry.spec.ts (2)
packages/bootstrap-vue-next/src/utils/keys.ts (1)
  • RegisterShowHideFnInput (137-146)
packages/bootstrap-vue-next/src/composables/useRegistry/index.ts (1)
  • _newShowHideRegistry (100-207)
⏰ 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 (10)
packages/bootstrap-vue-next/src/components/BNavbar/BNavbarToggle.vue (1)

18-54: Show/hide registry helper integration in BNavbarToggle looks correct

collapseExpanded and toggleExpand now consistently use getShowHideValue/getActiveShowHide with appropriate guards and string/array handling, aligning with the new per‑ID holder model without changing visible behavior.

packages/bootstrap-vue-next/src/utils/keys.ts (1)

137-174: New RegisterShowHideInstances type and values ref shape align with multi-instance registry

The RegisterShowHideInstances holder and the updated RegisterShowHideValue.values type cleanly model multiple instances per ID and support the getActiveShowHide helper without breaking existing injection patterns.

packages/bootstrap-vue-next/tests/utils/registryAccess.spec.ts (1)

1-96: registryAccess tests provide solid coverage of helper semantics

The suite exercises null/undefined/missing cases and typical consumer patterns (BToggle, useToggle, BNavbarToggle), giving good confidence in the new helpers’ behavior.

packages/bootstrap-vue-next/src/composables/useToggle/index.ts (1)

10-55: useToggle’s resolution of active show/hide instances is consistent with the new registry model

myComponent correctly switches from direct map access to getActiveShowHide for both explicit IDs and parent-component discovery, keeping existing API/behavior while honoring the per‑ID instance holder abstraction.

packages/bootstrap-vue-next/src/utils/registryAccess.ts (2)

12-20: LGTM! Well-designed edge case handling.

The function correctly uses distinct sentinel values (null for falsy id, undefined for null registry) to help callers distinguish between different failure modes. The optional chaining and the use of getActive() properly integrates with the new per-ID instance holder structure.


29-35: LGTM! Clean implementation with sensible defaults.

The function correctly extracts the boolean visibility state from the active instance's reactive value, and sensibly defaults to false (hidden) when the instance is not found. The delegation to getActiveShowHide keeps the code DRY.

packages/bootstrap-vue-next/tests/composables/useRegistry.spec.ts (4)

7-72: LGTM! Excellent test setup and register coverage.

The createMockInput helper provides a clean way to create test fixtures. The register tests thoroughly verify:

  • Instance array creation for new IDs
  • Appending multiple instances with the same ID
  • Independent handling of multiple IDs

The test assertions properly validate the internal registry state.


74-129: LGTM! Critical race condition scenario well covered.

The getActive tests properly verify that the "last instance wins" strategy works correctly, including the key race condition scenario (lines 113-128) where a new instance mounts before an old one unmounts. This directly addresses the core issue described in PR #2897.


131-214: LGTM! Comprehensive unregister coverage with robust error handling.

The unregister tests thoroughly cover:

  • Selective removal by UID from arrays with multiple instances
  • Proper cleanup of empty map entries
  • Defensive handling of missing IDs or instances (lines 176-197)
  • Race condition cleanup where old instances unmount after new ones mount (lines 199-213)

The error-handling tests ensure the implementation is resilient to unexpected states.


310-405: Excellent! Comprehensive race condition test coverage.

These tests directly address the core issue from PR #2897 and thoroughly exercise the scenarios described in the PR objectives:

  • Rapid mount/unmount cycles (lines 311-332): Validates that multiple instances can coexist and cleanup works correctly
  • Out-of-order unmounting (lines 334-354): Ensures getActive adapts correctly regardless of unmount order
  • ID changes during races (lines 356-371): Validates the registry handles ID updates while old instances linger
  • Stress test (lines 373-404): 10 rapid remounts with random cleanup order—excellent for catching edge cases

The stress test at lines 373-404 is particularly valuable, as it validates that getActive() consistently returns the last element in the instances array throughout complex lifecycle scenarios.

@dwgray
Copy link
Member Author

dwgray commented Nov 16, 2025

@xvaara and @sebbayer - I believe this will handle the duplicate id/race condition described in #2897 robustly without adding too much additional complexity to the code.

@dwgray dwgray requested a review from xvaara November 16, 2025 01:25
@dwgray
Copy link
Member Author

dwgray commented Nov 17, 2025

@sebbayer - in addition, I believe you can test the change using the stackblitz link for the PR above
Or just replace the bsvn line in your package.json with "bootstrap-vue-next": "https://pkg.pr.new/bootstrap-vue-next/bootstrap-vue-next@2911", and re-install.

@sebbayer
Copy link
Contributor

@dwgray I checked in my reproduction example and it looks good. thank you very much!

xvaara
xvaara previously approved these changes Nov 30, 2025
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.

lgtm

@dwgray
Copy link
Member Author

dwgray commented Nov 30, 2025

@xvaara I had to merge from my other PR (it was a one line merge) - but the system is requiring you to approve again to get this through. Thanks!

@dwgray dwgray requested a review from xvaara December 4, 2025 20:14
@xvaara xvaara merged commit 6f880fa into bootstrap-vue-next:main Dec 6, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Duplicate id:s in showHide components can cause them to be missing in the registry in nuxt after navigation

3 participants