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

Use Electron's shell.openpath() for pcap slices #2992

Merged
merged 3 commits into from
Feb 7, 2024
Merged

Conversation

philrz
Copy link
Contributor

@philrz philrz commented Jan 28, 2024

tl;dr

While debugging #2987, I burned excess time going down the rabbit hole being suspicious of apps/zui/src/js/lib/open.ts. While it turns out that was not the cause of the problems, I learned a lot about the history of the code and have learned that we're not even using it for its original purpose anymore and we can instead can call Electron's shell.openPath(path). Alas, even as I do that in this PR, it seems I can't quite remove the open.ts altogether, but I assume with a little help from @jameskerr we could get there.

Details

The history of that open.ts starts with PROD-1607 in our old issue tracking system. I was doing early demos of the Brim app and ran into a behavior unique to macOS such that if I already had a big pcap in the middle of being opened in Wireshark (i.e., to show how slow it is) and I tried to simultaneously open a small pcap slice from within Brim (i.e., to show how much faster it was) Wireshark would effectively "block" and refuse to open up the small slice until the big pcap was done opening. This same effect didn't happen on Windows and Linux. On those a separate Wireshark window would open up for the pcap slice.

I learned that on macOS I could get the non-blocking behavior from the shell if I launched Wireshark with open -n and pleaded with the dev team to hack something up to do the same in the app so I could have an effective demo, and @mason-fish did just this in #487. The change consisted of copying over https://github.com/sindresorhus/open and modifying it to do open -n on macOS.

That did its job for a few years, but we stopped leveraging it for its original purpose when #2759 merged. At that point the {newWindow: true} in this line:

open(pcapFile, {newWindow: true})

did not make the move into the "options" parameter in this line:

env.openExternal(dest)

So we've been back to "blocking" pcap slice behaviors on macOS since May of 2023. No users have spoken up about it, and I never noticed either despite being the person that originally asked for the change. Meanwhile, the README of https://github.com/sindresorhus/open advises:

If you need this for Electron, use shell.openPath() instead.

and @jameskerr spotted much the same even before we saw the README. The one down side is that shell.openPath() exhibits the "blocks on macOS, not on Windows or Linux" behavior and has no option to change that. But I'm personally happy to live without it now in exchange for making the code simpler.

Unfortunately, I was only able to pull the sweater thread to the degree you see in this PR, which left me short of being able to actually delete open.ts from the repo. I think apps/zui/src/electron/ops/open-link-op.ts can switch over to using Electron's shell.openExternal(), but apps/zui/src/js/components/Login.test.tsx still does some Jest mocking stuff that points at open.ts so I'm not confident I know what it takes to unwind that. I'm hoping @jameskerr can make the necessary surgical adjustments.

Testing

FWIW, I did confirm that if I put back the {newWindow: true} in current tip of main we get back the originally desired behavior on macOS, but once again, I'm fine living without it at this point to get the code simplicity. Once I started using the shell.openPath() approach in this branch, I confirmed via Actions Runs that the e2e packets tests still run fine on all OSes and the full CI e2e suite runs fine on Linux as we always run it. I also manually tested on all three OSes that we still maintain the non-blocking behavior on Windows and Linux and that we're back to blocking on macOS.

Question

Assuming we go ahead with this, should I go ahead and open a feature request on Electron to inquire about adding an option to shell.openPath() to get this back on macOS? I figure if it appeared one day, I'd happily go back to leveraging it. I went looking for an existing issue and I couldn't find one. It strikes me as odd that others haven't noticed this since Wireshark is definitely not the only app where I could imagine this being noticed and I'd expect macOS users to care that they have what appears to be the more annoying behavior across the three OSes.

@philrz philrz requested a review from jameskerr January 28, 2024 21:35
@philrz philrz self-assigned this Jan 28, 2024
@philrz philrz merged commit 7e9525a into main Feb 7, 2024
3 checks passed
@philrz philrz deleted the elec-shell-openpath branch February 7, 2024 17:55
@philrz
Copy link
Contributor Author

philrz commented May 14, 2024

Per the "Question" in this PR's opening text above, I've gone ahead and opened an Electron enhancement request at electron/electron#42171 to see if there's any interest in adding a "non-blocking" option for shell.openPath() on macOS. If that's ever offered I'll gladly make the change so Zui can start leveraging it to provide consistent behavior across all of macOS, Linux, and Windows.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide guidance on macOS/Linux when no app is tied to opening of pcaps
2 participants