Notifications (in-app + email digest)#94
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ 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: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
✨ 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 |
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 886c4452c6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| await sendEmail(env, { to: row.email, subject, html }); | ||
| stats.digestsSent++; | ||
| stats.itemsSent += items.results.length; | ||
|
|
||
| await env.DB.prepare( |
There was a problem hiding this comment.
sendEmail can return a non-OK result on transient API/network failures, but this code ignores the result, increments sent counters, and then advances last_digest_at anyway. When that happens, no digest is delivered yet future sweeps filter on newer added_at values, so the missed inbox items are skipped permanently instead of being retried.
Useful? React with 👍 / 👎.
| JOIN videos v ON v.id = i.video_id AND v.deleted_at IS NULL | ||
| LEFT JOIN user u ON u.id = i.channel_user_id | ||
| WHERE i.subscriber_user_id = ? | ||
| AND (? IS NULL OR i.added_at > ?) |
There was a problem hiding this comment.
Using i.added_at > ? with a cursor that is written via CURRENT_TIMESTAMP can lose notifications at second boundaries. If an inbox row is inserted in the same second that last_digest_at is updated (for example between the item query and cursor update), it gets added_at == last_digest_at and will never match subsequent sweeps, causing silent drops.
Useful? React with 👍 / 👎.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a notification system featuring an in-app notification bell and a scheduled email digest for new subscription uploads. Key additions include a notification_prefs database table, frontend components for managing settings and displaying notifications, and backend logic for the digest sweep. Review feedback identifies critical issues such as missing API endpoints for the inbox and seen status, scalability concerns regarding sequential processing in the worker, and logic errors in the digest cursor update that could result in missed notifications. Additionally, improvements for accessibility and error handling during email delivery were recommended.
| const r = await fetch('/api/users/me/inbox?limit=10', { credentials: 'include' }); | ||
| if (r.ok) { | ||
| const data = (await r.json()) as { items: InboxItem[] }; | ||
| setItems(data.items); | ||
| } | ||
| // Mark unseen as seen — fire-and-forget. | ||
| if (unread > 0) { | ||
| await fetch('/api/users/me/inbox/seen', { method: 'POST', credentials: 'include' }); |
There was a problem hiding this comment.
The frontend component attempts to fetch from /api/users/me/inbox and post to /api/users/me/inbox/seen, but these endpoints are not implemented in the notificationRoutes within src/workers/notifications.ts or anywhere else in the provided diff. This will cause the notification bell to fail when opened.
| for (const row of results) { | ||
| stats.consideredUsers++; | ||
| if (!digestDue({ digest_frequency: row.digest_frequency, last_digest_at: row.last_digest_at }, nowMs)) { | ||
| continue; | ||
| } | ||
| // Pull items added since last digest (or all unseen if first run). | ||
| const cutoff = row.last_digest_at; | ||
| const items = await env.DB.prepare( | ||
| `SELECT i.video_id, i.added_at, | ||
| v.title, v.thumbnail_url, | ||
| u.name AS channel_name, u.username AS channel_username | ||
| FROM subscription_inbox i | ||
| JOIN videos v ON v.id = i.video_id AND v.deleted_at IS NULL | ||
| LEFT JOIN user u ON u.id = i.channel_user_id | ||
| WHERE i.subscriber_user_id = ? | ||
| AND (? IS NULL OR i.added_at > ?) | ||
| ORDER BY i.added_at DESC | ||
| LIMIT 25`, | ||
| ) | ||
| .bind(row.user_id, cutoff, cutoff) | ||
| .all<DigestItem>(); | ||
|
|
||
| if (items.results.length === 0) { | ||
| // Nothing new — bump the cursor so we don't re-evaluate every cron. | ||
| await env.DB.prepare( | ||
| `INSERT INTO notification_prefs (user_id, digest_frequency, last_digest_at, updated_at) | ||
| VALUES (?, ?, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP) | ||
| ON CONFLICT(user_id) DO UPDATE SET | ||
| last_digest_at = CURRENT_TIMESTAMP, | ||
| updated_at = CURRENT_TIMESTAMP`, | ||
| ) | ||
| .bind(row.user_id, row.digest_frequency) | ||
| .run(); | ||
| continue; | ||
| } | ||
|
|
||
| const { subject, html } = renderDigestEmail(items.results, origin); | ||
| // sendEmail is fail-open when RESEND_API_KEY is unset. | ||
| await sendEmail(env, { to: row.email, subject, html }); | ||
| stats.digestsSent++; | ||
| stats.itemsSent += items.results.length; | ||
|
|
||
| await env.DB.prepare( | ||
| `INSERT INTO notification_prefs (user_id, digest_frequency, last_digest_at, updated_at) | ||
| VALUES (?, ?, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP) | ||
| ON CONFLICT(user_id) DO UPDATE SET | ||
| last_digest_at = CURRENT_TIMESTAMP, | ||
| updated_at = CURRENT_TIMESTAMP`, | ||
| ) | ||
| .bind(row.user_id, row.digest_frequency) | ||
| .run(); | ||
| } |
There was a problem hiding this comment.
The runDigestSweep function iterates over all users sequentially, performing multiple database queries and a network request (email delivery) for each. This approach is not scalable and will likely exceed the Cloudflare Worker execution time limit and subrequest limit (50 on free, 1000 on paid) as the user base grows. Consider processing users in batches or using a more efficient query to identify only those users who actually have new items since their last_digest_at before entering the loop.
| LIMIT 25`, | ||
| ) | ||
| .bind(row.user_id, cutoff, cutoff) | ||
| .all<DigestItem>(); | ||
|
|
||
| if (items.results.length === 0) { | ||
| // Nothing new — bump the cursor so we don't re-evaluate every cron. | ||
| await env.DB.prepare( | ||
| `INSERT INTO notification_prefs (user_id, digest_frequency, last_digest_at, updated_at) | ||
| VALUES (?, ?, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP) | ||
| ON CONFLICT(user_id) DO UPDATE SET | ||
| last_digest_at = CURRENT_TIMESTAMP, | ||
| updated_at = CURRENT_TIMESTAMP`, | ||
| ) | ||
| .bind(row.user_id, row.digest_frequency) | ||
| .run(); |
There was a problem hiding this comment.
Updating last_digest_at to CURRENT_TIMESTAMP while using a LIMIT 25 in the fetch query can lead to missed notifications. If a user has more than 25 new items, the older items (beyond the first 25) will be skipped in the current digest and will never be included in future digests because the cursor moves past them. It is safer to update the cursor to the added_at timestamp of the most recent item actually included in the digest.
| await sendEmail(env, { to: row.email, subject, html }); | ||
| stats.digestsSent++; | ||
| stats.itemsSent += items.results.length; | ||
|
|
||
| await env.DB.prepare( | ||
| `INSERT INTO notification_prefs (user_id, digest_frequency, last_digest_at, updated_at) | ||
| VALUES (?, ?, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP) | ||
| ON CONFLICT(user_id) DO UPDATE SET | ||
| last_digest_at = CURRENT_TIMESTAMP, | ||
| updated_at = CURRENT_TIMESTAMP`, | ||
| ) | ||
| .bind(row.user_id, row.digest_frequency) | ||
| .run(); |
There was a problem hiding this comment.
The last_digest_at cursor is updated regardless of whether the email delivery via sendEmail was successful. If the email fails to send, the user will miss those notifications entirely because the cursor has already moved forward. You should check the result of sendEmail and only update the database if it succeeds.
const emailResult = await sendEmail(env, { to: row.email, subject, html });
if (emailResult.ok) {
stats.digestsSent++;
stats.itemsSent += items.results.length;
await env.DB.prepare(
`INSERT INTO notification_prefs (user_id, digest_frequency, last_digest_at, updated_at)
VALUES (?, ?, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP)
ON CONFLICT(user_id) DO UPDATE SET
last_digest_at = CURRENT_TIMESTAMP,
updated_at = CURRENT_TIMESTAMP`,
)
.bind(row.user_id, row.digest_frequency)
.run();
}| to={`/watch/${encodeURIComponent(it.video_id)}`} | ||
| onClick={() => setOpen(false)} | ||
| style={{ display: 'flex', gap: 8, alignItems: 'center', textDecoration: 'none' }} |
There was a problem hiding this comment.
For better accessibility, links within a menu should explicitly have the role="menuitem" attribute. Additionally, consider adding role="none" to the parent <ul> and <li> elements to ensure screen readers treat the dropdown as a flat list of menu actions.
<Link
to={`/watch/${encodeURIComponent(it.video_id)}`}
onClick={() => setOpen(false)}
role="menuitem"
style={{ display: 'flex', gap: 8, alignItems: 'center', textDecoration: 'none' }}
>
Adds an in-app bell with unread badge backed by the existing subscription_inbox fan-out (ALO-156), plus a daily/weekly Resend email digest of new uploads from a user's subscriptions. Digest cadence is a per-user preference (default weekly, opt-out via 'off') exposed in account settings. The cron sweep runs in the existing scheduled handler; delivery is fail-open so a missing RESEND_API_KEY just skips the send. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
aloewright
force-pushed
the
conductor/alo-157-notifications-in-app-email-digest
branch
from
886c445 to
d03b059
Compare
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
Closes ALO-157.
Summary
email_digest_frequency(off/daily/weekly, defaultweekly) andemail_digest_last_sent_atcolumns directly on theusertable./api/users/me/notifications/unread-countpowers the header bell badge — countssubscription_inboxrows withseen_at IS NULL(capped at99+)./api/users/me/notifications/preferences(GET/PUT) lets users opt out or change cadence.runDigestSweeplives insrc/workers/digest.tsand is invoked from the existingscheduledhandler (daily 02:00 UTC cron). It selects users whose digest window has elapsed, aggregates new uploads fromsubscription_inboxsince their last digest, and sends a single Resend HTML email. Best-effort:last_sent_atonly advances on Resend success.NotificationBell) polls every 60s, opens a popover with recent inbox items, marks them seen on open.#notificationsanchor that the bell links to.Test plan
npm test— all 511 tests pass (includes newnotifications.test.ts,digest.test.ts, and the migration assertion)npm run lint— 0 findings, AI Gateway guard cleannpm run type-checknpm run build— frontend bundles cleanlyoffsuppresses the email and clearslast_sent_atfrom the prefs UI hint.🤖 Generated with Claude Code