Search - Highlight newly added expense #49192

ikevin127 · 2024-09-13T21:32:53Z

Details

Fixed Issues

$ #48716
PROPOSAL:

Tests

Switch to the Search page tab.
Click the global create button (FAB) > Submit expense > Submit an expense.
Verify you remain on the search page.
Verify that the newly added expense appears in the table and the row is briefly highlighted to draw attention to it.

Verify that no errors appear in the JS console

Offline tests

Switch to the Search page tab.
Go offline.
Click the global create button (FAB) > Submit expense > Submit an expense.
Verify you remain on the search page.
Go back online.
Verify that the newly added expense appears in the table and the row is briefly highlighted to draw attention to it.

QA Steps

TLDR: Same as Tests.

Switch to the Search page tab.
Click the global create button (FAB) > Submit expense > Submit an expense.
Verify you remain on the search page.
Verify that the newly added expense appears in the table and the row is briefly highlighted to draw attention to it.

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Android: Native

android.webm

Android: mWeb Chrome

android-mweb.webm

iOS: Native

ios.mp4

iOS: mWeb Safari

ios-mweb.mp4

MacOS: Chrome / Safari

web.mov

MacOS: Desktop

desktop.mov

melvin-bot · 2024-09-13T21:32:58Z

@dubielzyk-expensify @ishpaul777 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]

ikevin127 · 2024-09-13T21:37:28Z

src/components/Search/index.tsx

