Automated Test: notification-rule-enhancements #345
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>
📝 WalkthroughWalkthroughThis PR consolidates permission checks for alerting rules into plural ability hooks and introduces Grafana Prometheus rule-specific permission evaluation. The GrafanaRuleLoader component is replaced with a direct GrafanaRuleListItem pattern, and Ruler-based data fusion logic is removed from GrafanaGroupLoader. Permission system is expanded to evaluate both Ruler and Grafana sources. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
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 (3)
public/app/features/alerting/unified/rule-list/GrafanaGroupLoader.tsx (1)
24-28:⚠️ Potential issue | 🟡 MinorStale JSDoc: still mentions Ruler endpoint.
The comment says "Loads an evaluation group from Prometheus and Ruler endpoints" but Ruler loading has been removed in this PR.
📝 Suggested fix
/** - * Loads an evaluation group from Prometheus and Ruler endpoints. + * Loads an evaluation group from the Prometheus endpoint. * Displays a loading skeleton while the data is being fetched. * Polls the Prometheus endpoint every 20 seconds to refresh the data. */public/app/features/alerting/unified/rule-list/GrafanaGroupLoader.test.tsx (1)
211-218:⚠️ Potential issue | 🟡 Minor
folderUidis set togroup.name— likely incorrect mapping.Line 213 maps
folderUid: group.name, but folder UID and group name are different concepts. While this may not break current tests (sincegetGroupIdentifierderives the namespace UID fromgrafanaRulerNamespace.uid), it makes the test data semantically misleading.Consider using
grafanaRulerNamespace.uidor an appropriate folder UID value instead.public/app/features/alerting/unified/rule-list/components/RuleActionsButtons.V2.tsx (1)
104-106:⚠️ Potential issue | 🟠 MajorSilence drawer will not render when only
promRuleis provided.
SilenceGrafanaRuleDrawerrequires aRulerGrafanaRuleDTO(line 12 of SilenceGrafanaRuleDrawer.tsx), but the rendering condition on line 104 checksrulerRuleType.grafana.alertingRule(rule). When the component receives onlypromRule(as it does in the new alert list view that removed ruler API calls),ruleisundefined, so this condition is always false. The silence menu item remains visible and clickable inAlertRuleMenu, but clicking it setsshowSilenceDrawer = truewithout rendering the drawer—a silent failure.Either
SilenceGrafanaRuleDrawermust accept apromRuleparameter to handle both paths, or the rendering condition needs logic to construct a ruler rule from the prom rule when necessary.
🤖 Fix all issues with AI agents
In `@public/app/features/alerting/unified/rule-list/FilterView.tsx`:
- Around line 156-163: The map iteration dropped the required React key for
GrafanaRuleListItem causing reconciliation bugs; add the existing destructured
variable key as the key prop on GrafanaRuleListItem (the same key used by
DataSourceRuleLoader and UnknownRuleListItem) so each rendered
GrafanaRuleListItem receives key={key} and React can correctly track list items.
In `@public/app/features/alerting/unified/utils/rules.ts`:
- Around line 171-173: The function isProvisionedPromRule accesses
promRule.provenance but PromRuleDTO (PromAlertingRuleDTO | PromRecordingRuleDTO)
doesn't guarantee that property and prometheusRuleType.grafana.rule() is not a
type guard, causing a TS error; update isProvisionedPromRule to first perform
the runtime check using prometheusRuleType.grafana.rule(promRule) and then
cast/narrow promRule to the specific Grafana rule type (the variant that
contains provenance) before reading provenance, or use the existing type guard
isGrafanaRulerRule() to narrow the type, e.g. check grafana rule runtime then
treat promRule as the Grafana rule type and access provenance.
🧹 Nitpick comments (5)
public/app/features/alerting/unified/hooks/useAbilities.ts (1)
287-293: Stale comment// duplicateon Line 289.The comment
// duplicateappears to be a leftover note (perhaps from development or review). It should be removed to avoid confusion.Suggested fix
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);public/app/features/alerting/unified/rule-list/GrafanaRuleListItem.tsx (2)
35-48: Unnecessary optional chaining on required prop.
ruleis a required prop (non-optional inGrafanaRuleListItemProps), sorule?.health,rule?.lastError, andrule?.isPausedon lines 41, 42, 45 don't need optional chaining —rule.health,rule.lastError, andrule.isPausedsuffice. This is a minor nit.
50-53: Redundant type narrowing after type guard.Line 50's
prometheusRuleType.grafana.alertingRule(rule)is a TypeScript type guard that narrowsruletoGrafanaPromAlertingRuleDTO, which guarantees bothtotalsandstateproperties. The conditional on line 51 (rule && rule.type === PromRuleType.Alerting ? rule : undefined) re-checks the same constraint and can never beundefinedinside this branch.♻️ Suggested simplification
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/rule-list/GrafanaGroupLoader.test.tsx (2)
36-43:menuItems.deleteselector is defined but never asserted.The
deleteselector on line 42 is never used in any test. If delete is intentionally excluded from the "correct menu actions" test (line 191-207), consider removing the selector to avoid confusion, or add an assertion that delete is not present when appropriate.
221-234: Test helperrulerRuleToPromRuleomitslabels,annotations, andprovenance.These fields are accessed by
GrafanaRuleListItem(destructured or read via optional chaining). The tests work because of graceful handling ofundefined, but explicitly setting these fields (even as empty objects/undefined) would make the test data more intentional and robust.
| return ( | ||
| <GrafanaRuleLoader | ||
| key={key} | ||
| ruleIdentifier={{ ruleSourceName: 'grafana', uid: rule.uid }} | ||
| <GrafanaRuleListItem | ||
| rule={rule} | ||
| groupIdentifier={groupIdentifier} | ||
| namespaceName={ruleWithOrigin.namespaceName} | ||
| showLocation={true} | ||
| /> | ||
| ); |
There was a problem hiding this comment.
Missing key prop on GrafanaRuleListItem inside .map() iteration.
The key variable is available from the destructuring on Line 152 and is used by sibling elements (DataSourceRuleLoader on Line 165, UnknownRuleListItem on Line 168), but was dropped during the replacement of GrafanaRuleLoader. This will cause React reconciliation issues (incorrect reuse/reordering of list items).
Proposed fix
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.
| return ( | |
| <GrafanaRuleLoader | |
| key={key} | |
| ruleIdentifier={{ ruleSourceName: 'grafana', uid: rule.uid }} | |
| <GrafanaRuleListItem | |
| rule={rule} | |
| groupIdentifier={groupIdentifier} | |
| namespaceName={ruleWithOrigin.namespaceName} | |
| showLocation={true} | |
| /> | |
| ); | |
| return ( | |
| <GrafanaRuleListItem | |
| key={key} | |
| rule={rule} | |
| groupIdentifier={groupIdentifier} | |
| namespaceName={ruleWithOrigin.namespaceName} | |
| showLocation={true} | |
| /> | |
| ); |
🧰 Tools
🪛 Biome (2.3.13)
[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
156 - 163, The map iteration dropped the required React key for
GrafanaRuleListItem causing reconciliation bugs; add the existing destructured
variable key as the key prop on GrafanaRuleListItem (the same key used by
DataSourceRuleLoader and UnknownRuleListItem) so each rendered
GrafanaRuleListItem receives key={key} and React can correctly track list items.
| export function isProvisionedPromRule(promRule: PromRuleDTO): boolean { | ||
| return prometheusRuleType.grafana.rule(promRule) && Boolean(promRule.provenance); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the file we're reviewing
find . -path "*/public/app/features/alerting/unified/utils/rules.ts" -type f
# Check type definitions for PromRuleDTO and related types
rg -n "type PromRuleDTO|interface PromRuleDTO" --type=ts -A 5
# Check PromAlertingRuleDTO definition
rg -n "interface PromAlertingRuleDTO|type PromAlertingRuleDTO" --type=ts -A 15
# Check PromRecordingRuleDTO definition
rg -n "interface PromRecordingRuleDTO|type PromRecordingRuleDTO" --type=ts -A 15
# Check GrafanaPromRuleDTOBase
rg -n "GrafanaPromRuleDTOBase|GrafanaPromAlertingRuleDTO|GrafanaPromRecordingRuleDTO" --type=ts -A 10 | head -80
# Check the prometheusRuleType definition specifically
rg -n "prometheusRuleType" --type=ts -B 5 -A 15 | head -100Repository: admin-coderabbit/coderabbit-grafana-mirror
Length of output: 21911
🏁 Script executed:
#!/bin/bash
# Get more context on prometheusRuleType definition
rg -n "const prometheusRuleType|export const prometheusRuleType|export const prometheusRuleType" --type=ts -A 20
# Also search for where grafana.rule is defined in prometheusRuleType
rg -n "prometheusRuleType = {" --type=ts -A 25Repository: admin-coderabbit/coderabbit-grafana-mirror
Length of output: 2164
🏁 Script executed:
#!/bin/bash
# Get the full context around the isProvisionedPromRule function (lines 171-173)
sed -n '165,180p' public/app/features/alerting/unified/utils/rules.ts
# Check if there are any type guards or assertions nearby
rg -n "isProvisionedPromRule" --type=ts -B 3 -A 3
# Check the exact return type signature for isGrafanaPromAlertingRule and isGrafanaPromRecordingRule to confirm they are type guards
sed -n '103,109p' public/app/features/alerting/unified/utils/rules.ts
# Check if there's a tsconfig.json and what strictness settings are applied
cat tsconfig.json | head -50Repository: admin-coderabbit/coderabbit-grafana-mirror
Length of output: 5712
🏁 Script executed:
#!/bin/bash
# Let's verify the actual type chain more explicitly
cat public/app/types/unified-alerting-dto.ts | sed -n '121,182p'Repository: admin-coderabbit/coderabbit-grafana-mirror
Length of output: 1755
TypeScript type error: provenance does not exist on PromRuleDTO.
PromRuleDTO is PromAlertingRuleDTO | PromRecordingRuleDTO, neither of which declares a provenance property. Unlike isProvisionedRule (line 167), which uses isGrafanaRulerRule() as a proper type guard to narrow the type, prometheusRuleType.grafana.rule() is a plain boolean-returning function without a type predicate, so TypeScript won't narrow promRule after the &&. Accessing promRule.provenance violates type safety under strict mode.
Cast the narrowed rule after the runtime check:
Suggested fix
export function isProvisionedPromRule(promRule: PromRuleDTO): boolean {
- return prometheusRuleType.grafana.rule(promRule) && Boolean(promRule.provenance);
+ return prometheusRuleType.grafana.rule(promRule) && Boolean((promRule as GrafanaPromRuleDTO).provenance);
}📝 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: PromRuleDTO): 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 isProvisionedPromRule accesses promRule.provenance but PromRuleDTO
(PromAlertingRuleDTO | PromRecordingRuleDTO) doesn't guarantee that property and
prometheusRuleType.grafana.rule() is not a type guard, causing a TS error;
update isProvisionedPromRule to first perform the runtime check using
prometheusRuleType.grafana.rule(promRule) and then cast/narrow promRule to the
specific Grafana rule type (the variant that contains provenance) before reading
provenance, or use the existing type guard isGrafanaRulerRule() to narrow the
type, e.g. check grafana rule runtime then treat promRule as the Grafana rule
type and access provenance.
This pull request was automatically created by
@coderabbitai/e2e-reviewer.Batch created pull request.
Summary by CodeRabbit
Release Notes
Bug Fixes
Refactor