SupremeTechnopriest / react-idle-timer

User activity timer component
https://idletimer.dev
MIT License
1.15k stars 143 forks source link

`activate()` during prompt triggers `onActive` even though `isIdle() === false` #319

Closed tgelu closed 1 year ago

tgelu commented 1 year ago

Bug information

If during the prompt timeout I call activate, onActive is called, but the internal model says I was never idle, therefore onActive should not be called.

I have both crossTab: true and syncTimers: true though I am not sure it's related.

Affected Module

Describe the bug

This test for activate in tests/useIdleTimer.test.ts should describe it best:

        it('Should not call onActive if user was in prompted state', async () => {
          props.timeout = 200
          props.onActive = jest.fn()
          props.promptTimeout = 100
          const { result } = idleTimer()
          await waitFor(() => result.current.isPrompted())
          result.current.activate()
          expect(result.current.isIdle()).toBe(false) // this passes
          expect(props.onActive).not.toHaveBeenCalled() // this fails
        })

This is probably the culprit, though it seems to be intentional.

To Reproduce

See above test

Expected behavior

activate says it calls onActive if the user was "idle". This should happen even if isPrompted() is true.

Screenshots

N/A

System Information (please complete the following information)

Irrelevant

Additional context

-

SupremeTechnopriest commented 1 year ago

Hey @tgelu Thanks for the well put together issue. This is actually by design. I will update the typescript doc. Prompted and Idle are both considered idle states. I will explain more in your other issue, but the reason onActive is called when you activate from the prompted state is so you can use your onActive handler to close your prompt:

From the docs:

export function App () {
  // Set timeout values
  const timeout = 1000 * 60 * 30
  const promptTimeout = 1000 * 30

  // Modal open state
  const [open, setOpen] = useState(false)

  // Time before idle
  const [remaining, setRemaining] = useState(0)

  const onPrompt = () => {
    // onPrompt will be called after the timeout value is reached
    // In this case 30 minutes. Here you can open your prompt. 
    // All events are disabled while the prompt is active. 
    // If the user wishes to stay active, call the `reset()` method.
    // You can get the remaining prompt time with the `getRemainingTime()` method,
    setOpen(true)
    setRemaining(promptTimeout)
  }

  const onIdle = () => {
    // onIdle will be called after the promptTimeout is reached.
    // In this case 30 seconds. Here you can close your prompt and 
    // perform what ever idle action you want such as log out your user.
    // Events will be rebound as long as `stopOnMount` is not set.
    setOpen(false)
    setRemaining(0)
  }

  const onActive = () => {
    // onActive will only be called if `reset()` is called while `isPrompted()` 
    // is true. Here you will also want to close your modal and perform
    // any active actions. 
    setOpen(false)
    setRemaining(0)
  }

  const { getRemainingTime, isPrompted, activate } = useIdleTimer({
    timeout,
    promptTimeout,
    onPrompt,
    onIdle,
    onActive
  })

  const handleStillHere = () => {
    setOpen(false)
    activate()
  }

  useEffect(() => {
    const interval = setInterval(() => {
      if (isPrompted()) {
        setRemaining(Math.ceil(getRemainingTime() / 1000))
      }
    }, 1000)
    return () => {
      clearInterval(interval)
    }
  }, [getRemainingTime, isPrompted])

  return (
    <div className='modal' style={{ display: open ? 'block' : 'none' }}>
      <p>Logging you out in {remaining} seconds</p>
      <button onClick={handleStillHere}>Im Still Here</button>
    </div>
  )
}
tgelu commented 1 year ago

Thanks for the well put response 👍

the reason onActive is called when you activate from the prompted state is so you can use your onActive handler to close your prompt

I did realise this was probably the reason, but wanted to make sure anyway. And if this is the design then I agree this issue can be closed. But for completeness' sake I will still mention what I found difficult about this model and one radical suggestion :)

Point of confusion

Radical suggestion

I can share what I did with a package that's wrapping react-idle-timer and maybe it will (or won't) provide inspiration for a future major release:

I have this type:

type Presence =
  | { type: 'idle' }
  | { type: 'active'; prompted: boolean };

(I think in your model prompted would belong to the idle branch, but you get the point)

And the hook that the main export does not have onIdle, onActive or onPrompt but instead has only one callback: onPresenceChange with the following type:

  /**
   * Function to call with the latest "Presence" value when
   * it changed. This can happen either programatically or
   * based on user activity.
   *
   * This is where you would want to pause, kill or resume
   * the user's session. See `Presence` export for helpers
   * to make assumptions on the current state.
   */
  onPresenceChange: (presence: Presence) => void;

Of course, you can export many helper functions to work with the Presence type, like:

const isIdle = (presence: Presence): boolean => presence.type === 'idle';
const isActive = (presence: Presence): boolean => presence.type === 'active';
const isPrompted = (presence: Presence) =>
  presence.type === 'active' && presence.prompted === true;
 // ...and so on

Again, as you pointed out, this is just semantics, there's no actual difference in the kinds of things you can do. But I think semantics matter for the developer experience :D - the interface is simpler, you can make whatever assumptions you want in one callback, you don't need to remember what you did in one callback to know what to do in another, you can simply take that presence value and use it as the state of a component, or you can pass it around as you wish since one value now describes the "presence"... and so on.

I guess it's kind of how react moved from lifecycle methods to hooks, from a bunch of unrelated events that you needed to combine well, to functions that encapsulated the use cases better :)

Thanks for the library and hard work!

SupremeTechnopriest commented 1 year ago

I think I could add this to the next release. It wouldn't break anything as its purely additive. I will make it so you can use any of onPresenceChange, onIdle, onActive, onAction. I agree that prompt should be on the active branch. Im going to see what the implications of that would be.

SupremeTechnopriest commented 1 year ago

Added to the latest release candidate (8). Next I need to determine the implications of considering prompted a part of the active branch. Let me know if you have any other requests for the 5.5.0 release. Im planning on publishing it at the end of the week. Just need to update all the docs.