-
Notifications
You must be signed in to change notification settings - Fork 0
Notification Rule Processing Engine #6
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: notification-rule-baseline
Are you sure you want to change the base?
Notification Rule Processing Engine #6
Conversation
* wip * Add working actions for GMA rules based on Prom-only API * Remove Ruler-loader related code for Grafana rules Co-authored-by: Sonia Augilar <[email protected]> * Remove outdated tests * add some comments * remove commented code * remove showLocation property * Add missing mocks in tests * Add showLocation to GrafanaRuleListItem, improve useAbilities, address PR feedback * Enhance GrafanaGroupLoader tests: Add permission checks and More button functionality - Introduced user permission grants for alerting actions in tests. - Added tests for rendering the More button with action menu options. - Verified that each rule has its own action buttons and handles permissions correctly. - Ensured the edit button is not rendered when user lacks edit permissions. - Confirmed the correct menu actions are displayed when the More button is clicked. * Update translations --------- Co-authored-by: Sonia Aguilar <[email protected]> Co-authored-by: Sonia Augilar <[email protected]>
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.
💡 Codex Review
codex-grafana/public/app/features/alerting/unified/components/rule-viewer/AlertRuleMenu.tsx
Lines 123 to 175 in 3e6d620
| {canPause && rulerRuleType.grafana.rule(rulerRule) && groupIdentifier.groupOrigin === 'grafana' && ( | |
| <MenuItemPauseRule rule={rulerRule} groupIdentifier={groupIdentifier} onPauseChange={onPauseChange} /> | |
| )} | |
| {canSilence && ( | |
| <Menu.Item | |
| label={t('alerting.alert-menu.silence-notifications', 'Silence notifications')} | |
| icon="bell-slash" | |
| onClick={handleSilence} | |
| /> | |
| )} | |
| {/* TODO Migrate Declare Incident to plugin links extensions */} | |
| {shouldShowDeclareIncidentButton && <DeclareIncidentMenuItem title={promRule.name} url={''} />} | |
| {canDuplicate && ( | |
| <Menu.Item | |
| label={t('alerting.alert-menu.duplicate', 'Duplicate')} | |
| icon="copy" | |
| onClick={() => handleDuplicateRule(identifier)} | |
| /> | |
| )} | |
| {showDivider && <Menu.Divider />} | |
| {shareUrl && ( | |
| <Menu.Item | |
| label={t('alerting.alert-menu.copy-link', 'Copy link')} | |
| icon="share-alt" | |
| onClick={() => copyToClipboard(shareUrl)} | |
| /> | |
| )} | |
| {canExport && ( | |
| <Menu.Item | |
| label={t('alerting.alert-menu.export', 'Export')} | |
| icon="download-alt" | |
| childItems={[<ExportMenuItem key="export-with-modifications" identifier={identifier} />]} | |
| /> | |
| )} | |
| {extensionsAvailable && ( | |
| <> | |
| <Menu.Divider /> | |
| {ruleExtensionLinks.map((extension) => ( | |
| <Menu.Item key={extension.id} label={extension.title} icon={extension.icon} onClick={extension.onClick} /> | |
| ))} | |
| </> | |
| )} | |
| {canDelete && rulerRule && ( | |
| <> | |
| <Menu.Divider /> | |
| <Menu.Item | |
| label={t('alerting.common.delete', 'Delete')} | |
| icon="trash-alt" | |
| destructive | |
| onClick={() => handleDelete(rulerRule, groupIdentifier)} | |
| /> | |
| </> | |
| )} |
The new Grafana list only passes promRule into RuleActionsButtons (GrafanaRuleListItem.tsx now calls <RuleActionsButtons promRule={rule} … />), so AlertRuleMenu no longer receives a rulerRule. However the menu still requires rulerRule to render several actions (canPause && rulerRuleType.grafana.rule(rulerRule)… and canDelete && rulerRule at lines 123‑175). As a result the action menu on the list never exposes Pause/Restore or Delete, even when useGrafanaPromRuleAbilities reports the user is allowed, leaving users unable to manage Grafana-managed rules from the list at all. The earlier loader fetched both Prometheus and Ruler data so these actions were available. Please either stop gating these menu items on rulerRule (use the prom identifier instead) or provide the Ruler rule again.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /> | ||
| ); | ||
| })} |
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.
Guard GrafanaGroupLoader against empty group responses
When useGetGrafanaGroupsQuery returns successfully but the groups array is empty (e.g., the user opens a folder whose group was deleted or Prometheus momentarily returns no data), the new loader executes promResponse.data.groups.at(0)?.rules.map(...) without guarding for undefined. Because the optional chain only applies to .rules, the subsequent .map is still invoked on undefined, which throws Cannot read properties of undefined (reading 'map') and breaks the entire rule list. The previous implementation defaulted to an empty array, so this crash is a regression. Please default to an empty list (?.rules ?? []) before mapping.
Useful? React with 👍 / 👎.
|
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 2 weeks if no further activity occurs. Please feel free to give a status update or ping for review. Thank you for your contributions! |
Test 5
Replicated from ai-code-review-evaluation/grafana-cursor#5