- Notifications
You must be signed in to change notification settings - Fork923
Description
Is there an existing issue for this?
- I have searched the existing issues
Current Behavior
We have a few UI components using ouruseAppLink
hook (which includes our workspace and task components).
With how the hook works right now:
- The hook will give back both an external link and a click event handler, with the expectation that both will be attached to a "link-like" element. The link is what opens a separate window for launching the app, and the event handler is meant to provide additional safety nets in case something goes wrong.
- The link click event handler, when called, starts a 500ms timeout for warning the user that something went wrong. It also attaches a blur event handler to the window. When the external link is processed by the browser, it will fire a blur event, and focus will shift to the window that the external link created.
- This triggers the blur event handler, and as long as it fires quickly enough, it will clear out the timeout.
The main problem is that this is the current clear logic:
window.addEventListener("blur",()=>{clearTimeout(openAppExternallyFailed);});
The base Window blur event is incredibly broad and can fire for any number of reasons (like the user switching to a different tab that has nothing to do with Coder). So, depending on how long it takes for the new window to be created, it's possible for the user to trigger the blur event early from a different source. If the user actually has a problem with their setup, they would lose all feedback about what they need to do to get themselves unstuck.
Granted, this risk is fairly minor right now, but as we add more and more functionality to the app and keep adding stuff that interacts with JS's event loop, the risks of this happening more often will keep increasing. Similarly, if a new window is ever made without triggering the blur event, the user will be warned about a problem that doesn't exist.
Expected Behavior
The hook should be updated to have more fine-grained event listening logic to detect when a new window is created, rather than listening to every blur event indiscriminately.
Steps to Reproduce
I'm still trying to figure out a way to reproduce this reliably, because there's not really much you can do to artificially throttle the browser processing a link, since that doesn't happen inside JS. There might be a way to add extra dummy blur event listeners that take an artificially long amount of time, and that force the main blur logic to miss the timeout window (because it'll be processed after the others).
Once we have a way to artificially delay the blur event processing:
- Go to any workspace/task UI component that uses the
useAgentLink
hook. The most obvious one is the the workspaces table when you first log in. - Click any link that has an external integration (such as with JetBrains), and critically, make sure that all external data dependencies are installed (JetBrains has JetBrains Toolbox, etc.)
- Wait over half a second
- See that even though the link was created just fine, the user still got a toast notification about the external dependencies not being installed.
Environment
- Host OS: MacOS 13.6.7 (22G720)
- Coder version: v2.23.1-devel+56ff0fb65
Additional Context
I have tested this on the latest version