Uh oh!
There was an error while loading. Please reload this page.
Fix/bad notif token + system push notifs - #925
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThe PR updates notification flows to manage DeviceToken storage: NotificationService and MessageNotificationService now accept a DeviceToken repository, device registration removes stale tokens, notifications are delivered sequentially with per-token error handling and bad-token removal, and system-message detection was added to message notifications. Changes
Sequence DiagramsequenceDiagram
participant App as Client/App
participant NS as NotificationService
participant DT as DeviceToken Repo
participant Verif as Verification Repo
participant PP as Push Provider
App->>NS: registerDevice(linkedEName, deviceId, pushToken)
NS->>Verif: Ensure verification exists
NS->>DT: Query tokens by (linkedEName, deviceId)
DT-->>NS: existingTokens
alt existingTokens contain others
loop For each stale token (not current)
NS->>DT: remove stale token
NS->>Verif: remove token reference
end
end
NS->>DT: store current pushToken
App->>NS: sendNotificationToEName(payload)
NS->>NS: detect $$system-message$$ prefix (if any)
loop For each token (sequential)
NS->>PP: send push to token (HTTP)
alt success
PP-->>NS: 200 OK
NS->>NS: mark delivered, stop if desired
else failure
PP-->>NS: error
NS->>NS: classify bad token
NS->>DT: remove bad token
NS->>Verif: remove bad token reference
end
end
NS-->>App: delivery result (true/false)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
coodos
marked this pull request as ready for review
March 16, 2026 08:58
Contributor
There was a problem hiding this comment.
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 (2)
infrastructure/evault-core/src/services/MessageNotificationService.ts (1)
80-104:⚠️ Potential issue | 🟠 MajorDon't infer a trusted system notification from user-controlled text.
Any sender can prefix content with
$$system-message$$and get a push titled as a system message. That makes system notifications spoofable; this should come from a server-set flag/type or a trusted-sender check, not from the message body.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/evault-core/src/services/MessageNotificationService.ts` around lines 80 - 104, The code currently treats any message whose payload.content starts with "$$system-message$$" (rawText and isSystemMessage) as a system notification, which is spoofable; change the logic in the MessageNotificationService where rawText/isSystemMessage/messageText are used so system notifications come from trusted metadata instead of user-controlled content — e.g., check a server-set flag on the payload (payload.type === 'system' or payload.isSystem) or validate the sender via a trusted-sender check (use senderEName against a trusted list or call an isTrustedSender helper) before setting title/body as a system message; retain removing the marker from display text only for legacy compatibility if and only if the message is validated as a system message by the trusted flag/sender check.infrastructure/evault-core/src/services/NotificationService.ts (1)
52-64:⚠️ Potential issue | 🟡 MinorClean the previous owner's
device_tokenrow here too.This removes a transferred token from other users'
Verification.pushTokens, but it leaves the olddevice_token.tokensentry behind. After an account switch, the same token can still show up under both eNames in the DeviceToken-backed listing endpoints unless this path updates both stores.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/evault-core/src/services/NotificationService.ts` around lines 52 - 64, The current NotificationService flow removes the token from other Verification.pushTokens but doesn't remove it from the DeviceToken store, leaving device_token.tokens entries that still reference the old owner; update the same path in NotificationService (after the loop that updates verificationRepository entries) to query deviceTokenRepository for DeviceToken rows where :token = ANY(device_token.tokens) and linkedEName != registration.eName (or owner/ename field used on DeviceToken), remove the token from each DeviceToken.tokens array, update updatedAt, and save via deviceTokenRepository.save so the token is removed from the device_token.tokens store as well; reference NotificationService, verificationRepository, deviceTokenRepository, registration.eName, pushTokens, and device_token.tokens to locate changes.
🧹 Nitpick comments (1)
infrastructure/evault-core/src/controllers/NotificationController.ts (1)
80-100: Consider making the Verification + DeviceToken updates atomic.This path now does
unregister(old)→registerDevice(...)→register(new)as separate writes. If the request fails in the middle,Verification.pushTokensanddevice_token.tokenscan diverge and the endpoint may return 500 after a partial success. Moving the token sync into one service/transaction would make this much safer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/evault-core/src/controllers/NotificationController.ts` around lines 80 - 100, The current flow in NotificationController does separate writes (unregister old tokens via deviceTokenService, then notificationService.registerDevice which updates Verification, then deviceTokenService.register), risking divergence on failure; change this so the Verification pushTokens update and device_token register/unregister occur inside a single transaction (e.g., use AppDataSource.manager.transaction) by moving the token-sync logic into a single service method (or extend notificationService.registerDevice) that accepts a transactional EntityManager and performs the AppDataSource.getRepository("Verification") update plus the device token register/unregister operations under that manager; update deviceTokenService.unregister/register to accept an optional manager or provide transactional counterparts, and ensure external side-effects are performed only after the DB transaction commits or are compensatable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@infrastructure/evault-core/src/services/NotificationService.ts`:
- Around line 225-231: The method in NotificationService.ts currently returns a
boolean (delivered) which is ambiguous; change the method (e.g., sendPush /
sendPushNotification) to return a discriminated result (string enum or object)
that distinguishes at least: delivered, no_active_devices, and
all_attempts_failed (include counts/tokens as needed). Update the method logic
where delivered is computed to set the appropriate status and return that result
instead of a bare boolean, adjust logging to include the status, and update
callers (NotificationController.ts) to switch on the new status rather than
treating false as "no active devices."
- Around line 26-39: The BAD_TOKEN_ERRORS list in NotificationService.ts
incorrectly includes provider/auth errors (ExpiredProviderToken,
InvalidProviderToken, messaging/mismatched-credential) which are not
device-token failures and lead removeBadTokens to delete valid registrations;
remove those three entries from the BAD_TOKEN_ERRORS constant so isBadTokenError
only matches actual device/token errors (e.g.,
messaging/registration-token-not-valid, Unregistered, BadDeviceToken,
DeviceTokenNotForTopic) and verify any callers of isBadTokenError (such as
removeBadTokens) still behave correctly.
---
Outside diff comments:
In `@infrastructure/evault-core/src/services/MessageNotificationService.ts`:
- Around line 80-104: The code currently treats any message whose
payload.content starts with "$$system-message$$" (rawText and isSystemMessage)
as a system notification, which is spoofable; change the logic in the
MessageNotificationService where rawText/isSystemMessage/messageText are used so
system notifications come from trusted metadata instead of user-controlled
content — e.g., check a server-set flag on the payload (payload.type ===
'system' or payload.isSystem) or validate the sender via a trusted-sender check
(use senderEName against a trusted list or call an isTrustedSender helper)
before setting title/body as a system message; retain removing the marker from
display text only for legacy compatibility if and only if the message is
validated as a system message by the trusted flag/sender check.
In `@infrastructure/evault-core/src/services/NotificationService.ts`:
- Around line 52-64: The current NotificationService flow removes the token from
other Verification.pushTokens but doesn't remove it from the DeviceToken store,
leaving device_token.tokens entries that still reference the old owner; update
the same path in NotificationService (after the loop that updates
verificationRepository entries) to query deviceTokenRepository for DeviceToken
rows where :token = ANY(device_token.tokens) and linkedEName !=
registration.eName (or owner/ename field used on DeviceToken), remove the token
from each DeviceToken.tokens array, update updatedAt, and save via
deviceTokenRepository.save so the token is removed from the device_token.tokens
store as well; reference NotificationService, verificationRepository,
deviceTokenRepository, registration.eName, pushTokens, and device_token.tokens
to locate changes.
---
Nitpick comments:
In `@infrastructure/evault-core/src/controllers/NotificationController.ts`:
- Around line 80-100: The current flow in NotificationController does separate
writes (unregister old tokens via deviceTokenService, then
notificationService.registerDevice which updates Verification, then
deviceTokenService.register), risking divergence on failure; change this so the
Verification pushTokens update and device_token register/unregister occur inside
a single transaction (e.g., use AppDataSource.manager.transaction) by moving the
token-sync logic into a single service method (or extend
notificationService.registerDevice) that accepts a transactional EntityManager
and performs the AppDataSource.getRepository("Verification") update plus the
device token register/unregister operations under that manager; update
deviceTokenService.unregister/register to accept an optional manager or provide
transactional counterparts, and ensure external side-effects are performed only
after the DB transaction commits or are compensatable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ff2d11be-4743-4ca4-a02c-68bd6d2b3690
📒 Files selected for processing (4)
infrastructure/evault-core/src/controllers/NotificationController.tsinfrastructure/evault-core/src/core/protocol/graphql-server.tsinfrastructure/evault-core/src/services/MessageNotificationService.tsinfrastructure/evault-core/src/services/NotificationService.ts
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
coodos
marked this pull request as draft
March 16, 2026 09:11
coodos
marked this pull request as ready for review
March 16, 2026 09:19
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
infrastructure/evault-core/src/services/NotificationService.ts (1)
171-236:⚠️ Potential issue | 🟠 MajorThis fan-out is now serialized behind a 10s timeout per token.
If an
eNamehas several slow or stale tokens, the caller waits for each network attempt in sequence, so one notification can easily turn into a long-running request. Please keep the per-token cleanup, but move the send path to bounded concurrency or a background job.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/evault-core/src/services/NotificationService.ts` around lines 171 - 236, The current sequential loop in NotificationService.ts (iterating over allTokens and awaiting fetch per token) serializes sends behind a 10s timeout and can make a single notification block for many seconds; refactor by extracting the per-token logic into a sendToToken(token, platform) helper (reuse pushPayload, triggerUrl, isBadTokenError, and logging) and run those helpers with bounded concurrency (e.g., a simple semaphore/pool or p-limit) instead of awaiting each in sequence; collect results from Promise.allSettled to aggregate delivered and badTokens (mark delivered true if any task succeeded and push token to badTokens when the helper indicates a bad-token result), or alternatively dispatch sendToToken tasks to a background worker/queue and return immediately—preserve existing logging and error handling inside sendToToken and ensure the code updates delivered and badTokens only after tasks complete or via the background job processing.
♻️ Duplicate comments (1)
infrastructure/evault-core/src/services/NotificationService.ts (1)
244-250:⚠️ Potential issue | 🟠 MajorThe boolean result is still overloaded.
falsecovers both “no active devices” and “all push attempts failed”, whiletruecan also mean “saved for polling but no push tokens existed”. Callers cannot react correctly unless this becomes an explicit status/result type.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/evault-core/src/services/NotificationService.ts` around lines 244 - 250, The current return value (delivered boolean) from NotificationService conflates multiple outcomes (no active devices, saved-for-polling, push attempts failed, delivered); change it to an explicit result type (e.g., a discriminated union or enum like NotificationResult with variants such as NoActiveDevices, Delivered, SavedForPolling, AllPushFailed) and return that instead of the boolean; update the code that references delivered, allTokens and eName to set the correct variant (inspect token list length, whether messages were queued for polling, and whether any push succeeded) and update all callers to handle the new result type.
🧹 Nitpick comments (1)
infrastructure/evault-core/src/services/NotificationService.ts (1)
42-43: Don't let bad-token cleanup silently degrade to one table.With
deviceTokenRepository?, this service can be constructed in a state whereremoveBadTokensonly mutatesVerificationand leavesdevice_tokenstale. Making the dependency required, or failing fast when cleanup needs it, would keep this fix from becoming configuration-dependent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/evault-core/src/services/NotificationService.ts` around lines 42 - 43, NotificationService currently allows deviceTokenRepository to be optional which lets removeBadTokens run without touching the device_token table; make the dependency required or fail fast: update the NotificationService constructor to require deviceTokenRepository (remove the ? from the field) so it's always injected, or if you prefer runtime checking, have removeBadTokens explicitly throw a clear error if this.deviceTokenRepository is undefined before any mutations; reference NotificationService, removeBadTokens, deviceTokenRepository and ensure device_token cleanup is performed (or fails) rather than silently skipping.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@infrastructure/evault-core/src/services/NotificationService.ts`:
- Around line 171-236: The current sequential loop in NotificationService.ts
(iterating over allTokens and awaiting fetch per token) serializes sends behind
a 10s timeout and can make a single notification block for many seconds;
refactor by extracting the per-token logic into a sendToToken(token, platform)
helper (reuse pushPayload, triggerUrl, isBadTokenError, and logging) and run
those helpers with bounded concurrency (e.g., a simple semaphore/pool or
p-limit) instead of awaiting each in sequence; collect results from
Promise.allSettled to aggregate delivered and badTokens (mark delivered true if
any task succeeded and push token to badTokens when the helper indicates a
bad-token result), or alternatively dispatch sendToToken tasks to a background
worker/queue and return immediately—preserve existing logging and error handling
inside sendToToken and ensure the code updates delivered and badTokens only
after tasks complete or via the background job processing.
---
Duplicate comments:
In `@infrastructure/evault-core/src/services/NotificationService.ts`:
- Around line 244-250: The current return value (delivered boolean) from
NotificationService conflates multiple outcomes (no active devices,
saved-for-polling, push attempts failed, delivered); change it to an explicit
result type (e.g., a discriminated union or enum like NotificationResult with
variants such as NoActiveDevices, Delivered, SavedForPolling, AllPushFailed) and
return that instead of the boolean; update the code that references delivered,
allTokens and eName to set the correct variant (inspect token list length,
whether messages were queued for polling, and whether any push succeeded) and
update all callers to handle the new result type.
---
Nitpick comments:
In `@infrastructure/evault-core/src/services/NotificationService.ts`:
- Around line 42-43: NotificationService currently allows deviceTokenRepository
to be optional which lets removeBadTokens run without touching the device_token
table; make the dependency required or fail fast: update the NotificationService
constructor to require deviceTokenRepository (remove the ? from the field) so
it's always injected, or if you prefer runtime checking, have removeBadTokens
explicitly throw a clear error if this.deviceTokenRepository is undefined before
any mutations; reference NotificationService, removeBadTokens,
deviceTokenRepository and ensure device_token cleanup is performed (or fails)
rather than silently skipping.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fae4f0d8-e307-461b-9831-3a6a17b0f3a1
📒 Files selected for processing (1)
infrastructure/evault-core/src/services/NotificationService.ts
Uh oh!
There was an error while loading. Please reload this page.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of change
Fixes notification issues
Issue Number
Type of change
How the change has been tested
Change checklist
Summary by CodeRabbit
New Features
Improvements