[$500] Settings - Pronouns list is not translated dynamically

[kbecciv]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Action Performed:

1. Open the app and login with user A (device 1)
2. Open the app in another device and login with user A (device 2)
3. From device 1, click on your avatar
4. From device 1, open profile settings
5. From device 1, click on Pronouns option and type "they" for example in the search bar
6. From device 2, change language to Spanish

Expected Result:

The Pronouns list should translated dynamically, currently it does only if you click back and navigate to Pronouns settings again

Actual Result:

The Pronouns list is not translated dynamically

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.3.35-5
Reproducible in staging?: y
Reproducible in production?: y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation

9.New.Expensify.-.1.July.2023.1.mp4

Recording.3372.mp4

Expensify/Expensify Issue URL:
Issue reported by: @mejed-alkoutaini
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1688240365001069

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01a28be895fb2072a1
• Upwork Job ID: 1697070055301615616
• Last Price Increase: 2023-08-31

[melvin-bot]

Triggered auto assignment to @anmurali (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[kbecciv]

The same issue for User name.
Reproduction steps:
1- Open the app and login with user A (device 1)
2- Open the app and login with user A (device 2)
3- From device 1, Click on your avatar
4- From device 1, Go to profile settings
5- From device 1, Click on display name option
6- From device 2, change the display name

Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1688245970202529

Recording.5296.mp4

[c3024]

Proposal

Please re-state the problem that we are trying to solve in this issue.

PronounsPage search results do not update dynamically on changing locale from another device

What is the root cause of that problem?

loadPronouns function of PronounsPage updates only if props.currentUserPersonalDetails.pronouns changes. So when locale was updated this loadPronouns and pronounsList is not updated.

https://github.com/Expensify/App/blob/c0ece67e0edcb725d8f72cf57cd40d98854c552c/src/pages/settings/Profile/PronounsPage.js#L40C5-L73C53

What changes do you think we should make in order to solve the problem?

We should add props.translate as dependency for this useCallback.

While some other prop can be added as dependency to get the same effect, it is ideal to add the same props that are used in the callback function so I think adding props.translate to the callback is the best choice.

Optional

Also in the above function whenever currentUserPersonalDetails (and with our change props.translate also) change, the initiallyFocusedOptions change and from here

https://github.com/Expensify/App/blob/c0ece67e0edcb725d8f72cf57cd40d98854c552c/src/pages/settings/Profile/PronounsPage.js#L100C1-L102C34

searchValue gets updated. I think this is not desirable because the search string entered by user could be different and because from some other device if we change pronouns (or locale after this modification), search string gets cleared. We would want the intiallyFocusedOption to be in searchValue only on load of page. After that, we would want the search string to remain be the user input and not get updated due to changes from other devices. We want only the search results to be of the selected locale for the search string.
So, we should move the
https://github.com/Expensify/App/blob/c0ece67e0edcb725d8f72cf57cd40d98854c552c/src/pages/settings/Profile/PronounsPage.js#L48C17-L53C18
this into a separate useEffect with no dependency.
Also we are updating searchVlue to '' when loadPronouns change. This should also be updated.
https://github.com/Expensify/App/blob/c0ece67e0edcb725d8f72cf57cd40d98854c552c/src/pages/settings/Profile/PronounsPage.js#L104C1-L107C24
So, the updated code is this
added useEffect

useEffect(() => {
        const currentPronouns = lodashGet(props.currentUserPersonalDetails, 'pronouns', '');
        const currentPronounsKey = currentPronouns.replace(CONST.PRONOUNS.PREFIX, '');
        setInitiallyFocusedOption({
            text: lodashGet(props.translate('pronouns'), currentPronounsKey, ''),
            keyForList: currentPronounsKey
        })
    }, [])

Remove this in loadPronouns

if (isCurrentPronouns) {
                    setInitiallyFocusedOption({
                        text: value,
                        keyForList: key,
                    });

Add dependency in loadPronouns

[props.currentUserPersonalDetails.pronouns, props.translate]

change the onChangeText in useEffect to

 useEffect(() => {
        onChangeText(searchValue);
        loadPronouns();
    }, [loadPronouns]);

What alternative solutions did you explore?

This last useEffect can work better with useState I think. But since this is pronouns page, extra refactoring might not be worth the effort.

Result

Screen.Recording.2023-07-03.at.7.56.38.AM.mov

Apparently, en.js also includes other languages results, so results for English can be confusing.

[c3024]

This is not related to this. Please post it as another bug.
Further, I think the display name fields should be updated only when this modal loads and should not get updated automatically if user changes these from some other device. There could be some thing typed in these fields from first device and it gets overwritten suddenly on change from other device. So this is not a bug and intentional in my opinion.

[huzaifa-99]

Proposal

Please re-state the problem that we are trying to solve in this issue.

We want the pronouns list and search value to update dynamically whenever the preferred locale is change from another device

What is the root cause of that problem?

ATM we are not updating the pronouns list and search value in the PronounsPage, whenever the preferred locale changes.

What changes do you think we should make in order to solve the problem?

We should watch for the preferredLocale prop from withLocalize and add it to the loadPronouns useCallback as a dependency key, something like

const loadPronouns = useCallback(() => {
    // ... internal code
}, [props.currentUserPersonalDetails.pronouns, props.preferredLocale]); // add to dependency list

What alternative solutions did you explore? (Optional)

N/A

[anmurali]

I cannot reproduce

[huzaifa-99]

@anmurali It is reproducible with 2 devices/browser tabs with these steps

1. Open Tab A, go to pronouns page
2. Open Tab B, and change language
3. Go to Tab A and without refreshing the page or closing modal or navigating to a different screen check the pronouns list, notice it is not translated but the header of the pronouns list is translated

[mejed-alkoutaini]

@anmurali Hi, are you will not able to reproduce? I'm able to reproduce with many devices

[c3024]

@anmurali Please check with the steps produced above. Also, videos clearly show the steps for the bug. Please check and reopen the issue as issues like these which are not updated dynamically are being fixed in all cases.

[huzaifa-99]

@anmurali Bump, this is still reproducible. @kbecciv can you please confirm?

[kbecciv]

@huzaifa-99 Did you check again with latest build?

[huzaifa-99]

@kbecciv yes, just checked with v1.3.47-3

Screen.Recording.2023-07-28.at.9.37.22.PM.mp4

[huzaifa-99]

bump @kbecciv

[huzaifa-99]

Gentle bump for visibility and reopening @kbecciv @anmurali.

[kbecciv]

Hello @huzaifa-99 @anmurali Coming back from OOO, checking if able to reproduce in latest build

[kbecciv]

Reproduced on build 1.3.57-5

Recording.4031.mp4

Screen_Recording_20230826_101413_Chrome.mp4

[huzaifa-99]

Hello @huzaifa-99 @anmurali Coming back from OOO, checking if able to reproduce in latest build

Hi, thank you for checking again @kbecciv (hope you had a great time during the holidays! 🙌)

[melvin-bot]

@anmurali Whoops! This issue is 2 days overdue. Let's get this updated quick!

[anmurali]

Looks like still reproducible in latest build

[burczu]

Since proposals from both @c3024 and @huzaifa-99 are very similar, I prefer the one from @huzaifa-99 - I believe adding preferredLocale as a dependency of useCallback is more reasonable because it is the property that is actually changing when we change the language in the App. The change of the translate method (used in @c3024's proposal) is just a side effect of it.

🎀 👀 🎀 C+ reviewed

[melvin-bot]

Triggered auto assignment to @MonilBhavsar, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[c3024]

I see the point that props.preferredLocale is the fundamental value that is causing this change but as far as my understanding goes, React docs say we need to include all reactive values used in the hook in the dependency array or we need to override the linter check like this.

App/src/pages/settings/Profile/PronounsPage.js

Line 63 in 60c973d

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

So, from a pure React perspective it is ideal to add the reactive values used in the hook to be added in the dependency array so props.translate being the reactive value used here appears to me is the right dependency to be added here.

We have similar examples here

App/src/pages/workspace/reimburse/WorkspaceReimburseView.js

Line 98 in 60c973d

const getUnitLabel = useCallback((value) => translate(`common.${value}`), [translate]);

and

App/src/pages/workspace/reimburse/WorkspaceReimburseView.js

Line 106 in 60c973d

}, [translate, distanceCustomUnit, getUnitLabel, getRateLabel]);

and

App/src/components/AttachmentModal.js

Line 146 in 60c973d

[translate],

and with useEffect here

App/src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsSplitSelector.js

Line 164 in aab0b22

}, [betas, reports, participants, personalDetails, translate, searchTerm, setNewChatOptions]);

and with useMemo here

App/src/pages/iou/IOUCurrencySelection.js

Line 127 in 60c973d

}, [currencyList, searchValue, selectedCurrencyCode, translate]);

and

App/src/components/StatePicker/StateSelectorModal.js

Line 63 in 60c973d

[translate, currentState],

and many other places like PaymentMethodList, BaseLoginForm etc. in the repo where translate is added as a dependency and not preferredLocale because translate is the reactive value used in the hook.

Please see if this is worth considering and reconsider the review if using props.translate makes more sense. @burczu @MonilBhavsar

[burczu]

@c3024 In React docs the default setting recommended for the react-hooks/exhaustive-deps rule is warn, so in my opinion this rule is designed to notify developers that maybe they have forgotten adding some dependencies to useEffect/useCallback/useMemo hooks. But if adding all the values to the deps array does not make sense, we should not do it.

But maybe Expensify has different point of view here, so I'm curious about @MonilBhavsar opinion.

P.S. I've checked the code and it seems that the translate method may not be stable (it is a member of the class component LocaleContextProvider), so it may be re-created on every render and adding it into a hook deps arrays may not be best idea in terms of performance - it may be good to re-write this component to the functional one and wrap the translate method with useCallback.

[c3024]

I think my comment came off incorrectly. I didn't mean to add all dependencies used in a hook to the array mandatorily. I just meant that if we were to add a prop to the dependency array it might make more sense to add the exact prop used in hook that is translate here. I also found this principle followed on the repo at many other places.

