Skip to content

Conversation

@insipx
Copy link
Contributor

@insipx insipx commented Dec 11, 2025

Use offline resolver to order group message queries in xmtp_api_d14n without network dependency resolution

Introduce OfflineResolver alias and add D14nCombinatorExt::offline_ordered to perform ordering without network resolution; update ordered_query::OrderedQuery to support an offline mode and switch MLS group message queries to use offline ordering.

📍Where to Start

Start with the mls query changes that switch to offline ordering in mls.rs, then review the constructor and mode handling in ordered_query.rs, and finally the new combinator in combinators.rs.


Macroscope summarized a2b5821.

@insipx
Copy link
Contributor Author

insipx commented Dec 11, 2025

@claude
Copy link

claude bot commented Dec 11, 2025

Claude finished @insipx's task —— View job


Code Review - December 11, 2025, 21:06 UTC

Review Summary:

This PR introduces an offline_ordered combinator for ordering group message queries without network dependency resolution. The implementation is clean and well-structured, with only minor considerations around test coverage.

✅ Code Quality & Design

  • Well-designed abstraction: The OfflineResolver type alias and offline_ordered combinator provide a clear, type-safe API for offline ordering
  • Consistent architecture: The implementation reuses the existing Ordered builder pattern and simply switches between order() and order_offline() based on the offline flag (ordered_query.rs:46-50)
  • Type safety: The TypedNoopResolver<T> phantom type properly constrains the resolver's envelope type

🔍 Potential Issues

None identified. The implementation correctly:

  • Delegates to the existing order_offline method which handles icebox storage for orphaned envelopes (order.rs:144-153)
  • Uses TypedNoopResolver which returns empty resolved lists and preserves unresolved dependencies (dependency_resolution.rs:83-97)
  • Follows the same error handling pattern as the online resolver

⚡ Performance Considerations

Positive: This is a performance improvement - eliminates network round-trips for dependency resolution. The PR description indicates this is intentional for group message queries where dependencies will eventually arrive through the normal query flow.

🔒 Security

No security concerns. The change reduces network exposure by removing resolution queries.

🧪 Test Coverage

  • Coverage: 90.32% patch coverage (3 lines uncovered in ordered_query.rs)
  • Existing tests: The OrderedStream has comprehensive proptest coverage that exercises order_offline (ordered.rs:115-154)
  • Recommendation: Consider adding a focused test for query_group_messages using offline_ordered to verify the integration, though existing stream tests provide good coverage of the underlying mechanism

📝 Documentation

The inline documentation is clear (combinators.rs:31-33), explaining that missing envelopes are iced for future resolution. @neekolas asked about use cases - adding a brief note in the doc comment about when to use offline vs online ordering would be helpful.


@codecov
Copy link

codecov bot commented Dec 11, 2025

Codecov Report

❌ Patch coverage is 73.47561% with 174 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.79%. Comparing base (fc5cf12) to head (af240c4).
⚠️ Report is 12 commits behind head on push-pnvnmqsyqqqy.

Files with missing lines Patch % Lines
xmtp_api_d14n/src/protocol/traits/cursor_store.rs 18.42% 93 Missing ⚠️
xmtp_mls/src/cursor_store.rs 38.09% 13 Missing ⚠️
...i_d14n/src/queries/d14n/test/send_group_message.rs 0.00% 10 Missing ⚠️
..._api_d14n/src/queries/combinators/ordered_query.rs 55.00% 9 Missing ⚠️
..._api_d14n/src/protocol/utils/test/test_envelope.rs 85.18% 8 Missing ⚠️
xmtp_api_d14n/src/protocol/order.rs 95.65% 7 Missing ⚠️
xmtp_api_d14n/src/queries/stream/ordered.rs 83.33% 7 Missing ⚠️
xmtp_api_d14n/src/queries/combinators.rs 71.42% 6 Missing ⚠️
xmtp_api_d14n/src/protocol/traits.rs 0.00% 5 Missing ⚠️
xmtp_api_d14n/src/protocol/sort/causal.rs 0.00% 4 Missing ⚠️
... and 5 more
Additional details and impacted files
@@                  Coverage Diff                  @@
##           push-pnvnmqsyqqqy    #2937      +/-   ##
=====================================================
+ Coverage              73.67%   73.79%   +0.12%     
=====================================================
  Files                    402      404       +2     
  Lines                  51098    51641     +543     
=====================================================
+ Hits                   37644    38107     +463     
- Misses                 13454    13534      +80     

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

Copy link
Contributor

@insipx would be helpful to understand under which circumstances this is meant to be used

@insipx insipx force-pushed the push-pnvnmqsyqqqy branch 2 times, most recently from cabf7bd to 5a89d04 Compare December 11, 2025 20:42
@insipx
Copy link
Contributor Author

insipx commented Dec 12, 2025

@neekolas the offline_ordered does not do any network resolution for dependencies, so this brings query_group_messages in line with how stream_messages works. TBH i'm not sure it's the behavior that is needed -- it means that query_group_messages will icebox messages it does not have dependencies for rather than doing network-backoff resolution to the network

@insipx insipx force-pushed the push-pnvnmqsyqqqy branch 10 times, most recently from 70c022c to 6ab5ce5 Compare December 16, 2025 16:14
@insipx insipx closed this Jan 5, 2026
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.

3 participants