Skip to content

test(popover): cover Escape dismissal with nested tooltips - #1738

Open
mnkprs wants to merge 1 commit into
spartan-ng:mainfrom
mnkprs:test/popover-tooltip-escape-1663
Open

mnkprs wants to merge 1 commit into
spartan-ng:mainfrom
mnkprs:test/popover-tooltip-escape-1663

Conversation

@mnkprs

@mnkprs mnkprs commented Sep 9, 2026 •

Copy link
Copy Markdown

What does this PR do?

Adds browser regression coverage for Escape dismissal when a popover contains tooltips, as reported in #1663:

  • Escape closes a popover while its tooltip is visible.
  • Escape still closes it after the tooltip has hidden.
  • With nested popovers and a tooltip, the first Escape closes only the inner popover; a second closes the outer one. Closing also removes the tooltip.

These cases already pass on current main (0e05db585). The current tooltip uses CDK Overlay directly and does not enter BrnOverlayService's private stack. CDK skips overlays without keydown subscribers when dispatching keyboard events. This PR records the expected integration behavior without changing production dismissal logic or claiming to reproduce the reported failure on current code.

Related: #1663.

PR Checklist

  • Commit message follows the contributing guidelines.
  • Tests added.
  • Documentation not needed: test coverage only, no public API or behavior change.

PR Type

  • Other: regression test coverage.

Which package are you modifying?

  • popover
  • tooltip (integration coverage)

Validation

  • pnpm install --frozen-lockfile
  • pnpm exec vitest run --config libs/helm/vite.config.mts — all 119 tests in 33 files pass in Chromium, including the three new cases.
  • pnpm exec eslint libs/helm/popover/src/lib/hlm-popover-overlay.spec.ts
  • pnpm exec prettier --check libs/helm/popover/src/lib/hlm-popover-overlay.spec.ts
  • git diff --check

Does this PR introduce a breaking change?

  • No

Summary by CodeRabbit

  • Tests
    • Added coverage for dismissing popovers with Escape when tooltips are visible or have been hidden.
    • Verified that nested popovers close from the innermost layer first, followed by the outer popover on a second Escape press.

@coderabbitai

coderabbitai Bot commented Sep 9, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 1ffca714-4211-4f0a-88ed-8ab47e8491b3

📥 Commits

Reviewing files that changed from the base of the PR and between a499688 and 72aa3e4.

📒 Files selected for processing (1)
  • libs/helm/popover/src/lib/hlm-popover-overlay.spec.ts

Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.


📝 Walkthrough

Walkthrough

The popover overlay tests now cover tooltip interactions in nested popovers. They check Escape dismissal with visible or hidden tooltips and verify that nested popovers close from the inner popover outward.

Changes

Popover tooltip Escape behavior

Layer / File(s) Summary
Nested popover test fixture and dismissal cases
libs/helm/popover/src/lib/hlm-popover-overlay.spec.ts
Adds a nested popover fixture with tooltips. Tests Escape behavior with visible and hidden tooltips, and checks ordered popover dismissal.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Other

Merge Risk: ⚪ Minimal · up to 72aa3

The new tests cover Escape dismissal with tooltips and nested popovers; no merge-blocking risk is identified.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: adding popover regression tests for Escape-key dismissal with nested tooltips.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

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.

@greptile-apps

greptile-apps Bot commented Sep 9, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerView in GreptileConfidence Score: 4/5

The test-only change appears safe to merge, with a non-blocking import-boundary cleanup recommended.

Findings

  1. P2 Internal tooltip import ▶

Summary

  • Covers dismissal while a tooltip is visible and after it hides.
  • Verifies nested popovers close from inner to outer while removing the tooltip.
  • Does not modify production behavior.

Comment thread libs/helm/popover/src/lib/hlm-popover-overlay.spec.ts Outdated
@mnkprs
mnkprs force-pushed the test/popover-tooltip-escape-1663 branch from a499688 to 72aa3e4 Compare September 25, 2026 17:35

This branch has not been deployed

No deployments
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.

1 participant