feat(condo): DOMA-12992 encrypt data in push messages#7271
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdapters and the push transport now accept token-scoped payloads ( Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Caller
participant Transport as PushTransport
participant Encryptor as EncryptService
participant Adapter as Adapter
participant Provider as PushProvider
Client->>Transport: send({ notification, dataByToken, tokens, appIds, ... })
Transport->>Transport: prepareDataByToken(tokens, dataByToken, appIds)
alt encryption configured for some appIds
Transport->>Encryptor: encryptPushData(version, perTokenData, { appId })
Encryptor-->>Transport: encryptedOverlays (per-app)
Transport->>Transport: merge encrypted overlays into dataByToken
end
Transport->>Adapter: adapter.prepareBatchData(notification, dataByToken, tokens, pushTypes, appIds)
Adapter->>Provider: send batch payloads (per-token data / encrypted overlays)
Provider-->>Adapter: responses (success/fail per token)
Adapter-->>Transport: aggregated results
Transport-->>Client: final result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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)
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 |
Contributor
Confidence Score: 3/5
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
apps/condo/domains/notification/utils/serverSchema/push/encryption.js
Outdated
Show resolved
Hide resolved
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c042b58ed8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/condo/domains/notification/adapters/hcmAdapter.js (1)
253-258:⚠️ Potential issue | 🟠 Major
sendNotificationstill readsdata, so Huawei losesdataByTokenpayloads.Line 253 destructures
{ ... data, ... }, but transport now passesdataByToken(seeapps/condo/domains/notification/transports/push.js, Line 236). As a result, Line 257 callsprepareBatchDatawithundefineddata map.✅ Minimal fix
- async sendNotification ({ notification, data, tokens, pushTypes, appIds, metaByToken } = {}) { + async sendNotification ({ notification, dataByToken, tokens, pushTypes, appIds, metaByToken } = {}) { @@ - const [notifications, fakeNotifications, pushContext] = await HCMAdapter.prepareBatchData(notification, data, tokens, pushTypes, appIds) + const [notifications, fakeNotifications, pushContext] = await HCMAdapter.prepareBatchData(notification, dataByToken, tokens, pushTypes, appIds)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/condo/domains/notification/adapters/hcmAdapter.js` around lines 253 - 258, sendNotification is still destructuring and using "data" but the transport now supplies "dataByToken", causing prepareBatchData to receive undefined; update the sendNotification signature/parameter handling in HCMAdapter.sendNotification to accept and forward dataByToken (e.g. destructure { notification, data, dataByToken, tokens, pushTypes, appIds, metaByToken } and call HCMAdapter.prepareBatchData(notification, dataByToken ?? data, tokens, pushTypes, appIds)) so it prefers the per-token payload map while preserving the old data fallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/condo/domains/notification/adapters/webhookAdapter.js`:
- Around line 110-113: The conditional uses property access on the
Array.prototype.includes call (APPS_WITH_DISABLED_NOTIFICATIONS.includes[appId])
which is a typo; change it to a method call
APPS_WITH_DISABLED_NOTIFICATIONS.includes(appId) so the disabled-app filter
works correctly (leave the second check that uses includes(data.app) as-is) —
update the condition around notifications.push(pushData) referencing
APPS_WITH_DISABLED_NOTIFICATIONS, appId and data.app accordingly.
In `@apps/condo/domains/notification/transports/push.js`:
- Around line 224-237: The loop using encryptPushData can fail-open: when
encryption fails you currently only set encryptedDataByAppId but leave
dataByToken and tokens intact so adapter.sendNotification may send plaintext;
instead, after encryptPushData (in the same loop or immediately after) mark
per-token success and build a filtered payload containing only tokens that were
successfully encrypted (update tokens, appIds, pushTypes, metaByToken and
dataByToken to remove or omit failed tokens) and then call
adapter.sendNotification with that filtered payload (use encryptedDataByAppId
and per-token success flags to decide inclusion). Ensure the same fix is applied
to the other symmetric block mentioned (around the other occurrence near line
246) so sendNotification is only ever invoked for tokens with successful
encryption.
In `@apps/condo/domains/notification/utils/serverSchema/push/encryption.js`:
- Around line 13-17: The v1 function currently XORs payload with options.appId
(in v1), which is reversible and insecure; replace this with authenticated
encryption using a proper key derived from a server-side secret (not appId) and
a modern AEAD cipher (e.g., AES-GCM or libsodium secretbox): derive/lookup a
symmetric key on the server, generate a random nonce/IV per encryption, encrypt
the JSON payload with the AEAD API, include the nonce/IV and auth tag with the
ciphertext, and return their base64 encoding; update the v1 function to use
node's crypto or libsodium libs and ensure options.appId is only used to
select/lookup the server-side key (not as key material) and that decryption
verifies the auth tag before returning plaintext.
---
Outside diff comments:
In `@apps/condo/domains/notification/adapters/hcmAdapter.js`:
- Around line 253-258: sendNotification is still destructuring and using "data"
but the transport now supplies "dataByToken", causing prepareBatchData to
receive undefined; update the sendNotification signature/parameter handling in
HCMAdapter.sendNotification to accept and forward dataByToken (e.g. destructure
{ notification, data, dataByToken, tokens, pushTypes, appIds, metaByToken } and
call HCMAdapter.prepareBatchData(notification, dataByToken ?? data, tokens,
pushTypes, appIds)) so it prefers the per-token payload map while preserving the
old data fallback.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/condo/domains/notification/adapters/appleAdapter.jsapps/condo/domains/notification/adapters/appleAdapter.spec.jsapps/condo/domains/notification/adapters/firebaseAdapter.jsapps/condo/domains/notification/adapters/hcmAdapter.jsapps/condo/domains/notification/adapters/redStoreAdapter.jsapps/condo/domains/notification/adapters/webhookAdapter.jsapps/condo/domains/notification/transports/push.jsapps/condo/domains/notification/utils/serverSchema/push/encryption.js
apps/condo/domains/notification/utils/serverSchema/push/encryption.js
Outdated
Show resolved
Hide resolved
Member
SavelevMatthew
left a comment
SavelevMatthew
left a comment
There was a problem hiding this comment.
Open questions:
- Can we omit title / body. Probably no
- _actual data must be transformed to something dynamic
Member
SavelevMatthew
left a comment
SavelevMatthew
left a comment
There was a problem hiding this comment.
encryption version must be part of payload
Contributor
Author
|
apps/condo/domains/notification/utils/serverSchema/push/encryption.js
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
apps/condo/domains/notification/adapters/redStoreAdapter.spec.js (1)
232-258: Update skipped test to usedataByTokenfor consistency.This skipped test still uses the old
dataproperty instead ofdataByToken, making it inconsistent with the other tests in this file. Additionally, there's a duplicate key bug on lines 244-245 where[PUSH_FAKE_TOKEN_FAIL]appears twice inappIds, causing the second entry to overwrite the first.When this test is eventually enabled, it will likely fail due to the outdated structure.
♻️ Proposed fix
it.skip('doesnt send push notification to app with disabled notifications', async () => { const pushData = { tokens: [PUSH_FAKE_TOKEN_SUCCESS], notification: { title: 'Condo', body: `${dayjs().format()} Condo greets you!`, }, - data: { - appId : 'condo.app.clients', - type: 'notification', + dataByToken: { + [PUSH_FAKE_TOKEN_SUCCESS]: { + appId: 'condo.app.clients', + type: 'notification', + }, }, appIds: { - [PUSH_FAKE_TOKEN_FAIL]: 'condo.app.clients', - [PUSH_FAKE_TOKEN_FAIL]: 'condo', + [PUSH_FAKE_TOKEN_SUCCESS]: 'condo.app.clients', }, pushTypes: { - [PUSH_FAKE_TOKEN_FAIL]: PUSH_TYPE_DEFAULT, [PUSH_FAKE_TOKEN_SUCCESS]: PUSH_TYPE_DEFAULT, }, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/condo/domains/notification/adapters/redStoreAdapter.spec.js` around lines 232 - 258, The test uses the old `data` property and a duplicated key in `appIds`; update the `pushData` object used with `adapter.sendNotification` to follow the other tests by replacing `data` with `dataByToken`, ensure `tokens` includes both PUSH_FAKE_TOKEN_SUCCESS and PUSH_FAKE_TOKEN_FAIL, and set `dataByToken`, `appIds`, and `pushTypes` as maps keyed by each token (use `[PUSH_FAKE_TOKEN_SUCCESS]` and `[PUSH_FAKE_TOKEN_FAIL]` uniquely) so no keys are duplicated and the structure matches what `adapter.sendNotification` expects.apps/condo/domains/notification/transports/push.spec.js (1)
864-874: Remove the commented-out duplicate mock block.Keeping this disabled block makes the encryption test harder to scan and maintain.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/condo/domains/notification/transports/push.spec.js` around lines 864 - 874, Remove the commented-out duplicate mock block in the push.spec.js test: delete the commented jest.doMock that references getTokens/mockGetTokens and FirebaseAdapter/mockFirebaseAdapter so only the active mock implementation remains; ensure no other tests rely on that commented code and run the encryption tests to confirm readability and behavior are unchanged.apps/condo/domains/notification/utils/serverSchema/push/helpers.js (1)
41-41: Return a stable empty result shape fromgetTokens.Line 41 returns
[], while the normal path returns an object. Keeping a single return shape reduces downstream edge cases for new callers.♻️ Proposed refactor
async function getTokens (ownerId, remoteClientId, isVoIP = false) { - if (!ownerId && !remoteClientId) return [] + if (!ownerId && !remoteClientId) { + return { + tokensByTransport: { + [PUSH_TRANSPORT_FIREBASE]: [], + [PUSH_TRANSPORT_REDSTORE]: [], + [PUSH_TRANSPORT_HUAWEI]: [], + [PUSH_TRANSPORT_APPLE]: [], + [PUSH_TRANSPORT_WEBHOOK]: [], + }, + pushTypes: {}, + appIds: {}, + metaByToken: {}, + count: 0, + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/condo/domains/notification/utils/serverSchema/push/helpers.js` at line 41, The early-return in getTokens uses "if (!ownerId && !remoteClientId) return []" which returns an array while the normal success path returns an object; change this to return the same stable empty result shape as the normal path (e.g., an object with the same keys like tokens: [], devices: [], count: 0 or whatever fields getTokens normally returns) so callers always receive a consistent object; update the return to something like "return { tokens: [], ... }" matching the actual keys used by getTokens.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/condo/domains/notification/adapters/appleAdapter.js`:
- Around line 164-166: The early return when dataByToken[pushToken] is falsy
prevents FAKE-token handling—modify the loop that reads const data =
dataByToken[pushToken] so it does not unconditionally return on missing data:
when data is missing, treat that token as a FAKE-token case and call
injectFakeResults (or otherwise record a fake response) instead of returning;
apply the same change to the similar block around the logic referenced at
188-191 so FAKE-token emulation runs when per-token data is absent while real
tokens continue to use dataByToken entries.
In `@apps/condo/domains/notification/adapters/firebaseAdapter.js`:
- Around line 189-191: The handler currently bails out when
dataByToken[pushToken] is falsy (const data = dataByToken[pushToken]; if (!data)
return), which prevents recording emulated responses for fake-success/fake-fail
tokens; update the logic in the block around that expression (and the analogous
block at the 213–216 region) to not return early but instead treat a missing
data entry as a fake token case — create a fallback response/record (e.g., mark
as emulated success or failure per the fake token rules) and push that into the
same response/records path so fake tokens still produce the expected emulated
responses while real tokens continue to use data from dataByToken.
In `@apps/condo/domains/notification/transports/push.js`:
- Line 206: Remove the sensitive encrypted payload from the log call: stop
passing encryptedDataByAppId to logger.info in the push transport and replace it
with non-sensitive metadata (e.g., list of target appIds, count of encrypted
entries, or a redaction marker) so you still have useful debug info without
logging ciphertext; locate the logger.info invocation that references
encryptedDataByAppId in apps/condo/domains/notification/transports/push.js and
update it to log only safe fields (e.g., appIds, entry count, or
"encryptedPayload: [REDACTED]") instead of the actual encryptedDataByAppId
content.
- Around line 132-136: The code incorrectly stores raw data and still attempts
encryption when encryptionVersion is missing: replace the assignment that sets
dataByToken[token] = data with dataByToken[token] = dataForToken when
!encryptionVersion, and immediately continue/return for that token so you do not
call encryptPushData(encryptionVersion, dataForToken, { appId }) when
encryptionVersion is falsy; update any subsequent handling to treat that token
as non-encrypted (do not set encryption metadata) and proceed to send the
prepared dataForToken as-is.
- Around line 185-188: The forEach callbacks currently use concise arrow bodies
that implicitly return assignment/delete expressions (e.g., the map population
from encryptedDataByAppIdForCurrenyTransport into encryptedDataByAppId and the
delete calls for appIds/pushTypes/metaByToken), which triggers the lint rule;
change these to block-bodied callbacks or simple for...of loops so the
statements are executed without returning a value — specifically update the
Object.keys(...).forEach callback that assigns into encryptedDataByAppId (from
encryptedDataByAppIdForCurrenyTransport) and the tokensWithNoData.forEach
callback that iterates [appIds, pushTypes, metaByToken] to use { ... } bodies
(or explicit for loops) and perform the assignment/delete as statements inside
the block.
In `@apps/condo/domains/notification/transports/push.spec.js`:
- Around line 899-900: The nosemgrep suppression above the dynamic RegExp usage
lacks the required safety rationale; update the tests where RegExp is
constructed from TEST_ENCRYPTION_VERSIONS (the
expect(dataByToken[token][ENCRYPTED_APP_ID]).toMatch(new
RegExp(`^${TEST_ENCRYPTION_VERSIONS[ENCRYPTED_APP_ID]}_`)) and the similar
occurrences) by adding a clear comment immediately before each nosemgrep
directive explaining why the non-literal RegExp is safe (e.g., the input is a
hard-coded test constant, not user-controlled, and is validated/escaped), then
keep the nosemgrep line; apply the same explicit rationale comment for the other
two occurrences referenced in the review.
In `@apps/condo/domains/notification/utils/serverSchema/push/helpers.js`:
- Line 44: The file calls find('RemoteClient', ...) (assigning to remoteClients)
but never imports find, causing a ReferenceError; add the missing import at the
top of the file by importing the find helper from the module where it's defined
(match other files in the codebase that use find), e.g. add an import for find
and ensure it's exported/imported with the same name so the call in helpers.js
(the find('RemoteClient', conditions) usage) resolves at runtime.
---
Nitpick comments:
In `@apps/condo/domains/notification/adapters/redStoreAdapter.spec.js`:
- Around line 232-258: The test uses the old `data` property and a duplicated
key in `appIds`; update the `pushData` object used with
`adapter.sendNotification` to follow the other tests by replacing `data` with
`dataByToken`, ensure `tokens` includes both PUSH_FAKE_TOKEN_SUCCESS and
PUSH_FAKE_TOKEN_FAIL, and set `dataByToken`, `appIds`, and `pushTypes` as maps
keyed by each token (use `[PUSH_FAKE_TOKEN_SUCCESS]` and
`[PUSH_FAKE_TOKEN_FAIL]` uniquely) so no keys are duplicated and the structure
matches what `adapter.sendNotification` expects.
In `@apps/condo/domains/notification/transports/push.spec.js`:
- Around line 864-874: Remove the commented-out duplicate mock block in the
push.spec.js test: delete the commented jest.doMock that references
getTokens/mockGetTokens and FirebaseAdapter/mockFirebaseAdapter so only the
active mock implementation remains; ensure no other tests rely on that commented
code and run the encryption tests to confirm readability and behavior are
unchanged.
In `@apps/condo/domains/notification/utils/serverSchema/push/helpers.js`:
- Line 41: The early-return in getTokens uses "if (!ownerId && !remoteClientId)
return []" which returns an array while the normal success path returns an
object; change this to return the same stable empty result shape as the normal
path (e.g., an object with the same keys like tokens: [], devices: [], count: 0
or whatever fields getTokens normally returns) so callers always receive a
consistent object; update the return to something like "return { tokens: [], ...
}" matching the actual keys used by getTokens.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
apps/condo/domains/notification/adapters/appleAdapter.jsapps/condo/domains/notification/adapters/appleAdapter.spec.jsapps/condo/domains/notification/adapters/firebaseAdapter.jsapps/condo/domains/notification/adapters/firebaseAdapter.spec.jsapps/condo/domains/notification/adapters/hcmAdapter.jsapps/condo/domains/notification/adapters/redStoreAdapter.jsapps/condo/domains/notification/adapters/redStoreAdapter.spec.jsapps/condo/domains/notification/adapters/webhookAdapter.jsapps/condo/domains/notification/adapters/webhookAdapter.spec.jsapps/condo/domains/notification/transports/push.jsapps/condo/domains/notification/transports/push.spec.jsapps/condo/domains/notification/utils/serverSchema/push/encryption.jsapps/condo/domains/notification/utils/serverSchema/push/helpers.js
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/condo/domains/notification/utils/serverSchema/push/encryption.js
- apps/condo/domains/notification/adapters/redStoreAdapter.js
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/condo/domains/notification/utils/serverSchema/push/helpers.js (1)
42-43: Return a stable object shape fromgetTokensin the empty-input branch.Line 43 returns
[], while the normal path returns an object. This makes callers handle two result types and can make destructuring fragile. Prefer returning the same empty object shape withcount: 0.♻️ Proposed refactor
async function getTokens (ownerId, remoteClientId, isVoIP = false) { - if (!ownerId && !remoteClientId) return [] + if (!ownerId && !remoteClientId) { + return { + tokensByTransport: { + [PUSH_TRANSPORT_FIREBASE]: [], + [PUSH_TRANSPORT_REDSTORE]: [], + [PUSH_TRANSPORT_HUAWEI]: [], + [PUSH_TRANSPORT_APPLE]: [], + [PUSH_TRANSPORT_WEBHOOK]: [], + }, + pushTypes: {}, + appIds: {}, + metaByToken: {}, + count: 0, + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/condo/domains/notification/utils/serverSchema/push/helpers.js` around lines 42 - 43, The empty-input branch of getTokens currently returns an array ([]) causing inconsistent return shapes; change that branch to return the same object shape as the normal path (e.g., { count: 0, tokens: [] } or whatever fields getTokens normally returns) so callers can always destructure a stable object; update the return in getTokens to that empty-object shape.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/condo/domains/notification/transports/push.js`:
- Line 228: The log call references data.notificationId directly which can throw
if data is undefined; update the logger.info call in the send function to use
safe access for the entityId (e.g., data?.notificationId or a fallback like
(data && data.notificationId) || null) when constructing the log payload that
includes encryptionStatsInfo, so the logger never attempts to dereference an
undefined data object.
- Around line 141-144: The code calls adapter.constructor.prepareData(data,
token) into preparedDataForToken and then immediately calls encryptPushData
which will throw if preparedDataForToken is empty; add a guard after computing
preparedDataForToken to skip this token when it's falsy (e.g., if
(!preparedDataForToken) continue or return early from the per-token handler) so
encryption is only invoked for non-empty per-token payloads; update the block
around adapter.constructor.prepareData, preparedDataForToken and encryptPushData
to perform this check and ensure fake tokens still require entries in
dataByToken by treating missing preparedDataForToken as a token-level skip.
---
Nitpick comments:
In `@apps/condo/domains/notification/utils/serverSchema/push/helpers.js`:
- Around line 42-43: The empty-input branch of getTokens currently returns an
array ([]) causing inconsistent return shapes; change that branch to return the
same object shape as the normal path (e.g., { count: 0, tokens: [] } or whatever
fields getTokens normally returns) so callers can always destructure a stable
object; update the return in getTokens to that empty-object shape.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fe089514-68d6-4fd7-b474-c60c15de156f
📒 Files selected for processing (3)
apps/condo/domains/notification/transports/push.jsapps/condo/domains/notification/utils/serverSchema/push/encryption.jsapps/condo/domains/notification/utils/serverSchema/push/helpers.js
apps/condo/domains/notification/utils/serverSchema/push/encryption.js
Outdated
Show resolved
Hide resolved
YEgorLu
force-pushed
the
feat/condo/DOMA-12992/push-data-encryption
branch
from
49497e1 to
8c9971f
Compare
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 (1)
apps/condo/domains/notification/adapters/oneSignalAdapter.js (1)
158-171:⚠️ Potential issue | 🟡 MinorDefault
dataByTokeninprepareBatchDatato avoid direct-call crashes.Without a default,
dataByToken[pushToken]can throw if this static method is invoked withundefined.🔧 Proposed fix
-static prepareBatchData (notificationRaw, dataByToken, tokens = [], pushTypes = {}, isVoIP = false, appIds = {}) { +static prepareBatchData (notificationRaw, dataByToken = {}, tokens = [], pushTypes = {}, isVoIP = false, appIds = {}) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/condo/domains/notification/adapters/oneSignalAdapter.js` around lines 158 - 171, prepareBatchData can crash when called with dataByToken undefined; update the static method signature prepareBatchData(notificationRaw, dataByToken = {}, tokens = [], pushTypes = {}, isVoIP = false, appIds = {}) to provide a default empty object for dataByToken so accesses like dataByToken[pushToken] are safe, and keep existing logic that reads preparedData and iterates tokens (no other behavior changes needed).
🧹 Nitpick comments (1)
apps/condo/domains/notification/adapters/redStoreAdapter.js (1)
132-134: Trim conversational inline comments here.This comment thread reads like review history rather than code intent. Keeping only actionable context will improve readability.
🧹 Suggested cleanup
- // TODO(pahaz): check why `pushData.appId` used. but `data.app` is everywhere?! - // NOTE(YEgorLu) ^ there is more. I can't find in history where event data.app was provided previously or what even is this🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/condo/domains/notification/adapters/redStoreAdapter.js` around lines 132 - 134, Remove the conversational review-history comments around the conditional in redStoreAdapter.js (the TODO(pahaz) and NOTE(YEgorLu) lines) and replace them with a single concise, actionable comment that explains the intent of the check (e.g., clarify which field is expected: pushData.appId vs data.app and why the conditional exists); update the comment adjacent to the if(...) so it documents the behavior briefly without review chatter.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/condo/domains/notification/adapters/hcmAdapter.js`:
- Around line 261-266: The current logger.info call inside the
sendNotification/prepareBatchData flow logs full per-token payloads and token
lists (variables: notification, dataByToken, tokens, pushTypes, notifications,
fakeNotifications, pushContext). Replace that detailed dump with a sanitized
summary: log pushTypes and pushContext as-is, log non-sensitive notification
metadata only (e.g., notification.type or notification.id instead of full body),
do not include dataByToken or tokens arrays; instead log counts (e.g.,
tokensCount, dataByTokenCount) and any hashed/masked identifier summaries if
needed, and include counts for notifications and fakeNotifications; update the
logger.info call to emit this minimized payload to avoid leaking sensitive
content or identifiers.
In `@apps/condo/domains/notification/utils/serverSchema/push/helpers.js`:
- Line 49: getTokens currently returns an empty array when both ownerId and
remoteClientId are missing, but callers (e.g., transports/push.js) expect an
object; change the early return in getTokens from [] to {} so it returns a
consistent object shape, ensuring any code that does
Object.entries(initialPushTypes) or similar won't throw.
---
Outside diff comments:
In `@apps/condo/domains/notification/adapters/oneSignalAdapter.js`:
- Around line 158-171: prepareBatchData can crash when called with dataByToken
undefined; update the static method signature prepareBatchData(notificationRaw,
dataByToken = {}, tokens = [], pushTypes = {}, isVoIP = false, appIds = {}) to
provide a default empty object for dataByToken so accesses like
dataByToken[pushToken] are safe, and keep existing logic that reads preparedData
and iterates tokens (no other behavior changes needed).
---
Nitpick comments:
In `@apps/condo/domains/notification/adapters/redStoreAdapter.js`:
- Around line 132-134: Remove the conversational review-history comments around
the conditional in redStoreAdapter.js (the TODO(pahaz) and NOTE(YEgorLu) lines)
and replace them with a single concise, actionable comment that explains the
intent of the check (e.g., clarify which field is expected: pushData.appId vs
data.app and why the conditional exists); update the comment adjacent to the
if(...) so it documents the behavior briefly without review chatter.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7d630e08-764a-4a4f-a14c-6bb06b4e3b2c
📒 Files selected for processing (14)
apps/condo/domains/notification/adapters/appleAdapter.jsapps/condo/domains/notification/adapters/appleAdapter.spec.jsapps/condo/domains/notification/adapters/firebaseAdapter.jsapps/condo/domains/notification/adapters/firebaseAdapter.spec.jsapps/condo/domains/notification/adapters/hcmAdapter.jsapps/condo/domains/notification/adapters/oneSignalAdapter.jsapps/condo/domains/notification/adapters/redStoreAdapter.jsapps/condo/domains/notification/adapters/redStoreAdapter.spec.jsapps/condo/domains/notification/adapters/webhookAdapter.jsapps/condo/domains/notification/adapters/webhookAdapter.spec.jsapps/condo/domains/notification/transports/push.jsapps/condo/domains/notification/transports/push.spec.jsapps/condo/domains/notification/utils/serverSchema/push/encryption.jsapps/condo/domains/notification/utils/serverSchema/push/helpers.js
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/condo/domains/notification/adapters/appleAdapter.spec.js
- apps/condo/domains/notification/adapters/firebaseAdapter.spec.js
- apps/condo/domains/notification/adapters/redStoreAdapter.spec.js
- apps/condo/domains/notification/utils/serverSchema/push/encryption.js
- apps/condo/domains/notification/transports/push.spec.js
| * @returns {Promise<{tokensByTransport: {[p: string]: [], '[PUSH_TRANSPORT_HUAWEI]': *[], '[PUSH_TRANSPORT_APPLE]': *[], '[PUSH_TRANSPORT_FIREBASE]': *[]}, appIds: {}, pushTypes: {}, count}|*[]>} | ||
| */ | ||
| async function getTokens (ownerId, remoteClientId, isVoIP = false) { | ||
| if (!ownerId && !remoteClientId) return [] |
Contributor
There was a problem hiding this comment.
Return a consistent object shape from getTokens.
getTokens returns [] here, but the caller in transports/push.js treats the result as an object and uses Object.entries(initialPushTypes). This can throw when both IDs are missing.
🐛 Proposed fix
async function getTokens (ownerId, remoteClientId, isVoIP = false) {
- if (!ownerId && !remoteClientId) return []
+ if (!ownerId && !remoteClientId) {
+ return {
+ tokensByTransport: {
+ [PUSH_TRANSPORT_FIREBASE]: [],
+ [PUSH_TRANSPORT_REDSTORE]: [],
+ [PUSH_TRANSPORT_HUAWEI]: [],
+ [PUSH_TRANSPORT_APPLE]: [],
+ [PUSH_TRANSPORT_WEBHOOK]: [],
+ [PUSH_TRANSPORT_ONESIGNAL]: [],
+ },
+ pushTypes: {},
+ appIds: {},
+ metaByToken: {},
+ count: 0,
+ }
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/condo/domains/notification/utils/serverSchema/push/helpers.js` at line
49, getTokens currently returns an empty array when both ownerId and
remoteClientId are missing, but callers (e.g., transports/push.js) expect an
object; change the early return in getTokens from [] to {} so it returns a
consistent object shape, ensuring any code that does
Object.entries(initialPushTypes) or similar won't throw.
Contributor
Author
There was a problem hiding this comment.
Wow, didn't see that. It's been like that for ages, so I will fix this later
Contributor
There was a problem hiding this comment.
@YEgorLu, understood! It's a good catch regardless. Since you're planning to address it in a later PR, would you like me to open a GitHub issue to track this fix? I can create one with a reference back to this PR and comment for easy follow-up.
🧠 Learnings used
Learnt from: YEgorLu
Repo: open-condo-software/condo PR: 7271
File: apps/condo/domains/notification/adapters/firebaseAdapter.js:189-191
Timestamp: 2026-03-03T22:05:41.768Z
Learning: In `apps/condo/domains/notification/adapters/firebaseAdapter.js` (and the other push adapters in the same PR), fake tokens (PUSH_FAKE_TOKEN_SUCCESS / PUSH_FAKE_TOKEN_FAIL) are also required to have a corresponding entry in `dataByToken`. A call without data for any token — including fake tokens — is considered invalid and should be skipped via the `if (!data) return` early-exit guard. There is no special-case handling needed for fake tokens regarding missing data.
Learnt from: YEgorLu
Repo: open-condo-software/condo PR: 7271
File: apps/condo/domains/notification/transports/push.js:228-228
Timestamp: 2026-03-04T15:46:24.336Z
Learning: In `apps/condo/domains/notification/transports/push.js`, the `send` function always receives a fully-populated `data` object (including `data.notificationId`) because it is only ever called after `prepareMessageToSend`, which requires an existing Message entity. The optional default `= {}` in the destructured parameter is legacy and does not reflect real usage, so direct access like `data.notificationId` is safe.
Learnt from: YEgorLu
Repo: open-condo-software/condo PR: 7271
File: apps/condo/domains/notification/utils/serverSchema/push/encryption.js:13-17
Timestamp: 2026-03-03T19:05:35.589Z
Learning: In `apps/condo/domains/notification/utils/serverSchema/push/encryption.js`, the v1 encryptor uses XOR with appId as intentional obfuscation (not cryptographic security). This simple approach is by design to prevent casual inspection of push data, with the function named "encryption" to allow future upgrades to real encryption if needed.
|
abshnko
approved these changes
Mar 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Summary by CodeRabbit
New Features
Refactor
Tests