Skip to content

fix(ui-core-components): dismiss non-modal popovers outside#30358

Open
shah-harshit wants to merge 2 commits into
mainfrom
ui/fix-non-modal-select-popover-dismissal
Open

fix(ui-core-components): dismiss non-modal popovers outside#30358
shah-harshit wants to merge 2 commits into
mainfrom
ui/fix-non-modal-select-popover-dismissal

Conversation

@shah-harshit

@shah-harshit shah-harshit commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Describe your changes

Adds opt-in outside-interaction dismissal for non-modal core select popovers. The behavior honors shouldCloseOnInteractOutside filters and preserves the existing Safari-safe outline styling from current main.

Type of change

  • Bug fix

Tests

Use cases covered

  • A dismissible non-modal popover requests closure after an allowed outside interaction.
  • Trigger or filtered outside interactions can keep the popover open.
  • Modal and non-dismissible popovers retain React Aria's existing behavior.

Verification performed

  • ESLint on the changed core-components file
  • Prettier check on the changed core-components file
  • Core-components TypeScript check (tsc --noEmit)
  • Core-components production build (vite build)
  • git diff --check

Unit tests

Not added: the core-components package does not currently expose a unit-test runner.

Backend integration tests

  • Not applicable (no backend API changes).

Ingestion integration tests

  • Not applicable (no ingestion changes).

Playwright tests

  • Not applicable (core primitive behavior only).

UI screen recording / screenshots

Not applicable — this changes dismissal behavior without changing visuals.

Checklist

  • No JSON Schema or migration changes are required.
  • The transferred change was rebased onto current main styling.

Greptile Summary

This PR adds opt-in outside dismissal for non-modal select popovers. The main changes are:

  • Uses React Aria overlay context to close context-owned popovers.
  • Preserves trigger and outside-interaction filters.
  • Keeps existing behavior for modal and non-dismissible popovers.
  • Forwards the popover element ref.

Confidence Score: 5/5

This looks safe to merge.

  • The context-owned path now calls overlayState.close().
  • Controlled and standalone paths continue to use onOpenChange.
  • No blocking issue was found in the updated code.

Important Files Changed

Filename Overview
openmetadata-ui-core-components/src/main/resources/ui/src/components/base/select/popover.tsx Adds context-aware outside dismissal for non-modal popovers and closes context-owned overlay state.

Reviews (2): Last reviewed commit: "fix(ui-core-components): close context-o..." | Re-trigger Greptile

Context used (3)

  • Context used - openmetadata-ui-core-components/CLAUDE.md (source)
  • Context used - CLAUDE.md (source)
  • Context used - AGENTS.md (source)

@shah-harshit shah-harshit self-assigned this Jul 22, 2026
@shah-harshit shah-harshit added the UI UI specific issues label Jul 22, 2026
@shah-harshit
shah-harshit requested a review from karanh37 as a code owner July 22, 2026 15:03
@shah-harshit shah-harshit added safe to test Add this label to run secure Github workflows on PRs skip-pr-checks Bypass PR metadata validation check labels Jul 22, 2026
@gitar-bot

gitar-bot Bot commented Jul 22, 2026

Copy link
Copy Markdown
Code Review ✅ Approved 2 resolved / 2 findings

Implements opt-in outside-interaction dismissal for non-modal select popovers while preserving Safari-safe styling. Resolves potential issues with the unused onOpenChange callback and redundant trigger-click dismissal events.

✅ 2 resolved
Bug: Dismissal relies on onOpenChange, unused by consumers

📄 openmetadata-ui-core-components/src/main/resources/ui/src/components/base/select/popover.tsx:29-43
The dismissal handler closes the popover by calling props.onOpenChange?.(false), but every consumer (ComboBox, AutocompleteBase, MultiSelectBase) drives the popover's open state through the react-aria-components trigger context and never passes onOpenChange to this wrapper. When those components eventually set isDismissable, the outside-interaction handler will call an undefined callback and the popover will not close — the feature is effectively a no-op for its intended consumers. Close the popover via the OverlayTrigger/PopoverContext state (or require consumers to wire onOpenChange), and document that isDismissable has no effect unless onOpenChange is supplied.

Edge Case: Trigger click can fire dismissal alongside trigger toggle

📄 openmetadata-ui-core-components/src/main/resources/ui/src/components/base/select/popover.tsx:29-43
useInteractOutside fires for any interaction outside the popover element, including the trigger button that normally toggles the popover. Unless the consumer supplies a shouldCloseOnInteractOutside filter that returns false for the trigger, both the trigger's own toggle and this handler's onOpenChange(false) can run, causing flicker or an immediate reopen. Consider excluding the trigger element by default rather than relying solely on consumer-provided filtering.

Options

Display: compact → Showing less information.

Comment with these commands to change the behavior for this request:

Compact
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

@shah-harshit
shah-harshit enabled auto-merge July 22, 2026 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

safe to test Add this label to run secure Github workflows on PRs skip-pr-checks Bypass PR metadata validation check UI UI specific issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant