feat(condo): DOMA-13020 new arguments in syncRemoteClient for multiple tokens and security#7308
Merged
SavelevMatthew merged 9 commits intomainfrom Mar 6, 2026
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduces array-based push token handling with validation, deduplication, migration from legacy fields, and deviceKey ownership validation; adds GraphQL PushToken input and extends SyncRemoteClientInput; new utilities, tests, and error constants support the flow. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Resolver as SyncRemoteClientService
participant Validator as Push Token Validator
participant Deduplicator as Push Token Deduplicator
participant DB as Database
Client->>Resolver: syncRemoteClient(pushTokens[], deviceKey, legacyFields...)
Resolver->>Validator: getPushTokensValidationError(pushTokens || builtFromLegacy)
Validator->>Validator: check token presence, transport counts, pushType consistency
Validator-->>Resolver: Validation result (error or ok)
alt Validation Success
Resolver->>Deduplicator: deduplicatePushTokens(tokens)
Deduplicator->>Deduplicator: group by transport+token, merge flags
Deduplicator-->>Resolver: deduplicated tokens
Resolver->>Resolver: validate deviceKey format and ownership
Resolver->>DB: persist client record with tokens & deviceKey
DB-->>Resolver: persisted
Resolver-->>Client: success (updated client)
else Validation Failure
Resolver-->>Client: BAD_USER_INPUT with error details
end
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 |
![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: b72ce29e73
ℹ️ 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".
apps/condo/domains/notification/schema/SyncRemoteClientService.js
Outdated
Show resolved
Hide resolved
Contributor
Confidence Score: 2/5
Important Files Changed
Last reviewed commit: b72ce29 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
apps/condo/domains/notification/schema/SyncRemoteClientService.js
Outdated
Show resolved
Hide resolved
apps/condo/domains/notification/utils/serverSchema/syncRemoteClient/pushTokensInput.js
Outdated
Show resolved
Hide resolved
apps/condo/domains/notification/schema/SyncRemoteClientService.js
Outdated
Show resolved
Hide resolved
![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: 2
🧹 Nitpick comments (1)
apps/condo/domains/notification/utils/serverSchema/syncRemoteClient/pushTokensInput.spec.js (1)
1-5: Tests are coupled to implementation internals via exported validation templates.Consider asserting public behavior (
error.type,error.code) instead of importing and matching fullPUSH_TOKENS_VALIDATION_ERRORSobjects frompushTokensInput.As per coding guidelines: "Test public APIs and behavior, not implementation details - avoid exporting constants solely for testing".
Also applies to: 41-42, 52-53, 62-63, 73-74, 88-89, 96-97
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/condo/domains/notification/utils/serverSchema/syncRemoteClient/pushTokensInput.spec.js` around lines 1 - 5, The tests import PUSH_TOKENS_VALIDATION_ERRORS which couples tests to implementation internals; update tests using getPushTokensValidationError (and related assertions at lines referencing PUSH_TOKENS_VALIDATION_ERRORS) to assert public behavior instead — e.g. verify error.type and error.code (or other public fields returned by getPushTokensValidationError) and expected validation messages, rather than importing or matching the PUSH_TOKENS_VALIDATION_ERRORS object; remove imports of PUSH_TOKENS_VALIDATION_ERRORS and adjust assertions around getPushTokensValidationError and deduplicatePushTokens to rely only on their public outputs.
🤖 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/schema/SyncRemoteClientService.js`:
- Around line 73-79: The schema sets defaults for PushTransportType fields using
PUSH_TYPE_DEFAULT which is a PushType value ('default'), causing invalid enum
values; remove the default assignments from the pushTransport and
pushTransportVoIP field declarations (i.e., delete "= ${PUSH_TYPE_DEFAULT}" for
the pushTransport and pushTransportVoIP fields) so they no longer default to an
invalid PushType, keeping the types as PushTransportType and ensuring callers
must provide valid transport enum values.
In
`@apps/condo/domains/notification/utils/serverSchema/syncRemoteClient/pushTokensInput.js`:
- Around line 21-25: The validation message for TOO_MANY_TOKENS_FOR_TRANSPORT
currently claims “<=1 for simple pushes and <=1 for voip pushes” while the
enforcement allows up to 2 distinct tokens per transport; update the message
string in the TOO_MANY_TOKENS_FOR_TRANSPORT object (the one using
BAD_USER_INPUT) to accurately describe the enforced rule (e.g., "Each transport
can have at most 2 distinct tokens (e.g., one simple and one voip)") and make
the same change for the other identical occurrence of
TOO_MANY_TOKENS_FOR_TRANSPORT in this file so the text matches the actual
validation.
---
Nitpick comments:
In
`@apps/condo/domains/notification/utils/serverSchema/syncRemoteClient/pushTokensInput.spec.js`:
- Around line 1-5: The tests import PUSH_TOKENS_VALIDATION_ERRORS which couples
tests to implementation internals; update tests using
getPushTokensValidationError (and related assertions at lines referencing
PUSH_TOKENS_VALIDATION_ERRORS) to assert public behavior instead — e.g. verify
error.type and error.code (or other public fields returned by
getPushTokensValidationError) and expected validation messages, rather than
importing or matching the PUSH_TOKENS_VALIDATION_ERRORS object; remove imports
of PUSH_TOKENS_VALIDATION_ERRORS and adjust assertions around
getPushTokensValidationError and deduplicatePushTokens to rely only on their
public outputs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 67094ce8-6501-4911-931e-bbb70e25b216
📒 Files selected for processing (4)
apps/condo/domains/notification/constants/errors.jsapps/condo/domains/notification/schema/SyncRemoteClientService.jsapps/condo/domains/notification/utils/serverSchema/syncRemoteClient/pushTokensInput.jsapps/condo/domains/notification/utils/serverSchema/syncRemoteClient/pushTokensInput.spec.js
apps/condo/domains/notification/schema/SyncRemoteClientService.js
Outdated
Show resolved
Hide resolved
apps/condo/domains/notification/utils/serverSchema/syncRemoteClient/pushTokensInput.js
Show resolved
Hide resolved
| await passwordValidations(context, deviceKey) | ||
| const { keystone } = getSchemaCtx('User') | ||
| try { | ||
| keystone.lists['User'].fieldsByPath.password.validateNewPassword(deviceKey) |
Member
There was a problem hiding this comment.
wtf? why user.password is here?)
Contributor
Author
There was a problem hiding this comment.
This will be RemoteClient.password.
- keystone.lists['RemoteClient'] will be kept for "compare"
- maybe it will be kept for inner validation also, maybe we will just trigger RemoteClient.update / create and receive error
But for now to validate that users will send proper deviceKey lets trigger inner Password checks manually from other List, where Password already exists
Member
There was a problem hiding this comment.
Why can't we add deviceKey with Password type in current PR to avoid this mess?
Contributor
Author
There was a problem hiding this comment.
I don't want to involve migrations in here and make this and next (with migration) PRs event more complicated
Contributor
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
YEgorLu
force-pushed
the
feat/condo/DOMA-13020/support-multiple-tokens-in-syncReomteClient
branch
from
3425a78 to
185a4c6
Compare
Contributor
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
YEgorLu
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/condo/domains/notification/schema/SyncRemoteClientService.js (1)
162-162: Remove the AI-note comment in resolver path.This comment is not a stable engineering rationale and adds noise to a critical flow.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/condo/domains/notification/schema/SyncRemoteClientService.js` at line 162, Remove the transient AI-note comment "// AI says that direct modification can affect logs" from the resolver path in SyncRemoteClientService.js; locate the resolver function or export handling remote client sync in that file (e.g., the resolver or handler function that contains this inline comment) and delete the comment so the code contains only stable, engineering-focused comments or none at all.
🤖 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/schema/SyncRemoteClientService.js`:
- Around line 155-164: The code currently destructures pushTokensRaw from args
and does const pushTokensInput = [...pushTokensRaw], which throws if
pushTokensRaw is explicitly null; change this to guard against nullish values
before spreading (e.g., use a safe source like pushTokensRaw ?? [] or set a
local safePushTokens = pushTokensRaw == null ? [] : pushTokensRaw and then
spread that). Update the logic around the args destructuring and replace the
creation of pushTokensInput to use the null-guarded array to avoid TypeError
when pushTokens is null.
In `@apps/condo/domains/notification/schema/SyncRemoteClientService.test.js`:
- Around line 877-893: The assertion uses a hardcoded property payload.pushToken
causing the pushTokenVoIP case to be ignored; update the assertions in the test
block that reads deviceFromDB to compare against the dynamic fields using the
parametrized keys (use payload[remoteClientField] instead of payload.pushToken
and ensure push transport comparison uses payload[pushTransportField] already
present), locating the test around syncRemoteClientByTestClient,
RemoteClient.getOne and the variables payload, pushToken, payloadWithBoth,
remoteClientField and pushTransportField; replace the literal property access
with bracket notation using the parametrized field names so each case validates
the correct expected value.
In `@apps/condo/domains/notification/utils/testSchema/utils.js`:
- Around line 33-39: The getRandomPushTokenData logic biases and overrides
explicit flags by using truthy checks and deriving canBeUsedAsSimplePush as the
negation of canBeUsedAsVoIP; fix by computing each flag independently using
presence checks (e.g., Object.prototype.hasOwnProperty.call(extraAttrs,
'canBeUsedAsVoIP')) and defaulting to faker.datatype.boolean() when not
provided, do the same for canBeUsedAsSimplePush, stop deriving one from the
other, and keep ...extraAttrs in the returned object so explicit values still
override the defaults.
---
Nitpick comments:
In `@apps/condo/domains/notification/schema/SyncRemoteClientService.js`:
- Line 162: Remove the transient AI-note comment "// AI says that direct
modification can affect logs" from the resolver path in
SyncRemoteClientService.js; locate the resolver function or export handling
remote client sync in that file (e.g., the resolver or handler function that
contains this inline comment) and delete the comment so the code contains only
stable, engineering-focused comments or none at all.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9ce29f6a-1d4d-4956-a208-e0865326221b
📒 Files selected for processing (8)
apps/condo/domains/notification/constants/errors.jsapps/condo/domains/notification/schema/SyncRemoteClientService.jsapps/condo/domains/notification/schema/SyncRemoteClientService.spec.jsapps/condo/domains/notification/schema/SyncRemoteClientService.test.jsapps/condo/domains/notification/utils/serverSchema/syncRemoteClient/pushTokensInput.jsapps/condo/domains/notification/utils/serverSchema/syncRemoteClient/pushTokensInput.spec.jsapps/condo/domains/notification/utils/testSchema/utils.jsapps/condo/domains/user/utils/serverSchema/validateHelpers.js
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/condo/domains/notification/utils/serverSchema/syncRemoteClient/pushTokensInput.spec.js
- apps/condo/domains/notification/constants/errors.js
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
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/utils/serverSchema/syncRemoteClient/pushTokensInput.spec.js (1)
724-741:⚠️ Potential issue | 🟠 MajorMake the “too many tokens” case deterministic to avoid flaky failures.
Line [724] pulls random
canBeUsedAsVoIP/canBeUsedAsSimplePushvalues. If both becomefalse, this case can fail with a different validation error than expected.Deterministic test setup
- const samplePushToken = pick(getRandomPushTokenData(), ['canBeUsedAsVoIP', 'canBeUsedAsSimplePush', 'transport']) + const samplePushToken = pick( + getRandomPushTokenData({ + canBeUsedAsVoIP: true, + canBeUsedAsSimplePush: false, + }), + ['canBeUsedAsVoIP', 'canBeUsedAsSimplePush', 'transport'], + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/condo/domains/notification/utils/serverSchema/syncRemoteClient/pushTokensInput.spec.js` around lines 724 - 741, The test randomly sets canBeUsedAsVoIP/canBeUsedAsSimplePush which can both be false and cause a different validation error; make the "too many tokens" case deterministic by explicitly setting at least one of those flags to true (e.g., set canBeUsedAsSimplePush: true) when constructing the tokens in the failing test case (look for the array/objects where canBeUsedAsVoIP/canBeUsedAsSimplePush are assigned in the spec) so the test always hits the intended "too many tokens" validation path.
♻️ Duplicate comments (1)
apps/condo/domains/notification/schema/SyncRemoteClientService.test.js (1)
877-893:⚠️ Potential issue | 🟡 MinorUse the parametrized payload field in assertion (
pushTokenVoIPpath is not truly checked).Line [890] always asserts
payload.pushToken, so thepushTokenVoIPcase can pass without validating its own input field.Targeted fix
- const payload = getRandomTokenData({ pushTransport: 'apple', pushTransportVoIP: 'apple' }) + const payload = getRandomTokenData({ + pushTransport: 'apple', + pushTransportVoIP: 'huawei', + pushToken: faker.datatype.uuid(), + pushTokenVoIP: faker.datatype.uuid(), + }) @@ - expect(deviceFromDB[remoteClientField]).toEqual(payload.pushToken) + expect(deviceFromDB[remoteClientField]).toEqual(payload[remoteClientField])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/condo/domains/notification/schema/SyncRemoteClientService.test.js` around lines 877 - 893, The test always compares deviceFromDB[remoteClientField] to payload.pushToken, so the VoIP branch isn't actually validated; update the assertions to use the correct payload field based on the VoIP scenario (use payload.pushTokenVoIP when isVoIP is true, otherwise payload.pushToken). Locate the assertions after syncRemoteClientByTestClient (variables: payload, pushToken, payloadWithBoth, device, deviceFromDB, remoteClientField, pushTransportField) and replace the fixed payload.pushToken and payload[pushTransportField] checks with conditional references (or compute expectedToken = isVoIP ? payload.pushTokenVoIP : payload.pushToken and expectedTransport = isVoIP ? payload.pushTransportVoIP : payload.pushTransport) and assert deviceFromDB[remoteClientField] === expectedToken and deviceFromDB[pushTransportField] === expectedTransport.
🧹 Nitpick comments (3)
apps/condo/domains/notification/schema/SyncRemoteClientService.spec.js (1)
64-65: Minor: Remove extra blank line.There are two consecutive blank lines here. Consider removing one for consistency with the rest of the file.
✨ Suggested fix
expect(deviceFromDB1[pushTransportField]).toEqual(pushToken.transport) - const payloadWithBoth2 = {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/condo/domains/notification/schema/SyncRemoteClientService.spec.js` around lines 64 - 65, Remove the unnecessary extra blank line in SyncRemoteClientService.spec.js where two consecutive blank lines appear (inside the test file near the SyncRemoteClientService spec block); leave only a single blank line to match the file's formatting conventions and maintain consistency with surrounding tests.apps/condo/domains/notification/schema/SyncRemoteClientService.js (2)
49-49: Consider wrapping JSON.parse for resilience against malformed config.If
TEMP_PREFERRED_PUSH_TRANSPORTS_BY_APP_IDcontains invalid JSON, this throws at module load and crashes the service. While startup failures are visible, a try-catch with logging would be more graceful.🛠️ Suggested improvement
-const TEMP_PREFERRED_PUSH_TRANSPORTS_BY_APP_ID = JSON.parse(conf.TEMP_PREFERRED_PUSH_TRANSPORTS_BY_APP_ID || '{}') +let TEMP_PREFERRED_PUSH_TRANSPORTS_BY_APP_ID = {} +try { + TEMP_PREFERRED_PUSH_TRANSPORTS_BY_APP_ID = JSON.parse(conf.TEMP_PREFERRED_PUSH_TRANSPORTS_BY_APP_ID || '{}') +} catch (err) { + // Fallback to empty config if malformed + TEMP_PREFERRED_PUSH_TRANSPORTS_BY_APP_ID = {} +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/condo/domains/notification/schema/SyncRemoteClientService.js` at line 49, Wrap the JSON.parse call that initializes TEMP_PREFERRED_PUSH_TRANSPORTS_BY_APP_ID in a try-catch to avoid crashing on malformed config: attempt to parse conf.TEMP_PREFERRED_PUSH_TRANSPORTS_BY_APP_ID, on success assign the parsed object to TEMP_PREFERRED_PUSH_TRANSPORTS_BY_APP_ID, on failure log the error (use the module's logger or console.error) and fall back to an empty object ({}); keep the constant name TEMP_PREFERRED_PUSH_TRANSPORTS_BY_APP_ID and ensure downstream code still receives the fallback object.
162-162: Replace vague comment with meaningful context.The comment "AI says that direct modification can affect logs" is unclear. Consider documenting the actual intent (e.g., preserving original input for logging or avoiding mutation of args).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/condo/domains/notification/schema/SyncRemoteClientService.js` at line 162, Replace the vague comment "AI says that direct modification can affect logs" inside SyncRemoteClientService with a precise explanation of intent (e.g., "Do not mutate the incoming args because we preserve the original args object for structured logging and replay/debugging; copy or clone before modifying"), and mention the affected symbol(s) such as the args parameter and any local variable or method that performs mutation so readers know whether to clone (e.g., reference args and the method in SyncRemoteClientService that modifies request payloads).
🤖 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/schema/SyncRemoteClientService.js`:
- Around line 165-180: The code is unshifting token objects into pushTokensInput
even when pushTransport or pushTransportVoIP is missing, producing malformed
entries; update the conditions to require both the token and its transport
before adding (i.e., replace the current if (pushToken) and if (pushTokenVoIP)
checks with checks that also verify pushTransport and pushTransportVoIP
respectively), so pushTokensInput only receives objects with a defined transport
(or alternatively skip including the transport property unless it is defined) —
locate the logic around pushTokensInput, pushToken, pushTransport,
pushTokenVoIP, and pushTransportVoIP and apply the guard.
In
`@apps/condo/domains/notification/utils/serverSchema/syncRemoteClient/pushTokensInput.js`:
- Around line 33-37: The grouping logic uses user-controlled pushToken.transport
and pushToken.token as plain-object keys allowing prototype pollution; change
pushTokensByTransportAndToken to use a safe map (e.g., Object.create(null) or a
Map) and ensure nested buckets are also created with Object.create(null) (or
nested Maps), or explicitly guard keys like "__proto__", "constructor", etc.;
update the loop that iterates pushTokens and the creation logic for
pushTokensByTransportAndToken and its nested entries (referencing
pushTokensByTransportAndToken, pushTokens, pushToken, transport, token) so keys
cannot mutate object prototypes.
- Around line 63-74: The current loop groups pushTokens by transport+token via
_groupPushTokensByTransportAndToken then reduces duplicates with
reducePushTokensWithSameTokenToOneObject, but it doesn't reject mixed pushType
values for the same {transport, token} before dedup, causing order-dependent
behavior; update the validation inside the for-loop that iterates
Object.values(pushTokensByTransportAndToken) to inspect each
pushTokensWithSameToken group (the arrays from the inner Object.values) and if
those items have more than one distinct pushType value, return the appropriate
error (e.g., PUSH_TOKENS_VALIDATION_ERRORS.TOO_MANY_TOKENS_FOR_TRANSPORT) before
calling reducePushTokensWithSameTokenToOneObject so conflicting pushType
combinations are rejected deterministically.
In `@apps/condo/schema.graphql`:
- Around line 105748-105765: Update the three user-facing messages that
currently say "Password" to reference the deviceKey field instead: in the error
objects with "type": "WRONG_PASSWORD_FORMAT", "INVALID_PASSWORD_LENGTH", and
"PASSWORD_CONSISTS_OF_SMALL_SET_OF_CHARACTERS" replace the "messageForUser"
values to use a deviceKey-friendly phrase (e.g., "Device key must be in string
format", "Device key length must be between 8 and 128 characters", "Device key
must contain at least 4 different characters") so the user-facing wording
matches the input field name.
In `@apps/condo/schema.ts`:
- Around line 50477-50494: The three example error objects that reference
deviceKey use "Password …" in the messageForUser field; update those
messageForUser strings to reference deviceKey (e.g., "Device key must be in
string format", "Device key length must be between 8 and 128 characters",
"Device key must contain at least 4 different characters") so the user-facing
wording matches the mutation input; adjust the messageForUser for the examples
with types WRONG_PASSWORD_FORMAT, INVALID_PASSWORD_LENGTH, and
PASSWORD_CONSISTS_OF_SMALL_SET_OF_CHARACTERS accordingly.
- Around line 100345-100347: The schema currently defines pushTokens as a
documented no-op which can silently drop client data; update the input handling
for pushTokens: in the resolver or input validation that consumes the input
containing pushTokens and pushTransport, if pushTokens is provided and non-empty
either (A) immediately reject with a BAD_USER_INPUT error (e.g., throw
UserInputError or ApolloError with code 'BAD_USER_INPUT' and a clear message) or
(B) implement and enforce explicit precedence logic between pushTokens and
legacy token fields (and update the public docs/comments accordingly); make the
change in the code path that processes the input so the validation is enforced
before any further processing.
---
Outside diff comments:
In
`@apps/condo/domains/notification/utils/serverSchema/syncRemoteClient/pushTokensInput.spec.js`:
- Around line 724-741: The test randomly sets
canBeUsedAsVoIP/canBeUsedAsSimplePush which can both be false and cause a
different validation error; make the "too many tokens" case deterministic by
explicitly setting at least one of those flags to true (e.g., set
canBeUsedAsSimplePush: true) when constructing the tokens in the failing test
case (look for the array/objects where canBeUsedAsVoIP/canBeUsedAsSimplePush are
assigned in the spec) so the test always hits the intended "too many tokens"
validation path.
---
Duplicate comments:
In `@apps/condo/domains/notification/schema/SyncRemoteClientService.test.js`:
- Around line 877-893: The test always compares deviceFromDB[remoteClientField]
to payload.pushToken, so the VoIP branch isn't actually validated; update the
assertions to use the correct payload field based on the VoIP scenario (use
payload.pushTokenVoIP when isVoIP is true, otherwise payload.pushToken). Locate
the assertions after syncRemoteClientByTestClient (variables: payload,
pushToken, payloadWithBoth, device, deviceFromDB, remoteClientField,
pushTransportField) and replace the fixed payload.pushToken and
payload[pushTransportField] checks with conditional references (or compute
expectedToken = isVoIP ? payload.pushTokenVoIP : payload.pushToken and
expectedTransport = isVoIP ? payload.pushTransportVoIP : payload.pushTransport)
and assert deviceFromDB[remoteClientField] === expectedToken and
deviceFromDB[pushTransportField] === expectedTransport.
---
Nitpick comments:
In `@apps/condo/domains/notification/schema/SyncRemoteClientService.js`:
- Line 49: Wrap the JSON.parse call that initializes
TEMP_PREFERRED_PUSH_TRANSPORTS_BY_APP_ID in a try-catch to avoid crashing on
malformed config: attempt to parse
conf.TEMP_PREFERRED_PUSH_TRANSPORTS_BY_APP_ID, on success assign the parsed
object to TEMP_PREFERRED_PUSH_TRANSPORTS_BY_APP_ID, on failure log the error
(use the module's logger or console.error) and fall back to an empty object
({}); keep the constant name TEMP_PREFERRED_PUSH_TRANSPORTS_BY_APP_ID and ensure
downstream code still receives the fallback object.
- Line 162: Replace the vague comment "AI says that direct modification can
affect logs" inside SyncRemoteClientService with a precise explanation of intent
(e.g., "Do not mutate the incoming args because we preserve the original args
object for structured logging and replay/debugging; copy or clone before
modifying"), and mention the affected symbol(s) such as the args parameter and
any local variable or method that performs mutation so readers know whether to
clone (e.g., reference args and the method in SyncRemoteClientService that
modifies request payloads).
In `@apps/condo/domains/notification/schema/SyncRemoteClientService.spec.js`:
- Around line 64-65: Remove the unnecessary extra blank line in
SyncRemoteClientService.spec.js where two consecutive blank lines appear (inside
the test file near the SyncRemoteClientService spec block); leave only a single
blank line to match the file's formatting conventions and maintain consistency
with surrounding tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1fe70236-2e14-4a2f-a84c-9b8f306cfc9a
📒 Files selected for processing (10)
apps/condo/domains/notification/constants/errors.jsapps/condo/domains/notification/schema/SyncRemoteClientService.jsapps/condo/domains/notification/schema/SyncRemoteClientService.spec.jsapps/condo/domains/notification/schema/SyncRemoteClientService.test.jsapps/condo/domains/notification/utils/serverSchema/syncRemoteClient/pushTokensInput.jsapps/condo/domains/notification/utils/serverSchema/syncRemoteClient/pushTokensInput.spec.jsapps/condo/domains/notification/utils/testSchema/utils.jsapps/condo/domains/user/utils/serverSchema/validateHelpers.jsapps/condo/schema.graphqlapps/condo/schema.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/condo/domains/notification/utils/testSchema/utils.js
- apps/condo/domains/notification/constants/errors.js
- apps/condo/domains/user/utils/serverSchema/validateHelpers.js
apps/condo/domains/notification/schema/SyncRemoteClientService.js
Outdated
Show resolved
Hide resolved
apps/condo/domains/notification/utils/serverSchema/syncRemoteClient/pushTokensInput.js
Outdated
Show resolved
Hide resolved
apps/condo/domains/notification/utils/serverSchema/syncRemoteClient/pushTokensInput.js
Outdated
Show resolved
Hide resolved
apps/condo/domains/notification/schema/SyncRemoteClientService.js
Outdated
Show resolved
Hide resolved
apps/condo/domains/notification/schema/SyncRemoteClientService.js
Outdated
Show resolved
Hide resolved
apps/condo/domains/notification/schema/SyncRemoteClientService.js
Outdated
Show resolved
Hide resolved
apps/condo/domains/notification/schema/SyncRemoteClientService.js
Outdated
Show resolved
Hide resolved
apps/condo/domains/notification/schema/SyncRemoteClientService.js
Outdated
Show resolved
Hide resolved
apps/condo/domains/notification/schema/SyncRemoteClientService.js
Outdated
Show resolved
Hide resolved
apps/condo/domains/notification/schema/SyncRemoteClientService.js
Outdated
Show resolved
Hide resolved
apps/condo/domains/notification/schema/SyncRemoteClientService.js
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
apps/condo/domains/notification/utils/serverSchema/syncRemoteClient/pushTokensInput.js
Outdated
Show resolved
Hide resolved
apps/condo/domains/notification/utils/serverSchema/syncRemoteClient/pushTokensInput.js
Outdated
Show resolved
Hide resolved
apps/condo/domains/notification/utils/serverSchema/syncRemoteClient/pushTokensInput.js
Outdated
Show resolved
Hide resolved
apps/condo/domains/notification/utils/serverSchema/syncRemoteClient/pushTokensInput.js
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
apps/condo/domains/notification/utils/serverSchema/syncRemoteClient/pushTokensInput.js
Show resolved
Hide resolved
apps/condo/domains/notification/utils/serverSchema/syncRemoteClient/pushTokensInput.js
Show resolved
Hide resolved
apps/condo/domains/notification/utils/serverSchema/syncRemoteClient/pushTokensInput.js
Outdated
Show resolved
Hide resolved
apps/condo/domains/notification/utils/serverSchema/remoteClient/validateHelpers.js
Outdated
Show resolved
Hide resolved
apps/condo/domains/notification/utils/serverSchema/remoteClient/validateHelpers.js
Outdated
Show resolved
Hide resolved
apps/condo/domains/notification/utils/serverSchema/remoteClient/validateHelpers.js
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/condo/domains/notification/schema/SyncRemoteClientService.test.js (1)
843-865: Remove redundantadminclient creation.Line 845 creates a new admin client, but there's already one available from the
beforeAllat line 20. The other tests in this describe block correctly use the outeradminvariable.♻️ Proposed fix
test.each([ { remoteClientField: 'pushToken' }, { remoteClientField: 'pushTokenVoIP' }, ])('selects $remoteClientField from pushTokens as first pushToken of needed type', async ({ remoteClientField }) => { const client = await makeClient() - const admin = await makeLoggedInAdminClient() const payload = getRandomTokenData({ pushTransport: 'huawei', pushTransportVoIP: 'huawei', [remoteClientField]: null })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/condo/domains/notification/schema/SyncRemoteClientService.test.js` around lines 843 - 865, The test creates a redundant local admin client via makeLoggedInAdminClient() shadowing the outer admin from beforeAll; remove the local admin assignment (const admin = await makeLoggedInAdminClient()) and reuse the outer admin variable when calling RemoteClient.getOne so the test uses the shared admin set up earlier (affects the test block that calls makeClient(), syncRemoteClientByTestClient(), and RemoteClient.getOne()).
🤖 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/schema/SyncRemoteClientService.js`:
- Around line 147-155: The code spreads pushTokensRaw into pushTokensInput
causing a TypeError when clients send explicit null (GraphQL may pass
pushTokens: null); update the logic around the args destructuring so
pushTokensInput is created defensively, e.g. check Array.isArray(pushTokensRaw)
(or pushTokensRaw != null && Array.isArray(...)) and use an empty array when
it's null/invalid before spreading; apply this change where pushTokensRaw and
pushTokensInput are defined to prevent pushTokensRaw from being treated as
iterable.
In
`@apps/condo/domains/notification/utils/serverSchema/syncRemoteClient/pushTokensInput.js`:
- Around line 13-18: The validator and reducer in pushTokensInput.js still use
legacy flags isVoIP/isPush causing valid inputs using the new PushToken fields
to be rejected and deduped incorrectly; update the INVALID_PUSH_TOKEN message
and all validation/merging logic to check and set token fields named
canBeUsedAsVoIP and canBeUsedAsSimplePush (and preserve token and transport),
replace any reads/writes of isVoIP/isPush with the new field names, and ensure
deduplication/merge combines capabilities into
canBeUsedAsVoIP/canBeUsedAsSimplePush rather than the old properties so the
helper matches the GraphQL PushToken input shape (token, transport,
canBeUsedAsVoIP, canBeUsedAsSimplePush).
---
Nitpick comments:
In `@apps/condo/domains/notification/schema/SyncRemoteClientService.test.js`:
- Around line 843-865: The test creates a redundant local admin client via
makeLoggedInAdminClient() shadowing the outer admin from beforeAll; remove the
local admin assignment (const admin = await makeLoggedInAdminClient()) and reuse
the outer admin variable when calling RemoteClient.getOne so the test uses the
shared admin set up earlier (affects the test block that calls makeClient(),
syncRemoteClientByTestClient(), and RemoteClient.getOne()).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cecd46f2-e408-4885-aa56-9b8ca3eae072
📒 Files selected for processing (7)
apps/condo/domains/notification/constants/errors.jsapps/condo/domains/notification/schema/SyncRemoteClientService.jsapps/condo/domains/notification/schema/SyncRemoteClientService.test.jsapps/condo/domains/notification/utils/serverSchema/syncRemoteClient/pushTokensInput.jsapps/condo/domains/notification/utils/serverSchema/syncRemoteClient/pushTokensInput.spec.jsapps/condo/schema.graphqlapps/condo/schema.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/condo/domains/notification/utils/serverSchema/syncRemoteClient/pushTokensInput.spec.js
- apps/condo/domains/notification/constants/errors.js
apps/condo/domains/notification/utils/serverSchema/syncRemoteClient/pushTokensInput.js
Show resolved
Hide resolved
apps/condo/domains/notification/utils/serverSchema/syncRemoteClient/pushTokensInput.js
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
apps/condo/domains/common/utils/collections.js (1)
7-16:⚠️ Potential issue | 🔴 CriticalUse a safe, collision-free key store in
groupBy.This accumulator is keyed by arbitrary field values on a plain object. Keys like
__proto__/constructorcan break the bucket lookup, and tuple encoding viajoin(':')can merge distinct groups such as['a:b', 'c']and['a', 'b:c']. AMapkeyed by a collision-free tuple serialization avoids both bugs.Suggested fix
function groupBy (objs, keys) { - const groups = {} + const groups = new Map() for (const obj of objs) { - const groupKey = keys.map(k => obj[k]).join(':') - if (!groups[groupKey]) groups[groupKey] = [] - groups[groupKey].push(obj) + const groupKey = JSON.stringify(keys.map(key => obj[key])) + if (!groups.has(groupKey)) groups.set(groupKey, []) + groups.get(groupKey).push(obj) } - return Object.values(groups) + return Array.from(groups.values()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/condo/domains/common/utils/collections.js` around lines 7 - 16, The groupBy implementation uses a plain object and join(':') which is vulnerable to prototype key collisions and ambiguous tuple encoding; change groups to a Map and compute group keys using a collision-free serialization (e.g., JSON.stringify(keys.map(k => obj[k]))) instead of keys.map(...).join(':'), then use Map.prototype.get/set to collect arrays and finally return Array.from(groups.values()); update references to the variables groups and groupKey in the groupBy function accordingly.apps/condo/domains/notification/utils/serverSchema/syncRemoteClient/pushTokensInput.js (1)
11-15:⚠️ Potential issue | 🔴 CriticalUse the v2
PushTokenfield names throughout this helper.This code still validates and merges
isVoIP/isPush, but the resolver passes rawpushTokensinto this helper. If the current input shape iscanBeUsedAsVoIP/canBeUsedAsSimplePush, valid GraphQL input will fail validation on Line 50 and dedup back into the wrong properties.Suggested fix
const PUSH_TOKENS_VALIDATION_ERRORS = { UNUSABLE_TOKEN_PROVIDED: { code: BAD_USER_INPUT, type: UNUSABLE_TOKEN_PROVIDED, - message: 'Any push token must set "isVoIP" or "isPush" or both as "true"', + message: 'Any push token must set "canBeUsedAsVoIP" or "canBeUsedAsSimplePush" or both as "true"', }, @@ - const isVoIP = !!pushTokensWithSameToken.find(pushToken => pushToken.isVoIP) - const isPush = !!pushTokensWithSameToken.find(pushToken => pushToken.isPush) + const canBeUsedAsVoIP = pushTokensWithSameToken.some(pushToken => pushToken.canBeUsedAsVoIP) + const canBeUsedAsSimplePush = pushTokensWithSameToken.some(pushToken => pushToken.canBeUsedAsSimplePush) return { ...firstToken, - isVoIP, - isPush, + canBeUsedAsVoIP, + canBeUsedAsSimplePush, } } @@ - const tokenWithoutAllowedVoIPAndDefaultPush = pushTokens.find(pushToken => !pushToken.isVoIP && !pushToken.isPush) + const tokenWithoutAllowedVoIPAndDefaultPush = pushTokens.find(pushToken => !pushToken.canBeUsedAsVoIP && !pushToken.canBeUsedAsSimplePush) @@ - const voipTokens = allPushTokensForTransport.filter(token => token.isVoIP) - const defaultTokens = allPushTokensForTransport.filter(token => token.isPush) + const voipTokens = allPushTokensForTransport.filter(token => token.canBeUsedAsVoIP) + const defaultTokens = allPushTokensForTransport.filter(token => token.canBeUsedAsSimplePush)Based on learnings, the
PushTokenGraphQL input inapps/condo/domains/notification/schema/SyncRemoteClientService.jsonly includestoken,transport,canBeUsedAsVoIP, andcanBeUsedAsSimplePushin this PR.Also applies to: 36-41, 50-65
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/condo/domains/notification/utils/serverSchema/syncRemoteClient/pushTokensInput.js` around lines 11 - 15, The helper is still using legacy fields isVoIP/isPush; update all occurrences in this file (including the UNUSABLE_TOKEN_PROVIDED message and the validation/merge logic) to use the v2 PushToken field names canBeUsedAsVoIP and canBeUsedAsSimplePush so incoming GraphQL input (token, transport, canBeUsedAsVoIP, canBeUsedAsSimplePush) validates correctly; specifically replace checks, merges, dedupe logic that reference isVoIP/isPush with canBeUsedAsVoIP/canBeUsedAsSimplePush and update the error message text and any property normalization to honor the v2 names.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/condo/domains/common/utils/collections.js`:
- Around line 7-16: The groupBy implementation uses a plain object and join(':')
which is vulnerable to prototype key collisions and ambiguous tuple encoding;
change groups to a Map and compute group keys using a collision-free
serialization (e.g., JSON.stringify(keys.map(k => obj[k]))) instead of
keys.map(...).join(':'), then use Map.prototype.get/set to collect arrays and
finally return Array.from(groups.values()); update references to the variables
groups and groupKey in the groupBy function accordingly.
In
`@apps/condo/domains/notification/utils/serverSchema/syncRemoteClient/pushTokensInput.js`:
- Around line 11-15: The helper is still using legacy fields isVoIP/isPush;
update all occurrences in this file (including the UNUSABLE_TOKEN_PROVIDED
message and the validation/merge logic) to use the v2 PushToken field names
canBeUsedAsVoIP and canBeUsedAsSimplePush so incoming GraphQL input (token,
transport, canBeUsedAsVoIP, canBeUsedAsSimplePush) validates correctly;
specifically replace checks, merges, dedupe logic that reference isVoIP/isPush
with canBeUsedAsVoIP/canBeUsedAsSimplePush and update the error message text and
any property normalization to honor the v2 names.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ff063ca3-af79-4ba6-bd00-876a566c7087
📒 Files selected for processing (2)
apps/condo/domains/common/utils/collections.jsapps/condo/domains/notification/utils/serverSchema/syncRemoteClient/pushTokensInput.js
Contributor
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
Contributor
Author
|
for some reason in ci error message check with (expect().rejects.toThrow()) works 50/50 |
|
SavelevMatthew
deleted the
feat/condo/DOMA-13020/support-multiple-tokens-in-syncReomteClient
branch
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
2 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.
Here we only changing api and adding validation to new fields. Actual logic will be added in another PR.
TODO:
Summary by CodeRabbit
New Features
API Changes
Improvements
Tests