feat: introduce new event for discord bot + rework onboarding #2208
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
|
Warning Rate limit exceeded@edisontim has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 47 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes in this pull request involve multiple modifications across various components, primarily enhancing user account management, UI interactions, and event handling. Key updates include the integration of spectator mode in the Changes
Possibly related PRs
Suggested reviewers
Poem
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 (
|
|
You are out of MentatBot reviews. Your usage will refresh December 9 at 08:00 AM. |
|
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 and nitpick comments (12)
discord-bot/src/events/game_ended.rs (2)
26-34: Consider extracting color constantsThe hardcoded
Color::REDshould be moved to a constants file for consistency across different event types.- .color(poise::serenity_prelude::Color::RED) + .color(crate::constants::GAME_END_COLOR)
27-30: Consider handling long addresses gracefullyThe title message could become very long with the full felt address. Consider truncating or formatting the address for better readability.
- felt_to_string(&self.winner_address).unwrap_or(UNKNOWN_USER.to_string()), + truncate_address(&felt_to_string(&self.winner_address).unwrap_or(UNKNOWN_USER.to_string()))discord-bot/src/events/battle_start.rs (1)
19-19: Consider addressing the TODO commentThere's a TODO comment about using build.rs for ABI loading. Since you're updating the contract path, this might be a good time to implement that TODO.
Would you like me to help create a build.rs script for ABI loading?
discord-bot/src/actors/torii_client_subscriber.rs (1)
Line range hint
21-29: Critical: Add GameEnded model to subscription list.The new "s0_eternum-GameEnded" model needs to be added to the
TORII_SUBSCRIPTION_MODELSconstant array to ensure the bot receives these events.Apply this change:
-const TORII_SUBSCRIPTION_MODELS: [&str; 6] = [ +const TORII_SUBSCRIPTION_MODELS: [&str; 7] = [ "s0_eternum-BattleClaimData", "s0_eternum-BattleJoinData", "s0_eternum-BattleLeaveData", "s0_eternum-BattlePillageData", "s0_eternum-BattleStartData", "s0_eternum-SettleRealmData", + "s0_eternum-GameEnded", ];client/src/ui/modules/onboarding/Steps.tsx (3)
43-43: Consider performance impact of backdrop-blur effect.While the backdrop blur effect enhances visual consistency with LoadingScreen.tsx, it might impact performance on lower-end devices. Consider adding a reduced motion/effects setting for better accessibility.
- <div className="backdrop-blur-3xl bg-black/20 self-center rounded-lg border p-6 md:p-12 text-gold w-full md:min-w-[400px] md:max-w-[800px] min-w-[600px] overflow-hidden relative z-50 shadow-2xl border-gradient"> + <div className={`${prefersReducedMotion ? 'bg-black/60' : 'backdrop-blur-3xl bg-black/20'} self-center rounded-lg border p-6 md:p-12 text-gold w-full md:min-w-[400px] md:max-w-[800px] min-w-[600px] overflow-hidden relative z-50 shadow-2xl border-gradient`}>
Line range hint
58-63: Consider removing unnecessary timeout.The setTimeout usage here might be unnecessary. The state updates should be batched by React, and the event dispatch could happen immediately.
- setTimeout(() => { showBlankOverlay(false); handleUrlChange(new Position({ x: 0, y: 0 }).toMapLocationUrl()); window.dispatchEvent(new Event(ACCOUNT_CHANGE_EVENT)); - }, 250);
Line range hint
156-182: Critical: Secure handling of sensitive account data needed.The current implementation stores private keys in localStorage and exposes them through clipboard operations, which poses security risks:
- localStorage is not secure for storing private keys
- Clipboard operations expose private keys in plain text
Consider:
- Using a secure storage solution (e.g., encrypted storage)
- Never exposing full private keys to users
- Adding a warning about security implications
Also, improve error handling specificity:
} catch (e) { console.error(e); - setImportMessage("Invalid account"); + setImportMessage(e instanceof Error ? e.message : "Invalid account format"); }client/src/ui/modules/rewards/Rewards.tsx (1)
16-16: Consider using path aliases for cleaner importsThe relative import path with multiple "../" could be simplified using path aliases (e.g., "@/env") for better maintainability.
client/src/hooks/context/DojoContext.tsx (1)
187-187: Consider extracting account selection logic for better readabilityThe account selection logic combines multiple conditions (dev mode and spectator mode). Consider extracting this into a separate function for better maintainability.
-const accountToUse = !isSpectatorMode && !isDev ? controllerAccount : burnerAccount; +const getAccountToUse = (isSpectatorMode: boolean, isDev: boolean) => { + if (isSpectatorMode || isDev) { + return burnerAccount; + } + return controllerAccount; +}; +const accountToUse = getAccountToUse(isSpectatorMode, isDev);client/src/ui/elements/Tooltip.tsx (1)
39-42: Consider documenting positioning values.The translation values for different positions (-110%, 70%, 30%) seem carefully chosen. Consider adding a comment explaining these specific values to help future maintenance.
+ // Positioning values are chosen to ensure proper spacing and visibility: + // top: moves up by 110% to clear the element + // bottom: moves down by 70% to provide adequate spacing + // left/right: combines X/Y translations for optimal placement position == "top" && "-translate-y-[110%]", position == "bottom" && "translate-y-[70%]", position == "left" && "-translate-x-[110%] -translate-y-1/2", position == "right" && "translate-x-[30%] -translate-y-1/2",client/src/ui/modules/LoadingScreen.tsx (1)
52-53: Consider adding loading animation for better user experienceThe static message about the new season could benefit from a loading animation to indicate system activity.
-<div className="p-4 text-center"> +<div className="p-4 text-center animate-pulse">client/src/ui/modules/navigation/LeftNavigationModule.tsx (1)
Missing dependencies in useMemo dependency array
The dependency array is missing several variables that are used within the useMemo callback:
setViewtoggleModalisPopupOpenquestsPopupmilitaryconstructiontradeworldStructuresThese variables should be included in the dependency array to ensure proper memoization and prevent stale closures. Note that
IS_MOBILEcan be excluded if it's a constant.🔗 Analysis chain
Line range hint
266-275: Verify dependency list completeness in useMemoThe dependency array includes all necessary values, ensuring proper memoization of the navigation items.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential missing dependencies in useMemo # Look for variables used inside the useMemo callback ast-grep --pattern 'useMemo(() => { $$$ $var $$$ }, [$deps])'Length of output: 69
Script:
#!/bin/bash # Let's try to get the complete useMemo implementation to analyze dependencies rg -U "useMemo\(\(\) => \{[\s\S]*?\}, \[[^\]]*\]\)" client/src/ui/modules/navigation/LeftNavigationModule.tsxLength of output: 5972
Script:
#!/bin/bash # Let's check for any variables used in the useMemo callback that might be missing from deps array rg -U "const navigation = useMemo\(\(\) => \{[\s\S]*?\}, \[[^\]]*\]\)" client/src/ui/modules/navigation/LeftNavigationModule.tsx | rg -o "(military|construction|trade|worldStructures|setView|toggleModal|isPopupOpen|questsPopup|IS_MOBILE)"Length of output: 698
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
discord-bot/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
client/src/hooks/context/DojoContext.tsx(7 hunks)client/src/hooks/helpers/use-settle-realm.tsx(0 hunks)client/src/ui/elements/Tooltip.tsx(1 hunks)client/src/ui/modules/LoadingScreen.tsx(1 hunks)client/src/ui/modules/navigation/LeftNavigationModule.tsx(1 hunks)client/src/ui/modules/navigation/SecondaryMenuItems.tsx(2 hunks)client/src/ui/modules/onboarding/Steps.tsx(3 hunks)client/src/ui/modules/rewards/Rewards.tsx(2 hunks)contracts/torii.toml(1 hunks)discord-bot/Cargo.toml(2 hunks)discord-bot/src/actors/torii_client_subscriber.rs(2 hunks)discord-bot/src/events/battle_start.rs(2 hunks)discord-bot/src/events/game_ended.rs(1 hunks)discord-bot/src/events/mod.rs(1 hunks)
💤 Files with no reviewable changes (1)
- client/src/hooks/helpers/use-settle-realm.tsx
✅ Files skipped from review due to trivial changes (1)
- contracts/torii.toml
🔇 Additional comments (19)
discord-bot/Cargo.toml (3)
31-32: Verify Cainome update changelog
The update of Cainome dependencies from v0.4.7 to v0.4.9 looks good. Both dependencies are consistently using the same version.
✅ Verification successful
Cainome update from v0.4.7 to v0.4.9 is safe to proceed
The changelog shows no breaking changes across versions:
- v0.4.9: Added deep nested full recursion mode
- v0.4.8: Added operators support and composite generic_args hydration
- v0.4.7: Added event selector for event structs
All changes are feature additions and improvements that maintain backward compatibility. The event selector feature in v0.4.7 aligns well with the Discord bot event functionality being developed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check changelog between v0.4.7 and v0.4.9 for breaking changes
# Get release notes for the versions
gh api repos/cartridge-gg/cainome/releases --jq '.[] | select(.tag_name | test("v0.4.[7-9]")) | {version: .tag_name, body: .body}'
Length of output: 1088
23-29: LGTM! Good migration to official repository
The switch from a personal fork to the official dojoengine repository and the consistent use of v1.0.3 tag across all Dojo dependencies is a good improvement for maintainability.
5-5: Verify Rust 2021 edition compatibility
The upgrade to Rust 2021 edition is a good practice, but we should ensure all code is compatible with the new edition's changes.
✅ Verification successful
Rust 2021 edition upgrade is compatible with the codebase
The code analysis shows that the Discord bot's codebase primarily uses modern Rust features like async/await, which are well-supported in both 2018 and 2021 editions. No problematic patterns were found that would be affected by the 2021 edition changes:
- No array pattern matching that would be impacted by the new edition
- No direct macro resolution issues detected
- No const generics usage that would require 2021 edition
- The async/await syntax used is compatible with both editions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potential edition-specific issues
# Look for common patterns that might need updates in 2021 edition
# Check for array patterns (2021 edition changes how they work)
rg "let \[.*\]"
# Check for panic macro calls (2021 edition changes how they're resolved)
rg "panic!\("
# Check for uses of Cargo resolver v1 features
grep -l "\[features\]" $(fd -e toml)
Length of output: 2401
Script:
#!/bin/bash
# Let's check the discord-bot directory specifically for Rust code patterns
# that might be affected by the 2021 edition changes
# Check for array patterns in Rust files
rg "let \[.*\]" discord-bot/src/
# Check for potential edition-specific syntax
rg "async fn" discord-bot/src/
# Look for potential macro resolution changes
rg "use (std|tokio)::.+!;" discord-bot/src/
# Check for potential const generics usage (new in 2021)
rg "const N: \w+" discord-bot/src/
# Look at the actual Rust files to understand the codebase better
fd -e rs . discord-bot/src/ --exec echo "=== {} ===" \; -X cat
Length of output: 2164
discord-bot/src/events/game_ended.rs (1)
16-20: Consider adding validation for winner_address
The GameEnded struct should validate that the winner_address is not zero/empty to ensure meaningful announcements.
discord-bot/src/events/mod.rs (1)
6-6: LGTM!
The new module is properly registered and follows the existing pattern.
discord-bot/src/events/battle_start.rs (1)
59-59: LGTM!
The spacing fix in the message format improves readability.
discord-bot/src/actors/torii_client_subscriber.rs (3)
16-17: LGTM: Import changes are correct and well-organized.
The addition of game_ended::GameEnded import is properly placed with other related event imports.
166-172: Implementation looks good but verify subscription configuration.
The new event handling implementation follows the established pattern and is well-structured.
166-172: Verify GameEnded event implementation completeness.
Let's verify the complete implementation chain for the new GameEnded event.
✅ Verification successful
Let me check the actual implementation of GameEnded to ensure it's complete.
Let me check one more thing to verify the event handling chain.
GameEnded event implementation is complete and properly integrated
The implementation chain for GameEnded event is fully complete:
- Struct is properly defined with CairoSerde derivation
- ToDiscordMessage trait is fully implemented with proper message formatting
- Event is correctly registered and deserialized in the torii client subscriber
- No TODOs or unimplemented markers related to GameEnded
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify GameEnded event implementation completeness
# Check for GameEnded struct implementation
echo "Checking GameEnded struct implementation..."
rg -p "struct GameEnded" discord-bot/src/events/
# Check for ToDiscordMessage trait implementation
echo "Checking ToDiscordMessage implementation..."
rg -p "impl.*ToDiscordMessage.*for.*GameEnded" discord-bot/src/events/
# Check for any TODO or unimplemented! markers
echo "Checking for incomplete implementations..."
rg -p "TODO|unimplemented!.*GameEnded" discord-bot/src/events/
Length of output: 760
Script:
#!/bin/bash
# Check the complete implementation of GameEnded
echo "Checking GameEnded implementation details..."
cat discord-bot/src/events/game_ended.rs
# Check for any usage of GameEnded in handlers
echo -e "\nChecking GameEnded usage in handlers..."
rg -p "GameEnded" discord-bot/src/handlers/
Length of output: 2309
Script:
#!/bin/bash
# Check for event registration and handling
echo "Checking event registration..."
rg -p "GameEnded" discord-bot/src/actors/
Length of output: 388
client/src/ui/modules/onboarding/Steps.tsx (2)
80-83: LGTM: Improved button text clarity.
The button text changes from "Enter as Spectator" to "Spectate" and "Choose your Leader" to "Play" improve UI clarity and conciseness.
32-32: LGTM: Event constant export enables better code organization.
The export of ACCOUNT_CHANGE_EVENT constant improves code modularity by allowing its reuse across different components.
Let's verify the usage of this constant across the codebase:
✅ Verification successful
Export of ACCOUNT_CHANGE_EVENT constant is well-utilized across components
The constant is properly imported and consistently used across the codebase:
- Used in
Steps.tsxfor dispatching events in multiple locations - Imported and utilized in
DojoContext.tsxfor event handling - All usages maintain consistent event name "addressChanged"
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for all usages of ACCOUNT_CHANGE_EVENT to ensure consistent event handling
rg "ACCOUNT_CHANGE_EVENT" --type ts --type tsx
Length of output: 74
Script:
#!/bin/bash
# Search for ACCOUNT_CHANGE_EVENT without file type restrictions since the extension might not be recognized
rg "ACCOUNT_CHANGE_EVENT"
Length of output: 547
client/src/ui/modules/rewards/Rewards.tsx (1)
96-96: LGTM: Environment variable access updated correctly
The change from process.env to the new env module is implemented correctly.
client/src/hooks/context/DojoContext.tsx (2)
Line range hint 2-17: LGTM: Clean state management implementation
The new imports and state management hooks are well-organized and follow React best practices. The state management implementation using useUIStore is clean and properly structured.
Also applies to: 138-142
164-164: LGTM: Efficient loading screen implementation
The loading screen implementation is well-optimized:
- Background image is properly memoized to prevent unnecessary re-renders
- Loading states are handled consistently across different scenarios
Also applies to: 211-211, 217-217, 221-221, 266-266
client/src/ui/elements/Tooltip.tsx (1)
39-39: LGTM! Verify tooltip visibility with various content lengths.
The increased vertical offset for bottom-positioned tooltips (70%) provides better spacing from the trigger element. However, please verify that the tooltip remains fully visible with:
- Different content lengths
- Various screen sizes
- Different UI element positions
✅ Verification successful
Based on the search results, I can now generate the final response:
Tooltip positioning change appears safe and well-integrated
The increased vertical offset (70%) for bottom-positioned tooltips is consistent with the codebase usage patterns:
- The Tooltip component is used across multiple UI components but primarily through a centralized implementation
- The tooltip positioning is managed through a global UI store with standardized position types
- Most tooltip usages are for small UI elements (icons, buttons, labels) where the 70% offset provides better spacing without risking overflow
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find tooltip usage to verify the positioning change impact
# Look for components using the Tooltip with bottom position
# Search for direct tooltip usage with bottom position
rg -l "position.*bottom.*Tooltip" --type tsx
# Search for tooltip content to analyze typical lengths
ast-grep --pattern 'tooltip={{ content: $_, position: "bottom" }}'
Length of output: 143
Script:
#!/bin/bash
# Search for Tooltip usage with bottom position, adjusting file type
rg -l "position.*bottom.*Tooltip" --type ts --type tsx --type jsx --type js
# Search for tooltip content patterns with different variations
rg -A 2 'tooltip.*position.*bottom' --type ts --type tsx --type jsx --type js
# Look for any Tooltip component usage to analyze patterns
rg -A 2 '<Tooltip' --type ts --type tsx --type jsx --type js
Length of output: 304
Script:
#!/bin/bash
# Search for files containing Tooltip usage
rg -l "Tooltip"
# Then search for tooltip content and positioning patterns
rg -A 2 'tooltip.*position'
# Look for any Tooltip component usage
rg -A 2 '<Tooltip'
Length of output: 17553
client/src/ui/modules/LoadingScreen.tsx (1)
50-50: LGTM: Enhanced visual aesthetics with backdrop blur
The semi-transparent black background with blur effect provides better contrast and modern aesthetics compared to the previous brown background.
client/src/ui/modules/navigation/SecondaryMenuItems.tsx (2)
8-8: LGTM: Added HintModal import
The import is correctly placed with other UI component imports.
131-133: Verify HintModal integration with game state
The HintModal should be context-aware and show relevant hints based on the current game state.
client/src/ui/modules/navigation/LeftNavigationModule.tsx (1)
264-264: LGTM: Simplified navigation logic
The direct return of filteredNavigation improves code readability without affecting functionality.
| if (!isConnected && !isConnecting && !controllerAccount && !isSpectatorMode) { | ||
| return ( | ||
| <div className="relative h-screen w-screen pointer-events-auto"> | ||
| <img className="absolute h-screen w-screen object-cover" src={getRandomBackgroundImage()} alt="Cover" /> | ||
| <img | ||
| className="absolute h-screen w-screen object-cover" | ||
| src={`/images/covers/${backgroundImage}.png`} | ||
| alt="Cover" | ||
| /> | ||
| <div className="absolute z-10 w-screen h-screen flex justify-center flex-wrap self-center"> | ||
| <div className="self-center bg-brown rounded-lg border p-4 md:p-8 text-gold w-[90%] md:min-w-[600px] md:max-w-[800px] overflow-hidden relative z-50 shadow-2xl border-white/40 border-gradient mx-4"> | ||
| <div className="w-full text-center pt-2 md:pt-6"> | ||
| <div className="self-center bg-black/20 backdrop-blur-3xl rounded-lg border p-4 md:p-8 text-gold w-[50%] md:min-w-[600px] md:max-w-[800px] overflow-hidden relative z-50 shadow-2xl border-white/40 border-gradient mx-4"> | ||
| <div className="w-full text-center"> | ||
| <div className="mx-auto flex mb-2 md:mb-8"> | ||
| <img src="/images/eternum_with_snake.png" className="w-48 md:w-96 mx-auto" alt="Eternum Logo" /> | ||
| <EternumWordsLogo className="fill-current w-64 stroke-current mx-auto" /> | ||
| </div> | ||
| </div> | ||
| <div className="flex space-x-2 mt-2 md:mt-8 justify-center"> | ||
| <div className="flex space-x-8 mt-2 md:mt-8 justify-center mb-2"> | ||
| {!isConnected && ( | ||
| <button | ||
| className="px-3 md:px-4 py-2 bg-[#ffc52a] border-2 border-[#ffc52a] text-black flex font-bold rounded text-base md:text-lg fill-black uppercase leading-6 shadow-md hover:shadow-lg active:shadow-inner hover:scale-105 transition-all duration-300 hover:-translate-y-1" | ||
| onClick={connectWallet} | ||
| > | ||
| <CartridgeSmall className="w-5 md:w-6 mr-1 md:mr-2 fill-current self-center" /> Login | ||
| </button> | ||
| <> | ||
| <Button | ||
| size="md" | ||
| variant="outline" | ||
| className="w-28 border border-gold/30 hover:border-gold/50 h-12 hover:scale-105 hover:-translate-y-1" | ||
| onClick={onSpectatorModeClick} | ||
| > | ||
| Spectate | ||
| </Button> | ||
| <button | ||
| className="cursor-pointer px-3 md:px-4 py-2 bg-gold border-2 border-gold text-black flex font-bold rounded text-base md:text-lg fill-black uppercase leading-6 shadow-md hover:shadow-lg active:shadow-inner hover:scale-105 transition-all duration-300 hover:-translate-y-1" | ||
| onClick={connectWallet} | ||
| > | ||
| <CartridgeSmall className="w-5 md:w-6 mr-1 md:mr-2 fill-current self-center" /> Login | ||
| </button> | ||
| </> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve UI consistency and maintainability
The UI implementation has inconsistent button styling approaches:
- The "Spectate" button uses the Button component
- The "Login" button uses inline styles
Additionally, the conditional rendering could be simplified.
- Use the Button component consistently:
-<button
- className="cursor-pointer px-3 md:px-4 py-2 bg-gold border-2 border-gold text-black flex font-bold rounded text-base md:text-lg fill-black uppercase leading-6 shadow-md hover:shadow-lg active:shadow-inner hover:scale-105 transition-all duration-300 hover:-translate-y-1"
- onClick={connectWallet}
->
- <CartridgeSmall className="w-5 md:w-6 mr-1 md:mr-2 fill-current self-center" /> Login
-</button>
+<Button
+ size="md"
+ variant="primary"
+ className="flex items-center"
+ onClick={connectWallet}
+>
+ <CartridgeSmall className="w-5 md:w-6 mr-1 md:mr-2 fill-current" />
+ Login
+</Button>- Extract the button container for better maintainability:
const ActionButtons = ({ isConnected }: { isConnected: boolean }) => {
if (isConnected) return null;
return (
<div className="flex space-x-8 mt-2 md:mt-8 justify-center mb-2">
<Button
size="md"
variant="outline"
className="w-28 border border-gold/30 hover:border-gold/50 h-12 hover:scale-105 hover:-translate-y-1"
onClick={onSpectatorModeClick}
>
Spectate
</Button>
<Button
size="md"
variant="primary"
className="flex items-center"
onClick={connectWallet}
>
<CartridgeSmall className="w-5 md:w-6 mr-1 md:mr-2 fill-current" />
Login
</Button>
</div>
);
};| const onSpectatorModeClick = () => { | ||
| setTimeout(() => { | ||
| setSpectatorMode(true); | ||
| handleUrlChange(new Position({ x: 0, y: 0 }).toMapLocationUrl()); | ||
| window.dispatchEvent(new Event(ACCOUNT_CHANGE_EVENT)); | ||
| showBlankOverlay(false); | ||
| }, 500); | ||
| }; |
There was a problem hiding this comment.
Review setTimeout usage and add error handling
The spectator mode implementation has several potential issues:
- The setTimeout usage seems unnecessary and could lead to race conditions
- Multiple state updates could be batched
- Missing error handling for Position object creation
Consider this safer implementation:
-const onSpectatorModeClick = () => {
- setTimeout(() => {
+const onSpectatorModeClick = () => {
+ try {
setSpectatorMode(true);
handleUrlChange(new Position({ x: 0, y: 0 }).toMapLocationUrl());
window.dispatchEvent(new Event(ACCOUNT_CHANGE_EVENT));
showBlankOverlay(false);
- }, 500);
+ } catch (error) {
+ console.error('Failed to enter spectator mode:', error);
+ // Consider adding error handling UI feedback
+ }
};📝 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 onSpectatorModeClick = () => { | |
| setTimeout(() => { | |
| setSpectatorMode(true); | |
| handleUrlChange(new Position({ x: 0, y: 0 }).toMapLocationUrl()); | |
| window.dispatchEvent(new Event(ACCOUNT_CHANGE_EVENT)); | |
| showBlankOverlay(false); | |
| }, 500); | |
| }; | |
| const onSpectatorModeClick = () => { | |
| try { | |
| setSpectatorMode(true); | |
| handleUrlChange(new Position({ x: 0, y: 0 }).toMapLocationUrl()); | |
| window.dispatchEvent(new Event(ACCOUNT_CHANGE_EVENT)); | |
| showBlankOverlay(false); | |
| } catch (error) { | |
| console.error('Failed to enter spectator mode:', error); | |
| // Consider adding error handling UI feedback | |
| } | |
| }; |
| <div className="flex justify-center hover:scale-105 transition-all duration-300 hover:-translate-y-1"> | ||
| <CircleButton | ||
| tooltipLocation="bottom" | ||
| image={BuildingThumbs.discord} | ||
| label={"Discord"} | ||
| size="lg" | ||
| className="bg-black/0 border-none" | ||
| onClick={() => window.open("https://discord.gg/realmsworld")} | ||
| /> |
There was a problem hiding this comment.
💡 Codebase verification
Discord invite link needs attention - returns HTTP 301 redirect
The Discord invite link discord.gg/realmsworld returns a 301 status code (permanent redirect) instead of a 200 status code, indicating potential issues with the link's validity or accessibility. This should be reviewed and updated with a valid, permanent invite link.
🔗 Analysis chain
Verify Discord invite link validity
The Discord invite link should be checked periodically to ensure it remains active and valid.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify Discord invite link validity
# Expected result: HTTP 200 status code indicates valid invite
curl -s -o /dev/null -w "%{http_code}" https://discord.gg/realmsworld
Length of output: 75
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (9)
client/src/hooks/context/DojoContext.tsx (2)
192-196: Consider adding type safety for account determinationThe account determination logic could benefit from type guards to ensure type safety when switching between different account types.
- const accountToUse = isDev - ? burnerAccount - : isSpectatorMode - ? new Account(value.network.provider.provider, "0x0", "0x0") - : controllerAccount; + const accountToUse = (() => { + if (isDev && burnerAccount) { + return burnerAccount; + } + if (isSpectatorMode) { + return new Account(value.network.provider.provider, "0x0", "0x0"); + } + if (controllerAccount) { + return controllerAccount; + } + throw new Error("No valid account configuration found"); + })();
241-241: Consider extracting styles to a reusable componentThe container div has complex styling that could be reused across the application.
Consider creating a reusable component:
const GlassContainer: React.FC<{ children: React.ReactNode }> = ({ children }) => ( <div className="self-center bg-black/20 backdrop-blur-3xl rounded-lg border p-4 md:p-8 text-gold w-[50%] md:min-w-[600px] md:max-w-[800px] overflow-hidden relative z-50 shadow-2xl border-white/40 border-gradient mx-4"> {children} </div> );client/src/hooks/helpers/useResources.tsx (2)
Line range hint
71-156: Consider splitting usePlayerArrivals into smaller hooksThe hook handles multiple concerns (structure positions, arrivals, inventory) which could be separated for better maintainability and performance.
Consider splitting into:
-export const usePlayerArrivals = () => { +const usePlayerStructurePositions = () => { + // Position-related logic +}; + +const useArrivalsWithInventory = (positions: Position[]) => { + // Arrivals and inventory logic +}; + +export const usePlayerArrivals = () => { + const positions = usePlayerStructurePositions(); + const arrivals = useArrivalsWithInventory(positions); + return arrivals; +};
Line range hint
214-229: Address deprecated useBalance hookThe comment indicates this hook should be deprecated due to complexity and multiple useEffects. This should be addressed to prevent potential performance issues.
Consider replacing with:
-const useBalance = (entityId: ID, resourceId: ResourcesIds) => { - const currentDefaultTick = useUIStore.getState().currentDefaultTick; - const [resourceBalance, setResourceBalance] = useState<Resource>({ amount: 0, resourceId }); - - useEffect(() => { - const resourceManager = new ResourceManager(dojo.setup, entityId, resourceId); - setResourceBalance({ amount: resourceManager.balance(currentDefaultTick), resourceId }); - }, []); - - return resourceBalance; -}; +const useBalance = (entityId: ID, resourceId: ResourcesIds) => { + const currentDefaultTick = useUIStore.getState().currentDefaultTick; + const resourceManager = useMemo( + () => new ResourceManager(dojo.setup, entityId, resourceId), + [dojo.setup, entityId, resourceId] + ); + return useMemo( + () => ({ amount: resourceManager.balance(currentDefaultTick), resourceId }), + [resourceManager, currentDefaultTick] + ); +};client/src/hooks/helpers/useQuests.tsx (2)
92-95: Optimize structurePosition memoizationThe current memoization depends on getEntityInfo function reference which could cause unnecessary recalculations.
Consider extracting position retrieval:
-const structurePosition = useMemo( - () => getEntityInfo(structureEntityId)?.position || { x: 0, y: 0 }, - [structureEntityId, getEntityInfo], -); +const getPosition = useCallback( + (id: ID) => getEntityInfo(id)?.position || { x: 0, y: 0 }, + [getEntityInfo] +); +const structurePosition = useMemo( + () => getPosition(structureEntityId), + [structureEntityId, getPosition] +);
71-76: Add type safety to useQuestDependenciesWhile removing parameters simplifies the hook, adding type annotations would improve maintainability.
Consider adding types:
-const useQuestDependencies = () => { +interface QuestDependencies { + setup: SetupResult; + account: { account: Account }; +} + +const useQuestDependencies = (): QuestDependencies => {client/src/three/components/Minimap.ts (3)
154-154: LGTM! Consider adding error logging.The null check for canvas and mapSize prevents potential runtime errors. However, consider adding error logging to help with debugging when these conditions occur.
- if (!this.canvas || !this.mapSize) return; + if (!this.canvas || !this.mapSize) { + console.warn('Minimap: Cannot recompute scales - canvas or mapSize is not initialized'); + return; + }
330-332: LGTM! Consider consistent error handling.The null check for canvas prevents potential runtime errors. For consistency with recomputeScales(), consider adding error logging here as well.
- if (!this.canvas) return; + if (!this.canvas) { + console.warn('Minimap: Cannot show minimap - canvas is not initialized'); + return; + }
Line range hint
1-577: Consider implementing cleanup and additional error handling.A few suggestions to improve the robustness of the implementation:
- Memory Management:
- Add a dispose/cleanup method to remove event listeners and clear caches
- Consider implementing WeakMap for caching where appropriate
- Error Handling:
- Add null checks for canvas context in draw methods
- Handle image loading errors in loadImage method
- Performance:
- Consider using requestAnimationFrame for smoother animations
- Implement dirty checking to avoid unnecessary redraws
Would you like me to provide specific code examples for any of these improvements?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
client/src/hooks/context/DojoContext.tsx(7 hunks)client/src/hooks/helpers/useEntities.tsx(1 hunks)client/src/hooks/helpers/useQuests.tsx(3 hunks)client/src/hooks/helpers/useResources.tsx(2 hunks)client/src/main.tsx(1 hunks)client/src/three/components/Minimap.ts(2 hunks)client/src/ui/modules/navigation/LeftNavigationModule.tsx(1 hunks)client/src/ui/modules/navigation/SecondaryMenuItems.tsx(3 hunks)eternum-docs/docs/pages/mechanics/hyperstructures.mdx(1 hunks)eternum-docs/docs/pages/mechanics/realm/wonders.mdx(1 hunks)landing/src/hooks/query/players.tsx(0 hunks)landing/src/routes/my-empire.tsx(2 hunks)
💤 Files with no reviewable changes (1)
- landing/src/hooks/query/players.tsx
✅ Files skipped from review due to trivial changes (2)
- eternum-docs/docs/pages/mechanics/hyperstructures.mdx
- landing/src/routes/my-empire.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- client/src/ui/modules/navigation/SecondaryMenuItems.tsx
- client/src/ui/modules/navigation/LeftNavigationModule.tsx
🔇 Additional comments (6)
eternum-docs/docs/pages/mechanics/realm/wonders.mdx (1)
13-13: LGTM: Clear and valuable game mechanic documentation
The added information about donkey production costs for Wonders is well-documented and provides valuable information to players.
client/src/hooks/context/DojoContext.tsx (3)
Line range hint 96-118: LGTM: Clean provider implementation
The DojoProvider implementation properly handles the new backgroundImage prop and correctly validates single instance usage.
232-264: Improve UI consistency and maintainability
The UI implementation has inconsistent button styling approaches:
- The "Spectate" button uses the Button component
- The "Login" button uses inline styles
Additionally, the conditional rendering could be simplified.
Apply this refactor:
-<button
- className="cursor-pointer px-3 md:px-4 py-2 bg-gold border-2 border-gold text-black flex font-bold rounded text-base md:text-lg fill-black uppercase leading-6 shadow-md hover:shadow-lg active:shadow-inner hover:scale-105 transition-all duration-300 hover:-translate-y-1"
- onClick={connectWallet}
->
- <CartridgeSmall className="w-5 md:w-6 mr-1 md:mr-2 fill-current self-center" /> Login
-</button>
+<Button
+ size="md"
+ variant="primary"
+ className="flex items-center"
+ onClick={connectWallet}
+>
+ <CartridgeSmall className="w-5 md:w-6 mr-1 md:mr-2 fill-current" />
+ Login
+</Button>183-188:
Improve error handling in spectator mode
The spectator mode implementation lacks error handling for potential failures in Position object creation or event dispatch.
Apply this safer implementation:
-const onSpectatorModeClick = () => {
+const onSpectatorModeClick = () => {
+ try {
setSpectatorMode(true);
handleUrlChange(new Position({ x: 0, y: 0 }).toMapLocationUrl());
window.dispatchEvent(new Event(ACCOUNT_CHANGE_EVENT));
showBlankOverlay(false);
+ } catch (error) {
+ console.error('Failed to enter spectator mode:', error);
+ // Consider adding user feedback for failures
+ }
};client/src/hooks/helpers/useResources.tsx (1)
Line range hint 21-32: LGTM: Simplified setup access pattern
The refactoring improves code clarity while maintaining functionality. The ResourceManager instantiation pattern is consistent.
✅ Verification successful
LGTM: ResourceManager instantiation pattern is consistent across the codebase
The ResourceManager class is consistently instantiated with the same three parameters (setup, entityId, resourceId) throughout the codebase. The refactoring in useResources.tsx maintains this pattern and correctly uses the setup object from useDojo().
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify consistent ResourceManager instantiation pattern
ast-grep --pattern 'new ResourceManager($setup, $entityId, $resourceId)'
Length of output: 74
Script:
#!/bin/bash
# Search for ResourceManager class usage patterns
rg "new ResourceManager" -A 2
Length of output: 3703
Script:
#!/bin/bash
# Find ResourceManager class definition
fd "ResourceManager" --type f
Length of output: 126
Script:
#!/bin/bash
# Search for ResourceManager imports
rg "import.*ResourceManager"
Length of output: 735
Script:
#!/bin/bash
# Check ResourceManager class implementation
cat client/src/dojo/modelManager/ResourceManager.ts
Length of output: 8532
client/src/hooks/helpers/useEntities.tsx (1)
185-185: LGTM: Army component addition
The Army component is properly integrated into the components list.
client/src/main.tsx
Outdated
| @@ -47,7 +47,7 @@ async function init() { | |||
| root.render( | |||
| <React.StrictMode> | |||
| <StarknetProvider> | |||
| <DojoProvider value={setupResult}> | |||
| <DojoProvider value={setupResult} backgroundImage={backgroundImage}> | |||
There was a problem hiding this comment.
💡 Codebase verification
DojoProvider type definition needs to be updated
The backgroundImage prop is being passed to DojoProvider but it's not included in its type definition. From the search results:
type DojoProviderProps = {
children: ReactNode;
value: SetupResult;
};However, the component implementation accepts it:
export const DojoProvider = ({ children, value, backgroundImage }: DojoProviderProps & { backgroundImage: string })This inconsistency should be fixed by adding the backgroundImage prop directly to the DojoProviderProps type definition.
🔗 Analysis chain
Verify DojoProvider prop types
The backgroundImage prop is correctly passed to both DojoProvider and App components, maintaining consistency across the application.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify DojoProvider prop types include backgroundImage
ast-grep --pattern 'type DojoProviderProps = {
$$$
backgroundImage: string;
$$$
}'
Length of output: 90
Script:
#!/bin/bash
# Let's try to find the DojoProvider component definition and its props
rg -A 5 "DojoProvider.*=.*"
Length of output: 4019
Script:
#!/bin/bash
# Search for type definitions related to DojoProvider
rg "type.*DojoProvider"
Length of output: 162
Script:
#!/bin/bash
# Search for interface definitions related to DojoProvider
rg "interface.*DojoProvider"
Length of output: 30
Script:
#!/bin/bash
# Find files containing DojoProvider to analyze its implementation
fd -t f "DojoProvider"
Length of output: 22
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (11)
client/src/hooks/context/DojoContext.tsx (2)
204-213: Consider adding retry mechanism for username settingThe username setting logic could benefit from a retry mechanism in case of network issues.
const setUserName = async () => { + const MAX_RETRIES = 3; + let retries = 0; + + const attemptSetUsername = async () => { const username = await (connector as ControllerConnector)?.username(); if (!username) return; - value.systemCalls.set_address_name({ - signer: controllerAccount!, - name: username, - }); - setAddressName(username); + try { + await value.systemCalls.set_address_name({ + signer: controllerAccount!, + name: username, + }); + setAddressName(username); + } catch (error) { + if (retries < MAX_RETRIES) { + retries++; + await new Promise(resolve => setTimeout(resolve, 1000 * retries)); + await attemptSetUsername(); + } else { + console.error('Failed to set username after multiple attempts:', error); + } + } + }; + + await attemptSetUsername(); };
Line range hint
177-186: Add user feedback for wallet connection errorsConsider showing a toast or error message when wallet connection fails.
const connectWallet = async () => { try { console.log("Attempting to connect wallet..."); await connect({ connector: connectors[0] }); console.log("Wallet connected successfully."); } catch (error) { console.error("Failed to connect wallet:", error); + // Assuming you have a toast notification system + showToast({ + type: 'error', + message: 'Failed to connect wallet. Please try again.', + duration: 5000 + }); } };sdk/packages/eternum/src/provider/index.ts (2)
Line range hint
447-458: Consider parameterizing the frontend address increate_realm_devmethodIn the
create_realm_devmethod, thecalldataincludes a hardcoded frontend address"0x1a3e37c77be7de91a9177c6b57956faa6da25607e567b10a25cf64fea5e533b". To enhance flexibility and maintainability, consider parameterizing this address or documenting why a hardcoded value is necessary.
Line range hint
542-558: Parameterize the frontend address increate_multiple_realms_devmethodSimilar to the
create_realm_devmethod, thecreate_multiple_realms_devmethod includes a hardcoded frontend address in thecalldata. Consider parameterizing this value to improve adaptability across different development environments or clarifying the reason for hardcoding.client/src/ui/components/cityview/realm/SettleRealmComponentDev.tsx (1)
80-86: Ensure button displays appropriate label during loading stateIn the button for settling a selected realm, when
isLoadingistrue, the button label is set to an empty string. This may render an empty button during loading. Consider displaying a loading message like"Loading..."to inform the user that the action is in progress, ensuring consistent user feedback.Apply this diff to update the button label:
<Button isLoading={isLoading} onClick={async () => (!isLoading ? await settleRealm(tokenId) : null)} className="text-xl self-center" variant={"primary"} > - {!isLoading ? "Settle Realms" : ""} + {isLoading ? "Loading..." : "Settle Realm"} </Button>client/src/ui/components/cityview/realm/SettleRealmComponent.tsx (4)
116-119: Disable 'Settle' button when no realms are selectedTo prevent users from attempting to settle realms when none are selected, disable the 'Settle' button when
selectedRealmsis empty. This improves user experience by providing immediate visual feedback.Apply this diff to conditionally disable the button:
<Button className="text-xxs" variant="primary" size="xs" onClick={() => settleRealms(selectedRealms)} + disabled={selectedRealms.length === 0} > settle </Button>
195-195: Remove unnecessaryconsole.logstatementThe
console.logstatement appears to be leftover from debugging and should be removed to clean up the console output.Apply this diff to remove the
console.log:- console.log(seasonPassRealm.realmId, selected);
198-203: Improve accessibility by using abuttonelement for clickable contentUsing a
divwith anonClickhandler can lead to accessibility issues sincedivelements are not focusable or operable via keyboard by default. Consider using abuttonelement for better accessibility support.Apply this diff to replace the
divwith abutton:- <div + <button + type="button" className={`flex flex-col justify-between gap-2 border-4 p-2 rounded-md ${ selected ? "border-gold bg-gold/30" : "border-gold/10" } min-h-[10vh] max-h-[10vh] min-w-[15.8vw] max-w-[15.8vw] ${className}`} onClick={() => setSelected(!selected)} - > + >Ensure that the styles are adjusted appropriately to maintain the desired appearance.
130-130: Consider implementing pagination for large data setsThe GraphQL query
tokenBalancesuses a highlimitof 8000, which may lead to performance issues or timeouts when fetching large amounts of data. Implementing pagination can improve performance and user experience.Consider modifying the query to fetch data in smaller chunks and handle pagination:
query getAccountTokens($accountAddress: String!, $cursor: String) { tokenBalances(accountAddress: $accountAddress, first: 100, after: $cursor) { edges { node { // existing fields } } pageInfo { hasNextPage endCursor } } }Update the
querySeasonPassesfunction to handle pagination by repeatedly fetching data whilehasNextPageistrue.client/src/ui/layouts/Onboarding.tsx (2)
8-9: Add bounds checking to step navigationThe current navigation logic could potentially set
currentStepto invalid values. Consider adding bounds checking:- const nextStep = () => setCurrentStep(currentStep + 1); - const prevStep = () => setCurrentStep(currentStep - 1); + const nextStep = () => setCurrentStep(prev => Math.min(prev + 1, 4)); + const prevStep = () => setCurrentStep(prev => Math.max(prev - 1, env.VITE_PUBLIC_DEV ? 1 : 3));
20-23: Document the different onboarding flowsThe onboarding now has two distinct flows (development vs. production). Consider:
- Adding comments explaining the purpose of each flow
- Documenting why steps 1 and 2 are development-only
- Creating a visual diagram of the flows in the component's documentation
Also, since StepFour appears to be the final step (no onNext prop), consider renaming it to something more descriptive like
FinalSteporOnboardingComplete.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
client/src/dojo/createSystemCalls.ts(2 hunks)client/src/hooks/context/DojoContext.tsx(9 hunks)client/src/hooks/helpers/useResources.tsx(3 hunks)client/src/three/components/ArmyManager.ts(0 hunks)client/src/ui/components/cityview/realm/SettleRealmComponent.tsx(1 hunks)client/src/ui/components/cityview/realm/SettleRealmComponentDev.tsx(1 hunks)client/src/ui/layouts/Onboarding.tsx(2 hunks)client/src/ui/modules/onboarding/Steps.tsx(7 hunks)sdk/packages/eternum/src/provider/index.ts(4 hunks)sdk/packages/eternum/src/types/provider.ts(1 hunks)
💤 Files with no reviewable changes (1)
- client/src/three/components/ArmyManager.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- client/src/hooks/helpers/useResources.tsx
- client/src/ui/modules/onboarding/Steps.tsx
🔇 Additional comments (15)
client/src/hooks/context/DojoContext.tsx (4)
Line range hint 99-121: LGTM: Clean provider implementation
The backgroundImage prop is properly integrated into the provider hierarchy.
197-201: LGTM: Robust account selection logic
The account selection logic properly handles all cases: development, spectator mode, and production environments.
283-288: Maintain consistent button styling
This is a duplicate of a previous review comment. The login button should use the Button component for consistency.
188-193: Verify spectator mode state management
The spectator mode implementation looks good, but we should verify the state updates are handled correctly across component unmounts.
✅ Verification successful
Spectator mode state management is properly implemented
The spectator mode state is managed through a global UI store (useUIStore) rather than component-level useState, which ensures state persistence across component unmounts. The implementation:
- Centralizes state management in
useUIStore - Consistently uses the same state across different components
- Properly handles spectator mode in entity-related operations by using a zero address
- Has appropriate UI feedback in the navigation component
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all spectator mode state usages and updates
rg "setSpectatorMode|isSpectatorMode" --type typescript
Length of output: 92
Script:
#!/bin/bash
# Search for all spectator mode state usages and updates without file type restriction
rg "setSpectatorMode|isSpectatorMode"
# Also search for the state declaration
rg "spectatorMode.*useState"
Length of output: 1814
sdk/packages/eternum/src/provider/index.ts (2)
481-496: Verify parameter order in create_realm method
In the create_realm method, ensure that the calldata array [owner, realm_id, frontend] matches the expected parameter order for the create entrypoint in the realm_systems contract. Confirm that the contract expects parameters in this sequence to prevent potential transaction failures.
575-585: Confirm parameter alignment in create_multiple_realms method
In create_multiple_realms, the calldata includes [owner, realm_id, frontend] for each realm. Verify that this aligns with the create entrypoint's expected parameters in the realm_systems contract and that all provided realm IDs correspond correctly with the specified owner and frontend.
client/src/ui/components/cityview/realm/SettleRealmComponentDev.tsx (1)
18-18: Verify the logic for calculating numberRealms
The numberRealms variable is calculated as Math.max(mintedRealms, playerRealms().length). Please verify that this correctly represents the number of realms the player has settled. If mintedRealms can be less than playerRealms().length, consider adjusting the logic to accurately reflect the player's settled realms.
client/src/dojo/createSystemCalls.ts (3)
118-121: Integration of create_realm_dev method looks correct
The create_realm_dev function is properly defined and invokes provider.create_realm_dev(props) as expected. The integration aligns with the existing pattern for system calls.
126-129: Integration of create_multiple_realms_dev method looks correct
The create_multiple_realms_dev function correctly calls provider.create_multiple_realms_dev(props). The addition maintains consistency with other system call implementations.
354-358: New system calls are properly added to systemCalls object
The methods create_realm_dev and create_multiple_realms_dev are appropriately wrapped with queueing and error handling and added to the systemCalls object, ensuring they are accessible and reliable within the application.
sdk/packages/eternum/src/types/provider.ts (3)
168-170: Addition of owner and frontend properties to CreateMultipleRealmsProps
The CreateMultipleRealmsProps interface now includes owner and frontend properties. Ensure these properties are required by the corresponding system calls and are correctly utilized in the provider methods.
173-176: Definition of new interfaces for development realm creation
The new interfaces CreateMultipleRealmsDevProps and CreateRealmDevProps are properly defined with the necessary properties for development environment realm creation functions.
180-182: Inclusion of owner and frontend in CreateRealmProps
The CreateRealmProps interface now includes owner and frontend properties. Verify that these additions are correctly typed and align with the requirements of the create_realm method in the provider.
client/src/ui/layouts/Onboarding.tsx (2)
2-3: Verify environment configuration across different deployment environments
The onboarding flow now depends on the VITE_PUBLIC_DEV environment variable. Ensure this variable is properly configured across all deployment environments.
✅ Verification successful
Based on the shell script results, I can now generate the final response:
Environment configuration is properly handled across deployment environments
The VITE_PUBLIC_DEV environment variable is:
- Properly validated in both client and landing apps using zod schema
- Used consistently for feature flagging development-only steps in the onboarding flow
- Has appropriate fallbacks and error handling in deployment scripts
- Correctly configured in the documentation with example values
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for environment variable usage and configuration
# Look for .env files and environment variable definitions
fd -t f "^\.env" -x cat {} \;
rg "VITE_PUBLIC_DEV" -A 3
Length of output: 8985
6-6: Verify data dependencies for StepThree
The initial step is now set to 3 in production, skipping steps 1 and 2. Please confirm that StepThree doesn't depend on any data or state that would have been set in the skipped steps.
✅ Verification successful
Skipping steps 1 and 2 in production is safe
Based on the code analysis:
- StepThree is completely independent, only requiring an
onNextcallback prop - It manages its own internal state (
settledRealmId) and usesplayerRealmsfromuseEntities - Steps 1 (spectator mode setup) and 2 (naming) are development-only features that don't share state with StepThree
- The state transitions between steps are handled by simple increment/decrement of
currentStep
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential data dependencies in StepThree
# Look for prop types and state usage in StepThree
ast-grep --pattern 'const StepThree = ({ $$ }) => {
$$$
}'
# Look for state management related to these steps
rg -A 5 "StepThree|StepOne|Naming" --type typescript
Length of output: 152
Script:
#!/bin/bash
# Search for step components and their dependencies
rg -l "StepThree|StepOne|StepTwo" --type ts --type tsx
# Once we find the files, let's examine their content
rg "const Step(One|Two|Three)" -A 10 --type ts --type tsx
# Look for any state sharing or data flow between steps
rg "(useState|useContext|props)" -A 5 --type ts --type tsx client/src/ui/layouts/Onboarding.tsx
Length of output: 298
Script:
#!/bin/bash
# Search for step components and their dependencies without file type flags
rg -l "StepThree|StepOne|StepTwo"
# Once we find the files, let's examine their content
rg "const Step(One|Two|Three)" -A 10
# Look for any state sharing or data flow between steps in the Onboarding component
cat client/src/ui/layouts/Onboarding.tsx
Length of output: 3294
| const settleRealms = async (realmIds: number[]) => { | ||
| await create_multiple_realms({ | ||
| realm_ids: realmIds, | ||
| owner: account.address, | ||
| frontend: "0x0", | ||
| signer: account, | ||
| }); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling in settleRealms function
The settleRealms function lacks error handling. If create_multiple_realms fails, it could lead to unhandled promise rejections and a poor user experience. It's important to handle potential errors to provide feedback to the user and enhance reliability.
Apply this diff to include error handling:
const settleRealms = async (realmIds: number[]) => {
+ try {
await create_multiple_realms({
realm_ids: realmIds,
owner: account.address,
frontend: "0x0",
signer: account,
});
+ // Optionally, provide success feedback or perform additional actions
+ } catch (error) {
+ console.error('Error settling realms:', error);
+ // Optionally, display an error message to the user
+ }
};📝 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 settleRealms = async (realmIds: number[]) => { | |
| await create_multiple_realms({ | |
| realm_ids: realmIds, | |
| owner: account.address, | |
| frontend: "0x0", | |
| signer: account, | |
| }); | |
| }; | |
| const settleRealms = async (realmIds: number[]) => { | |
| try { | |
| await create_multiple_realms({ | |
| realm_ids: realmIds, | |
| owner: account.address, | |
| frontend: "0x0", | |
| signer: account, | |
| }); | |
| // Optionally, provide success feedback or perform additional actions | |
| } catch (error) { | |
| console.error('Error settling realms:', error); | |
| // Optionally, display an error message to the user | |
| } | |
| }; |
| const seasonPassElements = useMemo(() => { | ||
| let realms = []; | ||
| for (let i = 0; i < seasonPassRealms.length; i += 2) { | ||
| realms.push( | ||
| <div className={`grid grid-rows-2 gap-4 mr-4`}> | ||
| <SeasonPassRealm | ||
| seasonPassRealm={seasonPassRealms[i]} | ||
| selected={selectedRealms.includes(seasonPassRealms[i].realmId)} | ||
| setSelected={(selected) => | ||
| setSelectedRealms( | ||
| selected | ||
| ? [...selectedRealms, seasonPassRealms[i].realmId] | ||
| : selectedRealms.filter((id) => id !== seasonPassRealms[i].realmId), | ||
| ) | ||
| } | ||
| className={`row-start-1`} | ||
| /> | ||
| {seasonPassRealms[i + 1] && ( | ||
| <SeasonPassRealm | ||
| seasonPassRealm={seasonPassRealms[i + 1]} | ||
| selected={selectedRealms.includes(seasonPassRealms[i + 1].realmId)} | ||
| setSelected={(selected) => | ||
| setSelectedRealms( | ||
| selected | ||
| ? [...selectedRealms, seasonPassRealms[i + 1].realmId] | ||
| : selectedRealms.filter((id) => id !== seasonPassRealms[i + 1].realmId), | ||
| ) | ||
| } | ||
| className={`row-start-2`} | ||
| /> | ||
| )} | ||
| </div>, | ||
| ); | ||
| } | ||
| return realms; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add key prop to list elements when rendering seasonPassElements
When rendering lists in React, each element should have a unique key prop to help React identify which items have changed, are added, or are removed. The div elements pushed into the realms array lack a key prop, which could lead to rendering issues.
Apply this diff to add a key prop to the div elements:
for (let i = 0; i < seasonPassRealms.length; i += 2) {
realms.push(
+ <div key={seasonPassRealms[i].realmId} className={`grid grid-rows-2 gap-4 mr-4`}>
// existing content
</div>,
);
}If seasonPassRealms[i].realmId is not unique, consider using a combination of i and realmId or another unique identifier.
📝 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 seasonPassElements = useMemo(() => { | |
| let realms = []; | |
| for (let i = 0; i < seasonPassRealms.length; i += 2) { | |
| realms.push( | |
| <div className={`grid grid-rows-2 gap-4 mr-4`}> | |
| <SeasonPassRealm | |
| seasonPassRealm={seasonPassRealms[i]} | |
| selected={selectedRealms.includes(seasonPassRealms[i].realmId)} | |
| setSelected={(selected) => | |
| setSelectedRealms( | |
| selected | |
| ? [...selectedRealms, seasonPassRealms[i].realmId] | |
| : selectedRealms.filter((id) => id !== seasonPassRealms[i].realmId), | |
| ) | |
| } | |
| className={`row-start-1`} | |
| /> | |
| {seasonPassRealms[i + 1] && ( | |
| <SeasonPassRealm | |
| seasonPassRealm={seasonPassRealms[i + 1]} | |
| selected={selectedRealms.includes(seasonPassRealms[i + 1].realmId)} | |
| setSelected={(selected) => | |
| setSelectedRealms( | |
| selected | |
| ? [...selectedRealms, seasonPassRealms[i + 1].realmId] | |
| : selectedRealms.filter((id) => id !== seasonPassRealms[i + 1].realmId), | |
| ) | |
| } | |
| className={`row-start-2`} | |
| /> | |
| )} | |
| </div>, | |
| ); | |
| } | |
| return realms; | |
| const seasonPassElements = useMemo(() => { | |
| let realms = []; | |
| for (let i = 0; i < seasonPassRealms.length; i += 2) { | |
| realms.push( | |
| <div key={seasonPassRealms[i].realmId} className={`grid grid-rows-2 gap-4 mr-4`}> | |
| <SeasonPassRealm | |
| seasonPassRealm={seasonPassRealms[i]} | |
| selected={selectedRealms.includes(seasonPassRealms[i].realmId)} | |
| setSelected={(selected) => | |
| setSelectedRealms( | |
| selected | |
| ? [...selectedRealms, seasonPassRealms[i].realmId] | |
| : selectedRealms.filter((id) => id !== seasonPassRealms[i].realmId), | |
| ) | |
| } | |
| className={`row-start-1`} | |
| /> | |
| {seasonPassRealms[i + 1] && ( | |
| <SeasonPassRealm | |
| seasonPassRealm={seasonPassRealms[i + 1]} | |
| selected={selectedRealms.includes(seasonPassRealms[i + 1].realmId)} | |
| setSelected={(selected) => | |
| setSelectedRealms( | |
| selected | |
| ? [...selectedRealms, seasonPassRealms[i + 1].realmId] | |
| : selectedRealms.filter((id) => id !== seasonPassRealms[i + 1].realmId), | |
| ) | |
| } | |
| className={`row-start-2`} | |
| /> | |
| )} | |
| </div>, | |
| ); | |
| } | |
| return realms; |
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (16)
client/src/ui/layouts/OnboardingButton.tsx (2)
4-14: Add JSDoc documentation for the component and its props.Consider adding documentation to improve component maintainability and reusability.
+/** + * A specialized button component used in the onboarding flow. + * @param children - The content to be displayed inside the button + * @param className - Additional CSS classes to be applied + * @param onClick - Click handler function + * @param disabled - Whether the button is disabled + */ export const OnboardingButton = ({
16-24: Consider extracting hardcoded styles and making size configurable.The component has hardcoded styles and a fixed size which might limit its reusability.
Consider:
- Moving the styles to a CSS module or Tailwind class
- Making the size prop configurable
<Button - size="md" + size={size} variant="outline" - className={`!text-sm w-32 border border-gold hover:border-gold/50 h-12 hover:scale-105 hover:-translate-y-1 font-normal !normal-case ${className}`} + className={clsx(styles.onboardingButton, className)} onClick={onClick} disabled={disabled} >client/src/ui/elements/Button.tsx (2)
23-23: Inconsistent use of Tailwind's important modifier (!)The styles mix regular and important-flagged classes which could lead to specificity issues.
Consider standardizing the use of important flags:
- disabledStyle: "!bg-gray-300 cursor-not-allowed !border-gray-gold !text-gray-gold", + disabledStyle: "bg-gray-300 cursor-not-allowed border-gray-gold text-gray-gold", - outline: "border border-gold text-gold bg-transparent hover:bg-gold/10", + outline: "border border-gold text-gold bg-transparent hover:bg-gold/10", - danger: "bg-red border-red text-gold bg-transparent hover:bg-red/90 ", + danger: "bg-red border-red text-gold bg-transparent hover:bg-red/90", - secondary: "border border-orange text-orange bg-transparent hover:bg-orange/10", + secondary: "border border-orange text-orange bg-transparent hover:bg-orange/10",Also applies to: 26-28
62-64: Simplify className concatenation using clsx or classnames libraryThe current string concatenation for className is complex and hard to maintain.
Consider using a utility library:
+import clsx from 'clsx'; - className={` ${STYLES.baseStyle} ${STYLES[variant]} ${isLoading ? STYLES.loadingStyle : ""} ${ - isPulsing ? "animate-pulse" : "" - } ${SIZES[size]} ${disabled ? STYLES.disabledStyle : ""} ${className}`} + className={clsx( + STYLES.baseStyle, + STYLES[variant], + isLoading && STYLES.loadingStyle, + isPulsing && 'animate-pulse', + SIZES[size], + disabled && STYLES.disabledStyle, + className + )}sdk/packages/eternum/src/provider/index.ts (3)
Line range hint
447-463: Document the hardcoded value and add usage warningThe method contains a hardcoded value in the calldata that lacks documentation explaining its purpose:
"0x1a3e37c77be7de91a9177c6b57956faa6da25607e567b10a25cf64fea5e533b"Also, since this is a development method, consider adding a warning in the JSDoc comments about its intended use.
464-480: Update JSDoc example to match new parametersThe example in the documentation doesn't reflect the current method signature. It should include the new
ownerandfrontendparameters.Update the example to:
/** * @example * ```typescript * // Create realm with ID 123 * { * owner: "0x123...", * realm_id: 123, * frontend: "some_frontend", * signer: account * } * ``` */
Line range hint
542-557: Document the hardcoded value and consider optimization
- The hardcoded value lacks documentation:
"0x46f957b7fe3335010607174edd5c4c3fae87b12c3760dc167ac738959d8c03b"
- The calldata construction could be optimized by using map instead of flatMap since you're not flattening any nested arrays:
- let calldata = realm_ids.flatMap((realm_id) => { - let calldata = [ + let calldata = realm_ids.map((realm_id) => { contractAddress: getContractByName(this.manifest, `${NAMESPACE}-dev_realm_systems`), entrypoint: "create", calldata: [realm_id, "0x46f957b7fe3335010607174edd5c4c3fae87b12c3760dc167ac738959d8c03b"], - }, - ]; - return calldata; - }); + } + );client/src/ui/layouts/Onboarding.tsx (2)
23-33: Consider adding loading state and error handling for external linksThe mint season pass link could benefit from additional user feedback mechanisms.
<a className="text-gold cursor-pointer" + onClick={(e) => { + if (isLoading) { + e.preventDefault(); + } + }} href={`${ env.VITE_PUBLIC_DEV ? "https://empire-next.realms.world/season-passes" : "https://empire.realms.world/season-passes" }`} target="_blank" rel="noopener noreferrer" > - Mint Season Pass + {isLoading ? "Loading..." : "Mint Season Pass"} </a>
138-146: Consider using React Query for data fetchingThe current implementation of season pass data fetching could benefit from React Query's caching and revalidation capabilities.
+import { useQuery } from '@tanstack/react-query'; - const [seasonPassRealms, setSeasonPassRealms] = useState<SeasonPassRealm[]>([]); + const { data: seasonPassRealms = [], isLoading, error } = useQuery({ + queryKey: ['seasonPasses', account.address, realms], + queryFn: () => getUnusedSeasonPasses(account.address, realms), + enabled: !!account.address && !!realms + }); - useEffect(() => { - getUnusedSeasonPasses(account.address, realms).then((unsettledSeasonPassRealms) => { - setSeasonPassRealms(unsettledSeasonPassRealms); - }); - }, [realms]);client/src/ui/modules/onboarding/Steps.tsx (1)
Line range hint
32-39: Consider using useEffect for state synchronizationThe current implementation uses setTimeout for state updates which could lead to race conditions. Consider using useEffect to handle state changes and event dispatching in a more controlled manner.
- setTimeout(() => { - showBlankOverlay(false); - handleUrlChange(new Position({ x: 0, y: 0 }).toMapLocationUrl()); - window.dispatchEvent(new Event(ACCOUNT_CHANGE_EVENT)); - }, 250); + useEffect(() => { + if (setSpectatorMode) { + const timer = setTimeout(() => { + showBlankOverlay(false); + handleUrlChange(new Position({ x: 0, y: 0 }).toMapLocationUrl()); + window.dispatchEvent(new Event(ACCOUNT_CHANGE_EVENT)); + }, 250); + return () => clearTimeout(timer); + } + }, [setSpectatorMode]);client/src/ui/components/cityview/realm/SettleRealmComponent.tsx (2)
196-246: Remove commented mock dataThe file contains a large block of commented-out mock data that should be removed to improve code maintainability.
168-174: Enhance accessibility for interactive elementsThe div element is being used as an interactive element but lacks proper accessibility attributes.
<div key={seasonPassRealm.realmId} + role="button" + tabIndex={0} + onKeyPress={(e) => e.key === 'Enter' && setSelected(!selected)} className={`flex flex-col gap-2 p-2 rounded-md bg-gold/10 transition-colors duration-200 border border-2 ${ selected ? "border-gold bg-gold/30" : "border-transparent" } ${className} hover:border-gold`} onClick={() => setSelected(!selected)} >client/src/hooks/context/DojoContext.tsx (1)
Line range hint
152-197: Improve error handling in spectator mode implementation.The spectator mode implementation could benefit from proper error handling and state management.
Consider this safer implementation:
-const onSpectatorModeClick = () => { +const onSpectatorModeClick = () => { + try { setSpectatorMode(true); handleUrlChange(new Position({ x: 0, y: 0 }).toMapLocationUrl()); window.dispatchEvent(new Event(ACCOUNT_CHANGE_EVENT)); showBlankOverlay(false); + } catch (error) { + console.error('Failed to enter spectator mode:', error); + setSpectatorMode(false); // Reset state on error + } };client/src/hooks/helpers/useRealm.tsx (1)
294-340: Consider memoizing filter predicate and adding error boundary.The usePlayerRealms hook implementation could be optimized for performance and error handling.
Consider these improvements:
export function usePlayerRealms(): RealmInfo[] { const { account: { account }, setup: { components: { Realm, Position, Owner, Population, AddressName, Structure }, }, } = useDojo(); + // Memoize the filter predicate + const filterPredicate = useMemo(() => + HasValue(Owner, { address: ContractAddress(account.address) }), + [account.address] + ); - const realmEntities = useEntityQuery([Has(Realm), HasValue(Owner, { address: ContractAddress(account.address) })]); + const realmEntities = useEntityQuery([Has(Realm), filterPredicate]); const realms = useMemo((): RealmInfo[] => { + try { return Array.from(realmEntities) .map((entity) => { // ... existing mapping logic }) .filter((realm) => realm !== undefined); + } catch (error) { + console.error('Failed to process realm entities:', error); + return []; + } }, [realmEntities]); return realms; }client/src/ui/modules/controller/Controller.tsx (2)
10-17: Add TypeScript interface for component propsConsider adding proper TypeScript types for better maintainability and type safety.
+interface ControllerProps { + className?: string; + iconClassName?: string; +} + -export const Controller = ({ className, iconClassName }: { className?: string; iconClassName?: string }) => { +export const Controller = ({ className, iconClassName }: ControllerProps) => { - const [userName, setUserName] = useState<string>(); + const [userName, setUserName] = useState<string | undefined>();
50-77: Extract common styles and improve button reusabilityThe buttons share many common styles that could be extracted to reduce duplication and improve maintainability.
Consider creating a constant for common button styles:
const COMMON_BUTTON_STYLES = 'flex items-center hover:scale-105 hover:-translate-y-1 shadow-[0px_4px_4px_0px_#00000040] rounded-md !h-8 border !border-[#F5C2971F] backdrop-blur-xs'; // Then in JSX: className={`${COMMON_BUTTON_STYLES} !w-28 !text-gold !text-md !px-3 normal-case font-normal !bg-white/5 ${className}`}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (10)
client/src/assets/icons/back.svgis excluded by!**/*.svgclient/src/assets/icons/checkbox-checked.svgis excluded by!**/*.svgclient/src/assets/icons/checkbox-minus.svgis excluded by!**/*.svgclient/src/assets/icons/checkbox-unchecked.svgis excluded by!**/*.svgclient/src/assets/icons/disconnect.svgis excluded by!**/*.svgclient/src/assets/icons/eye.svgis excluded by!**/*.svgclient/src/assets/icons/lock.svgis excluded by!**/*.svgclient/src/assets/icons/sword.svgis excluded by!**/*.svgclient/src/assets/icons/treasure-chest.svgis excluded by!**/*.svgpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (19)
client/.env.production(1 hunks)client/env.ts(1 hunks)client/eslint.config.js(1 hunks)client/package.json(3 hunks)client/src/hooks/context/DojoContext.tsx(7 hunks)client/src/hooks/context/accountStore.tsx(1 hunks)client/src/hooks/helpers/useRealm.tsx(3 hunks)client/src/ui/components/cityview/realm/SettleRealmComponent.tsx(1 hunks)client/src/ui/elements/Button.tsx(2 hunks)client/src/ui/layouts/Onboarding.tsx(1 hunks)client/src/ui/layouts/OnboardingButton.tsx(1 hunks)client/src/ui/modules/controller/Controller.tsx(1 hunks)client/src/ui/modules/navigation/SecondaryMenuItems.tsx(7 hunks)client/src/ui/modules/onboarding/Steps.tsx(3 hunks)client/src/ui/modules/onboarding/StepsDevMode.tsx(1 hunks)client/src/ui/utils/realms.tsx(2 hunks)sdk/packages/eternum/src/provider/index.ts(4 hunks)sdk/packages/eternum/src/types/common.ts(1 hunks)sdk/packages/eternum/src/types/provider.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- client/src/ui/modules/navigation/SecondaryMenuItems.tsx
- sdk/packages/eternum/src/types/provider.ts
🧰 Additional context used
🪛 Biome (1.9.4)
client/src/ui/modules/onboarding/StepsDevMode.tsx
[error] 149-149: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
🔇 Additional comments (14)
sdk/packages/eternum/src/provider/index.ts (1)
575-593: LGTM! Well-structured implementation
The method is well implemented with:
- Clear separation of approval and creation calls
- Good variable naming
- Proper error handling
client/src/hooks/context/accountStore.tsx (1)
7-7: Allowing setAccount to accept null improves account state management
Updating the setAccount method to accept null aligns with the updated account state type, enabling proper handling when the account is unset or cleared.
client/eslint.config.js (1)
1-28:
Verify ESLint configuration and imports
There may be issues with how typescript-eslint is imported and used in your ESLint configuration. Ensure that the correct packages and configurations are applied.
Run the following script to verify the ESLint configuration:
Consider updating your ESLint configuration as follows:
module.exports = {
parser: '@typescript-eslint/parser',
parserOptions: {
ecmaVersion: 2020,
sourceType: 'module',
},
extends: [
'eslint:recommended',
'plugin:@typescript-eslint/recommended',
],
plugins: ['@typescript-eslint', 'react-hooks', 'react-refresh'],
rules: {
// your rules here
},
ignorePatterns: ['dist/', '**/src/assets/**'],
};client/.env.production (1)
29-29: Verify VITE_PUBLIC_CLIENT_FEE_RECIPIENT address
The value 0x0 for VITE_PUBLIC_CLIENT_FEE_RECIPIENT may not be a valid recipient address. Please ensure that this environment variable is set to the intended address to avoid issues in production.
client/src/ui/utils/realms.tsx (1)
55-56: Verify RealmInterface type definition includes imageUrl property
The addition of imageUrl to the return value looks correct, but we should ensure the RealmInterface type definition is updated accordingly.
Also applies to: 68-68
✅ Verification successful
RealmInterface type definition correctly includes imageUrl property
The RealmInterface type definition in sdk/packages/eternum/src/types/common.ts already includes the imageUrl property of type string at line 184, so no updates are needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if RealmInterface includes imageUrl property
ast-grep --pattern 'interface RealmInterface {
$$$
imageUrl: $_
$$$
}'
Length of output: 992
client/package.json (2)
6-6: Verify impact of removing --host flag
The removal of --host 0.0.0.0 from the dev script might affect developers who need to access the development server from other devices on the network.
78-78: Ensure compatibility with updated dependencies
The dependency updates and additions look reasonable, but we should verify compatibility:
@vitejs/plugin-reactupdated to ^4.3.2- New ESLint plugins added
viteupdated to ^5.4.8
Also applies to: 88-89, 96-96
✅ Verification successful
Dependencies update is safe to proceed
The verification shows that:
- All Vite configurations are using standard plugin APIs and features compatible with Vite 5.x
@vitejs/plugin-reactis properly imported and used consistently across all configurations- No version-specific syntax or deprecated features are being used
- The plugin configurations are simple and don't use any advanced features that might break with the update
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any peer dependency warnings in the lockfile
rg "peerDependencies|requires" package-lock.json
# Look for any vite config files that might need updates
fd -e js -e ts -e mjs -g "vite.config.*"
Length of output: 221
Script:
#!/bin/bash
# Check the vite config files content to verify compatibility
cat landing/vite.config.ts client/vite.config.ts balancing/vite.config.ts
# Check for any React-specific configurations or plugins
ast-grep --pattern 'import $_ from "@vitejs/plugin-react"'
# Look for any potential version-specific features or syntax
rg -A 5 "defineConfig|plugins" landing/vite.config.ts client/vite.config.ts balancing/vite.config.ts
Length of output: 6315
sdk/packages/eternum/src/types/common.ts (1)
184-184: LGTM!
The addition of imageUrl to RealmInterface is well-typed and properly integrated.
client/src/hooks/context/DojoContext.tsx (3)
2-8: LGTM: New imports are properly organized.
The new imports are well-organized and necessary for the new functionality.
201-205: LGTM: Account selection logic is clear and well-structured.
The account selection logic properly handles different scenarios (dev environment, spectator mode, and normal mode).
275-293: LGTM: UI components follow consistent styling.
The UI implementation now uses consistent component styling with OnboardingContainer and OnboardingButton components.
client/src/hooks/helpers/useRealm.tsx (1)
20-20: LGTM: Interface export enhances code reusability.
Making the RealmInfo interface exportable improves code modularity.
client/src/ui/modules/controller/Controller.tsx (2)
1-9: LGTM! Clean and well-organized imports
The imports are properly structured and all dependencies are relevant for the component's functionality.
1-77: Verify integration with onboarding flow
As this component is part of a larger onboarding rework, ensure it properly integrates with the new onboarding flow and handles all necessary state transitions.
✅ Verification successful
Let me gather more information about the integration between the Controller and onboarding components.
Let me gather more specific information about the onboarding flow integration.
Based on the gathered information, I can now provide the final response:
Controller component properly integrates with onboarding flow
The Controller component correctly handles account state transitions and integrates well with the onboarding flow through:
- Proper account state management using
useAccountStore - Correct handling of connect/disconnect flows with appropriate UI state updates
- Integration with the OnboardingOverlay component in the layout
- Proper event handling for account changes
The implementation follows the established patterns and maintains consistency with the rest of the application's state management approach.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for references to Controller component and related onboarding logic
rg -l "Controller" --type tsx
rg -l "onboarding" --type tsx
# Look for potential event handling between Controller and onboarding
ast-grep --pattern 'ACCOUNT_CHANGE_EVENT'
Length of output: 1033
Script:
#!/bin/bash
# Search for Controller references with proper file type
rg -l "Controller" --type-add 'tsx:*.tsx' --type tsx
# Look for connections between Controller and onboarding
rg -A 5 "Controller" --type-add 'tsx:*.tsx' --type tsx
# Check for account state handling in onboarding
ast-grep --pattern 'useAccountStore'
# Check for UI state management related to onboarding
ast-grep --pattern 'useUIStore'
Length of output: 55745
Script:
#!/bin/bash
# Check onboarding steps implementation
rg -A 10 "OnboardingOverlay" --type-add 'tsx:*.tsx' --type tsx
# Look for account state changes in onboarding
rg -A 5 "setAccount" --type-add 'tsx:*.tsx' --type tsx
# Check for connections between Controller and account state
ast-grep --pattern 'const { connector, account, setAccount } = useAccountStore'
Length of output: 6374
client/src/ui/layouts/Onboarding.tsx
Outdated
| <p className="text-xs align-bottom my-auto"> | ||
| By continuing you are agreeing to Eternum's <div className="inline underline">Terms of Service</div> | ||
| </p> |
There was a problem hiding this comment.
Make Terms of Service link functional
The Terms of Service link appears to be non-functional.
<p className="text-xs align-bottom my-auto">
- By continuing you are agreeing to Eternum's <div className="inline underline">Terms of Service</div>
+ By continuing you are agreeing to Eternum's{" "}
+ <a
+ href="/terms"
+ target="_blank"
+ rel="noopener noreferrer"
+ className="inline underline cursor-pointer"
+ >
+ Terms of Service
+ </a>
</p>📝 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.
| <p className="text-xs align-bottom my-auto"> | |
| By continuing you are agreeing to Eternum's <div className="inline underline">Terms of Service</div> | |
| </p> | |
| <p className="text-xs align-bottom my-auto"> | |
| By continuing you are agreeing to Eternum's{" "} | |
| <a | |
| href="/terms" | |
| target="_blank" | |
| rel="noopener noreferrer" | |
| className="inline underline cursor-pointer" | |
| > | |
| Terms of Service | |
| </a> | |
| </p> |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
client/src/ui/layouts/Onboarding.tsx (1)
158-158: Move hardcoded URL to environment variablesThe market URL should be configured through environment variables for better maintainability across different environments.
+// Add to env.ts +VITE_MARKET_URL: string; - href={`https://market.realms.world/collection/0x057675b9c0bd62b096a2e15502a37b290fa766ead21c33eda42993e48a714b80`} + href={`${env.VITE_MARKET_URL}/collection/0x057675b9c0bd62b096a2e15502a37b290fa766ead21c33eda42993e48a714b80`}client/src/ui/components/cityview/realm/SettleRealmComponent.tsx (2)
154-192: Improve accessibility in SeasonPassRealm componentThe component needs proper keyboard accessibility and ARIA attributes.
<div key={seasonPassRealm.realmId} + role="button" + tabIndex={0} + aria-pressed={selected} + onKeyPress={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + setSelected(!selected); + } + }} className={`flex flex-col gap-2 p-2 rounded-md bg-gold/10 transition-colors duration-200 border border-2 ${ selected ? "border-gold bg-gold/30" : "border-transparent" } ${className} hover:border-gold`} onClick={() => setSelected(!selected)} >
194-219: Improve type safety in getUnusedSeasonPassesThe function uses type assertions and could benefit from stronger typing.
+interface TokenMetadata { + __typename: string; + contractAddress?: string; + tokenId: string; +} +interface TokenNode { + node: { + tokenMetadata: TokenMetadata; + }; +} export const getUnusedSeasonPasses = async (accountAddress: string, realms: RealmInfo[]) => { const balances = await querySeasonPasses(accountAddress); return balances?.tokenBalances?.edges ?.filter( - (token: { node: { tokenMetadata: { __typename: string; contractAddress?: string } } }) => + (token: TokenNode): token is TokenNode => token?.node?.tokenMetadata.__typename === "ERC721__Token" && addAddressPadding(token.node.tokenMetadata.contractAddress ?? "0x0") === addAddressPadding(env.VITE_SEASON_PASS_ADDRESS ?? "0x0"), )client/src/ui/modules/onboarding/Steps.tsx (1)
Line range hint
234-241: Remove non-null assertion operatorThe use of non-null assertion operator (!.) in the URL handling could lead to runtime errors.
- handleUrlChange(url!); + if (url) { + handleUrlChange(url); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
client/src/ui/components/cityview/realm/SettleRealmComponent.tsx(1 hunks)client/src/ui/layouts/Onboarding.tsx(1 hunks)client/src/ui/modules/onboarding/Steps.tsx(3 hunks)
🔇 Additional comments (4)
client/src/ui/layouts/Onboarding.tsx (3)
79-81: Make Terms of Service link functional
The Terms of Service link appears to be non-functional.
98-102: 🛠️ Refactor suggestion
Add error handling for background image loading
The background image loading lacks error handling and fallback mechanism.
<img
className="absolute h-screen w-screen object-cover"
+ onError={(e) => {
+ console.error('Failed to load background image:', backgroundImage);
+ e.currentTarget.src = '/images/covers/fallback.png';
+ }}
src={`/images/covers/${backgroundImage}.png`}
alt="Cover"
/>Likely invalid or redundant comment.
143-145: 🛠️ Refactor suggestion
Add error handling for getUnusedSeasonPasses
The season pass data fetching lacks error handling.
- getUnusedSeasonPasses(account.address, realms).then((unsettledSeasonPassRealms) => {
- setSeasonPassRealms(unsettledSeasonPassRealms);
- });
+ getUnusedSeasonPasses(account.address, realms)
+ .then((unsettledSeasonPassRealms) => {
+ setSeasonPassRealms(unsettledSeasonPassRealms);
+ })
+ .catch((error) => {
+ console.error('Failed to fetch season passes:', error);
+ setSeasonPassRealms([]);
+ });Likely invalid or redundant comment.
client/src/ui/components/cityview/realm/SettleRealmComponent.tsx (1)
29-43: 🛠️ Refactor suggestion
Improve error handling in settleRealms
The function needs better error handling and success case management.
const settleRealms = async (realmIds: number[]) => {
setLoading(true);
try {
const res = await create_multiple_realms({
realm_ids: realmIds,
owner: account.address,
frontend: env.VITE_CLIENT_FEE_RECIPIENT,
signer: account,
season_pass_address: env.VITE_SEASON_PASS_ADDRESS,
});
+ setSelectedRealms([]);
+ // Notify success
} catch (error) {
console.error("Error settling realms:", error);
+ // Notify error to user
} finally {
setLoading(false);
}
};Likely invalid or redundant comment.
| <OnboardingButton | ||
| disabled={realms.length <= 0} | ||
| className={`!bg-gold border-none ${ | ||
| realms.length <= 0 ? "opacity-40 hover:none disabled:pointer-events-none" : "" | ||
| }`} | ||
| onClick={onSpectatorModeClick} | ||
| > |
There was a problem hiding this comment.
Fix duplicate onClick handler
Both "Spectate" and "Play" buttons use the same onClick handler (onSpectatorModeClick), which might not be the intended behavior for the "Play" button.
| const seasonPassElement = seasonPassRealms[i]; | ||
| const seasonPassElement2 = seasonPassRealms[i + 1]; | ||
| elements.push( | ||
| <div className="grid grid-cols-2 gap-3"> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add key prop to mapped elements
The div elements in the seasonPassElements map operation lack key props, which could cause React rendering issues.
- <div className="grid grid-cols-2 gap-3">
+ <div key={seasonPassElement.realmId} className="grid grid-cols-2 gap-3">Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (8)
client/src/ui/modules/navigation/SecondaryMenuItems.tsx (1)
66-67: Consider extracting common button propertiesMultiple CircleButton components share the same size and border styling. This repetition could be reduced by creating a styled component or extracting these properties into a common configuration.
Consider one of these approaches:
- Create a styled component:
const SecondaryCircleButton = styled(CircleButton)` border: none; `.withConfig({ shouldComponentUpdate: true, defaultProps: { size: 'sm' } });
- Or extract common props into a configuration:
const secondaryButtonProps = { size: 'sm' as const, className: 'border-none' }; // Usage <CircleButton {...secondaryButtonProps} label="Hints" onClick={() => toggleModal(<HintModal />)} />Also applies to: 91-92, 106-107, 123-126, 132-133, 140-141, 149-150
client/src/ui/layouts/Onboarding.tsx (1)
24-28: Consider extracting URL construction logicThe URL construction using environment variables could be moved to a constant or utility function for better maintainability.
+const SEASON_PASS_URL = env.VITE_PUBLIC_DEV + ? "https://empire-next.realms.world/season-passes" + : "https://empire.realms.world/season-passes"; <a className="text-gold cursor-pointer" - href={`${ - env.VITE_PUBLIC_DEV - ? "https://empire-next.realms.world/season-passes" - : "https://empire.realms.world/season-passes" - }`} + href={SEASON_PASS_URL} target="_blank" rel="noopener noreferrer" >client/src/hooks/context/DojoContext.tsx (3)
242-254: Extract retry configuration to constantsThe retry logic uses magic numbers. Consider extracting these to named constants.
+const MAX_RETRIES = 10; +const RETRY_DELAY = 100; -setTimeout(() => { - setRetries((prevRetries) => { - if (prevRetries < 10) { - return prevRetries + 1; - } else { - setAccountsInitialized(true); - return prevRetries; - } - }); -}, 100); +setTimeout(() => { + setRetries((prevRetries) => { + if (prevRetries < MAX_RETRIES) { + return prevRetries + 1; + } else { + setAccountsInitialized(true); + return prevRetries; + } + }); +}, RETRY_DELAY);
211-215: Remove or document commented codeThe commented-out code for setting address name should either be removed or documented with a TODO explaining why it's commented out.
203-203: Consider extracting hardcoded spectator account valuesThe spectator account uses hardcoded values ("0x0"). Consider extracting these to constants or configuration.
+const SPECTATOR_ADDRESS = "0x0"; +const SPECTATOR_PRIVATE_KEY = "0x0"; -new Account(value.network.provider.provider, "0x0", "0x0") +new Account(value.network.provider.provider, SPECTATOR_ADDRESS, SPECTATOR_PRIVATE_KEY)client/src/ui/modules/LoadingScreen.tsx (3)
7-24: Review the appropriateness of loading messagesWhile most loading messages are thematic and engaging, some might need reconsideration:
- "Cooking Donkeys" might be perceived as inappropriate or confusing to users
- Consider replacing it with something more aligned with the game's theme, like "Preparing Provisions" or "Stocking Supplies"
38-52: Consider optimizing the statement calculation logicThe timestamp and statement index calculation is duplicated in both the initial setup and the interval callback. Consider extracting this logic into a reusable function.
useEffect(() => { + const getStatementIndex = () => { + const timestamp = Math.floor(Date.now() / (1000 * 60)); + return timestamp % statements.length; + }; + - const timestamp = Math.floor(Date.now() / (1000 * 60)); - const statementIndex = timestamp % statements.length; - setCurrentStatement(statementIndex); + setCurrentStatement(getStatementIndex()); const dotInterval = setInterval(() => { setDots((prevDots) => (prevDots + 1) % 4); }, 1000); const statementInterval = setInterval(() => { - const newTimestamp = Math.floor(Date.now() / (1000 * 60)); - setCurrentStatement(newTimestamp % statements.length); + setCurrentStatement(getStatementIndex()); }, 60000); return () => { clearInterval(dotInterval); clearInterval(statementInterval); }; }, []);
61-64: Consider making transition and tos props configurableThe
transitionandtosprops are hardcoded tofalse. Consider making these configurable through component props if there are cases where different values might be needed.-export const LoadingScreen = ({ backgroundImage }: { backgroundImage: string }) => { +interface LoadingScreenProps { + backgroundImage: string; + showTransition?: boolean; + showTos?: boolean; +} + +export const LoadingScreen = ({ + backgroundImage, + showTransition = false, + showTos = false, +}: LoadingScreenProps) => { // ... rest of the component return ( <OnboardingContainer backgroundImage={backgroundImage} controller={false}> - <StepContainer tos={false} transition={false}> + <StepContainer tos={showTos} transition={showTransition}>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
client/src/hooks/context/DojoContext.tsx(7 hunks)client/src/ui/layouts/Onboarding.tsx(1 hunks)client/src/ui/modules/LoadingScreen.tsx(2 hunks)client/src/ui/modules/navigation/SecondaryMenuItems.tsx(7 hunks)client/src/ui/modules/onboarding/Steps.tsx(2 hunks)
🔇 Additional comments (11)
client/src/ui/modules/navigation/SecondaryMenuItems.tsx (3)
8-8: LGTM: Import follows project conventions
The HintModal import is correctly placed and follows the project's path aliasing conventions.
22-22: LGTM: Improved destructuring syntax
The updated destructuring pattern is more explicit and follows modern JavaScript conventions.
131-134: LGTM: Modal integration follows existing patterns
The HintModal integration is consistent with the codebase's modal handling patterns.
client/src/ui/layouts/Onboarding.tsx (3)
87-87: Make Terms of Service link functional
The Terms of Service link appears to be non-functional.
110-112: Add error handling for background image loading
The background image loading lacks error handling and loading states.
153-155: Add error handling for getUnusedSeasonPasses
The season pass data fetching lacks error handling.
client/src/ui/modules/onboarding/Steps.tsx (2)
120-120: Add key prop to mapped elements
The div elements in the seasonPassElements map operation lack key props.
97-99: 🛠️ Refactor suggestion
Add user feedback for settlement errors
The error handling exists but doesn't provide user feedback. Consider adding toast notifications.
} catch (error) {
console.error("Error settling realms:", error);
+ toast.error('Failed to settle realms. Please try again.');
setLoading(false);
}Likely invalid or redundant comment.
client/src/ui/modules/LoadingScreen.tsx (3)
28-28: LGTM!
Clean implementation of state for dots animation.
55-58: LGTM!
Clean implementation of the dots animation helper function.
38-52: Verify cleanup behavior during rapid unmounts
While the cleanup logic looks correct, it's important to verify that no memory leaks occur during rapid mounting/unmounting scenarios, especially with multiple intervals running.
✅ Verification successful
Cleanup implementation follows React best practices
The code review revealed that the interval cleanup pattern in LoadingScreen.tsx is consistent with other interval-based components in the codebase. All interval-based components, including TopLeftNavigation, Rewards, ResourceChip, BattleProgress, and others, properly clean up their intervals in the useEffect cleanup function. The implementation in LoadingScreen.tsx follows the same pattern:
- Intervals are created within useEffect
- Both intervals (dotInterval and statementInterval) are properly cleared in the cleanup function
- The cleanup pattern matches the established pattern across the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar interval-based components to verify consistent cleanup patterns
ast-grep --pattern 'useEffect(() => {
$$$
const $_ = setInterval($$$)
$$$
return () => {
clearInterval($_)
$$$
}
$$$
})'
Length of output: 142
Script:
#!/bin/bash
# Let's try a broader search for interval usage and cleanup patterns
rg "setInterval|clearInterval" -A 5 -B 5
Length of output: 13778
| setTimeout(() => { | ||
| showBlankOverlay(false); | ||
| handleUrlChange(new Position({ x: 0, y: 0 }).toMapLocationUrl()); | ||
| window.dispatchEvent(new Event(ACCOUNT_CHANGE_EVENT)); | ||
| }, 250); |
There was a problem hiding this comment.
Add cleanup for setTimeout
The setTimeout should be cleaned up to prevent memory leaks.
-setTimeout(() => {
+const timer = setTimeout(() => {
showBlankOverlay(false);
handleUrlChange(new Position({ x: 0, y: 0 }).toMapLocationUrl());
window.dispatchEvent(new Event(ACCOUNT_CHANGE_EVENT));
}, 250);
+return () => clearTimeout(timer);Committable suggestion skipped: line range outside the PR's diff.
…w-event-discord-and-onboarding
Summary by CodeRabbit
Release Notes
New Features
GameEndedevent for better game state handling in Discord.SettleRealmComponentDevfor improved realm settlement interactions.OnboardingButtoncomponent for streamlined onboarding actions.imageUrlproperty added to realms for better visual representation.Bug Fixes
Style
Documentation