Add Retry button to "Something went wrong" error states#96955
Add Retry button to "Something went wrong" error states#96955MelvinBot wants to merge 8 commits into
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) |
|
Most of the author checklist is a set of manual-testing attestations — "I ran the tests on all platforms & verified they passed on Android/iOS Native, mWeb, macOS", "I turned off my network connection and tested it offline", "I tested this PR with a High Traffic account against staging/production", "I included screenshots or videos for tests on all platforms". As the bot author I didn't run those manual device tests, so I can't honestly check those items. The What I can confirm about the change itself:
Since |
|
@MelvinBot Update the QA Steps to |
|
Updated the QA Steps section with the steps you provided. The rest of the PR body (including the Tests section) is unchanged. |
|
@MelvinBot Please mark PR ready for review |
|
@dubielzyk-expensify @linhvovan29546 One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
Done — marked this PR as ready for review. |
|
@dubielzyk-expensify This should be ready for you to review the design. Screen.Recording.2026-07-25.at.21.59.19.movScreen.Recording.2026-07-25.at.21.46.42.mov |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| </SubtitleWrapper> | ||
| )} | ||
| </View> | ||
| {!!onButtonPress && !!buttonTranslationKey && ( |
There was a problem hiding this comment.
❌ CLEAN-REACT-PATTERNS-1 (docs)
This adds two new optional content props (buttonTranslationKey, onButtonPress) to BlockingView whose 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 expanding BlockingView's prop interface and internals, increasing coupling and regression risk.
Prefer composition so BlockingView never 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:
// Consumer (e.g. FullPageErrorView / SpendOverTimeSectionContent) composes the action:
<BlockingView icon={...} title={...} subtitle={...}>
<BlockingView.Action onPress={retry}>{translate('common.tryAgain')}</BlockingView.Action>
</BlockingView>This keeps BlockingView stable and lets each consumer decide whether (and what) to render below the subtitle without threading buttonTranslationKey/onButtonPress through FullPageErrorView as 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.
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.- A new
BlockingView.Actionsub-component would need to be built. FullPageErrorViewwould need to forwardchildreninstead of the two pass-through props (buttonTranslationKey,onButtonPress).- Both consumers (
SpendOverTimeSectionContent.tsx,Search/index.tsx) would need to switch call syntax.
Scope risk: BlockingView/FullPageErrorView are 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.
![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: bca3e94250
ℹ️ 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".
| handleSearch({ | ||
| queryJSON, | ||
| searchKey: currentSearchKey, | ||
| offset, |
There was a problem hiding this comment.
When a load-more request fails, offset is already nonzero, while the failure update clears the entire snapshot with data: null in src/libs/actions/Search.ts:758. Retrying with that same offset therefore reloads only the later page into an empty snapshot, omitting the initial results; the component also retains the nonzero offset, so subsequent pagination can skip another page. Reset both the request and local pagination offset to zero when retrying this full-page error.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — addressed in 8979399.
The retry now resets pagination to the first page before re-firing the search: src/components/Search/index.tsx:951. Both the request offset and the local offset state (setOffset(0)) are reset to 0, so a retry after a failed load-more refetches from the start instead of loading only the later page into the snapshot that the failure cleared with data: null (src/libs/actions/Search.ts:758).
Since retrying always returns to the first page, I also evaluate totals eligibility as if we're on page 1 (shouldCalculateTotalsOnRetry) so a retry from a paginated error still requests totals like a fresh first-page load: src/components/Search/index.tsx:184.
…e search
Co-authored-by: Linh Vo <linhvovan29546@users.noreply.github.com>
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>
|
@dubielzyk-expensify @lakchote One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
3 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 stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari