-
-
Notifications
You must be signed in to change notification settings - Fork 173
Fix/show hide registry race condition #2911
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.
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
Fix/show hide registry race condition #2911
Conversation
|
|
|
Note Other AI code review bot(s) detectedCodeRabbit 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. WalkthroughReplaces single-instance-per-ID registry entries with per-ID holders containing multiple instances and an active getter; adds registryAccess helpers ( Changes
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
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (8)📚 Learning: 2025-08-19T14:23:46.775ZApplied to files:
📚 Learning: 2025-11-30T17:09:16.276ZApplied to files:
📚 Learning: 2025-09-30T23:57:21.526ZApplied to files:
📚 Learning: 2025-11-30T17:09:16.276ZApplied to files:
📚 Learning: 2025-11-30T17:09:16.276ZApplied to files:
📚 Learning: 2025-11-30T17:09:16.276ZApplied to files:
📚 Learning: 2025-05-28T07:57:19.915ZApplied to files:
📚 Learning: 2025-05-28T07:01:55.095ZApplied to files:
🪛 markdownlint-cli2 (0.18.1)packages/nuxt/CHANGELOG.md19-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.md19-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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this 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
RegisterShowHideInstancesinterface to store arrays of component instances per ID - Created helper functions
getActiveShowHideandgetShowHideValuein newregistryAccess.tsutility 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 |
packages/bootstrap-vue-next/src/composables/useRegistry/index.ts
Outdated
Show resolved
Hide resolved
commit: |
There was a problem hiding this 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 updateIdIn
_newShowHideRegistry,register()captures the initialidin its closure andunregister()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 thenewIdholder, and the oldoldIdholder may even be deleted. However,unregister()still uses the originalidfrom 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 mutablecurrentIdinside the closure and updating it at the end ofupdateId: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 tonullbefore passing to getActiveShowHide
showHideMapcan beundefineddue to optional chaining, whilegetActiveShowHideis typed to acceptRef<...> | 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
nullwhereshowHideMapis 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 ?? nulland 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 ?? nullThis keeps the runtime behavior the same but makes the contract with
getActiveShowHideexplicit 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
getActivebehavior is already thoroughly tested elsewhere.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 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.tspackages/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.tspackages/bootstrap-vue-next/src/composables/useToggle/index.tspackages/bootstrap-vue-next/src/composables/useRegistry/index.tspackages/bootstrap-vue-next/src/components/BNavbar/BNavbarToggle.vuepackages/bootstrap-vue-next/src/directives/BToggle/index.tspackages/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.tspackages/bootstrap-vue-next/src/composables/useToggle/index.tspackages/bootstrap-vue-next/src/utils/keys.tspackages/bootstrap-vue-next/src/composables/useRegistry/index.tspackages/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.tspackages/bootstrap-vue-next/src/components/BNavbar/BNavbarToggle.vuepackages/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.vuepackages/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.vuepackages/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
collapseExpandedandtoggleExpandnow consistently usegetShowHideValue/getActiveShowHidewith 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 registryThe
RegisterShowHideInstancesholder and the updatedRegisterShowHideValue.valuestype cleanly model multiple instances per ID and support thegetActiveShowHidehelper without breaking existing injection patterns.packages/bootstrap-vue-next/tests/utils/registryAccess.spec.ts (1)
1-96: registryAccess tests provide solid coverage of helper semanticsThe 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
myComponentcorrectly switches from direct map access togetActiveShowHidefor 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 togetActiveShowHidekeeps the code DRY.packages/bootstrap-vue-next/tests/composables/useRegistry.spec.ts (4)
7-72: LGTM! Excellent test setup and register coverage.The
createMockInputhelper 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
getActivetests 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 I checked in my reproduction example and it looks good. thank you very much! |
xvaara
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
|
@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! |
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
Small replication
See #2897
PR checklist
What kind of change does this PR introduce? (check at least one)
fix(...)feat(...)fix(...)docs(...)The PR fulfills these requirements:
CHANGELOGis generated from these messages, and determines the next version type. Pull requests that do not follow conventional commits or do not have an override will be deniedSummary by CodeRabbit
Refactor
Tests
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.