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

Add error screens for connecting to the JWT service #2952

Open
wants to merge 3 commits into
base: livekit
Choose a base branch
from

Conversation

robintown
Copy link
Member

@robintown robintown commented Jan 16, 2025

Based on #2951 diff robintown/element-call@error-screens...jwt-error-screens

The error screens look like this, leading with a minimally jargon-y message, then with a 'show details' button to put homeserver admins on the right track:

Screenshot 2025-01-15 at 22-23-11 Element Call The Testing Zone
Screenshot 2025-01-15 at 22-27-02 Element Call The Testing Zone
Screenshot 2025-01-15 at 22-22-10 Element Call The Testing Zone

(Ignore the "status code 200" :D I just flipped the condition to get the screenshot.)

@hughns
Copy link
Member

hughns commented Jan 16, 2025

@fkwp please can you review the wording with your Product hat on? is it too verbose?

Copy link
Member

@hughns hughns left a comment

Choose a reason for hiding this comment

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

It would be great to see some tests for this.

Can we mock the fetch response and assert that the error is correctly mapped?

Quick brain down of errors to mock:

  • DNS resolution failure
  • Connection timeout
  • 500 server response
  • 404 server response
  • JWT service rejects with non-okay response
  • successful response doesn't throw error

@robintown
Copy link
Member Author

@fkwp please can you review the wording with your Product hat on? is it too verbose?

Note that the 'verbose' parts are only shown once you press 'show details'.

Copy link
Member

@hughns hughns left a comment

Choose a reason for hiding this comment

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

Quick one: The PR needs a rebase/merge from upstream.

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.

2 participants