-
Notifications
You must be signed in to change notification settings - Fork 39
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
[Gateway] remove listener for account change #1689
[Gateway] remove listener for account change #1689
Conversation
WalkthroughThe recent updates to the wallet extension involve a reorganization of where version information is displayed and a refinement of the wallet connection logic. The footer now shows the version number, while the connected component no longer does. The wallet provider's initialization process has been improved, particularly in handling the presence of Changes
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 X ? TipsChat with CodeRabbit Bot (
|
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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- tools/walletextension/frontend/src/components/layouts/footer.tsx (2 hunks)
- tools/walletextension/frontend/src/components/modules/home/connected.tsx (2 hunks)
- tools/walletextension/frontend/src/components/providers/wallet-provider.tsx (2 hunks)
Additional comments: 8
tools/walletextension/frontend/src/components/layouts/footer.tsx (2)
8-11: The import of
useWalletConnection
and its usage to get theversion
is consistent with the PR's objective to refactor how version information is displayed.41-41: The conditional rendering of the
version
with a fallback to "Unknown" is a good practice to handle cases whereversion
might be undefined or null.tools/walletextension/frontend/src/components/modules/home/connected.tsx (2)
19-19: The removal of the
version
variable from the destructuring of theuseWalletConnection
hook is consistent with the PR's objective to remove the display of version information from theConnected
component.16-22: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [16-88]
After reviewing the entire
Connected
component, it appears that the removal of theversion
variable has no adverse effects on the remaining code. The variable is not referenced anywhere else in the component, which suggests that the change is safe and the component should function as expected without displaying version information.tools/walletextension/frontend/src/components/providers/wallet-provider.tsx (4)
48-54: The addition of the conditional check for
ethereum
in theinitialize
function is correct and follows best practices for early return if a required condition is not met.164-167: The
useEffect
hook is correctly used to callinitialize
on component mount. However, ensure that suppressing the linter warning for missing dependencies is intentional and that no dependencies are actually missing from the dependency array.43-64: While the hunk does not show the removal of the
handleAccountsChanged
function, it's important to ensure that any code that depended on this function is updated or removed accordingly to prevent potential issues.
#!/bin/bash # Search for any remaining references to `handleAccountsChanged` to ensure they have been properly updated or removed. rg 'handleAccountsChanged' src/**/*.tsx
- 43-64: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [43-171]
The PR overview mentions changes to the
Connected
andFooter
components, which are not part of the provided hunks. Ensure that these changes are consistent with the rest of the codebase and that theuseWalletConnection
hook is being used correctly in these components.
#!/bin/bash # Correct the pattern to verify that the `Connected` component no longer destructures the `version` variable. ast-grep --lang typescript --pattern $'const Connected = { $$$ const { $_, $_, $_, version } = useWalletConnection(); $$$ }' # Correct the pattern to verify that the `Footer` component now uses the `useWalletConnection` hook to display the `version`. ast-grep --lang typescript --pattern $'const Footer = { $$$ const { version } = useWalletConnection(); $$$ }'
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.
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.
LGTM 👍
Why this change is needed
Please provide a description and a link to the underlying ticket
https://github.com/ten-protocol/ten-internal/issues/2583
What changes were made as part of this PR
Please provide a high level list of the changes made
PR checks pre-merging
Please indicate below by ticking the checkbox that you have read and performed the required
PR checks