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

refactor #2645

Merged
merged 4 commits into from
Jan 16, 2025
Merged

refactor #2645

merged 4 commits into from
Jan 16, 2025

Conversation

aymericdelab
Copy link
Collaborator

@aymericdelab aymericdelab commented Jan 16, 2025

Summary by CodeRabbit

Based on the comprehensive summary, here are the release notes:

  • Code Refactoring

    • Reorganized utility functions and import paths across multiple components
    • Moved several utility functions to more centralized locations
    • Updated import statements to improve code modularity
  • Utility Updates

    • Added new hexagonal grid utility functions in three/utils.ts
    • Introduced functions for battle simulation and resource management
    • Refined address and entity-related utility functions
  • Battle Simulation

    • Renamed Battle class to BattleSimulator
    • Updated battle-related components to use new BattleSimulator
  • Resource Management

    • Added functions to calculate donkey transport requirements
    • Improved resource weight calculation utilities

These changes primarily focus on code organization and utility function management, with no significant user-facing changes.

Copy link

vercel bot commented Jan 16, 2025

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 Jan 16, 2025 4:37pm
eternum-docs ❌ Failed (Inspect) Jan 16, 2025 4:37pm
1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
eternum-landing ⬜️ Ignored (Inspect) Visit Preview Jan 16, 2025 4:37pm

Copy link
Contributor

mentatbot bot commented Jan 16, 2025

Hi @aymericdelab! You need to be added as a user to interact with me. Please ask @ponderingdemocritus to add you on the settings page.

You are receiving this comment because I am set to review all PRs. That is configurable here.

Copy link
Contributor

coderabbitai bot commented Jan 16, 2025

Warning

There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure.

🔧 eslint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

client/apps/game/src/three/managers/army-manager.ts

