Show error summary from API when transcript fails to load #1030

robertknight · 2023-06-30T16:59:42Z

The API returns JSON:API error objects with code, title and details fields. Show the title to the user instead of a generic "API call failed" message.

Part of #1028

Testing:

Go to http://localhost:9083/https://www.youtube.com/watch?v=e-43x-H4CNY
You should see the error message below:

Note that the error title and contents of the "Error details" box need a bunch of work on the backend to refine. For example subtitles are not so much "disabled" for this video as "not available" because the video doesn't have spoken words. This morning I also encountered the same error message for a video where the transcript just hadn't been generated. In the frontend we are just displaying what the backend sends.

The API returns JSON:API error objects with `code`, `title` and `details` fields. Show the `title` to the user instead of a generic "API call failed" message. Part of #1028

robertknight · 2023-06-30T17:01:06Z

via/static/scripts/video_player/components/test/TranscriptError-test.js

+    );
+    assert.equal(
+      wrapper.text(),
+      ['Unable to load transcript', 'Something went wrong'].join('')


[...].join('') is used here to separate out parts that are on different lines in the UI, but that don't have actual spaces between them.

robertknight · 2023-06-30T17:02:48Z

via/static/scripts/video_player/utils/api.ts

+      Array.isArray(resultData.errors) &&
+      resultData.errors.length > 0
+    ) {
+      error = resultData.errors[0];


The JSON:API format allows for multiple errors, but we currently only report the first one on the frontend. I'm not aware of a scenario where we'd expect the backend to report multiple yet.

robertknight · 2023-06-30T17:05:42Z

via/static/scripts/video_player/components/TranscriptError.tsx

+          <p>{error.error.detail}</p>
+        </details>
+      )}
+    </div>


Styling is obvious not final here.

Yep, don't worry about that for now. I've got it.

lyzadanger · 2023-07-05T12:37:37Z

@robertknight Can you help me understand the goal of the "details" box once it's more functional?

robertknight · 2023-07-05T12:43:00Z

@robertknight Can you help me understand the goal of the "details" box once it's more functional?

I was thinking it would work like the "Details" box in LMS error messages. It could include any technical information that we think might be useful for debugging or other purposes. In the LMS context, the support/success team for example will often open this if they are on a call with a user and inspect / make note of what is in there.

lyzadanger · 2023-07-05T12:48:55Z

I was thinking it would work like the "Details" box in LMS error messages. It could include any technical information that we think might be useful for debugging or other purposes. In the LMS context, the support/success team for example will often open this if they are on a call with a user and inspect / make note of what is in there.

Are you fairly convinced that it adds value? I know you want it to be tweaked, but right now it reads:

Could not retrieve a transcript for the video https://www.youtube.com/watch?v=e-43x-H4CNY! This is most likely caused by: Subtitles are disabled for this video If you are sure that the described cause is not responsible for this error and that a transcript should be retrievable, please create an issue at https://github.com/jdepoix/youtube-transcript-api/issues. Please add which version of youtube_transcript_api you are using and provide the information needed to replicate the error. Also make sure that there are no open issues which already describe your problem!

AFAICT that doesn't add any information or use beyond what the concise error message says. Can you give me an example of what it might say in future that would be useful?

Edited: A plausible path forward could be to omit the <details> element until it has something useful to say. Looks like it's easy to drop in?

robertknight · 2023-07-05T13:09:16Z

AFAICT that doesn't add any information or use beyond what the concise error message says. Can you give me an example of what it might say in future that would be useful?

There is an open issue at #1044. What I am expecting as a result of resolving that issue is that the backend would omit the detail field for this particular error, and so the <details> field would not be shown.

We don't currently have a case where we have extra information to show. I would expect the backend to simply omit the detail field whenever it has nothing to add.

lyzadanger · 2023-07-05T13:11:29Z

There is an open issue at #1044. What I am expecting as a result of resolving that issue is that the backend would omit the detail field for this particular error, and so the <details> field would not be shown.

So, maybe we could punt on the <details> element for now until we have a clear need for it?

robertknight · 2023-07-05T13:27:34Z

So, maybe we could punt on the
element for now until we have a clear need for it?

OK, I'll do that.

We have decided to remove this for the moment as we don't have a good example where it provides value. We'll re-add it when we do. See #1030 (comment)

robertknight · 2023-07-05T13:33:36Z

Done! - The <details> is now gone.

lyzadanger

Thanks for your patience! I'm working on collating some error-styling needs into a single ticket to track, so, I got that part.

lyzadanger · 2023-07-05T13:38:27Z

via/static/scripts/video_player/components/TranscriptError.tsx

+          <p>{error.error.detail}</p>
+        </details>
+      )}
+    </div>


Yep, don't worry about that for now. I've got it.

lyzadanger · 2023-07-05T13:39:21Z

via/static/scripts/video_player/components/test/TranscriptError-test.js

+import TranscriptError from '../TranscriptError';
+
+describe('TranscriptError', () => {
+  it('displays just `Error.message` if there are no error details', () => {


Does this test description need adjustment in light of the details removal?

This is still valid as "error details" includes the title. This case is for when we don't have a server-generated error at all, or it doesn't include even a title.

lyzadanger · 2023-07-05T13:51:13Z

via/static/scripts/video_player/components/VideoPlayerApp.tsx

-              <div data-testid="transcript-error">
-                Unable to load transcript: {transcript.message}
-              </div>
+              <TranscriptError error={transcript} />


I think it's neat that the same reference can accommodate either an actual transcript or an error (and am of the opinion that typing makes this possible versus a bug waiting to happen). But I admit I did a double-take at error={transcript} here.

Yeah, I might rename this to transcriptOrError in future.

lyzadanger · 2023-07-05T13:53:02Z

via/static/scripts/video_player/utils/test/api-test.js

+    it('throws exception if result is not JSON', async () => {
+      fakeFetch
+        .withArgs(videoPlayerConfig.api.transcript.url)
+        .resolves(new Response('<b>Oh no</b>', { status: 500 }));


Just confirming that we intend to invent a 500 here in these cases :). Granted this is not new behavior in these changes, so feel free to ignore me.

Yes, this is mean to simulate something like eg. a 5xx "Bad Gateway" error if the server failed to respond within the various timeouts that the proxies in front have.

Show error summary from API when transcript fails to load

6a539dc

The API returns JSON:API error objects with `code`, `title` and `details` fields. Show the `title` to the user instead of a generic "API call failed" message. Part of #1028

robertknight commented Jun 30, 2023

View reviewed changes

robertknight requested a review from lyzadanger June 30, 2023 17:03

robertknight commented Jun 30, 2023

View reviewed changes

Remove <details> element from errors until we have a use for it

2be935a

We have decided to remove this for the moment as we don't have a good example where it provides value. We'll re-add it when we do. See #1030 (comment)

lyzadanger approved these changes Jul 5, 2023

View reviewed changes

robertknight merged commit 917e3a1 into main Jul 5, 2023

robertknight deleted the api-errors branch July 5, 2023 14:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show error summary from API when transcript fails to load #1030

Show error summary from API when transcript fails to load #1030

robertknight commented Jun 30, 2023

robertknight Jun 30, 2023

robertknight Jun 30, 2023

robertknight Jun 30, 2023 •

edited

Loading

lyzadanger Jul 5, 2023

lyzadanger commented Jul 5, 2023

robertknight commented Jul 5, 2023

lyzadanger commented Jul 5, 2023 •

edited

Loading

robertknight commented Jul 5, 2023 •

edited

Loading

lyzadanger commented Jul 5, 2023

robertknight commented Jul 5, 2023

robertknight commented Jul 5, 2023

lyzadanger left a comment

lyzadanger Jul 5, 2023

lyzadanger Jul 5, 2023

robertknight Jul 5, 2023

lyzadanger Jul 5, 2023

robertknight Jul 5, 2023

lyzadanger Jul 5, 2023

robertknight Jul 5, 2023

Show error summary from API when transcript fails to load #1030

Show error summary from API when transcript fails to load #1030

Conversation

robertknight commented Jun 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertknight Jun 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lyzadanger commented Jul 5, 2023

robertknight commented Jul 5, 2023

lyzadanger commented Jul 5, 2023 • edited Loading

robertknight commented Jul 5, 2023 • edited Loading

lyzadanger commented Jul 5, 2023

robertknight commented Jul 5, 2023

robertknight commented Jul 5, 2023

lyzadanger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertknight Jun 30, 2023 •

edited

Loading

lyzadanger commented Jul 5, 2023 •

edited

Loading

robertknight commented Jul 5, 2023 •

edited

Loading