feat: add order_by and entity_models to sdk #358
Conversation
WalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
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: 0
🧹 Outside diff range and nitpick comments (5)
packages/sdk/src/getEventMessages.ts (1)
37-38: Update JSDoc documentation to include new parametersThe function's JSDoc comments should be updated to document the new
orderByandentityModelsparameters.Add the following to the JSDoc:
* @param {T} schema - The schema type for the entities. * @param {(response: { data?: StandardizedQueryResult<T>; error?: Error }) => void} callback - The callback function to handle the response. +* @param {torii.OrderBy[]} [orderBy=[]] - Optional array of ordering specifications. +* @param {string[]} [entityModels=[]] - Optional array of entity model names to filter by. * @param {number} [limit=100] - The maximum number of event messages to fetch per request. Default is 100.packages/sdk/src/getEntities.ts (1)
37-38: Update JSDoc documentation to include new parametersThe function's JSDoc comments should be updated to document the new
orderByandentityModelsparameters.Add the following to the JSDoc:
* @param {QueryType<T>} query - The query object used to filter entities. * @param {(response: { data?: StandardizedQueryResult<T>; error?: Error }) => void} callback - The callback function to handle the response. +* @param {torii.OrderBy[]} [orderBy=[]] - Optional array of ordering specifications. +* @param {string[]} [entityModels=[]] - Optional array of entity model names to filter by. * @param {number} [limit=100] - The maximum number of entities to fetch per request. Default is 100.packages/sdk/src/index.ts (2)
Line range hint
64-83: Update JSDoc for getEntities methodThe method's documentation should be updated to reflect the new parameters.
Add parameter documentation in the JSDoc:
* @param {GetParams<T>} params - Parameters object +* @param {GetParams<T>} params.orderBy - Optional array of ordering specifications +* @param {GetParams<T>} params.entityModels - Optional array of entity model names to filter by * @returns {Promise<StandardizedQueryResult<T>>} - A promise that resolves to the standardized query result.
Line range hint
90-109: Update JSDoc for getEventMessages methodThe method's documentation should be updated to reflect the new parameters.
Add parameter documentation in the JSDoc:
* @param {GetParams<T>} params - Parameters object +* @param {GetParams<T>} params.orderBy - Optional array of ordering specifications +* @param {GetParams<T>} params.entityModels - Optional array of entity model names to filter by * @returns {Promise<StandardizedQueryResult<T>>} - A promise that resolves to the standardized query result.packages/sdk/src/types.ts (1)
369-372: Enhance documentation for new parametersThe new parameters would benefit from more detailed JSDoc documentation, including:
- Examples of valid
orderByvalues- Format requirements for
entityModelsstrings- Impact of these parameters on query results
Apply this diff to improve the documentation:
error?: Error; }) => void; - // The order to sort the entities by. + /** + * Specifies the order in which entities should be sorted. + * @example + * orderBy: [ + * { field: "score", direction: "DESC" }, + * { field: "name", direction: "ASC" } + * ] + */ orderBy?: torii.OrderBy[]; - // The models to whitelist for fetching. Leave this empty to fetch all models. + /** + * Whitelist specific models for fetching. Format: "namespace.modelName" + * Leave empty to fetch all models. + * @example + * entityModels: ["world.Player", "world.Item"] + */ entityModels?: string[];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
package.json(1 hunks)packages/sdk/src/getEntities.ts(2 hunks)packages/sdk/src/getEventMessages.ts(2 hunks)packages/sdk/src/index.ts(2 hunks)packages/sdk/src/types.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- package.json
🔇 Additional comments (4)
packages/sdk/src/getEventMessages.ts (1)
51-54: LGTM! Clean implementation of query parameters
The implementation correctly maps the new parameters to their corresponding query fields while maintaining the existing functionality.
packages/sdk/src/getEntities.ts (1)
51-54: LGTM! Consistent implementation with getEventMessages
The implementation maintains consistency with getEventMessages.ts, which is excellent for API uniformity.
packages/sdk/src/index.ts (1)
Line range hint 64-109: LGTM! Clean and consistent SDK interface update
The SDK initialization correctly implements the new parameters and maintains consistency with the underlying functions.
Let's verify that the GetParams type has been updated to include the new parameters:
✅ Verification successful
GetParams interface correctly includes all required parameters
The GetParams interface definition perfectly aligns with the parameters used in both getEntities and getEventMessages functions, including:
- Required parameters: query, callback
- Optional parameters: orderBy, entityModels, limit, offset, options
All parameters are properly typed and documented with clear comments.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify GetParams type definition includes new parameters
rg -A 10 "interface GetParams" packages/sdk/src/types.ts
Length of output: 501
Script:
#!/bin/bash
# Let's see the complete GetParams interface definition to verify all parameters
rg -A 20 "interface GetParams" packages/sdk/src/types.ts
Length of output: 774
packages/sdk/src/types.ts (1)
369-370: Verify the structure of OrderBy type
Let's verify that the torii.OrderBy type is properly defined and imported.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
examples/example-vite-react-app-recs/src/dojo/setup.ts (2)
32-33: Document the purpose of the new parameters and consider making them configurable.The empty arrays passed to
getSyncEntities(likely fororderByandentityModels) lack documentation explaining their purpose. Additionally, consider making these parameters configurable through theconfigobject instead of hardcoding empty arrays.+ // Configuration for entity synchronization + const syncConfig = { + orderBy: config.syncOrderBy || [], + entityModels: config.syncEntityModels || [], + timeout: config.syncTimeout || 3000, + enableSync: config.enableSync ?? true + }; const getSync = await getSyncEntities( toriiClient, contractComponents as any, undefined, - [], - [], - [], - 3000, - true + syncConfig.orderBy, + syncConfig.entityModels, + [], // TODO: Document the purpose of this parameter + syncConfig.timeout, + syncConfig.enableSync );
50-51: Apply consistent configuration pattern for event synchronization.Similar to
getSyncEntities, the empty arrays passed togetSyncEventsshould be documented and made configurable through the config object for consistency.+ // Configuration for event synchronization + const eventSyncConfig = { + orderBy: config.eventOrderBy || [], + entityModels: config.eventEntityModels || [] + }; const eventSync = getSyncEvents( toriiClient, contractComponents as any, undefined, - [], - [], - [] + eventSyncConfig.orderBy, + eventSyncConfig.entityModels, + [] // TODO: Document the purpose of this parameter );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ 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 (3)
examples/example-vite-react-app-recs/src/dojo/setup.ts(2 hunks)examples/example-vite-react-phaser-recs/tailwind.config.js(2 hunks)examples/example-vite-react-threejs-recs/tailwind.config.js(2 hunks)
🔇 Additional comments (2)
examples/example-vite-react-threejs-recs/tailwind.config.js (1)
2-2: Same issues as in the Phaser example config
This file has the same incorrect plugin import syntax as discussed in the Phaser example's tailwind.config.js.
Please apply the same fixes as suggested in the previous file's review.
Also applies to: 76-76
examples/example-vite-react-app-recs/src/dojo/setup.ts (1)
88-89: Verify the removal of isSessionSignature parameter.
The publish method signature has been simplified by removing the isSessionSignature parameter. This could be a breaking change if other parts of the application rely on this parameter.
✅ Verification successful
Parameter removal is an intentional alignment with underlying API
Looking at the git diff, this is actually a safe change. The isSessionSignature parameter was being passed directly to toriiClient.publishMessage(), and the diff shows that the client method signature has been updated to match its underlying implementation. The parameter was optional (= false) and its removal appears to be part of a cleanup to match the actual API requirements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing usage of the publish method with three parameters
rg -A 2 "publish\([^)]+,[^)]+,[^)]+\)"
# Search for any references to isSessionSignature in the codebase
rg "isSessionSignature"
Length of output: 64
Script:
#!/bin/bash
# Let's try a broader search for publish method usage
rg -A 2 "\.publish\("
# Search for any files that might contain the old implementation
git diff HEAD~1 -- examples/example-vite-react-app-recs/src/dojo/setup.ts
# Let's also check for any test files that might use this method
fd "test|spec" --exec rg -l "publish"
Length of output: 2049
| @@ -1,5 +1,5 @@ | |||
| /** @type {import('tailwindcss').Config} */ | |||
| module.exports = { | |||
| export default { | |||
There was a problem hiding this comment.
Critical: Fix invalid plugin import syntax
The current changes introduce two issues:
- Using
import()in the plugins array is incorrect as it returns a Promise, but Tailwind expects plugin instances - Configuration files should use static imports for predictable initialization
Apply this fix:
+ import animate from "tailwindcss-animate";
/** @type {import('tailwindcss').Config} */
export default {
// ... rest of config ...
- plugins: [import("tailwindcss-animate")],
+ plugins: [animate],
};Alternatively, if you need to maintain CommonJS compatibility:
/** @type {import('tailwindcss').Config} */
- export default {
+ module.exports = {
// ... rest of config ...
- plugins: [import("tailwindcss-animate")],
+ plugins: [require("tailwindcss-animate")],
};Also applies to: 75-75
| [], | ||
| [], | ||
| [], | ||
| 3000, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving type safety and error handling.
- The
contractComponents as anytype assertion appears multiple times. Consider defining proper types to avoid usingany. - Error handling could be more robust for the sync operations, similar to how burner account initialization is handled.
+ try {
const getSync = await getSyncEntities(
toriiClient,
- contractComponents as any,
+ contractComponents,
// ... other parameters
);
+ } catch (e) {
+ console.error("Failed to initialize entity sync:", e);
+ // Consider proper error recovery strategy
+ }
+ try {
const eventSync = getSyncEvents(
toriiClient,
- contractComponents as any,
+ contractComponents,
// ... other parameters
);
+ } catch (e) {
+ console.error("Failed to initialize event sync:", e);
+ // Consider proper error recovery strategy
+ }Also applies to: 49-52
Closes #
Introduced changes
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
Style