This repository has been archived by the owner on Feb 8, 2024. It is now read-only.
desktop playback error handling #638
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 3 commits
March 1, 2022 22:40
…g re-created on each re-render
… ended with an error
ibeckermayer
requested review from
ravicious,
hatched,
zmb3,
gzdunek,
kimlisa and
rudream
zmb3
reviewed
Mar 2, 2022
kimlisa
reviewed
Mar 3, 2022
| } = useDesktopPlayer({ | ||
| sid, | ||
| clusterId, | ||
| }); | ||
|
|
||
| const displayCanvas = attempt.status === 'success' || attempt.status === ''; | ||
| const displayProgressBar = attempt.status !== 'processing'; |
There was a problem hiding this comment.
should the progress bar be visible on error? also wondering if the progress bar can still be moved around on error and if that would cause errors
There was a problem hiding this comment.
I decided that for now it should be, since sometimes the error is actually part of the playback (i.e. if the session itself ended with a tdp error). Currently moving the progress bar doesn't actually change anything, so it isn't a problem. When it does become active, it will need to be accounted for.
ibeckermayer
requested review from
kimlisa and
zmb3
and removed request for
ravicious,
hatched,
gzdunek and
rudream
zmb3
approved these changes
Mar 3, 2022
ravicious
approved these changes
Mar 3, 2022
ibeckermayer
pushed a commit
that referenced
this pull request
Mar 7, 2022
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds UI support for error messages during desktop session playback. There are three cases where we want to display an error message:
1. There is a backend error during playback
In this case, I've crafted a new json message that may be sent back by the playback player (i.e. here) of the form
{ "message": "error", "errorText": "some error text" }which is handled by the
PlayerClienthere and here.2. A TDP error happened during the session that's being played
Since I needed to create this error display regardless, I figured I may as well add support for displaying TDP error messages if the session ended with one. This was relatively simple to slot in, since the tdp
ClientwhichPlayerClientinherits already knows how to handle them.To do this, I told our server to record errors and then defined the relevant
TdpClientCanvaspropto handle them.network error
If the websocket disconnects for some unknown reason, we just display this error
closes #574
closes #575
corresponds with gravitational/teleport#10765