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
#6277 - Handle google login errors in Google sheet picker widget #6532
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #6532 +/- ##
==========================================
- Coverage 70.03% 70.02% -0.02%
==========================================
Files 1175 1175
Lines 36589 36613 +24
Branches 6902 6907 +5
==========================================
+ Hits 25625 25638 +13
- Misses 10964 10975 +11
☔ View full report in Codecov by Sentry. |
setErrorAnnotation(null); | ||
return schemaResult; | ||
} catch (error: unknown) { | ||
console.error(error); |
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.
Was this left in intentionally?
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.
The idea here is that I'm catching all errors that stem from the getSpreadsheets()
call (and surrounding logic, technically), not just the specific google sign-in errors. I figured that this was an ok trade-off to make right now, because, while it might make certain situations slightly harder for the user to understand what's happening, there is an obvious and immediate CTA to retry the request and recover in the UX. The downside here is that something like a "no internet connection" issue, if it happened right when the user tried to authenticate with the PKCE integration, would also produce the exact same error message and it might not totally be clear to the user what's happening. But I think this is a better trade-off than showing the user a stack trace (error boundary) in this situation and forcing them to reload the page editor.
The upshot of all this is that I left the console.error in here on purpose so that at least the real error is reported to the console for debugging purposes if we run into issues with non-login errors in this code.
What does this PR do?
Demo
https://www.loom.com/share/310a7839fd904b78ad093eb97a5acbc8
Checklist
src/tsconfig.strictNullChecks.json
(if possible)