Notification Rule Processing Engine#5
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>
WalkthroughThis update refactors and simplifies the handling of alerting rule permissions, rule list rendering, and related UI components. It introduces new hooks for permission checks, consolidates rule list item rendering into a single component, removes cross-source rule matching, and updates tests and type definitions to align with these changes. Several obsolete files and error messages are removed. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Rule List UI
participant Hooks as Permission Hooks
participant API as Prometheus API
UI->>API: Fetch Prometheus rule groups
UI->>Hooks: Check abilities for each rule (plural hooks)
Hooks->>API: (if needed) Fetch permissions for rules
UI->>UI: Render GrafanaRuleListItem for each rule
UI->>Hooks: On action (edit, silence, etc), check merged permissions
Note over UI: Actions enabled if allowed by either Ruler or Grafana Prom hooks
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@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: 1
🧹 Nitpick comments (2)
public/app/features/alerting/unified/rule-list/GrafanaGroupLoader.tsx (1)
67-80: Consider handling empty rule groups explicitly.While the optional chaining handles undefined groups safely, consider explicitly checking for empty rule arrays to provide better user feedback.
+ const rules = promResponse.data.groups.at(0)?.rules || []; + + if (rules.length === 0) { + return ( + <Alert + title={t('alerting.group-loader.no-rules', 'No rules found in group {{ groupName }}', { + groupName: groupIdentifier.groupName, + })} + severity="info" + /> + ); + } + return ( <> - {promResponse.data.groups.at(0)?.rules.map((promRule) => { + {rules.map((promRule) => { return ( <GrafanaRuleListItempublic/app/features/alerting/unified/hooks/useAbilities.ts (1)
243-245: TODO: Federated rules support is missing.The federated rules support is currently hardcoded to
false. This might impact users who rely on federated rule groups functionality.Would you like me to help implement the federated rules support or create an issue to track this TODO?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
public/app/features/alerting/unified/components/rule-viewer/AlertRuleMenu.tsx(3 hunks)public/app/features/alerting/unified/components/rules/RuleDetails.test.tsx(3 hunks)public/app/features/alerting/unified/components/rules/RulesTable.test.tsx(10 hunks)public/app/features/alerting/unified/hooks/useAbilities.ts(4 hunks)public/app/features/alerting/unified/rule-list/FilterView.tsx(2 hunks)public/app/features/alerting/unified/rule-list/GrafanaGroupLoader.test.tsx(5 hunks)public/app/features/alerting/unified/rule-list/GrafanaGroupLoader.tsx(4 hunks)public/app/features/alerting/unified/rule-list/GrafanaRuleListItem.tsx(1 hunks)public/app/features/alerting/unified/rule-list/GrafanaRuleLoader.tsx(0 hunks)public/app/features/alerting/unified/rule-list/components/RuleActionsButtons.V2.tsx(3 hunks)public/app/features/alerting/unified/rule-list/hooks/prometheusGroupsGenerator.ts(0 hunks)public/app/features/alerting/unified/utils/rules.ts(1 hunks)public/app/types/unified-alerting-dto.ts(1 hunks)public/locales/en-US/grafana.json(0 hunks)
💤 Files with no reviewable changes (3)
- public/app/features/alerting/unified/rule-list/hooks/prometheusGroupsGenerator.ts
- public/locales/en-US/grafana.json
- public/app/features/alerting/unified/rule-list/GrafanaRuleLoader.tsx
🧰 Additional context used
🧬 Code Graph Analysis (6)
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/utils/rules.ts (1)
public/app/types/unified-alerting-dto.ts (1)
PromRuleDTO(162-162)
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/components/rule-viewer/AlertRuleMenu.tsx (2)
public/app/features/alerting/unified/hooks/useAbilities.ts (3)
useRulerRuleAbilities(207-217)useGrafanaPromRuleAbilities(397-406)skipToken(380-380)public/app/features/alerting/unified/utils/rules.ts (1)
prometheusRuleType(128-137)
public/app/features/alerting/unified/rule-list/components/RuleActionsButtons.V2.tsx (4)
public/app/types/unified-alerting-dto.ts (1)
RulerRuleDTO(319-319)public/app/features/alerting/unified/hooks/useAbilities.ts (3)
useRulerRuleAbility(195-205)useGrafanaPromRuleAbility(386-392)skipToken(380-380)public/app/features/alerting/unified/utils/rules.ts (3)
prometheusRuleType(128-137)isProvisionedRule(167-169)isProvisionedPromRule(171-173)public/app/features/alerting/unified/Analytics.ts (1)
logWarning(35-35)
public/app/features/alerting/unified/hooks/useAbilities.ts (3)
public/app/types/unified-alerting-dto.ts (2)
RulerRuleDTO(319-319)GrafanaPromRuleDTO(181-181)public/app/features/alerting/unified/utils/rules.ts (5)
isProvisionedRule(167-169)rulerRuleType(111-126)isPluginProvidedRule(272-274)isProvisionedPromRule(171-173)prometheusRuleType(128-137)public/app/features/alerting/unified/utils/misc.ts (1)
isAdmin(272-274)
🪛 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)
🔇 Additional comments (21)
public/app/features/alerting/unified/utils/rules.ts (1)
171-173: LGTM! Well-structured utility function following established patterns.The
isProvisionedPromRulefunction correctly mirrors the pattern of the existingisProvisionedRulefunction, providing a consistent way to check provisioning status for Grafana Prometheus rules. The implementation properly combines the type check with the provenance property validation.public/app/types/unified-alerting-dto.ts (1)
135-135: LGTM! Necessary type extension for provenance support.The addition of the optional
provenanceproperty toGrafanaPromRuleDTOBasecorrectly provides the data field needed by the newisProvisionedPromRuleutility function. The optional typing is appropriate since not all rules will have provenance information.public/app/features/alerting/unified/components/rules/RuleDetails.test.tsx (1)
10-10: LGTM! Improved test setup with explicit data source configuration.The changes enhance the test by explicitly using the
mimirDataSourcefor cloud rule tests instead of relying on the default mock data source. This makes the test intent clearer and aligns with the broader refactoring that handles multiple data source types.Also applies to: 36-36, 87-87
public/app/features/alerting/unified/rule-list/FilterView.tsx (1)
15-15: LGTM! Component import updated correctly.The import change from
GrafanaRuleLoadertoGrafanaRuleListItemaligns with the refactoring to simplify rule rendering by removing the separate loading logic.public/app/features/alerting/unified/rule-list/GrafanaRuleListItem.tsx (1)
1-71: LGTM! Well-structured component with proper type handling.The
GrafanaRuleListItemcomponent is implemented correctly with:
- Clear props interface with appropriate TypeScript types
- Proper extraction of common properties from the rule
- Correct conditional rendering based on rule types using established type guards
- Consistent integration of action buttons and navigation
- Appropriate handling of optional props with sensible defaults
The component effectively consolidates rule rendering logic and eliminates the need for separate loading components.
public/app/features/alerting/unified/components/rule-viewer/AlertRuleMenu.tsx (2)
73-79: Good use of conditional hook invocation with skipToken.The implementation correctly uses
skipTokento conditionally skip the Grafana Prometheus rule abilities check when the rule is not a Grafana Prometheus rule. This prevents unnecessary API calls and follows React hook best practices.
89-92: Fix typo in duplicate ability assignment.There's a typo on line 90 where
grafanaDuplicateAbilityis incorrectly used for both the extraction and assignment. It should extract fromgrafanaDuplicateAbility.Apply this fix:
const [duplicateSupported, duplicateAllowed] = rulerDuplicateAbility; - const [grafanaDuplicateSupported, grafanaDuplicateAllowed] = grafanaDuplicateAbility; + const [grafanaDuplicateSupported, grafanaDuplicateAllowed] = grafanaDuplicateAbility;Likely an incorrect or invalid review comment.
public/app/features/alerting/unified/rule-list/GrafanaGroupLoader.test.tsx (2)
47-64: Well-structured test setup with comprehensive permissions.The
beforeEachhook properly sets up both user permissions and folder access controls, ensuring a consistent test environment. This approach makes the tests more maintainable and reduces duplication.
171-208: Comprehensive menu interaction test with clear documentation.The test thoroughly verifies menu item rendering based on permissions, with helpful comments explaining which permission enables each action. The verification of both individual items and total count ensures completeness.
public/app/features/alerting/unified/rule-list/GrafanaGroupLoader.tsx (1)
34-41: Good polling configuration for real-time updates.The component correctly uses the constant
RULE_LIST_POLL_INTERVAL_MSfor polling, ensuring rules are refreshed regularly. The query parameters are well-structured withlimitAlerts: 0to fetch all alerts.public/app/features/alerting/unified/components/rules/RulesTable.test.tsx (2)
61-74: Well-structured mock setup with safe defaults.The mock setup correctly initializes all hooks with restrictive defaults (returning
[false, false]), which is a good security practice. The plural hooks properly handle dynamic action arrays.
213-218: Clear separation of mocking concerns for Cloud rules.The comments clearly explain that Cloud rules only require
useRulerRuleAbilitiesmocking sinceuseGrafanaPromRuleAbilitiesreceivesskipToken. This documentation helps maintain the tests and understand the permission model.Also applies to: 232-237, 252-257, 271-276
public/app/features/alerting/unified/rule-list/components/RuleActionsButtons.V2.tsx (2)
25-29: Good use of RequireAtLeastOne type for mutual exclusivity.The type definition ensures that at least one of
ruleorpromRuleis provided, preventing the component from being called without any rule data. This is a robust type-safe approach.
130-151: Robust identifier construction with proper error handling.The function handles both rule types appropriately and provides helpful logging when unable to construct an identifier. The use of
satisfiesfor type assertion and returningundefinedinstead of throwing maintains component stability.public/app/features/alerting/unified/hooks/useAbilities.ts (7)
17-17: LGTM! Import statement properly updated.The addition of
GrafanaPromRuleDTOto the imports aligns with the new hooks being introduced for handling Grafana Prometheus rules.
24-30: LGTM! Utility imports support the new functionality.The imported utility functions (
isPluginProvidedRule,isProvisionedPromRule,isProvisionedRule,prometheusRuleType,rulerRuleType) are properly utilized throughout the file for rule type checking and permission logic.
219-229: Good deprecation strategy with performance optimization.The function is properly deprecated with clear guidance to use
useAllRulerRuleAbilities. The memoization ofgroupIdentifierV2addresses the performance concerns mentioned in the comment about multiple calls for ~80 rules.
287-337: Well-structured implementation for GrafanaPromRuleDTO abilities.The function properly handles Grafana Prometheus rules with appropriate:
- Permission checks scoped to 'grafana' source
- Silence support specific to Grafana rules with folder-level permissions
- Clear documentation about federated rules limitation
- Consistent ability mapping with the RulerRuleDTO counterpart
349-378: Clean implementation of folder-based permission checks.The hook correctly:
- Handles loading states and missing data gracefully
- Checks folder-level permissions using the appropriate metadata context
- Returns a consistent interface matching the existing pattern
380-406: Excellent optimization with skip token pattern.The skip token implementation provides a clean way to conditionally skip ability checks, avoiding unnecessary computations when the rule is not yet available. This pattern aligns with modern React optimization practices.
208-208: No breaking change detected: makingruleoptional is backwards compatibleI verified all call sites of
useRulerRuleAbilities,useRulerRuleAbility, anduseAllRulerRuleAbilities:
- public/app/features/alerting/unified/components/rule-viewer/AlertRuleMenu.tsx
- public/app/features/alerting/unified/rule-list/components/RuleActionsButtons.V2.tsx
- public/app/features/alerting/unified/components/rule-viewer/tabs/AlertVersionHistory.tsx
In every case, a concrete
RulerRuleDTOinstance is passed, so relaxing the parameter toRulerRuleDTO | undefineddoes not break existing usage and merely enables future undefined-safe calls.
| <GrafanaRuleListItem | ||
| rule={rule} | ||
| groupIdentifier={groupIdentifier} | ||
| namespaceName={ruleWithOrigin.namespaceName} | ||
| showLocation={true} | ||
| /> |
There was a problem hiding this comment.
Fix missing key prop for React list rendering.
The component replacement logic is correct, but the rendered GrafanaRuleListItem is missing the required key prop for React list items. This can cause rendering issues when the list order changes.
Apply this diff to add the missing key:
<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 to 162, the GrafanaRuleListItem component is missing the required key prop
for React list rendering. Add a unique key prop to the GrafanaRuleListItem
component, ideally using a unique identifier from the rule object or another
stable unique value to ensure proper rendering and reconciliation by React.
|
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 5
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Chores