Backend v2 foundations

[lourou]

Introduce v2 backend foundations by adding v2 auth, device registration, notifications routing, and database models for push with unique APNS token constraint and MAX_PUSH_FAILURES=10

• Add v2 database models and constraints for push notifications with DeviceRegistration and ClientIdentifier, including a unique index on (pushTokenType, apnsEnv, pushToken) and nullable pushToken in schema.prisma with corresponding migrations 20251003172324_add_v2_push_notification_models and 20251017193536_add_unique_constraint_to_push_tokens.
• Add shared XMTP webhook handler and router to support v2 and v1 flows, validation, APNS send, failure tracking with MAX_PUSH_FAILURES=10, device disablement, and cleanup in webhook-handler.ts and webhook.router.ts, and mount in v1 and v2 index routers.
• Add v2 routes for auth, device, and notifications, including /auth/token with rate limit 100/5m and AppCheck-only, /device/register with APNS-only token handling and uniqueness reassignment, and notifications subscribe/unsubscribe/unregister in v2 index, auth.router.ts, generate-token.ts, device.router.ts, register.ts, and notifications.router.ts.
• Add v2 middleware for AppCheck-only and JWT/AppCheck auth and JWT utilities enforcing MAX_JWT_METADATA_SIZE=1024 in auth.ts and jwt.ts; add webhook auth middleware in xmtpWebhookAuth.ts.
• Add config loader with required XMTP_NOTIFICATION_SECRET, JWT_SECRET, and NOTIFICATION_SERVER_URL, and replace direct process.env access across server, instrumentation, error handler, healthcheck, and notifications client in config.ts, index.ts, instrumentation.ts, errorHandler.ts, healthcheck.ts, and notifications client.
• Update notification services and types to support v1/v2 payloads, make encryptedMessage optional, redact APNS auth header in logs, and reject FCM in apns-push.service.ts, notifications-types.ts, and push-notification.service.ts.
• Extend v2 invites decode to include conversationExpiresAt, expiresAt, and expiresAfterUse, and update proto and tests in invite.proto and decode-invite-slug.ts.
• Adjust rate limits to 1000/5m general and 100/5m for auth in rateLimit.ts; add constants MAX_PUSH_FAILURES=10 and MAX_JWT_METADATA_SIZE=1024 in constants.ts.
• Bump Prisma to 6.17.0 and align schema/code; add tsconfig path alias for @prisma-zod/* and minor refactors in v1 handlers and tests.

📍Where to Start

Start with the shared XMTP webhook entrypoint handleXmtpNotification in webhook-handler.ts, then review the v2 device registration flow in register.ts and the v2 auth/token flow in generate-token.ts.

Changes since #141 opened

• Updated test data in V2 Invite API test case [9c17fdd]
• Added runtime validation for JWT_SECRET configuration [11ab9dc]
• Fixed IS_PRODUCTION and IS_DEVELOPMENT constants evaluation [11ab9dc]

---

📊 Macroscope summarized 11ab9dc. 28 files reviewed, 46 issues evaluated, 46 issues filtered, 0 comments posted

🗂️ Filtered Issues

proto/invite/v2/invite.proto — 0 comments posted, 1 evaluated, 1 filtered

• line 28: The field expiresAfterUse is declared as a plain bool in proto3 (bool expiresAfterUse = 9;). In proto3, scalar bool fields do not have presence, meaning there is no way to distinguish an "unset" value from an explicitly set false. If downstream logic needs to differentiate between "not provided" and false (e.g., for partial updates, defaults, or backward compatibility), this schema will silently conflate the two states. To preserve three-state semantics (unset/true/false), use google.protobuf.BoolValue or wrap the field in a oneof. Otherwise, explicitly document and enforce that the default false is always correct when the field is omitted. [ Low confidence ]

src/api/shared/notifications/services/apns-push.service.ts — 0 comments posted, 4 evaluated, 4 filtered

• line 109: Unhandled error path when generating the APNS JWT: getJwtToken() is invoked at line 109 without any try/catch. If jwt.sign() throws (e.g., invalid privateKey, malformed key format, unsupported algorithm, or runtime crypto error), sendPushNotification will throw and return a rejected promise instead of the advertised { success: boolean; error?: string } shape. This violates the method's contract and yields an ungraceful failure for callers. [ Out of scope ]
• line 121: Potential multiple resolution of the Promise due to multiple event handlers calling resolve: client.on('error') (lines 121–128), request.on('error') (lines 236–248), and request.on('end') (lines 181–234) each call resolve. Without guarding against re-entrancy (e.g., a settled flag) or using .once listeners, there is a risk that more than one of these handlers fires, causing double application of side effects (e.g., multiple client.close() calls) and violating the at-most-once completion invariant, even though subsequent resolve calls are ignored by the Promise. [ Out of scope ]
• line 145: Sensitive data leakage in logs: the code logs the entire payload object at lines 141–149. Given the accepted type AnyNotificationPayloadWithJWT, the payload may contain a JWT or other secrets specific to the notification, which will be exposed in logs. While the authorization header is redacted via safeHeaders, the payload is not redacted. Redact or omit sensitive fields in payload before logging. [ Out of scope ]
• line 152: No timeout or abort is configured for the HTTP/2 client or request. If APNS or the network stalls (no 'response', 'end', or 'error'), the Promise never resolves and the client connection can linger. This causes hanging calls and potential resource leakage. Add a request or client timeout and abort/close handlers to guarantee a terminal state. [ Out of scope ]

src/api/shared/notifications/services/notifications-types.ts — 0 comments posted, 1 evaluated, 1 filtered

• line 6: The type change making ProtocolNotificationData.encryptedMessage optional (encryptedMessage?: string) loosens the contract. Any downstream code that assumes this field is present and attempts to parse or decrypt it without an existence check could encounter runtime failures (e.g., using it as a string). Ensure that all consumers of ProtocolNotificationData handle the undefined case gracefully by checking for existence before use, and that any emitted artifacts based on this field remain valid when it is absent. [ Low confidence ]

src/api/shared/notifications/webhook-handler.ts — 0 comments posted, 6 evaluated, 6 filtered

• line 15: Module-level initialization of notificationClient relies on createNotificationClient() which now does not validate NOTIFICATION_SERVER_URL. If the env var is missing, subsequent calls (e.g., deleteInstallation, unsubscribe) will fail at runtime with transport errors. This should be validated pre-effect at module load or handled gracefully during request handling by guarding against undefined base URL and producing a clear error. [ Invalidated by documentation search ]
• line 49: Potential runtime crash due to assuming req.log exists on the Express Request. The code calls req.log.* in multiple places (e.g., req.log.error on parse failure). The imported Request type from express does not include log. If a logging middleware has not attached log to the request object for this route, accessing req.log will throw a TypeError at runtime. Either refine the request type to include log, add a guard before using it, or ensure middleware attaches log on all paths to this handler. [ Low confidence ]
• line 139: Potential crash due to missing null check on identity in the v1 path. In handleXmtpNotification, the code accesses identity.xmtpId when building the push notification payload. However, identity is loaded via include: { identity: true } from prisma.identitiesOnDevice.findUnique without any explicit guarantee that the related identity exists. If identity is null or missing, accessing identity.xmtpId will throw a runtime error. Add an explicit check that identity is present (and contains a valid xmtpId) before dereferencing it, and handle the error path gracefully. [ Low confidence ]
• line 186: cleanupFailedInstallation increments pushFailures on the device table (lines 186–198), while the v2 code uses deviceRegistration to track pushFailures and disabled state. This split accounting leads to inconsistent state: failures on v1 cleanup go to device.pushFailures, but v2 notification handling reads and updates deviceRegistration.pushFailures and deviceRegistration.disabled. As a result, thresholds may be enforced on deviceRegistration but not on device, and vice versa. Additionally, cleanupFailedInstallation does not perform any conditional disable when exceeding MAX_PUSH_FAILURES, so a device may accumulate failures without ever being disabled in the device model. This divergence can cause mismatches in skip/disable decisions and stale device state. [ Low confidence ]
• line 225: The pre-check in handleV2Notification skips sending when client.device.disabled or client.device.pushFailures >= MAX_PUSH_FAILURES (lines 225–233), but it does not enforce the disablement when the threshold is exceeded. In failure-handling paths, the code increments pushFailures and atomically sets disabled = true when the threshold is reached, but in this early-return path there is no state update — it just returns { success: false }. This creates inconsistent behavior: a device with pushFailures already at or above the threshold will be skipped but not marked disabled, leaving inconsistent flags across devices depending on which path was last executed. [ Low confidence ]
• line 248: When APNS is not configured, handleV2Notification returns { success: false } without recording a push failure or updating any device state. This silently drops notifications and prevents the failure counter and disable logic from ever being applied for the affected device. Concretely, at lines 248–251, if createApnsService() returns null, the code logs an error and returns early; it does not increment client.device.pushFailures, set lastFailureAt, or conditionally set disabled when reaching MAX_PUSH_FAILURES. This leads to devices repeatedly failing to send due to global misconfiguration without ever being disabled or tracked, violating the intended invariant that repeated failures should be accounted for and eventually cause disablement. [ Invalidated by documentation search ]

src/api/v1/notifications/handlers/register-installation.ts — 0 comments posted, 1 evaluated, 1 filtered

• line 64: Ambiguous request classification: the handler first attempts to parse the body against legacyRegistrationSchema and routes to handleLegacyRegistration if it succeeds. Zod's default object parsing permits unknown keys (non-strict), so a request that includes both legacy fields (e.g., installationId, deliveryMechanism) and current fields (e.g., deviceId, pushToken, installations) will still satisfy legacyRegistrationSchema. In such cases, the function will incorrectly prefer the legacy path and ignore the newer installations array, leading to unexpected behavior and an externally visible contract mismatch on response shape. This is a runtime logic bug because inputs that include superset fields are reachable, and there is no guard preventing dual-format bodies. [ Low confidence ]

src/api/v2/device/handlers/register.ts — 0 comments posted, 3 evaluated, 3 filtered

• line 27: req.log is used throughout the register handler without a visible guard or guarantee that the property exists on the Express Request object. In standard Express, req.log is not defined; unless middleware augments req, this will throw at runtime on the first use (e.g., req.log.warn(...)). This affects all logging calls: warn, info, and error within the handler. To prevent runtime crashes, either ensure a middleware always attaches a logger to req before this handler runs, or add a fallback/guard (e.g., use a module-level logger or check req.log presence). [ Low confidence ]
• line 56: Empty string input for pushToken cannot clear an existing token during update despite the comment stating normalization of empty string to null. The schema transforms empty string to undefined at parse time (transform((val) => (!val || val.trim() === "" ? undefined : val))), and the update logic only applies pushToken when body.pushToken !== undefined. As a result, sending an empty string will cause pushToken to be omitted from updateData, leaving any existing token unchanged. This contradicts the in-code comment "Normalize: undefined or empty string -> null." To fix: either remove the transform and handle empty string in the update branch (pushToken === "" -> null), or keep the transform but set updateData.pushToken = null explicitly when the client intends to clear it (e.g., introduce a distinct field or logic to interpret omission vs. explicit clear). [ Invalidated by documentation search ]
• line 100: When clearing the old device’s registration upon push token transfer, the code also forcefully sets pushTokenType to "apns" and apnsEnv to null (data: { pushToken: null, pushTokenType: "apns", apnsEnv: null }). The comment only states "Clear the push token from the old device to avoid unique constraint violation" and does not mention altering type/environment, so this change can cause unintended data loss for the old device (e.g., losing its previous apnsEnv setting). If the model requires non-null pushTokenType, a safer approach is to leave other fields unchanged (only set pushToken: null) or set pushTokenType to a neutral value only if necessary, with explicit documentation clarifying the intended invariant. [ Low confidence ]

src/api/v2/invites/handlers/decode-invite-slug.ts — 0 comments posted, 3 evaluated, 3 filtered

• line 95: decodeInviteSlug calls recoverPublicKey(signedInvite) but discards the result and performs no signature/authenticity verification of signedInvite.payload. As long as the signature is present with length 65, a forged or invalid signature and payload will be accepted, and the function will return a DecodedInvite without validating that the payload was actually signed by a trusted key (e.g., comparing the recovered public key to an expected signer or verifying the signature over the canonicalized payload). This is a runtime security bug: invalid or tampered invites can pass as valid. To fix, perform an actual signature verification (e.g., use secp256k1.ecdsaVerify or compare the recovered public key to the expected signer derived from creatorInboxId or another trusted source) and fail if verification does not pass. [ Out of scope ]
• line 145: timestampDate(decoded.conversationExpiresAt).toISOString() and timestampDate(decoded.expiresAt).toISOString() can throw at runtime if the input is truthy but invalid (e.g., a malformed timestamp) and timestampDate returns an invalid Date. Date.prototype.toISOString() throws a RangeError for invalid dates. There is no validation/try-catch around these conversions, so a malformed but truthy input can crash the handler. [ Low confidence ]
• line 150: expiresAfterUse is not normalized to null when absent, unlike other nullable fields (name, description, imageURL, conversationExpiresAt, expiresAt) which use ?? null. If decoded.expiresAfterUse is undefined, the response will contain expiresAfterUse: undefined. If the DecodeInviteSlugResponse contract expects null for missing values (as suggested by the other fields’ handling), this creates an inconsistent payload and may break consumers relying on explicit null for "absent" fields or strict JSON schemas that disallow undefined. [ Low confidence ]

src/api/v2/notifications/handlers/subscribe.ts — 0 comments posted, 7 evaluated, 7 filtered

• line 13: topic is validated as z.string() with no constraints; empty strings or excessively long/invalid topic names are accepted. These are likely rejected by the notification server, resulting in a 500 instead of a 400. Add schema constraints (e.g., .min(1) and domain-specific validation) to prevent invalid topics reaching downstream. [ Low confidence ]
• line 14: hmacKeys is validated as z.array(...) with no .min(1) constraint. An empty array is accepted, producing subscriptions with hmacKeys: []. This is likely invalid for the notification server and will cause a remote error, reported as 500. Enforce .min(1) to correctly reject invalid client input as 400. [ Low confidence ]
• line 16: thirtyDayPeriodsSinceEpoch is validated as a generic z.number() without ensuring it is an integer, non-negative, or within a reasonable range. Fractional or negative values are accepted and will be forwarded to the notification server, likely causing a remote validation error and a 500 response. The schema should enforce integer (z.int()), min(0), and potentially reasonable bounds to return a 400 for invalid client input. [ Low confidence ]
• line 28: Missing configuration guard for NOTIFICATION_SERVER_URL can cause runtime failure at module initialization or later remote-call failures. createNotificationClient() is invoked at module load (singleton notificationClient), but if NOTIFICATION_SERVER_URL is undefined or invalid, createConnectTransport({ baseUrl: NOTIFICATION_SERVER_URL }) may throw or produce a client that fails on first use. The previous explicit check was removed in the diff, so startup may proceed with a broken client and only fail during handling requests. [ Invalidated by documentation search ]
• line 37: req.log is assumed to exist, but express.Request does not guarantee a log property. If a request arrives without the logging middleware attached, the first use at req.log.info(...) will throw, and the catch block also calls req.log.error(...), which will throw again, potentially resulting in an unhandled exception and no response sent. A guard or fallback logger is required before use on both success and error paths. [ Low confidence ]
• line 81: Hex key validation allows any-length hex strings via z.string().regex(/^[0-9a-fA-F]+$/), but many hex-to-bytes converters (including hexToUint8Array) expect an even-length input. An odd-length hex string will cause hexToUint8Array(key.key) to throw at runtime and be reported as a 500 error instead of a 400 validation error. Enforce even length in the schema or handle conversion errors to return a client error. [ Invalidated by documentation search ]
• line 92: Push token type handling assumes only two values: 'apns' maps to case: 'apnsDeviceToken', and all other values map to case: 'firebaseDeviceToken'. If device.pushTokenType is absent, unrecognized, or a different APNS variant (e.g., sandbox), this conditional will mislabel the delivery mechanism and may cause remote rejection. The schema should validate pushTokenType against allowed values, and the mapping should cover all supported cases explicitly with a clear error when unsupported. [ Low confidence ]

src/api/v2/notifications/handlers/unregister.ts — 0 comments posted, 2 evaluated, 2 filtered

• line 38: Both unregister() and unsubscribe() pass installationId: clientId to the notification service (deleteInstallation and unsubscribe). In v1 paths and example usage, installationId denotes an XMTP installation identifier distinct from a device/client ID (e.g., xmtpInstallationId). If the notification service expects an installation identifier rather than a client identifier, these calls may target the wrong resource or no-op, leading to failed cleanup/unsubscription. Verify the contract: if v2 requires passing clientId instead of XMTP installation id, then the server must map accordingly; otherwise, translate clientId to the correct installationId before invoking the notification client. [ Low confidence ]
• line 38: In unregister(), cleanup operations are performed in two separate calls: notificationClient.deleteInstallation() followed by prisma.clientIdentifier.delete(). If the first call succeeds (external installation removed) but the second fails (database deletion), the system ends up in an inconsistent state: the client record remains locally while the remote installation has been deleted. Consider wrapping both operations in a single transactional pattern with compensating logic (e.g., delete local first then remote, or use a transaction with best-effort rollback where feasible) to maintain invariants. At minimum, ensure partial failure is handled by explicitly reconciling state to avoid dangling records. [ Low confidence ]

src/config.ts — 0 comments posted, 2 evaluated, 2 filtered

• line 31: Startup crash risk: JWT_SECRET_BYTES = new TextEncoder().encode(JWT_SECRET) will throw a TypeError at module initialization if process.env.JWT_SECRET is undefined, because TextEncoder.encode requires a string. If undefined configuration is deployment-invalid, it’s acceptable to terminate at initialization, but as written there’s no explicit guard or clearer error, and the throw will be a generic TypeError. Consider adding an explicit check with a descriptive error and failing fast before external effects, e.g., if (!JWT_SECRET) throw new Error("JWT_SECRET is required"). [ Low confidence ]
• line 32: NOTIFICATION_SERVER_URL is exported directly from process.env and can be undefined. Without validation at initialization, downstream code may assume it is a string and attempt to use it in URL construction or network calls, causing runtime errors (e.g., TypeError: Failed to construct 'URL'). If this configuration is required for correct operation, it should be validated as part of process/service initialization to fail fast before side effects. [ Invalidated by documentation search ]

src/instrumentation.ts — 0 comments posted, 1 evaluated, 1 filtered

• line 31: The resourceFromAttributes call may include an invalid attribute value: "deployment.environment": process.env.ENV. When process.env.ENV is undefined (which is reachable with no guards), the resulting attribute value will be undefined. OpenTelemetry Attributes do not permit undefined values, and emitting an attribute with an undefined value can lead to malformed resource data and potential exporter/runtime errors. This violates the requirement that emitted artifacts be valid under all reachable inputs. Guard this value (e.g., omit the key when undefined or provide a default such as process.env.NODE_ENV) so the attributes passed to resourceFromAttributes are always valid. [ Out of scope ]

src/middleware/errorHandler.ts — 0 comments posted, 2 evaluated, 2 filtered

• line 16: Dereferencing req.log.error without a guard can throw at runtime if req.log is not present on the Request object or if req.log.error is not a function. The standard Express Request type does not include a log property; it is commonly added by logging middleware (e.g., pino-http). If that middleware is absent or fails, req.log will be undefined, causing TypeError: Cannot read properties of undefined (reading 'error') in the error handler, which prevents sending any response for the original error. Add a defensive check and fallback (e.g., to console.error) to ensure the handler never throws during logging. [ Out of scope ]
• line 27: Serializing err.details directly in the AppError branch can throw at runtime if details contains non-JSON-serializable values (e.g., BigInt, Symbol, or circular references). res.json(...) will attempt to stringify the response; if details is problematic, the error handler itself will throw, resulting in no response sent and potentially an unhandled exception cascade. Since details is typed as unknown and can be any value, guard or sanitize it before serialization (e.g., map to a safe shape, stringify with a safe replacer, or omit when non-serializable). [ Out of scope ]

src/middleware/rateLimit.ts — 0 comments posted, 4 evaluated, 4 filtered

• line 6: The limit option in rateLimitMiddleware may be ignored or misinterpreted depending on the installed version of express-rate-limit (some versions expect max instead of limit). If the package version expects max, using limit could cause unintended defaults or behavior (e.g., different rate limits than configured). Ensure the option name matches the installed library version, or set both limit/max according to documented compatibility. [ Low confidence ]
• line 8: The standardHeaders option is set to "draft-8" in rateLimitMiddleware. Depending on the installed version of express-rate-limit, this string value may be unsupported, which can cause either a runtime error during middleware creation or silently fall back to unexpected behavior (missing or incorrectly formatted RateLimit headers). Validate that the package version supports this exact literal value, or use the documented boolean/union values supported by the installed version. [ Out of scope ]
• line 14: The limit option in authRateLimitMiddleware may be the wrong option name for certain versions of express-rate-limit (which use max). If the installed version expects max, using limit can result in unintended default limits or ignored configuration, potentially over-throttling or under-throttling auth requests. Confirm and match the option name to the library version. [ Low confidence ]
• line 16: The standardHeaders option is set to "draft-8" in authRateLimitMiddleware. If the installed express-rate-limit version does not support this exact value, middleware initialization may throw or headers may be missing/malformed at runtime, reducing client visibility into rate limits. Verify version support for "draft-8" or use the documented values for the installed version. [ Low confidence ]

src/middleware/v2/auth.ts — 0 comments posted, 3 evaluated, 3 filtered

• line 36: The middleware conflates server-side initialization/configuration errors with client authentication errors. If verifyAppCheckToken throws due to failures in getFirebaseApp() or getAppCheck(app) (e.g., Firebase not initialized, misconfiguration, transient backend error), the catch block treats it as an invalid client token and responds with 401 (res.status(401).json({ error: "Invalid AppCheck token" })). This misclassification masks operational issues as client faults, complicating troubleshooting, and may cause inappropriate client behavior (e.g., retrying with different tokens). To fix, differentiate between known token verification failures (e.g., explicit "invalid token" errors) and infrastructure/configuration errors, returning 401 only for invalid/missing tokens and 5xx for server-side failures, or propagate via next(error) for centralized handling. [ Invalidated by documentation search ]
• line 59: If both X-Firebase-AppCheck and X-Convos-AuthToken headers are present and the AppCheck token is invalid, the middleware rejects the request with 401 without attempting JWT verification. This causes valid requests (with a valid JWT) to be denied if an AppCheck header is present but invalid. The intent comment says "Try AppCheck first" but does not state that AppCheck should be exclusive. A more robust approach is to fall back to JWT verification when AppCheck verification fails but a JWT is also provided. [ Low confidence ]
• line 66: All AppCheck verification errors are mapped to a 401 Unauthorized response (res.status(401).json({ error: "Invalid AppCheck token" })). This conflates genuine authentication failures with service/internal errors (e.g., network or Firebase service outages). Internal/operational errors should return a 5xx to accurately communicate server failure and avoid misleading clients. [ Low confidence ]

src/middleware/xmtpWebhookAuth.ts — 0 comments posted, 1 evaluated, 1 filtered

• line 23: The middleware dereferences req.log.error without any guard that req.log exists on the Express Request. In many deployments, req.log is not a standard property and will be undefined unless a logging middleware augments the request. If req.log is missing, calls like req.log.error(...) will throw a TypeError at runtime, preventing the intended error response from being sent and potentially causing unhandled exceptions. This occurs on multiple lines where logging is used: e.g., at the configuration error catch, missing header case, and invalid header case. [ Low confidence ]

src/notifications/client.ts — 0 comments posted, 2 evaluated, 2 filtered

• line 16: createNotificationClient no longer validates NOTIFICATION_SERVER_URL. With the previous guard removed, createConnectTransport is called with baseUrl set to undefined when the environment variable is missing. This misconfiguration will only surface later at request time with opaque transport errors. Given that createNotificationClient() is invoked at module top-level in multiple places (e.g., src/api/shared/notifications/webhook-handler.ts and src/api/v2/notifications/handlers/*), this violates the invariant that required configuration should be validated pre-effect during initialization. Add an explicit check that NOTIFICATION_SERVER_URL is defined and fail fast before creating the client. [ Invalidated by documentation search ]
• line 226: Potential crash in v2 path if client.device is missing. In handleV2Notification, the code accesses client.device.disabled, and later constructs deviceForApns using multiple properties from client.device. The record is fetched with include: { device: true }, but there is no explicit guarantee that the related device exists. If client.device is null, these property accesses will throw at runtime. Add an explicit presence check (and an error path or cleanup) before dereferencing client.device. [ Low confidence ]

src/utils/v2/jwt.ts — 0 comments posted, 3 evaluated, 3 filtered

• line 19: The JWT payload validation schema (v2JWTPayloadSchema) accepts any non-empty string (including an empty string, since z.string() allows empty by default) for deviceId, while the token generation entrypoint enforces a UUID via generateTokenRequestSchema. This contract mismatch allows verification to accept tokens with a deviceId that would never be produced by the generator (e.g., empty string or non-UUID string), potentially leading to downstream assumptions being violated (handlers might assume deviceId is a UUID). Align the verification schema with generation constraints (e.g., z.string().uuid()). [ Low confidence ]
• line 76: All JWT verification errors are mapped to a 401 Unauthorized (throw new AppError(401, "Invalid or expired token", verifyError)), which conflates malformed/invalid tokens with internal errors (e.g., misconfigured JWT_SECRET_BYTES or jose runtime errors). This can mislead clients and hinder diagnostics; internal errors should be surfaced as 5xx while invalid tokens should remain 401. [ Invalidated by documentation search ]
• line 90: The helper isNotificationExtensionOnlyToken accesses payload.metadata without guarding that payload itself is non-null/defined. The expression payload.metadata?.notificationExtensionOnly === true will throw at runtime if payload is null or undefined. Since TypeScript types are erased at runtime and there is no explicit guard, passing an invalid payload (e.g., from deserialization or an external caller) will cause a crash. A safe alternative is to use payload?.metadata?.notificationExtensionOnly === true or validate payload before property access. [ Invalidated by documentation search ]

Summary by CodeRabbit

• New Features
  • v2 API: token issuance, device register/unregister/subscribe/unsubscribe endpoints, v2 auth and AppCheck, shared webhook mounted for v1/v2.
• Refactor
  • Centralized XMTP notification processing and unified webhook routing; shared notification types.
• Chores
  • DB migrations for device/client models and unique push-token constraint; new config and V2 JWT utilities; dependency reorganization.
• Tests
  • Tests updated to use shared notification modules and include new invite expiry fields.
• Bug Fixes
  • Adjusted API rate limits and auth-specific limiter.

[claude]

Claude finished @lourou's task —— View job

---

Code Review: Backend v2 foundations

I've completed a comprehensive review of PR #141. Overall, this is a well-architected implementation with strong security practices and thoughtful error handling. Below are my findings organized by category.

✅ Strengths

Security

• Excellent authentication architecture: Dual auth middleware supporting both Firebase AppCheck and JWT tokens (src/middleware/v2/auth.ts:40-97)
• Timing-safe webhook validation: Uses crypto.timingSafeEqual() to prevent timing attacks (src/middleware/xmtpWebhookAuth.ts:46-47)
• Fail-closed security model: Rejects requests when secrets are not configured (src/middleware/xmtpWebhookAuth.ts:14-31)
• Input validation: Comprehensive Zod schemas with UUID enforcement and hex validation
• JWT best practices: Short-lived tokens (15m default), metadata size limits (1KB), cached signing keys

Code Quality

• Atomic operations: Transaction-based failure tracking prevents race conditions (src/api/shared/notifications/webhook-handler.ts:316-336)
• Compensating transactions: Subscribe/unsubscribe handlers clean up partial state on errors (src/api/v2/notifications/handlers/subscribe.ts:105-144)
• Smart push token migration: Handles device transfers with unique constraint handling (src/api/v2/device/handlers/register.ts:70-106)
• Welcome message optimization: Detects large XMTP welcome messages to avoid APNS 4KB payload limit (src/api/shared/notifications/webhook-handler.ts:26-31)
• Comprehensive logging: Structured logging with context throughout

Database Design

• Proper cascading deletes: Foreign key constraints ensure data integrity
• Effective indexing: Composite indexes on (disabled, pushFailures) and unique constraint on (pushTokenType, apnsEnv, pushToken)
• Clean separation: V1 and V2 models coexist without conflicts

⚠️ Issues & Recommendations

1. CRITICAL: Missing UUID Validation in Webhook Handler

Location: src/api/shared/notifications/webhook-handler.ts:72-75

const v2Client = await prisma.clientIdentifier.findUnique({
  where: { id: notification.installation.id },  // ❌ No UUID validation
  include: { device: true },
});

Issue: The webhook handler trusts notification.installation.id without validating it's a valid UUID. Since ClientIdentifier.id is a UUID primary key, an invalid format could cause database errors or expose internal error messages.

Recommendation:

// Validate installation.id is a valid UUID before database lookup
const installationId = notification.installation.id;
if (!/^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i.test(installationId)) {
  req.log.warn({ installationId }, "Invalid installation ID format");
  res.status(400).json({ error: "Invalid installation ID format" });
  return;
}

2. Performance: Potential N+1 Query Pattern

Location: src/api/shared/notifications/webhook-handler.ts:72-89 and 92-100

Issue: The webhook handler performs two sequential database queries (v2 lookup, then v1 lookup). While this is by design for fallback logic, the v1 lookup includes two relations that could be optimized.

Current behavior:

• V2 lookup: 1 query with 1 join (efficient)
• V1 fallback: 1 query with 2 joins (device, identity)

Recommendation: Consider caching strategy for high-frequency installations or add database query monitoring to identify hot paths.

3. Error Handling: Inconsistent Cleanup Order

Location: src/api/shared/notifications/webhook-handler.ts:351-367

Issue: During v2 cleanup, the code deletes from local DB first, then attempts XMTP deletion. If XMTP deletion fails, it logs but continues. This is actually correct (DB is authoritative), but the comment could be clearer.

Recommendation: The current approach is good, but consider adding a background job to retry orphaned XMTP installations.

4. Security: JWT Expiration for NSE Could Be Shorter

Location: src/api/shared/notifications/webhook-handler.ts:236-242

const apiJWT = await createV2JwtToken({
  deviceId: client.deviceId,
  expirationTime: "24h",  // ⚠️ Very long for NSE use case
  metadata: { notificationExtensionOnly: true },
});

Issue: Notification Service Extensions (NSE) run briefly when push notifications arrive. A 24-hour JWT expiration is excessive for this use case.

Recommendation: Reduce to 1-2 hours for NSE tokens:

expirationTime: "1h",  // NSE tokens should be short-lived

5. Database Migration: Missing Rollback Strategy

Location: prisma/migrations/20251017193536_add_unique_constraint_to_push_tokens/migration.sql

Issue: The migration adds a unique constraint on (pushTokenType, apnsEnv, pushToken) but doesn't handle edge cases where multiple devices legitimately had the same token (e.g., from buggy client code).

Recommendation: Add pre-migration cleanup to handle duplicates:

-- Identify and clean duplicate push tokens before adding unique constraint
WITH duplicates AS (
  SELECT pushTokenType, apnsEnv, pushToken, 
         MIN(deviceId) as keep_device
  FROM "DeviceRegistration"
  WHERE pushToken IS NOT NULL
  GROUP BY pushTokenType, apnsEnv, pushToken
  HAVING COUNT(*) > 1
)
UPDATE "DeviceRegistration" dr
SET pushToken = NULL
FROM duplicates d
WHERE dr.pushTokenType = d.pushTokenType
  AND dr.apnsEnv = d.apnsEnv
  AND dr.pushToken = d.pushToken
  AND dr.deviceId != d.keep_device;

6. Code Quality: Magic Number in APNS Service

Location: src/api/shared/notifications/services/apns-push.service.ts:43-49

if (this.jwtTokenExpiry && this.jwtTokenExpiry > now + 300) {  // ❌ Magic number
  return this.jwtToken;
}

Recommendation: Extract to named constant:

const APNS_JWT_REFRESH_BUFFER_SECONDS = 300; // Refresh 5 minutes before expiry
if (this.jwtTokenExpiry && this.jwtTokenExpiry > now + APNS_JWT_REFRESH_BUFFER_SECONDS) {

7. Testing: Limited Test Coverage for V2 Endpoints

Observation: Only found tests/invites-v2.test.ts with V2 tests. The new v2 endpoints lack dedicated test coverage:

• /api/v2/auth/generate-token - No tests found
• /api/v2/device/register - No tests found
• /api/v2/notifications/subscribe - No tests found
• /api/v2/notifications/unsubscribe - No tests found
• /api/v2/notifications/unregister/:clientId - No tests found

Recommendation: Add integration tests covering:

• Happy path flows
• Edge cases (disabled devices, missing push tokens, duplicate tokens)
• Error scenarios (invalid UUIDs, missing AppCheck, expired JWTs)
• Concurrent request handling (race conditions)

🔍 Minor Observations

Type Safety Enhancement

Location: src/api/shared/notifications/webhook-handler.ts:280-295

The deviceForApns object uses os: "ios" as const but builds the object manually. Consider creating a type adapter function to ensure consistency with the Device type.

Logging Verbosity

Location: src/api/shared/notifications/services/apns-push.service.ts:141-268

Extensive verbose logging is excellent for debugging but should be toggleable via config in production to avoid log pollution.

Configuration Validation

Location: src/config.ts:8-19

Excellent fail-fast validation. Consider adding validation for APNS credentials if APNS is required (or document that it's optional).

📊 Summary

Category	Status
Security	✅ Excellent
Code Quality	✅ Very Good
Performance	⚠️ Good (minor concerns)
Error Handling	✅ Excellent
Database Design	✅ Very Good
Test Coverage	⚠️ Needs Improvement

🎯 Recommended Actions Before Merge

1. HIGH PRIORITY: Add UUID validation to webhook handler (Issue [Backend] User, Device, and Conversation tables #1)
2. HIGH PRIORITY: Add comprehensive tests for v2 endpoints (Issue Add /profiles endpoint #7)
3. MEDIUM PRIORITY: Reduce NSE JWT expiration from 24h to 1h (Issue Add API endpoints #4)
4. MEDIUM PRIORITY: Extract magic numbers to constants (Issue Refactor user create/update endpoints #6)
5. LOW PRIORITY: Consider query optimization strategy (Issue [Backend] Group Invites #2)

---

Overall Assessment: This PR demonstrates excellent engineering practices and is ready for merge after addressing the UUID validation issue and adding test coverage. The architecture is solid and the code quality is high. Great work! 🎉

[coderabbitai]

Note

Other AI code review bot(s) detected

CodeRabbit 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.

Walkthrough

Centralizes XMTP webhook handling and auth; adds v2 auth/device/notifications APIs and JWT utilities; introduces DeviceRegistration/ClientIdentifier Prisma models and migrations; consolidates config and webhook auth; tightens schemas/types, adjusts rate limits, and updates push-notification flows and imports.

Changes

Cohort / File(s)	Summary
Package & TS config package.json, tsconfig.json	Dependency bumps (@prisma/client/prisma → ^6.17.0); move zod-prisma-types to devDependencies; remove packageManager field; add @prisma-zod/* path alias.
Prisma schema & migrations prisma/schema.prisma, prisma/migrations/.../20251003172324_add_v2_push_notification_models/migration.sql, prisma/migrations/.../20251017193536_add_unique_constraint_to_push_tokens/migration.sql	Add DeviceRegistration and ClientIdentifier models, indices, FK with cascade; make pushToken nullable, clean empty strings to NULL, add unique composite index on (pushTokenType, apnsEnv, pushToken).
Shared notifications core src/api/shared/notifications/constants.ts, src/api/shared/notifications/services/notifications-types.ts, src/api/shared/notifications/webhook-handler.ts, src/api/shared/notifications/webhook.router.ts	Add MAX_PUSH_FAILURES and MAX_JWT_METADATA_SIZE; make payload choose XOR of inboxId/clientId; add V2NotificationPayload and AnyNotificationPayloadWithJWT; implement centralized handleXmtpNotification and webhookRouter with V2/V1 flows, cleanup, and logging.
Notifications client & types src/notifications/client.ts, src/api/shared/notifications/services/notifications-types.ts	Introduce webhookNotificationBodySchema and exported WebhookNotificationBody type; use config NOTIFICATION_SERVER_URL; switch to zod-based webhook validation.
V2 auth & JWT utilities src/middleware/v2/auth.ts, src/utils/v2/jwt.ts, src/api/v2/auth/auth.router.ts, src/api/v2/auth/handlers/generate-token.ts	Add AppCheck & V2 JWT middlewares and headers (AUTH_HEADER, APPCHECK_HEADER); create/verify V2 JWT utilities; add auth router and short-lived generateToken handler.
V2 device & notifications APIs src/api/v2/index.ts, src/api/v2/device/device.router.ts, src/api/v2/device/handlers/register.ts, src/api/v2/notifications/notifications.router.ts, src/api/v2/notifications/handlers/*.ts	Add v2 routers/handlers: device register, subscribe/unsubscribe/unregister; mount auth/app-check middleware and expose /notifications/xmtp via shared webhook router.
V1 refactors & re-exports src/api/v1/index.ts, src/api/v1/notifications/handlers/handle-xmtp-notification.ts, src/api/v1/notifications/notifications.router.ts, src/api/v1/devices/handlers/*.ts, src/api/v1/invites/handlers/*.ts, src/api/v1/notifications/handlers/subscribe-to-topics.ts	Centralize XMTP webhook implementation into shared handler (v1 file now re-exports); replace generated Zod imports with @prisma-zod; tighten hmac key validation to hex; remove legacy xmtp router export.
Config & global flags src/config.ts, src/index.ts, src/instrumentation.ts, src/middleware/errorHandler.ts, src/routes/healthcheck.ts, src/notifications/utils.ts	Add validated config exports (XMTP_NOTIFICATION_SECRET, JWT_SECRET, NOTIFICATION_SERVER_URL, JWT_SECRET_BYTES, NODE_ENV flags); replace direct NODE_ENV checks with IS_DEVELOPMENT/IS_PRODUCTION; use NOTIFICATION_SERVER_URL.
Webhook auth & rate limiting src/middleware/xmtpWebhookAuth.ts, src/middleware/rateLimit.ts, src/globals.d.ts	Add xmtpWebhookAuthMiddleware (timing-safe header check); add authRateLimitMiddleware (100 per 5m); increase general rateLimit to 1000 per 5m; extend Express.Locals with v2 fields (deviceId, jwtMetadata).
Push notification services src/api/shared/notifications/services/push-notification.service.ts, src/api/shared/notifications/services/apns-push.service.ts	Guard v1 notifications to require inboxId (early-return if missing); APNS service accepts AnyNotificationPayloadWithJWT and redacts Authorization in logs.
API wiring & routers src/api/v1/index.ts, src/api/v2/index.ts, various router files	Wire shared webhookRouter into v1/v2 mounts; add v2 auth/device/notifications mounts; remove legacy xmtp router export from v1 notifications router.
Scripts & tests scripts/*.ts, tests/*.test.ts	Update import paths to shared notification modules and @prisma-zod alias; test assertions adjusted for new invite fields; no core behavioral changes in tests/scripts.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant XMTP as XMTP Server
  participant Router as webhookRouter
  participant Auth as xmtpWebhookAuthMiddleware
  participant Handler as handleXmtpNotification
  participant DB as Prisma
  participant APNS as APNS Service

  XMTP->>Router: POST /handle-notification
  Router->>Auth: Validate auth header
  alt invalid
    Auth-->>XMTP: 401
  else valid
    Auth->>Handler: handleXmtpNotification(body)
    Handler->>DB: lookup by clientId (v2) or identitiesOnDevice (v1)
    alt v2 client
      Handler->>Handler: createV2JwtToken()
      Handler->>APNS: send V2 APNS payload
      alt success
        Handler->>DB: reset pushFailures, set lastSentAt
      else failure
        Handler->>DB: increment pushFailures / disable / cleanup
      end
    else v1 fallback
      Handler->>APNS: send v1 APNS payload (requires inboxId)
      opt repeated failures
        Handler->>DB: cleanupFailedInstallation()
      end
    end
    Handler-->>XMTP: 200 / error
  end

Loading

sequenceDiagram
  autonumber
  participant Client
  participant API as /v2/auth/token
  participant Rate as authRateLimitMiddleware
  participant Auth as authV2Middleware
  participant Handler as generateToken
  participant JWT as createV2JwtToken
  participant DB as Prisma

  Client->>API: POST { deviceId }
  API->>Rate: check rate limit
  API->>Auth: AppCheck or JWT verification
  Auth-->>API: next()
  API->>Handler: generateToken(req)
  Handler->>DB: find DeviceRegistration
  alt not found/disabled
    Handler-->>Client: 404 / 403
  else ok
    Handler->>JWT: sign token (15m)
    Handler-->>Client: 200 { token }
  end

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~75 minutes

Possibly related PRs

• fix: Put back new push notification registering system #72 — overlaps XMTP webhook handling, centralized handler, and installation cleanup logic.
• Nmalzieu/notifications types #119 — overlaps changes to notification payload types and APNS/push-service interfaces.
• Implement invites v2 #140 — touches the same invite protobuf additions and decoding changes.

Suggested reviewers

• neekolas
• rygine

Poem

I nibble on code beneath the sun,
Webhooks hop and join as one.
Tokens, devices, JWTs in tow,
Prisma burrows help them grow.
Hooray — notifications ready to go! 🐇✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title Check	✅ Passed	The title "Backend v2 foundations" is directly related to the main change in the changeset, which introduces foundational support for v2 across the application. The PR establishes new v2 API structures, authentication mechanisms, device registration, push notification webhooks, and supporting database models. The title accurately captures this primary theme of establishing v2 infrastructure. While the title is somewhat broad and does not detail specific domains like notifications or authentication, it is clear and specific enough that a teammate scanning the git history would understand this PR is about creating foundational v2 backend architecture. The title appropriately avoids false specificity while conveying the main purpose.
Docstring Coverage	✅ Passed	No functions found in the changes. Docstring coverage check skipped.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch lr/v2-backend-foundations

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 6

🧹 Nitpick comments (14)

src/api/shared/notifications/services/notifications-types.ts (1)

58-64: Consider making V2NotificationPayload more extensible.

The notificationType field is hardcoded to the string literal "Protocol", which limits future extensibility if additional notification types need to be supported in V2. While this is acceptable for foundation work, consider one of the following approaches:

• Use a type parameter to make it generic: V2NotificationPayload<T extends NotificationType>
• Create a discriminated union similar to the V1 approach when adding support for other notification types
• Document the limitation explicitly if V2 will only support Protocol notifications

Example of a more extensible approach:

-// V2 notification types
-export type V2NotificationPayload = {
-  clientIdentifier: string;
-  apiJWT: string;
-  notificationType: "Protocol";
-  notificationData: ProtocolNotificationData;
-};
+// V2 notification types (currently only Protocol is supported)
+type V2NotificationPayloadBase<T extends NotificationType> = {
+  clientIdentifier: string;
+  apiJWT: string;
+  notificationType: T;
+  notificationData: NotificationTypeToData[T];
+};
+
+// For now, only Protocol is supported in V2
+export type V2NotificationPayload = V2NotificationPayloadBase<"Protocol">;

src/api/v2/auth/auth.router.ts (2)

7-8: Clarify the comment about authentication requirements.

The comment states "Token generation requires AppCheck (main app only)" but the authV2Middleware (referenced in the relevant code snippets) accepts both AppCheck tokens and V2 JWT tokens. This could confuse developers about who can call this endpoint.

Consider updating the comment to reflect the actual authentication options:

-// Token generation requires AppCheck (main app only)
+// Token generation requires authentication (AppCheck for main app, or V2 JWT for NSE/Gateway)
 authRouter.post("/token", authV2Middleware, generateToken);

---

1-10: Consider rate limiting for token generation.

The /token endpoint generates authentication tokens, which is a security-sensitive operation. While the tokens are short-lived (15m), the endpoint should have rate limiting to prevent abuse such as token farming or brute force attempts.

Consider adding rate limiting middleware before the auth middleware:

import rateLimit from "express-rate-limit";

const tokenRateLimiter = rateLimit({
  windowMs: 15 * 60 * 1000, // 15 minutes
  max: 100, // limit each IP/client to 100 requests per windowMs
  message: "Too many token requests, please try again later"
});

authRouter.post("/token", tokenRateLimiter, authV2Middleware, generateToken);

src/api/v2/notifications/handlers/unregister.ts (1)

31-39: Consider reordering operations or adding error compensation.

The current flow deletes from the notification server (line 32-34) before deleting from the database (line 37-39). If the database delete fails after a successful notification server delete, the client record becomes orphaned in the database while being removed from the notification server, requiring manual cleanup.

Consider reversing the order (delete from database first, then notification server) to make failures more gracefully recoverable. Alternatively, implement compensating logic to re-register with the notification server if the database delete fails.

Example reordering:

-    // Delete installation from notification server
-    await notificationClient.deleteInstallation({
-      installationId: params.clientIdentifier,
-    });
-
     // Delete client from database
     await prisma.clientIdentifier.delete({
       where: { id: params.clientIdentifier },
     });
+
+    // Delete installation from notification server
+    await notificationClient.deleteInstallation({
+      installationId: params.clientIdentifier,
+    });

src/utils/v2/jwt.ts (1)

54-72: Consider validating payload structure after JWT verification.

Line 71 casts the verified payload to V2JWTPayload without runtime validation. While tokens created by createV2JwtToken will have the correct structure, adding Zod validation would provide defense-in-depth against malformed tokens or future schema changes.

Example validation:

import { z } from "zod";

const v2JWTPayloadSchema = z.object({
  clientIdentifier: z.string(),
  deviceId: z.string(),
  metadata: z.object({
    notificationExtensionOnly: z.boolean().optional(),
    gatewayAuthorized: z.boolean().optional(),
  }).optional(),
});

// In verifyV2JwtToken:
const validatedPayload = v2JWTPayloadSchema.parse(verified.payload);
return validatedPayload;

src/middleware/v2/auth.ts (1)

16-27: Redundant return statement after next() (optional nitpick).

Lines 20-21 call next() followed immediately by return. While not harmful, the explicit return is unnecessary since next() transfers control. The same pattern appears in lines 37-38.

Optional simplification:

     try {
       await verifyAppCheckToken(appCheckToken);
-      next();
-      return;
+      return next();
     } catch (error) {

src/api/v2/notifications/handlers/register.ts (2)

6-11: Move defaults into the Zod schema and require apnsEnv when tokenType is 'apns'

Simplifies handler logic and prevents invalid APNS registrations lacking environment. Based on learnings and coding guidelines.

-const registerRequestSchema = z.object({
-  deviceId: z.string(),
-  pushToken: z.string(),
-  tokenType: PushTokenTypeSchema.optional(),
-  apnsEnv: ApnsEnvironmentSchema.nullable().optional(),
-});
+const registerRequestSchema = z
+  .object({
+    deviceId: z.string(),
+    pushToken: z.string(),
+    tokenType: PushTokenTypeSchema.default("apns"),
+    apnsEnv: ApnsEnvironmentSchema.nullable().optional(),
+  })
+  .refine(
+    (v) => v.tokenType !== "apns" || (v.apnsEnv !== null && v.apnsEnv !== undefined),
+    { path: ["apnsEnv"], message: "apnsEnv is required when tokenType is 'apns'" },
+  );

       create: {
         deviceId: body.deviceId,
         pushToken: body.pushToken,
-        tokenType: body.tokenType ?? "apns",
-        apnsEnv: body.apnsEnv ?? null,
+        tokenType: body.tokenType,
+        apnsEnv: body.apnsEnv ?? null,
       },
       update: {
         pushToken: body.pushToken,
-        tokenType: body.tokenType ?? "apns",
-        apnsEnv: body.apnsEnv ?? null,
-        updatedAt: new Date(),
+        tokenType: body.tokenType,
+        apnsEnv: body.apnsEnv ?? null,
       },

Also applies to: 27-34

---

30-35: Remove manual updatedAt assignment

Prisma @updatedat auto-manages this. Cleaner and less error-prone. As per coding guidelines.

       update: {
         pushToken: body.pushToken,
         tokenType: body.tokenType ?? "apns",
         apnsEnv: body.apnsEnv ?? null,
-        updatedAt: new Date(),
       },

src/api/v2/notifications/handlers/subscribe.ts (3)

79-89: Remove redundant upsert (record already verified to exist)

You 404 when client is missing; this upsert’s create path is unreachable and the update only bumps updatedAt. Prefer removing the DB call.

-    // Create or update client identifier record
-    await prisma.clientIdentifier.upsert({
-      where: { id: body.clientIdentifier },
-      create: {
-        id: body.clientIdentifier,
-        deviceId: client.deviceId,
-      },
-      update: {
-        updatedAt: new Date(),
-      },
-    });
+    // No-op: client identifier already exists and is associated with the device

---

7-20: Validate HMAC key format (hex) at the schema level

Fail early with clear errors; avoids runtime conversion errors. As per coding guidelines.

 const subscribeRequestSchema = z.object({
   clientIdentifier: z.string(),
   topics: z.array(
     z.object({
       topic: z.string(),
       hmacKeys: z.array(
         z.object({
           thirtyDayPeriodsSinceEpoch: z.number(),
-          key: z.string(),
+          key: z.string().regex(/^[0-9a-fA-F]+$/, "hmacKeys.key must be hex"),
         }),
       ),
     }),
   ),
 });

Also applies to: 12-16

---

24-24: Consider lazy-initializing the notification client

Creating the client at module load will throw if NOTIFICATION_SERVER_URL is unset, crashing startup. Lazy init inside the handler avoids global side effects.

prisma/schema.prisma (1)

181-205: Tighten uniqueness and query performance for v2 models

• Prevent duplicate registrations for the same token across env/type.
• Future-proof querying disabled/failing devices.

 model DeviceRegistration {
   deviceId          String             @id
   pushToken         String             @db.Text
   tokenType         PushTokenType      @default(apns)
   apnsEnv           ApnsEnvironment?
   pushFailures      Int                @default(0)
   disabled          Boolean            @default(false)
   addedAt           DateTime           @default(now())
   updatedAt         DateTime           @updatedAt
   lastSentAt        DateTime?
   lastFailureAt     DateTime?
   clientIdentifiers ClientIdentifier[]
 
   @@index([pushToken])
+  // Prevent duplicate token registrations within the same delivery context
+  @@unique([tokenType, pushToken, apnsEnv])
+  // Optional: speed up maintenance scans
+  @@index([disabled, pushFailures])
 }

src/api/shared/notifications/webhook-handler.ts (2)

49-59: Use timing-safe comparison for the auth header

Avoid leaking timing information on auth checks. Minor hardening.

-    const authHeader = req.headers.authorization;
-    const expectedAuthHeader = getHttpDeliveryNotificationAuthHeader();
-
-    if (!authHeader || authHeader !== expectedAuthHeader) {
+    const authHeader = req.headers.authorization ?? "";
+    const expectedAuthHeader = getHttpDeliveryNotificationAuthHeader();
+    const a = Buffer.from(authHeader);
+    const b = Buffer.from(expectedAuthHeader);
+    const ok = a.length === b.length && crypto.timingSafeEqual(a, b);
+    if (!ok) {
       req.log.error("Invalid or missing authorization header");
       res.status(401).json({
         error: "Unauthorized: Invalid authentication token",
       });
       return;
     }

---

37-47: Validate webhook body structure

Parse and validate the webhook payload (e.g., with Zod) before use to return 400 on bad input instead of 500. Keeps handler robust to upstream changes.

Also applies to: 61-66, 78-87, 108-121, 218-229

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1f16a09 and f642613.

⛔ Files ignored due to path filters (1)

• bun.lock is excluded by !**/*.lock

