search

Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.12k stars 2.62k forks source link

[$500] Expense - Workspace under "To" field is highlighted after opening user avatar #36577

Closed izarutskaya closed 4 months ago

izarutskaya commented 4 months ago

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

Version Number: v1.4.42-0 Reproducible in staging?: Y Reproducible in production?: N Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: Issue reported by: Applause-Internal Team Slack conversation:

Action Performed:

  1. Navigate to staging.new.expensify.com
  2. Go to FAB > Request money > Manual.
  3. Enter amount > Next.
  4. Select a workspace > Next.
  5. On confirmation page, click on the workspace under "To".
  6. Click on the user avatar (not workspace avatar).
  7. Return to confirmation page via back button on the top left.

Expected Result:

The workspace name under "To" will not be highlighted.

Actual Result:

The workspace name under "To" is highlighted.

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

https://github.com/Expensify/App/assets/115492554/ba52b88c-f125-47b3-b1b3-80caeba73266

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01bd38066315528994
  • Upwork Job ID: 1758115460406108160
  • Last Price Increase: 2024-03-07
melvin-bot[bot] commented 4 months ago

Job added to Upwork: https://www.upwork.com/jobs/~01bd38066315528994

melvin-bot[bot] commented 4 months ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @ntdiary (External)

melvin-bot[bot] commented 4 months ago

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

github-actions[bot] commented 4 months ago

:wave: Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.
melvin-bot[bot] commented 4 months ago

Triggered auto assignment to @dangrous (Engineering), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

izarutskaya commented 4 months ago

We think that this bug might be related to #wave5-free-submitters CC @dylanexpensify

Beamanator commented 4 months ago

I agree this should be changed, but I don't think it should be a blocker

ikevin127 commented 4 months ago

Proposal

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

The workspace name option under "To" is highlighted after opening user avatar.

What is the root cause of that problem?

Offending PR is https://github.com/Expensify/App/pull/35594 -> before this PR we were not able to click on the workspace details option as it was disabled.

Here's what causes the option to be highlighted once user views avatar (opens modal):

https://github.com/Expensify/App/blob/32648bbd01b2f1e98ff7273a92deb4c4bb10d7cb/src/components/OptionsSelector/BaseOptionsSelector.js#L163-L171

this re-render / setState within the BaseOptionsSelector component shouldn't happen in our case since focusedIndex was initialized with 1 on initial component mount, we should keep the initial focusedIndex instead of updating it when not necessary.

To be more specific about what exactly causes the re-render / setState (requested by reviewer https://github.com/Expensify/App/issues/36577#issuecomment-1954632876):

Video https://github.com/Expensify/App/assets/56457735/e07f3127-d794-4bfe-987e-4e3f0a16e67f

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

Considering the most recent reviewer's https://github.com/Expensify/App/issues/36577#issuecomment-1959744733:

Actually, focusedIndex doesn't seem to be used on this confirm page, so some possible directions are: how to keep it unchanged when re-rendering, or whether it can be disabled, etc.

the simplest thing to do here is to take advantage of the disableFocusOptions and pass it as true or selectedOptions.length <= 1 here:

https://github.com/Expensify/App/blob/cd148302d6b2266c9ac9ac36d9b7e2a337baa61b/src/components/MoneyTemporaryForRefactorRequestConfirmationList.js#L849

within the MoneyTemporaryForRefactorRequestConfirmationList component, since focusedIndex doesn't seem to be used on this confirm page -> we just disable the focusing behaviour all together 🙂

Using selectedOptions.length <= 1 to disable the focusing would make sure to not create regressions for multi-select case as discussed in issue https://github.com/Expensify/App/issues/37238, without invalidating the solution implemented there.

Videos

MacOS: Chrome / Safari | Before | After | | ------------- | ------------- | |
Krishna2323 commented 4 months ago

Not reproducible on main branch, we don't allow users to open workspace details.

https://github.com/Expensify/App/assets/85894871/39225c01-c103-4cd9-a361-b1f63f356302

Pujan92 commented 4 months ago

Proposal

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

Workspace option kept highlighted once we come back after opening the profile avatar

What is the root cause of that problem?

It seems the problem persists after this PR gets merged. Reason: When we open the profile avatar it calls the api openPublicProfilePage and its response will update the personalDetailsList onyx data which eventually regenerates the participants array here. Due to that sections prop gets changed and the execution will reach to set a new focused index part unnecessarily in BaseOptionsSelector.

https://github.com/Expensify/App/blob/50e07e8604d40b278c1ede2fc5c6fb78c97f1c1f/src/pages/iou/request/step/IOURequestStepConfirmation.js#L99-L106

https://github.com/Expensify/App/blob/421bbc5e319ffe906471a93917d789ed606e1a7e/src/components/OptionsSelector/BaseOptionsSelector.js#L149-L172

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

I believe it should be || here instead of ?? bcoz currently it won't check the second condition as the first will always return the boolean value which isn't null. With that change, it returns early and won't make the unnecessary api call when the avatarURL is available.

https://github.com/Expensify/App/blob/38c6c164c91ccc4c1a31e513301ec94436eac9b6/src/pages/settings/Profile/ProfileAvatar.tsx#L30-L34

if (!ValidationUtils.isValidAccountRoute(Number(accountID)) || !!avatarURL) {
ntdiary commented 4 months ago

Under review.

ntdiary commented 4 months ago

I think the RCA in the above proposals are not accurate enough and need to be further clarified. Also, I'm still testing. 🤔

ntdiary commented 4 months ago

Which field caused the re-render? :)

ntdiary commented 4 months ago

In this case, for non-split type requests, selectedOptions is always empty. Also, BaseOptionsSelector is a common component, and if we want to change the condition, we also need to ensure that the code after the condition can be executed normally in other scenarios. :)

