feat: JS API reconnect by mofojed · Pull Request #1149 · deephaven/web-client-ui

mofojed · 2023-03-14T15:55:37Z

Listen for EVENT_DISCONNECT and EVENT_RECONNECT on the connection, and display a "Reconnecting..." message in the console.
No longer listen to the HACK_CONNECTION_FAILURE (it's been deprecated).
Listen for the SHUTDOWN event and display a message after shutdown.
Fixes Support reconnecting to the server #1140
Testing steps:

Start up server with deephaven-core JS API reconnect changes: JS API should attempt to reconnect to the gRPC server deephaven-core#3502
Use ngrok to start a tunnel to that port, e.g.: ngrok http 10000
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
Open the Web UI in Firefox, run some commands to make sure initial connection is fine.
Press Alt and from the File menu, select "Work Offline"
See it transition to a disconnected state. Disconnected message should appear and should not be able to enter new commands
Reconnect by deselecting "Work Offline" option from Step 5
See it transition to connected state. Should be able to run commands and have the results appear.
Kill the server (Ctrl+C). See it transition to a Shutdown state, app unloaded.

- Doesn't work correctly yet - Not sure how best to test locally

- Added unit tests - Display a message in Console when disconnected

codecov · 2023-03-23T16:36:56Z

Codecov Report

Merging #1149 (b72e295) into main (2ac25ae) will increase coverage by 0.03%.
The diff coverage is 27.58%.

@@            Coverage Diff             @@
##             main    #1149      +/-   ##
==========================================
+ Coverage   44.15%   44.19%   +0.03%     
==========================================
  Files         447      448       +1     
  Lines       33267    33310      +43     
  Branches     8356     8364       +8     
==========================================
+ Hits        14688    14720      +32     
- Misses      18530    18541      +11     
  Partials       49       49

Flag	Coverage Δ
unit	`44.19% <27.58%> (+0.03%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
packages/code-studio/src/main/AppInit.tsx	`0.00% <0.00%> (ø)`
packages/components/src/modal/InfoModal.tsx	`0.00% <0.00%> (ø)`
packages/console/src/HeapUsage.tsx	`0.00% <0.00%> (ø)`
...dashboard-core-plugins/src/panels/ConsolePanel.tsx	`46.04% <ø> (ø)`
packages/code-studio/src/main/AppMainContainer.tsx	`34.96% <64.00%> (+2.78%)`	⬆️

... and 6 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

mattrunyon

I left the window in the disconnected state for 2 minutes and when re-enabling network, the "Attempting to reconnect" stays visible. At some point, it attempts to reconnect, but the credentials are invalid. That info should be propagated to the user and prompt them to reload the page to reauthenticate

That error was an uncaught promise though, so might need to be propagated through the JS API in a cleaner way.

Uncaught (in promise) Error: Authentication details invalid
    onEnd http://8f2c-50-24-89-157.ngrok.io/jsapi/dh-internal.js:1
    unary http://8f2c-50-24-89-157.ngrok.io/jsapi/dh-internal.js:1
    rawOnEnd http://8f2c-50-24-89-157.ngrok.io/jsapi/dh-internal.js:1
    rawOnEnd http://8f2c-50-24-89-157.ngrok.io/jsapi/dh-internal.js:1
    onTransportEnd http://8f2c-50-24-89-157.ngrok.io/jsapi/dh-internal.js:1
    $onMessage http://8f2c-50-24-89-157.ngrok.io/jsapi/dh-core.js:19095
    handleEvent_2 http://8f2c-50-24-89-157.ngrok.io/jsapi/dh-core.js:19237

mattrunyon · 2023-03-27T22:30:39Z

    getObject: jest.fn(),
    runCode: jest.fn(),
-  };
+  } as unknown) as IdeSession;


Would be better to just mock all of IdeSession if it's not a pain. Shouldn't really affect the tests, but if IdeSession changes shape we don't know about it in the tests unless they fail

It's a bit of a pain in the ass. class Table doesn't match the types, missing many methods, and doesn't see the static events (since our typings expect them to be properties instead of static). Cleaning up mock should happen after we start using the actual types published by JS API.
I did a bit of cleanup, but was going down a bit of a yak shaving exercise.

- Mock IdeSession a bit more, add some more mock methods for Table - Will finish cleanup after proper types are published - Listen to RECONNECT_AUTH_FAILED event

- TableCsvExporter was depending on Table not having a `freeze` method

- Informs the user an error has occurred and they can press Refresh to try again

dsmmcken

Approving based on the screenshots you sent.

WIP JS API reconnect

69ef8ee

- Doesn't work correctly yet - Not sure how best to test locally

mofojed mentioned this pull request Mar 14, 2023

JS API should attempt to reconnect to the gRPC server deephaven/deephaven-core#3502

Merged

mofojed added 2 commits March 23, 2023 09:39

Merge remote-tracking branch 'origin/main' into jsapi-reconnect

fbb8faf

Add support for disconnect/reconnect

53e81e4

- Added unit tests - Display a message in Console when disconnected

mofojed requested a review from mattrunyon March 23, 2023 16:35

mofojed self-assigned this Mar 23, 2023

mofojed added this to the March 2023 milestone Mar 23, 2023

mofojed marked this pull request as ready for review March 23, 2023 16:36

Refactor to use an InfoModal instead

6edc067

mattrunyon requested changes Mar 27, 2023

View reviewed changes

Address review comments

d1abcc5

- Mock IdeSession a bit more, add some more mock methods for Table - Will finish cleanup after proper types are published - Listen to RECONNECT_AUTH_FAILED event

mofojed requested a review from mattrunyon March 28, 2023 18:01

Fix broken unit test from updating the mock

4bbff41

- TableCsvExporter was depending on Table not having a `freeze` method

mattrunyon requested changes Mar 28, 2023

View reviewed changes

Comment thread packages/code-studio/src/main/AppInit.tsx Outdated

Show a prompt when authentication fails to refresh the browser

b72e295

- Informs the user an error has occurred and they can press Refresh to try again

mofojed requested review from dsmmcken and mattrunyon March 29, 2023 17:33

mattrunyon approved these changes Mar 29, 2023

View reviewed changes

dsmmcken approved these changes Mar 29, 2023

View reviewed changes

mofojed merged commit 15551df into deephaven:main Mar 29, 2023

mofojed deleted the jsapi-reconnect branch March 29, 2023 18:50

github-actions Bot locked and limited conversation to collaborators Mar 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: JS API reconnect#1149

feat: JS API reconnect#1149
mofojed merged 7 commits intodeephaven:mainfrom
mofojed:jsapi-reconnect

mofojed commented Mar 14, 2023 •

edited

Loading

Uh oh!

codecov Bot commented Mar 23, 2023 •

edited

Loading

Uh oh!

mattrunyon left a comment

Uh oh!

mattrunyon Mar 27, 2023

Uh oh!

mofojed Mar 28, 2023

Uh oh!

Uh oh!

dsmmcken left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

mofojed commented Mar 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Mar 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mattrunyon left a comment

Choose a reason for hiding this comment

Uh oh!

mattrunyon Mar 27, 2023

Choose a reason for hiding this comment

Uh oh!

mofojed Mar 28, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dsmmcken left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mofojed commented Mar 14, 2023 •

edited

Loading

codecov Bot commented Mar 23, 2023 •

edited

Loading