refactor(handlers): split notification.rs god-file by domain - #796
Merged
Conversation
src/handlers/notification.rs was 2877 LOC: a dispatcher (handle_notification_impl) routing to ~18 independent free-fn handlers, each a distinct notification domain with no coupling between them, plus ~1200 LOC of tests. Convert it to a module directory. NotificationHandler, its StanzaHandler impl, the handle_notification_impl dispatcher, and the test module stay in mod.rs; the handlers move into four cohesive submodules: - device.rs: encrypt, account_sync (+ devices/identity/prekey/digest) - groups.rs: server_sync, group, newsletter, mex, disappearing_mode - privacy_business.rs: privacy_token, business - profile.rs: picture, status, contacts (+ notification_timestamp/learn_contact_modify_mappings, whose only callers live in this group) Pure intra-crate code-move: same codegen, no new dyn/Box/Arc/trait, no public API change. Handlers become pub(crate); mod.rs glob-imports the submodules so the dispatcher's calls are unchanged, and re-exports device::* (pub(crate)) so the external crate::handlers::notification::handle_local_identity_change path (device_registry.rs) keeps resolving. AccountSyncDevice's fields are pub(crate) so the test module (now a parent) can still read them. Verified: build, clippy --all-targets -D warnings, fmt, and the full whatsapp-rust lib test suite (737 passing, incl. all 40 notification tests).
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughThis PR implements a complete WhatsApp notification handler system that routes incoming ChangesNotification Handler System
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 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)
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 |
Collaborator
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Benchmark Results67 unchanged benchmark(s)
|
![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
🤖 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/handlers/notification/groups.rs`:
- Around line 382-385: The warn!/debug! calls that log
wacore::xml::DisplayableNodeRef(node) lack the structured log target used
elsewhere; update these macros (the warn! that logs "disappearing_mode
notification missing <disappearing_mode> child" and the related debug! calls
that print node details) to include a target parameter like target:
"Client/Group" so they match other handlers; locate the calls referencing
wacore::xml::DisplayableNodeRef(node) (and the similar occurrences around the
other reported spots) and add target: "Client/Group" to each macro invocation to
restore consistent log filtering.
In `@src/handlers/notification/profile.rs`:
- Around line 280-283: The cast using `as i64` on the `after` u64 should be
replaced with a safe conversion like `i64::try_from(after)` and propagate or
handle the conversion error rather than allowing wraparound; update the code
path where `let after = child.attrs().optional_u64("after").and_then(|after|
wacore::time::from_secs(after as i64));` to mirror the safe behavior used by
`notification_timestamp` (use `try_from`, map Err to None or an appropriate
error) before calling `wacore::time::from_secs` so timestamps >= 2^63 do not
wrap to negative values. Ensure you reference and reuse the same error
handling/utility used by `notification_timestamp` to keep behaviour consistent.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1418f9d5-bc75-4e4d-a389-a8d1adff08a1
📒 Files selected for processing (6)
src/handlers/notification.rssrc/handlers/notification/device.rssrc/handlers/notification/groups.rssrc/handlers/notification/mod.rssrc/handlers/notification/privacy_business.rssrc/handlers/notification/profile.rs
Comment on lines
+382
to
+385
| warn!( | ||
| "disappearing_mode notification missing <disappearing_mode> child: {}", | ||
| wacore::xml::DisplayableNodeRef(node) | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Inconsistent log target formatting.
The warn! and debug! macros here don't specify a target:, but every other handler in this file uses structured targets like "Client/Group", "Client/Mex", "Client/AppState". This makes log filtering and debugging harder when you're trying to track down issues at 3am.
Add consistent log targets
let Some(dm_node) = node.get_optional_child("disappearing_mode") else {
warn!(
+ target: "Client/DisappearingMode",
"disappearing_mode notification missing <disappearing_mode> child: {}",
wacore::xml::DisplayableNodeRef(node)
);
return;
}; else {
warn!(
+ target: "Client/DisappearingMode",
"disappearing_mode notification missing or invalid 't' attribute: {}",
wacore::xml::DisplayableNodeRef(node)
);
return;
};
debug!(
+ target: "Client/DisappearingMode",
"Disappearing mode changed for {}: duration={}s, t={}",
from.observe(),
duration,
setting_timestamp
);Also applies to: 403-406, 410-415
🤖 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/handlers/notification/groups.rs` around lines 382 - 385, The warn!/debug!
calls that log wacore::xml::DisplayableNodeRef(node) lack the structured log
target used elsewhere; update these macros (the warn! that logs
"disappearing_mode notification missing <disappearing_mode> child" and the
related debug! calls that print node details) to include a target parameter like
target: "Client/Group" so they match other handlers; locate the calls
referencing wacore::xml::DisplayableNodeRef(node) (and the similar occurrences
around the other reported spots) and add target: "Client/Group" to each macro
invocation to restore consistent log filtering.
Comment on lines
+280
to
+283
| let after = child | ||
| .attrs() | ||
| .optional_u64("after") | ||
| .and_then(|after| wacore::time::from_secs(after as i64)); |
There was a problem hiding this comment.
Look, we need to fix this unsafe cast—timestamps should never silently wrap to negative.
Line 283 uses as i64 which will wrap if the u64 value is >= 2^63. Your own notification_timestamp function at line 156 already does this correctly with try_from. We need consistency across the codebase, and we can't have malformed server data causing weird negative timestamps.
🔧 Fix to use safe conversion
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.
Suggested change
| let after = child | |
| .attrs() | |
| .optional_u64("after") | |
| .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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/handlers/notification/profile.rs` around lines 280 - 283, The cast using
`as i64` on the `after` u64 should be replaced with a safe conversion like
`i64::try_from(after)` and propagate or handle the conversion error rather than
allowing wraparound; update the code path where `let after =
child.attrs().optional_u64("after").and_then(|after|
wacore::time::from_secs(after as i64));` to mirror the safe behavior used by
`notification_timestamp` (use `try_from`, map Err to None or an appropriate
error) before calling `wacore::time::from_secs` so timestamps >= 2^63 do not
wrap to negative values. Ensure you reference and reuse the same error
handling/utility used by `notification_timestamp` to keep behaviour consistent.
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
1 participant
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.
What
src/handlers/notification.rswas 2877 LOC: a dispatcher (handle_notification_impl) routing to ~18 independent free-fn handlers, each a distinct notification domain with no coupling between them, plus ~1200 LOC of tests.This converts it to a module directory.
NotificationHandler, itsStanzaHandlerimpl, thehandle_notification_impldispatcher, and the#[cfg(test)]module stay inmod.rs; the handlers move into four cohesive submodules:AccountSyncDevice/parse_account_sync_device_list)notification_timestamp/learn_contact_modify_mappings, whose only callers live in this group)Why
Pure maintainability: the file was the second-largest in the repo and mixed 18 unrelated domains. Same approach as the earlier monolith split (#732).
Risk
None to behavior. Pure intra-crate code-move: same codegen, no new dyn/Box/Arc/trait, no public API change. Handlers become
pub(crate);mod.rsglob-imports the submodules so the dispatcher's calls are unchanged, and re-exportsdevice::*(pub(crate)) so the externalcrate::handlers::notification::handle_local_identity_changepath (used bydevice_registry.rs) keeps resolving.AccountSyncDevice's fields arepub(crate)so the test module (now a parent) can still read them.Verification
cargo build,cargo clippy --all-targets -- -D warnings,cargo fmt --check, and the fullwhatsapp-rustlib test suite (737 passing, including all 40 notification tests).