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]>
|
@coderabbitai review |
|
@coderabbitai review |
1 similar comment
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThe changes add Grafana Prometheus rule support throughout the alerting system by introducing new RBAC hooks for granular permissions, simplifying data-fetching components to rely solely on Prometheus data, consolidating GrafanaRuleLoader logic into a new GrafanaRuleListItem component, enhancing RuleActionsButtons to support both rule types, and updating corresponding tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Consumer as AlertRuleMenu/<br/>RuleActionsButtons
participant RulerHook as useRulerRuleAbilities
participant GrafanaHook as useGrafanaPromRuleAbilities
participant AbilityCalc as Ability Computation<br/>(both sources)
participant UI as UI Rendering
Consumer->>RulerHook: Request abilities for ruler rule
RulerHook->>AbilityCalc: Compute ruler-based permissions
Consumer->>GrafanaHook: Request abilities for grafana rule
GrafanaHook->>AbilityCalc: Compute grafana-specific permissions<br/>(folder perms, silence support)
AbilityCalc-->>Consumer: canPause, canDelete, canDuplicate...<br/>(OR of both sources)
Consumer->>UI: Render enabled/disabled menu items
sequenceDiagram
participant OldFlow as Old: GrafanaGroupLoader<br/>(Prometheus + Ruler)
participant NewFlow as New: GrafanaGroupLoader<br/>(Prometheus only)
participant Renderer as GrafanaRuleListItem<br/>(new component)
OldFlow->>OldFlow: useGetGrafanaGroupsQuery
OldFlow->>OldFlow: useGetGrafanaRulerGroupQuery
OldFlow->>OldFlow: matchRules (cross-source)
OldFlow->>OldFlow: Render matched + ruler-only items
NewFlow->>NewFlow: useGetGrafanaGroupsQuery
NewFlow->>Renderer: Prometheus rules only
Renderer->>Renderer: Determine rule type<br/>(alerting/recording/unknown)
Renderer->>UI: Render appropriate<br/>list item component
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
public/app/features/alerting/unified/rule-list/GrafanaGroupLoader.tsx (1)
24-28: Guard against emptygroupsarray when mapping rulesThe code at line 68 has a critical bug:
promResponse.data.groups.at(0)?.rules.map(...)will throw a TypeError whengroupsis an empty array. Optional chaining (?.) only protects the access torules—it does not prevent.map()from being called onundefinedwhenat(0)returns undefined.Consider defensively defaulting to an empty array before mapping:
- return ( - <> - {promResponse.data.groups.at(0)?.rules.map((promRule) => { - return ( - <GrafanaRuleListItem - key={promRule.uid} - rule={promRule} - groupIdentifier={groupIdentifier} - namespaceName={namespaceName} - // we don't show the location again for rules, it's redundant because they are shown in a folder > group hierarchy - showLocation={false} - /> - ); - })} - </> - ); + const rules = promResponse.data.groups.at(0)?.rules ?? []; + + return ( + <> + {rules.map((promRule) => ( + <GrafanaRuleListItem + key={promRule.uid} + rule={promRule} + groupIdentifier={groupIdentifier} + namespaceName={namespaceName} + // we don't show the location again for rules, it's redundant because they are shown in a folder > group hierarchy + showLocation={false} + /> + ))} + </> + );Also consider updating the JSDoc block (lines 24–28) to remove the Ruler reference, as this loader queries Prometheus only.
🧹 Nitpick comments (9)
public/app/features/alerting/unified/components/rules/RuleDetails.test.tsx (1)
36-37: Consider creatingmimirDsinbeforeEachfor better test isolation.The current implementation works correctly, but creating the data source at module level means it's shared across all tests. While there's no visible mutation in the current tests, moving this to
beforeEachwould follow best practices for test isolation and prevent potential future issues.Example refactor:
-const { dataSource: mimirDs } = mimirDataSource(); - beforeAll(() => { jest.clearAllMocks(); }); +let mimirDs: ReturnType<typeof mimirDataSource>['dataSource']; + beforeEach(() => { + mimirDs = mimirDataSource().dataSource; mocks.usePluginLinksMock.mockReturnValue({public/app/features/alerting/unified/components/rule-viewer/AlertRuleMenu.tsx (1)
13-18: Dual abilities wiring looks sound; consider small refactor for maintainabilityThe new pattern of fetching ruler and Grafana-prom abilities and then OR-ing the
(supported && allowed)flags forcanPause/Delete/Duplicate/Silence/Exportis logically consistent and keeps existing menu rendering intact. TheskipTokenguard onuseGrafanaPromRuleAbilitiesis also a good way to avoid unnecessary work whenpromRuleisn’t a Grafana-prom rule.Given the repetition, you might consider extracting a tiny helper like
combineAbility([supported, allowed], [gSupported, gAllowed])to reduce duplication and keep the mapping between requested actions andcanXflags easier to maintain going forward, but that’s optional.Also applies to: 41-43, 56-100
public/app/features/alerting/unified/rule-list/GrafanaRuleListItem.tsx (1)
18-71: Grafana rule list item composition and actions wiring look correctThis component cleanly centralizes Grafana-prom rule rendering:
- Common metadata (name, source, namespace, href, health/error, labels, paused state) is shared via
commonProps.isProvisionedderiving fromprovenancematches the provisioning helpers elsewhere.- Alerting vs recording routing via
prometheusRuleType.grafana.*plus the explicittype === PromRuleType.Alertingcheck is defensive and clear.- Actions are correctly delegated to
RuleActionsButtonswithpromRuleandgroupIdentifier, which should keep RBAC behavior consistent with other list items.If you want to DRY things up slightly, you could consider using
isProvisionedPromRule(rule)instead ofBoolean(provenance), but that’s purely optional.public/app/features/alerting/unified/rule-list/GrafanaGroupLoader.test.tsx (1)
5-63: Expanded RBAC- and menu-focused tests for Grafana groups look solidThe updated tests do a good job of covering:
- Permission scaffolding (
grantUserPermissions+setFolderAccessControl) for the common happy-path.- Presence and accessibility of the More button per rule, including multi-rule groups and correct Edit URLs.
- Negative case where folder-level edit permissions are revoked while still keeping the More button visible.
- Menu behavior when clicking More, asserting the expected menu structure and primary actions (silence, copy link, duplicate, export).
You’ve also prepared a
menuItems.deletequery helper; if delete behavior is expected to surface in some scenarios, you might add a small test asserting its presence/absence under the right permissions, but that’s a nice-to-have, not a blocker.Also applies to: 85-208
public/app/features/alerting/unified/components/rules/RulesTable.test.tsx (1)
7-15: RBAC hook mocking setup is clear butjestresets are redundantThe consolidated
mocksobject and neutral defaults inbeforeEachmake the RBAC matrix easy to reason about, and correctly distinguish singular vs plural hooks.You are calling
jest.clearAllMocks(),jest.restoreAllMocks(), andjest.resetAllMocks()together, which partially overlap in behaviour and can be confusing. In most cases, a singlejest.resetAllMocks()(or a clear + restore pair) is sufficient.Also applies to: 22-32, 55-74
public/app/features/alerting/unified/rule-list/components/RuleActionsButtons.V2.tsx (2)
51-62: Combined ruler + Grafanaability checks and identifier guard look correct
getIsProvisioned(rule, promRule)unifies provisioned status for both Ruler and Grafana Prom rules.canEditRuleshort‑circuits correctly when either the Ruler or Grafana ability tuple is[true, true].getEditableIdentifierreturningundefinedand the component bailing out early avoids rendering partial UI when an identifier can’t be safely constructed, which is preferable to crashingAlertRuleMenu.One thing to double‑check is that
useRulerRuleAbilitybehaves sensibly whenruleisundefined(as will be the case for Grafana‑only Prom rules), and that the extra hook call doesn’t introduce unnecessary work in hot paths. If needed, you could gate it with the sameprometheusRuleType/group origin logic you use for Grafana rules.Also applies to: 66-72, 89-103
118-151: Helper implementations align with existing rule identity semantics
getIsProvisionedcorrectly defers toisProvisionedRulefor ruler rules andisProvisionedPromRulefor Grafana Prom rules, with a safefalsefallback.
getEditableIdentifier:
- Delegates ruler rules to
ruleId.fromRulerRuleAndGroupIdentifierV2, matching existing patterns.- Constructs a minimal
GrafanaRuleIdentifierfor Grafana Prom rules.- Logs a warning and returns
undefinedotherwise, which is then handled gracefully by the caller.You might consider enriching the log message with some context (e.g., whether a promRule was present) if debugging this ever becomes necessary, but the current behaviour is functionally sound.
public/app/features/alerting/unified/hooks/useAbilities.ts (2)
231-281: Ruler abilities and silence support need null‑safety verification
useAllRulerRuleAbilitiesnow acceptsrule: RulerRuleDTO | undefinedand passes it into helpers like:
useIsRuleEditable(rulesSourceName, rule)isProvisionedRule(rule)rulerRuleType.grafana.rule(rule)isPluginProvidedRule(rule)useCanSilence(rule)and
useCanSilenceitself callsrulerRuleType.grafana.rule(rule)and readsrule.grafana_alert.namespace_uidwhen appropriate.This is all fine as long as those helpers are robust to
undefined(e.g., short‑circuit early). Please double‑check thatisPluginProvidedRule,rulerRuleType.grafana.rule, and related utilities explicitly handleundefinedto avoid runtime errors whenruleis absent (e.g., Grafana‑only Prom flows).The updated silence logic (global Grafana AM config + folder‑level permissions) also looks sound and consistent with
useGrafanaRulesSilenceSupport.Also applies to: 560-581, 595-603
339-343: Minor consistency and duplication considerationsA few small things you might consider over time:
IsGrafanaPromRuleEditableResultis very similar to whatuseIsRuleEditablereturns; if feasible, aligning the shapes or extracting a shared type could simplify future refactors.useFolderPermissions+useCanSilenceInFolderanduseIsGrafanaPromRuleEditableall derive folder‑level permissions; there may be an opportunity to centralize this logic later.toAbilityremains the single point for combining support and ACL checks, which keeps the abilities tables readable.These are non‑blocking and mainly about reducing duplication as the permissions surface grows.
Also applies to: 583-608, 610-615
📜 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/app/features/alerting/unified/rule-list/GrafanaRuleLoader.tsx
- public/locales/en-US/grafana.json
🧰 Additional context used
🪛 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: Unit tests (5 / 8)
🔇 Additional comments (9)
public/app/features/alerting/unified/components/rules/RuleDetails.test.tsx (2)
10-10: LGTM! Import added to support Cloud rule testing with mimir data source.The import is used correctly to create a mocked mimir data source for Cloud rule test scenarios.
87-87: LGTM! Cloud rule correctly configured with mimir data source.The
rulesSourceparameter is now properly provided togetCloudRule, which aligns with the PR's goal of adding Prometheus rule support. This correctly differentiates Cloud rule setup from Grafana rule setup (line 61), where the data source context is not needed.public/app/types/unified-alerting-dto.ts (1)
130-136: Provenance field addition aligns with provisioning logicAdding
provenance?: stringtoGrafanaPromRuleDTOBaseis consistent with howGrafanaRuleDefinition.provenanceis used and enables DTO-level provisioning checks (e.g.,isProvisionedPromRule,GrafanaRuleListItem.isProvisioned). Just ensure that the backend actually populates this field for provisioned Grafana-prom rules; otherwise these helpers will always treat them as non‑provisioned.public/app/features/alerting/unified/rule-list/GrafanaGroupLoader.tsx (1)
8-8: Prom-only loading and error handling look consistentUsing
isPromResponseLoadingfor skeletons and an explicit errorAlertwhenpromResponseis missing is straightforward and matches the new single‑source Prometheus flow. No functional issues here.Also applies to: 43-51, 53-64
public/app/features/alerting/unified/components/rules/RulesTable.test.tsx (2)
76-150: Grafana rules RBAC tests cover key edit/delete and lifecycle statesThe Grafana rules tests correctly:
- Use both
useAlertRuleAbilityand Grafana‑specific hooks to model permissions.- Distinguish between update vs delete action tuples.
- Cover creating/deleting lifecycle states with preloaded ruler state and plural ability hooks.
The expectations (Edit/Delete visibility, hiding view/edit while creating/deleting) align with the encoded ability tuples.
Also applies to: 151-200
203-283: Cloud rules RBAC tests mirror Grafana coverage appropriatelyCloud rule cases correctly:
- Drive permissions through
useRulerRuleAbility/useAlertRuleAbility.- Only rely on
useRulerRuleAbilitiesfor menu actions, consistent with comments aboutskipTokenfor Grafana hooks.- Assert the right presence/absence for Edit and Delete under allowed/denied scenarios.
This gives good confidence that RuleActionsButtons + AlertRuleMenu respect RBAC for both rule sources.
public/app/features/alerting/unified/rule-list/components/RuleActionsButtons.V2.tsx (1)
2-3: Props typing viaRequireAtLeastOneis a solid upgradeUsing
RequireAtLeastOnefor{ rule?, promRule? }and threading that throughPropsensures at least one rule source is present at compile time, while keeping the external API surface minimal. The added identifier types (EditableRuleIdentifier,GrafanaRuleIdentifier,RuleIdentifier) are consistent with the downstream usage inAlertRuleMenuandRedirectToCloneRule.Also applies to: 10-17, 25-37, 41-42
public/app/features/alerting/unified/hooks/useAbilities.ts (2)
17-18: Refactor touseAllRulerRuleAbilitieswithgroupIdentifieris a good consolidationRouting
useAlertRuleAbilityanduseAlertRuleAbilitiesthroughuseAllAlertRuleAbilities, and then intouseAllRulerRuleAbilities(rule.rulerRule, groupIdentifierV2), centralizes the Ruler‑side matrix logic and keeps the public API stable while you migrate callers.Marking
useAllAlertRuleAbilitiesas deprecated and derivinggroupIdentifierV2viagroupIdentifier.fromCombinedRule(rule)is a reasonable bridge strategy.Also applies to: 22-30, 195-217, 219-229
283-337: Grafana Prom rule abilities andskipTokenintegration look consistent
useAllGrafanaPromRuleAbilitiesmirrors the Ruler abilities matrix while:
- Using
isProvisionedPromRuleandprometheusRuleType.grafana.alertingRule(rule)to distinguish alerting vs recording rules.- Removing federated support (explicitly
false) and hard‑codingrulesPermissions('grafana').- Wiring silence support via
useGrafanaRulesSilenceSupportanduseCanSilenceInFolder(rule?.folderUid).
useIsGrafanaPromRuleEditablecorrectly bases edit/delete on folder‑scoped permissions, with sensible handling for missingrule/folderand aloadingflag feeding back into the abilities.
skipToken+useGrafanaPromRuleAbility/useGrafanaPromRuleAbilitiesprovide a clean way to opt out of GrafanaProm evaluation when there is no relevant rule, while still returning a well‑defined ability map.Overall this is a coherent extension of the existing abilities model to Grafana Prom rules.
Also applies to: 349-378, 380-406
| import { GrafanaRuleListItem } from './GrafanaRuleListItem'; | ||
| import LoadMoreHelper from './LoadMoreHelper'; |
There was a problem hiding this comment.
Add missing key prop to GrafanaRuleListItem in list rendering
In the rules.map loop, the Grafana branch now renders a <GrafanaRuleListItem> but doesn’t pass the key, which is what Biome is flagging and what React uses for stable reconciliation:
- case 'grafana':
- return (
- <GrafanaRuleListItem
- rule={rule}
- groupIdentifier={groupIdentifier}
- namespaceName={ruleWithOrigin.namespaceName}
- showLocation={true}
- />
- );
+ case 'grafana':
+ return (
+ <GrafanaRuleListItem
+ key={key}
+ rule={rule}
+ groupIdentifier={groupIdentifier}
+ namespaceName={ruleWithOrigin.namespaceName}
+ showLocation={true}
+ />
+ );This keeps the behavior consistent with the datasource and unknown branches and removes the linter error.
Also applies to: 154-163
🤖 Prompt for AI Agents
In public/app/features/alerting/unified/rule-list/FilterView.tsx around lines
15-16 (and similarly at 154-163), the Grafana branch renders
<GrafanaRuleListItem> inside a rules.map without a React key prop; update the
map to pass a stable unique key (e.g. key={rule.id} or
key={`${rule.datasource}-${rule.uid}`} or fallback to the index) to
GrafanaRuleListItem so its mapping matches the datasource and unknown branches
and resolves the linter/React reconciliation warning.
| export function isProvisionedPromRule(promRule: PromRuleDTO): boolean { | ||
| return prometheusRuleType.grafana.rule(promRule) && Boolean(promRule.provenance); | ||
| } |
There was a problem hiding this comment.
Align isProvisionedPromRule types with DTO shapes
Right now isProvisionedPromRule accepts PromRuleDTO, but provenance is only declared on GrafanaPromRuleDTOBase. From the types in this file, PromRuleDTO doesn’t expose provenance, so promRule.provenance is likely a TS type error.
Two concrete options:
- export function isProvisionedPromRule(promRule: PromRuleDTO): boolean {
- return prometheusRuleType.grafana.rule(promRule) && Boolean(promRule.provenance);
- }
+ export function isProvisionedPromRule(promRule: GrafanaPromRuleDTO): boolean {
+ return Boolean(promRule.provenance);
+ }or, if you intentionally want to accept all Prom rules:
-export function isProvisionedPromRule(promRule: PromRuleDTO): boolean {
- return prometheusRuleType.grafana.rule(promRule) && Boolean(promRule.provenance);
-}
+export function isProvisionedPromRule(promRule: PromRuleDTO | GrafanaPromRuleDTO): boolean {
+ return prometheusRuleType.grafana.rule(promRule) && Boolean((promRule as GrafanaPromRuleDTO).provenance);
+}Alternatively, you could lift provenance?: string into PromRuleDTOBase if you’re comfortable with it being structurally present on all Prom rules.
📝 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 isProvisionedPromRule(promRule: PromRuleDTO): boolean { | |
| return prometheusRuleType.grafana.rule(promRule) && Boolean(promRule.provenance); | |
| } | |
| export function isProvisionedPromRule(promRule: GrafanaPromRuleDTO): boolean { | |
| return Boolean(promRule.provenance); | |
| } |
| export function isProvisionedPromRule(promRule: PromRuleDTO): boolean { | |
| return prometheusRuleType.grafana.rule(promRule) && Boolean(promRule.provenance); | |
| } | |
| export function isProvisionedPromRule(promRule: PromRuleDTO | GrafanaPromRuleDTO): boolean { | |
| return prometheusRuleType.grafana.rule(promRule) && Boolean((promRule as GrafanaPromRuleDTO).provenance); | |
| } |
🤖 Prompt for AI Agents
In public/app/features/alerting/unified/utils/rules.ts around lines 171-173, the
function accepts PromRuleDTO but accesses provenance which only exists on
GrafanaPromRuleDTOBase; change the signature to a type guard so TypeScript knows
the narrower shape: declare the function as returning promRule is
GrafanaPromRuleDTOBase (e.g. function isProvisionedPromRule(promRule:
PromRuleDTO): promRule is GrafanaPromRuleDTOBase) and keep the runtime check
(prometheusRuleType.grafana.rule(promRule) && Boolean(promRule.provenance));
ensure GrafanaPromRuleDTOBase is imported or in scope. Alternatively, if you
want the function to only accept the Grafana subtype, change the parameter type
to GrafanaPromRuleDTOBase instead of PromRuleDTO, or add provenance?: string to
PromRuleDTOBase if provenance should be available on all Prom rules.
Test 5
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.
Replicated from ai-code-review-evaluation/grafana-coderabbit#5