Chore reduce allocations - #199
Conversation
📝 WalkthroughWalkthroughPre-allocation and borrow/clone reductions were added across multiple modules; a zero-copy-aware Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (14)
💤 Files with no reviewable changes (4)
🧰 Additional context used📓 Path-based instructions (2)**/*.rs📄 CodeRabbit inference engine (AGENTS.md)
Files:
wacore/**/*.rs📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (1)📚 Learning: 2025-12-25T03:36:19.692ZApplied to files:
🧬 Code graph analysis (5)src/store/error.rs (1)
src/message.rs (2)
src/store/persistence_manager.rs (3)
src/client/sessions.rs (1)
src/client.rs (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (19)
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 Report| Branch | chore-reduce-allocations |
| Testbed | ubuntu-latest |
⚠️ WARNING: No Threshold found!Without a Threshold, no Alerts will ever be generated.
- LL Hits (hits)
- Instructions (instructions)
- Estimated Cycles (cycles)
- RAM Hits (hits)
- Total read+write (reads/writes)
- L1 Hits (hits)
Click here to create a new Threshold
For more information, see the Threshold documentation.
To only post results if a Threshold exists, set the--ci-only-thresholdsflag.
Click to view all benchmark results
| Benchmark | Estimated Cycles | cycles x 1e3 | Instructions | instructions | L1 Hits | hits x 1e3 | LL Hits | hits | RAM Hits | hits | Total read+write | reads/writes x 1e3 |
|---|---|---|---|---|---|---|---|---|---|---|---|---|
| binary_benchmark::attr_parser_group::bench_attr_parser attr_lookup:setup_attr_marshaled() | 📈 view plot | 16.32 x 1e3 | 📈 view plot | 7,352.00 | 📈 view plot | 11.29 x 1e3 | 📈 view plot | 12.00 | 📈 view plot | 142.00 | 📈 view plot | 11.44 x 1e3 |
| binary_benchmark::marshal_group::bench_marshal_allocating | 📈 view plot | 263.59 x 1e3 | 📈 view plot | 159,494.00 | 📈 view plot | 245.93 x 1e3 | 📈 view plot | 117.00 | 📈 view plot | 488.00 | 📈 view plot | 246.53 x 1e3 |
| binary_benchmark::marshal_group::bench_marshal_reusing_buffer | 📈 view plot | 264.18 x 1e3 | 📈 view plot | 159,594.00 | 📈 view plot | 246.02 x 1e3 | 📈 view plot | 124.00 | 📈 view plot | 501.00 | 📈 view plot | 246.65 x 1e3 |
| binary_benchmark::unmarshal_group::bench_unmarshal large:setup_large_marshaled() | 📈 view plot | 81.26 x 1e3 | 📈 view plot | 50,338.00 | 📈 view plot | 76.36 x 1e3 | 📈 view plot | 77.00 | 📈 view plot | 129.00 | 📈 view plot | 76.57 x 1e3 |
| binary_benchmark::unmarshal_group::bench_unmarshal small:setup_small_marshaled() | 📈 view plot | 7.89 x 1e3 | 📈 view plot | 3,025.00 | 📈 view plot | 4.80 x 1e3 | 📈 view plot | 10.00 | 📈 view plot | 87.00 | 📈 view plot | 4.90 x 1e3 |
| binary_benchmark::unpack_group::bench_unpack_compressed | 📈 view plot | 1,143.97 x 1e3 | 📈 view plot | 468,057.00 | 📈 view plot | 893.99 x 1e3 | 📈 view plot | 3,481.00 | 📈 view plot | 6,645.00 | 📈 view plot | 904.11 x 1e3 |
| binary_benchmark::unpack_group::bench_unpack_uncompressed | 📈 view plot | 1.79 x 1e3 | 📈 view plot | 779.00 | 📈 view plot | 1.07 x 1e3 | 📈 view plot | 4.00 | 📈 view plot | 20.00 | 📈 view plot | 1.09 x 1e3 |
| reporting_token_benchmark::content_extraction_group::bench_content_extraction extended:setup_extended_message() | 📈 view plot | 35.18 x 1e3 | 📈 view plot | 11,800.00 | 📈 view plot | 16.95 x 1e3 | 📈 view plot | 42.00 | 📈 view plot | 515.00 | 📈 view plot | 17.51 x 1e3 |
| reporting_token_benchmark::content_extraction_group::bench_content_extraction simple:setup_simple_message() | 📈 view plot | 15.87 x 1e3 | 📈 view plot | 3,818.00 | 📈 view plot | 5.46 x 1e3 | 📈 view plot | 9.00 | 📈 view plot | 296.00 | 📈 view plot | 5.76 x 1e3 |
| reporting_token_benchmark::full_generation_group::bench_full_token_generation extended:setup_full_gen_extended() | 📈 view plot | 149.69 x 1e3 | 📈 view plot | 87,956.00 | 📈 view plot | 123.22 x 1e3 | 📈 view plot | 113.00 | 📈 view plot | 740.00 | 📈 view plot | 124.07 x 1e3 |
| reporting_token_benchmark::full_generation_group::bench_full_token_generation simple:setup_full_gen_simple() | 📈 view plot | 130.25 x 1e3 | 📈 view plot | 79,997.00 | 📈 view plot | 111.79 x 1e3 | 📈 view plot | 37.00 | 📈 view plot | 522.00 | 📈 view plot | 112.35 x 1e3 |
| reporting_token_benchmark::key_derivation_group::bench_key_derivation | 📈 view plot | 77.03 x 1e3 | 📈 view plot | 51,035.00 | 📈 view plot | 71.06 x 1e3 | 📈 view plot | 25.00 | 📈 view plot | 167.00 | 📈 view plot | 71.25 x 1e3 |
| reporting_token_benchmark::message_encoding_group::bench_message_encoding extended:setup_extended_message() | 📈 view plot | 24.46 x 1e3 | 📈 view plot | 5,719.00 | 📈 view plot | 8.38 x 1e3 | 📈 view plot | 31.00 | 📈 view plot | 455.00 | 📈 view plot | 8.87 x 1e3 |
| reporting_token_benchmark::message_encoding_group::bench_message_encoding simple:setup_simple_message() | 📈 view plot | 11.89 x 1e3 | 📈 view plot | 2,102.00 | 📈 view plot | 3.10 x 1e3 | 📈 view plot | 2.00 | 📈 view plot | 251.00 | 📈 view plot | 3.35 x 1e3 |
| reporting_token_benchmark::token_calculation_group::bench_token_calculation | 📈 view plot | 34.83 x 1e3 | 📈 view plot | 21,863.00 | 📈 view plot | 30.59 x 1e3 | 📈 view plot | 14.00 | 📈 view plot | 119.00 | 📈 view plot | 30.72 x 1e3 |
![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
🧹 Nitpick comments (4)
src/client/sender_keys.rs (1)
37-59: Good optimization: Pre-computing prefixes reduces allocations.Moving the prefix string formatting outside the filter loop (lines 38-43) is a solid optimization that eliminates repeated allocations per participant. The filtering logic correctly handles all three cases: colon prefix, at-sign prefix, and exact match.
Optional: Minor simplification opportunity
The exact match checks at lines 53 and 58 (
own_lid_user.as_ref().is_some_and(|lid| p.as_str() == lid)) are slightly redundant. Iflid_prefixesisSome, thenown_lid_useris alsoSome, so you could capture the user string in the prefix tuple to avoid the repeated lookup:let lid_prefixes = own_lid_user .as_ref() - .map(|lid| (format!("{lid}:"), format!("{lid}@"))); + .map(|lid| (format!("{lid}:"), format!("{lid}@"), lid.as_str()));Then in the filter:
-let is_own_lid = lid_prefixes.as_ref().is_some_and(|(colon, at)| { - p.starts_with(colon) - || p.starts_with(at) - || own_lid_user.as_ref().is_some_and(|lid| p.as_str() == lid) +let is_own_lid = lid_prefixes.as_ref().is_some_and(|(colon, at, exact)| { + p.starts_with(colon) || p.starts_with(at) || p.as_str() == *exact });However, this is a very minor improvement and the current code is clear and correct.
wacore/libsignal/src/protocol/session_cipher.rs (1)
494-557: Ownership-based session_state handling looks correct; consider a guard to future‑proof restoreThe
take_session_state+set_session_statepattern correctly restores the current session on all current paths (success,DuplicatedMessage, PreKey failure, Whisper fallback). This preserves behavior while avoiding a clone.If this function evolves, a small RAII guard (that writes back on drop unless explicitly “committed”) would reduce the risk of a future early‑return forgetting to call
set_session_state.wacore/libsignal/src/protocol/state/session.rs (1)
647-678: New take/restore APIs are coherent; consider enforcing the archived session cap
take_session_state,take_previous_session, andrestore_previous_sessionprovide the right primitives for ownership‑based manipulation of current/previous sessions and integrate cleanly withSessionState/SessionStructureconversions.One thing to watch:
restore_previous_sessiondoes not enforceconsts::ARCHIVED_STATES_MAX_LENGTH, unlikearchive_current_state_inner. If these APIs are later used heavily, it would be safer either to enforce the same cap here (e.g. drop the oldest when exceeding the limit) or to document that callers must uphold the invariant themselves.wacore/binary/src/node.rs (1)
2-8: ValueRef abstraction for attribute values is well‑structured; document the public API breakThe introduction of
ValueRef<'a>(String/Jid), its helpers (as_str,as_jid,to_jid,to_string_cow,Display), and the switch toAttrsRef<'a> = Vec<(Cow<'a, str>, ValueRef<'a>)>form a coherent model:
- Owned
Node→ borrowedNodeRefnow tags attrs asValueRef::Stringwithout changing behavior.NodeRef::get_attrandattrs_iterexpose the richer value type, enabling callers to avoid JID string allocations when they care.NodeRef::to_ownedcorrectly round‑trips back toNodeby usingto_string_cow().into_owned()for each value.Since
AttrsRefandNodeRef::get_attrare public, this is a source‑breaking change for downstream crates. It would be good to call that out in the crate’s changelog / release notes so users know to update their code to handleValueRefrather than plain strings.Also applies to: 9-16, 18-51, 53-63, 119-135, 203-205, 207-209, 240-247
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
src/appstate_sync.rssrc/client.rssrc/client/device_registry.rssrc/client/sender_keys.rssrc/client/sessions.rssrc/lid_pn_cache.rssrc/message.rssrc/prekeys.rssrc/session.rswacore/binary/src/attrs.rswacore/binary/src/decoder.rswacore/binary/src/encoder.rswacore/binary/src/node.rswacore/libsignal/src/protocol/session_cipher.rswacore/libsignal/src/protocol/state/session.rswacore/tests/binary_protocol_test.rs
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Never modify Device state directly; useDeviceCommand+PersistenceManager::process_command()for state changes
For read-only Device state access, usePersistenceManager::get_device_snapshot()
All blocking I/O (such asureqcalls) and heavy CPU-bound tasks (like media encryption) MUST be wrapped intokio::task::spawn_blockingto avoid stalling the async runtime
UseClient::chat_locksto serialize per-chat operations in asynchronous code
Usethiserrorfor custom domain-specific errors (e.g.,SocketError) andanyhow::Errorfor functions with multiple failure modes
Avoid.unwrap()and.expect()outside of tests and unrecoverable logic paths
Use theDownloadabletrait inwacore/src/download.rsfor implementing generic media download interfaces across message types
Always refreshMediaConnif it's expired before using it to get current media servers and auth tokens
Runcargo fmtbefore finalizing a feature or fix
Runcargo clippy --all-targetsbefore finalizing a feature or fix
Runcargo test --allbefore finalizing a feature or fix
Files:
src/session.rssrc/prekeys.rssrc/lid_pn_cache.rssrc/client/device_registry.rssrc/client/sessions.rssrc/client/sender_keys.rswacore/libsignal/src/protocol/state/session.rswacore/tests/binary_protocol_test.rssrc/appstate_sync.rswacore/libsignal/src/protocol/session_cipher.rssrc/client.rswacore/binary/src/decoder.rswacore/binary/src/node.rssrc/message.rswacore/binary/src/attrs.rswacore/binary/src/encoder.rs
wacore/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
wacorecrate must remain platform-agnostic and have no dependencies on Tokio or specific databases
Files:
wacore/libsignal/src/protocol/state/session.rswacore/tests/binary_protocol_test.rswacore/libsignal/src/protocol/session_cipher.rswacore/binary/src/decoder.rswacore/binary/src/node.rswacore/binary/src/attrs.rswacore/binary/src/encoder.rs
🧠 Learnings (2)
📚 Learning: 2025-12-25T03:36:19.692Z
Learnt from: CR
Repo: jlucaso1/whatsapp-rust PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-25T03:36:19.692Z
Learning: Applies to **/*.rs : Avoid `.unwrap()` and `.expect()` outside of tests and unrecoverable logic paths
Applied to files:
wacore/tests/binary_protocol_test.rs
📚 Learning: 2025-12-25T03:36:19.692Z
Learnt from: CR
Repo: jlucaso1/whatsapp-rust PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-25T03:36:19.692Z
Learning: Applies to wacore/**/*.rs : `wacore` crate must remain platform-agnostic and have no dependencies on Tokio or specific databases
Applied to files:
wacore/tests/binary_protocol_test.rs
🧬 Code graph analysis (6)
src/client/sessions.rs (1)
wacore/src/usync.rs (1)
jids(27-34)
src/client/sender_keys.rs (1)
wacore/binary/src/jid.rs (2)
lid(330-336)pn(321-327)
wacore/tests/binary_protocol_test.rs (2)
wacore/binary/src/node.rs (2)
as_str(20-25)get_attr(203-205)wacore/src/request.rs (1)
as_str(16-21)
wacore/binary/src/decoder.rs (1)
wacore/binary/src/token.rs (2)
get_double_token(41-46)get_single_token(37-39)
src/message.rs (2)
wacore/libsignal/src/protocol/protocol.rs (1)
ciphertext(456-458)wacore/libsignal/src/protocol/group_cipher.rs (1)
group_decrypt(152-235)
wacore/binary/src/encoder.rs (2)
wacore/binary/src/node.rs (2)
attrs(145-147)attrs(243-247)wacore/binary/src/builder.rs (1)
attrs(24-34)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build & Test
- GitHub Check: benchmark
🔇 Additional comments (11)
src/client.rs (1)
1229-1229: LGTM: Clean pre-allocation optimization.Pre-allocating
to_requestwith the exact capacity needed avoids reallocations during the subsequent push operations in the loop (lines 1232-1241).wacore/tests/binary_protocol_test.rs (1)
85-86: LGTM: Test correctly adapted for ValueRef changes.The change from
.as_ref()to.as_str().expect(...)correctly handles the newValueReftype that can represent either String or Jid values. The use of.expect()in test code is appropriate per coding guidelines.Also applies to: 93-94
src/session.rs (1)
112-113: LGTM: Appropriate pre-allocation for session batching.Pre-allocating both vectors with
jids_needing_sessions.len()is a sensible upper bound since each JID will go into eitherto_processorto_wait. This avoids reallocations during the classification loop (lines 115-128).src/prekeys.rs (1)
100-101: LGTM: Precise pre-allocation for pre-key generation.Pre-allocating with
WANTED_PRE_KEY_COUNTmatches the loop at line 133 that generates up to this many keys. The early break at line 142 for ID overflow is a rare edge case, so the capacity hint is appropriate for the common path.src/client/device_registry.rs (1)
148-156: LGTM: Efficient cloning strategy with clear intent.The change from assignment to
clone_from(line 148) allows reusing the existing allocation inrecord.user, and creating an explicitrecord_for_cache(line 151) makes the ownership flow clearer: the cache gets a clone while the backend consumes the original record (line 160). This reduces allocations compared to cloning for both destinations.src/lid_pn_cache.rs (1)
84-108: LGTM: Smart clone reduction from 2 to 1.Computing
should_update_pnupfront (lines 84-91) and cloning only once (line 94) is a nice optimization. When both maps need updating, this reduces clones from 2 to 1 by reusing the originalentryfor the PN map (lines 103-108) instead of cloning again.src/appstate_sync.rs (1)
137-150: LGTM: Well-chosen capacity hints for batch processing.Pre-allocating
need_db_lookupwithpatch.mutations.len()(line 137) is a reasonable upper bound since the loop (lines 138-146) filters duplicates. Similarly,db_prevcapacity ofneed_db_lookup.len()(line 150) is exact for the subsequent loop (lines 151-159) that populates it.src/client/sessions.rs (1)
55-58: Slice‑based API and preallocation changes are soundUsing
Vec::with_capacity(resolved_jids.len()), iteratingchunks(...)as slices, and changingfetch_and_establish_sessionsto take&[Jid]together remove redundant allocations without changing behavior. Callers (ensure_e2e_sessions,establish_primary_phone_session_immediate) are updated correctly.Also applies to: 83-89, 96-162, 194-197
src/message.rs (1)
380-382: Borrowing ciphertext slices instead of allocating Vecs is correct hereSwitching to
&[u8]forciphertextin both session and group batch paths, and passing those slices directly intoPreKeySignalMessage::try_from,SignalMessage::try_from,message_decrypt, andgroup_decryptremoves unnecessary clones while keeping lifetimes safe (the backingVec<u8>lives as long as theNode). The preallocation forsession_enc_nodes/group_content_enc_nodesis also harmless and aligns with the PR goal.Also applies to: 549-576, 560-590, 661-671, 849-852, 873-876
wacore/binary/src/decoder.rs (1)
3-4: ValueRef‑based attribute decoding matches prior semantics and avoids JID string allocs
read_valuecorrectly mirrorsread_value_as_string’s token mapping while returningValueRef(string vs JID) instead of eagerly allocating a full JID string.read_attributesnow uses this and falls back toValueRef::String(Cow::Borrowed(\"\"))forLIST_EMPTY, which matches the previous “missing value → empty string” behavior.The lifetime wiring (
ValueRef<'a>holdingJidRef<'a>orCow<'a, str>) is consistent withDecoder<'a>andNodeRef<'a>, so zero‑copy behavior is preserved.Also applies to: 204-240, 327-337
wacore/binary/src/encoder.rs (1)
322-331: write_attributes_ref correctly bridges ValueRef to the existing string encoderUsing
value.to_string_cow()here is the right pairing with the newValueRefrepresentation on the decode side: it preserves prior behavior for string attributes and ensures JID values are serialized via their string form sowrite_stringcan re‑encode them using the optimized JID tokens.
| use crate::node::{Attrs, Node, NodeRef, ValueRef}; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Get file size and content of attrs.rs
wc -l wacore/binary/src/attrs.rsRepository: jlucaso1/whatsapp-rust
Length of output: 96
🏁 Script executed:
# Read the attrs.rs file to understand the structure
cat -n wacore/binary/src/attrs.rs | head -200Repository: jlucaso1/whatsapp-rust
Length of output: 7305
🏁 Script executed:
# Search for ValueRef definition and AttrParserRef implementation
fd -t f "*.rs" wacore/ | xargs rg -l "ValueRef|AttrParserRef" | head -10Repository: jlucaso1/whatsapp-rust
Length of output: 502
🏁 Script executed:
# Search for optional_string usages across the codebase
rg -n "optional_string\(" --type rustRepository: jlucaso1/whatsapp-rust
Length of output: 4112
🏁 Script executed:
# Find ValueRef definition and key methods
cat -n wacore/binary/src/node.rs | grep -A 30 "enum ValueRef\|impl.*ValueRef"Repository: jlucaso1/whatsapp-rust
Length of output: 3857
🏁 Script executed:
# Search for as_str and to_jid implementations
rg -n "fn as_str|fn to_jid" wacore/binary/src/node.rs -A 5Repository: jlucaso1/whatsapp-rust
Length of output: 473
🏁 Script executed:
# Check the specific call sites mentioned as potentially problematic
rg -B 3 -A 3 'optional_string\("jid"\)' wacore/src/features/contacts.rsRepository: jlucaso1/whatsapp-rust
Length of output: 137
🏁 Script executed:
# Find contacts.rs in the correct location
find . -name "contacts.rs" -type fRepository: jlucaso1/whatsapp-rust
Length of output: 92
🏁 Script executed:
# Search for optional_string("jid") calls more broadly
rg -n 'optional_string\("jid"\)' --type rustRepository: jlucaso1/whatsapp-rust
Length of output: 455
🏁 Script executed:
# Check what the encoder does to understand how attributes become ValueRef variants
rg -n "ValueRef::" wacore/binary/src/encoder.rs -B 2 -A 2Repository: jlucaso1/whatsapp-rust
Length of output: 48
🏁 Script executed:
# Look for how attributes are decoded to understand if "jid" attributes are decoded as Jid or String variants
rg -n "jid.*=|ValueRef" wacore/binary/src/decoder.rs | head -40Repository: jlucaso1/whatsapp-rust
Length of output: 1165
🏁 Script executed:
# Search for how jid attributes are encoded/created
rg -n '"jid"' wacore/binary/src/ --type rust -B 2 -A 2Repository: jlucaso1/whatsapp-rust
Length of output: 48
🏁 Script executed:
# Check the actual call sites in contacts.rs to see how jid_str is used
rg -B 5 -A 10 'optional_string\("jid"\)' src/features/contacts.rsRepository: jlucaso1/whatsapp-rust
Length of output: 2053
🏁 Script executed:
# See if there's any code that explicitly creates ValueRef::Jid for jid attributes
rg -n 'attrs.*jid' wacore/binary/src/ --type rust -iRepository: jlucaso1/whatsapp-rust
Length of output: 48
🏁 Script executed:
# Look for attributes that might be JID-typed in the decoder logic
rg -n "JID_PAIR|AD_JID|INTEROP_JID|FB_JID" wacore/binary/src/decoder.rs -B 5 -A 5Repository: jlucaso1/whatsapp-rust
Length of output: 2461
🏁 Script executed:
# Check if there's any encoder logic that creates JID-typed attributes
rg -n "ValueRef::Jid" wacore/binary/src/encoder.rs -B 3 -A 3Repository: jlucaso1/whatsapp-rust
Length of output: 48
🏁 Script executed:
# Search for all attributes being read to understand which ones might be JID-typed
rg -n 'attrs\(\)\.optional_' src/ --type rust | head -50Repository: jlucaso1/whatsapp-rust
Length of output: 3497
🏁 Script executed:
# Look at the test to understand how attributes are structured
cat -n wacore/tests/binary_protocol_test.rs | head -100Repository: jlucaso1/whatsapp-rust
Length of output: 4435
🏁 Script executed:
# Check if there's any documentation or protocol spec about which attributes are JID-typed
find . -name "*.md" -type f | xargs grep -l "jid\|attribute" 2>/dev/null | head -5Repository: jlucaso1/whatsapp-rust
Length of output: 77
🏁 Script executed:
# Search for how attributes are created during decoding to see if any are JID-typed
rg -n "read_attribute\|read_attr" wacore/binary/src/decoder.rs -A 10Repository: jlucaso1/whatsapp-rust
Length of output: 48
🏁 Script executed:
# Check if there are any integration tests that show JID-typed attributes being used
rg -n "JID_PAIR\|AD_JID" wacore/ --type rustRepository: jlucaso1/whatsapp-rust
Length of output: 48
🏁 Script executed:
# Look at the actual protocol to understand which attributes might be JID-typed
rg -n 'token::' wacore/binary/src/decoder.rs | grep -i "attr\|jid" | head -20Repository: jlucaso1/whatsapp-rust
Length of output: 755
🏁 Script executed:
# Check the encoder to see if it explicitly encodes jid attributes as strings or JID tokens
rg -n "jid\|Jid" wacore/binary/src/encoder.rs -B 2 -A 2 | head -60Repository: jlucaso1/whatsapp-rust
Length of output: 48
🏁 Script executed:
# Look at how attributes are built in the builder to understand the protocol
cat -n wacore/binary/src/builder.rs | head -100Repository: jlucaso1/whatsapp-rust
Length of output: 2091
🏁 Script executed:
# Check if the encoder has any logic to encode attributes as JID types
cat -n wacore/binary/src/encoder.rsRepository: jlucaso1/whatsapp-rust
Length of output: 24604
Use optional_jid() for "jid" attributes; optional_string() now returns None for JID-typed attribute values
The migration of AttrParserRef to ValueRef introduced a behavior change in optional_string(): it now returns None for attributes encoded as JID tokens (JID_PAIR, AD_JID, etc.), whereas previously they would have been materialized as strings like "user@server".
This affects several call sites that read the "jid" attribute:
src/features/contacts.rs(lines 261, 290, 398)src/features/blocking.rs(line 74)
These locations call optional_string("jid") and then attempt to parse the result as a Jid. If the "jid" attribute is encoded as a JID token in the protocol (which occurs when the encoder detects valid JID format during serialization), the call returns None instead of the string representation.
Update these call sites to use optional_jid("jid") instead, which correctly handles both String and Jid variants.
🤖 Prompt for AI Agents
In wacore/binary/src/attrs.rs around lines 6-7 and referenced call sites
src/features/contacts.rs (lines 261, 290, 398) and src/features/blocking.rs
(line 74), replace uses of optional_string("jid") with optional_jid("jid") so
JID-typed attributes encoded as JID tokens are correctly returned and parsed;
update the imports if necessary to bring optional_jid into scope and run tests
to ensure compilation and Jid parsing work as before.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/client/sender_keys.rs (1)
31-35: Consider usingget_device_snapshot()for read-only device state access.As per coding guidelines, read-only Device state access should use
PersistenceManager::get_device_snapshot()instead of theget_device_arc().await+.read().awaitpattern. This would simplify the code and align with the established pattern.🔎 Suggested refactor
- let device_store = self.persistence_manager.get_device_arc().await; - let device_guard = device_store.read().await; - let own_lid_user = device_guard.lid.as_ref().map(|j| j.user.clone()); - let own_pn_user = device_guard.pn.as_ref().map(|j| j.user.clone()); - drop(device_guard); + let device_snapshot = self.persistence_manager.get_device_snapshot().await; + let own_lid_user = device_snapshot.lid.as_ref().map(|j| j.user.clone()); + let own_pn_user = device_snapshot.pn.as_ref().map(|j| j.user.clone());Based on coding guidelines for read-only Device state access.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/client/sender_keys.rswacore/libsignal/src/protocol/state/session.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- wacore/libsignal/src/protocol/state/session.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Never modify Device state directly; useDeviceCommand+PersistenceManager::process_command()for state changes
For read-only Device state access, usePersistenceManager::get_device_snapshot()
All blocking I/O (such asureqcalls) and heavy CPU-bound tasks (like media encryption) MUST be wrapped intokio::task::spawn_blockingto avoid stalling the async runtime
UseClient::chat_locksto serialize per-chat operations in asynchronous code
Usethiserrorfor custom domain-specific errors (e.g.,SocketError) andanyhow::Errorfor functions with multiple failure modes
Avoid.unwrap()and.expect()outside of tests and unrecoverable logic paths
Use theDownloadabletrait inwacore/src/download.rsfor implementing generic media download interfaces across message types
Always refreshMediaConnif it's expired before using it to get current media servers and auth tokens
Runcargo fmtbefore finalizing a feature or fix
Runcargo clippy --all-targetsbefore finalizing a feature or fix
Runcargo test --allbefore finalizing a feature or fix
Files:
src/client/sender_keys.rs
🧬 Code graph analysis (1)
src/client/sender_keys.rs (1)
wacore/binary/src/jid.rs (2)
lid(330-336)pn(321-327)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build & Test
- GitHub Check: benchmark
🔇 Additional comments (2)
src/client/sender_keys.rs (2)
37-44: Nice optimization to reduce allocations in the filter loop.Pre-computing the prefix strings outside the loop is an effective optimization. Instead of allocating format strings for each participant (potentially many times), the code now allocates just once per user type (lid and pn). This aligns well with the PR's objective to reduce allocations.
51-56: Filter logic correctly handles various JID formats.The filter logic appropriately checks for colon-prefixed (device-specific), at-sign-prefixed (full JID), and exact match (bare user) formats to identify and exclude own devices. The use of
is_some_andis idiomatic and efficient.
1 participant
Summary by CodeRabbit
Performance
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.