fix: Parse device JID attribute in device notifications - #243
Conversation
|
Caution Review failedThe pull request is closed. Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds a type-safe device notification parser under a new Changes
Sequence Diagram(s)sequenceDiagram
participant Handler as Notification Handler
participant Parser as DeviceNotification Parser
participant EventSys as Event System
participant Cache as LID-PN Cache
Handler->>Parser: pass <notification type="devices"> node
Parser->>Parser: try_parse() → DeviceNotification (from, lid_user, id, ts, operation)
Parser-->>Handler: DeviceNotification
Handler->>Handler: build DeviceListUpdate (update_type, devices, key_index?, contact_hash?)
Handler->>Cache: persist lid→pn mapping (LearningSource::DeviceNotification) [if present]
Handler->>EventSys: dispatch DeviceListUpdate(user, lid_user?, ...)
EventSys->>Cache: invalidate user device cache
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
Bencher ReportThere was a problem hiding this comment.
Pull request overview
This PR fixes the parsing of device notifications in the WhatsApp protocol to use the jid attribute (which contains the full JID including device ID) instead of a simple id attribute. It also adds support for learning LID-PN mappings from device notifications.
Changes:
- Adds a new
wacore::stanza::devicesmodule with type-safe parsing for device notification stanzas - Updates device notification handling to extract device IDs from JID attributes and learn LID-PN mappings when available
- Adds
DeviceNotificationas a newLearningSourcefor LID-PN cache mappings
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
wacore/src/stanza/mod.rs |
New module declaration for stanza types |
wacore/src/stanza/devices.rs |
Type-safe parser for device notification stanzas with JID-based device elements |
wacore/src/types/lid_pn.rs |
Adds DeviceNotification variant to LearningSource enum |
wacore/src/types/events.rs |
Updates DeviceListUpdate structure with more detailed device info and LID support (breaking change) |
wacore/src/lib.rs |
Registers new stanza module |
src/handlers/notification.rs |
Refactors device notification handling to use new parser and learn LID-PN mappings |
src/lid_pn_cache.rs |
Updates tests to include new learning source variant |
wacore/src/iq/dirty.rs |
Removes verbose module-level documentation |
| Other files | Removes section divider comments from tests and benchmarks |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let timestamp = node | ||
| .attrs() | ||
| .optional_u64("ts") | ||
| .ok_or_else(|| anyhow!("key-index-list missing required 'ts' attribute"))? | ||
| as i64; |
There was a problem hiding this comment.
Casting u64 to i64 can cause overflow for timestamps greater than i64::MAX (9223372036854775807, which is year 2262). While this is unlikely for WhatsApp timestamps in practice, consider using a checked conversion or explicitly handling the overflow case to make this safer. Alternatively, you could keep timestamp as u64 if the protocol doesn't require signed values.
| let timestamp = node | |
| .attrs() | |
| .optional_u64("ts") | |
| .ok_or_else(|| anyhow!("key-index-list missing required 'ts' attribute"))? | |
| as i64; | |
| let timestamp_u64 = node | |
| .attrs() | |
| .optional_u64("ts") | |
| .ok_or_else(|| anyhow!("key-index-list missing required 'ts' attribute"))?; | |
| let timestamp = i64::try_from(timestamp_u64) | |
| .map_err(|_| anyhow!("key-index-list 'ts' attribute value {} exceeds i64::MAX", timestamp_u64))?; |
| let stanza_id = optional_attr(node, "id") | ||
| .map(String::from) | ||
| .unwrap_or_default(); | ||
| let timestamp = node.attrs().optional_u64("t").unwrap_or(0) as i64; |
There was a problem hiding this comment.
Casting u64 to i64 can cause overflow for timestamps greater than i64::MAX. While unlikely for WhatsApp timestamps in practice, consider using a checked conversion or explicitly handling the overflow case. Alternatively, keep timestamp as u64 if the protocol doesn't require signed values.
| let timestamp = node.attrs().optional_u64("t").unwrap_or(0) as i64; | |
| let timestamp = match node.attrs().optional_u64("t") { | |
| Some(t) => { | |
| if t > i64::MAX as u64 { | |
| return Err(anyhow!("notification timestamp out of range: {}", t)); | |
| } | |
| t as i64 | |
| } | |
| None => 0, | |
| }; |
| .attrs() | ||
| .optional_jid("jid") | ||
| .ok_or_else(|| anyhow!("device missing required 'jid' attribute"))?; | ||
| let key_index = node.attrs().optional_u64("key-index").map(|v| v as u32); |
There was a problem hiding this comment.
Casting u64 to u32 can cause truncation for key-index values greater than u32::MAX. If the WhatsApp protocol guarantees key-index values fit in u32, consider using a checked conversion like try_into() to catch any protocol violations. Otherwise, document why the truncation is acceptable.
| let key_index = node.attrs().optional_u64("key-index").map(|v| v as u32); | |
| let key_index = match node.attrs().optional_u64("key-index") { | |
| Some(v) => Some(u32::try_from(v).map_err(|_| anyhow!("device 'key-index' out of range: {}", v))?), | |
| None => None, | |
| }; |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
…nd enforce required attributes
d5c2f16 to
317704c
Compare
Summary by CodeRabbit
New Features
Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.