Reports - Fallback WS avatar shown when clearing the "From" filter on "Submit" while offline by mkzie2 · Pull Request #69869 · Expensify/App

mkzie2 · 2025-09-04T14:10:29Z

Explanation of Change

Fixed Issues

$ #68821
PROPOSAL: #68821 (comment)

Tests

Prerequisite: Account has at least one workspace created.
Prerequisite 2: Delayed submissions enabled.

Open the ND.
Open any workspace chat.
Create two manual expenses.
Navigate to "Reports"
Turn off internet connection.
Tap on the "From" filter.
Tap on the checkmark of the selected user, to deselect it.
Tap on "Apply"
Verify that After clearing the "From" filter on "Submit" section while offline, the user should be redirected to the new search results (Offline screen)

Verify that no errors appear in the JS console

Offline tests

QA Steps

Prerequisite: Account has at least one workspace created.
Prerequisite 2: Delayed submissions enabled.

Open the ND.
Open any workspace chat.
Create two manual expenses.
Navigate to "Reports"
Turn off internet connection.
Tap on the "From" filter.
Tap on the checkmark of the selected user, to deselect it.
Tap on "Apply"

Expected Result:
After clearing the "From" filter on "Submit" section while offline, the user should be redirected to the new search results.

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Android: Native

android.mov

Android: mWeb Chrome

android-web.mov

iOS: Native

ios.mov

iOS: mWeb Safari

ios-web.mov

MacOS: Chrome / Safari

desktop.mov

MacOS: Desktop

web.mov

melvin-bot · 2025-09-04T14:42:51Z

@brunovjk Please 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]

trjExpensify

Looking to address this fallback avatar appearing makes sense in principle, but I'm not sure about how this is being achieved by removing lastNonEmptySearchResults.current. @JS00001 @luacmartins @s77rt - does this seem kosher to you?

s77rt · 2025-09-05T04:20:10Z

No clear idea. I'm more concerned about the existence of lastNonEmptySearchResults.current and what was it being used for? Is it safe to remove?

trjExpensify · 2025-09-05T11:33:16Z

Not sure, maybe came from #65421? CC: @mountiny @SzymczakJ

JS00001 · 2025-09-05T11:36:27Z

🤔 I'm a bit confused by this offline implementation. It looks like currently, if you're offline and select a search that you don't have cached, we show the results of old search, rather than a 'youre offline' message. @trjExpensify Is this the expected behavior when offline?

SzymczakJ · 2025-09-05T12:10:05Z

Not sure, maybe came from #65421? CC: @mountiny @SzymczakJ

The lastNonEmptySearchResults were there before, I was only refactoring to cut unwanted rerenders and keep the behaviour the same.

In fact I found the PR that introduced this concept, this is how it is explained here:

// save last non-empty search results to avoid ugly flash of loading screen when hash changes and onyx returns empty data

So to conclude, I think in offline mode we still show "you're offline" message and if we want to remove this lastNonEmptySearchResults we need to make sure that the flash doesn't happen anymore @mkzie2

JS00001 · 2025-09-05T12:12:50Z

Lets get Tom's opinion, but I think we should be showing the 'youre offline' screen for the 'fix' to this issue. I dont think we should be fixing the avatar, but rather, not letting the user get into that state at all

trjExpensify · 2025-09-05T13:22:42Z

It looks like currently, if you're offline and select a search that you don't have cached, we show the results of old search, rather than a 'youre offline' message. @trjExpensify Is this the expected behavior when offline?

Yep, I agree with that. 👍 Though, do we see this avatar bug on a cached search result as well potentially?

JS00001 · 2025-09-05T13:27:44Z

We shouldnt, there was a PR to fix that thats on staging now: #69335

trjExpensify · 2025-09-05T13:35:33Z

Ah okay, great. 👌

mkzie2 · 2025-09-05T15:28:52Z

The reason the avatar is displayed as fallback is that the hash from the SearchContext is the hash of the current query that doesn't have snapshot data, then personalDetails data is empty here.

@JS00001 I think the solution from #69335 is just a workaround. The RCA is the searchResults is the result of the previous query, while the hash is the value of the current query. Then any component on the SearchPage that uses the data from useOnyx can cause this problem, here is another example

JS00001 · 2025-09-05T15:58:05Z

Can you explain the RCA a bit more?

mkzie2 · 2025-09-05T16:38:03Z

@JS00001 The useOnyx hook uses the currentSearchHash from SearchContext to get the data from the snapshot. When we change the query from A to B, currentSearchHash is the hash of the query B, while the searchResults is the data of the query A. Because the data from query B is pending from the backend, useOnyx returns undefined on the Search page.

App/src/pages/Search/SearchPage.tsx

Line 780 in f286e03

queryJSON={queryJSON}

App/src/components/Search/index.tsx

Line 289 in f286e03

setCurrentSearchHashAndKey(hash, searchKey);

App/src/hooks/useOnyx.ts

Line 50 in f286e03

const {isOnSearch, currentSearchHash} = useSearchContext();

App/src/pages/Search/SearchPage.tsx

Line 633 in f286e03

    
           const searchResults = currentSearchResults?.data ? currentSearchResults : lastNonEmptySearchResults.current;

JS00001 · 2025-09-05T16:45:15Z

@mkzie2 I agree with all of that, my question though is what issues does that cause, and how? Once the issue has been defined, how would you fix it, and why would that fix work?

mkzie2 · 2025-09-05T17:18:45Z

my question though is what issues does that cause, and how? Once the issue has been defined, how would you fix it, and why would that fix work?

@JS00001 That is the reason for the fallback avatar bug; the search item is the result of the previous data, but the child component gets the data of the current query. In our case, it's personalDetails data

App/src/components/AvatarWithDisplayName.tsx

Line 160 in 96e0102

    
           const [personalDetails] = useOnyx(ONYXKEYS.PERSONAL_DETAILS_LIST, {canBeMissing: false}) ?? CONST.EMPTY_OBJECT;

App/src/components/ReportSearchHeader/index.tsx

Lines 7 to 20 in 96e0102

    
           function ReportSearchHeader({report, style, transactions, avatarBorderColor}: ReportSearchHeaderProps) { 
        
               const styles = useThemeStyles(); 
        
               const middleContent = useMemo(() => { 
        
                   return ( 
        
                       <AvatarWithDisplayName 
        
                           report={report} 
        
                           transactions={transactions} 
        
                           shouldUseCustomSearchTitleName 
        
                           shouldEnableDetailPageNavigation={false} 
        
                           shouldEnableAvatarNavigation={false} 
        
                           avatarBorderColor={avatarBorderColor} 
        
                       /> 
        
                   );

Or another example is that the violation can be empty at this time.

App/src/components/TransactionItemRow/TransactionItemRowRBRWithOnyx.tsx

Line 35 in 96e0102

    
           const transactionViolations = useTransactionViolations(transaction?.transactionID, false);

mkzie2 · 2025-09-05T17:20:19Z

how would you fix it, and why would that fix work?

My suggestion is to remove lastNonEmptySearchResults and always display the search list with the current query data. That ensures the SearchList and useOnyx always get the same snapshot data. Of course, its limitation is that the search page will show loading every time a new query has never gotten data.

JS00001 · 2025-09-05T17:39:31Z

That sounds okay to me. I agree w/ @s77rt , what was lastsearchresult being used for, whats the idea behind it and what happens if we remove it?

mkzie2 · 2025-09-12T03:12:09Z

Still finding the solution.

mkzie2 · 2025-09-16T21:53:12Z

lastNonEmptySearchResults is added to prevent the flash when sorting, I updated the PR to keep this behavior and still fix the current issue.

brunovjk · 2025-09-17T22:31:19Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified that the composer does not automatically focus or open the keyboard on mobile unless explicitly intended. This includes checking that returning the app from the background does not unexpectedly open the keyboard.
I verified tests pass on all platforms & I tested again on:
- Android: HybridApp
- Android: mWeb Chrome
- iOS: HybridApp
- iOS: mWeb Safari
- MacOS: Chrome / Safari
- MacOS: Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified there are no new alerts related to the canBeMissing param for useOnyx
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.ts or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
- I verified that all the inputs inside a form are aligned with each other.
- I added Design label and/or tagged @Expensify/design so the design team can review the changes.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
For any bug fix or new feature in this PR, I verified that sufficient unit tests are included to prevent regressions in this flow.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Android: HybridApp

69869_android_native.mov

Android: mWeb Chrome

69869_android_web.mov

iOS: HybridApp

69869_ios_native.mov

iOS: mWeb Safari

69869_ios_web.mov

MacOS: Chrome / Safari

69869_web_chrome.mov

MacOS: Desktop

69869_web_desktop.mov

brunovjk

LGTM

src/pages/Search/SearchPage.tsx

JS00001

Can you address #69869 (comment) and then we can merge?

mkzie2 · 2025-09-22T19:23:17Z

@JS00001 @carlosmiceli I updated.

OSBotify · 2025-09-22T21:20:52Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2025-09-23T19:58:27Z

🚀 Deployed to staging by https://github.com/JS00001 in version: 9.2.17-0 🚀

platform	result
🖥 desktop 🖥	success ✅
🕸 web 🕸	success ✅
🤖 android 🤖	success ✅
🍎 iOS 🍎	success ✅

OSBotify · 2025-09-24T22:48:20Z

🚀 Deployed to production by https://github.com/roryabraham in version: 9.2.17-6 🚀

platform	result
🖥 desktop 🖥	success ✅
🕸 web 🕸	success ✅
🤖 android 🤖	success ✅
🍎 iOS 🍎	success ✅

Fallback WS avatar issue

cc9a9c8

melvin-bot bot assigned mkzie2 Sep 4, 2025

mkzie2 marked this pull request as ready for review September 4, 2025 14:42

mkzie2 requested review from a team as code owners September 4, 2025 14:42

melvin-bot bot requested review from brunovjk and removed request for a team September 4, 2025 14:42

mkzie2 mentioned this pull request Sep 4, 2025

[Awaiting checklist] [$250] Reports - Fallback WS avatar shown when clearing the "From" filter on "Submit" while offline #68821

Closed

8 tasks

trjExpensify reviewed Sep 4, 2025

View reviewed changes

mkzie2 added 2 commits September 17, 2025 04:17

Merge branch 'main' into mkzie2-issue/68821

997ad96

add new solution for the sorting case

71d3274

add onSortPressedCallback prop

4a0f0a0

brunovjk approved these changes Sep 18, 2025

View reviewed changes

melvin-bot bot requested a review from carlosmiceli September 18, 2025 00:39

carlosmiceli reviewed Sep 18, 2025

View reviewed changes

src/pages/Search/SearchPage.tsx Outdated Show resolved Hide resolved

JS00001 self-requested a review September 18, 2025 10:57

JS00001 requested changes Sep 18, 2025

View reviewed changes

src/pages/Search/SearchPage.tsx Show resolved Hide resolved

JS00001 previously approved these changes Sep 22, 2025

View reviewed changes

carlosmiceli previously approved these changes Sep 22, 2025

View reviewed changes

mkzie2 added 2 commits September 23, 2025 02:12

Merge branch 'main' into mkzie2-issue/68821

0980c67

remove eslint

3dc581d

mkzie2 dismissed stale reviews from carlosmiceli and JS00001 via 3dc581d September 22, 2025 19:15

carlosmiceli approved these changes Sep 22, 2025

View reviewed changes

JS00001 approved these changes Sep 22, 2025

View reviewed changes

JS00001 merged commit 15e90b6 into Expensify:main Sep 22, 2025
23 checks passed

OSBotify mentioned this pull request Sep 23, 2025

Deploy Checklist: New Expensify 2025-09-23 #71102

Closed

84 tasks

Conversation

mkzie2 commented Sep 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Explanation of Change

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

Uh oh!

melvin-bot bot commented Sep 4, 2025

Uh oh!

trjExpensify left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

s77rt commented Sep 5, 2025

Uh oh!

trjExpensify commented Sep 5, 2025

Uh oh!

JS00001 commented Sep 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SzymczakJ commented Sep 5, 2025

Uh oh!

JS00001 commented Sep 5, 2025

Uh oh!

trjExpensify commented Sep 5, 2025

Uh oh!

JS00001 commented Sep 5, 2025

Uh oh!

trjExpensify commented Sep 5, 2025

Uh oh!

mkzie2 commented Sep 5, 2025

Uh oh!

JS00001 commented Sep 5, 2025

Uh oh!

mkzie2 commented Sep 5, 2025

Uh oh!

JS00001 commented Sep 5, 2025

Uh oh!

mkzie2 commented Sep 5, 2025

Uh oh!

mkzie2 commented Sep 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JS00001 commented Sep 5, 2025

Uh oh!

mkzie2 commented Sep 12, 2025

Uh oh!

mkzie2 commented Sep 16, 2025

Uh oh!

brunovjk commented Sep 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviewer Checklist

Screenshots/Videos

Uh oh!

brunovjk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

JS00001 left a comment

Choose a reason for hiding this comment

Uh oh!

mkzie2 commented Sep 22, 2025

Uh oh!

Uh oh!

OSBotify commented Sep 22, 2025

Uh oh!

OSBotify commented Sep 23, 2025

Uh oh!

OSBotify commented Sep 24, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

mkzie2 commented Sep 4, 2025 •

edited

Loading

trjExpensify left a comment •

edited

Loading

JS00001 commented Sep 5, 2025 •

edited

Loading

mkzie2 commented Sep 5, 2025 •

edited

Loading

brunovjk commented Sep 17, 2025 •

edited

Loading