use openExternal async

[vilmibm]

Overview

Fixes #8156

Description

This PR corrects our use of Electron's shell.openExternal since it changed to become async by default. There is only one invocation of openExternal where we check the result and potentially show an error; this code path should function once more as of this PR.

Release notes

Notes: no-notes

[billygriffin]

Thanks @vilmibm!

From #8156:

There are several other places that we use shell.openExternal in the code, so we'll want to do a thorough audit and make sure to use the appropriate method in each case.

So am I understanding correctly that we use it elsewhere, but it's only in this one instance you found that we actually need to make the change?

[vilmibm]

@billygriffin

So am I understanding correctly that we use it elsewhere, but it's only in this one instance you found that we actually need to make the change?

We use it in many places; however, in most places we just call it and discard the result. There is only one code path that checks the result, and it didn't need changing; I had to change our lower level wrapper around Electron's openExternal to actually return a success/fail boolean so that that one existing code path can work again.

[outofambit]

@vilmibm thank you! 🎉 so from what you saw we should be safe with async behavior in places we probably assumed were sync in the past?

[vilmibm]

@outofambit yup! in most places we just called it as openExternal(url) which would have the desired behavior async or not. in just that one place I linked[1] were we actually checking for an error result.

[1]

desktop/app/src/ui/lib/open-file.ts

Line 8 in f37e6bf

const result = await shell.openExternal(`file://${fullPath}`)

[outofambit]

@vilmibm awesome! pretty sure that was the case, but just wanted to be sure! ✨

[outofambit]

[mislav]

This looks good 👍

As a newcomer to the codebase, I found it slightly confusing that we have calls to a shell.openExternal() method in at least 2 places where the shell reference was to a very different object (Electron vs. IAppShell). However, this isn't something that this PR has introduced nor needs concern itself about.

[kuychaco]

Thanks for fixing this @vilmibm! This looks ship-ready to me 👍

I do have one non-blocking comment, which can be addressed in a separate PR if we decide it's worth addressing.

So am I understanding correctly that we use it elsewhere, but it's only in this one instance you found that we actually need to make the change?

We use it in many places; however, in most places we just call it and discard the result. There is only one code path that checks the result, and it didn't need changing

Absolutely true 💯. There is also an error handling aspect worth considering. In the other places that we use shell.openExternal (here and here), if the operation fails then we won't find out about it since the promise will get rejected asynchronously.

From the user's perspective, silent failures here aren't the worst thing in the world because if the operation fails it'll already be pretty apparent by the fact that the webpage didn't open and it seems that there's no subsequent logic relying on successful execution. However, it could certainly save us developers some debugging if we catch and log the errors when they occur.

[vilmibm]

@kuychaco seems worth a follow-up issue so we can collect UX feedback if needed. would you like to open one or should I?

[billygriffin]

seems worth a follow-up issue so we can collect UX feedback if needed. would you like to open one or should I?

@vilmibm @kuychaco If we don't feel like it's a thing we should do immediately, I'd prefer not to open an issue about it unless it surfaces in the wild. Does that seem reasonable? If you think it's worth adding logging for that type of case before merging this PR, I'm totally fine with that. Just for something that stays open and isn't super likely to get prioritized, I'd rather see it surface through a user-reported problem instead of trying to over-optimize ahead of time.

[billygriffin]

Hi @vilmibm and @kuychaco! Just wanted to follow up to see if y'all wanted to do additional logging here or if this PR should otherwise be merged?

[vilmibm]

@billygriffin I'm comfortable merging as is.

[kuychaco]

@billygriffin I'm totally cool to merge as is and get this fix in. I would like to see us fix up the other places we use Electron's shell.openExternal method (like append a .catch with logging), because I think it's a minor and risk-free change that could potentially save us some debugging time in the future. But that can happen in a follow-on PR. I'll defer to @vilmibm's preference. And I can maybe see about fixing up the other places when I've got a bit more bandwidth.