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

Update youtube-rewriter error handling to pass captioning errors as ChatCraft message #625

Merged
merged 2 commits into from
May 1, 2024

Conversation

rjwignar
Copy link
Collaborator

@rjwignar rjwignar commented May 1, 2024

This fixes #605

Background

If you use /import with a Youtube video that either has no English captions or no captions at all, ChatCraft will throw a non-descriptive error toast:
image

Caption extraction for YouTube videos is handled by youtube-captions-scraper within functions/lib/rewriters/youtube-rewriter.ts.

A previous PR for this (#618) would extract unhandled error messages from the HTML of a generated CloudFlare Pages page and put it inside an Error Toast.

Fix

In this PR, any errors thrown in youtube-rewriter will show up in the /import Response (as a ChatCraft message):
image

@rjwignar rjwignar requested a review from humphd May 1, 2024 21:52
@rjwignar rjwignar self-assigned this May 1, 2024
Copy link
Collaborator

@humphd humphd left a comment

Choose a reason for hiding this comment

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

Can we remove the other fix you made before, or does it still have value?

});
} catch (error) {
return new Response(`**Error**: ${error?.message}`, {
headers: { "Content-Type": "text/markdown; charset=UTF-8" },
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is probably OK, but I've never seen Markdown used as an Error body before.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wanted to bold it like the "Command" text, but I hadn't considered whether we should include Markdown in an Error body.
I can remove the Markdown for now

Copy link
Collaborator

Choose a reason for hiding this comment

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

Usually an error is informational, not stylistic. You don't want to assume too much about how the client will use it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I recently removed Markdown styling from the error message:

return new Response(`Error: ${error?.message}`, {

@rjwignar
Copy link
Collaborator Author

rjwignar commented May 1, 2024

Can we remove the other fix you made before, or does it still have value?

I had both fixes in one branch and found that only this fix would be triggered if an error occured (there'd be a ChatCraft message but no Error Toast).
We can remove the other fix.

Copy link

cloudflare-workers-and-pages bot commented May 1, 2024

Deploying chatcraft-org with  Cloudflare Pages  Cloudflare Pages

Latest commit: 48951a4
Status: ✅  Deploy successful!
Preview URL: https://62f568e2.console-overthinker-dev.pages.dev
Branch Preview URL: https://issue-605-2.console-overthinker-dev.pages.dev

View logs

@rjwignar rjwignar merged commit 0ce7e11 into main May 1, 2024
4 checks passed
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.

Errors when using /import on YouTube videos with no captions
2 participants