-
Notifications
You must be signed in to change notification settings - Fork 21
Wallet and review phase display issue fixes #1280
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
Conversation
| return false | ||
| } | ||
|
|
||
| if (fragment === 'iterative' && hasExplicitReviewPhase) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[correctness]
The check for fragment === 'iterative' is case-sensitive. If there is any chance that fragment could be in a different case (e.g., 'Iterative'), consider normalizing the case of fragment before comparison to ensure consistency.
| ) | ||
| } | ||
| }, [walletDetails]) | ||
| const balanceSum = useMemo( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[performance]
Using useMemo here is appropriate for performance optimization, but ensure that walletDetails.account.balances is not mutated elsewhere in the component lifecycle, as it could lead to stale values being used.
| if (typeof error === 'string') { | ||
| errorMessage = error | ||
| } else if (error && typeof error === 'object' && 'message' in error) { | ||
| errorMessage = (error as Error).message || errorMessage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[correctness]
The error handling logic assumes that the error object has a message property. Consider adding a type guard or validation to ensure this property exists to avoid potential runtime errors.
| export interface Response<T> { | ||
| data?: Readonly<T> | ||
| error?: Readonly<string> | ||
| error?: Readonly<Error> | string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[correctness]
Changing error to Readonly<Error> | string allows for more flexibility, but ensure that all parts of the code handling this error property can correctly process both Error objects and strings. This change might require additional checks or transformations where error is used to avoid runtime issues.
Refactored the HomeTab in wallet to keep under lint complexity complaints and also fixed how it handles errors that are objects, not strings, which was causing a problem loading the page.
Also includes a fix to the review UI where a recent fix caused reviews to not show in certain cases.