update main social tab #3356
update main social tab #3356
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
WalkthroughThis update introduces a global player data store and a global battle logs store, along with UI components and infrastructure for displaying and managing battle logs in the game. It extends player and structure data models, updates SQL queries, and integrates the new logs view into the right navigation panel. Additional UI enhancements and store managers are included. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI (BattleLogsTable)
participant useBattleLogsStore
participant API
User->>UI (BattleLogsTable): Open Logs view
UI (BattleLogsTable)->>useBattleLogsStore: useBattleLogs(), fetchInitialBattleLogs()
useBattleLogsStore->>API: fetchBattleLogs()
API-->>useBattleLogsStore: Battle log events
useBattleLogsStore-->>UI (BattleLogsTable): Processed battle logs
User->>UI (BattleLogsTable): Apply filter/search/pagination
UI (BattleLogsTable)->>useBattleLogsStore: useBattleLogsByPlayer(), useLatestBattleLogs()
useBattleLogsStore-->>UI (BattleLogsTable): Filtered logs
sequenceDiagram
participant UI (PlayersPanel)
participant usePlayerStore
participant PlayerDataStore
participant API
UI (PlayersPanel)->>usePlayerStore: useCurrentPlayerData(), usePlayerDataByAddress()
usePlayerStore->>PlayerDataStore: getPlayerDataByAddress()
PlayerDataStore->>API: fetchAndStoreData()
API-->>PlayerDataStore: Player data with structure counts
PlayerDataStore-->>usePlayerStore: Transformed player data
usePlayerStore-->>UI (PlayersPanel): Player data (with banks, villages, etc.)
Possibly related PRs
Suggested reviewers
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/services/api.tsOops! Something went wrong! :( ESLint: 9.26.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. 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/three/managers/player-data-store.tsOops! Something went wrong! :( ESLint: 9.26.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. 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/ui/components/events/battle-logs-table.tsxOops! Something went wrong! :( ESLint: 9.26.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 7
🧹 Nitpick comments (7)
client/apps/game/src/ui/components/worldmap/players/players-panel.tsx (1)
156-161: Consider improving message clarity and reducing visual distraction.While the implementation is technically sound, the dramatic language and pulsing animation might be overwhelming in the UI context. Consider:
- More user-friendly language: Replace dramatic terms like "ULTIMATE POWER" with clearer game mechanics explanation
- Remove distracting animations: The
animate-pulseclass could be distracting during regular gameplay- Make threshold configurable: The hardcoded "9.6M" value should ideally come from a configuration constant
- <div className="my-2 py-2 px-3 border-2 border-gold-600/70 rounded-lg bg-slate-900/70 shadow-lg shadow-gold-500/20 text-center"> - <p className="font-serif text-lg text-amber-400 animate-pulse tracking-wider leading-relaxed"> - {" "} - SHOULD ANY LORD GATHER 9.6M POINTS, THEY GAIN THE ULTIMATE POWER TO <br></br> END THIS GAME - </p> - </div> + <div className="my-2 py-2 px-3 border border-gold-600/50 rounded-lg bg-slate-900/50 text-center"> + <p className="text-sm text-amber-300 tracking-wide"> + Game ends when a player reaches 9.6M points + </p> + </div>client/apps/game/src/ui/components/events/battle-logs-table.tsx (1)
1-946: Consider splitting this large component into smaller modules.At 946 lines, this file could benefit from being split into separate files for better maintainability:
- Move custom hooks (
useEntityData,useFilteredBattleLogs) to a separate hooks file- Extract sub-components (
EntityName,SmartFilter,BattleCard) to individual component files- Consider extracting constants and utility functions
client/apps/game/src/hooks/store/use-player-store.ts (4)
20-21: Add missing documentation for consistency.The
getStructureNamemethod lacks JSDoc documentation while other methods are well-documented.+ /** + * Get structure name by structure ID + * @param structureId - The structure entity ID + * @returns Structure name or empty string if not found + */ getStructureName: (structureId: string) => Promise<string>;Also applies to: 132-136
21-22: Add missing documentation forgetAllPlayersData.This method should have JSDoc documentation to explain its purpose and return value.
+ /** + * Get all players data + * @returns Array of all players' transformed data + */ getAllPlayersData: () => Promise<PlayerDataTransformed[]>;Also applies to: 205-209
232-250: Optimize loading state management to prevent unnecessary updates.The loading state is set to true even when the address is null, which can cause unnecessary re-renders.
export const usePlayerDataByAddress = (address: string | null) => { const getPlayerDataByAddress = usePlayerStore((state) => state.getPlayerDataByAddress); const [data, setData] = React.useState<PlayerDataTransformed | undefined>(undefined); const [loading, setLoading] = React.useState(false); React.useEffect(() => { if (!address) { setData(undefined); + setLoading(false); return; } setLoading(true); getPlayerDataByAddress(address) .then(setData) .finally(() => setLoading(false)); }, [address, getPlayerDataByAddress]); return { data, loading }; };
257-275: Apply the same loading state optimization to other hooks.The same optimization should be applied to
usePlayerDataByStructureIdandusePlayerDataByExplorerIdfor consistency.Also applies to: 282-300
client/apps/game/src/hooks/store/use-battle-logs-store.ts (1)
189-194: Remove commented-out code for clarity.The commented code should be removed as it could cause confusion. The current implementation correctly keeps existing logs during refresh for better UX.
refreshBattleLogs: async () => { - // Keep existing logs during refresh, rely on isLoading state - // set({ - // battleLogs: [], - // lastFetchTimestamp: null, - // error: null - // }); set({ lastFetchTimestamp: null, error: null }); // Clear timestamp and error, but keep logs await get().fetchInitialBattleLogs(); },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
client/apps/game/src/dojo/queries.ts(1 hunks)client/apps/game/src/hooks/store/use-battle-logs-store.ts(1 hunks)client/apps/game/src/hooks/store/use-player-store.ts(1 hunks)client/apps/game/src/index.css(1 hunks)client/apps/game/src/services/api.ts(5 hunks)client/apps/game/src/three/managers/player-data-store.ts(5 hunks)client/apps/game/src/three/systems/system-manager.ts(6 hunks)client/apps/game/src/types/index.ts(1 hunks)client/apps/game/src/ui/components/events/battle-logs-table.tsx(1 hunks)client/apps/game/src/ui/components/worldmap/players/player-list.tsx(1 hunks)client/apps/game/src/ui/components/worldmap/players/players-panel.tsx(1 hunks)client/apps/game/src/ui/config.tsx(2 hunks)client/apps/game/src/ui/modules/navigation/right-navigation-module.tsx(3 hunks)client/apps/game/src/ui/modules/social/social.tsx(2 hunks)client/apps/game/src/ui/store-managers.tsx(2 hunks)packages/core/src/managers/leaderboard-manager.ts(1 hunks)packages/core/src/utils/players.ts(2 hunks)packages/types/src/types/common.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
client/apps/game/src/ui/store-managers.tsx (2)
client/apps/game/src/hooks/store/use-player-store.ts (1)
usePlayerStore(49-211)client/apps/game/src/hooks/store/use-battle-logs-store.ts (1)
useBattleLogsStore(110-225)
packages/core/src/utils/players.ts (2)
packages/types/src/types/common.ts (4)
Player(592-596)ContractAddress(346-346)ContractAddress(352-354)PlayerInfo(575-590)packages/types/src/dojo/create-client-components.ts (1)
ClientComponents(4-4)
client/apps/game/src/services/api.ts (2)
client/apps/game/src/three/utils/label-utils.ts (1)
query(109-127)client/apps/landing/src/hooks/services/queries.ts (1)
QUERIES(1-315)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test-various (test_troop_battle)
- GitHub Check: test-various (test_troop_movement)
- GitHub Check: test-various (models)
- GitHub Check: test-various (quest)
- GitHub Check: test-various (troop_management)
- GitHub Check: test-season-pass
🔇 Additional comments (40)
client/apps/game/src/index.css (3)
617-626: LGTM! Well-designed entrance animation.The
battle-card-enteranimation uses smooth cubic-bezier easing and appropriate scale/transform effects for a polished user experience.
628-646: LGTM! Subtle and effective pulse animations.The green and red border pulse animations provide good visual feedback with appropriate opacity and shadow values that aren't distracting.
660-674: LGTM! Excellent hover effects with smooth transitions.The battle card hover effects use sophisticated shadow layering and smooth cubic-bezier transitions that enhance the user experience without being excessive.
client/apps/game/src/types/index.ts (1)
9-9: LGTM! Clean enum extension.The
Logsenum value follows the existing naming conventions and integrates well with the navigation system.packages/types/src/types/common.ts (1)
586-587: LGTM! Consistent interface extension.The new
villagesandbanksproperties follow the same pattern as existing structure count properties and are appropriately typed as numbers.client/apps/game/src/dojo/queries.ts (1)
108-108: LGTM! Proper model configuration addition.The
"s1_eternum-PlayerRegisteredPoints"model follows the consistent naming pattern and is correctly added to the one-key configuration models.client/apps/game/src/ui/components/worldmap/players/player-list.tsx (1)
188-192: LGTM! Proper expansion of structure counting.The addition of
banksandvillagesto the total structure count is correctly implemented with defensive programming using|| 0to handle potentially undefined values. This aligns well with the expanded player data model described in the PR.client/apps/game/src/ui/config.tsx (2)
128-128: LGTM! Appropriate icon choice for logs feature.The addition of the logs entry with an hourglass icon is well-suited for representing battle logs/history functionality.
142-142: LGTM! Consistent enum extension.The logs menu enum addition follows the established pattern and properly supports the new logs navigation feature.
client/apps/game/src/ui/modules/navigation/right-navigation-module.tsx (3)
6-6: LGTM: Clean import additionThe import follows the established pattern and path structure.
92-106: LGTM: Consistent menu item implementationThe new logs menu item follows the exact same pattern as existing menu items (ResourceTable, Bridge, Automation) with proper:
- Menu configuration
- CircleButton setup with all required props
- Active state management
- Click handler that toggles between the view and None
156-160: LGTM: Proper view rendering integrationThe conditional rendering for the logs view is consistent with other views and properly integrated into the panel structure.
client/apps/game/src/ui/modules/social/social.tsx (1)
2-4: LGTM: Proper imports for new player store integrationThe imports are correctly added to support the refactoring to use the global player store.
client/apps/game/src/ui/store-managers.tsx (4)
1-8: LGTM: Clean imports for new store managersAll necessary imports are properly added to support the new store managers.
79-103: LGTM: Well-structured PlayerDataStoreManagerThe component properly:
- Initializes the player store singleton on mount
- Updates current player data when the account changes
- Uses correct dependencies in useEffect hooks
- Follows the established pattern of other store managers
105-163: LGTM: Comprehensive BattleLogsStoreManager implementationExcellent implementation that handles multiple scenarios:
- ✅ Initial data fetch only when needed (battleLogs.length === 0)
- ✅ Periodic refresh every minute after initial load
- ✅ Refresh on page visibility change for better UX
- ✅ Proper cleanup of intervals to prevent memory leaks
- ✅ Correct use of useRef for interval management
The component demonstrates good performance awareness and proper lifecycle management.
171-172: LGTM: Proper integration of new store managersBoth new store managers are correctly added to the StoreManagers component.
client/apps/game/src/three/systems/system-manager.ts (7)
1-1: LGTM: Import added for global player store integrationCorrectly imports the global player store hook to replace local instance usage.
189-189: LGTM: Simplified constructor after removing local player storeThe constructor is properly simplified by removing the local player data store initialization, now that the global store is used.
222-224: LGTM: Correct usage of global player storeProper implementation using
usePlayerStore.getState()to access the Zustand store outside of React components. This is the recommended pattern for accessing Zustand stores in non-React contexts.
241-245: LGTM: Consistent global store usage for explorer managementThe explorer-structure mapping updates correctly use the global player store methods, maintaining the same functionality while using centralized data management.
252-256: LGTM: Proper async data retrieval from global storeThe async calls to retrieve player data by explorer ID and address are correctly implemented using the global store methods.
343-365: LGTM: Well-structured structure owner data managementThe logic for querying, updating, and refreshing structure owner data is properly implemented:
- Correctly handles ownership updates when RECS data differs from cached data
- Includes fallback logic for new players requiring data refresh
- Maintains data consistency between different sources
372-375: LGTM: Consistent logged-in account data retrievalUses the global store consistently for retrieving logged-in account player data, maintaining the same pattern as other data access calls.
packages/core/src/utils/players.ts (3)
1-2: LGTM! Clean import statement.The removal of unused imports improves code cleanliness.
10-19: Good performance optimization with the precomputed structure counts.Replacing dynamic queries with a precomputed map is an excellent optimization that reduces database calls and improves performance.
59-63: Excellent defensive programming with fallback values.Using optional chaining and nullish coalescing (
?? 0) ensures the code handles missing data gracefully.client/apps/game/src/three/managers/player-data-store.ts (2)
12-17: Well-structured interface extension.The new structure count properties follow a consistent naming convention and integrate well with the existing interface.
137-144: Good addition of utility methods.The new
getStructureNameandgetAllPlayersDatamethods provide clean APIs for accessing the stored data with proper refresh checks.client/apps/game/src/services/api.ts (3)
61-65: Excellent use of conditional counting.Using
COUNT(DISTINCT CASE ...)is an efficient way to get counts by category in a single query.
136-163: Well-structured UNION query for combining event types.The BATTLE_LOGS query effectively combines data from two tables with proper field mapping and NULL handling for non-matching columns.
53-53:✅ Verification successful
Verify SQL concatenation syntax compatibility.
The use of
||for string concatenation may not be supported in all SQL databases (e.g., MySQL uses CONCAT).
🌐 Web query:
What SQL database does Torii use and does it support the || concatenation operator?💡 Result:
Torii is an automatic indexer and networking layer for a world contract that utilizes a SQLite database to store indexed data. (book.dojoengine.org)
SQLite supports the
||operator for string concatenation, which is the standard ANSI SQL operator. (devanddep.com)Therefore, when using Torii with SQLite, you can use the
||operator to concatenate strings in your SQL queries.Citations:
- 1: https://book.dojoengine.org/toolchain/torii/reference?utm_source=openai
- 2: https://www.devanddep.com/tutorial/sql/sql-concatenation-operator.html?utm_source=openai
SQL concatenation with
||is valid in Torii’s SQLiteSQLite—the database underlying Torii—fully supports the ANSI
||string-concatenation operator, so your use ofGROUP_CONCAT(DISTINCT s.entity_id || ':' || s.`metadata.realm_id`) AS structure_idsis correct and requires no changes.
client/apps/game/src/ui/components/events/battle-logs-table.tsx (3)
59-183: Excellent entity resolution implementation.The
useEntityDatahook demonstrates:
- Good error handling with try-catch blocks
- Efficient parallel processing with
Promise.allSettled- Special case handling for different entity types
- Proper state management
676-684: Good attention to decimal precision handling.The reward amount formatting properly handles the conversion from raw to display amounts and handles edge cases like very small numbers.
776-784: Well-designed loading state UI.The loading animation with contextual messaging provides good user feedback during data fetching.
client/apps/game/src/hooks/store/use-player-store.ts (2)
1-4: LGTM!The imports are correct and appropriate for a Zustand store with React integration.
49-211: Well-implemented store with proper error handling!The store implementation follows best practices:
- Proper null checks before accessing playerDataStore
- Comprehensive error handling with console logging
- Immediate data refresh on initialization
- Clear separation of concerns between actions
The 6-hour default refresh interval is reasonable for player data that doesn't change frequently.
client/apps/game/src/hooks/store/use-battle-logs-store.ts (3)
1-18: Well-structured interfaces and imports!The interfaces properly extend the base
BattleLogEventwith additional processed fields for better usability.
227-263: Excellent use of React.useMemo for performance optimization!The convenience hooks are well-implemented with proper memoization to prevent unnecessary re-computations.
58-70:✅ Verification successful
Verify the timestamp unit assumption.
The function assumes timestamps are in seconds and converts to milliseconds. This assumption should be verified with the API documentation or actual data.
🌐 Web query:
What unit does the Eternum game API use for timestamps in battle log events - seconds or milliseconds?💡 Result:
The Eternum game API utilizes timestamps in battle log events that are formatted as human-readable date and time strings, such as "Mon Apr 22 10:54:25 2024." (caribdis.itch.io) This format indicates that the timestamps are in seconds, not milliseconds.
Citations:
Confirmed timestamp unit: seconds
The Eternum game API returns battle log timestamps with second-level precision (e.g. “Mon Apr 22 10:54:25 2024”), so treating the hex value as seconds and multiplying by 1,000 to convert to milliseconds is correct.
- File:
client/apps/game/src/hooks/store/use-battle-logs-store.ts
parseTimestampproperly converts a seconds‐based BigInt hex timestamp to milliseconds.
| .loading-spin { | ||
| animation: spin 1s linear infinite; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Verify that the spin animation is defined.
The .loading-spin class references a spin animation that should be defined elsewhere in the codebase.
🏁 Script executed:
#!/bin/bash
# Description: Verify that the spin animation is defined in the CSS codebase
# Expected: Find the @keyframes spin definition
rg -A 10 "@keyframes spin" --type cssLength of output: 39
Define @keyframes spin for the .loading-spin animation
It looks like we couldn’t locate any @keyframes spin definition in your styles, so the .loading-spin rule has nothing to reference. Please add a keyframes block—e.g., in client/apps/game/src/index.css (near the top or alongside your other global animations):
+ /* Define the spin animation used by .loading-spin */
+ @keyframes spin {
+ from { transform: rotate(0deg); }
+ to { transform: rotate(360deg); }
+ }Then your existing rule will work as intended:
.loading-spin {
animation: spin 1s linear infinite;
}🤖 Prompt for AI Agents
In client/apps/game/src/index.css around lines 703 to 705, the .loading-spin
class uses an animation named spin, but there is no @keyframes spin defined in
the CSS. To fix this, add a @keyframes spin block in the same CSS file,
preferably near the top or with other global animations, defining the spin
animation (e.g., rotating from 0deg to 360deg). This will ensure the
.loading-spin animation works as intended.
| const pointsPrecision = 1_000_000n; | ||
| const registeredPoints = Number(playerRegisteredPoints.registered_points / pointsPrecision); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract magic number into a named constant for better maintainability.
The precision scaling logic is correct, but the hardcoded 1_000_000n should be extracted into a named constant for clarity and maintainability.
+ private static readonly POINTS_PRECISION_FACTOR = 1_000_000n;
+
private getPlayerPoints(): Map<ContractAddress, number> {
const pointsPerPlayer = new Map<ContractAddress, number>();
const registredPoints = runQuery([Has(this.components.PlayerRegisteredPoints)]);
for (const entityId of registredPoints) {
const playerRegisteredPoints = getComponentValue(this.components.PlayerRegisteredPoints, entityId);
console.log({ playerRegisteredPoints });
if (!playerRegisteredPoints) continue;
const playerAddress = ContractAddress(playerRegisteredPoints.address);
- const pointsPrecision = 1_000_000n;
- const registeredPoints = Number(playerRegisteredPoints.registered_points / pointsPrecision);
+ const registeredPoints = Number(playerRegisteredPoints.registered_points / LeaderboardManager.POINTS_PRECISION_FACTOR);
pointsPerPlayer.set(playerAddress, registeredPoints);
}📝 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 pointsPrecision = 1_000_000n; | |
| const registeredPoints = Number(playerRegisteredPoints.registered_points / pointsPrecision); | |
| export class LeaderboardManager { | |
| // Extracted magic number for clarity and easy adjustment | |
| private static readonly POINTS_PRECISION_FACTOR = 1_000_000n; | |
| private getPlayerPoints(): Map<ContractAddress, number> { | |
| const pointsPerPlayer = new Map<ContractAddress, number>(); | |
| const registredPoints = runQuery([Has(this.components.PlayerRegisteredPoints)]); | |
| for (const entityId of registredPoints) { | |
| const playerRegisteredPoints = getComponentValue( | |
| this.components.PlayerRegisteredPoints, | |
| entityId | |
| ); | |
| console.log({ playerRegisteredPoints }); | |
| if (!playerRegisteredPoints) continue; | |
| const playerAddress = ContractAddress(playerRegisteredPoints.address); | |
| const registeredPoints = Number( | |
| playerRegisteredPoints.registered_points / | |
| LeaderboardManager.POINTS_PRECISION_FACTOR | |
| ); | |
| pointsPerPlayer.set(playerAddress, registeredPoints); | |
| } | |
| return pointsPerPlayer; | |
| } | |
| } |
🤖 Prompt for AI Agents
In packages/core/src/managers/leaderboard-manager.ts around lines 97 to 98, the
magic number 1_000_000n used for points precision should be extracted into a
named constant. Define a descriptive constant name for this value at the top of
the file or near the usage, then replace the hardcoded number with this constant
to improve code clarity and maintainability.
| const loadPlayerData = async () => { | ||
| // Create a Map to store address -> structure counts mapping (using bigint keys) | ||
| const playerStructureCountsMap = new Map< | ||
| bigint, | ||
| { | ||
| banks: number; | ||
| mines: number; | ||
| realms: number; | ||
| hyperstructures: number; | ||
| villages: number; | ||
| } | ||
| >(); | ||
|
|
||
| const playerStore = usePlayerStore.getState(); | ||
| const allPlayersData = await playerStore.getAllPlayersData(); | ||
|
|
||
| if (allPlayersData) { | ||
| allPlayersData.forEach((playerData: PlayerDataTransformed) => { | ||
| playerStructureCountsMap.set(BigInt(playerData.ownerAddress), { | ||
| banks: playerData.bankCount ?? 0, | ||
| mines: playerData.mineCount ?? 0, | ||
| realms: playerData.realmsCount ?? 0, | ||
| hyperstructures: playerData.hyperstructuresCount ?? 0, | ||
| villages: playerData.villageCount ?? 0, | ||
| }); | ||
| }); | ||
| } | ||
|
|
||
| setPlayerInfo( | ||
| getPlayerInfo(players, ContractAddress(account.address), playersByRank, playerStructureCountsMap, components), | ||
| ); | ||
| }; | ||
|
|
||
| loadPlayerData(); | ||
| }, [players, account.address, playersByRank, components, setPlayerInfo]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for async player data loading
The refactoring to use the global player store is well-structured, but the async operation lacks error handling which could lead to unhandled promise rejections.
Add error handling to the async function:
const loadPlayerData = async () => {
+ try {
// Create a Map to store address -> structure counts mapping (using bigint keys)
const playerStructureCountsMap = new Map<
bigint,
{
banks: number;
mines: number;
realms: number;
hyperstructures: number;
villages: number;
}
>();
const playerStore = usePlayerStore.getState();
const allPlayersData = await playerStore.getAllPlayersData();
if (allPlayersData) {
allPlayersData.forEach((playerData: PlayerDataTransformed) => {
playerStructureCountsMap.set(BigInt(playerData.ownerAddress), {
banks: playerData.bankCount ?? 0,
mines: playerData.mineCount ?? 0,
realms: playerData.realmsCount ?? 0,
hyperstructures: playerData.hyperstructuresCount ?? 0,
villages: playerData.villageCount ?? 0,
});
});
}
setPlayerInfo(
getPlayerInfo(players, ContractAddress(account.address), playersByRank, playerStructureCountsMap, components),
);
+ } catch (error) {
+ console.error('Failed to load player data:', error);
+ // Optionally set fallback data or show error state
+ }
};🤖 Prompt for AI Agents
In client/apps/game/src/ui/modules/social/social.tsx around lines 59 to 93, the
async function loadPlayerData lacks error handling, which can cause unhandled
promise rejections. Wrap the contents of loadPlayerData in a try-catch block,
and in the catch block, log or handle the error appropriately to ensure any
issues during data fetching are caught and managed gracefully.
| @@ -1,20 +1,27 @@ | |||
| import { shortString } from "starknet"; | |||
| import realms from "../../../../../public/jsons/realms.json"; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using an absolute import path or alias.
The relative path ../../../../../public/jsons/realms.json is fragile and could break if the file structure changes.
Consider using a path alias like @public/jsons/realms.json or moving the JSON file closer to where it's used.
🤖 Prompt for AI Agents
In client/apps/game/src/three/managers/player-data-store.ts at line 2, replace
the fragile relative import path for realms.json with an absolute import path or
alias such as '@public/jsons/realms.json'. Configure the project’s module
resolver or tsconfig paths to support this alias, or alternatively move the JSON
file closer to the code using it to simplify the import path.
| let actualStructureId = structureId.split(":")[0]; | ||
| let actualRealmId = structureId.split(":")[1]; | ||
| this.structureToAddressMap.set(actualStructureId, transformedItem.ownerAddress); | ||
| const realmName = actualRealmId === "0" ? "Village" : realmsData[actualRealmId].name; | ||
| this.structureToNameMap.set(actualStructureId, realmName); |
There was a problem hiding this comment.
Add validation for realm data access to prevent runtime errors.
Accessing realmsData[actualRealmId] without checking if the key exists could throw an error if the realm ID is not in the JSON data.
Apply this diff to add proper validation:
-const realmName = actualRealmId === "0" ? "Village" : realmsData[actualRealmId].name;
+const realmName = actualRealmId === "0" ? "Village" : (realmsData[actualRealmId]?.name || `Realm #${actualRealmId}`);📝 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.
| let actualStructureId = structureId.split(":")[0]; | |
| let actualRealmId = structureId.split(":")[1]; | |
| this.structureToAddressMap.set(actualStructureId, transformedItem.ownerAddress); | |
| const realmName = actualRealmId === "0" ? "Village" : realmsData[actualRealmId].name; | |
| this.structureToNameMap.set(actualStructureId, realmName); | |
| let actualStructureId = structureId.split(":")[0]; | |
| let actualRealmId = structureId.split(":")[1]; | |
| this.structureToAddressMap.set(actualStructureId, transformedItem.ownerAddress); | |
| - const realmName = actualRealmId === "0" ? "Village" : realmsData[actualRealmId].name; | |
| + const realmName = | |
| + actualRealmId === "0" | |
| + ? "Village" | |
| + : (realmsData[actualRealmId]?.name || `Realm #${actualRealmId}`); | |
| this.structureToNameMap.set(actualStructureId, realmName); |
🤖 Prompt for AI Agents
In client/apps/game/src/three/managers/player-data-store.ts around lines 88 to
92, the code accesses realmsData[actualRealmId] without verifying if
actualRealmId exists as a key, which can cause runtime errors. To fix this, add
a check to confirm that realmsData has the actualRealmId key before accessing
its name property; if the key is missing, provide a safe fallback value for
realmName to avoid exceptions.
| export const fetchBattleLogs = async (afterTimestamp?: string): Promise<BattleLogEvent[]> => { | ||
| const whereClause = afterTimestamp ? `WHERE timestamp > '${afterTimestamp ? afterTimestamp : "0"}'` : ""; | ||
|
|
||
| const query = QUERIES.BATTLE_LOGS.replaceAll("{whereClause}", whereClause); | ||
| const url = `${API_BASE_URL}?query=${encodeURIComponent(query)}`; | ||
| const response = await fetch(url); | ||
|
|
||
| if (!response.ok) { | ||
| throw new Error(`Failed to fetch battle logs: ${response.statusText}`); | ||
| } | ||
|
|
||
| const data = await response.json(); | ||
| return data; | ||
| }; |
There was a problem hiding this comment.
Potential SQL injection vulnerability in WHERE clause construction.
The afterTimestamp parameter is directly interpolated into the SQL query without proper sanitization, which could lead to SQL injection attacks.
Apply this diff to properly escape the timestamp parameter:
-const whereClause = afterTimestamp ? `WHERE timestamp > '${afterTimestamp ? afterTimestamp : "0"}'` : "";
+const whereClause = afterTimestamp ? `WHERE timestamp > ${encodeURIComponent(JSON.stringify(afterTimestamp))}` : "";Additionally, consider using parameterized queries if the API supports them.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In client/apps/game/src/services/api.ts around lines 487 to 500, the
afterTimestamp parameter is directly interpolated into the SQL WHERE clause,
creating a potential SQL injection vulnerability. To fix this, sanitize or
escape the afterTimestamp value before including it in the query string, or
preferably refactor the code to use parameterized queries if supported by the
API. This will prevent malicious input from altering the intended SQL command.
| const allLogs = [...newLogs, ...battleLogs]; | ||
|
|
||
| // Sort by timestamp (most recent first) and limit to prevent memory issues | ||
| allLogs.sort((a, b) => b.timestampMs - a.timestampMs); | ||
| const limitedLogs = allLogs; | ||
| const newLastTimestamp = limitedLogs.length > 0 ? limitedLogs[0].timestamp : lastFetchTimestamp; |
There was a problem hiding this comment.
Implement the log limiting to prevent memory issues.
The comment mentions limiting logs to prevent memory issues, but the actual limiting is not implemented. This could lead to unbounded memory growth.
const allLogs = [...newLogs, ...battleLogs];
// Sort by timestamp (most recent first) and limit to prevent memory issues
allLogs.sort((a, b) => b.timestampMs - a.timestampMs);
- const limitedLogs = allLogs;
+ const MAX_LOGS = 1000; // Adjust based on memory constraints
+ const limitedLogs = allLogs.slice(0, MAX_LOGS);
const newLastTimestamp = limitedLogs.length > 0 ? limitedLogs[0].timestamp : lastFetchTimestamp;📝 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 allLogs = [...newLogs, ...battleLogs]; | |
| // Sort by timestamp (most recent first) and limit to prevent memory issues | |
| allLogs.sort((a, b) => b.timestampMs - a.timestampMs); | |
| const limitedLogs = allLogs; | |
| const newLastTimestamp = limitedLogs.length > 0 ? limitedLogs[0].timestamp : lastFetchTimestamp; | |
| const allLogs = [...newLogs, ...battleLogs]; | |
| // Sort by timestamp (most recent first) and limit to prevent memory issues | |
| allLogs.sort((a, b) => b.timestampMs - a.timestampMs); | |
| const MAX_LOGS = 1000; // Adjust based on memory constraints | |
| const limitedLogs = allLogs.slice(0, MAX_LOGS); | |
| const newLastTimestamp = limitedLogs.length > 0 ? limitedLogs[0].timestamp : lastFetchTimestamp; |
🤖 Prompt for AI Agents
In client/apps/game/src/hooks/store/use-battle-logs-store.ts around lines 165 to
170, the code merges and sorts logs but does not implement the intended limit on
the number of logs to prevent memory issues. Fix this by slicing the sorted
allLogs array to a maximum allowed length (e.g., a constant limit) before
assigning it to limitedLogs, ensuring the logs array size is capped to avoid
unbounded memory growth.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
client/apps/game/src/services/api.ts (1)
488-488: SQL injection vulnerability needs to be addressed.This security issue has already been identified in a previous review. The
afterTimestampparameter must be properly sanitized before interpolation.
🧹 Nitpick comments (2)
client/apps/game/src/ui/components/events/battle-logs-table.tsx (2)
66-193: Consider implementing entity resolution caching.The entity resolution logic re-fetches data for the same entities on every render. Consider implementing a caching mechanism to improve performance, especially when dealing with large datasets.
Would you like me to help implement a caching layer for entity resolution to improve performance?
866-877: Remove commented-out code.This commented block appears to be redundant as the loading state is already handled earlier in the component. Clean up the code by removing it.
- {/* This block is now handled by the main isLoading && battleLogs.length === 0 check further up */} - { - /* {isLoading && battleLogs.length === 0 ? ( - <div className="min-h-[300px] flex flex-col items-center justify-center p-5 text-center"> - <div className="bg-stone-900/80 p-10 rounded-lg shadow-2xl backdrop-blur-md border border-amber-600/40"> - <RefreshCw className="w-8 h-8 text-amber-400 animate-spin mx-auto mb-4" /> - <h3 className="text-lg font-semibold text-amber-300">Loading Chronicles...</h3> - <p className="text-amber-500/80 text-sm max-w-xs mt-1"> - Gathering latest reports from the battlefield. - </p> - </div> - </div> - ) : */ filteredLogs.length === 0 ? ( + {filteredLogs.length === 0 ? (
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
client/apps/game/src/services/api.ts(5 hunks)client/apps/game/src/three/managers/player-data-store.ts(5 hunks)client/apps/game/src/ui/components/events/battle-logs-table.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- client/apps/game/src/three/managers/player-data-store.ts
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: test-various (quest)
- GitHub Check: test-various (test_troop_movement)
- GitHub Check: test-various (models)
- GitHub Check: test-various (troop_management)
- GitHub Check: test-season-pass
🔇 Additional comments (5)
client/apps/game/src/services/api.ts (3)
136-163: LGTM! Well-structured UNION query.The BATTLE_LOGS query correctly combines data from two event tables with proper column mapping and ordering. The use of NULL values for missing columns in each table ensures consistent output structure.
212-229: LGTM! Well-typed interfaces.The new count fields in
PlayersDatacorrectly correspond to the SQL query changes, and theBattleLogEventinterface properly handles nullable fields and uses appropriate types.
53-53:❓ Verification inconclusive
Consider the performance impact of concatenating multiple fields.
The
GROUP_CONCATnow includes realm_id and category in addition to entity_id, which could impact performance with large datasets. Ensure appropriate indexes exist on these columns.
🏁 Script executed:
#!/bin/bash # Check if there are database migration files that might contain index definitions fd -e sql -e ts -e js migration | head -20Length of output: 44
Verify index coverage for GROUP_CONCAT columns
I couldn’t find any migration or index definitions in the repo. To avoid query slow-downs when concatenating
entity_id,metadata.realm_id, andcategoryover large tables, please confirm that appropriate indexes exist on:
- client/apps/game/src/services/api.ts (line 53):
GROUP_CONCAT(DISTINCT s.entity_id || ':' || s.\metadata.realm_id` || ':' || s.`category`)`- Columns to index:
entity_idmetadata.realm_idcategoryclient/apps/game/src/ui/components/events/battle-logs-table.tsx (2)
675-708: LGTM! Clean reward display implementation.The reward section correctly handles resource amount conversion using RESOURCE_PRECISION and provides good UX by showing "~ 0" for very small amounts.
59-59:❌ Incorrect review comment
Verify the player name comparison logic.
The string comparison
name == "0"might not work as expected if the API returns a numeric 0. Consider using a more robust check.const cleanPlayerName = (name: string) => { - if (name == "0") { + if (!name || name === "0" || name === "") { return ""; } return name; };
🏁 Script executed:
#!/bin/bash # Check how player names are used in the codebase to understand the expected format rg -A 3 -B 3 "player_name|playerName" --type tsLength of output: 25104
🏁 Script executed:
#!/bin/bash # Extract cleanPlayerName function with surrounding context rg -A5 -B5 "const cleanPlayerName" client/apps/game/src/ui/components/events/battle-logs-table.tsxLength of output: 309
No change needed for cleanPlayerName
The
cleanPlayerNamehelper always receives a string (even numeric zeros are converted to""by the data store), so:
name == "0"is equivalent toname === "0"here.- Empty strings already pass through correctly (returning
"").- There’s no case where
nameis a number.You can safely ignore the previous suggestion.
Likely an incorrect or invalid review comment.
| className={`w-4 h-4 ${ | ||
| filterState.eventType === key ? `text-${activeColor}` : `text-${baseColor} group-hover:text-amber-300` | ||
| }`} |
There was a problem hiding this comment.
Dynamic Tailwind classes may not work as expected.
Tailwind CSS requires complete class names to be present in the code for proper compilation. Using template literals with dynamic values like `text-${activeColor}` won't work reliably.
Consider using a class map approach:
- className={`w-4 h-4 ${
- filterState.eventType === key ? `text-${activeColor}` : `text-${baseColor} group-hover:text-amber-300`
- }`}
+ className={`w-4 h-4 ${
+ filterState.eventType === key
+ ? (key === 'battles' ? 'text-red-500' : key === 'raids' ? 'text-violet-500' : 'text-amber-400')
+ : 'text-stone-400 group-hover:text-amber-300'
+ }`}Also applies to: 481-481
🤖 Prompt for AI Agents
In client/apps/game/src/ui/components/events/battle-logs-table.tsx around lines
476 to 478 and line 481, the use of dynamic Tailwind CSS classes with template
literals like `text-${activeColor}` prevents Tailwind from generating the
necessary styles. Replace these dynamic class names with a class map object that
explicitly maps each possible color key to its full Tailwind class string, then
select the appropriate class from this map based on the current state. This
ensures all required classes are statically present in the code for Tailwind to
compile correctly.
Note: Battle history PR needs to be merged first
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores