feat(notifications): scope notification preferences and split email vs in-app#297
Merged
martian56 merged 2 commits intoJul 11, 2026
Merged
Conversation
…s in-app Notification preferences were global-only with a single toggle per type, even though the schema carries workspace_id/project_id and the notification path already fanned in-app and email together. This makes the scoping and the two channels real. Backend: per-type email_* columns (in-app stays the existing booleans); UserNotificationPreferenceStore gains GetScoped/UpsertScoped and Resolve (project -> workspace -> account); the notification fan-out now gates the in-app and email channels independently against the resolved preference; account GET/PUT return and accept the email fields; new workspace- and project-scoped GET/PUT endpoints. Frontend: a reusable NotificationPreferencesPanel with independent In-app and Email columns, mounted in account, workspace, and project settings via a new Notifications section; notificationPreferenceService for the scoped endpoints. Closes Devlaner#203 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughNotification preferences now support account, workspace, and project scopes with project-to-workspace-to-account inheritance. Email and in-app channels have independent toggles across API delivery, persistence, and web settings. ChangesScoped preference storage and resolution
Notification delivery
Settings UI
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant SettingsPage
participant NotificationPreferenceService
participant NotificationPreferenceHandler
participant UserNotificationPreferenceStore
participant NotificationService
participant EmailQueue
SettingsPage->>NotificationPreferenceService: load or save scoped preferences
NotificationPreferenceService->>NotificationPreferenceHandler: GET or PUT preference endpoint
NotificationPreferenceHandler->>UserNotificationPreferenceStore: resolve or upsert scoped preference
UserNotificationPreferenceStore-->>NotificationPreferenceHandler: effective or saved preference
NotificationPreferenceHandler-->>NotificationPreferenceService: preference response
NotificationService->>UserNotificationPreferenceStore: resolve receiver preference
UserNotificationPreferenceStore-->>NotificationService: channel-specific toggles
NotificationService->>EmailQueue: enqueue allowed email notifications
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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
🧹 Nitpick comments (2)
apps/web/src/components/settings/NotificationPreferencesPanel.tsx (1)
90-120: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo feedback distinguishing "loading" from "failed to load".
When
load()rejects,prefsis set tonull(line 99), which is the same state as before load resolves. All toggles are simply disabled with no message, so a user hitting a network error sees the same UI as a slow load with no way to retry or understand what happened.🤖 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/components/settings/NotificationPreferencesPanel.tsx` around lines 90 - 120, Distinguish the initial loading state from load failure in the notification preferences component. Update the state and useEffect error handling around load() to track an explicit error, render a clear failure message with a retry action that re-invokes loading, and keep toggles disabled only while loading or when preferences are unavailable.apps/api/internal/store/user_notification_preference_test.go (1)
15-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd email_ field assertions to verify the core feature.*
The test verifies in-app field resolution precedence (Comment, Mention) but never asserts any
email_*fields, even thoughUpsertScopedwrites all ten toggles via the map. Since this PR's primary objective is separate email toggles, the store-level test should verify thatEmailComment,EmailMention, etc. are correctly persisted and resolved across scopes.♻️ Proposed additions
// Workspace row with comment on, mention off -> now wins over account. require.NoError(t, s.UpsertScoped(ctx, &model.UserNotificationPreference{ UserID: w.User.ID, WorkspaceID: &ws, PropertyChange: true, StateChange: true, Comment: true, Mention: false, IssueCompleted: true, + EmailPropertyChange: true, EmailStateChange: true, EmailComment: false, EmailMention: true, EmailIssueCompleted: true, })) got, err = s.Resolve(ctx, w.User.ID, &ws, &proj) require.NoError(t, err) require.True(t, got.Comment) require.False(t, got.Mention, "workspace row wins over account") + require.False(t, got.EmailComment, "workspace email_comment off") + require.True(t, got.EmailMention, "workspace email_mention on")🤖 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/internal/store/user_notification_preference_test.go` around lines 15 - 59, Add assertions for the email preference fields in TestNotifPref_ResolvePrecedence, including EmailComment and EmailMention (and the remaining email_* toggles as appropriate), after each account, workspace, project, and fallback Resolve call. Seed distinct email values in each UpsertGlobal and UpsertScoped fixture so the assertions verify persistence and scope precedence rather than only default values.
🤖 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/internal/handler/notification_preference.go`:
- Around line 92-99: Update baseForScope to return the resolved preference and
an error, propagating Resolve failures instead of falling back to
DefaultNotificationPreference. Preserve the ID reset on successful resolution,
then update UpdateWorkspace and UpdateProject to handle the returned error and
respond consistently with respondResolved before proceeding to UpsertScoped.
In `@apps/web/src/components/settings/NotificationPreferencesPanel.tsx`:
- Around line 106-117: In toggle, avoid restoring the entire captured prefs
snapshot when save fails, since concurrent updates may have changed other keys.
In the catch handler, update only the failed key back to its previous value
while preserving the latest state for all unrelated preferences, using the key
and its captured prior value.
---
Nitpick comments:
In `@apps/api/internal/store/user_notification_preference_test.go`:
- Around line 15-59: Add assertions for the email preference fields in
TestNotifPref_ResolvePrecedence, including EmailComment and EmailMention (and
the remaining email_* toggles as appropriate), after each account, workspace,
project, and fallback Resolve call. Seed distinct email values in each
UpsertGlobal and UpsertScoped fixture so the assertions verify persistence and
scope precedence rather than only default values.
In `@apps/web/src/components/settings/NotificationPreferencesPanel.tsx`:
- Around line 90-120: Distinguish the initial loading state from load failure in
the notification preferences component. Update the state and useEffect error
handling around load() to track an explicit error, render a clear failure
message with a retry action that re-invokes loading, and keep toggles disabled
only while loading or when preferences are unavailable.
🪄 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: 6139ac88-445b-4c39-ace1-2866417aa35f
📒 Files selected for processing (17)
apps/api/internal/handler/auth.goapps/api/internal/handler/notification_preference.goapps/api/internal/handler/notification_preference_test.goapps/api/internal/model/user_notification_preference.goapps/api/internal/router/router.goapps/api/internal/service/notification.goapps/api/internal/service/notification_test.goapps/api/internal/store/user_notification_preference.goapps/api/internal/store/user_notification_preference_test.goapps/api/migrations/000008_notification_pref_channels.down.sqlapps/api/migrations/000008_notification_pref_channels.up.sqlapps/web/src/api/types.tsapps/web/src/components/settings/NotificationPreferencesPanel.tsxapps/web/src/components/settings/SettingsNav.tsxapps/web/src/components/settings/sections-config.tsxapps/web/src/pages/SettingsPage.tsxapps/web/src/services/notificationPreferenceService.ts
…ed toggle baseForScope now returns Resolve errors instead of falling back to the all-true default, so a transient DB error can't reset inherited (disabled) fields on save. The preferences panel reverts only the failed key on error (and merges only that key on success) via functional updates, so a concurrent toggle of another switch is never stomped. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
martian56
approved these changes
Jul 11, 2026
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
2 participants
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.
Feature summary
Notification preferences can now be scoped to a workspace or project (falling back project → workspace → account), and each type has independent in-app and email toggles. Previously preferences were account-only with a single toggle per type, and every in-app notification also sent an email.
Linked issues / discussion
Closes #203
User-facing behavior
Settings → Notifications now shows a matrix: each notification type (property changes, state change, completed, comments, mentions) with an In-app and an Email switch. The same panel appears in three places:
A GET returns the effective (resolved) preferences for the scope; toggling writes an override at that scope. Muting email for a type keeps the in-app notification, and vice versa.
What changed
API (
apps/api/)000008: per-typeemail_*columns (the existing booleans remain the in-app channel; email defaults to on to preserve today's behavior).UserNotificationPreferenceStore:GetScoped,UpsertScoped, andResolve(project → workspace → account). Writes go through a map so a disabled (false) toggle is persisted rather than dropped to the column default.service/notification.go: the fan-out resolves each receiver's scoped preference and gates the in-app and email channels independently.handler/auth.go: account GET/PUT now return/accept theemail_*fields (shared render/apply helpers).handler/notification_preference.go: new workspace- and project-scoped GET/PUT endpoints with membership/access checks.UI (
apps/web/)NotificationPreferencesPanel: reusable In-app/Email matrix, driven byload/saveprops so one component serves all three scopes.notificationPreferenceService: the scoped endpoints.SettingsPage+ settings nav: a Notifications section under workspace and project settings; the account section now uses the panel.Database
email_*boolean columns), down migration included.Why this design
The schema already carried
workspace_id/project_id, so scoping is "most specific stored row wins", which is simple and predictable. Splitting the fan-out into independent in-app and email sets (rather than emailing whoever got an in-app notification) is what makes the two channels genuinely separable.Test plan
go vet ./...and fullgo test ./...green (testcontainers).TestChannelAllows(per-channel gating),TestNotifPref_ResolvePrecedence(project→workspace→account resolution),TestNotifPref_ScopedRoundTrip+TestNotifPref_NonMemberForbidden(scoped endpoints, inheritance, isolation, access control). Existing account-pref test still passes.npm run typecheck,npm run lint,npm run buildgreen.email_comment=falsewithcomment(in-app) still true, the account row still hasemail_comment=true(project override doesn't leak up), and the workspace scope resolves to the account value (inheritance).Out of scope (follow-ups)
Rollout notes
AI assistance
Claude Code— and AI-assisted commits include aCo-Authored-By:trailerChecklist
apps/api/migrations/000008_*.up.sqlAND matching.down.sql--no-verifybypass on the commit (full suite ran on pre-commit)Summary by CodeRabbit
New Features
Bug Fixes