Open
Conversation
1 task
📝 WalkthroughWalkthrough이미지 디코딩/회전 로직이 개선되어 두 단계 디코딩(inJustDecodeBounds + 샘플링)과 public한 Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 상세 리뷰긍정적인 부분
검토 권장사항 (왜 문제가 되는가 → 어떻게 개선할지)
전반적으로 구현 방향은 적절하며 메모리·취소 안전성·상태 일관성 측면에서 향상된 부분이 보입니다. 위의 검증 포인트를 보완하면 안정성과 문서화가 더욱 좋아질 것입니다. 🚥 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)
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: 1
🧹 Nitpick comments (1)
core/ui/src/main/java/com/twix/ui/image/ImageGenerator.kt (1)
66-69: 타겟 해상도(1920x1080) 하드코딩에 대한 제안현재 FHD(1920x1080)가 하드코딩되어 있습니다. PR 설명에서 리뷰어에게 이 기준에 대한 의견을 요청하셨는데, FHD는 대부분의 모바일 화면에서 충분한 해상도이므로 합리적인 선택으로 보입니다.
다만, 향후 유지보수성을 위해 상수로 추출하는 것은 어떨까요?
♻️ 상수 추출 제안
companion object { private const val IMAGE_COMPRESSION_ERROR_MESSAGE = "Config: %s, Size: %dx%d" + private const val TARGET_WIDTH = 1920 + private const val TARGET_HEIGHT = 1080 }그리고 사용처에서:
- inSampleSize = calculateInSampleSize(bounds, 1920, 1080) + inSampleSize = calculateInSampleSize(bounds, TARGET_WIDTH, TARGET_HEIGHT)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/ui/src/main/java/com/twix/ui/image/ImageGenerator.kt` around lines 66 - 69, The code in ImageGenerator.kt hardcodes the 1920x1080 target resolution when creating BitmapFactory.Options (inSampleSize = calculateInSampleSize(bounds, 1920, 1080)); extract these magic numbers into named constants (e.g., TARGET_WIDTH and TARGET_HEIGHT) either as top-level constants or in a companion object of ImageGenerator, replace the literals in the calculateInSampleSize call with those constants, and update any related usages/comments to reference the new constants so the target resolution is configurable and easier to maintain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/ui/src/main/java/com/twix/ui/image/ImageGenerator.kt`:
- Around line 60-74: In uriToBitmap, validate the first-pass bounds before
calling calculateInSampleSize: after the inJustDecodeBounds decode, check
bounds.outWidth and bounds.outHeight for <= 0 (or -1) and if invalid throw
ImageProcessException.DecodeFailedException(imageUri) so you don't proceed with
a bogus inSampleSize; then continue to create the real BitmapFactory.Options and
call calculateInSampleSize only when bounds are valid (reference symbols:
uriToBitmap, bounds.outWidth, bounds.outHeight, calculateInSampleSize,
ImageProcessException.DecodeFailedException).
---
Nitpick comments:
In `@core/ui/src/main/java/com/twix/ui/image/ImageGenerator.kt`:
- Around line 66-69: The code in ImageGenerator.kt hardcodes the 1920x1080
target resolution when creating BitmapFactory.Options (inSampleSize =
calculateInSampleSize(bounds, 1920, 1080)); extract these magic numbers into
named constants (e.g., TARGET_WIDTH and TARGET_HEIGHT) either as top-level
constants or in a companion object of ImageGenerator, replace the literals in
the calculateInSampleSize call with those constants, and update any related
usages/comments to reference the new constants so the target resolution is
configurable and easier to maintain.
- `capture` 시 `CancellableContinuation`의 활성 상태(`isActive`)를 확인하여 불필요한 resume 방지 - `unbind` 호출 시 `surfaceRequests`를 초기화하도록 수정 - `bindToLifecycle` 시 기존에 생성된 `cameraProvider`가 있다면 재사용하도록 개선
There was a problem hiding this comment.
🧹 Nitpick comments (1)
feature/photolog/capture/src/main/java/com/twix/photolog/capture/model/camera/CaptureCamera.kt (1)
73-83: 코루틴 취소 시 리소스 정리를 위한invokeOnCancellation핸들러 추가를 고려해보세요.현재 구현에서
isActive체크는 취소된 continuation에 resume하는 것을 방지해주지만, 코루틴이 취소되더라도 CameraX의takePicture작업은 백그라운드에서 계속 진행됩니다. 이로 인해 사용자가 화면을 떠난 후에도 이미지가 MediaStore에 저장될 수 있습니다.CameraX의
takePicture는 자체적인 취소 API를 제공하지 않지만,invokeOnCancellation을 통해 최소한 로깅이나 상태 추적은 가능합니다:suspendCancellableCoroutine { continuation -> continuation.invokeOnCancellation { // 취소 시 로깅 또는 필요한 정리 작업 Log.d("CaptureCamera", "Image capture was cancelled") } // ... 기존 코드 }이 부분은 현재 동작에 문제를 일으키지는 않으므로, 향후 개선 사항으로 검토해주시면 됩니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/photolog/capture/src/main/java/com/twix/photolog/capture/model/camera/CaptureCamera.kt` around lines 73 - 83, Add an invokeOnCancellation handler to the suspendCancellableCoroutine in CaptureCamera.takePicture() so that when the coroutine is cancelled you can perform cleanup/logging; inside the suspendCancellableCoroutine block (where outputOptions is created and imageCapture.takePicture(...) is called with capture(continuation)), call continuation.invokeOnCancellation { /* log cancellation and/or update state to avoid leftover background work */ } to track or log cancellations and prevent silent background saves.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@feature/photolog/capture/src/main/java/com/twix/photolog/capture/model/camera/CaptureCamera.kt`:
- Around line 73-83: Add an invokeOnCancellation handler to the
suspendCancellableCoroutine in CaptureCamera.takePicture() so that when the
coroutine is cancelled you can perform cleanup/logging; inside the
suspendCancellableCoroutine block (where outputOptions is created and
imageCapture.takePicture(...) is called with capture(continuation)), call
continuation.invokeOnCancellation { /* log cancellation and/or update state to
avoid leftover background work */ } to track or log cancellations and prevent
silent background saves.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: e8ff4419-a7cf-4de0-9043-b4cfe746557d
📒 Files selected for processing (1)
feature/photolog/capture/src/main/java/com/twix/photolog/capture/model/camera/CaptureCamera.kt
There was a problem hiding this comment.
🧹 Nitpick comments (1)
feature/photolog/capture/src/main/java/com/twix/photolog/capture/PhotologCaptureViewModel.kt (1)
87-96: 상태 읽기와 업데이트 사이의 일관성 개선 제안현재
currentState.lens를reduce블록 외부에서 읽고 있습니다. 이론적으로 외부 읽기와reduce호출 사이에 다른 상태 변경이 발생하면 의도하지 않은 동작이 발생할 수 있습니다.
reduce블록 내부에서는this가 현재 상태를 참조하므로, 상태 기반 계산을 블록 내부로 이동하면 더 안전합니다. 현재 코드가 실질적으로 문제를 일으킬 가능성은 낮지만, MVI 패턴의 일관성을 위해 고려해볼 수 있습니다.♻️ reduce 블록 내부로 로직 이동 제안
private fun reduceLens() { - val newLens = - if (currentState.lens == CameraSelector.DEFAULT_BACK_CAMERA) { - CameraSelector.DEFAULT_FRONT_CAMERA - } else { - CameraSelector.DEFAULT_BACK_CAMERA - } - - reduce { copy(lens = newLens, torch = TorchStatus.Off) } + reduce { + val newLens = + if (lens == CameraSelector.DEFAULT_BACK_CAMERA) { + CameraSelector.DEFAULT_FRONT_CAMERA + } else { + CameraSelector.DEFAULT_BACK_CAMERA + } + copy(lens = newLens, torch = TorchStatus.Off) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/photolog/capture/src/main/java/com/twix/photolog/capture/PhotologCaptureViewModel.kt` around lines 87 - 96, reduceLens에서 외부의 currentState.lens를 읽지 말고 상태 일관성을 위해 상태 계산을 reduce 블록 내부로 이동하세요: inside reduceLens's reduce { ... } use this.lens (current state) to choose between CameraSelector.DEFAULT_FRONT_CAMERA and CameraSelector.DEFAULT_BACK_CAMERA and return copy(lens = chosenLens, torch = TorchStatus.Off). This keeps the logic inside reduce (referencing reduceLens, reduce, currentState.lens, TorchStatus.Off) and prevents races between read and update.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@feature/photolog/capture/src/main/java/com/twix/photolog/capture/PhotologCaptureViewModel.kt`:
- Around line 87-96: reduceLens에서 외부의 currentState.lens를 읽지 말고 상태 일관성을 위해 상태 계산을
reduce 블록 내부로 이동하세요: inside reduceLens's reduce { ... } use this.lens (current
state) to choose between CameraSelector.DEFAULT_FRONT_CAMERA and
CameraSelector.DEFAULT_BACK_CAMERA and return copy(lens = chosenLens, torch =
TorchStatus.Off). This keeps the logic inside reduce (referencing reduceLens,
reduce, currentState.lens, TorchStatus.Off) and prevents races between read and
update.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: ebc4ae90-52c7-4ab4-968e-1a3c489f99f8
📒 Files selected for processing (3)
core/design-system/src/main/java/com/twix/designsystem/components/comment/model/CommentUiModel.ktfeature/photolog/capture/src/main/java/com/twix/photolog/capture/PhotologCaptureViewModel.ktfeature/photolog/capture/src/main/java/com/twix/photolog/capture/contract/PhotologCaptureUiState.kt
💤 Files with no reviewable changes (2)
- feature/photolog/capture/src/main/java/com/twix/photolog/capture/contract/PhotologCaptureUiState.kt
- core/design-system/src/main/java/com/twix/designsystem/components/comment/model/CommentUiModel.kt
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
1 participant
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.
이슈 번호
#119
작업내용
🥊 문제 상황
갤럭시 S25 기준 촬영 이미지 해상도는 4080x3060 이며 이를 원본 그대로 presigned URL을 통해 업로드하면서 최대 2.6초까지 소요되는 현상을 확인했어
📝 원인 분석
PresignedUploader에 로깅을 추가해 계측한 결과 다음 결과가 나왔어
이를 통해 고해상도 이미지를 리사이징 없이 그대로 업로드하면서 네트워크 전송 구간에서 병목이 발생하고 있음을 확인했어
🛎️ 개선 내용
BitmapFactory.Options(inJustDecodeBounds = true)를 사용해실제 디코딩 없이 이미지 크기만 먼저 측정
이번 작업을 진행하면서 공부한 내용을 블로그에 정리해봤어
결과물
PresignedUploader에 로그를 통해서 업로드에 걸리는 시간을 측정한 결과야 !
리뷰어에게 추가로 요구하는 사항 (선택)
샘플링 기준을 FHD 기준으로 잡았는는데 혹시 의견 있다면 편하게 말해줘 !