+    const searchTriggeredRef = useRef(false);
+
+    useEffect(() => {
+        const previousTransactionsLength = previousTransactions && Object.keys(previousTransactions).length;
+        const transactionsLength = transactions && Object.keys(transactions).length;
+
+        if (!previousTransactionsLength || !transactionsLength || previousTransactionsLength === transactionsLength) {
+            return;
+        }
+
+        // Check if the search block was already triggered
+        if (!searchTriggeredRef.current && transactionsLength > previousTransactionsLength) {
+            // A transaction was added, trigger the action again
+            SearchActions.search({queryJSON, offset});
+            searchTriggeredRef.current = true; // Set the flag to true to prevent further triggers
+        }
+    }, [transactions, previousTransactions, queryJSON, offset]);
+
+    // Reset the flag using InteractionManager after the search action
+    useEffect(() => {
+        if (!searchTriggeredRef.current) {
+            return;
+        }
+
+        // Schedule a task to reset the flag after all current interactions have completed
+        const interactionHandle = InteractionManager.runAfterInteractions(() => {
+            searchTriggeredRef.current = false; // Reset the flag after interactions
+        });
+


@luacmartins Ideally, we would update the Search snapshot in BE and pop the newly added transaction in the search list via pusher and we wouldn't have to check transactions / previous transactions in this way - but it's better than performing a full page reload from a UX point of view, given that we want to pop the row in with a nice animation.

ishpaul777 · 2024-09-13T21:41:54Z

this needs a c+, right ? @ikevin127

ikevin127 · 2024-09-13T21:45:05Z

@ishpaul777 I assume it does, coming from the issue - currently we're still polishing the animation and I have to fix the failed checks. Will let you know once this is ready for C+ review 👍

ishpaul777 · 2024-09-13T21:46:34Z

Nice, thanks for clarifying 🤗 let me know if i can help anyways..

…7-searchAnimateHighlight

ikevin127 · 2024-09-14T00:00:12Z

@luacmartins Not sure why the tests are failing, could it be because of the FE implementation mentioned in #49192 ? 🤔

Unit tests (job 1)

Summary of all failing tests
FAIL tests/actions/OnyxUpdateManagerTest.ts

● Test suite failed to run
 Jest worker encountered 4 child process exceptions, exceeding retry limit
at ChildProcessWorker.initialize (node_modules/jest-runner/node_modules/jest-worker/build/workers/ChildProcessWorker.js:181:21)

Test Suites: 1 failed, 30 passed, 31 total
Tests:       307 passed, 307 total
Snapshots:   0 total
Time:        62.075 s, estimated 85 s

Unit tests (job 2)

Summary of all failing tests
FAIL tests/actions/ReportTest.ts (32.242 s)  
  ● actions/Report › should show a notification for report action updates with shouldNotify  

    expect(jest.fn()).toBeCalledWith(...expected)  

    Expected: "1", {"actionName": "ADDCOMMENT"}  

    Number of calls: 0  

      549 | .then(() => {  
      550 | // Ensure we show a notification for this new report action  
    > 551 | expect(Report.showReportActionNotification).toBeCalledWith(REPORT_ID, REPORT_ACTION);  
      552 | })  
      553 |  

      at toBeCalledWith (tests/actions/ReportTest.ts:551:61)  

FAIL tests/unit/OnyxUpdateManagerTest.ts  
  ● OnyxUpdateManager › should fetch missing Onyx updates once, defer updates and apply after missing updates  

    expect(jest.fn()).toHaveBeenCalledTimes(expected)  

    Expected number of calls: 1  
    Received number of calls: 0  

      53 |  
      54 | // There are no gaps in the deferred updates, therefore only one call to getMissingOnyxUpdates should be triggered  
    > 55 | expect(App.getMissingOnyxUpdates).toHaveBeenCalledTimes(1);  
      56 | expect(ApplyUpdates.applyUpdates).toHaveBeenCalledTimes(1);  
      57 |  
      58 | // validateAndApplyDeferredUpdates should be called twice, once for the initial deferred updates and once for the remaining deferred updates with gaps.  

      at toHaveBeenCalledTimes (tests/unit/OnyxUpdateManagerTest.ts:55:47)  

  ● OnyxUpdateManager › should only apply deferred updates that are newer than the last locally applied update (pending updates)  

    expect(jest.fn()).toHaveBeenCalledTimes(expected)  

    Expected number of calls: 1  
    Received number of calls: 0  

      87 |  
      88 | // There are no gaps in the deferred updates, therefore only one call to getMissingOnyxUpdates should be triggered  
    > 89 | expect(App.getMissingOnyxUpdates).toHaveBeenCalledTimes(1);  
      90 | expect(ApplyUpdates.applyUpdates).toHaveBeenCalledTimes(1);  
      91 |  
      92 | // validateAndApplyDeferredUpdates should be called twice, once for the initial deferred updates and once for the remaining deferred updates with gaps.  

      at toHaveBeenCalledTimes (tests/unit/OnyxUpdateManagerTest.ts:89:47)  

  ● OnyxUpdateManager › should re-fetch missing updates if the deferred updates have a gap  

    expect(jest.fn()).toHaveBeenCalledTimes(expected)  

    Expected number of calls: 2  
    Received number of calls: 0  

      117 |  
      118 | // Even though there is a gap in the deferred updates, we only want to fetch missing updates once per batch.  
    > 119 | expect(App.getMissingOnyxUpdates).toHaveBeenCalledTimes(2);  
      120 | expect(ApplyUpdates.applyUpdates).toHaveBeenCalledTimes(2);  
      121 |  
      122 | // validateAndApplyDeferredUpdates should be called twice, once for the initial deferred updates and once for the remaining deferred updates with gaps.  

      at toHaveBeenCalledTimes (tests/unit/OnyxUpdateManagerTest.ts:119:47)  

  ● OnyxUpdateManager › should re-fetch missing deferred updates only once per batch  

    expect(jest.fn()).toHaveBeenCalledTimes(expected)  

    Expected number of calls: 2  
    Received number of calls: 0  

      153 |  
      154 | // Even though there are multiple gaps in the deferred updates, we only want to fetch missing updates once per batch.  
    > 155 | expect(App.getMissingOnyxUpdates).toHaveBeenCalledTimes(2);  
      156 | expect(ApplyUpdates.applyUpdates).toHaveBeenCalledTimes(2);  
      157 | // validateAndApplyDeferredUpdates should be called twice, once for the initial deferred updates and once for the remaining deferred updates with gaps.  
      158 | // Unfortunately, we cannot easily count the calls of this function with Jest, since it recursively calls itself.  

      at toHaveBeenCalledTimes (tests/unit/OnyxUpdateManagerTest.ts:155:47)  

  ● OnyxUpdateManager › should not re-fetch missing updates if the lastUpdateIDFromClient has been updated  

    expect(jest.fn()).toHaveBeenCalledTimes(expected)  

    Expected number of calls: 1  
    Received number of calls: 0  

      193 |  
      194 | // Even though there are multiple gaps in the deferred updates, we only want to fetch missing updates once per batch.  
    > 195 | expect(App.getMissingOnyxUpdates).toHaveBeenCalledTimes(1);  
      196 |  
      197 | // validateAndApplyDeferredUpdates should be called once for the initial deferred updates  
      198 | // Unfortunately, we cannot easily count the calls of this function with Jest, since it recursively calls itself.  

      at toHaveBeenCalledTimes (tests/unit/OnyxUpdateManagerTest.ts:195:47)  

  ● OnyxUpdateManager › should re-fetch missing updates if the lastUpdateIDFromClient has increased, but there are still gaps after the locally applied update  

    expect(jest.fn()).toHaveBeenCalledTimes(expected)  

    Expected number of calls: 2  
    Received number of calls: 0  

      232 |  
      233 | // Even though there are multiple gaps in the deferred updates, we only want to fetch missing updates once per batch.  
    > 234 | expect(App.getMissingOnyxUpdates).toHaveBeenCalledTimes(2);  
      235 |  
      236 | // validateAndApplyDeferredUpdates should be called twice, once for the initial deferred updates and once for the remaining deferred updates with gaps.  
      237 | // Unfortunately, we cannot easily count the calls of this function with Jest, since it recursively calls itself.  

      at toHaveBeenCalledTimes (tests/unit/OnyxUpdateManagerTest.ts:234:47)  

  ● OnyxUpdateManager › should only fetch missing updates that are not outdated (older than already locally applied update)  

    expect(jest.fn()).toHaveBeenCalledTimes(expected)  

    Expected number of calls: 1  
    Received number of calls: 0  

      277 |  
      278 | // There are no gaps in the deferred updates, therefore only one call to getMissingOnyxUpdates should be triggered  
    > 279 | expect(App.getMissingOnyxUpdates).toHaveBeenCalledTimes(1);  
      280 | })  
      281 | })  
      282 |  

      at toHaveBeenCalledTimes (tests/unit/OnyxUpdateManagerTest.ts:279:47)  

