OUT-3929 | Gate IU emails via platform notification settings - #1377
OUT-3929 | Gate IU emails via platform notification settings#1377arpandhakal wants to merge 2 commits into
Conversation
Implements OUT-3929. Instead of the app deciding IU email delivery, we now pass
the Assembly-declared notificationSettingId on IU email sends and let the
platform enforce each IU's per-surface preference (suppressed surfaces are
dropped silently; omitting the id preserves old behavior).
- add notificationSettingId to the notification payload schema
- CopilotAPI.getNotificationSettings(): resolve our install via listAppInstalls
(installs aren't in the token), fetch installs/{id}/notification-settings
- resolveTasksNotificationSettingId(): single-setting model for now, caches the
stable setting id per workspace (never the mutable IU preference)
- thread the id onto IU email sends: create(), createBulkNotification(), the
grouped flush summary, and the reply job. Stored on the buffered email so both
the single-event replay and the grouped summary carry it. The immediate
in-product dispatch is intentionally left ungated to protect the notification
center + InternalUserNotification tracking.
- keep isIuEmailEnabled() as the app-side rollout kill switch; when on, IU emails
now ride the platform preference. If no setting is declared, the id is omitted
and delivery falls back to today's behavior.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Greptile SummaryImplements OUT-3929: IU email notifications now carry a
Confidence Score: 3/5The settings-fetch is now on the critical path for IU email buffering — a new external dependency that didn't exist before this PR. Adding a resilience fallback before merge is recommended. The core concern is that src/app/api/notification/notification.service.ts — both Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant NS as NotificationService
participant RSN as resolveTasksNotificationSettingId
participant Cache as Module Cache
participant CAPI as CopilotAPI
participant Platform as Assembly Platform
NS->>RSN: "resolve({copilot, workspaceId})"
RSN->>Cache: get(workspaceId)
alt cache hit (within TTL)
Cache-->>RSN: "{id, expiresAt}"
RSN-->>NS: settingId (or undefined)
else cache miss
RSN->>CAPI: getNotificationSettings()
CAPI->>Platform: listAppInstalls()
Platform-->>CAPI: installs[]
CAPI->>Platform: "GET /v1/installs/{id}/notification-settings"
Platform-->>CAPI: "{notifications:[{id,label,surfaces}]}"
CAPI-->>RSN: NotificationSettingsResponse
RSN->>Cache: "set(workspaceId, {id, expiresAt})"
RSN-->>NS: settingId (or undefined)
end
NS->>NS: bufferGroupedEmailEvent(...notificationSettingId)
Note over NS: In-product dispatch omits notificationSettingId
NS->>Platform: sendGroupedEmail / createNotification
Platform-->>NS: suppress or deliver per IU preference
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant NS as NotificationService
participant RSN as resolveTasksNotificationSettingId
participant Cache as Module Cache
participant CAPI as CopilotAPI
participant Platform as Assembly Platform
NS->>RSN: "resolve({copilot, workspaceId})"
RSN->>Cache: get(workspaceId)
alt cache hit (within TTL)
Cache-->>RSN: "{id, expiresAt}"
RSN-->>NS: settingId (or undefined)
else cache miss
RSN->>CAPI: getNotificationSettings()
CAPI->>Platform: listAppInstalls()
Platform-->>CAPI: installs[]
CAPI->>Platform: "GET /v1/installs/{id}/notification-settings"
Platform-->>CAPI: "{notifications:[{id,label,surfaces}]}"
CAPI-->>RSN: NotificationSettingsResponse
RSN->>Cache: "set(workspaceId, {id, expiresAt})"
RSN-->>NS: settingId (or undefined)
end
NS->>NS: bufferGroupedEmailEvent(...notificationSettingId)
Note over NS: In-product dispatch omits notificationSettingId
NS->>Platform: sendGroupedEmail / createNotification
Platform-->>NS: suppress or deliver per IU preference
Reviews (1): Last reviewed commit: "feat(notifications): gate IU emails via ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // IU emails carry the Tasks notification setting so the platform enforces the IU's | ||
| // per-surface preference. Stored on the buffered email, so both the grouped summary and | ||
| // the single-event replay dispatch it. |
There was a problem hiding this comment.
resolveTasksNotificationSettingId is awaited before bufferGroupedEmailEvent. If getNotificationSettings() fails after all retries — e.g., a transient 5xx that lasts longer than the retry window, an unexpected 404 from the settings endpoint, or a Zod parse error on a new platform response shape — the entire buffering step is skipped and the IU loses the email silently with no retry opportunity. Pre-PR this path was unconditional.
Wrapping with a try/catch and falling back to undefined on failure restores the pre-OUT-3929 behaviour (email delivered, just without platform-side suppression) and keeps the guard non-blocking on API instability.
There was a problem hiding this comment.
Fixed in 1e8ee73. resolveTasksNotificationSettingId now catches getNotificationSettings failures and returns undefined instead of throwing, so a failure no longer skips buffering + the in-product dispatch. Undefined id = platform delivers all (fail-open); the failure isn't cached, so the next send retries and it self-heals. Note: a separate requirement update means the setting id now also rides the in-product surface — which makes fail-open the only coherent failure mode (fail-closed would mean dropping the whole notification during a settings outage).
| senderCompanyId: sender?.senderCompanyId, | ||
| recipientInternalUserId: group.recipientIuId, | ||
| // All IU rows in a window share the single Tasks setting; read it off any buffered email. | ||
| notificationSettingId: liveEvents.map((e) => e.individualEmail?.notificationSettingId).find(Boolean) ?? undefined, |
There was a problem hiding this comment.
?? undefined is redundant — .find(Boolean) already returns undefined when no element matches, so the nullish-coalescing branch is never reached.
| notificationSettingId: liveEvents.map((e) => e.individualEmail?.notificationSettingId).find(Boolean) ?? undefined, | |
| notificationSettingId: liveEvents.map((e) => e.individualEmail?.notificationSettingId).find(Boolean), |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Done — dropped the redundant ?? undefined in 1e8ee73.
| async _getNotificationSettings(): Promise<NotificationSettingsResponse> { | ||
| const appId = z.string({ message: 'Missing AppID in environment' }).parse(APP_ID) |
There was a problem hiding this comment.
_* method in this class opens with a console.info log, but _getNotificationSettings skips it. This makes the method invisible in production traces when debugging a mis-delivered or unexpectedly-suppressed IU email.
| async _getNotificationSettings(): Promise<NotificationSettingsResponse> { | |
| const appId = z.string({ message: 'Missing AppID in environment' }).parse(APP_ID) | |
| async _getNotificationSettings(): Promise<NotificationSettingsResponse> { | |
| console.info('CopilotAPI#_getNotificationSettings', this.token) | |
| const appId = z.string({ message: 'Missing AppID in environment' }).parse(APP_ID) |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Done — added the console.info trace to _getNotificationSettings in 1e8ee73.
Two follow-ups on OUT-3929:
1. Gate the in-product surface as well (per updated requirement). The
notificationSettingId is now attached to the immediate in-product IU dispatch
in create()/createBulkNotification, not just the buffered email, so the
platform gates both surfaces against the IU's preference. We don't gate
ourselves; we just pass the id.
2. Address Greptile review:
- P1: resolveTasksNotificationSettingId catches getNotificationSettings
failures and returns undefined instead of throwing. A failure no longer
skips buffering + the in-product dispatch (which silently dropped the whole
notification). Undefined id = platform delivers all (fail-open); the failure
is not cached so the next send retries. With in-product now gated too,
fail-open is the coherent choice - failing closed would mean dropping the
entire notification during a settings-API outage.
- P2: drop redundant `?? undefined` after `.find(Boolean)` in the flush.
- P2: add the standard console.info trace to _getNotificationSettings.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Deployment failed with the following error: Learn More: https://vercel.link/multiple-function-regions |
1 participant
Summary
Implements OUT-3929. The Assembly platform now ships a real IU notification-preference mechanism (per the handoff doc), which is quite different from the anticipated approach: instead of the app deciding IU email delivery via a local flag, the app passes a platform-declared
notificationSettingIdon each IU send and the platform enforces the IU's per-surface preference. Suppressed surfaces are dropped silently; omitting the id preserves pre-OUT-3929 behavior.Stacked on #1376 (OUT-3927) — base this PR on
OUT-3927-send-comment-emails-to-ius.What's here
notificationSettingIdadded toNotificationRequestBodySchema.CopilotAPI.getNotificationSettings(): resolves our install vialistAppInstalls()(installs aren't in the token, so match byappId), then fetchesGET /v1/installs/{installId}/notification-settings.resolveTasksNotificationSettingId(): single-setting model for now — returns the sole declared setting's id, orundefinedwhen none is declared (→ safe fallback, no suppression). Caches only the stable setting id per workspace with a short TTL; the mutable IU preference is never cached (it's evaluated by the platform on every send).create(),createBulkNotification(), the grouped-flush summary, and the reply job. Stored on the buffered email so both the single-event replay and the grouped summary carry it.isIuEmailEnabled()stays as the app-side rollout gate (product decision); when on, IU emails now ride the platform preference. Comment inconfigupdated to reflect its new role.Scoping decisions (please review)
InternalUserNotificationand drive the notification center, so letting the platform silently drop them risks phantom/orphaned rows. Consequence: the Product toggle on the IU settings page does not yet affect Tasks in-product notifications. If we want that too, it's a follow-up (or declare the setting email-only in the dashboard).Test plan
yarn tsccleansendGroupedEmailpassthrough🤖 Generated with Claude Code