Cdd 3363 fe bug logout warning countdown drifts from real session when tab is inactive#986
Conversation
…ts for logout tests (communication of intent)
…ts-from-real-session-when-tab-is-inactive
…b is asleep is working fine
Unit tests coverage
|
dw-ukhsa
left a comment
dw-ukhsa
left a comment
There was a problem hiding this comment.
Looks good overall and the wall-clock approach makes sense. I've left one comment where I think the tab recovery path can still extend the session beyond timeoutMinutes.
| } | ||
| } | ||
|
|
||
| window.addEventListener('visibilitychange', onVisibilityChange) |
There was a problem hiding this comment.
Should this be on document instead of window? visibilitychnage fires on document?
There was a problem hiding this comment.
Good spot. Fixed.
Worked for me before, but probably wasn't browser-compatible.
| }, [clearTimers]) | ||
|
|
||
| // Start the countdown when the warning is triggered | ||
| const startCountdown = useCallback(() => { |
There was a problem hiding this comment.
Won't this restart the full warning period when the tab becomes visible again? E.g, with timeoutMinutes = 4 and warningMinutes = 2, if lastActivity was 00:00 and the user returns at 03:00, this calls startCountdown(), which sets logoutAtRef to 03:00 + 2 mins = 05:00. I'd expect logout to still happen at lastActivity _timeoutMinutes, i.e 04:00 with the modal showing 01:00
There was a problem hiding this comment.
Another good spot. Fixed.
startCountdown() gets called from different places, so I am passing an optional argument now to get round this.
| // jest.setSystemTime moves Date.now() forward WITHOUT firing any timers, | ||
| // just like a real browser suspending JavaScript in a hidden tab. | ||
| // The visibilitychange then simulates the user returning. | ||
| const switchAwayFromTabThenReturn = (awayInMicroSeconds: number) => { |
There was a problem hiding this comment.
This looks to be in milliseconds not microseconds. Maybe rename it to awayInMilliseconds?
There was a problem hiding this comment.
Ha ha, yes, of course.
|
2 participants
Description
Fixes #CDD-2469
Type of change
How Has This Been Tested?
Checklist: