Skip to content
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

Open
wants to merge 7 commits into
base: next
Choose a base branch
from
Open

feat: Add user battles tab #2102

wants to merge 7 commits into from

Conversation

edisontim
Copy link
Collaborator

@edisontim edisontim commented Nov 13, 2024

closes #2023
closes #2085
closes #2086

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced the useUserBattles hook for retrieving user-associated battles.
    • Added UserBattles component to display user battles.
    • Implemented a tabbed interface in the Military component for switching between army and battles views.
    • Enhanced Checkbox component with click handling and optional text display.
  • Improvements

    • Simplified logic in BattleListItem for button visibility.
    • Improved rendering logic in Entities and CombatEntityDetails components.
    • Enhanced user interface in EnemyArmies with clear section headings.
  • Bug Fixes

    • Corrected filtering and sorting in getPlayerStructures and getOtherStructures functions.

Copy link

vercel bot commented Nov 13, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
eternum ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 15, 2024 8:28pm
eternum-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 15, 2024 8:28pm
eternum-landing ❌ Failed (Inspect) Nov 15, 2024 8:28pm

Copy link
Contributor

coderabbitai bot commented Nov 13, 2024

Walkthrough

The pull request introduces several enhancements across multiple components and hooks in the application. A new hook, useUserBattles, retrieves battles associated with user armies. Modifications to existing hooks improve structure filtering and sorting. The BattleListItem and UserBattles components are updated to streamline rendering and interaction with battle data. The Checkbox component is enhanced for better user interaction, while the Entities component gains interactivity through toggleable sections. Other changes include adjustments to the Military component for a tabbed interface and various updates to the troop management system in the contract.

Changes

File Change Summary
client/src/hooks/helpers/battles/useBattles.tsx Added useUserBattles hook to retrieve user-associated battles.
client/src/hooks/helpers/useEntities.tsx Updated getPlayerStructures and getOtherStructures for filtering and sorting.
client/src/ui/components/battles/BattleListItem.tsx Removed getUserArmyInBattle, added showCompass prop, simplified button rendering logic.
client/src/ui/components/military/UserBattles.tsx Introduced UserBattles component to display user battles.
client/src/ui/elements/Checkbox.tsx Updated Checkbox to include onClick and text props, enhanced layout.
client/src/ui/modules/entity-details/Battles.tsx Modified outer <div> class for layout adjustments.
client/src/ui/modules/entity-details/CombatEntityDetails.tsx Removed useEffect, simplified state management.
client/src/ui/modules/entity-details/EnemyArmies.tsx Adjusted rendering logic in getArmyChip and updated structure for displaying enemy armies.
client/src/ui/modules/entity-details/Entities.tsx Updated function signature to include battles, added state for visibility toggles.
client/src/ui/modules/military/Military.tsx Introduced tabbed interface with UserBattles and state management for selected tab.
contracts/src/systems/combat/contracts/troop_systems.cairo Removed owner_address from troop-related methods, streamlined army management logic.
scripts/indexer.sh Added --external argument for enhanced script execution, updated command parameters.
client/src/ui/components/structures/worldmap/StructureCard.tsx Made StructureMergeTroopsPanel local, updated props and troop merging logic.

Assessment against linked issues

Objective Addressed Explanation
Show battles you are involved in in military tab (2023)
Sort realms in alphabetical order (2085)
Fix multiple battles bug (2086) Unclear if the changes fully address the simultaneous battle interaction issue.

Possibly related PRs

Suggested reviewers

  • aymericdelab
  • bob0005

Poem

🐇 In the fields where battles bloom,
A new hook brings forth the loom.
User armies now take flight,
With each battle, a new delight.
Click the checkbox, see them merge,
In this world, our troops converge! 🌼


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

Failed to generate code suggestions for PR

Copy link
Contributor

@mentatbot mentatbot bot left a 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:

  1. Several components could benefit from better memoization of callbacks and derived values
  2. Some type definitions could be more specific
  3. The UI layout uses hardcoded widths which could be made more flexible
  4. 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!

client/src/hooks/helpers/useEntities.tsx Show resolved Hide resolved
client/src/ui/elements/Checkbox.tsx Show resolved Hide resolved
client/src/ui/modules/military/Military.tsx Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 like onToggle or onStateChange 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 props

Consider 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 visibility

The 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 grid

While 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:

  1. Add error checking after critical operations (contract deployment, file operations)
  2. Validate generated contract addresses before updating configuration
  3. Add logging to track important state changes
  4. 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 optimizations

The 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 semantics

The 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 condition

The 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 state

While 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 component

The 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 to true for ViewOnMapIcon. 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:

  1. Extract random troop count generation into a helper function to reduce code duplication
  2. 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 implementation

The 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 -d

Length 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 -l

Length of output: 537

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 5b1e27b and 0fb3019.

📒 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:

  1. Adding a dedicated "Battles" tab for viewing user battles
  2. Maintaining existing army functionality in a separate tab
  3. 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 in useBattles.tsx with all necessary properties
  • Battles component in entity-details/Battles.tsx correctly accepts and uses the required props (ownArmy and battles)
  • 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 using BattleListItem 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.)

client/src/ui/components/military/UserBattles.tsx Outdated Show resolved Hide resolved
Comment on lines 12 to 14
{battles.map((battle) => (
<BattleListItem key={battle.entity_id} battle={battle} ownArmySelected={undefined} showCompass />
))}
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 13, 2024

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

Copy link
Collaborator

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 ?

Copy link
Contributor

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!

client/src/ui/elements/Checkbox.tsx Show resolved Hide resolved
Comment on lines +33 to +35
onChange={(index: any) => {
setSelectedTab(index);
}}
Copy link
Contributor

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.

Suggested change
onChange={(index: any) => {
setSelectedTab(index);
}}
onChange={(index: number) => {
setSelectedTab(index);
}}

scripts/indexer.sh Outdated Show resolved Hide resolved
client/src/hooks/helpers/battles/useBattles.tsx Outdated Show resolved Hide resolved
client/src/hooks/helpers/useEntities.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 responsiveness

The 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 operations

The 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 function

The 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 elements

The 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 interactions

While 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

📥 Commits

Reviewing files that changed from the base of the PR and between 0fb3019 and 2cdc98d.

📒 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: ⚠️ Potential issue

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: ⚠️ Potential issue

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:

  1. Configuration files use environment variables for contract addresses
  2. SDK and client configurations are designed to be updated at runtime
  3. 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be memod?

Copy link
Collaborator

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

Copy link
Collaborator Author

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

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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_id

The 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

📥 Commits

Reviewing files that changed from the base of the PR and between 8a18e82 and e6d73ff.

📒 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

Comment on lines +94 to +98
const userArmiesInBattleEntityIds = runQuery([
Has(Army),
NotValue(Army, { battle_id: 0 }),
HasValue(EntityOwner, { entity_owner_id: realm.entity_id }),
]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Comment on lines +91 to +111
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]);
Copy link
Contributor

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.

Suggested change
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]);

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 mechanism

Instead of using Bounded::MAX for release_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

📥 Commits

Reviewing files that changed from the base of the PR and between 3dc608d and 5306996.

📒 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);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants