various fixes/ renaming #2657
various fixes/ renaming #2657
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Hi @aymericdelab! You need to be added as a user to interact with me. Please ask @ponderingdemocritus to add you on the settings page. You are receiving this comment because I am set to review all PRs. That is configurable here. |
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
client/apps/landing/src/components/ui/select-resources.tsxOops! Something went wrong! :( ESLint: 9.18.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by WalkthroughThis pull request introduces a comprehensive restructuring of the project's module organization, focusing on consolidating and standardizing import paths, type definitions, and configuration management. The changes span multiple files across different packages, emphasizing a more modular and consistent approach to code organization. Key modifications include renaming import paths, moving type definitions, and updating configuration management strategies. Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant Setup as Setup Module
participant ConfigManager as ClientConfigManager
participant Network as Network Setup
App->>Setup: Initialize application
Setup->>Network: Configure network
Network-->>Setup: Return network components
Setup->>ConfigManager: Set Dojo configuration
ConfigManager-->>Setup: Configuration initialized
Setup-->>App: Return setup result
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
💤 Files with no reviewable changes (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 (
|
|
Failed to generate code suggestions for PR |
![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 comments (1)
packages/core/src/types/common.ts (1)
Line range hint
184-184: Remove unused enum member.The
UNKNOWNmember inEntityTypeenum is unused according to the static analysis tool. Consider removing it if it's not needed or document its purpose if it should be kept.export enum EntityType { DONKEY, TROOP, - UNKNOWN, }
🧹 Nitpick comments (7)
client/apps/game/src/ui/components/worldmap/structures/immunity-timer.tsx (1)
8-16: Consider combining memoized valuesThe two
useMemohooks could be combined since they share the same dependencies and are closely related.- const isImmune = useMemo( - () => isStructureImmune(structure, nextBlockTimestamp || 0), - [structure, nextBlockTimestamp], - ); - - const timer = useMemo( - () => getStructureImmunityTimer(structure, nextBlockTimestamp || 0), - [structure, nextBlockTimestamp], - ); + const { isImmune, timer } = useMemo( + () => ({ + isImmune: isStructureImmune(structure, nextBlockTimestamp || 0), + timer: getStructureImmunityTimer(structure, nextBlockTimestamp || 0) + }), + [structure, nextBlockTimestamp], + );packages/core/src/managers/config-manager.ts (5)
67-93: Remove or clarify commented-out code ininitializeResourceInputsThere is a block of commented-out code in the
initializeResourceInputsmethod. If this code is no longer needed, consider removing it to improve code clarity. If it serves a purpose, add a comment explaining why it is kept.
140-174: Remove or clarify commented-out code ininitializeRealmUpgradeCostsSimilarly, the
initializeRealmUpgradeCostsmethod contains commented-out code. Cleaning up unused code enhances readability.
176-210: Remove or clarify commented-out code ininitializeResourceBuildingCostsThe method
initializeResourceBuildingCostsalso has a significant amount of commented-out code. It's advisable to remove it or provide context.
212-247: Remove or clarify commented-out code ininitializeBuildingCostsThe
initializeBuildingCostsmethod contains commented-out sections. Clearing these would make the codebase cleaner.
66-700: Consider adding null checks where appropriateThroughout the class, methods like
getValueOrDefaultrely on component values that might beundefined. Ensure that all usages safely handleundefinedvalues to prevent runtime errors.🧰 Tools
🪛 Biome (1.9.4)
[error] 531-531: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
packages/core/src/types/common.ts (1)
43-78: Consider adding JSDoc comments for complex types.The new types (
BattleInfo,ArmyInfo,Structure) are well-structured but would benefit from documentation explaining their purpose and property meanings.Example documentation:
+/** + * Represents battle information including structure battles and positioning. + */ export type BattleInfo = ComponentValue<ClientComponents["Battle"]["schema"]> & { isStructureBattle: boolean; position: ComponentValue<ClientComponents["Position"]["schema"]>; }; +/** + * Comprehensive army information including stats, position, and status. + */ export type ArmyInfo = ComponentValue<ClientComponents["Army"]["schema"]> & { // ... existing properties }; +/** + * Represents a game structure with ownership and protection details. + */ export type Structure = ComponentValue<ClientComponents["Structure"]["schema"]> & { // ... existing properties };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (37)
client/apps/game/src/ui/components/structures/worldmap/structure-card.tsx(1 hunks)client/apps/game/src/ui/components/worldmap/armies/army-info-label.tsx(1 hunks)client/apps/game/src/ui/components/worldmap/structures/immunity-timer.tsx(1 hunks)client/apps/game/src/ui/components/worldmap/structures/structure-label.tsx(1 hunks)client/apps/game/src/ui/layouts/world.tsx(0 hunks)client/apps/landing/env.ts(0 hunks)packages/core/src/dojo/create-client-components.ts(1 hunks)packages/core/src/dojo/index.ts(1 hunks)packages/core/src/dojo/setup.ts(1 hunks)packages/core/src/dojo/setupNetwork.ts(1 hunks)packages/core/src/index.ts(1 hunks)packages/core/src/managers/army-manager.ts(1 hunks)packages/core/src/managers/army-movement-manager.ts(1 hunks)packages/core/src/managers/battle-manager.ts(1 hunks)packages/core/src/managers/config-manager.ts(1 hunks)packages/core/src/managers/index.ts(1 hunks)packages/core/src/managers/leaderboard-manager.ts(1 hunks)packages/core/src/managers/market-manager.ts(1 hunks)packages/core/src/managers/resource-inventory-manager.ts(1 hunks)packages/core/src/managers/resource-manager.ts(1 hunks)packages/core/src/managers/stamina-manager.ts(1 hunks)packages/core/src/managers/tile-manager.ts(2 hunks)packages/core/src/managers/utils/army-movement-utils.ts(1 hunks)packages/core/src/managers/utils/index.ts(1 hunks)packages/core/src/managers/utils/leaderboard-utils.ts(1 hunks)packages/core/src/modelManager/ConfigManager.ts(0 hunks)packages/core/src/modelManager/index.ts(0 hunks)packages/core/src/modelManager/types/index.ts(0 hunks)packages/core/src/modelManager/utils/index.ts(0 hunks)packages/core/src/types/common.ts(2 hunks)packages/core/src/utils/army.ts(1 hunks)packages/core/src/utils/entities.ts(2 hunks)packages/core/src/utils/realm.ts(1 hunks)packages/core/src/utils/resources.ts(1 hunks)packages/core/src/utils/structure.ts(1 hunks)packages/core/src/utils/transport.ts(1 hunks)packages/core/src/utils/utils.ts(1 hunks)
💤 Files with no reviewable changes (6)
- packages/core/src/modelManager/utils/index.ts
- packages/core/src/modelManager/ConfigManager.ts
- client/apps/landing/env.ts
- packages/core/src/modelManager/types/index.ts
- packages/core/src/modelManager/index.ts
- client/apps/game/src/ui/layouts/world.tsx
✅ Files skipped from review due to trivial changes (19)
- packages/core/src/managers/utils/index.ts
- client/apps/game/src/ui/components/worldmap/armies/army-info-label.tsx
- packages/core/src/managers/index.ts
- packages/core/src/dojo/create-client-components.ts
- packages/core/src/managers/stamina-manager.ts
- packages/core/src/managers/market-manager.ts
- packages/core/src/index.ts
- packages/core/src/dojo/setupNetwork.ts
- packages/core/src/managers/resource-manager.ts
- packages/core/src/managers/tile-manager.ts
- packages/core/src/utils/resources.ts
- packages/core/src/managers/leaderboard-manager.ts
- packages/core/src/dojo/index.ts
- packages/core/src/utils/army.ts
- packages/core/src/managers/utils/army-movement-utils.ts
- client/apps/game/src/ui/components/structures/worldmap/structure-card.tsx
- packages/core/src/managers/battle-manager.ts
- packages/core/src/managers/army-manager.ts
- packages/core/src/managers/army-movement-manager.ts
🧰 Additional context used
📓 Learnings (2)
client/apps/game/src/ui/components/worldmap/structures/structure-label.tsx (1)
Learnt from: aymericdelab
PR: BibliothecaDAO/eternum#1818
File: client/src/ui/components/worldmap/structures/StructureLabel.tsx:47-0
Timestamp: 2024-11-12T03:28:50.421Z
Learning: In `StructureLabel.tsx`, when `timer` is negative, the immunity message is not shown, so handling negative `timer` values isn't necessary.
client/apps/game/src/ui/components/worldmap/structures/immunity-timer.tsx (1)
Learnt from: aymericdelab
PR: BibliothecaDAO/eternum#1818
File: client/src/ui/components/worldmap/structures/StructureLabel.tsx:47-0
Timestamp: 2024-11-12T03:28:50.421Z
Learning: In `StructureLabel.tsx`, when `timer` is negative, the immunity message is not shown, so handling negative `timer` values isn't necessary.
🪛 GitHub Actions: knip
packages/core/src/types/common.ts
[warning] 184-184: Unused enum member 'UNKNOWN' in EntityType
🪛 Biome (1.9.4)
packages/core/src/managers/config-manager.ts
[error] 531-531: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
🔇 Additional comments (16)
packages/core/src/dojo/setup.ts (1)
Line range hint
1-17: PR objective mismatch.The PR title suggests removing console, but this file doesn't contain any console-related changes. Please clarify if this change is intentional and part of a larger refactoring effort.
client/apps/game/src/ui/components/worldmap/structures/immunity-timer.tsx (1)
5-26: Clean component extraction with proper implementation!The component follows React best practices with proper typing, memoization, and conditional rendering. The extraction from structure-label.tsx improves code organization and reusability.
client/apps/game/src/ui/components/worldmap/structures/structure-label.tsx (1)
5-8: Clean import reorganization!The imports are well-organized and the ImmunityTimer component is properly imported from its new location.
packages/core/src/utils/structure.ts (2)
1-1: PR title doesn't match the changesThe PR title "remove console" doesn't reflect the actual changes, which focus on extracting the ImmunityTimer component and restructuring imports. Consider updating the PR title to better describe the changes, e.g., "refactor: extract ImmunityTimer component".
4-4: Verify import path resolution across the codebaseThe import path change from "../modelManager" to ".." should be verified to ensure consistent module resolution.
✅ Verification successful
Import path change is correct and consistent
The change from "../modelManager" to ".." is valid as the main index.ts properly exports all necessary components, including Structure, through the standardized module structure. This appears to be part of a larger refactoring moving from modelManager/ to managers/.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other imports from modelManager to ensure consistency rg -l "from ['\"].*modelManager['\"]" packages/Length of output: 53
Script:
#!/bin/bash # Check the directory structure and verify exports echo "=== Directory Structure ===" fd . packages/core/src --type f echo -e "\n=== Checking relative imports ===" rg "from ['\"]\.\.['\""]" packages/core/src echo -e "\n=== Checking Structure exports ===" rg "export.*Structure" packages/core/srcLength of output: 2772
Script:
#!/bin/bash # Check index exports and imports echo "=== Main Index Exports ===" cat packages/core/src/index.ts echo -e "\n=== Managers Index Exports ===" cat packages/core/src/managers/index.ts echo -e "\n=== Structure Usage ===" rg "Structure" packages/core/src/Length of output: 14617
packages/core/src/utils/transport.ts (2)
2-2: Confirm updated import path forClientConfigManagerThe import path for
ClientConfigManagerhas been changed. Ensure that the new path'..'correctly resolves to the module and does not introduce any module resolution issues.
Line range hint
5-14: LGTM!The functions
calculateDonkeysNeededandgetTotalResourceWeightare correctly utilizing theClientConfigManager. No issues found.packages/core/src/managers/utils/leaderboard-utils.ts (3)
2-3: Verify the import path forClientConfigManagerandconfigManagerThe import statement for
ClientConfigManagerandconfigManagerhas been updated. Please ensure that importing from'..'is accurate and does not lead to module resolution issues.
Line range hint
22-22: Check for duplicate instantiation ofconfigManagerInside the
calculateCompletionPointsfunction,configManageris instantiated usingClientConfigManager.instance(). IfconfigManageris intended to be a singleton, verify that multiple instantiations do not cause unintended side effects.
Line range hint
7-36: Ensure accurate calculation of contribution pointsThe logic for calculating completion points appears correct, formatting contributions appropriately and computing the initial contribution points. Good job!
packages/core/src/managers/resource-inventory-manager.ts (1)
4-5: LGTM! Import paths standardized.The changes align with the project's standardization efforts:
- Consolidated type imports
- Updated to kebab-case naming convention
packages/core/src/utils/utils.ts (1)
2-2: LGTM! Import path standardized.Import path updated to use the new centralized structure.
packages/core/src/utils/realm.ts (1)
4-4: LGTM! Import paths standardized.Import paths updated to use the new centralized structure:
- configManager from root
- realmsJson from data directory
Also applies to: 6-6
packages/core/src/utils/entities.ts (1)
3-3: LGTM! Improved string handling for Starknet compatibility.The change properly decodes Starknet short strings, which is the correct way to handle strings in this context.
Also applies to: 83-83
packages/core/src/types/common.ts (2)
2-2: LGTM!The import is properly defined and used by the new
DojoAccounttype.
41-41: LGTM!The
DojoAccounttype union is well-defined and provides good type compatibility.
| @@ -1,6 +1,6 @@ | |||
| import { DojoConfig } from "@dojoengine/core"; | |||
| import { createClientComponents } from ".."; | |||
There was a problem hiding this comment.
💡 Codebase verification
Import path should be relative to the same directory
The createClientComponents is exported from create-client-components.ts in the same directory. The import should use ./create-client-components instead of .. to maintain proper module organization and prevent potential circular dependencies.
🔗 Analysis chain
Verify the new import path.
The import path has been changed from a local import to a parent directory import. Let's ensure this change is consistent with the new module organization.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the new import path and check for any potential circular dependencies
# Check if createClientComponents exists in the parent directory
echo "Checking for createClientComponents in parent directory..."
fd --type f --full-path "packages/core/src" -x rg -l "export.*createClientComponents"
# Check for potential circular dependencies
echo "Checking for potential circular dependencies..."
rg -l "from \"\.\.\"" "packages/core/src"
Length of output: 960
Script:
#!/bin/bash
# Check if createClientComponents is re-exported from index.ts
echo "Checking for re-exports in index files..."
fd index.ts --type f --full-path "packages/core/src" -x rg -l "export.*createClientComponents"
# Check the actual usage pattern
echo "Checking actual import usage..."
rg "createClientComponents" "packages/core/src/dojo/setup.ts" -A 2
Length of output: 584
|
|
||
| // Get amounts for each tier | ||
| for (const tier in ResourceTier) { | ||
| if (isNaN(Number(tier))) continue; // Skip non-numeric enum values |
There was a problem hiding this comment.
Use Number.isNaN instead of isNaN
Using isNaN() can lead to unexpected results due to type coercion. It's safer to use Number.isNaN() when checking for NaN values.
Apply this diff to fix the issue:
- if (isNaN(Number(tier))) continue; // Skip non-numeric enum values
+ if (Number.isNaN(Number(tier))) continue; // Skip non-numeric enum values📝 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.
| if (isNaN(Number(tier))) continue; // Skip non-numeric enum values | |
| if (Number.isNaN(Number(tier))) continue; // Skip non-numeric enum values |
🧰 Tools
🪛 Biome (1.9.4)
[error] 531-531: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
| const numerator = Number(adminBank?.owner_fee_num) ?? 0; | ||
| const denominator = Number(adminBank?.owner_fee_denom) ?? 0; | ||
| return numerator / denominator; |
There was a problem hiding this comment.
Prevent potential division by zero in getAdminBankOwnerFee
The denominator might be zero, which would cause a division by zero error. Ensure that you handle the case when denominator is zero.
Apply this diff to fix the issue:
const denominator = Number(adminBank?.owner_fee_denom) ?? 0;
+ if (denominator === 0) {
+ // Handle division by zero appropriately
+ return 0; // Or throw an error if that suits your application's logic
+ }
return numerator / denominator;📝 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 numerator = Number(adminBank?.owner_fee_num) ?? 0; | |
| const denominator = Number(adminBank?.owner_fee_denom) ?? 0; | |
| return numerator / denominator; | |
| const numerator = Number(adminBank?.owner_fee_num) ?? 0; | |
| const denominator = Number(adminBank?.owner_fee_denom) ?? 0; | |
| if (denominator === 0) { | |
| // Handle division by zero appropriately | |
| return 0; // Or throw an error if that suits your application's logic | |
| } | |
| return numerator / denominator; |
| return bankConfig.lpFeesNumerator / bankConfig.lpFeesDenominator; | ||
| } |
There was a problem hiding this comment.
Prevent potential division by zero in getAdminBankLpFee
Similar to the previous issue, there is a risk of dividing by zero if bankConfig.lpFeesDenominator is zero. Add a check to handle this case.
Apply this diff to fix the issue:
const bankConfig = this.getBankConfig();
+ if (bankConfig.lpFeesDenominator === 0) {
+ // Handle division by zero appropriately
+ return 0; // Or throw an error if necessary
+ }
return bankConfig.lpFeesNumerator / bankConfig.lpFeesDenominator;Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
client/apps/game/src/main.tsx (1)
Line range hint
33-96: Consider comprehensive error handling in the init function.The
initfunction handles multiple async operations (setup, config, sync, graphics) but lacks comprehensive error handling. This could lead to a poor user experience if any step fails.Consider wrapping the entire initialization flow in a try-catch:
async function init() { + try { const rootElement = document.getElementById("root"); if (!rootElement) throw new Error("React root not found"); const root = ReactDOM.createRoot(rootElement as HTMLElement); // ... PWA setup ... const backgroundImage = getRandomBackgroundImage(); root.render(<LoadingScreen backgroundImage={backgroundImage} />); const state = useUIStore.getState(); const setupResult = await setup(/* ... */); const eternumConfig = await ETERNUM_CONFIG(); configManager.setDojo(setupResult.components, eternumConfig); await initialSync(setupResult, state); const graphic = new GameRenderer(setupResult); graphic.initScene(); // ... rest of initialization ... + } catch (error) { + console.error('Game initialization failed:', error); + // Render an error UI instead of the game + root?.render( + <div className="error-screen"> + <h1>Failed to initialize game</h1> + <p>Please refresh the page or try again later.</p> + </div> + ); + } }
🧹 Nitpick comments (3)
packages/core/src/dojo/create-system-calls.ts (1)
29-31: Enhance error handling for season pass minting.The
mint_season_passesfunction should include proper error handling to manage provider failures gracefully.Consider adding error handling:
const mint_season_passes = async (props: SystemProps.MintSeasonPassesProps) => { + try { await provider.mint_season_passes(props); + } catch (error) { + throw new Error(`Failed to mint season passes: ${error.message}`); + } };packages/core/src/dojo/setup-network.ts (2)
Line range hint
35-40: Enhance error handling in burner account initialization.The current error handling simply logs to console and continues execution, which could lead to silent failures. Consider:
- Implementing proper error recovery or propagation
- Adding retry mechanism for network operations
- Validating environment variables before usage
Here's a suggested improvement:
- try { - await burnerManager.init(); - if (env.useBurner === true && burnerManager.list().length === 0) { - await burnerManager.create(); - } - } catch (e) { - console.error(e); - } + try { + await burnerManager.init(); + if (env.useBurner === true && burnerManager.list().length === 0) { + await burnerManager.create(); + } + } catch (e) { + console.error('Failed to initialize burner account:', e); + if (env.useBurner === true) { + // Only throw if burner account was requested + throw new Error('Failed to initialize required burner account'); + } + // Log warning if burner account was optional + console.warn('Continuing without burner account initialization'); + }
Line range hint
11-16: Strengthen type definitions for configuration.The environment configuration object could benefit from stronger typing and validation.
Consider adding:
export interface NetworkEnvironmentConfig { vrfProviderAddress: `0x${string}`; // Enforce hex string format useBurner: boolean; } export async function setupNetwork( config: DojoConfig, env: NetworkEnvironmentConfig, ) { // Validate VRF address format if (!env.vrfProviderAddress.match(/^0x[a-fA-F0-9]+$/)) { throw new Error('Invalid VRF provider address format'); } // ... rest of the function }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
client/apps/game/src/dojo/sync.ts(0 hunks)client/apps/game/src/main.tsx(3 hunks)client/apps/game/src/ui/components/structures/construction/structure-construction-menu.tsx(0 hunks)client/apps/landing/src/dojo/createSystemCalls.ts(0 hunks)client/apps/landing/src/dojo/setup.ts(0 hunks)client/apps/landing/src/dojo/setupNetwork.ts(0 hunks)client/apps/landing/src/dojo/world.ts(0 hunks)client/apps/landing/src/main.tsx(3 hunks)packages/core/src/dojo/create-system-calls.ts(2 hunks)packages/core/src/dojo/index.ts(1 hunks)packages/core/src/dojo/setup-network.ts(1 hunks)packages/core/src/dojo/setup.ts(1 hunks)
💤 Files with no reviewable changes (6)
- client/apps/landing/src/dojo/world.ts
- client/apps/game/src/ui/components/structures/construction/structure-construction-menu.tsx
- client/apps/landing/src/dojo/createSystemCalls.ts
- client/apps/landing/src/dojo/setupNetwork.ts
- client/apps/landing/src/dojo/setup.ts
- client/apps/game/src/dojo/sync.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/core/src/dojo/setup.ts
- packages/core/src/dojo/index.ts
🔇 Additional comments (8)
packages/core/src/dojo/create-system-calls.ts (4)
338-346: LGTM! New system calls are properly exported.The new functions are correctly added to the
systemCallsobject, maintaining consistent code style and logical grouping.
33-39: 🛠️ Refactor suggestionAdd validation for lords management operations.
The
attach_lordsanddetach_lordsfunctions should include validation to prevent invalid operations and ensure transaction atomicity.Consider adding validation:
const attach_lords = async (props: SystemProps.AttachLordsProps) => { + // Validate that lords aren't already attached + const isAttached = await provider.checkLordsAttachment(props); + if (isAttached) { + throw new Error('Lords are already attached'); + } await provider.attach_lords(props); }; const detach_lords = async (props: SystemProps.DetachLordsProps) => { + // Validate that lords are actually attached + const isAttached = await provider.checkLordsAttachment(props); + if (!isAttached) { + throw new Error('Lords are not attached'); + } await provider.detach_lords(props); };Likely invalid or redundant comment.
45-55: 🛠️ Refactor suggestionEnhance bridge operations with validation and timeout handling.
The bridge operations need additional safeguards:
- The two-step withdrawal process (
startandfinish) should handle timeouts.- Resource amounts and realm existence should be validated.
- Consider adding transaction status tracking.
Consider implementing these improvements:
const bridge_resources_into_realm = async (props: SystemProps.BridgeResourcesIntoRealmProps) => { + // Validate resource amounts and realm existence + await validateBridgeOperation(props); return await provider.bridge_resources_into_realm(props); }; const bridge_start_withdraw_from_realm = async (props: SystemProps.BridgeStartWithdrawFromRealmProps) => { + // Start withdrawal with timeout + const timeout = setTimeout(() => { + throw new Error('Withdrawal initiation timeout'); + }, WITHDRAWAL_TIMEOUT); + const result = await provider.bridge_start_withdraw_from_realm(props); + clearTimeout(timeout); + + // Store withdrawal state for verification in finish step + await storeWithdrawalState(result); return result; }; const bridge_finish_withdraw_from_realm = async (props: SystemProps.BridgeFinishWithdrawFromRealmProps) => { + // Verify withdrawal state and check timeout + const withdrawalState = await getWithdrawalState(props); + if (!withdrawalState || isWithdrawalExpired(withdrawalState)) { + throw new Error('Invalid or expired withdrawal'); + } return await provider.bridge_finish_withdraw_from_realm(props); };Let's verify the bridge operations implementation:
25-27: Add environment guards for test utility functions.The test utility functions
mint_test_realmandmint_test_lordsshould be protected to prevent accidental usage in production environments.Consider adding environment checks:
const mint_test_realm = async (props: SystemProps.MintTestRealmProps) => { + if (process.env.NODE_ENV === 'production') { + throw new Error('Test utilities are not available in production'); + } await provider.mint_test_realm(props); }; const mint_test_lords = async (props: SystemProps.MintTestLordsProps) => { + if (process.env.NODE_ENV === 'production') { + throw new Error('Test utilities are not available in production'); + } await provider.mint_test_lords(props); };Also applies to: 41-43
packages/core/src/dojo/setup-network.ts (1)
7-7: Verify the new import path for defineContractComponents.The import path has been changed from a local directory to the parent directory. Let's verify that this function exists in the new location and that all related imports are consistently updated.
Let's run the following script to:
- Verify the existence of defineContractComponents in the parent directory
- Check for any inconsistencies in import paths across the codebase
client/apps/landing/src/main.tsx (1)
9-9: LGTM! Improved import organization.The imports have been properly organized, centralizing configuration management through
@bibliothecadao/eternumand separating config utilities.Also applies to: 18-18
client/apps/game/src/main.tsx (2)
3-3: LGTM! Consistent import organization.The imports follow the same pattern as the landing app, maintaining consistency across the codebase.
Also applies to: 23-23
75-77: 🛠️ Refactor suggestionConsider error handling for configuration setup.
Similar to the landing app, the configuration setup lacks error handling.
Consider adding error handling:
+ try { const eternumConfig = await ETERNUM_CONFIG(); configManager.setDojo(setupResult.components, eternumConfig); + } catch (error) { + console.error('Failed to set dojo configuration:', error); + // Consider showing a user-friendly error message + throw error; + }Additionally, verify that
setupResult.componentsis not undefined before using it.✅ Verification successful
Error handling is indeed missing and necessary for configuration setup
The review comment is valid as:
- ETERNUM_CONFIG makes network calls that could fail
- No error handling exists in either the game or landing apps
- setupResult.components is used without validation
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Search for ETERNUM_CONFIG implementation and usage echo "=== ETERNUM_CONFIG Implementation ===" ast-grep --pattern 'const ETERNUM_CONFIG = $_' echo -e "\n=== ETERNUM_CONFIG Usage ===" rg "ETERNUM_CONFIG" -A 2 echo -e "\n=== Configuration Setup Patterns ===" rg "configManager\.setDojo" -A 2 echo -e "\n=== setupResult.components Usage ===" rg "setupResult\.components" -A 2 echo -e "\n=== Error Handling Patterns in Config ===" rg "try.*catch.*config" -B 2 -A 4Length of output: 13726
| const initializeApp = async () => { | ||
| const setupResult = await setup(dojoConfig, { | ||
| vrfProviderAddress: import.meta.env.VITE_PUBLIC_VRF_PROVIDER_ADDRESS, | ||
| useBurner: import.meta.env.VITE_PUBLIC_CHAIN === "local", | ||
| }); | ||
| const eternumConfig = await ETERNUM_CONFIG(); | ||
| ClientConfigManager.instance().setDojo(setupResult.components, eternumConfig); | ||
| return setupResult; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider error handling in initialization.
The initializeApp function encapsulates the setup logic well, but it lacks error handling for potential failures in setup or config loading.
Consider adding try-catch blocks:
const initializeApp = async () => {
+ try {
const setupResult = await setup(dojoConfig, {
vrfProviderAddress: import.meta.env.VITE_PUBLIC_VRF_PROVIDER_ADDRESS,
useBurner: import.meta.env.VITE_PUBLIC_CHAIN === "local",
});
const eternumConfig = await ETERNUM_CONFIG();
ClientConfigManager.instance().setDojo(setupResult.components, eternumConfig);
return setupResult;
+ } catch (error) {
+ console.error('Failed to initialize app:', error);
+ // Consider showing a user-friendly error message in the UI
+ throw error;
+ }
};📝 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 initializeApp = async () => { | |
| const setupResult = await setup(dojoConfig, { | |
| vrfProviderAddress: import.meta.env.VITE_PUBLIC_VRF_PROVIDER_ADDRESS, | |
| useBurner: import.meta.env.VITE_PUBLIC_CHAIN === "local", | |
| }); | |
| const eternumConfig = await ETERNUM_CONFIG(); | |
| ClientConfigManager.instance().setDojo(setupResult.components, eternumConfig); | |
| return setupResult; | |
| }; | |
| const initializeApp = async () => { | |
| try { | |
| const setupResult = await setup(dojoConfig, { | |
| vrfProviderAddress: import.meta.env.VITE_PUBLIC_VRF_PROVIDER_ADDRESS, | |
| useBurner: import.meta.env.VITE_PUBLIC_CHAIN === "local", | |
| }); | |
| const eternumConfig = await ETERNUM_CONFIG(); | |
| ClientConfigManager.instance().setDojo(setupResult.components, eternumConfig); | |
| return setupResult; | |
| } catch (error) { | |
| console.error('Failed to initialize app:', error); | |
| // Consider showing a user-friendly error message in the UI | |
| throw error; | |
| } | |
| }; |
| @@ -40,7 +51,7 @@ | |||
| </div>, | |||
| ); | |||
|
|
|||
| const setupResult = await setup(dojoConfig); | |||
| const setupResult = await initializeApp(); | |||
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
The initialization flow requires error handling for a robust user experience
The review comment is correct - the initialization sequence performs critical async operations without error handling. The suggested try-catch implementation with error UI feedback follows established patterns in the codebase and would prevent undefined application states on initialization failures.
🔗 Analysis chain
Verify error handling in the initialization flow.
The error handling for initializeApp() should be implemented to maintain a good user experience when initialization fails.
Consider wrapping the initialization in a try-catch block:
- const setupResult = await initializeApp();
+ try {
+ const setupResult = await initializeApp();
+ } catch (error) {
+ // Show an error UI instead of the loading screen
+ root.render(
+ <div className="flex-grow flex items-center justify-center text-gold h-screen bg-brown w-screen">
+ <div className="flex flex-col items-center">
+ <TypeH1 className="text-center mt-4 text-ellipsis">Initialization Failed</TypeH1>
+ <div className="relative bottom-1 text-center text-xl">
+ Please refresh the page or try again later.
+ </div>
+ </div>
+ </div>
+ );
+ return;
+ }🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Find implementation of initializeApp
ast-grep --pattern 'function initializeApp() {
$$$
}'
# Also search for async/const implementation
ast-grep --pattern 'const initializeApp = async () => {
$$$
}'
# Look for similar initialization patterns and error UIs
rg -g '!*.test.*' -g '!*.spec.*' 'initializeApp|initialization failed|error UI' -A 3 -B 3
Length of output: 1708
Script:
#!/bin/bash
# Find setup function implementation
ast-grep --pattern 'function setup($$$) {
$$$
}'
ast-grep --pattern 'const setup = async ($$$) => {
$$$
}'
# Look for error boundaries and error handling patterns
rg -g '!*.test.*' -g '!*.spec.*' 'ErrorBoundary|catch\s*\(|onError' -A 2 -B 2
Length of output: 67690
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
client/apps/game/src/three/types/common.ts (1)
10-20: Consider improving naming consistency in HyperstructureTypesNames.The stage numbering in
HyperstructureTypesNamesis inconsistent withStructureProgress:
HyperstructureTypesNamesuses "stage0" through "stage2"StructureProgressuses STAGE_1 through STAGE_3Consider aligning the naming:
export enum HyperstructureTypesNames { - STAGE_1 = "hyperstructure_stage0", - STAGE_2 = "hyperstructure_stage1", - STAGE_3 = "hyperstructure_stage2", + STAGE_1 = "hyperstructure_stage1", + STAGE_2 = "hyperstructure_stage2", + STAGE_3 = "hyperstructure_stage3", }client/apps/game/src/three/scenes/constants.ts (2)
Line range hint
1-269: Consider using TypeScript enums for numeric constants.The
PROGRESS_HALF_THRESHOLDandPROGRESS_FINAL_THRESHOLDconstants could be moved into an enum for better type safety and maintainability.Consider this refactoring:
-export const PROGRESS_HALF_THRESHOLD = 50; -export const PROGRESS_FINAL_THRESHOLD = 100; +export enum ProgressThreshold { + HALF = 50, + FINAL = 100 +}
Line range hint
187-269: Consider extracting material parameters to a separate configuration file.The
MinesMaterialsParamsobject is quite large and specific to materials. Consider moving it to a dedicated materials configuration file for better maintainability.Consider creating a new file
materials-config.tsand moving the parameters there:// materials-config.ts import * as THREE from "three"; export const MinesMaterialsParams: Record< number, { color: THREE.Color; emissive: THREE.Color; emissiveIntensity: number } > = { // ... existing parameters };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
client/apps/game/src/hooks/store/_three-store.ts(1 hunks)client/apps/game/src/three/game-renderer.ts(1 hunks)client/apps/game/src/three/managers/army-manager.ts(1 hunks)client/apps/game/src/three/managers/battle-manager.ts(1 hunks)client/apps/game/src/three/managers/input-manager.ts(1 hunks)client/apps/game/src/three/managers/structure-manager.ts(1 hunks)client/apps/game/src/three/scene-manager.ts(1 hunks)client/apps/game/src/three/scenes/constants.ts(1 hunks)client/apps/game/src/three/scenes/hexagon-scene.ts(1 hunks)client/apps/game/src/three/scenes/hexception.tsx(2 hunks)client/apps/game/src/three/scenes/worldmap.ts(2 hunks)client/apps/game/src/three/systems/system-manager.ts(1 hunks)client/apps/game/src/three/types/common.ts(1 hunks)client/apps/game/src/three/types/index.ts(1 hunks)client/apps/game/src/three/types/systems.ts(1 hunks)client/apps/game/src/types/index.ts(1 hunks)client/apps/game/src/ui/constants.ts(0 hunks)
💤 Files with no reviewable changes (1)
- client/apps/game/src/ui/constants.ts
✅ Files skipped from review due to trivial changes (9)
- client/apps/game/src/three/types/index.ts
- client/apps/game/src/hooks/store/_three-store.ts
- client/apps/game/src/three/game-renderer.ts
- client/apps/game/src/three/managers/structure-manager.ts
- client/apps/game/src/three/managers/army-manager.ts
- client/apps/game/src/three/managers/input-manager.ts
- client/apps/game/src/three/systems/system-manager.ts
- client/apps/game/src/three/managers/battle-manager.ts
- client/apps/game/src/three/scene-manager.ts
🔇 Additional comments (10)
client/apps/game/src/three/scenes/hexagon-scene.ts (1)
13-13: LGTM! Import restructuring improves module organization.The changes consolidate type imports and use relative paths for better encapsulation within the module structure.
Also applies to: 21-21
client/apps/game/src/three/scenes/hexception.tsx (1)
11-11: LGTM! Import restructuring improves type coverage.The changes consolidate type imports and add previously missing types (
BuildingSystemUpdate,RealmSystemUpdate) while maintaining the same relative path structure as other files.Also applies to: 35-35
client/apps/game/src/three/scenes/worldmap.ts (1)
16-16: LGTM! Import restructuring improves type organization.The changes maintain consistency with other files by:
- Using relative paths for system-specific types
- Keeping shared types in the global types module
Also applies to: 36-36
client/apps/game/src/three/types/common.ts (4)
1-3: LGTM! Well-organized imports.The imports are specific and properly scoped to what's needed in the file.
5-8: LGTM! Clear and descriptive scene names.The enum provides a type-safe way to reference different scenes in the game.
43-60: LGTM! Well-structured movement and rendering interfaces.The interfaces are well-designed with:
- Appropriate use of THREE.Vector3 for 3D positions
- Consistent progress tracking
- Clear dimension specifications
22-41: Consider standardizing the owner field structure.The
ownerfield structure is inconsistent between interfaces:
StructureInfo.owneronly hasaddressArmyData.ownerhasaddress,ownerName, andguildNameThis might indicate missing information in
StructureInfoor unnecessary information inArmyData.Let's check if this inconsistency is intentional:
client/apps/game/src/types/index.ts (1)
Line range hint
1-24: LGTM! Good separation of concerns.The file has been streamlined to focus on UI-specific types (views and battle info), which improves code organization. The removed types have been relocated to more appropriate locations based on their domain.
Let's verify the new locations of the removed types:
✅ Verification successful
Verified: Types have been properly relocated to Three.js domain
All removed types have been successfully relocated to
client/apps/game/src/three/types/common.ts, which is a more appropriate location as they are specifically related to Three.js rendering and game state management. This reorganization improves code maintainability by better grouping related types together.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the removed types in their new locations echo "Searching for removed types in new locations..." rg --type ts "enum (SceneName|HyperstructureTypesNames|StructureProgress)" -A 2 rg --type ts "interface (StructureInfo|ArmyData|MovingArmyData|MovingLabelData|RenderChunkSize)" -A 2Length of output: 1983
client/apps/game/src/three/types/systems.ts (1)
1-3: LGTM! Import path change improves module organization.The import path change from "@/types" to "./common" better reflects the local nature of these types and improves module organization.
Let's verify the existence and content of the common types file:
✅ Verification successful
Import path change verified, StructureProgress enum exists in common.ts
The import path change is valid as the StructureProgress enum is properly defined in the common.ts file with the expected stages (0, 1, 2) for structure construction progress tracking.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the existence and content of common.ts echo "Checking common.ts for StructureProgress..." fd -t f "common.ts" -x cat {}Length of output: 16910
client/apps/game/src/three/scenes/constants.ts (1)
11-12: LGTM! Import path changes align with type reorganization.The import path changes from "@/types" to "../types" maintain consistency with the project's type reorganization.
Let's verify the consistency of import paths across related files:
✅ Verification successful
Import paths are consistent across the codebase
The change from "@/types" to "../types" in constants.ts aligns with the established pattern where files outside the types/ directory import from "../types". This is consistent with other files like hexception.tsx.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for consistent import patterns echo "Checking import patterns in related files..." rg --type ts "import.*from.*types.*HyperstructureTypesNames|StructureProgress" -g '!node_modules'Length of output: 1908
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (5)
client/apps/landing/src/components/ui/elements/resource-cost.tsx (1)
Line range hint
55-55: Fix dynamic Tailwind class generation.The current implementation uses template literals to construct Tailwind classes dynamically (
text-${textSize}). This approach can lead to missing styles in production as Tailwind's JIT compiler won't detect classes constructed at runtime.Consider refactoring to use complete class names with conditional logic:
- className={clsx(`relative text-${textSize} font-bold`, props.color)} + className={clsx( + 'relative font-bold', + { + 'text-xxs': textSize === 'xxs', + 'text-xs': textSize === 'xs', + 'text-sm': textSize === 'sm', + 'text-md': textSize === 'md', + 'text-lg': textSize === 'lg', + }, + props.color + )}Apply similar changes to the other dynamic class name:
- className={`text-${textSize} leading-[10px] self-start relative mt-1 font-normal`} + className={clsx( + 'leading-[10px] self-start relative mt-1 font-normal', + { + 'text-xxs': textSize === 'xxs', + 'text-xs': textSize === 'xs', + 'text-sm': textSize === 'sm', + 'text-md': textSize === 'md', + 'text-lg': textSize === 'lg', + } + )}Also applies to: 67-67
client/apps/landing/src/components/ui/select-resources.tsx (3)
Line range hint
12-22: Add proper TypeScript types to improve type safety.The extensive use of
anytypes reduces type safety and negates the benefits of using TypeScript. This could lead to runtime errors and makes the code harder to maintain.Consider using these types:
interface Resource { id: number; // Add other resource properties } interface SelectResourcesProps { selectedResourceIds: number[]; setSelectedResourceIds: (ids: number[]) => void; selectedResourceAmounts: Record<number, number>; setSelectedResourceAmounts: (amounts: Record<number, number>) => void; entity_id: ID; }
Line range hint
89-91: Fix typo in button text.The button text contains an unintended suffix "abcd".
- Add Resourceabcd + Add Resource
Line range hint
11-248: Consider extracting shared logic into custom hooks.There's significant code duplication between
SelectResourcesandSelectSingleResourcecomponents, particularly in resource handling logic and state management.Consider creating these custom hooks:
function useResourceSelection(entityId: ID) { const { getBalance } = useResourceBalance({ entityId }); const addResource = ( selectedResourceIds: number[], setSelectedResourceIds: (ids: number[]) => void, selectedResourceAmounts: Record<number, number>, setSelectedResourceAmounts: (amounts: Record<number, number>) => void, unselectedResources: Resource[] ) => { setSelectedResourceIds([...selectedResourceIds, unselectedResources[0].id]); setSelectedResourceAmounts({ ...selectedResourceAmounts, [unselectedResources[0].id]: divideByPrecision(getBalance(unselectedResources[0].id) || 0), }); }; return { addResource, getBalance }; }This would:
- Reduce code duplication
- Make the components more maintainable
- Centralize resource handling logic
- Make testing easier
client/apps/landing/src/components/modules/season-registration-timer.tsx (1)
Line range hint
39-39: Fix incomplete font style prop.There's a trailing hyphen in the
font-className prop which appears to be incomplete.- <h2 className="text-2xl font-bold text-primary mb-4 font-">Registration Countdown: </h2> + <h2 className="text-2xl font-bold text-primary mb-4">Registration Countdown: </h2>
🧹 Nitpick comments (6)
client/apps/landing/src/components/modules/season-registration-timer.tsx (2)
Line range hint
12-35: Consider extracting time calculation logic.The time calculation logic could be extracted into a separate utility function for better maintainability and reusability.
Here's a suggested refactor:
const calculateTimeLeft = (endTimestamp: number) => { const now = Math.floor(Date.now() / 1000); if (now >= endTimestamp) { return { hours: "00", minutes: "00", seconds: "00" }; } const diff = endTimestamp - now; const hours = Math.floor(diff / 3600); const minutes = Math.floor((diff % 3600) / 60); const seconds = diff % 60; return { hours: String(hours).padStart(2, "0"), minutes: String(minutes).padStart(2, "0"), seconds: String(seconds).padStart(2, "0"), }; };This would simplify the useEffect:
useEffect(() => { if (!registrationEnd) return; const timer = setInterval(() => { setTimeLeft(calculateTimeLeft(Number(registrationEnd))); }, 1000); return () => clearInterval(timer); }, [registrationEnd]);
Line range hint
22-24: Consider using constants for time calculations.The magic numbers in time calculations could be extracted into named constants for better readability.
const SECONDS_PER_HOUR = 3600; const SECONDS_PER_MINUTE = 60;client/apps/landing/src/hooks/context/dojo-context.tsx (2)
Line range hint
39-47: Enhance error message for better debugging.Consider making the error message more descriptive to help developers quickly identify the issue.
- if (currentValue) throw new Error("DojoProvider can only be used once"); + if (currentValue) throw new Error("Multiple DojoProvider instances detected. DojoProvider can only be used once in the component tree.");
Line range hint
82-86: Improve RPC URL configuration handling.The hardcoded fallback URL "http://localhost:5050" might not be suitable for production environments. Consider either:
- Making the fallback URL configurable through environment variables
- Throwing an error if the RPC URL is not set
const rpcProvider = useMemo( () => new RpcProvider({ - nodeUrl: env.VITE_PUBLIC_NODE_URL || "http://localhost:5050", + nodeUrl: env.VITE_PUBLIC_NODE_URL ?? (() => { + throw new Error("VITE_PUBLIC_NODE_URL is required but not set"); + })(), }), [], );client/apps/landing/src/hooks/use-mint-season-pass.tsx (1)
Line range hint
31-40: Clean up commented code and enhance error handling.
- The commented
mint_season_passescode block should be removed if it's no longer needed- Consider enhancing error handling:
- Add specific error types/messages
- Implement proper error recovery
- Consider consolidating the state management using a reducer pattern
- /*await mint_season_passes({ - signer: account, - recipient: recipient ?? account.address, - token_ids: tokenIdsNumberArray, - season_pass_address: seasonPassAddress, - })*/client/apps/landing/src/main.tsx (1)
Line range hint
46-50: Enhance loading screen with progress information.Consider improving the user experience by:
- Adding a progress indicator showing actual loading status
- Providing specific information about what's being loaded (e.g., "Initializing Dojo...", "Loading configuration...")
<div className="flex flex-col items-center animate-pulse"> <img src="/images/logos/eternum-loader.png" className="w-32 sm:w-24 lg:w-24 xl:w-28 2xl:mt-2 mx-auto my-8" /> <TypeH1 className="text-center mt-4 text-ellipsis">Loading...</TypeH1> - <div className="relative bottom-1 text-center text-xl">{`We are experiencing high loading times. Please be patient.`}</div> + <div className="relative bottom-1 text-center text-xl"> + <div>{loadingStep}</div> + <div className="text-sm mt-2">Please be patient while we set up your experience.</div> + </div> </div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (33)
client/apps/game-docs/docs/pages/development/client.mdx(1 hunks)client/apps/landing/src/components/modules/bridge-fees.tsx(1 hunks)client/apps/landing/src/components/modules/bridge-in.tsx(2 hunks)client/apps/landing/src/components/modules/bridge-out-step-1.tsx(2 hunks)client/apps/landing/src/components/modules/bridge-out-step-2.tsx(1 hunks)client/apps/landing/src/components/modules/bridged-resources.tsx(1 hunks)client/apps/landing/src/components/modules/player-leaderboard-panel.tsx(1 hunks)client/apps/landing/src/components/modules/realm-card.tsx(1 hunks)client/apps/landing/src/components/modules/realm-mint-dialog.tsx(1 hunks)client/apps/landing/src/components/modules/season-pass-card.tsx(1 hunks)client/apps/landing/src/components/modules/season-pass-mint-dialog.tsx(1 hunks)client/apps/landing/src/components/modules/season-registration-timer.tsx(1 hunks)client/apps/landing/src/components/modules/top-navigation-view.tsx(1 hunks)client/apps/landing/src/components/modules/transfer-season-pass-dialog.tsx(1 hunks)client/apps/landing/src/components/ui/elements/resource-cost.tsx(1 hunks)client/apps/landing/src/components/ui/select-resources.tsx(1 hunks)client/apps/landing/src/hooks/context/dojo-context.tsx(1 hunks)client/apps/landing/src/hooks/helpers/use-donkey-arrivals.tsx(1 hunks)client/apps/landing/src/hooks/use-bridge.tsx(1 hunks)client/apps/landing/src/hooks/use-donkeys-burned.tsx(1 hunks)client/apps/landing/src/hooks/use-lords-bridged.tsx(1 hunks)client/apps/landing/src/hooks/use-mint-season-pass.tsx(1 hunks)client/apps/landing/src/hooks/use-mint-test-realm.tsx(1 hunks)client/apps/landing/src/hooks/use-player-count.tsx(1 hunks)client/apps/landing/src/hooks/use-structures.tsx(1 hunks)client/apps/landing/src/main.tsx(3 hunks)client/apps/landing/src/routeTree.gen.ts(1 hunks)client/apps/landing/src/routes/claim.lazy.tsx(1 hunks)client/apps/landing/src/routes/index.lazy.tsx(1 hunks)client/apps/landing/src/routes/mint.lazy.tsx(1 hunks)client/apps/landing/src/routes/my-empire.tsx(0 hunks)client/apps/landing/src/routes/trade.lazy.tsx(1 hunks)client/apps/landing/tsconfig.app.tsbuildinfo(0 hunks)
💤 Files with no reviewable changes (2)
- client/apps/landing/tsconfig.app.tsbuildinfo
- client/apps/landing/src/routes/my-empire.tsx
✅ Files skipped from review due to trivial changes (23)
- client/apps/landing/src/components/modules/top-navigation-view.tsx
- client/apps/landing/src/components/modules/season-pass-card.tsx
- client/apps/landing/src/components/modules/realm-mint-dialog.tsx
- client/apps/landing/src/components/modules/transfer-season-pass-dialog.tsx
- client/apps/landing/src/components/modules/realm-card.tsx
- client/apps/landing/src/hooks/use-mint-test-realm.tsx
- client/apps/landing/src/hooks/use-donkeys-burned.tsx
- client/apps/landing/src/components/modules/season-pass-mint-dialog.tsx
- client/apps/landing/src/routes/index.lazy.tsx
- client/apps/landing/src/hooks/use-player-count.tsx
- client/apps/landing/src/components/modules/player-leaderboard-panel.tsx
- client/apps/landing/src/routes/trade.lazy.tsx
- client/apps/landing/src/hooks/use-bridge.tsx
- client/apps/landing/src/hooks/use-structures.tsx
- client/apps/landing/src/components/modules/bridged-resources.tsx
- client/apps/landing/src/components/modules/bridge-fees.tsx
- client/apps/landing/src/hooks/use-lords-bridged.tsx
- client/apps/landing/src/routes/mint.lazy.tsx
- client/apps/landing/src/hooks/helpers/use-donkey-arrivals.tsx
- client/apps/landing/src/components/modules/bridge-out-step-2.tsx
- client/apps/landing/src/routes/claim.lazy.tsx
- client/apps/landing/src/components/modules/bridge-in.tsx
- client/apps/landing/src/components/modules/bridge-out-step-1.tsx
🔇 Additional comments (17)
client/apps/game-docs/docs/pages/development/client.mdx (1)
52-52: Import path update aligns with kebab-case convention.The change from
DojoContexttodojo-contextfollows modern React/TypeScript conventions for file naming. This improves consistency across the codebase.Let's verify that this change is consistent across the documentation and actual implementation:
✅ Verification successful
Import path change is correctly implemented and documented
The new import path
@/hooks/context/dojo-contextaccurately reflects the actual file structure, and the change maintains proper React conventions (kebab-case for files, PascalCase for components).🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the import path exists and check for any inconsistencies # between documentation and implementation # Check if the new import path exists fd -e tsx -e ts "dojo-context\.tsx?$" client/ # Look for any remaining references to the old path rg -i "DojoContext" client/Length of output: 3827
client/apps/landing/src/components/ui/elements/resource-cost.tsx (2)
Line range hint
8-20: LGTM! Well-structured type definition.The new
textSizeprop follows the existing pattern and TypeScript best practices.
5-5: Verify the consistency of the import path.The import path change aligns with the kebab-case naming convention being standardized across the project.
Let's verify the existence and consistency of the resource-icon component:
✅ Verification successful
Import path change is consistent with the codebase
The import path change to
./resource-iconcorrectly reflects the actual file name, and no other files are using the old PascalCase format for imports.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the resource-icon file exists with various extensions fd -e tsx -e ts "resource-icon" client/apps/landing/src/components/ui/elements/ # Verify if there are any remaining instances of PascalCase ResourceIcon imports rg -l "from ['\"].*ResourceIcon" client/apps/landing/src/componentsLength of output: 216
client/apps/landing/src/components/ui/select-resources.tsx (1)
1-1: LGTM! Import paths standardization looks good.The changes to kebab-case naming convention and simplified relative paths improve consistency and maintainability.
Also applies to: 5-9
client/apps/landing/src/components/modules/season-registration-timer.tsx (2)
Line range hint
4-42: Well-structured component with good practices!The component demonstrates several good practices:
- Proper cleanup of timer interval
- Comprehensive edge case handling
- Type-safe state management
- Clear and efficient time calculations
1-1: Verify the updated import path and its consistency.The import path change aligns with the kebab-case standardization effort.
Let's verify the import path exists and check for any inconsistencies in the naming convention:
✅ Verification successful
Import path change verified successfully
The new kebab-case import path exists and exports the required hook. No inconsistencies found in the naming convention across hook files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the new import path exists fd "use-prize-claim.ts" --type f # Check for any remaining camelCase hook files that need to be renamed fd "use[A-Z].*\.ts" --type f # Verify the hook's exports rg -A 5 "export.*useLeaderboardStatus"Length of output: 650
client/apps/landing/src/hooks/context/dojo-context.tsx (2)
Line range hint
11-21: Well-structured type definitions!The interface changes are well-designed:
- Good use of type extension with
SetupResult- Clear separation of concerns between account and network-related types
- Proper handling of nullable types for accounts
2-2: Verify the @bibliothecadao/eternum package dependency.Please ensure that the
@bibliothecadao/eternumpackage is properly listed in the project's dependencies.client/apps/landing/src/hooks/use-mint-season-pass.tsx (1)
7-7: Verify consistent naming convention across the codebase.The import path change follows kebab-case naming convention, which is good. Let's verify that this naming pattern is consistently applied across the codebase.
✅ Verification successful
Naming convention is consistently applied across the codebase
The import path change follows the established kebab-case pattern, which is consistently implemented across all context files and their imports. The only PascalCase reference found is in a commented-out line, which doesn't affect the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining PascalCase context files and their imports # to ensure consistent naming convention echo "Checking for PascalCase context files:" fd -e tsx -e ts ".*Context\." echo -e "\nChecking for inconsistent imports:" rg -g "*.{ts,tsx}" "from ['\"].*Context['\"]"Length of output: 376
Script:
#!/bin/bash # Search for all context-related files and their imports echo "All context-related files:" fd -e tsx -e ts ".*context.*" echo -e "\nAll context-related imports:" rg -g "*.{ts,tsx}" "from ['\"].*context.*['\"]"Length of output: 2737
client/apps/landing/src/main.tsx (3)
29-37: Consider error handling in initialization.The
initializeAppfunction encapsulates the setup logic well, but it lacks error handling for potential failures in setup or config loading.
54-54: The initialization flow requires error handling for a robust user experience.The initialization sequence performs critical async operations without error handling. The suggested try-catch implementation with error UI feedback would prevent undefined application states on initialization failures.
9-9: Verify package version and compatibility.The migration from local implementation to the
@bibliothecadao/eternumpackage requires verification of:
- Package version compatibility
- API consistency between the local and package implementations
Also applies to: 15-15, 18-18
✅ Verification successful
Import changes are correctly implemented
The migration to
@bibliothecadao/eternumpackage is properly configured:
- Package is correctly set up as a workspace dependency
- Required exports (
ClientConfigManagerandsetup) are available in the core package🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check package.json for version and verify package exports echo "Checking package.json for @bibliothecadao/eternum version..." rg -l "@bibliothecadao/eternum" package.json # Verify the exported symbols echo "Verifying exported symbols..." ast-grep --pattern 'export { $$$ClientConfigManager$$$ }' ast-grep --pattern 'export { $$$setup$$$ }'Length of output: 363
Script:
#!/bin/bash # Extract package version and location in package.json echo "Package version and dependency type:" jq -r '.dependencies["@bibliothecadao/eternum"] // .devDependencies["@bibliothecadao/eternum"] // "not found"' package.json # Search for exports in the codebase echo -e "\nSearching for relevant exports:" rg -A 2 "export.*(?:ClientConfigManager|setup)" --type tsLength of output: 2334
client/apps/landing/src/routeTree.gen.ts (5)
Line range hint
1-15: Warning: This is an auto-generated file.This file is automatically generated by TanStack Router and should not be manually modified. Any changes made to this file may be overwritten during the next generation.
Consider adding this file to your
.gitignoreor excluding it from your formatter/linter configurations to prevent accidental modifications.
59-132: Type declarations are consistent with route changes.The interfaces
FileRoutesByPath,FileRoutesByFullPath,FileRoutesByTo, andFileRoutesByIdhave been correctly updated to reflect the removal of the/my-empireroute.
136-153: Route tree configuration is properly updated.The
RootRouteChildreninterface and route tree configuration correctly reflect the current route structure.
Line range hint
155-189: Route manifest is in sync with configuration.The route manifest JSON structure accurately reflects the current route configuration.
19-55: Verify the removal of/my-empireroute.The removal of the
MyEmpireRoutemight impact existing navigation or components that depend on this route.Let's verify the impact:
✅ Verification successful
The
/my-empireroute removal is safeNo references to the removed route were found in the codebase, indicating it was properly cleaned up.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to the removed /my-empire route # Search for references in components, tests, and documentation echo "Searching for references to /my-empire route..." rg -l "my-empire" --type ts --type tsx --type md # Search for navigation links using the route echo "Searching for navigation links using /my-empire..." ast-grep --pattern 'to="/my-empire"'Length of output: 331
Script:
#!/bin/bash # Search for various forms of my-empire route references echo "Searching for my-empire references..." rg -l "my-empire|myEmpire" --type-add 'tsx:*.tsx' --type-add 'ts:*.ts' --type ts --type tsx --type md echo -e "\nSearching for route definitions and imports..." ast-grep --pattern 'import $_ from $$$"$_my-empire$_"' ast-grep --pattern 'createFileRoute("/my-empire")$_' echo -e "\nSearching for navigation components..." ast-grep --pattern '<$_$_Link$_ to="/my-empire"$_>' ast-grep --pattern 'navigate("/my-empire")'Length of output: 602
| const { contract } = useContract({ | ||
| abi, | ||
| address: seasonPassAddress, | ||
| }); | ||
|
|
||
| const { send, error, isPending, isSuccess } = useSendTransaction({}); | ||
| const { send, isPending, isSuccess } = useSendTransaction({}); |
There was a problem hiding this comment.
💡 Codebase verification
Add toast notifications for consistent error handling
The codebase consistently uses Sonner toast for user feedback in similar hooks. Add toast notifications to match the established pattern:
- Import toast from 'sonner'
- Add error notification in the catch block
- Add success notification when the transaction completes
The TODO comment about Starknet React appears to be isolated to this file and can be addressed separately.
🔗 Analysis chain
Consider enhancing error handling and addressing the TODO comment.
While the try-catch block handles errors appropriately, consider:
- Adding user-friendly error messages or toast notifications
- Implementing proper error state management
- Addressing the TODO comment about using "Starknet React in production"
Let's check if there are similar TODO comments or error handling patterns that need attention:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Look for similar patterns across the codebase
echo "Checking for Starknet React TODOs:"
rg "TODO.*Starknet React"
echo -e "\nChecking error handling patterns in hooks:"
rg -A 5 "useSendTransaction.*error" "src/hooks"
Length of output: 404
Script:
#!/bin/bash
# Search for error handling patterns and notifications in the correct directory
echo "Checking error handling in hooks:"
rg -A 5 "useSendTransaction" "client/apps/landing/src/hooks"
echo -e "\nChecking for toast/notification implementations:"
rg "toast|notification" "client/apps/landing/src"
echo -e "\nChecking for other error handling patterns:"
rg "catch.*error" "client/apps/landing/src"
Length of output: 3541
Summary by CodeRabbit
Based on the comprehensive summary, here are the updated release notes:
New Features
ClientConfigManagerto centralize configuration management across the application.Refactoring
Chores
Removals
MyEmpireroute and its associated component.