Refactor Discover v2 rules menu: merge alerts into legacy-rules submenu#259649
Conversation
Instead of introducing a separate `legacyRules` registry ID and requiring profile extensions to dual-register items, this keeps the existing `alerts` registration path intact. When v2 is enabled, items from the `alerts` menu are merged into the `createRule` menu's `legacy-rules` submenu, and the `alerts` menu item is removed. This eliminates the `AppMenuActionId.legacyRules` indirection, removes duplicate item registration in profile extensions, and fixes the empty menu item bug caused by the hidden `legacyRules` container appearing in overflow popovers. Made-with: Cursor
ApprovabilityVerdict: Needs human review The author doesn't own any of the changed files (all owned by @elastic/kibana-data-discovery or @elastic/obs-exploration-team), and there's an unresolved design question from another reviewer about exposing the deleteItem API. The changes affect UI menu structure and runtime behavior, warranting review by designated code owners. You can customize Macroscope's approvability policy. Learn more. |
| href: getManageRulesUrl(services), | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
We no longer need to re-craft this manually, it comes over when we merge in the Alerts menu into the v2 Rules sub-menu
| order: 1, | ||
| label: i18n.translate('discover.localMenu.createRuleTitle', { | ||
| defaultMessage: 'Create rule', | ||
| defaultMessage: 'Create v2 ES|QL rule', |
There was a problem hiding this comment.
We will be explicit for M1, decide on much better naming and labeling for M2
| order: 2, | ||
| label: i18n.translate('discover.localMenu.legacyRulesTitle', { | ||
| defaultMessage: 'Create legacy rules', | ||
| defaultMessage: 'Create v1 rules', |
There was a problem hiding this comment.
Must avoid the word "legacy" for v1 rules (v1 is also terrible but we will fix for M2)
| items: [], | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
We don't need to register this "dummy" item, we re-use the alerts one, then remove/delete it after we've merged its contents into the sub-nav.
| AppMenuActionId.legacyRules | ||
| AppMenuActionId.alerts | ||
| ); | ||
| registry.deleteItem(AppMenuActionId.alerts); |
There was a problem hiding this comment.
new line to remove the old "alerts" menu item, we've merged its contents into the v2 Rules sub-nav item for M1
dominiqueclarke
left a comment
There was a problem hiding this comment.
Code review plus manual test. LGTM!
| AppMenuActionId.legacyRules | ||
| AppMenuActionId.alerts | ||
| ); | ||
| registry.deleteItem(AppMenuActionId.alerts); |
There was a problem hiding this comment.
nit: is there any benefit in exposing the deleteItem method from the registry over just deleting it in mergePopoverItems?
There was a problem hiding this comment.
Fair point, I think I like how explicit it is here, so we can see the flow in line.
The refactor removed the only usage of canCreateESQLRule inside the memo body (the showLegacyAlerts condition), but left it in the dependency array. ESLint's react-hooks/exhaustive-deps flagged it. Made-with: Cursor
- Add deleteItem() tests to AppMenuRegistry (3 new tests)
- Update get_create_rule tests to match new labels ("Create v2 ES|QL
rule", "Create v1 rules") and empty legacy-rules submenu (items are
now merged in at runtime from the alerts menu, not hardcoded)
- Remove tests for items that are no longer pre-populated in the
legacy-rules submenu (search threshold, manage rules)
Made-with: Cursor
⏳ Build in-progress, with failures
Failed CI StepsTest Failures
History
|
Summary
Refactors how the v2 "Rules" menu in Discover handles legacy (v1) rule types, replacing the
legacyRulesregistry indirection with a simpler merge-and-delete approach.Before: A new
AppMenuActionId.legacyRuleswas introduced as a hidden container. Profile extensions had to dual-register items to bothalertsandlegacyRules. ThelegacyRulescontainer caused an empty menu item bug in overflow popovers.After: Profile extensions register to
alertsas they always have. When v2 is enabled, items fromalertsare merged into thecreateRulemenu'slegacy-rulessubmenu, thenalertsis removed. No new registry IDs, no dual registration, no empty menu items.Changes
app_menu_registry.ts: AddeddeleteItem()method to remove menu items by IDtypes.ts: RemovedAppMenuActionId.legacyRulesenum valueget_create_rule.tsx: Removed hardcoded legacy items from the v2 menu (emptylegacy-rulessubmenu is populated by merge). Updated labels to "Create v2 ES|QL rule" and "Create v1 rules"use_top_nav_links.tsx: RemovedlegacyRulescontainer registration. Changed merge source fromlegacyRulestoalerts. AddeddeleteItemcall to removealertsafter merge. Always registeralertsmenu (not gated byshowLegacyAlerts)get_app_menu.tsx(observability profile): Removed duplicateregisterPopoverItemcalls tolegacyRules, keeping onlyalertsregistrationNet effect: -88 lines, +18 lines
Test plan
Made with Cursor