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설정 화면을 마이페이지 화면으로 마이그레이션하며, 새로운 계정 도메인 계층, 데이터 소스, 리포지토리, 서비스를 추가합니다. Compose 기반의 마이페이지 UI를 구현하고 기존 설정 관련 파일을 제거하며 DI 모듈과 리소스를 업데이트합니다. Changes
Sequence Diagram(s)sequenceDiagram
participant User as 사용자
participant MyPageUI as MyPage UI
participant ViewModel as MyPageViewModel
participant AccountRepo as AccountRepository
participant DataSource as AccountRemoteDataSource
participant Service as AccountService
participant API as 백엔드 API
User->>MyPageUI: 마이페이지 진입
MyPageUI->>ViewModel: loadProfile()
ViewModel->>AccountRepo: loadMyProfile()
AccountRepo->>DataSource: getMyProfile()
DataSource->>Service: getMyProfile()
Service->>API: GET /accounts/me
API-->>Service: MyProfileResponse
Service-->>DataSource: MyProfileResponse
DataSource-->>AccountRepo: TuripResult<MyProfileResponse>
AccountRepo->>AccountRepo: toDomain() 매핑
AccountRepo-->>ViewModel: TuripResult<Account>
ViewModel->>ViewModel: toUiModel() 변환
ViewModel-->>MyPageUI: profileState = Success(ProfileModel)
MyPageUI->>User: 프로필 표시
Estimated code review effort🎯 4 (복잡) | ⏱️ ~50분 이 PR은 대규모 마이그레이션으로 여러 계층(도메인, 데이터, UI, DI)에 걸쳐 새로운 코드를 추가하고, 기존 설정 화면을 완전히 제거하며, 복잡한 ViewModel 로직이 포함되어 있습니다. 다양한 종류의 변경(새 계층 구조, Compose UI, 상태 관리, 의존성 주입)이 포함되어 있어 각 섹션별로 별도의 검토가 필요합니다. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (11)
android/app/src/main/java/com/on/turip/ui/compose/login/LoginScreen.kt (1)
76-76: 마이그레이션 다이얼로그 dismiss 문자열 키의 의미적 정합성 검토
my_page_logout_dialog_dismiss키는 로그아웃 다이얼로그용으로 네이밍되어 있지만, 여기서는 게스트 데이터 마이그레이션 다이얼로그의 dismiss 버튼에 사용되고 있습니다. 실제 문자열 값이 "취소" 등 범용 텍스트라면 런타임 동작에는 문제가 없으나, 문자열 키 이름과 사용 맥락이 일치하지 않아 향후 로그아웃 다이얼로그 텍스트 수정 시 의도치 않게 마이그레이션 다이얼로그에도 영향이 생길 수 있습니다.마이그레이션 다이얼로그 전용 키(예:
my_page_migration_dialog_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, The dismissText in LoginScreen.kt for the guest data migration dialog is using stringResource(R.string.my_page_logout_dialog_dismiss), which semantically mismatches the migration context; add a new string resource (e.g., my_page_migration_dialog_dismiss) in strings.xml with the appropriate text (e.g., "취소") and update the dismissText call in the migration dialog code to stringResource(R.string.my_page_migration_dialog_dismiss) so the migration dialog has its own dedicated key separate from logout strings.android/app/src/main/java/com/on/turip/ui/compose/mypage/component/MyPageAppBar.kt (2)
23-29: Preview 함수명 오타:MyPageAppBarPreView→MyPageAppBarPreview프로젝트 내 다른 Preview 함수들과 일관되게
Preview로 통일하는 것이 좋습니다 (예:HomeLoadingPreview,HomeSuccessPreview).✏️ 수정 제안
-private fun MyPageAppBarPreView() { +private fun MyPageAppBarPreview() {🤖 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/MyPageAppBar.kt` around lines 23 - 29, Rename the mis-typed preview function MyPageAppBarPreView to MyPageAppBarPreview to match the project's Preview naming convention; update the function declaration and any internal references (the `@Preview/`@Composable annotated function that calls MyPageAppBar within TuripTheme) so the symbol is now MyPageAppBarPreview and the rest of the code remains unchanged.
14-18: 하드코딩된 문자열을 string resource로 교체하는 것을 권장합니다.
HomeScreen.kt에서는stringResource(R.string.home_top_title)등 string resource를 사용하고 있으나, 여기서는"마이페이지"가 직접 하드코딩되어 있습니다. PR에서strings.xml에my_page_*키들을 추가한 것으로 보이므로, 일관성을 위해 string resource를 사용하는 것이 좋습니다.🤖 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/MyPageAppBar.kt` around lines 14 - 18, Replace the hardcoded "마이페이지" literal in the Text composable inside MyPageAppBar (the Text with text = "마이페이지", textAlign = TextAlign.Center, style = TuripTheme.typography.display) with a string resource lookup (e.g. stringResource(R.string.my_page_title)); ensure you import and use androidx.compose.ui.res.stringResource and update the Text's text parameter to use that resource key (matching the my_page_* keys added to strings.xml).android/app/src/main/java/com/on/turip/ui/compose/home/HomeScreen.kt (1)
173-180: 임시 네비게이션 텍스트: 바텀 네비게이션 구현 후 제거 필요.PR 설명에 명시된 대로 임시 텍스트인 점은 확인했습니다. 바텀 네비게이션 작업 후 제거를 잊지 않도록 TODO 주석을 추가해두는 것을 권장합니다.
✏️ 수정 제안
+ // TODO: 바텀 네비게이션 구현 후 제거 Text( text = "임시 마이페이지 확인용 텍스트", modifier = Modifier .padding(top = TuripTheme.spacing.large) .clickable(onClick = onNavigateToMyPage), )🤖 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/home/HomeScreen.kt` around lines 173 - 180, Add a TODO comment above the temporary Text composable in HomeScreen.kt indicating it must be removed after bottom navigation is implemented; specifically reference the Text block that displays "임시 마이페이지 확인용 텍스트" and the onNavigateToMyPage clickable handler so future readers know this is temporary and tied to the upcoming bottom navigation work (e.g., "// TODO: Remove this temporary navigation text after implementing bottom navigation - uses onNavigateToMyPage").android/app/src/main/java/com/on/turip/ui/compose/mypage/component/ProfileSection.kt (1)
140-149:Success분기에서modifier재사용 — 잠재적 레이아웃 의도 불명확
Text에modifier = modifier를 전달하면weight(1f)가Box내부에 적용됩니다.Box는weight를 인식하지 않으므로 지금은 무해하지만, 호출 측의 Modifier가 변경될 경우 의도치 않은 레이아웃 변화가 생길 수 있습니다.Box가 이미modifier를 받아 크기를 결정하므로, 내부Text에는 필요한 수식자만 직접 지정하는 것이 명확합니다.♻️ 제안 수정
is MyPageSectionState.Success -> { Text( text = state.data.nickname, textAlign = TextAlign.Start, style = TuripTheme.typography.title1, maxLines = 2, overflow = TextOverflow.Ellipsis, - modifier = modifier, + modifier = Modifier.fillMaxWidth(), ) }🤖 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/ProfileSection.kt` around lines 140 - 149, The Text in the MyPageSectionState.Success branch reuses the incoming modifier (modifier) which may include layout modifiers like weight(1f) intended for the parent Box; remove passing the full modifier into the Text and instead apply only the required modifiers (e.g., padding, fillMaxWidth, or none) directly on the Text to avoid leaking parent layout intentions. Update the Success branch in ProfileSection (the Text composable inside is MyPageSectionState.Success) to stop forwarding the top-level modifier and use a new, explicit modifier chain appropriate for the Text's needs.android/app/src/main/java/com/on/turip/ui/compose/mypage/component/MyPageSettingItem.kt (1)
65-65:TextAlign.Left대신TextAlign.Start사용 권장
TextAlign.Left는 항상 왼쪽 정렬하지만TextAlign.Start는 레이아웃 방향(RTL/LTR)에 따라 올바르게 동작하는 Compose 관용 표현입니다.♻️ 제안 수정
- textAlign = TextAlign.Left, + textAlign = TextAlign.Start,🤖 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/MyPageSettingItem.kt` at line 65, Replace the hard-coded left alignment with a start-aligned value to respect RTL/LTR layout direction: in MyPageSettingItem (where textAlign = TextAlign.Left is set on the Text composable), change the alignment to TextAlign.Start so the Text composable uses layout direction-aware alignment.android/app/src/main/java/com/on/turip/ui/compose/mypage/component/BookmarkedContentSection.kt (1)
192-198:CircularProgressIndicator의.align(Alignment.Center)중복 적용부모
Box가 이미contentAlignment = Alignment.Center를 사용하고 있으므로,CircularProgressIndicator에 추가된.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 192 - 198, The CircularProgressIndicator has an unnecessary .align(Alignment.Center) because its parent Box already sets contentAlignment = Alignment.Center; remove the redundant .align(Alignment.Center) from the CircularProgressIndicator declaration in BookmarkedContentSection (the component creating the Box and the CircularProgressIndicator) so the indicator is centered by the Box alone.android/app/src/main/java/com/on/turip/ui/main/home/HomeFragment.kt (1)
70-73: 임시 My Page 진입점 — 정리 추적이 필요합니다.PR 설명에 명시된 것처럼 이 코드는 bottom navigation 구현 전 임시 진입점입니다. 추후 정리가 누락될 리스크를 줄이기 위해 별도 이슈로 트래킹하는 것을 권장합니다.
이 임시 코드 정리를 추적하는 이슈를 생성할까요?
🤖 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/home/HomeFragment.kt` around lines 70 - 73, Temporary My Page entrypoint is left in HomeFragment (navigateToMyPage lambda invoking MyPageActivity.newIntent) and needs tracked cleanup; create a dedicated issue in the repo (describe that this is a temporary bottom-nav placeholder and must be removed/reworked when bottom navigation is implemented), then update the HomeFragment navigateToMyPage block to include a concise TODO comment referencing the new issue ID (e.g., TODO: track removal in ISSUE-123) so the temporary MyPageActivity.newIntent usage is clearly tracked until the bottom-nav implementation replaces it.android/app/src/main/java/com/on/turip/ui/compose/mypage/component/MyPageSettingsSection.kt (1)
56-58: 컴포저블 본문 내 직접 사이드 이펙트(Timber.e())는SideEffect {}로 감싸는 것을 권장합니다Compose는 재구성 시 컴포저블 함수를 여러 번 호출할 수 있으므로,
Timber.e()와 같은 사이드 이펙트를 컴포지션 단계에서 직접 호출하면 예상치 못한 횟수로 로그가 출력될 수 있습니다.SideEffect는 성공적인 재구성마다 한 번 실행됨을 보장합니다.♻️ 권장 수정안
UserType.NONE -> { - Timber.e("멤버가 지정되지 않았습니다.") + SideEffect { 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 56 - 58, The Timber.e() call inside the UserType.NONE branch of the MyPageSettingsSection composable should be moved into a SideEffect block to avoid emitting logs during composition; wrap the existing Timber.e("멤버가 지정되지 않았습니다.") in SideEffect { ... } within the same UserType.NONE branch and keep returning SettingForGuest(onLoginClick) as before so the side effect runs once per successful recomposition.android/app/src/main/java/com/on/turip/ui/compose/mypage/MyPageViewModel.kt (1)
76-77: 매직 넘버10, 0L을 상수로 추출하는 것을 권장합니다.페이지 사이즈와 초기 커서 값의 의미를 명확하게 하기 위해
companion object에 상수로 정의하면 가독성이 향상됩니다.♻️ 리팩토링 제안
companion object { private const val INVALID_FID = "FID_LOAD_FAIL" + private const val BOOKMARK_PAGE_SIZE = 10 + private const val BOOKMARK_INITIAL_CURSOR = 0L }- bookmarkRepository - .loadBookmarks(10, 0L) + bookmarkRepository + .loadBookmarks(BOOKMARK_PAGE_SIZE, BOOKMARK_INITIAL_CURSOR)🤖 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 76 - 77, The call in MyPageViewModel to bookmarkRepository.loadBookmarks(10, 0L) uses magic numbers; extract them into named constants (e.g., PAGE_SIZE and INITIAL_CURSOR) in MyPageViewModel's companion object and replace the literal 10 and 0L in the loadBookmarks call with those constants to improve readability and intent.android/app/src/main/java/com/on/turip/ui/compose/mypage/MyPageScreen.kt (1)
169-181: Preview에서hiltViewModel()호출로 인한 크래시 가능성.
MyPageScreenPreview에서viewModel파라미터를 전달하지 않으면 기본값인hiltViewModel()이 호출되는데, Preview 환경에는 Hilt가 없어 크래시가 발생합니다. 일반적으로 Screen composable의 preview는 상태를 직접 주입하거나, 내부 content composable을 별도로 추출하여 preview합니다.🤖 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/MyPageScreen.kt` around lines 169 - 181, MyPageScreenPreview currently lets MyPageScreen call hiltViewModel() (via the default viewModel param) which crashes in Preview; update the preview to supply a fake/test ViewModel instance or extract the inner stateless composable and preview that instead. Specifically, either (A) create and pass a simple preview ViewModel or mocked state into MyPageScreen's viewModel parameter in MyPageScreenPreview, or (B) refactor MyPageScreen to expose a MyPageContent (or similar) composable that accepts plain state/callbacks and use that in the preview while keeping MyPageScreen to obtain the real hiltViewModel() at runtime. Ensure references to MyPageScreenPreview, MyPageScreen and hiltViewModel() are updated accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@android/app/src/main/java/com/on/turip/domain/accounts/Role.kt`:
- Around line 11-14: The from(role: String) function in the Role enum silently
falls back to GUEST when an unknown role string is received; update it to log a
warning (including the raw received role and the normalized value) before
returning GUEST so API/contract mismatches are visible, and rename the local
variable (currently tag) to a clearer name like normalized or normalizedTag to
avoid confusion with the enum's it.tag; use the project logging facility (e.g.,
Log.w/Timber/etc.) and reference Role.from, Role.entries, and GUEST when making
the change.
In
`@android/app/src/main/java/com/on/turip/ui/compose/mypage/component/BookmarkedContentItem.kt`:
- Around line 162-171: The Icon inside BookmarkedContentItem.kt currently sets
contentDescription = null which hides meaning from screen readers; change it to
a localized description (e.g., use stringResource(R.string.remove_bookmark) or a
formatted string including item.title/ID) and pass that non-null value into the
Icon inside the IconButton that calls onRemoveBookmark(item.content.id), so
TalkBack reads a clear action label; use androidx.compose.ui.res.stringResource
and add the new string key (remove_bookmark) to resources.
In
`@android/app/src/main/java/com/on/turip/ui/compose/mypage/component/MyPageSettingsSection.kt`:
- Around line 47-60: AuthState.type is read as a non-observed `@Volatile` value so
Compose won't recompose when auth changes; refactor the MyPageSettingsSection
composable to accept userType: UserType as a hoisted parameter (pass the
ViewModel's StateFlow via collectAsStateWithLifecycle() from the caller) and
switch on that userType to choose SettingForMember/SettingForGuest, and move the
Timber.e("멤버가 지정되지 않았습니다.") into a SideEffect { Timber.e(...) } so the log is an
explicit side-effect rather than executed during composition.
In
`@android/app/src/main/java/com/on/turip/ui/compose/mypage/component/ProfileSection.kt`:
- Around line 128-134: The Loading branch in ProfileSection uses the incoming
modifier (which is weight(1f)) and then calls .size(2.dp), producing
weight(1f).size(2.dp) so the weight is ignored and the spinner is rendered at
2×2dp; change the CircularProgressIndicator to not chain the passed-in modifier:
replace the use of "modifier.size(2.dp)" with a fresh modifier (e.g.,
Modifier.size(24.dp) or another appropriate visible size, optionally combined
with align/center) so CircularProgressIndicator receives its own explicit size
instead of overriding the parent modifier; update the MyPageSectionState.Loading
branch in ProfileSection.kt to use that fresh Modifier for
CircularProgressIndicator.
In `@android/app/src/main/java/com/on/turip/ui/compose/mypage/MyPageViewModel.kt`:
- Around line 169-189: When server-side logout/withdraw succeeds but
userStorageRepository.clearTokens() fails, the app currently only logs the error
and leaves the user in a mismatched state; update confirmLogout() and
confirmWithdraw() so that after memberRepository.logout()/withdraw() succeeds
you always navigate to the login screen (send MyPageUiEffect.NavigateToLogin)
even if clearTokens() fails, and surface the failure (e.g., send an error UI
effect or log and optionally call handleError) so the user isn't left with stale
tokens; modify the clearTokens() .onSuccess/.onFailure branches in both
confirmLogout() and confirmWithdraw() to perform the navigation in the failure
branch and report the error instead of only Timber.e.
- Around line 213-233: handleError currently only matches UiError.Global, which
causes UiError.Feature cases (e.g., UiError.Feature.PermissionDenied, NotFound,
Duplicated, InValid) returned by toUiError() to be ignored; update handleError
to also handle UiError.Feature by adding an else/when branch that maps each
UiError.Feature case to an appropriate ErrorUiState (or new states if needed)
and send the corresponding _uiEffect (use MyPageUiEffect.ShowError with the
mapped ErrorUiState and the given retryAction or another suitable effect like
navigation), referencing handleError, toUiError, UiError.Feature, ErrorUiState,
MyPageUiEffect.ShowError, and MyPageRetryAction.
In `@android/app/src/main/java/com/on/turip/ui/mypage/MyPageActivity.kt`:
- Around line 38-46: The navigateToInquiry lambda in MyPageActivity.kt calls
startActivity(intent) directly which crashes if no email app exists; change it
to use the same safe start pattern as navigateToPrivacyPolicy (e.g., call
safeStartActivityWithToast(intent) or wrap startActivity in a try/catch catching
ActivityNotFoundException and show a toast) so the mailto Intent constructed
from InquiryMail.RECIPIENT / TITLE / mail.content is safely launched without
crashing.
In `@android/app/src/main/res/values/strings.xml`:
- Around line 50-51: The two string resources my_page_bookmark_content_title and
my_page_empty_bookmark_content do not match the PR screenshot copy; confirm the
final design copy and update those resource values to match exactly (including
punctuation and line breaks) so the UI matches the design; ensure
my_page_bookmark_content_title and my_page_empty_bookmark_content are updated
together and any localized variants are adjusted accordingly.
- Around line 48-67: The SettingItemPreview in SettingItem.kt references a
removed string key (R.string.setting_inquiry) causing a compile error; update
the preview to use the existing key R.string.my_page_inquiry so the preview
builds. Locate the `@Preview` composable SettingItemPreview (private fun
SettingItemPreview) and change the titleRes parameter passed into SettingItem
from R.string.setting_inquiry to R.string.my_page_inquiry; leave the rest of the
preview and the SettingItem composable unchanged.
---
Nitpick comments:
In `@android/app/src/main/java/com/on/turip/ui/compose/home/HomeScreen.kt`:
- Around line 173-180: Add a TODO comment above the temporary Text composable in
HomeScreen.kt indicating it must be removed after bottom navigation is
implemented; specifically reference the Text block that displays "임시 마이페이지 확인용
텍스트" and the onNavigateToMyPage clickable handler so future readers know this is
temporary and tied to the upcoming bottom navigation work (e.g., "// TODO:
Remove this temporary navigation text after implementing bottom navigation -
uses onNavigateToMyPage").
In `@android/app/src/main/java/com/on/turip/ui/compose/login/LoginScreen.kt`:
- Line 76: The dismissText in LoginScreen.kt for the guest data migration dialog
is using stringResource(R.string.my_page_logout_dialog_dismiss), which
semantically mismatches the migration context; add a new string resource (e.g.,
my_page_migration_dialog_dismiss) in strings.xml with the appropriate text
(e.g., "취소") and update the dismissText call in the migration dialog code to
stringResource(R.string.my_page_migration_dialog_dismiss) so the migration
dialog has its own dedicated key separate from logout strings.
In
`@android/app/src/main/java/com/on/turip/ui/compose/mypage/component/BookmarkedContentSection.kt`:
- Around line 192-198: The CircularProgressIndicator has an unnecessary
.align(Alignment.Center) because its parent Box already sets contentAlignment =
Alignment.Center; remove the redundant .align(Alignment.Center) from the
CircularProgressIndicator declaration in BookmarkedContentSection (the component
creating the Box and the CircularProgressIndicator) so the indicator is centered
by the Box alone.
In
`@android/app/src/main/java/com/on/turip/ui/compose/mypage/component/MyPageAppBar.kt`:
- Around line 23-29: Rename the mis-typed preview function MyPageAppBarPreView
to MyPageAppBarPreview to match the project's Preview naming convention; update
the function declaration and any internal references (the `@Preview/`@Composable
annotated function that calls MyPageAppBar within TuripTheme) so the symbol is
now MyPageAppBarPreview and the rest of the code remains unchanged.
- Around line 14-18: Replace the hardcoded "마이페이지" literal in the Text
composable inside MyPageAppBar (the Text with text = "마이페이지", textAlign =
TextAlign.Center, style = TuripTheme.typography.display) with a string resource
lookup (e.g. stringResource(R.string.my_page_title)); ensure you import and use
androidx.compose.ui.res.stringResource and update the Text's text parameter to
use that resource key (matching the my_page_* keys added to strings.xml).
In
`@android/app/src/main/java/com/on/turip/ui/compose/mypage/component/MyPageSettingItem.kt`:
- Line 65: Replace the hard-coded left alignment with a start-aligned value to
respect RTL/LTR layout direction: in MyPageSettingItem (where textAlign =
TextAlign.Left is set on the Text composable), change the alignment to
TextAlign.Start so the Text composable uses layout direction-aware alignment.
In
`@android/app/src/main/java/com/on/turip/ui/compose/mypage/component/MyPageSettingsSection.kt`:
- Around line 56-58: The Timber.e() call inside the UserType.NONE branch of the
MyPageSettingsSection composable should be moved into a SideEffect block to
avoid emitting logs during composition; wrap the existing Timber.e("멤버가 지정되지
않았습니다.") in SideEffect { ... } within the same UserType.NONE branch and keep
returning SettingForGuest(onLoginClick) as before so the side effect runs once
per successful recomposition.
In
`@android/app/src/main/java/com/on/turip/ui/compose/mypage/component/ProfileSection.kt`:
- Around line 140-149: The Text in the MyPageSectionState.Success branch reuses
the incoming modifier (modifier) which may include layout modifiers like
weight(1f) intended for the parent Box; remove passing the full modifier into
the Text and instead apply only the required modifiers (e.g., padding,
fillMaxWidth, or none) directly on the Text to avoid leaking parent layout
intentions. Update the Success branch in ProfileSection (the Text composable
inside is MyPageSectionState.Success) to stop forwarding the top-level modifier
and use a new, explicit modifier chain appropriate for the Text's needs.
In `@android/app/src/main/java/com/on/turip/ui/compose/mypage/MyPageScreen.kt`:
- Around line 169-181: MyPageScreenPreview currently lets MyPageScreen call
hiltViewModel() (via the default viewModel param) which crashes in Preview;
update the preview to supply a fake/test ViewModel instance or extract the inner
stateless composable and preview that instead. Specifically, either (A) create
and pass a simple preview ViewModel or mocked state into MyPageScreen's
viewModel parameter in MyPageScreenPreview, or (B) refactor MyPageScreen to
expose a MyPageContent (or similar) composable that accepts plain
state/callbacks and use that in the preview while keeping MyPageScreen to obtain
the real hiltViewModel() at runtime. Ensure references to MyPageScreenPreview,
MyPageScreen and hiltViewModel() are updated accordingly.
In `@android/app/src/main/java/com/on/turip/ui/compose/mypage/MyPageViewModel.kt`:
- Around line 76-77: The call in MyPageViewModel to
bookmarkRepository.loadBookmarks(10, 0L) uses magic numbers; extract them into
named constants (e.g., PAGE_SIZE and INITIAL_CURSOR) in MyPageViewModel's
companion object and replace the literal 10 and 0L in the loadBookmarks call
with those constants to improve readability and intent.
In `@android/app/src/main/java/com/on/turip/ui/main/home/HomeFragment.kt`:
- Around line 70-73: Temporary My Page entrypoint is left in HomeFragment
(navigateToMyPage lambda invoking MyPageActivity.newIntent) and needs tracked
cleanup; create a dedicated issue in the repo (describe that this is a temporary
bottom-nav placeholder and must be removed/reworked when bottom navigation is
implemented), then update the HomeFragment navigateToMyPage block to include a
concise TODO comment referencing the new issue ID (e.g., TODO: track removal in
ISSUE-123) so the temporary MyPageActivity.newIntent usage is clearly tracked
until the bottom-nav implementation replaces it.
Comment on lines
+11
to
+14
| fun from(role: String): Role { | ||
| val tag = role.uppercase() | ||
| return entries.firstOrNull { it.tag == tag } ?: GUEST | ||
| } |
There was a problem hiding this comment.
알 수 없는 역할 수신 시 경고 로그 누락
from()이 서버에서 알 수 없는 역할 문자열을 수신할 경우 아무런 경고 없이 GUEST로 폴백합니다. API 계약 위반이나 역할 추가 시 디버깅이 어려워질 수 있습니다.
🔍 제안 수정
companion object {
fun from(role: String): Role {
- val tag = role.uppercase()
- return entries.firstOrNull { it.tag == tag } ?: GUEST
+ val normalized = role.uppercase()
+ return entries.firstOrNull { it.tag == normalized } ?: run {
+ Timber.w("알 수 없는 역할 수신: $role — GUEST로 폴백합니다.")
+ GUEST
+ }
}
}추가로, 람다 내부에서 it.tag == tag의 tag는 지역 변수(line 12)를 참조하고, it.tag는 enum의 private 필드를 참조합니다. 이름이 동일해 혼란스러울 수 있으므로 지역 변수명을 normalized처럼 구분해 주는 것이 가독성에 좋습니다.
🤖 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/domain/accounts/Role.kt` around lines
11 - 14, The from(role: String) function in the Role enum silently falls back to
GUEST when an unknown role string is received; update it to log a warning
(including the raw received role and the normalized value) before returning
GUEST so API/contract mismatches are visible, and rename the local variable
(currently tag) to a clearer name like normalized or normalizedTag to avoid
confusion with the enum's it.tag; use the project logging facility (e.g.,
Log.w/Timber/etc.) and reference Role.from, Role.entries, and GUEST when making
the change.
Comment on lines
+162
to
+171
| IconButton( | ||
| onClick = { onRemoveBookmark(item.content.id) }, | ||
| modifier = Modifier.size(48.dp), | ||
| ) { | ||
| Icon( | ||
| painter = painterResource(R.drawable.btn_bookmark_selected), | ||
| contentDescription = null, | ||
| tint = TuripTheme.colors.primary, | ||
| ) | ||
| } |
There was a problem hiding this comment.
북마크 제거 버튼에 접근성 설명(contentDescription)이 누락되었습니다.
Icon의 contentDescription = null 설정은 순수 장식 요소에만 사용해야 합니다. 북마크 제거 버튼은 사용자 인터랙션을 유발하는 능동적 액션이므로, 공식 Android 문서에서는 IconButton 내 Icon에 반드시 지역화된 contentDescription을 제공해야 함을 보여줍니다. contentDescription = null인 경우, TalkBack은 단순히 "button"만 읽어 시각장애 사용자에게 아무 도움이 되지 않습니다.
🛡️ 제안 수정
IconButton(
onClick = { onRemoveBookmark(item.content.id) },
modifier = Modifier.size(48.dp),
) {
Icon(
painter = painterResource(R.drawable.btn_bookmark_selected),
- contentDescription = null,
+ contentDescription = stringResource(R.string.my_page_remove_bookmark),
tint = TuripTheme.colors.primary,
)
}🤖 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 162 - 171, The Icon inside BookmarkedContentItem.kt currently sets
contentDescription = null which hides meaning from screen readers; change it to
a localized description (e.g., use stringResource(R.string.remove_bookmark) or a
formatted string including item.title/ID) and pass that non-null value into the
Icon inside the IconButton that calls onRemoveBookmark(item.content.id), so
TalkBack reads a clear action label; use androidx.compose.ui.res.stringResource
and add the new string key (remove_bookmark) to resources.
Comment on lines
+47
to
+60
| when (AuthState.type) { | ||
| UserType.MEMBER -> { | ||
| SettingForMember(onLogoutClick, onWithdrawClick) | ||
| } | ||
|
|
||
| UserType.GUEST -> { | ||
| SettingForGuest(onLoginClick) | ||
| } | ||
|
|
||
| UserType.NONE -> { | ||
| Timber.e("멤버가 지정되지 않았습니다.") | ||
| SettingForGuest(onLoginClick) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "*.kt" | xargs grep -l "class AuthState\|object AuthState" | head -5Repository: woowacourse-teams/2025-Turip
Length of output: 132
🏁 Script executed:
find . -type f -name "MyPageSettingsSection.kt" | head -1Repository: woowacourse-teams/2025-Turip
Length of output: 165
🏁 Script executed:
find . -path "*/common/*" -name "*.kt" | grep -i auth | head -10Repository: woowacourse-teams/2025-Turip
Length of output: 132
🏁 Script executed:
cat -n ./android/app/src/main/java/com/on/turip/common/AuthState.ktRepository: woowacourse-teams/2025-Turip
Length of output: 361
🏁 Script executed:
head -100 ./android/app/src/main/java/com/on/turip/ui/compose/mypage/component/MyPageSettingsSection.kt | cat -nRepository: woowacourse-teams/2025-Turip
Length of output: 3694
AuthState.type을 Compose 비관찰 상태로 읽으면 재구성이 트리거되지 않습니다
AuthState.type이 @Volatile var로 정의되어 있어 Compose의 스냅샷 시스템에 의해 관찰되지 않습니다. @Volatile은 Java의 스레드 가시성을 보장하지만 Compose 재구성을 트리거하지 않으므로, 로그인/로그아웃 후에도 이 컴포저블은 자동으로 재구성되지 않아 잘못된 메뉴(게스트/멤버)가 계속 표시될 수 있습니다.
권장 수정 방향: userType: UserType을 파라미터로 올려서(state hoisting), ViewModel의 StateFlow에서 collectAsStateWithLifecycle()을 통해 전달받도록 변경하세요.
🛠️ 권장 수정안
`@Composable`
fun MyPageSettingsSection(
onInquiryClick: () -> Unit,
onPrivacyPolicyClick: () -> Unit,
onLoginClick: () -> Unit,
onLogoutClick: () -> Unit,
onWithdrawClick: () -> Unit,
+ userType: UserType,
modifier: Modifier = Modifier,
) {
...
- when (AuthState.type) {
+ when (userType) {추가로 라인 57의 Timber.e()는 컴포지션 내에서 직접 실행되는 사이드 이펙트입니다. SideEffect {} 블록으로 감싸서 명시적으로 관리하는 것이 권장됩니다.
🤖 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 - 60, AuthState.type is read as a non-observed `@Volatile` value
so Compose won't recompose when auth changes; refactor the MyPageSettingsSection
composable to accept userType: UserType as a hoisted parameter (pass the
ViewModel's StateFlow via collectAsStateWithLifecycle() from the caller) and
switch on that userType to choose SettingForMember/SettingForGuest, and move the
Timber.e("멤버가 지정되지 않았습니다.") into a SideEffect { Timber.e(...) } so the log is an
explicit side-effect rather than executed during composition.
Comment on lines
128
to
134
| when (state) { | ||
| MyPageSectionState.Loading -> { | ||
| CircularProgressIndicator( | ||
| modifier = modifier.size(2.dp), | ||
| color = TuripTheme.colors.gray03, | ||
| ) | ||
| } |
There was a problem hiding this comment.
CircularProgressIndicator가 2.dp 크기로 사실상 보이지 않습니다 — 주요 UI 버그
modifier 파라미터는 호출 측(ProfileSection)에서 Modifier.weight(1f)로 전달됩니다. 이를 그대로 받아 .size(2.dp)를 추가하면 최종 Modifier 체인은 weight(1f).size(2.dp)가 되어, Box 내부에서 weight는 무시되고 size(2.dp)만 적용됩니다. 결과적으로 스피너가 2×2dp — 사실상 투명하게 렌더링되어 사용자에게 로딩 피드백이 전달되지 않습니다.
🐛 제안 수정
MyPageSectionState.Loading -> {
CircularProgressIndicator(
- modifier = modifier.size(2.dp),
+ modifier = Modifier.size(24.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/ProfileSection.kt`
around lines 128 - 134, The Loading branch in ProfileSection uses the incoming
modifier (which is weight(1f)) and then calls .size(2.dp), producing
weight(1f).size(2.dp) so the weight is ignored and the spinner is rendered at
2×2dp; change the CircularProgressIndicator to not chain the passed-in modifier:
replace the use of "modifier.size(2.dp)" with a fresh modifier (e.g.,
Modifier.size(24.dp) or another appropriate visible size, optionally combined
with align/center) so CircularProgressIndicator receives its own explicit size
instead of overriding the parent modifier; update the MyPageSectionState.Loading
branch in ProfileSection.kt to use that fresh Modifier for
CircularProgressIndicator.
Contributor
There was a problem hiding this comment.
c: 여기는 ProgressBar가 없어도 될 거 같은데 어떻게 생각하시나요?
There was a problem hiding this comment.
@jerry8282 동의합니다! 프로필 로딩은 일반적으로 빠르게 완료되고, 특히 API 응답이 빠른 경우 Loading 상태의 CircularProgressIndicator가 화면에 거의 나타나지 않아 사용자에게 큰 의미가 없을 수 있습니다.
또한 위에서 CodeRabbit이 지적한 것처럼 현재 구현은 modifier.size(2.dp)로 인해 스피너가 사실상 보이지 않는 버그도 있습니다.
제안:
- Loading 상태에서는 빈 공간을 유지하거나, 닉네임 영역에 간단한 플레이스홀더(예: 회색 텍스트 "...")를 표시하는 것이 더 깔끔할 것 같습니다.
- Error 상태의 재시도 버튼만 남겨두고 Loading UI는 제거하는 것도 좋은 선택입니다.
프로덕트 UX를 더 잘 아시니 최종 판단은 맡기겠습니다! 👍
Contributor
Author
There was a problem hiding this comment.
c: 여기는 ProgressBar가 없어도 될 거 같은데 어떻게 생각하시나요?
넵, 좋아요!
Section 별로 에러, 로딩, 정상 화면에 대해 추상화 되어 있던 상태였고,
추후에 Loading 상황에 어떤 UI를 보여주게 될 수도 있을까 싶어서 빈 블록으로 남겨두었는데 완전히 제거하는 걸 원하시면 다시 반영하겠습니다!
반영 커밋 : afee69e
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!
Contributor
There was a problem hiding this comment.
빈 블록으로 남겨두는 게 좋은 거 같습니당👍
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/compose/mypage/MyPageViewModel.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.
Actionable comments posted: 2
🧹 Nitpick comments (3)
android/app/src/main/java/com/on/turip/ui/main/favorite/MyPageFragment.kt (1)
61-65:ivMyPageMoreOptions클릭 리스너는 현재 사실상 dead code입니다.로그 메시지 자체가 "제거 해도 되는 부분" 임을 명시하고 있습니다.
Timber.d는 릴리스 빌드에서 no-op이 되므로, 사용자 입장에서 이 버튼은 아무 동작도 하지 않는 상태입니다. 바텀 네비게이션 마이그레이션이 완료된 이후 이 Fragment 자체가 제거될 예정이지만, 그 전에 클릭 리스너 전체를 제거하거나 추적 티켓을 참조하는 TODO 주석으로 대체하는 것을 권장합니다.♻️ 클릭 리스너 제거 제안
private fun setupListeners() { - binding.ivMyPageMoreOptions.setOnClickListener { - Timber.d("마이 페이지 컴포즈로 마이그레이션 됨 제거 해도 되는 부분") - } + // TODO: 바텀 네비게이션 작업(`#xxx`) 완료 후 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/favorite/MyPageFragment.kt` around lines 61 - 65, The click listener on ivMyPageMoreOptions inside setupListeners in MyPageFragment is dead code (only logs “제거 해도 되는 부분” via Timber.d); remove the setOnClickListener block entirely or replace it with a concise TODO comment referencing the migration ticket (e.g., TODO: handle more options after nav migration - TICKET-123) so the UI button is not left with a no-op handler; make the change in MyPageFragment by deleting the binding.ivMyPageMoreOptions.setOnClickListener { ... } or swapping it for the single-line TODO comment.android/app/src/main/java/com/on/turip/ui/compose/mypage/component/ProfileSection.kt (1)
139-147: Success 상태의Text에weightmodifier를 직접 전달하는 것은 불필요합니다.
Box내부에서weight(1f)는Row/Column컨텍스트에서만 유효하므로 여기서는 no-op입니다. 이미Box자체에 해당 modifier가 적용되어 있으므로,Text에는 독립적인Modifier를 사용하는 것이 명확합니다.♻️ 제안 수정
is MyPageSectionState.Success -> { Text( text = state.data.nickname, textAlign = TextAlign.Start, style = TuripTheme.typography.title1, maxLines = 2, overflow = TextOverflow.Ellipsis, - modifier = modifier, + modifier = Modifier.fillMaxWidth(), ) }🤖 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/ProfileSection.kt` around lines 139 - 147, The Text inside the MyPageSectionState.Success branch is receiving a weight modifier that is a no-op inside a Box; remove any .weight(1f) usage from the Text's modifier and instead pass a plain Modifier (or the existing modifier without weight) so layout intent is clear—update the Text call in the MyPageSectionState.Success block (the Text composable showing state.data.nickname) and rely on the Box's modifier for weight handling.android/app/src/main/java/com/on/turip/ui/compose/mypage/MyPageViewModel.kt (1)
76-76:loadBookmarks(10, 0L)매직 넘버 — 상수 추출 권장.페이지 크기
10과 오프셋0L을 인라인으로 사용하는 것은 의도를 파악하기 어렵습니다.♻️ 제안 수정
+ companion object { + private const val INVALID_FID = "FID_LOAD_FAIL" + private const val BOOKMARK_PREVIEW_SIZE = 10 + private const val BOOKMARK_INITIAL_OFFSET = 0L + } - bookmarkRepository.loadBookmarks(10, 0L) + bookmarkRepository.loadBookmarks(BOOKMARK_PREVIEW_SIZE, BOOKMARK_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` at line 76, The call .loadBookmarks(10, 0L) uses magic numbers; extract them into named constants (e.g., BOOKMARK_PAGE_SIZE = 10 and BOOKMARK_DEFAULT_OFFSET = 0L) and replace the literals with those constants to clarify intent; define the constants either in the MyPageViewModel companion object or as top-level constants and use them where loadBookmarks is invoked so the purpose of the values is explicit.
🤖 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/mypage/MyPageScreen.kt`:
- Around line 114-122: Scaffold is applying system bar insets via its
innerPadding while you also add Modifier.systemBarsPadding(), causing double top
inset; fix by either removing Modifier.systemBarsPadding() from the Scaffold
modifier OR override the Scaffold's insets so it doesn't add them (set
contentWindowInsets = WindowInsets() instead of using
ScaffoldDefaults.contentWindowInsets) and keep using innerPadding with your
LazyColumn Modifier.padding(innerPadding); update references in MyPageScreen
where Scaffold, Modifier.systemBarsPadding(), innerPadding, and the LazyColumn
padding are used.
In `@android/app/src/main/java/com/on/turip/ui/compose/mypage/MyPageViewModel.kt`:
- Around line 53-87: Both loadProfile() and loadBookmarkContents() ignore
specific error types (like TokenExpired) and only set MyPageSectionState.Error,
preventing TokenExpired from triggering navigation to login; update each
onFailure to receive the error (e.g., onFailure { error: ErrorType -> ... }),
call handleError(error, /* pass appropriate emit or scope to trigger
NavigateToLogin as used by confirmLogout()/confirmWithdraw() */) and still
update the section state (or set to Error) as needed so TokenExpired flows to
the same NavigateToLogin path; reference loadProfile, loadBookmarkContents,
handleError, confirmLogout, confirmWithdraw, TokenExpired, NavigateToLogin, and
MyPageSectionState when making the change.
---
Duplicate comments:
In
`@android/app/src/main/java/com/on/turip/ui/compose/mypage/component/ProfileSection.kt`:
- Around line 128-133: The spinner is being forced to 2×2dp because the code
uses modifier.size(2.dp) while the caller passes Modifier.weight(1f); change the
sizing to compose the passed modifier with a reasonable spinner size instead of
overriding it — replace modifier.size(2.dp) in the MyPageSectionState.Loading
branch's CircularProgressIndicator with modifier.then(Modifier.size(24.dp)) (or
another suitable size) so the caller's modifiers (e.g., Modifier.weight(1f)) are
preserved and the spinner is visible.
In `@android/app/src/main/java/com/on/turip/ui/compose/mypage/MyPageViewModel.kt`:
- Around line 212-232: handleError currently only handles UiError.Global and
drops UiError.Feature (e.g., UiError.Feature.PermissionDenied) returned by
errorType.toUiError(); update handleError (the suspend function handleError in
MyPageViewModel) to also branch on UiError.Feature and map each Feature case to
an appropriate UI effect/state (use _uiEffect.send with existing MyPageUiEffect
variants or create a new MyPageUiEffect.ShowError with a new
ErrorUiState.Feature/PermissionDenied as needed), ensuring PermissionDenied and
other Feature cases produce visible feedback instead of being ignored.
- Around line 168-210: After successful server logout/delete (in confirmLogout
and confirmWithdraw) ensure the app still navigates to the login screen even if
userStorageRepository.clearTokens() fails to avoid server/local state mismatch:
after calling clearTokens(), always send
_uiEffect.send(MyPageUiEffect.NavigateToLogin) regardless of clearTokens
onSuccess/onFailure, and in the onFailure branch log the error and surface it
(e.g., call handleError or send a dedicated UI effect) so the failure is
recorded but the user is redirected; update the code paths around
memberRepository.logout(), memberRepository.deleteMember(), and
userStorageRepository.clearTokens() to implement this flow.
In `@android/app/src/main/java/com/on/turip/ui/mypage/MyPageActivity.kt`:
- Around line 38-46: The navigateToInquiry lambda currently launches the mail
intent with startActivity(intent) which can crash on devices without an email
app; change it to use the existing safeStartActivityWithToast helper (or perform
a resolveActivity check) when launching the Intent created in navigateToInquiry
(the Intent with ACTION_SENDTO and the mailto Uri built from
InquiryMail.RECIPIENT/TITLE/mail.content) so the call uses
safeStartActivityWithToast(intent) or shows a toast instead of throwing
ActivityNotFoundException.
---
Nitpick comments:
In
`@android/app/src/main/java/com/on/turip/ui/compose/mypage/component/ProfileSection.kt`:
- Around line 139-147: The Text inside the MyPageSectionState.Success branch is
receiving a weight modifier that is a no-op inside a Box; remove any .weight(1f)
usage from the Text's modifier and instead pass a plain Modifier (or the
existing modifier without weight) so layout intent is clear—update the Text call
in the MyPageSectionState.Success block (the Text composable showing
state.data.nickname) and rely on the Box's modifier for weight handling.
In `@android/app/src/main/java/com/on/turip/ui/compose/mypage/MyPageViewModel.kt`:
- Line 76: The call .loadBookmarks(10, 0L) uses magic numbers; extract them into
named constants (e.g., BOOKMARK_PAGE_SIZE = 10 and BOOKMARK_DEFAULT_OFFSET = 0L)
and replace the literals with those constants to clarify intent; define the
constants either in the MyPageViewModel companion object or as top-level
constants and use them where loadBookmarks is invoked so the purpose of the
values is explicit.
In `@android/app/src/main/java/com/on/turip/ui/main/favorite/MyPageFragment.kt`:
- Around line 61-65: The click listener on ivMyPageMoreOptions inside
setupListeners in MyPageFragment is dead code (only logs “제거 해도 되는 부분” via
Timber.d); remove the setOnClickListener block entirely or replace it with a
concise TODO comment referencing the migration ticket (e.g., TODO: handle more
options after nav migration - TICKET-123) so the UI button is not left with a
no-op handler; make the change in MyPageFragment by deleting the
binding.ivMyPageMoreOptions.setOnClickListener { ... } or swapping it for the
single-line TODO comment.
android/app/src/main/java/com/on/turip/ui/compose/mypage/MyPageScreen.kt
Show resolved
Hide resolved
Comment on lines
53
to
87
| fun loadProfile() { | ||
| viewModelScope.launch { | ||
| _uiState.update { it.copy(profileState = MyPageSectionState.Loading) } | ||
|
|
||
| accountRepository | ||
| .loadMyProfile() | ||
| .onSuccess { result: Account -> | ||
| _uiState.update { | ||
| Timber.d("마이페이지 프로필 조회 성공") | ||
| it.copy(profileState = MyPageSectionState.Success(result.toUiModel())) | ||
| } | ||
| }.onFailure { | ||
| Timber.e("마이페이지 프로필 조회 에러 발생") | ||
| _uiState.update { it.copy(profileState = MyPageSectionState.Error) } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| fun loadBookmarkContents() { | ||
| viewModelScope.launch { | ||
| _uiState.update { it.copy(bookmarkContentState = MyPageSectionState.Loading) } | ||
|
|
||
| bookmarkRepository | ||
| .loadBookmarks(10, 0L) | ||
| .onSuccess { result: PagedBookmarkContents -> | ||
| Timber.d("마이페이지 북마크 목록 조회 성공") | ||
| _uiState.update { | ||
| it.copy(bookmarkContentState = MyPageSectionState.Success(result.bookmarkContents.toImmutableList())) | ||
| } | ||
| }.onFailure { | ||
| Timber.e("마이페이지 북마크 목록 조회 에러 발생") | ||
| _uiState.update { it.copy(bookmarkContentState = MyPageSectionState.Error) } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
loadProfile()·loadBookmarkContents()에서 TokenExpired 에러를 처리하지 않아 사용자가 로그인 화면으로 이동하지 못합니다.
confirmLogout()과 confirmWithdraw()는 onFailure { errorType: ErrorType -> handleError(errorType, ...) } 패턴으로 토큰 만료 시 NavigateToLogin을 emit합니다. 반면 두 로딩 함수는 오류 타입을 무시하고 MyPageSectionState.Error만 설정합니다. 토큰이 만료된 경우 재시도 버튼을 눌러도 계속 실패하며 사용자가 로그인 화면으로 이동할 방법이 없습니다.
🐛 수정 예시 (loadProfile 기준)
- }.onFailure {
+ }.onFailure { errorType: ErrorType ->
Timber.e("마이페이지 프로필 조회 에러 발생")
_uiState.update { it.copy(profileState = MyPageSectionState.Error) }
+ if (errorType == ErrorType.Auth) {
+ _uiEffect.send(MyPageUiEffect.NavigateToLogin)
+ }
}loadBookmarkContents()에도 동일한 패턴을 적용해 주세요.
🤖 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 53 - 87, Both loadProfile() and loadBookmarkContents() ignore
specific error types (like TokenExpired) and only set MyPageSectionState.Error,
preventing TokenExpired from triggering navigation to login; update each
onFailure to receive the error (e.g., onFailure { error: ErrorType -> ... }),
call handleError(error, /* pass appropriate emit or scope to trigger
NavigateToLogin as used by confirmLogout()/confirmWithdraw() */) and still
update the section state (or set to Error) as needed so TokenExpired flows to
the same NavigateToLogin path; reference loadProfile, loadBookmarkContents,
handleError, confirmLogout, confirmWithdraw, TokenExpired, NavigateToLogin, and
MyPageSectionState when making the change.
Contributor
There was a problem hiding this comment.
c: 이거 해결하면 좋을 거 같아요😊 토큰 만료시에 로그인 화면으로 이동하게 하면 좋을 거 같아요
근데 생각해보면 마이페이지 화면으로 가려면 홈화면을 거치면서 이미 한번 refresh토큰을 재발급하게 되는데 꼭 필요한 로직인지는 잘 모르겠는데
채넛 생각으로 명시하면 좋을 거 같다고 판단되면 추가하면 좋을 거 같아요😊
There was a problem hiding this comment.
@jerry8282, 좋은 지적이에요! 홈 화면을 거치면서 이미 토큰 갱신이 이루어지는 점은 맞습니다.
다만 다음과 같은 엣지 케이스를 고려하면 방어적으로 처리하는 게 좋을 것 같아요:
- 앱이 백그라운드에 오래 있다가 마이페이지로 직접 복귀하는 경우 - 홈 화면을 거치지 않고 마이페이지가 복원될 수 있습니다
- 토큰 갱신이 실패한 후 마이페이지 API가 호출되는 경우 - 홈에서의 갱신 실패 후에도 화면 전환이 일어날 수 있습니다
- 일관성 -
confirmLogout()과confirmWithdraw()는 이미ErrorType.Auth처리를 하고 있어서, 같은 패턴을 적용하면 코드의 일관성이 높아집니다
또한 기존 학습 규칙에 따르면 ErrorType.Auth인 경우 토큰을 삭제하고 GUEST로 전환하는 패턴이 정립되어 있어요. 이 패턴을 모든 API 호출에서 일관되게 적용하는 게 안전할 것 같습니다.
@yrsel 최종 결정은 채넛께 맡기지만, 일관성과 방어적 프로그래밍 측면에서 추가하는 걸 추천드려요! 😊
🧠 Learnings used
Learnt from: yrsel
Repo: woowacourse-teams/2025-Turip PR: 593
File: android/app/src/main/java/com/on/turip/di/NetworkModule.kt:128-132
Timestamp: 2026-02-18T03:12:48.417Z
Learning: 토큰 갱신(refresh) 실패 시 `ErrorType.Auth`인 경우에만 저장된 토큰을 삭제하고 사용자 상태를 GUEST로 전환해야 합니다. `ErrorType.Network` 같은 일시적 오류에서는 토큰을 보존해야 합니다.
Contributor
Author
There was a problem hiding this comment.
백그라운드에서 오래 있다가 진입하는 경우를 생각해서 처리하는 게 좋긴 하지만, 현재 Ktor에서 401을 먼저 가로채서 처리하다보니 에러가 내려오질 않아 홈으로 이동 시키는 로직이 실행되지 않는 문제가 있어요!
이번에 Navigation을 적용하면서 제리가 언급했던 이벤트 버스를 여기에 적용해 단일 Activity에서 Login으로 이동시키는 로직을 추가하는 게 더 좋을 것 같아보여요!
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!
2 tasks
jerry8282
requested changes
Feb 25, 2026
Contributor
jerry8282
left a comment
jerry8282
left a comment
There was a problem hiding this comment.
고생하셨습니다 채넛!! 리뷰가 늦어져서 죄송합니다.😭 너무 구현을 잘해주셨어요 역채넛👍
몇가지 수정사항과 궁금한 부분들을 남겨두어서 확인 한번 해주시면 감사하겠습니다
Comment on lines
67
to
116
| @Composable | ||
| private fun ProfileImage( | ||
| imageUrl: String?, | ||
| modifier: Modifier = Modifier, | ||
| ) { | ||
| val shape = CircleShape | ||
|
|
||
| Box( | ||
| modifier = | ||
| modifier | ||
| .aspectRatio(1f) | ||
| .shadow( | ||
| elevation = 14.dp, | ||
| shape = shape, | ||
| ambientColor = TuripTheme.colors.black, | ||
| spotColor = TuripTheme.colors.black, | ||
| ).clip(shape) | ||
| .background(TuripTheme.colors.white) | ||
| .border( | ||
| width = 2.dp, | ||
| color = TuripTheme.colors.gray01, | ||
| shape = shape, | ||
| ), | ||
| contentAlignment = Alignment.Center, | ||
| ) { | ||
| if (imageUrl.isNullOrBlank()) { | ||
| Icon( | ||
| painter = painterResource(R.drawable.ic_profile_default), | ||
| contentDescription = stringResource(R.string.my_page_profile_image_description), | ||
| tint = Color.Unspecified, | ||
| modifier = Modifier.fillMaxSize(0.4f), | ||
| ) | ||
| } else { | ||
| AsyncImage( | ||
| model = | ||
| ImageRequest | ||
| .Builder(LocalContext.current) | ||
| .data(imageUrl) | ||
| .crossfade(true) | ||
| .build(), | ||
| contentDescription = stringResource(R.string.my_page_profile_image_description), | ||
| contentScale = ContentScale.Crop, | ||
| modifier = Modifier.matchParentSize(), | ||
| placeholder = painterResource(R.drawable.bg_image_placeholder), | ||
| error = painterResource(R.drawable.ic_sorry), | ||
| ) | ||
| } | ||
| } | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
@Composable
private fun ProfileImage(
imageUrl: String?,
isError: Boolean,
onRetry: () -> Unit,
modifier: Modifier = Modifier,
) {
val shape = CircleShape
Box(modifier = modifier) {
Box(
modifier =
Modifier
.aspectRatio(1f)
.shadow(
elevation = 14.dp,
shape = shape,
ambientColor = TuripTheme.colors.black,
spotColor = TuripTheme.colors.black,
).clip(shape)
.background(TuripTheme.colors.white)
.border(
width = 2.dp,
color = TuripTheme.colors.gray01,
shape = shape,
),
contentAlignment = Alignment.Center,
) {
if (imageUrl.isNullOrBlank()) {
Icon(
painter = painterResource(R.drawable.ic_profile_default),
contentDescription = stringResource(R.string.my_page_profile_image_description),
tint = Color.Unspecified,
modifier = Modifier.fillMaxSize(0.4f),
)
} else {
AsyncImage(
model =
ImageRequest
.Builder(LocalContext.current)
.data(imageUrl)
.crossfade(true)
.build(),
contentDescription = stringResource(R.string.my_page_profile_image_description),
contentScale = ContentScale.Crop,
modifier = Modifier.matchParentSize(),
placeholder = painterResource(R.drawable.bg_image_placeholder),
error = painterResource(R.drawable.ic_sorry),
)
}
}
if (isError) {
Box(
modifier =
Modifier
.align(Alignment.BottomEnd)
.size(24.dp)
.shadow(elevation = 4.dp, shape = CircleShape)
.clip(CircleShape)
.background(TuripTheme.colors.white)
.border(
width = 1.dp,
color = TuripTheme.colors.gray03,
shape = CircleShape,
).clickable(onClick = onRetry),
contentAlignment = Alignment.Center,
) {
Icon(
imageVector = Icons.Rounded.Refresh,
contentDescription = stringResource(R.string.retry),
tint = TuripTheme.colors.gray03,
modifier = Modifier.size(14.dp),
)
}
}
}
}
c: 다음과 같이 profile이미지 우측하단에 조그맣게 로딩버튼을 두는 게 제 생각에는 더 좋은 UI같은데 채넛의 생각은 어떠신가요?😊
그리고 현재는 Icons.Default에 있는 걸 사용했지만
위에 이미지 같은 로딩 버튼과 함께 색깔은 Primary색깔로 하면 더 좋을 거 같아요
Contributor
Author
There was a problem hiding this comment.
제리가 제안해준 방식 UI가 깔끔하고 좋네요! 👍
다만, 현재 저희는 프로필 이미지를 지원하지 않아 항상 기본 아이콘으로만 보이게 되는데 이미지 하단에 재시도 버튼을 클릭 으로 닉네임만 갱신되는 흐름 괜찮을까요 ??
백엔드 팀원들에게도 의견 물어보면 좋을 것 같아요!
Contributor
There was a problem hiding this comment.
넵 백엔드 팀원들에게도 물어보고 반영하면 좋을 거 같아요👍
Comment on lines
128
to
134
| when (state) { | ||
| MyPageSectionState.Loading -> { | ||
| CircularProgressIndicator( | ||
| modifier = modifier.size(2.dp), | ||
| color = TuripTheme.colors.gray03, | ||
| ) | ||
| } |
Contributor
There was a problem hiding this comment.
c: 여기는 ProgressBar가 없어도 될 거 같은데 어떻게 생각하시나요?
| data class MyPageUiState( | ||
| val profileState: MyPageSectionState<ProfileModel>, | ||
| val bookmarkContentState: MyPageSectionState<ImmutableList<BookmarkContent>>, | ||
| val dialogState: MyPageDialogState?, |
Contributor
There was a problem hiding this comment.
c: 현재 dialogState의 책임이 여러개인 거 같아요
- 현재 dialog가 떠있는지 안떠있는지
- 현재 dialog의 상태가 어떤 상태인지
위처럼 되면서 sealed interface를 씀에도 null -> {} 이 생기는 게 어색한 거 같은데
dialogState: Boolean,
dialogMode: MyPAgeDialogState,
이렇게 하는 건 어떻게 생각하시나요?😊
Contributor
Author
There was a problem hiding this comment.
저는 DialogState에서 null 값이 Dialog가 보이지 않는 상태를 표현한다고 생각해서 2번의 역할만 한다고 생각했어요!
dialogState와 dialogMode로 구분하게 된다면 uiState의 유휴 상태의 값을 정의하기 위해 MyPageDialogState에 None 또는 Idle 과 같은 상태가 추가적으로 필요하게 되고 그렇게되면 null을 사용하는 방식과 동일하게 되어버릴 것 같아요
null 표현이 별로라면
uiState.dialogState?.let { state ->
when(state) {
MyPageDialogState.ConfirmWithdraw -> TODO()
MyPageDialogState.LogoutRequired -> TODO()
}
}이런 형태로 사용할 수 있을 것 같아요!
아니면 showXXXDialog:Boolean 변수로 관리하는 방식이 더 괜찮을까요 ?!
Contributor
There was a problem hiding this comment.
저는 그동안 Dialog 상태를 showXXXDialog: Boolean 형태로 관리해와서, 지금 구조가 조금 낯설게 느껴졌던 것 같아요.🥲
위 코드도 결국 state = null이라는 하나의 상태를 표현하고 있는 것처럼 보이는데, 개인적으로는 null로 상태를 표현하는 것보다는 Boolean으로 명확하게 드러내는 방식이 더 직관적이고 이해하기 쉬운 것 같다는 생각이 들었습니다.😊
| .build(), | ||
| contentDescription = contentDescription, | ||
| contentScale = ContentScale.Crop, | ||
| modifier = Modifier.matchParentSize(), |
Contributor
There was a problem hiding this comment.
Suggested change
| modifier = Modifier.matchParentSize(), | |
| modifier = Modifier.fillMaxSize(), |
c: fillMaxSize를 하는 것과 차이가 없는 거 같은데 무슨 차이가 있나유?
Contributor
Author
There was a problem hiding this comment.
현재 코드에서 차이가 없어요, XML 방식에서의 matchParent를 생각하면 될 것 같아요!
Modifier 속성을 구경하다가 설정했던 것 같네요 😅
공식문서 설명
공식문서에서 차이가 발생하는 경우를 설명해주고 있네요!
Comment on lines
53
to
58
| Column( | ||
| modifier = | ||
| modifier | ||
| .padding(TuripTheme.spacing.extraSmall) | ||
| .clip(TuripTheme.shape.container) | ||
| .clickable { onContentClick(item.content.id) }, |
Contributor
There was a problem hiding this comment.
Suggested change
| Column( | |
| modifier = | |
| modifier | |
| .padding(TuripTheme.spacing.extraSmall) | |
| .clip(TuripTheme.shape.container) | |
| .clickable { onContentClick(item.content.id) }, | |
| Column( | |
| modifier | |
| .border(1.dp, TuripTheme.colors.gray01, TuripTheme.shape.container) | |
| .padding(TuripTheme.spacing.extraSmall) | |
| .clip(TuripTheme.shape.container) | |
| .clickable { onContentClick(item.content.id) }, |
c: 경계가 없어 좀 어색한 느낌이 드는데 border로 살짝의 경계를 주는 건 어떤가요?
Comment on lines
197
to
204
| userStorageRepository | ||
| .clearTokens() | ||
| .onSuccess { | ||
| _uiEffect.send(MyPageUiEffect.NavigateToLogin) | ||
| Timber.d("회원탈퇴 성공") | ||
| }.onFailure { | ||
| Timber.e("토큰 초기화 실패") | ||
| } |
Contributor
There was a problem hiding this comment.
r: 이거 메서드로 따로 빼면 좋을 거 같아요😊
Comment on lines
53
to
87
| fun loadProfile() { | ||
| viewModelScope.launch { | ||
| _uiState.update { it.copy(profileState = MyPageSectionState.Loading) } | ||
|
|
||
| accountRepository | ||
| .loadMyProfile() | ||
| .onSuccess { result: Account -> | ||
| _uiState.update { | ||
| Timber.d("마이페이지 프로필 조회 성공") | ||
| it.copy(profileState = MyPageSectionState.Success(result.toUiModel())) | ||
| } | ||
| }.onFailure { | ||
| Timber.e("마이페이지 프로필 조회 에러 발생") | ||
| _uiState.update { it.copy(profileState = MyPageSectionState.Error) } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| fun loadBookmarkContents() { | ||
| viewModelScope.launch { | ||
| _uiState.update { it.copy(bookmarkContentState = MyPageSectionState.Loading) } | ||
|
|
||
| bookmarkRepository | ||
| .loadBookmarks(10, 0L) | ||
| .onSuccess { result: PagedBookmarkContents -> | ||
| Timber.d("마이페이지 북마크 목록 조회 성공") | ||
| _uiState.update { | ||
| it.copy(bookmarkContentState = MyPageSectionState.Success(result.bookmarkContents.toImmutableList())) | ||
| } | ||
| }.onFailure { | ||
| Timber.e("마이페이지 북마크 목록 조회 에러 발생") | ||
| _uiState.update { it.copy(bookmarkContentState = MyPageSectionState.Error) } | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
c: 이거 해결하면 좋을 거 같아요😊 토큰 만료시에 로그인 화면으로 이동하게 하면 좋을 거 같아요
근데 생각해보면 마이페이지 화면으로 가려면 홈화면을 거치면서 이미 한번 refresh토큰을 재발급하게 되는데 꼭 필요한 로직인지는 잘 모르겠는데
채넛 생각으로 명시하면 좋을 거 같다고 판단되면 추가하면 좋을 거 같아요😊
Comment on lines
71
to
75
| Icon( | ||
| painter = painterResource(R.drawable.btn_chevron_right), | ||
| tint = TuripTheme.colors.gray02, | ||
| contentDescription = null, | ||
| ) |
Contributor
There was a problem hiding this comment.
Suggested change
| Icon( | |
| painter = painterResource(R.drawable.btn_chevron_right), | |
| tint = TuripTheme.colors.gray02, | |
| contentDescription = null, | |
| ) | |
| Icon( | |
| imageVector = Icons.AutoMirrored.Default.KeyboardArrowRight, | |
| tint = TuripTheme.colors.gray02, | |
| contentDescription = null, | |
| ) |
r: Icons에 있는 걸 활용하는 게 좋을 거 같아요😊
Comment on lines
93
to
98
| Icon( | ||
| painter = painterResource(R.drawable.ic_profile_default), | ||
| contentDescription = stringResource(R.string.my_page_profile_image_description), | ||
| tint = Color.Unspecified, | ||
| modifier = Modifier.fillMaxSize(0.4f), | ||
| ) |
Contributor
There was a problem hiding this comment.
Suggested change
| Icon( | |
| painter = painterResource(R.drawable.ic_profile_default), | |
| contentDescription = stringResource(R.string.my_page_profile_image_description), | |
| tint = Color.Unspecified, | |
| modifier = Modifier.fillMaxSize(0.4f), | |
| ) | |
| Icon( | |
| imageVector = Icons.Default.Person, | |
| contentDescription = stringResource(R.string.my_page_profile_image_description), | |
| tint = Color.Unspecified, | |
| modifier = Modifier.fillMaxSize(0.6f), | |
| ) |
r: Icons에 있는 걸 활용하는 게 좋은 거 같아요😊
Contributor
Author
There was a problem hiding this comment.
오, 제공해주는 게 있었군요 좋네요 ! :)
반영 커밋 : 828390e
Contributor
There was a problem hiding this comment.
a: 낙관적 UI 적용을 너무 잘해주시는 것 같습니다! 역채넛👍
There was a problem hiding this comment.
🧹 Nitpick comments (2)
android/app/src/main/java/com/on/turip/ui/compose/mypage/component/MyPageSettingsSection.kt (1)
108-125: Preview에서 전역 상태 변경은SideEffect또는LaunchedEffect로 감싸는 것이 좋습니다.Preview 함수 내에서
AuthState.change()를 직접 호출하면 컴포지션 중에 사이드 이펙트가 발생합니다. Preview 코드이므로 프로덕션에 영향은 없지만, Compose의 관례를 따르려면 다음과 같이 수정할 수 있습니다:♻️ 권장 수정안
`@Preview`(showBackground = true) `@Composable` private fun MyPageSettingsSectionMemberPreview() { + LaunchedEffect(Unit) { + AuthState.change(UserType.MEMBER) + } TuripTheme { - AuthState.change(UserType.MEMBER) MyPageSettingsSection(🤖 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 108 - 125, The preview MyPageSettingsSectionMemberPreview is calling AuthState.change(UserType.MEMBER) directly during composition which is a side-effect; wrap that call in a Compose side-effect (e.g., SideEffect or LaunchedEffect) inside MyPageSettingsSectionMemberPreview so the state change runs outside composition. Locate MyPageSettingsSectionMemberPreview and replace the direct AuthState.change(...) invocation with a SideEffect { AuthState.change(UserType.MEMBER) } (or LaunchedEffect(Unit) { AuthState.change(...) }) to follow Compose conventions.android/app/src/main/java/com/on/turip/ui/compose/mypage/component/ProfileSection.kt (1)
133-142:Text에 전달된modifier가 중복 적용될 수 있습니다.
ProfileRightContent가 받은modifier(라인 120)는 이미Box에 적용되어 있는데(라인 123),Success상태의Text에도 동일한modifier가 다시 전달되고 있습니다(라인 140). 이로 인해weight(1f)가 Box와 Text 양쪽에 적용될 수 있습니다.♻️ 권장 수정안
is MyPageSectionState.Success -> { Text( text = state.data.nickname, textAlign = TextAlign.Start, style = TuripTheme.typography.title1, maxLines = 2, overflow = TextOverflow.Ellipsis, - modifier = modifier, + modifier = Modifier.fillMaxWidth(), ) }🤖 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/ProfileSection.kt` around lines 133 - 142, ProfileRightContent receives a modifier that's already applied to the outer Box, and that same modifier is being passed again into the Success branch Text causing duplicate layout effects (e.g., weight applied twice); fix by not forwarding the incoming modifier into the Text — remove the "modifier = modifier" from the Text in the MyPageSectionState.Success branch and, if the Text needs its own layout changes, give it a fresh Modifier (e.g., Modifier.weight(1f) or Modifier.fillMaxWidth()) composed locally so only the Box consumes the passed-in modifier and the Text uses a distinct modifier.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@android/app/src/main/java/com/on/turip/ui/compose/mypage/component/MyPageSettingsSection.kt`:
- Around line 108-125: The preview MyPageSettingsSectionMemberPreview is calling
AuthState.change(UserType.MEMBER) directly during composition which is a
side-effect; wrap that call in a Compose side-effect (e.g., SideEffect or
LaunchedEffect) inside MyPageSettingsSectionMemberPreview so the state change
runs outside composition. Locate MyPageSettingsSectionMemberPreview and replace
the direct AuthState.change(...) invocation with a SideEffect {
AuthState.change(UserType.MEMBER) } (or LaunchedEffect(Unit) {
AuthState.change(...) }) to follow Compose conventions.
In
`@android/app/src/main/java/com/on/turip/ui/compose/mypage/component/ProfileSection.kt`:
- Around line 133-142: ProfileRightContent receives a modifier that's already
applied to the outer Box, and that same modifier is being passed again into the
Success branch Text causing duplicate layout effects (e.g., weight applied
twice); fix by not forwarding the incoming modifier into the Text — remove the
"modifier = modifier" from the Text in the MyPageSectionState.Success branch
and, if the Text needs its own layout changes, give it a fresh Modifier (e.g.,
Modifier.weight(1f) or Modifier.fillMaxWidth()) composed locally so only the Box
consumes the passed-in modifier and the Text uses a distinct modifier.
ℹ️ 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/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/BookmarkedContentItem.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.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- android/app/src/main/java/com/on/turip/ui/compose/mypage/component/BookmarkedContentItem.kt
- android/app/src/main/java/com/on/turip/ui/compose/mypage/component/MyPageSettingItem.kt
yrsel
commented
Feb 26, 2026
Contributor
Author
yrsel
left a comment
yrsel
left a comment
There was a problem hiding this comment.
제리, 리뷰 반영하고 남겨주신 의견에 대한 생각 남겼습니다!
다시 한 번 확인 부탁드립니다 감사합니다 😊
Comment on lines
128
to
134
| when (state) { | ||
| MyPageSectionState.Loading -> { | ||
| CircularProgressIndicator( | ||
| modifier = modifier.size(2.dp), | ||
| color = TuripTheme.colors.gray03, | ||
| ) | ||
| } |
Contributor
Author
There was a problem hiding this comment.
c: 여기는 ProgressBar가 없어도 될 거 같은데 어떻게 생각하시나요?
넵, 좋아요!
Section 별로 에러, 로딩, 정상 화면에 대해 추상화 되어 있던 상태였고,
추후에 Loading 상황에 어떤 UI를 보여주게 될 수도 있을까 싶어서 빈 블록으로 남겨두었는데 완전히 제거하는 걸 원하시면 다시 반영하겠습니다!
반영 커밋 : afee69e
Comment on lines
53
to
87
| fun loadProfile() { | ||
| viewModelScope.launch { | ||
| _uiState.update { it.copy(profileState = MyPageSectionState.Loading) } | ||
|
|
||
| accountRepository | ||
| .loadMyProfile() | ||
| .onSuccess { result: Account -> | ||
| _uiState.update { | ||
| Timber.d("마이페이지 프로필 조회 성공") | ||
| it.copy(profileState = MyPageSectionState.Success(result.toUiModel())) | ||
| } | ||
| }.onFailure { | ||
| Timber.e("마이페이지 프로필 조회 에러 발생") | ||
| _uiState.update { it.copy(profileState = MyPageSectionState.Error) } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| fun loadBookmarkContents() { | ||
| viewModelScope.launch { | ||
| _uiState.update { it.copy(bookmarkContentState = MyPageSectionState.Loading) } | ||
|
|
||
| bookmarkRepository | ||
| .loadBookmarks(10, 0L) | ||
| .onSuccess { result: PagedBookmarkContents -> | ||
| Timber.d("마이페이지 북마크 목록 조회 성공") | ||
| _uiState.update { | ||
| it.copy(bookmarkContentState = MyPageSectionState.Success(result.bookmarkContents.toImmutableList())) | ||
| } | ||
| }.onFailure { | ||
| Timber.e("마이페이지 북마크 목록 조회 에러 발생") | ||
| _uiState.update { it.copy(bookmarkContentState = MyPageSectionState.Error) } | ||
| } | ||
| } | ||
| } |
Contributor
Author
There was a problem hiding this comment.
백그라운드에서 오래 있다가 진입하는 경우를 생각해서 처리하는 게 좋긴 하지만, 현재 Ktor에서 401을 먼저 가로채서 처리하다보니 에러가 내려오질 않아 홈으로 이동 시키는 로직이 실행되지 않는 문제가 있어요!
이번에 Navigation을 적용하면서 제리가 언급했던 이벤트 버스를 여기에 적용해 단일 Activity에서 Login으로 이동시키는 로직을 추가하는 게 더 좋을 것 같아보여요!
Comment on lines
93
to
98
| Icon( | ||
| painter = painterResource(R.drawable.ic_profile_default), | ||
| contentDescription = stringResource(R.string.my_page_profile_image_description), | ||
| tint = Color.Unspecified, | ||
| modifier = Modifier.fillMaxSize(0.4f), | ||
| ) |
Contributor
Author
There was a problem hiding this comment.
오, 제공해주는 게 있었군요 좋네요 ! :)
반영 커밋 : 828390e
Comment on lines
67
to
116
| @Composable | ||
| private fun ProfileImage( | ||
| imageUrl: String?, | ||
| modifier: Modifier = Modifier, | ||
| ) { | ||
| val shape = CircleShape | ||
|
|
||
| Box( | ||
| modifier = | ||
| modifier | ||
| .aspectRatio(1f) | ||
| .shadow( | ||
| elevation = 14.dp, | ||
| shape = shape, | ||
| ambientColor = TuripTheme.colors.black, | ||
| spotColor = TuripTheme.colors.black, | ||
| ).clip(shape) | ||
| .background(TuripTheme.colors.white) | ||
| .border( | ||
| width = 2.dp, | ||
| color = TuripTheme.colors.gray01, | ||
| shape = shape, | ||
| ), | ||
| contentAlignment = Alignment.Center, | ||
| ) { | ||
| if (imageUrl.isNullOrBlank()) { | ||
| Icon( | ||
| painter = painterResource(R.drawable.ic_profile_default), | ||
| contentDescription = stringResource(R.string.my_page_profile_image_description), | ||
| tint = Color.Unspecified, | ||
| modifier = Modifier.fillMaxSize(0.4f), | ||
| ) | ||
| } else { | ||
| AsyncImage( | ||
| model = | ||
| ImageRequest | ||
| .Builder(LocalContext.current) | ||
| .data(imageUrl) | ||
| .crossfade(true) | ||
| .build(), | ||
| contentDescription = stringResource(R.string.my_page_profile_image_description), | ||
| contentScale = ContentScale.Crop, | ||
| modifier = Modifier.matchParentSize(), | ||
| placeholder = painterResource(R.drawable.bg_image_placeholder), | ||
| error = painterResource(R.drawable.ic_sorry), | ||
| ) | ||
| } | ||
| } | ||
| } | ||
|
|
Contributor
Author
There was a problem hiding this comment.
제리가 제안해준 방식 UI가 깔끔하고 좋네요! 👍
다만, 현재 저희는 프로필 이미지를 지원하지 않아 항상 기본 아이콘으로만 보이게 되는데 이미지 하단에 재시도 버튼을 클릭 으로 닉네임만 갱신되는 흐름 괜찮을까요 ??
백엔드 팀원들에게도 의견 물어보면 좋을 것 같아요!
| data class MyPageUiState( | ||
| val profileState: MyPageSectionState<ProfileModel>, | ||
| val bookmarkContentState: MyPageSectionState<ImmutableList<BookmarkContent>>, | ||
| val dialogState: MyPageDialogState?, |
Contributor
Author
There was a problem hiding this comment.
저는 DialogState에서 null 값이 Dialog가 보이지 않는 상태를 표현한다고 생각해서 2번의 역할만 한다고 생각했어요!
dialogState와 dialogMode로 구분하게 된다면 uiState의 유휴 상태의 값을 정의하기 위해 MyPageDialogState에 None 또는 Idle 과 같은 상태가 추가적으로 필요하게 되고 그렇게되면 null을 사용하는 방식과 동일하게 되어버릴 것 같아요
null 표현이 별로라면
uiState.dialogState?.let { state ->
when(state) {
MyPageDialogState.ConfirmWithdraw -> TODO()
MyPageDialogState.LogoutRequired -> TODO()
}
}이런 형태로 사용할 수 있을 것 같아요!
아니면 showXXXDialog:Boolean 변수로 관리하는 방식이 더 괜찮을까요 ?!
Comment on lines
53
to
58
| Column( | ||
| modifier = | ||
| modifier | ||
| .padding(TuripTheme.spacing.extraSmall) | ||
| .clip(TuripTheme.shape.container) | ||
| .clickable { onContentClick(item.content.id) }, |
Comment on lines
100
to
104
| MyPageSettingItem( | ||
| imageRes = R.drawable.ic_withdraw, | ||
| titleRes = R.string.my_page_withdraw, | ||
| onClick = onWithdrawClick, | ||
| ) |
Comment on lines
197
to
204
| userStorageRepository | ||
| .clearTokens() | ||
| .onSuccess { | ||
| _uiEffect.send(MyPageUiEffect.NavigateToLogin) | ||
| Timber.d("회원탈퇴 성공") | ||
| }.onFailure { | ||
| Timber.e("토큰 초기화 실패") | ||
| } |
Comment on lines
71
to
75
| Icon( | ||
| painter = painterResource(R.drawable.btn_chevron_right), | ||
| tint = TuripTheme.colors.gray02, | ||
| contentDescription = null, | ||
| ) |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
android/app/src/main/res/values/strings.xml (1)
61-63:⚠️ Potential issue | 🟡 Minor회원 탈퇴 문구 띄어쓰기를 일관되게 맞춰주세요.
Line 57(
회원 탈퇴)와 Line 61~62(회원탈퇴) 표기가 달라 UI 문구 일관성이 깨집니다.✏️ 제안 수정
- <string name="my_page_withdraw_dialog_message">정말 회원탈퇴 하시겠습니까?</string> - <string name="my_page_withdraw_dialog_confirm">회원탈퇴</string> + <string name="my_page_withdraw_dialog_message">정말 회원 탈퇴 하시겠습니까?</string> + <string name="my_page_withdraw_dialog_confirm">회원 탈퇴</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 61 - 63, The three strings my_page_withdraw_dialog_message, my_page_withdraw_dialog_confirm, and my_page_withdraw_dialog_dismiss use "회원탈퇴" without a space while the other label uses "회원 탈퇴"; update the values of these string resources to use the spaced form "회원 탈퇴" (e.g., change "정말 회원탈퇴 하시겠습니까?" and "회원탈퇴" instances to "정말 회원 탈퇴 하시겠습니까?" and "회원 탈퇴") so UI wording is consistent across the app.
🤖 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/mypage/component/ProfileSection.kt`:
- Line 107: The compile error is caused by a missing import for
Modifier.matchParentSize used in ProfileSection.kt; add the import
androidx.compose.foundation.layout.matchParentSize to the file's imports (near
other androidx.compose imports) so the call to Modifier.matchParentSize() in the
ProfileSection composable resolves correctly.
---
Duplicate comments:
In `@android/app/src/main/res/values/strings.xml`:
- Around line 61-63: The three strings my_page_withdraw_dialog_message,
my_page_withdraw_dialog_confirm, and my_page_withdraw_dialog_dismiss use "회원탈퇴"
without a space while the other label uses "회원 탈퇴"; update the values of these
string resources to use the spaced form "회원 탈퇴" (e.g., change "정말 회원탈퇴 하시겠습니까?"
and "회원탈퇴" instances to "정말 회원 탈퇴 하시겠습니까?" and "회원 탈퇴") so UI wording is
consistent across the app.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
android/app/src/main/java/com/on/turip/ui/compose/mypage/component/MyPageAppBar.ktandroid/app/src/main/java/com/on/turip/ui/compose/mypage/component/ProfileSection.ktandroid/app/src/main/res/values/strings.xml
android/app/src/main/java/com/on/turip/ui/compose/mypage/component/ProfileSection.kt
Outdated
Show resolved
Hide resolved
jerry8282
approved these changes
Feb 26, 2026
Contributor
jerry8282
left a comment
jerry8282
left a comment
There was a problem hiding this comment.
고생하셨습니다 채넛!! 빠른 리뷰 반영 너무 좋습니당
| .build(), | ||
| contentDescription = contentDescription, | ||
| contentScale = ContentScale.Crop, | ||
| modifier = Modifier.matchParentSize(), |
Comment on lines
67
to
116
| @Composable | ||
| private fun ProfileImage( | ||
| imageUrl: String?, | ||
| modifier: Modifier = Modifier, | ||
| ) { | ||
| val shape = CircleShape | ||
|
|
||
| Box( | ||
| modifier = | ||
| modifier | ||
| .aspectRatio(1f) | ||
| .shadow( | ||
| elevation = 14.dp, | ||
| shape = shape, | ||
| ambientColor = TuripTheme.colors.black, | ||
| spotColor = TuripTheme.colors.black, | ||
| ).clip(shape) | ||
| .background(TuripTheme.colors.white) | ||
| .border( | ||
| width = 2.dp, | ||
| color = TuripTheme.colors.gray01, | ||
| shape = shape, | ||
| ), | ||
| contentAlignment = Alignment.Center, | ||
| ) { | ||
| if (imageUrl.isNullOrBlank()) { | ||
| Icon( | ||
| painter = painterResource(R.drawable.ic_profile_default), | ||
| contentDescription = stringResource(R.string.my_page_profile_image_description), | ||
| tint = Color.Unspecified, | ||
| modifier = Modifier.fillMaxSize(0.4f), | ||
| ) | ||
| } else { | ||
| AsyncImage( | ||
| model = | ||
| ImageRequest | ||
| .Builder(LocalContext.current) | ||
| .data(imageUrl) | ||
| .crossfade(true) | ||
| .build(), | ||
| contentDescription = stringResource(R.string.my_page_profile_image_description), | ||
| contentScale = ContentScale.Crop, | ||
| modifier = Modifier.matchParentSize(), | ||
| placeholder = painterResource(R.drawable.bg_image_placeholder), | ||
| error = painterResource(R.drawable.ic_sorry), | ||
| ) | ||
| } | ||
| } | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
넵 백엔드 팀원들에게도 물어보고 반영하면 좋을 거 같아요👍
| data class MyPageUiState( | ||
| val profileState: MyPageSectionState<ProfileModel>, | ||
| val bookmarkContentState: MyPageSectionState<ImmutableList<BookmarkContent>>, | ||
| val dialogState: MyPageDialogState?, |
Contributor
There was a problem hiding this comment.
저는 그동안 Dialog 상태를 showXXXDialog: Boolean 형태로 관리해와서, 지금 구조가 조금 낯설게 느껴졌던 것 같아요.🥲
위 코드도 결국 state = null이라는 하나의 상태를 표현하고 있는 것처럼 보이는데, 개인적으로는 null로 상태를 표현하는 것보다는 Boolean으로 명확하게 드러내는 방식이 더 직관적이고 이해하기 쉬운 것 같다는 생각이 들었습니다.😊
Comment on lines
128
to
134
| when (state) { | ||
| MyPageSectionState.Loading -> { | ||
| CircularProgressIndicator( | ||
| modifier = modifier.size(2.dp), | ||
| color = TuripTheme.colors.gray03, | ||
| ) | ||
| } |
Contributor
There was a problem hiding this comment.
빈 블록으로 남겨두는 게 좋은 거 같습니당👍
There was a problem hiding this comment.
🧹 Nitpick comments (1)
android/app/src/main/java/com/on/turip/ui/compose/mypage/component/ProfileSection.kt (1)
166-174:onRetry()호출 타이밍 확인.현재
onRetry()가 코루틴 바깥에서 즉시 호출되어, 회전 애니메이션이 완료되기 전에 재시도 요청이 발생합니다. 의도된 UX라면 괜찮지만, 애니메이션 완료 후 재시도를 원한다면 코루틴 내부로 이동해야 합니다.💡 애니메이션 완료 후 재시도 호출 (선택 사항)
) { scope.launch { isSpinning = true rotation.snapTo(0f) rotation.animateTo(360f, tween(450, easing = LinearEasing)) isSpinning = false + onRetry() } - onRetry() },🤖 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/ProfileSection.kt` around lines 166 - 174, 현재 onRetry()가 코루틴 외부에서 즉시 호출되어 애니메이션이 끝나기 전에 재시도가 발생하므로, onRetry() 호출을 scope.launch 블록 내부로 옮겨 rotation.animateTo(...)가 완료된 다음에 실행되도록 변경하세요; 구체적으로 ProfileSection.kt의 해당 클릭 핸들러에서 scope.launch { isSpinning = true; rotation.snapTo(0f); rotation.animateTo(360f, ...); isSpinning = false; onRetry() } 순으로 배치하여 애니메이션 완료 후 재시도가 이루어지게 하세요 (참조 심볼: onRetry, scope.launch, rotation, isSpinning, rotation.animateTo).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@android/app/src/main/java/com/on/turip/ui/compose/mypage/component/ProfileSection.kt`:
- Around line 166-174: 현재 onRetry()가 코루틴 외부에서 즉시 호출되어 애니메이션이 끝나기 전에 재시도가 발생하므로,
onRetry() 호출을 scope.launch 블록 내부로 옮겨 rotation.animateTo(...)가 완료된 다음에 실행되도록
변경하세요; 구체적으로 ProfileSection.kt의 해당 클릭 핸들러에서 scope.launch { isSpinning = true;
rotation.snapTo(0f); rotation.animateTo(360f, ...); isSpinning = false;
onRetry() } 순으로 배치하여 애니메이션 완료 후 재시도가 이루어지게 하세요 (참조 심볼: onRetry, scope.launch,
rotation, isSpinning, rotation.animateTo).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
android/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/ProfileSection.ktandroid/app/src/main/java/com/on/turip/ui/compose/mypage/util/MyPageMapper.ktandroid/app/src/main/java/com/on/turip/ui/mypage/MyPageActivity.kt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issues
✔️ Check-list
🗒️ Work Description
마이페이지 화면 새로운 UI를 반영하며 Compose 로 마이그레이션 진행했습니다
현재 바텀네비게이션 작업이 진행되지 않은 상태라 임시로 홈 화면 하단에 작업한 부분으로 이동할 수 있는 텍스트를 만들었습니다.(바텀네비게이션 작업 완료 후 반영 후 머지될 예정)
계정에 대한 랜덤 닉네임이 정상적으로 생성되는 것을 확인했습니다.(API 연동 완료)
마이그레이션 전에 북마크 콘텐츠 제거 연속 클릭시, 에러가 발생했던 부분은 mutex와 낙관적 UI를 적용했습니다.
저장한 콘텐츠모두 보기버튼 클릭 시 이동하는 화면이 아직 Compose 마이그레이션 되지 않은 상태라 추후 연동이 필요합니다.📷 Screenshot
제리가 제안한 프로필 UI 반영
retry_profile.webm
프로필 불러오기 실패 스낵바 알림
profile_load_fail.webm
북마크 콘텐츠 불러오기 실패 스낵바 알림
bookmarks_load_fail.webm
📚 Reference
Summary by CodeRabbit
릴리스 노트
새 기능
개선