(thunderstore-api) Implement user-facing error handling improvements and update tests for authentication errors #1614
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
| queryParams: {}, | ||
| }) | ||
| ).rejects.toThrowError("401: Unauthorized"); | ||
| ).rejects.toThrowError("Authentication required. Please sign in."); |
There was a problem hiding this comment.
If the thrown error gives us clean access to the status code, it could be less brittle to check that instead of the message. Unless the message itself is the very behaviour we are interested in.
| const MAX_NB_RETRY = 5; | ||
| const RETRY_DELAY_MS = 200; | ||
|
|
||
| /** |
There was a problem hiding this comment.
Nitpick: I think the function name is pretty self explanatory and this comment is unnecessary.
| } | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
Nitpick: I think the function name is pretty self explanatory and this comment is unnecessary. Also "used by X" comments are a bit problematic as they tend to get easily outdated. I guess there might be some cases where they're warranted but this doesn't seem one of those to me.
| } | ||
|
|
||
| export type apiFetchArgs<B, QP> = { | ||
| /** |
There was a problem hiding this comment.
This comment is actively confusing me. It seems it would be more suited for apiFetchArgs than SchemaOrUndefined. And if it would be a comment for apiFetchArgs I would nitpick that it provides no value.
| type SchemaOrUndefined<Schema extends z.ZodSchema | undefined> = | ||
| Schema extends z.ZodSchema ? z.infer<Schema> : undefined; | ||
|
|
||
| export type apiFetchArgs< |
There was a problem hiding this comment.
I've noticed in the few last PRs that especially types seems to be exported even when they're used only locally. I would rather not export stuff just in case. It's easy to add export later if needed, removing it is more effort if it's used somewhere outside already. And I think it also increases the odds of IDEs autoimporting stuff from these files directly, instead of the root of the package where the public interface of the package should be defined.
There was a problem hiding this comment.
This export probably a remnant of where it was used in the fetch functions. Removed the export
| responseJson = undefined; | ||
| } | ||
|
|
||
| const serverMessage = responseJson |
There was a problem hiding this comment.
If I'm reading this correctly, extract message looks like it could handle things if we just did const serverMessage = extractMessage(responseJson ?? responseText);
| message: buildApiErrorMessage({ response, responseJson, context }), | ||
| response: response, | ||
| responseJson: responseJson, | ||
| responseText, |
There was a problem hiding this comment.
This object duplicates the data. It might be reasonable if we assume the error responses from the API are short, and having both the original data and the "ready-to-use" data available can improve DX. I'm just wondering does this cause issues e.g. if the API returns e.g. the whole Django 404 page (as I think it does if the API endpoint doesn't exists). Granted, at that point the error message isn't our biggest problem.
Just thinking out loud, doesn't require action at this point.
There was a problem hiding this comment.
Mm, I guess it could be handy if it's there, but I'm not sure to which extent? Definitely something that can be added if the need arises 👍
| } | ||
| } | ||
|
|
||
| function buildApiErrorMessage(args: { |
There was a problem hiding this comment.
Sidenote: the implementation on L177-L201 looks awful.
That being said I'd repeat the comment from the previous PR: this doing UI level stuff in a package that should focus on HTTP traffic. IMO error messages should be handled in cyberstorm-remix.
There was a problem hiding this comment.
As I see it the parts of the error message handling that should be in cyberstorm-remix are there and the stuff that shouldn't be there are in here. Mentioned this in another reply, but I'd be happy to refactor the implementation on a separate PR if we come to a consensus on the separation 👍
| } | ||
|
|
||
| function extractMessage(value: JSONValue | undefined): string | undefined { | ||
| if (value === undefined || value === null) return undefined; |
There was a problem hiding this comment.
Nitpick: Can value ever be null here? Or if we want to accept nulls, should the argument types be updated?
There was a problem hiding this comment.
JSONValue type can be null
| } | ||
| } | ||
|
|
||
| for (const entry of Object.values(objectValue)) { |
There was a problem hiding this comment.
Doesn't this have a decent chance of us showing just some random garbage to the user?
There was a problem hiding this comment.
The function has a very good chance of getting something out of the given input, but the where it's used, I'd say it's a very little risk of showing random garbage. Though en pistä päätä pantiks, it's been a long day so can't think straight anymore
Oksamies
changed the base branch from
11-19-implement_nimbus_error_handling_components_and_utilities_for_consistent_user-facing_error_messages
to
graphite-base/1614
Oksamies
force-pushed
the
graphite-base/1614
branch
from
2d3f2a3 to
e63509b
Compare
Oksamies
force-pushed
the
11-19-_thunderstore-api_implement_user-facing_error_handling_improvements_and_update_tests_for_authentication_errors
branch
from
635eec4 to
f78b11b
Compare
Oksamies
changed the base branch from
graphite-base/1614
to
11-19-implement_nimbus_error_handling_components_and_utilities_for_consistent_user-facing_error_messages
…and update tests for authentication errors
… where unnecessary for improved performance
Oksamies
changed the base branch from
11-19-implement_nimbus_error_handling_components_and_utilities_for_consistent_user-facing_error_messages
to
graphite-base/1614
Oksamies
force-pushed
the
graphite-base/1614
branch
from
e63509b to
e20d99e
Compare
Oksamies
force-pushed
the
11-19-_thunderstore-api_implement_user-facing_error_handling_improvements_and_update_tests_for_authentication_errors
branch
from
f78b11b to
13fecd4
Compare
Oksamies
changed the base branch from
graphite-base/1614
to
11-19-implement_nimbus_error_handling_components_and_utilities_for_consistent_user-facing_error_messages
(thunderstore-api) Implement user-facing error handling improvements and update tests for authentication errors
(thunderstore-api) Refactor API fetch functions to remove async/await where unnecessary for improved performance