feat(alert): improves deployment alerts UI#1519
Conversation
WalkthroughThis set of changes introduces finer-grained feature flags for alert creation and update, refines authorization logic for alert operations, and updates both backend and frontend alert handling. The frontend now supports toggling alert enabled status (with feature flag gating), removes alert deletion from the UI, and updates test and type definitions accordingly. Error handling and authorization checks are strengthened across services and repositories. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend
participant API
participant NotificationsService
User->>Frontend: Toggle alert enabled status
Frontend->>API: PATCH /v1/alerts/:id (if NOTIFICATIONS_ALERT_UPDATE flag enabled)
API->>NotificationsService: Validate user, check permissions
NotificationsService->>NotificationsService: Check allowed fields for update
NotificationsService-->>API: Update "enabled" field if permitted
API-->>Frontend: Return updated alert
Frontend-->>User: Show success or error notification
Possibly related PRs
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (13)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (11)
⏰ Context from checks skipped due to timeout of 90000ms (5)
🔇 Additional comments (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
ygrishajev
force-pushed
the
feature/alerts-list
branch
from
7524615 to
a43ca67
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1519 +/- ##
==========================================
- Coverage 40.48% 40.43% -0.06%
==========================================
Files 871 871
Lines 21115 21149 +34
Branches 3841 3854 +13
==========================================
+ Hits 8548 8551 +3
+ Misses 12386 12288 -98
- Partials 181 310 +129
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/deploy-web/src/components/alerts/AlertsListContainer/AlertsListContainer.tsx (1)
81-106: Well-implemented toggle functionality!The toggle function properly manages loading state, provides user feedback, and handles errors. Good consistency with the remove function's pattern.
Consider adding data test IDs to the success and error notifications for better testability:
- notificator.success(`Alert ${enabled ? "enabled" : "disabled"}`); + notificator.success(`Alert ${enabled ? "enabled" : "disabled"}`, { + dataTestId: `alert-toggle-success-notification` + });- notificator.error("Failed to update alert"); + notificator.error("Failed to update alert", { + dataTestId: "alert-toggle-error-notification" + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/api/src/core/services/feature-flags/feature-flags.service.spec.ts(5 hunks)apps/api/src/core/services/feature-flags/feature-flags.ts(1 hunks)apps/api/src/deployment/controllers/deployment/deployment.controller.ts(1 hunks)apps/api/src/notifications/routes/proxy/proxy.route.ts(1 hunks)apps/deploy-web/src/components/alerts/AlertsListContainer/AlertsListContainer.tsx(4 hunks)apps/deploy-web/src/components/alerts/AlertsListView/AlertsListView.spec.tsx(5 hunks)apps/deploy-web/src/components/alerts/AlertsListView/AlertsListView.tsx(4 hunks)apps/deploy-web/src/components/alerts/AlertsPage.tsx(1 hunks)apps/notifications/src/interfaces/rest/interceptors/auth/auth.interceptor.ts(1 hunks)apps/notifications/src/modules/alert/repositories/alert/alert.repository.ts(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
apps/api/src/deployment/controllers/deployment/deployment.controller.ts (1)
Learnt from: baktun14
PR: akash-network/console#1428
File: apps/api/src/deployment/controllers/deployment/deployment.controller.ts:0-0
Timestamp: 2025-06-03T15:06:34.211Z
Learning: The `getByOwnerAndDseq` method in `apps/api/src/deployment/controllers/deployment/deployment.controller.ts` is intentionally public without the `@Protected` decorator because it serves public blockchain data from an indexer, following the pattern of public blockchain APIs.
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: validate-api
- GitHub Check: test-api-build
- GitHub Check: validate-notifications
- GitHub Check: validate-deploy-web
- GitHub Check: test-deploy-web-build
🔇 Additional comments (20)
apps/api/src/deployment/controllers/deployment/deployment.controller.ts (1)
128-132: LGTM - Excellent defensive programming improvement.The explicit error handling ensures a proper 404 HTTP response when a deployment doesn't exist, preventing undefined/null returns. This follows the consistent error handling pattern used throughout the codebase with
assert.apps/deploy-web/src/components/alerts/AlertsPage.tsx (1)
12-13: LGTM - Clear UI text improvement.The change from "Alerts" to "Configured Alerts" provides better clarity and aligns with the refined alert management capabilities introduced in this PR.
apps/notifications/src/interfaces/rest/interceptors/auth/auth.interceptor.ts (1)
27-28: LGTM - Excellent security improvement with granular permissions.The change from broad "manage" rights to specific "read" all fields and "update" only the "enabled" field implements the principle of least privilege. This aligns perfectly with the new toggle functionality while preventing unauthorized modifications to other alert properties.
apps/api/src/notifications/routes/proxy/proxy.route.ts (2)
69-69: LGTM - Good feature flag granularity for alert creation.Splitting to
NOTIFICATIONS_ALERT_CREATEprovides more granular control and allows independent rollout of create functionality.
72-72: LGTM - Good feature flag granularity for alert updates.Using
NOTIFICATIONS_ALERT_UPDATEfor PATCH operations complements the granular authorization changes and allows independent control of update functionality.apps/api/src/core/services/feature-flags/feature-flags.service.spec.ts (3)
34-34: LGTM - Test updated for new feature flag structure.Correctly updated to use
NOTIFICATIONS_ALERT_CREATEinstead of the deprecated combined flag.
61-61: LGTM - Good typo fix and test update.Fixed the typo from "environement" to "environment" while updating the test for the new feature flag structure.
84-84: LGTM - Consistent test updates for new feature flag.All test cases have been properly updated to use
NOTIFICATIONS_ALERT_CREATE, maintaining test coverage for the refactored feature flag structure.Also applies to: 86-86, 107-107, 119-119
apps/api/src/core/services/feature-flags/feature-flags.ts (1)
1-4: Good refactor for granular permission control!Splitting the combined mutation flag into separate CREATE and UPDATE flags follows the principle of least privilege and allows for more fine-grained access control.
apps/notifications/src/modules/alert/repositories/alert/alert.repository.ts (3)
1-8: Well-structured imports for field-level authorization!The CASL imports and lodash utility are appropriate for implementing granular permission checks on field updates.
Also applies to: 73-73
84-88: Good addition for preserving ability context!Storing the ability parameters enables field-level permission checks in the update operations.
103-113: Robust field-level authorization implementation!The permission check correctly validates that only permitted fields are being updated. The error message clearly indicates which fields are unauthorized.
Consider whether skipping the check when
abilityParamsis undefined is intentional. If this method should always require authorization, you might want to throw an error whenabilityParamsis not set.apps/deploy-web/src/components/alerts/AlertsListView/AlertsListView.spec.tsx (2)
41-46: Test updates correctly reflect the new checkbox functionality!The tests properly verify both checked and unchecked states of the alert enable/disable checkbox.
Also applies to: 64-68
108-122: Well-implemented mock for feature flag testing!The mock useFlag implementation and dependency injection pattern provide good testability for feature-flag-controlled behavior.
apps/deploy-web/src/components/alerts/AlertsListContainer/AlertsListContainer.tsx (2)
22-24: Good refactor to unify loading state management!Using a generic
loadingIdsstate that can track multiple async operations (removal, toggling) is a cleaner abstraction than operation-specific states.Also applies to: 35-35, 45-46
51-51: Also applies to: 71-76apps/deploy-web/src/components/alerts/AlertsListView/AlertsListView.tsx (4)
16-18: Excellent dependency injection pattern for testability!The DEPENDENCIES constant and optional prop with defaults provide a clean way to inject mocks during testing while keeping production code simple.
Also applies to: 28-28, 39-42
45-62: Well-implemented interactive checkbox column!The checkbox properly handles loading states, user interactions, and visibility is correctly controlled by the feature flag.
Also applies to: 112-114
64-72: Good UX improvement for deployment name links!Conditionally rendering the link only when a
dseqparameter exists prevents broken links and improves user experience.
82-94: User-friendly alert type labels!The specific labels ("Threshold", "Deployment Close") are much more intuitive than raw type values.
ygrishajev
force-pushed
the
feature/alerts-list
branch
from
a43ca67 to
aef58d9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/deploy-web/src/components/alerts/AlertsListView/AlertsListView.spec.tsx (1)
31-89: Consider adding checkbox interaction testWhile the tests verify checkbox rendering and state, consider adding a test that simulates checkbox interaction to ensure
onToggleis called with the correct parameters.Add this test case after the existing tests:
+import { render, screen, fireEvent } from "@testing-library/react"; it("renders pagination when total is greater than minimum page size", () => { // existing test... }); + + it("calls onToggle when checkbox is clicked", () => { + const mockAlert = buildAlert({ + id: "test-id", + enabled: false + }); + const props = setup({ data: [mockAlert] }); + + const checkbox = screen.getByRole("checkbox"); + fireEvent.click(checkbox); + + expect(props.onToggle).toHaveBeenCalledWith("test-id", true); + });apps/deploy-web/src/components/alerts/AlertsListView/AlertsListView.tsx (1)
81-94: Consider safer type checking for paramsWhile the logic is clear, the type checking could be more defensive.
Consider adding explicit type guards for better type safety:
cell: info => { const type = info.getValue(); const params = info.row.original.params; if (type === "DEPLOYMENT_BALANCE") { return "Threshold"; - } else if (type === "CHAIN_MESSAGE" && params && "type" in params && params.type === "DEPLOYMENT_CLOSED") { + } else if ( + type === "CHAIN_MESSAGE" && + params && + typeof params === "object" && + "type" in params && + params.type === "DEPLOYMENT_CLOSED" + ) { return "Deployment Close"; } return startCase(type.toLowerCase()); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
apps/api/src/core/services/feature-flags/feature-flags.service.spec.ts(5 hunks)apps/api/src/core/services/feature-flags/feature-flags.ts(1 hunks)apps/api/src/deployment/controllers/deployment/deployment.controller.ts(1 hunks)apps/api/src/notifications/routes/proxy/proxy.route.ts(1 hunks)apps/deploy-web/src/components/alerts/AlertsListContainer/AlertsListContainer.tsx(4 hunks)apps/deploy-web/src/components/alerts/AlertsListView/AlertsListView.spec.tsx(5 hunks)apps/deploy-web/src/components/alerts/AlertsListView/AlertsListView.tsx(4 hunks)apps/deploy-web/src/components/alerts/AlertsPage.tsx(1 hunks)apps/notifications/src/interfaces/rest/interceptors/auth/auth.interceptor.ts(1 hunks)apps/notifications/src/modules/alert/alert.module.ts(2 hunks)apps/notifications/src/modules/alert/repositories/alert/alert.repository.ts(4 hunks)apps/notifications/src/modules/alert/repositories/alert/deployment-alert.repository.ts(1 hunks)apps/notifications/src/modules/alert/services/deployment-alert/deployment-alert.service.spec.ts(2 hunks)apps/notifications/src/modules/alert/services/deployment-alert/deployment-alert.service.ts(2 hunks)apps/notifications/test/functional/alert-crud.spec.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/notifications/src/modules/alert/alert.module.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- apps/deploy-web/src/components/alerts/AlertsPage.tsx
- apps/api/src/notifications/routes/proxy/proxy.route.ts
- apps/api/src/core/services/feature-flags/feature-flags.service.spec.ts
- apps/api/src/core/services/feature-flags/feature-flags.ts
- apps/api/src/deployment/controllers/deployment/deployment.controller.ts
- apps/deploy-web/src/components/alerts/AlertsListContainer/AlertsListContainer.tsx
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: validate-deploy-web
- GitHub Check: test-deploy-web-build
- GitHub Check: validate-notifications
- GitHub Check: validate-api
- GitHub Check: test-api-build
🔇 Additional comments (24)
apps/notifications/src/interfaces/rest/interceptors/auth/auth.interceptor.ts (1)
27-29: LGTM: Enhanced security with granular permissions.The authorization refinement from blanket "manage" permissions to specific actions is a security improvement. The restricted "update" permission on only the "enabled" field aligns well with the UI toggle functionality mentioned in the summary.
Please ensure that all existing alert operations across the application are compatible with these refined permissions, particularly any code that previously relied on the broader "manage" permission.
#!/bin/bash # Description: Verify that all alert operations use the correct permissions # Expected: All alert operations should use read/delete/update permissions appropriately # Search for alert repository usage to verify permission usage rg -A 5 -B 5 "accessibleBy.*Alert" --type tsapps/notifications/src/modules/alert/services/deployment-alert/deployment-alert.service.spec.ts (2)
10-10: LGTM: Consistent test updates for specialized repository.The test file is correctly updated to use the new
DeploymentAlertRepositoryinstead of the genericAlertRepository.
199-201: LGTM: Test setup properly updated.The test module configuration and setup are correctly updated to use the specialized repository, maintaining consistency with the service implementation changes.
apps/notifications/src/modules/alert/services/deployment-alert/deployment-alert.service.ts (2)
7-8: LGTM: Imports updated for specialized repository.The import statements are correctly updated to use the specialized
DeploymentAlertRepositoryinstead of the genericAlertRepository.
72-72: LGTM: Constructor dependency updated.The constructor parameter type is correctly updated to use the specialized repository, maintaining consistency with the import changes.
apps/notifications/src/modules/alert/repositories/alert/deployment-alert.repository.ts (1)
1-11: LGTM: Clean specialization of repository for deployment alerts.The
DeploymentAlertRepositorycorrectly extends the baseAlertRepositoryand overrides theaccessibleBymethod to use "DeploymentAlert" as the authorization subject. This enables separate authorization rules for deployment alerts while inheriting all the base functionality.The implementation is appropriately simple and focused on its specific purpose.
apps/notifications/test/functional/alert-crud.spec.ts (2)
15-15: LGTM: Import added for direct repository access.The
AlertRepositoryimport is correctly added to support the newprepareAlertfunction that bypasses HTTP API creation.
36-45: LGTM: Clean implementation of repository-based alert creation.The
prepareAlertfunction provides a clean workaround for creating alerts directly via repository when API creation is not permitted. The implementation correctly uses the repository to create an alert with the necessary test data.apps/deploy-web/src/components/alerts/AlertsListView/AlertsListView.spec.tsx (5)
8-9: Import updates are correctThe type import changes properly reflect the renamed interface and the addition of
useFlagtype is necessary for type-safe mocking.
41-41: Test assertion correctly reflects UI changesThe test now properly checks for the displayed "Threshold" text instead of the raw alert type, matching the component's updated rendering logic.
44-46: Checkbox assertions properly test the new toggle functionalityThe tests correctly validate the checkbox element and its checked state, using accessible role-based queries which is a testing best practice.
Also applies to: 64-66
68-68: Assertion matches the simplified DSEQ column displayThe test correctly expects "N/A" for alerts without DSEQ parameters.
91-127: Setup function properly reflects component changesThe updated setup function correctly:
- Uses the new Props interface with
onToggleandloadingIds- Implements a comprehensive mock for the
useFlaghook- Uses dependency injection pattern for better testability
apps/deploy-web/src/components/alerts/AlertsListView/AlertsListView.tsx (6)
4-4: Well-structured dependency injection patternThe imports and DEPENDENCIES constant properly implement dependency injection, improving testability.
Also applies to: 10-10, 16-19
20-29: Props interface properly updated for new functionalityThe interface changes correctly support the toggle feature with appropriate typing for all new properties.
45-62: Checkbox column implementation is robustThe enabled column properly:
- Handles loading states to prevent race conditions
- Includes accessibility label
- Correctly converts checked value to boolean
64-73: Nice UX improvement for deployment linksThe conditional link rendering provides better navigation when DSEQ is available.
112-114: Feature flag properly controls column visibilityThe enabled column is correctly hidden when the update feature is disabled.
160-160: Table styling improvements enhance consistencyThe fixed heights and padding provide a more polished table appearance.
Also applies to: 170-170
apps/notifications/src/modules/alert/repositories/alert/alert.repository.ts (5)
1-2: Imports correctly support field-level authorizationAll new imports are necessary for the authorization implementation, including the corrected value import for
AnyAbility.Also applies to: 4-4, 8-8
19-19: Type and property additions support authorization stateThe
AbilityParamsWithSubjecttype andabilityParamsproperty properly maintain the authorization context.Also applies to: 74-75
82-82: Authorization setup properly handles subject contextThe methods correctly propagate the subject through the authorization chain, enabling granular permission control.
Also applies to: 85-89
78-78: Protected visibility enables proper inheritanceMaking
dbprotected correctly supports repository inheritance patterns.
105-114: Verify the default field permission behaviorThe field-level authorization check is well-implemented. However, the fallback to all Alert fields when
rule.fieldsis undefined could be overly permissive.#!/bin/bash # Description: Check how field permissions are defined in the auth interceptor # Search for Alert permission definitions to understand field specifications rg -A 10 "subject.*Alert|can.*Alert" --type ts # Look for ability rule definitions that might specify fields ast-grep --pattern 'can($_, "Alert", { fields: [$$$] })'
ygrishajev
force-pushed
the
feature/alerts-list
branch
from
aef58d9 to
244eada
Compare
2 participants
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Refactor
Chores