[$250] when selecting categories, the selected categories get reset if we click outside the checkbox

[m-natarajan]

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: 9.0.36-1 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 Expensify/Expensify Issue URL: Issue reported by: @rushatgabhane Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1725906909191599

Action Performed:

1. Enable categories on a controlled workspace
2. Click More features > Categories > Click check box to select all categories
3. Click on any row to select individual category

   Expected Result:

   RHP opened for selected category and other category remain selected

   Actual Result:

   RHP opened for the category and all other categories are reset (Unchecked)

   Workaround:

   Unknown

   Platforms:

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

   • [ ] Android: Native
   • [ ] Android: mWeb Chrome
   • [ ] iOS: Native
   • [ ] iOS: mWeb Safari
   • [x] MacOS: Chrome / Safari
   • [ ] MacOS: Desktop

Screenshots/Videos

https://github.com/user-attachments/assets/c70804b4-fad5-4664-9503-65335a48dd4a

https://github.com/user-attachments/assets/28f50025-55a1-4324-a1c6-5318a074ad3f

Add any screenshot/video evidence

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021835867544323146964
• Upwork Job ID: 1835867544323146964
• Last Price Increase: 2024-09-17

Issue Owner

Current Issue Owner: @

Issue Owner

Current Issue Owner: @rushatgabhane

[truph01]

@rushatgabhane Can you help re-review my proposal?

[MonilBhavsar]

@rushatgabhane if you could please take a look when you get a chance

[melvin-bot[bot]]

@rushatgabhane, @jliexpensify, @MonilBhavsar Huh... This is 4 days overdue. Who can take care of this?

[jliexpensify]

Bumping @rushatgabhane here!

[melvin-bot[bot]]

@rushatgabhane, @jliexpensify, @MonilBhavsar 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

[jliexpensify]

Am going to DM @rushatgabhane on Slack

[rushatgabhane]

hello! i still think that we should assign @ChavdaSachin because their proposal was first (and meets all the requirements)

[truph01]

@rushatgabhane That solution does not fix the case we click on "import spreadsheet" on other pages such as the categories, tags pages

https://github.com/user-attachments/assets/1bd6cf89-b641-4bf0-8c58-f100ed836072

[rushatgabhane]

@truph01 could you please add steps and what you expect to happen, so that we're on the same page

[ChavdaSachin]

I have already provided solution for the same here cc. @rushatgabhane

[rushatgabhane]

i feel that we should just go with @ChavdaSachin's proposal and any minor issues can be ironed out in the PR

[truph01]

In the future, If we introduce a new component like "Import spreadsheet" button (click on it will navigate to other screen), the bug will appear again.

[truph01]

i feel that we should just go with @ChavdaSachin's proposal and any minor issues can be ironed out in the PR

Also, do you have any feedback about my proposal? So I can improve it in other issues. Thanks

[rushatgabhane]

Also, do you have any feedback about my proposal? So I can improve it in other issues. Thanks

@truph01

@ChavdaSachin pointed out an issue in your proposal here. So I felt it did not meet the requirements.

selections should not clear after taping raw selection should be cleared upon visiting other pages like Tags, Workflow, etc... and coming back

[truph01]

@rushatgabhane I`ve updated my proposal via comment here to match that requirement.

selections should not clear after taping raw selection should be cleared upon visiting other pages like Tags, Workflow, etc... and coming back

Maybe you miss that comment.

[rushatgabhane]

i know, you have updated but @ChavdaSachin was the one to point it and fix it. So I consider their proposal as the one that came first.

[truph01]

Ah, I’m a newbie, and I think we should consider the advantages each solution brings to the app, in addition to the order of the proposals.

However, it seems we tend to choose the first solution that resolves the issue, even if the second one is more general and robust. I'll keep that in mind for future proposals. Thanks!

[ChavdaSachin]

Pr Draft is ready, I have checked functionality thoroughly, and will upload videos in few hours. cc. @rushatgabhane @MonilBhavsar

[ChavdaSachin]

PR ready for review. cc. @rushatgabhane @MonilBhavsar

[MonilBhavsar]

@ChavdaSachin please wait until the assignment

[truph01]

@MonilBhavsar FYI, C+ has chosen proposal based on comment. Just wait for your final review.

If possible, please take a look at my proposal as well. FYI, both proposals are nearly the same, except for how they handle the following:

selections should not clear after taping raw selection should be cleared upon visiting other pages like Tags, Workflow, etc... and coming back

[MonilBhavsar]

Yep, a long discussion in this issue https://github.com/Expensify/App/pull/40136 And, we concluded that selection should be cleared when navigated away

[ChavdaSachin]

Thanks for linking Monil

[MonilBhavsar]

@truph01 Linking a POC or test branch would have been helpful to verify such proposals

[truph01]

@MonilBhavsar Thanks. The test branch for the solution here: https://github.com/truph01/App/tree/fix/49322.

[ChavdaSachin]

@MonilBhavsar 2 more input from my side.

1. The new hook suggested by truph01 would break on WorkspaceMembersPage, WorkspaceReoprtFieldsPage, PolicyDestanceRate, and WorkspaceTaxesPage. These pages store selected item list as array while the new hook only covers the case if list is stored as object.
2. If we still decide to fine tune @truph01's solution and move ahead then I recommend to create handle functions for onSelectRaw, rather than modifying navigation function. IMO a navigation function should not have any other functionality rather than navigation.

[truph01]

These pages store selected item list as array while the new hook only covers the case if list is stored as object

• Thanks. It is easy to address in PR stage. Instead of passing the set function such as:

const useCleanupSelectedOptions = (setFunction: ({}: any) => void) => {

we can pass the cleanup function:

const useCleanupSelectedOptions = (cleanupFunction: () => void) => {

rather than modifying navigation function

• Sorry, I don't suggest to modify the navigation function in my proposal, or am I missing something?

[melvin-bot[bot]]

@rushatgabhane @jliexpensify @MonilBhavsar 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!

[jliexpensify]

@MonilBhavsar - just a heads up, I'm OOO from 3rd to 14th, but I don't think anything is needed from me during this period. Feel free to reassign to another B0 if needed though!

[ChavdaSachin]

I have found additional case of possible regression,

If we preserve selections and delete category from categorySettingsPage, selection counter on header would not update.

https://github.com/user-attachments/assets/4d8b42a9-9e86-4f85-9dfb-2562fa7f6879

So for that problem - In addition to My Proposal we need to include a new function - refreshSelection

    const refreshSelection = () => {
        const availableCategories = categoryList.filter((category) => category.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE);
        setSelectedCategories(Object.fromEntries(availableCategories.map((item) => Object.keys(selectedCategories).includes(item.keyForList) ? [item.keyForList, true]: [])));
    }

and make a function call on focus.

    useEffect(() => {
        if (isFocused || shouldPreserveSelection) {
            setShouldPreserveSelection(false);
            refreshSelection();
            return;
        }
        setSelectedCategories({});
        // eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps
    }, [isFocused]);

[MonilBhavsar]

@ChavdaSachin @truph01 Thanks so much for digging into the root cause and proposing solutions to fix this bug! After reviewing, It looks like that combining both of your solutions would get us to the complete, efficient and scalable fix. A good instance to showcase team work and work on this bug together!?

We would like to fix this issue happening for all pages and all instances as pointed by @ChavdaSachin While we also want to make it scalable by simply creating a hook and using it in the new similar page we might create in future, as proposed by @truph01

Would you be up for teaming up to work on this issue together? If so, we'll split the bounty between both of you.

Please comment here and let us know if you're down to collaborate!

[ChavdaSachin]

Alright I am in.👍

[truph01]

@MonilBhavsar I’m happy to have a team collaborate on this issue if both proposals contribute to resolving the bugs. But here is my thoughts about the bug:

"If we preserve selections and delete category from categorySettingsPage, selection counter on header would not update."

1. I don't believe this is a regression, as the bug existed before. If you try selecting 3 categories in one tab, then try to delete one of them in another device/browser, the counter doesn't update correctly as well. This happens because we don't have a mechanism in place to automatically update the header counter when a category is deleted.

2. The solution suggested here seems more like a workaround, as the useEffect is only triggered when isFocused changes. As a result, it cannot fix the case I mentioned above.

3. If we still want to fix that bug in this issue, I think we need to create a new hook so that we can use it in other pages:

import {useEffect} from 'react';
import {ListItem} from '@components/SelectionList/types';

const useAutoUdpateSelectedOptions = (listItem: ListItem[], selectedOptions: Record<string, boolean>, setSelectedOptions: React.Dispatch<React.SetStateAction<Record<string, boolean>>>) => {
    useEffect(() => {
        const availableOptions = listItem.filter((o) => !o.isDisabled).map((o) => o.text);
        const originalSelectedOptions = Object.keys(selectedOptions);
        const newSelectedOptions = originalSelectedOptions.filter((o) => availableOptions.includes(o));
        setSelectedOptions(
            newSelectedOptions.reduce((acc, key) => {
                acc[key] = true;
                return acc;
            }, {}),
        );
    }, [listItem]);
};
export default useAutoUdpateSelectedOptions;

It will auto-update the selected options once the options change.

then use it in workspace category page or other similar pages:

useAutoUdpateSelectedOptions(categoryList, selectedCategories, setSelectedCategories);

4. Since fixing that bug will expand the scope of the issue and require additional testing, I believe we should consider increasing the issue price if we still want to fix that bug, or we can create a new separate issue.

[jliexpensify]

Hi @MonilBhavsar - I'm back, anything needed from me? Also, looks like we have this comment to be addressed, especially around payment. It seems like it could be a big job so happy to consider an increase - thoughts?

[melvin-bot[bot]]

@rushatgabhane @jliexpensify @MonilBhavsar this issue is now 4 weeks old, please consider:

• Finding a contributor to fix the bug
• Closing the issue if BZ has been unable to add the issue to a VIP or Wave project
• If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

[truph01]

@MonilBhavsar What do you think about my thoughts?

[melvin-bot[bot]]

This issue has not been updated in over 15 days. @rushatgabhane, @jliexpensify, @MonilBhavsar eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[jliexpensify]

Bumping @MonilBhavsar for a review!

[truph01]

@MonilBhavsar Please help take a look at this issue once you have a chance. Thanks

[truph01]

@MonilBhavsar Please take a look at my comment when you have a chance. Thanks