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.

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

fix(app): Run timer stays at 00:00:00 if you reconnect in the middle of a delay #7841

Closed
wants to merge 11 commits into from
37 changes: 34 additions & 3 deletions app/src/redux/robot/reducer/session.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,18 @@ export type SessionState = {
remoteTimeCompensation: number | null,
startTime: ?number,
runTime: number,
/**
* Used to calculate the `pausedDuration`
* todo: hacky in that it is managed locally. Not part of the start-time fix,
* but partially fixes issue of pausing without pausing the timer. Keep?
*/
pausedTime: number,
/**
* Updated after cycle of pause -> resume.
* todo: hacky in that it is managed locally. Not part of the start-time fix,
* but partially fixes issue of pausing without pausing the timer. Keep?
*/
pausedDuration: number,
SyntaxColoring marked this conversation as resolved.
Show resolved Hide resolved
apiLevel: [number, number] | null,
capabilities: Array<string>,
}
Expand Down Expand Up @@ -104,6 +116,8 @@ const INITIAL_STATE: SessionState = {
remoteTimeCompensation: null,
startTime: null,
runTime: 0,
pausedTime: 0,
pausedDuration: 0,
apiLevel: null,
}

Expand Down Expand Up @@ -222,6 +236,7 @@ function handleSessionUpdate(
function handleSessionInProgress(state: SessionState): SessionState {
return {
...state,
pausedDuration: 0,
runTime: 0,
startTime: null,
remoteTimeCompensation: null,
Expand Down Expand Up @@ -262,7 +277,11 @@ function handleInvalidFile(
}

function handleRun(state: SessionState, action: any): SessionState {
return { ...state, runTime: 0, runRequest: { inProgress: true, error: null } }
return {
...state,
runTime: Date.now(),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think calls to anything non-idempotent should be strictly avoided in Redux reducers. Which makes it very embarrassing to realize that handleTickRunTime below has been sitting there for 4 years with a call to Date.now() that I definitely wrote...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, don't be too hard on yourself. It's probably one of my edits :)

runRequest: { inProgress: true, error: null },
}
}

function handleRunResponse(state: SessionState, action: any): SessionState {
Expand All @@ -280,7 +299,12 @@ function handleTickRunTime(state: SessionState, action: any): SessionState {
}

function handlePause(state: SessionState, action: any): SessionState {
return { ...state, pauseRequest: { inProgress: true, error: null } }
return {
...state,
pausedTime: Date.now(),
pauseRequest: { inProgress: true, error: null },
runTime: Date.now(),
}
}

function handlePauseResponse(state: SessionState, action: any): SessionState {
Expand All @@ -294,7 +318,14 @@ function handlePauseResponse(state: SessionState, action: any): SessionState {
}

function handleResume(state: SessionState, action: any): SessionState {
return { ...state, resumeRequest: { inProgress: true, error: null } }
const pausedDuration = state.pausedTime ? Date.now() - state.pausedTime : 0
return {
...state,
pausedDuration: state.pausedDuration + pausedDuration,
pausedTime: 0,
resumeRequest: { inProgress: true, error: null },
runTime: Date.now(),
}
}

function handleResumeResponse(state: SessionState, action: any): SessionState {
Expand Down
25 changes: 12 additions & 13 deletions app/src/redux/robot/selectors.js
Original file line number Diff line number Diff line change
Expand Up @@ -203,12 +203,13 @@ export const getSessionError: State => string | null = createSelector(

const getStartTimeMs = (state: State): number | null => {
const { startTime, remoteTimeCompensation } = session(state)

if (startTime == null || remoteTimeCompensation === null) {
if (startTime == null) {
return null
}

return startTime + remoteTimeCompensation
// [7740] ce: 5/26/2021 - upon reload `remoteTimeCompensation` is null.
// will look to see why, but in the meantime this solves the start-time
// issue on reload but may create other issues?
return startTime + remoteTimeCompensation || 0
}

export const getStartTime: (state: State) => string | null = createSelector(
Expand All @@ -218,15 +219,13 @@ export const getStartTime: (state: State) => string | null = createSelector(
}
)

export const getRunSeconds: State => number = createSelector(
getStartTimeMs,
(state: State) => session(state).runTime,
(startTime: ?number, runTime: ?number): number => {
return runTime && startTime && runTime > startTime
? Math.floor((runTime - startTime) / 1000)
: 0
}
)
export function getRunSeconds(state: State): number {
const startTime = getStartTimeMs(state)
const { runTime, pausedDuration } = session(state)
return runTime && startTime
? Math.max(0, Math.floor((runTime - startTime - pausedDuration) / 1000))
: 0
}

export function formatSeconds(runSeconds: number): string {
const hours = padStart(`${Math.floor(runSeconds / 3600)}`, 2, '0')
Expand Down
40 changes: 27 additions & 13 deletions app/src/redux/robot/test/selectors.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,7 @@ describe('robot selectors', () => {
const state = {
robot: {
session: {
pausedDuration: 0,
startTime: null,
runTime: 42,
},
Expand All @@ -227,28 +228,41 @@ describe('robot selectors', () => {
expect(getRunTime(state)).toEqual('00:00:00')
})

it('getRunTime with no remoteTimeCompensation', () => {
const state = {
robot: {
session: {
remoteTimeCompensation: null,
startTime: 40,
runTime: 42,
it('getRunTime without remoteTimeCompensation', () => {
const testGetRunTime = (seconds, expected) => {
const stateWithRunTime = {
robot: {
session: {
pausedDuration: 0,
remoteTimeCompensation: null,
startTime: 40,
runTime: 42 + 1000 * seconds,
},
},
},
}

expect(getRunTime(stateWithRunTime)).toEqual(expected)
}

expect(getRunTime(state)).toEqual('00:00:00')
testGetRunTime(0, '00:00:00')
testGetRunTime(1, '00:00:01')
testGetRunTime(59, '00:00:59')
testGetRunTime(60, '00:01:00')
testGetRunTime(61, '00:01:01')
testGetRunTime(3599, '00:59:59')
testGetRunTime(3600, '01:00:00')
testGetRunTime(3601, '01:00:01')
})

it('getRunTime', () => {
it('getRunTime with remoteTimeCompensation', () => {
const testGetRunTime = (seconds, expected) => {
const stateWithRunTime = {
robot: {
session: {
remoteTimeCompensation: 0,
startTime: 42,
runTime: 42 + 1000 * seconds,
pausedDuration: 0,
remoteTimeCompensation: 1000,
startTime: 40,
runTime: 1042 + 1000 * seconds,
},
},
}
Expand Down
13 changes: 11 additions & 2 deletions app/src/redux/robot/test/session-reducer.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ describe('robot reducer - session', () => {
startTime: null,
runTime: 0,
apiLevel: null,
pausedDuration: 0,
pausedTime: 0,
})
})

Expand Down Expand Up @@ -113,6 +115,7 @@ describe('robot reducer - session', () => {
remoteTimeCompensation: null,
startTime: null,
runTime: 0,
pausedDuration: 0,
})
})

Expand Down Expand Up @@ -198,15 +201,14 @@ describe('robot reducer - session', () => {
it('handles RUN action', () => {
const state = {
session: {
runTime: now,
runRequest: { inProgress: false, error: new Error('AH') },
},
}
const action = { type: actionTypes.RUN }

expect(reducer(state, action).session).toEqual({
runRequest: { inProgress: true, error: null },
runTime: 0,
runTime: now,
})
})

Expand Down Expand Up @@ -248,7 +250,9 @@ describe('robot reducer - session', () => {
const action = { type: actionTypes.PAUSE }

expect(reducer(state, action).session).toEqual({
pausedTime: now,
pauseRequest: { inProgress: true, error: null },
runTime: now,
})
})

Expand Down Expand Up @@ -281,13 +285,18 @@ describe('robot reducer - session', () => {
it('handles RESUME action', () => {
const state = {
session: {
pausedDuration: 0,
pausedTime: now - 1000,
resumeRequest: { inProgress: false, error: new Error('AH') },
},
}
const action = { type: actionTypes.RESUME }

expect(reducer(state, action).session).toEqual({
pausedDuration: 1000,
pausedTime: 0,
resumeRequest: { inProgress: true, error: null },
runTime: now,
})
})

Expand Down