Skip to content
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,8 @@ function FloatingActionButtonAndPopover(
showCreateMenu();
}
};
// eslint-disable-next-line react-hooks/exhaustive-deps
const selfDMReportID = useMemo(() => ReportUtils.findSelfDMReportID(), [isLoading]);
Copy link
Contributor

@hungvu193 hungvu193 Apr 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This useMemo cause the issue I mentioned.

selfDMReportID here 's always undefined, even I logout 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you test the PR with high traffic account? @nkdengineer

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just tested with the account I used to test other issues. What is the high-traffic account?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, you should test your PR with a high traffic account, it's account with ton of reports, check it out here:
https://github.com/Expensify/App/blob/68e69623a8bca0dce5f384fa351d5cbb2214aeb4/contributingGuides/CONTRIBUTING.md#high-traffic-accounts

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hungvu193 I just tested it in high traffic account and it works well


return (
<View style={styles.flexGrow1}>
Expand All @@ -274,7 +276,7 @@ function FloatingActionButtonAndPopover(
text: translate('sidebarScreen.fabNewChat'),
onSelected: () => interceptAnonymousUser(Report.startNewChat),
},
...(canUseTrackExpense
...(canUseTrackExpense && selfDMReportID
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how did you record the video in the checklist author but this line completely broke the current logic, it will make the Track expense button disappear if it's not selfDMReportID 😱

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tbh, I prefer disable that button while report is loading, because even when you can go to next page when report is loading, you still can't get the personal detail 🤔 , yeah I'm talking about slow 3g case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screen.Recording.2024-04-19.at.08.16.46.mov

Copy link
Contributor Author

@nkdengineer nkdengineer Apr 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will make the Track expense button disappear if it's not selfDMReportID

You mean in case the selfDMReportID is undefined, the Track expense button will disappear, right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah

Copy link
Contributor

@hungvu193 hungvu193 Apr 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is BE bug, revert the change make it worked as expected

Copy link
Contributor

@hungvu193 hungvu193 Apr 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I see the problem now. For some accounts, ie my high traffic account, selfDMReport is not added to our allReports, I tried both reseting cache and logging out but no luck.

Copy link
Contributor

@hungvu193 hungvu193 Apr 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah there definitely is a BE component to it - however I think we should also handle it better in the FE.

I like the idea of hiding the option

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok cool. I'll complete the checklist then

? [
{
icon: Expensicons.DocumentPlus,
Expand Down