-
Notifications
You must be signed in to change notification settings - Fork 394
adrienne / added modal provider for wallets #10191
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
adrienne / added modal provider for wallets #10191
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
A production App ID was automatically generated for this PR. (log)
Click here to copy & paste above information. |
|
🚨 Lighthouse report for the changes in this PR:
Lighthouse ran with https://deriv-app-git-fork-adrienne-deriv-wallets-generic-modal.binary.sx/ |
…iv-app into wallets-generic-modal
|
SonarCloud Quality Gate failed.
|
review-deriv
left a comment
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.
AI Review (AI review can be wrong. Do not use it as the only source of feedback)
This code seems reasonably organized and functional for a ModalProvider component and the provider is suitably incorporated in the main application.
However, there are a few areas that could be improved:
-
Null reference:
document.getElementById('wallets_modal_root')can return null and this isn't being handled. The code assumes that a div with id 'wallets_modal_root' would always exist, which if not met, could lead to further problems at runtime when attempting to use createPortal. -
Error Handling: The function
useModalthrows an error if the context isn't available, but there is no catch or user-friendly fallback set up. -
Security: There's no explicit security issue present in the given code since it doesn't deal with any sensitive user data, API requests etc. However, injecting user supplied contents within modal could lead to a potential risk of XSS(Cross Site Scripting) vulnerabilities.
In summary, the code is solid in terms of the basic functionality of modal display context setup, but it needs to address null reference issues and has somewhat barebones error handling. It doesn't explicitly have any security issues but care should be taken when integrating it.











Changes:
Please provide a summary of the change.
Screenshots:
Please provide some screenshots of the change.