Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 25 minutes and 35 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR implements a real-time notification system for the backend-relayer application. It adds WebSocket support via Socket.IO, introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client as WebSocket Client
participant Gateway as NotificationGateway
participant JwtService as JwtService
participant PrismaDB as Prisma/Database
participant Service as NotificationService
participant Server as Server/TradesService
Client->>Gateway: connect (with auth token or Bearer header)
Gateway->>JwtService: verifyAsync(token, secret)
JwtService-->>Gateway: JWT payload with sub (userId)
Gateway->>Gateway: Store userId on client.data
Gateway->>Gateway: socket.join(`user:<userId>`)
Gateway-->>Client: connection established
Server->>Service: create(userId, type, title, body, payload)
Service->>PrismaDB: notification.create(...)
PrismaDB-->>Service: Notification record
Service->>Gateway: pushToUser(userId, notification)
Gateway->>Gateway: io.to(`user:<userId>`).emit('notification', ...)
Gateway-->>Client: 'notification' event received
Client->>Gateway: disconnect
Gateway->>Gateway: Log userId removal
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/backend-relayer/package.json (1)
23-67:⚠️ Potential issue | 🔴 CriticalUpdate
pnpm-lock.yamlto match the new dependencies.CI is failing with
ERR_PNPM_OUTDATED_LOCKFILE: the lockfile does not includesocket.ioand has a stale specifier for@nestjs/platform-socket.io. Runpnpm installlocally and commit the updatedpnpm-lock.yaml.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend-relayer/package.json` around lines 23 - 67, The lockfile is out of date relative to the dependency changes (missing socket.io and stale specifier for `@nestjs/platform-socket.io`); run pnpm install in the repo root to regenerate pnpm-lock.yaml so it includes the new socket.io entry and the updated `@nestjs/platform-socket.io` specifier, then commit the updated pnpm-lock.yaml alongside the package.json change.
🧹 Nitpick comments (6)
apps/backend-relayer/package.json (1)
56-56: Consider movingprismatodevDependencies.The
prismaCLI is typically a dev-only tool; keeping it independenciesbloats production installs. Only keep it here ifprisma migrate deploymust run from the production image at runtime (in which case this is fine as-is).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend-relayer/package.json` at line 56, The prisma CLI is declared under dependencies as "prisma": "6.16.1" which should be a dev-only tool; remove the "prisma": "6.16.1" entry from the dependencies block in package.json and add the same entry under devDependencies instead, then reinstall (npm/yarn) so the lockfile updates; only leave it in dependencies if you intentionally need prisma at runtime (e.g., running prisma migrate deploy in production) — check package.json scripts for any runtime usage before moving.apps/backend-relayer/src/modules/notifications/dto/notification.dto.ts (1)
25-28: Minor:cursortype narrower than service contract.
NotificationService.listacceptscursor?: string | null, but this DTO types it asstring. Not a bug (controller passescursor ?? null), just a small contract inconsistency worth aligning.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend-relayer/src/modules/notifications/dto/notification.dto.ts` around lines 25 - 28, The DTO's cursor property is typed as string while NotificationService.list accepts string | null; update the DTO so the cursor property type matches the service contract (make it string | null) and mark it nullable in the OpenAPI metadata (e.g., set ApiPropertyOptional to indicate nullable) so the API contract aligns with NotificationService.list; locate and update the cursor property in the notification DTO and ensure any callers/controllers continue to pass cursor ?? null where used.apps/backend-relayer/prisma/schema.prisma (1)
353-367: No foreign key betweenNotification.tradeIdandTrade.
tradeIdis stored as a free-form nullable string with only an index — there is no relation/FK toTrade. This lets notifications outlive deleted trades (likely intentional), but it also means nothing prevents stale/invalid IDs. If preservation afterTradedeletion is the goal, consider an explicit relation withonDelete: SetNull; if not, wire a proper relation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend-relayer/prisma/schema.prisma` around lines 353 - 367, The Notification model currently stores tradeId as a free-form nullable String without a relation to Trade; decide whether notifications should keep tradeId after a Trade is deleted and implement the corresponding FK: if you want to preserve notifications but null out the link, change tradeId to a relation to the Trade model (referencing Trade.id) and set onDelete: SetNull on the relation; if you want full cascade/remove behavior, add the relation with onDelete: Cascade instead. Update the Notification model's fields (tradeId and the relation definition) and adjust @@index usage accordingly to reflect the new relation.apps/backend-relayer/src/modules/notifications/notification.module.ts (1)
12-18: RedundantJwtServiceprovider.
JwtModule.register({})already exportsJwtService; listingJwtServiceagain inproviderscreates a second, module-scoped instance that may shadow the one fromJwtModule. Consider removing it unless intentional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend-relayer/src/modules/notifications/notification.module.ts` around lines 12 - 18, The providers array in the NotificationModule registers JwtService redundantly which creates a second module-scoped instance and can shadow the JwtService exported by JwtModule.register({}); remove JwtService from the providers list in notification.module.ts and rely on the JwtService exported by JwtModule (ensure JwtModule.register({...}) is present in the module imports) so NotificationService, NotificationGateway (and UserJwtGuard) inject the correct shared JwtService instance.apps/backend-relayer/src/modules/notifications/notification.service.spec.ts (1)
49-64: Optional: assert the shape passed toprisma.notification.create.The test verifies call count and gateway push, but not that the
datapayload mapped from the input is what gets persisted (e.g.,userId,type,tradeId,title,bodyland indata). AtoHaveBeenCalledWith(...)assertion would catch regressions in that mapping.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend-relayer/src/modules/notifications/notification.service.spec.ts` around lines 49 - 64, Add an assertion to the test that verifies the exact payload passed into prisma.notification.create so the input-to-DB mapping is covered: after calling service.create in the test, assert prisma.notification.create was called with an object whose data contains the fields userId, type, tradeId, title, and body mapped from the create input (use makeRow() as the expected created value or match object shape), referencing prisma.notification.create and service.create to locate the call; keep the existing assertions for call count and gateway.pushToUser.apps/backend-relayer/test/e2e/notifications.e2e-spec.ts (1)
69-96: Add coverage for the real-time notification push.This verifies persistence through REST, but not the Socket.IO delivery path added by the PR. Add one test that connects as the ad creator to
/notificationsand asserts anotificationevent is received when the bridger creates the trade.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend-relayer/test/e2e/notifications.e2e-spec.ts` around lines 69 - 96, Add a Socket.IO-based assertion: before posting to '/v1/trades/create' in the test that uses seedAdWithLoggedInCreator(), open a socket.io-client connection to the server namespace/path '/notifications' using app.getHttpServer() and the creator's access token (pass token via auth or query as used by your server), register a one-time listener for the 'notification' event, then trigger the POST to '/v1/trades/create' and await the socket event; assert the received payload has type 'TRADE_CREATED', tradeId matching the created trade (or non-null) and read === false, and finally disconnect the socket to clean up. Use the existing helper names (seedAdWithLoggedInCreator, app.getHttpServer()) and the same endpoints ('/v1/trades/create', '/notifications') so the test covers the real-time delivery path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/backend-relayer/src/modules/notifications/notification.service.ts`:
- Around line 86-92: The markRead method is currently filtering updates with
read: false which makes repeated requests return null; change the updateMany
call in markRead to remove the read: false predicate so the where becomes { id,
userId } (keep ownership check) and still call
this.prisma.notification.findUnique({ where: { id } }) afterwards; keep the
existing logic that returns null when updateMany().count === 0 so only non-owned
notifications still yield null.
- Around line 60-77: The pagination is non-deterministic because orderBy uses
only createdAt; update the notification listing in the list method
(prisma.notification.findMany call in notification.service.ts) to include id as
a tiebreaker by changing orderBy from { createdAt: 'desc' } to an array like [{
createdAt: 'desc' }, { id: 'desc' }], keep the existing cursor behavior (cursor:
{ id: query.cursor }, skip: 1) and keep nextCursor as the last item's id so
pages remain deterministic when timestamps collide.
- Around line 105-115: The userIdForAddress function currently calls
normalizeChainAddress(address) without chain context, causing Stellar addresses
to be mis-normalized and missed; update async userIdForAddress(address: string,
chainKind?: ChainKind) to pass chainKind into normalizeChainAddress and include
chainKind in the Prisma lookup (where: { address: normalized, chainKind }) so
the query matches the unique [address, chainKind] constraint; then update call
sites that have route info to supply the chain kind: for TRADE_CREATED pass
ad.route.adToken.chain.kind, for BRIDGER_CLAIMED pass the trade route's chain
kind, and for TRADE_LOCKED load the route data and pass its chain kind when
calling userIdForAddress.
In `@apps/backend-relayer/src/modules/trades/trade.service.ts`:
- Around line 515-529: The recipient lookup (notifications.userIdForAddress)
runs outside the non-throwing notification path so if it throws the endpoint can
return 500 after the trade is committed; create a small non-throwing helper
(e.g. notifyUserSafely or safeNotifyForAddress) that calls
notifications.userIdForAddress inside a try/catch and, if a userId is resolved,
calls notifications.safeCreate, swallowing/logging lookup errors but not
rethrowing; replace the direct await notifications.userIdForAddress(...) +
notifications.safeCreate(...) sequences in the shown block and the similar
blocks at the other occurrences (around lines 1229-1239 and 1396-1406) with
calls to this helper so all notification lookup+create is protected from
throwing.
- Around line 1395-1406: The branch sending a "your turn" notification to the ad
creator when !isAdCreator should be guarded by whether the ad creator had
already claimed; modify the logic around notifications.userIdForAddress /
notifications.safeCreate to first check the trade's previous claim state (e.g.
authorizationLog.trade.adCreatorClaimed or similar flag on
authorizationLog.trade) and only call safeCreate with type 'BRIDGER_CLAIMED'
when that flag indicates the ad creator has not already claimed; update the
condition around isAdCreator to include this guard so you don't send an
actionable "your turn" after the creator already claimed.
---
Outside diff comments:
In `@apps/backend-relayer/package.json`:
- Around line 23-67: The lockfile is out of date relative to the dependency
changes (missing socket.io and stale specifier for `@nestjs/platform-socket.io`);
run pnpm install in the repo root to regenerate pnpm-lock.yaml so it includes
the new socket.io entry and the updated `@nestjs/platform-socket.io` specifier,
then commit the updated pnpm-lock.yaml alongside the package.json change.
---
Nitpick comments:
In `@apps/backend-relayer/package.json`:
- Line 56: The prisma CLI is declared under dependencies as "prisma": "6.16.1"
which should be a dev-only tool; remove the "prisma": "6.16.1" entry from the
dependencies block in package.json and add the same entry under devDependencies
instead, then reinstall (npm/yarn) so the lockfile updates; only leave it in
dependencies if you intentionally need prisma at runtime (e.g., running prisma
migrate deploy in production) — check package.json scripts for any runtime usage
before moving.
In `@apps/backend-relayer/prisma/schema.prisma`:
- Around line 353-367: The Notification model currently stores tradeId as a
free-form nullable String without a relation to Trade; decide whether
notifications should keep tradeId after a Trade is deleted and implement the
corresponding FK: if you want to preserve notifications but null out the link,
change tradeId to a relation to the Trade model (referencing Trade.id) and set
onDelete: SetNull on the relation; if you want full cascade/remove behavior, add
the relation with onDelete: Cascade instead. Update the Notification model's
fields (tradeId and the relation definition) and adjust @@index usage
accordingly to reflect the new relation.
In `@apps/backend-relayer/src/modules/notifications/dto/notification.dto.ts`:
- Around line 25-28: The DTO's cursor property is typed as string while
NotificationService.list accepts string | null; update the DTO so the cursor
property type matches the service contract (make it string | null) and mark it
nullable in the OpenAPI metadata (e.g., set ApiPropertyOptional to indicate
nullable) so the API contract aligns with NotificationService.list; locate and
update the cursor property in the notification DTO and ensure any
callers/controllers continue to pass cursor ?? null where used.
In `@apps/backend-relayer/src/modules/notifications/notification.module.ts`:
- Around line 12-18: The providers array in the NotificationModule registers
JwtService redundantly which creates a second module-scoped instance and can
shadow the JwtService exported by JwtModule.register({}); remove JwtService from
the providers list in notification.module.ts and rely on the JwtService exported
by JwtModule (ensure JwtModule.register({...}) is present in the module imports)
so NotificationService, NotificationGateway (and UserJwtGuard) inject the
correct shared JwtService instance.
In `@apps/backend-relayer/src/modules/notifications/notification.service.spec.ts`:
- Around line 49-64: Add an assertion to the test that verifies the exact
payload passed into prisma.notification.create so the input-to-DB mapping is
covered: after calling service.create in the test, assert
prisma.notification.create was called with an object whose data contains the
fields userId, type, tradeId, title, and body mapped from the create input (use
makeRow() as the expected created value or match object shape), referencing
prisma.notification.create and service.create to locate the call; keep the
existing assertions for call count and gateway.pushToUser.
In `@apps/backend-relayer/test/e2e/notifications.e2e-spec.ts`:
- Around line 69-96: Add a Socket.IO-based assertion: before posting to
'/v1/trades/create' in the test that uses seedAdWithLoggedInCreator(), open a
socket.io-client connection to the server namespace/path '/notifications' using
app.getHttpServer() and the creator's access token (pass token via auth or query
as used by your server), register a one-time listener for the 'notification'
event, then trigger the POST to '/v1/trades/create' and await the socket event;
assert the received payload has type 'TRADE_CREATED', tradeId matching the
created trade (or non-null) and read === false, and finally disconnect the
socket to clean up. Use the existing helper names (seedAdWithLoggedInCreator,
app.getHttpServer()) and the same endpoints ('/v1/trades/create',
'/notifications') so the test covers the real-time delivery path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fc589bbd-45d9-4b10-b01c-2c4b44b219a9
📒 Files selected for processing (14)
apps/backend-relayer/package.jsonapps/backend-relayer/prisma/migrations/20260418000000_add_notifications/migration.sqlapps/backend-relayer/prisma/schema.prismaapps/backend-relayer/src/app.module.tsapps/backend-relayer/src/modules/notifications/dto/notification.dto.tsapps/backend-relayer/src/modules/notifications/notification.controller.tsapps/backend-relayer/src/modules/notifications/notification.gateway.tsapps/backend-relayer/src/modules/notifications/notification.module.tsapps/backend-relayer/src/modules/notifications/notification.service.spec.tsapps/backend-relayer/src/modules/notifications/notification.service.tsapps/backend-relayer/src/modules/trades/trade.module.tsapps/backend-relayer/src/modules/trades/trade.service.tsapps/backend-relayer/test/e2e/notifications.e2e-spec.tsapps/frontend/components/orders/OrdersTable.tsx
- NotificationService.list: orderBy [{createdAt desc}, {id desc}] so
cursor pagination is deterministic when timestamps collide.
- NotificationService.markRead: drop read:false predicate so re-marking
an already-read row returns the row instead of 404ing. Ownership check
still gates non-owners via count === 0.
- NotificationService.safeCreateForAddress: new non-throwing helper that
wraps userIdForAddress + create in one try/catch. Without it, a Prisma
lookup throw would 500 the trade endpoint *after* the trade state has
already been committed.
- TradesService: route all three notification trigger points (create,
confirmChainAction LOCKED, confirmUnlockChainAction bridger) through
safeCreateForAddress.
- TradesService BRIDGER_CLAIMED: gate on !bridgerClaimed && !adCreatorClaimed
so we don't send "your turn" to the ad creator when they've already
claimed and the trade is completing.
- Tests: +5 unit tests (idempotent markRead, ownership guard,
safeCreateForAddress happy path / no-op / swallow-throw, composite
orderBy). 53/53 unit + 62/62 e2e pass
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds a notification pipeline (persisted + pushed over WebSocket) for the
trade lifecycle. Three events are wired up:
lock tx is confirmed and the trade transitions to
LOCKED.unlock tx is confirmed.
Summary by CodeRabbit
New Features
Refactor