feat: add ToriiQueryBuilder and ClauseBuilder #378
Conversation
WalkthroughThis pull request introduces several patches across various Dojo Engine packages, including the addition of two new classes, Changes
Sequence DiagramsequenceDiagram
participant User
participant ToriiQueryBuilder
participant ClauseBuilder
participant ToriiClient
User->>ToriiQueryBuilder: Create query
ToriiQueryBuilder->>ToriiQueryBuilder: Configure query parameters
ToriiQueryBuilder->>ClauseBuilder: Build query clauses
ClauseBuilder-->>ToriiQueryBuilder: Return clause
ToriiQueryBuilder->>ToriiClient: Execute query
ToriiClient-->>User: Return results
Possibly related PRs
Poem
✨ Finishing Touches
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 (
|
MartianGreed
force-pushed
the
feat/torii-query-builder
branch
from
8c138db to
b974a50
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
packages/sdk/src/clauseBuilder.ts (1)
35-36: Avoid suppressing TypeScript errors with@ts-expect-error.Using
@ts-expect-errorto suppress TypeScript errors can hide legitimate issues. It would be better to properly initializethis.clausewith a valid type or adjust the type definitions.Consider initializing
this.clauseasnullor defining a defaultClausestructure that represents an empty state.examples/example-vite-react-sdk/src/App.tsx (1)
34-53: Add error handling and cleanup to the experimental code.While the experimental code demonstrates the usage of ToriiQueryBuilder, it needs improvements before being uncommented:
- Add error handling for the fetch operation
- Add proper cleanup in useEffect
- Consider adding loading state management
Apply this diff when uncommenting:
useEffect(() => { + let mounted = true; + async function fetchToriiClause() { - const res = await sdk.client.getEntities( - new ToriiQueryBuilder() - .withClause( - new ClauseBuilder() - .keys([], [undefined], "VariableLen") - .build() - ) - .withLimit(2) - .addOrderBy(ModelsMapping.Moves, "remaining", "Desc") - .build() - ); - return res; + try { + const res = await sdk.client.getEntities( + new ToriiQueryBuilder() + .withClause( + new ClauseBuilder() + .keys([], [undefined], "VariableLen") + .build() + ) + .withLimit(2) + .addOrderBy(ModelsMapping.Moves, "remaining", "Desc") + .build() + ); + if (mounted) { + return res; + } + } catch (error) { + console.error("Error fetching entities:", error); + } } - fetchToriiClause().then(console.log); + fetchToriiClause(); + + return () => { + mounted = false; + }; });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.changeset/witty-moons-care.md(1 hunks)examples/example-vite-react-sdk/README.md(0 hunks)examples/example-vite-react-sdk/src/App.tsx(2 hunks)packages/sdk/src/__tests__/clauseBuilder.test.ts(1 hunks)packages/sdk/src/__tests__/toriiQueryBuilder.test.ts(1 hunks)packages/sdk/src/clauseBuilder.ts(1 hunks)packages/sdk/src/convertQuerytoClause.ts(1 hunks)packages/sdk/src/convertToMemberValue.ts(1 hunks)packages/sdk/src/index.ts(1 hunks)packages/sdk/src/toriiQueryBuilder.ts(1 hunks)
💤 Files with no reviewable changes (1)
- examples/example-vite-react-sdk/README.md
🧰 Additional context used
🪛 GitHub Check: build
packages/sdk/src/__tests__/toriiQueryBuilder.test.ts
[failure] 28-28: src/tests/toriiQueryBuilder.test.ts > ToriiQueryBuilder > basic query building > should create a query with default values
AssertionError: expected { limit: 100, offset: +0, …(5) } to deeply equal { limit: 100, offset: +0, …(5) }
- Expected
-
Received
Object {
"clause": undefined,
- "dont_include_hashed_keys": false,
- "dont_include_hashed_keys": true,
"entity_models": Array [],
"entity_updated_after": 0,
"limit": 100,
"offset": 0,
"order_by": Array [],
}
❯ src/tests/toriiQueryBuilder.test.ts:28:27
[failure] 124-124: src/tests/toriiQueryBuilder.test.ts > ToriiQueryBuilder > timestamp and hashed keys handling > should handle hashed keys inclusion
AssertionError: expected false to be true // Object.is equality
- Expected
- Received
- true
- false
❯ src/tests/toriiQueryBuilder.test.ts:124:52
🪛 GitHub Actions: typedoc
packages/sdk/src/__tests__/toriiQueryBuilder.test.ts
[error] 28-28: Test failure: Expected dont_include_hashed_keys to be false but received true in query builder default values
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: check
🔇 Additional comments (11)
packages/sdk/src/convertToMemberValue.ts (2)
21-22: 🛠️ Refactor suggestionMaintain consistency in
MemberValuestructure for string types.While other primitive types are wrapped under
Primitive, the string type is not. For consistency and to meet API expectations, consider wrapping string values underPrimitive.Apply this diff to adjust the structure:
- return { String: value }; + return { Primitive: { String: value } };Verify if the Torii client expects string values under the
Primitivekey.
15-20:⚠️ Potential issueEnsure accurate conversion of
BigIntvalues toFelt252.The current conversion of
BigIntusestorii.cairoShortStringToFelt(value.toString()), which is intended for strings, not numericBigIntvalues. This may lead to incorrect conversions.Apply this diff to correct the conversion:
- Felt252: torii.cairoShortStringToFelt(value.toString()), + Felt252: value.toString(),Verify if
Felt252expects a numeric string or aBigInt. If it acceptsBigIntdirectly, you can pass the value without conversion:- Felt252: torii.cairoShortStringToFelt(value.toString()), + Felt252: value,packages/sdk/src/toriiQueryBuilder.ts (2)
4-118: Well-structured implementation ofToriiQueryBuilder.The
ToriiQueryBuilderclass provides a clean and fluent API for building queries. Methods are clearly defined, and default values are appropriately set.
110-117: Verify the pagination calculation inwithPagination.The
withPaginationmethod calculates the offset aspage * pageSize. If page numbering starts from 1, this calculation will skip the first page. Adjust the calculation to account for page numbers starting from 1.Apply this diff if page numbers start from 1:
- .withOffset(page * pageSize); + .withOffset((page - 1) * pageSize);Ensure that the pagination logic aligns with how page numbers are intended to be used.
packages/sdk/src/__tests__/toriiQueryBuilder.test.ts (2)
22-166: Well-structured and comprehensive test suite.The test suite is well-organized with clear sections and descriptive test names. It provides good coverage of the ToriiQueryBuilder functionality, including basic operations, order by clauses, entity models, timestamps, and method chaining.
🧰 Tools
🪛 GitHub Check: build
[failure] 28-28: src/tests/toriiQueryBuilder.test.ts > ToriiQueryBuilder > basic query building > should create a query with default values
AssertionError: expected { limit: 100, offset: +0, …(5) } to deeply equal { limit: 100, offset: +0, …(5) }
- Expected
Received
Object {
"clause": undefined,
- "dont_include_hashed_keys": false,
- "dont_include_hashed_keys": true,
"entity_models": Array [],
"entity_updated_after": 0,
"limit": 100,
"offset": 0,
"order_by": Array [],
}❯ src/tests/toriiQueryBuilder.test.ts:28:27
[failure] 124-124: src/tests/toriiQueryBuilder.test.ts > ToriiQueryBuilder > timestamp and hashed keys handling > should handle hashed keys inclusion
AssertionError: expected false to be true // Object.is equality
- Expected
- Received
- true
- false
❯ src/tests/toriiQueryBuilder.test.ts:124:52
🪛 GitHub Actions: typedoc
[error] 28-28: Test failure: Expected dont_include_hashed_keys to be false but received true in query builder default values
120-125:⚠️ Potential issueFix inverted logic in
includeHashedKeysmethod.The test is failing because
includeHashedKeys(false)setsdont_include_hashed_keystofalse, but it should betrue. The implementation seems to have inverted logic.const builder = new ToriiQueryBuilder<TestModels>(); - const query = builder.includeHashedKeys(false).build(); + const query = builder.includeHashedKeys(true).build(); expect(query.dont_include_hashed_keys).toBe(true);Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Check: build
[failure] 124-124: src/tests/toriiQueryBuilder.test.ts > ToriiQueryBuilder > timestamp and hashed keys handling > should handle hashed keys inclusion
AssertionError: expected false to be true // Object.is equality
- Expected
- Received
- true
- false
❯ src/tests/toriiQueryBuilder.test.ts:124:52
packages/sdk/src/__tests__/clauseBuilder.test.ts (1)
32-211: Comprehensive test coverage for ClauseBuilder.The test suite effectively covers all major functionalities of the ClauseBuilder:
- Key-based queries with different pattern matching
- Member clauses with various value types
- Complex composite clauses with logical operators
The tests are well-organized and provide good validation of the builder's capabilities.
packages/sdk/src/index.ts (1)
12-13: Clean integration of new modules.The new exports for
clauseBuilderandtoriiQueryBuilderare properly integrated following the existing pattern.packages/sdk/src/convertQuerytoClause.ts (1)
6-6: Good refactoring of value conversion logic.Moving
convertToPrimitiveto a separate file improves code organization and maintainability.examples/example-vite-react-sdk/src/App.tsx (1)
271-271: LGTM! Consistent padding for entity IDs.The addition of
addAddressPaddingensures consistent display of entity IDs in the table..changeset/witty-moons-care.md (1)
1-15: Verify the completeness of package updates.The changeset correctly identifies all affected packages and provides a clear description of the changes. However, consider adding more details about:
- The experimental nature of the features
- Any breaking changes or migration steps
- Usage examples or documentation references
Run this script to ensure all affected packages are included:
✅ Verification successful
Changeset is complete and accurate
The changeset correctly includes all affected packages. The new query builders are implemented in the SDK package and all dependent packages are properly versioned. Example applications using these features don't require version updates as they're not published packages.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all packages affected by ToriiQueryBuilder and ClauseBuilder are included in the changeset. # Test: Search for imports of the new query builders rg -l "ToriiQueryBuilder|ClauseBuilder" | grep -v "^\.changeset/" # Test: Search for package.json files that might need updates fd "package.json" | xargs rg -l "@dojoengine/(sdk|torii-client)"Length of output: 981
| "ComposeClause is empty. Add .or([clause]) or .and([clause])" | ||
| ); |
There was a problem hiding this comment.
Correct typos and grammatical errors in error messages.
There are typos and grammatical errors in the error messages:
- Line 119: "ComposeClause" should be "CompositeClause" to match the class name.
- Line 162: "CompositeClause is not properly build" should be "CompositeClause is not properly built".
Apply this diff to fix the errors:
- "ComposeClause is empty. Add .or([clause]) or .and([clause])"
+ "CompositeClause is empty. Add .or([clause]) or .and([clause])"
- throw new Error("CompositeClause is not properly build");
+ throw new Error("CompositeClause is not properly built");Also applies to: 162-162
| return { | ||
| Composite: { | ||
| operator: "Or", | ||
| clauses: this.orClauses, | ||
| }, | ||
| }; | ||
| } | ||
|
|
There was a problem hiding this comment.
Fix logical errors in conditional expressions in the build method.
The conditional checks in the build method are incorrect because arrays are always truthy. The conditions should check the length of the arrays instead.
Apply this diff to correct the conditional expressions:
- if (this.orClauses && this.andClauses.length === 0) {
+ if (this.orClauses.length > 0 && this.andClauses.length === 0) {
- if (this.andClauses && this.orClauses.length === 0) {
+ if (this.andClauses.length > 0 && this.orClauses.length === 0) {
- if (this.andClauses && this.orClauses) {
+ if (this.andClauses.length > 0 && this.orClauses.length > 0) {Also applies to: 134-142, 145-160
| * Start a composite clause chain | ||
| */ | ||
| compose(): CompositeBuilder<T> { | ||
| return new CompositeBuilder<T>(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Export the CompositeBuilder class for external use.
The compose method returns a new CompositeBuilder, but since CompositeBuilder is not exported, it cannot be accessed outside this module. Exporting it allows users to build composite clauses.
Apply this diff to export the CompositeBuilder class:
-class CompositeBuilder<T extends Record<string, Record<string, any>>> {
+export class CompositeBuilder<T extends Record<string, Record<string, any>>> {Also applies to: 103-103
| expect(query).toEqual({ | ||
| limit: 100, | ||
| offset: 0, | ||
| clause: undefined, | ||
| dont_include_hashed_keys: false, | ||
| order_by: [], | ||
| entity_models: [], | ||
| entity_updated_after: 0, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Fix test assertion to match implementation.
The test is failing because it expects dont_include_hashed_keys to be false by default, but the implementation returns true. Either:
- Update the test assertion to match the implementation's default value, or
- Update the implementation to match the expected default value.
expect(query).toEqual({
limit: 100,
offset: 0,
clause: undefined,
- dont_include_hashed_keys: false,
+ dont_include_hashed_keys: true,
order_by: [],
entity_models: [],
entity_updated_after: 0,
});📝 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.
| expect(query).toEqual({ | |
| limit: 100, | |
| offset: 0, | |
| clause: undefined, | |
| dont_include_hashed_keys: false, | |
| order_by: [], | |
| entity_models: [], | |
| entity_updated_after: 0, | |
| }); | |
| }); | |
| expect(query).toEqual({ | |
| limit: 100, | |
| offset: 0, | |
| clause: undefined, | |
| dont_include_hashed_keys: true, | |
| order_by: [], | |
| entity_models: [], | |
| entity_updated_after: 0, | |
| }); | |
| }); |
🧰 Tools
🪛 GitHub Check: build
[failure] 28-28: src/tests/toriiQueryBuilder.test.ts > ToriiQueryBuilder > basic query building > should create a query with default values
AssertionError: expected { limit: 100, offset: +0, …(5) } to deeply equal { limit: 100, offset: +0, …(5) }
- Expected
-
Received
Object {
"clause": undefined,
- "dont_include_hashed_keys": false,
- "dont_include_hashed_keys": true,
"entity_models": Array [],
"entity_updated_after": 0,
"limit": 100,
"offset": 0,
"order_by": Array [],
}
❯ src/tests/toriiQueryBuilder.test.ts:28:27
🪛 GitHub Actions: typedoc
[error] 28-28: Test failure: Expected dont_include_hashed_keys to be false but received true in query builder default values
MartianGreed
force-pushed
the
feat/torii-query-builder
branch
from
b974a50 to
5f335d0
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
packages/sdk/src/__tests__/clauseBuilder.test.ts (1)
32-211: Enhance test coverage with edge cases and error conditions.The test suite should include additional test cases:
- Error handling for empty arrays in
keysmethod- Error handling for undefined/null values in
wheremethod- Validation of error messages from
buildmethod when clause is emptyExample test cases to add:
it("should throw error for empty keys array", () => { const builder = new ClauseBuilder<TestModels>(); expect(() => builder.keys(["dojo_starter-Moves"], []).build() ).toThrow(); }); it("should throw error for empty clause", () => { const builder = new ClauseBuilder<TestModels>(); expect(() => builder.build()).toThrow("You cannot build an empty Clause"); });packages/sdk/src/toriiQueryBuilder.ts (1)
4-12: Extract default options to configurable constants.The default values should be configurable to allow customization without modifying the code.
+export const DEFAULT_QUERY_LIMIT = 100; +export const DEFAULT_QUERY_OFFSET = 0; + const defaultToriiOptions = { - limit: 100, // default limit - offset: 0, + limit: DEFAULT_QUERY_LIMIT, + offset: DEFAULT_QUERY_OFFSET, clause: undefined, dont_include_hashed_keys: true, order_by: [], entity_models: [], entity_updated_after: 0, };packages/sdk/src/__tests__/toriiQueryBuilder.test.ts (1)
155-184: Enhance chaining tests with more combinations.The chaining test could be more comprehensive by testing different order of operations and all possible combinations of chainable methods.
Example additional test:
it("should support different chaining order", () => { const builder = new ToriiQueryBuilder<TestModels>(); const query = builder .addOrderBy("dojo_starter-Position", "x", "Asc") .withLimit(10) .includeHashedKeys() .withOffset(20) .build(); expect(query).toEqual({ limit: 10, offset: 20, clause: undefined, dont_include_hashed_keys: false, order_by: [{ model: "dojo_starter-Position", member: "x", direction: "Asc" }], entity_models: [], entity_updated_after: 0 }); });packages/sdk/src/convertQuerytoClause.ts (1)
Line range hint
145-284: Consider enhancing error handling for operator conversion.While the functions are well-implemented, consider adding more detailed error messages for unsupported operators to help developers debug query issues more effectively.
Example enhancement for the
convertOperatorfunction:function convertOperator(operator: string): torii.ComparisonOperator { switch (operator) { case "$eq": return "Eq"; // ... other cases ... default: - throw new Error(`Unsupported operator: ${operator}`); + throw new Error( + `Unsupported operator: ${operator}. ` + + `Supported operators are: $eq, $neq, $gt, $gte, $lt, $lte, $in, $nin` + ); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.changeset/witty-moons-care.md(1 hunks)examples/example-vite-react-sdk/README.md(0 hunks)examples/example-vite-react-sdk/src/App.tsx(2 hunks)packages/sdk/src/__tests__/clauseBuilder.test.ts(1 hunks)packages/sdk/src/__tests__/toriiQueryBuilder.test.ts(1 hunks)packages/sdk/src/clauseBuilder.ts(1 hunks)packages/sdk/src/convertQuerytoClause.ts(1 hunks)packages/sdk/src/convertToMemberValue.ts(1 hunks)packages/sdk/src/index.ts(1 hunks)packages/sdk/src/toriiQueryBuilder.ts(1 hunks)
💤 Files with no reviewable changes (1)
- examples/example-vite-react-sdk/README.md
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/sdk/src/index.ts
- packages/sdk/src/convertToMemberValue.ts
- examples/example-vite-react-sdk/src/App.tsx
- .changeset/witty-moons-care.md
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: check
🔇 Additional comments (6)
packages/sdk/src/clauseBuilder.ts (3)
119-120: Fix typos in error messages.The error messages contain typos that need to be corrected:
- "ComposeClause" should be "CompositeClause"
- "not properly build" should be "not properly built"
Also applies to: 161-162
124-132: Fix logical errors in array checks.The conditional checks in the build method are incorrect. Arrays are always truthy, so the conditions should check array lengths instead.
Also applies to: 134-142, 144-160
102-102: Export CompositeBuilder class.The CompositeBuilder class should be exported to allow external use when building composite clauses.
packages/sdk/src/convertQuerytoClause.ts (3)
Line range hint
15-57: LGTM! Well-structured query conversion logic.The main function is well-documented and handles various cases appropriately, including special handling for "entityIds" and proper combination of multiple clauses.
Line range hint
59-143: LGTM! Comprehensive model processing logic.The function effectively handles complex query conditions, including special "$is" conditions, and properly generates corresponding clauses.
6-6: Verify the imported function's compatibility with existing usage.The removal of the local
convertToPrimitiveimplementation in favor of importing it from a separate module is a good refactoring decision. However, we should ensure that the imported function maintains the same behavior and handles all the primitive types correctly.Let's verify the implementation of the imported function:
✅ Verification successful
Import change is verified and compatible ✓
The imported
convertToPrimitivefunction from./convertToMemberValueprovides complete support for all required primitive types (U32, Bool, Felt252, List) and maintains consistent behavior across its usage in query clause conversion.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the implementation of convertToPrimitive in the imported module # and its usage across the codebase # Check the implementation echo "Checking convertToPrimitive implementation:" rg -A 10 "export.*convertToPrimitive" "./packages/sdk/src/convertToMemberValue.ts" # Check all usages to ensure consistent behavior echo "Checking all usages:" rg "convertToPrimitive\(" "./packages/sdk"Length of output: 1210
| it("should set limit and offset", () => { | ||
| const builder = new ToriiQueryBuilder<TestModels>(); | ||
| const query = builder.withLimit(10).withOffset(20).build(); | ||
|
|
||
| expect(query.limit).toBe(10); | ||
| expect(query.offset).toBe(20); | ||
| }); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error condition tests for invalid inputs.
The test suite should include tests for invalid inputs to ensure proper error handling.
Example test cases to add:
it("should throw error for negative limit", () => {
const builder = new ToriiQueryBuilder<TestModels>();
expect(() => builder.withLimit(-10)).toThrow("Limit must be a positive number");
});
it("should throw error for negative offset", () => {
const builder = new ToriiQueryBuilder<TestModels>();
expect(() => builder.withOffset(-20)).toThrow("Offset must be a non-negative number");
});
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (16)
scripts/build-examples.sh (1)
18-18: Add a comment to indicate experimental status.Since this is an experimental SDK example, it would be helpful to add a comment indicating its status.
+ # Experimental SDK example - not recommended for production use "examples/example-vite-experimental-sdk"examples/example-vite-experimental-sdk/vite.config.ts (1)
6-7: Consider adding more documentation for experimental features.While the link to Vite's config documentation is helpful, consider adding comments explaining:
- Why WebAssembly support is needed
- What features require top-level await
- Any limitations of the experimental SDK setup
examples/example-vite-experimental-sdk/tsconfig.json (2)
6-6: Consider expanding the lib array for broader compatibility.The
libarray only includes "ES2023". Consider adding DOM-related libraries if browser APIs are needed:- "lib": ["ES2023"], + "lib": ["ES2023", "DOM", "DOM.Iterable"],
21-21: Consider including test files in the compilation.The
includearray might need to include test files if they exist. Consider adding test paths:- "include": ["src", "vite.config.ts"] + "include": ["src", "vite.config.ts", "test", "**/*.test.ts"]packages/sdk/src/toriiQueryBuilder.ts (1)
19-21: Ensure deep merging of default optionsIn the constructor, when merging
defaultToriiOptionswith providedoptions, consider performing a deep merge to prevent nested objects or arrays indefaultToriiOptionsfrom being overwritten entirely if they are present inoptions.You might consider using a deep merge utility or spreading nested properties individually.
packages/sdk/src/experimental/__tests__/convertClauseToEntityKeysClause.test.ts (1)
5-22: Enhance test coverage with additional test cases.The current test suite only covers basic scenarios. Consider adding test cases for:
- Invalid clause structure
- Non-KeysClause input
- Different key patterns and models
Example additional test:
it("returns empty array for non-KeysClause", () => { const nonKeysClause = { someOtherProperty: "value" }; expect(intoEntityKeysClause(nonKeysClause as Clause)).toEqual([]); });examples/example-vite-experimental-sdk/src/theme-manager.ts (1)
9-12: Consider version control and fallback for CDN resources.Hardcoding CDN URLs with specific versions can lead to maintenance issues. Consider:
- Using environment variables for version control
- Adding fallback themes
- Implementing error handling for failed CDN loads
Example:
private readonly CDN_VERSION = process.env.HIGHLIGHT_JS_VERSION || "11.8.0"; private readonly CDN_BASE = `https://cdnjs.cloudflare.com/ajax/libs/highlight.js/${this.CDN_VERSION}/styles/`; private readonly themes = [ `${this.CDN_BASE}atom-one-dark.min.css`, `${this.CDN_BASE}atom-one-light.min.css` ];examples/example-vite-experimental-sdk/src/typescript/contracts.gen.ts (2)
15-29: Enhance error handling in actions_move function.The current error handling simply logs and re-throws the error. Consider adding more context about the failed operation to help with debugging.
} catch (error) { - console.error(error); + console.error(`Failed to execute move action with direction ${direction}:`, error); throw error; }
39-50: Enhance error handling in actions_spawn function.Similar to the move action, the error handling could be improved with more context.
} catch (error) { - console.error(error); + console.error('Failed to execute spawn action:', error); throw error; }examples/example-vite-experimental-sdk/src/main.ts (1)
13-26: Consider extracting domain configuration to config file.The domain configuration is hardcoded. Consider moving it to
dojoConfig.tsfor consistency with other configuration values.examples/example-vite-experimental-sdk/src/update-manager.ts (1)
66-78: Add ARIA labels for better accessibility.The copy button lacks proper accessibility attributes.
copyButton.textContent = "copy"; +copyButton.setAttribute('aria-label', 'Copy to clipboard'); +copyButton.setAttribute('role', 'button');🧰 Tools
🪛 Biome (1.9.4)
[error] 72-72: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 76-76: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
examples/example-vite-experimental-sdk/src/typescript/models.gen.ts (2)
66-73: Consider using string literal type for Direction.Instead of using a generic string type for Direction values, consider using string literal types for better type safety:
-export type Direction = { - Left: string; - Right: string; - Up: string; - Down: string; -}; +export type Direction = { + Left: ''; + Right: ''; + Up: ''; + Down: ''; +};
88-160: Consider extracting default values to constants.The schema initialization contains repeated default values that could be extracted into named constants for better maintainability.
+const DEFAULT_DIRECTION = new CairoCustomEnum({ + Left: '', + Right: undefined, + Up: undefined, + Down: undefined, +}); export const schema: SchemaType = { dojo_starter: { DirectionsAvailable: { fieldOrder: ["player", "directions"], player: "", - directions: [ - new CairoCustomEnum({ - Left: "", - Right: undefined, - Up: undefined, - Down: undefined, - }), - ], + directions: [DEFAULT_DIRECTION], }, // Apply similar changes to other occurrencesexamples/example-vite-experimental-sdk/src/style.css (2)
39-41: Consider adding transform-style: preserve-3d for better performance.When using
will-change: filter, consider addingtransform-style: preserve-3dto potentially improve rendering performance..logo { height: 6em; padding: 1.5em; will-change: filter; + transform-style: preserve-3d; transition: filter 300ms; }
57-74: Add :disabled state styles for buttons.The button styles lack styling for the disabled state, which is important for user feedback.
button { /* existing styles */ } +button:disabled { + opacity: 0.6; + cursor: not-allowed; + border-color: transparent; +}packages/sdk/src/clauseBuilder.ts (1)
34-37: Document the @ts-expect-error suppression.Please add a more detailed comment explaining why the type error suppression is necessary and safe here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
examples/example-vite-experimental-sdk/public/vite.svgis excluded by!**/*.svgpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (24)
examples/example-vite-experimental-sdk/.gitignore(1 hunks)examples/example-vite-experimental-sdk/dojoConfig.ts(1 hunks)examples/example-vite-experimental-sdk/index.html(1 hunks)examples/example-vite-experimental-sdk/package.json(1 hunks)examples/example-vite-experimental-sdk/src/main.ts(1 hunks)examples/example-vite-experimental-sdk/src/style.css(1 hunks)examples/example-vite-experimental-sdk/src/theme-manager.ts(1 hunks)examples/example-vite-experimental-sdk/src/typescript/contracts.gen.ts(1 hunks)examples/example-vite-experimental-sdk/src/typescript/models.gen.ts(1 hunks)examples/example-vite-experimental-sdk/src/update-manager.ts(1 hunks)examples/example-vite-experimental-sdk/src/vite-env.d.ts(1 hunks)examples/example-vite-experimental-sdk/tsconfig.json(1 hunks)examples/example-vite-experimental-sdk/vite.config.ts(1 hunks)packages/sdk/package.json(1 hunks)packages/sdk/src/__tests__/clauseBuilder.test.ts(1 hunks)packages/sdk/src/__tests__/parseEntities.test.ts(4 hunks)packages/sdk/src/clauseBuilder.ts(1 hunks)packages/sdk/src/experimental/__tests__/convertClauseToEntityKeysClause.test.ts(1 hunks)packages/sdk/src/experimental/convertClauseToEntityKeysClause.ts(1 hunks)packages/sdk/src/experimental/index.ts(1 hunks)packages/sdk/src/parseEntities.ts(2 hunks)packages/sdk/src/toriiQueryBuilder.ts(1 hunks)packages/sdk/tsup.config.ts(1 hunks)scripts/build-examples.sh(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- examples/example-vite-experimental-sdk/src/vite-env.d.ts
- examples/example-vite-experimental-sdk/index.html
- examples/example-vite-experimental-sdk/.gitignore
- examples/example-vite-experimental-sdk/package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/sdk/src/tests/clauseBuilder.test.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/sdk/src/experimental/index.ts
[error] 23-23: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
[error] 34-34: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
examples/example-vite-experimental-sdk/src/update-manager.ts
[error] 72-72: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 76-76: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: check
- GitHub Check: build
🔇 Additional comments (21)
scripts/build-examples.sh (1)
18-18: Verify the new example directory configuration.The new experimental example has been added to the build script. Let's verify its setup:
✅ Verification successful
Directory and build configuration verified successfully
The new example directory is properly set up with the required build script (
tsc && vite build).🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the new example directory exists and has the required build configuration. # Check if the directory exists if [ ! -d "examples/example-vite-experimental-sdk" ]; then echo "Error: examples/example-vite-experimental-sdk directory not found" exit 1 fi # Check if package.json exists and contains a build script if [ -f "examples/example-vite-experimental-sdk/package.json" ]; then echo "Checking build script in package.json..." cat examples/example-vite-experimental-sdk/package.json | jq '.scripts.build' else echo "Error: package.json not found in examples/example-vite-experimental-sdk" exit 1 fiLength of output: 309
examples/example-vite-experimental-sdk/vite.config.ts (2)
7-17: LGTM! Well-structured Vite configuration.The configuration is well-organized with appropriate plugins, path resolution, and build target settings.
1-4: Consider pinning plugin versions in package.json.While the imports are correct, it's recommended to specify exact versions for
vite-plugin-wasmandvite-plugin-top-level-awaitin package.json to ensure build consistency across environments.Let's check the current package.json for these dependencies:
examples/example-vite-experimental-sdk/tsconfig.json (3)
1-22: Overall configuration looks good with modern TypeScript features.The configuration enables strict type checking and modern ECMAScript features, which is great for maintaining code quality. The bundler mode settings are appropriate for a Vite project.
10-10: Verify bundler compatibility with TypeScript extensions.
allowImportingTsExtensions: truemight cause issues with some bundlers. Ensure your bundler configuration correctly handles TypeScript extensions.
7-7: Consider the implications of skipLibCheck.While
skipLibCheck: trueimproves compilation speed, it might hide type issues from dependencies. Consider enabling it only in development if faster builds are needed.✅ Verification successful
skipLibCheck setting is appropriate for this project
The
skipLibCheck: truesetting is justified here as:
- The codebase demonstrates strong typing practices with no type assertions or suppressions
- Type checking is properly handled during builds with
tsup --dts-resolve- The setting helps optimize development workflow in a large monorepo with multiple packages
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for type issues in node_modules that might be hidden fd -e d.ts . node_modules -x tsc --noEmit {} 2>&1 | grep -i "error TS"Length of output: 69
Script:
#!/bin/bash # Check dependencies in package.json echo "=== Checking package.json dependencies ===" fd package.json --exec cat {} # Look for type-related comments and potential type issues echo -e "\n=== Checking for type-related comments ===" rg -i "type|typescript|@types" -g "!*.d.ts" -g "!*.map" --type ts # Look for type assertion or suppression patterns echo -e "\n=== Checking for type assertions and suppressions ===" ast-grep --pattern 'as any' rg "@ts-ignore|@ts-nocheck" --type tsLength of output: 79542
packages/sdk/src/toriiQueryBuilder.ts (3)
26-29: Add input validation forwithLimitmethodThe
withLimitmethod currently does not validate thelimitparameter. It's advisable to ensure thatlimitis a positive integer to prevent potential issues when negative or zero values are provided.Apply this diff to add input validation:
withLimit(limit: number): ToriiQueryBuilder<T> { + if (limit <= 0) { + throw new Error("Limit must be a positive integer"); + } this.query.limit = limit; return this; }
34-37: Add input validation forwithOffsetmethodSimilarly, the
withOffsetmethod should validate that theoffsetparameter is a non-negative integer to avoid unexpected behavior with negative values.Apply this diff to add input validation:
withOffset(offset: number): ToriiQueryBuilder<T> { + if (offset < 0) { + throw new Error("Offset must be a non-negative integer"); + } this.query.offset = offset; return this; }
99-102: Add input validation forupdatedAftermethodThe
updatedAftermethod should include validation to ensure thetimestampparameter is non-negative, as negative timestamps may not be valid.Apply this diff to add input validation:
updatedAfter(timestamp: number): ToriiQueryBuilder<T> { + if (timestamp < 0) { + throw new Error("Timestamp must be a non-negative number"); + } this.query.entity_updated_after = timestamp; return this; }examples/example-vite-experimental-sdk/dojoConfig.ts (1)
1-7: LGTM!The configuration is correctly initialized, and the code is clear and concise.
packages/sdk/tsup.config.ts (1)
12-12: Verify inclusion of experimental entry point in buildBy adding
"src/experimental": "src/experimental/index.ts"to the build configuration, you are including experimental features in the packaged output. Ensure this is intentional and that appropriate warnings or documentation are provided for users regarding the stability of these features.If this is intended for internal testing or development purposes, consider excluding it from the production build or clearly documenting its experimental status.
packages/sdk/src/experimental/index.ts (1)
23-31:⚠️ Potential issueImprove type safety and add error handling for subscriptions.
The subscription methods use generic Function type and lack proper error handling.
- subscribeEntities: async (query: torii.Query, callback: Function) => { + subscribeEntities: async ( + query: torii.Query, + callback: (entity: torii.Entity) => void + ) => { + try { return [ parseEntities(await client.getEntities(query)), client.onEntityUpdated( intoEntityKeysClause(query.clause), callback ), ]; + } catch (error) { + console.error("Failed to subscribe to entities:", error); + throw error; + } }, - subscribeEvents: async ( - query: torii.Query, - callback: Function, - historical: boolean = false - ) => { + subscribeEvents: async ( + query: torii.Query, + callback: (event: torii.Event) => void, + historical: boolean = false + ) => { + try { return [ parseEntities(await client.getEntities(query)), client.onEventMessageUpdated( intoEntityKeysClause(query.clause), historical, callback ), ]; + } catch (error) { + console.error("Failed to subscribe to events:", error); + throw error; + } },Let's verify the usage of these subscription methods:
Also applies to: 32-45
🧰 Tools
🪛 Biome (1.9.4)
[error] 23-23: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
examples/example-vite-experimental-sdk/src/typescript/contracts.gen.ts (1)
4-60: Consider adding input validation for the direction parameter.The
actions_movefunction accepts aCairoCustomEnumfor direction but doesn't validate its contents before use.examples/example-vite-experimental-sdk/src/typescript/models.gen.ts (1)
27-28: Consider adding validation for remaining moves.The
remainingfield inMovesinterface usesBigNumberishtype but might benefit from validation to ensure it's non-negative.packages/sdk/src/clauseBuilder.ts (7)
1-30: LGTM! Well-structured type definitions.The helper types
ModelPathandGetModelTypeare well-designed to handle nested model structures and type extraction.
60-82: LGTM! Robust type-safe implementation.The
wheremethod effectively handles both single values and arrays, with proper type constraints and value conversion.
102-102: Export the CompositeBuilder class.The class needs to be exported to be usable by external code constructing composite clauses.
124-160: Fix array existence checks in conditional statements.The conditions checking array existence should instead check array lengths.
119-119: Fix typos in error messages.
- "ComposeClause" should be "CompositeClause"
- "not properly build" should be "not properly built"
Also applies to: 161-161
42-55: Consider validating keys array length.The
keysarray length should match themodelsarray length to ensure correct mapping between models and their keys.
144-159: Consider adding unit tests for complex composite scenarios.The logic for combining both AND and OR clauses is complex. Please ensure comprehensive test coverage for this scenario.
| console.log(clause); | ||
| return []; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove console.log and improve error handling.
Using console.log in production code is not recommended. Consider proper error handling or logging strategy.
- console.log(clause);
- return [];
+ // Consider throwing an error or using a proper logging strategy
+ return [];📝 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(clause); | |
| return []; | |
| // Consider throwing an error or using a proper logging strategy | |
| return []; |
| if (Object.hasOwn(clause, "Keys")) { | ||
| return [clause as unknown as EntityKeysClause]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace unsafe type casting with type guard.
The current type casting approach using as unknown as EntityKeysClause is unsafe and could lead to runtime errors if the object structure doesn't match EntityKeysClause.
Consider using a type guard:
- if (Object.hasOwn(clause, "Keys")) {
- return [clause as unknown as EntityKeysClause];
+ function isEntityKeysClause(clause: Clause): clause is EntityKeysClause {
+ return Object.hasOwn(clause, "Keys") &&
+ typeof clause.Keys === "object" &&
+ clause.Keys !== null;
+ }
+
+ if (isEntityKeysClause(clause)) {
+ return [clause];Committable suggestion skipped: line range outside the PR's diff.
| toggleTheme() { | ||
| this.currentTheme = (this.currentTheme + 1) % this.themes.length; | ||
| ( | ||
| document.querySelector('link[rel="stylesheet"]')! as HTMLLinkElement | ||
| ).href = this.themes[this.currentTheme]; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for theme switching.
The theme toggle function uses non-null assertion and lacks error handling.
toggleTheme() {
this.currentTheme = (this.currentTheme + 1) % this.themes.length;
- (
- document.querySelector('link[rel="stylesheet"]')! as HTMLLinkElement
- ).href = this.themes[this.currentTheme];
+ const stylesheet = document.querySelector('link[rel="stylesheet"]');
+ if (!stylesheet) {
+ console.error("Theme stylesheet not found");
+ return;
+ }
+ (stylesheet as HTMLLinkElement).href = this.themes[this.currentTheme];
}📝 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.
| toggleTheme() { | |
| this.currentTheme = (this.currentTheme + 1) % this.themes.length; | |
| ( | |
| document.querySelector('link[rel="stylesheet"]')! as HTMLLinkElement | |
| ).href = this.themes[this.currentTheme]; | |
| } | |
| toggleTheme() { | |
| this.currentTheme = (this.currentTheme + 1) % this.themes.length; | |
| const stylesheet = document.querySelector('link[rel="stylesheet"]'); | |
| if (!stylesheet) { | |
| console.error("Theme stylesheet not found"); | |
| return; | |
| } | |
| (stylesheet as HTMLLinkElement).href = this.themes[this.currentTheme]; | |
| } |
| setupToggleButton() { | ||
| const button = document.createElement("button"); | ||
| button.id = "themeToggle"; | ||
| button.textContent = "Toggle Theme"; | ||
| button.style.position = "fixed"; | ||
| button.style.top = "10px"; | ||
| button.style.right = "10px"; | ||
| button.style.padding = "5px"; | ||
|
|
||
| button.onclick = () => this.toggleTheme(); | ||
| document.body.appendChild(button); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance button accessibility and implement cleanup.
The theme toggle button lacks accessibility features and proper cleanup.
setupToggleButton() {
const button = document.createElement("button");
button.id = "themeToggle";
button.textContent = "Toggle Theme";
+ button.setAttribute("aria-label", "Toggle color theme");
+ button.setAttribute("role", "switch");
+ button.setAttribute("aria-checked", "false");
button.style.position = "fixed";
button.style.top = "10px";
button.style.right = "10px";
button.style.padding = "5px";
- button.onclick = () => this.toggleTheme();
+ const handleClick = () => {
+ this.toggleTheme();
+ button.setAttribute("aria-checked", String(this.currentTheme === 1));
+ };
+ button.addEventListener("click", handleClick);
document.body.appendChild(button);
+
+ // Return cleanup function
+ return () => {
+ button.removeEventListener("click", handleClick);
+ button.remove();
+ };
}📝 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.
| setupToggleButton() { | |
| const button = document.createElement("button"); | |
| button.id = "themeToggle"; | |
| button.textContent = "Toggle Theme"; | |
| button.style.position = "fixed"; | |
| button.style.top = "10px"; | |
| button.style.right = "10px"; | |
| button.style.padding = "5px"; | |
| button.onclick = () => this.toggleTheme(); | |
| document.body.appendChild(button); | |
| } | |
| setupToggleButton() { | |
| const button = document.createElement("button"); | |
| button.id = "themeToggle"; | |
| button.textContent = "Toggle Theme"; | |
| button.setAttribute("aria-label", "Toggle color theme"); | |
| button.setAttribute("role", "switch"); | |
| button.setAttribute("aria-checked", "false"); | |
| button.style.position = "fixed"; | |
| button.style.top = "10px"; | |
| button.style.right = "10px"; | |
| button.style.padding = "5px"; | |
| const handleClick = () => { | |
| this.toggleTheme(); | |
| button.setAttribute("aria-checked", String(this.currentTheme === 1)); | |
| }; | |
| button.addEventListener("click", handleClick); | |
| document.body.appendChild(button); | |
| // Return cleanup function | |
| return () => { | |
| button.removeEventListener("click", handleClick); | |
| button.remove(); | |
| }; | |
| } |
| // @ts-ignore | ||
| const client = await torii.createClient(options.client); |
There was a problem hiding this comment.
Remove @ts-ignore and fix type issues.
Using @ts-ignore masks potential type safety issues. Consider fixing the underlying type issues.
- // @ts-ignore
- const client = await torii.createClient(options.client);
+ const client = await torii.createClient(options.client as torii.ClientConfig);Committable suggestion skipped: line range outside the PR's diff.
| * @param {Object} update - The update to display as a JSON object. | ||
| */ | ||
| displayUpdate(title: string, update: any) { | ||
| const updatesDiv = document.getElementById("updates")!; |
There was a problem hiding this comment.
Add null check for updates div.
The code assumes the updates div exists. Add a null check to handle cases where the element might not be found.
- const updatesDiv = document.getElementById("updates")!;
+ const updatesDiv = document.getElementById("updates");
+ if (!updatesDiv) {
+ console.error("Updates container not found");
+ return;
+ }📝 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 updatesDiv = document.getElementById("updates")!; | |
| const updatesDiv = document.getElementById("updates"); | |
| if (!updatesDiv) { | |
| console.error("Updates container not found"); | |
| return; | |
| } |
| result[entityId] = parsedEntity; | ||
|
|
||
| if (options?.logging) { | ||
| console.log(`Parsed entity:`, parsedEntity); | ||
| } | ||
| } | ||
| return parsedEntity; |
There was a problem hiding this comment.
Remove unnecessary return statement in forEach.
The return statement inside forEach doesn't affect the result. It should be removed as it might cause confusion.
result[entityId] = parsedEntity;
if (options?.logging) {
console.log(`Parsed entity:`, parsedEntity);
}
- return parsedEntity;📝 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.
| result[entityId] = parsedEntity; | |
| if (options?.logging) { | |
| console.log(`Parsed entity:`, parsedEntity); | |
| } | |
| } | |
| return parsedEntity; | |
| result[entityId] = parsedEntity; | |
| if (options?.logging) { | |
| console.log(`Parsed entity:`, parsedEntity); | |
| } |
| // @ts-ignore | ||
| const result: StandardizedQueryResult<T> = entities; |
There was a problem hiding this comment.
Remove @ts-ignore and add proper type assertion.
Using @ts-ignore masks potential type issues. Consider adding proper type assertions or fixing the underlying type mismatch.
- // @ts-ignore
- const result: StandardizedQueryResult<T> = entities;
+ const result: StandardizedQueryResult<T> = {} as StandardizedQueryResult<T>;Committable suggestion skipped: line range outside the PR's diff.
| expect( | ||
| res[ | ||
| "0x43ebbfee0476dcc36cae36dfa9b47935cc20c36cb4dc7d014076e5f875cf164" | ||
| ].models.onchain_dash.CallerCounter.timestamp | ||
| ).toEqual(expected); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove @ts-ignore comments and add proper type assertions.
Instead of using @ts-ignore, consider adding proper type assertions or fixing the underlying type issues.
- // @ts-ignore can be undefined
- expect(
- res[
- "0x43ebbfee0476dcc36cae36dfa9b47935cc20c36cb4dc7d014076e5f875cf164"
- ].models.onchain_dash.CallerCounter.timestamp
- ).toEqual(expected);
+ expect(
+ (res[
+ "0x43ebbfee0476dcc36cae36dfa9b47935cc20c36cb4dc7d014076e5f875cf164"
+ ] as ParsedEntity<SchemaType>).models.onchain_dash.CallerCounter.timestamp
+ ).toEqual(expected);Also applies to: 285-289, 347-351
| "./experimental": { | ||
| "types": "./dist/src/experimental.d.ts", | ||
| "import": "./dist/src/experimental.js", | ||
| "require": "./dist/src/experimental.cjs" |
There was a problem hiding this comment.
💡 Codebase verification
Add documentation for experimental features.
The new experimental module export should be documented in the README or documentation to inform users about its experimental nature and potential API changes. While there is an example project (example-vite-experimental-sdk), proper documentation is still needed to:
- Explain the experimental nature of the module
- Warn about potential API changes
- Provide usage guidelines and limitations
🔗 Analysis chain
Add documentation for experimental features.
The new experimental module export should be documented in the README or documentation to inform users about its experimental nature and potential API changes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if experimental features are documented
rg -l 'experimental' README.md docs/
Length of output: 132
Script:
#!/bin/bash
# Find all markdown files and search for experimental features
fd -e md | xargs rg -l "experimental"
# Also check package.json files as they might contain documentation
fd package.json | xargs rg -l "experimental"
Length of output: 160
MartianGreed
force-pushed
the
feat/torii-query-builder
branch
from
3cbe3d7 to
2af1d00
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (3)
packages/sdk/src/parseEntities.ts (2)
10-11:⚠️ Potential issueRemove @ts-ignore and add proper type assertion.
The @ts-ignore comment masks potential type issues. Consider adding proper type assertions.
Apply this diff to fix the type assertion:
- // @ts-ignore - const result: StandardizedQueryResult<T> = entities; + const result: StandardizedQueryResult<T> = {} as StandardizedQueryResult<T>;
43-48:⚠️ Potential issueRemove unnecessary return statement in forEach.
The return statement inside forEach doesn't affect the result and may cause confusion.
Apply this diff to remove the unnecessary return:
result[entityId] = parsedEntity; if (options?.logging) { console.log(`Parsed entity:`, parsedEntity); } - return parsedEntity;packages/sdk/src/__tests__/parseEntities.test.ts (1)
228-232:⚠️ Potential issueRemove @ts-ignore comments and add proper type assertions.
Using @ts-ignore masks potential type issues. Consider adding proper type assertions.
Apply this diff to fix the type assertions:
- // @ts-ignore can be undefined expect( - res[ + (res[ "0x43ebbfee0476dcc36cae36dfa9b47935cc20c36cb4dc7d014076e5f875cf164" - ].models.onchain_dash.CallerCounter.timestamp + ] as ParsedEntity<SchemaType>).models.onchain_dash.CallerCounter.timestamp ).toEqual(expected);Also applies to: 285-289, 347-351
🧹 Nitpick comments (8)
examples/example-vite-experimental-sdk/src/update-manager.ts (3)
8-18: Consider moving styles to a separate CSS file and fixing ID conflict.The inline styles could be moved to a separate CSS file for better maintainability. Also, there's a potential issue where the constructor creates a div with ID "updates" but
displayUpdatemethod tries to find the same ID, which could lead to duplicate IDs if multiple instances are created.Consider these improvements:
- Move styles to a CSS file:
.update-container { height: 80vh; overflow-y: auto; margin-top: 5vh; padding: 10px; }
- Use the instance container instead of finding by ID:
- this.container.id = "updates"; + this.container.className = "update-container";
66-78: Improve error handling and simplify setTimeout logic.The copy button click handler could be improved with better error handling and cleaner setTimeout logic.
copyButton.onclick = async () => { + const resetButton = () => { + setTimeout(() => { + copyButton.textContent = "copy"; + }, 1000); + }; + try { await navigator.clipboard.writeText( JSON.stringify(update, null, 2) ); copyButton.textContent = "✓"; - setTimeout(() => (copyButton.textContent = "copy"), 1000); + resetButton(); } catch (err) { - console.error("Failed to copy:", err); + console.error("Failed to copy to clipboard:", err); copyButton.textContent = "❌"; - setTimeout(() => (copyButton.textContent = "copy"), 1000); + resetButton(); } };🧰 Tools
🪛 Biome (1.9.4)
[error] 72-72: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 76-76: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
33-41: Move inline styles to CSS file.Multiple elements use inline styles which makes maintenance harder and duplicates code.
Consider moving these styles to a CSS file:
.update-title { padding: 8px 12px; border-top: 1px solid #ddd; color: #666; font-family: monospace; font-size: 0.9em; } .update-content { margin: 8px; padding: 12px; background-color: #f5f5f5; border-radius: 4px; font-family: monospace; font-size: 10px; } .copy-button { position: absolute; top: 5px; right: 5px; padding: 2px 4px; border: none; border-radius: 4px; background: #e0e0e0; cursor: pointer; }Then update the elements to use these classes instead of inline styles.
Also applies to: 43-50, 53-64
packages/sdk/src/experimental/convertClauseToEntityKeysClause.ts (1)
28-30: Improve error message clarity.The error message could be more descriptive to help users understand what action they need to take.
- "You cannot use CompositeClause | MemberClause to subscribe to entity updates, include initial data with hashed keys" + "CompositeClause and MemberClause require initial data with hashed keys for entity update subscriptions. Please provide initial data or use a KeysClause instead."examples/example-vite-experimental-sdk/src/typescript/contracts.gen.ts (1)
26-28: Improve error handling strategy.Console logging errors before rethrowing them is redundant as the error will be logged at the catch site. Consider either logging or rethrowing, but not both.
- console.error(error); - throw error; + throw error; // Let the caller handle error loggingAlso applies to: 47-49
examples/example-vite-experimental-sdk/src/typescript/models.gen.ts (2)
94-99: Consider using an enum for Direction values.The Direction values are repeated multiple times in the schema. Consider defining an enum to maintain consistency and reduce duplication.
+export enum DirectionValue { + Left = "Left", + Right = "Right", + Up = "Up", + Down = "Down" +} new CairoCustomEnum({ - Left: "", - Right: undefined, - Up: undefined, - Down: undefined, + [DirectionValue.Left]: "", + [DirectionValue.Right]: undefined, + [DirectionValue.Up]: undefined, + [DirectionValue.Down]: undefined, }),Also applies to: 105-110, 143-148, 153-157
67-73: Add JSDoc comments for better documentation.Consider adding JSDoc comments to describe the purpose and usage of the Direction type.
+/** + * Represents the available movement directions in the game. + * Used for player movement and direction-based calculations. + */ export type Direction = { Left: string; Right: string; Up: string; Down: string; };packages/sdk/src/__tests__/parseEntities.test.ts (1)
143-188: Add test cases for error scenarios.The test suite only covers successful scenarios. Consider adding test cases for error conditions such as:
- Invalid entity data
- Missing required fields
- Malformed model names
Would you like me to help generate additional test cases for error scenarios?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
examples/example-vite-experimental-sdk/public/vite.svgis excluded by!**/*.svgpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (24)
examples/example-vite-experimental-sdk/.gitignore(1 hunks)examples/example-vite-experimental-sdk/dojoConfig.ts(1 hunks)examples/example-vite-experimental-sdk/index.html(1 hunks)examples/example-vite-experimental-sdk/package.json(1 hunks)examples/example-vite-experimental-sdk/src/main.ts(1 hunks)examples/example-vite-experimental-sdk/src/style.css(1 hunks)examples/example-vite-experimental-sdk/src/theme-manager.ts(1 hunks)examples/example-vite-experimental-sdk/src/typescript/contracts.gen.ts(1 hunks)examples/example-vite-experimental-sdk/src/typescript/models.gen.ts(1 hunks)examples/example-vite-experimental-sdk/src/update-manager.ts(1 hunks)examples/example-vite-experimental-sdk/src/vite-env.d.ts(1 hunks)examples/example-vite-experimental-sdk/tsconfig.json(1 hunks)examples/example-vite-experimental-sdk/vite.config.ts(1 hunks)packages/sdk/package.json(1 hunks)packages/sdk/src/__tests__/clauseBuilder.test.ts(1 hunks)packages/sdk/src/__tests__/parseEntities.test.ts(4 hunks)packages/sdk/src/clauseBuilder.ts(1 hunks)packages/sdk/src/experimental/__tests__/convertClauseToEntityKeysClause.test.ts(1 hunks)packages/sdk/src/experimental/convertClauseToEntityKeysClause.ts(1 hunks)packages/sdk/src/experimental/index.ts(1 hunks)packages/sdk/src/parseEntities.ts(2 hunks)packages/sdk/src/toriiQueryBuilder.ts(1 hunks)packages/sdk/tsup.config.ts(1 hunks)scripts/build-examples.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (15)
- examples/example-vite-experimental-sdk/src/vite-env.d.ts
- examples/example-vite-experimental-sdk/dojoConfig.ts
- packages/sdk/tsup.config.ts
- examples/example-vite-experimental-sdk/vite.config.ts
- scripts/build-examples.sh
- examples/example-vite-experimental-sdk/index.html
- examples/example-vite-experimental-sdk/.gitignore
- packages/sdk/package.json
- examples/example-vite-experimental-sdk/src/theme-manager.ts
- examples/example-vite-experimental-sdk/package.json
- examples/example-vite-experimental-sdk/src/style.css
- packages/sdk/src/clauseBuilder.ts
- examples/example-vite-experimental-sdk/tsconfig.json
- examples/example-vite-experimental-sdk/src/main.ts
- packages/sdk/src/tests/clauseBuilder.test.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/sdk/src/experimental/index.ts
[error] 20-20: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
[error] 41-41: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
examples/example-vite-experimental-sdk/src/update-manager.ts
[error] 72-72: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 76-76: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (5)
examples/example-vite-experimental-sdk/src/update-manager.ts (1)
29-29: Add null check for updates div.The code assumes the updates div exists. Add a null check to handle cases where the element might not be found.
packages/sdk/src/experimental/convertClauseToEntityKeysClause.ts (1)
18-19:⚠️ Potential issueReplace unsafe type casting with type guard.
The current type casting approach using
as unknown as EntityKeysClauseis unsafe and could lead to runtime errors if the object structure doesn't matchEntityKeysClause.Consider using a type guard:
- if (Object.hasOwn(clause, "Keys")) { - return [clause as unknown as EntityKeysClause]; + function isEntityKeysClause(clause: Clause): clause is EntityKeysClause { + return Object.hasOwn(clause, "Keys") && + typeof clause.Keys === "object" && + clause.Keys !== null; + } + + if (isEntityKeysClause(clause)) { + return [clause];Likely invalid or redundant comment.
packages/sdk/src/experimental/__tests__/convertClauseToEntityKeysClause.test.ts (1)
5-64: Great test coverage!The test suite is comprehensive and well-structured, covering all major use cases including edge cases and error conditions.
packages/sdk/src/experimental/index.ts (1)
7-64: 🛠️ Refactor suggestionAdd cleanup mechanism for subscriptions.
The subscription methods return arrays containing subscription objects but don't provide a cleanup mechanism, which could lead to memory leaks.
export async function init<T extends SchemaType>(options: SDKConfig) { const client = await torii.createClient(options.client); + const subscriptions = new Set(); + + const cleanup = () => { + for (const subscription of subscriptions) { + subscription.unsubscribe(); + } + subscriptions.clear(); + }; return { // ... other methods ... subscribeEntities: async (query: torii.Query, callback: Function) => { // ... other code ... + const subscription = client.onEntityUpdated( + intoEntityKeysClause<T>(query.clause, entities), + callback + ); + subscriptions.add(subscription); return [ entities, - client.onEntityUpdated( - intoEntityKeysClause<T>(query.clause, entities), - callback - ), + subscription, ]; }, // Similar changes for subscribeEvents + cleanup, }; }Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 20-20: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
[error] 41-41: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
packages/sdk/src/toriiQueryBuilder.ts (1)
16-122: Great implementation of the builder pattern!The class is well-structured with clear documentation and a fluent interface.
| /** | ||
| * Displays an update in the updates div. | ||
| * | ||
| * The `updates` div must be defined in the HTML file. | ||
| * | ||
| * @param {Object} update - The update to display as a JSON object. | ||
| */ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update method documentation to reflect actual implementation.
The documentation incorrectly states that the "updates div must be defined in the HTML file", but the container is actually created in the constructor.
Update the documentation to:
/**
* Displays an update in the updates container.
*
* @param {string} title - The title of the update.
* @param {any} update - The update to display as a JSON object.
*/| updateElement.innerHTML = `<code class="language-json">${JSON.stringify(update, null, 2)}</code>`; | ||
| hljs.highlightElement(updateElement.firstChild! as HTMLElement); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add safety checks for DOM operations.
The code uses unsafe type assertions and doesn't handle potential null cases when working with DOM elements.
- updateElement.innerHTML = `<code class="language-json">${JSON.stringify(update, null, 2)}</code>`;
- hljs.highlightElement(updateElement.firstChild! as HTMLElement);
+ const code = document.createElement('code');
+ code.className = 'language-json';
+ code.textContent = JSON.stringify(update, null, 2);
+ updateElement.appendChild(code);
+ hljs.highlightElement(code);📝 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.
| updateElement.innerHTML = `<code class="language-json">${JSON.stringify(update, null, 2)}</code>`; | |
| hljs.highlightElement(updateElement.firstChild! as HTMLElement); | |
| const code = document.createElement('code'); | |
| code.className = 'language-json'; | |
| code.textContent = JSON.stringify(update, null, 2); | |
| updateElement.appendChild(code); | |
| hljs.highlightElement(code); |
| return await provider.execute( | ||
| snAccount, | ||
| build_actions_move_calldata(direction), | ||
| "dojo_starter" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract hardcoded contract name to a configuration parameter.
The contract name "dojo_starter" is hardcoded, which reduces reusability and makes the code less maintainable.
-export function setupWorld(provider: DojoProvider) {
+export function setupWorld(provider: DojoProvider, contractName: string = "dojo_starter") {
// ... other code ...
- "dojo_starter"
+ contractName
// ... other code ...
- "dojo_starter"
+ contractNameAlso applies to: 44-44
| ? parseHistoricalEvents(events) | ||
| : parseEntities(events); | ||
| }, | ||
| subscribeEntities: async (query: torii.Query, callback: Function) => { |
There was a problem hiding this comment.
Replace Function type with specific callback signatures.
Using the Function type is unsafe as it accepts any function-like value. Define specific callback types for better type safety.
- subscribeEntities: async (query: torii.Query, callback: Function) => {
+ subscribeEntities: async (query: torii.Query, callback: (entity: T) => void) => {
- subscribeEvents: async (query: torii.Query, callback: Function, historical: boolean = false) => {
+ subscribeEvents: async (query: torii.Query, callback: (event: T) => void, historical: boolean = false) => {Also applies to: 41-41
🧰 Tools
🪛 Biome (1.9.4)
[error] 20-20: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
| withLimit(limit: number): ToriiQueryBuilder<T> { | ||
| this.query.limit = limit; | ||
| return this; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for numeric parameters.
The methods accepting numeric parameters should validate their inputs to prevent negative values.
withLimit(limit: number): ToriiQueryBuilder<T> {
+ if (limit <= 0) {
+ throw new Error("Limit must be a positive number");
+ }
this.query.limit = limit;
return this;
}
withOffset(offset: number): ToriiQueryBuilder<T> {
+ if (offset < 0) {
+ throw new Error("Offset must be a non-negative number");
+ }
this.query.offset = offset;
return this;
}
updatedAfter(timestamp: number): ToriiQueryBuilder<T> {
+ if (timestamp < 0) {
+ throw new Error("Timestamp must be a non-negative number");
+ }
this.query.entity_updated_after = timestamp;
return this;
}Also applies to: 34-37, 99-102
Summary by CodeRabbit
New Features
ToriiQueryBuilderandClauseBuilderfor enhanced querying capabilities.ThemeManagerfor managing themes in the application.UpdateManagerfor displaying updates in a structured format.dojoConfigfor application configuration.setupWorldfunction for interacting with the Dojo provider.Improvements
Tests
ToriiQueryBuilder,ClauseBuilder, andintoEntityKeysClausefunction.Chores