Add pillage simulation tool #2131
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
WalkthroughThe pull request introduces a pillage simulation feature to the application, enhancing the existing military simulation capabilities. It includes the addition of new components such as Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Failed to generate code suggestions for PR |
![mentatbot[bot]](https://avatars.githubusercontent.com/in/837875?s=60&v=4){kind=small}
There was a problem hiding this comment.
The PR adds a well-implemented pillage simulation feature with good code organization and reuse. The UI is consistent with existing patterns and provides clear feedback. A few suggestions for improvement:
- Some complex calculations could use documentation
- Some code duplication in troop calculations could be extracted
- Error handling could be more robust in a few places
Overall this is a solid addition that maintains code quality standards while adding useful functionality.
Thanks for using MentatBot. Give comments a 👍 or 👎 to help me improve!
client/src/ui/components/worldmap/battles/PillageSimulationPanel.tsx
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (15)
client/src/ui/components/worldmap/battles/PillageSimulationPanel.tsx (3)
65-67: Handle cases when attackers or defenders have no troopsWhen either the attacker or defender has zero troops, the function returns early without providing a default value for
simulationResults, resulting inundefined. While the subsequent conditional rendering handles this case, it might be helpful to explicitly return a default structure or message.Consider returning a default object or adding comments to clarify this behavior.
149-169: Display resource amounts using consistent formattingIn the "Resources Stolen" display, the amount is shown without formatting, which may affect readability. Other numeric values use formatting functions.
Apply consistent formatting to
simulationResults.maxResourceAmountStolen:<div className="text-xl font-bold"> - {simulationResults.maxResourceAmountStolen} + {currencyFormat(Number(simulationResults.maxResourceAmountStolen), 0)} </div>
83-124: Refactor troop loss calculations for clarity and accuracyThe calculations for remaining troops are complex and involve multiple type conversions between
BigIntandNumber. This can be simplified for better readability and to reduce potential errors.Consider creating a utility function to handle troop loss calculations, ensuring consistent logic and simplifying the component code.
client/src/ui/modules/simulation/pillage-simulation.tsx (2)
1-5: Consider standardizing import pathsThe import statements mix absolute paths (starting with "@/") and relative paths (starting with "../"). Consider standardizing all imports to use absolute paths for better maintainability and to avoid fragile relative paths.
-import useUIStore from "../../../hooks/store/useUIStore"; -import { pillageSimulation } from "../../components/navigation/Config"; -import { OSWindow } from "../../components/navigation/OSWindow"; +import useUIStore from "@/hooks/store/useUIStore"; +import { pillageSimulation } from "@/ui/components/navigation/Config"; +import { OSWindow } from "@/ui/components/navigation/OSWindow";
13-22: Consider making the window width responsiveThe hard-coded width of "600px" might not provide the best experience across different screen sizes. Consider:
- Using relative units (e.g., rem, em)
- Implementing responsive breakpoints
- Using min/max width constraints
- width="600px" + width="min(600px, 90vw)"Also, consider adding TypeScript interfaces for better type safety:
interface PillageSimulationProps { // Add any future props here } export const PillageSimulation: React.FC<PillageSimulationProps> = () => { // ... existing implementation };client/src/ui/components/navigation/Config.tsx (1)
12-13: Well-structured integration of the new feature.The addition of pillage simulation to the navigation system follows a clean, modular approach that maintains consistency with the existing architecture. This ensures the new feature integrates seamlessly with the existing battle simulation functionality.
Also applies to: 39-39
client/src/ui/components/worldmap/battles/Troops.tsx (2)
16-16: Add ARIA attributes for better accessibility.The hover effect should be accompanied by proper ARIA attributes to improve accessibility.
-<div className={`p-2 bg-gold/10 hover:bg-gold/30 `} key={resource}> +<div + className={`p-2 bg-gold/10 hover:bg-gold/30`} + key={resource} + role="listitem" + aria-label={`${getDisplayResourceName(resource as keyof typeof ResourcesIds)} troops`} +>
6-46: Consider performance optimizations.For better performance with large troop counts:
- Memoize the component to prevent unnecessary re-renders
- Consider using virtualization for large troop lists
+import { memo } from 'react'; + -export const Troops = ({ +export const Troops = memo(({ troops, setTroops, }: { troops: Partial<Record<ResourcesIds, bigint>>; setTroops?: React.Dispatch<React.SetStateAction<Partial<Record<ResourcesIds, bigint>>>>; -}) => { +}) => { // ... component implementation ... -}; +});If the troop list grows large, consider using a virtualization library like
react-windowfor better performance.client/src/ui/components/military/EntitiesArmyTable.tsx (1)
26-28: Consider UI/UX improvements and component optimization.
UI/UX Suggestions:
- Add visual distinction between battle and pillage simulation buttons
- Include tooltips to explain the difference between the two simulation types
Performance Optimization:
- Consider lazy loading the simulation components
- Consider conditional rendering based on which simulation is active
Example implementation:
+ import { Tooltip } from "@/ui/elements/Tooltip"; + import { lazy, Suspense } from "react"; + const LazyBattleSimulation = lazy(() => import("@/ui/modules/simulation/BattleSimulation")); + const LazyPillageSimulation = lazy(() => import("@/ui/modules/simulation/pillage-simulation")); return ( <> <div className="w-full flex justify-center mt-4"> - <Button variant="primary" className="mx-auto" size="md" onClick={() => togglePopup(battleSimulation)}> - Simulate a battle - </Button> - <Button variant="primary" className="mx-auto" size="md" onClick={() => togglePopup(pillageSimulation)}> - Simulate a pillage - </Button> + <Tooltip content="Simulate combat between armies"> + <Button + variant="primary" + className="mx-auto" + size="md" + onClick={() => togglePopup(battleSimulation)} + > + Simulate a battle + </Button> + </Tooltip> + <Tooltip content="Simulate resource pillaging from territories"> + <Button + variant="secondary" + className="mx-auto" + size="md" + onClick={() => togglePopup(pillageSimulation)} + > + Simulate a pillage + </Button> + </Tooltip> </div> - <BattleSimulation /> - <PillageSimulation /> + <Suspense fallback={<div>Loading simulation...</div>}> + {activeSimulation === 'battle' && <LazyBattleSimulation />} + {activeSimulation === 'pillage' && <LazyPillageSimulation />} + </Suspense>Also applies to: 31-31
client/src/ui/modules/military/battle-view/utils.tsx (2)
7-14: LGTM! Consider adding JSDoc documentation.The
ArmyBattleInfotype is well-structured and uses appropriate types for game mechanics. Consider adding JSDoc documentation to describe the purpose and usage of this type.Add documentation like this:
+/** + * Represents an army's battle information including troop counts and health status. + * @property troops - The counts of different troop types in the army + * @property health - The current and maximum health values of the army + */ type ArmyBattleInfo = {
51-56: Consider adding input validation for negative values.The function handles the zero case correctly, but consider adding validation for negative values to ensure robust error handling.
Consider this enhancement:
function percentageLeft(health: { current: bigint; lifetime: bigint }): bigint { + if (health.current < 0n || health.lifetime < 0n) { + throw new Error('Health values cannot be negative'); + } if (health.lifetime === 0n) { return 0n; } return (health.current * BigInt(Percentage._100())) / health.lifetime; }client/src/ui/components/worldmap/battles/BattleSimulationPanel.tsx (2)
84-93: Consider extracting common styles into reusable classesThe layout structure is well-organized, but consider extracting repeated styling classes (e.g., "text-center", "text-xl font-bold mb-4") into reusable Tailwind components or CSS classes for better maintainability.
Example approach:
+ // In a separate styles file + const battlePanelStyles = { + container: "w-full mb-4 p-6 rounded-lg shadow-lg", + grid: "grid grid-cols-2 gap-8", + heading: "text-xl font-bold mb-4", + }; - <div className="w-full mb-4 p-6 rounded-lg shadow-lg"> + <div className={battlePanelStyles.container}>
100-105: Enhance accessibility for battle duration displayConsider adding ARIA labels and roles to improve accessibility for screen readers.
- <div className="bg-black rounded-lg p-3"> + <div className="bg-black rounded-lg p-3" role="status" aria-label="Battle duration status"> - <div className="text-sm">Battle Duration</div> + <div className="text-sm" id="duration-label">Battle Duration</div> - <div className="text-xl font-bold">⏳ {formatTime(battle?.calculateDuration() ?? 0)}</div> + <div className="text-xl font-bold" aria-labelledby="duration-label"> + ⏳ {formatTime(battle?.calculateDuration() ?? 0)} + </div>client/src/ui/modules/military/battle-view/BattleActions.tsx (2)
249-255: Consider making the army loss percentage configurable.The 25% army loss value is hardcoded in the warning message. This value should be configurable to accommodate future balance changes.
Consider extracting this value to a configuration constant or getting it from the game's configuration:
+ const RAID_ARMY_LOSS_PERCENTAGE = 25; // Move to game config or constants file if (raidStatus !== RaidStatus.isRaidable) { setTooltip({ content: <div className="">{raidStatus}</div>, position: "top" }); } else if (selectedArmy?.battle_id !== 0) { - content.push(<div>Raiding will make you leave and lose 25% of your army</div>); + content.push(<div key="warning">Raiding will make you leave and lose {RAID_ARMY_LOSS_PERCENTAGE}% of your army</div>); }
Line range hint
94-106: Enhance error feedback for users during raid operations.The error handling for raid operations only logs to console. Consider providing user-friendly error messages through the UI.
Consider enhancing the error handling:
try { await battleManager.pillageStructure(selectedArmy!, structure!.entity_id); toggleModal( <ModalContainer size="half"> <Headline>Pillage History</Headline> <PillageHistory structureId={structure!.entity_id} /> </ModalContainer>, ); setBattleView(null); setView(LeftView.None); } catch (error) { console.error("Error during pillage:", error); + toggleModal( + <ModalContainer size="half"> + <Headline>Pillage Failed</Headline> + <div className="text-red"> + {error instanceof Error ? error.message : "Failed to execute pillage operation"} + </div> + </ModalContainer>, + ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
client/src/ui/components/military/EntitiesArmyTable.tsx(2 hunks)client/src/ui/components/navigation/Config.tsx(2 hunks)client/src/ui/components/worldmap/battles/BattleSimulationPanel.tsx(4 hunks)client/src/ui/components/worldmap/battles/PillageSimulationPanel.tsx(1 hunks)client/src/ui/components/worldmap/battles/Troops.tsx(1 hunks)client/src/ui/modules/military/battle-view/BattleActions.tsx(2 hunks)client/src/ui/modules/military/battle-view/utils.tsx(4 hunks)client/src/ui/modules/simulation/pillage-simulation.tsx(1 hunks)sdk/packages/eternum/src/utils/battleSimulation.ts(2 hunks)
🧰 Additional context used
🪛 Biome
client/src/ui/modules/military/battle-view/BattleActions.tsx
[error] 229-229: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
[error] 230-230: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
[error] 234-234: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
[error] 238-238: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
[error] 242-242: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
🔇 Additional comments (13)
client/src/ui/components/worldmap/battles/PillageSimulationPanel.tsx (1)
39-134: Improve dependency management in simulationResults calculation
The useMemo hook for simulationResults depends on troopConfig, which may not update if troopConfig itself doesn't re-compute due to an empty dependency array.
Please ensure that troopConfig updates correctly when its underlying data changes. Run the following script to verify whether troopConfig updates when expected:
If the verification shows that troopConfig may become stale, consider adjusting the dependencies.
client/src/ui/modules/simulation/pillage-simulation.tsx (2)
7-11: LGTM! Clean hook implementation
The component follows React best practices by:
- Using separate selectors for state management
- Keeping the state logic focused and simple
1-23: Verify integration with existing battle simulation
Since this is part of a larger feature adding pillage simulation alongside existing battle simulation, let's verify the consistency of the implementation.
✅ Verification successful
Let me try different commands to verify the integration patterns.
Let me search for more specific patterns to verify the battle simulation implementation.
Implementation is consistent with battle simulation patterns
The verification confirms that the pillage simulation implementation follows the same patterns as the battle simulation:
- Both use the same
OSWindowcomponent with identical props structure - Both use
useUIStorefor managing popup state consistently - Both correctly use
HintSection.Combatfor combat-related hints - Both have similarly structured simulation panels with consistent troop management and result displays
The implementation maintains consistency while adding new pillage-specific functionality like success chance and resource stealing calculations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for similar patterns in battle simulation implementation
echo "Checking for battle simulation patterns..."
rg -l "BattleSimulation" --type tsx
# Verify consistent hint section usage
echo "Verifying hint section usage..."
ast-grep --pattern 'hintSection={HintSection.Combat}'
Length of output: 287
Script:
#!/bin/bash
# Search for battle simulation files with broader extension patterns
echo "Checking for battle simulation patterns..."
rg -l "BattleSimulation" --type ts --type tsx --type jsx --type js
# Look for similar window component usage patterns
echo "Looking for OSWindow usage patterns..."
rg "OSWindow.*title.*hintSection" -A 2
# Check hint section imports and usage
echo "Checking hint section usage..."
rg "HintSection\.Combat"
Length of output: 1003
Script:
#!/bin/bash
# Search for BattleSimulation component implementation
echo "Checking BattleSimulation component..."
fd -e tsx -e ts -e js BattleSimulation --exec cat {}
# Look for similar UI store usage patterns
echo "Checking UI store usage patterns..."
rg "useUIStore.*isPopupOpen" -A 2
# Check for similar simulation panel patterns
echo "Looking for simulation panel components..."
fd -e tsx -e ts SimulationPanel --exec cat {}
Length of output: 20186
client/src/ui/components/navigation/Config.tsx (2)
12-13: LGTM! Type definition follows existing patterns.
The addition of "PillageSimulation" to the OSWindows type is consistent with the existing type structure and naming conventions.
39-39: LGTM! Verify constant usage across components.
The pillageSimulation constant declaration follows the established patterns and type safety conventions.
Let's verify the constant's usage across the components mentioned in the PR objectives:
✅ Verification successful
Constant is properly integrated and used consistently
The pillageSimulation constant is correctly imported and used in the expected components:
- Used in
pillage-simulation.tsxfor popup state management and toggle functionality - Used in
EntitiesArmyTable.tsxfor button click handling - Follows the same pattern as other
OSWindowsconstants inConfig.tsx
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of pillageSimulation constant across components
# Expected: Usage in EntitiesArmyTable.tsx, PillageSimulationPanel.tsx, and pillage-simulation.tsx
# Search for pillageSimulation imports and usage
rg -l "pillageSimulation" "client/src/ui"
# Search for specific component implementations using this constant
ast-grep --pattern 'import { $$$, pillageSimulation, $$$ } from "$_"'
Length of output: 270
Script:
#!/bin/bash
# Let's check how pillageSimulation is actually used in these files
rg "pillageSimulation" "client/src/ui/modules/simulation/pillage-simulation.tsx" -A 2
rg "pillageSimulation" "client/src/ui/components/military/EntitiesArmyTable.tsx" -A 2
# Also check if there are any other window-related constants being used similarly
ast-grep --pattern 'export const $_: OSWindows = "$_";'
Length of output: 1761
client/src/ui/components/worldmap/battles/Troops.tsx (2)
1-12: LGTM! Well-structured type definitions and imports.
The type definitions are clear and appropriate, with good use of TypeScript features like Partial for the troops mapping.
33-38: 🛠️ Refactor suggestion
Add validation and make step size configurable.
The current implementation has several potential improvements:
- The step size is hardcoded
- Missing maximum value validation
- Potential precision loss in BigInt conversion
Let's verify the usage of these troop numbers in the codebase:
Consider these improvements:
<NumberInput
min={0}
- step={100}
+ step={props.stepSize ?? 100}
+ max={Number.MAX_SAFE_INTEGER}
value={Number(count)}
- onChange={(amount) => setTroops({ ...troops, [resource]: BigInt(amount) })}
+ onChange={(amount) => {
+ if (amount > Number.MAX_SAFE_INTEGER) {
+ console.warn('Troop count exceeds safe integer limit');
+ return;
+ }
+ setTroops({ ...troops, [resource]: BigInt(amount) });
+ }}
/>client/src/ui/components/military/EntitiesArmyTable.tsx (2)
8-9: LGTM! Import changes are well-structured.
The new imports for pillage simulation functionality are properly organized and follow the existing naming conventions.
Also applies to: 13-13
8-9: Verify integration with dependent components.
Let's ensure that all required components and configurations are properly set up.
Also applies to: 13-13
✅ Verification successful
Integration is properly structured and verified
The verification shows that all components are properly integrated:
- PillageSimulation component exists and is correctly structured with proper UI store integration
- PillageSimulationPanel component is fully implemented with battle simulation logic
- BattleSimulation component is properly structured
- Both components are correctly imported in EntitiesArmyTable.tsx
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if all required components exist and are properly structured
# Verify the existence and structure of pillage simulation component
echo "Checking pillage simulation component..."
ast-grep --pattern 'export const PillageSimulation = () => {
$$$
}'
# Verify Config.tsx contains required configuration
echo "Checking configuration..."
ast-grep --pattern 'export const pillageSimulation: OSWindow = {
$$$
}'
# Check for any potential circular dependencies
echo "Checking for circular dependencies..."
rg -l "from.*pillage-simulation" | xargs -I {} rg -l "from.*EntitiesArmyTable" {}
Length of output: 1912
Script:
#!/bin/bash
# Let's verify the Config.tsx with a different pattern and check other integrations
# Check for pillageSimulation configuration
echo "Checking pillageSimulation configuration..."
rg "pillageSimulation.*=.*{" -A 5
# Check for imports and usage of PillageSimulation
echo "Checking PillageSimulation usage..."
rg "import.*PillageSimulation" -l
# Check for PillageSimulationPanel component
echo "Checking PillageSimulationPanel component..."
ast-grep --pattern 'export const PillageSimulationPanel = () => {
$$$
}'
# Check if BattleSimulation is properly structured (since it's also imported)
echo "Checking BattleSimulation component..."
ast-grep --pattern 'export const BattleSimulation = () => {
$$$
}'
Length of output: 21627
client/src/ui/modules/military/battle-view/utils.tsx (2)
16-18: LGTM! Type signature update is consistent.
The function signature update to use ArmyBattleInfo is appropriate and maintains type safety while preserving the existing battle mechanics.
69-70: Verify the impact of health loss calculation changes.
The removal of resourceMultiplier division from health loss calculations might affect game balance. Please verify that this change is intentional and aligns with the game design.
Let's check for any related configuration or balance discussions:
Also applies to: 73-73, 94-94, 99-99
client/src/ui/components/worldmap/battles/BattleSimulationPanel.tsx (1)
Line range hint 65-81: LGTM! Clean implementation of remaining troops calculation
The code properly handles null cases and uses appropriate typing with ResourcesIds.
sdk/packages/eternum/src/utils/battleSimulation.ts (1)
63-69: LGTM! Clean implementation of troops getter method.
The method provides a well-structured interface for accessing troop counts, which aligns with the pillage simulation requirements while maintaining good encapsulation practices.
client/src/ui/components/worldmap/battles/PillageSimulationPanel.tsx
Outdated
Show resolved
Hide resolved
client/src/ui/components/worldmap/battles/PillageSimulationPanel.tsx
Outdated
Show resolved
Hide resolved
client/src/ui/components/worldmap/battles/PillageSimulationPanel.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
client/src/ui/modules/military/battle-view/utils.tsx (2)
7-14: LGTM! Consider adding 'I' prefix for interface types.The new
ArmyBattleInfotype properly encapsulates the army data structure with explicit typing. Consider following TypeScript naming conventions by using an 'I' prefix for interface types (e.g.,IArmyBattleInfo).
106-114: Extract repeated troop loss calculation logic.The troop loss calculation formula is repeated for each troop type. Consider extracting this into a helper function to improve maintainability and reduce code duplication.
+const calculateTroopLoss = (troopCount: bigint, healthLoss: number, lifetimeHealth: bigint): number => { + return Math.ceil((Number(troopCount) * healthLoss) / Number(lifetimeHealth)); +}; const attackerKnightLost = calculateTroopLoss( attackerArmy.troops.knight_count, attackerHealthLoss, attackerArmy.health.lifetime );Also applies to: 122-130
client/src/ui/components/worldmap/battles/PillageSimulationPanel.tsx (2)
41-137: Consider extracting simulation logic into a separate functionThe
simulationResultsuseMemo hook contains complex logic for calculating simulation outcomes. Extracting this logic into a separate function or custom hook can improve readability and maintainability.Apply this refactor:
-export const PillageSimulationPanel = () => { +const calculateSimulationResults = ( + attackingTroopsNumber: Partial<Record<ResourcesIds, bigint>>, + defendingTroopsNumber: Partial<Record<ResourcesIds, bigint>>, + troopConfig: TroopConfigType, +) => { + // ...current simulation logic... +}; + +export const PillageSimulationPanel = () => { // ... const simulationResults = useMemo(() => { if (!troopConfig) return; - // ...current simulation logic... + return calculateSimulationResults(attackingTroopsNumber, defendingTroopsNumber, troopConfig); }, [attackingTroopsNumber, defendingTroopsNumber, troopConfig]); // ... };
93-105: Optimize repeated calls toconfigManager.getResourcePrecision()The
configManager.getResourcePrecision()function is called multiple times. Storing its value in a constant can improve performance and readability.Apply this change:
+ const resourcePrecision = configManager.getResourcePrecision(); const attackerRemainingTroops = { [ResourcesIds.Crossbowman]: roundDownToPrecision( attackerArmyInfo.troops.crossbowman_count - BigInt(attackerCrossbowmanLost), - configManager.getResourcePrecision(), + resourcePrecision, ), [ResourcesIds.Knight]: roundDownToPrecision( attackerArmyInfo.troops.knight_count - BigInt(attackerKnightLost), - configManager.getResourcePrecision(), + resourcePrecision, ), [ResourcesIds.Paladin]: roundDownToPrecision( attackerArmyInfo.troops.paladin_count - BigInt(attackerPaladinLost), - configManager.getResourcePrecision(), + resourcePrecision, ), }; const defenderRemainingTroops = { [ResourcesIds.Crossbowman]: roundDownToPrecision( defenderArmyInfo.troops.crossbowman_count - BigInt(defenderCrossbowmanLost), - configManager.getResourcePrecision(), + resourcePrecision, ), [ResourcesIds.Knight]: roundDownToPrecision( defenderArmyInfo.troops.knight_count - BigInt(defenderKnightLost), - configManager.getResourcePrecision(), + resourcePrecision, ), [ResourcesIds.Paladin]: roundDownToPrecision( defenderArmyInfo.troops.paladin_count - BigInt(defenderPaladinLost), - configManager.getResourcePrecision(), + resourcePrecision, ), };Also applies to: 107-119
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
client/src/ui/components/worldmap/battles/PillageSimulationPanel.tsx(1 hunks)client/src/ui/modules/military/battle-view/utils.tsx(3 hunks)
🧰 Additional context used
🪛 Biome
client/src/ui/modules/military/battle-view/utils.tsx
[error] 68-68: Don't use 'Number' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'number' instead
(lint/complexity/noBannedTypes)
[error] 72-72: Don't use 'Number' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'number' instead
(lint/complexity/noBannedTypes)
🔇 Additional comments (2)
client/src/ui/modules/military/battle-view/utils.tsx (1)
100-105: Document and optimize health loss calculations.
The health loss calculations involve multiple divisions that could be optimized and need documentation.
Consider:
- Combining the resource precision divisions:
- const attackerHealthLoss = Math.floor(
- (defenceDelta * duration) /
- troopConfig.pillage_health_divisor /
- configManager.getResourcePrecision() /
- configManager.getResourcePrecision(),
- );
+ const combinedDivisor = troopConfig.pillage_health_divisor *
+ Math.pow(configManager.getResourcePrecision(), 2);
+ const attackerHealthLoss = Math.floor((defenceDelta * duration) / combinedDivisor);- Adding documentation to explain the formula and its purpose, as suggested in the previous review.
Also applies to: 116-121
client/src/ui/components/worldmap/battles/PillageSimulationPanel.tsx (1)
152-190: UI rendering logic is clear and user-friendly
The conditional rendering of simulation results and the structured layout provide a good user experience. The component is well-organized, and the visual hierarchy is intuitive.
client/src/ui/components/worldmap/battles/PillageSimulationPanel.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
client/src/ui/modules/military/battle-view/utils.tsx (1)
7-23: Consider adding validation constraints for troop counts.The types are well-structured, but consider adding runtime validation to ensure troop counts are non-negative.
type ArmyBattleInfo = { troops: { knight_count: bigint; paladin_count: bigint; crossbowman_count: bigint; }; health: { current: bigint; lifetime: bigint }; // Add validation method validate(): boolean { return this.troops.knight_count >= 0n && this.troops.paladin_count >= 0n && this.troops.crossbowman_count >= 0n && this.health.current >= 0n && this.health.lifetime >= 0n; } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
client/src/ui/modules/military/battle-view/utils.tsx(3 hunks)
🧰 Additional context used
🪛 Biome
client/src/ui/modules/military/battle-view/utils.tsx
[error] 77-77: Don't use 'Number' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'number' instead
(lint/complexity/noBannedTypes)
[error] 81-81: Don't use 'Number' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'number' instead
(lint/complexity/noBannedTypes)
🔇 Additional comments (3)
client/src/ui/modules/military/battle-view/utils.tsx (3)
77-83: LGTM with existing review comments.
The implementation looks correct. Please address the type declarations and precision loss concerns raised in the previous review.
🧰 Tools
🪛 Biome
[error] 77-77: Don't use 'Number' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'number' instead
(lint/complexity/noBannedTypes)
[error] 81-81: Don't use 'Number' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'number' instead
(lint/complexity/noBannedTypes)
159-181: LGTM! Clean implementation of troop loss aggregation.
The function correctly handles rounding and aggregation of losses for each troop type.
117-122: 🛠️ Refactor suggestion
Consider precision loss in health calculations.
The multiple divisions by configManager.getResourcePrecision() could lead to significant precision loss. Consider combining the divisions:
- const attackerHealthLoss = Math.floor(
- (defenceDelta * duration) /
- troopConfig.pillage_health_divisor /
- configManager.getResourcePrecision() /
- configManager.getResourcePrecision(),
- );
+ const resourcePrecisionSquared = configManager.getResourcePrecision() ** 2;
+ const attackerHealthLoss = Math.floor(
+ (defenceDelta * duration) /
+ (troopConfig.pillage_health_divisor * resourcePrecisionSquared),
+ );Also applies to: 133-138
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (7)
client/src/ui/modules/military/battle-view/utils.tsx (4)
68-76: Consider adding documentation for the resource stealing formula.While the implementation is correct, adding JSDoc comments explaining the formula and its parameters would improve maintainability.
110-115: Document the health loss calculation formula.The health loss calculation involves multiple divisions by precision and a pillage health divisor. Consider adding comments explaining this formula and its purpose.
Also applies to: 126-131
116-124: Consider refactoring repeated troop loss calculations.The troop loss calculation pattern is repeated for each troop type. Consider extracting this into a helper function to reduce code duplication.
+const calculateTroopLoss = (troopCount: bigint, healthLoss: number, lifetime: bigint): number => { + return Math.ceil((Number(troopCount) * healthLoss) / Number(lifetime)); +}; -const attackerKnightLost = Math.ceil( - (Number(attackerArmy.troops.knight_count) * Number(attackerHealthLoss)) / Number(attackerArmy.health.lifetime), -); +const attackerKnightLost = calculateTroopLoss( + attackerArmy.troops.knight_count, + attackerHealthLoss, + attackerArmy.health.lifetime +);Also applies to: 132-140
165-172: Consider consolidating troop loss calculations.The final troop loss calculations involve multiple similar operations. Consider using
Array.reduceto make this more concise and maintainable.-const attackerTroopsLoss = - roundUpToPrecision(BigInt(attackerKnightLost), configManager.getResourcePrecision()) + - roundUpToPrecision(BigInt(attackerPaladinLost), configManager.getResourcePrecision()) + - roundUpToPrecision(BigInt(attackerCrossbowmanLost), configManager.getResourcePrecision()); +const attackerTroopsLoss = [attackerKnightLost, attackerPaladinLost, attackerCrossbowmanLost] + .reduce((acc, loss) => acc + roundUpToPrecision(BigInt(loss), configManager.getResourcePrecision()), 0n);client/src/ui/components/worldmap/battles/PillageSimulationPanel.tsx (3)
27-36: Consider using a type-safe constant for default troops.Extract the default troops configuration into a named constant to improve maintainability and reusability.
+const DEFAULT_TROOP_COUNTS: Record<ResourcesIds, bigint> = { + [ResourcesIds.Crossbowman]: 0n, + [ResourcesIds.Knight]: 0n, + [ResourcesIds.Paladin]: 0n, +} as const; -const defaultTroops = { - [ResourcesIds.Crossbowman]: 0n, - [ResourcesIds.Knight]: 0n, - [ResourcesIds.Paladin]: 0n, -};
65-67: Enhance error handling for edge cases.The current error handling only checks for zero troops. Consider adding validation for:
- Negative troop numbers
- Numbers exceeding maximum allowed values
- Invalid troop configurations
-if (attacker.count() === 0n || defender.count() === 0n) { +if (!this.isValidTroopConfiguration(attacker, defender)) { return; } +private isValidTroopConfiguration(attacker: TroopsSimulator, defender: TroopsSimulator): boolean { + if (attacker.count() === 0n || defender.count() === 0n) { + return false; + } + // Add additional validation logic + return true; +}
137-192: Enhance accessibility and error states.The UI could benefit from improved accessibility and more informative error states:
- Add ARIA labels for better screen reader support
- Provide more detailed feedback when simulation cannot be performed
-<div className="w-full mb-4 p-6 rounded-lg shadow-lg"> +<div + className="w-full mb-4 p-6 rounded-lg shadow-lg" + role="region" + aria-label="Pillage Simulation Panel" +> -<div className="text-center mt-8">Please select troops to simulate battle</div> +<div + className="text-center mt-8 text-yellow-500" + role="alert" +> + {!attackingTroopsNumber || !defendingTroopsNumber + ? "Please select troops for both sides to simulate battle" + : "Unable to perform simulation with current troop configuration" + } +</div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
client/src/ui/components/worldmap/battles/PillageSimulationPanel.tsx(1 hunks)client/src/ui/modules/military/battle-view/utils.tsx(3 hunks)client/src/ui/utils/utils.tsx(1 hunks)
🧰 Additional context used
🪛 Biome
client/src/ui/utils/utils.tsx
[error] 62-62: Don't use 'Number' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'number' instead
(lint/complexity/noBannedTypes)
[error] 66-66: Don't use 'Number' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'number' instead
(lint/complexity/noBannedTypes)
🔇 Additional comments (5)
client/src/ui/modules/military/battle-view/utils.tsx (3)
8-24: LGTM! Well-structured type definitions.
The new ArmyBattleInfo type and TroopLosses interface are well-defined with appropriate types and follow TypeScript best practices.
61-66: LGTM! Proper handling of edge cases.
The health percentage calculation correctly handles the case when lifetime is zero and uses appropriate bigint operations.
Line range hint 26-44: Consider handling potential precision loss in strength calculations.
The conversion from bigint to number in the strength calculations could lead to precision loss for large armies.
Let's verify the maximum possible values:
client/src/ui/components/worldmap/battles/PillageSimulationPanel.tsx (1)
1-19: LGTM! Imports are well-organized and necessary.
The imports are properly structured and include all required dependencies for the component's functionality.
client/src/ui/utils/utils.tsx (1)
62-68: Verify usage in pillage simulation
These rounding functions are critical for the pillage simulation feature. Let's verify their usage to ensure consistent behavior.
✅ Verification successful
Based on the search results, I can now generate the final response:
Rounding functions are correctly used in pillage simulation
The rounding functions are used consistently in PillageSimulationPanel.tsx for both attacker and defender troop calculations:
roundDownToPrecisionis used for calculating remaining troops after battleroundUpToPrecisionis used for calculating troop losses
The implementation ensures consistent precision handling across all pillage-related calculations, maintaining numerical accuracy when dealing with bigint troop counts.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of the rounding functions in pillage-related code
# Search for function usage in pillage simulation files
echo "Searching for roundDownToPrecision usage:"
rg -A 3 "roundDownToPrecision" --glob "**/*pillage*.{ts,tsx}"
echo -e "\nSearching for roundUpToPrecision usage:"
rg -A 3 "roundUpToPrecision" --glob "**/*pillage*.{ts,tsx}"
# Search for potential similar rounding implementations
echo -e "\nChecking for similar rounding implementations:"
ast-grep --pattern 'BigInt($$$)' --glob "**/*pillage*.{ts,tsx}"
Length of output: 726
Script:
#!/bin/bash
# Let's try a broader search to find any usage of these functions and pillage-related code
# Search for function usage across all TypeScript/React files
echo "Searching for roundDownToPrecision usage:"
rg -A 3 "roundDownToPrecision"
echo -e "\nSearching for roundUpToPrecision usage:"
rg -A 3 "roundUpToPrecision"
# Search for files containing 'pillage' to understand the feature scope
echo -e "\nSearching for pillage-related files:"
fd "pillage" --type f
# Search for BigInt operations in the codebase
echo -e "\nChecking for BigInt operations:"
rg "BigInt\(" -A 2
Length of output: 91610
🧰 Tools
🪛 Biome
[error] 62-62: Don't use 'Number' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'number' instead
(lint/complexity/noBannedTypes)
[error] 66-66: Don't use 'Number' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'number' instead
(lint/complexity/noBannedTypes)
client/src/ui/components/worldmap/battles/PillageSimulationPanel.tsx
Outdated
Show resolved
Hide resolved
client/src/ui/utils/utils.tsx
Outdated
| export function roundDownToPrecision(value: bigint, precision: Number) { | ||
| return BigInt(Number(value) - (Number(value) % Number(precision))); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix type declaration and add safety checks
The function has the following issues:
- Uses 'Number' type instead of 'number'
- Lacks safety checks for large numbers that could lose precision
- Missing documentation
Apply this diff to improve the implementation:
-export function roundDownToPrecision(value: bigint, precision: Number) {
+/**
+ * Rounds down a bigint to the nearest multiple of the precision factor
+ * @param value The value to round down
+ * @param precisionFactor The precision factor to round to
+ * @throws {Error} If the value exceeds safe integer bounds
+ * @returns The rounded down value as a bigint
+ */
+export function roundDownToPrecision(value: bigint, precisionFactor: number) {
+ if (value > BigInt(Number.MAX_SAFE_INTEGER)) {
+ throw new Error('Value exceeds safe integer bounds');
+ }
return BigInt(Number(value) - (Number(value) % Number(precision)));
}Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome
[error] 62-62: Don't use 'Number' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'number' instead
(lint/complexity/noBannedTypes)
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
client/src/ui/components/worldmap/battles/PillageSimulationPanel.tsx(1 hunks)client/src/ui/components/worldmap/battles/Troops.tsx(1 hunks)client/src/ui/modules/military/battle-view/BattleActions.tsx(2 hunks)client/src/ui/modules/military/battle-view/utils.tsx(3 hunks)client/src/ui/utils/utils.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- client/src/ui/components/worldmap/battles/PillageSimulationPanel.tsx
- client/src/ui/components/worldmap/battles/Troops.tsx
- client/src/ui/modules/military/battle-view/BattleActions.tsx
- client/src/ui/utils/utils.tsx
🔇 Additional comments (3)
client/src/ui/modules/military/battle-view/utils.tsx (3)
8-24: LGTM! Well-structured type definitions.
The new ArmyBattleInfo type and TroopLosses interface are well-defined with appropriate types and consistent naming.
152-175: LGTM! Clean implementation of troop loss aggregation.
The function properly aggregates individual troop losses with appropriate precision rounding.
27-28: Verify function signature changes across the codebase.
The type changes from ArmyInfo to ArmyBattleInfo in function signatures could affect other components.
Also applies to: 69-70, 78-80
| const attackerHealthLoss = Math.floor( | ||
| (defenceDelta * duration) / | ||
| troopConfig.pillage_health_divisor / | ||
| configManager.getResourcePrecision() / | ||
| configManager.getResourceMultiplier(); | ||
| const attackerTroopsLoss = | ||
| (getTotalTroops(attackerArmy.troops) * attackerHealthLoss) / Number(attackerArmy.health.lifetime); | ||
| troopConfig.pillage_health_divisor / | ||
| configManager.getResourcePrecision() / | ||
| configManager.getResourcePrecision(), | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Document health loss calculations and review precision handling.
The health loss calculations involve multiple divisions that could lead to excessive rounding:
- Division by
pillage_health_divisor - Two consecutive divisions by
resourcePrecision
Consider:
- Adding documentation explaining the formula's purpose and the role of each divisor
- Combining the precision divisions to minimize rounding errors
+ // Health loss is calculated by:
+ // 1. Multiplying attack/defense delta by battle duration
+ // 2. Dividing by pillage_health_divisor to scale down the impact
+ // 3. Adjusting for resource precision to maintain consistent units
const attackerHealthLoss = Math.floor(
(defenceDelta * duration) /
- troopConfig.pillage_health_divisor /
- configManager.getResourcePrecision() /
- configManager.getResourcePrecision(),
+ (troopConfig.pillage_health_divisor *
+ configManager.getResourcePrecision() *
+ configManager.getResourcePrecision()),
);Also applies to: 126-131
| const attackerKnightLost = Math.ceil( | ||
| (Number(attackerArmy.troops.knight_count) * Number(attackerHealthLoss)) / Number(attackerArmy.health.lifetime), | ||
| ); | ||
| const attackerPaladinLost = Math.ceil( | ||
| (Number(attackerArmy.troops.paladin_count) * Number(attackerHealthLoss)) / Number(attackerArmy.health.lifetime), | ||
| ); | ||
| const attackerCrossbowmanLost = Math.ceil( | ||
| (Number(attackerArmy.troops.crossbowman_count) * Number(attackerHealthLoss)) / Number(attackerArmy.health.lifetime), | ||
| ); |
There was a problem hiding this comment.
Add safety checks for numeric conversions.
The troop loss calculations involve multiple conversions between bigint and number types, which could lead to precision loss for large values.
Add safety checks:
+ const MAX_SAFE_TROOPS = Number.MAX_SAFE_INTEGER / 1000; // Buffer for calculations
+
const attackerKnightLost = Math.ceil(
+ // Verify values are within safe range
+ Number(attackerArmy.troops.knight_count) > MAX_SAFE_TROOPS
+ ? MAX_SAFE_TROOPS
+ : (Number(attackerArmy.troops.knight_count) * Number(attackerHealthLoss)) /
Number(attackerArmy.health.lifetime),
);Also applies to: 132-140
| export const calculateRemainingTroops = ( | ||
| armyInfo: { troops: { crossbowman_count: bigint; knight_count: bigint; paladin_count: bigint } }, | ||
| troopLosses: { crossbowmanLost: number; knightLost: number; paladinLost: number }, | ||
| ) => { | ||
| return { | ||
| [ResourcesIds.Crossbowman]: roundDownToPrecision( | ||
| armyInfo.troops.crossbowman_count - BigInt(troopLosses.crossbowmanLost), | ||
| configManager.getResourcePrecision(), | ||
| ), | ||
| [ResourcesIds.Knight]: roundDownToPrecision( | ||
| armyInfo.troops.knight_count - BigInt(troopLosses.knightLost), | ||
| configManager.getResourcePrecision(), | ||
| ), | ||
| [ResourcesIds.Paladin]: roundDownToPrecision( | ||
| armyInfo.troops.paladin_count - BigInt(troopLosses.paladinLost), | ||
| configManager.getResourcePrecision(), | ||
| ), | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add guards against negative troop counts.
The subtraction operations could potentially result in negative values if troop losses exceed current troops.
return {
[ResourcesIds.Crossbowman]: roundDownToPrecision(
- armyInfo.troops.crossbowman_count - BigInt(troopLosses.crossbowmanLost),
+ armyInfo.troops.crossbowman_count > BigInt(troopLosses.crossbowmanLost)
+ ? armyInfo.troops.crossbowman_count - BigInt(troopLosses.crossbowmanLost)
+ : 0n,
configManager.getResourcePrecision(),
),
// Apply similar changes for Knight and PaladinCommittable suggestion skipped: line range outside the PR's diff.
This pull request adds a new feature to the client that allows users to simulate pillage scenarios. It includes the following changes:
Added a new file
pillage-simulation.tsxin theclient/src/ui/modules/simulationdirectory that contains the implementation of the pillage simulation tool.Modified the
Config.tsxfile in theclient/src/ui/components/navigationdirectory to include a new constantpillageSimulationthat represents the pillage simulation tool in the navigation menu.Modified the
EntitiesArmyTable.tsxfile in theclient/src/ui/components/militarydirectory to add a new button that triggers the pillage simulation tool.Added a new file
Troops.tsxin theclient/src/ui/components/worldmap/battlesdirectory that contains the component for displaying troops in the pillage simulation tool.Modified the
BattleActions.tsxfile in theclient/src/ui/modules/military/battle-viewdirectory to display the outcome of the pillage simulation, including troops loss, success chance, and max resources stolen.Fixes #2123
Summary by CodeRabbit
Release Notes
New Features
PillageSimulationfeature, allowing users to simulate pillage scenarios alongside existing battle simulations.PillageSimulationPanelto display results and outcomes of the pillage simulation.BattleSimulationPanelwith improved layout and clearer display of battle results.EntitiesArmyTable.User Interface Improvements
Troopscomponent to manage and display troop resources in a grid layout.Bug Fixes
ArmySelectorcomponent with the updated user interface.These changes collectively improve the simulation experience and provide users with clearer and more engaging interfaces.