Grafana / PR #5

Notification Rule Processing Engine

grafana/grafana#106778 TypeScript / Go
Golden recall: 100% Extra findings: 7

Golden Comments (2/2 found)

# Comment Severity Status FriendlyReviewer Detail
#1 GrafanaRuleListItem missing key prop React — may cause rendering issues when list order changes HIGH (upgraded) ✓ Found FriendlyReviewer found exactly: "Missing React key prop on <GrafanaRuleListItem> inside .map() loop. Add key={key} to avoid reconciliation warnings and UI issues." (FilterView.tsx:162) — upgraded severity from Medium to High.
#2 RuleActionsButtons invoked with promRule only, but SilenceGrafanaRuleDrawer depends on rulerRule — clicking "Silence notifications" has no visible effect HIGH ✓ Found FriendlyReviewer found the exact mechanism and extended it: "Silence drawer gated on rulerRule but canSilence can be true from promRule path. Users may click Silence with no feedback." Also flagged the same pattern for Pause and Delete in AlertRuleMenu.tsx (L117, L149) — these were not in the golden set.

Supplementary Findings (11 findings)

Finding File Severity Legitimate?
Type error: isProvisionedPromRule parameter type PromRuleDTO lacks provenance property — TypeScript error utils/rules.ts:171 HIGH ✓ Real type bug — must be GrafanaPromRuleDTO
False positive tests: RuleDetails.test.tsx asserts absence of Edit/Delete buttons that the component never renders (regardless of permissions) RuleDetails.test.tsx:60 HIGH ✓ Tests verifying behavior that does not exist
Pause menu dead UI: canPause true via promRule path, but menu item gated on rulerRule AlertRuleMenu.tsx:117 HIGH ✓ Same pattern as golden #2 — extension finding
Delete menu dead UI: canDelete true via promRule path, but menu item gated on rulerRule AlertRuleMenu.tsx:149 HIGH ✓ Same pattern as golden #2 — extension finding
Inconsistent silence ability semantics: recording rules return [silenceSupported, false] in new path vs [false, false] in old path useAbilities.ts:317 MEDIUM ✓ Different UI behavior depending on code path
Missing folder-loading guard: during folder load, silenceSupported is true instead of false useAbilities.ts:317 MEDIUM ✓ Incorrect transient state
Missing dependency: ctx not in useMemo dependency array useAbilities.ts:350 MEDIUM ✓ Potential stale closure
Stray comment // duplicate on useIsGrafanaPromRuleEditable call useAbilities.ts:286 LOW ✓ Code quality
Unused mocks for useRulerRuleAbility and useGrafanaPromRuleAbility RulesTable.test.tsx:21 LOW ✓ Test cleanup
Inconsistent mock overrides for useGrafanaPromRuleAbility across tests RulesTable.test.tsx:82 LOW ✓ Test cleanup
Unused mock for useRulerRuleAbility in cloud rules test RulesTable.test.tsx:210 LOW ✓ Test cleanup

Honest Analysis

FriendlyReviewer detected both golden bugs perfectly. Golden #1 (missing React key prop) was an exact match, and FriendlyReviewer upgraded its severity from Medium to High, which is justified: missing keys in dynamic lists cause reconciliation bugs that are notoriously hard to debug. Golden #2 (Silence drawer dead UI) was found with the exact mechanism described, and FriendlyReviewer went further by identifying the same pattern for Pause and Delete actions in AlertRuleMenu.tsx — dead UI states the golden comments did not mention.

The most valuable supplementary finding is the type error in isProvisionedPromRule: using PromRuleDTO instead of GrafanaPromRuleDTO means the provenance property does not exist on the parameter type. This is a genuine TypeScript compilation error that would surface in strict mode, and it propagates to both rules.ts and useAbilities.ts. A human reviewer would likely catch this during a compilation check, but FriendlyReviewer found it from static analysis alone.

The false positive test finding in RuleDetails.test.tsx is also noteworthy. The test asserts that Edit/Delete buttons are not visible under certain permission conditions, but the component never renders those buttons at all — regardless of permissions. This means the test validates nothing about the RBAC logic it claims to test. FriendlyReviewer correctly identified that the test is testing a behavior that does not exist in the component, making it a complete no-op. This kind of structural test analysis — verifying that the assertions match the rendered component tree — is something human reviewers frequently overlook in a diff-heavy review.

The three Medium findings (inconsistent silence semantics, missing folder-loading guard, missing ctx dependency) are all legitimate production bugs. The ctx missing from the useMemo dependency array is a classic stale closure — the kind of bug that React developers encounter routinely but that static analysis surfaces reliably. The folder-loading guard is a transient-state issue: during load, silenceSupported is true instead of false, which could cause the UI to show a silence button that won't work until the folder resolves.

On the weaker side, the two Low findings (stray comment, unused mocks) are noise — they are real but trivial. The stray // duplicate comment and orphaned mock imports are cleanup items a linter would catch; FriendlyReviewer's inclusion of them reflects its low threshold for reporting rather than a meaningful contribution to code quality. This is acceptable: the trade-off for catching the genuine bugs is tolerating some surface-level findings.

What FriendlyReviewer did not find is harder to assess since the golden set was fully covered. One could argue the analysis could have explored the broader impact of migrating from ruler API to prometheus API — the PR introduces a parallel permission-checking path, and the interaction between the two paths under concurrent state changes (e.g., a rule being edited via ruler while the list view reads from prometheus) was not explored. However, this is speculative and would require runtime reasoning that no static analysis tool performs today.

Overall, FriendlyReviewer performed adequately: 2/2 golden bugs were found, and the supplementary findings are genuinely useful — particularly the type error and false-positive test identification. However, the tool does not reason about architectural concerns at all: it cannot assess whether the parallel permission-checking path is an appropriate design, whether its interaction with the existing ruler-based path creates fragility, or whether the broader test strategy is sound. These are exactly the areas where a human reviewer adds value beyond what static analysis provides. The 4 LOW findings are noise that a human reviewer would not mention in a PR comment — they belong in a linter report, not a code review. FriendlyReviewer is a useful supplement to human review, not a replacement for it.

FriendlyReviewer's full review

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

friendly-reviewer bot commented now

Hi there 👋

🌕 Tech
6 high, 3 medium, 4 low

The merge request introduces a new parallel permission-checking path for GrafanaPromRuleDTO-based rules, migrating the list view from ruler API to prometheus API. However, several critical issues must be resolved before merging:

  • Type error in isProvisionedPromRule: The function parameter type PromRuleDTO does not include provenance, causing TypeScript errors. Affects both rules.ts and useAbilities.ts.
  • Missing React key prop in FilterView.tsx: GrafanaRuleListItem inside .map() lacks a key, causing reconciliation warnings and potential UI glitches.
  • False positive RBAC tests in RuleDetails.test.tsx: Tests assert absence of Edit/Delete buttons that are never rendered by the component, regardless of permissions. The component does not use the RBAC hooks that are being tested.
  • Inconsistent silence ability semantics: The new useAllGrafanaPromRuleAbilities returns [silenceSupported, false] for recording rules, while the old path returns [false, false], leading to different UI behavior (silence button shown but disabled vs. hidden).
  • Dead UI states in AlertRuleMenu.tsx and RuleActionsButtons.V2.tsx: Permission checks (e.g., canPause, canDelete, canSilence) may pass via the new promRule path, but the corresponding UI elements still require rulerRule to render, causing silent no-ops.
  • Missing folder-loading guard for silence ability in useAllGrafanaPromRuleAbilities.
  • Potential missing dependency array entry in useIsGrafanaPromRuleEditable (ctx not listed).
  • Stray developer comment // duplicate in useAbilities.ts.
  • Unused mock code in RulesTable.test.tsx and inconsistent mock scopes.

These issues need to be addressed to ensure correctness, consistency, and test validity.

These might need a close look

  • 🔴 public/app/features/alerting/unified/utils/rules.ts (L171)
    Type error: isProvisionedPromRule parameter type PromRuleDTO lacks provenance property. Change to GrafanaPromRuleDTO or use type narrowing.
  • 🔴 public/app/features/alerting/unified/rule-list/FilterView.tsx (L162)
    Missing React key prop on <GrafanaRuleListItem> inside .map() loop. Add key={key} to avoid reconciliation warnings and UI issues.
  • 🔴 public/app/features/alerting/unified/components/rules/RuleDetails.test.tsx (L60)
    False positive test: asserts Edit/Delete buttons not in document, but component never renders them. Tests do not validate RBAC enforcement. Refactor to test relevant component or remove.
  • 🔴 public/app/features/alerting/unified/components/rule-viewer/AlertRuleMenu.tsx (L117)
    Pause menu item renders only when rulerRule exists, but canPause can be true from promRule path. Extend component or add branch for promRule.
  • 🔴 public/app/features/alerting/unified/components/rule-viewer/AlertRuleMenu.tsx (L149)
    Delete menu item condition requires rulerRule, but canDelete can be true from promRule path. Handle promRule-only case.
  • 🔴 public/app/features/alerting/unified/rule-list/components/RuleActionsButtons.V2.tsx (L83)
    Silence drawer gated on rulerRule but canSilence can be true from promRule path. Users may click Silence with no feedback. Adapt drawer to accept promRule.

Worth checking

  • 🟡 public/app/features/alerting/unified/hooks/useAbilities.ts (L317)
    Inconsistency in silence ability semantics: recording rules return [silenceSupported, false] in new path vs [false, false] in old path. Add isAlertingRule check for supported field.
  • 🟡 public/app/features/alerting/unified/hooks/useAbilities.ts (L317)
    Missing folder-loading guard for silence ability: during folder load, silenceSupported is true instead of false. Add loading check.
  • 🟡 public/app/features/alerting/unified/hooks/useAbilities.ts (L350)
    Potential missing dependency ctx in useMemo dependency array. Add ctx or document exclusion.

Small things (take or leave)

  • 🔵 public/app/features/alerting/unified/hooks/useAbilities.ts (L286)
    Stray comment // duplicate on useIsGrafanaPromRuleEditable call. Remove or clarify.
  • 🔵 public/app/features/alerting/unified/components/rules/RulesTable.test.tsx (L21)
    Unused mocks for useRulerRuleAbility and useGrafanaPromRuleAbility (singular) added. Remove or document future use.
  • 🔵 public/app/features/alerting/unified/components/rules/RulesTable.test.tsx (L82)
    Inconsistent mock overrides for useGrafanaPromRuleAbility across tests. Apply consistently or remove unused mocks.
  • 🔵 public/app/features/alerting/unified/components/rules/RulesTable.test.tsx (L210)
    Unused mock for useRulerRuleAbility in cloud rules test. Remove or comment.