Test Suites: 2 failed, 29 passed, 31 total  
Tests: 8 failed, 297 passed, 305 total  
Snapshots: 0 total  
Time: 63.428 s

luacmartins · 2024-09-15T21:50:43Z

Not sure either. They all seem unrelated. Are they consistently failing?

ikevin127 · 2024-09-15T22:16:02Z

@luacmartins They seem to be (job 1, 2 and performance) - will try another sync w/ main but if that doesn't do it, not sure what the next steps should be here 🤔

dubielzyk-expensify · 2024-09-16T00:10:33Z

I don't seem to always get the highlight animation when submitting from an empty state:

CleanShot.2024-09-16.at.10.08.06.mp4

I've tried to reproduce it and get the animation once in a while, but not reliably

dubielzyk-expensify · 2024-09-16T00:11:59Z

As for the animation, I tested it locally and it looks really good 😄 Great job! 👏 cc @Expensify/design for viz

…7-searchAnimateHighlight

ikevin127 · 2024-09-16T20:29:18Z

I don't seem to always get the highlight animation when submitting from an empty state:

@dubielzyk-expensify Thanks for double-checking this, you saved us a regression with this one as I missed checking the case when the list is empty (new account) - which caused the following two issues:

When the list is empty (new account), there won't be any previous transactions -> the Search get wouldn't be called with previous logic. ✅ Fixed the logic to account for this!
When the list is empty (new account), there won't be any previous transactions -> the shouldAnimateInHighlight would be false because the newSearchResultKey would try to determine if new transaction key exists in the previous transactions list, but because this is empty on a new account -> newSearchResultKey would be undefined hence the newly added transaction wouldn't animate in / highlight. ✅ Fixed the logic to account for this!

