Fix device registration#143
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughModified device registration and notification subscription handlers to implement idempotent conflict resolution. Device registration now updates existing devices on unique constraint violations instead of returning errors. Notification subscription allows subscriptions without push tokens, conditionally executing installation registration only when tokens are present. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler
participant DB
participant Remote as Remote Service
rect rgb(200, 230, 255)
note over Handler: Device Registration Flow (Before)
Client->>Handler: Register Device
Handler->>DB: Create Device
alt Conflict (P2002)
DB-->>Handler: Unique Constraint Error
Handler->>Client: 409 Conflict
end
end
rect rgb(200, 230, 255)
note over Handler: Device Registration Flow (After)
Client->>Handler: Register Device
Handler->>DB: Create Device
alt Conflict (P2002)
DB-->>Handler: Unique Constraint Error
Handler->>Handler: Build Update Data
opt Push Token Provided
Handler->>DB: Clear Token from Other Devices
end
Handler->>DB: Update Current Device
Handler->>Client: 200 Success
end
end
sequenceDiagram
participant Client
participant Handler
participant Remote as Remote Service
participant DB
rect rgb(230, 255, 200)
note over Handler: Notification Subscribe Flow (Before)
Client->>Handler: Subscribe (no push token)
Handler->>Client: 400 Bad Request (error early)
end
rect rgb(230, 255, 200)
note over Handler: Notification Subscribe Flow (After)
Client->>Handler: Subscribe
alt Has Push Token
Handler->>Remote: Register Installation
Remote-->>Handler: Installation ID
Handler->>Remote: Subscribe to Topic
Remote-->>Handler: Subscription OK
else No Push Token
Handler->>Handler: Skip Remote Operations (info log)
end
Handler->>DB: Create/Update Client Record
Handler->>Client: 200 Success
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes The changes involve conditional logic modifications and state management updates across two handlers with interconnected concerns (device registration and push tokens). While individual changes are clear, understanding the implications of making push tokens optional and the idempotent update behavior requires careful verification of edge cases and error paths. Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
|
Claude finished @lourou's task —— View job Code Review Complete ✅I've thoroughly reviewed this PR. Below is my feedback organized by category: 🎯 SummaryThis PR successfully implements idempotent device registration and allows subscription without push tokens. The changes are well-structured and address the core requirements. However, there are several important issues that should be addressed before merging. 🐛 Critical Issues1. Empty String Push Token Handling Inconsistency (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/api/v2/device/handlers/register.ts (2)
76-105: Don’t mutate old device’s pushTokenType/apnsEnv; also clear all matches, not just one.
- Updating the old row with pushTokenType: "apns" and apnsEnv: null changes its metadata unnecessarily.
- findFirst + single update can leave multiple stale rows if data drift exists.
Null only the token and use updateMany constrained by the same type/env tuple.
- const existingDevice = await tx.deviceRegistration.findFirst({ - where: { - pushToken: body.pushToken, - pushTokenType, - apnsEnv, - deviceId: { not: body.deviceId }, - }, - }); - - if (existingDevice) { - req.log.info( - { - oldDeviceId: existingDevice.deviceId, - newDeviceId: body.deviceId, - pushToken: body.pushToken, - }, - "Push token moving from old device to new device - clearing old registration", - ); - - // Clear the push token from the old device to avoid unique constraint violation - await tx.deviceRegistration.update({ - where: { deviceId: existingDevice.deviceId }, - data: { - pushToken: null, - pushTokenType: "apns", - apnsEnv: null, - }, - }); - } + const cleared = await tx.deviceRegistration.updateMany({ + where: { + deviceId: { not: body.deviceId }, + pushToken: body.pushToken, + pushTokenType, + apnsEnv, + }, + data: { pushToken: null }, + }); + if (cleared.count > 0) { + req.log.info( + { oldRegistrationsCleared: cleared.count, newDeviceId: body.deviceId }, + "Cleared stale registrations with same push token", + ); + }
87-94: Avoid logging raw pushToken values.push tokens can be sensitive identifiers. Prefer not logging them; if needed, log a hash prefix.
- pushToken: body.pushToken, + hasPushToken: true,src/api/v2/notifications/handlers/subscribe.ts (1)
150-151: Add explicit return after sending the response.Guideline: always return immediately after res.status().send()/json().
- res.status(200).send(); + res.status(200).send(); + return;
🧹 Nitpick comments (4)
src/api/v2/device/handlers/register.ts (3)
9-15: Empty-string normalization doesn’t clear tokens as the comment suggests.The transform turns "" into undefined, so updateData.pushToken isn’t set and the token won’t be cleared. If you intend to clear on empty string, accept null and normalize to null.
- pushToken: z - .string() - .optional() - .transform((val) => (!val || val.trim() === "" ? undefined : val)), + pushToken: z + .string() + .nullable() + .optional() + .transform((val) => (val == null ? null : val.trim() === "" ? null : val)), @@ - if (body.pushToken !== undefined) { - // Normalize: undefined or empty string -> null - updateData.pushToken = body.pushToken || null; - } + if (body.pushToken !== undefined) { + updateData.pushToken = body.pushToken; // may be string or null + }Also applies to: 56-60
138-206: Reduce dual-path logic: handle idempotency inside the transaction, drop P2002 catch branch.Current flow duplicates update logic and diverges on validation and cleanup filters. Prefer a single transactional path that:
- clears conflicting rows with updateMany scoped by (pushToken, pushTokenType, apnsEnv),
- then upserts.
This removes the need for the P2002 catch or reduces it to a last-resort retry.
121-128: Error handling consistency with AppError/logError utilities.Consider using the shared AppError pattern and centralized error middleware for consistency across routes.
Also applies to: 210-213
src/api/v2/notifications/handlers/subscribe.ts (1)
16-18: Validate even-length hex for HMAC keys.hexToUint8Array typically requires an even number of hex chars. Enforce this in Zod to fail fast.
- key: z.string().regex(/^[0-9a-fA-F]+$/, "Invalid hex string"), + key: z + .string() + .regex(/^(?:[0-9a-fA-F]{2})+$/, "Invalid hex string (must be even-length hex)"),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/api/v2/device/handlers/register.ts(1 hunks)src/api/v2/notifications/handlers/subscribe.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{ts,tsx}: TypeScript with strict mode and ESNext features
Implement consistent error handling using AppError class
Use object parameter syntax for function arguments: '(args: { paramOne: string }) => {}' instead of '(paramOne: string) => {}' for better readability and extensibility
Keep functions focused and avoid optional parameters. If a function has many optional parameters, consider splitting it into multiple specialized functions for clearer intent and better maintainability
Prefer type inference from Prisma models
Keep types close to their usage (same directory)
Use type exports for shared types
Leverage path aliases (@/) for imports
Don't specify return type on functions. Prefer inferring the value.
Use transactions for multi-table operations
Include necessary relations in queries using include
Use custom AppError class for application errors
Log errors with context using logError utility
Log security-relevant events
Implement caching where needed
Optimize database queries
Use appropriate batch operations
Add JSDoc comments for public APIs
Files:
src/api/v2/notifications/handlers/subscribe.tssrc/api/v2/device/handlers/register.ts
**/{api,routes}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
**/{api,routes}/**/*.{ts,tsx}: For API route Response types, use simple typing (e.g., 'res: Response') instead of verbose generic types (e.g., 'res: Response<unknown, any>') to keep code clean and readable
Follow RESTful principles for endpoint naming
Always place the 'return;' statement on a new line immediately after sending a response with 'res.status().json()' or similar methods
Use Zod schemas for request validation and type inference
Return appropriate HTTP status codes
Handle file uploads efficiently
Files:
src/api/v2/notifications/handlers/subscribe.tssrc/api/v2/device/handlers/register.ts
**/handlers/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
Use dedicated handlers directory for route logic
Files:
src/api/v2/notifications/handlers/subscribe.tssrc/api/v2/device/handlers/register.ts
**/{api,routes,handlers, middleware}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
**/{api,routes,handlers, middleware}/**/*.{ts,tsx}: Implement consistent error responses
Handle validation errors separately from other errors
Files:
src/api/v2/notifications/handlers/subscribe.tssrc/api/v2/device/handlers/register.ts
**/*
📄 CodeRabbit inference engine (.cursorrules)
Follow consistent file naming (kebab-case)
Files:
src/api/v2/notifications/handlers/subscribe.tssrc/api/v2/device/handlers/register.ts
**/{middleware,api,routes}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
**/{middleware,api,routes}/**/*.{ts,tsx}: Implement rate limiting
Use security headers (Helmet)
Handle authentication consistently
Files:
src/api/v2/notifications/handlers/subscribe.tssrc/api/v2/device/handlers/register.ts
**/{api,routes, middleware}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
Validate and sanitize inputs
Files:
src/api/v2/notifications/handlers/subscribe.tssrc/api/v2/device/handlers/register.ts
🧬 Code graph analysis (1)
src/api/v2/device/handlers/register.ts (1)
src/utils/prisma.ts (1)
prisma(3-3)
🔇 Additional comments (1)
src/api/v2/notifications/handlers/subscribe.ts (1)
87-93: No action required—behavior is intentional and correct.The Prisma schema explicitly marks FCM as "Not supported - kept for database compatibility only," with only two enum values:
apnsandfcm. The code correctly reflects this:
register.tsrejects new FCM registrations (line 27)subscribe.tsgracefully handles legacy FCM devices by mapping them tofirebaseDeviceToken(lines 89-91)This is backward-compatible handling of legacy database rows, not an inconsistency. The fallback is appropriate for devices that predate the FCM deprecation.
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
1 participant
Fix device registration by clearing duplicate push tokens across existing registrations in
src/api/v2/device/handlers/register.tsand allowing subscription without a push token insrc/api/v2/notifications/handlers/subscribe.tsThis pull request introduces a helper to wrap the device registration transaction and updates the registration and subscription handlers to align with new push token handling behavior. It centralizes the clearing of duplicate push tokens during registration and adjusts the subscription flow to proceed without requiring a push token.
performDeviceRegistrationhelper in register.ts that clears matchingpushTokenvalues on other device registrations and upserts the current device with defaults forpushTokenTypeandapnsEnvregisterhandler in register.ts to useperformDeviceRegistrationand remove explicit PrismaP2002conflict handlingsubscribehandler in subscribe.ts to skip notification installation and subscription when a device has nopushToken, logging and returning 200 regardless📍Where to Start
Start with the
performDeviceRegistrationhelper and its usage in theregisterhandler in register.ts, then review the conditional subscription flow in subscribe.ts.📊 Macroscope summarized 5ba336f. 2 files reviewed, 10 issues evaluated, 9 issues filtered, 0 comments posted
🗂️ Filtered Issues
src/api/v2/device/handlers/register.ts — 0 comments posted, 2 evaluated, 1 filtered
Prismaerror codeP2002) were mapped to a 409 conflict response with a specific message. The new implementation removed that handling and now falls through to the generic 500 response for all errors. This changes the externally visible contract: clients encountering a push-token registration race or constraint violation will now receive a 500 instead of a 409, losing actionable semantics and potentially breaking client error handling logic. [ Low confidence ]src/api/v2/notifications/handlers/subscribe.ts — 0 comments posted, 8 evaluated, 8 filtered
topicis only validated as a string and may be empty ("") or contain invalid characters for the downstream topic format. Forwarding an empty/invalid topic can cause remote API errors. The schema should enforce non-empty and, if applicable, a topic pattern (e.g.,.min(1)or a regex). [ Low confidence ]hmacKeysarrays are allowed to be empty (z.array(...)without.min(1)). Subscribing with an emptyhmacKeysarray likely violates the downstream API’s requirements for HMAC validation and may cause runtime errors on subscription. Enforce a minimum length (e.g.,.min(1)) or explicitly handle the empty case with a 400 response. [ Invalidated by documentation search ]thirtyDayPeriodsSinceEpochis only validated as a number. Non-integer or negative values are accepted by the schema and forwarded to the notification server. If the downstream expects a non-negative integer period, this can cause remote errors or undefined behavior at runtime. The schema should enforce an integer and range, e.g.,z.number().int().nonnegative(). [ Low confidence ]req.logis used multiple times without a visible guarantee that the ExpressRequestis augmented with alogproperty at runtime. If middleware addingreq.logis not applied to this route,req.logwill be undefined and calls likereq.log.info(...)will throw a TypeError, resulting in a 500. Either ensure a guard or rely on types/middleware that guarantee its presence at runtime for this handler. [ Low confidence ]hmacKeysarrays andkeylength could cause excessive memory usage during conversion toUint8Array, leading to performance degradation or process memory exhaustion (DoS risk). There is no size limit onhmacKeysper topic, and no maximum hex length forkey. Consider adding reasonable.max(...)constraints for array length andkeylength. [ Low confidence ]hexToUint8Arrayto throw at runtime. The Zod schema (hmacKeys.*.key) only checks that the string contains hex characters, not that it has an even number of characters.hexToUint8Arraytypically requires an even-length hex string; an odd-length input can cause a runtime error, resulting in a 500 response instead of a 400 validation error. Add an even-length constraint to the schema or wrap the conversion in a guard and respond with 400 on invalid hex. [ Low confidence ]device.pushTokenis absent, the handler returns 200 after logging, but does not persist the requested subscriptions anywhere. This silently drops the client’s subscription intent with no mechanism shown to "activate once token is registered." If/when the push token is later registered, the server has no record of which topics/hmac keys to subscribe, resulting in a broken external contract and lost subscriptions. [ Low confidence ]device.pushTokenis set butdevice.pushTokenTypeis null/undefined or invalid, the ternary defaults tofirebaseDeviceToken. This can register an APNS token as Firebase (or vice versa), leading to delivery failures. The code should validatepushTokenTypewhenpushTokenexists and return a clear 500/400 or map only known values with an explicit else-case error. [ Low confidence ]Summary by CodeRabbit
New Features
Bug Fixes