fix(popover, collapsible): DP-185811 cancel transitions on unmount - #1241
Conversation
|
Please add either the |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited) Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (2)
Prevents post-unmount CSS transition callbacks from running by adding beforeUnmount hooks that set _isUnmounting, guarding transition-complete handlers in DtPopover and DtCollapsible, and (for DtPopover) disabling in-flight transitions on the teleported content element during unmount. Overall Judgement: ✅ Ready to merge — Small, targeted changes with tests that reliably prevent transition callbacks from touching unmounted components and fix DP-185811. WalkthroughAdds unmount guards to Collapsible and Popover: components set Changes
Sequence Diagram(s)sequenceDiagram
participant Parent
participant Component as DtPopover/DtCollapsible
participant ContentEl as DOM:contentElement
participant EventBus as ParentListener
rect rgba(200,200,255,0.5)
Parent->>Component: trigger unmount()
Component->>Component: set _isUnmounting = true
end
rect rgba(200,255,200,0.5)
Component->>ContentEl: force style.transition = 'none' (Popover)
ContentEl-->>Component: cancel transitionend callbacks
end
rect rgba(255,200,200,0.5)
ContentEl->>Component: onEnter/LeaveTransitionComplete()
Component--xEventBus: early return when _isUnmounting (no emit)
end
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
Review rate limit: 9/10 reviews remaining, refill in 6 minutes. Comment |
Brad Paugh (braddialpad)
added
the
no-visual-test
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/dialtone-vue/components/popover/popover.vue (1)
912-937:⚠️ Potential issue | 🟠 MajorAdd guards after each
awaitto prevent side effects if unmount occurs during async operations.The transition handlers check
_isUnmountingat entry (lines 913, 928) but don't re-check after awaits. If the component unmounts duringawait this.focusFirstElement(),await this.$nextTick(), or other async operations, the handler continues and emits events or runs side effects on a destroyed component. ThebeforeUnmountcomment explicitly intends to prevent this. Add guards after each await to close the race window.Suggested hardening
async onLeaveTransitionComplete () { if (this._isUnmounting) return; if (this.modal) { await this.focusFirstElement(this.$refs.anchor); + if (this._isUnmounting) return; // await next tick in case the user wants to change focus themselves. await this.$nextTick(); + if (this._isUnmounting) return; this.enableScrolling(); } + if (this._isUnmounting) return; this.tip?.unmount(); this.$emit('opened', false); if (this.open !== null) { this.$emit('update:open', false); } }, async onEnterTransitionComplete () { if (this._isUnmounting) return; this.focusInitialElement(); // await next tick in case the user wants to change focus themselves. await this.$nextTick(); + if (this._isUnmounting) return; this.preventScrolling(); this.$emit('opened', true, this.$refs.popover__content); if (this.open !== null) { this.$emit('update:open', true); } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/dialtone-vue/components/popover/popover.vue` around lines 912 - 937, Add post-await unmount guards to both transition handlers: in onLeaveTransitionComplete and onEnterTransitionComplete, after each await (e.g., after await this.focusFirstElement(...), await this.$nextTick(), await this.focusInitialElement(), etc.) check this._isUnmounting and return early if true so no further side effects (calls to enableScrolling/preventScrolling, tip?.unmount, or this.$emit/update:open) run on an unmounted component; update the sequences around focusFirstElement, focusInitialElement, this.$nextTick, tip?.unmount, enableScrolling, preventScrolling, and the $emit/update:open branches to bail out immediately when unmounted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/dialtone-vue/components/popover/popover.vue`:
- Around line 912-937: Add post-await unmount guards to both transition
handlers: in onLeaveTransitionComplete and onEnterTransitionComplete, after each
await (e.g., after await this.focusFirstElement(...), await this.$nextTick(),
await this.focusInitialElement(), etc.) check this._isUnmounting and return
early if true so no further side effects (calls to
enableScrolling/preventScrolling, tip?.unmount, or this.$emit/update:open) run
on an unmounted component; update the sequences around focusFirstElement,
focusInitialElement, this.$nextTick, tip?.unmount, enableScrolling,
preventScrolling, and the $emit/update:open branches to bail out immediately
when unmounted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 9e0bd7d0-bacd-44ad-b97d-70d112083eea
📒 Files selected for processing (2)
packages/dialtone-vue/components/collapsible/collapsible.vuepackages/dialtone-vue/components/popover/popover.vue
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/dialtone-vue/components/collapsible/collapsible.test.js`:
- Around line 148-162: Extend the unmounting tests to also assert suppression of
the controlled-mode event by setting the component into controlled mode (make
open !== null via prop) and then verifying that calling
wrapper.vm.onLeaveTransitionComplete() and
wrapper.vm.onEnterTransitionComplete() do NOT emit 'update:open' in addition to
not emitting 'opened'; use wrapper.vm._isUnmounting = true and the existing
wrapper reference, and target the same methods (onLeaveTransitionComplete,
onEnterTransitionComplete) and emitted events ('opened' and 'update:open').
In `@packages/dialtone-vue/components/popover/popover.test.js`:
- Around line 315-324: The tests only assert the guarded path where
wrapper.vm._isUnmounting = true; add companion assertions that set
wrapper.vm._isUnmounting = false and then call onLeaveTransitionComplete() and
onEnterTransitionComplete() to verify wrapper.emitted('opened') contains the
expected emission(s); update the two test cases around onLeaveTransitionComplete
and onEnterTransitionComplete to first assert no emission when _isUnmounting =
true and then reset _isUnmounting = false, invoke the same handler, and assert
that wrapper.emitted('opened') is defined/contains the expected event to prove
the normal path still emits.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: a0bda81a-c5ac-420c-a4de-b1ec6280c91e
📒 Files selected for processing (2)
packages/dialtone-vue/components/collapsible/collapsible.test.jspackages/dialtone-vue/components/popover/popover.test.js
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/dialtone-vue/components/popover/popover.vue (1)
714-725:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRelease scroll lock during unmount cleanup.
If unmount happens while the modal popover is open,
preventScrolling()may already have disabled scrolling. With the new_isUnmountingearly returns,onLeaveTransitionComplete()no longer guaranteesenableScrolling()runs, so scrolling can stay locked after teardown.Suggested fix
beforeUnmount () { this._isUnmounting = true; // Cancel any in-progress CSS transitions so transitionend cannot fire // after this component is torn down and call into dead lifecycle methods. if (this.popoverContentEl) { this.popoverContentEl.style.transition = 'none'; } + // Ensure any modal scroll lock is always released on teardown. + if (this.isOpen && this.modal) { + this.enableScrolling(); + } this.tip?.destroy(); this.intersectionObserver?.disconnect(); this.mutationObserver?.disconnect(); this.removeReferences(); this.removeEventListeners(); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/dialtone-vue/components/popover/popover.vue` around lines 714 - 725, The beforeUnmount cleanup must release any scroll lock set by preventScrolling so enableScrolling is always called even if _isUnmounting causes early exits; update the beforeUnmount method (and/or related teardown path) to call enableScrolling() (or the component's corresponding scroll-unlock helper) unconditionally or whenever preventScrolling was previously applied, ensuring scroll is restored when popoverContentEl is torn down and before/after calling removeReferences/removeEventListeners, and reference the existing symbols beforeUnmount, _isUnmounting, preventScrolling, enableScrolling, and onLeaveTransitionComplete to locate and modify the logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/dialtone-vue/components/popover/popover.test.js`:
- Around line 315-335: Extend the two transition-complete tests to also assert
controlled-mode behavior: set the component into controlled mode by assigning a
non-null open prop (e.g., set wrapper props so open !== null), call
wrapper.vm.onLeaveTransitionComplete() and
wrapper.vm.onEnterTransitionComplete() as in the tests, and assert that
wrapper.emitted('update:open') is undefined (no update:open emitted) when open
!== null; also verify the opposite (update:open is emitted) when open is
null/uncontrolled to ensure the guard in onLeaveTransitionComplete and
onEnterTransitionComplete behaves correctly.
---
Outside diff comments:
In `@packages/dialtone-vue/components/popover/popover.vue`:
- Around line 714-725: The beforeUnmount cleanup must release any scroll lock
set by preventScrolling so enableScrolling is always called even if
_isUnmounting causes early exits; update the beforeUnmount method (and/or
related teardown path) to call enableScrolling() (or the component's
corresponding scroll-unlock helper) unconditionally or whenever preventScrolling
was previously applied, ensuring scroll is restored when popoverContentEl is
torn down and before/after calling removeReferences/removeEventListeners, and
reference the existing symbols beforeUnmount, _isUnmounting, preventScrolling,
enableScrolling, and onLeaveTransitionComplete to locate and modify the logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 47b68d28-4629-4e11-b1b7-d67b5ec3ab30
📒 Files selected for processing (3)
packages/dialtone-vue/components/collapsible/collapsible.test.jspackages/dialtone-vue/components/popover/popover.test.jspackages/dialtone-vue/components/popover/popover.vue
Belu Montoya (belumontoya)
left a comment
Belu Montoya (belumontoya)
left a comment
There was a problem hiding this comment.
no mayor issues from the ones already flagged.
|
Thanks! made the fixes |
|
✔️ Deploy previews ready! |
# [9.182.0](dialtone/v9.181.0...dialtone/v9.182.0) (2026-05-01) ### Bug Fixes * **Popover, Collapsible:** DP-185811 cancel transitions on unmount ([#1241](#1241)) ([6883a0e](6883a0e)) * **Rich Text Editor:** NO-JIRA fix multiple rich text issues ([#1240](#1240)) ([f285a3e](f285a3e)) ### Features * DLT-3352 refresh GEO standard, publish llms.txt, add freshness check ([#1235](#1235)) ([e43643d](e43643d))
# [3.220.0-next.4](dialtone-vue/v3.220.0-next.3...dialtone-vue/v3.220.0-next.4) (2026-05-01) ### Bug Fixes * **Popover, Collapsible:** DP-185811 cancel transitions on unmount ([#1241](#1241)) ([6883a0e](6883a0e)) * **Rich Text Editor:** NO-JIRA fix multiple rich text issues ([#1240](#1240)) ([f285a3e](f285a3e))
2 participants
🛠️ Type Of Change
📖 Jira Ticket
DP-185811
📖 Description
Fixes a race condition where a CSS leave transition (fade/collapse) could complete after
DtPopoverorDtCollapsiblewas already unmounted, causing Vue to attempt updates on a dead component instance.💡 Context
Reported from the product side — implementing components were unmounting while a popover/collapsible leave animation was in progress. The
after-leavehook then fired on the already-torn-down component, producing warnings and potential errors about updating unmounted component instances.📝 Checklist
For all PRs:
For all Vue changes: