fix: remove misspelled selector default and notify on monitor downgrade#259
Closed
fix: remove misspelled selector default and notify on monitor downgrade#259
Conversation
…owngrade (#254, #253) Clear the bogus "name of the selctor" default value from the selector field so users see the placeholder instead. Add a tier-downgrade email that lists which monitors were switched from hourly to daily when a subscription is cancelled or downgraded. Closes #254 Closes #253 https://claude.ai/code/session_01EaiPVsbd2waiYqem5rzKSY
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR fixes a misspelled CSS selector placeholder in CreateMonitorDialog and adds email notifications when users' hourly monitors are downgraded during subscription tier changes. The storage layer return type is updated to include monitor names alongside downgrade counts, enabling the email service to notify users of affected monitors. Changes
Sequence DiagramsequenceDiagram
participant Stripe as Stripe Webhook
participant Handler as webhookHandlers
participant Storage as DatabaseStorage
participant Email as emailService
participant Resend as Resend API
Stripe->>Handler: Subscription downgrade event
Handler->>Storage: downgradeHourlyMonitors(userId)
Storage->>Storage: Query hourly monitors
Storage-->>Handler: { count, monitorNames }
alt count > 0
Handler->>Email: sendTierDowngradeEmail(userId, monitorNames)
Email->>Email: Check canSendEmail()
Email->>Email: Lookup user & email
Email->>Email: Build HTML/text content
Email->>Resend: Send email
Resend-->>Email: Response
Email-->>Handler: { success, id } or { success: false, error }
Handler->>Handler: Log email result
end
Handler-->>Stripe: Webhook processed
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/services/email.ts`:
- Around line 437-442: The current check rejects when user.email is missing even
though user.notificationEmail may be present; change the validation to compute
recipientEmail = (user.notificationEmail || user.email) first and then validate
that recipientEmail exists and is non-empty (e.g., non-null/trimmed) before
proceeding (update the early return in server/services/email.ts accordingly),
and update the error message to say "User has no recipient email" or similar;
keep the monitorList creation using sanitizePlainText as-is.
In `@server/webhookHandlers.test.ts`:
- Around line 29-35: Add two Vitest test cases in webhookHandlers.test.ts that
exercise the mocked downgrade flow: (1) set the mock for downgradeHourlyMonitors
to resolve { count: N, monitorNames: [...] } with N>0 and assert that
sendTierDowngradeEmail was called once with the expected monitorNames (and any
other expected args); (2) set downgradeHourlyMonitors to resolve { count: 0,
monitorNames: [] } and assert that sendTierDowngradeEmail was not called. Use
the existing vi.mock hooks for downgradeHourlyMonitors and
sendTierDowngradeEmail, reset mocks between tests (vi.clearAllMocks or similar),
and call the webhook handler function that triggers these services so the
assertions verify the behavior.
In `@server/webhookHandlers.ts`:
- Around line 107-108: The call sites that await sendTierDowngradeEmail (e.g.,
the call using user.id and monitorNames around the Stripe handling and the two
other sites) only handle rejections but not a returned { success: false };
update each site to await the result into a variable (e.g., const res = await
sendTierDowngradeEmail(...)) and explicitly check res.success — if false, log an
error including the user id, monitorNames and any res.error/message; keep the
existing .catch for thrown errors or convert to try/catch and log thrown errors
similarly so both failure modes are recorded.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4004753c-db4b-4927-bb50-0267597e4f5e
📒 Files selected for processing (6)
client/src/components/CreateMonitorDialog.tsxserver/services/email.tsserver/storage.test.tsserver/storage.tsserver/webhookHandlers.test.tsserver/webhookHandlers.ts
server/services/email.ts
Outdated
Comment on lines
+437
to
+442
| if (!user || !user.email) { | ||
| return { success: false, error: "User has no email address" }; | ||
| } | ||
|
|
||
| const recipientEmail = user.notificationEmail || user.email; | ||
| const monitorList = monitorNames.map(n => ` • ${sanitizePlainText(n)}`).join("\n"); |
There was a problem hiding this comment.
Recipient validation is too strict and can drop valid notification emails.
This branch rejects users without user.email even when user.notificationEmail is set. Validate the resolved recipient instead.
Proposed fix
- const user = await authStorage.getUser(userId);
- if (!user || !user.email) {
- return { success: false, error: "User has no email address" };
- }
-
- const recipientEmail = user.notificationEmail || user.email;
+ const user = await authStorage.getUser(userId);
+ if (!user) {
+ return { success: false, error: "User not found" };
+ }
+
+ const recipientEmail = user.notificationEmail || user.email;
+ if (!recipientEmail) {
+ return { success: false, error: "User has no email address" };
+ }📝 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.
Suggested change
| if (!user || !user.email) { | |
| return { success: false, error: "User has no email address" }; | |
| } | |
| const recipientEmail = user.notificationEmail || user.email; | |
| const monitorList = monitorNames.map(n => ` • ${sanitizePlainText(n)}`).join("\n"); | |
| const user = await authStorage.getUser(userId); | |
| if (!user) { | |
| return { success: false, error: "User not found" }; | |
| } | |
| const recipientEmail = user.notificationEmail || user.email; | |
| if (!recipientEmail) { | |
| return { success: false, error: "User has no email address" }; | |
| } | |
| const monitorList = monitorNames.map(n => ` • ${sanitizePlainText(n)}`).join("\n"); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/services/email.ts` around lines 437 - 442, The current check rejects
when user.email is missing even though user.notificationEmail may be present;
change the validation to compute recipientEmail = (user.notificationEmail ||
user.email) first and then validate that recipientEmail exists and is non-empty
(e.g., non-null/trimmed) before proceeding (update the early return in
server/services/email.ts accordingly), and update the error message to say "User
has no recipient email" or similar; keep the monitorList creation using
sanitizePlainText as-is.
Comment on lines
+107
to
+108
| await sendTierDowngradeEmail(user.id, monitorNames).catch(err => | ||
| console.error(`[Stripe] Failed to send downgrade email for user ${user.id}:`, err)); |
There was a problem hiding this comment.
Unsuccessful downgrade-email results are currently silent.
sendTierDowngradeEmail can return { success: false } without throwing, but these call sites only handle rejected promises. Log/handle unsuccessful results explicitly.
Proposed fix pattern (apply to all three call sites)
- await sendTierDowngradeEmail(user.id, monitorNames).catch(err =>
- console.error(`[Stripe] Failed to send downgrade email for user ${user.id}:`, err));
+ try {
+ const emailResult = await sendTierDowngradeEmail(user.id, monitorNames);
+ if (!emailResult.success) {
+ console.error(
+ `[Stripe] Failed to send downgrade email for user ${user.id}: ${emailResult.error ?? "unknown error"}`
+ );
+ }
+ } catch (err) {
+ console.error(`[Stripe] Failed to send downgrade email for user ${user.id}:`, err);
+ }Also applies to: 153-154, 181-182
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/webhookHandlers.ts` around lines 107 - 108, The call sites that await
sendTierDowngradeEmail (e.g., the call using user.id and monitorNames around the
Stripe handling and the two other sites) only handle rejections but not a
returned { success: false }; update each site to await the result into a
variable (e.g., const res = await sendTierDowngradeEmail(...)) and explicitly
check res.success — if false, log an error including the user id, monitorNames
and any res.error/message; keep the existing .catch for thrown errors or convert
to try/catch and log thrown errors similarly so both failure modes are recorded.
… error handling - Add recordUsage() calls to sendTierDowngradeEmail for proper Resend cap tracking - Add early return for empty monitorNames array - Use fallback "(unnamed monitor)" for empty monitor names in email body - Use String(error?.message ?? error) for safer error extraction - Add test for empty monitorNames, downgrade email send, and no-send scenarios https://claude.ai/code/session_01EaiPVsbd2waiYqem5rzKSY
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
"name of the selctor"default value from the CSS Selector field inCreateMonitorDialog. The field already has a proper placeholder (.price-tag or #main-content), so users now see that instead of a bogus pre-filled value.sendTierDowngradeEmail()to notify users when their monitors are switched from hourly to daily frequency due to a subscription cancellation or tier downgrade. UpdateddowngradeHourlyMonitors()to return monitor names alongside the count, and wired the email into all three webhook handler paths.Closes #254
Closes #253
Test plan
npm run checkpasses (TypeScript)npm run testpasses (1727 tests, 63 suites)https://claude.ai/code/session_01EaiPVsbd2waiYqem5rzKSY
Summary by CodeRabbit
New Features
Bug Fixes