feat: add unified session telemetry - #238
Conversation
📝 WalkthroughWalkthroughIntroduces a unified session subsystem: new UnifiedSessionManager for server-time offset tracking, session ID calculation, deduplicated sends, IB stanza types for unified session, and wiring into Client, presence, and pairing flows with tests. Changes
Sequence DiagramsequenceDiagram
participant Client
participant UnifiedSessionManager
participant Server
Client->>Server: Login / Pairing requests
Server-->>Client: Response (node with attr t = server time)
Client->>UnifiedSessionManager: update_server_time_offset(node)
UnifiedSessionManager->>UnifiedSessionManager: compute server_time_offset_ms & reset sequence if needed
Client->>UnifiedSessionManager: prepare_send()
UnifiedSessionManager->>UnifiedSessionManager: calculate session_id, check last_sent_id, increment sequence
UnifiedSessionManager-->>Client: (Node unified-session stanza, sequence) or None
Client->>Server: Send IB UnifiedSession stanza (if produced)
Server-->>Client: ACK
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 |
984e1bd to
cd3659e
Compare
Bencher Report![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@wacore/src/ib.rs`:
- Around line 24-33: The calculate_id function can produce negative IDs because
Rust’s % preserves the sign; replace the `% WEEK_MS` operation with a
non-negative modulo using rem_euclid to ensure the result is in [0, WEEK_MS). In
the calculate_id function, compute id as (adjusted_now +
OFFSET_MS).rem_euclid(WEEK_MS) (keeping DAY_MS, WEEK_MS, OFFSET_MS and
adjusted_now as-is) and return that value as the string so session IDs are
always non-negative.
- Around line 51-60: The try_from_node implementation currently silently
defaults the session id when parsing Node.attrs; replace the fallback logic in
try_from_node with the required_attr helper so missing ids return a clear error:
call required_attr(node, "id")? to obtain the id instead of
node.attrs.get("id").cloned().unwrap_or_default(), keeping the rest of the
function unchanged so it returns Ok(Self { id }) on success.
🧹 Nitpick comments (1)
wacore/src/ib.rs (1)
119-179: Consider adding tests for error paths andIbStanza::try_from_node.The current tests cover happy paths well. Consider adding:
- A test for
IbStanza::try_from_node(parsing round-trip)- Error case tests (wrong tag, missing content)
These would improve confidence in the parsing logic.
Example additional tests
#[test] fn test_ib_stanza_try_from_node() { let stanza = IbStanza::unified_session(UnifiedSession::new("123456789")); let node = stanza.into_node(); let parsed = IbStanza::try_from_node(&node).unwrap(); match parsed.content { IbContent::UnifiedSession(us) => assert_eq!(us.id, "123456789"), } } #[test] fn test_ib_stanza_wrong_tag_error() { let node = NodeBuilder::new("wrong").build(); assert!(IbStanza::try_from_node(&node).is_err()); } #[test] fn test_ib_stanza_missing_content_error() { let node = NodeBuilder::new("ib").build(); assert!(IbStanza::try_from_node(&node).is_err()); }
| pub fn calculate_id(server_time_offset_ms: i64) -> String { | ||
| const DAY_MS: i64 = 24 * 60 * 60 * 1000; | ||
| const WEEK_MS: i64 = 7 * DAY_MS; | ||
| const OFFSET_MS: i64 = 3 * DAY_MS; | ||
|
|
||
| let now = chrono::Utc::now().timestamp_millis(); | ||
| let adjusted_now = now + server_time_offset_ms; | ||
| let id = (adjusted_now + OFFSET_MS) % WEEK_MS; | ||
| id.to_string() | ||
| } |
There was a problem hiding this comment.
Use rem_euclid() to ensure non-negative session IDs.
Rust's % operator preserves the sign of the dividend. With a sufficiently negative server_time_offset_ms, (adjusted_now + OFFSET_MS) could be negative, producing a negative session ID. Use rem_euclid() for a mathematically correct modulo that always returns a value in [0, WEEK_MS).
Proposed fix
fn calculate_id(server_time_offset_ms: i64) -> String {
const DAY_MS: i64 = 24 * 60 * 60 * 1000;
const WEEK_MS: i64 = 7 * DAY_MS;
const OFFSET_MS: i64 = 3 * DAY_MS;
let now = chrono::Utc::now().timestamp_millis();
let adjusted_now = now + server_time_offset_ms;
- let id = (adjusted_now + OFFSET_MS) % WEEK_MS;
+ let id = (adjusted_now + OFFSET_MS).rem_euclid(WEEK_MS);
id.to_string()
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fn calculate_id(server_time_offset_ms: i64) -> String { | |
| const DAY_MS: i64 = 24 * 60 * 60 * 1000; | |
| const WEEK_MS: i64 = 7 * DAY_MS; | |
| const OFFSET_MS: i64 = 3 * DAY_MS; | |
| let now = chrono::Utc::now().timestamp_millis(); | |
| let adjusted_now = now + server_time_offset_ms; | |
| let id = (adjusted_now + OFFSET_MS) % WEEK_MS; | |
| id.to_string() | |
| } | |
| pub fn calculate_id(server_time_offset_ms: i64) -> String { | |
| const DAY_MS: i64 = 24 * 60 * 60 * 1000; | |
| const WEEK_MS: i64 = 7 * DAY_MS; | |
| const OFFSET_MS: i64 = 3 * DAY_MS; | |
| let now = chrono::Utc::now().timestamp_millis(); | |
| let adjusted_now = now + server_time_offset_ms; | |
| let id = (adjusted_now + OFFSET_MS).rem_euclid(WEEK_MS); | |
| id.to_string() | |
| } |
🤖 Prompt for AI Agents
In `@wacore/src/ib.rs` around lines 24 - 33, The calculate_id function can produce
negative IDs because Rust’s % preserves the sign; replace the `% WEEK_MS`
operation with a non-negative modulo using rem_euclid to ensure the result is in
[0, WEEK_MS). In the calculate_id function, compute id as (adjusted_now +
OFFSET_MS).rem_euclid(WEEK_MS) (keeping DAY_MS, WEEK_MS, OFFSET_MS and
adjusted_now as-is) and return that value as the string so session IDs are
always non-negative.
| fn try_from_node(node: &Node) -> Result<Self> { | ||
| if node.tag != "unified_session" { | ||
| return Err(anyhow::anyhow!( | ||
| "expected <unified_session>, got <{}>", | ||
| node.tag | ||
| )); | ||
| } | ||
| let id = node.attrs.get("id").cloned().unwrap_or_default(); | ||
| Ok(Self { id }) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if required_attr and helper functions exist in wacore/src/iq/node.rs
cat wacore/src/iq/node.rs | head -100Repository: jlucaso1/whatsapp-rust
Length of output: 3768
🏁 Script executed:
#!/bin/bash
# Search for usage of required_attr, optional_attr in the codebase
echo "=== required_attr usage ==="
rg "required_attr" --type rust -B2 -A2 | head -50
echo ""
echo "=== optional_attr usage ==="
rg "optional_attr" --type rust -B2 -A2 | head -50Repository: jlucaso1/whatsapp-rust
Length of output: 4737
🏁 Script executed:
#!/bin/bash
# Check the UnifiedSession struct and its full context
cat -n wacore/src/ib.rs | head -70Repository: jlucaso1/whatsapp-rust
Length of output: 2448
Use required_attr helper for consistent parsing.
The id attribute should be parsed using the required_attr helper from wacore/src/iq/node.rs to comply with coding guidelines and provide proper error handling. Replace line 58 with:
let id = required_attr(node, "id")?;This ensures clear error messages when the id attribute is missing, rather than silently defaulting to an empty string.
🤖 Prompt for AI Agents
In `@wacore/src/ib.rs` around lines 51 - 60, The try_from_node implementation
currently silently defaults the session id when parsing Node.attrs; replace the
fallback logic in try_from_node with the required_attr helper so missing ids
return a clear error: call required_attr(node, "id")? to obtain the id instead
of node.attrs.get("id").cloned().unwrap_or_default(), keeping the rest of the
function unchanged so it returns Ok(Self { id }) on success.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
How can I reopen WhatsApp view once |
|
@aameliaaa74-sys this is related to this change? I think opening a issue is better |
3 participants
Summary
<ib><unified_session id="..."/>stanza support matching WhatsApp Web behaviorwacore/src/ib.rssrc/unified_session.rsTest plan
Summary by CodeRabbit
New Features
Behavioral Changes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.