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

Jump to bottom

Improve Chat Switch timing in Grafana so it waits for all messages to layout #2684

Merged
merged 7 commits into from
May 10, 2021
Merged

Improve Chat Switch timing in Grafana so it waits for all messages to layout #2684

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
99a972b
move to report actions view
marcaaron May 4, 2021
6ee24bd
Use the debounce time in CONST
marcaaron May 4, 2021
ba525f0
improve comment
marcaaron May 4, 2021
2f66572
Merge remote-tracking branch 'origin' into marcaaron-chatSwitchTest
marcaaron May 5, 2021
778400c
Cancel the debounce to avoid firing it after switching
marcaaron May 5, 2021
4277ea6
Fix conflict
marcaaron May 7, 2021
72d2a94
increase debounce time slightl
marcaaron May 7, 2021
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
1 change: 1 addition & 0 deletions src/CONST.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ const CONST = {
HOMEPAGE_REPORTS_LOADED: 'homepage_reports_loaded',
SWITCH_REPORT: 'switch_report',
COLD: 'cold',
REPORT_ACTION_ITEM_LAYOUT_DEBOUNCE_TIME: 1000,
Copy link
Contributor

Choose a reason for hiding this comment

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

NAB, but is it true that a slow child component taking longer than the denounce time to render would trigger a false measurement?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think if we had a child that takes more than a second to render we'd definitely get a funky measurement.

I think it's pretty unlikely for that to happen.

I came across another issue here though that I'm going to look into addressing now.

If we switch chats before thing are done rendering it's possible to get a negative value recorded so I'll look into how to prevent this before proceeding.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok figured that one out and we just need to cancel the debounced method in componentWillUnmount() so it does not incorrectly set the timer end when switching to the next chat.

So, do you think it's OK to have this... or should we perhaps increase the time?

a slow child component taking longer than the denounce time to render would trigger a false measurement

Copy link
Contributor

Choose a reason for hiding this comment

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

As unlikely as this is, I think it would be better to set a slightly higher debounce time -- just in case. But it's NAB. I'll leave it up to you 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gonna bump it to 1500

},
MESSAGES: {
// eslint-disable-next-line max-len
Expand Down
5 changes: 3 additions & 2 deletions src/libs/actions/Timing.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,11 @@ function start(eventName) {
*
* @param {String} eventName - event name used as timestamp key
* @param {String} [secondaryName] - optional secondary event name, passed to grafana
* @param {Number} [offset] - optional param to offset the time
*/
function end(eventName, secondaryName = '') {
function end(eventName, secondaryName = '', offset = 0) {
if (eventName in timestampData) {
const eventTime = Date.now() - timestampData[eventName];
const eventTime = Date.now() - timestampData[eventName] - offset;
const grafanaEventName = secondaryName
? `expensify.cash.${eventName}.${secondaryName}`
: `expensify.cash.${eventName}`;
Expand Down
5 changes: 4 additions & 1 deletion src/pages/home/report/ReportActionItem.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,9 @@ const propTypes = {

// Should we display the new indicator on top of the comment?
shouldDisplayNewIndicator: PropTypes.bool.isRequired,

// Runs when the view enclosing the chat message lays out indicating it has rendered
onLayout: PropTypes.func.isRequired,
};

const defaultProps = {
Expand Down Expand Up @@ -122,7 +125,7 @@ class ReportActionItem extends Component {
{this.props.shouldDisplayNewIndicator && (
<UnreadActionIndicator />
)}
<View style={getReportActionItemStyle(hovered)}>
<View style={getReportActionItemStyle(hovered)} onLayout={this.props.onLayout}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh nice, didn't know that View has an onLayout prop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://reactnative.dev/docs/view#onlayout

The docs do say

This event is fired immediately once the layout has been calculated, but the new layout may not yet be reflected on the screen at the time the event is received, especially if a layout animation is in progress.

I think this is maybe as close as we can get to treating an individual chat message as "rendered". There might be a better indicator to use.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey I thought that as well, I don't mean to get you sidetracked but here's what I've found looking for an alternative:

viewabilityConfigCallbackPairs

List of ViewabilityConfig/onViewableItemsChanged pairs. A specific onViewableItemsChanged will be called when its corresponding ViewabilityConfig's conditions are met. See ViewabilityHelper.js for flow type and further documentation.

And in ViewabilityHelper.js

Minimum amount of time (in milliseconds) that an item must be physically viewable before the
viewability callback will be fired.

I've never used this but it seems you can configure this and wait for the callback to get called for all the items or just the first 5-10 (Thought what about when there are less than 10...)

Copy link
Contributor

Choose a reason for hiding this comment

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

Anyway I think onLayout is close enough maybe 1-2 frames before the item is rendered so that like 16-32ms which is probably too little to care about

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, that's interesting! The docs are kind of sparse on that stuff so I'm not too sure if I'll go down this road right now. I think it will maybe be slightly more complicated to use this then to do the naive idea I had.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mostly because onViewableItemsChanged is called not just when the items first appear, but when things scroll etc. And I don't want to think about it 😄

{!this.props.displayAsGroup
? (
<ReportActionItemSingle action={this.props.action}>
Expand Down
20 changes: 19 additions & 1 deletion src/pages/home/report/ReportActionsView.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,10 @@ class ReportActionsView extends React.Component {
this.onVisibilityChange = this.onVisibilityChange.bind(this);
this.loadMoreChats = this.loadMoreChats.bind(this);
this.startRecordMaxActionTimer = this.startRecordMaxActionTimer.bind(this);
this.recordTimeToMeasureItemLayout = _.debounce(
this.recordTimeToMeasureItemLayout.bind(this),
CONST.TIMING.REPORT_ACTION_ITEM_LAYOUT_DEBOUNCE_TIME,
);
this.sortedReportActions = [];
this.timers = [];

Expand Down Expand Up @@ -115,7 +119,6 @@ class ReportActionsView extends React.Component {
setNewMarkerPosition(this.props.reportID, oldestUnreadSequenceNumber);

fetchActions(this.props.reportID);
Timing.end(CONST.TIMING.SWITCH_REPORT, CONST.TIMING.COLD);
}

shouldComponentUpdate(nextProps, nextState) {
Expand Down Expand Up @@ -189,6 +192,10 @@ class ReportActionsView extends React.Component {
this.keyboardEvent.remove();
}

// We must cancel the debounce function so that we do not call the function when switching to a new chat before
// the previous one has finished loading completely.
this.recordTimeToMeasureItemLayout.cancel();

AppState.removeEventListener('change', this.onVisibilityChange);

_.each(this.timers, timer => clearTimeout(timer));
Expand Down Expand Up @@ -304,6 +311,16 @@ class ReportActionsView extends React.Component {
updateLastReadActionID(this.props.reportID);
}

/**
* Runs each time a ReportActionItem is laid out. This method is debounced so we wait until the component has
* finished laying out items before recording the chat as switched.
*/
recordTimeToMeasureItemLayout() {
// We are offsetting the time measurement here so that we can subtract our debounce time from the initial time
// and get the actual time it took to load the report
Timing.end(CONST.TIMING.SWITCH_REPORT, CONST.TIMING.COLD, CONST.TIMING.REPORT_ACTION_ITEM_LAYOUT_DEBOUNCE_TIME);
}

/**
* This function overrides the CellRendererComponent (defaults to a plain View), giving each ReportActionItem a
* higher z-index than the one below it. This prevents issues where the ReportActionContextMenu overlapping between
Expand Down Expand Up @@ -350,6 +367,7 @@ class ReportActionsView extends React.Component {
isMostRecentIOUReportAction={item.action.sequenceNumber === this.mostRecentIOUReportSequenceNumber}
iouReportID={this.props.report.iouReportID}
hasOutstandingIOU={this.props.report.hasOutstandingIOU}
onLayout={this.recordTimeToMeasureItemLayout}
/>
);
}
Expand Down