feat: support torii pagination #435
Conversation
WalkthroughThis update introduces cursor-based pagination support across multiple Changes
Sequence Diagram(s)sequenceDiagram
participant App/Hook
participant SDK
participant ToriiClient
participant Pagination
App/Hook->>SDK: getEntities({ query })
SDK->>ToriiClient: fetchEntities(clause, ...)
ToriiClient-->>SDK: entityData[]
SDK->>Pagination: Pagination.fromQuery(query).withItems(entityData[])
Pagination-->>SDK: Pagination instance
SDK-->>App/Hook: Pagination instance (with data, cursor, limit)
App/Hook->>Pagination: getItems()
Pagination-->>App/Hook: entityData[]
App/Hook->>Pagination: getNextQuery(query)
Pagination-->>App/Hook: nextQuery
Possibly related PRs
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 11
🔭 Outside diff range comments (5)
examples/example-node-worker/main.ts (2)
123-135:⚠️ Potential issueType mismatch between function signature and call site
initPositionFromEventis declared to accept aStandardizedQueryResult<SchemaType>, yet the caller at line 166 suppliesevents.getItems()(an array of parsed entities).
This causes a compile-time error and also hides that the function really iterates over plain entities.-function initPositionFromEvent( - events: StandardizedQueryResult<SchemaType> -): PositionCount { +function initPositionFromEvent( + events: ParsedEntity<SchemaType>[] +): PositionCount {After this change the call at line 166 is correct and no further edits are required.
Don’t forget to importParsedEntity(already in the import list).
126-134: 🛠️ Refactor suggestionAvoid mutating the shared
defaultPositionCountobject
const pc = defaultPositionCount;copies the reference, so each invocation mutates the module-level constant and every subsequent call starts from an already-incremented object.- const pc = defaultPositionCount; + const pc: PositionCount = { ...defaultPositionCount };This guarantees a fresh accumulator and prevents surprising cross-call side-effects.
packages/react/src/useQuerySync.ts (1)
31-37:⚠️ Potential issueMissing dependency in
useCallbackdependency array
clauseis captured insidesetupSyncbut is not listed in the dependency list, leading to stale closures whenclausechanges.- }, [toriiClient, components]); + }, [toriiClient, components, clause]);This also resolves the Biome
useExhaustiveDependencieswarning.🧰 Tools
🪛 Biome (1.9.4)
[error] 35-35: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.(lint/complexity/noUselessCatch)
[error] 31-31: This hook does not specify all of its dependencies: clause
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
packages/state/src/recs/index.ts (2)
256-279: 🛠️ Refactor suggestionRename
entityKeyClause→clausefor consistencyEverywhere else the API was simplified to a single
Clause; keeping the old name here is misleading and breaks the mental model.-export const getEntitiesQuery = async <S extends Schema>( - client: ToriiClient, - components: Component<S, Metadata, undefined>[], - entityKeyClause: Clause, +export const getEntitiesQuery = async <S extends Schema>( + client: ToriiClient, + components: Component<S, Metadata, undefined>[], + clause: Clause, ... - clause: entityKeyClause, + clause,Don’t forget to update the call-sites and JSDoc that still mention “entityKeyClause”.
🧰 Tools
🪛 Biome (1.9.4)
[error] 277-277: This type annotation is trivially inferred from its initialization.
Safe fix: Remove the type annotation.
(lint/style/noInferrableTypes)
[error] 278-278: This type annotation is trivially inferred from its initialization.
Safe fix: Remove the type annotation.
(lint/style/noInferrableTypes)
[error] 279-279: This type annotation is trivially inferred from its initialization.
Safe fix: Remove the type annotation.
(lint/style/noInferrableTypes)
285-310:⚠️ Potential issueInfinite/duplicate-fetch loop in
getEntitiesQueryOnly the first page is ever fetched (
client.getEntitiesis called before thewhile), but the loop still iterates, re-processing the same data or—worse—running forever whenlimitis hit.-const fetchedEntities = await client.getEntities({ ... }); - -while (continueFetching) { - // use fetchedEntities but never refresh it +while (continueFetching) { + const fetchedEntities = await client.getEntities({ + pagination: { + limit, + cursor, + direction: "Forward", + order_by: orderBy, + }, + clause, + no_hashed_keys: false, + models: entityModels, + historical, + });This pulls the next page on every iteration and resolves the potential infinite loop.
🧹 Nitpick comments (12)
packages/sdk/src/internal/toriiQueryBuilder.ts (1)
8-10: Consider using type-only imports for PaginationThe static analysis tool suggests converting the import on line 10 to a type-only import since it's used only as a type.
-import { Pagination } from "@dojoengine/torii-wasm"; +import type { Pagination } from "@dojoengine/torii-wasm";🧰 Tools
🪛 Biome (1.9.4)
[error] 8-8: Some named imports are only used as types.
This import is only used as a type.
Importing the types with import type ensures that they are removed by the compilers and avoids loading unnecessary modules.
Safe fix: Add inline type keywords.(lint/style/useImportType)
[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)
examples/example-node-worker/main.ts (1)
1-11: Unused import after refactorOnce
StandardizedQueryResultis no longer used in this file (see first comment), drop it from the import list to keep the file clean and prevent eslint/tsc warnings.🧰 Tools
🪛 Biome (1.9.4)
[error] 1-10: Some named imports are only used as types.
This import is only used as a type.
Importing the types with import type ensures that they are removed by the compilers and avoids loading unnecessary modules.
Safe fix: Add inline type keywords.(lint/style/useImportType)
packages/react/src/useQuerySync.ts (1)
14-24: Update JSDoc to reflect the new APIThe documentation still refers to
entityKeyClauseand marksclauseas optional, which is misleading after the signature change.Please revise these lines so consumers do not rely on outdated examples.
packages/sdk/src/internal/pagination.ts (1)
75-84: Potential unintended mutation of the caller’s query builder
const q = query.withLimit(this.limit);reuses the same builder instance ifwithLimitmutatesthis.
Consumers might expectgetNextQueryto be side-effect free.If
ToriiQueryBuilder.withLimitmutates in place, consider cloning first (assuming aclone()API) or document the behaviour explicitly.-const q = query.withLimit(this.limit); +const q = query.clone().withLimit(this.limit); // avoids mutating the originalmigration/v1-4-v1-5.md (1)
40-45: Minor typo – missing period in code commentFor consistency with other snippets, end the inline comment with a period.
-// New static helper for pagination +// New static helper for pagination.packages/sdk/src/node/index.ts (2)
260-270: JSDoc is out-of-date with new parameter typeThe documentation still refers to
torii.EntityKeysClause[], but the code now accepts a singletorii.Clause.- * @param {torii.EntityKeysClause[]} clauses - New array of key clauses for filtering + * @param {torii.Clause} clauses - Clause used for filtering
278-289: Duplicate doc mismatch & typo
- Same mismatch as above (
EntityKeysClause[]→torii.Clause).- Typo:
Funtion→Function.Please update both occurrences to keep generated API docs correct.
packages/state/src/recs/index.ts (2)
52-57: JSDoc no longer matches the new, non-optionalclauseparameter
getSyncEntitiesnow requires aClause, yet the docs (and the@paramlist above) still state that it is optional and reference a removedentityKeyClauseparameter. This will confuse users and tooling that relies on the annotations.- * @param clause - An optional clause to filter entities. - * @param entityKeyClause - An array of entity key clauses to synchronize. + * @param clause - **Required.** Clause used to filter entities.🧰 Tools
🪛 Biome (1.9.4)
[error] 55-55: This type annotation is trivially inferred from its initialization.
Safe fix: Remove the type annotation.
(lint/style/noInferrableTypes)
[error] 56-56: This type annotation is trivially inferred from its initialization.
Safe fix: Remove the type annotation.
(lint/style/noInferrableTypes)
318-331: Documentation still says “array of Clause”
syncEntitiesnow takes a singleClause, but the accompanying comment (and the example) talk about an “array”. Update the wording to avoid misuse.🧰 Tools
🪛 Biome (1.9.4)
[error] 330-330: This type annotation is trivially inferred from its initialization.
Safe fix: Remove the type annotation.
(lint/style/noInferrableTypes)
packages/sdk/src/web/index.ts (1)
282-286: Parameter name/documentation drift
updateEntitySubscription(and the event variant) now accept a singleClause, yet the JSDoc still says “array of key clauses”. Renameclauses→clause(singular) or fix the comment to avoid confusion.packages/sdk/src/internal/types.ts (2)
1-6: Useimport typefor type-only import (Biome warning)The
Paginationsymbol is only used in type positions; importing it as a value can cause unnecessary bundle bloat.-import { Pagination } from "./pagination.ts"; +import type { Pagination } from "./pagination.ts";🧰 Tools
🪛 Biome (1.9.4)
[error] 5-5: 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)
249-257: Stray comment & type clarityThe isolated
// ToriiResponse<T>comment above theSubscriptionCallbackblock looks accidental and may confuse readers. Consider deleting or moving it next to the actual type alias.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
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/**examples/example-vue-app-recs/src/dojo/generated/setup.tsis excluded by!**/generated/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (34)
.changeset/stale-eggs-heal.md(1 hunks)examples/example-node-worker/main.ts(4 hunks)examples/example-vite-grpc-playground/src/App.tsx(3 hunks)examples/example-vite-kitchen-sink/src/components/caller-counter.tsx(3 hunks)examples/example-vite-kitchen-sink/src/components/chat.tsx(2 hunks)examples/example-vite-kitchen-sink/src/components/global-counter.tsx(3 hunks)examples/example-vite-kitchen-sink/src/components/theme-switch.tsx(3 hunks)examples/example-vite-react-app-recs/package.json(1 hunks)examples/example-vite-react-app-recs/src/App.tsx(2 hunks)examples/example-vite-react-app-recs/src/dojo/setup.ts(3 hunks)examples/example-vite-react-phaser-recs/package.json(1 hunks)examples/example-vite-react-pwa-recs/package.json(1 hunks)examples/example-vite-react-threejs-recs/package.json(1 hunks)examples/example-vue-app-recs/package.json(1 hunks)migration/v1-4-v1-5.md(1 hunks)packages/react/src/useQuerySync.ts(2 hunks)packages/sdk/src/__tests__/convertClauseToEntityKeysClause.test.ts(0 hunks)packages/sdk/src/__tests__/models.gen.ts(1 hunks)packages/sdk/src/__tests__/pagination.test.ts(1 hunks)packages/sdk/src/internal/convertClauseToEntityKeysClause.ts(0 hunks)packages/sdk/src/internal/errors.ts(1 hunks)packages/sdk/src/internal/pagination.ts(1 hunks)packages/sdk/src/internal/parseEntities.ts(1 hunks)packages/sdk/src/internal/subscribeQueryModel.ts(1 hunks)packages/sdk/src/internal/toriiQueryBuilder.ts(2 hunks)packages/sdk/src/internal/types.ts(8 hunks)packages/sdk/src/node/index.ts(5 hunks)packages/sdk/src/web/experimental/index.ts(2 hunks)packages/sdk/src/web/index.ts(6 hunks)packages/sdk/src/web/react/hooks/entities.ts(1 hunks)packages/sdk/src/web/react/hooks/events.ts(2 hunks)packages/sdk/src/web/react/hooks/hooks.ts(4 hunks)packages/state/src/recs/index.ts(11 hunks)packages/torii-wasm/dojo.c(1 hunks)
💤 Files with no reviewable changes (2)
- packages/sdk/src/tests/convertClauseToEntityKeysClause.test.ts
- packages/sdk/src/internal/convertClauseToEntityKeysClause.ts
🧰 Additional context used
🧬 Code Graph Analysis (9)
packages/sdk/src/internal/toriiQueryBuilder.ts (2)
packages/sdk/src/internal/errors.ts (1)
UNDEFINED_CLAUSE(5-6)packages/sdk/src/internal/pagination.ts (1)
Pagination(11-103)
examples/example-vite-react-app-recs/src/App.tsx (1)
packages/react/src/useQuerySync.ts (1)
useQuerySync(26-57)
packages/sdk/src/__tests__/models.gen.ts (2)
examples/example-vite-kitchen-sink/src/components/caller-counter.tsx (1)
CallerCounter(19-122)examples/example-vite-kitchen-sink/src/typescript/models.gen.ts (9)
CallerCounter(13-17)CallerCounterValue(20-23)CustomTheme(26-28)GlobalCounter(31-34)GlobalCounterValue(37-39)Message(42-46)MessageValue(49-51)Theme(54-59)ThemeValue(62-66)
examples/example-vite-kitchen-sink/src/components/chat.tsx (1)
examples/example-vite-kitchen-sink/src/typescript/models.gen.ts (1)
Message(42-46)
packages/sdk/src/internal/subscribeQueryModel.ts (3)
packages/sdk/src/__tests__/models.gen.ts (1)
SchemaType(87-99)packages/sdk/src/internal/types.ts (3)
SchemaType(51-71)SubscriptionCallback(281-283)StandardizedQueryResult(194-196)packages/sdk/src/internal/parseEntities.ts (1)
parseEntities(10-59)
packages/react/src/useQuerySync.ts (1)
packages/state/src/recs/index.ts (1)
getSyncEntities(49-69)
packages/sdk/src/web/react/hooks/hooks.ts (1)
packages/sdk/src/internal/types.ts (3)
SchemaType(51-71)SubscribeParams(511-518)SubscribeResponse(254-257)
packages/sdk/src/internal/types.ts (2)
packages/sdk/src/internal/pagination.ts (1)
Pagination(11-103)packages/sdk/src/internal/toriiQueryBuilder.ts (1)
ToriiQueryBuilder(27-160)
packages/sdk/src/web/index.ts (4)
packages/sdk/src/internal/parseEntities.ts (1)
parseEntities(10-59)packages/sdk/src/internal/pagination.ts (1)
Pagination(11-103)packages/sdk/src/internal/subscribeQueryModel.ts (1)
subscribeQueryModelCallback(17-42)packages/sdk/src/internal/types.ts (4)
SubscribeParams(511-518)SubscribeResponse(254-257)GetParams(520-525)ToriiResponse(249-252)
🪛 Biome (1.9.4)
packages/sdk/src/internal/toriiQueryBuilder.ts
[error] 8-8: Some named imports are only used as types.
This import is only used as a type.
Importing the types with import type ensures that they are removed by the compilers and avoids loading unnecessary modules.
Safe fix: Add inline type keywords.
(lint/style/useImportType)
[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)
examples/example-vite-react-app-recs/src/App.tsx
[error] 24-24: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
packages/react/src/useQuerySync.ts
[error] 31-31: This hook does not specify all of its dependencies: clause
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
packages/sdk/src/web/react/hooks/hooks.ts
[error] 57-57: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
packages/sdk/src/internal/pagination.ts
[error] 11-11: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 38-38: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
packages/sdk/src/internal/types.ts
[error] 5-5: 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)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: check
🔇 Additional comments (40)
examples/example-vite-react-threejs-recs/package.json (1)
20-20: Add @dojoengine/sdk workspace dependency
Includes the new SDK package in this example, aligning it with other updated sample projects for Torii pagination support.examples/example-vite-react-pwa-recs/package.json (1)
21-21: Add @dojoengine/sdk workspace dependency
Ensures PWA example leverages the latest SDK features introduced in this PR, keeping consistency across sample apps.examples/example-vite-react-phaser-recs/package.json (1)
19-19: Add @dojoengine/sdk workspace dependency
Brings Phaser-based example into alignment with the new SDK’s pagination APIs and query improvements.packages/sdk/src/internal/errors.ts (1)
5-6: Introduce UNDEFINED_CLAUSE error constant
Provides a clear, descriptive error when attempting to access a clause before it's defined, improving safety inToriiQueryBuilder.examples/example-vue-app-recs/package.json (1)
21-21: Add @dojoengine/sdk workspace dependency
Updates the Vue example to include the SDK, ensuring compatibility with the new cursor-based pagination support.packages/sdk/src/internal/parseEntities.ts (1)
28-28: Syntax cleanup: Removed trailing comma.The removal of the trailing comma in the tuple type assertion improves code consistency and follows TypeScript style conventions.
examples/example-vite-react-app-recs/package.json (1)
20-20: Added @dojoengine/sdk dependency.Adding the SDK dependency is necessary to support the new pagination features and updated query clause handling.
examples/example-vite-react-app-recs/src/dojo/setup.ts (3)
4-5: Added necessary KeysClause import.The import of KeysClause from the SDK package has been correctly added to support the updated API usage below.
31-31: Updated getSyncEntities to use KeysClause.Changed from using undefined to explicitly building a clause with
KeysClause([], [], "VariableLen").build(), which aligns with the new unified clause handling approach.
49-49: Updated getSyncEvents to use KeysClause.Similarly updated the getSyncEvents call to use a consistent KeysClause pattern, ensuring uniform clause handling across entity and event synchronization.
examples/example-vite-react-app-recs/src/App.tsx (3)
3-3: Type import for Entity adds clarity.The change to a type-only import for
Entityhelps clarify that this is only being used for type checking, not at runtime.
5-5: Adding KeysClause import supports the new pagination model.This import enables the construction of more flexible query clauses for entity filtering.
22-26: Updated useQuerySync call to support the new pagination model.The implementation has been changed to use the new
KeysClausebuilder pattern with "VariableLen" instead of an empty array, aligning with the updated API that requires a properClauseparameter.However, there's a static analysis warning about the use of
anyin the cast ofcontractComponents. Consider adding proper typing:-useQuerySync( - toriiClient, - contractComponents as any, - KeysClause([], [], "VariableLen").build() -); +useQuerySync( + toriiClient, + contractComponents, + KeysClause([], [], "VariableLen").build() +);If type issues persist, consider defining a proper type for
contractComponentsrather than usingany.🧰 Tools
🪛 Biome (1.9.4)
[error] 24-24: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
.changeset/stale-eggs-heal.md (1)
1-15: Well-structured changeset for the pagination feature.The changeset correctly lists all affected packages with patch version bumps, which is appropriate for a feature addition that maintains backward compatibility. The description "feat: Support torii pagination" clearly communicates the primary change.
examples/example-vite-kitchen-sink/src/components/caller-counter.tsx (3)
5-7: Type-only imports improve clarity and bundle size.Converting these imports to type-only declarations clearly indicates they're not needed at runtime, which could help with tree-shaking and bundle size optimization.
Also applies to: 10-12
75-75: More explicit Number.parseInt improves code quality.Replacing the global
parseIntwithNumber.parseIntis more explicit and follows better coding practices by avoiding global namespace pollution. This change also ensures consistent base-16 parsing.Also applies to: 90-90
85-86: Updated to use pagination API with getItems() method.This change adapts to the new pagination model where query results are now wrapped in a
Paginationclass that requires calling.getItems()to access the underlying data.packages/sdk/src/web/experimental/index.ts (2)
42-42: Simplified clause handling in subscribeEntities.The code now passes the raw query clause directly to the Torii client instead of using the deprecated
intoEntityKeysClauseutility, aligning with the unifiedClausetype approach.
84-84: Simplified clause handling in subscribeEvents.Similar to the changes in subscribeEntities, this simplification removes the use of
intoEntityKeysClauseand directly passes the query clause to the underlying client method.examples/example-vite-kitchen-sink/src/components/theme-switch.tsx (2)
3-3: Improved type safety with type-only importsConverting imports to type-only using the
typekeyword is a good practice as it ensures these imports are only used for type checking and won't be included in the compiled JavaScript bundle, potentially reducing bundle size.Also applies to: 7-9, 11-11, 25-25
122-124: Updated data access pattern for paginated resultsThe code now uses
.getItems()[0]to extract data from the subscription results instead of direct array indexing. This aligns with the new pagination support where query results are wrapped in aPaginationclass.packages/sdk/src/internal/toriiQueryBuilder.ts (2)
147-155: Well-implemented Result wrapper for clause accessThe
getClause()method provides a safer way to access the internal clause by wrapping it in a Result type from neverthrow, properly handling the case when the clause is undefined.
157-159: Direct pagination access methodThe
getPagination()method provides a straightforward accessor for the pagination configuration, which helps support the new pagination framework throughout the SDK.examples/example-vite-kitchen-sink/src/components/chat.tsx (3)
6-12: Improved type safety with type-only importsConverting imports to type-only using the
typekeyword reduces bundle size by ensuring these imports are only used for TypeScript typechecking and not at runtime.Also applies to: 18-19, 22-23, 24-26
108-108: Updated data access pattern for paginationThe code now calls
.getItems()to extract the actual entities from the paginated result, which is consistent with the new pagination framework introduced in this PR.
112-113: Safer integer parsing with Number.parseIntReplacing
parseIntwithNumber.parseIntis a good practice as it avoids potential issues with the globalparseIntfunction and makes the code more explicit.examples/example-vite-grpc-playground/src/App.tsx (4)
4-5: Good use of type-only importsClean refactoring to use TypeScript's
typekeyword for imports that are only used as type annotations. This clarifies intent and can potentially improve build optimization.
8-11: Good use of type-only imports for SDK typesProper use of TypeScript's
typeimports for SDK types that are only used in type annotations, not at runtime.
111-111: Updated to use new pagination APIThis change aligns with the new pagination pattern, using
response.getItems()to access the array of items from the paginated response.
122-122: Updated to use new pagination API for length calculationConsistent with line 111, now using
response.getItems().lengthto access the length of the results array from the paginated response.packages/sdk/src/__tests__/models.gen.ts (1)
89-97: Clean removal of WithFieldOrder wrapper typeThe model interface types are now used directly instead of being wrapped with
WithFieldOrder<T>. This simplifies the type system and removes the need to track field ordering separately.packages/sdk/src/internal/subscribeQueryModel.ts (1)
1-42: Great abstraction for standardized subscription callbacksThis new utility function creates a standardized way to handle entity subscription callbacks with proper error handling and data parsing. The implementation:
- Processes raw entity data through
parseEntities- Wraps the result in a standardized response format
- Handles errors gracefully by converting them to Error objects
- Maintains consistent callback interface
This abstraction will help reduce code duplication and enforce consistent error handling patterns across the codebase.
examples/example-vite-kitchen-sink/src/components/global-counter.tsx (4)
6-8: Good use of type-only importsProperly updated imports to use TypeScript's
typekeyword for imports that are only used as type annotations, improving type safety and build clarity.Also applies to: 10-10, 12-12, 14-14
66-66: Improved number parsing methodReplaced global
parseIntwith the more explicitNumber.parseInt, which makes the code clearer and follows better JavaScript practices.
79-80: Updated to use new pagination APIModified to use the new pagination pattern by accessing results through
initialEntities.getItems()[0]instead of direct array access, aligning with the SDK's updated response format.
84-84: Consistent number parsing methodReplaced global
parseIntwithNumber.parseIntfor consistency with the change on line 66.examples/example-node-worker/main.ts (1)
160-168: Fix the call after adjusting the function signatureIf you adopt the first suggestion, the call here is already correct (
events.getItems()returns an array).
Should you keep the current signature instead, remove.getItems():- positionCount = initPositionFromEvent(events.getItems()); + positionCount = initPositionFromEvent(events);Align both ends to keep the types consistent.
packages/sdk/src/internal/pagination.ts (1)
92-102: Direction reversal logic OK – but verify cursor semantics
getPreviousQueryreuses the current cursor; depending on backend semantics you might need the first item’s cursor of the current page rather thanthis.cursor(often the last cursor). Double-check this against the Torii API to avoid empty or duplicated pages.packages/sdk/src/node/index.ts (1)
27-30: Unused import?
subscribeQueryModelCallbackis imported but only used insidesubscribeEntityQuery/subscribeEventQuery.
If tree-shaking is unavailable on Node builds, that’s fine, otherwise verify the import isn’t accidentally duplicated elsewhere.packages/sdk/src/web/index.ts (1)
85-88:q.clausemay beundefinedbutonEntityUpdatedexpects aClauseIf the user builds a query without
.withClause(...), this will compile (becauseclauseis optional on theQuerytype) but will fail at runtime if the Torii client requires a non-undefined clause.Add a guard or default clause before subscribing, e.g.:
const clause = q.clause ?? { All: {} };or surface a descriptive error.
| @@ -18,7 +18,7 @@ export function useEntityQuery<Schema extends SchemaType>( | |||
| subscribeMethod: (options) => sdk.subscribeEntityQuery(options), | |||
| updateSubscriptionMethod: (subscription, clause) => | |||
| sdk.updateEntitySubscription(subscription, clause), | |||
| queryToHashedKeysMethod: (query) => sdk.toriiQueryIntoHashedKeys(query), | |||
| queryToHashedKeysMethod: (query) => query.getClause()._unsafeUnwrap(), | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Replaced deprecated key conversion with direct clause extraction.
The code now directly extracts the clause using query.getClause()._unsafeUnwrap() instead of the deprecated sdk.toriiQueryIntoHashedKeys(query) method.
The use of _unsafeUnwrap() suggests bypassing safety checks. Verify that the query always returns a valid clause in this context to prevent potential runtime errors.
🏁 Script executed:
#!/bin/bash
# Look for defensive checks or error handling around the use of _unsafeUnwrap
rg -A 5 -B 5 "_unsafeUnwrap\(\)" --type tsLength of output: 3001
I’ll locate the getClause implementation to confirm its return type and whether it can ever be empty, which would make _unsafeUnwrap() unsafe.
🏁 Script executed:
#!/bin/bash
# Find where getClause is declared/implemented
rg -n "getClause\(" --type ts
# Find Query class/interface definitions
rg -nE "class .*Query|interface .*Query" --type tsLength of output: 951
Let’s inspect the full getClause implementation to see under what conditions it can return an error:
🏁 Script executed:
#!/bin/bash
# Dump the getClause method from toriiQueryBuilder for review
sed -n '140,200p' packages/sdk/src/internal/toriiQueryBuilder.tsLength of output: 974
I’ll inspect createSubscriptionHook to see how and when the query object is instantiated and whether it guarantees a clause before calling _unsafeUnwrap():
🏁 Script executed:
#!/bin/bash
# Locate the file defining createSubscriptionHook and dump its implementation
FILE=$(rg -l "createSubscriptionHook" --type ts | head -n1)
echo "createSubscriptionHook found in: $FILE"
sed -n '1,200p' "$FILE"Length of output: 1814
I’ll inspect the ToriiQueryBuilder constructor, clause initialization, and default options to confirm whether clause is always defined before calling _unsafeUnwrap().
🏁 Script executed:
#!/bin/bash
# Preview ToriiQueryBuilder constructor and clause handling
sed -n '1,100p' packages/sdk/src/internal/toriiQueryBuilder.ts
# Look for where a clause is set on the builder
rg -n "withClause" -A 5 packages/sdk/src/internal/toriiQueryBuilder.ts
# Check default options to see if clause is always provided
rg -n "defaultToriiOptions" -A 20 packages/sdk/src/internalLength of output: 6176
Ensure clause is set before unwrapping
The call to query.getClause()._unsafeUnwrap() will throw if no clause has been provided. Since defaultToriiOptions initializes clause to undefined, any ToriiQueryBuilder not explicitly configured with .withClause(...) will cause a runtime error.
• packages/sdk/src/web/react/hooks/entities.ts:21
queryToHashedKeysMethod: (query) => query.getClause()._unsafeUnwrap()
Please address this by either:
- Enforcing at the type or API level that the builder passed into
useEntityQuerymust include a clause (e.g. require.withClause(...)before the hook), or - Handling the
Result<Clause, string>safely instead of using_unsafeUnwrap(), for example:or// Throw with a clear message const clause = query.getClause().unwrapOrElse(() => { throw new Error("Dojo.js - useEntityQuery: missing query clause") });
// Match on both cases query.getClause().match({ ok: (c) => sdk.updateEntitySubscription(sub, c), err: (e) => console.error("Missing clause:", e), });
This will prevent unexpected runtime errors when no clause has been set.
| @@ -22,8 +22,7 @@ export function useEventQuery<Schema extends SchemaType>( | |||
| subscribeMethod: (options) => sdk.subscribeEventQuery(options), | |||
| updateSubscriptionMethod: (subscription, clause) => | |||
| sdk.updateEventMessageSubscription(subscription, clause, false), | |||
| queryToHashedKeysMethod: (query) => | |||
| sdk.toriiEventMessagesQueryIntoHashedKeys(query, false), | |||
| queryToHashedKeysMethod: (query) => query.getClause()._unsafeUnwrap(), | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider safer unwrapping of the clause Result
The current implementation uses _unsafeUnwrap() which will throw an exception if the clause is undefined. Consider using a safer approach to handle potential errors.
-queryToHashedKeysMethod: (query) => query.getClause()._unsafeUnwrap(),
+queryToHashedKeysMethod: (query) => {
+ const clauseResult = query.getClause();
+ if (clauseResult.isErr()) {
+ throw new Error(`Failed to get clause: ${clauseResult.error}`);
+ }
+ return clauseResult.value;
+},📝 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.
| queryToHashedKeysMethod: (query) => query.getClause()._unsafeUnwrap(), | |
| queryToHashedKeysMethod: (query) => { | |
| const clauseResult = query.getClause(); | |
| if (clauseResult.isErr()) { | |
| throw new Error(`Failed to get clause: ${clauseResult.error}`); | |
| } | |
| return clauseResult.value; | |
| }, |
| @@ -61,8 +60,7 @@ export function useHistoricalEventsQuery<Schema extends SchemaType>( | |||
| subscribeMethod: (options) => sdk.subscribeEventQuery(options), | |||
| updateSubscriptionMethod: (subscription, clause) => | |||
| sdk.updateEventMessageSubscription(subscription, clause, true), | |||
| queryToHashedKeysMethod: (query) => | |||
| sdk.toriiEventMessagesQueryIntoHashedKeys(query, true), | |||
| queryToHashedKeysMethod: (query) => query.getClause()._unsafeUnwrap(), | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Same unsafe unwrapping issue as above
This line has the same issue as line 25 - using _unsafeUnwrap() without error handling.
-queryToHashedKeysMethod: (query) => query.getClause()._unsafeUnwrap(),
+queryToHashedKeysMethod: (query) => {
+ const clauseResult = query.getClause();
+ if (clauseResult.isErr()) {
+ throw new Error(`Failed to get clause: ${clauseResult.error}`);
+ }
+ return clauseResult.value;
+},📝 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.
| queryToHashedKeysMethod: (query) => query.getClause()._unsafeUnwrap(), | |
| queryToHashedKeysMethod: (query) => { | |
| const clauseResult = query.getClause(); | |
| if (clauseResult.isErr()) { | |
| throw new Error(`Failed to get clause: ${clauseResult.error}`); | |
| } | |
| return clauseResult.value; | |
| }, |
| import { describe, it, expect } from "vitest"; | ||
| import { Pagination } from "../internal/pagination"; | ||
| import { ToriiQueryBuilder } from "../internal/toriiQueryBuilder"; | ||
| import type { schema } from "./models.gen"; |
There was a problem hiding this comment.
Importing schema as a type only breaks typeof usage
typeof schema requires schema to be a runtime value.
Swap to a value import (and keep a type alias if needed):
-import type { schema } from "./models.gen";
+import { schema } from "./models.gen";
+import type { schema as SchemaType } from "./models.gen";Then adjust the generic arguments to use SchemaType if you still need the type.
Compilation will otherwise fail with “schema only refers to a type...”.
📝 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.
| import type { schema } from "./models.gen"; | |
| -import type { schema } from "./models.gen"; | |
| +import { schema } from "./models.gen"; | |
| +import type { schema as SchemaType } from "./models.gen"; |
| * @template T - The schema type that extends SchemaType | ||
| * @template Inner - The type of items being paginated (must be an array type) | ||
| */ | ||
| export class Pagination<T extends SchemaType, Inner extends any[]> { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid any in the generic constraint
Extending Inner from any[] defeats the purpose of strong typing and triggers the noExplicitAny linter error.
Prefer unknown[] (or a more precise item type) to keep type-safety while still allowing arrays.
-export class Pagination<T extends SchemaType, Inner extends any[]> {
+export class Pagination<T extends SchemaType, Items extends unknown[]> {You will also need to rename the occurrences of Inner inside the class.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (1.9.4)
[error] 11-11: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
| constructor( | ||
| public limit: number, | ||
| public cursor?: string, | ||
| public direction?: string | ||
| ) { | ||
| this.items = [] as unknown as Inner; | ||
| if (!direction) { | ||
| this.direction = "Forward"; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
direction should use the PaginationDirection enum, not string
Typing direction as a plain string removes compile-time guarantees and forces later casts (as PaginationDirection).
Make the parameter strongly typed and give it a default value directly in the signature.
- constructor(
- public limit: number,
- public cursor?: string,
- public direction?: string
- ) {
- this.items = [] as unknown as Inner;
- if (!direction) {
- this.direction = "Forward";
- }
+ constructor(
+ public limit: number,
+ public cursor?: string,
+ public direction: PaginationDirection = "Forward",
+ ) {
+ this.items = [] as unknown as Items;
}This eliminates the cast at line 81 and the fallback logic at lines 27-30.
📝 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.
| constructor( | |
| public limit: number, | |
| public cursor?: string, | |
| public direction?: string | |
| ) { | |
| this.items = [] as unknown as Inner; | |
| if (!direction) { | |
| this.direction = "Forward"; | |
| } | |
| } | |
| constructor( | |
| public limit: number, | |
| public cursor?: string, | |
| public direction: PaginationDirection = "Forward", | |
| ) { | |
| this.items = [] as unknown as Items; | |
| } |
| const entities = await client.getEntities(q); | ||
| const parsedEntities = parseEntities<T>(entities.items); | ||
| return [ | ||
| entities, | ||
| Pagination.fromQuery(query).withItems(parsedEntities), | ||
| client.onEntityUpdated( | ||
| intoEntityKeysClause<T>(q.clause, entities), | ||
| (_: string, entityData: torii.Entity) => { | ||
| try { | ||
| if (callback) { | ||
| const parsedData = parseEntities<T>([ | ||
| entityData, | ||
| ]); | ||
| callback({ | ||
| data: parsedData, | ||
| error: undefined, | ||
| }); | ||
| } | ||
| } catch (error) { | ||
| if (callback) { | ||
| callback({ | ||
| data: undefined, | ||
| error: | ||
| error instanceof Error | ||
| ? error | ||
| : new Error(String(error)), | ||
| }); | ||
| } | ||
| } | ||
| } | ||
| q.clause, | ||
| subscribeQueryModelCallback(callback) | ||
| ), |
There was a problem hiding this comment.
parseEntities receives the wrong shape – likely runtime crash
client.getEntities returns an object whose items property is a map ({ [hashedKey]: models }).
parseEntities expects an array of entities ([{ hashed_keys, models }]). Passing the map leads to:
TypeError: Cannot convert undefined or null to object
inside for (const modelName in entity.models).
Either convert the map to an array:
-const parsedEntities = parseEntities<T>(entities.items);
+const parsedEntities = parseEntities<T>(Object.values(entities.items));or adjust parseEntities to accept the map. Apply the same fix in subscribeEventQuery, getEntities, and getEventMessages.
MartianGreed
force-pushed
the
feat/support-torii-pagination
branch
from
f31385b to
75d0968
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (2)
packages/react/src/useQuerySync.ts (1)
31-37:⚠️ Potential issueMissing dependency in useCallback
The
setupSyncuseCallback usesclausebut doesn't list it in its dependency array. This could cause stale closure issues ifclausechanges but the callback doesn't update.- }, [toriiClient, components]); + }, [toriiClient, components, clause]);🧰 Tools
🪛 Biome (1.9.4)
[error] 35-35: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.(lint/complexity/noUselessCatch)
[error] 31-31: This hook does not specify all of its dependencies: clause
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
packages/sdk/src/web/react/hooks/hooks.ts (1)
66-67:⚠️ Potential issueNon-null assertion should be guarded
The same issue with non-null assertion exists here as well.
fetchingRef.current!should be guarded to prevent potential runtime errors.- const [initialData, subscription] = await config.subscribeMethod({ - query: fetchingRef.current!, + if (!fetchingRef.current) { + isUpdating.current = false; + return null; + } + + const [initialData, subscription] = await config.subscribeMethod({ + query: fetchingRef.current,🧰 Tools
🪛 Biome (1.9.4)
[error] 67-67: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
♻️ Duplicate comments (8)
packages/sdk/src/web/react/hooks/hooks.ts (1)
54-62: Remove unsafe non-null assertion and add a guardThe code still contains a non-null assertion on
fetchingRef.current!which could lead to runtime errors if it's null.- await config.updateSubscriptionMethod( - subscriptionRef.current, - config.queryToHashedKeysMethod( - fetchingRef.current!, - config.historical - ), - config.historical - ); + if (fetchingRef.current) { + await config.updateSubscriptionMethod( + subscriptionRef.current, + config.queryToHashedKeysMethod( + fetchingRef.current, + config.historical + ), + config.historical + ); + }🧰 Tools
🪛 Biome (1.9.4)
[error] 58-58: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
packages/sdk/src/internal/pagination.ts (2)
11-13: Avoidanyin the generic constraint (same issue as previously raised)
Inner extends any[]defeats type–safety and keeps the linter error (noExplicitAny).
Useunknown[](or an even more precise item type) instead.-export class Pagination<T extends SchemaType, Inner extends any[]> { +export class Pagination<T extends SchemaType, Items extends unknown[]> {Remember to rename every occurrence of
InnertoItemsthroughout the file.🧰 Tools
🪛 Biome (1.9.4)
[error] 11-11: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
21-30: Makedirectionstrongly-typed & give it a default directly in the signatureThe constructor still accepts
direction?: stringand then patches missing values at runtime.
Prefer compile-time guarantees with the enum and drop the runtime fallback:- constructor( - public limit: number, - public cursor?: string, - public direction?: string - ) { - this.items = [] as unknown as Inner; - if (!direction) { - this.direction = "Forward"; - } + constructor( + public limit: number, + public cursor?: string, + public direction: PaginationDirection = "Forward", + ) { + this.items = [] as unknown as Items; }This removes the unnecessary cast at line 81 and guarantees
this.directionis always defined.packages/sdk/src/web/index.ts (4)
81-84: Shape mismatch:parseEntitiesstill receives a map, not an arrayPrevious review pointed this out—
entities.itemsis a map{ [hashedKey]: models }, whereasparseEntitiesexpects an array.-const parsedEntities = parseEntities<T>(entities.items); +const parsedEntities = parseEntities<T>(Object.values(entities.items));Apply the same fix everywhere
parseEntitiesis called withentities.items.
103-107: Repeat of the map/array mismatch insidesubscribeEventQuerySame correction as above is required here.
-const parsedEntities = parseEntities<T>(entities.items); +const parsedEntities = parseEntities<T>(Object.values(entities.items));
135-139: Third occurrence –getEntitiesPlease replace
entities.itemswithObject.values(entities.items)before parsing.
152-156: Fourth occurrence –getEventMessagesApply the same fix to avoid runtime crashes.
packages/sdk/src/node/index.ts (1)
62-69:client.getEntities(q)now expected to return{ items: ... }Previously
parseEntitiesconsumed the entire response. Ensuretorii-wasmindeed wraps results in anitemsfield; otherwise this will parseundefinedand return an empty array silently.To verify this implementation properly handles the new response format:
#!/bin/bash # Check if torii-wasm is properly returning items field structure ast-grep --pattern 'client.getEntities' | grep -A 5 -B 5
🧹 Nitpick comments (5)
packages/sdk/src/internal/toriiQueryBuilder.ts (1)
8-10: Consider usingimport typefor typesThese imports should use the
import typesyntax since some of them are only used for type annotations.-import { Result, ok, err } from "neverthrow"; -import { UNDEFINED_CLAUSE } from "./errors.ts"; -import { Pagination } from "@dojoengine/torii-wasm"; +import { ok, err } from "neverthrow"; +import type { Result } from "neverthrow"; +import { UNDEFINED_CLAUSE } from "./errors.ts"; +import type { Pagination } from "@dojoengine/torii-wasm";This helps with tree-shaking and clarifies which imports are used for runtime vs. type information.
🧰 Tools
🪛 Biome (1.9.4)
[error] 8-8: Some named imports are only used as types.
This import is only used as a type.
Importing the types with import type ensures that they are removed by the compilers and avoids loading unnecessary modules.
Safe fix: Add inline type keywords.(lint/style/useImportType)
[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)
packages/sdk/src/internal/pagination.ts (1)
26-27: Casting away type-safety
this.items = [] as unknown as Inner;suppresses all checks and can hide mistakes.
After switching toItems extends unknown[], you can safely initialise with:this.items = [] as unknown as Items;…but consider adding a factory (
items?: Items = [] as unknown as Items) to avoid the cast altogether.packages/sdk/src/web/index.ts (1)
274-277: Doc-comment no longer matches the signatureThe parameter is now
torii.Clause(single clause), but the JSDoc still says “array of key clauses”.
Update the description to avoid confusion for SDK consumers.migration/v1-4-v1-5.md (1)
40-45: Typo: static helper name differs from implementationThe guide refers to
ToriiQueryBuilder.withPagination, but the codebase introducesToriiQueryBuilder.fromPagination(or similar) in some examples.
Please align the helper name in the docs with the actual exported API to prevent onboarding friction.packages/sdk/src/internal/types.ts (1)
5-5: Consider using import type for PaginationThe static analysis tools suggest that this import is only used as a type, not as a value.
-import { Pagination } from "./pagination.ts"; +import type { Pagination } from "./pagination.ts";🧰 Tools
🪛 Biome (1.9.4)
[error] 5-5: 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)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (5)
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/**examples/example-vue-app-recs/src/dojo/generated/setup.tsis excluded by!**/generated/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (34)
.changeset/stale-eggs-heal.md(1 hunks)examples/example-node-worker/main.ts(4 hunks)examples/example-vite-grpc-playground/src/App.tsx(3 hunks)examples/example-vite-kitchen-sink/src/components/caller-counter.tsx(3 hunks)examples/example-vite-kitchen-sink/src/components/chat.tsx(2 hunks)examples/example-vite-kitchen-sink/src/components/global-counter.tsx(3 hunks)examples/example-vite-kitchen-sink/src/components/theme-switch.tsx(3 hunks)examples/example-vite-react-app-recs/package.json(1 hunks)examples/example-vite-react-app-recs/src/App.tsx(2 hunks)examples/example-vite-react-app-recs/src/dojo/setup.ts(3 hunks)examples/example-vite-react-phaser-recs/package.json(1 hunks)examples/example-vite-react-pwa-recs/package.json(1 hunks)examples/example-vite-react-threejs-recs/package.json(1 hunks)examples/example-vue-app-recs/package.json(1 hunks)migration/v1-4-v1-5.md(1 hunks)packages/react/src/useQuerySync.ts(2 hunks)packages/sdk/src/__tests__/convertClauseToEntityKeysClause.test.ts(0 hunks)packages/sdk/src/__tests__/models.gen.ts(1 hunks)packages/sdk/src/__tests__/pagination.test.ts(1 hunks)packages/sdk/src/internal/convertClauseToEntityKeysClause.ts(0 hunks)packages/sdk/src/internal/errors.ts(1 hunks)packages/sdk/src/internal/pagination.ts(1 hunks)packages/sdk/src/internal/parseEntities.ts(1 hunks)packages/sdk/src/internal/subscribeQueryModel.ts(1 hunks)packages/sdk/src/internal/toriiQueryBuilder.ts(2 hunks)packages/sdk/src/internal/types.ts(8 hunks)packages/sdk/src/node/index.ts(5 hunks)packages/sdk/src/web/experimental/index.ts(2 hunks)packages/sdk/src/web/index.ts(6 hunks)packages/sdk/src/web/react/hooks/entities.ts(1 hunks)packages/sdk/src/web/react/hooks/events.ts(2 hunks)packages/sdk/src/web/react/hooks/hooks.ts(5 hunks)packages/state/src/recs/index.ts(11 hunks)packages/torii-wasm/dojo.c(1 hunks)
💤 Files with no reviewable changes (2)
- packages/sdk/src/tests/convertClauseToEntityKeysClause.test.ts
- packages/sdk/src/internal/convertClauseToEntityKeysClause.ts
✅ Files skipped from review due to trivial changes (4)
- examples/example-vite-react-threejs-recs/package.json
- examples/example-vite-react-phaser-recs/package.json
- packages/sdk/src/internal/parseEntities.ts
- .changeset/stale-eggs-heal.md
🚧 Files skipped from review as they are similar to previous changes (19)
- packages/sdk/src/internal/errors.ts
- examples/example-vite-react-app-recs/package.json
- examples/example-vue-app-recs/package.json
- examples/example-vite-react-app-recs/src/dojo/setup.ts
- packages/torii-wasm/dojo.c
- examples/example-vite-kitchen-sink/src/components/theme-switch.tsx
- examples/example-vite-react-pwa-recs/package.json
- packages/sdk/src/web/react/hooks/entities.ts
- packages/sdk/src/web/react/hooks/events.ts
- examples/example-vite-kitchen-sink/src/components/global-counter.tsx
- packages/sdk/src/web/experimental/index.ts
- examples/example-vite-kitchen-sink/src/components/chat.tsx
- packages/sdk/src/internal/subscribeQueryModel.ts
- examples/example-node-worker/main.ts
- examples/example-vite-grpc-playground/src/App.tsx
- packages/sdk/src/tests/models.gen.ts
- examples/example-vite-kitchen-sink/src/components/caller-counter.tsx
- packages/sdk/src/tests/pagination.test.ts
- packages/state/src/recs/index.ts
🧰 Additional context used
🧬 Code Graph Analysis (4)
examples/example-vite-react-app-recs/src/App.tsx (1)
packages/react/src/useQuerySync.ts (1)
useQuerySync(26-57)
packages/react/src/useQuerySync.ts (1)
packages/state/src/recs/index.ts (1)
getSyncEntities(49-69)
packages/sdk/src/internal/toriiQueryBuilder.ts (2)
packages/sdk/src/internal/errors.ts (1)
UNDEFINED_CLAUSE(5-6)packages/sdk/src/internal/pagination.ts (1)
Pagination(11-103)
packages/sdk/src/web/react/hooks/hooks.ts (1)
packages/sdk/src/internal/types.ts (2)
SubscribeParams(511-518)SubscribeResponse(254-257)
🪛 Biome (1.9.4)
packages/sdk/src/internal/types.ts
[error] 5-5: 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)
examples/example-vite-react-app-recs/src/App.tsx
[error] 24-24: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
packages/react/src/useQuerySync.ts
[error] 31-31: This hook does not specify all of its dependencies: clause
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
packages/sdk/src/internal/pagination.ts
[error] 11-11: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 38-38: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
packages/sdk/src/internal/toriiQueryBuilder.ts
[error] 8-8: Some named imports are only used as types.
This import is only used as a type.
Importing the types with import type ensures that they are removed by the compilers and avoids loading unnecessary modules.
Safe fix: Add inline type keywords.
(lint/style/useImportType)
[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)
packages/sdk/src/web/react/hooks/hooks.ts
[error] 58-58: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: check
- GitHub Check: build
🔇 Additional comments (21)
examples/example-vite-react-app-recs/src/App.tsx (2)
3-3: Good use of type importConverting the
Entityimport to a type-only import is a positive change that helps with tree-shaking and clarifies the intent of the import.
22-26:❌ Incorrect review comment
Type safety concern with the
as anyassertionThe updated code correctly adapts to the new pagination API by using
KeysClause, but theas anytype assertion should be avoided as it bypasses TypeScript's type checking.useQuerySync( toriiClient, - contractComponents as any, + contractComponents, KeysClause([], [], "VariableLen").build() );If there's a type compatibility issue that necessitates the assertion, consider creating a properly typed adapter function instead.
🏁 Script executed:
#!/bin/bash # Check if the contractComponents is compatible with the expected type grep -r "contractComponents" ./examples/example-vite-react-app-recs/src/dojo/Length of output: 1202
🏁 Script executed:
#!/bin/bash # Display the beginning of App.tsx to inspect imports and usage of useQuerySync sed -n '1,200p' examples/example-vite-react-app-recs/src/App.tsxLength of output: 8975
🏁 Script executed:
#!/bin/bash # Display the content of useDojo.ts to inspect the types of contractComponents returned sed -n '1,200p' examples/example-vite-react-app-recs/src/dojo/useDojo.tsLength of output: 172
🏁 Script executed:
#!/bin/bash # Display the content of setup.ts to inspect the definition and typing of contractComponents sed -n '1,200p' examples/example-vite-react-app-recs/src/dojo/setup.tsLength of output: 3347
🏁 Script executed:
#!/bin/bash # Find and display the type definition of defineContractComponents file_path=$(fd 'models\.gen\.ts$' -t f examples/example-vite-react-app-recs/src) echo "Found models.gen at: $file_path" sed -n '1,200p' "$file_path"Length of output: 6076
Keep
as anyoncontractComponentsdue to current SDK typing mismatchThe cast is required because the generated
ContractComponentstype doesn’t yet match the parameter type expected byuseQuerySync(and similarly bygetSyncEntities/getSyncEventsinsetup.ts). Removing it will introduce TypeScript errors.• examples/example-vite-react-app-recs/src/App.tsx (lines 22–26): retain
useQuerySync( toriiClient, contractComponents as any, KeysClause([], [], "VariableLen").build() );• examples/example-vite-react-app-recs/src/dojo/setup.ts: likewise casts
contractComponents as anyforgetSyncEntitiesandgetSyncEventsNo changes needed here; instead, consider filing an issue or PR against
@dojoengine/reactto update theuseQuerySyncsignature (or introduce a properly typed adapter function) so thatContractComponentscan be passed withoutany.Likely an incorrect or invalid review comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 24-24: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
packages/sdk/src/internal/toriiQueryBuilder.ts (2)
150-155: Good error handling with Result typeThe implementation of
getClause()using the Result type from neverthrow is a robust approach to handling potential undefined values. This prevents runtime errors and provides clear error messaging.
157-159: Simple accessor for paginationThis accessor method provides a clean way to access the pagination configuration, supporting the new pagination model.
packages/react/src/useQuerySync.ts (2)
2-8: Good use of type importsConverting imports to use
import typefor type-only imports is a good practice that helps with tree-shaking and clarifies the intent of the imports.
26-30: API change:clauseparameter is now requiredThe function signature change makes the API clearer by requiring a
Clauseparameter instead of making it optional or accepting anentityKeyClause. This is aligned with the new pagination model.packages/sdk/src/web/react/hooks/hooks.ts (1)
85-85: Updated data extraction with pagination modelUsing the
getItems()method aligns well with the new pagination model and provides a cleaner approach to access the data.packages/sdk/src/node/index.ts (6)
27-29: Great addition of new imports for pagination supportThese new imports support the implementation of cursor-based pagination, centralizing entity parsing and subscription callback handling.
84-91: Updated to use cursor-based pagination for event messagesThe event subscription handling has been updated to use the new Pagination model with
itemsarray extraction. This is consistent with the changes to entity subscriptions.
116-120: Entity queries now return paginated resultsGood conversion to the new pagination pattern. Entity queries now return a
Paginationobject rather than raw arrays, allowing for easier navigation between pages.
133-137: Event message queries now return paginated resultsSimilar to entity queries, event message queries now correctly return a
Paginationobject for consistent API usage and cursor-based navigation.
286-294: Type change for updateEventMessageSubscription parameterSimilar to the entity subscription update, the event message subscription update now expects a single Clause instead of an array of EntityKeysClause objects.
267-272:✅ Verification successful
Type change from EntityKeysClause[] to a single Clause
The
updateEntitySubscriptionmethod now accepts a singletorii.Clauseinstead of an array ofEntityKeysClause. Ensure that all call sites have been updated to match this signature change.
🏁 Script executed:
#!/bin/bash # Search for updateEntitySubscription calls to verify they use the new signature rg "updateEntitySubscription\(" --type ts -A 2 -B 2Length of output: 1219
All
updateEntitySubscriptioncalls now use a singletorii.ClauseVerified that every invocation of
updateEntitySubscriptionhas been updated to accept a singletorii.Clauserather than an array:
- packages/sdk/src/web/index.ts
- packages/sdk/src/web/react/hooks/entities.ts
- packages/sdk/src/node/index.ts
No remaining call sites use
EntityKeysClause[].packages/sdk/src/internal/types.ts (8)
249-252: Updated ToriiResponse to use Pagination classGood change. The
ToriiResponsetype now consistently returns aPaginationobject, simplifying the API and improving type safety.
254-257: Simplified SubscribeResponse typeThe removal of the conditional Historical generic parameter makes the API more consistent and easier to understand.
311-314: Simplified subscribeEntityQuery signatureGood removal of the unnecessary generic parameter from the method signature, making it more straightforward to use.
323-325: Simplified subscribeEventQuery signatureSimilar to the entity query subscription, this method signature has been simplified by removing the generic parameter.
345-345: Simplified getEntities and getEventMessages return typesBoth methods now return a consistent
ToriiResponse<T>type using the Pagination class, improving API consistency.Also applies to: 354-354
432-433: Updated update subscription methods to use a single ClauseThe change from accepting an array of EntityKeysClause objects to a single Clause object simplifies the API and aligns with the broader refactoring.
Also applies to: 449-450
511-518: Appropriately deprecated historical flag in SubscribeParamsGood use of the deprecation comment to guide users to use
query.historicalinstead. Consider also updating documentation to reflect this change.
520-525: Appropriately deprecated historical flag in GetParamsSimilar to SubscribeParams, the historical flag is properly marked as deprecated with guidance to users.
| getNextQuery(query: ToriiQueryBuilder<T>): ToriiQueryBuilder<T> { | ||
| const q = query.withLimit(this.limit); | ||
| if (this.cursor) { | ||
| q.withCursor(this.cursor); | ||
| } | ||
| if (q.getPagination().direction !== this.direction) { | ||
| q.withDirection(this.direction as PaginationDirection); | ||
| } | ||
| return q; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
getNextQuery mutates the caller’s builder – may cause subtle bugs
const q = query.withLimit(this.limit); mutates the same ToriiQueryBuilder instance, so the original query is no longer reusable by the caller.
If ToriiQueryBuilder.withLimit is not immutable, clone before mutating:
-const q = query.withLimit(this.limit);
+const q = query.clone().withLimit(this.limit);(or add a .clone() inside ToriiQueryBuilder).
Otherwise subsequent calls that reuse query will silently carry the modified limit/cursor.
Committable suggestion skipped: line range outside the PR's diff.
MartianGreed
force-pushed
the
feat/support-torii-pagination
branch
from
75d0968 to
baadeed
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
packages/react/src/useQuerySync.ts (1)
31-37: 💡 Verification agent🧩 Analysis chain
Add
clauseto useCallback dependencies.The
setupSyncfunction depends on theclauseparameter but it's missing from the dependency array. This could cause stale closures if the clause changes but the callback doesn't update.- }, [toriiClient, components]); + }, [toriiClient, components, clause]);
🏁 Script executed:
#!/bin/bash # Check for similar patterns in other hooks in the codebase rg -A 15 "useCallback\(\([^)]*\)[^{]*{[^}]*clause[^}]*}\s*,\s*\[[^\]]*\]"Length of output: 248
🏁 Script executed:
#!/bin/bash # Show useCallback usage and verify if 'clause' appears in the dependency array rg -n "useCallback" packages/react/src/useQuerySync.ts rg -n "clause" packages/react/src/useQuerySync.tsLength of output: 375
Add
clausetouseCallbackdependencies
ThesetupSynccallback reads theclausevariable but it isn’t listed in its dependency array—this can lead to stale closures whenclausechanges.• File:
packages/react/src/useQuerySync.ts
• Location: lines 31–37Suggested diff:
- }, [toriiClient, components]); + }, [toriiClient, components, clause]);🧰 Tools
🪛 Biome (1.9.4)
[error] 35-35: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.(lint/complexity/noUselessCatch)
[error] 31-31: This hook does not specify all of its dependencies: clause
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
♻️ Duplicate comments (6)
packages/sdk/src/web/react/hooks/hooks.ts (1)
57-60:⚠️ Potential issueFix unsafe non-null assertion
The use of non-null assertion on
fetchingRef.current!is flagged by Biome and presents a potential runtime error iffetchingRef.currentis null.Apply this guard to prevent potential crashes:
- config.queryToHashedKeysMethod( - fetchingRef.current!, - config.historical - ), + fetchingRef.current ? config.queryToHashedKeysMethod( + fetchingRef.current, + config.historical + ) : undefined,🧰 Tools
🪛 Biome (1.9.4)
[error] 58-58: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
packages/sdk/src/internal/pagination.ts (4)
11-11:⚠️ Potential issueAvoid using
anyin generic constraintsUsing
anyin generic constraints bypasses TypeScript's type checking and trigger linter errors. This was also flagged in a previous review.-export class Pagination<T extends SchemaType, Inner extends any[]> { +export class Pagination<T extends SchemaType, Inner extends unknown[]> {🧰 Tools
🪛 Biome (1.9.4)
[error] 11-11: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
21-30: 🛠️ Refactor suggestionUse enum type for direction parameter
The direction parameter should use the
PaginationDirectionenum type rather than string for better type safety. This was also flagged in a previous review.constructor( public limit: number, public cursor?: string, - public direction?: string + public direction: PaginationDirection = "Forward" ) { this.items = [] as unknown as Inner; - if (!direction) { - this.direction = "Forward"; - } }
75-84: 🛠️ Refactor suggestionAvoid mutating the caller's query builder
The
getNextQuerymethod potentially mutates the caller'sToriiQueryBuilderinstance by directly modifying it after assignment, which could lead to unexpected behavior if the original query is reused.getNextQuery(query: ToriiQueryBuilder<T>): ToriiQueryBuilder<T> { - const q = query.withLimit(this.limit); + const q = query.clone().withLimit(this.limit); if (this.cursor) { q.withCursor(this.cursor); } if (q.getPagination().direction !== this.direction) { q.withDirection(this.direction as PaginationDirection); } return q; }
92-101: 🛠️ Refactor suggestionHandle potential undefined direction in getPreviousQuery
The
getPreviousQuerymethod doesn't check ifthis.directionis undefined before passing it togetReversedDirection(), which could lead to a runtime error.getPreviousQuery(query: ToriiQueryBuilder<T>): ToriiQueryBuilder<T> { - const q = query.withLimit(this.limit); + const q = query.clone().withLimit(this.limit); if (this.cursor) { q.withCursor(this.cursor); } + if (!this.direction) { + this.direction = "Forward"; + } if (q.getPagination().direction === this.direction) { q.withDirection(getReversedDirection(this.direction)); } return q; }Also apply the same
clone()fix as ingetNextQueryto avoid mutation of the original query.packages/sdk/src/node/index.ts (1)
62-64: Response structure change requiresitemsfield access.The code now expects
client.getEntities(q)to return an object with anitemsfield, which is then passed toparseEntities. This aligns with the new pagination structure but represents a significant change in the API contract.
🧹 Nitpick comments (2)
packages/sdk/src/internal/toriiQueryBuilder.ts (1)
8-10: Use import type for type-only imports.The imports from
neverthrowand@dojoengine/torii-wasmshould use theimport typesyntax since they are only used as type references.-import { Result, ok, err } from "neverthrow"; -import { UNDEFINED_CLAUSE } from "./errors.ts"; -import { Pagination } from "@dojoengine/torii-wasm"; +import type { Result } from "neverthrow"; +import { ok, err } from "neverthrow"; +import { UNDEFINED_CLAUSE } from "./errors.ts"; +import type { Pagination } from "@dojoengine/torii-wasm";🧰 Tools
🪛 Biome (1.9.4)
[error] 8-8: Some named imports are only used as types.
This import is only used as a type.
Importing the types with import type ensures that they are removed by the compilers and avoids loading unnecessary modules.
Safe fix: Add inline type keywords.(lint/style/useImportType)
[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)
packages/sdk/src/internal/types.ts (1)
5-5: Import statement should useimport type.The
Paginationimport is only used as a type in this file and should use theimport typesyntax to ensure it's removed during compilation, avoiding unnecessary module loading.-import { Pagination } from "./pagination.ts"; +import type { Pagination } from "./pagination.ts";🧰 Tools
🪛 Biome (1.9.4)
[error] 5-5: 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)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (5)
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/**examples/example-vue-app-recs/src/dojo/generated/setup.tsis excluded by!**/generated/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (35)
.changeset/stale-eggs-heal.md(1 hunks)examples/example-node-worker/main.ts(4 hunks)examples/example-vite-grpc-playground/src/App.tsx(3 hunks)examples/example-vite-kitchen-sink/src/components/caller-counter.tsx(3 hunks)examples/example-vite-kitchen-sink/src/components/chat.tsx(2 hunks)examples/example-vite-kitchen-sink/src/components/global-counter.tsx(3 hunks)examples/example-vite-kitchen-sink/src/components/theme-switch.tsx(3 hunks)examples/example-vite-react-app-recs/package.json(1 hunks)examples/example-vite-react-app-recs/src/App.tsx(2 hunks)examples/example-vite-react-app-recs/src/dojo/setup.ts(3 hunks)examples/example-vite-react-app-recs/tsconfig.json(1 hunks)examples/example-vite-react-phaser-recs/package.json(1 hunks)examples/example-vite-react-pwa-recs/package.json(1 hunks)examples/example-vite-react-threejs-recs/package.json(1 hunks)examples/example-vue-app-recs/package.json(1 hunks)migration/v1-4-v1-5.md(1 hunks)packages/react/src/useQuerySync.ts(2 hunks)packages/sdk/src/__tests__/convertClauseToEntityKeysClause.test.ts(0 hunks)packages/sdk/src/__tests__/models.gen.ts(1 hunks)packages/sdk/src/__tests__/pagination.test.ts(1 hunks)packages/sdk/src/internal/convertClauseToEntityKeysClause.ts(0 hunks)packages/sdk/src/internal/errors.ts(1 hunks)packages/sdk/src/internal/pagination.ts(1 hunks)packages/sdk/src/internal/parseEntities.ts(1 hunks)packages/sdk/src/internal/subscribeQueryModel.ts(1 hunks)packages/sdk/src/internal/toriiQueryBuilder.ts(2 hunks)packages/sdk/src/internal/types.ts(8 hunks)packages/sdk/src/node/index.ts(5 hunks)packages/sdk/src/web/experimental/index.ts(2 hunks)packages/sdk/src/web/index.ts(6 hunks)packages/sdk/src/web/react/hooks/entities.ts(1 hunks)packages/sdk/src/web/react/hooks/events.ts(2 hunks)packages/sdk/src/web/react/hooks/hooks.ts(5 hunks)packages/state/src/recs/index.ts(11 hunks)packages/torii-wasm/dojo.c(1 hunks)
💤 Files with no reviewable changes (2)
- packages/sdk/src/tests/convertClauseToEntityKeysClause.test.ts
- packages/sdk/src/internal/convertClauseToEntityKeysClause.ts
✅ Files skipped from review due to trivial changes (2)
- packages/sdk/src/internal/parseEntities.ts
- .changeset/stale-eggs-heal.md
🚧 Files skipped from review as they are similar to previous changes (21)
- examples/example-vite-react-threejs-recs/package.json
- examples/example-vue-app-recs/package.json
- packages/sdk/src/internal/errors.ts
- examples/example-vite-react-phaser-recs/package.json
- examples/example-vite-react-app-recs/package.json
- examples/example-vite-react-pwa-recs/package.json
- packages/sdk/src/web/react/hooks/entities.ts
- packages/torii-wasm/dojo.c
- examples/example-vite-react-app-recs/src/dojo/setup.ts
- examples/example-node-worker/main.ts
- examples/example-vite-kitchen-sink/src/components/theme-switch.tsx
- packages/sdk/src/web/react/hooks/events.ts
- examples/example-vite-kitchen-sink/src/components/caller-counter.tsx
- examples/example-vite-kitchen-sink/src/components/chat.tsx
- examples/example-vite-kitchen-sink/src/components/global-counter.tsx
- examples/example-vite-grpc-playground/src/App.tsx
- packages/sdk/src/web/experimental/index.ts
- packages/sdk/src/internal/subscribeQueryModel.ts
- packages/sdk/src/tests/pagination.test.ts
- packages/sdk/src/tests/models.gen.ts
- packages/state/src/recs/index.ts
🧰 Additional context used
🧬 Code Graph Analysis (5)
packages/sdk/src/internal/types.ts (2)
packages/sdk/src/internal/pagination.ts (1)
Pagination(11-103)packages/sdk/src/internal/toriiQueryBuilder.ts (1)
ToriiQueryBuilder(27-160)
packages/sdk/src/node/index.ts (4)
packages/sdk/src/internal/parseEntities.ts (1)
parseEntities(10-59)packages/sdk/src/internal/pagination.ts (1)
Pagination(11-103)packages/sdk/src/internal/subscribeQueryModel.ts (1)
subscribeQueryModelCallback(17-42)packages/sdk/src/internal/types.ts (4)
SubscribeParams(511-518)SubscribeResponse(254-257)GetParams(520-525)ToriiResponse(249-252)
examples/example-vite-react-app-recs/src/App.tsx (1)
packages/react/src/useQuerySync.ts (1)
useQuerySync(26-57)
packages/sdk/src/internal/toriiQueryBuilder.ts (2)
packages/sdk/src/internal/errors.ts (1)
UNDEFINED_CLAUSE(5-6)packages/sdk/src/internal/pagination.ts (1)
Pagination(11-103)
packages/sdk/src/web/react/hooks/hooks.ts (1)
packages/sdk/src/internal/types.ts (2)
SubscribeParams(511-518)SubscribeResponse(254-257)
🪛 Biome (1.9.4)
packages/sdk/src/internal/types.ts
[error] 5-5: 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)
examples/example-vite-react-app-recs/src/App.tsx
[error] 24-24: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
packages/react/src/useQuerySync.ts
[error] 31-31: This hook does not specify all of its dependencies: clause
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
packages/sdk/src/internal/pagination.ts
[error] 11-11: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 38-38: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
packages/sdk/src/internal/toriiQueryBuilder.ts
[error] 8-8: Some named imports are only used as types.
This import is only used as a type.
Importing the types with import type ensures that they are removed by the compilers and avoids loading unnecessary modules.
Safe fix: Add inline type keywords.
(lint/style/useImportType)
[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)
packages/sdk/src/web/react/hooks/hooks.ts
[error] 58-58: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: check
- GitHub Check: build
🔇 Additional comments (36)
packages/react/src/useQuerySync.ts (3)
27-30: Theclauseparameter is now required, removing entityKeyClause.This change aligns with the new pagination approach, replacing the previous model that used
EntityKeysClause.
2-2: Good use of import type.Using
import typeis the correct approach for these imports that are only used as type references.
33-33: Function call updated to match new signature.Removing the
entityKeyClauseparameter from thegetSyncEntitiescall aligns with the updated API.examples/example-vite-react-app-recs/tsconfig.json (1)
8-8: Upgraded moduleResolution to "bundler".Changing from "node" to "bundler" improves TypeScript's module resolution for modern bundler environments, which is appropriate for a Vite application.
examples/example-vite-react-app-recs/src/App.tsx (2)
22-26: Updated useQuerySync to use KeysClause.The code now correctly uses the new pagination API with
KeysClause([], [], "VariableLen").build()instead of an empty array, aligning with the breaking changes in the useQuerySync function.🧰 Tools
🪛 Biome (1.9.4)
[error] 24-24: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
3-3: Good use of import type.Using
import typeis the correct approach for imports that are only used as type references.packages/sdk/src/internal/toriiQueryBuilder.ts (2)
147-159: Good addition of pagination helper methods.The new
getClause()andgetPagination()methods provide a safer and more consistent way to access query components. ThegetClause()method correctly returns aResulttype to handle the potential undefined state.
150-155: Good error handling with Result type.The use of the
Resulttype fromneverthrowprovides clean error handling for the clause access, returning a descriptive error message when the clause is undefined.packages/sdk/src/web/react/hooks/hooks.ts (2)
1-1: LGTM: Simplified method signatures for pagination supportThe type imports and method signature changes correctly align with the new pagination model, removing the generic
Historicalparameter and simplifying the return types.Also applies to: 10-10, 21-22, 28-31
85-85: LGTM: Updated to use pagination's getItems() methodCorrectly updated to extract entities using the new pagination API by calling
getItems()on the response object.packages/sdk/src/internal/pagination.ts (1)
105-123: LGTM: Well-implemented direction reversal utilityThe
getReversedDirectionfunction is well implemented with proper type safety and defensive fallback.packages/sdk/src/web/index.ts (7)
68-68: LGTM: Simplified client instantiationDirect instantiation of the client object is cleaner than using a wrapper function.
29-31: LGTM: Added pagination and subscription handlersThe new imports properly support the pagination functionality and centralized subscription handling.
81-88: LGTM: Updated subscribeEntityQuery to use PaginationCorrectly updated to use the new pagination model, including wrapping results in a Pagination object and using the centralized subscription callback handler.
103-110: LGTM: Updated subscribeEventQuery to use PaginationSimilarly to subscribeEntityQuery, this method has been correctly updated to use the new pagination model.
137-139: LGTM: Updated query methods to return Pagination objectsBoth
getEntitiesandgetEventMessagesmethods now correctly wrap their results in Pagination objects, consistent with the migration guide.Also applies to: 154-156
284-284: LGTM: Simplified subscription update parametersThe update methods now accept a single Clause object instead of arrays of EntityKeysClause objects, which simplifies the API and aligns with the new query model.
Also applies to: 303-303
82-82:✅ Verification successful
Verify entity shape for parseEntities
Previous reviews flagged a potential mismatch between the shape of
entities.itemsand whatparseEntitiesexpects. Ensure this has been addressed in the implementation ofparseEntities.Also applies to: 138-138, 155-155
🏁 Script executed:
#!/bin/bash # Check the implementation of parseEntities to verify it can handle the shape of entities.items rg -A 10 "function parseEntities" packages/sdk/src/internal/parseEntities.tsLength of output: 462
🏁 Script executed:
#!/bin/bash # Show context around parseEntities call in web/index.ts rg -n -C5 "parseEntities" packages/sdk/src/web/index.tsLength of output: 2296
🏁 Script executed:
#!/bin/bash # 1. Show return in parseEntities implementation rg -n -C5 "return " packages/sdk/src/internal/parseEntities.ts # 2. Inspect type definitions for StandardizedQueryResult and SchemaType rg -n -C5 "StandardizedQueryResult" packages/sdk/src/internal rg -n -C5 "SchemaType" packages/sdk/src/internal # 3. Inspect imports and client usage in web/index.ts head -n 60 packages/sdk/src/web/index.tsLength of output: 41904
No entity shape mismatch in parseEntities
TheparseEntitiesfunction expects atorii.Entity[]and correctly accessesentity.hashed_keysandentity.models, matching the shape ofentities.items. Calls at lines 82, 138, and 155 inpackages/sdk/src/web/index.tsalign with this signature, and no changes are needed.migration/v1-4-v1-5.md (1)
1-207: LGTM: Comprehensive migration guideThe migration guide clearly explains the changes from version 1.4 to 1.5, including:
- The shift from offset-based to cursor-based pagination
- Introduction of the
Paginationclass and its methods- Changes to response handling with code examples
- Breaking changes and migration steps
- Best practices for implementing pagination
This will be very helpful for users upgrading to the new version.
packages/sdk/src/node/index.ts (7)
27-29: Added new imports for pagination support.The new imports introduce necessary components for the pagination feature:
subscribeQueryModelCallbackcentralizes subscription handlingPaginationimplements the cursor-based pagination featureparseEntitiesis now imported explicitly rather than being called directlyThese imports align with the PR objective to support pagination in Torii.
65-69: Subscription response now returns a Pagination object.The subscription response structure has been updated to use the new
Paginationclass, which:
- Extracts pagination parameters from the query using
fromQuery- Wraps the parsed entities using
withItems- Passes the original query clause directly instead of converting to entity keys
This change improves consistency with the cursor-based pagination model and simplifies the codebase by removing unnecessary conversion logic.
78-92: Event query subscription updated to use pagination.Similar to the entity subscription, the event subscription now:
- Uses the pagination model
- Simplifies the type signature by removing the Historical generic
- Uses the centralized
subscribeQueryModelCallbackhandler- Directly passes the clause without conversion
This change ensures consistent behavior between entity and event subscriptions.
116-120: Entity queries now return paginated results.The
getEntitiesmethod has been updated to:
- Access the response through the
itemsproperty- Return results wrapped in a
Paginationobject- Use the pagination parameters from the original query
Consumers of this API will need to adapt to access entities via
.getItems()rather than directly working with the returned array.
133-137: Event message queries now return paginated results.Similar to the entity queries, the event message queries now return results wrapped in a
Paginationobject, which provides consistent handling across all query types.
269-270: Entity subscription update now uses a single Clause.The
updateEntitySubscriptionmethod now accepts a singletorii.Clauseinstead of an array oftorii.EntityKeysClause. This simplifies the API and aligns with the unified clause model used throughout the codebase.
288-289: Event subscription update now uses a single Clause.Similar to the entity subscription update, the event subscription update now accepts a single
torii.Clauseinstead of an array of clauses, providing consistent handling across subscription types.packages/sdk/src/internal/types.ts (10)
249-252: ToriiResponse type simplified to always use Pagination.The
ToriiResponsetype has been updated to consistently return aPaginationobject wrappingStandardizedQueryResult<T>, removing the conditional handling based on the Historical parameter. This creates a unified return type for all queries, simplifying client code but requiring updates to existing code that expects direct array access.
254-257: SubscribeResponse type simplified to match ToriiResponse changes.The
SubscribeResponsetype now aligns with the updatedToriiResponsetype, removing the Historical generic parameter. This ensures consistency across the API.
311-313: SDK interface simplified for entity query subscriptions.The
subscribeEntityQuerymethod signature has been simplified by removing the Historical generic parameter, aligning with the broader pagination changes. Client code will need to be updated to handle the new return type.
323-325: SDK interface simplified for event query subscriptions.Similar to entity subscriptions, the event query subscription method has been simplified to remove the Historical generic parameter, providing consistent handling across subscription types.
345-345: SDK getEntities return type updated to use Pagination.The
getEntitiesmethod now consistently returns aPaginationobject wrapping the query results, removing the conditional handling based on the Historical parameter.
354-354: SDK getEventMessages return type updated to use Pagination.Similar to
getEntities, thegetEventMessagesmethod now returns aPaginationobject, providing a consistent interface across the SDK.
432-432: updateEntitySubscription now uses a single Clause.The
updateEntitySubscriptionmethod parameter has been changed fromtorii.EntityKeysClause[]totorii.Clause, aligning with the unified clause model used throughout the SDK.
449-450: updateEventMessageSubscription now uses a single Clause.Similar to
updateEntitySubscription, theupdateEventMessageSubscriptionmethod now accepts atorii.Clauseinstead of an array of clauses.
511-518: SubscribeParams interface simplified.The
SubscribeParamsinterface has been simplified by:
- Removing the Historical generic parameter
- Updating the callback to directly handle
StandardizedQueryResult<T>- Marking the
historicalparameter as deprecated in favor of usingquery.historicalThese changes align with the shift to cursor-based pagination and simplify the type system.
520-525: GetParams interface simplified.Similar to
SubscribeParams, theGetParamsinterface has been simplified by removing the Historical generic parameter and deprecating thehistoricalfield, encouraging the use ofquery.historicalinstead.
| useQuerySync(toriiClient, contractComponents as any, []); | ||
| useQuerySync( | ||
| toriiClient, | ||
| contractComponents as any, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unnecessary 'any' type assertion.
The code uses an unsafe any type that bypasses type checking. Consider using a more specific type or properly typing the contractComponents.
- contractComponents as any,
+ contractComponents,If type compatibility is an issue, consider adding proper type declarations instead of using any.
📝 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.
| contractComponents as any, | |
| contractComponents, |
🧰 Tools
🪛 Biome (1.9.4)
[error] 24-24: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
| * @param query - The query builder to extract pagination parameters from | ||
| * @returns A new Pagination instance configured with the query's pagination settings | ||
| */ | ||
| static fromQuery<T extends SchemaType, Inner extends any[]>( |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using any in fromQuery generic constraint
Similar to the class definition, the static method also uses any in its generic constraint.
-static fromQuery<T extends SchemaType, Inner extends any[]>(
+static fromQuery<T extends SchemaType, Inner extends unknown[]>(📝 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.
| static fromQuery<T extends SchemaType, Inner extends any[]>( | |
| static fromQuery<T extends SchemaType, Inner extends unknown[]>( |
🧰 Tools
🪛 Biome (1.9.4)
[error] 38-38: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
MartianGreed
force-pushed
the
feat/support-torii-pagination
branch
from
baadeed to
f86fc77
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
packages/react/src/useQuerySync.ts (1)
31-37:⚠️ Potential issueAdd missing dependency to useCallback.
The
clauseparameter is used in thesetupSynccallback but isn't included in the dependency array. This could lead to stale closures.- }, [toriiClient, components]); + }, [toriiClient, components, clause]);Adding
clauseto the dependencies ensures the sync is properly updated when the clause changes.🧰 Tools
🪛 Biome (1.9.4)
[error] 35-35: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.(lint/complexity/noUselessCatch)
[error] 31-31: This hook does not specify all of its dependencies: clause
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
♻️ Duplicate comments (5)
packages/sdk/src/web/react/hooks/hooks.ts (1)
54-62: Remove unsafe non-null assertion.The non-null assertion on
fetchingRef.current!should be replaced with a proper null check:if (subscriptionRef.current) { await config.updateSubscriptionMethod( subscriptionRef.current, - config.queryToHashedKeysMethod( - fetchingRef.current!, - config.historical - ), + fetchingRef.current ? config.queryToHashedKeysMethod( + fetchingRef.current, + config.historical + ) : undefined, config.historical ); return null; }This makes the code safer by handling the potential null case explicitly.
🧰 Tools
🪛 Biome (1.9.4)
[error] 58-58: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
packages/sdk/src/internal/pagination.ts (4)
11-11: Avoidanyin the generic constraintUsing
anyin the generic constraint defeats the purpose of strong typing and triggers thenoExplicitAnylinter error.-export class Pagination<T extends SchemaType, Inner extends any[]> { +export class Pagination<T extends SchemaType, Inner extends unknown[]> {🧰 Tools
🪛 Biome (1.9.4)
[error] 11-11: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
38-38: Avoidanyin the static method's generic constraintSimilar to the class definition, the static method also uses
anyin its generic constraint.- static fromQuery<T extends SchemaType, Inner extends any[]>( + static fromQuery<T extends SchemaType, Inner extends unknown[]>(🧰 Tools
🪛 Biome (1.9.4)
[error] 38-38: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
75-84: 🛠️ Refactor suggestionAvoid mutating the caller's query builder
The
getNextQuerymethod mutates the caller's query builder instance, which could lead to unexpected behavior if the caller reuses the query builder for other operations.getNextQuery(query: ToriiQueryBuilder<T>): ToriiQueryBuilder<T> { - const q = query.withLimit(this.limit); + // Clone or create a new instance to avoid mutating the original + const q = new ToriiQueryBuilder<T>(query.build()) + .withLimit(this.limit); if (this.cursor) { q.withCursor(this.cursor); } if (q.getPagination().direction !== this.direction) { q.withDirection(this.direction as PaginationDirection); } return q; }Consider implementing a
clone()method in theToriiQueryBuilderclass to make this pattern more explicit and consistent.
21-30: 🛠️ Refactor suggestionUse
PaginationDirectionenum type for better type safetyThe
directionparameter should be typed asPaginationDirectionrather than string, and given a default value directly in the constructor signature to eliminate the conditional logic and later type casting.constructor( public limit: number, public cursor?: string, - public direction?: string + public direction: PaginationDirection = "Forward" ) { this.items = [] as unknown as Inner; - if (!direction) { - this.direction = "Forward"; - } }
🧹 Nitpick comments (9)
packages/sdk/src/internal/toriiQueryBuilder.ts (1)
8-10: Consider using type-only imports for better code optimizations.The imports for
Result,ok,errandPaginationshould use thetypekeyword since Biome indicates they're only used as types in this file.-import { Result, ok, err } from "neverthrow"; -import { UNDEFINED_CLAUSE } from "./errors.ts"; -import { Pagination } from "@dojoengine/torii-wasm"; +import { ok, err } from "neverthrow"; +import type { Result } from "neverthrow"; +import { UNDEFINED_CLAUSE } from "./errors.ts"; +import type { Pagination } from "@dojoengine/torii-wasm";This ensures the imports are properly optimized by TypeScript's compiler.
🧰 Tools
🪛 Biome (1.9.4)
[error] 8-8: Some named imports are only used as types.
This import is only used as a type.
Importing the types with import type ensures that they are removed by the compilers and avoids loading unnecessary modules.
Safe fix: Add inline type keywords.(lint/style/useImportType)
[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)
packages/sdk/src/internal/pagination.ts (2)
81-81: Remove type casting by fixing the constructor parameter typeIf you implement the suggested constructor change to use
PaginationDirectiondirectly, this type casting becomes unnecessary.- q.withDirection(this.direction as PaginationDirection); + q.withDirection(this.direction);
116-123: The guard clause can be eliminated with proper TypeScript configurationThe code suggests that
directionshould never be anything other than "Forward" or "Backward". With proper TypeScript configuration (like strict literal types), this fallback case can be eliminated.Consider using a more restricted type:
-function getReversedDirection( - direction: PaginationDirection -): PaginationDirection { +function getReversedDirection( + direction: "Forward" | "Backward" +): "Forward" | "Backward" { if (direction === "Forward") return "Backward"; - if (direction === "Backward") return "Forward"; - // we should not go down here. - return "Forward"; + return "Forward"; // Only remaining case is "Backward" }migration/v1-4-v1-5.md (3)
85-108: Consider adding examples with real schema typesThe documentation would be more helpful with concrete examples showing pagination with actual schema types from the codebase, rather than just generic types.
// Example with real schema type import { SchemaType } from "./your-schema"; const response = await sdk.getEntities<SchemaType>(query); const players = response.getItems(); // Players are now strongly typed as StandardizedQueryResult<SchemaType>
110-123: Mention potential migration challengesConsider adding a section about common challenges developers might face during migration, such as handling existing code that expects raw arrays instead of
Paginationobjects.### Common Migration Challenges 1. **Existing Array Operations** If your code previously performed array operations directly on the response: ```typescript // Old code const entities = await sdk.getEntities(query); const firstEntity = entities[0]; const count = entities.length; // New code const pagination = await sdk.getEntities(query); const entities = pagination.getItems(); const firstEntity = entities[0]; const count = entities.length;
Passing Results to Components
Components expecting raw entity arrays will need to be updated:
// Old code <EntityList entities={entities} /> // New code <EntityList entities={pagination.getItems()} />--- `185-185`: **Consider expanding on implementing error boundaries** The guide mentions implementing error boundaries but doesn't provide examples. This would be helpful for developers. ```markdown 3. **Review Error Handling** - Update error handling to account for new error types - Implement proper error boundaries in React components ```tsx // Example error boundary for pagination components class PaginationErrorBoundary extends React.Component { state = { hasError: false, error: null }; static getDerivedStateFromError(error) { return { hasError: true, error }; } render() { if (this.state.hasError) { return <div>Error loading paginated data: {this.state.error.message}</div>; } return this.props.children; } } // Usage <PaginationErrorBoundary> <EntityPaginatedList query={query} /> </PaginationErrorBoundary></blockquote></details> <details> <summary>packages/sdk/src/internal/types.ts (3)</summary><blockquote> `5-5`: **Use `import type` for type-only imports** Since the `Pagination` import is only used for type definitions, use `import type` to ensure it's removed at compile time. ```diff -import { Pagination } from "./pagination.ts"; +import type { Pagination } from "./pagination.ts";🧰 Tools
🪛 Biome (1.9.4)
[error] 5-5: 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)
312-313: Update method signature documentationThe JSDoc comment for
subscribeEntityQuerystill references the old type parameter structure which no longer exists./** * Subscribes to entity updates based on the provided query and invokes the callback with the updated data. * * @template T - The schema type. - * @param {SubscribeParams<T, false>} params - Parameters object - * @returns {Promise<SubscribeResponse<T, false>>} - A promise that resolves to a Torii subscription. + * @param {SubscribeParams<T>} params - Parameters object + * @returns {Promise<SubscribeResponse<T>>} - A promise that resolves to a Torii subscription. */
345-345: Update method signature documentation forgetEntitiesSimilar to the subscription method, this method's JSDoc comment should be updated to reflect the simplified type structure.
/** * Fetches entities from the Torii client based on the provided query. * * @template T - The schema type. - * @param {GetParams<T, false>} params - Parameters object - * @returns {Promise<ToriiResponse<T, false>>} - A promise that resolves to the standardized query result. + * @param {GetParams<T>} params - Parameters object + * @returns {Promise<ToriiResponse<T>>} - A promise that resolves to the standardized query result. */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (5)
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/**examples/example-vue-app-recs/src/dojo/generated/setup.tsis excluded by!**/generated/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (39)
.changeset/stale-eggs-heal.md(1 hunks)examples/example-node-worker/main.ts(4 hunks)examples/example-vite-grpc-playground/src/App.tsx(3 hunks)examples/example-vite-kitchen-sink/src/components/caller-counter.tsx(3 hunks)examples/example-vite-kitchen-sink/src/components/chat.tsx(2 hunks)examples/example-vite-kitchen-sink/src/components/global-counter.tsx(3 hunks)examples/example-vite-kitchen-sink/src/components/theme-switch.tsx(3 hunks)examples/example-vite-react-app-recs/package.json(1 hunks)examples/example-vite-react-app-recs/src/App.tsx(2 hunks)examples/example-vite-react-app-recs/src/dojo/setup.ts(3 hunks)examples/example-vite-react-app-recs/tsconfig.json(1 hunks)examples/example-vite-react-phaser-recs/package.json(1 hunks)examples/example-vite-react-phaser-recs/tsconfig.json(1 hunks)examples/example-vite-react-pwa-recs/package.json(1 hunks)examples/example-vite-react-pwa-recs/tsconfig.json(1 hunks)examples/example-vite-react-threejs-recs/package.json(1 hunks)examples/example-vite-react-threejs-recs/tsconfig.json(1 hunks)examples/example-vue-app-recs/package.json(1 hunks)examples/example-vue-app-recs/tsconfig.json(1 hunks)migration/v1-4-v1-5.md(1 hunks)packages/react/src/useQuerySync.ts(2 hunks)packages/sdk/src/__tests__/convertClauseToEntityKeysClause.test.ts(0 hunks)packages/sdk/src/__tests__/models.gen.ts(1 hunks)packages/sdk/src/__tests__/pagination.test.ts(1 hunks)packages/sdk/src/internal/convertClauseToEntityKeysClause.ts(0 hunks)packages/sdk/src/internal/errors.ts(1 hunks)packages/sdk/src/internal/pagination.ts(1 hunks)packages/sdk/src/internal/parseEntities.ts(1 hunks)packages/sdk/src/internal/subscribeQueryModel.ts(1 hunks)packages/sdk/src/internal/toriiQueryBuilder.ts(2 hunks)packages/sdk/src/internal/types.ts(8 hunks)packages/sdk/src/node/index.ts(5 hunks)packages/sdk/src/web/experimental/index.ts(2 hunks)packages/sdk/src/web/index.ts(6 hunks)packages/sdk/src/web/react/hooks/entities.ts(1 hunks)packages/sdk/src/web/react/hooks/events.ts(2 hunks)packages/sdk/src/web/react/hooks/hooks.ts(5 hunks)packages/state/src/recs/index.ts(11 hunks)packages/torii-wasm/dojo.c(1 hunks)
💤 Files with no reviewable changes (2)
- packages/sdk/src/tests/convertClauseToEntityKeysClause.test.ts
- packages/sdk/src/internal/convertClauseToEntityKeysClause.ts
✅ Files skipped from review due to trivial changes (3)
- examples/example-vite-react-pwa-recs/tsconfig.json
- examples/example-vue-app-recs/tsconfig.json
- packages/sdk/src/internal/parseEntities.ts
🚧 Files skipped from review as they are similar to previous changes (25)
- packages/sdk/src/internal/errors.ts
- examples/example-vite-react-app-recs/tsconfig.json
- examples/example-vite-react-app-recs/package.json
- examples/example-vite-react-pwa-recs/package.json
- examples/example-vite-react-phaser-recs/package.json
- examples/example-vite-react-app-recs/src/dojo/setup.ts
- packages/sdk/src/web/experimental/index.ts
- packages/torii-wasm/dojo.c
- examples/example-vue-app-recs/package.json
- packages/sdk/src/web/react/hooks/entities.ts
- examples/example-vite-kitchen-sink/src/components/chat.tsx
- examples/example-vite-kitchen-sink/src/components/global-counter.tsx
- packages/sdk/src/internal/subscribeQueryModel.ts
- examples/example-vite-kitchen-sink/src/components/caller-counter.tsx
- examples/example-vite-kitchen-sink/src/components/theme-switch.tsx
- examples/example-node-worker/main.ts
- .changeset/stale-eggs-heal.md
- packages/sdk/src/tests/pagination.test.ts
- examples/example-vite-grpc-playground/src/App.tsx
- packages/sdk/src/tests/models.gen.ts
- packages/sdk/src/web/react/hooks/events.ts
- examples/example-vite-react-threejs-recs/package.json
- packages/sdk/src/web/index.ts
- packages/state/src/recs/index.ts
- packages/sdk/src/node/index.ts
🧰 Additional context used
🧬 Code Graph Analysis (5)
packages/sdk/src/internal/types.ts (4)
packages/sdk/src/__tests__/models.gen.ts (1)
SchemaType(85-97)examples/example-node-worker/src/typescript/models.gen.ts (1)
SchemaType(80-94)packages/sdk/src/internal/pagination.ts (1)
Pagination(11-103)packages/sdk/src/internal/toriiQueryBuilder.ts (1)
ToriiQueryBuilder(27-160)
examples/example-vite-react-app-recs/src/App.tsx (1)
packages/react/src/useQuerySync.ts (1)
useQuerySync(26-57)
packages/react/src/useQuerySync.ts (1)
packages/state/src/recs/index.ts (1)
getSyncEntities(49-69)
packages/sdk/src/internal/pagination.ts (2)
packages/sdk/src/internal/types.ts (2)
SchemaType(51-71)ToriiQueryBuilder(526-526)packages/sdk/src/internal/toriiQueryBuilder.ts (1)
ToriiQueryBuilder(27-160)
packages/sdk/src/internal/toriiQueryBuilder.ts (2)
packages/sdk/src/internal/errors.ts (1)
UNDEFINED_CLAUSE(5-6)packages/sdk/src/internal/pagination.ts (1)
Pagination(11-103)
🪛 Biome (1.9.4)
packages/sdk/src/internal/types.ts
[error] 5-5: 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)
examples/example-vite-react-app-recs/src/App.tsx
[error] 24-24: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
packages/react/src/useQuerySync.ts
[error] 31-31: This hook does not specify all of its dependencies: clause
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
packages/sdk/src/internal/pagination.ts
[error] 11-11: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 38-38: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
packages/sdk/src/internal/toriiQueryBuilder.ts
[error] 8-8: Some named imports are only used as types.
This import is only used as a type.
Importing the types with import type ensures that they are removed by the compilers and avoids loading unnecessary modules.
Safe fix: Add inline type keywords.
(lint/style/useImportType)
[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)
packages/sdk/src/web/react/hooks/hooks.ts
[error] 58-58: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: check
🔇 Additional comments (16)
examples/example-vite-react-threejs-recs/tsconfig.json (1)
8-8:✅ Verification successful
Ensure TypeScript version supports
bundlerresolution
The new"moduleResolution": "bundler"aligns this example with others, but it requires TypeScript v5.0+ to work correctly. Please verify that the example’spackage.json(or monorepo root) specifies a compatible TS version.Run this script to confirm the TS version used in the example project:
🏁 Script executed:
#!/bin/bash # Verify TypeScript version in the example project rg '"typescript"' -A1 examples/example-vite-react-threejs-recs/package.jsonLength of output: 135
Compatibility OK: TypeScript v5.6.2 supports "bundler" resolution
The example’s package.json specifies"typescript": "^5.6.2", which meets the v5.0+ requirement for the"moduleResolution": "bundler"option. No changes needed.examples/example-vite-react-phaser-recs/tsconfig.json (1)
9-9: Align module resolution with bundler expectations
UpdatingmoduleResolutionto"bundler"is consistent with Vite projects and the other example configs. This ensures imports resolve correctly under Rollup/Webpack-style bundlers.examples/example-vite-react-app-recs/src/App.tsx (2)
3-5: Good use of type-only imports.The change to use a type-only import for
Entityand add theKeysClauseimport prepares the code for the pagination support changes.
22-26: Updated query construction for pagination support.The code now properly constructs a variable-length key clause instead of using an empty array, which aligns with the new pagination model in the SDK.
However, the
anytype assertion on line 24 should be removed:- contractComponents as any, + contractComponents,This removes a type safety risk that was flagged by static analysis.
🧰 Tools
🪛 Biome (1.9.4)
[error] 24-24: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
packages/sdk/src/internal/toriiQueryBuilder.ts (1)
147-159: Good implementation of error handling for query components.The addition of
getClause()andgetPagination()methods provides a clean API for accessing internal query components, with proper error handling for undefined clauses using the Result type. This is a good practice that makes error cases explicit.packages/react/src/useQuerySync.ts (2)
2-8: Proper migration to type-only imports.Good update to use type-only imports for types that don't need to be included in the runtime.
26-30: Interface simplification for pagination support.The hook interface has been simplified by removing the
entityKeyClauseparameter and makingclausemandatory, which aligns with the pagination model changes.packages/sdk/src/web/react/hooks/hooks.ts (2)
20-31: Good simplification of method signatures.Removing the generic
Historicalparameter from method signatures and updating the return type ofqueryToHashedKeysMethodto a synchronousClausesimplifies the API.🧰 Tools
🪛 Biome (1.9.4)
[error] 25-25: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
85-85: Updated data processing to use pagination API.Good update to use
getItems()to extract entities from the paginated response, which aligns with the new pagination model.migration/v1-4-v1-5.md (1)
1-207: The migration guide clearly explains the pagination changesThis migration guide effectively communicates the transition from offset-based to cursor-based pagination and the introduction of the
Paginationclass. The examples and explanations are clear and comprehensive.packages/sdk/src/internal/types.ts (6)
249-252: Good update to simplify theToriiResponsetype withPaginationThe new
ToriiResponsetype properly utilizes thePaginationclass, providing a consistent interface for pagination across the SDK.
254-257: SimplifiedSubscribeResponsetype with clear generic parametersThe updated
SubscribeResponsetype removes unnecessary complexity and aligns with the new pagination model.
432-432: Update parameter type from array to single clauseThe
updateEntitySubscriptionmethod now accepts a singletorii.Clauseinstead of an array, which aligns with the API changes described in the migration guide.
449-449: Similar simplification forupdateEventMessageSubscriptionThe
updateEventMessageSubscriptionmethod also now accepts a singletorii.Clause, maintaining consistency with the other API changes.
511-518: Good simplification ofSubscribeParamsinterfaceThe updated interface removes the generic
Historicalparameter and uses a simpler boolean flag, reducing complexity while maintaining functionality.
520-525: Similar simplification forGetParamsinterfaceThe
GetParamsinterface has been updated consistently withSubscribeParams, maintaining a cohesive API design.
| getPreviousQuery(query: ToriiQueryBuilder<T>): ToriiQueryBuilder<T> { | ||
| const q = query.withLimit(this.limit); | ||
| if (this.cursor) { | ||
| q.withCursor(this.cursor); | ||
| } | ||
| if (q.getPagination().direction === this.direction) { | ||
| q.withDirection(getReversedDirection(this.direction)); | ||
| } | ||
|
|
||
| return q; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Apply the same cloning fix to getPreviousQuery and handle potential undefined direction
This method has the same issue as getNextQuery and additionally might have an undefined direction.
getPreviousQuery(query: ToriiQueryBuilder<T>): ToriiQueryBuilder<T> {
- const q = query.withLimit(this.limit);
+ // Clone or create a new instance to avoid mutating the original
+ const q = new ToriiQueryBuilder<T>(query.build())
+ .withLimit(this.limit);
if (this.cursor) {
q.withCursor(this.cursor);
}
+ // Make sure direction is defined before using it
+ const direction = this.direction || "Forward";
- if (q.getPagination().direction === this.direction) {
- q.withDirection(getReversedDirection(this.direction));
+ if (q.getPagination().direction === direction) {
+ q.withDirection(getReversedDirection(direction));
}
return q;
}📝 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.
| getPreviousQuery(query: ToriiQueryBuilder<T>): ToriiQueryBuilder<T> { | |
| const q = query.withLimit(this.limit); | |
| if (this.cursor) { | |
| q.withCursor(this.cursor); | |
| } | |
| if (q.getPagination().direction === this.direction) { | |
| q.withDirection(getReversedDirection(this.direction)); | |
| } | |
| return q; | |
| getPreviousQuery(query: ToriiQueryBuilder<T>): ToriiQueryBuilder<T> { | |
| // Clone or create a new instance to avoid mutating the original | |
| const q = new ToriiQueryBuilder<T>(query.build()) | |
| .withLimit(this.limit); | |
| if (this.cursor) { | |
| q.withCursor(this.cursor); | |
| } | |
| // Make sure direction is defined before using it | |
| const direction = this.direction || "Forward"; | |
| if (q.getPagination().direction === direction) { | |
| q.withDirection(getReversedDirection(direction)); | |
| } | |
| return q; | |
| } |
MartianGreed
force-pushed
the
feat/support-torii-pagination
branch
from
f86fc77 to
97aaa22
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
packages/react/src/useQuerySync.ts (1)
31-37:⚠️ Potential issueMissing dependency in useCallback hook.
The
clauseparameter is used in the callback function but not listed in the dependency array. This could lead to stale closures ifclausechanges.- }, [toriiClient, components]); + }, [toriiClient, components, clause]);🧰 Tools
🪛 Biome (1.9.4)
[error] 35-35: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.(lint/complexity/noUselessCatch)
[error] 31-31: This hook does not specify all of its dependencies: clause
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
♻️ Duplicate comments (7)
examples/example-vite-react-app-recs/src/App.tsx (1)
22-26: Updated query syntax to use new clause-based model.The
useQuerySynccall has been properly updated to use the newKeysClauseconstructor andbuild()method, replacing the previous entity keys array approach. This change is required to align with the new unifiedClausetype system and pagination support.However, you should remove the unsafe
anytype assertion which was flagged in a previous review:- contractComponents as any, + contractComponents,🧰 Tools
🪛 Biome (1.9.4)
[error] 24-24: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
packages/sdk/src/web/react/hooks/hooks.ts (1)
54-62:⚠️ Potential issueRemove unsafe non-null assertion in subscription update.
The code uses an unsafe non-null assertion that could cause runtime errors if
fetchingRef.currentis null.- await config.updateSubscriptionMethod( - subscriptionRef.current, - config.queryToHashedKeysMethod( - fetchingRef.current!, - config.historical - ), - config.historical - ); + if (fetchingRef.current) { + await config.updateSubscriptionMethod( + subscriptionRef.current, + config.queryToHashedKeysMethod( + fetchingRef.current, + config.historical + ), + config.historical + ); + }🧰 Tools
🪛 Biome (1.9.4)
[error] 58-58: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
packages/sdk/src/internal/pagination.ts (5)
11-11: Replaceany[]withunknown[]for type safetyUsing
anyin generic constraints defeats type-safety. This triggers the linter error and could allow unsafe operations.-export class Pagination<T extends SchemaType, Inner extends any[]> { +export class Pagination<T extends SchemaType, Inner extends unknown[]> {🧰 Tools
🪛 Biome (1.9.4)
[error] 11-11: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
21-30: UsePaginationDirectionenum type for the direction parameterThe direction parameter should use a strongly typed enum instead of string to provide type-safety and eliminate the need for casting later (line 81).
constructor( public limit: number, public cursor?: string, - public direction?: string + public direction: PaginationDirection = "Forward" ) { this.items = [] as unknown as Inner; - if (!direction) { - this.direction = "Forward"; - } }
38-38: Replaceany[]withunknown[]in static method generic constraintSimilar to the class definition, the static method should use a more type-safe constraint.
-static fromQuery<T extends SchemaType, Inner extends any[]>( +static fromQuery<T extends SchemaType, Inner extends unknown[]>(🧰 Tools
🪛 Biome (1.9.4)
[error] 38-38: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
75-84: Clone the query builder to avoid mutating the caller's instanceThe current implementation mutates the original query builder passed by the caller, which can cause unexpected side effects if the query is reused later.
getNextQuery(query: ToriiQueryBuilder<T>): ToriiQueryBuilder<T> { - const q = query.withLimit(this.limit); + // Clone the query to avoid mutation + const q = new ToriiQueryBuilder<T>(query.build()) + .withLimit(this.limit); if (this.cursor) { q.withCursor(this.cursor); } if (q.getPagination().direction !== this.direction) { q.withDirection(this.direction as PaginationDirection); } return q; }
92-102: Apply the same cloning pattern to thegetPreviousQuerymethodThe
getPreviousQuerymethod has the same issue of mutating the caller's query builder instance.getPreviousQuery(query: ToriiQueryBuilder<T>): ToriiQueryBuilder<T> { - const q = query.withLimit(this.limit); + // Clone the query to avoid mutation + const q = new ToriiQueryBuilder<T>(query.build()) + .withLimit(this.limit); if (this.cursor) { q.withCursor(this.cursor); } if (q.getPagination().direction === this.direction) { q.withDirection(getReversedDirection(this.direction)); } return q; }
🧹 Nitpick comments (3)
packages/sdk/src/internal/toriiQueryBuilder.ts (1)
8-10: Use proper type imports for better tree-shaking.While adding necessary imports for the new Result type and pagination functionality, you should use
import typefor type-only imports.-import { Result, ok, err } from "neverthrow"; -import { UNDEFINED_CLAUSE } from "./errors.ts"; -import { Pagination } from "@dojoengine/torii-wasm"; +import { ok, err } from "neverthrow"; +import type { Result } from "neverthrow"; +import { UNDEFINED_CLAUSE } from "./errors.ts"; +import type { Pagination } from "@dojoengine/torii-wasm";🧰 Tools
🪛 Biome (1.9.4)
[error] 8-8: Some named imports are only used as types.
This import is only used as a type.
Importing the types with import type ensures that they are removed by the compilers and avoids loading unnecessary modules.
Safe fix: Add inline type keywords.(lint/style/useImportType)
[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)
packages/sdk/src/internal/types.ts (2)
5-5: Useimport typefor Pagination importSince
Paginationis only used in type positions, it should be imported withimport typeto ensure it's removed during compilation.-import { Pagination } from "./pagination.ts"; +import type { Pagination } from "./pagination.ts";🧰 Tools
🪛 Biome (1.9.4)
[error] 5-5: 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)
511-518: Consider removing deprecatedhistoricalparameterThe
historicalparameter is marked as deprecated, suggesting it should be removed in a future version. Consider creating a tracking issue to ensure this is done in the next major release.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (5)
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/**examples/example-vue-app-recs/src/dojo/generated/setup.tsis excluded by!**/generated/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (40)
.changeset/stale-eggs-heal.md(1 hunks)examples/example-node-worker/main.ts(4 hunks)examples/example-vite-grpc-playground/src/App.tsx(3 hunks)examples/example-vite-kitchen-sink/src/components/caller-counter.tsx(3 hunks)examples/example-vite-kitchen-sink/src/components/chat.tsx(2 hunks)examples/example-vite-kitchen-sink/src/components/global-counter.tsx(3 hunks)examples/example-vite-kitchen-sink/src/components/theme-switch.tsx(3 hunks)examples/example-vite-react-app-recs/package.json(1 hunks)examples/example-vite-react-app-recs/src/App.tsx(2 hunks)examples/example-vite-react-app-recs/src/dojo/setup.ts(3 hunks)examples/example-vite-react-app-recs/tsconfig.json(1 hunks)examples/example-vite-react-phaser-recs/package.json(1 hunks)examples/example-vite-react-phaser-recs/src/phaser/config/configurePhaser.ts(1 hunks)examples/example-vite-react-phaser-recs/tsconfig.json(1 hunks)examples/example-vite-react-pwa-recs/package.json(1 hunks)examples/example-vite-react-pwa-recs/tsconfig.json(1 hunks)examples/example-vite-react-threejs-recs/package.json(1 hunks)examples/example-vite-react-threejs-recs/tsconfig.json(1 hunks)examples/example-vue-app-recs/package.json(1 hunks)examples/example-vue-app-recs/tsconfig.json(1 hunks)migration/v1-4-v1-5.md(1 hunks)packages/react/src/useQuerySync.ts(2 hunks)packages/sdk/src/__tests__/convertClauseToEntityKeysClause.test.ts(0 hunks)packages/sdk/src/__tests__/models.gen.ts(1 hunks)packages/sdk/src/__tests__/pagination.test.ts(1 hunks)packages/sdk/src/internal/convertClauseToEntityKeysClause.ts(0 hunks)packages/sdk/src/internal/errors.ts(1 hunks)packages/sdk/src/internal/pagination.ts(1 hunks)packages/sdk/src/internal/parseEntities.ts(1 hunks)packages/sdk/src/internal/subscribeQueryModel.ts(1 hunks)packages/sdk/src/internal/toriiQueryBuilder.ts(2 hunks)packages/sdk/src/internal/types.ts(8 hunks)packages/sdk/src/node/index.ts(5 hunks)packages/sdk/src/web/experimental/index.ts(2 hunks)packages/sdk/src/web/index.ts(6 hunks)packages/sdk/src/web/react/hooks/entities.ts(1 hunks)packages/sdk/src/web/react/hooks/events.ts(2 hunks)packages/sdk/src/web/react/hooks/hooks.ts(5 hunks)packages/state/src/recs/index.ts(11 hunks)packages/torii-wasm/dojo.c(1 hunks)
💤 Files with no reviewable changes (2)
- packages/sdk/src/tests/convertClauseToEntityKeysClause.test.ts
- packages/sdk/src/internal/convertClauseToEntityKeysClause.ts
✅ Files skipped from review due to trivial changes (3)
- packages/sdk/src/internal/parseEntities.ts
- packages/sdk/src/web/react/hooks/events.ts
- examples/example-vite-react-phaser-recs/src/phaser/config/configurePhaser.ts
🚧 Files skipped from review as they are similar to previous changes (27)
- packages/sdk/src/web/react/hooks/entities.ts
- examples/example-vite-react-pwa-recs/package.json
- examples/example-vite-react-phaser-recs/package.json
- examples/example-vite-react-app-recs/package.json
- examples/example-vite-react-app-recs/src/dojo/setup.ts
- examples/example-vite-react-threejs-recs/package.json
- examples/example-vite-react-phaser-recs/tsconfig.json
- examples/example-vite-react-app-recs/tsconfig.json
- packages/sdk/src/internal/errors.ts
- examples/example-vite-react-threejs-recs/tsconfig.json
- examples/example-vite-react-pwa-recs/tsconfig.json
- examples/example-vue-app-recs/package.json
- examples/example-vue-app-recs/tsconfig.json
- packages/torii-wasm/dojo.c
- examples/example-vite-kitchen-sink/src/components/theme-switch.tsx
- examples/example-vite-grpc-playground/src/App.tsx
- packages/sdk/src/tests/pagination.test.ts
- packages/sdk/src/web/experimental/index.ts
- examples/example-vite-kitchen-sink/src/components/chat.tsx
- .changeset/stale-eggs-heal.md
- examples/example-vite-kitchen-sink/src/components/global-counter.tsx
- packages/sdk/src/internal/subscribeQueryModel.ts
- packages/sdk/src/tests/models.gen.ts
- examples/example-node-worker/main.ts
- examples/example-vite-kitchen-sink/src/components/caller-counter.tsx
- packages/sdk/src/web/index.ts
- packages/state/src/recs/index.ts
🧰 Additional context used
🧬 Code Graph Analysis (3)
packages/sdk/src/internal/types.ts (4)
packages/sdk/src/__tests__/models.gen.ts (1)
SchemaType(85-97)examples/example-node-worker/src/typescript/models.gen.ts (1)
SchemaType(80-94)packages/sdk/src/internal/pagination.ts (1)
Pagination(11-103)packages/sdk/src/internal/toriiQueryBuilder.ts (1)
ToriiQueryBuilder(27-160)
packages/react/src/useQuerySync.ts (1)
packages/state/src/recs/index.ts (1)
getSyncEntities(49-69)
packages/sdk/src/internal/toriiQueryBuilder.ts (2)
packages/sdk/src/internal/errors.ts (1)
UNDEFINED_CLAUSE(5-6)packages/sdk/src/internal/pagination.ts (1)
Pagination(11-103)
🪛 Biome (1.9.4)
packages/sdk/src/internal/types.ts
[error] 5-5: 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)
examples/example-vite-react-app-recs/src/App.tsx
[error] 24-24: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
packages/react/src/useQuerySync.ts
[error] 31-31: This hook does not specify all of its dependencies: clause
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
packages/sdk/src/internal/pagination.ts
[error] 11-11: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 38-38: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
packages/sdk/src/internal/toriiQueryBuilder.ts
[error] 8-8: Some named imports are only used as types.
This import is only used as a type.
Importing the types with import type ensures that they are removed by the compilers and avoids loading unnecessary modules.
Safe fix: Add inline type keywords.
(lint/style/useImportType)
[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)
packages/sdk/src/web/react/hooks/hooks.ts
[error] 58-58: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: check
- GitHub Check: build
🔇 Additional comments (17)
examples/example-vite-react-app-recs/src/App.tsx (1)
3-5: Import updates align with type-safety and new SDK features.These import changes are part of the migration to the new pagination model:
- Using
import typeforEntityis good TypeScript practice- Adding
KeysClauseis necessary for the new clause-based query systempackages/sdk/src/internal/toriiQueryBuilder.ts (1)
147-159: Good addition of safe clause access and pagination getter.The new methods provide better error handling and access to internal query components:
getClause()safely returns the clause as a Result type with proper error handlinggetPagination()exposes the pagination settings directlyThese additions support the new pagination model and enhance the builder pattern API.
packages/react/src/useQuerySync.ts (3)
2-8: Good use of type-only imports for cleaner output.The change to use
import typefor schema and component types is a best practice that improves tree-shaking in the compiled output.
26-30: Updated hook signature to remove entity key clause parameter.This change correctly removes the deprecated
entityKeyClauseparameter and makesclausemandatory, aligning with the new unifiedClausetype approach.
31-34: Simplified call to getSyncEntities.The update correctly simplifies the call to
getSyncEntitiesby passing only the necessary parameters: client, components, and the unified clause.🧰 Tools
🪛 Biome (1.9.4)
[error] 31-31: This hook does not specify all of its dependencies: clause
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
packages/sdk/src/web/react/hooks/hooks.ts (3)
1-11: Type imports updated to support the new pagination model.The import changes properly align with the updated types in the Torii system, particularly for the
ClauseandSubscriptiontypes.
20-31: Simplified method signatures for the subscription hook factory.The changes correctly simplify the API by:
- Removing the generic
Historicalparameter fromsubscribeMethod- Making
queryToHashedKeysMethodreturn a synchronousClauseinstead of a promiseThese simplifications make the API more straightforward and consistent with the new pagination model.
🧰 Tools
🪛 Biome (1.9.4)
[error] 25-25: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
85-86: Updated initial data processing to use new pagination API.The change correctly uses the new
getItems()method to extract entities from the paginated response, aligning with the newPaginationclass approach.migration/v1-4-v1-5.md (1)
1-207: Well-structured migration guide for usersThe migration guide provides comprehensive documentation for users upgrading from v1.4 to v1.5.1, with clear explanations and examples for the major changes including:
- The shift to cursor-based pagination
- Updated response structure with the new
Paginationclass- New access patterns for paginated data
- Best practices for implementing the changes
This will greatly help users adapt to the API changes.
packages/sdk/src/node/index.ts (5)
67-69: Passing raw clause instead of entity keysThe code now passes the raw query clause directly to
onEntityUpdatedinstead of converting it to entity keys first. This is a cleaner approach that eliminates unnecessary conversion steps.
84-91: Consistent handling of pagination in event subscriptionsThe event subscription now uses the same pagination pattern as entity subscriptions, improving consistency across the API.
117-120: Improved entity query pagination with standardized result wrappingThe
getEntitiesmethod now wraps the results in aPaginationobject, providing a consistent interface for accessing paginated data.
62-64:❓ Verification inconclusive
Verify that torii-wasm returns entities with an
itemspropertyThis refactor assumes that
client.getEntities(q)now returns an object with anitemsproperty, whereas before it might have returned the entities directly. Ensure thetorii-wasmpackage has been updated to match this expectation.
🏁 Script executed:
#!/bin/bash # Check how the client.getEntities response is structured ast-grep --pattern $'const entities = await client.getEntities'Length of output: 64
Verify torii-wasm’s getEntities response shape
The SDK now does:
const entities = await client.getEntities(q); const parsedEntities = parseEntities<T>(entities.items);Please confirm that in the latest torii-wasm release,
client.getEntitiesindeed returns an object with anitemsarray (not the array directly).• Check
getEntities’s return type in torii-wasm’s source or its TypeScript definitions.
• Update any tests or documentation in torii-wasm to reflect the{ items: Entity[] }shape if needed.
288-289:✅ Verification successful
Changed parameter type for updateEventMessageSubscription
Similar to the entity subscription update, the event subscription update now uses
torii.Clauseinstead of an array of entity key clauses, which is a breaking change but aligns with the overall pagination design.
🏁 Script executed:
#!/bin/bash # Check if there are any direct usages of updateEventMessageSubscription in the codebase rg -A 3 "updateEventMessageSubscription" --type tsLength of output: 2214
updateEventMessageSubscription signature updated consistently across the SDK
I’ve verified that all call sites have been updated to accept a single
torii.Clause(not an array), including:
packages/sdk/src/internal/types.tspackages/sdk/src/web/index.tspackages/sdk/src/node/index.ts- React hooks in
packages/sdk/src/web/react/hooks/events.tsNo remaining references to the old array-based clause signature were found. This breaking change is implemented uniformly.
packages/sdk/src/internal/types.ts (3)
249-252: Updated response type to use Pagination wrapperThe
ToriiResponsetype now returns aPaginationobject wrapping the standardized query results, which is a good change that provides a consistent interface for handling paginated data.
432-433: Updated parameter type for updateEntitySubscriptionThe
clausesparameter type is nowtorii.Clauseinstead oftorii.EntityKeysClause[], which aligns with the updated pagination model and simplifies clause handling.
449-450: Updated parameter type for updateEventMessageSubscriptionThe
clausesparameter type is nowtorii.Clauseinstead oftorii.EntityKeysClause[], which aligns with the updated pagination model and simplifies clause handling.
| clauses: torii.Clause | ||
| ): Promise<void> => { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Changed parameter type for updateEntitySubscription
The clauses parameter type has been changed from torii.EntityKeysClause[] to torii.Clause, which aligns with the changes in the pagination model but may be a breaking change for existing code.
🏁 Script executed:
#!/bin/bash
# Check if there are any direct usages of updateEntitySubscription in the codebase
rg -A 3 "updateEntitySubscription" --type tsLength of output: 1725
Breaking change: updateEntitySubscription signature updated
The clauses parameter for updateEntitySubscription has been changed from an array of torii.EntityKeysClause[] to a single torii.Clause in all SDK entry points:
- packages/sdk/src/web/index.ts
- packages/sdk/src/node/index.ts
- packages/sdk/src/internal/types.ts
No remaining references to the old array-based signature were found in the codebase.
This is a breaking change for any clients consuming the SDK. Please:
- Bump the SDK’s major version.
- Update documentation and migration guides to reflect the new
clauses: torii.Clausesignature. - (Optional) Consider adding an overload or deprecation wrapper to ease the transition for existing consumers.
Closes #
Introduced changes
Checklist
Summary by CodeRabbit
New Features
Improvements
.getItems()methods.Bug Fixes
Documentation
Chores
Tests