feat: sdk reusable query hooks #402
Conversation
WalkthroughThis pull request introduces a new changeset file documenting minor version updates for multiple Dojo Engine packages, accompanied by several updates across example projects and the SDK. The modifications include TypeScript configuration changes, dependency version updates, and a transition from manual subscription management to declarative hook-based patterns (e.g., Changes
Sequence Diagram(s)sequenceDiagram
participant U as User/Account
participant A as App Component
participant H as SDK Hooks
participant E as Events Component
U->>A: Render App
A->>H: Call useEntityId(account.address)
H-->>A: Return entityId
A->>H: Call useEntityQuery(query with entityId)
H-->>A: Return query result
A->>E: Render Events with query data
E->>H: Call useEventQuery/useHistoricalEventsQuery(query)
H-->>E: Return event data
sequenceDiagram
participant C as Client Code
participant SDK as SDK Init Object
C->>SDK: Call toriiQueryIntoHashedKeys(query)
SDK->>SDK: Build query and convert to hashed keys
SDK-->>C: Return [ToriiResponse, EntityKeysClause[]]
Possibly related PRs
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Pro Plan! Simply use the command ✨ 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 (
|
![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
🔭 Outside diff range comments (1)
examples/example-nodejs-bot/tsconfig.json (1)
2-17: 💡 Verification agent🧩 Analysis chain
Consider documenting the removal of "bun-types" in a comment
I notice the
"types": ["bun-types"]has been removed from the compiler options. This change may impact type checking for Bun-specific APIs and indicates a potential shift away from using Bun as a runtime. Consider adding a comment explaining this architectural decision for clarity.
🏁 Script executed:
#!/bin/bash # Check if there are other references to bun in the repository rg -i 'bun' --type ts --type jsonLength of output: 18496
Clarify the Removal of
"bun-types"in tsconfig.jsonIn the file
examples/example-nodejs-bot/tsconfig.json(lines 2–17), the"types": ["bun-types"]entry has been removed. However, note that the repository still references Bun (e.g.,"bun-types": "latest"inexamples/example-nodejs-bot/package.json). To avoid confusion about the intentional change and its impact on Bun-specific API type checking, please add an inline comment explaining the rationale behind this removal—whether it reflects a shift in how Bun dependencies are managed (e.g., relying on the package configuration) or a move away from using Bun’s type system.
- File:
examples/example-nodejs-bot/tsconfig.json- Suggestion: Add a comment clarifying the architectural decision regarding the removal of
"bun-types"and how the type checking for Bun-specific APIs is now being handled.
🧹 Nitpick comments (10)
packages/sdk/src/state/index.ts (1)
18-18: Add documentation for the new mergeEntities methodThe new
mergeEntitiesmethod extends the GameState interface to support merging entities. To maintain consistency with the rest of the codebase and improve developer experience, consider adding JSDoc comments explaining:
- The purpose of this method
- How it differs from
setEntities- How entity merging is handled (e.g., whether it's a deep or shallow merge)
- Any side effects developers should be aware of
+/** + * Merges the provided entities into the current state. + * Unlike setEntities which replaces the entities, this method + * merges the new entities with existing ones. + * + * @param entities - Array of parsed entities to merge + */ mergeEntities: (entities: ParsedEntity<T>[]) => void;packages/sdk/src/state/zustand.ts (1)
45-104: The newmergeEntitiesmethod is a valuable addition.This method provides an efficient way to update multiple entities in a single state update, following the same merging logic as the existing
updateEntitymethod but operating on an array of entities.However, there are a few optimization opportunities:
- Consider using
for...ofinstead offorEachfor better performance with large arrays:- entities.forEach((entity) => { + for (const entity of entities) { if (entity.entityId && entity.models) { // ...existing code... } - }); + }
- Similarly for the inner loop:
- Object.entries(entity.models).forEach( - ([namespace, namespaceModels]) => { + for (const [namespace, namespaceModels] of Object.entries(entity.models)) { // ...existing code... - } - ); + }
- Avoid using
anytype at line 73 for better type safety:- ] = {} as any; + ] = {} as Record<string, unknown>;🧰 Tools
🪛 Biome (1.9.4)
[error] 47-102: Prefer for...of instead of forEach.
forEach may lead to performance issues when working with large arrays. When combined with functions like filter or map, this causes multiple iterations over the same type.
(lint/complexity/noForEach)
[error] 61-86: Prefer for...of instead of forEach.
forEach may lead to performance issues when working with large arrays. When combined with functions like filter or map, this causes multiple iterations over the same type.
(lint/complexity/noForEach)
[error] 73-73: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
examples/example-vite-react-sdk/src/events.tsx (2)
1-40: New Events component is well structured but has minor improvement opportunities.The component effectively demonstrates the use of the new SDK hooks for querying and displaying event data. The implementation is clean and provides proper error handling for when an account isn't connected.
A few suggestions for improvement:
- Use optional chaining for the moved property on line 32:
- Player Last Movement : {moved && moved.direction}{" "} + Player Last Movement : {moved?.direction}{" "}
There's commented-out code for event mapping (lines 35-37) that should either be implemented or removed.
Consider adding loading states when querying data.
🧰 Tools
🪛 Biome (1.9.4)
[error] 32-32: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
10-20: Query includes unused features.The query setup correctly uses the
KeysClausebut includes some unused or potentially redundant features.
The empty array in
KeysClause([], [addAddressPadding(account?.address ?? "0")], "VariableLen")suggests you're not filtering by any specific keys in the first position. Consider whether this is intended or if there should be keys here.The call to
.includeHashedKeys()is included but its purpose isn't clear in this context. If it's necessary, consider adding a comment explaining why.packages/sdk/src/types.ts (1)
436-455: New SDK interface methods enhance query functionality.The addition of
toriiQueryIntoHashedKeysandtoriiEventMessagesQueryIntoHashedKeysmethods provides valuable functionality for converting queries into hashed keys, which is useful for more efficient entity lookups.There's a minor documentation issue though:
The JSDocs for both methods have identical return type descriptions:
@returns [ToriiResponse<T,false>,torii.EntityKeysClause[]]For the second method, this doesn't accurately reflect the actual generic return type:
Promise<[ToriiResponse<T, H>, torii.EntityKeysClause[]]>Please update the second method's documentation to:
- @returns [ToriiResponse<T,false>,torii.EntityKeysClause[]] + @returns [ToriiResponse<T,H>,torii.EntityKeysClause[]]packages/sdk/src/index.ts (1)
17-17: Useimport typefor ToriiQueryBuilder.Since
ToriiQueryBuilderis only used as a type in parameter declarations, useimport typeto avoid bundling unnecessary code.-import { ToriiQueryBuilder } from "./toriiQueryBuilder"; +import type { ToriiQueryBuilder } from "./toriiQueryBuilder";🧰 Tools
🪛 Biome (1.9.4)
[error] 17-17: All these imports are only used as types.
Importing the types with import type ensures that they are removed by the compilers and avoids loading unnecessary modules.
Safe fix: Use import type.(lint/style/useImportType)
packages/sdk/package.json (1)
65-65: Double-check peer dependency approach.
Re-adding@tanstack/react-queryand reorganizingreact-domandstarknetas peer dependencies is fine if downstream consumers are required to install these themselves. Ensure the project properly documents this requirement so users do not face missing dependency issues.Also applies to: 69-70
packages/sdk/src/react/hooks.ts (3)
1-16: Use ‘import type’ for type-only imports.
Static analysis indicates that some imports (e.g.BigNumberish,GameState,DojoContextType) are only used as types. Converting them toimport typestatements may improve build performance and clarity.Here’s a quick example diff:
-import { StandardizedQueryResult, type SchemaType } from "../types"; -import { DojoContext, type DojoContextType } from "./provider"; -import { create, type StoreApi, type UseBoundStore } from "zustand"; -import { Subscription } from "@dojoengine/torii-client"; -import { getEntityIdFromKeys } from "@dojoengine/utils"; +import type { StandardizedQueryResult, SchemaType } from "../types"; +import type { DojoContextType } from "./provider"; +import type { StoreApi, UseBoundStore } from "zustand"; +import type { Subscription } from "@dojoengine/torii-client"; +import type { BigNumberish } from "starknet"; +import type { GameState } from "../state";🧰 Tools
🪛 Biome (1.9.4)
[error] 10-10: All these imports are only used as types.
Importing the types with import type ensures that they are removed by the compilers and avoids loading unnecessary modules.
Safe fix: Use import type.(lint/style/useImportType)
[error] 15-15: All these imports are only used as types.
Importing the types with import type ensures that they are removed by the compilers and avoids loading unnecessary modules.
Safe fix: Use import type.(lint/style/useImportType)
[error] 16-16: All these imports are only used as types.
Importing the types with import type ensures that they are removed by the compilers and avoids loading unnecessary modules.
Safe fix: Use import type.(lint/style/useImportType)
68-70: Avoid usinganyto improve type safety.
Static analysis flags multiple instances ofany(notably lines 68, 70, 298, 398). Usinganydisables key type checks, making it harder to catch type issues at compile time. Consider introducing a generic or a more specific type to avoid these placeholders.Also applies to: 298-298, 398-398
🧰 Tools
🪛 Biome (1.9.4)
[error] 68-68: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 68-68: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
109-109: Add missing dependencies or remove extraneous ones in hooks.
Static analysis indicates your callback and effect dependencies (fetchData,sdkmethods, andstateactions) are not fully listed in theuseEffect/useCallbackdependency arrays. Some dependencies (e.g.subscriptionRef) appear unnecessary. This can lead to stale references or unintentional re-renders.A possible fix for
useEntityQuery(similar pattern applies to other hooks):-useEffect(() => { +useEffect(() => { if (!deepEqual(query, fetchingRef.current)) { ... } -}, [query, subscriptionRef, fetchData]); +}, [query, fetchData]);…and ensure
fetchDataincludes[sdk, state, subscriptionRef, query]or whichever references are used inside.Also applies to: 151-151, 204-204, 254-254, 308-308, 357-357
🧰 Tools
🪛 Biome (1.9.4)
[error] 109-109: This hook does not specify all of its dependencies: sdk.subscribeEntityQuery
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
[error] 109-109: This hook does not specify all of its dependencies: sdk.updateEntitySubscription
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
[error] 109-109: This hook specifies more dependencies than necessary: subscriptionRef
This dependency can be removed from the list.
(lint/correctness/useExhaustiveDependencies)
[error] 109-109: This hook does not specify all of its dependencies: state.updateEntity
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
[error] 109-109: This hook does not specify all of its dependencies: sdk.toriiQueryIntoHashedKeys
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
[error] 109-109: This hook does not specify all of its dependencies: state.mergeEntities
This dependency is not specified in the hook dependency list.
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (19)
.changeset/good-eagles-speak.md(1 hunks)examples/example-nodejs-bot/tsconfig.json(1 hunks)examples/example-vite-react-sdk/dojoConfig.ts(1 hunks)examples/example-vite-react-sdk/package.json(1 hunks)examples/example-vite-react-sdk/src/App.tsx(3 hunks)examples/example-vite-react-sdk/src/events.tsx(1 hunks)examples/example-vite-react-sdk/src/historical-events.tsx(2 hunks)examples/example-vite-react-sdk/src/starknet-provider.tsx(1 hunks)examples/example-vite-react-sdk/tsconfig.app.tsbuildinfo(1 hunks)examples/example-vite-react-sql/src/hooks/usePlayerActions.ts(1 hunks)examples/example-vite-react-sql/src/routes/__root.tsx(1 hunks)examples/example-vite-react-threejs-recs/src/gameComponents/Player.tsx(1 hunks)examples/example-vite-react-threejs-recs/src/gameComponents/Three.tsx(1 hunks)packages/sdk/package.json(1 hunks)packages/sdk/src/index.ts(2 hunks)packages/sdk/src/react/hooks.ts(2 hunks)packages/sdk/src/state/index.ts(1 hunks)packages/sdk/src/state/zustand.ts(1 hunks)packages/sdk/src/types.ts(1 hunks)
✅ Files skipped from review due to trivial changes (5)
- examples/example-vite-react-sdk/dojoConfig.ts
- examples/example-vite-react-threejs-recs/src/gameComponents/Three.tsx
- examples/example-vite-react-threejs-recs/src/gameComponents/Player.tsx
- .changeset/good-eagles-speak.md
- examples/example-vite-react-sdk/src/starknet-provider.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
examples/example-vite-react-sdk/src/events.tsx
[error] 32-32: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/sdk/src/index.ts
[error] 17-17: All these imports are only used as types.
Importing the types with import type ensures that they are removed by the compilers and avoids loading unnecessary modules.
Safe fix: Use import type.
(lint/style/useImportType)
packages/sdk/src/react/hooks.ts
[error] 10-10: All these imports are only used as types.
Importing the types with import type ensures that they are removed by the compilers and avoids loading unnecessary modules.
Safe fix: Use import type.
(lint/style/useImportType)
[error] 15-15: All these imports are only used as types.
Importing the types with import type ensures that they are removed by the compilers and avoids loading unnecessary modules.
Safe fix: Use import type.
(lint/style/useImportType)
[error] 16-16: All these imports are only used as types.
Importing the types with import type ensures that they are removed by the compilers and avoids loading unnecessary modules.
Safe fix: Use import type.
(lint/style/useImportType)
[error] 68-68: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 68-68: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 120-120: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 128-128: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 216-216: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 230-230: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 298-298: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 320-320: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 333-333: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 398-398: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 398-398: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 109-109: This hook does not specify all of its dependencies: sdk.subscribeEntityQuery
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
[error] 109-109: This hook does not specify all of its dependencies: sdk.updateEntitySubscription
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
[error] 109-109: This hook specifies more dependencies than necessary: subscriptionRef
This dependency can be removed from the list.
(lint/correctness/useExhaustiveDependencies)
[error] 109-109: This hook does not specify all of its dependencies: state.updateEntity
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
[error] 109-109: This hook does not specify all of its dependencies: sdk.toriiQueryIntoHashedKeys
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
[error] 109-109: This hook does not specify all of its dependencies: state.mergeEntities
This dependency is not specified in the hook dependency list.
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
[error] 151-151: This hook specifies more dependencies than necessary: subscriptionRef
This dependency can be removed from the list.
(lint/correctness/useExhaustiveDependencies)
[error] 204-204: This hook does not specify all of its dependencies: sdk.subscribeEventQuery
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
[error] 204-204: This hook does not specify all of its dependencies: sdk.updateEventMessageSubscription
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
[error] 204-204: This hook specifies more dependencies than necessary: subscriptionRef
This dependency can be removed from the list.
(lint/correctness/useExhaustiveDependencies)
[error] 204-204: This hook does not specify all of its dependencies: state.updateEntity
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
[error] 204-204: This hook does not specify all of its dependencies: sdk.toriiEventMessagesQueryIntoHashedKeys
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
[error] 204-204: This hook does not specify all of its dependencies: state.mergeEntities
This dependency is not specified in the hook dependency list.
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
[error] 254-254: This hook specifies more dependencies than necessary: subscriptionRef
This dependency can be removed from the list.
(lint/correctness/useExhaustiveDependencies)
[error] 308-308: This hook does not specify all of its dependencies: sdk.subscribeEventQuery
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
[error] 308-308: This hook does not specify all of its dependencies: sdk.updateEventMessageSubscription
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
[error] 308-308: This hook does not specify all of its dependencies: sdk.toriiEventMessagesQueryIntoHashedKeys
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
[error] 308-308: This hook specifies more dependencies than necessary: subscriptionRef
This dependency can be removed from the list.
(lint/correctness/useExhaustiveDependencies)
[error] 357-357: This hook specifies more dependencies than necessary: subscriptionRef
This dependency can be removed from the list.
(lint/correctness/useExhaustiveDependencies)
packages/sdk/src/state/zustand.ts
[error] 47-102: Prefer for...of instead of forEach.
forEach may lead to performance issues when working with large arrays. When combined with functions like filter or map, this causes multiple iterations over the same type.
(lint/complexity/noForEach)
[error] 61-86: Prefer for...of instead of forEach.
forEach may lead to performance issues when working with large arrays. When combined with functions like filter or map, this causes multiple iterations over the same type.
(lint/complexity/noForEach)
[error] 73-73: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: check
🔇 Additional comments (12)
examples/example-vite-react-sdk/tsconfig.app.tsbuildinfo (1)
1-1: Auto-generated file reflects new events.tsx inclusionThis change correctly reflects the addition of the new events.tsx file to the TypeScript build process. As this is an auto-generated file, no action is required.
packages/sdk/src/state/index.ts (1)
14-43:✅ Verification successful
Verify implementation consistency for mergeEntities
The interface has been extended with
mergeEntities, but we should verify that the implementation in the store factory properly handles entity merging. This method should be tested for both new entities and updates to existing entities to ensure correct behavior.
🏁 Script executed:
#!/bin/bash # Search for the implementation of mergeEntities rg -A 10 "mergeEntities" --type tsLength of output: 4680
MergeEntities Implementation Verified
I've reviewed the implementation and usage of the
mergeEntitiesmethod:
- Store Implementation: The method in
packages/sdk/src/state/zustand.tsiterates through each provided entity, checks for an existing entity in the state, and usesObject.assignto merge models when an entity already exists. This indicates that both new entities and updates to existing entities are handled.- Usage in React Hooks: Several call sites in
packages/sdk/src/react/hooks.tsinvokemergeEntities, confirming its integration in the data flows.No inconsistencies were found, and the behavior appears aligned with expectations. Please ensure that comprehensive tests cover all edge cases (such as conflicting model properties) to further validate this behavior.
examples/example-vite-react-sdk/package.json (1)
24-25: Version specification standardization looks good.The change from specific version constraints (
^18.3.1) to the catalog-based resolution (catalog:) for React dependencies aligns with the pattern used for other dependencies in this project.examples/example-vite-react-sql/src/hooks/usePlayerActions.ts (1)
1-20: Great refactoring to use the new SDK React hooks!The refactoring of this component significantly improves code quality by leveraging the new hooks system. By replacing manual subscription management with
useEntityIdanduseEntityQuery, the code is now more concise, declarative, and less prone to bugs like memory leaks from unmanaged subscriptions.examples/example-vite-react-sdk/src/historical-events.tsx (2)
4-17: Great use of the new useHistoricalEventsQuery hook!The refactoring to use the new hook simplifies the code by removing manual subscription and state management logic, making the component more declarative and easier to maintain.
28-28:❓ Verification inconclusive
Remove TypeScript ignore comment or add explanation.
This
@ts-ignorecomment suppresses TypeScript errors, which could lead to runtime issues. Consider addressing the underlying type issue instead or adding a comment explaining why the ignore is necessary.
🏁 Script executed:
#!/bin/bash # Check if there are PRs or issues related to TypeScript errors in events handling gh search issues "ts-ignore events" --repo dojoengine/dojo.jsLength of output: 63
Action Required: Clarify '@ts-ignore' usage in historical-events.tsx
The file currently contains a bare
@ts-ignorecomment, which can hide underlying type issues and potentially lead to runtime problems. While our quick search for related issues in the repository didn’t yield any obvious problems, this still warrants manual verification. Please either remove the@ts-ignoreif the underlying type error can be fixed or add a detailed inline comment explaining why ignoring the error is necessary.
- Verify whether the suppressed type error is valid or can be properly handled.
- If suppression is required, document the specific reason for clarity.
packages/sdk/src/index.ts (1)
401-439: Good addition of utility methods for query conversion.These methods provide a foundation for the new React hooks system, converting queries into entity key clauses that can be used for subscriptions. The JSDoc comments clearly explain their purpose.
examples/example-vite-react-sdk/src/App.tsx (3)
8-15: Good use of new SDK React hooks!The imports have been updated to include the new hooks from the SDK, which will simplify state management and subscriptions throughout the component.
207-207: Good addition of the Events component.Adding the Events component enhances the UI by providing event visualization capabilities, making the application more feature-rich.
30-43:❓ Verification inconclusive
Verify the change from "VariableLen" to "FixedLen".
The KeysClause type has been changed from "VariableLen" to "FixedLen", which changes the matching behavior from partial key matching to exact key matching. Ensure this doesn't affect the expected query results.
🏁 Script executed:
#!/bin/bash # Check if there are other places in the codebase that use KeysClause with FixedLen rg "KeysClause\(.+?FixedLen\)" --type ts # Compare with VariableLen usage rg "KeysClause\(.+?VariableLen\)" --type ts # Check if there's documentation about the difference fd -t f README.md | xargs rg -l "FixedLen|VariableLen"Length of output: 145
Verify Query Behavior Change in KeysClause Usage
The change from
"VariableLen"to"FixedLen"in theKeysClausecall may affect key matching behavior—from partial to exact matching. Our initial searches for occurrences of"FixedLen"and"VariableLen"in the codebase (including documentation) did not yield additional instances, so it’s unclear if any behavior depends on the previous"VariableLen"setting. Please perform a manual review of how this change impacts query results, especially in contexts where partial matching was expected. Verify that all relevant queries continue to work as intended.packages/sdk/package.json (2)
52-62: Verify updated devDependencies for compatibility and security.
These updated package versions look reasonable, and no immediate conflicts stand out. However, please confirm that each new version remains compatible with your tooling (rollup plugin, tsup, etc.) and that there are no new security advisories affecting these updates.Would you like me to generate a script to scan for potential breaking changes or known vulnerability reports for these upgraded packages?
75-75: Validate usage of new @dojoengine/utils package and updated Zustand.
The addition of@dojoengine/utilsand the bump inzustandare good steps if they're actually in use. Confirm that any new APIs in@dojoengine/utilsare documented and tested adequately.Also applies to: 77-77
| @@ -21,6 +21,7 @@ function RootComponent() { | |||
| <div className="min-h-screen bg-background"> | |||
| <Header /> | |||
| <main className="container py-6 mx-auto my-0"> | |||
| {/* @ts-ignore */} | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
❓ Verification inconclusive
Avoid using @ts-ignore without explanation
Using @ts-ignore suppresses TypeScript errors without addressing the underlying issue. This can lead to unexpected runtime errors and makes it harder to maintain the codebase in the long term.
Consider either:
- Adding a comment explaining why the ignore is necessary
- Properly typing the component to resolve the TypeScript error
- If it's a library issue, adding a reference to the relevant issue
-{/* @ts-ignore */}
+{/* @ts-ignore - Working around issue with Outlet type definitions in @tanstack/react-router - see [ISSUE_URL] */}Alternatively, investigate and fix the actual type issue:
🏁 Script executed:
#!/bin/bash
# Check the version of @tanstack/react-router being used
grep -r "@tanstack/react-router" --include="package.json" .
# Check if this pattern appears in other files
rg "@ts-ignore.*Outlet" --type tsxLength of output: 214
Below is a revised review comment. Note that the previous command encountered an error with the file type filter; please re-run the search manually (or use the snippet below) to confirm that no additional instances require fixing. For example:
#!/bin/bash
# Verify the @ts-ignore usage in the specific file
rg '@ts-ignore' examples/example-vite-react-sql/src/routes/__root.tsxAction Required: Annotate the Suppressed TypeScript Error in __root.tsx
-
File & Location:
examples/example-vite-react-sql/src/routes/__root.tsxat line 24 -
Current Code:
{/* @ts-ignore */}
-
Recommended Revision:
-{/* @ts-ignore */} +{/* @ts-ignore - Working around issue with Outlet type definitions in @tanstack/react-router v1.97.0 - see [ISSUE_URL] */}
-
Next Steps:
Either document why the suppression is needed (for example, a known issue withOutlettypes in the current version of@tanstack/react-router) or resolve the underlying typing conflict.
Please manually verify there are no other occurrences of unexplained @ts-ignore comments that might hide type errors.
| // @ts-expect-error will fix | ||
| intoEntityKeysClause<T>(q.clause, events), |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Fix the TypeScript error instead of suppressing it.
The comment // @ts-expect-error will fix acknowledges a type issue that should be addressed. Consider creating a ticket to track and fix this technical debt.
🏁 Script executed:
#!/bin/bash
# Check if there are open issues related to TypeScript errors in the SDK
gh search issues "ts-expect-error will fix" --repo dojoengine/dojo.jsLength of output: 71
Action Required: Address the TypeScript error in packages/sdk/src/index.ts
At lines 436–437, the code temporarily suppresses a type-check error with the comment // @ts-expect-error will fix. Instead of leaving this suppression in place, please resolve the underlying type mismatch. Additionally, it appears there isn’t an open issue tracking this technical debt—please create a ticket to ensure this fix is properly tracked and eventually resolved.
- Location:
packages/sdk/src/index.ts(lines 436–437) - Action Steps:
- Fix the underlying type error rather than suppressing it.
- Open a ticket in the issue tracker to monitor and resolve this technical debt.
|
|
||
| if (subscriptionRef.current) { | ||
| const [entities, clause] = await sdk.toriiQueryIntoHashedKeys( | ||
| fetchingRef.current! |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove non-null assertions to avoid runtime errors.
The code often uses fetchingRef.current! or subscriptionRef.current!. Non-null assertions can mask potential null or undefined at runtime. Consider a safer optional chaining or explicit checks before usage.
Here’s a quick example fix:
-const [entities, clause] = await sdk.toriiQueryIntoHashedKeys(fetchingRef.current!);
+if (!fetchingRef.current) return;
+const [entities, clause] = await sdk.toriiQueryIntoHashedKeys(fetchingRef.current);Also applies to: 128-128, 216-216, 230-230, 320-320, 333-333
🧰 Tools
🪛 Biome (1.9.4)
[error] 120-120: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
| // subscriptionRef.current?.cancel(); | ||
| // subscriptionRef.current = null; | ||
| // setSub(null); | ||
| } | ||
| }; | ||
| }, [query, subscriptionRef, fetchData]); | ||
| } |
There was a problem hiding this comment.
Restore subscription cancellation logic to prevent stale listeners.
Your code comments out the lines that cancel (and nullify) subscriptionRef. This can lead to memory leaks or stale listeners if the component unmounts or queries change. Consider restoring or re-implementing subscription cleanup in each effect’s return.
Possible fix:
return () => {
if (subscriptionRef.current) {
- // subscriptionRef.current?.cancel();
- // subscriptionRef.current = null;
+ subscriptionRef.current.cancel();
+ subscriptionRef.current = null;
}
};Also applies to: 280-284, 381-385
MartianGreed
force-pushed
the
feat/sdk-react-hook
branch
from
2f78852 to
a9ccef3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
packages/sdk/src/index.ts (1)
421-439:⚠️ Potential issueNew helper method contains TypeScript error that needs fixing.
Good addition, but there's a TypeScript error that needs to be addressed rather than suppressed with
@ts-expect-error.The error occurs because
intoEntityKeysClausemight not handle historical event data properly. Consider fixing the type issue or creating a ticket to track this technical debt.packages/sdk/src/react/hooks.ts (2)
96-124: 🛠️ Refactor suggestionRemove non-null assertions to avoid potential runtime errors.
Using non-null assertions (
!) bypasses TypeScript's null checks and could lead to runtime errors.Consider using optional chaining or explicit null checks instead:
-const [entities, clause] = await sdk.toriiQueryIntoHashedKeys(fetchingRef.current!); +if (!fetchingRef.current) return null; +const [entities, clause] = await sdk.toriiQueryIntoHashedKeys(fetchingRef.current);🧰 Tools
🪛 Biome (1.9.4)
[error] 120-120: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 109-109: This hook does not specify all of its dependencies: sdk.subscribeEntityQuery
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
[error] 109-109: This hook does not specify all of its dependencies: sdk.updateEntitySubscription
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
[error] 109-109: This hook specifies more dependencies than necessary: subscriptionRef
This dependency can be removed from the list.
(lint/correctness/useExhaustiveDependencies)
[error] 109-109: This hook does not specify all of its dependencies: state.updateEntity
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
[error] 109-109: This hook does not specify all of its dependencies: sdk.toriiQueryIntoHashedKeys
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
[error] 109-109: This hook does not specify all of its dependencies: state.mergeEntities
This dependency is not specified in the hook dependency list.
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
176-184:⚠️ Potential issueRestore subscription cleanup to prevent memory leaks.
The commented-out code prevents proper cleanup of subscriptions, which can lead to memory leaks and stale listeners.
Uncomment and restore the subscription cleanup logic:
return () => { if (subscriptionRef.current) { - // subscriptionRef.current?.cancel(); - // subscriptionRef.current = null; - // setSub(null); + subscriptionRef.current.cancel(); + subscriptionRef.current = null; } };This issue also applies to similar cleanup functions in
useEventQueryanduseHistoricalEventsQuery.
🧹 Nitpick comments (8)
packages/sdk/src/state/zustand.ts (1)
45-104: Improve performance and reduce code duplication.The new
mergeEntitiesmethod is well-structured but has two issues:
- It contains significant code duplication with the existing
updateEntitymethod- It uses
forEachloops which could lead to performance issues with large arraysConsider these improvements:
mergeEntities: (entities: ParsedEntity<T>[]) => { set((state: Draft<GameState<T>>) => { - entities.forEach((entity) => { + for (const entity of entities) { if (entity.entityId && entity.models) { const existingEntity = state.entities[entity.entityId]; if (existingEntity) { // Create new models object without spread const mergedModels: typeof existingEntity.models = Object.assign( {}, existingEntity.models ); // Iterate through each namespace in the new models - Object.entries(entity.models).forEach( - ([namespace, namespaceModels]) => { + for (const [namespace, namespaceModels] of Object.entries(entity.models)) { const typedNamespace = namespace as keyof ParsedEntity<T>["models"]; if ( !( typedNamespace in mergedModels ) ) { mergedModels[ typedNamespace as keyof typeof mergedModels - ] = {} as any; + ] = {} as Record<string, unknown>; } mergedModels[ typedNamespace as keyof typeof mergedModels ] = Object.assign( {}, mergedModels[ typedNamespace as keyof typeof mergedModels ], namespaceModels ); - } - ); + } // Update the entity state.entities[entity.entityId] = { ...existingEntity, ...entity, models: mergedModels, }; } else { // Set new entity state.entities[entity.entityId] = entity as WritableDraft< ParsedEntity<T> >; } } - }); + } }); },Additionally, consider extracting the common entity merging logic into a helper function to reduce duplication between
mergeEntitiesandupdateEntity.🧰 Tools
🪛 Biome (1.9.4)
[error] 47-102: Prefer for...of instead of forEach.
forEach may lead to performance issues when working with large arrays. When combined with functions like filter or map, this causes multiple iterations over the same type.
(lint/complexity/noForEach)
[error] 61-86: Prefer for...of instead of forEach.
forEach may lead to performance issues when working with large arrays. When combined with functions like filter or map, this causes multiple iterations over the same type.
(lint/complexity/noForEach)
[error] 73-73: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
examples/example-vite-react-sdk/src/events.tsx (1)
1-40: Good implementation with minor improvements needed.The Events component works well for displaying player movement data. A few suggestions for improvement:
- Use optional chaining for the moved property:
- Player Last Movement : {moved && moved.direction}{" "} + Player Last Movement : {moved?.direction}{" "}
- The commented-out code should either be removed or implemented:
{/* {events.map((event: ParsedEntity<SchemaType>, key) => { return <Event event={event} key={key} />; })} */}
- Consider adding error handling for the query or if the model fails to load.
🧰 Tools
🪛 Biome (1.9.4)
[error] 32-32: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/sdk/src/index.ts (3)
17-17: Consider using a type-only import.Since
ToriiQueryBuilderis only used as a type, it would be better to use a type-only import to ensure it's removed by compilers and avoid loading unnecessary modules.-import { ToriiQueryBuilder } from "./toriiQueryBuilder"; +import type { ToriiQueryBuilder } from "./toriiQueryBuilder";🧰 Tools
🪛 Biome (1.9.4)
[error] 17-17: All these imports are only used as types.
Importing the types with import type ensures that they are removed by the compilers and avoids loading unnecessary modules.
Safe fix: Use import type.(lint/style/useImportType)
416-420: Documentation return type is incorrect.The function documentation for
toriiEventMessagesQueryIntoHashedKeysmentions an incorrect return type.-* @returns [ToriiResponse<T,false>,torii.EntityKeysClause[]] +* @returns [ToriiResponse<T,H>,torii.EntityKeysClause[]]
427-430: Simplify the historical fallback condition.The ternary expression can be simplified.
-historical ? historical : false +historical || falseexamples/example-vite-react-sdk/src/historical-events.tsx (1)
28-28: Resolve TypeScript issue instead of ignoring it.The
@ts-ignorecomment suggests there's a type issue that should be addressed rather than suppressed.Consider properly typing the events data or adding a type guard to avoid using the type suppression comment.
packages/sdk/src/react/hooks.ts (2)
1-17: Consider using type-only imports for types.Several imports are only used as types and should use the
import typesyntax.import { useCallback, useContext, useEffect, useMemo, useRef, useState, } from "react"; -import { BigNumberish } from "starknet"; +import type { BigNumberish } from "starknet"; -import { StandardizedQueryResult, type SchemaType } from "../types"; +import type { StandardizedQueryResult, SchemaType } from "../types"; import { DojoContext, type DojoContextType } from "./provider"; import { create, type StoreApi, type UseBoundStore } from "zustand"; import { createDojoStoreFactory } from "../state/zustand"; -import type { GameState } from "../state"; +import type { GameState } from "../state"; -import { ToriiQueryBuilder } from "../toriiQueryBuilder"; +import type { ToriiQueryBuilder } from "../toriiQueryBuilder"; -import { Subscription } from "@dojoengine/torii-client"; +import type { Subscription } from "@dojoengine/torii-client"; import { getEntityIdFromKeys } from "@dojoengine/utils";🧰 Tools
🪛 Biome (1.9.4)
[error] 10-10: All these imports are only used as types.
Importing the types with import type ensures that they are removed by the compilers and avoids loading unnecessary modules.
Safe fix: Use import type.(lint/style/useImportType)
[error] 15-15: All these imports are only used as types.
Importing the types with import type ensures that they are removed by the compilers and avoids loading unnecessary modules.
Safe fix: Use import type.(lint/style/useImportType)
[error] 16-16: All these imports are only used as types.
Importing the types with import type ensures that they are removed by the compilers and avoids loading unnecessary modules.
Safe fix: Use import type.(lint/style/useImportType)
398-446: Consider using a library for deep equality checks.The custom
deepEqualfunction is quite verbose and might not handle all edge cases correctly.Consider using a well-tested library like lodash's
isEqualinstead:import { isEqual } from 'lodash'; // Then replace all deepEqual calls with isEqual if (!isEqual(query, fetchingRef.current)) { // ... }🧰 Tools
🪛 Biome (1.9.4)
[error] 398-398: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 398-398: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (18)
.changeset/good-eagles-speak.md(1 hunks)examples/example-nodejs-bot/tsconfig.json(1 hunks)examples/example-vite-react-sdk/package.json(1 hunks)examples/example-vite-react-sdk/src/App.tsx(3 hunks)examples/example-vite-react-sdk/src/events.tsx(1 hunks)examples/example-vite-react-sdk/src/historical-events.tsx(2 hunks)examples/example-vite-react-sdk/src/starknet-provider.tsx(1 hunks)examples/example-vite-react-sdk/tsconfig.app.tsbuildinfo(1 hunks)examples/example-vite-react-sql/src/hooks/usePlayerActions.ts(1 hunks)examples/example-vite-react-sql/src/routes/__root.tsx(1 hunks)examples/example-vite-react-threejs-recs/src/gameComponents/Player.tsx(1 hunks)examples/example-vite-react-threejs-recs/src/gameComponents/Three.tsx(1 hunks)packages/sdk/package.json(1 hunks)packages/sdk/src/index.ts(2 hunks)packages/sdk/src/react/hooks.ts(2 hunks)packages/sdk/src/state/index.ts(1 hunks)packages/sdk/src/state/zustand.ts(1 hunks)packages/sdk/src/types.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- examples/example-vite-react-sql/src/routes/__root.tsx
- examples/example-nodejs-bot/tsconfig.json
- packages/sdk/src/state/index.ts
- examples/example-vite-react-threejs-recs/src/gameComponents/Three.tsx
- .changeset/good-eagles-speak.md
- examples/example-vite-react-sdk/src/starknet-provider.tsx
- examples/example-vite-react-sdk/package.json
- examples/example-vite-react-sql/src/hooks/usePlayerActions.ts
- examples/example-vite-react-threejs-recs/src/gameComponents/Player.tsx
- examples/example-vite-react-sdk/tsconfig.app.tsbuildinfo
- packages/sdk/package.json
🧰 Additional context used
🪛 Biome (1.9.4)
examples/example-vite-react-sdk/src/events.tsx
[error] 32-32: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/sdk/src/index.ts
[error] 17-17: All these imports are only used as types.
Importing the types with import type ensures that they are removed by the compilers and avoids loading unnecessary modules.
Safe fix: Use import type.
(lint/style/useImportType)
packages/sdk/src/react/hooks.ts
[error] 10-10: All these imports are only used as types.
Importing the types with import type ensures that they are removed by the compilers and avoids loading unnecessary modules.
Safe fix: Use import type.
(lint/style/useImportType)
[error] 15-15: All these imports are only used as types.
Importing the types with import type ensures that they are removed by the compilers and avoids loading unnecessary modules.
Safe fix: Use import type.
(lint/style/useImportType)
[error] 16-16: All these imports are only used as types.
Importing the types with import type ensures that they are removed by the compilers and avoids loading unnecessary modules.
Safe fix: Use import type.
(lint/style/useImportType)
[error] 68-68: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 68-68: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 120-120: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 128-128: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 216-216: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 230-230: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 298-298: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 320-320: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 333-333: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 398-398: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 398-398: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 109-109: This hook does not specify all of its dependencies: sdk.subscribeEntityQuery
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
[error] 109-109: This hook does not specify all of its dependencies: sdk.updateEntitySubscription
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
[error] 109-109: This hook specifies more dependencies than necessary: subscriptionRef
This dependency can be removed from the list.
(lint/correctness/useExhaustiveDependencies)
[error] 109-109: This hook does not specify all of its dependencies: state.updateEntity
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
[error] 109-109: This hook does not specify all of its dependencies: sdk.toriiQueryIntoHashedKeys
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
[error] 109-109: This hook does not specify all of its dependencies: state.mergeEntities
This dependency is not specified in the hook dependency list.
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
[error] 151-151: This hook specifies more dependencies than necessary: subscriptionRef
This dependency can be removed from the list.
(lint/correctness/useExhaustiveDependencies)
[error] 204-204: This hook does not specify all of its dependencies: sdk.subscribeEventQuery
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
[error] 204-204: This hook does not specify all of its dependencies: sdk.updateEventMessageSubscription
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
[error] 204-204: This hook specifies more dependencies than necessary: subscriptionRef
This dependency can be removed from the list.
(lint/correctness/useExhaustiveDependencies)
[error] 204-204: This hook does not specify all of its dependencies: state.updateEntity
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
[error] 204-204: This hook does not specify all of its dependencies: sdk.toriiEventMessagesQueryIntoHashedKeys
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
[error] 204-204: This hook does not specify all of its dependencies: state.mergeEntities
This dependency is not specified in the hook dependency list.
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
[error] 254-254: This hook specifies more dependencies than necessary: subscriptionRef
This dependency can be removed from the list.
(lint/correctness/useExhaustiveDependencies)
[error] 308-308: This hook does not specify all of its dependencies: sdk.subscribeEventQuery
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
[error] 308-308: This hook does not specify all of its dependencies: sdk.updateEventMessageSubscription
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
[error] 308-308: This hook does not specify all of its dependencies: sdk.toriiEventMessagesQueryIntoHashedKeys
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
[error] 308-308: This hook specifies more dependencies than necessary: subscriptionRef
This dependency can be removed from the list.
(lint/correctness/useExhaustiveDependencies)
[error] 357-357: This hook specifies more dependencies than necessary: subscriptionRef
This dependency can be removed from the list.
(lint/correctness/useExhaustiveDependencies)
packages/sdk/src/state/zustand.ts
[error] 47-102: Prefer for...of instead of forEach.
forEach may lead to performance issues when working with large arrays. When combined with functions like filter or map, this causes multiple iterations over the same type.
(lint/complexity/noForEach)
[error] 61-86: Prefer for...of instead of forEach.
forEach may lead to performance issues when working with large arrays. When combined with functions like filter or map, this causes multiple iterations over the same type.
(lint/complexity/noForEach)
[error] 73-73: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: check
- GitHub Check: build
🔇 Additional comments (9)
packages/sdk/src/types.ts (1)
436-454: Method signatures and documentation look good.The new methods for converting Torii queries to hashed keys are well-typed with appropriate use of generics. The documentation clearly explains the purpose of each method.
examples/example-vite-react-sdk/src/App.tsx (3)
8-15: Good migration to hook-based approach.Moving to declarative hooks for SDK functionality is a good improvement. The imports are well organized.
30-43: Nice simplification of entity querying logic.The new hook-based approach for entity ID and entity queries simplifies the code compared to manual subscription management. The query is well structured with appropriate clauses.
Note that there's a change from "VariableLen" to "FixedLen" in the KeysClause. Make sure this change is intentional and matches the expected query pattern.
207-207: Good integration of the Events component.The Events component is properly integrated into the UI.
packages/sdk/src/index.ts (1)
407-413: New helper method looks good.This is a useful utility that extracts common logic from the entity subscription code, making it reusable for the new React hooks.
examples/example-vite-react-sdk/src/historical-events.tsx (2)
4-5: Good job adding proper imports for the new hook pattern.The code now imports the necessary dependencies for the new hook-based approach.
9-17: Clean refactoring to hook-based pattern.Excellent job replacing the manual subscription logic with the new declarative hook. This makes the code more maintainable and concise.
packages/sdk/src/react/hooks.ts (2)
80-89: Well-implemented entity ID hook.The
useEntityIdhook is nicely implemented with proper caching usinguseMemofor performance optimization.
453-455: Good helper function for async operations.The
sleeputility function is a clean implementation for handling asynchronous waiting operations.
| return subscription; | ||
| }, [query, subscriptionRef]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix the dependency array in useCallback.
The fetchData useCallback is missing several dependencies that it uses internally.
Add the missing dependencies to avoid potential bugs or stale closures:
-}, [query, subscriptionRef]);
+}, [query, sdk.toriiQueryIntoHashedKeys, sdk.updateEntitySubscription, sdk.subscribeEntityQuery, state.mergeEntities, state.updateEntity]);📝 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.
| return subscription; | |
| }, [query, subscriptionRef]); | |
| return subscription; | |
| }, [query, sdk.toriiQueryIntoHashedKeys, sdk.updateEntitySubscription, sdk.subscribeEntityQuery, state.mergeEntities, state.updateEntity]); |
Closes #
Introduced changes
Checklist
Summary by CodeRabbit
New Features
Refactor
Chores
Documentation