User Preferences for Notification Channels#3657
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughAdds a notification preferences subsystem: DB migration and EdgeDB type, GraphQL DTOs/inputs/resolver, Neo4j and EdgeQL repositories, Nest module, service integration into notification delivery, enums/type-registration plumbing, and module augmentations for concrete notifications. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 4
🧹 Nitpick comments (6)
src/components/notifications/notification.module.ts (1)
13-13: AddforwardRefon both sides of the circular module dependency.
NotificationPreferencesModulealready wraps its back-reference withforwardRef(() => NotificationModule). "In order to resolve circular dependencies between modules, use the sameforwardRef()utility function on both sides of the modules association." Without it here, startup behaviour depends on module load order, which is usually not stable, so havingforwardRefeverywhere is the safer approach.🛠 Proposed fix
-import { Module } from '@nestjs/common'; +import { forwardRef, Module } from '@nestjs/common'; ... `@Module`({ - imports: [NotificationPreferencesModule], + imports: [forwardRef(() => NotificationPreferencesModule)],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/notifications/notification.module.ts` at line 13, The NotificationModule's imports currently list NotificationPreferencesModule without forwardRef; to fix the circular dependency wrap that entry with forwardRef(() => NotificationPreferencesModule) in the imports array of NotificationModule (import forwardRef from `@nestjs/common` if not already) so it matches the back-reference in NotificationPreferencesModule and resolves startup order instability caused by the circular module dependency.src/components/notifications/preferences/notification-preference.dto.ts (1)
56-56: Extraneous| undefinedondescription.
@Field({ nullable: true })maps tonullin the GraphQL schema; the| undefinedbroadens the TypeScript type unnecessarily.string | nullis sufficient and consistent with the nullable contract.✨ Suggested fix
- readonly description: string | null | undefined; + readonly description: string | null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/notifications/preferences/notification-preference.dto.ts` at line 56, The NotificationPreference DTO's description property currently types as "string | null | undefined"; update the property declaration in notification-preference.dto (the description field on the DTO class) to "readonly description: string | null;" to match the `@Field`({ nullable: true }) GraphQL contract and remove the extraneous "| undefined". Also scan other DTO fields in the same class for similar "| undefined" usages and align them to either plain types or "T | null" when decorated with nullable.src/components/notifications/notification.strategy.ts (1)
46-50:getLabel()usesthis.constructor.name— beware of minification.If the codebase is ever minified (class names mangled),
this.constructor.namewill produce garbled labels. This is fine for a server-side Node.js app that doesn't minify, but worth a quick note.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/notifications/notification.strategy.ts` around lines 46 - 50, getLabel() relies on this.constructor.name which breaks under minification; replace this runtime-reflection with an explicit identifier: add a static or instance property (e.g., static label or protected label) on each concrete NotificationStrategy subclass and change getLabel() to read that property (falling back to a safe default) instead of using this.constructor.name and Case.capital; update NotificationStrategy and all subclasses to declare the label value.src/components/notifications/preferences/notification-preferences.service.ts (1)
96-109: Double-grouping ingetOverridesMapis slightly wasteful.The rows are already fetched filtered by
notificationType(passed torepo.getOverridesForUsers), yetgroupOverridesByTypere-groups them by type before extracting just the one type. This works correctly but does redundant work. Not a problem at current scale — just noting for awareness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/notifications/preferences/notification-preferences.service.ts` around lines 96 - 109, getOverridesMap does redundant work by calling groupOverridesByType after repo.getOverridesForUsers already returned rows filtered by notificationType; instead, avoid the extra grouping by type and directly group rows by user id and compute the Partial<ChannelSettings> for the given notificationType. Update getOverridesMap to use groupToMapBy(rows, row => row.user.id) (or existing grouping helper) and for each user’s rows merge/compute overrides for the provided notificationType (instead of calling groupOverridesByType), keeping the existing return shape and using the same mapValues/...asMap pattern.src/components/notifications/notification.service.ts (2)
78-81: Duplicated type-name derivation acknowledged but still a maintenance risk.The comment on Line 79 acknowledges this is duplicated from the
NotificationStrategydecorator. If the naming convention ever changes (e.g., class suffix changes), both sites must be updated in lockstep.Consider extracting a shared helper or looking up the decorator metadata directly via the class (since
typeClassToNameexists on the impl). SincegetTypeNameis on the abstractNotificationServiceandtypeClassToNameis on the impl, a simple lookup won't work here — but worth considering if this method could be moved to the impl or use the map.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/notifications/notification.service.ts` around lines 78 - 81, The getTypeName method in NotificationService duplicates the class-name-to-type conversion used by the NotificationStrategy decorator, creating a maintenance hazard; fix it by centralizing the logic: either extract the conversion into a shared helper (e.g., a new util function like deriveNotificationTypeName) and call that from both NotificationService.getTypeName and the NotificationStrategy decorator, or move getTypeName out of the abstract NotificationService into the concrete implementation and reuse the existing typeClassToName map/metadata on the impl; update all call sites (getTypeName, NotificationStrategy) to use the single shared function or the impl's map so the suffix-to-type rule is maintained in one place.
196-223: Duplicatedinstanceofcheck across two iterations of the samediscoveredarray.The validation at Lines 201–205 and Lines 212–216 are identical checks on the same instances. Consider iterating once and building all three maps in a single pass.
Proposed refactor
async onModuleInit() { const discovered = this.discovery.discover(NotificationStrategy).classes(); - this.strategiesByClassType = mapEntries( - discovered, - ({ meta: { cls }, instance }) => { - if (!(instance instanceof INotificationStrategy)) { - throw new ServerException( - `Strategy for ${cls.name} does not implement INotificationStrategy`, - ); - } - return [cls, instance]; - }, - ).asMap; - this.strategiesByNameType = mapEntries( - discovered, - ({ meta: { cls, typeName }, instance }) => { - if (!(instance instanceof INotificationStrategy)) { - throw new ServerException( - `Strategy for ${cls.name} does not implement INotificationStrategy`, - ); - } - return [typeName, instance]; - }, - ).asMap; - this.typeClassToName = mapEntries(discovered, ({ meta }) => [ - meta.cls, - meta.typeName, - ]).asMap; + const byClass = new Map<ResourceShape<Notification>, INotificationStrategy<Notification>>(); + const byName = new Map<NotificationType, INotificationStrategy<Notification>>(); + const classToName = new Map<ResourceShape<Notification>, NotificationType>(); + for (const { meta: { cls, typeName }, instance } of discovered) { + if (!(instance instanceof INotificationStrategy)) { + throw new ServerException( + `Strategy for ${cls.name} does not implement INotificationStrategy`, + ); + } + byClass.set(cls, instance); + byName.set(typeName, instance); + classToName.set(cls, typeName); + } + this.strategiesByClassType = byClass; + this.strategiesByNameType = byName; + this.typeClassToName = classToName; this.ready.set(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/notifications/notification.service.ts` around lines 196 - 223, The onModuleInit currently loops over discovered twice and duplicates the instanceof INotificationStrategy check; instead, iterate once over discovered (the result of this.discovery.discover(NotificationStrategy).classes()), validate each instance is instanceof INotificationStrategy once (throwing ServerException with the same message if not) and inside that single loop populate strategiesByClassType, strategiesByNameType and typeClassToName (using the same mapping logic currently in the two mapEntries calls) so you produce the three maps in one pass and then assign them to this.strategiesByClassType, this.strategiesByNameType and this.typeClassToName.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/notifications/dto/notification-channel.enum.ts`:
- Around line 15-22: The resolve function's second parameter is too narrow;
update the signature of resolve to accept DB-null overrides by changing the type
of the second parameter to boolean | null | undefined (e.g., resolve:
(availability: EnumType<typeof ca>, override: boolean | null | undefined) => {
... }) so callers can pass NotificationChannelPreference.override (typed boolean
| null) directly while the implementation can continue using ?? to handle
defaults.
In
`@src/components/notifications/preferences/notification-preferences.gel.repository.ts`:
- Around line 33-40: The mapping that builds PreferenceOverrideRow uses
castToEnum for both notificationType and channel but the channel fallback is
currently undefined (via "undefined as unknown as NotificationChannel"), which
can produce undefined at runtime and violate the non-optional
PreferenceOverrideRow.channel; change the channel fallback to a defined sentinel
like '' as NotificationChannel (or another valid NotificationChannel sentinel)
so castToEnum(...) for channel mirrors notificationType's fallback and always
returns a non-undefined NotificationChannel; update the castToEnum call for
channel accordingly where the PreferenceOverrideRow is constructed.
- Around line 19-44: The Gel implementation of getOverridesForUsers is missing
the optional notificationType parameter and thus ignores it; update the
getOverridesForUsers method signature to accept notificationType?:
NotificationType and forward that value into the db.run call (pass it into
getOverridesForUsersQuery params), then modify getOverridesForUsersQuery to
accept the notificationType param and include a server-side filter on
pref.notificationType (using the same castToEnum<typeof pref.notificationType,
NotificationType> helper) so that when notificationType is provided only
matching Preference rows are returned; reference symbols: getOverridesForUsers,
getOverridesForUsersQuery, pref.notificationType, NotificationType, castToEnum.
In
`@src/components/notifications/preferences/notification-preferences.resolver.ts`:
- Around line 39-52: The `@Mutation` decorator for updateNotificationPreferences
declares a single NotificationPreference but the resolver returns an array;
change the decorator to return an array type (use the array form of
NotificationPreference like the sibling Query) so the GraphQL SDL matches the
resolver signature for updateNotificationPreferences and NotificationPreference.
---
Nitpick comments:
In `@src/components/notifications/notification.module.ts`:
- Line 13: The NotificationModule's imports currently list
NotificationPreferencesModule without forwardRef; to fix the circular dependency
wrap that entry with forwardRef(() => NotificationPreferencesModule) in the
imports array of NotificationModule (import forwardRef from `@nestjs/common` if
not already) so it matches the back-reference in NotificationPreferencesModule
and resolves startup order instability caused by the circular module dependency.
In `@src/components/notifications/notification.service.ts`:
- Around line 78-81: The getTypeName method in NotificationService duplicates
the class-name-to-type conversion used by the NotificationStrategy decorator,
creating a maintenance hazard; fix it by centralizing the logic: either extract
the conversion into a shared helper (e.g., a new util function like
deriveNotificationTypeName) and call that from both
NotificationService.getTypeName and the NotificationStrategy decorator, or move
getTypeName out of the abstract NotificationService into the concrete
implementation and reuse the existing typeClassToName map/metadata on the impl;
update all call sites (getTypeName, NotificationStrategy) to use the single
shared function or the impl's map so the suffix-to-type rule is maintained in
one place.
- Around line 196-223: The onModuleInit currently loops over discovered twice
and duplicates the instanceof INotificationStrategy check; instead, iterate once
over discovered (the result of
this.discovery.discover(NotificationStrategy).classes()), validate each instance
is instanceof INotificationStrategy once (throwing ServerException with the same
message if not) and inside that single loop populate strategiesByClassType,
strategiesByNameType and typeClassToName (using the same mapping logic currently
in the two mapEntries calls) so you produce the three maps in one pass and then
assign them to this.strategiesByClassType, this.strategiesByNameType and
this.typeClassToName.
In `@src/components/notifications/notification.strategy.ts`:
- Around line 46-50: getLabel() relies on this.constructor.name which breaks
under minification; replace this runtime-reflection with an explicit identifier:
add a static or instance property (e.g., static label or protected label) on
each concrete NotificationStrategy subclass and change getLabel() to read that
property (falling back to a safe default) instead of using this.constructor.name
and Case.capital; update NotificationStrategy and all subclasses to declare the
label value.
In `@src/components/notifications/preferences/notification-preference.dto.ts`:
- Line 56: The NotificationPreference DTO's description property currently types
as "string | null | undefined"; update the property declaration in
notification-preference.dto (the description field on the DTO class) to
"readonly description: string | null;" to match the `@Field`({ nullable: true })
GraphQL contract and remove the extraneous "| undefined". Also scan other DTO
fields in the same class for similar "| undefined" usages and align them to
either plain types or "T | null" when decorated with nullable.
In
`@src/components/notifications/preferences/notification-preferences.service.ts`:
- Around line 96-109: getOverridesMap does redundant work by calling
groupOverridesByType after repo.getOverridesForUsers already returned rows
filtered by notificationType; instead, avoid the extra grouping by type and
directly group rows by user id and compute the Partial<ChannelSettings> for the
given notificationType. Update getOverridesMap to use groupToMapBy(rows, row =>
row.user.id) (or existing grouping helper) and for each user’s rows
merge/compute overrides for the provided notificationType (instead of calling
groupOverridesByType), keeping the existing return shape and using the same
mapValues/...asMap pattern.
src/components/notifications/preferences/notification-preferences.gel.repository.ts
Show resolved
Hide resolved
src/components/notifications/preferences/notification-preferences.gel.repository.ts
Show resolved
Hide resolved
src/components/notifications/preferences/notification-preferences.resolver.ts
Outdated
Show resolved
Hide resolved
…hannels can be always on/off
In the future this could be elevated by having the notification listener `added$()` check if the current user has an override. That way the notification could be unconditionally broadcasted, but the listening part is filtered.
CarsonF
force-pushed
the
notifications/config
branch
from
76285c5 to
338ced1
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/components/notifications/notification.service.ts (1)
198-223: Duplicatedinstanceofvalidation across twomapEntriespasses — consider a single pass.The
instanceof INotificationStrategycheck is identical in bothstrategiesByClassTypeandstrategiesByNameTypebuilds. A single validated pass would be slightly cleaner and avoid iteratingdiscoveredtwice.♻️ Single-pass initialization
- this.strategiesByClassType = mapEntries( - discovered, - ({ meta: { cls }, instance }) => { - if (!(instance instanceof INotificationStrategy)) { - throw new ServerException( - `Strategy for ${cls.name} does not implement INotificationStrategy`, - ); - } - return [cls, instance]; - }, - ).asMap; - this.strategiesByNameType = mapEntries( - discovered, - ({ meta: { cls, typeName }, instance }) => { - if (!(instance instanceof INotificationStrategy)) { - throw new ServerException( - `Strategy for ${cls.name} does not implement INotificationStrategy`, - ); - } - return [typeName, instance]; - }, - ).asMap; - this.typeClassToName = mapEntries(discovered, ({ meta }) => [ - meta.cls, - meta.typeName, - ]).asMap; + const validated = discovered.map(({ meta: { cls, typeName }, instance }) => { + if (!(instance instanceof INotificationStrategy)) { + throw new ServerException( + `Strategy for ${cls.name} does not implement INotificationStrategy`, + ); + } + return { cls, typeName, instance }; + }); + this.strategiesByClassType = new Map(validated.map(({ cls, instance }) => [cls, instance])); + this.strategiesByNameType = new Map(validated.map(({ typeName, instance }) => [typeName, instance])); + this.typeClassToName = new Map(validated.map(({ cls, typeName }) => [cls, typeName]));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/notifications/notification.service.ts` around lines 198 - 223, The duplicate instanceof checks should be consolidated into a single pass over discovered: iterate discovered once (using mapEntries or a simple loop), validate instance instanceof INotificationStrategy (throwing the same ServerException when invalid), and during that pass produce the three mappings needed for strategiesByClassType, strategiesByNameType and typeClassToName; then assign their .asMap (or converted maps) to strategiesByClassType, strategiesByNameType and typeClassToName respectively. Ensure you reference the same meta.cls, meta.typeName and instance when building the three outputs so you no longer iterate discovered twice.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/notifications/notification.service.ts`:
- Around line 119-155: The notification is persisted via super.create(...)
before preferences are fetched, so a failure in preferences fetch can leave an
undelivered row; move the preferences lookup and channels computation ahead of
persisting: call this.preferencesService.getOverridesMap(...) and compute
channelsForUsers (using the original recipients parameter or out.recipients
fallback logic—i.e., the same userRecipients calculation but based on the input
recipients) and determine strategy.broadcastTo() targets first, then call
super.create(type, recipients, input) to persist and finally call
deliverToAppChannel(notification, ...). Update code paths that reference
out.recipients so they still work (if you need out.recipients after create, keep
creating but compute the preference-based channel lists beforehand), and ensure
any errors from getOverridesMap still bubble or are handled explicitly (so you
don’t persist without delivery).
In
`@src/components/notifications/preferences/notification-preferences.repository.ts`:
- Around line 33-45: The current MATCH uses
node('pref','NotificationPreference',{ notificationType }) which passes
undefined as null and causes MATCH to restrict to notificationType IS null;
change this to always match NotificationPreference without properties and, if
notificationType is provided, add a conditional .where({
'pref.notificationType': notificationType }) (or only call .where for that
predicate when notificationType !== undefined) so getPreferences() and other
callers return all preferences when the filter is omitted; keep the existing
apoc.map.merge and return logic intact.
---
Duplicate comments:
In `@src/components/notifications/dto/notification-channel.enum.ts`:
- Around line 15-22: The resolve function's signature narrows override to
boolean | undefined while callers may pass DB-derived values that can be null;
widen the override parameter type to boolean | null | undefined in the resolve
declaration (the function with parameters "availability: EnumType<typeof ca>"
and "override") so the TypeScript types match runtime behavior and the null case
handled by the existing ?? expression remains valid.
---
Nitpick comments:
In `@src/components/notifications/notification.service.ts`:
- Around line 198-223: The duplicate instanceof checks should be consolidated
into a single pass over discovered: iterate discovered once (using mapEntries or
a simple loop), validate instance instanceof INotificationStrategy (throwing the
same ServerException when invalid), and during that pass produce the three
mappings needed for strategiesByClassType, strategiesByNameType and
typeClassToName; then assign their .asMap (or converted maps) to
strategiesByClassType, strategiesByNameType and typeClassToName respectively.
Ensure you reference the same meta.cls, meta.typeName and instance when building
the three outputs so you no longer iterate discovered twice.
src/components/notifications/preferences/notification-preferences.repository.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/notifications/notification.repository.ts`:
- Around line 87-101: The generated Cypher can end up with two consecutive WHERE
clauses when recipients is non-null; in the .apply(...) block that returns
q.raw(...) for appAvailability handling (referencing appAvailability,
isDefaultOn, this.getType(type)), insert a "WITH recipient" (i.e., pass the
recipient forward) before emitting the raw WHERE/EXISTS clause so the WHERE
binds to a new clause rather than repeating after the previous MATCH/WHERE;
update the .apply call to emit a WITH recipient then the existing raw EXISTS
predicate to ensure valid Cypher in both the MATCH and CALL {} paths.
…references The create query now checks: - Strategy's App ChannelAvailability (AlwaysOff skips all recipients) - DefaultOn: excludes recipients who disabled App via NotificationPreference - DefaultOff: only includes recipients who enabled App via NotificationPreference - AlwaysOn: no filtering (existing behavior) Co-authored-by: CarsonF <932566+CarsonF@users.noreply.github.com>
CarsonF
force-pushed
the
notifications/config
branch
from
c819fb1 to
903840f
Compare
Apparently you can't pass undefined, because it creates a parameter binding that this is not fulfilled.
There was a problem hiding this comment.
Pull request overview
Adds user-configurable notification preferences per delivery channel (currently only App), and integrates preference-aware delivery and recipient filtering into the notification pipeline.
Changes:
- Introduces Notification Preferences GraphQL API (query + mutation) with DTOs/service + split DB repositories (Neo4j + Gel).
- Adds notification channel & availability enums and a dynamic
NotificationTypeenum driven by registered strategies. - Updates notification creation/delivery to honor per-user channel preferences (and filters stored recipients accordingly for the App channel).
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/components/notifications/preferences/notification-preferences.service.ts | Builds effective preferences and persists/removes per-channel overrides; provides overrides lookup map for delivery. |
| src/components/notifications/preferences/notification-preferences.resolver.ts | Exposes GraphQL query/mutation for reading/updating current user preferences plus a convenience channels field. |
| src/components/notifications/preferences/notification-preferences.repository.ts | Neo4j repository for reading/upserting/deleting preference override nodes. |
| src/components/notifications/preferences/notification-preferences.gel.repository.ts | Gel repository implementation of the same overrides read/write operations. |
| src/components/notifications/preferences/notification-preferences.module.ts | Wires preferences service/resolver and split DB repository; exports service for notification delivery usage. |
| src/components/notifications/preferences/notification-preference.dto.ts | GraphQL object/input types for preferences and per-channel overrides. |
| src/components/notifications/notification.strategy.ts | Enhances strategy metadata to register notification types and adds channel availability/label/description hooks. |
| src/components/notifications/notification.service.ts | Adds strategy lookup maps by class/name and delivers notifications according to user channel preferences. |
| src/components/notifications/notification.repository.ts | Filters recipient creation for App channel based on stored preference overrides (Neo4j). |
| src/components/notifications/notification.gel.repository.ts | Updates strategy iteration to use the renamed strategy map (Gel). |
| src/components/notifications/notification.module.ts | Imports preferences module so delivery can consult overrides. |
| src/components/notifications/index.ts | Re-exports NotificationType and NotificationMap for module augmentation. |
| src/components/notifications/dto/notification-type.enum.ts | Introduces dynamic NotificationType enum derived from registered concrete notification types. |
| src/components/notifications/dto/notification-channel.enum.ts | Adds NotificationChannel and ChannelAvailability enums (+ helpers). |
| src/components/notifications/dto/index.ts | Exports new enums/types from notifications DTO barrel. |
| src/components/notification-system/system-notification.dto.ts | Registers System notification type via NotificationMap module augmentation. |
| src/components/comments/mention-notification/comment-via-mention-notification.dto.ts | Registers CommentViaMention notification type via NotificationMap module augmentation. |
| dbschema/notifications.gel | Adds Notification::Preference schema for storing per-user overrides in Gel. |
| dbschema/migrations/00030-m1ded7e.edgeql | Migration creating the new Gel preference type + exclusive constraint. |
Comments suppressed due to low confidence (2)
src/components/notifications/preferences/notification-preferences.service.ts:109
getOverridesMap()groups rows by user, but then callsgroupOverridesByType()for each user group even though the repo query is already filtered to a singlenotificationType. This adds unnecessary extra grouping/allocations per user. Consider building the per-user channel settings directly from that user’s rows (channel → enabled) without re-grouping by type.
async getOverridesMap(
notificationType: NotificationType,
userIds: ReadonlyArray<ID<'User'>>,
): Promise<ReadonlyMap<ID<'User'>, Partial<ChannelSettings>>> {
if (userIds.length === 0) return new Map();
const rows = await this.repo.getOverridesForUsers(
userIds,
notificationType,
);
return mapValues(
groupToMapBy(rows, (row) => row.user.id),
(_, rows) => this.groupOverridesByType(rows).get(notificationType) ?? {},
).asMap;
}
src/components/notifications/preferences/notification-preferences.resolver.ts:52
- This PR introduces new GraphQL query/mutation behavior (reading/updating notification preferences) and changes notification delivery filtering based on those preferences, but there are no e2e tests covering the new operations or the delivery behavior (e.g., opting out prevents app delivery / listing). Add e2e coverage to prevent regressions as new channels (Email) are added.
@Query(() => [NotificationPreference], {
description: 'The effective notification preferences for the current user',
})
async notificationPreferences(): Promise<readonly NotificationPreference[]> {
return await this.preferences.getPreferences();
}
@ResolveField(() => [NotificationChannel], {
description: 'Convenience list of channels that are currently enabled',
})
channels(
@Parent() preference: NotificationPreference,
): readonly NotificationChannel[] {
return preference.channelPreferences.flatMap((p) =>
p.enabled ? p.channel : [],
);
}
@Mutation(() => [NotificationPreference], {
description: stripIndent`
Update the current user's notification preferences for specific types
Note that items should only have a notificationType once, and duplicates are ignored.
`,
})
async updateNotificationPreferences(
@Args('input', { type: () => [UpdateNotificationPreference] })
input: readonly UpdateNotificationPreference[],
): Promise<readonly NotificationPreference[]> {
await this.preferences.updatePreferences(input);
return await this.preferences.getPreferences();
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+28
to
+37
| private readonly getOverridesForUsersQuery = e.params( | ||
| { userIds: e.array(e.uuid), notificationType: e.optional(e.str) }, | ||
| ({ userIds, notificationType }) => { | ||
| const users = e.cast(e.User, e.array_unpack(userIds)); | ||
| return e.select(e.Notification.Preference, (pref) => ({ | ||
| filter: e.op( | ||
| e.op(pref.user, 'in', users), | ||
| 'and', | ||
| e.op(pref.notificationType, '=', notificationType), | ||
| ), |
There was a problem hiding this comment.
In Gel mode, this query always filters by pref.notificationType = notificationType. When notificationType is not provided, the optional parameter will be empty and the equality check prevents any rows from matching, so getOverridesForUsers(userIds) will effectively always return an empty list. Make the notificationType predicate conditional (only applied when a notificationType is passed) so fetching all overrides works correctly.
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
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.
This adds functionality to allow users to opt in & out of certain notifications by each delivery channel. Currently there's still only
Appdelivery channel. ButEmailis next.