fix(core/tooltip): guard popover calls against detached dialog#2611
fix(core/tooltip): guard popover calls against detached dialog#2611SAY-5 wants to merge 2 commits into
Conversation
Signed-off-by: Sai Asish Y <say.apm35@gmail.com>
✅ Deploy Preview for ix-storybook ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughGuards are added inside the delayed ChangesTooltip Popover Guard Fixes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds safety checks to prevent calling showPopover on disconnected dialogs or when a popover is already open, and avoids calling hidePopover when the popover is closed, accompanied by new unit tests. A review comment identifies a bug where the early return check for :popover-open prevents the tooltip from correctly transitioning and updating its position when switching between different anchor elements, and provides a code suggestion to resolve this.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if (!dialog.isConnected || dialog.matches(':popover-open')) { | ||
| return; | ||
| } | ||
| this.setAnchorElement(anchorElement); | ||
| dialog.showPopover(); |
There was a problem hiding this comment.
If the tooltip is currently visible for Anchor A, and the user hovers over Anchor B before the hide delay of Anchor A completes, the hide timeout is cleared, meaning the popover remains open (:popover-open is true).
When the show timeout for Anchor B fires, dialog.matches(':popover-open') will evaluate to true. With the current implementation, this causes an early return, which skips updating the anchor element (setAnchorElement) and repositioning the tooltip (applyTooltipPosition). As a result, the tooltip will remain stuck at Anchor A instead of transitioning to Anchor B.
To fix this, we should only guard the dialog.showPopover() call itself with !dialog.matches(':popover-open'), while still allowing the anchor update and repositioning logic to execute.
| if (!dialog.isConnected || dialog.matches(':popover-open')) { | |
| return; | |
| } | |
| this.setAnchorElement(anchorElement); | |
| dialog.showPopover(); | |
| if (!dialog.isConnected) { | |
| return; | |
| } | |
| this.setAnchorElement(anchorElement); | |
| if (!dialog.matches(':popover-open')) { | |
| dialog.showPopover(); | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/components/tooltip/test/tooltip.spec.tsx`:
- Around line 16-32: The test suite for the showTooltip method currently only
covers the disconnected branch guard path, but there are two branches that need
testing: disconnected and popover-open. Add a new test case (similar in
structure to the existing "does not call showPopover on a dialog that was
removed before the delay" test) that specifically tests the popover-open guard
path by setting up a scenario where the dialog is already open with the
:popover-open pseudo-class before calling showTooltip, and verify that the
appropriate guard logic prevents duplicate popover calls.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: adeec3fc-4051-45a6-8a65-3faa90eb041e
📒 Files selected for processing (2)
packages/core/src/components/tooltip/test/tooltip.spec.tsxpackages/core/src/components/tooltip/tooltip.tsx
| it('does not call showPopover on a dialog that was removed before the delay', async () => { | ||
| const { root, waitForChanges } = await render( | ||
| <ix-tooltip show-delay={0}></ix-tooltip> | ||
| ); | ||
| const tooltip = root as HTMLIxTooltipElement; | ||
| const dialog = tooltip.shadowRoot!.querySelector('dialog')!; | ||
| const showPopover = vi.fn(); | ||
| dialog.showPopover = showPopover; | ||
|
|
||
| const anchor = document.createElement('div'); | ||
| await tooltip.showTooltip(anchor); | ||
| dialog.remove(); | ||
| await flushTimeout(); | ||
| await waitForChanges(); | ||
|
|
||
| expect(showPopover).not.toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Add a test for the showTooltip “already open” guard path.
The new showTooltip guard has two branches (disconnected and :popover-open), but only the disconnected branch is covered here.
Proposed test addition
describe('ix-tooltip', () => {
+ it('does not call showPopover when the popover is already open', async () => {
+ const { root, waitForChanges } = await render(
+ <ix-tooltip show-delay={0}></ix-tooltip>
+ );
+ const tooltip = root as HTMLIxTooltipElement;
+ const dialog = tooltip.shadowRoot!.querySelector('dialog')!;
+
+ const showPopover = vi.fn();
+ dialog.showPopover = showPopover;
+ vi.spyOn(dialog, 'matches').mockImplementation((selector: string) => {
+ if (selector === ':popover-open') return true;
+ return false;
+ });
+
+ const anchor = document.createElement('div');
+ await tooltip.showTooltip(anchor);
+ await flushTimeout();
+ await waitForChanges();
+
+ expect(showPopover).not.toHaveBeenCalled();
+ });Also applies to: 34-54
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/components/tooltip/test/tooltip.spec.tsx` around lines 16 -
32, The test suite for the showTooltip method currently only covers the
disconnected branch guard path, but there are two branches that need testing:
disconnected and popover-open. Add a new test case (similar in structure to the
existing "does not call showPopover on a dialog that was removed before the
delay" test) that specifically tests the popover-open guard path by setting up a
scenario where the dialog is already open with the :popover-open pseudo-class
before calling showTooltip, and verify that the appropriate guard logic prevents
duplicate popover calls.
Signed-off-by: Sai Asish Y <say.apm35@gmail.com>
|
| private isPopoverOpen(dialog: HTMLElement) { | ||
| try { | ||
| return dialog.matches(':popover-open'); | ||
| } catch { | ||
| // Engines without :popover-open support (e.g. jsdom) throw on the | ||
| // selector; treat the popover as not open in that case. | ||
| return false; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Is the css selector really necessary here? DId you consider to use the open attribute of the instance? https://developer.mozilla.org/en-US/docs/Web/API/HTMLDialogElement/open
2 participants
GitHub Issue Number: #2559
💡 What is the current behavior?
ix-tooltipresolves the<dialog>element withdialogRef.waitForCurrent()and then callsdialog.showPopover()/dialog.hidePopover()inside a deferredsetTimeout. If the tooltip is disconnected (or the popover state changes) before the timeout fires, the resolved element is no longer connected, which throwsdialog.hidePopover is not a function/showPopovererrors. We see these frequently in RUM.🆕 What is the new behavior?
The deferred callbacks now check the dialog before acting:
showTooltipskips when the dialog is not connected or already open, andhideTooltiponly callshidePopover()when the popover is actually open. This matches the existing guards used inix-dropdown.🏁 Checklist
A pull request can only be merged if all of these conditions are met (where applicable):
pnpm test)pnpm lint)pnpm build, changes pushed)👨💻 Help & support
Summary by CodeRabbit
Bug Fixes