[$250] LHN - Two different chats can be opened at the same time #56947
Comments
|
Triggered auto assignment to @anmurali ( |
|
🚨 Edited by proposal-police: This proposal was edited at 2025-03-02 03:57:54 UTC. ProposalPlease re-state the problem that we are trying to solve in this issue.Two different chats are opened, when the user taps on any conversation and taps on a different one, before the first one is displayed What is the root cause of that problem?In narrow layout, when clicking on any report, we call: when the navigation is still in progress, if user clicks on any other report in LHN, it also navigates to that report. As a result, two chats is opened in stack. What changes do you think we should make in order to solve the problem?We can utilize a debounce function to prevent handling multiple rapid clicks by ignoring subsequent clicks within the debounce period: const showReportPageDebounce = useMemo(() => {
if (shouldUseNarrowLayout) {
return debounce(showReportPage, 1000, {leading: true, trailing: false});
}
return showReportPage;
}, [showReportPage, shouldUseNarrowLayout]);Ensure this debounced function is used at: Can use existed What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?Render the LHN, trigger two consecutive click events in quick succession, and verify that only a single report screen is added to the navigation stack. What alternative solutions did you explore? (Optional)We need to implement a mechanism to prevent users from opening more than one report simultaneously.: const isNavigatingRef = useRef(false);
const isFocused = useIsFocused();
const prevIsFocused = usePrevious(isFocused);
useEffect(() => {
if (isFocused && !prevIsFocused && isNavigatingRef.current) {
isNavigatingRef.current = false;
}
}, [isFocused, prevIsFocused]);Finally, in here add: if (isNavigatingRef.current && shouldUseNarrowLayout) {
return;
}
isNavigatingRef.current = true |
ProposalPlease re-state the problem that we are trying to solve in this issue.
What is the root cause of that problem?
What changes do you think we should make in order to solve the problem?
onSelectRow={singleExecution(onSelectRow)}What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?N/A - Navigation issue What alternative solutions did you explore? (Optional)ResultMonosnap.screencast.2025-02-18.17-48-51.mp4 |
|
@anmurali Uh oh! This issue is overdue by 2 days. Don't forget to update your issues! |
|
@anmurali 6 days overdue. This is scarier than being forced to listen to Vogon poetry! |
|
Triggered auto assignment to @slafortune ( |
|
Job added to Upwork: https://www.upwork.com/jobs/~021895183258683508433 |
|
Triggered auto assignment to Contributor-plus team member for initial proposal review - @jayeshmangwani ( |
|
🚨 Edited by proposal-police: This proposal was edited at 2025-03-02 00:46:23 UTC. ProposalPlease re-state the problem that we are trying to solve in this issue.Two different chats can be opened at the same time What is the root cause of that problem?Since we selected two reports at the same time and did not implement any prevention, clicking on two reports will execute two navigations What changes do you think we should make in order to solve the problem?To resolve this issue, we should prevent multiple selections and allow only one report to be selected. Additionally, navigation should only occur when the active report is 'Home'. App/src/pages/home/sidebar/SidebarLinks.tsx Lines 64 to 77 in fbc01be update to: const showReportPage = useCallback(
(option: Report) => {
// Prevent opening Report page when clicking LHN row quickly after clicking FAB icon
// or when clicking the active LHN row on large screens
// or when continuously clicking different LHNs, only apply to small screen
// since getTopmostReportId always returns on other devices
const reportActionID = Navigation.getTopmostReportActionId();
if ((option.reportID === Navigation.getTopmostReportId() && !reportActionID) || (shouldUseNarrowLayout && isActiveReport(option.reportID) && !reportActionID)) {
return;
}
if (Navigation.getActiveRoute() !== '/home' && shouldUseNarrowLayout) { // add this
return;
}// add this
return; // add this
} // add this
Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(option.reportID));
},
[shouldUseNarrowLayout, isActiveReport],
);Screen.Recording.2025-03-02.at.00.06.30.mp4What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?None What alternative solutions did you explore? (Optional)Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job. |
|
@Krishna2323 Have you tested the Suggestion: Please always add a reason behind N/A for automated tests. |
|
@daledah I don’t entirely agree with using debounce here. I tested the alternative proposal you suggested, but it doesn’t seem to work for me. We can still navigate to two different chats in this case—adding the video below. navigation-bug-perisist.mov |
|
@jayeshmangwani Sorry, I forgot to include the detailed code changes in my initial proposal. I've now updated my alternative proposal with the necessary details. Can you test it again, thanks? |
I suggested using debounce because it is a common technique for handling rapid, successive event triggers within a short timeframe. By delaying the execution of the event handler until after a specified period of inactivity, debounce helps prevent unnecessary re-renders, redundant API calls, or in this case is the excessive computations that could degrade performance. |
Yes, it may be a common technique, but I feel that we should use it only when needed. If we have a better option than setTimeout, that should always be implemented. |
Yes, I can confirm that it would work. |
|
@huult You're suggesting using |
|
yes, @jayeshmangwani , I will update the proposal to use |
Proposal updated
@jayeshmangwani Could you check it again? |
|
Triggered auto assignment to @youssef-lr, see https://stackoverflow.com/c/expensify/questions/7972 for more details. |
|
@youssef-lr, whenever you get a chance, please check this comment for the selected proposal. Thanks!🙏 |
|
📣 @huult 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app! Offer link |
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.99-0
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: N
If this was caught during regression testing, add the test name, ID and link from TestRail: N/A
Issue reported by: Applause - Internal Team
Action Performed:
Prerequisite: Account should have at least two different conversations on LHN
Expected Result:
Only one conversation should be opened, despite tapping on a different one, before it opens
Actual Result:
Two different chats are opened, when the user taps on any conversation and taps on a different one, before the first one is displayed
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Add any screenshot/video evidence
Bug6745766_1739801510828.Chats.mp4
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @jayeshmangwaniThe text was updated successfully, but these errors were encountered: