fix: readme + phaser recs example #287
Conversation
WalkthroughThe pull request introduces a new entry for the submodule "worlds/dojo-starter" in the Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous 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: 3
🧹 Outside diff range and nitpick comments (3)
examples/example-nodejs-bot/README.md (1)
Line range hint
100-103: Update deployment instructions for pnpm.The deployment section still references
bun, which is inconsistent with the rest of the README that has been updated to usepnpm. Please update these instructions to usepnpminstead.Here's a suggested update:
Update the project settings: **Build Command:** -`bun run build` +`pnpm build` **Run Commands:** -`bun run serve` +`pnpm serve`readme.md (1)
99-100: Approve with suggestion: Improve formatting of the migration noteThis note provides valuable troubleshooting information for users who might encounter issues when migrating worlds. To improve readability, consider formatting it as a blockquote or adding emphasis. For example:
> **Note**: If you can't migrate worlds running `sozo migrate apply`, be sure to comment out `world_address` in the corresponding `worlds/${world}/dojo_dev.toml` file.This formatting will make the note stand out more in the documentation.
examples/example-vanillajs-phaser-recs/src/scenes/scene-main.ts (1)
138-158: Ensure consistent order of camera centering and movement commandsThere is inconsistency in the order of
this.cameras.main.centerOn(...)andawait this.dojo.systemCalls.move(...)calls across different movement directions. In the 'Up' direction,centerOnis called before theawaitcall, whereas in other directions, it's called after. This could lead to inconsistent behavior in camera movement and player actions.Apply this diff to standardize the order:
Option 1: Call
centerOnbefore the movement command in all cases// Down direction if (null !== this.keyS && this.keyS.isDown) { this.followPoint.y += this.cameraSpeed; + this.cameras.main.centerOn(this.followPoint.x, this.followPoint.y); await this.dojo.systemCalls.move(this.dojo.account, Direction.Down); - this.cameras.main.centerOn(this.followPoint.x, this.followPoint.y); return; } // Left direction if (null !== this.keyA && this.keyA.isDown) { this.followPoint.x -= this.cameraSpeed; + this.cameras.main.centerOn(this.followPoint.x, this.followPoint.y); await this.dojo.systemCalls.move(this.dojo.account, Direction.Left); - this.cameras.main.centerOn(this.followPoint.x, this.followPoint.y); return; } // Right direction if (null !== this.keyD && this.keyD.isDown) { this.followPoint.x += this.cameraSpeed; + this.cameras.main.centerOn(this.followPoint.x, this.followPoint.y); await this.dojo.systemCalls.move( this.dojo.account, Direction.Right ); - this.cameras.main.centerOn(this.followPoint.x, this.followPoint.y); return; }Option 2: Call
centerOnafter the movement command in all cases// Up direction if (null !== this.keyW && this.keyW.isDown) { this.followPoint.y -= this.cameraSpeed; - this.cameras.main.centerOn(this.followPoint.x, this.followPoint.y); await this.dojo.systemCalls.move(this.dojo.account, Direction.Up); + this.cameras.main.centerOn(this.followPoint.x, this.followPoint.y); return; }Choose the option that aligns with the intended game experience—whether the camera should move immediately with the key press or after the movement command completes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- .gitmodules (1 hunks)
- examples/example-nodejs-bot/README.md (2 hunks)
- examples/example-vanillajs-phaser-recs/src/main.ts (1 hunks)
- examples/example-vanillajs-phaser-recs/src/scenes/scene-main.ts (2 hunks)
- readme.md (1 hunks)
🔇 Additional comments (8)
examples/example-vanillajs-phaser-recs/src/main.ts (1)
34-34: Simplified error handling looks good, but verify previous implementation's intent.The simplified error handling is a good improvement in terms of code conciseness. It maintains the error logging functionality while adhering to the "fail fast" principle.
However, please verify that:
- There was no specific reason for the previous implementation's structure.
- The removal of the explicit
undefinedreturn in error cases doesn't affect any downstream error handling.To ensure no critical information was lost in the simplification, let's check the git history:
✅ Verification successful
Simplified error handling verified and approved.
The change maintains error logging functionality and simplifies the code without altering its behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the git history for the previous implementation of the error handling. # Test: Show the previous implementation of the error handling. git log -p -1 -- examples/example-vanillajs-phaser-recs/src/main.ts | grep -A 5 "\.catch"Length of output: 193
examples/example-nodejs-bot/README.md (4)
21-21: LGTM: Package manager updated correctly.The change from
bun installtopnpm installis correct and consistent with the switch to pnpm as the package manager.
27-27: LGTM: Serve command updated correctly.The change from
bun run servetopnpm serveis correct and consistent with the switch to pnpm. Note that pnpm allows for this shorter syntax (pnpm serveinstead ofpnpm run serve), which is a nice optimization.
33-33: LGTM: Build command updated correctly.The change from
bun run build --watchtopnpm build --watchis correct and consistent with the switch to pnpm. The shorter syntax is appropriately used here as well.
49-49: LGTM: Codegen command updated correctly.The change from
bun run codegentopnpm codegenis correct and consistent with the switch to pnpm. The shorter syntax is appropriately used here as well.readme.md (3)
98-98: LGTM: Helpful note for usersThis note provides crucial information for users attempting to run the examples. It's well-placed and clearly written.
Line range hint
1-102: Overall assessment: Valuable additions to the documentationThe changes to this readme file provide more detailed and helpful instructions for users working with the Dojo project. The additions address potential issues and offer clear guidance for setting up and running examples. While some minor improvements in formatting and clarification have been suggested, the overall changes significantly enhance the documentation's usefulness.
Great job on improving the user experience!
102-102: Approve with suggestions: Clarify Torii command and address security considerationThe added Torii command is helpful for users setting up their environment. However, consider the following suggestions:
Clarify if the world address is an example or a fixed value. If it's an example, you might want to use a placeholder like
<world_address>instead.The
--allowed-origins "*"flag allows requests from any origin, which could pose security risks in production environments. Consider adding a note about this, such as:> **Security Note**: The `--allowed-origins "*"` flag is used here for development purposes. In production environments, specify the allowed origins explicitly to enhance security.These clarifications will help users understand the command better and be aware of potential security considerations.
To verify if the world address is used consistently across the documentation, you can run the following script:
✅ Verification successful
Verification Successful: Torii Command Usage Consistent
- The world address
0xb4079627ebab1cd3cf9fd075dda1ad2454a7a448bf659591f259efa2519b18is consistently used in the Torii command withinreadme.md.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for occurrences of the world address in the documentation # Test: Search for the specific world address rg --type md '0xb4079627ebab1cd3cf9fd075dda1ad2454a7a448bf659591f259efa2519b18'Length of output: 190
| [submodule "worlds/dojo-starter"] | ||
| path = worlds/dojo-starter | ||
| url = https://github.com/dojoengine/dojo-starter |
There was a problem hiding this comment.
Remove duplicate submodule entries
The newly added submodule entry for "worlds/dojo-starter" (lines 10-12) is a duplicate of the existing entry (lines 1-3). Additionally, there's a duplicate entry for "onchain-dash" (lines 7-9). Duplicate entries in the .gitmodules file can lead to confusion and potential issues with Git submodule management.
To resolve this, please remove the duplicate entries. Here's the suggested change:
[submodule "dojo-starter"]
path = worlds/dojo-starter
url = https://github.com/dojoengine/dojo-starter
[submodule "onchain-dash"]
path = worlds/onchain-dash
url = https://github.com/MartianGreed/onchain-dash
-[submodule "worlds/onchain-dash"]
- path = worlds/onchain-dash
- url = https://github.com/MartianGreed/onchain-dash
-[submodule "worlds/dojo-starter"]
- path = worlds/dojo-starter
- url = https://github.com/dojoengine/dojo-starterThis change will maintain the correct submodule configurations without redundancy.
📝 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.
| [submodule "worlds/dojo-starter"] | |
| path = worlds/dojo-starter | |
| url = https://github.com/dojoengine/dojo-starter | |
| [submodule "dojo-starter"] | |
| path = worlds/dojo-starter | |
| url = https://github.com/dojoengine/dojo-starter | |
| [submodule "onchain-dash"] | |
| path = worlds/onchain-dash | |
| url = https://github.com/MartianGreed/onchain-dash |
| @@ -135,25 +135,28 @@ export default class SceneMain extends Scene { | |||
| if (null !== this.keyW && this.keyW.isDown) { | |||
| this.followPoint.y -= this.cameraSpeed; | |||
| this.cameras.main.centerOn(this.followPoint.x, this.followPoint.y); | |||
| this.dojo.systemCalls.move(this.dojo.account, Direction.Up); | |||
| await this.dojo.systemCalls.move(this.dojo.account, Direction.Up); | |||
There was a problem hiding this comment.
Add error handling for asynchronous movement commands
When calling await this.dojo.systemCalls.move(...), any errors thrown by the promise will not be caught, potentially causing unhandled exceptions. To ensure application stability, wrap the await calls in try-catch blocks to handle possible errors gracefully.
Apply this diff to add error handling:
// Up direction
if (null !== this.keyW && this.keyW.isDown) {
this.followPoint.y -= this.cameraSpeed;
this.cameras.main.centerOn(this.followPoint.x, this.followPoint.y);
- await this.dojo.systemCalls.move(this.dojo.account, Direction.Up);
+ try {
+ await this.dojo.systemCalls.move(this.dojo.account, Direction.Up);
+ } catch (error) {
+ console.error('Failed to move Up:', error);
+ }
return;
}
// Down direction
if (null !== this.keyS && this.keyS.isDown) {
this.followPoint.y += this.cameraSpeed;
this.cameras.main.centerOn(this.followPoint.x, this.followPoint.y);
- await this.dojo.systemCalls.move(this.dojo.account, Direction.Down);
+ try {
+ await this.dojo.systemCalls.move(this.dojo.account, Direction.Down);
+ } catch (error) {
+ console.error('Failed to move Down:', error);
+ }
return;
}
// Left direction
if (null !== this.keyA && this.keyA.isDown) {
this.followPoint.x -= this.cameraSpeed;
this.cameras.main.centerOn(this.followPoint.x, this.followPoint.y);
- await this.dojo.systemCalls.move(this.dojo.account, Direction.Left);
+ try {
+ await this.dojo.systemCalls.move(this.dojo.account, Direction.Left);
+ } catch (error) {
+ console.error('Failed to move Left:', error);
+ }
return;
}
// Right direction
if (null !== this.keyD && this.keyD.isDown) {
this.followPoint.x += this.cameraSpeed;
this.cameras.main.centerOn(this.followPoint.x, this.followPoint.y);
- await this.dojo.systemCalls.move(
- this.dojo.account,
- Direction.Right
- );
+ try {
+ await this.dojo.systemCalls.move(
+ this.dojo.account,
+ Direction.Right
+ );
+ } catch (error) {
+ console.error('Failed to move Right:', error);
+ }
return;
}This ensures that any errors during the movement calls are handled appropriately, preventing potential crashes.
Also applies to: 143-143, 149-149, 155-158
| @@ -85,7 +85,7 @@ | |||
| return chunk; | |||
| } | |||
|
|
|||
| update() { | |||
| async update() { | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid making the update() method asynchronous in Phaser scenes
Making the update() method asynchronous in Phaser can lead to unexpected behavior and performance issues. The Phaser game loop expects update() to be a synchronous function called on each frame. Introducing await inside this method may cause frame drops or delays. It's recommended to keep update() synchronous and handle asynchronous operations differently.
Apply this diff to remove the async keyword and adjust the await calls:
- async update() {
+ update() {In each movement case, remove the await keyword:
if (null !== this.keyW && this.keyW.isDown) {
this.followPoint.y -= this.cameraSpeed;
this.cameras.main.centerOn(this.followPoint.x, this.followPoint.y);
- await this.dojo.systemCalls.move(this.dojo.account, Direction.Up);
+ this.dojo.systemCalls.move(this.dojo.account, Direction.Up);
return;
}Consider handling the promises returned by this.dojo.systemCalls.move without awaiting them inside the update() method, or by using event-driven approaches to manage asynchronous behavior.
Committable suggestion was skipped due to low confidence.
Summary by CodeRabbit
New Features
Bug Fixes
buntopnpm.Documentation