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

Handle reconnect auth fail in the embed-widget app #1256

Closed
mofojed opened this issue May 1, 2023 · 4 comments · Fixed by #2023
Closed

Handle reconnect auth fail in the embed-widget app #1256

mofojed opened this issue May 1, 2023 · 4 comments · Fixed by #2023
Assignees
Labels
bug Something isn't working
Milestone

Comments

@mofojed
Copy link
Member

mofojed commented May 1, 2023

Description

Right now if the JS API throws a reconnect auth fail error, the embed-widget (embed-grid/embed-chart deprecated) packages do not handle it. The change #1149 to wire up the reconnect auth fail event only added it to code-studio package, so the embed-widget package would not react the same.

Possibly add it to AppBootstrap or some other common component between the three packages.

Follow these steps to reproduce an auth failure:

  1. Start up server with deephaven-core
  2. Use ngrok to start a tunnel to that port, e.g.: ngrok http 10000
  3. Start up Web UI connecting to that tunnel, e.g.: VITE_CORE_API_URL=http://acfc-23-233-0-34.ngrok.io/jsapi npm start
  4. Open the Web UI in Firefox, run some commands to make sure initial connection is fine.
  5. Press Alt and from the File menu, select "Work Offline"
  6. See it transition to a disconnected state. Disconnected message should appear and should not be able to enter new commands
  7. Reconnect by deselecting "Work Offline" option from Step 5
  8. See it transition to connected state. Should be able to run commands and have the results appear.
  9. Kill the server (Ctrl+C). See it transition to a Shutdown state, app unloaded.
@mofojed mofojed added bug Something isn't working triage Issue requires triage labels May 1, 2023
@vbabich vbabich added this to the May 2023 milestone May 3, 2023
@vbabich vbabich removed the triage Issue requires triage label May 3, 2023
@mofojed mofojed modified the milestones: May 2023, June 2023 Jun 19, 2023
@mofojed mofojed modified the milestones: June 2023, July 2023 Aug 4, 2023
@mofojed mofojed modified the milestones: July 2023, August 2023 Aug 28, 2023
@mofojed mofojed modified the milestones: August 2023, September 2023 Sep 11, 2023
@mofojed mofojed modified the milestones: September 2023, November 2023 Nov 2, 2023
@mofojed mofojed modified the milestones: November 2023, January 2024 Jan 9, 2024
@mofojed mofojed modified the milestones: January 2024, February 2024 Feb 13, 2024
@mofojed mofojed changed the title Handle reconnect auth fail in the embed-grid/embed-chart apps Handle reconnect auth fail in the embed-widget app Mar 12, 2024
@mofojed mofojed modified the milestones: February 2024, March 2024 Mar 12, 2024
@mofojed mofojed modified the milestones: March 2024, April 2024 Apr 12, 2024
@mofojed mofojed modified the milestones: April 2024, May 2024 May 13, 2024
@AkshatJawne
Copy link
Contributor

Update:

After trying and testing different approaches, feel that the approach where I add an event listener and corresponding functions to handle the Auth Failure event in ConnectionBootstrap.tsx is a viable approach.

I made code changes as shown below, and tested as discussed with @mattrunyon by clicking "Work Offline" to go offline -> Restarting the Deephaven server -> clicking "Work Offline" again to go online. This is seeming to work, but there are a couple of issues:

a. Adding the logic here would make most of the logic from lines 725 - 763 of AppMainContainer.tsx in the code-studio redundant, specifically the listening for the disconnect and the auth failed. However, we would still need something for handling reconnect there. Or, we could move all of it to the connection bootstrap, but I am hesistent about doing so until I know the reason why the ConnectionBootstrap.tsx only handled disconnect (not reconnect and auth failed) prior to my changes, since that was not one of the changes implemented in #1149

b. The modal that appears in the embed-widget is similar to the one that we see in the main code-studio, but there are styling differences, not sure why?

c. The refresh here works but then the variable does not persist, I know that there are workarounds with this with Application Mode and initializing server state, but I feel like a solution should not be contingent upon using a specific mode.

@mofojed @mattrunyon any thoughts?

Some of the code changes:

Screenshot 2024-05-16 at 9 03 49 AM
Screenshot 2024-05-16 at 9 04 06 AM

Difference in Modal Styling:

In Code Studio:
Screenshot 2024-05-16 at 9 16 37 AM

In Embedded Widget Screen:
Screenshot 2024-05-16 at 9 17 43 AM

@mofojed
Copy link
Member Author

mofojed commented May 16, 2024

@AkshatJawne
a) ConnectionBootstrap was created after the JS API Reconnect PR (#1180 was after #1149). Possible the ConnectionBootstrap PR was started in parallel and didn't add all the reconnect logic there, so I think reasonable to include that reconnect login in ConnectionBootstrap
b) It looks like some fonts may not be loading? I'd double check that the fonts are loading correctly in embed-widget.
c) The variable will not persist if you restart the Deephaven server; there isn't anything we can do about that, since it's a new server instance. Are you restarting the Deephaven server as part of the steps to reproduce? If not, then the variable should still exist, and we shouldn't be getting a variable not found error, something else must be going on.

@AkshatJawne
Copy link
Contributor

a) ConnectionBootstrap was created after the JS API Reconnect PR (#1180 was after #1149). Possible the ConnectionBootstrap PR was started in parallel and didn't add all the reconnect logic there, so I think reasonable to include that reconnect login in ConnectionBootstrap

This makes sense, I just wanted to make sure that my changes were not taking the wrong side of a design tradeoff that may have been made when creating the ConnectionBootstrap

b) It looks like some fonts may not be loading? I'd double check that the fonts are loading correctly in embed-widget.

Yes, it appears so, will take a deeper dive here

Are you restarting the Deephaven server as part of the steps to reproduce? If not, then the variable should still exist, and we shouldn't be getting a variable not found error, something else must be going on.

Yes I am, that was the way that @mattrunyon and I could reproduce the issue. But yeah, the creation of the new server instance having that behaviour makes sense, and if it is unavoidable, then no issues on that front.

Will continue working on this and will update on progress, thanks for the comment @mofojed

@mattrunyon
Copy link
Collaborator

You could restart the server and then recreate the variable in a separate browser. Or run the server using app mode to create it at startup.

Unless there's another easy way to simulate the auth reconnect failure. @mofojed

@mofojed mofojed modified the milestones: May 2024, June 2024 Jun 4, 2024
AkshatJawne added a commit that referenced this issue Jun 12, 2024
Resolves #1256

**Changes Implemented:** 
- Refactored disconnection, shutdown, and auth fail handling logic to
now be `ConnectionBootstrap` in order to properly handle auth fail event
in the `embed-widget`

**Visual of modal that appears on auth fail:** 

![image](https://github.com/deephaven/web-client-ui/assets/69530774/a0874c7e-a4f3-40fb-ad1f-23fe75012b1f)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants