fix transaction highlighting on SearchMoneyRequestReportPage #66801

SzymczakJ · 2025-07-21T14:59:29Z

Explanation of Change

Fixed Issues

$ #66799
PROPOSAL:

Tests

On account with at least one expense reports.
Clear cache and restart
go to Reports > Expense Reports
enter one of the reports
Make sure that old transactions are not highlighted
Add a transaction to report(with plus button) and make sure it is highlighted.
Enter and empty report and add few unreported expenses, then make sure they are highlighted.

Offline tests

QA Steps

Same as tests.

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 verified there are no new alerts related to the canBeMissing param for useOnyx
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

Android: mWeb Chrome

iOS: Native

iOS: mWeb Safari

MacOS: Chrome / Safari

Screen.Recording.2025-07-21.at.16.19.09.mov

MacOS: Desktop

melvin-bot · 2025-07-21T14:59:34Z

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

mountiny · 2025-07-21T15:57:23Z

@SzymczakJ Since this regression came again, can you tihnk of automated way to cover this logic?

SzymczakJ · 2025-07-22T05:31:47Z

I think it's impossible to test logic for this particular fix. The bug consisted of two things:

parent of a component holding useNewTransactions was unmounting and then mounting again, which caused the hook to not skip the first rerender. The bug was present because we simply used useNewTransactions in the wrong place
the useEffect using requestAnimationFrame was run before the onyx could successfully apply the changes. Now with the new setTimeout implementation, the tests already cover this case.

In other words, the existing tests cover everything that can be reasonably tested

mountiny · 2025-07-22T17:00:03Z

Should we close this one now?

SzymczakJ · 2025-07-23T13:12:49Z

I just managed to reproduce this bug, so not closing 😄

Pujan92 · 2025-07-24T17:06:00Z

src/hooks/useNewTransactions.ts

-        });
+        setTimeout(() => {
+            skipFirstTransactionsChange.current = false;
+        }, 1000);


Do we really need setTimeout here? Isn't requestAnimationFrame enough here?

We should avoid using set timeout

Yeah this just seems like a workaround

Both setTimeout and requestAnimationFrame + new Promise are workarounds for Onyx which is not merging results of OpenReport API call in one render, but in a couple of consecutive renders. This Onyx behaviour makes so, that loading state is set to false in one render and data is set in the next render(or even later), which can cause bugs if we don't wait for it.
We have no guarantee that the requestAnimationFrame + new Promise will for 100% ensure that the Onyx command will be fully merged.
On the other hand setTimeout option is much safer, just because it waits for a longer period of time.
So in conclusion, they both are a nasty workarounds, but if we don't want this bug to come back in the future(because Onyx behaviour changes a bit), we should stick with the setTimeout.
WDYT @mountiny @Pujan92

@SzymczakJ Can you think of ways that are not workarounds? or redesign this logic so we do not have to rely on workaround like this

Thanks, I'm passing this to the team to investigate.

If you watch report and reportMetadata objects you can spot that reportMetadata is set as loaded at least one render, before the object is hydrated.

@SzymczakJ I've read through all of the information you provided, and I'm not sure I understand what you mean.

In the SearchMoneyRequestReportPage the report and reportMetadata are two different values which are stored under two different onyx keys:
report -> ${ONYXKEYS.COLLECTION.REPORT}${reportIDFromRoute}
reportMetadata -> ${ONYXKEYS.COLLECTION.REPORT_METADATA}${reportIDFromRoute}

App/src/pages/Search/SearchMoneyRequestReportPage.tsx

Lines 44 to 45 in 6ace187

const [report] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${reportIDFromRoute}`, {allowStaleData: true, canBeMissing: true});

const [reportMetadata = defaultReportMetadata] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_METADATA}${reportIDFromRoute}`, {canBeMissing: true, allowStaleData: true});

They are loading independently. Besides that the reportMetadata has the default value.

If you want to control the report value loading from onyx, you can do it in the next way:

const [report, reportResult] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${reportIDFromRoute}`, {allowStaleData: true, canBeMissing: true}); const isReportLoadingFromOnyx = isLoadingOnyxValue(reportResult);

@SzymczakJ Let me know if I misunderstood anything 😅

Yes, they are two separate values, but they are both coming from OpenReport API call. ReportMetadata in successData and report from the BE. I also can't use const [report, reportResult], because in this case the Onyx is empty, so it doesn't load from Onyx, but from the BE.

My main problem is that these two result of the same API call are applied in different renders(ReportMetadata first) which causes my loading state to hide even though it doesn't have the report yet.

Do you think it can be fixed?

Okay, let me double-check it all!

My main problem is that these two result of the same API call are applied in different renders(ReportMetadata first) which causes my loading state to hide even though it doesn't have the report yet.

I think I can reproduce it, the successData is applied a little bit earlier than the data returned from the API. I'm investigating why it happens

mountiny

Overall looks good, but the setTimeout seems like hack

mountiny · 2025-07-24T21:14:23Z

src/hooks/useNewTransactions.ts

-        });
+        setTimeout(() => {
+            skipFirstTransactionsChange.current = false;
+        }, 1000);


Yeah this just seems like a workaround

VickyStash · 2025-07-30T11:40:16Z

@SzymczakJ During the testing, I've noticed that the initial bug (transaction highlighting on SearchMoneyRequestReportPage) isn't reproduced anymore on the main branch. I can reproduce it on the staging, but not in the main. Could you check it please?

SzymczakJ · 2025-07-30T13:26:54Z

Yes, the bug is no longer reproducible on main, but it's not reproducible only out of pure luck. The faulty Onyx behaviour stays the same.
Here's another video in which I go over the report rendering flow with the debugger:

Screen.Recording.2025-07-30.at.15.16.02.mov

In 1:08 we can see that reportMetadata is already merged and report is sitll an empty object, which then causes next render to show empty screen in 1:19. This is the problematic part.

I followed these repro steps: #66801 (comment)

VickyStash · 2025-07-30T16:25:58Z

Yeah @SzymczakJ, sorry for the confusion, as I've mentioned here I was able to reproduce the issue!

What I've defined for now is that:

Both API response onyx updates and successData updates are put into queuedOnyxUpdates.
Then, the Onyx.update is called with queued updates:

App/src/libs/actions/QueuedOnyxUpdates.ts

Line 54 in 4dc9dd5

return Onyx.update(copyUpdates);

Example of the data passed to the Onyx.update during the flow: queuedUpdatesData.json. It contains both API response onyx updates and successData updates.

Inside Onyx.update, at the end all of the updates are applied with Promise.all: https://github.com/Expensify/react-native-onyx/blob/632c00e71873d596668daf7418452155d513434c/lib/Onyx.ts#L654. I believe the root cause of the issue can be there, I'm going to do more testing tomorrow to confirm.

VickyStash · 2025-07-31T16:23:42Z

I think I was able to define the root cause of the issue. In the response of OpenReport API call we have several mergecollection/merge updates to onyx:

Inside the Onyx.update these updates are grouped into one mergeCollection update

The inconsistency appears in notifying subscribers about the onyx update.
mergeCollectionWithPatches → scheduleNotifyCollectionSubscribers for collection updates takes much longer than
applyMerge → broadcastUpdate → scheduleSubscriberUpdate for the usual merge
ending up the merge updates received a little bit earlier by the app while the mergecollection updates are processed.

So if I try to replace mergecollection calls to simple merge -> it works faster and there is no issue.
I'll try to think about the way we can improve it!

VickyStash · 2025-08-01T14:46:18Z

Hey, unfortunately, I haven't had enough time today for investigations here, as I needed to switch to the deploy blockers investigation. I'll continue here on Monday!

SzymczakJ · 2025-08-01T18:30:26Z

FYI I'm OOO for the whole next week.

VickyStash · 2025-08-04T16:54:04Z

I've defined that the batch updates logic in the onyx was working for withOnyx subscribers only. I've tried to make it work for useOnyx hook subscribers, and it fixed the issue with inconsistency in notifying subscribers about Onyx.update updates (mentioned it here).
I'm testing over the updates right now to see if there are any side effects due to the updates.

VickyStash · 2025-08-05T16:43:14Z

I've defined that the batch updates logic in the onyx was working for withOnyx subscribers only. I've tried to make it work for useOnyx hook subscribers, and it fixed the issue with inconsistency in notifying subscribers about Onyx.update updates (mentioned it #66801 (comment)).
I'm testing over Expensify/react-native-onyx#669 right now to see if there are any side effects due to the updates.

UPD: I'm investigating some flaky tests in both E/App and react-native-onyx. Trying to figure out if the problem is in the tests or in the functionality updates.

VickyStash · 2025-08-06T16:26:30Z

UPD: Still working on testing/checking unit tests. It turns out the app tests were also affected by the changes in v2.0.128-2.0.130, which confused me a little.
But overall, the manual testing gives good results!

VickyStash · 2025-08-07T15:06:23Z

UPD: Opened Onyx PR for review!

VickyStash · 2025-08-25T15:01:07Z

Hi there! I was finally unblocked for preparing the Onyx bump PR to v2.0.135! Most likely the PR will be ready for review tomorrow!

SzymczakJ · 2025-08-25T15:23:28Z

I checked it and it seems that this bump will solve this bug right away. When the bump is merged I might refactor useNewTransactions.

VickyStash · 2025-08-26T15:50:42Z

Unfortunately, one issue popped up: Expensify/react-native-onyx#669 (comment)
I'll need to double-check how many places can be affected in the app and maybe change the approach a little.

VickyStash · 2025-08-28T15:56:59Z

UPD: I've started preparations for the new onyx PR, it's going to batch updates in case they come from one Onyx.update. It should fix the issue we have without side effect, I'm actively testing it

VickyStash · 2025-09-01T14:09:45Z

UPD: I've opened a new Onyx PR for review 🤞

VickyStash · 2025-09-04T14:31:52Z

UPD: I've started preparing Onyx bump PR!
@SzymczakJ I'd really appreciate if you can check if the issue you reported is fixed in the PR

SzymczakJ · 2025-09-05T07:48:01Z

Sure, I will take a look today.

SzymczakJ · 2025-09-05T14:49:02Z

I can confirm that the onyx issue was fixed @VickyStash 😁

VickyStash · 2025-09-08T14:16:21Z

I've opened bump PR for the review: #69872

VickyStash · 2025-09-10T06:17:47Z

Unfortunately the bump PR was reverted due to the deploy blocker Expensify/App#70184 😞
I'm going to be OOO September 11 - 21 🌴 , so I'll be able to get back to it after that

SzymczakJ · 2025-09-10T07:54:01Z

I wonder, if your PR really caused a bug or if the expense highlighting logic was based on previous useOnyx, which was not working correctly. Enjoy your holidays 😄.

VickyStash · 2025-09-10T07:57:44Z

I wonder, if your PR really caused a bug or if the expense highlighting logic was based on previous useOnyx, which was not working correctly. Enjoy your holidays 😄.

I've investigated it a little yesterday and the reason was in how useEffects hooks were triggered after my update, so yeah, it most likely can be just fixed on the app side.
BUT, I have a feeling there can be a lot of places like this, so at least every DB needs to be checked

Pujan92 · 2025-11-26T06:28:45Z

@SzymczakJ can we close this PR?

fix transaction highlighting on SearchMoneyRequestReportPage

fb130cf

SzymczakJ requested a review from a team as a code owner July 21, 2025 14:59

melvin-bot bot requested a review from Pujan92 July 21, 2025 14:59

melvin-bot bot removed the request for review from a team July 21, 2025 14:59

mountiny assigned SzymczakJ Jul 21, 2025

Pujan92 reviewed Jul 24, 2025

View reviewed changes

mountiny reviewed Jul 24, 2025

View reviewed changes

VickyStash mentioned this pull request Aug 7, 2025

Update batching to be applied to useOnyx updates as well Expensify/react-native-onyx#669

Merged

42 tasks

sumo-slonik mentioned this pull request Aug 19, 2025

Add the ability to cycle through reports #66608

Merged

51 tasks

Pujan92 mentioned this pull request Aug 19, 2025

[$250] Report opens with expense rows highlighted and page scrolled down #68685

Closed

16 tasks

SzymczakJ mentioned this pull request Aug 27, 2025

Remove flashing empty state when cycling through reports. #68363

Closed

VickyStash mentioned this pull request Aug 28, 2025

Update batching to be applied to useOnyx if they come from Onyx.update Expensify/react-native-onyx#681

Merged

42 tasks

VickyStash mentioned this pull request Sep 4, 2025

Bump onyx to v2.0.139 #69872

Merged

51 tasks

VickyStash mentioned this pull request Sep 25, 2025

Old transaction are highlighted, when the cache was cleared #66799

Closed

16 tasks

SzymczakJ closed this Dec 4, 2025

	const [report] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${reportIDFromRoute}`, {allowStaleData: true, canBeMissing: true});
	const [reportMetadata = defaultReportMetadata] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_METADATA}${reportIDFromRoute}`, {canBeMissing: true, allowStaleData: true});

fix transaction highlighting on SearchMoneyRequestReportPage #66801

fix transaction highlighting on SearchMoneyRequestReportPage #66801

Uh oh!

Conversation

SzymczakJ commented Jul 21, 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 Jul 21, 2025

Uh oh!

mountiny commented Jul 21, 2025

Uh oh!

SzymczakJ commented Jul 22, 2025

Uh oh!

mountiny commented Jul 22, 2025

Uh oh!

SzymczakJ commented Jul 23, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SzymczakJ Jul 25, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mountiny left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VickyStash commented Jul 30, 2025

Uh oh!

SzymczakJ commented Jul 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

VickyStash commented Jul 30, 2025

Uh oh!

VickyStash commented Jul 31, 2025

Uh oh!

VickyStash commented Aug 1, 2025

Uh oh!

SzymczakJ commented Aug 1, 2025

Uh oh!

VickyStash commented Aug 4, 2025

Uh oh!

VickyStash commented Aug 5, 2025

Uh oh!

VickyStash commented Aug 6, 2025

Uh oh!

VickyStash commented Aug 7, 2025

Uh oh!

VickyStash commented Aug 25, 2025

Uh oh!

SzymczakJ commented Aug 25, 2025

Uh oh!

VickyStash commented Aug 26, 2025

Uh oh!

VickyStash commented Aug 28, 2025

Uh oh!

SzymczakJ commented Jul 21, 2025 •

edited

Loading

SzymczakJ Jul 25, 2025 •

edited

Loading

SzymczakJ commented Jul 30, 2025 •

edited

Loading