📒 Files selected for processing (29)

• package.json (2 hunks)
• prisma/migrations/20251003172324_add_v2_push_notification_models/migration.sql (1 hunks)
• prisma/schema.prisma (1 hunks)
• scripts/simple-push-test.ts (1 hunks)
• scripts/test-push-notification.ts (1 hunks)
• src/api/shared/notifications/services/notifications-types.ts (1 hunks)
• src/api/shared/notifications/webhook-handler.ts (1 hunks)
• src/api/shared/notifications/webhook.router.ts (1 hunks)
• src/api/v1/devices/handlers/create-device.handler.ts (1 hunks)
• src/api/v1/devices/handlers/update-device.handler.ts (1 hunks)
• src/api/v1/index.ts (2 hunks)
• src/api/v1/invites/handlers/request-to-join.ts (1 hunks)
• src/api/v1/invites/handlers/update-invite-code.ts (1 hunks)
• src/api/v1/notifications/handlers/handle-xmtp-notification.ts (1 hunks)
• src/api/v1/notifications/handlers/register-installation.ts (1 hunks)
• src/api/v1/notifications/notifications.router.ts (1 hunks)
• src/api/v2/auth/auth.router.ts (1 hunks)
• src/api/v2/auth/handlers/generate-token.ts (1 hunks)
• src/api/v2/index.ts (1 hunks)
• src/api/v2/notifications/handlers/register.ts (1 hunks)
• src/api/v2/notifications/handlers/subscribe.ts (1 hunks)
• src/api/v2/notifications/handlers/unregister.ts (1 hunks)
• src/api/v2/notifications/handlers/unsubscribe.ts (1 hunks)
• src/api/v2/notifications/notifications.router.ts (1 hunks)
• src/middleware/v2/auth.ts (1 hunks)
• src/utils/v2/jwt.ts (1 hunks)
• tests/notifications-invites.test.ts (1 hunks)
• tests/notifications-service.test.ts (1 hunks)
• tsconfig.json (1 hunks)

🧰 Additional context used

📓 Path-based instructions (12)

**/*

📄 CodeRabbit inference engine (.cursorrules)

Follow consistent file naming (kebab-case)

Files:

• tsconfig.json
• tests/notifications-service.test.ts
• src/api/v1/devices/handlers/update-device.handler.ts
• src/api/v1/index.ts
• src/api/v2/notifications/handlers/unregister.ts
• src/api/v2/auth/auth.router.ts
• src/api/shared/notifications/services/notifications-types.ts
• prisma/migrations/20251003172324_add_v2_push_notification_models/migration.sql
• src/api/v1/notifications/notifications.router.ts
• src/utils/v2/jwt.ts
• src/api/shared/notifications/webhook.router.ts
• scripts/test-push-notification.ts
• src/middleware/v2/auth.ts
• src/api/v1/notifications/handlers/register-installation.ts
• src/api/v2/notifications/handlers/unsubscribe.ts
• src/api/v1/devices/handlers/create-device.handler.ts
• package.json
• prisma/schema.prisma
• src/api/v1/notifications/handlers/handle-xmtp-notification.ts
• tests/notifications-invites.test.ts
• scripts/simple-push-test.ts
• src/api/v2/index.ts
• src/api/v2/notifications/handlers/register.ts
• src/api/v2/auth/handlers/generate-token.ts
• src/api/shared/notifications/webhook-handler.ts
• src/api/v2/notifications/notifications.router.ts
• src/api/v2/notifications/handlers/subscribe.ts
• src/api/v1/invites/handlers/update-invite-code.ts
• src/api/v1/invites/handlers/request-to-join.ts

**/tsconfig*.json

📄 CodeRabbit inference engine (.cursorrules)

Use TypeScript path aliases

Files:

• tsconfig.json

**/*.{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:

• tests/notifications-service.test.ts
• src/api/v1/devices/handlers/update-device.handler.ts
• src/api/v1/index.ts
• src/api/v2/notifications/handlers/unregister.ts
• src/api/v2/auth/auth.router.ts
• src/api/shared/notifications/services/notifications-types.ts
• src/api/v1/notifications/notifications.router.ts
• src/utils/v2/jwt.ts
• src/api/shared/notifications/webhook.router.ts
• scripts/test-push-notification.ts
• src/middleware/v2/auth.ts
• src/api/v1/notifications/handlers/register-installation.ts
• src/api/v2/notifications/handlers/unsubscribe.ts
• src/api/v1/devices/handlers/create-device.handler.ts
• src/api/v1/notifications/handlers/handle-xmtp-notification.ts
• tests/notifications-invites.test.ts
• scripts/simple-push-test.ts
• src/api/v2/index.ts
• src/api/v2/notifications/handlers/register.ts
• src/api/v2/auth/handlers/generate-token.ts
• src/api/shared/notifications/webhook-handler.ts
• src/api/v2/notifications/notifications.router.ts
• src/api/v2/notifications/handlers/subscribe.ts
• src/api/v1/invites/handlers/update-invite-code.ts
• src/api/v1/invites/handlers/request-to-join.ts

**/*.{test,spec}.{ts,tsx}

📄 CodeRabbit inference engine (.cursorrules)

**/*.{test,spec}.{ts,tsx}: Write tests for API endpoints
Use helper functions for test setup
Mock external services
Test error cases
Follow AAA pattern (Arrange, Act, Assert)

Files:

• tests/notifications-service.test.ts
• tests/notifications-invites.test.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/v1/devices/handlers/update-device.handler.ts
• src/api/v1/index.ts
• src/api/v2/notifications/handlers/unregister.ts
• src/api/v2/auth/auth.router.ts
• src/api/shared/notifications/services/notifications-types.ts
• src/api/v1/notifications/notifications.router.ts
• src/api/shared/notifications/webhook.router.ts
• src/api/v1/notifications/handlers/register-installation.ts
• src/api/v2/notifications/handlers/unsubscribe.ts
• src/api/v1/devices/handlers/create-device.handler.ts
• src/api/v1/notifications/handlers/handle-xmtp-notification.ts
• src/api/v2/index.ts
• src/api/v2/notifications/handlers/register.ts
• src/api/v2/auth/handlers/generate-token.ts
• src/api/shared/notifications/webhook-handler.ts
• src/api/v2/notifications/notifications.router.ts
• src/api/v2/notifications/handlers/subscribe.ts
• src/api/v1/invites/handlers/update-invite-code.ts
• src/api/v1/invites/handlers/request-to-join.ts

**/handlers/**/*.{ts,tsx}

