-
Notifications
You must be signed in to change notification settings - Fork 74
Use String for ContentType in Archives #3012
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Claude finished @codabrink's task —— View job Code Review CompleteReview Date: January 15, 2026, 02:18 UTC SummaryThis PR migrates from enum-based to string-based Feedback1. Fallback Logic Issue (xmtp_db/src/encrypted_store/group_message/convert.rs:14-17)The fallback logic has a subtle bug. The current implementation: let mut content_type: ContentType = value.content_type_save().into();
if matches!(content_type, ContentType::Unknown) {
content_type = value.content_type.into();
}Problem: This only falls back to Recommendation: Reverse the priority - try the new string field first, and only fall back to the enum if the string is empty: let content_type = if value.content_type.is_empty() {
value.content_type_save().into()
} else {
value.content_type.into()
};2. Missing Test CoverageNo tests verify the conversion between
Recommendation: Add unit tests in 3. Proto Design: Extensibility ConcernThe
Note: This is likely intentional for backward compatibility, but worth documenting that 4. Performance: String OperationsOn line 104 of convert.rs, Consideration: This is likely negligible performance impact for typical backup operations, but could be optimized with memoization if this becomes a bottleneck in large exports. 5. Code Quality: Positive Observations✅ Proper use of VerdictApprove with suggestions. The core implementation is sound, but the fallback logic priority should be reversed to avoid incorrectly treating empty strings as "Unknown". Test coverage for the conversion logic would strengthen confidence in backward compatibility. |
Use a string
|
e0dd5c6 to
da09ccf
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3012 +/- ##
==========================================
+ Coverage 74.29% 74.50% +0.20%
==========================================
Files 413 416 +3
Lines 53389 54130 +741
==========================================
+ Hits 39665 40329 +664
- Misses 13724 13801 +77 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
da09ccf to
825ca3c
Compare
| } | ||
| }; | ||
| Ok(content_type) | ||
| _ => Self::Unknown, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This matches the From<String> for Content type impl.
Use a string
|
proto xmtp/proto#316