feat(analytics): add amplitude to notifications for emails#1991
feat(analytics): add amplitude to notifications for emails#1991
Conversation
WalkthroughAdds Amplitude analytics to notifications: new dependencies, env keys, Amplitude and Hash providers, AnalyticsService with deterministic sampling, EmailSenderService integration and tests, and stricter sampling validation in the API analytics service. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant ES as EmailSenderService
participant Novu as Novu API
participant AS as AnalyticsService
participant Amp as Amplitude SDK
ES->>Novu: trigger email send
alt Success
Novu-->>ES: success
ES->>AS: track(userId, "email_sent", props)
AS->>AS: shouldSampleUser(userId)
alt Sampled
AS->>Amp: amplitude.track(event, { user_id, ...props })
AS-->>ES: tracked
else Not sampled
AS-->>ES: skipped
end
else Failure
Novu-->>ES: error
ES-->>ES: rethrow (no analytics tracking)
end
sequenceDiagram
autonumber
participant DI as Nest DI
participant CFG as ConfigService
participant AmpSDK as Amplitude SDK
participant Hasher as murmurhash
DI->>CFG: getOrThrow("notifications.AMPLITUDE_API_KEY")
CFG-->>DI: API_KEY
DI->>AmpSDK: init(API_KEY)
Note right of AmpSDK: AmplitudeProvider created
DI->>Hasher: provide HASHER = murmurhash.v3
Note right of Hasher: HashProvider created
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (3)**/*.spec.{ts,tsx}📄 CodeRabbit inference engine (.cursor/rules/no-jest-mock.mdc)
Files:
**/*.{ts,tsx}📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Files:
**/*.{js,ts,tsx}📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Files:
🧠 Learnings (2)📚 Learning: 2025-07-21T08:25:07.474ZApplied to files:
📚 Learning: 2025-07-21T08:25:07.474ZApplied to files:
🧬 Code graph analysis (2)apps/notifications/src/modules/notifications/services/email-sender/email-sender.service.spec.ts (2)
apps/notifications/src/modules/notifications/services/email-sender/email-sender.service.ts (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
🔇 Additional comments (5)
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: 6
🧹 Nitpick comments (5)
apps/notifications/src/modules/notifications/providers/amplitude.provider.ts (1)
13-17: Consider configuring Amplitude initialization options.The SDK is initialized without any options. For a server-side analytics service, you may want to configure flush intervals, batch sizes, or retry behavior to optimize performance and reliability.
Apply this diff to add recommended server-side options:
useFactory: (configService: ConfigService<NotificationsConfig>) => { const apiKey = configService.getOrThrow("notifications.AMPLITUDE_API_KEY"); - amplitude.init(apiKey); + amplitude.init(apiKey, { + flushIntervalMillis: 10000, + flushQueueSize: 100, + flushMaxRetries: 3 + }); return { track: amplitude.track }; },Based on learnings: The modern
@amplitude/analytics-nodeSDK supports configuration options for batching and retry logic, which improve server-side event delivery reliability.apps/notifications/src/modules/notifications/services/analytics/analytics.service.ts (3)
25-27: Consider adding error handling for Amplitude track failures.The
amplitude.trackcall can throw errors (e.g., network issues, invalid payload). Currently, these errors would propagate and could disrupt the email sending flow. Consider wrapping the call in a try-catch to log errors without breaking the primary operation.Apply this diff:
track(userId: string, eventName: AnalyticsEvent, eventProperties: Record<string, unknown> = {}) { if (this.shouldSampleUser(userId)) { - this.amplitude.track(eventName, eventProperties, { - user_id: userId - }); - this.loggerService.debug({ event: "ANALYTICS_EVENT_REPORTED", userId, eventName }); + try { + this.amplitude.track(eventName, eventProperties, { + user_id: userId + }); + this.loggerService.debug({ event: "ANALYTICS_EVENT_REPORTED", userId, eventName }); + } catch (error) { + this.loggerService.error({ event: "ANALYTICS_EVENT_FAILED", userId, eventName, error }); + } } }Based on learnings: The Amplitude Node SDK batches events and flushes asynchronously, so track() itself is typically non-blocking, but defensive error handling ensures analytics failures don't impact core functionality.
32-36: Consider usinggetOrThrowor providing a default forAMPLITUDE_SAMPLING.Line 35 uses
configService.get(...)which returnsundefinedif the key is missing. This would result inNaNwhen multiplied by 100, causing the comparison to always be false and disabling sampling entirely. While the environment config likely defines a default, usinggetOrThroworconfigService.get(...) ?? 1.0would make the behavior explicit and prevent silent failures.Apply this diff to use
getOrThrow:- return percentage < this.configService.get("notifications.AMPLITUDE_SAMPLING") * 100; + return percentage < this.configService.getOrThrow("notifications.AMPLITUDE_SAMPLING") * 100;Or provide an explicit default:
- return percentage < this.configService.get("notifications.AMPLITUDE_SAMPLING") * 100; + return percentage < (this.configService.get("notifications.AMPLITUDE_SAMPLING") ?? 1.0) * 100;
28-28: Consider PII implications of logginguserId.The debug log includes
userId, which may be personally identifiable information. While debug logs are typically disabled in production and theuserIdis already sent to Amplitude, consider whether logging it is necessary or if it should be redacted/hashed for additional privacy protection.apps/notifications/src/modules/notifications/services/analytics/analytics.service.spec.ts (1)
49-61: Fix the setup helper signature. Our testing guidelines require thesetuphelper to accept a single parameter with an inline type. Please change this to something likeasync function setup({ sampling = "1.0" }: { sampling?: string }) { ... }so future overrides remain type-safe and the convention stays consistent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/notifications/package.json(2 hunks)apps/notifications/src/modules/notifications/config/env.config.ts(1 hunks)apps/notifications/src/modules/notifications/notifications.module.ts(1 hunks)apps/notifications/src/modules/notifications/providers/amplitude.provider.ts(1 hunks)apps/notifications/src/modules/notifications/providers/hash.provider.ts(1 hunks)apps/notifications/src/modules/notifications/services/analytics/analytics.service.spec.ts(1 hunks)apps/notifications/src/modules/notifications/services/analytics/analytics.service.ts(1 hunks)apps/notifications/src/modules/notifications/services/email-sender/email-sender.service.spec.ts(4 hunks)apps/notifications/src/modules/notifications/services/email-sender/email-sender.service.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never use type any or cast to type any. Always define the proper TypeScript types.
Files:
apps/notifications/src/modules/notifications/services/email-sender/email-sender.service.tsapps/notifications/src/modules/notifications/providers/hash.provider.tsapps/notifications/src/modules/notifications/providers/amplitude.provider.tsapps/notifications/src/modules/notifications/config/env.config.tsapps/notifications/src/modules/notifications/services/analytics/analytics.service.spec.tsapps/notifications/src/modules/notifications/services/analytics/analytics.service.tsapps/notifications/src/modules/notifications/notifications.module.tsapps/notifications/src/modules/notifications/services/email-sender/email-sender.service.spec.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{js,ts,tsx}: Never use deprecated methods from libraries.
Don't add unnecessary comments to the code
Files:
apps/notifications/src/modules/notifications/services/email-sender/email-sender.service.tsapps/notifications/src/modules/notifications/providers/hash.provider.tsapps/notifications/src/modules/notifications/providers/amplitude.provider.tsapps/notifications/src/modules/notifications/config/env.config.tsapps/notifications/src/modules/notifications/services/analytics/analytics.service.spec.tsapps/notifications/src/modules/notifications/services/analytics/analytics.service.tsapps/notifications/src/modules/notifications/notifications.module.tsapps/notifications/src/modules/notifications/services/email-sender/email-sender.service.spec.ts
**/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/no-jest-mock.mdc)
Don't use
jest.mock()to mock dependencies in test files. Instead, usejest-mock-extendedto create mocks and pass mocks as dependencies to the service under test.
**/*.spec.{ts,tsx}: Usesetupfunction instead ofbeforeEachin test files
setupfunction must be at the bottom of the rootdescribeblock in test files
setupfunction creates an object under test and returns it
setupfunction should accept a single parameter with inline type definition
Don't use shared state insetupfunction
Don't specify return type ofsetupfunction
Files:
apps/notifications/src/modules/notifications/services/analytics/analytics.service.spec.tsapps/notifications/src/modules/notifications/services/email-sender/email-sender.service.spec.ts
🧬 Code graph analysis (6)
apps/notifications/src/modules/notifications/services/email-sender/email-sender.service.ts (3)
apps/api/src/core/services/config/config.service.ts (1)
ConfigService(9-22)apps/notifications/src/lib/types/namespaced-config.type.ts (1)
Namespaced(1-3)apps/deploy-web/src/services/analytics/analytics.service.ts (1)
AnalyticsService(142-271)
apps/notifications/src/modules/notifications/providers/amplitude.provider.ts (1)
apps/api/src/core/services/config/config.service.ts (1)
ConfigService(9-22)
apps/notifications/src/modules/notifications/services/analytics/analytics.service.spec.ts (5)
apps/notifications/src/modules/notifications/providers/amplitude.provider.ts (1)
Amplitude(7-9)apps/notifications/src/modules/notifications/providers/hash.provider.ts (1)
Hasher(4-4)apps/api/src/core/services/config/config.service.ts (1)
ConfigService(9-22)apps/notifications/src/lib/types/namespaced-config.type.ts (1)
Namespaced(1-3)apps/notifications/src/modules/notifications/config/env.config.ts (1)
NotificationEnvConfig(10-10)
apps/notifications/src/modules/notifications/services/analytics/analytics.service.ts (6)
apps/notifications/src/modules/notifications/services/email-sender/email-sender.service.ts (1)
Injectable(17-64)apps/notifications/src/modules/notifications/providers/amplitude.provider.ts (1)
Amplitude(7-9)apps/notifications/src/modules/notifications/providers/hash.provider.ts (1)
Hasher(4-4)apps/api/src/core/services/config/config.service.ts (1)
ConfigService(9-22)apps/notifications/src/lib/types/namespaced-config.type.ts (1)
Namespaced(1-3)apps/notifications/src/modules/notifications/config/env.config.ts (1)
NotificationEnvConfig(10-10)
apps/notifications/src/modules/notifications/notifications.module.ts (4)
apps/notifications/src/modules/notifications/config/env.config.ts (1)
schema(3-8)apps/notifications/src/modules/notifications/providers/amplitude.provider.ts (1)
AmplitudeProvider(11-19)apps/notifications/src/modules/notifications/providers/hash.provider.ts (1)
HashProvider(6-9)apps/deploy-web/src/services/analytics/analytics.service.ts (1)
AnalyticsService(142-271)
apps/notifications/src/modules/notifications/services/email-sender/email-sender.service.spec.ts (2)
apps/deploy-web/src/services/analytics/analytics.service.ts (1)
AnalyticsService(142-271)apps/notifications/test/mocks/provider.mock.ts (1)
MockProvider(4-6)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: validate / validate-app
- GitHub Check: test-build
🔇 Additional comments (9)
apps/notifications/src/modules/notifications/services/email-sender/email-sender.service.spec.ts (4)
20-54: LGTM! Analytics tracking verified on email success.The test correctly verifies that
analyticsService.trackis called with the expected event name"email_sent"and payload when an email is sent successfully. The assertions cover all relevant properties:recipient_count,subject, andworkflow_id.
56-77: LGTM! Analytics tracking verified on email failure.The test correctly verifies that analytics tracking occurs even when the email send fails. The error message is properly captured in the event payload, and the test uses
rejects.toThrowto verify the error is still propagated.
80-85: Remove the explicit return type fromsetup.The coding guidelines specify that the
setupfunction should not have an explicit return type.As per coding guidelines.
Apply this diff:
- async function setup(): Promise<{ - service: EmailSenderService; - novu: MockProxy<Novu>; - analyticsService: MockProxy<AnalyticsService>; - novuWorkflowId: string; - }> { + async function setup() {
20-77: LGTM! Comprehensive test coverage for analytics integration.The tests correctly verify that:
- Analytics tracking occurs on successful email sends with the
email_sentevent.- Analytics tracking occurs on failures with the
email_failedevent and error details.- Both tests properly use the arrange-act-assert pattern and validate the complete payload structure.
apps/notifications/src/modules/notifications/providers/amplitude.provider.ts (3)
7-9: LGTM! Minimal type surface matches actual usage.The
Amplitudetype intentionally exposes only thetrackmethod, which aligns with howAnalyticsServiceuses the provider. This restricted interface clarifies the intended API surface while the factory returns the full SDK instance.
11-19: LGTM! Clean provider factory with appropriate error handling.The provider correctly uses
getOrThrowto ensure the API key is present, failing fast at startup if missing. The factory pattern and dependency injection are properly structured.
15-15: Amplitude.init is synchronous; no await needed. init() configures the SDK synchronously and events are queued for async sending, so you can calltrackimmediately.apps/notifications/src/modules/notifications/services/analytics/analytics.service.ts (2)
12-36: LGTM! Clean service design with correct deterministic sampling.The service is well-structured with:
- Proper dependency injection and logger context setup.
- Type-safe event names via the
AnalyticsEventunion type.- A correct deterministic sampling algorithm that ensures consistent behavior per user (hash-based modulo 100 distribution).
The sampling logic will correctly sample users based on the configured percentage, with the same user always getting the same sampling decision.
25-27: No changes required for theamplitude.trackcall signature. The@amplitude/analytics-nodeSDK supports thetrack(eventName, eventProperties, options)form used here.
apps/notifications/src/modules/notifications/config/env.config.ts
Outdated
Show resolved
Hide resolved
apps/notifications/src/modules/notifications/providers/amplitude.provider.ts
Show resolved
Hide resolved
| expect(amplitude.track).toHaveBeenCalledWith("email_failed", { error: "Network timeout" }, { user_id: "user123" }); | ||
| expect(loggerService.debug).toHaveBeenCalledWith({ |
There was a problem hiding this comment.
Align expectations with real Amplitude Node API. @amplitude/analytics-node exposes track(eventObject); it doesn't accept (eventName, props, options) like the browser SDK. The assertions (and corresponding service implementation) currently expect that signature, so the tests will fail once the correct client is wired. Please update the service/tests to send { event_type, user_id, event_properties } via a client instance returned by init.
🤖 Prompt for AI Agents
In
apps/notifications/src/modules/notifications/services/analytics/analytics.service.spec.ts
around lines 41-42, the test expects amplitude.track to be called with
(eventName, props, options) but @amplitude/analytics-node requires a single
event object; update the test (and the service if necessary) to call
amplitude.track({ event_type: "email_failed", user_id: "user123",
event_properties: { error: "Network timeout" } }) and change the expectation to
toHaveBeenCalledWith that single object returned by the init client instance.
apps/notifications/src/modules/notifications/services/analytics/analytics.service.ts
Outdated
Show resolved
Hide resolved
apps/notifications/src/modules/notifications/services/analytics/analytics.service.ts
Show resolved
Hide resolved
apps/notifications/src/modules/notifications/services/email-sender/email-sender.service.spec.ts
Outdated
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1991 +/- ##
==========================================
+ Coverage 45.46% 45.53% +0.07%
==========================================
Files 1005 1008 +3
Lines 28506 28560 +54
Branches 7457 7474 +17
==========================================
+ Hits 12961 13006 +45
+ Misses 15163 14343 -820
- Partials 382 1211 +829
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/notifications/src/modules/notifications/config/env.config.ts (1)
7-7: Clamp AMPLITUDE_SAMPLING between 0 and 1.We still accept values outside the [0, 1] range, which breaks sampling semantics. Enforce the bounds when coercing so misconfigured envs can’t silently slip through.
- AMPLITUDE_SAMPLING: z.number({ coerce: true }).optional().default(1) + AMPLITUDE_SAMPLING: z.coerce.number().min(0).max(1).default(1)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
apps/api/src/core/services/analytics/analytics.service.ts(2 hunks)apps/notifications/src/modules/notifications/config/env.config.ts(1 hunks)apps/notifications/src/modules/notifications/services/analytics/analytics.service.ts(1 hunks)apps/notifications/src/modules/notifications/services/email-sender/email-sender.service.spec.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/notifications/src/modules/notifications/services/email-sender/email-sender.service.spec.ts
- apps/notifications/src/modules/notifications/services/analytics/analytics.service.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never use type any or cast to type any. Always define the proper TypeScript types.
Files:
apps/notifications/src/modules/notifications/config/env.config.tsapps/api/src/core/services/analytics/analytics.service.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{js,ts,tsx}: Never use deprecated methods from libraries.
Don't add unnecessary comments to the code
Files:
apps/notifications/src/modules/notifications/config/env.config.tsapps/api/src/core/services/analytics/analytics.service.ts
🧬 Code graph analysis (1)
apps/api/src/core/services/analytics/analytics.service.ts (3)
apps/api/src/core/providers/amplitude.provider.ts (2)
AMPLITUDE(6-6)Amplitude(16-18)apps/deploy-web/src/services/analytics/analytics.service.ts (2)
Amplitude(138-138)AnalyticsService(142-271)apps/api/src/core/providers/hash.provider.ts (2)
HASHER(4-4)Hasher(8-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
apps/notifications/src/modules/notifications/services/analytics/analytics.service.spec.ts (2)
17-17: Amplitude API signature mismatch (previously flagged).The test expects
amplitude.track(eventName, props, options)but@amplitude/analytics-noderequires a single event object:amplitude.track({ event_type, user_id, event_properties }). This issue was already flagged in previous review comments.
46-46: Amplitude API signature mismatch (previously flagged).The test expects
amplitude.track(eventName, props, options)but@amplitude/analytics-noderequires a single event object. This issue was already flagged in previous review comments on these lines.
🧹 Nitpick comments (2)
apps/notifications/src/modules/notifications/services/analytics/analytics.service.spec.ts (1)
25-39: Refactor to use setup function with parameters.This test creates mocks inline instead of using the
setupfunction, which is inconsistent with the other tests. Refactor it to usesetupwith a parameter to configure the sampling behavior.Apply this diff to refactor the test:
- it("should not track events when user is not sampled", async () => { - const amplitude = mock<Amplitude>(); - const hasher = mock<Hasher>(); - const configService = mock<ConfigService<Namespaced<"notifications", NotificationEnvConfig>>>(); - const loggerService = mock<LoggerService>(); - - jest.spyOn(configService, "getOrThrow").mockReturnValue("0.0"); - hasher.hash.mockReturnValue(50); // Hash value that would normally be sampled - - const service = new AnalyticsService(amplitude, hasher, configService, loggerService); + it("should not track events when user is not sampled", async () => { + const { service, amplitude } = await setup({ samplingRate: "0.0" }); service.track("user123", "email_sent", { recipient_count: 1 }); expect(amplitude.track).not.toHaveBeenCalled(); });And update the setup function to accept parameters:
- async function setup() { + async function setup(options?: { samplingRate?: string }) { const amplitude = mock<Amplitude>(); const hasher = mock<Hasher>(); const configService = mock<ConfigService<Namespaced<"notifications", NotificationEnvConfig>>>(); const loggerService = mock<LoggerService>(); - jest.spyOn(configService, "getOrThrow").mockReturnValue("1.0"); + jest.spyOn(configService, "getOrThrow").mockReturnValue(options?.samplingRate ?? "1.0"); hasher.hash.mockReturnValue(50); // Mock hash value that will be sampled const service = new AnalyticsService(amplitude, hasher, configService, loggerService); return { service, amplitude, hasher, configService, loggerService }; }apps/api/src/core/services/analytics/analytics.service.spec.ts (1)
151-168: Setup function is compliant with coding guidelines.The function correctly:
- Accepts a single parameter with inline type definition
- Has no return type annotation
- Uses
jest-mock-extendedfor mocking- Has no shared state
- Is positioned at the bottom of the root describe block
However, consider making the config mock more specific to prevent future issues:
Apply this diff to make the config mock more specific:
- config.get.mockReturnValue(input.samplingRate ?? "1.0"); + config.get.calledWith("AMPLITUDE_SAMPLING").mockReturnValue(input.samplingRate ?? "1.0");This ensures the mock only returns the sampling rate when specifically querying
"AMPLITUDE_SAMPLING", preventing unexpected behavior if the service callsconfig.getwith other keys in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/api/src/core/services/analytics/analytics.service.spec.ts(6 hunks)apps/notifications/src/modules/notifications/services/analytics/analytics.service.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/no-jest-mock.mdc)
Don't use
jest.mock()to mock dependencies in test files. Instead, usejest-mock-extendedto create mocks and pass mocks as dependencies to the service under test.
**/*.spec.{ts,tsx}: Usesetupfunction instead ofbeforeEachin test files
setupfunction must be at the bottom of the rootdescribeblock in test files
setupfunction creates an object under test and returns it
setupfunction should accept a single parameter with inline type definition
Don't use shared state insetupfunction
Don't specify return type ofsetupfunction
Files:
apps/api/src/core/services/analytics/analytics.service.spec.tsapps/notifications/src/modules/notifications/services/analytics/analytics.service.spec.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never use type any or cast to type any. Always define the proper TypeScript types.
Files:
apps/api/src/core/services/analytics/analytics.service.spec.tsapps/notifications/src/modules/notifications/services/analytics/analytics.service.spec.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{js,ts,tsx}: Never use deprecated methods from libraries.
Don't add unnecessary comments to the code
Files:
apps/api/src/core/services/analytics/analytics.service.spec.tsapps/notifications/src/modules/notifications/services/analytics/analytics.service.spec.ts
🧬 Code graph analysis (2)
apps/api/src/core/services/analytics/analytics.service.spec.ts (2)
apps/api/src/core/providers/amplitude.provider.ts (1)
Amplitude(16-18)apps/api/src/core/providers/hash.provider.ts (1)
Hasher(8-8)
apps/notifications/src/modules/notifications/services/analytics/analytics.service.spec.ts (5)
apps/notifications/src/modules/notifications/providers/amplitude.provider.ts (1)
Amplitude(7-9)apps/notifications/src/modules/notifications/providers/hash.provider.ts (1)
Hasher(4-4)apps/api/src/core/services/config/config.service.ts (1)
ConfigService(9-22)apps/notifications/src/lib/types/namespaced-config.type.ts (1)
Namespaced(1-3)apps/notifications/src/modules/notifications/config/env.config.ts (1)
NotificationEnvConfig(10-10)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: validate (apps/stats-web) / validate-unsafe
- GitHub Check: validate (apps/api) / validate-unsafe
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
🔇 Additional comments (4)
apps/api/src/core/services/analytics/analytics.service.spec.ts (4)
1-8: LGTM! Clean imports with proper type safety.The imports correctly use
jest-mock-extendedfor mocking and properly import types using thetypekeyword, which is good for tree-shaking and clarity.
10-17: LGTM! Constructor test is properly structured.The test correctly verifies that the logger context is set during service initialization.
19-78: LGTM! Track method tests are comprehensive and correct.All three test cases properly validate:
- Event tracking when user is sampled (hash < threshold)
- Event skipping when user is not sampled (hash >= threshold)
- Default empty object for properties when none provided
The sampling logic is tested correctly with the explicit
samplingRateparameter.
80-149: LGTM! Comprehensive coverage of sampling logic edge cases.The tests thoroughly validate:
- Sampling decisions at the threshold boundary
- 100% sampling rate (all users included)
- 0% sampling rate (all users excluded)
- Negative hash value handling
All assertions correctly reflect the expected deterministic sampling behavior based on hash values and thresholds.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/notifications/env/.env.test (1)
10-11: Silence dotenv-linter by fixing order and trailing newline.
dotenv-lintercurrently warns that the new key is out of order and the file lacks a trailing newline; please reorder the entry and re-add the newline so the check passes cleanly.RPC_NODE_ENDPOINT=https://consolerpc.akashnet.net -API_NODE_ENDPOINT=https://consoleapi.akashnet.net +AMPLITUDE_API_KEY=AMPLITUDE_API_KEY +API_NODE_ENDPOINT=https://consoleapi.akashnet.net APP_NAME=local POSTGRES_BASE_URL=postgres://postgres:password@localhost:5432 EVENT_BROKER_POSTGRES_URI=postgres://postgres:password@localhost:5432/events NOTIFICATIONS_POSTGRES_URL=postgres://postgres:password@localhost:5432/notification_service NOVU_SECRET_KEY=some-dummy-string NOVU_MAILER_WORKFLOW_ID=generic-v2 CONSOLE_WEB_URL=console.akash.network STD_OUT_LOG_FORMAT=pretty -AMPLITUDE_API_KEY=AMPLITUDE_API_KEY +
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/notifications/env/.env.test(1 hunks)
🧰 Additional context used
🪛 dotenv-linter (3.3.0)
apps/notifications/env/.env.test
[warning] 11-11: [EndingBlankLine] No blank line at the end of the file
(EndingBlankLine)
[warning] 11-11: [UnorderedKey] The AMPLITUDE_API_KEY key should go before the API_NODE_ENDPOINT key
(UnorderedKey)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: test-build
apps/notifications/src/modules/notifications/services/email-sender/email-sender.service.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/notifications/src/modules/notifications/services/email-sender/email-sender.service.spec.ts(4 hunks)apps/notifications/src/modules/notifications/services/email-sender/email-sender.service.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never use type any or cast to type any. Always define the proper TypeScript types.
Files:
apps/notifications/src/modules/notifications/services/email-sender/email-sender.service.tsapps/notifications/src/modules/notifications/services/email-sender/email-sender.service.spec.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{js,ts,tsx}: Never use deprecated methods from libraries.
Don't add unnecessary comments to the code
Files:
apps/notifications/src/modules/notifications/services/email-sender/email-sender.service.tsapps/notifications/src/modules/notifications/services/email-sender/email-sender.service.spec.ts
**/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/no-jest-mock.mdc)
Don't use
jest.mock()to mock dependencies in test files. Instead, usejest-mock-extendedto create mocks and pass mocks as dependencies to the service under test.
**/*.spec.{ts,tsx}: Usesetupfunction instead ofbeforeEachin test files
setupfunction must be at the bottom of the rootdescribeblock in test files
setupfunction creates an object under test and returns it
setupfunction should accept a single parameter with inline type definition
Don't use shared state insetupfunction
Don't specify return type ofsetupfunction
Files:
apps/notifications/src/modules/notifications/services/email-sender/email-sender.service.spec.ts
🧠 Learnings (2)
📚 Learning: 2025-07-21T08:25:07.474Z
Learnt from: CR
PR: akash-network/console#0
File: .cursor/rules/setup-instead-of-before-each.mdc:0-0
Timestamp: 2025-07-21T08:25:07.474Z
Learning: Applies to **/*.spec.{ts,tsx} : Don't specify return type of `setup` function
Applied to files:
apps/notifications/src/modules/notifications/services/email-sender/email-sender.service.spec.ts
📚 Learning: 2025-07-21T08:25:07.474Z
Learnt from: CR
PR: akash-network/console#0
File: .cursor/rules/setup-instead-of-before-each.mdc:0-0
Timestamp: 2025-07-21T08:25:07.474Z
Learning: Applies to **/*.spec.{ts,tsx} : `setup` function should accept a single parameter with inline type definition
Applied to files:
apps/notifications/src/modules/notifications/services/email-sender/email-sender.service.spec.ts
🧬 Code graph analysis (2)
apps/notifications/src/modules/notifications/services/email-sender/email-sender.service.ts (3)
apps/api/src/core/services/config/config.service.ts (1)
ConfigService(9-22)apps/notifications/src/lib/types/namespaced-config.type.ts (1)
Namespaced(1-3)apps/deploy-web/src/services/analytics/analytics.service.ts (1)
AnalyticsService(142-271)
apps/notifications/src/modules/notifications/services/email-sender/email-sender.service.spec.ts (2)
apps/deploy-web/src/services/analytics/analytics.service.ts (1)
AnalyticsService(142-271)apps/notifications/test/mocks/provider.mock.ts (1)
MockProvider(4-6)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: provider-proxy-ci
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
🔇 Additional comments (6)
apps/notifications/src/modules/notifications/services/email-sender/email-sender.service.ts (2)
8-8: LGTM! Standard dependency injection.The import and injection of
AnalyticsServicefollows the established NestJS pattern consistently with other dependencies.Also applies to: 22-22
21-21: LGTM! Improved type safety.Adding the explicit type parameter to
ConfigServiceprovides compile-time safety for configuration access and aligns with the established pattern.apps/notifications/src/modules/notifications/services/email-sender/email-sender.service.spec.ts (4)
8-8: LGTM! Standard import.The import of
AnalyticsServicefor testing purposes is correct.
20-21: LGTM! Clear test intent.The test name accurately reflects the behavior being verified, and the destructuring pattern is clean.
49-53: LGTM! Comprehensive assertion.The test thoroughly verifies that analytics tracking is called with the correct userId, event name, and all expected properties.
57-57: LGTM! Adheres to coding guidelines.The setup function correctly:
- Omits the explicit return type (as per coding guidelines)
- Registers
AnalyticsServiceas a mocked provider- Returns the mocked
analyticsServicefor test assertionsAs per coding guidelines and learnings.
Also applies to: 59-59, 73-73
apps/notifications/src/modules/notifications/services/email-sender/email-sender.service.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/notifications/src/modules/notifications/services/email-sender/email-sender.service.spec.ts(4 hunks)apps/notifications/src/modules/notifications/services/email-sender/email-sender.service.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/notifications/src/modules/notifications/services/email-sender/email-sender.service.spec.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never use type any or cast to type any. Always define the proper TypeScript types.
Files:
apps/notifications/src/modules/notifications/services/email-sender/email-sender.service.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{js,ts,tsx}: Never use deprecated methods from libraries.
Don't add unnecessary comments to the code
Files:
apps/notifications/src/modules/notifications/services/email-sender/email-sender.service.ts
🧬 Code graph analysis (1)
apps/notifications/src/modules/notifications/services/email-sender/email-sender.service.ts (4)
apps/api/src/core/services/config/config.service.ts (1)
ConfigService(9-22)apps/notifications/src/lib/types/namespaced-config.type.ts (1)
Namespaced(1-3)apps/deploy-web/src/services/analytics/analytics.service.ts (1)
AnalyticsService(142-271)packages/logging/src/services/logger/logger.service.ts (1)
error(107-109)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
🔇 Additional comments (2)
apps/notifications/src/modules/notifications/services/email-sender/email-sender.service.ts (2)
6-6: LGTM!The imports for
LoggerServiceandAnalyticsServiceare correctly added and follow the project's import conventions.Also applies to: 9-9
20-27: LGTM!The constructor correctly injects
AnalyticsServiceandLoggerServicefollowing NestJS patterns, and properly initializes the logger context.
apps/notifications/src/modules/notifications/services/email-sender/email-sender.service.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/api/src/core/services/analytics/analytics.service.ts (1)
40-53: Restore default sampling rate when unset.As flagged in a previous review,
validateSamplingRate()now throws whenAMPLITUDE_SAMPLINGis missing, whereas the previous logic defaulted to 1. Any environment (tests, dev, CI) that hasn't set this variable will crash during DI resolution.Apply this diff to default to 1 when undefined while still validating range:
private validateSamplingRate(): number { const rawValue = this.coreConfigService.get("AMPLITUDE_SAMPLING"); + + if (rawValue === undefined) { + return 1; + } + const samplingRate = Number(rawValue); if (!Number.isFinite(samplingRate)) { throw new Error(`Invalid AMPLITUDE_SAMPLING value: "${rawValue}". Must be a finite number.`); } if (samplingRate < 0 || samplingRate > 1) { throw new Error(`Invalid AMPLITUDE_SAMPLING value: ${samplingRate}. Must be between 0 and 1 (inclusive).`); } return samplingRate; }
🧹 Nitpick comments (1)
apps/api/src/core/services/analytics/analytics.service.ts (1)
24-32: Remove redundant cast.The
eventPropertiesparameter is already typed asRecord<string, unknown>, making the cast on line 26 redundant.Apply this diff to remove the unnecessary cast:
track(userId: string, eventName: AnalyticsEvent, eventProperties: Record<string, unknown> = {}) { if (this.shouldSampleUser(userId)) { - const amplitudeProperties = eventProperties as Record<string, unknown>; - this.amplitude.track(eventName, amplitudeProperties, { + this.amplitude.track(eventName, eventProperties, { user_id: userId }); this.loggerService.debug({ event: "ANALYTICS_EVENT_REPORTED", userId, eventName }); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/api/src/core/services/analytics/analytics.service.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never use type any or cast to type any. Always define the proper TypeScript types.
Files:
apps/api/src/core/services/analytics/analytics.service.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{js,ts,tsx}: Never use deprecated methods from libraries.
Don't add unnecessary comments to the code
Files:
apps/api/src/core/services/analytics/analytics.service.ts
🧬 Code graph analysis (1)
apps/api/src/core/services/analytics/analytics.service.ts (3)
apps/api/src/core/providers/amplitude.provider.ts (2)
AMPLITUDE(6-6)Amplitude(16-18)apps/deploy-web/src/services/analytics/analytics.service.ts (1)
Amplitude(138-138)apps/api/src/core/providers/hash.provider.ts (2)
HASHER(4-4)Hasher(8-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
🔇 Additional comments (2)
apps/api/src/core/services/analytics/analytics.service.ts (2)
12-12: LGTM!The
samplingRatefield is properly declared asprivate readonlyand will be initialized in the constructor.
21-21: LGTM!The
samplingRateis correctly initialized during construction. Note that ifvalidateSamplingRate()throws, DI resolution will fail (see comments on the validation method below).
* feat(analytics): add amplitude to notifications for emails * fix(notifications): pr fixes * fix: analytics tests * fix: add test amplitude key * fix(analytics): remove fail send amplitude * fix(analytics): don't fail on analytics track * fix: don't throw when undefined * fix: logging
* feat(analytics): add amplitude to notifications for emails * fix(notifications): pr fixes * fix: analytics tests * fix: add test amplitude key * fix(analytics): remove fail send amplitude * fix(analytics): don't fail on analytics track * fix: don't throw when undefined * fix: logging
* feat(analytics): add amplitude to notifications for emails * fix(notifications): pr fixes * fix: analytics tests * fix: add test amplitude key * fix(analytics): remove fail send amplitude * fix(analytics): don't fail on analytics track * fix: don't throw when undefined * fix: logging
2 participants
Summary by CodeRabbit
New Features
Tests
Chores