-
Notifications
You must be signed in to change notification settings - Fork 1
[FIX] 전체 사진첩 개수 응답을 위한 PageResponse 수정 #150
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
Conversation
📝 WalkthroughWalkthroughThe pull request migrates photo album pagination from Slice-based to Page-based approach across repository, service, and controller layers. Additionally, two new service methods are introduced: Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 🧪 Unit Test Generation v2 is now available!We have significantly improved our unit test generation capabilities. To enable: Add this to your reviews:
finishing_touches:
unit_tests:
enabled: trueTry it out by using the Have feedback? Share your thoughts on our Discord thread! Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/cc/backend/photoAlbum/service/PhotoAlbumServiceImpl.java (1)
323-323:⚠️ Potential issue | 🟠 MajorThe in-code comment indicates pending Page refactoring that remains unaddressed.
The repository method
findByMember_IdOrderByIdDescis currently defined to returnSlice<AmateurShow>(line 50 of AmateurShowRepository.java), matching the service implementation at line 323. However, the Korean comment//Page 방식 - 이름 수정 필요("Page method - name needs to be changed") explicitly documents that this should usePageinstead. This refactoring aligns with the apparent PR goal: line 324 shows a separate manual call tocountByMember_Id(memberId)to retrieve the total count, which would be provided automatically byPage<AmateurShow>. Update the repository method to returnPageand adjust the service code accordingly to eliminate the redundant count query.
🧹 Nitpick comments (5)
src/main/java/cc/backend/photoAlbum/repository/PhotoAlbumRepository.java (1)
6-6: Unused import after migration.The
Sliceimport is no longer used in this file sincefindByPerformerIdnow returnsPage. Consider removing it.🧹 Remove unused import
import org.springframework.data.domain.Page; import org.springframework.data.domain.Pageable; -import org.springframework.data.domain.Slice; import org.springframework.data.jpa.repository.JpaRepository;src/main/java/cc/backend/photoAlbum/controller/PhotoAlbumController.java (2)
5-5: Potentially unused import.The
SliceResponseimport may no longer be needed after migrating toPageResponse. Verify and remove if unused.
12-12: Unused import from protobuf.
com.google.protobuf.Apiappears to be an unused import. It's not referenced anywhere in this controller.🧹 Remove unused import
-import com.google.protobuf.Api;src/main/java/cc/backend/photoAlbum/service/PhotoAlbumServiceImpl.java (1)
414-416: Avoid usingprintStackTrace()for error logging.Using
e.printStackTrace()bypasses the application's logging framework. Replace with proper logger to maintain consistent logging and enable log aggregation.♻️ Suggested fix using a logger
Add a logger field to the class:
private static final Logger log = LoggerFactory.getLogger(PhotoAlbumServiceImpl.class);Then replace the catch block:
} catch (Exception e) { - e.printStackTrace(); + log.error("Failed to merge schedule: start={}, end={}", start, end, e); return ""; }Note: If using Lombok, you can use
@Slf4jannotation on the class instead.src/main/java/cc/backend/photoAlbum/service/PhotoAlbumService.java (1)
8-8: Potentially unused import.The
Sliceimport may no longer be needed in this interface. Verify and remove if unused.🧹 Remove unused import
import org.springframework.data.domain.Pageable; -import org.springframework.data.domain.Slice; import org.springframework.stereotype.Service;
#이슈번호,#이슈번호Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.