Conversation
|
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북마크 콘텐츠 목록 화면을 Fragment에서 Compose로 마이그레이션하고, 데이터 모델을 PagedBookmarkContents에서 제네릭 Page로 개선하며, 패키지 명명을 단수형(account, bookmark)으로 정규화합니다. Changes
Sequence DiagramsequenceDiagram
actor User
participant BookmarkContentActivity
participant BookmarkContentListScreen
participant BookmarkContentListViewModel
participant BookmarkRepository
participant BookmarkRemoteDataSource
User->>BookmarkContentActivity: 북마크 콘텐츠 열기
activate BookmarkContentActivity
BookmarkContentActivity->>BookmarkContentListScreen: 화면 구성
activate BookmarkContentListScreen
BookmarkContentListScreen->>BookmarkContentListViewModel: 초기화
activate BookmarkContentListViewModel
BookmarkContentListViewModel->>BookmarkRepository: loadBookmarks(REFRESH)
activate BookmarkRepository
BookmarkRepository->>BookmarkRemoteDataSource: 북마크 목록 조회
activate BookmarkRemoteDataSource
BookmarkRemoteDataSource-->>BookmarkRepository: Page<BookmarkContent>
deactivate BookmarkRemoteDataSource
BookmarkRepository-->>BookmarkContentListViewModel: Page<BookmarkContent>
deactivate BookmarkRepository
BookmarkContentListViewModel->>BookmarkRepository: loadBookmarkCount()
activate BookmarkRepository
BookmarkRepository->>BookmarkRemoteDataSource: 북마크 개수 조회
activate BookmarkRemoteDataSource
BookmarkRemoteDataSource-->>BookmarkRepository: Int
deactivate BookmarkRemoteDataSource
BookmarkRepository-->>BookmarkContentListViewModel: Int
deactivate BookmarkRepository
BookmarkContentListViewModel-->>BookmarkContentListScreen: 상태 업데이트 (items, totalCount)
deactivate BookmarkContentListViewModel
User->>BookmarkContentListScreen: 아이템 클릭 / 스크롤
BookmarkContentListScreen->>BookmarkContentListViewModel: loadMore() / removeBookmark()
activate BookmarkContentListViewModel
BookmarkContentListViewModel->>BookmarkRepository: loadBookmarks(APPEND)
BookmarkRepository-->>BookmarkContentListViewModel: Page<BookmarkContent>
BookmarkContentListViewModel-->>BookmarkContentListScreen: 상태 업데이트 (append mode)
deactivate BookmarkContentListViewModel
BookmarkContentListScreen-->>User: 콘텐츠 목록 표시
deactivate BookmarkContentListScreen
deactivate BookmarkContentActivity
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 17
> [!CAUTION]
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)android/app/src/main/java/com/on/turip/ui/compose/bookmarks/component/BookmarkContentAppBar.kt (1)
23-28:⚠️ Potential issue | 🟡 Minor뒤로가기 아이콘의 최소 터치 영역이 보장되지 않습니다.
Icon에Modifier.clickable만 적용하면 아이콘 크기(기본 24dp) 자체가 터치 영역이 됩니다. Material 가이드라인에서 권장하는 최소 48dp 터치 타겟을 충족하지 못해 사용자가 누르기 어려울 수 있습니다.IconButton으로 감싸면 자동으로 48dp 최소 터치 영역이 보장됩니다.🔧 IconButton 사용 제안
start = { - Icon( - imageVector = Icons.AutoMirrored.Default.ArrowBack, - contentDescription = stringResource(R.string.all_back_description), - modifier = Modifier.clickable(onClick = onBackClick), - ) + IconButton(onClick = onBackClick) { + Icon( + imageVector = Icons.AutoMirrored.Default.ArrowBack, + contentDescription = stringResource(R.string.all_back_description), + ) + } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/com/on/turip/ui/compose/bookmarks/component/BookmarkContentAppBar.kt` around lines 23 - 28, The back arrow Icon in BookmarkContentAppBar uses Modifier.clickable directly so its touch target is just the 24dp icon; wrap the Icon with an IconButton (or replace the clickable modifier with a container using minimumInteractiveComponentSize(48.dp)) to guarantee the 48dp minimum touch target per Material guidelines; update the start slot where Icon(...) is defined to use IconButton(onClick = onBackClick) { Icon(...) } (or apply Modifier.sizeIn/minimumInteractiveComponentSize to the parent) so the touch area for the back action is accessible.
♻️ Duplicate comments (1)
android/app/src/main/java/com/on/turip/ui/compose/mypage/component/BookmarkedContentItem.kt (1)
84-201:BookmarkContentItem.kt와의 중복 — 공통 composable 추출을 권장합니다.이 파일의
ContentThumbnail,ContentInformation,ContentInfoItem은bookmarks/component/BookmarkContentItem.kt의 동일 이름 함수들과 거의 동일합니다. 앞서 해당 파일 리뷰에서 언급한 대로, 이 세 composable을 공통 위치로 추출하여 재사용하는 것을 권장합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/com/on/turip/ui/compose/mypage/component/BookmarkedContentItem.kt` around lines 84 - 201, This code duplicates three composables (ContentThumbnail, ContentInformation, ContentInfoItem) already present in bookmarks/component/BookmarkContentItem.kt; extract these shared composables into a single common file (e.g., a ui/compose/common or shared composables file) and update both BookmarkContentItem.kt and this file to import and call the shared versions; ensure the shared file exposes the same function signatures (including BookmarkContent parameter and TuripTheme usage) and remove the local duplicates so both components use the single implementation.
🧹 Nitpick comments (18)
android/app/src/main/res/drawable/ic_account.xml (1)
8-8: 하드코딩된 색상값 → 색상 리소스로 교체 권장
android:fillColor="#5B5B5B"는 하드코딩된 값으로, 다크 모드 대응이나 테마 변경 시 색상이 고정되어 동적으로 바뀌지 않습니다.colors.xml에 색상 리소스를 정의하거나 테마 속성을 사용하는 것이 좋습니다.🎨 색상 리소스 적용 예시
res/values/colors.xml에 색상을 정의한 뒤:- android:fillColor="#5B5B5B" + android:fillColor="@color/icon_secondary"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/res/drawable/ic_account.xml` at line 8, Replace the hardcoded color in ic_account.xml (android:fillColor="#5B5B5B") with a color resource or theme attribute: add a named color (e.g., colorAccountIcon) to res/values/colors.xml (and res/values-night/colors.xml or use a theme attribute for dark mode), then update the android:fillColor in ic_account.xml to reference that resource (e.g., `@color/colorAccountIcon` or ?attr/yourIconColorAttr) so the icon follows theme/dark-mode changes.android/app/src/main/java/com/on/turip/ui/compose/login/LoginScreen.kt (1)
76-76: 마이그레이션 다이얼로그에 로그아웃 전용 dismiss 문자열 재사용 중
my_page_logout_dialog_dismiss는 My Page의 로그아웃 다이얼로그 전용으로 의미론적으로 귀속된 키입니다. 현재 이 키를 로그인 화면의 마이그레이션 다이얼로그(dismissText)에서 재사용하고 있어, 향후 로그아웃 다이얼로그 dismiss 텍스트가 변경될 경우 마이그레이션 다이얼로그에도 의도치 않게 영향을 줄 수 있습니다.login_dialog_migration_dismiss와 같이 전용 문자열 리소스를 분리하는 것이 더 안전합니다.♻️ 제안: 전용 dismiss 문자열 리소스 분리
strings.xml에 마이그레이션 다이얼로그 전용 키 추가:+ <string name="login_dialog_migration_dismiss">취소</string>
LoginScreen.kt에서 해당 키 참조:- dismissText = stringResource(R.string.my_page_logout_dialog_dismiss), + dismissText = stringResource(R.string.login_dialog_migration_dismiss),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/com/on/turip/ui/compose/login/LoginScreen.kt` at line 76, 해당 마이그레이션 다이얼로그가 로그아웃 전용 리소스(my_page_logout_dialog_dismiss)를 재사용하고 있어 향후 변경에 의도치 않게 영향받을 수 있으니, strings.xml에 새로운 키(login_dialog_migration_dismiss)를 추가하고 LoginScreen 내 dismissText 호출을 현재 stringResource(R.string.my_page_logout_dialog_dismiss)에서 stringResource(R.string.login_dialog_migration_dismiss)로 바꿔 사용하도록 수정하세요; 관련 위치는 LoginScreen.kt의 dismissText 속성 참조를 업데이트하면 됩니다.android/app/src/main/java/com/on/turip/ui/common/paging/PagingLoadMode.kt (1)
3-9:@Immutable어노테이션 불필요Compose 컴파일러는
enum class를 기본적으로 안정(stable)으로 추론합니다.@Immutable은 기술적으로 틀리지 않지만 중복 선언이며, 독자에게 특별한 처리가 필요한 타입인 것처럼 오해를 줄 수 있습니다.-import androidx.compose.runtime.Immutable - -@Immutable enum class PagingLoadMode { REFRESH, APPEND, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/com/on/turip/ui/common/paging/PagingLoadMode.kt` around lines 3 - 9, Remove the redundant `@Immutable` annotation from the PagingLoadMode enum declaration: the Compose compiler already treats enum classes as stable, so delete the import androidx.compose.runtime.Immutable and the `@Immutable` annotation on the PagingLoadMode enum to avoid misleading readers while leaving the enum (REFRESH, APPEND) unchanged.android/app/src/main/res/drawable/btn_chevron_right.xml (1)
7-8: fillColor 하드코딩 — 색상 리소스 참조 권장
#C1C1C1을 직접 하드코딩하면 디자인 시스템 색상 토큰과 동기화가 끊어질 수 있습니다. 프로젝트에 색상 리소스(또는 Compose 토큰)가 이미 정의되어 있다면 해당 값을 참조하는 것이 유지보수에 유리합니다.- android:fillColor="#C1C1C1"/> + android:fillColor="@color/gray_300"/>사용처에서
tint속성이나 Compose의colorFilter로 재정의하는 방식도 동일하게 권장됩니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/res/drawable/btn_chevron_right.xml` around lines 7 - 8, The chevron drawable hardcodes android:fillColor="#C1C1C1" which breaks use of shared color tokens; replace the hardcoded value with a color resource reference (e.g., android:fillColor="@color/chevron_color" or an attribute like "@attr/colorControlNormal") and add/update the corresponding color resource in your colors.xml (or theme attribute). Alternatively ensure consumers override the drawable via android:tint (or Compose colorFilter) so the icon is tinted by the app theme; update btn_chevron_right.xml to use the resource/attribute reference instead of the literal hex.android/app/src/main/java/com/on/turip/ui/bookmarks/BookmarkContentActivity.kt (1)
52-59: Intent Extra 키를 매직 스트링 대신 상수로 추출해야 합니다.
"BOOKMARK_CONTENT_HAS_BOOKMARK_CHANGES_FLAG"문자열이finish()내에서만 사용되고, 이 결과를 읽는 호출 측에서도 동일한 문자열을 하드코딩해야 합니다. 오탈자에 의한 조용한 버그 위험이 있으므로companion object에 상수로 정의하고 결과를 읽는 헬퍼 함수도 함께 제공하는 것이 좋습니다.♻️ 상수 및 헬퍼 함수 추출 제안
companion object { + private const val EXTRA_HAS_BOOKMARK_CHANGES = "BOOKMARK_CONTENT_HAS_BOOKMARK_CHANGES_FLAG" + fun newIntent(context: Context): Intent = Intent(context, BookmarkContentActivity::class.java) + + fun getHasBookmarkChanges(intent: Intent): Boolean = + intent.getBooleanExtra(EXTRA_HAS_BOOKMARK_CHANGES, false) }override fun finish() { val data = Intent().apply { - putExtra("BOOKMARK_CONTENT_HAS_BOOKMARK_CHANGES_FLAG", hasBookmarkChanges) + putExtra(EXTRA_HAS_BOOKMARK_CHANGES, hasBookmarkChanges) } setResult(Activity.RESULT_OK, data) super.finish() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/com/on/turip/ui/bookmarks/BookmarkContentActivity.kt` around lines 52 - 59, Extract the magic string in finish() into a companion object constant (e.g., const val EXTRA_HAS_BOOKMARK_CHANGES) inside BookmarkContentActivity and replace the hardcoded key in finish() with that constant; also add a small public helper in the companion (e.g., fun hasBookmarkChangesFrom(intent: Intent): Boolean or a parseResult(intent: Intent): Boolean) that callers can use to read the boolean from the result Intent so callers no longer hardcode the same string.android/app/src/main/java/com/on/turip/ui/common/paging/PagingState.kt (1)
7-13: 비제한 타입 파라미터T에@Immutable대신@Stable사용을 권장합니다.
@Immutable은 Compose 컴파일러에게 모든 공개 프로퍼티의 값이 **깊이 불변(deeply immutable)**임을 보장하는 강한 계약입니다. 그러나 타입 파라미터T가 제한되지 않아,T가 가변(mutable) 타입으로 인스턴스화될 경우 이 계약이 위반되고 Compose의 재구성(recomposition) 최적화가 잘못 동작할 수 있습니다.@Stable은 보다 완화된 안정성 보장을 제공하면서도 Compose 최적화를 유지하므로 제네릭 UI 상태 홀더에 더 적합합니다.♻️ `@Stable` 사용 제안
-@Immutable +@Stable data class PagingState<T>( val items: ImmutableList<T>, val hasNext: Boolean, val isAppending: Boolean, val errorUiState: ErrorUiState, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/com/on/turip/ui/common/paging/PagingState.kt` around lines 7 - 13, The PagingState<T> data class is annotated with `@Immutable` while T is an unconstrained type parameter; replace `@Immutable` with `@Stable` on the PagingState declaration (i.e., change the annotation on PagingState<T>) so the generic UI state holder uses the relaxed stability contract suitable for unconstrained T; keep the existing properties (items: ImmutableList<T>, hasNext, isAppending, errorUiState) unchanged.android/app/src/main/java/com/on/turip/ui/main/bookmarks/BookmarkContentFragment.kt (1)
11-35: 마이그레이션 중간 단계로서 적절하지만, 이 stub fragment의 제거를 추적하세요.Fragment가 항상 빈 화면만 표시하도록 축소되었고, 실제 기능은 Compose 기반
BookmarkContentActivity/BookmarkContentScreen으로 이관된 것으로 보입니다. 마이그레이션 과도기 코드로 이해되지만,@AndroidEntryPoint가 더 이상 필요 없고 (DI 주입 없음), 이 Fragment 자체가 추후 제거 대상입니다. 별도 이슈나 TODO 코멘트로 정리 시점을 추적해 두시면 좋겠습니다.이 stub fragment 제거를 추적하기 위한 이슈를 생성하시겠습니까?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/com/on/turip/ui/main/bookmarks/BookmarkContentFragment.kt` around lines 11 - 35, BookmarkContentFragment is now a stub showing only an empty view (showEmptyView) and no longer uses DI, so remove the unnecessary `@AndroidEntryPoint` annotation and add a clear TODO comment and/or create a tracking issue referencing BookmarkContentFragment (or the class name) indicating it will be deleted once Compose-based BookmarkContentActivity/BookmarkContentScreen is fully adopted; ensure the companion object instance() can be removed in the same follow-up, or mark it deprecated in the TODO so future cleanup is unambiguous.android/app/src/main/res/values/strings.xml (1)
48-74:my_page_bookmark_content_title(Line 50)과bookmark_content_title(Line 70)이 동일한 값 "저장한 콘텐츠"입니다.두 string resource가 같은 텍스트를 가리킵니다. 의도적으로 서로 다른 화면(MyPage 섹션 헤더 vs. BookmarkContent 전체 목록 AppBar)에서 독립적으로 변경 가능하게 분리한 것이라면 괜찮지만, 항상 동일한 텍스트를 유지할 예정이라면 하나로 통합하여 관리 부담을 줄이는 것을 고려해 주세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/res/values/strings.xml` around lines 48 - 74, These two string resources my_page_bookmark_content_title and bookmark_content_title both contain "저장한 콘텐츠"; decide whether to unify or keep separate: if they should always match, pick one canonical name (e.g., bookmark_content_title), update all usages (MyPage header and Bookmark screen) to reference that single resource, and remove the duplicate entry; if they must remain independently editable, add a clarifying comment above each resource explaining the different contexts (MyPage header vs Bookmark AppBar) to avoid accidental consolidation.android/app/src/main/java/com/on/turip/ui/compose/mypage/MyPageMapper.kt (1)
10-10:imageUrl = null하드코딩에 TODO 주석 추가 권장
Account도메인 모델에 이미지 URL 필드가 없어 현재는null이 맞지만, 향후 프로필 이미지 기능이 추가될 경우 이 매핑도 수정이 필요합니다. 추적을 위해 TODO 주석을 남겨두는 것을 고려해 보세요.💡 제안 예시
fun Account.toUiModel(): ProfileModel = ProfileModel( id = id, nickname = nickname, - imageUrl = null, + imageUrl = null, // TODO: 백엔드에서 프로필 이미지 URL을 제공하면 Account 도메인 및 이 매핑을 함께 업데이트 )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/com/on/turip/ui/compose/mypage/MyPageMapper.kt` at line 10, 현재 MyPageMapper의 매핑에서 imageUrl = null이 하드코딩되어 있는데 향후 Account 도메인에 프로필 이미지가 추가되면 이 부분을 함께 수정해야 하므로 imageUrl = null 할당 옆에 TODO 주석을 추가해 추적 가능하도록 하세요; MyPageMapper(또는 해당 매핑 함수)에서 imageUrl 필드에 TODO: handle Account.profileImage or map when Account gains imageUrl 같은 간단한 주석을 남기고, 향후 Account 모델에 image URL이 생기면 해당 주석을 찾아 매핑 로직(이미지 URL을 Account에서 읽어오는 부분)으로 대체하도록 구현할 것이라는 안내를 포함하세요.android/app/src/main/java/com/on/turip/ui/mypage/MyPageActivity.kt (1)
82-86: 매직 스트링"BOOKMARK_CONTENT_HAS_BOOKMARK_CHANGES_FLAG"를 상수로 추출해주세요.
BookmarkContentActivity.finish()(relevant snippet)와MyPageActivity(Line 84) 양쪽에서 같은 문자열 키를 하드코딩하고 있습니다. 한쪽만 오타가 나면 데이터 전달이 조용히 실패합니다. 공유 상수로 추출하면 컴파일 타임에 일관성을 보장할 수 있습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/com/on/turip/ui/mypage/MyPageActivity.kt` around lines 82 - 86, Extract the hard-coded string "BOOKMARK_CONTENT_HAS_BOOKMARK_CHANGES_FLAG" into a shared constant and use that constant in both MyPageActivity and BookmarkContentActivity.finish(); locate MyPageActivity (the code reading result.data?.getBooleanExtra(...)) and BookmarkContentActivity.finish() (the code putting the boolean extra) and replace the literal with a single constant (e.g., a public const val in BookmarkContentActivity companion object or a shared object/class) so both sides reference the same identifier and avoid silent mismatches.android/app/src/main/java/com/on/turip/ui/compose/mypage/component/BookmarkedContentSection.kt (1)
191-198:CircularProgressIndicator의.align(Alignment.Center)가 불필요합니다.부모
Box에 이미contentAlignment = Alignment.Center가 지정되어 있으므로 Line 196의.align(Alignment.Center)는 중복입니다.♻️ 수정 제안
CircularProgressIndicator( modifier = Modifier - .size(36.dp) - .align(Alignment.Center), + .size(36.dp), color = TuripTheme.colors.gray03, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/com/on/turip/ui/compose/mypage/component/BookmarkedContentSection.kt` around lines 191 - 198, Remove the redundant .align(Alignment.Center) on the CircularProgressIndicator’s Modifier since the parent Box already sets contentAlignment = Alignment.Center; locate the CircularProgressIndicator call (and its Modifier.size(36.dp)) and delete the .align(Alignment.Center) chain so the indicator relies on the Box's contentAlignment instead.android/app/src/main/java/com/on/turip/ui/compose/bookmarks/BookmarkContentUiState.kt (1)
10-31:errorUiState가PagingState내부와BookmarkContentUiState최상위에 중복으로 존재합니다.
PagingState(Line 23-27) 내부의errorUiState와BookmarkContentUiState(Line 13) 최상위의errorUiState가 각각 어떤 오류 상태를 담당하는지 역할이 모호할 수 있습니다. 예를 들어 페이징 에러와 초기 로드 에러를 구분하는 것이라면, 네이밍이나 주석으로 의도를 명확히 해주면 유지보수에 도움이 됩니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/com/on/turip/ui/compose/bookmarks/BookmarkContentUiState.kt` around lines 10 - 31, The top-level errorUiState on BookmarkContentUiState and the errorUiState inside PagingState overlap and are ambiguous; decide whether they represent separate concerns (e.g., initial/load error vs. paging error) or a single error and make that explicit by renaming and documenting: for example rename BookmarkContentUiState.errorUiState to loadErrorUiState (or PagingState.errorUiState to pageErrorUiState) and update the isEmpty getter and the Idle factory to reference the new names, or collapse to a single error field by removing one of them and adjusting all usages; ensure comments are added to BookmarkContentUiState and PagingState indicating each field's purpose.android/app/src/main/java/com/on/turip/ui/compose/mypage/MyPageUiState.kt (1)
9-12:BookmarkContent에@Immutable어노테이션 추가를 권장합니다.
ProfileModel은 UI 계층을 위해 만든 전용 모델로@Immutable이 표시되어 있지만,bookmarkContentState에 사용된BookmarkContent는 도메인 계층의 모델로 안정성 어노테이션이 없습니다.MyPageUiState에@Immutable이 표시되어 있더라도, 내부의BookmarkContent가 Compose 컴파일러에 의해 불안정한 타입으로 판단되어 불필요한 리컴포지션이 발생할 수 있습니다.ImmutableList로 감싼 것만으로는 내부 요소의 안정성을 보장하지 못합니다.
BookmarkContent에@Immutable어노테이션을 추가하거나,ProfileModel처럼 UI 계층용 모델 래퍼를 만드는 것을 권장합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/com/on/turip/ui/compose/mypage/MyPageUiState.kt` around lines 9 - 12, MyPageUiState is annotated `@Immutable` but its bookmarkContentState holds domain model BookmarkContent which lacks immutability annotation; add `@Immutable` to the BookmarkContent data class (or create a UI-specific wrapper similar to ProfileModel) so Compose sees the elements as stable—update the BookmarkContent class (or introduce a BookmarkContentUiModel and convert where MyPageUiState.bookmarkContentState is populated) to ensure stable types for recomposition.android/app/src/main/java/com/on/turip/ui/compose/mypage/component/MyPageSettingsSection.kt (1)
47-59: Composable 내부에서 전역 상태AuthState.type을 직접 읽고 있습니다.
AuthState.type은 Compose State가 아닌 전역/정적 값이므로, 값이 변경되어도 recomposition이 트리거되지 않습니다. 또한 Preview와 테스트에서 상태를 제어할 수 없게 됩니다.UserType을 파라미터로 받도록 수정하면 state hoisting 원칙에 부합하고 테스트/프리뷰가 용이해집니다.추가로,
UserType.NONE분기의Timber.e()는 recomposition마다 호출되어 로그 스팸이 발생할 수 있습니다.LaunchedEffect로 감싸거나, ViewModel 레이어에서 처리하는 것이 적절합니다.♻️ 파라미터로 UserType을 전달하도록 리팩터링 제안
`@Composable` fun MyPageSettingsSection( + userType: UserType, onInquiryClick: () -> Unit, onPrivacyPolicyClick: () -> Unit, onLoginClick: () -> Unit, onLogoutClick: () -> Unit, onWithdrawClick: () -> Unit, modifier: Modifier = Modifier, ) { // ... - when (AuthState.type) { + when (userType) { UserType.MEMBER -> { SettingForMember(onLogoutClick, onWithdrawClick) } UserType.GUEST -> { SettingForGuest(onLoginClick) } UserType.NONE -> { - Timber.e("멤버가 지정되지 않았습니다.") SettingForGuest(onLoginClick) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/com/on/turip/ui/compose/mypage/component/MyPageSettingsSection.kt` around lines 47 - 59, The composable is directly reading the global AuthState.type which breaks state hoisting and prevents recomposition and testability; change the MyPageSettingsSection composable to accept a UserType parameter (e.g., userType: UserType) and use that parameter in the when branch to choose between SettingForMember(onLogoutClick, onWithdrawClick) and SettingForGuest(onLoginClick) instead of referencing AuthState.type, and remove direct global reads; also move the Timber.e("멤버가 지정되지 않았습니다.") call out of the composable (handle it in ViewModel or wrap it in a LaunchedEffect keyed to userType if it must run from the UI) so logging does not execute on every recomposition and tests/previews can pass a controlled UserType.android/app/src/main/java/com/on/turip/ui/compose/bookmarks/BookmarkContentViewModel.kt (2)
214-244: 낙관적 삭제 실패 시 아이템이 UI에 복원되지 않습니다.
removeBookmark에서 API 호출 실패 시ShowBookmarkRemoveFailed이펙트만 전송하고, 사전에 제거된 아이템을 리스트에 복원하지 않습니다. 사용자가 스낵바의 액션 버튼을 통해refreshBookmarkContents를 호출해야만 원래 상태로 돌아갑니다.낙관적 UI의 핵심은 실패 시 롤백(rollback)입니다. 삭제 전 아이템을 캡처하여 실패 시 복원하면 UX가 개선됩니다.
♻️ 롤백 패턴 제안
try { + // 삭제 전 상태 스냅샷 + var removedItem: BookmarkContent? = null val removed = removeBookmarkMutex.withLock { val contents = _uiState.value.bookmarkContents if (contents.items.none { it.content.id == contentId }) return@withLock false + removedItem = contents.items.first { it.content.id == contentId } val updated = contents.items.filter { it.content.id != contentId }.toImmutableList() _uiState.update { state -> state.copy(bookmarkContents = state.bookmarkContents.copy(items = updated)) } true } if (!removed) return@launch bookmarkRepository .deleteBookmark(contentId) .onSuccess { _uiEffect.send(BookmarkContentUiEffect.BookmarkRemoved) }.onFailure { + // 실패 시 아이템 복원 + removedItem?.let { item -> + _uiState.update { state -> + val restored = (state.bookmarkContents.items + item) + .sortedByDescending { it.content.id } + .toImmutableList() + state.copy(bookmarkContents = state.bookmarkContents.copy(items = restored)) + } + } _uiEffect.send(BookmarkContentUiEffect.ShowBookmarkRemoveFailed) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/com/on/turip/ui/compose/bookmarks/BookmarkContentViewModel.kt` around lines 214 - 244, When performing the optimistic removal in removeBookmark, capture the removed item(s) before updating _uiState (inside removeBookmarkMutex.withLock) so you can rollback on API failure; call bookmarkRepository.deleteBookmark(contentId) as you do, but in its onFailure handler re-insert the previously captured item into _uiState (also under removeBookmarkMutex.withLock) and then send BookmarkContentUiEffect.ShowBookmarkRemoveFailed; keep the success path sending BookmarkContentUiEffect.BookmarkRemoved and ensure removingIds removal in the finally block remains unchanged.
54-200: 에러 처리 로직이 REFRESH/APPEND × 에러타입별로 중복이 많습니다.현재 REFRESH와 APPEND 각각에서 Network/Server/TokenExpired를 개별 처리하고 있어 코드가 장황합니다. 에러 타입 매핑을 헬퍼 함수로 추출하면 가독성과 유지보수성이 개선됩니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/com/on/turip/ui/compose/bookmarks/BookmarkContentViewModel.kt` around lines 54 - 200, The loadBookmarkContents function duplicates error handling for REFRESH and APPEND across Network/Server/TokenExpired; extract a helper (e.g., handleBookmarkLoadError(errorType: ErrorType, loadMode: PagingLoadMode)) that converts errorType.toUiError(), then switches on UiError.Global and loadMode to perform the correct _uiState.update and optionally _uiEffect.send(BookmarkContentUiEffect.NavigateToLogin); replace the duplicated when(loadMode) blocks in the onFailure lambda with a single call to this helper so all mapping logic (Network -> ErrorUiState.Network, Server -> ErrorUiState.Server, TokenExpired -> navigate to login and stop loading/appending) is centralized and reused.android/app/src/main/java/com/on/turip/ui/compose/mypage/MyPageViewModel.kt (2)
213-233:UiError.Global이외의 에러 타입은 무시됩니다.
handleError에서UiError.Global이 아닌 경우 아무 처리 없이 반환됩니다. 현재는Global서브타입만 존재할 수 있지만, 향후 다른 에러 타입이 추가될 경우 조용히 무시될 수 있습니다. 방어적으로 로깅이나 기본 처리를 추가하는 것을 고려해 주세요.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/com/on/turip/ui/compose/mypage/MyPageViewModel.kt` around lines 213 - 233, handleError 함수가 UiError.Global 이외의 경우를 무시하므로 미래에 다른 UiError 서브타입이 추가되면 조용히 빠지는 문제가 있습니다; handleError에서 ErrorType -> UiError 변환 후 UiError가 UiError.Global이 아닐 때 기본 처리(예: _uiEffect를 통한 MyPageUiEffect.ShowError(ErrorUiState.Unknown, retryAction) 전송 또는 processLogger로 경고 로깅)를 추가해 방어적으로 처리하고, 현재 분기에서 처리된 케이스들(UiError.Global.Network / Server / TokenExpired)과 함께 기본 케이스를 명시적으로 처리하도록 수정하세요 (참조 심볼: handleError, ErrorType, UiError, UiError.Global, _uiEffect, MyPageUiEffect).
72-77: 매직 넘버10과0L을 상수로 추출하는 것을 권장합니다.
BookmarkContentViewModel에서는PAGE_SIZE를companion object에 상수로 정의하고 있는 반면, 여기서는loadBookmarks(10, 0L)로 하드코딩되어 있습니다. 가독성과 일관성을 위해 상수 추출을 권장합니다.♻️ 상수 추출 제안
companion object { private const val INVALID_FID = "FID_LOAD_FAIL" + private const val BOOKMARK_PREVIEW_SIZE = 10 + private const val INITIAL_OFFSET = 0L }- bookmarkRepository - .loadBookmarks(10, 0L) + bookmarkRepository + .loadBookmarks(BOOKMARK_PREVIEW_SIZE, INITIAL_OFFSET)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/com/on/turip/ui/compose/mypage/MyPageViewModel.kt` around lines 72 - 77, The call to bookmarkRepository.loadBookmarks(10, 0L) in loadBookmarkContents uses hardcoded magic numbers; extract these into named constants (e.g., PAGE_SIZE and INITIAL_OFFSET) in MyPageViewModel's companion object (or reuse the existing PAGE_SIZE from BookmarkContentViewModel if appropriate) and replace the literals with those constants so the call becomes loadBookmarks(PAGE_SIZE, INITIAL_OFFSET), improving readability and consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@android/app/src/main/java/com/on/turip/data/accounts/dto/MyProfileResponse.kt`:
- Around line 6-14: MyProfileResponse's non-nullable fields nickname and role
lack safe defaults, causing kotlinx.serialization to throw when those keys are
missing or null; update the data class (MyProfileResponse) to provide safe
defaults (e.g., nickname = "" and role = "GUEST") or make them nullable with
sensible fallbacks so deserialization won't crash, and ensure callers that use
Role.from(role) still work with the chosen default.
In `@android/app/src/main/java/com/on/turip/domain/accounts/Role.kt`:
- Line 13: The current Role lookup quietly falls back to GUEST
(entries.firstOrNull { it.tag == tag } ?: GUEST), which hides unknown or
mistyped server role values; modify the Role lookup function in Role.kt (the
code that uses entries.firstOrNull and the GUEST fallback) to log a warning when
no matching entry is found before returning GUEST—include the unmatched tag
value in the log message and ensure the logger used is appropriate for the
module (e.g., Android Log.w or your app logger) so unexpected roles are visible
during debugging.
In
`@android/app/src/main/java/com/on/turip/ui/bookmarks/BookmarkContentActivity.kt`:
- Line 18: hasBookmarkChanges is not persisted across process death; implement
state save/restore by adding a boolean entry (e.g. KEY_HAS_BOOKMARK_CHANGES) to
the savedInstanceState Bundle in onSaveInstanceState(Bundle) and restore it in
onCreate(Bundle?) or onRestoreInstanceState(Bundle) so the field
hasBookmarkChanges is rehydrated when the Activity is recreated, ensuring
changes are still delivered to the caller (update any logic that reads
hasBookmarkChanges such as result handling before finish()).
In
`@android/app/src/main/java/com/on/turip/ui/compose/bookmarks/BookmarkContentScreen.kt`:
- Around line 380-382: Rename the preview function BookmarkContentErrorSuccess
to BookmarkContentSuccessPreview to match the `@Preview` name "정상" and its intent;
update the function declaration and any internal or external references (usages,
imports, tests) that refer to BookmarkContentErrorSuccess so they point to
BookmarkContentSuccessPreview, keeping the `@Preview` and `@Composable` annotations
unchanged.
In
`@android/app/src/main/java/com/on/turip/ui/compose/bookmarks/BookmarkContentViewModel.kt`:
- Around line 89-100: The TOCTOU bug occurs in BookmarkContentViewModel when
computing lastItemId in PagingLoadMode.APPEND by re-reading uiState.value before
calling .last(), which can throw NoSuchElementException if removeBookmark()
emptied the list; fix by taking a single snapshot like val items =
uiState.value.bookmarkContents.items (use that for the empty check and for
retrieving the last element), or use items.lastOrNull() with a safe fallback
(e.g., 0L) so PagingLoadMode.APPEND never calls .last() on an empty collection.
- Around line 124-198: The ViewModel's error handling only handles
UiError.Global and ignores UiError.Feature, leaving loading flags stuck; update
the onFailure branches in BookmarkContentViewModel (inside the
PagingLoadMode.REFRESH and PagingLoadMode.APPEND cases) to add an
else-for-non-Global uiError path: after computing val uiError =
errorType.toUiError(), if uiError is not UiError.Global then call
_uiState.update(...) to set isLoading = false for REFRESH and to set
bookmarkContents = bookmarkContents.copy(isAppending = false) for APPEND (you
may also set a generic ErrorUiState if desired), and keep the existing
TokenExpired navigation logic unchanged.
In
`@android/app/src/main/java/com/on/turip/ui/compose/bookmarks/component/BookmarkContentItem.kt`:
- Around line 99-265: The three private composables ContentThumbnail,
ContentInformation, and ContentInfoItem are duplicated in this file and in
mypage/component/BookmarkedContentItem.kt; extract them into a single shared
composable file (make them non-private/public) so both places import and reuse
the same implementations, keep their signatures and dependencies
(TuripUrlConverter, AsyncImage, stringResource usage, TuripTheme styles/colors,
and parameter types like BookmarkContent and onRemoveBookmark), remove the
duplicate definitions from both files, and update the two callers to import the
shared composables (ensuring resource and painter references remain unchanged).
In `@android/app/src/main/java/com/on/turip/ui/compose/home/HomeScreen.kt`:
- Around line 174-180: The Text debug entry "임시 마이페이지 확인용 텍스트" in
HomeScreen/HomeScreenContent should be removed or behind a debug-only guard:
either delete the Text and remove the navigateToMyPage / onNavigateToMyPage
parameters (as suggested) or wrap it in a build-config/debug flag check (e.g.,
if (BuildConfig.DEBUG) { ... }) so it never appears in release builds; if you
keep a temporary clickable entry, replace the hardcoded Korean string with
stringResource(...) for i18n and add accessibility semantics to the clickable
(provide role = Role.Button and an onClickLabel via Modifier.semantics or
clickable semantics) so it isn’t exposed to users unintentionally and remains
localizable and accessible.
In
`@android/app/src/main/java/com/on/turip/ui/compose/mypage/component/MyPageAppBar.kt`:
- Around line 11-21: Replace the hardcoded "마이페이지" in MyPageAppBar with
stringResource(R.string.my_page_title) and stop using TextAlign.Center inside
the start slot because it has no effect there; either move the title Text into
TuripAppBar's center slot to truly center the app bar title (e.g., put the Text
composable in the center lambda) or keep it in the start slot and remove
TextAlign.Center so it uses default left alignment; update the MyPageAppBar
function accordingly and ensure string resource exists.
In
`@android/app/src/main/java/com/on/turip/ui/compose/mypage/component/ProfileSection.kt`:
- Around line 128-133: The CircularProgressIndicator in the
MyPageSectionState.Loading branch is using the passed-in modifier and size(2.dp)
which makes it nearly invisible and reuses the parent modifier (which may
include .weight(1f)); update the Loading branch to use Modifier.size(24.dp)
(i.e., use the local Modifier instead of the function parameter modifier) and
set the size to 24.dp so the spinner is visible while avoiding reapplying the
external modifier.
- Around line 139-148: The Text in the MyPageSectionState.Success branch is
receiving the external modifier (modifier) that was already applied to the
parent Box, causing duplicate modifiers; change the Text composable to use a
fresh local Modifier (e.g. Modifier or Modifier.fillMaxWidth() as appropriate)
instead of the passed-in "modifier" so only the parent keeps the external
modifier—update the branch in ProfileSection.kt where MyPageSectionState.Success
is handled to remove "modifier = modifier" and replace with "modifier =
Modifier" (or another local Modifier) on the Text.
In `@android/app/src/main/java/com/on/turip/ui/compose/mypage/MyPageScreen.kt`:
- Around line 169-181: The Preview fails because MyPageScreenPreview relies on
the default hiltViewModel() parameter in MyPageScreen; replace the Preview usage
with a ViewModel-free composable by extracting the UI into a state-based
internal composable (e.g., MyPageContent) that accepts a MyPageUiState and
callbacks, then update MyPageScreen to call MyPageContent when given a real
ViewModel and update MyPageScreenPreview to call MyPageContent with a sample
MyPageUiState (e.g., Idle) and no-op callbacks so the Preview doesn't invoke
hiltViewModel().
In `@android/app/src/main/java/com/on/turip/ui/compose/mypage/MyPageViewModel.kt`:
- Around line 169-211: The code ignores failures from
userStorageRepository.clearTokens() and only logs them; update confirmLogout and
confirmWithdraw so that clearTokens() failure still results in consistent UI
state: either always send _uiEffect.send(MyPageUiEffect.NavigateToLogin) after
memberRepository.logout()/deleteMember() succeeds (regardless of clearTokens
outcome) or on clearTokens().onFailure send an error UI effect (e.g.,
_uiEffect.send(MyPageUiEffect.ShowError(...))) and then navigate; ensure you
reference confirmLogout, confirmWithdraw, userStorageRepository.clearTokens,
_uiEffect, and MyPageUiEffect.NavigateToLogin (or a ShowError effect) to keep
server/local state consistent and give user feedback when token clearing fails.
- Around line 54-88: Both loadProfile and loadBookmarkContents currently treat
all failures the same; update their onFailure handlers to check for the
TokenExpired error and trigger the same login navigation/flow used in
BookmarkContentViewModel instead of only setting MyPageSectionState.Error.
Specifically, inside loadProfile and loadBookmarkContents replace the blanket
onFailure block with logic that inspects the throwable (e.g., when(it is
TokenExpired or maps to UiError.Global.TokenExpired)) and calls the existing
helper/navigation used by BookmarkContentViewModel to route to the login screen,
while keeping the existing MyPageSectionState.Error fallback for other errors.
- Around line 96-131: In removeBookmark, preserve the current bookmark list
before performing the optimistic UI removal and, if
bookmarkRepository.deleteBookmark(contentId) fails, restore _uiState to that
saved list and still emit MyPageUiEffect.ShowBookmarkRemoveFailed; specifically,
inside removeBookmark capture the previous contents (from
(_uiState.value.bookmarkContentState as? MyPageSectionState.Success)?.data) into
a local variable before updating _uiState, then in the onFailure block update
_uiState back to MyPageSectionState.Success(previousContents) (while respecting
the existing removeBookmarkMutex/removingIds cleanup in the finally block) so
the UI is rolled back on API failure.
In `@android/app/src/main/java/com/on/turip/ui/main/favorite/MyPageFragment.kt`:
- Around line 61-65: The click handler in MyPageFragment.setupListeners
currently leaves ivMyPageMoreOptions inert (Timber.d only); replace the dummy
log with functional behavior: either start the intended MyPageActivity (create
an Intent and call startActivity) or, until navigation exists, disable or hide
ivMyPageMoreOptions (set isEnabled = false or isVisible = false) so users get no
inactive control; update any references to SettingActivity to point to
MyPageActivity if that is the new target and ensure the code uses the fragment's
context/startActivity correctly.
In `@android/app/src/main/java/com/on/turip/ui/mypage/MyPageActivity.kt`:
- Around line 47-54: The navigateToInquiry lambda currently calls
startActivity(intent) directly, risking ActivityNotFoundException if no email
app exists; change it to use the existing safeStartActivityWithToast helper (the
same approach used by navigateToPrivacyPolicy) when launching the Intent
(Intent.ACTION_SENDTO) built from InquiryMail (RECIPIENT, TITLE, mail.content)
so the app shows a toast and avoids a crash when no activity can handle the
intent.
---
Outside diff comments:
In
`@android/app/src/main/java/com/on/turip/ui/compose/bookmarks/component/BookmarkContentAppBar.kt`:
- Around line 23-28: The back arrow Icon in BookmarkContentAppBar uses
Modifier.clickable directly so its touch target is just the 24dp icon; wrap the
Icon with an IconButton (or replace the clickable modifier with a container
using minimumInteractiveComponentSize(48.dp)) to guarantee the 48dp minimum
touch target per Material guidelines; update the start slot where Icon(...) is
defined to use IconButton(onClick = onBackClick) { Icon(...) } (or apply
Modifier.sizeIn/minimumInteractiveComponentSize to the parent) so the touch area
for the back action is accessible.
---
Duplicate comments:
In
`@android/app/src/main/java/com/on/turip/ui/compose/mypage/component/BookmarkedContentItem.kt`:
- Around line 84-201: This code duplicates three composables (ContentThumbnail,
ContentInformation, ContentInfoItem) already present in
bookmarks/component/BookmarkContentItem.kt; extract these shared composables
into a single common file (e.g., a ui/compose/common or shared composables file)
and update both BookmarkContentItem.kt and this file to import and call the
shared versions; ensure the shared file exposes the same function signatures
(including BookmarkContent parameter and TuripTheme usage) and remove the local
duplicates so both components use the single implementation.
---
Nitpick comments:
In
`@android/app/src/main/java/com/on/turip/ui/bookmarks/BookmarkContentActivity.kt`:
- Around line 52-59: Extract the magic string in finish() into a companion
object constant (e.g., const val EXTRA_HAS_BOOKMARK_CHANGES) inside
BookmarkContentActivity and replace the hardcoded key in finish() with that
constant; also add a small public helper in the companion (e.g., fun
hasBookmarkChangesFrom(intent: Intent): Boolean or a parseResult(intent:
Intent): Boolean) that callers can use to read the boolean from the result
Intent so callers no longer hardcode the same string.
In `@android/app/src/main/java/com/on/turip/ui/common/paging/PagingLoadMode.kt`:
- Around line 3-9: Remove the redundant `@Immutable` annotation from the
PagingLoadMode enum declaration: the Compose compiler already treats enum
classes as stable, so delete the import androidx.compose.runtime.Immutable and
the `@Immutable` annotation on the PagingLoadMode enum to avoid misleading readers
while leaving the enum (REFRESH, APPEND) unchanged.
In `@android/app/src/main/java/com/on/turip/ui/common/paging/PagingState.kt`:
- Around line 7-13: The PagingState<T> data class is annotated with `@Immutable`
while T is an unconstrained type parameter; replace `@Immutable` with `@Stable` on
the PagingState declaration (i.e., change the annotation on PagingState<T>) so
the generic UI state holder uses the relaxed stability contract suitable for
unconstrained T; keep the existing properties (items: ImmutableList<T>, hasNext,
isAppending, errorUiState) unchanged.
In
`@android/app/src/main/java/com/on/turip/ui/compose/bookmarks/BookmarkContentUiState.kt`:
- Around line 10-31: The top-level errorUiState on BookmarkContentUiState and
the errorUiState inside PagingState overlap and are ambiguous; decide whether
they represent separate concerns (e.g., initial/load error vs. paging error) or
a single error and make that explicit by renaming and documenting: for example
rename BookmarkContentUiState.errorUiState to loadErrorUiState (or
PagingState.errorUiState to pageErrorUiState) and update the isEmpty getter and
the Idle factory to reference the new names, or collapse to a single error field
by removing one of them and adjusting all usages; ensure comments are added to
BookmarkContentUiState and PagingState indicating each field's purpose.
In
`@android/app/src/main/java/com/on/turip/ui/compose/bookmarks/BookmarkContentViewModel.kt`:
- Around line 214-244: When performing the optimistic removal in removeBookmark,
capture the removed item(s) before updating _uiState (inside
removeBookmarkMutex.withLock) so you can rollback on API failure; call
bookmarkRepository.deleteBookmark(contentId) as you do, but in its onFailure
handler re-insert the previously captured item into _uiState (also under
removeBookmarkMutex.withLock) and then send
BookmarkContentUiEffect.ShowBookmarkRemoveFailed; keep the success path sending
BookmarkContentUiEffect.BookmarkRemoved and ensure removingIds removal in the
finally block remains unchanged.
- Around line 54-200: The loadBookmarkContents function duplicates error
handling for REFRESH and APPEND across Network/Server/TokenExpired; extract a
helper (e.g., handleBookmarkLoadError(errorType: ErrorType, loadMode:
PagingLoadMode)) that converts errorType.toUiError(), then switches on
UiError.Global and loadMode to perform the correct _uiState.update and
optionally _uiEffect.send(BookmarkContentUiEffect.NavigateToLogin); replace the
duplicated when(loadMode) blocks in the onFailure lambda with a single call to
this helper so all mapping logic (Network -> ErrorUiState.Network, Server ->
ErrorUiState.Server, TokenExpired -> navigate to login and stop
loading/appending) is centralized and reused.
In `@android/app/src/main/java/com/on/turip/ui/compose/login/LoginScreen.kt`:
- Line 76: 해당 마이그레이션 다이얼로그가 로그아웃 전용 리소스(my_page_logout_dialog_dismiss)를 재사용하고 있어
향후 변경에 의도치 않게 영향받을 수 있으니, strings.xml에 새로운 키(login_dialog_migration_dismiss)를
추가하고 LoginScreen 내 dismissText 호출을 현재
stringResource(R.string.my_page_logout_dialog_dismiss)에서
stringResource(R.string.login_dialog_migration_dismiss)로 바꿔 사용하도록 수정하세요; 관련 위치는
LoginScreen.kt의 dismissText 속성 참조를 업데이트하면 됩니다.
In
`@android/app/src/main/java/com/on/turip/ui/compose/mypage/component/BookmarkedContentSection.kt`:
- Around line 191-198: Remove the redundant .align(Alignment.Center) on the
CircularProgressIndicator’s Modifier since the parent Box already sets
contentAlignment = Alignment.Center; locate the CircularProgressIndicator call
(and its Modifier.size(36.dp)) and delete the .align(Alignment.Center) chain so
the indicator relies on the Box's contentAlignment instead.
In
`@android/app/src/main/java/com/on/turip/ui/compose/mypage/component/MyPageSettingsSection.kt`:
- Around line 47-59: The composable is directly reading the global
AuthState.type which breaks state hoisting and prevents recomposition and
testability; change the MyPageSettingsSection composable to accept a UserType
parameter (e.g., userType: UserType) and use that parameter in the when branch
to choose between SettingForMember(onLogoutClick, onWithdrawClick) and
SettingForGuest(onLoginClick) instead of referencing AuthState.type, and remove
direct global reads; also move the Timber.e("멤버가 지정되지 않았습니다.") call out of the
composable (handle it in ViewModel or wrap it in a LaunchedEffect keyed to
userType if it must run from the UI) so logging does not execute on every
recomposition and tests/previews can pass a controlled UserType.
In `@android/app/src/main/java/com/on/turip/ui/compose/mypage/MyPageMapper.kt`:
- Line 10: 현재 MyPageMapper의 매핑에서 imageUrl = null이 하드코딩되어 있는데 향후 Account 도메인에 프로필
이미지가 추가되면 이 부분을 함께 수정해야 하므로 imageUrl = null 할당 옆에 TODO 주석을 추가해 추적 가능하도록 하세요;
MyPageMapper(또는 해당 매핑 함수)에서 imageUrl 필드에 TODO: handle Account.profileImage or
map when Account gains imageUrl 같은 간단한 주석을 남기고, 향후 Account 모델에 image URL이 생기면 해당
주석을 찾아 매핑 로직(이미지 URL을 Account에서 읽어오는 부분)으로 대체하도록 구현할 것이라는 안내를 포함하세요.
In `@android/app/src/main/java/com/on/turip/ui/compose/mypage/MyPageUiState.kt`:
- Around line 9-12: MyPageUiState is annotated `@Immutable` but its
bookmarkContentState holds domain model BookmarkContent which lacks immutability
annotation; add `@Immutable` to the BookmarkContent data class (or create a
UI-specific wrapper similar to ProfileModel) so Compose sees the elements as
stable—update the BookmarkContent class (or introduce a BookmarkContentUiModel
and convert where MyPageUiState.bookmarkContentState is populated) to ensure
stable types for recomposition.
In `@android/app/src/main/java/com/on/turip/ui/compose/mypage/MyPageViewModel.kt`:
- Around line 213-233: handleError 함수가 UiError.Global 이외의 경우를 무시하므로 미래에 다른
UiError 서브타입이 추가되면 조용히 빠지는 문제가 있습니다; handleError에서 ErrorType -> UiError 변환 후
UiError가 UiError.Global이 아닐 때 기본 처리(예: _uiEffect를 통한
MyPageUiEffect.ShowError(ErrorUiState.Unknown, retryAction) 전송 또는 processLogger로
경고 로깅)를 추가해 방어적으로 처리하고, 현재 분기에서 처리된 케이스들(UiError.Global.Network / Server /
TokenExpired)과 함께 기본 케이스를 명시적으로 처리하도록 수정하세요 (참조 심볼: handleError, ErrorType,
UiError, UiError.Global, _uiEffect, MyPageUiEffect).
- Around line 72-77: The call to bookmarkRepository.loadBookmarks(10, 0L) in
loadBookmarkContents uses hardcoded magic numbers; extract these into named
constants (e.g., PAGE_SIZE and INITIAL_OFFSET) in MyPageViewModel's companion
object (or reuse the existing PAGE_SIZE from BookmarkContentViewModel if
appropriate) and replace the literals with those constants so the call becomes
loadBookmarks(PAGE_SIZE, INITIAL_OFFSET), improving readability and consistency.
In
`@android/app/src/main/java/com/on/turip/ui/main/bookmarks/BookmarkContentFragment.kt`:
- Around line 11-35: BookmarkContentFragment is now a stub showing only an empty
view (showEmptyView) and no longer uses DI, so remove the unnecessary
`@AndroidEntryPoint` annotation and add a clear TODO comment and/or create a
tracking issue referencing BookmarkContentFragment (or the class name)
indicating it will be deleted once Compose-based
BookmarkContentActivity/BookmarkContentScreen is fully adopted; ensure the
companion object instance() can be removed in the same follow-up, or mark it
deprecated in the TODO so future cleanup is unambiguous.
In `@android/app/src/main/java/com/on/turip/ui/mypage/MyPageActivity.kt`:
- Around line 82-86: Extract the hard-coded string
"BOOKMARK_CONTENT_HAS_BOOKMARK_CHANGES_FLAG" into a shared constant and use that
constant in both MyPageActivity and BookmarkContentActivity.finish(); locate
MyPageActivity (the code reading result.data?.getBooleanExtra(...)) and
BookmarkContentActivity.finish() (the code putting the boolean extra) and
replace the literal with a single constant (e.g., a public const val in
BookmarkContentActivity companion object or a shared object/class) so both sides
reference the same identifier and avoid silent mismatches.
In `@android/app/src/main/res/drawable/btn_chevron_right.xml`:
- Around line 7-8: The chevron drawable hardcodes android:fillColor="#C1C1C1"
which breaks use of shared color tokens; replace the hardcoded value with a
color resource reference (e.g., android:fillColor="@color/chevron_color" or an
attribute like "@attr/colorControlNormal") and add/update the corresponding
color resource in your colors.xml (or theme attribute). Alternatively ensure
consumers override the drawable via android:tint (or Compose colorFilter) so the
icon is tinted by the app theme; update btn_chevron_right.xml to use the
resource/attribute reference instead of the literal hex.
In `@android/app/src/main/res/drawable/ic_account.xml`:
- Line 8: Replace the hardcoded color in ic_account.xml
(android:fillColor="#5B5B5B") with a color resource or theme attribute: add a
named color (e.g., colorAccountIcon) to res/values/colors.xml (and
res/values-night/colors.xml or use a theme attribute for dark mode), then update
the android:fillColor in ic_account.xml to reference that resource (e.g.,
`@color/colorAccountIcon` or ?attr/yourIconColorAttr) so the icon follows
theme/dark-mode changes.
In `@android/app/src/main/res/values/strings.xml`:
- Around line 48-74: These two string resources my_page_bookmark_content_title
and bookmark_content_title both contain "저장한 콘텐츠"; decide whether to unify or
keep separate: if they should always match, pick one canonical name (e.g.,
bookmark_content_title), update all usages (MyPage header and Bookmark screen)
to reference that single resource, and remove the duplicate entry; if they must
remain independently editable, add a clarifying comment above each resource
explaining the different contexts (MyPage header vs Bookmark AppBar) to avoid
accidental consolidation.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (63)
android/app/src/main/AndroidManifest.xmlandroid/app/src/main/java/com/on/turip/data/accounts/AccountMapper.ktandroid/app/src/main/java/com/on/turip/data/accounts/datasource/AccountRemoteDataSource.ktandroid/app/src/main/java/com/on/turip/data/accounts/datasource/DefaultAccountRemoteDataSource.ktandroid/app/src/main/java/com/on/turip/data/accounts/dto/MyProfileResponse.ktandroid/app/src/main/java/com/on/turip/data/accounts/repository/DefaultAccountRepository.ktandroid/app/src/main/java/com/on/turip/data/accounts/service/AccountService.ktandroid/app/src/main/java/com/on/turip/data/bookmarks/BookmarkMapper.ktandroid/app/src/main/java/com/on/turip/data/bookmarks/repository/DefaultBookmarkRepository.ktandroid/app/src/main/java/com/on/turip/di/DataSourceModule.ktandroid/app/src/main/java/com/on/turip/di/RepositoryModule.ktandroid/app/src/main/java/com/on/turip/di/ServiceModule.ktandroid/app/src/main/java/com/on/turip/domain/accounts/Account.ktandroid/app/src/main/java/com/on/turip/domain/accounts/AccountRepository.ktandroid/app/src/main/java/com/on/turip/domain/accounts/Role.ktandroid/app/src/main/java/com/on/turip/domain/bookmark/PagedBookmarkContents.ktandroid/app/src/main/java/com/on/turip/domain/bookmark/repository/BookmarkRepository.ktandroid/app/src/main/java/com/on/turip/domain/common/paging/Page.ktandroid/app/src/main/java/com/on/turip/ui/bookmarks/BookmarkContentActivity.ktandroid/app/src/main/java/com/on/turip/ui/common/paging/PagingLoadMode.ktandroid/app/src/main/java/com/on/turip/ui/common/paging/PagingState.ktandroid/app/src/main/java/com/on/turip/ui/compose/bookmarks/BookmarkContentScreen.ktandroid/app/src/main/java/com/on/turip/ui/compose/bookmarks/BookmarkContentUiEffect.ktandroid/app/src/main/java/com/on/turip/ui/compose/bookmarks/BookmarkContentUiState.ktandroid/app/src/main/java/com/on/turip/ui/compose/bookmarks/BookmarkContentViewModel.ktandroid/app/src/main/java/com/on/turip/ui/compose/bookmarks/component/BookmarkContentAppBar.ktandroid/app/src/main/java/com/on/turip/ui/compose/bookmarks/component/BookmarkContentItem.ktandroid/app/src/main/java/com/on/turip/ui/compose/home/HomeScreen.ktandroid/app/src/main/java/com/on/turip/ui/compose/login/LoginScreen.ktandroid/app/src/main/java/com/on/turip/ui/compose/mypage/MyPageDialogState.ktandroid/app/src/main/java/com/on/turip/ui/compose/mypage/MyPageMapper.ktandroid/app/src/main/java/com/on/turip/ui/compose/mypage/MyPageScreen.ktandroid/app/src/main/java/com/on/turip/ui/compose/mypage/MyPageSectionState.ktandroid/app/src/main/java/com/on/turip/ui/compose/mypage/MyPageUiEffect.ktandroid/app/src/main/java/com/on/turip/ui/compose/mypage/MyPageUiState.ktandroid/app/src/main/java/com/on/turip/ui/compose/mypage/MyPageViewModel.ktandroid/app/src/main/java/com/on/turip/ui/compose/mypage/component/BookmarkedContentItem.ktandroid/app/src/main/java/com/on/turip/ui/compose/mypage/component/BookmarkedContentSection.ktandroid/app/src/main/java/com/on/turip/ui/compose/mypage/component/MyPageAppBar.ktandroid/app/src/main/java/com/on/turip/ui/compose/mypage/component/MyPageSettingItem.ktandroid/app/src/main/java/com/on/turip/ui/compose/mypage/component/MyPageSettingsSection.ktandroid/app/src/main/java/com/on/turip/ui/compose/mypage/component/ProfileSection.ktandroid/app/src/main/java/com/on/turip/ui/compose/mypage/model/AppEnvironmentInfoModel.ktandroid/app/src/main/java/com/on/turip/ui/compose/mypage/model/InquiryMail.ktandroid/app/src/main/java/com/on/turip/ui/compose/mypage/model/ProfileModel.ktandroid/app/src/main/java/com/on/turip/ui/compose/mypage/util/AppEnvironmentInfoProvider.ktandroid/app/src/main/java/com/on/turip/ui/compose/setting/SettingScreen.ktandroid/app/src/main/java/com/on/turip/ui/compose/setting/SettingUiEffect.ktandroid/app/src/main/java/com/on/turip/ui/compose/setting/SettingUiState.ktandroid/app/src/main/java/com/on/turip/ui/compose/setting/SettingViewModel.ktandroid/app/src/main/java/com/on/turip/ui/compose/setting/component/SettingItem.ktandroid/app/src/main/java/com/on/turip/ui/main/bookmarks/BookmarkContentFragment.ktandroid/app/src/main/java/com/on/turip/ui/main/bookmarks/BookmarkContentUiEffect.ktandroid/app/src/main/java/com/on/turip/ui/main/bookmarks/BookmarkContentUiState.ktandroid/app/src/main/java/com/on/turip/ui/main/bookmarks/BookmarkContentViewModel.ktandroid/app/src/main/java/com/on/turip/ui/main/favorite/MyPageFragment.ktandroid/app/src/main/java/com/on/turip/ui/main/home/HomeFragment.ktandroid/app/src/main/java/com/on/turip/ui/mypage/MyPageActivity.ktandroid/app/src/main/java/com/on/turip/ui/setting/SettingActivity.ktandroid/app/src/main/res/drawable/btn_chevron_right.xmlandroid/app/src/main/res/drawable/ic_account.xmlandroid/app/src/main/res/drawable/ic_profile_default.xmlandroid/app/src/main/res/values/strings.xml
💤 Files with no reviewable changes (10)
- android/app/src/main/java/com/on/turip/ui/main/bookmarks/BookmarkContentUiEffect.kt
- android/app/src/main/java/com/on/turip/ui/compose/setting/component/SettingItem.kt
- android/app/src/main/java/com/on/turip/ui/compose/setting/SettingViewModel.kt
- android/app/src/main/java/com/on/turip/ui/compose/setting/SettingUiState.kt
- android/app/src/main/java/com/on/turip/ui/main/bookmarks/BookmarkContentViewModel.kt
- android/app/src/main/java/com/on/turip/ui/compose/setting/SettingScreen.kt
- android/app/src/main/java/com/on/turip/ui/setting/SettingActivity.kt
- android/app/src/main/java/com/on/turip/ui/compose/setting/SettingUiEffect.kt
- android/app/src/main/java/com/on/turip/ui/main/bookmarks/BookmarkContentUiState.kt
- android/app/src/main/java/com/on/turip/domain/bookmark/PagedBookmarkContents.kt
android/app/src/main/java/com/on/turip/data/accounts/dto/MyProfileResponse.kt
Show resolved
Hide resolved
android/app/src/main/java/com/on/turip/ui/bookmarks/BookmarkContentActivity.kt
Show resolved
Hide resolved
android/app/src/main/java/com/on/turip/ui/compose/bookmarks/BookmarkContentScreen.kt
Outdated
Show resolved
Hide resolved
android/app/src/main/java/com/on/turip/ui/compose/bookmarks/BookmarkContentViewModel.kt
Outdated
Show resolved
Hide resolved
android/app/src/main/java/com/on/turip/ui/compose/mypage/MyPageViewModel.kt
Outdated
Show resolved
Hide resolved
| fun removeBookmark(contentId: Long) { | ||
| viewModelScope.launch { | ||
| val acquired = removeBookmarkMutex.withLock { removingIds.add(contentId) } | ||
| // 이미 삭제 진행 중이라면 반환 | ||
| if (!acquired) return@launch | ||
|
|
||
| try { | ||
| val removed = | ||
| removeBookmarkMutex.withLock { | ||
| val current = _uiState.value.bookmarkContentState | ||
| val contents = | ||
| (current as? MyPageSectionState.Success)?.data ?: return@withLock false | ||
|
|
||
| // 이미 UI 제거 완료된 상태 (API 호출 완료) | ||
| if (contents.none { it.content.id == contentId }) return@withLock false | ||
|
|
||
| val updated = | ||
| contents.filter { it.content.id != contentId }.toImmutableList() | ||
| _uiState.update { | ||
| it.copy(bookmarkContentState = MyPageSectionState.Success(updated)) | ||
| } | ||
| true | ||
| } | ||
|
|
||
| if (!removed) return@launch | ||
|
|
||
| bookmarkRepository | ||
| .deleteBookmark(contentId) | ||
| .onFailure { | ||
| _uiEffect.send(MyPageUiEffect.ShowBookmarkRemoveFailed) | ||
| } | ||
| } finally { | ||
| // 중복 API 호출 방지 리소스 정리 | ||
| removeBookmarkMutex.withLock { removingIds.remove(contentId) } | ||
| } | ||
| } |
There was a problem hiding this comment.
낙관적 UI 업데이트 실패 시 롤백 누락 — 사용자에게 삭제된 것처럼 보이지만 실제로는 삭제되지 않은 상태가 됩니다.
removeBookmark에서 API 호출 전에 UI에서 항목을 제거하지만, API 실패 시 ShowBookmarkRemoveFailed 이펙트만 보내고 제거된 항목을 복원하지 않습니다. 사용자가 화면을 나갔다 돌아오기 전까지는 해당 북마크가 사라진 것으로 보이게 됩니다.
실패 시 이전 상태로 롤백하는 로직이 필요합니다.
🔧 롤백 로직 추가 제안
fun removeBookmark(contentId: Long) {
viewModelScope.launch {
val acquired = removeBookmarkMutex.withLock { removingIds.add(contentId) }
if (!acquired) return@launch
try {
+ val snapshot = _uiState.value.bookmarkContentState
val removed =
removeBookmarkMutex.withLock {
val current = _uiState.value.bookmarkContentState
val contents =
(current as? MyPageSectionState.Success)?.data ?: return@withLock false
if (contents.none { it.content.id == contentId }) return@withLock false
val updated =
contents.filter { it.content.id != contentId }.toImmutableList()
_uiState.update {
it.copy(bookmarkContentState = MyPageSectionState.Success(updated))
}
true
}
if (!removed) return@launch
bookmarkRepository
.deleteBookmark(contentId)
.onFailure {
+ _uiState.update { it.copy(bookmarkContentState = snapshot) }
_uiEffect.send(MyPageUiEffect.ShowBookmarkRemoveFailed)
}
} finally {
removeBookmarkMutex.withLock { removingIds.remove(contentId) }
}
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@android/app/src/main/java/com/on/turip/ui/compose/mypage/MyPageViewModel.kt`
around lines 96 - 131, In removeBookmark, preserve the current bookmark list
before performing the optimistic UI removal and, if
bookmarkRepository.deleteBookmark(contentId) fails, restore _uiState to that
saved list and still emit MyPageUiEffect.ShowBookmarkRemoveFailed; specifically,
inside removeBookmark capture the previous contents (from
(_uiState.value.bookmarkContentState as? MyPageSectionState.Success)?.data) into
a local variable before updating _uiState, then in the onFailure block update
_uiState back to MyPageSectionState.Success(previousContents) (while respecting
the existing removeBookmarkMutex/removingIds cleanup in the finally block) so
the UI is rolled back on API failure.
android/app/src/main/java/com/on/turip/ui/compose/mypage/MyPageViewModel.kt
Show resolved
Hide resolved
android/app/src/main/java/com/on/turip/ui/main/favorite/MyPageFragment.kt
Show resolved
Hide resolved
android/app/src/main/java/com/on/turip/ui/mypage/MyPageActivity.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
♻️ Duplicate comments (3)
android/app/src/main/java/com/on/turip/ui/compose/bookmarks/BookmarkContentScreen.kt (1)
386-388:⚠️ Potential issue | 🟡 Minor프리뷰 함수명이 상태와 불일치합니다
성공 상태 프리뷰인데 함수명이
BookmarkContentErrorSuccess로 되어 있습니다. 이름을 성공 상태에 맞게 정리해 주세요.✏️ 수정 제안
-private fun BookmarkContentErrorSuccess() { +private fun BookmarkContentSuccessPreview() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/com/on/turip/ui/compose/bookmarks/BookmarkContentScreen.kt` around lines 386 - 388, The preview function name BookmarkContentErrorSuccess is misleading for a success-state preview; rename the function to something that reflects success (e.g., BookmarkContentSuccessPreview or BookmarkContentPreviewSuccess) and update any internal references/usages to that symbol, keeping the `@Preview`(showBackground = true, name = "정상") and `@Composable` annotations intact and ensuring no other functions or tests reference the old name remain unchanged.android/app/src/main/java/com/on/turip/ui/compose/bookmarks/BookmarkContentViewModel.kt (2)
139-208:⚠️ Potential issue | 🟠 MajorUiError.Feature 경로에서 로딩 상태가 해제되지 않습니다
uiError가UiError.Global이 아닐 때 REFRESH/APPEND의 로딩 플래그 해제가 누락되어 무한 로딩/추가 로드 불가 상태가 될 수 있습니다.else분기로 최소한 로딩 상태를 내려 주세요.🐛 수정 제안
PagingLoadMode.REFRESH -> { val uiError: UiError = errorType.toUiError() if (uiError is UiError.Global) { when (uiError) { UiError.Global.Network -> { ... } UiError.Global.Server -> { ... } UiError.Global.TokenExpired -> { ... } } + } else { + _uiState.update { it.copy(isLoading = false) } } } PagingLoadMode.APPEND -> { val uiError: UiError = errorType.toUiError() if (uiError is UiError.Global) { when (uiError) { UiError.Global.Network -> { ... } UiError.Global.Server -> { ... } UiError.Global.TokenExpired -> { ... } } + } else { + _uiState.update { + it.copy(bookmarkContents = it.bookmarkContents.copy(isAppending = false)) + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/com/on/turip/ui/compose/bookmarks/BookmarkContentViewModel.kt` around lines 139 - 208, The error-handling branches for PagingLoadMode.REFRESH and .APPEND only clear loading flags when uiError is a UiError.Global subtype; add an else branch after the existing "if (uiError is UiError.Global)" in both REFRESH and APPEND blocks inside BookmarkContentViewModel to ensure you always clear loading: for REFRESH call _uiState.update { it.copy(isLoading = false) } and for APPEND call _uiState.update { it.copy(bookmarkContents = it.bookmarkContents.copy(isAppending = false)) } so non-global UiError.Feature cases don't leave the UI stuck loading.
101-110:⚠️ Potential issue | 🟠 MajorAPPEND에서
.last()재조회로 빈 리스트 예외 가능APPEND 분기에서
items.isEmpty()체크 후 다시uiState.value를 읽어.last()를 호출하면, 중간에 삭제가 일어나 빈 리스트가 될 경우NoSuchElementException이 발생할 수 있습니다. 한 번 스냅샷을 잡아 재사용하거나lastOrNull()로 방어해 주세요.🐛 수정 제안
PagingLoadMode.APPEND -> { - uiState.value.bookmarkContents.items - .last() - .content.id + uiState.value.bookmarkContents.items + .lastOrNull() + ?.content?.id ?: return@launch }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/com/on/turip/ui/compose/bookmarks/BookmarkContentViewModel.kt` around lines 101 - 110, The APPEND branch reads uiState.value twice and calls .last() which can throw NoSuchElementException if the list is mutated between reads; fix by taking a single snapshot of the list (e.g., val items = uiState.value.bookmarkContents.items) and then use a safe lookup like lastOrNull() (or check items.isEmpty() once) to obtain the last item's content id, providing a safe fallback (e.g., 0L or appropriate sentinel) when no last element exists; update the logic around lastItemId in BookmarkContentViewModel to use this snapshot + safe access instead of uiState.value.bookmarkContents.items.last().
🧹 Nitpick comments (4)
android/app/src/main/java/com/on/turip/data/bookmarks/dto/BookmarkCountResponse.kt (1)
7-9:@SerialName("count")는 생략 가능합니다 (선택적).kotlinx.serialization은 별도 설정 없이 프로퍼티 이름을 JSON 키로 사용하므로, 필드명과 직렬화 키가 동일한 경우
@SerialName은 중복입니다.♻️ 제안 (선택사항)
`@Serializable` data class BookmarkCountResponse( - `@SerialName`("count") val count: Int, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/com/on/turip/data/bookmarks/dto/BookmarkCountResponse.kt` around lines 7 - 9, The `@SerialName`("count") annotation on the BookmarkCountResponse data class is redundant because kotlinx.serialization uses the property name by default; remove the `@SerialName` annotation from the count property in BookmarkCountResponse so the class simply declares val count: Int, keeping the rest of the class unchanged.android/app/src/main/java/com/on/turip/ui/compose/bookmarks/BookmarkContentUiState.kt (1)
19-32:Idle상태명이 실제 동작과 어긋납니다
Idle이isLoading = true로 시작해 실제로는 로딩 상태입니다. 혼동을 줄이려면Idle을Loading/Initial로 rename하거나isLoading = false로 맞추는 쪽을 검토해 주세요.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/com/on/turip/ui/compose/bookmarks/BookmarkContentUiState.kt` around lines 19 - 32, The companion object defines Idle: BookmarkContentUiState but sets isLoading = true, which mismatches the name; either rename Idle to Loading/Initial or change isLoading to false. Update the symbol BookmarkContentUiState.Companion.Idle (or rename it) so the state name matches its behavior: if you intend an initial loading state rename Idle to Loading or Initial; if you intend a true idle state set isLoading = false and adjust any callers expecting Idle accordingly.android/app/src/main/java/com/on/turip/ui/compose/bookmarks/BookmarkContentViewModel.kt (1)
246-255: 삭제 실패 시 UI/서버 불일치가 남습니다낙관적 제거 후 실패 시 리스트를 복원하지 않아 서버 상태와 UI가 어긋날 수 있습니다. 실패 시 자동 refresh 또는 복원 로직을 고려해 주세요.
🔄 최소 복구안 (자동 refresh)
}.onFailure { _uiEffect.send(BookmarkContentUiEffect.ShowBookmarkRemoveFailed) + refreshBookmarkContents() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/com/on/turip/ui/compose/bookmarks/BookmarkContentViewModel.kt` around lines 246 - 255, The optimistic delete updates UI and emits BookmarkRemoved but does not restore state on failure; modify the delete flow for bookmarkRepository.deleteBookmark(contentId) to revert or refresh UI on .onFailure: either increment totalBookmarkCount back via _uiState.update (undo the earlier state.copy(totalBookmarkCount = state.totalBookmarkCount?.minus(1))) or trigger a full refresh by calling the existing reload/fetch method and then send BookmarkContentUiEffect.ShowBookmarkRemoveFailed; ensure the .onSuccess still sends BookmarkContentUiEffect.BookmarkRemoved and .onFailure both sends ShowBookmarkRemoveFailed and performs the restore/refresh to keep UI/server in sync.android/app/src/main/java/com/on/turip/ui/compose/bookmarks/BookmarkContentScreen.kt (1)
200-226:shouldLoadMore가 최신 pagingState 변경을 반영하지 못할 수 있습니다
remember { derivedStateOf { ... } }가 비-스냅샷 값인pagingState를 캡처해hasNext/isAppending/errorUiState변경이 반영되지 않을 수 있습니다.rememberUpdatedState로 최신 값을 반영해 주세요.♻️ 개선 예시
import androidx.compose.runtime.remember +import androidx.compose.runtime.rememberUpdatedState ... - val pagingState: PagingState<BookmarkContent> = uiState.bookmarkContents + val pagingState by rememberUpdatedState(uiState.bookmarkContents)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/com/on/turip/ui/compose/bookmarks/BookmarkContentScreen.kt` around lines 200 - 226, The derived shouldLoadMore currently captures a stale pagingState via remember { derivedStateOf { ... } }; wrap the latest pagingState in rememberUpdatedState (e.g., val currentPagingState by rememberUpdatedState(pagingState)) and use currentPagingState inside the derivedStateOf so changes to hasNext/isAppending/errorUiState/items are observed; keep using listState and the existing LaunchedEffect/snapshotFlow to collect and call loadMore().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@android/app/src/main/java/com/on/turip/ui/compose/bookmarks/BookmarkContentScreen.kt`:
- Around line 386-388: The preview function name BookmarkContentErrorSuccess is
misleading for a success-state preview; rename the function to something that
reflects success (e.g., BookmarkContentSuccessPreview or
BookmarkContentPreviewSuccess) and update any internal references/usages to that
symbol, keeping the `@Preview`(showBackground = true, name = "정상") and `@Composable`
annotations intact and ensuring no other functions or tests reference the old
name remain unchanged.
In
`@android/app/src/main/java/com/on/turip/ui/compose/bookmarks/BookmarkContentViewModel.kt`:
- Around line 139-208: The error-handling branches for PagingLoadMode.REFRESH
and .APPEND only clear loading flags when uiError is a UiError.Global subtype;
add an else branch after the existing "if (uiError is UiError.Global)" in both
REFRESH and APPEND blocks inside BookmarkContentViewModel to ensure you always
clear loading: for REFRESH call _uiState.update { it.copy(isLoading = false) }
and for APPEND call _uiState.update { it.copy(bookmarkContents =
it.bookmarkContents.copy(isAppending = false)) } so non-global UiError.Feature
cases don't leave the UI stuck loading.
- Around line 101-110: The APPEND branch reads uiState.value twice and calls
.last() which can throw NoSuchElementException if the list is mutated between
reads; fix by taking a single snapshot of the list (e.g., val items =
uiState.value.bookmarkContents.items) and then use a safe lookup like
lastOrNull() (or check items.isEmpty() once) to obtain the last item's content
id, providing a safe fallback (e.g., 0L or appropriate sentinel) when no last
element exists; update the logic around lastItemId in BookmarkContentViewModel
to use this snapshot + safe access instead of
uiState.value.bookmarkContents.items.last().
---
Nitpick comments:
In
`@android/app/src/main/java/com/on/turip/data/bookmarks/dto/BookmarkCountResponse.kt`:
- Around line 7-9: The `@SerialName`("count") annotation on the
BookmarkCountResponse data class is redundant because kotlinx.serialization uses
the property name by default; remove the `@SerialName` annotation from the count
property in BookmarkCountResponse so the class simply declares val count: Int,
keeping the rest of the class unchanged.
In
`@android/app/src/main/java/com/on/turip/ui/compose/bookmarks/BookmarkContentScreen.kt`:
- Around line 200-226: The derived shouldLoadMore currently captures a stale
pagingState via remember { derivedStateOf { ... } }; wrap the latest pagingState
in rememberUpdatedState (e.g., val currentPagingState by
rememberUpdatedState(pagingState)) and use currentPagingState inside the
derivedStateOf so changes to hasNext/isAppending/errorUiState/items are
observed; keep using listState and the existing LaunchedEffect/snapshotFlow to
collect and call loadMore().
In
`@android/app/src/main/java/com/on/turip/ui/compose/bookmarks/BookmarkContentUiState.kt`:
- Around line 19-32: The companion object defines Idle: BookmarkContentUiState
but sets isLoading = true, which mismatches the name; either rename Idle to
Loading/Initial or change isLoading to false. Update the symbol
BookmarkContentUiState.Companion.Idle (or rename it) so the state name matches
its behavior: if you intend an initial loading state rename Idle to Loading or
Initial; if you intend a true idle state set isLoading = false and adjust any
callers expecting Idle accordingly.
In
`@android/app/src/main/java/com/on/turip/ui/compose/bookmarks/BookmarkContentViewModel.kt`:
- Around line 246-255: The optimistic delete updates UI and emits
BookmarkRemoved but does not restore state on failure; modify the delete flow
for bookmarkRepository.deleteBookmark(contentId) to revert or refresh UI on
.onFailure: either increment totalBookmarkCount back via _uiState.update (undo
the earlier state.copy(totalBookmarkCount = state.totalBookmarkCount?.minus(1)))
or trigger a full refresh by calling the existing reload/fetch method and then
send BookmarkContentUiEffect.ShowBookmarkRemoveFailed; ensure the .onSuccess
still sends BookmarkContentUiEffect.BookmarkRemoved and .onFailure both sends
ShowBookmarkRemoveFailed and performs the restore/refresh to keep UI/server in
sync.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
android/app/src/main/java/com/on/turip/data/bookmarks/datasource/BookmarkRemoteDataSource.ktandroid/app/src/main/java/com/on/turip/data/bookmarks/datasource/DefaultBookmarkRemoteDataSource.ktandroid/app/src/main/java/com/on/turip/data/bookmarks/dto/BookmarkCountResponse.ktandroid/app/src/main/java/com/on/turip/data/bookmarks/repository/DefaultBookmarkRepository.ktandroid/app/src/main/java/com/on/turip/data/bookmarks/service/BookmarkService.ktandroid/app/src/main/java/com/on/turip/domain/bookmark/repository/BookmarkRepository.ktandroid/app/src/main/java/com/on/turip/ui/compose/bookmarks/BookmarkContentScreen.ktandroid/app/src/main/java/com/on/turip/ui/compose/bookmarks/BookmarkContentUiState.ktandroid/app/src/main/java/com/on/turip/ui/compose/bookmarks/BookmarkContentViewModel.ktandroid/app/src/main/res/values/strings.xml
- BookmarkContent Result로 내려주는 변경여부 정보를 savedInstanceState로 저장 - 문의하기 인텐트 로직도 safe 하게 처리하도록 수정 - preview 네이밍 수정
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
android/app/src/main/java/com/on/turip/ui/compose/bookmarks/BookmarkContentViewModel.kt (1)
63-74:⚠️ Potential issue | 🟠 Major
UiError.Global강제 캐스팅은 런타임 크래시 위험이 있습니다.
toUiError()가UiError.Feature를 반환하면ClassCastException로 앱이 종료됩니다. 최소한 모든 에러 타입에서 로딩 플래그가 해제되도록 분기 처리가 필요합니다.🛠️ 수정 제안
- .onFailure { errorType: ErrorType -> + .onFailure { errorType: ErrorType -> Timber.e("북마크 화면 에러 loadMode = $loadMode") - val uiError: UiError.Global = errorType.toUiError() as UiError.Global - when (loadMode) { - PagingLoadMode.REFRESH -> applyBookmarkContentsRefreshFailure(uiError) - PagingLoadMode.APPEND -> applyBookmarkContentsAppendFailure(uiError) - } + when (val uiError = errorType.toUiError()) { + is UiError.Global -> { + when (loadMode) { + PagingLoadMode.REFRESH -> applyBookmarkContentsRefreshFailure(uiError) + PagingLoadMode.APPEND -> applyBookmarkContentsAppendFailure(uiError) + } + } + else -> { + // 최소 로딩 해제 (필요 시 Feature 타입에 맞는 UI 상태 매핑) + when (loadMode) { + PagingLoadMode.REFRESH -> + _uiState.update { it.copy(isLoading = false, errorUiState = ErrorUiState.Server) } + PagingLoadMode.APPEND -> + _uiState.update { state -> + state.copy( + bookmarkContents = state.bookmarkContents.copy( + isAppending = false, + errorUiState = ErrorUiState.Server, + ), + ) + } + } + } + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/com/on/turip/ui/compose/bookmarks/BookmarkContentViewModel.kt` around lines 63 - 74, In the onFailure lambda of bookmarkRepository.loadBookmarks in BookmarkContentViewModel, remove the unsafe cast to UiError.Global and instead branch on the actual UiError returned by toUiError() (handle UiError.Global and UiError.Feature cases explicitly); for each branch call the appropriate failure handler (applyBookmarkContentsRefreshFailure or applyBookmarkContentsAppendFailure) or a generalized failure handler, and ensure the loading flags are cleared in every path so a ClassCastException cannot crash the app and loading state is always reset.
🧹 Nitpick comments (1)
android/app/src/main/res/values/strings.xml (1)
71-76: “콘텐츠/컨텐츠” 용어 통일 권장.같은 섹션에서 “콘텐츠”와 “컨텐츠”가 혼용됩니다(예: 빈 상태 문구). 한 표기로 통일하면 UX/번역 관리가 쉬워집니다.
♻️ 제안 수정안
- <string name="bookmark_content_empty_suggest_title">컨텐츠를 저장 해보세요!</string> - <string name="bookmark_content_empty_suggest_description">원하는 동선이 담긴\n여행 일정을 저장해 보세요.</string> + <string name="bookmark_content_empty_suggest_title">콘텐츠를 저장해 보세요!</string> + <string name="bookmark_content_empty_suggest_description">원하는 동선이 담긴\n여행 일정을 저장해 보세요.</string>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/res/values/strings.xml` around lines 71 - 76, The strings file mixes two Korean spellings ("콘텐츠" and "컨텐츠"); normalize to one spelling (use "콘텐츠") across the affected resources: update bookmark_content_empty_suggest_title and bookmark_content_empty_suggest_description to use "콘텐츠" (keeping existing phrasing and line breaks), and verify bookmark_content_title, bookmark_content_count, and bookmark_content_count_fail remain consistent after the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@android/app/src/main/java/com/on/turip/ui/compose/bookmarks/BookmarkContentViewModel.kt`:
- Around line 232-268: The optimistic delete in removeBookmark updates _uiState
(bookmarkContents and totalBookmarkCount) before the API call but does not
restore the previous state on failure; capture a snapshot of the current items
and count before you mutate (inside removeBookmark / the removeBookmarkMutex
block), then if bookmarkRepository.deleteBookmark(contentId) fails, restore
_uiState using that snapshot and send the ShowBookmarkRemoveFailed effect; also
ensure you clean up removingIds (remove contentId) in the failure and/or finally
path so the mutex state is consistent.
In `@android/app/src/main/res/values/strings.xml`:
- Around line 49-68: The strings for withdrawal are inconsistent: update the
values of my_page_withdraw_dialog_message and my_page_withdraw_dialog_confirm to
use the spaced form "회원 탈퇴" to match my_page_withdraw (and keep dismiss/message
wording unchanged); locate and edit the string resources named
my_page_withdraw_dialog_message and my_page_withdraw_dialog_confirm and replace
"회원탈퇴" with "회원 탈퇴" so all occurrences are consistent.
---
Duplicate comments:
In
`@android/app/src/main/java/com/on/turip/ui/compose/bookmarks/BookmarkContentViewModel.kt`:
- Around line 63-74: In the onFailure lambda of bookmarkRepository.loadBookmarks
in BookmarkContentViewModel, remove the unsafe cast to UiError.Global and
instead branch on the actual UiError returned by toUiError() (handle
UiError.Global and UiError.Feature cases explicitly); for each branch call the
appropriate failure handler (applyBookmarkContentsRefreshFailure or
applyBookmarkContentsAppendFailure) or a generalized failure handler, and ensure
the loading flags are cleared in every path so a ClassCastException cannot crash
the app and loading state is always reset.
---
Nitpick comments:
In `@android/app/src/main/res/values/strings.xml`:
- Around line 71-76: The strings file mixes two Korean spellings ("콘텐츠" and
"컨텐츠"); normalize to one spelling (use "콘텐츠") across the affected resources:
update bookmark_content_empty_suggest_title and
bookmark_content_empty_suggest_description to use "콘텐츠" (keeping existing
phrasing and line breaks), and verify bookmark_content_title,
bookmark_content_count, and bookmark_content_count_fail remain consistent after
the change.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
android/app/src/main/java/com/on/turip/ui/bookmarks/BookmarkContentActivity.ktandroid/app/src/main/java/com/on/turip/ui/common/paging/PagingLoadMode.ktandroid/app/src/main/java/com/on/turip/ui/compose/bookmarks/BookmarkContentScreen.ktandroid/app/src/main/java/com/on/turip/ui/compose/bookmarks/BookmarkContentViewModel.ktandroid/app/src/main/java/com/on/turip/ui/mypage/MyPageActivity.ktandroid/app/src/main/res/values/strings.xml
| // 낙관적 UI | ||
| fun removeBookmark(contentId: Long) { | ||
| viewModelScope.launch { | ||
| val acquired = removeBookmarkMutex.withLock { removingIds.add(contentId) } | ||
| // 이미 삭제 진행 중이라면 반환 | ||
| if (!acquired) return@launch | ||
|
|
||
| try { | ||
| val removed = | ||
| removeBookmarkMutex.withLock { | ||
| val contents = _uiState.value.bookmarkContents | ||
|
|
||
| // 이미 UI 제거 완료된 상태 (API 호출 완료) | ||
| if (contents.items.none { it.content.id == contentId }) return@withLock false | ||
|
|
||
| val updated = | ||
| contents.items.filter { it.content.id != contentId }.toImmutableList() | ||
|
|
||
| _uiState.update { state -> | ||
| state.copy(bookmarkContents = state.bookmarkContents.copy(items = updated)) | ||
| } | ||
|
|
||
| true | ||
| } | ||
|
|
||
| if (!removed) return@launch | ||
|
|
||
| bookmarkRepository | ||
| .deleteBookmark(contentId) | ||
| .onSuccess { | ||
| _uiState.update { state -> | ||
| state.copy(totalBookmarkCount = state.totalBookmarkCount?.minus(1)) | ||
| } | ||
| _uiEffect.send(BookmarkContentUiEffect.BookmarkRemoved) | ||
| }.onFailure { | ||
| _uiEffect.send(BookmarkContentUiEffect.ShowBookmarkRemoveFailed) | ||
| } |
There was a problem hiding this comment.
삭제 실패 시 UI 롤백이 없어 리스트/카운트 불일치가 발생할 수 있습니다.
낙관적 삭제 후 API 실패 시 아이템이 복원되지 않아, “삭제 실패” 안내와 실제 UI 상태가 어긋납니다. 실패 시 최소한 리스트를 롤백하는 처리가 필요합니다.
🛠️ 수정 제안 (롤백 스냅샷 사용)
- try {
- val removed =
+ try {
+ val removedSnapshot =
removeBookmarkMutex.withLock {
val contents = _uiState.value.bookmarkContents
-
- // 이미 UI 제거 완료된 상태 (API 호출 완료)
- if (contents.items.none { it.content.id == contentId }) return@withLock false
-
- val updated =
- contents.items.filter { it.content.id != contentId }.toImmutableList()
+ val index = contents.items.indexOfFirst { it.content.id == contentId }
+ if (index == -1) return@withLock null
+ val removedItem = contents.items[index]
+ val updated =
+ contents.items.filter { it.content.id != contentId }.toImmutableList()
_uiState.update { state ->
state.copy(bookmarkContents = state.bookmarkContents.copy(items = updated))
}
-
- true
+ index to removedItem
}
- if (!removed) return@launch
+ val (index, removedItem) = removedSnapshot ?: return@launch
bookmarkRepository
.deleteBookmark(contentId)
.onSuccess {
_uiState.update { state ->
state.copy(totalBookmarkCount = state.totalBookmarkCount?.minus(1))
}
_uiEffect.send(BookmarkContentUiEffect.BookmarkRemoved)
}.onFailure {
_uiEffect.send(BookmarkContentUiEffect.ShowBookmarkRemoveFailed)
+ removeBookmarkMutex.withLock {
+ _uiState.update { state ->
+ val items = state.bookmarkContents.items.toMutableList()
+ if (items.none { it.content.id == removedItem.content.id }) {
+ items.add(index, removedItem)
+ }
+ state.copy(
+ bookmarkContents = state.bookmarkContents.copy(
+ items = items.toImmutableList(),
+ ),
+ )
+ }
+ }
}
} finally {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@android/app/src/main/java/com/on/turip/ui/compose/bookmarks/BookmarkContentViewModel.kt`
around lines 232 - 268, The optimistic delete in removeBookmark updates _uiState
(bookmarkContents and totalBookmarkCount) before the API call but does not
restore the previous state on failure; capture a snapshot of the current items
and count before you mutate (inside removeBookmark / the removeBookmarkMutex
block), then if bookmarkRepository.deleteBookmark(contentId) fails, restore
_uiState using that snapshot and send the ShowBookmarkRemoveFailed effect; also
ensure you clean up removingIds (remove contentId) in the failure and/or finally
path so the mutex state is consistent.
There was a problem hiding this comment.
r: 낙관적 삭제 후 롤백이 현재 없는 거 같아요 반영하면 좋을 거 같습니다!
There was a problem hiding this comment.
@jerry8282, 네, 맞습니다! 현재 코드에서는 낙관적 삭제 후 API 실패 시 UI를 원상복구하지 않아서 사용자에게 혼란을 줄 수 있습니다.
삭제 실패 스낵바는 표시되지만 실제로는 아이템이 이미 리스트에서 사라진 상태이고, totalBookmarkCount도 감소된 상태로 남아있게 됩니다. 위에서 제안한 것처럼 삭제 전 스냅샷을 저장해두고 실패 시 복원하는 로직을 추가하면 일관성 있는 UX를 제공할 수 있을 것 같습니다.
구현 시 도움이 필요하시면 말씀해 주세요! 😊
There was a problem hiding this comment.
낙관적 삭제 실패시, 북마크 해제 실패했어요. 새로고침 시 다시 나타날 수 있어요. 로 실패를 알려주고, 새로고침을 제공하고 있는데 이 흐름은 어색할까요 ??
그리고 코드래빗이 말한 totalBookmarkCount는 API 성공일 때만 감소하도록 되어 있어 문제가 되지 않을 것 같아요!
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
jerry8282
left a comment
There was a problem hiding this comment.
낙관적 UI에 이어 페이지네이션까지 고생많으셨습니다 채넛!! 구현 너무나 잘해주셨네요👍👍
❗️ 현재 API 요청에 lastId, size를 원시타입의 매개변수로 담아주고 있는데 이 부분도 Cursor(lastId: Long?, size: Int) 로 페이지 전용 도메인으로 만드는 게 좋을까요?
저는 묶는 게 좋아보입니다! 나중에 cursor가 string값으로 오는 정책으로 변경이 된다고 하면
외부 사용 코드를 크게 수정하지 않고 내부 구현만 변경할 수 있다는 점에서 대응이 더 좋아보입니다.
몇가지 궁금한 점들을 남겨두어서 확인 한번 해주시면 감사하겠습니다.😊
|
|
||
| @Composable | ||
| fun BookmarkContentScreen( | ||
| navigateToBack: () -> Unit, |
There was a problem hiding this comment.
c: 보통 goBack을 많이 쓰는데 goBack쓰는 거 어떠신가요?😊
| import kotlinx.coroutines.flow.filter | ||
|
|
||
| @Composable | ||
| fun BookmarkContentScreen( |
There was a problem hiding this comment.
c: 네이밍이 현재 Bookmarked와 헷갈리는데 BookmarkListScreen는 어떤가유?😊
| ) { | ||
| val pagingState: PagingState<BookmarkContent> = uiState.bookmarkContents | ||
| val listState = rememberLazyListState() | ||
| val threshold = 1 |
There was a problem hiding this comment.
c: 무한 스크롤을 적용하려면 threshold를 조금 더 높여야 될 거 같아요 😊
There was a problem hiding this comment.
화면에 2.5개 정도 아이템이 보이는 것 같은데 3 정도면 괜찮을까요?!
일단 3개로 반영해놨어요! 다른 값을 원하시면 수정하겠습니다 : )
반영 커밋 : 241b5ec
| if (uiState.totalBookmarkCount != null) { | ||
| stringResource(R.string.bookmark_content_count, uiState.totalBookmarkCount) | ||
| } else { | ||
| stringResource(R.string.bookmark_content_count_fail) |
There was a problem hiding this comment.
c: 이 string은 현재 안쓰이고 있는 거 같은데 어디서 쓰일 예정인가요?
There was a problem hiding this comment.
북마크 콘텐츠가 주요 정보, 콘텐츠 수는 사용자에게 추가정보라고 생각했어요.
콘텐츠 수 조회 부분만 재요청하는 UX를 제공하기 보다는 콘텐츠 - 와 같이 수량을 제공하지 않도록 구현했는데 재시도 요청이 필요할까요 ??
해당 상황에 대한 preview는 추가했습니다!
반영 커밋 : b1886c7
| ) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
c: 더보기 에러 보기위한 preview 추가하면 좋을 거 같아요😊
@Preview(showBackground = true, name = "더보기 에러")
@Composable
private fun BookmarkContentLoadMoreErrorPreview() {
val contents =
persistentListOf(
BookmarkContent(
content =
Content(
1L,
Creator(1L, "채널명", ""),
VideoData("콘텐츠 제목", "thumbnail", "2026-01-12"),
City("대구"),
true,
),
tripDuration = TripDuration(1, 2),
tripPlaceCount = 2,
),
BookmarkContent(
content =
Content(
2L,
Creator(2L, "다른 채널", ""),
VideoData("두 번째 콘텐츠", "thumbnail", "2025-01-12"),
City("서울"),
true,
),
tripDuration = TripDuration(0, 1),
tripPlaceCount = 1,
),
)
TuripTheme {
BookmarkContentContent(
uiState =
BookmarkContentUiState(
isLoading = false,
bookmarkContents =
PagingState(
items = contents,
hasNext = true,
isAppending = false,
errorUiState = ErrorUiState.Network,
),
totalBookmarkCount = 2,
errorUiState = ErrorUiState.None,
),
onRetryClick = {},
onContentClick = {},
onBookmarkClick = {},
loadMoreContents = {},
)
}
}
| import kotlinx.collections.immutable.persistentListOf | ||
|
|
||
| @Immutable | ||
| data class BookmarkContentUiState( |
There was a problem hiding this comment.
c: MyPageSectionState에서는 loading Success error를 분리하셨는데 여기서는 안하신 이유가 있나요?
There was a problem hiding this comment.
MyPageSectionState에서는 하단에 문의하기, 로그인/로그아웃, 개인정보처리방침도 있어 영역별로 에러, 로딩, 성공을 관리하는 게 적절하다고 판단해서 그런 방식을 적용했고
북마크 목록은 화면전체가 북마크 데이터여서 따로 분리하지 않았습니다! :)
| // 낙관적 UI | ||
| fun removeBookmark(contentId: Long) { | ||
| viewModelScope.launch { | ||
| val acquired = removeBookmarkMutex.withLock { removingIds.add(contentId) } | ||
| // 이미 삭제 진행 중이라면 반환 | ||
| if (!acquired) return@launch | ||
|
|
||
| try { | ||
| val removed = | ||
| removeBookmarkMutex.withLock { | ||
| val contents = _uiState.value.bookmarkContents | ||
|
|
||
| // 이미 UI 제거 완료된 상태 (API 호출 완료) | ||
| if (contents.items.none { it.content.id == contentId }) return@withLock false | ||
|
|
||
| val updated = | ||
| contents.items.filter { it.content.id != contentId }.toImmutableList() | ||
|
|
||
| _uiState.update { state -> | ||
| state.copy(bookmarkContents = state.bookmarkContents.copy(items = updated)) | ||
| } | ||
|
|
||
| true | ||
| } | ||
|
|
||
| if (!removed) return@launch | ||
|
|
||
| bookmarkRepository | ||
| .deleteBookmark(contentId) | ||
| .onSuccess { | ||
| _uiState.update { state -> | ||
| state.copy(totalBookmarkCount = state.totalBookmarkCount?.minus(1)) | ||
| } | ||
| _uiEffect.send(BookmarkContentUiEffect.BookmarkRemoved) | ||
| }.onFailure { | ||
| _uiEffect.send(BookmarkContentUiEffect.ShowBookmarkRemoveFailed) | ||
| } |
There was a problem hiding this comment.
r: 낙관적 삭제 후 롤백이 현재 없는 거 같아요 반영하면 좋을 거 같습니다!
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
android/app/src/main/java/com/on/turip/ui/compose/mypage/component/MyPageBookmarkContentSection.kt (1)
47-47: 내부 함수 네이밍도MyPage접두로 맞추면 가독성이 더 좋아집니다.공개 API는
MyPageBookmarkContentSection로 정리됐는데, 같은 파일의 private 함수들은BookmarkedContent*네이밍을 유지하고 있어 컨텍스트 전환 비용이 조금 있습니다. 이번 PR 범위가 네이밍 정규화라면 함께 맞추는 것을 권장합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/com/on/turip/ui/compose/mypage/component/MyPageBookmarkContentSection.kt` at line 47, Private helper functions in this file use the BookmarkedContent* prefix while the public API is named MyPageBookmarkContentSection; rename those private helpers to use the MyPage* prefix (e.g., BookmarkedContentHeader -> MyPageBookmarkContentHeader, BookmarkedContentItem -> MyPageBookmarkContentItem) and update all local references/usages accordingly to keep naming consistent and avoid breaking visibility; ensure function signatures and modifiers remain unchanged and run a build to catch any unresolved references.android/app/src/main/java/com/on/turip/ui/compose/bookmark/component/BookmarkContentListItem.kt (1)
36-42: Modifier 순서로 인해 ripple 효과가 보이지 않을 수 있음현재
clickable이background이전에 적용되어 있어 ripple 효과가 백그라운드 컬러에 가려질 수 있습니다. Compose에서 modifier는 순차적으로 적용되므로, ripple이 정상적으로 표시되려면background를clickable앞에 배치해야 합니다.♻️ 수정 제안
modifier = modifier .fillMaxWidth() .clip(TuripTheme.shape.container) - .clickable { onContentClick(content.content.id) } .background(TuripTheme.colors.white) + .clickable { onContentClick(content.content.id) } .padding(TuripTheme.spacing.extraSmall),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/com/on/turip/ui/compose/bookmark/component/BookmarkContentListItem.kt` around lines 36 - 42, The ripple is being drawn under the background because in BookmarkContentListItem the modifier chain applies .clickable before .background; move .background(TuripTheme.colors.white) (and .clip(TuripTheme.shape.container) if needed for shape) earlier in the chain so the .clickable { onContentClick(content.content.id) } is applied after the background/clip — this ensures the ripple is visible; update the modifier chain on the composable that builds the item accordingly.android/app/src/main/java/com/on/turip/ui/compose/mypage/component/MyPageBookmarkContentItem.kt (1)
28-60: LGTM! 컴포넌트 구조가 잘 설계되었습니다.테마 값들을 일관성 있게 사용하고, 공통 컴포넌트(ContentThumbnail, BookmarkContentTitleRow, BookmarkContentMetaSection)를 적절히 재사용하고 있습니다. Modifier 체인 순서도 적절합니다.
한 가지 개선 고려사항으로,
clickablemodifier에 접근성을 위한 semantic 정보 추가를 고려해볼 수 있습니다:.clickable( onClickLabel = "콘텐츠 상세 보기", // 또는 stringResource 사용 onClick = { onContentClick(item.content.id) } )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/com/on/turip/ui/compose/mypage/component/MyPageBookmarkContentItem.kt` around lines 28 - 60, Add accessibility semantics to the clickable modifier in MyPageBookmarkContentItem by providing an onClickLabel (use a localized string via stringResource) when calling clickable so screen readers announce the action; update the clickable invocation that currently calls onContentClick(item.content.id) to include onClickLabel describing the action (e.g., "콘텐츠 상세 보기") while preserving the onContentClick callback and item.content.id usage.android/app/src/main/java/com/on/turip/ui/common/component/bookmark/BookmarkContentTitleRow.kt (1)
13-13: Preview에서 feature(home) 컴포넌트 의존은 분리하는 편이 좋습니다.
ui.common.component.bookmark의 preview가ui.compose.home.component.RegionChip를 직접 참조하면 공통 UI 레이어의 의존 경계가 흐려집니다. preview 샘플은 로컬 더미 trailing UI(예:Text/Box)로 바꿔두는 것을 권장합니다.Also applies to: 46-49
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/com/on/turip/ui/common/component/bookmark/BookmarkContentTitleRow.kt` at line 13, The preview in BookmarkContentTitleRow.kt currently imports and uses RegionChip (com.on.turip.ui.compose.home.component.RegionChip), which leaks the home feature into the common UI layer; update the Preview composable(s) in this file (and the similar usage noted around lines 46-49) to remove the RegionChip import and replace its usage with a local dummy UI (e.g., simple Text or Box) so the preview no longer depends on the home feature and the common UI layer dependency boundary is preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@android/app/src/main/java/com/on/turip/ui/common/component/bookmark/BookmarkContentTitleRow.kt`:
- Around line 30-33: In BookmarkContentTitleRow.kt the call to
Modifier.padding() without arguments causes a compile error—remove the empty
.padding() call or replace it with an explicit value (e.g., .padding(8.dp) or
another appropriate Dp) on the Modifier chain in the composable where
.weight(1f) is used; also fix the reversed dependency by removing the home
package import of RegionChip from the common component (move RegionChip to a
preview/test or the common package, or create a local preview-only stub) so the
common component does not depend on home (check imports in
BookmarkContentTitleRow and Preview functions).
In
`@android/app/src/main/java/com/on/turip/ui/compose/bookmark/BookmarkContentListViewModel.kt`:
- Around line 68-74: The code currently casts errorType.toUiError() unsafely to
UiError.Global which can throw ClassCastException; change to capture the result
(val uiError = errorType.toUiError()) and perform a safe type check
(when/uiError is UiError.Global / UiError.Feature) before calling the handlers:
if uiError is UiError.Global, call applyBookmarkContentsRefreshFailure(uiError)
or applyBookmarkContentsAppendFailure(uiError) based on loadMode; if uiError is
UiError.Feature, handle it explicitly (map it to an appropriate UiError.Global,
call a feature-specific handler, or log/propagate) so no unsafe cast is used.
In
`@android/app/src/main/java/com/on/turip/ui/compose/bookmark/component/BookmarkContentListAppBar.kt`:
- Around line 24-28: The back icon in BookmarkContentListAppBar is currently
made clickable by applying Modifier.clickable to Icon which yields a small touch
target; wrap the Icon with an IconButton (instead of using Modifier.clickable)
so the touch target and semantics meet Material guidelines and match other app
bars like RegionResultAppBar — move the onBackClick to the IconButton's onClick,
remove the clickable modifier from the Icon, and preserve the contentDescription
via stringResource(R.string.all_back_description).
---
Nitpick comments:
In
`@android/app/src/main/java/com/on/turip/ui/common/component/bookmark/BookmarkContentTitleRow.kt`:
- Line 13: The preview in BookmarkContentTitleRow.kt currently imports and uses
RegionChip (com.on.turip.ui.compose.home.component.RegionChip), which leaks the
home feature into the common UI layer; update the Preview composable(s) in this
file (and the similar usage noted around lines 46-49) to remove the RegionChip
import and replace its usage with a local dummy UI (e.g., simple Text or Box) so
the preview no longer depends on the home feature and the common UI layer
dependency boundary is preserved.
In
`@android/app/src/main/java/com/on/turip/ui/compose/bookmark/component/BookmarkContentListItem.kt`:
- Around line 36-42: The ripple is being drawn under the background because in
BookmarkContentListItem the modifier chain applies .clickable before
.background; move .background(TuripTheme.colors.white) (and
.clip(TuripTheme.shape.container) if needed for shape) earlier in the chain so
the .clickable { onContentClick(content.content.id) } is applied after the
background/clip — this ensures the ripple is visible; update the modifier chain
on the composable that builds the item accordingly.
In
`@android/app/src/main/java/com/on/turip/ui/compose/mypage/component/MyPageBookmarkContentItem.kt`:
- Around line 28-60: Add accessibility semantics to the clickable modifier in
MyPageBookmarkContentItem by providing an onClickLabel (use a localized string
via stringResource) when calling clickable so screen readers announce the
action; update the clickable invocation that currently calls
onContentClick(item.content.id) to include onClickLabel describing the action
(e.g., "콘텐츠 상세 보기") while preserving the onContentClick callback and
item.content.id usage.
In
`@android/app/src/main/java/com/on/turip/ui/compose/mypage/component/MyPageBookmarkContentSection.kt`:
- Line 47: Private helper functions in this file use the BookmarkedContent*
prefix while the public API is named MyPageBookmarkContentSection; rename those
private helpers to use the MyPage* prefix (e.g., BookmarkedContentHeader ->
MyPageBookmarkContentHeader, BookmarkedContentItem -> MyPageBookmarkContentItem)
and update all local references/usages accordingly to keep naming consistent and
avoid breaking visibility; ensure function signatures and modifiers remain
unchanged and run a build to catch any unresolved references.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (38)
android/app/src/main/java/com/on/turip/data/account/AccountMapper.ktandroid/app/src/main/java/com/on/turip/data/account/datasource/AccountRemoteDataSource.ktandroid/app/src/main/java/com/on/turip/data/account/datasource/DefaultAccountRemoteDataSource.ktandroid/app/src/main/java/com/on/turip/data/account/dto/MyProfileResponse.ktandroid/app/src/main/java/com/on/turip/data/account/repository/DefaultAccountRepository.ktandroid/app/src/main/java/com/on/turip/data/account/service/AccountService.ktandroid/app/src/main/java/com/on/turip/data/accounts/AccountMapper.ktandroid/app/src/main/java/com/on/turip/data/bookmark/BookmarkMapper.ktandroid/app/src/main/java/com/on/turip/data/bookmark/datasource/BookmarkRemoteDataSource.ktandroid/app/src/main/java/com/on/turip/data/bookmark/datasource/DefaultBookmarkRemoteDataSource.ktandroid/app/src/main/java/com/on/turip/data/bookmark/dto/BookmarkAddRequest.ktandroid/app/src/main/java/com/on/turip/data/bookmark/dto/BookmarkContentResponse.ktandroid/app/src/main/java/com/on/turip/data/bookmark/dto/BookmarkContentsResponse.ktandroid/app/src/main/java/com/on/turip/data/bookmark/dto/BookmarkCountResponse.ktandroid/app/src/main/java/com/on/turip/data/bookmark/repository/DefaultBookmarkRepository.ktandroid/app/src/main/java/com/on/turip/data/bookmark/service/BookmarkService.ktandroid/app/src/main/java/com/on/turip/di/DataSourceModule.ktandroid/app/src/main/java/com/on/turip/di/RepositoryModule.ktandroid/app/src/main/java/com/on/turip/di/ServiceModule.ktandroid/app/src/main/java/com/on/turip/domain/account/Account.ktandroid/app/src/main/java/com/on/turip/domain/account/AccountRepository.ktandroid/app/src/main/java/com/on/turip/domain/account/Role.ktandroid/app/src/main/java/com/on/turip/ui/bookmarks/BookmarkContentActivity.ktandroid/app/src/main/java/com/on/turip/ui/common/component/bookmark/BookmarkContentMetaSection.ktandroid/app/src/main/java/com/on/turip/ui/common/component/bookmark/BookmarkContentTitleRow.ktandroid/app/src/main/java/com/on/turip/ui/common/component/content/ContentThumbnail.ktandroid/app/src/main/java/com/on/turip/ui/compose/bookmark/BookmarkContentListScreen.ktandroid/app/src/main/java/com/on/turip/ui/compose/bookmark/BookmarkContentListUiEffect.ktandroid/app/src/main/java/com/on/turip/ui/compose/bookmark/BookmarkContentListUiState.ktandroid/app/src/main/java/com/on/turip/ui/compose/bookmark/BookmarkContentListViewModel.ktandroid/app/src/main/java/com/on/turip/ui/compose/bookmark/component/BookmarkContentListAppBar.ktandroid/app/src/main/java/com/on/turip/ui/compose/bookmark/component/BookmarkContentListItem.ktandroid/app/src/main/java/com/on/turip/ui/compose/mypage/MyPageScreen.ktandroid/app/src/main/java/com/on/turip/ui/compose/mypage/MyPageViewModel.ktandroid/app/src/main/java/com/on/turip/ui/compose/mypage/component/MyPageBookmarkContentItem.ktandroid/app/src/main/java/com/on/turip/ui/compose/mypage/component/MyPageBookmarkContentSection.ktandroid/app/src/main/java/com/on/turip/ui/compose/mypage/util/MyPageMapper.ktandroid/app/src/main/java/com/on/turip/ui/main/MainActivity.kt
💤 Files with no reviewable changes (1)
- android/app/src/main/java/com/on/turip/data/accounts/AccountMapper.kt
✅ Files skipped from review due to trivial changes (1)
- android/app/src/main/java/com/on/turip/ui/main/MainActivity.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- android/app/src/main/java/com/on/turip/ui/compose/mypage/MyPageViewModel.kt
android/app/src/main/java/com/on/turip/ui/common/component/bookmark/BookmarkContentTitleRow.kt
Outdated
Show resolved
Hide resolved
android/app/src/main/java/com/on/turip/ui/compose/bookmark/BookmarkContentListViewModel.kt
Show resolved
Hide resolved
...id/app/src/main/java/com/on/turip/ui/compose/bookmark/component/BookmarkContentListAppBar.kt
Outdated
Show resolved
Hide resolved
- 북마크 콘텐츠 타이틀 영역 사용안하는 Modifier 제거, Preview 수정 - 북마크 목록 앱바 뒤로가기버튼 영역 크기 조절
yrsel
left a comment
There was a problem hiding this comment.
제리, preview도 사용할 수 있는 형태로 만들어서 주시고 감사해요 :)
남겨주신 리뷰 및 Cursor에 대한 의견 확인하고 반영했습니다!
수정이 필요한 부분이 있다면 다시 리뷰 부탁드립니다 🤓
| // 낙관적 UI | ||
| fun removeBookmark(contentId: Long) { | ||
| viewModelScope.launch { | ||
| val acquired = removeBookmarkMutex.withLock { removingIds.add(contentId) } | ||
| // 이미 삭제 진행 중이라면 반환 | ||
| if (!acquired) return@launch | ||
|
|
||
| try { | ||
| val removed = | ||
| removeBookmarkMutex.withLock { | ||
| val contents = _uiState.value.bookmarkContents | ||
|
|
||
| // 이미 UI 제거 완료된 상태 (API 호출 완료) | ||
| if (contents.items.none { it.content.id == contentId }) return@withLock false | ||
|
|
||
| val updated = | ||
| contents.items.filter { it.content.id != contentId }.toImmutableList() | ||
|
|
||
| _uiState.update { state -> | ||
| state.copy(bookmarkContents = state.bookmarkContents.copy(items = updated)) | ||
| } | ||
|
|
||
| true | ||
| } | ||
|
|
||
| if (!removed) return@launch | ||
|
|
||
| bookmarkRepository | ||
| .deleteBookmark(contentId) | ||
| .onSuccess { | ||
| _uiState.update { state -> | ||
| state.copy(totalBookmarkCount = state.totalBookmarkCount?.minus(1)) | ||
| } | ||
| _uiEffect.send(BookmarkContentUiEffect.BookmarkRemoved) | ||
| }.onFailure { | ||
| _uiEffect.send(BookmarkContentUiEffect.ShowBookmarkRemoveFailed) | ||
| } |
There was a problem hiding this comment.
낙관적 삭제 실패시, 북마크 해제 실패했어요. 새로고침 시 다시 나타날 수 있어요. 로 실패를 알려주고, 새로고침을 제공하고 있는데 이 흐름은 어색할까요 ??
그리고 코드래빗이 말한 totalBookmarkCount는 API 성공일 때만 감소하도록 되어 있어 문제가 되지 않을 것 같아요!
| import kotlinx.coroutines.flow.filter | ||
|
|
||
| @Composable | ||
| fun BookmarkContentScreen( |
| ) { | ||
| val pagingState: PagingState<BookmarkContent> = uiState.bookmarkContents | ||
| val listState = rememberLazyListState() | ||
| val threshold = 1 |
There was a problem hiding this comment.
화면에 2.5개 정도 아이템이 보이는 것 같은데 3 정도면 괜찮을까요?!
일단 3개로 반영해놨어요! 다른 값을 원하시면 수정하겠습니다 : )
반영 커밋 : 241b5ec
| if (uiState.totalBookmarkCount != null) { | ||
| stringResource(R.string.bookmark_content_count, uiState.totalBookmarkCount) | ||
| } else { | ||
| stringResource(R.string.bookmark_content_count_fail) |
There was a problem hiding this comment.
북마크 콘텐츠가 주요 정보, 콘텐츠 수는 사용자에게 추가정보라고 생각했어요.
콘텐츠 수 조회 부분만 재요청하는 UX를 제공하기 보다는 콘텐츠 - 와 같이 수량을 제공하지 않도록 구현했는데 재시도 요청이 필요할까요 ??
해당 상황에 대한 preview는 추가했습니다!
반영 커밋 : b1886c7
| ) | ||
| } | ||
| } | ||
| } |
| import kotlinx.collections.immutable.persistentListOf | ||
|
|
||
| @Immutable | ||
| data class BookmarkContentUiState( |
There was a problem hiding this comment.
MyPageSectionState에서는 하단에 문의하기, 로그인/로그아웃, 개인정보처리방침도 있어 영역별로 에러, 로딩, 성공을 관리하는 게 적절하다고 판단해서 그런 방식을 적용했고
북마크 목록은 화면전체가 북마크 데이터여서 따로 분리하지 않았습니다! :)
Issues
✔️ Check-list
🗒️ Work Description
북마크 콘텐츠 목록 화면을 Compose로 마이그레이션 진행 완료했습니다!
#603 :
마이페이지 화면 PR에 이어서 작업한 부분이라 603번 PR과 현재 PR에 커밋 내역이 섞여 있어요! 603번 PR이 머지되면 현재 PR에서 커밋내역이 사라질 예정입니다! 임시로 여기 링크를 통해 변경 사항 확인 부탁드려요!머지 완료#609 : 북마크 콘텐츠 수를 표시하는 부분은 608번 PR 콘텐츠 수 조회 API 머지 되고나서야 정상 작동 가능합니다! API 연동은 되어 있고 608번 브랜치에서 정상 작동하는 것 확인했습니다!
북마크 콘텐츠에 대한 페이지네이션 적용 : 백엔드 API 수정 후 정상 적용될 예정입니다. 임시로 20개의 데이터를 받아오도록 했습니다!
페이지네이션을 공통적으로 처리하기 위한 도메인 객체, ui 객체 생성했는데 괜찮은 지 확인 부탁드려요!
도메인
❗️ 현재 API 요청에 lastId, size를 원시타입의 매개변수로 담아주고 있는데 이 부분도
Cursor(lastId: Long?, size: Int)로 페이지 전용 도메인으로 만드는 게 좋을까요?Ui/Presentation
PagingState
items페이지네이션 데이터hasNext다음 데이터를 더 읽을 수 있는 지 여부isAppending데이터 읽어오는 중errorUiState페이지네이션 데이터 요청 중 에러 발생 처리PagingLoadMode : 새로 고침(최초, 재시도)과 페이지네이션 추가 데이터 요청을 구분하도록 했습니다.
📷 Screenshot
정상작동
record.webm
빈 화면
📚 Reference
Summary by CodeRabbit
북마크 기능 업데이트
새로운 기능
리팩토링