From d36c5a3b0419eccacc494e3a5d9115f3ad30427b Mon Sep 17 00:00:00 2001 From: Rayane Djouah <77965000+rayane-djouah@users.noreply.github.com> Date: Mon, 11 Sep 2023 15:31:54 +0000 Subject: [PATCH 1/4] fix Conversation history does not scroll down after sending a message and IOU --- src/pages/home/report/ReportActionsList.js | 54 +++++++++++++++++++--- 1 file changed, 48 insertions(+), 6 deletions(-) diff --git a/src/pages/home/report/ReportActionsList.js b/src/pages/home/report/ReportActionsList.js index 7f897ee825fb..f840e1d2c7a3 100644 --- a/src/pages/home/report/ReportActionsList.js +++ b/src/pages/home/report/ReportActionsList.js @@ -176,6 +176,54 @@ function ReportActionsList({ // eslint-disable-next-line react-hooks/exhaustive-deps }, [report.lastReadTime]); + const scrollToBottomAndMarkReportAsRead = () => { + reportScrollManager.scrollToBottom(); + readActionSkipped.current = false; + Report.readNewestAction(report.reportID); + }; + + // In the component we are subscribing to the arrival of new actions. + // As there is the possibility that there are multiple instances of a ReportScreen + // for the same report, we only ever want one subscription to be active, as + // the subscriptions could otherwise be conflicting. + const newActionUnsubscribeMap = {}; + + useEffect(() => { + // Why are we doing this, when in the cleanup of the useEffect we are already calling the unsubscribe function? + // Answer: On web, when navigating to another report screen, the previous report screen doesn't get unmounted, + // meaning that the cleanup might not get called. When we then open a report we had open already previosuly, a new + // ReportScreen will get created. Thus, we have to cancel the earlier subscription of the previous screen, + // because the two subscriptions could conflict! + // In case we return to the previous screen (e.g. by web back navigation) the useEffect for that screen would + // fire again, as the focus has changed and will set up the subscription correctly again. + const previousSubUnsubscribe = newActionUnsubscribeMap[report.reportID]; + if (previousSubUnsubscribe) { + previousSubUnsubscribe(); + } + + // This callback is triggered when a new action arrives via Pusher and the event is emitted from Report.js. This allows us to maintain + // a single source of truth for the "new action" event instead of trying to derive that a new action has appeared from looking at props. + const unsubscribe = Report.subscribeToNewActionEvent(report.reportID, (isFromCurrentUser) => { + // If a new comment is added and it's from the current user scroll to the bottom otherwise leave the user positioned where + // they are now in the list. + if (!isFromCurrentUser) return; + scrollToBottomAndMarkReportAsRead(); + }); + const cleanup = () => { + if (unsubscribe) { + unsubscribe(); + } + Report.unsubscribeFromReportChannel(report.reportID); + }; + + newActionUnsubscribeMap[report.reportID] = cleanup; + + return () => { + cleanup(); + }; + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [report.reportID]); + /** * Show/hide the new floating message counter when user is scrolling back/forth in the history of messages. */ @@ -199,12 +247,6 @@ function ReportActionsList({ onScroll(event); }; - const scrollToBottomAndMarkReportAsRead = () => { - reportScrollManager.scrollToBottom(); - readActionSkipped.current = false; - Report.readNewestAction(report.reportID); - }; - /** * Calculates the ideal number of report actions to render in the first render, based on the screen height and on * the height of the smallest report action possible. From f1a69127fe5083c504073b1f058c134e9eaa9208 Mon Sep 17 00:00:00 2001 From: Rayane Djouah <77965000+rayane-djouah@users.noreply.github.com> Date: Mon, 11 Sep 2023 16:02:26 +0000 Subject: [PATCH 2/4] use reportScrollManager.scrollToBottom instead of scrollToBottomAndMarkReportAsRead --- src/pages/home/report/ReportActionsList.js | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/pages/home/report/ReportActionsList.js b/src/pages/home/report/ReportActionsList.js index f840e1d2c7a3..0c5827e3a6d6 100644 --- a/src/pages/home/report/ReportActionsList.js +++ b/src/pages/home/report/ReportActionsList.js @@ -176,12 +176,6 @@ function ReportActionsList({ // eslint-disable-next-line react-hooks/exhaustive-deps }, [report.lastReadTime]); - const scrollToBottomAndMarkReportAsRead = () => { - reportScrollManager.scrollToBottom(); - readActionSkipped.current = false; - Report.readNewestAction(report.reportID); - }; - // In the component we are subscribing to the arrival of new actions. // As there is the possibility that there are multiple instances of a ReportScreen // for the same report, we only ever want one subscription to be active, as @@ -207,7 +201,7 @@ function ReportActionsList({ // If a new comment is added and it's from the current user scroll to the bottom otherwise leave the user positioned where // they are now in the list. if (!isFromCurrentUser) return; - scrollToBottomAndMarkReportAsRead(); + reportScrollManager.scrollToBottom(); }); const cleanup = () => { if (unsubscribe) { @@ -222,7 +216,7 @@ function ReportActionsList({ cleanup(); }; // eslint-disable-next-line react-hooks/exhaustive-deps - }, [report.reportID]); + }, [report.reportID, reportScrollManager]); /** * Show/hide the new floating message counter when user is scrolling back/forth in the history of messages. @@ -247,6 +241,12 @@ function ReportActionsList({ onScroll(event); }; + const scrollToBottomAndMarkReportAsRead = () => { + reportScrollManager.scrollToBottom(); + readActionSkipped.current = false; + Report.readNewestAction(report.reportID); + }; + /** * Calculates the ideal number of report actions to render in the first render, based on the screen height and on * the height of the smallest report action possible. From e2f4db8bb4c5776044aabdbe0257a9cecbd1e8e0 Mon Sep 17 00:00:00 2001 From: Rayane Djouah <77965000+rayane-djouah@users.noreply.github.com> Date: Tue, 12 Sep 2023 17:34:06 +0000 Subject: [PATCH 3/4] refactor for readability --- src/pages/home/report/ReportActionsList.js | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/src/pages/home/report/ReportActionsList.js b/src/pages/home/report/ReportActionsList.js index 0c5827e3a6d6..a0480a246d4a 100644 --- a/src/pages/home/report/ReportActionsList.js +++ b/src/pages/home/report/ReportActionsList.js @@ -74,6 +74,12 @@ const MSG_VISIBLE_THRESHOLD = 250; // the useRef value gets reset when the reportID changes, so we use a global variable to keep track let prevReportID = null; +// In the component we are subscribing to the arrival of new actions. +// As there is the possibility that there are multiple instances of a ReportScreen +// for the same report, we only ever want one subscription to be active, as +// the subscriptions could otherwise be conflicting. +const newActionUnsubscribeMap = {}; + /** * Create a unique key for each action in the FlatList. * We use the reportActionID that is a string representation of a random 64-bit int, which should be @@ -176,12 +182,6 @@ function ReportActionsList({ // eslint-disable-next-line react-hooks/exhaustive-deps }, [report.lastReadTime]); - // In the component we are subscribing to the arrival of new actions. - // As there is the possibility that there are multiple instances of a ReportScreen - // for the same report, we only ever want one subscription to be active, as - // the subscriptions could otherwise be conflicting. - const newActionUnsubscribeMap = {}; - useEffect(() => { // Why are we doing this, when in the cleanup of the useEffect we are already calling the unsubscribe function? // Answer: On web, when navigating to another report screen, the previous report screen doesn't get unmounted, @@ -200,9 +200,12 @@ function ReportActionsList({ const unsubscribe = Report.subscribeToNewActionEvent(report.reportID, (isFromCurrentUser) => { // If a new comment is added and it's from the current user scroll to the bottom otherwise leave the user positioned where // they are now in the list. - if (!isFromCurrentUser) return; + if (!isFromCurrentUser) { + return; + } reportScrollManager.scrollToBottom(); }); + const cleanup = () => { if (unsubscribe) { unsubscribe(); @@ -212,9 +215,8 @@ function ReportActionsList({ newActionUnsubscribeMap[report.reportID] = cleanup; - return () => { - cleanup(); - }; + return cleanup; + // eslint-disable-next-line react-hooks/exhaustive-deps }, [report.reportID, reportScrollManager]); From 3c3b3ea16d72536436fa66b4ed377228e5460c93 Mon Sep 17 00:00:00 2001 From: Rayane Djouah <77965000+rayane-djouah@users.noreply.github.com> Date: Wed, 13 Sep 2023 20:18:37 +0000 Subject: [PATCH 4/4] remove reportScrollManager from useEffect deps array --- src/pages/home/report/ReportActionsList.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pages/home/report/ReportActionsList.js b/src/pages/home/report/ReportActionsList.js index a0480a246d4a..9daaba36dbe2 100644 --- a/src/pages/home/report/ReportActionsList.js +++ b/src/pages/home/report/ReportActionsList.js @@ -218,7 +218,7 @@ function ReportActionsList({ return cleanup; // eslint-disable-next-line react-hooks/exhaustive-deps - }, [report.reportID, reportScrollManager]); + }, [report.reportID]); /** * Show/hide the new floating message counter when user is scrolling back/forth in the history of messages.