Add Retry button to "Something went wrong" error states #96955
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -182,6 +182,10 @@ function Search({ | |||||||
| const searchDataType = useMemo(() => (shouldUseLiveData ? CONST.SEARCH.DATA_TYPES.EXPENSE_REPORT : searchResults?.search?.type), [shouldUseLiveData, searchResults?.search?.type]); | ||||||||
| const shouldCalculateTotals = useSearchShouldCalculateTotals(currentSearchKey, hash, offset === 0, areAllMatchingItemsSelected); | ||||||||
|
|
||||||||
| // Retrying a failed page always resets pagination to the first page, so totals eligibility | ||||||||
|
Contributor
There was a problem hiding this comment. Put an empty line before the comment unless it’s on the first line of a block.
Suggested change
Contributor
Author
There was a problem hiding this comment. Done in |
||||||||
| // must be evaluated as if we're on the first page rather than the (possibly paginated) offset. | ||||||||
| const shouldCalculateTotalsOnRetry = useSearchShouldCalculateTotals(currentSearchKey, hash, true, areAllMatchingItemsSelected); | ||||||||
|
|
||||||||
| const previousReportActions = usePrevious(reportActions); | ||||||||
| const {translate} = useLocalize(); | ||||||||
| const searchListRef = useRef<SelectionListHandle<SearchListItem> | null>(null); | ||||||||
|
|
@@ -942,6 +946,24 @@ function Search({ | |||||||
| isBreakLine: shouldUseNarrowLayout, | ||||||||
| })} | ||||||||
| subtitle={translate(isInvalidQuery ? 'errorPage.wrongTypeSubtitle' : 'errorPage.subtitle')} | ||||||||
| // Retrying an invalid query won't help, so the retry button is only offered for other errors. | ||||||||
|
Contributor
There was a problem hiding this comment.
Suggested change
Contributor
Author
There was a problem hiding this comment. Couldn't apply this one — the blank line sits between two JSX attributes ( |
||||||||
| {...(!isInvalidQuery && { | ||||||||
| buttonTranslationKey: 'common.tryAgain', | ||||||||
|
Comment on lines
+950
to
+951
There was a problem hiding this comment.
When a malformed search is submitted through Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment. Good catch — this was a real gap. Fixed in Root cause:
(The dedup in Fix: made
Checks run
I wasn't able to run the invalid-query / failing-network flow locally, so a quick functional confirm of both scenarios (invalid query → no Retry; retriable failure after a prior invalid query → Retry shown) would be great. |
||||||||
| onButtonPress: () => { | ||||||||
| // A failed load-more clears the whole snapshot (data: null), so retrying with the | ||||||||
| // paginated offset would refetch only the later page into an empty snapshot and drop | ||||||||
| // the initial results. Reset pagination to the first page before retrying. | ||||||||
| setOffset(0); | ||||||||
| handleSearch({ | ||||||||
| queryJSON, | ||||||||
| searchKey: currentSearchKey, | ||||||||
| offset: 0, | ||||||||
| shouldCalculateTotals: shouldCalculateTotalsOnRetry, | ||||||||
| prevReportsLength: filteredDataLength, | ||||||||
| isLoading: !!searchResults?.search?.isLoading, | ||||||||
| }); | ||||||||
| }, | ||||||||
| })} | ||||||||
| /> | ||||||||
| </View> | ||||||||
| ); | ||||||||
|
|
||||||||
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.
❌ CLEAN-REACT-PATTERNS-1 (docs)
This adds two new optional content props (
buttonTranslationKey,onButtonPress) toBlockingViewwhose sole purpose is to conditionally render a UI element. Both props are used only inside this{!!onButtonPress && !!buttonTranslationKey && (<Button .../>)}block — nothing else. Per Case 2, an optional prop that exists only to gate a{!!prop && <Element />}render should be a composable child instead. Every future addition (e.g. a second action, an icon on the button) would again require expandingBlockingView's prop interface and internals, increasing coupling and regression risk.Prefer composition so
BlockingViewnever has to change when a consumer needs a CTA. For example, expose the button as a child the consumer opts into rather than a prop pair:This keeps
BlockingViewstable and lets each consumer decide whether (and what) to render below the subtitle without threadingbuttonTranslationKey/onButtonPressthroughFullPageErrorViewas pure pass-throughs.Reviewed at: bca3e94 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
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.
It's a legitimate, well-reasoned style point, but not a correctness bug — non-blocking for this PR.
Consistent with existing precedent.
ButtonComposed(imported in this very file) explicitly documents itself as replacing "a large flat props list" with children composition, so the critique aligns with the direction the codebase is already heading, not just abstract preference.But adopting it here is a bigger lift than it looks:
BlockingViewneeds a realchildrenslot — it has none today. The return is a fixedScrollView→ icon/animation → title/subtitle → button, with no insertion point.BlockingView.Actionsub-component would need to be built.FullPageErrorViewwould need to forwardchildreninstead of the two pass-through props (buttonTranslationKey,onButtonPress).SpendOverTimeSectionContent.tsx,Search/index.tsx) would need to switch call syntax.Scope risk:
BlockingView/FullPageErrorVieware used broadly across the app (dozens of consumers). Widening its public contract is the kind of change worth doing deliberately — not as a rider on an issue explicitly scoped as a "lightweight interim improvement," where a more holistic approach was called out as intentionally out of scope.