Bob/fix landing #2649
Bob/fix landing #2649
Conversation
* contracts & client: increase resource precision from 3 to 9 * contracts & discord-bot: update folder structure * rustfmt
* new production wip * new production wip * production simplified * new production compiles * update production implementation * scarb fmt * improve scripts * update scripts * improve scripts * knip (#2635) * fix knip * fixed landing knip * fix knip balancing * knip * improve scripts * prettier (#2636) * clean * add prettier rules * update prettier ignore * prettier format * gql codegen * Fix lint CI * scarb fmt --------- Co-authored-by: Bob <[email protected]> * improve configuration * improve configuration * Enh/clean assets (#2637) * Delete deprecated pngs from /game * Delete deprecated mp3 from /game * Consolidate public assets * Adapt imports * Fix spelling * improve configuration * Fix naming conventions (#2638) * Fix naming conventions in game directory * fix build * Kebab * fix imports * adapt naming conventions * formatting * knip * Formatting * update policies (#2639) * update policies * fix knip * prettier * improve scripts * improve config deploy flow based on env * improve config deploy flow based on env * improve config deploy flow based on env * fix indexing * fix indexing * add permisson revoke script * ++Refactor use-armies and clean battles (#2640) * clean battles * refactor use-armies * refactor use bank * update * working client * working client * rename s0_eternum to s1_eternum * update world address * update env * Merge branch 'next' of https://github.com/BibliothecaDAO/eternum into feat/labor --------- Co-authored-by: raschel <[email protected]> Co-authored-by: Bob <[email protected]> Co-authored-by: Bob <[email protected]>
- fix build
…O/eternum into next-into-season-1
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
|
Hi @bob0005! 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/dojoConfig.tsOops! 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 involves multiple modifications across the client application, focusing on configuration management, import path restructuring, and component updates. The changes primarily affect how game manifests, addresses, and configurations are imported and utilized. Key modifications include updating import paths, changing function invocations to be asynchronous, adjusting type definitions, and simplifying configuration management. The changes aim to improve type safety, streamline configuration retrieval, and enhance the overall structure of the client-side application. Changes
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)
client/apps/landing/src/components/modules/bridge-fees.tsx (1)
Import BRIDGE_FEE_DENOMINATOR from shared configuration
The constant is already defined in
config/environments/_shared_.ts. To maintain consistency and avoid potential errors, import it from there instead of redefining it in the component.
- Replace the local definition with:
import { BRIDGE_FEE_DENOMINATOR } from "config/environments/_shared_";🔗 Analysis chain
Line range hint
9-10: Address the TODO comment about configuration.The comment suggests that BRIDGE_FEE_DENOMINATOR should be obtained from configuration. This should be addressed or the comment should be updated with the reason why it's hardcoded.
Let's check if this value exists in the configuration:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for BRIDGE_FEE_DENOMINATOR in configuration files rg -i "bridge.*fee.*denominator" --type ts --type jsonLength of output: 2048
client/apps/landing/src/routes/mint.lazy.tsx (1)
Line range hint
143-149: Fix undefinedstatusvariable and improve error handling.The
statusvariable used inSeasonPassMintDialogis undefined, which could cause runtime errors. Additionally, the component lacks proper error handling.
- Add proper status handling:
+ const [status, setStatus] = useState<'idle' | 'loading' | 'success' | 'error'>('idle'); {isOpen && ( <SeasonPassMintDialog isOpen={isOpen} setIsOpen={setIsOpen} deselectAllNfts={deselectAllNfts} isSuccess={status === "success"} realm_ids={selectedTokenIds} /> )}
- Consider adding error boundaries and loading states to improve user experience.
🧹 Nitpick comments (13)
client/apps/landing/src/types/index.ts (1)
11-23: LGTM! Consider enhancing type safety.The
SeasonPassMinttype is well-structured for GraphQL integration. Consider making the__typenameforToken__Balancenon-optional since it's a discriminator field.- __typename?: "Token__Balance"; + __typename: "Token__Balance";config/utils/utils.ts (1)
Line range hint
12-12: Remove debug console.log statement.Debug logging should be removed before merging to production.
- console.log({ ADDRESSES_FILE });client/apps/landing/src/hooks/use-lords-bridged.tsx (3)
2-3: Remove or resolve the commented import.The comment "why not working" suggests an unresolved issue. Either document the reason why this import doesn't work or remove the commented code if it's no longer needed.
5-5: Simplify the import path.The relative import path with multiple parent directory references (
../../../../../) is fragile and hard to maintain. Consider using path aliases or restructuring the imports.Consider using a path alias like this:
-import { Chain, getGameManifest } from "../../../../../config/utils/utils"; +import { Chain, getGameManifest } from "@/config/utils";
22-24: Add proper typing for the manifest object.The inline type annotation for manifest is verbose and could be moved to a proper type definition.
Consider creating a proper type:
type GameManifest = { contracts?: Array<{ tag: string; address?: string; }>; };Then use it like:
- const manifest: { contracts?: Array<{ tag: string; address?: string }> } = await getGameManifest( + const manifest: GameManifest = await getGameManifest( VITE_PUBLIC_CHAIN as Chain, );client/apps/landing/src/hooks/use-rewards.tsx (1)
Line range hint
21-44: Consider extracting the prize pool fetching logic.The useEffect contains complex logic that could be extracted into a separate function for better readability and testability.
Consider refactoring like this:
const fetchPrizePool = async () => { const season_pool_fee_recipient = configManager?.getResourceBridgeFeeSplitConfig()?.season_pool_fee_recipient; if (!season_pool_fee_recipient) { throw new Error("Season pool fee recipient not configured"); } const recipientAddress = "0x" + season_pool_fee_recipient.toString(16); const balance = await getBalance(recipientAddress); if (balance?.[0]) { return BigInt(balance[0]); } return 0n; }; useEffect(() => { const getPrizePool = async () => { try { const pool = await fetchPrizePool(); setPrizePool(pool); } catch (err) { console.error("Error getting prize pool:", err); } }; getPrizePool(); const interval = setInterval(getPrizePool, 60000); return () => clearInterval(interval); }, []);client/apps/landing/src/components/providers/starknet-provider.tsx (2)
9-9: Remove or resolve the commented import.The commented import for cartridgeController suggests confusion about which import to use.
24-33: Clean up duplicate namespace configuration.The namespace is declared both as a constant and in the configuration object. Additionally, some properties could be grouped better.
Consider refactoring like this:
const cartridgeController = new ControllerConnector({ chains: [{ rpcUrl: env.VITE_PUBLIC_NODE_URL }], defaultChainId: constants.StarknetChainId.SN_SEPOLIA, - namespace, - slot, - preset, - theme, + appearance: { + theme, + preset, + }, + config: { + namespace, + slot, + }, tokens: { erc20: [LORDS, ...otherResources], }, - // namespace, });client/apps/landing/src/components/modules/bridge-fees.tsx (2)
94-94: Improve type safety when accessing ResourcesIds.The type casting could be made safer by using type guards or ensuring the type at compile time.
Consider refactoring like this:
function isValidResourceId(id: string): id is keyof typeof ResourcesIds { return id in ResourcesIds; } // Then use it like: const resourceName = isValidResourceId(fees.id) ? ResourcesIds[fees.id].toString() : 'Unknown Resource';
43-43: Consider extracting fee calculation logic.The fee calculation logic is repeated multiple times and could be extracted into a utility function for better maintainability.
Consider creating a utility function:
type FeeType = 'deposit' | 'withdrawal'; type FeeCalculation = { velordsFee: number; seasonPoolFee: number; clientFee: number; bankFee: number; }; function calculateFees(amount: string, type: FeeType, config: typeof bridgeConfig): FeeCalculation { return { velordsFee: calculateBridgeFee( type === "deposit" ? config.velords_fee_on_dpt_percent : config.velords_fee_on_wtdr_percent, amount ), seasonPoolFee: calculateBridgeFee( type === "deposit" ? config.season_pool_fee_on_dpt_percent : config.season_pool_fee_on_wtdr_percent, amount ), clientFee: calculateBridgeFee( type === "deposit" ? config.client_fee_on_dpt_percent : config.client_fee_on_wtdr_percent, amount ), bankFee: calculateBridgeFee( type === "deposit" ? config.max_bank_fee_dpt_percent : config.max_bank_fee_wtdr_percent, amount ), }; }Also applies to: 48-65
packages/core/src/modelManager/ConfigManager.ts (1)
90-90: Remove unnecessary return statements.The
return;statements at the end of these initialization methods are unnecessary as they don't return any value and are at the end of the function.- return;Also applies to: 166-166, 203-203, 241-241
🧰 Tools
🪛 GitHub Actions: lint
[warning] Code formatting does not match Prettier standards. Run Prettier with --write to fix.
client/apps/landing/src/routes/mint.lazy.tsx (2)
54-54: Improve type safety and maintainability of environment variable handling.While the logic is correct, consider these improvements:
- Add type assertions for environment variables
- Extract magic strings into constants
- Move this logic to a configuration utility
+const CHAIN_TYPE = { + LOCAL: 'local' +} as const; + +// In a separate config utility file +const isDevelopmentMode = () => { + const chain = import.meta.env.VITE_PUBLIC_CHAIN as string; + const isDev = import.meta.env.VITE_PUBLIC_DEV as string; + return chain === CHAIN_TYPE.LOCAL || isDev === 'true'; +}; + -const isDev = import.meta.env.VITE_PUBLIC_CHAIN === "local" || import.meta.env.VITE_PUBLIC_DEV === "true"; +const isDev = isDevelopmentMode();
Line range hint
31-157: Consider splitting the Mint component for better maintainability.The component handles multiple responsibilities and has grown quite large. Consider breaking it down into smaller, focused components:
- NFTSelectionManager
- MintingActions
- GridContainer
This would improve:
- Testability
- Code reusability
- Maintenance
- Performance optimization opportunities
Would you like me to provide a detailed refactoring plan?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
client/apps/landing/dojoConfig.ts(2 hunks)client/apps/landing/package.json(1 hunks)client/apps/landing/src/components/modules/app-sidebar.tsx(3 hunks)client/apps/landing/src/components/modules/bridge-fees.tsx(3 hunks)client/apps/landing/src/components/modules/player-leaderboard-panel.tsx(2 hunks)client/apps/landing/src/components/providers/starknet-provider.tsx(2 hunks)client/apps/landing/src/components/ui/sidebar.tsx(2 hunks)client/apps/landing/src/components/ui/utils/addresses.ts(1 hunks)client/apps/landing/src/hooks/use-lords-bridged.tsx(2 hunks)client/apps/landing/src/hooks/use-rewards.tsx(1 hunks)client/apps/landing/src/routes/mint.lazy.tsx(1 hunks)client/apps/landing/src/types/index.ts(1 hunks)client/apps/landing/tsconfig.json(1 hunks)client/apps/landing/vite.config.ts(1 hunks)config/utils/utils.ts(2 hunks)packages/core/src/modelManager/ConfigManager.ts(4 hunks)
✅ Files skipped from review due to trivial changes (3)
- client/apps/landing/src/components/modules/app-sidebar.tsx
- client/apps/landing/src/components/ui/utils/addresses.ts
- client/apps/landing/src/components/ui/sidebar.tsx
🧰 Additional context used
🪛 GitHub Actions: knip
client/apps/landing/src/components/modules/bridge-fees.tsx
[warning] 9-9: Unused export 'BRIDGE_FEE_DENOMINATOR'
🪛 GitHub Actions: lint
packages/core/src/modelManager/ConfigManager.ts
[warning] Code formatting does not match Prettier standards. Run Prettier with --write to fix.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: clippy
- GitHub Check: Run pr agent on every pull request, respond to user comments
🔇 Additional comments (6)
client/apps/landing/dojoConfig.ts (1)
2-2: Verify top-level await support in build configuration.The change to async manifest loading is correct. The required
vite-plugin-top-level-awaitis present in the build config.Run this script to verify the import path exists:
Also applies to: 16-16
✅ Verification successful
Import path and exported symbols verified successfully
The import path is valid and both
Chaintype andgetGameManifestfunction are properly exported fromconfig/utils/utils.ts. The async implementation is correctly aligned with the top-level await usage.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the new import path exists fd "utils.ts$" config/utils/Length of output: 52
Script:
#!/bin/bash # Check for exports of Chain and getGameManifest ast-grep --pattern 'export { $$ Chain $$ }' ast-grep --pattern 'export function getGameManifest'Length of output: 100
Script:
#!/bin/bash # Check file content for exports with broader patterns rg "export.*Chain" config/utils/utils.ts rg "export.*getGameManifest" config/utils/utils.ts rg "export type Chain" config/utils/utils.tsLength of output: 503
config/utils/utils.ts (1)
16-16: Consider using more robust path resolution for JSON imports.The relative paths to JSON files might be brittle. Consider:
- Using path resolution utilities
- Moving paths to configuration
- Adding existence checks for the files
Also applies to: 41-41
client/apps/landing/src/components/modules/player-leaderboard-panel.tsx (2)
Line range hint
1-190: Well-structured component with accessibility and UX considerations!The implementation includes:
- Clear prop documentation
- Accessible tooltips
- Responsive grid layout
- Proper number formatting
- Efficient pagination
- Search functionality
14-14: Verify the PlayerInfo type compatibility.The change from
Player[]toPlayerInfo[]type needs verification to ensure all required properties (name, realms, mines, hyperstructures, points, percentage, lords, isAlive) are present in the new type.✅ Verification successful
Type change is compatible and correctly implemented
The PlayerInfo interface contains all required properties (name, realms, mines, hyperstructures, points, percentage, lords, isAlive) and they are properly used throughout the leaderboard component.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for PlayerInfo type definition and its usage ast-grep --pattern 'type PlayerInfo = { $$$ }' # Check for any other components using PlayerInfo rg -l "PlayerInfo"Length of output: 756
Script:
#!/bin/bash # Get PlayerInfo type definition cat packages/core/src/types/common.ts | grep -A 20 "type PlayerInfo" # Check property usage in leaderboard panel rg -A 5 "players\." client/apps/landing/src/components/modules/player-leaderboard-panel.tsxLength of output: 497
Script:
#!/bin/bash # Try both interface and type patterns for PlayerInfo ast-grep --pattern 'interface PlayerInfo { $$$ }' ast-grep --pattern 'type PlayerInfo = { $$$ }' # Get more context from the component cat client/apps/landing/src/components/modules/player-leaderboard-panel.tsxLength of output: 9141
packages/core/src/modelManager/ConfigManager.ts (1)
Line range hint
91-98: Good simplification of initialization methods!The refactor improves maintainability by:
- Removing complex component queries
- Using direct config access
- Simplifying the initialization logic
Also applies to: 167-172, 204-209, 242-247
🧰 Tools
🪛 GitHub Actions: lint
[warning] Code formatting does not match Prettier standards. Run Prettier with --write to fix.
client/apps/landing/tsconfig.json (1)
7-9: Clean and minimal path configuration!The simplified paths configuration maintains the essential
@/*mapping while removing unnecessary complexity.
| @@ -15,6 +15,7 @@ export default defineConfig(({ mode }) => { | |||
| resolve: { | |||
| alias: { | |||
| "@": path.resolve(__dirname, "./src"), | |||
| "@/assets": path.resolve(__dirname, "../../public/assets"), | |||
There was a problem hiding this comment.
💡 Codebase verification
Misaligned public directory configurations
The current setup has inconsistent public directory configurations:
publicDirpoints to../../common/public@/assetsalias points to../../public/assets- Actual assets are located in
client/public/assets/
These paths should be aligned to use the same base public directory to ensure correct asset serving and building.
🔗 Analysis chain
Verify assets directory structure and alignment.
The new alias points to ../../public/assets while the publicDir is set to ../../common/public. Verify that this directory structure is intentional and the assets are in the correct location.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if both directories exist and their relationship
echo "Checking public directories..."
fd -t d "public$" --max-depth 3
echo "Checking assets directories..."
fd -t d "assets$" --max-depth 4
Length of output: 246
Script:
#!/bin/bash
# Check the vite config file for publicDir setting
echo "Checking vite.config.ts content..."
cat client/apps/landing/vite.config.ts
Length of output: 1145
| "storybook": "storybook dev -p 6006", | ||
| "vite-plugin-pwa": "^0.20.5" |
There was a problem hiding this comment.
Move vite-plugin-pwa to dependencies section.
The vite-plugin-pwa entry is incorrectly placed in the scripts section. It should be moved to the dependencies section.
"scripts": {
- "storybook": "storybook dev -p 6006",
- "vite-plugin-pwa": "^0.20.5"
+ "storybook": "storybook dev -p 6006"
},
"dependencies": {
+ "vite-plugin-pwa": "^0.20.5",
"@ark-project/core": "3.0.0-beta.1",Committable suggestion skipped: line range outside the PR's diff.
Summary by CodeRabbit
Release Notes
New Features
Improvements
Changes
Technical Updates