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 <sonia.aguilar@grafana.com> * 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 <soniaaguilarpeiron@gmail.com> Co-authored-by: Sonia Augilar <sonia.aguilar@grafana.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThe pull request refactors alert rule permission checks to support dual-source validation: both Grafana Prometheus rules and Ruler rules can now be evaluated for capabilities independently, with permissions combined via logical OR. Related components are simplified by removing reconciliation logic and consolidating rule rendering paths. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @public/app/features/alerting/unified/hooks/useAbilities.ts:
- Around line 287-294: Remove the stray inline comment "// duplicate" in
useAllGrafanaPromRuleAbilities; locate the call to
useIsGrafanaPromRuleEditable(rule) (the const { isEditable, isRemovable, loading
} = useIsGrafanaPromRuleEditable(rule); line) and delete the trailing "//
duplicate" comment so the code and comment history are clean.
In @public/app/features/alerting/unified/rule-list/FilterView.tsx:
- Around line 157-162: The GrafanaRuleListItem rendered inside the .map() is
missing a React key; update the JSX where GrafanaRuleListItem is returned (the
component using props rule, groupIdentifier, namespaceName, showLocation) to
include key={key} (using the destructured key from ruleWithOrigin) to ensure a
stable unique key for list reconciliation.
🧹 Nitpick comments (4)
public/app/features/alerting/unified/rule-list/GrafanaGroupLoader.test.tsx (2)
85-102: Consider verifying the More button'saria-haspopupattribute.The test checks
aria-labeland text content, but for accessibility compliance, it would be valuable to also verify that the More button hasaria-haspopup="true"or similar attribute indicating it opens a menu.💡 Optional enhancement for accessibility testing
// Verify More button accessibility expect(moreButton).toHaveAttribute('aria-label', 'More'); expect(moreButton).toHaveTextContent('More'); + // Verify dropdown menu indicator for screen readers + expect(moreButton).toHaveAttribute('aria-haspopup', 'true');
171-208: Test assumes exactly 4 menu items, which is fragile.Line 207 asserts
menuItems.lengthequals 4. If a new menu action is added in the future, this test will fail even though the tested functionality works correctly.♻️ Suggested improvement to reduce test fragility
- // Verify that the menu contains all 4 expected menu items - const menuItems = byRole('menuitem').getAll(); - expect(menuItems.length).toBe(4); + // Verify that all expected menu items are present (minimum expected items) + const menuItems = byRole('menuitem').getAll(); + expect(menuItems.length).toBeGreaterThanOrEqual(4);Alternatively, remove this assertion entirely since the individual menu item checks above already verify the expected items are present.
public/app/features/alerting/unified/rule-list/GrafanaRuleListItem.tsx (1)
50-64: Redundant type check at Line 51.The
prometheusRuleType.grafana.alertingRule(rule)guard at line 50 already narrows the type to an alerting rule. The additional checkrule.type === PromRuleType.Alertingat line 51 is redundant.♻️ Simplify redundant type narrowing
if (prometheusRuleType.grafana.alertingRule(rule)) { - const promAlertingRule = rule && rule.type === PromRuleType.Alerting ? rule : undefined; - const instancesCount = totalFromStats(promAlertingRule?.totals ?? {}); + const instancesCount = totalFromStats(rule.totals ?? {}); return ( <AlertRuleListItem {...commonProps} summary={rule.annotations?.summary} - state={promAlertingRule?.state} + state={rule.state} instancesCount={instancesCount} operation={operation} showLocation={showLocation} /> ); }public/app/features/alerting/unified/components/rules/RulesTable.test.tsx (1)
56-73: Good default mock setup, but consider DRY improvement.The default mocks are well-structured. However, both
useRulerRuleAbilitiesanduseGrafanaPromRuleAbilitieshave nearly identical implementations. Consider extracting a helper.♻️ Optional: Extract mock factory for plural hooks
// Helper to create default plural hook mock const createPluralHookMock = () => (_rule: unknown, ...rest: unknown[]) => { const actions = rest[rest.length - 1] as unknown[]; return actions.map(() => [false, false]); }; // In beforeEach: mocks.useRulerRuleAbilities.mockImplementation(createPluralHookMock()); mocks.useGrafanaPromRuleAbilities.mockImplementation(createPluralHookMock());
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
public/app/features/alerting/unified/components/rule-viewer/AlertRuleMenu.tsxpublic/app/features/alerting/unified/components/rules/RuleDetails.test.tsxpublic/app/features/alerting/unified/components/rules/RulesTable.test.tsxpublic/app/features/alerting/unified/hooks/useAbilities.tspublic/app/features/alerting/unified/rule-list/FilterView.tsxpublic/app/features/alerting/unified/rule-list/GrafanaGroupLoader.test.tsxpublic/app/features/alerting/unified/rule-list/GrafanaGroupLoader.tsxpublic/app/features/alerting/unified/rule-list/GrafanaRuleListItem.tsxpublic/app/features/alerting/unified/rule-list/GrafanaRuleLoader.tsxpublic/app/features/alerting/unified/rule-list/components/RuleActionsButtons.V2.tsxpublic/app/features/alerting/unified/rule-list/hooks/prometheusGroupsGenerator.tspublic/app/features/alerting/unified/utils/rules.tspublic/app/types/unified-alerting-dto.tspublic/locales/en-US/grafana.json
💤 Files with no reviewable changes (3)
- public/locales/en-US/grafana.json
- public/app/features/alerting/unified/rule-list/GrafanaRuleLoader.tsx
- public/app/features/alerting/unified/rule-list/hooks/prometheusGroupsGenerator.ts
🧰 Additional context used
🧬 Code graph analysis (6)
public/app/features/alerting/unified/utils/rules.ts (1)
public/app/types/unified-alerting-dto.ts (1)
PromRuleDTO(162-162)
public/app/features/alerting/unified/rule-list/FilterView.tsx (1)
public/app/features/alerting/unified/rule-list/GrafanaRuleListItem.tsx (1)
GrafanaRuleListItem(26-71)
public/app/features/alerting/unified/components/rules/RuleDetails.test.tsx (2)
public/app/features/alerting/unified/mocks/server/configure.ts (1)
mimirDataSource(178-201)public/app/features/alerting/unified/mocks.ts (1)
getCloudRule(711-728)
public/app/features/alerting/unified/rule-list/GrafanaRuleListItem.tsx (5)
public/app/types/unified-alerting-dto.ts (1)
GrafanaPromRuleDTO(181-181)public/app/features/alerting/unified/utils/datasource.ts (1)
GrafanaRulesSource(36-40)public/app/features/alerting/unified/rule-list/components/RuleActionsButtons.V2.tsx (1)
RuleActionsButtons(41-116)public/app/features/alerting/unified/utils/rules.ts (1)
prometheusRuleType(128-137)public/app/features/alerting/unified/utils/ruleStats.ts (1)
totalFromStats(5-12)
public/app/features/alerting/unified/components/rules/RulesTable.test.tsx (2)
public/app/features/alerting/unified/hooks/useAbilities.ts (5)
useRulerRuleAbility(195-205)useAlertRuleAbility(179-185)useGrafanaPromRuleAbility(386-392)useRulerRuleAbilities(207-217)useGrafanaPromRuleAbilities(397-406)public/app/features/alerting/unified/mocks.ts (1)
getGrafanaRule(699-709)
public/app/features/alerting/unified/rule-list/GrafanaGroupLoader.test.tsx (4)
public/app/features/alerting/unified/mocks.ts (1)
grantUserPermissions(637-641)public/app/features/alerting/unified/mocks/server/configure.ts (1)
setFolderAccessControl(56-58)public/app/features/alerting/unified/mocks/grafanaRulerApi.ts (2)
grafanaRulerGroup(65-69)grafanaRulerNamespace(26-26)public/app/features/alerting/unified/rule-list/GrafanaGroupLoader.tsx (1)
GrafanaGroupLoader(29-82)
🪛 Biome (2.1.2)
public/app/features/alerting/unified/rule-list/FilterView.tsx
[error] 157-163: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sqlite (5/8)
🔇 Additional comments (22)
public/app/features/alerting/unified/utils/rules.ts (1)
171-173: LGTM!The new
isProvisionedPromRulefunction correctly mirrors the existingisProvisionedRulepattern for ruler rules. The type guardprometheusRuleType.grafana.rule(promRule)properly narrows the type toGrafanaPromRuleDTOwhich includes theprovenanceproperty fromGrafanaPromRuleDTOBase.public/app/types/unified-alerting-dto.ts (1)
130-136: LGTM!The addition of
provenance?: stringtoGrafanaPromRuleDTOBaseis consistent with the existingprovenancefield onGrafanaRuleDefinition(line 290) and enables provenance checking for Prometheus-sourced Grafana rules.public/app/features/alerting/unified/components/rules/RuleDetails.test.tsx (1)
10-10: LGTM!The test setup correctly uses
mimirDataSource()to configure a proper Mimir data source for cloud rule tests, aligning with the dual-source permission handling introduced in this PR.Also applies to: 36-37, 87-87
public/app/features/alerting/unified/components/rule-viewer/AlertRuleMenu.tsx (1)
56-100: LGTM!The dual-source permission check implementation is well-structured. The pattern of checking both ruler-based and Grafana-based abilities with a logical OR ensures actions are permitted if either source grants permission. The use of
skipTokenwhenpromRuleis not a Grafana rule correctly prevents unnecessary ability checks.public/app/features/alerting/unified/rule-list/components/RuleActionsButtons.V2.tsx (3)
25-37: LGTM!The use of
RequireAtLeastOnefromtype-festprovides good type safety, ensuring consumers must provide at least one ofruleorpromRule. The component signature clearly expresses this constraint.Also applies to: 41-41
66-70: Good defensive handling for missing identifier.Returning
nullwhen an editable identifier cannot be constructed is a safe approach that prevents the component from crashing while still allowing parent components to render other content.
118-151: LGTM!The helper functions
getIsProvisionedandgetEditableIdentifiercleanly separate concerns and handle both rule sources appropriately. The warning log at line 146 aids debugging when identifier construction fails.public/app/features/alerting/unified/rule-list/GrafanaGroupLoader.tsx (2)
43-51: LGTM!The simplified loading state that only depends on the Prometheus response aligns with the removal of ruler-based data fusion. The skeleton count from
expectedRulesCountprovides a good loading UX.
66-81: LGTM!The direct rendering from
promResponse.data.groups.at(0)?.rulesis clean and handles edge cases safely via optional chaining. The removal of complex ruler-prometheus reconciliation logic significantly simplifies this component.public/app/features/alerting/unified/rule-list/GrafanaGroupLoader.test.tsx (3)
36-43: LGTM! Well-structured UI selectors for menu items.The grouped
menuItemsobject provides good organization for accessing action menu elements in tests. The regex patterns (/silence/i) provide flexibility for potential i18n variations.
47-63: LGTM! Good permission scaffolding in beforeEach.The test setup properly grants both user-level permissions via
grantUserPermissionsand folder-level access control viasetFolderAccessControl. The comments explaining what each permission enables (e.g., "For duplicate action", "For export action") improve maintainability.
148-169: The test correctly denies edit permissions. For Grafana-managed alert rules, the edit button visibility depends only on folder-level permissions checked viactx.hasPermissionInMetadata(), not user-level permissions. ThegrantUserPermissionscall inbeforeEachmockscontextSrv.hasPermission()(user-level), which is unused for this rule type. Therefore, overridingsetFolderAccessControlto denyAlertingRuleUpdateis sufficient and the test accurately reflects the no-edit-permission scenario.public/app/features/alerting/unified/rule-list/GrafanaRuleListItem.tsx (2)
26-48: LGTM! Well-structured component with clear prop extraction.The component correctly extracts common properties and assembles them into a shared props object. The use of
createRelativeUrlensures consistent URL generation, andGrafanaRulesSourcecorrectly identifies the source.
66-70: LGTM! Good fallback handling for unknown rule types.The component properly handles recording rules and unknown rule types with appropriate fallback components, ensuring graceful degradation.
public/app/features/alerting/unified/components/rules/RulesTable.test.tsx (3)
167-178: LGTM! Comprehensive mock setup for creating/deleting state tests.The beforeEach properly configures all required hooks with permissive mocks, ensuring the creating/deleting state UI behavior can be tested in isolation from permission concerns.
213-218: LGTM! Cloud rule tests properly useuseRulerRuleAbilities.The comments correctly document that cloud rules use
useRulerRuleAbilitieswhileuseGrafanaPromRuleAbilitiesreceivesskipToken. This aligns with the dual-source validation architecture described in the AI summary.Also applies to: 232-237, 252-257, 271-276
79-93: Remove the unnecessaryuseGrafanaPromRuleAbilitymock from the edit test.The singular
useGrafanaPromRuleAbilityhook is not used by eitherRuleActionsButtonsorAlertRuleMenucomponents. The components use the pluraluseGrafanaPromRuleAbilitieshook for permission checks. The delete test correctly mocks only the required plural hook; the edit test should be updated for consistency by removing the unused singular hook mock at lines 85-87.Likely an incorrect or invalid review comment.
public/app/features/alerting/unified/hooks/useAbilities.ts (5)
219-229: LGTM! Clean deprecation with proper delegation.The deprecated
useAllAlertRuleAbilitiescorrectly delegates to the new implementation while maintaining backward compatibility. The memoized group identifier conversion prevents unnecessary re-renders.
349-378: LGTM! Well-implemented folder permission check.The
useIsGrafanaPromRuleEditablehook correctly:
- Returns early with safe defaults when rule or folderUID is missing
- Handles loading state appropriately
- Checks folder-specific permissions using
hasPermissionInMetadata
380-406: LGTM! Clean skipToken pattern implementation.The
skipTokensymbol pattern is a well-established practice (similar to RTK Query'sskipToken) for conditionally skipping hook execution. The implementation correctly handles the skip case by passingundefinedto the underlying abilities hook.
231-281: Verify memoization dependencies are complete.Line 278 lists dependencies for the useMemo, but
rulesSourceNameis derived fromgroupIdentifier(line 235). IfgroupIdentifierchanges butrulesSourceNamestays the same, the memo won't update. This should be fine given the current logic, but it's worth noting for future maintainers.
295-337: Code correctly handles loading state transitions.The
loadingdependency is properly included in the dependency array (line 337), andMaybeSupportedcorrectly evaluates toNotSupportedduring loading. When the loading state changes,useMemorecalculates the abilities, and consuming components automatically re-render with the updated ability values. The loading-to-loaded transition is handled gracefully by React's memoization and equality checks.
| export function useAllGrafanaPromRuleAbilities(rule: GrafanaPromRuleDTO | undefined): Abilities<AlertRuleAction> { | ||
| // For GrafanaPromRuleDTO, we use useIsGrafanaPromRuleEditable instead | ||
| const { isEditable, isRemovable, loading } = useIsGrafanaPromRuleEditable(rule); // duplicate | ||
| const [_, exportAllowed] = useAlertingAbility(AlertingAction.ExportGrafanaManagedRules); | ||
| const canSilence = useCanSilence(rule); | ||
|
|
||
| const silenceSupported = useGrafanaRulesSilenceSupport(); | ||
| const canSilenceInFolder = useCanSilenceInFolder(rule?.folderUid); | ||
|
|
There was a problem hiding this comment.
Duplicate comment on Line 289.
There's a stray "// duplicate" comment at line 289 that appears to be leftover from development.
🧹 Remove stray comment
export function useAllGrafanaPromRuleAbilities(rule: GrafanaPromRuleDTO | undefined): Abilities<AlertRuleAction> {
// For GrafanaPromRuleDTO, we use useIsGrafanaPromRuleEditable instead
- const { isEditable, isRemovable, loading } = useIsGrafanaPromRuleEditable(rule); // duplicate
+ const { isEditable, isRemovable, loading } = useIsGrafanaPromRuleEditable(rule);
const [_, exportAllowed] = useAlertingAbility(AlertingAction.ExportGrafanaManagedRules);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function useAllGrafanaPromRuleAbilities(rule: GrafanaPromRuleDTO | undefined): Abilities<AlertRuleAction> { | |
| // For GrafanaPromRuleDTO, we use useIsGrafanaPromRuleEditable instead | |
| const { isEditable, isRemovable, loading } = useIsGrafanaPromRuleEditable(rule); // duplicate | |
| const [_, exportAllowed] = useAlertingAbility(AlertingAction.ExportGrafanaManagedRules); | |
| const canSilence = useCanSilence(rule); | |
| const silenceSupported = useGrafanaRulesSilenceSupport(); | |
| const canSilenceInFolder = useCanSilenceInFolder(rule?.folderUid); | |
| export function useAllGrafanaPromRuleAbilities(rule: GrafanaPromRuleDTO | undefined): Abilities<AlertRuleAction> { | |
| // For GrafanaPromRuleDTO, we use useIsGrafanaPromRuleEditable instead | |
| const { isEditable, isRemovable, loading } = useIsGrafanaPromRuleEditable(rule); | |
| const [_, exportAllowed] = useAlertingAbility(AlertingAction.ExportGrafanaManagedRules); | |
| const silenceSupported = useGrafanaRulesSilenceSupport(); | |
| const canSilenceInFolder = useCanSilenceInFolder(rule?.folderUid); | |
🤖 Prompt for AI Agents
In @public/app/features/alerting/unified/hooks/useAbilities.ts around lines 287
- 294, Remove the stray inline comment "// duplicate" in
useAllGrafanaPromRuleAbilities; locate the call to
useIsGrafanaPromRuleEditable(rule) (the const { isEditable, isRemovable, loading
} = useIsGrafanaPromRuleEditable(rule); line) and delete the trailing "//
duplicate" comment so the code and comment history are clean.
| <GrafanaRuleListItem | ||
| rule={rule} | ||
| groupIdentifier={groupIdentifier} | ||
| namespaceName={ruleWithOrigin.namespaceName} | ||
| showLocation={true} | ||
| /> |
There was a problem hiding this comment.
Missing key prop on list item.
The GrafanaRuleListItem is rendered inside a .map() but is missing the key prop. The key variable is already available from the destructured ruleWithOrigin. Without a key, React cannot efficiently reconcile list updates.
🔧 Proposed fix
case 'grafana':
return (
<GrafanaRuleListItem
+ key={key}
rule={rule}
groupIdentifier={groupIdentifier}
namespaceName={ruleWithOrigin.namespaceName}
showLocation={true}
/>
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <GrafanaRuleListItem | |
| rule={rule} | |
| groupIdentifier={groupIdentifier} | |
| namespaceName={ruleWithOrigin.namespaceName} | |
| showLocation={true} | |
| /> | |
| <GrafanaRuleListItem | |
| key={key} | |
| rule={rule} | |
| groupIdentifier={groupIdentifier} | |
| namespaceName={ruleWithOrigin.namespaceName} | |
| showLocation={true} | |
| /> |
🧰 Tools
🪛 Biome (2.1.2)
[error] 157-163: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
🤖 Prompt for AI Agents
In @public/app/features/alerting/unified/rule-list/FilterView.tsx around lines
157 - 162, The GrafanaRuleListItem rendered inside the .map() is missing a React
key; update the JSX where GrafanaRuleListItem is returned (the component using
props rule, groupIdentifier, namespaceName, showLocation) to include key={key}
(using the destructured key from ruleWithOrigin) to ensure a stable unique key
for list reconciliation.
|
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! |
|
This pull request has been automatically closed because it has not had any further activity in the last 2 weeks. Thank you for your contributions! |
2 participants
Test 5nn
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.
nn---n*Replicated from [ai-code-review-evaluation/grafana-coderabbit#5](https://github.com/ai-code-review-evaluation/grafana-coderabbit/pull/5)*