feat: always show alerts tab and lock email alerts when keys are missing #1478
Conversation
WalkthroughThe changes in this pull request involve modifications to several components in the web application related to alert features. The "Alerts" menu item in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SideMenu
participant Features
participant AlertsPage
User->>SideMenu: Open Menu
SideMenu->>User: Display "Alerts" Item
User->>AlertsPage: Navigate to Alerts
AlertsPage->>Features: Check emailAlertsEnabled
Features-->>AlertsPage: Return emailAlertsEnabled
AlertsPage->>User: Show Email Input or Warning
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
apps/webapp/app/components/navigation/SideMenu.tsx(node:42229) ESLintIgnoreWarning: The ".eslintignore" file is no longer supported. Switch to using the "ignores" property in "eslint.config.js": https://eslint.org/docs/latest/use/configure/migration-guide#ignoring-files Oops! Something went wrong! :( ESLint: 9.15.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![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
🧹 Outside diff range and nitpick comments (1)
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.alerts.new/route.tsx (1)
291-294: Enhance the warning message with specific configuration details.The warning message could be more helpful by specifying which configuration is missing (FROM email and Resend API key).
Consider updating the warning message to be more specific:
- Email integration is not available. Please contact your organization - administrator. + Email integration is not available because the FROM email or Resend API key + is not configured. Please contact your organization administrator to set up + the email configuration.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
apps/webapp/app/components/navigation/SideMenu.tsx(1 hunks)apps/webapp/app/features.server.ts(0 hunks)apps/webapp/app/hooks/useFeatures.ts(1 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.alerts.new/route.tsx(4 hunks)
💤 Files with no reviewable changes (1)
- apps/webapp/app/features.server.ts
🔇 Additional comments (4)
apps/webapp/app/hooks/useFeatures.ts (2)
8-8: LGTM! Change aligns with PR objectives.
The removal of alertsEnabled from the default return value is consistent with the PR's goal of removing this feature flag and making alerts always visible.
Line range hint 1-9: Verify dependent components and type updates.
Let's ensure this change is consistent across the codebase.
✅ Verification successful
Based on the shell script results, I can generate the final response as we have sufficient information:
Changes are consistent with feature flag usage across the codebase
The verification shows:
TriggerFeaturestype infeatures.server.tsonly includesisManagedCloudandv3Enabled- No remaining references to
alertsEnabledwere found - All components using
useFeatures()hook only accessisManagedCloudandv3Enabledflags
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the removal of alertsEnabled across the codebase
# Check if TriggerFeatures type has been updated
echo "Checking TriggerFeatures type definition:"
rg -A 5 "type TriggerFeatures" apps/webapp/app/features.server.ts
# Find any remaining references to alertsEnabled that might need updating
echo "\nChecking for any remaining references to alertsEnabled:"
rg "alertsEnabled" apps/webapp/
# Find components using useFeatures hook to verify they handle the new return type
echo "\nFinding components using useFeatures hook:"
rg "useFeatures\(\)" apps/webapp/
Length of output: 1689
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.alerts.new/route.tsx (1)
26-26: LGTM! Server-side environment variable check is implemented correctly.
The implementation securely checks for required email configuration variables (ALERT_FROM_EMAIL and ALERT_RESEND_API_KEY) in the server-side loader function, following Remix's best practices.
Also applies to: 154-156, 160-160
apps/webapp/app/components/navigation/SideMenu.tsx (1)
686-692: LGTM! Implementation follows established patterns.
The Alerts menu item is well-implemented, following the consistent pattern of other menu items in the V3ProjectSideMenu component.
Let's verify that the alerts route is properly configured:
✅ Verification successful
Route is properly configured and actively used
The verification shows that v3ProjectAlertsPath is:
- Correctly defined in
pathBuilder.tswith proper organization and project parameters - Used in multiple route handlers:
alerts/route.tsxfor the main alerts viewalerts.new/route.tsxfor creating new alerts
- Actively used for navigation and redirects in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the v3ProjectAlertsPath route exists and is properly configured
# Check for the route definition
rg -A 5 "v3ProjectAlertsPath"
# Check for any route handlers or components related to alerts
ast-grep --pattern 'export function $_(organization, project) {
$$$
return $$$alerts$$$;
}'
Length of output: 8381
|
I was worried there might be conflicts with #1474, but it looks like we're changing very different parts of the same file. So we should be ok! |
When I was testing my self hosted installation, I noticed that the alerts tab was missing. It was still accessible if I manually changed the url, and the webhook alerts were fully functional. This PR removes the feature flag
alertsEnabledand makes the alerts page always visible. Similarly to the Slack alert, it'll now show a notice to the user if they don't have their resend configured for alerts.It has the same behaviour as before for checking whether email alerts are enabled. It uses the two environment variables. I've never used Remix before so let me know if it's not safe to check those there. I think it is as I did see there is other server code in that file, and the remix docs say that loaders run on the server.
If there is interest, I could also update the self hosting docs with how to configure email alerts in a follow up pr?
✅ Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
alertsEnabledfeature.