📄 CodeRabbit inference engine (.cursorrules)

Use dedicated handlers directory for route logic

Files:

• src/api/v1/devices/handlers/update-device.handler.ts
• src/api/v2/notifications/handlers/unregister.ts
• src/api/v1/notifications/handlers/register-installation.ts
• src/api/v2/notifications/handlers/unsubscribe.ts
• src/api/v1/devices/handlers/create-device.handler.ts
• src/api/v1/notifications/handlers/handle-xmtp-notification.ts
• src/api/v2/notifications/handlers/register.ts
• src/api/v2/auth/handlers/generate-token.ts
• src/api/v2/notifications/handlers/subscribe.ts
• src/api/v1/invites/handlers/update-invite-code.ts
• src/api/v1/invites/handlers/request-to-join.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/v1/devices/handlers/update-device.handler.ts
• src/api/v1/index.ts
• src/api/v2/notifications/handlers/unregister.ts
• src/api/v2/auth/auth.router.ts
• src/api/shared/notifications/services/notifications-types.ts
• src/api/v1/notifications/notifications.router.ts
• src/api/shared/notifications/webhook.router.ts
• src/api/v1/notifications/handlers/register-installation.ts
• src/api/v2/notifications/handlers/unsubscribe.ts
• src/api/v1/devices/handlers/create-device.handler.ts
• src/api/v1/notifications/handlers/handle-xmtp-notification.ts
• src/api/v2/index.ts
• src/api/v2/notifications/handlers/register.ts
• src/api/v2/auth/handlers/generate-token.ts
• src/api/shared/notifications/webhook-handler.ts
• src/api/v2/notifications/notifications.router.ts
• src/api/v2/notifications/handlers/subscribe.ts
• src/api/v1/invites/handlers/update-invite-code.ts
• src/api/v1/invites/handlers/request-to-join.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/v1/devices/handlers/update-device.handler.ts
• src/api/v1/index.ts
• src/api/v2/notifications/handlers/unregister.ts
• src/api/v2/auth/auth.router.ts
• src/api/shared/notifications/services/notifications-types.ts
• src/api/v1/notifications/notifications.router.ts
• src/api/shared/notifications/webhook.router.ts
• src/middleware/v2/auth.ts
• src/api/v1/notifications/handlers/register-installation.ts
• src/api/v2/notifications/handlers/unsubscribe.ts
• src/api/v1/devices/handlers/create-device.handler.ts
• src/api/v1/notifications/handlers/handle-xmtp-notification.ts
• src/api/v2/index.ts
• src/api/v2/notifications/handlers/register.ts
• src/api/v2/auth/handlers/generate-token.ts
• src/api/shared/notifications/webhook-handler.ts
• src/api/v2/notifications/notifications.router.ts
• src/api/v2/notifications/handlers/subscribe.ts
• src/api/v1/invites/handlers/update-invite-code.ts
• src/api/v1/invites/handlers/request-to-join.ts

**/{api,routes, middleware}/**/*.{ts,tsx}

📄 CodeRabbit inference engine (.cursorrules)

Validate and sanitize inputs

Files:

• src/api/v1/devices/handlers/update-device.handler.ts
• src/api/v1/index.ts
• src/api/v2/notifications/handlers/unregister.ts
• src/api/v2/auth/auth.router.ts
• src/api/shared/notifications/services/notifications-types.ts
• src/api/v1/notifications/notifications.router.ts
• src/api/shared/notifications/webhook.router.ts
• src/api/v1/notifications/handlers/register-installation.ts
• src/api/v2/notifications/handlers/unsubscribe.ts
• src/api/v1/devices/handlers/create-device.handler.ts
• src/api/v1/notifications/handlers/handle-xmtp-notification.ts
• src/api/v2/index.ts
• src/api/v2/notifications/handlers/register.ts
• src/api/v2/auth/handlers/generate-token.ts
• src/api/shared/notifications/webhook-handler.ts
• src/api/v2/notifications/notifications.router.ts
• src/api/v2/notifications/handlers/subscribe.ts
• src/api/v1/invites/handlers/update-invite-code.ts
• src/api/v1/invites/handlers/request-to-join.ts

**/index.ts

📄 CodeRabbit inference engine (.cursorrules)

Use index.ts files for exports

Files:

• src/api/v1/index.ts
• src/api/v2/index.ts

**/middleware/**/*.{ts,tsx}

📄 CodeRabbit inference engine (.cursorrules)

Keep middleware in dedicated directory

Files:

• src/middleware/v2/auth.ts

**/*.prisma

📄 CodeRabbit inference engine (.cursorrules)

**/*.prisma: Follow schema naming conventions (PascalCase for models)
Use appropriate field types and constraints
Add indexes for frequently queried fields
Use appropriate database indexes

Files:

• prisma/schema.prisma

🧠 Learnings (2)

📚 Learning: 2025-08-05T14:39:39.983Z

Learnt from: CR
PR: ephemeraHQ/convos-backend#0
File: .cursorrules:0-0
Timestamp: 2025-08-05T14:39:39.983Z
Learning: Applies to **/tsconfig*.json : Use TypeScript path aliases

Applied to files:

• tsconfig.json

📚 Learning: 2025-08-05T14:39:39.983Z

Learnt from: CR
PR: ephemeraHQ/convos-backend#0
File: .cursorrules:0-0
Timestamp: 2025-08-05T14:39:39.983Z
Learning: Applies to **/*.{ts,tsx} : Leverage path aliases (@/) for imports

Applied to files:

• tsconfig.json

🧬 Code graph analysis (13)

src/api/v1/index.ts (1)

src/api/shared/notifications/webhook.router.ts (1)

• webhookRouter (9-9)

src/api/v2/notifications/handlers/unregister.ts (2)

src/notifications/client.ts (1)

• createNotificationClient (12-21)

src/utils/prisma.ts (1)

• prisma (3-3)

src/api/v2/auth/auth.router.ts (2)

src/middleware/v2/auth.ts (1)

• authV2Middleware (8-47)

src/api/v2/auth/handlers/generate-token.ts (1)

• generateToken (15-58)

src/utils/v2/jwt.ts (2)

src/utils/errors.ts (1)

• AppError (1-11)

src/utils/try-catch.ts (1)

• tryCatch (14-23)

src/api/shared/notifications/webhook.router.ts (1)

src/api/shared/notifications/webhook-handler.ts (1)

• handleXmtpNotification (31-141)

src/middleware/v2/auth.ts (2)

src/utils/firebase.ts (1)

• verifyAppCheckToken (36-40)

src/utils/v2/jwt.ts (1)

• verifyV2JwtToken (54-72)

src/api/v2/notifications/handlers/unsubscribe.ts (2)

src/notifications/client.ts (1)

• createNotificationClient (12-21)

src/utils/prisma.ts (1)

• prisma (3-3)

src/api/v2/index.ts (4)

src/api/v2/auth/auth.router.ts (1)

• authRouter (10-10)

src/api/v1/notifications/notifications.router.ts (1)

• notificationsRouter (17-17)

src/api/v2/notifications/notifications.router.ts (1)

• notificationsRouter (20-20)

src/api/shared/notifications/webhook.router.ts (1)

• webhookRouter (9-9)

src/api/v2/notifications/handlers/register.ts (1)

src/utils/prisma.ts (1)

• prisma (3-3)

src/api/v2/auth/handlers/generate-token.ts (2)

src/utils/prisma.ts (1)

• prisma (3-3)

src/utils/v2/jwt.ts (1)

• createV2JwtToken (17-52)

src/api/shared/notifications/webhook-handler.ts (7)

src/notifications/client.ts (2)

• createNotificationClient (12-21)
• WebhookNotificationBody (28-51)

src/api/shared/notifications/services/push-notification.service.ts (1)

• getPushNotificationService (224-229)

src/notifications/utils.ts (1)

• getHttpDeliveryNotificationAuthHeader (1-6)

src/utils/prisma.ts (1)

• prisma (3-3)

src/utils/v2/jwt.ts (1)

• createV2JwtToken (17-52)

src/api/shared/notifications/services/apns-push.service.ts (1)

• createApnsService (272-294)

src/api/shared/notifications/services/notifications-types.ts (2)

• V2NotificationPayload (59-64)
• NotificationPayloadWithJWTToken (54-56)

src/api/v2/notifications/notifications.router.ts (5)

src/middleware/v2/auth.ts (1)

• authV2Middleware (8-47)

src/api/v2/notifications/handlers/register.ts (1)

• register (15-47)

src/api/v2/notifications/handlers/subscribe.ts (1)

• subscribe (26-100)

src/api/v2/notifications/handlers/unsubscribe.ts (1)

• unsubscribe (15-47)

src/api/v2/notifications/handlers/unregister.ts (1)

• unregister (14-50)

src/api/v2/notifications/handlers/subscribe.ts (2)

src/notifications/client.ts (1)

• createNotificationClient (12-21)

src/utils/prisma.ts (1)

• prisma (3-3)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)

• GitHub Check: Typecheck, format check, lint
• GitHub Check: claude-review
• GitHub Check: push_to_registry
• GitHub Check: Review for correctness

🔇 Additional comments (25)

scripts/simple-push-test.ts (1)

2-2: LGTM!

The import path update correctly references the shared APNS service location, aligning with the PR's consolidation of notification services.

src/api/v1/invites/handlers/request-to-join.ts (1)

4-5: LGTM!

The import paths correctly use the @/ alias to reference shared notification types and services, aligning with the project's coding guidelines and the PR's consolidation strategy.

tsconfig.json (1)

12-15: LGTM!

The new @prisma-zod/* path alias correctly maps to the Prisma-generated Zod schemas directory, enabling cleaner imports throughout the codebase. This aligns with the project's coding guidelines for using path aliases.

src/api/v1/invites/handlers/update-invite-code.ts (1)

1-1: LGTM!

The import correctly uses the new @prisma-zod/* path alias, eliminating the relative path traversal and improving code maintainability.

src/api/shared/notifications/webhook.router.ts (1)

1-9: LGTM!

The shared webhook router is correctly structured. Note that authentication is handled within the handleXmtpNotification handler itself (via authorization header verification) rather than through Express middleware, which is an appropriate pattern for webhook endpoints.

src/api/v1/index.ts (2)

2-2: LGTM!

The imports correctly reference the shared webhook router and the updated notifications router export, aligning with the PR's consolidation of webhook handling.

Also applies to: 5-5

---

25-25: LGTM!

The routing correctly delegates to the shared webhookRouter while maintaining the /notifications/xmtp endpoint path for backward compatibility. The webhook route correctly appears before the authenticated notifications routes, ensuring proper request handling.

src/api/v1/notifications/notifications.router.ts (1)

17-17: Safe removal: no remaining usages of xmtpNotificationsRouter
A global search for xmtpNotificationsRouter yielded no imports or references in the codebase, confirming the breaking change is safe.

src/api/v1/notifications/handlers/handle-xmtp-notification.ts (1)

1-3: LGTM!

Clean deprecation pattern with clear comments and backwards compatibility via re-export. This follows best practices for transitioning to shared infrastructure.

scripts/test-push-notification.ts (1)

2-2: LGTM!

Import path updated to align with the shared notification infrastructure. No functional changes.

tests/notifications-service.test.ts (2)

16-16: LGTM!

Import paths updated to use shared notification types and services. Test logic remains unchanged.

---

20-20: LGTM!

Import path updated to align with the shared notification infrastructure.

src/api/v1/devices/handlers/update-device.handler.ts (1)

1-1: LGTM - Verify path alias configuration.

Import updated to use the @prisma-zod/index path alias. Same consideration as in create-device.handler.ts - ensure the path alias is properly configured.

src/api/v1/devices/handlers/create-device.handler.ts (1)

1-1: Verify @prisma-zod alias and generated files existence.
Import updated to @prisma-zod/index, but prisma/generated/zod/index.ts wasn’t found. Confirm the alias target directory exists and that DeviceSchema is exported from the generated index.

src/api/v2/index.ts (1)

10-14: No conflicting routes between /notifications and /notifications/xmtp. The notificationsRouter only defines explicit endpoints and will not intercept /notifications/xmtp requests.

src/api/v1/notifications/handlers/register-installation.ts (1)

1-1: LGTM! Path alias improves maintainability.

The change from a deep relative path to the @prisma-zod/index path alias improves code maintainability and aligns with the tsconfig updates in this PR.

prisma/migrations/20251003172324_add_v2_push_notification_models/migration.sql (1)

1-34: LGTM! Migration structure follows best practices.

The migration correctly establishes:

• Two tables with appropriate primary keys and constraints
• Foreign key relationship with CASCADE for automatic cleanup
• Indices on likely query columns (pushToken, deviceId)
• Timestamp defaults aligned with Prisma conventions

tests/notifications-invites.test.ts (1)

19-22: LGTM! Import reorganization supports v1/v2 code sharing.

The migration to shared notification paths enables code reuse while maintaining test behavior. This aligns with the PR's objective to establish shared notification infrastructure.

src/api/v2/notifications/notifications.router.ts (1)

1-20: LGTM! Clean router structure with consistent authentication.

The router follows RESTful principles and applies authV2Middleware consistently across all endpoints. The separation of handlers into dedicated files aligns with coding guidelines.

src/api/v2/notifications/handlers/unregister.ts (1)

12-12: Module-level client instantiation is acceptable.

The notification client is instantiated at module load time (line 12), which will cause an early failure if NOTIFICATION_SERVER_URL is not configured. This fail-fast behavior is appropriate for required configuration.

src/api/v2/notifications/handlers/unsubscribe.ts (1)

15-47: LGTM! Unsubscribe handler follows correct pattern.

The handler appropriately validates input, looks up the client, and calls the notification service. The logic correctly leaves database state unchanged since unsubscribe is a reversible operation.

src/utils/v2/jwt.ts (2)

17-52: LGTM! JWT creation follows security best practices.

The token creation function correctly:

• Validates required environment configuration
• Uses HS256 for symmetric signing
• Sets reasonable expiration (15 minutes default)
• Handles errors gracefully with structured error wrapping

---

74-76: LGTM! Helper function is simple and correct.

The boolean check safely handles undefined metadata by using strict equality with true.

src/middleware/v2/auth.ts (2)

8-47: LGTM! Auth middleware follows secure patterns.

The middleware correctly:

• Prioritizes AppCheck (main app) over JWT (NSE/Gateway)
• Validates tokens before granting access
• Logs authentication failures
• Returns appropriate 401 responses
• Stores JWT payload in res.locals for handlers

---

16-43: Drop AppCheck res.locals warning for v2: v2 handlers don’t reference res.locals.clientIdentifier, deviceId, or jwtMetadata, so there’s no undefined-field risk.

Likely an incorrect or invalid review comment.

[coderabbitai]

Actionable comments posted: 3

♻️ Duplicate comments (4)

src/api/v2/auth/handlers/generate-token.ts (1)

65-65: Add explicit return after res.json.

Follow the "return immediately after responding" rule to prevent accidental double-sends. As per coding guidelines.

     res.json({ token });
+    return;

src/api/v2/notifications/handlers/subscribe.ts (2)

59-77: Add compensating cleanup for partial failures.

If registerInstallation succeeds but subscribeWithMetadata fails (or if the subsequent DB upsert fails), the installation on the notification server becomes orphaned, leading to state inconsistency.

This issue was previously flagged. Consider wrapping the registration and subscription in a try-catch block with compensating deletion:

+    try {
       await notificationClient.registerInstallation({
         installationId: body.clientIdentifier,
         deliveryMechanism: {
           deliveryMechanismType: {
             case:
               client.device.tokenType === "apns"
                 ? "apnsDeviceToken"
                 : "firebaseDeviceToken",
             value: client.device.pushToken,
           },
         },
       });
 
       await notificationClient.subscribeWithMetadata({
         installationId: body.clientIdentifier,
         subscriptions,
       });
+    } catch (remoteErr) {
+      // Compensate: delete installation to avoid orphaned state
+      try {
+        await notificationClient.deleteInstallation({
+          installationId: body.clientIdentifier,
+        });
+      } catch {}
+      throw remoteErr;
+    }

---

91-91: Add explicit return after response.

As per coding guidelines, add an explicit return; statement after sending the response:

     res.status(200).send();
+    return;

src/api/shared/notifications/webhook-handler.ts (1)

210-250: Route by tokenType to support both APNS and FCM.

The v2 handler hardcodes APNS service creation and sets os: "ios", preventing FCM/Android push notifications from being delivered. This blocks v2 functionality for non-iOS devices.

As previously noted, modify the code to inspect client.device.tokenType and route to the appropriate push service:

-  // Create APNS service
-  const apnsService = createApnsService();
-
-  if (!apnsService) {
-    req.log.error("APNS service not configured");
-    return { success: false };
-  }
+  // Use shared push service to support both APNS and FCM
+  // (already initialized at module scope as pushNotificationService)

-  // Create a device-like object for APNS service
-  const deviceForApns = {
+  // Create a device-like object for push service
+  const deviceForPush = {
     id: client.deviceId,
     pushToken: client.device.pushToken,
     pushTokenType: client.device.tokenType,
     apnsEnv: client.device.apnsEnv,
     pushFailures: client.device.pushFailures,
     name: null,
-    os: "ios" as const,
+    os: (client.device.tokenType === "apns" ? "ios" : "android") as const,
     appVersion: null,
     appBuildNumber: null,
     createdAt: client.device.addedAt,
     updatedAt: client.device.updatedAt,
     lastPushSuccessAt: client.device.lastSentAt,
   };
 
-  const result = await apnsService.sendPushNotification({
-    device: deviceForApns,
+  const result = await pushNotificationService.sendPushNotification({
+    device: deviceForPush,
     notification: v2Notification as unknown as NotificationPayloadWithJWTToken,
   });

🧹 Nitpick comments (1)

src/middleware/rateLimit.ts (1)

11-18: Consider adding JSDoc comments for public API.

As per coding guidelines, public APIs should include JSDoc comments describing purpose, usage, and behavior.

+/**
+ * Stricter rate limiting middleware for authentication endpoints.
+ * Limits to 10 requests per 15 minutes per IP address.
+ * Use this middleware to protect JWT generation and other auth-sensitive routes.
+ * 
+ * @example
+ * router.post('/token', authRateLimitMiddleware, authController.generateToken);
+ */
 export const authRateLimitMiddleware = rateLimit({

Based on coding guidelines.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f642613 and 5ac44ca.

📒 Files selected for processing (11)

• prisma/migrations/20251003172324_add_v2_push_notification_models/migration.sql (1 hunks)
• prisma/schema.prisma (1 hunks)
• src/api/shared/notifications/constants.ts (1 hunks)
• src/api/shared/notifications/webhook-handler.ts (1 hunks)
• src/api/v1/notifications/handlers/subscribe-to-topics.ts (1 hunks)
• src/api/v2/auth/auth.router.ts (1 hunks)
• src/api/v2/auth/handlers/generate-token.ts (1 hunks)
• src/api/v2/notifications/handlers/subscribe.ts (1 hunks)
• src/index.ts (1 hunks)
• src/middleware/rateLimit.ts (1 hunks)
• src/utils/v2/jwt.ts (1 hunks)

✅ Files skipped from review due to trivial changes (1)

• src/api/shared/notifications/constants.ts

🚧 Files skipped from review as they are similar to previous changes (3)

• prisma/migrations/20251003172324_add_v2_push_notification_models/migration.sql
• src/api/v2/auth/auth.router.ts
• src/utils/v2/jwt.ts

🧰 Additional context used

📓 Path-based instructions (10)

**/*.{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/index.ts
• src/middleware/rateLimit.ts
• src/api/v1/notifications/handlers/subscribe-to-topics.ts
• src/api/shared/notifications/webhook-handler.ts
• src/api/v2/notifications/handlers/subscribe.ts
• src/api/v2/auth/handlers/generate-token.ts

**/index.ts

📄 CodeRabbit inference engine (.cursorrules)

Use index.ts files for exports

Files:

• src/index.ts

**/*

📄 CodeRabbit inference engine (.cursorrules)

Follow consistent file naming (kebab-case)

Files:

• src/index.ts
• src/middleware/rateLimit.ts
• src/api/v1/notifications/handlers/subscribe-to-topics.ts
• src/api/shared/notifications/webhook-handler.ts
• src/api/v2/notifications/handlers/subscribe.ts
• prisma/schema.prisma
• src/api/v2/auth/handlers/generate-token.ts

**/middleware/**/*.{ts,tsx}

📄 CodeRabbit inference engine (.cursorrules)

Keep middleware in dedicated directory

Files:

• src/middleware/rateLimit.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/middleware/rateLimit.ts
• src/api/v1/notifications/handlers/subscribe-to-topics.ts
• src/api/shared/notifications/webhook-handler.ts
• src/api/v2/notifications/handlers/subscribe.ts
• src/api/v2/auth/handlers/generate-token.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/v1/notifications/handlers/subscribe-to-topics.ts
• src/api/shared/notifications/webhook-handler.ts
• src/api/v2/notifications/handlers/subscribe.ts
• src/api/v2/auth/handlers/generate-token.ts

**/handlers/**/*.{ts,tsx}

📄 CodeRabbit inference engine (.cursorrules)

Use dedicated handlers directory for route logic

Files:

• src/api/v1/notifications/handlers/subscribe-to-topics.ts
• src/api/v2/notifications/handlers/subscribe.ts
• src/api/v2/auth/handlers/generate-token.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/v1/notifications/handlers/subscribe-to-topics.ts
• src/api/shared/notifications/webhook-handler.ts
• src/api/v2/notifications/handlers/subscribe.ts
• src/api/v2/auth/handlers/generate-token.ts

**/{api,routes, middleware}/**/*.{ts,tsx}

📄 CodeRabbit inference engine (.cursorrules)

Validate and sanitize inputs

Files:

• src/api/v1/notifications/handlers/subscribe-to-topics.ts
• src/api/shared/notifications/webhook-handler.ts
• src/api/v2/notifications/handlers/subscribe.ts
• src/api/v2/auth/handlers/generate-token.ts

**/*.prisma

📄 CodeRabbit inference engine (.cursorrules)

**/*.prisma: Follow schema naming conventions (PascalCase for models)
Use appropriate field types and constraints
Add indexes for frequently queried fields
Use appropriate database indexes

Files:

• prisma/schema.prisma

🧠 Learnings (2)

📚 Learning: 2025-08-05T14:39:39.983Z

Learnt from: CR
PR: ephemeraHQ/convos-backend#0
File: .cursorrules:0-0
Timestamp: 2025-08-05T14:39:39.983Z
Learning: Applies to **/{middleware,api,routes}/**/*.{ts,tsx} : Implement rate limiting

Applied to files:

• src/middleware/rateLimit.ts

📚 Learning: 2025-08-05T14:39:39.983Z

Learnt from: CR
PR: ephemeraHQ/convos-backend#0
File: .cursorrules:0-0
Timestamp: 2025-08-05T14:39:39.983Z
Learning: Applies to **/{api,routes}/**/*.{ts,tsx} : Always place the 'return;' statement on a new line immediately after sending a response with 'res.status().json()' or similar methods

Applied to files:

• src/api/v2/auth/handlers/generate-token.ts

🧬 Code graph analysis (3)

src/api/shared/notifications/webhook-handler.ts (8)

src/notifications/client.ts (2)

• createNotificationClient (12-21)
• WebhookNotificationBody (28-51)

src/api/shared/notifications/services/push-notification.service.ts (1)

• getPushNotificationService (224-229)

src/notifications/utils.ts (1)

• getHttpDeliveryNotificationAuthHeader (1-6)

src/utils/prisma.ts (1)

• prisma (3-3)

src/api/shared/notifications/constants.ts (1)

• MAX_PUSH_FAILURES (4-4)

src/utils/v2/jwt.ts (1)

• createV2JwtToken (18-60)

src/api/shared/notifications/services/apns-push.service.ts (1)

• createApnsService (272-294)

src/api/shared/notifications/services/notifications-types.ts (2)

• V2NotificationPayload (59-64)
• NotificationPayloadWithJWTToken (54-56)

src/api/v2/notifications/handlers/subscribe.ts (2)

src/notifications/client.ts (1)

• createNotificationClient (12-21)

src/utils/prisma.ts (1)

• prisma (3-3)

src/api/v2/auth/handlers/generate-token.ts (2)

src/utils/prisma.ts (1)

• prisma (3-3)

src/utils/v2/jwt.ts (1)

• createV2JwtToken (18-60)

🔇 Additional comments (10)

src/middleware/rateLimit.ts (2)

3-9: LGTM! Comment clarification improves readability.

The general rate limiting configuration is appropriate for API-wide protection.

---

11-18: Consider a distributed store for multi-instance deployments

• The default in-memory store won’t sync counters across processes or containers; if you scale to multiple instances (e.g. Kubernetes pods), switch to a shared store (e.g. Redis via rate-limit-redis).
• Extract windowMs and limit into environment variables (e.g. AUTH_RATE_LIMIT_WINDOW_MS, AUTH_RATE_LIMIT_MAX) for per-environment flexibility.

src/index.ts (1)

14-22: LGTM! Startup validation prevents runtime failures.

The environment variable checks are well-placed before app initialization, ensuring fail-fast behavior if required secrets are missing. The error logging and process exit are appropriate.

src/api/v1/notifications/handlers/subscribe-to-topics.ts (1)

22-22: LGTM! Hex validation prevents runtime conversion errors.

The regex validation ensures invalid hex strings are rejected with a 400 error at parse time, rather than causing a 500 error during hexToUint8Array conversion. This aligns with the v2 handler's validation approach.

src/api/v2/notifications/handlers/subscribe.ts (3)

7-20: LGTM! Schema validation includes hex string validation.

The request schema correctly validates HMAC keys as hex strings using regex, preventing conversion errors downstream. The structure is well-defined with clear types.

---

26-48: LGTM! Client validation and authorization are correctly implemented.

The handler properly fetches the client with device relation, returns 404 for missing clients, and 403 for disabled devices. Control flow is explicit with proper returns.

---

92-99: LGTM! Error handling is comprehensive.

The handler properly distinguishes validation errors (400) from internal errors (500), logs errors with context, and maintains explicit control flow.

src/api/shared/notifications/webhook-handler.ts (3)

28-138: LGTM! Webhook handler correctly supports v1 and v2 flows.

The handler properly validates authentication, attempts v2 processing first, falls back to v1, and includes appropriate cleanup on unrecoverable failures. Control flow is explicit with proper returns throughout.

---

140-180: LGTM! Cleanup logic is transactional and thorough.

The cleanup function correctly uses a Prisma transaction to atomically update multiple records, ensuring consistency when clearing failed installations. Error handling is appropriate.

---

252-296: LGTM! Success/failure tracking and cleanup logic are correct.

The handler properly tracks push metrics, resets failure counts on success, increments on failure, disables devices at threshold, and cleans up on unrecoverable errors (BadDeviceToken, DeviceNotRegistered).

[coderabbitai]

Actionable comments posted: 1

♻️ Duplicate comments (2)

src/api/shared/notifications/webhook-handler.ts (2)

36-50: Validation is correctly implemented.

The webhook payload validation using Zod's safeParse and the use of parseResult.data properly addresses the validation concern. The early return on validation failure with a 400 status is the correct approach.

Note: This addresses the previously flagged concern about invalid req.body shapes causing unhandled TypeErrors.

---

225-265: V2 path hardcodes APNS; FCM tokens won't deliver.

This is a critical functional blocker that has been previously identified. The code always creates an APNS service and hardcodes os: "ios" on line 254, which prevents FCM deliveries for Android devices.

As previously recommended, route by tokenType (or use the shared push service) to handle both APNS and FCM, and set OS accordingly.

Note: This duplicates the concern raised in the previous review regarding APNS hardcoding.

🧹 Nitpick comments (2)

src/api/shared/notifications/webhook-handler.ts (2)

269-288: Consider transactional updates for consistency.

The success path (lines 269-275) and failure path (lines 281-288) update DeviceRegistration separately. If either update fails, the push notification state could become inconsistent with the actual delivery result.

Consider wrapping the push notification send and the corresponding DB update in a try-catch to ensure they're handled atomically, or use a compensating transaction pattern if the push service throws.

Example pattern:

try {
  const result = await apnsService.sendPushNotification({...});
  
  // Update based on result
  await prisma.deviceRegistration.update({
    where: { deviceId: client.deviceId },
    data: result.success 
      ? { pushFailures: 0, lastSentAt: new Date() }
      : { 
          pushFailures: { increment: 1 },
          lastFailureAt: new Date(),
          disabled: client.device.pushFailures + 1 >= MAX_PUSH_FAILURES
        }
  });
} catch (error) {
  // Handle unexpected errors
  req.log.error({ error }, "Error in v2 notification flow");
}

---

205-213: Return value is unused by caller.

The early return on line 212 returns { success: false }, but the caller at lines 81-86 doesn't check or use this return value. Similarly, the function returns result at line 310.

For consistency and clarity:

1. Either make the return type explicit (e.g., Promise<{ success: boolean }>)
2. Or change the function to return Promise<void> and handle all outcomes internally through logging

Since the webhook handler treats all v2 notifications as successful (returns 200), the current approach is acceptable but could be clearer.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5ac44ca and f78b5c2.

📒 Files selected for processing (5)

• package.json (3 hunks)
• src/api/shared/notifications/webhook-handler.ts (1 hunks)
• src/api/v2/notifications/handlers/register.ts (1 hunks)
• src/notifications/client.ts (2 hunks)
• src/utils/v2/jwt.ts (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (3)

• package.json
• src/utils/v2/jwt.ts
• src/api/v2/notifications/handlers/register.ts

🧰 Additional context used

📓 Path-based instructions (6)

**/*.{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/notifications/client.ts
• src/api/shared/notifications/webhook-handler.ts

**/*

📄 CodeRabbit inference engine (.cursorrules)

Follow consistent file naming (kebab-case)

Files:

• src/notifications/client.ts
• src/api/shared/notifications/webhook-handler.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/shared/notifications/webhook-handler.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/shared/notifications/webhook-handler.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/shared/notifications/webhook-handler.ts

**/{api,routes, middleware}/**/*.{ts,tsx}

📄 CodeRabbit inference engine (.cursorrules)

Validate and sanitize inputs

Files:

• src/api/shared/notifications/webhook-handler.ts

🧬 Code graph analysis (1)

src/api/shared/notifications/webhook-handler.ts (8)

src/notifications/client.ts (3)

• createNotificationClient (13-22)
• webhookNotificationBodySchema (30-53)
• WebhookNotificationBody (55-57)

src/api/shared/notifications/services/push-notification.service.ts (1)

• getPushNotificationService (224-229)

src/notifications/utils.ts (1)

• getHttpDeliveryNotificationAuthHeader (1-6)

src/utils/prisma.ts (1)

• prisma (3-3)

src/api/shared/notifications/constants.ts (1)

• MAX_PUSH_FAILURES (4-4)

src/utils/v2/jwt.ts (1)

• createV2JwtToken (18-64)

src/api/shared/notifications/services/apns-push.service.ts (1)

• createApnsService (272-294)

src/api/shared/notifications/services/notifications-types.ts (2)

• V2NotificationPayload (59-64)
• NotificationPayloadWithJWTToken (54-56)

⏰ 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). (3)

• GitHub Check: claude-review
• GitHub Check: push_to_registry
• GitHub Check: Review for correctness

🔇 Additional comments (5)

src/notifications/client.ts (1)

29-57: LGTM! Well-structured Zod schema for webhook validation.

The schema comprehensively validates the webhook notification payload structure and the type inference using z.infer is the correct approach. This provides both runtime validation and compile-time type safety.

src/api/shared/notifications/webhook-handler.ts (4)

73-88: V2 flow routing looks correct.

The v2 client lookup and delegation to handleV2Notification is clean. The function returns a result object but it's not checked here, which is acceptable since errors are logged internally and the webhook should return 200 in most cases.

---

90-148: V1 fallback flow is correctly implemented.

The fallback to v1 lookup by xmtpInstallationId and the cleanup on unrecoverable failures follows the expected pattern. The conditional cleanup based on result.shouldCleanup is the right approach.

---

155-195: Cleanup function handles failures appropriately.

The transactional cleanup of the database state followed by the XMTP server deletion request is the correct approach. The separation allows the local DB to be cleaned even if the external call fails, which is logged for monitoring.

---

215-223: Verify JWT expiration time aligns with requirements.

The JWT is configured with a 24-hour expiration (expirationTime: "24h"), but the PR summary mentions 15-minute v2 JWTs. While the extended expiration might be intentional for Notification Service Extensions (NSE) to allow background processing, please confirm this aligns with the security requirements.

If the 24-hour expiration is intentional for NSE-specific use cases, consider adding a comment explaining why it differs from the standard 15-minute tokens mentioned in the PR.

[neekolas]

What do we use the clientIdentifier for? It seems like the deviceId is the only thing that is trustworthy.

The clientIdentifier is generated by the client and can't be verified, right? If we tried to use it for sybil resistance in the gateway we'd get hosed.

What if we left it out entirely and treated the deviceId as the only identifier?

[lourou]

@neekolas clientIdentifier is mapped to the inboxId locally by Convos iOS. We use clientIdentifier it to route notifications to the correct inbox through this alias which is not bound to XMTP, so that when the push arrives, the NSE maps clientIdentifier to the inboxId, and knows with which key to decode the message.

[lourou]

You're absolutely right that clientIdentifier can't be trusted for Gateway authentication. We don't use it for that, just for notif routing.

[lourou]

Should we keep both here: the JWT should explicitly authorize the deviceId + clientIdentifier pairing?

After diving further, I'm removing the client identifier from the JWT token!

[neekolas]

For performance you might want to move this into a config or variable on startup rather than reading from the env directly on each usage.

[lourou]

Thanks for this! I'm now caching the most used environment variables at module load time instead of reading from process.env

[neekolas]

Seems like this could be a middleware

[lourou]

Yes 💯 this is done

[neekolas]

When would we be creating tokens where this would be set to false? If there isn't a case we might not need the field.

[lourou]

Yes, I've removed it as we don't need to have it here as metadata

[coderabbitai]

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)

src/api/shared/notifications/services/apns-push.service.ts (1)

140-149: Critical: logs leak secrets (APNS bearer, apiJWT, possibly encrypted payload).

Headers include Authorization; payload includes apiJWT and message content. This is a security/privacy breach.

Redact or avoid logging sensitive fields:

-      const headers = {
+      const headers = {
         ":method": "POST",
         ":path": `/3/device/${device.pushToken}`,
-        authorization: `bearer ${token}`,
+        authorization: `bearer ${token}`,
         "apns-topic": this.config.bundleId,
         "apns-push-type": isSilent ? "background" : "alert",
         "apns-priority": isSilent ? "5" : "10",
         "content-type": "application/json",
       };
 
-      logger.info(
-        {
-          url: `https://${hostname}/3/device/${device.pushToken}`,
-          headers,
-          payload,
-          deviceId: device.id,
-          verbose: true,
-        },
-        "[VERBOSE] Sending APNS HTTP/2 request",
-      );
+      const safeHeaders = { ...headers, authorization: "[REDACTED]" };
+      const { apiJWT, encryptedMessage, ...payloadSansSecrets } = (payload as any);
+      logger.info(
+        {
+          url: `https://${hostname}/3/device/${device.pushToken}`,
+          headers: safeHeaders,
+          payloadPreview: { ...payloadSansSecrets, apiJWT: "[REDACTED]", encryptedMessage: encryptedMessage ? "[REDACTED]" : undefined },
+          deviceId: device.id,
+          verbose: true,
+        },
+        "[VERBOSE] Sending APNS HTTP/2 request (redacted)",
+      );
 
@@
-      const payloadStr = JSON.stringify(payload);
+      const payloadStr = JSON.stringify(payload);
       logger.info(
         {
           payloadLength: payloadStr.length,
           deviceId: device.id,
           verbose: true,
         },
         "[VERBOSE] Writing APNS payload to HTTP/2 stream",
       );

Also applies to: 250-258, 130-138

♻️ Duplicate comments (1)

src/api/v2/notifications/handlers/subscribe.ts (1)

138-138: Add explicit return after sending 200.

Match our “return immediately after responding” rule to avoid accidental double-sends.

-    res.status(200).send();
+    res.status(200).send();
+    return;

As per coding guidelines.

🧹 Nitpick comments (8)

src/api/v2/notifications/handlers/subscribe.ts (2)

14-18: Strengthen HMAC key validation (even-length hex, integer epoch).

Ensure keys are valid byte sequences and epoch is an integer and non‑negative. Prevents runtime throws inside hexToUint8Array and bad metadata upstream.

Apply:

-      hmacKeys: z.array(
-        z.object({
-          thirtyDayPeriodsSinceEpoch: z.number(),
-          key: z.string().regex(/^[0-9a-fA-F]+$/, "Invalid hex string"),
-        }),
-      ),
+      hmacKeys: z.array(
+        z.object({
+          thirtyDayPeriodsSinceEpoch: z.number().int().min(0),
+          // pairs of hex chars only (even length)
+          key: z
+            .string()
+            .regex(/^(?:[0-9a-fA-F]{2})+$/, "Invalid hex (must be even-length)"),
+        }),
+      ),

---

10-21: Optional: bound topics array and topic length.

Prevent oversized payloads/DoS by bounding counts and topic size.

-  topics: z.array(
-    z.object({
-      topic: z.string(),
+  topics: z.array(
+    z.object({
+      topic: z.string().min(1).max(512),
       ...
-    }),
-  ),
+    }),
+  ).min(1).max(100),

src/api/v2/auth/handlers/generate-token.ts (1)

62-69: Return Zod details for consistency (optional).

Subscribe handler returns error.details; mirror that here for client parity.

-      res.status(400).json({ error: "Invalid request body" });
+      res.status(400).json({ error: "Invalid request body", details: error.errors });

src/api/shared/notifications/services/apns-push.service.ts (2)

117-121: Add connection timeout to avoid hangs.

http2 sessions can hang without a timeout; add a reasonable default.

       const client = http2.connect(`https://${hostname}`, {
         settings: { enablePush: false },
       });
+      client.setTimeout(10000, () => {
+        logger.warn({ deviceId: device.id }, "APNS HTTP/2 session timed out");
+        try { client.close(); } catch {}
+        resolve({ success: false, error: "Timeout" });
+      });

---

68-75: DRY: use getApnsUrl() and compute hostname once.

hostname/path are computed multiple times; either use getApnsUrl() or a single helper.

-    const hostname =
-      device.apnsEnv === "sandbox"
-        ? "api.sandbox.push.apple.com"
-        : "api.push.apple.com";
-    return `https://${hostname}/3/device/${device.pushToken}`;
+    const hostname = device.apnsEnv === "sandbox"
+      ? "api.sandbox.push.apple.com"
+      : "api.push.apple.com";
+    return `https://${hostname}/3/device/${device.pushToken}`;
@@
-      const hostname =
-        device.apnsEnv === "sandbox"
-          ? "api.sandbox.push.apple.com"
-          : "api.push.apple.com";
-
-      const client = http2.connect(`https://${hostname}`, {
+      const url = this.getApnsUrl(device);
+      const client = http2.connect(url.replace(/\/3\/device\/.*/, ""), {
         settings: { enablePush: false },
       });
@@
-        ":path": `/3/device/${device.pushToken}`,
+        ":path": new URL(url).pathname,

Also applies to: 111-121, 130-138

src/api/shared/notifications/services/notifications-types.ts (1)

62-73: Optional: V2NotificationPayload may be redundant.

NotificationPayloadWithJWTToken already covers the v2 branch (clientId). Consider removing V2NotificationPayload and using the existing union to reduce type surface, unless callers need the narrower literal type.

src/api/shared/notifications/webhook-handler.ts (2)

247-252: Reuse APNS service rather than constructing per request.

Creating a new service each call discards the 1‑hour JWT cache and adds overhead. Instantiate once at module scope and reuse.

-  const apnsService = createApnsService();
+  // at module top:
+  // const apnsService = createApnsService();
+  // here, reuse:
+  const apnsService = createApnsService();

If you prefer, export a singleton from the factory. Based on learnings.

---

169-172: Add explicit return after 500 response (minor).

For consistency with other handlers and to prevent accidental follow‑through.

   } catch (error) {
     req.log.error({ error }, "Outer error processing notification");
     res.status(500).json({ error: "Internal server error" });
+    return;
   }

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 06775d6 and bed066f.

📒 Files selected for processing (11)

• src/api/shared/notifications/services/apns-push.service.ts (2 hunks)
• src/api/shared/notifications/services/notifications-types.ts (3 hunks)
• src/api/shared/notifications/webhook-handler.ts (1 hunks)
• src/api/v2/auth/handlers/generate-token.ts (1 hunks)
• src/api/v2/device/handlers/register.ts (1 hunks)
• src/api/v2/notifications/handlers/subscribe.ts (1 hunks)
• src/api/v2/notifications/handlers/unregister.ts (1 hunks)
• src/api/v2/notifications/handlers/unsubscribe.ts (1 hunks)
• src/middleware/xmtpWebhookAuth.ts (1 hunks)
• src/notifications/utils.ts (1 hunks)
• src/utils/v2/jwt.ts (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (2)

• src/api/v2/notifications/handlers/unregister.ts
• src/api/v2/device/handlers/register.ts

🧰 Additional context used

📓 Path-based instructions (8)

**/*.{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/notifications/utils.ts
• src/api/shared/notifications/webhook-handler.ts
• src/api/shared/notifications/services/apns-push.service.ts
• src/middleware/xmtpWebhookAuth.ts
• src/api/v2/notifications/handlers/unsubscribe.ts
• src/api/v2/notifications/handlers/subscribe.ts
• src/api/v2/auth/handlers/generate-token.ts
• src/api/shared/notifications/services/notifications-types.ts
• src/utils/v2/jwt.ts

**/*

📄 CodeRabbit inference engine (.cursorrules)

Follow consistent file naming (kebab-case)

Files:

• src/notifications/utils.ts
• src/api/shared/notifications/webhook-handler.ts
• src/api/shared/notifications/services/apns-push.service.ts
• src/middleware/xmtpWebhookAuth.ts
• src/api/v2/notifications/handlers/unsubscribe.ts
• src/api/v2/notifications/handlers/subscribe.ts
• src/api/v2/auth/handlers/generate-token.ts
• src/api/shared/notifications/services/notifications-types.ts
• src/utils/v2/jwt.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/shared/notifications/webhook-handler.ts
• src/api/shared/notifications/services/apns-push.service.ts
• src/api/v2/notifications/handlers/unsubscribe.ts
• src/api/v2/notifications/handlers/subscribe.ts
• src/api/v2/auth/handlers/generate-token.ts
• src/api/shared/notifications/services/notifications-types.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/shared/notifications/webhook-handler.ts
• src/api/shared/notifications/services/apns-push.service.ts
• src/api/v2/notifications/handlers/unsubscribe.ts
• src/api/v2/notifications/handlers/subscribe.ts
• src/api/v2/auth/handlers/generate-token.ts
• src/api/shared/notifications/services/notifications-types.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/shared/notifications/webhook-handler.ts
• src/api/shared/notifications/services/apns-push.service.ts
• src/middleware/xmtpWebhookAuth.ts
• src/api/v2/notifications/handlers/unsubscribe.ts
• src/api/v2/notifications/handlers/subscribe.ts
• src/api/v2/auth/handlers/generate-token.ts
• src/api/shared/notifications/services/notifications-types.ts

**/{api,routes, middleware}/**/*.{ts,tsx}

📄 CodeRabbit inference engine (.cursorrules)

Validate and sanitize inputs

Files:

• src/api/shared/notifications/webhook-handler.ts
• src/api/shared/notifications/services/apns-push.service.ts
• src/api/v2/notifications/handlers/unsubscribe.ts
• src/api/v2/notifications/handlers/subscribe.ts
• src/api/v2/auth/handlers/generate-token.ts
• src/api/shared/notifications/services/notifications-types.ts

**/middleware/**/*.{ts,tsx}

📄 CodeRabbit inference engine (.cursorrules)

Keep middleware in dedicated directory

Files:

• src/middleware/xmtpWebhookAuth.ts

**/handlers/**/*.{ts,tsx}

📄 CodeRabbit inference engine (.cursorrules)

Use dedicated handlers directory for route logic

Files:

• src/api/v2/notifications/handlers/unsubscribe.ts
• src/api/v2/notifications/handlers/subscribe.ts
• src/api/v2/auth/handlers/generate-token.ts

🧠 Learnings (3)

📚 Learning: 2025-09-04T14:37:49.215Z

Learnt from: nmalzieu
PR: ephemeraHQ/convos-backend#133
File: src/api/v1/devices/handlers/list-devices.handler.ts:6-0
Timestamp: 2025-09-04T14:37:49.215Z
Learning: In the convos-backend codebase, authenticated endpoints rely on auth middleware to validate and set res.locals.xmtpId, so individual handlers don't need additional auth guards for undefined xmtpId values.

Applied to files:

• src/middleware/xmtpWebhookAuth.ts

📚 Learning: 2025-09-04T14:37:41.542Z

Learnt from: nmalzieu
PR: ephemeraHQ/convos-backend#133
File: src/api/v1/devices/handlers/get-device.handler.ts:15-16
Timestamp: 2025-09-04T14:37:41.542Z
Learning: For authenticated endpoints in the convos-backend Express.js application, res.locals.xmtpId is guaranteed to be present when handlers execute because the auth middleware validates JWT tokens and returns 401 before reaching handlers if authentication fails. Additional guards for res.locals.xmtpId are not needed in authenticated route handlers.

Applied to files:

• src/middleware/xmtpWebhookAuth.ts

📚 Learning: 2025-08-05T14:39:39.983Z

Learnt from: CR
PR: ephemeraHQ/convos-backend#0
File: .cursorrules:0-0
Timestamp: 2025-08-05T14:39:39.983Z
Learning: Applies to **/{api,routes}/**/*.{ts,tsx} : Always place the 'return;' statement on a new line immediately after sending a response with 'res.status().json()' or similar methods

Applied to files:

• src/api/v2/auth/handlers/generate-token.ts

🧬 Code graph analysis (8)

src/notifications/utils.ts (1)

src/config.ts (1)

• XMTP_NOTIFICATION_SECRET (22-22)

src/api/shared/notifications/webhook-handler.ts (7)

src/notifications/client.ts (3)

• createNotificationClient (14-20)
• webhookNotificationBodySchema (28-54)
• WebhookNotificationBody (56-58)

src/api/shared/notifications/services/push-notification.service.ts (1)

• getPushNotificationService (233-238)

src/utils/prisma.ts (1)

• prisma (3-3)

src/api/shared/notifications/constants.ts (1)

• MAX_PUSH_FAILURES (4-4)

src/utils/v2/jwt.ts (1)

• createV2JwtToken (27-69)

src/api/shared/notifications/services/apns-push.service.ts (1)

• createApnsService (272-294)

src/api/shared/notifications/services/notifications-types.ts (1)

• V2NotificationPayload (63-68)

src/api/shared/notifications/services/apns-push.service.ts (1)

src/api/shared/notifications/services/notifications-types.ts (1)

• AnyNotificationPayloadWithJWT (71-73)

src/middleware/xmtpWebhookAuth.ts (1)

src/notifications/utils.ts (1)

• getHttpDeliveryNotificationAuthHeader (3-10)

src/api/v2/notifications/handlers/unsubscribe.ts (2)

src/notifications/client.ts (1)

• createNotificationClient (14-20)

src/utils/prisma.ts (1)

• prisma (3-3)

src/api/v2/notifications/handlers/subscribe.ts (2)

src/notifications/client.ts (1)

• createNotificationClient (14-20)

src/utils/prisma.ts (1)

• prisma (3-3)

src/api/v2/auth/handlers/generate-token.ts (2)

src/utils/prisma.ts (1)

• prisma (3-3)

src/utils/v2/jwt.ts (1)

• createV2JwtToken (27-69)

src/utils/v2/jwt.ts (4)

src/api/shared/notifications/constants.ts (1)

• MAX_JWT_METADATA_SIZE (9-9)

src/utils/errors.ts (1)

• AppError (1-11)

src/utils/try-catch.ts (1)

• tryCatch (14-23)

src/config.ts (1)

• JWT_SECRET_BYTES (24-24)

⏰ 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). (3)

• GitHub Check: claude-review
• GitHub Check: push_to_registry
• GitHub Check: Review for correctness

🔇 Additional comments (16)

src/api/v2/notifications/handlers/unsubscribe.ts (1)

32-39: Add explicit return statement after response.

Per coding guidelines, a return; statement must be placed on a new line immediately after sending a response to prevent accidental continued execution.

Apply this diff:

     req.log.warn(
       { clientId: body.clientId },
       "Client not found for unsubscribe",
     );
     res.status(404).json({ error: "Client not found" });
+    return;
-    return;
   }

Likely an incorrect or invalid review comment.

src/notifications/utils.ts (2)

1-1: LGTM! Import follows path alias convention.

The import correctly uses the @/ path alias as per coding guidelines.

---

3-10: Previous concern fully addressed!

The function now validates that XMTP_NOTIFICATION_SECRET is configured before returning it, preventing silent authentication failures. The error message is clear and actionable.

src/middleware/xmtpWebhookAuth.ts (5)

1-3: LGTM! Imports are appropriate.

The use of timingSafeEqual is correct for preventing timing attacks in webhook authentication, and imports follow the path alias convention.

---

5-8: Good documentation.

The JSDoc comment clearly describes the middleware's purpose and security function.

---

25-34: LGTM! Auth header validation is correct.

The middleware properly validates the presence of the authorization header and returns an appropriate 401 response with a generic error message (good security practice).

---

36-47: LGTM! Timing-safe comparison correctly implemented.

The implementation properly prevents timing attacks by:

• Checking buffer length equality first (required by timingSafeEqual)
• Using constant-time comparison
• Using explicit UTF-8 encoding for both buffers

The middleware correctly returns after sending the 401 response instead of falling through.

---

49-49: LGTM! Correct control flow.

The middleware correctly calls next() only after successful authentication, allowing the request to proceed to the next handler.

src/api/v2/auth/handlers/generate-token.ts (2)

21-23: Schema LGTM.

nonempty() + max(255) is appropriate for deviceId.

---

59-61: Good: immediate return after res.json.

Prevents double-sends and follows our handler pattern.

src/api/shared/notifications/services/notifications-types.ts (1)

39-47: Good: XOR enforces exactly one of inboxId/clientId.

Prevents ambiguous payloads at compile time.

src/utils/v2/jwt.ts (2)

32-43: Correct byte-size enforcement for metadata.

Using TextEncoder() avoids UTF‑16 length pitfalls.

---

71-87: JWT verify + schema check looks solid.

Good split of crypto verification and payload validation; clear 401s on failure.

src/api/shared/notifications/webhook-handler.ts (3)

244-252: V2 path hardcodes APNS; confirm Android expectations.

If any v2 clients can be FCM, this will drop pushes. If iOS‑only for now, OK; otherwise, route via shared push service (like v1) to support both.

Would you like a patch to switch v2 to pushNotificationService and derive OS from tokenType?

Also applies to: 281-301

---

317-336: Atomic failure increment + conditional disable: nice improvement.

Prevents races on pushFailures/disabled under concurrency.

---

370-381: Local‑first cleanup order LGTM.

Deleting DB first avoids re‑targeting already‑deleted clients; remote deletion best‑effort is fine.

[coderabbitai]

Actionable comments posted: 7

♻️ Duplicate comments (4)

src/api/v2/notifications/handlers/subscribe.ts (3)

141-141: Add explicit return after 200.

   res.status(200).send();
+  return;

---

60-64: Guard: missing push token should 4xx before remote calls.

   if (device.disabled) {
     req.log.warn({ deviceId: body.deviceId }, "Device is disabled");
     res.status(403).json({ error: "Device is disabled" });
     return;
   }
+  if (!device.pushToken) {
+    req.log.warn({ deviceId: body.deviceId }, "Device has no push token");
+    res.status(400).json({ error: "Device has no push token" });
+    return;
+  }

---

156-157: Add explicit return after 500 error.

   req.log.error({ error }, "Failed to subscribe to topics");
   res.status(500).json({ error: "Failed to subscribe to topics" });
+  return;

src/api/v2/notifications/handlers/unsubscribe.ts (1)

59-61: Add explicit return after 500 error.

   req.log.error({ error }, "Failed to unsubscribe from topics");
   res.status(500).json({ error: "Failed to unsubscribe from topics" });
+  return;

🧹 Nitpick comments (4)

src/api/v2/notifications/handlers/unregister.ts (1)

23-35: Consider idempotent DELETE semantics (204 when not found).

Returning 204 for missing client can make unregister idempotent and simplify clients; keep 404 if product requires strictness.

src/api/v2/notifications/handlers/unsubscribe.ts (2)

6-9: Tighten topic string validation.

Bound each topic’s length to avoid oversized payloads; keep array bounds.

 const unsubscribeRequestSchema = z.object({
   clientId: z.string().min(1).max(255),
-  topics: z.array(z.string()).min(1).max(100),
+  topics: z.array(z.string().min(1).max(255)).min(1).max(100),
 });

---

41-46: Optional: de-duplicate topics before calling remote.

Avoid redundant unsubscribe calls if clients send duplicates.

-    await notificationClient.unsubscribe({
-      installationId: body.clientId,
-      topics: body.topics,
-    });
+    const uniqueTopics = [...new Set(body.topics)];
+    await notificationClient.unsubscribe({
+      installationId: body.clientId,
+      topics: uniqueTopics,
+    });

src/api/v2/notifications/handlers/subscribe.ts (1)

7-24: Strengthen schema (topic length, integer bounds, even-length hex, key count).

Prevents runtime errors and oversized payloads.

 const subscribeRequestSchema = z.object({
   deviceId: z.string().min(1).max(255),
   clientId: z.string().min(1).max(255),
   topics: z
     .array(
       z.object({
-        topic: z.string(),
+        topic: z.string().min(1).max(255),
         hmacKeys: z.array(
           z.object({
-            thirtyDayPeriodsSinceEpoch: z.number(),
-            key: z.string().regex(/^[0-9a-fA-F]+$/, "Invalid hex string"),
+            thirtyDayPeriodsSinceEpoch: z.number().int().min(0),
+            key: z
+              .string()
+              .regex(/^[0-9a-fA-F]+$/, "Invalid hex string")
+              .refine((s) => s.length % 2 === 0, { message: "Hex must have even length" }),
           }),
-        ),
+        ).min(1).max(16),
       }),
     )
     .min(1)
     .max(100),
 });

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between bed066f and 7f2213d.

📒 Files selected for processing (6)

• src/api/v2/auth/handlers/generate-token.ts (1 hunks)
• src/api/v2/device/handlers/register.ts (1 hunks)
• src/api/v2/notifications/handlers/subscribe.ts (1 hunks)
• src/api/v2/notifications/handlers/unregister.ts (1 hunks)
• src/api/v2/notifications/handlers/unsubscribe.ts (1 hunks)
• src/middleware/rateLimit.ts (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (3)

• src/api/v2/device/handlers/register.ts
• src/middleware/rateLimit.ts
• src/api/v2/auth/handlers/generate-token.ts

🧰 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.ts
• src/api/v2/notifications/handlers/unsubscribe.ts
• src/api/v2/notifications/handlers/unregister.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.ts
• src/api/v2/notifications/handlers/unsubscribe.ts
• src/api/v2/notifications/handlers/unregister.ts

**/handlers/**/*.{ts,tsx}

📄 CodeRabbit inference engine (.cursorrules)

Use dedicated handlers directory for route logic

Files:

• src/api/v2/notifications/handlers/subscribe.ts
• src/api/v2/notifications/handlers/unsubscribe.ts
• src/api/v2/notifications/handlers/unregister.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.ts
• src/api/v2/notifications/handlers/unsubscribe.ts
• src/api/v2/notifications/handlers/unregister.ts

**/*

📄 CodeRabbit inference engine (.cursorrules)

Follow consistent file naming (kebab-case)

Files:

• src/api/v2/notifications/handlers/subscribe.ts
• src/api/v2/notifications/handlers/unsubscribe.ts
• src/api/v2/notifications/handlers/unregister.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.ts
• src/api/v2/notifications/handlers/unsubscribe.ts
• src/api/v2/notifications/handlers/unregister.ts

**/{api,routes, middleware}/**/*.{ts,tsx}

📄 CodeRabbit inference engine (.cursorrules)

Validate and sanitize inputs

Files:

• src/api/v2/notifications/handlers/subscribe.ts
• src/api/v2/notifications/handlers/unsubscribe.ts
• src/api/v2/notifications/handlers/unregister.ts

🧠 Learnings (1)

📚 Learning: 2025-08-05T14:39:39.983Z

Learnt from: CR
PR: ephemeraHQ/convos-backend#0
File: .cursorrules:0-0
Timestamp: 2025-08-05T14:39:39.983Z
Learning: Applies to **/{api,routes}/**/*.{ts,tsx} : Always place the 'return;' statement on a new line immediately after sending a response with 'res.status().json()' or similar methods

Applied to files:

• src/api/v2/notifications/handlers/unsubscribe.ts

🧬 Code graph analysis (3)

src/api/v2/notifications/handlers/subscribe.ts (2)

src/notifications/client.ts (1)

• createNotificationClient (14-20)

src/utils/prisma.ts (1)

• prisma (3-3)

src/api/v2/notifications/handlers/unsubscribe.ts (2)

src/notifications/client.ts (1)

• createNotificationClient (14-20)

src/utils/prisma.ts (1)

• prisma (3-3)

src/api/v2/notifications/handlers/unregister.ts (2)

src/notifications/client.ts (1)

• createNotificationClient (14-20)

src/utils/prisma.ts (1)

• prisma (3-3)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)

• GitHub Check: claude-review
• GitHub Check: push_to_registry
• GitHub Check: Review for correctness

[coderabbitai]

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)

