fix(notifications): heal NULL DeviceRegistration.accountId push drops#335
fix(notifications): heal NULL DeviceRegistration.accountId push drops#335lourou wants to merge 1 commit into
Conversation
|
Claude finished @lourou's task —— View job Code Review Complete ✅I've thoroughly reviewed this PR. The implementation is well-designed and thoughtful, with excellent documentation and test coverage. Below are my findings across the requested areas: Code Quality & Best Practices ✨Strengths:
Minor suggestions:
Potential Bugs & Issues 🐛Critical finding from existing review comments:
No new bugs identified in the current implementation:
Performance Considerations ⚡Efficient implementation:
Scale considerations:
Security Concerns 🔒Strong security posture:
The removed migration was the right security decision - inferring ownership from historical Test Coverage 🧪Excellent coverage (222 lines):
Coverage gaps (minor):
Additional Observations 📋Backwards compatibility (per CLAUDE.md requirements):
Code follows repository conventions:
Documentation quality:
SummaryThis is a high-quality fix that addresses a real production issue (~121 affected devices) with a thoughtful, security-conscious approach:
Recommendation: The code is ready to merge pending any additional reviewer-specific concerns. The only nitpick is the explicit return type annotation, which is a minor style inconsistency with project conventions but doesn't affect correctness. |
WalkthroughThe subscribe handler now persists subscription identity inside one Prisma transaction. A new helper upserts ChangesSubscription identity transaction flow
Sequence Diagram(s)sequenceDiagram
participant Client
participant SubscribeHandler as "subscribe handler"
participant PrismaTransaction as "prisma.$transaction"
participant PersistSubscriptionIdentity as persistSubscriptionIdentity
participant ClientIdentifierUpsert as "clientIdentifier.upsert"
participant DeviceRegistrationUpdateMany as "deviceRegistration.updateMany"
Client->>SubscribeHandler: submit subscription identity
SubscribeHandler->>PrismaTransaction: call transaction with tx callback
PrismaTransaction->>PersistSubscriptionIdentity: invoke callback with tx
PersistSubscriptionIdentity->>ClientIdentifierUpsert: upsert deviceId and accountId
opt accountId provided
PersistSubscriptionIdentity->>DeviceRegistrationUpdateMany: updateMany accountId adoption predicate
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
ApprovabilityVerdict: Needs human review This PR changes runtime behavior in the notification subscribe flow by now writing DeviceRegistration.accountId when it's NULL or differs from the subscriber. This gates push delivery logic and affects which notifications get delivered vs dropped. The changed files are owned by xmtplabs/engineering, who should review this behavioral change. You can customize Macroscope's approvability policy. Learn more. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/api/v2/notifications/handlers/subscribe.ts (2)
63-67: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winOmit
accountIdfromcreatewhen it is absent.The update branch already avoids writing
accountIdfor legacy JWTs; the create branch still passesaccountId: undefined, relying on Prisma to omit it. Use the same conditional spread here and update the test expectation accordingly.Suggested fix
create: { id: clientId, deviceId, - accountId, + ...(accountId !== undefined ? { accountId } : {}), },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/v2/notifications/handlers/subscribe.ts` around lines 63 - 67, The subscribe handler’s create payload is still always including accountId, which can pass undefined for legacy JWTs; update the create branch in subscribe.ts to use the same conditional spread approach as the update branch so accountId is only present when defined, and adjust the related subscribe test expectation to match the omitted field behavior.
53-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLet TypeScript infer this helper’s return type.
Remove
: Promise<void>to match the project’ssrc/**/*TypeScript style. As per coding guidelines, "src/**/*.{ts,tsx}: Don't specify return type on functions. Prefer inferring the value in TypeScript".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/v2/notifications/handlers/subscribe.ts` around lines 53 - 58, The helper persistSubscriptionIdentity currently hardcodes a Promise<void> return annotation, which conflicts with the TypeScript style used under src/**. Remove the explicit return type from the function signature and let TypeScript infer it, keeping the change localized to persistSubscriptionIdentity and any similar helper signatures in this area if present.Source: Coding guidelines
tests/notifications-subscribe-identity.test.ts (1)
121-138: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftCover the handler’s
$transactionwiring, not just the helper.This proves the helper uses the injected
tx, but it would still pass ifsubscribestopped wrapping it inprisma.$transaction. Add a handler-level test that mocksprisma.$transactionand asserts the callback uses that transaction client.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/notifications-subscribe-identity.test.ts` around lines 121 - 138, The current test only verifies persistSubscriptionIdentity uses the injected tx, but it does not prove subscribe still wraps the helper in prisma.$transaction. Add a handler-level test around subscribe that mocks prisma.$transaction and asserts the callback receives and uses the transaction client, so both writes are verified through the same transaction path. Use the existing subscribe and persistSubscriptionIdentity symbols to place the new assertion alongside the current transaction-client coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dev/scripts/backfillDeviceAccountId.ts`:
- Around line 94-101: The shutdown flow in main() currently ignores the Prisma
disconnect promise by using void in the finally block, which can hide cleanup
failures. Update the promise chain around main() so prisma.$disconnect() is
returned from the finally handler and any disconnect error is handled in the
following catch, keeping the existing logger and process exit behavior intact.
In
`@prisma/migrations/20260625120000_backfill_device_registration_account_id/migration.sql`:
- Around line 41-45: The backfill query in the migration can still select a
unanimous `ClientIdentifier.accountId` value that does not exist in `Account`,
which will later fail when updating `DeviceRegistration.accountId`. Update the
`SELECT` used by the backfill to keep the unanimity check in `ClientIdentifier`,
then add an `Account` join or `EXISTS` filter before the update so only account
IDs present in `Account` are applied. Use the migration’s backfill query block
and the `ClientIdentifier`/`Account` tables as the fix point.
---
Nitpick comments:
In `@src/api/v2/notifications/handlers/subscribe.ts`:
- Around line 63-67: The subscribe handler’s create payload is still always
including accountId, which can pass undefined for legacy JWTs; update the create
branch in subscribe.ts to use the same conditional spread approach as the update
branch so accountId is only present when defined, and adjust the related
subscribe test expectation to match the omitted field behavior.
- Around line 53-58: The helper persistSubscriptionIdentity currently hardcodes
a Promise<void> return annotation, which conflicts with the TypeScript style
used under src/**. Remove the explicit return type from the function signature
and let TypeScript infer it, keeping the change localized to
persistSubscriptionIdentity and any similar helper signatures in this area if
present.
In `@tests/notifications-subscribe-identity.test.ts`:
- Around line 121-138: The current test only verifies
persistSubscriptionIdentity uses the injected tx, but it does not prove
subscribe still wraps the helper in prisma.$transaction. Add a handler-level
test around subscribe that mocks prisma.$transaction and asserts the callback
receives and uses the transaction client, so both writes are verified through
the same transaction path. Use the existing subscribe and
persistSubscriptionIdentity symbols to place the new assertion alongside the
current transaction-client coverage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a396357d-3e2c-4872-b794-9a9345de702c
📒 Files selected for processing (4)
dev/scripts/backfillDeviceAccountId.tsprisma/migrations/20260625120000_backfill_device_registration_account_id/migration.sqlsrc/api/v2/notifications/handlers/subscribe.tstests/notifications-subscribe-identity.test.ts
lourou
force-pushed
the
fix/push-notif-null-accountid
branch
from
73b0a00 to
2751220
Compare
lourou
force-pushed
the
fix/push-notif-null-accountid
branch
from
2751220 to
8203c79
Compare
A NULL DeviceRegistration.accountId was silently dropped by the
isAccountIdMismatch webhook guard (it fails closed unless the
ClientIdentifier and DeviceRegistration accountIds are equal and
non-null), so an affected device received no pushes at all.
On an authenticated subscribe, write the subscriber's accountId onto
the device transactionally alongside the ClientIdentifier upsert:
set DeviceRegistration.accountId when it is NULL or differs from the
subscriber ("current-subscriber-wins"). Only an accountId-bearing JWT
mutates anything, so legacy/non-SIWE builds never clobber a prior or
backfilled value. The guard still fails closed, so a stale device
accountId can only ever drop a push, never deliver cross-account; the
state self-heals on the next authenticated subscribe.
Logic is extracted into persistSubscriptionIdentity and unit-tested
(adopt-on-null, overwrite-on-differ, untouched-without-accountId,
same-transaction), plus a handler-level test asserting subscribe wraps
both writes in one prisma.$transaction.
lourou
force-pushed
the
fix/push-notif-null-accountid
branch
from
8203c79 to
b126653
Compare
|
@claude re the iOS token-recycling leak (Critical #2) and the Account FK guard (Critical #1): both are about the backfill migration, which we've removed from the branch. We took the "skip the migration, rely on the subscribe-time fix" option — so the token-recycling leak and the FK concern are both moot. Updated the PR description to match. Guard stays fail-closed. |
|
Addressed in b126653 — conditional-spread on the |
2 participants
Push notifications: heal NULL
DeviceRegistration.accountIddropsProblem: pushes were silently dropped for devices whose
DeviceRegistration.accountIdis NULL — theisAccountIdMismatchguard (added in #272, fails closed) drops them. ~121 active devices affected (prod DB), plus a legacy tail.Root cause:
accountIdwas never reliably written toDeviceRegistration—/v2/device/registeris AppCheck-only and doesn't set it, and the/auth/tokenSIWE backfillupdateManyno-ops when the device row doesn't exist yet (register/auth are independent and unordered).Fix:
subscribe.tsstampsDeviceRegistration.accountIdfrom the authenticated subscriber, in the same transaction as theClientIdentifierupsert (current-subscriber-wins; fails closed; no request-schema change → backwards-compatible). Self-heals every active device on its next authenticated subscribe.Backfill migration intentionally dropped: an earlier revision included a one-shot data migration to backfill existing NULLs; it was removed because inferring current ownership from historical
ClientIdentifierrows is unsound under iOS token recycling (cross-account delivery risk flagged by macroscope + the Claude review). We rely on the leak-safe subscribe-side self-heal instead.Guard unchanged (kept fail-closed). Tests added;
tscclean.Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Fix NULL
DeviceRegistration.accountIdby stamping account during push subscriptionpersistSubscriptionIdentityinsubscribe.tsto upsertClientIdentifierand conditionally updateDeviceRegistration.accountIdwhere it is NULL or stale, within a single transaction.subscribehandler now wraps both writes inprisma.$transaction, ensuring the account adoption and identity upsert are atomic.accountIdis omitted fromClientIdentifiercreate/update when undefined, preventing unguarded NULL writes.notifications-subscribe-identity.test.tscovers authenticated/unauthenticated paths and verifies transactional wrapping.Macroscope summarized b126653.
Summary by CodeRabbit