Kill `currentlyViewedReportID` #11450

marcaaron · 2022-09-29T20:55:03Z

Details

We track the currentlyViewedReportID by parsing it out of the current route in the ReportScreen and saving it in Onyx
Storing it triggers a re-render of the SidebarLinks which is kind of backwards since we are able to set the "current reportID" higher up the chain and avoid the re-render
Doesn't really make much sense because we can save the currentlyViewedReportID locally (no need to use Onyx for this)

Also cleaned up some cases where we are using an HOC to pass the navigation object down to the SidebarScreen, but this is not necessary as it exists in the drawer props.

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/230467

Tests

Web & Desktop

As User A navigate to a report between User A and User B
Verify it is highlighted as the current report
Navigate away from the chat to another chat
Send a message on the first chat from User B
Verify that User A gets a notification because the report is not in view
Navigate to the chat as User A again
Send another message as user B (important! make sure User A is focused on the chat on the device - do not navigate away at this part of the test)
Verify that no notification appears since the report is in view for User A

Mobile Web / Android App / iOS App

There are no local notifications for these platforms so we will do a simpler test here:

Put the app into focus mode
Visit a chat via Search
Verify the chat that is currently in view is in the Sidebar

Verify that no errors appear in the JS console

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

The Contributor+ will copy/paste it into a new comment and complete it after the author checklist is completed

QA Steps

As User A navigate to a report between User A and User B
Verify it is highlighted as the current report
Navigate away from the chat to another chat
Send a message on the first chat from User B
Verify that User A gets a notification because the report is not in view
Navigate to the chat as User A again
Send another message as user B (important! make sure User A is focused on the chat on the device - do not navigate away at this part of the test)
Verify that no notification appears since the report is in view for User A

Verify that no errors appear in the JS console

Screenshots

Web

2022-10-04_08-50-21.mp4

Mobile Web - Chrome

2022-10-04_10-15-57.mp4

Mobile Web - Safari

2022-10-04_10-22-22.mp4

Desktop

2022-10-04_10-02-13.mp4

iOS

2022-10-04_10-33-06.mp4

Android

2022-10-04_11-05-17.mp4

marcaaron · 2022-09-29T20:57:49Z

src/setup/index.js

