fix: nextBlockTimestamp refactor #2405
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
WalkthroughThis pull request introduces a new custom hook, Changes
Possibly related PRs
Suggested reviewers
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 (
|
|
You are out of MentatBot reviews. Your usage will refresh December 16 at 08:00 AM. |
|
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: 2
🧹 Outside diff range and nitpick comments (14)
client/src/hooks/useNextBlockTimestamp.tsx (2)
6-8: Consider adding validation for timestamp initializationThe initial state setup looks good, but consider adding validation to ensure the timestamp is always positive and within reasonable bounds.
- const [nextBlockTimestamp, setNextBlockTimestamp] = useState<number>(Math.floor(Date.now() / 1000)); + const [nextBlockTimestamp, setNextBlockTimestamp] = useState<number>(() => { + const timestamp = Math.floor(Date.now() / 1000); + return timestamp > 0 ? timestamp : 0; + });
10-29: Consider optimizing update frequencyThe implementation looks good and aligns with the PR's goal of improving UI responsiveness. A few suggestions:
- The 10-second interval might be too frequent for blockchain state. Consider making it configurable.
- The tick calculations could be memoized if they're used frequently.
useEffect(() => { + const UPDATE_INTERVAL = 10000; // Consider making this configurable const tickConfigArmies = configManager.getTick(TickIds.Armies); const tickConfigDefault = configManager.getTick(TickIds.Default); + + // Memoize tick calculations if used frequently + const calculateTicks = (timestamp: number) => ({ + defaultTick: Math.floor(timestamp / Number(tickConfigDefault)), + armiesTick: Math.floor(timestamp / Number(tickConfigArmies)) + }); const updateTimestamp = () => { const timestamp = Math.floor(Date.now() / 1000); + const { defaultTick, armiesTick } = calculateTicks(timestamp); startTransition(() => { setNextBlockTimestamp(timestamp); - setCurrentDefaultTick(Math.floor(timestamp / Number(tickConfigDefault))); - setCurrentArmiesTick(Math.floor(timestamp / Number(tickConfigArmies))); + setCurrentDefaultTick(defaultTick); + setCurrentArmiesTick(armiesTick); }); }; - const intervalId = setInterval(updateTimestamp, 10000); + const intervalId = setInterval(updateTimestamp, UPDATE_INTERVAL);client/src/hooks/store/useBlockchainStore.tsx (1)
25-25: Remove unused isPending variableThe isPending state from useTransition is declared but never used. Consider removing it if not needed.
-const [isPending, startTransition] = useTransition(); +const [, startTransition] = useTransition();client/src/ui/components/worldmap/structures/StructureLabel.tsx (1)
53-56: Consider memoizing the timestamp-dependent calculationsWhile the refactoring improves UI update lag, these calculations run on every render. Consider using useMemo for isImmune and timer if they're used in frequently updating components.
const { nextBlockTimestamp } = useNextBlockTimestamp(); - const isImmune = useIsStructureImmune(structure, nextBlockTimestamp || 0); - const timer = useStructureImmunityTimer(structure as Structure, nextBlockTimestamp || 0); + const isImmune = useMemo(() => + useIsStructureImmune(structure, nextBlockTimestamp || 0), + [structure, nextBlockTimestamp] + ); + const timer = useMemo(() => + useStructureImmunityTimer(structure as Structure, nextBlockTimestamp || 0), + [structure, nextBlockTimestamp] + );client/src/ui/modules/entity-details/realm/RealmDetails.tsx (2)
Line range hint
48-49: Consider memoizing the timestamp-dependent calculationsSimilar to StructureLabel.tsx, these calculations run on every render. Consider memoizing them for better performance.
- const isImmune = useIsStructureImmune(structure, nextBlockTimestamp || 0); - const timer = useStructureImmunityTimer(structure as Structure, nextBlockTimestamp || 0); + const isImmune = useMemo(() => + useIsStructureImmune(structure, nextBlockTimestamp || 0), + [structure, nextBlockTimestamp] + ); + const timer = useMemo(() => + useStructureImmunityTimer(structure as Structure, nextBlockTimestamp || 0), + [structure, nextBlockTimestamp] + );
Line range hint
1-1: Architectural Review: Solid refactoring approachThe centralization of nextBlockTimestamp access through a custom hook is a good architectural decision that:
- Reduces UI update lag as intended
- Improves maintainability by centralizing timestamp logic
- Makes the code more testable by isolating the timestamp functionality
Consider adding memoization to timestamp-dependent calculations across components to further optimize performance.
client/src/ui/modules/entity-details/EnemyArmies.tsx (1)
Line range hint
46-92: Fix useCallback dependencies and memoizationThe
useCallbackdependencies array includes theuseIsStructureImmunehook, which is incorrect as hooks should not be used as dependencies.Apply this fix:
[ nextBlockTimestamp, ownArmySelected, ownArmySelected?.entity_id, position, getEntityInfo, - useIsStructureImmune, setBattleView, setTooltip, structureAtPosition, ownArmyIsImmune, dojo, ]Additionally, consider memoizing the
isImmunecalculation to avoid recalculating it on every render:const getArmyChip = useCallback( (army: ArmyInfo, index: number) => { const structure = getEntityInfo(army.entityOwner.entity_owner_id).structure; - const isImmune = useIsStructureImmune(structure, nextBlockTimestamp!) || ownArmyIsImmune; + const isImmune = useMemo( + () => useIsStructureImmune(structure, nextBlockTimestamp!) || ownArmyIsImmune, + [structure, nextBlockTimestamp, ownArmyIsImmune] + );client/src/ui/components/battles/BattleListItem.tsx (2)
28-28: Consider memoizing battleManager initializationThe
battleManagerinitialization could be moved inside theupdatedBattlememo to avoid unnecessary recalculations.Here's a suggested improvement:
- const battleManager = useMemo(() => new BattleManager(battle.entity_id, dojo), [battle]); - - const updatedBattle = useMemo(() => { - const updatedBattle = battleManager.getUpdatedBattle(nextBlockTimestamp!); - return updatedBattle; - }, [nextBlockTimestamp]); + const updatedBattle = useMemo(() => { + const battleManager = new BattleManager(battle.entity_id, dojo); + return battleManager.getUpdatedBattle(nextBlockTimestamp!); + }, [battle.entity_id, dojo, nextBlockTimestamp]);Also applies to: 39-42
Line range hint
51-86: Add type safety for button generationThe buttons memo could benefit from better type safety and error handling.
Consider this improvement:
const buttons = useMemo(() => { if (!nextBlockTimestamp) return []; + if (!updatedBattle) return []; + if (ownArmySelected?.entity_id === undefined) return [eyeButton]; const isBattleOngoing = battleManager.isBattleOngoing(nextBlockTimestamp); // ... rest of the code }, [nextBlockTimestamp, battleManager, ownArmySelected, updatedBattle]);client/src/ui/components/worldmap/structures/StructureListItem.tsx (2)
Line range hint
58-62: Consider improving error handling for undefined timestampInstead of throwing an error, consider providing a fallback value or handling the undefined case more gracefully to prevent potential UI disruptions.
-if (!nextBlockTimestamp) throw new Error("Current timestamp is undefined"); +if (!nextBlockTimestamp) { + console.warn("Current timestamp is undefined, using fallback value"); + return { updatedBattle: null }; +}
Line range hint
69-70: Consider consistent error handling approachSimilar to the previous suggestion, consider handling undefined timestamp more gracefully here as well.
-if (!nextBlockTimestamp) throw new Error("Current timestamp is undefined"); +if (!nextBlockTimestamp) { + console.warn("Current timestamp is undefined, using fallback value"); + return []; +}client/src/hooks/helpers/useResources.tsx (1)
Line range hint
147-147: Consider deprecating useBalance hookThe comment suggests deprecating this hook in favor of getBalance, and the presence of "too many useEffects" indicates potential performance concerns.
Consider removing this hook and updating all consumers to use getBalance instead, which would align better with the PR's performance improvement goals.
client/src/ui/components/trading/MarketOrderPanel.tsx (2)
500-501: Consider consolidating hook callsThe component has two separate calls to
useNextBlockTimestamphook. While this works, it's better to consolidate them into a single call at the top of the component for better maintainability.- const { nextBlockTimestamp } = useNextBlockTimestamp(); + const { nextBlockTimestamp, currentDefaultTick } = useNextBlockTimestamp(); const { play: playLordsSound } = useUiSounds(soundSelector.addLords); // ... other code ... - const { currentDefaultTick } = useNextBlockTimestamp();Also applies to: 562-563
Line range hint
1-700: LGTM: Overall implementationThe migration to
useNextBlockTimestamphook is well-implemented across all components. This change aligns with the PR objective of fixing UI lag by centralizing timestamp management.Key improvements:
- Consistent usage pattern across components
- Proper separation of concerns
- Improved maintainability
Consider documenting the performance improvement in the component's comments for future reference.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (26)
client/src/hooks/helpers/use-resource-arrivals.tsx(2 hunks)client/src/hooks/helpers/useResources.tsx(2 hunks)client/src/hooks/helpers/useStructures.tsx(2 hunks)client/src/hooks/helpers/useTrade.tsx(3 hunks)client/src/hooks/store/useBlockchainStore.tsx(3 hunks)client/src/hooks/store/useLeaderBoardStore.tsx(2 hunks)client/src/hooks/useNextBlockTimestamp.tsx(1 hunks)client/src/ui/components/battles/BattleListItem.tsx(2 hunks)client/src/ui/components/hyperstructures/CoOwners.tsx(2 hunks)client/src/ui/components/hyperstructures/Leaderboard.tsx(2 hunks)client/src/ui/components/military/ArmyChip.tsx(2 hunks)client/src/ui/components/resources/EntityResourceTable.tsx(2 hunks)client/src/ui/components/structures/worldmap/StructureCard.tsx(2 hunks)client/src/ui/components/trading/MarketOrderPanel.tsx(5 hunks)client/src/ui/components/worldmap/armies/ArmyInfoLabel.tsx(2 hunks)client/src/ui/components/worldmap/structures/StructureLabel.tsx(3 hunks)client/src/ui/components/worldmap/structures/StructureListItem.tsx(2 hunks)client/src/ui/elements/StaminaResource.tsx(1 hunks)client/src/ui/elements/StaminaResourceCost.tsx(2 hunks)client/src/ui/modules/entity-details/EnemyArmies.tsx(2 hunks)client/src/ui/modules/entity-details/realm/RealmDetails.tsx(2 hunks)client/src/ui/modules/military/battle-view/BattleActions.tsx(2 hunks)client/src/ui/modules/military/battle-view/BattleProgress.tsx(2 hunks)client/src/ui/modules/military/battle-view/BattleSideView.tsx(2 hunks)client/src/ui/modules/military/battle-view/BattleView.tsx(2 hunks)client/src/ui/modules/navigation/TopLeftNavigation.tsx(5 hunks)
🔇 Additional comments (37)
client/src/ui/modules/military/battle-view/BattleSideView.tsx (2)
7-7: LGTM: Clean import of new hook
The import statement follows the project's conventions and aligns with the timestamp refactoring objective.
47-47: Verify non-null assertion on timestamp usage
The implementation looks good and maintains consistent naming. However, let's verify the non-null assertion (currentTimestamp!) is safe in the isActive calculation.
✅ Verification successful
Let me gather more information about the hook implementation and the battleManager.isBattleOngoing usage.
Let me check one more thing to understand how the timestamp is being used in the battle context.
Based on the comprehensive analysis of the codebase, I can now provide the final response:
Non-null assertion on currentTimestamp is safe in this context
The non-null assertion on currentTimestamp in isBattleOngoing() is safe because:
- The
useNextBlockTimestamphook is consistently used across the codebase and always returns a value - The
BattleManager.isBattleOngoing()method has proper null checks and defensive programming - The battle system's core logic handles timestamp validation appropriately
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that useNextBlockTimestamp always returns a non-null value
# and check its usage pattern across the codebase
# Check the hook implementation
ast-grep --pattern 'export default function useNextBlockTimestamp() {
$$$
return {
$$$
}
}'
# Check other usages of the hook for null checks
rg -A 2 'useNextBlockTimestamp\(\)'
Length of output: 8741
Script:
#!/bin/bash
# Check the hook implementation
ast-grep --pattern 'function useNextBlockTimestamp() {
$$$
}'
# Check battleManager.isBattleOngoing implementation and usage
ast-grep --pattern 'isBattleOngoing($$$) {
$$$
}'
# Check if there are any null checks on nextBlockTimestamp
rg 'nextBlockTimestamp.*\|\|.*null'
rg 'nextBlockTimestamp.*===.*null'
rg 'nextBlockTimestamp.*\?\?'
Length of output: 1340
Script:
#!/bin/bash
# Check the battle-related files for timestamp handling
rg -A 3 'currentTimestamp' client/src/ui/modules/military/battle-view/
rg -A 3 'nextBlockTimestamp' client/src/ui/modules/military/battle-view/
# Check if there are any defensive checks around timestamp in battle manager
ast-grep --pattern 'class BattleManager {
$$$
}'
Length of output: 57796
client/src/hooks/useNextBlockTimestamp.tsx (1)
31-32: LGTM: Clean return interface
The hook's return interface is clean and well-structured, providing all necessary timestamp-related values.
client/src/ui/elements/StaminaResourceCost.tsx (1)
17-17: LGTM: Clean migration to useNextBlockTimestamp
The migration from useUIStore to useNextBlockTimestamp is clean and maintains the component's functionality while potentially improving its performance.
client/src/ui/components/resources/EntityResourceTable.tsx (2)
13-13: LGTM: Clean destructuring of currentDefaultTick
The migration to useNextBlockTimestamp is clean and the destructuring with rename to 'tick' maintains good code readability.
1-1: Verify the impact on UI responsiveness
The implementation looks solid, but let's verify that it actually improves UI responsiveness as intended.
✅ Verification successful
Based on the gathered evidence, I can now generate the final response:
Implementation confirms improved UI responsiveness
The timestamp management implementation shows solid improvements to UI responsiveness through:
- Centralized timestamp management via
configManagersingleton - React 18 concurrent features usage (
useTransition,createRoot,StrictMode) - Optimized update interval (10-second) with batched state updates
- Proper cleanup of intervals to prevent memory leaks
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential performance bottlenecks in timestamp usage
# Look for direct Date.now() calls that might bypass the centralized management
rg "Date\.now\(\)" --type tsx --type ts
# Check for other timestamp-related state management that might conflict
rg "useUIStore.*timestamp" --type tsx --type ts
# Look for potential multiple interval instances
rg "setInterval.*timestamp" --type tsx --type ts
Length of output: 226
Script:
#!/bin/bash
# Description: Check for potential performance bottlenecks in timestamp usage
# Look for direct Date.now() calls that might bypass the centralized management
rg "Date\.now\(\)"
# Check for timestamp-related state management
rg "useUIStore.*timestamp"
# Look for potential multiple interval instances
rg "setInterval.*timestamp"
# Check configManager usage for timestamp management
rg "configManager.*timestamp"
# Look for any React performance hooks usage
rg "useCallback|useMemo|useEffect.*timestamp"
Length of output: 58407
Script:
#!/bin/bash
# Check for timestamp management implementation
ast-grep --pattern 'const timestamp = configManager.getTimestamp()'
# Look for timestamp update intervals
rg "setInterval.*configManager.*timestamp"
# Check for React concurrent features usage
rg "useTransition|useDeferredValue"
# Look for configManager implementation
rg -A 5 "class.*configManager"
Length of output: 1543
Script:
#!/bin/bash
# Check configManager implementation in setup
rg -A 10 "configManager.*=" client/src/dojo/setup.ts
# Look for timestamp-related state updates
rg -A 5 "setNextBlockTimestamp|updateTimestamp" client/src/
# Check for React concurrent mode setup
rg "<React.StrictMode|createRoot" client/src/
Length of output: 3698
client/src/hooks/store/useLeaderBoardStore.tsx (1)
7-7: LGTM: Clean migration to useNextBlockTimestamp
The change from useUIStore to useNextBlockTimestamp is clean and maintains the same functionality. The dependency array in useCallback correctly includes nextBlockTimestamp to ensure proper updates.
Let's verify this is the only instance where nextBlockTimestamp was used from useUIStore in this file:
Also applies to: 30-30
✅ Verification successful
LGTM: Migration to useNextBlockTimestamp is complete and correct
The file content confirms that the migration from useUIStore to useNextBlockTimestamp is complete. There are no remaining references to useUIStore, and nextBlockTimestamp is correctly imported and used within the useHyperstructureData hook's updateLeaderboard callback with proper dependency tracking.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining useUIStore references related to nextBlockTimestamp
rg -l "useUIStore.*nextBlockTimestamp" "client/src/hooks/store/useLeaderBoardStore.tsx"
Length of output: 87
Script:
#!/bin/bash
# Let's first check the entire content of the file to understand the context better
cat client/src/hooks/store/useLeaderBoardStore.tsx
# Also search for any useUIStore references in the file
rg "useUIStore" client/src/hooks/store/useLeaderBoardStore.tsx
Length of output: 1878
client/src/hooks/store/useBlockchainStore.tsx (2)
25-25: Excellent performance optimization using useTransition
The implementation of useTransition and state batching is a good approach to reduce UI lag. The startTransition wrapper helps prevent UI freezing during updates by marking them as non-urgent.
Also applies to: 41-41
Line range hint 44-48: Consider re-implementing the interval updates
The commented-out interval code suggests that periodic updates were previously implemented. Consider whether this should be re-implemented to ensure timely updates.
Let's check if this functionality exists elsewhere:
Would you like help implementing a debounced interval update that works well with the new useTransition implementation?
client/src/ui/elements/StaminaResource.tsx (1)
6-6: LGTM: Consistent with refactoring pattern
The migration to useNextBlockTimestamp for currentArmiesTick is clean and maintains the same functionality. The useMemo dependencies are correctly updated to reflect this change.
Let's verify the migration is complete and no timestamp-related useUIStore usages remain:
Also applies to: 14-14
✅ Verification successful
Migration to useNextBlockTimestamp is complete and correct
The verification confirms that useUIStore is now only used for tooltip functionality (setTooltip), while timestamp-related logic has been properly migrated to useNextBlockTimestamp. The component's dependencies and functionality are maintained correctly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining useUIStore references related to timestamps
rg "useUIStore.*(?:nextBlockTimestamp|currentArmiesTick|currentDefaultTick)" "client/src/ui/elements/StaminaResource.tsx"
Length of output: 121
Script:
#!/bin/bash
# Let's first check all useUIStore usages in the file
rg "useUIStore" "client/src/ui/elements/StaminaResource.tsx" -A 2
# Also check the complete file content to understand the context better
cat "client/src/ui/elements/StaminaResource.tsx"
Length of output: 2845
client/src/ui/components/worldmap/structures/StructureLabel.tsx (1)
9-9: LGTM: Clean import of the new hook
The addition of useNextBlockTimestamp import aligns with the PR's objective to centralize timestamp access.
client/src/ui/components/hyperstructures/Leaderboard.tsx (2)
5-5: LGTM: Clean import of the new hook
The addition of useNextBlockTimestamp import maintains consistency with the refactoring approach.
Line range hint 29-35: LGTM: Proper memoization of timestamp-dependent calculation
The playerPointsLeaderboard calculation is correctly memoized with nextBlockTimestamp as a dependency, which helps prevent unnecessary recalculations.
client/src/ui/modules/entity-details/realm/RealmDetails.tsx (1)
8-8: LGTM: Clean implementation of the new hook
The addition and usage of useNextBlockTimestamp maintains consistency with the refactoring approach.
Also applies to: 19-20
client/src/ui/modules/entity-details/EnemyArmies.tsx (1)
8-8: LGTM: Clean migration to useNextBlockTimestamp hook
The migration from direct store access to the new custom hook is implemented correctly.
Also applies to: 27-27
client/src/ui/modules/military/battle-view/BattleView.tsx (1)
7-7: LGTM: Clean implementation of the new hook
The replacement of the UI store with the dedicated useNextBlockTimestamp hook is clean and maintains the existing component logic.
Also applies to: 18-18
client/src/hooks/helpers/use-resource-arrivals.tsx (1)
16-16: LGTM: Proper integration of timestamp hook in arrival calculations
The integration of useNextBlockTimestamp maintains the existing arrival timing logic while potentially improving UI update performance.
Also applies to: 171-171
client/src/ui/components/worldmap/structures/StructureListItem.tsx (1)
10-10: LGTM: Clean hook integration
The new hook is properly imported and integrated into the component.
Also applies to: 43-44
client/src/hooks/helpers/useTrade.tsx (3)
10-10: LGTM: Clean import addition
The addition of useNextBlockTimestamp import aligns with the PR's objective to centralize timestamp management.
145-145: LGTM: Improved timestamp management in useGetMyOffers
The replacement of useUIStore with useNextBlockTimestamp for timestamp retrieval is a clean improvement that should help reduce UI update lag.
170-170: LGTM: Consistent timestamp management in useSetMarket
The implementation maintains consistency with the new timestamp management approach.
client/src/ui/modules/military/battle-view/BattleProgress.tsx (2)
4-4: LGTM: Clean import addition
The addition of useNextBlockTimestamp import is consistent with the centralized timestamp management approach.
138-138: LGTM: Improved timestamp management with clear aliasing
Good use of destructuring with alias for clarity. The nextBlockTimestamp: currentTimestamp maintains readable code while adopting the new timestamp management approach.
client/src/hooks/helpers/useResources.tsx (1)
18-18: LGTM: Clean import addition
The addition of useNextBlockTimestamp import aligns with the centralized timestamp management approach.
client/src/hooks/helpers/useStructures.tsx (1)
11-11: LGTM: Clean timestamp source refactor
The refactoring of useIsResourcesLocked to use the new useNextBlockTimestamp hook is clean and maintains the existing logic while potentially improving performance.
Also applies to: 248-249
client/src/ui/components/hyperstructures/CoOwners.tsx (2)
7-7: LGTM: Clean import and hook usage
The addition of useNextBlockTimestamp and its integration is clean and follows best practices.
Also applies to: 63-63
Line range hint 71-81: Verify null safety in canUpdate logic
The null check for nextBlockTimestamp is correctly implemented, preventing potential issues when the timestamp is not available.
✅ Verification successful
Based on the comprehensive search results, I can now provide the final response:
Null safety in canUpdate is properly implemented with consistent patterns
The null check for nextBlockTimestamp in the canUpdate logic is consistent with the codebase's patterns. The evidence shows:
- The type definition in
useBlockchainStore.tsxdeclaresnextBlockTimestampasnumber | undefined - The
useNextBlockTimestamphook is used consistently across the codebase - Other components follow similar null-checking patterns when using this value
- The implementation matches the type safety requirements of the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other usages of nextBlockTimestamp to ensure consistent null checking
rg -A 2 'nextBlockTimestamp\!'
Length of output: 137
Script:
#!/bin/bash
# Search for usages of nextBlockTimestamp to verify null safety patterns
rg -A 2 'nextBlockTimestamp'
Length of output: 33116
Script:
#!/bin/bash
# Search for the component definition to understand the context
ast-grep --pattern 'const $_: FC = () => {
$$$
const nextBlockTimestamp = $$$
$$$
}'
Length of output: 92
Script:
#!/bin/bash
# Search for any potential type definitions of nextBlockTimestamp
rg 'nextBlockTimestamp.*:.*' -A 1
Length of output: 2961
client/src/ui/components/military/ArmyChip.tsx (2)
10-10: LGTM: Clean timestamp hook integration
The addition of useNextBlockTimestamp and its integration is clean and follows best practices.
Also applies to: 75-75
Line range hint 84-88: Verify battle state calculation with new timestamp source
The updatedArmy calculation correctly uses the new timestamp source and maintains proper memoization. However, we should verify the battle manager's timestamp handling.
✅ Verification successful
Timestamp handling in BattleManager is correctly implemented
The getUpdatedBattle method in BattleManager properly handles the timestamp parameter by:
- Using it to calculate elapsed time since last update
- Correctly updating army health based on the time delta
- Properly handling siege timing conditions
- Maintaining battle state consistency with the new timestamp source
The implementation aligns with the usage in ArmyChip.tsx and maintains proper battle state calculations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check BattleManager's timestamp handling
ast-grep --pattern 'class BattleManager {
$$$
getUpdatedBattle($_) {
$$$
}
$$$
}'
Length of output: 95
Script:
#!/bin/bash
# Let's try to find BattleManager implementation with ripgrep
rg -l "class.*BattleManager" --type typescript
# Then look for getUpdatedBattle method
rg "getUpdatedBattle" --type typescript -A 5
Length of output: 163
Script:
#!/bin/bash
# Search for BattleManager class without type filter
rg -l "class.*BattleManager"
# Search for getUpdatedBattle method implementation with context
rg "getUpdatedBattle" -A 5
Length of output: 10766
Script:
#!/bin/bash
# Let's examine the BattleManager implementation
cat client/src/dojo/modelManager/BattleManager.ts
Length of output: 16598
client/src/ui/modules/military/battle-view/BattleActions.tsx (1)
28-28: LGTM: Improved performance by consolidating timestamp access
The refactoring from useUIStore to useNextBlockTimestamp hook is a good improvement that:
- Reduces store subscriptions
- Centralizes timestamp-related state management
- Maintains the same functionality while potentially improving UI update performance
Also applies to: 71-71
client/src/ui/modules/navigation/TopLeftNavigation.tsx (2)
7-7: LGTM: Improved state management for timestamps
The refactoring from useUIStore to useNextBlockTimestamp hook is well implemented and maintains consistency with the codebase-wide changes.
Also applies to: 108-109
362-375: Good improvement in tooltip content memoization
The memoization of tooltip content has been properly updated with the correct dependencies, which should prevent unnecessary re-renders.
client/src/ui/components/structures/worldmap/StructureCard.tsx (1)
8-8: LGTM: Consistent implementation of timestamp management
The refactoring aligns with the codebase-wide changes and maintains the same functionality while potentially improving UI responsiveness.
Also applies to: 39-39
client/src/ui/components/trading/MarketOrderPanel.tsx (4)
8-8: LGTM: Clean import of the new hook
The import statement for useNextBlockTimestamp is properly placed with other hook imports.
50-50: LGTM: Proper hook usage in MarketResource
The currentDefaultTick is correctly destructured from the hook and used for balance calculations.
296-296: LGTM: Proper hook usage in OrderRow
The currentDefaultTick is correctly destructured and used for donkey balance calculations.
500-501: Verify the expiration timestamp calculation
The nextBlockTimestamp is used to set the order expiration time. Ensure that adding ONE_MONTH to this timestamp aligns with the business requirements for order expiration.
✅ Verification successful
Let me gather more context about the expiration timestamp calculation.
Based on the gathered information, I can now provide a final response:
Order expiration calculation is correctly implemented
The expiration timestamp calculation using nextBlockTimestamp + ONE_MONTH is consistent with the system's design and implementation:
ONE_MONTHis defined as 2,628,000 seconds (approximately 30.42 days) insdk/packages/eternum/src/constants/global.ts- The contract validates expiration by checking
trade.expires_at > tswheretsis the current block timestamp - The expiration is used consistently across the trading system for both order creation and acceptance
- The UI displays the expiration time to users in a readable format using
new Date(offer.expiresAt * 1000).toLocaleString()
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other usages of ONE_MONTH constant to verify consistency
rg -A 2 "ONE_MONTH"
# Check for other order expiration calculations
ast-grep --pattern 'expires_at: $_ + ONE_MONTH'
Length of output: 743
Script:
#!/bin/bash
# Check for other order expiration related code
rg -A 5 "expires_at"
# Check for any tests or validations related to order expiration
rg -A 5 "expir"
# Look for order-related types or interfaces
ast-grep --pattern 'interface $_ {
$$$
expires_at
$$$
}'
Length of output: 33469
| const { nextBlockTimestamp } = useNextBlockTimestamp(); | ||
|
|
||
| const isImmune = useIsStructureImmune(structure, nextBlockTimestamp || 0); | ||
| const timer = useStructureImmunityTimer(structure as Structure, nextBlockTimestamp || 0); |
There was a problem hiding this comment.
Consider handling undefined nextBlockTimestamp more gracefully
Using 0 as a fallback for nextBlockTimestamp could lead to incorrect immunity and timer calculations. Consider showing a loading state or error message when the timestamp is not available.
Here's a suggested improvement:
- const { nextBlockTimestamp } = useNextBlockTimestamp();
+ const { nextBlockTimestamp } = useNextBlockTimestamp();
+
+ if (!nextBlockTimestamp) {
+ return (
+ <BaseThreeTooltip position={Position.CLEAN} className="pointer-events-none w-[250px]">
+ <div>Loading...</div>
+ </BaseThreeTooltip>
+ );
+ }
- const isImmune = useIsStructureImmune(structure, nextBlockTimestamp || 0);
- const timer = useStructureImmunityTimer(structure as Structure, nextBlockTimestamp || 0);
+ const isImmune = useIsStructureImmune(structure, nextBlockTimestamp);
+ const timer = useStructureImmunityTimer(structure as Structure, nextBlockTimestamp);Committable suggestion skipped: line range outside the PR's diff.
| @@ -27,7 +28,7 @@ export function useResourcesUtils() { | |||
| }, []); | |||
|
|
|||
| const useResourcesFromBalance = (entityId: ID) => { | |||
| const currentDefaultTick = useUIStore((state) => state.currentDefaultTick); | |||
| const { currentDefaultTick } = useNextBlockTimestamp(); | |||
There was a problem hiding this comment.
Inconsistent timestamp retrieval methods detected
Several functions still use useUIStore.getState().currentDefaultTick while others have been updated to use useNextBlockTimestamp. This inconsistency could lead to synchronization issues in timestamp values.
Consider updating these instances to use the new hook:
- const currentDefaultTick = useUIStore.getState().currentDefaultTick;
+ const { currentDefaultTick } = useNextBlockTimestamp();Affected functions:
- getResourcesFromBalance
- getRealmsWithSpecificResource
- getFoodResources
- getBalance
- useBalance
Also applies to: 49-49, 89-89, 126-126, 147-147
The lag in updating UI components dependent on nextBlockTimestamp has been eliminated, and instead of 300-400ms, it is now unnoticeable.
Summary by CodeRabbit
New Features
useNextBlockTimestamp, for managing block timestamps across various components.Bug Fixes
nextBlockTimestampto prevent undefined errors in dependent calculations.Refactor
nextBlockTimestampfrom the UI store with the new hook in multiple components, streamlining state management.Documentation