Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Kill currentlyViewedReportID #11450

Merged
merged 13 commits into from
Oct 5, 2022
Merged

Kill currentlyViewedReportID #11450

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
289c62b
Kill currentlyViewedReportID
marcaaron Sep 29, 2022
51254b7
Clean up
marcaaron Sep 29, 2022
925cd48
fix conflicts
marcaaron Sep 30, 2022
d31836e
fix some random propTypes warnings
marcaaron Sep 30, 2022
f311b23
Fix tests
marcaaron Sep 30, 2022
7c3ed8a
Get reportID from route instead of saving it in the drawer
marcaaron Sep 30, 2022
dbb2e11
remove toString
marcaaron Sep 30, 2022
51f029e
fix conflicts
marcaaron Sep 30, 2022
9ba6616
Increase timeout and add comment
marcaaron Sep 30, 2022
dddb605
Cast reportID to string so comparison succeeds
marcaaron Oct 4, 2022
4cbffa5
fix conflicts
marcaaron Oct 4, 2022
5dac271
fix conflicts
marcaaron Oct 4, 2022
0f4f8df
Make requested changes
marcaaron Oct 5, 2022
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
3 changes: 0 additions & 3 deletions src/ONYXKEYS.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,6 @@ export default {
// Stores current date
CURRENT_DATE: 'currentDate',

// Currently viewed reportID
CURRENTLY_VIEWED_REPORTID: 'currentlyViewedReportID',

// Credentials to authenticate the user
CREDENTIALS: 'credentials',

Expand Down
12 changes: 11 additions & 1 deletion src/libs/Navigation/AppNavigator/MainDrawerNavigator.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import FullScreenLoadingIndicator from '../../../components/FullscreenLoadingInd
import ONYXKEYS from '../../../ONYXKEYS';
import SCREENS from '../../../SCREENS';
import Permissions from '../../Permissions';
import * as Report from '../../actions/Report';

// Screens
import ReportScreen from '../../../pages/home/ReportScreen';
Expand Down Expand Up @@ -68,7 +69,16 @@ const MainDrawerNavigator = (props) => {
// This is usually needed after login/create account and re-launches
return (
<BaseDrawerNavigator
drawerContent={() => <SidebarScreen />}
drawerContent={({navigation, state}) => {
const currentlyViewedReportID = lodashGet(state, ['routes', 0, 'params', 'reportID']);
Report.updateCurrentlyViewedReportID(currentlyViewedReportID);
return (
<SidebarScreen
navigation={navigation}
currentlyViewedReportID={currentlyViewedReportID}
/>
);
}}
screens={[
{
name: SCREENS.REPORT,
Expand Down
11 changes: 3 additions & 8 deletions src/libs/SidebarUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import _ from 'underscore';
import Str from 'expensify-common/lib/str';
import lodashGet from 'lodash/get';
import ONYXKEYS from '../ONYXKEYS';
import * as Report from './actions/Report';
import * as ReportUtils from './ReportUtils';
import * as Localize from './Localize';
import CONST from '../CONST';
Expand Down Expand Up @@ -31,12 +32,6 @@ Onyx.connect({
callback: val => personalDetails = val,
});

let currentlyViewedReportID;
Onyx.connect({
key: ONYXKEYS.CURRENTLY_VIEWED_REPORTID,
callback: val => currentlyViewedReportID = val,
});

let priorityMode;
Onyx.connect({
key: ONYXKEYS.NVP_PRIORITY_MODE,
Expand Down Expand Up @@ -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()
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

&& !report.isPinned
&& !hasDraftComment
&& shouldFilterReport
Expand Down Expand Up @@ -186,7 +181,7 @@ function getOrderedReportIDs() {

// If the active report has a draft, we do not put it in the group of draft reports because we want it to maintain it's current position. Otherwise the report's position
// jumps around in the LHN and it's kind of confusing to the user to see the LHN reorder when they start typing a comment on a report.
} else if (report.hasDraft && report.reportID.toString() !== currentlyViewedReportID) {
} else if (report.hasDraft && report.reportID.toString() !== Report.getCurrentlyViewedReportID()) {
draftReportOptions.push(report);
} else {
recentReportOptions.push(report);
Expand Down
21 changes: 12 additions & 9 deletions src/libs/actions/Report.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,12 +41,7 @@ Onyx.connect({
},
});

let lastViewedReportID;
Onyx.connect({
key: ONYXKEYS.CURRENTLY_VIEWED_REPORTID,
callback: val => lastViewedReportID = val ? Number(val) : null,
});

let currentlyViewedReportID;
const allReports = {};
let conciergeChatReportID;
const typingWatchTimers = {};
Expand Down Expand Up @@ -957,7 +952,14 @@ function handleReportChanged(report) {
* @param {Number} reportID
*/
function updateCurrentlyViewedReportID(reportID) {
Onyx.merge(ONYXKEYS.CURRENTLY_VIEWED_REPORTID, String(reportID));
currentlyViewedReportID = String(reportID);
}

/**
* @returns {String}
*/
function getCurrentlyViewedReportID() {
return currentlyViewedReportID;
}

Onyx.connect({
Expand Down Expand Up @@ -1275,7 +1277,7 @@ function addPolicyReport(policy, reportName, visibility) {
);

// Onyx.set is used on the optimistic data so that it is present before navigating to the workspace room. With Onyx.merge the workspace room reportID is not present when
// storeCurrentlyViewedReport is called on the ReportScreen, so fetchChatReportsByIDs is called which is unnecessary since the optimistic data will be stored in Onyx.
// fetchReportIfNeeded is called on the ReportScreen, so fetchChatReportsByIDs is called which is unnecessary since the optimistic data will be stored in Onyx.
// If there was an error creating the room, then fetchChatReportsByIDs throws an error and the user is navigated away from the report instead of showing the RBR error message.
// Therefore, Onyx.set is used instead of Onyx.merge.
const optimisticData = [
Expand Down Expand Up @@ -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()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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".

Log.info('[LOCAL_NOTIFICATION] No notification because it was a comment for the current report');
return;
}
Expand Down Expand Up @@ -1557,4 +1559,5 @@ export {
updatePolicyRoomName,
clearPolicyRoomNameErrors,
clearIOUError,
getCurrentlyViewedReportID,
};
9 changes: 2 additions & 7 deletions src/libs/actions/User.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import getSkinToneEmojiFromIndex from '../../components/EmojiPicker/getSkinToneE
import * as SequentialQueue from '../Network/SequentialQueue';
import PusherUtils from '../PusherUtils';
import DateUtils from '../DateUtils';
import * as Report from './Report';

let sessionAuthToken = '';
let currentUserAccountID = '';
Expand All @@ -30,12 +31,6 @@ Onyx.connect({
},
});

let currentlyViewedReportID = '';
Onyx.connect({
key: ONYXKEYS.CURRENTLY_VIEWED_REPORTID,
callback: val => currentlyViewedReportID = val || '',
});

/**
* Changes a password for a given account
*
Expand Down Expand Up @@ -215,7 +210,7 @@ function setSecondaryLoginAndNavigate(login, password) {
*/
function validateLogin(accountID, validateCode) {
const isLoggedIn = !_.isEmpty(sessionAuthToken);
const redirectRoute = isLoggedIn ? ROUTES.getReportRoute(currentlyViewedReportID) : ROUTES.HOME;
const redirectRoute = isLoggedIn ? ROUTES.getReportRoute(Report.getCurrentlyViewedReportID()) : ROUTES.HOME;
Onyx.merge(ONYXKEYS.ACCOUNT, {...CONST.DEFAULT_ACCOUNT_DATA, isLoading: true});

DeprecatedAPI.ValidateEmail({
Expand Down
16 changes: 6 additions & 10 deletions src/pages/home/ReportScreen.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,15 +132,18 @@ class ReportScreen extends React.Component {
}

componentDidMount() {
this.storeCurrentlyViewedReport();
this.fetchReportIfNeeded();
toggleReportActionComposeView(true);
this.removeViewportResizeListener = addViewportResizeListener(this.updateViewportOffsetTop);
}

componentDidUpdate(prevProps) {
if (this.props.route.params.reportID === prevProps.route.params.reportID) {
return;
}
this.storeCurrentlyViewedReport();

this.fetchReportIfNeeded();
toggleReportActionComposeView(true);
}

componentWillUnmount() {
Expand Down Expand Up @@ -170,20 +173,13 @@ class ReportScreen extends React.Component {
return !getReportID(this.props.route) || isLoadingInitialReportActions || !this.props.report.reportID;
}

/**
* Persists the currently viewed report id
*/
storeCurrentlyViewedReport() {
fetchReportIfNeeded() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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).

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

const reportIDFromPath = getReportID(this.props.route);
if (_.isNaN(reportIDFromPath)) {
Report.handleInaccessibleReport();
return;
}

// Always reset the state of the composer view when the current reportID changes
toggleReportActionComposeView(true);
Report.updateCurrentlyViewedReportID(reportIDFromPath);

// It possible that we may not have the report object yet in Onyx yet e.g. we navigated to a URL for an accessible report that
// is not stored locally yet. If props.report.reportID exists, then the report has been stored locally and nothing more needs to be done.
// If it doesn't exist, then we fetch the report from the API.
Expand Down
5 changes: 1 addition & 4 deletions src/pages/home/sidebar/SidebarLinks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ const propTypes = {
avatar: PropTypes.string,
}),

/** Currently viewed reportID */
/** Currently viewed reportID. This value is parsed from the react navigation state object */
currentlyViewedReportID: PropTypes.string,

/** Whether we are viewing below the responsive breakpoint */
Expand Down Expand Up @@ -185,9 +185,6 @@ export default compose(
personalDetails: {
key: ONYXKEYS.PERSONAL_DETAILS,
},
currentlyViewedReportID: {
key: ONYXKEYS.CURRENTLY_VIEWED_REPORTID,
},
priorityMode: {
key: ONYXKEYS.NVP_PRIORITY_MODE,
},
Expand Down
1 change: 1 addition & 0 deletions src/pages/home/sidebar/SidebarScreen/BaseSidebarScreen.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ class BaseSidebarScreen extends Component {
onAvatarClick={this.navigateToSettings}
isSmallScreenWidth={this.props.isSmallScreenWidth}
isDrawerOpen={this.props.isDrawerOpen}
currentlyViewedReportID={this.props.currentlyViewedReportID}
Copy link
Contributor Author

@marcaaron marcaaron Sep 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

/>
<FAB
accessibilityLabel={this.props.translate('sidebarScreen.fabNewChat')}
Expand Down
2 changes: 0 additions & 2 deletions src/pages/home/sidebar/SidebarScreen/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import withLocalize from '../../../../components/withLocalize';
import ONYXKEYS from '../../../../ONYXKEYS';
import {sidebarPropTypes, sidebarDefaultProps} from './sidebarPropTypes';
import BaseSidebarScreen from './BaseSidebarScreen';
import withNavigation from '../../../../components/withNavigation';

const SidebarScreen = (props) => {
let baseSidebarScreen = null;
Expand Down Expand Up @@ -40,7 +39,6 @@ SidebarScreen.defaultProps = sidebarDefaultProps;
SidebarScreen.displayName = 'SidebarScreen';

export default compose(
withNavigation,
withLocalize,
withWindowDimensions,
withOnyx({
Expand Down
2 changes: 0 additions & 2 deletions src/pages/home/sidebar/SidebarScreen/index.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import withLocalize from '../../../../components/withLocalize';
import ONYXKEYS from '../../../../ONYXKEYS';
import {sidebarPropTypes, sidebarDefaultProps} from './sidebarPropTypes';
import BaseSidebarScreen from './BaseSidebarScreen';
import withNavigation from '../../../../components/withNavigation';

// eslint-disable-next-line react/jsx-props-no-spreading
const SidebarScreen = props => <BaseSidebarScreen {...props} />;
Expand All @@ -16,7 +15,6 @@ SidebarScreen.defaultProps = sidebarDefaultProps;
SidebarScreen.displayName = 'SidebarScreen';

export default compose(
withNavigation,
withLocalize,
withWindowDimensions,
withOnyx({
Expand Down
1 change: 0 additions & 1 deletion src/setup/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ export default function () {
Onyx.init({
keys: ONYXKEYS,
safeEvictionKeys: [ONYXKEYS.COLLECTION.REPORT_ACTIONS],
keysToDisableSyncEvents: [ONYXKEYS.CURRENTLY_VIEWED_REPORTID],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Great! Let's remove it

captureMetrics: Metrics.canCaptureOnyxMetrics(),
initialKeyStates: {

Expand Down
Loading