Pagination #1530
Pagination #1530
Conversation
ARADDCC012
dismissed
their stale review
Unapproving after testing merge protections.
ARADDCC012
dismissed
their stale review
Unapproving after fixing branch protection on main
| @@ -20,6 +20,8 @@ export const getModelsSearchSchema = z.object({ | |||
| search: z.string().optional().default(''), | |||
| allowTemplating: strictCoerceBoolean(z.boolean().optional()), | |||
| schemaId: z.string().optional(), | |||
| currentPage: z.string().optional(), | |||
There was a problem hiding this comment.
We can use z.coerce.number().optional() to validate currentPage and pageSize as numbers and transform them to numbers at the same time. That way we don't ever have to treat them as strings.
backend/src/services/model.ts
Outdated
| } | ||
| const promise = Promise.all([ | ||
| ModelModel.find(query) | ||
| .limit(pageSize) |
There was a problem hiding this comment.
Looks like Github actions is having a wobbly here as this is definitely valid based on the mongoose docs: https://mongoosejs.com/docs/6.x/docs/api/query.html#query_Query-limit
backend/src/services/model.ts
Outdated
| // Sort by text search | ||
| cursor = cursor.sort({ score: { $meta: 'textScore' } }) | ||
| } | ||
| const promise = Promise.all([ |
There was a problem hiding this comment.
This can be simplified to const [results, count] = await Promise.all([...
backend/src/services/model.ts
Outdated
| const auths = await authorisation.models(user, results, ModelAction.View) | ||
| return results.filter((_, i) => auths[i].success) | ||
| return { results: results.filter((_, i) => auths[i].success), totalEntries: count } |
There was a problem hiding this comment.
I'm not sure if this is an issue or not, but totalEntries will include entries that the user doesn't have access to.
Should we instead return a count of all the entires the user does have access to?
There was a problem hiding this comment.
As we check the user auth after making the call, we'll need to fetch all the documents that match the query without limiting the result based on current page and page size etc.
I don't think we can do this as part of the Mongo query which means that we'll be losing a lot of the benefits that pagination provides, but I'm not sure we can get around this?
|
|
||
| interface PaginationSelectorProps { | ||
| currentPage: number | string | ||
| currentPageOnChange: (newValue: number | string) => void |
There was a problem hiding this comment.
To follow convention consider renaming to onCurrentPageChange
| ) | ||
|
|
||
| const menuItems = useMemo(() => { | ||
| return [...Array(lastPage | 0)].map((_, i) => { |
There was a problem hiding this comment.
Minor, but I'm never a fan of single character variable names. In most cases it's fair to assume that i is an index, but that involves either making an assumption or reading the entire block of code to check. It's easier to simply name this index in my opinion.
There was a problem hiding this comment.
index is still one of the few places I default to using i, left over my Java days! Happy to change though :)
| [currentPageOnChange], | ||
| ) | ||
|
|
||
| const menuItems = useMemo(() => { |
There was a problem hiding this comment.
Consider renaming to something more descriptive like pageNumberOptions
| label='Age' | ||
| onChange={handleManualPageChange} | ||
| MenuProps={{ | ||
| style: { |
| <Select | ||
| size='small' | ||
| value={currentPage.toString()} | ||
| label='Age' |
| <Typography>Page:</Typography> | ||
| <Select | ||
| size='small' | ||
| value={currentPage.toString()} |
There was a problem hiding this comment.
Two things here. Firstly we usually use string templates for this sort of thing: value={${currentPage}}.
Secondly, we shouldn't need to transform it to a string as the menu items' values are numbers.
No description provided.