src/api/v1/notifications/handlers/register-installation.ts (3)

279-281: Add explicit return after error response.

Missing explicit return; statement after res.status(500).json(...) on line 280. This violates the coding guideline and is inconsistent with the pattern established at line 84.

As per coding guidelines

Apply this diff:

     req.log.error({ error }, "Failed to register installation(s)");
     res.status(500).json({ error: "Failed to register installation(s)" });
+    return;
   }
 }

---

300-306: Add explicit return after error response.

Missing explicit return; statement after res.status(500).json(...) on line 305. This violates the coding guideline and creates inconsistency with the fix applied at line 84.

As per coding guidelines

Apply this diff:

     }
     res.status(500).json({ error: "Failed to register installation" });
+    return;
   }
 }

---

237-245: Use body.pushTokenType to determine the delivery mechanism type.

The code always hardcodes "apnsDeviceToken" at lines 237-245, but the v2 handler (src/api/v2/notifications/handlers/subscribe.ts:92-94) correctly maps token types:

device.pushTokenType === "apns"
  ? "apnsDeviceToken"
  : "firebaseDeviceToken",

Update the v1 handler to apply the same conditional logic based on body.pushTokenType:

const response = await notificationClient.registerInstallation({
  installationId: installation.xmtpInstallationId,
  deliveryMechanism: {
    deliveryMechanismType: {
      case: body.pushTokenType === "apns" ? "apnsDeviceToken" : "firebaseDeviceToken",
      value: body.pushToken,
    },
  },
});

