Grafana / PR #24

Notification Rule Processing Engine

grafana/grafana#106778
Golden recall: 100% Extra findings: 10

Golden Comments (2/2 found)

# Comment Severity Status FriendlyReviewer Detail
#1 The rendered GrafanaRuleListItem is missing the required key prop for React list items. This can cause rendering issues when the list order changes. MEDIUM ✓ Found "GrafanaRuleListItem rendered inside rules.map(...) lost its React key prop, while DataSourceRuleLoader and UnknownRuleListItem in the same switch still receive key={key}. Without a stable key React reconciles by index, so component state can be reused for the wrong rule during infinite scroll / filter re-runs and React logs a key warning" (FilterView.tsx L157)
#2 RuleActionsButtons is invoked with only promRule, but SilenceGrafanaRuleDrawer inside RuleActionsButtons still depends on a Grafana Ruler rule being present, so for Grafana rules coming from list views the 'Silence notifications' menu entry (now driven by Grafana Prom abilities) will toggle showSilenceDrawer without ever rendering the drawer. This means clicking 'Silence notifications' for these rules has no visible effect. HIGH ✓ Found "The Silence menu action does nothing in the new Grafana list view. The silence drawer only renders when rulerRuleType.grafana.alertingRule(rule) && showSilenceDrawer, and SilenceGrafanaRuleDrawer requires a RulerGrafanaRuleDTO; with rule undefined the drawer never opens, yet AlertRuleMenu still shows the "Silence notifications" item" (RuleActionsButtons.V2.tsx L96)

Supplementary Findings (10 findings)

Finding File Severity Legitimate?
Delete and Pause actions missing in the new Grafana list view — menu items hard-gated on a ruler rule that is never supplied RuleActionsButtons.V2.tsx:87 HIGH ✓ User-visible regression — neither action renders even with the right permissions; the new grafana abilities are dead code in the list view
canDuplicate spuriously enabled for cloud/external rules — skipToken path still computes a real duplicate ability AlertRuleMenu.tsx:91 MEDIUM ✓ Real permissions bug — a user holding only the Grafana create permission sees "Duplicate" on cloud rules they cannot duplicate
ModifyExport/Pause/Restore ability logic diverges between the list and detail views for Grafana recording rules useAbilities.ts:321 MEDIUM ✓ Borderline: real inconsistency between the two ability sets, but the intended behavior needs confirmation
Provisioning detection now depends entirely on provenance in the Prometheus /api/v1/rules response rules.ts:171 MEDIUM ✓ Legitimate robustness concern — rules served without provenance become editable/deletable
Misleading comment and variable name after removing the ruler preload dispatch prometheusGroupsGenerator.ts:81 LOW ✓ Correct — nothing ruler-related is preloaded anymore; rename and drop the comment
Leftover // duplicate debug comment useAbilities.ts:288 LOW ✓ Trivial but real leftover cleanup
provenance typed as loose string instead of the known value union unified-alerting-dto.ts:136 LOW ✓ Borderline — a typing suggestion, non-blocking; matches how the ruler-side field is treated
Dead useGrafanaPromRuleAbility mock with a misleading comment in RulesTable tests RulesTable.test.tsx:26 LOW ✓ Real — the mocked hook is never called by the rendered component path
Grafana edit tests exercise only the legacy ability path — the new prom-based edit behavior is never asserted RulesTable.test.tsx:118 LOW ✓ Real test gap — the MR's new edit logic is not locked in
RuleDetails RBAC assertions pass trivially regardless of the mocked permission RuleDetails.test.tsx:48 LOW ✓ Borderline — pre-existing weakness, but this MR touched the file

Honest Analysis

FriendlyReviewer found both golden bugs of this PR — recall 100%. The two dataset goldens are regressions of the new alert list view, and both are matched directly. The first (Medium) is the GrafanaRuleListItem rendered inside rules.map(...) without a React key prop, while its siblings in the same switch still receive one — the candidate reproduces the exact consequence (React reconciles by index, component state can be reused for the wrong rule). The second (High) is the 'Silence notifications' menu entry that toggles its drawer flag without ever rendering the drawer, because SilenceGrafanaRuleDrawer still requires a Grafana Ruler rule that the promRule-only list view never supplies — same mechanism, same consequence as the reference comment.

The review also flagged the sibling regression the dataset does not cover: the Delete and Pause actions disappear entirely from the new list view (RuleActionsButtons.V2 L87), because AlertRuleMenu still hard-gates them on a ruler rule. It is the same root-cause family as the Silence golden — the menu is driven by Grafana Prom abilities while the underlying actions still require ruler data — and it is arguably the most visible UX break of the three, yet it is not scored.

The remaining extras are mostly test and consistency concerns: a dead mock and misleading comment in RulesTable.test.tsx, Grafana edit tests that only exercise the legacy ability path, trivial RBAC assertions in RuleDetails, the spurious Duplicate ability for cloud rules, the divergence between the two ability sets for recording rules, and the provenance dependency of the new provisioning check. None matches a golden, but all are specific, localizable, and defensible. A couple are borderline — the provenance union typing suggestion and the pre-existing RuleDetails test weakness — and are presented as such.

FriendlyReviewer's full review

As posted on the PR — the complete output from FriendlyReviewer.

friendly-reviewer bot commented now

Hi there 👋

🌥️ Tech 🌥️ Feat
2 high, 4 medium, 6 low 1 issue, 1 mitigated

This MR removes the ruler-API dependency from the Grafana alert list view, replacing the deleted GrafanaRuleLoader with the new GrafanaRuleListItem and deriving list-view actions directly from prometheus (promRule) data. The core refactor direction is sound: the DTO change (optional provenance field) and the new isProvisionedPromRule helper are additive, type-sound, and consistent with the backend contract, and the i18n catalog cleanup is correct with no dangling keys. However, the change introduces several user-visible regressions in the new list view: the Delete and Pause actions no longer appear (the menu items remain hard-gated on a ruler rule that is never supplied), the Silence menu item renders but does not open its drawer, and a permissions bug can spuriously surface the Duplicate action for cloud/external rules. There are also consistency gaps that should be addressed: a missing React key on list items, divergent ability logic between the list and detail views for recording rules, dead test mocks that overstate coverage of the new edit path, and a few stale comments. These issues should be resolved before merging.

These might need a close look

  • 🔴 public/app/features/alerting/unified/rule-list/components/RuleActionsButtons.V2.tsx (L87)
    Delete and Pause actions are missing in the new Grafana list view. In the promRule-only path rule is undefined, and AlertRuleMenu (AlertRuleMenu.tsx:161) still hard-gates Delete on canDelete && rulerRule and Pause on rulerRuleType.grafana.rule(rulerRule) && groupOrigin==='grafana', so neither renders even when the user holds the corresponding permissions. The newly computed grafana pause/delete abilities are dead code in the list view, and the new GrafanaGroupLoader.test.tsx test codifies the dropdown without delete. Either drive delete/pause from the promRule/grafana-ability path or explicitly justify removing the actions.
  • 🔴 public/app/features/alerting/unified/rule-list/components/RuleActionsButtons.V2.tsx (L96)
    The Silence menu action does nothing in the new Grafana list view. The silence drawer only renders when rulerRuleType.grafana.alertingRule(rule) && showSilenceDrawer, and SilenceGrafanaRuleDrawer requires a RulerGrafanaRuleDTO; with rule undefined the drawer never opens, yet AlertRuleMenu still shows the "Silence notifications" item. Either derive the needed rule data from promRule/GrafanaPromRuleDTO or hide the item in the promRule-only path.

Worth checking

  • 🟡 public/app/features/alerting/unified/rule-list/FilterView.tsx (L157)
    GrafanaRuleListItem rendered inside rules.map(...) lost its React key prop, while DataSourceRuleLoader and UnknownRuleListItem in the same switch still receive key={key}. Without a stable key React reconciles by index, so component state can be reused for the wrong rule during infinite scroll / filter re-runs and React logs a key warning. Add key={key}. (Also fix the stray trailing } in the getGrafanaRuleKey template literal at line 207.)
  • 🟡 public/app/features/alerting/unified/components/rule-viewer/AlertRuleMenu.tsx (L91)
    canDuplicate is spuriously enabled for cloud/external rules. When promRule is not Grafana-managed, useGrafanaPromRuleAbilities is invoked with skipToken but still computes a real [true, hasPermission(AlertingRuleCreate)] for duplicate, so a user holding only the Grafana AlertingRuleCreate permission (not AlertingRuleExternalWrite) sees "Duplicate" for a cloud rule they cannot duplicate. Gate the grafana side of the OR on prometheusRuleType.grafana.rule(promRule) or make the skipToken path return [false, false] for every action.
  • 🟡 public/app/features/alerting/unified/hooks/useAbilities.ts (L321)
    ModifyExport/Pause/Restore diverge between the two ability sets: the ruler set uses isGrafanaManagedAlertRule (true for both Grafana alerting and recording rules) while the grafana set uses isAlertingRule (true only for alerting rules). For a Grafana recording rule, canExport/canPause differ between the rule-viewer and list views. Confirm the intended behavior and align both sets.
  • 🟡 public/app/features/alerting/unified/utils/rules.ts (L171)
    isProvisionedPromRule makes provisioning detection (which gates edit/delete of rules) depend entirely on the Grafana Prometheus /api/v1/rules response carrying provenance. The backend populates it today, but any Grafana-managed rule served through a path without provenance would be treated as non-provisioned and become editable/deletable, whereas the old ruler-based path protected it. Add a doc comment noting this reliance and consider a conservative check for unknown provenance state.
