feat(chat): add push notification delivery endpoint and notify commands#96
Draft
dhairyashiil wants to merge 9 commits into
Draft
feat(chat): add push notification delivery endpoint and notify commands#96dhairyashiil wants to merge 9 commits into
dhairyashiil wants to merge 9 commits into
Conversation
- Add POST /api/notifications/deliver route with secret-based auth - Add ChatPushPayload type and buildPushCard() card formatter - Add per-identifier Slack and Telegram delivery modules - Add deliverNotifications() fan-out service using Promise.allSettled - Add /cal notify on|off Slack subcommand - Add /notify on|off Telegram command - Add 4 subscription management methods to Cal.com client - Add CALCOM_DELIVERY_SECRET env var validation
Contributor
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
Deployment failed with the following error: View Documentation: https://vercel.com/docs/accounts/team-members-and-roles |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Co-authored-by: devin-ai-integration[bot] <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
- Validate Slack teamId at runtime in parseDeliverRequest - Use structured SlackAPIError detection instead of string coercion - Strip comma from weekday in formatPushTime - Show reason for BOOKING_REJECTED (not just CANCELLED) - Validate meetingUrl and data.url as https before rendering - Extract DeliverResult to types.ts (no sibling imports) - Split service.ts map by platform branch (remove type cast) - Catch 404 on notify off for not-subscribed users - Pre-compute secret HMAC at module load - Guard env warning with NODE_ENV === development
- Reject /notify in Telegram groups with DM hint (#3) - Validate payload fields (title, timeZone, start, end, hosts, attendees) in parseDeliverRequest (#4) - Wrap deliverNotifications in try/catch, return 422 on formatter crash (#4) - Add logging across route, service, and delivery modules (#5) - Add /notify to Slack and Telegram help cards (#7) - Add error field to DeliverResult for backend retry/unsubscribe decisions (#9) - Change confirmation message from 'here' to 'via DM' (#10) - Validate timeZone with Intl.DateTimeFormat before processing (#11) - Add fallback badge for unknown notificationType (#12) - Reject /notify in groups with 'check DMs' message (#13)
- Show non-HTTP meetingUrl as plain text instead of falling through to location - Revert HMAC pre-computation — read secret per-request for rotation support - Use missing[] array for CALCOM_DELIVERY_SECRET in production (consistent with other vars) - Cap subscriptions array at 500 in parseDeliverRequest - Add 409 handling on notify on for already-subscribed (Slack + Telegram) - Add getLinkedUser check in Slack notify handler (consistent with other subcommands)
- fix(client): rename registerSlackSubscription input field deviceId → teamId
to match the DeliverRequest type and parseDeliverRequest validator; the
mismatch would have caused every Slack delivery request to be rejected
with 400
- fix(slack): update call site to pass { identifier, teamId } (shorthand)
instead of { identifier, deviceId: teamId }
- fix(route): use descriptive secretHmac/headerHmac variable names in
verifyDeliverySecret instead of opaque a/b
- fix(route): add notificationType non-empty string validation and
Date.parse validation for start/end in parseDeliverRequest; invalid
dates would otherwise silently produce "Invalid Date" in the push card
- fix(route): log warn on JSON parse failure path
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Co-authored-by: devin-ai-integration[bot] <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Pass the actual error object to console.error so the message, stack trace, and HTTP status are visible in logs — previously only a static string label was logged, making the 403 root cause invisible. Also add warn logs for registration failures in the hook and provider so any future auth or scope issues surface immediately.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 15 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/chat/lib/push-notifications/formatter.ts">
<violation number="1" location="apps/chat/lib/push-notifications/formatter.ts:31">
P2: `isHttpUrl` is too permissive for values that are interpolated into markdown/link fields. Use strict URL parsing so malformed or markdown-breaking URLs are rejected before building card links.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| }; | ||
|
|
||
| function isHttpUrl(url: string): boolean { | ||
| return /^https?:\/\//i.test(url); |
Contributor
There was a problem hiding this comment.
P2: isHttpUrl is too permissive for values that are interpolated into markdown/link fields. Use strict URL parsing so malformed or markdown-breaking URLs are rejected before building card links.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/chat/lib/push-notifications/formatter.ts, line 31:
<comment>`isHttpUrl` is too permissive for values that are interpolated into markdown/link fields. Use strict URL parsing so malformed or markdown-breaking URLs are rejected before building card links.</comment>
<file context>
@@ -0,0 +1,113 @@
+};
+
+function isHttpUrl(url: string): boolean {
+ return /^https?:\/\//i.test(url);
+}
+
</file context>
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
1 participant
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
Adds push notification delivery to the companion chat app so it can receive booking notifications dispatched by the
/calbackend (PR #2927).New endpoint:
POST /api/notifications/deliver— authenticates viax-cal-delivery-secretheader (HMAC +timingSafeEqual), validates the request body (including runtime validation of SlackteamId, payload fields, and timezone), then delegates todeliverNotifications()which fans out viaPromise.allSettledto per-platform delivery modules. Returns 422 if the formatter crashes on bad input.New commands:
/cal notify on|off(Slack) and/notify on|off(Telegram) — toggle push notification subscriptions via the Cal.com API. Handles 404 specifically when the user isn't subscribed. Telegram rejects/notifyin group chats (only works in private DM with the bot).Files created:
apps/chat/lib/push-notifications/types.ts— sharedDeliverResulttype (witherrorfield for retry/unsubscribe decisions)apps/chat/lib/push-notifications/formatter.ts—ChatPushPayloadtype +buildPushCard()with URL validation, unknown notificationType fallbackapps/chat/lib/push-notifications/deliver-slack.ts— per-identifier Slack DM with structuredSlackAPIErrordetection + loggingapps/chat/lib/push-notifications/deliver-telegram.ts— per-identifier Telegram DM with invalid chat detection + loggingapps/chat/lib/push-notifications/service.ts—deliverNotifications()fan-out with discriminated union narrowing + loggingapps/chat/app/api/notifications/deliver/route.ts— thin POST route (auth + parse + delegate) with pre-computed HMAC, payload validation, loggingFiles modified:
apps/chat/lib/calcom/client.ts— 4 subscription methods (register/remove × Slack/Telegram)apps/chat/lib/handlers/slack.ts—case "notify":subcommand with 404 handlingapps/chat/lib/handlers/telegram.ts—/notifycommand with group rejection +CalcomApiErrorimportapps/chat/lib/notifications.ts—/notifyadded to both Slack and Telegram help cardsapps/chat/lib/env.ts—CALCOM_DELIVERY_SECRETvalidation (hard fail prod, warn dev only)apps/chat/.env.example— documented new env varReview & Testing Checklist for Human
/caldeliver Slack subscriptions as{ identifier, teamId }or{ identifier, deviceId }? Registration sendsdeviceIdbut delivery expectsteamId— if the backend doesn't remap, every Slack delivery will silently fail/cal notify ontwice) — if it creates duplicates, users get duplicate notificationsdata.urlwith the specific booking URL — currently falls back tohttps://app.cal.com/bookingsCALCOM_DELIVERY_SECRETmatchesCALCOM_CHAT_DELIVERY_SECRETon the/calbackend before deployingPOST /api/notifications/deliverwith valid/invalid secrets, malformed payloads (bad timezone, null hosts)/cal notify onand/cal notify offin Slack — verify DM confirmation and 404 handling/notify onin a Telegram group — should reject with "check your DMs" message/notify onand/notify offin a private Telegram chat — verify subscription lifecycleNotes
apply-labels-from-issuebot/notify - Toggle booking push notifications on/offin BotFather (Settings → Edit Commands)ChatPushPayloadmirrors the type from/calPR #2927 —notificationTypeis typed asstringto gracefully handle future values with a fallback badgeLink to Devin session: https://app.devin.ai/sessions/53397cfd38d34318bf6b3694a8ff7721
Requested by: @dhairyashiil