-
-
Notifications
You must be signed in to change notification settings - Fork 173
fix(BToggle): stop looking for missing targets after directive is unmounted #2857
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(BToggle): stop looking for missing targets after directive is unmounted #2857
Conversation
|
|
WalkthroughThe vBToggle directive’s internal loop now includes a guard to stop iterating if the element’s dataset.bvtoggle is cleared (e.g., after unmount). Only the while-condition changed; comments were added. No public APIs or other logic paths were modified. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor C as Component
participant D as vBToggle Directive
participant E as Element (dataset.bvtoggle)
participant T as Target Resolver
C->>D: mount/update
D->>E: read dataset.bvtoggle
loop up to 5 iterations with guard (stops if bvtoggle cleared)
alt bvtoggle present
D->>T: resolve show/hide targets
T-->>D: target(s) found or not
else bvtoggle cleared (unmounted)
Note over D,E: Guard exits loop immediately
end
end
C-->>D: unmount (clears dataset.bvtoggle)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
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
🧹 Nitpick comments (1)
packages/bootstrap-vue-next/src/directives/BToggle/index.ts (1)
69-75: Re-check unmounted state after the await to avoid a late warn.If the directive unmounts during the 100ms delay on the final attempt, a warning can still fire. Add a post-await guard before logging.
Apply this diff:
- count++ - await new Promise((resolve) => setTimeout(resolve, 100)) + count++ + await new Promise((resolve) => setTimeout(resolve, 100)) + // Abort if directive got unmounted during the delay + if (!(el as HTMLElement).dataset.bvtoggle) break if (count < 4) continue // eslint-disable-next-line no-console console.warn(`[v-b-toggle] Target with ID ${targetId} not found`) break
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/bootstrap-vue-next/src/directives/BToggle/index.ts(1 hunks)
⏰ 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 (1)
packages/bootstrap-vue-next/src/directives/BToggle/index.ts (1)
65-66: Good guard to stop retries after unmount.The added condition prevents post-unmount polling and aligns with the PR goal.
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.
Nice little check!
* upstream/main: chore: release main (bootstrap-vue-next#2868) fix(useModalOrchestrator): circular dependency (bootstrap-vue-next#2874) docs(BModal): Parity pass (bootstrap-vue-next#2866) docs: Enable directly loading examples into StackBlitz (bootstrap-vue-next#2869) fix(BApp): wrap our test app in BApp in main.ts to enable easy verification of useModal, etc. (bootstrap-vue-next#2865) export useScrollLock() (bootstrap-vue-next#2854) chore: release main (bootstrap-vue-next#2858) fix(BToggle): stop looking for missing targets after directive is unmounted (bootstrap-vue-next#2857) chore: release main (bootstrap-vue-next#2851) Fix modal transition delays by making TransitionGroup name conditional (bootstrap-vue-next#2845) chore: release main (bootstrap-vue-next#2842) fix(BTable): events being wrongly stopped when sent from elements inside TRs (bootstrap-vue-next#2841)
Describe the PR
The BToggle directive searches for targets in its mounted hook. If a target isn't immediately found, it calls setTimeout and keeps looking for it for 400ms. If the directive is unmounted before that 400ms is up, it still keeps looking even though there's no point anymore. This PR adds a check on that loop that exits it if the directive has been unmounted.
Small replication
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