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

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

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

AkshatJawne
Copy link
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 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
packages/app-utils/src/components/ConnectionBootstrap.tsx Outdated Show resolved Hide resolved
packages/app-utils/src/components/ConnectionBootstrap.tsx Outdated Show resolved Hide resolved
packages/app-utils/src/components/ConnectionBootstrap.tsx Outdated Show resolved Hide resolved
packages/app-utils/src/components/ConnectionBootstrap.tsx Outdated Show resolved Hide resolved
packages/embed-widget/src/index.scss Outdated Show resolved Hide resolved
packages/app-utils/src/components/ConnectionBootstrap.tsx Outdated Show resolved Hide resolved
packages/app-utils/src/components/ConnectionBootstrap.tsx Outdated Show resolved Hide resolved
Copy link
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.

packages/embed-widget/src/index.tsx Outdated Show resolved Hide resolved
packages/app-utils/src/components/ConnectionBootstrap.tsx Outdated Show resolved Hide resolved
@AkshatJawne
Copy link
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".

packages/app-utils/package.json Outdated Show resolved Hide resolved
@AkshatJawne
Copy link
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.

Copy link
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
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.

Handle reconnect auth fail in the embed-widget app
3 participants