This is how it looks now on a new account when creating the first transaction:

Screen.Recording.2024-09-16.at.13.23.02.mov

dubielzyk-expensify · 2024-09-17T03:43:49Z

Boom! That's looking excellent

luacmartins · 2024-09-18T23:18:23Z

Nice! This is looking great. @ikevin127 we have some conflicts. Also is this ready for review?

…7-searchAnimateHighlight

ikevin127 · 2024-09-19T18:24:24Z

♻️ I will look into this today, use some AI to try and find a link between my changes and the tests that are failing.

Update: Turns out the reason why tests are failing is caused by the usage of useAnimatedHighlightStyle in BaseListItem. I'm looking into fixing the tests and getting this ready for review.

Update 2: Because the useAnimatedHighlightStyle hook uses useScreenWrapperTransitionStatus I had to add targeted mocks of this hook to the files where the tests were failing and also to the SelectionList.perf-test.tsx in order to have all tests (including performance) pass.

Note

I had to target specific files because if I were to add the useScreenWrapperTransitionStatus mock gloabally in jest/setup.ts then other unrelated tests would fail because not all components are using that hook.

…7-searchAnimateHighlight

ikevin127 · 2024-09-25T23:00:13Z

@luacmartins Thanks for double checking this and catching the bugs 👍 I just applied the fixes for the two bugs mentioned (ref1, ref2) and also performed the refactoring requested in #49192 (comment) which ensures we keep the logic in the Search component small.

Here's a video after the fix was applied:

bugfixes.mov

Please check this again and let me know if there are any other issues 🤞

luacmartins · 2024-09-26T00:55:15Z

Nice! Thanks for working on those. It seems like a few tests are failing. @ishpaul777 could you please review this PR again once the tests above are fixed?

ishpaul777 · 2024-09-26T10:50:15Z

i will rereview and try to finish this in couple of hours 🙏

ishpaul777 · 2024-09-26T15:20:21Z

Bug: Changing sorting, highlights and scroll to random expense item

Screen.Recording.2024-09-26.at.8.39.19.PM.mov

ishpaul777 · 2024-09-26T15:28:31Z

Bug: Search results scroll and highlights expense when changing status from outstading to All

Screen.Recording.2024-09-26.at.8.55.46.PM.mov

ishpaul777 · 2024-09-26T15:33:20Z

Bug: while scrolling down search result, there is auto scroll randomly and sometimes random expense is hightlighted (This might requires account with many (~100+) expenses)

Screen.Recording.2024-09-26.at.8.59.52.PM.mov

ikevin127 · 2024-09-27T20:01:37Z

@ishpaul777 ♻️ I refactored the code to use a single hook (useSearchHighlightAndScroll) and improved the logic to fix the 3 issues you mentioned above. Please test again and let me know if there are any other issues!

Note: The ❌ Prettier check fail is not related to the changes here because I ran it before pushing the commit and everything was fine.

…7-searchAnimateHighlight

ishpaul777 · 2024-09-27T20:23:26Z

About to leave for the day, i will take a peek now, and review afresh tomorrow 🙏

ishpaul777 · 2024-09-30T15:36:09Z

we have conflicts here @ikevin127

i did not face those bugs (or any other bugs) again in my testing, and also code wise it Looks good!

…7-searchAnimateHighlight

luacmartins

LGTM

OSBotify · 2024-09-30T23:04:13Z

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

github-actions · 2024-10-02T00:31:33Z

🚀 Deployed to staging by https://github.com/luacmartins in version: 9.0.43-0 🚀

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

github-actions · 2024-10-02T01:10:13Z

🚀 Deployed to staging by https://github.com/luacmartins in version: 9.0.43-0 🚀

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

Julesssss · 2024-10-02T13:03:49Z

Hey we have a minor regression here. I'm removing the blocker label, and let me know if you'd like to handle this as a follow up.

ikevin127 · 2024-10-02T17:16:24Z

@Julesssss ✅ I confirm that #50046 is indeed a regression of this PR ~~and I will open a follow-up PR to fix it since this is still in the regression period.~~

Actually, the PR's reviewer will open a follow-up and I will review it, coming from #50046 (comment).

github-actions · 2024-10-03T03:34:24Z

🚀 Deployed to production by https://github.com/jasperhuangg in version: 9.0.43-6 🚀

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

ikevin127 · 2024-10-04T22:36:59Z

src/components/SelectionList/Search/ReportListItem.tsx

@@ -77,6 +77,9 @@ function ReportListItem<TItem extends ListItem>({
        styles.overflowHidden,
        item.isSelected && styles.activeComponentBG,
        isFocused && styles.sidebarLinkActive,
+        // Removing some of the styles because they are added to the parent OpacityView via animatedHighlightStyle
+        {backgroundColor: 'unset'},


This style overriding caused the following issue:

[HOLD for payment 2024-10-11] Search - Expenses are not highlighted when selected #50046

Which we fixed by moving the style up, under the overflowHidden and changing it to styles.bgTransparent for compatibility with native.

Search - Highlight newly added expense

4668b8a

ikevin127 requested a review from a team as a code owner September 13, 2024 21:32

melvin-bot bot requested review from dubielzyk-expensify and ishpaul777 and removed request for a team September 13, 2024 21:32

ikevin127 mentioned this pull request Sep 13, 2024

[HOLD for payment 2024-10-11] [HOLD for payment 2024-10-10] [$250] [Search v2.2] Reload the page and highlight the newly added expense when adding a new expense from the search page #48716

Closed

ikevin127 commented Sep 13, 2024

View reviewed changes

luacmartins self-requested a review September 13, 2024 21:49

ikevin127 added 2 commits September 13, 2024 16:34

fixed type issues

14818c8

Merge branch 'main' of https://github.com/Expensify/App into ikevin12…

43a49eb

…7-searchAnimateHighlight

ikevin127 added 2 commits September 16, 2024 13:27

adjusted logic for new account (empty Search transaction list)

b38ba48

Merge branch 'main' of https://github.com/Expensify/App into ikevin12…

db55ff0

…7-searchAnimateHighlight

Merge branch 'main' of https://github.com/Expensify/App into ikevin12…

3da3f78

…7-searchAnimateHighlight

This comment was marked as outdated.

Sign in to view

ikevin127 added 3 commits September 19, 2024 14:33

added useScreenWrapperTransitionStatus mock to fix failing tests

4595e26

added BootSplash mock and adjusted previously added mock

24c9e8f

targeted useScreenWrapperTransitionStatus mock to failing test files

aa9317b

Merge branch 'main' of https://github.com/Expensify/App into ikevin12…

34c133f

…7-searchAnimateHighlight

fixed lint and prettier

cc892d8

This comment was marked as outdated.

Sign in to view

refactoring and bugfixing

90ddc4d

ikevin127 added 2 commits September 27, 2024 13:05

Merge branch 'main' of https://github.com/Expensify/App into ikevin12…

265916d

…7-searchAnimateHighlight

prettier

c66f148

Merge branch 'main' of https://github.com/Expensify/App into ikevin12…

52c515a

…7-searchAnimateHighlight

luacmartins approved these changes Sep 30, 2024

View reviewed changes

luacmartins merged commit 8f5f609 into Expensify:main Sep 30, 2024
17 checks passed

SzymczakJ mentioned this pull request Oct 1, 2024

Add recent serches and recent chats in Search router #49457

Merged

48 tasks

ikevin127 mentioned this pull request Oct 1, 2024

Search highlight and scroll animation refactoring for 49457 #50028

Merged

50 tasks

github-actions bot mentioned this pull request Oct 2, 2024

Deploy Checklist: New Expensify 2024-10-02 #50030

Closed

56 tasks

Krishna2323 mentioned this pull request Oct 2, 2024

[HOLD for payment 2024-10-11] Search - Expenses are not highlighted when selected #50046

Closed

6 tasks

This comment was marked as outdated.

Sign in to view

ikevin127 commented Oct 4, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Search - Highlight newly added expense #49192

Search - Highlight newly added expense #49192

ikevin127 commented Sep 13, 2024

melvin-bot bot commented Sep 13, 2024

ikevin127 Sep 13, 2024

ishpaul777 commented Sep 13, 2024

ikevin127 commented Sep 13, 2024

ishpaul777 commented Sep 13, 2024

ikevin127 commented Sep 14, 2024 •

edited

Loading

luacmartins commented Sep 15, 2024

ikevin127 commented Sep 15, 2024

dubielzyk-expensify commented Sep 16, 2024

dubielzyk-expensify commented Sep 16, 2024

ikevin127 commented Sep 16, 2024

dubielzyk-expensify commented Sep 17, 2024

luacmartins commented Sep 18, 2024

This comment was marked as outdated.

ikevin127 commented Sep 19, 2024 •

edited

Loading

ikevin127 commented Sep 25, 2024

luacmartins commented Sep 26, 2024

ishpaul777 commented Sep 26, 2024

ishpaul777 commented Sep 26, 2024 •

edited

Loading

ishpaul777 commented Sep 26, 2024

ishpaul777 commented Sep 26, 2024

This comment was marked as outdated.

ikevin127 commented Sep 27, 2024 •

edited

Loading

ishpaul777 commented Sep 27, 2024

ishpaul777 commented Sep 30, 2024

luacmartins left a comment

OSBotify commented Sep 30, 2024

github-actions bot commented Oct 2, 2024

github-actions bot commented Oct 2, 2024

Julesssss commented Oct 2, 2024

This comment was marked as outdated.

ikevin127 commented Oct 2, 2024 •

edited

Loading

github-actions bot commented Oct 3, 2024

ikevin127 Oct 4, 2024

Search - Highlight newly added expense #49192

Search - Highlight newly added expense #49192

Conversation

ikevin127 commented Sep 13, 2024

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented Sep 13, 2024

ikevin127 Sep 13, 2024

Choose a reason for hiding this comment

ishpaul777 commented Sep 13, 2024

ikevin127 commented Sep 13, 2024

ishpaul777 commented Sep 13, 2024

ikevin127 commented Sep 14, 2024 • edited Loading

luacmartins commented Sep 15, 2024

ikevin127 commented Sep 15, 2024

dubielzyk-expensify commented Sep 16, 2024

dubielzyk-expensify commented Sep 16, 2024

ikevin127 commented Sep 16, 2024

dubielzyk-expensify commented Sep 17, 2024

luacmartins commented Sep 18, 2024

This comment was marked as outdated.

ikevin127 commented Sep 19, 2024 • edited Loading

ikevin127 commented Sep 25, 2024

luacmartins commented Sep 26, 2024

ishpaul777 commented Sep 26, 2024

ishpaul777 commented Sep 26, 2024 • edited Loading

ishpaul777 commented Sep 26, 2024

ishpaul777 commented Sep 26, 2024

This comment was marked as outdated.

ikevin127 commented Sep 27, 2024 • edited Loading

ishpaul777 commented Sep 27, 2024

ishpaul777 commented Sep 30, 2024

luacmartins left a comment

Choose a reason for hiding this comment

OSBotify commented Sep 30, 2024

github-actions bot commented Oct 2, 2024

github-actions bot commented Oct 2, 2024

Julesssss commented Oct 2, 2024

This comment was marked as outdated.

ikevin127 commented Oct 2, 2024 • edited Loading

github-actions bot commented Oct 3, 2024

ikevin127 Oct 4, 2024

Choose a reason for hiding this comment

ikevin127 commented Sep 14, 2024 •

edited

Loading

ikevin127 commented Sep 19, 2024 •

edited

Loading

ishpaul777 commented Sep 26, 2024 •

edited

Loading

ikevin127 commented Sep 27, 2024 •

edited

Loading

ikevin127 commented Oct 2, 2024 •

edited

Loading