urls: Tell user when tapped link doesn't parse; fix a bug affecting iOS downloads #5650
Conversation
chrisbobbe
force-pushed
the
pr-link-opening-fixes
branch
from
4638aee to
14b64c1
Compare
src/utils/openLink.js
Outdated
| export function openLinkEmbedded(url: URL): void { | ||
| if (Platform.OS === 'ios') { | ||
| WebBrowser.openBrowserAsync(encodeURI(url)); | ||
| WebBrowser.openBrowserAsync(encodeURI(url.toString())); |
There was a problem hiding this comment.
Hmm, does this version indeed work for fixing #4136?
Based on the discussion in that thread, I'd expect the issue to be present as long as this encodeURI call is here.
Once the URL always comes from a URL object, we should be able to just remove the encodeURI call; that should fix #4136, and when the URL is coming from a URL object it shouldn't reintroduce #3315.
I guess this illustrates that before closing #4136 we should do some actual testing of the fix 😉 Per the issue description:
A key step in fixing this is going to be just end-to-end testing: make a URL filled with a ton of these characters, post it in a Zulip message, try following that link, and see what URL actually comes through.
There was a problem hiding this comment.
Ah right, thanks for the catch! I'll be sure to test my next revision manually. 🙂
Do you think we have to round-trip through new URL and remove the encodeURI in the same commit, or is it OK to have a commit that does new URL and still has encodeURI in place?
There was a problem hiding this comment.
or is it OK to have a commit that does
new URLand still hasencodeURIin place?
I think that'd be OK, as long as we're merging at the same time a subsequent commit that removes the encodeURI.
It would make #4136 apply more widely, so I wouldn't want to release in that state and hence wouldn't want to merge a branch that left things in that state. But the new URL change ought to be harmless, and the fact that it isn't is because of an existing bug (the use of encodeURI), so I'm OK with having a commit in the history that does that first.
Just before this commit, if an `href` fails to parse, it would do so in the `new URL` call that's meant to pass a URL object to `getNarrowFromLink` (the line with the comment "TODO: Use parsedUrl, below"). The error would propagate up to messageLinkPress's caller, in handleWebViewOutboundEvent, and from there to a React event handler, with the result (I expect) that the app wouldn't crash, but it also wouldn't give any useful feedback to the user. So, give some useful feedback to the user. The issue is filed as "Fail early (and tell user) on a message-link tap when the URL doesn't parse" (zulip#5518) . I guess we have been failing early, we just haven't been telling the user. Fixes: zulip#5518
For the following URLs, this adds a round-trip through the URL constructor before passing it along to be opened: - https://zulip.readthedocs.io/en/stable/overview/release-lifecycle.html#compatibility-and-upgrading - https://zulip.readthedocs.io/en/stable/production/mobile-push-notifications.html - https://zulip.com/policies/?nav=no - https://itunes.apple.com/app/zulip/id1203036395 - https://play.google.com/store/apps/details?id=com.zulipmobile - the URL passed to webAuth.openBrowser (which was actually already round-tripped through the URL constructor; see its caller) - A tapped link in a message, when the link is off-realm We expect just the last of these to have a functional change: it'll make zulip#4136 apply more widely ("[iOS] Links get double-%-encoded, breaking downloads"): zulip#5650 (comment) But we'll fix zulip#4136 in the next commit.
… (iOS) This doesn't re-introduce zulip#3315 (SafariView complaining on non-ASCII URLs) because all non-ASCII characters are still percent-encoded. That gets done by the URL constructor, which we started using in this codepath in the previous commit. See discussion and more detail: zulip#4136 (comment) Fixes: zulip#4136
chrisbobbe
force-pushed
the
pr-link-opening-fixes
branch
from
14b64c1 to
1400979
Compare
|
Thanks for the review! Revision pushed. For testing, I didn't manage to get my hands on a working file upload link; for some reason neither CZO nor a Zulip Cloud realm (Recurse) were giving me an S3 URL. But I did try this link: https://www.google.com/search?q=0%25+de+100%C2%A2+%C3%A7a+fait+combien+de+%24+%2F+%E2%82%AC%E2%80%8E On
And at the tip of this PR branch, it pre-fills with this (much nicer, and what I'd intended):
|
|
Thanks! Looks good; merging. |
2 participants
Fixes: #5518
Fixes: #4136