Oops! Something went wrong! :(

ESLint: 9.18.0

ESLint couldn't find an eslint.config.(js|mjs|cjs) file.

From ESLint v9.0.0, the default configuration file is now eslint.config.js.
If you are using a .eslintrc.* file, please follow the migration guide
to update your configuration file to the new format:

https://eslint.org/docs/latest/use/configure/migration-guide

If you still have problems after following the migration guide, please stop by
https://eslint.org/chat/help to chat with the team.

Walkthrough

This pull request involves a comprehensive reorganization of import paths and utility functions across multiple files in the client application. The primary focus is on consolidating and relocating utility functions, particularly those related to hexagonal grid calculations, resource management, and entity-related operations. The changes span various modules, including three-dimensional scene management, UI components, and core SDK utilities, with a consistent theme of moving imports from @/ui/utils/utils to more specific or relative paths.

Changes

File Path Change Summary
client/apps/game/src/three/managers/* Updated import paths for hexagonal utility functions from @/ui/utils/utils to ../utils
client/apps/game/src/three/scenes/* Modified import statements for world position and hexagonal coordinate functions
client/apps/game/src/three/utils.ts Added new utility functions for hexagonal grid calculations
client/apps/game/src/ui/components/* Reorganized imports, moving some utility functions to @bibliothecadao/eternum
client/sdk/packages/core/src/utils/* Renamed classes, added new utility functions, updated export configurations

Sequence Diagram

sequenceDiagram
    participant Client as Client Application
    participant Utils as Utility Module
    participant SDK as Core SDK
    
    Client->>Utils: Request Hexagonal Utility Functions
    Utils-->>Client: Return Hexagonal Coordinates
    
    Client->>SDK: Request Entity and Resource Utilities
    SDK-->>Client: Provide Updated Utility Functions
    
    Note over Client,SDK: Import paths reorganized
    Note over Client,Utils: Utility functions consolidated
Loading

Possibly related PRs

Suggested reviewers

  • bob0005

Poem

🐰 A Rabbit's Ode to Code Refactoring 🐰

Imports dancing, paths rearranged,
Utility functions neatly exchanged,
From utils to SDK, a journey so bright,
Hexagonal magic takes elegant flight!

Hop, hop, refactor with glee! 🌈

Finishing Touches

  • 📝 Generate Docstrings (Beta)

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 generate docstrings to generate docstrings for this PR. (Beta)
  • @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

@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: 1

🔭 Outside diff range comments (2)
client/apps/game/src/ui/components/trading/market-order-panel.tsx (2)

Line range hint 64-67: Fix malformed className string.

The className string contains an incomplete hover class (hover:) which might cause unexpected styling behavior.

-        className={`w-full border-gold/5 rounded-xl h-8 p-1 cursor-pointer grid grid-cols-5 gap-1 hover:bg-gold/10 hover:  group ${
+        className={`w-full border-gold/5 rounded-xl h-8 p-1 cursor-pointer grid grid-cols-5 gap-1 hover:bg-gold/10 group ${

Line range hint 446-456: Handle potential floating-point precision errors in bid calculations.

The current bid calculation (lords / resource) might lead to floating-point precision errors. Consider using a decimal arithmetic library like decimal.js or big.js for precise financial calculations.

+import { Decimal } from 'decimal.js';

-setBid(String(lords / resource));
+const decimalLords = new Decimal(lords);
+const decimalResource = new Decimal(resource);
+setBid(decimalLords.dividedBy(decimalResource).toString());
🧹 Nitpick comments (11)
client/sdk/packages/core/src/utils/resources.ts (1)

70-79: Consider using array includes for better maintainability.

The function logic is correct, but could be more maintainable using array includes.

 export const isResourceProductionBuilding = (buildingId: BuildingType) => {
-  return (
-    buildingId === BuildingType.Resource ||
-    buildingId === BuildingType.Farm ||
-    buildingId === BuildingType.FishingVillage ||
-    buildingId === BuildingType.Barracks ||
-    buildingId === BuildingType.ArcheryRange ||
-    buildingId === BuildingType.Stable
-  );
+  const productionBuildings = [
+    BuildingType.Resource,
+    BuildingType.Farm,
+    BuildingType.FishingVillage,
+    BuildingType.Barracks,
+    BuildingType.ArcheryRange,
+    BuildingType.Stable
+  ];
+  return productionBuildings.includes(buildingId);
 };
client/apps/game/src/ui/components/resources/travel-info.tsx (1)

Line range hint 39-58: Document the special case handling for donkeys and lords.

The comment "hacky way to set can carry to true if only donkeys and lords" suggests this is a special case that needs proper documentation. Consider:

  1. Adding a comment explaining why this case is handled differently
  2. Creating a separate function with a descriptive name for this check
-    const onlyDonkeysAndLords = resources.every(
-      (r) => r.resourceId === ResourcesIds.Donkey || r.resourceId === ResourcesIds.Lords,
-    );
+    // Donkeys and Lords are exempt from weight restrictions as they represent
+    // special resources that don't contribute to the caravan's weight limit
+    const isExemptFromWeightRestriction = resources.every(
+      (r) => r.resourceId === ResourcesIds.Donkey || r.resourceId === ResourcesIds.Lords,
+    );

     if (setCanCarry) {
-      // TODO: hacky way to set can carry to true if only donkeys and lords
-      onlyDonkeysAndLords ? setCanCarry(true) : setCanCarry(calculatedDonkeyBalance >= neededDonkeys);
+      isExemptFromWeightRestriction
+        ? setCanCarry(true)
+        : setCanCarry(calculatedDonkeyBalance >= neededDonkeys);
     }
client/apps/game/src/three/utils.ts (2)

19-33: Extract duplicated constants into named variables.

The hexagonal grid constants are duplicated in both getWorldPositionForHex and getHexForWorldPosition functions. Consider extracting them into named constants at the module level for better maintainability.

+const HEX_RADIUS = HEX_SIZE;
+const HEX_HEIGHT = HEX_RADIUS * 2;
+const HEX_WIDTH = Math.sqrt(3) * HEX_RADIUS;
+const VERT_DIST = HEX_HEIGHT * 0.75;
+const HORIZ_DIST = HEX_WIDTH;

 export const getWorldPositionForHex = (hexCoords: HexPosition, flat: boolean = true) => {
-  const hexRadius = HEX_SIZE;
-  const hexHeight = hexRadius * 2;
-  const hexWidth = Math.sqrt(3) * hexRadius;
-  const vertDist = hexHeight * 0.75;
-  const horizDist = hexWidth;

   const col = hexCoords.col;
   const row = hexCoords.row;
-  const rowOffset = ((row % 2) * Math.sign(row) * horizDist) / 2;
-  const x = col * horizDist - rowOffset;
-  const z = row * vertDist;
+  const rowOffset = ((row % 2) * Math.sign(row) * HORIZ_DIST) / 2;
+  const x = col * HORIZ_DIST - rowOffset;
+  const z = row * VERT_DIST;
   const y = flat ? 0 : pseudoRandom(x, z) * 2;
   return new THREE.Vector3(x, y, z);
 };

 export const getHexForWorldPosition = (worldPosition: { x: number; y: number; z: number }): HexPosition => {
-  const hexRadius = HEX_SIZE;
-  const hexHeight = hexRadius * 2;
-  const hexWidth = Math.sqrt(3) * hexRadius;
-  const vertDist = hexHeight * 0.75;
-  const horizDist = hexWidth;

-  const row = Math.round(worldPosition.z / vertDist);
+  const row = Math.round(worldPosition.z / VERT_DIST);
   // hexception offsets hack
-  const rowOffset = ((row % 2) * Math.sign(row) * horizDist) / 2;
-  const col = Math.round((worldPosition.x + rowOffset) / horizDist);
+  const rowOffset = ((row % 2) * Math.sign(row) * HORIZ_DIST) / 2;
+  const col = Math.round((worldPosition.x + rowOffset) / HORIZ_DIST);

   return {
     col,
     row,
   };
 };

Also applies to: 35-51


80-83: Document the pseudoRandom function's purpose and implementation.

Add JSDoc comments to explain the purpose of the pseudoRandom function and how it generates deterministic height values.

+/**
+ * Generates a deterministic pseudo-random value between 0 and 1 based on x,y coordinates.
+ * Uses a sine-based hash function to ensure consistent height values for the same coordinates.
+ */
 const pseudoRandom = (x: number, y: number) => {
   const n = Math.sin(x * 12.9898 + y * 78.233) * 43758.5453123;
   return n - Math.floor(n);
 };
client/apps/game/src/three/scenes/worldmap.ts (3)

Line range hint 1022-1056: Consider optimizing network requests in computeTileEntities.

The method makes separate network requests for tiles and positions that could be combined to reduce network overhead. Consider using a single query with a more complex composite clause.

-      const promiseTiles = getEntities(/*...*/);
-      const promisePositions = getEntities(/*...*/);
-      Promise.all([promiseTiles, promisePositions]).then(() => {
+      const promiseEntities = getEntities(
+        this.dojo.network.toriiClient,
+        {
+          Composite: {
+            operator: "Or",
+            clauses: [
+              // Tile query clause
+              {/*...*/},
+              // Position query clause
+              {/*...*/}
+            ]
+          }
+        },
+        this.dojo.network.contractComponents as any,
+        [],
+        [
+          "s0_eternum-Tile",
+          "s0_eternum-Army",
+          "s0_eternum-Position",
+          "s0_eternum-Health",
+          "s0_eternum-EntityOwner",
+          "s0_eternum-Protectee",
+          "s0_eternum-Stamina"
+        ],
+        1000,
+        false
+      );
+      promiseEntities.then(() => {

Line range hint 1217-1236: Consider implementing more granular cache invalidation.

The updateVisibleChunks method currently invalidates the entire chunk cache when force-updating. Consider implementing more granular cache invalidation based on the specific entities that changed.

   if (this.currentChunk !== chunkKey || force) {
     this.currentChunk = chunkKey;
-    // Calculate the starting position for the new chunk
-    this.updateHexagonGrid(startRow, startCol, this.renderChunkSize.height, this.renderChunkSize.width);
+    // Only update changed entities within the chunk
+    if (force) {
+      const changedEntities = this.getChangedEntitiesInChunk(chunkKey);
+      if (changedEntities.length > 0) {
+        this.updateHexagonGridPartial(startRow, startCol, changedEntities);
+      } else {
+        this.updateHexagonGrid(startRow, startCol, this.renderChunkSize.height, this.renderChunkSize.width);
+      }
+    } else {
+      this.updateHexagonGrid(startRow, startCol, this.renderChunkSize.height, this.renderChunkSize.width);
+    }
     this.armyManager.updateChunk(chunkKey);
     this.structureManager.updateChunk(chunkKey);
   }

Line range hint 123-127: Consider using a subscription manager for better cleanup.

While the code handles unsubscription, it could benefit from a more systematic approach to subscription management to prevent memory leaks.

+  private subscriptions: Array<{ unsubscribe: () => void }> = [];
+
   constructor(/*...*/) {
     // ...
-    this.armySubscription?.unsubscribe();
-    this.armySubscription = this.systemManager.Army.onUpdate((update: ArmySystemUpdate) => {
+    const armySubscription = this.systemManager.Army.onUpdate((update: ArmySystemUpdate) => {
       this.armyManager.onUpdate(update).then((needsUpdate) => {
         if (needsUpdate) {
           this.updateVisibleChunks();
         }
       });
     });
+    this.subscriptions.push(armySubscription);
   }
+
+  public dispose() {
+    this.subscriptions.forEach(sub => sub.unsubscribe());
+    this.subscriptions = [];
+  }
client/sdk/packages/core/src/utils/transport.ts (1)

13-21: Consider simplifying the reduce callback for better readability.

While the logic is correct, the expression could be more readable.

-      total + (resource ? resource.amount * configManager.getResourceWeight(resource.resourceId) || 0 : 0),
+      total + (resource?.amount * (configManager.getResourceWeight(resource.resourceId) ?? 0) ?? 0),
client/sdk/packages/core/src/utils/battle-simulator.ts (1)

384-386: Consider enhancing the error message.

The error message could be more descriptive to help with debugging.

-      throw new Error("Health is not a multiple of normalization factor.");
+      throw new Error(`Health ${this.current} is not a multiple of normalization factor ${singleTroopHealth}.`);
client/apps/game/src/ui/components/trading/market-order-panel.tsx (2)

5-5: Consider more granular imports for utility functions.

Instead of importing from a generic utils file, consider creating specific utility modules for currency/number formatting functions to enable better tree-shaking and reduce bundle size.


Line range hint 249-422: Optimize resource balance calculations and state updates.

Several performance improvements can be made:

  1. Combine related useMemo hooks for balance calculations
  2. Consider using a reducer for complex state management
  3. Move heavy calculations outside the component or into a custom hook

Example of combining related useMemo hooks:

-const resourceBalanceRatio = useMemo(
-  () => (resourceBalance < getsDisplayNumber ? resourceBalance / getsDisplayNumber : 1),
-  [resourceBalance, getsDisplayNumber],
-);
-const lordsBalanceRatio = useMemo(
-  () => (lordsBalance < getTotalLords ? lordsBalance / getTotalLords : 1),
-  [lordsBalance, getTotalLords],
-);
+const { resourceBalanceRatio, lordsBalanceRatio } = useMemo(
+  () => ({
+    resourceBalanceRatio: resourceBalance < getsDisplayNumber ? resourceBalance / getsDisplayNumber : 1,
+    lordsBalanceRatio: lordsBalance < getTotalLords ? lordsBalance / getTotalLords : 1
+  }),
+  [resourceBalance, getsDisplayNumber, lordsBalance, getTotalLords],
+);
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b359932 and 1c85386.

📒 Files selected for processing (27)
  • client/apps/game/src/three/managers/army-manager.ts (1 hunks)
  • client/apps/game/src/three/managers/battle-manager.ts (1 hunks)
  • client/apps/game/src/three/managers/highlight-hex-manager.ts (1 hunks)
  • client/apps/game/src/three/managers/interactive-hex-manager.ts (1 hunks)
  • client/apps/game/src/three/managers/minimap.ts (1 hunks)
  • client/apps/game/src/three/managers/navigator.ts (1 hunks)
  • client/apps/game/src/three/managers/structure-manager.ts (1 hunks)
  • client/apps/game/src/three/scenes/hexagon-scene.ts (1 hunks)
  • client/apps/game/src/three/scenes/hexception.tsx (2 hunks)
  • client/apps/game/src/three/scenes/worldmap.ts (2 hunks)
  • client/apps/game/src/three/utils.ts (1 hunks)
  • client/apps/game/src/ui/components/construction/select-preview-building.tsx (2 hunks)
  • client/apps/game/src/ui/components/military/pillage-history.tsx (1 hunks)
  • client/apps/game/src/ui/components/resources/realm-transfer.tsx (1 hunks)
  • client/apps/game/src/ui/components/resources/travel-info.tsx (1 hunks)
  • client/apps/game/src/ui/components/trading/market-order-panel.tsx (1 hunks)
  • client/apps/game/src/ui/components/worldmap/battles/battle-simulation-panel.tsx (2 hunks)
  • client/apps/game/src/ui/modules/military/battle-view/utils.tsx (2 hunks)
  • client/apps/game/src/ui/modules/stream/event-stream.tsx (2 hunks)
  • client/apps/game/src/ui/modules/world-structures/world-structures-menu.tsx (2 hunks)
  • client/apps/game/src/ui/utils/utils.ts (0 hunks)
  • client/sdk/packages/core/src/utils/battle-simulator.ts (2 hunks)
  • client/sdk/packages/core/src/utils/entities.ts (3 hunks)
  • client/sdk/packages/core/src/utils/index.ts (1 hunks)
  • client/sdk/packages/core/src/utils/realm.ts (1 hunks)
  • client/sdk/packages/core/src/utils/resources.ts (2 hunks)
  • client/sdk/packages/core/src/utils/transport.ts (1 hunks)
💤 Files with no reviewable changes (1)
  • client/apps/game/src/ui/utils/utils.ts
✅ Files skipped from review due to trivial changes (6)
  • client/apps/game/src/three/managers/interactive-hex-manager.ts
  • client/apps/game/src/three/managers/structure-manager.ts
  • client/apps/game/src/three/managers/army-manager.ts
  • client/apps/game/src/three/managers/highlight-hex-manager.ts
  • client/apps/game/src/three/managers/battle-manager.ts
  • client/apps/game/src/three/scenes/hexagon-scene.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Run pr agent on every pull request, respond to user comments
🔇 Additional comments (27)
client/apps/game/src/ui/components/construction/select-preview-building.tsx (2)

Line range hint 9-28: LGTM! Import reorganization looks good.

The import of isResourceProductionBuilding has been correctly moved to @bibliothecadao/eternum while maintaining clean import organization.


Line range hint 353-355: LGTM! Proper usage of isResourceProductionBuilding.

The function is correctly used to determine if a resource icon should be displayed for production buildings.

client/apps/game/src/ui/components/resources/travel-info.tsx (2)

3-3: LGTM! Import statements are well-organized.

The new utility functions and types are properly imported and grouped by their respective sources.

Also applies to: 6-12


Line range hint 39-47: LGTM! Balance calculation is now more precise.

The use of multiplyByPrecision and divideByPrecision utilities along with the getBalance function ensures accurate balance calculations, preventing potential floating-point precision issues.

client/apps/game/src/three/utils.ts (3)

5-17: Well-implemented coordinate conversion function!

The getHexagonCoordinates function correctly handles the conversion between instanced mesh coordinates and hexagonal grid coordinates.


53-59: Well-implemented distance calculation!

The calculateDistanceInHexes function correctly handles the conversion from world distance to hex distance, including proper error handling for invalid distances.


61-78: Well-implemented offset calculation!

The calculateOffset function correctly handles positioning elements around a hexagon, including support for multiple layers when there are more than 6 elements.

client/apps/game/src/three/managers/navigator.ts (1)

7-7: LGTM! Import path updated correctly.

The import path for utility functions has been updated to use the new centralized location.

client/apps/game/src/three/managers/minimap.ts (1)

11-11: LGTM! Import path updated correctly.

The import path for utility functions has been updated to use the new centralized location.

client/apps/game/src/three/scenes/hexception.tsx (1)

23-23: LGTM! Imports organized correctly.

The imports have been properly organized:

  • getEntityIdFromKeys from the eternum package
  • Hexagonal grid utilities from the local utils module

Also applies to: 41-41

client/apps/game/src/three/scenes/worldmap.ts (3)

42-42: LGTM! Import path refactoring looks good.

The relative import path for getWorldPositionForHex aligns with the PR's objective of reorganizing utility functions.


26-31: LGTM! System update types are properly imported.

The imports for ArmySystemUpdate and TileSystemUpdate are correctly added to support typed system updates.


Line range hint 123-127: LGTM! Army system update subscription is properly typed.

The subscription to army updates correctly uses the ArmySystemUpdate type for type safety.

client/sdk/packages/core/src/utils/transport.ts (1)

6-11: LGTM! Clean implementation of donkey capacity calculation.

The function correctly handles the weight-to-donkeys conversion, including proper rounding up to ensure sufficient transport capacity.

client/apps/game/src/ui/components/worldmap/battles/battle-simulation-panel.tsx (1)

Line range hint 56-62: LGTM! Clean migration to BattleSimulator.

The replacement of Battle with BattleSimulator is implemented correctly, maintaining the same interface.

client/sdk/packages/core/src/utils/entities.ts (2)

68-71: LGTM! Clean implementation of realm name formatting.

The function effectively handles the wonder case using template literals.


Line range hint 115-120: LGTM! Good rename to make the function more generic.

The rename from getPlayerAddressFromEntity to getAddressFromEntity better reflects its purpose, as it can be used for any entity owner, not just players.

client/sdk/packages/core/src/utils/index.ts (2)

18-18: LGTM! Consistent naming convention applied.

The change from battleSimulation to battle-simulator aligns with kebab-case naming convention.


26-26: Verify the new transport module's usage.

A new module has been added to handle transport-related functionality.

Let's verify its usage across the codebase:

✅ Verification successful

Transport module integration verified successfully

The transport module is properly integrated into the core SDK's utility functions, containing focused functionality for weight calculations and resource transportation. The module is accessible through the widely-used utils barrel file, making it available where needed across the codebase.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for references to the new transport module
rg -l "from.*['\"].*transport['\"]"

Length of output: 85


Script:

#!/bin/bash
# Check the content of the transport module and look for indirect usage through utils
echo "=== Transport module content ==="
cat client/sdk/packages/core/src/utils/transport.ts || echo "File not found"

echo -e "\n=== Indirect usage through utils ==="
rg -l "from.*['\"].*utils['\"]"

Length of output: 11395

client/apps/game/src/ui/components/military/pillage-history.tsx (1)

34-35: LGTM! Improved function naming clarity.

The change from getPlayerAddressFromEntity to getAddressNameFromEntity better reflects the function's purpose, and the dependency array has been correctly updated.

client/apps/game/src/ui/modules/stream/event-stream.tsx (1)

3-3: LGTM! Consistent with entity address retrieval refactoring.

The change aligns with the broader refactoring of entity address retrieval functions across the codebase.

Also applies to: 56-56

client/apps/game/src/ui/modules/military/battle-view/utils.tsx (1)

4-4: LGTM! Consistent with battle simulation module renaming.

The change from Battle to BattleSimulator is consistent with the module renaming and better reflects the class's purpose.

Also applies to: 99-99

client/apps/game/src/ui/components/resources/realm-transfer.tsx (1)

5-13: LGTM! Import paths have been reorganized.

The utility functions have been correctly moved to @bibliothecadao/eternum, aligning with the PR's objective of consolidating utility functions.

client/apps/game/src/ui/modules/world-structures/world-structures-menu.tsx (1)

18-18: LGTM! Function has been renamed and usage updated.

The getPlayerAddressFromEntity function has been correctly renamed to getAddressFromEntity and its usage in the structureOwner hook has been updated accordingly.

Also applies to: 197-200

client/sdk/packages/core/src/utils/battle-simulator.ts (2)

13-13: LGTM! Class name better reflects its purpose.

The rename from Battle to BattleSimulator provides better clarity about the class's responsibility.


286-362: LGTM! Well-structured class definitions.

The TroopConfig and TroopsSimulator classes are well-organized with clear responsibilities and proper encapsulation.

client/apps/game/src/ui/components/trading/market-order-panel.tsx (1)

Line range hint 423-607: Enhance input validation and transaction security.

Consider implementing these security improvements:

  1. Add input sanitization for all numeric inputs
  2. Implement rate limiting for order creation
  3. Add transaction confirmation timeouts
  4. Consider adding a maximum order size limit

Comment on lines +11 to +26
export const getRealmWithPosition = (entityId: ID, components: ClientComponents) => {
const { Realm, Owner, Position } = components;
const entity = getEntityIdFromKeys([BigInt(entityId)]);
const realm = getComponentValue(Realm, entity);
if (!realm) return undefined;

const position = getComponentValue(Position, entity);
const owner = getComponentValue(Owner, entity);

return {
...realm,
position,
name: getRealmNameById(realm.realm_id),
owner,
} as RealmWithPosition;
};
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

Enhance type safety and validation in getRealmWithPosition.

  1. The type assertion as RealmWithPosition bypasses TypeScript's type checking. Consider creating the object with explicit typing.
  2. The function doesn't validate if position and owner exist before returning.
-  return {
-    ...realm,
-    position,
-    name: getRealmNameById(realm.realm_id),
-    owner,
-  } as RealmWithPosition;
+  const realmWithPosition: RealmWithPosition = {
+    ...realm,
+    position: position ?? undefined,
+    name: getRealmNameById(realm.realm_id),
+    owner: owner ?? undefined,
+  };
+  return realmWithPosition;
📝 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
export const getRealmWithPosition = (entityId: ID, components: ClientComponents) => {
const { Realm, Owner, Position } = components;
const entity = getEntityIdFromKeys([BigInt(entityId)]);
const realm = getComponentValue(Realm, entity);
if (!realm) return undefined;
const position = getComponentValue(Position, entity);
const owner = getComponentValue(Owner, entity);
return {
...realm,
position,
name: getRealmNameById(realm.realm_id),
owner,
} as RealmWithPosition;
};
export const getRealmWithPosition = (entityId: ID, components: ClientComponents) => {
const { Realm, Owner, Position } = components;
const entity = getEntityIdFromKeys([BigInt(entityId)]);
const realm = getComponentValue(Realm, entity);
if (!realm) return undefined;
const position = getComponentValue(Position, entity);
const owner = getComponentValue(Owner, entity);
const realmWithPosition: RealmWithPosition = {
...realm,
position: position ?? undefined,
name: getRealmNameById(realm.realm_id),
owner: owner ?? undefined,
};
return realmWithPosition;
};

@bob0005 bob0005 merged commit 91b2c17 into next Jan 16, 2025
6 of 9 checks passed
@bob0005 bob0005 deleted the raschel/refactor2 branch January 16, 2025 22:03
This was referenced Jan 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants