Skip to content

edit message core functionality#3344

Open
mchenani wants to merge 2 commits intomc/edit-message-dbfrom
mc/edit-message-core
Open

edit message core functionality#3344
mchenani wants to merge 2 commits intomc/edit-message-dbfrom
mc/edit-message-core

Conversation

@mchenani
Copy link
Copy Markdown
Contributor

@mchenani mchenani commented Mar 17, 2026

TL;DR

Adds message editing functionality to XMTP MLS groups, allowing users to edit their own messages with proper validation, authorization, and event streaming.

What changed?

  • Added EditMessageError enum with validation cases (unauthorized, non-editable, deleted message, content type mismatch, empty content)
  • Implemented edit_message() method on groups that validates sender authorization, message editability, and content type consistency
  • Added process_edit_message() for handling incoming edit messages from the network with graceful error handling
  • Created database schema and queries for storing and retrieving message edits (StoredMessageEdit, QueryMessageEdit)
  • Added Editable trait to mark which content types and message kinds can be edited (text, markdown, replies, attachments are editable; reactions, system messages are not)
  • Enhanced message enrichment to show edit metadata and apply edits to referenced messages in reply chains
  • Added stream_message_edits() for real-time edit notifications
  • Filtered EditMessage content type from default message queries to keep edit operations hidden from normal message lists
  • Added comprehensive test coverage for authorization, validation, edge cases, and multi-user scenarios

How to test?

  1. Create a group and send a text message
  2. Call group.edit_message(message_id, new_content) to edit the message
  3. Verify the edit appears in find_enriched_messages() with edit metadata
  4. Test authorization by having another user try to edit (should fail)
  5. Test editing non-editable content like reactions (should fail)
  6. Test editing deleted messages (should fail)
  7. Test content type mismatches (editing text with reply content should fail)
  8. Test reply chains show updated edit state for referenced messages
  9. Test streaming edits with stream_message_edits()

Why make this change?

Message editing is a core messaging feature that users expect. This implementation ensures:

  • Security: Only original senders can edit their messages
  • Consistency: Content types must match between original and edited versions
  • Reliability: Graceful handling of out-of-order messages and malformed data
  • User Experience: Real-time edit notifications and proper display in reply chains
  • Data Integrity: Edit history is preserved while showing the latest version to users

Note

Add edit message core functionality to MLS groups

  • Adds MlsGroup::edit_message in groups/mod.rs to validate, encode, send, and persist message edits, emitting a LocalEvents::MessageEdited event on success.
  • Adds process_edit_message in mls_sync.rs to handle incoming edit messages during sync, storing valid edits and ignoring invalid/unauthorized ones without breaking sync.
  • Enriches DecodedMessage with an edited field populated by enrich_messages in enrichment.rs, which validates edits, applies them to replies' in_reply_to, and suppresses edit metadata for deleted messages.
  • Adds stream_message_edits_with_callback on Client and a MessageEdited variant on LocalEvents for real-time edit notifications.
  • Edit messages are excluded from standard message list queries; EditMessageError covers all failure cases (not authorized, not found, content type mismatch, already deleted, etc.) and are non-retryable.

Macroscope summarized ae2387b.

Copy link
Copy Markdown
Contributor Author

mchenani commented Mar 17, 2026

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.
Learn more


How to use the Graphite Merge Queue

Add 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
Copy link
Copy Markdown

claude bot commented Mar 17, 2026

Claude finished @mchenani's task —— View job


Code Review Complete

Review Date: March 18, 2026 06:11 UTC


Summary

The edit message implementation is generally well-designed with strong authorization, validation, and error handling. The code follows good patterns established in the codebase (similar to delete message functionality). However, there are a few areas that could use attention:


Key Findings

1. Potential Content Type Validation Issue (Medium Priority)

Location: crates/xmtp_mls/src/messages/enrichment.rs:110-132

The is_edit_valid function performs content type validation by comparing edited_type_id with original_type_str, but there's a subtle issue:

let edited_type_id = edited_content
    .r#type
    .as_ref()
    .map(|t| t.type_id.as_str())  // Only comparing type_id
    .unwrap_or("unknown");
let original_type_str = message.content_type.to_string();

if edited_type_id != original_type_str {
    // rejection logic
}

The ContentType::to_string() might produce a different format than just the type_id field. If ContentType includes authority/version in its string representation, this comparison could fail for valid edits or pass for invalid ones. Consider using a more robust comparison that checks all components (authority_id, type_id, version).

Also seen in: crates/xmtp_mls/src/groups/mod.rs:1086-1099 (same issue in the edit_message function)


2. Missing Content Type Validation in process_edit_message (High Priority)

Location: crates/xmtp_mls/src/groups/mls_sync.rs:1630-1750

The process_edit_message function doesn't validate content type matching when processing incoming edits from the network. While it validates authorization and editability, it skips the critical check that edit_message performs at lines 1086-1099 of mod.rs:

// This validation is MISSING in process_edit_message:
if edited_type_id != original_type_str {
    return Err(EditMessageError::ContentTypeMismatch { ... }.into());
}

Without this check, the network processing path could allow content type mismatches that would be rejected when editing locally. This inconsistency could lead to:

  • Database containing invalid edits that fail validation during enrichment
  • Security issue if content type restrictions have security implications

Recommendation: Add the same content type validation in process_edit_message before line 1718 (or accept that validation only happens at enrichment time and document this explicitly).


3. Test Coverage Gaps

Based on the codecov report showing 40 lines uncovered:

  • crates/xmtp_mls/src/groups/error.rs (4 lines): The EditMessageError Display trait implementations are not covered
  • crates/xmtp_mls/src/groups/mls_sync.rs (20 lines): Some error paths in process_edit_message lack coverage
  • crates/xmtp_mls/src/messages/enrichment.rs (8 lines): Some edge cases in edit validation
  • crates/xmtp_mls/src/subscriptions/mod.rs (5 lines): Event streaming for edits

The 20 missing lines in mls_sync.rs are concerning since that's the network-facing code that processes untrusted input.


4. Potential Race Condition with Out-of-Order Messages (Low Priority)

Location: crates/xmtp_mls/src/groups/mls_sync.rs:1718-1735

The code handles out-of-order edits gracefully (comment at line 1718-1719), but there's no clear mechanism to re-validate or emit events when the original message arrives later. Consider:

  • Edit arrives first → stored with store_or_ignore
  • Original message arrives later → no event is emitted for the edit
  • Client may never see the MessageEdited event unless they re-query

This might be intentional (enrichment will show the edit), but worth documenting explicitly if so.


5. Empty Content Validation Inconsistency (Low Priority)

Location: crates/xmtp_mls/src/groups/mod.rs:1080-1082 vs mls_sync.rs:1668-1671

Both functions check for empty content but use slightly different approaches:

  • edit_message: checks edited_content.content.is_empty()
  • process_edit_message: checks same thing but earlier in validation

This is fine, but consider if empty content should fail silently (as in process_edit_message) or return an error (as in edit_message). Current behavior seems reasonable but ensure it's intentional.


Positive Observations

  1. Authorization is properly enforced - Only original sender can edit (no super-admin override, which is correct for edits)
  2. Cross-group edit prevention - Group ID validation prevents editing messages from other groups
  3. Graceful error handling - Network processing returns Ok(()) for invalid edits to avoid disrupting sync
  4. Event emission - Both local and remote edits emit MessageEdited events appropriately
  5. Edit history - Using get_latest_edits_for_messages suggests support for edit history (good design)
  6. Deleted message protection - Cannot edit deleted messages (checked in multiple places)
  7. Code consistency - Edit implementation mirrors delete message patterns well

Recommendations

  1. High Priority: Add content type validation to process_edit_message (or document why it's deferred to enrichment)
  2. Medium Priority: Review content type string comparison logic in both edit_message and is_edit_valid
  3. Low Priority: Add tests to cover the missing 40 lines (especially network processing error paths)
  4. Low Priority: Document out-of-order message handling behavior explicitly

@mchenani mchenani marked this pull request as ready for review March 17, 2026 22:28
@mchenani mchenani requested a review from a team as a code owner March 17, 2026 22:28
@macroscopeapp
Copy link
Copy Markdown
Contributor

macroscopeapp bot commented Mar 17, 2026

Approvability

Verdict: Needs human review

This PR introduces a new core feature (message editing) with substantial new runtime behavior including authorization logic, network message processing, enrichment, and event streaming. Despite the author owning all files and having strong tenure, new feature capabilities of this scope warrant human review to ensure the design aligns with product requirements.

No code changes detected at ae2387b. Prior analysis still applies.

You can customize Macroscope's approvability policy. Learn more.

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 17, 2026

Codecov Report

❌ Patch coverage is 94.03875% with 40 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.12%. Comparing base (2be66f4) to head (ae2387b).

Files with missing lines Patch % Lines
crates/xmtp_mls/src/groups/mls_sync.rs 95.89% 20 Missing ⚠️
crates/xmtp_mls/src/messages/enrichment.rs 87.69% 8 Missing ⚠️
crates/xmtp_mls/src/subscriptions/mod.rs 83.33% 5 Missing ⚠️
crates/xmtp_mls/src/groups/error.rs 0.00% 4 Missing ⚠️
crates/xmtp_mls/src/groups/mod.rs 95.94% 3 Missing ⚠️
Additional details and impacted files
@@                  Coverage Diff                   @@
##           mc/edit-message-db    #3344      +/-   ##
======================================================
+ Coverage               82.86%   83.12%   +0.25%     
======================================================
  Files                     378      378              
  Lines                   51365    52034     +669     
======================================================
+ Hits                    42566    43254     +688     
+ Misses                   8799     8780      -19     

☔ 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant