feature: allow multiple banks #2706
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
WalkthroughThis pull request introduces extensive refactoring and feature modifications across workflows, Dojo data querying and synchronization, UI components, Three.js managers, and resource management. Key updates include replacing debounced queries with synchronous data synchronization functions, removing outdated battle and combat components, updating naming conventions for entity IDs and components, and integrating environment‐specific logic in the Starknet provider. Several dependencies and configuration files have also been updated or removed to streamline testing and production workflows. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User/App
participant SP as StarknetProvider
participant PA as PredeployedAccounts (Connector)
participant CC as ControllerConnector
U->>SP: Initialize provider
SP->>SP: Check environment variable (chain)
alt Chain = "local"
SP->>PA: Call predeployedAccounts()
PA-->>SP: Return predeployedAccountsConnector[]
SP->>U: Return first predeployed account connector
else Chain = "mainnet" or "sepolia"
SP->>CC: Instantiate ControllerConnector with proper params
CC-->>SP: Connector instance created
SP->>U: Return new ControllerConnector
end
sequenceDiagram
participant W as World Component
participant SY as Sync Functions
participant DS as Dojo Setup
participant UI as UI Store
W->>SY: Call syncMarketAndBankData(...)
W->>SY: Call syncPlayerStructuresData(...)
W->>SY: Call syncStructureData(structureEntityId, setLoading, [position])
SY->>DS: Retrieve updated market, structure, and player data
DS-->>SY: Return synchronized data
SY->>W: Update state with new entity data
W->>UI: Render updated world view (armies, structures, explored hexes)
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
client/apps/game/src/dojo/queries.tsOops! Something went wrong! :( ESLint: 9.20.1 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. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by client/apps/game/src/dojo/debounced-queries.tsOops! Something went wrong! :( ESLint: 9.20.1 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. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by client/apps/game/src/dojo/sync.tsOops! Something went wrong! :( ESLint: 9.20.1 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. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by
Tip CodeRabbit's docstrings feature is now available as part of our Pro Plan! Simply use the command ✨ Finishing Touches
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently, please check "Code review limits" under "Moderation" settings.
Actionable comments posted: 14
🔭 Outside diff range comments (2)
client/apps/game/src/three/types/systems.ts (1)
31-38: 💡 Verification agent🧩 Analysis chain
Unused type declaration: BattleSystemUpdate.
The pipeline has flagged that the BattleSystemUpdate type is exported but not used anywhere in the codebase.
Consider either removing this unused type or updating it if it will be used as part of this feature implementation.
🏁 Script executed:
#!/bin/bash # Check if BattleSystemUpdate is used anywhere in the codebase rg -l "BattleSystemUpdate" --type ts --type tsxLength of output: 75
I'll run a new verification script that explicitly defines the tsx file type before searching. Please execute the following and verify the output:
If this search returns no results, it confirms that
BattleSystemUpdateis indeed unused throughout the codebase, and you can either remove it or update its implementation as needed for the feature. Please verify the results and adjust accordingly.
🏁 Script executed:
#!/bin/bash # Define tsx file type and check for any occurrence of "BattleSystemUpdate" in the repository rg --type-add "tsx:*.tsx" -l "BattleSystemUpdate"Length of output: 93
Action Required: Remove or update the unused export
BattleSystemUpdate.Our recent verification confirmed that the only occurrence of
BattleSystemUpdateis its declaration inclient/apps/game/src/three/types/systems.ts, and it isn’t referenced anywhere else in the codebase. Please remove this type if it isn’t needed, or update its implementation if it’s intended for upcoming feature work.
- File: client/apps/game/src/three/types/systems.ts (Lines: 31-38)
🧰 Tools
🪛 GitHub Actions: knip
[warning] 31-31: Unused exported type 'BattleSystemUpdate'.
client/apps/game/src/three/scenes/constants.ts (1)
76-81: 💡 Verification agent🧩 Analysis chain
Removed Settlement from structure type mappings.
The
StructureType.Settlementhas been removed from thestructureTypeToBuildingTyperecord. This change is likely part of the broader refactoring to support multiple banks, as the concept of settlements may have been obsoleted or merged with another structure type.
🏁 Script executed:
#!/bin/bash # Check if there are any remaining references to StructureType.Settlement rg "StructureType.Settlement" --type tsLength of output: 173
Attention: Inconsistent removal of
StructureType.SettlementThe removal of
StructureType.Settlementfrom thestructureTypeToBuildingTypemapping inclient/apps/game/src/three/scenes/constants.ts(lines 76–81) is confirmed. However, our verification indicates thatStructureType.Settlementis still referenced in the tests (specifically inpackages/core/src/managers/__tests__/battle-manager.test.tsvia thegenerateMockStructurecall).
File Update:
client/apps/game/src/three/scenes/constants.ts:StructureType.Settlementhas been removed as expected.Test Reference:
packages/core/src/managers/__tests__/battle-manager.test.ts: Contains a call togenerateMockStructure(StructureType.Settlement). This reference may need to be updated or removed if the removal of settlements is intentional.Please review and update the test file to align with the new structure type mapping.
🧹 Nitpick comments (39)
client/apps/game/src/ui/components/modal-container.tsx (1)
41-41: Consider adding an accessibility enhancement for the title.The title implementation looks good, but for better accessibility, consider adding an
aria-labelledbyattribute to the modal container div and anidto the title div.- <div className={`z-50 bg-brown/90 text-gold ${containerClasses} fixed overflow-hidden`} tabIndex={0}> + <div className={`z-50 bg-brown/90 text-gold ${containerClasses} fixed overflow-hidden`} tabIndex={0} aria-labelledby="modal-title"> <div className="flex flex-col h-full"> <div className="flex justify-between items-center p-2"> - <div className="text-lg font-semibold text-gold px-4">{title}</div> + <div id="modal-title" className="text-lg font-semibold text-gold px-4">{title}</div>client/apps/game/src/ui/components/resources/deposit-resources.tsx (1)
54-58: Refactored button text logicThe button text logic has been simplified to focus only on resource availability and weight. The conditional logic is clearer now but notice there are nested ternary operators which can reduce readability. Consider refactoring to improve readability.
- {resources.length === 0 && weight?.weight && weight.weight > 0n - ? "Resources syncing..." - : resources.length === 0 && weight?.weight && weight.weight === 0n - ? "No resources to deposit" - : "Deposit Resources"} + {(() => { + if (resources.length === 0) { + if (weight?.weight && weight.weight > 0n) { + return "Resources syncing..."; + } else if (weight?.weight && weight.weight === 0n) { + return "No resources to deposit"; + } + } + return "Deposit Resources"; + })()}client/apps/game/src/ui/components/structures/worldmap/structure-card.tsx (2)
46-47: Ensure the memoized function includes all relevant dependencies.
Sincedojo.setup.componentsis also referenced in the memoized callback, it’s best practice to include it in the dependency array to ensure the callback re-runs ifdojo.setup.componentschanges.Below is a suggested fix:
- [structure?.owner], + [structure?.owner, dojo.setup.components],
186-186: Offer help with the “WIP” Troop Exchange.
The placeholder text indicates incomplete logic. Let me know if you’d like me to open a new issue or propose a draft implementation for the troop exchange functionality.client/apps/game/src/ui/components/resources/resource-chip.tsx (1)
84-94: Added debug logging for resource information.Added console logging for resource ID and trait information. This is helpful for debugging but should be removed or guarded with a development-only flag before production deployment.
Consider guarding the console.log with a development-only check to prevent unnecessary logging in production:
const icon = useMemo(() => { - console.log("Resource ID:", resourceId, "trait", findResourceById(resourceId)?.trait); + if (process.env.NODE_ENV === 'development') { + console.log("Resource ID:", resourceId, "trait", findResourceById(resourceId)?.trait); + } return ( <ResourceIcon withTooltip={false} resource={findResourceById(resourceId)?.trait as string} size="sm" className="mr-3 self-center" /> ); }, [resourceId, isLabor]);client/apps/game/src/hooks/helpers/use-navigate-to-realm-view-by-account.ts (1)
18-21: Fix the typo in variable name "strucutureBase"There's a spelling error in the variable name on line 18. It should be "structureBase" (with correct spelling) to match the variable used on line 21 and elsewhere in the file.
- const strucutureBase = randomRealmEntity + const structureBase = randomRealmEntity ? getComponentValue(components.Structure, randomRealmEntity)?.base : undefined; - strucutureBase && navigateToHexView(new Position({ x: strucutureBase.coord_x, y: strucutureBase.coord_y })); + structureBase && navigateToHexView(new Position({ x: structureBase.coord_x, y: structureBase.coord_y }));client/apps/game/src/hooks/helpers/use-structure-entity-id.ts (1)
30-31: Remove debugging console.log statementThe console.log statement should be removed before merging to production. Leaving debug logs in production code can impact performance and potentially leak sensitive information.
- console.log({ occupier, structure, newStructureId }); -client/apps/game/src/ui/components/resources/realm-transfer.tsx (1)
45-45: Updated resource balance calculation methodThe component now uses
balanceWithProductioninstead of the previousbalancemethod to calculate resource amounts. This change aligns with the centralized liquidity management goal mentioned in the PR objectives by considering production values in the balance calculation.Make sure to update all other components that depend on this balance calculation to ensure consistency throughout the application.
client/apps/game/src/ui/elements/stamina-resource-cost.tsx (1)
2-15: Enhanced path-based stamina cost calculationThe component has been significantly refactored to handle dynamic path-based stamina costs rather than simple travel length calculations. This allows for more accurate resource management with:
- Detailed path-specific stamina costs
- Per-tile biome information
- Visual feedback on sufficient/insufficient stamina
Consider adding error handling for edge cases where
pathmight be empty or contain invalid data:const pathInfo = useMemo(() => { if (!stamina) return; + if (!path || path.length === 0) return { + isExplored, + totalCost: 0, + balanceColor: "text-order-brilliance", + balance: stamina.amount + }; // Calculate total cost and collect biome info const totalCost = path.reduce((acc, tile) => { - return acc + (tile.staminaCost ?? 0); + return acc + (tile?.staminaCost ?? 0); }, 0);Also applies to: 21-37
client/apps/game/src/ui/components/production/buildings-list.tsx (1)
51-54: Refactored resource management to use ResourceManager.Instead of directly accessing component values, the code now uses the
ResourceManagerclass to handle resource calculation, which provides better encapsulation and consistency, especially important when supporting multiple banks.When introducing a new pattern like this, consider adding a small comment explaining the rationale, especially since this is part of a larger refactoring effort. It helps future developers understand why the change was made.
client/apps/game/src/ui/components/trading/market-trading-history.tsx (1)
44-48: Updated owner retrieval to use Structure component.This change reflects the architectural shift to support multiple banks by retrieving ownership information from the Structure component instead of the Owner component. This approach allows for more flexible ownership management across multiple banks.
Consider extracting this owner retrieval logic into a reusable utility function since it's used multiple times in this file.
Also applies to: 50-54
client/apps/game/src/ui/components/trading/transfer-view.tsx (1)
48-54: Improved structure property access patternThe code now directly accesses position and owner data from the structure object, reducing component dependencies and streamlining the code. Position is constructed directly from coordinates, and owner is accessed directly from the structure object.
Consider creating a helper function or utility to consistently transform structure data into the required format, especially since this pattern is repeated elsewhere in the codebase.
client/apps/game/src/ui/modules/entity-details/building-entity-details.tsx (1)
139-150: Refactored building destruction logicThe logic for checking if a building can be destroyed has been significantly improved:
- Now directly retrieves the structure using the
Structurecomponent- Fetches population data from
StructureBuildingsinstead of a different source- More clearly compares population capacity using
population?.population.maxandpopulation?.population.currentThis approach simplifies the code flow and improves clarity in how population constraints are evaluated.
The nullish coalescing check on line 150 could be slightly simplified if
population.population.maxandpopulation.population.currentare guaranteed to be defined when population exists. Consider adding a type guard to make this more explicit.client/apps/game/src/ui/components/military/structure-defence.tsx (1)
145-147: Hardcoded cooldown time value needs to be configurable.The CooldownTimer is always initialized with 24 hours (24 * 60 * 60 seconds), but this should be configurable or passed through the cooldownSlots data.
interface CooldownTimerProps { slot: number; time: number; // seconds } // Then in the component usage {cooldownSlots.includes(index) ? ( - <CooldownTimer slot={index} time={24 * 60 * 60} /> + <CooldownTimer slot={index} time={cooldownTimes[index] || 24 * 60 * 60} /> ) : defense ? (Consider adding a
cooldownTimesproperty to theStructureDefencePropsinterface:interface StructureDefenceProps { maxDefenses: number; // 1-4 troops: DefenseTroop[]; cooldownSlots?: number[]; // Slots with active cooldown [1, 4] cooldownTimes?: Record<number, number>; // Map of slot index to cooldown time in seconds }client/apps/game/src/three/scenes/hexception.tsx (1)
625-625: Changed biome retrieval to use static method.The code now uses
Biome.getBiome()(static method) instead ofthis.biome.getBiome()(instance method), reflecting an architectural change in how biome data is accessed.Since the
biomeproperty was removed from theHexceptionSceneclass, ensure that the staticBiome.getBiome()method is properly implemented and accessible throughout the codebase. Consider adding error handling if the method could potentially return undefined values:-const biome = Biome.getBiome(targetHex.col, targetHex.row); +const biome = Biome.getBiome(targetHex.col, targetHex.row) || BiomeType.Grassland; // Provide fallbackclient/apps/game/src/ui/components/military/troop-chip.tsx (1)
38-38: Tier display enhancements.Exposing the tier value is helpful for clarity. If multiple tiers become more nuanced, you might consider styling or icons for each tier level (e.g., a different color or badge).
client/apps/game/src/three/helpers/pathfinding.ts (1)
79-79: Heuristic aligns with hex logic.Using a hex distance as the heuristic is appropriate for this coordinate system, ensuring consistent calculations. If your domain expands—for example, to handle certain terrain costs—consider tweaking the heuristic to reflect that complexity.
client/apps/game/src/ui/components/military/army-chip.tsx (4)
79-80: Update conditional rendering to use a more specific propertyThe current condition uses
armyas a boolean check, which is too generic and may lead to unintended behavior. Consider using a more specific property of the army object that accurately reflects whether it's a defensive army.- ${army ? "defensive-army-selector" : "attacking-army-selector"} + ${army.isDefensive ? "defensive-army-selector" : "attacking-army-selector"}
107-108: Use type coercion for consistent comparisonThe troops.count is a BigInt (indicated by the 0n literal), but the conditional rendering is using a direct comparison. It's better to explicitly compare with BigInt values for consistency.
-${army.troops.count === 0n ? "animate-pulse" : ""} +${army.troops.count === 0n ? "animate-pulse" : ""} -${army ? "defensive-army-edit-selector" : "attacking-army-edit-selector"}`} +${army.isDefensive ? "defensive-army-edit-selector" : "attacking-army-edit-selector"}`}
137-138: Use consistent defensive/attacking army checkSimilar to previous comments, the conditional here is using the army object itself as a boolean check, which should be replaced with a more specific property.
-${army ? "defensive-army-swap-selector" : "attacking-army-swap-selector"} +${army.isDefensive ? "defensive-army-swap-selector" : "attacking-army-swap-selector"}
209-242: Re-implementation needed for ArmyMergeTroopsPanelThe ArmyMergeTroopsPanel is showing a work-in-progress message with most of its implementation commented out. This indicates a partial refactoring, which should be completed to maintain functionality for merging troops.
According to the PR description about allowing multiple banks, this component might need to be updated to handle troop movements between banks at different positions. Would you like me to help with reimplementing this component to support the new multi-bank feature?
client/apps/game/src/ui/components/worldmap/armies/action-info.tsx (1)
118-120: Potential issue with isExplored checkThe isExplored check relies on the presence of biomeType in the last element of actionPath. This might cause unexpected behavior if actionPath is empty or undefined, as it would access properties of undefined.
- const isExplored = useMemo(() => { - return actionPath?.[actionPath.length - 1].biomeType !== undefined; - }, [actionPath]); + const isExplored = useMemo(() => { + return actionPath && actionPath.length > 0 && actionPath[actionPath.length - 1].biomeType !== undefined; + }, [actionPath]);client/apps/game/src/ui/components/military/army-management-card.tsx (1)
27-31: Keep troop-to-resource mapping updated if new troop types are introduced.
If additional troop types are added in the future, ensure they are mapped here to avoid undefined behavior.client/apps/game/src/three/systems/system-manager.ts (1)
112-118: Placeholder fields need eventual resolution.
battleId,defender, andcurrentHealthare hardcoded. Since they’re marked as “todo,” consider finalizing or removing them soon.client/apps/game/src/three/scenes/worldmap.tsx (3)
58-58: Memory considerations for storing biome info.
Switching from a set to aMap<number, BiomeType>is efficient for tile data, but note that large maps can be memory-intensive if the world is large.
495-589: Chunk-based updates for explored tiles.
Smart approach to re-render only relevant chunks. Confirm performance with large scale expansions.
341-356: Movement logic appears sound.
Consider adding error handling formoveArmyto provide user feedback if the call fails.client/apps/game/src/ui/layouts/world.tsx (2)
120-130: Consider edge cases and potential duplicates in subscriptions.This block adds
structureEntityId,ADMIN_BANK_ENTITY_ID, and all filtered structures to the subscription object, which may lead to duplicates if they overlap. Additionally, iffilteredStructuresis very large, memory usage might grow quickly. Confirm that this approach remains performant for large numbers of structures.
187-187: Review necessity of the extra<div>.This new
<div>adds an additional wrapper. If it does not serve a styling or semantic purpose, consider removing it or using a fragment (<>) for cleaner markup.client/apps/game/src/three/managers/army-manager.ts (1)
311-316: Refactor expandedmoveArmysignature with caution.This method now requires
armyHexes,structureHexes, andexploredTiles. If these datasets become large or change frequently, consider caching or throttling calls to avoid performance slowdowns.client/apps/game/src/ui/components/military/army-list.tsx (4)
35-35: Use a stable loading approach.Switching to a boolean loading state is simpler than an enum, but confirm that you handle multiple async actions in parallel. If multiple operations can happen, consider implementing a reference count or separate flags to avoid collisions.
37-50: Clarify forced defensive army creation.Subtracting one from
maxArmiesPerStructureeffectively forces the user to create a defensive army first. While it might be by design, this could cause confusion. A comment clarifying the rationale or a more explicit approach may help.
53-53: Avoid overloading a singlestructureArmiesarray for both attacking and defending counts.Both
numberAttackingArmiesandnumberDefensiveArmiesare set tostructureArmies.length. This lumps all armies together. If you plan to differentiate them later, keep logic consistent here or rename these variables to avoid confusion.Also applies to: 58-58
100-100: Consider semantic grouping.This
<div className="flex justify-center gap-4 my-6">might benefit from a more descriptive element or ARIA labels if multiple controls are grouped.client/apps/game/src/dojo/sync.ts (4)
181-198: Event entities query withVariableLenkey matching.Consider adding granular logging for different event types to improve debug visibility.
316-345: New syncStructureData function.As usage grows, consider grouping related fetches into smaller method calls to make error handling more granular.
347-411: syncPlayerStructuresData method.This method fetches multiple sets of data in a row. If you’d like, I can help create a combined result structure or potential concurrency approach so multiple awaits run simultaneously.
413-442: syncMarketAndBankData method.Suggest logging the query time for
debouncedGetMarketFromToriijust like with the bank query to maintain consistent performance telemetry.client/apps/game/src/ui/components/trading/market-order-panel.tsx (1)
606-606: Lords production manager includes production.If Lords can also be passively produced, consider clarifying that in docs or inline comments to avoid confusion for new contributors.
🛑 Comments failed to post (14)
client/apps/game/src/ui/components/entities/entity.tsx (1)
76-76:
⚠️ Potential issuePotential bug in arrival time calculation
The calculation
Number(0) - currentBlockTimestampwill result in a negative time value, which seems incorrect. The original calculation likely usedNumber(arrival.arrivesAt) - currentBlockTimestampto calculate the time remaining.- Arriving in {formatTime(Number(0) - currentBlockTimestamp)} to{" "} + Arriving in {formatTime(Number(arrival.arrivesAt) - currentBlockTimestamp)} to{" "}📝 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.Arriving in {formatTime(Number(arrival.arrivesAt) - currentBlockTimestamp)} to{" "}client/apps/game/src/ui/components/production/resource-production-controls.tsx (1)
114-114: 💡 Verification agent
🧩 Analysis chain
Method update: Use balanceWithProduction instead of balance.
Changed from
resourceManager.balance(currentDefaultTick)toresourceManager.balanceWithProduction(currentDefaultTick), which suggests the balance calculation now incorporates production factors. This could be important for accurate resource management across multiple banks.Let's check if this method is consistently updated across other components:
🏁 Script executed:
#!/bin/bash # Check for any remaining instances of the old balance method rg "resourceManager\.balance\(" --type ts --type tsx | grep -v "balanceWithProduction"Length of output: 114
Action: Confirm consistent method update across the codebase
The update in
client/apps/game/src/ui/components/production/resource-production-controls.tsx(line 114) replacesconst balance = resourceManager.balance(currentDefaultTick);with
const balance = resourceManager.balanceWithProduction(currentDefaultTick);so that the balance calculation now correctly includes production factors. While our initial search encountered issues due to file type recognition, the new script above is configured to search both
.tsand.tsxfiles for any remaining occurrences ofresourceManager.balance(that haven't been updated.Please run the above verification script and manually confirm that no outdated usages remain.
🏁 Script executed:
[run_scripts] #!/bin/bash echo "Searching in .ts files:" rg "resourceManager\.balance\(" --glob "*.ts" | grep -v "balanceWithProduction" echo "Searching in .tsx files:" rg "resourceManager\.balance\(" --glob "*.tsx" | grep -v "balanceWithProduction"Length of output: 484
Action: Address inconsistent method usage for resource management
The update in
client/apps/game/src/ui/components/production/resource-production-controls.tsxcorrectly replacesconst balance = resourceManager.balance(currentDefaultTick);with
const balance = resourceManager.balanceWithProduction(currentDefaultTick);to incorporate production factors. However, our additional search in the codebase revealed an instance in:
- File:
packages/core/src/utils/resources.ts
Line (approximate): where it callsconst balance = resourceManager.balance();Please review whether this outdated usage should also be updated to
balanceWithProduction()for consistency and accurate resource management across multiple banks. If the differing usage is intentional due to context-specific logic, kindly add a clarifying comment in the file.client/apps/game/src/ui/components/production/labor-production-controls.tsx (1)
74-75:
⚠️ Potential issueFix the hardcoded resource balance implementation.
The resource balance calculation has been replaced with a hardcoded value of
0and a TODO comment. This prevents users from seeing or using their actual resource balances for labor production, making the feature non-functional.Please implement the proper resource balance calculation. Likely this needs to be replaced with a call to a
ResourceManagermethod, similar to changes made in other components like the production sidebar.client/apps/game/src/ui/components/worldmap/structures/structure-label.tsx (1)
24-24: 💡 Verification agent
🧩 Analysis chain
Updated structure owner access pattern
The code has been modified to directly use
structure?.ownerinstead of accessingstructure?.owner.address. This change suggests the structure of the owner property has been simplified from an object to a direct value.
🏁 Script executed:
#!/bin/bash # Check for other instances of owner.address to ensure consistent updates echo "Checking for other instances of owner.address access pattern..." rg "owner\.address" -t ts -t tsxLength of output: 199
Below is a revised review comment with an updated verification step to ensure the new access pattern is applied consistently across the codebase:
Verify Updated Owner Access Pattern
The change from usingstructure?.owner.addresstostructure?.ownercorrectly reflects the simplified structure for the owner property. To ensure that no residual references to the oldowner.addresspattern remain, please re-run a verification search using the updated script below:If this search returns no results, it confirms that all occurrences have been properly updated. Please verify the output manually.
🏁 Script executed:
[run_scripts] #!/bin/bash echo "Rechecking for any usage of 'owner.address' pattern in TypeScript files..." rg "owner\.address" --glob "*.ts" --glob "*.tsx" .Length of output: 815
Unified Owner Access Update Needed
The update inclient/apps/game/src/ui/components/worldmap/structures/structure-label.tsxcorrectly simplifies the owner access by usingstructure?.ownerinstead ofstructure?.owner.address. However, a subsequent search detected that the old access pattern (owner.address) is still used elsewhere in the codebase:
client/apps/game/src/three/managers/structure-manager.ts
- Usage instances:
isAddressEqualToAccount(owner.address)structure.isMine = isAddressEqualToAccount(structure.owner.address)
client/apps/game/src/three/managers/army-manager.ts
- Usage instances:
isAddressEqualToAccount(owner.address)army.isMine = isAddressEqualToAccount(army.owner.address)To ensure consistency with the new, simplified data structure for the owner property, please review and update these occurrences accordingly.
client/apps/game/src/ui/components/bank/swap.tsx (1)
54-58:
⚠️ Potential issueTODO comment indicates incomplete work.
The bank protector logic has been commented out and marked as needing fixing. This suggests that the protection mechanism for banks is being redesigned to accommodate multiple banks.
Make sure to address this TODO before finalizing the PR. The bank protector logic may be crucial for security or gameplay balance, especially in a multi-bank system.
client/apps/game/src/three/managers/highlight-hex-manager.ts (1)
8-23:
⚠️ Potential issueRemove unused exported function.
The exported function
getHighlightColorForActionis not being used elsewhere in the codebase according to the pipeline warning. Either remove the export keyword or use this function in other parts of the codebase.-export const getHighlightColorForAction = (actionType: ActionType): THREE.Vector3 => { +const getHighlightColorForAction = (actionType: ActionType): THREE.Vector3 => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const getHighlightColorForAction = (actionType: ActionType): THREE.Vector3 => { switch (actionType) { case ActionType.Explore: return new THREE.Vector3(0.0, 1.5, 1.5); // More intense cyan case ActionType.Move: return new THREE.Vector3(0.0, 1.5, 0.0); // More intense green case ActionType.Attack: return new THREE.Vector3(2.0, 0.0, 0.0); // More intense red case ActionType.Help: return new THREE.Vector3(1.5, 0.0, 1.5); // More intense purple case ActionType.Build: return new THREE.Vector3(0.0, 0.0, 1.5); // More intense blue default: return new THREE.Vector3(1.5, 1.5, 1.5); // More intense white } };🧰 Tools
🪛 GitHub Actions: knip
[warning] 8-8: Unused export 'getHighlightColorForAction'.
client/apps/game/src/ui/components/military/structure-defence.tsx (1)
88-91: 🛠️ Refactor suggestion
Remove console.log statement before production.
The function logs the new defense order to the console but doesn't provide a way for parent components to use this updated order.
const handleConfirm = () => { - console.log("New defense order confirmed:", defenseTroops); setIsReordering(false); };Consider adding an
onOrderChangecallback prop to allow parent components to receive the updated order:interface StructureDefenceProps { maxDefenses: number; // 1-4 troops: DefenseTroop[]; cooldownSlots?: number[]; // Slots with active cooldown [1, 4] + onOrderChange?: (newOrder: DefenseTroop[]) => void; } // Then update handleConfirm const handleConfirm = () => { - console.log("New defense order confirmed:", defenseTroops); + onOrderChange?.(defenseTroops); setIsReordering(false); };📝 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.interface StructureDefenceProps { maxDefenses: number; // 1-4 troops: DefenseTroop[]; cooldownSlots?: number[]; // Slots with active cooldown [1, 4] onOrderChange?: (newOrder: DefenseTroop[]) => void; } const handleConfirm = () => { onOrderChange?.(defenseTroops); setIsReordering(false); };client/apps/game/src/ui/components/worldmap/structures/structure-list-item.tsx (1)
76-76:
⚠️ Potential issueFix incorrect rendering of showButtons prop.
The boolean
showButtonsprop is being rendered directly in the JSX, which will display the text "true" or "false" when rendered. This was likely meant to conditionally render UI buttons.- {showButtons} + {showButtons && ( + <div className="flex flex-col gap-2 ml-2"> + {/* Add your buttons here */} + </div> + )}📝 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.{showButtons && ( <div className="flex flex-col gap-2 ml-2"> {/* Add your buttons here */} </div> )}client/apps/game/src/hooks/context/starknet-provider.tsx (2)
81-81:
⚠️ Potential issueUnsafe type casting in connector prop.
The code uses a double type cast
connector as never as Connectorwhich is a workaround for type incompatibility. This might hide potential runtime errors.Consider adding proper type handling:
-connectors={[connector as never as Connector]} +connectors={connector ? [connector as Connector] : []}📝 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.connectors={connector ? [connector as Connector] : []}
31-38:
⚠️ Potential issuePotential race condition in predeployed accounts initialization.
The predeployed accounts are loaded asynchronously without proper error handling or safeguards. The
predeployedAccountsConnectorarray might be empty when accessed in theuseMemohook if the promise hasn't resolved yet.Consider adding error handling and a loading state:
+import { useEffect, useState } from "react"; +const [predeployedAccountsConnector, setPredeployedAccountsConnector] = useState<PredeployedAccountsConnector[]>([]); +const [isLoading, setIsLoading] = useState(env.VITE_PUBLIC_CHAIN === "local"); +useEffect(() => { if (env.VITE_PUBLIC_CHAIN === "local") { + setIsLoading(true); predeployedAccounts({ rpc: env.VITE_PUBLIC_NODE_URL as string, id: "katana", name: "Katana", - }).then((p) => (predeployedAccountsConnector = p)); + }) + .then((p) => setPredeployedAccountsConnector(p)) + .catch((err) => console.error("Failed to load predeployed accounts:", err)) + .finally(() => setIsLoading(false)); } +}, [env.VITE_PUBLIC_CHAIN]);📝 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.import { useEffect, useState } from "react"; // ... other imports and code const [predeployedAccountsConnector, setPredeployedAccountsConnector] = useState<PredeployedAccountsConnector[]>([]); const [isLoading, setIsLoading] = useState(env.VITE_PUBLIC_CHAIN === "local"); useEffect(() => { if (env.VITE_PUBLIC_CHAIN === "local") { setIsLoading(true); predeployedAccounts({ rpc: env.VITE_PUBLIC_NODE_URL as string, id: "katana", name: "Katana", }) .then((p) => setPredeployedAccountsConnector(p)) .catch((err) => console.error("Failed to load predeployed accounts:", err)) .finally(() => setIsLoading(false)); } }, [env.VITE_PUBLIC_CHAIN]); // ... rest of the component codeclient/apps/game/src/ui/components/military/army-management-card.tsx (2)
81-90: 🛠️ Refactor suggestion
Handle promise for asynchronous troop addition.
armyManager.addTroopsis called withoutawait. Consider awaiting the promise to handle errors or confirm transaction success before resetting the troop count.const handleBuyArmy = async () => { if (!selectedTroopType) return; setIsLoading(true); const troops = { /* omitted for brevity */ }; - armyManager.addTroops(account, troops); + await armyManager.addTroops(account, troops).catch(console.error); setTroopCount(0); setIsLoading(false); };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const handleBuyArmy = async () => { if (!selectedTroopType) return; setIsLoading(true); const troops = { [ResourcesIds.Knight]: 0, [ResourcesIds.Crossbowman]: 0, [ResourcesIds.Paladin]: 0, [troopTypeToResourcesId[selectedTroopType]]: multiplyByPrecision(troopCount), }; await armyManager.addTroops(account, troops).catch(console.error); setTroopCount(0); setIsLoading(false); };
135-135:
⚠️ Potential issueFix syntax error in the Paladin line.
There is an extra parenthesis aftercurrencyFormatcausing a parse error.- current: army?.troops.category === TroopType.Paladin ? currencyFormat(Number(army?.troops.count || 0), 0), 0) : 0, + current: army?.troops.category === TroopType.Paladin ? currencyFormat(Number(army?.troops.count || 0), 0) : 0,📝 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.current: army?.troops.category === TroopType.Paladin ? currencyFormat(Number(army?.troops.count || 0), 0) : 0,client/apps/game/src/three/managers/army-manager.ts (1)
326-330: 🛠️ Refactor suggestion
Extract stamina usage and path limit to a more flexible system.
Currently, the code uses
Paladinstamina as a universal maximum. This may not scale if you introduce diverse troop types or unique stamina rules. Consider building a more generic approach to adapt to different unit types.- // todo: currently taking max stamina of paladin as max stamina but need to refactor - const maxTroopStamina = configManager.getTroopStaminaConfig(TroopType.Paladin); - const maxHex = Math.floor(Number(maxTroopStamina) / configManager.getMinTravelStaminaCost()); + // Proposed flexible system for multiple troop types or custom travel limits + const troopStamina = someDynamicTroopStamina(armyData.order, armyData.troops); + const maxHex = calculateMaxHexDistance(troopStamina);Committable suggestion skipped: line range outside the PR's diff.
client/apps/game/src/ui/components/military/army-list.tsx (1)
64-65: 🛠️ Refactor suggestion
Improve error handling in
handleCreateArmy.Throwing an error if
entity_idis undefined can abruptly break the UI. Consider user-friendly error messages or a fallback path. Also ensure that callingsetLoading(true)is balanced with a guaranteed cleanup in case early returns occur.Also applies to: 68-68, 71-71
Summary by CodeRabbit
UI Improvements
Gameplay Updates
Removed Features