[Fix] canvas 이미지 URL 변환 오류 수정 - presigned URL로 변환 후 AI 처리 가능#199
[Fix] canvas 이미지 URL 변환 오류 수정 - presigned URL로 변환 후 AI 처리 가능#199
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. 📝 WalkthroughWalkthroughChatServiceImpl의 채팅 초기화 시 asset URL 변환 로직을 수정하여, mentionedAssetIds와 canvasImageIds를 모두 집계한 통합 목록(allAssetIdsForUrl)을 생성 후 convertAssetIdsToUrls에 전달합니다. 로그 메시지도 두 asset ID 그룹과 결과 URL을 반영하도록 업데이트되었습니다. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
🧹 Nitpick comments (1)
src/main/java/com/proovy/domain/conversation/service/ChatServiceImpl.java (1)
183-190:allAssetIdsForUrl변수 제거 및 중복 DB 조회 개선 권고
allAssetIdsForUrl(183-189)은 이미 144~151라인에서 동일한 로직으로 구성된allAssetIds와 완전히 동일한 값을 가집니다. 같은 람다 스코프 내에서allAssetIds를 그대로 재사용할 수 있으므로,allAssetIdsForUrl블록 전체가 불필요한 중복 코드입니다.또한
convertAssetIdsToUrls(490번 라인)는 내부적으로assetRepository.findAllByIdInAndUserId를 다시 호출하므로, 153번 라인에서 이미 수행된 동일한 DB 쿼리가 같은 트랜잭션 안에서 두 번 실행됩니다.♻️ allAssetIdsForUrl 제거 및 allAssetIds 재사용 제안
- // 6. 자산 URL 변환 (mentionedAssetIds + canvasImageIds 모두 포함) - List<Long> allAssetIdsForUrl = new ArrayList<>(); - if (request.getMentionedAssetIds() != null) { - allAssetIdsForUrl.addAll(request.getMentionedAssetIds()); - } - if (request.getCanvasImageIds() != null) { - allAssetIdsForUrl.addAll(request.getCanvasImageIds()); - } - List<String> filesUrl = convertAssetIdsToUrls(allAssetIdsForUrl, userId); + // 6. 자산 URL 변환 (4-1에서 구성한 allAssetIds 재사용) + List<String> filesUrl = convertAssetIdsToUrls(allAssetIds, userId);더 나아가 중복 DB 조회를 제거하려면,
convertAssetIdsToUrls가List<Asset>을 직접 받도록 오버로드하고, 153번 라인에서 조회된assets를 전달하는 방식을 고려할 수 있습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/proovy/domain/conversation/service/ChatServiceImpl.java` around lines 183 - 190, Remove the redundant allAssetIdsForUrl block and reuse the already-built allAssetIds; stop calling convertAssetIdsToUrls with IDs that cause a second DB lookup via assetRepository.findAllByIdInAndUserId. Instead overload or add a convertAssetIdsToUrls(List<Asset> assets, Long userId) (or similar) and pass the already-fetched assets variable from the earlier query (assets) into it so the assets are not queried twice; update call sites to use the new overload and remove allAssetIdsForUrl and its population logic.
🤖 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/main/java/com/proovy/domain/conversation/service/ChatServiceImpl.java`:
- Around line 191-192: The INFO-level log in ChatServiceImpl (the log statement
that logs filesUrl) exposes S3 presigned URLs; remove the presigned URL from
that log or downgrade it to DEBUG and log only non-sensitive identifiers (e.g.,
mentionedAssetIds, canvasImageIds) instead, and ensure the same change is
applied inside convertAssetIdsToUrls where the log "Presigned URL 생성 - assetId:
{}, s3Key: {}, url: {}" is emitted so that the URL value is no longer written at
INFO level (log only assetId/s3Key at INFO or include the full URL only at
DEBUG).
- Around line 183-189: The code builds allAssetIdsForUrl by concatenating
request.getMentionedAssetIds() and request.getCanvasImageIds() without
deduplication, causing duplicate asset IDs to trigger false mismatches in
convertAssetIdsToUrls and duplicate presigned URLs; fix by deduplicating after
merging (e.g., collect into a LinkedHashSet or use stream().distinct()) when
constructing allAssetIdsForUrl and apply the same deduplication to allAssetIds
(the variable created earlier) so both the URL conversion call and payload to
the AI service receive unique asset IDs; ensure references to
request.getMentionedAssetIds(), request.getCanvasImageIds(), allAssetIdsForUrl,
allAssetIds, and convertAssetIdsToUrls are updated accordingly.
---
Nitpick comments:
In `@src/main/java/com/proovy/domain/conversation/service/ChatServiceImpl.java`:
- Around line 183-190: Remove the redundant allAssetIdsForUrl block and reuse
the already-built allAssetIds; stop calling convertAssetIdsToUrls with IDs that
cause a second DB lookup via assetRepository.findAllByIdInAndUserId. Instead
overload or add a convertAssetIdsToUrls(List<Asset> assets, Long userId) (or
similar) and pass the already-fetched assets variable from the earlier query
(assets) into it so the assets are not queried twice; update call sites to use
the new overload and remove allAssetIdsForUrl and its population logic.
2 participants
📌 관련 이슈
🏷️ PR 타입
📝 작업 내용
convertAssetIdsToUrls()함수에 canvasImageIds 추가하여 AI가 이미지 다운로드 및 OCR 처리 가능하게 수정📸 스크린샷
✅ 체크리스트
📎 기타 참고사항
Summary by CodeRabbit