Pass errors into Stream Callbacks #1182
Conversation
|
Warning Rate limit exceeded@insipx has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 26 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe pull request introduces several changes across multiple files, primarily focusing on enhancing error handling and refining function signatures within the XMTP codebase. Key modifications include the addition of new error types, adjustments to the visibility of struct fields, and updates to method signatures to propagate errors effectively. Additionally, configuration settings for Rust Analyzer have been added to improve diagnostics. Changes to dependency management in Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
insipx
force-pushed
the
insipx/stream-error-callbacks
branch
from
e688233 to
9fd8190
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (5)
.vscode/settings.json (1)
5-10: LGTM! Consider additional FFI-related diagnostic settings.The addition of Rust Analyzer diagnostic settings is appropriate and will help with development of the error handling improvements. The disabled diagnostics are reasonable choices given the codebase's use of FFI and macros.
Consider adding these additional settings to improve FFI development experience:
"rust-analyzer.diagnostics.disabled": [ "unlinked-file", "unresolved-macro-call", "unresolved-proc-macro" - ], + ], + "rust-analyzer.check.command": "clippy", + "rust-analyzer.check.extraArgs": ["--", "-W", "clippy::all", "-W", "clippy::nursery"],This will enable Clippy's FFI and safety-related lints during development, which could help catch potential FFI boundary issues early.
xmtp_mls/src/lib.rs (1)
Line range hint
91-95: Maintain consistent error formatting across macro variants.While the first variant of the macro now uses the Display format (
{}), the second variant still uses Debug format ({:?}). This creates inconsistency in error logging.Apply this diff to maintain consistency:
( $e: expr, $msg: tt ) => { match $e { Ok(v) => Some(v), Err(e) => { - tracing::error!("{}: {:?}", $msg, e); + tracing::error!("{}: {}", $msg, e); None } } };bindings_node/src/conversations.rs (1)
Line range hint
235-245: Consider applying similar error handling to stream_all_messages.For consistency with the improved error handling in the
streammethod, consider updating thestream_all_messagescallback to handle potential errors:move |message| { - tsfn.call(Ok(message.into()), ThreadsafeFunctionCallMode::Blocking); + tsfn.call( + Ok(message) + .map(NapiMessage::from) + .map_err(ErrorWrapper::from) + .map_err(Error::from), + ThreadsafeFunctionCallMode::Blocking, + ); },This would ensure consistent error handling across both streaming methods.
bindings_node/src/groups.rs (1)
561-567: LGTM: Improved error handling in stream callbackThe changes successfully implement error propagation through the FFI boundary by properly mapping and converting errors. This aligns with the PR objective to pass stream errors through to callbacks, allowing external SDKs to handle errors appropriately.
Consider documenting the error types that can be passed through the callback in the TypeScript type definition, as this represents a potentially breaking change in error handling behavior.
xmtp_mls/src/groups/subscriptions.rs (1)
106-106: Remove commented-out code for cleanlinessThe commented-out line appears unnecessary and can be removed to improve code readability.
Apply this diff to remove the commented code:
- // .map_err(|e| GroupError::Generic(e.to_string()))?;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
bindings_wasm/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
- .vscode/settings.json (1 hunks)
- bindings_node/src/conversations.rs (1 hunks)
- bindings_node/src/groups.rs (2 hunks)
- xmtp_mls/src/api/mls.rs (2 hunks)
- xmtp_mls/src/builder.rs (0 hunks)
- xmtp_mls/src/groups/mod.rs (1 hunks)
- xmtp_mls/src/groups/subscriptions.rs (14 hunks)
- xmtp_mls/src/groups/sync.rs (0 hunks)
- xmtp_mls/src/lib.rs (1 hunks)
- xmtp_mls/src/subscriptions.rs (9 hunks)
💤 Files with no reviewable changes (2)
- xmtp_mls/src/builder.rs
- xmtp_mls/src/groups/sync.rs
🔇 Additional comments (18)
xmtp_mls/src/lib.rs (1)
83-83: LGTM! Improved error message readability.Using the Display trait (
{}) instead of Debug ({:?}) provides more user-friendly error messages, which aligns well with the PR's goal of improving error handling.bindings_node/src/conversations.rs (1)
216-216: LGTM! Improved error handling in stream callback.The changes successfully implement proper error propagation through the FFI boundary while maintaining type safety. The error handling chain correctly maps Rust errors to Node.js errors, which aligns well with the PR's objective of passing stream errors through to callbacks.
Also applies to: 220-223
bindings_node/src/groups.rs (1)
78-86: LGTM: Clean implementation of From traitThe implementation correctly handles the conversion from
MlsGroup<RustXmtpClient>toNapiGroup, properly transferring all required fields.xmtp_mls/src/api/mls.rs (2)
5-5: LGTM: Import addition aligns with PR objectives.The addition of
XmtpMlsStreamsimport supports the enhanced error handling in stream callbacks.
265-265: Verify impact of return type change.The return type change from generic stream to
XmtpMlsStreams::GroupMessageStreamimproves error handling through FFI, which aligns with the PR objectives. However, this could be a breaking change.Consider:
- Adding documentation to explain the new error handling behavior
- Verifying that all
XmtpMlsStreamsimplementations properly handle errorsLet's verify the impact:
✅ Verification successful
Let me gather more information about the trait definition and its implementations to better understand the impact.
Based on the gathered information, I can now provide the final response:
Return type change is safe and well-implemented across the codebase
The change from
impl futures::Stream<Item = Result<GroupMessage, ApiError>> + '_to<ApiClient as XmtpMlsStreams>::GroupMessageStream<'_>is safe because:
- All implementations of
GroupMessageStreamconsistently useResult<GroupMessage, Error>as the stream item type- The trait is properly implemented across different contexts:
xmtp_api_http: UsesBoxStreamfor async contextsxmtp_api_grpc: Has a dedicatedGroupMessageStreamtype- Test utilities: Support both
BoxStreamandLocalBoxStreamThe change maintains compatibility while improving type safety and error handling through FFI. No breaking changes are introduced as all implementations follow the same error handling pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other implementations and usages that might be affected # Find all implementations of XmtpMlsStreams echo "Checking XmtpMlsStreams implementations..." ast-grep --pattern 'impl $trait for $type { $$$ }' | rg 'XmtpMlsStreams' # Find all direct usages of subscribe_group_messages echo "Checking subscribe_group_messages usages..." rg -A 3 'subscribe_group_messages\('Length of output: 4172
Script:
#!/bin/bash # Find the XmtpMlsStreams trait definition echo "Checking XmtpMlsStreams trait definition..." ast-grep --pattern 'trait XmtpMlsStreams { $$$ }' # Find all type definitions for GroupMessageStream echo "Checking GroupMessageStream type definitions..." rg "type GroupMessageStream" # Find implementations of XmtpMlsStreams specifically echo "Checking XmtpMlsStreams implementations..." ast-grep --pattern 'impl XmtpMlsStreams for $type { $$$ }'Length of output: 1368
xmtp_mls/src/groups/mod.rs (2)
230-230: LGTM: New error variants enhance error handling.The new error variants in
GroupErrorprovide clear and specific error messages for DM validation, metadata field validation, and sync operation failures. The error messages follow Rust conventions and are well integrated with the codebase.
230-230: Verify the necessity of exposing the client field.The visibility change of the
clientfield frompub(crate)topubis a breaking change that exposes internal implementation details. While this may be necessary for error propagation through callbacks, it could impact encapsulation and make future refactoring more difficult.Run the following script to verify if this field is actually used outside the crate:
✅ Verification successful
The results show that the
clientfield is indeed being accessed inbindings_node/src/groups.rs, which is outside thexmtp_mlscrate. Let me verify if this is part of the intended public API by checking the module structure and documentation.
Let me check the bindings layer more specifically to understand if this is part of the intended public API design.
Based on the evidence gathered, I can now provide a final response:
The
pubvisibility of the client field is justifiedThe visibility change from
pub(crate)topubis necessary as the field is legitimately used in the Node.js bindings layer (bindings_node/src/groups.rs). Specifically, it's used in theFromimplementation that convertsMlsGrouptoNapiGroup, which is part of the public API bridge to Node.js. This is a valid use case for exposing the field, as bindings layers often need access to internal clients for proper integration.
- The field is used in
bindings_node/src/groups.rsfor the Node.js bindings implementation- Within the core crate, usage is still controlled and appears in expected locations like
subscriptions.rsandintents.rs- The visibility change aligns with the codebase's architecture where the bindings layer needs access to core functionality
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for external usage of the client field # Test: Search for direct access to the client field outside the crate. Expect: No matches outside xmtp_mls. rg -A 3 'group\.client' --type rustLength of output: 866
Script:
#!/bin/bash # Check module documentation and structure rg -B 3 -A 3 "pub struct MlsGroup" --type rust # Check if there are any other references to this field in documentation or tests rg "client.*field" --type rustLength of output: 383
Script:
#!/bin/bash # Check the bindings implementation and usage context rg -B 5 -A 5 "inner_client: mls_group\.client" --type rust # Check if there are any related tests or documentation ast-grep --pattern 'pub struct MlsGroup { $$$ }'Length of output: 687
xmtp_mls/src/groups/subscriptions.rs (7)
15-15: ImportingSubscribeErrorfor enhanced error handlingThe addition of
SubscribeErroraligns with the new error handling strategy and is appropriate.
25-25: Updated return type inprocess_stream_entryimproves error specificityChanging the return type to
Result<StoredGroupMessage, SubscribeError>enhances error reporting by using a more specific error type.
63-72: Ensure comprehensive error handling inprocess_stream_entryCurrently, the code handles
SubscribeError::Receive(_)by attempting a sync and logs other errors. Consider verifying that all potentialSubscribeErrorvariants are appropriately handled to prevent unintended behavior.
114-116: Updated return type instreammethod enhances error propagationChanging the return type to a stream of
Result<StoredGroupMessage, SubscribeError>ensures that errors within the stream are properly propagated to the caller.
187-195: Filter logic correctly skips inconsequential errorsThe filter appropriately skips over
SubscribeError::GroupMessageNotFounderrors, preventing unnecessary processing of missing messages.
206-206: Callback signature updated to handle errors effectivelyModifying the callback to accept
Result<StoredGroupMessage, SubscribeError>allows for proper error handling within the callback function.
369-369:⚠️ Potential issueCheck for errors before asserting in tests
Before asserting on the message content, ensure that the
ResultisOkto prevent panics.Apply this diff:
- .starts_with("hello".as_bytes())); + match value { + Ok(msg) => assert!(msg.decrypted_message_bytes.starts_with("hello".as_bytes())), + Err(e) => panic!("Failed to process message: {:?}", e), + }Likely invalid or redundant comment.
xmtp_mls/src/subscriptions.rs (4)
10-15: Imports updated appropriatelyThe imports have been updated to include necessary modules for the new error handling implementation.
89-109: Well-structuredSubscribeErrorenum definitionThe new
SubscribeErrorenum is defined appropriately, encapsulating all relevant error scenarios for subscription processes.
111-126: Correct implementation ofRetryableErrorforSubscribeErrorThe implementation correctly determines the retryable nature of each
SubscribeErrorvariant.
248-250: Callback signature updated to enhance error handlingThe
stream_conversations_with_callbackfunction now passesResult<MlsGroup<Self>, SubscribeError>to the callback, enhancing error handling capabilities.
insipx
force-pushed
the
insipx/stream-error-callbacks
branch
from
9fd8190 to
500e6c6
Compare
insipx
force-pushed
the
insipx/stream-error-callbacks
branch
from
326184f to
63fddda
Compare
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (5)
bindings_ffi/src/lib.rs (1)
49-53: Architecture looks good for error propagation.The new
FfiSubscribeErrortype is well-structured for FFI compatibility and aligns with the PR's objective of passing stream errors through callbacks. The use ofuniffi::Errorandthiserrorensures proper error handling across language boundaries.Cargo.toml (2)
Line range hint
65-78: Review the implications of new dependencies.Several significant additions and changes:
- The tracing setup with
release_max_level_debugcould impact production performance- The diesel ecosystem integration suggests new database functionality
- Vendored OpenSSL dependencies ensure build consistency but increase bundle size
Consider:
- Documenting the logging level configuration for different environments
- Adding migration guides for the new database functionality
- Evaluating the impact of vendored dependencies on binary size
Line range hint
107-134: Profile configurations are well-optimized.The profile settings show careful consideration of different build targets:
- Incremental builds in release mode can speed up development
- Binary size optimizations for WASM and node bindings
- LTO enabled where appropriate
Consider documenting the performance implications of these profile settings, especially:
- The impact of
opt-level = "s"vs"z"on different targets- The tradeoffs of enabling incremental builds in release mode
xmtp_mls/src/groups/subscriptions.rs (2)
71-72: Enhance error logging messageThe error message could be more descriptive to aid in debugging.
Apply this diff:
- tracing::error!("Process stream entry {:?}", e); + tracing::error!("Failed to process stream entry: {:?}", e);
Line range hint
1-428: Overall implementation looks solidThe changes successfully implement error propagation through callbacks, which was the main objective of this PR. The implementation includes:
- Proper error type handling
- Appropriate error logging
- Consistent error propagation through streams and callbacks
The code structure is clean and maintainable.
Consider documenting the error handling behavior in the module-level documentation to help users understand how to properly handle errors in their callbacks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockbindings_wasm/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
- .vscode/settings.json (1 hunks)
- Cargo.toml (2 hunks)
- bindings_ffi/Cargo.toml (2 hunks)
- bindings_ffi/Makefile (1 hunks)
- bindings_ffi/gen_kotlin.sh (1 hunks)
- bindings_ffi/src/lib.rs (1 hunks)
- bindings_ffi/src/mls.rs (39 hunks)
- bindings_node/src/conversations.rs (7 hunks)
- bindings_node/src/groups.rs (2 hunks)
- dev/release-kotlin (1 hunks)
- xmtp_mls/src/builder.rs (0 hunks)
- xmtp_mls/src/groups/mod.rs (1 hunks)
- xmtp_mls/src/groups/subscriptions.rs (13 hunks)
- xmtp_mls/src/groups/sync.rs (0 hunks)
- xmtp_mls/src/lib.rs (1 hunks)
- xmtp_mls/src/subscriptions.rs (17 hunks)
💤 Files with no reviewable changes (2)
- xmtp_mls/src/builder.rs
- xmtp_mls/src/groups/sync.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- .vscode/settings.json
- bindings_node/src/conversations.rs
- xmtp_mls/src/lib.rs
🔇 Additional comments (17)
dev/release-kotlin (1)
35-37: LGTM! Clean directory management logic.The directory cleanup and movement operations are well-structured:
- Removes the existing jniLibs directory to prevent stale artifacts
- Moves the newly generated libraries to the target location
bindings_ffi/gen_kotlin.sh (1)
21-21: LGTM! Build command is now more specificThe build command now explicitly targets the xmtpv3 package, which improves clarity and efficiency.
bindings_ffi/Cargo.toml (2)
16-16: LGTM! Good optimization of uniffi features.Moving the "cli" feature out of the main dependency is a good practice. This ensures the CLI tools are only included when actually needed for binding generation, reducing unnecessary dependencies in the main library.
31-32: LGTM! Well-structured binary configuration.The changes to the binary configuration are well thought out:
- Moving binding generation code to a dedicated
bindgendirectory improves organization- Adding
required-features = ["uniffi/cli"]ensures CLI tools are only included when needed for binding generationbindings_ffi/Makefile (1)
Line range hint
86-90: LGTM! Build process improvement.The updated command for generating Swift bindings is an improvement as it:
- Ensures the binary is built with the correct features through explicit
--features uniffi/cliflag- Follows cargo's recommended practices by using
cargo run- Provides a more reliable and self-contained build process
Cargo.toml (1)
62-63: Verify tonic version change impact on mobile platforms.The change from
^0.12.2to0.12is more restrictive. As noted in the comment, this could affect iOS/Android builds.Please ensure:
- The build works correctly on both iOS and Android platforms
- TLS functionality remains intact across all platforms
Consider documenting the mobile platform testing requirements in the project's contribution guidelines.
bindings_node/src/groups.rs (2)
78-86: LGTM! Clean implementation of From traitThe implementation correctly maps all necessary fields from
MlsGrouptoNapiGroup, providing a safer and more idiomatic way to perform the conversion.
561-567: LGTM! Improved error handling in stream callbackThe implementation now properly propagates errors through the FFI boundary to Node.js callbacks, aligning with the PR objectives. The error conversion chain is well-structured and type-safe.
Note: The use of
ThreadsafeFunctionCallMode::Blockingmight impact performance under high message volumes. Consider documenting the rationale for choosing blocking mode over non-blocking.Let's verify the usage of blocking mode in other stream implementations:
xmtp_mls/src/groups/subscriptions.rs (1)
369-372: LGTM with a note about test assertionsThe test assertions look correct, but consider addressing the unwrap-related suggestions from previous reviews to improve test robustness.
xmtp_mls/src/subscriptions.rs (2)
89-126: Well-structured error handling implementation!The new
SubscribeErrorenum is well-designed with:
- Clear and descriptive error variants
- Proper error conversion using
#[from]- Good implementation of
RetryableErrortrait for retry logic
192-217: Good error propagation in stream functions!The modifications correctly implement error handling in streams:
- Proper return type using
Result<_, SubscribeError>- Well-structured error handling in filter_group closure
- Correct stream composition with error propagation
xmtp_mls/src/groups/mod.rs (3)
230-230: LGTM: New error variants enhance error handling.The new error variants
InvalidDmMissingInboxId,MissingMetadataField, andSyncFailedToWaitare well-defined and provide clear error messages. They improve error handling by making specific failure cases more identifiable.
230-230: LGTM: Robust error handling implementation.The error handling implementation is comprehensive with proper error propagation through the RetryableError trait. The code correctly handles various error scenarios including retryable errors.
230-230: Verify the impact of makingclientfield public.The visibility change of the
clientfield frompub(crate)topubis a breaking change that exposes internal implementation details. Please ensure this change is necessary for error propagation and doesn't break encapsulation principles.bindings_ffi/src/mls.rs (3)
1175-1179: Ensure consistent error mapping toFfiSubscribeErrorThe method
process_streamed_conversation_messagenow returnsResult<FfiMessage, FfiSubscribeError>. Please verify that all errors fromself.inner.process_streamed_group_messageare correctly mapped toFfiSubscribeError, ensuring consistent error handling and that no errors are unhandled or improperly converted.
844-846: Handle possible integer conversion errorsThe conversion of
num_groups_syncedtou32includes error handling withmap_err(|_| GenericError::FailedToConvertToU32). This ensures that any potentialTryIntoconversion errors are properly managed and propagated.
1322-1325: Consistent error handling in admin list updatesThe methods
add_admin,remove_admin,add_super_admin, andremove_super_adminusemap_err(Into::into)to handle errors fromself.inner.update_admin_list. Verify that the error conversion is appropriate and that errors are correctly mapped to the expected error types.Also applies to: 1329-1332, 1336-1339, 1343-1346
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
| @@ -1799,14 +1575,16 @@ impl FfiStreamCloser { | |||
| } | |||
| } | |||
|
|
|||
| #[uniffi::export(callback_interface)] | |||
| #[uniffi::export(with_foreign)] | |||
There was a problem hiding this comment.
callback_interface was deprecated
insipx
requested review from
nplasterer
and removed request for
richardhuaaa and
nplasterer
* Callbacks now take a Result<T> * Add `on_error` to ffi Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
closes #899
rel: #1149
Summary by CodeRabbit
New Features
SubscribeErrorenum for improved subscription processes.Bug Fixes
Documentation
Chores