perf!: reduce allocations in upload, send, and Jid paths - #471
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughRefactors media crypto fields from Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 docstrings
🧪 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 |
Bencher Report
|
@claude review |
|
Claude finished @jlucaso1's task in 1m 25s —— View job Code Review:
|
![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/upload.rs (1)
232-242:⚠️ Potential issue | 🟠 MajorRedact
UploadResponsebefore these fields hit logs.With
media_keyon this public type, the existing derivedDebugwill dump secret key material verbatim. Because the same struct also carries the retrieval paths, a stray{:?}log is enough to expose uploaded media metadata outside the process.Suggested redacted
Debugimpl-#[derive(Debug, Clone)] +#[derive(Clone)] pub struct UploadResponse { pub url: String, pub direct_path: String, pub media_key: [u8; 32], pub file_enc_sha256: [u8; 32], pub file_sha256: [u8; 32], pub file_length: u64, /// Unix timestamp (seconds) when the media key was generated. pub media_key_timestamp: i64, } + +impl std::fmt::Debug for UploadResponse { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("UploadResponse") + .field("url", &self.url) + .field("direct_path", &self.direct_path) + .field("media_key", &"<redacted>") + .field("file_enc_sha256", &"<redacted>") + .field("file_sha256", &"<redacted>") + .field("file_length", &self.file_length) + .field("media_key_timestamp", &self.media_key_timestamp) + .finish() + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/upload.rs` around lines 232 - 242, The UploadResponse struct currently derives Debug and will print secret key material; replace the derived Debug with a custom impl for UploadResponse that redacts sensitive fields (media_key, file_enc_sha256, file_sha256 and optionally media_key_timestamp) when formatted, while still printing non-sensitive fields (url, direct_path, file_length); locate the UploadResponse type and remove #[derive(Debug)] then add a Debug implementation that emits placeholder text like "<redacted>" or hex-truncated values for those specific fields to prevent key leakage in logs.
🤖 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/sticker_pack.rs`:
- Around line 212-219: The builder currently only serializes
zip_upload.media_key but ignores a mismatched thumbnail key; add an explicit
validation in the sticker pack builder (where zip_upload and thumb_upload are
used) to require zip_upload.media_key == thumb_upload.media_key and fail early
if not (e.g., return a Result::Err or propagate a typed error) so you never emit
a proto with incompatible media keys; alternatively enforce this invariant in
the input types (e.g., accept a single MediaKey used for both uploads) and
update the code paths that set file_sha256/thumbnail_sha256 to use the
validated/shared media_key.
---
Outside diff comments:
In `@src/upload.rs`:
- Around line 232-242: The UploadResponse struct currently derives Debug and
will print secret key material; replace the derived Debug with a custom impl for
UploadResponse that redacts sensitive fields (media_key, file_enc_sha256,
file_sha256 and optionally media_key_timestamp) when formatted, while still
printing non-sensitive fields (url, direct_path, file_length); locate the
UploadResponse type and remove #[derive(Debug)] then add a Debug implementation
that emits placeholder text like "<redacted>" or hex-truncated values for those
specific fields to prevent key leakage in logs.
🪄 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: 9395d103-82a0-46ce-ad82-7ee7ae0f4f3d
📒 Files selected for processing (7)
src/features/status.rssrc/upload.rstests/e2e/tests/media.rstests/e2e/tests/newsletter.rswacore/binary/src/jid.rswacore/src/send.rswacore/src/sticker_pack.rs
- UploadResponse/MediaUploadInfo: Vec<u8> → [u8; 32] for media_key, file_sha256, file_enc_sha256 (eliminates heap allocs on construction and makes Clone a cheap memcpy) - UploadOptions::media_key: Vec<u8> → [u8; 32] (removes try_from dance) - Status::send_image/send_video: take UploadResponse by value (moves instead of cloning Strings) - DM send: move message into DeviceSentMessage instead of cloning - Group send: compute phash eagerly, avoiding full Vec<Jid> clone - Jid::new: accept impl Into<String> so callers with owned Strings avoid reallocation
a4ff6d7 to
6d51c9c
Compare
1 participant
Summary
media_key,file_sha256,file_enc_sha256changed fromVec<u8>to[u8; 32]— eliminates heap allocations on construction and makesClonea 96-byte memcpy instead of 3 heap allocsOption<Vec<u8>>→Option<[u8; 32]>— removes thetry_fromvalidation dance since the type is now statically correctUploadResponseby value — moves Strings instead of cloning themmessage_for_encryptionintoDeviceSentMessageinstead of cloning (1 full protobuf Message clone eliminated per DM)Vec<Jid>cloneuser: &str→user: impl Into<String>— callers with ownedStrings skip reallocationBreaking changes & migration
UploadResponsefields changed fromVec<u8>to[u8; 32]UploadOptions::with_media_keytakes[u8; 32]instead ofVec<u8>MediaUploadInfo::newtakes[u8; 32]instead ofVec<u8>Status::send_image/send_videotakeUploadResponseby valueJid::newacceptsimpl Into<String>(non-breaking for most callers)Existing
Jid::new("user", "server")calls continue to work. Callers with ownedStrings now avoid a reallocation:Test plan
cargo clippy --all --tests— cleancargo test --all --lib— all unit tests passSummary by CodeRabbit
Performance
Bug Fixes
Refactor