Fix example Vite config, misc cleanup #724
Conversation
|
WalkthroughThe pull request includes updates to the Changes
Possibly related PRs
Suggested reviewers
Warning Rate limit exceeded@rygine has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 59 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. 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 (
|
Deploying xmtp-js-docs with
|
| Latest commit: |
931ce82
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e0573e53.xmtp-js.pages.dev |
| Branch Preview URL: | https://rygine-fixes.xmtp-js.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
content-types/content-type-primitives/package.json (1)
41-44: LGTM! Changes align with source map requirements.The addition of source files and tsconfig.json will enable proper source map functionality in production environments. The test file exclusion pattern is correctly configured to prevent shipping tests to production. Note that this change will slightly increase the published package size, but this is an acceptable trade-off for better debugging capabilities in production.
Consider documenting this source map configuration approach in the project's contributing guidelines to maintain consistency across all XMTP packages.
content-types/content-type-text/package.json (1)
41-44: LGTM! Changes align with source map requirements.The addition of source files and tsconfig.json will enable proper source map functionality in production, while correctly excluding test files. Note that this will slightly increase the published package size, but the tradeoff is justified for better debugging capabilities.
Consider documenting this source map enhancement in the package's README to help users understand why source files are included and how to leverage them for debugging.
content-types/content-type-read-receipt/package.json (1)
41-44: LGTM! Consider optimizing package size.The changes to include source files for source map support are good. However, consider being more specific with exclusions to minimize package size:
"files": [ "dist", "src", - "!src/**/*.test.*", + "!src/**/*.test.*", + "!src/**/__tests__/**", + "!src/**/*.stories.*", + "!src/**/*.spec.*", "tsconfig.json" ],
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
content-types/content-type-group-updated/package.json(1 hunks)content-types/content-type-primitives/package.json(1 hunks)content-types/content-type-reaction/package.json(1 hunks)content-types/content-type-read-receipt/package.json(1 hunks)content-types/content-type-remote-attachment/package.json(1 hunks)content-types/content-type-reply/package.json(1 hunks)content-types/content-type-text/package.json(1 hunks)content-types/content-type-transaction-reference/package.json(1 hunks)examples/react-vite-browser-sdk/vite.config.ts(1 hunks)packages/frames-validator/package.json(1 hunks)sdks/browser-sdk/README.md(2 hunks)
🔇 Additional comments (14)
examples/react-vite-browser-sdk/vite.config.ts (1)
8-9: LGTM! Dependency optimization configuration looks correct.
The changes properly handle the SDK's dependency requirements:
- Excluding @xmtp/browser-sdk prevents pre-bundling issues with import.meta.url
- Including @xmtp/proto ensures proper pre-bundling of the protocol package
Let's verify the dependency structure to ensure these changes align with the package requirements:
✅ Verification successful
Vite dependency configuration is correctly optimized
The verification confirms that:
@xmtp/browser-sdkis properly excluded from pre-bundling as it usesimport.meta.urlin its implementation@xmtp/protoinclusion is necessary as it's a transitive dependency throughbrowser-sdk- The example correctly imports and uses the SDK in
App.tsxandcreateClient.ts - Required COOP/COEP headers are configured for proper functionality
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the dependency structure and import.meta.url usage
# Check for import.meta.url usage in browser-sdk
rg -l "import\.meta\.url" --type ts
# Check package.json dependencies
cat package.json | jq '.dependencies | select(has("@xmtp/browser-sdk", "@xmtp/proto"))'
Length of output: 248
Script:
#!/bin/bash
# Let's check the package.json in the example directory specifically
cat examples/react-vite-browser-sdk/package.json | jq '.dependencies'
# Check for any import.meta.url usage in the example
rg -l "import\.meta\.url" examples/react-vite-browser-sdk/
# Verify if these packages are actually used in the example
rg -l "@xmtp/(browser-sdk|proto)" examples/react-vite-browser-sdk/
Length of output: 624
Script:
#!/bin/bash
# Let's check the actual usage context in the example files
rg -A 5 "@xmtp/(browser-sdk|proto)" examples/react-vite-browser-sdk/src/
# Check the content of vite.config.ts to see the complete configuration
cat examples/react-vite-browser-sdk/vite.config.ts
Length of output: 1574
packages/frames-validator/package.json (2)
23-26: LGTM! Changes align with source map requirements
The addition of src and tsconfig.json to the published files is appropriate for enabling source maps in production environments. The exclusion of test files helps maintain a clean package while still providing debugging capabilities.
23-26: Verify the impact on package size
While including source files is necessary for source maps, it's good practice to verify the impact on the published package size.
✅ Verification successful
Based on the output from the npm pack command, I can now provide a final response about the package size impact:
Package size is reasonable with source files included
The package size with source files included is 1.2MB compressed (3.8MB unpacked), which is reasonable for a frames validator package that includes TypeScript source files for source maps. The source files are essential for debugging and don't significantly bloat the package size.
Key size metrics:
- Compressed size: 1.2MB
- Unpacked size: 3.8MB
- Total files: 373
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Compare the package size before and after including src files
# Get the current size of the published package
npm pack --dry-run | grep -A 1 "package size:" || true
# List all files that would be included in the package
npm pack --dry-run | grep -A 100 "npm notice === Tarball Contents ===" | grep -B 100 "npm notice === Tarball Details ===" || true
Length of output: 44394
content-types/content-type-reaction/package.json (2)
41-44: Consider adding source map configuration in tsconfig.json
Since the goal is to support source maps in production, ensure that your tsconfig.json has the appropriate source map settings.
Let's verify the source map configuration:
#!/bin/bash
# Description: Check source map configuration in tsconfig.json
# Expected: sourceMap should be enabled
# Check if sourceMap is enabled in tsconfig.json
cat content-types/content-type-reaction/tsconfig.json | jq '.compilerOptions.sourceMap'41-44: LGTM! Including source files will enable source map functionality.
The addition of src and tsconfig.json to the published files is correct and aligns with the PR objective of making source maps work in production. The exclusion of test files (!src/**/*.test.*) is also a good practice.
Let's verify that this change is consistent across other related packages:
✅ Verification successful
Verified: Files configuration is consistent across all content type packages
The verification shows that all 8 content type packages in the repository have identical files configuration in their package.json:
distsrc!src/**/*.test.*tsconfig.json
This confirms that the changes are consistently applied across the entire content types ecosystem, which is the correct approach for maintaining source map functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if other XMTP content type packages have similar files configuration
# Expected: All packages should have similar entries in their files array
# Find all package.json files in content-types directory
fd package.json content-types/ --exec jq -r '.files | select(. != null) | join(", ")'
Length of output: 248
Script:
#!/bin/bash
# Let's try a different approach to check package.json files in content-types directory
# First, let's find all package.json files
echo "Package.json files found:"
fd package.json content-types/
# Now let's examine each package.json's files field
echo -e "\nFiles configuration in each package.json:"
fd package.json content-types/ -x cat {} | jq '.files'
Length of output: 1171
content-types/content-type-read-receipt/package.json (1)
41-44: Verify the impact on published package size.
Let's check the impact of including the src directory on the published package size.
✅ Verification successful
Let me gather more information about the files that would be included in the package.
Let me check if this is a new package or if there are any TypeScript source files in the repository.
Package size is acceptable with source files included
The current package size of 1.2MB includes both the compiled dist files and TypeScript source files. This is a reasonable size considering:
- The package contains only 3 TypeScript source files
- Source files are beneficial for debugging and source maps
- Test files are correctly excluded via
!src/**/*.test.* - This is a minor version update (v1.1.11) with established patterns
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Compare the package size before and after including src files
# Get the current package size
npm pack --dry-run 2>&1 | grep "package size:" || echo "Failed to get package size"
# List all files that would be included in the package
npm pack --dry-run 2>&1 | grep -A 100 "npm notice === Tarball Contents ===" | grep -B 100 "npm notice === Tarball Details ===" || echo "Failed to list files"
Length of output: 250
Script:
#!/bin/bash
# List all files in src and dist directories to understand the package contents
echo "Files in src directory:"
fd . src --type f
echo -e "\nFiles in dist directory:"
fd . dist --type f
echo -e "\nTotal size of src directory:"
du -sh src 2>/dev/null || echo "src directory not found"
echo -e "\nTotal size of dist directory:"
du -sh dist 2>/dev/null || echo "dist directory not found"
Length of output: 636
Script:
#!/bin/bash
# Check for TypeScript files in the repository
echo "TypeScript files in the repository:"
fd -e ts -e tsx . --type f
echo -e "\nPackage version from package.json:"
cat content-types/content-type-read-receipt/package.json | jq .version
echo -e "\nRepository structure:"
tree content-types/content-type-read-receipt -L 2
Length of output: 11172
content-types/content-type-group-updated/package.json (2)
41-44: LGTM! Changes align with source map objectives.
The inclusion of source files and tsconfig.json while excluding test files is a good approach for enabling source maps in production environments. The pattern !src/**/*.test.* effectively excludes test files from the published package.
41-44: Verify the impact on package size.
Including the src directory will increase the published package size. While this is acceptable for enabling source maps, it's good practice to verify the impact.
✅ Verification successful
Package size impact is acceptable
The package size comparison shows:
- Current published size: 9.8KB (unpacked)
- New size with src files: 5.6KB (unpacked)
Including the src directory actually results in a smaller package size in this case, likely due to other optimizations or removals in the package contents. The source files are minimal (951B for GroupUpdated.ts and 32B for index.ts) and the overall package remains lightweight.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Compare package sizes before and after including src files
# Get the current package size from npm
PACKAGE_NAME=$(node -p "require('./content-types/content-type-group-updated/package.json').name")
PACKAGE_VERSION=$(node -p "require('./content-types/content-type-group-updated/package.json').version")
echo "Checking size for $PACKAGE_NAME@$PACKAGE_VERSION"
# Get published package size from npm
npm view $PACKAGE_NAME@$PACKAGE_VERSION dist.unpackedSize
# Get local package size after changes
cd content-types/content-type-group-updated && \
npm pack --dry-run | grep -A 1 "package size:" || \
cd ../..
Length of output: 1222
content-types/content-type-transaction-reference/package.json (2)
41-44: LGTM! Including source files will improve debugging experience
The addition of src and tsconfig.json to the published package will enable source maps in production environments, making it easier to debug issues. The test exclusion pattern ensures that test files aren't shipped to production.
43-43: Verify test file exclusion pattern and sensitive information
Let's ensure the test exclusion pattern is comprehensive and no sensitive information would be exposed through source files.
✅ Verification successful
Test exclusion pattern is correctly configured and no sensitive information exposed
The test exclusion pattern !src/**/*.test.* is correctly configured as:
- The directory structure shows a clear separation of test files (TransactionReference.test.ts) from source files
- The test file follows the
.test.tsnaming convention that matches the exclusion pattern - No sensitive information patterns were found in the visible source files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential test files that might slip through and sensitive information
# Check if the test exclusion pattern catches all test-related files
echo "Checking for test files that might slip through..."
fd . src/ -t f | grep -iE '(test|spec|mock|stub|fixture)'
# Look for potential sensitive patterns in source files
echo "Checking for potential sensitive patterns..."
rg -i '(password|secret|key|token|credential)' src/
# Check if there are any .env files or similar that might get included
echo "Checking for configuration files..."
fd -H '\.env' src/
Length of output: 654
Script:
#!/bin/bash
# First, let's check if the src directory exists at a different location
echo "Finding source directory..."
fd -t d "src"
# Let's also check the repository structure
echo "Checking repository structure..."
ls -la
# Let's examine the package.json to understand the project structure
echo "Examining package.json..."
cat package.json
Length of output: 4047
Script:
#!/bin/bash
# Let's check the specific content-type-transaction-reference directory
echo "Checking content-type-transaction-reference structure..."
cd content-types/content-type-transaction-reference && ls -la src/
# Check for test files in this specific package
echo "Checking for test files..."
fd -t f . content-types/content-type-transaction-reference/src/ | grep -iE '(test|spec|mock|stub|fixture)'
# Look for sensitive patterns in this specific package
echo "Checking for sensitive patterns..."
rg -i '(password|secret|key|token|credential)' content-types/content-type-transaction-reference/src/
# Check the actual files that would be included based on package.json patterns
echo "Files that would be included..."
cd content-types/content-type-transaction-reference && npm pack --dry-run
Length of output: 1355
content-types/content-type-reply/package.json (1)
41-44: LGTM! Changes align with source map requirements.
The addition of source files and TypeScript configuration while excluding test files is a good approach for enabling source maps in production.
content-types/content-type-remote-attachment/package.json (1)
41-44: LGTM! Including source files for source maps is a good practice.
The changes to include source files while excluding tests align well with the PR objective of ensuring source maps work correctly in production. This is a common practice for TypeScript packages.
Let's verify the impact on package size:
✅ Verification successful
Based on the npm pack output, I can now generate the final response:
Package size impact is minimal and acceptable
The source files add negligible size to the package:
- Total package size: 1.2 MB compressed, 3.8 MB uncompressed
- TypeScript source files are small text files:
- RemoteAttachment.ts: 4.9 KB
- Attachment.ts: 1.2 KB
- Other source files: ~1-4 KB each
Including source files for source maps while excluding tests is a common practice that improves debugging without significantly impacting package size.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the package size impact of including source files
# Expected: Source files should not significantly increase package size as they're mainly TypeScript files
# Get current package size
npm pack --dry-run . | grep "package size:"
# List and count files that would be included
echo "Files to be included in package:"
npm pack --dry-run . | grep -A 100 "npm notice === Tarball Contents ===" | grep -B 100 "npm notice === Tarball Details ==="
Length of output: 44454
sdks/browser-sdk/README.md (2)
71-71: LGTM! Clear explanation of technical requirement.
The added explanation about import.meta.url usage clearly communicates why special bundler configuration is needed.
82-83: Verify the updated Vite configuration.
The changes to the Vite configuration align with the PR objectives to fix the example Vite config. However, let's verify that these are the only dependencies that need special handling.
✅ Verification successful
Based on the output from checking package.json files and import statements, I can now generate a final response regarding the Vite configuration verification.
The Vite configuration is correctly set up with the necessary dependencies.
The current configuration in sdks/browser-sdk/README.md properly excludes @xmtp/browser-sdk and includes @xmtp/proto. This is the correct setup because:
@xmtp/browser-sdkneeds to be excluded to prevent bundling issues as it's the main SDK package@xmtp/protoneeds to be included as it's a critical dependency used throughout the codebase for protocol-related functionality
No other dependencies require special handling in the Vite configuration based on:
- Analysis of import statements showing no other dependencies with similar bundling requirements
- Package dependencies structure showing these are the key packages affecting browser bundling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other potential dependencies using import.meta.url
# that might need to be included in the Vite config
# Search for import.meta.url usage in dependencies
cd examples/react-vite-browser-sdk
if [ -f "package-lock.json" ]; then
deps=$(jq -r '.dependencies | keys[]' package-lock.json)
for dep in $deps; do
echo "Checking $dep for import.meta.url usage..."
rg -l "import\.meta\.url" "node_modules/$dep" || true
done
fi
# Also check the source code for any direct dependencies that might need special handling
cd ../..
rg --type typescript --type javascript "from ['\"](\..*|@xmtp).*['\""]" ./src
Length of output: 173
Script:
#!/bin/bash
# Let's try a different approach to verify the Vite configuration
# First, let's check the direct dependencies in package.json
echo "Checking package.json dependencies..."
fd package.json --type f --exec cat {} \;
# Then, let's look for any import statements in the codebase
echo -e "\nChecking import statements..."
rg "from ['\"]@xmtp.*['\"]" --type ts --type js
# Finally, let's check the vite.config.ts file
echo -e "\nChecking vite configuration..."
fd "vite.config" --type f --exec cat {} \;
Length of output: 52678
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
packages/consent-proof-signature/package.json (1)
41-42: LGTM! Improved source maps support.Including the
srcdirectory while excluding test files (!src/**/*.test.*) is beneficial because:
- Source maps will work correctly in production environments
- Test files are appropriately excluded from the published package
This change aligns with modern bundler expectations and improves debugging capabilities in production environments.
sdks/browser-sdk/package.json (1)
40-40: LGTM! Including src directory is essential for source maps.Adding the
srcdirectory to the published files is crucial for enabling proper source map functionality in production environments, making debugging significantly easier for consumers of this SDK.This change will help developers debug issues in production by:
- Enabling accurate stack traces that map to the original source code
- Allowing step-through debugging in browser developer tools
- Making it easier to understand runtime behavior in production environments
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (14)
content-types/content-type-group-updated/package.json(1 hunks)content-types/content-type-primitives/package.json(1 hunks)content-types/content-type-reaction/package.json(1 hunks)content-types/content-type-read-receipt/package.json(1 hunks)content-types/content-type-remote-attachment/package.json(1 hunks)content-types/content-type-reply/package.json(1 hunks)content-types/content-type-text/package.json(1 hunks)content-types/content-type-transaction-reference/package.json(1 hunks)packages/consent-proof-signature/package.json(2 hunks)packages/frames-client/package.json(1 hunks)packages/frames-validator/package.json(1 hunks)sdks/browser-sdk/package.json(1 hunks)sdks/js-sdk/package.json(1 hunks)sdks/node-sdk/package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- content-types/content-type-group-updated/package.json
- content-types/content-type-primitives/package.json
- content-types/content-type-reaction/package.json
- content-types/content-type-read-receipt/package.json
- content-types/content-type-remote-attachment/package.json
- content-types/content-type-reply/package.json
- content-types/content-type-text/package.json
- content-types/content-type-transaction-reference/package.json
- packages/frames-validator/package.json
🔇 Additional comments (7)
packages/consent-proof-signature/package.json (1)
32-33: LGTM! Good practice to expose package.json.
Adding "./package.json": "./package.json" to exports follows the Node.js package exports specification and allows consumers to directly import package metadata when needed.
packages/frames-client/package.json (3)
31-32: LGTM: Package.json export follows best practices
The addition of an explicit package.json export follows Node.js best practices for package exports and provides better control over what consumers can import.
39-41: Verify contents of src directory for production distribution
Including the src directory for source maps is good, but let's verify there's no sensitive information or unnecessary files being published.
✅ Verification successful
Files array configuration looks appropriate and safe
The verification shows:
- No sensitive files (
.env,.key,.pem,.secret) present in the src directory - Test files are properly excluded by the
!src/**/*.test.*pattern, confirmed by seeing.test.tsfiles that will be filtered out - Only standard TypeScript source files are included: client, constants, converters, errors, types, utils, etc.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check src directory contents and size impact
echo "Checking src directory contents:"
fd . "packages/frames-client/src" -t f
echo -e "\nChecking for potential sensitive files:"
fd . "packages/frames-client/src" -t f -e env -e key -e pem -e secret
echo -e "\nChecking file patterns that should be excluded:"
fd . "packages/frames-client/src" -t f -e test.ts -e spec.ts -e mock.ts
Length of output: 1016
Line range hint 82-84: Verify workspace and peer dependency handling
The package uses workspace dependencies and has peer dependencies. Let's ensure these are handled correctly in the published package.
Also applies to: 89-91
✅ Verification successful
Let me gather more information about the peer dependency configuration and its version range.
Dependencies are properly configured and used
The verification shows:
- Workspace dependency
tsconfigis correctly referenced in package.json and properly extends the base config @xmtp/xmtp-jspeer dependency is appropriately declared with version>9.3.1- The imports from
@xmtp/xmtp-jsare used consistently across test and source files, importing only the necessary types and classes - No potential dependency conflicts or misconfigurations were found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check workspace dependency resolution
echo "Checking workspace dependency references:"
rg "workspace:\*" packages/frames-client/
# Check peer dependency usage
echo -e "\nChecking @xmtp/xmtp-js usage in source:"
rg "@xmtp/xmtp-js" packages/frames-client/src/
Length of output: 738
Script:
#!/bin/bash
# Check peer dependency declaration
echo "Checking peer dependency declaration:"
jq '.peerDependencies' packages/frames-client/package.json
# Check actual @xmtp/xmtp-js imports and usage patterns
echo -e "\nChecking detailed @xmtp/xmtp-js usage:"
ast-grep --pattern 'import { $$$ } from "@xmtp/xmtp-js"'
# Check if tsconfig workspace dependency is properly referenced
echo -e "\nChecking tsconfig reference:"
cat packages/frames-client/tsconfig.json
Length of output: 1592
sdks/node-sdk/package.json (1)
31-32: LGTM: Package.json export addition is correct
The addition of "./package.json": "./package.json" to exports follows the standard pattern for Node.js packages, allowing direct access to package metadata.
sdks/browser-sdk/package.json (1)
32-33: LGTM! Explicit package.json export is a good practice.
Adding explicit package.json export mapping provides better control over package entry points and helps with module resolution.
sdks/js-sdk/package.json (1)
62-62: Verify source map functionality in production builds.
The addition of the "src" directory is intended to fix source maps in production. Let's verify the current source map configuration.
✅ Verification successful
Let me check the specific source map configuration in the js-sdk's rollup and typescript configs to verify the setup.
Source map configuration is properly set up for production builds
The source map functionality is correctly configured:
- Source maps are enabled in all build outputs (
.js,.cjs, and browser/bundler variants) - TypeScript compilation is configured with source maps enabled
- The shared build configuration also has source maps enabled
The addition of the "src" directory to the package is aligned with the source map setup.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check source map configuration in build setup
# Check if source maps are enabled in build configuration
rg -l "sourcemap.*true" .
# Look for source map related configurations
rg -l "sourceMap.*true" .
# Check rollup config for source map settings
fd -e js -e ts -e json "rollup.config" .
Length of output: 1630
Script:
#!/bin/bash
# Check js-sdk rollup config source map settings
cat sdks/js-sdk/rollup.config.js
# Check js-sdk typescript config
cat sdks/js-sdk/tsconfig.json
# Check shared typescript build config
cat shared/tsconfig/build.json
Length of output: 3143
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
sdks/node-sdk/src/Conversations.ts (2)
Line range hint
50-55: Consider adding a migration guide or deprecation notice.Since renaming
newConversationtonewGroupis a breaking change, it would be helpful to provide migration guidance for users updating from previous versions.Would you like me to help draft a migration guide section for the README or create a GitHub issue to track this documentation task?
Breaking change requires updates to documentation and tests
The method rename from
newConversationtonewGrouprequires the following updates:
- Test files in
sdks/node-sdk/test/need to be updated to usenewGroup- CHANGELOG.md needs to document this as a breaking change
- No API documentation or README files were found referencing the old method name
🔗 Analysis chain
Line range hint
50-55: Implementation looks good but introduces a breaking change.The renaming from
newConversationtonewGroupimproves clarity by making the method's purpose explicit. The implementation is clean and follows the established patterns in the codebase.Let's verify if all documentation has been updated to reflect this breaking change:
Documentation and test files need to be updated to reflect the breaking change
The verification reveals several files that need updates due to the method rename from
newConversationtonewGroup:
sdks/node-sdk/CHANGELOG.mdstill referencesnewConversation- Multiple test files in
sdks/node-sdk/test/still usenewConversation:
Client.test.tsConversation.test.tsLet's verify if there are any API documentation or example files that need updates:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to newConversation in docs and examples rg -g '!*.{log,lock}' -i 'newConversation' # Search for documentation files that might need updates fd -e md -e mdx | xargs rg -l 'conversation|group'Length of output: 15035
Script:
#!/bin/bash # Check for API documentation files fd -e ts -e tsx -p 'api|docs|example' | xargs rg -l 'newConversation' # Check README files fd README.md | xargs rg -l 'newConversation'Length of output: 114
sdks/node-sdk/test/Conversations.test.ts (4)
Line range hint
15-73: LGTM! Consider adding edge case tests.The group creation test is comprehensive, covering essential aspects like properties, members, metadata, and list operations. However, consider adding tests for edge cases:
- Creating a group with no members
- Creating a group with duplicate members
- Creating a group with invalid member addresses
Line range hint
145-166: Add negative test cases for retrieval operations.While the basic retrieval tests are good, consider adding tests for:
- Retrieving non-existent groups
- Retrieving non-existent messages
- Retrieving messages from non-existent groups
Line range hint
167-256: Consider adding input validation tests.The test suite thoroughly covers all group creation options. To strengthen it, consider adding tests for:
- Invalid image URLs
- Empty or whitespace-only names
- Malformed URLs for pinned frames
- Maximum length validation for description and name
Line range hint
257-472: Add error handling and timeout tests for streams.While the streaming tests cover the happy path well, consider adding:
- Error handling tests for stream interruptions
- Timeout handling to prevent infinite loops
- Tests for concurrent stream operations
- Clean up of stream resources
Example test structure:
it("should handle stream errors", async () => { // Setup const stream = client.conversations.stream(); // Simulate network error or interruption setTimeout(() => stream.destroy(), 100); // Assert error handling await expect(async () => { for await (const _ of stream) { // Should not reach here } }).rejects.toThrow(); }); it("should timeout after specified duration", async () => { const stream = client.conversations.stream({ timeout: 1000 }); const start = Date.now(); for await (const _ of stream) { if (Date.now() - start > 1000) { throw new Error("Stream did not timeout"); } } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
sdks/node-sdk/src/Conversations.ts(1 hunks)sdks/node-sdk/test/Conversations.test.ts(16 hunks)
🔇 Additional comments (1)
sdks/node-sdk/test/Conversations.test.ts (1)
Line range hint 74-144: LGTM! DM creation test is thorough.
The test comprehensively covers DM-specific functionality, including proper permission settings, consent state, and peer inbox ID verification.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
sdks/node-sdk/test/Conversation.test.ts (1)
Line range hint
407-411: LGTM: Clear separation between group and DM conversationsThe changes demonstrate a clear separation of concerns by using distinct methods:
newGroupfor group conversations (renamed fromnewConversation)newDmfor direct message conversationsThis separation improves API clarity and maintains consistency with other SDKs.
sdks/node-sdk/test/Conversations.test.ts (5)
Line range hint
15-73: Consider adding edge cases and extracting permission assertionsThe test case thoroughly covers the happy path for group creation, but consider:
- Adding edge cases (e.g., creating a group with no members, with invalid addresses)
- Extracting permission assertions into a helper function for reuse across tests
// Helper function for verifying default group permissions const assertDefaultGroupPermissions = (permissions: any) => { expect(permissions.policyType).toBe(GroupPermissionsOptions.AllMembers); expect(permissions.policySet).toEqual({ addMemberPolicy: 0, removeMemberPolicy: 2, addAdminPolicy: 3, removeAdminPolicy: 3, updateGroupNamePolicy: 0, updateGroupDescriptionPolicy: 0, updateGroupImageUrlSquarePolicy: 0, updateGroupPinnedFrameUrlPolicy: 0, }); };
Line range hint
74-144: Consider adding DM edge cases and helper functionsThe DM test is thorough but could be enhanced by:
- Adding edge cases (e.g., creating a DM with self, creating multiple DMs with same peer)
- Extracting DM-specific assertions into a helper function
// Helper function for verifying DM permissions and metadata const assertDmProperties = (dm: any, creatorInboxId: string, peerInboxId: string) => { expect(dm.permissions.policyType).toBe(GroupPermissionsOptions.CustomPolicy); expect(dm.metadata.conversationType).toBe("dm"); expect(dm.metadata.creatorInboxId).toBe(creatorInboxId); expect(dm.dmPeerInboxId).toBe(peerInboxId); };
Line range hint
145-166: Enhance group retrieval test coverageConsider adding test cases for:
- Retrieving a non-existent group ID
- Retrieving a group after updates (name, members, etc.)
- Retrieving a deleted/inactive group
Line range hint
167-257: Refactor long test case into separate test casesThe current test covers multiple scenarios in a single test case. Consider splitting it into separate test cases for better maintainability and clarity:
- Test group creation with name
- Test group creation with image URL
- Test group creation with permissions
- Test group creation with description
- Test group creation with pinned frame URL
describe("group creation with options", () => { let client1: Client; let user2: User; beforeEach(async () => { const user1 = createUser(); user2 = createUser(); client1 = await createRegisteredClient(user1); await createRegisteredClient(user2); }); it("should create group with name", async () => { const group = await client1.conversations.newGroup( [user2.account.address], { groupName: "foo" } ); expect(group.name).toBe("foo"); }); // Additional test cases for other options... });
Line range hint
258-473: Enhance streaming tests with error cases and reduce duplicationConsider the following improvements:
- Extract common setup code into beforeEach hooks
- Add error cases (e.g., network disconnection, invalid messages)
- Test stream cleanup/closure
describe("streaming", () => { let client1: Client, client2: Client, client3: Client; beforeEach(async () => { const [user1, user2, user3] = Array.from({ length: 3 }, () => createUser()); [client1, client2, client3] = await Promise.all([ createRegisteredClient(user1), createRegisteredClient(user2), createRegisteredClient(user3) ]); }); // Individual test cases... });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
sdks/node-sdk/src/Conversations.ts(1 hunks)sdks/node-sdk/test/Client.test.ts(1 hunks)sdks/node-sdk/test/Conversation.test.ts(15 hunks)sdks/node-sdk/test/Conversations.test.ts(16 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- sdks/node-sdk/src/Conversations.ts
🔇 Additional comments (6)
sdks/node-sdk/test/Conversation.test.ts (5)
16-16: LGTM: Consistent method rename for conversation metadata tests
The rename from newConversation to newGroup is consistently applied across all conversation metadata test cases while preserving the existing test coverage and assertions.
Also applies to: 42-42, 68-68, 94-94
122-122: LGTM: Consistent method rename for member management tests
The rename from newConversation to newGroup is properly applied in both address-based and inbox-based member management test cases.
Also applies to: 161-161
197-197: LGTM: Consistent method rename for message handling tests
The rename from newConversation to newGroup is properly applied across all message handling test cases, maintaining comprehensive coverage for different messaging scenarios.
Also applies to: 229-229, 245-245, 284-284, 300-300, 314-314
353-353: LGTM: Consistent method rename for admin management tests
The rename from newConversation to newGroup is properly applied in both regular and super admin management test cases.
Also applies to: 379-379
Line range hint 16-407: Verify migration path for breaking change
The rename from newConversation to newGroup is a breaking change. Let's verify the migration path and documentation updates.
sdks/node-sdk/test/Conversations.test.ts (1)
Line range hint 1-473: LGTM! The changes align well with the PR objectives
The rename from newConversation to newGroup has been consistently applied throughout the test file, maintaining clear distinction between group and DM operations. The test coverage remains comprehensive and well-structured.
Summary
package.jsonfor several packages to include the/srcfolder when publishing so that source maps work in productiontsconfig.jsonfrom published filespackage.jsonConversations.newConversationtoConversations.newGroupin the Node SDK to match other SDKsSummary by CodeRabbit
newConversationmethod tonewGroupin the Conversations class for clearer functionality regarding group conversations.