Add Retry button to "Something went wrong" error states - #96955
Conversation
Co-authored-by: Lucien Akchoté <lakchote@users.noreply.github.com>
|
Neither failing check is caused by this PR's code — no code fix is needed.
Evidence
|
|
There's no code fix to push here — both red checks are non-code failures, confirmed again just now:
|
linhvovan29546
left a comment
linhvovan29546
left a comment
There was a problem hiding this comment.
@MelvinBot Please check the request changes
| buttonTranslationKey={isInvalidQuery ? undefined : 'common.tryAgain'} | ||
| onButtonPress={ | ||
| isInvalidQuery | ||
| ? undefined | ||
| : () => | ||
| handleSearch({ | ||
| queryJSON, | ||
| searchKey: currentSearchKey, | ||
| offset, | ||
| shouldCalculateTotals, | ||
| prevReportsLength: filteredDataLength, | ||
| isLoading: !!searchResults?.search?.isLoading, | ||
| }) | ||
| } |
There was a problem hiding this comment.
isInvalidQuery is checked twice for two sibling props. Could be simplified via a spread ({...(!isInvalidQuery && {buttonTranslationKey: 'common.tryAgain', onButtonPress: retryHandler })}) to reduce duplication
There was a problem hiding this comment.
Done — collapsed the two isInvalidQuery checks into a single conditional spread so the button props are set together only when the error is retryable: src/components/Search/index.tsx:946.
| <Button | ||
| style={styles.mt4} | ||
| onPress={onButtonPress} | ||
| sentryLabel="BlockingViewRetryButton" |
There was a problem hiding this comment.
Please add sentryLabel to CONST, like the other SENTRY_LABEL constants RETRY_BUTTON
There was a problem hiding this comment.
Done — added SENTRY_LABEL.BLOCKING_VIEW.RETRY_BUTTON to CONST (src/CONST/index.ts:8395) and referenced it in the button (src/components/BlockingViews/BlockingView.tsx:204) instead of the inline string.
…exit 143) Co-authored-by: Linh Vo <linhvovan29546@users.noreply.github.com>
|
The only substantive failing check was Root cause: The job ended with The other two red checks — PR Author Checklist and PR Reviewer Checklist — are human-completed checklists, not code checks, so I left those alone. What I did: Since no code fix was warranted, I pushed an empty commit to re-trigger CI (the direct job re-run wasn't available to me here). The test workflow should now run clean. |
|
All automated checks are green or currently re-running — there's no code fix needed.
To clear the last check: please complete the PR Author Checklist items in the PR description (check off the boxes and fill in the Tests/QA sections). Nothing else needs a code change. |
|
✅ The real failure is resolved. After I re-triggered CI, The only check still showing red is No code changes were needed for the check failure. |
…y props via spread Co-authored-by: Linh Vo <linhvovan29546@users.noreply.github.com>
Co-authored-by: Linh Vo <linhvovan29546@users.noreply.github.com>
|
The re-trigger cleared the earlier Failing check: This is unrelated to this PR, which only touches The only other red check, PR Author Checklist, is a human-completed checklist, not a code check. What I did: Pushed an empty commit to re-trigger CI. If |
…to claude-retryButtonErrorState Co-authored-by: Linh Vo <linhvovan29546@users.noreply.github.com>
…to claude-retryButtonErrorState Co-authored-by: Linh Vo <linhvovan29546@users.noreply.github.com>
|
Addressed both review comments and pushed to the branch:
Checks run
I did not run the full |
|
MelvinBot has made 6 consecutive commits trying to fix CI failures. Automatic CI fix dispatches are now paused for this PR. Next steps: A human should review the recent commits and resolve the conflicting CI errors manually. |
1 similar comment
|
MelvinBot has made 6 consecutive commits trying to fix CI failures. Automatic CI fix dispatches are now paused for this PR. Next steps: A human should review the recent commits and resolve the conflicting CI errors manually. |
|
Both requested changes are addressed and pushed (
|
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
|
@MelvinBot I think we should add bottom padding to the |
Co-authored-by: Linh Vo <linhvovan29546@users.noreply.github.com>
|
Good catch — added bottom padding to the error state so the Try again button no longer sits flush against the widget's bottom edge (
|
|
@MelvinBot Please update the test section and QA test to |
Reviewer Checklist
Screenshots/VideosAndroid: HybridApptelegram-cloud-document-5-6280350773645680724.mp4telegram-cloud-document-5-6280350773645680728.mp4Android: mWeb Chrometelegram-cloud-document-5-6280350773645680727.mp4iOS: HybridAppScreen.Recording.2026-07-25.at.22.48.21.moviOS: mWeb Safari
MacOS: Chrome / SafariScreen.Recording.2026-07-25.at.21.59.19.movScreen.Recording.2026-07-25.at.21.46.42.mov |
|
Updated the Tests and QA Steps sections of the PR description with the steps you provided (kept the "no console errors" checkbox on each). Take a look and let me know if anything needs tweaking. |
|
@MelvinBot Please complete the PR Author checklist, just like I did in my review #96955 (comment) |
|
Looks pretty good to me. cc @Expensify/design for eyes as well |
|
Looking good to me too. Can we double check the spacing though? It looks a little spread out to me compared to our other empty states. This is our standard spacing for these: |
|
@MelvinBot Please address this comment #96955 (comment) |
Drive the retry button's spacing from the container gap instead of stacking a hardcoded mt4 on top of it (which produced a 36px gap in the Spend over time widget), and apply the widget's gap5 + mt0/mb2 spacing to FullPageErrorView so both error surfaces match the standard 20/8/20 (icon/title/subtitle/button) empty-state spacing. Co-authored-by: Linh Vo <linhvovan29546@users.noreply.github.com>
|
Good catch — the button was doubling up its spacing. Fixed in eee5c92 so both error surfaces now use the standard 20 / 8 / 20 (icon → title → subtitle → button). What was off
Changes:
Could you re-check the spacing when you get a chance? I wasn't able to run it locally, so a quick visual confirm would be great. |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eee5c92a94
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| {...(!isInvalidQuery && { | ||
| buttonTranslationKey: 'common.tryAgain', |
There was a problem hiding this comment.
When a malformed search is submitted through searchInServer or opened directly, useSearchPageSetup fires the request without updating searchRequestResponseStatusCode; that state therefore remains null (or can retain the status from a previous query). Consequently isInvalidQuery is false and this spread displays a Retry button that only repeats the invalid request—or, with a stale invalid status, hides Retry for a later retriable failure. Persist or propagate the response code from the page-level request and associate it with the current query before deciding whether to render the button.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — this was a real gap. Fixed in 6ecc256.
Root cause: searchRequestResponseStatusCode was only maintained on the user-driven re-search path (handleSearchAction). The page-level fire in useSearchPageSetup.ts:76 — which runs on initial mount / deep link / query change — fired search() without capturing the resolved jsonCode, and the state was never reset when the query changed. So exactly the two failure modes you flagged:
- An invalid query opened directly left the status at
null→isInvalidQueryfalse → a pointless Try again was shown that just repeats the invalid request. - A stale
INVALID_SEARCH_QUERYfrom a previous query hid Try again on a later retriable failure.
(The dedup in search() made this worse: whichever fire path won the in-flight dedup returned the promise, so the component-level .then was frequently skipped entirely — Search.ts:960-963.)
Fix: made useSearchPageSetup the single source of truth for the response code, so it's tied to the current query regardless of which path fires the request:
- The page-level fire now captures the
jsonCode:useSearchPageSetup.ts:92-94. - The code resets on query (
hash) change, so a staleINVALID_SEARCH_QUERYcan't leak into the next query:useSearchPageSetup.ts:44-51(adjusted during render rather than in an effect, per thereact-hooks/set-state-in-effectrule). - Both
SearchPage(wide) andSearchPageNarrownow consume this one value/setter instead of keeping their own separateuseState, so the user-driven re-search feeds the same source of truth:SearchPage.tsx:51,SearchPageNarrow/index.tsx:161-171.
Checks run
npm run fmt(oxfmt) — appliednpm run typecheck-tsgo— no new errors in the changed files (thereact-native-nitro-fetch/ResultMetadataerrors are pre-existing and unrelated)npm run lint-changed— 0 errors on the changed files (the one remaining warning is the pre-existing grandfatheredsetIsSortinginSearchPage.tsx)npm run react-compiler-compliance-check checkon all three files — passes (exit 0)
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.
|
@dannymcclain Does this work for you? 
|
|
Yeah that looks good, thanks! |
Co-authored-by: {"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/comments#get-an-issue-comment","status":"404"} <{"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/comments#get-an-issue-comment","status":"404"}@users.noreply.github.com>
|
@MelvinBot Please resolve conflicts |
…orState Co-authored-by: Linh Vo <linhvovan29546@users.noreply.github.com> # Conflicts: # src/hooks/useSearchPageSetup.ts # src/pages/Search/SearchPage.tsx # src/pages/Search/SearchPageNarrow/index.tsx # src/pages/home/SpendOverTimeSection/useSpendOverTimeData.ts
|
🤖 Resolved the merge conflicts and pushed the merge commit ( Four files conflicted, all in the Search area. Here's how each was resolved:
Verified after resolving: no conflict markers remain, Please double-check the resolutions above — especially dropping |
|
Good catch, Danny! |
|
@MelvinBot Please resolve the spell check https://github.com/Expensify/App/actions/runs/30282117282/job/90030839005?pr=96955 |
Co-authored-by: Linh Vo <linhvovan29546@users.noreply.github.com>
|
🤖 Fixed the failing spellcheck. CSpell flagged one unknown word — |
|
@lakchote This should be ready for your review. |
lakchote
left a comment
lakchote
left a comment
There was a problem hiding this comment.
Left some suggestions @linhvovan29546 please accept them
|
|
||
| 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 |
There was a problem hiding this comment.
Put an empty line before the comment unless it’s on the first line of a block.
| // Retrying a failed page always resets pagination to the first page, so totals eligibility | |
| // Retrying a failed page always resets pagination to the first page, so totals eligibility |
There was a problem hiding this comment.
Done in 2f3ee38 — added the blank line before the comment.
| 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. |
There was a problem hiding this comment.
| // Retrying an invalid query won't help, so the retry button is only offered for other errors. | |
| // Retrying an invalid query won't help, so the retry button is only offered for other errors. |
There was a problem hiding this comment.
Couldn't apply this one — the blank line sits between two JSX attributes (subtitle= and the comment), and oxfmt strips blank lines inside a JSX attribute list. I added it and re-ran npm run fmt, which removed it again, so committing it would just fail the Oxfmt CI check. The comment is left directly under the subtitle prop without the leading blank line.
| queryJSON?: SearchQueryJSON; | ||
| searchResults?: SearchResults; | ||
| isMobileSelectionModeEnabled: boolean; | ||
| /** jsonCode of the current query's most recent SEARCH response (owned by useSearchPageSetup in the parent). */ |
There was a problem hiding this comment.
Comment is useless, variable is self-explanatory
| /** jsonCode of the current query's most recent SEARCH response (owned by useSearchPageSetup in the parent). */ |
There was a problem hiding this comment.
Done in 2f3ee38 — removed the comment.
| isMobileSelectionModeEnabled: boolean; | ||
| /** jsonCode of the current query's most recent SEARCH response (owned by useSearchPageSetup in the parent). */ | ||
| searchRequestResponseStatusCode: number | null; | ||
| /** Setter for the response status code, so user-driven re-searches update the same source of truth. */ |
There was a problem hiding this comment.
Comment is useless
| /** Setter for the response status code, so user-driven re-searches update the same source of truth. */ |
There was a problem hiding this comment.
Done in 2f3ee38 — removed the comment.
|
@MelvinBot Please address the review suggestions #96955 (review) |
Co-authored-by: Linh Vo <linhvovan29546@users.noreply.github.com>
|
🤖
|
Done |
|
@linhvovan29546 next time please accept my suggestions directly, so we both get credit for it. |
|
🚧 lakchote has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
7 participants
Explanation of Change
The "Something went wrong" error state was a dead end:
BlockingView(and itsFullPageErrorViewwrapper) only supported an icon, title, subtitle and an optional inline link, so a failed request left the user stranded with no way to re-attempt it.This adds an optional CTA button to
BlockingViewvia two new props (buttonTranslationKey+onButtonPress), rendered withButtonComposedonly when both are provided so every existing consumer is unchanged.FullPageErrorViewthreads the same two props through. A Retry button (reusing the existingcommon.tryAgainlabel) is then wired into the two error surfaces:searchcall already fired on mount inonConfigChanged(auseEffectEvent, so it can't be returned from the hook) is extracted into a plainretryfunction that keeps the existing!queryJSON || isSearchLoading || isOfflineguard.onConfigChangednow just callsretry(), and the hook returnsretryfor theERRORBlockingView.handleSearch, which resetssearchRequestResponseStatusCodetonullbefore re-firing the search. The button is intentionally hidden forINVALID_SEARCH_QUERYerrors (both props leftundefined), since retrying an invalid query won't help.Fixed Issues
$ #96380
PROPOSAL: #96380 (comment)
Tests
Onyx.merge('network', {shouldFailAllRequests: true}).Onyx.merge('network', {shouldFailAllRequests: false}), then tap Try again.Onyx.merge('network', {shouldFailAllRequests: true})again, open the Search page (any saved search).Onyx.merge('network', {shouldFailAllRequests: false}), tap Try again.Offline tests
QA Steps
Onyx.merge('network', {shouldFailAllRequests: true})or enableSimulate failing network requestsvia Test Preferences.Onyx.merge('network', {shouldFailAllRequests: false}), then tap Try again.Onyx.merge('network', {shouldFailAllRequests: true})again, open the Search page (any saved search).Onyx.merge('network', {shouldFailAllRequests: false}), tap Try again.PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videosundefined