feat: adds more notifications on start trial event#1827
Conversation
WalkthroughAdds a PgBoss-backed job queue, an APP_INITIALIZER lifecycle contract and wiring, DomainEventsService with a TrialStarted event and handler, notification-job templates and handler, getOrCreate wallet flow with event emission, CLI daemon/drain changes, feature-flag startup integration, tests, and two new dependencies. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Process
participant App as App bootstrap
participant DI as DI container
participant DB as Database/migrations
participant FF as FeatureFlagsService
participant JQ as JobQueueService
Process->>App: start()
App->>DI: resolveAll(APP_INITIALIZER)
par initializers + DB
Note left of App #f0f4c3: run all ON_APP_START() in parallel
App->>FF: [ON_APP_START]()
App->>JQ: [ON_APP_START]()
App->>DB: initDb()/migrate & authenticate
end
JQ->>PgBoss: start()
App-->>Process: ready
sequenceDiagram
autonumber
participant WalletInit as WalletInitializerService
participant DE as DomainEventsService
participant JQ as JobQueueService
participant TH as TrialStartedHandler
participant DB as UserRepository
participant NS as NotificationService
participant NH as NotificationHandler
WalletInit->>DE: publish(TrialStarted{userId})
DE->>JQ: enqueue(TrialStarted)
JQ->>TH: process TrialStarted
TH->>DB: findById(userId)
alt user found & has email
TH->>NS: create(start-trial notification)
end
TH->>JQ: enqueue(NotificationJob(s))
loop workers
JQ->>NH: process NotificationJob
NH->>DB: findById(userId)
NH->>NS: createNotification(template(user, vars))
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1827 +/- ##
==========================================
+ Coverage 42.94% 43.25% +0.30%
==========================================
Files 943 953 +10
Lines 26553 26711 +158
Branches 6957 6960 +3
==========================================
+ Hits 11403 11553 +150
+ Misses 14788 14085 -703
- Partials 362 1073 +711
🚀 New features to boost your workflow:
|
stalniy
force-pushed
the
feat/trial-events
branch
2 times, most recently
from
812eb43 to
c3d80ca
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/api/src/notifications/services/notification/notification.service.ts (2)
21-28: Removeas anyon parameters to comply with TS guidelinesCasting to
anyviolates the repo guideline. Use the generated OpenAPI types for parameters.Apply this diff:
- .createNotification({ - parameters: { - header: { - "x-user-id": user.id - } - } as any, - body: notification - }) + .createNotification({ + parameters: { + header: { + "x-user-id": user.id + } + } as operations["createNotification"]["parameters"], + body: notification + })
45-49: Removeas anyin createDefaultChannel parametersSame issue as above; leverage the generated type.
Apply this diff:
- .createDefaultChannel({ - parameters: { - header: { - "x-user-id": user.id - } as any - }, + .createDefaultChannel({ + parameters: { + header: { + "x-user-id": user.id + } + } as operations["createDefaultChannel"]["parameters"], body: {apps/api/src/notifications/services/notification/notification.service.spec.ts (1)
70-73: Add afterEach to restore real timers in this describe blockYou switched timers to fake in tests here but didn’t restore them; this can leak to other tests.
Apply this diff:
describe("createNotification", () => { + afterEach(() => { + jest.useRealTimers(); + }); it("sends notification", async () => { jest.useFakeTimers();
🧹 Nitpick comments (25)
apps/api/src/billing/repositories/user-wallet/user-wallet.repository.ts (1)
71-81: Nit: keep conversions centralized by reusing toInput when insertingNot required here (you only pass userId), but for consistency and future-proofing, consider reusing toInput when building insert values so numeric-to-string conversions remain centralized.
- const value = { - userId: input.userId, - address: input.address - }; + const value = this.toInput({ + userId: input.userId, + address: input.address + });apps/api/src/notifications/services/notification-templates/after-trial-ends-notification.ts (1)
4-16: LGTM, but consider consolidating with trialEnded template to reduce duplicationThis is correct as-is. Given you also have trialEndedNotification with similar copy, consider a single “trialEnd” template with a variant or param to avoid drift between messages.
apps/api/src/core/providers/app-initializer.ts (1)
3-3: Optional: use Symbol.for for cross-module token stabilityIf the token can be referenced across module boundaries/bundles,
Symbol.foravoids accidental duplication. Not required if everything is bundled together.Apply this diff:
-export const APP_INITIALIZER: InjectionToken<AppInitializer> = Symbol("APP_INITIALIZER"); +export const APP_INITIALIZER: InjectionToken<AppInitializer> = Symbol.for("APP_INITIALIZER");apps/api/src/notifications/services/notification/notification.service.ts (1)
51-56: Optional: guard against missing email in createDefaultChannelIf this method is called elsewhere with
user.emailmissing, it will crash at runtime. Consider an explicit guard with a clear error message.Example:
async createDefaultChannel(user: UserInput): Promise<void> { + if (!user.email) { + throw new Error("createDefaultChannel requires a user with an email"); + } await backOff(async () => {apps/api/src/notifications/services/notification/notification.service.spec.ts (4)
128-128: Rename test to match behavior (no logger anymore)The test title mentions logging, which no longer happens. Make the name accurate.
Apply this diff:
-it("does not create default channel when user has no email and logs an error", async () => { +it("does not create default channel when user has no email and rejects with cause", async () => {
149-149: Rename test to remove logging referenceSimilarly, align the title with actual behavior.
Apply this diff:
-it("fails after 10 attempts if notification service is not available and logs an error", async () => { +it("fails after 10 attempts if notification service is not available", async () => {
40-44: Remove duplicateuseFakeTimerscallThere are two consecutive
jest.useFakeTimers()calls in this test; one is enough.Apply this diff:
- jest.useFakeTimers(); const user = { id: "user-1", email: "user@example.com" };
170-175: Align setup helper with repo test conventionsPer repo guidelines,
setupshould accept a single parameter with inline type definition (even if unused).Apply this diff:
- function setup() { + function setup(_: {} = {}) { const api = mockDeep<NotificationsApiClient>(); const service = new NotificationService(api); return { service, api }; }apps/api/test/services/wallet-testing.service.ts (1)
129-141: Side-effect remains for NotificationService.createDefaultChannelUnrelated to this PR but worth noting: this spy ensures no outbound call when registering a user. If the new domain-event flow fully supersedes this, consider removing the dependency on NotificationService here to reduce test coupling.
Would you like me to open a follow-up to remove this spy once the domain-events integration is complete?
apps/api/src/app/providers/jobs.provider.ts (1)
12-12: Make handler registration more scalable via a multi-provider tokenHardcoding handler list here requires touching this file for every new handler. Consider introducing a JOB_HANDLERS injection token and registering each handler under it; then resolveAll(JOB_HANDLERS) here.
I can sketch this pattern if you want (token, registrations, and provider wiring).
apps/api/src/app.ts (1)
202-205: Double-check startup ordering: initDb() runs concurrently with initializersIf any initializer depends on DB readiness (including JobQueueManager start/migrations), running everything in Promise.all may race. If that’s a risk in your env, serialize DB init before the initializers.
Apply if you want deterministic ordering:
- await Promise.all([initDb(), ...container.resolveAll(APP_INITIALIZER).map(initializer => initializer[ON_APP_START]())]); + await initDb(); + await Promise.all(container.resolveAll(APP_INITIALIZER).map(initializer => initializer[ON_APP_START]()));apps/api/src/billing/events/trial-started.ts (1)
9-13: Optional: export a reusable payload typeSmall ergonomics win for handlers/tests that reuse the payload type.
Add alongside the class:
export type TrialStartedPayload = { userId: UserOutput["id"] };and update the constructor to use TrialStartedPayload.
apps/api/src/core/services/domain-events/domain-events.service.ts (2)
6-8: Remove redundant property from DomainEventJob already declares version: number. Re-declaring it here is redundant.
Apply:
-export interface DomainEvent extends Job { - version: number; -} +export interface DomainEvent extends Job {}
14-16: Consider a batch publish APIIf you often emit multiple events together, a publishAll can reduce overhead and improve readability (enqueue in parallel).
Example:
async publishAll(events: DomainEvent[]): Promise<void> { await Promise.all(events.map(e => this.jobQueueManager.enqueue(e))); }apps/api/src/console.ts (3)
112-121: CLI startup: migrations, DB auth, and initializers run concurrentlyParallelizing migratePG(), chainDb.authenticate(), and APP_INITIALIZER may race if any initializer expects migrations or DB connections to be ready.
Safer ordering:
- await Promise.all([migratePG(), chainDb.authenticate(), ...container.resolveAll(APP_INITIALIZER).map(initializer => initializer[ON_APP_START]())]); + await migratePG(); + await chainDb.authenticate(); + await Promise.all(container.resolveAll(APP_INITIALIZER).map(initializer => initializer[ON_APP_START]()));
140-144: Tighten types in daemon wrapperReturn a function typed with OptionValues to match Commander’s options shape and your signature.
Apply:
-function daemon(fn: (options: OptionValues, command: Command) => Promise<unknown>): (options: OptionValues, command: Command) => Promise<void> { - return async (options: Record<string, unknown>, command: Command) => { +function daemon(fn: (options: OptionValues, command: Command) => Promise<unknown>): (options: OptionValues, command: Command) => Promise<void> { + return async (options: OptionValues, command: Command) => { await executeCliHandler(command.name(), () => fn(options, command), { type: "daemon" }); }; }
146-151: Shutdown path: optional logging/error guardsNice one-time shutdown via once(). Consider try/catch around close operations with individual logs to simplify troubleshooting on partial failures. Optional.
apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.ts (2)
28-35: Avoid shadowing by renaming inner wallet variable for readability.The const wallet inside the try block shadows the earlier userWallet variable, making the code harder to follow.
Apply this diff:
- const wallet = await this.walletManager.createAndAuthorizeTrialSpending({ addressIndex: userWallet.id }); + const chainWallet = await this.walletManager.createAndAuthorizeTrialSpending({ addressIndex: userWallet.id }); userWallet = await this.userWalletRepository.updateById( userWallet.id, { - address: wallet.address, - deploymentAllowance: wallet.limits.deployment, - feeAllowance: wallet.limits.fees + address: chainWallet.address, + deploymentAllowance: chainWallet.limits.deployment, + feeAllowance: chainWallet.limits.fees }, { returning: true } );
46-52: Consider soft-failing domain event publishing.If the job queue is down, awaiting publish will fail the whole flow after provisioning the wallet. Depending on UX requirements, you may prefer to log and continue, or enqueue in a best-effort way.
Example pattern:
try { await this.domainEvents.publish(new TrialStarted({ userId })); } catch (err) { this.logger?.warn?.({ event: "TRIAL_STARTED_PUBLISH_FAILED", userId, error: err }); }apps/api/src/app/services/trial-started/trial-started.handler.ts (1)
45-58: Edge case: negative schedule offsets.If TRIAL_ALLOWANCE_EXPIRATION_DAYS < 7, the first reminder will schedule in the past and could run immediately. If that’s undesirable, guard offsets to be >= 0.
Example:
const before7 = Math.max(0, TRIAL_ALLOWANCE_EXPIRATION_DAYS - 7); startAfter: addDays(new Date(), before7);apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts (2)
88-106: Test passes even if event publishes wrong userId (uses currentUser.id). Add a regression test.Because setup aligns currentUser.id with the method argument userId, the test won’t catch mistakenly publishing with the current user instead of the target user. Add a test where the current user differs from the target user and assert that the event’s data.userId equals the target userId.
Example addition:
+ it('publishes TrialStarted for the target user, not the current user', async () => { + const targetUserId = 'target-user-id'; + const currentUserId = 'actor-user-id'; + const newWallet = UserWalletSeeder.create({ userId: targetUserId }); + const di = setup({ + userId: currentUserId, + getOrCreateWallet: jest.fn().mockResolvedValue({ wallet: newWallet, isNew: true }), + createAndAuthorizeTrialSpending: jest.fn().mockResolvedValue(createChainWallet()), + updateWalletById: jest.fn().mockResolvedValue(newWallet), + enabledFeatures: [] + }); + await di.resolve(WalletInitializerService).initializeAndGrantTrialLimits(targetUserId); + expect(di.resolve(DomainEventsService).publish).toHaveBeenCalledWith(new TrialStarted({ userId: targetUserId })); + });
109-157: Good use of jest-mock-extended and setup function placement.
- Uses jest-mock-extended mocks; no jest.mock() usage.
- setup function is at the bottom of the root describe and has an inline-typed parameter.
- No shared mutable state between tests.
One small nit: container.clearInstances() affects the global container. Prefer interacting only with the child container to avoid cross-test side effects.
Replace:
container.clearInstances();with no-op or clean up only the child container if needed.
apps/api/src/core/services/job-queue/job-queue.service.ts (3)
29-39: Potential startup race: callingcreateQueuebeforepgBoss.start().
registerHandlersusesthis.pgBoss.createQueue(...)butstart()is invoked separately via[ON_APP_START]. Unless app initializer execution order is guaranteed,registerHandlerscan run beforestart(), causing failures. You create queues again indrain(), so the first call is also redundant.Options:
- Ensure
JobQueueManager.[ON_APP_START]()runs before any initializer that callsregisterHandlers().- Or move queue creation entirely into
drain()(and remove fromregisterHandlers()).- Or have
registerHandlers()only store handlers;drain()ensures queues and starts workers.Do you want me to refactor to a dedicated
ensureQueues()method and wire it fromdrain()?Also applies to: 125-136
139-143: Consider removingnamefromJobto avoid dual sources of truth.Handlers derive queue names from the static
[JOB_NAME]symbol, whileenqueuecan rely on the same. Keepingnameon the instance risks divergence and subtle routing bugs.If you remove
name:
- Derive the queue name in
enqueueexclusively via(job.constructor as JobType<Job>)[JOB_NAME].- Update job classes accordingly if they were setting
name.
73-76: Reduce PII/noise in logs; prefer job identifiers over full payloads.Current logs include full job payloads on enqueue, start, and on completion/failure. For notification jobs, this may expose user data in logs. Consider logging
queueName,job.id(if available), and minimal metadata.Example (conceptual):
- On enqueue:
{ event: 'JOB_ENQUEUED', queue: name, keys: Object.keys(job.data) }- On started/done/failed:
{ event: 'JOB_DONE', queue: queueName, id: jobs[index].id }Also applies to: 95-99, 103-113
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (22)
apps/api/package.json(2 hunks)apps/api/src/app.ts(3 hunks)apps/api/src/app/providers/jobs.provider.ts(1 hunks)apps/api/src/app/services/trial-started/trial-started.handler.ts(1 hunks)apps/api/src/billing/controllers/wallet/wallet.controller.ts(0 hunks)apps/api/src/billing/events/trial-started.ts(1 hunks)apps/api/src/billing/repositories/user-wallet/user-wallet.repository.ts(2 hunks)apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts(6 hunks)apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.ts(3 hunks)apps/api/src/console.ts(4 hunks)apps/api/src/core/providers/app-initializer.ts(1 hunks)apps/api/src/core/providers/postgres.provider.ts(2 hunks)apps/api/src/core/services/domain-events/domain-events.service.ts(1 hunks)apps/api/src/core/services/feature-flags/feature-flags.service.ts(2 hunks)apps/api/src/core/services/job-queue/job-queue.service.ts(1 hunks)apps/api/src/notifications/services/notification-handler/notification-handler.ts(1 hunks)apps/api/src/notifications/services/notification-templates/after-trial-ends-notification.ts(1 hunks)apps/api/src/notifications/services/notification-templates/before-trial-ends-notification.ts(1 hunks)apps/api/src/notifications/services/notification-templates/trial-ended-notification.ts(1 hunks)apps/api/src/notifications/services/notification/notification.service.spec.ts(2 hunks)apps/api/src/notifications/services/notification/notification.service.ts(2 hunks)apps/api/test/services/wallet-testing.service.ts(2 hunks)
💤 Files with no reviewable changes (1)
- apps/api/src/billing/controllers/wallet/wallet.controller.ts
🧰 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/api/src/notifications/services/notification-templates/trial-ended-notification.tsapps/api/src/app/providers/jobs.provider.tsapps/api/src/notifications/services/notification-templates/after-trial-ends-notification.tsapps/api/src/billing/repositories/user-wallet/user-wallet.repository.tsapps/api/src/notifications/services/notification-templates/before-trial-ends-notification.tsapps/api/src/core/providers/app-initializer.tsapps/api/test/services/wallet-testing.service.tsapps/api/src/core/providers/postgres.provider.tsapps/api/src/notifications/services/notification/notification.service.spec.tsapps/api/src/app/services/trial-started/trial-started.handler.tsapps/api/src/billing/events/trial-started.tsapps/api/src/core/services/domain-events/domain-events.service.tsapps/api/src/notifications/services/notification/notification.service.tsapps/api/src/billing/services/wallet-initializer/wallet-initializer.service.tsapps/api/src/core/services/job-queue/job-queue.service.tsapps/api/src/console.tsapps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.tsapps/api/src/notifications/services/notification-handler/notification-handler.tsapps/api/src/core/services/feature-flags/feature-flags.service.tsapps/api/src/app.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/notifications/services/notification-templates/trial-ended-notification.tsapps/api/src/app/providers/jobs.provider.tsapps/api/src/notifications/services/notification-templates/after-trial-ends-notification.tsapps/api/src/billing/repositories/user-wallet/user-wallet.repository.tsapps/api/src/notifications/services/notification-templates/before-trial-ends-notification.tsapps/api/src/core/providers/app-initializer.tsapps/api/test/services/wallet-testing.service.tsapps/api/src/core/providers/postgres.provider.tsapps/api/src/notifications/services/notification/notification.service.spec.tsapps/api/src/app/services/trial-started/trial-started.handler.tsapps/api/src/billing/events/trial-started.tsapps/api/src/core/services/domain-events/domain-events.service.tsapps/api/src/notifications/services/notification/notification.service.tsapps/api/src/billing/services/wallet-initializer/wallet-initializer.service.tsapps/api/src/core/services/job-queue/job-queue.service.tsapps/api/src/console.tsapps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.tsapps/api/src/notifications/services/notification-handler/notification-handler.tsapps/api/src/core/services/feature-flags/feature-flags.service.tsapps/api/src/app.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/api/src/notifications/services/notification/notification.service.spec.tsapps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts
🧠 Learnings (1)
📚 Learning: 2025-07-21T08:24:24.269Z
Learnt from: CR
PR: akash-network/console#0
File: .cursor/rules/no-jest-mock.mdc:0-0
Timestamp: 2025-07-21T08:24:24.269Z
Learning: Applies to **/*.spec.{ts,tsx} : Don't use `jest.mock()` to mock dependencies in test files. Instead, use `jest-mock-extended` to create mocks and pass mocks as dependencies to the service under test.
Applied to files:
apps/api/src/notifications/services/notification/notification.service.spec.ts
🧬 Code Graph Analysis (17)
apps/api/src/notifications/services/notification-templates/trial-ended-notification.ts (1)
apps/api/src/notifications/services/notification/notification.service.ts (1)
CreateNotificationInput(74-76)
apps/api/src/app/providers/jobs.provider.ts (2)
apps/api/src/core/providers/app-initializer.ts (2)
APP_INITIALIZER(3-3)ON_APP_START(4-4)apps/api/src/core/services/job-queue/job-queue.service.ts (1)
ON_APP_START(134-136)
apps/api/src/notifications/services/notification-templates/after-trial-ends-notification.ts (1)
apps/api/src/notifications/services/notification/notification.service.ts (1)
CreateNotificationInput(74-76)
apps/api/src/notifications/services/notification-templates/before-trial-ends-notification.ts (1)
apps/api/src/notifications/services/notification/notification.service.ts (1)
CreateNotificationInput(74-76)
apps/api/src/core/providers/app-initializer.ts (3)
apps/api/src/app/providers/jobs.provider.ts (1)
ON_APP_START(10-13)apps/api/src/core/services/feature-flags/feature-flags.service.ts (1)
ON_APP_START(80-82)apps/api/src/core/services/job-queue/job-queue.service.ts (1)
ON_APP_START(134-136)
apps/api/src/notifications/services/notification/notification.service.spec.ts (3)
apps/api/src/notifications/services/notification/notification.service.ts (1)
CreateNotificationInput(74-76)packages/logging/src/servicies/logger/logger.service.ts (1)
error(107-109)apps/api/src/notifications/providers/notifications-api.provider.ts (1)
NotificationsApiClient(9-9)
apps/api/src/app/services/trial-started/trial-started.handler.ts (6)
apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.ts (1)
singleton(11-68)apps/api/src/core/services/domain-events/domain-events.service.ts (1)
singleton(10-17)apps/api/src/notifications/services/notification-handler/notification-handler.ts (2)
singleton(34-68)NotificationJob(20-32)apps/api/src/notifications/services/notification/notification.service.ts (1)
singleton(13-67)apps/api/src/billing/events/trial-started.ts (1)
TrialStarted(4-14)apps/api/src/notifications/services/notification-templates/start-trial-notification.ts (1)
startTrialNotification(4-16)
apps/api/src/billing/events/trial-started.ts (1)
apps/api/src/core/services/domain-events/domain-events.service.ts (1)
DomainEvent(6-8)
apps/api/src/core/services/domain-events/domain-events.service.ts (3)
apps/api/src/core/services/job-queue/job-queue.service.ts (1)
Job(139-143)apps/api/src/app/services/trial-started/trial-started.handler.ts (1)
singleton(13-84)apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.ts (1)
singleton(11-68)
apps/api/src/notifications/services/notification/notification.service.ts (1)
apps/api/src/notifications/providers/notifications-api.provider.ts (2)
NOTIFICATIONS_API_CLIENT(10-10)NotificationsApiClient(9-9)
apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.ts (3)
apps/api/src/billing/repositories/user-wallet/user-wallet.repository.ts (2)
UserWalletInput(9-14)UserWalletPublicOutput(27-33)apps/api/src/core/services/feature-flags/feature-flags.ts (1)
FeatureFlags(1-5)apps/api/src/billing/events/trial-started.ts (1)
TrialStarted(4-14)
apps/api/src/core/services/job-queue/job-queue.service.ts (5)
apps/api/src/core/providers/app-initializer.ts (3)
APP_INITIALIZER(3-3)AppInitializer(6-8)ON_APP_START(4-4)apps/api/src/app/services/trial-started/trial-started.handler.ts (1)
singleton(13-84)apps/api/src/core/services/domain-events/domain-events.service.ts (1)
singleton(10-17)apps/api/src/notifications/services/notification-handler/notification-handler.ts (1)
singleton(34-68)apps/api/src/app/providers/jobs.provider.ts (1)
ON_APP_START(10-13)
apps/api/src/console.ts (5)
apps/api/src/core/providers/postgres.provider.ts (2)
migratePG(24-24)closeConnections(42-42)apps/api/src/db/dbConnection.ts (1)
chainDb(20-30)apps/api/src/core/providers/app-initializer.ts (2)
APP_INITIALIZER(3-3)ON_APP_START(4-4)apps/api/src/app/providers/jobs.provider.ts (1)
ON_APP_START(10-13)apps/api/src/core/services/job-queue/job-queue.service.ts (1)
ON_APP_START(134-136)
apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts (5)
apps/api/src/billing/services/managed-user-wallet/managed-user-wallet.service.ts (1)
createAndAuthorizeTrialSpending(44-57)apps/api/src/core/services/feature-flags/feature-flags.ts (1)
FeatureFlags(1-5)apps/api/test/seeders/user-wallet.seeder.ts (1)
UserWalletSeeder(6-29)apps/api/test/seeders/chain-wallet.seeder.ts (1)
createChainWallet(4-12)apps/api/src/billing/events/trial-started.ts (1)
TrialStarted(4-14)
apps/api/src/notifications/services/notification-handler/notification-handler.ts (7)
apps/api/src/notifications/services/notification-templates/trial-ended-notification.ts (1)
trialEndedNotification(4-16)apps/api/src/notifications/services/notification-templates/before-trial-ends-notification.ts (1)
beforeTrialEndsNotification(6-19)apps/api/src/notifications/services/notification-templates/after-trial-ends-notification.ts (1)
afterTrialEndsNotification(4-16)apps/api/src/notifications/services/notification-templates/start-trial-notification.ts (1)
startTrialNotification(4-16)apps/api/src/core/services/job-queue/job-queue.service.ts (3)
Job(139-143)JOB_NAME(145-145)JobHandler(152-155)apps/api/src/app/services/trial-started/trial-started.handler.ts (1)
singleton(13-84)apps/api/src/notifications/services/notification/notification.service.ts (1)
singleton(13-67)
apps/api/src/core/services/feature-flags/feature-flags.service.ts (3)
apps/api/src/core/services/job-queue/job-queue.service.ts (2)
registry(10-137)ON_APP_START(134-136)apps/api/src/core/providers/app-initializer.ts (3)
APP_INITIALIZER(3-3)AppInitializer(6-8)ON_APP_START(4-4)apps/api/src/app/providers/jobs.provider.ts (1)
ON_APP_START(10-13)
apps/api/src/app.ts (4)
apps/api/src/core/providers/app-initializer.ts (2)
APP_INITIALIZER(3-3)ON_APP_START(4-4)apps/api/src/app/providers/jobs.provider.ts (1)
ON_APP_START(10-13)apps/api/src/core/services/feature-flags/feature-flags.service.ts (1)
ON_APP_START(80-82)apps/api/src/core/services/job-queue/job-queue.service.ts (1)
ON_APP_START(134-136)
⏰ 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). (1)
- GitHub Check: validate / validate-app
🔇 Additional comments (15)
apps/api/package.json (1)
70-70: @ucast/mongo2js compatibility and server-only usage verified
- @ucast/mongo2js@1.4.0 declares a dependency on @ucast/core ^1.6.1; our @ucast/core@^1.10.2 satisfies that range.
- The only import of @ucast/mongo2js is in apps/api/src/notifications/services/notification-handler/notification-handler.ts, confirming it remains server-side.
No further changes needed.
apps/api/src/notifications/services/notification-templates/trial-ended-notification.ts (1)
4-16: LGTM: simple, deterministic templateThe template shape and deterministic notificationId look good for de-duplication. No issues.
apps/api/src/notifications/services/notification/notification.service.ts (1)
15-15: Constructor change LGTM; verify upstream wiring and error handling expectationsRemoving the logger and propagating errors is fine. Ensure upstream callers (and tests) no longer depend on side-effect logging or swallowed errors.
Do all call sites expect
createNotificationto reject on failure now? If any relied on local logging and success fallback, they will need to handle rejections.apps/api/src/core/providers/postgres.provider.ts (1)
28-41: Typed DI tokens and table resolvers look solid
- POSTGRES_DB typed as InjectionToken and registered with the container is correct.
- Table-name keyed registrations and
resolveTablegeneric typing are clear and safe.InjectPg/InjectPgTabledecorators are straightforward and consistent.apps/api/test/services/wallet-testing.service.ts (1)
34-35: DomainEventsService is registered as a singleton
Verified via the@singleton()decorator on DomainEventsService in
apps/api/src/core/services/domain-events/domain-events.service.ts:10–11.
Thejest.spyOn(container.resolve(DomainEventsService),…)call will intercept publishes as intended.apps/api/src/app/providers/jobs.provider.ts (1)
8-15: JobQueueManager startup already handledJobQueueManager is registered as an APP_INITIALIZER via the @registry decorator in
apps/api/src/core/services/job-queue/job-queue.service.ts(lines 10–13), and its[ON_APP_START]implementation callsstart(). Handlers registered injobs.provider.tswill be applied when the queue starts, so no additionalstart()call is needed here.Likely an incorrect or invalid review comment.
apps/api/src/app.ts (1)
2-2: Side-effect import for initializers: LGTMBringing in the jobs provider via side-effect ensures APP_INITIALIZER is registered at startup.
apps/api/src/billing/events/trial-started.ts (1)
4-14: Event shape and typing look correctImplements DomainEvent correctly (name, version, data), and uses DOMAIN_EVENT_NAME as intended. Good.
apps/api/src/console.ts (1)
105-109: Clarify daemon lifecycle for drain-job-queuesThis command runs through daemon(), which suppresses the final shutdown. Ensure JobQueueManager.drain() keeps the process alive as intended; otherwise, open connections may keep the process hanging after drain completes, or the process may exit too early.
Consider either:
- Not using daemon() for one-shot drains and allowing shutdown(), or
- Keeping daemon() and ensuring drain() is a long-running loop (or leaves timers/handles active) until an external signal is received.
apps/api/src/core/services/feature-flags/feature-flags.service.ts (3)
28-48: Initialize-then-use assertion is good; onChanged remains no-op if disabled.The explicit assert in isEnabled prevents accidental usage before initialization, and the early return when FEATURE_FLAGS_ENABLE_ALL is set preserves behavior. onChanged guarded with ?. is appropriate.
54-73: Ensure correct Unleash client startup and shutdownWe’ve confirmed the project depends on
unleash-client@^6.6.0. According to the Node client docs for v6.x:
new Unleash(config)does not auto-start the client. You must callclient.start()before awaiting the"synchronized"event.- There is no
destroyWithFlush()method—useclient.stop()to cleanly shut down (it flushes metrics by default).Please update accordingly:
- In
apps/api/src/core/services/feature-flags/feature-flags.service.tswithininitialize()(lines ~54–73):
• After creating the client, callclient.start()before thenew Promisethat awaits"synchronized".- In
dispose()(lines ~74–78):
• Replacethis.client?.destroyWithFlush()withawait this.client?.stop().
75-83: App initializer integration looks correct.Registering via APP_INITIALIZER and deferring to initialize() on ON_APP_START keeps feature flags ready before use. isEnabled’s assert further enforces correct use.
apps/api/src/notifications/services/notification-handler/notification-handler.ts (1)
13-18: Template registry approach is clean and extensible.Mapping template keys to pure functions keeps the handler simple and testable. Good choice.
apps/api/src/app/services/trial-started/trial-started.handler.ts (1)
29-33: Immediate send path looks good (logs and direct notification).The direct send for the start-trial notification with structured logging is straightforward and avoids queuing latency.
apps/api/src/core/services/job-queue/job-queue.service.ts (1)
85-119: Daemon wrapper ensures process stays alivePgBoss.work() does resolve once the worker is registered (it doesn’t block indefinitely), so
drain()will return after scheduling all handlers. However, in console.ts you invoke it via thedaemonhelper (executeCliHandler with{ type: "daemon" }), which skips callingshutdown()and leaves the Node process running—thus your workers continue polling as expected.No changes required.
apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.ts
Outdated
Show resolved
Hide resolved
apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.ts
Outdated
Show resolved
Hide resolved
apps/api/src/notifications/services/notification-handler/notification-handler.ts
Show resolved
Hide resolved
apps/api/src/notifications/services/notification-handler/notification-handler.ts
Outdated
Show resolved
Hide resolved
apps/api/src/notifications/services/notification-templates/before-trial-ends-notification.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
apps/api/src/notifications/services/notification/notification.service.ts (3)
22-28: Replace forbiddenas anywith precise operation parameter typingPer coding guidelines, avoid
any. You can strongly type theparametersusing the OpenAPI-generatedoperationstypes.Apply this diff:
- const result = await this.notificationsApi.v1 - .createNotification({ - parameters: { - header: { - "x-user-id": user.id - } - } as any, - body: notification - }) + const parameters: operations["createNotification"]["parameters"] = { + header: { "x-user-id": user.id } + }; + const result = await this.notificationsApi.v1.createNotification({ + parameters, + body: notification + }) .catch(error => ({ error, response: null }));
45-49: Removeas anyin createDefaultChannel parametersSame issue as above; use the generated type for
createDefaultChannelparameters.Apply this diff:
- .createDefaultChannel({ - parameters: { - header: { - "x-user-id": user.id - } as any - }, + .createDefaultChannel({ + parameters: ({ header: { "x-user-id": user.id } } as operations["createDefaultChannel"]["parameters"]), body: { data: { name: "Default", type: "email", config: {Alternatively, assign a local
parametersconst first, mirroring the previous suggestion.
17-39: Ensure callers handlecreateNotificationerrorsThe service now throws after exhausting retries, and both downstream handlers invoke it without any error handling:
• apps/api/src/app/services/trial-started/trial-started.handler.ts:31
await this.notificationService.createNotification(startTrialNotification(user));• apps/api/src/notifications/services/notification-handler/notification-handler.ts:66
await this.notificationService.createNotification(notificationTemplate(user));Please wrap these calls in
try/catch(and log or handle the error) to prevent unhandled promise rejections, for example:try { await this.notificationService.createNotification(...); } catch (err) { this.logger.error({ event: "NOTIFICATION_FAILED", error: err }); // decide whether to rethrow or swallow }apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts (1)
154-156: Do not clear the root DI container from within test setup
container.clearInstances()mutates the global container, potentially causing cross-test interference and brittle ordering issues. Since you’re using a child container, this is unnecessary and risky.Remove it:
- container.clearInstances(); - return di;
🧹 Nitpick comments (22)
apps/api/src/core/providers/postgres.provider.ts (1)
28-28: Prefer Symbol-based DI tokens to prevent collisionsTyping the token is a nice improvement. Using a plain string token can collide across modules; prefer a Symbol to make it unambiguous and safer at scale.
Apply:
-export const POSTGRES_DB: InjectionToken<ApiPgDatabase> = "POSTGRES_DB"; +export const POSTGRES_DB = Symbol("POSTGRES_DB") as InjectionToken<ApiPgDatabase>;No other call sites need changes since inject/register accept symbols.
apps/api/src/billing/repositories/user-wallet/user-wallet.repository.ts (1)
49-69: Concurrency-safe getOrCreate looks good — unique index present; optional typing/readability nits
- Reasoning: the ON CONFLICT ... updatedAt = now() makes the method idempotent/race-safe; isNew via createdAt === updatedAt is a practical signal.
- Unique constraint verified:
- apps/api/drizzle/0003_magenta_mandarin.sql — ALTER TABLE "user_wallets" ADD CONSTRAINT "user_wallets_user_id_unique" UNIQUE("user_id");
- apps/api/src/billing/model-schemas/user-wallet/user-wallet.schema.ts — userId: uuid("user_id").references(() => Users.id, …).unique()
- Optional nits (apply if you want improved clarity/readability):
- Use NonNullable for the input userId type.
- Compute isNew in a local const before returning.
Apply:
-async getOrCreate(input: { userId: Exclude<UserWalletInput["userId"], undefined | null> }): Promise<{ wallet: UserWalletOutput; isNew: boolean }> { +async getOrCreate(input: { userId: NonNullable<UserWalletInput["userId"]> }): Promise<{ wallet: UserWalletOutput; isNew: boolean }> { const foundWallet = await this.findOneByUserId(input.userId); if (foundWallet) return { wallet: foundWallet, isNew: false }; this.ability?.throwUnlessCanExecute(input); const [potentiallyNewWallet] = await this.cursor .insert(this.table) .values(input) .onConflictDoUpdate({ target: [this.table.userId], set: { updatedAt: sql`now()` } }) .returning(); - return { - wallet: this.toOutput(potentiallyNewWallet), - isNew: potentiallyNewWallet.createdAt!.getTime() === potentiallyNewWallet.updatedAt!.getTime() - }; + const isNew = potentiallyNewWallet.createdAt!.getTime() === potentiallyNewWallet.updatedAt!.getTime(); + return { wallet: this.toOutput(potentiallyNewWallet), isNew }; }apps/api/src/notifications/services/notification-templates/trial-ended-notification.ts (1)
4-16: Optional Refactor: Consolidate Notification Template LogicI confirmed that
CreateNotificationInputis defined as the request body forcreateNotification(fieldsnotificationIdandpayload) plus auserobject. All four trial-lifecycle templates—startTrialNotification,beforeTrialEndsNotification,afterTrialEndsNotification, andtrialEndedNotification—currently duplicate the same shape construction.To DRY this up, you can introduce a shared helper, for example:
// apps/api/src/notifications/services/notification-templates/util.ts import type { UserOutput } from "@src/user/repositories"; import type { CreateNotificationInput } from "../notification/notification.service"; export function makeUserNotification( idPrefix: string, user: UserOutput, payload: { summary: string; description: string } ): CreateNotificationInput { return { notificationId: `${idPrefix}.${user.id}`, payload, user: { id: user.id, email: user.email }, }; }Then each template simplifies to:
-export function trialEndedNotification(user: UserOutput): CreateNotificationInput { - return { - notificationId: `trialEnded.${user.id}`, - payload: { - summary: "Trial Ended", - description: "Your trial of Akash Network has ended", - }, - user: { id: user.id, email: user.email }, - }; -} +export function trialEndedNotification(user: UserOutput): CreateNotificationInput { + return makeUserNotification("trialEnded", user, { + summary: "Trial Ended", + description: "Your trial of Akash Network has ended", + }); +}This reduces boilerplate and keeps all templates consistent.
apps/api/src/core/providers/app-initializer.ts (1)
4-7: Optional: strengthen symbol typing for better type inferenceAnnotating ON_APP_START as a unique symbol can improve type inference in computed keys across modules.
Apply:
-export const ON_APP_START = Symbol("ON_APP_START"); +export const ON_APP_START: unique symbol = Symbol("ON_APP_START");apps/api/src/notifications/services/notification-templates/before-trial-ends-notification.ts (1)
6-7: Guard createdAt or avoid non-null assertioncreatedAt! assumes the field is always present. If it’s missing for some legacy users, this will throw at runtime. Either validate and bail, or make the template resilient.
Example:
-export function beforeTrialEndsNotification(user: UserOutput): CreateNotificationInput { - const daysLeft = differenceInDays(user.createdAt!, new Date()); +export function beforeTrialEndsNotification(user: UserOutput, daysLeft: number): CreateNotificationInput { + // assume daysLeft already computed upstream; this keeps the template pure and safeapps/api/src/core/services/job-queue/job-queue.service.ts (2)
72-83: Avoid logging full job payloads (PII leak risk)Logging full job data can include emails and other PII. Prefer logging metadata only.
- this.logger.info({ - event: "JOB_ENQUEUED", - job - }); + this.logger.info({ + event: "JOB_ENQUEUED", + job: { name: job.name, version: job.version } + });
147-150: Replace any with unknown[] in constructor type helperHouse rule: no any. You can avoid any here.
-export type JobType<T extends Job> = { - new (...args: any[]): T; +export type JobType<T extends Job> = { + new (...args: unknown[]): T; [JOB_NAME]: string; };apps/api/test/services/wallet-testing.service.ts (1)
34-34: Consider restoring the spy after tests to avoid cross-test bleedIf this helper is used across multiple tests, the spy will persist. In each test, prefer using mockImplementationOnce or restore after usage.
Example in test setup/teardown:
const spy = jest.spyOn(container.resolve(DomainEventsService), "publish").mockResolvedValue(undefined); afterEach(() => spy.mockRestore());apps/api/src/notifications/services/notification/notification.service.ts (1)
41-41: TightencreateDefaultChannelsignature to require!Change in apps/api/src/notifications/services/notification/notification.service.ts:
- async createDefaultChannel(user: UserInput): Promise<void> { + async createDefaultChannel(user: UserInput & { email: string }): Promise<void> { @@ - addresses: [user.email!] + addresses: [user.email]• All existing call-sites pass a user object that always includes
createNotification, andUserOutputfromUserServicehasemail: string), so this tightens the contract without breaking anything.
• Removes the need for the non-null assertion and makes the requirement explicit.apps/api/src/core/services/domain-events/domain-events.service.ts (1)
6-8: DomainEvent interface redundantly re-declaresversion
Jobalready includesversion; extending and re-declaring it is redundant. Consider simplifying to a type alias.Apply this diff:
-export interface DomainEvent extends Job { - version: number; -} +export type DomainEvent = Job;apps/api/src/core/services/feature-flags/feature-flags.service.ts (1)
67-70: Prevent resource leak on initialization error (destroy Unleash client on error)If the
errorevent fires beforesynchronized, the created client isn't destroyed. Clean it up to avoid leaking sockets/listeners.Apply this diff:
- await new Promise((resolve, reject) => { - client.once("synchronized", resolve); - client.once("error", reject); - }); + await new Promise<void>((resolve, reject) => { + client.once("synchronized", resolve); + client.once("error", err => { + try { + client.destroy(); + } catch {} + reject(err); + }); + });apps/api/src/app.ts (1)
204-205: Ensure DB-Dependent Initializers Don’t Race with DB InitJobQueueManager’s
[ON_APP_START]()callsstart()on PgBoss (apps/api/src/core/services/job-queue/job-queue.service.ts), which in turn opens a Postgres connection. Running it in parallel withinitDb()risks connection failures if migrations aren’t applied yet. To avoid this, you can either:
- Run
initDb()to completion before invoking DB-dependent initializers- Add a lightweight readiness/retry loop inside
JobQueueManager.start()Locations to review:
- apps/api/src/app.ts (lines 204–205): current parallel invocation
- apps/api/src/core/services/job-queue/job-queue.service.ts:
[ON_APP_START]()→start()Suggested optional refactor (pseudo-code):
- await Promise.all([ - initDb(), - ...container - .resolveAll(APP_INITIALIZER) - .map(initializer => initializer[ON_APP_START]()) - ]); + // first ensure DB and migrations are applied + await initDb(); + // then start all initializers (including PgBoss) + await Promise.all( + container + .resolveAll(APP_INITIALIZER) + .map(initializer => initializer[ON_APP_START]()) + ); + startScheduler();apps/api/src/notifications/services/notification/notification.service.spec.ts (2)
138-146: Good negative-path assertions; ensure Response availability in test envThe rejection flow and cause propagation checks are solid. One caveat: constructing
new Response()assumes a Node test environment with WHATWG Fetch globals (Node 18+). If CI uses an environment withoutResponse, this will throw.Two options:
- Verify your Jest testEnvironment provides
Response(e.g., Node >= 18) or polyfill fetch/Response.- Or remove the
responsefield entirely from the mock since the production code only checksresult.error.Example minimal change:
- api.v1.createNotification.mockResolvedValue({ error, response: new Response() }); + api.v1.createNotification.mockResolvedValue({ error });
170-175: Adopt a parameterized setup() to match repo testing conventionsCurrent
setup()has no parameter. Our test guidelines prefer a single-parametersetupwith inline type, enabling per-test overrides without shared state. You can keep defaults while allowing overrides.-function setup() { - const api = mockDeep<NotificationsApiClient>(); - const service = new NotificationService(api); - return { service, api }; -} +function setup(input?: { api?: NotificationsApiClient }) { + const api = input?.api ?? mockDeep<NotificationsApiClient>(); + const service = new NotificationService(api); + return { service, api }; +}apps/api/src/console.ts (3)
105-109: Type-sound daemon usage for drain-job-queuesYou’re passing a zero-arg async fn to
daemon, whose signature expects(options, command). Runtime is fine, but TS may flag this under strict function types. Minor polish: accept and ignore the params.- .action(daemon(async () => container.resolve(JobQueueManager).drain())); + .action(daemon(async (_options, _command) => container.resolve(JobQueueManager).drain()));
112-138: Span is never ended; close it to avoid leaking telemetry spans
tracer.startSpan(name)is created but never ended. Wrap the handler in a try/finally and callspan.end()after execution to ensure proper span lifecycle.-async function executeCliHandler(name: string, handler: () => Promise<unknown>, options?: { type?: "action" | "daemon" }) { - await context.with(trace.setSpan(context.active(), tracer.startSpan(name)), async () => { +async function executeCliHandler(name: string, handler: () => Promise<unknown>, options?: { type?: "action" | "daemon" }) { + const span = tracer.startSpan(name); + await context.with(trace.setSpan(context.active(), span), async () => { logger.info({ event: "COMMAND_START", name }); // eslint-disable-next-line @typescript-eslint/no-var-requires const { migratePG } = require("./core/providers/postgres.provider"); try { await Promise.all([migratePG(), chainDb.authenticate(), ...container.resolveAll(APP_INITIALIZER).map(initializer => initializer[ON_APP_START]())]); const result = await handler(); if (result && result instanceof Err) { logger.error({ event: "COMMAND_ERROR", name, result: result.val }); process.exitCode = 1; } else { logger.info({ event: "COMMAND_END", name }); } } catch (error) { logger.error({ event: "COMMAND_ERROR", name, error }); process.exitCode = 1; } finally { + span.end(); if (options?.type !== "daemon") { await shutdown(); } } }); }
119-121: Guard optional ON_APP_START hooks to avoid hard failuresIf any
APP_INITIALIZERdoesn’t implement[ON_APP_START], this will throw. Safeguard with a presence check.- await Promise.all([migratePG(), chainDb.authenticate(), ...container.resolveAll(APP_INITIALIZER).map(initializer => initializer[ON_APP_START]())]); + await Promise.all([ + migratePG(), + chainDb.authenticate(), + ...container.resolveAll(APP_INITIALIZER).flatMap(initializer => (typeof initializer[ON_APP_START] === "function" ? [initializer[ON_APP_START]()] : [])) + ]);apps/api/src/app/services/trial-started/trial-started.handler.ts (2)
29-33: Don’t abort scheduling if the immediate notification failsA transient failure in
createNotificationwill prevent enqueuing the follow-up notifications. Prefer to log and continue scheduling; retries for the immediate notification can be handled separately if needed.- if (user.email) { - this.logger.info({ event: "START_TRIAL_NOTIFICATION_SENDING", userId: user.id }); - await this.notificationService.createNotification(startTrialNotification(user)); - this.logger.info({ event: "START_TRIAL_NOTIFICATION_SENT", userId: user.id }); - } + if (user.email) { + this.logger.info({ event: "START_TRIAL_NOTIFICATION_SENDING", userId: user.id }); + try { + await this.notificationService.createNotification(startTrialNotification(user)); + this.logger.info({ event: "START_TRIAL_NOTIFICATION_SENT", userId: user.id }); + } catch (error) { + this.logger.error({ event: "START_TRIAL_NOTIFICATION_FAILED", userId: user.id, error }); + } + }
35-82: Clamp scheduling to avoid past startAfter times when trial is shorter than 7 daysIf
TRIAL_ALLOWANCE_EXPIRATION_DAYS < 7,addDays(new Date(), TRIAL_ALLOWANCE_EXPIRATION_DAYS - 7)results in a past timestamp. Depending on the queue semantics, this may fire immediately. If that’s intended, ignore. Otherwise, clamp to “now”.If you prefer clamping, consider:
- const TRIAL_ALLOWANCE_EXPIRATION_DAYS = this.coreConfig.get("TRIAL_ALLOWANCE_EXPIRATION_DAYS"); + const TRIAL_ALLOWANCE_EXPIRATION_DAYS = this.coreConfig.get("TRIAL_ALLOWANCE_EXPIRATION_DAYS"); + const now = new Date(); + const startAfter = (days: number) => { + const d = addDays(now, days); + return d < now ? now : d; + }; ... - startAfter: addDays(new Date(), TRIAL_ALLOWANCE_EXPIRATION_DAYS - 7) + startAfter: startAfter(TRIAL_ALLOWANCE_EXPIRATION_DAYS - 7) ... - startAfter: addDays(new Date(), TRIAL_ALLOWANCE_EXPIRATION_DAYS - 1) + startAfter: startAfter(TRIAL_ALLOWANCE_EXPIRATION_DAYS - 1) ... - startAfter: addDays(new Date(), TRIAL_ALLOWANCE_EXPIRATION_DAYS) + startAfter: startAfter(TRIAL_ALLOWANCE_EXPIRATION_DAYS) ... - startAfter: addDays(new Date(), TRIAL_ALLOWANCE_EXPIRATION_DAYS + 7) + startAfter: startAfter(TRIAL_ALLOWANCE_EXPIRATION_DAYS + 7)Also, compute
nowonce to ensure consistent offsets across enqueues.apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts (1)
127-138: Avoid double cast via unknown; build a proper typed mockMinor, but
return this as unknown as UserWalletRepositoryis a smell. You can returnthis as any as UserWalletRepositoryis still a cast. Prefer constructing a small adapter foraccessibleBy()to return a repository-shaped object explicitly.Example:
- accessibleBy() { - return this as unknown as UserWalletRepository; - }, + accessibleBy() { + return { + ...this, + getOrCreate: input?.getOrCreateWallet + } as UserWalletRepository; + },apps/api/src/notifications/services/notification-handler/notification-handler.ts (2)
55-62: Differentiate log reasons for better observability.Merging “user not found” and “email not found” under a single event makes debugging noisier. Split them for clearer signals and better alert routing.
- if (!user || !user.email) { - this.logger.info({ - event: "NOTIFICATION_SEND_FAILED", - userId: payload.userId, - reason: "User or email not found" - }); - return; - } + if (!user) { + this.logger.info({ + event: "NOTIFICATION_SEND_FAILED", + userId: payload.userId, + reason: "USER_NOT_FOUND" + }); + return; + } + if (!user.email) { + this.logger.info({ + event: "NOTIFICATION_SEND_FAILED", + userId: payload.userId, + reason: "USER_EMAIL_NOT_FOUND" + }); + return; + }
44-67: Consider adding unit tests for handler paths.Add focused tests to cover:
- Unknown template → logs UNKNOWN_NOTIFICATION_TYPE and returns
- User not found → returns
- User without email → returns
- Conditions present but not matched → returns
- Happy path → createNotification invoked with the correct template
I can scaffold Jest tests that stub UserRepository, NotificationService, and LoggerService, with representative payloads for each path. Want me to push a test file?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (22)
apps/api/package.json(2 hunks)apps/api/src/app.ts(3 hunks)apps/api/src/app/providers/jobs.provider.ts(1 hunks)apps/api/src/app/services/trial-started/trial-started.handler.ts(1 hunks)apps/api/src/billing/controllers/wallet/wallet.controller.ts(0 hunks)apps/api/src/billing/events/trial-started.ts(1 hunks)apps/api/src/billing/repositories/user-wallet/user-wallet.repository.ts(2 hunks)apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts(6 hunks)apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.ts(3 hunks)apps/api/src/console.ts(4 hunks)apps/api/src/core/providers/app-initializer.ts(1 hunks)apps/api/src/core/providers/postgres.provider.ts(2 hunks)apps/api/src/core/services/domain-events/domain-events.service.ts(1 hunks)apps/api/src/core/services/feature-flags/feature-flags.service.ts(2 hunks)apps/api/src/core/services/job-queue/job-queue.service.ts(1 hunks)apps/api/src/notifications/services/notification-handler/notification-handler.ts(1 hunks)apps/api/src/notifications/services/notification-templates/after-trial-ends-notification.ts(1 hunks)apps/api/src/notifications/services/notification-templates/before-trial-ends-notification.ts(1 hunks)apps/api/src/notifications/services/notification-templates/trial-ended-notification.ts(1 hunks)apps/api/src/notifications/services/notification/notification.service.spec.ts(2 hunks)apps/api/src/notifications/services/notification/notification.service.ts(2 hunks)apps/api/test/services/wallet-testing.service.ts(2 hunks)
💤 Files with no reviewable changes (1)
- apps/api/src/billing/controllers/wallet/wallet.controller.ts
🧰 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/api/src/notifications/services/notification-templates/trial-ended-notification.tsapps/api/src/app/providers/jobs.provider.tsapps/api/src/app/services/trial-started/trial-started.handler.tsapps/api/test/services/wallet-testing.service.tsapps/api/src/core/services/job-queue/job-queue.service.tsapps/api/src/core/providers/app-initializer.tsapps/api/src/core/services/domain-events/domain-events.service.tsapps/api/src/billing/repositories/user-wallet/user-wallet.repository.tsapps/api/src/notifications/services/notification-templates/before-trial-ends-notification.tsapps/api/src/billing/events/trial-started.tsapps/api/src/notifications/services/notification-templates/after-trial-ends-notification.tsapps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.tsapps/api/src/core/providers/postgres.provider.tsapps/api/src/app.tsapps/api/src/notifications/services/notification/notification.service.tsapps/api/src/core/services/feature-flags/feature-flags.service.tsapps/api/src/billing/services/wallet-initializer/wallet-initializer.service.tsapps/api/src/notifications/services/notification/notification.service.spec.tsapps/api/src/console.tsapps/api/src/notifications/services/notification-handler/notification-handler.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/notifications/services/notification-templates/trial-ended-notification.tsapps/api/src/app/providers/jobs.provider.tsapps/api/src/app/services/trial-started/trial-started.handler.tsapps/api/test/services/wallet-testing.service.tsapps/api/src/core/services/job-queue/job-queue.service.tsapps/api/src/core/providers/app-initializer.tsapps/api/src/core/services/domain-events/domain-events.service.tsapps/api/src/billing/repositories/user-wallet/user-wallet.repository.tsapps/api/src/notifications/services/notification-templates/before-trial-ends-notification.tsapps/api/src/billing/events/trial-started.tsapps/api/src/notifications/services/notification-templates/after-trial-ends-notification.tsapps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.tsapps/api/src/core/providers/postgres.provider.tsapps/api/src/app.tsapps/api/src/notifications/services/notification/notification.service.tsapps/api/src/core/services/feature-flags/feature-flags.service.tsapps/api/src/billing/services/wallet-initializer/wallet-initializer.service.tsapps/api/src/notifications/services/notification/notification.service.spec.tsapps/api/src/console.tsapps/api/src/notifications/services/notification-handler/notification-handler.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/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.tsapps/api/src/notifications/services/notification/notification.service.spec.ts
🧠 Learnings (1)
📚 Learning: 2025-07-21T08:24:24.269Z
Learnt from: CR
PR: akash-network/console#0
File: .cursor/rules/no-jest-mock.mdc:0-0
Timestamp: 2025-07-21T08:24:24.269Z
Learning: Applies to **/*.spec.{ts,tsx} : Don't use `jest.mock()` to mock dependencies in test files. Instead, use `jest-mock-extended` to create mocks and pass mocks as dependencies to the service under test.
Applied to files:
apps/api/src/notifications/services/notification/notification.service.spec.ts
🧬 Code Graph Analysis (17)
apps/api/src/notifications/services/notification-templates/trial-ended-notification.ts (1)
apps/api/src/notifications/services/notification/notification.service.ts (1)
CreateNotificationInput(74-76)
apps/api/src/app/providers/jobs.provider.ts (2)
apps/api/src/core/providers/app-initializer.ts (2)
APP_INITIALIZER(3-3)ON_APP_START(4-4)apps/api/src/core/services/job-queue/job-queue.service.ts (1)
ON_APP_START(134-136)
apps/api/src/app/services/trial-started/trial-started.handler.ts (7)
apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.ts (1)
singleton(11-68)apps/api/src/core/services/domain-events/domain-events.service.ts (1)
singleton(10-17)apps/api/src/notifications/services/notification-handler/notification-handler.ts (2)
singleton(34-68)NotificationJob(20-32)apps/api/src/notifications/services/notification/notification.service.ts (1)
singleton(13-67)apps/api/src/core/services/job-queue/job-queue.service.ts (1)
JobHandler(152-155)apps/api/src/billing/events/trial-started.ts (1)
TrialStarted(4-14)apps/api/src/notifications/services/notification-templates/start-trial-notification.ts (1)
startTrialNotification(4-16)
apps/api/src/core/services/job-queue/job-queue.service.ts (6)
apps/api/src/core/providers/app-initializer.ts (3)
APP_INITIALIZER(3-3)AppInitializer(6-8)ON_APP_START(4-4)apps/api/src/app/services/trial-started/trial-started.handler.ts (1)
singleton(13-84)apps/api/src/core/services/domain-events/domain-events.service.ts (1)
singleton(10-17)apps/api/src/notifications/services/notification-handler/notification-handler.ts (1)
singleton(34-68)packages/logging/src/servicies/logger/logger.service.ts (1)
error(107-109)apps/api/src/app/providers/jobs.provider.ts (1)
ON_APP_START(10-13)
apps/api/src/core/providers/app-initializer.ts (3)
apps/api/src/app/providers/jobs.provider.ts (1)
ON_APP_START(10-13)apps/api/src/core/services/feature-flags/feature-flags.service.ts (1)
ON_APP_START(80-82)apps/api/src/core/services/job-queue/job-queue.service.ts (1)
ON_APP_START(134-136)
apps/api/src/core/services/domain-events/domain-events.service.ts (2)
apps/api/src/core/services/job-queue/job-queue.service.ts (1)
Job(139-143)apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.ts (1)
singleton(11-68)
apps/api/src/notifications/services/notification-templates/before-trial-ends-notification.ts (1)
apps/api/src/notifications/services/notification/notification.service.ts (1)
CreateNotificationInput(74-76)
apps/api/src/billing/events/trial-started.ts (1)
apps/api/src/core/services/domain-events/domain-events.service.ts (1)
DomainEvent(6-8)
apps/api/src/notifications/services/notification-templates/after-trial-ends-notification.ts (1)
apps/api/src/notifications/services/notification/notification.service.ts (1)
CreateNotificationInput(74-76)
apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts (5)
apps/api/src/billing/services/managed-user-wallet/managed-user-wallet.service.ts (1)
createAndAuthorizeTrialSpending(44-57)apps/api/src/core/services/feature-flags/feature-flags.ts (1)
FeatureFlags(1-5)apps/api/test/seeders/user-wallet.seeder.ts (1)
UserWalletSeeder(6-29)apps/api/test/seeders/chain-wallet.seeder.ts (1)
createChainWallet(4-12)apps/api/src/billing/events/trial-started.ts (1)
TrialStarted(4-14)
apps/api/src/app.ts (4)
apps/api/src/core/providers/app-initializer.ts (2)
APP_INITIALIZER(3-3)ON_APP_START(4-4)apps/api/src/app/providers/jobs.provider.ts (1)
ON_APP_START(10-13)apps/api/src/core/services/feature-flags/feature-flags.service.ts (1)
ON_APP_START(80-82)apps/api/src/core/services/job-queue/job-queue.service.ts (1)
ON_APP_START(134-136)
apps/api/src/notifications/services/notification/notification.service.ts (1)
apps/api/src/notifications/providers/notifications-api.provider.ts (2)
NOTIFICATIONS_API_CLIENT(10-10)NotificationsApiClient(9-9)
apps/api/src/core/services/feature-flags/feature-flags.service.ts (3)
apps/api/src/core/services/job-queue/job-queue.service.ts (2)
registry(10-137)ON_APP_START(134-136)apps/api/src/core/providers/app-initializer.ts (3)
APP_INITIALIZER(3-3)AppInitializer(6-8)ON_APP_START(4-4)apps/api/src/app/providers/jobs.provider.ts (1)
ON_APP_START(10-13)
apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.ts (3)
apps/api/src/billing/repositories/user-wallet/user-wallet.repository.ts (2)
UserWalletInput(9-14)UserWalletPublicOutput(27-33)apps/api/src/core/services/feature-flags/feature-flags.ts (1)
FeatureFlags(1-5)apps/api/src/billing/events/trial-started.ts (1)
TrialStarted(4-14)
apps/api/src/notifications/services/notification/notification.service.spec.ts (3)
apps/api/src/notifications/services/notification/notification.service.ts (1)
CreateNotificationInput(74-76)packages/logging/src/servicies/logger/logger.service.ts (1)
error(107-109)apps/api/src/notifications/providers/notifications-api.provider.ts (1)
NotificationsApiClient(9-9)
apps/api/src/console.ts (5)
apps/api/src/core/providers/postgres.provider.ts (2)
migratePG(24-24)closeConnections(42-42)apps/api/src/db/dbConnection.ts (1)
chainDb(20-30)apps/api/src/core/providers/app-initializer.ts (2)
APP_INITIALIZER(3-3)ON_APP_START(4-4)apps/api/src/app/providers/jobs.provider.ts (1)
ON_APP_START(10-13)apps/api/src/core/services/job-queue/job-queue.service.ts (1)
ON_APP_START(134-136)
apps/api/src/notifications/services/notification-handler/notification-handler.ts (7)
apps/api/src/notifications/services/notification-templates/trial-ended-notification.ts (1)
trialEndedNotification(4-16)apps/api/src/notifications/services/notification-templates/before-trial-ends-notification.ts (1)
beforeTrialEndsNotification(6-19)apps/api/src/notifications/services/notification-templates/after-trial-ends-notification.ts (1)
afterTrialEndsNotification(4-16)apps/api/src/notifications/services/notification-templates/start-trial-notification.ts (1)
startTrialNotification(4-16)apps/api/src/core/services/job-queue/job-queue.service.ts (3)
Job(139-143)JOB_NAME(145-145)JobHandler(152-155)apps/api/src/app/services/trial-started/trial-started.handler.ts (1)
singleton(13-84)apps/api/src/notifications/services/notification/notification.service.ts (1)
singleton(13-67)
⏰ 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: validate / validate-app
🔇 Additional comments (28)
apps/api/package.json (1)
70-70: Limited usage of @ucast/mongo2js confirmedThe only import of @ucast/mongo2js is in:
- apps/api/src/notifications/services/notification-handler/notification‐handler.ts
No other ES-module imports or CommonJS requires were found across the codebase.
• Please ensure your client-side bundler or tree-shaking config excludes this API-only file so that @ucast/mongo2js does not end up in any browser bundles.
apps/api/src/core/providers/postgres.provider.ts (1)
6-6: Type-only import usage is correctImporting InjectionToken as a type keeps runtime clean and is aligned with tsyringe usage. No issues.
apps/api/src/billing/repositories/user-wallet/user-wallet.repository.ts (1)
1-1: Importing sql is correct for now() usageGood catch including sql for the upsert timestamp bump. No issues.
apps/api/src/notifications/services/notification-templates/after-trial-ends-notification.ts (1)
4-16: Align with shared helper for consistencyLGTM — if you introduce a shared builder, please update all trial-lifecycle templates for easier maintenance.
• Optional refactor using a helper:
-export function afterTrialEndsNotification(user: UserOutput): CreateNotificationInput { - return { - notificationId: `afterTrialEnds.${user.id}`, - payload: { - summary: "Your Trial Has Ended", - description: "Your trial of Akash Network has ended. Please upgrade to continue using the platform." - }, - user: { - id: user.id, - email: user.email - } - }; -} +export function afterTrialEndsNotification(user: UserOutput): CreateNotificationInput { + return makeUserNotification("afterTrialEnds", user, { + summary: "Your Trial Has Ended", + description: "Your trial of Akash Network has ended. Please upgrade to continue using the platform.", + }); +}• Verify that the TrialStarted handler actually enqueues all three templates (
beforeTrialEndsNotification,trialEndedNotification,afterTrialEndsNotification). A quick grep only found their declarations, not any calls in the scheduler—please ensure the handler (likely inapps/api/src/.../trial-startedor similar) invokes each notification builder at the appropriate time.apps/api/src/core/providers/app-initializer.ts (1)
3-8: Solid DI contract for app startup hooksThe tokens and computed-method interface read clean and enable safe multi-initializer composition. No issues spotted.
apps/api/src/core/services/job-queue/job-queue.service.ts (3)
29-39: Queue creation options likely ignored; verify pg-boss APIcreateQueue(queueName, { name: queueName, retryLimit: 10 }) looks suspicious. Typically, retryLimit is a send/job option, not a queue creation option. Unrecognized fields are ignored.
Proposed fix (if confirmed): drop name/retryLimit here and rely on per-message SendOptions (already supported by enqueue).
- await this.pgBoss.createQueue(queueName, { - name: queueName, - retryLimit: 10 - }); + await this.pgBoss.createQueue(queueName);If you want default retry policy, enforce it at enqueue sites via options or wrap enqueue to inject a default retryLimit.
125-132: Good: lifecycle wiring and error reportingStarting the queue in ON_APP_START and wiring an 'error' listener is sound. Just ensure the listener isn’t added multiple times across restarts (singletons mitigate this).
85-119: Ensure pg-boss can retry failed jobsWrapping
handler.handle()calls inPromise.allSettledhides errors from pg-boss, causing the batch to be treated as “handled” and preventing built-in retries.Locations to update:
- apps/api/src/core/services/job-queue/job-queue.service.ts (lines 85–119)
Suggested refactor (Option A – let exceptions bubble):
await this.pgBoss.work<Job["data"]>(queueName, { batchSize: 10, ...options }, async jobs => { this.logger.info({ event: "JOBS_STARTED", jobs }); - const promises = jobs.map(async job => handler.handle(job.data)); - const results = await Promise.allSettled(promises); - results.forEach((result, index) => { - if (result.status === "fulfilled") { - this.logger.info({ event: "JOB_DONE", job: jobs[index] }); - } else { - this.logger.error({ event: "JOB_FAILED", job: jobs[index], error: result.reason }); - } - }); + for (const job of jobs) { + // let exceptions bubble to trigger pg-boss retry behavior + await handler.handle(job.data); + this.logger.info({ event: "JOB_DONE", job }); + } });If pg-boss v10.3.2 supports per-item ack/fail in batch mode, you could instead explicitly fail only errored jobs. Otherwise, Option A is the safer approach.
Please verify pg-boss batch callback semantics in your version before merging.apps/api/test/services/wallet-testing.service.ts (1)
10-10: Test dependency updated to domain events — good pivotSwitching the spy to DomainEventsService.publish aligns tests with the new event-driven flow.
apps/api/src/app/providers/jobs.provider.ts (1)
8-15: Handler registration on app start looks correctHandlers are resolved via container and registered once at startup. This plays nicely with JobQueueManager’s ON_APP_START to start the queue.
One caveat: initializers may run in parallel; if the queue starts before registration completes, it’s still fine, but you may see a brief window without workers registered. If strict ordering is needed, register handlers inside JobQueueManager.start or sequence initializers.
If ordering matters, ensure app.ts awaits registration before starting workers, or co-locate registration in JobQueueManager.
apps/api/src/notifications/services/notification/notification.service.ts (1)
15-15: Constructor DI simplification looks goodRemoving the logger dependency and injecting only the NotificationsApiClient aligns with single-responsibility and keeps this service focused.
apps/api/src/billing/events/trial-started.ts (1)
4-14: Event shape LGTM; consistent with Job/DomainEvent contractsThe static domain event name, name getter, versioning, and payload shape are clear and type-safe.
apps/api/src/core/services/domain-events/domain-events.service.ts (2)
5-5: Alias export for DOMAIN_EVENT_NAME is clearRe-exporting
JOB_NAMEasDOMAIN_EVENT_NAMEprovides a nice semantic boundary between jobs and domain events.
14-16: Publish implementation is appropriately thinDelegating to JobQueueManager is the right abstraction; letting errors bubble is fine.
apps/api/src/core/services/feature-flags/feature-flags.service.ts (2)
10-12: APP_INITIALIZER integration looks goodRegistering FeatureFlagsService as an app initializer and implementing
[ON_APP_START]removes ad-hoc startup wiring and keeps concerns localized.
18-26: Injectable client factory is a solid testability improvementInjecting
createUnleashClientenables deterministic tests and decouples from the real client.apps/api/src/app.ts (2)
2-2: Side-effect import for jobs provider is appropriateThis ensures job handlers are registered via APP_INITIALIZER before the app starts serving requests.
24-24: Switch to APP_INITIALIZER/ON_APP_START tokens is the right abstractionDecouples startup sequencing from specific services and scales as more initializers are added.
apps/api/src/notifications/services/notification/notification.service.spec.ts (1)
151-167: Resilience and failure semantics look correctThis test cleanly validates retry-on-rejection with preserved
.cause. Assertions and timer orchestration viaPromise.allSettled([...runAllTimersAsync()])are appropriate and align with the service behavior.apps/api/src/console.ts (1)
146-154: Graceful shutdown sequence looks solidOnce-wrapped shutdown, parallelized close of PG, Sequelize, and DI container, and signal handlers are well implemented for both action and daemon flows.
apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.ts (1)
26-43: Happy-path cleanup looks correctCreating wallet + authorizing trial spending, updating allowances, and rolling back the new wallet on failure is sound. The
isTrialSpendingAuthorizedguard avoids publishing events on failure.apps/api/src/app/services/trial-started/trial-started.handler.ts (1)
35-36: Verifiedtrialfield on UserOutputThe
UserOutputtype inapps/api/src/user/repositories/user/user.repository.ts(line 11) declarestrial?: boolean, so the{ trial: true }condition is valid and will match as intended. No further changes needed.apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts (2)
85-86: Correct: no domain event on failure pathGood check to ensure
DomainEventsService.publishisn’t called when authorization fails. This aligns with the service guard.
88-106: Event assertion matches new domain flowAsserting
publish(new TrialStarted({ userId }))when the FF is disabled correctly reflects the new domain-events-based design.apps/api/src/notifications/services/notification-handler/notification-handler.ts (4)
13-18: Template registry looks consistent and type-safe.The registry is cohesive with the provided templates and
keyof typeof notificationTemplatesgives strongly-typed job payloads.
34-37: Correct JobHandler binding.
accepts = NotificationJobcorrectly binds this handler to the job type for the queue manager to route jobs.
21-23: JOB_NAME “notifications” is unique across the codebase
Confirmed by running:
•rg -nP --type=ts 'static\s+readonly\s+\[JOB_NAME\]\s*=\s*"notifications"'
Only the declaration inapps/api/src/notifications/services/notification-handler/notification-handler.tswas found—no collisions detected.
26-30: Confirmedtrialfield exists on UserOutput
TheUserOutputtype inapps/api/src/user/repositories/user/user.repository.ts(lines 11–13) declarestrial?: boolean, so using{ trial: true }inconditionsis valid and will not suppress notifications.
apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.ts
Outdated
Show resolved
Hide resolved
apps/api/src/notifications/services/notification-handler/notification-handler.ts
Outdated
Show resolved
Hide resolved
apps/api/src/notifications/services/notification-handler/notification-handler.ts
Outdated
Show resolved
Hide resolved
apps/api/src/notifications/services/notification-templates/before-trial-ends-notification.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/api/src/notifications/services/notification/notification.service.ts (2)
23-27: Removeas anyonparameters(violates no-any guideline)Casting to
anydefeats type safety and violates the repository guideline for TS. Type the header properly instead of casting the wholeparametersobject toany.Apply this diff:
- parameters: { - header: { - "x-user-id": user.id - } - } as any, + parameters: { + header: { "x-user-id": user.id } as Record<string, string> + },Optionally, to avoid repeating the inline cast, introduce a small helper type once in this file:
type UserIdHeader = Record<"x-user-id", string>;and then use
as UserIdHeaderinstead ofRecord<string, string>.
45-49: Removeas anyon header in createDefaultChannelSame issue as above: casting to
anyviolates the TS guideline. Narrow the type to a concrete header shape.Apply this diff:
- parameters: { - header: { - "x-user-id": user.id - } as any - }, + parameters: { + header: { "x-user-id": user.id } as Record<string, string> + },
🧹 Nitpick comments (23)
apps/api/package.json (1)
70-70: pg-boss addition: watch DB connection pools and bootstrap permissionsGood addition for the new job queue. Keep in mind:
- You’ll now run two different PG clients (postgres-js via Drizzle and pg via pg-boss). Cap total connections appropriately (pg-boss has its own pool). Configure pg-boss max connections to avoid hitting DB limits in constrained envs.
- Ensure the DB role has rights to create the pg-boss schema on first start, or run the bootstrap once in a privileged context.
Also applies to: 96-96
apps/api/src/notifications/services/notification-templates/after-trial-ends-notification.ts (1)
6-9: Align notificationId naming with existing templatesIf you already have “before-trial-ends” / “trial-ended” templates, consider harmonizing the identifier naming (e.g., afterTrialEnds vs trialEnded) to avoid future confusion and to ease querying/grouping.
apps/api/src/billing/repositories/user-wallet/user-wallet.repository.ts (1)
65-68: Timestamp equality for isNew is brittleRelying on createdAt === updatedAt can be fragile with triggers/defaults. The refactor above bases isNew on whether an insert actually happened, which is more robust.
apps/api/src/notifications/services/notification/notification.service.ts (1)
41-66: Guard against missing user email instead of non-null assertion
addresses: [user.email!]relies on a non-null assertion. While the call site checks foruser.emailbefore invoking this method today,createDefaultChannelis public and could be used elsewhere. Add a fast path guard to avoid accidental misuse and remove the!.Apply this diff:
async createDefaultChannel(user: UserInput): Promise<void> { + if (!user.email) { + // Nothing to create without a user email; exit early. + return; + } await backOff(async () => { const result = await this.notificationsApi.v1 .createDefaultChannel({ parameters: { header: { "x-user-id": user.id } as Record<string, string> }, body: { data: { name: "Default", type: "email", config: { - addresses: [user.email!] + addresses: [user.email] } } } }) .catch(error => ({ error, response: null }));apps/api/src/notifications/services/notification/notification.service.spec.ts (5)
39-47: Remove duplicatejest.useFakeTimers()callTimers are enabled twice in this test, which is redundant.
Apply this diff:
- jest.useFakeTimers(); const { service, api } = setup(); - jest.useFakeTimers(); + jest.useFakeTimers();
70-97: Prefer resetting timers after tests in this blockYou reset timers in the “createDefaultChannel” block via
afterEach, but not here. Add a matchingafterEach(jest.useRealTimers)in thisdescribeto prevent timer leakage across tests.Example patch:
describe("createNotification", () => { + afterEach(() => { + jest.useRealTimers(); + });
128-147: Rename test to reflect behavior (no logging now), keep rejection assertionsThe implementation no longer logs. The test title still mentions logging, which is misleading. The assertions correctly check rejection with
cause.Apply this diff:
- it("does not create default channel when user has no email and logs an error", async () => { + it("does not create default channel when user has no email and rejects with cause", async () => {
150-167: Rename test to reflect behavior (rejection), assertions look goodSimilarly, the title should focus on rejection after retries rather than logging.
Apply this diff:
- it("fails after 10 attempts if notification service is not available and logs an error", async () => { + it("fails after 10 attempts if notification service is not available and rejects with original cause", async () => {
170-175: Align setup() signature with project testing guidelineThe repository guideline asks for a setup function that accepts a single parameter with an inline type. Consider adding an optional parameter to conform (and use a leading underscore to avoid unused-var lint).
Apply this diff:
- function setup() { + function setup(_opts: { /* add options as needed */ } = {}) { const api = mockDeep<NotificationsApiClient>(); const service = new NotificationService(api); return { service, api }; }apps/api/src/core/services/feature-flags/feature-flags.service.ts (1)
67-73: Wait-for-sync init path is fine; consider error surface area and cleanupThe
synchronized/errorlatch is good. If you ever decide to expose initialization failures to callers, you might want to attach the error ascauseor wrap with context. Not critical.apps/api/test/services/wallet-testing.service.ts (1)
33-41: Consider restoring spies to avoid cross-test leakageSpying on container-resolved singletons can leak between tests. Consider scoping or restoring these spies in tests that use this helper.
For example, if you invoke this helper in tests, add:
const publishSpy = jest.spyOn(container.resolve(DomainEventsService), "publish").mockResolvedValue(undefined); // ... test ... publishSpy.mockRestore();Alternatively, ensure your test framework calls
jest.restoreAllMocks()in a global afterEach.apps/api/src/app.ts (1)
204-204: Ensure DB is ready before running initializers & guard ON_APP_STARTTo avoid any app-initializer racing against your database setup (e.g.
JobQueueManager.start()opening its own Postgres connection)—and to skip any mis-registered provider without anON_APP_STARThook—apply this optional refactor:- await Promise.all([ - initDb(), - ...container.resolveAll(APP_INITIALIZER).map(initializer => initializer[ON_APP_START]()) - ]); + // ensure DB schema and connections are established first + await initDb(); + const initializers = container.resolveAll(APP_INITIALIZER); + await Promise.all( + initializers.map(init => + typeof init[ON_APP_START] === "function" + ? init[ON_APP_START]() + : Promise.resolve() + ) + );optional refactor—no breaking changes expected.
apps/api/src/core/services/domain-events/domain-events.service.ts (1)
6-8: Remove redundant version redeclaration in DomainEventJob already includes version: number. Redeclaring it in DomainEvent is redundant and may confuse readers.
Apply this diff:
-export interface DomainEvent extends Job { - version: number; -} +export interface DomainEvent extends Job {}apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.ts (3)
21-24: Drop the non-null assertion on userId by tightening the method signature.
userId!will blow up at runtime if undefined. Prefer making the param non-nullable and pass it directly.Apply this diff:
- async initializeAndGrantTrialLimits(userId: UserWalletInput["userId"]): Promise<UserWalletPublicOutput> { - const { wallet, isNew } = await this.userWalletRepository.accessibleBy(this.authService.ability, "create").getOrCreate({ userId: userId! }); + async initializeAndGrantTrialLimits(userId: NonNullable<UserWalletInput["userId"]>): Promise<UserWalletPublicOutput> { + const { wallet, isNew } = await this.userWalletRepository + .accessibleBy(this.authService.ability, "create") + .getOrCreate({ userId });
28-35: Avoid variable shadowing; rename local wallet to improve readability.
const wallet = await this.walletManager...shadows the earlierwalletfromgetOrCreate, making the block harder to read.Apply this diff:
- const wallet = await this.walletManager.createAndAuthorizeTrialSpending({ addressIndex: userWallet.id }); + const chainWallet = await this.walletManager.createAndAuthorizeTrialSpending({ addressIndex: userWallet.id }); userWallet = await this.userWalletRepository.updateById( userWallet.id, { - address: wallet.address, - deploymentAllowance: wallet.limits.deployment, - feeAllowance: wallet.limits.fees + address: chainWallet.address, + deploymentAllowance: chainWallet.limits.deployment, + feeAllowance: chainWallet.limits.fees }, { returning: true } );
26-43: Consider a transactional boundary around create/authorize/update with compensating delete on failure.You already compensate by deleting on failure, which is good. A DB transaction for the
updateByIdstep would further reduce the risk of partial writes under concurrent access (e.g., if there are triggers or follow-up writes).Would you like me to sketch a transaction-aware version using your repository abstractions?
apps/api/src/app/services/trial-started/trial-started.handler.ts (1)
37-82: DRY the scheduling of follow-up notifications.The four enqueue calls are repetitive. A small loop reduces duplication and the chance of inconsistent keys.
Apply this diff:
- await Promise.all([ - this.jobQueueManager.enqueue( - new NotificationJob({ - template: "beforeTrialEnds", - userId: user.id, - conditions: notificationConditions - }), - { - singletonKey: `beforeTrialEnds.${user.id}.${TRIAL_ALLOWANCE_EXPIRATION_DAYS - 7}`, - startAfter: addDays(new Date(), TRIAL_ALLOWANCE_EXPIRATION_DAYS - 7) - } - ), - this.jobQueueManager.enqueue( - new NotificationJob({ - template: "beforeTrialEnds", - userId: user.id, - conditions: notificationConditions - }), - { - singletonKey: `beforeTrialEnds.${user.id}.${TRIAL_ALLOWANCE_EXPIRATION_DAYS - 1}`, - startAfter: addDays(new Date(), TRIAL_ALLOWANCE_EXPIRATION_DAYS - 1) - } - ), - this.jobQueueManager.enqueue( - new NotificationJob({ - template: "trialEnded", - userId: user.id, - conditions: notificationConditions - }), - { - singletonKey: `trialEnded.${user.id}`, - startAfter: addDays(new Date(), TRIAL_ALLOWANCE_EXPIRATION_DAYS) - } - ), - this.jobQueueManager.enqueue( - new NotificationJob({ - template: "afterTrialEnds", - userId: user.id, - conditions: notificationConditions - }), - { - singletonKey: `afterTrialEnds.${user.id}`, - startAfter: addDays(new Date(), TRIAL_ALLOWANCE_EXPIRATION_DAYS + 7) - } - ) - ]); + const schedules: Array<{ template: NotificationJob["data"]["template"]; offsetDays: number; keySuffix?: string }> = [ + { template: "beforeTrialEnds", offsetDays: TRIAL_ALLOWANCE_EXPIRATION_DAYS - 7, keySuffix: String(TRIAL_ALLOWANCE_EXPIRATION_DAYS - 7) }, + { template: "beforeTrialEnds", offsetDays: TRIAL_ALLOWANCE_EXPIRATION_DAYS - 1, keySuffix: String(TRIAL_ALLOWANCE_EXPIRATION_DAYS - 1) }, + { template: "trialEnded", offsetDays: TRIAL_ALLOWANCE_EXPIRATION_DAYS }, + { template: "afterTrialEnds", offsetDays: TRIAL_ALLOWANCE_EXPIRATION_DAYS + 7 } + ]; + + await Promise.all( + schedules.map(s => + this.jobQueueManager.enqueue( + new NotificationJob({ + template: s.template, + userId: user.id, + conditions: notificationConditions + }), + { + singletonKey: `${s.template}.${user.id}${s.keySuffix ? `.${s.keySuffix}` : ""}`, + startAfter: addDays(new Date(), s.offsetDays) + } + ) + ) + );Also consider clamping negative offsets (e.g., when the config is < 7 days) if your queue/backoff semantics don’t already handle “start in the past”.
apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts (3)
154-157: Avoid clearing the global DI container from within test setup.
container.clearInstances()clears the root container and may cause cross-test interference. Not needed when using a child container; if you must clear, call it on the child.Apply this diff:
- container.clearInstances(); - return di;If you intended to clear the child container, use
di.clearInstances()right before returning (rarely needed).
35-48: Make the feature-flag gating explicit in assertions.Since ANONYMOUS_FREE_TRIAL is enabled in this test, assert that no domain event is published to avoid regressions.
Apply this diff:
await di.resolve(WalletInitializerService).initializeAndGrantTrialLimits(userId); expect(getOrCreateWallet).toHaveBeenCalledWith({ userId }); expect(createAndAuthorizeTrialSpending).toHaveBeenCalledWith({ addressIndex: newWallet.id }); expect(updateWalletById).toHaveBeenCalledWith( newWallet.id, { address: chainWallet.address, deploymentAllowance: chainWallet.limits.deployment, feeAllowance: chainWallet.limits.fees }, expect.any(Object) ); + expect(di.resolve(DomainEventsService).publish).not.toHaveBeenCalled();
50-50: Nit: test description grammar.Rename to “does not authorize trial spending for existing wallet”.
Apply this diff:
- it("does not authorizes trial spending for existing wallet", async () => { + it("does not authorize trial spending for existing wallet", async () => {apps/api/src/console.ts (1)
141-144: Align the wrapper’s options typing.The inner function already types
optionsasOptionValues. Keep the returned wrapper consistent.Apply this diff:
-function daemon(fn: (options: OptionValues, command: Command) => Promise<unknown>): (options: OptionValues, command: Command) => Promise<void> { - return async (options: Record<string, unknown>, command: Command) => { +function daemon(fn: (options: OptionValues, command: Command) => Promise<unknown>): (options: OptionValues, command: Command) => Promise<void> { + return async (options: OptionValues, command: Command) => { await executeCliHandler(command.name(), () => fn(options, command), { type: "daemon" }); }; }apps/api/src/core/services/job-queue/job-queue.service.ts (2)
32-35: RemovenamefromcreateQueueoptions; the queue name is already passed as the first argument.Passing
nameinside options is redundant and not part of PgBoss createQueue options.- await this.pgBoss.createQueue(queueName, { - name: queueName, - retryLimit: 10 - }); + await this.pgBoss.createQueue(queueName, { + retryLimit: 10 + });
90-93: Same here:createQueueoptions shouldn’t includename.- await this.pgBoss.createQueue(queueName, { - name: queueName, - retryLimit: 10 - }); + await this.pgBoss.createQueue(queueName, { + retryLimit: 10 + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (22)
apps/api/package.json(2 hunks)apps/api/src/app.ts(3 hunks)apps/api/src/app/providers/jobs.provider.ts(1 hunks)apps/api/src/app/services/trial-started/trial-started.handler.ts(1 hunks)apps/api/src/billing/controllers/wallet/wallet.controller.ts(0 hunks)apps/api/src/billing/events/trial-started.ts(1 hunks)apps/api/src/billing/repositories/user-wallet/user-wallet.repository.ts(2 hunks)apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts(6 hunks)apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.ts(3 hunks)apps/api/src/console.ts(4 hunks)apps/api/src/core/providers/app-initializer.ts(1 hunks)apps/api/src/core/providers/postgres.provider.ts(2 hunks)apps/api/src/core/services/domain-events/domain-events.service.ts(1 hunks)apps/api/src/core/services/feature-flags/feature-flags.service.ts(2 hunks)apps/api/src/core/services/job-queue/job-queue.service.ts(1 hunks)apps/api/src/notifications/services/notification-handler/notification-handler.ts(1 hunks)apps/api/src/notifications/services/notification-templates/after-trial-ends-notification.ts(1 hunks)apps/api/src/notifications/services/notification-templates/before-trial-ends-notification.ts(1 hunks)apps/api/src/notifications/services/notification-templates/trial-ended-notification.ts(1 hunks)apps/api/src/notifications/services/notification/notification.service.spec.ts(2 hunks)apps/api/src/notifications/services/notification/notification.service.ts(2 hunks)apps/api/test/services/wallet-testing.service.ts(2 hunks)
💤 Files with no reviewable changes (1)
- apps/api/src/billing/controllers/wallet/wallet.controller.ts
🧰 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/api/src/notifications/services/notification-templates/after-trial-ends-notification.tsapps/api/src/notifications/services/notification-templates/trial-ended-notification.tsapps/api/test/services/wallet-testing.service.tsapps/api/src/core/services/domain-events/domain-events.service.tsapps/api/src/core/providers/app-initializer.tsapps/api/src/app/providers/jobs.provider.tsapps/api/src/billing/events/trial-started.tsapps/api/src/core/providers/postgres.provider.tsapps/api/src/notifications/services/notification/notification.service.spec.tsapps/api/src/billing/repositories/user-wallet/user-wallet.repository.tsapps/api/src/notifications/services/notification-templates/before-trial-ends-notification.tsapps/api/src/billing/services/wallet-initializer/wallet-initializer.service.tsapps/api/src/app/services/trial-started/trial-started.handler.tsapps/api/src/app.tsapps/api/src/core/services/job-queue/job-queue.service.tsapps/api/src/notifications/services/notification/notification.service.tsapps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.tsapps/api/src/notifications/services/notification-handler/notification-handler.tsapps/api/src/console.tsapps/api/src/core/services/feature-flags/feature-flags.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/notifications/services/notification-templates/after-trial-ends-notification.tsapps/api/src/notifications/services/notification-templates/trial-ended-notification.tsapps/api/test/services/wallet-testing.service.tsapps/api/src/core/services/domain-events/domain-events.service.tsapps/api/src/core/providers/app-initializer.tsapps/api/src/app/providers/jobs.provider.tsapps/api/src/billing/events/trial-started.tsapps/api/src/core/providers/postgres.provider.tsapps/api/src/notifications/services/notification/notification.service.spec.tsapps/api/src/billing/repositories/user-wallet/user-wallet.repository.tsapps/api/src/notifications/services/notification-templates/before-trial-ends-notification.tsapps/api/src/billing/services/wallet-initializer/wallet-initializer.service.tsapps/api/src/app/services/trial-started/trial-started.handler.tsapps/api/src/app.tsapps/api/src/core/services/job-queue/job-queue.service.tsapps/api/src/notifications/services/notification/notification.service.tsapps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.tsapps/api/src/notifications/services/notification-handler/notification-handler.tsapps/api/src/console.tsapps/api/src/core/services/feature-flags/feature-flags.service.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/api/src/notifications/services/notification/notification.service.spec.tsapps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts
🧠 Learnings (1)
📚 Learning: 2025-07-21T08:24:24.269Z
Learnt from: CR
PR: akash-network/console#0
File: .cursor/rules/no-jest-mock.mdc:0-0
Timestamp: 2025-07-21T08:24:24.269Z
Learning: Applies to **/*.spec.{ts,tsx} : Don't use `jest.mock()` to mock dependencies in test files. Instead, use `jest-mock-extended` to create mocks and pass mocks as dependencies to the service under test.
Applied to files:
apps/api/src/notifications/services/notification/notification.service.spec.ts
🧬 Code Graph Analysis (17)
apps/api/src/notifications/services/notification-templates/after-trial-ends-notification.ts (1)
apps/api/src/notifications/services/notification/notification.service.ts (1)
CreateNotificationInput(74-76)
apps/api/src/notifications/services/notification-templates/trial-ended-notification.ts (1)
apps/api/src/notifications/services/notification/notification.service.ts (1)
CreateNotificationInput(74-76)
apps/api/src/core/services/domain-events/domain-events.service.ts (2)
apps/api/src/core/services/job-queue/job-queue.service.ts (1)
Job(139-143)apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.ts (1)
singleton(11-68)
apps/api/src/core/providers/app-initializer.ts (3)
apps/api/src/app/providers/jobs.provider.ts (1)
ON_APP_START(10-13)apps/api/src/core/services/feature-flags/feature-flags.service.ts (1)
ON_APP_START(80-82)apps/api/src/core/services/job-queue/job-queue.service.ts (1)
ON_APP_START(134-136)
apps/api/src/app/providers/jobs.provider.ts (2)
apps/api/src/core/providers/app-initializer.ts (2)
APP_INITIALIZER(3-3)ON_APP_START(4-4)apps/api/src/core/services/job-queue/job-queue.service.ts (1)
ON_APP_START(134-136)
apps/api/src/billing/events/trial-started.ts (1)
apps/api/src/core/services/domain-events/domain-events.service.ts (1)
DomainEvent(6-8)
apps/api/src/notifications/services/notification/notification.service.spec.ts (2)
apps/api/src/notifications/services/notification/notification.service.ts (1)
CreateNotificationInput(74-76)apps/api/src/notifications/providers/notifications-api.provider.ts (1)
NotificationsApiClient(9-9)
apps/api/src/notifications/services/notification-templates/before-trial-ends-notification.ts (1)
apps/api/src/notifications/services/notification/notification.service.ts (1)
CreateNotificationInput(74-76)
apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.ts (3)
apps/api/src/billing/repositories/user-wallet/user-wallet.repository.ts (2)
UserWalletInput(9-14)UserWalletPublicOutput(27-33)apps/api/src/core/services/feature-flags/feature-flags.ts (1)
FeatureFlags(1-5)apps/api/src/billing/events/trial-started.ts (1)
TrialStarted(4-14)
apps/api/src/app/services/trial-started/trial-started.handler.ts (7)
apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.ts (1)
singleton(11-68)apps/api/src/core/services/domain-events/domain-events.service.ts (1)
singleton(10-17)apps/api/src/notifications/services/notification-handler/notification-handler.ts (2)
singleton(34-68)NotificationJob(20-32)apps/api/src/notifications/services/notification/notification.service.ts (1)
singleton(13-67)apps/api/src/core/services/job-queue/job-queue.service.ts (1)
JobHandler(152-155)apps/api/src/billing/events/trial-started.ts (1)
TrialStarted(4-14)apps/api/src/notifications/services/notification-templates/start-trial-notification.ts (1)
startTrialNotification(4-16)
apps/api/src/app.ts (4)
apps/api/src/core/providers/app-initializer.ts (2)
APP_INITIALIZER(3-3)ON_APP_START(4-4)apps/api/src/app/providers/jobs.provider.ts (1)
ON_APP_START(10-13)apps/api/src/core/services/feature-flags/feature-flags.service.ts (1)
ON_APP_START(80-82)apps/api/src/core/services/job-queue/job-queue.service.ts (1)
ON_APP_START(134-136)
apps/api/src/core/services/job-queue/job-queue.service.ts (5)
apps/api/src/core/providers/app-initializer.ts (3)
APP_INITIALIZER(3-3)AppInitializer(6-8)ON_APP_START(4-4)apps/api/src/app/services/trial-started/trial-started.handler.ts (1)
singleton(13-84)apps/api/src/core/services/domain-events/domain-events.service.ts (1)
singleton(10-17)apps/api/src/notifications/services/notification-handler/notification-handler.ts (1)
singleton(34-68)apps/api/src/app/providers/jobs.provider.ts (1)
ON_APP_START(10-13)
apps/api/src/notifications/services/notification/notification.service.ts (1)
apps/api/src/notifications/providers/notifications-api.provider.ts (2)
NOTIFICATIONS_API_CLIENT(10-10)NotificationsApiClient(9-9)
apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts (5)
apps/api/src/billing/services/managed-user-wallet/managed-user-wallet.service.ts (1)
createAndAuthorizeTrialSpending(44-57)apps/api/src/core/services/feature-flags/feature-flags.ts (1)
FeatureFlags(1-5)apps/api/test/seeders/user-wallet.seeder.ts (1)
UserWalletSeeder(6-29)apps/api/test/seeders/chain-wallet.seeder.ts (1)
createChainWallet(4-12)apps/api/src/billing/events/trial-started.ts (1)
TrialStarted(4-14)
apps/api/src/notifications/services/notification-handler/notification-handler.ts (7)
apps/api/src/notifications/services/notification-templates/trial-ended-notification.ts (1)
trialEndedNotification(4-16)apps/api/src/notifications/services/notification-templates/before-trial-ends-notification.ts (1)
beforeTrialEndsNotification(6-19)apps/api/src/notifications/services/notification-templates/after-trial-ends-notification.ts (1)
afterTrialEndsNotification(4-16)apps/api/src/notifications/services/notification-templates/start-trial-notification.ts (1)
startTrialNotification(4-16)apps/api/src/core/services/job-queue/job-queue.service.ts (3)
Job(139-143)JOB_NAME(145-145)JobHandler(152-155)apps/api/src/app/services/trial-started/trial-started.handler.ts (1)
singleton(13-84)apps/api/src/notifications/services/notification/notification.service.ts (1)
singleton(13-67)
apps/api/src/console.ts (5)
apps/api/src/core/providers/postgres.provider.ts (2)
migratePG(24-24)closeConnections(42-42)apps/api/src/db/dbConnection.ts (1)
chainDb(20-30)apps/api/src/core/providers/app-initializer.ts (2)
APP_INITIALIZER(3-3)ON_APP_START(4-4)apps/api/src/app/providers/jobs.provider.ts (1)
ON_APP_START(10-13)apps/api/src/core/services/job-queue/job-queue.service.ts (1)
ON_APP_START(134-136)
apps/api/src/core/services/feature-flags/feature-flags.service.ts (3)
apps/api/src/core/services/job-queue/job-queue.service.ts (2)
registry(10-137)ON_APP_START(134-136)apps/api/src/core/providers/app-initializer.ts (3)
APP_INITIALIZER(3-3)AppInitializer(6-8)ON_APP_START(4-4)apps/api/src/app/providers/jobs.provider.ts (1)
ON_APP_START(10-13)
🔇 Additional comments (23)
apps/api/src/core/providers/postgres.provider.ts (1)
6-6: Type-safe DI token — LGTMTyping the token as InjectionToken improves DI safety while retaining a simple string token at runtime. No issues spotted.
Also applies to: 28-28
apps/api/src/notifications/services/notification-templates/after-trial-ends-notification.ts (1)
4-16: Solid, pure template functionShape looks correct and keeps idempotency via notificationId. Looks good.
apps/api/src/core/providers/app-initializer.ts (1)
3-8: Clear bootstrap contract via symbol-keyed interface — LGTMSymbol tokens prevent collisions, and the AppInitializer interface cleanly models async startup steps. No concerns.
apps/api/src/billing/repositories/user-wallet/user-wallet.repository.ts (1)
49-57: No action needed: schema defaults cover missing fieldsAll required columns either have defaults or allow nulls, so inserting only userId won’t fail:
• deploymentAllowance & feeAllowance are NOT NULL with
.default("0.00")
• isTrialing, createdAt & updatedAt have default values
• address is nullable (no.notNull()) and unique (Postgres allows multiple NULLs)
• userId is provided and uniqueapps/api/src/notifications/services/notification/notification.service.ts (2)
15-15: Constructor simplification LGTMDropping LoggerService from the ctor tightens the service API and removes side effects. No concerns here.
37-39: Error propagation with cause is solidRe-throwing with the original error in
causeprovides good debuggability and aligns with the updated tests’ expectations.apps/api/src/notifications/services/notification/notification.service.spec.ts (1)
1-1: Good use of jest-mock-extendedUsing
mockDeepcomplies with the testing guideline to avoidjest.mock()and pass typed mocks into SUT.apps/api/src/core/services/feature-flags/feature-flags.service.ts (4)
10-13: Registration as APP_INITIALIZER looks correctDI registration via
@registry([{ token: APP_INITIALIZER, useToken: FeatureFlagsService }])and implementingAppInitializeris consistent with the new startup flow.
21-26: Injectable client factory is a good testability hookAllowing an overridable
createUnleashClientvia DI improves testability and decouples from concrete construction.
75-78: Dispose may want to await the client shutdown (library-dependent)If
destroyWithFlush()is async in your version ofunleash-client(some versions expose async shutdown), consider awaiting it to ensure metrics are flushed before process exit.Would you confirm whether
destroyWithFlush()returns a Promise in your installed version? If yes, switching toawait this.client?.destroyWithFlush()would make the shutdown deterministic.
80-82: ON_APP_START hook wiring LGTMAuto-initialization on app start keeps feature flags ready before use. No issues spotted.
apps/api/test/services/wallet-testing.service.ts (1)
10-11: Domain events spy aligns with the new event-driven flowMocking
DomainEventsService.publishfits the refactor away from direct NotificationService calls.apps/api/src/notifications/services/notification-templates/trial-ended-notification.ts (1)
4-16: LGTM: simple, idempotent templateThe structure matches CreateNotificationInput, and the notificationId format is stable per user. Consistent with other templates.
apps/api/src/app.ts (1)
2-2: Good call: side-effect import to wire job handlers/providersEnsures providers register before initializers run. The import is early enough to avoid DI timing issues.
apps/api/src/core/services/domain-events/domain-events.service.ts (1)
14-16: LGTM: publish delegates to JobQueueManager.enqueueSimple, clear abstraction for domain event publishing.
apps/api/src/billing/events/trial-started.ts (1)
4-14: LGTM: well-typed domain event with symbol-based nameThe DOMAIN_EVENT_NAME symbol usage avoids string duplication and the payload is precisely typed to UserOutput["id"].
apps/api/src/notifications/services/notification-handler/notification-handler.ts (1)
20-32: Job definition and template typing are clean.Good use of a symbolic job name and a constrained template key type.
apps/api/src/app/services/trial-started/trial-started.handler.ts (2)
29-33: Immediate “start trial” notification path looks good.The guard on
user.emailand structured logging around send attempt are appropriate.
35-36: Conditions shape confirmedThe
UserOutputtype includes atrial?: booleanfield—populated infindUserWithWalletfromuserWallets.isTrialing(defaulting totrue). The{ trial: true }guard therefore matches the actual shape and will work as intended.apps/api/src/console.ts (2)
105-109: Confirm daemon semantics for drain-job-queues.Wrapping
drain-job-queuesindaemon()prevents shutdown in finally. Ifdrain()completes and should exit, consider using a regular action. If it’s long-running (watch/worker), daemon is correct.Would you like this to be a one-shot drain (exit after processing) or a long-running queue worker? I can adjust accordingly.
146-153: Graceful shutdown path LGTM.Using
onceto guard shutdown and wiring SIGTERM/SIGINT is solid. Deferred require also avoids circular deps.apps/api/src/core/services/job-queue/job-queue.service.ts (2)
85-119: Ensure the job-queue worker is startedI found that
drain()is only invoked in the CLI, not in the main API process:
- apps/api/src/console.ts:108
.command("drain-job-queues")….action(() => container.resolve(JobQueueManager).drain())If you intend to run a separate worker via this CLI command, you’re all set—just deploy and run
drain-job-queues. Otherwise, add a call tocontainer.resolve(JobQueueManager).drain()in your API’s bootstrap (for example, inapps/api/src/main.tsafterregisterHandlers(...)) to ensure jobs are processed on startup.
94-115: No explicit acknowledgements needed for pg-boss batch handlersAccording to the pg-boss documentation, returning a Promise from your
work()handler (including the batch form) is sufficient for pg-boss to mark jobs as completed or failed. Explicit calls tojob.done()orboss.complete()per item aren’t required when you return a promise. Your current implementation withPromise.allSettledcorrectly handles job completion semantics.Likely an incorrect or invalid review comment.
apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.ts
Outdated
Show resolved
Hide resolved
apps/api/src/notifications/services/notification-handler/notification-handler.ts
Outdated
Show resolved
Hide resolved
apps/api/src/notifications/services/notification-templates/before-trial-ends-notification.ts
Outdated
Show resolved
Hide resolved
stalniy
force-pushed
the
feat/trial-events
branch
from
c3d80ca to
ca86f1f
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/src/core/services/feature-flags/feature-flags.service.ts (1)
21-21: Fix tsyringe inject option name: use optional, not isOptionaltsyringe’s inject decorator expects { optional: true }, not isOptional. As-is, resolution can fail if the token isn’t registered, and the default param won’t help because DI resolution happens before invoking the constructor.
Apply this diff:
- @inject("createUnleashClient", { isOptional: true }) createClient = createUnleashClient + @inject("createUnleashClient", { optional: true }) createClient = createUnleashClient
♻️ Duplicate comments (10)
apps/api/src/billing/repositories/user-wallet/user-wallet.repository.ts (2)
49-69: Prefer onConflictDoNothing; avoid mutating existing rows and compute isNew without timestamp heuristicsUpdating updatedAt on conflict causes unnecessary writes and skews auditing semantics. Also, detecting “isNew” by createdAt === updatedAt depends on DB defaults matching exactly. Use onConflictDoNothing and infer isNew from insert returning instead.
Apply this diff to simplify and make the flow race-safe without mutating existing rows:
async getOrCreate(input: { userId: Exclude<UserWalletInput["userId"], undefined | null> }): Promise<{ wallet: UserWalletOutput; isNew: boolean }> { - const foundWallet = await this.findOneByUserId(input.userId); - if (foundWallet) return { wallet: foundWallet, isNew: false }; + const foundWallet = await this.findOneByUserId(input.userId); + if (foundWallet) return { wallet: foundWallet, isNew: false }; this.ability?.throwUnlessCanExecute(input); - const [potentiallyNewWallet] = await this.cursor + const [inserted] = await this.cursor .insert(this.table) .values(input) - .onConflictDoUpdate({ - target: [this.table.userId], - set: { - updatedAt: sql`now()` - } - }) + .onConflictDoNothing({ target: [this.table.userId] }) .returning(); - return { - wallet: this.toOutput(potentiallyNewWallet), - isNew: potentiallyNewWallet.createdAt!.getTime() === potentiallyNewWallet.updatedAt!.getTime() - }; + if (inserted) { + return { wallet: this.toOutput(inserted), isNew: true }; + } + // Another writer raced us; fetch the now-existing wallet. + const wallet = await this.findOneByUserId(input.userId); + return { wallet: wallet!, isNew: false }; }
49-69: ON CONFLICT target requires a unique constraint on UserWallets.userId (likely missing)Without a unique (or exclusion) constraint backing the ON CONFLICT target on userId, Postgres will error at runtime. Please ensure a UNIQUE constraint or UNIQUE INDEX exists on user_wallets.userId.
Run this script to locate a unique constraint/index on userId in your Drizzle migrations/schema:
#!/bin/bash # Search for unique constraint/index on user_wallets.userId rg -n -C3 -P '(?i)(user_wallets|UserWallets).*(unique|uniqueIndex).*userId|CONSTRAINT.*UNIQUE.*\(([^)]*userId[^)]*)\)|CREATE\s+UNIQUE\s+INDEX[^\n]*\(\s*"?userId"?\s*\)' \ apps/api | sed -n '1,200p'apps/api/src/notifications/services/notification-templates/before-trial-ends-notification.ts (1)
6-19: Bug: daysLeft computed from createdAt yields negative/incorrect values; accept daysLeft or compute to trial enddifferenceInDays(user.createdAt!, new Date()) reports days elapsed since signup, not days remaining. This will produce negative numbers and unstable notificationId. Also, the non-null assertion on createdAt is unsafe.
Apply this minimal, template-focused change to accept a daysLeft argument and format safely:
-import { differenceInDays } from "date-fns"; - import type { UserOutput } from "@src/user/repositories"; import type { CreateNotificationInput } from "../notification/notification.service"; -export function beforeTrialEndsNotification(user: UserOutput): CreateNotificationInput { - const daysLeft = differenceInDays(user.createdAt!, new Date()); - return { - notificationId: `beforeTrialEnds.${daysLeft}.${user.id}`, - payload: { - summary: "Your Trial Ends Soon", - description: `Your trial of Akash Network is ending in ${daysLeft} days` - }, - user: { - id: user.id, - email: user.email - } - }; -} +export function beforeTrialEndsNotification(user: UserOutput, daysLeft: number): CreateNotificationInput { + const safeDaysLeft = Math.max(0, Math.floor(daysLeft)); + return { + notificationId: `beforeTrialEnds.${safeDaysLeft}.${user.id}`, + payload: { + summary: "Your Trial Ends Soon", + description: `Your trial of Akash Network is ending in ${safeDaysLeft} day${safeDaysLeft === 1 ? "" : "s"}` + }, + user: { + id: user.id, + email: user.email + } + }; +}Follow-ups outside this file (can help implement quickly):
- In TrialStartedHandler, when scheduling the two reminders, include payload.daysLeft equal to 7 and 1 respectively (or compute based on your configured TRIAL_ALLOWANCE_EXPIRATION_DAYS).
- In NotificationHandler, when resolving the template, call beforeTrialEndsNotification(user, payload.daysLeft).
apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.ts (2)
21-25: Avoid non-null assertion on userId; validate it explicitly (prev feedback still applies)userId originates from a Partial type; using userId! can throw at runtime without a clear error. Guard it once and drop non-null assertions.
- async initializeAndGrantTrialLimits(userId: UserWalletInput["userId"]): Promise<UserWalletPublicOutput> { - const { wallet, isNew } = await this.userWalletRepository.accessibleBy(this.authService.ability, "create").getOrCreate({ userId: userId! }); + async initializeAndGrantTrialLimits(userId: UserWalletInput["userId"]): Promise<UserWalletPublicOutput> { + if (!userId) { + throw new Error("userId is required to initialize and grant trial limits"); + } + const { wallet, isNew } = await this.userWalletRepository + .accessibleBy(this.authService.ability, "create") + .getOrCreate({ userId });
46-51: Bug: publishing TrialStarted with currentUser.id instead of the target userId (prev feedback still applies)Admin/system-initiated flows will emit the event for the actor, not the wallet owner. Publish with the validated method argument userId.
- await this.domainEvents.publish( - new TrialStarted({ - userId: this.authService.currentUser.id - }) - ); + await this.domainEvents.publish( + new TrialStarted({ + userId + }) + );apps/api/src/core/services/job-queue/job-queue.service.ts (5)
29-36:retryLimitincreateQueueis a no-op; remove it to avoid confusion.PgBoss applies retries per job send/publish, not at queue creation. Keeping it here misleads and has no effect.
Apply this diff in both places:
- await this.pgBoss.createQueue(queueName, { - name: queueName, - retryLimit: 10 - }); + await this.pgBoss.createQueue(queueName);Also applies to: 90-93
95-113: Sanitize worker logs to avoid PII/oversized payloads; log metadata only.Avoid logging complete job objects or arbitrary errors. Keep logs small and non-sensitive.
Apply this diff:
- this.logger.info({ - event: "JOBS_STARTED", - jobs - }); + this.logger.info({ + event: "JOBS_STARTED", + queue: queueName, + count: jobs.length + }); @@ - if (result.status === "fulfilled") { - this.logger.info({ - event: "JOB_DONE", - job: jobs[index] - }); - } else { - this.logger.error({ - event: "JOB_FAILED", - job: jobs[index], - error: result.reason - }); - } + if (result.status === "fulfilled") { + this.logger.info({ + event: "JOB_DONE", + queue: queueName, + id: jobs[index].id + }); + } else { + this.logger.error({ + event: "JOB_FAILED", + queue: queueName, + id: jobs[index].id, + error: + result.reason instanceof Error + ? { name: result.reason.name, message: result.reason.message } + : String(result.reason) + }); + }
16-27: Fix tsyringe optional injection: useoptional, notisOptional.
@injectexpects{ optional: boolean }. Current code won’t type-check and makes the dependency non-optional at runtime.Apply this diff:
- @inject(PG_BOSS_TOKEN, { isOptional: true }) pgBoss?: PgBoss + @inject(PG_BOSS_TOKEN, { optional: true }) pgBoss?: PgBoss
72-83: Enqueue is targeting the wrong queue and logs PII; use static[JOB_NAME], set default retries, and sanitize logs.
- Workers are registered on queues named by the class’ static
[JOB_NAME]; publishing tojob.namerisks orphaned jobs.- Log only safe metadata, not the full job payload.
- Apply a sensible default
retryLimitat enqueue-time.Apply this diff:
async enqueue(job: Job, options?: EnqueueOptions): Promise<string | null> { - this.logger.info({ - event: "JOB_ENQUEUED", - job - }); + const name = (job.constructor as JobType<Job>)[JOB_NAME] ?? job.name; + const optionsWithDefaults: EnqueueOptions = { retryLimit: 10, ...options }; + this.logger.info({ + event: "JOB_ENQUEUED", + queue: name, + version: job.version + }); - return await this.pgBoss.send({ - name: job.name, - data: { ...job.data, version: job.version }, - options - }); + return await this.pgBoss.send({ + name, + data: { ...job.data, version: job.version }, + options: optionsWithDefaults + }); }
147-150: Avoidanyin public types; usereadonly unknown[]for ctor args.Repo guideline forbids
any. This keeps the type flexible without usingany.Apply this diff:
export type JobType<T extends Job> = { - new (...args: any[]): T; + new (...args: readonly unknown[]): T; [JOB_NAME]: string; };
🧹 Nitpick comments (5)
apps/api/src/core/services/feature-flags/feature-flags.service.ts (2)
67-70: Harden initialization: add timeout when waiting for synchronizedIf the Unleash server is unreachable, initialize() will hang indefinitely. Add a timeout (e.g., 10s or from config) to fail fast with a clear error.
Apply this diff:
- await new Promise((resolve, reject) => { - client.once("synchronized", resolve); - client.once("error", reject); - }); + const timeoutMs = 10_000; + await Promise.race([ + new Promise<void>((resolve, reject) => { + client.once("synchronized", () => resolve()); + client.once("error", err => reject(err)); + }), + new Promise((_, reject) => + setTimeout(() => reject(new Error(`Unleash client did not synchronize within ${timeoutMs}ms`)), timeoutMs) + ) + ]);
75-78: Verify destroyWithFlush availability; fall back to destroy()Some unleash-client versions expose destroy() (flushing internally) but not destroyWithFlush(). If destroyWithFlush is undefined, disposal will fail silently.
Apply this defensive diff:
dispose(): void { - this.client?.destroyWithFlush(); + // destroyWithFlush may not exist in all versions; use destroy if unavailable. + const anyClient = this.client as unknown as { destroyWithFlush?: () => void; destroy?: () => void }; + if (anyClient?.destroyWithFlush) anyClient.destroyWithFlush(); + else anyClient?.destroy?.(); this.client?.removeAllListeners(); }apps/api/src/core/services/job-queue/job-queue.service.spec.ts (1)
251-271: Place setup at the very bottom of the root describe block (style guideline)Our tests guideline asks for the setup function at the bottom of the root describe. Here, TestJob/TestHandler classes follow setup. Consider moving setup below those helper classes.
apps/api/src/notifications/services/notification-handler/notification.handler.spec.ts (1)
134-161: Test name mismatches what’s asserted; either rename or assert days explicitlyThe test claims “correct days calculation” but it doesn’t validate any day math; it only asserts the template function result is passed through. Either:
- Rename the test to reflect what it actually verifies (creation), or
- Add an assertion on the generated notificationId/payload that encodes “days left”.
Here’s a minimal rename (keeps intent clear without over-coupling to implementation details):
- it("creates beforeTrialEnds notification with correct days calculation", async () => { + it("creates beforeTrialEnds notification", async () => {Additionally, double-check the day math in before-trial-ends-notification.ts. The snippet shows differenceInDays(user.createdAt!, new Date()), which yields negative values for past createdAt. If the intent is “days until trial ends”, the template likely needs awareness of trial duration to compute a positive “days left”.
apps/api/src/console.ts (1)
1-6: Load reflect-metadata first to avoid DI edge casesIt’s safer to import reflect-metadata before anything else to ensure decorators/metadata are available for DI across all modules.
-import "@akashnetwork/env-loader"; -import "@src/utils/protobuf"; -import "reflect-metadata"; +import "reflect-metadata"; +import "@akashnetwork/env-loader"; +import "@src/utils/protobuf"; import "./open-telemetry"; import "./app/providers/jobs.provider";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (26)
apps/api/package.json(2 hunks)apps/api/src/app.ts(3 hunks)apps/api/src/app/providers/jobs.provider.ts(1 hunks)apps/api/src/app/services/trial-started/trial-started.handler.spec.ts(1 hunks)apps/api/src/app/services/trial-started/trial-started.handler.ts(1 hunks)apps/api/src/billing/controllers/wallet/wallet.controller.ts(0 hunks)apps/api/src/billing/events/trial-started.ts(1 hunks)apps/api/src/billing/repositories/user-wallet/user-wallet.repository.ts(2 hunks)apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts(6 hunks)apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.ts(3 hunks)apps/api/src/console.ts(4 hunks)apps/api/src/core/providers/app-initializer.ts(1 hunks)apps/api/src/core/providers/postgres.provider.ts(2 hunks)apps/api/src/core/services/domain-events/domain-events.service.ts(1 hunks)apps/api/src/core/services/feature-flags/feature-flags.service.ts(2 hunks)apps/api/src/core/services/job-queue/job-queue.service.spec.ts(1 hunks)apps/api/src/core/services/job-queue/job-queue.service.ts(1 hunks)apps/api/src/notifications/services/notification-handler/notification.handler.spec.ts(1 hunks)apps/api/src/notifications/services/notification-handler/notification.handler.ts(1 hunks)apps/api/src/notifications/services/notification-templates/after-trial-ends-notification.ts(1 hunks)apps/api/src/notifications/services/notification-templates/before-trial-ends-notification.ts(1 hunks)apps/api/src/notifications/services/notification-templates/trial-ended-notification.ts(1 hunks)apps/api/src/notifications/services/notification/notification.service.spec.ts(2 hunks)apps/api/src/notifications/services/notification/notification.service.ts(2 hunks)apps/api/test/seeders/user.seeder.ts(2 hunks)apps/api/test/services/wallet-testing.service.ts(2 hunks)
💤 Files with no reviewable changes (1)
- apps/api/src/billing/controllers/wallet/wallet.controller.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- apps/api/test/services/wallet-testing.service.ts
- apps/api/src/app/providers/jobs.provider.ts
- apps/api/src/notifications/services/notification-templates/trial-ended-notification.ts
- apps/api/package.json
- apps/api/src/core/providers/app-initializer.ts
- apps/api/src/notifications/services/notification-templates/after-trial-ends-notification.ts
- apps/api/src/core/providers/postgres.provider.ts
- apps/api/src/notifications/services/notification/notification.service.ts
- apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts
- apps/api/src/core/services/domain-events/domain-events.service.ts
- apps/api/src/app/services/trial-started/trial-started.handler.ts
- apps/api/src/app.ts
- apps/api/src/notifications/services/notification/notification.service.spec.ts
🧰 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/api/src/billing/events/trial-started.tsapps/api/src/app/services/trial-started/trial-started.handler.spec.tsapps/api/src/notifications/services/notification-handler/notification.handler.spec.tsapps/api/src/notifications/services/notification-templates/before-trial-ends-notification.tsapps/api/src/notifications/services/notification-handler/notification.handler.tsapps/api/src/core/services/job-queue/job-queue.service.spec.tsapps/api/test/seeders/user.seeder.tsapps/api/src/billing/repositories/user-wallet/user-wallet.repository.tsapps/api/src/billing/services/wallet-initializer/wallet-initializer.service.tsapps/api/src/core/services/job-queue/job-queue.service.tsapps/api/src/core/services/feature-flags/feature-flags.service.tsapps/api/src/console.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/billing/events/trial-started.tsapps/api/src/app/services/trial-started/trial-started.handler.spec.tsapps/api/src/notifications/services/notification-handler/notification.handler.spec.tsapps/api/src/notifications/services/notification-templates/before-trial-ends-notification.tsapps/api/src/notifications/services/notification-handler/notification.handler.tsapps/api/src/core/services/job-queue/job-queue.service.spec.tsapps/api/test/seeders/user.seeder.tsapps/api/src/billing/repositories/user-wallet/user-wallet.repository.tsapps/api/src/billing/services/wallet-initializer/wallet-initializer.service.tsapps/api/src/core/services/job-queue/job-queue.service.tsapps/api/src/core/services/feature-flags/feature-flags.service.tsapps/api/src/console.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/api/src/app/services/trial-started/trial-started.handler.spec.tsapps/api/src/notifications/services/notification-handler/notification.handler.spec.tsapps/api/src/core/services/job-queue/job-queue.service.spec.ts
🧠 Learnings (1)
📚 Learning: 2025-07-27T12:16:08.566Z
Learnt from: CR
PR: akash-network/console#0
File: .cursor/rules/general.mdc:0-0
Timestamp: 2025-07-27T12:16:08.566Z
Learning: Applies to **/*.{ts,tsx} : Never use type any or cast to type any. Always define the proper TypeScript types.
Applied to files:
apps/api/src/core/services/job-queue/job-queue.service.ts
🧬 Code Graph Analysis (10)
apps/api/src/billing/events/trial-started.ts (1)
apps/api/src/core/services/domain-events/domain-events.service.ts (1)
DomainEvent(6-8)
apps/api/src/app/services/trial-started/trial-started.handler.spec.ts (2)
apps/api/test/seeders/user.seeder.ts (1)
UserSeeder(5-45)apps/api/src/notifications/services/notification-handler/notification.handler.ts (1)
NotificationJob(20-32)
apps/api/src/notifications/services/notification-handler/notification.handler.spec.ts (5)
apps/api/test/seeders/user.seeder.ts (1)
UserSeeder(5-45)apps/api/src/notifications/services/notification-templates/trial-ended-notification.ts (1)
trialEndedNotification(4-16)apps/api/src/notifications/services/notification-templates/start-trial-notification.ts (1)
startTrialNotification(4-16)apps/api/src/notifications/services/notification-templates/before-trial-ends-notification.ts (1)
beforeTrialEndsNotification(6-19)apps/api/src/notifications/services/notification-templates/after-trial-ends-notification.ts (1)
afterTrialEndsNotification(4-16)
apps/api/src/notifications/services/notification-templates/before-trial-ends-notification.ts (1)
apps/api/src/notifications/services/notification/notification.service.ts (1)
CreateNotificationInput(74-76)
apps/api/src/notifications/services/notification-handler/notification.handler.ts (7)
apps/api/src/notifications/services/notification-templates/trial-ended-notification.ts (1)
trialEndedNotification(4-16)apps/api/src/notifications/services/notification-templates/before-trial-ends-notification.ts (1)
beforeTrialEndsNotification(6-19)apps/api/src/notifications/services/notification-templates/after-trial-ends-notification.ts (1)
afterTrialEndsNotification(4-16)apps/api/src/notifications/services/notification-templates/start-trial-notification.ts (1)
startTrialNotification(4-16)apps/api/src/core/services/job-queue/job-queue.service.ts (3)
Job(139-143)JOB_NAME(145-145)JobHandler(152-155)apps/api/src/app/services/trial-started/trial-started.handler.ts (1)
singleton(13-90)apps/api/src/notifications/services/notification/notification.service.ts (1)
singleton(13-67)
apps/api/src/core/services/job-queue/job-queue.service.spec.ts (3)
apps/api/src/core/services/job-queue/job-queue.service.ts (4)
Job(139-143)JOB_NAME(145-145)JobHandler(152-155)ON_APP_START(134-136)apps/api/src/app/providers/jobs.provider.ts (1)
ON_APP_START(10-13)apps/api/src/core/providers/app-initializer.ts (1)
ON_APP_START(4-4)
apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.ts (3)
apps/api/src/billing/repositories/user-wallet/user-wallet.repository.ts (2)
UserWalletInput(9-14)UserWalletPublicOutput(27-33)apps/api/src/core/services/feature-flags/feature-flags.ts (1)
FeatureFlags(1-5)apps/api/src/billing/events/trial-started.ts (1)
TrialStarted(4-14)
apps/api/src/core/services/job-queue/job-queue.service.ts (5)
apps/api/src/core/providers/app-initializer.ts (3)
APP_INITIALIZER(3-3)AppInitializer(6-8)ON_APP_START(4-4)apps/api/src/app/services/trial-started/trial-started.handler.ts (1)
singleton(13-90)apps/api/src/core/services/domain-events/domain-events.service.ts (1)
singleton(10-17)apps/api/src/notifications/services/notification-handler/notification.handler.ts (1)
singleton(34-68)apps/api/src/app/providers/jobs.provider.ts (1)
ON_APP_START(10-13)
apps/api/src/core/services/feature-flags/feature-flags.service.ts (3)
apps/api/src/core/services/job-queue/job-queue.service.ts (2)
registry(10-137)ON_APP_START(134-136)apps/api/src/core/providers/app-initializer.ts (3)
APP_INITIALIZER(3-3)AppInitializer(6-8)ON_APP_START(4-4)apps/api/src/app/providers/jobs.provider.ts (1)
ON_APP_START(10-13)
apps/api/src/console.ts (5)
apps/api/src/core/providers/postgres.provider.ts (2)
migratePG(24-24)closeConnections(42-42)apps/api/src/db/dbConnection.ts (1)
chainDb(20-30)apps/api/src/core/providers/app-initializer.ts (2)
APP_INITIALIZER(3-3)ON_APP_START(4-4)apps/api/src/app/providers/jobs.provider.ts (1)
ON_APP_START(10-13)apps/api/src/core/services/job-queue/job-queue.service.ts (1)
ON_APP_START(134-136)
⏰ 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). (14)
- GitHub Check: validate (apps/provider-proxy) / validate-unsafe
- GitHub Check: validate (apps/provider-console) / validate-unsafe
- 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: 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 (14)
apps/api/src/billing/repositories/user-wallet/user-wallet.repository.ts (1)
1-1: Import of sql is appropriateThe addition of sql is justified by its usage in the upsert.
apps/api/src/core/services/feature-flags/feature-flags.service.ts (2)
10-12: Good: wired into app startup via APP_INITIALIZERRegistering FeatureFlagsService as an app initializer and implementing [ON_APP_START] aligns it with the new startup framework.
31-48: Context building looks solidUsing execution context for user and client info with environment properties is consistent. No issues spotted.
apps/api/src/core/services/job-queue/job-queue.service.spec.ts (1)
1-249: Strong test coverage across the JobQueueService API surfaceGood use of jest-mock-extended, thorough assertions for registerHandlers, enqueue, drain, start, dispose, and ON_APP_START.
apps/api/test/seeders/user.seeder.ts (1)
22-24: Seeder now supports trial state; LGTMDefaulting trial to false and including createdAt in the output matches the expanded UserOutput. No issues.
apps/api/src/billing/events/trial-started.ts (1)
4-13: Event shape and typing look solidThe event follows the Job/DomainEvent contract, uses the computed static name pattern consistently, and types data.userId via UserOutput["id"] which keeps it aligned with the user repo. LGTM.
apps/api/src/notifications/services/notification-handler/notification.handler.spec.ts (1)
234-249: Good adherence to testing guidelines and clean setup pattern
- Mocks created via jest-mock-extended (no jest.mock()).
- setup() lives at the bottom of the root describe, accepts a single typed param, and returns the SUT and mocks.
- No shared state.
LGTM.
apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.ts (1)
46-51: Confirm flag semantics before suppressing event emissionYou currently suppress publishing when ANONYMOUS_FREE_TRIAL is enabled. If the business intent is to always emit the event on trial start (anonymous or not), the gating should be revisited.
If the intent is correct, ignore this. Otherwise, invert or adjust the condition.
apps/api/src/app/services/trial-started/trial-started.handler.spec.ts (3)
88-156: Strong coverage for scheduling semanticsVerifies all four enqueues with correct templates, singleton keys, and startAfter offsets under a fixed “now” and multiple trial durations. This is exactly the right level of behavior verification. LGTM.
29-48: Good behavior for “no email” pathAsserting no notification creation while still enqueuing the reminder jobs covers the nuanced behavior well. LGTM.
191-216: Setup helper is clean and localized
- Uses jest-mock-extended.
- Single typed param, no shared state, returns SUT and mocks.
- Configurable trialExpirationDays for scenario coverage.
LGTM.
apps/api/src/console.ts (2)
118-121: Initializers run in parallel with DB migration/auth; consider sequencingSome initializers (e.g., job queue start/handler registration) may assume DB is migrated and connected. Safer to await DB readiness first, then run initializers.
- await Promise.all([migratePG(), chainDb.authenticate(), ...container.resolveAll(APP_INITIALIZER).map(initializer => initializer[ON_APP_START]())]); + await Promise.all([migratePG(), chainDb.authenticate()]); + await Promise.all(container.resolveAll(APP_INITIALIZER).map(initializer => initializer[ON_APP_START]()));If any initializer depends on schema changes, this avoids race conditions during startup.
105-109: CLI daemon wrapper and drain wiring look goodThe daemonization pattern keeps lifecycle handling centralized (executeCliHandler + shutdown guard). The drain command uses JobQueueService, consistent with the new queue abstraction.
apps/api/src/notifications/services/notification-handler/notification.handler.ts (1)
20-24: Good job: clear job contract with static queue name and versioning.Using a static
[JOB_NAME]and explicit versioning onNotificationJobis aligned with the queueing model and makes migrations easier.
apps/api/src/notifications/services/notification-handler/notification.handler.ts
Outdated
Show resolved
Hide resolved
stalniy
force-pushed
the
feat/trial-events
branch
from
ca86f1f to
be4adb0
Compare
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 (1)
apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts (1)
146-157: Do not clear the global tsyringe container inside setup; this can cause cross-test flakiness.setup() creates a child container (di), but then calls container.clearInstances() on the shared root container. This mutates global state and can break other suites running in parallel. Child containers are sufficient for isolation here; remove the clearInstances() on the root.
di.registerInstance( FeatureFlagsService, mock<FeatureFlagsService>({ isEnabled: jest.fn(flag => !!input?.enabledFeatures?.includes(flag)) }) ); - - container.clearInstances(); return di;If you need teardown, prefer per-test disposal of the child container (e.g., di.clearInstances()) in the test that owns it, or avoid clearing altogether since each test calls setup() and uses a fresh child container.
♻️ Duplicate comments (6)
apps/api/src/billing/repositories/user-wallet/user-wallet.repository.ts (1)
57-59: Ensure UNIQUE constraint on user_wallets.userId for ON CONFLICT target.Postgres requires a unique/exclusion constraint backing the target of ON CONFLICT; otherwise this will error at runtime. Please confirm a UNIQUE constraint or index exists on user_wallets(userId) in your migrations. If missing, add one (migration examples below). This was flagged earlier; repeating here as it remains a hard requirement.
Migration examples (pick one):
ALTER TABLE "user_wallets" ADD CONSTRAINT "user_wallets_userId_unique" UNIQUE ("userId"); -- or: CREATE UNIQUE INDEX "idx_user_wallets_userId" ON "user_wallets" ("userId");Run this script to verify in-repo (searches migrations/schema for a constraint or unique index on userId):
#!/bin/bash # Find Drizzle migrations/schema and check for a UNIQUE on user_wallets.userId fd -t f -i -g '*drizzle*' -g '*migration*' -g '*schema*' 2>/dev/null \ | rg -n -C3 -P '(?i)(user_wallets|UserWallets).*(unique|constraint|index).*userId|CREATE\s+UNIQUE\s+INDEX.*\(?"?userId"?\)?'apps/api/src/core/services/job-queue/job-queue.service.ts (5)
17-17: Fix tsyringe optional injection: use "optional", not "isOptional".
@inject's second argument expects{ optional: boolean }. Using{ isOptional: true }won't compile and will make this dependency non-optional at runtime.Apply this diff:
- @inject(PG_BOSS_TOKEN, { isOptional: true }) pgBoss?: PgBoss + @inject(PG_BOSS_TOKEN, { optional: true }) pgBoss?: PgBoss
30-33:retryLimitdoes not belong tocreateQueueoptions; it won't take effect.PgBoss applies
retryLimitper-job (send/publish options), not at queue creation. Keeping it here is a no-op and may mislead future readers into thinking retries are configured.Apply this diff at both places:
- await this.pgBoss.createQueue(queueName, { - name: queueName, - retryLimit: 10 - }); + await this.pgBoss.createQueue(queueName);Also applies to: 88-91
141-144: AvoidanyinJobTypeconstructor signature.The repo guidelines disallow
any. Useunknown[]to maintain flexibility while adhering to the guidelines.Apply this diff:
export type JobType<T extends Job> = { - new (...args: any[]): T; + new (...args: unknown[]): T; [JOB_NAME]: string; };
70-81: Enqueue is using instance propertyjob.nameinstead of the static queue name.Jobs are published to
job.name, but workers are registered on queues named via the class' static[JOB_NAME]. If the instancenamedeviates from the static value, jobs will land on an unprocessed queue. Also, logging the full job object may leak sensitive data.Apply this diff to use the static queue name and sanitize logs:
async enqueue(job: Job, options?: EnqueueOptions): Promise<string | null> { + const queueName = (job.constructor as JobType<Job>)[JOB_NAME]; this.logger.info({ event: "JOB_ENQUEUED", - job + queue: queueName, + version: job.version }); return await this.pgBoss.send({ - name: job.name, + name: queueName, data: { ...job.data, version: job.version }, - options + options: { retryLimit: 10, ...options } }); }
93-113: Avoid logging full job payloads to prevent PII leakage.The logs include full job objects which may contain emails, subjects, message bodies, etc. Log only safe metadata instead.
Apply this diff:
this.logger.info({ event: "JOBS_STARTED", - jobsIds: jobs.map(job => job.id) + queue: queueName, + count: jobs.length }); @@ if (result.status === "fulfilled") { this.logger.info({ event: "JOB_DONE", - jobId: jobs[index].id + queue: queueName, + id: jobs[index].id }); } else { this.logger.error({ event: "JOB_FAILED", - jobId: jobs[index].id, + queue: queueName, + id: jobs[index].id, error: result.reason }); }
🧹 Nitpick comments (8)
apps/api/src/billing/repositories/user-wallet/user-wallet.repository.ts (2)
49-52: Guard against empty userId input.Type-wise userId excludes null/undefined, but empty strings can still slip through and reach the insert. Add a fast-fail to avoid invalid rows or DB errors if constraints change.
async getOrCreate(input: { userId: Exclude<UserWalletInput["userId"], undefined | null> }): Promise<{ wallet: UserWalletOutput; isNew: boolean }> { - const foundWallet = await this.findOneByUserId(input.userId); + if (typeof input.userId !== "string" || input.userId.trim().length === 0) { + throw new Error("userId must be a non-empty string"); + } + const foundWallet = await this.findOneByUserId(input.userId); if (foundWallet) return { wallet: foundWallet, isNew: false };
54-56: Nit: insert only the fields you intend to persist.Even though input currently only contains userId, being explicit is safer and consistent with create(). It also future-proofs if the input shape evolves.
- .insert(this.table) - .values(input) + .insert(this.table) + .values({ userId: input.userId })apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts (2)
50-66: Fix test description grammar.Rename to improve readability.
- it("does not authorizes trial spending for existing wallet", async () => { + it("does not authorize trial spending for existing wallet", async () => {
88-106: LGTM: event-driven path when ANONYMOUS_FREE_TRIAL is disabled.The test correctly asserts publication of TrialStarted with the expected payload.
Consider adding a complementary case for “ANONYMOUS_FREE_TRIAL disabled + existing wallet (isNew=false)” to pin intended semantics (should we publish TrialStarted only on first wallet creation or also when the wallet already exists?). If the desired behavior is “only on new,” assert publish not called in that scenario to guard regressions.
Optional sketch:
it('does not publish TrialStarted for existing wallet when feature disabled', async () => { const userId = 'test-user-id'; const existingWallet = UserWalletSeeder.create({ userId }); const di = setup({ getOrCreateWallet: jest.fn().mockResolvedValue({ wallet: existingWallet, isNew: false }), enabledFeatures: [] }); await di.resolve(WalletInitializerService).initializeAndGrantTrialLimits(userId); expect(di.resolve(DomainEventsService).publish).not.toHaveBeenCalled(); });apps/api/src/notifications/services/notification-templates/before-trial-ends-notification.ts (1)
6-14: Consider improving the notification description for better user experience.The template works correctly with the
trialEndsAtparameter passed fromTrialStartedHandler. However, the description could be more user-friendly when handling edge cases.Consider these improvements:
- Handle singular/plural for "day" vs "days"
- Add more context when trial is ending today
export function beforeTrialEndsNotification(user: UserOutput, vars: { trialEndsAt: string }): CreateNotificationInput { const trialEndsDate = new Date(vars.trialEndsAt); const daysLeft = trialEndsDate.getTime() < Date.now() ? 0 : differenceInDays(trialEndsDate, new Date()); + const dayText = daysLeft === 1 ? "day" : "days"; + const description = daysLeft === 0 + ? "Your trial of Akash Network is ending today" + : `Your trial of Akash Network is ending in ${daysLeft} ${dayText}`; return { notificationId: `beforeTrialEnds.${daysLeft}.${user.id}`, payload: { summary: "Your Trial Ends Soon", - description: `Your trial of Akash Network is ending in ${daysLeft} days` + description }, user: { id: user.id, email: user.email } }; }apps/api/src/app/services/trial-started/trial-started.handler.ts (1)
35-39: Consider making the immediate notification asynchronous for better resilience.The immediate notification is sent synchronously while the scheduled notifications use the job queue. For consistency and resilience, consider enqueuing the immediate notification as well.
Consider using the job queue for the immediate notification:
if (user.email) { this.logger.info({ event: "START_TRIAL_NOTIFICATION_SENDING", userId: user.id }); - await this.notificationService.createNotification(startTrialNotification(user)); + await this.jobQueueManager.enqueue( + new NotificationJob({ + template: "startTrial", + userId: user.id + }), + { + singletonKey: `startTrial.${user.id}` + } + ); this.logger.info({ event: "START_TRIAL_NOTIFICATION_SENT", userId: user.id }); }apps/api/src/notifications/services/notification-handler/notification.handler.ts (2)
47-56: The template lookup can never fail; remove the unnecessary check.Since
payload.templateis typed askeyof NotificationTemplatesandnotificationTemplatesis a const object with all keys defined, the lookup will always succeed. Theas GenericNotificationTemplatecast and null check are unnecessary.Simplify the template resolution:
async handle(payload: NotificationJob["data"]): Promise<void> { - const notificationTemplate = notificationTemplates[payload.template] as GenericNotificationTemplate; - if (!notificationTemplate) { - this.logger.error({ - event: "UNKNOWN_NOTIFICATION_TYPE", - type: payload.template - }); - return; - } + const notificationTemplate = notificationTemplates[payload.template];
80-82: Complex type utility could be simplified.The
TemplateVarsParametertype utility is complex and could be made more readable. Additionally, it returnsunknownfor functions with 0 or 1 parameters, which should be more specific.Consider this clearer implementation:
-type TemplateVarsParameter<T extends NotificationTemplates[keyof NotificationTemplates]> = Parameters<T>["length"] extends 0 | 1 - ? unknown - : { vars: Parameters<T>[1] }; +type TemplateVarsParameter<T extends NotificationTemplates[keyof NotificationTemplates]> = + Parameters<T> extends [UserOutput, infer Vars] + ? { vars: Vars } + : {};This makes it clear that templates with a second parameter require
vars, while templates without it don't need the property at all.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (26)
apps/api/package.json(2 hunks)apps/api/src/app.ts(3 hunks)apps/api/src/app/providers/jobs.provider.ts(1 hunks)apps/api/src/app/services/trial-started/trial-started.handler.spec.ts(1 hunks)apps/api/src/app/services/trial-started/trial-started.handler.ts(1 hunks)apps/api/src/billing/controllers/wallet/wallet.controller.ts(0 hunks)apps/api/src/billing/events/trial-started.ts(1 hunks)apps/api/src/billing/repositories/user-wallet/user-wallet.repository.ts(1 hunks)apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts(6 hunks)apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.ts(3 hunks)apps/api/src/console.ts(4 hunks)apps/api/src/core/providers/app-initializer.ts(1 hunks)apps/api/src/core/providers/postgres.provider.ts(2 hunks)apps/api/src/core/services/domain-events/domain-events.service.ts(1 hunks)apps/api/src/core/services/feature-flags/feature-flags.service.ts(2 hunks)apps/api/src/core/services/job-queue/job-queue.service.spec.ts(1 hunks)apps/api/src/core/services/job-queue/job-queue.service.ts(1 hunks)apps/api/src/notifications/services/notification-handler/notification.handler.spec.ts(1 hunks)apps/api/src/notifications/services/notification-handler/notification.handler.ts(1 hunks)apps/api/src/notifications/services/notification-templates/after-trial-ends-notification.ts(1 hunks)apps/api/src/notifications/services/notification-templates/before-trial-ends-notification.ts(1 hunks)apps/api/src/notifications/services/notification-templates/trial-ended-notification.ts(1 hunks)apps/api/src/notifications/services/notification/notification.service.spec.ts(2 hunks)apps/api/src/notifications/services/notification/notification.service.ts(2 hunks)apps/api/test/seeders/user.seeder.ts(2 hunks)apps/api/test/services/wallet-testing.service.ts(2 hunks)
💤 Files with no reviewable changes (1)
- apps/api/src/billing/controllers/wallet/wallet.controller.ts
🚧 Files skipped from review as they are similar to previous changes (18)
- apps/api/src/notifications/services/notification-templates/after-trial-ends-notification.ts
- apps/api/src/core/providers/app-initializer.ts
- apps/api/src/billing/events/trial-started.ts
- apps/api/test/services/wallet-testing.service.ts
- apps/api/src/core/services/job-queue/job-queue.service.spec.ts
- apps/api/src/notifications/services/notification-templates/trial-ended-notification.ts
- apps/api/test/seeders/user.seeder.ts
- apps/api/src/notifications/services/notification-handler/notification.handler.spec.ts
- apps/api/src/core/providers/postgres.provider.ts
- apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.ts
- apps/api/src/app/providers/jobs.provider.ts
- apps/api/src/app/services/trial-started/trial-started.handler.spec.ts
- apps/api/src/core/services/feature-flags/feature-flags.service.ts
- apps/api/src/console.ts
- apps/api/src/notifications/services/notification/notification.service.ts
- apps/api/src/notifications/services/notification/notification.service.spec.ts
- apps/api/package.json
- apps/api/src/core/services/domain-events/domain-events.service.ts
🧰 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/api/src/billing/repositories/user-wallet/user-wallet.repository.tsapps/api/src/notifications/services/notification-handler/notification.handler.tsapps/api/src/core/services/job-queue/job-queue.service.tsapps/api/src/app.tsapps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.tsapps/api/src/notifications/services/notification-templates/before-trial-ends-notification.tsapps/api/src/app/services/trial-started/trial-started.handler.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/billing/repositories/user-wallet/user-wallet.repository.tsapps/api/src/notifications/services/notification-handler/notification.handler.tsapps/api/src/core/services/job-queue/job-queue.service.tsapps/api/src/app.tsapps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.tsapps/api/src/notifications/services/notification-templates/before-trial-ends-notification.tsapps/api/src/app/services/trial-started/trial-started.handler.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/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts
🧠 Learnings (1)
📚 Learning: 2025-07-27T12:16:08.566Z
Learnt from: CR
PR: akash-network/console#0
File: .cursor/rules/general.mdc:0-0
Timestamp: 2025-07-27T12:16:08.566Z
Learning: Applies to **/*.{ts,tsx} : Never use type any or cast to type any. Always define the proper TypeScript types.
Applied to files:
apps/api/src/core/services/job-queue/job-queue.service.ts
🧬 Code graph analysis (5)
apps/api/src/notifications/services/notification-handler/notification.handler.ts (7)
apps/api/src/notifications/services/notification-templates/trial-ended-notification.ts (1)
trialEndedNotification(4-16)apps/api/src/notifications/services/notification-templates/before-trial-ends-notification.ts (1)
beforeTrialEndsNotification(6-20)apps/api/src/notifications/services/notification-templates/after-trial-ends-notification.ts (1)
afterTrialEndsNotification(4-16)apps/api/src/notifications/services/notification-templates/start-trial-notification.ts (1)
startTrialNotification(4-16)apps/api/src/notifications/services/notification/notification.service.ts (2)
CreateNotificationInput(74-76)singleton(13-67)apps/api/src/core/services/job-queue/job-queue.service.ts (4)
Job(133-137)JOB_NAME(139-139)singleton(9-131)JobHandler(146-149)apps/api/src/app/services/trial-started/trial-started.handler.ts (1)
singleton(13-94)
apps/api/src/core/services/job-queue/job-queue.service.ts (4)
apps/api/src/app/services/trial-started/trial-started.handler.ts (1)
singleton(13-94)apps/api/src/core/services/domain-events/domain-events.service.ts (2)
singleton(10-17)JOB_NAME(5-5)apps/api/src/notifications/services/notification-handler/notification.handler.ts (1)
singleton(37-78)packages/logging/src/servicies/logger/logger.service.ts (1)
error(107-109)
apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts (5)
apps/api/src/billing/services/managed-user-wallet/managed-user-wallet.service.ts (1)
createAndAuthorizeTrialSpending(44-57)apps/api/src/core/services/feature-flags/feature-flags.ts (1)
FeatureFlags(1-5)apps/api/test/seeders/user-wallet.seeder.ts (1)
UserWalletSeeder(6-29)apps/api/test/seeders/chain-wallet.seeder.ts (1)
createChainWallet(4-12)apps/api/src/billing/events/trial-started.ts (1)
TrialStarted(4-14)
apps/api/src/notifications/services/notification-templates/before-trial-ends-notification.ts (1)
apps/api/src/notifications/services/notification/notification.service.ts (1)
CreateNotificationInput(74-76)
apps/api/src/app/services/trial-started/trial-started.handler.ts (6)
apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.ts (1)
singleton(11-64)apps/api/src/core/services/job-queue/job-queue.service.ts (2)
singleton(9-131)JobHandler(146-149)apps/api/src/notifications/services/notification-handler/notification.handler.ts (2)
singleton(37-78)NotificationJob(23-35)apps/api/src/notifications/services/notification/notification.service.ts (1)
singleton(13-67)apps/api/src/billing/events/trial-started.ts (1)
TrialStarted(4-14)apps/api/src/notifications/services/notification-templates/start-trial-notification.ts (1)
startTrialNotification(4-16)
⏰ 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: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
🔇 Additional comments (5)
apps/api/src/billing/repositories/user-wallet/user-wallet.repository.ts (1)
49-72: Solid, race-safe getOrCreate (no unintended writes).Good use of onConflictDoNothing + returning to detect creation and fall back to a read on conflict. Ability check is performed prior to insert, and the final read is ability-scoped via whereAccessibleBy. This is the right shape for concurrency without mutating existing rows.
apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts (2)
24-48: LGTM: getOrCreate wallet flow is exercised end-to-end.Mocks and expectations align with the new repository API. The assertions on update payload (address and limits) match ManagedUserWalletService semantics.
68-86: LGTM: error path cleans up and avoids emitting events.Test asserts delete-on-failure and that no TrialStarted is published. Behavior is crisp and side-effect safe.
apps/api/src/app.ts (1)
2-2: LGTM! Clean addition of job provider import and app initializer pattern.The integration of the jobs provider and the new initialization pattern with
APP_INITIALIZERandON_APP_STARTis well-structured. This properly decouples the initialization logic and makes the startup flow more modular.Also applies to: 204-204
apps/api/src/app/services/trial-started/trial-started.handler.ts (1)
41-41:trialfield is correctly defined and populated in UserOutput
- The
UserOutputtype exported inapps/api/src/user/repositories/user/user.repository.tsincludes an optionaltrial?: booleanproperty (lines 11–13).- The repository’s
findUserWithWalletmethod always setstrialfromuserWallets?.isTrialing(defaulting totrueif unavailable), ensuringuser.trialis defined at runtime.- Consequently, the guard invocation
withif (payload.conditions && !guard<UserOutput>(payload.conditions)(user)) { … }conditions: { trial: true }will correctly filter for trialing users as intended.No changes are required here.
stalniy
force-pushed
the
feat/trial-events
branch
from
be4adb0 to
6209e45
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
apps/api/src/billing/repositories/user-wallet/user-wallet.repository.ts (1)
49-72: LGTM! Race-safe implementation with proper conflict handling.The implementation correctly uses
onConflictDoNothingto avoid unnecessary updates and properly handles the race condition by fetching the existing wallet when no row is inserted. The approach is more efficient than the previous suggestions that would have modified existing rows.apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.ts (1)
46-48: Good fix - using the correct userId for the TrialStarted event.The event is now correctly published with the
userIdparameter (the wallet owner) instead ofcurrentUser.id, which resolves the issue from previous reviews where admin actions would send notifications to the wrong user.apps/api/src/app/services/trial-started/trial-started.handler.ts (1)
41-92: Notification conditions may prevent delivery of "trialEnded" and "afterTrialEnds" notifications.The handler schedules all notifications with
conditions: { trial: true }. This is problematic because:
- The
NotificationHandlerevaluates these conditions againstUserOutput, which doesn't have atrialfield (it hasisTrialing).- Even if the field existed, "trialEnded" and "afterTrialEnds" notifications should be sent when the trial has ended (i.e.,
trial: false), not when it's still active.This will likely cause these notifications to never be sent.
Apply this fix to remove conditions for post-trial notifications:
await Promise.all([ this.jobQueueManager.enqueue( new NotificationJob({ template: "beforeTrialEnds", userId: user.id, vars, conditions: notificationConditions }), { singletonKey: `beforeTrialEnds.${user.id}.${TRIAL_ALLOWANCE_EXPIRATION_DAYS - 7}`, startAfter: addDays(new Date(), TRIAL_ALLOWANCE_EXPIRATION_DAYS - 7) } ), this.jobQueueManager.enqueue( new NotificationJob({ template: "beforeTrialEnds", userId: user.id, vars, conditions: notificationConditions }), { singletonKey: `beforeTrialEnds.${user.id}.${TRIAL_ALLOWANCE_EXPIRATION_DAYS - 1}`, startAfter: addDays(new Date(), TRIAL_ALLOWANCE_EXPIRATION_DAYS - 1) } ), this.jobQueueManager.enqueue( new NotificationJob({ template: "trialEnded", userId: user.id, - conditions: notificationConditions }), { singletonKey: `trialEnded.${user.id}`, startAfter: addDays(new Date(), TRIAL_ALLOWANCE_EXPIRATION_DAYS) } ), this.jobQueueManager.enqueue( new NotificationJob({ template: "afterTrialEnds", userId: user.id, - conditions: notificationConditions }), { singletonKey: `afterTrialEnds.${user.id}`, startAfter: addDays(new Date(), TRIAL_ALLOWANCE_EXPIRATION_DAYS + 7) } ) ]);
🧹 Nitpick comments (2)
apps/api/src/app.ts (2)
2-2: Side‑effect provider import: OK, but consider centralizing provider bootstrapsImporting "./app/providers/jobs.provider" for its side effects is fine for DI registration. To reduce ordering pitfalls and make tree‑shaking/bundling safer, consider a single providers/index.ts that aggregates all provider registrations and import that here instead. This keeps bootstrap wiring discoverable in one place.
203-206: Potential race: DB init runs in parallel with ON_APP_START initializers (e.g., job queue start)If any initializer touches the DB (jobs.provider starts the job queue; handlers may hit app DB), running them alongside initDb() can race against connection/authenticate/schema sync. Safer to complete initDb() first, then run initializers. Also add a small guard so we only call callable ON_APP_START hooks.
Apply this diff:
try { - await Promise.all([initDb(), ...container.resolveAll(APP_INITIALIZER).map(initializer => initializer[ON_APP_START]())]); + const initializers = container.resolveAll(APP_INITIALIZER); + await initDb(); + await Promise.all( + initializers + .map(i => i[ON_APP_START]) + .filter((fn): fn is () => Promise<void> => typeof fn === "function") + .map(fn => fn()) + ); startScheduler();Alternatively, if you want best‑effort startup diagnostics without losing parallelism across initializers, use Promise.allSettled and throw an AggregateError if any failed (optional).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (26)
apps/api/package.json(2 hunks)apps/api/src/app.ts(3 hunks)apps/api/src/app/providers/jobs.provider.ts(1 hunks)apps/api/src/app/services/trial-started/trial-started.handler.spec.ts(1 hunks)apps/api/src/app/services/trial-started/trial-started.handler.ts(1 hunks)apps/api/src/billing/controllers/wallet/wallet.controller.ts(0 hunks)apps/api/src/billing/events/trial-started.ts(1 hunks)apps/api/src/billing/repositories/user-wallet/user-wallet.repository.ts(1 hunks)apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts(6 hunks)apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.ts(3 hunks)apps/api/src/console.ts(4 hunks)apps/api/src/core/providers/app-initializer.ts(1 hunks)apps/api/src/core/providers/postgres.provider.ts(2 hunks)apps/api/src/core/services/domain-events/domain-events.service.ts(1 hunks)apps/api/src/core/services/feature-flags/feature-flags.service.ts(2 hunks)apps/api/src/core/services/job-queue/job-queue.service.spec.ts(1 hunks)apps/api/src/core/services/job-queue/job-queue.service.ts(1 hunks)apps/api/src/notifications/services/notification-handler/notification.handler.spec.ts(1 hunks)apps/api/src/notifications/services/notification-handler/notification.handler.ts(1 hunks)apps/api/src/notifications/services/notification-templates/after-trial-ends-notification.ts(1 hunks)apps/api/src/notifications/services/notification-templates/before-trial-ends-notification.ts(1 hunks)apps/api/src/notifications/services/notification-templates/trial-ended-notification.ts(1 hunks)apps/api/src/notifications/services/notification/notification.service.spec.ts(2 hunks)apps/api/src/notifications/services/notification/notification.service.ts(2 hunks)apps/api/test/seeders/user.seeder.ts(2 hunks)apps/api/test/services/wallet-testing.service.ts(2 hunks)
💤 Files with no reviewable changes (1)
- apps/api/src/billing/controllers/wallet/wallet.controller.ts
🚧 Files skipped from review as they are similar to previous changes (19)
- apps/api/src/notifications/services/notification-templates/after-trial-ends-notification.ts
- apps/api/src/core/services/domain-events/domain-events.service.ts
- apps/api/src/console.ts
- apps/api/src/notifications/services/notification-templates/trial-ended-notification.ts
- apps/api/package.json
- apps/api/src/app/services/trial-started/trial-started.handler.spec.ts
- apps/api/src/core/providers/app-initializer.ts
- apps/api/src/app/providers/jobs.provider.ts
- apps/api/src/notifications/services/notification/notification.service.ts
- apps/api/src/notifications/services/notification-handler/notification.handler.spec.ts
- apps/api/src/core/services/feature-flags/feature-flags.service.ts
- apps/api/src/core/services/job-queue/job-queue.service.spec.ts
- apps/api/src/core/providers/postgres.provider.ts
- apps/api/src/notifications/services/notification-templates/before-trial-ends-notification.ts
- apps/api/test/services/wallet-testing.service.ts
- apps/api/src/notifications/services/notification/notification.service.spec.ts
- apps/api/src/notifications/services/notification-handler/notification.handler.ts
- apps/api/test/seeders/user.seeder.ts
- apps/api/src/core/services/job-queue/job-queue.service.ts
🧰 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/api/src/billing/repositories/user-wallet/user-wallet.repository.tsapps/api/src/app.tsapps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.tsapps/api/src/billing/events/trial-started.tsapps/api/src/app/services/trial-started/trial-started.handler.tsapps/api/src/billing/services/wallet-initializer/wallet-initializer.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/billing/repositories/user-wallet/user-wallet.repository.tsapps/api/src/app.tsapps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.tsapps/api/src/billing/events/trial-started.tsapps/api/src/app/services/trial-started/trial-started.handler.tsapps/api/src/billing/services/wallet-initializer/wallet-initializer.service.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/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts
🧬 Code graph analysis (5)
apps/api/src/app.ts (3)
apps/api/src/core/providers/app-initializer.ts (2)
APP_INITIALIZER(3-3)ON_APP_START(4-4)apps/api/src/app/providers/jobs.provider.ts (1)
ON_APP_START(11-15)apps/api/src/core/services/feature-flags/feature-flags.service.ts (1)
ON_APP_START(80-82)
apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts (5)
apps/api/src/billing/services/managed-user-wallet/managed-user-wallet.service.ts (1)
createAndAuthorizeTrialSpending(44-57)apps/api/src/core/services/feature-flags/feature-flags.ts (1)
FeatureFlags(1-5)apps/api/test/seeders/user-wallet.seeder.ts (1)
UserWalletSeeder(6-29)apps/api/test/seeders/chain-wallet.seeder.ts (1)
createChainWallet(4-12)apps/api/src/billing/events/trial-started.ts (1)
TrialStarted(4-14)
apps/api/src/billing/events/trial-started.ts (1)
apps/api/src/core/services/domain-events/domain-events.service.ts (1)
DomainEvent(6-8)
apps/api/src/app/services/trial-started/trial-started.handler.ts (7)
apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.ts (1)
singleton(11-64)apps/api/src/core/services/domain-events/domain-events.service.ts (1)
singleton(10-17)apps/api/src/core/services/job-queue/job-queue.service.ts (2)
singleton(9-131)JobHandler(146-149)apps/api/src/notifications/services/notification-handler/notification.handler.ts (2)
singleton(37-78)NotificationJob(23-35)apps/api/src/notifications/services/notification/notification.service.ts (1)
singleton(13-67)apps/api/src/billing/events/trial-started.ts (1)
TrialStarted(4-14)apps/api/src/notifications/services/notification-templates/start-trial-notification.ts (1)
startTrialNotification(4-16)
apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.ts (3)
apps/api/src/billing/repositories/user-wallet/user-wallet.repository.ts (1)
UserWalletPublicOutput(27-33)apps/api/src/core/services/feature-flags/feature-flags.ts (1)
FeatureFlags(1-5)apps/api/src/billing/events/trial-started.ts (1)
TrialStarted(4-14)
⏰ 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: 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: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
🔇 Additional comments (2)
apps/api/src/billing/events/trial-started.ts (1)
1-14: LGTM! Well-structured domain event implementation.The
TrialStarteddomain event follows the established pattern correctly, with proper type references and versioning.apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts (1)
88-106: LGTM! Comprehensive test coverage for domain event publishing.The test correctly verifies that the
TrialStartedevent is published with the proper userId when theANONYMOUS_FREE_TRIALfeature flag is disabled, and confirms that domain events are not published on error scenarios.
apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.ts
Show resolved
Hide resolved
stalniy
force-pushed
the
feat/trial-events
branch
from
6209e45 to
8330263
Compare
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 (1)
apps/api/src/notifications/services/notification/notification.service.ts (1)
26-27: Replaceas anyon request headers with precise types from the OpenAPI typings.Casting to
anyviolates the repo guideline and hides shape mismatches. Use the generatedoperations[...]types for the parameters header.Apply this diff:
import { NOTIFICATIONS_API_CLIENT, NotificationsApiClient, operations } from "../../providers/notifications-api.provider"; const DEFAULT_BACKOFF_OPTIONS: BackoffOptions = { @@ async createNotification(input: CreateNotificationInput): Promise<void> { const { user, ...notification } = input; await backOff(async () => { const result = await this.notificationsApi.v1 .createNotification({ parameters: { header: { - "x-user-id": user.id - } - } as any, + "x-user-id": user.id + } as operations["createNotification"]["parameters"]["header"] + }, body: notification }) .catch(error => ({ error, response: null }));
♻️ Duplicate comments (4)
apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.ts (1)
21-24: ValidateuserIdinput early to avoid repository calls with invalid IDs.Empty or whitespace-only IDs should short-circuit with a clear error.
- async initializeAndGrantTrialLimits(userId: string): Promise<UserWalletPublicOutput> { - const { wallet, isNew } = await this.userWalletRepository.accessibleBy(this.authService.ability, "create").getOrCreate({ userId }); + async initializeAndGrantTrialLimits(userId: string): Promise<UserWalletPublicOutput> { + if (!userId || userId.trim() === "") { + throw new Error("userId is required and cannot be empty"); + } + const { wallet, isNew } = await this.userWalletRepository.accessibleBy(this.authService.ability, "create").getOrCreate({ userId });apps/api/src/core/services/job-queue/job-queue.service.ts (3)
86-91: Repeat ofcreateQueueoptions issue insidedrain.Same fix as above.
- await this.pgBoss.createQueue(queueName, { - name: queueName, - retryLimit: 10 - }); + await this.pgBoss.createQueue(queueName);
17-18: tsyringe:@injectoption is invalid (isOptional). Useoptional.As written, this won’t type-check and the dependency won’t be optional at runtime.
Apply this diff:
- @inject(PG_BOSS_TOKEN, { isOptional: true }) pgBoss?: PgBoss + @inject(PG_BOSS_TOKEN, { optional: true }) pgBoss?: PgBoss
70-81: Enqueue usesjob.nameinstead of the class static queue name; retries lack defaults; logs may leak PII.
- Target the queue via the class’ static
[JOB_NAME](aligns withregisterHandlers).- Set a sane default
retryLimit(10) if not provided.- Avoid logging full job payloads.
Apply this diff:
- async enqueue(job: Job, options?: EnqueueOptions): Promise<string | null> { - this.logger.info({ - event: "JOB_ENQUEUED", - job - }); - - return await this.pgBoss.send({ - name: job.name, - data: { ...job.data, version: job.version }, - options - }); - } + async enqueue(job: Job, options?: EnqueueOptions): Promise<string | null> { + const queueName = (job.constructor as JobType<Job>)[JOB_NAME] ?? job.name; + const sendOptions: EnqueueOptions = { retryLimit: 10, ...options }; + + this.logger.info({ + event: "JOB_ENQUEUED", + queue: queueName, + version: job.version + }); + + return await this.pgBoss.send({ + name: queueName, + data: { ...job.data, version: job.version }, + options: sendOptions + }); + }
🧹 Nitpick comments (11)
apps/api/src/notifications/services/notification/notification.service.ts (2)
41-66: NarrowcreateDefaultChannel’s parameter type to require email (and remove non-null assertion).This method is only called after an email guard; reflect that at the type level and avoid
user.email!.Apply this diff:
- async createDefaultChannel(user: UserInput): Promise<void> { + async createDefaultChannel(user: UserInput & { email: string }): Promise<void> { await backOff(async () => { const result = await this.notificationsApi.v1 .createDefaultChannel({ parameters: { header: { - "x-user-id": user.id - } as any + "x-user-id": user.id + } as operations["createDefaultChannel"]["parameters"]["header"] }, body: { data: { name: "Default", type: "email", config: { - addresses: [user.email!] + addresses: [user.email] } } } }) .catch(error => ({ error, response: null }));
33-38: Consider surfacing upstream error code/message in the thrown Error.Wrapping with a generic message loses debuggability. Including
result.error.codeorstatusCodein the message aids tracing retries/failures.Example:
- throw new Error("Failed to create notification", { cause: result.error }); + throw new Error( + `Failed to create notification: ${"code" in result.error ? result.error.code : "UNKNOWN"}`, + { cause: result.error } + );apps/api/src/notifications/services/notification/notification.service.spec.ts (4)
43-44: Remove duplicatejest.useFakeTimers()invocation.Called twice within the same test; one is sufficient.
it("retries if notification service returns an error", async () => { jest.useFakeTimers(); const { service, api } = setup(); - jest.useFakeTimers(); const user = { id: "user-1", email: "user@example.com" };
128-131: Rename test to reflect new behavior (promise rejection), not logging.The service no longer logs; it rejects with
cause. Update the test name for clarity.- it("does not create default channel when user has no email and logs an error", async () => { + it("does not create default channel when user has no email and rejects with the original error", async () => { jest.useFakeTimers(); const { service, api } = setup();
149-152: Rename test to reflect retry exhaustion leading to rejection (not logging).- it("fails after 10 attempts if notification service is not available and logs an error", async () => { + it("fails after 10 attempts if notification service is not available and rejects", async () => { jest.useFakeTimers(); const { service, api } = setup();
170-175: Optional: allowsetupto accept overrides for flexibility.This enables test-specific API behavior without local re-mocking.
- function setup() { - const api = mockDeep<NotificationsApiClient>(); - const service = new NotificationService(api); - return { service, api }; - } + function setup(overrides: { api?: NotificationsApiClient } = {}) { + const api = overrides.api ?? mockDeep<NotificationsApiClient>(); + const service = new NotificationService(api); + return { service, api }; + }apps/api/src/core/services/domain-events/domain-events.service.ts (1)
5-8: DomainEvent re-declaresversionalready present inJob.It’s redundant and can confuse readers. Keep DomainEvent as an alias (or empty extension).
-export interface DomainEvent extends Job { - version: number; -} +export type DomainEvent = Job;apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.ts (1)
46-51: Decide whether event-publish failure should fail wallet initialization.If queueing the event is non-critical, consider catching and logging (or background retry) instead of throwing and failing the user flow.
Do you want me to prepare a patch that wraps
domainEvents.publish(...)in a best-effort try/catch with a warning log so wallet creation succeeds even if the queue is temporarily unavailable?apps/api/src/core/services/job-queue/job-queue.service.ts (3)
27-35:createQueueoptions are incorrect;retryLimithere is a no-op.PgBoss applies retry policy at send/publish time, not queue creation. Passing
{ name, retryLimit }is misleading.Apply this diff (here and below in drain):
- await this.pgBoss.createQueue(queueName, { - name: queueName, - retryLimit: 10 - }); + await this.pgBoss.createQueue(queueName);
93-111: Sanitize worker logs to avoid PII and oversized payloads.Do not log full job objects; prefer minimal metadata.
Apply this diff:
- this.logger.info({ - event: "JOBS_STARTED", - jobsIds: jobs.map(job => job.id) - }); + this.logger.info({ + event: "JOBS_STARTED", + queue: queueName, + count: jobs.length + }); @@ - if (result.status === "fulfilled") { - this.logger.info({ - event: "JOB_DONE", - jobId: jobs[index].id - }); - } else { - this.logger.error({ - event: "JOB_FAILED", - jobId: jobs[index].id, - error: result.reason - }); - } + if (result.status === "fulfilled") { + this.logger.info({ event: "JOB_DONE", queue: queueName, id: jobs[index].id }); + } else { + this.logger.error({ event: "JOB_FAILED", queue: queueName, id: jobs[index].id, error: result.reason }); + }
141-144: Avoidanyin public types (repo guideline).Change the constructor arg list to
unknown[](or the job’s data shape) to comply.export type JobType<T extends Job> = { - new (...args: any[]): T; + new (...args: readonly unknown[]): T; [JOB_NAME]: string; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (26)
apps/api/package.json(2 hunks)apps/api/src/app.ts(3 hunks)apps/api/src/app/providers/jobs.provider.ts(1 hunks)apps/api/src/app/services/trial-started/trial-started.handler.spec.ts(1 hunks)apps/api/src/app/services/trial-started/trial-started.handler.ts(1 hunks)apps/api/src/billing/controllers/wallet/wallet.controller.ts(0 hunks)apps/api/src/billing/events/trial-started.ts(1 hunks)apps/api/src/billing/repositories/user-wallet/user-wallet.repository.ts(1 hunks)apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts(6 hunks)apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.ts(3 hunks)apps/api/src/console.ts(4 hunks)apps/api/src/core/providers/app-initializer.ts(1 hunks)apps/api/src/core/providers/postgres.provider.ts(2 hunks)apps/api/src/core/services/domain-events/domain-events.service.ts(1 hunks)apps/api/src/core/services/feature-flags/feature-flags.service.ts(2 hunks)apps/api/src/core/services/job-queue/job-queue.service.spec.ts(1 hunks)apps/api/src/core/services/job-queue/job-queue.service.ts(1 hunks)apps/api/src/notifications/services/notification-handler/notification.handler.spec.ts(1 hunks)apps/api/src/notifications/services/notification-handler/notification.handler.ts(1 hunks)apps/api/src/notifications/services/notification-templates/after-trial-ends-notification.ts(1 hunks)apps/api/src/notifications/services/notification-templates/before-trial-ends-notification.ts(1 hunks)apps/api/src/notifications/services/notification-templates/trial-ended-notification.ts(1 hunks)apps/api/src/notifications/services/notification/notification.service.spec.ts(2 hunks)apps/api/src/notifications/services/notification/notification.service.ts(2 hunks)apps/api/test/seeders/user.seeder.ts(2 hunks)apps/api/test/services/wallet-testing.service.ts(2 hunks)
💤 Files with no reviewable changes (1)
- apps/api/src/billing/controllers/wallet/wallet.controller.ts
🚧 Files skipped from review as they are similar to previous changes (20)
- apps/api/src/notifications/services/notification-templates/before-trial-ends-notification.ts
- apps/api/src/core/providers/postgres.provider.ts
- apps/api/src/billing/events/trial-started.ts
- apps/api/src/notifications/services/notification-templates/after-trial-ends-notification.ts
- apps/api/src/app/services/trial-started/trial-started.handler.spec.ts
- apps/api/src/core/providers/app-initializer.ts
- apps/api/src/billing/repositories/user-wallet/user-wallet.repository.ts
- apps/api/test/services/wallet-testing.service.ts
- apps/api/src/app/services/trial-started/trial-started.handler.ts
- apps/api/src/core/services/job-queue/job-queue.service.spec.ts
- apps/api/src/console.ts
- apps/api/src/notifications/services/notification-handler/notification.handler.spec.ts
- apps/api/src/app.ts
- apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts
- apps/api/src/notifications/services/notification-handler/notification.handler.ts
- apps/api/package.json
- apps/api/src/core/services/feature-flags/feature-flags.service.ts
- apps/api/src/app/providers/jobs.provider.ts
- apps/api/test/seeders/user.seeder.ts
- apps/api/src/notifications/services/notification-templates/trial-ended-notification.ts
🧰 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/api/src/core/services/domain-events/domain-events.service.tsapps/api/src/notifications/services/notification/notification.service.spec.tsapps/api/src/notifications/services/notification/notification.service.tsapps/api/src/billing/services/wallet-initializer/wallet-initializer.service.tsapps/api/src/core/services/job-queue/job-queue.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/domain-events/domain-events.service.tsapps/api/src/notifications/services/notification/notification.service.spec.tsapps/api/src/notifications/services/notification/notification.service.tsapps/api/src/billing/services/wallet-initializer/wallet-initializer.service.tsapps/api/src/core/services/job-queue/job-queue.service.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/api/src/notifications/services/notification/notification.service.spec.ts
🧠 Learnings (2)
📚 Learning: 2025-07-21T08:24:24.269Z
Learnt from: CR
PR: akash-network/console#0
File: .cursor/rules/no-jest-mock.mdc:0-0
Timestamp: 2025-07-21T08:24:24.269Z
Learning: Applies to **/*.spec.{ts,tsx} : Don't use `jest.mock()` to mock dependencies in test files. Instead, use `jest-mock-extended` to create mocks and pass mocks as dependencies to the service under test.
Applied to files:
apps/api/src/notifications/services/notification/notification.service.spec.ts
📚 Learning: 2025-07-27T12:16:08.566Z
Learnt from: CR
PR: akash-network/console#0
File: .cursor/rules/general.mdc:0-0
Timestamp: 2025-07-27T12:16:08.566Z
Learning: Applies to **/*.{ts,tsx} : Never use type any or cast to type any. Always define the proper TypeScript types.
Applied to files:
apps/api/src/core/services/job-queue/job-queue.service.ts
🧬 Code graph analysis (5)
apps/api/src/core/services/domain-events/domain-events.service.ts (2)
apps/api/src/core/services/job-queue/job-queue.service.ts (2)
Job(133-137)singleton(9-131)apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.ts (1)
singleton(11-64)
apps/api/src/notifications/services/notification/notification.service.spec.ts (2)
apps/api/src/notifications/services/notification/notification.service.ts (1)
CreateNotificationInput(74-76)apps/api/src/notifications/providers/notifications-api.provider.ts (1)
NotificationsApiClient(9-9)
apps/api/src/notifications/services/notification/notification.service.ts (1)
apps/api/src/notifications/providers/notifications-api.provider.ts (2)
NOTIFICATIONS_API_CLIENT(10-10)NotificationsApiClient(9-9)
apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.ts (3)
apps/api/src/billing/repositories/user-wallet/user-wallet.repository.ts (1)
UserWalletPublicOutput(27-33)apps/api/src/core/services/feature-flags/feature-flags.ts (1)
FeatureFlags(1-5)apps/api/src/billing/events/trial-started.ts (1)
TrialStarted(4-14)
apps/api/src/core/services/job-queue/job-queue.service.ts (3)
apps/api/src/app/services/trial-started/trial-started.handler.ts (1)
singleton(13-94)apps/api/src/core/services/domain-events/domain-events.service.ts (1)
singleton(10-17)apps/api/src/notifications/services/notification-handler/notification.handler.ts (1)
singleton(37-78)
⏰ 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: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
- 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
🔇 Additional comments (4)
apps/api/src/notifications/services/notification/notification.service.ts (1)
15-15: Constructor simplification looks good.Dropping LoggerService tightens the service’s responsibilities and the DI signature is cleaner.
apps/api/src/core/services/domain-events/domain-events.service.ts (1)
14-16: Publish delegation is straightforward. LGTM.apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.ts (2)
39-42: Good cleanup on failure path.Deleting the newly created wallet on authorization failure prevents dangling records.
46-48: Correct subject for domain event.Publishing TrialStarted with the method’s
userIdfixes prior “wrong user” risk. Nicely done.
stalniy
force-pushed
the
feat/trial-events
branch
from
8330263 to
bec73de
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts (1)
154-154: Do not mutate the global TSyringe container in tests.
container.clearInstances()alters shared global state and can cause cross-test flakiness in parallel runs. Clear only the child container or omit clearing entirely.Apply one of the following diffs (preferred first):
Option A — clear only the child container:
- container.clearInstances(); + di.clearInstances();Option B — remove clearing (child container is short-lived in each test):
- container.clearInstances(); + // no-opapps/api/src/core/services/feature-flags/feature-flags.service.ts (2)
21-26: Fix DI: tsyringe @Inject usage is invalid and will fail resolution; make the factory truly optional.
injectfrom tsyringe does not accept an options object. As written, container resolution will throw if no provider is registered for "createUnleashClient" and the default parameter will never be used. Use@optional()and fall back in code.Apply this diff:
-import { Disposable, inject, registry, singleton } from "tsyringe"; +import { Disposable, inject, optional, registry, singleton } from "tsyringe"; @@ - @inject("createUnleashClient", { isOptional: true }) createClient = createUnleashClient + @inject("createUnleashClient") @optional() createClient?: typeof createUnleashClient ) { @@ - this.createClient = createClient; + this.createClient = createClient ?? createUnleashClient;Also applies to: 2-2
60-73: Harden initialize(): avoid indefinite startup hang; start client explicitly; clean up on failure.Waiting only on
"synchronized"without a timeout can stall app startup forever if Unleash is unreachable. Also,new Unleash(config)may not start fetching untilstart()(SDK-version dependent). Start the client, race with a timeout, and destroy on error to avoid orphaned timers.Apply this diff:
assert(url && token, "UNLEASH_SERVER_API_URL and UNLEASH_SERVER_API_TOKEN are required"); - const client = this.createClient({ + const client = this.createClient({ url, appName: this.configService.get("UNLEASH_APP_NAME"), customHeaders: { Authorization: token } }); - await new Promise((resolve, reject) => { - client.once("synchronized", resolve); - client.once("error", reject); - }); + // Explicitly start (no-op on SDKs that auto-start) + (client as any).start?.(); + + const timeoutMs = 10_000; // keep startup snappy; make configurable later if needed + try { + await Promise.race([ + new Promise<void>((resolve, reject) => { + client.once("ready", resolve); // initial bootstrap fetched + client.once("synchronized", resolve); // subsequent syncs (covers both cases) + client.once("error", reject); + }), + new Promise((_, reject) => + setTimeout(() => reject(new Error(`Unleash failed to synchronize within ${timeoutMs}ms`)), timeoutMs) + ) + ]); + } catch (err) { + // Ensure we don't leak background timers on init failure + try { (client as any).destroy?.(); } catch { /* ignore */ } + client.removeAllListeners(); + throw err; + } this.client = client;
♻️ Duplicate comments (5)
apps/api/src/core/services/job-queue/job-queue.service.ts (5)
30-33: Remove unsupported retryLimit from createQueue (no-op where it is)Apply:
- await this.pgBoss.createQueue(queueName, { - name: queueName, - retryLimit: 10 - }); + await this.pgBoss.createQueue(queueName);
88-91: Ditto: remove retryLimit from createQueue in startWorkersApply:
- await this.pgBoss.createQueue(queueName, { - name: queueName, - retryLimit: 10 - }); + await this.pgBoss.createQueue(queueName);
17-25: Invalid tsyringe optional injection — replace with injectAll (or container check)@Inject doesn’t accept an options object; { isOptional: true } will not compile and won’t make the dependency optional.
Apply:
-import { inject, InjectionToken, singleton } from "tsyringe"; +import { injectAll, InjectionToken, singleton } from "tsyringe"; @@ - constructor( - private readonly logger: LoggerService, - private readonly coreConfig: CoreConfigService, - @inject(PG_BOSS_TOKEN, { isOptional: true }) pgBoss?: PgBoss - ) { - this.pgBoss = - pgBoss ?? - new PgBoss({ - connectionString: this.coreConfig.get("POSTGRES_DB_URI"), - schema: "pgboss" - }); - } + constructor( + private readonly logger: LoggerService, + private readonly coreConfig: CoreConfigService, + @injectAll(PG_BOSS_TOKEN) pgBosses: PgBoss[] = [] + ) { + const injected = pgBosses[0]; + this.pgBoss = + injected ?? + new PgBoss({ + connectionString: this.coreConfig.get("POSTGRES_DB_URI"), + schema: "pgboss" + }); + }Run to ensure there are no remaining invalid usages:
#!/bin/bash rg -nP '@inject\s*\(\s*PG_BOSS_TOKEN\b.*\)'
70-81: Enqueue to the correct queue and avoid logging the full job; also set a default retryLimitApply:
async enqueue(job: Job, options?: EnqueueOptions): Promise<string | null> { - this.logger.info({ - event: "JOB_ENQUEUED", - job - }); + const name = (job.constructor as JobType<Job>)[JOB_NAME] ?? job.name; + this.logger.info({ event: "JOB_ENQUEUED", queue: name, version: job.version }); - return await this.pgBoss.send({ - name: job.name, - data: { ...job.data, version: job.version }, - options - }); + const optionsWithDefaults: EnqueueOptions = { retryLimit: 10, ...options }; + return await this.pgBoss.send({ + name, + data: { ...job.data, version: job.version }, + options: optionsWithDefaults + }); }#!/bin/bash # Expect no remaining pgBoss.send calls that reference job.name rg -nP 'pgBoss\.send\(\s*\{\s*name:\s*job\.name' -C2
141-144: Avoid any[] in public type (repo guideline)Apply:
export type JobType<T extends Job> = { - new (...args: any[]): T; + new (...args: readonly unknown[]): T; [JOB_NAME]: string; };
🧹 Nitpick comments (12)
apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts (5)
29-33: Add a negative assertion: no domain event when free-trial flag is enabled.Lock in behavior that publishing is skipped when ANONYMOUS_FREE_TRIAL is enabled and the flow succeeds.
Apply this diff at the end of the first test:
); }); + expect(di.resolve(DomainEventsService).publish).not.toHaveBeenCalled(); });Also applies to: 37-48
50-50: Fix test title grammar.Prefer “does not authorize” over “does not authorizes”.
Apply this diff:
- it("does not authorizes trial spending for existing wallet", async () => { + it("does not authorize trial spending for existing wallet", async () => {
88-106: Event publication path looks correct; consider one more coverage case.Publishing TrialStarted when ANONYMOUS_FREE_TRIAL is disabled is covered. Consider adding the counterpart: when the wallet already exists and the flag is disabled, no event should be published.
Suggested additional test:
+ it('does not publish "TrialStarted" when ANONYMOUS_FREE_TRIAL is disabled and wallet exists', async () => { + const userId = "test-user-id"; + const existingWallet = UserWalletSeeder.create({ userId }); + const getOrCreateWallet = jest.fn().mockResolvedValue({ wallet: existingWallet, isNew: false }); + const di = setup({ + userId, + getOrCreateWallet, + enabledFeatures: [] + }); + + await di.resolve(WalletInitializerService).initializeAndGrantTrialLimits(userId); + expect(di.resolve(DomainEventsService).publish).not.toHaveBeenCalled(); + });
124-138: Avoid unsafe double casts; prefer a typed repo mock with proper chaining.The current approach uses
as unknown as UserWalletRepositoryand a hand-rolledaccessibleBy()that returnsthis. Use jest-mock-extended’s typed mocks andmockReturnThis()-style chaining to eliminate casts and improve readability.Apply this diff to replace the repository registration:
- di.registerInstance( - UserWalletRepository, - mock<UserWalletRepository>({ - getOrCreate: input?.getOrCreateWallet, - updateById: input?.updateWalletById, - deleteById: input?.deleteWalletById ?? jest.fn(), - accessibleBy() { - return this as unknown as UserWalletRepository; - }, - toPublic: value => ({ - ...value, - isTrialing: !!value.isTrialing - }) - }) as unknown as UserWalletRepository - ); + const repo = mock<UserWalletRepository>(); + if (input?.getOrCreateWallet) repo.getOrCreate.mockImplementation(input.getOrCreateWallet); + if (input?.updateWalletById) repo.updateById.mockImplementation(input.updateWalletById); + repo.deleteById.mockImplementation(input?.deleteWalletById ?? jest.fn()); + repo.accessibleBy.mockReturnValue(repo); + repo.toPublic.mockImplementation(value => ({ + ...value, + isTrialing: !!value.isTrialing + })); + di.registerInstance(UserWalletRepository, repo);
156-156: Return the SUT from setup to follow the testing guideline.The guideline says the setup function creates and returns the object under test. Returning the SUT improves clarity and reduces DI leakage into tests.
Apply this diff and then update call sites accordingly:
- return di; + return { + di, + sut: di.resolve(WalletInitializerService) + };Example usage in tests:
- const di = setup({ /* ... */ }); - await di.resolve(WalletInitializerService).initializeAndGrantTrialLimits(userId); + const { sut, di } = setup({ /* ... */ }); + await sut.initializeAndGrantTrialLimits(userId);apps/api/src/core/services/feature-flags/feature-flags.service.ts (3)
75-78: Dispose does not await flush; prefer deterministic shutdown or explicit best-effort.
destroyWithFlush()returns a Promise;Disposable.dispose()is sync, so the flush is fire-and-forget. Either switch todestroy()(deterministic, no Promise) or make the best-effort explicit and clear the reference.Apply this diff:
- dispose(): void { - this.client?.destroyWithFlush(); - this.client?.removeAllListeners(); - } + dispose(): void { + const client = this.client; + this.client = undefined; + if (!client) return; + // Best effort: flush in background; fall back to immediate destroy if unsupported + try { (client as any).destroyWithFlush?.(); } catch { (client as any).destroy?.(); } + client.removeAllListeners(); + }
31-31: Clarify assertion message to reflect auto-initialization path.The service now initializes via
ON_APP_START. The message should guide operators accordingly.Apply this diff:
- assert(this.client, "Feature flags service was not initialized. Call initialize() method first."); + assert(this.client, "FeatureFlagsService is not initialized yet. Ensure ON_APP_START has executed or call initialize() explicitly.");
5-6: Optional: prefer typed tokens over string tokens for DI.Using a string token ("createUnleashClient") is brittle. A symbol-based
InjectionToken<typeof createUnleashClient>improves safety and discoverability.If you keep it in this file, minimally:
-import { APP_INITIALIZER, AppInitializer, ON_APP_START } from "@src/core/providers/app-initializer"; +import { APP_INITIALIZER, AppInitializer, ON_APP_START } from "@src/core/providers/app-initializer"; +import type { InjectionToken } from "tsyringe"; @@ - private readonly createClient: typeof createUnleashClient; + private readonly createClient: typeof createUnleashClient; + // eslint-disable-next-line @typescript-eslint/consistent-type-definitions + static readonly CREATE_UNLEASH_CLIENT: InjectionToken<typeof createUnleashClient> = Symbol("createUnleashClient"); @@ - @inject("createUnleashClient") @optional() createClient?: typeof createUnleashClient + @inject(FeatureFlagsService.CREATE_UNLEASH_CLIENT) @optional() createClient?: typeof createUnleashClientThen register the default elsewhere if needed:
container.register(FeatureFlagsService.CREATE_UNLEASH_CLIENT, { useValue: createUnleashClient });Also applies to: 16-16, 21-21
apps/api/src/notifications/services/notification-templates/before-trial-ends-notification.ts (2)
12-14: Pluralization for 0/1 days"1 days" reads awkwardly and "0 days" could be “today”. Tweak description to handle singular/plural (and optionally “today”).
Apply:
- description: `Your trial of Akash Network is ending in ${daysLeft} days` + description: + daysLeft === 0 + ? "Your trial of Akash Network ends today" + : `Your trial of Akash Network is ending in ${daysLeft} day${daysLeft === 1 ? "" : "s"}`
10-14: Stabilize notificationId across retries to avoid duplicate sendsIf a job is retried hours later, daysLeft may change (e.g., 7 → 6), producing a different notificationId and defeating dedupe based on ID. Consider passing a stable daysLeft in vars (computed at enqueue time) and prefer that over live recomputation for notificationId.
If your Notifications API dedupes by notificationId, confirm whether changing daysLeft between retries results in extra notifications. If so, I can propose a small refactor to accept vars.daysLeft and use that in notificationId while still showing a recomputed value in the message if you prefer.
apps/api/src/core/services/job-queue/job-queue.service.ts (1)
95-95: Nit: jobsIds → jobIdsApply:
- jobsIds: jobs.map(job => job.id) + jobIds: jobs.map(job => job.id)apps/api/src/notifications/services/notification-handler/notification.handler.ts (1)
67-74: Typo in event name: “MISTMATCH” → “MISMATCH”Apply:
- event: "USER_CONDITIONS_MISTMATCH", + event: "USER_CONDITIONS_MISMATCH",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (26)
apps/api/package.json(2 hunks)apps/api/src/app.ts(3 hunks)apps/api/src/app/providers/jobs.provider.ts(1 hunks)apps/api/src/app/services/trial-started/trial-started.handler.spec.ts(1 hunks)apps/api/src/app/services/trial-started/trial-started.handler.ts(1 hunks)apps/api/src/billing/controllers/wallet/wallet.controller.ts(0 hunks)apps/api/src/billing/events/trial-started.ts(1 hunks)apps/api/src/billing/repositories/user-wallet/user-wallet.repository.ts(1 hunks)apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts(6 hunks)apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.ts(3 hunks)apps/api/src/console.ts(4 hunks)apps/api/src/core/providers/app-initializer.ts(1 hunks)apps/api/src/core/providers/postgres.provider.ts(2 hunks)apps/api/src/core/services/domain-events/domain-events.service.ts(1 hunks)apps/api/src/core/services/feature-flags/feature-flags.service.ts(2 hunks)apps/api/src/core/services/job-queue/job-queue.service.spec.ts(1 hunks)apps/api/src/core/services/job-queue/job-queue.service.ts(1 hunks)apps/api/src/notifications/services/notification-handler/notification.handler.spec.ts(1 hunks)apps/api/src/notifications/services/notification-handler/notification.handler.ts(1 hunks)apps/api/src/notifications/services/notification-templates/after-trial-ends-notification.ts(1 hunks)apps/api/src/notifications/services/notification-templates/before-trial-ends-notification.ts(1 hunks)apps/api/src/notifications/services/notification-templates/trial-ended-notification.ts(1 hunks)apps/api/src/notifications/services/notification/notification.service.spec.ts(2 hunks)apps/api/src/notifications/services/notification/notification.service.ts(2 hunks)apps/api/test/seeders/user.seeder.ts(2 hunks)apps/api/test/services/wallet-testing.service.ts(2 hunks)
💤 Files with no reviewable changes (1)
- apps/api/src/billing/controllers/wallet/wallet.controller.ts
🚧 Files skipped from review as they are similar to previous changes (20)
- apps/api/src/notifications/services/notification-handler/notification.handler.spec.ts
- apps/api/src/core/services/domain-events/domain-events.service.ts
- apps/api/src/core/providers/app-initializer.ts
- apps/api/src/app/providers/jobs.provider.ts
- apps/api/src/notifications/services/notification-templates/after-trial-ends-notification.ts
- apps/api/src/core/providers/postgres.provider.ts
- apps/api/src/core/services/job-queue/job-queue.service.spec.ts
- apps/api/src/billing/repositories/user-wallet/user-wallet.repository.ts
- apps/api/src/billing/events/trial-started.ts
- apps/api/src/notifications/services/notification/notification.service.ts
- apps/api/src/app.ts
- apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.ts
- apps/api/src/notifications/services/notification-templates/trial-ended-notification.ts
- apps/api/test/services/wallet-testing.service.ts
- apps/api/test/seeders/user.seeder.ts
- apps/api/src/app/services/trial-started/trial-started.handler.spec.ts
- apps/api/package.json
- apps/api/src/notifications/services/notification/notification.service.spec.ts
- apps/api/src/app/services/trial-started/trial-started.handler.ts
- apps/api/src/console.ts
🧰 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/api/src/notifications/services/notification-templates/before-trial-ends-notification.tsapps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.tsapps/api/src/core/services/feature-flags/feature-flags.service.tsapps/api/src/notifications/services/notification-handler/notification.handler.tsapps/api/src/core/services/job-queue/job-queue.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/notifications/services/notification-templates/before-trial-ends-notification.tsapps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.tsapps/api/src/core/services/feature-flags/feature-flags.service.tsapps/api/src/notifications/services/notification-handler/notification.handler.tsapps/api/src/core/services/job-queue/job-queue.service.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/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts
🧠 Learnings (1)
📚 Learning: 2025-07-27T12:16:08.566Z
Learnt from: CR
PR: akash-network/console#0
File: .cursor/rules/general.mdc:0-0
Timestamp: 2025-07-27T12:16:08.566Z
Learning: Applies to **/*.{ts,tsx} : Never use type any or cast to type any. Always define the proper TypeScript types.
Applied to files:
apps/api/src/core/services/job-queue/job-queue.service.ts
🧬 Code graph analysis (5)
apps/api/src/notifications/services/notification-templates/before-trial-ends-notification.ts (1)
apps/api/src/notifications/services/notification/notification.service.ts (1)
CreateNotificationInput(74-76)
apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts (5)
apps/api/src/billing/services/managed-user-wallet/managed-user-wallet.service.ts (1)
createAndAuthorizeTrialSpending(44-57)apps/api/src/core/services/feature-flags/feature-flags.ts (1)
FeatureFlags(1-5)apps/api/test/seeders/user-wallet.seeder.ts (1)
UserWalletSeeder(6-29)apps/api/test/seeders/chain-wallet.seeder.ts (1)
createChainWallet(4-12)apps/api/src/billing/events/trial-started.ts (1)
TrialStarted(4-14)
apps/api/src/core/services/feature-flags/feature-flags.service.ts (2)
apps/api/src/core/providers/app-initializer.ts (3)
APP_INITIALIZER(3-3)AppInitializer(6-8)ON_APP_START(4-4)apps/api/src/app/providers/jobs.provider.ts (1)
ON_APP_START(11-15)
apps/api/src/notifications/services/notification-handler/notification.handler.ts (7)
apps/api/src/notifications/services/notification-templates/trial-ended-notification.ts (1)
trialEndedNotification(4-16)apps/api/src/notifications/services/notification-templates/before-trial-ends-notification.ts (1)
beforeTrialEndsNotification(6-20)apps/api/src/notifications/services/notification-templates/after-trial-ends-notification.ts (1)
afterTrialEndsNotification(4-16)apps/api/src/notifications/services/notification-templates/start-trial-notification.ts (1)
startTrialNotification(4-16)apps/api/src/notifications/services/notification/notification.service.ts (2)
CreateNotificationInput(74-76)singleton(13-67)apps/api/src/core/services/job-queue/job-queue.service.ts (4)
Job(133-137)JOB_NAME(139-139)singleton(9-131)JobHandler(146-149)apps/api/src/app/services/trial-started/trial-started.handler.ts (1)
singleton(13-94)
apps/api/src/core/services/job-queue/job-queue.service.ts (3)
apps/api/src/app/services/trial-started/trial-started.handler.ts (1)
singleton(13-94)apps/api/src/core/services/domain-events/domain-events.service.ts (1)
singleton(10-17)apps/api/src/notifications/services/notification-handler/notification.handler.ts (1)
singleton(37-78)
⏰ 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: test-build
- 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: test-build
- GitHub Check: test-build
🔇 Additional comments (8)
apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts (6)
5-6: Domain-event imports look correct.Switching tests to assert on DomainEventsService and TrialStarted aligns with the new design.
64-66: Existing-wallet path assertion is good.You correctly ensure createAndAuthorizeTrialSpending is not called.
71-86: Error path behavior looks solid.On failure to authorize, you assert delete and ensure no event is published. Good coverage of the rollback path.
109-116: Setup signature aligns with the guide.Single parameter with inline type and no explicit return type: good.
146-146: Good: DomainEventsService provided via typed mock.This keeps the tests focused on published payloads and call counts.
150-151: Feature flag evaluation is clean.Simple predicate over the provided enabledFeatures list works well here.
apps/api/src/notifications/services/notification-templates/before-trial-ends-notification.ts (1)
7-9: Harden date handling in before-trial-ends notification templateTo prevent subtle drift, avoid calling
new Date()twice and guard against invalid or non-ISO date inputs:• File: apps/api/src/notifications/services/notification-templates/before-trial-ends-notification.ts (lines 7–9)
• Capture “now” once in a local variable
• Parse and validatevars.trialEndsAtusingDate.parse+Number.isFinite
• Clamp negative day counts to 0
• UsedifferenceInDayswith millisecond timestamps (it acceptsDate | number)Proposed diff:
- const trialEndsDate = new Date(vars.trialEndsAt); - const daysLeft = trialEndsDate.getTime() < Date.now() ? 0 : differenceInDays(trialEndsDate, new Date()); + const now = new Date(); + const trialEndsMs = Date.parse(vars.trialEndsAt); + const daysLeft = + Number.isFinite(trialEndsMs) && trialEndsMs >= now.getTime() + ? differenceInDays(trialEndsMs, now) + : 0;Please verify that
vars.trialEndsAtis always provided as an ISO-formatted UTC string; otherwise consider a stricter parser or explicit timezone handling.apps/api/src/core/services/job-queue/job-queue.service.ts (1)
93-113: LGTM: sanitized job logging in workersOnly job IDs are logged; payloads/PII are omitted. Thanks.
apps/api/src/notifications/services/notification-handler/notification.handler.ts
Show resolved
Hide resolved
apps/api/src/notifications/services/notification-handler/notification.handler.ts
Show resolved
Hide resolved
* feat: adds more notifications on start trial event * chore: code review
3 participants
Why
ref #1735
Summary by CodeRabbit
New Features
Improvements
Chores
Tests