Skip to content

fix: Reconnect Auth Fail Fix - embed-widget#2023

Merged
AkshatJawne merged 17 commits intodeephaven:mainfrom
AkshatJawne:1256-embed-widget-auth-fail-fix
Jun 12, 2024
Merged

fix: Reconnect Auth Fail Fix - embed-widget#2023
AkshatJawne merged 17 commits intodeephaven:mainfrom
AkshatJawne:1256-embed-widget-auth-fail-fix

Conversation

@AkshatJawne
Copy link
Copy Markdown
Contributor

@AkshatJawne AkshatJawne commented May 16, 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

@AkshatJawne AkshatJawne requested review from mattrunyon and mofojed May 16, 2024 19:18
@AkshatJawne AkshatJawne self-assigned this May 16, 2024
@AkshatJawne AkshatJawne marked this pull request as draft May 16, 2024 19:18
@AkshatJawne AkshatJawne marked this pull request as ready for review May 16, 2024 19:48
Comment thread packages/app-utils/src/components/ConnectionBootstrap.tsx Outdated
Comment thread packages/app-utils/src/components/ConnectionBootstrap.tsx Outdated
Comment thread packages/app-utils/src/components/ConnectionBootstrap.tsx Outdated
Comment thread packages/app-utils/src/components/ConnectionBootstrap.tsx Outdated
Comment thread packages/embed-widget/src/index.scss
Comment thread packages/app-utils/src/components/ConnectionBootstrap.tsx Outdated
Comment thread packages/app-utils/src/components/ConnectionBootstrap.tsx Outdated
@AkshatJawne AkshatJawne requested a review from mattrunyon May 28, 2024 20:01
Comment thread packages/app-utils/src/components/ConnectionBootstrap.tsx Outdated
Comment thread packages/app-utils/src/components/ConnectionBootstrap.tsx Outdated
Comment thread packages/code-studio/src/main/AppMainContainer.tsx Outdated
Comment thread packages/app-utils/src/components/ConnectionBootstrap.tsx
Comment thread packages/code-studio/src/main/AppMainContainer.tsx Outdated
@AkshatJawne AkshatJawne requested a review from mattrunyon May 29, 2024 17:32
Copy link
Copy Markdown
Collaborator

@mattrunyon mattrunyon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also noticed this doesn't stop listening when the server is shutdown which is different behavior than what we currently have. I tested this by just stopping/starting the core server. The app showed the "auth failed" modal in that case. Previously this would just stay on the "Server shutdown" component.

Comment thread packages/embed-widget/src/index.tsx
Comment thread packages/app-utils/src/components/ConnectionBootstrap.tsx Outdated
@AkshatJawne
Copy link
Copy Markdown
Contributor Author

I also noticed this doesn't stop listening when the server is shutdown which is different behavior than what we currently have. I tested this by just stopping/starting the core server. The app showed the "auth failed" modal in that case. Previously this would just stay on the "Server shutdown" component.

@mattrunyon Have been testing on my local branch, not able to reproduce, when I shut down the core server and start it again, I do not see the "auth failed" modal, it just stays on the "Server shutdown".

@AkshatJawne AkshatJawne requested a review from mattrunyon June 4, 2024 17:56
Comment thread packages/app-utils/package.json Outdated
Comment thread packages/app-utils/src/components/ConnectionBootstrap.tsx Outdated
Comment thread packages/app-utils/src/components/ConnectionBootstrap.tsx Outdated
@AkshatJawne AkshatJawne requested a review from mattrunyon June 5, 2024 13:32
Comment thread packages/app-utils/src/components/ConnectionBootstrap.tsx
Comment thread packages/app-utils/src/components/ConnectionBootstrap.tsx Outdated
Comment thread packages/app-utils/src/components/AppBootstrap.test.tsx Outdated
Comment thread packages/app-utils/src/components/ConnectionBootstrap.tsx Outdated
Comment thread packages/code-studio/src/main/AppMainContainer.tsx Outdated
@AkshatJawne AkshatJawne requested a review from mattrunyon June 6, 2024 13:19
@AkshatJawne
Copy link
Copy Markdown
Contributor Author

Changed test case based on new logic, until the connection is resolved, it will show the modal, and hence for the first assertion of the connection bootstrap, should not be null.

Comment thread packages/app-utils/src/components/ConnectionBootstrap.tsx Outdated
@AkshatJawne AkshatJawne requested a review from mattrunyon June 6, 2024 18:49
Copy link
Copy Markdown
Collaborator

@mattrunyon mattrunyon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will still need @mofojed to review again. I want to make sure he's ok w/ the state names as well.

Changes for the states could be not_connecting -> errored. And failed -> authFailed instead. I'm indifferent though

@mattrunyon mattrunyon requested a review from mofojed June 6, 2024 21:34
@AkshatJawne AkshatJawne merged commit 3e52242 into deephaven:main Jun 12, 2024
@github-actions github-actions Bot locked and limited conversation to collaborators Jun 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Handle reconnect auth fail in the embed-widget app

3 participants