-
Notifications
You must be signed in to change notification settings - Fork 0
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
Users pagination #35
Users pagination #35
Conversation
WalkthroughThe changes in this pull request enhance the Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/services/user.ts (1)
82-87
: LGTM! Consider improvingtotalPages
calculation.The changes to include pagination information are good. They provide necessary data for implementing pagination on the client side.
However, consider improving the
totalPages
calculation to handle the case when there are no users:- const totalPages = Math.ceil(total / limit) + const totalPages = Math.max(1, Math.ceil(total / limit))This ensures that
totalPages
is always at least 1, which can be more intuitive for consumers of this function.Also applies to: 94-100
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/services/user.ts (1 hunks)
🔇 Additional comments (2)
src/services/user.ts (2)
Line range hint
1-100
: Overall assessment: Good implementation of pagination.The changes to the
queryUsers
function effectively implement pagination support. The addition of total count, total pages, and other pagination metadata will greatly improve the usability of this function for client-side pagination.A few minor improvements have been suggested, mainly around edge case handling and ensuring backwards compatibility. Once these are addressed, the implementation will be robust and ready for use.
Remember to update any consumers of this function to handle the new return type, as it has changed significantly from returning just an array of users to an object containing pagination information.
94-100
: Reminder: Update consumers ofqueryUsers
function.The return type of
queryUsers
has changed significantly. It now returns an object with pagination information instead of just an array of users.Please ensure that all consumers of this function are updated to handle the new return type. Run the following script to find potential usage of
queryUsers
:Review the results and update the consumers accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/services/user.ts (1)
86-86
: Consider adding a safeguard against division by zeroWhile the current implementation is robust, it's worth adding a small safeguard to prevent potential division by zero in the
totalPages
calculation. This could happen iflimit
is accidentally set to 0.Consider updating the line as follows:
- const totalPages = Math.max(1, Math.ceil(total / limit)) + const totalPages = Math.max(1, Math.ceil(total / Math.max(1, limit)))This ensures that even if
limit
is 0, the function will behave as if the limit is 1, preventing any potential errors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/services/user.ts (1 hunks)
🔇 Additional comments (3)
src/services/user.ts (3)
82-101
: Great implementation of pagination logic!The changes effectively address the previous comment about handling cases where
page
exceedstotalPages
. The implementation is correct and efficient, providing a robust pagination solution. Here's a breakdown of the improvements:
- Calculation of total users and pages (lines 82-87)
- Introduction of
adjustedPage
to handle edge cases (line 87)- Using
adjustedPage
in theskip
calculation (line 93)- Returning comprehensive pagination details (lines 95-101)
These changes significantly enhance the
queryUsers
function, making it more versatile and user-friendly.
82-101
: Summary of changes toqueryUsers
functionThe modifications to the
queryUsers
function significantly improve its functionality by introducing robust pagination. Key improvements include:
- Calculation of total users and pages
- Handling of edge cases where requested page exceeds total pages
- Returning comprehensive pagination details
These changes enhance the function's versatility and user-friendliness. The implementation addresses previous comments and provides a solid foundation for paginated user queries.
Remember to verify the usage of this function across the codebase due to the change in return type, and consider the minor optimization suggested for the
totalPages
calculation.Overall, these changes represent a substantial improvement to the user service.
95-101
: Verify usage ofqueryUsers
function across the codebaseThe return type of the
queryUsers
function has been significantly changed. It now returns an object with pagination details instead of just an array of users. This change, while beneficial, may impact other parts of the codebase that use this function.Please ensure that all callers of the
queryUsers
function are updated to handle the new return type. You can use the following script to identify potential usage sites:This will help identify areas that may need updates to accommodate the new return structure.
✅ Verification successful
Usage of
queryUsers
function verifiedOnly one usage of the
queryUsers
function was found insrc/controllers/user.ts
. Please ensure that this usage correctly handles the new return type with pagination details.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find potential usage of queryUsers function # Search for queryUsers function calls echo "Searching for queryUsers function calls:" rg --type typescript "queryUsers\(" -A 5 # Search for destructuring of users that might need updating echo "\nSearching for potential destructuring of users that might need updating:" rg --type typescript "const\s+{\s*users\s*}\s*=\s*await\s*queryUsers" -A 5Length of output: 542
Script:
#!/bin/bash # Description: Find potential usage of queryUsers function with corrected file type and regex. # Search for queryUsers function calls echo "Searching for queryUsers function calls:" rg --type ts "queryUsers\(" -A 5 # Search for destructuring of users that might need updating echo "\nSearching for potential destructuring of users that might need updating:" rg --type ts 'const\s+{\s*users\s*}\s*=\s*await\s*queryUsers' -A 5Length of output: 839
Script:
#!/bin/bash # Description: Find potential usage of queryUsers function with corrected regex patterns. # Search for queryUsers function calls echo "Searching for queryUsers function calls:" rg --type ts "queryUsers\(" -A 5 # Search for destructuring of users that might need updating echo "\nSearching for potential destructuring of users that might need updating:" rg --type ts 'const\s+\{\s*users\s*\}\s*=\s*await\s*queryUsers' -A 5Length of output: 703
@coderabbitai |
Tip For best results, initiate chat on the files or code changes. @likui628: Understood. Encapsulating the pagination logic will improve code reusability and readability. Here's how you can do it: Create a pagination utility function: // src/utils/pagination.ts
export function paginate(page: number, limit: number, total: number) {
const totalPages = Math.ceil(total / limit) || 1;
const adjustedPage = Math.min(Math.max(1, page), totalPages);
const skip = (adjustedPage - 1) * limit;
return {
page: adjustedPage,
limit,
total,
totalPages,
skip,
};
} Update the // src/services/user.ts
import { paginate } from '../utils/pagination';
export async function queryUsers(page: number, limit: number) {
const total = await prisma.user.count();
const { page: adjustedPage, limit: adjustedLimit, skip, totalPages } = paginate(page, limit, total);
const users = await prisma.user.findMany({
skip,
take: adjustedLimit,
orderBy: { id: 'asc' },
});
return {
users,
page: adjustedPage,
limit: adjustedLimit,
total,
totalPages,
};
} This way, the pagination logic is encapsulated in a separate utility function, which can be reused across different services whenever pagination is needed. ✏️ Learnings added
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
src/utils/prisma.ts (1)
36-39
: Consider performance implications of counting large datasetsUsing
db.count({ where })
can introduce performance overhead on large datasets because it requires scanning and counting a potentially vast number of records. This can impact the responsiveness of your application.Suggestions:
- Implement approximate counting: If exact counts are not critical, consider using database-specific features to get an estimated count more efficiently.
- Introduce caching: Cache count results for frequent queries to reduce database load.
- Limit maximum
pageSize
: Enforce a reasonable maximum onpageSize
to prevent users from requesting excessively large pages.These strategies can help mitigate performance issues and improve the scalability of your application.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- src/services/user.ts (2 hunks)
- src/utils/prisma.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/services/user.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
src/utils/prisma.ts (1)
36-60
: Excellent pagination logic and return valueThe implementation of the
paginate
function is well done:
- The use of
Promise.all
for concurrent database operations is efficient.- The
totalPages
calculation now correctly handles the case whentotalCount
is zero, addressing a previous review comment.- The return object provides all necessary information for pagination.
These improvements enhance the function's performance and correctness.
Consider adding a comment explaining the
totalPages
calculation for future maintainers:// Calculate total pages, ensuring it's 0 when there are no items const totalPages = totalCount > 0 ? Math.ceil(totalCount / pageSize) : 0
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- src/services/user.ts (2 hunks)
- src/utils/prisma.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/services/user.ts
🔇 Additional comments (3)
src/utils/prisma.ts (3)
1-3
: LGTM: Import and prisma client initializationThe addition of
Prisma
to the import statement is appropriate for the new type definitions. The prisma client initialization remains correct.
5-12
:⚠️ Potential issueConsider using Prisma's public types instead of internal ones
While the type definitions are well-structured, using
Prisma.TypeMap
accesses Prisma's internal types, which may change in future versions and lead to potential issues. Consider using Prisma's public types for better stability and maintainability.As suggested in a previous review, you could refactor these types to use the model delegate directly:
type PaginationOptions<ModelDelegate> = { model: ModelDelegate; where?: Parameters<ModelDelegate['findMany']>[0]['where']; orderBy?: Parameters<ModelDelegate['findMany']>[0]['orderBy']; include?: Parameters<ModelDelegate['findMany']>[0]['include']; page?: number; pageSize?: number; };This approach leverages Prisma Client's existing types and avoids relying on internal structures.
22-34
:⚠️ Potential issueGood input validation, but remove @ts-expect-error
The input validation for
page
andpageSize
is a great addition, ensuring they are positive integers. This addresses a previous review comment and improves the function's robustness.However, the use of
@ts-expect-error
is concerning as it suppresses a TypeScript error instead of addressing the underlying issue.As suggested in a previous review, consider refactoring the function to accept the model delegate directly:
export async function paginate<ModelDelegate extends { findMany: Function; count: Function }>({ model, page = 1, pageSize = 10, where, orderBy, include, }: PaginationOptions<ModelDelegate>) { // Rest of your code... }This approach eliminates the need for
@ts-expect-error
and provides better type safety.
type PaginationOptions<ModelName extends ModelNames> = { | ||
modelName: ModelName | ||
where?: PrismaFindManyArgs<ModelName>['where'] | ||
orderBy?: PrismaFindManyArgs<ModelName>['orderBy'] | ||
include?: PrismaFindManyArgs<ModelName>['include'] | ||
page?: number | ||
pageSize?: number | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well-structured PaginationOptions type, but reconsider use of internal Prisma types
The PaginationOptions
type is well-defined and includes all necessary fields for pagination. However, it relies on the previously defined types that use Prisma's internal types.
Consider refactoring this type along with the previous ones to use Prisma's public types, as suggested in the previous comment. This will make your code more resilient to future Prisma updates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
src/types/index.ts (1)
Line range hint
17-19
: Consider makingtoken
optional inUserData
interfaceThe
UserData
interface extends theUser
type and adds atoken
field. However, not all user data might include a token. Consider making thetoken
field optional for more flexibility:export interface UserData extends User { token?: string }This change would allow the
UserData
interface to be used in contexts where a token might not be present, improving its reusability.src/validations/user.ts (1)
10-13
: LGTM! Consider adding a default sorting option.The addition of
orderBy
andorder
properties to thegetUsersSchema
is a good enhancement. It allows for flexible sorting of user queries, which aligns well with the pagination feature.Consider setting default values for these properties to ensure consistent behavior when they're not explicitly provided. For example:
orderBy: z .enum(['name', 'email', 'role', 'createdAt', 'updatedAt']) .default('createdAt') .optional(), order: z.enum(['asc', 'desc']).default('desc').optional(),This would default to sorting by 'createdAt' in descending order if not specified.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- src/services/user.ts (2 hunks)
- src/types/index.ts (1 hunks)
- src/validations/user.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/services/user.ts
@@ -1,9 +1,9 @@ | |||
import { Role, User } from '@prisma/client' | |||
|
|||
export interface Pagination { | |||
orderBy?: 'name' | 'email' | 'createdAt' | 'updatedAt' | |||
orderBy?: string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider using a union type for orderBy
instead of string
While changing orderBy
to string
provides more flexibility, it reduces type safety. This could potentially lead to runtime errors if invalid values are used. Consider using a union type with a catch-all string literal type for future additions:
orderBy?: 'name' | 'email' | 'createdAt' | 'updatedAt' | (string & {})
This approach maintains some type safety while allowing for future additions.
order?: 'asc' | 'desc' | ||
limit?: number | ||
pageSize?: number |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
Incomplete Rename of limit
to pageSize
in Pagination Context
limit
is still used insrc/validations/common.ts
within the pagination validation schema and should be renamed topageSize
to ensure consistency.- The usage of
limit
insrc/middlewares/rate-limiter.ts
is appropriate and unrelated to pagination.
🔗 Analysis chain
Rename from limit
to pageSize
looks good
The rename from limit
to pageSize
is a good change as it's more descriptive and aligns better with pagination terminology. However, ensure that this change is consistently applied throughout the codebase to maintain coherence.
Let's verify the impact of this rename:
Further Verification Needed
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining uses of 'limit' in pagination context
# and ensure 'pageSize' is used consistently
# Search for 'limit' usage in TypeScript files
echo "Searching for 'limit' usage:"
rg --type typescript '\blimit\b' -C 2
# Search for 'pageSize' usage in TypeScript files
echo "Searching for 'pageSize' usage:"
rg --type typescript '\bpageSize\b' -C 2
Length of output: 304
Script:
#!/bin/bash
# Description: Check for any remaining uses of 'limit' in pagination context
# and ensure 'pageSize' is used consistently
# Search for 'limit' usage in TypeScript files
echo "Searching for 'limit' usage:"
rg --glob '*.ts' --glob '*.tsx' '\blimit\b' -C 2
# Search for 'pageSize' usage in TypeScript files
echo "Searching for 'pageSize' usage:"
rg --glob '*.ts' --glob '*.tsx' '\bpageSize\b' -C 2
Length of output: 2739
Summary by CodeRabbit