feat: c version #306
feat: c version #306
Conversation
WalkthroughThe Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (8)
packages/sdk/src/getEventMessages.ts (1)
Based on the git history, I can now provide a final response:
Breaking change confirmed:
dont_include_hashed_keysparameter was changed fromtruetofalseThe git history shows that this parameter was previously defaulting to
trueand was changed tofalsein commit 0b76d37. This is indeed a breaking change as it alters the response structure by including previously excluded data.Action items needed:
- Add this change to the SDK's changelog (packages/sdk/CHANGELOG.md)
- Bump the major version (currently at 1.0.0-alpha.22)
- Document the migration steps for clients that may be affected by the larger response payloads
The change affects both
getEventMessages.tsandgetEntities.tsas shown in the ripgrep results, making this a significant API behavior modification.🔗 Analysis chain
Line range hint
1-89: Flag potential breaking change.Setting
dont_include_hashed_keysto false changes the response structure by including previously excluded data. This could be a breaking change for clients that:
- Expect hashed keys to be excluded
- Have fixed response parsing logic
- Have memory constraints for handling larger payloads
Consider:
- Adding this change to the changelog
- Bumping the major version if following semver
- Adding a migration guide for affected clients
Let me check the changelog and version information for the SDK package where this change is being made.
Let me check the torii-client package to understand if this is actually a breaking change.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Look for version and changelog files to ensure breaking changes are documented # Search for changelog files fd -i changelog # Search for version related files fd -i "version|release" -e json -e mdLength of output: 565
Script:
#!/bin/bash # Check SDK changelog and package.json for version info cat packages/sdk/CHANGELOG.md cat packages/sdk/package.json # Check if dont_include_hashed_keys is used elsewhere in the codebase rg "dont_include_hashed_keys" -B 2 -A 2 # Check the torii-client package for the Query interface definition cat packages/torii-client/src/types.tsLength of output: 3353
Script:
#!/bin/bash # Check torii-client package for the Query interface and its usage fd -t f . packages/torii-client -x cat {} # Check if this parameter was previously defaulting to true git log -p packages/sdk/src/getEventMessages.tsLength of output: 11105
packages/sdk/src/__example__/index.ts (4)
Line range hint
3-40: LGTM! Well-structured type definitions.The interfaces are well-organized and provide good type safety. The consistent use of
fieldOrderacross all models helps maintain a predictable structure.Consider adding JSDoc comments to document the purpose of each interface and their fields, especially since this is an example file that others might learn from.
Line range hint
78-91: Consider using more realistic example URLs.The placeholder URLs (
your-rpc-url,your-torii-url, etc.) could be replaced with example.com URLs or documented format patterns to better guide users.- rpcUrl: "your-rpc-url", - toriiUrl: "your-torii-url", - relayUrl: "your-relay-url", - worldAddress: "your-world-address", + rpcUrl: "https://rpc.example.com", + toriiUrl: "https://torii.example.com", + relayUrl: "https://relay.example.com", + worldAddress: "0x1234567890abcdef1234567890abcdef12345678",
Line range hint
103-112: Remove or comment out the invalid example.The invalid example might be confusing as it appears to be executable code. Consider either commenting it out or wrapping it in a
@ts-expect-errordirective to make it clear this is intentionally wrong.- const invalidMessage = { - id: "0x123", - name: "Alice", - score: 100, - unknownField: "Invalid", - }; - - const invalidTypedData = db.generateTypedData( - "Player", - invalidMessage // TypeScript Error - ); + // Example of incorrect usage that TypeScript will catch: + // @ts-expect-error - Demonstrating type safety + // const invalidTypedData = db.generateTypedData("Player", { + // id: "0x123", + // name: "Alice", + // score: 100, + // unknownField: "Invalid", // Unknown field will cause type error + // });
Line range hint
114-134: Refactor duplicate callback handling logic.The error handling and data logging logic is duplicated between the subscription and query callbacks. Consider extracting this into a reusable function.
+ // Helper function for handling query responses + const handleQueryResponse = (context: string) => (resp: any) => { + if (resp.error) { + console.error(`Error querying ${context}:`, resp.error.message); + return; + } + if (resp.data) { + console.log(`Queried ${context}:`, resp.data.map((a) => a.models)); + } + }; + db.subscribeEntityQuery( { world: { player: { $: { where: { name: { $is: "Alice" }, score: { $is: 10 }, }, }, }, }, }, - (resp) => { - if (resp.error) { - console.error( - "Error querying todos and goals:", - resp.error.message - ); - return; - } - if (resp.data) { - console.log( - "Queried todos and goals:", - resp.data.map((a) => a.models.world) - ); - } - } + handleQueryResponse("player data") );Also applies to: 135-177
packages/sdk/src/__tests__/convertQueryToClause.test.ts (1)
Line range hint
9-180: Consider adding edge case testsWhile the current test coverage is good, consider adding tests for:
- Empty conditions
- Invalid operator combinations
- Boundary value cases
Would you like me to help generate additional test cases for these scenarios?
packages/sdk/src/convertQuerytoClause.ts (1)
Line range hint
1-319: Consider architectural improvements for better maintainability.The file handles complex query transformations and could benefit from:
- Breaking down into smaller, focused modules (e.g., separate files for operators, primitives, and clause builders)
- Adding more comprehensive error handling with custom error types
- Enhancing documentation with examples for common use cases
Would you like help with restructuring this into smaller, more focused modules?
packages/sdk/src/types.ts (1)
Line range hint
163-178: Document migration path for breaking changes.The renaming of
QueryTypetoModelQueryTypeand the introduction of the new unionQueryTypeimprove type clarity. However, these are breaking changes that require migration from existing codebases.Consider adding a comment or updating documentation to guide users in migrating from the old type structure to the new one. For example:
/** * @deprecated Use ModelQueryType for model-specific queries * Migration guide: * 1. Rename existing QueryType<T> to ModelQueryType<T> * 2. Use new QueryType<T> when you need either base or model queries */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- packages/sdk/src/example/index.ts (1 hunks)
- packages/sdk/src/tests/convertQueryToClause.test.ts (6 hunks)
- packages/sdk/src/convertQuerytoClause.ts (3 hunks)
- packages/sdk/src/getEntities.ts (1 hunks)
- packages/sdk/src/getEventMessages.ts (1 hunks)
- packages/sdk/src/types.ts (3 hunks)
🧰 Additional context used
🔇 Additional comments (9)
packages/sdk/src/getEventMessages.ts (1)
52-52: Verify consistent handling of hashed keys across the codebase.The change to include hashed keys aligns with similar changes in
getEntities.ts, but we should verify this behavior is consistent across all query-related functions.#!/bin/bash # Description: Search for other instances of dont_include_hashed_keys to ensure consistent configuration # Expected: All instances should be set to false or removed # Search for dont_include_hashed_keys usage rg -A 2 "dont_include_hashed_keys" # Search for Query type/interface definition ast-grep --pattern 'interface Query { $$$ dont_include_hashed_keys?: $_ $$$ }'Note: Including hashed keys will increase the response payload size. Consider:
- Adding documentation about the memory implications
- Implementing response compression if not already in place
- Adding pagination best practices to the documentation
packages/sdk/src/getEntities.ts (1)
52-52: Verify the intentional removal of conditional hashed keys behavior.The change to always include hashed keys (
dont_include_hashed_keys: false) removes the previous conditional behavior based onquery.entityIds. While this simplifies the logic, it may have implications:
- Response payload size will increase as hashed keys are always included
- This could affect performance for large result sets
- Existing consumers might be impacted if they relied on hashed keys being excluded in certain scenarios
Let's verify the usage patterns in the codebase:
Consider adding a comment in the code or updating the JSDoc to document this behavioral change, particularly noting that hashed keys are always included in the response.
✅ Verification successful
Let me try a different approach to verify the usage and impact of this change.
Based on the search results, I can now provide a conclusive response about the change to
dont_include_hashed_keys.The change to always include hashed keys is consistent with existing usage patterns
The codebase search reveals that:
- In
packages/state/src/recs/index.ts, all direct calls toclient.getEntities()already explicitly setdont_include_hashed_keys: false- The change aligns with the existing behavior across the codebase
- No consumers were found that rely on hashed keys being excluded
The modification appears to be a refactoring that standardizes the behavior that was already established in practice, rather than introducing a breaking change.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for patterns of consuming code that might be affected by this change rg -A 5 "getEntities.*dont_include_hashed_keys" --type tsLength of output: 59
Script:
#!/bin/bash # Search for all usages of getEntities function rg -A 10 "getEntities\(" --type ts # Also check for any tests that might reveal expected behavior fd "test|spec" -e ts -e js -X rg -l "getEntities" # Look for the function definition to understand the full context ast-grep --pattern 'export $_ function getEntities($_) { $$$ }'Length of output: 14030
packages/sdk/src/__example__/index.ts (1)
Line range hint
42-76: LGTM! Well-structured schema definition.The schema implementation properly matches the type definitions and provides appropriate default values. The organization into 'world' and 'universe' namespaces provides good separation of concerns.
packages/sdk/src/__tests__/convertQueryToClause.test.ts (2)
7-7: LGTM: Import addition enhances type safetyThe addition of
SchemaTypeimport aligns with the enhanced type system.
30-30: Verify operator changes match implementationThe operator changes from "And" to "Or" appear consistent across test cases. Let's verify this aligns with the main implementation.
Also applies to: 84-84, 142-142, 113-128
✅ Verification successful
The output shows that the "Or" operator is indeed present in the main implementation file
convertQuerytoClause.ts, matching the test cases. However, let me verify the complete context by checking both the implementation and test files for a comprehensive comparison of operator usage.This will help us confirm that the operator changes in the test cases are fully aligned with the implementation's logic and usage patterns.
Based on the comprehensive output, I can now generate the final response as we have sufficient evidence to verify the operator changes.
Operator changes are correctly implemented and consistent
The operator changes in the test cases are perfectly aligned with the implementation:
- The main implementation in
convertQuerytoClause.tssupports both "Or" and "And" operators in composite clauses- The test cases' operator changes to "Or" match the implementation's behavior, particularly in the composite clause construction
- The nested operator usage in test cases (including the complex nested "Or" operators) is supported by the implementation's composite clause handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify operator changes in convertQueryToClause implementation # Expected: Should find "Or" operator usage in the main implementation # Check the main implementation file rg -A 5 'operator:\s*"Or"' packages/sdk/src/convertQuerytoClause.tsLength of output: 227
Script:
#!/bin/bash # Get a broader context of operator usage in both files echo "=== Implementation file ===" rg -B 2 -A 2 'operator:' packages/sdk/src/convertQuerytoClause.ts echo -e "\n=== Test file ===" rg -B 2 -A 2 'operator:' packages/sdk/src/__tests__/convertQueryToClause.test.tsLength of output: 4115
packages/sdk/src/convertQuerytoClause.ts (2)
161-162: LGTM: Consistent casing for logical operators.The update to use title case ("And"/"Or") improves consistency with the Torii client types.
36-37: Verify the impact of changing AND to OR operator.This change fundamentally alters how multiple clauses are combined, switching from requiring all conditions to match (AND) to allowing any condition to match (OR). This could significantly broaden query results and potentially break existing queries.
Let's verify the impact:
packages/sdk/src/types.ts (2)
158-160: LGTM! Good separation of concerns.The introduction of
BaseQueryTypeas a foundation for query types is a clean design choice that promotes better type organization.
93-93: Verify compatibility with torii.LogicalOperator.The change to use
torii.LogicalOperatorpromotes better maintainability. However, let's verify that the imported type matches the expected values.✅ Verification successful
Let me gather more information about the LogicalOperator type and its implementation.
This will help us understand:
- Where the torii types are imported from
- Any local definitions of logical operators
- The actual implementation in convertQuerytoClause.ts to see how these operators are used
Let me check the actual torii-client package to verify the LogicalOperator type definition.
This will help us:
- Find the type definition in the dependency
- Verify the package dependency
- Check test cases for usage examples
Based on the evidence gathered so far:
- The code consistently imports
toriifrom "@dojoengine/torii-client" (a workspace dependency)- The implementation in
convertQuerytoClause.tsshows thatLogicalOperatoris used with only two values: "And" and "Or"- The type is used consistently across the codebase in a type-safe manner
- The package.json confirms @dojoengine/torii-client as a workspace dependency
Let me generate the final response:
The usage of torii.LogicalOperator is correctly implemented
The change to use
torii.LogicalOperatoris safe and well-implemented. The codebase consistently uses only "And" and "Or" operators, which are the standard logical operators supported by the torii client. The implementation inconvertQuerytoClause.tsconfirms proper type usage and mapping.The verification is conclusive because:
- The implementation shows clear mapping of logical operators
- The type is used consistently across the codebase
- The dependency is properly managed as a workspace package
- There's no evidence of any compatibility issues or mismatched types
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the LogicalOperator type definition in torii-client # and its usage across the codebase # Test 1: Find the LogicalOperator type definition echo "Looking for LogicalOperator definition in torii-client:" rg -p "type LogicalOperator" # Test 2: Find all usages of LogicalOperator echo "Finding all usages of LogicalOperator:" rg -p "LogicalOperator"Length of output: 481
Script:
#!/bin/bash # Let's find more context about LogicalOperator and its usage # Find the torii import statement echo "Finding torii import:" rg -p "import.*torii.*from" # Look for any enum or type definitions related to logical operators echo "Finding logical operator related definitions:" rg -p "(?i)(type|enum|interface).*logical.*operator" -A 5 # Check the convertQuerytoClause.ts file for more context echo "Checking convertQuerytoClause.ts implementation:" cat packages/sdk/src/convertQuerytoClause.tsLength of output: 13418
Script:
#!/bin/bash # Let's examine the node_modules for torii-client type definitions echo "Looking for LogicalOperator in node_modules:" fd -e d.ts -e ts "index.d.ts" -x grep -l "LogicalOperator" # Also check package.json to confirm the dependency echo "Checking package.json for torii-client dependency:" cat packages/sdk/package.json # Check if there are any tests using LogicalOperator echo "Checking tests for LogicalOperator usage:" rg -p "LogicalOperator.*=.*" packages/sdk/src/__tests__/Length of output: 2223
| console.log("result", result); | ||
|
|
There was a problem hiding this comment.
Remove debug console.log statement
Debug logging statements should be removed before merging to production.
- console.log("result", result);
-📝 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.
| console.log("result", result); |
| @@ -299,6 +299,7 @@ function convertToPrimitive(value: any): torii.MemberValue { | |||
| * @throws {Error} - If the operator is unsupported. | |||
| */ | |||
| function convertOperator(operator: string): torii.ComparisonOperator { | |||
| console.log(operator); | |||
There was a problem hiding this comment.
Remove or improve debug logging.
The console.log statement appears to be debugging code that was accidentally committed. If logging is needed, consider:
- Using a proper logging framework with level control
- Adding more context about what's being logged
- Moving it to debug/trace level
Remove the debug logging:
- console.log(operator);Or improve it with better context:
- console.log(operator);
+ logger.debug(`Converting query operator: ${operator}`);Committable suggestion was skipped due to low confidence.
Summary by CodeRabbit