fix: centralize timestamp handling via wacore::time and fix signed parsing - #532
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR introduces four new timestamp conversion helpers in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/handlers/notification.rs (1)
1709-1711:⚠️ Potential issue | 🟡 MinorTest helper uses
u64parsing but production code now usesi64.The test helper
parse_disappearing_modeparses the timestamp asu64(line 1710-1711), but the production code at line 1399 now parses asi64. This inconsistency means the test helper won't catch edge cases involving negative timestamps that the production code would handle.🔧 Suggested fix to align test helper with production code
fn parse_disappearing_mode(node: &Node) -> Option<(u32, u64)> { let dm_node = node.get_optional_child("disappearing_mode")?; let mut dm_attrs = dm_node.attrs(); let duration = dm_attrs .optional_string("duration") .and_then(|s| s.parse::<u32>().ok()) .unwrap_or(0); let setting_timestamp = dm_attrs .optional_string("t") - .and_then(|s| s.parse::<u64>().ok())?; - Some((duration, setting_timestamp)) + .and_then(|s| s.parse::<i64>().ok()) + .and_then(|t| chrono::DateTime::from_timestamp(t, 0))?; + Some((duration, setting_timestamp.timestamp() as u64)) }Alternatively, update the return type to
Option<(u32, DateTime<Utc>)>to fully match production behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/handlers/notification.rs` around lines 1709 - 1711, The test helper parse_disappearing_mode currently parses the "t" attribute as u64 via dm_attrs.optional_string("t").and_then(|s| s.parse::<u64>().ok()), but production code parses it as i64 at the corresponding logic; update the helper to parse the timestamp as i64 (s.parse::<i64>().ok()) and propagate the signed value into the same downstream handling used in production, or alternatively adjust the helper's return type to Option<(u32, DateTime<Utc>)> to fully mirror production behavior; ensure you change any subsequent uses of the parsed value in parse_disappearing_mode to accept i64 semantics.
🤖 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/handlers/notification.rs`:
- Line 852: The code is falling back to Unix epoch via unwrap_or_default() when
building the timestamp (DateTime::from_timestamp(notification.timestamp,
0).unwrap_or_default()), which is inconsistent with the notification_timestamp
helper that uses Utc::now(); change the fallback to use chrono::Utc::now()
instead of the default epoch so invalid notification.timestamp values yield the
current time—update the construction here and ensure it matches the
notification_timestamp helper behavior.
---
Outside diff comments:
In `@src/handlers/notification.rs`:
- Around line 1709-1711: The test helper parse_disappearing_mode currently
parses the "t" attribute as u64 via dm_attrs.optional_string("t").and_then(|s|
s.parse::<u64>().ok()), but production code parses it as i64 at the
corresponding logic; update the helper to parse the timestamp as i64
(s.parse::<i64>().ok()) and propagate the signed value into the same downstream
handling used in production, or alternatively adjust the helper's return type to
Option<(u32, DateTime<Utc>)> to fully mirror production behavior; ensure you
change any subsequent uses of the parsed value in parse_disappearing_mode to
accept i64 semantics.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d5f36821-0cc7-4696-ad36-0167f6b6cca1
📒 Files selected for processing (2)
src/handlers/notification.rswacore/src/types/events.rs
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wacore/src/types/events.rs`:
- Around line 450-466: The chat_jid() helper incorrectly returns None for the
Event::JoinedGroup variant even though JoinedGroup has an associated group JID;
update the match arm for JoinedGroup in the chat_jid method to return the joined
group's JID (e.g., Some(&joined.group_jid) or the correct field name on the
JoinedGroup struct) instead of None so that routing/filtering sees this
chat-scoped event; ensure you reference the JoinedGroup payload field that holds
the JID consistent with other variants like Event::GroupUpdate(g) ->
g.group_jid.
- Around line 318-322: BusinessStatusUpdate and many other public event structs
switched their timestamp fields to DateTime<Utc> which will serialize to RFC3339
strings; to preserve existing Unix-second integer semantics, add #[serde(with =
"chrono::serde::ts_seconds")] to each timestamp field (e.g., the timestamp field
on BusinessStatusUpdate and likewise on Receipt, PictureUpdated,
UserAboutUpdate, ContactUpdated, PhoneNumberChanged, ContactSyncRequested,
GroupUpdate, ContactUpdate, PinUpdate, MuteUpdate, ArchiveUpdate, StarUpdate,
MarkChatAsReadUpdate, DeleteChatUpdate, DeleteMessageForMeUpdate) so serde
serializes/deserializes as integer seconds, or alternatively document this as a
breaking API change if you intend RFC3339 output.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2a129ada-e8da-40c0-ac27-7e33c577f8b3
📒 Files selected for processing (1)
wacore/src/types/events.rs
|
@claude review |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/handlers/notification.rs (1)
1703-1713:⚠️ Potential issue | 🟡 MinorTest helper does not apply handler's timestamp validation.
The handler validates
tvia bothparse::<i64>()ANDchrono::DateTime::from_timestamp(), rejecting out-of-range values. The test helper only parsesi64, allowing invalid timestamps to pass tests.🔧 Align test helper with handler validation
- fn parse_disappearing_mode(node: &Node) -> Option<(u32, i64)> { + fn parse_disappearing_mode( + node: &Node, + ) -> Option<(u32, chrono::DateTime<chrono::Utc>)> { let dm_node = node.get_optional_child("disappearing_mode")?; let mut dm_attrs = dm_node.attrs(); let duration = dm_attrs .optional_string("duration") .and_then(|s| s.parse::<u32>().ok()) .unwrap_or(0); let setting_timestamp = dm_attrs .optional_string("t") - .and_then(|s| s.parse::<i64>().ok())?; + .and_then(|s| s.parse::<i64>().ok()) + .and_then(|t| chrono::DateTime::from_timestamp(t, 0))?; Some((duration, setting_timestamp)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/handlers/notification.rs` around lines 1703 - 1713, The test helper currently parses the disappearing-mode timestamp only via parse::<i64>() which allows out-of-range timestamps that the real handler (parse_disappearing_mode) rejects by also validating with chrono timestamp conversion; update the test helper's handling of dm_attrs.optional_string("t") to, after parsing to i64, also attempt the same chrono timestamp conversion used in the handler (e.g., DateTime/NaiveDateTime from_timestamp/_opt) and treat values that fail that conversion as invalid so tests mirror parse_disappearing_mode's validation logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/handlers/notification.rs`:
- Around line 1703-1713: The test helper currently parses the disappearing-mode
timestamp only via parse::<i64>() which allows out-of-range timestamps that the
real handler (parse_disappearing_mode) rejects by also validating with chrono
timestamp conversion; update the test helper's handling of
dm_attrs.optional_string("t") to, after parsing to i64, also attempt the same
chrono timestamp conversion used in the handler (e.g., DateTime/NaiveDateTime
from_timestamp/_opt) and treat values that fail that conversion as invalid so
tests mirror parse_disappearing_mode's validation logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4d736860-b4c6-45b0-ba19-689d22faa23c
📒 Files selected for processing (1)
src/handlers/notification.rs
|
@claude review |
|
@Salientekill ele so escuta eu |
Não gostei 🧐 |
- BusinessStatusUpdate.timestamp: i64 → DateTime<Utc> - DisappearingModeChanged.setting_timestamp: u64 → DateTime<Utc> - PushNameUpdate.message: Box<MessageInfo> → Arc<MessageInfo> - PushNameUpdate: add from_full_sync: bool (matches other app-state sync events)
- Document the primary JID field in every event struct (PresenceUpdate, UserAboutUpdate, ContactUpdated, ContactUpdate, PushNameUpdate, PinUpdate, MuteUpdate, ArchiveUpdate, StarUpdate, MarkChatAsReadUpdate, DeleteChatUpdate, DeleteMessageForMeUpdate, BusinessStatusUpdate, NewsletterLiveUpdate) so the naming differences are self-explanatory - Add Event::chat_jid() -> Option<&Jid> to extract the primary JID from any event without exhaustive matching — useful for routing/filtering
…up chat_jid exception
Replace all non-test chrono::Utc::now calls with wacore::time::now_utc to support WASM environments where std::time::SystemTime is unavailable.
7e1df55 to
7933363
Compare
|
@claude review |
|
Claude Code is working… I'll analyze this and get back to you. |
jlucaso1
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/handlers/notification.rs (1)
1703-1713:⚠️ Potential issue | 🟡 MinorMake the test helper validate the same timestamp range as the handler.
handle_disappearing_mode_notification()now rejectstvalues that failDateTime::from_timestamp(...), but this helper only parses ani64. That lets out-of-range fixtures pass in tests even though production would drop them.🧪 Proposed fix
-fn parse_disappearing_mode(node: &Node) -> Option<(u32, i64)> { +fn parse_disappearing_mode(node: &Node) -> Option<(u32, chrono::DateTime<chrono::Utc>)> { let dm_node = node.get_optional_child("disappearing_mode")?; let mut dm_attrs = dm_node.attrs(); let duration = dm_attrs .optional_string("duration") .and_then(|s| s.parse::<u32>().ok()) .unwrap_or(0); let setting_timestamp = dm_attrs .optional_string("t") - .and_then(|s| s.parse::<i64>().ok())?; + .and_then(|s| s.parse::<i64>().ok()) + .and_then(|t| chrono::DateTime::from_timestamp(t, 0))?; Some((duration, setting_timestamp)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/handlers/notification.rs` around lines 1703 - 1713, The test helper parse_disappearing_mode currently accepts any i64 for the "t" field but the production handler handle_disappearing_mode_notification rejects t values that cannot be converted to a DateTime (via DateTime::from_timestamp or equivalent), so update parse_disappearing_mode to perform the same range validation: after parsing dm_attrs.optional_string("t") to i64, attempt the same DateTime timestamp conversion used in handle_disappearing_mode_notification and return None (i.e., treat as invalid) if the conversion fails or is out of range; keep the existing returns for duration and setting_timestamp but only return Some when the timestamp is valid. Ensure you reference parse_disappearing_mode, dm_node.attrs(), dm_attrs.optional_string("t"), and the DateTime::from_timestamp conversion used in the handler so the helper mirrors the handler's behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wacore/src/types/events.rs`:
- Around line 452-505: The helper chat_jid() currently returns identifiers for
non-chat events (e.g., IdentityChange, DeviceListUpdate, NewsletterLiveUpdate),
so rename the method to primary_jid() and update its docstring to reflect that
it returns the primary JID for any event (not just chat-scoped ones); update all
call sites to use primary_jid(), and add a short #[deprecated] chat_jid()
wrapper that delegates to primary_jid() to preserve backward compatibility while
signaling the rename. Ensure the match arms in primary_jid() still include the
listed variants (Event::IdentityChange, Event::DeviceListUpdate,
Event::NewsletterLiveUpdate, etc.) and update any external docs/tests
accordingly.
---
Outside diff comments:
In `@src/handlers/notification.rs`:
- Around line 1703-1713: The test helper parse_disappearing_mode currently
accepts any i64 for the "t" field but the production handler
handle_disappearing_mode_notification rejects t values that cannot be converted
to a DateTime (via DateTime::from_timestamp or equivalent), so update
parse_disappearing_mode to perform the same range validation: after parsing
dm_attrs.optional_string("t") to i64, attempt the same DateTime timestamp
conversion used in handle_disappearing_mode_notification and return None (i.e.,
treat as invalid) if the conversion fails or is out of range; keep the existing
returns for duration and setting_timestamp but only return Some when the
timestamp is valid. Ensure you reference parse_disappearing_mode,
dm_node.attrs(), dm_attrs.optional_string("t"), and the DateTime::from_timestamp
conversion used in the handler so the helper mirrors the handler's behavior.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 76ff9f83-bc81-41f7-bb3e-e998bc6835f8
📒 Files selected for processing (4)
src/handlers/notification.rssrc/pdo.rswacore/src/messages.rswacore/src/types/events.rs
- Add from_secs, from_secs_or_now, from_millis, from_millis_or_now helpers to wacore::time so all chrono::DateTime conversion goes through the pluggable time module - Replace all direct chrono::DateTime::from_timestamp calls across the codebase with the new helpers - Change JoinedGroup from tuple variant to struct variant with an explicit group_jid field so chat_jid() can return it without requiring proto decoding at the call site
- Rename Event::chat_jid() to Event::primary_jid() since it returns JIDs for non-chat events too (IdentityChange, DeviceListUpdate, etc.) - Update parse_disappearing_mode test helper to validate timestamp range via wacore::time::from_secs, mirroring the production handler
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/handlers/notification.rs`:
- Around line 1171-1174: The current conversion uses `u64 as i64` which wraps on
overflow; change the `after` handling (starting from
`child.attrs().optional_u64("after")`) to safely reject values > i64::MAX by
using a fallible conversion (e.g., `try_into()`/`checked_*` or an explicit `if
after <= i64::MAX as u64`) and only call `wacore::time::from_secs` with the
safely converted `i64`; return `None` for oversized `after` values so malformed
timestamps are rejected instead of wrapping.
In `@src/history_sync.rs`:
- Around line 223-229: The code dispatches Event::JoinedGroup based only on
conv.id.parse() and eagerly calls lazy_conv.get(), which can emit non-group
chats and defeats the lazy decode; change to parse the ID from the
partially-decoded conv (reuse the existing partial decode used to access
conv.id) into group_jid, check group_jid.is_group() and only then call
lazy_conv.get() to fully decode the conversation and dispatch Event::JoinedGroup
({ group_jid, conversation: lazy_conv }), ensuring non-group JIDs are skipped
and lazy decoding is preserved; use the same symbols conv.id.parse(),
group_jid.is_group(), lazy_conv.get(), and Event::JoinedGroup to locate where to
apply this gating.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ab2e8e4d-21b9-4054-bb2a-cb8a94e11692
📒 Files selected for processing (10)
src/features/chat_actions.rssrc/handlers/notification.rssrc/handlers/presence.rssrc/history_sync.rssrc/pdo.rssrc/version.rswacore/src/messages.rswacore/src/stanza/notification.rswacore/src/time.rswacore/src/types/events.rs
| let after = child | ||
| .attrs() | ||
| .optional_u64("after") | ||
| .and_then(|after| chrono::DateTime::from_timestamp(after as i64, 0)); | ||
| .and_then(|after| wacore::time::from_secs(after as i64)); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Read the file around lines 1171-1174 to see the actual code context
head -n 1180 src/handlers/notification.rs | tail -n 20Repository: jlucaso1/whatsapp-rust
Length of output: 689
🏁 Script executed:
# Search for the wacore::time module to understand from_secs
rg "from_secs" -A 3 --type rsRepository: jlucaso1/whatsapp-rust
Length of output: 92
🏁 Script executed:
# Check if there's a wacore crate in the project and inspect time module
fd -e rs -x grep -l "wacore" | head -5Repository: jlucaso1/whatsapp-rust
Length of output: 257
🏁 Script executed:
# Look for the definition of from_secs to understand what it accepts
rg "fn from_secs" -A 5 --type rsRepository: jlucaso1/whatsapp-rust
Length of output: 92
🏁 Script executed:
# Search for from_secs definition without type flag
grep -r "fn from_secs" --include="*.rs"Repository: jlucaso1/whatsapp-rust
Length of output: 240
🏁 Script executed:
# Check the wacore module structure
find wacore -name "*.rs" -type f | head -20Repository: jlucaso1/whatsapp-rust
Length of output: 681
🏁 Script executed:
# Look at the time module in wacore
grep -r "from_secs" wacore --include="*.rs" -B 2 -A 10Repository: jlucaso1/whatsapp-rust
Length of output: 7301
🏁 Script executed:
# Check if there are any tests or usages that show from_secs behavior
grep -r "from_secs" --include="*.rs" -B 2 -A 5 | head -60Repository: jlucaso1/whatsapp-rust
Length of output: 3823
Avoid u64 as i64 for after to reject oversized timestamps.
Rust wraps on overflow, so a malformed after above i64::MAX becomes a negative timestamp (before 1970) instead of None, likely unintended for a sync request attribute.
💡 Suggested fix
let after = child
.attrs()
.optional_u64("after")
- .and_then(|after| wacore::time::from_secs(after as i64));
+ .and_then(|after| i64::try_from(after).ok())
+ .and_then(wacore::time::from_secs);📝 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.
| let after = child | |
| .attrs() | |
| .optional_u64("after") | |
| .and_then(|after| chrono::DateTime::from_timestamp(after as i64, 0)); | |
| .and_then(|after| wacore::time::from_secs(after as i64)); | |
| let after = child | |
| .attrs() | |
| .optional_u64("after") | |
| .and_then(|after| i64::try_from(after).ok()) | |
| .and_then(wacore::time::from_secs); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/handlers/notification.rs` around lines 1171 - 1174, The current
conversion uses `u64 as i64` which wraps on overflow; change the `after`
handling (starting from `child.attrs().optional_u64("after")`) to safely reject
values > i64::MAX by using a fallible conversion (e.g., `try_into()`/`checked_*`
or an explicit `if after <= i64::MAX as u64`) and only call
`wacore::time::from_secs` with the safely converted `i64`; return `None` for
oversized `after` values so malformed timestamps are rejected instead of
wrapping.
| if let Some(conv) = lazy_conv.get() | ||
| && let Ok(group_jid) = conv.id.parse() | ||
| { | ||
| self.core.event_bus.dispatch(Event::JoinedGroup { | ||
| group_jid, | ||
| conversation: lazy_conv, | ||
| }); |
There was a problem hiding this comment.
JoinedGroup still accepts non-group chats and forces eager decode.
conv.id.parse() only proves the ID is syntactically valid, so 1:1/newsletter conversations will still be emitted as Event::JoinedGroup. That also makes the JID extractor in wacore/src/types/events.rs:469-476 return a non-group primary JID for this variant. On top of that, lazy_conv.get() fully decodes every conversation here, which defeats the lazy path documented in this file. Reuse a partial decode for id and gate on group_jid.is_group() before dispatching.
💡 Suggested fix
- let lazy_conv = LazyConversation::from_bytes(raw_bytes);
- if let Some(conv) = lazy_conv.get()
- && let Ok(group_jid) = conv.id.parse()
- {
+ if let Ok(conv) =
+ <ConversationTcTokenFields as prost::Message>::decode(raw_bytes.clone())
+ && let Ok(group_jid) = conv.id.parse::<wacore_binary::Jid>()
+ && group_jid.is_group()
+ {
+ let lazy_conv = LazyConversation::from_bytes(raw_bytes);
self.core.event_bus.dispatch(Event::JoinedGroup {
group_jid,
conversation: lazy_conv,
});
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/history_sync.rs` around lines 223 - 229, The code dispatches
Event::JoinedGroup based only on conv.id.parse() and eagerly calls
lazy_conv.get(), which can emit non-group chats and defeats the lazy decode;
change to parse the ID from the partially-decoded conv (reuse the existing
partial decode used to access conv.id) into group_jid, check
group_jid.is_group() and only then call lazy_conv.get() to fully decode the
conversation and dispatch Event::JoinedGroup ({ group_jid, conversation:
lazy_conv }), ensuring non-group JIDs are skipped and lazy decoding is
preserved; use the same symbols conv.id.parse(), group_jid.is_group(),
lazy_conv.get(), and Event::JoinedGroup to locate where to apply this gating.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/history_sync.rs (1)
222-229:⚠️ Potential issue | 🟠 Major
JoinedGroupis still emitted for non-group conversations, and the lazy path is still eager.
conv.id.parse()only proves the ID is syntactically a JID, so 1:1/newsletter conversations can still be emitted asEvent::JoinedGroup. Callinglazy_conv.get()here also forces a decode before dispatch, which defeats theLazyConversationcontract. Reuse the partial decode already modeled byConversationTcTokenFields, requiregroup_jid.is_group(), and then dispatch the untouchedLazyConversation.Suggested change
- let lazy_conv = LazyConversation::from_bytes(raw_bytes); - if let Some(conv) = lazy_conv.get() - && let Ok(group_jid) = conv.id.parse() - { - self.core.event_bus.dispatch(Event::JoinedGroup { - group_jid, - conversation: lazy_conv, - }); - } + if let Ok(conv) = + <ConversationTcTokenFields as prost::Message>::decode(raw_bytes.clone()) + && let Ok(group_jid) = conv.id.parse::<wacore_binary::Jid>() + && group_jid.is_group() + { + self.core.event_bus.dispatch(Event::JoinedGroup { + group_jid, + conversation: LazyConversation::from_bytes(raw_bytes), + }); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/history_sync.rs` around lines 222 - 229, The code is eagerly decoding the lazy conversation and emitting JoinedGroup for non-group JIDs; replace the eager decode by parsing the minimal token fields (use ConversationTcTokenFields or equivalent to extract conv.id), call conv.id.parse() and then check group_jid.is_group() before dispatching, remove the lazy_conv.get() call so the original LazyConversation from LazyConversation::from_bytes is passed to self.core.event_bus.dispatch(Event::JoinedGroup { group_jid, conversation: lazy_conv }), and ensure you only construct group_jid after confirming is_group() to avoid emitting for 1:1/newsletter conversations.src/handlers/notification.rs (1)
1171-1174:⚠️ Potential issue | 🟡 MinorReject oversized
aftervalues before converting to seconds.Line 1174 still uses
after as i64. Values abovei64::MAXwill wrap into negative seconds, so a malformedaftercan become a bogus pre-1970DateTimeinstead of being rejected.Suggested fix
let after = child .attrs() .optional_u64("after") - .and_then(|after| wacore::time::from_secs(after as i64)); + .and_then(|after| i64::try_from(after).ok()) + .and_then(wacore::time::from_secs);#!/bin/bash set -euo pipefail echo "Inspect the current conversion in src/handlers/notification.rs:" sed -n '1171,1176p' src/handlers/notification.rs echo echo "Inspect wacore::time::from_secs signature:" sed -n '56,67p' wacore/src/time.rsExpected result: the first snippet shows the current
after as i64cast, and the second confirmsfrom_secstakesi64, so a fallible conversion should happen before calling it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/handlers/notification.rs` around lines 1171 - 1174, The current conversion uses a direct cast "after as i64" which can wrap large u64 values; change the closure on child.attrs().optional_u64("after") to perform a fallible conversion (e.g. i64::try_from or u64::try_into<i64>) and only call wacore::time::from_secs on the successfully converted i64; reference the optional_u64("after") closure and wacore::time::from_secs so the code rejects values > i64::MAX instead of allowing wrapping.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wacore/src/stanza/notification.rs`:
- Around line 21-24: Replace the unsigned-to-signed unchecked cast that causes
wraparound: instead of calling node.attrs().optional_u64("t").and_then(|t|
crate::time::from_secs(t as i64)).unwrap_or_else(crate::time::now_utc), parse or
convert the attribute into a signed i64 safely (e.g., use an optional_i64
accessor or try_from/checked conversion on the u64) so out-of-range u64 values
fail the conversion and trigger the fallback to crate::time::now_utc; locate the
code around node.attrs(), optional_u64("t"), crate::time::from_secs and
crate::time::now_utc and change the conversion to a checked/validated i64 path.
In `@wacore/src/types/events.rs`:
- Around line 455-506: The primary_jid() helper currently returns None for
PairSuccess and PairError even though those variants carry account JIDs; update
the match in primary_jid() to return those IDs (e.g., match
Event::PairSuccess(p) => Some(&p.id) and Event::PairError(e) => Some(&e.lid)) so
JID-based routing includes these variants, or alternatively update the doc
comment to explicitly exclude pairing-result events if you prefer not to expose
those fields.
---
Duplicate comments:
In `@src/handlers/notification.rs`:
- Around line 1171-1174: The current conversion uses a direct cast "after as
i64" which can wrap large u64 values; change the closure on
child.attrs().optional_u64("after") to perform a fallible conversion (e.g.
i64::try_from or u64::try_into<i64>) and only call wacore::time::from_secs on
the successfully converted i64; reference the optional_u64("after") closure and
wacore::time::from_secs so the code rejects values > i64::MAX instead of
allowing wrapping.
In `@src/history_sync.rs`:
- Around line 222-229: The code is eagerly decoding the lazy conversation and
emitting JoinedGroup for non-group JIDs; replace the eager decode by parsing the
minimal token fields (use ConversationTcTokenFields or equivalent to extract
conv.id), call conv.id.parse() and then check group_jid.is_group() before
dispatching, remove the lazy_conv.get() call so the original LazyConversation
from LazyConversation::from_bytes is passed to
self.core.event_bus.dispatch(Event::JoinedGroup { group_jid, conversation:
lazy_conv }), and ensure you only construct group_jid after confirming
is_group() to avoid emitting for 1:1/newsletter conversations.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 363e980d-21d0-4080-a215-6d38f3d729f8
📒 Files selected for processing (10)
src/features/chat_actions.rssrc/handlers/notification.rssrc/handlers/presence.rssrc/history_sync.rssrc/pdo.rssrc/version.rswacore/src/messages.rswacore/src/stanza/notification.rswacore/src/time.rswacore/src/types/events.rs
| node.attrs() | ||
| .optional_u64("t") | ||
| .and_then(|t| chrono::DateTime::from_timestamp(t as i64, 0)) | ||
| .and_then(|t| crate::time::from_secs(t as i64)) | ||
| .unwrap_or_else(crate::time::now_utc) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Rust, what happens when you convert u64toi64withasand the value is greater thani64::MAX? Does it wrap, saturate, or error?
💡 Result:
When converting a u64 to i64 using the as cast where the u64 value exceeds i64::MAX (9223372036854775807), it performs a reinterpretation of the bit pattern. Since both types are 64 bits and Rust uses two's complement for signed integers, casting u64 to i64 is a no-op on the bits—it reinterprets the unsigned bits as a signed value. For values <= i64::MAX, the numerical value is preserved. For values > i64::MAX, the result is a negative i64. Specifically, u64::MAX (all bits 1) becomes -1i64, as confirmed by the official documentation: u64::cast_signed example shows u64::MAX.cast_signed == -1i64, and this produces the same result as as. This is neither saturation (clamping to i64::MAX), nor does it error or panic. It "wraps" in the sense of bit reinterpretation, producing a negative number equivalent to the two's complement interpretation. Official Rust Reference confirms: "Casting between two integers of the same size (e.g. i32 -> u32) is a no-op (Rust uses 2's complement for negative numbers)." Rust By Example notes: "When casting to a signed type, the (bitwise) result is the same as first casting to the corresponding unsigned type." It does not panic on overflow like arithmetic operations in debug mode; casts are always defined and do not trigger overflow checks.
Citations:
- 1: https://doc.rust-lang.org/reference/expressions/operator-expr.html?highlight=casting
- 2: https://doc.rust-lang.org/stable/rust-by-example/types/cast.html
- 3: https://doc.rust-lang.org/std/primitive.u64.html
- 4: https://doc.rust-lang.org/rust-by-example/types/cast.html
- 5: https://stackoverflow.com/questions/78530936/why-doesnt-i32-as-usize-panic-on-overflow
🏁 Script executed:
cat -n wacore/src/stanza/notification.rs | head -50Repository: jlucaso1/whatsapp-rust
Length of output: 2503
🏁 Script executed:
rg "parse_disappearing_mode" wacore/src/stanza/notification.rs -A 10Repository: jlucaso1/whatsapp-rust
Length of output: 2290
🏁 Script executed:
rg "fn from_secs" wacore/src/time.rs -A 5Repository: jlucaso1/whatsapp-rust
Length of output: 521
🏁 Script executed:
rg "notification_timestamp" wacore/src -B 2 -A 5Repository: jlucaso1/whatsapp-rust
Length of output: 1924
🏁 Script executed:
rg "from_timestamp" wacore/src -B 2 -A 2Repository: jlucaso1/whatsapp-rust
Length of output: 813
🏁 Script executed:
# Check if there are any tests for notification_timestamp that show the wrapping issue
rg "test.*notification_timestamp" wacore/src/stanza/notification.rs -A 15Repository: jlucaso1/whatsapp-rust
Length of output: 48
Use signed parsing or checked conversion for the timestamp.
The current code uses optional_u64("t") followed by as i64. When the attribute contains a value larger than i64::MAX (e.g., 18446744073709551615), the cast reinterprets the bit pattern as a negative i64 (e.g., -1). Since chrono::DateTime::from_timestamp() accepts negative timestamps (valid pre-1970 dates), the conversion succeeds instead of returning None, and the invalid input produces a 1969 timestamp rather than falling back to now_utc().
Suggested change
pub fn notification_timestamp(node: &Node) -> chrono::DateTime<chrono::Utc> {
node.attrs()
- .optional_u64("t")
- .and_then(|t| crate::time::from_secs(t as i64))
+ .optional_string("t")
+ .and_then(|t| t.parse::<i64>().ok())
+ .and_then(crate::time::from_secs)
.unwrap_or_else(crate::time::now_utc)
}Parsing as i64 will reject out-of-range values, triggering the fallback as intended. No test currently covers this edge case.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wacore/src/stanza/notification.rs` around lines 21 - 24, Replace the
unsigned-to-signed unchecked cast that causes wraparound: instead of calling
node.attrs().optional_u64("t").and_then(|t| crate::time::from_secs(t as
i64)).unwrap_or_else(crate::time::now_utc), parse or convert the attribute into
a signed i64 safely (e.g., use an optional_i64 accessor or try_from/checked
conversion on the u64) so out-of-range u64 values fail the conversion and
trigger the fallback to crate::time::now_utc; locate the code around
node.attrs(), optional_u64("t"), crate::time::from_secs and crate::time::now_utc
and change the conversion to a checked/validated i64 path.
| /// Returns the primary JID associated with this event, if any. | ||
| /// | ||
| /// Useful for routing or filtering events without exhaustively matching every variant. | ||
| /// Returns `None` for connection-lifecycle and sync events that have no associated JID. | ||
| pub fn primary_jid(&self) -> Option<&Jid> { | ||
| match self { | ||
| Event::Message(_, info) => Some(&info.source.chat), | ||
| Event::Receipt(r) => Some(&r.source.chat), | ||
| Event::UndecryptableMessage(u) => Some(&u.info.source.chat), | ||
| Event::ChatPresence(c) => Some(&c.source.chat), | ||
| Event::Presence(p) => Some(&p.from), | ||
| Event::PictureUpdate(p) => Some(&p.jid), | ||
| Event::UserAboutUpdate(u) => Some(&u.jid), | ||
| Event::ContactUpdated(c) => Some(&c.jid), | ||
| Event::ContactNumberChanged(c) => Some(&c.new_jid), | ||
| Event::GroupUpdate(g) => Some(&g.group_jid), | ||
| Event::JoinedGroup { group_jid, .. } => Some(group_jid), | ||
| Event::ContactUpdate(c) => Some(&c.jid), | ||
| Event::PushNameUpdate(p) => Some(&p.jid), | ||
| Event::PinUpdate(p) => Some(&p.jid), | ||
| Event::MuteUpdate(m) => Some(&m.jid), | ||
| Event::ArchiveUpdate(a) => Some(&a.jid), | ||
| Event::StarUpdate(s) => Some(&s.chat_jid), | ||
| Event::MarkChatAsReadUpdate(m) => Some(&m.jid), | ||
| Event::DeleteChatUpdate(d) => Some(&d.jid), | ||
| Event::DeleteMessageForMeUpdate(d) => Some(&d.chat_jid), | ||
| Event::BusinessStatusUpdate(b) => Some(&b.jid), | ||
| Event::DisappearingModeChanged(d) => Some(&d.from), | ||
| Event::NewsletterLiveUpdate(n) => Some(&n.newsletter_jid), | ||
| Event::DeviceListUpdate(d) => Some(&d.user), | ||
| Event::IdentityChange(i) => Some(&i.user), | ||
| Event::Connected(_) | ||
| | Event::Disconnected(_) | ||
| | Event::PairSuccess(_) | ||
| | Event::PairError(_) | ||
| | Event::LoggedOut(_) | ||
| | Event::PairingQrCode { .. } | ||
| | Event::PairingCode { .. } | ||
| | Event::QrScannedWithoutMultidevice(_) | ||
| | Event::ClientOutdated(_) | ||
| | Event::SelfPushNameUpdated(_) | ||
| | Event::HistorySync(_) | ||
| | Event::OfflineSyncPreview(_) | ||
| | Event::OfflineSyncCompleted(_) | ||
| | Event::StreamReplaced(_) | ||
| | Event::TemporaryBan(_) | ||
| | Event::ConnectFailure(_) | ||
| | Event::StreamError(_) | ||
| | Event::ContactSyncRequested(_) | ||
| | Event::Notification(_) | ||
| | Event::RawNode(_) => None, | ||
| } |
There was a problem hiding this comment.
primary_jid() skips pairing-result events that already carry account JIDs.
PairSuccess and PairError both contain id/lid, but they currently fall through to None. That makes generic JID-based routing miss two public variants even though this helper is documented as the primary JID extractor. Either surface one of those identifiers here or narrow the doc comment so the contract matches the behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wacore/src/types/events.rs` around lines 455 - 506, The primary_jid() helper
currently returns None for PairSuccess and PairError even though those variants
carry account JIDs; update the match in primary_jid() to return those IDs (e.g.,
match Event::PairSuccess(p) => Some(&p.id) and Event::PairError(e) =>
Some(&e.lid)) so JID-based routing includes these variants, or alternatively
update the doc comment to explicitly exclude pairing-result events if you prefer
not to expose those fields.
…te changes - Restore JoinedGroup as tuple variant to preserve lazy-parsing design and serde compatibility - Remove primary_jid() — premature abstraction with zero callers - Restore PushNameUpdate to Box<MessageInfo> without from_full_sync since the struct is never constructed
jlucaso1
changed the title
2 participants
Summary
Centralized time helpers (
wacore::time)from_secs,from_secs_or_now,from_millis,from_millis_or_nowhelperschrono::DateTime::from_timestamp*andchrono::Utc::nowcalls withwacore::timehelpers — ensures WASM compatibility via the pluggableTimeProvidernotification.rs,presence.rs,pdo.rs,version.rs,chat_actions.rs,wacore/messages.rs,wacore/stanza/notification.rsType fixes
BusinessStatusUpdate.timestamp:i64→DateTime<Utc>with#[serde(with = "chrono::serde::ts_seconds")]— consistent with every other timestamped event, preserves integer serializationDisappearingModeChanged.setting_timestamp:u64→DateTime<Utc>withts_seconds— fixes sign (parsed asi64to match WA Web'sattrTime/castToUnixTimesemantics)Test alignment
parse_disappearing_modetest helper to validate timestamp range viawacore::time::from_secs, mirroring the production handlerDocumentation
Breaking changes
BusinessStatusUpdate.timestamp:i64→DateTime<Utc>(serde output preserved as integer viats_seconds)DisappearingModeChanged.setting_timestamp:u64→DateTime<Utc>(serde output preserved as integer viats_seconds)cargo clippy --all --testsandcargo fmt --allpass clean.Summary by CodeRabbit
Release Notes