[fix] #161 ArchiveResult 화면별 Result 타입 분리#166
Merged
Conversation
- 모든사진/모든앨범/앨범상세에서 ArchiveResult 송신 추가 - 아카이빙메인에 AlbumChanged 수신 추가 - 모든앨범에 ResultEffect 수신 추가
RefreshArchiveMainPhotos → RefreshArchiveMain으로 변경하여 사진뿐만 아니라 앨범 목록, 즐겨찾기 요약도 함께 갱신
- AllAlbum 앨범 생성 시 AlbumChanged 전파 - AllPhoto 즐겨찾기 토글 시 FavoriteChanged 전파 - AllPhoto에서 PhotoUploaded 수신 시 paging refresh 처리
Channel 기반 1:1 소비 구조에서 자기소비 문제 해결을 위해 ArchiveResult를 화면별 data object로 분리
- 각 화면이 자기 타입만 방출 (세부 이벤트 없이 data object) - 수신 측은 직속 자식 화면의 Result만 수신하여 전체 갱신 - 불필요한 수신용 Intent 제거 (PhotoDeleted, FavoriteChanged) - RefreshPhotos 시 로컬 상태(deletedPhotoIds, updatedFavorites) 초기화 - MainScreen 외부 송신 PhotoUploadedResult로 변경
|
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:
Walkthrough아카이브 결과 타입이 페이로드에서 마커 싱글톤으로 재설계되었고, 화면들은 로컬 Changes
Sequence Diagram(s)sequenceDiagram
participant VM as ViewModel
participant Side as SideEffect
participant Route as Route/Screen
participant Bus as LocalResultEventBus
participant Nav as ArchiveEntryProvider
participant TargetVM as Target ViewModel
VM->>Side: postSideEffect(NotifyResult / NotifyPhotoUpdated)
Side->>Route: sideEffect emitted
Route->>Bus: sendResult(MarkerResult, allowDuplicate=false)
Bus->>Nav: result event delivered
Nav->>TargetVM: dispatch Refresh intent (RefreshPhotos / RefreshAlbums / RefreshArchiveMain)
TargetVM->>TargetVM: fetch... (동시 또는 개별 리프레시)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
- PhotoDetailScreen에서 ActionBar의 높이를 측정하여 PhotoDetailImageItem에 전달 - 메모 수정 모드(MemoMode.Editing)일 때 ActionBar가 이미지를 가리지 않도록 하단 패딩 적용 - PhotoDetailActionBar 노출 여부 분기 처리를 AnimatedVisibility에서 if 문으로 변경 및 높이 측정 로직 추가
- `PhotoDetailState`의 `currentIndex` 계산 방식을 나머지 연산에서 `coerceIn`을 통한 범위 제한으로 변경 - `HorizontalPager`의 `pageCount`를 `Int.MAX_VALUE`에서 실제 사진 리스트 크기로 변경하여 무한 스크롤 제거 - `PhotoDetailViewModel`에서 다음 사진 이동 시 마지막 인덱스 초과 방지 조건 추가 - `PhotoDetailScreen` 내 불필요한 `Box` 레이아웃 제거 및 코드 정리
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
feature/archive/impl/src/main/kotlin/com/neki/android/feature/archive/impl/navigation/ArchiveEntryProvider.kt (1)
88-101:⚠️ Potential issue | 🟠 MajorAllPhoto 및 AllAlbum 진입점에
PhotoUploadedResult핸들러 누락PR 목표 및 커밋 메시지("PhotoUploaded 수신 시 페이징 리프레시")와 달리, AllPhoto와 AllAlbum 진입점에서
PhotoUploadedResult를 처리하지 않습니다. Archive 메인 화면에서는 올바르게 처리하고 있습니다.사용자가 AllPhoto나 AllAlbum 화면에 있을 때 다른 플로우에서 사진이 업로드되면 페이징이 갱신되지 않습니다.
AllPhoto에 추가할 수 있는 핸들러 예시
ResultEffect<PhotoUploadedResult>(resultBus) { viewModel.store.onIntent(AllPhotoIntent.RefreshPhotos) }AllAlbum도 마찬가지로
PhotoUploadedResult핸들러를 추가해야 합니다:ResultEffect<PhotoUploadedResult>(resultBus) { viewModel.store.onIntent(AllAlbumIntent.RefreshAlbums) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/archive/impl/src/main/kotlin/com/neki/android/feature/archive/impl/navigation/ArchiveEntryProvider.kt` around lines 88 - 101, The AllPhoto and AllAlbum entry blocks are missing a ResultEffect handler for PhotoUploadedResult, so add a ResultEffect<PhotoUploadedResult>(resultBus) in each entry (the same place where ResultEffect<PhotoDetailResult> is used) and call the corresponding refresh intent on the view model (for AllPhoto use viewModel.store.onIntent(AllPhotoIntent.RefreshPhotos); for AllAlbum use viewModel.store.onIntent(AllAlbumIntent.RefreshAlbums)); reuse the existing resultBus (LocalResultEventBus.current) and the hiltViewModel instances already created in those entry blocks so uploaded photos trigger a paging refresh.
🧹 Nitpick comments (3)
feature/archive/impl/src/main/kotlin/com/neki/android/feature/archive/impl/album/AllAlbumScreen.kt (1)
23-24: Route 결과 발행 boilerplate는 helper로 묶어도 좋겠습니다.이번 PR에서
LocalResultEventBus.current조회와NotifyResult -> sendResult(..., allowDuplicate = false)분기가 여러 Route에 반복됩니다. 작은 helper/extension으로 빼 두면 이후 결과 타입 추가 때 side effect와 marker를 엇갈리게 연결할 가능성을 줄이기 쉽습니다.Also applies to: 46-58
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/archive/impl/src/main/kotlin/com/neki/android/feature/archive/impl/album/AllAlbumScreen.kt` around lines 23 - 24, Multiple routes repeat the same pattern of reading LocalResultEventBus.current and then converting NotifyResult into sendResult(..., allowDuplicate = false); extract that boilerplate into a small helper/extension (e.g., an extension on LocalResultEventBus or a top-level function) to centralize: fetch current bus, handle null-safety, map specific result types like AllAlbumResult/NotifyResult and call sendResult(result, allowDuplicate = false) so callers (the routes in AllAlbumScreen) only call the helper (instead of duplicating the LocalResultEventBus.current check and the NotifyResult branch). Ensure the helper name references LocalResultEventBus.current and NotifyResult/sendResult so you can find and replace the repeated blocks.feature/archive/api/src/main/kotlin/com/neki/android/feature/archive/api/ArchiveResult.kt (1)
3-13: 결과 타입 경계는 지금부터 패키지/모듈 단위로 나눠 두는 편이 좋겠습니다.화면명 기반 marker가
feature/archive/api한 파일에 계속 쌓이면, 이후 photo/album 단위로 쪼갤 때 API 표면이 UI 구조와 같이 잠깁니다. 최소한result/photo,result/album처럼 패키지를 나누거나, 가능하면 소비자 경계별 API로 잘라 두면 다음 단계 모듈 분리에 더 수월합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/archive/api/src/main/kotlin/com/neki/android/feature/archive/api/ArchiveResult.kt` around lines 3 - 13, ArchiveResult과 그 구현체들(PhotoDetailResult, AlbumDetailResult, AllPhotoResult, AllAlbumResult, PhotoUploadedResult)이 한 파일에 모여 있어 UI화면 기반 마커가 API 표면에 고정됩니다; 각 도메인/소비자 경계별로 분리하려면 현재 ArchiveResult 인터페이스와 위 데이터 객체들을 기능별 패키지로 옮기세요(예: result.photo 패키지에 PhotoDetailResult/AllPhotoResult/PhotoUploadedResult, result.album 패키지에 AlbumDetailResult/AllAlbumResult 또는 소비자별 API 패키지로 분리). ArchiveResult 인터페이스는 공통 계약이 필요하면 공유 패키지에 두고, 각 구현체 파일명은 해당 타입명으로 분리해 임포트 경로가 UI가 아닌 도메인/모듈 경계를 반영하도록 수정하세요.feature/archive/impl/src/main/kotlin/com/neki/android/feature/archive/impl/main/ArchiveMainViewModel.kt (1)
48-54: 로딩 상태 및 에러 처리 고려
RefreshArchiveMain은fetchInitialData와 달리isLoading상태를 설정하지 않고 에러 피드백도 제공하지 않습니다. 의도적인 설계(무음 새로고침)일 수 있지만, 사용자가 새로고침이 진행 중인지 알 수 없습니다.병렬
reduce호출은MutableStateFlow.update()의 원자성 덕분에 안전합니다.♻️ 로딩 상태 추가를 원하시면 (선택사항)
ArchiveMainIntent.RefreshArchiveMain -> viewModelScope.launch { + reduce { copy(isLoading = true) } + try { awaitAll( async { fetchFavoriteSummary(reduce) }, async { fetchPhotos(reduce) }, async { fetchFolders(reduce) }, ) + } finally { + reduce { copy(isLoading = false) } + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/archive/impl/src/main/kotlin/com/neki/android/feature/archive/impl/main/ArchiveMainViewModel.kt` around lines 48 - 54, RefreshArchiveMain currently launches parallel fetches (fetchFavoriteSummary, fetchPhotos, fetchFolders) without setting isLoading or surfacing errors; update the RefreshArchiveMain handler (the ArchiveMainIntent.RefreshArchiveMain branch in the viewModelScope.launch that calls awaitAll) to set the view state isLoading = true before starting, catch exceptions from the parallel work (wrap awaitAll in try/catch), update the view state to include an error message or error flag on failure, and always set isLoading = false in a finally block while continuing to call reduce for each successful fetch as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@feature/archive/api/src/main/kotlin/com/neki/android/feature/archive/api/ArchiveResult.kt`:
- Line 13: PhotoUploadedResult currently only triggers handling in
ArchiveEntryProvider's ArchiveMain entry point (dispatching
ArchiveMainIntent.RefreshArchiveMain), so AllPhoto and AlbumDetail screens don't
refresh after uploads; update ArchiveEntryProvider to handle PhotoUploadedResult
in the entry points for AllPhoto and AlbumDetail as well and dispatch
AllPhotoIntent.RefreshPhotos and AlbumDetailIntent.RefreshPhotos respectively
when PhotoUploadedResult is received (ensure you reference the
PhotoUploadedResult enum/object, the ArchiveEntryProvider handlers for AllPhoto
and AlbumDetail, and the intent classes AllPhotoIntent.RefreshPhotos and
AlbumDetailIntent.RefreshPhotos).
---
Outside diff comments:
In
`@feature/archive/impl/src/main/kotlin/com/neki/android/feature/archive/impl/navigation/ArchiveEntryProvider.kt`:
- Around line 88-101: The AllPhoto and AllAlbum entry blocks are missing a
ResultEffect handler for PhotoUploadedResult, so add a
ResultEffect<PhotoUploadedResult>(resultBus) in each entry (the same place where
ResultEffect<PhotoDetailResult> is used) and call the corresponding refresh
intent on the view model (for AllPhoto use
viewModel.store.onIntent(AllPhotoIntent.RefreshPhotos); for AllAlbum use
viewModel.store.onIntent(AllAlbumIntent.RefreshAlbums)); reuse the existing
resultBus (LocalResultEventBus.current) and the hiltViewModel instances already
created in those entry blocks so uploaded photos trigger a paging refresh.
---
Nitpick comments:
In
`@feature/archive/api/src/main/kotlin/com/neki/android/feature/archive/api/ArchiveResult.kt`:
- Around line 3-13: ArchiveResult과 그 구현체들(PhotoDetailResult, AlbumDetailResult,
AllPhotoResult, AllAlbumResult, PhotoUploadedResult)이 한 파일에 모여 있어 UI화면 기반 마커가
API 표면에 고정됩니다; 각 도메인/소비자 경계별로 분리하려면 현재 ArchiveResult 인터페이스와 위 데이터 객체들을 기능별 패키지로
옮기세요(예: result.photo 패키지에 PhotoDetailResult/AllPhotoResult/PhotoUploadedResult,
result.album 패키지에 AlbumDetailResult/AllAlbumResult 또는 소비자별 API 패키지로 분리).
ArchiveResult 인터페이스는 공통 계약이 필요하면 공유 패키지에 두고, 각 구현체 파일명은 해당 타입명으로 분리해 임포트 경로가 UI가
아닌 도메인/모듈 경계를 반영하도록 수정하세요.
In
`@feature/archive/impl/src/main/kotlin/com/neki/android/feature/archive/impl/album/AllAlbumScreen.kt`:
- Around line 23-24: Multiple routes repeat the same pattern of reading
LocalResultEventBus.current and then converting NotifyResult into
sendResult(..., allowDuplicate = false); extract that boilerplate into a small
helper/extension (e.g., an extension on LocalResultEventBus or a top-level
function) to centralize: fetch current bus, handle null-safety, map specific
result types like AllAlbumResult/NotifyResult and call sendResult(result,
allowDuplicate = false) so callers (the routes in AllAlbumScreen) only call the
helper (instead of duplicating the LocalResultEventBus.current check and the
NotifyResult branch). Ensure the helper name references
LocalResultEventBus.current and NotifyResult/sendResult so you can find and
replace the repeated blocks.
In
`@feature/archive/impl/src/main/kotlin/com/neki/android/feature/archive/impl/main/ArchiveMainViewModel.kt`:
- Around line 48-54: RefreshArchiveMain currently launches parallel fetches
(fetchFavoriteSummary, fetchPhotos, fetchFolders) without setting isLoading or
surfacing errors; update the RefreshArchiveMain handler (the
ArchiveMainIntent.RefreshArchiveMain branch in the viewModelScope.launch that
calls awaitAll) to set the view state isLoading = true before starting, catch
exceptions from the parallel work (wrap awaitAll in try/catch), update the view
state to include an error message or error flag on failure, and always set
isLoading = false in a finally block while continuing to call reduce for each
successful fetch as before.
🪄 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
Run ID: abb2a7fc-93cf-49a6-a04f-74d74d6f4b3a
📒 Files selected for processing (17)
app/src/main/java/com/neki/android/app/main/MainScreen.ktfeature/archive/api/src/main/kotlin/com/neki/android/feature/archive/api/ArchiveResult.ktfeature/archive/impl/src/main/kotlin/com/neki/android/feature/archive/impl/album/AllAlbumContract.ktfeature/archive/impl/src/main/kotlin/com/neki/android/feature/archive/impl/album/AllAlbumScreen.ktfeature/archive/impl/src/main/kotlin/com/neki/android/feature/archive/impl/album/AllAlbumViewModel.ktfeature/archive/impl/src/main/kotlin/com/neki/android/feature/archive/impl/album_detail/AlbumDetailContract.ktfeature/archive/impl/src/main/kotlin/com/neki/android/feature/archive/impl/album_detail/AlbumDetailScreen.ktfeature/archive/impl/src/main/kotlin/com/neki/android/feature/archive/impl/album_detail/AlbumDetailViewModel.ktfeature/archive/impl/src/main/kotlin/com/neki/android/feature/archive/impl/main/ArchiveMainContract.ktfeature/archive/impl/src/main/kotlin/com/neki/android/feature/archive/impl/main/ArchiveMainViewModel.ktfeature/archive/impl/src/main/kotlin/com/neki/android/feature/archive/impl/navigation/ArchiveEntryProvider.ktfeature/archive/impl/src/main/kotlin/com/neki/android/feature/archive/impl/photo/AllPhotoContract.ktfeature/archive/impl/src/main/kotlin/com/neki/android/feature/archive/impl/photo/AllPhotoScreen.ktfeature/archive/impl/src/main/kotlin/com/neki/android/feature/archive/impl/photo/AllPhotoViewModel.ktfeature/archive/impl/src/main/kotlin/com/neki/android/feature/archive/impl/photo_detail/PhotoDetailContract.ktfeature/archive/impl/src/main/kotlin/com/neki/android/feature/archive/impl/photo_detail/PhotoDetailScreen.ktfeature/archive/impl/src/main/kotlin/com/neki/android/feature/archive/impl/photo_detail/PhotoDetailViewModel.kt
|
|
||
| data object AllAlbumResult : ArchiveResult | ||
|
|
||
| data object PhotoUploadedResult : ArchiveResult |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== ArchiveEntryProvider =="
fd 'ArchiveEntryProvider\.kt$' -x sed -n '1,320p' {}
echo
echo "== PhotoUploadedResult consumers and follow-up refresh intents =="
rg -n -C3 --glob '!**/build/**' --glob '!**/dist/**' '\bPhotoUploadedResult\b|\bRefreshArchiveMain\b|\bRefreshPhotos\b|\bRefreshAlbums\b' .Repository: YAPP-Github/27th-App-Team-2-Android
Length of output: 30061
PhotoUploadedResult의 이벤트 전파(fan-out)가 불완전합니다.
현재 PhotoUploadedResult는 ArchiveEntryProvider의 ArchiveMain 진입점에서만 소비되어 ArchiveMainIntent.RefreshArchiveMain을 dispatch합니다. 하지만 AllPhoto와 AlbumDetail 화면은 이 결과를 전혀 처리하지 않아, 사용자가 업로드 후 이 화면들에 머물고 있다면 새로운 사진이 반영되지 않습니다.
AllPhoto와 AlbumDetail도 업로드 이벤트에 응답하도록, ArchiveEntryProvider의 해당 진입점들에서 PhotoUploadedResult를 처리하고 각각 AllPhotoIntent.RefreshPhotos와 AlbumDetailIntent.RefreshPhotos를 dispatch해야 합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@feature/archive/api/src/main/kotlin/com/neki/android/feature/archive/api/ArchiveResult.kt`
at line 13, PhotoUploadedResult currently only triggers handling in
ArchiveEntryProvider's ArchiveMain entry point (dispatching
ArchiveMainIntent.RefreshArchiveMain), so AllPhoto and AlbumDetail screens don't
refresh after uploads; update ArchiveEntryProvider to handle PhotoUploadedResult
in the entry points for AllPhoto and AlbumDetail as well and dispatch
AllPhotoIntent.RefreshPhotos and AlbumDetailIntent.RefreshPhotos respectively
when PhotoUploadedResult is received (ensure you reference the
PhotoUploadedResult enum/object, the ArchiveEntryProvider handlers for AllPhoto
and AlbumDetail, and the intent classes AllPhotoIntent.RefreshPhotos and
AlbumDetailIntent.RefreshPhotos).
Ojongseok
approved these changes
Apr 6, 2026
Member
Ojongseok
left a comment
Ojongseok
left a comment
There was a problem hiding this comment.
고생 많으셨습니다👍👍
후 암만 고민해봐도 더 나은 방법이 생각나지 않네요..! SharedFlow 라던지 Result 타입을 분리하기 전 ResultEvent를 보낸 송신처(?)를 함께 기록해서 던진다던지 하는 방법을 고민해봤는데 구현해주신 방법 보다 획기적으로 나은 방법이 떠오르지 않네요.. 몬가 좀 더 국룰적인(?) 방법이 있을 것 같은데 말이죠. Nav3에서 Event 핸들링하는 레퍼런스들을 찾아보면서 같이 고민해 보겠습니닷
Q. 아카이빙 모듈 내에 각 화면들에 대한 역할이 점점 커져가서, 이 화면들을 모듈로 나누는 편이 나을까요?
A. 그러게욥. 아카이빙이 주로 3, 4차 스프린트에 포함이 되면서 photo와 album이 점점 거대해지고 있꾼요
feature:archive:api
feature:archive:main:impl
feature:archive:album:impl
feature:archive:photo:impl
이런 느낌으로 나누는건 어떨까요? 아카이빙 탭 내 화면 전환을 담당하는 api 하나와 메인, 사진, 앨범 정도로요!?
Comment on lines
+57
to
+70
| ResultEffect<PhotoDetailResult>(resultBus) { | ||
| viewModel.store.onIntent(ArchiveMainIntent.RefreshArchiveMain) | ||
| } | ||
| ResultEffect<AlbumDetailResult>(resultBus) { | ||
| viewModel.store.onIntent(ArchiveMainIntent.RefreshArchiveMain) | ||
| } | ||
| ResultEffect<AllPhotoResult>(resultBus) { | ||
| viewModel.store.onIntent(ArchiveMainIntent.RefreshArchiveMain) | ||
| } | ||
| ResultEffect<AllAlbumResult>(resultBus) { | ||
| viewModel.store.onIntent(ArchiveMainIntent.RefreshArchiveMain) | ||
| } | ||
| ResultEffect<PhotoUploadedResult>(resultBus) { | ||
| viewModel.store.onIntent(ArchiveMainIntent.RefreshArchiveMain) |
Member
There was a problem hiding this comment.
현재 어떤 ArchiveResult를 받더라도 동일한 Intent를 전달하고 있는데 이렇다면 전달할 Intent를 기준으로 RefreshArrchiveMain 이런 ArchiveResult를 하나만 던지는건 어떤가요?
추후 어차피 세부 이벤트 처리를 위해 나누어질 예정이라면 그냥 두시죠 !
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
feature/archive/impl/src/main/kotlin/com/neki/android/feature/archive/impl/photo_detail/PhotoDetailViewModel.kt (1)
96-105: 페이지 변경 시 memo 초기화 로직 확인 필요
PageChanged처리에서newIndex를photos.size로 모듈로 연산하고 있지만, Line 91-93의ClickRightPhoto에서는lastIndex를 경계로 사용합니다. 이로 인해 두 곳에서 인덱스 계산 방식이 다릅니다.또한,
photos.getOrNull(newIndex)?.memo.orEmpty()로 memo를 가져오는데, 이미currentIndexgetter가 있으므로 일관성을 위해 이를 활용하는 것이 좋을 수 있습니다.💡 제안: currentIndex 활용
is PhotoDetailIntent.PageChanged -> { reduce { - val newIndex = if (photos.isEmpty()) 0 else intent.page % photos.size copy( currentPage = intent.page, - memo = photos.getOrNull(newIndex)?.memo.orEmpty(), + memo = photos.getOrNull(currentIndex)?.memo.orEmpty(), ) } preloadIfNeeded(reduce) }단,
copy내부에서currentIndex를 참조하면 새currentPage가 적용되지 않은 상태일 수 있으므로, 순서에 주의가 필요합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/archive/impl/src/main/kotlin/com/neki/android/feature/archive/impl/photo_detail/PhotoDetailViewModel.kt` around lines 96 - 105, PageChanged currently computes newIndex with modulo while ClickRightPhoto uses lastIndex boundary and it also reads memo inside copy (can't rely on currentIndex getter there); change PageChanged to compute the target index using the same boundary logic as ClickRightPhoto (use photos.lastIndex when non-empty, or 0 when empty), compute the memo from photos.getOrNull(newIndex)?.memo.orEmpty() into a local variable before calling copy, then call copy(currentPage = intent.page, memo = newMemo) and keep the existing preloadIfNeeded(reduce) call; reference: PhotoDetailIntent.PageChanged, ClickRightPhoto, currentIndex getter, photos, reduce, preloadIfNeeded.feature/archive/impl/src/main/kotlin/com/neki/android/feature/archive/impl/photo_detail/component/PhotoDetailActionBar.kt (1)
89-104: Preview 함수명이 실제 테스트 내용과 불일치합니다.
PhotoDetailActionBarMemoActivePreview가isFavorite = true만 전달하고 있어 실제로 메모 활성 상태를 테스트하지 않습니다. 현재PhotoDetailActionBar에는 메모 활성 상태를 표시하는 파라미터가 없어서 프리뷰에서 메모 상태를 확인할 수 없습니다.💡 제안: 명확한 프리뷰 이름 사용
`@ComponentPreview` `@Composable` -private fun PhotoDetailActionBarClosedPreview() { +private fun PhotoDetailActionBarNotFavoritePreview() { NekiTheme { PhotoDetailActionBar( isFavorite = false, ) } } `@ComponentPreview` `@Composable` -private fun PhotoDetailActionBarMemoActivePreview() { +private fun PhotoDetailActionBarFavoritePreview() { NekiTheme { PhotoDetailActionBar( isFavorite = true, ) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/archive/impl/src/main/kotlin/com/neki/android/feature/archive/impl/photo_detail/component/PhotoDetailActionBar.kt` around lines 89 - 104, The preview function PhotoDetailActionBarMemoActivePreview is misnamed and doesn't test "memo active" because PhotoDetailActionBar has no memo state parameter; either rename the preview to reflect it only toggles isFavorite (e.g., PhotoDetailActionBarFavoriteActivePreview) or add a memo state parameter to PhotoDetailActionBar (e.g., memoActive: Boolean) and update PhotoDetailActionBarMemoActivePreview to pass memoActive = true (and adjust other previews like PhotoDetailActionBarClosedPreview accordingly); change the preview name or add the new parameter in the PhotoDetailActionBar composable to make the preview semantics correct.feature/archive/impl/src/main/kotlin/com/neki/android/feature/archive/impl/photo_detail/PhotoDetailScreen.kt (1)
136-137:isScrollInProgress와isTapEnabled가 서로 다른 상태 소스에서 파생됩니다.
isScrollInProgress는PagerState에서,isTapEnabled는uiState.memoModeOf()에서 가져옵니다. 이 두 값이 동일 프레임 내에서 일관성이 보장되지 않을 수 있어 탭 핸들링 조건문(isScrollInProgress && isTapEnabled)에서 예상치 못한 동작이 발생할 가능성이 있습니다.현재 로직상 큰 문제는 아니지만, 복잡한 상태 조합 시 주의가 필요합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/archive/impl/src/main/kotlin/com/neki/android/feature/archive/impl/photo_detail/PhotoDetailScreen.kt` around lines 136 - 137, The two booleans come from different state sources (pagerState.isScrollInProgress vs pageMemoMode / MemoMode) and can become inconsistent across frames; replace the separate checks by deriving a single composed state (e.g., using derivedStateOf) that reads pagerState.isScrollInProgress and pageMemoMode together and exposes a single flag (e.g., isTapAllowed or isTapEnabled) to use in the tap-handling logic and where isTapEnabled is passed down; update usages of pagerState.isScrollInProgress and the existing pageMemoMode check to consume this single derived boolean so they are snapshot-consistent.feature/archive/impl/src/main/kotlin/com/neki/android/feature/archive/impl/photo_detail/component/MemoTextField.kt (3)
59-73: 두 LaunchedEffect 간 메모 텍스트 초기화 로직 중복Lines 59-63과 Lines 65-73 모두
memoState.edit { replace(...) }를 수행합니다.MemoMode.Editing진입 시 두 Effect가 동시에 실행될 수 있어 텍스트가 두 번 교체됩니다. 동일한 값이므로 기능상 문제는 없지만, 로직이 분산되어 유지보수가 어렵습니다.하나의 LaunchedEffect로 통합하는 것을 고려해 주세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/archive/impl/src/main/kotlin/com/neki/android/feature/archive/impl/photo_detail/component/MemoTextField.kt` around lines 59 - 73, Combine the two LaunchedEffect blocks into a single LaunchedEffect that observes both memo and memoMode (e.g., LaunchedEffect(memo, memoMode)); inside it, perform the memoState.edit { replace(0, length, memo) } only when entering MemoMode.Editing, call focusRequester.requestFocus() when memoMode == MemoMode.Editing, call keyboardController?.hide() when previousMemoMode == MemoMode.Editing and memoMode != MemoMode.Editing, and finally update previousMemoMode = memoMode at the end; use the existing symbols memoState, memo, memoMode, previousMemoMode, focusRequester, and keyboardController to locate and change the logic.
177-190: readOnly 필드에 불필요한 inputTransformation
readOnly = true로 설정된BasicTextField에inputTransformation = InputTransformation.maxLength(100)은 불필요합니다. 입력이 불가능하므로 길이 제한이 적용되지 않습니다.♻️ 제안하는 수정 사항
BasicTextField( state = memoState, modifier = Modifier .fillMaxWidth() .focusRequester(focusRequester) .onFocusChanged { if (it.isFocused) onClickMemoText() }, textStyle = NekiTheme.typography.body16Medium, readOnly = true, - inputTransformation = InputTransformation.maxLength(100), cursorBrush = SolidColor(Color.Transparent), decorator = { innerTextField -> innerTextField() }, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/archive/impl/src/main/kotlin/com/neki/android/feature/archive/impl/photo_detail/component/MemoTextField.kt` around lines 177 - 190, The BasicTextField in MemoTextField sets inputTransformation = InputTransformation.maxLength(100) while readOnly = true; remove the unnecessary inputTransformation (or conditionally apply it only when the field is editable) so that inputTransformation is not applied for the read-only BasicTextField (refer to BasicTextField, MemoTextField, memoState, and the inputTransformation parameter).
75-80: snapshotFlow 초기 값 방출로 인한 불필요한 콜백 발생
snapshotFlow는collect시작 시 현재 값을 즉시 방출합니다.MemoMode.Editing진입 시 Line 67에서 이미 텍스트를 설정했으므로, 동일한 값으로onMemoTextChanged가 불필요하게 호출됩니다.
distinctUntilChanged()를 추가하거나 이전 값과 비교하여 중복 콜백을 방지하세요.♻️ 제안하는 수정 사항
LaunchedEffect(memoState, memoMode) { if (memoMode == MemoMode.Editing) { snapshotFlow { memoState.text.toString() } + .distinctUntilChanged() .collect { text -> onMemoTextChanged(text) } } }
distinctUntilChangedimport 추가:import kotlinx.coroutines.flow.distinctUntilChanged🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/archive/impl/src/main/kotlin/com/neki/android/feature/archive/impl/photo_detail/component/MemoTextField.kt` around lines 75 - 80, The snapshotFlow used inside LaunchedEffect is emitting the current memoState.text immediately causing a duplicate onMemoTextChanged call when entering MemoMode.Editing; fix this by applying distinctUntilChanged() to the snapshotFlow (import kotlinx.coroutines.flow.distinctUntilChanged) or manually compare previous value before invoking onMemoTextChanged so that snapshotFlow { memoState.text.toString() } only forwards changes, keeping the LaunchedEffect(memoState, memoMode) and the onMemoTextChanged call intact.feature/archive/impl/src/main/kotlin/com/neki/android/feature/archive/impl/photo_detail/component/PhotoDetailImageItem.kt (1)
108-112: IME 패딩 로직의 의도 명확화 필요
MemoMode.Editing이 아닐 때WindowInsets.ime.exclude(WindowInsets(bottom = actionBarHeight))를 사용하는 로직이 복잡합니다. IME가 표시될 때 액션바 높이를 제외하는 의도가 명확하지 않아 예상치 못한 레이아웃 문제가 발생할 수 있습니다.의도한 동작에 대한 주석을 추가하거나, 단순화를 검토해 주세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/archive/impl/src/main/kotlin/com/neki/android/feature/archive/impl/photo_detail/component/PhotoDetailImageItem.kt` around lines 108 - 112, The IME padding branch is unclear: when memoMode != MemoMode.Editing the code uses Modifier.windowInsetsPadding(WindowInsets.ime.exclude(WindowInsets(bottom = actionBarHeight))) which is complex and risks layout bugs; clarify intent by adding a concise comment explaining why the actionBarHeight is excluded when IME is shown (or simplify to consistent IME handling), and if simplification is chosen replace the exclude logic with a straightforward IME padding approach (e.g., use Modifier.windowInsetsPadding(WindowInsets.ime) or keep Modifier.imePadding()) so behavior matches the editing branch; update the code around memoMode, MemoMode.Editing, Modifier.imePadding(), Modifier.windowInsetsPadding(...), WindowInsets.ime.exclude(...), and actionBarHeight accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@feature/archive/impl/src/main/kotlin/com/neki/android/feature/archive/impl/photo_detail/PhotoDetailScreen.kt`:
- Line 103: actionBarHeightDp이 0.dp로 초기화되어 onSizeChanged로 측정되기 전까지 IME 패딩 계산이
잘못될 수 있으니 actionBarHeightDp를 0.dp 대신 예상 높이(예: 56.dp 또는 테마에서 가져온 ActionBar 높이)로
초기화하거나, PhotoDetailImageItem의 편집 진입 로직을 onSizeChanged가 완료될 때까지(측정 플래그 사용) 차단하도록
변경하세요; 관련 식별자: actionBarHeightDp, onSizeChanged, PhotoDetailImageItem,
WindowInsets.ime.exclude(WindowInsets(bottom = actionBarHeight)).
---
Nitpick comments:
In
`@feature/archive/impl/src/main/kotlin/com/neki/android/feature/archive/impl/photo_detail/component/MemoTextField.kt`:
- Around line 59-73: Combine the two LaunchedEffect blocks into a single
LaunchedEffect that observes both memo and memoMode (e.g., LaunchedEffect(memo,
memoMode)); inside it, perform the memoState.edit { replace(0, length, memo) }
only when entering MemoMode.Editing, call focusRequester.requestFocus() when
memoMode == MemoMode.Editing, call keyboardController?.hide() when
previousMemoMode == MemoMode.Editing and memoMode != MemoMode.Editing, and
finally update previousMemoMode = memoMode at the end; use the existing symbols
memoState, memo, memoMode, previousMemoMode, focusRequester, and
keyboardController to locate and change the logic.
- Around line 177-190: The BasicTextField in MemoTextField sets
inputTransformation = InputTransformation.maxLength(100) while readOnly = true;
remove the unnecessary inputTransformation (or conditionally apply it only when
the field is editable) so that inputTransformation is not applied for the
read-only BasicTextField (refer to BasicTextField, MemoTextField, memoState, and
the inputTransformation parameter).
- Around line 75-80: The snapshotFlow used inside LaunchedEffect is emitting the
current memoState.text immediately causing a duplicate onMemoTextChanged call
when entering MemoMode.Editing; fix this by applying distinctUntilChanged() to
the snapshotFlow (import kotlinx.coroutines.flow.distinctUntilChanged) or
manually compare previous value before invoking onMemoTextChanged so that
snapshotFlow { memoState.text.toString() } only forwards changes, keeping the
LaunchedEffect(memoState, memoMode) and the onMemoTextChanged call intact.
In
`@feature/archive/impl/src/main/kotlin/com/neki/android/feature/archive/impl/photo_detail/component/PhotoDetailActionBar.kt`:
- Around line 89-104: The preview function PhotoDetailActionBarMemoActivePreview
is misnamed and doesn't test "memo active" because PhotoDetailActionBar has no
memo state parameter; either rename the preview to reflect it only toggles
isFavorite (e.g., PhotoDetailActionBarFavoriteActivePreview) or add a memo state
parameter to PhotoDetailActionBar (e.g., memoActive: Boolean) and update
PhotoDetailActionBarMemoActivePreview to pass memoActive = true (and adjust
other previews like PhotoDetailActionBarClosedPreview accordingly); change the
preview name or add the new parameter in the PhotoDetailActionBar composable to
make the preview semantics correct.
In
`@feature/archive/impl/src/main/kotlin/com/neki/android/feature/archive/impl/photo_detail/component/PhotoDetailImageItem.kt`:
- Around line 108-112: The IME padding branch is unclear: when memoMode !=
MemoMode.Editing the code uses
Modifier.windowInsetsPadding(WindowInsets.ime.exclude(WindowInsets(bottom =
actionBarHeight))) which is complex and risks layout bugs; clarify intent by
adding a concise comment explaining why the actionBarHeight is excluded when IME
is shown (or simplify to consistent IME handling), and if simplification is
chosen replace the exclude logic with a straightforward IME padding approach
(e.g., use Modifier.windowInsetsPadding(WindowInsets.ime) or keep
Modifier.imePadding()) so behavior matches the editing branch; update the code
around memoMode, MemoMode.Editing, Modifier.imePadding(),
Modifier.windowInsetsPadding(...), WindowInsets.ime.exclude(...), and
actionBarHeight accordingly.
In
`@feature/archive/impl/src/main/kotlin/com/neki/android/feature/archive/impl/photo_detail/PhotoDetailScreen.kt`:
- Around line 136-137: The two booleans come from different state sources
(pagerState.isScrollInProgress vs pageMemoMode / MemoMode) and can become
inconsistent across frames; replace the separate checks by deriving a single
composed state (e.g., using derivedStateOf) that reads
pagerState.isScrollInProgress and pageMemoMode together and exposes a single
flag (e.g., isTapAllowed or isTapEnabled) to use in the tap-handling logic and
where isTapEnabled is passed down; update usages of
pagerState.isScrollInProgress and the existing pageMemoMode check to consume
this single derived boolean so they are snapshot-consistent.
In
`@feature/archive/impl/src/main/kotlin/com/neki/android/feature/archive/impl/photo_detail/PhotoDetailViewModel.kt`:
- Around line 96-105: PageChanged currently computes newIndex with modulo while
ClickRightPhoto uses lastIndex boundary and it also reads memo inside copy
(can't rely on currentIndex getter there); change PageChanged to compute the
target index using the same boundary logic as ClickRightPhoto (use
photos.lastIndex when non-empty, or 0 when empty), compute the memo from
photos.getOrNull(newIndex)?.memo.orEmpty() into a local variable before calling
copy, then call copy(currentPage = intent.page, memo = newMemo) and keep the
existing preloadIfNeeded(reduce) call; reference: PhotoDetailIntent.PageChanged,
ClickRightPhoto, currentIndex getter, photos, reduce, preloadIfNeeded.
🪄 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
Run ID: f7860ba7-7c75-410b-a627-76e3fb7fb422
📒 Files selected for processing (13)
core/data-api/src/main/java/com/neki/android/core/dataapi/repository/PhotoRepository.ktcore/data/src/main/java/com/neki/android/core/data/remote/api/PhotoService.ktcore/data/src/main/java/com/neki/android/core/data/remote/model/request/UpdateMemoRequest.ktcore/data/src/main/java/com/neki/android/core/data/remote/model/response/PhotoResponse.ktcore/data/src/main/java/com/neki/android/core/data/repository/impl/PhotoRepositoryImpl.ktcore/designsystem/src/main/res/drawable/icon_memo.xmlcore/model/src/main/java/com/neki/android/core/model/Photo.ktfeature/archive/impl/src/main/kotlin/com/neki/android/feature/archive/impl/photo_detail/PhotoDetailContract.ktfeature/archive/impl/src/main/kotlin/com/neki/android/feature/archive/impl/photo_detail/PhotoDetailScreen.ktfeature/archive/impl/src/main/kotlin/com/neki/android/feature/archive/impl/photo_detail/PhotoDetailViewModel.ktfeature/archive/impl/src/main/kotlin/com/neki/android/feature/archive/impl/photo_detail/component/MemoTextField.ktfeature/archive/impl/src/main/kotlin/com/neki/android/feature/archive/impl/photo_detail/component/PhotoDetailActionBar.ktfeature/archive/impl/src/main/kotlin/com/neki/android/feature/archive/impl/photo_detail/component/PhotoDetailImageItem.kt
✅ Files skipped from review due to trivial changes (2)
- core/data/src/main/java/com/neki/android/core/data/remote/model/request/UpdateMemoRequest.kt
- core/designsystem/src/main/res/drawable/icon_memo.xml
...impl/src/main/kotlin/com/neki/android/feature/archive/impl/photo_detail/PhotoDetailScreen.kt
Show resolved
Hide resolved
Channel 1:1 소비 구조로 인해 AllPhoto, AlbumDetail, AllAlbum이 Result를 소비하면 ArchiveMain에 전달되지 않는 문제를 릴레이 패턴으로 해결
There was a problem hiding this comment.
🧹 Nitpick comments (1)
feature/archive/impl/src/main/kotlin/com/neki/android/feature/archive/impl/navigation/ArchiveEntryProvider.kt (1)
57-68: 중복 Refresh 호출 가능성 검토 필요Archive main이 직접 child result(
PhotoDetailResult,AlbumDetailResult)와 중간 화면에서 relay된 result(AllPhotoResult,AllAlbumResult) 모두를 구독하고 있습니다.예를 들어,
PhotoDetail에서PhotoDetailResult를 emit하면:
- Archive main이
PhotoDetailResult를 수신 →RefreshArchiveMain(1회)AllPhoto가PhotoDetailResult를 수신 →AllPhotoResultemit- Archive main이
AllPhotoResult를 수신 →RefreshArchiveMain(2회)
RefreshArchiveMain이 idempotent하거나 내부적으로 debounce 처리가 되어 있다면 문제없지만, 그렇지 않다면 불필요한 중복 API 호출이 발생할 수 있습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/archive/impl/src/main/kotlin/com/neki/android/feature/archive/impl/navigation/ArchiveEntryProvider.kt` around lines 57 - 68, Archive main is currently subscribed to both child results and relayed results causing duplicate RefreshArchiveMain triggers; remove the direct ResultEffect subscriptions for PhotoDetailResult and AlbumDetailResult (the blocks calling viewModel.store.onIntent(ArchiveMainIntent.RefreshArchiveMain)) and rely only on the relayed AllPhotoResult/AllAlbumResult handlers, or alternatively implement a de-dup/debounce in the store's onIntent handling (e.g., track last refresh timestamp or result id in the ViewModel/store) so multiple emissions do not cause duplicate API calls; update the ResultEffect blocks around ResultEffect<PhotoDetailResult>, ResultEffect<AlbumDetailResult>, ResultEffect<AllPhotoResult>, and ResultEffect<AllAlbumResult> and the ArchiveMainIntent.RefreshArchiveMain handling accordingly.
🤖 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/archive/impl/src/main/kotlin/com/neki/android/feature/archive/impl/navigation/ArchiveEntryProvider.kt`:
- Around line 57-68: Archive main is currently subscribed to both child results
and relayed results causing duplicate RefreshArchiveMain triggers; remove the
direct ResultEffect subscriptions for PhotoDetailResult and AlbumDetailResult
(the blocks calling
viewModel.store.onIntent(ArchiveMainIntent.RefreshArchiveMain)) and rely only on
the relayed AllPhotoResult/AllAlbumResult handlers, or alternatively implement a
de-dup/debounce in the store's onIntent handling (e.g., track last refresh
timestamp or result id in the ViewModel/store) so multiple emissions do not
cause duplicate API calls; update the ResultEffect blocks around
ResultEffect<PhotoDetailResult>, ResultEffect<AlbumDetailResult>,
ResultEffect<AllPhotoResult>, and ResultEffect<AllAlbumResult> and the
ArchiveMainIntent.RefreshArchiveMain handling accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f7a8df89-cb54-48e9-9458-145565c073ed
📒 Files selected for processing (1)
feature/archive/impl/src/main/kotlin/com/neki/android/feature/archive/impl/navigation/ArchiveEntryProvider.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
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.
🔗 관련 이슈
📙 작업 설명
ResultEventBus의 Channel 1:1 소비 구조에서 송신 화면이 자기 이벤트를 소비하는 문제 해결ArchiveResult를 화면별data object로 분리하여 Channel 키 분리PhotoDetailResult,AlbumDetailResult,AllPhotoResult,AllAlbumResult,PhotoUploadedResultArchiveMain의RefreshArchiveMain이 사진뿐 아니라 즐겨찾기 + 앨범까지 병렬 갱신하도록 변경🧪 테스트 내역 (선택)
PhotoDetail → ArchiveMain
PhotoDetail → AllPhoto
PhotoDetail → AlbumDetail
AlbumDetail → ArchiveMain
AlbumDetail → AllAlbum
AllPhoto → ArchiveMain
AllAlbum → ArchiveMain
📸 스크린샷 또는 시연 영상 (선택)
💬 추가 설명 or 리뷰 포인트 (선택)
ResultEventBus가 Channel 기반 1:1 소비 구조라, 모든 화면이 동일한ArchiveResult타입을 사용하면 송신 화면의ResultEffect가 자기가 보낸 이벤트를 먼저 소비해 상위 화면에 전달되지 않는 문제가 있었습니다.ArchiveResult.FavoriteChanged를 송신 → AllPhoto 자신의ResultEffect<ArchiveResult>가 먼저 소비 → ArchiveMain에 도달하지 않음data object만 방출하고 있으며, 추후 기능 추가 시 별도 이슈로 세부 Result 이벤트를 추가할 예정입니다.PhotoDetailResult→PhotoDetailResult.FavoriteChanged(photoId, isFavorite),PhotoDetailResult.PhotoDeleted(photoIds)등Q. 아카이빙 모듈 내에 각 화면들에 대한 역할이 점점 커져가서, 이 화면들을 모듈로 나누는 편이 나을까요?
Summary by CodeRabbit