feat: create reusable hooks and add sql hook to sdk #376
Conversation
WalkthroughThe pull request introduces significant architectural changes to the Dojo SDK, focusing on restructuring the state management, SDK initialization, and React integration. The modifications span multiple packages and example projects, with a primary emphasis on consolidating context management, introducing new SDK modules, and streamlining the way components interact with the Dojo ecosystem. Key changes include replacing context-based providers with a new Changes
Sequence DiagramsequenceDiagram
participant App as React Application
participant Provider as DojoSdkProvider
participant SDK as Dojo SDK
participant Client as World Client
participant Store as Zustand Store
App->>Provider: Render with SDK configuration
Provider->>SDK: Initialize SDK
SDK->>Client: Create World Client
SDK->>Store: Create Zustand Store
Provider-->>App: Provide Context with SDK, Client, Store
App->>Provider: Access SDK via useDojoSDK hook
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (8)
packages/sdk/tsconfig.json (1)
Line range hint
18-18: Include .tsx files in the compilation.The current include pattern
"include": ["src/**/*.ts"]will miss TypeScript React files. Consider updating it to include .tsx files:- "include": ["src/**/*.ts"], + "include": ["src/**/*.{ts,tsx}"],examples/example-vite-react-sdk/src/historical-events.tsx (1)
Line range hint
23-46: Includesdkin dependency array and add cleanup function inuseEffect.
Dependency Array Issue: The
sdkvariable is used within thesubscribeHistoricalEventfunction inside theuseEffect, but it's not included in the dependency array. Ifsdkchanges, the effect won't re-run, potentially causing the component to use a stalesdkinstance.Missing Cleanup Function: To prevent memory leaks, it's important to unsubscribe from the event subscription when the component unmounts or dependencies change. Currently, there's no cleanup function to handle this.
Apply this diff to include
sdkin the dependency array and add a cleanup function:useEffect(() => { async function subscribeHistoricalEvent(account: AccountInterface) { try { const s = await sdk.subscribeEventQuery({ // ... }); setSubscription(s); } catch (error) { setEvents([]); if (subscription) { subscription.free(); } console.error(error); } } if (account) { subscribeHistoricalEvent(account); } + return () => { + if (subscription) { + subscription.free(); + } + }; -}, [account, setEvents]); +}, [account, setEvents, sdk]);examples/example-vite-react-sql/src/hooks/useSystemCalls.ts (1)
Line range hint
18-45: Consider adding timeout to waitForEntityChange.The
waitForEntityChangecall could potentially hang indefinitely if the entity update never matches the condition.-await state.waitForEntityChange(entityId.toString(), (entity) => { +await state.waitForEntityChange(entityId.toString(), (entity) => { return ( entity?.models?.dojo_starter?.Moves?.remaining === remainingMoves ); -}); +}, { timeout: 30000 }); // Add 30-second timeoutexamples/example-vite-kitchen-sink/src/components/global-counter.tsx (1)
Line range hint
32-49: Consider handling error in the callback.The callback in
getEntityignores both data and error parameters, which could lead to missed error conditions.-callback: ({ data, error }) => {}, +callback: ({ data, error }) => { + if (error) { + console.error('Error fetching entity:', error); + } +},examples/example-vite-kitchen-sink/src/components/caller-counter.tsx (1)
Line range hint
105-115: Improve subscription cleanup logic.The subscription cleanup could be more robust by checking the subscription state before freeing.
-if (sub) { +if (sub?.free && typeof sub.free === 'function') { sub.free(); }examples/example-vite-kitchen-sink/src/components/chat.tsx (1)
Line range hint
89-91: Remove ts-expect-error comments.The code contains multiple ts-expect-error comments that should be addressed properly with correct typing.
Consider adding proper type definitions or using type assertions instead of suppressing TypeScript errors.
examples/example-vite-kitchen-sink/src/components/theme-switch.tsx (2)
Line range hint
102-104: Remove ts-expect-error comments and fix typing issues.The code contains multiple ts-expect-error comments that should be addressed with proper typing.
Consider creating proper type definitions for the theme-related data structures to avoid type assertions and error suppressions.
Line range hint
142-146: Add error handling for theme subscription failures.The subscription error handling could be more robust with proper user feedback.
if (error) { - throw error; + console.error('Theme subscription error:', error); + setTheme({ + current: AvailableThemeClassMap[AvailableTheme.Light], + next: AvailableThemeClassMap[AvailableTheme.Light] + }); + // Consider showing a user-friendly error message }
🧹 Nitpick comments (18)
packages/sdk/tsup.config.ts (1)
7-12: Document the new module structure in README.Consider adding documentation that explains:
- The purpose of each module
- How to import from specific modules
- Example usage for each module
Example documentation structure:
## SDK Modules The SDK is organized into several modules: - `@dojoengine/sdk`: Core SDK functionality - `@dojoengine/sdk/state`: State management utilities - `@dojoengine/sdk/react`: React hooks and components - `@dojoengine/sdk/sql`: SQL integration utilities ### Usage ```typescript // Core SDK import { DojoProvider } from '@dojoengine/sdk'; // State management import { useDojoState } from '@dojoengine/sdk/state'; // React hooks import { useDojoSDK } from '@dojoengine/sdk/react'; // SQL utilities import { useDojoSQL } from '@dojoengine/sdk/sql';</blockquote></details> <details> <summary>packages/sdk/src/sql/index.ts (2)</summary><blockquote> `41-43`: **Add comprehensive JSDoc documentation.** The hook is missing proper documentation explaining its purpose, parameters, and return values. Add detailed JSDoc: ```typescript /** * React hook for querying Torii SQL endpoint with TanStack Query integration. * * @template Output - The type of formatted data returned by the query * @template Input - The type of raw data returned from SQL * @param {string} query - SQL query to execute * @param {(rows: Input) => Output} formatFn - Function to format the query results * @param {UseQueryOptions<Output>["placeholderData"]} [defaultValue] - Default value while loading * @param {string} [toriiUrl] - Optional Torii endpoint URL (defaults to localhost:8080) * @returns {Object} Query result object * @returns {Output} .data - The formatted query results * @returns {Error} .error - Any error that occurred * @returns {boolean} .isPending - Whether the query is loading * @returns {boolean} .isRefetching - Whether the query is refetching */
44-69: Improve type safety and configuration.Several improvements could make this hook more robust:
- The
@ts-expect-errorneeds explanation or proper typing- The default Torii URL should be configurable
- Query key could be more specific for better cache management
Consider these improvements:
+const DEFAULT_TORII_URL = "http://127.0.0.1:8080"; + export function useToriiSQLQuery<Output, Input>( query: string, formatFn: (rows: Input) => Output, defaultValue?: UseQueryOptions<Output>["placeholderData"], toriiUrl?: string ) { const { data, error, isPending, isRefetching } = useTanStackQuery({ - queryKey: [query], + queryKey: ["torii-sql", toriiUrl ?? DEFAULT_TORII_URL, query], queryFn: async () => { return await queryTorii( - toriiUrl ?? "http://127.0.0.1:8080", + toriiUrl ?? DEFAULT_TORII_URL, query, formatFn ); }, - // @ts-expect-error trust me - placeholderData: defaultValue ?? [], + placeholderData: defaultValue ?? ([] as unknown as Output), }); return { data, error, isPending, isRefetching, }; }packages/sdk/package.json (1)
63-63: LGTM! Consider documenting the architectural changes.The addition of @dojoengine/core as a workspace dependency aligns with the SDK restructuring.
Consider adding a CHANGELOG.md entry or updating the README.md to document these architectural changes, especially:
- The new modular structure with state, react, and sql submodules
- The relationship between @dojoengine/core and @dojoengine/sdk
packages/sdk/src/state/zustand.ts (5)
Line range hint
23-45: Consider strengthening type constraints for better type safety.The interface is well-structured, but some parameters could benefit from additional type constraints:
entityIdparameters could be constrained to match your entity ID formattimeoutparameter could use a more specific type thannumberConsider adding these type improvements:
export interface GameState<T extends SchemaType> { - subscribeToEntity: (entityId: string, listener: (entity: ParsedEntity<T> | undefined) => void) => () => void; + subscribeToEntity: (entityId: `${string}:${string}`, listener: (entity: ParsedEntity<T> | undefined) => void) => () => void; - waitForEntityChange: (entityId: string, predicate: (entity: ParsedEntity<T> | undefined) => boolean, timeout?: number) => Promise<ParsedEntity<T> | undefined>; + waitForEntityChange: (entityId: `${string}:${string}`, predicate: (entity: ParsedEntity<T> | undefined) => boolean, timeout?: TimeoutMS) => Promise<ParsedEntity<T> | undefined>; } +type TimeoutMS = number & { readonly brand: unique symbol }; +const asTimeout = (ms: number): TimeoutMS => ms as TimeoutMS;
Line range hint
47-56: Enhance factory function documentation.While the function is documented, it could benefit from more detailed JSDoc comments including:
- Parameter constraints
- Return value details
- Usage examples
Consider expanding the documentation:
/** * Factory function to create a Zustand store based on a given SchemaType. * + * @description + * Creates a type-safe store with entity management, optimistic updates, and query capabilities. + * The store is configured with Immer and selector subscription middleware. * * @template T - The schema type. + * @throws {Error} When schema validation fails + * @example + * ```typescript + * type MySchema = { + * namespace1: { + * model1: { field1: string } + * } + * }; + * const useStore = createDojoStore<MySchema>(); + * ``` * @returns A Zustand hook tailored to the provided schema. */
Line range hint
57-107: Optimize entity update logic for better performance and type safety.The current implementation of
updateEntityuses multiple type assertions and complex object merging. Consider these improvements:
- Use TypeScript's type inference more effectively
- Optimize performance for large entity updates
- Reduce the use of type assertions
Consider this optimization:
updateEntity: (entity: Partial<ParsedEntity<T>>) => { set((state: Draft<GameState<T>>) => { if (entity.entityId && entity.models) { const existingEntity = state.entities[entity.entityId]; if (existingEntity) { - // Create new models object without spread - const mergedModels: typeof existingEntity.models = Object.assign({}, existingEntity.models); - // Iterate through each namespace in the new models - Object.entries(entity.models).forEach(([namespace, namespaceModels]) => { - const typedNamespace = namespace as keyof ParsedEntity<T>["models"]; - if (!(typedNamespace in mergedModels)) { - mergedModels[typedNamespace as keyof typeof mergedModels] = {} as any; - } - mergedModels[typedNamespace as keyof typeof mergedModels] = Object.assign( - {}, - mergedModels[typedNamespace as keyof typeof mergedModels], - namespaceModels - ); - }); + // Type-safe model merging with better performance + const mergedModels = Object.fromEntries( + Object.entries(entity.models).map(([namespace, models]) => [ + namespace, + { + ...(existingEntity.models[namespace as keyof T] || {}), + ...models + } + ]) + ) as ParsedEntity<T>["models"]; state.entities[entity.entityId] = { ...existingEntity, ...entity, models: mergedModels, }; } else { state.entities[entity.entityId] = entity as WritableDraft<ParsedEntity<T>>; } } }); }
Line range hint
108-143: Add transaction cleanup and timeout handling.The transaction handling is robust but could benefit from:
- Automatic cleanup of stale transactions
- Timeout handling for pending transactions
- Transaction status tracking
Consider adding transaction timeouts and cleanup:
+interface PendingTransaction { + transactionId: string; + patches: Patch[]; + inversePatches: Patch[]; + timestamp: number; + status: 'pending' | 'confirmed' | 'reverted'; +} applyOptimisticUpdate: (transactionId, updateFn) => { const currentState = get(); const [nextState, patches, inversePatches] = produceWithPatches(currentState, (draftState: Draft<GameState<T>>) => { updateFn(draftState); }); set(() => nextState); set((state: Draft<GameState<T>>) => { state.pendingTransactions[transactionId] = { transactionId, patches, inversePatches, + timestamp: Date.now(), + status: 'pending' }; }); + // Auto-cleanup after timeout + setTimeout(() => { + const transaction = get().pendingTransactions[transactionId]; + if (transaction?.status === 'pending') { + get().revertOptimisticUpdate(transactionId); + } + }, 30000); // 30 second timeout }
Line range hint
144-199: Add batch subscription support and prevent memory leaks.The subscription and query methods could be enhanced with:
- Batch subscription capabilities for better performance
- Memory leak prevention through subscription cleanup
- Enhanced filtering capabilities
Consider these improvements:
+interface SubscriptionOptions { + batchSize?: number; + debounceMs?: number; +} -subscribeToEntity: (entityId, listener) => { +subscribeToEntity: (entityId, listener, options?: SubscriptionOptions) => { + let batchTimeout: NodeJS.Timeout | null = null; + let batch: ParsedEntity<T>[] = []; + + const cleanup = () => { + if (batchTimeout) { + clearTimeout(batchTimeout); + batchTimeout = null; + } + }; const unsubscribe = useStore.subscribe( (state) => state.entities[entityId], - (entity) => { + (entity) => { + if (options?.batchSize) { + batch.push(entity); + if (batch.length >= options.batchSize) { + listener(entity); + batch = []; + } else if (options?.debounceMs && !batchTimeout) { + batchTimeout = setTimeout(() => { + listener(entity); + batch = []; + batchTimeout = null; + }, options.debounceMs); + } + } else { listener(entity); + } } ); - return unsubscribe; + return () => { + cleanup(); + unsubscribe(); + }; }packages/sdk/src/react/provider.tsx (2)
28-29: Fix the broken word in the inline comment.The word "cannot" is split across two lines, which affects readability.
Apply this diff to combine the lines:
-// @ts-expect-error Since we c -// annot dynamically set context at runtime, we will get a type error. +// @ts-expect-error Since we cannot dynamically set context at runtime, we will get a type error.
52-52: Update the error message to match the component name.The error message incorrectly references "DojoProvider" instead of "DojoSdkProvider".
Apply this diff to correct the error message:
if (currentValue) { - throw new Error("DojoProvider can only be used once"); + throw new Error("DojoSdkProvider can only be used once"); }examples/example-vite-react-sql/src/components/playground/total-entities.tsx (1)
15-18: Handle loading and error states when usinguseToriiSQLQueryCurrently, the component assumes that
totalEntitiesis readily available. To enhance user experience and prevent potential runtime errors, consider handling loading and error states returned byuseToriiSQLQuery.Here's how you might update the component:
const { data: totalEntities, error, isLoading } = useToriiSQLQuery( TOTAL_ENTITIES_QUERY, formatFn ); if (isLoading) return <div>Loading...</div>; if (error) return <div>Error: {error.message}</div>;packages/sdk/src/react/hooks.ts (1)
36-41: Consider adding JSDoc documentation.The new
useDojoSDKhook would benefit from JSDoc documentation explaining its purpose, generic parameters, and return value, similar to the documentation provided foruseModel.+/** + * Custom hook to access the Dojo SDK context. + * + * @typeParam Client - The type of the client function + * @typeParam Schema - The type of the schema extending SchemaType + * @returns The Dojo context containing SDK functionality + */ export function useDojoSDK< Client extends (...args: any) => any, Schema extends SchemaType, >(): DojoContextType<Client, Schema> { return useContext(DojoContext); }examples/example-vite-react-sql/src/components/playground/direction-count.tsx (1)
66-72: Consider extracting default rendering to a constant.The default values are duplicated between the conditional rendering and the
defaultDirectionObject. Consider reusing the object.+const DefaultDirectionDisplay = ({ directions = defaultDirectionObject }) => ( + <div> + Player went :<br /> + Left <b>{directions.Left}</b> times<br /> + Up <b>{directions.Up}</b> times<br /> + Down <b>{directions.Down}</b> times<br /> + Right <b>{directions.Right}</b> times<br /> + </div> +); -if (!directions) { - return ( - <div> - Player went :<br /> - Left <b>0</b> times<br /> - Up <b>0</b> times<br /> - Down <b>0</b> times<br /> - Right <b>0</b> times<br /> - </div> - ); -} +if (!directions) { + return <DefaultDirectionDisplay />; +}examples/example-vite-kitchen-sink/src/components/global-counter.tsx (1)
Line range hint
89-96: Consider using AbortController for cleanup.The subscription cleanup could be enhanced using AbortController to handle race conditions during component unmount.
useEffect(() => { + const abortController = new AbortController(); async function subscribeToEntityUpdates(db: SDK<SchemaType>) { const sub = await db.subscribeEntityQuery({ query: new QueryBuilder<SchemaType>() .namespace("onchain_dash", (n) => n.entity("GlobalCounter", (e) => e.eq("global_counter_key", 9999999) ) ) .build(), - callback: ({ data, error }) => { + callback: ({ data, error }) => { + if (abortController.signal.aborted) return; // ... rest of the callback }, }); setSub(sub); } if (db && sub === null) { subscribeToEntityUpdates(db) .then(() => {}) .catch(console.error); } return () => { + abortController.abort(); if (sub) { sub.free(); } }; }, [db, sub, setIsLoading]);examples/example-vite-kitchen-sink/src/components/caller-counter.tsx (1)
34-34: Consider using type inference for better maintainability.The explicit type parameters might be unnecessary if the SDK can infer types from the setupWorld.
-const { sdk } = useDojoSDK<typeof setupWorld, SchemaType>(); +const { sdk } = useDojoSDK();examples/example-vite-kitchen-sink/src/components/chat.tsx (1)
34-34: Improve type safety with SDK alias.The SDK alias might cause confusion. Consider using a more descriptive name.
-const { sdk: db } = useDojoSDK<typeof setupWorld, SchemaType>(); +const { sdk: messageSDK } = useDojoSDK<typeof setupWorld, SchemaType>();examples/example-vite-kitchen-sink/src/components/theme-switch.tsx (1)
51-54: Improve SDK destructuring with proper typing.The SDK destructuring could be more concise and type-safe.
-const { sdk: db, client: actions } = useDojoSDK< - typeof setupWorld, - SchemaType ->(); +const { sdk: themeSDK, client: themeActions } = useDojoSDK<typeof setupWorld, SchemaType>();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (34)
.changeset/great-kiwis-peel.md(1 hunks)examples/example-vite-kitchen-sink/src/components/caller-counter.tsx(5 hunks)examples/example-vite-kitchen-sink/src/components/chat.tsx(2 hunks)examples/example-vite-kitchen-sink/src/components/global-counter.tsx(2 hunks)examples/example-vite-kitchen-sink/src/components/theme-switch.tsx(2 hunks)examples/example-vite-kitchen-sink/src/dojo/provider.tsx(0 hunks)examples/example-vite-kitchen-sink/src/main.tsx(2 hunks)examples/example-vite-react-sdk/src/App.tsx(2 hunks)examples/example-vite-react-sdk/src/historical-events.tsx(1 hunks)examples/example-vite-react-sdk/src/main.tsx(2 hunks)examples/example-vite-react-sdk/src/useDojo.tsx(0 hunks)examples/example-vite-react-sdk/src/useModel.tsx(0 hunks)examples/example-vite-react-sql/package.json(2 hunks)examples/example-vite-react-sql/src/components/playground/action.tsx(1 hunks)examples/example-vite-react-sql/src/components/playground/direction-count.tsx(4 hunks)examples/example-vite-react-sql/src/components/playground/schema.tsx(2 hunks)examples/example-vite-react-sql/src/components/playground/total-entities.tsx(2 hunks)examples/example-vite-react-sql/src/hooks/useDojoStore.ts(0 hunks)examples/example-vite-react-sql/src/hooks/usePlayerActions.ts(1 hunks)examples/example-vite-react-sql/src/hooks/useQuery.ts(0 hunks)examples/example-vite-react-sql/src/hooks/useSystemCalls.ts(2 hunks)examples/example-vite-react-sql/src/main.tsx(2 hunks)examples/example-vite-react-sql/tsconfig.app.tsbuildinfo(1 hunks)examples/example-vite-react-sql/tsconfig.node.tsbuildinfo(1 hunks)packages/sdk/package.json(2 hunks)packages/sdk/src/index.ts(0 hunks)packages/sdk/src/react/hooks.ts(3 hunks)packages/sdk/src/react/index.ts(1 hunks)packages/sdk/src/react/provider.tsx(1 hunks)packages/sdk/src/sql/index.ts(1 hunks)packages/sdk/src/state/zustand.ts(1 hunks)packages/sdk/tsconfig.json(1 hunks)packages/sdk/tsup.config.ts(1 hunks)pnpm-workspace.yaml(1 hunks)
💤 Files with no reviewable changes (6)
- examples/example-vite-react-sdk/src/useDojo.tsx
- examples/example-vite-react-sql/src/hooks/useDojoStore.ts
- packages/sdk/src/index.ts
- examples/example-vite-kitchen-sink/src/dojo/provider.tsx
- examples/example-vite-react-sdk/src/useModel.tsx
- examples/example-vite-react-sql/src/hooks/useQuery.ts
✅ Files skipped from review due to trivial changes (2)
- examples/example-vite-react-sql/tsconfig.node.tsbuildinfo
- packages/sdk/src/react/index.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/sdk/src/react/provider.tsx
[error] 47-47: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: check
🔇 Additional comments (31)
packages/sdk/tsconfig.json (1)
3-3: LGTM! JSX transform configuration looks good.The addition of
"jsx": "react-jsx"is appropriate for the new React integration in the SDK.examples/example-vite-react-sql/tsconfig.app.tsbuildinfo (1)
1-1: Skip detailed review of auto-generated build info file.This is an auto-generated TypeScript build info file that doesn't require direct review.
However, let's verify the consistency of hook usage across the codebase since the PR involves significant changes to hooks:
#!/bin/bash # Description: Verify the transition from useDojoStore to useDojoSDK # and ensure no legacy hook usage remains # Check for any remaining useDojoStore references echo "Checking for remaining useDojoStore references..." rg -l "useDojoStore" # Check for new useDojoSDK usage echo "Checking new useDojoSDK usage..." rg -l "useDojoSDK" # Look for potential hook-related imports that might need updates echo "Checking hook-related imports..." rg -l "import.*from.*@dojoengine/sdk/(hooks|react)"packages/sdk/tsup.config.ts (1)
7-12: LGTM! Verify the entry point paths exist.The modular structure with separate entry points for different SDK functionalities is a good architectural decision. This allows for better tree-shaking and more flexible imports.
Let's verify that all the entry point files exist:
✅ Verification successful
✓ Entry point paths verified
All specified entry points exist and contain exports, confirming the configuration is valid.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all entry point files exist # Expected: All files should exist and contain exports for file in "src/index.ts" "src/state/index.ts" "src/react/index.ts" "src/sql/index.ts"; do echo "Checking $file..." if [ -f "packages/sdk/$file" ]; then # Check if file contains any exports rg -l "export" "packages/sdk/$file" else echo "File $file does not exist" fi doneLength of output: 1099
pnpm-workspace.yaml (1)
10-13:⚠️ Potential issueFix invalid React package versions.
The specified version
^18.3.1for React packages is invalid as it doesn't exist. React's latest stable version is 18.2.0.Apply this diff to use the correct versions:
- "@types/react": ^18.3.1 - "@types/react-dom": ^18.3.1 - react: ^18.3.1 - react-dom: ^18.3.1 + "@types/react": ^18.2.0 + "@types/react-dom": ^18.2.0 + react: ^18.2.0 + react-dom: ^18.2.0Let's verify the latest available versions:
packages/sdk/src/sql/index.ts (1)
1-4: LGTM! Clean and focused imports.Good practice renaming
useQuerytouseTanStackQueryto avoid potential naming conflicts.packages/sdk/package.json (1)
8-30: LGTM! Verify build outputs exist for new modules.The exports configuration is well-structured and follows Node.js best practices for dual CJS/ESM support.
Let's verify the build outputs exist:
✅ Verification successful
✅ Source files and configuration verified
All exported modules have corresponding source files in the correct locations, and TypeScript is properly configured to generate the necessary build outputs. The package structure aligns perfectly with the exports configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all exported module paths exist after build # Check if the build outputs exist for all exported modules fd -t f "^(index|state|react|sql)\.(d\.ts|js|cjs)$" dist/Length of output: 152
Script:
#!/bin/bash # Check source files and configuration echo "=== Checking source files ===" fd -e ts -e tsx "^(index|state|react|sql)\.(ts|tsx)$" src/ echo -e "\n=== Checking TypeScript config ===" cat tsconfig.json 2>/dev/null || echo "tsconfig.json not found" echo -e "\n=== Checking package structure ===" ls -la src/ 2>/dev/null || echo "src/ directory not found"Length of output: 1982
Script:
#!/bin/bash echo "=== Checking SDK package structure ===" ls -la packages/sdk/src/ 2>/dev/null || echo "SDK src/ directory not found" echo -e "\n=== Checking source files ===" fd -e ts -e tsx "^(index|state|react|sql)\.(ts|tsx)$" packages/sdk/src/ echo -e "\n=== Checking SDK package config ===" cat packages/sdk/tsconfig.json 2>/dev/null || echo "SDK tsconfig.json not found"Length of output: 2414
examples/example-vite-react-sql/package.json (2)
17-18: LGTM! Synchronized TanStack Router updates.The TanStack Router ecosystem (@tanstack/router-devtools, @tanstack/router-plugin, @tanstack/react-router) has been consistently updated to version ^1.97.0, maintaining version alignment across related packages.
Also applies to: 48-48
28-28: Verify TypeScript compatibility with the new SDK.The TypeScript update to ^5.7.3 might introduce stricter type checking. Ensure this version is compatible with the new SDK hooks and SQL integration.
Run this script to check for any TypeScript-related issues:
✅ Verification successful
TypeScript 5.7.3 is compatible with the SDK implementation.
The codebase shows proper type integration between the example project and SDK, with well-structured type definitions and no compatibility issues detected. The type system is robust with proper schema types, model generation, and type safety mechanisms in place.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for TypeScript configuration and potential type issues # Check if tsconfig exists and its content if [ -f "tsconfig.json" ]; then echo "TypeScript Config:" cat tsconfig.json # Look for any TypeScript-specific TODO or FIXME comments echo -e "\nChecking for TypeScript-related TODOs:" rg -i "todo|fixme|hack" . -g "*.ts" -g "*.tsx" fiLength of output: 2542
Script:
#!/bin/bash # Check SDK and example TypeScript integration # Find TypeScript files in SDK and example directories echo "TypeScript files in SDK and example:" fd -e ts -e tsx . packages/sdk examples/example-vite-react-sql # Look for type definitions echo -e "\nType definitions:" fd -g "*.d.ts" . packages/sdk examples/example-vite-react-sql # Check for type imports between example and SDK echo -e "\nType imports:" rg "import.*from.*@coderabbit" examples/example-vite-react-sql -g "*.ts" -g "*.tsx" -A 1Length of output: 3820
Script:
#!/bin/bash # Check for type usage and potential issues # Look for type imports and usage in example project echo "Type imports and usage in example project:" rg "import.*type" examples/example-vite-react-sql/src -g "*.ts" -g "*.tsx" -A 1 # Check SDK type exports echo -e "\nSDK type exports:" rg "export.*type" packages/sdk/src/types.ts -A 1 # Look for any error handling related to types echo -e "\nType-related error handling:" rg "TypeError|type.*error" packages/sdk/src -g "*.ts" -g "*.tsx"Length of output: 3725
examples/example-vite-react-sdk/src/App.tsx (5)
1-1: Imports updated appropriately.The necessary React hooks are correctly imported.
12-12: ImportinguseDojoSDKfrom the correct package.The
useDojoSDKhook is correctly imported from@dojoengine/sdk/react.
20-20: Removedsdkparameter fromAppfunction.The
Appcomponent no longer requires thesdkprop since it's now accessed via theuseDojoSDKhook internally, simplifying the component interface.
21-21: UsinguseDojoSDKto access SDK and store.Destructured
useDojoStore,client, andsdkfromuseDojoSDK(), which is an appropriate use of the hook to access the necessary SDK components.
286-286: Removedsdkprop fromHistoricalEventscomponent.The
HistoricalEventscomponent now accessessdkvia theuseDojoSDKhook internally, eliminating the need to passsdkas a prop and enhancing component encapsulation.examples/example-vite-react-sdk/src/historical-events.tsx (3)
7-7: ImportinguseDojoSDKfrom the correct package.The
useDojoSDKhook is correctly imported from@dojoengine/sdk/react.
9-9: Removedsdkparameter fromHistoricalEventsfunction.The
HistoricalEventscomponent no longer requires thesdkprop since it's now accessed via theuseDojoSDKhook internally, simplifying the component interface.
11-11: UsinguseDojoSDKto accesssdk.Destructured
sdkfromuseDojoSDK(), which is appropriate for accessing the SDK within the component.examples/example-vite-react-sdk/src/main.tsx (2)
43-51: Verify provider nesting for correct context propagationPlease ensure that wrapping
StarknetProviderinsideDojoSdkProviderprovides the required contexts to all child components. Confirm that the order of the providers does not affect the availability of necessary contexts inAppand its descendants.
44-46: Confirm compatibility ofsetupWorldwithclientFnVerify that the
setupWorldfunction passed asclientFntoDojoSdkProvidermatches the expected interface and functionality required by the provider. Ensure thatsetupWorldproperly initializes the client as intended.examples/example-vite-kitchen-sink/src/main.tsx (2)
38-46: Verify initialization ofDojoSdkProviderwith correct propsEnsure that the props passed to
DojoSdkProvider(sdk,dojoConfig,clientFn) are accurate and compatible with the provider's expected interface. Confirm thatsetupWorldis the appropriate function forclientFnand thatdojoConfigcontains all necessary configurations.
18-18: Ensure all references todbare updated tosdkSince the variable
dbhas been renamed tosdk, please verify that all references todbthroughout the codebase have been updated accordingly to prevent any undefined references or runtime errors.You can run the following script to check for any remaining references:
packages/sdk/src/react/hooks.ts (1)
15-24: LGTM! Enhanced type safety with generic parameters.The addition of
ClientandSchemageneric parameters touseModelimproves type safety and provides better type inference when accessing the store through context.examples/example-vite-react-sql/src/main.tsx (1)
53-57: LGTM! Clean integration with the new SDK provider.The transition to
DojoSdkProviderwith explicit configuration throughdojoConfigandclientFnprops provides a more structured and maintainable way to initialize the SDK.examples/example-vite-react-sql/src/components/playground/action.tsx (1)
1-1: LGTM! Aligned with SDK restructuring.The import path change from local hooks to
@dojoengine/sdk/reactaligns with the architectural changes to centralize hooks in the SDK.examples/example-vite-react-sql/src/components/playground/schema.tsx (1)
1-1: LGTM! Migrated to SDK's SQL module.The transition from local
useQuerytouseToriiSQLQueryfrom the SDK's SQL module maintains the same functionality while leveraging the centralized SDK implementation.Also applies to: 49-49
examples/example-vite-react-sql/src/components/playground/direction-count.tsx (2)
1-3: LGTM! Clean import restructuring.The imports have been properly updated to use the new SDK structure, improving modularity by separating React-specific functionality into
@dojoengine/sdk/reactand SQL functionality into@dojoengine/sdk/sql.
57-60: LGTM! SQL query implementation.The transition to
useToriiSQLQueryaligns with the new SDK structure while maintaining the same functionality.examples/example-vite-react-sql/src/hooks/usePlayerActions.ts (2)
1-6: LGTM! Clean import structure.The imports have been properly updated to use the new SDK structure, with clear separation of concerns.
10-12: Consider adding error handling for SDK initialization.The destructuring of SDK hooks could fail if the SDK is not properly initialized.
examples/example-vite-react-sql/src/hooks/useSystemCalls.ts (1)
14-14: LGTM! Clean SDK hook usage.The transition to useDojoSDK is properly implemented, maintaining the necessary functionality while improving the architecture.
examples/example-vite-kitchen-sink/src/components/global-counter.tsx (1)
29-29: LGTM! Proper SDK type parameters.The SDK initialization with proper type parameters (
setupWorldandSchemaType) ensures type safety throughout the component..changeset/great-kiwis-peel.md (1)
1-15: Verify the migration impact across the codebase.The architectural change of moving hooks to the SDK and introducing new modules (
@dojoengine/sdk/state,@dojoengine/sdk/react,@dojoengine/sdk/sql) is significant. Let's verify the migration's completeness.
| export async function queryTorii<Input, Output>( | ||
| toriiUrl: string, | ||
| query: string, | ||
| formatFn: (rows: Input) => Output | ||
| ): Promise<Output> { | ||
| try { | ||
| const response = await fetch(`${toriiUrl}/sql`, { | ||
| method: "POST", | ||
| headers: { | ||
| "Content-Type": "application/json", | ||
| }, | ||
| body: query, | ||
| }); | ||
| if (!response.ok) throw new Error("Failed to fetch data"); | ||
| return formatFn(await response.json()); | ||
| } catch (error) { | ||
| console.error("Error fetching data:", error); | ||
| throw error; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation and improve error handling.
The current implementation has several areas that could be improved for robustness:
- The query is sent as raw string in the body instead of a JSON structure
- No timeout handling for the fetch operation
- Basic error handling could be more informative
Consider this improved implementation:
export async function queryTorii<Input, Output>(
toriiUrl: string,
query: string,
formatFn: (rows: Input) => Output
): Promise<Output> {
+ if (!toriiUrl?.trim()) {
+ throw new Error("Invalid Torii URL");
+ }
+ if (!query?.trim()) {
+ throw new Error("Empty SQL query");
+ }
try {
- const response = await fetch(`${toriiUrl}/sql`, {
+ const controller = new AbortController();
+ const timeout = setTimeout(() => controller.abort(), 5000);
+ const response = await fetch(`${toriiUrl}/sql`, {
method: "POST",
headers: {
"Content-Type": "application/json",
},
- body: query,
+ body: JSON.stringify({ query }),
+ signal: controller.signal,
});
+ clearTimeout(timeout);
- if (!response.ok) throw new Error("Failed to fetch data");
+ if (!response.ok) {
+ throw new Error(`Failed to fetch data: ${response.status} ${response.statusText}`);
+ }
return formatFn(await response.json());
} catch (error) {
- console.error("Error fetching data:", error);
+ if (error.name === 'AbortError') {
+ throw new Error('Request timeout');
+ }
+ console.error("[queryTorii] Error:", error);
throw error;
}
}📝 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.
| export async function queryTorii<Input, Output>( | |
| toriiUrl: string, | |
| query: string, | |
| formatFn: (rows: Input) => Output | |
| ): Promise<Output> { | |
| try { | |
| const response = await fetch(`${toriiUrl}/sql`, { | |
| method: "POST", | |
| headers: { | |
| "Content-Type": "application/json", | |
| }, | |
| body: query, | |
| }); | |
| if (!response.ok) throw new Error("Failed to fetch data"); | |
| return formatFn(await response.json()); | |
| } catch (error) { | |
| console.error("Error fetching data:", error); | |
| throw error; | |
| } | |
| } | |
| export async function queryTorii<Input, Output>( | |
| toriiUrl: string, | |
| query: string, | |
| formatFn: (rows: Input) => Output | |
| ): Promise<Output> { | |
| if (!toriiUrl?.trim()) { | |
| throw new Error("Invalid Torii URL"); | |
| } | |
| if (!query?.trim()) { | |
| throw new Error("Empty SQL query"); | |
| } | |
| try { | |
| const controller = new AbortController(); | |
| const timeout = setTimeout(() => controller.abort(), 5000); | |
| const response = await fetch(`${toriiUrl}/sql`, { | |
| method: "POST", | |
| headers: { | |
| "Content-Type": "application/json", | |
| }, | |
| body: JSON.stringify({ query }), | |
| signal: controller.signal, | |
| }); | |
| clearTimeout(timeout); | |
| if (!response.ok) { | |
| throw new Error(`Failed to fetch data: ${response.status} ${response.statusText}`); | |
| } | |
| return formatFn(await response.json()); | |
| } catch (error) { | |
| if (error.name === 'AbortError') { | |
| throw new Error('Request timeout'); | |
| } | |
| console.error("[queryTorii] Error:", error); | |
| throw error; | |
| } | |
| } |
| "@types/react": "catalog:", | ||
| "@types/react-dom": "catalog:", | ||
| "@tanstack/react-query": "^5.62.16", | ||
| "starknet": "catalog:", | ||
| "react": "catalog:", | ||
| "react-dom": "catalog:" |
There was a problem hiding this comment.
Fix invalid version specifiers in peer dependencies.
The "catalog:" version specifier is invalid. Additionally, using a fixed version for react-query in peer dependencies could cause version conflicts.
Apply this diff to fix the version specifiers:
- "@types/react": "catalog:",
- "@types/react-dom": "catalog:",
+ "@types/react": "^18.0.0",
+ "@types/react-dom": "^18.0.0",
"@tanstack/react-query": "^5.62.16",
- "starknet": "catalog:",
- "react": "catalog:",
- "react-dom": "catalog:"
+ "starknet": "^5.14.1",
+ "react": "^18.0.0",
+ "react-dom": "^18.0.0"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "@types/react": "catalog:", | |
| "@types/react-dom": "catalog:", | |
| "@tanstack/react-query": "^5.62.16", | |
| "starknet": "catalog:", | |
| "react": "catalog:", | |
| "react-dom": "catalog:" | |
| "@types/react": "^18.0.0", | |
| "@types/react-dom": "^18.0.0", | |
| "@tanstack/react-query": "^5.62.16", | |
| "starknet": "^5.14.1", | |
| "react": "^18.0.0", | |
| "react-dom": "^18.0.0" |
| * | ||
| * @param props.children - Child components that will have access to the Dojo context | ||
| * @param props.burnerManager - Instance of BurnerManager for handling burner accounts | ||
| * @throws {Error} If DojoProvider is used more than once in the component tree |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove the outdated parameter description in the comment.
The burnerManager prop is mentioned in the comment but is not part of the function's parameters.
Apply this diff to remove the unnecessary comment:
* @param props.children - Child components that will have access to the Dojo context
- * @param props.burnerManager - Instance of BurnerManager for handling burner accounts
* @throws {Error} If DojoProvider is used more than once in the component tree📝 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.
| * @throws {Error} If DojoProvider is used more than once in the component tree | |
| * @param props.children - Child components that will have access to the Dojo context | |
| * @throws {Error} If DojoProvider is used more than once in the component tree |
| Schema extends SchemaType, | ||
| > { | ||
| /** The Dojo client instance */ | ||
| config: DojoConfig; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Correct the comment for the 'config' property.
The comment for the config property incorrectly states "The Dojo client instance". It should describe the actual purpose of the property.
Apply this diff to fix the comment:
- /** The Dojo client instance */
+ /** The Dojo configuration */Committable suggestion skipped: line range outside the PR's diff.
| }: { | ||
| dojoConfig: DojoConfig; | ||
| sdk: SDK<Schema>; | ||
| clientFn: Function; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using 'Function' as a type annotation.
Using Function as a type is discouraged because it is too generic and can lead to type safety issues. Prefer specifying the exact function signature.
Modify the type annotation to reflect the expected function shape. For example:
- clientFn: Function;
+ clientFn: (provider: DojoProvider) => ReturnType<Client>;Replace (provider: DojoProvider) => ReturnType<Client> with the actual function signature expected by clientFn.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (1.9.4)
[error] 47-47: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
| @@ -39,9 +41,10 @@ function formatFn(rows: QueryResponse): DirectionCount { | |||
|
|
|||
| export function DirectionCount() { | |||
| // use queryClient to invalidateQuery when state is changing. | |||
| const { useDojoStore } = useContext(DojoContext); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using useDojoSDK hook instead of useContext.
The component is still using context directly while the PR's objective is to transition to the new SDK hooks structure.
-const { useDojoStore } = useContext(DojoContext);
+const { useDojoStore } = useDojoSDK();Also applies to: 47-47
| if (address && sdk) { | ||
| getEntity(sdk, address).then(setCount).catch(console.error); | ||
| } | ||
| }, [address, db]); | ||
| }, [address, sdk]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error boundary for SDK initialization.
The SDK usage should be wrapped in proper error handling to gracefully handle initialization failures.
-if (address && sdk) {
+try {
+ if (address && sdk) {
getEntity(sdk, address).then(setCount).catch(console.error);
+ }
+} catch (error) {
+ console.error('Failed to initialize SDK:', error);
+ // Consider showing a user-friendly error message
}📝 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.
| if (address && sdk) { | |
| getEntity(sdk, address).then(setCount).catch(console.error); | |
| } | |
| }, [address, db]); | |
| }, [address, sdk]); | |
| try { | |
| if (address && sdk) { | |
| getEntity(sdk, address).then(setCount).catch(console.error); | |
| } | |
| } catch (error) { | |
| console.error('Failed to initialize SDK:', error); | |
| // Consider showing a user-friendly error message | |
| } | |
| }, [address, sdk]); |
MartianGreed
force-pushed
the
feat/add-sdk-packages
branch
from
044df1f to
125c510
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (4)
examples/example-vite-kitchen-sink/src/components/theme-switch.tsx (2)
Line range hint
89-89: Address type safety issues and remove @ts-expect-error comments.There are several type safety issues that should be addressed:
- Enum value resolution needs proper type checking
- EntityId query type needs to be properly typed
- Theme mapping needs proper type guards
Here's how to fix these issues:
// 1. Create a type guard for theme values function isValidTheme(theme: string): theme is keyof typeof AvailableTheme { return theme in AvailableTheme; } // 2. Type the entity query properly interface ThemeEntity { entityId: string; // Add other required properties } // 3. Use type guards for theme mapping const themeValue = entity.models?.onchain_dash?.Theme?.value.unwrap(); if (themeValue && isValidTheme(themeValue)) { setTheme({ current: AvailableThemeClassMap[AvailableTheme[themeValue]], next: AvailableThemeClassMap[AvailableTheme[themeValue]] }); }Also applies to: 115-115, 134-135
Line range hint
71-146: Improve component lifecycle management and error handling.Several improvements could enhance the component's reliability:
- Add subscription cleanup
- Implement proper error handling
- Simplify useEffect logic
Here's how to improve the code:
useEffect(() => { if (!db || !entityId) return; let isSubscribed = true; async function subscribeToEntityUpdates() { try { const subscription = await db.subscribeEntityQuery({ query: [entityId], callback: ({ data, error }) => { if (!isSubscribed) return; if (error) { // Handle error properly, possibly with an error state onError?.(error); return; } const entity = data?.pop() as ParsedEntity<SchemaType>; if (!entity?.models?.onchain_dash?.Theme?.value) { setTheme({ current: AvailableThemeClassMap[AvailableTheme.Light], next: AvailableThemeClassMap[AvailableTheme.Light] }); return; } // Process theme update... } }); setSub(subscription); } catch (error) { onError?.(error); } } subscribeToEntityUpdates(); return () => { isSubscribed = false; sub?.unsubscribe(); }; }, [db, entityId]);Consider extracting the theme subscription logic into a custom hook for better reusability and testing.
examples/example-vite-kitchen-sink/src/main.tsx (1)
Line range hint
18-35: Add error handling for SDK initialization.The SDK initialization should be wrapped in proper error handling to gracefully handle failures.
async function main() { + try { const sdk = await init<SchemaType>( { client: { rpcUrl: getRpcUrl(), toriiUrl: env.VITE_TORII_URL, relayUrl: env.VITE_RELAY_URL, worldAddress: dojoConfig.manifest.world.address, }, domain: { name: "OnChainDash", revision: "1", chainId: "1", version: "1", }, }, schema ); + } catch (error) { + console.error("Failed to initialize SDK:", error); + // Consider showing a user-friendly error message or fallback UI + return; + }examples/example-vite-react-sdk/src/historical-events.tsx (1)
Line range hint
44-65: Improve subscription error handling.The subscription error handling could be improved to provide better cleanup and error recovery.
} catch (error) { - setEvents([]); if (subscription) { subscription.free(); } - console.error(error); + console.error('Failed to subscribe to historical events:', error); + // Consider implementing retry logic + const retryTimeout = setTimeout(() => { + if (account) { + subscribeHistoricalEvent(account); + } + }, 5000); + return () => clearTimeout(retryTimeout); }
♻️ Duplicate comments (2)
packages/sdk/package.json (1)
55-60:⚠️ Potential issueFix invalid version specifiers in peer dependencies.
The "catalog:" version specifier is invalid and using fixed versions for peer dependencies could cause version conflicts.
packages/sdk/src/sql/index.ts (1)
20-39:⚠️ Potential issueEnhance error handling and request formatting.
The current implementation needs improvements in several areas:
- The query is sent as a raw string instead of JSON
- Missing input validation for toriiUrl and query
- No timeout handling for fetch operations
- Basic error handling could be more informative
The previous review comment's suggestions for improvement are still valid and should be implemented.
🧹 Nitpick comments (15)
examples/example-vite-react-sdk/src/useSystemCalls.ts (3)
4-4: Great architectural improvement!The transition from context-based to SDK-based initialization provides better encapsulation and centralized state management. This change aligns well with modern React patterns and makes the codebase more maintainable.
Also applies to: 14-14
Line range hint
30-67: Consider enhancing error handling specificity.While the error handling is good, it could be more specific to help with debugging and user feedback. Consider categorizing different types of errors (e.g., network issues, contract errors) and handling them separately.
Here's a suggested improvement:
try { await client.actions.spawn(account!); await state.waitForEntityChange(entityId, (entity) => { return ( entity?.models?.dojo_starter?.Moves?.remaining === remainingMoves ); }); } catch (error) { state.revertOptimisticUpdate(transactionId); - console.error("Error executing spawn:", error); - throw error; + if (error instanceof Error) { + console.error(`Spawn failed: ${error.message}`); + if (error.message.includes("timeout")) { + throw new Error("Spawn timed out. Please try again."); + } else if (error.message.includes("rejected")) { + throw new Error("Transaction was rejected. Please check your account balance."); + } + } + throw new Error("Failed to spawn entity. Please try again later."); } finally { state.confirmTransaction(transactionId); }
13-15: Add explicit type annotations for better type safety.While TypeScript can infer types, explicit annotations improve code maintainability and documentation.
Consider adding these type annotations:
-export const useSystemCalls = () => { - const { useDojoStore, client } = useDojoSDK(); - const state = useDojoStore((state) => state); +export const useSystemCalls = (): { spawn: () => Promise<void> } => { + const { useDojoStore, client } = useDojoSDK(); + const state = useDojoStore((state: DojoState) => state);examples/example-vite-kitchen-sink/src/components/theme-switch.tsx (1)
19-19: LGTM! Consider improving variable naming clarity.The transition to
useDojoSDKwith proper type parameters enhances type safety. However, the destructuring aliases (sdk: dbandclient: actions) might be confusing. Consider using the original property names for better clarity:- const { sdk: db, client: actions } = useDojoSDK< + const { sdk, client } = useDojoSDK< typeof setupWorld, SchemaType >();Also applies to: 51-54
packages/sdk/src/react/provider.tsx (1)
28-29: Merge the split comment.The comment explaining the ts-expect-error is split across two lines, making it harder to read.
Apply this diff to merge the comment:
-// @ts-expect-error Since we c -// annot dynamically set context at runtime, we will get a type error. +// @ts-expect-error Since we cannot dynamically set context at runtime, we will get a type error.packages/sdk/src/sql/index.ts (2)
41-43: Add missing documentation for useToriiSQLQuery function.The function lacks proper documentation. Consider adding JSDoc comments describing:
- Purpose and usage of the function
- Template parameters
OutputandInput- Function parameters including their types and purposes
- Return value structure
/** + * React hook to query Torii instance over SQL endpoint with TanStack Query integration. + * + * @template Output - The type of formatted data after processing + * @template Input - The type of raw data from SQL query + * @param {string} query - Raw SQL query to execute + * @param {(rows: Input) => Output} formatFn - Function to format the query results + * @param {UseQueryOptions<Output>["placeholderData"]} [defaultValue] - Optional default value while loading + * @param {string} [toriiUrl] - Optional Torii URL (defaults to http://127.0.0.1:8080) + * @returns {Object} Query result object containing: + * - data: The formatted query results + * - error: Any error that occurred + * - isPending: Loading state indicator + * - isRefetching: Refetch state indicator */
54-54: Move default Torii URL to a configuration constant.The hardcoded URL
"http://127.0.0.1:8080"should be moved to a configuration constant or environment variable for better maintainability and flexibility.+const DEFAULT_TORII_URL = process.env.TORII_URL || "http://127.0.0.1:8080"; + export function useToriiSQLQuery<Output, Input>( query: string, formatFn: (rows: Input) => Output, defaultValue?: UseQueryOptions<Output>["placeholderData"], toriiUrl?: string ) { // ... - toriiUrl ?? "http://127.0.0.1:8080", + toriiUrl ?? DEFAULT_TORII_URL,examples/example-vite-kitchen-sink/src/main.tsx (1)
38-46: Consider adding error boundaries.The
DojoSdkProvidershould be wrapped in an error boundary to handle runtime errors gracefully.+import { ErrorBoundary } from 'react-error-boundary'; createRoot(document.getElementById("root")!).render( <StrictMode> + <ErrorBoundary fallback={<div>Something went wrong</div>}> <DojoSdkProvider sdk={sdk} dojoConfig={dojoConfig} clientFn={setupWorld} > <RootLayout> <Home /> </RootLayout> </DojoSdkProvider> + </ErrorBoundary> </StrictMode> );examples/example-vite-react-sdk/src/main.tsx (1)
Line range hint
17-23: Update JSDoc to reflect new provider structure.The JSDoc comment is outdated as it references initialization and burner manager setup that's no longer relevant.
/** * Initializes and bootstraps the Dojo application. - * Sets up the SDK, burner manager, and renders the root component. + * Sets up the SDK with DojoSdkProvider and renders the root component. * - * @throws {Error} If initialization fails + * @throws {Error} If SDK initialization fails */examples/example-vite-react-sdk/src/historical-events.tsx (1)
Line range hint
18-31: Consider uncommenting and using the structured query.The commented query provides better type safety and filtering capabilities compared to the current entityIds approach.
- query: { entityIds: [addAddressPadding(account.address)] }, + query: { + event_messages_historical: { + Moved: { + $: { where: { player: { $eq: addAddressPadding(account.address) } } } + } + } + },examples/example-vite-kitchen-sink/src/components/caller-counter.tsx (1)
Line range hint
105-115: Improve subscription cleanup logic.The subscription cleanup could be more robust by handling potential cleanup errors.
return () => { if (sub) { + try { sub.free(); + } catch (error) { + console.error('Failed to cleanup subscription:', error); + } } };examples/example-vite-react-sdk/src/App.tsx (2)
284-284: Remove outdated comment.The comment about passing SDK as props is no longer relevant since the component now uses context.
- {/* // Here sdk is passed as props but this can be done via contexts */} <HistoricalEvents />
Line range hint
19-24: Consider extracting store initialization.The store initialization logic could be extracted to a custom hook for better reusability and testing.
// useGameStore.ts export function useGameStore() { const { useDojoStore, client, sdk } = useDojoSDK(); const state = useDojoStore((state) => state); const entities = useDojoStore((state) => state.entities); return { state, entities, client, sdk }; }packages/sdk/src/react/hooks.ts (2)
Line range hint
6-34: Update JSDoc to document new generic parameters.The function documentation should be updated to include descriptions for the new generic parameters
ClientandSchema./** * Custom hook to retrieve a specific model for a given entityId within a specified namespace. * + * @typeParam Client - The client function type + * @typeParam Schema - The schema type extending SchemaType * @param entityId - The ID of the entity. * @param model - The model to retrieve, specified as a string in the format "namespace-modelName". * @returns The model structure if found, otherwise undefined. */
36-41: Add documentation and error handling for useDojoSDK hook.The new hook would benefit from:
- JSDoc documentation explaining its purpose and usage
- Error handling for when used outside the DojoContext provider
+/** + * Custom hook to access the Dojo SDK context. + * + * @typeParam Client - The client function type + * @typeParam Schema - The schema type extending SchemaType + * @returns The Dojo context value + * @throws {Error} When used outside of DojoContext.Provider + */ export function useDojoSDK< Client extends (...args: any) => any, Schema extends SchemaType, >(): DojoContextType<Client, Schema> { - return useContext(DojoContext); + const context = useContext(DojoContext); + if (!context) { + throw new Error('useDojoSDK must be used within a DojoContext.Provider'); + } + return context; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (36)
.changeset/great-kiwis-peel.md(1 hunks)examples/example-vite-kitchen-sink/src/components/caller-counter.tsx(5 hunks)examples/example-vite-kitchen-sink/src/components/chat.tsx(2 hunks)examples/example-vite-kitchen-sink/src/components/global-counter.tsx(2 hunks)examples/example-vite-kitchen-sink/src/components/theme-switch.tsx(2 hunks)examples/example-vite-kitchen-sink/src/dojo/provider.tsx(0 hunks)examples/example-vite-kitchen-sink/src/main.tsx(2 hunks)examples/example-vite-react-sdk/src/App.tsx(2 hunks)examples/example-vite-react-sdk/src/historical-events.tsx(1 hunks)examples/example-vite-react-sdk/src/main.tsx(2 hunks)examples/example-vite-react-sdk/src/useDojo.tsx(0 hunks)examples/example-vite-react-sdk/src/useModel.tsx(0 hunks)examples/example-vite-react-sdk/src/useSystemCalls.ts(2 hunks)examples/example-vite-react-sdk/tsconfig.app.tsbuildinfo(1 hunks)examples/example-vite-react-sql/package.json(2 hunks)examples/example-vite-react-sql/src/components/playground/action.tsx(1 hunks)examples/example-vite-react-sql/src/components/playground/direction-count.tsx(4 hunks)examples/example-vite-react-sql/src/components/playground/schema.tsx(2 hunks)examples/example-vite-react-sql/src/components/playground/total-entities.tsx(2 hunks)examples/example-vite-react-sql/src/hooks/useDojoStore.ts(0 hunks)examples/example-vite-react-sql/src/hooks/usePlayerActions.ts(1 hunks)examples/example-vite-react-sql/src/hooks/useQuery.ts(0 hunks)examples/example-vite-react-sql/src/hooks/useSystemCalls.ts(2 hunks)examples/example-vite-react-sql/src/main.tsx(2 hunks)examples/example-vite-react-sql/tsconfig.app.tsbuildinfo(1 hunks)examples/example-vite-react-sql/tsconfig.node.tsbuildinfo(1 hunks)packages/sdk/package.json(2 hunks)packages/sdk/src/index.ts(0 hunks)packages/sdk/src/react/hooks.ts(3 hunks)packages/sdk/src/react/index.ts(1 hunks)packages/sdk/src/react/provider.tsx(1 hunks)packages/sdk/src/sql/index.ts(1 hunks)packages/sdk/src/state/zustand.ts(1 hunks)packages/sdk/tsconfig.json(1 hunks)packages/sdk/tsup.config.ts(1 hunks)pnpm-workspace.yaml(1 hunks)
💤 Files with no reviewable changes (6)
- examples/example-vite-react-sdk/src/useDojo.tsx
- packages/sdk/src/index.ts
- examples/example-vite-kitchen-sink/src/dojo/provider.tsx
- examples/example-vite-react-sdk/src/useModel.tsx
- examples/example-vite-react-sql/src/hooks/useQuery.ts
- examples/example-vite-react-sql/src/hooks/useDojoStore.ts
✅ Files skipped from review due to trivial changes (1)
- examples/example-vite-react-sdk/tsconfig.app.tsbuildinfo
🚧 Files skipped from review as they are similar to previous changes (18)
- examples/example-vite-react-sql/tsconfig.node.tsbuildinfo
- packages/sdk/tsconfig.json
- packages/sdk/src/state/zustand.ts
- packages/sdk/src/react/index.ts
- .changeset/great-kiwis-peel.md
- examples/example-vite-react-sql/tsconfig.app.tsbuildinfo
- examples/example-vite-react-sql/src/components/playground/action.tsx
- packages/sdk/tsup.config.ts
- examples/example-vite-react-sql/src/main.tsx
- examples/example-vite-react-sql/src/components/playground/schema.tsx
- examples/example-vite-react-sql/src/hooks/usePlayerActions.ts
- examples/example-vite-react-sql/src/components/playground/total-entities.tsx
- pnpm-workspace.yaml
- examples/example-vite-react-sql/src/hooks/useSystemCalls.ts
- examples/example-vite-kitchen-sink/src/components/chat.tsx
- examples/example-vite-kitchen-sink/src/components/global-counter.tsx
- examples/example-vite-react-sql/src/components/playground/direction-count.tsx
- examples/example-vite-react-sql/package.json
🧰 Additional context used
🪛 Biome (1.9.4)
packages/sdk/src/react/provider.tsx
[error] 47-47: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: check
🔇 Additional comments (10)
packages/sdk/package.json (2)
8-30: LGTM! Well-structured exports configuration.The exports field is well-organized with proper submodule separation and follows Node.js dual package hazard patterns. The TypeScript types are correctly configured.
63-63: LGTM! Correct workspace dependency configuration.The addition of @dojoengine/core as a workspace dependency is properly configured using the workspace:* syntax.
packages/sdk/src/react/provider.tsx (4)
36-37: Remove the outdated parameter description in the comment.The
burnerManagerprop is mentioned in the comment but is not part of the function's parameters.
47-47: Avoid using 'Function' as a type annotation.Using
Functionas a type is too generic and can lead to type safety issues. Prefer specifying the exact function signature.🧰 Tools
🪛 Biome (1.9.4)
[error] 47-47: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
39-73: Well-structured provider implementation!The
DojoSdkProviderimplementation is clean and follows best practices:
- Prevents multiple provider instances
- Properly initializes the provider with config
- Provides all necessary context values to children
🧰 Tools
🪛 Biome (1.9.4)
[error] 47-47: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
14-16: 🛠️ Refactor suggestionFix duplicate JSDoc comment.
The comment "The Dojo client instance" is used for both
configandclientproperties. The comment forconfigshould be updated to reflect its actual purpose.Apply this diff to fix the comments:
- /** The Dojo client instance */ + /** The Dojo configuration object */ config: DojoConfig; /** The Dojo client instance */ client: ReturnType<Client>;Likely invalid or redundant comment.
examples/example-vite-react-sdk/src/main.tsx (1)
43-51: LGTM! Well-structured provider hierarchy.The nesting of providers (
DojoSdkProvider->StarknetProvider->App) is logically organized.examples/example-vite-kitchen-sink/src/components/caller-counter.tsx (1)
58-61: Add error boundary for SDK initialization.The SDK usage should be wrapped in proper error handling to gracefully handle initialization failures.
packages/sdk/src/react/hooks.ts (2)
1-4: LGTM! Clean import organization.The imports are well-organized and follow a good separation of concerns by moving types to a dedicated file.
Line range hint
1-41: Verify migration path for existing consumers.Since this change relocates hooks to the SDK and introduces breaking changes (useDojoDb → useDojoSDK), please ensure:
- All example projects have been updated
- Migration guide is provided
- Version bump follows semver
✅ Verification successful
Migration verification completed successfully
The codebase shows complete adoption of the new hooks:
- All example projects have been updated to use useDojoSDK/useModel
- No legacy useDojoDb usage found
- Multiple reference implementations available in examples/
While explicit migration documentation could be beneficial, the example projects (particularly example-vite-react-sdk) serve as comprehensive implementation references.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any remaining useDojoDb usage that needs migration rg "useDojoDb" # Check for package.json to verify version bump cat package.json | jq .versionLength of output: 52
Script:
#!/bin/bash # Find and check SDK package.json cat packages/sdk/package.json | jq .version # Look for example projects fd -t d "example" # Check for migration guide fd -t f "MIGRATION|UPGRADING|CHANGELOG" -iLength of output: 917
Script:
#!/bin/bash # Check SDK changelog for migration notes cat packages/sdk/CHANGELOG.md # Check hook usage in example projects rg -A 2 "useDojoSDK|useModel" examples/Length of output: 10425
| // @ts-expect-error trust me | ||
| placeholderData: defaultValue ?? [], |
There was a problem hiding this comment.
Fix type issue instead of suppressing TypeScript error.
The @ts-expect-error comment suggests a type compatibility issue that should be fixed rather than suppressed. The default value of an empty array might not be compatible with all possible Output types.
Consider implementing proper type checking:
- // @ts-expect-error trust me
- placeholderData: defaultValue ?? [],
+ placeholderData: defaultValue ?? ([] as unknown as Output),Additionally, consider making the default value type-safe by:
- Making it required when Output is not array-like
- Using a type guard to ensure compatibility
📝 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.
| // @ts-expect-error trust me | |
| placeholderData: defaultValue ?? [], | |
| placeholderData: defaultValue ?? ([] as unknown as Output), |
MartianGreed
force-pushed
the
feat/add-sdk-packages
branch
2 times, most recently
from
37a0eae to
a4efc6f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (3)
examples/example-vite-react-sdk/src/historical-events.tsx (3)
Line range hint
25-29: Fix TypeScript type annotations for event messages.The
@ts-expect-errorcomment indicates a type mismatch. Consider adding proper type annotations for the event messages response.- // @ts-expect-error FIX: type here - setEvents(e); + type EventMessage = ParsedEntity<SchemaType>[][]; + setEvents(e as EventMessage);
Line range hint
44-67: Fix potential memory leak in subscription cleanup.The subscription cleanup is only handled in the error case, but not when the component unmounts or when the dependencies change.
useEffect(() => { async function subscribeHistoricalEvent(account: AccountInterface) { try { const s = await sdk.subscribeEventQuery({ query: { entityIds: [addAddressPadding(account.address)] }, callback: ({ data, error }) => { console.log(data, error); }, historical: true, }); setSubscription(s); } catch (error) { setEvents([]); if (subscription) { subscription.free(); } console.error(error); } } if (account) { subscribeHistoricalEvent(account); } + return () => { + if (subscription) { + subscription.free(); + } + }; - }, [account, setEvents]); + }, [account, setEvents, subscription]);
Line range hint
93-94: Fix TypeScript type annotations for Event component.The
@ts-expect-errorcomment indicates a type mismatch. Consider adding proper type annotations for the direction field.- {/* @ts-expect-error type is ok here */} - <div>Direction: {direction}</div> + <div>Direction: {direction?.toString()}</div>
♻️ Duplicate comments (2)
packages/sdk/package.json (1)
55-60:⚠️ Potential issueFix invalid version specifiers in peer dependencies.
The "catalog:" version specifiers are invalid and need to be replaced with proper semantic version ranges.
packages/sdk/src/sql/index.ts (1)
20-39:⚠️ Potential issueEnhance security and robustness of the query implementation.
The previous review comment about adding input validation and improving error handling remains valid. Additionally, consider these security aspects:
- Add SQL injection protection by implementing query parameterization
- Consider adding rate limiting headers
- Implement request sanitization
Extend the previous solution with these security enhancements:
export async function queryTorii<Input, Output>( toriiUrl: string, query: string, + params: Record<string, unknown> = {}, formatFn: (rows: Input) => Output ): Promise<Output> { if (!toriiUrl?.trim()) { throw new Error("Invalid Torii URL"); } if (!query?.trim()) { throw new Error("Empty SQL query"); } try { const controller = new AbortController(); const timeout = setTimeout(() => controller.abort(), 5000); const response = await fetch(`${toriiUrl}/sql`, { method: "POST", headers: { "Content-Type": "application/json", + "X-Request-ID": crypto.randomUUID(), }, - body: JSON.stringify({ query }), + body: JSON.stringify({ + query, + params: sanitizeParams(params), + }), signal: controller.signal, }); clearTimeout(timeout); if (!response.ok) { throw new Error(`Failed to fetch data: ${response.status} ${response.statusText}`); } return formatFn(await response.json()); } catch (error) { if (error.name === 'AbortError') { throw new Error('Request timeout'); } console.error("[queryTorii] Error:", error); throw error; } } +function sanitizeParams(params: Record<string, unknown>): Record<string, unknown> { + return Object.fromEntries( + Object.entries(params).map(([key, value]) => [ + key, + typeof value === 'string' ? value.replace(/[^\w\s-]/gi, '') : value + ]) + ); +}
🧹 Nitpick comments (12)
packages/sdk/src/sql/index.ts (3)
41-43: Add comprehensive JSDoc documentation.The function is missing proper documentation. Add JSDoc comments explaining:
- Purpose and usage of the hook
- Parameter descriptions and types
- Return value structure
- Example usage
/** + * React hook for querying Torii SQL endpoint with TanStack Query integration. + * + * @template Output - The type of the formatted data + * @template Input - The type of the raw SQL query result + * @param {string} query - SQL query to execute + * @param {(rows: Input) => Output} formatFn - Function to format the query results + * @param {UseQueryOptions<Output>["placeholderData"]} [defaultValue] - Default value while loading + * @param {string} [toriiUrl] - Optional Torii endpoint URL (defaults to localhost:8080) + * @returns {{ data: Output | undefined, error: Error | null, isPending: boolean, isRefetching: boolean }} + * + * @example + * ```tsx + * const { data, error, isPending } = useToriiSQLQuery<EntityCount[], RawCount[]>( + * "SELECT count(*) as count FROM entities", + * (rows) => rows.map(row => ({ count: Number(row.count) })) + * ); + * ``` */
54-54: Avoid hardcoding the default URL.The default Torii URL should be configurable at a higher level, possibly through environment variables or SDK configuration.
- toriiUrl ?? "http://127.0.0.1:8080", + toriiUrl ?? process.env.TORII_URL ?? "http://127.0.0.1:8080",
51-51: Enhance query key composition.The current query key only includes the query string. Consider adding more dimensions to prevent cache collisions and enable better cache management.
- queryKey: [query], + queryKey: ['torii', 'sql', toriiUrl ?? 'default', query],examples/example-vite-react-sdk/src/useSystemCalls.ts (1)
13-15: Consider enhancing type safety.While the implementation is solid, consider adding explicit return types to the hook and type definitions for the store state:
export const useSystemCalls = (): { spawn: () => Promise<void>; } => { const { useDojoStore, client } = useDojoSDK(); const state = useDojoStore<DojoState>((state) => state); // ...examples/example-vite-kitchen-sink/src/components/theme-switch.tsx (1)
51-54: Consider aligning with SDK naming conventions.While the code works, renaming
sdktodbandclienttoactionsthrough destructuring makes the code less intuitive and harder to maintain. Consider using the SDK's native property names:- const { sdk: db, client: actions } = useDojoSDK< + const { sdk, client } = useDojoSDK< typeof setupWorld, SchemaType >();This would:
- Better reflect the SDK's architecture
- Make the code more maintainable
- Align with other components using the SDK
examples/example-vite-kitchen-sink/src/main.tsx (1)
Line range hint
18-46: LGTM! Clean transition to the new SDK provider pattern.The initialization and provider setup look good. The change from
dbtosdknaming improves consistency with the new architecture.Consider adding error boundaries around the
DojoSdkProviderto gracefully handle SDK initialization failures:<StrictMode> + <ErrorBoundary fallback={<div>Failed to initialize Dojo SDK</div>}> <DojoSdkProvider sdk={sdk} dojoConfig={dojoConfig} clientFn={setupWorld} > <RootLayout> <Home /> </RootLayout> </DojoSdkProvider> + </ErrorBoundary> </StrictMode>examples/example-vite-kitchen-sink/src/components/caller-counter.tsx (3)
Line range hint
36-57: Enhance type safety in getEntity function.The entity parsing and type checking could be improved to be more type-safe:
async function getEntity(db: SDK<SchemaType>, address: string) { + type CallerCounterEntity = ParsedEntity<SchemaType> & { + models: { + onchain_dash?: { + CallerCounter?: { + counter: bigint; + }; + }; + }; + }; const entity = await db.getEntities({ query: new QueryBuilder<SchemaType>() .namespace("onchain_dash", (n) => n.entity("CallerCounter", (e) => e.eq("caller", addAddressPadding(address)) ) ) .build(), callback: () => {}, }); - const counter = entity.pop() as ParsedEntity<SchemaType>; + const counter = entity.pop() as CallerCounterEntity; if (!counter) { return 0; } - const count = counter.models.onchain_dash?.CallerCounter?.counter; - if (undefined === count) { - return 0; - } - return parseInt(count.toString(), 16); + return Number(counter.models.onchain_dash?.CallerCounter?.counter ?? 0n); }
Line range hint
105-115: Improve subscription setup and error handling.The subscription setup could be improved:
- Remove the empty
.then()block- Add proper error handling for the subscription setup
-if (address && sdk && sub === null) { - subscribeToEntityUpdates(sdk, address) - .then(() => {}) - .catch(console.error); -} +if (address && sdk && sub === null) { + subscribeToEntityUpdates(sdk, address).catch((error) => { + console.error('Failed to setup subscription:', error); + // Consider showing a user-friendly error message + }); +}
Line range hint
15-33: Improve loading state and error handling.Consider adding error state management and improving the loading state handling:
export default function CallerCounter() { const [count, setCount] = useState(0); const [isLoading, setIsLoading] = useState(false); + const [error, setError] = useState<Error | null>(null); const [sub, setSub] = useState<Subscription | null>(null); const { address } = useAccount(); const { send: incrementCallerCounter } = useSendTransaction({ calls: [ { contractAddress: dojoConfig.manifest.contracts[0].address, entrypoint: "increment_caller_counter", calldata: [], }, ], }); const handleCallerClick = useCallback(async () => { - incrementCallerCounter(); - setIsLoading(true); + try { + setError(null); + setIsLoading(true); + await incrementCallerCounter(); + } catch (err) { + setError(err instanceof Error ? err : new Error('Transaction failed')); + } finally { + setIsLoading(false); + } }, [incrementCallerCounter]);examples/example-vite-react-sdk/src/App.tsx (3)
13-18: Update JSDoc comment to reflect context-based SDK access.The JSDoc comment still references the removed
sdkprop. Update it to reflect the new context-based SDK access pattern./** * Main application component that provides game functionality and UI. * Handles entity subscriptions, state management, and user interactions. - * - * @param props.sdk - The Dojo SDK instance configured with the game schema */
Line range hint
34-106: Consider extracting subscription and fetching logic into custom hooks.The entity subscription and fetching logic could be moved to custom hooks for better reusability and separation of concerns. Additionally, consider improving error handling with user feedback.
Example structure:
// useEntitySubscription.ts export function useEntitySubscription(account: AccountInterface | undefined) { const { sdk, useDojoStore } = useDojoSDK(); const state = useDojoStore((state) => state); useEffect(() => { // ... subscription logic ... }, [sdk, account]); } // useEntityFetching.ts export function useEntityFetching(account: AccountInterface | undefined) { const { sdk, useDojoStore } = useDojoSDK(); const state = useDojoStore((state) => state); useEffect(() => { // ... fetching logic with improved error handling ... }, [sdk, account]); }
283-285: Remove outdated comment about SDK props.The comment about passing SDK as props is outdated since the component now uses context-based SDK access.
- {/* // Here sdk is passed as props but this can be done via contexts */} <HistoricalEvents />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (36)
.changeset/great-kiwis-peel.md(1 hunks)examples/example-vite-kitchen-sink/src/components/caller-counter.tsx(5 hunks)examples/example-vite-kitchen-sink/src/components/chat.tsx(2 hunks)examples/example-vite-kitchen-sink/src/components/global-counter.tsx(2 hunks)examples/example-vite-kitchen-sink/src/components/theme-switch.tsx(2 hunks)examples/example-vite-kitchen-sink/src/dojo/provider.tsx(0 hunks)examples/example-vite-kitchen-sink/src/main.tsx(2 hunks)examples/example-vite-react-sdk/src/App.tsx(2 hunks)examples/example-vite-react-sdk/src/historical-events.tsx(1 hunks)examples/example-vite-react-sdk/src/main.tsx(2 hunks)examples/example-vite-react-sdk/src/useDojo.tsx(0 hunks)examples/example-vite-react-sdk/src/useModel.tsx(0 hunks)examples/example-vite-react-sdk/src/useSystemCalls.ts(2 hunks)examples/example-vite-react-sdk/tsconfig.app.tsbuildinfo(1 hunks)examples/example-vite-react-sql/package.json(2 hunks)examples/example-vite-react-sql/src/components/playground/action.tsx(1 hunks)examples/example-vite-react-sql/src/components/playground/direction-count.tsx(4 hunks)examples/example-vite-react-sql/src/components/playground/schema.tsx(2 hunks)examples/example-vite-react-sql/src/components/playground/total-entities.tsx(2 hunks)examples/example-vite-react-sql/src/hooks/useDojoStore.ts(0 hunks)examples/example-vite-react-sql/src/hooks/usePlayerActions.ts(1 hunks)examples/example-vite-react-sql/src/hooks/useQuery.ts(0 hunks)examples/example-vite-react-sql/src/hooks/useSystemCalls.ts(2 hunks)examples/example-vite-react-sql/src/main.tsx(2 hunks)examples/example-vite-react-sql/tsconfig.app.tsbuildinfo(1 hunks)examples/example-vite-react-sql/tsconfig.node.tsbuildinfo(1 hunks)packages/sdk/package.json(2 hunks)packages/sdk/src/index.ts(0 hunks)packages/sdk/src/react/hooks.ts(3 hunks)packages/sdk/src/react/index.ts(1 hunks)packages/sdk/src/react/provider.tsx(1 hunks)packages/sdk/src/sql/index.ts(1 hunks)packages/sdk/src/state/zustand.ts(1 hunks)packages/sdk/tsconfig.json(1 hunks)packages/sdk/tsup.config.ts(1 hunks)pnpm-workspace.yaml(1 hunks)
💤 Files with no reviewable changes (6)
- examples/example-vite-react-sql/src/hooks/useDojoStore.ts
- examples/example-vite-react-sdk/src/useDojo.tsx
- packages/sdk/src/index.ts
- examples/example-vite-react-sdk/src/useModel.tsx
- examples/example-vite-react-sql/src/hooks/useQuery.ts
- examples/example-vite-kitchen-sink/src/dojo/provider.tsx
🚧 Files skipped from review as they are similar to previous changes (18)
- packages/sdk/tsconfig.json
- examples/example-vite-react-sdk/tsconfig.app.tsbuildinfo
- examples/example-vite-react-sql/tsconfig.node.tsbuildinfo
- .changeset/great-kiwis-peel.md
- packages/sdk/src/react/index.ts
- examples/example-vite-react-sql/src/components/playground/action.tsx
- examples/example-vite-react-sql/src/components/playground/total-entities.tsx
- examples/example-vite-react-sql/tsconfig.app.tsbuildinfo
- examples/example-vite-react-sql/src/components/playground/schema.tsx
- examples/example-vite-react-sql/src/main.tsx
- packages/sdk/tsup.config.ts
- examples/example-vite-kitchen-sink/src/components/chat.tsx
- examples/example-vite-kitchen-sink/src/components/global-counter.tsx
- pnpm-workspace.yaml
- packages/sdk/src/state/zustand.ts
- examples/example-vite-react-sql/src/hooks/usePlayerActions.ts
- examples/example-vite-react-sql/src/components/playground/direction-count.tsx
- examples/example-vite-react-sql/package.json
🧰 Additional context used
🪛 Biome (1.9.4)
packages/sdk/src/react/provider.tsx
[error] 47-47: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: check
🔇 Additional comments (18)
packages/sdk/package.json (2)
8-30: Well-structured module exports configuration!The exports field properly defines submodules with consistent paths for TypeScript types, ESM imports, and CommonJS requires. This configuration ensures compatibility across different module systems while maintaining proper type support.
63-63: LGTM: Proper workspace dependency configuration!The addition of
@dojoengine/coreas a workspace dependency is correctly configured using theworkspace:*syntax, which is appropriate for monorepo setups.examples/example-vite-react-sdk/src/historical-events.tsx (1)
9-11: LGTM! Good refactor to context-based SDK access.The change from prop-based to context-based SDK access using
useDojoSDKhook improves component encapsulation and follows React best practices.packages/sdk/src/sql/index.ts (2)
1-5: LGTM! Clean and well-organized imports.Good practice renaming
useQuerytouseTanStackQueryto avoid potential naming conflicts.
59-60: The previous review comment about fixing the type suppression remains valid. The suggestion to use([] as unknown as Output)or make the default value type-safe should be implemented.examples/example-vite-react-sdk/src/useSystemCalls.ts (1)
4-4: LGTM! Good migration to the new SDK structure.The change from local hook to SDK package import aligns well with the goal of creating reusable hooks.
examples/example-vite-kitchen-sink/src/components/theme-switch.tsx (2)
19-20: LGTM! Import changes align with the new SDK architecture.The imports correctly reflect the transition to the new SDK structure, maintaining proper organization with external dependencies followed by local imports.
Line range hint
95-134: Improve type safety and error handling.
- Several
@ts-expect-errorcomments indicate type system issues:// @ts-expect-error we should be able to use ['entityId'] here query: [entityId], // ... // @ts-expect-error this is ok current: AvailableThemeClassMap[at], // @ts-expect-error this is ok next: AvailableThemeClassMap[at],
- Error handling in the subscription callback could crash the application:
if (error) { throw error; }Consider:
- Properly typing the query parameters and theme mapping to remove
@ts-expect-errorcomments- Implementing proper error handling with error boundaries or error state management
Run this script to check for similar patterns across the codebase:
packages/sdk/src/react/hooks.ts (2)
15-23: LGTM! Good use of generics for type safety.The addition of
ClientandSchemageneric parameters touseModelimproves type safety and flexibility.
36-41: LGTM! Clean implementation of the SDK hook.The
useDojoSDKhook provides a straightforward way to access the SDK context. The generic type parameters ensure type safety across the application.examples/example-vite-react-sdk/src/main.tsx (1)
43-51: LGTM! Consistent implementation with other examples.The provider setup follows the same pattern as other examples, maintaining consistency across the codebase.
packages/sdk/src/react/provider.tsx (3)
36-37: Remove outdated parameter documentation.The comment still references the removed
burnerManagerparameter.
13-14: Fix incorrect property documentation.The comment for the
configproperty incorrectly describes it as "The Dojo client instance".
47-47:⚠️ Potential issueReplace 'Function' type with a specific function signature.
Using the
Functiontype reduces type safety. Define a specific function signature instead.Apply this diff:
- clientFn: Function; + clientFn: (provider: DojoProvider) => ReturnType<Client>;Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 47-47: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
examples/example-vite-react-sql/src/hooks/useSystemCalls.ts (1)
Line range hint
1-14: LGTM! Clean transition to the new SDK hook.The migration from
useDojotouseDojoSDKis well-implemented, maintaining the existing optimistic update functionality while adopting the new SDK pattern.examples/example-vite-kitchen-sink/src/components/caller-counter.tsx (2)
2-9: LGTM! Well-structured imports with proper typing.The import structure is clean and properly organized, with good use of TypeScript generics for type safety with the SDK.
58-61: Add error boundary for SDK initialization.The SDK usage should still be wrapped in proper error handling to gracefully handle initialization failures.
examples/example-vite-react-sdk/src/App.tsx (1)
Line range hint
19-24: LGTM! Clean transition to context-based SDK access.The initialization using
useDojoSDKhook provides a cleaner way to access SDK functionality and state management.
| @@ -12,11 +11,9 @@ import { useAccount } from "@starknet-react/core"; | |||
| * - spawn: Function to spawn a new entity with initial moves | |||
| */ | |||
| export const useSystemCalls = () => { | |||
| const { useDojoStore, client } = useDojoSDK(); | |||
There was a problem hiding this comment.
💡 Codebase verification
Migration incomplete in other example projects
The file under review correctly uses the new useDojoSDK pattern, but several other example projects still use the old useDojo pattern:
example-vite-react-threejs-recsexample-vite-react-app-recsexample-vite-react-phaser-recsexample-vite-react-pwa-recsexample-vue-app-recs(uses old setup pattern)
🔗 Analysis chain
Verify completeness of SDK migration.
The new initialization pattern looks good and provides cleaner access to SDK features. However, let's verify that all usages of the old context have been migrated.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining usages of the old useDojo pattern
rg "useDojo|setup: *{ *client *}" examples/
Length of output: 174
Script:
#!/bin/bash
# Search for old SDK patterns
echo "=== Old useDojo pattern ==="
rg "useDojo\(" examples/
echo -e "\n=== Old setup pattern ==="
rg "setup.*client" examples/
echo -e "\n=== New SDK pattern ==="
rg "useDojoSDK" examples/
Length of output: 2942
MartianGreed
force-pushed
the
feat/add-sdk-packages
branch
2 times, most recently
from
028076d to
b263d31
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (3)
examples/example-vite-kitchen-sink/src/components/theme-switch.tsx (2)
Line range hint
92-93: Address TypeScript type safety issues.There are multiple
@ts-expect-errorcomments that should be addressed:
- Line 92: Entity ID type mismatch in query
- Lines 122-123: Theme mapping type issues
- Lines 127-128: Theme mapping type issues
These comments indicate underlying type system issues that could lead to runtime errors.
Consider these improvements:
// Create a type-safe theme mapping type ThemeMapping = { [K in AvailableTheme]: string; }; const AvailableThemeClassMap: ThemeMapping = { [AvailableTheme.Light]: "light", [AvailableTheme.Dark]: "dark", [AvailableTheme.Dojo]: "dojo", } as const; // Use proper type assertions for theme value const theme = entity.models?.onchain_dash?.Theme?.value.unwrap() as keyof typeof AvailableTheme;Also applies to: 122-123, 127-128
Line range hint
96-146: Enhance component implementation with proper cleanup and error handling.The subscription and state management implementation could be improved in several areas:
- Missing cleanup for subscriptions
- Basic error handling with console.error
- Complex state management with multiple effects
Consider these improvements:
useEffect(() => { async function subscribeToEntityUpdates(db: SDK<SchemaType>) { const sub = await db.subscribeEntityQuery({ query: [entityId], callback: ({ data, error }) => { if (error) { - throw error; + console.error('Subscription error:', error); + setTheme({ + current: AvailableThemeClassMap[AvailableTheme.Light], + next: AvailableThemeClassMap[AvailableTheme.Light], + }); + return; } if (data) { // ... existing code ... } }, }); setSub(sub); } if (entityId && db && sub === null) { subscribeToEntityUpdates(db).then().catch(console.error); } + return () => { + if (sub) { + sub.unsubscribe(); + setSub(null); + } + }; }, [entityId, db, sub, theme]);examples/example-vite-react-sdk/src/historical-events.tsx (1)
Line range hint
41-67: Add cleanup logic and improve event handling.Several improvements needed:
- Add cleanup logic in useEffect's return function to prevent memory leaks
- Remove console.log from production code
- Consider handling subscription data more meaningfully than just logging
useEffect(() => { async function subscribeHistoricalEvent(account: AccountInterface) { try { const s = await sdk.subscribeEventQuery({ query: { entityIds: [addAddressPadding(account.address)] }, callback: ({ data, error }) => { - console.log(data, error); + if (error) { + // Handle error appropriately + return; + } + if (data) { + // Handle new event data + setEvents(prevEvents => [...prevEvents, data]); + } }, historical: true, }); setSubscription(s); } catch (error) { setEvents([]); if (subscription) { subscription.free(); } console.error(error); } } if (account) { subscribeHistoricalEvent(account); } + return () => { + if (subscription) { + subscription.free(); + } + }; }, [account, setEvents]);
♻️ Duplicate comments (4)
packages/sdk/src/sql/index.ts (2)
20-39:⚠️ Potential issueEnhance error handling and add request timeout.
Building upon the previous review comment, the implementation needs improvements in several areas:
- Input validation for URL and query
- Request timeout handling
- Structured JSON request body
- Enhanced error handling with specific error messages
Additionally, consider adding retry logic for transient failures:
export async function queryTorii<Input, Output>( toriiUrl: string, query: string, formatFn: (rows: Input) => Output ): Promise<Output> { + const MAX_RETRIES = 3; + const RETRY_DELAY = 1000; + + if (!toriiUrl?.trim()) { + throw new Error("Invalid Torii URL"); + } + if (!query?.trim()) { + throw new Error("Empty SQL query"); + } + + let lastError: Error; + for (let attempt = 1; attempt <= MAX_RETRIES; attempt++) { try { + const controller = new AbortController(); + const timeout = setTimeout(() => controller.abort(), 5000); const response = await fetch(`${toriiUrl}/sql`, { method: "POST", headers: { "Content-Type": "application/json", }, - body: query, + body: JSON.stringify({ query }), + signal: controller.signal, }); + clearTimeout(timeout); if (!response.ok) throw new Error("Failed to fetch data"); return formatFn(await response.json()); } catch (error) { - console.error("Error fetching data:", error); + lastError = error; + if (error.name === 'AbortError' || attempt === MAX_RETRIES) { + break; + } + await new Promise(resolve => setTimeout(resolve, RETRY_DELAY)); + continue; } + } + console.error(`[queryTorii] Failed after ${MAX_RETRIES} attempts:`, lastError); + throw lastError; } }
59-60:⚠️ Potential issueFix type compatibility issue instead of suppressing it.
The
@ts-expect-errorsuppression should be replaced with proper type handling.Additionally, consider making the default value type-safe by using a type constraint:
- // @ts-expect-error trust me - placeholderData: defaultValue ?? [], + placeholderData: defaultValue ?? ([] as unknown as Output extends any[] ? Output : never),packages/sdk/src/react/provider.tsx (1)
47-47: 🛠️ Refactor suggestionReplace generic Function type with specific function signature.
Using the generic Function type reduces type safety.
- clientFn: Function; + clientFn: (provider: DojoProvider) => ReturnType<Client>;🧰 Tools
🪛 Biome (1.9.4)
[error] 47-47: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
examples/example-vite-kitchen-sink/src/components/caller-counter.tsx (1)
58-61: 🛠️ Refactor suggestionAdd error boundary for SDK initialization.
The SDK usage should be wrapped in proper error handling.
🧹 Nitpick comments (12)
examples/example-vite-react-sdk/src/App.tsx (3)
11-11: Update component documentation to reflect context-based SDK access.The JSDoc comment still references the removed
sdkprop, but the component now uses theuseDojoSDKhook for SDK access.Apply this diff to update the documentation:
/** * Main application component that provides game functionality and UI. * Handles entity subscriptions, state management, and user interactions. - * - * @param props.sdk - The Dojo SDK instance configured with the game schema */Also applies to: 19-19
Line range hint
35-71: Consider extracting entity subscription logic into a custom hook.The entity subscription and fetching logic could be encapsulated in a custom hook to improve reusability and reduce component complexity. This would also help consolidate error handling.
Consider creating a hook like this:
function useEntitySync(account: AccountInterface | undefined) { const { useDojoStore, sdk } = useDojoSDK(); const state = useDojoStore((state) => state); useEffect(() => { if (!account) return; let unsubscribe: (() => void) | undefined; const subscribe = async () => { // ... subscription logic ... }; const fetchEntities = async () => { // ... fetching logic ... }; subscribe(); fetchEntities(); return () => unsubscribe?.(); }, [sdk, account]); }This would simplify the component to:
function App() { const { account } = useAccount(); useEntitySync(account); // ... rest of the component }Also applies to: 73-103
283-284: Remove outdated comment about props passing.The comment about SDK props passing is no longer relevant since the component now uses context.
- {/* // Here sdk is passed as props but this can be done via contexts */} <HistoricalEvents />examples/example-vite-kitchen-sink/src/components/theme-switch.tsx (2)
19-20: LGTM! Consider enhancing type safety further.The transition to
useDojoSDKwith explicit type parameters improves type safety. The destructuring pattern withsdk: dbandclient: actionsmakes the code more maintainable.Consider using TypeScript's
as constassertion to prevent accidental modifications:- const { sdk: db, client: actions } = useDojoSDK< + const { sdk: db, client: actions } = useDojoSDK< typeof setupWorld, SchemaType - >(); + >() as const;Also applies to: 51-54
Line range hint
38-170: Consider wrapping the component with an error boundary.Given the complexity of the theme switching logic and potential for runtime errors (network issues, contract failures), consider implementing an error boundary to gracefully handle failures.
Example implementation:
class ThemeSwitchErrorBoundary extends React.Component { static getDerivedStateFromError(error: Error) { return { hasError: true }; } componentDidCatch(error: Error, errorInfo: React.ErrorInfo) { // Log to error reporting service console.error('Theme switch error:', error, errorInfo); } render() { if (this.state.hasError) { return <Button variant="outline">Theme (Error)</Button>; } return this.props.children; } } // Usage: <ThemeSwitchErrorBoundary> <ThemeSwitchButton /> </ThemeSwitchErrorBoundary>examples/example-vite-react-sql/vite.config.ts (1)
15-17: Remove unnecessary ESM format configuration.The
format: "esm"configuration is redundant as it's already the default output format for Vite. Consider removing this configuration to keep the config file cleaner.rollupOptions: { external: ["chalk"], - output: { - format: "esm", - }, },packages/sdk/src/sql/index.ts (1)
41-43: Add missing documentation for useToriiSQLQuery function.The function lacks documentation explaining its purpose, parameters, and return value. Consider adding JSDoc documentation similar to
queryTorii./** + * React hook to query Torii instance over SQL endpoint using TanStack Query. + * + * @template Output - The type of formatted data + * @template Input - The type of raw SQL data + * @param {string} query - Raw SQL query + * @param {(rows: Input) => Output} formatFn - Format function callback + * @param {UseQueryOptions<Output>["placeholderData"]} [defaultValue] - Default value while loading + * @param {string} [toriiUrl] - Optional Torii URL (defaults to http://127.0.0.1:8080) + * @returns {{ data: Output, error: Error, isPending: boolean, isRefetching: boolean }} */examples/example-vite-react-sdk/src/historical-events.tsx (2)
Line range hint
15-39: Clean up commented query code and improve error handling.Two suggestions for improvement:
- Remove the commented query code if it's no longer needed, or document why it's kept for reference.
- Consider adding user feedback for error states instead of silently setting empty events.
try { const e = await sdk.getEventMessages({ - // query: { - // event_messages_historical: { - // Moved: { - // $: { where: { player: { $eq: addAddressPadding(account.address) } } } - // } - // } - // }, query: { entityIds: [addAddressPadding(account.address)] }, callback: () => {}, historical: true, }); // @ts-expect-error FIX: type here setEvents(e); } catch (error) { setEvents([]); + // Consider adding a state for error handling + // setError('Failed to fetch historical events'); console.error(error); }
Line range hint
69-102: Add loading state and fix type safety issues.The rendering logic needs improvement in several areas:
- Add loading state handling
- Fix the TypeScript type issue with the direction property
- Consider adding error state rendering
+ const [isLoading, setIsLoading] = useState(false); if (!account) { return ( <div className="mt-6"> <h2 className="text-white">Please connect your wallet</h2> </div> ); } + if (isLoading) { + return ( + <div className="mt-6"> + <h2 className="text-white">Loading events...</h2> + </div> + ); + } return ( <div className="mt-6"> <h2 className="text-white">Player Events :</h2> {events.map((e: ParsedEntity<SchemaType>[], key) => { return <Event event={e[0]} key={key} />; })} </div> ); } function Event({ event }: { event: ParsedEntity<SchemaType> }) { if (!event) return null; const player = event.models?.dojo_starter?.Moved?.player; - const direction = event.models?.dojo_starter?.Moved?.direction; + const direction = event.models?.dojo_starter?.Moved?.direction as string | undefined; return ( <div className="text-white flex gap-3"> <div>{event.entityId.toString()}</div> <div> <div>Player: {player}</div> - {/* @ts-expect-error type is ok here */} <div>Direction: {direction}</div> </div> </div> ); }packages/core/src/constants/index.ts (1)
1-3: LGTM! Consider adding documentation.The changes to use
127.0.0.1consistently across all local endpoints is a good improvement. The newLOCAL_RELAYconstant follows the same pattern.Consider adding JSDoc comments to document the purpose of each constant and any specific requirements or limitations.
packages/sdk/src/react/provider.tsx (1)
13-14: Fix duplicate comment for config and client properties.The JSDoc comment for both properties is identical: "The Dojo client instance".
/** The Dojo configuration */ config: DojoConfig; /** The Dojo client instance */ client: ReturnType<Client>;Also applies to: 15-16
examples/example-vite-kitchen-sink/src/components/caller-counter.tsx (1)
Line range hint
105-115: Consider adding loading state for subscription initialization.The subscription setup could fail silently. Consider adding a loading state and error handling.
if (address && sdk && sub === null) { + setIsLoading(true); subscribeToEntityUpdates(sdk, address) - .then(() => {}) + .then(() => { + setIsLoading(false); + }) - .catch(console.error); + .catch((error) => { + console.error('Failed to subscribe:', error); + setIsLoading(false); + // Consider showing user-friendly error message + }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (38)
.changeset/great-kiwis-peel.md(1 hunks)examples/example-vite-kitchen-sink/src/components/caller-counter.tsx(5 hunks)examples/example-vite-kitchen-sink/src/components/chat.tsx(2 hunks)examples/example-vite-kitchen-sink/src/components/global-counter.tsx(2 hunks)examples/example-vite-kitchen-sink/src/components/theme-switch.tsx(2 hunks)examples/example-vite-kitchen-sink/src/dojo/provider.tsx(0 hunks)examples/example-vite-kitchen-sink/src/main.tsx(2 hunks)examples/example-vite-react-sdk/src/App.tsx(2 hunks)examples/example-vite-react-sdk/src/historical-events.tsx(1 hunks)examples/example-vite-react-sdk/src/main.tsx(2 hunks)examples/example-vite-react-sdk/src/useDojo.tsx(0 hunks)examples/example-vite-react-sdk/src/useModel.tsx(0 hunks)examples/example-vite-react-sdk/src/useSystemCalls.ts(2 hunks)examples/example-vite-react-sdk/tsconfig.app.tsbuildinfo(1 hunks)examples/example-vite-react-sql/package.json(2 hunks)examples/example-vite-react-sql/src/components/playground/action.tsx(1 hunks)examples/example-vite-react-sql/src/components/playground/direction-count.tsx(4 hunks)examples/example-vite-react-sql/src/components/playground/schema.tsx(2 hunks)examples/example-vite-react-sql/src/components/playground/total-entities.tsx(2 hunks)examples/example-vite-react-sql/src/hooks/useDojoStore.ts(0 hunks)examples/example-vite-react-sql/src/hooks/usePlayerActions.ts(1 hunks)examples/example-vite-react-sql/src/hooks/useQuery.ts(0 hunks)examples/example-vite-react-sql/src/hooks/useSystemCalls.ts(2 hunks)examples/example-vite-react-sql/src/main.tsx(2 hunks)examples/example-vite-react-sql/tsconfig.app.tsbuildinfo(1 hunks)examples/example-vite-react-sql/tsconfig.node.tsbuildinfo(1 hunks)examples/example-vite-react-sql/vite.config.ts(1 hunks)packages/core/src/constants/index.ts(1 hunks)packages/sdk/package.json(2 hunks)packages/sdk/src/index.ts(0 hunks)packages/sdk/src/react/hooks.ts(3 hunks)packages/sdk/src/react/index.ts(1 hunks)packages/sdk/src/react/provider.tsx(1 hunks)packages/sdk/src/sql/index.ts(1 hunks)packages/sdk/src/state/zustand.ts(1 hunks)packages/sdk/tsconfig.json(1 hunks)packages/sdk/tsup.config.ts(1 hunks)pnpm-workspace.yaml(1 hunks)
💤 Files with no reviewable changes (6)
- examples/example-vite-react-sql/src/hooks/useDojoStore.ts
- examples/example-vite-react-sdk/src/useDojo.tsx
- packages/sdk/src/index.ts
- examples/example-vite-react-sql/src/hooks/useQuery.ts
- examples/example-vite-kitchen-sink/src/dojo/provider.tsx
- examples/example-vite-react-sdk/src/useModel.tsx
🚧 Files skipped from review as they are similar to previous changes (22)
- packages/sdk/tsconfig.json
- examples/example-vite-react-sql/tsconfig.node.tsbuildinfo
- packages/sdk/src/react/index.ts
- .changeset/great-kiwis-peel.md
- packages/sdk/src/state/zustand.ts
- examples/example-vite-react-sql/src/components/playground/schema.tsx
- examples/example-vite-react-sql/tsconfig.app.tsbuildinfo
- packages/sdk/tsup.config.ts
- examples/example-vite-react-sql/src/components/playground/action.tsx
- examples/example-vite-react-sql/src/components/playground/total-entities.tsx
- examples/example-vite-react-sql/src/hooks/usePlayerActions.ts
- examples/example-vite-react-sdk/tsconfig.app.tsbuildinfo
- pnpm-workspace.yaml
- examples/example-vite-react-sdk/src/useSystemCalls.ts
- examples/example-vite-react-sql/src/hooks/useSystemCalls.ts
- examples/example-vite-kitchen-sink/src/components/global-counter.tsx
- examples/example-vite-kitchen-sink/src/main.tsx
- examples/example-vite-react-sql/src/main.tsx
- examples/example-vite-react-sql/package.json
- examples/example-vite-kitchen-sink/src/components/chat.tsx
- packages/sdk/package.json
- examples/example-vite-react-sql/src/components/playground/direction-count.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
packages/sdk/src/react/provider.tsx
[error] 47-47: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: check
- GitHub Check: build
🔇 Additional comments (8)
examples/example-vite-react-sdk/src/App.tsx (1)
Line range hint
20-24: Well-structured state management using context and hooks!The shift to context-based state management using
useDojoSDKanduseDojoStoreimproves modularity and follows React best practices. The separation of concerns is clear, making the code more maintainable.examples/example-vite-react-sdk/src/main.tsx (3)
6-10: LGTM! Well-organized imports.The imports are logically organized and properly separated by type (core, SDK, generated code).
43-51: LGTM! Clean implementation of the new SDK provider.The
DojoSdkProvideris properly configured with all required props and maintains the correct component hierarchy.
46-46: Verify the setupWorld implementation.Since
setupWorldis a generated file, ensure it's properly generated and contains the expected contract setup logic.Let's verify the implementation:
✅ Verification successful
Implementation of
setupWorldis properly generated and completeThe generated contract file contains the expected setup logic with:
- Game actions (move, spawn) with proper error handling
- Contract interaction through DojoProvider
- Type-safe implementation with TypeScript interfaces
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the setupWorld implementation in the generated contracts file # Check if the file exists and contains the setupWorld function rg -A 10 "export function setupWorld" "./typescript/contracts.gen.ts"Length of output: 139
Script:
#!/bin/bash # Find all contracts.gen.ts files fd "contracts.gen.ts" # Search for setupWorld function in any .ts or .tsx files rg -A 10 "export.*function.*setupWorld" -t ts -t tsxLength of output: 444
Script:
#!/bin/bash # Check the specific contracts file content cat "examples/example-vite-react-sdk/src/typescript/contracts.gen.ts" # Search for setupWorld function in this specific file rg -A 10 "export.*function.*setupWorld" "examples/example-vite-react-sdk/src/typescript/contracts.gen.ts"Length of output: 2092
packages/sdk/src/sql/index.ts (1)
44-69: 🛠️ Refactor suggestionEnhance hook configuration and flexibility.
The current implementation has several areas for improvement:
- Hardcoded default URL should be configurable
- Query key should include URL for proper caching
- Missing options for customizing TanStack Query behavior
Consider this enhanced implementation:
+const DEFAULT_TORII_URL = "http://127.0.0.1:8080"; + export function useToriiSQLQuery<Output, Input>( query: string, formatFn: (rows: Input) => Output, defaultValue?: UseQueryOptions<Output>["placeholderData"], - toriiUrl?: string + toriiUrl?: string, + options?: Omit<UseQueryOptions<Output>, 'queryKey' | 'queryFn'> ) { const { data, error, isPending, isRefetching } = useTanStackQuery({ - queryKey: [query], + queryKey: ['torii-sql', toriiUrl ?? DEFAULT_TORII_URL, query], queryFn: async () => { return await queryTorii( - toriiUrl ?? "http://127.0.0.1:8080", + toriiUrl ?? DEFAULT_TORII_URL, query, formatFn ); }, - // @ts-expect-error trust me - placeholderData: defaultValue ?? [], + placeholderData: defaultValue ?? ([] as unknown as Output extends any[] ? Output : never), + ...options }); return { data, error, isPending, isRefetching, }; }Likely invalid or redundant comment.
examples/example-vite-react-sdk/src/historical-events.tsx (1)
1-9: LGTM! Good improvement in SDK access pattern.The transition to using
useDojoSDKhook instead of prop drilling the SDK instance improves component encapsulation and reusability.packages/sdk/src/react/hooks.ts (1)
15-17: Good addition of generic type parameters.The addition of
ClientandSchemageneric parameters improves type safety and flexibility.packages/sdk/src/react/provider.tsx (1)
35-37: Remove outdated parameter description.The
burnerManagerparameter is documented but not used in the function signature.
| // Ensure external modules are properly handled | ||
| external: ["chalk"], |
There was a problem hiding this comment.
💡 Codebase verification
Remove unnecessary chalk externalization
The chalk package is marked as external in the Vite config, but there's no evidence of its usage in the codebase. While a chalk dependency exists in one of the package.json files, no actual imports or usage of chalk were found in the source code.
- Remove the
external: ["chalk"]configuration as it's unnecessary and could cause confusion.
🔗 Analysis chain
Review the externalization of "chalk".
The externalization of the "chalk" package seems unusual for a frontend React application, as chalk is primarily used for terminal text styling in Node.js environments. This could lead to runtime errors if chalk is actually used in the browser context.
Let's verify the chalk usage in the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for chalk imports and usage
echo "Searching for chalk imports..."
rg -l "chalk" --type ts --type tsx
echo "\nChecking specific chalk usage..."
ast-grep --pattern 'import $_ from "chalk"'
Length of output: 258
Script:
#!/bin/bash
# Check for chalk in package.json
echo "Checking package.json for chalk dependency..."
fd package.json --exec cat {} | jq 'if .dependencies? then .dependencies.chalk else empty end, if .devDependencies? then .devDependencies.chalk else empty end'
echo "\nSearching for chalk imports and usage..."
# Search in js/ts files
rg -l "chalk" --type js --type ts
echo "\nChecking for various chalk import patterns..."
ast-grep --pattern 'import * from "chalk"'
ast-grep --pattern 'import { $_ } from "chalk"'
ast-grep --pattern 'const $_ = require("chalk")'
Length of output: 915
| export function HistoricalEvents() { | ||
| const { account } = useAccount(); | ||
| const { sdk } = useDojoSDK(); | ||
| const [events, setEvents] = useState<ParsedEntity<SchemaType>[][]>([]); | ||
| const [subscription, setSubscription] = useState<Subscription | null>(null); |
There was a problem hiding this comment.
💡 Codebase verification
Fix needed for TypeScript type issue in historical-events.tsx
The @ts-expect-error comment marked with "FIX: type here" indicates a known type issue that needs addressing, unlike other similar comments in the codebase which are intentional workarounds. Consider implementing proper type definitions for the event data.
🔗 Analysis chain
Consider addressing the TypeScript type issue.
The code includes a @ts-expect-error comment indicating a known type issue. Consider creating proper type definitions for the event data to improve type safety.
Let's check if there are similar type issues elsewhere in the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for ts-expect-error comments related to types
rg "@ts-expect-error" -A 2
Length of output: 6894
| export function useDojoSDK< | ||
| Client extends (...args: any) => any, | ||
| Schema extends SchemaType, | ||
| >(): DojoContextType<Client, Schema> { | ||
| return useContext(DojoContext); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling to useDojoSDK hook.
Similar to useModel, the hook should handle the case when used outside of DojoSdkProvider.
export function useDojoSDK<
Client extends (...args: any) => any,
Schema extends SchemaType,
>(): DojoContextType<Client, Schema> {
- return useContext(DojoContext);
+ const context = useContext(DojoContext);
+ if (!context) {
+ throw new Error('useDojoSDK must be used within a DojoSdkProvider');
+ }
+ return context;
}📝 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.
| export function useDojoSDK< | |
| Client extends (...args: any) => any, | |
| Schema extends SchemaType, | |
| >(): DojoContextType<Client, Schema> { | |
| return useContext(DojoContext); | |
| } | |
| export function useDojoSDK< | |
| Client extends (...args: any) => any, | |
| Schema extends SchemaType, | |
| >(): DojoContextType<Client, Schema> { | |
| const context = useContext(DojoContext); | |
| if (!context) { | |
| throw new Error('useDojoSDK must be used within a DojoSdkProvider'); | |
| } | |
| return context; | |
| } |
| const { useDojoStore } = useContext(DojoContext) as DojoContextType< | ||
| Client, | ||
| Schema | ||
| >; |
There was a problem hiding this comment.
💡 Codebase verification
The type casting safety concern is valid and should be addressed
The suggested runtime check aligns with established patterns across the codebase, as evidenced by similar implementations in multiple example applications. Adding this check will make the code more consistent and provide better error handling.
• Location: packages/sdk/src/react/hooks.ts:20-23
The suggested change should be implemented as proposed in the review comment.
🔗 Analysis chain
Verify type casting safety.
The type casting of useContext(DojoContext) could potentially fail at runtime if the context is not properly initialized.
Consider adding a runtime check:
- const { useDojoStore } = useContext(DojoContext) as DojoContextType<
- Client,
- Schema
- >;
+ const context = useContext(DojoContext);
+ if (!context) {
+ throw new Error('useModel must be used within a DojoSdkProvider');
+ }
+ const { useDojoStore } = context as DojoContextType<Client, Schema>;🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Find DojoContext definition and related implementations
ast-grep --pattern 'const DojoContext = createContext'
# Find context provider implementations
ast-grep --pattern 'function DojoSdkProvider'
# Look for similar context usage patterns
rg "useContext\(DojoContext\)" -A 2
# Check for existing runtime checks
rg "if.*!context" --type ts
Length of output: 3687
| // @ts-expect-error Since we c | ||
| // annot dynamically set context at runtime, we will get a type error. | ||
| export const DojoContext = createContext<DojoContextType>(undefined); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix incomplete comment and improve error handling.
The comment is cut off, and the type assertion to undefined needs better error handling.
-// @ts-expect-error Since we c
-// annot dynamically set context at runtime, we will get a type error.
-export const DojoContext = createContext<DojoContextType>(undefined);
+// We cannot dynamically set context at runtime, so we initialize with null
+export const DojoContext = createContext<DojoContextType<any, any> | null>(null);📝 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.
| // @ts-expect-error Since we c | |
| // annot dynamically set context at runtime, we will get a type error. | |
| export const DojoContext = createContext<DojoContextType>(undefined); | |
| // We cannot dynamically set context at runtime, so we initialize with null | |
| export const DojoContext = createContext<DojoContextType<any, any> | null>(null); |
MartianGreed
force-pushed
the
feat/add-sdk-packages
branch
from
b263d31 to
42ab8cf
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
examples/example-vite-kitchen-sink/src/main.tsx (1)
Line range hint
18-35: Consider adding error handling for SDK initialization.The SDK initialization should include proper error handling to gracefully handle failures.
async function main() { + try { const sdk = await init<SchemaType>( { client: { rpcUrl: getRpcUrl(), toriiUrl: env.VITE_TORII_URL, relayUrl: env.VITE_RELAY_URL, worldAddress: dojoConfig.manifest.world.address, }, domain: { name: "OnChainDash", revision: "1", chainId: "1", version: "1", }, }, schema ); + } catch (error) { + console.error("Failed to initialize SDK:", error); + // Consider showing a user-friendly error message or fallback UI + throw error; + }examples/example-vite-kitchen-sink/src/components/theme-switch.tsx (1)
Line range hint
89-124: Address type safety issues.There are multiple @ts-expect-error comments that should be addressed for better type safety:
- Line 89: Theme enum type casting
- Line 108: Entity ID type in query
- Lines 122-123: Theme class map access
Consider adding proper type definitions or using type guards to eliminate the need for type assertions.
♻️ Duplicate comments (7)
packages/sdk/package.json (1)
55-60:⚠️ Potential issueFix invalid version specifiers in peer dependencies.
The "catalog:" version specifier is invalid. Additionally, using a fixed version for react-query in peer dependencies could cause version conflicts.
Apply this diff to fix the version specifiers:
- "@types/react": "catalog:", - "@types/react-dom": "catalog:", + "@types/react": "^18.0.0", + "@types/react-dom": "^18.0.0", "@tanstack/react-query": "^5.62.16", - "starknet": "catalog:", - "react": "catalog:", - "react-dom": "catalog:" + "starknet": "^5.14.1", + "react": "^18.0.0", + "react-dom": "^18.0.0"packages/sdk/src/react/provider.tsx (4)
14-16: 🛠️ Refactor suggestionFix incorrect and duplicated property comments.
The comments for the properties need correction:
- Line 14: The comment incorrectly describes
configas "The Dojo client instance"- Line 16: The comment is duplicated from line 14
Apply this diff to fix the comments:
- /** The Dojo client instance */ + /** The Dojo configuration */ config: DojoConfig; - /** The Dojo client instance */ + /** The client instance returned by clientFn */ client: ReturnType<Client>;
28-30: 🛠️ Refactor suggestionFix incomplete comment and improve type safety.
The context creation has multiple issues:
- The ts-expect-error comment is incomplete
- Using undefined as the initial value is not type-safe
Apply this diff to fix these issues:
-// @ts-expect-error Since we c -// annot dynamically set context at runtime, we will get a type error. -export const DojoContext = createContext<DojoContextType>(undefined); +// We cannot dynamically set context at runtime, so we initialize with null +export const DojoContext = createContext<DojoContextType<any, any> | null>(null);
35-37: 🛠️ Refactor suggestionRemove outdated parameter description.
The
burnerManagerparameter is mentioned in the JSDoc but is not part of the function's parameters.Apply this diff to fix the documentation:
* @param props.children - Child components that will have access to the Dojo context -* @param props.burnerManager - Instance of BurnerManager for handling burner accounts * @throws {Error} If DojoProvider is used more than once in the component tree
47-47: 🛠️ Refactor suggestionReplace generic Function type with specific function signature.
Using
Functionas a type is discouraged as it's too generic and bypasses TypeScript's type checking.Apply this diff to improve type safety:
- clientFn: Function; + clientFn: (provider: DojoProvider) => ReturnType<Client>;🧰 Tools
🪛 Biome (1.9.4)
[error] 47-47: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
packages/sdk/src/sql/index.ts (2)
20-39: 🛠️ Refactor suggestionAdd input validation and improve error handling.
The current implementation needs improvements for robustness, including input validation, timeout handling, and better error handling.
Previous review comment still applies. The implementation should:
- Validate input parameters
- Add timeout handling
- Send query as JSON structure
- Improve error handling with more informative messages
59-60: 🛠️ Refactor suggestionFix type issue instead of suppressing TypeScript error.
The type compatibility issue should be fixed rather than suppressed.
🧹 Nitpick comments (9)
examples/example-vite-react-sql/src/components/playground/direction-count.tsx (2)
Line range hint
31-40: Consider adding error handling and removing ts-expect-error.The helper functions could be improved:
- Add error handling for JSON parsing
- Use type assertions or proper typing instead of ts-expect-error
function parseDirection(str: string): string { - const parsed = JSON.parse(str); + try { + const parsed = JSON.parse(str); + return Object.keys(parsed)[0]; + } catch (e) { + console.error('Failed to parse direction:', e); + return 'Unknown'; + } - return Object.keys(parsed)[0]; } function formatFn(rows: QueryResponse): DirectionCount { const directions = defaultDirectionObject; rows.forEach((r) => { const direction = parseDirection(r.direction); - // @ts-expect-error this is ok compiler - directions[direction] = r.count; + if (direction in directions) { + (directions as Record<string, number>)[direction] = r.count; + } }); return directions; }
57-60: Remove unnecessary ts-expect-error comment.The isRefetching variable is used for triggering re-renders, so the ts-expect-error comment is unnecessary.
- // @ts-expect-error it's ok if I dont use this variable compiler, react needs it - const { data: directions, isRefetching } = useToriiSQLQuery( + const { data: directions, isRefetching } = useToriiSQLQuery( DIRECTION_COUNT_QUERY, formatFn );packages/sdk/src/react/provider.tsx (1)
55-58: Consider cleanup for DojoProvider instance.The
DojoProviderinstance is created on every render but there's no cleanup mechanism. Consider moving the provider creation to a useMemo hook to prevent unnecessary re-creation and add cleanup if needed.+ const dojoProvider = useMemo( + () => new DojoProvider(dojoConfig.manifest, dojoConfig.rpcUrl), + [dojoConfig.manifest, dojoConfig.rpcUrl] + ); - const dojoProvider = new DojoProvider( - dojoConfig.manifest, - dojoConfig.rpcUrl - );packages/sdk/src/sql/index.ts (2)
41-43: Add missing documentation for useToriiSQLQuery function.The function lacks proper JSDoc documentation. Consider adding documentation that describes:
- Function purpose and usage
- Template parameters explanation
- Parameter descriptions
- Return value details
- Example usage
/** + * React hook to query Torii instance over SQL endpoint with TanStack Query integration. + * + * @template Output - The type of formatted data after processing + * @template Input - The type of raw data from SQL query + * @param {string} query - Raw SQL query + * @param {(rows: Input) => Output} formatFn - Function to format the raw data + * @param {UseQueryOptions<Output>["placeholderData"]} [defaultValue] - Default value while loading + * @param {string} [toriiUrl] - Optional Torii URL (defaults to localhost:8080) + * @returns {{ data: Output, error: Error, isPending: boolean, isRefetching: boolean }} + * + * @example + * const { data, error, isPending } = useToriiSQLQuery<EntityCount[], RawCount[]>( + * "SELECT count(*) FROM entities", + * (rows) => rows.map(row => ({ count: row.count })) + * ); */
51-51: Enhance query key for better cache management.The current query key only includes the query string. Consider adding more dimensions for better cache management:
- Include the Torii URL to prevent cache conflicts across different instances
- Add a version or namespace to facilitate cache invalidation
- queryKey: [query], + queryKey: ["torii", "sql", toriiUrl ?? "default", query],examples/example-vite-kitchen-sink/src/components/caller-counter.tsx (1)
Line range hint
34-115: Consider extracting entity subscription logic.The entity subscription logic is complex and could benefit from being extracted into a custom hook for better reusability and maintainability.
+// Create a new file: hooks/useCallerCounter.ts +import { useEffect, useState } from 'react'; +import { ParsedEntity, QueryBuilder, SDK } from "@dojoengine/sdk"; +import { useDojoSDK } from "@dojoengine/sdk/react"; +import { Subscription } from "@dojoengine/torii-wasm"; + +export function useCallerCounter(address: string) { + const [count, setCount] = useState(0); + const [sub, setSub] = useState<Subscription | null>(null); + const { sdk } = useDojoSDK<typeof setupWorld, SchemaType>(); + + // ... rest of the subscription logic + + return { count, isLoading }; +}examples/example-vite-react-sdk/src/App.tsx (3)
284-284: Remove outdated comment.The comment about passing SDK as props is outdated since the codebase has already moved to a context-based approach.
- {/* // Here sdk is passed as props but this can be done via contexts */} <HistoricalEvents />
Line range hint
35-107: Consider extracting subscription and fetching logic into custom hooks.The entity subscription and fetching logic could be better organized by:
- Creating custom hooks (e.g.,
useEntitySubscriptionanduseEntityFetching)- Centralizing error handling logic
- Reducing code duplication in error handling
This would improve maintainability and reusability.
Example refactor:
// useEntitySubscription.ts export function useEntitySubscription(sdk, account, state) { useEffect(() => { if (!account) return; const handleError = (error) => { console.error("Entity subscription error:", error); // Add centralized error handling here }; const subscription = sdk.subscribeEntityQuery({ query: buildEntityQuery(account), callback: ({ error, data }) => { if (error) { handleError(error); return; } if (data?.[0]?.entityId !== "0x0") { state.updateEntity(data[0]); } }, }); return () => subscription?.cancel(); }, [sdk, account]); } // useEntityFetching.ts export function useEntityFetching(sdk, account, state) { useEffect(() => { if (!account) return; const fetchEntities = async () => { try { await sdk.getEntities({ query: buildEntityQuery(account), callback: handleEntitiesResponse(state), }); } catch (error) { console.error("Entity fetching error:", error); // Add centralized error handling here } }; fetchEntities(); }, [sdk, account]); }
Line range hint
108-283: Consider breaking down the UI into smaller components.The UI rendering could be improved by:
- Extracting the movement controls mapping to a constant
- Creating a separate component for the entities table
- Organizing Tailwind classes using composition
This would enhance maintainability and readability.
Example refactor:
// constants.ts export const MOVEMENT_CONTROLS = [ { direction: new CairoCustomEnum({ Up: "()" }), label: "↑", col: "col-start-2" }, { direction: new CairoCustomEnum({ Left: "()" }), label: "←", col: "col-start-1" }, { direction: new CairoCustomEnum({ Right: "()" }), label: "→", col: "col-start-3" }, { direction: new CairoCustomEnum({ Down: "()" }), label: "↓", col: "col-start-2" }, ]; // EntityTable.tsx function EntityTable({ entities }) { return ( <div className="mt-8 overflow-x-auto"> <table className="w-full border-collapse border border-gray-700"> {/* ... table implementation ... */} </table> </div> ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (38)
.changeset/great-kiwis-peel.md(1 hunks)examples/example-vite-kitchen-sink/src/components/caller-counter.tsx(5 hunks)examples/example-vite-kitchen-sink/src/components/chat.tsx(2 hunks)examples/example-vite-kitchen-sink/src/components/global-counter.tsx(2 hunks)examples/example-vite-kitchen-sink/src/components/theme-switch.tsx(2 hunks)examples/example-vite-kitchen-sink/src/dojo/provider.tsx(0 hunks)examples/example-vite-kitchen-sink/src/main.tsx(2 hunks)examples/example-vite-react-sdk/src/App.tsx(2 hunks)examples/example-vite-react-sdk/src/historical-events.tsx(1 hunks)examples/example-vite-react-sdk/src/main.tsx(2 hunks)examples/example-vite-react-sdk/src/useDojo.tsx(0 hunks)examples/example-vite-react-sdk/src/useModel.tsx(0 hunks)examples/example-vite-react-sdk/src/useSystemCalls.ts(2 hunks)examples/example-vite-react-sdk/tsconfig.app.tsbuildinfo(1 hunks)examples/example-vite-react-sql/package.json(3 hunks)examples/example-vite-react-sql/src/components/playground/action.tsx(1 hunks)examples/example-vite-react-sql/src/components/playground/direction-count.tsx(4 hunks)examples/example-vite-react-sql/src/components/playground/schema.tsx(2 hunks)examples/example-vite-react-sql/src/components/playground/total-entities.tsx(2 hunks)examples/example-vite-react-sql/src/hooks/useDojoStore.ts(0 hunks)examples/example-vite-react-sql/src/hooks/usePlayerActions.ts(1 hunks)examples/example-vite-react-sql/src/hooks/useQuery.ts(0 hunks)examples/example-vite-react-sql/src/hooks/useSystemCalls.ts(2 hunks)examples/example-vite-react-sql/src/main.tsx(2 hunks)examples/example-vite-react-sql/tsconfig.app.tsbuildinfo(1 hunks)examples/example-vite-react-sql/tsconfig.node.tsbuildinfo(1 hunks)examples/example-vite-react-sql/vite.config.ts(1 hunks)packages/core/src/constants/index.ts(1 hunks)packages/sdk/package.json(2 hunks)packages/sdk/src/index.ts(0 hunks)packages/sdk/src/react/hooks.ts(3 hunks)packages/sdk/src/react/index.ts(1 hunks)packages/sdk/src/react/provider.tsx(1 hunks)packages/sdk/src/sql/index.ts(1 hunks)packages/sdk/src/state/zustand.ts(1 hunks)packages/sdk/tsconfig.json(1 hunks)packages/sdk/tsup.config.ts(1 hunks)pnpm-workspace.yaml(1 hunks)
💤 Files with no reviewable changes (6)
- examples/example-vite-react-sql/src/hooks/useDojoStore.ts
- examples/example-vite-react-sdk/src/useDojo.tsx
- examples/example-vite-react-sql/src/hooks/useQuery.ts
- examples/example-vite-kitchen-sink/src/dojo/provider.tsx
- examples/example-vite-react-sdk/src/useModel.tsx
- packages/sdk/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (22)
- examples/example-vite-react-sdk/tsconfig.app.tsbuildinfo
- packages/sdk/tsconfig.json
- packages/sdk/src/react/index.ts
- examples/example-vite-react-sql/src/components/playground/action.tsx
- .changeset/great-kiwis-peel.md
- examples/example-vite-react-sql/tsconfig.node.tsbuildinfo
- packages/sdk/tsup.config.ts
- examples/example-vite-react-sql/src/components/playground/schema.tsx
- examples/example-vite-react-sql/src/hooks/usePlayerActions.ts
- examples/example-vite-react-sql/vite.config.ts
- examples/example-vite-react-sdk/src/historical-events.tsx
- examples/example-vite-react-sql/tsconfig.app.tsbuildinfo
- pnpm-workspace.yaml
- examples/example-vite-kitchen-sink/src/components/chat.tsx
- examples/example-vite-react-sql/src/hooks/useSystemCalls.ts
- examples/example-vite-react-sdk/src/useSystemCalls.ts
- examples/example-vite-kitchen-sink/src/components/global-counter.tsx
- packages/sdk/src/react/hooks.ts
- packages/core/src/constants/index.ts
- examples/example-vite-react-sql/src/components/playground/total-entities.tsx
- packages/sdk/src/state/zustand.ts
- examples/example-vite-react-sql/package.json
🧰 Additional context used
🪛 Biome (1.9.4)
packages/sdk/src/react/provider.tsx
[error] 47-47: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: check
🔇 Additional comments (11)
packages/sdk/package.json (2)
8-30: Well-structured module configuration!The exports configuration properly handles:
- TypeScript types declaration
- Dual package hazard (CJS/ESM)
- Submodule tree-shaking
Line range hint
63-67: LGTM! Proper workspace dependency configuration.The
@dojoengine/coredependency is correctly configured using the workspace protocol, which is appropriate for monorepo setups.examples/example-vite-react-sql/src/components/playground/direction-count.tsx (2)
1-3: LGTM! Well-structured imports and type definitions.The imports correctly utilize the new SDK packages and the type definitions provide good type safety.
44-44: Consider using useDojoSDK hook instead of useContext.The component is still using context directly while the PR's objective is to transition to the new SDK hooks structure.
-const { useDojoStore } = useContext(DojoContext); +const { useDojoStore } = useDojoSDK();Also applies to: 47-47
examples/example-vite-kitchen-sink/src/main.tsx (1)
10-10: LGTM! Clean architectural improvement.The transition from context-based providers to
DojoSdkProviderimproves the architecture by:
- Consolidating context management into a single provider
- Making the SDK instance explicitly available
- Providing clear configuration through props
Also applies to: 38-46
examples/example-vite-react-sdk/src/main.tsx (1)
43-51: LGTM! Well-structured implementation.The implementation includes:
- Clean provider setup with required props
- Proper error handling at the main function level
- Clear separation of concerns
examples/example-vite-react-sql/src/main.tsx (1)
53-57: LGTM! Clean integration with router.The implementation successfully combines the new SDK provider with router setup.
examples/example-vite-kitchen-sink/src/components/caller-counter.tsx (1)
58-61: Add error boundary for SDK initialization.The SDK usage should be wrapped in proper error handling to gracefully handle initialization failures.
examples/example-vite-kitchen-sink/src/components/theme-switch.tsx (1)
51-54: LGTM! Clean SDK integration.The destructuring of SDK hooks with proper type parameters improves type safety.
examples/example-vite-react-sdk/src/App.tsx (2)
11-11: Great architectural improvement!The shift from prop-based SDK to context-based SDK access using
useDojoSDKhook reduces prop drilling and follows React best practices. The addition of theuseModelhook provides a more idiomatic way to access model data.Also applies to: 19-19
20-20: Clean SDK initialization pattern!The use of
useDojoSDKhook with destructuring provides a clean and maintainable way to access the store, client, and SDK instance.
| queryKey: [query], | ||
| queryFn: async () => { | ||
| return await queryTorii( | ||
| toriiUrl ?? "http://127.0.0.1:8080", |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Move hardcoded URL to configuration.
The default Torii URL should not be hardcoded. Consider:
- Moving it to a configuration file
- Making it a required parameter
- Using an environment variable
- toriiUrl ?? "http://127.0.0.1:8080",
+ toriiUrl ?? process.env.TORII_URL ?? "http://127.0.0.1:8080",📝 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.
| toriiUrl ?? "http://127.0.0.1:8080", | |
| toriiUrl ?? process.env.TORII_URL ?? "http://127.0.0.1:8080", |
Summary by CodeRabbit
Release Notes
New Features
@dojoengine/sdk/state,@dojoengine/sdk/react, and@dojoengine/sdk/sqlDojoSdkProviderfor centralized SDK configurationuseDojoSDKhook for simplified SDK accessBreaking Changes
useDojoDbwithuseDojoSDKDojoContext,useDojo, anduseDojoStorehooksImprovements
useToriiSQLQueryDependency Updates