♻️ Duplicate comments (2)

prisma/schema.prisma (1)

181-207: ** UUID validation for client-provided IDs remains unaddressed.**

The previous review comment correctly identified that deviceId and clientIdentifier (the id field in ClientIdentifier) require strict format validation since they are client-provided primary keys without @default. Examining the handler in src/api/v2/device/handlers/register.ts (line 8) confirms that deviceId still uses z.string().min(1).max(255) instead of z.string().uuid().

This allows arbitrary strings to be persisted as primary keys, creating collision and injection risks.

src/api/v2/device/handlers/register.ts (1)

7-12: ** Enforce UUID format for deviceId.**

Line 8 still uses z.string().min(1).max(255) for deviceId instead of z.string().uuid() as requested in the previous review. This permits arbitrary strings to be persisted as primary keys in DeviceRegistration, creating collision and injection risks.

Apply this diff:

 const registerRequestSchema = z.object({
-  deviceId: z.string().min(1).max(255),
+  deviceId: z.string().uuid(),
   pushToken: z.string().optional(),
   pushTokenType: PushTokenTypeSchema.optional(),
   apnsEnv: ApnsEnvironmentSchema.nullable().optional(),
 });

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7f2213d and 03ad380.

📒 Files selected for processing (8)

• prisma/migrations/20251017193536_add_unique_constraint_to_push_tokens/migration.sql (1 hunks)
• prisma/schema.prisma (1 hunks)
• src/api/v1/notifications/handlers/register-installation.ts (2 hunks)
• src/api/v2/device/handlers/register.ts (1 hunks)
• src/api/v2/notifications/handlers/subscribe.ts (1 hunks)
• src/api/v2/notifications/handlers/unregister.ts (1 hunks)
• src/api/v2/notifications/handlers/unsubscribe.ts (1 hunks)
• src/middleware/xmtpWebhookAuth.ts (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (4)

• src/middleware/xmtpWebhookAuth.ts
• src/api/v2/notifications/handlers/unsubscribe.ts
• src/api/v2/notifications/handlers/unregister.ts
• src/api/v2/notifications/handlers/subscribe.ts

🧰 Additional context used

📓 Path-based instructions (8)

**/*.prisma

📄 CodeRabbit inference engine (.cursorrules)

**/*.prisma: Follow schema naming conventions (PascalCase for models)
Use appropriate field types and constraints
Add indexes for frequently queried fields
Use appropriate database indexes

Files:

• prisma/schema.prisma

**/*

📄 CodeRabbit inference engine (.cursorrules)

Follow consistent file naming (kebab-case)

Files:

• prisma/schema.prisma
• prisma/migrations/20251017193536_add_unique_constraint_to_push_tokens/migration.sql
• src/api/v2/device/handlers/register.ts
• src/api/v1/notifications/handlers/register-installation.ts

**/*.{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/device/handlers/register.ts
• src/api/v1/notifications/handlers/register-installation.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/device/handlers/register.ts
• src/api/v1/notifications/handlers/register-installation.ts

**/handlers/**/*.{ts,tsx}

📄 CodeRabbit inference engine (.cursorrules)

Use dedicated handlers directory for route logic

Files:

• src/api/v2/device/handlers/register.ts
• src/api/v1/notifications/handlers/register-installation.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/device/handlers/register.ts
• src/api/v1/notifications/handlers/register-installation.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/device/handlers/register.ts
• src/api/v1/notifications/handlers/register-installation.ts

**/{api,routes, middleware}/**/*.{ts,tsx}

📄 CodeRabbit inference engine (.cursorrules)

Validate and sanitize inputs

Files:

• src/api/v2/device/handlers/register.ts
• src/api/v1/notifications/handlers/register-installation.ts

🧠 Learnings (1)

📚 Learning: 2025-08-05T14:39:39.983Z

Learnt from: CR
PR: ephemeraHQ/convos-backend#0
File: .cursorrules:0-0
Timestamp: 2025-08-05T14:39:39.983Z
Learning: Applies to **/{api,routes}/**/*.{ts,tsx} : Always place the 'return;' statement on a new line immediately after sending a response with 'res.status().json()' or similar methods

Applied to files:

• src/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)

⏰ 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). (3)

• GitHub Check: push_to_registry
• GitHub Check: claude-review
• GitHub Check: Review for correctness

🔇 Additional comments (4)

src/api/v1/notifications/handlers/register-installation.ts (2)

1-1: LGTM: Path alias improves maintainability.

The migration from a deep relative path to the @prisma-zod/index alias is a good improvement that reduces brittleness and improves code readability.

---

83-84: LGTM: Explicit return follows coding guidelines.

The added explicit return; statement correctly follows the coding guideline to place return statements immediately after response methods.

prisma/migrations/20251017193536_add_unique_constraint_to_push_tokens/migration.sql (1)

1-8: Migration sequence is correct.

The cleanup step on line 5 is essential—it converts empty strings to NULL before the unique constraint is applied, preventing violations on existing data. The migration correctly sequences: make nullable → clean data → add constraint.

src/api/v2/device/handlers/register.ts (1)

69-83: Error handling structure and explicit returns are correct.

The function properly separates Zod validation errors (400) from server errors (500) and includes explicit return statements after each response, adhering to the coding guidelines.

[coderabbitai]

Actionable comments posted: 0

♻️ Duplicate comments (5)

src/api/v2/device/handlers/register.ts (2)

84-96: Handle Prisma unique constraint violations gracefully.

The schema enforces @@unique([pushTokenType, apnsEnv, pushToken]) (prisma/schema.prisma line 195). When a token is already registered to another device, Prisma throws a P2002 error, which currently falls through to the generic 500 handler. This is a user-recoverable scenario (device switch, app reinstall).

Apply this diff to handle P2002 errors specifically:

   } catch (error) {
     if (error instanceof z.ZodError) {
       req.log.warn(
         { errors: error.errors },
         "Invalid request body for register",
       );
       res.status(400).json({ error: "Invalid request body" });
       return;
     }
+    // Handle unique constraint violations (token already registered)
+    if (error && typeof error === 'object' && 'code' in error && error.code === 'P2002') {
+      req.log.warn(
+        { deviceId: body.deviceId, error },
+        "Push token already registered to another device",
+      );
+      res.status(409).json({ 
+        error: "Push token already registered to another device" 
+      });
+      return;
+    }
     req.log.error({ error }, "Failed to register device");
     res.status(500).json({ error: "Failed to register device" });
     return;
   }

---

7-12: Validate deviceId and pushToken formats to prevent invalid data.

The schema accepts arbitrary strings for deviceId (line 8) and allows empty strings for pushToken (line 9). This can lead to invalid data being persisted.

Apply this diff to enforce proper validation:

 const registerRequestSchema = z.object({
-  deviceId: z.string().min(1).max(255),
-  pushToken: z.string().optional(),
+  deviceId: z.string().uuid(),
+  pushToken: z.string().min(1).optional(),
   pushTokenType: PushTokenTypeSchema.optional(),
   apnsEnv: ApnsEnvironmentSchema.nullable().optional(),
 });

Rationale:

• deviceId uses @id in the Prisma schema (prisma/schema.prisma line 183), so enforcing UUID format prevents collisions and malformed IDs
• Empty pushToken strings should be rejected when provided; clients should omit the field entirely if no token is available

src/api/v2/notifications/handlers/subscribe.ts (3)

87-98: Add timeouts to all remote notification service calls.

None of the notificationClient calls (registerInstallation, subscribeWithMetadata, deleteInstallation) have timeouts. If the remote service hangs, request threads will block indefinitely, degrading availability.

Wrap each remote call with a timeout to prevent thread exhaustion.

Apply a timeout wrapper to all remote calls:

