Closed ponderingdemocritus closed 1 week ago
vercel[bot]
commented
1 week ago The latest updates on your projects. Learn more about Vercel for Git ↗︎
| Name | Status | Preview | Comments | Updated (UTC) |
|---|---|---|---|---|
| eternum | ✅ Ready (Inspect) | Visit Preview | 💬 Add feedback | Jun 24, 2024 6:06am |
github-actions[bot]
commented
1 week ago | ⏱️ Estimated effort to review [1-5] | 4 |
| 🧪 Relevant tests | No |
| 🔒 Security concerns | No |
| ⚡ Key issues to review |
Possible Bug: In useTrade.tsx, the change in the calculation logic for perLords might introduce a bug if not properly tested. The order of arguments in calculateRatio function has been swapped, which could potentially lead to incorrect calculations if not handled correctly. |
|
Code Clarity: In ResourceWeight.tsx, the renaming of table headers and data might cause confusion or misalignment with other parts of the application that depend on these names. It's important to ensure that all references to these names are updated accordingly. | |
|
Redundancy: In MarketOrderPanel.tsx, the ResourceIcon component is rendered twice consecutively with different props which might be an error or unnecessary duplication. | |
|
Data Handling: In TransferBetweenEntities.tsx, the introduction of SelectedEntity interface and changes in state management for entity selection could lead to unexpected behavior if the new data structure is not integrated properly throughout the application. |
github-actions[bot]
commented
1 week ago | Category | Suggestion | Score |
| Possible bug |
Add a check to prevent division by zero, ensuring the application's stability___ **Replace the direct division oflords / resource with a safer check to ensure resource is not zero, preventing division by zero errors.** [client/src/ui/components/trading/MarketOrderPanel.tsx [343]](https://github.com/BibliothecaDAO/eternum/pull/993/files#diff-00bad72a59745d3d904730035fcdeaddda30f53c9212d94f35f277ef202bd0bdR343-R343) ```diff -return (lords / resource).toFixed(2); +return resource !== 0 ? (lords / resource).toFixed(2) : '0.00'; ``` Suggestion importance[1-10]: 10Why: This suggestion prevents a critical issue of division by zero, which could cause runtime errors and application instability. It is a significant improvement for ensuring the application's reliability. | 10 |
Add a null check for the array before accessing its elements to prevent runtime errors___ **Consider adding a null check fortakerGets before accessing its first element to avoid potential runtime errors if takerGets is undefined or empty.**
[client/src/hooks/helpers/useTrade.tsx [91]](https://github.com/BibliothecaDAO/eternum/pull/993/files#diff-e42008cf5c4b4bd687484849a10da2429f055707a9335a9c28ac67da60a45f34R91-R91)
```diff
-takerGets[0]?.resourceId == ResourcesIds.Lords
+takerGets && takerGets[0]?.resourceId == ResourcesIds.Lords
```
Suggestion importance[1-10]: 9Why: This suggestion addresses a potential runtime error by adding a null check for `takerGets` before accessing its first element. This is crucial for preventing crashes if `takerGets` is undefined or empty. | 9 | |
Implement null checks before accessing object properties to prevent errors___ **Use null checks forselectedEntityIdFrom and selectedEntityIdTo before using their properties to avoid potential null reference errors.** [client/src/ui/components/trading/TransferBetweenEntities.tsx [84-85]](https://github.com/BibliothecaDAO/eternum/pull/993/files#diff-5ce2f1e45fead506c5de11a56a5f1927ff1efbe1645641f5cf0fcb88094d64baR84-R85) ```diff -owner_entity_id: selectedEntityIdFrom?.entityId!, -recipient_entity_id: selectedEntityIdTo?.entityId!, +owner_entity_id: selectedEntityIdFrom ? selectedEntityIdFrom.entityId : undefined, +recipient_entity_id: selectedEntityIdTo ? selectedEntityIdTo.entityId : undefined, ``` Suggestion importance[1-10]: 9Why: This suggestion adds necessary null checks before accessing properties of `selectedEntityIdFrom` and `selectedEntityIdTo`, which helps prevent potential null reference errors and improves code safety. | 9 | |
Use optional chaining to handle potential undefined values safely___ **Ensure consistent use of optional chaining fortravelTime to prevent potential undefined errors, similar to other usages in the code.** [client/src/ui/components/resources/ResourceWeight.tsx [52]](https://github.com/BibliothecaDAO/eternum/pull/993/files#diff-cb1944e8fb6258c30583665a6237d4196296b5fe8e4fd798889097796753e0cfR52-R52) ```diff -{`${travelTime!} hrs`} +{`${travelTime ? `${travelTime} hrs` : 'N/A'}`} ``` Suggestion importance[1-10]: 8Why: This suggestion ensures consistent use of optional chaining for `travelTime`, which helps prevent potential undefined errors and improves code robustness. | 8 | |
| Possible issue |
Remove redundant z-index from the CSS class list to avoid confusion and ensure correct styling___ **The z-index ofListbox.Options is set to both z-50 and z-100 which is likely a mistake as only one z-index value can be applied at a time. Consider removing the redundant z-index to avoid confusion and ensure the CSS behaves as expected.** [client/src/ui/elements/ListSelect.tsx [76]](https://github.com/BibliothecaDAO/eternum/pull/993/files#diff-f5c0bdecb0ad9b6ec67a48dba52979ef620c3d90a6e55d9b38f569cf41730f57R76-R76) ```diff -"z-50 mt-3 w-full rounded-md py-1 h-72 overflow-scroll z-100 border border-gold/10" +"z-50 mt-3 w-full rounded-md py-1 h-72 overflow-scroll border border-gold/10" ``` Suggestion importance[1-10]: 9Why: The suggestion correctly identifies a redundancy in the CSS class list that could lead to confusion or unexpected behavior. Removing the redundant z-index improves code clarity and maintainability. | 9 |
Verify and adjust the border color opacity to ensure it meets the design requirements___ **The border color opacity has been changed fromborder-gold to border-gold/10. If this change was intended to reduce the visibility of the border, it's fine. However, if the change was accidental, consider reverting it to maintain the original appearance.** [client/src/ui/elements/NumberInput.tsx [21]](https://github.com/BibliothecaDAO/eternum/pull/993/files#diff-dcfea276430231a9d63af9a1ab38e9d6164a68bd71147a665e7f7c1c3f591a55R21-R21) ```diff -className="flex items-center justify-center h-full px-1 border-r cursor-pointer border-gold/10 hover:bg-gold/30 " +className="flex items-center justify-center h-full px-1 border-r cursor-pointer border-gold hover:bg-gold/30 " ``` Suggestion importance[1-10]: 7Why: The suggestion is valid as it prompts verification of a potentially unintended change in border color opacity, which could affect the visual design. However, it is a minor issue. | 7 | |
| Maintainability |
Remove or properly handle the commented-out SVG to clean up the code___ **The SVG element is commented out but still partially present in the code. If the SVG isnot needed, consider removing the entire SVG block to clean up the code. If it is temporarily disabled, ensure that it is properly documented or handled.** [client/src/ui/elements/ListSelect.tsx [60-72]](https://github.com/BibliothecaDAO/eternum/pull/993/files#diff-f5c0bdecb0ad9b6ec67a48dba52979ef620c3d90a6e55d9b38f569cf41730f57R60-R72) ```diff -{/* | 8 |
| Enhancement |
Explicitly set a default background color for inactive states in conditional styling___ **The conditional styling for active items inListbox.Option uses an empty string for inactive states, which might lead to unintended CSS results. Consider explicitly setting a default background color for better clarity and maintainability.** [client/src/ui/elements/ListSelect.tsx [85]](https://github.com/BibliothecaDAO/eternum/pull/993/files#diff-f5c0bdecb0ad9b6ec67a48dba52979ef620c3d90a6e55d9b38f569cf41730f57R85-R85) ```diff -active ? "bg-gold/10 text-white/90" : "" +active ? "bg-gold/10 text-white/90" : "bg-transparent" ``` Suggestion importance[1-10]: 8Why: The suggestion improves code clarity and maintainability by ensuring that inactive states have a defined background color, preventing potential unintended CSS results. | 8 |
PR Type
enhancement, bug fix
Description
takerGets[0]?.resourceIdinuseTradeto prevent potential runtime errors.ResourceWeight.tsxfor better UX and styling consistency.MarketOrderPanel.tsx.TransferBetweenEntities.tsx.ListSelectcomponent and removed an unused SVG element.NumberInputcomponent.Changes walkthrough 📝
useTrade.tsx
Add optional chaining to prevent runtime errorsclient/src/hooks/helpers/useTrade.tsx
takerGets[0]?.resourceIdto preventpotential runtime errors.
ResourceWeight.tsx
Reorder table rows and update stylingclient/src/ui/components/resources/ResourceWeight.tsx
MarketOrderPanel.tsx
Adjust resource icon placement and fix bid calculationclient/src/ui/components/trading/MarketOrderPanel.tsx
TransferBetweenEntities.tsx
Enhance transfer details and improve layoutclient/src/ui/components/trading/TransferBetweenEntities.tsx
confirmation.
ListSelect.tsx
Update styling and remove unused SVGclient/src/ui/elements/ListSelect.tsx
NumberInput.tsx
Update border styling for number inputclient/src/ui/elements/NumberInput.tsx - Updated border styling for number input component.