Fix/various #2740
Fix/various #2740
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
WalkthroughThe pull request updates multiple parts of the application. In the military module, the ArmyChip component now passes the owner entity via Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant AC as ArmyChip
participant AMC as ArmyManagementCard
participant AM as ArmyManager
U->>AC: Initiates troop action
AC->>AMC: Pass updated owner entity and event
AMC->>AMC: Compute homeDirection using position data
AMC->>AM: Call addTroopsToExplorer with homeDirection & troop count
AM->>AM: Adjust troop count via multiplyByPrecision
AM-->>AMC: Return update result
sequenceDiagram
participant U as User
participant VIcon as ViewOnMapIcon
participant UI as UI Store
U->>VIcon: Mouse enters icon (if tooltip allowed)
VIcon->>UI: Set tooltip "View on Map"
U->>VIcon: Clicks icon
VIcon->>UI: Clear tooltip
VIcon->>Map: Navigate to map view with position prop
Possibly related PRs
Poem
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
ERR_PNPM_OPTIONAL_DEPS_REQUIRE_PROD_DEPS Optional dependencies cannot be installed without production dependencies 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (5)
✨ Finishing Touches
🪧 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
client/apps/game/src/ui/modules/navigation/secondary-menu-items.tsx (1)
120-135: Consider UI/UX improvements for the bridge functionality.The implementation for the "Bridge Lords & Resources" button looks good functionally, but there are a few points to consider:
- The absolute positioning (
absolute top-12 right-0) may cause layout issues if the parent container's position isn't set appropriately.- The pattern of wrapping a button inside an anchor tag is not conventional and could be confusing from a usability perspective.
Consider refactoring to either:
- <a - className="text-brown cursor-pointer text-lg w-full" - href={`https://empire.realms.world/trade`} - target="_blank" - rel="noopener noreferrer" - > - <Button variant="secondary" className="w-full"> - <div className="flex items-center gap-2"> - <ResourceIcon resource="Lords" size="xs" /> - Bridge Lords & Resources - </div> - </Button> - </a> + <Button + variant="secondary" + className="w-full" + onClick={() => window.open("https://empire.realms.world/trade", "_blank", "noopener,noreferrer")} + > + <div className="flex items-center gap-2"> + <ResourceIcon resource="Lords" size="xs" /> + Bridge Lords & Resources + </div> + </Button>Or if the anchor tag is preferred for SEO or other reasons:
- <a - className="text-brown cursor-pointer text-lg w-full" - href={`https://empire.realms.world/trade`} - target="_blank" - rel="noopener noreferrer" - > - <Button variant="secondary" className="w-full"> - <div className="flex items-center gap-2"> - <ResourceIcon resource="Lords" size="xs" /> - Bridge Lords & Resources - </div> - </Button> - </a> + <a + className="text-brown cursor-pointer text-lg w-full no-underline" + href="https://empire.realms.world/trade" + target="_blank" + rel="noopener noreferrer" + style={{ display: 'block' }} + > + <div className="flex items-center gap-2 p-2 bg-secondary rounded"> + <ResourceIcon resource="Lords" size="xs" /> + Bridge Lords & Resources + </div> + </a>client/apps/game/src/ui/elements/view-on-map-icon.tsx (1)
1-41: Well-structured component extracted into its own file.The ViewOnMapIcon component has been properly extracted into its own file, improving code organization and reusability across the application.
However, there's a potential naming issue with the imported SVG component:
-import { ReactComponent as Map } from "@/assets/icons/common/world.svg"; +import { ReactComponent as WorldIcon } from "@/assets/icons/common/world.svg";And then update the usage:
- <Map + <WorldIconRenaming the imported SVG component from
Mapto something likeWorldIconwould avoid shadowing the globalMapobject, which is a built-in JavaScript constructor.🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Do not shadow the global "Map" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
client/apps/game/src/ui/components/military/army-chip.tsx(2 hunks)client/apps/game/src/ui/components/military/army-management-card.tsx(3 hunks)client/apps/game/src/ui/components/trading/market-header.tsx(1 hunks)client/apps/game/src/ui/elements/view-on-map-icon.tsx(1 hunks)client/apps/game/src/ui/modules/chat/chat.tsx(2 hunks)client/apps/game/src/ui/modules/navigation/left-navigation-module.tsx(0 hunks)client/apps/game/src/ui/modules/navigation/right-navigation-module.tsx(0 hunks)client/apps/game/src/ui/modules/navigation/secondary-menu-items.tsx(3 hunks)client/apps/game/src/ui/modules/navigation/top-left-navigation.tsx(1 hunks)client/apps/game/src/ui/modules/social/player-id.tsx(1 hunks)client/apps/game/src/ui/modules/stream/event-stream.tsx(4 hunks)client/apps/game/src/ui/modules/world-structures/world-structures-menu.tsx(1 hunks)client/apps/game/src/ui/utils/utils.ts(1 hunks)client/update-policies.js(0 hunks)config/environments/utils/building.ts(1 hunks)packages/core/src/managers/army-manager.ts(1 hunks)packages/core/src/utils/army.ts(2 hunks)packages/core/src/utils/utils.ts(1 hunks)
💤 Files with no reviewable changes (3)
- client/apps/game/src/ui/modules/navigation/right-navigation-module.tsx
- client/apps/game/src/ui/modules/navigation/left-navigation-module.tsx
- client/update-policies.js
🧰 Additional context used
🪛 Biome (1.9.4)
client/apps/game/src/ui/elements/view-on-map-icon.tsx
[error] 1-1: Do not shadow the global "Map" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
🔇 Additional comments (25)
client/apps/game/src/ui/modules/navigation/secondary-menu-items.tsx (2)
8-10: New imports added for UI enhancement.The new imports for
Button,ResourceIcon, anduseAccountare appropriate for the functionality being added.Also applies to: 17-17
32-32: Good use of React hook for connection status.Using the
useAccounthook to determine if a user is connected is a clean implementation that follows React best practices.config/environments/utils/building.ts (1)
197-197:❓ Verification inconclusive
Verify game balance impact of the significant resource cost increase
The cost of Ignium has been increased from 750 to 750,000 wheat, which represents a 1000x increase. This is a substantial change that will significantly impact resource economics and game balance.
🏁 Script executed:
#!/bin/bash # Check if this resource cost change is consistently applied elsewhere in the codebase # and if there are any other changes to Ignium costs or production rates echo "Checking for other Ignium cost references:" rg "Ignium.*amount:" --type ts -A 1 -B 1 echo "Checking for any Ignium production or balance references:" rg "Ignium.*production|Ignium.*balance" --type tsLength of output: 1705
Action Required: Confirm Consistency and Impact on Game Economy
The updated Ignium cost in
config/environments/utils/building.tsshows an increase from 750 to 750,000 wheat—a 1000x jump. However, related Ignium values inconfig/environments/utils/resource.ts(e.g., amounts like 4.2, 1, 5,000) are still significantly lower. Please verify that:
- The increased cost in the building configuration is deliberate and balanced against production/usage rates defined elsewhere.
- There is no unintended mismatch between the building cost and the resource definitions that could disrupt the game's economy.
client/apps/game/src/ui/modules/social/player-id.tsx (1)
7-7: Import path updated following component relocationThe import path for ViewOnMapIcon has been updated to reflect its new location in the elements directory. This change is part of a larger refactoring that centralizes the ViewOnMapIcon component.
packages/core/src/utils/utils.ts (1)
118-118:❓ Verification inconclusive
Improved precision handling with explicit floor operation
The
divideByPrecisionfunction now explicitly floors the result of the division, ensuring consistent integer outputs. This change prevents potential rounding issues when dealing with resource values.
🏁 Script executed:
#!/bin/bash # Find all usages of divideByPrecision to verify the impact of this change echo "Checking all usages of divideByPrecision:" rg "divideByPrecision\(" --type ts -A 1 -B 1 echo "Checking if there are any places where non-integer values from this function might cause issues:" rg "divideByPrecision.*(?:\.|-\>)(toFixed|toPrecision|decimal|float|fraction)" --type tsLength of output: 31779
Precision Handling Update – Manual Verification Recommended
The changes in
packages/core/src/utils/utils.tsnow explicitly floor the result by returningMath.floor(value / RESOURCE_PRECISION), ensuring that all outputs from thedivideByPrecisionfunction are integers. This modification minimizes rounding issues when handling resource values.A broad search across the codebase confirms that many files use
divideByPrecision, so it’s advised to manually verify that none of these usages rely on fractional precision. Note that our automated check for potential non-integer downstream assumptions encountered a regex error. To further validate, please run or review the following corrected search command:#!/bin/bash echo "Re-running search for potential non-integer issues from divideByPrecision:" rg "divideByPrecision.*(\\.|->)(toFixed|toPrecision|decimal|float|fraction)" --type tsOnce the output is verified, please confirm that no part of the system expects a floating-point value from this utility function.
client/apps/game/src/ui/modules/world-structures/world-structures-menu.tsx (1)
8-8: Import path updated following component relocationThe import path for ViewOnMapIcon has been updated to reference its new location in the elements directory, consistent with the same change in other files.
client/apps/game/src/ui/modules/navigation/top-left-navigation.tsx (1)
11-11: Updated import path for ViewOnMapIcon.The import path for the ViewOnMapIcon component has been updated to the new location in
@/ui/elements/view-on-map-icon. This change aligns with the component's relocation as part of code reorganization.client/apps/game/src/ui/components/trading/market-header.tsx (1)
6-6: Updated import path for ViewOnMapIcon.The import path for the ViewOnMapIcon component has been updated to the new location in
@/ui/elements/view-on-map-icon. This change maintains consistency with other files that use this component.client/apps/game/src/ui/components/military/army-chip.tsx (2)
7-8: Improved import organization for clearer dependencies.The imports have been organized more clearly, separating the ViewOnMapIcon import from the ArmyManagementCard import. This improves code readability and better reflects the actual component dependencies.
Also applies to: 13-13
92-92: Fixed entity owner reference in ArmyManagementCard.Updated the
owner_entityprop to usearmy.entity_owner_idinstead ofarmy.entityId, which correctly identifies the army's owner entity. This fixes an issue related to troop management functionality.packages/core/src/utils/army.ts (2)
43-43: Improved the home detection logic.The code now uses a more flexible approach to determine if an army is at its home structure, checking for adjacency rather than exact position matching.
155-162: Good addition of the adjacency checking function.This new utility function correctly determines if an army is adjacent to a structure by checking if the army's position matches any of the structure's neighboring hexes. This approach is more intuitive from a gameplay perspective and properly encapsulates this logic for reuse.
client/apps/game/src/ui/modules/stream/event-stream.tsx (4)
3-3: Import path updated for ViewOnMapIcon.The import path has been updated to reflect the component's new location.
13-13: Added hideChat prop to component signature.The EventStream component now accepts a hideChat boolean prop, allowing for conditional rendering based on chat visibility state.
93-94: Implemented conditional visibility for outer container.The outermost div now properly applies visibility classes based on the hideChat prop.
119-121: Added conditional visibility and height adjustment for event list.The event list container now adjusts its height and visibility based on the hideChat prop, ensuring proper layout responsiveness.
client/apps/game/src/ui/modules/chat/chat.tsx (2)
145-145: Added hideChat prop to EventStream component.The hideChat state is now correctly passed to the EventStream component, connecting the Chat component's state to the EventStream's visibility.
309-309: Fixed class formatting in Messages component.Minor formatting improvement by removing an extra space before the text-xs class.
client/apps/game/src/ui/utils/utils.ts (1)
232-232:❓ Verification inconclusive
Simplified time formatting.
The formatArrivalTime function has been simplified to only return the time in HH:MM format, removing additional contextual information like day, month, and time remaining.
Verify that this simplification doesn't negatively impact any UI components that might depend on the additional information previously provided:
🏁 Script executed:
#!/bin/bash # Search for usages of formatArrivalTime to ensure the simplified output is sufficient rg -A 2 -B 2 "formatArrivalTime" --type tsx --type tsLength of output: 81
Action: Re-verify UI Component Dependencies on Arrival Time Format
The updated
formatArrivalTimefunction now returns only an HH:MM-formatted string. Please ensure that no UI component relies on the previously provided additional details (day, month, time remaining). The initial search using ripgrep with the tsx file type flag returned an error. To help with manual verification, please run the following updated shell script command to search across both TypeScript and TSX files:#!/bin/bash # Include tsx type explicitly and search for usages of formatArrivalTime rg -A 2 -B 2 --type-add 'tsx:*.tsx' -t tsx -t ts "formatArrivalTime"If any usages are found that expect extra information, update them accordingly.
packages/core/src/managers/army-manager.ts (3)
114-114: Good improvement to precision handlingAdding
multiplyByPrecisionto the troop count ensures consistent precision handling when updating explorer troops, which aligns with how the contract functions are called.
117-117: Consistent precision handlingSimilar to the explorer troops update, adding
multiplyByPrecisionhere ensures guard troops are updated with the proper precision, maintaining consistency across the codebase.
128-128:❓ Verification inconclusive
Enhanced flexibility with homeDirection parameter
The addition of the
homeDirectionparameter replaces what was likely a hardcoded value (0), providing more flexibility and correctness when adding troops to an explorer army.To verify this change addresses issue #2520:
Also applies to: 134-135
🏁 Script executed:
#!/bin/bash # Check if there were any other locations using hardcoded direction values rg -A 3 -B 3 "home_direction: 0" --type typescriptLength of output: 87
Verification Update: Enhanced Flexibility with
homeDirectionParameterThe change in
packages/core/src/managers/army-manager.tsat line 128 replaces a previously hardcoded direction value (0) with a configurablehomeDirectionparameter. This update improves flexibility and correctness when assembling explorer armies, and similar adjustments appear to have been applied around lines 134–135.To ensure no other instances of a hardcoded direction remain, please run the following verification command, which now correctly targets TypeScript files:
#!/bin/bash # Check for remaining instances of a hardcoded home_direction value in TypeScript files rg -A 3 -B 3 "home_direction: 0" --type tsclient/apps/game/src/ui/components/military/army-management-card.tsx (3)
78-84: Good implementation for calculating homeDirectionThe new code properly calculates the direction between the army's current position and its structure's base coordinates. This is crucial for correctly placing troops when reinforcing armies at home.
88-96: Improved troop addition logicThe updated condition now requires both
army.isHomeAND a validhomeDirectionbefore troops can be added. This prevents potential issues where troops might be added incorrectly, and thehomeDirectionis now correctly passed to theaddTroopsToExplorermethod.
301-302: Improved display of army positionThe conditional rendering now clearly indicates if the army is at base, on the map, or in an unknown location. The integration with
ViewOnMapIconprovides a good user experience for locating armies.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
client/apps/game/src/ui/modules/navigation/secondary-menu-items.tsx (1)
120-133: Consider layout positioning and accessibility improvements.The bridge button functionality looks good, but there are a few potential improvements to consider:
The
absolutepositioning combined with fixed offset values (top-12 right-0) might cause layout issues if the parent container's dimensions change. Consider using a more flexible positioning approach.The button lacks explicit accessibility attributes like
aria-labelwhich would help screen reader users understand its purpose.The button's background color (
bg-brown/90) appears to be semi-transparent, which might affect text contrast and readability depending on what's behind it.{isConnected && ( - <div className="absolute top-12 right-0 bg-brown/90 mx-2"> + <div className="absolute top-12 right-0 bg-brown mx-2 z-10 rounded shadow"> <Button variant="secondary" className="w-full" + aria-label="Bridge Lords and Resources to external platform" onClick={() => window.open("https://empire.realms.world/trade", "_blank", "noopener,noreferrer")} > <div className="flex items-center gap-2"> <ResourceIcon resource="Lords" size="xs" /> Bridge Lords & Resources </div> </Button> </div> )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
client/apps/game/src/hooks/context/policies.ts(21 hunks)client/apps/game/src/ui/elements/view-on-map-icon.tsx(1 hunks)client/apps/game/src/ui/modules/navigation/secondary-menu-items.tsx(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- client/apps/game/src/hooks/context/policies.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- client/apps/game/src/ui/elements/view-on-map-icon.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test-season-pass
🔇 Additional comments (2)
client/apps/game/src/ui/modules/navigation/secondary-menu-items.tsx (2)
8-17: Clean import organization.The newly added imports are appropriately placed in the import section and correctly align with the components being used later in the code.
32-32: Proper hook usage.The
useAccounthook is correctly implemented to retrieve the connection status, which is then used to conditionally render the bridge button.
Closes #2211
Closes #2520
Fix add troops
Summary by CodeRabbit
New Features
Bug Fixes
Refactor