Subtract idle time from time spent #43018
Conversation
apps/src/StudioApp.js
Outdated
| @@ -183,6 +183,13 @@ class StudioApp extends EventEmitter { | |||
| */ | |||
| this.milestoneStartTime = undefined; | |||
|
|
|||
| /** | |||
| * The amount of idle time when the last milestone was recorded. Used for | |||
There was a problem hiding this comment.
Maybe indicate the units here?
apps/src/redux/studioAppActivity.js
Outdated
| @@ -0,0 +1,20 @@ | |||
| const SET_IDLE_TIME = 'instructions/SET_IDLE_TIME'; | |||
There was a problem hiding this comment.
What does instructions here refer to? (Also on lines 3 and 7 below.)
There was a problem hiding this comment.
This is a copy-pasta error, thanks for catching that!
| if (this.state.idleStart) { | ||
| const now = new Date().getTime(); | ||
| const timeSpentIdle = now - this.state.idleStart; | ||
| this.props.setIdleTime(this.props.totalIdleTime + timeSpentIdle); |
There was a problem hiding this comment.
What happens if a user goes back and forth between idle and active several times within a session? Should we reset this.state.idleStart here?
There was a problem hiding this comment.
(minor) I wonder if it would be a bit clearer if the redux action was addIdleTime? If that's the case, I don't think this component needs to know about totalIdleTime at all.
There was a problem hiding this comment.
I was going back and forth about how much of this should live in redux. I think that'd be a good way to go.
There was a problem hiding this comment.
For your first question, idleStart is set in onIdle
There was a problem hiding this comment.
Oh, I see. I might still reset this.state.idleStart here as a defensive measure but up to you.
| timeout={IDLE_AFTER} | ||
| onIdle={this.onIdle} | ||
| onActive={this.onActive} | ||
| debounce={250} |
There was a problem hiding this comment.
Since we didn't pass an onAction callback, do we need to pass a debounce value?
There was a problem hiding this comment.
I misread the docs and thought this was for onActive, good catch.
| this.setState({idleStart: new Date().getTime()}); | ||
| }; | ||
|
|
||
| onActive = () => { |
There was a problem hiding this comment.
It looks like we only record idle time when we return to being active. Do we need to worry about a scenario where onActive() isn't called / gets called too late?
There was a problem hiding this comment.
Good question. I think I decided this was unlikely. The user will hit run for a milestone to be posted and the click action triggers onActive. This may be an issue if the user hovers over run and then is idle and then a while later clicks. Or is it possible that we'll record milestones on idle after some period of time (maybe on a channel-backed level)?
I can play around with some scenarios a bit more and see if there's an issue.
apps/src/StudioApp.js
Outdated
| @@ -1738,6 +1752,8 @@ StudioApp.prototype.report = function(options) { | |||
| // otherwise if we don't leave the page we are compounding the total time | |||
| this.milestoneStartTime = currentTime; | |||
|
|
|||
| this.milestoneStartTotalIdleTime = totalIdleTime; | |||
There was a problem hiding this comment.
(minor) Wondering if it's easier to dispatch an action here that resets idleTimeMs and not have to track idleTimeSinceLastMilestone. Up to you.
There was a problem hiding this comment.
@jamescodeorg I've made some changes to move all of the idle calculations into redux and just expose the amount of idle time since the last report. Let me know if this is more clear
Added react-idle-timer component to
StudioAppWrapperto be included on Studio Apps. The timer will track time spent idle, idle time will start tracking after 2 minutes of the user being idle. Idle time will then be set tostudioAppActivityredux where it can be read byStudioApp.js. InStudioApp.jswhen time spent is recorded, we will subtract the idle time since the last milestone has posted.Links
Testing story
I tested locally that the time spent idle is being recorded correctly and that the time_spent set on the user_level seemed right. I also added some unit tests. If anyone has ideas for additional tests, I'm open to ideas.
PR Checklist: