fix: trade history crash #2445
fix: trade history crash #2445
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
WalkthroughThe pull request introduces enhancements to the 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: 0
🧹 Outside diff range and nitpick comments (4)
client/src/ui/components/trading/TradeHistoryEvent.tsx (3)
47-52: Consider adding type safety for amount valuesWhile the null checks prevent crashes from undefined resources, consider adding type safety for the amount values to prevent potential NaN issues in the currency formatting.
- <div>{`${currencyIntlFormat(divideByPrecision(resourceTaken.amount), 2)} for ${currencyIntlFormat( - divideByPrecision(resourceGiven.amount), - 2, - )}`}</div> + <div>{`${currencyIntlFormat(divideByPrecision(Number(resourceTaken.amount) || 0), 2)} for ${currencyIntlFormat( + divideByPrecision(Number(resourceGiven.amount) || 0), + 2, + )}`}</div>
62-62: Consider improving readability of resource ID selectionThe nested ternary within array access could be made more readable by extracting the logic into a separate variable.
- resourceTaken.resourceId === ResourcesIds.Lords ? resourceGiven.resourceId : resourceTaken.resourceId, + const displayResourceId = resourceTaken.resourceId === ResourcesIds.Lords + ? resourceGiven.resourceId + : resourceTaken.resourceId; + displayResourceId,
74-82: Consider improving error handling feedbackWhile catching errors is good, silently returning 0 might hide issues from users. Consider:
- Adding more specific error types
- Providing user feedback for calculation errors
try { const lordsResource = resourceA.resourceId === ResourcesIds.Lords ? resourceA : resourceB; const otherResource = resourceA.resourceId === ResourcesIds.Lords ? resourceB : resourceA; return Number(lordsResource.amount) / Number(otherResource.amount); } catch (e) { console.error(e); - return 0; + const error = e instanceof Error ? e : new Error('Unknown error'); + console.error('Price calculation failed:', error); + // Consider showing a user-friendly error message in the UI + return null; }client/src/ui/components/trading/MarketTradingHistory.tsx (1)
Line range hint
127-131: Consider improving filter condition readabilityWhile the null checks are good, the filter condition could be more readable by breaking it down into smaller parts.
- .filter((trade) => - trade.event.resourceGiven && trade.event.resourceTaken && selectedResourceId - ? trade.event.resourceGiven.resourceId === selectedResourceId || - trade.event.resourceTaken.resourceId === selectedResourceId - : true, - ) + .filter((trade) => { + // First check if we have valid resources + if (!trade.event.resourceGiven || !trade.event.resourceTaken) { + return false; + } + + // If no resource is selected, show all trades + if (!selectedResourceId) { + return true; + } + + // Show trades matching the selected resource + return trade.event.resourceGiven.resourceId === selectedResourceId || + trade.event.resourceTaken.resourceId === selectedResourceId; + })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
client/src/ui/components/trading/MarketTradingHistory.tsx(1 hunks)client/src/ui/components/trading/TradeHistoryEvent.tsx(4 hunks)
🔇 Additional comments (2)
client/src/ui/components/trading/TradeHistoryEvent.tsx (1)
29-33: LGTM: Good defensive programming practice!
The early null check for undefined resources effectively prevents the component from crashing when either resourceTaken or resourceGiven is undefined. This is a clean solution that follows best practices.
client/src/ui/components/trading/MarketTradingHistory.tsx (1)
Line range hint 127-131: Verify comprehensive handling of undefined resources
Let's verify that all potential undefined resource scenarios are covered.
✅ Verification successful
Based on the search results, I can now generate the final response as we have comprehensive information about the usage of resourceGiven and resourceTaken in the codebase.
All undefined resource scenarios are properly handled
The codebase shows proper handling of undefined resources in both locations where these properties are used:
- In
MarketTradingHistory.tsx: The code uses a safe conditional checktrade.event.resourceGiven && trade.event.resourceTakenbefore accessing their properties - In
TradeHistoryEvent.tsx: The code implements explicit null checks withif (!resourceTaken || !resourceGiven)after destructuring
No other unsafe usages of these properties were found in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct access to resourceGiven or resourceTaken without null checks
rg -n "\.resourceGiven|\.resourceTaken" --type ts
# Search for potential null checks on these properties
rg -n "(!|\?\.).*resource(Given|Taken)" --type ts
# Look for similar patterns that might need the same protection
ast-grep --pattern 'trade.event.$_'
Length of output: 2299
Summary by CodeRabbit
New Features
Bug Fixes
resourceGivenandresourceTakenare defined before processing trade events.Refactor
Chores