refactor + knip + prettier #2669
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
|
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. |
WalkthroughThis pull request introduces a comprehensive set of changes across multiple files, primarily focusing on modifying asynchronous function calls to synchronous ones, updating import paths, and adjusting configuration files. The changes span various modules including game configuration, resource management, and TypeScript configurations. Key modifications include removing async/await keywords, introducing new path aliases like Changes
Sequence DiagramsequenceDiagram
participant Client
participant ConfigUtils
participant ContractUtils
Client->>ConfigUtils: Request Configuration
ConfigUtils->>ContractUtils: Get Game Manifest
ContractUtils-->>ConfigUtils: Return Manifest
ConfigUtils-->>Client: Return Synchronous Configuration
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
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: 2
🔭 Outside diff range comments (2)
contracts/season_resources/scripts/deployment/config/index.ts (1)
Line range hint
24-37: Replace arbitrary delay with proper transaction confirmation.The current implementation uses an arbitrary 3-second delay after setting the whitelist config. This is unreliable and should be replaced with proper transaction confirmation.
Consider this approach:
- console.log(`Finished configuring whitelist for in-game asset bridge ${tx.statusReceipt}`); - // wait for 3 seconds - await new Promise((resolve) => setTimeout(resolve, 3000)); + console.log(`Configuring whitelist for in-game asset bridge...`); + await provider.waitForTransaction(tx.transaction_hash); + console.log(`Finished configuring whitelist for in-game asset bridge`);client/apps/game/src/ui/modules/rewards/rewards.tsx (1)
Line range hint
86-96: Remove duplicatesetIsLoading(false)call.There's a duplicate
setIsLoading(false)call after thefinallyblock. Since the loading state is already being reset in thefinallyblock, this duplicate call is unnecessary and should be removed.} catch (error) { console.error("Error claiming rewards", error); } finally { setIsLoading(false); } - setIsLoading(false);
🧹 Nitpick comments (3)
client/apps/game/dojoConfig.ts (1)
Line range hint
31-34: Remove debug console.log statementsThese console.log statements appear to be debug code and should be removed.
const config = ETERNUM_CONFIG(); -console.log("logging eternum configuration json from file"); -console.log({ config });contracts/utils/utils.ts (1)
43-45: Consider making GameManifest interface more specificThe current interface allows any key-value pairs. Consider defining specific required properties to improve type safety.
Example improvement:
-interface GameManifest { - [key: string]: unknown; -} +interface GameManifest { + version: string; + name: string; + contracts: Record<string, string>; + // Add other known properties + [key: string]: unknown; // For backward compatibility +}client/apps/landing/src/components/modules/bridge-fees.tsx (1)
Line range hint
27-30: Consider extracting fee calculation logic to a utility function.The fee calculation logic could be moved to a separate utility function to improve maintainability and reusability. This would also help reduce the component's responsibilities.
Consider creating a utility file like this:
// utils/bridge-fees.ts export const calculateBridgeFeeDisplayPercent = (percent: number) => { return (percent * 100) / BRIDGE_FEE_DENOMINATOR; }; export const calculateBridgeFee = (percent: number, amount: string) => { return (percent * Number(amount)) / BRIDGE_FEE_DENOMINATOR; };Then import and use these utilities in the component:
-const calculateBridgeFee = (percent: number, amount: string) => { - return (percent * Number(amount)) / BRIDGE_FEE_DENOMINATOR; -}; +import { calculateBridgeFee, calculateBridgeFeeDisplayPercent } from '../utils/bridge-fees'; export const BridgeFees = ({ isOpen, onOpenChange, resourceSelections, type, setResourceFees, }: FeesCollapsibleProps) => { const bridgeConfig = configManager.getResourceBridgeFeeSplitConfig(); - - const calculateBridgeFeeDisplayPercent = (percent: number) => { - return (percent * 100) / BRIDGE_FEE_DENOMINATOR; - };Also applies to: 36-40
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (50)
.knip.json(2 hunks)client/apps/game/dojoConfig.ts(3 hunks)client/apps/game/src/dojo/debounced-queries.ts(0 hunks)client/apps/game/src/dojo/queries.ts(0 hunks)client/apps/game/src/hooks/context/policies.ts(0 hunks)client/apps/game/src/hooks/context/starknet-provider.tsx(1 hunks)client/apps/game/src/hooks/helpers/use-quests.ts(1 hunks)client/apps/game/src/three/managers/biome.ts(3 hunks)client/apps/game/src/ui/components/worldmap/armies/selected-army.tsx(1 hunks)client/apps/game/src/ui/modules/onboarding/steps.tsx(1 hunks)client/apps/game/src/ui/modules/rewards/rewards.tsx(2 hunks)client/apps/game/src/utils/addresses.ts(1 hunks)client/apps/game/src/utils/biome/fixed-point.ts(9 hunks)client/apps/game/src/utils/biome/simplex-noise.ts(1 hunks)client/apps/game/src/utils/biome/vec3.ts(1 hunks)client/apps/game/src/utils/biome/vec4.ts(1 hunks)client/apps/game/src/utils/config.ts(1 hunks)client/apps/game/tsconfig.json(2 hunks)client/apps/game/vite.config.ts(1 hunks)client/apps/landing/dojoConfig.ts(2 hunks)client/apps/landing/src/components/modules/bridge-fees.tsx(1 hunks)client/apps/landing/src/components/modules/bridge-in.tsx(1 hunks)client/apps/landing/src/components/modules/bridge-out-step-1.tsx(1 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(0 hunks)client/apps/landing/src/components/providers/starknet-provider.tsx(1 hunks)client/apps/landing/src/components/ui/utils/addresses.ts(1 hunks)client/apps/landing/src/config.ts(1 hunks)client/apps/landing/src/hooks/use-lords-bridged.tsx(1 hunks)client/apps/landing/src/hooks/use-player-count.tsx(0 hunks)client/apps/landing/src/hooks/use-rewards.tsx(1 hunks)client/apps/landing/src/hooks/use-structures.tsx(0 hunks)client/apps/landing/src/utils/config.ts(1 hunks)client/apps/landing/tsconfig.json(2 hunks)client/apps/landing/vite.config.ts(1 hunks)client/update-policies.js(1 hunks)config/deployer/config.ts(2 hunks)config/deployer/index.ts(1 hunks)config/environments/_shared_.ts(3 hunks)config/tsconfig.json(1 hunks)config/utils/utils.ts(2 hunks)contracts/season_resources/scripts/deployment/config/index.ts(2 hunks)contracts/utils/utils.ts(1 hunks)packages/core/src/constants/quests.ts(1 hunks)packages/core/src/constants/utils.ts(1 hunks)packages/core/src/managers/resource-manager.ts(0 hunks)packages/core/tsconfig.json(1 hunks)packages/react/tsconfig.json(1 hunks)packages/tsconfig.base.json(1 hunks)
💤 Files with no reviewable changes (7)
- client/apps/game/src/hooks/context/policies.ts
- client/apps/landing/src/hooks/use-player-count.tsx
- client/apps/landing/src/hooks/use-structures.tsx
- packages/core/src/managers/resource-manager.ts
- client/apps/game/src/dojo/debounced-queries.ts
- client/apps/game/src/dojo/queries.ts
- client/apps/landing/src/components/modules/player-leaderboard-panel.tsx
✅ Files skipped from review due to trivial changes (8)
- client/apps/game/src/utils/biome/vec4.ts
- packages/core/src/constants/utils.ts
- client/apps/game/src/three/managers/biome.ts
- client/apps/game/src/utils/biome/vec3.ts
- client/update-policies.js
- packages/tsconfig.base.json
- client/apps/game/src/utils/biome/fixed-point.ts
- client/apps/landing/src/hooks/use-lords-bridged.tsx
🔇 Additional comments (39)
packages/core/src/constants/quests.ts (1)
3-9: LGTM! Good practice using explicit enum values.The explicit enum values improve maintainability and prevent unintended value changes during future enum modifications. The sequential numbering without gaps is also good practice.
client/apps/game/src/utils/config.ts (1)
4-6: Verify that getConfigFromNetwork is synchronous.The removal of async/await suggests that
getConfigFromNetworkis now synchronous. Please verify this change is intentional and that the function doesn't perform any asynchronous operations internally.Consider improving type safety.
The non-null assertion on
env.VITE_PUBLIC_CHAINcould be risky. Consider adding a runtime check or using a default value.- const config = getConfigFromNetwork(env.VITE_PUBLIC_CHAIN! as Chain); + if (!env.VITE_PUBLIC_CHAIN) { + throw new Error('VITE_PUBLIC_CHAIN environment variable is not set'); + } + const config = getConfigFromNetwork(env.VITE_PUBLIC_CHAIN as Chain);client/apps/landing/src/utils/config.ts (1)
4-6: Consider extracting shared config logic.This code is duplicated in both game and landing apps. Consider extracting it into a shared utility.
The same concerns about
getConfigFromNetworkbeing synchronous and the non-null assertion onenv.VITE_PUBLIC_CHAINapply here as well.client/apps/landing/src/config.ts (1)
3-5: Verify that address getters are synchronous.Multiple address getter functions (
getLordsAddress,getSeasonPassAddress,getRealmsAddress) have been changed from async to sync. Please verify that these functions don't perform any asynchronous operations internally.✅ Verification successful
✓ Address getters are correctly implemented as synchronous functions
The implementation of all three getters (
getLordsAddress,getSeasonPassAddress,getRealmsAddress) shows they are simple synchronous functions that return values directly from thegetSeasonAddresseslookup, with no asynchronous operations involved.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for address getter implementations echo "Searching for getLordsAddress implementation..." ast-grep --pattern 'function getLordsAddress() { $$$ }' echo "Searching for getSeasonPassAddress implementation..." ast-grep --pattern 'function getSeasonPassAddress() { $$$ }' echo "Searching for getRealmsAddress implementation..." ast-grep --pattern 'function getRealmsAddress() { $$$ }'Length of output: 512
Script:
#!/bin/bash # Search with broader patterns and ripgrep echo "=== Searching with ast-grep for various function patterns ===" ast-grep --pattern 'export const getLordsAddress = $_' ast-grep --pattern 'export const getSeasonPassAddress = $_' ast-grep --pattern 'export const getRealmsAddress = $_' echo -e "\n=== Searching with ripgrep for function definitions and imports ===" rg "getLordsAddress|getSeasonPassAddress|getRealmsAddress" -A 2 echo -e "\n=== Searching for import statements ===" rg "import.*(?:getLordsAddress|getSeasonPassAddress|getRealmsAddress)"Length of output: 7296
client/apps/game/src/utils/addresses.ts (2)
Line range hint
4-16: Breaking change: Address getter functions are now synchronousThe removal of
asynckeywords from these functions is a breaking change that could affect consumers. Ensure all callers are updated to handle synchronous returns instead of Promises.
1-1: Verify that getSeasonAddresses is synchronousThe refactor assumes
getSeasonAddressesfrom@contractsis synchronous. Let's verify this assumption.✅ Verification successful
✓ Verified: getSeasonAddresses is synchronous
The implementation in
@contractsis a synchronous function that directly returns addresses based on the chain parameter. The refactor is correct.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if getSeasonAddresses is async ast-grep --pattern 'export const getSeasonAddresses = async'Length of output: 62
Script:
#!/bin/bash # Find getSeasonAddresses implementation with broader pattern ast-grep --pattern 'export $_ getSeasonAddresses' # Backup search with ripgrep for any occurrence rg "getSeasonAddresses" -A 5Length of output: 6776
client/apps/landing/src/components/ui/utils/addresses.ts (2)
4-9: LGTM! Type assertion improves safetyThe explicit type assertion for resource addresses provides better type safety.
19-21: Inconsistency: getRealmsAddress exists here but was removed from game/addresses.tsThis function exists here but was removed from
client/apps/game/src/utils/addresses.ts. Verify if this inconsistency is intentional.client/apps/landing/dojoConfig.ts (1)
1-1: Verify that getGameManifest is synchronousThe refactor assumes
getGameManifestfrom@contractsis synchronous. Let's verify this assumption.Also applies to: 15-15
✅ Verification successful
✓ getGameManifest is confirmed to be synchronous
The implementation in
contracts/utils/utils.tsshows thatgetGameManifestis a synchronous function that directly returns aGameManifestobject. The synchronous usage indojoConfig.tsis correct.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if getGameManifest is async ast-grep --pattern 'export const getGameManifest = async'Length of output: 59
Script:
#!/bin/bash # Find getGameManifest implementation with multiple patterns ast-grep --pattern 'getGameManifest = $_' ast-grep --pattern 'function getGameManifest($_)' ast-grep --pattern 'export $_ getGameManifest' # Backup search with ripgrep rg "getGameManifest" -A 5Length of output: 6522
client/apps/game/dojoConfig.ts (1)
31-31: Verify that ETERNUM_CONFIG is synchronousThe refactor assumes
ETERNUM_CONFIGis synchronous. Let's verify this assumption.✅ Verification successful
ETERNUM_CONFIG is confirmed to be synchronous
The implementation in
client/apps/game/src/utils/config.tsshows a simple synchronous function that directly returns the config object. The usage in dojoConfig.ts is correct.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if ETERNUM_CONFIG is async ast-grep --pattern 'export const ETERNUM_CONFIG = async'Length of output: 58
Script:
#!/bin/bash # Find ETERNUM_CONFIG definition with broader pattern ast-grep --pattern 'export const ETERNUM_CONFIG' # Backup search with ripgrep rg "ETERNUM_CONFIG" -A 5Length of output: 17964
config/deployer/index.ts (1)
2-2: LGTM! Import path refactoring looks good.The change to use
@contractsalias and separate imports improves code organization.Also applies to: 6-6
config/utils/utils.ts (1)
34-34: LGTM! Minor formatting improvement.The removal of extra space before semicolon aligns with prettier formatting standards.
client/apps/landing/src/hooks/use-rewards.tsx (1)
11-11: Verify error handling for synchronous getLordsAddress.The change from async to sync looks good, but please ensure that error handling is still appropriate for synchronous execution.
Let's verify the error handling in getLordsAddress implementation:
✅ Verification successful
Synchronous getLordsAddress implementation is appropriate
The function simply returns a configured address value and doesn't require async error handling. Any potential errors would be configuration issues caught during development, while runtime errors are properly handled at the contract call level.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check error handling in getLordsAddress implementation # Find the getLordsAddress implementation echo "getLordsAddress implementation:" rg -A 10 'export function getLordsAddress' --type ts # Check for try-catch usage around getLordsAddress calls echo -e "\nUsage with error handling:" rg -B 2 -A 2 'getLordsAddress' --type tsLength of output: 3526
client/apps/landing/src/components/providers/starknet-provider.tsx (1)
9-9: Verify synchronous initialization of resourceAddressesThe change from async to sync initialization needs verification to ensure it doesn't cause race conditions during component initialization.
Let's verify the implementation of
getResourceAddresses:✅ Verification successful
✓ Synchronous initialization of resourceAddresses is safe
The change from async to sync is correct as
getResourceAddresses()and its dependencies only access static configuration data without any asynchronous operations.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if getResourceAddresses is consistently synchronous across the codebase ast-grep --pattern 'export $_ function getResourceAddresses() { $$$ }'Length of output: 74
Script:
#!/bin/bash # Try multiple patterns to locate getResourceAddresses implementation echo "=== Searching with ast-grep for function declaration ===" ast-grep --pattern 'function getResourceAddresses() { $$$ }' echo "=== Searching with ast-grep for const declaration ===" ast-grep --pattern 'const getResourceAddresses = $_' echo "=== Fallback to ripgrep for any occurrence ===" rg "getResourceAddresses" -A 5Length of output: 13682
Script:
#!/bin/bash # Verify getSeasonAddresses implementation echo "=== Searching for getSeasonAddresses implementation ===" ast-grep --pattern 'function getSeasonAddresses($_) { $$$ }' echo "=== Searching for const declaration ===" ast-grep --pattern 'const getSeasonAddresses = $_' echo "=== Fallback to ripgrep ===" rg "getSeasonAddresses" -A 5Length of output: 7531
client/apps/game/src/hooks/context/starknet-provider.tsx (1)
16-16: Ensure consistent initialization across providersThe synchronous initialization matches the landing app's implementation, but we should verify it doesn't affect controller initialization timing.
Let's check for any timing-dependent controller initialization:
✅ Verification successful
Synchronous initialization is safe ✅
The controller initialization is independent of resource addresses, and the synchronous
getResourceAddresses()is used consistently across both game and landing apps for configuration purposes only.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for controller initialization patterns that might depend on async operations rg -p "new ControllerConnector.*getResourceAddresses" --type tsLength of output: 65
Script:
#!/bin/bash # Search for ControllerConnector initialization and resource address usage echo "=== ControllerConnector initialization patterns ===" rg -p "ControllerConnector" -A 5 --type ts echo -e "\n=== Resource addresses usage patterns ===" rg -p "resourceAddresses" -A 5 --type tsLength of output: 7182
client/apps/game/src/hooks/helpers/use-quests.ts (1)
2-2: LGTM! Good consolidation of QuestStatus enumMoving to a centralized enum definition in
@bibliothecadao/eternumimproves maintainability and reduces code duplication.contracts/utils/utils.ts (1)
20-37: LGTM! Well-structured utility functionsThe implementation is clean, type-safe, and includes proper error handling. The JSDoc comments are thorough and helpful.
Also applies to: 53-70
contracts/season_resources/scripts/deployment/config/index.ts (2)
3-3: LGTM! Good refactoring of address management.The changes improve code organization by centralizing address management in utils.ts and simplifying the implementation.
Also applies to: 5-7
50-50: LGTM! Good simplification of manifest retrieval.Centralizing manifest selection in
getGameManifestimproves maintainability.client/apps/game/vite.config.ts (1)
65-65: LGTM! Good addition of @contracts alias.The new alias improves code organization and makes imports more maintainable.
client/apps/game/src/ui/components/worldmap/armies/selected-army.tsx (1)
20-20: LGTM! Good encapsulation of SelectedArmyContent.Removing the export improves component organization as this component is only used internally.
client/apps/landing/src/components/modules/bridge-fees.tsx (1)
1-2: LGTM! Good centralization of BRIDGE_FEE_DENOMINATOR.Importing the constant from @bibliothecadao/eternum reduces potential inconsistencies.
client/apps/game/src/utils/biome/simplex-noise.ts (1)
Line range hint
1-125: Verify the impact of removing thenoise_octavesfunction.The removal of the
noise_octavesfunction could affect components that rely on multi-octave noise generation. Let's verify its usage across the codebase.config/environments/_shared_.ts (1)
138-138: Verify type compatibility after removing BigInt conversions.The
BigIntconversions have been removed from fee recipient assignments. Let's verify that this doesn't cause type mismatches where these values are used.Also applies to: 271-272
client/apps/landing/src/components/modules/bridge-out-step-2.tsx (1)
47-47: LGTM! Consistent with the async-to-sync refactor.The change from async to sync call of
getResourceAddressesaligns with the broader refactor of making address retrieval synchronous across the codebase.client/apps/landing/src/components/modules/bridge-out-step-1.tsx (1)
132-132: Verify error handling for synchronous getResourceAddresses call.The function has been changed from async to sync. Ensure that any potential errors from
getResourceAddresses()are properly caught and handled.client/apps/landing/src/components/modules/bridge-in.tsx (1)
132-132: Verify error handling for synchronous getResourceAddresses call.The function has been changed from async to sync. Ensure that any potential errors from
getResourceAddresses()are properly caught and handled, especially since it's used to set component state.config/deployer/config.ts (1)
4-4: LGTM! Good consolidation of constants.Moving
BRIDGE_FEE_DENOMINATORto@bibliothecadao/eternumhelps centralize and standardize the constant across the codebase.packages/core/tsconfig.json (1)
2-6: Verify the inherited TypeScript configuration.The configuration has been simplified by extending from
../tsconfig.base.json. Please ensure that all necessary compiler options are properly inherited from the base configuration.✅ Verification successful
TypeScript configuration inheritance verified successfully
The base configuration at
packages/tsconfig.base.jsoncontains all necessary compiler options, and the path resolution is correct. The simplified configuration inpackages/core/tsconfig.jsonproperly extends the base while only specifying its unique settings.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the base TypeScript configuration. # Check if the base config exists and show its content cat ../tsconfig.base.jsonLength of output: 81
Script:
#!/bin/bash # Find all TypeScript configuration files echo "=== All TypeScript config files ===" fd tsconfig.json echo -e "\n=== Looking for base config files ===" fd -g '*tsconfig*.json' | grep -i 'base'Length of output: 459
Script:
#!/bin/bash # Show the content of the base configuration echo "=== Base TypeScript Configuration ===" cat packages/tsconfig.base.jsonLength of output: 543
packages/react/tsconfig.json (2)
2-2: Verify the base TypeScript configuration exists.The configuration extends from "../tsconfig.base.json". Let's verify this file exists and contains the necessary compiler options that were removed from this file.
✅ Verification successful
Base TypeScript configuration path is correct ✅
The relative path "../tsconfig.base.json" correctly points to the existing file at "packages/tsconfig.base.json".
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the base tsconfig exists and examine its content if [ -f "../tsconfig.base.json" ]; then cat "../tsconfig.base.json" else echo "Base tsconfig not found!" fiLength of output: 93
Script:
#!/bin/bash # Search for tsconfig.base.json files in the repository fd "tsconfig.base.json"Length of output: 51
4-4: Verify the output directory structure.The outDir path "../../dist/react" points two levels up from the current location. Ensure this aligns with the project's build output structure.
✅ Verification successful
The output directory structure is correctly configured ✅
The
outDirpath "../../dist/react" is correct as it follows the consistent pattern used across the monorepo, where other packages (like core) use the same structure. Both packages use tsup for building and maintain their outputs in the root-level dist directory.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the target directory structure exists if [ -d "../../dist" ]; then echo "dist directory exists" ls -la ../../dist else echo "dist directory not found at expected location" fiLength of output: 124
Script:
#!/bin/bash # Find all tsconfig.json files and their outDir settings echo "=== TSConfig files ===" fd tsconfig.json echo -e "\n=== OutDir settings ===" fd tsconfig.json --exec grep -l "outDir" {} \; --exec echo "File: {}" \; --exec grep "outDir" {} \; echo -e "\n=== Package.json files in root and packages ===" find . -maxdepth 3 -name "package.json" -exec echo "File: {}" \; -exec grep -A 2 "\"build\"" {} \; 2>/dev/nullLength of output: 1499
.knip.json (1)
9-9: Verify the scope of ignored paths.The changes broaden the ignore patterns significantly:
- "packages/" is more inclusive than the previous "client/sdk/packages/"
- New ignore patterns for "config/" and "contracts/" have been added
Let's verify what files would be excluded by these patterns to ensure we're not accidentally ignoring important files.
Also applies to: 26-29
client/apps/game/tsconfig.json (1)
6-7: LGTM! Path mappings are consistent.The path mappings and include array updates maintain consistency with the root configuration:
- "@config" and "@contracts" aliases point to the correct utility directories
- Include array properly references both config and contracts utilities
Also applies to: 29-29
client/apps/landing/tsconfig.json (2)
7-8: LGTM! Path aliases are well organized.Good organization of path aliases:
- "@components" provides a clean way to import landing-specific components
- "@contracts" maintains consistency with other configs
30-30: LGTM! Include paths are comprehensive.The include array properly references all necessary paths:
- Source files: "src"
- Config utilities: "../../../config/utils/utils"
- Components: "src/components/*"
- Contract utilities: "../../../contracts/utils/utils"
client/apps/landing/src/components/modules/bridged-resources.tsx (2)
Line range hint
1-24: Well-structured component with robust error handling!The implementation demonstrates good practices:
- Proper error handling in contract calls
- Efficient state management with useCallback
- Clean UI implementation with sorting capabilities
Also applies to: 26-220
22-22: Verify synchronous behavior of getResourceAddresses.The change from async to sync call aligns with the refactoring objective. However, let's verify that
getResourceAddressesis properly refactored to be synchronous.✅ Verification successful
Synchronous implementation verified
The
getResourceAddressesfunction inclient/apps/landing/src/components/ui/utils/addresses.tsis correctly implemented as a synchronous function, making the removal ofawaitappropriate.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if getResourceAddresses is properly refactored to be synchronous # and verify no async/await usage remains # Search for the implementation of getResourceAddresses ast-grep --pattern 'export $_ getResourceAddresses$_() { $$$ }' # Check for any remaining async/await usage rg -A 5 'getResourceAddresses.*async'Length of output: 827
Script:
#!/bin/bash # Search for getResourceAddresses implementation in client directory ast-grep --pattern 'function getResourceAddresses() { $$$ }' # Also search for const declaration ast-grep --pattern 'const getResourceAddresses = $_' # Get the content of the component file to check imports cat client/apps/landing/src/components/modules/bridged-resources.tsx # Search for files that might export getResourceAddresses rg -l "export.*getResourceAddresses"Length of output: 10141
client/apps/game/src/ui/modules/onboarding/steps.tsx (2)
Line range hint
1-104: Well-implemented onboarding flow with proper error handling!The implementation demonstrates good practices:
- Proper error handling in realm settlement
- Efficient state management with useMemo
- Clean UI implementation for realm selection
Also applies to: 106-220
105-105: Verify synchronous behavior of getSeasonPassAddress.The change from async to sync call aligns with the refactoring objective. However, let's verify that
getSeasonPassAddressis properly refactored to be synchronous.✅ Verification successful
Confirmed: getSeasonPassAddress is correctly implemented as synchronous
The function implementation in
utils/addresses.tsis properly synchronous, returning a value directly without any async operations. The removal ofawaitin this file is correct.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if getSeasonPassAddress is properly refactored to be synchronous # and verify no async/await usage remains # Search for the implementation of getSeasonPassAddress ast-grep --pattern 'export $_ getSeasonPassAddress$_() { $$$ }' # Check for any remaining async/await usage rg -A 5 'getSeasonPassAddress.*async'Length of output: 107
Script:
#!/bin/bash # First, find files that might contain getSeasonPassAddress rg -l "getSeasonPassAddress" # Then look for its implementation with more context rg -B 3 -A 5 "getSeasonPassAddress" # Also check for any imports of this function rg "import.*getSeasonPassAddress"Length of output: 6749
| @@ -16,6 +16,7 @@ export default defineConfig(({ mode }) => { | |||
| "@": path.resolve(__dirname, "./src"), | |||
| "@/assets": path.resolve(__dirname, "../../public/assets"), | |||
| "@config": path.resolve(__dirname, "../../../config/utils/utils"), | |||
| "@contracts": path.resolve(__dirname, "../../../contracts/utils/utils"), | |||
There was a problem hiding this comment.
💡 Codebase verification
Path resolution error in @contracts alias
The alias path "../../../contracts/utils/utils" points to a non-existent directory, while the actual file is contracts/utils/utils.ts. This will cause module resolution failures.
- Update the path to:
"../../../contracts/utils/utils.ts"
🔗 Analysis chain
LGTM! Verify path resolution across the codebase.
The new @contracts alias addition aligns with the refactoring objectives.
Let's verify the path resolution and its usage:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify @contracts alias resolution and usage
# Check if the target directory exists
if [ ! -d "contracts/utils/utils" ]; then
echo "Warning: Target directory 'contracts/utils/utils' not found"
exit 1
fi
# Find all TypeScript/JavaScript files using the @contracts alias
echo "Files using @contracts alias:"
rg -l '@contracts' --type ts --type js
# Verify the directory structure
echo -e "\nDirectory structure:"
ls -R contracts/utils/utils
Length of output: 182
Script:
#!/bin/bash
# First, find any directory named 'contracts' to understand the structure
echo "Looking for contracts directory:"
fd -t d contracts
# Then look for any files with 'contracts' in the path
echo -e "\nFiles with 'contracts' in path:"
fd -t f . | grep -i contracts
# Finally, search for @contracts usage
echo -e "\nFiles using @contracts imports:"
rg -l "@contracts"
Length of output: 66540
| "baseUrl": ".", | ||
| "paths": { | ||
| "@contracts": ["../contracts/utils/utils"] | ||
| }, |
There was a problem hiding this comment.
💡 Codebase verification
Path alias points to non-existent directory
The path alias "@contracts" is configured to point to "../contracts/utils/utils", but this directory does not exist. The correct path should likely be "../contracts/utils" as this directory exists in the codebase.
- Update the path in
config/tsconfig.json:
"paths": {
"@contracts": ["../contracts/utils"]
}🔗 Analysis chain
Verify the contracts utility path exists.
The new path alias "@contracts" points to "../contracts/utils/utils". Let's verify this directory exists and contains the expected utilities.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the contracts utils directory exists and examine its content
if [ -d "../contracts/utils/utils" ]; then
echo "Directory exists"
ls -la "../contracts/utils/utils"
else
echo "Contracts utils directory not found!"
fi
Length of output: 120
Script:
#!/bin/bash
# Check from repository root and search for utils directories
echo "Checking from repository root:"
if [ -d "contracts/utils/utils" ]; then
echo "Directory exists at contracts/utils/utils"
ls -la contracts/utils/utils
else
echo "Directory not found at contracts/utils/utils"
fi
echo -e "\nSearching for utils directories:"
fd --type d "utils"
Length of output: 617
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
client/apps/game-docs/docs/components/QuestRewards.tsx (1)
Line range hint
6-29: Optimize config access and add error handling.The component makes multiple calls to ETERNUM_CONFIG() and lacks error handling for missing resources:
export default function QuestRewards() { + const config = ETERNUM_CONFIG(); + + if (!config.questResources) { + return <div>No quest resources available</div>; + } + return ( <div className="grid grid-cols-1 gap-6"> - {Object.entries(ETERNUM_CONFIG().questResources).map(([questType, rewards]) => ( + {Object.entries(config.questResources).map(([questType, rewards]) => ( <div key={questType} className="p-4 rounded-lg border border-gray-200 dark:border-gray-700 bg-white/5"> <div className="font-bold text-lg mb-4"> {addSpacesBeforeCapitals(QuestType[Number(questType)] || "Unknown Quest")} </div> <div className="grid grid-cols-2 sm:grid-cols-3 lg:grid-cols-5 gap-4"> {rewards.map((reward, index) => { const resource = findResourceById(reward.resource); - const resourceName = resource?.trait || "Unknown Resource"; + if (!resource) { + console.warn(`Resource not found for ID: ${reward.resource}`); + return null; + } + const resourceName = resource.trait; return ( <div key={index} className="flex items-center px-4 py-2 rounded-md bg-gray-800"> <ResourceIcon size="lg" id={reward.resource} name={resourceName} /> <span className="font-medium text-gray-300 ml-4">{formatAmount(reward.amount)}</span> </div> ); })} </div> </div> ))} </div> ); }
🧹 Nitpick comments (6)
client/apps/game-docs/env.ts (1)
2-2: Consider environment-specific defaults for VITE_PUBLIC_CHAIN.Defaulting to "mainnet" could be risky if the environment variable is accidentally unset in non-production environments. Consider making the default value environment-specific or requiring explicit configuration.
- VITE_PUBLIC_CHAIN: process.env.VITE_PUBLIC_CHAIN || "mainnet", + VITE_PUBLIC_CHAIN: process.env.VITE_PUBLIC_CHAIN || (process.env.NODE_ENV === 'production' ? "mainnet" : "testnet"),🧰 Tools
🪛 GitHub Actions: knip
[warning] Unused file detected by knip. This file is not being used in the project and should be removed or utilized.
client/apps/game-docs/vocs.config.ts (1)
23-25: Document the new alias in the development guide.Since this alias is part of a refactoring effort, consider updating the development documentation (
/developmentsection) to explain the purpose and usage of the@configalias for other developers.client/apps/game-docs/docs/utils/formatting.ts (2)
13-14: Improve readability and safety of number formatting logic.The nested ternary operations and magic numbers make the code hard to maintain. Consider:
- Extracting magic numbers as named constants
- Splitting complex conditions into separate functions
- Adding validation for RESOURCE_PRECISION
+const FORMAT_THRESHOLD = { + KILO: 1, + MEGA: RESOURCE_PRECISION +}; + export const formatAmount = (amount: number) => { + if (RESOURCE_PRECISION === 0) { + throw new Error('RESOURCE_PRECISION cannot be zero'); + } - if (amount < 1) { + if (amount < FORMAT_THRESHOLD.KILO) { return `${amount * RESOURCE_PRECISION}`; - } else if (amount < RESOURCE_PRECISION) { - return `${amount.toFixed(amount % 1 === 0 ? 0 : (amount % 1) % 0.1 === 0 ? 1 : 2)}K`; + } else if (amount < FORMAT_THRESHOLD.MEGA) { + const decimals = getDecimalPlaces(amount); + return `${amount.toFixed(decimals)}K`; } else { - return `${(amount / RESOURCE_PRECISION).toFixed(amount % RESOURCE_PRECISION === 0 ? 0 : (amount % RESOURCE_PRECISION) % 10 === 0 ? 1 : 2)}M`; + const scaledAmount = amount / RESOURCE_PRECISION; + const decimals = getDecimalPlaces(amount, RESOURCE_PRECISION); + return `${scaledAmount.toFixed(decimals)}M`; } }; + +function getDecimalPlaces(amount: number, divisor: number = 1): number { + const remainder = amount % divisor; + if (remainder === 0) return 0; + if ((remainder % (divisor / 10)) === 0) return 1; + return 2; +}Also applies to: 17-17
36-36: Add validation for currencyFormat.Similar to formatAmount, currencyFormat should validate RESOURCE_PRECISION to prevent division by zero:
export const currencyFormat = (num: number, decimals: number): string => { + if (RESOURCE_PRECISION === 0) { + throw new Error('RESOURCE_PRECISION cannot be zero'); + } return formatNumber(num / RESOURCE_PRECISION, decimals); };client/apps/game-docs/docs/components/ResourceTable.tsx (1)
6-6: Cache ETERNUM_CONFIG outside component.The config is static and doesn't need to be initialized on every render:
-const eternumConfig = ETERNUM_CONFIG(); +const eternumConfig = ETERNUM_CONFIG(); // Move this to module scopeclient/apps/game-docs/docs/components/TroopsTable.tsx (1)
67-67: Add type safety for the config object.While the refactoring improves performance by avoiding repeated function calls, consider adding TypeScript type definitions for the config structure to ensure type safety.
Add a type definition:
type EternumConfig = { stamina: { travelCost: number; exploreCost: number; }; // ... other config properties };Also applies to: 72-72
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
client/apps/game-docs/.gitignore(1 hunks)client/apps/game-docs/docs/components/QuestRewards.tsx(1 hunks)client/apps/game-docs/docs/components/ResourceTable.tsx(1 hunks)client/apps/game-docs/docs/components/TroopsTable.tsx(2 hunks)client/apps/game-docs/docs/utils/config.ts(1 hunks)client/apps/game-docs/docs/utils/formatting.ts(3 hunks)client/apps/game-docs/env.ts(1 hunks)client/apps/game-docs/tsconfig.json(1 hunks)client/apps/game-docs/vocs.config.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- client/apps/game-docs/tsconfig.json
🧰 Additional context used
🪛 GitHub Actions: knip
client/apps/game-docs/env.ts
[warning] Unused file detected by knip. This file is not being used in the project and should be removed or utilized.
🔇 Additional comments (5)
client/apps/game-docs/.gitignore (1)
1-3: LGTM! The gitignore entries follow best practices.The additions properly exclude:
- Node.js dependencies (
node_modules/)- Generated Vocs configuration files (
vocs.config.ts.timestamp-*.mjs)client/apps/game-docs/env.ts (1)
1-3: Verify file usage - knip reports this as unused.The pipeline indicates this file is unused, but the summary suggests it's used in
client/apps/game/dojoConfig.ts. Let's verify this discrepancy.🧰 Tools
🪛 GitHub Actions: knip
[warning] Unused file detected by knip. This file is not being used in the project and should be removed or utilized.
client/apps/game-docs/vocs.config.ts (1)
24-24: Verify the path resolution and consistency across configurations.The new alias
@configpoints to a shared utilities folder. Let's ensure the path exists and is consistent with other configuration files.✅ Verification successful
✓ Path resolution and configuration consistency verified
The
@configalias is correctly implemented and consistent with other app configurations in the monorepo. The target path exists and follows the established pattern.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the path existence and check for similar aliases in other config files # Check if the target path exists fd -t d "utils" config/ # Look for other Vite/TypeScript config files that might need similar updates fd -e config.ts -e config.js # Check for existing alias patterns in other config files rg -A 5 "@config.*path.resolve"Length of output: 2000
client/apps/game-docs/docs/components/TroopsTable.tsx (2)
67-67: LGTM! Performance improvement.Good refactoring to avoid repeated function calls by storing the config result in a constant.
Also applies to: 72-72
1-1: Consider adding error handling for configuration initialization.The synchronous configuration initialization at the module level could fail silently. Consider wrapping it in a try-catch block or moving it into a React hook with proper error boundaries.
Let's verify the ETERNUM_CONFIG implementation:
Also applies to: 6-6
| export const ETERNUM_CONFIG = () => { | ||
| const config = getConfigFromNetwork(env.VITE_PUBLIC_CHAIN! as Chain); | ||
| return config; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and type safety for environment variable.
The non-null assertion (!) on env.VITE_PUBLIC_CHAIN could lead to runtime errors if the environment variable is missing. Consider adding proper validation:
export const ETERNUM_CONFIG = () => {
- const config = getConfigFromNetwork(env.VITE_PUBLIC_CHAIN! as Chain);
+ if (!env.VITE_PUBLIC_CHAIN) {
+ throw new Error('VITE_PUBLIC_CHAIN environment variable is required');
+ }
+ if (!Object.values(Chain).includes(env.VITE_PUBLIC_CHAIN as Chain)) {
+ throw new Error(`Invalid chain value: ${env.VITE_PUBLIC_CHAIN}`);
+ }
+ const config = getConfigFromNetwork(env.VITE_PUBLIC_CHAIN as Chain);
return config;
};📝 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 ETERNUM_CONFIG = () => { | |
| const config = getConfigFromNetwork(env.VITE_PUBLIC_CHAIN! as Chain); | |
| return config; | |
| }; | |
| export const ETERNUM_CONFIG = () => { | |
| if (!env.VITE_PUBLIC_CHAIN) { | |
| throw new Error('VITE_PUBLIC_CHAIN environment variable is required'); | |
| } | |
| if (!Object.values(Chain).includes(env.VITE_PUBLIC_CHAIN as Chain)) { | |
| throw new Error(`Invalid chain value: ${env.VITE_PUBLIC_CHAIN}`); | |
| } | |
| const config = getConfigFromNetwork(env.VITE_PUBLIC_CHAIN as Chain); | |
| return config; | |
| }; |
| for (const resourceId of Object.keys(eternumConfig.resources.resourceInputs) as unknown as ResourcesIds[]) { | ||
| if (resourceId === ResourcesIds.Lords) continue; | ||
| const calldata = { | ||
| resource: findResourceById(Number(resourceId)), | ||
| amount: RESOURCE_PRODUCTION_OUTPUT_AMOUNTS[resourceId], | ||
| amount: eternumConfig.resources.resourceInputs[resourceId], | ||
| resource_type: resourceId, | ||
| cost: RESOURCE_PRODUCTION_INPUT_RESOURCES[resourceId].map((cost: any) => ({ | ||
| cost: eternumConfig.resources.resourceInputs[resourceId].map((cost: any) => ({ | ||
| ...cost, | ||
| amount: cost.amount * ETERNUM_CONFIG().resources.resourcePrecision, | ||
| amount: cost.amount * RESOURCE_PRECISION, | ||
| name: findResourceById(cost.resource)?.trait || "", | ||
| })), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety and error handling in resource processing.
The code uses unsafe type casting and lacks proper type definitions:
+interface ResourceCost {
+ resource: number;
+ amount: number;
+}
+
const resourceTable = useMemo(() => {
const resources = [];
- for (const resourceId of Object.keys(eternumConfig.resources.resourceInputs) as unknown as ResourcesIds[]) {
+ const resourceIds = Object.keys(eternumConfig.resources.resourceInputs)
+ .filter((id): id is keyof typeof ResourcesIds => {
+ const numId = Number(id);
+ return !isNaN(numId) && Object.values(ResourcesIds).includes(numId);
+ });
+
+ for (const resourceId of resourceIds) {
if (resourceId === ResourcesIds.Lords) continue;
+
+ const resource = findResourceById(Number(resourceId));
+ if (!resource) {
+ console.warn(`Resource not found for ID: ${resourceId}`);
+ continue;
+ }
+
const calldata = {
- resource: findResourceById(Number(resourceId)),
+ resource,
amount: eternumConfig.resources.resourceInputs[resourceId],
resource_type: resourceId,
- cost: eternumConfig.resources.resourceInputs[resourceId].map((cost: any) => ({
+ cost: eternumConfig.resources.resourceInputs[resourceId].map((cost: ResourceCost) => ({
...cost,
amount: cost.amount * RESOURCE_PRECISION,
name: findResourceById(cost.resource)?.trait || "",
})),
};Committable suggestion skipped: line range outside the PR's diff.
Summary by CodeRabbit
Release Notes
Configuration Updates
@contractsacross multiple projects.Code Refactoring
Resource and Quest Management
Bridge and Fee Management
BRIDGE_FEE_DENOMINATORconstant across packages.Dependency and Import Improvements
These changes aim to improve code maintainability, reduce complexity, and standardize configuration management across the project.