feat: draft zustand state management system #280
Conversation
WalkthroughThe pull request introduces updates to the Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
💤 Files not reviewed due to no reviewable changes (1)
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: 9
🧹 Outside diff range and nitpick comments (8)
examples/example-vite-react-sdk/package.json (6)
16-16: LGTM: Addition of @types/uuidThe addition of @types/uuid is appropriate for introducing UUID functionality, which may be necessary for the new state management system.
Consider pinning the version to ensure consistent builds across environments:
- "@types/uuid": "^10.0.0", + "@types/uuid": "10.0.0",
17-17: LGTM: Addition of immerThe addition of immer aligns perfectly with the PR objectives of implementing Immer for state management. This library will facilitate working with immutable state in a more convenient way.
Consider pinning the version to ensure consistent builds across environments:
- "immer": "^10.1.1", + "immer": "10.1.1",
20-20: LGTM: Addition of uuidThe addition of uuid is consistent with the earlier addition of @types/uuid and supports the implementation of the new state management system.
Consider pinning the version to ensure consistent builds across environments:
- "uuid": "^10.0.0", + "uuid": "10.0.0",
22-22: Unchanged vite-plugin-wasm dependencyThe vite-plugin-wasm dependency remains unchanged, which is fine as it's not directly related to the state management changes in this PR.
Consider pinning the version to ensure consistent builds across environments:
- "vite-plugin-wasm": "^3.3.0", + "vite-plugin-wasm": "3.3.0",
23-23: LGTM: Addition of zustandThe addition of zustand aligns perfectly with the PR objectives of implementing Zustand for state management. This library will provide a small, fast, and scalable state management solution for the project.
Consider pinning the version to ensure consistent builds across environments:
- "zustand": "^4.5.5" + "zustand": "4.5.5"
13-23: Summary: Dependencies updated to support new state management systemThe changes to the package.json file align well with the PR objectives of implementing Zustand and Immer as a state management system. The key points are:
- Shift to workspace dependencies for @dojoengine packages, supporting a monorepo structure.
- Addition of Zustand and Immer for state management.
- Introduction of UUID functionality, potentially for unique state identifiers.
These changes provide a solid foundation for the new state management system. However, there are a couple of points to consider:
- The state management implementation is still in the examples directory. As per the PR objectives, it should be moved to the SDK.
- Testing and performance checks are needed to ensure the new state management system functions correctly and efficiently.
To complete this PR:
- Move the state management implementation from the examples directory to the SDK.
- Implement comprehensive tests for the new state management system.
- Conduct performance checks to ensure efficiency.
Once these tasks are completed, the PR will be ready for final review and merge.
examples/example-vite-react-sdk/src/state.ts (1)
43-47: Optimize entity updates insetEntitiesmethodIn the
setEntitiesmethod, you're individually updating entities in a loop, which can be optimized:set((state) => { entities.forEach((entity) => { state.entities[entity.entityId] = entity; }); });Consider using
Object.assignto batch the updates for better performance:set((state) => { - entities.forEach((entity) => { - state.entities[entity.entityId] = entity; - }); + Object.assign( + state.entities, + Object.fromEntries(entities.map((entity) => [entity.entityId, entity])) + ); });This reduces the number of iterations and can lead to more efficient state updates.
examples/example-vite-react-sdk/src/App.tsx (1)
107-107: Remove duplicate console log statementsThe log message
console.log("Updating entities...");appears both in thetryblock and in thefinallyblock. This could lead to redundant logs. Consider removing one of them to avoid confusion.Also applies to: 112-112
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
🔇 Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
- examples/example-vite-react-sdk/package.json (1 hunks)
- examples/example-vite-react-sdk/src/App.tsx (4 hunks)
- examples/example-vite-react-sdk/src/state.ts (1 hunks)
🔇 Additional comments not posted (3)
examples/example-vite-react-sdk/package.json (2)
13-13: LGTM: Workspace dependency for @dojoengine/coreThe change to use a workspace dependency for @dojoengine/core is appropriate. This aligns with the project's move towards a monorepo structure and will facilitate easier development and testing of interdependent packages.
15-15: LGTM: Workspace dependency for @dojoengine/torii-wasmThe change to use a workspace dependency for @dojoengine/torii-wasm is consistent with the previous change and supports the monorepo structure.
examples/example-vite-react-sdk/src/state.ts (1)
50-53: Consistency in updating entitiesIn the
updateEntitymethod, you're updating an entity directly:set((state) => { state.entities[entity.entityId] = entity; });This is consistent with how entities are managed in the state.
| set((state) => applyPatches(state, transaction.inversePatches)); | ||
| set((state) => { | ||
| delete state.pendingTransactions[transactionId]; | ||
| }); | ||
| } |
There was a problem hiding this comment.
Correct state mutation when reverting optimistic updates
In revertOptimisticUpdate, you're using applyPatches within set as follows:
set((state) => applyPatches(state, transaction.inversePatches));However, applyPatches returns a new state and does not mutate the draft state in place. Since Immer's producer expects mutations on the draft state, this could lead to unexpected behavior.
Modify the set function to properly apply patches to the draft state:
- set((state) => applyPatches(state, transaction.inversePatches));
+ set((state) => {
+ const patchedState = applyPatches(state, transaction.inversePatches);
+ Object.assign(state, patchedState);
+ });This ensures that the state is correctly updated within the Immer producer. Additionally, you can combine the subsequent set call to remove the transaction entry:
- set((state) => {
- delete state.pendingTransactions[transactionId];
- });
+ set((state) => {
+ const patchedState = applyPatches(state, transaction.inversePatches);
+ Object.assign(state, patchedState);
+ delete state.pendingTransactions[transactionId];
+ });| subscribeToEntity: (entityId, listener): (() => void) => { | ||
| const unsubscribe: () => void = useGameState.subscribe((state) => { | ||
| const entity = state.entities[entityId]; | ||
| listener(entity); | ||
| }); | ||
| return unsubscribe; | ||
| }, |
There was a problem hiding this comment.
Avoid using useGameState hook inside store definition
The useGameState hook should not be used inside the store definition, as it violates React's rules of hooks and can lead to issues. In the subscribeToEntity method, you're using useGameState.subscribe within the store:
const unsubscribe: () => void = useGameState.subscribe((state) => {
const entity = state.entities[entityId];
listener(entity);
});To fix this, you can access the store's subscribe method outside the store definition. Remove subscribeToEntity from the store and define it separately:
export const subscribeToEntity = (
entityId: string,
listener: (entity: ParsedEntity<Schema> | undefined) => void
): () => void => {
const unsubscribe = useGameState.subscribe(
(state) => state.entities[entityId],
listener
);
return unsubscribe;
};By doing this, you avoid using the hook inside the store and adhere to best practices.
| waitForEntityChange: (entityId, predicate, timeout = 6000) => { | ||
| return new Promise<ParsedEntity<Schema> | undefined>( | ||
| (resolve, reject) => { | ||
| const unsubscribe = useGameState.subscribe((state) => { | ||
| const entity = state.entities[entityId]; | ||
| if (predicate(entity)) { | ||
| clearTimeout(timer); | ||
| unsubscribe(); | ||
| resolve(entity); | ||
| } | ||
| }); | ||
|
|
||
| const timer = setTimeout(() => { | ||
| unsubscribe(); | ||
| reject( | ||
| new Error( | ||
| `waitForEntityChange: Timeout of ${timeout}ms exceeded` | ||
| ) | ||
| ); | ||
| }, timeout); | ||
| } | ||
| ); | ||
| }, |
There was a problem hiding this comment.
Refactor waitForEntityChange to prevent memory leaks and misuse of hooks
Similar to the previous issue, waitForEntityChange uses useGameState.subscribe inside the store, which should be avoided. Additionally, if the promise is neither resolved nor rejected, it could lead to a memory leak due to the lingering subscription.
Refactor waitForEntityChange to be defined outside the store:
export const waitForEntityChange = (
entityId: string,
predicate: (entity: ParsedEntity<Schema> | undefined) => boolean,
timeout = 6000
): Promise<ParsedEntity<Schema> | undefined> => {
return new Promise<ParsedEntity<Schema> | undefined>((resolve, reject) => {
const unsubscribe = useGameState.subscribe(
(state) => state.entities[entityId],
(entity) => {
if (predicate(entity)) {
clearTimeout(timer);
unsubscribe();
resolve(entity);
}
}
);
const timer = setTimeout(() => {
unsubscribe();
reject(new Error(`waitForEntityChange: Timeout of ${timeout}ms exceeded`));
}, timeout);
});
};This ensures that you're adhering to React's hooks rules and preventing potential memory leaks by managing the subscription properly.
| applyOptimisticUpdate: (transactionId, updateFn) => { | ||
| const currentState = get(); | ||
| const [nextState, patches, inversePatches] = produceWithPatches( | ||
| currentState, | ||
| (draft) => { | ||
| updateFn(draft); | ||
| } | ||
| ); | ||
|
|
||
| set(() => nextState); | ||
|
|
||
| set((state) => { | ||
| state.pendingTransactions[transactionId] = { | ||
| transactionId, | ||
| patches, | ||
| inversePatches, | ||
| }; | ||
| }); | ||
| }, |
There was a problem hiding this comment.
Ensure correct state updates in applyOptimisticUpdate
In the applyOptimisticUpdate function, you're replacing the entire state with nextState using set(() => nextState);. When using the Immer middleware with Zustand, it's recommended to mutate the draft state rather than replacing it to maintain reactivity and avoid potential issues.
Consider merging the changes into the current state by mutating the draft:
- set(() => nextState);
+ set((state) => {
+ Object.assign(state, nextState);
+ });This way, you update the state correctly within the Immer producer. Also, you can combine the two set calls into one for efficiency:
- set((state) => {
- state.pendingTransactions[transactionId] = {
- transactionId,
- patches,
- inversePatches,
- };
- });
+ set((state) => {
+ Object.assign(state, nextState);
+ state.pendingTransactions[transactionId] = {
+ transactionId,
+ patches,
+ inversePatches,
+ };
+ });📝 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.
| applyOptimisticUpdate: (transactionId, updateFn) => { | |
| const currentState = get(); | |
| const [nextState, patches, inversePatches] = produceWithPatches( | |
| currentState, | |
| (draft) => { | |
| updateFn(draft); | |
| } | |
| ); | |
| set(() => nextState); | |
| set((state) => { | |
| state.pendingTransactions[transactionId] = { | |
| transactionId, | |
| patches, | |
| inversePatches, | |
| }; | |
| }); | |
| }, | |
| applyOptimisticUpdate: (transactionId, updateFn) => { | |
| const currentState = get(); | |
| const [nextState, patches, inversePatches] = produceWithPatches( | |
| currentState, | |
| (draft) => { | |
| updateFn(draft); | |
| } | |
| ); | |
| set((state) => { | |
| Object.assign(state, nextState); | |
| state.pendingTransactions[transactionId] = { | |
| transactionId, | |
| patches, | |
| inversePatches, | |
| }; | |
| }); | |
| }, |
| const state = useGameState((state) => state); | ||
| const entities = useGameState((state) => state.entities); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify state usage to prevent unnecessary re-renders
You are accessing the entire state and state.entities separately using useGameState. This can lead to unnecessary re-renders. Since you have the entire state with state, you can use state.entities directly without needing to select it separately.
| const entityId = | ||
| "0x571368d35c8fe136adf81eecf96a72859c43de7efd8fdd3d6f0d17e308df984"; |
There was a problem hiding this comment.
Avoid hardcoding the entityId
Hardcoding the entityId reduces flexibility and may cause issues if the ID changes. Consider passing the entityId as a parameter or obtaining it dynamically based on the application's context.
| const transactionId = uuidv4(); | ||
|
|
||
| state.applyOptimisticUpdate(transactionId, (draft) => { | ||
| draft.entities[entityId].models.dojo_starter.Moves!.remaining = 10; |
There was a problem hiding this comment.
Ensure entity exists before applying optimistic update
When applying the optimistic update, ensure that draft.entities[entityId] exists to prevent potential runtime errors if the entity is undefined.
Suggested fix:
state.applyOptimisticUpdate(transactionId, (draft) => {
+ if (draft.entities[entityId]) {
draft.entities[entityId].models.dojo_starter.Moves!.remaining = 10;
+ }
});📝 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.
| draft.entities[entityId].models.dojo_starter.Moves!.remaining = 10; | |
| state.applyOptimisticUpdate(transactionId, (draft) => { | |
| if (draft.entities[entityId]) { | |
| draft.entities[entityId].models.dojo_starter.Moves!.remaining = 10; | |
| } | |
| }); |
| // Define your specific condition here | ||
| return entity?.models.dojo_starter.Moves?.can_move === true; |
There was a problem hiding this comment.
Define the specific condition for waitForEntityChange
The condition in waitForEntityChange is currently a placeholder. Please replace it with the actual condition that determines when the entity has been updated.
Example:
return entity?.models.dojo_starter.Moves?.can_move === true;
- // Define your specific condition here
+ // Condition: Checks if the 'can_move' flag is trueCommittable suggestion was skipped due to low confidence.
| } finally { | ||
| console.log("Updating entities..."); | ||
| state.confirmTransaction(transactionId); | ||
| } |
There was a problem hiding this comment.
Review transaction confirmation placement
Calling state.confirmTransaction(transactionId); in the finally block means it executes regardless of success or failure. If the transaction fails and is reverted, confirming it may not be appropriate. Consider moving the confirmation inside the try block after the update succeeds.
Suggested change:
} catch (error) {
console.error("Error updating entities:", error);
state.revertOptimisticUpdate(transactionId);
- } finally {
+ } finally {
console.log("Updating entities...");
- state.confirmTransaction(transactionId);
}
+ state.confirmTransaction(transactionId);📝 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.
| } finally { | |
| console.log("Updating entities..."); | |
| state.confirmTransaction(transactionId); | |
| } | |
| } catch (error) { | |
| console.error("Error updating entities:", error); | |
| state.revertOptimisticUpdate(transactionId); | |
| } finally { | |
| console.log("Updating entities..."); | |
| } | |
| state.confirmTransaction(transactionId); |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (3)
packages/sdk/src/state/zustand.ts (3)
23-43: Add documentation to theGameStateinterface methodsConsider adding JSDoc comments to each method within the
GameStateinterface. This will enhance code readability and help other developers understand the purpose and usage of each method.
45-50: Expand the documentation forcreateDojoStoreThe JSDoc comment for
createDojoStoreis a good start. Providing more detailed descriptions of the parameters and the returned store can further improve clarity for users of this function.
110-114: Consolidate transaction cleanup logicBoth
revertOptimisticUpdateandconfirmTransactionmethods delete the transaction frompendingTransactions. Consider creating a helper function to handle this cleanup to reduce code duplication.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
🔇 Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
- examples/example-vite-react-sdk/src/App.tsx (4 hunks)
- packages/sdk/package.json (1 hunks)
- packages/sdk/src/index.ts (1 hunks)
- packages/sdk/src/state/index.ts (1 hunks)
- packages/sdk/src/state/zustand.ts (1 hunks)
- packages/sdk/src/types.ts (1 hunks)
🔇 Additional comments not posted (16)
packages/sdk/src/state/index.ts (1)
1-1: LGTM! Verify Zustand implementation and address remaining tasks.The export statement aligns well with the PR objective of implementing Zustand as part of the state management system. It provides a clean, modular approach to organizing the state management functionalities.
To ensure the Zustand implementation is complete, please run the following verification script:
This script will help verify the Zustand implementation and check for potential Immer integration.
As a reminder, please ensure you complete the remaining tasks mentioned in the PR objectives:
- Implement Immer alongside Zustand if not already done.
- Conduct testing and performance checks for the new state management system.
If you need any assistance with implementing tests or performance checks, please let me know, and I'd be happy to help or create a GitHub issue to track these tasks.
packages/sdk/package.json (1)
39-39: LGTM: Addition of Immer dependency.The addition of Immer (version ^10.1.1) as a dependency aligns with the PR objective of implementing a new state management system. Immer is a great choice for working with immutable state in a more convenient way.
However, I noticed that while Immer has been added, Zustand is not visible in this diff. Could you please verify if Zustand is already a dependency, or if it needs to be added as well? The PR objectives mention implementing both Zustand and Immer.
✅ Verification successful
Verified: Both Zustand and Immer dependencies are present.
The
packages/sdk/package.jsonnow includes both Zustand (^4.5.5) and Immer (^10.1.1) as dependencies, fulfilling the PR objective of implementing a new state management system with these libraries.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if Zustand is already a dependency in the package.json file # Test: Search for Zustand in the package.json file grep -n "zustand" packages/sdk/package.jsonLength of output: 74
examples/example-vite-react-sdk/src/App.tsx (11)
1-8: Improved state management and type safetyThe introduction of
createDojoStoreand the customuseGameStatehook centralizes state management, which is a good practice. The use of TypeScript with theSchematype enhances type safety. These changes lay a solid foundation for more robust state handling in the application.
11-12: Simplify state usage to prevent unnecessary re-rendersThe current implementation still accesses the entire state and
state.entitiesseparately usinguseGameState. This can lead to unnecessary re-renders. Since you have the entire state withstate, you can usestate.entitiesdirectly without needing to select it separately.Consider refactoring to:
const state = useGameState();And then use
state.entitieswhere needed.
36-36: Improved entity update logicThe simplification of entity update logic by directly using
state.setEntitiesis a positive change. This approach reduces code complexity, aligns well with the centralized state management, and potentially reduces the risk of inconsistencies in state updates.Also applies to: 74-74
86-116: Approve: Introduction of optimistic updatesThe addition of the
optimisticUpdatefunction is a positive change. It improves user experience by allowing immediate UI updates while waiting for server confirmation.
87-88: Avoid hardcoding theentityIdHardcoding the
entityIdreduces flexibility and may cause issues if the ID changes. Consider passing theentityIdas a parameter or obtaining it dynamically based on the application's context.
92-94: Ensure entity exists before applying optimistic updateWhen applying the optimistic update, ensure that
draft.entities[entityId]exists to prevent potential runtime errors if the entity is undefined.Suggested fix:
state.applyOptimisticUpdate(transactionId, (draft) => { + if (draft.entities[entityId]?.models.dojo_starter.Moves) { draft.entities[entityId].models.dojo_starter.Moves!.remaining = 10; + } });
112-115: Review transaction confirmation placementCalling
state.confirmTransaction(transactionId);in thefinallyblock means it executes regardless of success or failure. If the transaction fails and is reverted, confirming it may not be appropriate. Consider moving the confirmation inside thetryblock after the update succeeds.Suggested change:
} catch (error) { console.error("Error updating entities:", error); state.revertOptimisticUpdate(transactionId); - } finally { + } finally { console.log("Updating entities..."); - state.confirmTransaction(transactionId); } + state.confirmTransaction(transactionId);
121-124: Improved UI interaction and entity renderingThe addition of the update button provides a clear way to test the optimistic update functionality, enhancing the development and testing process. The change in entity rendering from array mapping to object entries reflects the new state structure, ensuring consistency with the updated state management approach.
130-131: Improved handling of potentially undefined valuesThe use of optional chaining (
?.) in accessingentity.models.dojo_starter.Position?.vec?.xis a good practice. It safely handles potentially undefined values, preventing runtime errors. The fallback to "N/A" ensures a consistent UI even when data is missing.
133-134: Consistent improvement in Y coordinate renderingThe update to the Y coordinate rendering is consistent with the X coordinate change. It uses optional chaining and maintains the "N/A" fallback, ensuring safe access to potentially undefined values and a consistent UI.
Line range hint
1-150: Overall assessment: Successful implementation of new state management systemThis PR successfully introduces a proof of concept for implementing Zustand and Immer as a state management system, including features for optimistic rendering. The changes align well with the stated objectives and significantly improve the application's state management approach.
Key improvements:
- Centralized state management using
createDojoStoreanduseGameState.- Implementation of optimistic updates, enhancing user experience.
- Simplified entity update logic.
- Improved rendering of entity data with better handling of undefined values.
Areas for further improvement:
- Avoid hardcoding the
entityIdin the optimistic update function.- Ensure proper error handling and entity existence checks in the optimistic update function.
- Review the placement of transaction confirmation in the optimistic update process.
Overall, this PR represents a significant step forward in the project's state management capabilities. Once the remaining issues are addressed, it will provide a solid foundation for further development.
packages/sdk/src/state/zustand.ts (3)
57-64: Ensure entity IDs are defined before assignmentIn the
setEntitiesmethod, while assigning entities to the state, consider adding a check to ensure thatentity.entityIdis defined to prevent potential runtime errors if an entity lacks an ID.
121-145: Ensure timely cleanup of subscriptions inwaitForEntityChangeIn
waitForEntityChange, subscriptions are cleaned up on both resolve and reject paths, which is good. Verify that there are no edge cases where the subscription might not be properly unsubscribed, to prevent memory leaks.
51-150: Well-structured implementation of Zustand store with ImmerThe
createDojoStorefunction is effectively implemented, leveraging Zustand and Immer for robust state management. The approach to handling optimistic updates and transactions is sound and aligns with best practices.
| @@ -8,6 +8,7 @@ import { SchemaType, SDK, UnionOfModelData } from "./types"; | |||
| import { Account, Signature, StarknetDomain, TypedData } from "starknet"; | |||
|
|
|||
| export * from "./types"; | |||
| export * from "./state"; | |||
There was a problem hiding this comment.
💡 Codebase verification
Issue Detected: Missing state.ts Module in Export Statement.
The export statement export * from "./state"; in packages/sdk/src/index.ts references a non-existent state.ts file, which will lead to import errors for SDK consumers.
packages/sdk/src/state.tsdoes not exist.
🔗 Analysis chain
LGTM! Verify for potential naming conflicts.
The new export statement is consistent with the PR objective of implementing a state management system. It appropriately exposes the state module's functionality to SDK consumers.
To ensure there are no naming conflicts, please run the following script:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential naming conflicts between the new state exports and existing exports
# Get existing exports
existing_exports=$(rg -o --no-filename 'export \{.*?\}' packages/sdk/src/index.ts | tr -d '{}' | tr ',' '\n' | sed 's/^ *//g')
# Get exports from the state module
state_exports=$(rg -o --no-filename 'export .*' packages/sdk/src/state.ts | sed 's/export //g')
# Check for conflicts
conflicts=$(comm -12 <(echo "$existing_exports" | sort) <(echo "$state_exports" | sort))
if [ -n "$conflicts" ]; then
echo "Potential naming conflicts detected:"
echo "$conflicts"
else
echo "No naming conflicts detected."
fi
Length of output: 543
| updateEntity: (entity: Partial<ParsedEntity<T>>) => { | ||
| set((state: Draft<GameState<T>>) => { | ||
| if ( | ||
| entity.entityId && | ||
| state.entities[entity.entityId] | ||
| ) { | ||
| Object.assign( | ||
| state.entities[entity.entityId], | ||
| entity | ||
| ); | ||
| } | ||
| }); | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle the case where an entity does not exist in the state
In the updateEntity method, if the entity.entityId does not exist in state.entities, the method currently does nothing. Consider handling this case explicitly, perhaps by logging a warning or creating a new entry, depending on the desired behavior.
| revertOptimisticUpdate: (transactionId) => { | ||
| const transaction = | ||
| get().pendingTransactions[transactionId]; | ||
| if (transaction) { | ||
| set((state: Draft<GameState<T>>) => | ||
| applyPatches(state, transaction.inversePatches) | ||
| ); | ||
| set((state: Draft<GameState<T>>) => { | ||
| delete state.pendingTransactions[transactionId]; | ||
| }); | ||
| } | ||
| }, |
There was a problem hiding this comment.
Add error handling for missing transactions in revertOptimisticUpdate
When a transaction ID is not found in pendingTransactions, the revertOptimisticUpdate method silently does nothing. Consider adding error handling or logging to notify when a transaction ID is invalid or missing.
| subscribeToEntity: (entityId, listener): (() => void) => { | ||
| return useStore.subscribe((state) => { | ||
| const entity = state.entities[entityId]; | ||
| listener(entity); | ||
| }); | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clarify the use of useStore within store methods
The methods subscribeToEntity and waitForEntityChange are using useStore inside the store definition. While this works due to closures, it might be clearer to access the store via get() or restructure the code to avoid potential confusion.
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
- examples/example-vite-react-sdk/package.json (1 hunks)
- examples/example-vite-react-sdk/src/App.tsx (4 hunks)
- packages/sdk/package.json (2 hunks)
- packages/sdk/src/types.ts (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/example-vite-react-sdk/package.json
🔇 Additional comments (15)
packages/sdk/package.json (5)
Line range hint
1-65: Overall assessment of package.json changesThe changes to
package.jsonalign well with the PR objectives:
- The addition of
immerand the presence ofzustandsupport the implementation of the new state management system.- The addition of
@dojoengine/create-burnersuggests progress on the new SDK structure.- The addition of
@rollup/plugin-commonjs, while not explicitly mentioned in the PR objectives, likely improves the build process.These changes seem to be moving in the right direction for implementing the proof of concept for Zustand and Immer as a state management system within the new SDK structure.
41-41: New dependency added: immerThe addition of
immeras a dependency aligns with the PR objective of implementing Immer for state management. Immer is a library that allows you to work with immutable state in a more convenient way.Let's verify the usage of Immer in the codebase:
#!/bin/bash # Search for imports of immer rg --type js --type ts "from ['\"]immer['\"]"
42-44: Verify presence of zustandThe PR objectives mention implementing Zustand for state management. I can see that
zustandis already present in the dependencies list, which aligns with the PR objectives.Let's verify the usage of Zustand in the codebase:
#!/bin/bash # Search for imports of zustand rg --type js --type ts "from ['\"]zustand['\"]"
25-25: New development dependency added: @rollup/plugin-commonjsThe addition of
@rollup/plugin-commonjsas a development dependency suggests changes in the build process. This plugin allows external dependencies to be bundled even if they're in CommonJS format.Let's verify if this plugin is being used in the build configuration:
39-39: New dependency added: @dojoengine/create-burnerThe addition of
@dojoengine/create-burneras a workspace dependency aligns with the PR objective of implementing the new SDK structure. This package likely provides functionality for creating burner wallets within the Dojo ecosystem.Let's verify the usage of this new dependency:
examples/example-vite-react-sdk/src/App.tsx (7)
1-8: Improved state management with SDK integrationThe introduction of
createDojoStorefrom the SDK and the creation ofuseDojoStorehook represent a significant improvement in state management. This centralized approach will likely lead to more maintainable and efficient state handling throughout the application.The addition of the UUID library suggests an intention to use unique identifiers, which is a good practice for tracking transactions or other unique elements in the application.
36-36: Streamlined entity updatesThe use of
state.setEntitiesfor both subscription updates and initial data fetching is a good improvement. This approach:
- Simplifies the code by removing the need for separate state update logic.
- Ensures consistency in how entities are updated throughout the application.
- Leverages the centralized state management effectively.
This change aligns well with the new state management system and should make the code more maintainable.
Also applies to: 74-74
121-121: User interface for testing optimistic updatesThe addition of the update button is a good way to demonstrate and test the new optimistic update functionality. This allows for easy triggering of the
optimisticUpdatefunction, which is crucial for verifying its behavior during development and testing.
122-124: Updated entity rendering logicThe change from mapping over an array to using
Object.entries(entities)correctly reflects the new structure of entities as an object. This adjustment ensures that the rendering logic is consistent with the updated state management approach. It's a necessary and well-implemented change.
130-134: Improved property access with optional chainingThe use of optional chaining (
?.) when accessing entity properties is an excellent improvement. This change:
- Enhances the robustness of the code by safely handling potentially undefined properties.
- Prevents runtime errors that could occur if an entity or its properties are not yet loaded.
- Improves the overall reliability of the rendering logic.
This is a good practice and should be consistently applied throughout the codebase.
Line range hint
1-153: Overall assessment of changesThe changes in this file represent a significant improvement in state management and introduce valuable new features:
- The transition to a centralized state management using the SDK and
useDojoStoreis a positive architectural change.- The implementation of optimistic updates enhances the user experience and demonstrates advanced state handling.
- The rendering logic has been updated to reflect the new state structure, improving consistency.
While these changes are generally well-implemented, there are a few areas for further refinement:
- Optimize state selection to prevent unnecessary re-renders.
- Address the hardcoded
entityIdand placeholder condition in the optimistic update function.- Refine the transaction confirmation logic in the optimistic update.
Overall, these changes represent a substantial step forward in the application's architecture and functionality. With the suggested refinements, the code will be more robust, flexible, and maintainable.
86-116:⚠️ Potential issueRobust optimistic update implementation with areas for improvement
The
optimisticUpdatefunction is a significant addition that implements a robust optimistic update pattern. This is a valuable feature for improving perceived performance. However, there are a few areas that need attention:
The
entityIdis hardcoded (lines 87-88), which reduces flexibility. Consider making this dynamic or parameterized.The condition in
waitForEntityChange(lines 101-102) is still a placeholder. Please replace it with the actual condition for determining when the entity has been updated.The transaction confirmation in the
finallyblock (line 114) may not be appropriate if the transaction fails. Consider moving this inside thetryblock after the update succeeds.Consider adding more detailed error handling and potentially a way to notify the user of the update status.
packages/sdk/src/types.ts (3)
359-364: LGTM: Well-structuredBurnertypeThe
Burnertype is well-defined and provides essential information about a burner account. The use of optional fields formasterAccountandaccountIndexallows for flexibility in different scenarios.
366-371: LGTM: Well-definedBurnerManagerOptionsinterfaceThe
BurnerManagerOptionsinterface is well-structured and includes all necessary options for managing burner accounts. The use of required fields ensures that all essential configuration options are provided when initializing a burner manager.
373-389: 🛠️ Refactor suggestionSecurity considerations for clipboard operations
The
copyToClipboardandapplyFromClipboardmethods might have security implications. Copying sensitive account information to the clipboard could potentially expose it to other applications. Consider adding warnings or confirmations before performing these operations, and ensure that sensitive data is cleared from the clipboard after a short period.Additionally, the
checkIsDeployedmethod accepts an optionaldeployTxparameter. If the deployment status can be checked using just the address, consider removing this parameter to simplify the method signature:checkIsDeployed: (address: string) => Promise<boolean>;This change would make the method more straightforward and potentially more efficient if the
deployTxis not necessary for the check.
| const state = useDojoStore((state) => state); | ||
| const entities = useDojoStore((state) => state.entities); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider optimizing state selection
While using useDojoStore for state management is a good approach, selecting the entire state on line 11 might lead to unnecessary re-renders. Consider selecting only the specific parts of the state that this component needs.
Example:
const { entities, someOtherNeededState } = useDojoStore(state => ({
entities: state.entities,
someOtherNeededState: state.someOtherNeededState
}));This way, the component will only re-render when these specific parts of the state change.
| [M in keyof T[K]]?: T[K][M] extends object | ||
| ? Partial<T[K][M]> | ||
| : T[K][M]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refine type check for better type safety
The current implementation applies Partial<T[K][M]> when T[K][M] extends object. This includes arrays and functions, which might not be the intended behavior. To improve type safety, consider refining the type check to target only plain object types:
[M in keyof T[K]]?: T[K][M] extends object
? T[K][M] extends Record<string, any> ? Partial<T[K][M]> : T[K][M]
: T[K][M];This change ensures that Partial is applied only to plain objects, excluding arrays and functions.
| export type BurnerStorage = { | ||
| [address: string]: BurnerRecord; | ||
| }; | ||
|
|
||
| export type BurnerRecord = { | ||
| chainId: string; | ||
| privateKey: string; | ||
| publicKey: string; | ||
| deployTx: string; | ||
| masterAccount: string; | ||
| active: boolean; | ||
| accountIndex?: number; | ||
| metadata?: any; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a more specific type for metadata
The BurnerRecord type includes a metadata field of type any. While this provides flexibility, it might be too permissive and could lead to type-related issues in the future. Consider defining a more specific type for metadata if possible, or use Record<string, unknown> as a safer alternative to any.
Example:
export type BurnerRecord = {
// ... other fields
metadata?: Record<string, unknown>;
};This change would provide better type safety while still allowing for flexible metadata.
| export interface BurnerCreateOptions { | ||
| secret?: string; | ||
| index?: number; | ||
| metadata?: any; | ||
| prefundedAmount?: string; | ||
| maxFee?: number; | ||
| } | ||
|
|
||
| export interface BurnerKeys { | ||
| privateKey: string; | ||
| publicKey: string; | ||
| address: string; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add security note for handling secret
The BurnerCreateOptions interface includes a secret field, which is likely to contain sensitive information. Consider adding a comment to emphasize the importance of handling this field securely:
export interface BurnerCreateOptions {
/**
* A secret value used to generate the burner account.
* IMPORTANT: This value should be treated as sensitive information
* and never logged or stored in plain text.
*/
secret?: string;
// ... other fields
}This comment will remind developers to handle the secret with appropriate security measures.
| export type Predeployed = Burner & { name?: string }; | ||
|
|
||
| export type PredeployedStorage = { | ||
| [address: string]: PredeployedAccount; | ||
| }; | ||
|
|
||
| export interface PredeployedManagerOptions { | ||
| rpcProvider: RpcProvider; | ||
| predeployedAccounts: PredeployedAccount[]; | ||
| } | ||
|
|
||
| export type PredeployedAccount = { | ||
| name?: string; | ||
| address: string; | ||
| privateKey: string; | ||
| active: boolean; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add security note for handling privateKey in PredeployedAccount
The PredeployedAccount type includes a privateKey field, which contains highly sensitive information. Consider adding a comment to emphasize the importance of secure handling:
export type PredeployedAccount = {
name?: string;
address: string;
/**
* SECURITY CRITICAL: This field contains the private key of the account.
* Ensure that this value is never logged, stored in plain text, or exposed
* in any way. Implement proper encryption and secure storage mechanisms
* when dealing with private keys.
*/
privateKey: string;
active: boolean;
};This comment will serve as a crucial reminder for developers to implement proper security measures when handling private keys.
There was a problem hiding this comment.
Actionable comments posted: 11
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
- packages/sdk/package.json (1 hunks)
- packages/sdk/src/example/index.ts (1 hunks)
- packages/sdk/src/tests/state.test.ts (1 hunks)
- packages/sdk/src/tests/zustand.perf.test.ts (1 hunks)
- packages/sdk/src/state/zustand.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/sdk/package.json
- packages/sdk/src/state/zustand.ts
🔇 Additional comments (3)
packages/sdk/src/__example__/index.ts (3)
26-30: LGTM: New GalaxyModel interface is well-structured and consistent.The new
GalaxyModelinterface is well-defined and follows the existing pattern of model definitions in the file. It includes thefieldOrderproperty, which is consistent with other models, maintaining a uniform structure throughout the codebase.
39-39: LGTM: MockSchemaType refactoring improves code structure.The update to the
MockSchemaTypeinterface, using the newly definedGalaxyModel, is a good refactoring. This change:
- Simplifies the interface definition
- Improves code readability and maintainability
- Reduces duplication
- Maintains consistency with how other properties in the interface are defined
This refactoring aligns well with best practices for code organization and reusability.
Line range hint
43-71: Update schema constant and verify usage of universe.galaxyWhile the interface changes look good, there are two follow-up actions required:
The
schemaconstant needs to be updated to match the newGalaxyModelinterface. Currently, it's still using the old structure.Although the
exampleUsagefunction doesn't directly use thegalaxymodel, it's important to verify that any code using theuniverse.galaxyproperty is updated if necessary.To help with the second point, you can run the following script to find any usage of
universe.galaxy:This will help identify any places where the code might need to be updated to match the new
GalaxyModelstructure.✅ Verification successful
Adjusted search for
universe.galaxyusage in TypeScript filesThe previous search command failed due to an unrecognized file type. Please run the following script to accurately locate any usage of
universe.galaxyin.tsand.tsxfiles:
Verified: No usage of
universe.galaxyfoundThe search for
universe.galaxyin TypeScript files did not return any results. This indicates that no additional updates are necessary for theuniverse.galaxyproperty.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of universe.galaxy in the codebase rg --type typescript 'universe\.galaxy'Length of output: 76
Script:
#!/bin/bash # Search for usage of universe.galaxy in TypeScript files rg 'universe\.galaxy' --glob '*.ts' --glob '*.tsx'Length of output: 52
| it("should benchmark setEntities performance", async () => { | ||
| const suite = new Benchmark.Suite(); | ||
|
|
||
| suite | ||
| .add("setEntities", () => { | ||
| useStore.getState().setEntities(mockEntities); | ||
| }) | ||
| .on("cycle", (event: any) => { | ||
| console.log(String(event.target)); | ||
| }) | ||
| .on("complete", function () { | ||
| console.log("Fastest is " + this.filter("fastest").map("name")); | ||
| }) | ||
| .run({ async: false }); | ||
|
|
||
| // Optional: Assert that setEntities completes within a reasonable time | ||
| // Example: expect(setEntitiesTime).toBeLessThan(100); // in milliseconds | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unnecessary async keyword from test function
The test function is declared with async but does not contain any await expressions or handle any asynchronous operations. Removing the async keyword can prevent potential confusion and align with best practices.
Apply this diff to remove the async keyword:
-it("should benchmark setEntities performance", async () => {
+it("should benchmark setEntities performance", () => {📝 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.
| it("should benchmark setEntities performance", async () => { | |
| const suite = new Benchmark.Suite(); | |
| suite | |
| .add("setEntities", () => { | |
| useStore.getState().setEntities(mockEntities); | |
| }) | |
| .on("cycle", (event: any) => { | |
| console.log(String(event.target)); | |
| }) | |
| .on("complete", function () { | |
| console.log("Fastest is " + this.filter("fastest").map("name")); | |
| }) | |
| .run({ async: false }); | |
| // Optional: Assert that setEntities completes within a reasonable time | |
| // Example: expect(setEntitiesTime).toBeLessThan(100); // in milliseconds | |
| }); | |
| it("should benchmark setEntities performance", () => { | |
| const suite = new Benchmark.Suite(); | |
| suite | |
| .add("setEntities", () => { | |
| useStore.getState().setEntities(mockEntities); | |
| }) | |
| .on("cycle", (event: any) => { | |
| console.log(String(event.target)); | |
| }) | |
| .on("complete", function () { | |
| console.log("Fastest is " + this.filter("fastest").map("name")); | |
| }) | |
| .run({ async: false }); | |
| // Optional: Assert that setEntities completes within a reasonable time | |
| // Example: expect(setEntitiesTime).toBeLessThan(100); // in milliseconds | |
| }); |
| it("should benchmark updateEntity performance", async () => { | ||
| // First, set entities | ||
| useStore.getState().setEntities(mockEntities); | ||
|
|
||
| const suite = new Benchmark.Suite(); | ||
|
|
||
| suite | ||
| .add("updateEntity", () => { | ||
| useStore.getState().updateEntity({ | ||
| entityId: "entity500", | ||
| models: { | ||
| world: { | ||
| player: { | ||
| name: "UpdatedPlayer500", | ||
| score: 999, | ||
| }, | ||
| }, | ||
| universe: {}, | ||
| }, | ||
| }); | ||
| }) | ||
| .on("cycle", (event: any) => { | ||
| console.log(String(event.target)); | ||
| }) | ||
| .on("complete", function () { | ||
| console.log("Fastest is " + this.filter("fastest").map("name")); | ||
| }) | ||
| .run({ async: false }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unnecessary async keyword from test function
The test function is declared with async but does not contain any await expressions or handle any asynchronous operations. Removing the async keyword can prevent potential confusion and align with best practices.
Apply this diff to remove the async keyword:
-it("should benchmark updateEntity performance", async () => {
+it("should benchmark updateEntity performance", () => {📝 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.
| it("should benchmark updateEntity performance", async () => { | |
| // First, set entities | |
| useStore.getState().setEntities(mockEntities); | |
| const suite = new Benchmark.Suite(); | |
| suite | |
| .add("updateEntity", () => { | |
| useStore.getState().updateEntity({ | |
| entityId: "entity500", | |
| models: { | |
| world: { | |
| player: { | |
| name: "UpdatedPlayer500", | |
| score: 999, | |
| }, | |
| }, | |
| universe: {}, | |
| }, | |
| }); | |
| }) | |
| .on("cycle", (event: any) => { | |
| console.log(String(event.target)); | |
| }) | |
| .on("complete", function () { | |
| console.log("Fastest is " + this.filter("fastest").map("name")); | |
| }) | |
| .run({ async: false }); | |
| }); | |
| it("should benchmark updateEntity performance", () => { | |
| // First, set entities | |
| useStore.getState().setEntities(mockEntities); | |
| const suite = new Benchmark.Suite(); | |
| suite | |
| .add("updateEntity", () => { | |
| useStore.getState().updateEntity({ | |
| entityId: "entity500", | |
| models: { | |
| world: { | |
| player: { | |
| name: "UpdatedPlayer500", | |
| score: 999, | |
| }, | |
| }, | |
| universe: {}, | |
| }, | |
| }); | |
| }) | |
| .on("cycle", (event: any) => { | |
| console.log(String(event.target)); | |
| }) | |
| .on("complete", function () { | |
| console.log("Fastest is " + this.filter("fastest").map("name")); | |
| }) | |
| .run({ async: false }); | |
| }); |
| it("should benchmark revertOptimisticUpdate performance", async () => { | ||
| // First, set entities | ||
| useStore.getState().setEntities(mockEntities); | ||
|
|
||
| // Apply an optimistic update | ||
| useStore.getState().applyOptimisticUpdate("txn_perf", (draft) => { | ||
| draft.entities["entity500"].models.world!.item!.durability = 75; | ||
| }); | ||
|
|
||
| const suite = new Benchmark.Suite(); | ||
|
|
||
| suite | ||
| .add("revertOptimisticUpdate", () => { | ||
| useStore.getState().revertOptimisticUpdate("txn_perf"); | ||
| }) | ||
| .on("cycle", (event: any) => { | ||
| console.log(String(event.target)); | ||
| }) | ||
| .on("complete", function () { | ||
| console.log("Fastest is " + this.filter("fastest").map("name")); | ||
| }) | ||
| .run({ async: false }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unnecessary async keyword from test function
The test function is declared with async but does not contain any await expressions or handle any asynchronous operations. Removing the async keyword can prevent potential confusion and align with best practices.
Apply this diff to remove the async keyword:
-it("should benchmark revertOptimisticUpdate performance", async () => {
+it("should benchmark revertOptimisticUpdate performance", () => {📝 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.
| it("should benchmark revertOptimisticUpdate performance", async () => { | |
| // First, set entities | |
| useStore.getState().setEntities(mockEntities); | |
| // Apply an optimistic update | |
| useStore.getState().applyOptimisticUpdate("txn_perf", (draft) => { | |
| draft.entities["entity500"].models.world!.item!.durability = 75; | |
| }); | |
| const suite = new Benchmark.Suite(); | |
| suite | |
| .add("revertOptimisticUpdate", () => { | |
| useStore.getState().revertOptimisticUpdate("txn_perf"); | |
| }) | |
| .on("cycle", (event: any) => { | |
| console.log(String(event.target)); | |
| }) | |
| .on("complete", function () { | |
| console.log("Fastest is " + this.filter("fastest").map("name")); | |
| }) | |
| .run({ async: false }); | |
| }); | |
| it("should benchmark revertOptimisticUpdate performance", () => { | |
| // First, set entities | |
| useStore.getState().setEntities(mockEntities); | |
| // Apply an optimistic update | |
| useStore.getState().applyOptimisticUpdate("txn_perf", (draft) => { | |
| draft.entities["entity500"].models.world!.item!.durability = 75; | |
| }); | |
| const suite = new Benchmark.Suite(); | |
| suite | |
| .add("revertOptimisticUpdate", () => { | |
| useStore.getState().revertOptimisticUpdate("txn_perf"); | |
| }) | |
| .on("cycle", (event: any) => { | |
| console.log(String(event.target)); | |
| }) | |
| .on("complete", function () { | |
| console.log("Fastest is " + this.filter("fastest").map("name")); | |
| }) | |
| .run({ async: false }); | |
| }); |
| it("should benchmark applyOptimisticUpdate performance", async () => { | ||
| // First, set entities | ||
| useStore.getState().setEntities(mockEntities); | ||
|
|
||
| const suite = new Benchmark.Suite(); | ||
|
|
||
| suite | ||
| .add("applyOptimisticUpdate", () => { | ||
| useStore | ||
| .getState() | ||
| .applyOptimisticUpdate("txn_perf", (draft) => { | ||
| draft.entities[ | ||
| "entity500" | ||
| ].models.world!.item!.durability = 75; | ||
| }); | ||
| }) | ||
| .on("cycle", (event: any) => { | ||
| console.log(String(event.target)); | ||
| }) | ||
| .on("complete", function () { | ||
| console.log("Fastest is " + this.filter("fastest").map("name")); | ||
| }) | ||
| .run({ async: false }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unnecessary async keyword from test function
The test function is declared with async but does not contain any await expressions or handle any asynchronous operations. Removing the async keyword can prevent potential confusion and align with best practices.
Apply this diff to remove the async keyword:
-it("should benchmark applyOptimisticUpdate performance", async () => {
+it("should benchmark applyOptimisticUpdate performance", () => {📝 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.
| it("should benchmark applyOptimisticUpdate performance", async () => { | |
| // First, set entities | |
| useStore.getState().setEntities(mockEntities); | |
| const suite = new Benchmark.Suite(); | |
| suite | |
| .add("applyOptimisticUpdate", () => { | |
| useStore | |
| .getState() | |
| .applyOptimisticUpdate("txn_perf", (draft) => { | |
| draft.entities[ | |
| "entity500" | |
| ].models.world!.item!.durability = 75; | |
| }); | |
| }) | |
| .on("cycle", (event: any) => { | |
| console.log(String(event.target)); | |
| }) | |
| .on("complete", function () { | |
| console.log("Fastest is " + this.filter("fastest").map("name")); | |
| }) | |
| .run({ async: false }); | |
| }); | |
| it("should benchmark applyOptimisticUpdate performance", () => { | |
| // First, set entities | |
| useStore.getState().setEntities(mockEntities); | |
| const suite = new Benchmark.Suite(); | |
| suite | |
| .add("applyOptimisticUpdate", () => { | |
| useStore | |
| .getState() | |
| .applyOptimisticUpdate("txn_perf", (draft) => { | |
| draft.entities[ | |
| "entity500" | |
| ].models.world!.item!.durability = 75; | |
| }); | |
| }) | |
| .on("cycle", (event: any) => { | |
| console.log(String(event.target)); | |
| }) | |
| .on("complete", function () { | |
| console.log("Fastest is " + this.filter("fastest").map("name")); | |
| }) | |
| .run({ async: false }); | |
| }); |
| // Generate a large number of mock entities for testing | ||
| const numberOfEntities = 1000; | ||
| for (let i = 0; i < numberOfEntities; i++) { | ||
| mockEntities.push({ | ||
| entityId: `entity${i}`, | ||
| models: { | ||
| world: { | ||
| player: { | ||
| fieldOrder: ["id", "name", "score"], | ||
| id: `player${i}`, | ||
| name: `Player${i}`, | ||
| score: i, | ||
| }, | ||
| game: { | ||
| fieldOrder: ["id", "status"], | ||
| id: `game${i}`, | ||
| status: i % 2 === 0 ? "active" : "inactive", | ||
| }, | ||
| item: { | ||
| fieldOrder: ["id", "type", "durability"], | ||
| id: `item${i}`, | ||
| type: i % 3 === 0 ? "sword" : "shield", | ||
| durability: 100 - (i % 100), | ||
| }, | ||
| }, | ||
| universe: { | ||
| galaxy: { | ||
| fieldOrder: ["id", "name"], | ||
| id: `galaxy${i}`, | ||
| name: `Galaxy${i}`, | ||
| }, | ||
| }, | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Parameterize numberOfEntities for flexible performance testing
Currently, the numberOfEntities is fixed at 1000. By parameterizing this value, you can test the store's performance with different dataset sizes, which can provide more insights into how it scales and behaves under varying loads.
Consider modifying the code to accept an environment variable or function parameter:
-const numberOfEntities = 1000;
+const numberOfEntities = process.env.NUMBER_OF_ENTITIES ? parseInt(process.env.NUMBER_OF_ENTITIES) : 1000;This allows you to run tests with different entity counts by setting the NUMBER_OF_ENTITIES environment variable.
📝 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.
| // Generate a large number of mock entities for testing | |
| const numberOfEntities = 1000; | |
| for (let i = 0; i < numberOfEntities; i++) { | |
| mockEntities.push({ | |
| entityId: `entity${i}`, | |
| models: { | |
| world: { | |
| player: { | |
| fieldOrder: ["id", "name", "score"], | |
| id: `player${i}`, | |
| name: `Player${i}`, | |
| score: i, | |
| }, | |
| game: { | |
| fieldOrder: ["id", "status"], | |
| id: `game${i}`, | |
| status: i % 2 === 0 ? "active" : "inactive", | |
| }, | |
| item: { | |
| fieldOrder: ["id", "type", "durability"], | |
| id: `item${i}`, | |
| type: i % 3 === 0 ? "sword" : "shield", | |
| durability: 100 - (i % 100), | |
| }, | |
| }, | |
| universe: { | |
| galaxy: { | |
| fieldOrder: ["id", "name"], | |
| id: `galaxy${i}`, | |
| name: `Galaxy${i}`, | |
| }, | |
| }, | |
| }, | |
| }); | |
| } | |
| // Generate a large number of mock entities for testing | |
| const numberOfEntities = process.env.NUMBER_OF_ENTITIES ? parseInt(process.env.NUMBER_OF_ENTITIES) : 1000; | |
| for (let i = 0; i < numberOfEntities; i++) { | |
| mockEntities.push({ | |
| entityId: `entity${i}`, | |
| models: { | |
| world: { | |
| player: { | |
| fieldOrder: ["id", "name", "score"], | |
| id: `player${i}`, | |
| name: `Player${i}`, | |
| score: i, | |
| }, | |
| game: { | |
| fieldOrder: ["id", "status"], | |
| id: `game${i}`, | |
| status: i % 2 === 0 ? "active" : "inactive", | |
| }, | |
| item: { | |
| fieldOrder: ["id", "type", "durability"], | |
| id: `item${i}`, | |
| type: i % 3 === 0 ? "sword" : "shield", | |
| durability: 100 - (i % 100), | |
| }, | |
| }, | |
| universe: { | |
| galaxy: { | |
| fieldOrder: ["id", "name"], | |
| id: `galaxy${i}`, | |
| name: `Galaxy${i}`, | |
| }, | |
| }, | |
| }, | |
| }); | |
| } |
| setTimeout(() => { | ||
| useStore.getState().updateEntity({ | ||
| entityId: "game1", | ||
| models: { | ||
| world: { | ||
| game: { | ||
| status: "completed", | ||
| }, | ||
| }, | ||
| universe: {}, | ||
| }, | ||
| }); | ||
| }, 100); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use fake timers to improve test reliability and performance
Using setTimeout can introduce flakiness and slow down tests. Leveraging Vitest's fake timers will make the test more deterministic and faster.
Enable fake timers at the beginning of the test:
vi.useFakeTimers();Replace setTimeout and use vi.advanceTimersByTime:
// Simulate async update without real delay
useStore.getState().updateEntity({
entityId: "game1",
models: {
world: {
game: {
status: "completed",
},
},
universe: {},
},
});
// Advance timers to simulate the passage of time
vi.advanceTimersByTime(100);
// Wait for the promise to resolve
const result = await promise;
expect(result?.models.world?.game?.status).toBe("completed");
// Restore real timers at the end
vi.useRealTimers();| const promise = useStore | ||
| .getState() | ||
| .waitForEntityChange( | ||
| "game1", | ||
| (entity) => entity?.models.world?.game?.status === "never", | ||
| 500 | ||
| ); | ||
|
|
||
| // Do not update the entity to meet predicate | ||
|
|
||
| await expect(promise).rejects.toThrow( | ||
| "waitForEntityChange: Timeout of 500ms exceeded" | ||
| ); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use fake timers to control timeout behavior in tests
Waiting for real timeouts can slow down the test suite. Using fake timers will make the test execute faster and more reliably.
Enable fake timers and advance time to trigger the timeout:
vi.useFakeTimers();
const promise = useStore
.getState()
.waitForEntityChange(
"game1",
(entity) => entity?.models.world?.game?.status === "never",
500
);
// Advance timers to exceed the timeout duration
vi.advanceTimersByTime(500);
await expect(promise).rejects.toThrow(
"waitForEntityChange: Timeout of 500ms exceeded"
);
vi.useRealTimers();| const updateFn = (draft: any) => { | ||
| draft.entities["item1"].models.world!.item!.durability = 30; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Specify the type of 'draft' to enhance type safety
Defining a specific type for draft instead of any will improve type checking and code clarity.
- const updateFn = (draft: any) => {
+ const updateFn = (draft: Draft<DojoState>) => {
draft.entities["item1"].models.world!.item!.durability = 30;
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const updateFn = (draft: any) => { | |
| draft.entities["item1"].models.world!.item!.durability = 30; | |
| }; | |
| const updateFn = (draft: Draft<DojoState>) => { | |
| draft.entities["item1"].models.world!.item!.durability = 30; | |
| }; |
| draft.entities["item1"].models.world!.item!.durability = 30; | ||
| }; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Specify the type of 'draft' to enhance type safety
Using draft: any in the updateFn function reduces type safety. Specifying the type will help catch errors at compile time.
- const updateFn = (draft: any) => {
+ const updateFn = (draft: Draft<DojoState>) => {
draft.entities["item1"].models.world!.item!.durability = 30;
};Committable suggestion was skipped due to low confidence.
| const updateFn = (draft: any) => { | ||
| draft.entities["item1"].models.world!.item!.durability = 30; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Specify the type of 'draft' to enhance type safety
The updateFn function currently uses draft: any, which bypasses TypeScript's type checking. Defining a specific type for draft will improve code reliability and maintainability.
Consider importing Draft from immer and specifying the state type:
+ import { Draft } from "immer";
+ import { DojoState } from "../state/zustand"; // Assuming DojoState is the state type
const updateFn = (draft: Draft<DojoState>) => {
draft.entities["item1"].models.world!.item!.durability = 30;
};Committable suggestion was skipped due to low confidence.
WIP:
Summary by CodeRabbit
New Features
immer,uuid,starknet, andzustand.GalaxyModelfor better structure and reusability.Refactor
useDojoStore.Bug Fixes
Tests