Conversation
✅ Deploy Preview for dashboard-v2-novu-staging canceled.
|
github-actions
bot
changed the title
WalkthroughThis pull request introduces context-aware subscription support across the API and internal SDK. The change adds optional 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
libs/dal/src/repositories/topic/topic-subscribers.repository.ts (1)
250-285: Handle emptycontextKeysto avoid unfiltered cross-context results.
if (contextKeys)skips empty arrays, but callers can pass[](e.g., via query transform) to mean “no context”. That currently drops the filter and returns all contexts.🐛 Proposed fix
- if (contextKeys) { + if (contextKeys !== undefined) { Object.assign(query, this.buildContextExactMatchQuery(contextKeys)); }apps/api/src/app/topics-v2/e2e/create-topic-subscriptions.e2e.ts (1)
16-29: RestoreIS_CONTEXT_PREFERENCES_ENABLEDafter the suite to prevent leakage.The env var is set in
beforebut never reset, which can make unrelated tests context-aware unexpectedly.🔧 Suggested fix
describe('Create topic subscriptions - /v2/topics/:topicKey/subscriptions (POST) `#novu-v2`', async () => { let session: UserSession; let novuClient: Novu; let subscriber1: SubscriberEntity; let subscriber2: SubscriberEntity; let subscriber3: SubscriberEntity; let topicSubscribersRepository: TopicSubscribersRepository; + let previousContextPreferencesEnabled: string | undefined; before(async () => { - (process.env as Record<string, string>).IS_CONTEXT_PREFERENCES_ENABLED = 'true'; + previousContextPreferencesEnabled = process.env.IS_CONTEXT_PREFERENCES_ENABLED; + process.env.IS_CONTEXT_PREFERENCES_ENABLED = 'true'; ... }); + + after(() => { + if (previousContextPreferencesEnabled === undefined) { + delete process.env.IS_CONTEXT_PREFERENCES_ENABLED; + return; + } + process.env.IS_CONTEXT_PREFERENCES_ENABLED = previousContextPreferencesEnabled; + });
🧹 Nitpick comments (10)
apps/dashboard/src/components/header-navigation/no-changes-modal.tsx (1)
6-10: Consider removing unusedtargetEnvironmentprop.The
targetEnvironmentprop is defined but never used in the component. If it's not needed for future functionality, consider removing it to keep the interface clean.♻️ Proposed refactor
type NoChangesModalProps = { isOpen: boolean; onClose: () => void; - targetEnvironment?: IEnvironment; };libs/internal-sdk/src/models/operations/subscriberscontrollerlistsubscribertopics.ts (1)
60-63: Consider validating the documentedcontextKeysformat.Line 61 documents
"type:id", but the schema currently accepts any string. Adding a light regex/refine would fail fast on obvious bad inputs and improve error messages. Since this file is generated, the change should be applied in the generator/spec template.Proposed schema refinement
- contextKeys: z.array(z.string()).optional(), + contextKeys: z + .array(z.string().regex(/.+:.+/, 'Expected "type:id"')) + .optional(),Also applies to: 90-111
apps/api/src/app/topics-v2/dtos/delete-topic-subscriptions-response.dto.ts (1)
109-115: Consider adding validation decorators for consistency.The
contextKeysfield lacks validation decorators (@IsOptional,@IsArray,@IsString({ each: true })) that are present on other optional fields likeidentifier(lines 92-94). While this DTO appears to be used primarily for response serialization, adding validation decorators would maintain consistency with the codebase patterns and provide defense-in-depth.Suggested improvement
+ `@IsOptional`() + `@IsArray`() + `@IsString`({ each: true }) `@ApiProperty`({ description: 'Context keys that scope this subscription (e.g., tenant:org-a, project:proj-123)', example: ['tenant:org-a', 'project:proj-123'], type: [String], required: false, }) contextKeys?: string[];apps/api/src/app/subscriptions/usecases/get-subscription/get-subscription.usecase.ts (1)
178-186: Consider reusing the feature flag result to avoid duplicate async calls.The
IS_CONTEXT_PREFERENCES_ENABLEDflag is checked twice per request—once inexecute()(line 40) and again inbuildContextExactMatchQuery(). Consider passing the flag value as a parameter to avoid the redundant async call.♻️ Suggested refactor
- private async buildContextExactMatchQuery( + private buildContextExactMatchQuery( contextKeys: string[] | undefined, - organizationId: string + isContextEnabled: boolean ): Promise<Record<string, unknown>> { - const useContextFiltering = await this.featureFlagsService.getFlag({ - key: FeatureFlagsKeysEnum.IS_CONTEXT_PREFERENCES_ENABLED, - defaultValue: false, - organization: { _id: organizationId }, - }); - - if (!useContextFiltering) { + if (!isContextEnabled) { return {}; // FF OFF: no context filtering (pre-feature behavior) } // ... rest of method }Then call it with:
const contextQuery = this.buildContextExactMatchQuery(subscription.contextKeys, isContextEnabled);apps/api/src/app/shared/dtos/subscriptions/create-subscriptions-response.dto.ts (1)
178-183: Consider adding validation decorators for consistency.Other optional array fields in this DTO (e.g.,
preferencesat lines 174-176) use@IsArray()and@IsOptional()decorators. For consistency, consider adding them tocontextKeysas well.♻️ Suggested addition
`@ApiPropertyOptional`({ description: 'Context keys that scope this subscription (e.g., tenant:org-a, project:proj-123)', example: ['tenant:org-a', 'project:proj-123'], type: [String], }) + `@IsArray`() + `@IsOptional`() contextKeys?: string[];apps/api/src/app/subscriptions/usecases/update-subscription/update-subscription.usecase.ts (1)
140-165: Consider caching the feature flag result to reduce redundant calls.The
IS_CONTEXT_PREFERENCES_ENABLEDflag is checked multiple times per request (inexecute(),updatePreferencesForSubscription(), andfetchPreferencesForSubscription()). Consider fetching it once inexecute()and passing it down to avoid redundant async calls.apps/api/src/app/shared/dtos/subscription-details-response.dto.ts (1)
38-44: Consider adding validation decorators for consistency with other fields.Other fields in this DTO use validation decorators (e.g.,
preferenceshas@IsArray,@IsOptional). For consistency, consider adding them tocontextKeys.♻️ Suggested addition
`@ApiPropertyOptional`({ description: 'Context keys that scope this subscription (e.g., tenant:org-a, project:proj-123)', example: ['tenant:org-a', 'project:proj-123'], type: [String], }) + `@IsArray`() + `@IsOptional`() contextKeys?: string[];apps/api/src/app/subscriptions/usecases/create-subscriptions/create-subscriptions.usecase.ts (1)
622-648: Consider consolidating redundant feature flag checks.The
IS_CONTEXT_PREFERENCES_ENABLEDflag is checked both in theexecutemethod (lines 59-68) and again here inresolveContexts. The second check is effectively unreachable when the first check returnsundefinedforcontextKeys, making this check redundant.While not incorrect, consolidating to a single check point would improve clarity.
apps/api/src/app/topics-v2/e2e/list-topic-subscriptions.e2e.ts (1)
16-18: Consider cleaning up the environment variable after tests.Setting
process.env.IS_CONTEXT_PREFERENCES_ENABLEDdirectly without cleanup in anafterhook could potentially leak to other test files running in the same process. Consider adding cleanup:♻️ Suggested cleanup
before(async () => { (process.env as Record<string, string>).IS_CONTEXT_PREFERENCES_ENABLED = 'true'; // ... rest of setup }); + + after(() => { + delete (process.env as Record<string, string>).IS_CONTEXT_PREFERENCES_ENABLED; + });libs/internal-sdk/src/react-query/topicsSubscriptionsList.core.ts (1)
58-72: NormalizecontextKeysorder in the query key
contextKeysare documented as order-insensitive, so different permutations would currently generate different cache keys. Sorting before constructing the key avoids cache fragmentation for equivalent requests.♻️ Proposed update to normalize contextKeys
export function queryKeyTopicsSubscriptionsList( topicKey: string, parameters: { after?: string | undefined; before?: string | undefined; limit?: number | undefined; orderDirection?: operations.TopicsControllerListTopicSubscriptionsQueryParamOrderDirection | undefined; orderBy?: string | undefined; includeCursor?: boolean | undefined; subscriberId?: string | undefined; contextKeys?: Array<string> | undefined; idempotencyKey?: string | undefined; } ): QueryKey { - return ['@novu/api', 'Subscriptions', 'list', topicKey, parameters]; + const normalizedContextKeys = parameters.contextKeys + ? [...parameters.contextKeys].sort() + : undefined; + + return [ + '@novu/api', + 'Subscriptions', + 'list', + topicKey, + { + ...parameters, + contextKeys: normalizedContextKeys, + }, + ]; }
1 participant
What changed? Why was the change needed?
This pull request adds support for context-aware topic subscriptions, allowing subscriptions to be scoped and filtered by context keys (such as tenant or project). This enables more granular management and querying of topic subscriptions based on contextual information. The changes include updates to DTOs, API request/response validation, filtering logic, and comprehensive end-to-end tests for context-aware behaviors.
The most important changes are:
Context-aware subscription support:
contextKeysfield to relevant DTOs (SubscriptionDetailsResponseDto,SubscriptionResponseDto,SubscriptionDto,TopicSubscriptionResponseDto) to represent the context that scopes a subscription. This is now included in API responses and documented via Swagger decorators. [1] [2] [3] [4]contextpayload, validate it, and map it tocontextKeysfor storage and downstream use. [1] [2] [3] [4] [5] [6]Filtering and querying by context:
ListSubscriberSubscriptionsQueryDto,ListTopicSubscriptionsQueryDto) to support filtering subscriptions by exactcontextKeys(order-insensitive), with appropriate transformation and validation. [1] [2]contextKeysis undefined, all subscriptions are returned; if an empty array, only subscriptions with no context are matched; otherwise, only exact context matches are returned for security.contextKeysfrom API requests to the business logic layer.End-to-end tests for context features: