add edit message

[mchenani]

Add edit message support across Rust core, FFI bindings, and iOS/Android/Node/WASM SDKs

• Introduces EditMessage as a new content type with a codec, proto definition, and message_edits DB table (via migration) storing edits keyed to original messages.
• Adds Group.edit_message in Rust that validates authorship, editability, and content-type match before storing and broadcasting a MessageEdited local event.
• Extends message enrichment to attach the latest valid edit to DecodedMessage.edited, with deleted messages never showing edits and reply threading preserved.
• Exposes edit_message, delete_message, and stream_message_edits across all bindings: mobile FFI, Node N-API, and WASM, with FfiMessageEditCallback / StreamCallback.on_message_edited interfaces.
• Risk: DbQuery now requires QueryMessageEdit, so all implementors must add the new trait; enriched DecodedMessage.content() returns edited content by default, changing observable content for callers who do not check is_edited.

^{Macroscope summarized 6be5aaf.}

[claude]

Claude finished @mchenani's task —— View job

---

PR Review: Edit Message Support

Review date: 2026-03-05 19:19 UTC

Todo List

• Read core implementation files
• Review binding implementations
• Analyze test coverage
• Provide feedback

---

Summary

This is a well-structured implementation of message editing functionality. The core logic is sound with proper validation, good test coverage including edge cases (concurrent edits, out-of-order delivery, authorization), and consistent patterns across all bindings. Most issues flagged by automated reviews have been addressed.

Issues Found

1. Potential race condition in edit_message (crates/xmtp_mls/src/groups/mod.rs:903-919)

Severity: Medium

The edit_message function sends the edit message to the network first (send_message_optimistic), then stores it locally (edit.store(&conn)?). If the local storage fails after network send succeeds, the edit exists on the network but not locally, causing inconsistency.

While the author notes "chances are rare and nothing harmful would happen," this could lead to:

• The sender's client showing the original content while other clients show the edited content
• The edit not appearing in the sender's message history
• Confusion in multi-device scenarios

Recommendation: Reverse the order—store locally first, then send to network. If the network send fails, the local state can be cleaned up on next sync. This is a safer failure mode.

// Store the edit record first
edit.store(&conn)?;

// Then send to network
let edit_message_id = self.send_message_optimistic(&buf, SendMessageOpts::default())?;

---

2. Missing compression field restoration in iOS codec (sdks/ios/Sources/XMTPiOS/Codecs/EditMessageCodec.swift:52-74)

Severity: Medium
Status: Already flagged in review comments but noting here for completeness

The decode function doesn't restore the compression field from FfiEncodedContent back to EncodedContent. After encode converts encoded.compression to ffiContent.compression, a round-trip loses this value. This breaks decompression.

This was already identified in the macroscopeapp review from 2026-03-03.

---

3. DB schema allows duplicate edits (crates/xmtp_db/migrations/2026-01-15-000000-0000_edit_message/up.sql:5-31)

Severity: Low

The message_edits table uses id (the edit message ID) as the primary key but has no uniqueness constraint on (original_message_id, id). This theoretically allows the same edit message to be inserted multiple times for different original messages, though store_or_ignore prevents duplicates in practice.

Impact: The current implementation works correctly due to store_or_ignore usage, but the schema doesn't enforce data integrity at the database level.

Recommendation: Consider adding a comment in the migration explaining why duplicate prevention relies on application logic rather than a DB constraint, or add a unique constraint if appropriate.

---

4. Dynamic SQL construction in get_latest_edits_for_messages (crates/xmtp_db/src/encrypted_store/message_edit.rs:194-220)

Severity: Low (informational)

The function uses string interpolation to build SQL with placeholders, then binds parameters. While the implementation is safe (using proper parameter binding with Diesel's typed bindings), the pattern could be confusing for reviewers expecting pure Diesel query builder syntax.

let placeholders = message_ids.iter().map(|_| "?").collect::<Vec<_>>().join(", ");
let query = format!("SELECT ... WHERE original_message_id IN ({})", placeholders);

Assessment: This is safe—no SQL injection risk. The code correctly uses parameterized queries with typed bindings. This is actually a reasonable approach for the window function query that Diesel's query builder might not express cleanly.

---

Positive Observations

1. Comprehensive test coverage: Tests cover authorization, content-type validation, concurrent edits, out-of-order delivery, deleted messages, and cross-group scenarios
2. Consistent validation: Both client-side (edit_message) and sync-side (process_edit_message) validate authorization and editability
3. Good error handling: Clear error types (EditMessageError) with descriptive variants
4. Cross-platform consistency: All bindings (mobile FFI, Node, WASM) implement edit functionality with consistent patterns
5. Event streaming: Proper MessageEdited events with streaming support across platforms
6. Performance: Efficient DB queries with proper indexing (idx_message_edits_original_timestamp)

---

Coverage Analysis

Codecov reports 83.05% patch coverage with 225 lines missing. Key areas with low coverage:

• bindings/node/src/messages/decoded_message.rs: 0% (40 lines) - Node bindings not tested
• bindings/node/src/conversations/streams.rs: 0% (25 lines) - Stream functions untested
• bindings/mobile/src/mls.rs: 19.04% (34 lines) - FFI bindings partially tested

Recommendation: The core Rust logic has excellent test coverage. The binding coverage gaps are typical for FFI layers which are usually tested at the SDK level (Swift/Kotlin/TypeScript). Consider adding integration tests in bindings/node/test/ to exercise the Node binding paths.

---

Minor Notes

• The author's response about data loss being "rare and nothing harmful" underestimates multi-device sync implications (see issue Refactor to separate wasm bindings from rust crate #1)
• stream_message_edits in Node bindings already passes on_close callback (line 264: || {}) despite review comment saying it's missing—review comment may be outdated
• All macroscopeapp automated review comments marked "No longer relevant" appear to have been addressed

---

[codecov]

Codecov Report

❌ Patch coverage is 83.05723% with 225 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.50%. Comparing base (a513705) to head (6be5aaf).

Files with missing lines	Patch %	Lines
bindings/node/src/messages/decoded_message.rs	0.00%	40 Missing ⚠️
crates/xmtp_db/src/encrypted_store/message_edit.rs	91.42%	35 Missing ⚠️
bindings/mobile/src/mls.rs	19.04%	34 Missing ⚠️
crates/xmtp_mls/src/groups/mls_sync.rs	94.34%	28 Missing ⚠️
bindings/node/src/conversations/streams.rs	0.00%	25 Missing ⚠️
bindings/mobile/src/message.rs	60.78%	20 Missing ⚠️
bindings/node/src/conversation/messages.rs	0.00%	17 Missing ⚠️
crates/xmtp_mls/src/messages/enrichment.rs	88.88%	7 Missing ⚠️
crates/xmtp_mls/src/subscriptions/mod.rs	83.33%	5 Missing ⚠️
crates/xmtp_mls/src/groups/error.rs	0.00%	4 Missing ⚠️
... and 4 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3072      +/-   ##
==========================================
+ Coverage   72.37%   72.50%   +0.13%     
==========================================
  Files         477      479       +2     
  Lines       58942    60264    +1322     
==========================================
+ Hits        42658    43694    +1036     
- Misses      16284    16570     +286     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[mchenani]

Code Quality Issues

1. Missing on_close callback in Node.js stream_message_edits (bindings/node/src/conversations/streams.rs:241)

The stream_message_edits doesn't expose an on_close callback in Node.js bindings. However, stream_message_deletions follows the same pattern.

2. Potential data loss if edit.store(&conn)? fails after network send (crates/xmtp_mls/src/groups/mod.rs:905-921)

The edit_message function sends the message first, then stores the edit record locally. If storage fails, there's a potential inconsistency. But the chances here are rare and nothing harmful would happen.

[macroscopeapp]

Approvability

Verdict: Needs human review

This adds an 'edit message' feature with schema changes, new content types, and cross-platform bindings (mobile, WASM, Node, iOS, Android). Because it introduces user-facing behavior and database migrations, I'd recommend a human verify the changes.

^{You can customize Macroscope's approvability policy. Learn more.}