Skip to content

Conversation

@zaibkhan
Copy link

@zaibkhan zaibkhan commented Sep 9, 2025

Mirrors ai-code-review-evaluation#5 for like-for-like benchmarking.

  • Base: notification-rule-baseline
  • Head: notification-rule-enhancements

Original PR excerpt:

Test 5

* 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]>
@codoki-pr-intelligence
Copy link

codoki-pr-intelligence bot commented Sep 9, 2025

Codoki PR Review

Summary: URL-encode UID in link, prevent malformed paths
What’s good: Solid branching between alerting/recording/unknown rules with careful optional chaining; type additions for provenance align with usage.
Review Status: ❌ Requires changes
Overall Priority: Critical
Review Update:
• Coverage: Reviewed all 14 files across 2 batches

Issues (Critical & High only)

Severity Issue Why it matters
Critical Correctness — Undefined 'ctx' in permission checks causes build failure …/hooks/useAbilities.ts
ctx is not defined in this scope, which will cause a compile-time error and break the build; use the existing access control service (e.g., contextSrv) or a dedicated hook to check folder permissions for Grafana-managed rules. Replace both permission checks to avoid runtime failures.

Showing top 1 issues. Critical: 1, High: 0. See inline suggestions for more.

Key Feedback (click to expand)
  • Needs improvement: - Encode dynamic path segments to avoid edge-case breakage when UIDs contain special characters.
  • Consider reusing the new isProvisionedPromRule() helper to keep provisioned checks consistent across the codebase.
  • Testing: Add a UI/unit test ensuring the generated href properly encodes UIDs containing characters like spaces or slashes.
  • Open questions: Are rule UIDs guaranteed to be strictly URL-safe (alphanumeric/ULID)? If not, encoding is needed to avoid broken links.

Confidence: 2/5 — Not ready to merge (1 critical · status: Requires changes)

React with 👍 or 👎 if you found this review useful.

loading,
};
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛑 Critical: ctx is not defined in this scope, which will cause a compile-time error and break the build; use the existing access control service (e.g., contextSrv) or a dedicated hook to check folder permissions for Grafana-managed rules. Replace both permission checks to avoid runtime failures.

Suggested change
import { contextSrv } from 'app/core/core';
const canEditGrafanaRules = contextSrv.hasPermissionInMetadata(rulesPermissions.update, folder);
const canRemoveGrafanaRules = contextSrv.hasPermissionInMetadata(rulesPermissions.delete, folder);

rulesSource: GrafanaRulesSource,
group: groupIdentifier.groupName,
namespace: namespaceName,
href: createRelativeUrl(`/alerting/grafana/${uid}/view`),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔷 Medium: UID is interpolated into the path without URL encoding; if a UID contains reserved characters, the link can break or route incorrectly. Encode the segment to ensure a valid, safe URL.

Suggested change
href: createRelativeUrl(`/alerting/grafana/${uid}/view`),
```suggestion
href: createRelativeUrl(`/alerting/grafana/${encodeURIComponent(uid)}/view`),

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants