Fixed held expense disappearing after approving pending expense from Reports

[LorenzoBloedow]

Explanation of Change

Make sure search is triggered when approving an IOU with a held expense.

Fixed Issues

$ #57605
PROPOSAL: #57605 (comment)

Tests

Pretty much same as QA:

1. Go to staging.new.expensify.com
2. Create a new workspace.
3. Go to workspace chat.
4. Submit two manual expenses to the workspace chat.
5. Click Submit button.
6. Click on the expense preview.
7. Hold one of the expenses.
8. Go to Reports > Expense reports.
9. Click Review.
10. Click Approve.
11. Approve the pending amount (green button).
12. Close the RHP.
13. Verify the held expense appears as a separate report after approving the pending amount.
14. Refresh the page.
15. Verify the held expense still appears as a separate report.

• Verify that no errors appear in the JS console

Offline tests

Same as tests

QA Steps

Same as tests

• Verify that no errors appear in the JS console

PR Author Checklist

• I linked the correct issue in the ### Fixed Issues section above
• I wrote clear testing steps that cover the changes made in this PR
  • I added steps for local testing in the Tests section
  • I added steps for the expected offline behavior in the Offline steps section
  • I added steps for Staging and/or Production testing in the QA steps section
  • I added steps to cover 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 tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
• I included screenshots or videos for tests on all platforms
• I ran the tests on all platforms & verified they passed on:
  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop
• I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
• I followed proper code patterns (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
    • If any non-english text was added/modified, I used JaimeGPT to get English > Spanish translation. I then posted it in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
  • 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 followed the guidelines as stated in the Review Guidelines
• I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
• 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
• I verified that if a function's arguments changed that all usages have also been updated correctly
• 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.
• I added unit tests for any new feature or bug fix in this PR to help automatically prevent regressions in this user 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.

Screenshots/Videos

Android: Native

Screen.Recording.2025-03-05.at.5.41.54.PM.mov

Android: mWeb Chrome

Screen.Recording.2025-03-05.at.5.24.58.PM.mov

iOS: Native

Screen.Recording.2025-03-05.at.5.54.29.PM.mov

iOS: mWeb Safari

Screen.Recording.2025-03-05.at.5.04.27.PM.mov

MacOS: Chrome / Safari

Screen.Recording.2025-03-05.at.4.53.02.PM.mov

MacOS: Desktop

Screen.Recording.2025-03-05.at.5.58.18.PM.mov

Unrelated errors:

PS: It seems there's an unrelated race condition on mobile where if you click "Submit" too fast the search isn't triggered, this should probably be a new issue.
Fixed with cfa0ed3.

[melvin-bot]

@parasharrajat 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]

[github-actions]

All contributors have signed the CLA ✍️ ✅
_{Posted by the CLA Assistant Lite bot.}

[LorenzoBloedow]

I have read the CLA Document and I hereby sign the CLA

[parasharrajat]

Screenshots

🔲 iOS / native

🔲 iOS / Safari

🔲 MacOS / Desktop

🔲 MacOS / Chrome

🔲 Android / Chrome

🔲 Android / native

[parasharrajat]

I am currently looking at the performance impact of this solution, and it seems that it might not be the best solution as our reports can change very often in the background. So if a user is looking at transactions and a new message arrives at the back, we do not want to rerun the search. This is just unnecessary.

We might have to optimize this @LorenzoBloedow? Unfortunately, I overlooked this during the proposal review.

[LorenzoBloedow]

We might have to optimize this @LorenzoBloedow? Unfortunately, I overlooked this during the proposal review.

@parasharrajat No worries, just pushed a commit that constrained the if statement to the previous behavior or an IOU change.
Let me know if it looks good or if I should attempt to optimize this further.

[parasharrajat]

I didn't understand this. What are we trying to accomplish here exactly?

[LorenzoBloedow]

When we pay the expense (that may contain a held expense), an IOU event is triggered and added to the end of the array.
We still need to trigger the search when this happens in order to fix the bug, but now we're limiting the amount of times that search is triggered.

[parasharrajat]

But it's not the proper way bcz there can be other IOU on in the actions.

[LorenzoBloedow]

You're right, that wasn't the best solution.
Updated the code to only trigger when the search when the user actually clicks the pay button.
Is this better?

[LorenzoBloedow]

@parasharrajat Can you please review the new changes addressing the performance concerns? Thanks :)

[parasharrajat]

I wil review this asap.

[parasharrajat]

Looks like we have completely detoured from the original solution. I am will take a look at the issue again and see what should be done here. I wan't much active last week.

[LorenzoBloedow]

Looks like we have completely detoured from the original solution.

I don't think there's much that can be done to optimize the original solution but I can take a look at it again if we should stick to it.

Performance-wise, I think the current solution is the best though as it only triggers the search once.

[parasharrajat]

Thanks for waiting. I am back on it. I will for sure update this PR tomorrow morning (12 hours from now.).

[LorenzoBloedow]

@parasharrajat Could you please take a look at this PR when you have a moment?
I'm making sure to follow the contributor guidelines so I can't do more work until this PR is resolved =P

[parasharrajat]

Thanks for the reminder. Apologies for this delay, I will try to get some time for this.

I am not 100% up with your solution that's why I can't directly jump to the review part. First we need to finalized the solution.

[LorenzoBloedow]

Thanks for the reminder. Apologies for this delay, I will try to get some time for this.

No worries =)

I am not 100% up with your solution that's why I can't directly jump to the review part. First we need to finalized the solution.

Do you mean you don't understand it? If so, here's a brief explanation:

Given your concerns about the original solution’s performance, the latest solution does the following:

1. Pass the entire query string used for search down to ReportScreen.
2. Create a triggerSearch function that basically just calls SearchActions.search with the query string passed to ReportScreen.
3. Pass the triggerSearch function down to the modal that pops up when you try to approve an IOU with a held expense.
4. Use the modal's onSubmit to trigger the search only once after the IOU is approved.

This solution fixes both the performance problem and a previous race condition where if you click "Submit" too fast the search isn't triggered.

I have to admit I'm a little confused on what I should do so we can move this forward 😅, is there anything you'd like me to change about this solution?

Thanks for your patience =)

[parasharrajat]

I will probably review this EOD and try to unblock. Thanks for waiting.

[parasharrajat]

Looking it now.

[parasharrajat]

I don't think we should trigger the search from the report page. Technically, there should not be any need to do so as we can use Onyx keys to check and trigger search.

Also, we should keep the search functionality within its scope.

[parasharrajat]

Based on the recent changes, I can that the our initial solution was already implemented in a different PR. I am currently validating that solution if that is accepted, we don't need to make any change here. This issue will be considered done.

[LorenzoBloedow]

Based on the recent changes, I can that the our initial solution was already implemented in a different PR. I am currently validating that solution if that is accepted, we don't need to make any change here. This issue will be considered done.

Let me know how that goes, should we continue here, I'll make sure to switch to the original solution and find a way to fix the race condition and multiple requests from the original solution as that's why we switched to the current one.

[LorenzoBloedow]

I was taking a look at the recent changes, and though the core issue might be fixed, we could use this PR to improve it, because, as you mentioned here:

it might not be the best solution as our reports can change very often in the background

If it still has the same performance issues, maybe it's not the best solution.
Also, the original solution had a unit test, so that's another reason we could continue this PR too.

[parasharrajat]

That works for me. I am waiting for the discussion on that solution to conclude. Once finalized, I will let you know.

[parasharrajat]

So I have not got any objections on that solution so far. Let's see if we can improve the original solution here and wrap this one.

[parasharrajat]

Yes, please migrate only the files that you have modified originally in the source. Leave others.

1. src/hooks/useSearchHighlightAndScroll.ts
2. tests/unit/useSearchHighlightAndScrollTest.ts

[LorenzoBloedow]

@blimpich Sorry about that, reading back, I probably should've been clearer how I was proposing fixing all linter errors specifically from main.

Reverted the changes and migrated only the necessary files, everything should be passing now :)

[blimpich]

LGTM. Linter is still complaining about files that you didn't touch, but that should be fine to merge still I think.

[melvin-bot]

@blimpich looks like this was merged without a test passing. Please add a note explaining why this was done and remove the Emergency label if this is not an emergency.

[blimpich]

Not an emergency, silly linter was complaining about files that we didn't touch in this PR.

[OSBotify]

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

[LorenzoBloedow]

Not an emergency, silly linter was complaining about files that we didn't touch in this PR.

Interesting, might have to do with the state of the git tree before reverting.
Glad It's ok though :)

Now that my first PR has been merged, according to the contributing guidelines, I'm assuming it's ok for me to potentially work on multiple PRs at the same time, or do I have to wait for something else? @blimpich

Also, thanks for all the help everyone!

[blimpich]

Yup! All good to work on as many issues as you want! Nexts steps here are:

1. this PR will be deployed to our staging server
2. our QA teams will test the PR using the QA steps (should pass since we didn't add any production code changes)
3. once all PRs for the deploy are QA'ed and any issues with the deploy are resolved we'll deploy the changes to production
4. there will be a regression period (I think this is 5 days? I forget) that we'll wait before paying the amount. Again this won't really be a concern here because it's not really possible for this PR to cause a regression.
5. Payment for this PR will be handled by @Christinadobrzyn after the regression period is done. Feel free to post any questions on the issue itself or post to the #expensify-open-source slack channel

Thank you for contributing! 🙂

[nlemma]

@LorenzoBloedow  This PR is failing because of issue #62042

Bug6831312_1747245700219.bandicam_2025-05-14_19-50-48-199.mp4

[parasharrajat]

That is unrelated to this PR @nlemma

[nlemma]

I might be missing something, but is step 13 expected to behave this way?

[parasharrajat]

I didn't notice the steps on this PR due to time lag. They might be outdated as a lot has changed since this PR was created. We didn't change any code in this PR, so this PR can't cause that issue. Please confirm internally whether step 13 is expected or not. Don't rely on these steps.

[parasharrajat]

@nlemma Do you think #62042 should be covered in #57605? Or they are the same. If so, we can work on that as part of this issue.

[nlemma]

It looks like both issues have the same underlying problem. If you agree, this could be tracked and be fixed as part of #57605

[LorenzoBloedow]

They don't seem to have the same root problem, but they are indeed similar, if needed I can work on it, just need some help on how to proceed in regards to where the work should be done since this PR was already merged.

[parasharrajat]

@LorenzoBloedow In this case, you can create another PR to solve that.

[LorenzoBloedow]

@parasharrajat Looks like the issue comes from a recent PR in staging, I asked whether we should revert it or open a new PR here.

[parasharrajat]

Thanks for digging that up. I agree with you reopening that issue