feat: handle devices notification#2436
Conversation
…e IDs" This reverts commit c03f9d8.
|
Thanks for opening this pull request and contributing to the project! The next step is for the maintainers to review your changes. If everything looks good, it will be approved and merged into the main branch. In the meantime, anyone in the community is encouraged to test this pull request and provide feedback. ✅ How to confirm it worksIf you’ve tested this PR, please comment below with: This helps us speed up the review and merge process. 📦 To test this PR locally:If you encounter any issues or have feedback, feel free to comment as well. |
|
|
This PR is stale because it has been open for 14 days with no activity. Remove the stale label or comment or this will be closed in 14 days |
|
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:
📝 WalkthroughWalkthroughDevice synchronization infrastructure is enhanced: a timeout constant is added to defaults, a devicesMutex and userDevicesCache are introduced for thread-safe device updates, a new handleDevicesNotification function processes incoming device notifications with add/update/remove semantics, and call latency handling is extended to relay latency events. ChangesDevice Synchronization Infrastructure
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
Right! Fixed @purpshell |
There was a problem hiding this comment.
2 issues found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/Socket/messages-recv.ts">
<violation number="1" location="src/Socket/messages-recv.ts:800">
P2: Device notifications are only processed for the first device/child, so later device changes in the same stanza are ignored.</violation>
</file>
<file name="src/Defaults/index.ts">
<violation number="1" location="src/Defaults/index.ts:96">
P1: Stateful `userDevicesCache` instance is embedded in shared `DEFAULT_CONNECTION_CONFIG` and will be unintentionally shared across all sockets because socket initialization shallow-spreads defaults.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Defaults/index.ts`:
- Line 56: Remove the stray trailing whitespace artifacts in
src/Defaults/index.ts: delete the extra blank line introduced in the top-level
export/block around the Defaults module (the empty line at Line 56) and remove
the trailing tab character at Line 100; ensure the file ends with a single
newline and no trailing spaces so Prettier passes.
- Around line 96-99: DEFAULT_CONNECTION_CONFIG currently creates a singleton
userDevicesCache at module load which causes cross-account contamination; remove
userDevicesCache from DEFAULT_CONNECTION_CONFIG and instead provide a per-socket
fallback inside the socket factory (e.g. in makeMessagesSocket or
makeMessagesRecvSocket) following the same pattern used for msgRetryCounterCache
in messages-recv.ts: read config.userDevicesCache and if falsy create a new
NodeCache with stdTTL DEFAULT_CACHE_TTLS.USER_DEVICES and useClones:false, then
use that instance for the socket so each makeWASocket call gets its own
userDevicesCache.
In `@src/Socket/messages-recv.ts`:
- Around line 828-831: The 'add' branch in messages-recv.ts is storing only {
user, device } (via updatedDevices) after jidDecode returned a FullJid (which
includes server), causing messages-send.ts's jidEncode(d.user, d.server,
d.device) to receive undefined server; fix by including server when creating the
cached entry (i.e., change the object pushed in updatedDevices in the case 'add'
branch to include server from the decoded JID so the cache stores FullJid-like
entries used by jidEncode/jidDecode and ensures the new device receives
messages).
- Around line 800-802: The current handler only reads devices[0] (deviceJid) and
ignores additional <device> entries; update the logic to iterate over all
entries in devices (e.g., for each deviceEl in devices) decode each JID with
jidDecode and handle the tag (child!.tag as 'add' | 'remove' | 'update') for
every device; acquire the existing mutex-protected cache for the user once,
mutate the user's device list for each device entry (applying add/remove/update)
and then persist the single updated cache entry before releasing the mutex so
you do one get/set per user rather than one per device.
- Around line 833-843: When handling the device removal branch, if
updatedDevices is empty you must remove the user's cache entry instead of
skipping set so stale entries don't persist; update the logic around
updatedDevices / existingCache in the switch handling (the block using logger,
deviceHash, device and the surrounding switch) to call the cache deletion API
(e.g., userDevicesCache?.del(user) or .delete(user) depending on your cache
implementation) when updatedDevices.length === 0, and keep the existing
userDevicesCache?.set(user, updatedDevices) path when updatedDevices is
non-empty.
- Around line 785-807: The handler handleDevicesNotification is dereferencing
child and deviceJid too early; first ensure getAllBinaryNodeChildren(node)
returned a child and devices is non-empty before accessing child.tag or calling
jidDecode. Specifically: check that child exists (and has expected attrs) before
using child!.tag or child!.attrs.device_hash, move the guard that verifies
deviceJid (and devices.length) before calling jidDecode(deviceJid), remove the
non-null assertions (the ! uses) and defensively handle jidDecode returning
undefined (e.g., bail out or log and return) so destructuring const { user,
device } = ... cannot throw.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b0b78375-99a3-4430-9396-1cf4a0fbcf0e
📒 Files selected for processing (2)
src/Defaults/index.tssrc/Socket/messages-recv.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/Socket/messages-recv.ts (4)
845-847:⚠️ Potential issue | 🟠 MajorDelete cache key when removal leaves no devices.
When
updatedDevicesbecomes empty, the stale user entry is kept instead of removed. This can keep routing to removed devices until TTL expiry.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Socket/messages-recv.ts` around lines 845 - 847, The cache update logic in messages-recv.ts currently only sets userDevicesCache when updatedDevices.length > 0, leaving a stale entry when updatedDevices is empty; change it so that when updatedDevices is empty you remove the cache key for the user (call the cache delete/remove method on userDevicesCache with the user key) and otherwise await userDevicesCache?.set(user, updatedDevices) so no empty device lists remain in cache.
800-843:⚠️ Potential issue | 🟠 MajorProcess all
<device>entries, not justdevices[0].Current logic only applies
add/remove/updateto the first device, so multi-device notifications can leave cache/session state partially updated.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Socket/messages-recv.ts` around lines 800 - 843, The code only processes devices[0], causing partial updates for multi-device notifications; inside the devicesMutex.mutex callback iterate over all entries in devices (not just devices[0]) and for each device entry: extract deviceJid, call jidDecode(deviceJid) to get { user, device }, derive the tag from child!.tag, and apply the tag-specific logic per device (for 'remove' call signalRepository.deleteSession with that deviceJid; for 'add' and 'remove' update the existingCache by adding/removing the specific { user, device } entry). Update userDevicesCache once after processing all device entries (using the computed updatedDevices), and preserve logging via logger.info/debug and using deviceHash/child!.attrs.device_hash per entry as appropriate. Ensure any early returns (e.g., no deviceJid) only skip that device, not the whole loop.
834-835:⚠️ Potential issue | 🟠 MajorPreserve
serverwhen caching added devices.The
addbranch stores{ user, device }only; if downstream usesjidEncode(user, server, device), missingserverbreaks routing for newly added devices.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Socket/messages-recv.ts` around lines 834 - 835, The add-branch cache update removes the server field and currently sets updatedDevices = [...existingCache.filter(d => d.device !== device), { user, device }]; preserve the original server by including it in the cached object (e.g., { user, server, device }) so downstream callers using jidEncode(user, server, device) have the server available; update the code paths that construct the new cached entry in the add branch (and any similar cache-write sites) to include the server value taken from the current scope or existingCache entry.
786-803:⚠️ Potential issue | 🔴 CriticalAdd null/undefined guards before accessing
childand decoded JID.
childand decoded JID are dereferenced too early (Line 789, Line 801, Line 802). If the node is malformed, this can still throw before reaching the current guards.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Socket/messages-recv.ts` around lines 786 - 803, The code dereferences child and the result of jidDecode too early; update the notification handling in messages-recv.ts (around getAllBinaryNodeChildren/getBinaryNodeChildren usage) to first verify child exists and is an object before calling getBinaryNodeChildren or accessing child.tag, and guard device access (ensure devices && devices.length && devices[0]) before reading devices[0].attrs.jid; then guard the decoded JID by calling const decoded = jidDecode(deviceJid) and returning/logging if decoded is null/undefined before destructuring { user, device } and before using tag — remove non-null assertions (child! and jidDecode(...)!) and add early returns or logging when these values are missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Socket/messages-recv.ts`:
- Around line 816-818: The remove branch is calling the async
signalRepository.deleteSession([deviceJid]) without awaiting it which can race
with cache updates and triggers a lint error; update the handler so the call is
awaited (await signalRepository.deleteSession([deviceJid])) and ensure the
enclosing function is declared async (or properly handles the returned promise)
so the deletion completes before proceeding, referencing the tag === 'remove'
branch and the signalRepository.deleteSession call to locate the change.
---
Duplicate comments:
In `@src/Socket/messages-recv.ts`:
- Around line 845-847: The cache update logic in messages-recv.ts currently only
sets userDevicesCache when updatedDevices.length > 0, leaving a stale entry when
updatedDevices is empty; change it so that when updatedDevices is empty you
remove the cache key for the user (call the cache delete/remove method on
userDevicesCache with the user key) and otherwise await
userDevicesCache?.set(user, updatedDevices) so no empty device lists remain in
cache.
- Around line 800-843: The code only processes devices[0], causing partial
updates for multi-device notifications; inside the devicesMutex.mutex callback
iterate over all entries in devices (not just devices[0]) and for each device
entry: extract deviceJid, call jidDecode(deviceJid) to get { user, device },
derive the tag from child!.tag, and apply the tag-specific logic per device (for
'remove' call signalRepository.deleteSession with that deviceJid; for 'add' and
'remove' update the existingCache by adding/removing the specific { user, device
} entry). Update userDevicesCache once after processing all device entries
(using the computed updatedDevices), and preserve logging via logger.info/debug
and using deviceHash/child!.attrs.device_hash per entry as appropriate. Ensure
any early returns (e.g., no deviceJid) only skip that device, not the whole
loop.
- Around line 834-835: The add-branch cache update removes the server field and
currently sets updatedDevices = [...existingCache.filter(d => d.device !==
device), { user, device }]; preserve the original server by including it in the
cached object (e.g., { user, server, device }) so downstream callers using
jidEncode(user, server, device) have the server available; update the code paths
that construct the new cached entry in the add branch (and any similar
cache-write sites) to include the server value taken from the current scope or
existingCache entry.
- Around line 786-803: The code dereferences child and the result of jidDecode
too early; update the notification handling in messages-recv.ts (around
getAllBinaryNodeChildren/getBinaryNodeChildren usage) to first verify child
exists and is an object before calling getBinaryNodeChildren or accessing
child.tag, and guard device access (ensure devices && devices.length &&
devices[0]) before reading devices[0].attrs.jid; then guard the decoded JID by
calling const decoded = jidDecode(deviceJid) and returning/logging if decoded is
null/undefined before destructuring { user, device } and before using tag —
remove non-null assertions (child! and jidDecode(...)!) and add early returns or
logging when these values are missing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2bc234de-abfb-4f36-ab48-8ff491c32a1d
📒 Files selected for processing (1)
src/Socket/messages-recv.ts
…n handler
- Defaults: drop the stateful userDevicesCache singleton from
DEFAULT_CONNECTION_CONFIG. The cache was created at module load and
shared across every makeWASocket() call, leaking device lists across
accounts. The per-socket fallback in makeMessagesSocket already covers
the unset case; expose that instance on sock so makeMessagesRecvSocket
shares it instead of pulling the (now absent) config singleton. Also
cleans the stray blank line and trailing tab the linter was flagging.
- handleDevicesNotification: the previous handler had several bugs that
this change fixes together to keep the logic in one piece:
* Guard for missing child / undecodable JIDs before any deref so
child!.tag and jidDecode(...)! cannot throw.
* Iterate every <device> entry instead of only devices[0]; later
devices in the same stanza were silently ignored.
* Include 'server' alongside user/device when caching new devices,
matching FullJid shape; messages-send.ts uses jidEncode(d.user,
d.server, d.device) and was getting undefined server.
* When a 'remove' empties the cache list, delete the user's entry
instead of leaving stale data behind.
* await signalRepository.deleteSession(...) so the no-floating-promises
rule is satisfied.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Socket/messages-recv.ts`:
- Around line 778-869: The devicesMutex used in handleDevicesNotification does
not protect other writers to userDevicesCache (e.g., USync refresh code in
src/Socket/messages-send.ts that calls userDevicesCache.mset()/set()), causing
races; fix by consolidating cache serialization: expose or move the same
devicesMutex so both handleDevicesNotification and the USync refresh / methods
that call userDevicesCache.mset()/set() acquire devicesMutex.mutex(...) around
any modifications to userDevicesCache; update messages-send.ts (the USync
refresh / any functions that write userDevicesCache) to wrap their cache writes
with the same devicesMutex, and ensure devicesMutex is exported/shared (or lift
it to a shared module) so all code paths (handleDevicesNotification, USync
refresh, signalRepository-related code) serialize cache mutations.
- Around line 838-853: The incremental "add" branch seeds userDevicesCache with
only the notification delta when existingCache is empty (variables:
existingCache, tag === 'add', entries, updatedDevices), causing getUSyncDevices
to treat the partial list as authoritative; change the logic so when
existingCache is empty and tag === 'add' you trigger a full fetch/seed instead
of applying the delta—i.e., detect empty existingCache before the switch and
either call the full USync device fetch (or skip caching the delta) or merge by
obtaining the complete device list, then assign updatedDevices and persist to
userDevicesCache so the cache contains a full baseline rather than only the
delta.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: eb722c93-17af-4c68-87d9-4ffe77df4d32
📒 Files selected for processing (5)
src/Defaults/index.tssrc/Socket/chats.tssrc/Socket/messages-recv.tssrc/Socket/messages-send.tssrc/Utils/companion-reg-client-utils.ts
✅ Files skipped from review due to trivial changes (1)
- src/Utils/companion-reg-client-utils.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Defaults/index.ts
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/Socket/messages-recv.ts">
<violation number="1" location="src/Socket/messages-recv.ts:839">
P1: Skip incremental `add` cache writes when the user has no cached baseline; otherwise this can persist a partial device list and miss other devices until a full refresh repopulates the cache.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
…o baseline - Move devicesMutex into makeMessagesSocket alongside userDevicesCache and expose it on sock so handleDevicesNotification (in messages-recv) and the USync-refresh write block in getUSyncDevices serialize against the same lock. Previously a USync .mset()/.set() could race with a notification's read-modify-write on the same key. - handleDevicesNotification: when a device 'add' arrives but the cache has no baseline for that user, skip persisting the delta. Caching only the notification entries made the partial list look authoritative to getUSyncDevices, which would then stop messaging the user's other devices. Letting the cache stay empty defers to the next USync refresh.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Socket/messages-recv.ts`:
- Around line 833-835: When handling the 'remove' tag after calling
signalRepository.deleteSession(entries.map(e => e.jid)), also invalidate the
in-memory peer session cache so assertSessions() won’t trust stale entries;
iterate the same JIDs and remove their entries from peerSessionsCache (or clear
their TTL) to force a refetch on the send path (refer to deleteSession,
peerSessionsCache, and assertSessions to locate the logic).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d7aa2258-a51b-4cff-afe3-aea946b1d91e
📒 Files selected for processing (2)
src/Socket/messages-recv.tssrc/Socket/messages-send.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/Socket/messages-recv.ts (1)
949-949: 💤 Low valueUnsafe
tagcast — unknown tags reach the cache mutation path.
child.tag as 'add' | 'remove' | 'update'is a pure type assertion; if WhatsApp sends a tag that isn't one of these three, the early-exit checks at Lines 988 and 994 are bypassed and execution falls through to theswitchat Line 1009. Thedefaultarm at Line 1021 saves you for the cache-mutation step, but thesignalRepository.deleteSession(...)call at Line 994 runs only fortag === 'remove', while the cache-emptiness check at Line 999 already returnedcontinuefor empty caches, so the actual risk surface is small — primarily a misleading type if a new tag (e.g. some future variant) is introduced.Consider validating up front and bailing on unknown tags so the contract is explicit:
♻️ Suggested guard
- const tag = child.tag as 'add' | 'remove' | 'update' + const tag = child.tag + if (tag !== 'add' && tag !== 'remove' && tag !== 'update') { + logger.debug({ tag, from }, 'unknown devices notification tag, skipping') + return + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Socket/messages-recv.ts` at line 949, child.tag is asserted to 'add'|'remove'|'update' but not validated at runtime, which can let unknown tags pass into the cache mutation path; change the code that reads child.tag (the variable tag used in the switch and in the signalRepository.deleteSession call) to perform a runtime check (e.g. if (tag !== 'add' && tag !== 'remove' && tag !== 'update') continue/return) before any early-exits or the switch, so unknown tags are explicitly ignored and the subsequent logic (including the cache-empty check and signalRepository.deleteSession) only runs for the three valid values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/Socket/messages-recv.ts`:
- Line 949: child.tag is asserted to 'add'|'remove'|'update' but not validated
at runtime, which can let unknown tags pass into the cache mutation path; change
the code that reads child.tag (the variable tag used in the switch and in the
signalRepository.deleteSession call) to perform a runtime check (e.g. if (tag
!== 'add' && tag !== 'remove' && tag !== 'update') continue/return) before any
early-exits or the switch, so unknown tags are explicitly ignored and the
subsequent logic (including the cache-empty check and
signalRepository.deleteSession) only runs for the three valid values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 750983c6-74c3-4429-911b-2a07742eb5cd
📒 Files selected for processing (3)
src/Defaults/index.tssrc/Socket/messages-recv.tssrc/Socket/messages-send.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Socket/messages-send.ts
…hiskeySockets#2517), reachout/limits XWAs (WhiskeySockets#2442/WhiskeySockets#2445), retry SKDM (WhiskeySockets#2506/WhiskeySockets#2507), app state resilience (WhiskeySockets#2456), null/undefined hardening (WhiskeySockets#1892), devices notification (WhiskeySockets#2436), companion registration (WhiskeySockets#2460), pastParticipants (WhiskeySockets#2426), LID mapping mex (WhiskeySockets#2472) Resolved chats.ts (kept fork space-indent style) and package.json (kept fork name/description, bumped to 7.0.0-rc.18 mirroring upstream rc10).
(cherry picked from commit 800042f)
This pull request introduces enhancements to device notification handling and caching in the messaging socket. The main focus is on adding a dedicated cache for user devices, improving the processing of device notifications, and refactoring related code for better maintainability and performance.
Device notification and caching improvements:
userDevicesCacheto theDEFAULT_CONNECTION_CONFIGusingNodeCache, with a default TTL of 5 minutes, to efficiently store and manage user device lists. (src/Defaults/index.ts, [1] [2]handleDevicesNotificationfunction, which processes device additions, removals, and updates, and synchronizes cache changes using a mutex. (src/Socket/messages-recv.ts, src/Socket/messages-recv.tsR724-R787)processNotificationto use the new handler and improved error handling. (src/Socket/messages-recv.ts, src/Socket/messages-recv.tsL741-L751)Type and import updates:
JidWithDevicetype and updated imports to support improved device handling. (src/Socket/messages-recv.ts, src/Socket/messages-recv.tsR69-R84)src/Defaults/index.tsto includePossiblyExtendedCacheStorefor the new cache.Cleanup:
DEFAULT_CACHE_TTLSdefinition from the bottom ofsrc/Defaults/index.tsafter moving it to the top.Summary by CodeRabbit
New Features
Improvements