Enhance view state management by integrating UiStore for scroll position and selected event persistence

[metheos]

addresses httptoolkit/httptoolkit#470 and further adds selected event persistence

[CLAassistant]

All committers have signed the CLA.

[pimterry]

Thanks for the contribution @metheos! At a glance it looks like this is very promising, this is a nice improvement I'd definitely like to include. It looks like something has autoformatted all the UI store code along the way though, which makes this impossible to review and would break git blame later on.

Can you undo that please, so only the actual changes for this PR are included changes?

[metheos]

Should be good now. Missed that when I was testing. Changes were pretty minimal there!

[pimterry]

Thanks @metheos! That's much clearer. I've add a proper review here - I think this will be great, but it needs a little work to tidy up the details and avoid the setTimeout etc workarounds.

By the way, in case you're not aware, HTTP Toolkit Pro is totally free for all contributors! I've just set up an account linked to your git commit email, just click 'Get Pro' then 'Log into existing account' and enter your email to get started. Thanks for getting involved! 👍

[pimterry]

Would be good to avoid the fixed timeouts here. I think we can do so though - we just need to wait until listBodyRef is ready, right? I think we can convert this into a callback ref, which means we can trigger the DOM interaction (updating the scroll position) immediately after that's rendered.

That will be a bit more responsive (no visible jumping around I hope) and should avoid most of the risk of the race condition you're protecting against with hasRestoredInitialState here as well. Does that make sense?

[metheos]

yes, good idea. I wasn't sure what to look for. I'll get rid of this and set it to be handled by a new callback ref (setListBodyRef)

[pimterry]

I'm not sure I totally understand the goal of this bit. Can you explain when this applies in practice, and how it helps? I haven't noticed issues with the scroll position while filtering.

[metheos]

I put it there as more of a safeguard since I've had issues with wonky scroll position behavior depending on the browser/rendering engine in use. We can take it out if you don't expect that to change, I wasn't sure how consistent the web view experience was across platforms.

[pimterry]

Ditto here - we should restructure this to trigger when listRef is set, directly, instead of guessing how long it'll take.

[metheos]

got it, added new callback (setListRef)

[pimterry]

Nice, thanks @metheos! Yes, those were exactly the changes I was looking for. I've taken that a little further, dropped the setTimeout (we don't need to worry much about browser compat - Electron means we effectively ship our own specific browser version for most uses, and I would expect there won't be many issues anyway), and tried to simplify the logic around the view page controlling the list. Can you give it a quick test, and make sure it still works as you expect? I'm not sure if I've missed some case you were trying to cover. If you're happy though, just let me know and I think it's good to merge now.

[metheos]

Your commit c300f10 removes the intended restoration of the scroll position. Without that code the list will always scroll to the selected event rather than restoring the actual scroll position.

[pimterry]

Without that code the list will always scroll to the selected event rather than restoring the actual scroll position.

Hmm, it's just it seems like that was what it was trying to do before though:

• View page set shouldRestoreViewStateOnRefSet if uiStore.selectedEventId was set
• The ref triggered a call to restoreViewState
• In restoreViewState, your code intentionally did exactly what you describe:

  if (this.props.selectedEvent) {
    this.scrollToEvent(this.props.selectedEvent);
  } else {
    this.restoreScrollPosition();
  }
  

But regardless, I think you are right - restoring the scroll position (never jumping to the automatically persisted selected event) by default is the best choice.

I should have a minute to look at that and fix up my changes tomorrow, or you're welcome to take a look. Sorry this has been delayed here a while, but we should be able to get it live this week!