memo the market #2525
memo the market #2525
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
WalkthroughThe pull request focuses on performance optimization across multiple UI components in the client-side React application. The changes primarily involve using React's Changes
Possibly related PRs
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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: 13
🧹 Nitpick comments (9)
client/src/ui/components/trading/RealmProduction.tsx (1)
18-18: Consider moving resourcesInputs outside componentSince
configManager.resourceInputsappears to be static and the useMemo has no dependencies, this value could be defined outside the component to avoid recreating the memoized value on each mount.+const resourcesInputs = configManager.resourceInputs; export const RealmProduction = () => { - const resourcesInputs = useMemo(() => configManager.resourceInputs, []);client/src/ui/components/trading/SelectEntityFromList.tsx (1)
24-24: Consider adding min-height to prevent layout shifts.The
overflow-y-scrollwithmax-heightcould cause layout shifts when the content is smaller than the container.-<div className="overflow-y-scroll max-h-72 border border-gold/10 gap-2 flex-col"> +<div className="overflow-y-scroll max-h-72 min-h-40 border border-gold/10 gap-2 flex-col">client/src/ui/components/trading/TransferBetweenEntities.tsx (3)
65-65: Consider replacing 'any' type with proper interfaceThe
entitiesListWithAccountNamesprop usesany[]which reduces type safety. Consider creating a proper interface for the entity type.interface Entity { entity_id: string; name: string; accountName?: string; // add other relevant fields }
277-291: Consider optimizing props for better memoizationThe current prop passing pattern might cause unnecessary re-renders. Consider grouping related props into objects.
- <SelectEntitiesStep - selectedEntityIdFrom={selectedEntityIdFrom} - selectedEntityIdTo={selectedEntityIdTo} - setSelectedEntityIdFrom={setSelectedEntityIdFrom} - setSelectedEntityIdTo={setSelectedEntityIdTo} - travelTime={travelTime} - entitiesListWithAccountNames={entitiesListWithAccountNames} - fromSearchTerm={fromSearchTerm} - setFromSearchTerm={setFromSearchTerm} - toSearchTerm={toSearchTerm} - setToSearchTerm={setToSearchTerm} - filtered={filtered} - filterBy={filterBy} - setSelectedStepId={setSelectedStepId} + <SelectEntitiesStep + selection={{ + from: { entity: selectedEntityIdFrom, setEntity: setSelectedEntityIdFrom, searchTerm: fromSearchTerm, setSearchTerm: setFromSearchTerm }, + to: { entity: selectedEntityIdTo, setEntity: setSelectedEntityIdTo, searchTerm: toSearchTerm, setSearchTerm: setToSearchTerm } + }} + filtering={{ filtered, filterBy }} + travelTime={travelTime} + entitiesListWithAccountNames={entitiesListWithAccountNames} + onNext={() => setSelectedStepId(STEP_ID.SELECT_RESOURCES)} />
341-351: Improve semantic markup in success messageThe success message should use more semantic HTML elements for better accessibility.
- <div className=" justify-center items-center text-center"> - <h4>Transfer successful!</h4> - <p>Check transfers in the right sidebar transfer menu.</p> + <section className="justify-center items-center text-center" role="alert" aria-live="polite"> + <h2 className="text-xl">Transfer successful!</h2> + <p>Check transfers in the right sidebar transfer menu.</p>client/src/ui/components/trading/MarketOrderPanel.tsx (1)
58-60: Remove Unnecessary Dependency 'production' from useMemoThe
useMemoforbalanceincludesproductionin its dependency array, butproductionisn't used inside the hook. This could cause unnecessary recalculations.Suggested fix:
}, [resourceManager, production, currentDefaultTick]); +}, [resourceManager, currentDefaultTick]);client/src/ui/components/bank/ResourceBar.tsx (1)
65-74: Optimize setTimeout usageThe
setTimeoutwith 0ms delay inhandleOpenChangeis commonly used to defer focus, but it can be replaced with a more efficient approach.Consider using
requestAnimationFramefor better performance:const handleOpenChange = (newOpen: boolean) => { setOpen(newOpen); if (newOpen && inputRef.current) { setResourceId(ResourcesIds.Wood); setSearchInput(""); - setTimeout(() => { + requestAnimationFrame(() => { inputRef.current?.focus(); - }, 0); + }); } };client/src/ui/modules/navigation/QuestMenu.tsx (2)
Line range hint
43-63: Optimize quest claiming functionsThe quest claiming functions (
handleClaimQuestandhandleClaimAllQuests) share similar error handling patterns and state management. They could be optimized for better maintainability and error recovery.Consider extracting a common claiming utility:
const useQuestClaiming = () => { const [isLoading, setIsLoading] = useState(false); const claimQuests = async (questIds: bigint[], onSuccess?: () => void) => { setIsLoading(true); try { await claim_quest({ signer: account, quest_ids: questIds, receiver_id: structureEntityId, }); onSuccess?.(); } catch (error) { console.error('Failed to claim quests:', error); // Consider adding error handling UI feedback } finally { setIsLoading(false); } }; return { isLoading, claimQuests }; };
Line range hint
89-126: Consider extracting tooltip logicThe tooltip positioning logic in
handleClaimMouseEntercould be extracted into a custom hook for reusability and better separation of concerns.const useTooltipPosition = (tooltipWidth: number) => { const calculatePosition = useCallback((rect: DOMRect) => { let x = rect.left + rect.width / 2 - tooltipWidth / 2; x = Math.min(Math.max(x, 20), window.innerWidth - tooltipWidth - 20); return { x, y: rect.bottom + 10 }; }, [tooltipWidth]); return calculatePosition; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
client/src/ui/components/bank/ResourceBar.tsx(1 hunks)client/src/ui/components/trading/MarketOrderPanel.tsx(2 hunks)client/src/ui/components/trading/MarketResourceSideBar.tsx(2 hunks)client/src/ui/components/trading/MarketTradingHistory.tsx(4 hunks)client/src/ui/components/trading/RealmProduction.tsx(2 hunks)client/src/ui/components/trading/ResourceArrivals.tsx(1 hunks)client/src/ui/components/trading/SelectEntityFromList.tsx(1 hunks)client/src/ui/components/trading/TransferBetweenEntities.tsx(4 hunks)client/src/ui/modules/navigation/QuestMenu.tsx(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
client/src/ui/components/bank/ResourceBar.tsx
[error] 110-110: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
[error] 113-113: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
client/src/ui/components/trading/TransferBetweenEntities.tsx
[error] 83-83: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
client/src/ui/components/trading/MarketOrderPanel.tsx
[error] 523-523: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
🔇 Additional comments (14)
client/src/ui/components/trading/SelectEntityFromList.tsx (2)
19-21: Good use of memoization for performance optimization.
The memo wrapper is well-applied here, as it will prevent unnecessary re-renders when parent components update but the props remain the same. This is particularly valuable for list components.
7-17: LGTM! Well-structured type definitions.
The new interfaces improve type safety and code clarity. The Entity and SelectEntityFromListProps interfaces are well-defined and properly type the component's data structures.
Let's verify the ID type usage across the codebase:
✅ Verification successful
ID type is properly imported and used consistently
The ID type is correctly imported from @bibliothecadao/eternum in the component file, and it's consistently used across the codebase. The type is defined in sdk/packages/eternum/src/types/common.ts as type ID = number and is widely used in similar interfaces and type definitions throughout the project.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of ID type from eternum
# Check for other occurrences of ID type usage
rg -t typescript "ID.*from.*eternum" -A 2
Length of output: 78
Script:
#!/bin/bash
# Let's try different approaches to verify ID type usage
# Search for ID type imports
rg "import.*\{.*ID.*\}" --type ts
# Search for ID type usage in type definitions
rg ":\s*ID\b" --type ts -A 1
# Search for ID type declarations
rg "type\s+ID\s*=" --type ts
Length of output: 61394
client/src/ui/components/trading/TransferBetweenEntities.tsx (1)
Line range hint 44-351: Overall implementation looks good!
The refactoring improves component organization and performance through proper use of memo. The code is well-structured and the separation of concerns is clear. The suggested improvements are mostly for better type safety and accessibility, but the core functionality is solid.
🧰 Tools
🪛 Biome (1.9.4)
[error] 83-83: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
client/src/ui/components/trading/MarketOrderPanel.tsx (7)
33-91: Confirm Appropriate Use of React.memo for 'MarketResource' Component
Wrapping the MarketResource component with React.memo can improve performance by preventing unnecessary re-renders. Ensure that all props passed to MarketResource are primitives or stable references. If any props are objects or functions that change on every render, memoization might not be effective.
93-137: Confirm Appropriate Use of React.memo for 'MarketOrderPanel' Component
Applying React.memo to MarketOrderPanel can optimize rendering. Please verify that the props provided to this component are stable and do not cause unintended re-renders due to reference changes in objects or functions.
139-216: Confirm Appropriate Use of React.memo for 'MarketOrders' Component
Using React.memo with MarketOrders is beneficial for performance. Ensure that the props are immutable or have stable references to maximize the effectiveness of memoization.
Line range hint 218-238: Confirm Appropriate Use of React.memo for 'OrderRowHeader' Component
The OrderRowHeader component is now wrapped with React.memo. Since this component likely receives static props, this should prevent unnecessary re-renders. Confirm that the props are indeed stable and that memoization is appropriate here.
🧰 Tools
🪛 Biome (1.9.4)
[error] 523-523: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
239-495: Confirm Appropriate Use of React.memo for 'OrderRow' Component
Wrapping OrderRow with React.memo can enhance performance. However, if any of the props are objects or functions that change between renders, memoization might not yield benefits. Please verify the stability of the props.
497-699: Confirm Appropriate Use of React.memo for 'OrderCreation' Component
The OrderCreation component is wrapped with React.memo. Ensure that all props and functions used within are stable to fully utilize memoization benefits.
🧰 Tools
🪛 Biome (1.9.4)
[error] 523-523: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
569-571:
Add Missing Dependency to useMemo Hook for 'resourceProduction'
The useMemo hook calculating resourceProduction depends on resourceProductionManager but has an empty dependency array. Add resourceProductionManager to ensure proper memoization.
Suggested fix:
}, []);
+}, [resourceProductionManager]);Likely invalid or redundant comment.
client/src/ui/components/trading/ResourceArrivals.tsx (1)
8-22: Confirm Appropriate Use of React.memo for 'AllResourceArrivals' Component
Wrapping AllResourceArrivals with React.memo can improve performance by avoiding unnecessary re-renders. Verify that the props (arrivals, className) are stable and that memoization is beneficial in this context.
client/src/ui/components/trading/MarketResourceSideBar.tsx (1)
32-61:
Check Dependencies of useMemo for 'resourceList'
The resourceList is memoized using useMemo, but the dependency array might be missing dependencies used within the memoized function, such as bankEntityId, setup, and onClick. Ensure all external variables used are included in the dependency array to prevent stale values.
Suggested fix:
}, [filteredResources, bankEntityId, setup, resourceBidOffers, resourceAskOffers, selectedResource, entityId, onClick]);
+}, [filteredResources, bankEntityId, setup, resourceBidOffers, resourceAskOffers, selectedResource, entityId, onClick, ResourcesIds]);Likely invalid or redundant comment.
client/src/ui/components/trading/MarketTradingHistory.tsx (2)
9-9: Confirm Appropriate Use of React.memo for 'MarketTradingHistory' Component
By wrapping MarketTradingHistory with React.memo, you're aiming to prevent unnecessary re-renders. Ensure that all props and internal state changes are properly managed so that memoization is effective.
106-109: Properly Define Dependencies in useMemo for 'filteredTradeEvents'
The useMemo hook for filteredTradeEvents depends on showOnlyYourSwaps and tradeEvents. Ensure that these are correctly specified in the dependency array to prevent stale or incorrect memoized values.
| const filteredRealms = useMemo(() => { | ||
| if (!realms) return []; | ||
|
|
||
| return realms.filter((realm) => { | ||
| if (!realm) return false; | ||
|
|
||
| const resourcesProduced = unpackResources(realm.resourceTypesPacked); | ||
| if (filterProduced && !resourcesProduced.includes(filterProduced)) return false; | ||
|
|
||
| if (filterConsumed) { | ||
| const resourcesConsumed = new Set( | ||
| resourcesProduced.flatMap((resourceId) => | ||
| resourcesInputs[resourceId] | ||
| .filter((input) => input.resource !== ResourcesIds["Wheat"] && input.resource !== ResourcesIds["Fish"]) | ||
| .map((input) => input.resource), | ||
| ), | ||
| ); | ||
|
|
||
| if (!resourcesConsumed.has(filterConsumed)) return false; | ||
| } | ||
|
|
||
| return true; | ||
| }); | ||
| }, [realms, filterProduced, filterConsumed, resourcesInputs]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add proper typing for realm objects
The filtering logic looks solid, but using any type reduces type safety. Consider defining proper interfaces for realm objects to catch potential issues at compile time.
interface Realm {
realmId: string;
entityId: string;
owner: string;
ownerName: string;
name: string;
resourceTypesPacked: string; // adjust type based on actual data structure
}| const handleRealmClick = (realm: any) => { | ||
| toggleModal(null); | ||
| setSelectedPlayer(realm.owner); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add proper typing for handleRealmClick parameter
Replace the any type with the proper Realm interface to maintain type safety.
- const handleRealmClick = (realm: any) => {
+ const handleRealmClick = (realm: Realm) => {Committable suggestion skipped: line range outside the PR's diff.
| {filteredRealms.map((realm, index) => ( | ||
| <div | ||
| key={index} | ||
| className="mb-5 border border-gold/40 rounded-xl p-3 hover:opacity-70" | ||
| onClick={() => handleRealmClick(realm)} | ||
| > | ||
| <p className="text-md">{realm.ownerName}</p> | ||
| <p className="text-md mb-1 font-bold">{realm.name}</p> | ||
| <hr /> | ||
| {realm.realmId && <RealmResourcesIO realmEntityId={realm.entityId} />} | ||
| </div> | ||
| ))} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use stable keys for realm items
Using array indices as keys can lead to issues when items are reordered or filtered. Consider using a more stable identifier like realmId or entityId.
- key={index}
+ key={realm.entityId}Additionally, consider wrapping the entire component with React.memo for better performance, especially since it handles filtering and rendering of potentially large lists.
-export const RealmProduction = () => {
+export const RealmProduction = React.memo(() => {
// ... component implementation ...
-};
+});📝 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.
| {filteredRealms.map((realm, index) => ( | |
| <div | |
| key={index} | |
| className="mb-5 border border-gold/40 rounded-xl p-3 hover:opacity-70" | |
| onClick={() => handleRealmClick(realm)} | |
| > | |
| <p className="text-md">{realm.ownerName}</p> | |
| <p className="text-md mb-1 font-bold">{realm.name}</p> | |
| <hr /> | |
| {realm.realmId && <RealmResourcesIO realmEntityId={realm.entityId} />} | |
| </div> | |
| ))} | |
| {filteredRealms.map((realm) => ( | |
| <div | |
| key={realm.entityId} | |
| className="mb-5 border border-gold/40 rounded-xl p-3 hover:opacity-70" | |
| onClick={() => handleRealmClick(realm)} | |
| > | |
| <p className="text-md">{realm.ownerName}</p> | |
| <p className="text-md mb-1 font-bold">{realm.name}</p> | |
| <hr /> | |
| {realm.realmId && <RealmResourcesIO realmEntityId={realm.entityId} />} | |
| </div> | |
| ))} |
| <div | ||
| key={entity.entity_id} | ||
| className={clsx( | ||
| "flex w-full justify-between hover:bg-white/10 items-center p-1 text-xs pl-2", | ||
| isSelected && "border-gold/10 border", | ||
| )} | ||
| onClick={() => onSelect(entity.name, entity.entity_id)} | ||
| > |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider performance and accessibility improvements.
A few suggestions to enhance the implementation:
- Memoize the click handler to prevent unnecessary recreations
- Use semantic HTML (
buttonorlielements) - Add proper ARIA attributes for accessibility
-<div
+<li
key={entity.entity_id}
className={clsx(
"flex w-full justify-between hover:bg-white/10 items-center p-1 text-xs pl-2",
isSelected && "border-gold/10 border",
)}
- onClick={() => onSelect(entity.name, entity.entity_id)}
+ role="listitem"
+ aria-selected={isSelected}
>And add this before the mapping:
const handleSelect = useCallback(
(name: string, entityId: ID) => () => onSelect(name, entityId),
[onSelect]
);Then use it in the JSX:
onClick={handleSelect(entity.name, entity.entity_id)}| <div className="w-full flex justify-center items-center"> | ||
| Travel Time: {Math.floor((travelTime || 0) / 60)} hrs {(travelTime || 0) % 60} mins | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve accessibility for interactive elements
Add proper ARIA labels and roles:
- Travel time information should be marked up semantically
- The "Tribe Only" checkbox needs proper accessibility attributes
- <div className="w-full flex justify-center items-center">
+ <div className="w-full flex justify-center items-center" role="status" aria-label="Travel time information">
- <div className="flex space-x-2 items-center cursor-pointer" onClick={() => filterBy(!filtered)}>
+ <label className="flex space-x-2 items-center cursor-pointer" onClick={() => filterBy(!filtered)}>Also applies to: 123-127
|
|
||
| const handleBidChange = (newBid: number) => { | ||
| const numericBid = Number(newBid); | ||
| if (!isNaN(numericBid) && numericBid > 0) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use Number.isNaN Instead of isNaN for Safer Checking
The global isNaN function can yield unexpected results due to type coercion. Replace isNaN with Number.isNaN to ensure accurate checking without type coercion.
Suggested fix:
-if (!isNaN(numericBid) && numericBid > 0) {
+if (!Number.isNaN(numericBid) && numericBid > 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.
| if (!isNaN(numericBid) && numericBid > 0) { | |
| if (!Number.isNaN(numericBid) && numericBid > 0) { |
🧰 Tools
🪛 Biome (1.9.4)
[error] 523-523: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
| useEffect(() => { | ||
| setInputValue( | ||
| isBuy | ||
| ? divideByPrecision(offer.makerGets[0].amount) * resourceBalanceRatio | ||
| : divideByPrecision(offer.takerGets[0].amount) * lordsBalanceRatio, | ||
| ); | ||
| }, [resourceBalanceRatio, lordsBalanceRatio]); |
There was a problem hiding this comment.
Include Missing Dependencies in useEffect Hook
The useEffect hook updates inputValue based on isBuy, offer, and balance ratios, but only includes resourceBalanceRatio and lordsBalanceRatio in the dependency array. To ensure inputValue updates correctly, include isBuy and offer in the dependencies.
Suggested fix:
}, [resourceBalanceRatio, lordsBalanceRatio]);
+}, [resourceBalanceRatio, lordsBalanceRatio, isBuy, offer]);📝 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.
| useEffect(() => { | |
| setInputValue( | |
| isBuy | |
| ? divideByPrecision(offer.makerGets[0].amount) * resourceBalanceRatio | |
| : divideByPrecision(offer.takerGets[0].amount) * lordsBalanceRatio, | |
| ); | |
| }, [resourceBalanceRatio, lordsBalanceRatio]); | |
| useEffect(() => { | |
| setInputValue( | |
| isBuy | |
| ? divideByPrecision(offer.makerGets[0].amount) * resourceBalanceRatio | |
| : divideByPrecision(offer.takerGets[0].amount) * lordsBalanceRatio, | |
| ); | |
| }, [resourceBalanceRatio, lordsBalanceRatio, isBuy, offer]); |
| export const ResourceBar = memo( | ||
| ({ | ||
| entityId, | ||
| lordsFee, | ||
| resources, | ||
| resourceId, | ||
| setResourceId, | ||
| amount, | ||
| setAmount, | ||
| disableInput = false, | ||
| onFocus, | ||
| onBlur, | ||
| max = Infinity, | ||
| }: { | ||
| entityId: ID; | ||
| lordsFee: number; | ||
| resources: Resources[]; | ||
| resourceId: ResourcesIds; | ||
| setResourceId: (resourceId: ResourcesIds) => void; | ||
| amount: number; | ||
| setAmount: (amount: number) => void; | ||
| disableInput?: boolean; | ||
| onFocus?: () => void; // New prop | ||
| onBlur?: () => void; // New prop | ||
| max?: number; | ||
| }) => { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Verify memoization effectiveness
While wrapping the component in memo is a good start for optimization, the component receives multiple callback props (setResourceId, setAmount, onFocus, onBlur) which could negate the benefits of memoization if their references change frequently in the parent component.
Consider wrapping the callback props with useCallback in the parent component to maintain stable references:
// In parent component:
const handleSetResourceId = useCallback((resourceId: ResourcesIds) => {
setResourceId(resourceId);
}, []);
const handleSetAmount = useCallback((amount: number) => {
setAmount(amount);
}, []);| className="flex text-xs text-gold/70 mt-1 justify-center items-center relative text-center self-center mx-auto w-full cursor-pointer" | ||
| onClick={() => handleAmountChange(finalResourceBalance)} | ||
| > | ||
| Max: {isNaN(selectedResourceBalance) ? "0" : selectedResourceBalance.toLocaleString()} |
There was a problem hiding this comment.
Replace unsafe isNaN with Number.isNaN
The global isNaN() function performs type coercion which can lead to unexpected results. Use Number.isNaN() instead for more predictable behavior.
Apply this fix:
- Max: {isNaN(selectedResourceBalance) ? "0" : selectedResourceBalance.toLocaleString()}
+ Max: {Number.isNaN(selectedResourceBalance) ? "0" : selectedResourceBalance.toLocaleString()}
{hasLordsFees && (
<div className="text-danger ml-2">
- <div>{`[+${isNaN(lordsFee) ? "0" : formatNumber(lordsFee, 2)}]`}</div>
+ <div>{`[+${Number.isNaN(lordsFee) ? "0" : formatNumber(lordsFee, 2)}]`}</div>Also applies to: 113-113
🧰 Tools
🪛 Biome (1.9.4)
[error] 110-110: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
|
|
||
| export const QuestsMenu = () => { | ||
| export const QuestsMenu = memo(() => { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize memoization implementation
While the component is wrapped in memo, it relies heavily on Zustand store selectors which could trigger re-renders. The current implementation might not provide optimal performance benefits.
Consider these optimizations:
- Use shallow equality comparison for store selectors
- Memoize complex computations
- Extract child components for better render optimization
Example implementation:
// 1. Use shallow comparison for store selectors
const worldLoading = useWorldStore((state) => state.isWorldLoading, shallow);
// 2. Memoize quest filtering
const currentQuest = useMemo(() =>
quests?.find(quest =>
quest.status === QuestStatus.InProgress ||
quest.status === QuestStatus.Completed
),
[quests]
);
// 3. Extract QuestButtons as a separate memoized component
const QuestButtons = memo(({ onClaim, onStart, disabled }) => {
// Button rendering logic
});
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
client/src/three/scenes/HexagonScene.ts (1)
417-421: Consider performance impact of try-catch in update loop.The try-catch block is within the update loop that runs frequently. While error handling is important, wrapping each animation update in try-catch could impact performance.
Consider these alternatives:
- Move error handling to the
BiomeTypeclass and handle errors there- Implement a more efficient error boundary at a higher level
- Add error tracking to identify frequently failing biomes and handle them separately
Would you like me to provide example implementations for any of these approaches?
| try { | ||
| biome.updateAnimations(deltaTime); | ||
| } catch (error) { | ||
| console.error(`Error updating biome animations:`, error); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling with more context and recovery strategy.
While adding error handling is good, the current implementation could be improved to:
- Include more context about which biome failed
- Implement a recovery strategy for failed animations
Consider this enhanced implementation:
this.biomeModels.forEach((biome, biomeType) => {
try {
biome.updateAnimations(deltaTime);
} catch (error) {
- console.error(`Error updating biome animations:`, error);
+ console.error(`Error updating animations for biome type ${biomeType}:`, error);
+ // Attempt to reset the animation state
+ try {
+ biome.resetAnimations?.();
+ } catch (resetError) {
+ console.error(`Failed to reset animations for biome type ${biomeType}:`, resetError);
+ }
}
});Committable suggestion skipped: line range outside the PR's diff.
💡 Codebase verification
Based on the collected evidence, I can now provide the final response:
Add error handling for animation mixer updates
The current error handling for biome.updateAnimations() is good practice, but there's an unprotected animation update in Hexception.tsx where mixer.update(deltaTime) is called without error handling. To maintain consistency and prevent potential runtime errors, similar error handling should be added there as well.
client/src/three/scenes/Hexception.tsx: Add try-catch block aroundmixer.update(deltaTime)in the update method
🔗 Analysis chain
Verify error handling coverage across animation-related code.
Let's verify if similar error handling is implemented in other animation-related methods.
Let me analyze the findings and generate additional verification scripts to get a clearer picture.
The initial results show multiple animation-related code paths, but we need to specifically check the error handling in animation update methods and mixer updates. These additional scripts will help us verify the coverage of error handling in critical animation-related operations.
Let me try one more search pattern to ensure we have a complete view of animation-related error handling.
The previous results show only one mixer update without error handling, but we need more context to understand the full scope of animation-related updates in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other animation-related methods that might need error handling
echo "Searching for animation-related methods..."
rg -t ts "animation|animate" --no-filename
# Search for existing try-catch blocks in animation code
echo -e "\nChecking existing error handling in animation code..."
ast-grep --pattern 'try {
$$$
animation
$$$
} catch'
Length of output: 16752
Script:
#!/bin/bash
# Search for methods that update animations to check their error handling
ast-grep --pattern 'updateAnimations($$$) {
$$$
}'
# Search for animation mixers and their error handling
ast-grep --pattern 'mixer.update($$$)'
# Look for other try-catch blocks in animation-related code
rg -A 2 -B 2 "try.*catch.*animation"
Length of output: 208
Script:
#!/bin/bash
# Search for all animation update methods with context
rg -A 5 -B 5 "update.*animation|animation.*update"
# Look for mixer updates with more context
rg -A 5 -B 5 "mixer\.update"
# Search for animation-related class methods
ast-grep --pattern 'class $_ {
$$$
update($$$) {
$$$
animation
$$$
}
$$$
}'
Length of output: 686
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
client/src/ui/modules/military/battle-view/Battle.tsx (3)
21-47: Consider consolidating related propsThe component accepts many related props that could be grouped logically to improve maintainability and reduce prop drilling.
Consider consolidating related props into objects:
- attackerArmies: ArmyInfo[]; - attackerHealth: Health; - attackerTroops: ComponentValue<ClientComponents["Army"]["schema"]>["troops"]; - defenderArmies: (ArmyInfo | undefined)[]; - defenderHealth: Health | undefined; - defenderTroops: ComponentValue<ClientComponents["Army"]["schema"]>["troops"] | undefined; + attacker: { + armies: ArmyInfo[]; + health: Health; + troops: ComponentValue<ClientComponents["Army"]["schema"]>["troops"]; + }; + defender: { + armies: (ArmyInfo | undefined)[]; + health: Health | undefined; + troops: ComponentValue<ClientComponents["Army"]["schema"]>["troops"] | undefined; + };
54-63: Consider performance impact of motion animationsThe motion.div animation might affect performance during battle state changes. Consider using CSS transitions for simpler animations or adding
layoutIdfor better motion performance.<motion.div className="absolute bottom-0 " variants={{ hidden: { y: "100%" }, visible: { y: "0%", opacity: 1, transition: { duration: 0.5 } }, }} initial="hidden" animate="visible" exit="hidden" + layoutId="battle-container" >
103-118: Consider extracting conditional rendering logicThe conditional rendering between
LockedResourcesandBattleActionscould be clearer with an extracted component or helper function.+ const BattleMiddleSection = ({ showBattleDetails, battleAdjusted, ...props }) => { + if (showBattleDetails && battleAdjusted) { + return ( + <LockedResources + attackersResourcesEscrowEntityId={battleAdjusted?.attackers_resources_escrow_id} + defendersResourcesEscrowEntityId={battleAdjusted?.defenders_resources_escrow_id} + /> + ); + } + return <BattleActions {...props} battleAdjusted={battleAdjusted} />; + }; // In JSX - {showBattleDetails && battleAdjusted ? ( - <LockedResources ... /> - ) : ( - <BattleActions ... /> - )} + <BattleMiddleSection + showBattleDetails={showBattleDetails} + battleAdjusted={battleAdjusted} + {...battleActionProps} + />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/src/ui/modules/military/battle-view/Battle.tsx(1 hunks)
🔇 Additional comments (1)
client/src/ui/modules/military/battle-view/Battle.tsx (1)
12-12: Verify memo effectiveness for this component
While adding memo is a good optimization strategy, its effectiveness depends on props stability. Given the complex props structure (armies, health, troops), verify that these objects maintain referential stability in parent components to prevent memo from being bypassed.
Consider these improvements:
- Memoize complex prop calculations in the parent component using
useMemo - Consider breaking down the component if too many props are changing frequently
Also applies to: 20-21
✅ Verification successful
Memo usage is appropriate and well-implemented
The Battle component's memo optimization is justified and properly supported by the codebase:
-
Props stability is maintained through extensive useMemo usage in the parent BattleView component:
- All complex calculations and derived state (battlePosition, armies, health, troops, etc.) are memoized
- Object props are stabilized through useMemo hooks
- Array props (attackerArmies, defenderArmies) have memoized references
-
The component handles complex battle state rendering with multiple child components and conditional logic, making memo's re-render optimization valuable.
-
Parent component (BattleView) follows React performance best practices by memoizing all computed values and complex objects passed as props.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential prop stability issues
# Look for direct object/array creations in parent components that might bypass memo
# Search for parent components that render Battle
rg -l "Battle\s*?[^a-zA-Z]" --type tsx
# Look for inline object/array creations in parent components
ast-grep --pattern 'const $_ = {$$$}'
ast-grep --pattern 'const $_ = [$$$]'
Length of output: 67868
Script:
#!/bin/bash
# Let's check for potential prop stability issues in parent components that render Battle
# First, find all tsx files that import and render Battle
rg -l "import.*Battle.*from.*battle-view/Battle" --type ts --type tsx
# Then look for any inline object/array creations passed as props to Battle
rg "Battle.*?{.*?}" --type ts --type tsx -A 2 -B 2
# Also check for any useMemo usage with Battle props
rg "useMemo.*?Battle" --type ts --type tsx -A 2 -B 2
Length of output: 342
Script:
#!/bin/bash
# Let's try different search patterns to find Battle component usage and prop patterns
# Search for Battle component imports and usage
rg "import.*Battle"
# Look for Battle component rendering with props
rg "<Battle" -A 3 -B 3
# Check for any useMemo hooks in the same files
rg "useMemo" client/src/ui/modules/military/battle-view/
Length of output: 25827
| userArmiesOnThatSide={userArmiesInBattle.filter( | ||
| (army) => army.battle_side === BattleSide[BattleSide.Attack], | ||
| )} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize array filtering operations
The array filtering operations within the JSX could impact performance on frequent re-renders. Consider memoizing these filtered arrays.
+ const attackSideArmies = useMemo(
+ () => userArmiesInBattle.filter(
+ (army) => army.battle_side === BattleSide[BattleSide.Attack]
+ ),
+ [userArmiesInBattle]
+ );
+ const defenseSideArmies = useMemo(
+ () => userArmiesInBattle.filter(
+ (army) => army.battle_side === BattleSide[BattleSide.Defence]
+ ),
+ [userArmiesInBattle]
+ );
// In JSX
- userArmiesOnThatSide={userArmiesInBattle.filter(
- (army) => army.battle_side === BattleSide[BattleSide.Attack],
- )}
+ userArmiesOnThatSide={attackSideArmies}
- userArmiesOnThatSide={userArmiesInBattle.filter(
- (army) => army.battle_side === BattleSide[BattleSide.Defence],
- )}
+ userArmiesOnThatSide={defenseSideArmies}Also applies to: 128-130
Summary by CodeRabbit
New Features
SelectEntitiesStepandFinalTransfer.Bug Fixes
ResourceBarcomponent.Documentation
SelectEntityFromListcomponent.Refactor