-
Notifications
You must be signed in to change notification settings - Fork 37
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Add user battles tab #2102
base: next
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe pull request introduces several enhancements across multiple components and hooks in the application. A new hook, Changes
Assessment against linked issues
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 (
|
Failed to generate code suggestions for PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR adds a user battles tab and improves the battle UI components. The changes look generally good but there are some opportunities for optimization:
- Several components could benefit from better memoization of callbacks and derived values
- Some type definitions could be more specific
- The UI layout uses hardcoded widths which could be made more flexible
- Some complex logic could be extracted into custom hooks
Overall the changes improve the battle functionality and UI organization, but could use some minor optimizations for better performance and maintainability.
Thanks for using MentatBot. Give comments a 👍 or 👎 to help me improve!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (20)
client/src/ui/components/military/UserBattles.tsx (2)
1-3
: Consider using absolute imports consistently.While the first import uses an absolute path (
@/
), the second one uses a relative path. Consider using absolute imports consistently for better maintainability.-import { BattleListItem } from "../battles/BattleListItem"; +import { BattleListItem } from "@/ui/components/battles/BattleListItem";
8-8
: Make the width responsive.The hardcoded width might not work well on different screen sizes. Consider using responsive width classes.
- <div className="w-[31rem] py-2 pl-2"> + <div className="w-full md:w-[31rem] py-2 pl-2">client/src/ui/elements/Checkbox.tsx (1)
3-7
: Consider using a more semantic name for the onClick prop.While the implementation is correct, consider renaming
onClick
to something more specific likeonToggle
oronStateChange
to better reflect its purpose in a checkbox context.type CheckboxProps = { enabled: boolean; - onClick?: () => void; + onToggle?: () => void; text?: string; };client/src/ui/modules/military/Military.tsx (2)
17-27
: Consider memoizing the tabs configuration.The tabs array is recreated on every render. Consider using useMemo to optimize performance.
+const tabs = useMemo(() => [ -const tabs = [ { label: "Army", component: isMapView ? ( <EntitiesArmyTable /> ) : ( selectedStructure && <EntityArmyList structure={selectedStructure} /> ), }, { label: "Battles", component: <UserBattles /> }, -]; +], [isMapView, selectedStructure]);
31-49
: Enhance accessibility with explicit ARIA labels.While the tabs implementation is solid, consider adding explicit ARIA labels for better accessibility.
<Tabs selectedIndex={selectedTab} onChange={(index: number) => { setSelectedTab(index); }} - className="h-full" + className="h-full" + aria-label="Military Information" >client/src/ui/modules/entity-details/Entities.tsx (3)
11-19
: Add JSDoc documentation for the component propsConsider adding JSDoc documentation to describe the purpose of each prop, especially the new
battles
prop, to improve code maintainability.+/** + * Entities component displays various entity details including structures, battles, and armies + * @param position - The position coordinates on the map + * @param ownArmy - The player's army information if present + * @param battles - List of battles occurring at the given position + */ export const Entities = ({ position, ownArmy, battles, }: {
20-22
: Consider persisting user preferences for section visibilityThe visibility states are reset to
true
on every component mount. Consider storing these preferences in localStorage or user settings to maintain consistency across sessions.+const STORAGE_KEY = 'entityDetails.visibility'; + +const getStoredVisibility = () => { + try { + return JSON.parse(localStorage.getItem(STORAGE_KEY) || '{}'); + } catch { + return {}; + } +}; + const [showStructure, setShowStructure] = useState(true); const [showBattles, setShowBattles] = useState(true); const [showArmies, setShowArmies] = useState(true); + +useEffect(() => { + const stored = getStoredVisibility(); + if (stored.showStructure !== undefined) setShowStructure(stored.showStructure); + if (stored.showBattles !== undefined) setShowBattles(stored.showBattles); + if (stored.showArmies !== undefined) setShowArmies(stored.showArmies); +}, []);
31-36
: Consider improving responsive design for the checkbox gridWhile the grid layout works well, using
grid-cols-3
might cause issues on smaller screens. Consider making it responsive.-<div className="w-full grid grid-cols-3"> +<div className="w-full grid grid-cols-1 sm:grid-cols-2 md:grid-cols-3 gap-2">scripts/indexer.sh (1)
Line range hint
1-91
: Consider enhancing script reliability and observability.The script handles critical deployment and indexing setup, but could benefit from some architectural improvements:
- Add error checking after critical operations (contract deployment, file operations)
- Validate generated contract addresses before updating configuration
- Add logging to track important state changes
- Consider using environment variables for configurable values
Example improvements:
#!/bin/bash + +# Exit on error +set -e + +# Logging function +log_info() { + echo "[INFO] $(date '+%Y-%m-%d %H:%M:%S') - $1" +} + +# Error handling function +handle_error() { + echo "[ERROR] $(date '+%Y-%m-%d %H:%M:%S') - $1" >&2 + exit 1 +} + +# Validate contract address +validate_address() { + local addr=$1 + if [[ ! $addr =~ ^0x[a-fA-F0-9]{64}$ ]]; then + handle_error "Invalid contract address format: $addr" + fi +}Consider extracting configurable values to environment variables:
-torii --world 0x05013b17c43a2b664ec2a38aa45f6d891db1188622ec7cf320411321c3248fb5 \ +torii --world "${WORLD_ADDRESS}" \client/src/hooks/helpers/battles/useBattles.tsx (1)
91-107
: Consider performance optimizationsThe current implementation involves multiple array transformations and queries which could impact performance with large datasets.
Consider these optimizations:
export const useUserBattles = () => { const { setup: { components: { Army, Battle, EntityOwner, Position, Structure }, }, } = useDojo(); const { playerRealms } = useEntities(); const realms = playerRealms(); - const battleEntityIds = realms - .map((realm) => { - const userArmiesInBattleEntityIds = runQuery([ - Has(Army), - NotValue(Army, { battle_id: 0 }), - HasValue(EntityOwner, { entity_owner_id: realm.entity_id }), - ]); - const battleEntityIds = Array.from(userArmiesInBattleEntityIds) - .map((armyEntityId) => { - const army = getComponentValue(Army, armyEntityId); - if (!army) return; - return getEntityIdFromKeys([BigInt(army.battle_id)]); - }) - .filter((battleEntityId): battleEntityId is Entity => Boolean(battleEntityId)); - return battleEntityIds; - }) - .flatMap((battleEntityIds) => Array.from(battleEntityIds)); + // Use Set to avoid duplicate battle IDs + const battleEntityIds = new Set<Entity>(); + + for (const realm of realms) { + const userArmiesInBattle = runQuery([ + Has(Army), + NotValue(Army, { battle_id: 0 }), + HasValue(EntityOwner, { entity_owner_id: realm.entity_id }), + ]); + + for (const armyEntityId of userArmiesInBattle) { + const army = getComponentValue(Army, armyEntityId); + if (army) { + battleEntityIds.add(getEntityIdFromKeys([BigInt(army.battle_id)])); + } + } + } + + return getExtraBattleInformation(Array.from(battleEntityIds), Battle, Position, Structure);client/src/ui/modules/entity-details/EnemyArmies.tsx (2)
107-114
: Simplify fragment nesting and improve heading semanticsThe current implementation has unnecessary fragment nesting. Consider simplifying the structure and improving the heading's semantic meaning.
Here's a suggested improvement:
- <> - <h5 className="pl-2 ">Enemy armies</h5> - <React.Fragment> - <div className="grid grid-cols-1 gap-2 p-2"> - {armies.length > 0 && armies.map((army: ArmyInfo, index) => getArmyChip(army, index)).filter(Boolean)} - </div> - </React.Fragment> - </> + <section aria-label="Enemy armies"> + <h2 className="pl-2 text-sm font-semibold">Enemy armies</h2> + <div className="grid grid-cols-1 gap-2 p-2"> + {armies.length > 0 && armies.map((army: ArmyInfo, index) => getArmyChip(army, index)).filter(Boolean)} + </div> + </section>
111-111
: Simplify the army length check conditionThe current implementation has a redundant length check since the outer condition already ensures armies.length !== 0.
Consider simplifying:
- {armies.length > 0 && armies.map((army: ArmyInfo, index) => getArmyChip(army, index)).filter(Boolean)} + {armies.map((army: ArmyInfo, index) => getArmyChip(army, index)).filter(Boolean)}client/src/ui/modules/entity-details/CombatEntityDetails.tsx (1)
55-55
: Add validation for battles data.While passing battles data to the Entities component aligns with the PR objective to display battles in the military tab, consider adding validation to handle cases where battles data might be undefined or null.
- component: selectedHex && <Entities position={hexPosition} ownArmy={ownArmy} battles={battles} />, + component: selectedHex && <Entities position={hexPosition} ownArmy={ownArmy} battles={battles ?? []} />,client/src/ui/components/battles/BattleListItem.tsx (2)
Line range hint
81-85
: Add error handling for undefined battle stateWhile the simplified logic is clearer, consider adding error handling for cases where
updatedBattle
might be undefined in the click handlers.onClick={() => + if (!updatedBattle) return; setBattleView({ battleEntityId: updatedBattle!.entity_id, targetArmy: undefined, ownArmyEntityId: ownArmySelected!.entity_id, }) }
94-94
: Consider extracting battle content into a separate componentThe nested JSX structure is becoming complex. Consider extracting the battle content into a separate component for better maintainability.
// Suggested new component const BattleContent: React.FC<{ battle: BattleInfo; updatedBattle: Battle; showCompass: boolean; position: Position; }> = ({ battle, updatedBattle, showCompass, position }) => ( <div className="flex w-full justify-between"> {/* ... existing content ... */} </div> );Also, the
hideTooltip
prop is hardcoded totrue
forViewOnMapIcon
. Consider making this configurable if there are cases where the tooltip might be needed.Also applies to: 99-100
contracts/src/systems/map/map_generation.cairo (1)
Line range hint
171-199
: Consider refactoring for improved maintainability.The troop generation logic could benefit from some refactoring:
- Extract random troop count generation into a helper function to reduce code duplication
- Consider splitting army creation and troop assignment into separate functions for better separation of concerns
Here's a suggested refactoring approach:
fn generate_random_troop_count( salt: u256, lower_bound: u64, upper_bound: u64, ) -> u64 { random::random( salt.low, upper_bound.into() - lower_bound.into() ) .try_into() .unwrap() + lower_bound } fn generate_mercenary_troops( tx_hash: felt252, timestamp: u64, nonce: felt252, config: MercenariesConfig, ) -> Troops { let salt_one: u256 = tx_hash.into(); let salt_two: u256 = timestamp.into(); let salt_three: u256 = nonce.into(); let mut troops = Troops { knight_count: generate_random_troop_count( salt_one, config.knights_lower_bound, config.knights_upper_bound ), paladin_count: generate_random_troop_count( salt_two, config.paladins_lower_bound, config.paladins_upper_bound ), crossbowman_count: generate_random_troop_count( salt_three, config.crossbowmen_lower_bound, config.crossbowmen_upper_bound ) }; troops.normalize_counts(); troops }client/src/hooks/helpers/useEntities.tsx (1)
160-161
: Consider optimizing the sorting implementationThe current implementation performs filtering and sorting on every render. Consider maintaining a sorted state of
playerStructures
to avoid the double iteration.Here's a suggested optimization:
const playerStructures = useMemo(() => { return filterPlayerStructures .map((id) => { // ... existing mapping logic ... }) .filter((structure): structure is PlayerStructure => structure !== undefined) .sort((a, b) => { if (a.category === StructureType[StructureType.Realm]) return -1; if (b.category === StructureType[StructureType.Realm]) return 1; - return a.category.localeCompare(b.category); + // Sort by category first, then by name + const categoryCompare = a.category.localeCompare(b.category); + return categoryCompare !== 0 ? categoryCompare : a.name.localeCompare(b.name); }); }, [filterPlayerStructures]); const getPlayerStructures = (filterFn?: (structure: PlayerStructure) => boolean) => { return useMemo(() => { - const structures = filterFn ? playerStructures.filter(filterFn) : playerStructures; - return structures.sort((a, b) => a.name.localeCompare(b.name)); + return filterFn ? playerStructures.filter(filterFn) : playerStructures; }, [playerStructures, filterFn]); };contracts/src/systems/combat/contracts/troop_systems.cairo (2)
Line range hint
333-411
: Consider improving maintainability of military building calculations.The military building count calculation could be extracted into a helper function for better maintainability and reusability. Additionally, the stamina initialization logic is complex and could benefit from being moved to a dedicated function.
Consider refactoring like this:
- let total_military_building_count = stables_building_count - + archery_range_building_count - + barracks_building_count; + let total_military_building_count = calculate_total_military_buildings( + stables_building_count, + archery_range_building_count, + barracks_building_count + );And add this helper function:
fn calculate_total_military_buildings( stables: u128, archery: u128, barracks: u128 ) -> u128 { stables + archery + barracks }
Line range hint
438-461
: LGTM! Base army creation is thorough and secure.The implementation correctly validates ownership and initializes all required components. However, consider adding a comment explaining the significance of the default battle_id (0) and battle_side values for better maintainability.
Add a comment like this:
world .write_model( @Army { - entity_id: army_id, troops: Default::default(), battle_id: 0, battle_side: Default::default() + entity_id: army_id, + troops: Default::default(), + battle_id: 0, // 0 indicates army is not in battle + battle_side: Default::default() // Default side until army joins a battle } );contracts/manifest_dev.json (1)
Contract address format issue detected
Found one contract address that doesn't match the expected format:
"0x3a6bbb82f9e670c99647f3b0c4aaf1be82be712e1a393336b79b9dab44cc5"
While all class hashes are valid, this address appears to be truncated or malformed as it's shorter than the expected 62-64 hex characters after '0x'. This could cause issues with contract interactions.
🔗 Analysis chain
Line range hint
1246-4036
: Contract definitions are well-structured!The contracts follow best practices with:
- Clear separation of concerns (bank, battle, building systems, etc.)
- Consistent implementation of upgradeable pattern
- Standardized event handling
- Well-defined interfaces and systems
Verify the contract addresses are correctly configured:
The script results show no duplicate addresses or class hashes were found, which is good. However, let's verify the format and validity of the addresses and class hashes to ensure they follow the expected pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify contract addresses are valid and match class hashes # Check for any duplicate addresses echo "Checking for duplicate addresses..." grep -A 1 '"address":' contracts/manifest_dev.json | grep "0x" | sort | uniq -d # Check for any duplicate class hashes echo "Checking for duplicate class hashes..." grep -A 1 '"class_hash":' contracts/manifest_dev.json | grep "0x" | sort | uniq -dLength of output: 330
Script:
#!/bin/bash # Verify address and class hash format # Check address format (should be 0x followed by 62-64 hex chars) echo "Checking address format..." grep '"address":' contracts/manifest_dev.json | grep -v '0x[0-9a-f]\{62,64\}"' || echo "All addresses valid" # Check class hash format (should be 0x followed by 62-64 hex chars) echo "Checking class hash format..." grep '"class_hash":' contracts/manifest_dev.json | grep -v '0x[0-9a-f]\{62,64\}"' || echo "All class hashes valid" # Count total number of contracts echo "Total contracts:" grep '"address":' contracts/manifest_dev.json | wc -lLength of output: 537
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (15)
client/src/hooks/helpers/battles/useBattles.tsx
(2 hunks)client/src/hooks/helpers/useEntities.tsx
(1 hunks)client/src/ui/components/battles/BattleListItem.tsx
(3 hunks)client/src/ui/components/military/UserBattles.tsx
(1 hunks)client/src/ui/elements/Checkbox.tsx
(1 hunks)client/src/ui/modules/entity-details/Battles.tsx
(1 hunks)client/src/ui/modules/entity-details/CombatEntityDetails.tsx
(2 hunks)client/src/ui/modules/entity-details/EnemyArmies.tsx
(1 hunks)client/src/ui/modules/entity-details/Entities.tsx
(2 hunks)client/src/ui/modules/military/Military.tsx
(1 hunks)contracts/manifest_dev.json
(52 hunks)contracts/src/systems/combat/contracts/troop_systems.cairo
(4 hunks)contracts/src/systems/map/map_generation.cairo
(1 hunks)contracts/torii.toml
(1 hunks)scripts/indexer.sh
(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- client/src/ui/modules/entity-details/Battles.tsx
- contracts/torii.toml
🔇 Additional comments (21)
client/src/ui/modules/military/Military.tsx (2)
5-6
: LGTM: Import statements are appropriate for the new functionality.
The new imports for UserBattles and Tabs components, along with the useState hook, are correctly added to support the tabbed interface implementation.
Also applies to: 8-8
17-27
: Implementation successfully meets PR objectives.
The tabbed interface effectively addresses issue #2023 by:
- Adding a dedicated "Battles" tab for viewing user battles
- Maintaining existing army functionality in a separate tab
- Providing clear navigation between different views
Also applies to: 31-49
✅ Verification successful
The verification shows that the UserBattles component is properly implemented in client/src/ui/components/military/UserBattles.tsx
. Let me verify the hook it uses and the BattleListItem component to ensure complete implementation.
Implementation successfully meets PR objectives.
The verification confirms that the implementation is complete and properly structured:
- UserBattles component correctly renders the list of user battles
- useUserBattles hook properly fetches battle data using the Dojo framework
- BattleListItem component provides rich battle information display with proper interactions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the UserBattles component implementation
ast-grep --pattern 'export const UserBattles = $_'
Length of output: 1715
Script:
#!/bin/bash
# Check the useUserBattles hook implementation
ast-grep --pattern 'export const useUserBattles = $_'
# Check the BattleListItem component
ast-grep --pattern 'export const BattleListItem = $_'
Length of output: 14292
client/src/ui/modules/entity-details/Entities.tsx (3)
1-1
: LGTM: Import statements are well-organized
The new imports are properly structured and necessary for the battle tab functionality.
Also applies to: 6-8
37-41
: LGTM: Well-structured conditional rendering
The conditional rendering logic is clean and properly implemented. The enemy armies check prevents unnecessary rendering of empty arrays.
38-38
: Verify battle information display
The implementation adds battle visibility as required. Let's verify the integration with the battle system.
✅ Verification successful
Battle implementation is properly integrated
The verification shows proper battle system integration:
BattleInfo
type is correctly defined inuseBattles.tsx
with all necessary propertiesBattles
component inentity-details/Battles.tsx
correctly accepts and uses the required props (ownArmy
andbattles
)- Battle hooks (
useBattlesByPosition
,useUserBattles
) are available for fetching battle data - Component is properly integrated with the battle list item display system
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify proper battle integration and usage
# Check for proper battle prop passing in parent components
rg -l "Entities.*battles={" --type tsx
# Check for proper battle type usage
ast-grep --pattern 'type $_ = { battles: BattleInfo[] }'
Length of output: 201
Script:
#!/bin/bash
# Let's break down the verification into steps
# 1. Check how battles are passed to Entities component
rg "battles.*Entities" --type tsx -B 2 -A 2
# 2. Check the Battles component implementation
rg "export.*Battles" --type tsx -B 2 -A 2
# 3. Look for BattleInfo type definition
rg "type.*BattleInfo" --type ts --type tsx -B 1 -A 3
# 4. Check for battle-related props and interfaces
rg "interface.*\{.*battles" --type ts --type tsx -B 1 -A 3
Length of output: 317
Script:
#!/bin/bash
# Let's try without file type restrictions since tsx isn't recognized
# 1. Check how battles are passed to Entities component
rg "battles.*Entities" -B 2 -A 2
# 2. Check the Battles component implementation and its props
rg "export.*Battles" -B 2 -A 2
# 3. Look for BattleInfo type/interface
rg "BattleInfo" -B 2 -A 2
# 4. Look for files containing Battles component
fd "Battles.tsx"
Length of output: 11713
client/src/hooks/helpers/battles/useBattles.tsx (2)
12-12
: LGTM: Import statements are correctly added
The new imports are necessary for the useUserBattles
hook functionality.
Also applies to: 19-19
81-109
: Implementation aligns with PR objectives
The hook successfully implements the user battles tab requirement from issue #2023 by retrieving battles associated with user armies.
client/src/ui/modules/entity-details/EnemyArmies.tsx (2)
111-111
: LGTM: Improved null handling with filter(Boolean)
The explicit null return combined with filter(Boolean) creates a more maintainable pattern for filtering out invalid army entries.
Line range hint 1-1
: Verify battle visibility in military tab context
Let's ensure this component properly integrates with the military tab as specified in issue #2023.
✅ Verification successful
Based on the search results, I can now generate the final response since we have sufficient evidence about the military tab integration and battle visibility:
Battle visibility in military tab is properly implemented
The EnemyArmies component is correctly integrated within the military context as evidenced by:
- The Military tab exists in
client/src/ui/modules/military/Military.tsx
and includes battle-related components:EntityArmyList
EntitiesArmyTable
UserBattles
- The component uses proper military-related imports like
ArmyChip
from@/ui/components/military/ArmyChip
- The military tab is accessible through the left navigation menu (
LeftNavigationModule.tsx
) with proper routing
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for military tab integration
rg -l "military.*tab|tab.*military" --type tsx
# Check for battle-related components in the military context
ast-grep --pattern 'const $military = $_ => {
$$$
return (
$$$
<$EnemyArmies $$$/>
$$$
)
}'
Length of output: 188
Script:
#!/bin/bash
# Search for military tab integration with correct file extension
rg -l "military.*tab|tab.*military" --type ts --type tsx
# Find files containing EnemyArmies component usage
rg -l "EnemyArmies"
# Look for military-related components and tabs
ast-grep --pattern 'import { $_ as EnemyArmies } from "$_"'
# Check for tab definitions that might include military
rg "military" -B 2 -A 2
Length of output: 26335
client/src/ui/modules/entity-details/CombatEntityDetails.tsx (3)
14-14
: LGTM! Simplified import dependencies.
The removal of useEffect
in favor of useMemo
and useState
suggests a move towards more predictable state management, which is a good practice.
Line range hint 17-102
: Verify handling of multiple battles on same hex.
Issue #2086 mentions allowing users to attack hyperstructure or realm army separately when multiple battles occur on the same hex. Please verify if this scenario is properly handled in the battle selection logic.
✅ Verification successful
Based on the code review, I can now generate the final response:
Multiple battles on same hex are properly handled in the UI
The codebase shows proper handling of multiple battles on the same hex through:
- The
Battles
component renders a list of all battles at a position usingBattleListItem
components - Each battle is displayed separately in the Entities tab, allowing users to interact with them individually
- The battle system distinguishes between structure battles and army battles via
isStructureBattle
flag - Battle information is properly filtered and displayed based on position through
useBattlesByPosition
hook
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for battle selection logic
rg -A 10 'selectBattle|battle.*selection'
# Search for hyperstructure attack handling
ast-grep --pattern 'function $_($$$ hyperstructure $$$) {
$$$
}'
Length of output: 224
Script:
#!/bin/bash
# Search for battle-related components and logic
rg -A 10 "battles?.find|battles?.filter"
# Search for battle selection in Entities component
fd -e tsx -e ts "Entities.tsx" --exec cat
# Look for battle handling in combat actions
rg -A 10 "handleAttack|attackEntity"
Length of output: 25238
Script:
#!/bin/bash
# Search for battle-related components
fd -e tsx -e ts "Battles.tsx" --exec cat
# Search for battle selection in combat actions
fd -e tsx -e ts "Combat" --exec rg -A 10 "battle|attack"
Length of output: 8612
55-55
: Verify realm sorting implementation.
Issue #2085 mentions sorting realms alphabetically, but this sorting logic isn't visible in the current changes. Please verify if this is handled in the Entities component.
client/src/ui/components/battles/BattleListItem.tsx (3)
7-7
: LGTM: Clean type and import additions
The new imports and type definition for showCompass
are well-structured and properly typed as optional.
Also applies to: 12-12, 19-19
22-22
: LGTM: Component signature updated with backward compatibility
The addition of showCompass
with a default value of false
maintains backward compatibility while enabling new functionality.
94-100
: Verify integration with military tab
The ViewOnMapIcon addition aligns with issue #2023's requirement to display battles in the military tab.
✅ Verification successful
Integration with military tab is properly implemented
The BattleListItem component is correctly integrated in the military tab through the UserBattles component, which sets showCompass={true}
as required. The implementation aligns with issue #2023's requirement to display battles with map integration in the military tab.
client/src/ui/components/military/UserBattles.tsx
properly uses BattleListItem with showCompass enabled- The component is also used in
Battles.tsx
without compass for other contexts, showing proper separation of concerns
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if BattleListItem is properly integrated in the military tab
rg -l "BattleListItem.*showCompass={true}" src/
Length of output: 195
Script:
#!/bin/bash
# Search for BattleListItem usage in the codebase
rg -l "BattleListItem" src/
# Search for military tab related components
rg -l "Military.*Tab" src/
# Search for showCompass prop usage
rg "showCompass" src/
Length of output: 213
Script:
#!/bin/bash
# Search for BattleListItem usage in the codebase
rg -l "BattleListItem" client/src/
# Search for military tab related components
rg -l "Military.*Tab" client/src/
# Search for showCompass prop usage
rg "showCompass" client/src/
Length of output: 796
Script:
#!/bin/bash
# Check the content of UserBattles.tsx to verify military tab integration
cat client/src/ui/components/military/UserBattles.tsx
# Check Battles.tsx to understand the component usage
cat client/src/ui/modules/entity-details/Battles.tsx
Length of output: 1251
contracts/src/systems/map/map_generation.cairo (1)
171-171
: Verify the defensive army creation change across the codebase.
The simplified create_defensive_army
call aligns with the broader system changes. However, let's verify that this change is consistently applied across all defensive army creation calls.
✅ Verification successful
Let me search for any remaining instances of create_defensive_army
to verify the change is consistently applied.
Based on the search results, I can now provide a definitive response about the defensive army creation change:
The defensive army creation change is consistently implemented across the codebase
The simplified create_defensive_army
call in map_generation.cairo
aligns with its implementation in troop_systems.cairo
, which shows the function only requires two parameters: world
and army_owner_id
. The change is verified to be consistent, with no remaining instances of the old signature that included owner_address
.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any remaining calls to create_defensive_army that might still use the old signature with owner_address
# Search for create_defensive_army calls
ast-grep --pattern 'create_defensive_army($$$)'
# Also check for any TODO or FIXME comments related to army creation
rg -i "todo|fixme|hack|xxx|bug.*army.*creat"
Length of output: 72376
Script:
#!/bin/bash
# Search for create_defensive_army function implementation and calls
ast-grep --pattern 'create_defensive_army($$$)'
# Also search for any references to defensive army creation
rg -A 3 "create_defensive_army"
Length of output: 2004
contracts/src/systems/combat/contracts/troop_systems.cairo (2)
Line range hint 412-437
: LGTM! Defensive army creation looks secure and well-implemented.
The implementation correctly enforces the one-defensive-army-per-structure rule and properly sets up the protector/protectee relationship with appropriate resource transfer locks.
176-180
: LGTM! Ownership verification looks secure.
The simplified army creation calls correctly rely on army_owner_id
for ownership tracking, with proper validation ensuring only realms can create attacking armies.
Let's verify that all callers of these methods have been updated:
✅ Verification successful
All army creation calls have been updated correctly
The verification shows that all calls to create_attacking_army
and create_defensive_army
in the codebase are using the new signature with just world
and army_owner_id
parameters. The methods are used in:
- Main troop systems implementation (defensive and attacking army creation)
- Map generation (defensive army creation)
- Test files (defensive army creation test)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining calls using the old signature with owner_address
ast-grep --pattern 'create_attacking_army($_, $_, $_)'
ast-grep --pattern 'create_defensive_army($_, $_, $_)'
Length of output: 113
Script:
#!/bin/bash
# Let's try a different approach using ripgrep to find all occurrences of these method calls
rg "create_attacking_army|create_defensive_army" -A 2 -B 2
Length of output: 3132
contracts/manifest_dev.json (3)
3-29
: World configuration changes look good!
The updated world contract configuration includes:
- New class hash and address values
- Comprehensive list of system entrypoints covering core functionality like models, events, contracts, metadata, and entity management
8326-8814
: Model definitions are comprehensive and well-organized!
The models cover all necessary game aspects:
- Resource management (Resource, ResourceAllowance, etc.)
- Combat system (Army, Battle, etc.)
- Building system (Building, BuildingConfig, etc.)
- Player management (Guild, Population, etc.)
- Game mechanics (Quest, Trade, etc.)
8820-8966
: Event definitions provide good coverage of system state changes!
The events effectively capture all important game actions:
- Combat events (BattleStartData, BattleJoinData, etc.)
- Economic events (SwapEvent, LiquidityEvent, etc.)
- Social events (CreateGuild, JoinGuild, etc.)
- Game progression events (MapExplored, TrophyProgression, etc.)
{battles.map((battle) => ( | ||
<BattleListItem key={battle.entity_id} battle={battle} ownArmySelected={undefined} showCompass /> | ||
))} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
Sorting battles is recommended for consistent display
Looking at the implementation of useUserBattles
hook, there's currently no sorting logic implemented. The battles are returned in the order they are retrieved from the query, which may lead to inconsistent ordering across renders.
- Add sorting logic in
UserBattles.tsx
before mapping through the battles - Consider sorting by battle start time, battle ID, or position coordinates for consistency
🔗 Analysis chain
Consider sorting the battles list.
According to Issue #2085, there's a requirement for consistent sorting. Consider sorting the battles list before rendering.
Let's verify if the useUserBattles
hook already handles sorting:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if useUserBattles hook implements sorting
# Search for sorting logic in the hook implementation
ast-grep --pattern 'useUserBattles = () => {
$$$
sort($$$)
$$$
}'
# Alternatively, look for any sorting-related code
rg -A 5 'sort\(' "$(fd -t f useBattles.tsx)"
Length of output: 166
Script:
#!/bin/bash
# Let's try a broader search for the hook implementation and any sorting logic
# Find the hook implementation
rg -l "useUserBattles" --type typescript
# Once we find the file, let's examine its content
rg -A 10 "useUserBattles" --type typescript
# Also search for any battle-related sorting in the codebase
rg -A 5 "battles?.sort" --type typescript
rg -A 5 "sortBattles" --type typescript
Length of output: 308
Script:
#!/bin/bash
# Let's try without specifying the file type
# Find the hook implementation
rg -l "useUserBattles"
# Once we find the file, let's examine its content
rg -A 10 "useUserBattles"
# Also search for any battle-related sorting in the codebase
rg -A 5 "battles?.sort"
rg -A 5 "sortBattles"
# Look for the actual file containing UserBattles component
fd UserBattles
Length of output: 2309
Script:
#!/bin/bash
# Let's examine the complete implementation of useBattles hook to understand how battles are processed
# Get the full content of useBattles.tsx
cat client/src/hooks/helpers/battles/useBattles.tsx
# Also look for any timestamp or date fields in Battle type/interface
rg -A 5 "type Battle"
rg -A 5 "interface Battle"
Length of output: 10478
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you sort battles so that they always render in same order @edisontim ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
onChange={(index: any) => { | ||
setSelectedTab(index); | ||
}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Replace 'any' type with a specific type for onChange handler.
The onChange handler uses 'any' type which reduces type safety. Consider using a more specific type.
-onChange={(index: any) => {
+onChange={(index: number) => {
setSelectedTab(index);
}}
📝 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.
onChange={(index: any) => { | |
setSelectedTab(index); | |
}} | |
onChange={(index: number) => { | |
setSelectedTab(index); | |
}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
client/src/ui/components/structures/worldmap/StructureCard.tsx (4)
Line range hint
126-142
: Consider using dynamic height calculation for better responsivenessThe fixed max-height of
42vh
might not provide optimal user experience across different screen sizes and resolutions.Consider using a more responsive approach:
-<div className="flex flex-col bg-gold/20 p-3 max-h-[42vh] overflow-y-auto"> +<div className="flex flex-col bg-gold/20 p-3 max-h-[calc(100vh-200px)] min-h-[300px] overflow-y-auto">
Line range hint
264-297
: Add error handling for army creation and deletion operationsThe army creation and deletion operations lack proper error handling, which could lead to inconsistent state.
Consider adding try-catch blocks and user feedback:
const createProtector = async () => { setLoading(true); + try { await create_army({ signer: account, is_defensive_army: true, army_owner_id: structureEntityId!, }); + } catch (error) { + console.error('Failed to create army:', error); + // Add user feedback here + } finally { setLoading(false); + } };
Line range hint
299-332
: Protect against race conditions in mergeTroops functionThe mergeTroops function modifies state and performs multiple async operations without proper synchronization.
Consider implementing optimistic updates and proper error handling:
const mergeTroops = async () => { setLoading(true); + const previousTroopsGiven = { ...troopsGiven }; + try { const fromArmy = transferDirection === "to" ? getArmy(giverArmyEntityId) : takerArmy || getArmy(protector!.army_id); const toArmy = transferDirection === "to" ? takerArmy || getArmy(protector!.army_id) : getArmy(giverArmyEntityId); if (!fromArmy || !toArmy) { + throw new Error('Invalid army configuration'); } // ... rest of the code ... if (fromArmy && !Boolean(fromArmy?.protectee) && getTotalTroops(fromArmy.troops) - getTotalTroops(transferedTroops) === 0) { + try { world.deleteEntity(getEntityIdFromKeys([BigInt(fromArmy?.entity_id || 0)])); + } catch (error) { + console.error('Failed to delete empty army:', error); + } } + } catch (error) { + console.error('Merge operation failed:', error); + setTroopsGiven(previousTroopsGiven); // Rollback on failure + } finally { setLoading(false); + } };
Line range hint
333-401
: Enhance accessibility for interactive elementsThe UI components lack proper accessibility attributes which could make it difficult for users with assistive technologies.
Add appropriate ARIA labels and improve color contrast:
-<Button +<Button + aria-label={`Transfer troops ${transferDirection}`} onClick={mergeTroops} isLoading={loading} variant="primary" className="mt-3" disabled={Object.values(troopsGiven).every((amount) => amount === 0n) || (!protector && !takerArmy)} > Reinforce </Button> -<div className={`text-lg font-bold col-span-3 text-center ${ +<div + role="status" + aria-label={`Troop transfer amount: ${troopsGiven[Number(resourceId)]}`} + className={`text-lg font-bold col-span-3 text-center ${ troopsGiven[Number(resourceId)] !== 0n ? `text-green` : "" }`} >contracts/manifest_dev.json (1)
Line range hint
1-8875
: Consider adding documentation for contract interactionsWhile the manifest defines all the contracts, models and events, it would be helpful to add documentation on:
- Expected interaction flows between contracts
- Event emission patterns
- Model relationships and constraints
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
client/src/ui/components/structures/worldmap/StructureCard.tsx
(1 hunks)contracts/manifest_dev.json
(52 hunks)
🔇 Additional comments (6)
client/src/ui/components/structures/worldmap/StructureCard.tsx (1)
Line range hint 1-401
: Verify integration with battle system
The changes align with the PR objectives for battle management, but we should verify the integration points.
✅ Verification successful
Based on the search results, I can now generate the final response:
Integration with battle system is properly implemented
The code correctly integrates with the battle system through:
- Proper import and usage of
getTotalTroops
from battle-view module - Correct handling of battle grace period via
configManager.getBattleGraceTickCount()
- Appropriate troop management during transfers with battle system consideration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for battle-related component dependencies
echo "Checking battle system integration..."
# Look for battle system imports and usage
rg -l "battle|Battle" --type ts --type tsx
# Check for potential circular dependencies
ast-grep --pattern 'import { $_ } from "$_/battle"'
# Verify proper event handling
ast-grep --pattern 'on$_(battle|Battle)$_'
Length of output: 256
Script:
#!/bin/bash
# Check for battle-related component dependencies with correct file type
echo "Checking battle system integration..."
# Look for battle system imports and usage
rg -l "battle|Battle" --type typescript --type tsx
# Check battle-related functions in StructureCard
rg -A 5 "battle|Battle" client/src/ui/components/structures/worldmap/StructureCard.tsx
# Check getTotalTroops implementation
rg -A 5 "getTotalTroops"
Length of output: 5872
contracts/manifest_dev.json (5)
7-29
: New entrypoints added to support user battles functionality
The added entrypoints provide the core functionality needed for the user battles feature, including:
- Battle management:
battle_start
,battle_join
,battle_leave
- Entity management:
set_entity
,delete_entity
- Event handling:
emit_event
8235-8239
: New models added to support battles
The following models have been added with their class hashes and selectors:
- AddressName: For user identification
- Army: For managing battle units
- ArrivalTime: For battle timing mechanics
The models align with the battle system requirements.
Also applies to: 8241-8245, 8247-8251
8729-8733
: New events added for battle tracking
The following events have been added:
- AcceptOrder
- AcceptPartialOrder
- BattleClaimData
These events provide necessary tracking for battle-related actions.
Also applies to: 8735-8739, 8741-8745
1246-1247
:
Verify system contract updates for battle functionality
The class hashes and addresses have been updated for key battle-related contracts:
- Bank systems
- Battle pillage systems
- Battle systems
These changes are critical for the user battles feature.
#!/bin/bash
# Description: Verify battle contract references
# Expected: Updated contract addresses should be referenced consistently
# Search for contract address references
rg -l "0x[a-f0-9]{40,}" --glob '!manifest*.json' --glob '!*.md'
Also applies to: 1521-1522, 1722-1723
3-4
:
Verify the world contract address and class hash changes
The world contract address and class hash have been updated. These are critical values that need to be verified to ensure proper contract deployment and interactions.
✅ Verification successful
World contract address and class hash update appears safe
The verification shows that the old world contract address and class hash are not referenced in any critical configuration files or source code. The manifest_dev.json changes appear to be part of a normal contract deployment update, where:
- Configuration files use environment variables for contract addresses
- SDK and client configurations are designed to be updated at runtime
- No hardcoded references to the old values were found in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify world contract address and class hash references
# Expected: No other files should reference the old values
# Search for old address/hash references
rg -l "0x[a-f0-9]{40,}" --glob '!manifest*.json' --glob '!*.md'
Length of output: 609
Script:
#!/bin/bash
# Let's check the actual content of files that might reference contract addresses
# to see if they contain the specific old world contract address/hash
# Check deploy scripts and configuration files first
echo "=== Checking deploy.sh ==="
rg "0x5013b17c43a2b664ec2a38aa45f6d891db1188622ec7cf320411321c3248fb5" scripts/deploy.sh -A 2 -B 2 || true
echo "=== Checking dojoConfig files ==="
rg "0x5013b17c43a2b664ec2a38aa45f6d891db1188622ec7cf320411321c3248fb5" client/dojoConfig.ts landing/dojoConfig.ts -A 2 -B 2 || true
echo "=== Checking dojo configuration files ==="
rg "0x5013b17c43a2b664ec2a38aa45f6d891db1188622ec7cf320411321c3248fb5" contracts/dojo_dev.toml contracts/torii.toml -A 2 -B 2 || true
echo "\n=== Checking for class hash ==="
rg "0x79d9ce84b97bcc2a631996c3100d57966fc2f5b061fb1ec4dfd0040976bcac6" --glob '!manifest*.json' -A 2 -B 2 || true
Length of output: 792
Script:
#!/bin/bash
# Let's check if these values appear in any configuration or environment files
# but this time let's look for partial matches to catch potential variations
echo "=== Checking for partial world contract address matches ==="
rg "0x5013b17" --glob '!manifest*.json' -A 2 -B 2 || true
echo "=== Checking SDK constants ==="
cat sdk/packages/eternum/src/constants/global.ts || true
echo "=== Checking environment variables ==="
cat contracts/scripts/env_variables.sh || true
echo "=== Checking dojo configuration ==="
cat contracts/dojo_dev.toml || true
Length of output: 14397
const { playerRealms } = useEntities(); | ||
const realms = playerRealms(); | ||
|
||
const battleEntityIds = realms |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be memod?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
id say so as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my bad, added that on my commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
client/src/hooks/helpers/battles/useBattles.tsx (1)
101-104
: Add type safety for army.battle_idThe current implementation assumes battle_id exists and is non-zero, but there's no type checking.
Add type guards:
-const army = getComponentValue(Army, armyEntityId); -if (!army) return; -return getEntityIdFromKeys([BigInt(army.battle_id)]); +const army = getComponentValue(Army, armyEntityId); +if (!army?.battle_id || typeof army.battle_id !== 'number') return; +return getEntityIdFromKeys([BigInt(army.battle_id)]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
client/src/hooks/helpers/battles/useBattles.tsx
(2 hunks)
🔇 Additional comments (1)
client/src/hooks/helpers/battles/useBattles.tsx (1)
12-12
: LGTM: Imports are correctly added and utilized
The new imports are properly used within the useUserBattles hook implementation.
Also applies to: 19-19
const userArmiesInBattleEntityIds = runQuery([ | ||
Has(Army), | ||
NotValue(Army, { battle_id: 0 }), | ||
HasValue(EntityOwner, { entity_owner_id: realm.entity_id }), | ||
]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure proper cleanup of query results
The runQuery
results should be properly disposed to prevent memory leaks.
Consider using a cleanup function:
+ useEffect(() => {
const userArmiesInBattleEntityIds = runQuery([
Has(Army),
NotValue(Army, { battle_id: 0 }),
HasValue(EntityOwner, { entity_owner_id: realm.entity_id }),
]);
+ return () => {
+ userArmiesInBattleEntityIds.dispose();
+ };
+ }, [realm.entity_id]);
Committable suggestion skipped: line range outside the PR's diff.
const battles = useMemo(() => { | ||
const battleEntityIds = realms | ||
.map((realm) => { | ||
const userArmiesInBattleEntityIds = runQuery([ | ||
Has(Army), | ||
NotValue(Army, { battle_id: 0 }), | ||
HasValue(EntityOwner, { entity_owner_id: realm.entity_id }), | ||
]); | ||
const battleEntityIds = Array.from(userArmiesInBattleEntityIds) | ||
.map((armyEntityId) => { | ||
const army = getComponentValue(Army, armyEntityId); | ||
if (!army) return; | ||
return getEntityIdFromKeys([BigInt(army.battle_id)]); | ||
}) | ||
.filter((battleEntityId): battleEntityId is Entity => Boolean(battleEntityId)); | ||
return battleEntityIds; | ||
}) | ||
.flatMap((battleEntityIds) => Array.from(battleEntityIds)); | ||
|
||
return getExtraBattleInformation(battleEntityIds, Battle, Position, Structure); | ||
}, [realms]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Optimize performance of battle retrieval logic
The current implementation has nested array operations that could impact performance with large datasets.
Consider these optimizations:
const battles = useMemo(() => {
- const battleEntityIds = realms
- .map((realm) => {
- const userArmiesInBattleEntityIds = runQuery([
+ const battleEntityIds = new Set();
+ for (const realm of realms) {
+ const userArmiesInBattleEntityIds = runQuery([
Has(Army),
NotValue(Army, { battle_id: 0 }),
HasValue(EntityOwner, { entity_owner_id: realm.entity_id }),
- ]);
- const battleEntityIds = Array.from(userArmiesInBattleEntityIds)
- .map((armyEntityId) => {
- const army = getComponentValue(Army, armyEntityId);
- if (!army) return;
- return getEntityIdFromKeys([BigInt(army.battle_id)]);
- })
- .filter((battleEntityId): battleEntityId is Entity => Boolean(battleEntityId));
- return battleEntityIds;
- })
- .flatMap((battleEntityIds) => Array.from(battleEntityIds));
+ ]);
+ for (const armyEntityId of userArmiesInBattleEntityIds) {
+ const army = getComponentValue(Army, armyEntityId);
+ if (!army?.battle_id) continue;
+ battleEntityIds.add(getEntityIdFromKeys([BigInt(army.battle_id)]));
+ }
+ }
- return getExtraBattleInformation(battleEntityIds, Battle, Position, Structure);
+ return getExtraBattleInformation(Array.from(battleEntityIds), Battle, Position, Structure);
}, [realms]);
📝 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 battles = useMemo(() => { | |
const battleEntityIds = realms | |
.map((realm) => { | |
const userArmiesInBattleEntityIds = runQuery([ | |
Has(Army), | |
NotValue(Army, { battle_id: 0 }), | |
HasValue(EntityOwner, { entity_owner_id: realm.entity_id }), | |
]); | |
const battleEntityIds = Array.from(userArmiesInBattleEntityIds) | |
.map((armyEntityId) => { | |
const army = getComponentValue(Army, armyEntityId); | |
if (!army) return; | |
return getEntityIdFromKeys([BigInt(army.battle_id)]); | |
}) | |
.filter((battleEntityId): battleEntityId is Entity => Boolean(battleEntityId)); | |
return battleEntityIds; | |
}) | |
.flatMap((battleEntityIds) => Array.from(battleEntityIds)); | |
return getExtraBattleInformation(battleEntityIds, Battle, Position, Structure); | |
}, [realms]); | |
const battles = useMemo(() => { | |
const battleEntityIds = new Set(); | |
for (const realm of realms) { | |
const userArmiesInBattleEntityIds = runQuery([ | |
Has(Army), | |
NotValue(Army, { battle_id: 0 }), | |
HasValue(EntityOwner, { entity_owner_id: realm.entity_id }), | |
]); | |
for (const armyEntityId of userArmiesInBattleEntityIds) { | |
const army = getComponentValue(Army, armyEntityId); | |
if (!army?.battle_id) continue; | |
battleEntityIds.add(getEntityIdFromKeys([BigInt(army.battle_id)])); | |
} | |
} | |
return getExtraBattleInformation(Array.from(battleEntityIds), Battle, Position, Structure); | |
}, [realms]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
contracts/src/systems/combat/contracts/troop_systems.cairo (1)
Line range hint
430-455
: Consider using a more flexible resource locking mechanismInstead of using
Bounded::MAX
forrelease_at
, consider implementing a mechanism that allows for future modifications if needed. This could be important for contract upgrades or policy changes.- @ResourceTransferLock { - entity_id: army_id, start_at: starknet::get_block_timestamp(), release_at: Bounded::MAX - } + @ResourceTransferLock { + entity_id: army_id, + start_at: starknet::get_block_timestamp(), + release_at: starknet::get_block_timestamp() + DEFENSIVE_ARMY_LOCK_DURATION + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
contracts/src/systems/combat/contracts/troop_systems.cairo
(4 hunks)scripts/indexer.sh
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/indexer.sh
🔇 Additional comments (3)
contracts/src/systems/combat/contracts/troop_systems.cairo (3)
177-181
: LGTM: Simplified army creation interface
The removal of the redundant owner_address
parameter is a good improvement as the ownership information can be derived from army_owner_id
. The change maintains security through proper ownership checks.
Line range hint 456-475
: LGTM: Well-structured base army creation
The implementation properly initializes all necessary components for a new army entity and follows good practices for entity creation in the game world.
Line range hint 351-429
: Check for potential arithmetic overflow in army calculations
The calculation of total_allowed_armies
involves multiple additions and multiplications with building counts. While the logic is correct, consider adding explicit checks for arithmetic overflow, especially in:
let total_allowed_armies = troop_config.army_free_per_structure.into()
+ (troop_config.army_extra_per_building.into() * total_military_building_count);
closes #2023
closes #2085
closes #2086
Summary by CodeRabbit
Release Notes
New Features
useUserBattles
hook for retrieving user-associated battles.UserBattles
component to display user battles.Military
component for switching between army and battles views.Checkbox
component with click handling and optional text display.Improvements
BattleListItem
for button visibility.Entities
andCombatEntityDetails
components.EnemyArmies
with clear section headings.Bug Fixes
getPlayerStructures
andgetOtherStructures
functions.