feat: Svelte #310
feat: Svelte #310
Conversation
WalkthroughThis pull request introduces a new Svelte project template that integrates Vite, Svelte, TypeScript, and the Dojo framework. Key changes include the addition of configuration files, a 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: 34
🧹 Outside diff range and nitpick comments (16)
examples/example-vite-svelte-recs/dojoConfig.ts (1)
1-1: Consider making the manifest path configurable.The hard-coded relative path to the development manifest might make it difficult to switch between different environments (dev, staging, prod). Consider:
- Using environment variables to configure the manifest path
- Moving manifest files closer to the application code
Example implementation using environment variables:
-import manifest from "../../worlds/dojo-starter/manifests/dev/deployment/manifest.json"; +import manifest from process.env.DOJO_MANIFEST_PATH || "../../worlds/dojo-starter/manifests/dev/deployment/manifest.json";examples/example-vite-svelte-recs/svelte.config.js (1)
3-7: Configuration looks good, consider documenting additional preprocessor options.The basic configuration is correct and sufficient for a starter template. However, since this is a starter template, it might be helpful to document common preprocessor options that users might want to add later.
Consider adding more documentation comments about useful preprocessor options:
export default { // Consult https://svelte.dev/docs#compile-time-svelte-preprocess // for more information about preprocessors + // Common options you might want to add later: + // - scss: Enable SCSS/SASS support + // - postcss: Add PostCSS processing + // - babel: Add custom babel transforms preprocess: vitePreprocess(), };examples/example-vite-svelte-recs/vite.config.ts (1)
1-4: Add documentation for plugin purposes.Consider adding brief comments explaining the purpose of each plugin to improve maintainability.
import { defineConfig } from "vite"; +// Enables Svelte component compilation and hot module replacement import { svelte } from "@sveltejs/vite-plugin-svelte"; +// Enables top-level await syntax in modules import topLevelAwait from "vite-plugin-top-level-await"; +// Enables WebAssembly module imports import wasm from "vite-plugin-wasm";scripts/build-examples.sh (1)
Line range hint
20-25: Consider adding directory existence checks.The build loop could be more robust by checking if directories exist before attempting to build them.
Here's a suggested improvement:
# Iterate over each example directory and run the build command for example in "${examples[@]}"; do echo "Building in $example..." + if [ ! -d "$example" ]; then + echo "Warning: Directory $example does not exist, skipping..." + continue + fi cd "$example" pnpm run build cd ../../ doneexamples/example-vite-svelte-recs/src/handlers.ts (1)
1-26: Consider architectural improvements for better maintainability.Recommendations for improving the overall architecture:
- Create a dedicated BurnerService class to encapsulate burner management logic
- Use TypeScript discriminated unions for better error handling
- Add event logging for debugging and monitoring
- Consider implementing the Command pattern for better undo/redo support
Example structure:
interface BurnerService { changeBurner(id: string): Result<void, BurnerError>; createBurner(): Promise<Result<void, BurnerError>>; clearBurners(): Result<void, BurnerError>; } type Result<T, E> = { ok: true; value: T } | { ok: false; error: E }; type BurnerError = | { type: 'NOT_INITIALIZED' } | { type: 'INVALID_BURNER' } | { type: 'CREATION_FAILED' };Would you like me to provide a complete implementation of this improved architecture?
examples/example-vite-svelte-recs/package.json (1)
1-5: Consider adding more package.json metadata fields.The basic metadata is present, but adding fields like
description,repository,license, andengineswould improve package documentation and compatibility checking.{ "name": "svelte-starter", "private": true, "version": "0.0.0", "type": "module", + "description": "Svelte starter template with Dojo Engine integration", + "engines": { + "node": ">=18.0.0" + }, + "license": "MIT"examples/example-vite-svelte-recs/src/dojo/typescript/contracts.gen.ts (3)
4-6: Remove unused importbyteArrayThe
byteArrayimport from starknet is not used anywhere in this file.-import { Account, byteArray } from "starknet"; +import { Account } from "starknet";
28-30: Enhance error logging for better debuggingConsider including the account address in the error log to make debugging easier.
- console.error("Error executing spawn:", error); + console.error(`Error executing spawn for account ${props.account.address}:`, error);
10-86: Consider refactoring for better maintainabilityThe current implementation has several areas that could be improved:
- Error handling is duplicated across all functions
- Contract name and namespace are hardcoded
- Similar execution patterns are repeated
Consider these improvements:
- Create a common error handler:
const handleExecuteError = (action: string, error: unknown, account: Account) => { console.error(`Error executing ${action} for account ${account.address}:`, error); throw error; };
- Move configuration to constants:
const CONFIG = { contractName: 'actions', namespace: 'dojo_starter' } as const;
- Create a helper for contract execution:
const executeAction = async ( account: Account, entrypoint: string, calldata: any[] = [] ) => { try { return await provider.execute( account, { contractName: CONFIG.contractName, entrypoint, calldata, }, CONFIG.namespace ); } catch (error) { handleExecuteError(entrypoint, error, account); } };These changes would make the code more maintainable and reduce duplication.
examples/example-vite-svelte-recs/README.md (3)
1-8: Consider enhancing the setup instructions.While the IDE setup is clear, consider adding:
- Quick start commands (
npm create,npm install,npm run dev)- Additional helpful VS Code extensions (e.g., ESLint, Prettier)
- Prerequisites (Node.js version, npm/yarn)
17-17: Fix grammar in the routing solution point.Add a comma after "solution" as the "which" clause is non-restrictive:
- It brings its own routing solution which might not be preferable for some users. + It brings its own routing solution, which might not be preferable for some users.🧰 Tools
🪛 LanguageTool
[uncategorized] ~17-~17: Possible missing comma found.
Context: ...teKit?** - It brings its own routing solution which might not be preferable for some ...(AI_HYDRA_LEO_MISSING_COMMA)
20-20: Improve phrasing for clarity.Consider these style improvements:
-while taking into account the developer experience with regards to +while considering the developer experience regarding🧰 Tools
🪛 LanguageTool
[style] ~20-~20: ‘taking into account’ might be wordy. Consider a shorter alternative.
Context: ... with Vite + TypeScript + Svelte, while taking into account the developer experience with regards t...(EN_WORDINESS_PREMIUM_TAKING_INTO_ACCOUNT)
[misspelling] ~20-~20: Although “with regards to” is sometimes used in casual speech, it is typically considered a nonstandard phrase.
Context: ...g into account the developer experience with regards to HMR and intellisense. It demonstrates c...(IN_OR_WITH_REGARDS_TO_OF)
examples/example-vite-svelte-recs/src/App.svelte (1)
16-21: Consider initializing state variables with default values.While TypeScript provides type safety, initializing state variables with default values can prevent potential undefined behavior and improve code clarity.
-let entityId: Entity; -let account: Account; -let position: ComponentStore; -let moves: ComponentStore; -let burners: Burner[]; +let entityId: Entity | undefined; +let account: Account | undefined; +let position: ComponentStore | undefined; +let moves: ComponentStore | undefined; +let burners: Burner[] = [];examples/example-vite-svelte-recs/src/dojo/typescript/models.gen.ts (1)
60-111: Add coordinate system documentation and fix primitive types.
- The Position and Vec2 interfaces would benefit from documentation explaining the coordinate system (origin position, axis direction, units).
- Continue fixing primitive type casing throughout these interfaces.
export interface Moves { - player: BigInt; + player: bigint; - remaining: Number; + remaining: number; last_direction: Direction; - can_move: Boolean; + can_move: boolean; } +/** Represents a 2D vector in the game's coordinate system + * @property x - The x-coordinate (TODO: document direction and units) + * @property y - The y-coordinate (TODO: document direction and units) + */ export interface Vec2 { - x: Number; + x: number; - y: Number; + y: number; }🧰 Tools
🪛 Biome
[error] 61-61: Don't use 'BigInt' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'bigint' instead(lint/complexity/noBannedTypes)
[error] 62-62: Don't use 'Number' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'number' instead(lint/complexity/noBannedTypes)
[error] 64-64: Don't use 'Boolean' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'boolean' instead(lint/complexity/noBannedTypes)
[error] 75-75: Don't use 'BigInt' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'bigint' instead(lint/complexity/noBannedTypes)
[error] 76-76: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead(lint/complexity/noBannedTypes)
[error] 85-85: Don't use 'BigInt' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'bigint' instead(lint/complexity/noBannedTypes)
[error] 95-95: Don't use 'Number' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'number' instead(lint/complexity/noBannedTypes)
[error] 96-96: Don't use 'Number' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'number' instead(lint/complexity/noBannedTypes)
[error] 105-105: Don't use 'BigInt' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'bigint' instead(lint/complexity/noBannedTypes)
examples/example-vite-svelte-recs/src/dojo/setup.ts (2)
63-70: Enhance error handling in thecatchblockCurrently, the
catchblock only logs the error to the console. Depending on the criticality of theburnerManagerinitialization, consider implementing more robust error handling. This could include retry mechanisms, user notifications, or rethrowing the error after logging.
33-38: Remove or utilize unused variablessyncandeventSyncThe variables
syncandeventSyncare declared but not used elsewhere in the code. Unused variables can lead to confusion and may affect code readability.Apply this diff to remove the unused variables if they are not needed:
-const sync = await getSyncEntities( - toriiClient, - contractComponents as any, - [] -); -const eventSync = getSyncEvents( - toriiClient, - contractComponents as any, - undefined, - [] -);If these variables are intended for future use, consider adding a comment to indicate their purpose.
Also applies to: 39-44
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
examples/example-vite-svelte-recs/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlexamples/example-vite-svelte-recs/public/vite.svgis excluded by!**/*.svgpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (23)
examples/example-vite-svelte-recs/.gitignore(1 hunks)examples/example-vite-svelte-recs/README.md(1 hunks)examples/example-vite-svelte-recs/dojoConfig.ts(1 hunks)examples/example-vite-svelte-recs/index.html(1 hunks)examples/example-vite-svelte-recs/package.json(1 hunks)examples/example-vite-svelte-recs/src/App.svelte(1 hunks)examples/example-vite-svelte-recs/src/app.css(1 hunks)examples/example-vite-svelte-recs/src/dojo/componentValueStore.ts(1 hunks)examples/example-vite-svelte-recs/src/dojo/createClientComponents.ts(1 hunks)examples/example-vite-svelte-recs/src/dojo/setup.ts(1 hunks)examples/example-vite-svelte-recs/src/dojo/typescript/contracts.gen.ts(1 hunks)examples/example-vite-svelte-recs/src/dojo/typescript/models.gen.ts(1 hunks)examples/example-vite-svelte-recs/src/dojo/world.ts(1 hunks)examples/example-vite-svelte-recs/src/handlers.ts(1 hunks)examples/example-vite-svelte-recs/src/main.ts(1 hunks)examples/example-vite-svelte-recs/src/stores.ts(1 hunks)examples/example-vite-svelte-recs/src/vite-env.d.ts(1 hunks)examples/example-vite-svelte-recs/svelte.config.js(1 hunks)examples/example-vite-svelte-recs/tsconfig.json(1 hunks)examples/example-vite-svelte-recs/tsconfig.node.json(1 hunks)examples/example-vite-svelte-recs/vite.config.ts(1 hunks)packages/create-dojo/src/commands/start.ts(1 hunks)scripts/build-examples.sh(1 hunks)
✅ Files skipped from review due to trivial changes (6)
- examples/example-vite-svelte-recs/.gitignore
- examples/example-vite-svelte-recs/index.html
- examples/example-vite-svelte-recs/src/dojo/world.ts
- examples/example-vite-svelte-recs/src/vite-env.d.ts
- examples/example-vite-svelte-recs/tsconfig.json
- examples/example-vite-svelte-recs/tsconfig.node.json
🧰 Additional context used
🪛 LanguageTool
examples/example-vite-svelte-recs/README.md
[uncategorized] ~11-~11: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...ort for TypeScript, SCSS, and Less, and easily-added support for mdsvex, GraphQL, PostCSS, T...
(HYPHENATED_LY_ADVERB_ADJECTIVE)
[uncategorized] ~17-~17: Possible missing comma found.
Context: ...teKit?** - It brings its own routing solution which might not be preferable for some ...
(AI_HYDRA_LEO_MISSING_COMMA)
[style] ~20-~20: ‘taking into account’ might be wordy. Consider a shorter alternative.
Context: ... with Vite + TypeScript + Svelte, while taking into account the developer experience with regards t...
(EN_WORDINESS_PREMIUM_TAKING_INTO_ACCOUNT)
[misspelling] ~20-~20: Although “with regards to” is sometimes used in casual speech, it is typically considered a nonstandard phrase.
Context: ...g into account the developer experience with regards to HMR and intellisense. It demonstrates c...
(IN_OR_WITH_REGARDS_TO_OF)
[uncategorized] ~40-~40: Possible missing comma found.
Context: ...omponent, consider creating an external store which would not be replaced by HMR. ``...
(AI_HYDRA_LEO_MISSING_COMMA)
🪛 Biome
examples/example-vite-svelte-recs/src/dojo/typescript/models.gen.ts
[error] 39-39: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead
(lint/complexity/noBannedTypes)
[error] 40-40: Don't use 'BigInt' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'bigint' instead
(lint/complexity/noBannedTypes)
[error] 41-41: Don't use 'Number' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'number' instead
(lint/complexity/noBannedTypes)
[error] 51-51: Don't use 'BigInt' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'bigint' instead
(lint/complexity/noBannedTypes)
[error] 61-61: Don't use 'BigInt' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'bigint' instead
(lint/complexity/noBannedTypes)
[error] 62-62: Don't use 'Number' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'number' instead
(lint/complexity/noBannedTypes)
[error] 64-64: Don't use 'Boolean' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'boolean' instead
(lint/complexity/noBannedTypes)
[error] 75-75: Don't use 'BigInt' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'bigint' instead
(lint/complexity/noBannedTypes)
[error] 76-76: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead
(lint/complexity/noBannedTypes)
[error] 85-85: Don't use 'BigInt' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'bigint' instead
(lint/complexity/noBannedTypes)
[error] 95-95: Don't use 'Number' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'number' instead
(lint/complexity/noBannedTypes)
[error] 96-96: Don't use 'Number' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'number' instead
(lint/complexity/noBannedTypes)
[error] 105-105: Don't use 'BigInt' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'bigint' instead
(lint/complexity/noBannedTypes)
🔇 Additional comments (15)
examples/example-vite-svelte-recs/svelte.config.js (1)
1-1: LGTM! Correct import for Vite + Svelte integration.
The import statement correctly uses the official Svelte Vite plugin, which is the recommended approach for Vite-based Svelte projects.
examples/example-vite-svelte-recs/src/stores.ts (2)
1-4: LGTM! Well-structured imports.
The imports are properly organized and follow TypeScript best practices with explicit type imports.
6-9: Verify proper cleanup of sensitive wallet information.
The stores contain sensitive account and wallet information. Ensure proper cleanup when components are unmounted or when the user disconnects.
Let's check how these stores are used across the codebase:
✅ Verification successful
Let me gather more information about how these stores are used and cleaned up.
Proper cleanup mechanism exists for sensitive wallet information
The codebase already implements appropriate cleanup mechanisms:
- The
handleClearBurners()function inhandlers.tsproperly cleans up sensitive data by:- Clearing the burner manager state
- Resetting the burner store to an empty list
- Setting the account store to null
The stores are used in a Svelte-idiomatic way with reactive declarations ($:) and the framework handles subscription cleanup automatically when components are destroyed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check store usage and cleanup patterns
# Look for store cleanup patterns
rg -l "accountStore|burnerStore" | xargs rg "onDestroy|onMount|\$:"
# Check for store subscription cleanup
ast-grep --pattern 'subscribe($$$)'
Length of output: 174
Script:
#!/bin/bash
# Search for store usage patterns more broadly
rg -A 5 "accountStore|burnerStore|dojoStore"
# Look for store unsubscribe patterns
ast-grep --pattern '$_.subscribe(($$$) => {
$$$
})'
# Check for store reset/cleanup patterns
rg -A 3 "set\(null\)|set\(\[\]\)"
Length of output: 9850
examples/example-vite-svelte-recs/src/dojo/createClientComponents.ts (3)
1-2: LGTM! Clean import organization.
Imports follow best practices with external dependencies first and proper type imports.
4-4: LGTM! Excellent type definition approach.
Using ReturnType ensures the type stays synchronized with the function implementation automatically.
6-16: 🛠️ Refactor suggestion
Verify component existence and consider implementation improvements.
While the implementation is functional, there are a few considerations:
- We should verify that 'Position' and 'Moves' components exist in the contract
- The spread operation followed by specific overrides could be risky if component names change
Consider this safer implementation:
export function createClientComponents({
contractComponents,
}: {
contractComponents: ContractComponents;
}) {
return {
- ...contractComponents,
Position: overridableComponent(contractComponents.Position),
Moves: overridableComponent(contractComponents.Moves),
+ ...contractComponents,
};
}Consider adding a JSDoc comment explaining:
- The purpose of wrapping with
overridableComponent - Which components are being wrapped and why
✅ Verification successful
Review concerns have been addressed
Based on the code analysis:
- Both
PositionandMovescomponents are properly defined in the models.gen.ts file with their complete type definitions and are registered in thedefineContractComponentsfunction. - This pattern of wrapping components with
overridableComponentis consistently used across multiple examples in the codebase (vue, react, svelte implementations). - The spread operation concern is not critical as both components are properly defined in the contract components.
The implementation is safe and follows an established pattern across the Dojo framework examples.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify Position and Moves components are defined in the models
rg -A 5 "export (type|interface) Position" "**/*.gen.ts"
rg -A 5 "export (type|interface) Moves" "**/*.gen.ts"
Length of output: 218
Script:
#!/bin/bash
# Let's first find any generated files or type definitions that might contain these components
fd -e ts -e tsx | grep -i "gen\|type\|component"
# Then let's search for Position and Moves component definitions more broadly
rg -l "Position.*Component|Moves.*Component"
# Let's also check the actual usage of these components
ast-grep --pattern 'contractComponents.$_'
Length of output: 7188
Script:
#!/bin/bash
# Let's check the contract components definition in the same directory
cat examples/example-vite-svelte-recs/src/dojo/typescript/contracts.gen.ts
# Also check the models definition
cat examples/example-vite-svelte-recs/src/dojo/typescript/models.gen.ts
# Let's also check what overridableComponent does
rg -A 5 "function overridableComponent"
Length of output: 8709
scripts/build-examples.sh (1)
16-16: Verify the new example directory exists.
The addition follows the correct pattern for example directories.
Let's verify the directory exists and has the required build configuration:
✅ Verification successful
The new example directory exists and has the correct build configuration ✅
The verification confirms:
- Directory
examples/example-vite-svelte-recsexists package.jsonis present- Build script is properly configured with
vite build
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the new Svelte example directory and its build configuration
# Test 1: Check if directory exists
if ! [ -d "examples/example-vite-svelte-recs" ]; then
echo "Error: Directory examples/example-vite-svelte-recs does not exist"
exit 1
fi
# Test 2: Verify package.json exists and contains build script
if ! [ -f "examples/example-vite-svelte-recs/package.json" ]; then
echo "Error: package.json not found"
exit 1
fi
# Test 3: Check for build script in package.json
cat examples/example-vite-svelte-recs/package.json | jq -r '.scripts.build // empty'
Length of output: 204
examples/example-vite-svelte-recs/src/main.ts (1)
1-23: Consider architectural improvements for better maintainability and testing.
As this is a starter template, consider these architectural improvements:
- Extract store initialization into a separate module
- Add configuration validation
- Implement dependency injection for better testing
- Add initialization status management
This will make the template more robust and easier to maintain.
Let's check if there are any existing initialization patterns in the codebase:
examples/example-vite-svelte-recs/package.json (2)
12-20: Verify TypeScript version compatibility.
The TypeScript version ^5.5.3 seems to be a pre-release version that might not be stable yet.
#!/bin/bash
# Check if this version exists in npm registry
npm view typescript versions --json | jq 'map(select(startswith("5.5."))) | .[]'Additionally, consider adding these development dependencies for a better development experience:
"devDependencies": {
"@sveltejs/vite-plugin-svelte": "^3.1.1",
"@tsconfig/svelte": "^5.0.4",
"svelte": "^4.2.18",
"svelte-check": "^3.8.5",
"tslib": "^2.6.3",
"typescript": "^5.5.3",
- "vite": "^5.4.1"
+ "vite": "^5.4.1",
+ "vitest": "^1.0.0",
+ "prettier": "^3.0.0",
+ "prettier-plugin-svelte": "^3.0.0",
+ "eslint": "^8.0.0",
+ "eslint-plugin-svelte": "^2.0.0"21-32: Review workspace dependencies and version constraints.
Several dependencies use workspace:* which is good for monorepo development, but consider:
- The mix of workspace and fixed version dependencies might cause compatibility issues
- Some dependencies might need peer dependency declarations
Also, consider adding explicit peer dependencies:
"dependencies": {
"@dojoengine/core": "workspace:*",
"@dojoengine/create-burner": "workspace:*",
"@dojoengine/recs": "^2.0.13",
"@dojoengine/state": "workspace:*",
"@dojoengine/torii-client": "workspace:*",
"@dojoengine/utils": "workspace:*",
"@latticexyz/utils": "^2.1.1",
"starknet": "^6.11.0",
"vite-plugin-top-level-await": "^1.4.4",
"vite-plugin-wasm": "^3.3.0"
- }
+ },
+ "peerDependencies": {
+ "react": "^18.0.0",
+ "react-dom": "^18.0.0"
+ }examples/example-vite-svelte-recs/src/App.svelte (1)
1-15: LGTM! Well-organized imports and good separation of concerns.
The imports are properly structured, and the separation of handlers into a dedicated file promotes maintainability.
examples/example-vite-svelte-recs/src/dojo/typescript/models.gen.ts (2)
11-21: 🛠️ Refactor suggestion
Consider using TypeScript enum for Direction type.
Converting the Direction type to an enum would provide better type safety, autocomplete support, and runtime validation.
-export type Direction =
- | { type: "None" }
- | { type: "Left" }
- | { type: "Right" }
- | { type: "Up" }
- | { type: "Down" };
+export enum Direction {
+ None = "None",
+ Left = "Left",
+ Right = "Right",
+ Up = "Up",
+ Down = "Down"
+}Likely invalid or redundant comment.
174-191: Verify Position component metadata types.
The Position component's metadata types array only includes "ContractAddress" while the component actually uses both ContractAddress and Vec2. Ensure the types array accurately reflects all used types.
examples/example-vite-svelte-recs/src/dojo/componentValueStore.ts (2)
49-51: Verify that update.value contains the expected component data
When processing updates in the subscription, it's assumed that update.value contains the component data. To prevent potential runtime errors, ensure that update.value is defined and contains the necessary information before using it.
Consider adding a check for nextValue before setting the store:
const [nextValue] = update.value;
+if (nextValue !== undefined) {
set(nextValue as ComponentValue<S>);
+}This safeguard ensures that the store is only updated when valid component data is available.
18-58: Well-structured implementation of componentValueStore
The componentValueStore function is effectively designed, utilizing Svelte's reactive stores and properly handling subscriptions with cleanup. The logic for updating the store based on component and entity changes is sound and follows best practices.
| export const dojoConfig = createDojoConfig({ | ||
| manifest, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type safety to the configuration.
The configuration lacks type information which could lead to runtime errors if the manifest structure changes.
Consider adding type information:
+import type { DojoConfig } from "@dojoengine/core";
+import type { Manifest } from "@dojoengine/core";
-export const dojoConfig = createDojoConfig({
+export const dojoConfig: DojoConfig = createDojoConfig<Manifest>({
manifest,
});📝 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.
| export const dojoConfig = createDojoConfig({ | |
| manifest, | |
| }); | |
| import type { DojoConfig } from "@dojoengine/core"; | |
| import type { Manifest } from "@dojoengine/core"; | |
| export const dojoConfig: DojoConfig = createDojoConfig<Manifest>({ | |
| manifest, | |
| }); |
💡 Codebase verification
The manifest import path is incorrect and needs to be fixed
The import statement references a manifest file at ../../worlds/dojo-starter/manifests/dev/deployment/manifest.json, but this file does not exist in the repository. While the worlds/dojo-starter directory exists, it appears to be empty or doesn't contain the expected manifest structure.
- The manifest file should either be:
- Added to the expected location
- Or the import path should be updated to point to the correct manifest location
🔗 Analysis chain
Verify manifest path resolution.
The relative import path traverses outside the example directory. Let's verify that this path resolves correctly.
Let me check the actual import statement and manifest location in the codebase.
Let me verify if the referenced manifest path exists relative to the example directory.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the manifest file exists and is accessible
# Test 1: Check if the manifest file exists
if [ -f "worlds/dojo-starter/manifests/dev/deployment/manifest.json" ]; then
echo "✓ Manifest file exists"
# Test 2: Verify it's a valid JSON file
if jq empty "worlds/dojo-starter/manifests/dev/deployment/manifest.json" 2>/dev/null; then
echo "✓ Manifest is valid JSON"
else
echo "✗ Invalid JSON in manifest file"
fi
else
echo "✗ Manifest file not found"
fi
# Test 3: Check for other manifest files that might be relevant
echo "Other manifest files in the project:"
fd -e json -p "manifest.*\.json$"
Length of output: 251
Script:
#!/bin/bash
# Check the actual import statement in the file
echo "Content of dojoConfig.ts:"
cat examples/example-vite-svelte-recs/dojoConfig.ts
echo -e "\nSearching for manifest files in or near the example directory:"
fd manifest examples/example-vite-svelte-recs -e json
echo -e "\nChecking for manifest-related imports:"
rg "import.*manifest" examples/example-vite-svelte-recs
Length of output: 806
Script:
#!/bin/bash
# Check if the manifest exists at the relative path from the example
cd examples/example-vite-svelte-recs
if [ -f "../../worlds/dojo-starter/manifests/dev/deployment/manifest.json" ]; then
echo "✓ Manifest file exists at the relative path"
jq empty "../../worlds/dojo-starter/manifests/dev/deployment/manifest.json" 2>/dev/null && echo "✓ Valid JSON" || echo "✗ Invalid JSON"
else
echo "✗ Manifest file not found at the relative path"
echo -e "\nListing contents of parent directories to debug:"
ls -la ../..
echo -e "\nListing worlds directory if it exists:"
ls -la ../../worlds 2>/dev/null || echo "worlds directory not found"
fi
Length of output: 2346
| export default defineConfig({ | ||
| plugins: [svelte(), wasm(), topLevelAwait()], | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing the Vite configuration with additional options.
While the current setup is functional, consider adding these commonly used Vite options to improve the development experience:
basefor deployment path configurationbuild.targetfor browser compatibilityserveroptions for development settings
Here's an enhanced configuration:
export default defineConfig({
- plugins: [svelte(), wasm(), topLevelAwait()],
+ plugins: [svelte(), wasm(), topLevelAwait()],
+ base: './',
+ build: {
+ target: 'esnext',
+ sourcemap: true
+ },
+ server: {
+ port: 3000,
+ open: true
+ }
});📝 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.
| export default defineConfig({ | |
| plugins: [svelte(), wasm(), topLevelAwait()], | |
| }); | |
| export default defineConfig({ | |
| plugins: [svelte(), wasm(), topLevelAwait()], | |
| base: './', | |
| build: { | |
| target: 'esnext', | |
| sourcemap: true | |
| }, | |
| server: { | |
| port: 3000, | |
| open: true | |
| } | |
| }); |
| export const dojoStore = writable<SetupResult>(); | ||
| export const accountStore = writable<Account | null>(); | ||
|
|
||
| export const burnerStore = writable<Burner[]>(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing store declarations with initial values and documentation.
While the stores are properly typed, there are several improvements that could make them more robust:
- Add initial values to prevent undefined states
- Include JSDoc documentation explaining each store's purpose
- Consider adding type guards for state transitions
Here's a suggested implementation:
+/**
+ * Stores the Dojo setup configuration and game world state
+ */
-export const dojoStore = writable<SetupResult>();
+export const dojoStore = writable<SetupResult | null>(null);
+/**
+ * Manages the current account state for user authentication
+ */
-export const accountStore = writable<Account | null>();
+export const accountStore = writable<Account | null>(null);
+/**
+ * Tracks available burner wallets for the current session
+ */
-export const burnerStore = writable<Burner[]>();
+export const burnerStore = writable<Burner[]>([]);Additionally, consider creating derived stores for computed values:
import { derived } from 'svelte/store';
export const isConnected = derived(
accountStore,
$account => $account !== null
);| import "./app.css"; | ||
| import App from "./App.svelte"; | ||
| import { setup } from "./dojo/setup"; | ||
| import { dojoConfig } from "../dojoConfig"; | ||
| import { accountStore, burnerStore, dojoStore } from "./stores"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider configuring path aliases for better maintainability.
Instead of using relative paths, consider setting up path aliases in your TypeScript configuration. This would make imports more maintainable and easier to refactor.
Example configuration in tsconfig.json:
{
"compilerOptions": {
"baseUrl": ".",
"paths": {
"@components/*": ["src/components/*"],
"@stores/*": ["src/stores/*"],
"@dojo/*": ["src/dojo/*"]
}
}
}Then imports would look cleaner:
import App from "@components/App.svelte";
import { setup } from "@dojo/setup";
import { accountStore, burnerStore, dojoStore } from "@stores/index";| return app; | ||
| } | ||
|
|
||
| export default initApp(); |
There was a problem hiding this comment.
Consider a more controlled initialization approach.
Immediately executing the initialization function on import could cause issues:
- The DOM might not be ready
- Testing becomes more difficult
- Race conditions could occur
Consider this alternative approach:
-export default initApp();
+export { initApp };Then in your HTML or entry point:
import { initApp } from './main';
document.addEventListener('DOMContentLoaded', () => {
initApp().catch(error => {
console.error('Failed to initialize app:', error);
});
});| $entity != null | ||
| ? (getComponentValue( | ||
| component, | ||
| $entity | ||
| ) as ComponentValue<S>) | ||
| : (defaultValue as ComponentValue<S>) | ||
| ); |
There was a problem hiding this comment.
Handle possible undefined return value from getComponentValue
The getComponentValue function may return undefined if the component value for the given entity doesn't exist. Casting the result directly to ComponentValue<S> could lead to unexpected undefined values in the store. Consider explicitly handling the undefined case to ensure the store reflects the component's value accurately.
You can modify the code to provide a fallback when getComponentValue returns undefined:
set(
$entity != null
? (
- getComponentValue(
+ getComponentValue(
component,
$entity
- ) as ComponentValue<S>)
+ ) ?? (defaultValue as ComponentValue<S>)
+ ) as ComponentValue<S>
: (defaultValue as ComponentValue<S>)
);This adjustment ensures that if the component value is undefined, the store will use the defaultValue instead.
📝 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.
| $entity != null | |
| ? (getComponentValue( | |
| component, | |
| $entity | |
| ) as ComponentValue<S>) | |
| : (defaultValue as ComponentValue<S>) | |
| ); | |
| $entity != null | |
| ? (getComponentValue( | |
| component, | |
| $entity | |
| ) ?? (defaultValue as ComponentValue<S>) | |
| ) as ComponentValue<S> | |
| : (defaultValue as ComponentValue<S>) | |
| ); |
| masterAccount: new Account( | ||
| { | ||
| nodeUrl: config.rpcUrl, | ||
| }, | ||
| config.masterAddress, | ||
| config.masterPrivateKey | ||
| ), |
There was a problem hiding this comment.
Ensure secure handling of sensitive information
The BurnerManager is initialized with config.masterPrivateKey. Make sure that this private key is securely stored and not exposed in the codebase or configuration files. It's recommended to use environment variables or a secure secrets management system to handle such sensitive data.
|
|
||
| export type SetupResult = Awaited<ReturnType<typeof setup>>; | ||
|
|
||
| export async function setup({ ...config }: DojoConfig) { |
There was a problem hiding this comment.
Possible incorrect use of spread operator in function parameters
Using { ...config } in the function parameter may not have the intended effect. The spread operator within parameter destructuring is not standard and might not work as expected. Consider passing config directly, and if you need to clone or manipulate it, do so within the function body.
Apply this diff to fix the function signature:
-export async function setup({ ...config }: DojoConfig) {
+export async function setup(config: DojoConfig) {📝 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.
| export async function setup({ ...config }: DojoConfig) { | |
| export async function setup(config: DojoConfig) { |
| publish: (typedData: string, signature: ArraySignatureType) => { | ||
| toriiClient.publishMessage(typedData, signature); | ||
| }, |
There was a problem hiding this comment.
Handle potential errors when publishing messages
The publish method doesn't handle errors that may occur during toriiClient.publishMessage. If this method returns a promise that can be rejected, it's important to handle any potential exceptions to prevent unhandled promise rejections.
Apply this diff to add error handling:
-publish: (typedData: string, signature: ArraySignatureType) => {
- toriiClient.publishMessage(typedData, signature);
+publish: async (typedData: string, signature: ArraySignatureType) => {
+ try {
+ await toriiClient.publishMessage(typedData, signature);
+ } catch (error) {
+ console.error('Failed to publish message:', error);
+ // Additional error handling as needed
+ }
}📝 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.
| publish: (typedData: string, signature: ArraySignatureType) => { | |
| toriiClient.publishMessage(typedData, signature); | |
| }, | |
| publish: async (typedData: string, signature: ArraySignatureType) => { | |
| try { | |
| await toriiClient.publishMessage(typedData, signature); | |
| } catch (error) { | |
| console.error('Failed to publish message:', error); | |
| // Additional error handling as needed | |
| } | |
| }, |
| @@ -0,0 +1,87 @@ | |||
| import type { DojoConfig } from "@dojoengine/core"; | |||
| import { DojoProvider } from "@dojoengine/core"; | |||
| import * as torii from "@dojoengine/torii-client"; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Import only necessary members from @dojoengine/torii-client
Currently, the entire torii-client module is imported, which can increase bundle size unnecessarily. Import only the required functions or classes to optimize performance.
Apply this diff to import specific members:
-import * as torii from "@dojoengine/torii-client";
+import { createClient, publishMessage } from "@dojoengine/torii-client";Update the usage accordingly:
-const toriiClient = await torii.createClient({
+const toriiClient = await createClient({
...
-publish: async (typedData: string, signature: ArraySignatureType) => {
- await toriiClient.publishMessage(typedData, signature);
+publish: async (typedData: string, signature: ArraySignatureType) => {
+ await toriiClient.publishMessage(typedData, signature);Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
examples/example-vite-svelte-recs/src/dojo/setup.ts (1)
33-45: Document or configure sync parameters.Both
getSyncEntitiesandgetSyncEventsare called withundefinedand empty arrays as parameters. This might indicate:
- Missing configuration
- Incomplete setup
- Unused functionality
Consider either documenting why these are empty or configuring them properly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
examples/example-vue-app-recs/src/dojo/generated/setup.tsis excluded by!**/generated/**
📒 Files selected for processing (2)
examples/example-vite-svelte-recs/src/dojo/setup.ts(1 hunks)examples/example-vite-svelte-recs/tsconfig.node.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/example-vite-svelte-recs/tsconfig.node.json
🔇 Additional comments (1)
examples/example-vite-svelte-recs/src/dojo/setup.ts (1)
17-22: Validate configuration parameters for Torii client.
The empty relay URL and fallback to empty string for world address might cause issues:
relayUrl: ""- If relay functionality is needed, this empty value could cause connection problemsworldAddress: config.manifest.world.address || ""- Falling back to an empty string might mask configuration errors
| return { | ||
| client, | ||
| clientComponents, | ||
| contractComponents, | ||
| publish: (typedData: string, signature: ArraySignatureType) => { | ||
| toriiClient.publishMessage(typedData, signature); | ||
| }, | ||
| config, | ||
| dojoProvider, | ||
| burnerManager, | ||
| toriiClient, | ||
| eventSync, | ||
| torii, | ||
| sync, | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Limit exposed API surface.
Exposing the entire torii instance in the return object could lead to unintended usage patterns and make it harder to maintain backward compatibility. Consider exposing only the necessary methods and properties.
return {
client,
clientComponents,
contractComponents,
publish,
config,
dojoProvider,
burnerManager,
toriiClient,
eventSync,
- torii,
sync,
};📝 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 { | |
| client, | |
| clientComponents, | |
| contractComponents, | |
| publish: (typedData: string, signature: ArraySignatureType) => { | |
| toriiClient.publishMessage(typedData, signature); | |
| }, | |
| config, | |
| dojoProvider, | |
| burnerManager, | |
| toriiClient, | |
| eventSync, | |
| torii, | |
| sync, | |
| }; | |
| return { | |
| client, | |
| clientComponents, | |
| contractComponents, | |
| publish: (typedData: string, signature: ArraySignatureType) => { | |
| toriiClient.publishMessage(typedData, signature); | |
| }, | |
| config, | |
| dojoProvider, | |
| burnerManager, | |
| toriiClient, | |
| eventSync, | |
| sync, | |
| }; |
| try { | ||
| await burnerManager.init(); | ||
| if (burnerManager.list().length === 0) { | ||
| await burnerManager.create(); | ||
| } | ||
| } catch (e) { | ||
| console.error(e); | ||
| } |
There was a problem hiding this comment.
Improve error handling specificity.
The current catch-all error handling might mask specific issues. Consider:
- Adding specific error types for different failure scenarios
- Implementing appropriate recovery or fallback mechanisms
- Providing more detailed error information to aid debugging
try {
await burnerManager.init();
if (burnerManager.list().length === 0) {
await burnerManager.create();
}
-} catch (e) {
- console.error(e);
+} catch (error) {
+ if (error instanceof BurnerManagerInitError) {
+ console.error('Failed to initialize burner manager:', error);
+ // Specific recovery logic
+ } else if (error instanceof BurnerCreationError) {
+ console.error('Failed to create burner:', error);
+ // Specific recovery logic
+ } else {
+ console.error('Unexpected error in burner setup:', error);
+ throw error; // Re-throw unexpected errors
+ }
}Committable suggestion was skipped due to low confidence.
| const sync = await getSyncEntities( | ||
| toriiClient, | ||
| contractComponents as any, | ||
| undefined, | ||
| [] | ||
| ); |
There was a problem hiding this comment.
Remove unsafe type assertion.
The as any type assertion bypasses TypeScript's type checking system. Consider defining proper types for the contract components.
-contractComponents as any,
+contractComponents,Committable suggestion was skipped due to low confidence.
Summary by CodeRabbit
Release Notes
New Features
Appcomponent for managing application state and user interactions.burnerManagerto handle burner state management.Improvements
Bug Fixes
Chores