Skip to content

fix: display error when fetching OAuth2 provider apps #11713

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

Merged
merged 2 commits into from
Jan 22, 2024

Conversation

code-asher
Copy link
Member

This was pulled out of #11609. I am trying to break out parts of that PR so we can review and merge it more easily.

@code-asher code-asher changed the title Display error when fetching OAuth2 provider apps fix: display error when fetching OAuth2 provider apps Jan 19, 2024
@code-asher code-asher requested a review from Parkreiner January 19, 2024 20:48
@code-asher code-asher marked this pull request as ready for review January 19, 2024 20:48
Copy link
Member

@Parkreiner Parkreiner left a comment

Choose a reason for hiding this comment

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

Looks good! Just had one question about how the error was getting passed in

@@ -28,12 +29,14 @@ type OAuth2AppsSettingsProps = {
apps?: TypesGen.OAuth2ProviderApp[];
isEntitled: boolean;
isLoading: boolean;
error?: unknown;
Copy link
Member

@Parkreiner Parkreiner Jan 22, 2024

Choose a reason for hiding this comment

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

Should this be optional? Feels like making a dev always pass a value in (even if it's hopefully undefined 99% of the time) would help make sure we don't forget our errors as the code gets updated over time

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, I just assumed it could be undefined from React Query but I did not actually check. 😛

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah I see, it can be null but not undefined. Fixing.

@code-asher code-asher merged commit 7589df3 into main Jan 22, 2024
@code-asher code-asher deleted the asher/display-oauth2-app/error branch January 22, 2024 18:56
@github-actions github-actions bot locked and limited conversation to collaborators Jan 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants