[Issue #3152] properly error on client for max search term length #3554
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
doug-s-nava
changed the title
doug-s-nava
requested review from
acouch,
andycochran and
btabaska
as code owners
doug-s-nava
force-pushed
the
dschrashun/3152-client-max-search-length
branch
from
e206b3f to
a2d503d
Compare
acouch
reviewed
Jan 21, 2025
| const ErrorAlert = | ||
| parsedErrorData.details && parsedErrorData.type === "ValidationError" ? ( | ||
| <Alert type="error" heading={t("validationError")} headingLevel="h4"> | ||
| {`Error in ${parsedErrorData.details.field || "a search field"}: ${parsedErrorData.details.message || "adjust your search and try again"}`} |
There was a problem hiding this comment.
Could we do a follow-up to translate this, ie the "adjust your search and try again"?
There was a problem hiding this comment.
I can make a ticket, but I'm not exactly sure what the scope would need to be. To get this to work as a fully translated UI we'd need to handle translating stuff that's coming from the API, including error messages and our field names. That seems to open up a lot of questions, and since it doesn't seem like we're prioritizing translation right now I don't know when the right time for that conversation is
There was a problem hiding this comment.
I meant the a search field part.
acouch
reviewed
Jan 21, 2025
frontend/src/errors.ts
Outdated
| */ | ||
|
|
||
| import { QueryParamData } from "src/types/search/searchRequestTypes"; | ||
|
|
||
| export interface FrontendErrorDetails { |
There was a problem hiding this comment.
Is there a reason not to store this in src/types/... ?
…earch validation error
doug-s-nava
force-pushed
the
dschrashun/3152-client-max-search-length
branch
from
5960572 to
3d0dc4e
Compare
acouch
approved these changes
Jan 22, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #3152
Time to review: 20 mins
Changes proposed
Main goal here is to show a helpful error message with a relevant call to action to users when they enter a search term longer than the API allows (100 characters).
To do that this change:
This change also fixes a bug that was preventing the search form state to be maintained in the case of an error.
I also observed when doing this work that Next seems to treat the errors thrown by the search fetcher to be unhandled, even though they redirect to the search error page. This means the error toast thing shows up on dev, and the error is logged to the console as unhandled. Is this expected?
Context for reviewers
Test steps for client side validation
npm run devTest steps for error response handling
Totally synthetic but the easiest way to do this is to disable the client side check.
Additional information
Server side error
Client side validation error