feat(user): add notification preferences - #60
Conversation
Lets each user turn off the weekly digest and/or follow-up reminder emails independently, instead of the previous fixed behavior baked into SendWeeklyDigestUseCase and SendFollowUpRemindersUseCase. Scoped to the two notification channels that actually exist today (weekly digest email, follow-up reminder email) rather than the full daily/weekly/off cadence and multi-channel design in the original ticket, since a daily digest pipeline and Slack/Discord channel (JEF-15) don't exist yet — those are natural follow-ups once that infrastructure lands. - New weeklyDigestEnabled/followUpRemindersEnabled columns on User (default true, so existing users keep receiving both emails) - SendWeeklyDigestUseCase skips users with weeklyDigestEnabled=false (counted as skipped, without querying their applications) - SendFollowUpRemindersUseCase skips applications belonging to users with followUpRemindersEnabled=false - New notificationPreferences query and updateNotificationPreferences mutation, backed by Get/UpdateNotificationPreferencesUseCase - New Notification preferences section in account.tsx with two toggles - Verified end-to-end against a live server: registered a user, confirmed both preferences default to enabled, disabled the weekly digest, and confirmed the change persisted while the reminder preference was untouched
|
Warning Review limit reached
Next review available in: 36 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (17)
WalkthroughAdds persistent notification preferences for weekly digests and follow-up reminders, exposes them through GraphQL, provides account-page toggles, and prevents disabled notifications from being sent. ChangesNotification preferences
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
actor AccountUser
participant AccountPage
participant GraphQLAPI
participant UserResolver
participant UserRepository
AccountUser->>AccountPage: Open account page
AccountPage->>GraphQLAPI: Query notificationPreferences
GraphQLAPI->>UserResolver: getNotificationPreferences(userId)
UserResolver->>UserRepository: findById(userId)
UserRepository-->>UserResolver: User preferences
UserResolver-->>GraphQLAPI: NotificationPreferences
GraphQLAPI-->>AccountPage: weeklyDigestEnabled and followUpRemindersEnabled
AccountUser->>AccountPage: Toggle preference
AccountPage->>GraphQLAPI: updateNotificationPreferences(values)
GraphQLAPI->>UserResolver: updateNotificationPreferences(userId, values)
UserResolver->>UserRepository: update(userId, values)
UserRepository-->>UserResolver: Updated User
GraphQLAPI-->>AccountPage: true
AccountPage->>GraphQLAPI: Refetch notificationPreferences
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/api/src/infrastructure/db/repositories/PrismaUserRepository.ts`:
- Around line 58-68: Move the Prisma-row-to-domain conversion out of
PrismaUserRepository’s private toEntity method into a dedicated mapper under
interface-adapters/mappers/, then update PrismaUserRepository to call that
mapper and remove its local mapping implementation. Preserve the existing User
field mappings while keeping Prisma-specific conversion centralized in the
mapper layer.
In `@apps/web/src/routes/_authenticated/account.tsx`:
- Around line 112-121: Update onToggleWeeklyDigest and onToggleFollowUpReminders
to prevent overlapping preference mutations by disabling the relevant controls
or serializing updates while a request is pending, ensuring the final user
selection cannot be overwritten by an earlier response. Add failure handling for
the mutation and surface errors through the existing account UI feedback
mechanism.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5f00b2ee-13be-4c4d-b7ff-2f1a01721386
📒 Files selected for processing (27)
apps/api/prisma/migrations/20260722231319_add_notification_preferences/migration.sqlapps/api/prisma/schema.prismaapps/api/src/__tests__/application/reminders/SendFollowUpRemindersUseCase.test.tsapps/api/src/__tests__/application/user/GetNotificationPreferencesUseCase.test.tsapps/api/src/__tests__/application/user/UpdateNotificationPreferencesUseCase.test.tsapps/api/src/__tests__/digest/SendWeeklyDigestUseCase.test.tsapps/api/src/__tests__/helpers/createTestDb.tsapps/api/src/__tests__/helpers/mocks.tsapps/api/src/__tests__/infrastructure/db/repositories/PrismaUserRepository.test.tsapps/api/src/__tests__/interface-adapters/resolvers/UserResolver.test.tsapps/api/src/domain/user/User.tsapps/api/src/http/container.tsapps/api/src/http/schema/index.tsapps/api/src/http/schema/mutations/userMutations.tsapps/api/src/http/schema/queries/userQueries.tsapps/api/src/http/schema/types/NotificationPreferencesType.tsapps/api/src/infrastructure/db/repositories/PrismaUserRepository.tsapps/api/src/interface-adapters/resolvers/UserResolver.tsapps/api/src/use-cases/digest/SendWeeklyDigestUseCase.tsapps/api/src/use-cases/ports/IUserRepository.tsapps/api/src/use-cases/reminders/SendFollowUpRemindersUseCase.tsapps/api/src/use-cases/user/GetNotificationPreferencesUseCase.tsapps/api/src/use-cases/user/IGetNotificationPreferencesUseCase.tsapps/api/src/use-cases/user/IUpdateNotificationPreferencesUseCase.tsapps/api/src/use-cases/user/UpdateNotificationPreferencesUseCase.tsapps/web/src/__tests__/components/AccountPage.test.tsxapps/web/src/routes/_authenticated/account.tsx
| weeklyDigestEnabled: boolean; | ||
| followUpRemindersEnabled: boolean; | ||
| createdAt: Date; | ||
| updatedAt: Date; | ||
| }): User { | ||
| return { | ||
| id: row.id, | ||
| email: row.email, | ||
| passwordHash: row.passwordHash, | ||
| weeklyDigestEnabled: row.weeklyDigestEnabled, | ||
| followUpRemindersEnabled: row.followUpRemindersEnabled, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Move Prisma-to-domain mapping into the prescribed mapper layer.
PrismaUserRepository now extends its private toEntity method to convert Prisma rows directly into the domain User. Put this conversion in interface-adapters/mappers/ and have the repository use that mapper, keeping persistence mapping centralised and aligned with the project architecture.
As per coding guidelines: apps/api/**/*.ts files must use mappers in interface-adapters/mappers/ to convert Prisma models to domain entities rather than coupling domain entities to Prisma.
🤖 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/api/src/infrastructure/db/repositories/PrismaUserRepository.ts` around
lines 58 - 68, Move the Prisma-row-to-domain conversion out of
PrismaUserRepository’s private toEntity method into a dedicated mapper under
interface-adapters/mappers/, then update PrismaUserRepository to call that
mapper and remove its local mapping implementation. Preserve the existing User
field mappings while keeping Prisma-specific conversion centralized in the
mapper layer.
Source: Coding guidelines
| const onToggleWeeklyDigest = async (checked: boolean) => { | ||
| await gqlClient.request(UPDATE_NOTIFICATION_PREFERENCES, { weeklyDigestEnabled: checked }); | ||
| await qc.invalidateQueries({ queryKey: ['notificationPreferences'] }); | ||
| }; | ||
|
|
||
| const onToggleFollowUpReminders = async (checked: boolean) => { | ||
| await gqlClient.request(UPDATE_NOTIFICATION_PREFERENCES, { | ||
| followUpRemindersEnabled: checked, | ||
| }); | ||
| await qc.invalidateQueries({ queryKey: ['notificationPreferences'] }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Prevent out-of-order preference writes.
The controls stay active during requests. Rapidly toggling one setting can leave overlapping mutations resolving out of order, persisting an earlier value instead of the user’s final choice. Disable or serialise updates while a preference mutation is pending, and show failures.
Also applies to: 327-340
🤖 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/src/routes/_authenticated/account.tsx` around lines 112 - 121,
Update onToggleWeeklyDigest and onToggleFollowUpReminders to prevent overlapping
preference mutations by disabling the relevant controls or serializing updates
while a request is pending, ensuring the final user selection cannot be
overwritten by an earlier response. Add failure handling for the mutation and
surface errors through the existing account UI feedback mechanism.
mankatcheung
left a comment
mankatcheung
left a comment
There was a problem hiding this comment.
Overview
This PR adds per-user notification preferences (weeklyDigestEnabled / followUpRemindersEnabled), correctly wiring them through all Clean Architecture layers: Prisma schema/migration → domain User → IUserRepository.update → new Get/UpdateNotificationPreferencesUseCase → UserResolver → Pothos query/mutation → account.tsx toggles. Both existing sending pipelines (SendWeeklyDigestUseCase, SendFollowUpRemindersUseCase) now respect the flags. The PR description is transparent about scope being narrower than the original ticket (no daily cadence, no channel selection) since that infra doesn't exist yet — reasonable call.
Code Quality
- Follows established conventions precisely: mutation/query auth checks (
if (!ctx.user) throw GraphQLError(...)),ctx.user.subscoping,fromCodedErrorwrapping, AwilixTRANSIENTregistration for use cases — all match the siblingupdateEmail/updatePassword/exportUserDatacode exactly. SendWeeklyDigestUseCaseshort-circuits before querying applications for opted-out users — good, avoids unnecessary DB work as called out in the PR description.- Good test coverage added across layers: use-case unit tests (including the "leaves fields untouched when undefined" case, which correctly asserts Prisma-style partial update semantics),
PrismaUserRepositoryintegration tests viacreateTestDb, resolver tests, and web component tests for both toggles. - Migration is a straightforward additive
ALTER TABLE ... DEFAULT true, matching the timestamp-prefixed naming convention of prior migrations and defaulting existing users to enabled (no behavior change on rollout).
Issues & Risks
- Silent failure on toggle mutations (
apps/web/.../account.tsx,onToggleWeeklyDigest/onToggleFollowUpReminders): unlike every other mutation in this file (email, password, delete — all usesetError('root', ...)to surface failures), these two have notry/catchat all. A failed mutation becomes an unhandled promise rejection with zero user feedback — the checkbox will just silently not update. Even the "silent" export-download catch has an explicit comment justifying it; this one has neither a catch nor a comment. getNotificationPreferencesquery resolver doesn't wrap the use-case call in try/catch +fromCodedErrorthe way the mutation does — ifGetNotificationPreferencesUseCasethrows itsNOT_FOUNDcoded error, it'll surface as a raw GraphQL error without the coded extensions. Low risk in practice (an authenticated user's own row won't normally be missing) but inconsistent with the mutation's handling one function away. (Note:exportUserDatahas this same gap pre-existing, so it's not a regression introduced here, but worth fixing while touching this file.)- Authorization is correctly self-scoped everywhere (
ctx.user.subonly) — no way to read/update another user's preferences. No concerns there. - Migration safety: additive, backward-compatible, defaults preserve existing behavior. No concerns.
Suggestions
- Wrap the two
onToggleWeeklyDigest/onToggleFollowUpRemindershandlers in try/catch and surface an error message (a small inline error banner under the toggles, consistent with the rest of the page) instead of letting failures pass silently. - Consider reverting the checkbox to its prior state on failure (currently relies on it never having visually flipped since it's driven by server state +
invalidateQueries, so this is mostly a UX/feedback gap rather than a correctness bug). - Optional: align
notificationPreferencesquery error handling with the mutation'sfromCodedErrorwrapping for consistency (can be done together with the pre-existingexportUserDatagap in a follow-up).
Test Coverage
Strong and appropriately layered: new use-case tests (happy path, not-found, partial-update semantics), repository integration tests via the real in-memory DB helper, resolver delegation tests, digest/reminder skip-behavior tests, and web component tests covering both toggle interactions and the loading of initial state. No coverage gap of concern; the one thing not covered is a toggle-mutation failure path (understandably, since there's no error handling to test yet).
Verdict
Approving — the implementation is correct, well-scoped, properly authorized, and thoroughly tested. The missing error handling on the toggle handlers is a real but minor UX gap, not a blocker; recommend addressing in a fast-follow.
1 participant
Summary
SendWeeklyDigestUseCase/SendFollowUpRemindersUseCaseweeklyDigestEnabled/followUpRemindersEnabledcolumns onUser, defaulting totrueso existing users are unaffectedSendWeeklyDigestUseCasenow skips users with the digest disabled (without even querying their applications);SendFollowUpRemindersUseCaseskips applications for users with reminders disablednotificationPreferencesquery andupdateNotificationPreferencesmutation, backed byGetNotificationPreferencesUseCase/UpdateNotificationPreferencesUseCaseaccount.tsxwith two toggle checkboxesFixes JEF-21
Test plan
pnpm typecheck(api + web)pnpm lint(api + web)pnpm test— 410 API tests + 77 web tests passing, including new coverage for both use cases, the updated digest/reminder skip behavior,PrismaUserRepository,UserResolver, and the new toggles UI. (One pre-existing, unrelated flaky test inPrismaDocumentRepository— a timestamp-ordering race — reproduces on this branch too; confirmed it also fails standalone on 3 reruns with no changes of mine involved.)pnpm build(all packages)true, calledupdateNotificationPreferences(weeklyDigestEnabled: false), confirmed it persisted whilefollowUpRemindersEnabledstayed untouched🤖 Generated with Claude Code
https://claude.ai/code/session_01DdEiNRTcUnM6kE5AdFQ3n8
Summary by CodeRabbit