Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds an isRead (read-status) boolean filter to "get articles by category" across controller, application ports/usecase, service, and repository; repository query now includes article.isRead predicate. Also adds https://api.pinack.today to allowed CORS origins. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Controller as ArticleController
participant Usecase as GetArticleUsecase
participant Service as ArticleGetServicePort
participant Repo as ArticleRepositoryCustom
participant DB
Client->>Controller: GET /articles?categoryId&read-status
Controller->>Usecase: getAllArticlesByCategory(user, categoryId, isRead, page)
Usecase->>Service: findAllByCategory(user, category, isRead, pageReq)
Service->>Repo: findAllByCategory(userId, categoryId, isRead, pageable)
Repo->>DB: SELECT ... WHERE category_id=? AND is_read=?
DB-->>Repo: Rows + counts
Repo-->>Service: ArticlesWithUnreadCount
Service-->>Usecase: DTO
Usecase-->>Controller: ArticlesPageResponse
Controller-->>Client: 200 OK (filtered list)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
api/src/main/java/com/pinback/api/config/cors/CorsConfig.java (1)
36-47: Remove empty header entry from allowed headersThe empty string at Line 46 is invalid and may yield unpredictable CORS behavior.
Apply:
configuration.setAllowedHeaders(Arrays.asList( "Authorization", "Content-Type", "X-Requested-With", "Accept", "Origin", "Access-Control-Request-Method", "Access-Control-Request-Headers", "X-Forwarded-For", - "X-Real-IP", - "" + "X-Real-IP" ));infrastructure/src/main/java/com/pinback/infrastructure/article/repository/ArticleRepositoryCustomImpl.java (1)
61-88: Data leak + incorrect unread count in category query
- Missing
userIdpredicate: returns articles across all users in the category.unReadCountis computed with the sameisRead-filtered conditions; it becomes 0 whenisRead=true, and it ignores the user's scope.Apply:
- public ArticlesWithUnreadCount findAllByCategory(UUID userId, long categoryId, boolean isRead, Pageable pageable) { - - BooleanExpression conditions = article.category.id.eq(categoryId) - .and(article.isRead.eq(isRead)); + public ArticlesWithUnreadCount findAllByCategory(UUID userId, long categoryId, boolean isRead, Pageable pageable) { + // Scope to user + category first + BooleanExpression baseConditions = article.user.id.eq(userId) + .and(article.category.id.eq(categoryId)); + // Then apply read-status filter for the page content + BooleanExpression conditions = baseConditions.and(article.isRead.eq(isRead)); List<Article> articles = queryFactory .selectFrom(article) .join(article.category, category).fetchJoin() .where(conditions) .offset(pageable.getOffset()) .limit(pageable.getPageSize()) .orderBy(article.createdAt.desc()) .fetch(); JPAQuery<Long> countQuery = queryFactory .select(article.count()) .from(article) .where(conditions); - Long unReadCount = queryFactory + // Unread count should ignore the requested isRead filter + Long unReadCount = queryFactory .select(article.count()) .from(article) - .where(conditions.and(article.isRead.isFalse())) + .where(baseConditions.and(article.isRead.isFalse())) .fetchOne(); return new ArticlesWithUnreadCount(unReadCount, PageableExecutionUtils.getPage(articles, pageable, countQuery::fetchOne)); }
🧹 Nitpick comments (2)
application/src/main/java/com/pinback/application/article/port/out/ArticleGetServicePort.java (1)
35-36: Consistency check (optional)Both remind methods use
Boolean isRead; controller passes primitive. This is fine, but consider aligning both to primitive if null is never a valid state.infrastructure/src/main/java/com/pinback/infrastructure/article/repository/ArticleRepositoryCustom.java (1)
16-17: Parameter name is misleading
articleIdshould becategoryId.Apply:
- ArticlesWithUnreadCount findAllByCategory(UUID userId, long articleId, boolean isRead, Pageable pageable); + ArticlesWithUnreadCount findAllByCategory(UUID userId, long categoryId, boolean isRead, Pageable pageable);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
api/src/main/java/com/pinback/api/article/controller/ArticleController.java(1 hunks)api/src/main/java/com/pinback/api/config/cors/CorsConfig.java(1 hunks)application/src/main/java/com/pinback/application/article/port/in/GetArticlePort.java(1 hunks)application/src/main/java/com/pinback/application/article/port/out/ArticleGetServicePort.java(1 hunks)application/src/main/java/com/pinback/application/article/usecase/query/GetArticleUsecase.java(1 hunks)infrastructure/src/main/java/com/pinback/infrastructure/article/repository/ArticleRepositoryCustom.java(1 hunks)infrastructure/src/main/java/com/pinback/infrastructure/article/repository/ArticleRepositoryCustomImpl.java(2 hunks)infrastructure/src/main/java/com/pinback/infrastructure/article/service/ArticleGetService.java(2 hunks)
🔇 Additional comments (5)
application/src/main/java/com/pinback/application/article/port/out/ArticleGetServicePort.java (1)
29-29: Signature update looks goodPropagates
isReadthrough the port cleanly. Ensure implementors are updated (compile will catch).application/src/main/java/com/pinback/application/article/port/in/GetArticlePort.java (1)
19-19: Interface change acknowledgedAdding
isReadmatches the new API surface.infrastructure/src/main/java/com/pinback/infrastructure/article/repository/ArticleRepositoryCustom.java (1)
18-23: Remind queries: API shape LGTMOptional read filter is exposed as
Booleanconsistently across custom methods.application/src/main/java/com/pinback/application/article/usecase/query/GetArticleUsecase.java (1)
70-75: Use-case propagation LGTMCorrectly passes
isReadthrough to the service port; response mapping unchanged.infrastructure/src/main/java/com/pinback/infrastructure/article/service/ArticleGetService.java (1)
57-61: Service wiring LGTM
- Category listing forwards
isReadto repository.- Remind endpoints pass the date window and read filter correctly.
Also applies to: 76-87
| @Parameter(description = "읽음 상태 (true: 읽음, false: 안읽음)", example = "true") @RequestParam(name = "read-status") boolean isRead, | ||
| @Parameter(description = "페이지 번호 (0부터 시작)") @RequestParam(defaultValue = "0") int page, | ||
| @Parameter(description = "페이지 크기") @RequestParam(defaultValue = "8") int size | ||
| ) { | ||
| PageQuery query = new PageQuery(page, size); | ||
| ArticlesPageResponse response = getArticlePort.getAllArticlesByCategory(user, categoryId, query); | ||
| ArticlesPageResponse response = getArticlePort.getAllArticlesByCategory(user, categoryId, isRead, query); | ||
| return ResponseDto.ok(response); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Avoid breaking clients: make read-status optional with a default
Primitive boolean without a default makes the new query param mandatory and will 400 existing callers.
Apply:
- @Parameter(description = "읽음 상태 (true: 읽음, false: 안읽음)", example = "true") @RequestParam(name = "read-status") boolean isRead,
+ @Parameter(description = "읽음 상태 (true: 읽음, false: 안읽음). 미지정 시 기본값 false")
+ @RequestParam(name = "read-status", required = false, defaultValue = "false") boolean isRead,If the FE is already updated to always send it, confirm and keep as-is.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
target="api/src/main/java/com/pinback/api/article/controller/ArticleController.java"
if [ -f "$target" ]; then
echo "FOUND:$target"
nl -ba "$target" | sed -n '1,220p'
exit 0
fi
file=$(rg -n --hidden --no-ignore-vcs -S "class\s+ArticleController" -g 'api/**' -l || true)
if [ -n "$file" ]; then
echo "FOUND_SEARCH:$file"
nl -ba "$file" | sed -n '1,220p'
exit 0
fi
echo "NOT_FOUND"Length of output: 122
🏁 Script executed:
#!/bin/bash
set -euo pipefail
f="api/src/main/java/com/pinback/api/article/controller/ArticleController.java"
if [ ! -f "$f" ]; then
echo "NOT_FOUND:$f"
rg -n --hidden --no-ignore-vcs -S "class\s+ArticleController" -g 'api/**' || true
exit 0
fi
echo "FILE:$f"
awk '{printf "%5d: %s\n", NR, $0}' "$f" | sed -n '1,240p'
echo "----@RequestParam lines----"
awk '/@RequestParam/ {printf "%5d: %s\n", NR, $0}' "$f" || trueLength of output: 9764
Make read-status query param optional (default false)
Primitive boolean params are required by default and will 400 callers that don't send them. Two occurrences found in api/src/main/java/com/pinback/api/article/controller/ArticleController.java — update both:
- getAllArticlesByCategory(...)
- getRemindArticles(...)
Apply (or equivalent) for each occurrence:
- @Parameter(description = "읽음 상태 (true: 읽음, false: 안읽음)", example = "true") @RequestParam(name = "read-status") boolean isRead,
+ @Parameter(description = "읽음 상태 (true: 읽음, false: 안읽음). 미지정 시 기본값 false", example = "true")
+ @RequestParam(name = "read-status", required = false, defaultValue = "false") boolean isRead,- @Parameter(description = "읽음 상태 (true: 읽음, false: 안읽음)", example = "true") @RequestParam(name = "read-status") boolean readStatus,
+ @Parameter(description = "읽음 상태 (true: 읽음, false: 안읽음). 미지정 시 기본값 false", example = "true")
+ @RequestParam(name = "read-status", required = false, defaultValue = "false") boolean readStatus,If the frontend is guaranteed to always send this param, confirm and keep as-is.
📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Parameter(description = "읽음 상태 (true: 읽음, false: 안읽음)", example = "true") @RequestParam(name = "read-status") boolean isRead, | |
| @Parameter(description = "페이지 번호 (0부터 시작)") @RequestParam(defaultValue = "0") int page, | |
| @Parameter(description = "페이지 크기") @RequestParam(defaultValue = "8") int size | |
| ) { | |
| PageQuery query = new PageQuery(page, size); | |
| ArticlesPageResponse response = getArticlePort.getAllArticlesByCategory(user, categoryId, query); | |
| ArticlesPageResponse response = getArticlePort.getAllArticlesByCategory(user, categoryId, isRead, query); | |
| return ResponseDto.ok(response); | |
| @Parameter(description = "읽음 상태 (true: 읽음, false: 안읽음). 미지정 시 기본값 false", example = "true") | |
| @RequestParam(name = "read-status", required = false, defaultValue = "false") boolean isRead, | |
| @Parameter(description = "페이지 번호 (0부터 시작)") @RequestParam(defaultValue = "0") int page, | |
| @Parameter(description = "페이지 크기") @RequestParam(defaultValue = "8") int size | |
| ) { | |
| PageQuery query = new PageQuery(page, size); | |
| ArticlesPageResponse response = getArticlePort.getAllArticlesByCategory(user, categoryId, isRead, query); | |
| return ResponseDto.ok(response); |
| "https://www.pinback.today", | ||
| "https://api.pinack.today" | ||
| )); |
There was a problem hiding this comment.
Fix origin typo: api.pinack.today → api.pinback.today
Likely a blocking CORS bug for the production API domain.
Apply:
- "https://api.pinack.today"
+ "https://api.pinback.today"📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "https://www.pinback.today", | |
| "https://api.pinack.today" | |
| )); | |
| "https://www.pinback.today", | |
| "https://api.pinback.today" | |
| )); |
🤖 Prompt for AI Agents
In api/src/main/java/com/pinback/api/config/cors/CorsConfig.java around lines
28-30, the allowed origins list contains a typo "https://api.pinack.today" that
should be "https://api.pinback.today"; update that string to the correct domain
and run the app/CORS tests to verify the production API origin is accepted (also
scan the file for any other occurrences of the misspelled domain and correct
them).
1 participant
🚀 PR 요약
카테고리별 아티클 조회시 읽음 여부를 포함하여 조회하도록 구현했습니다.
✨ PR 상세 내용
🚨 주의 사항
없습니다
✅ 체크 리스트
Summary by CodeRabbit
New Features
Chores