Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAGENTS.md가 갱신되었고, Clerk JWT 인증이 dev/prod로 분기되며 사용자별 ChangesReports API 및 인증
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant get_current_user
participant PyJWKClient
participant DB
Client->>get_current_user: Bearer 토큰 전달
get_current_user->>PyJWKClient: JWKS 서명 키 조회
PyJWKClient-->>get_current_user: 키 반환
get_current_user->>get_current_user: jwt.decode로 RS256 검증
get_current_user->>DB: clerk_user_id로 User 조회
DB-->>get_current_user: User 또는 없음
get_current_user-->>Client: User 반환 또는 401
sequenceDiagram
participant Client
participant reports.router
participant get_current_user
participant DB
Client->>reports.router: GET /api/reports?page&limit
reports.router->>get_current_user: 현재 사용자 확인
get_current_user-->>reports.router: User 반환
reports.router->>DB: total count 조회
DB-->>reports.router: total
reports.router->>DB: created_at 내림차순 목록 조회
DB-->>reports.router: reports
reports.router-->>Client: total/page/limit/reports 응답
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 5
🧹 Nitpick comments (1)
backend/app/routers/reports.py (1)
63-86: 🚀 Performance & Scalability | 🔵 Trivial인덱스 확인 권장.
user_id+is_deleted조합으로 매 요청마다 count/list 쿼리를 실행합니다. 리포트 테이블이 커질 경우(user_id, is_deleted, created_at)복합 인덱스가 있는지 확인하는 것을 권장합니다.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/routers/reports.py` around lines 63 - 86, The reports query in the `reports` router repeatedly filters by `Report.user_id` and `Report.is_deleted` and sorts by `Report.created_at`, so verify that the `Report` model/table has an appropriate composite index for this access pattern. Add or confirm an index on `Report` covering `user_id`, `is_deleted`, and `created_at` so the `total` count and paginated `stmt` query stay efficient as the table grows.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@AGENTS.md`:
- Line 100: Update the `get_current_user` documentation in AGENTS.md to match
the actual implementation: it should describe Clerk JWT verification and
returning the current user, and note that `/api/reports` uses it. Remove the
outdated note about raising 501 and not being used anywhere, and keep the
surrounding guidance consistent with the current auth flow and related symbols
like `get_current_user` and `/api/reports`.
In `@backend/app/core/deps.py`:
- Around line 22-67: `get_current_user` is missing the required `ENV=dev`
bypass. Update this dependency to check `settings.ENV` (or the existing
environment flag used elsewhere) before JWT validation, and when it is `dev`,
skip the Clerk token decode path and return the fixed development user from the
DB. Keep the bypass scoped only to `get_current_user`, preserve the existing 401
behavior for non-dev environments, and do not change any `X-Admin-Key` handling.
- Around line 44-48: The exception handling in the token verification path is
too broad and drops the original traceback. Update the logic in the
authentication dependency around the JWT/JWKS validation flow to catch only the
expected verification/authentication errors instead of a blanket Exception, and
preserve the original exception by re-raising the HTTPException with explicit
exception chaining. Keep server/config/JWKS failures distinguishable from
invalid-token cases by handling them separately in the relevant dependency
function.
- Around line 34-42: `verify_clerk_token`에서 매 요청마다 새로 만드는 `jwt.PyJWKClient` 사용을
제거하고, 모듈 레벨 싱글턴이나 재사용 가능한 공용 의존성으로 JWKS 클라이언트를 공유하도록 바꾸세요.
`settings.CLERK_JWKS_URL`로 생성한 `PyJWKClient`를 한 번만 만들고,
`get_signing_key_from_jwt` 호출은 그 인스턴스를 통해 수행해 캐시가 유지되게 하세요.
In `@backend/app/routers/reports.py`:
- Around line 83-86: The reports query currently orders only by
Report.created_at, which can make pagination unstable when multiple rows share
the same timestamp. Update the ordering in the reports listing query to add a
unique tie-breaker such as Report.id after created_at, so the result order is
deterministic across pages. Use the query chain in the reports router where
Report.created_at.desc() is applied and extend it with an additional descending
sort on the unique identifier.
---
Nitpick comments:
In `@backend/app/routers/reports.py`:
- Around line 63-86: The reports query in the `reports` router repeatedly
filters by `Report.user_id` and `Report.is_deleted` and sorts by
`Report.created_at`, so verify that the `Report` model/table has an appropriate
composite index for this access pattern. Add or confirm an index on `Report`
covering `user_id`, `is_deleted`, and `created_at` so the `total` count and
paginated `stmt` query stay efficient as the table grows.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 42b3ca1e-a4bd-4f0c-b821-bbd267e42231
📒 Files selected for processing (6)
AGENTS.mdbackend/app/core/deps.pybackend/app/main.pybackend/app/routers/__init__.pybackend/app/routers/reports.pybackend/app/schemas/reports.py
| except Exception: | ||
| raise HTTPException( | ||
| status_code=status.HTTP_401_UNAUTHORIZED, | ||
| detail="유효하지 않은 인증 토큰입니다", | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
포괄적 except Exception 및 raise ... from 누락.
모든 예외(네트워크 오류, 잘못된 CLERK_JWKS_URL 설정, JWKS 파싱 실패 등)를 동일하게 "유효하지 않은 인증 토큰입니다" 401로 뭉뚱그리고, 원본 트레이스백도 버려집니다(Ruff BLE001/B904). 서버 설정 오류까지 401로 감춰 디버깅이 어려워질 수 있습니다.
As per static analysis hints (Ruff BLE001, B904).
🛠️ Proposed fix
- except Exception:
+ except jwt.PyJWTError as exc:
raise HTTPException(
status_code=status.HTTP_401_UNAUTHORIZED,
detail="유효하지 않은 인증 토큰입니다",
- )
+ ) from exc📝 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.
| except Exception: | |
| raise HTTPException( | |
| status_code=status.HTTP_401_UNAUTHORIZED, | |
| detail="유효하지 않은 인증 토큰입니다", | |
| ) | |
| except jwt.PyJWTError as exc: | |
| raise HTTPException( | |
| status_code=status.HTTP_401_UNAUTHORIZED, | |
| detail="유효하지 않은 인증 토큰입니다", | |
| ) from exc |
🧰 Tools
🪛 Ruff (0.15.20)
[warning] 44-44: Do not catch blind exception: Exception
(BLE001)
[warning] 45-48: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/app/core/deps.py` around lines 44 - 48, The exception handling in the
token verification path is too broad and drops the original traceback. Update
the logic in the authentication dependency around the JWT/JWKS validation flow
to catch only the expected verification/authentication errors instead of a
blanket Exception, and preserve the original exception by re-raising the
HTTPException with explicit exception chaining. Keep server/config/JWKS failures
distinguishable from invalid-token cases by handling them separately in the
relevant dependency function.
Source: Linters/SAST tools
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/app/core/deps.py`:
- Line 131: The User query filter in the dependency logic uses a boolean
comparison that triggers Ruff E712 and is inconsistent with the rest of the PR.
Update the filter in the relevant dependency function that builds the User
lookup to use User.is_deleted.is_(False) instead of comparing to False directly,
matching the style already used in reports.py and keeping the SQLAlchemy boolean
check consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a2bd7292-e358-4c54-86ea-a1b21812c3c3
📒 Files selected for processing (4)
backend/app/core/deps.pybackend/app/main.pybackend/app/routers/__init__.pybackend/app/routers/reports.py
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/app/main.py
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/app/core/deps.py`:
- Line 131: The User query filter in the dependency logic uses a boolean
comparison that triggers Ruff E712 and is inconsistent with the rest of the PR.
Update the filter in the relevant dependency function that builds the User
lookup to use User.is_deleted.is_(False) instead of comparing to False directly,
matching the style already used in reports.py and keeping the SQLAlchemy boolean
check consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a2bd7292-e358-4c54-86ea-a1b21812c3c3
📒 Files selected for processing (4)
backend/app/core/deps.pybackend/app/main.pybackend/app/routers/__init__.pybackend/app/routers/reports.py
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/app/main.py
🛑 Comments failed to post (1)
backend/app/core/deps.py (1)
131-131: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
== False대신.is_(False)사용.Ruff E712 오류이며, 같은 PR의
reports.py는Report.is_deleted.is_(False)를 쓰고 있어 스타일도 불일치합니다.🛠️ 제안 수정
- .filter(User.clerk_user_id == clerk_user_id, User.is_deleted == False) + .filter(User.clerk_user_id == clerk_user_id, User.is_deleted.is_(False))📝 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..filter(User.clerk_user_id == clerk_user_id, User.is_deleted.is_(False))🧰 Tools
🪛 Ruff (0.15.20)
[error] 131-131: Avoid equality comparisons to
False; usenot User.is_deleted:for false checksReplace with
not User.is_deleted(E712)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/core/deps.py` at line 131, The User query filter in the dependency logic uses a boolean comparison that triggers Ruff E712 and is inconsistent with the rest of the PR. Update the filter in the relevant dependency function that builds the User lookup to use User.is_deleted.is_(False) instead of comparing to False directly, matching the style already used in reports.py and keeping the SQLAlchemy boolean check consistent.Source: Linters/SAST tools
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
2 participants
Summary by CodeRabbit