Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
How to use the Graphite Merge QueueAdd the label mergequeue to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
Claude finished @mchenani's task —— View job Code Review CompleteReview Date: March 18, 2026 SummaryThis PR adds comprehensive message editing functionality across all three bindings (mobile/uniffi, node/napi, WASM). The implementation is well-structured with strong test coverage, but there are important security and edge case concerns that should be addressed. Critical Issues1. Reply Structure Preservation Logic - Potential Data Loss 🔴Location: The logic that preserves reply structure when editing has a subtle issue: // Preserve in_reply_to from the original Reply
if let (MessageBody::Reply(original_reply), MessageBody::Reply(edited_reply)) =
(&msg.content, &mut edited_body)
{
edited_reply.in_reply_to = original_reply.in_reply_to.clone(); // ✅ Good
}
// Wrap non-Reply edited content in Reply if original was a Reply
else if let MessageBody::Reply(original_reply) = &msg.content {
edited_body = MessageBody::Reply(ProcessedReply {
in_reply_to: original_reply.in_reply_to.clone(),
content: Box::new(edited_body),
reference_id: original_reply.reference_id.clone(),
});
final_content_type = metadata.content_type.clone();
}Problem: When editing a reply with plain text, the Recommendation: Add validation to ensure the 2. Missing Validation - Empty Edited Content 🟡Location: The test Question: Is the validation in pub fn edit_message(&self, message_id: Vec<u8>, new_content: Vec<u8>) -> Result<Vec<u8>, FfiError> {
if new_content.is_empty() {
return Err(FfiError::Generic("Edited content cannot be empty".to_string()));
}
// ... rest of implementation
}3. Low Test Coverage in Critical Paths 🟡Per Codecov Report:
The coverage gaps are concerning for new functionality. Based on the file structure:
Recommendation:
Security Concerns4. Authorization Check Location 🔴The tests verify that only the sender can edit (line 1690-1732), but where is this check enforced? Looking at the FFI code, let edit_id = group.edit_message(message_id_bytes, new_content.encode_to_vec())Critical Question: Is authorization checked in the core Verification Needed: Confirm authorization is enforced in 5. Race Condition on Concurrent Edits 🟡Scenario: User A and User B both try to edit the same message simultaneously. Current behavior (inferred from tests):
Potential issue:
Recommendation: Add a test case for concurrent edits and document the expected behavior. Performance Considerations6. Streaming Efficiency 🟡Location: The
Concern: For high-frequency edit streams (e.g., collaborative editing), this could create significant overhead. Recommendation:
7. Missing Streaming Error Handling 🟡Location: pub async fn stream_message_edits(
&self,
callback: ThreadsafeFunction<DecodedMessage, ()>,
) -> Result<StreamCloser> {
let stream_closer = RustXmtpClient::stream_message_edits_with_callback(
self.inner_client.clone(),
move |message| match message {
Ok(decoded_message) => match DecodedMessage::try_from(decoded_message) {
Ok(msg) => {
let _ = callback.call(Ok(msg), ThreadsafeFunctionCallMode::Blocking);
}
Err(e) => {
let _ = callback.call(Err(e), ThreadsafeFunctionCallMode::Blocking); // ⚠️ Error swallowed
}
},
Err(e) => {
let _ = callback.call(
Err(Error::from(ErrorWrapper::from(e))),
ThreadsafeFunctionCallMode::Blocking,
); // ⚠️ Error swallowed
}
},
|| {},
);Issue: Errors in callbacks are swallowed ( Comparison: The Recommendation: Add error logging similar to match callback.call(Ok(msg), ...) {
Ok(_) => {},
Err(e) => tracing::error!("Edit stream callback failed: {:?}", e),
}Code Quality Issues8. Inconsistent Error Messages 🟢The test expectations are good, but error messages could be more informative:
This helps with debugging in production. 9. Magic Numbers in Tests 🟢Location: let fake_msg_id = vec![0xDE, 0xAD, 0xBE, 0xEF];Suggestion: Use a constant or helper function: const INVALID_MESSAGE_ID: [u8; 4] = [0xDE, 0xAD, 0xBE, 0xEF];Positive Observations ✅
Recommendations SummaryBefore Merge:
Post-Merge (Nice to Have): Overall Assessment: This is a solid implementation with good architectural decisions. The main concerns are around test coverage gaps reported by Codecov and ensuring authorization is enforced at the core layer. Once these are addressed, this should be safe to merge. |
ApprovabilityVerdict: Needs human review This PR introduces new edit message functionality across all FFI bindings (mobile, node, wasm), including streaming for edits and complex Reply structure preservation logic. While the author owns all changed files and the PR includes comprehensive tests, this adds significant new user-facing capability with non-trivial runtime logic that warrants human review. No code changes detected at You can customize Macroscope's approvability policy. Learn more. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## mc/edit-message-core #3346 +/- ##
========================================================
+ Coverage 83.12% 83.14% +0.02%
========================================================
Files 378 378
Lines 52034 52124 +90
========================================================
+ Hits 43254 43341 +87
- Misses 8780 8783 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
TL;DR
Added message editing functionality to XMTP mobile, node, and wasm bindings with support for streaming edit notifications and proper handling of reply message edits.
What changed?
FfiEditMessagestruct andEditMessagecontent type support in mobile bindingsedit_message()method on conversations to edit messages by IDstream_message_edits()functionality to receive real-time edit notifications viaFfiMessageEditCallbackFfiDecodedMessagewithedited_at_nstimestamp andis_edited()boolean flagin_reply_toreferencesEditMessagecontent typeeditMessage()andstreamMessageEdits()methodseditMessage(),deleteMessage(), andstreamMessageEdits()How to test?
edit_message(message_id, new_content)to edit itis_edited() == trueand hasedited_at_nstimestampstream_message_edits()callback to receive real-time edit notificationsWhy make this change?
This enables users to correct typos, update information, or modify their messages after sending, which is a standard feature in modern messaging applications. The implementation properly handles complex scenarios like editing replies while preserving message threading, and provides real-time notifications so all participants can see message updates immediately.
Note
Add edit message bindings for mobile, Node, and WASM clients
edit_messageandstream_message_editsmethods to conversation and conversations bindings across mobile (uniffi), Node (napi), and WASM targets.FfiEditMessage/FfiDecodedMessageedit fields (edited_at_ns,is_edited) in the mobile FFI layer, with equivalent fields in Node and WASMDecodedMessage.encode_edit_message/decode_edit_messagehelpers for serializing edit payloads to/fromEncodedContentbytes.Macroscope summarized 1f95d4c.