+const REMOTE_TIMEOUT_MS = 5000;
+
+const withTimeout = <T>(promise: Promise<T>, ms = REMOTE_TIMEOUT_MS): Promise<T> =>
+  Promise.race([
+    promise,
+    new Promise<never>((_, reject) =>
+      setTimeout(() => reject(new Error("Remote call timed out")), ms)
+    ),
+  ]);
+
 export async function subscribe(
   req: Request<unknown, unknown, ISubscribeRequestBody>,
   res: Response,
 ) {
   try {
     // ...
     try {
-      await notificationClient.registerInstallation({
+      await withTimeout(notificationClient.registerInstallation({
         installationId: body.clientId,
         deliveryMechanism: {
           deliveryMechanismType: {
             case:
               device.pushTokenType === "apns"
                 ? "apnsDeviceToken"
                 : "firebaseDeviceToken",
             value: device.pushToken,
           },
         },
-      });
+      }));
 
-      await notificationClient.subscribeWithMetadata({
+      await withTimeout(notificationClient.subscribeWithMetadata({
         installationId: body.clientId,
         subscriptions,
-      });
+      }));
     } catch (remoteErr) {
       try {
-        await notificationClient.deleteInstallation({
+        await withTimeout(notificationClient.deleteInstallation({
           installationId: body.clientId,
-        });
+        }));
       } catch (cleanupErr) {
         // ...
       }
       throw remoteErr;
     }
 
     try {
       // ...
     } catch (dbErr) {
       try {
-        await notificationClient.deleteInstallation({
+        await withTimeout(notificationClient.deleteInstallation({
           installationId: body.clientId,
-        });
+        }));
       } catch (cleanupErr) {
         // ...
       }
       throw dbErr;
     }

Consider extracting withTimeout to a shared utility module.

Also applies to: 101-104, 108-110, 134-136

---

91-94: Explicitly validate pushTokenType instead of defaulting to firebase.

The ternary defaults to "firebaseDeviceToken" for any pushTokenType that is not "apns". This silently accepts invalid or unsupported token types, masking configuration errors.

Explicitly check for supported values and return a 400 error for unsupported types.

Apply this diff:

-            case:
-              device.pushTokenType === "apns"
-                ? "apnsDeviceToken"
-                : "firebaseDeviceToken",
+            case: (() => {
+              if (device.pushTokenType === "apns") return "apnsDeviceToken";
+              if (device.pushTokenType === "firebase") return "firebaseDeviceToken";
+              req.log.warn(
+                { deviceId: body.deviceId, pushTokenType: device.pushTokenType },
+                "Unsupported push token type"
+              );
+              res.status(400).json({ error: "Unsupported push token type" });
+              return null as never;
+            })(),

Alternatively, throw an error or return early before the remote calls if pushTokenType is unsupported:

+    const deliveryCase =
+      device.pushTokenType === "apns"
+        ? "apnsDeviceToken"
+        : device.pushTokenType === "firebase"
+        ? "firebaseDeviceToken"
+        : null;
+
+    if (!deliveryCase) {
+      req.log.warn(
+        { deviceId: body.deviceId, pushTokenType: device.pushTokenType },
+        "Unsupported push token type"
+      );
+      res.status(400).json({ error: "Unsupported push token type" });
+      return;
+    }
+
     // Convert HMAC keys to Uint8Array
     const subscriptions = body.topics.map((topic) => ({
       topic: topic.topic,
       isSilent: false,
       hmacKeys: topic.hmacKeys.map((key) => ({
         thirtyDayPeriodsSinceEpoch: key.thirtyDayPeriodsSinceEpoch,
         key: hexToUint8Array(key.key),
       })),
     }));
 
     // Register installation with notification server
     try {
       await notificationClient.registerInstallation({
         installationId: body.clientId,
         deliveryMechanism: {
           deliveryMechanismType: {
-            case:
-              device.pushTokenType === "apns"
-                ? "apnsDeviceToken"
-                : "firebaseDeviceToken",
+            case: deliveryCase,
             value: device.pushToken,
           },
         },
       });

---

150-150: Add explicit return after sending 200 response.

The handler sends res.status(200).send() but does not explicitly return, violating the coding guideline: "Always place the 'return;' statement on a new line immediately after sending a response." Error paths (lines 162, 166) correctly include explicit returns; the success path should match.

As per coding guidelines

Apply this diff:

     res.status(200).send();
+    return;
   } catch (error) {

🧹 Nitpick comments (1)

src/api/v2/notifications/handlers/subscribe.ts (1)

78-78: Consider making isSilent configurable.

The isSilent flag is currently hardcoded to false for all subscriptions. Depending on future requirements, you may want to make this configurable per topic or per subscription request.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 03ad380 and 7aed79b.

📒 Files selected for processing (7)

• prisma/schema.prisma (2 hunks)
• src/api/shared/notifications/services/apns-push.service.ts (3 hunks)
• src/api/shared/notifications/services/push-notification.service.ts (2 hunks)
• src/api/shared/notifications/webhook-handler.ts (1 hunks)
• src/api/v2/device/handlers/register.ts (1 hunks)
• src/api/v2/notifications/handlers/subscribe.ts (1 hunks)
• tests/notifications-service.test.ts (2 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• tests/notifications-service.test.ts

🧰 Additional context used

📓 Path-based instructions (8)

**/*.{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/shared/notifications/services/push-notification.service.ts
• src/api/v2/notifications/handlers/subscribe.ts
• src/api/v2/device/handlers/register.ts
• src/api/shared/notifications/webhook-handler.ts
• src/api/shared/notifications/services/apns-push.service.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/shared/notifications/services/push-notification.service.ts
• src/api/v2/notifications/handlers/subscribe.ts
• src/api/v2/device/handlers/register.ts
• src/api/shared/notifications/webhook-handler.ts
• src/api/shared/notifications/services/apns-push.service.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/shared/notifications/services/push-notification.service.ts
• src/api/v2/notifications/handlers/subscribe.ts
• src/api/v2/device/handlers/register.ts
• src/api/shared/notifications/webhook-handler.ts
• src/api/shared/notifications/services/apns-push.service.ts

**/*

📄 CodeRabbit inference engine (.cursorrules)

Follow consistent file naming (kebab-case)

Files:

• src/api/shared/notifications/services/push-notification.service.ts
• src/api/v2/notifications/handlers/subscribe.ts
• src/api/v2/device/handlers/register.ts
• prisma/schema.prisma
• src/api/shared/notifications/webhook-handler.ts
• src/api/shared/notifications/services/apns-push.service.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/shared/notifications/services/push-notification.service.ts
• src/api/v2/notifications/handlers/subscribe.ts
• src/api/v2/device/handlers/register.ts
• src/api/shared/notifications/webhook-handler.ts
• src/api/shared/notifications/services/apns-push.service.ts

**/{api,routes, middleware}/**/*.{ts,tsx}

📄 CodeRabbit inference engine (.cursorrules)

Validate and sanitize inputs

Files:

• src/api/shared/notifications/services/push-notification.service.ts
• src/api/v2/notifications/handlers/subscribe.ts
• src/api/v2/device/handlers/register.ts
• src/api/shared/notifications/webhook-handler.ts
• src/api/shared/notifications/services/apns-push.service.ts

**/handlers/**/*.{ts,tsx}

📄 CodeRabbit inference engine (.cursorrules)

Use dedicated handlers directory for route logic

Files:

• src/api/v2/notifications/handlers/subscribe.ts
• src/api/v2/device/handlers/register.ts

**/*.prisma

📄 CodeRabbit inference engine (.cursorrules)

**/*.prisma: Follow schema naming conventions (PascalCase for models)
Use appropriate field types and constraints
Add indexes for frequently queried fields
Use appropriate database indexes

Files:

• prisma/schema.prisma

🧠 Learnings (1)

📚 Learning: 2025-08-05T14:39:39.983Z

Learnt from: CR
PR: ephemeraHQ/convos-backend#0
File: .cursorrules:0-0
Timestamp: 2025-08-05T14:39:39.983Z
Learning: Applies to **/{api,routes}/**/*.{ts,tsx} : Always place the 'return;' statement on a new line immediately after sending a response with 'res.status().json()' or similar methods

Applied to files:

• src/api/v2/device/handlers/register.ts

🧬 Code graph analysis (4)

src/api/v2/notifications/handlers/subscribe.ts (2)

src/notifications/client.ts (1)

• createNotificationClient (14-20)

src/utils/prisma.ts (1)

• prisma (3-3)

src/api/v2/device/handlers/register.ts (1)

src/utils/prisma.ts (1)

• prisma (3-3)

src/api/shared/notifications/webhook-handler.ts (7)

src/notifications/client.ts (3)

• createNotificationClient (14-20)
• webhookNotificationBodySchema (28-54)
• WebhookNotificationBody (56-58)

src/api/shared/notifications/services/push-notification.service.ts (1)

• getPushNotificationService (233-238)

src/utils/prisma.ts (1)

• prisma (3-3)

src/api/shared/notifications/constants.ts (1)

• MAX_PUSH_FAILURES (4-4)

src/utils/v2/jwt.ts (1)

• createV2JwtToken (27-69)

src/api/shared/notifications/services/apns-push.service.ts (1)

• createApnsService (273-295)

src/api/shared/notifications/services/notifications-types.ts (1)

• V2NotificationPayload (63-68)

src/api/shared/notifications/services/apns-push.service.ts (1)

src/api/shared/notifications/services/notifications-types.ts (1)

• AnyNotificationPayloadWithJWT (71-73)

⏰ 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: claude-review
• GitHub Check: Review for correctness

🔇 Additional comments (13)

src/api/shared/notifications/services/apns-push.service.ts (2)

6-6: LGTM: Type updated to support v1 and v2 payloads.

The change from NotificationPayloadWithJWTToken to AnyNotificationPayloadWithJWT correctly supports the union of v1 and v2 notification payload types introduced in this PR.

Also applies to: 78-78

---

140-144: LGTM: Authorization header properly redacted in logs.

Good security practice to redact sensitive authorization tokens before logging request headers.

src/api/shared/notifications/services/push-notification.service.ts (2)

108-115: LGTM: Defensive guard prevents JWT creation without inboxId.

The early return when inboxId is missing prevents unnecessary JWT token creation and provides clear error logging. This aligns with the v1 notification requirements.

---

159-165: LGTM: Error severity and messaging updated appropriately.

Upgrading FCM to error level and clarifying the message is appropriate since FCM is explicitly unsupported (as noted in schema.prisma line 30).

prisma/schema.prisma (2)

27-31: LGTM: Clear documentation of supported push token types.

The comment clarifies that only APNS is supported, and the enum value for fcm is retained for database compatibility. This aligns with the error handling in the push notification services.

---

181-208: LGTM: V2 models are well-structured with appropriate constraints.

The new DeviceRegistration and ClientIdentifier models have:

• Proper indexes for query optimization (pushToken, disabled+pushFailures, deviceId)
• Appropriate unique constraint to prevent token reuse across devices
• Correct CASCADE deletion to maintain referential integrity
• Reasonable defaults for tracking push failures and timestamps

Note: Ensure request handlers validate deviceId and clientIdentifier formats before persisting (flagged in register.ts review).

src/api/shared/notifications/webhook-handler.ts (6)

26-31: LGTM: Welcome message detection prevents APNS payload overflow.

The dual detection strategy (content topic pattern /w- and message type v3-welcome) correctly identifies large MLS welcome messages that exceed the APNS 4KB limit. Omitting encrypted content for these messages is the right approach.

---

167-168: Return statement placement is correct.

The return is properly placed on a new line after res.status(200).end(), following coding guidelines.

---

175-215: LGTM: V1 cleanup logic properly ordered and transactional.

The cleanup sequence is correct:

1. Atomic DB transaction clears xmtpInstallationId and push token, increments failures
2. Then attempts XMTP server cleanup

If XMTP deletion fails, local state is already consistent. Good error handling and logging throughout.

---

302-336: LGTM: Atomic failure tracking and conditional disable.

The transaction properly:

1. Atomically increments pushFailures and sets lastFailureAt
2. Conditionally disables the device only if not already disabled and threshold reached (using updateMany with WHERE clause to avoid races)

This addresses the concurrency concerns from past reviews.

---

350-380: LGTM: V2 cleanup prioritizes local consistency.

The cleanup sequence correctly:

1. Deletes from local DB first (line 352-354)
2. Then attempts XMTP deletion with separate try-catch (lines 357-367)
3. Logs but doesn't fail if XMTP deletion fails

This ensures the local database remains the source of truth. Good error handling strategy.

---

87-87: Place return statement on a new line after sending response.

Per coding guidelines, the return statement should be on a separate line immediately after res.status(200).end() to make control flow explicit.

Apply this diff:

-      res.status(200).end();
-      return;
+      res.status(200).end();
+      return;

Note: Current code already has the return on line 88, but ensure consistent formatting. Based on coding guidelines.

⛔ Skipped due to learnings

Learnt from: CR
PR: ephemeraHQ/convos-backend#0
File: .cursorrules:0-0
Timestamp: 2025-08-05T14:39:39.983Z
Learning: Applies to **/{api,routes}/**/*.{ts,tsx} : Always place the 'return;' statement on a new line immediately after sending a response with 'res.status().json()' or similar methods

src/api/v2/notifications/handlers/subscribe.ts (1)

122-130: Verify the intended semantics of clientId device reassignment.

The update: { deviceId: body.deviceId } clause allows an existing clientId to be reassociated with a different device. This could be intentional (e.g., for device migration or testing) or could mask a bug where a clientId is accidentally reused across devices.

Confirm whether:

• A single clientId should be permanently tied to one device, or
• Device reassignment is expected (e.g., user reinstalls app and reuses clientId).

If reassignment is unintended, add validation to reject requests where the existing clientId has a different deviceId.

Consider adding a check before the upsert:

const existingClient = await prisma.clientIdentifier.findUnique({
  where: { id: body.clientId },
});

if (existingClient && existingClient.deviceId !== body.deviceId) {
  req.log.warn(
    { clientId: body.clientId, existingDeviceId: existingClient.deviceId, newDeviceId: body.deviceId },
    "Attempt to reassign clientId to different device"
  );
  res.status(409).json({ error: "Client ID already associated with a different device" });
  return;
}

[coderabbitai]

Actionable comments posted: 0

♻️ Duplicate comments (3)

src/api/v2/notifications/handlers/subscribe.ts (3)

85-118: Add timeouts to all remote notification client calls.

Remote calls to registerInstallation, subscribeWithMetadata, and deleteInstallation lack timeout guards, which can cause request threads to hang indefinitely if the notification service is slow or unresponsive. This degrades user experience and can exhaust server resources.

Apply this pattern to wrap all remote calls with a timeout utility:

+const REMOTE_TIMEOUT_MS = 5000;
+
+function withTimeout<T>(promise: Promise<T>, ms = REMOTE_TIMEOUT_MS): Promise<T> {
+  return Promise.race([
+    promise,
+    new Promise<T>((_, reject) =>
+      setTimeout(() => reject(new Error("Remote call timed out")), ms)
+    ),
+  ]);
+}
+
 // Register installation with notification server
 try {
-  await notificationClient.registerInstallation({
+  await withTimeout(notificationClient.registerInstallation({
     installationId: body.clientId,
     deliveryMechanism: {
       deliveryMechanismType: {
         case:
           device.pushTokenType === "apns"
             ? "apnsDeviceToken"
             : "firebaseDeviceToken",
         value: device.pushToken,
       },
     },
-  });
+  }));

   // Subscribe to topics
-  await notificationClient.subscribeWithMetadata({
+  await withTimeout(notificationClient.subscribeWithMetadata({
     installationId: body.clientId,
     subscriptions,
-  });
+  }));
 } catch (remoteErr) {
   // Compensate: best-effort delete installation to avoid orphaned state
   try {
-    await notificationClient.deleteInstallation({
+    await withTimeout(notificationClient.deleteInstallation({
       installationId: body.clientId,
-    });
+    }));
   } catch (cleanupErr) {

Consider extracting withTimeout to a shared utility module for reuse across the codebase.

---

91-94: Validate pushTokenType explicitly instead of defaulting.

The ternary operator silently defaults to "firebaseDeviceToken" for any pushTokenType that isn't "apns". This masks configuration errors and could send push notifications via the wrong delivery mechanism if an unsupported or misspelled token type is stored in the database.

Apply this diff to fail fast on unsupported token types:

+    const deliveryCase =
+      device.pushTokenType === "apns"
+        ? "apnsDeviceToken"
+        : device.pushTokenType === "firebase"
+        ? "firebaseDeviceToken"
+        : null;
+
+    if (!deliveryCase) {
+      req.log.warn(
+        { deviceId: body.deviceId, pushTokenType: device.pushTokenType },
+        "Unsupported push token type",
+      );
+      res.status(400).json({ error: "Unsupported push token type" });
+      return;
+    }
+
     await notificationClient.registerInstallation({
       installationId: body.clientId,
       deliveryMechanism: {
         deliveryMechanismType: {
-          case:
-            device.pushTokenType === "apns"
-              ? "apnsDeviceToken"
-              : "firebaseDeviceToken",
+          case: deliveryCase,
           value: device.pushToken,
         },
       },
     });

---

146-150: Add explicit return after sending response.

Per coding guidelines, a return; statement should immediately follow response sends to ensure consistent control flow and prevent accidental code execution.

As per coding guidelines

Apply this diff:

     req.log.info(
       { deviceId: body.deviceId, clientId: body.clientId },
       "Subscribed successfully",
     );
     res.status(200).send();
+    return;
   } catch (error) {

🧹 Nitpick comments (4)

src/api/v2/auth/handlers/generate-token.ts (1)

29-31: Consider adding JSDoc for the public API handler.

Per coding guidelines, public APIs should include JSDoc comments. While the security model comment above is excellent, adding a brief JSDoc directly on this function would improve discoverability and IDE support.

Example:

+/**
+ * Generates a short-lived JWT token for V2 NSE/Gateway authentication.
+ * Requires AppCheck validation via outer middleware.
+ * @param req - Express request with validated deviceId in body
+ * @param res - Express response
+ */
 export async function generateToken(
   req: Request<unknown, unknown, IGenerateTokenRequestBody>,
   res: Response,
 ) {

src/api/v2/invites/handlers/decode-invite-slug.ts (2)

144-150: Add fallback for expiresAfterUse to ensure boolean type.

While proto3 booleans default to false, TypeScript typing may allow undefined for backward compatibility with old payloads. Ensure the response always contains a boolean.

Apply this diff to add an explicit fallback:

         expiresAt: decoded.expiresAt
           ? timestampDate(decoded.expiresAt).toISOString()
           : null,
-        expiresAfterUse: decoded.expiresAfterUse,
+        expiresAfterUse: decoded.expiresAfterUse ?? false,

---

104-106: Consider adding explicit default for expiresAfterUse.

When decoding old-format invites that lack the expiresAfterUse field, ensure the value is explicitly set to false for consistency.

Apply this diff:

       conversationExpiresAt: payload.conversationExpiresAt,
       expiresAt: payload.expiresAt,
-      expiresAfterUse: payload.expiresAfterUse,
+      expiresAfterUse: payload.expiresAfterUse ?? false,

src/api/v2/device/handlers/register.ts (1)

67-119: Transaction logic for push token transfer is well-implemented.

The transaction correctly handles the scenario where a push token is transferred from an old device to a new one, preventing unique constraint violations. The conflict detection, old device clearing, and upsert flow are sound.

Consider extracting the default values into constants to reduce duplication and clarify intent:

+const DEFAULT_PUSH_TOKEN_TYPE = "apns" as const;
+const DEFAULT_APNS_ENV = null;
+
 export async function register(
   req: Request<unknown, unknown, IRegisterRequestBody>,
   res: Response,
 ) {
   try {
     ...
       if (body.pushToken) {
-        const pushTokenType = body.pushTokenType ?? "apns";
-        const apnsEnv = body.apnsEnv ?? null;
+        const pushTokenType = body.pushTokenType ?? DEFAULT_PUSH_TOKEN_TYPE;
+        const apnsEnv = body.apnsEnv ?? DEFAULT_APNS_ENV;
         ...
           await tx.deviceRegistration.update({
             where: { deviceId: existingDevice.deviceId },
             data: {
               pushToken: null,
-              pushTokenType: "apns",
-              apnsEnv: null,
+              pushTokenType: DEFAULT_PUSH_TOKEN_TYPE,
+              apnsEnv: DEFAULT_APNS_ENV,
             },
           });
         ...
       await tx.deviceRegistration.upsert({
         where: { deviceId: body.deviceId },
         create: {
           deviceId: body.deviceId,
           pushToken: body.pushToken ?? null,
-          pushTokenType: body.pushTokenType ?? "apns",
-          apnsEnv: body.apnsEnv ?? null,
+          pushTokenType: body.pushTokenType ?? DEFAULT_PUSH_TOKEN_TYPE,
+          apnsEnv: body.apnsEnv ?? DEFAULT_APNS_ENV,
         },
         update: updateData,
       });

Additionally, consider adding a brief comment at line 101 explaining why we set pushTokenType when clearing the token (it's required by the schema even though the token itself is null).

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7aed79b and 6e0c0f6.

📒 Files selected for processing (8)

• proto/invite/v2/invite.proto (2 hunks)
• src/api/v2/auth/handlers/generate-token.ts (1 hunks)
• src/api/v2/device/handlers/register.ts (1 hunks)
• src/api/v2/invites/handlers/decode-invite-slug.ts (5 hunks)
• src/api/v2/notifications/handlers/subscribe.ts (1 hunks)
• src/api/v2/notifications/handlers/unregister.ts (1 hunks)
• src/api/v2/notifications/handlers/unsubscribe.ts (1 hunks)
• tests/invites-v2.test.ts (2 hunks)

🚧 Files skipped from review as they are similar to previous changes (2)

• src/api/v2/notifications/handlers/unregister.ts
• src/api/v2/notifications/handlers/unsubscribe.ts

🧰 Additional context used

📓 Path-based instructions (8)

**/*.{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:

• tests/invites-v2.test.ts
• src/api/v2/invites/handlers/decode-invite-slug.ts
• src/api/v2/auth/handlers/generate-token.ts
• src/api/v2/device/handlers/register.ts
• src/api/v2/notifications/handlers/subscribe.ts

**/*

📄 CodeRabbit inference engine (.cursorrules)

Follow consistent file naming (kebab-case)

Files:

• tests/invites-v2.test.ts
• src/api/v2/invites/handlers/decode-invite-slug.ts
• src/api/v2/auth/handlers/generate-token.ts
• proto/invite/v2/invite.proto
• src/api/v2/device/handlers/register.ts
• src/api/v2/notifications/handlers/subscribe.ts

**/*.{test,spec}.{ts,tsx}

📄 CodeRabbit inference engine (.cursorrules)

**/*.{test,spec}.{ts,tsx}: Write tests for API endpoints
Use helper functions for test setup
Mock external services
Test error cases
Follow AAA pattern (Arrange, Act, Assert)

Files:

• tests/invites-v2.test.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/invites/handlers/decode-invite-slug.ts
• src/api/v2/auth/handlers/generate-token.ts
• src/api/v2/device/handlers/register.ts
• src/api/v2/notifications/handlers/subscribe.ts

**/handlers/**/*.{ts,tsx}

📄 CodeRabbit inference engine (.cursorrules)

Use dedicated handlers directory for route logic

Files:

• src/api/v2/invites/handlers/decode-invite-slug.ts
• src/api/v2/auth/handlers/generate-token.ts
• src/api/v2/device/handlers/register.ts
• src/api/v2/notifications/handlers/subscribe.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/invites/handlers/decode-invite-slug.ts
• src/api/v2/auth/handlers/generate-token.ts
• src/api/v2/device/handlers/register.ts
• src/api/v2/notifications/handlers/subscribe.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/invites/handlers/decode-invite-slug.ts
• src/api/v2/auth/handlers/generate-token.ts
• src/api/v2/device/handlers/register.ts
• src/api/v2/notifications/handlers/subscribe.ts

**/{api,routes, middleware}/**/*.{ts,tsx}

📄 CodeRabbit inference engine (.cursorrules)

Validate and sanitize inputs

Files:

• src/api/v2/invites/handlers/decode-invite-slug.ts
• src/api/v2/auth/handlers/generate-token.ts
• src/api/v2/device/handlers/register.ts
• src/api/v2/notifications/handlers/subscribe.ts

🧠 Learnings (1)

📚 Learning: 2025-08-05T14:39:39.983Z

Learnt from: CR
PR: ephemeraHQ/convos-backend#0
File: .cursorrules:0-0
Timestamp: 2025-08-05T14:39:39.983Z
Learning: Applies to **/{api,routes}/**/*.{ts,tsx} : Always place the 'return;' statement on a new line immediately after sending a response with 'res.status().json()' or similar methods

Applied to files:

• src/api/v2/auth/handlers/generate-token.ts
• src/api/v2/device/handlers/register.ts

🧬 Code graph analysis (3)

src/api/v2/auth/handlers/generate-token.ts (2)

src/utils/prisma.ts (1)

• prisma (3-3)

src/utils/v2/jwt.ts (1)

• createV2JwtToken (27-69)

src/api/v2/device/handlers/register.ts (1)

src/utils/prisma.ts (1)

• prisma (3-3)

src/api/v2/notifications/handlers/subscribe.ts (2)

src/notifications/client.ts (1)

• createNotificationClient (14-20)

src/utils/prisma.ts (1)

• prisma (3-3)

🪛 GitHub Actions: Validate code quality

tests/invites-v2.test.ts

[error] 1-1: Test failure: Expected HTTP 200 but received 400 for authentication path in V2 Invite API tests

⏰ 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: push_to_registry
• GitHub Check: claude-review

🔇 Additional comments (19)

src/api/v2/auth/handlers/generate-token.ts (2)

21-23: UUID validation properly addresses empty string concern.

The .uuid() validator is stricter than the previously suggested .nonempty() and correctly rejects empty strings along with any non-UUID format. This fully addresses the past concern about empty deviceId values passing validation.

---

29-74: Excellent implementation—all past issues resolved.

The handler correctly implements the token generation flow with proper error handling, explicit returns after all responses, and safe null checking using optional chaining. All concerns from previous reviews have been addressed:

• Explicit returns are present after every response (lines 46, 60, 68, 72)
• The device?.disabled check safely handles null devices
• UUID validation prevents empty deviceId values

The logic correctly allows token generation for unregistered devices while rejecting disabled ones, matching the documented security model.

tests/invites-v2.test.ts (1)

88-124: LGTM! Backward-compatible test coverage for new fields.

The test correctly validates that old-format invites (without the new expiration fields) return the expected defaults: null for optional timestamps and false for the boolean flag. This ensures backward compatibility.

proto/invite/v2/invite.proto (2)

3-3: LGTM! Correct import for Timestamp fields.

The google.protobuf.Timestamp import is properly added to support the new expiration timestamp fields.

---

22-28: LGTM! Well-structured expiration fields.

The three new fields are well-designed:

• Optional timestamps for flexible expiration tracking
• Non-optional boolean for single-use behavior (defaults to false in proto3)
• Sequential field numbering (7, 8, 9) avoids conflicts
• Clear comments documenting purpose

src/api/v2/invites/handlers/decode-invite-slug.ts (2)

3-3: LGTM! Correct import for timestamp conversion.

The timestampDate utility from @bufbuild/protobuf/wkt is the proper way to convert protobuf Timestamp messages to JavaScript Date objects.

---

29-31: LGTM! Type extension matches proto definition.

The DecodedInvite type correctly includes the three new fields from the InvitePayload proto schema.

src/api/v2/notifications/handlers/subscribe.ts (8)

1-5: LGTM: Clean imports with proper path aliases.

The imports follow the project's path alias conventions and include all necessary dependencies for the handler.

---

7-24: LGTM: Robust schema with proper validation.

The schema includes UUID validation, array bounds (1-100 topics), and hex string validation for HMAC keys. The regex pattern for hex validation addresses the previous concern about runtime exceptions from malformed hex strings.

---

26-28: LGTM: Clean type inference and client initialization.

The type is properly inferred from the schema, and module-level client initialization is appropriate for a stateless notification client.

---

46-73: LGTM: Comprehensive device guards with proper error handling.

All necessary device validations are in place (existence, disabled state, push token presence) with appropriate status codes and explicit returns.

---

75-83: LGTM: Proper HMAC key transformation.

The hex-to-binary conversion is safe because the schema validates hex format upstream. The transformation correctly maps the request structure to the notification client's expected format.

---

120-144: LGTM: Proper upsert with compensating cleanup.

The upsert now correctly updates deviceId in the update clause (addressing the previous empty-update issue), and includes compensating cleanup to maintain consistency between the database and remote notification service if the database operation fails.

---

151-167: LGTM: Comprehensive error handling without secret leakage.

The error handling properly distinguishes validation errors (400) from system errors (500), includes helpful hints in the validation response, and avoids logging sensitive data like HMAC keys.

---

30-33: Use object parameter syntax per coding guidelines.

The function signature should use object parameter syntax for better readability and extensibility.

As per coding guidelines

Apply this diff:

-export async function subscribe(
-  req: Request<unknown, unknown, ISubscribeRequestBody>,
-  res: Response,
-) {
+export async function subscribe(args: {
+  req: Request<unknown, unknown, ISubscribeRequestBody>;
+  res: Response;
+}) {
+  const { req, res } = args;

Likely an incorrect or invalid review comment.

src/api/v2/device/handlers/register.ts (4)

1-15: Schema validation looks solid.

The Zod schema correctly validates the request structure, and the pushToken transform (lines 10-12) properly normalizes empty strings to undefined, addressing previous feedback.

---

26-37: FCM rejection is clear and properly handled.

The explicit rejection of FCM tokens with an informative error message is good. The return statement on line 36 correctly follows the coding guideline.

---

49-65: Conditional update logic is well-designed.

Building the update object conditionally (only including explicitly provided fields) is the right approach and avoids unintended overwrites during partial updates.

---

128-159: Error handling is comprehensive and well-structured.

The error paths correctly differentiate validation errors (400), unique constraint violations (409), and unexpected errors (500), with explicit returns on all branches per coding guidelines. The P2002 handler appropriately acknowledges potential race conditions and instructs clients to retry.