feat: optimistic example #293
Conversation
WalkthroughThe changes in this pull request introduce a new custom React hook, Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (6)
readme.md (3)
156-162: LGTM: Improved formatting and consistency in example listThe list of examples has been reformatted for better consistency and readability. The paths to the examples have been correctly updated.
Consider adding a brief description (1-2 words) next to each example to give readers a quick idea of what each example demonstrates. For instance:
- [example-vite-react-sdk](./examples/example-vite-react-sdk/) - Basic React integration - [example-vite-react-phaser-recs](./examples/example-vite-react-phaser-recs/) - Phaser game integrationThis would help users quickly identify which example is most relevant to their needs.
Line range hint
164-264: Enhance the Contributing sectionThe contributing guidelines are comprehensive and well-structured. To further improve this section:
Consider adding a subsection about coding standards and pull request guidelines. This could include:
- Code style guidelines (e.g., use of linters, formatting tools)
- Commit message format
- Pull request template
- Code review process
This addition would help maintain consistency in contributions and streamline the review process.
Line range hint
1-264: LGTM: Overall improvements to README structure and contentThe changes to the README file have significantly improved its structure, clarity, and usability. The updates to example references and paths are consistent throughout the document, and the contributing section provides comprehensive guidance for developers.
To further enhance the document:
- Consider adding a "Quick Links" section at the top of the README with links to important resources like documentation, community channels, and the main Dojo repository.
- You might want to include a brief section on "Compatibility" or "Requirements" to inform users about the necessary environment or dependencies for using Dojo.js.
These additions would make it even easier for new users to get started with the project.
packages/sdk/readme.md (1)
347-419: Excellent addition of the "Optimistic Client Rendering" section!This new section effectively explains the concept of optimistic updates and provides a detailed, well-commented code example. It aligns perfectly with the PR objective of adding an optimistic example to the project.
The use of immer for efficient state updates is a great practice. The step-by-step explanation and the code example will be very helpful for developers implementing this feature.
Consider adding a brief explanation of why optimistic rendering is beneficial (e.g., improved perceived performance, better user experience) to further emphasize its importance.
examples/example-vite-react-sdk/src/App.tsx (2)
9-9: LGTM. Consider adding documentation for the new hook.The addition of the
useSystemCallshook is a good step towards better organization of system-related calls. This aligns well with React best practices for custom hooks.To improve maintainability, consider adding JSDoc comments for the
useSystemCallshook in its implementation file, explaining its purpose and usage.
Line range hint
1-314: Overall good refactoring, but clarification needed on optimistic updates.The changes in this file improve the organization of system calls by introducing and using the
useSystemCallshook. This is a positive step towards better code structure and potentially easier management of side effects.However, the PR objectives mention adding an optimistic example, which is not immediately apparent in these changes. Could you provide more information on how the optimistic updates are implemented? Are they handled within the
useSystemCallshook, or is there additional code not shown in this diff that implements this feature?Consider documenting the optimistic update strategy, either in comments within the
useSystemCallshook or in a separate documentation file, to make the implementation clear for other developers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- examples/example-vite-react-sdk/src/App.tsx (3 hunks)
- examples/example-vite-react-sdk/src/useSystemCalls.ts (1 hunks)
- packages/sdk/readme.md (1 hunks)
- readme.md (3 hunks)
🧰 Additional context used
🪛 Biome
examples/example-vite-react-sdk/src/useSystemCalls.ts
[error] 33-34: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (7)
readme.md (2)
23-23: LGTM: Improved clarity in Table of ContentsThe renaming of "Examples" to "All Examples" in the Table of Contents enhances clarity and consistency with the corresponding section title later in the document.
73-74: LGTM: Updated example referencesThe paths to the example projects have been correctly updated, ensuring that the links are accurate and consistent with the project structure.
packages/sdk/readme.md (3)
Line range hint
14-21: Great addition of the "Key Features" section!This new section provides a clear and concise overview of the SDK's main features. It effectively highlights the key benefits for potential users, which aligns well with the overall documentation improvements mentioned in the PR objectives.
Line range hint
421-460: Great update to the "Advanced Usage" section!The addition of a more complex query example effectively demonstrates how to create 'AND' with 'OR' statements in queries. This example will be valuable for users who need to construct sophisticated queries.
The code is well-formatted and easy to understand, which aligns with the overall improvement of the documentation mentioned in the PR objectives.
Line range hint
1-460: Excellent improvements to the SDK documentation!The changes made to this README file significantly enhance its clarity and comprehensiveness. The addition of the "Key Features" section, the detailed explanation of "Optimistic Client Rendering" with a practical example, and the improved "Advanced Usage" section with complex query examples all contribute to making the documentation more valuable for SDK users.
These improvements align perfectly with the PR objectives of adding an optimistic example and enhancing the README. Great job on making the Dojo SDK documentation more user-friendly and informative!
examples/example-vite-react-sdk/src/App.tsx (2)
167-167: LGTM. Clarify the implementation of optimistic updates.The modification to use the
spawnfunction fromuseSystemCallsis consistent with the earlier changes and correctly handles the asynchronous nature of the call.However, the PR objectives mention adding an optimistic example, which is not immediately apparent in this change. Could you clarify how the optimistic updates are implemented?
To help verify this, let's check the implementation of the
spawnfunction:#!/bin/bash # Description: Check the implementation of the spawn function # Test: Look for the definition of spawn function within useSystemCalls rg --type typescript 'function useSystemCalls' -A 20 | rg 'spawn.*='
21-22: Good refactoring. Verify the implementation ofuseSystemCalls.Extracting the
spawnfunction fromuseSystemCallsis a good refactoring step. It centralizes system calls and potentially allows for easier management of side effects or optimistic updates.To ensure the correctness of this change, let's verify the implementation of
useSystemCalls:
| // Confirm the transaction if successful | ||
| state.confirmTransaction(transactionId); | ||
| } |
There was a problem hiding this comment.
Confirm transaction only on successful spawn
Currently, state.confirmTransaction(transactionId); is called in the finally block, which means it executes regardless of whether the spawn action succeeded or failed. This could lead to confirming a transaction that was reverted due to an error.
Move state.confirmTransaction(transactionId); to after the successful completion of the spawn action.
Suggestion:
}
- } catch (error) {
+ // Confirm the transaction if successful
+ state.confirmTransaction(transactionId);
+ } catch (error) {
// Revert the optimistic update if an error occurs
state.revertOptimisticUpdate(transactionId);
console.error("Error executing spawn:", error);
throw error;
- } finally {
- // Confirm the transaction if successful
- state.confirmTransaction(transactionId);
}Committable suggestion was skipped due to low confidence.
| (draft.entities[entityId].models.dojo_starter.Moves!.remaining = | ||
| remainingMoves) |
There was a problem hiding this comment.
Avoid assignment within an expression
Assigning a value within an expression can be confusing and may lead to unintended side effects. It's generally recommended to separate assignments from expressions for better readability.
Refactor the code to separate the assignment from the expression.
Suggestion:
state.applyOptimisticUpdate(
transactionId,
- (draft) =>
- (draft.entities[entityId].models.dojo_starter.Moves!.remaining =
- remainingMoves)
+ (draft) => {
+ draft.entities[entityId].models.dojo_starter.Moves!.remaining = remainingMoves;
+ }
);📝 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 = | |
| remainingMoves) | |
| state.applyOptimisticUpdate( | |
| transactionId, | |
| (draft) => { | |
| draft.entities[entityId].models.dojo_starter.Moves!.remaining = remainingMoves; | |
| } | |
| ); |
🧰 Tools
🪛 Biome
[error] 33-34: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
| } = useDojo(); | ||
|
|
||
| const generateEntityId = () => { | ||
| return getEntityIdFromKeys([BigInt(account?.address)]); |
There was a problem hiding this comment.
Handle potential 'undefined' account in generateEntityId
In the generateEntityId function, calling BigInt(account?.address) may throw an error if account or account.address is undefined, since BigInt(undefined) will result in a TypeError.
Consider adding a check to ensure that account and account.address are defined before converting to BigInt.
Suggestion:
const generateEntityId = () => {
+ if (!account?.address) {
+ throw new Error('Account address is undefined');
+ }
return getEntityIdFromKeys([BigInt(account.address)]);
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return getEntityIdFromKeys([BigInt(account?.address)]); | |
| if (!account?.address) { | |
| throw new Error('Account address is undefined'); | |
| } | |
| return getEntityIdFromKeys([BigInt(account.address)]); |
| const spawn = async () => { | ||
| // Generate a unique entity ID | ||
| const entityId = generateEntityId(); | ||
|
|
||
| // Generate a unique transaction ID | ||
| const transactionId = uuidv4(); | ||
|
|
||
| // The value to update the Moves model with | ||
| const remainingMoves = 100; | ||
|
|
||
| // Apply an optimistic update to the state | ||
| // this uses immer drafts to update the state | ||
| state.applyOptimisticUpdate( | ||
| transactionId, | ||
| (draft) => | ||
| (draft.entities[entityId].models.dojo_starter.Moves!.remaining = | ||
| remainingMoves) | ||
| ); | ||
|
|
||
| try { | ||
| // Execute the spawn action from the client | ||
| await client.actions.spawn({ account }); | ||
|
|
||
| // Wait for the entity to be updated with the new state | ||
| await state.waitForEntityChange(entityId, (entity) => { | ||
| return ( | ||
| entity?.models?.dojo_starter?.Moves?.remaining === | ||
| remainingMoves | ||
| ); | ||
| }); | ||
| } catch (error) { | ||
| // Revert the optimistic update if an error occurs | ||
| state.revertOptimisticUpdate(transactionId); | ||
| console.error("Error executing spawn:", error); | ||
| throw error; | ||
| } finally { | ||
| // Confirm the transaction if successful | ||
| state.confirmTransaction(transactionId); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Ensure account is defined before proceeding in spawn
Within the spawn function, if account is undefined, calls to generateEntityId() and client.actions.spawn({ account }) may fail, leading to runtime errors.
Add a check at the beginning of the spawn function to ensure account is defined.
Suggestion:
const spawn = async () => {
+ if (!account) {
+ throw new Error('Account is undefined');
+ }
// Generate a unique entity ID
const entityId = generateEntityId();
// ...📝 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 spawn = async () => { | |
| // Generate a unique entity ID | |
| const entityId = generateEntityId(); | |
| // Generate a unique transaction ID | |
| const transactionId = uuidv4(); | |
| // The value to update the Moves model with | |
| const remainingMoves = 100; | |
| // Apply an optimistic update to the state | |
| // this uses immer drafts to update the state | |
| state.applyOptimisticUpdate( | |
| transactionId, | |
| (draft) => | |
| (draft.entities[entityId].models.dojo_starter.Moves!.remaining = | |
| remainingMoves) | |
| ); | |
| try { | |
| // Execute the spawn action from the client | |
| await client.actions.spawn({ account }); | |
| // Wait for the entity to be updated with the new state | |
| await state.waitForEntityChange(entityId, (entity) => { | |
| return ( | |
| entity?.models?.dojo_starter?.Moves?.remaining === | |
| remainingMoves | |
| ); | |
| }); | |
| } catch (error) { | |
| // Revert the optimistic update if an error occurs | |
| state.revertOptimisticUpdate(transactionId); | |
| console.error("Error executing spawn:", error); | |
| throw error; | |
| } finally { | |
| // Confirm the transaction if successful | |
| state.confirmTransaction(transactionId); | |
| } | |
| }; | |
| const spawn = async () => { | |
| if (!account) { | |
| throw new Error('Account is undefined'); | |
| } | |
| // Generate a unique entity ID | |
| const entityId = generateEntityId(); | |
| // Generate a unique transaction ID | |
| const transactionId = uuidv4(); | |
| // The value to update the Moves model with | |
| const remainingMoves = 100; | |
| // Apply an optimistic update to the state | |
| // this uses immer drafts to update the state | |
| state.applyOptimisticUpdate( | |
| transactionId, | |
| (draft) => | |
| (draft.entities[entityId].models.dojo_starter.Moves!.remaining = | |
| remainingMoves) | |
| ); | |
| try { | |
| // Execute the spawn action from the client | |
| await client.actions.spawn({ account }); | |
| // Wait for the entity to be updated with the new state | |
| await state.waitForEntityChange(entityId, (entity) => { | |
| return ( | |
| entity?.models?.dojo_starter?.Moves?.remaining === | |
| remainingMoves | |
| ); | |
| }); | |
| } catch (error) { | |
| // Revert the optimistic update if an error occurs | |
| state.revertOptimisticUpdate(transactionId); | |
| console.error("Error executing spawn:", error); | |
| throw error; | |
| } finally { | |
| // Confirm the transaction if successful | |
| state.confirmTransaction(transactionId); | |
| } | |
| }; |
🧰 Tools
🪛 Biome
[error] 33-34: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
Summary by CodeRabbit
New Features
useSystemCalls, for managing system-related functionalities.Documentation
Bug Fixes