Small things (take or leave)
  • 🔵 public/app/features/alerting/unified/rule-list/hooks/prometheusGroupsGenerator.ts (L81)
    After removing the getGrafanaRulerGroup preload dispatch, the comment "This is not mandatory to preload ruler rules, but it improves the UX" and the variable name cacheAndRulerPreload (line 84) are misleading — nothing ruler-related is preloaded anymore. Rename (e.g. cacheAndPreload) and drop the comment.
  • 🔵 public/app/features/alerting/unified/hooks/useAbilities.ts (L288)
    Leftover // duplicate debug comment on the useIsGrafanaPromRuleEditable(rule) call — minor cleanup.
  • 🔵 public/app/types/unified-alerting-dto.ts (L136)
    provenance?: string is non-breaking, but the known values are limited to ""/api/file/converted_prometheus; typing it as a union would give stronger compile-time guarantees for the new isProvisionedPromRule consumers. Not blocking — the loose string matches how isProvisionedRule treats the ruler-side field.
  • 🔵 public/app/features/alerting/unified/components/rules/RulesTable.test.tsx (L26)
    The useGrafanaPromRuleAbility (singular) mock is dead code: RulesTable renders the non-V2 RuleActionsButtons, which never calls the singular hook (only RuleActionsButtons.V2 does), and AlertRuleMenu uses the plural useGrafanaPromRuleAbilities. The mock and its misleading comment ("We can also use useGrafanaPromRuleAbility (singular) for simpler mocking") imply the prom-based edit ability is exercised when it is not. Remove it or add a test that actually renders a component path using the new edit ability.
  • 🔵 public/app/features/alerting/unified/components/rules/RulesTable.test.tsx (L118)
    The Grafana edit tests ("Should render Edit button..." / "should not render Edit...") exercise only the legacy useAlertRuleAbility path via the non-V2 RuleActionsButtons, so the MR's new useGrafanaPromRuleAbility/useGrafanaPromRuleAbilities edit behavior is never asserted. To lock in the new prom-based edit logic, exercise the V2 path (e.g. via GrafanaRuleListItem/RuleActionsButtons.V2) or add an explicit assertion for the plural edit ability in AlertRuleMenu.
  • 🔵 public/app/features/alerting/unified/components/rules/RuleDetails.test.tsx (L48)
    The RBAC assertions in RuleDetails ("Should not render Edit/Delete button...") pass trivially: the rendered tree never produces an Edit link or Delete button regardless of the mocked permission, so the absence is guaranteed by the component structure rather than the RBAC logic they claim to test. Pre-existing weakness, but this MR touched the file; consider asserting on behavior actually influenced by the mocked permission or removing the dead assertions.

✅ Feature-level checklist

Not there yet

  • ❌ The change works as expected from a user's perspective (PR checklist item).
    The new list view has user-visible regressions: the Delete and Pause actions no longer render (menu items hard-gated on a rule that is never supplied), and the Silence menu item is shown but its drawer never opens. Additionally, a permissions bug can surface the Duplicate action for cloud/external rules the user cannot duplicate. These break the expected user experience and should be fixed before merging.

Partially covered

  • ⚠️ The docs are updated, and if this is a notable improvement it is added to the What's New doc (PR checklist item).
    This MR is a refactor of an existing view rather than a new notable improvement, so formal What's New documentation may not be mandatory. However, the behavioral changes (removal of Delete/Pause/Silence capabilities from the list view) are currently undocumented and need to be justified/documented, and several code-level cleanups (comments, naming, dead mocks) remain outstanding.