@@ -22,7 +22,6 @@ export default function () {
    Onyx.init({
        keys: ONYXKEYS,
        safeEvictionKeys: [ONYXKEYS.COLLECTION.REPORT_ACTIONS],
-        keysToDisableSyncEvents: [ONYXKEYS.CURRENTLY_VIEWED_REPORTID],


Probably we don't need this feature in Onyx. I think we added it so that the reportID would stay "synced" but if we are using a local value instead of Onyx then we won't have this problem.

Great! Let's remove it

marcaaron · 2022-09-29T21:07:15Z

src/pages/home/sidebar/SidebarScreen/BaseSidebarScreen.js

@@ -112,6 +112,7 @@ class BaseSidebarScreen extends Component {
                                onAvatarClick={this.navigateToSettings}
                                isSmallScreenWidth={this.props.isSmallScreenWidth}
                                isDrawerOpen={this.props.isDrawerOpen}
+                                currentlyViewedReportID={this.props.currentlyViewedReportID}


Passing this here is enough to ensure the SidebarLinks update with the currentlyViewedReportID (and recalculate the options). That might not be obvious though so going to add a comment I think.

marcaaron · 2022-09-29T21:11:09Z

src/pages/home/ReportScreen.js

-     * Persists the currently viewed report id
-     */
-    storeCurrentlyViewedReport() {
+    fetchReportIfNeeded() {


Renamed as we are not storing the reportID anymore (didn't really like that we are resetting the hide composer thing in the same method anyway).

NAB: to me, the name should be fetchReport because the IfNeeded part is an implementation detail of the method which the outside world doesn't need to know about.

marcaaron · 2022-09-29T21:12:27Z

src/libs/actions/Report.js

@@ -1447,7 +1449,7 @@ function viewNewReportAction(reportID, action) {
    }

    // If we are currently viewing this report do not show a notification.
-    if (reportID === lastViewedReportID && Visibility.isVisible()) {
+    if (reportID === currentlyViewedReportID && Visibility.isVisible()) {


Randomly noticed while working on this, but I think this code could be improved. We only check app "visibility" but the report could be blocked by the sidebar and the app itself might still be "visible".

tgolen

I love seeing this cleaned up!

tgolen · 2022-09-29T22:11:17Z

src/libs/SidebarUtils.js

@@ -136,7 +131,7 @@ function getOrderedReportIDs() {

        const shouldFilterReportIfRead = hideReadReports && !ReportUtils.isUnread(report);
        const shouldFilterReport = shouldFilterReportIfEmpty || shouldFilterReportIfRead;
-        if (report.reportID.toString() !== currentlyViewedReportID
+        if (report.reportID.toString() !== Report.getCurrentlyViewedReportID()


Since the SidebarLinks component now has access to the reportID directly from the route, I think I'd like to pass it as a parameter to getOrderedReportIDs().

That would only leave one reference to Report.getCurrentlyViewedReportID() (the one in User.js). I'm not sure if there is any other way for User.js to access that reportID :(

I'm just thinking if there is a way to get rid of it entirely.

Let me think about that for a bit. I had a version where we were passing that more explicitly and then switched to it being better to do 2 things consistently (but less ideal) than 1 right thing and 1 wrong thing.

My solution here ended up being to pass the params + parse the reportID from the navigation state for the one case where we needed that (and weren't triggering an action from a component). Both cases are using the reportID from the route as the "source of truth" so solves it for me, but lmk if you agree.

src/pages/home/sidebar/SidebarLinks.js

marcaaron · 2022-09-30T01:50:05Z

src/libs/actions/User.js

@@ -237,7 +230,7 @@ function validateLogin(accountID, validateCode) {
        }
    }).finally(() => {
        Onyx.merge(ONYXKEYS.ACCOUNT, {isLoading: false});
-        Navigation.navigate(redirectRoute);
+        Navigation.navigate(ROUTES.HOME);


I decided to just change the behavior here as I think it makes more sense this way. If you navigate to the HOME route it will open the drawer (which is where you would have been before validating a login via the settings page - which I presume is the isLoggedIn version of this).

tgolen

I love those changes. This looks great now!

tgolen · 2022-09-30T17:55:56Z

src/libs/Navigation/Navigation.js

+    const drawerState = lodashGet(navigationRef.current.getState(), ['routes', 0, 'state']);
+    const reportRoute = lodashGet(drawerState, ['routes', 0]);
+    return lodashGet(reportRoute, ['params', 'reportID'], '');


NAB: I'm guessing you did this for ultra clarity, but I think it's best to just do a single lodashGet.

Yep that's the reason. I think it's important for people to think about where these values are coming from. Especially if we have to change up something about the navigators at some point in the future. We could use a code comment to do the same I suppose. I'm finding myself more in the camp of using variables to explain stuff vs having long winded explanations. Not sure if one approach is better than the other or if it's something we should standardize.

That's fair. I think I'm coming from mostly recently discovering that lodashGet isn't very performant, so I think we should be careful with where we use it. I'm not entirely sure how often this code is triggered to understand if it's a performance concern, but you might know that better than me.

I'm not too concerned as this will only be triggered when someone sends you a new comment. That said, I can't guarantee how someone will decide to use it in the future.

Anyways, feels like the lodashGet performance issue would be a good thing to bring up somewhere since the prevailing advice feels like it's becoming:

use lodash.get everywhere for consistency... except where it's not performant to do so

The argument for consistency is weakened by this new exception to the rule. I'm a bit concerned that people will start picking up a "lodash bad" mentality without really thinking too hard about why it should or should not be used.

Haha, that's fair. Though, I think the opposite is equally bad (ie. lodash get everywhere!) as I've seen it misused way too many times to count. Regardless, I think your use of it here is fine as-is.

danieldoglas

Code LGTM, just one question.

src/components/ReportActionItem/IOUAction.js

marcaaron · 2022-10-04T00:17:56Z

Seems like we are ready to merge this, but need a reviewer to go through the checklist.

tgolen · 2022-10-04T17:08:00Z

Here goes

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- iOS / native
- Android / native
- iOS / Safari
- Android / Chrome
- MacOS / Chrome
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product was added in all src/languages/* files
- I verified any copy / text that was added to the app is correct English and approved by marketing by tagging the marketing team on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- Any functional components have the displayName property
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

tgolen · 2022-10-04T17:10:17Z

@marcaaron While I start working on that, the PR is missing screenshots/videos for the platforms

tgolen · 2022-10-04T17:22:17Z

@marcaaron I started testing on web, and the first set of tests work fine. However, when I attempt the second set of tests and get to point 8, I'm still getting a notification (yes, I've made sure that user A still has that chat open). I tried to get a video of it... but I think SnagIt prevents notifications from appearing (the notification never appeared as I was recording it, but appeared 100% of the time when I wasn't recording). Weird, but can you see if you're able to reproduce what I'm seeing?

marcaaron · 2022-10-04T17:28:25Z

Thanks @tgolen I'll update with some screens in a bit and try to reproduce what you are seeing.

marcaaron · 2022-10-04T17:29:35Z

the PR is missing screenshots/videos for the platforms

Just to clarify - are you thinking that all PRs should have videos and screenshots or would a screenshot suffice? Unrelated to this PR I think this is a good thing to have everyone standardize on.

marcaaron · 2022-10-04T18:54:46Z

@tgolen I was able to reproduce it and there's definitely an issue 😄 The reportID is still a number so the comparison fails. I also (like you) saw that SnagIt is hiding the notification so I used this diff to tell if it is sending a notification or not.

diff --git a/src/libs/actions/Report.js b/src/libs/actions/Report.js
index 09e8c5f01..458f086c6 100644
--- a/src/libs/actions/Report.js
+++ b/src/libs/actions/Report.js
@@ -1421,8 +1421,11 @@ function viewNewReportAction(reportID, action) {

     // If we are currently viewing this report do not show a notification.
     if (reportID === Navigation.getReportIDFromRoute() && Visibility.isVisible()) {
+        console.log('@marcaaron NOT SHOWING NOTIFICATION ', {reportID, reportIDInRoute: Navigation.getReportIDFromRoute(), isVisible: Visibility.isVisible()});
         Log.info('[LOCAL_NOTIFICATION] No notification because it was a comment for the current report');
         return;
+    } else {
+        console.log('@marcaaron SHOWING NOTIFICATION ', {reportID, reportIDInRoute: Navigation.getReportIDFromRoute(), isVisible: Visibility.isVisible()});
     }

marcaaron · 2022-10-04T19:15:18Z

@tgolen's recent changes made some rough conflicts for me so I will update you guys when this PR is ready again.

marcaaron · 2022-10-04T21:12:14Z

Ok I think this is ready now for another review.

tgolen

I'll start testing now

tgolen · 2022-10-05T16:19:37Z

src/libs/actions/Report.js

@@ -1500,7 +1487,7 @@ Onyx.connect({
                return;
            }

-            viewNewReportAction(reportID, action);
+            viewNewReportAction(reportID.toString(), action);


I saw this, and immediately thought... why isn't this a string already? I see that it's being parsed as an integer from the onyx key up above. I think a better solution then is to stop parsing it as an integer in the first place. Does that work?

Ah nice catch! yep sure that will work.

tgolen · 2022-10-05T16:21:43Z

src/pages/home/ReportScreen.js

-     * Persists the currently viewed report id
-     */
-    storeCurrentlyViewedReport() {
+    fetchReportIfNeeded() {


NAB: to me, the name should be fetchReport because the IfNeeded part is an implementation detail of the method which the outside world doesn't need to know about.

tgolen · 2022-10-05T16:22:30Z

src/setup/index.js

@@ -22,7 +22,6 @@ export default function () {
    Onyx.init({
        keys: ONYXKEYS,
        safeEvictionKeys: [ONYXKEYS.COLLECTION.REPORT_ACTIONS],
-        keysToDisableSyncEvents: [ONYXKEYS.CURRENTLY_VIEWED_REPORTID],


Great! Let's remove it

tgolen · 2022-10-05T16:24:24Z

tests/unit/LHNFilterTest.js

@@ -306,15 +305,14 @@ describe('Sidebar', () => {
                        ...LHNTestUtils.getAdvancedFakeReport(...boolArr),
                        policyID: policy.policyID,
                    };
-                    const sidebarLinks = LHNTestUtils.getDefaultRenderedSidebarLinks();
+                    const sidebarLinks = LHNTestUtils.getDefaultRenderedSidebarLinks(report1.reportID.toString());


Similar thinking here, but why isn't the reportID a string in the first place? getFakeReport() should return it as a string (maybe you're just copying what was there before and I missed these on another PR).

I think you're right that we don't need the cast. But I assumed here that the toString() was necessary since it's already in the test here:

App/tests/unit/LHNFilterTest.js

Line 317 in 23ab716

[ONYXKEYS.CURRENTLY_VIEWED_REPORTID]: report1.reportID.toString(),

maybe you're just copying what was there before and I missed these on another PR

yep this

tgolen · 2022-10-05T17:47:53Z

Sorry, I got pulled into a fire this morning and still haven't been able to test this out on anything but web. I'll get to it this afternoon hopefully

marcaaron · 2022-10-05T17:53:33Z

Thanks for the extra notes @tgolen. Updated this one 🙇

tgolen

Great, thanks! I completed my testing and it all looks good 👍

danieldoglas

LGTM as well :)

melvin-bot · 2022-10-05T22:02:08Z

@marcaaron looks like this was merged without passing tests. Please add a note explaining why this was done and remove the Emergency label if this is not an emergency.

marcaaron · 2022-10-05T22:03:10Z

Checklist is checked. Unsure why failing.

OSBotify · 2022-10-05T22:06:48Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2022-10-06T00:45:11Z

🚀 Deployed to staging by @marcaaron in version: 1.2.12-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2022-10-11T23:14:37Z

🚀 Deployed to production by @AndrewGable in version: 1.2.12-4 🚀

platform	result
🤖 android 🤖	failure ❌
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	failure ❌
🕸 web 🕸	success ✅

…rentlyViewedReportID Kill `currentlyViewedReportID`

marcaaron added 2 commits September 29, 2022 10:41

Kill currentlyViewedReportID

289c62b

Clean up

51254b7

marcaaron commented Sep 29, 2022

View reviewed changes

marcaaron marked this pull request as ready for review September 29, 2022 21:12

marcaaron requested a review from a team as a code owner September 29, 2022 21:12

marcaaron self-assigned this Sep 29, 2022

melvin-bot bot requested review from danieldoglas and removed request for a team September 29, 2022 21:13

tgolen reviewed Sep 29, 2022

View reviewed changes

marcaaron added 2 commits September 29, 2022 15:12

fix conflicts

925cd48

fix some random propTypes warnings

d31836e

marcaaron commented Sep 30, 2022

View reviewed changes

marcaaron added 3 commits September 29, 2022 15:57

Fix tests

f311b23

Get reportID from route instead of saving it in the drawer

7c3ed8a

remove toString

dbb2e11

tgolen previously approved these changes Sep 30, 2022

View reviewed changes

fix conflicts

51f029e

marcaaron dismissed tgolen’s stale review via 51f029e September 30, 2022 20:03

Increase timeout and add comment

9ba6616

danieldoglas previously approved these changes Oct 3, 2022

View reviewed changes

src/components/ReportActionItem/IOUAction.js Show resolved Hide resolved

marcaaron added 2 commits October 4, 2022 08:55

Cast reportID to string so comparison succeeds

dddb605

fix conflicts

4cbffa5

fix conflicts

5dac271

marcaaron dismissed danieldoglas’s stale review via 5dac271 October 4, 2022 19:56

marcaaron requested review from danieldoglas and tgolen October 4, 2022 21:12

tgolen requested changes Oct 5, 2022

View reviewed changes

Make requested changes

0f4f8df

marcaaron mentioned this pull request Oct 5, 2022

Remove keysToDisableSyncEvents Expensify/react-native-onyx#190

Merged

tgolen approved these changes Oct 5, 2022

View reviewed changes

danieldoglas approved these changes Oct 5, 2022

View reviewed changes

marcaaron merged commit 770d605 into main Oct 5, 2022

marcaaron deleted the marcaaron-removeCurrentlyViewedReportID branch October 5, 2022 22:02

melvin-bot bot added the Emergency label Oct 5, 2022

marcaaron removed the Emergency label Oct 5, 2022

OSBotify mentioned this pull request Oct 5, 2022

Deploy Checklist: New Expensify 2022-10-05 #11623

Closed

48 tasks

marcochavezf mentioned this pull request Oct 6, 2022

Refactor ValidateLogin #11457

Merged

93 tasks

smrutiparida pushed a commit to autosave-app/App that referenced this pull request Oct 13, 2022

Merge pull request Expensify#11450 from Expensify/marcaaron-removeCur…

8c39149

…rentlyViewedReportID Kill `currentlyViewedReportID`

deetergp mentioned this pull request Nov 18, 2022

[HOLD for payment 2022-11-16] [$500] Draft messages jump around in the LHN when accessed a second time #11330

Closed

Kill currentlyViewedReportID #11450

Kill currentlyViewedReportID #11450

Conversation

marcaaron commented Sep 29, 2022 • edited Loading

Details

Fixed Issues

Tests

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Screenshots

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcaaron Sep 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgolen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgolen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danieldoglas left a comment

Choose a reason for hiding this comment

marcaaron commented Oct 4, 2022

tgolen commented Oct 4, 2022 • edited by danieldoglas Loading

tgolen commented Oct 4, 2022

tgolen commented Oct 4, 2022 • edited Loading

marcaaron commented Oct 4, 2022

marcaaron commented Oct 4, 2022

marcaaron commented Oct 4, 2022 • edited Loading

marcaaron commented Oct 4, 2022

marcaaron commented Oct 4, 2022

tgolen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgolen commented Oct 5, 2022

marcaaron commented Oct 5, 2022

tgolen left a comment

Choose a reason for hiding this comment

danieldoglas left a comment

Choose a reason for hiding this comment

melvin-bot bot commented Oct 5, 2022

marcaaron commented Oct 5, 2022

OSBotify commented Oct 5, 2022

OSBotify commented Oct 6, 2022

OSBotify commented Oct 11, 2022

Kill `currentlyViewedReportID` #11450

Kill `currentlyViewedReportID` #11450

marcaaron commented Sep 29, 2022 •

edited

Loading

marcaaron Sep 29, 2022 •

edited

Loading

tgolen commented Oct 4, 2022 •

edited by danieldoglas

Loading

tgolen commented Oct 4, 2022 •

edited

Loading

marcaaron commented Oct 4, 2022 •

edited

Loading