-
Notifications
You must be signed in to change notification settings - Fork 3
New OSS::ContextMenu::Panel component #626
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
base: feature/bulk-action-workflow
Are you sure you want to change the base?
Conversation
804c768 to
586ad40
Compare
42ff1ac to
cdf0be3
Compare
586ad40 to
5ff494e
Compare
| export type ContextMenuItem = { | ||
| items?: ContextMenuItem[]; | ||
| groupKey?: string; | ||
| rowRenderer?: ReturnType<typeof ensureSafeComponent>; // move to Component |
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.
ensureSafeComponent returns deprecation warning, should we replace by using direct class import like recommended by ember ?
https://github.com/embroider-build/embroider/blob/main/docs/replacing-component-helper.md#when-youre-passing-a-component-to-someone-else
| @@ -0,0 +1,4 @@ | |||
| <div> | |||
| 🦆 - | |||
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.
🦆
| willDestroy(): void { | ||
| super.willDestroy(); | ||
| this.currentPanel.querySelector('.oss-scrollable-panel-content')?.removeEventListener('scroll', this.onScrollbound); | ||
| } |
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.
Missing cleanup call causes memory leak on destroy
High Severity
The cleanupDrodpownAutoplacement function returned by attachDropdown is stored but never invoked in willDestroy. The attachDropdown utility returns a cleanup function from floating-ui's autoUpdate that needs to be called to stop position listeners. Without this cleanup call, event listeners continue running and references are never released after the component is destroyed, causing a memory leak each time a context menu panel is opened and closed.
Additional Locations (1)
| @action | ||
| willDestroy(): void { | ||
| super.willDestroy(); | ||
| this.currentPanel.querySelector('.oss-scrollable-panel-content')?.removeEventListener('scroll', this.onScrollbound); |
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.
Null reference error when panel destroyed early
Medium Severity
In willDestroy, this.currentPanel.querySelector(...) is called without checking if currentPanel exists. The property is declared with declare (uninitialized) and only set in registerPanel. If the component is destroyed before registerPanel fires (e.g., rapid conditional toggling), currentPanel remains undefined and calling .querySelector() on it throws a TypeError.
| <OSS::ScrollablePanel | ||
| id={{this.portalId}} | ||
| class="context-menu-panel__scrollable-container" | ||
| {{did-insert this.registerPanel}} |
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.
Duplicate registration causes leaked listeners and overwrites
Medium Severity
The template calls registerPanel via did-insert on both OSS::ScrollablePanel (line 7) and OSS::InfiniteSelect (line 20). Since scheduleOnce uses a new inline arrow function each call, both invocations of initializeDropdown execute. The second attachDropdown call overwrites cleanupDrodpownAutoplacement, losing the first cleanup function and leaking that auto-update listener. Additionally, scroll listeners may be added to both elements while currentPanel gets overwritten.
Additional Locations (1)
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.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| @items={{@items}} | ||
| @searchEnabled={{false}} | ||
| @onSelect={{this.noop}} | ||
| @onClose={{this.closeDropdown}} |
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.
Reference to undefined closeDropdown method
Medium Severity
The template references this.closeDropdown for the @onClose callback, but no closeDropdown method exists in the component class. This results in undefined being passed to OSS::InfiniteSelect, meaning keyboard events like Escape and Tab that trigger onClose won't have any effect on closing the context menu panel.
| class="infinit-select-option-panel-container {{if (eq index this.subReferenceIndex) 'active' ''}}" | ||
| /> | ||
| {{else if item.rowRenderer}} | ||
| <item.rowRenderer @item={{item}} {{on "mouseenter" this.closeSubMenu}} {{on "click" this.closeSubMenu}} /> |
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.
Custom rowRenderer items don't trigger item action on click
Medium Severity
Items with a custom rowRenderer have their action callback silently ignored. The click handler only calls this.closeSubMenu and doesn't invoke item.action. This is inconsistent with regular items, which call the action via @onSelect={{item.action}} on the Option component. Users defining custom row renderers with actions would expect those actions to execute on click.
| @@ -0,0 +1,4 @@ | |||
| <div> | |||
| 🦆 - | |||
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.
56b55a9 to
ef65320
Compare
| @disabled={{item.disabled}} | ||
| @onSelect={{item.action}} | ||
| {{on "mouseenter" this.closeSubMenu}} | ||
| {{on "click" this.closeSubMenu}} |
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.
What does this PR do?
Related to: #DRA-4463
What are the observable changes?
New
ContextMenu::Panelcomponent, which displays a context menu panel anchored to a specified reference target. It supports nested submenus, customizable placement, and offset options. The panel can trigger actions when menu items are selected and handle mouse leave events.Furthermore each row can be customized by passing a component in the
rowRendererEnregistrement.de.l.ecran.2026-01-28.a.10.58.09.mov
Good PR checklist
Note
Medium Risk
Adds a new floating/portal-based context menu and updates shared
attachDropdownbehavior (offset typing + maxHeight fix), which could affect positioning/styling of other dropdowns relying on this utility.Overview
Introduces a new
OSS::ContextMenu::Panelcomponent that renders a floating, anchored context-menu panel (viaattachDropdown) and supports nested submenus, click-outside handling, scroll-driven submenu closing, and optional per-row custom rendering viarowRenderer.Extends
attach-dropdownto accept object offsets ({ mainAxis, crossAxis }) and fixes an incorrectmaxHeightstyle assignment; adds new styling for the context menu, Storybook docs, a dummy-app usage example, and integration tests covering positioning, offsets, submenu behavior, custom row rendering, and mouse-leave callbacks.Written by Cursor Bugbot for commit ef65320. This will update automatically on new commits. Configure here.