Convert SearchPage to functional component by Piotrfj · Pull Request #23076 · Expensify/App

Piotrfj · 2023-07-18T12:28:32Z

Details

Fixed Issues

$ #16251
PROPOSAL: proposal not yet created

Tests

Click on search icon in chat list to enter search page.
Verify if the results are the same as on production version of the application.

Verify that no errors appear in the JS console

Offline tests

Click on search icon in chat list to enter search page.
Verify if the results are the same as on production version of the application.

QA Steps

Click on search icon in chat list to enter search page.
Verify if the results are the same as on production version of the application.

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Web

Kapture.2023-08-23.at.14.57.44.mp4

Mobile Web - Chrome

Mobile Web - Safari

Desktop

Kapture.2023-08-23.at.14.57.44.mp4

iOS

Kapture.2023-08-24.at.13.12.01.mp4

Android

Kapture.2023-08-23.at.15.14.34.mp4

github-actions · 2023-07-18T12:28:48Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

Piotrfj · 2023-07-18T12:30:12Z

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

gedu · 2023-07-19T08:23:10Z

-            userToInvite,
-        };
-    }
+    const debouncedUpdateOptions =_.debounce(updateOptions, 75);


_.debounce returns always a new function, probably would be good to wrap it into an useCallback for the useEffect, so you can add it into the dependency array safely

Yes, the app should have the same reference across the re-renders. useCallback is necessary here.

gedu · 2023-07-19T08:25:45Z

-        this.setState({searchValue}, this.debouncedUpdateOptions);
-    }
+    useEffect(() => {
+        updateOptions();


if updateOptions isn't part of the dependency array, please add a comment explaining why

@gedu, wdyt about the next option - https://github.com/Expensify/App/pull/23076/files#r1267798688?

I just realized that updateOptions useCallback, also is recreated when the searchValue change, I think could be some useEffects and function duplications, would be good to update the logic, and merge where is possible

gedu · 2023-07-19T08:26:44Z

-            personalDetails,
-        });
+    useEffect(() => {
+        debouncedUpdateOptions()


why are you updating the options after every new word? You are listening to reports and personalDetails to update the options

gedu · 2023-07-19T08:37:25Z

 SearchPage.propTypes = propTypes;
 SearchPage.defaultProps = defaultProps;
-
+SearchPage.displayName = 'SearchPage';


use hooks useLocalize and useWindowDimensions

rezkiy37 · 2023-07-19T09:15:22Z

-    }
+    useEffect(() => {
+        updateOptions();
+    },[props.reports, props.personalDetails])


I think this useEffect should have only this function as a dependency. It is logically. Finally, it uses the same deps.

Suggested change

},[props.reports, props.personalDetails])

},[updateOptions])

rezkiy37 · 2023-07-19T09:21:07Z

-            recentReports,
-            personalDetails,
-        });
+    useEffect(() => {


Could you please group all useEffects in one place instead of having them in random places across the component.

fabioh8010 · 2023-07-24T12:32:09Z

-class SearchPage extends Component {
-    constructor(props) {
-        super(props);
+function SearchPage(props) {


Let's use prop destructuring now in order to be aligned with the style guidelines -> https://github.com/Expensify/App/blob/main/contributingGuides/STYLE.md#destructuring

~~Also, you have to remove usage of defaultProps and assign default values during the prop destructuring.~~

You don't need to do this one now, we should still use defaultProps in JS files.

fabioh8010

Just one nitpick, LGTM!

fabioh8010 · 2023-07-25T09:25:16Z

-    reports: {},
-};
+function SearchPage({betas = [], personalDetails = {}, reports= {}}) {
+    //Data for initialization (runs only on the first render)


Suggested change

//Data for initialization (runs only on the first render)

// Data for initialization (runs only on the first render)

thiagobrez · 2023-07-25T11:04:12Z

-    personalDetails: {},
-    reports: {},
-};
+function SearchPage({betas = [], personalDetails = {}, reports= {}}) {


Suggested change

function SearchPage({betas = [], personalDetails = {}, reports= {}}) {

function SearchPage({betas = [], personalDetails = {}, reports = {}}) {

Super nitpick sorry XD

thiagobrez · 2023-07-25T11:28:22Z

@Piotrfj The whole searching flow looks more complex than it needs to be, in my opinion. I know it was already like this before, but I propose the following to make it simpler and prevent a few re-renders:

Merge activeRecentReports, activePersonalDetails and activeUserToInvite into one state object: searchOptions, so you don't have to set 3 states (and therefore 3 re-renders), but only 1
Currently, the onChangeText function is only setting state for searchValue. Then, when searchValue is changed, in the useEffect you're calling debouncedUpdateOptions which calls updateOptions. My suggestion here is to move the updateOptions logic inside onChangeText, so you can delete updateOptions, debouncedUpdateOptions and consequently the useEffect

This is how it would look like in the end:

function SearchPage({betas = [], personalDetails = {}, reports= {}}) {

...

    const [searchValue, setSearchValue] = useState('')
    
    // Only 1 state for the 3 objects here
    const [searchOptions, setSearchOptions] = useState({
      activeRecentReports: initialRecentReports,
      activePersonalDetails: initialPersonalDetails,
      activeUserToInvite: initialUserToInvite
    });

    const {translate} = useLocalize();

    // delete updateOptions

    // delete debouncedUpdateOptions
  
    ...
    
    // delete the debouncedUpdateOptions useEffect

    ...
    
    const onChangeText = (value = '') => {
        setSearchValue(value);
        
        // `updateOptions` logic is here, with the debounce
        _.debounce(() => {
            const {recentReports: localRecentReports, personalDetails: localPersonalDetails, userToInvite: localUserToInvite} = OptionsListUtils.getSearchOptions(
                reports,
                personalDetails,
                searchValue.trim(),
                betas,
            );

            // only 1 state change here prevents rerenders
            setSearchOptions({
                activeUserToInvite: localUserToInvite,
                activeRecentReports: localRecentReports,
                activePersonalDetails: localPersonalDetails,
            });
        }, 75);
    }

}

This way when you look at onChangeText, you know exactly what's gonna happen all in one place, and also prevents re-renders from setting multiple states, and calling unnecessary effects

rezkiy37 · 2023-07-26T11:08:11Z

-    }
+    useEffect(() => {
+        debouncedUpdateOptions()
+    }, [searchValue, debouncedUpdateOptions])


Actually, searchValue not necessary here.
useEffect has debouncedUpdateOptions has updateOptions has searchValue. BTW, if you want to simplify this chain, you can pass searchValue as an argument to updateOptions.

Suggested change

}, [searchValue, debouncedUpdateOptions])

}, [debouncedUpdateOptions])

rezkiy37 · 2023-07-26T11:10:02Z

-};
+function SearchPage({betas = [], personalDetails = {}, reports = {}}) {
+    // Data for initialization (runs only on the first render)
+    const {recentReports: initialRecentReports, personalDetails: initialPersonalDetails, userToInvite: initialUserToInvite} = useMemo(() => OptionsListUtils.getSearchOptions(reports, personalDetails, '', betas), []);


Linter will complain on this, so just disable this rule

Suggested change

const {recentReports: initialRecentReports, personalDetails: initialPersonalDetails, userToInvite: initialUserToInvite} = useMemo(() => OptionsListUtils.getSearchOptions(reports, personalDetails, '', betas), []);

// eslint-disable-next-line react-hooks/exhaustive-deps

const {recentReports: initialRecentReports, personalDetails: initialPersonalDetails, userToInvite: initialUserToInvite} = useMemo(() => OptionsListUtils.getSearchOptions(reports, personalDetails, '', betas), []);

melvin-bot · 2023-07-27T08:47:13Z

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

marcaaron · 2023-07-28T18:44:19Z

@thesahindia let's get this tested and reviewed 🙏 🙇

allroundexperts · 2023-08-16T09:46:18Z

@allroundexperts lint issues are fixed, PR is ready

@Piotrfj Can you also please resolve the outstanding comments on your PR?

Piotrfj · 2023-08-21T09:25:48Z

@marcaaron @allroundexperts please accept a pr

allroundexperts · 2023-08-21T09:30:56Z

@Piotrfj I'm on it today!

allroundexperts · 2023-08-21T10:17:55Z

@Piotrfj I still see some un-resolved comments.

Can you please resolve these?

fabioh8010 · 2023-08-21T14:10:59Z

@allroundexperts @Piotrfj You can ignore this comment, is not valid:

Also, you have to remove usage of defaultProps and assign default values during the prop destructuring.

But @Piotrfj let's do this one:

Let's use prop destructuring now in order to be aligned with the style guidelines -> https://github.com/Expensify/App/blob/main/contributingGuides/STYLE.md#destructuring

allroundexperts · 2023-08-21T23:18:04Z

@Piotrfj Can you please upload screen recordings for all the platforms?

Piotrfj · 2023-08-22T08:37:42Z

@allroundexperts sure, gonna provide them today

allroundexperts · 2023-08-29T21:27:13Z

Bump @Piotrfj

Piotrfj · 2023-08-31T16:48:24Z

@allroundexperts examples provided, sorry for the delay, I was actually providing them earlier but forgot to save them...

marcaaron

Nice changes overall. We are on a merge freeze I think for the next couple of days. So, we will wait a bit before merging. If it goes for too long please tag me back in here!

marcaaron · 2023-09-06T00:20:29Z

The checklist also looks like it went out of date somewhere if we can fix that in the meantime @allroundexperts

marcaaron · 2023-09-14T00:37:43Z

Merge freeze is over so taking this off hold. @allroundexperts can you check my comment above?

allroundexperts · 2023-09-14T00:39:31Z

Merge freeze is over so taking this off hold. @allroundexperts can you check my comment above?

Hi @marcaaron!
I'd be updating it along with testing it again.

allroundexperts

Still testing well!

allroundexperts · 2023-09-14T01:04:22Z

This is ready @marcaaron!

marcaaron

LGTM. Let's 🪨 🤘

OSBotify · 2023-09-14T02:03: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.

OSBotify · 2023-09-14T10:52:10Z

🚀 Deployed to staging by https://github.com/marcaaron in version: 1.3.70-0 🚀

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

OSBotify · 2023-09-18T10:10:09Z

🚀 Deployed to production by https://github.com/thienlnam in version: 1.3.70-8 🚀

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

Convert SearchPage to functional component

1a892d2

gedu reviewed Jul 19, 2023

View reviewed changes

Comment thread src/pages/SearchPage.js Outdated

gedu reviewed Jul 19, 2023

View reviewed changes

rezkiy37 reviewed Jul 19, 2023

View reviewed changes

fabioh8010 suggested changes Jul 24, 2023

View reviewed changes

Fix comments in SearchPage refactoring

2d2676e

fabioh8010 approved these changes Jul 25, 2023

View reviewed changes

thiagobrez reviewed Jul 25, 2023

View reviewed changes

marcaaron mentioned this pull request Jul 25, 2023

[HOLD for payment 2023-12-29] [$250] Migrate SearchPage.js to function component #16251

Closed

1 task

marcaaron self-requested a review July 25, 2023 19:38

marcaaron assigned Piotrfj Jul 25, 2023

Piotrfj added 3 commits July 26, 2023 11:50

refactor state in SearchPage

804cc20

fix typo

d49f271

fix debounced updated function

c187f69

rezkiy37 reviewed Jul 26, 2023

View reviewed changes

Piotrfj marked this pull request as ready for review July 27, 2023 08:47

Piotrfj requested a review from a team as a code owner July 27, 2023 08:47

melvin-bot Bot removed the request for review from a team July 27, 2023 08:47

melvin-bot Bot requested a review from thesahindia July 27, 2023 08:47

thiagobrez mentioned this pull request Jul 28, 2023

LOW: [$500] Selection List Refactor: Simple Selection List #20354

Closed

fix prettier

6024bce

destructure the props

22042e5

marcaaron changed the title ~~Convert SearchPage to functional component~~ [HOLD merge freeze] Convert SearchPage to functional component Sep 6, 2023

marcaaron approved these changes Sep 6, 2023

View reviewed changes

marcaaron changed the title ~~[HOLD merge freeze] Convert SearchPage to functional component~~ Convert SearchPage to functional component Sep 14, 2023

allroundexperts approved these changes Sep 14, 2023

View reviewed changes

melvin-bot Bot requested a review from marcaaron September 14, 2023 01:03

marcaaron requested changes Sep 14, 2023

View reviewed changes

marcaaron self-requested a review September 14, 2023 01:57

marcaaron approved these changes Sep 14, 2023

View reviewed changes

marcaaron merged commit e18ec42 into Expensify:main Sep 14, 2023

OSBotify mentioned this pull request Sep 14, 2023

Deploy Checklist: New Expensify 2023-09-14 #27425

Closed

This was referenced Sep 14, 2023

[HOLD for payment 2023-09-27] mWeb/safari- Transition is not smooth when search page opened #27457

Closed

Revert "Convert SearchPage to functional component" #27476

Merged

	//Data for initialization (runs only on the first render)
	// Data for initialization (runs only on the first render)

	function SearchPage({betas = [], personalDetails = {}, reports= {}}) {
	function SearchPage({betas = [], personalDetails = {}, reports = {}}) {

	}, [searchValue, debouncedUpdateOptions])
	}, [debouncedUpdateOptions])

	const {recentReports: initialRecentReports, personalDetails: initialPersonalDetails, userToInvite: initialUserToInvite} = useMemo(() => OptionsListUtils.getSearchOptions(reports, personalDetails, '', betas), []);
	// eslint-disable-next-line react-hooks/exhaustive-deps
	const {recentReports: initialRecentReports, personalDetails: initialPersonalDetails, userToInvite: initialUserToInvite} = useMemo(() => OptionsListUtils.getSearchOptions(reports, personalDetails, '', betas), []);

Conversation

Piotrfj commented Jul 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

Uh oh!

github-actions Bot commented Jul 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Piotrfj commented Jul 18, 2023

Uh oh!

Uh oh!

gedu Jul 19, 2023 • 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!

gedu Jul 19, 2023 • 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!

rezkiy37 Jul 19, 2023 • 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabioh8010 Jul 24, 2023 • 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!

fabioh8010 left a comment

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!

thiagobrez commented Jul 25, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

melvin-bot Bot commented Jul 27, 2023

Uh oh!

marcaaron commented Jul 28, 2023

Uh oh!

allroundexperts commented Aug 16, 2023

Uh oh!

Piotrfj commented Aug 21, 2023

Uh oh!

allroundexperts commented Aug 21, 2023

Uh oh!

allroundexperts commented Aug 21, 2023

Uh oh!

fabioh8010 commented Aug 21, 2023

Uh oh!

allroundexperts commented Aug 21, 2023

Uh oh!

Piotrfj commented Aug 22, 2023

Uh oh!

allroundexperts commented Aug 29, 2023

Piotrfj commented Jul 18, 2023 •

edited

Loading

github-actions Bot commented Jul 18, 2023 •

edited

Loading

gedu Jul 19, 2023 •

edited

Loading

gedu Jul 19, 2023 •

edited

Loading

rezkiy37 Jul 19, 2023 •

edited

Loading

fabioh8010 Jul 24, 2023 •

edited

Loading