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

[HOTFIX] Browser: handle unsupported URLs #5799

Merged
merged 2 commits into from
Feb 21, 2023

Conversation

jpcloureiro
Copy link
Contributor

@jpcloureiro jpcloureiro commented Feb 20, 2023

Description

In-App browser do not handle special URLs (deeplinks like market://details?...)

Change from native URL API to url-parse package.
This allows the app to handle unsupported URLs instead of throwing an error.

Screenshots/Recordings

Screen that will appear when the user tries to use a special URL / deeplink inside In-App browser
image

Issue

Progresses #5794

this will prevent app crashes when url is malformed &
let the error be handled
@jpcloureiro jpcloureiro added needs-qa Any New Features that needs a full manual QA prior to being added to a release. needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) release-6.0.1 labels Feb 20, 2023
@jpcloureiro jpcloureiro requested a review from a team as a code owner February 20, 2023 21:30
@jpcloureiro jpcloureiro self-assigned this Feb 20, 2023
@github-actions
Copy link
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@jpcloureiro jpcloureiro changed the title use url-parse instead of native URL api [HOTFIX] Browser: handle unsupported URLs Feb 20, 2023
@Gudahtt
Copy link
Member

Gudahtt commented Feb 20, 2023

We are already using this polyfill: https://github.com/charpeni/react-native-url-polyfill which apparently follows the whatwg standard for URL, which does support custom protocols. I'm curious to know which specific scenario is failing that url-parse fixes.

@jpcloureiro
Copy link
Contributor Author

jpcloureiro commented Feb 20, 2023

We are already using this polyfill: https://github.com/charpeni/react-native-url-polyfill which apparently follows the whatwg standard for URL, which does support custom protocols. I'm curious to know which specific scenario is failing that url-parse fixes.

Invalid URL error handling needs to be implemented before replacing parse-url with the native URL API.

In this case market://details?src=io.metamask. ... deeplinks are transformed into an invalid URL in the form of null?src=io.metamask.

url-parse being designed to not throw errors (like legacy nodejs url API) keeps the app from crashing

This hotfix will prevent users from being stuck on the error boundary screen.

A proper way to handle URLs should be implemented next

@Gudahtt
Copy link
Member

Gudahtt commented Feb 20, 2023

deeplinks are transformed into an invalid URL

Ahhh I see. I thought you meant that new URL was throwing directly. That makes more sense.

Copy link
Member

@andreahaku andreahaku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@chrisleewilcox
Copy link
Contributor

@jpcloureiro what is expected if user has coingecko app installed on device? Will coingecko app open? Can you confirm this? Will need this confirmed in development before handing this over to QA.

Copy link
Contributor

@chrisleewilcox chrisleewilcox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please review my comments about behavior for coingecko app on device.

@sethkfman sethkfman removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Feb 21, 2023
@jpcloureiro
Copy link
Contributor Author

@chrisleewilcox
In 5.14.0, the browser presents the "Something went wrong" screen regardless of having the coingecko app installed or not.

I would say this is the expected behaviour for this PR as well.

@chrisleewilcox chrisleewilcox merged commit 8a46793 into release/6.0.1 Feb 21, 2023
@chrisleewilcox chrisleewilcox deleted the fix/browser-crash-unhandled-url branch February 21, 2023 17:41
@github-actions github-actions bot locked and limited conversation to collaborators Feb 21, 2023
@chrisleewilcox chrisleewilcox added QA Passed A successful QA run through has been done and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Feb 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
QA Passed A successful QA run through has been done release-6.0.1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants