Merged
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR introduces CASL-based access control for student posts, refactors attachment handling to async operations with S3 integration, and removes the uploads controller. Establishes authorization types and ability definitions for role-specific permissions while updating services to enforce access control via AppAbility instead of ad-hoc checks. Changes
Sequence DiagramsequenceDiagram
participant Client
participant StudentPostsService
participant AbilityBuilder
participant AppAbility
participant Repository
participant Database
Client->>StudentPostsService: GET /posts (with userType, profileId)
StudentPostsService->>AbilityBuilder: buildAbility(userType, profileId)
AbilityBuilder->>AppAbility: defineStudentPostAbility(context)
AppAbility-->>StudentPostsService: AppAbility instance
StudentPostsService->>AppAbility: accessibleBy(ability, 'read')
AppAbility-->>StudentPostsService: accessFilter (Prisma condition)
StudentPostsService->>Repository: findMany({...params, accessFilter})
Repository->>Database: query StudentPost with accessFilter
Database-->>Repository: filtered posts
Repository-->>StudentPostsService: posts data
StudentPostsService->>StudentPostsService: normalizeAttachments(async)
StudentPostsService-->>Client: posts with presigned URLs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 (1)
src/services/comments.service.ts (1)
593-627:⚠️ Potential issue | 🟠 MajorRisk of data loss if new file upload fails after deleting old attachments.
Old attachments are deleted from S3 (lines 599-603) before new files are uploaded (lines 608-623). If an upload fails mid-way, the previously deleted files are unrecoverable. Consider uploading new files first, then deleting old ones on success.
🛡️ Proposed fix to upload before delete
- // 새 파일이 업로드될 경우, 기존 직접 첨부 파일(직접 업로드된 파일) S3에서 삭제 (고아 파일 방지) - if (files?.length) { - const existingDirectAttachments = - await this.commentsRepository.findDirectAttachmentsByCommentId( - commentId, - ); - for (const attachment of existingDirectAttachments) { - if (attachment.fileUrl) { - await this.fileStorageService.delete(attachment.fileUrl); - } - } - } - // 직접 첨부 파일 처리 (S3 업로드) const uploadedAttachments: { filename: string; fileUrl: string }[] = []; if (files?.length) { for (const file of files) { const randomId = randomUUID(); const ext = path.extname(file.originalname); const now = new Date(); const year = now.getFullYear(); const month = String(now.getMonth() + 1).padStart(2, '0'); const key = `attachments/${year}/${month}/${randomId}${ext}`; const fileUrl = await this.fileStorageService.upload(file, key); uploadedAttachments.push({ filename: file.originalname, fileUrl, }); } + + // 새 파일 업로드 성공 후 기존 직접 첨부 파일 삭제 (고아 파일 방지) + const existingDirectAttachments = + await this.commentsRepository.findDirectAttachmentsByCommentId( + commentId, + ); + await Promise.all( + existingDirectAttachments + .filter((a) => a.fileUrl) + .map((a) => this.fileStorageService.delete(a.fileUrl!)), + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/comments.service.ts` around lines 593 - 627, The current flow deletes existing direct attachments via commentsRepository.findDirectAttachmentsByCommentId and fileStorageService.delete before uploading new files, risking data loss if uploads fail; change the logic in the method handling attachments so you first perform all uploads into uploadedAttachments using fileStorageService.upload (and if any upload fails, cleanup any successfully uploaded files), only after all uploads succeed fetch existingDirectAttachments and delete them via fileStorageService.delete, then merge attachments (materialAttachments + uploadedAttachments) and persist; ensure to reference and update the same symbols: uploadedAttachments, existingDirectAttachments, fileStorageService.upload, fileStorageService.delete, and commentsRepository.findDirectAttachmentsByCommentId, and wrap DB changes in a transaction or equivalent to keep state consistent.
🧹 Nitpick comments (6)
src/casl/student-post.ability.test.ts (1)
7-148: Good test coverage for core permission scenarios.The test suite covers the essential positive and negative cases across all four user roles. The use of
subject()helper ensures type-safe subject wrapping.Consider adding tests for edge cases in a follow-up:
CreateandUpdateStatusactions (currently onlyRead,Update,Deletetested)- Empty
enrollmentIdsarray for STUDENT/PARENTListaction verification,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/casl/student-post.ability.test.ts` around lines 7 - 148, Add edge-case tests to src/casl/student-post.ability.test.ts: extend the existing Describe blocks (STUDENT, INSTRUCTOR, ASSISTANT, PARENT) to include assertions for Action.Create and Action.UpdateStatus where relevant using the same subject('StudentPost', ...) helper and the existing defineStudentPostAbility instances, add tests that construct STUDENT/PARENT abilities with empty enrollmentIds/parentEnrollmentIds arrays to assert denied permissions, and include at least one List action test (Action.List) per role to verify listing behavior; reference the Action enum, defineStudentPostAbility, and subject('StudentPost', ...) when adding these new it() cases.src/services/comments.service.ts (2)
544-560: Same parallelization opportunity indeleteCommentAttachmentsByPostId.⚡ Proposed parallelization
async deleteCommentAttachmentsByPostId( postId: string, postType: 'instructorPost' | 'studentPost', ) { const attachments = await this.commentsRepository.findDirectAttachmentsByPostId( postId, postType, ); - for (const attachment of attachments) { - if (attachment.fileUrl) { - await this.fileStorageService.delete(attachment.fileUrl); - } - } + await Promise.all( + attachments + .filter((a) => a.fileUrl) + .map((a) => this.fileStorageService.delete(a.fileUrl!)), + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/comments.service.ts` around lines 544 - 560, The loop in deleteCommentAttachmentsByPostId serially awaits fileStorageService.delete for each attachment which is slow; fetch attachments via commentsRepository.findDirectAttachmentsByPostId as before but replace the for-await pattern with a parallel delete using Promise.all over attachments.filter(a => a.fileUrl).map(a => this.fileStorageService.delete(a.fileUrl)), or use a bounded concurrency helper if needed for rate-limiting, so deletes run concurrently and the method awaits their completion.
532-540: Consider parallelizing S3 delete operations.Sequential
awaitin a loop can be slow when deleting multiple attachments. UsePromise.allfor concurrent deletions.⚡ Proposed parallelization
// 직접 업로드된 첨부 파일을 S3에서 삭제 (고아 파일 방지) const directAttachments = await this.commentsRepository.findDirectAttachmentsByCommentId(commentId); - for (const attachment of directAttachments) { - if (attachment.fileUrl) { - await this.fileStorageService.delete(attachment.fileUrl); - } - } + await Promise.all( + directAttachments + .filter((a) => a.fileUrl) + .map((a) => this.fileStorageService.delete(a.fileUrl!)), + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/comments.service.ts` around lines 532 - 540, The loop that deletes direct attachments is performing sequential awaits which is slow; update the deletion to run in parallel by mapping directAttachments (from commentsRepository.findDirectAttachmentsByCommentId) to delete promises via fileStorageService.delete for each attachment.fileUrl (filter out falsy fileUrl) and then await Promise.all on that array so all S3 deletions run concurrently; ensure you still await the Promise.all to propagate errors correctly.src/repos/comments.repo.ts (1)
159-181: Consider adding transaction client support for consistency.Other methods in this repository accept an optional
tx?: Prisma.TransactionClientparameter, but these new methods always usethis.prismadirectly. This could cause issues if these methods need to be called within a transaction context in the future (e.g., during cascading deletes).♻️ Proposed change to add transaction support
- async findDirectAttachmentsByCommentId(commentId: string) { - return this.prisma.commentAttachment.findMany({ + async findDirectAttachmentsByCommentId(commentId: string, tx?: Prisma.TransactionClient) { + const client = tx ?? this.prisma; + return client.commentAttachment.findMany({ where: { commentId, materialId: null }, select: { id: true, fileUrl: true, filename: true }, }); } /** 특정 게시글의 모든 댓글에 딸린 직접 첨부 파일 조회 (post 삭제 시 S3 정리용) */ async findDirectAttachmentsByPostId( postId: string, postType: 'instructorPost' | 'studentPost', + tx?: Prisma.TransactionClient, ) { - return this.prisma.commentAttachment.findMany({ + const client = tx ?? this.prisma; + return client.commentAttachment.findMany({ where: { comment: { [postType + 'Id']: postId, }, materialId: null, }, select: { fileUrl: true }, }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/repos/comments.repo.ts` around lines 159 - 181, Add optional transaction client support to the two new repo methods so they behave like other methods: update findDirectAttachmentsByCommentId and findDirectAttachmentsByPostId to accept an optional tx?: Prisma.TransactionClient parameter and use (tx ?? this.prisma) for the query instead of directly calling this.prisma; keep the existing where/select logic and parameter types (commentId, postId, postType) unchanged so these methods can be used inside transactions for cascading deletes.src/services/student-posts.service.ts (2)
444-451: Consider parallelizing S3 delete operations.Sequential
awaitin a loop can be slow when deleting multiple attachments.⚡ Proposed parallelization
// S3 첨부파일 삭제 if (post.attachments?.length) { - for (const attachment of post.attachments) { - if (attachment.fileUrl) { - await this.fileStorageService.delete(attachment.fileUrl); - } - } + await Promise.all( + post.attachments + .filter((a) => a.fileUrl) + .map((a) => this.fileStorageService.delete(a.fileUrl!)), + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/student-posts.service.ts` around lines 444 - 451, The current deletion loop in student-posts.service.ts sequentially awaits this.fileStorageService.delete for each attachment, slowing down removals; change it to run deletes in parallel by collecting promises from post.attachments.filter(a => a.fileUrl).map(a => this.fileStorageService.delete(a.fileUrl)) and then awaiting them together (use Promise.all or Promise.allSettled to avoid one failure blocking others). Ensure you still only attempt deletes when post.attachments exists and has fileUrl entries, and handle or log failures from fileStorageService.delete appropriately.
136-158: Inconsistent early-return logic for PARENT vs ability-based access control.For PARENT users, there's explicit early return logic at lines 136-146 that checks for childLinks and enrollmentIds. However, the ability system at line 149 would also handle this case (empty
parentEnrollmentIdswould result in an ability that blocks access). Consider either:
- Removing the early returns and letting the ability system handle all access decisions consistently, or
- Adding similar early-exit logic for STUDENT users when they have no enrollments
The current approach mixes two access-control paradigms.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/student-posts.service.ts` around lines 136 - 158, The early-return checks for PARENT (permissionService.getChildLinks and permissionService.getParentEnrollmentIds) are mixing imperative checks with the ability-based access control built via buildAbility and accessibleBy used later; remove the PARENT-specific early-return logic (the childLinks/enrollmentIds checks and the return { posts: [], ... }) so all access decisions are consistently enforced by buildAbility/accessibleBy and studentPostsRepository.findMany, or alternatively implement the equivalent early-exit for STUDENT to mirror the parent logic—pick one approach and make the code for permissionService.getChildLinks, permissionService.getParentEnrollmentIds, buildAbility, accessibleBy and the studentPostsRepository.findMany access flow consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/casl/student-post.ability.ts`:
- Around line 35-46: The non-null assertions on ctx.effectiveInstructorId used
when building the condition for UT.INSTRUCTOR and UT.ASSISTANT (the condition
passed to can(A.Read, 'StudentPost', condition) and can(A.List, 'StudentPost',
condition)) are unsafe; add an explicit guard that validates
ctx.effectiveInstructorId before creating condition and calling can — if the id
is missing, either throw a clear error or skip/deny granting those StudentPost
abilities (returning no condition) so you don't pass { instructorId: undefined }
into downstream queries. Ensure the check is applied in both the UT.INSTRUCTOR
and UT.ASSISTANT branches where ctx.effectiveInstructorId is referenced.
In `@src/services/student-posts.service.ts`:
- Around line 390-397: The current code deletes existing attachments (using
this.fileStorageService.delete on post.attachments) before new file uploads
complete, risking data loss if uploads fail; change the flow so new files are
uploaded first (collect their returned fileUrls), update the post's attachments
in the DB with the new attachment entries, and only after a successful upload+DB
update delete the old attachment fileUrls from storage (use post.attachments to
identify oldUrls and this.fileStorageService.delete to remove them). Ensure the
sequence is: upload new files, persist new attachment metadata, then delete old
files — and handle/rollback on upload or DB errors to avoid leaving the post
without valid attachments.
---
Outside diff comments:
In `@src/services/comments.service.ts`:
- Around line 593-627: The current flow deletes existing direct attachments via
commentsRepository.findDirectAttachmentsByCommentId and
fileStorageService.delete before uploading new files, risking data loss if
uploads fail; change the logic in the method handling attachments so you first
perform all uploads into uploadedAttachments using fileStorageService.upload
(and if any upload fails, cleanup any successfully uploaded files), only after
all uploads succeed fetch existingDirectAttachments and delete them via
fileStorageService.delete, then merge attachments (materialAttachments +
uploadedAttachments) and persist; ensure to reference and update the same
symbols: uploadedAttachments, existingDirectAttachments,
fileStorageService.upload, fileStorageService.delete, and
commentsRepository.findDirectAttachmentsByCommentId, and wrap DB changes in a
transaction or equivalent to keep state consistent.
---
Nitpick comments:
In `@src/casl/student-post.ability.test.ts`:
- Around line 7-148: Add edge-case tests to
src/casl/student-post.ability.test.ts: extend the existing Describe blocks
(STUDENT, INSTRUCTOR, ASSISTANT, PARENT) to include assertions for Action.Create
and Action.UpdateStatus where relevant using the same subject('StudentPost',
...) helper and the existing defineStudentPostAbility instances, add tests that
construct STUDENT/PARENT abilities with empty enrollmentIds/parentEnrollmentIds
arrays to assert denied permissions, and include at least one List action test
(Action.List) per role to verify listing behavior; reference the Action enum,
defineStudentPostAbility, and subject('StudentPost', ...) when adding these new
it() cases.
In `@src/repos/comments.repo.ts`:
- Around line 159-181: Add optional transaction client support to the two new
repo methods so they behave like other methods: update
findDirectAttachmentsByCommentId and findDirectAttachmentsByPostId to accept an
optional tx?: Prisma.TransactionClient parameter and use (tx ?? this.prisma) for
the query instead of directly calling this.prisma; keep the existing
where/select logic and parameter types (commentId, postId, postType) unchanged
so these methods can be used inside transactions for cascading deletes.
In `@src/services/comments.service.ts`:
- Around line 544-560: The loop in deleteCommentAttachmentsByPostId serially
awaits fileStorageService.delete for each attachment which is slow; fetch
attachments via commentsRepository.findDirectAttachmentsByPostId as before but
replace the for-await pattern with a parallel delete using Promise.all over
attachments.filter(a => a.fileUrl).map(a =>
this.fileStorageService.delete(a.fileUrl)), or use a bounded concurrency helper
if needed for rate-limiting, so deletes run concurrently and the method awaits
their completion.
- Around line 532-540: The loop that deletes direct attachments is performing
sequential awaits which is slow; update the deletion to run in parallel by
mapping directAttachments (from
commentsRepository.findDirectAttachmentsByCommentId) to delete promises via
fileStorageService.delete for each attachment.fileUrl (filter out falsy fileUrl)
and then await Promise.all on that array so all S3 deletions run concurrently;
ensure you still await the Promise.all to propagate errors correctly.
In `@src/services/student-posts.service.ts`:
- Around line 444-451: The current deletion loop in student-posts.service.ts
sequentially awaits this.fileStorageService.delete for each attachment, slowing
down removals; change it to run deletes in parallel by collecting promises from
post.attachments.filter(a => a.fileUrl).map(a =>
this.fileStorageService.delete(a.fileUrl)) and then awaiting them together (use
Promise.all or Promise.allSettled to avoid one failure blocking others). Ensure
you still only attempt deletes when post.attachments exists and has fileUrl
entries, and handle or log failures from fileStorageService.delete
appropriately.
- Around line 136-158: The early-return checks for PARENT
(permissionService.getChildLinks and permissionService.getParentEnrollmentIds)
are mixing imperative checks with the ability-based access control built via
buildAbility and accessibleBy used later; remove the PARENT-specific
early-return logic (the childLinks/enrollmentIds checks and the return { posts:
[], ... }) so all access decisions are consistently enforced by
buildAbility/accessibleBy and studentPostsRepository.findMany, or alternatively
implement the equivalent early-exit for STUDENT to mirror the parent logic—pick
one approach and make the code for permissionService.getChildLinks,
permissionService.getParentEnrollmentIds, buildAbility, accessibleBy and the
studentPostsRepository.findMany access flow consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: aaf0e5d1-b5fb-4df1-b7b9-55b4069e5dd5
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (18)
package.jsonsrc/casl/ability.types.tssrc/casl/actions.tssrc/casl/student-post.ability.test.tssrc/casl/student-post.ability.tssrc/casl/subjects.tssrc/config/container.config.tssrc/controllers/uploads.controller.tssrc/repos/comments.repo.tssrc/repos/student-posts.repo.tssrc/routes/svc/v1/index.tssrc/routes/svc/v1/student-posts.route.tssrc/routes/svc/v1/uploads.route.tssrc/services/comments.service.tssrc/services/instructor-posts.service.tssrc/services/student-posts.service.test.tssrc/services/student-posts.service.tssrc/test/mocks/services.mock.ts
💤 Files with no reviewable changes (4)
- src/routes/svc/v1/index.ts
- src/routes/svc/v1/uploads.route.ts
- src/controllers/uploads.controller.ts
- src/config/container.config.ts
play-ancora-gyungmin
approved these changes
Mar 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🔗 관련 이슈
✨ 변경 사항
🧪 테스트 방법
리뷰어가 어떻게 테스트하면 되는지 적어주세요.
📸 스크린샷 (선택)
UI 변경이 있다면 첨부해주세요.
✅ 체크리스트
Summary by CodeRabbit
Release Notes
New Features
Improvements
Removals