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

admin: don't use state change to update offset #4815

Merged
merged 2 commits into from
May 16, 2024
Merged

Conversation

minrk
Copy link
Member

@minrk minrk commented May 13, 2024

set offset -> request page -> response sets offset is a recipe for races

instead, send request with new offset and only update offset state based on the actual loaded data

made easier by consolidating page update requests into single loadPageData

this also fixes the problem where the pagination info would be updated before the content, so as seen in #4180, it would go:

  1. page 1-10
  2. click next
  3. pagination updated to 11-20, send request for 11-20, 1-10 still displayed
  4. request completes, update display to 11-20

by detaching the "request next page" from the state cycle, the looping requests shouldn't happen. I tried to solve this in general by debouncing loadPageData, but had a hard time figuring that out. And offset is really the only state variable that's an input to the request which can also be set to a new value by the response.

closes #4810

@minrk minrk added the bug label May 13, 2024
set offset -> request page -> response sets offset is a recipe for races

instead, send request with new offset and only update offset state

made easier by consolidating page update requests into single loadPageData
Copy link
Member

@consideRatio consideRatio left a comment

Choose a reason for hiding this comment

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

Optional cleanup suggestion of now unused setOffset in util file - LGTM!

EDIT: Sorry setOffset is still in use - in Groups.jsx - so my review boils down to the question of it it should be, but if that should be changed also is scope bleed of this PR so LGTM as it is!

Comment on lines +147 to +150
// single callback to reload the page
// uses current state, or params can be specified if state
// should be updated _after_ load, e.g. offset
const loadPageData = (params) => {
Copy link
Member

Choose a reason for hiding this comment

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

Very thankful for these function descriptions!

@minrk
Copy link
Member Author

minrk commented May 15, 2024

You're right, Groups will definitely have the same issue. Updated it to match.

I think having setOffset defined even though we don't currently use it is still sensible. It's specific to how it's used in these two views, where we call setPagination with the result. If, instead, we used setOffset to trigger the request and didn't call setPagination with the response, that would avoid the issue as well.

@minrk minrk merged commit 282cc02 into jupyterhub:main May 16, 2024
20 checks passed
@minrk minrk deleted the admin-test branch May 16, 2024 06:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Admin UI glitch - double pressing on "Next" page button causes bugged state
2 participants