-
Notifications
You must be signed in to change notification settings - Fork 32
fix: Handle segments ProviderError #3852
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3852 +/- ##
=======================================
Coverage 98.72% 98.72%
=======================================
Files 827 827
Lines 15006 15031 +25
Branches 4293 4297 +4
=======================================
+ Hits 14815 14840 +25
Misses 184 184
Partials 7 7
Continue to review full report in Codecov by Sentry.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #3852 +/- ##
=======================================
Coverage 98.72% 98.72%
=======================================
Files 827 827
Lines 15006 15031 +25
Branches 4293 4297 +4
=======================================
+ Hits 14815 14840 +25
Misses 184 184
Partials 7 7
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #3852 +/- ##
=======================================
Coverage 98.72% 98.72%
=======================================
Files 827 827
Lines 15006 15031 +25
Branches 4285 4297 +12
=======================================
+ Hits 14815 14840 +25
Misses 184 184
Partials 7 7
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #3852 +/- ##
=======================================
Coverage 98.72% 98.72%
=======================================
Files 827 827
Lines 15006 15031 +25
Branches 4285 4297 +12
=======================================
+ Hits 14815 14840 +25
Misses 184 184
Partials 7 7
Continue to review full report in Codecov by Sentry.
|
acf7d29
to
6731c18
Compare
Bundle ReportChanges will increase total bundle size by 4.71kB (0.04%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: gazebo-production-systemAssets Changed:
Files in
Files in
view changes for bundle: gazebo-production-esmAssets Changed:
Files in
Files in
|
✅ Sentry found no issues in your recent changes ✅ |
Bundle ReportChanges will increase total bundle size by 4.71kB (0.04%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: gazebo-staging-systemAssets Changed:
Files in
Files in
view changes for bundle: gazebo-staging-esmAssets Changed:
Files in
Files in
|
✅ Deploy preview for gazebo ready!Previews expire after 1 month automatically.
|
6731c18
to
d89167c
Compare
return <DiffRenderer impactedFile={comparisonData.impactedFile} path={path} /> | ||
// since above we've handled when segments is of one of the other union types, | ||
// we can safely assert that it's of type SegmentComparisons now | ||
const impactedFile = comparisonData.impactedFile as ImpactedFileType |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need to assert this? TS should automatically narrow it. If it's not doing that, a case may be missed in the conditional 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm yeah I think the problem was that the type narrowing only applied to the top-level variable, but didn't carry through to the nested property access. I wasn't able to get it to infer the narrowed type in that deeper context, so did that uglier workaround.
I pulled out the value into a separate variable, and that seems to have allowed TypeScript to apply the narrowing correctly, so we can go with this version!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's goooo
return <DiffRenderer impactedFile={comparisonData.impactedFile} path={path} /> | ||
// since above we've handled when segments is of one of the other union types, | ||
// we can safely assert that it's of type SegmentComparisons now | ||
const impactedFile = comparisonData.impactedFile as ImpactedFileType |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same thing down here, TS narrowing should make it so we don't need to cast types
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks yeah same as above ^
!comparisonData?.impactedFile || | ||
!path || | ||
hadErrorFetchingFileFromProvider | ||
) { | ||
return <ErrorDisplayMessage /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we able to display a more helpful error message for these new errors that we're detecting? To better inform the user what's going on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah sure good call. The existing one was appropriate for the ProviderError and I added one for the UnknownPath error
Handle when segments returns
ProviderError
orUnknownPath
here.Closes codecov/engineering-team#3485

