fix: create-dojo + update manifest path #338
Conversation
WalkthroughThis pull request includes modifications to several files, primarily focusing on updating import paths and restructuring the project initialization process. The Changes
Possibly related PRs
Suggested reviewers
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: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
examples/example-vite-kitchen-sink/dojoConfig.ts(1 hunks)examples/example-vite-kitchen-sink/src/main.tsx(1 hunks)packages/create-dojo/src/commands/start.ts(3 hunks)
🔇 Additional comments (6)
examples/example-vite-kitchen-sink/dojoConfig.ts (1)
Line range hint 5-7: LGTM: Configuration setup looks correct
The usage of createDojoConfig with the manifest looks correct.
examples/example-vite-kitchen-sink/src/main.tsx (1)
12-12: Verify the dojoConfig path resolution
The import path has been changed from an alias-based import to a relative import. Let's verify that the new path correctly resolves to the config file.
✅ Verification successful
Import path resolution is correct
The verification confirms that:
- The
dojoConfig.tsfile exists at the expected location:examples/example-vite-kitchen-sink/dojoConfig.ts - The relative import path
"../dojoConfig"inmain.tsxcorrectly resolves to this file - No other files in the project are importing
dojoConfigdifferently
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and location of dojoConfig file
# Expected: Find the config file in examples/example-vite-kitchen-sink/dojoConfig.ts
# Check if the file exists at the expected location
fd -e ts -e js "dojoConfig" examples/example-vite-kitchen-sink/
# Verify any other files that might be importing dojoConfig to ensure consistency
rg "from.*dojoConfig" examples/example-vite-kitchen-sink/
Length of output: 264
packages/create-dojo/src/commands/start.ts (4)
47-47: Definition of contractPath is correct
The contractPath is appropriately defined as a subdirectory of projectPath, pointing to the "contract" directory.
52-52: Creation of contractPath directory
Using fs.mkdir with { recursive: true } ensures that the "contract" directory is created successfully, even if parent directories do not exist.
69-70: Invocation of rewriteDojoConfigFile function
The call to await rewriteDojoConfigFile(clientPath); correctly introduces the function to update import paths in dojoConfig.ts, aligning with the new directory structure.
121-124: Verify that the regular expression correctly updates import paths
Ensure that the regular expression accurately matches all relevant import statements and that the replacement string points to the correct manifest file in the new directory structure.
Run the following script to confirm all import paths have been updated:
✅ Verification successful
Let me gather more information about the context where this replacement is being used.
Let me try a different approach to understand the context of this file modification.
Based on the context gathered, I can now provide the final response:
The regular expression correctly handles manifest import path updates
The regex /from ['"]\.{0,2}\/.*manifest(?:_dev)?\.json['"]/g is correctly implemented to:
- Match imports using both single/double quotes
- Handle relative paths with optional
./or../ - Match both
manifest.jsonandmanifest_dev.json - Replace with the standardized path
../contract/target/dev/manifest.json
This is used in the rewriteDojoConfigFile function which updates the import paths when initializing new projects from templates, ensuring consistent manifest file references across all new projects.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for manifest import statements in dojoConfig.ts files
rg "from ['\"]\.{0,2}/.*manifest(?:_dev)?\.json['\"]" -g "**/dojoConfig.ts" -A 2
Length of output: 2805
Script:
#!/bin/bash
# Search for the file content where the regex replacement is being used
rg -l "content\.replace\(.*manifest(?:_dev)?\.json"
# Also check the surrounding context of where this replacement is happening
rg -B 5 -A 5 "content\.replace\(.*manifest(?:_dev)?\.json"
Length of output: 114
Script:
#!/bin/bash
# Let's look at the content of the start.ts file to understand the context
cat packages/create-dojo/src/commands/start.ts
# Also search for any dojoConfig.ts files in the template directory
fd dojoConfig.ts packages/create-dojo/
Length of output: 7102
| @@ -1,6 +1,6 @@ | |||
| import { createDojoConfig } from "@dojoengine/core"; | |||
|
|
|||
| import manifest from "../../worlds/dojo-starter/manifest_dev.json"; | |||
| import manifest from "../../worlds/onchain-dash/manifests/release/deployment/manifest.json"; | |||
There was a problem hiding this comment.
💡 Codebase verification
Manifest path is incorrect and inconsistent with other examples
The verification reveals critical issues:
- The specified manifest path
worlds/onchain-dash/manifests/release/deployment/manifest.jsondoes not exist - All other example projects consistently use
worlds/dojo-starter/manifest_dev.json - Only the kitchen-sink example deviates with two files referencing the non-existent path:
examples/example-vite-kitchen-sink/dojoConfig.tsexamples/example-vite-kitchen-sink/src/dojo.config.ts
The change should be reverted to use the same development manifest as other examples (worlds/dojo-starter/manifest_dev.json) to maintain consistency and ensure the example works out of the box.
🔗 Analysis chain
Verify the manifest path exists and is accessible
The manifest path has been changed to point to a release manifest in the onchain-dash project. Let's verify this path exists and is consistent across the repository.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the manifest path exists and check for other manifest references
# Check if the manifest file exists
if [ -f "worlds/onchain-dash/manifests/release/deployment/manifest.json" ]; then
echo "✓ Manifest file exists"
else
echo "✗ Manifest file not found"
fi
# Search for other manifest references to ensure consistency
echo "Checking other manifest references:"
rg -g '!*.json' "manifest.*\.json"
Length of output: 1925
🛠️ Refactor suggestion
Consider making the manifest path configurable
The hardcoded relative path to the manifest file might be problematic:
- It assumes a specific directory structure
- It's using a release manifest for what appears to be a development/example setup
- The relative path (
../../) could break if the file structure changes
Consider making the manifest path configurable through environment variables:
-import manifest from "../../worlds/onchain-dash/manifests/release/deployment/manifest.json";
+import manifest from process.env.DOJO_MANIFEST_PATH || "../../worlds/onchain-dash/manifests/release/deployment/manifest.json";This would allow developers to:
- Override the manifest path for local development
- Use different manifests for different environments
- Avoid breaking changes if the directory structure changes
Committable suggestion skipped: line range outside the PR's diff.
| @@ -9,7 +9,7 @@ import "./app/globals.css"; | |||
| import { init } from "@dojoengine/sdk"; | |||
| import { OnchainDashSchemaType, schema } from "@/dojo/models"; | |||
| import { env, getRpcUrl } from "@/env"; | |||
| import { dojoConfig } from "@/dojo.config"; | |||
| import { dojoConfig } from "../dojoConfig"; | |||
There was a problem hiding this comment.
💡 Codebase verification
Path alias change is inconsistent with project configuration
The change from @/dojo.config to ../dojoConfig should be reverted as the project explicitly configures the @ path alias in both:
tsconfig.jsonwith"@/*": ["./src/*"]vite.config.tswith a corresponding Vite alias configuration
Additionally, there are 42 other imports using the @ alias pattern throughout the codebase, making this change inconsistent with the project's established path resolution strategy.
🔗 Analysis chain
Review build configuration for path alias changes
The change from @/dojo.config to ../dojoConfig removes usage of the @ path alias. Ensure that this change is intentional and consistent with the project's build configuration (tsconfig.json/vite.config.ts).
Let me check the build configuration files to complete the verification.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check build configurations for path alias definitions
# Expected: Confirm if @ alias should be removed or maintained
# Check TypeScript config
fd -e json "tsconfig" examples/example-vite-kitchen-sink/
# Check Vite config
fd -e ts -e js "vite.config" examples/example-vite-kitchen-sink/
# Look for other @ alias usages that might need similar updates
rg "from\s+['\"]@/" examples/example-vite-kitchen-sink/src/
Length of output: 5355
Script:
#!/bin/bash
# Check the content of TypeScript and Vite configs
cat examples/example-vite-kitchen-sink/tsconfig.json
cat examples/example-vite-kitchen-sink/vite.config.ts
Length of output: 1225
MartianGreed
force-pushed
the
fix/create-dojo
branch
from
ca7ed9c to
428cfaf
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
examples/example-vite-kitchen-sink/dojoConfig.ts(1 hunks)examples/example-vite-kitchen-sink/src/main.tsx(1 hunks)packages/create-dojo/src/commands/start.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- examples/example-vite-kitchen-sink/dojoConfig.ts
- examples/example-vite-kitchen-sink/src/main.tsx
🔇 Additional comments (2)
packages/create-dojo/src/commands/start.ts (2)
47-47: LGTM: Improved directory structure
The separation into client and contract directories provides a clearer project structure.
Also applies to: 52-52
69-70: LGTM: Config update properly sequenced
The config rewrite is correctly placed after template cloning to ensure proper path updates.
| const contractRes = spawn.sync( | ||
| "npx", | ||
| ["degit", `dojoengine/dojo-starter`, contractPath], | ||
| { | ||
| stdio: "inherit", | ||
| } | ||
| ); | ||
| if (contractRes.status !== 0) { | ||
| throw new Error(`Failed to clone template: ${template}`); | ||
| } |
There was a problem hiding this comment.
Fix incorrect error message in contract download
The error message still references the template variable when failing to clone the dojo-starter repository.
Apply this fix:
if (contractRes.status !== 0) {
- throw new Error(`Failed to clone template: ${template}`);
+ throw new Error('Failed to clone dojo-starter repository');
}📝 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 contractRes = spawn.sync( | |
| "npx", | |
| ["degit", `dojoengine/dojo-starter`, contractPath], | |
| { | |
| stdio: "inherit", | |
| } | |
| ); | |
| if (contractRes.status !== 0) { | |
| throw new Error(`Failed to clone template: ${template}`); | |
| } | |
| const contractRes = spawn.sync( | |
| "npx", | |
| ["degit", `dojoengine/dojo-starter`, contractPath], | |
| { | |
| stdio: "inherit", | |
| } | |
| ); | |
| if (contractRes.status !== 0) { | |
| throw new Error('Failed to clone dojo-starter repository'); | |
| } |
| async function rewriteDojoConfigFile(clientPath: string) { | ||
| const dojoConfigPath = path.join(clientPath, "dojoConfig.ts"); | ||
|
|
||
| try { | ||
| let content = await fs.readFile(dojoConfigPath, "utf-8"); | ||
|
|
||
| // Update relative imports to account for new directory structure | ||
| content = content.replace( | ||
| /from ['"]\.{0,2}\/.*manifest(?:_dev)?\.json['"]/g, | ||
| 'from "../contract/target/dev/manifest.json"' | ||
| ); | ||
|
|
||
| await fs.writeFile(dojoConfigPath, content, "utf-8"); | ||
| } catch (error) { | ||
| console.warn(`Warning: Could not update dojoConfig.ts: ${error}`); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Revise manifest path handling in rewriteDojoConfigFile
The current implementation needs adjustment based on the examples:
-
The target path
"../contract/target/dev/manifest.json"is incorrect. Examples show two common patterns:- Most examples use:
"../../worlds/dojo-starter/manifest_dev.json" - Kitchen sink uses:
"../../worlds/onchain-dash/manifests/release/deployment/manifest.json"
- Most examples use:
-
The regex pattern should be more specific but flexible enough to handle both patterns.
async function rewriteDojoConfigFile(clientPath: string) {
const dojoConfigPath = path.join(clientPath, "dojoConfig.ts");
+ const manifestPath = "../../worlds/dojo-starter/manifest_dev.json";
try {
let content = await fs.readFile(dojoConfigPath, "utf-8");
content = content.replace(
- /from ['"]\.{0,2}\/.*manifest(?:_dev)?\.json['"]/g,
- 'from "../contract/target/dev/manifest.json"'
+ /from ['"](?:\.{1,2}\/)+(?:worlds|contract)\/.*?manifest(?:_dev)?\.json['"]/g,
+ `from "${manifestPath}"`
);
await fs.writeFile(dojoConfigPath, content, "utf-8");
} catch (error) {
throw new Error(
`Failed to update manifest path in dojoConfig.ts: ${error}. ` +
`Please ensure the file exists and contains a manifest import.`
);
}
}🔗 Analysis chain
Enhance robustness of config file rewriting
Several improvements could make this function more reliable:
- The regex pattern could be more specific to avoid unintended matches
- The hardcoded path assumes a specific structure
- Error handling should be more robust for a critical setup step
Consider this enhanced implementation:
async function rewriteDojoConfigFile(clientPath: string) {
const dojoConfigPath = path.join(clientPath, "dojoConfig.ts");
+ const manifestPath = "../contract/target/dev/manifest.json";
try {
let content = await fs.readFile(dojoConfigPath, "utf-8");
- // Update relative imports to account for new directory structure
content = content.replace(
- /from ['"]\.{0,2}\/.*manifest(?:_dev)?\.json['"]/g,
- 'from "../contract/target/dev/manifest.json"'
+ /from ['"](\.{0,2}\/[^"']*?manifest(?:_dev)?\.json)['"]/g,
+ `from "${manifestPath}"`
);
await fs.writeFile(dojoConfigPath, content, "utf-8");
} catch (error) {
- console.warn(`Warning: Could not update dojoConfig.ts: ${error}`);
+ throw new Error(
+ `Failed to update manifest path in dojoConfig.ts: ${error}. ` +
+ `Please ensure the file exists and contains a manifest import.`
+ );
}
}Let's verify the manifest path structure:
This will help us verify the actual import paths used in the examples to ensure our regex and target path assumptions are correct.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the manifest path structure is consistent across templates
for template in $(fd -t d -d 1 . examples/ | grep 'example-'); do
rg -l "manifest(_dev)?\.json" "$template"
done
Length of output: 1956
Script:
#!/bin/bash
# Check the actual manifest import paths in dojoConfig files
for config in $(rg -l "manifest(_dev)?\.json" examples/); do
echo "=== $config ==="
rg -A 1 -B 1 "from ['\"].*manifest(_dev)?\.json['\"]" "$config"
done
Length of output: 4433
Fixes
create-dojostart command with providing correct path to manifestSummary by CodeRabbit
New Features
Bug Fixes
Refactor