Add 1:1 messaging to Browser SDK #703
Conversation
|
Deploying xmtp-js-docs with
|
| Latest commit: |
04a8244
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://3037ef32.xmtp-js.pages.dev |
| Branch Preview URL: | https://rygine-browser-sdk-update.xmtp-js.pages.dev |
WalkthroughThis pull request introduces updates to the Changes
Possibly related PRs
Suggested reviewers
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (17)
sdks/browser-sdk/test/Utils.test.ts (1)
Line range hint
6-19: Consider adding more test cases for error scenarios.Given that this utility class is crucial for the new 1:1 messaging functionality, consider adding test cases for:
- Invalid address formats
- Network errors during inbox ID retrieval
- Edge cases like empty/null addresses
Example test case structure:
it("should handle invalid address format", async () => { const utils = new Utils(); await expect(utils.generateInboxId("invalid-address")) .rejects .toThrow("Invalid address format"); });sdks/browser-sdk/src/utils/createClient.ts (1)
17-18: Document the dbPath option in JSDoc.The new configurable dbPath option needs documentation to explain its purpose, expected format, and any restrictions.
Add JSDoc comments above the options parameter:
+ /** + * Creates an XMTP client instance + * @param accountAddress The account address + * @param options Additional client options + * @param options.dbPath Custom path for the client database. Must end with .db3 extension. + * Defaults to `xmtp-{env}-{accountAddress}.db3` + */ export const createClient = async ( accountAddress: string, options?: Omit<ClientOptions, "codecs">, ) => {sdks/browser-sdk/src/Conversations.ts (3)
31-35: Consider adding explicit return type annotation.For better type safety and documentation, consider adding an explicit return type to the method.
- async getDmByInboxId(inboxId: string) { + async getDmByInboxId(inboxId: string): Promise<Conversation> {
48-54: Add return type and JSDoc documentation.For better type safety and documentation:
- Add explicit return type
- Add JSDoc to document the options parameter and its effect
+ /** + * Lists all group conversations. + * @param options Optional parameters for filtering and pagination + * @returns Promise<Conversation[]> Array of group conversations + */ - async listGroups( + async listGroups( options?: Omit<SafeListConversationsOptions, "conversation_type">, - ) { + ): Promise<Conversation[]> {
56-62: Add return type and JSDoc documentation.For better type safety and documentation:
- Add explicit return type
- Add JSDoc to document the options parameter and its effect
+ /** + * Lists all direct message conversations. + * @param options Optional parameters for filtering and pagination + * @returns Promise<Conversation[]> Array of DM conversations + */ - async listDms( + async listDms( options?: Omit<SafeListConversationsOptions, "conversation_type">, - ) { + ): Promise<Conversation[]> {sdks/browser-sdk/package.json (1)
68-68: Consider pinning the wasm-bindings version.Given that this is a critical dependency for the new messaging functionality and we're dealing with pre-1.0 versions, consider pinning the exact version instead of using the caret range to prevent unexpected updates.
- "@xmtp/wasm-bindings": "^0.0.2", + "@xmtp/wasm-bindings": "0.0.2",sdks/browser-sdk/src/WorkerConversations.ts (2)
46-53: Consider adding input validation and explicit return type.While the implementation follows class conventions, consider these improvements:
- Add input validation for
inboxIdparameter- Add explicit return type annotation:
WorkerConversation | undefined- getDmByInboxId(inboxId: string) { + getDmByInboxId(inboxId: string): WorkerConversation | undefined { + if (!inboxId?.trim()) { + throw new Error("inboxId is required"); + } try {
62-69: Add JSDoc documentation for better developer experience.Consider adding documentation to explain the available options and return type:
+/** + * Lists all group conversations. + * @param options - Filtering options for listing groups (excluding conversation_type) + * @returns Promise<WorkerConversation[]> - Array of group conversations + */ async listGroups( options?: Omit<SafeListConversationsOptions, "conversation_type">, ): Promise<WorkerConversation[]> {sdks/browser-sdk/src/WorkerConversation.ts (1)
170-172: LGTM! Consider adding JSDoc comments.The implementation is clean and follows the established patterns in the codebase. The method serves as a proper wrapper for the Wasm binding's
dm_peer_inbox_id()method, which aligns well with the PR's objective of enhancing direct messaging capabilities.Consider adding JSDoc comments to document the method's purpose and return type:
+ /** + * Gets the inbox ID of the peer in a direct message conversation. + * @returns {Promise<string>} The inbox ID of the direct message peer. + */ dmPeerInboxId() { return this.#group.dm_peer_inbox_id(); }sdks/browser-sdk/src/Client.ts (1)
126-129: Consider adding JSDoc to document the optional parameter.The implementation correctly handles the optional parameter with a sensible default value. Consider adding JSDoc to document the parameter's purpose and default behavior.
+/** + * Retrieves the current inbox state + * @param refreshFromNetwork - When true, forces a refresh from the network. Defaults to false. + * @returns The current inbox state + */ async inboxState(refreshFromNetwork?: boolean) { return this.sendMessage("inboxState", { refreshFromNetwork: refreshFromNetwork ?? false, }); }sdks/browser-sdk/src/Conversation.ts (1)
286-290: Add documentation and type information.The implementation looks good and aligns with the class's patterns. However, consider these improvements:
Add JSDoc documentation and TypeScript return type:
+ /** + * Gets the inbox ID of the peer in a direct message conversation. + * @returns Promise<string> The inbox ID of the peer + * @throws {Error} If the conversation is not a direct message + */ - async dmPeerInboxId() { + async dmPeerInboxId(): Promise<string> { return this.#client.sendMessage("getDmPeerInboxId", { id: this.#id, }); }sdks/browser-sdk/test/Client.test.ts (1)
199-236: Consider adding cleanup for consent states.While the test effectively verifies consent state management, consider adding cleanup to reset consent states after the test to prevent potential side effects in other tests running concurrently.
Add cleanup in an
afterEachor at the end of the test:await client2.setConsentStates([ { entityType: WasmConsentEntityType.GroupId, entity: group2!.id, state: WasmConsentState.Unknown, }, ]);sdks/browser-sdk/test/Conversations.test.ts (1)
77-140: Add test cases for error conditions and edge cases.The current test suite focuses on positive cases. Consider adding tests for:
- Creating a DM with an invalid address
- Creating a DM with self
- Creating multiple DMs with the same peer
- Edge cases around consent states
Would you like me to help generate these additional test cases?
sdks/browser-sdk/test/Conversation.test.ts (1)
378-411: Enhance consent state test coverage.While the test effectively verifies basic consent state transitions, consider adding:
- Negative test cases (e.g., denied consent)
- Error cases (e.g., invalid state transitions)
- Additional state transitions
- Verification from both participants' perspectives
- Tests for consent revocation
Here's a suggested test case structure:
it("should handle all consent states and transitions", async () => { // Setup (similar to current test) // Test initial state expect(await dmConvo.consentState()).toBe(WasmConsentState.Unknown); expect(await dmConvo2.consentState()).toBe(WasmConsentState.Unknown); // Test denied consent await dmConvo.denyConsent(); expect(await dmConvo.consentState()).toBe(WasmConsentState.Denied); // Test consent revocation await dmConvo.send("gm!"); expect(await dmConvo.consentState()).toBe(WasmConsentState.Allowed); await dmConvo.revokeConsent(); expect(await dmConvo.consentState()).toBe(WasmConsentState.Unknown); // Test error cases await expect(dmConvo.revokeConsent()).rejects.toThrow(); });sdks/node-sdk/test/Conversations.test.ts (2)
87-88: Consider reordering timestamp assertions for better readability.The assertions for timestamp properties should follow a logical order. Consider reordering them to check
createdAtbeforecreatedAtNssincecreatedAtis the human-readable version.- expect(group.createdAtNs).toBeDefined(); - expect(group.createdAt).toBeDefined(); + expect(group.createdAt).toBeDefined(); + expect(group.createdAtNs).toBeDefined();
Line range hint
87-170: Consider splitting the "should create a dm group" test for better readability.This test is quite long and verifies multiple aspects of DM creation. Consider splitting it into smaller, focused tests:
- Basic DM creation and properties
- DM permissions verification
- DM member verification
- DM metadata verification
Example structure:
describe('DM Creation', () => { it('should create a basic DM with correct properties', async () => { // Test basic properties (id, timestamps, etc.) }); it('should set correct permissions for DMs', async () => { // Test permissions }); it('should properly set up DM members', async () => { // Test member setup }); it('should set correct metadata for DMs', async () => { // Test metadata }); });sdks/browser-sdk/src/workers/client.ts (1)
251-261: Consider adding input validation for accountAddressThe implementation of
newDmhandler is correct, but could benefit from explicit validation of theaccountAddressparameter before callingconversations.newDm.Consider adding validation:
case "newDm": { + if (!data.accountAddress || typeof data.accountAddress !== 'string') { + postMessageError({ + id, + action, + error: "Invalid account address", + }); + break; + } const conversation = await client.conversations.newDm( data.accountAddress, ); postMessage({
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (17)
sdks/browser-sdk/package.json(1 hunks)sdks/browser-sdk/src/Client.ts(2 hunks)sdks/browser-sdk/src/Conversation.ts(1 hunks)sdks/browser-sdk/src/Conversations.ts(3 hunks)sdks/browser-sdk/src/WorkerClient.ts(1 hunks)sdks/browser-sdk/src/WorkerConversation.ts(1 hunks)sdks/browser-sdk/src/WorkerConversations.ts(1 hunks)sdks/browser-sdk/src/types/clientEvents.ts(5 hunks)sdks/browser-sdk/src/utils/conversions.ts(5 hunks)sdks/browser-sdk/src/utils/createClient.ts(1 hunks)sdks/browser-sdk/src/workers/client.ts(6 hunks)sdks/browser-sdk/test/Client.test.ts(3 hunks)sdks/browser-sdk/test/Conversation.test.ts(2 hunks)sdks/browser-sdk/test/Conversations.test.ts(2 hunks)sdks/browser-sdk/test/Utils.test.ts(1 hunks)sdks/browser-sdk/test/helpers.ts(3 hunks)sdks/node-sdk/test/Conversations.test.ts(1 hunks)
🔇 Additional comments (28)
sdks/browser-sdk/test/Utils.test.ts (1)
5-5: LGTM! Good performance optimization.
The change to use describe.concurrent is a good optimization that will help improve test execution time by running tests in parallel.
sdks/browser-sdk/src/Conversations.ts (1)
Line range hint 31-79: Implementation aligns well with PR objectives.
The new direct messaging capabilities are well-implemented and follow consistent patterns throughout the class. The code is clean and maintainable. The suggested improvements around type safety, documentation, and validation will further enhance the robustness of the implementation.
sdks/browser-sdk/package.json (1)
68-68: Verify the wasm-bindings version and its compatibility.
The dependency update to @xmtp/wasm-bindings@^0.0.2 is required to support the new 1:1 messaging functionality. However, this change warrants careful consideration:
- Being a pre-1.0 version, changes between 0.0.1 and 0.0.2 might include breaking changes
- The caret (^) allows patch updates which might introduce instability
✅ Verification successful
Version bump to wasm-bindings@^0.0.2 is necessary and safe
The version update is required to support new types (WasmConversationType, WasmDirection, WasmGroupMembershipState) used in the conversation listing options. Based on the evidence:
- 0.0.2 is currently the latest version on npm
- The types are actively used in
sdks/browser-sdk/src/utils/conversions.ts - The change from 0.0.1 to 0.0.2 appears to be additive, introducing new types without breaking existing functionality
- The dependency was recently introduced, starting with 0.0.1
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the wasm-bindings version and its usage
# Test 1: Check if this is the latest version
echo "Latest version from npm:"
npm view @xmtp/wasm-bindings version
# Test 2: Look for direct usage of wasm-bindings
echo -e "\nFiles importing wasm-bindings:"
rg -l '@xmtp/wasm-bindings'
# Test 3: Check for any migration guides or breaking changes
echo -e "\nChecking for migration documentation:"
fd -g 'MIGRATION*.md' -g 'CHANGELOG*.md' -g 'BREAKING*.md'
Length of output: 1492
Script:
#!/bin/bash
# Let's check the actual usage of wasm-bindings types and functions to understand the impact
# Look for imports and usage patterns
echo "Checking wasm-bindings usage patterns:"
rg -A 3 "from '@xmtp/wasm-bindings'" sdks/browser-sdk/src/
# Check for any changes in test files that might indicate breaking changes
echo -e "\nChecking test file changes:"
rg -A 3 "WasmConversationType|WasmDirection|WasmGroupMembershipState" sdks/browser-sdk/
# Check package.json history to see when this dependency was introduced
echo -e "\nChecking package.json history:"
git log -p sdks/browser-sdk/package.json | grep -A 3 -B 3 "@xmtp/wasm-bindings"
# Look for any documentation about version requirements
echo -e "\nChecking for version requirement documentation:"
rg "wasm-bindings.*0\.0\.[12]" sdks/browser-sdk/
Length of output: 2081
sdks/browser-sdk/test/helpers.ts (3)
7-7: LGTM: Clean import of UUID generator
The specific import of v4 from uuid is appropriate and follows best practices for tree-shaking.
49-49: Verify test database handling and update documentation
The switch from using account address to UUID for test database paths is a good change for test isolation, but requires verification of test cleanup and documentation updates.
Consider adding a comment explaining the UUID-based database path strategy for future maintainers.
return Client.create(user.account.address, {
...opts,
- dbPath: `./test-${user.uuid}.db3`,
+ // Use UUID for test database path to prevent conflicts when running tests
+ // with the same wallet address in different states
+ dbPath: `./test-${user.uuid}.db3`,
});✅ Verification successful
Database cleanup is properly handled, but documentation could be improved
The switch from account address to UUID for test database paths is safe:
- Test database cleanup is handled in
node-sdk/vitest.setup.tswhich removes all test database files matchingtest-*.db3*pattern - The change aligns with the existing database path patterns in both browser and node SDKs
- No documentation updates are needed as there are no explicit references to database paths in markdown files
Consider adding the suggested comment to explain the UUID-based strategy for better maintainability.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for database cleanup in test files
rg "afterEach|afterAll" --type ts
# Check for documentation that might need updating
fd -e md -e txt . -x grep -l "test-.*\.db3"
Length of output: 243
Script:
#!/bin/bash
# Let's check for database file handling in test files
rg "\.db3" --type ts -B 2 -A 2
# Check for test cleanup or database removal patterns
rg "unlink.*\.db3|fs.*unlink.*\.db3|rimraf" --type ts -B 2 -A 2
# Look for database initialization patterns
rg "Client\.create.*dbPath" --type ts -B 2 -A 2
Length of output: 1727
25-25: LGTM: UUID addition provides better test isolation
The UUID addition enables better test isolation by allowing multiple test instances for the same wallet address.
✅ Verification successful
Let me gather more context about the usage of UUID in the test helpers and related files.
Let me check one more thing to understand the complete test setup context.
Based on the gathered evidence, I can now provide the final response:
LGTM: UUID is correctly used for test database isolation
The UUID addition is properly implemented and serves its intended purpose:
- Used consistently in both browser-sdk and node-sdk test suites
- Ensures unique database paths for each test instance (
test-${user.uuid}.db3) - Prevents test interference when running concurrent tests with the same wallet address
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that the UUID is used consistently across the test suite
# and not just for the database path
# Look for other uses of the user.uuid property
rg "user\.uuid" --type ts
Length of output: 415
Script:
#!/bin/bash
# Check the implementation of createUser and related test utilities
ast-grep --pattern 'function createUser'
# Look for the User type definition
rg "type User" -A 5 --type ts
# Check for database path usage patterns
rg "dbPath.*test-.*\.db3" --type ts
Length of output: 1176
Script:
#!/bin/bash
# Check the complete implementation context of test files
rg "beforeEach|afterEach|describe.*Client" -A 5 --type ts
# Look for database cleanup patterns
rg "unlink.*\.db3" --type ts
Length of output: 18247
sdks/browser-sdk/src/WorkerConversations.ts (1)
Line range hint 1-91: Verify WASM bindings compatibility.
Let's ensure all WASM methods being used are properly exported from the bindings package.
sdks/browser-sdk/src/Client.ts (1)
98-100: LGTM! Implementation follows established patterns.
The new method follows the same pattern as other signature text methods in the class and maintains consistency in the codebase.
sdks/browser-sdk/test/Client.test.ts (4)
35-41: LGTM! Well-structured test for messaging capability.
The test properly verifies the canMessage functionality with appropriate address case handling.
78-113: LGTM! Comprehensive test for adding wallet association.
The test thoroughly verifies the wallet association workflow, including signature generation, verification, and state checks.
163-197: LGTM! Well-structured test for installation management.
The test effectively verifies the installation revocation workflow with proper state checks before and after the operation.
1-16: Verify concurrent test execution safety.
The switch to describe.concurrent could improve test execution time, but ensure that the tests are truly independent and don't share mutable state or resources that could cause race conditions.
✅ Verification successful
Concurrent test execution is safe
The test file shows proper test isolation:
- Each test creates fresh user and client instances using
createUser()andcreateClient()/createRegisteredClient() - No shared mutable state exists between tests
- Variables are scoped within individual test blocks
- Each test operates on its own independent resources
The use of describe.concurrent is appropriate and will help improve test execution performance.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential shared state or resources across tests
rg -A 3 "let |const " "sdks/browser-sdk/test/Client.test.ts" | grep -E "^[^-]" | grep -v "expect"
Length of output: 3422
sdks/browser-sdk/src/types/clientEvents.ts (3)
82-82: LGTM: Action type renamed for clarity.
The rename from "applySignaturesRequests" to "applySignatures" improves clarity while maintaining the same functionality.
434-441: LGTM: DM peer inbox ID action properly typed.
The getDmPeerInboxId action is well-defined and follows the established pattern for action types. This helper action complements the DM functionality by providing a way to retrieve the peer's inbox ID.
167-174: LGTM: New DM actions properly typed.
The new actions for DM functionality (getDmByInboxId, getGroups, getDms, newDm) are well-structured and consistent with existing patterns. The type definitions appropriately handle optional parameters and return types.
Let's verify the consistency of these new action types with their implementations:
Also applies to: 183-198, 208-215
✅ Verification successful
Based on the verification results, I can now generate the final response since I have sufficient information about the implementations of the new DM-related actions in both browser and node SDKs.
Verified: DM action types match implementations
The type definitions for the new DM actions are correctly implemented across both SDKs:
getDmByInboxId: ReturnsSafeConversation | undefinedas typed, with implementations in both SDKs properly handling the return typelistGroups/listDms: Return array ofSafeConversationwith correct options type excluding conversation_typenewDm: Returns singleSafeConversationwith proper accountAddress parameter
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the implementations match these type definitions
# Test: Search for the implementation of these new methods
ast-grep --pattern 'class Conversations {
$$$
getDmByInboxId($$$) {
$$$
}
$$$
}'
ast-grep --pattern 'class Conversations {
$$$
listGroups($$$) {
$$$
}
$$$
}'
ast-grep --pattern 'class Conversations {
$$$
listDms($$$) {
$$$
}
$$$
}'
ast-grep --pattern 'class Conversations {
$$$
newDm($$$) {
$$$
}
$$$
}'
Length of output: 68799
sdks/browser-sdk/test/Conversations.test.ts (3)
12-15: LGTM! Good addition of granular checks.
The additional checks for DMs and groups provide better test coverage and clarity about the initial state.
73-74: LGTM! Good verification of conversation types.
The additional checks ensure that conversations are properly categorized as either DMs or groups after syncing.
1-8: Verify test independence for concurrent execution.
The transition to describe.concurrent could lead to race conditions if tests share resources or state. Please ensure that:
- Each test uses unique users and clients
- No test depends on the state of another test
- No shared resources are modified across tests
✅ Verification successful
Tests are properly isolated with unique users and clients per test case
The verification shows that each test case creates its own unique users and clients using createUser() and createRegisteredClient(). There is no shared state between tests as:
- Each test creates fresh user instances (user1, user2, etc.)
- Each test initializes new client instances from these unique users
- No global/shared variables are used across test cases
- Resources (conversations, groups) are created within test scope
This confirms that the transition to describe.concurrent is safe as all tests are properly isolated.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potential shared state or resources across tests
rg -A 5 "let |const " "sdks/browser-sdk/test/Conversations.test.ts" | grep -E "user|client"
Length of output: 3795
sdks/browser-sdk/src/utils/conversions.ts (3)
15-15: LGTM: New type imports align with PR objectives.
The added types from wasm-bindings support the new messaging features and conversation listing capabilities.
Also applies to: 17-18
147-147: LGTM: Message listing direction support properly implemented.
The addition of the direction property to SafeListMessagesOptions and its corresponding conversion functions is implemented correctly and symmetrically.
Also applies to: 157-157, 171-171
175-176: Verify usage of new conversation listing options.
Let's ensure the new filtering options are properly handled throughout the codebase.
✅ Verification successful
New conversation listing options are properly integrated
The verification shows that the new options (allowed_states and conversation_type) are:
- Correctly defined in the WASM interface types
- Properly integrated into the conversion functions between safe and WASM types
- Consistently used across the SDK in list operations (list, listGroups, listDms)
- Properly handled in the worker conversations implementation
The changes are well-structured and maintain consistency with the existing codebase patterns. No issues found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage of conversation listing options to ensure compatibility
# Test 1: Find direct usages of ListConversationsOptions
echo "Searching for ListConversationsOptions usage..."
rg -l "ListConversationsOptions" --type typescript
# Test 2: Find specific usage of the new properties
echo "Searching for allowed_states usage..."
rg "allowed_states.*WasmGroupMembershipState" --type typescript
echo "Searching for conversation_type usage..."
rg "conversation_type.*WasmConversationType" --type typescript
Length of output: 567
Script:
#!/bin/bash
# Let's try without typescript file type restriction
echo "Searching for ListConversationsOptions usage..."
rg "ListConversationsOptions"
echo -e "\nSearching for allowed_states usage..."
rg "allowed_states.*WasmGroupMembershipState"
echo -e "\nSearching for conversation_type usage..."
rg "conversation_type.*WasmConversationType"
# Let's also check the interface definition
echo -e "\nChecking ListConversationsOptions interface definition..."
rg -A 10 "interface ListConversationsOptions"
# Let's also look for any tests related to these options
echo -e "\nChecking for related test files..."
rg "describe.*conversation.*list" -A 5
Length of output: 3626
Script:
#!/bin/bash
# Let's check the conversion functions and their usage
echo "Checking conversion functions implementation..."
rg -A 10 "fromSafeListConversationsOptions"
echo -e "\nChecking for any test files related to conversations..."
fd "conversation.*test" --type f
echo -e "\nChecking for any usage of these options in tests..."
rg "allowed_states|conversation_type" --glob "*test*"
# Let's also check if there are any WASM type definitions
echo -e "\nChecking WASM type definitions..."
rg "WasmGroupMembershipState|WasmConversationType" -A 5
Length of output: 6033
sdks/browser-sdk/test/Conversation.test.ts (1)
1-1: Verify concurrent test execution safety.
The test suite has been configured to run concurrently with describe.concurrent. While this can improve test execution speed, please ensure:
- Tests are truly independent with no shared state
- All test cleanup is properly handled
- Other test files in the suite follow the same pattern for consistency
Also applies to: 3-3, 11-11
✅ Verification successful
Concurrent test execution pattern is consistently used across test files
The verification shows that describe.concurrent is consistently used across all test files in the browser SDK:
Utils.test.tsConversation.test.tsConversations.test.tsClient.test.ts
This indicates a deliberate pattern rather than an isolated change, suggesting the test suite has been designed to handle concurrent execution safely.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if other test files also use concurrent execution
echo "Checking test files for concurrent execution pattern..."
rg "describe\.concurrent" "sdks/browser-sdk/test/"
Length of output: 495
sdks/node-sdk/test/Conversations.test.ts (2)
Line range hint 1-600: Excellent test coverage for DM functionality!
The test suite comprehensively covers all the new DM-related functionality mentioned in the PR objectives, including:
- Creating DMs with
newDm - Listing DMs with
listDms - Retrieving DMs by inbox ID with
getDmByInboxId - Streaming DM conversations and messages
- Proper verification of DM-specific properties and metadata
The tests include both positive and negative cases, and properly clean up resources.
Line range hint 1-600: Verify test coverage in Browser SDK.
While these tests thoroughly cover the new 1:1 messaging functionality, they are part of the Node SDK test suite. Since the PR objectives specifically mention adding this functionality to the Browser SDK, please verify that similar test coverage exists in the Browser SDK test suite.
sdks/browser-sdk/src/workers/client.ts (4)
102-110: LGTM: New signature handler follows established patterns
The implementation of getRevokeInstallationsSignatureText handler is consistent with other signature-related handlers and properly maintains the worker's error handling patterns.
119-120: LGTM: Method rename improves clarity
The rename from applySignaturesRequests to applySignatures aligns with the PR objectives and improves method naming clarity without changing functionality.
289-297: LGTM: DM retrieval handler properly handles null case
The implementation of getDmByInboxId handler correctly handles the case when no conversation is found and maintains type safety through toSafeConversation.
215-238: LGTM: New DM listing handlers maintain consistency
The implementation of getGroups and getDms handlers follows the established patterns for conversation listing and maintains proper type safety through the toSafeConversation conversion.
Let's verify the consistency of conversation type handling:
✅ Verification successful
Consistent conversation type handling confirmed
The verification shows that toSafeConversation is consistently used across all conversation-related handlers in the worker client, including the new getGroups and getDms cases. The type definitions in conversions.ts properly define the SafeConversation type, and all conversation objects are properly converted using the same utility function.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent handling of conversation types across the codebase
# Look for any potential inconsistencies in conversation type handling
# Search for conversation type definitions and usage
rg -A 2 'type.*Conversation.*='
# Search for conversation conversion patterns
rg -A 2 'toSafeConversation'
Length of output: 2758
| const dbPath = | ||
| options?.dbPath ?? `xmtp-${options?.env ?? "dev"}-${accountAddress}.db3`; |
There was a problem hiding this comment.
Add validation for user-provided dbPath.
The dbPath option should be validated to ensure it meets security requirements and has the correct extension.
Add validation before using the dbPath:
const dbPath =
options?.dbPath ?? `xmtp-${options?.env ?? "dev"}-${accountAddress}.db3`;
+ if (!dbPath.endsWith('.db3')) {
+ throw new Error('Database path must end with .db3 extension');
+ }
+ if (dbPath.includes('../') || dbPath.includes('..\\')) {
+ throw new Error('Database path cannot contain parent directory references');
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const dbPath = | |
| options?.dbPath ?? `xmtp-${options?.env ?? "dev"}-${accountAddress}.db3`; | |
| const dbPath = | |
| options?.dbPath ?? `xmtp-${options?.env ?? "dev"}-${accountAddress}.db3`; | |
| if (!dbPath.endsWith('.db3')) { | |
| throw new Error('Database path must end with .db3 extension'); | |
| } | |
| if (dbPath.includes('../') || dbPath.includes('..\\')) { | |
| throw new Error('Database path cannot contain parent directory references'); | |
| } |
| async newDm(accountAddress: string) { | ||
| const conversation = await this.#client.sendMessage("newDm", { | ||
| accountAddress, | ||
| }); | ||
|
|
||
| return new Conversation(this.#client, conversation.id, conversation); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add return type, input validation, and documentation.
Consider the following improvements:
- Add explicit return type
- Validate the accountAddress parameter
- Add JSDoc documentation
+ /**
+ * Creates a new direct message conversation with the specified account.
+ * @param accountAddress The Ethereum address of the account to start a DM with
+ * @returns Promise<Conversation> The newly created DM conversation
+ * @throws {Error} If the account address is invalid
+ */
- async newDm(accountAddress: string) {
+ async newDm(accountAddress: string): Promise<Conversation> {
+ if (!accountAddress || !/^0x[a-fA-F0-9]{40}$/.test(accountAddress)) {
+ throw new Error('Invalid Ethereum address');
+ }
+
const conversation = await this.#client.sendMessage("newDm", {
accountAddress,
});
return new Conversation(this.#client, conversation.id, conversation);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async newDm(accountAddress: string) { | |
| const conversation = await this.#client.sendMessage("newDm", { | |
| accountAddress, | |
| }); | |
| return new Conversation(this.#client, conversation.id, conversation); | |
| } | |
| /** | |
| * Creates a new direct message conversation with the specified account. | |
| * @param accountAddress The Ethereum address of the account to start a DM with | |
| * @returns Promise<Conversation> The newly created DM conversation | |
| * @throws {Error} If the account address is invalid | |
| */ | |
| async newDm(accountAddress: string): Promise<Conversation> { | |
| if (!accountAddress || !/^0x[a-fA-F0-9]{40}$/.test(accountAddress)) { | |
| throw new Error('Invalid Ethereum address'); | |
| } | |
| const conversation = await this.#client.sendMessage("newDm", { | |
| accountAddress, | |
| }); | |
| return new Conversation(this.#client, conversation.id, conversation); | |
| } |
| async newDm(accountAddress: string) { | ||
| const group = await this.#conversations.create_dm(accountAddress); | ||
| return new WorkerConversation(this.#client, group); | ||
| } |
There was a problem hiding this comment.
Add error handling and input validation.
The implementation should be more robust with these improvements:
+ /**
+ * Creates a new DM conversation with the specified account.
+ * @param accountAddress - The Ethereum address of the account to start a DM with
+ * @returns Promise<WorkerConversation> - The newly created DM conversation
+ * @throws Error if the account address is invalid or DM creation fails
+ */
- async newDm(accountAddress: string) {
+ async newDm(accountAddress: string): Promise<WorkerConversation> {
+ if (!accountAddress?.trim()) {
+ throw new Error("Account address is required");
+ }
+ try {
const group = await this.#conversations.create_dm(accountAddress);
return new WorkerConversation(this.#client, group);
+ } catch (error) {
+ throw new Error(`Failed to create DM: ${error.message}`);
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async newDm(accountAddress: string) { | |
| const group = await this.#conversations.create_dm(accountAddress); | |
| return new WorkerConversation(this.#client, group); | |
| } | |
| /** | |
| * Creates a new DM conversation with the specified account. | |
| * @param accountAddress - The Ethereum address of the account to start a DM with | |
| * @returns Promise<WorkerConversation> - The newly created DM conversation | |
| * @throws Error if the account address is invalid or DM creation fails | |
| */ | |
| async newDm(accountAddress: string): Promise<WorkerConversation> { | |
| if (!accountAddress?.trim()) { | |
| throw new Error("Account address is required"); | |
| } | |
| try { | |
| const group = await this.#conversations.create_dm(accountAddress); | |
| return new WorkerConversation(this.#client, group); | |
| } catch (error) { | |
| throw new Error(`Failed to create DM: ${error.message}`); | |
| } | |
| } |
| async listDms( | ||
| options?: Omit<SafeListConversationsOptions, "conversation_type">, | ||
| ) { | ||
| const groups = (await this.#conversations.list_dms( | ||
| options ? fromSafeListConversationsOptions(options) : undefined, | ||
| )) as WasmGroup[]; | ||
| return groups.map((group) => new WorkerConversation(this.#client, group)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring to reduce code duplication with listGroups.
The implementation is nearly identical to listGroups. Consider extracting the common logic:
+ private async listByType(
+ type: 'groups' | 'dms',
+ options?: Omit<SafeListConversationsOptions, "conversation_type">,
+ ): Promise<WorkerConversation[]> {
+ const method = type === 'groups' ? 'list_groups' : 'list_dms';
+ const groups = (await this.#conversations[method](
+ options ? fromSafeListConversationsOptions(options) : undefined,
+ )) as WasmGroup[];
+ return groups.map((group) => new WorkerConversation(this.#client, group));
+ }
+ /**
+ * Lists all DM conversations.
+ * @param options - Filtering options for listing DMs (excluding conversation_type)
+ * @returns Promise<WorkerConversation[]> - Array of DM conversations
+ */
async listDms(
options?: Omit<SafeListConversationsOptions, "conversation_type">,
): Promise<WorkerConversation[]> {
- const groups = (await this.#conversations.list_dms(
- options ? fromSafeListConversationsOptions(options) : undefined,
- )) as WasmGroup[];
- return groups.map((group) => new WorkerConversation(this.#client, group));
+ return this.listByType('dms', options);
}Committable suggestion skipped: line range outside the PR's diff.
| async applySignatures() { | ||
| return this.#client.applySignatureRequests(); | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Method naming inconsistency found across SDKs
The verification reveals that applySignatureRequests is still being used in the Node.js SDK's Client implementation while the Browser SDK has partially migrated to applySignatures. This creates an inconsistency between SDKs:
sdks/node-sdk/src/Client.ts: UsesapplySignatureRequestssdks/browser-sdk/src/WorkerClient.ts: CallsapplySignatureRequestsinternally- Both SDKs expose
applySignaturesas the public API (based on test files)
The change should be applied consistently across all SDK implementations to maintain API parity.
🔗 Analysis chain
Breaking Change: Method rename requires attention.
The renaming of applySignaturesRequests to applySignatures is a breaking change that could affect existing consumers. Additionally, there's an inconsistency between the public method name applySignatures and the internal call to applySignatureRequests.
Let's verify the impact and consistency:
Consider either:
- Updating the internal call to match the new method name for consistency
- Adding a code comment explaining why the internal method name differs
Here's the suggested fix for consistency:
async applySignatures() {
- return this.#client.applySignatureRequests();
+ return this.#client.applySignatures();
}🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to the old method name and verify consistency
# Test 1: Check for any remaining references to old method name
echo "Checking for references to 'applySignaturesRequests'..."
rg -i "applySignaturesRequests"
# Test 2: Check for all references to both old and new internal method name
echo "Checking for references to internal method 'applySignatureRequests'..."
rg "applySignatureRequests"
# Test 3: Check for all references to new method name
echo "Checking for references to new method 'applySignatures'..."
rg "applySignatures"
Length of output: 1735
| async dmPeerInboxId() { | ||
| return this.#client.sendMessage("getDmPeerInboxId", { | ||
| id: this.#id, | ||
| }); | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Handler implementation verified but needs async support
The client worker implements the "getDmPeerInboxId" case handler, but there's a mismatch in the implementation:
- The
Conversation.tscallssendMessagewith an async expectation - The worker's handler calls
group.dmPeerInboxId()synchronously withoutawait
This could lead to timing issues in message handling.
Locations needing attention:
sdks/browser-sdk/src/workers/client.ts: ThegetDmPeerInboxIdcase handler should be marked as async and await the result
🔗 Analysis chain
Verify client message handler implementation.
Let's ensure the client properly handles the "getDmPeerInboxId" action.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the client implements the getDmPeerInboxId message handler
# Test: Search for getDmPeerInboxId handler implementation
rg -A 5 "getDmPeerInboxId.*case"
Length of output: 34
Script:
#!/bin/bash
# Let's try a broader search for getDmPeerInboxId implementation
rg -i "getDmPeerInboxId"
# Also search for message handler patterns in client implementation
rg -A 10 "sendMessage.*case|handleMessage.*case|onMessage.*case"
# Look for client class implementation
ast-grep --pattern 'class $_ {
$$$
sendMessage($$$) {
$$$
}
$$$
}'
Length of output: 416
Script:
#!/bin/bash
# Let's check the implementation in the client worker
cat sdks/browser-sdk/src/workers/client.ts
Length of output: 18834
| it("should revoke a wallet association from the client", async () => { | ||
| const user = createUser(); | ||
| const user2 = createUser(); | ||
| const client = await createRegisteredClient(user); | ||
| const signatureText = await client.getAddWalletSignatureText( | ||
| user2.account.address, | ||
| ); | ||
| expect(signatureText).toBeDefined(); | ||
|
|
||
| // sign message | ||
| const signature = await user.wallet.signMessage({ | ||
| message: signatureText!, | ||
| }); | ||
| const signature2 = await user2.wallet.signMessage({ | ||
| message: signatureText!, | ||
| }); | ||
|
|
||
| await client.addSignature( | ||
| WasmSignatureRequestType.AddWallet, | ||
| toBytes(signature), | ||
| ); | ||
| await client.addSignature( | ||
| WasmSignatureRequestType.AddWallet, | ||
| toBytes(signature2), | ||
| ); | ||
| await client.applySignatures(); | ||
|
|
||
| const signatureText2 = await client.getRevokeWalletSignatureText( | ||
| user2.account.address, | ||
| ); | ||
| expect(signatureText2).toBeDefined(); | ||
|
|
||
| // sign message | ||
| const signature3 = await user.wallet.signMessage({ | ||
| message: signatureText2!, | ||
| }); | ||
|
|
||
| await client.addSignature( | ||
| WasmSignatureRequestType.RevokeWallet, | ||
| toBytes(signature3), | ||
| ); | ||
| await client.applySignatures(); | ||
| const inboxState = await client.inboxState(); | ||
| expect(inboxState.accountAddresses).toEqual([ | ||
| user.account.address.toLowerCase(), | ||
| ]); | ||
| expect(inboxState.recoveryAddress).toBe(user.account.address.toLowerCase()); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider reducing code duplication in wallet association tests.
The setup code for adding a wallet association is duplicated between this test and the previous one.
Consider extracting the common setup into a helper function:
async function setupWalletAssociation(client: Client, user: User, user2: User) {
const signatureText = await client.getAddWalletSignatureText(
user2.account.address,
);
const signature = await user.wallet.signMessage({
message: signatureText!,
});
const signature2 = await user2.wallet.signMessage({
message: signatureText!,
});
await client.addSignature(
WasmSignatureRequestType.AddWallet,
toBytes(signature),
);
await client.addSignature(
WasmSignatureRequestType.AddWallet,
toBytes(signature2),
);
await client.applySignatures();
return signatureText;
}| it("should create a dm group", async () => { | ||
| const user1 = createUser(); | ||
| const user2 = createUser(); | ||
| const client1 = await createRegisteredClient(user1); | ||
| const client2 = await createRegisteredClient(user2); | ||
| const group = await client1.conversations.newDm(user2.account.address); | ||
| expect(group).toBeDefined(); | ||
| expect(group.id).toBeDefined(); | ||
| expect(group.createdAtNs).toBeDefined(); | ||
| expect(group.createdAt).toBeDefined(); | ||
| expect(group.isActive).toBe(true); | ||
| expect(group.name).toBe(""); | ||
| expect(group.permissions?.policyType).toBe( | ||
| WasmGroupPermissionsOptions.CustomPolicy, | ||
| ); | ||
| expect(group.permissions?.policySet).toEqual({ | ||
| addAdminPolicy: 1, | ||
| addMemberPolicy: 1, | ||
| removeAdminPolicy: 1, | ||
| removeMemberPolicy: 1, | ||
| updateGroupDescriptionPolicy: 0, | ||
| updateGroupImageUrlSquarePolicy: 0, | ||
| updateGroupNamePolicy: 0, | ||
| updateGroupPinnedFrameUrlPolicy: 0, | ||
| }); | ||
| expect(group.addedByInboxId).toBe(client1.inboxId); | ||
| expect((await group.messages()).length).toBe(1); | ||
| const members = await group.members(); | ||
| expect(members.length).toBe(2); | ||
| const memberInboxIds = members.map((member) => member.inboxId); | ||
| expect(memberInboxIds).toContain(client1.inboxId); | ||
| expect(memberInboxIds).toContain(client2.inboxId); | ||
| expect(group.metadata?.conversationType).toBe("dm"); | ||
| expect(group.metadata?.creatorInboxId).toBe(client1.inboxId); | ||
|
|
||
| expect(await group.consentState()).toBe(WasmConsentState.Allowed); | ||
|
|
||
| const group1 = await client1.conversations.list(); | ||
| expect(group1.length).toBe(1); | ||
| expect(group1[0].id).toBe(group.id); | ||
| expect(await group1[0].dmPeerInboxId()).toBe(client2.inboxId); | ||
|
|
||
| expect((await client1.conversations.listDms()).length).toBe(1); | ||
| expect((await client1.conversations.listGroups()).length).toBe(0); | ||
|
|
||
| expect((await client2.conversations.list()).length).toBe(0); | ||
|
|
||
| await client2.conversations.sync(); | ||
|
|
||
| const group2 = await client2.conversations.list(); | ||
| expect(group2.length).toBe(1); | ||
| expect(group2[0].id).toBe(group.id); | ||
| expect(await group2[0].dmPeerInboxId()).toBe(client1.inboxId); | ||
|
|
||
| expect((await client2.conversations.listDms()).length).toBe(1); | ||
| expect((await client2.conversations.listGroups()).length).toBe(0); | ||
|
|
||
| const dm1 = await client1.conversations.getDmByInboxId(client2.inboxId!); | ||
| expect(dm1).toBeDefined(); | ||
| expect(dm1!.id).toBe(group.id); | ||
|
|
||
| const dm2 = await client2.conversations.getDmByInboxId(client1.inboxId!); | ||
| expect(dm2).toBeDefined(); | ||
| expect(dm2!.id).toBe(group.id); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider splitting the DM test into smaller, focused test cases.
While the test is thorough, it covers multiple aspects (creation, permissions, sync, retrieval) in a single test case. This could make it harder to identify specific failures and maintain the test.
Consider splitting into these focused test cases:
describe('DM conversations', () => {
it('should create a DM with correct metadata', async () => {
// Test lines 82-110
});
it('should set correct consent state', async () => {
// Test lines 112
});
it('should sync DM between peers', async () => {
// Test lines 114-132
});
it('should retrieve DM by peer inbox ID', async () => {
// Test lines 134-140
});
});| allowed_states?: WasmGroupMembershipState[]; | ||
| conversation_type?: WasmConversationType; |
There was a problem hiding this comment.
Fix asymmetric conversion in ListConversationsOptions.
The conversion functions for SafeListConversationsOptions are asymmetric:
fromSafeListConversationsOptionshandles the new propertiestoSafeListConversationsOptionsdoesn't map them, which could lead to data loss
Update toSafeListConversationsOptions to include the new properties:
export const toSafeListConversationsOptions = (
options: WasmListConversationsOptions,
): SafeListConversationsOptions => ({
+ allowed_states: options.allowed_states,
+ conversation_type: options.conversation_type,
created_after_ns: options.created_after_ns,
created_before_ns: options.created_before_ns,
limit: options.limit,
});Also applies to: 194-195
| case "getDmPeerInboxId": { | ||
| const group = client.conversations.getConversationById(data.id); | ||
| if (group) { | ||
| const result = group.dmPeerInboxId(); | ||
| postMessage({ | ||
| id, | ||
| action, | ||
| result, | ||
| }); | ||
| } else { | ||
| postMessageError({ | ||
| id, | ||
| action, | ||
| error: "Group not found", | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix: Add missing break statement
The implementation of getDmPeerInboxId handler is missing a break statement after the if-else block, which could lead to unexpected behavior.
Add the missing break statement:
case "getDmPeerInboxId": {
const group = client.conversations.getConversationById(data.id);
if (group) {
const result = group.dmPeerInboxId();
postMessage({
id,
action,
result,
});
} else {
postMessageError({
id,
action,
error: "Group not found",
});
}
+ break;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| case "getDmPeerInboxId": { | |
| const group = client.conversations.getConversationById(data.id); | |
| if (group) { | |
| const result = group.dmPeerInboxId(); | |
| postMessage({ | |
| id, | |
| action, | |
| result, | |
| }); | |
| } else { | |
| postMessageError({ | |
| id, | |
| action, | |
| error: "Group not found", | |
| }); | |
| } | |
| } | |
| case "getDmPeerInboxId": { | |
| const group = client.conversations.getConversationById(data.id); | |
| if (group) { | |
| const result = group.dmPeerInboxId(); | |
| postMessage({ | |
| id, | |
| action, | |
| result, | |
| }); | |
| } else { | |
| postMessageError({ | |
| id, | |
| action, | |
| error: "Group not found", | |
| }); | |
| } | |
| break; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
sdks/browser-sdk/src/workers/client.ts (1)
251-261: Consider adding input validation for accountAddressWhile the implementation is correct, consider adding explicit validation for the accountAddress parameter before calling newDm to provide more specific error messages to the client.
case "newDm": { + if (!data.accountAddress) { + postMessageError({ + id, + action, + error: "Account address is required", + }); + break; + } const conversation = await client.conversations.newDm( data.accountAddress, ); postMessage({
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
sdks/browser-sdk/src/utils/conversions.ts(6 hunks)sdks/browser-sdk/src/utils/createClient.ts(1 hunks)sdks/browser-sdk/src/workers/client.ts(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- sdks/browser-sdk/src/utils/createClient.ts
🔇 Additional comments (8)
sdks/browser-sdk/src/utils/conversions.ts (3)
15-15: LGTM: New type imports are correctly added.
The new WASM types are properly imported and will be used by the modified options types.
Also applies to: 17-18
147-147: LGTM: Message listing direction support is properly implemented.
The direction property is correctly added to SafeListMessagesOptions with symmetric conversions in both directions.
Also applies to: 157-157, 171-171
175-176: LGTM: Conversation listing options are now symmetrically converted.
The conversion functions for SafeListConversationsOptions now properly handle both allowed_states and conversation_type in both directions, addressing the asymmetric conversion issue from the previous review.
Also applies to: 185-186, 196-197
sdks/browser-sdk/src/workers/client.ts (5)
102-110: LGTM: Signature handling implementation looks good!
The new signature handler and the renamed method follow consistent patterns and maintain proper error handling.
Also applies to: 119-120
215-238: LGTM: Conversation listing handlers are well-implemented!
The new handlers for listing groups and DMs maintain consistency with existing patterns and properly handle conversation safety conversions.
289-297: LGTM: DM retrieval implementation is solid!
The handler properly handles both successful and not-found cases while maintaining consistency with existing patterns.
734-751: LGTM: DM peer inbox ID handler is properly implemented!
The handler follows consistent patterns for group operations and includes proper error handling.
Line range hint 1-7: Verify type definitions for new message handlers
Let's ensure that all new message handlers are properly typed in the ClientEventsActions type.
✅ Verification successful
All new message handlers are properly typed in ClientEventsActions
The verification confirms that all new message handlers are correctly defined in the types:
- getRevokeInstallationsSignatureText
- getGroups
- getDms
- newDm
- getDmByInboxId
- getDmPeerInboxId
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify type definitions for new message handlers
# Expected: All new handlers should be defined in ClientEventsActions type
# Search for ClientEventsActions type definition
rg -A 20 "type ClientEventsActions = " "sdks/browser-sdk/src/types"
# Verify that all new handlers are included
echo "Checking for new handler definitions..."
for handler in "getRevokeInstallationsSignatureText" "getGroups" "getDms" "newDm" "getDmByInboxId" "getDmPeerInboxId"; do
rg "\"$handler\"" "sdks/browser-sdk/src/types"
done
Length of output: 3229
Summary
getRevokeInstallationsSignatureTextmethod to clientapplySignaturesRequeststoapplySignaturesnewDm,listGroups,listDms, andgetDmByInboxIdmethods toConversationsdmPeerInboxIdtoConversationdirectionto list messages optionsallowed_statesandconversation_typeto list conversations optionsSummary by CodeRabbit
Release Notes
New Features
getDmByInboxId,listGroups,listDms, andnewDm.Bug Fixes
Tests