ntdiary commented 4 months ago

@ntdiary Hope this one satisfies 🙂

@ikevin127, thank you very much for your efforts and updates, but I don't think we should add such a condition (e.g., other fields may also be pushed by the backend, thus triggering the same problem). My previous intention to clarify the specific field was to let everyone understand the essence of the problem and solve it more reasonably, rather than using isLoading as a condition.

Actually, focusedIndex doesn't seem to be used on this confirm page, so some possible directions are: how to keep it unchanged when re-rendering, or whether it can be disabled, etc.

looking forward a better proposal. ❤️

melvin-bot[bot] commented 4 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

brandonhenry commented 4 months ago

Proposal

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

When navigating back from the user avatar screen, the "To" field button in the MoneyRequestConfirmationList component is incorrectly styled with the defaultHoverBG color. This issue only occurs once, and manually removing the styling from the button prevents it from reoccurring.

What is the root cause of that problem?

The root cause of the problem is related to the hover state management in the PressableWithoutFocus component. The navigation event from the user avatar screen is causing the hover state to be incorrectly set, resulting in the defaultHoverBG color being applied to the "To" field button.

This is happening, because when we call Navigation.navigate(ROUTES.PROFILE_AVATAR.getRoute(icon.id));,

https://github.com/Expensify/App/blob/4a885f596547a16893ae40ebabcff41c82416453/src/components/RoomHeaderAvatars.js#L26

You might be wondering why it is not happening when we click the workspace avatar though. That is because inside of WorkspaceAvatar we are not doing anything that would cause Onyx to update that would send out a re-render update to specific components (verified via devtools)

Specifically, the useEffect inside ProfileAvatar is causing an update that forces the hoverBackground color onto the To field

https://github.com/Expensify/App/blob/8885fb8d98533d2ab323c0fd3830bb2a9e4fcb0a/src/pages/settings/Profile/ProfileAvatar.tsx#L28

(2) New Expensify - 23 February 2024 - Watch Video

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

We should extend our BaseGenericPressable class to account for pressable that we don't want to have a hover state. The button that has a hover state shouldn't be turning that way, even when we send updates to re-render

https://github.com/Expensify/App/blob/4a885f596547a16893ae40ebabcff41c82416453/src/components/Pressable/GenericPressable/BaseGenericPressable.tsx#L137

Specific changes include:

  1. Updating MoneyTemporaryForRefactorRequestConfiramationList so that the menu item To is pushed in with the property to make sure it never gets highlighted:

Ex. Add shouldHoverFocus: false here

  1. Update Section type to include: 

    /** Whether this section should be hoverable or not */
shouldHoverFocus?: boolean;

  2. Now, whenever we render an item, we can reference the field added on the section, and decide to not to apply hoverState styles based on this boolean

With these changes, we can ensure that the update only affects the To field and this allows us to create OptionRows that don't have the hoverStyle appended

We could change the hoverStyle be empty if shouldHoverFocus is true

We could also do something like 

optionIsFocused={!shouldHoverFocus && !disableFocusOptions && !isItemDisabled && focusedIndex === index + section.indexOffset}
ikevin127 commented 4 months ago

Updated proposal

Actually, focusedIndex doesn't seem to be used on this confirm page, so some possible directions are: how to keep it unchanged when re-rendering, or whether it can be disabled, etc.

cc @ntdiary Maybe this one will satisfy 🙂

brandonhenry commented 4 months ago

Hmm Kevin's updated proposal is essentially mine but with slight more specific detail on where to disable the pressable for the "To" field (didn't notice late night that the pressable already has support for disabling the hover if that pressable doesn't need it)

ikevin127 commented 4 months ago

Hmm Kevin's updated proposal is essentially mine but with slight more specific detail on where to disable the pressable

Nope it's not 😅 I'm not disabling any Pressable nor changing any existing styles. I'm using an already existing prop to disable the focus behaviour since as mentioned by latest reviewer's comment:

Actually, focusedIndex doesn't seem to be used on this confirm page

which also hinted at disabling the focus behaviour if possible.

My solution is simply adding a prop (1-liner), exactly what was suggested for as possible solution since all my previous solutions were too complicated. Also your solution is pretty vague:

We should extend our BaseGenericPressable class to account for pressable that we don't want to have a hover state.

don't you think ? Besides suggesting changing the common BaseGenericPressable's styles which has nothing to do with the RCA of our issue 🙂

brandonhenry commented 4 months ago

@ikevin127 ahh you're right. I see your proposal doesn't target what I believe is the fix haha. Good call, I personally think the issue is in the BaseGenericPressable and not the OptionsSelector itself

melvin-bot[bot] commented 4 months ago

@dangrous, @ntdiary, @slafortune Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

slafortune commented 4 months ago

@ntdiary is there a proposal here that will do the trick or are we still waiting on a better one?

ntdiary commented 4 months ago

@ntdiary is there a proposal here that will do the trick or are we still waiting on a better one?

@slafortune, so far, @ikevin127's proposal is a straightforward solution, but I'm still willing to wait a bit longer, There might be potentially better ideas. BTW, this issue is somewhat similar to issue #37238, so I will also discuss it there. :)

brandonhenry commented 4 months ago

@ntdiary i just updated my proposal. same root cause but I don't think my proposed solution was outlined enough for it to be considered.

My change is similar to @ikevin127 but I think the Section that each of these OptionRow are built off needs a new property that way we can control specifically which OptionRow should never be hoverable. If we set that new property for the to field, and update the OptionRow to not allow a change in hover style if that variable is set - we fix this issue and #37238

ikevin127 commented 4 months ago

There might be potentially better ideas. BTW, this issue is somewhat similar to issue https://github.com/Expensify/App/issues/37238, so I will also discuss it there. :)

This issue's root cause is related to OpenPublicProfilePage being called which updates focusedIndex because isLoading changes its boolean value, while the other issue's accepted RCA was pretty vague, mentioning that the sections prop changes and suggested to compare previous / current sections as a fix.

Turns out the fix PR from https://github.com/Expensify/App/issues/37238 selected proposal didn't work as expected and a regression was reported.

I think regardless of the root cause of the other issue, my latest proposed solution still fixes our issue, since as you mentioned above:

Actually, focusedIndex doesn't seem to be used on this confirm page

To me, this looks like a straight-forward slam dunk 🏀 I'm confused as to what's the hold up w/ this one really 🤔

cc @ntdiary @dangrous

melvin-bot[bot] commented 4 months ago

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

dangrous commented 4 months ago

I just went on parental leave so I haven't been paying attention to this one haha - @slafortune do you think you could fish for an internal volunteer in Slack? I can help if needed!

ikevin127 commented 4 months ago

Updated proposal

To +1 on my previous https://github.com/Expensify/App/issues/36577#issuecomment-1967517205, this is a direct quote of @ neil-marcellini from https://github.com/Expensify/App/issues/37238#issuecomment-1968029563:

I think we have a pretty clear problem and solution for this specific issue. There are other issues I'm sure, but let's solve those elsewhere.

My take from that is that we should handle this issue here and treat it as separate from the other similar issue - which just merged the PR that will soon arrive on staging.

I'm pretty sure our issue will still occur even after that issue's PR arrives on staging, since my proposal's RCA will still occur even though the other proposal's solution added an early return if previous - current sections are equal.

cc @ntdiary

melvin-bot[bot] commented 4 months ago

@dangrous @ntdiary @slafortune this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

melvin-bot[bot] commented 4 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

slafortune commented 4 months ago

@ntdiary are you able to recreate this still?

ntdiary commented 4 months ago

@ntdiary are you able to recreate this still?

I can reproduce it. And I'm still investigating further. :)

melvin-bot[bot] commented 4 months ago

@dangrous, @ntdiary, @slafortune Whoops! This issue is 2 days overdue. Let's get this updated quick!

ntdiary commented 4 months ago

Not overdue

slafortune commented 4 months ago

@ntdiary do you have an update?

ntdiary commented 4 months ago

@ntdiary do you have an update?

@slafortune, ok, so far, I think we can hold this issue for the SelectionList refactor, because I personally feel this issue is an edge case, and OptionsSelector will be replaced with SelectionList, after which this issue will disappear automatically. :)

melvin-bot[bot] commented 4 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] commented 4 months ago

@dangrous @ntdiary @slafortune this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

slafortune commented 4 months ago

AH - Thanks for the details and linking that @ntdiary ! I think since this is a pretty edge case and OptionsSelector is deprecated we don't really need to hold we can simply close this.