-
Notifications
You must be signed in to change notification settings - Fork 5
Added Pagination #32
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
base: main
Are you sure you want to change the base?
Added Pagination #32
Conversation
fred-maina
left a comment
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.
LGTM. Will merge when change is effected on Frontend
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.
Pull request overview
This PR adds backend pagination support to the anonymous chat history endpoint (/api/chat/session_history), converting it from returning all messages at once to supporting page-based retrieval. The implementation follows Spring Data JPA pagination patterns using Page and Pageable.
Key Changes:
- Added pagination parameters (
pageandsize) to the chat history endpoint with sensible defaults (page=0, size=50) - Created a new
PaginationDtoto encapsulate paginated response metadata (page number, size, total pages, hasNextPage flag) - Updated the repository query to accept
Pageableand returnPage<ChatMessage>instead ofList<ChatMessage>
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
PaginationDto.java |
New DTO containing pagination metadata and message list; naming could be more specific |
ChatMessageRepository.java |
Updated findFullChatHistory to accept Pageable parameter; has conflicting ORDER BY clauses |
ChatService.java |
Modified to accept pagination params, create PageRequest, and return PaginationDto; lacks input validation |
ChatController.java |
Added pagination request parameters with defaults and expanded response to include pagination metadata |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
| @Transactional | ||
| public List<ChatMessageDto> getChatHistoryForAnonymous(String sessionId, String recipientUsername) { | ||
| public PaginationDto getChatHistoryForAnonymous(String sessionId, String recipientUsername, | ||
| int page,int size) { |
Copilot
AI
Dec 1, 2025
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.
Missing input validation for page and size parameters. Negative values for page or non-positive values for size would cause issues when passed to PageRequest.of(). Consider adding validation to ensure page >= 0 and size > 0, or handle the IllegalArgumentException that PageRequest.of() throws for invalid values. You could also set reasonable upper bounds for size to prevent excessive memory usage.
| int page,int size) { | |
| int page,int size) { | |
| // Validate page and size parameters | |
| if (page < 0) { | |
| throw new IllegalArgumentException("Page index must not be negative: " + page); | |
| } | |
| int MAX_PAGE_SIZE = 100; | |
| if (size <= 0 || size > MAX_PAGE_SIZE) { | |
| throw new IllegalArgumentException("Page size must be between 1 and " + MAX_PAGE_SIZE + ": " + size); | |
| } |
|
|
||
| List<ChatMessage> messages = chatMessageRepository.findFullChatHistory(sessionId,recipient.getId()); | ||
| Pageable pageable = PageRequest.of(page, size, Sort.by("timestamp").descending()); | ||
| Page<ChatMessage> messages = chatMessageRepository.findFullChatHistory(sessionId,recipient.getId(),pageable); |
Copilot
AI
Dec 1, 2025
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.
Missing space after commas in method call. Should be findFullChatHistory(sessionId, recipient.getId(), pageable) instead of findFullChatHistory(sessionId,recipient.getId(),pageable) for consistency with Java formatting conventions.
| Page<ChatMessage> messages = chatMessageRepository.findFullChatHistory(sessionId,recipient.getId(),pageable); | |
| Page<ChatMessage> messages = chatMessageRepository.findFullChatHistory(sessionId, recipient.getId(), pageable); |
| Page<ChatMessage> findFullChatHistory(@Param("sessionId") String sessionId, | ||
| @Param("recipientId") UUID recipientId | ||
| , Pageable pageable); |
Copilot
AI
Dec 1, 2025
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.
Inconsistent parameter formatting. The Pageable pageable parameter should be on the same line as the previous parameter or consistently indented. The comma-space before Pageable should follow the same pattern as line 30.
| Page<ChatMessage> findFullChatHistory(@Param("sessionId") String sessionId, | |
| @Param("recipientId") UUID recipientId | |
| , Pageable pageable); | |
| Page<ChatMessage> findFullChatHistory( | |
| @Param("sessionId") String sessionId, | |
| @Param("recipientId") UUID recipientId, | |
| Pageable pageable); |
| @RequestParam(defaultValue = "0") int page, | ||
| @RequestParam(defaultValue = "50") int size |
Copilot
AI
Dec 1, 2025
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.
Inconsistent indentation. The @RequestParam annotation on line 68 has extra leading spaces compared to the @RequestParam on line 69, making the parameter list formatting inconsistent.
| @RequestParam(defaultValue = "0") int page, | |
| @RequestParam(defaultValue = "50") int size | |
| @RequestParam(defaultValue = "0") int page, | |
| @RequestParam(defaultValue = "50") int size |
|
|
||
| @Data | ||
| @AllArgsConstructor | ||
| public class PaginationDto { |
Copilot
AI
Dec 1, 2025
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.
The class name PaginationDto is too generic for a DTO that is specifically designed for chat message pagination. Based on the PR description mentioning ChatHistoryPageDto and following the naming pattern of other DTOs in this package (like ChatMessageDto, ChatSessionDto), this should be named ChatHistoryPageDto instead. This would make the purpose clearer and avoid confusion if other paginated endpoints are added in the future.
| public class PaginationDto { | |
| public class ChatHistoryPageDto { |
| "(m.toSessionId = :sessionId AND m.fromUser.id = :recipientId) " + | ||
| "ORDER BY m.timestamp") |
Copilot
AI
Dec 1, 2025
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.
The hardcoded ORDER BY m.timestamp in the @Query annotation conflicts with the Sort parameter passed through the Pageable. When Sort.by("timestamp").descending() is provided in the Pageable (as done in ChatService.java line 86), Spring Data JPA will append it to the query, resulting in duplicate ORDER BY clauses which could cause unexpected behavior or errors. Remove the ORDER BY m.timestamp from the query string and let the Pageable handle the sorting.
| "(m.toSessionId = :sessionId AND m.fromUser.id = :recipientId) " + | |
| "ORDER BY m.timestamp") | |
| "(m.toSessionId = :sessionId AND m.fromUser.id = :recipientId)") |
| @Transactional | ||
| public List<ChatMessageDto> getChatHistoryForAnonymous(String sessionId, String recipientUsername) { | ||
| public PaginationDto getChatHistoryForAnonymous(String sessionId, String recipientUsername, | ||
| int page,int size) { |
Copilot
AI
Dec 1, 2025
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.
Missing space after comma in the parameter list. Should be int page, int size instead of int page,int size for consistency with Java formatting conventions.
| int page,int size) { | |
| int page, int size) { |
✅ Backend Pagination Added
Updated ChatMessageRepository.findFullChatHistory to accept Pageable instead of returning a full list.
Modified ChatService.getChatHistoryForAnonymous to:
Accept page and size
Use PageRequest.of(page, size, Sort.by("timestamp").descending())
Return a new ChatHistoryPageDto that includes:
paginated messages
page number
size
total pages
hasNextPage flag
✅ New DTO Added
Created PaginationDto.ChatHistoryPageDto containing:
List messages
int page
int size
int totalPages
boolean hasNextPage
✅ Controller Updated