Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sync with prod to fix small account detaisl bug #847

Merged
merged 2 commits into from
Sep 9, 2024
Merged

Conversation

marc-aurele-besner
Copy link
Collaborator

@marc-aurele-besner marc-aurele-besner commented Sep 9, 2024

User description

Sync with prod to fix small account detaisl bug


PR Type

Bug fix


Description

  • Fixed a bug in AccountBalancePieChart by correcting the conversion of otherNumber to a big number.
  • Added a check in bigNumberToNumber function to return 0 if the input bigNumber is falsy, preventing potential errors.

Changes walkthrough 📝

Relevant files
Bug fix
AccountBalancePieChart.tsx
Fix big number conversion in AccountBalancePieChart           

explorer/src/components/Consensus/Account/AccountBalancePieChart.tsx

  • Fixed a bug in the conversion of otherNumber to a big number.
+1/-1     
number.ts
Add falsy check for bigNumber conversion                                 

explorer/src/utils/number.ts

  • Added a check to return 0 if bigNumber is falsy.
+1/-0     

💡 PR-Agent usage:
Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

@marc-aurele-besner marc-aurele-besner merged commit 3734619 into production Sep 9, 2024
11 of 14 checks passed
Copy link

netlify bot commented Sep 9, 2024

Deploy Preview for astral-prod ready!

Name Link
🔨 Latest commit cd93a13
🔍 Latest deploy log https://app.netlify.com/sites/astral-prod/deploys/66defea6be3433000863626f
😎 Deploy Preview https://deploy-preview-847--astral-prod.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Sep 9, 2024

Deploy Preview for dev-astral ready!

Name Link
🔨 Latest commit cd93a13
🔍 Latest deploy log https://app.netlify.com/sites/dev-astral/deploys/66defea623d1610008d37c3f
😎 Deploy Preview https://deploy-preview-847--dev-astral.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

github-actions bot commented Sep 9, 2024

PR Reviewer Guide 🔍

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Key issues to review

Possible Bug
The variable otherNumber is used but not defined or imported in this file. Ensure that otherNumber is properly declared and available in this scope.

Code Improvement
The check for bigNumber being falsy should also handle cases where bigNumber might be an invalid number format. Consider adding more robust validation or error handling for different types of input.

Copy link

github-actions bot commented Sep 9, 2024

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    Score
Possible issue
Ensure bigNumberToNumber handles only expected types by adding a type check

Add a type check for bigNumber in the bigNumberToNumber function to ensure it
handles only expected types (string, bigint, or number). This prevents potential
runtime errors when unexpected types are passed.

explorer/src/utils/number.ts [29]

-if (typeof bigNumber !== 'string') bigNumber = bigNumber.toString()
+if (['string', 'bigint', 'number'].includes(typeof bigNumber)) {
+    bigNumber = bigNumber.toString()
+} else {
+    throw new TypeError('bigNumberToNumber expects a string, bigint, or number');
+}
 
Suggestion importance[1-10]: 9

Why: Adding a type check for bigNumber ensures that the function handles only expected types, preventing potential runtime errors and improving the reliability of the function. This is a significant improvement in terms of type safety.

9
Possible bug
Add a validation check for otherNumber to ensure it's a valid number before conversion

Consider checking if otherNumber is not only truthy but also a valid number before
converting it using bigNumberToNumber. This ensures that the conversion function
does not receive unexpected types or values.

explorer/src/components/Consensus/Account/AccountBalancePieChart.tsx [18]

-const other = otherNumber ? bigNumberToNumber(otherNumber) : 0
+const other = typeof otherNumber === 'number' && !isNaN(otherNumber) ? bigNumberToNumber(otherNumber) : 0
 
Suggestion importance[1-10]: 8

Why: The suggestion adds a validation check to ensure otherNumber is a valid number before conversion, which can prevent potential runtime errors and improve code robustness.

8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant