[dashboard] add pagination to admin search #13161
Conversation
|
started the job as gitpod-build-sefftinge-populate-admin-dashboard-8983.1 because the annotations in the pull request description changed |
|
/werft run 👍 started the job as gitpod-build-sefftinge-populate-admin-dashboard-8983.2 |
svenefftinge
force-pushed
the
sefftinge/populate-admin-dashboard-8983
branch
from
55365a0 to
746638b
Compare
|
/werft run with-preview 👍 started the job as gitpod-build-sefftinge-populate-admin-dashboard-8983.5 |
svenefftinge
changed the title
svenefftinge
force-pushed
the
sefftinge/populate-admin-dashboard-8983
branch
from
e31d0b8 to
f247773
Compare
| orderBy: "creationDate", | ||
| offset: 0, | ||
| offset: (currentPage - 1) * pageLength, |
There was a problem hiding this comment.
currentPage starts at value of 0, wouldn't this cause the offset to be negative?
There was a problem hiding this comment.
currentPage should start at 1.
There was a problem hiding this comment.
Now it does, but before it was definitely initialized to 0.
There was a problem hiding this comment.
Yes on 3 out of 4 occasions 😬 (the four search pages are a bit copy and paste and of course I only tested on one of them, which also explains why the limit was set to 2 only on one of them)
| // Disables empty search on the workspace search page | ||
| if (!props.user && queryTerm.length === 0) { | ||
| return; | ||
| } | ||
| // if (!props.user && queryTerm.length === 0) { | ||
| // return; | ||
| // } |
There was a problem hiding this comment.
I believe this was the cause of an incident when we originally added the WS search - it would cause too many queries (but may be okay now)
There was a problem hiding this comment.
Yes, was this maybe before we introduced limits? cc @geropl Do you remember?
It's for sure a SaaS hack that makes the view less useful in smaller self-hosted installations. If we need to protect us we better do that on the server based on a saas-specific configuration.
There was a problem hiding this comment.
AFAIR the problem was we executed an unlimited query with an empty query term, multiple times on page load.
If we do proper limits (and maybe just once on page load) this should be fine (did not check the code).
| @@ -43,6 +44,11 @@ export function WorkspaceSearch(props: Props) { | |||
| const [queryTerm, setQueryTerm] = useState(""); | |||
| const [searching, setSearching] = useState(false); | |||
| const [currentWorkspace, setCurrentWorkspaceState] = useState<WorkspaceAndInstance | undefined>(undefined); | |||
| const pageLength = 2; | |||
There was a problem hiding this comment.
Why so low? Originally this was set to 100
There was a problem hiding this comment.
For testing. But I have updated it 50 again.
There was a problem hiding this comment.
Out of curiosity, why is it needed to commit test code? I'm asking because I'm trying to understand the problem and because this is something worth digging into to eliminate. Committing it risks we actually land it (and we have done so in past and caused incidents).
There was a problem hiding this comment.
In this case, it was just an overlook by myself.
It could have been a conscious decision to allow others testing the changes more easily. In that case I would have added a 'hold' and a comment.
svenefftinge
force-pushed
the
sefftinge/populate-admin-dashboard-8983
branch
from
f247773 to
e6df4f6
Compare
Description
This change adds pagination support for the various admin dashboard list views.
Related Issue(s)
Fixes #8983
How to test
Release Notes
Documentation
Werft options:
Valid options are
all,workspace,webapp,ide