Disclose partner email notifications in confirmation flows - #4080
Merged
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughThe PR updates modal copy across partner and bounty flows to mention email notifications and plural partner wording. It also adds a reward confirmation modal and routes reward create, update, and delete actions through it. ChangesPartner notification copy and reward confirmation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/ui/modals/bulk-ban-partners-modal.tsx`:
- Around line 201-204: The bulk-ban modal copy in bulk-ban-partners-modal.tsx
hardcodes the plural notification text, so it reads wrong when only one partner
is selected. Update the paragraph that builds the message around partnerWord to
switch the final sentence between singular and plural wording based on the
selected partner count, so the text is grammatically correct for exactly one
partner and for multiple partners.
In `@apps/web/ui/modals/bulk-deactivate-partners-modal.tsx`:
- Around line 148-154: Update the notification copy in
bulk-deactivate-partners-modal.tsx so it only promises emails to partners who
are actually eligible to receive them. In the JSX that renders the bulk
deactivation message, adjust the `partners.length === 1` branch and the
pluralized branch to reflect “eligible partners” or “partners with user
accounts” instead of saying all selected partners will be notified. Keep the
wording aligned with the deactivation email job behavior and the identifiers
`partners`, `partnerWord`, and the bulk-deactivation modal content.
In `@apps/web/ui/modals/deactivate-partner-modal.tsx`:
- Around line 100-103: Update the confirmation copy in DeactivatePartnerModal so
it does not promise an email unconditionally. The text in the modal should be
phrased conditionally, matching the deactivation mailer behavior that only
queues a notification when the partner has an associated user account. Use the
DeactivatePartnerModal content block as the reference point and adjust the
sentence to reflect “if applicable” or equivalent.
In `@apps/web/ui/partners/rewards/add-edit-reward-sheet.tsx`:
- Around line 208-213: The confirmation flow is treating an unloaded or stale
`partnersCount` from usePartnersCount as 0, which can mislead the disclosure
copy. Update add-edit-reward-sheet.tsx so the reward confirmation only uses a
group-specific count once group.id is available and the hook has resolved the
current value; otherwise keep the count as unknown/undefined and avoid coercing
it to 0 in the openConfirmRewardChangeModal path and the
ConfirmRewardChangeModal copy. Use the usePartnersCount and
openConfirmRewardChangeModal/ConfirmRewardChangeModal symbols to locate the
affected state and confirmation wiring.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0c998f24-e4fd-4306-b2e4-0ca0874c9750
📒 Files selected for processing (11)
apps/web/ui/modals/ban-partner-modal.tsxapps/web/ui/modals/bulk-ban-partners-modal.tsxapps/web/ui/modals/bulk-deactivate-partners-modal.tsxapps/web/ui/modals/bulk-reject-partners-modal.tsxapps/web/ui/modals/change-group-modal.tsxapps/web/ui/modals/confirm-approve-bounty-submission-modal.tsxapps/web/ui/modals/confirm-reward-change-modal.tsxapps/web/ui/modals/deactivate-partner-modal.tsxapps/web/ui/modals/delete-group-modal.tsxapps/web/ui/partners/bounties/reject-bounty-submission-modal.tsxapps/web/ui/partners/rewards/add-edit-reward-sheet.tsx
Collaborator
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Contributor
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 (1)
apps/web/ui/modals/confirm-approve-bounty-submission-modal.tsx (1)
81-92: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winOnly promise an email when one exists.
approveBountySubmissiononly sends the notification whenpartner.emailis present, so this sentence is false for submissions without an email address. Gate the copy onsubmission.partner.emailor soften it.Suggested fix
- and notify them by email. + {submission.partner.email + ? " and notify them by email." + : "."}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/ui/modals/confirm-approve-bounty-submission-modal.tsx` around lines 81 - 92, The confirmation copy in confirm-approve-bounty-submission-modal should not always say the partner will be notified by email, because approveBountySubmission only sends that email when submission.partner.email is present. Update the modal text around the commission/notification message to either conditionally include the email promise based on submission.partner.email or soften it to a non-specific notification statement, using the existing submission.partner and approveBountySubmission flow as the reference points.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/ui/modals/ban-partner-modal.tsx`:
- Around line 117-120: Make the ban confirmation text in BanPartnerModal
conditional on email availability, since the ban flow in the partner ban route
only sends the notification when a partner email exists. Update the copy in
ban-partner-modal to avoid promising an email for every partner, and use the
same partner/email check logic that the ban API uses so the UI message matches
the actual behavior.
In `@apps/web/ui/modals/bulk-ban-partners-modal.tsx`:
- Around line 204-207: The notification copy in bulk-ban-partners-modal.tsx
currently assumes every selected partner will receive email, but only partners
with an email address can actually be notified. Update the text in the bulk-ban
modal branch that uses partners.length and pluralize/nFormatter to count only
the partners that have an email, and keep the wording aligned with the
single-partner card’s optional email handling.
---
Outside diff comments:
In `@apps/web/ui/modals/confirm-approve-bounty-submission-modal.tsx`:
- Around line 81-92: The confirmation copy in
confirm-approve-bounty-submission-modal should not always say the partner will
be notified by email, because approveBountySubmission only sends that email when
submission.partner.email is present. Update the modal text around the
commission/notification message to either conditionally include the email
promise based on submission.partner.email or soften it to a non-specific
notification statement, using the existing submission.partner and
approveBountySubmission flow as the reference points.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: cff1ecb1-9a1d-4ca0-9b63-314e35c2c38b
📒 Files selected for processing (12)
apps/web/lib/swr/use-partners-count.tsapps/web/ui/modals/ban-partner-modal.tsxapps/web/ui/modals/bulk-ban-partners-modal.tsxapps/web/ui/modals/bulk-deactivate-partners-modal.tsxapps/web/ui/modals/bulk-reject-partners-modal.tsxapps/web/ui/modals/change-group-modal.tsxapps/web/ui/modals/confirm-approve-bounty-submission-modal.tsxapps/web/ui/modals/confirm-reward-change-modal.tsxapps/web/ui/modals/deactivate-partner-modal.tsxapps/web/ui/modals/delete-group-modal.tsxapps/web/ui/partners/bounties/reject-bounty-submission-modal.tsxapps/web/ui/partners/rewards/add-edit-reward-sheet.tsx
Comment on lines
+204
to
+207
| reversible.{" "} | ||
| {partners.length === 1 | ||
| ? "This partner will be notified by email." | ||
| : `${nFormatter(partners.length, { full: true })} ${pluralize("partner", partners.length)} will be notified by email.`} |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Count only partners who can actually receive email.
The ban cron path only sends mail when a partner has an email address, but this branch says all selected partners will be notified. In this same modal, the single-partner card already treats email as optional, so that mismatch is reachable.
Suggested fix
+ {(() => {
+ const emailableCount = partners.filter((p) => p.email).length;
+ if (emailableCount === 0) return "No notification email will be sent.";
+ if (emailableCount === 1) return "1 partner will be notified by email.";
+ return `${nFormatter(emailableCount, { full: true })} ${pluralize("partner", emailableCount)} will be notified by email.`;
+ })()}
- {partners.length === 1
- ? "This partner will be notified by email."
- : `${nFormatter(partners.length, { full: true })} ${pluralize("partner", partners.length)} will be notified by email.`}📝 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
| reversible.{" "} | |
| {partners.length === 1 | |
| ? "This partner will be notified by email." | |
| : `${nFormatter(partners.length, { full: true })} ${pluralize("partner", partners.length)} will be notified by email.`} | |
| reversible.{" "} | |
| {(() => { | |
| const emailableCount = partners.filter((p) => p.email).length; | |
| if (emailableCount === 0) return "No notification email will be sent."; | |
| if (emailableCount === 1) return "1 partner will be notified by email."; | |
| return `${nFormatter(emailableCount, { full: true })} ${pluralize("partner", emailableCount)} will be notified by email.`; | |
| })()} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/ui/modals/bulk-ban-partners-modal.tsx` around lines 204 - 207, The
notification copy in bulk-ban-partners-modal.tsx currently assumes every
selected partner will receive email, but only partners with an email address can
actually be notified. Update the text in the bulk-ban modal branch that uses
partners.length and pluralize/nFormatter to count only the partners that have an
email, and keep the wording aligned with the single-partner card’s optional
email handling.
steven-tey
approved these changes
Jun 28, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/ui/modals/bulk-reject-partners-modal.tsx (1)
72-77: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winBranch the notification sentence for the singular case.
When
partners.length === 1, this readsreject this application? They will be ..., which is grammatically off and makes it sound like the application is getting the email.Proposed fix
<p className="text-content-subtle text-base font-medium"> Are you sure you want to reject{" "} {pluralize("this application", partners.length, { plural: "these applications", })} - ? They will be <PartnerEmailNotificationTooltipHelper />. + ?{" "} + {partners.length === 1 ? "The partner will be " : "The partners will be "} + <PartnerEmailNotificationTooltipHelper />. </p>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/ui/modals/bulk-reject-partners-modal.tsx` around lines 72 - 77, The confirmation text in bulk-reject-partners-modal.tsx needs to branch on the singular case so the notification sentence reads correctly when only one partner is being rejected. Update the JSX around the pluralize usage in the modal copy so the message for partners.length === 1 uses singular wording and does not follow “reject this application?” with “They will be ...”. Keep the existing plural form for multiple partners and adjust the surrounding sentence to reference the right subject in each case.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/ui/shared/partner-email-notification-tooltip-helper.tsx`:
- Around line 9-12: The tooltip trigger in
partner-email-notification-tooltip-helper is a plain span, so it is not
keyboard-focusable. Update the trigger element in the helper component (the one
wrapping {text} inside Tooltip) to use a focusable control such as a button, or
add the missing tabIndex and keyboard/focus handling so the tooltip can be
reached and opened from the keyboard.
---
Outside diff comments:
In `@apps/web/ui/modals/bulk-reject-partners-modal.tsx`:
- Around line 72-77: The confirmation text in bulk-reject-partners-modal.tsx
needs to branch on the singular case so the notification sentence reads
correctly when only one partner is being rejected. Update the JSX around the
pluralize usage in the modal copy so the message for partners.length === 1 uses
singular wording and does not follow “reject this application?” with “They will
be ...”. Keep the existing plural form for multiple partners and adjust the
surrounding sentence to reference the right subject in each case.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 367ec8b8-c284-4cab-92bb-457a6af844be
📒 Files selected for processing (8)
apps/web/ui/modals/ban-partner-modal.tsxapps/web/ui/modals/bulk-ban-partners-modal.tsxapps/web/ui/modals/bulk-deactivate-partners-modal.tsxapps/web/ui/modals/bulk-reject-partners-modal.tsxapps/web/ui/modals/confirm-reward-change-modal.tsxapps/web/ui/modals/deactivate-partner-modal.tsxapps/web/ui/modals/delete-group-modal.tsxapps/web/ui/shared/partner-email-notification-tooltip-helper.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/web/ui/modals/deactivate-partner-modal.tsx
- apps/web/ui/modals/ban-partner-modal.tsx
- apps/web/ui/modals/delete-group-modal.tsx
- apps/web/ui/modals/confirm-reward-change-modal.tsx
Comment on lines
+9
to
+12
| <Tooltip content="Only partners who have previously created an account on [partners.dub.co](https://partners.dub.co) will receive an email notification."> | ||
| <span className="cursor-help underline decoration-dotted underline-offset-2"> | ||
| {text} | ||
| </span> |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
sed -n '1,40p' apps/web/ui/shared/partner-email-notification-tooltip-helper.tsx
sed -n '77,110p' packages/ui/src/tooltip.tsxRepository: dubinc/dub
Length of output: 1371
Make the tooltip trigger keyboard-focusable The helper renders a plain <span>, so the eligibility disclosure can’t be reached from the keyboard. Use a focusable element here (for example, a button) or add the missing focus/keyboard handling to the trigger.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/ui/shared/partner-email-notification-tooltip-helper.tsx` around
lines 9 - 12, The tooltip trigger in partner-email-notification-tooltip-helper
is a plain span, so it is not keyboard-focusable. Update the trigger element in
the helper component (the one wrapping {text} inside Tooltip) to use a focusable
control such as a button, or add the missing tabIndex and keyboard/focus
handling so the tooltip can be reached and opened from the keyboard.
This was referenced Aug 3, 2026
Merged
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 by CodeRabbit
New Features
Bug Fixes