chore: audit follow-ups (perf, cleanup, helpers) - #584
Merged
Conversation
Outcome of a codebase audit pass. All validated with cargo fmt, clippy --workspace --all-targets, and the full test suite (1249 tests passing, excluding e2e). Perf: avoid String allocation for numeric and bool attrs Add From<integer> and From<bool> impls for NodeValue, routing integers through itoa + CompactString so they inline on the stack instead of heap-allocating via .to_string(). Per-attr cost drops ~50%, a loop of 100 attrs drops ~58%, RAM hits drop ~42% (iai-callgrind, see the new numeric_attr_benchmark). Refactored ~27 call sites across src/ and wacore/ to pass numerics and bools directly to .attr(...). Updated the ProtocolNode derive macro to emit the new pattern for U32/U64 fields. Dead code - Remove UnimplementedHandler (struct, its file, and handle_unimplemented in client). Nothing ever instantiated it. - Remove unused runtime field from NoiseSocket (was tagged #[allow(dead_code)] with a "kept for potential future spawns" note). Refactor and docs - Move extract_content_bytes and extract_content_uint from wacore/src/iq/prekeys.rs into wacore/src/iq/node.rs, where the other node helpers already live. - Add a require_from_jid! macro under src/handlers/macros.rs and apply it to four uniform notification handlers that shared the same match-and-warn pattern. - Rewrite the complete_offline_sync ordering comment so it matches the actual code: readers observing the flag short-circuit without touching the semaphore, old workers drain on their old Arc, new workers pick up the 64-permit semaphore.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR centralizes numeric/boolean XML attribute handling by adding From impls for primitive types into NodeValue and updating builders to pass raw numeric/bool values to NodeBuilder::attr(). It also adds a require_from_jid! macro, removes the UnimplementedHandler and Client::handle_unimplemented, and adds related benchmarks and helpers. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
—This needs to work correctly; verify serialized attribute outputs and handler behavior. 🚥 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)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 37ce56bd41
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .attr("expiration", self.expiration.to_string()) | ||
| .attr("admin", self.admin_jid.to_string()) | ||
| .attr("expiration", self.expiration) | ||
| .attr("admin", &self.admin_jid) |
There was a problem hiding this comment.
Passing admin_jid directly here changes encoding from string-path JID serialization to NodeValue::Jid, which calls write_jid_owned and hard-fails when jid.device > 255 (it does a u8::try_from). The previous to_string() path did not error on those values and still produced a wire value, so this introduces a new runtime failure mode for AcceptGroupInviteV4Iq when invites carry high device IDs.
Useful? React with 👍 / 👎.
![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
🤖 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/iq/groups.rs`:
- Around line 2247-2251: Add a regression test that exercises the
NodeBuilder::new("accept") path (the block that calls .attr("code", &self.code),
.attr("expiration", self.expiration), .attr("admin", &self.admin_jid) and
.build()) and assert the built node contains the exact serialized attribute
values for "expiration" and "admin"; specifically, create a deterministic invite
with a known i64 expiration and a known Jid, build the node via the same code
path, and assert node.attr("expiration") equals the expected decimal string and
node.attr("admin") equals the expected Jid string to lock down the trait
conversions for i64 and &Jid.
🪄 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: a7c55080-3c17-40a8-b30f-d6897ea45b5b
📒 Files selected for processing (27)
src/client.rssrc/client/sessions.rssrc/features/newsletter.rssrc/handlers/macros.rssrc/handlers/mod.rssrc/handlers/notification.rssrc/handlers/unimplemented.rssrc/retry.rssrc/socket/noise_socket.rssrc/unified_session.rswacore/binary/Cargo.tomlwacore/binary/benches/numeric_attr_benchmark.rswacore/binary/src/node.rswacore/derive/src/lib.rswacore/src/iq/dirty.rswacore/src/iq/groups.rswacore/src/iq/mediaconn.rswacore/src/iq/node.rswacore/src/iq/prekeys.rswacore/src/iq/privacy.rswacore/src/iq/props.rswacore/src/iq/tctoken.rswacore/src/media_retry.rswacore/src/pair.rswacore/src/send.rswacore/src/stanza/devices.rswacore/src/usync.rs
💤 Files with no reviewable changes (2)
- src/socket/noise_socket.rs
- src/handlers/unimplemented.rs
Benchmark Results59 unchanged benchmark(s)
|
Adds a regression test around the NodeBuilder::new("accept") path that
the perf refactor touched: builds an AcceptGroupInviteV4Iq with a known
i64 expiration and a known admin Jid, and asserts the serialized
attribute values.
This pins the behavior of the From<i64> and From<&Jid> trait
conversions on NodeValue so that any future change to them trips this
test first. The .attr("admin", &self.admin_jid) path is already
ubiquitous in this crate (devices.rs, prekeys.rs, privacy.rs,
business.rs, groups.rs participant/subgroup nodes) and stays consistent
here.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/iq/groups.rs`:
- Around line 3547-3552: The test currently checks admin using parsed
equivalence via accept.attrs().optional_jid("admin") == Some(admin_jid); tighten
it to assert the exact serialized wire string instead by calling
accept.attrs().optional_string("admin").as_deref() and compare to the expected
literal (the exact JID string you expect on the wire) so changes in formatting
will be detected; replace the optional_jid assertion with an optional_string
assertion referencing accept.attrs() and the expected wire string instead of
admin_jid.
🪄 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: 28de0bec-dbbd-4fa7-b0b7-944c2a50e223
📒 Files selected for processing (1)
wacore/src/iq/groups.rs
Tighten test_accept_group_invite_v4_iq_attrs to compare the "admin" attribute via optional_string against the expected literal "5511999887766@s.whatsapp.net" instead of parsing it back into a Jid for a structural compare. optional_string on a NodeValue::Jid goes through the Display impl (the same path the old .to_string() call used to take), so this locks down formatting drift too.
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.
Summary
Follow-ups from a codebase audit pass. Bundled together because each
piece is small and reviewable on its own.
Perf: avoid String allocation for numeric and bool attrs
From<u8..u128 / i8..i128 / usize / isize / bool>impls onNodeValueroute integers throughitoa+CompactString, keepingthem inline on the stack instead of heap-allocating via
.to_string()(max 20 chars fits the 24-byte inline capacity).src/andwacore/to pass valuesdirectly to
.attr(...).ProtocolNodederive macro now emits the new pattern forU32/U64fields, so downstream generated code inherits the improvement.
numeric_attr_benchmark(iai-callgrind) locks in the result.Measured against the
.to_string() → NodeValuepath:RAM hits drop ~42% on individual attrs, consistent with removing the
heap path.
Dead code
UnimplementedHandlerin its entirety (struct, the file, andhandle_unimplementedonClient). Nothing instantiated it anywherein the crate.
runtimefield fromNoiseSocket. It was tagged#[allow(dead_code)]with a kept for potential future spawns notebut never read; the field's
Arc<dyn Runtime>was already beingcloned into the sender task locally.
Refactor and docs
extract_content_bytesandextract_content_uintfromwacore/src/iq/prekeys.rsintowacore/src/iq/node.rs, where theother node helpers (
required_child,optional_attr, etc.) alreadylive, and dropped the local copies from
prekeys.rs.require_from_jid!macro undersrc/handlers/macros.rsandapplied it to four notification handlers (
handle_identity_change,account_sync_devices,handle_picture_notification,handle_status_notification) that shared the exact samematch-and-warn boilerplate. Other call sites with different semantics
(optional JID, different return type, extra normalization) were left
alone.
complete_offline_syncso itdescribes what the code actually does: readers observing
offline_sync_completed = trueshort-circuit without touching thesemaphore, any in-flight worker keeps draining on its old 1-permit
Arc, and newly-spawned workers pick up the 64-permit semaphore viaread_message_semaphore().Test plan
cargo fmt --allcargo build --workspacecargo clippy --workspace --all-targets --exclude e2e-tests(no warnings)cargo test --workspace --exclude e2e-tests(1249 passing, 0 failed)cargo bench -p wacore-binary --bench numeric_attr_benchmark(captured the numbers above)