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 context to error logged when anchoring fails #5639

Merged
merged 1 commit into from
Jul 19, 2023

Conversation

robertknight
Copy link
Member

If the transcript fails to load in the video player app, it will reject the contentReady promise that the client waits on. The client would then cause the error to be thrown from Guest.anchor. Catch this error and add context to indicate what has gone wrong.

The upshot of this is that if the transcript fails to load in the video player, anyone looking at the browser console will see a more meaningful message.

Before:

Anchoring error before

After:

Anchoring error

@codecov
Copy link

codecov bot commented Jul 19, 2023

Codecov Report

Merging #5639 (d0a8842) into main (57fa3b2) will decrease coverage by 0.02%.
The diff coverage is 88.88%.

❗ Current head d0a8842 differs from pull request most recent head aed61e5. Consider uploading reports for the commit aed61e5 to get more accurate results

@@            Coverage Diff             @@
##             main    #5639      +/-   ##
==========================================
- Coverage   99.43%   99.42%   -0.02%     
==========================================
  Files         240      240              
  Lines        9429     9436       +7     
  Branches     2236     2237       +1     
==========================================
+ Hits         9376     9382       +6     
- Misses         53       54       +1     
Impacted Files Coverage Δ
src/annotator/config/index.ts 100.00% <ø> (ø)
src/annotator/guest.ts 99.67% <88.88%> (-0.33%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@acelaya acelaya left a comment

Choose a reason for hiding this comment

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

Other than the missing test spotted by Codecov, looks good to me.

Base automatically changed from content-ready-promise to main July 19, 2023 13:34
If the transcript fails to load in the video player app, it will reject the
`contentReady` promise that the client waits on. The client would then cause the
error to be thrown from `Guest.anchor`. Catch this error and add context to
indicate what has gone wrong.
@robertknight
Copy link
Member Author

Other than the missing test spotted by Codecov, looks good to me.

As that branch is a trival log statement in a catch handler I have marked it as ignored for now. We do have coverage of the main body of that function.

@robertknight robertknight merged commit 1bfb577 into main Jul 19, 2023
2 checks passed
@robertknight robertknight deleted the content-load-error-context branch July 19, 2023 14:00
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