[Feat] 설정화면 Compose 마이그레이션 #26
Conversation
앱 설정 화면(`SettingScreen`)을 Jetpack Compose로 신규 구현했습니다. 사용자가 대학 소식 알림을 설정하고, 앱 정보(버전, 약관, 문의하기)를 확인할 수 있는 UI를 구성했습니다.
- **`SettingScreen.kt` 추가:**
- `Scaffold`와 공통 `Header` 컴포넌트를 사용하여 전체 화면 레이아웃을 구성했습니다.
- `SubscriptionContent` 컴포저블을 통해 대학 이름 표시 및 알림 구독 상태를 제어하는 `Switch` UI를 구현했습니다.
- `AppInfoContent` 컴포저블을 통해 앱 버전 정보 표시, 이용 약관 및 문의하기 버튼을 리스트 형태로 구현했습니다.
- `HorizontalDivider`를 사용하여 콘텐츠 섹션을 시각적으로 구분하고, `FestabookTheme` 디자인 시스템을 적용했습니다.
설정 화면의 데이터를 `FestivalUiState`로 관리하도록 변경하고, 구독 스위치의 활성화 제어 기능을 추가했습니다. 또한 상단 앱바 컴포넌트를 교체하여 UI 일관성을 맞췄습니다.
- **`SettingScreen.kt` 수정:**
- 컴포저블 파라미터를 `universityName` 문자열 대신 `FestivalUiState`로 변경하여, 상태(`Success`, `Error`)에 따라 콘텐츠 표시 및 에러 처리를 수행하도록 구조를 개편했습니다.
- `LaunchedEffect`를 추가하여 `UiState.Error` 발생 시 `onError` 콜백이 호출되도록 로직을 추가했습니다.
- 기존 `Header` 컴포넌트를 `FestabookTopAppBar`로 교체하고, 전체 화면 배경색을 `White`로 지정했습니다.
- `SubscriptionContent`에 `isSubscribeEnabled` 파라미터를 추가하여 스위치의 `enabled` 속성을 제어하고, 비활성화 상태일 때의 트랙 색상을 정의했습니다.
`SettingViewModel`의 데이터 흐름을 개선하기 위해 기존 `LiveData` 기반의 상태 관리를 Coroutines Flow API로 마이그레이션했습니다.
- **`SettingViewModel.kt` 수정:**
- UI 상태(`isAllowed`, `isLoading`)를 관리하던 `MutableLiveData`를 `MutableStateFlow`로 교체했습니다.
- 일회성 이벤트(`permissionCheckEvent`, `error`, `success`) 처리를 위해 사용하던 `SingleLiveData`를 `MutableSharedFlow`로 변경했습니다.
- 데이터 발행 방식을 `setValue`에서 `tryEmit`으로 수정하고, `Boolean?` 타입 체크 로직을 Flow의 `value` 접근 방식으로 간소화했습니다.
기존 XML 및 DataBinding 기반의 설정 화면을 Jetpack Compose로 마이그레이션했습니다.
- **`SettingFragment.kt` 리팩토링:**
- `onCreateView`에서 `ComposeView`를 반환하도록 변경하고, `SettingScreen` 컴포저블을 통해 UI를 렌더링하도록 수정했습니다.
- `DataBinding` 및 `View` 관련 로직을 제거하고, `collectAsStateWithLifecycle`을 사용하여 `ViewModel`의 상태를 구독하도록 변경했습니다.
- 권한 요청 및 URL 이동 등의 사이드 이펙트를 처리하기 위해 `ObserveAsEvents`를 적용했습니다.
- **`ObserveAsEvents.kt` 추가:**
- Compose 환경에서 `Flow` 기반의 이벤트를 생명주기에 맞춰 안전하게 수집할 수 있는 유틸리티 함수를 추가했습니다.
`SettingViewModel` 테스트 코드에서 `getOrAwaitValue` 확장 함수 의존성을 제거하고, `StateFlow` 및 `SharedFlow` 특성에 맞는 검증 로직으로 변경했습니다.
- **`SettingViewModelTest.kt` 수정:**
- 불필요해진 `getOrAwaitValue` import 구문을 제거했습니다.
- `permissionCheckEvent` 검증 시 `value` 대신 `replayCache.first()`를 사용하여 이벤트를 확인하도록 수정했습니다.
- `isAllowed` 상태 검증 시 `getOrAwaitValue()` 호출을 `value` 프로퍼티 접근으로 대체했습니다.
- 일부 코드의 줄바꿈 포맷팅을 수정했습니다.
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThe pull request migrates the SettingFragment UI from a binding-based Fragment setup to a Compose-based implementation, introducing a lifecycle-aware Flow observation utility, converting SettingViewModel's reactive properties from LiveData to Flow/StateFlow variants, and adding a new Compose component for the settings screen. Changes
Sequence Diagram(s)sequenceDiagram
participant SF as SettingFragment
participant VM as SettingViewModel
participant CS as SettingScreen
participant Flow as Flow/StateFlow
SF->>SF: onCreateView() creates ComposeView
SF->>VM: collectAsStateWithLifecycle(permissionCheckEvent)
SF->>VM: collectAsStateWithLifecycle(isAllowed)
SF->>VM: collectAsStateWithLifecycle(error)
SF->>VM: collectAsStateWithLifecycle(isLoading)
SF->>VM: collectAsStateWithLifecycle(successFlow)
VM->>Flow: StateFlow emits current state
Flow->>SF: State collected as Compose State
SF->>CS: Pass state and callbacks to SettingScreen
CS->>CS: Render with ObserveAsEvents for events
Note over CS: User interaction (subscribe toggle, policy click)
CS->>VM: Call callback (onSubscribeClick, onPolicyClick)
VM->>Flow: Emit new state/event
Flow->>SF: Observe lifecycle & collect on STARTED
SF->>CS: Recompose with new state
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
@app/src/main/java/com/daedan/festabook/presentation/setting/SettingFragment.kt:
- Around line 86-88: Remove the redundant super.onCreateView(...) invocation
inside SettingFragment (it inflates fragment_setting.xml in BaseFragment and its
result is unused) and update the class generic: change the class declaration to
extend BaseFragment<FragmentSettingBinding>() if the base requires a type
parameter, or remove the generic from BaseFragment usage entirely if not needed;
ensure no other code in SettingFragment references the discarded binding and
keep the ComposeView creation and setViewCompositionStrategy call intact.
🧹 Nitpick comments (7)
app/src/test/java/com/daedan/festabook/setting/SettingViewModelTest.kt (1)
24-25: Consider removingInstantTaskExecutorRuleif no longer needed.Since the ViewModel properties have been migrated from LiveData to StateFlow/SharedFlow, this rule may no longer be necessary. The only remaining LiveData is
success(viaasLiveData()), but it's not tested directly in these tests.app/src/main/java/com/daedan/festabook/presentation/common/component/ObserveAsEvents.kt (1)
20-26: ThewithContext(Dispatchers.Main.immediate)may be redundant.
LaunchedEffectalready runs in the composition's coroutine context (Main dispatcher), andrepeatOnLifecyclepreserves this context. The explicitwithContextcall adds unnecessary overhead.♻️ Simplified implementation
LaunchedEffect(flow, lifecycleOwner.lifecycle) { lifecycleOwner.repeatOnLifecycle(Lifecycle.State.STARTED) { - withContext(Dispatchers.Main.immediate) { - flow.collect(onEvent) - } + flow.collect(onEvent) } }app/src/main/java/com/daedan/festabook/presentation/setting/SettingFragment.kt (1)
70-70: Consider usingrequireView()instead ofview!!.Using
view!!can throw an unhandledNullPointerException. While this callback occurs after view creation, usingrequireView()provides a clearer error message if called at an unexpected time.♻️ Safer alternative
- showNotificationDeniedSnackbar(view!!, requireContext()) + showNotificationDeniedSnackbar(requireView(), requireContext())app/src/main/java/com/daedan/festabook/presentation/setting/component/SettingScreen.kt (4)
59-64: Extract duplicatedscreenWidthDpcalculation.The screen width calculation is duplicated in
SettingScreen(lines 59-64) andAppInfoContent(lines 191-196). Consider extracting this to a local composable helper or passing the value as a parameter.♻️ Pass screenWidthDp as parameter to AppInfoContent
@Composable private fun AppInfoContent( appVersion: String, onPolicyClick: () -> Unit, onContactUsClick: () -> Unit, + screenWidthDp: Dp, modifier: Modifier = Modifier, ) { - val windowInfo = LocalWindowInfo.current - val density = LocalDensity.current - val screenWidthDp = - with(density) { - windowInfo.containerSize.width.toDp() - }Also applies to: 191-196
165-179: Consider making disabled state visually distinct.The disabled track colors (
disabledCheckedTrackColor = FestabookColor.black,disabledUncheckedTrackColor = FestabookColor.gray200) match the enabled colors. Users may not realize the switch is disabled during loading. Consider using a lighter/muted color for the disabled state.
224-272: Consider extracting a reusableClickableInfoRowcomposable.The Policy and Contact Us rows share identical structure. Extracting a reusable component would reduce duplication.
♻️ Extracted component example
@Composable private fun ClickableInfoRow( text: String, screenWidthDp: Dp, onClick: () -> Unit, modifier: Modifier = Modifier, ) { Row( horizontalArrangement = Arrangement.SpaceBetween, verticalAlignment = Alignment.CenterVertically, modifier = modifier .requiredWidth(screenWidthDp) .clickable(onClick = onClick) .padding( horizontal = festabookSpacing.paddingScreenGutter, vertical = festabookSpacing.paddingBody3, ), ) { Text(text = text, style = FestabookTypography.titleMedium) Icon( painter = painterResource(R.drawable.ic_arrow_forward_right), contentDescription = stringResource(R.string.move), tint = Color.Unspecified, ) } }
90-101: Consider adding a loading indicator for non-Success states.When
festivalUiStateisLoading, theSubscriptionContentarea is empty. Consider showing a shimmer/skeleton placeholder for better UX during data loading.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/src/main/java/com/daedan/festabook/presentation/common/component/ObserveAsEvents.ktapp/src/main/java/com/daedan/festabook/presentation/setting/SettingFragment.ktapp/src/main/java/com/daedan/festabook/presentation/setting/SettingViewModel.ktapp/src/main/java/com/daedan/festabook/presentation/setting/component/SettingScreen.ktapp/src/test/java/com/daedan/festabook/setting/SettingViewModelTest.kt
🧰 Additional context used
🧬 Code graph analysis (2)
app/src/main/java/com/daedan/festabook/presentation/setting/component/SettingScreen.kt (2)
app/src/main/java/com/daedan/festabook/presentation/common/component/FestabookTopAppBar.kt (1)
FestabookTopAppBar(13-31)app/src/main/java/com/daedan/festabook/presentation/theme/FestabookTheme.kt (1)
FestabookTheme(15-34)
app/src/main/java/com/daedan/festabook/presentation/setting/SettingFragment.kt (4)
app/src/main/java/com/daedan/festabook/presentation/common/PermissionUtil.kt (1)
showNotificationDeniedSnackbar(13-32)app/src/main/java/com/daedan/festabook/presentation/common/component/ObserveAsEvents.kt (1)
ObserveAsEvents(14-27)app/src/main/java/com/daedan/festabook/presentation/common/FragmentUtil.kt (3)
showErrorSnackBar(54-56)showErrorSnackBar(58-75)showErrorSnackBar(77-109)app/src/main/java/com/daedan/festabook/presentation/setting/component/SettingScreen.kt (1)
SettingScreen(47-120)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run-PR-Test
🔇 Additional comments (10)
app/src/test/java/com/daedan/festabook/setting/SettingViewModelTest.kt (2)
45-60: LGTM!The test correctly uses
replayCache.first()to access the emitted event from theSharedFlowwithreplay = 1, which is the appropriate pattern for testing one-shot events.
62-118: LGTM!The remaining tests correctly use
StateFlow.valueto access state and properly verify the rollback behavior when server operations fail.app/src/main/java/com/daedan/festabook/presentation/common/component/ObserveAsEvents.kt (1)
12-13: Acknowledge duplication note.The comment appropriately documents the intentional duplication with PR #24. Ensure proper conflict resolution during merge.
app/src/main/java/com/daedan/festabook/presentation/setting/SettingFragment.kt (1)
89-131: LGTM!The Compose migration is well-structured:
- State collection uses
collectAsStateWithLifecyclefor lifecycle awarenessObserveAsEventscorrectly handles one-shot events (permissions, success, ViewModel errors)SettingScreencallbacks are properly wired- Error handling distinguishes between
FestivalUiState.Error(UI state) and ViewModel operation errorsapp/src/main/java/com/daedan/festabook/presentation/setting/component/SettingScreen.kt (1)
65-72: LGTM!The use of
rememberUpdatedStateforonErrorcorrectly prevents stale closure issues inLaunchedEffect, ensuring the latest callback is always invoked.app/src/main/java/com/daedan/festabook/presentation/setting/SettingViewModel.kt (5)
75-93: Verify the intended UX for optimistic success notification.The success event is emitted (line 78) before the server call completes. If the server call fails, users see the success snackbar followed by an error snackbar. Consider emitting success only after server confirmation, or clarifying this is the intended behavior.
27-31: Be aware of event replay on resubscription.
MutableSharedFlow(replay = 1)caches the last emitted event, which replays to new subscribers (e.g., on configuration change). This could cause duplicate snackbars or permission requests. Consider usingresetReplayCache()after consumption if this becomes problematic.Also applies to: 39-43, 48-51
55-61: LGTM!The
notificationAllowClickfunction correctly checks the current allowed state before deciding to request permission or delete the notification.
96-118: LGTM!The
deleteNotificationIdfunction correctly implements the optimistic UI pattern with proper rollback on failure.
48-53: No action needed. Bothsuccess(LiveData) andsuccessFlow(SharedFlow) are actively used in different parts of the codebase:MainActivity.ktobserves the LiveData version whileSettingFragment.ktuses the SharedFlow viaObserveAsEvents. The dual exposure is intentional to support different consumer patterns.Likely an incorrect or invalid review comment.
| ComposeView(requireContext()).apply { | ||
| super.onCreateView(inflater, container, savedInstanceState) | ||
| setViewCompositionStrategy(ViewCompositionStrategy.DisposeOnViewTreeLifecycleDestroyed) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "*.kt" | xargs grep -l "class BaseFragment" | head -5Repository: festabook/android
Length of output: 137
🏁 Script executed:
cat -n app/src/main/java/com/daedan/festabook/presentation/setting/SettingFragment.kt | sed -n '1,50p'Repository: festabook/android
Length of output: 2673
🏁 Script executed:
cat -n app/src/main/java/com/daedan/festabook/presentation/setting/SettingFragment.kt | sed -n '80,110p'Repository: festabook/android
Length of output: 1721
🏁 Script executed:
cat -n ./app/src/main/java/com/daedan/festabook/presentation/common/BaseFragment.ktRepository: festabook/android
Length of output: 2326
🏁 Script executed:
cat -n app/src/main/java/com/daedan/festabook/presentation/setting/SettingFragment.kt | sed -n '80,132p'Repository: festabook/android
Length of output: 2794
🏁 Script executed:
# Check if FragmentSettingBinding is used anywhere in SettingFragment
rg "FragmentSettingBinding" app/src/main/java/com/daedan/festabook/presentation/setting/SettingFragment.kt -nRepository: festabook/android
Length of output: 173
🏁 Script executed:
# Check if SettingFragment accesses binding anywhere
rg "binding\." app/src/main/java/com/daedan/festabook/presentation/setting/SettingFragment.kt -nRepository: festabook/android
Length of output: 126
🏁 Script executed:
# Check if binding is accessed in SettingFragment
rg "this\.binding" app/src/main/java/com/daedan/festabook/presentation/setting/SettingFragment.kt -nRepository: festabook/android
Length of output: 43
🏁 Script executed:
# Verify the complete SettingFragment file to ensure nothing uses binding
wc -l app/src/main/java/com/daedan/festabook/presentation/setting/SettingFragment.ktRepository: festabook/android
Length of output: 143
Remove unnecessary super.onCreateView() call and unused binding type parameter.
The super.onCreateView() call at line 87 inflates the XML layout (fragment_setting.xml) via DataBindingUtil.inflate() in BaseFragment, but the created binding and return value are immediately discarded since SettingFragment returns a ComposeView instead. This wastes resources during initialization. Additionally, the FragmentSettingBinding type parameter is never accessed in this fragment.
Remove the super.onCreateView() call and change the class declaration to BaseFragment<FragmentSettingBinding>() if the base class type parameter is required by the framework, or refactor to remove the type parameter entirely if possible.
🤖 Prompt for AI Agents
In
@app/src/main/java/com/daedan/festabook/presentation/setting/SettingFragment.kt
around lines 86 - 88, Remove the redundant super.onCreateView(...) invocation
inside SettingFragment (it inflates fragment_setting.xml in BaseFragment and its
result is unused) and update the class generic: change the class declaration to
extend BaseFragment<FragmentSettingBinding>() if the base requires a type
parameter, or remove the generic from BaseFragment usage entirely if not needed;
ensure no other code in SettingFragment references the discarded binding and
keep the ComposeView creation and setViewCompositionStrategy call intact.
| ) { | ||
| val windowInfo = LocalWindowInfo.current | ||
| val density = LocalDensity.current | ||
| val screenWidthDp = |
There was a problem hiding this comment.
해당 값을 직접 계산해서 사용하기 보다는, 패딩이 필요한 컨텐츠에 한해서 따로 패딩을 주는 건 어떨까요?
| .clickable { | ||
| onPolicyClick() | ||
| }.padding( | ||
| horizontal = festabookSpacing.paddingScreenGutter, | ||
| vertical = festabookSpacing.paddingBody3, |
There was a problem hiding this comment.
패딩을 주기 전 부분만 클릭 속성이 적용되어 있는데, 의도된 부분일까요?
| Row( | ||
| horizontalArrangement = Arrangement.SpaceBetween, | ||
| verticalAlignment = Alignment.CenterVertically, | ||
| modifier = | ||
| Modifier | ||
| .requiredWidth(screenWidthDp) | ||
| .clickable { | ||
| onContactUsClick() | ||
| }.padding( | ||
| horizontal = festabookSpacing.paddingScreenGutter, | ||
| vertical = festabookSpacing.paddingBody3, | ||
| ), | ||
| ) { | ||
| Text( | ||
| text = stringResource(R.string.setting_contact_us), | ||
| style = FestabookTypography.titleMedium, | ||
| ) | ||
|
|
||
| Icon( | ||
| painter = painterResource(R.drawable.ic_arrow_forward_right), | ||
| contentDescription = stringResource(R.string.move), | ||
| tint = Color.Unspecified, | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
해당 메뉴 아이템을 하나의 컴포저블로 구성할 수 있을 거 같아요~
| ) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| @Composable | ||
| private fun AppInfoContent( | ||
| appVersion: String, |
There was a problem hiding this comment.
스위치도 앱 내에서 동일하게 사용하기 위해 하나의 공통 컴포저블로 만드는 것은 어떨까요?
parkjiminnnn
left a comment
parkjiminnnn
left a comment
There was a problem hiding this comment.
마이그레이션 제가 한다고 큰소리 쳤는데 못해서 미안합니다 🙏
고생 많으셨어요 밀러 알림은 잘 오는 듯 해요!
코멘트 몇 개만 확인 부탁드려요~
| @Composable | ||
| fun <T> ObserveAsEvents( | ||
| flow: Flow<T>, | ||
| onEvent: suspend (T) -> Unit, | ||
| ) { | ||
| val lifecycleOwner = LocalLifecycleOwner.current | ||
| LaunchedEffect(flow, lifecycleOwner.lifecycle) { | ||
| lifecycleOwner.repeatOnLifecycle(Lifecycle.State.STARTED) { | ||
| withContext(Dispatchers.Main.immediate) { | ||
| flow.collect(onEvent) | ||
| } | ||
| } | ||
| } | ||
| } |
| } else { | ||
| Timber.d("Notification permission denied") | ||
| showNotificationDeniedSnackbar(binding.root, requireContext()) | ||
| showNotificationDeniedSnackbar(view!!, requireContext()) |
There was a problem hiding this comment.
에디터가 requireView()를 추천하길래 알아봤더니 view!!나 requireView() 둘 다 뷰가 생성된 시점에 호출하면 둘 다 view객체를 반환한다고하네요.
하지만 예외 발생 측면에서 봤을 때 NPE 보다는 IllegalStateException으로 생명주기 위반을 좀 더 명확하게 알 수 있는 점에서 requireView()는 어떠신가용?
| MutableSharedFlow( | ||
| replay = 1, | ||
| ) |
There was a problem hiding this comment.
저희 fragment가 싱글톤이고 화면 회전도 없어서 불필요한 이벤트 발생은 없을 것 같네요 😮
| val success: LiveData<Unit> = _success.asLiveData() | ||
| val successFlow = _success.asSharedFlow() |
There was a problem hiding this comment.
요친구만 LiveData로 하신 이유를 알 수 있을까요?
| appVersion = appVersion, | ||
| onPolicyClick = onPolicyClick, | ||
| onContactUsClick = onContactUsClick, | ||
| modifier = Modifier.background(color = FestabookColor.white), |
There was a problem hiding this comment.
background를 따로 주신 이유가 궁금합니다.
There was a problem hiding this comment.
테스트 코드에 전체적으로 assertAll이 있으면 좋을 것 같아요!
There was a problem hiding this comment.
@get:Rule
val instantTaskExecutorRule = InstantTaskExecutorRule()라이브데이터가 사라져서 요친구도 이제 없어도 될 듯해요!
#️⃣ 이슈 번호
🛠️ 작업 내용
🙇🏻 중점 리뷰 요청
기존 SingleLiveData을 SharedFlow의 replayCache = 1로 설정하여 최대 한 번의 이벤트는 유실되지 않게끔 처리하였습니다.
(Channel의 경우 다른 곳에서 이벤트를 소비할 경우 동작하지 않습니다)
ObserveAsEvent는 해당 PR 에서도 같은 코드를 사용하고 있습니다. PR이 머지가 된다면 해당 코드를 적용하겠습니다.
학생회 페이지에서 푸시 알림 테스트는 통과하였습니다만...혹시 모르니 한 번씩 테스트 부탁드립니다.
📸 이미지 첨부 (Optional)
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.