fix: release build issue with sdk + querybuilder type #340
Conversation
WalkthroughThe pull request introduces several modifications across multiple files, primarily focusing on the Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (9)
examples/example-vite-react-sdk/src/contracts.gen.ts (2)
Line range hint
47-48: Fix incorrect error message in move functionThe error message incorrectly references "spawn" instead of "move", which could be misleading during debugging.
- console.error("Error executing spawn:", error); + console.error("Error executing move:", error);
Line range hint
35-41: Improve type safety for direction parameterThe current implementation uses
anytype and a hardcoded array for direction validation. Consider using a proper TypeScript enum or union type for better type safety.+type Direction = 'None' | 'Left' | 'Right' | 'Up' | 'Down'; + -const move = async (props: { account: Account; direction: any }) => { +const move = async (props: { account: Account; direction: { type: Direction } }) => { try { return await provider.execute( props.account, { contractName: contract_name, entrypoint: "move", - calldata: [["None", "Left", "Right", "Up", "Down"].indexOf(props.direction.type)], + calldata: [Object.values(Direction).indexOf(props.direction.type)], },examples/example-vite-react-app-recs/src/dojo/setup.ts (1)
83-92: Consider adding JSDoc documentationThe new
isSessionSignatureparameter would benefit from documentation explaining its purpose and when it should be used.+ /** + * Publishes a message to the Torii network + * @param typedData - The typed data to publish + * @param signature - The signature of the typed data + * @param isSessionSignature - Whether the signature is a session signature + */ publish: ( typedData: string, signature: ArraySignatureType, isSessionSignature = false ) => {examples/example-vanillajs-phaser-recs/src/dojo/setup.ts (2)
Line range hint
22-144: Consider enhancing error handling robustnessWhile the error handling is thorough with try-catch blocks for each initialization step, consider these improvements:
- Add more specific error messages that include context:
- console.error("Failed to create torii client:", e); + console.error("Failed to create torii client. Check network connectivity and endpoint URLs:", e);
- Consider implementing cleanup in catch blocks where necessary:
let toriiClient = null; try { toriiClient = await torii.createClient({...}); } catch (e) { if (toriiClient) { await toriiClient.dispose(); // If a dispose method exists } console.error("Failed to create torii client:", e); throw e; }
Line range hint
22-144: Enhance type safety with explicit typingThe code could benefit from more explicit TypeScript types and stricter null checks.
Consider these improvements:
- let toriiClient = null; + let toriiClient: torii.ToriiClient | null = null; - let contractModels = null; + let contractModels: ReturnType<typeof createClientComponents> | null = null; - let sync = null; + let sync: Awaited<ReturnType<typeof getSyncEntities>> | null = null;Also, consider using type guards for more robust error handling:
if (!(e instanceof Error)) { e = new Error(String(e)); } console.error("Failed to create torii client:", e.message);examples/example-vite-react-sdk/src/App.tsx (1)
Line range hint
41-73: Consider enhancing error handling and magic values.The QueryBuilder implementation looks good, but consider these improvements:
- Add type narrowing for the error object in the callback
- Extract the '0x0' check into a named constant for better maintainability
+ const EMPTY_ENTITY_ID = '0x0'; + const subscription = await sdk.subscribeEntityQuery({ query: new QueryBuilder<Schema>() .namespace("dojo_starter", (n) => n .entity("Moves", (e) => e.eq( "player", addAddressPadding(account.account.address) ) ) .entity("Position", (e) => e.is( "player", addAddressPadding(account.account.address) ) ) ) .build(), callback: (response) => { - if (response.error) { + if ('error' in response && response.error) { console.error( "Error setting up entity sync:", response.error ); } else if ( response.data && - response.data[0].entityId !== "0x0" + response.data[0].entityId !== EMPTY_ENTITY_ID ) { console.log("subscribed", response.data[0]); state.updateEntity(response.data[0]); } }, });packages/sdk/src/queryBuilder.ts (3)
3-5: Consider simplifying theFirstLevelKeystype definitionThe
FirstLevelKeystype is defined as:type FirstLevelKeys<ObjectType> = ObjectType extends object ? keyof ObjectType & (string | number) : never;This type aims to extract the first-level keys of an object, intersecting with
string | number. Sincekeyof ObjectTypealready returnsstring | number | symbol, and most object keys are strings, the intersection may be unnecessary unless you intend to excludesymbolkeys explicitly.
94-97: Simplify complex type definitions in constraint methodsThe
ismethod signature is:public is( field: FirstLevelKeys<T[keyof T & string][keyof T[keyof T & string]]>, value: any ): QueryEntity<T> { return this.addConstraint(field, value, Operator.is); }The type for
fieldis quite complex and may impact readability. Consider introducing a type alias for better clarity:type EntityFieldKeys = FirstLevelKeys< T[keyof T & string][keyof T[keyof T & string]] >;Then update the method:
public is(field: EntityFieldKeys, value: any): QueryEntity<T> { return this.addConstraint(field, value, Operator.is); }This approach enhances maintainability without changing functionality.
101-104: Apply the simplified type alias to all constraint methodsThe same complex type is used in the
eq,neq,gt,gte,lt, andltemethods. After defining theEntityFieldKeystype alias, update these methods accordingly:public eq(field: EntityFieldKeys, value: any): QueryEntity<T> { /* ... */ } public neq(field: EntityFieldKeys, value: any): QueryEntity<T> { /* ... */ } public gt(field: EntityFieldKeys, value: any): QueryEntity<T> { /* ... */ } public gte(field: EntityFieldKeys, value: any): QueryEntity<T> { /* ... */ } public lt(field: EntityFieldKeys, value: any): QueryEntity<T> { /* ... */ } public lte(field: EntityFieldKeys, value: any): QueryEntity<T> { /* ... */ }This will improve code readability and maintain consistency across your class methods.
Also applies to: 108-111, 115-118, 122-125, 129-132, 136-139
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
examples/example-vite-react-phaser-recs/src/dojo/generated/setup.tsis excluded by!**/generated/**examples/example-vite-react-pwa-recs/src/dojo/generated/setup.tsis excluded by!**/generated/**examples/example-vite-react-threejs-recs/src/dojo/generated/setup.tsis excluded by!**/generated/**
📒 Files selected for processing (8)
examples/example-vanillajs-phaser-recs/src/dojo/setup.ts(1 hunks)examples/example-vite-react-app-recs/src/dojo/setup.ts(1 hunks)examples/example-vite-react-sdk/src/App.tsx(5 hunks)examples/example-vite-react-sdk/src/contracts.gen.ts(1 hunks)examples/example-vite-react-sdk/tsconfig.app.tsbuildinfo(1 hunks)examples/example-vite-react-sdk/tsconfig.node.tsbuildinfo(1 hunks)examples/example-vite-svelte-recs/.gitignore(1 hunks)packages/sdk/src/queryBuilder.ts(3 hunks)
✅ Files skipped from review due to trivial changes (2)
- examples/example-vite-react-sdk/tsconfig.node.tsbuildinfo
- examples/example-vite-svelte-recs/.gitignore
🔇 Additional comments (9)
examples/example-vite-react-sdk/src/contracts.gen.ts (1)
5-5: Verify auto-generation configuration
The removal of the byteArray import appears intentional, but since this is an auto-generated file, please ensure the sozo build --typescript command is using the correct configuration.
✅ Verification successful
Import change is correct - no action needed
The removal of the byteArray import from contracts.gen.ts is correct as the file doesn't use this import anywhere. While other auto-generated contract files (example-vite-react-app-recs and example-vite-svelte-recs) still include this import, its absence in this specific file is appropriate since unused imports should be removed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if byteArray is used elsewhere in the codebase
# to verify if the import removal was correct
rg "byteArray" --type ts
Length of output: 939
Script:
#!/bin/bash
# Let's check if byteArray is actually used in the file under review
rg "byteArray" examples/example-vite-react-sdk/src/contracts.gen.ts
# Also check if there are any other auto-generated contract files to compare
fd "contracts.gen.ts" examples/
Length of output: 297
examples/example-vite-react-app-recs/src/dojo/setup.ts (2)
83-92: LGTM: Type safety is maintained
The implementation maintains type safety with:
- Proper typing for all parameters
- Consistent use of TypeScript's optional parameter syntax
- Correct passing of parameters to the underlying
toriiClient.publishMessage
83-92: Verify ToriiClient compatibility with session signatures
The addition of the isSessionSignature parameter looks correct, but we should verify that the toriiClient.publishMessage method supports this parameter.
✅ Verification successful
The isSessionSignature parameter is properly supported by ToriiClient
The verification shows that the publishMessage method in the SDK (packages/sdk/src/index.ts) fully supports the isSessionSignature parameter with proper typing and documentation. The implementation is consistent across multiple example applications in the codebase, confirming this is a well-integrated feature.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if ToriiClient's publishMessage method supports isSessionSignature
# Look for the method definition in the codebase
ast-grep --pattern 'publishMessage(typedData: $_, signature: $_, isSessionSignature: $_)'
# Alternatively, check for any references to session signatures in Torii
rg -A 3 'isSessionSignature|sessionSignature'
Length of output: 3899
examples/example-vanillajs-phaser-recs/src/dojo/setup.ts (1)
56-57: Verify the impact of getSyncEntities parameter changes
The parameters to getSyncEntities have been modified from [], 1000 to undefined, []. This change could affect how entities are synchronized from the world state.
Let's verify the parameter usage and potential impact:
Consider documenting the expected behavior of these parameter changes in the code comments, specifically:
- What passing
undefinedas the first parameter means for entity filtering - Why the second parameter was changed from a numeric limit to an empty array
- How this affects the synchronization behavior in the game engine
✅ Verification successful
Let me gather more information about the function signature and its documentation to understand the parameter changes better.
Let me try a different approach to find the function definition and its documentation.
Parameter changes to getSyncEntities are consistent with its API
Based on the function definition found in packages/state/src/recs/index.ts, the changes from [], 1000 to undefined, [] are valid and align with the API:
undefinedis a valid value for theclauseparameter which is optional and used for filtering entities[]is the correct type forentityKeyClauseparameter which expects an array of entity key clauses- The removed
1000value was likely thelimitparameter which has a default value of 100, making it optional
Looking at other examples in the codebase, this parameter pattern is consistently used across multiple projects:
- examples/example-vite-svelte-recs
- examples/example-vue-app-recs
- examples/example-vite-react-phaser-recs
and others all useundefinedfor the clause parameter.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the usage of getSyncEntities across the codebase and its parameter patterns
# Search for getSyncEntities usage patterns
rg -A 3 "getSyncEntities\("
# Check the type definition to understand the parameter requirements
ast-grep --pattern 'function getSyncEntities($_) {
$$$
}'
Length of output: 3543
Script:
#!/bin/bash
# Search for getSyncEntities function definition and documentation
rg -B 5 -A 10 "export async function getSyncEntities"
# Look for any type definitions or interfaces related to getSyncEntities parameters
rg -B 2 -A 2 "getSyncEntities.*interface|type.*getSyncEntities"
Length of output: 121
Script:
#!/bin/bash
# Search in the packages directory for getSyncEntities with more context
rg -B 10 -A 10 "getSyncEntities" packages/
# Look for type definitions in .d.ts files
fd -e d.ts -x rg -l "getSyncEntities"
# Search specifically in the state package where we saw the function earlier
rg -B 10 -A 10 "getSyncEntities" packages/state/
Length of output: 9823
examples/example-vite-react-sdk/src/App.tsx (2)
2-2: LGTM: Import changes align with the new QueryBuilder usage.
The addition of QueryBuilder to the imports is appropriate for the new query construction approach.
Line range hint 90-113: Verify the entity query completeness and standardize error handling.
The entity fetching implementation has a few concerns:
- The query only includes 'Moves' while the subscription includes both 'Moves' and 'Position'. Verify if this is intentional.
- Error handling differs from the subscription implementation.
Consider standardizing the implementation:
await sdk.getEntities({
query: new QueryBuilder<Schema>()
.namespace("dojo_starter", (n) =>
- n.entity("Moves", (e) =>
- e.eq(
- "player",
- addAddressPadding(account.account.address)
- )
- )
+ n
+ .entity("Moves", (e) =>
+ e.eq(
+ "player",
+ addAddressPadding(account.account.address)
+ )
+ )
+ .entity("Position", (e) =>
+ e.is(
+ "player",
+ addAddressPadding(account.account.address)
+ )
+ )
)
.build(),
callback: (resp) => {
- if (resp.error) {
+ if ('error' in resp && resp.error) {
console.error(
"resp.error.message:",
resp.error.message
);
return;
}
if (resp.data) {
state.setEntities(resp.data);
}
},
});packages/sdk/src/queryBuilder.ts (3)
88-92: Ensure consistency in the entity method between classes
In the QueryEntity class, the entity method is defined similarly:
public entity(
name: FirstLevelKeys<T[keyof T & string]>,
cb: (entity: QueryEntity<T>) => void
): QueryEntity<T> {
return this.parent.entity(name, cb);
}Confirm that the type parameters used here are consistent with those in the Namespace class to maintain uniform behavior across your query builder.
144-148: Check type safety in the addConstraint method
The addConstraint method is defined as:
private addConstraint(
field: FirstLevelKeys<T[keyof T & string][keyof T[keyof T & string]]>,
value: any,
op: Operator
): QueryEntity<T> {
this.constraints.set(field as string, new Constraint(op, value));
return this;
}Ensure that casting field to a string is safe and that all possible keys are accounted for. If field could be a number or symbol, this cast might cause issues. Revisiting the FirstLevelKeys type or handling different key types explicitly could enhance type safety.
Run the following script to identify any potential issues with key types:
✅ Verification successful
Let me gather more information about the FirstLevelKeys type and its usage.
Type casting in addConstraint is safe and well-constrained
The FirstLevelKeys type is explicitly defined to only allow string or number keys:
type FirstLevelKeys<ObjectType> = ObjectType extends object
? keyof ObjectType & (string | number)
: never;The type cast to string in addConstraint is safe because:
- The type definition explicitly constrains keys to
string | number - Numbers are safely convertible to strings in JavaScript/TypeScript when used as object keys
- Symbols are explicitly excluded from the type definition
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find usage of `addConstraint` where `field` may not be a string.
# Expectation: No instances where `field` is not a string.
# Command: Search for `addConstraint` calls and check the type of `field`.
rg 'addConstraint\(' -A 2
Length of output: 1449
Script:
#!/bin/bash
# Search for FirstLevelKeys type definition and its usage
ast-grep --pattern 'type FirstLevelKeys$_'
# Also search for any string related code
rg "FirstLevelKeys" -B 2 -A 2
Length of output: 3510
63-67: Verify the correctness of the entity method's type parameters
In the Namespace class, the entity method signature has been updated to:
public entity(
name: FirstLevelKeys<T[keyof T & string]>,
cb: (entity: QueryEntity<T>) => void
): QueryEntity<T> {
const entity = new QueryEntity(this);
this.entities.set(name as string, entity);
cb(entity);
return entity;
}Ensure that FirstLevelKeys<T[keyof T & string]> correctly represents the intended keys of the entities within the namespace. Verifying this will help maintain type safety and prevent potential runtime errors.
You can run the following script to check for potential type issues:
✅ Verification successful
Type parameters in the entity method are correctly implemented
The verification shows that:
FirstLevelKeystype is properly defined to extract keys from nested object types- The type is consistently used across the codebase in both the
NamespaceandQueryEntityclasses - The implementation aligns with the schema type definition in
types.ts, which defines a structured schema type system - Usage examples in tests and example code demonstrate proper type inference and no type errors
The type cast as string is safely used only for internal map operations and doesn't affect the type safety of the public API.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the `entity` method's type parameters are correctly used throughout the codebase.
# Expectation: No TypeScript errors related to `entity` method usage.
# Command: Compile the TypeScript project without emitting output.
tsc --noEmit
Length of output: 108
Script:
#!/bin/bash
# Let's analyze the type usage and implementation
# Find the definition of FirstLevelKeys type
rg "type\s+FirstLevelKeys" -A 3
# Find usages of the entity method to verify type correctness
rg "\.entity\(" -B 2 -A 2
# Look for the QueryEntity class definition
ast-grep --pattern 'class QueryEntity {
$$$
}'
# Check for any type-related imports or declarations
rg "import.*type" packages/sdk/src/queryBuilder.ts
Length of output: 3839
Script:
#!/bin/bash
# Let's examine the schema type and usage context
# Check the schema type definition
rg "type MockSchemaType" -A 5 packages/sdk/src/__tests__
# Look for the complete QueryEntity class implementation
ast-grep --pattern 'class QueryEntity<$_> {
$$$
}'
# Check the types.ts file for related type definitions
cat packages/sdk/src/types.ts
# Look for any type errors or type assertions in the codebase
rg "as\s+string" packages/sdk/src/queryBuilder.ts -B 2 -A 2
Length of output: 16444
| {"root":["./src/app.tsx","./src/dojocontext.tsx","./src/bindings.ts","./src/contracts.gen.ts","./src/main.tsx","./src/usedojo.tsx","./src/usemodel.tsx","./src/usesystemcalls.ts","./src/vite-env.d.ts"],"version":"5.6.3"} |
There was a problem hiding this comment.
💡 Codebase verification
Add .tsbuildinfo files to .gitignore to prevent committing build artifacts
The verification confirms:
.tsbuildinfofiles are currently not ignored in version control- All referenced TypeScript files exist in the codebase (with slight case differences)
Add this line to your .gitignore:
*.tsbuildinfo
🔗 Analysis chain
Consider adding .tsbuildinfo files to .gitignore
The .tsbuildinfo files are TypeScript build cache files that should typically not be committed to version control. They are automatically generated during compilation and can cause unnecessary merge conflicts.
Let's verify if this file is already ignored and check the existence of the newly added TypeScript files:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify .gitignore settings and existence of TypeScript files
# Check if .tsbuildinfo is already ignored
echo "Checking .gitignore for .tsbuildinfo:"
rg "\.tsbuildinfo$" .gitignore || echo "Not ignored"
# Verify existence of the newly added TypeScript files
echo -e "\nVerifying existence of TypeScript files:"
for file in "dojocontext.tsx" "contracts.gen.ts" "usedojo.tsx" "usemodel.tsx" "usesystemcalls.ts"; do
fd -p "examples/example-vite-react-sdk/src/${file}$" || echo "Missing: $file"
done
Length of output: 1314
Summary by CodeRabbit
Release Notes
New Features
QueryBuilder.publishmethod to accept an optional session signature parameter.Bug Fixes
movefunction for better clarity.Chores
.gitignoreto exclude thedist/directory from version control.Documentation
QueryBuilderfor improved type safety and clarity.