fix: paths #311
fix: paths #311
Conversation
WalkthroughThe pull request consists of changes to multiple 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 (1)
examples/example-vite-react-app-recs/dojoConfig.ts (1)
3-3: Consider documenting the manifest path convention.Since this path change is applied across multiple example projects, it would be helpful to document the manifest file naming and location convention.
Consider adding a comment above the import:
+// Import manifest from the root worlds directory following the {name}_dev.json convention import manifest from "../../worlds/dojo-starter/manifest_dev.json";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
examples/example-nodejs-bot/dojoConfig.ts(1 hunks)examples/example-vanillajs-phaser-recs/dojoConfig.ts(1 hunks)examples/example-vite-kitchen-sink/dojoConfig.ts(1 hunks)examples/example-vite-react-app-recs/dojoConfig.ts(1 hunks)examples/example-vite-react-phaser-recs/dojoConfig.ts(1 hunks)examples/example-vite-react-pwa-recs/dojoConfig.ts(1 hunks)examples/example-vite-react-sdk/dojoConfig.ts(1 hunks)examples/example-vite-react-threejs-recs/dojoConfig.ts(1 hunks)examples/example-vite-svelte-recs/dojoConfig.ts(1 hunks)examples/example-vue-app-recs/dojoConfig.ts(1 hunks)readme.md(1 hunks)
🔇 Additional comments (11)
examples/example-vite-react-phaser-recs/dojoConfig.ts (1)
1-1: Verify the new manifest path and file compatibility.
The simplified path structure is an improvement. However, let's verify the new manifest location and ensure compatibility.
examples/example-vite-kitchen-sink/dojoConfig.ts (1)
3-3: Path simplification looks good, but let's verify the manifest structure.
The simplified path structure is cleaner and the _dev suffix better indicates the manifest's purpose.
Let's verify the manifest file existence and check for consistency across other config files:
examples/example-vite-react-app-recs/dojoConfig.ts (1)
3-3: Verify manifest file existence at new location.
The path simplification aligns with the PR objective to fix paths. However, let's verify that the manifest file exists at the new location.
examples/example-vite-react-sdk/dojoConfig.ts (2)
Line range hint 1-8: LGTM! Clean and focused configuration.
The configuration structure remains minimal and well-organized, properly utilizing the createDojoConfig from @dojoengine/core. The manifest path change doesn't impact the configuration pattern.
3-3: Verify the existence of the new manifest file location.
The path change simplifies the manifest location, which aligns with the PR objectives. However, we should verify that the new manifest file exists at the specified location.
examples/example-vite-react-threejs-recs/dojoConfig.ts (1)
3-3: Verify the new manifest path and structure.
The simplified path aligns with the PR's objective to fix paths in dojoconfig.
Let's verify the manifest file existence and structure:
✅ Verification successful
Path change appears to be consistent with codebase-wide updates
The import path ../../worlds/dojo-starter/manifest_dev.json is consistently used across multiple example projects in the codebase, confirming this is an intentional standardization. While the file is not found in the sandbox environment (which is expected as it's likely generated during build), the widespread adoption of this path pattern in 10+ example projects indicates this is the new standard location.
The only exception is in example-vite-kitchen-sink/src/dojo.config.ts which uses a different world (onchain-dash), but follows a similar pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and structure of the new manifest file
# Expected: The new manifest file should exist and maintain the same structure
# Check if the new manifest file exists
if [ -f "worlds/dojo-starter/manifest_dev.json" ]; then
echo "✓ New manifest file exists"
# Compare structure with old manifest if available
if [ -f "worlds/dojo-starter/manifests/dev/deployment/manifest.json" ]; then
echo "Comparing manifest structures:"
jq -S 'keys' "worlds/dojo-starter/manifest_dev.json"
jq -S 'keys' "worlds/dojo-starter/manifests/dev/deployment/manifest.json"
fi
else
echo "✗ New manifest file not found at worlds/dojo-starter/manifest_dev.json"
fi
# Check for any other files still referencing the old path
echo "Checking for any remaining references to old path:"
rg "manifests/dev/deployment/manifest.json"
Length of output: 356
Script:
#!/bin/bash
# Let's first check the actual directory structure to understand where the manifest files are located
fd manifest -t f -e json
# Then let's check all import statements for manifest files to understand the pattern
rg "import.*manifest.*from.*json"
# Also check for any manifest-related paths in the codebase
rg "manifest.*\.json" -g "!package*.json"
Length of output: 3095
examples/example-vite-svelte-recs/dojoConfig.ts (2)
Line range hint 1-8: LGTM! Path simplification aligns with PR objectives.
The change simplifies the manifest import path while maintaining the same functionality. This is part of a broader standardization across example projects as mentioned in the AI summary.
1-1: Verify the existence of the new manifest path.
The path simplification looks good, but let's verify the existence of the manifest file at the new location.
examples/example-nodejs-bot/dojoConfig.ts (1)
Line range hint 1-8: LGTM! Path simplification improves maintainability
The simplified manifest path makes the configuration more maintainable while preserving the correct relative path navigation and JSON type assertion. The overall structure of the dojoConfig setup remains clean and follows the expected pattern.
examples/example-vanillajs-phaser-recs/dojoConfig.ts (1)
3-3: Verify manifest file location
The path simplification aligns with the PR objective to fix paths. Let's verify the manifest file exists at the new location.
readme.md (1)
227-227: Verify the new migration command syntax.
Please confirm that sozo migrate is the correct new syntax and that the apply subcommand has been officially deprecated.
| @@ -1,6 +1,6 @@ | |||
| import { createDojoConfig } from "@dojoengine/core"; | |||
|
|
|||
| import manifest from "../../worlds/dojo-starter/manifests/dev/deployment/manifest.json"; | |||
| import manifest from "../../worlds/dojo-starter/manifest_dev.json"; | |||
There was a problem hiding this comment.
💡 Codebase verification
Invalid manifest import path across multiple examples
The verification reveals critical issues:
- The new import path
../../worlds/dojo-starter/manifest_dev.jsonis invalid as the file doesn't exist - Found inconsistency in
example-vite-kitchen-sinkusing a different path:../../../worlds/onchain-dash/manifests/release/deployment/manifest.json - References in
example-nodejs-botsuggest manifest should be indojo-starter/target/dev/manifest.json
This indicates the path changes are incorrect and need to be fixed to point to the actual manifest location.
🔗 Analysis chain
Verify the manifest file location and consistency.
The import path has been simplified, which aligns with the PR objective to fix paths. Let's verify the new structure is consistent and valid.
Let me verify if the manifest file exists in a different location or if there are any related manifest files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify manifest file existence and path consistency across examples
# Check if the new manifest file exists
echo "Checking manifest_dev.json existence:"
ls -l worlds/dojo-starter/manifest_dev.json
# Check for consistency across all dojoConfig.ts files
echo -e "\nChecking manifest import paths in all dojoConfig.ts files:"
fd dojoConfig.ts --exec grep -l "manifest.*json" {} \;
# Show the actual import lines to verify consistency
echo -e "\nShowing manifest import lines from all dojoConfig.ts files:"
fd dojoConfig.ts --exec grep "manifest.*json" {} \;
Length of output: 1817
Script:
#!/bin/bash
# Search for manifest files in the repository
echo "Searching for manifest files:"
fd manifest.*json
# Search for any references to manifest files in the codebase
echo -e "\nSearching for manifest file references in code:"
rg "manifest.*json" -g "!package*.json" -g "!dojoConfig.ts"
Length of output: 708
| @@ -1,6 +1,6 @@ | |||
| import { createDojoConfig } from "@dojoengine/core"; | |||
|
|
|||
| import manifest from "../../worlds/dojo-starter/manifests/dev/deployment/manifest.json"; | |||
| import manifest from "../../worlds/dojo-starter/manifest_dev.json"; | |||
There was a problem hiding this comment.
💡 Codebase verification
Path inconsistency detected across examples
The verification reveals inconsistencies in manifest file imports:
-
Most examples use
../../worlds/dojo-starter/manifest_dev.json -
One example (kitchen-sink/src/dojo.config.ts) still uses the old path structure:
../../../worlds/onchain-dash/manifests/release/deployment/manifest.json -
The manifest files referenced in the imports are not present in the worlds directory structure
-
examples/example-vite-kitchen-sink/src/dojo.config.tsneeds to be updated to use the new path structure -
The manifest files (
manifest_dev.json) are missing from theworlds/dojo-starterdirectory, which will cause runtime errors -
Consider adding a note in the PR to ensure the manifest files are properly generated or copied to the new locations
🔗 Analysis chain
Verify the new manifest path structure.
The simplified path structure looks good and aligns with the PR's objective to fix paths. However, let's ensure the changes are complete and consistent.
Let's verify the changes:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify manifest file existence and check for any remaining old path references
# Test 1: Check if the new manifest file exists
if [ -f "worlds/dojo-starter/manifest_dev.json" ]; then
echo "✓ New manifest file exists"
else
echo "⨯ New manifest file not found"
fi
# Test 2: Search for any remaining references to the old path structure
echo "Checking for remaining references to old path..."
rg -l "manifests/dev/deployment/manifest.json"
# Test 3: Verify no other files are still using the old manifest path
echo "Checking for other dojoConfig files..."
fd "dojoConfig.ts" | xargs rg "manifests/dev/deployment"
Length of output: 415
Script:
#!/bin/bash
# Let's verify with a broader search to understand the manifest file structure
# Check for any manifest files in the repository
echo "=== Looking for manifest files ==="
fd "manifest.*\.json"
# Search for any manifest-related imports
echo -e "\n=== Checking manifest imports ==="
rg "import.*manifest.*from" -A 1
# Look for manifest-related paths in configuration files
echo -e "\n=== Checking configuration files for manifest paths ==="
fd "config|dojoConfig" | xargs rg "manifest"
# Check the specific directory structure
echo -e "\n=== Checking worlds directory structure ==="
ls -R worlds/ 2>/dev/null || echo "worlds directory not found"
Length of output: 5268
| @@ -1,4 +1,4 @@ | |||
| import manifest from "../../worlds/dojo-starter/manifests/dev/deployment/manifest.json" assert { type: "json" }; | |||
| import manifest from "../../worlds/dojo-starter/manifest_dev.json" assert { type: "json" }; | |||
There was a problem hiding this comment.
💡 Codebase verification
Path change needs revision - manifest file not found
The verification reveals critical issues with the manifest path change:
- The new manifest file
manifest_dev.jsondoes not exist at the specified location - Multiple example projects are attempting to use this non-existent path
- The tsconfig.json suggests the manifest should be at
dojo-starter/target/dev/manifest.json
This path change appears to be incorrect and needs to be revised to point to the actual manifest location.
🔗 Analysis chain
Verify manifest file existence and compatibility
The import path has been simplified to use manifest_dev.json. Let's verify the file exists and maintains the expected structure.
Let me gather more information about the manifest file location and structure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify manifest file existence and check other examples for consistency
# Check if the new manifest file exists
echo "Checking manifest file existence..."
if [ -f "worlds/dojo-starter/manifest_dev.json" ]; then
echo "✓ manifest_dev.json exists"
else
echo "✗ manifest_dev.json not found"
fi
# Check if all example configs use the same manifest path
echo -e "\nChecking consistency across example configs..."
rg -l "manifest.*\.json.*assert" examples/
Length of output: 396
Script:
#!/bin/bash
# Check for any manifest files in the repository
echo "Searching for manifest files..."
fd manifest.*\.json
# Check the content of the PR diff to understand the path changes
echo -e "\nChecking PR changes related to manifest paths..."
git diff HEAD~1 | grep -A 2 -B 2 "manifest.*\.json"
# Look for any references to manifest files in the codebase
echo -e "\nSearching for manifest references in the code..."
rg "manifest.*\.json" -B 2 -A 2
Length of output: 5378
| @@ -224,7 +224,7 @@ To run the examples, you'll need to set up three terminal windows: | |||
|
|
|||
| ```bash | |||
| sozo build | |||
| sozo migrate apply | |||
| sozo migrate | |||
There was a problem hiding this comment.
Inconsistent migration commands in the documentation.
The migration command has been updated to sozo migrate in the Contributing section, but it still shows as sozo build && sozo migrate apply in the "Quick start in 5 minutes" section. This inconsistency could confuse users.
Update both instances to use the same command syntax:
- sozo build && sozo migrate apply
+ sozo build && sozo migrateCommittable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/sdk/src/subscribeEventQuery.ts (1)
43-43: Document the purpose of thetrueparameter.The newly added boolean parameter to
onEventMessageUpdatedlacks documentation explaining its purpose and impact. Consider updating the JSDoc to include this parameter's meaning.packages/sdk/src/getEventMessages.ts (1)
56-56: Document the purpose of the new boolean parameter.The addition of
trueas a parameter toclient.getEventMessagesis not documented in the function's JSDoc. This could make it difficult for other developers to understand its purpose and impact.Please update the JSDoc to include:
- The purpose of this boolean parameter
- Whether it's required or optional
- The impact on the function's behavior
packages/state/src/recs/index.ts (1)
166-174: Consider introducing a configuration object for better maintainability.Instead of adding individual boolean flags, consider introducing a configuration object pattern for these client methods. This would:
- Make the code more maintainable and self-documenting
- Provide better flexibility for future additions
- Make it easier to maintain consistency across different client method calls
Example approach:
interface EventConfig { includeHashedKeys?: boolean; // or whatever this flag represents // other config options... } // Then in the functions: function getEvents(..., config: EventConfig = defaultConfig) { // Use config.includeHashedKeys instead of hardcoded true }Also applies to: 309-309
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
packages/sdk/src/getEventMessages.ts(1 hunks)packages/sdk/src/subscribeEventQuery.ts(1 hunks)packages/state/src/recs/index.ts(2 hunks)
🔇 Additional comments (4)
packages/sdk/src/subscribeEventQuery.ts (1)
43-43: Verify consistent usage of the boolean parameter across the codebase.
Let's ensure this change is consistently applied across all usages of onEventMessageUpdated.
✅ Verification successful
Based on the search results, I can see that onEventMessageUpdated is used in two locations:
- packages/state/src/recs/index.ts
- packages/sdk/src/subscribeEventQuery.ts
In both cases, the function is called with three parameters, and both use true as the third parameter. Let's verify the function definition to ensure this is the expected signature.
Let's try a different approach to find the definition, as it might be in a different format or in an interface.
Boolean parameter usage is consistent across the codebase
The verification shows that onEventMessageUpdated is called consistently with three parameters in both locations where it's used:
- packages/state/src/recs/index.ts
- packages/sdk/src/subscribeEventQuery.ts
Both calls use true as the third parameter, maintaining consistency throughout the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for all occurrences of onEventMessageUpdated to verify consistent parameter usage
# Expected: All calls should include the boolean parameter
# Search for all calls to onEventMessageUpdated
rg -A 2 "onEventMessageUpdated\("
# Search specifically for the pattern without the boolean parameter to find any inconsistencies
rg "onEventMessageUpdated\([^,]+,[^,]+\)"
Length of output: 497
Script:
#!/bin/bash
# Search for the definition of onEventMessageUpdated
ast-grep --pattern 'onEventMessageUpdated($_) {
$$$
}'
# In case it's an interface definition
ast-grep --pattern 'onEventMessageUpdated($_): $_'
Length of output: 111
Script:
#!/bin/bash
# Search for any mention of onEventMessageUpdated in interface or type definitions
rg "onEventMessageUpdated" -A 3 -B 3
# Also search specifically in .d.ts files which might contain the type definitions
fd -e d.ts -x cat {} | rg "onEventMessageUpdated" -A 3 -B 3
Length of output: 1078
packages/sdk/src/getEventMessages.ts (1)
56-56: Verify the impact of the new parameter across the codebase.
The addition of the boolean parameter could affect other callers of this function.
Let's verify the usage and ensure consistency:
✅ Verification successful
Let me gather more specific information about the usage of getEventMessages.
Let me try one more search to understand the client interface and the parameter usage.
Let me try one final search to find the type definition specifically in the SDK package.
Based on the gathered information, I can now provide the final response:
The boolean parameter addition is safe and consistent with the codebase
The code review shows that all usages of getEventMessages are properly handled:
- The client interface in
packages/sdk/src/types.tsdefines the method signature - The implementation in
packages/sdk/src/getEventMessages.tsis the wrapper that calls the client method - All other usages in the codebase are going through this wrapper, which properly passes the boolean parameter
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other calls to getEventMessages to ensure they're updated
rg -l "getEventMessages" --type ts
# Check the torii-client implementation to understand the parameter
ast-grep --pattern 'getEventMessages($$$) {
$$$
}'
Length of output: 211
Script:
#!/bin/bash
# Check actual calls to getEventMessages with context
rg "getEventMessages\(" -A 2 -B 2 --type ts
# Check the function signature/definition
rg "getEventMessages.*=.*{" --type ts
rg "getEventMessages.*\(.*\).*{" --type ts
Length of output: 1628
Script:
#!/bin/bash
# Search for client interface or type definition
rg "interface.*Client" -A 10 --type ts
rg "type.*Client" -A 10 --type ts
# Look for getEventMessages in type definitions
ast-grep --pattern 'interface $_ {
$$$
getEventMessages($$$): $$$
$$$
}'
Length of output: 11571
Script:
#!/bin/bash
# Look specifically in the SDK package for the client interface and getEventMessages
rg "getEventMessages" -B 5 -A 5 packages/sdk/src/types.ts
# Check if there are any other implementations or usages in the SDK
rg "getEventMessages" packages/sdk/src/ --type ts -C 3
Length of output: 3878
packages/state/src/recs/index.ts (2)
309-309: Document and consider parameterizing the new boolean flag.
The addition of true as a parameter to client.onEventMessageUpdated raises similar documentation concerns. Consider:
- Documenting the purpose of this flag in the JSDoc
- Making it a configurable parameter of
syncEventswith a default value - Using a descriptive constant instead of a magic boolean
Let's verify if this parameter is documented in the client interface:
#!/bin/bash
# Search for the onEventMessageUpdated definition to understand the parameter
rg -t ts "onEventMessageUpdated.*\(" --no-heading166-174: Document the purpose of the new boolean parameter.
The addition of true as the second argument to client.getEventMessages needs documentation. Please:
- Update the function's JSDoc to explain this parameter
- Consider making it a named parameter in the function signature for better clarity
Let's verify if this parameter is documented in the client interface:
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
manifestvariable across multipledojoConfig.tsfiles, ensuring consistency and reliability in configuration.Documentation