Merged
Conversation
…or_v2 # Conflicts: # imagepicker/src/main/java/io/github/seunghee17/imagepicker/presentation/gallery/GalleryScreen.kt
- GalleryGridItem: 비디오 탭 동작을 직접 선택 토글로 변경 (이전: 항상 onOpenEditor() 호출 → ImagePickerScreen에서 재확인) (개선: 비디오는 직접 onSelectionBadgeTap() 호출로 명확화) - MediaStoreDataSource: 미사용 queryImages() 메서드 제거 (PagingSource가 직접 MediaStore.Files를 쿼리하므로 불필요) (클래스 주석 업데이트) Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Walkthrough이 PR은 이미지 피커에 비디오 선택 지원을 추가하고 내부 데이터를 페이징 스트리밍으로 전환했으며, 에디터를 페이저 기반 다중 편집으로 리팩토링하고 지역화 및 관련 빌드/의존성(페이징, Coil 비디오 디코더)을 추가합니다. Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant ImagePickerScreen
participant GalleryViewModel
participant Pager as Pager<GalleryImage>
participant PagingSource as GalleryImagePagingSource
participant MediaStore
App->>ImagePickerScreen: Open picker
ImagePickerScreen->>GalleryViewModel: collect pagingFlow
GalleryViewModel->>Pager: create Pager(pageSize=30)
Pager->>PagingSource: load(params offset/limit)
PagingSource->>MediaStore: query MediaStore.Files (images [+ videos])
MediaStore-->>PagingSource: cursor rows
PagingSource-->>Pager: LoadResult.Page(items, prevKey, nextKey)
Pager-->>GalleryViewModel: emit PagingData
GalleryViewModel-->>ImagePickerScreen: pagingFlow emits
ImagePickerScreen->>ImagePickerScreen: collectAsLazyPagingItems() -> render grid
sequenceDiagram
participant GalleryScreen
participant User
participant ImagePickerScreen
participant EditorRoute
participant PagerState
participant EditorImagePage
participant EditorViewModel
User->>GalleryScreen: Tap image (non-video)
GalleryScreen-->>ImagePickerScreen: request open editor(image)
ImagePickerScreen->>EditorRoute: show with initialIndex & allImages
EditorRoute->>PagerState: rememberPagerState(initialIndex)
PagerState->>EditorImagePage: render current page
EditorImagePage->>EditorViewModel: create/ViewModel per image.id
EditorViewModel-->>EditorImagePage: state updates
User->>EditorImagePage: perform edit -> onEditApplied(PickedImage)
EditorRoute-->>ImagePickerScreen: return edited PickedImage
ImagePickerScreen-->>App: deliver result(s)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 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 docstrings
🧪 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.idea/inspectionProfiles/Project_Default.xml (1)
20-31:⚠️ Potential issue | 🟠 MajorGlance Preview 검사 도구에
previewFile옵션을 추가하세요.Compose Preview 검사 도구들에는
previewFile옵션이 추가되었는데, Glance Preview 검사 도구들(GlancePreviewDimensionRespectsLimit, GlancePreviewMustBeTopLevelFunction, GlancePreviewNeedsComposableAnnotation, GlancePreviewNotSupportedInUnitTestFiles)에는 여전히composableFile옵션만 있습니다.Android Jetpack Glance 라이브러리는 1.1.0 버전(2024년 5월)부터
androidx.glance.preview패키지의@Preview어노테이션을 통해 preview 파일을 지원하므로, 일관성을 위해 Glance 검사 도구들도previewFile옵션을 추가해야 합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.idea/inspectionProfiles/Project_Default.xml around lines 20 - 31, The four Glance Preview inspection_tool entries (GlancePreviewDimensionRespectsLimit, GlancePreviewMustBeTopLevelFunction, GlancePreviewNeedsComposableAnnotation, GlancePreviewNotSupportedInUnitTestFiles) only have the composableFile option; add a previewFile option to each by inserting an <option name="previewFile" value="true" /> entry for those inspection_tool classes so they match the other Compose preview tools and support androidx.glance.preview `@Preview` files.imagepicker/src/main/java/io/github/seunghee17/imagepicker/presentation/picker/ImagePickerViewModel.kt (1)
54-61:⚠️ Potential issue | 🟠 Major부분 허용 상태에서 권한 재요청 루프가 발생할 수 있습니다.
현재 구조에서는
PARTIALLY_GRANTED가PERMISSION_RESULT로 들어올 때마다RequestPermission을 다시 보내 반복 팝업으로 이어질 수 있습니다. 1회 가드가 필요합니다.🐛 제안 수정안
internal class ImagePickerViewModel : ViewModel() { @@ private var editorEntryCounter = 0L + private var hasRequestedFullAccessAfterPartial = false @@ private fun handlePermissionEvaluated( status: PermissionStatus, source: ImagePickerContract.PermissionCheckSource ) { _state.update { current -> current.copy(permissionStatus = status) } when (status) { - PermissionStatus.GRANTED -> Unit + PermissionStatus.GRANTED -> { + hasRequestedFullAccessAfterPartial = false + } PermissionStatus.PARTIALLY_GRANTED -> { - if (source != ImagePickerContract.PermissionCheckSource.RESUME) { + if (source != ImagePickerContract.PermissionCheckSource.RESUME && + !hasRequestedFullAccessAfterPartial + ) { + hasRequestedFullAccessAfterPartial = true sendEffect(ImagePickerContract.Effect.RequestPermission) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@imagepicker/src/main/java/io/github/seunghee17/imagepicker/presentation/picker/ImagePickerViewModel.kt` around lines 54 - 61, ImagePickerViewModel can loop-request permission when PermissionStatus.PARTIALLY_GRANTED because each PERMISSION_RESULT triggers sendEffect(RequestPermission); add a one-time guard in the view model state (e.g., add a boolean like hasRequestedPermission or pendingPermissionRequest to the UI state) and update its value when you call sendEffect(ImagePickerContract.Effect.RequestPermission) so subsequent PARTIALLY_GRANTED events skip sending again; use _state.update to set/reset that flag (reset when status becomes GRANTED or DENIED) and wrap the existing PermissionStatus.PARTIALLY_GRANTED branch (which checks ImagePickerContract.PermissionCheckSource.RESUME) to also check the guard before calling sendEffect.
🧹 Nitpick comments (9)
imagepicker/src/main/java/io/github/seunghee17/imagepicker/domain/repository/GalleryRepository.kt (1)
16-19: 메서드 이름과 KDoc이 이제 실제 반환 의미와 어긋날 수 있습니다.allowVideo 흐름까지 이 메서드가 담당한다면,
getPagedImages와 Line 16 설명은 여전히 이미지 전용 API처럼 읽힙니다.getPagedMedia류로 이름을 맞추거나 KDoc에 video 포함 조건을 명시해두면 호출부 오해를 줄일 수 있습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@imagepicker/src/main/java/io/github/seunghee17/imagepicker/domain/repository/GalleryRepository.kt` around lines 16 - 19, The method getPagedImages and its KDoc are misleading if the function can return videos too (e.g., controlled by an allowVideo flow); either rename the method to getPagedMedia (or similar) wherever declared/used, or update the KDoc for getPagedImages to explicitly state it may include video items when allowVideo is enabled and what parameter/flag controls that behavior—search for getPagedImages in GalleryRepository and its implementations to apply the rename consistently or adjust the documentation text in the interface and implementing classes.imagepicker/src/main/java/io/github/seunghee17/imagepicker/ImagePickerConfig.kt (1)
6-15: KDoc 용어를 “이미지”에서 “미디어”로 맞추는 것을 권장합니다.
allowVideo가 추가된 만큼maxSelectionCount설명도 “최대 선택 가능 미디어 수”로 맞추면 API 의미가 더 정확해집니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@imagepicker/src/main/java/io/github/seunghee17/imagepicker/ImagePickerConfig.kt` around lines 6 - 15, KDoc에서 “이미지” 용어들을 미디어로 통일해 API 의미를 명확히 하세요: ImagePickerConfig의 파라미터 설명 중 maxSelectionCount 문구를 “최대 선택 가능 미디어 수”로 변경하고 allowVideo 관련 설명 및 기타 KDoc(예: showAlbumSelector, allowEditing)의 언급에서 필요한 경우 “이미지”를 “미디어”로 대체해 일관성을 유지하도록 수정하세요.imagepicker/src/main/java/io/github/seunghee17/imagepicker/presentation/gallery/AlbumDropdown.kt (1)
41-47: 문자열을 리소스로 올려 i18n 일관성을 맞춰주세요.현재
"All","Dropdown","${album.name} (${album.count})"가 하드코딩되어 있어 다국어 대응 일관성이 깨집니다.♻️ 제안 수정안
+import androidx.compose.ui.res.stringResource +import io.github.seunghee17.imagepicker.R ... - text = selectedAlbum?.name ?: "All", + text = selectedAlbum?.name ?: stringResource(R.string.album_all), ... - contentDescription = "Dropdown", + contentDescription = stringResource(R.string.cd_dropdown), ... - text = { Text("${album.name} (${album.count})") }, + text = { + Text( + stringResource( + R.string.album_item_with_count, + album.name, + album.count + ) + ) + },Also applies to: 57-57
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@imagepicker/src/main/java/io/github/seunghee17/imagepicker/presentation/gallery/AlbumDropdown.kt` around lines 41 - 47, Replace hardcoded UI strings in AlbumDropdown.kt (the Text that uses selectedAlbum?.name ?: "All", the Icon contentDescription "Dropdown", and the album label string "${album.name} (${album.count})") with string resources via stringResource(...). Add entries to strings.xml such as "all", "dropdown", and a formatted "album_count" with placeholders (e.g., "%1$s (%2$d)"), then use stringResource(R.string.all) for the fallback, stringResource(R.string.dropdown) for the Icon contentDescription, and stringResource(R.string.album_count, album.name, album.count) when rendering each album name/count so i18n is consistent.imagepicker/src/main/java/io/github/seunghee17/imagepicker/domain/model/PickedImage.kt (1)
12-14: 비디오 관련 필드 조합의 불변식 검증을 추가하는 것을 권장합니다.현재는
isVideo=false인데videoDurationMs>0같은 비일관 상태를 만들 수 있어 모델 안정성이 떨어집니다.♻️ 제안 수정안
data class PickedImage( val originalUri: Uri, val editedUri: Uri? = null, val rotationDegrees: Int = 0, // 0, 90, 180, 270 (시계 방향) val cropRect: CropRect? = null, // null = 크롭 좌표 미제공 (크롭 결과는 editedUri에 반영) val isCropped: Boolean = cropRect != null, // cropRect가 없어도 true로 명시 가능 val isVideo: Boolean = false, val videoDurationMs: Long = 0L, // isVideo == true 일 때만 유효 (밀리초) -) +) { + init { + require(videoDurationMs >= 0L) { "videoDurationMs must be >= 0" } + require(isVideo || videoDurationMs == 0L) { + "videoDurationMs must be 0 when isVideo is false" + } + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@imagepicker/src/main/java/io/github/seunghee17/imagepicker/domain/model/PickedImage.kt` around lines 12 - 14, Add an invariant check to the PickedImage model to prevent inconsistent video state: inside the PickedImage class (e.g., an init block or a companion factory) validate the combination of isVideo and videoDurationMs (for example, require that when isVideo is false then videoDurationMs == 0, and when isVideo is true videoDurationMs >= 0 or >0 per your domain rule); if the invariant fails, throw IllegalArgumentException or normalize the value so callers cannot create inconsistent instances. Ensure the checks reference the fields isVideo and videoDurationMs so the model enforces consistency at construction time.imagepicker/src/main/java/io/github/seunghee17/imagepicker/presentation/gallery/GalleryContract.kt (1)
23-24:selectionOrderMap가 접근마다 새로 생성됩니다.Line 24의 getter는 호출할 때마다
mapIndexed + toMap할당이 발생합니다. 접근 빈도가 높다면 불필요한 오버헤드가 생길 수 있으니, 상태 갱신 시점에 한 번 계산해 보관하거나 사용 지점에서remember로 캐싱하는 쪽을 권장합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@imagepicker/src/main/java/io/github/seunghee17/imagepicker/presentation/gallery/GalleryContract.kt` around lines 23 - 24, The getter for selectionOrderMap currently recomputes selectedImages.mapIndexed { idx, img -> img.id to (idx + 1) }.toMap() on every access which is wasteful; change it to compute and store the map only when selectedImages changes (e.g., update selectionOrderMap whenever selectedImages is mutated or expose a cached backing property updated in selection methods) or move the computation to the UI call-site and wrap it with remember to cache the result; update references to selectionOrderMap and any code that mutates selectedImages so the cached map is refreshed appropriately.imagepicker/src/main/java/io/github/seunghee17/imagepicker/presentation/editor/EditorScreen.kt (1)
214-219: 현재 페이지 상태 노출 패턴에 대한 고려사항
LaunchedEffect(isCurrentPage)내에서viewModel.state.collect를 사용하여 부모에 상태를 노출하는 패턴이 기능적으로 동작하지만, 다소 비관습적입니다.isCurrentPage가 변경될 때마다 새로운 collect가 시작되며, 이전 collect는 취소됩니다. 현재 구현은 정상 동작하지만, 향후 유지보수 시 이 흐름을 주석으로 명확히 해두시면 좋겠습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@imagepicker/src/main/java/io/github/seunghee17/imagepicker/presentation/editor/EditorScreen.kt` around lines 214 - 219, The LaunchedEffect(isCurrentPage) block starts a new collector each time isCurrentPage changes and cancels the previous one, which is intentional here; update the LaunchedEffect containing viewModel.state.collect to include a concise comment explaining that restarting/canceling behavior and why latestOnActivate(viewModel.state, viewModel::handleIntent) is used, or refactor to use a clearer pattern (e.g., collectLatest or snapshotFlow) if you prefer – target the LaunchedEffect, isCurrentPage, viewModel.state.collect, latestOnActivate, and viewModel::handleIntent sites when applying the change.CLAUDE.md (2)
272-276: 코드 블록에 언어 지정자가 없습니다.마크다운 린터 경고(MD040)에 따라 펜스 코드 블록에 언어를 지정하면 구문 강조가 적용되어 가독성이 향상됩니다.
📝 제안된 수정
Line 272:
-``` +```text User Action → Intent → ViewModel → State update → Composable renderLine 286:
-``` +```text presentation/ picker/ ← Gallery selection screenAlso applies to: 286-300
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLAUDE.md` around lines 272 - 276, Add a language identifier to the fenced code blocks so the Markdown linter (MD040) stops warning and syntax highlighting is applied: update the block containing "User Action → Intent → ViewModel → State update → Composable render" to start with ```text (and close with ```), and likewise update the block showing the tree (the lines starting with "presentation/" and "picker/ ← Gallery selection screen") to start with ```text (and close with ```); ensure both fenced blocks use the same ```text marker.
5-5: 마크다운 제목 레벨이 올바르지 않습니다.
### Development Goals는# Project Purpose(h1) 바로 다음에 나와서 h2를 건너뛰고 있습니다. 마크다운 린터 경고(MD001)에 따라## Development Goals로 변경하세요.📝 제안된 수정
-### Development Goals +## Development Goals🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLAUDE.md` at line 5, The markdown heading "### Development Goals" should be changed to "## Development Goals" so it doesn't skip the h2 level after the existing "# Project Purpose" heading; locate the line containing the exact text "### Development Goals" and replace the leading "###" with "##" to satisfy MD001.imagepicker/src/main/java/io/github/seunghee17/imagepicker/presentation/gallery/GalleryScreen.kt (1)
93-110: 자동 스크롤 중 선택 해제가 되지 않습니다.현재 로직은 드래그 자동 스크롤 중 이미 선택된 이미지 위를 지나가도 선택 해제하지 않습니다(Line 100의
none조건). 이것이 의도된 UX라면 괜찮지만, 드래그로 선택/해제를 모두 지원하려면 토글 로직 검토가 필요합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@imagepicker/src/main/java/io/github/seunghee17/imagepicker/presentation/gallery/GalleryScreen.kt` around lines 93 - 110, The drag-auto-scroll currently only selects images because the if uses currentState.selectedImages.none { it.id == key } (so existing selections are ignored); change the behavior so crossing any item toggles its selection state: inside the LaunchedEffect/snapshotFlow block where you resolve key via gridState.gridItemKeyAtPosition(offset), remove the none check and always call onIntent(GalleryContract.Intent.ToggleImageSelection(image)) when currentItemsById[key] yields an image (or explicitly check membership with currentState.selectedImages.any { it.id == key } but still call the same ToggleImageSelection to allow unselecting), keeping the rest of the auto-scroll logic (gridState.scrollBy, delay) intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@imagepicker/src/main/java/io/github/seunghee17/imagepicker/presentation/gallery/GalleryGridItem.kt`:
- Around line 53-59: The badge tap routing in GalleryGridItem.kt is incorrect:
the lambda handleTap currently calls currentOnOpenEditor() for MediaType.IMAGE
but the badge should always toggle selection. Modify handleTap so it always
invokes currentOnSelectionBadgeTap() regardless of image.mediaType (i.e., remove
the mediaType conditional or make both branches call
currentOnSelectionBadgeTap), leaving currentOnOpenEditor() unused here.
In
`@imagepicker/src/main/java/io/github/seunghee17/imagepicker/presentation/gallery/GalleryScreenViewModel.kt`:
- Around line 117-121: The snackbar message is being built in
GalleryScreenViewModel (inside viewModelScope.launch calling _effect.send with
GalleryContract.Effect.ShowSelectionLimitSnackbar) which hardcodes Korean text;
instead change the Effect payload to carry only the numeric limit (e.g., pass
current.maxSelectionCount or rename the effect to ShowSelectionLimit with an
Int), update GalleryContract.Effect.ShowSelectionLimitSnackbar to accept an Int
(or add a new ShowSelectionLimit(count: Int) variant) and send that from the
view model, then update the UI layer to resolve the localized string resource
(selection_limit) and format it with the count before showing the Snackbar.
Ensure references to GalleryContract.Effect.ShowSelectionLimitSnackbar and
current.maxSelectionCount are updated accordingly.
In
`@imagepicker/src/main/java/io/github/seunghee17/imagepicker/presentation/picker/PickerPermissionHelper.kt`:
- Around line 54-57: 현재 primaryPermission(= fullPermissions.first())만 대상으로
ActivityCompat.shouldShowRequestPermissionRationale를 확인해 영구거부를 오판하고 있으므로, 거부된
권한들(allowed 거부된 항목들)을 전부 확인하도록 변경하세요: PickerPermissionHelper.kt의
primaryPermission/shouldShowRationale 처리부를 수정해 fullPermissions에서 거부된 권한들만
골라(ActivityCompat.checkSelfPermission == DENIED) 각 권한에 대해
ActivityCompat.shouldShowRequestPermissionRationale를 호출하고, 그 중 하나라도 true면 상태를
DENIED로 반환하고, 모두 false일 때만 PERMANENTLY_DENIED를 반환하도록 구현하세요; 또한 기존
hasRequestedPermission 분기와 결합해 기존 로직(예: hasRequestedPermission,
PERMANENTLY_DENIED/ DENIED 반환)을 유지하되 rationale 판정은 위 방식으로 대체하세요.
In
`@imagepicker/src/main/java/io/github/seunghee17/imagepicker/presentation/utils/Utils.kt`:
- Around line 33-35: Auto-scroll can repeatedly toggle the same item because
only coordinates are exposed via currentDragOffset; update the exposed state to
include the target item key (e.g., pair the Offset with an id/key or add a
MutableState<Any?> like lastAutoScrollKey) so GalleryScreen's auto-scroll loop
can either share and check the last processed key to avoid duplicate
ToggleImageSelection calls or switch to dispatching an idempotent Select action
instead of ToggleImageSelection; modify the modifier in Utils.kt that defines
currentDragOffset to provide the item key (or lastProcessedKey) and adjust
GalleryScreen to dedupe by comparing against that key or replace
ToggleImageSelection with Select for auto-scroll-driven selections.
In `@imagepicker/src/main/res/values/strings.xml`:
- Line 20: The string resource "selection_limit" currently uses "images" which
is incorrect when allowVideo=true; update this resource to use a neutral term
like "media" or add a separate string variant for video (e.g.,
"selection_limit_media" and/or "selection_limit_video") and update any related
permission-denied strings (the similar resources around lines 26-28) to also be
generalized or to provide separate photo/video variants; ensure callers (code
that references selection_limit) are updated to pick the appropriate resource
based on allowVideo (or always use the neutral "media" key) so UI messages
accurately reflect whether photos, videos, or mixed media are selectable.
---
Outside diff comments:
In @.idea/inspectionProfiles/Project_Default.xml:
- Around line 20-31: The four Glance Preview inspection_tool entries
(GlancePreviewDimensionRespectsLimit, GlancePreviewMustBeTopLevelFunction,
GlancePreviewNeedsComposableAnnotation,
GlancePreviewNotSupportedInUnitTestFiles) only have the composableFile option;
add a previewFile option to each by inserting an <option name="previewFile"
value="true" /> entry for those inspection_tool classes so they match the other
Compose preview tools and support androidx.glance.preview `@Preview` files.
In
`@imagepicker/src/main/java/io/github/seunghee17/imagepicker/presentation/picker/ImagePickerViewModel.kt`:
- Around line 54-61: ImagePickerViewModel can loop-request permission when
PermissionStatus.PARTIALLY_GRANTED because each PERMISSION_RESULT triggers
sendEffect(RequestPermission); add a one-time guard in the view model state
(e.g., add a boolean like hasRequestedPermission or pendingPermissionRequest to
the UI state) and update its value when you call
sendEffect(ImagePickerContract.Effect.RequestPermission) so subsequent
PARTIALLY_GRANTED events skip sending again; use _state.update to set/reset that
flag (reset when status becomes GRANTED or DENIED) and wrap the existing
PermissionStatus.PARTIALLY_GRANTED branch (which checks
ImagePickerContract.PermissionCheckSource.RESUME) to also check the guard before
calling sendEffect.
---
Nitpick comments:
In `@CLAUDE.md`:
- Around line 272-276: Add a language identifier to the fenced code blocks so
the Markdown linter (MD040) stops warning and syntax highlighting is applied:
update the block containing "User Action → Intent → ViewModel → State update →
Composable render" to start with ```text (and close with ```), and likewise
update the block showing the tree (the lines starting with "presentation/" and
"picker/ ← Gallery selection screen") to start with ```text (and close with
```); ensure both fenced blocks use the same ```text marker.
- Line 5: The markdown heading "### Development Goals" should be changed to "##
Development Goals" so it doesn't skip the h2 level after the existing "# Project
Purpose" heading; locate the line containing the exact text "### Development
Goals" and replace the leading "###" with "##" to satisfy MD001.
In
`@imagepicker/src/main/java/io/github/seunghee17/imagepicker/domain/model/PickedImage.kt`:
- Around line 12-14: Add an invariant check to the PickedImage model to prevent
inconsistent video state: inside the PickedImage class (e.g., an init block or a
companion factory) validate the combination of isVideo and videoDurationMs (for
example, require that when isVideo is false then videoDurationMs == 0, and when
isVideo is true videoDurationMs >= 0 or >0 per your domain rule); if the
invariant fails, throw IllegalArgumentException or normalize the value so
callers cannot create inconsistent instances. Ensure the checks reference the
fields isVideo and videoDurationMs so the model enforces consistency at
construction time.
In
`@imagepicker/src/main/java/io/github/seunghee17/imagepicker/domain/repository/GalleryRepository.kt`:
- Around line 16-19: The method getPagedImages and its KDoc are misleading if
the function can return videos too (e.g., controlled by an allowVideo flow);
either rename the method to getPagedMedia (or similar) wherever declared/used,
or update the KDoc for getPagedImages to explicitly state it may include video
items when allowVideo is enabled and what parameter/flag controls that
behavior—search for getPagedImages in GalleryRepository and its implementations
to apply the rename consistently or adjust the documentation text in the
interface and implementing classes.
In
`@imagepicker/src/main/java/io/github/seunghee17/imagepicker/ImagePickerConfig.kt`:
- Around line 6-15: KDoc에서 “이미지” 용어들을 미디어로 통일해 API 의미를 명확히 하세요:
ImagePickerConfig의 파라미터 설명 중 maxSelectionCount 문구를 “최대 선택 가능 미디어 수”로 변경하고
allowVideo 관련 설명 및 기타 KDoc(예: showAlbumSelector, allowEditing)의 언급에서 필요한 경우
“이미지”를 “미디어”로 대체해 일관성을 유지하도록 수정하세요.
In
`@imagepicker/src/main/java/io/github/seunghee17/imagepicker/presentation/editor/EditorScreen.kt`:
- Around line 214-219: The LaunchedEffect(isCurrentPage) block starts a new
collector each time isCurrentPage changes and cancels the previous one, which is
intentional here; update the LaunchedEffect containing viewModel.state.collect
to include a concise comment explaining that restarting/canceling behavior and
why latestOnActivate(viewModel.state, viewModel::handleIntent) is used, or
refactor to use a clearer pattern (e.g., collectLatest or snapshotFlow) if you
prefer – target the LaunchedEffect, isCurrentPage, viewModel.state.collect,
latestOnActivate, and viewModel::handleIntent sites when applying the change.
In
`@imagepicker/src/main/java/io/github/seunghee17/imagepicker/presentation/gallery/AlbumDropdown.kt`:
- Around line 41-47: Replace hardcoded UI strings in AlbumDropdown.kt (the Text
that uses selectedAlbum?.name ?: "All", the Icon contentDescription "Dropdown",
and the album label string "${album.name} (${album.count})") with string
resources via stringResource(...). Add entries to strings.xml such as "all",
"dropdown", and a formatted "album_count" with placeholders (e.g., "%1$s
(%2$d)"), then use stringResource(R.string.all) for the fallback,
stringResource(R.string.dropdown) for the Icon contentDescription, and
stringResource(R.string.album_count, album.name, album.count) when rendering
each album name/count so i18n is consistent.
In
`@imagepicker/src/main/java/io/github/seunghee17/imagepicker/presentation/gallery/GalleryContract.kt`:
- Around line 23-24: The getter for selectionOrderMap currently recomputes
selectedImages.mapIndexed { idx, img -> img.id to (idx + 1) }.toMap() on every
access which is wasteful; change it to compute and store the map only when
selectedImages changes (e.g., update selectionOrderMap whenever selectedImages
is mutated or expose a cached backing property updated in selection methods) or
move the computation to the UI call-site and wrap it with remember to cache the
result; update references to selectionOrderMap and any code that mutates
selectedImages so the cached map is refreshed appropriately.
In
`@imagepicker/src/main/java/io/github/seunghee17/imagepicker/presentation/gallery/GalleryScreen.kt`:
- Around line 93-110: The drag-auto-scroll currently only selects images because
the if uses currentState.selectedImages.none { it.id == key } (so existing
selections are ignored); change the behavior so crossing any item toggles its
selection state: inside the LaunchedEffect/snapshotFlow block where you resolve
key via gridState.gridItemKeyAtPosition(offset), remove the none check and
always call onIntent(GalleryContract.Intent.ToggleImageSelection(image)) when
currentItemsById[key] yields an image (or explicitly check membership with
currentState.selectedImages.any { it.id == key } but still call the same
ToggleImageSelection to allow unselecting), keeping the rest of the auto-scroll
logic (gridState.scrollBy, delay) intact.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 305b02f7-b800-409f-a239-899a12afc242
📒 Files selected for processing (40)
.idea/inspectionProfiles/Project_Default.xmlAGENTS.mdCLAUDE.mdapp/src/main/java/com/universe/dynamicimagepicker/MainActivity.ktgradle.propertiesgradle/libs.versions.tomlimagepicker/build.gradle.ktsimagepicker/src/main/AndroidManifest.xmlimagepicker/src/main/java/io/github/seunghee17/imagepicker/DynamicImagePicker.ktimagepicker/src/main/java/io/github/seunghee17/imagepicker/ImagePickerConfig.ktimagepicker/src/main/java/io/github/seunghee17/imagepicker/data/repository/GalleryRepositoryImpl.ktimagepicker/src/main/java/io/github/seunghee17/imagepicker/data/source/GalleryImagePagingSource.ktimagepicker/src/main/java/io/github/seunghee17/imagepicker/data/source/MediaStoreDataSource.ktimagepicker/src/main/java/io/github/seunghee17/imagepicker/domain/model/GalleryAlbum.ktimagepicker/src/main/java/io/github/seunghee17/imagepicker/domain/model/GalleryImage.ktimagepicker/src/main/java/io/github/seunghee17/imagepicker/domain/model/MediaType.ktimagepicker/src/main/java/io/github/seunghee17/imagepicker/domain/model/PickedImage.ktimagepicker/src/main/java/io/github/seunghee17/imagepicker/domain/repository/GalleryRepository.ktimagepicker/src/main/java/io/github/seunghee17/imagepicker/domain/usecase/GetImagesInAlbumUseCase.ktimagepicker/src/main/java/io/github/seunghee17/imagepicker/domain/usecase/GetPagedImagesUseCase.ktimagepicker/src/main/java/io/github/seunghee17/imagepicker/presentation/component/TopBarWithCount.ktimagepicker/src/main/java/io/github/seunghee17/imagepicker/presentation/editor/EditorContract.ktimagepicker/src/main/java/io/github/seunghee17/imagepicker/presentation/editor/EditorScreen.ktimagepicker/src/main/java/io/github/seunghee17/imagepicker/presentation/editor/EditorViewModel.ktimagepicker/src/main/java/io/github/seunghee17/imagepicker/presentation/gallery/AlbumDropdown.ktimagepicker/src/main/java/io/github/seunghee17/imagepicker/presentation/gallery/GalleryContract.ktimagepicker/src/main/java/io/github/seunghee17/imagepicker/presentation/gallery/GalleryGridItem.ktimagepicker/src/main/java/io/github/seunghee17/imagepicker/presentation/gallery/GalleryScreen.ktimagepicker/src/main/java/io/github/seunghee17/imagepicker/presentation/gallery/GalleryScreenViewModel.ktimagepicker/src/main/java/io/github/seunghee17/imagepicker/presentation/gallery/GalleryScreenViewModelFactory.ktimagepicker/src/main/java/io/github/seunghee17/imagepicker/presentation/picker/EditorRoute.ktimagepicker/src/main/java/io/github/seunghee17/imagepicker/presentation/picker/ImagePickerContract.ktimagepicker/src/main/java/io/github/seunghee17/imagepicker/presentation/picker/ImagePickerScreen.ktimagepicker/src/main/java/io/github/seunghee17/imagepicker/presentation/picker/ImagePickerViewModel.ktimagepicker/src/main/java/io/github/seunghee17/imagepicker/presentation/picker/PermissionFallbackContent.ktimagepicker/src/main/java/io/github/seunghee17/imagepicker/presentation/picker/PickerPermissionHelper.ktimagepicker/src/main/java/io/github/seunghee17/imagepicker/presentation/utils/Utils.ktimagepicker/src/main/res/values-ko/strings.xmlimagepicker/src/main/res/values/strings.xmlimagepicker/stability_config.conf
💤 Files with no reviewable changes (3)
- imagepicker/src/main/java/io/github/seunghee17/imagepicker/presentation/editor/EditorViewModel.kt
- AGENTS.md
- imagepicker/src/main/java/io/github/seunghee17/imagepicker/domain/usecase/GetImagesInAlbumUseCase.kt
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
imagepicker/src/main/java/io/github/seunghee17/imagepicker/presentation/gallery/GalleryScreenViewModel.kt (1)
94-103:⚠️ Potential issue | 🟠 Major선택된 앨범을 최신 목록에 다시 매핑하지 않아 stale 상태가 남습니다.
여기서는
_albumFilter만 동기화하고selectedAlbum자체는 그대로 유지합니다. 그래서 선택된 앨범이 삭제되거나 새 emission에서 같은 id의 새 인스턴스로 교체되면, 드롭다운과 pager가 둘 다 오래된 앨범 객체/id를 계속 바라보게 됩니다.🔧 수정 예시
getAlbums() .onEach { albums -> - _state.update { current -> - val selectedAlbum = current.selectedAlbum ?: albums.firstOrNull() - current.copy(albums = albums, selectedAlbum = selectedAlbum) - } - val selectedAlbumId = _state.value.selectedAlbum?.id - val nextFilter = AlbumFilter.Active(selectedAlbumId) + val resolvedSelectedAlbum = albums.firstOrNull { album -> + album.id == _state.value.selectedAlbum?.id + } ?: albums.firstOrNull() + + _state.update { current -> + current.copy( + albums = albums, + selectedAlbum = resolvedSelectedAlbum, + ) + } + val nextFilter = AlbumFilter.Active(resolvedSelectedAlbum?.id) if (_albumFilter.value != nextFilter) { _albumFilter.value = nextFilter } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@imagepicker/src/main/java/io/github/seunghee17/imagepicker/presentation/gallery/GalleryScreenViewModel.kt` around lines 94 - 103, The current onEach updates _albumFilter but leaves _state.selectedAlbum stale; after receiving albums you should remap the selected album to the new instance (or clear/fallback) so UI components don't hold deleted or old objects. Inside the onEach where you have albums and current, compute val oldId = current.selectedAlbum?.id, then find val newSelected = albums.find { it.id == oldId } ?: albums.firstOrNull() (or null if you prefer to clear when deleted) and update _state.copy(albums = albums, selectedAlbum = newSelected); then set _albumFilter to AlbumFilter.Active(newSelected?.id) only if it differs from _albumFilter.value.imagepicker/src/main/java/io/github/seunghee17/imagepicker/presentation/picker/ImagePickerScreen.kt (1)
121-157:⚠️ Potential issue | 🟠 Major
LaunchedEffect(Unit)에서 최신 상태를 읽도록 수정하세요.
LaunchedEffect(Unit)는 재시작되지 않아서 캡처된hasRequestedPermission,galleryState.selectedImages,onResult,onCancel등이 초기 composition 값으로 고정됩니다. 이로 인해 권한 재평가가 이전 플래그로 실행되거나 editor 진입 시 선택 목록이 오래된 상태를 기준으로 index를 계산합니다.rememberUpdatedState로 감싸서 항상 최신 값을 읽도록 해주세요.🔧 수정 예시
import androidx.compose.runtime.getValue import androidx.compose.runtime.mutableStateOf import androidx.compose.runtime.remember +import androidx.compose.runtime.rememberUpdatedState import androidx.compose.runtime.saveable.rememberSaveable import androidx.compose.runtime.saveable.rememberSaveableStateHolder import androidx.compose.runtime.setValue @@ var hasRequestedPermission by rememberSaveable { mutableStateOf(false) } var editorDestination by rememberSaveable(stateSaver = editorDestinationSaver()) { mutableStateOf<EditorDestination?>(null) } val saveableStateHolder = rememberSaveableStateHolder() + val currentContext by rememberUpdatedState(context) + val currentGalleryState by rememberUpdatedState(galleryState) + val currentHasRequestedPermission by rememberUpdatedState(hasRequestedPermission) + val currentAllowVideo by rememberUpdatedState(config.allowVideo) + val currentOnResult by rememberUpdatedState(onResult) + val currentOnCancel by rememberUpdatedState(onCancel) @@ - LaunchedEffect(Unit) { + LaunchedEffect(viewModel) { viewModel.effect.collectLatest { effect -> when (effect) { is ImagePickerContract.Effect.CheckPermission -> { viewModel.handleIntent( ImagePickerContract.Intent.OnPermissionEvaluated( status = resolvePermissionStatus( - context = context, - hasRequestedPermission = hasRequestedPermission, - allowVideo = config.allowVideo + context = currentContext, + hasRequestedPermission = currentHasRequestedPermission, + allowVideo = currentAllowVideo ), source = effect.source ) ) } is ImagePickerContract.Effect.RequestPermission -> { hasRequestedPermission = true permissionLauncher.launch(requestedPermissionsForPicker(config.allowVideo)) } - is ImagePickerContract.Effect.NavigateToSettings -> openAppSettings(context) - is ImagePickerContract.Effect.ReturnResult -> onResult(effect.result) - is ImagePickerContract.Effect.Cancelled -> onCancel() + is ImagePickerContract.Effect.NavigateToSettings -> openAppSettings(currentContext) + is ImagePickerContract.Effect.ReturnResult -> currentOnResult(effect.result) + is ImagePickerContract.Effect.Cancelled -> currentOnCancel() is ImagePickerContract.Effect.NavigateToEditor -> { val tappedImage = effect.image - val selected = galleryState.selectedImages + val selected = currentGalleryState.selectedImages val index = selected.indexOfFirst { it.id == tappedImage.id }.coerceAtLeast(0) editorDestination = EditorDestination( entryId = effect.entryId,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@imagepicker/src/main/java/io/github/seunghee17/imagepicker/presentation/picker/ImagePickerScreen.kt` around lines 121 - 157, LaunchedEffect currently captures stale values (hasRequestedPermission, galleryState.selectedImages, onResult, onCancel) because it runs once; update these to be read via rememberUpdatedState so the collector in LaunchedEffect(viewModel.effect.collectLatest { ... }) always uses current values. Specifically, create rememberUpdatedState wrappers for hasRequestedPermission, galleryState.selectedImages, onResult, and onCancel, and inside the ImagePickerContract.Effect handling read the .value of those rememberUpdatedState instances (e.g., use hasRequestedPermissionState.value, selectedImagesState.value, onResultState.value, onCancelState.value) when launching permission, returning results, cancelling, or computing the editor index.
🧹 Nitpick comments (1)
imagepicker/src/main/java/io/github/seunghee17/imagepicker/presentation/utils/Utils.kt (1)
64-75: 상태 갱신/초기화 로직은 헬퍼로 묶어도 좋겠습니다.동일한
DragSelectionState생성/리셋 코드가 반복되어 있어, 작은 헬퍼로 추출하면 유지보수성이 더 좋아집니다.♻️ 예시 리팩터링
pointerInput(Unit) { var initialKey: Long? = null var currentKey: Long? = null + + fun updateDragState(offset: Offset, key: Long? = currentDragState.value?.lastProcessedKey) { + currentDragState.value = DragSelectionState( + offset = offset, + lastProcessedKey = key, + ) + } + + fun clearDragState() { + initialKey = null + currentKey = null + autoScrollSpeed.value = 0f + currentDragState.value = null + } detectDragGesturesAfterLongPress( onDragStart = { offset -> lazyGridState.gridItemKeyAtPosition(offset)?.let { key -> if (currentSelectedImages.none { it.id == key }) { @@ - currentDragState.value = DragSelectionState( - offset = offset, - lastProcessedKey = key, - ) + updateDragState(offset, key) } } }, - onDragCancel = { - initialKey = null - currentKey = null - autoScrollSpeed.value = 0f - currentDragState.value = null - }, - onDragEnd = { - initialKey = null - currentKey = null - autoScrollSpeed.value = 0f - currentDragState.value = null - }, + onDragCancel = { clearDragState() }, + onDragEnd = { clearDragState() }, onDrag = { change, _ -> if (initialKey != null) { - currentDragState.value = DragSelectionState( - offset = change.position, - lastProcessedKey = currentDragState.value?.lastProcessedKey, - ) + updateDragState(change.position) @@ - currentDragState.value = DragSelectionState( - offset = change.position, - lastProcessedKey = key, - ) + updateDragState(change.position, key) } } } } ) }Also applies to: 78-81, 95-98
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@imagepicker/src/main/java/io/github/seunghee17/imagepicker/presentation/utils/Utils.kt` around lines 64 - 75, Extract the repeated drag-reset logic into a single helper (e.g., resetDragState) and call it from the duplicated handlers (onDragCancel, onDragEnd and the other similar blocks around the other occurrences). The helper should set initialKey = null, currentKey = null, autoScrollSpeed.value = 0f and currentDragState.value = null so you replace the four-line reset in each handler with a single call to resetDragState; update references in the handlers that currently perform those updates directly to invoke the new helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@imagepicker/src/main/java/io/github/seunghee17/imagepicker/presentation/gallery/GalleryScreenViewModel.kt`:
- Around line 94-103: The current onEach updates _albumFilter but leaves
_state.selectedAlbum stale; after receiving albums you should remap the selected
album to the new instance (or clear/fallback) so UI components don't hold
deleted or old objects. Inside the onEach where you have albums and current,
compute val oldId = current.selectedAlbum?.id, then find val newSelected =
albums.find { it.id == oldId } ?: albums.firstOrNull() (or null if you prefer to
clear when deleted) and update _state.copy(albums = albums, selectedAlbum =
newSelected); then set _albumFilter to AlbumFilter.Active(newSelected?.id) only
if it differs from _albumFilter.value.
In
`@imagepicker/src/main/java/io/github/seunghee17/imagepicker/presentation/picker/ImagePickerScreen.kt`:
- Around line 121-157: LaunchedEffect currently captures stale values
(hasRequestedPermission, galleryState.selectedImages, onResult, onCancel)
because it runs once; update these to be read via rememberUpdatedState so the
collector in LaunchedEffect(viewModel.effect.collectLatest { ... }) always uses
current values. Specifically, create rememberUpdatedState wrappers for
hasRequestedPermission, galleryState.selectedImages, onResult, and onCancel, and
inside the ImagePickerContract.Effect handling read the .value of those
rememberUpdatedState instances (e.g., use hasRequestedPermissionState.value,
selectedImagesState.value, onResultState.value, onCancelState.value) when
launching permission, returning results, cancelling, or computing the editor
index.
---
Nitpick comments:
In
`@imagepicker/src/main/java/io/github/seunghee17/imagepicker/presentation/utils/Utils.kt`:
- Around line 64-75: Extract the repeated drag-reset logic into a single helper
(e.g., resetDragState) and call it from the duplicated handlers (onDragCancel,
onDragEnd and the other similar blocks around the other occurrences). The helper
should set initialKey = null, currentKey = null, autoScrollSpeed.value = 0f and
currentDragState.value = null so you replace the four-line reset in each handler
with a single call to resetDragState; update references in the handlers that
currently perform those updates directly to invoke the new helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6001d4b5-a1a7-4f81-b20e-3e0a49d6ffd4
📒 Files selected for processing (13)
.idea/inspectionProfiles/Project_Default.xmlREADME.mdimagepicker/src/main/java/io/github/seunghee17/imagepicker/presentation/gallery/GalleryContract.ktimagepicker/src/main/java/io/github/seunghee17/imagepicker/presentation/gallery/GalleryGridItem.ktimagepicker/src/main/java/io/github/seunghee17/imagepicker/presentation/gallery/GalleryScreen.ktimagepicker/src/main/java/io/github/seunghee17/imagepicker/presentation/gallery/GalleryScreenViewModel.ktimagepicker/src/main/java/io/github/seunghee17/imagepicker/presentation/picker/ImagePickerContract.ktimagepicker/src/main/java/io/github/seunghee17/imagepicker/presentation/picker/ImagePickerScreen.ktimagepicker/src/main/java/io/github/seunghee17/imagepicker/presentation/picker/ImagePickerViewModel.ktimagepicker/src/main/java/io/github/seunghee17/imagepicker/presentation/picker/PickerPermissionHelper.ktimagepicker/src/main/java/io/github/seunghee17/imagepicker/presentation/utils/Utils.ktimagepicker/src/main/res/values-ko/strings.xmlimagepicker/src/main/res/values/strings.xml
✅ Files skipped from review due to trivial changes (3)
- imagepicker/src/main/res/values-ko/strings.xml
- README.md
- imagepicker/src/main/res/values/strings.xml
🚧 Files skipped from review as they are similar to previous changes (3)
- .idea/inspectionProfiles/Project_Default.xml
- imagepicker/src/main/java/io/github/seunghee17/imagepicker/presentation/gallery/GalleryContract.kt
- imagepicker/src/main/java/io/github/seunghee17/imagepicker/presentation/gallery/GalleryScreen.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.
Summary by CodeRabbit
새로운 기능
개선 사항
기타