OUT-3927 | Send comment & reply email notifications to IUs - #1376
Conversation
Implements OUT-3927. IUs now receive email (not just in-product) for new comments and thread replies, reusing the existing CU comment templates. - comment job: IU recipients now get email (gated on isIuEmailEnabled()) via the grouped buffer, consistent with CU comment emails - reply job: IU initiator branch now includes the email delivery target alongside inProduct, gated on isIuEmailEnabled() Removes the deferred routing landmine (was tracked for this ticket): the `?? !email` inference in buildNotificationDetails assumed "has email => client", which inverts once IUs get emails. Flipping the comment IU call to email:true would have routed IU comment emails to recipientClientId. Now: - createBulkNotification takes an explicit isRecipientIu opt; recipient type is declared by the caller (the same Commented action fans out to both CU and IU lists, so it can't be inferred from the action) - buildNotificationDetails uses isRecipientIu directly, no email-absence fallback - completion callers pass isRecipientIu: true explicitly Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| : undefined | ||
| // Recipient type is declared by the caller — the same action (e.g. Commented) fans out to | ||
| // both CU and IU recipient lists, so it can't be inferred from the action alone. | ||
| const isRecipientIu = opts?.isRecipientIu ?? false |
There was a problem hiding this comment.
Silent
false default is a future misrouting footgun
isRecipientIu now defaults to false rather than being inferred from the action. Every current IU-recipient caller was correctly updated, but a future call that dispatches to IU recipients and omits the opt will silently set recipientClientId to an IU ID in buildNotificationDetails — no compile-time warning, no runtime error, just a quietly misconfigured notification.
Consider making the default explicit in the JSDoc or using a required parameter (perhaps via a discriminated union for the opts type) so callers can't accidentally omit it when adding new IU-targeted bulk notifications.
There was a problem hiding this comment.
Valid — good catch. Fixed in a3893dc by taking the stronger option you suggested: isRecipientIu is now a required field on createBulkNotification's opts, so a future IU-recipient caller can't silently omit it — the compiler demands it. All existing CU callers now pass isRecipientIu: false explicitly (which is how tsc surfaced them), and IU callers pass true. This removes the silent-false-default footgun entirely.
There was a problem hiding this comment.
addressed. please check again. @greptile
There was a problem hiding this comment.
Confirmed — the fix is solid. isRecipientIu is now a required field on the opts type:
opts: {
isRecipientIu: boolean
email?: boolean
...
}And read directly as const isRecipientIu = opts.isRecipientIu with no default fallback. The JSDoc comment above the opts block also makes the requirement explicit for future callers. TypeScript will now enforce this at compile time — the footgun is fully closed. ✅
…quired Addresses Greptile review: defaulting isRecipientIu to false is a silent misrouting footgun — a future IU-recipient bulk caller that omits it would route to recipientClientId with no compile-time or runtime signal. Make isRecipientIu a required field on the opts object so every caller must declare recipient type. All existing CU callers now pass isRecipientIu: false explicitly; IU callers already pass true. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Deployment failed with the following error: Learn More: https://vercel.link/multiple-function-regions |
|
@greptile please review again |
Summary
Implements OUT-3927 — internal users now receive email (not just in-product) for new comments and thread replies, reusing the existing CU comment/reply templates. Stacked on the IU-email milestone branch.
send-comment-create-notifications.ts): IU recipients now get email, routed through the existing 5-minute grouped buffer (consistent with CU comment emails). Gated onisIuEmailEnabled()so it stays prod-safe until OUT-3929 ships the platform preference.send-reply-create-notifications.ts): the IU initiator branch now includes theemaildelivery target alongsideinProduct, also gated onisIuEmailEnabled().Commentedcopy is reused, per the ticket.Also resolves a deferred routing landmine
Flipping the comment IU call to
email: trueis exactly what would have tripped the old?? !emailinference inbuildNotificationDetails("has email ⇒ client"), which inverts once IUs get emails — it would have routed IU comment emails torecipientClientId(an IU id in a client field). Fixed at the root:createBulkNotificationnow takes an explicitisRecipientIuopt; recipient type is declared by the caller rather than inferred (the sameCommentedaction fans out to both CU and IU recipient lists, so it can't be derived from the action).buildNotificationDetailsusesisRecipientIudirectly — the email-absence fallback is gone.isRecipientIu: trueexplicitly.Note for reviewers
IU reply emails are immediate, not grouped — the reply job dispatches directly for both CU and IU recipients, so I preserved that behavior (CU replies weren't grouped either). Only the comment path is buffered. Say the word if IU reply emails should also be batched; that's a larger change.
Test plan
yarn tsccleanIU_EMAIL_ALWAYS_ENABLED=true: comment on a task → IUs with access get a grouped comment email; reply in a thread → prior IU participants get a reply email🤖 Generated with Claude Code