Feat: Cursor based Pagination #479
Conversation
|
@mit-27 is attempting to deploy a commit to the Panora Team on Vercel. A member of the Team first needs to authorize it. |
|
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes introduce cursor-based pagination to the CRM API's contact management. This includes modifications to the Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant User
participant Controller
participant Service
participant Database
User->>+Controller: GetContacts(pageSize, cursor)
Controller->>+Service: getUnifiedContacts(pageSize, cursor)
Service->>+Database: Fetch contacts with cursor
Database-->>-Service: Return contacts, prev_cursor, next_cursor
Service-->>-Controller: Return contacts, prev_cursor, next_cursor
Controller-->>-User: Return paginated contacts
Assessment against linked issues
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
- Added cursor-based pagination to GET: crm/contacts endpoint
- Introduced unique constraint and default value for
created_atincrm_contactsmodel and table - Modified CRON schedule in
sync.service.tsfor testing - Added
GetContactsQueryDtoclass for pagination support - Updated Swagger spec to include pagination parameters
| : 'contact_id' in source_contact | ||
| ? String(source_contact.contact_id) | ||
| : undefined; | ||
| ? String(source_contact.contact_id) |
There was a problem hiding this comment.
Corrected indentation for better readability.
| const defaultPageSize = 10; | ||
|
|
||
| if (cursor) { | ||
| const isCursorPresent = await this.prisma.crm_contacts.findFirst({ | ||
| where: { |
There was a problem hiding this comment.
Check if the provided cursor exists and throw NotFoundError if it does not.
packages/api/src/crm/contact/types/dto/get-contacts-query.dto.ts
Outdated
Show resolved
Hide resolved
|
|
||
| @IsOptional() | ||
| @IsDefined() | ||
| @Transform(({ value }) => value === 'true' ? true : value === 'false' ? false : value) |
There was a problem hiding this comment.
@IsDefined() is redundant when used with @IsOptional(). Remove @IsDefined().
| @Transform(p => Buffer.from(p.value, 'base64').toString()) | ||
| @IsUUID() | ||
| cursor: string; | ||
|
|
There was a problem hiding this comment.
Use @IsBase64() to validate the cursor before transforming it.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- packages/api/prisma/schema.prisma (1 hunks)
- packages/api/scripts/init.sql (1 hunks)
- packages/api/src/crm/contact/contact.controller.ts (3 hunks)
- packages/api/src/crm/contact/services/contact.service.ts (5 hunks)
- packages/api/src/crm/contact/sync/sync.service.ts (1 hunks)
- packages/api/src/crm/contact/types/dto/get-contacts-query.dto.ts (1 hunks)
- packages/api/swagger/swagger-spec.json (1 hunks)
Files skipped from review due to trivial changes (1)
- packages/api/src/crm/contact/sync/sync.service.ts
Additional comments not posted (10)
packages/api/src/crm/contact/types/dto/get-contacts-query.dto.ts (3)
7-11: The implementation of theremote_datafield with a custom transformer to handle boolean values as strings is appropriate and follows best practices for handling boolean query parameters.
13-16: The use of the@Transformdecorator to convertpageSizeto a number ensures type safety and is a good practice for pagination parameters.
18-21: Transforming thecursorfrom a base64 string to a UUID string enhances security by obscuring direct database identifiers, which is a recommended practice.packages/api/src/crm/contact/contact.controller.ts (3)
11-12: The addition ofUsePipesandValidationPipealong with the import ofGetContactsQueryDtoare appropriate for enabling validation and handling pagination in thegetContactsmethod.Also applies to: 31-31
61-72: The addition ofpageSizeandcursorquery parameters, along with their documentation in the Swagger API, enhances the API's functionality and discoverability.
76-92: The modifications in thegetContactsmethod to useGetContactsQueryDtofor handling pagination are correctly implemented. The method correctly handles thepageSizeandcursorparameters for pagination.packages/api/src/crm/contact/services/contact.service.ts (2)
14-14: The addition of imports related to error handling and additional services is appropriate and supports the expanded functionality of the service.
Line range hint
496-631: The implementation of pagination logic in thegetContactsmethod, including handling ofpageSizeandcursorparameters, is correctly done. The method appropriately checks for the presence of a cursor and handles pagination boundaries.packages/api/scripts/init.sql (1)
468-475: The addition of a default value forcreated_atand a unique constraint in thecrm_contactstable enhances data integrity and prevents duplicate entries, aligning with the objectives of the PR.packages/api/swagger/swagger-spec.json (1)
1554-1569: Addition of pagination parameters for/crm/contactsendpoint.The addition of
pageSizeandcursorparameters aligns with the PR objectives to implement cursor-based pagination. This change is crucial for handling large datasets efficiently and avoiding issues like double entries mentioned in the linked issue. Good job on updating the Swagger documentation to reflect these new query parameters.
packages/api/prisma/schema.prisma
Outdated
| @@ -164,7 +164,7 @@ model crm_contacts { | |||
| id_crm_contact String @id(map: "pk_crm_contacts") @db.Uuid | |||
| first_name String | |||
| last_name String | |||
| created_at DateTime @db.Timestamp(6) | |||
| created_at DateTime @unique(map: "force_createdat_unique") @default(now()) @db.Timestamp(6) | |||
There was a problem hiding this comment.
Potential issue with unique constraint on created_at.
The addition of a unique constraint on the created_at field might lead to issues in environments with high write volumes, as multiple contacts could potentially be created at the same exact timestamp. Consider if this constraint is necessary or if it could be replaced with another unique identifier.
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- packages/api/src/crm/contact/contact.controller.ts (3 hunks)
- packages/api/src/crm/contact/sync/sync.service.ts (2 hunks)
- packages/api/src/crm/contact/types/dto/get-contacts-query.dto.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/api/src/crm/contact/types/dto/get-contacts-query.dto.ts
Additional context used
Biome
packages/api/src/crm/contact/contact.controller.ts
[error] 78-78: Decorators are not valid here. (parse)
Decorators are only valid on class declarations, class expressions, and class methods.
You can enable parameter decorators by setting theunsafeParameterDecoratorsEnabledoption totruein your configuration file.
[error] 79-79: Decorators are not valid here. (parse)
Decorators are only valid on class declarations, class expressions, and class methods.
You can enable parameter decorators by setting theunsafeParameterDecoratorsEnabledoption totruein your configuration file.
[error] 120-120: Decorators are not valid here. (parse)
Decorators are only valid on class declarations, class expressions, and class methods.
You can enable parameter decorators by setting theunsafeParameterDecoratorsEnabledoption totruein your configuration file.
[error] 121-121: Decorators are not valid here. (parse)
Decorators are only valid on class declarations, class expressions, and class methods.
You can enable parameter decorators by setting theunsafeParameterDecoratorsEnabledoption totruein your configuration file.
[error] 148-148: Decorators are not valid here. (parse)
Decorators are only valid on class declarations, class expressions, and class methods.
You can enable parameter decorators by setting theunsafeParameterDecoratorsEnabledoption totruein your configuration file.
[error] 149-149: Decorators are not valid here. (parse)
Decorators are only valid on class declarations, class expressions, and class methods.
You can enable parameter decorators by setting theunsafeParameterDecoratorsEnabledoption totruein your configuration file.
[error] 150-150: Decorators are not valid here. (parse)
Decorators are only valid on class declarations, class expressions, and class methods.
You can enable parameter decorators by setting theunsafeParameterDecoratorsEnabledoption totruein your configuration file.
[error] 190-190: Decorators are not valid here. (parse)
Decorators are only valid on class declarations, class expressions, and class methods.
You can enable parameter decorators by setting theunsafeParameterDecoratorsEnabledoption totruein your configuration file.
[error] 191-191: Decorators are not valid here. (parse)
Decorators are only valid on class declarations, class expressions, and class methods.
You can enable parameter decorators by setting theunsafeParameterDecoratorsEnabledoption totruein your configuration file.
[error] 192-192: Decorators are not valid here. (parse)
Decorators are only valid on class declarations, class expressions, and class methods.
You can enable parameter decorators by setting theunsafeParameterDecoratorsEnabledoption totruein your configuration file.
[error] 217-217: Decorators are not valid here. (parse)
Decorators are only valid on class declarations, class expressions, and class methods.
You can enable parameter decorators by setting theunsafeParameterDecoratorsEnabledoption totruein your configuration file.
[error] 218-218: Decorators are not valid here. (parse)
Decorators are only valid on class declarations, class expressions, and class methods.
You can enable parameter decorators by setting theunsafeParameterDecoratorsEnabledoption totruein your configuration file.packages/api/src/crm/contact/sync/sync.service.ts
[error] 32-32: Decorators are not valid here. (parse)
Decorators are only valid on class declarations, class expressions, and class methods.
You can enable parameter decorators by setting theunsafeParameterDecoratorsEnabledoption totruein your configuration file.
[error] 223-223: Unexpected any. Specify a different type. (lint/suspicious/noExplicitAny)
any disables many type checking rules. Its use should be avoided.
[error] 231-231: Use === instead of ==.
== is only allowed when comparing againstnull(lint/suspicious/noDoubleEquals)== is only allowed when comparing against null
Using == may be unsafe if you are relying on type coercion
Unsafe fix: Use ===
[error] 262-262: Unexpected any. Specify a different type. (lint/suspicious/noExplicitAny)
any disables many type checking rules. Its use should be avoided.
[error] 292-293: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 317-318: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 342-342: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 368-368: Unexpected any. Specify a different type. (lint/suspicious/noExplicitAny)
any disables many type checking rules. Its use should be avoided.
Additional comments not posted (6)
packages/api/src/crm/contact/contact.controller.ts (4)
31-31: Import statement forGetContactsQueryDtois correctly added.
61-72: Added pagination query parameters correctly.
11-12: Ensure proper configuration forValidationPipe.
79-79: EnsureGetContactsQueryDtois correctly utilized in the method signature.Verification successful
The
GetContactsQueryDtoclass is correctly defined and utilized in the method signature.
GetContactsQueryDtois imported and used incontact.controller.ts.- The class is defined with appropriate decorators and transformations in
get-contacts-query.dto.ts.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `GetContactsQueryDto` in the project. # Test: Search for `GetContactsQueryDto` usage in the project. Expect: Correct usage in method signatures. rg --type typescript $'GetContactsQueryDto'Length of output: 77
Script:
#!/bin/bash # Description: Verify the usage of `GetContactsQueryDto` in the project. # Test: Search for `GetContactsQueryDto` usage in the project. Expect: Correct usage in method signatures. rg --type ts 'GetContactsQueryDto'Length of output: 350
Script:
#!/bin/bash # Description: Display the contents of `get-contacts-query.dto.ts` to verify the definition of `GetContactsQueryDto`. # Test: Display the contents of the file to ensure `GetContactsQueryDto` is correctly defined. cat packages/api/src/crm/contact/types/dto/get-contacts-query.dto.tsLength of output: 598
Tools
Biome
[error] 79-79: Decorators are not valid here. (parse)
Decorators are only valid on class declarations, class expressions, and class methods.
You can enable parameter decorators by setting theunsafeParameterDecoratorsEnabledoption totruein your configuration file.packages/api/src/crm/contact/sync/sync.service.ts (2)
88-93: Correctly fetches user details based on the provided user ID.
372-372: Ensure proper handling of date fields in the database operations.
| @ApiCustomResponse(UnifiedContactOutput) | ||
| @UseGuards(ApiKeyAuthGuard) | ||
| @Get() | ||
| @UsePipes(new ValidationPipe({ transform: true, disableErrorMessages: true })) |
There was a problem hiding this comment.
Decorator usage error: @UsePipes should not be used here.
- @UsePipes(new ValidationPipe({ transform: true, disableErrorMessages: true }))Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| @UsePipes(new ValidationPipe({ transform: true, disableErrorMessages: true })) |
mit-27
force-pushed
the
feat/cursor-pagination
branch
from
1cb54a4
to
3855a53
Compare
It sounds good for me. |
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Introduced cursor-based pagination for GET: crm/contacts Unified API
- Updated Dockerfiles to use turbo version 1.13.4
- Removed FieldMappingModal.tsx component
- Added
id_linked_userfield to EventsTable and schema - Enhanced error handling in CRM utilities
| @@ -26,6 +28,7 @@ import { | |||
| import { ApiKeyAuthGuard } from '@@core/auth/guards/api-key.guard'; | |||
There was a problem hiding this comment.
Consider adding a brief comment explaining the purpose of the GetContactsQueryDto import for clarity.
…sor-pagination
|
/bounty 200 |
💎 $200 bounty • Panora (YC S24)Steps to solve:
Additional opportunities:
Thank you for contributing to panoratech/Panora! Add a bounty • Share on socials
|
|
/attempt #479 /claim #479
|
|
💡 @mit-27 submitted a pull request that claims the bounty. You can visit your bounty board to reward. |
|
🎉🎈 @mit-27 has been awarded $200! 🎈🎊 |
/claim #479
Hello @naelob and @rflihxyz , currently I have added cursor pagination for GET: crm/contacts Unified API to validate the format of the response. Let me know if there are any changes required. then Upon your approval, I will implement cursor pagination for all unified APIs. Thanks.
API response Format
Summary by CodeRabbit
New Features
pageSizeandcursorparameters.Improvements
Testing Enhancements