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

Draft
wants to merge 6 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.

@robintown
Copy link
Member Author

@hughns The new error messages seem okay, and I certainly wouldn't mind them being in the product. But seeing as you were interested in establishing a general strategy for the error messages we'll be writing in the future, I want to try to articulate what I was happy with about the original copy, that I now miss. (with my design hat on)

In terms of the portion that's immediately visible (without clicking "show details"), I liked that the original copy makes it clear that the application at least understood what the user's intent was (connecting to the call) and was trying to fulfill that request. In other words, that the error didn't come out of nowhere. If there are any error states for which this is relevant, it's going to be errors that are presented as a full-screen page, since they're taken out of any visual context that could otherwise relate them to the current task.

It also isn't super important to do this, I mean, though the transition is jarring users are going to remember what they were trying to get the application to do. But as long as we're here giving error states some extra care, and it's trivial, why not design errors to be aware of the task that generated them / that they're disrupting? This is what my inclination to avoid templates is about—errors should, ideally, be free to show some awareness of their context.

Concerning the error details, I think the way you've now got EC analyzing more of the possible failure modes in detail is nice. What I miss is that it's no longer providing targeted advice to admins in the form of pointing them to lk-jwt-service, which, given the difficulties DIY homeserver admins have had in setting up our app, felt appropriate to include. That was the other reason I saw for not trying to generalize the errors.

In short: I see the table we created yesterday as an excellent way to organize our work on errors and ensure that we're suggesting the appropriate actions to the user+admin based on what category the error falls into, but my suggestion is that we apply it less literally to the designs, allowing the errors within each category to show more context-awareness (where that's actually worth the effort). Hopefully this clarifies how I was failing to see why we should throw out the original strings, and how I would like to approach the errors I write for future tasks—if it makes sense to you?

@robintown
Copy link
Member Author

Being more explicit about (for example) "this is a configuration issue" in the URL building case makes a lot of sense to me. I see where you were going there.

@hughns
Copy link
Member

hughns commented Jan 22, 2025

FTR, I've continued this conversation "out of band".

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