I was unaware of the inefficiency of translate as it exists now. If it is inefficient, then refactoring translate or all the hooks with one of the dependencies as translate might be worth the effort.

[burczu]

Just to inform: I'll be OOO on Thursday and Friday (7-8 September).

[tienifr]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Changing language does not update pronouns list accordingly.

What is the root cause of that problem?

Currently, we're filtering the pronouns list based on searchValue only without considering locale changes.

App/src/pages/settings/Profile/PronounsPage.js

Lines 81 to 87 in 34d4e54

	const filteredPronounsList = useMemo(() => {
	const searchedValue = searchValue.trim();
	if (searchedValue.length === 0) {
	return [];
	}
	return _.filter(pronounsList, (pronous) => pronous.text.toLowerCase().indexOf(searchedValue.toLowerCase()) >= 0);
	}, [pronounsList, searchValue]);

What changes do you think we should make in order to solve the problem?

There's another issue relating to locale change for emoji suggestion list here #25735 where they've concluded that searching in English should also display the Spanish result and vice versa.

Add another pronouns list holding the translated version of filteredPronounsList, since the key for each pronoun item is also the translation key, we can reuse it:

const translatedFilteredPronounsList = useMemo(
    () =>
        _.map([...filteredPronounsList], (pronous) => ({
            ...pronous,
            text: props.translate(`pronouns.${pronous.keyForList}`),
        })),
    [filteredPronounsList, props.preferredLocale],
);

Note: Some limitations if we only add props.preferredLocale to loadPronouns's dependencies:

1. The searched results list is not translated accordingly, only the current pronoun is translated. For example, if my current pronoun was He / Him / His and I searched for She, switched to Spanish, it would show Él and Él / Ellos, which is the translation of He / Him / His / They / Them / Theirs, while it's supposed to be Ella and Ella / Ellos.

Screen.Recording.2023-09-06.at.21.33.08.mov

2. When the search value is cleared, the pronouns list will display a list mixed of both English and Spanish pronouns:

Result

Screen.Recording.2023-08-25.at.16.35.34-compressed.mov

[MonilBhavsar]

I see we have mixed use of translate and props.preferredLocale in dependencies. I don't think we have a rule here. we can discuss in slack in #expensify-opensource, and agree on using one dependency.

[tienifr]

@burczu My proposal was a little bit late, but can you take a look?

[MonilBhavsar]

Awaiting reviews

[melvin-bot]

@burczu, @anmurali, @MonilBhavsar Whoops! This issue is 2 days overdue. Let's get this updated quick!

[burczu]

@tienifr:

There's another issue relating to locale change for emoji suggestion list here #25735 where they've concluded that searching in English should also display the Spanish result and vice versa.

I don't think what applies to searching emojis applies also to searching for pronouns... Also I have concerns about the "vice versa" part of your comment - for emojis they wanted to allow searching in english if you set spanish, but not allowing searching in spanish when you set language to english.

This behaviour for emojis seems reasonable but for regular search it is strange... What do you think @MonilBhavsar?

[c3024]

It appears that the page was refactored and changed significantly and translate was added to the dependencies already.

[burczu]

@c3024 You mean it's fixed already?

[c3024]

Yes

[burczu]

Ok, I'll test it later to confirm.

[MonilBhavsar]

@burczu did you get a chance to take a look?
I see that dependency is added, so should be fixed

[burczu]

@MonilBhavsar sorry for the delay - I have quite a lot on me now. I'll check it today.

[burczu]

@MonilBhavsar Re-tested and I can confirm it's fixed now.

[anmurali]

So we can close this I believe? Based on #22089 (comment)

Please reopen if I misunderstood.