Skip to content

Conversation

@HI-JIN2
Copy link
Member

@HI-JIN2 HI-JIN2 commented Apr 9, 2025

Summary

로딩바를 하려다가... 그러면 UiState에서 Loading을 구분해야하는데 ... 생각해보니 IntroActivity에서는 로딩을 할게 없더라구요. 일단 best practice를 하나 만들어 두면 좋을 것 같아서, IntroActivity IntroViewModel을 선두로 유리님이 언급하신 SharedFlow를 적용해봤습니다.

분기처리는 크게

  1. 토큰이 없는 경우 -> NoValidToken -> LoginAcitivity
  2. 토큰이 있지만 유효하지 않은 경우 -> NoValidToken -> LoginActivity
  3. 토큰이 있고 유효한 경우 -> Success -> MainActivity

기존에는 Intro에서 토큰 유효성 검증은 안하고 토큰 유무만 확인해었습니다!

To reviewers

  • Handler로 2초 지연하는건 불필요할 거 같아서 뺐어요. uiState 변화 감지해서 화면 이동하는게 맞다고 생각해서 지웠습니다!
  • 말씀하신 SharedFlow가 이게 맞나요??
  • 그리고 UiState를 공통 구조를 하나 만들어 놓고 상속해서 쓰는게 나을까요?? 아니면 이렇게 각 화면마다 따로 정의하는게 나을까요? (에러를 빼게 되면 공통되는 구조는 딱히 없지 않을까 싶습니다..!)
  • 엑세스 토큰이 유효한지 체크하는 api는 아직 없어서 boolean을 반환하는 api로 일단 꼽아뒀습니다!
  • 토큰 인터셉터에서 404 500처리를 잘못해서 이상한 오류가 나는건 아닌가 싶어서, 불필요한 처리가 저기 있는 것 같아 일단 주석처리 했습니다!

@HI-JIN2 HI-JIN2 requested a review from Copilot April 9, 2025 07:35
@HI-JIN2 HI-JIN2 self-assigned this Apr 9, 2025
@HI-JIN2 HI-JIN2 requested a review from kangyuri1114 April 9, 2025 07:35
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

private val userRepository: UserRepository,
) {
suspend operator fun invoke(): Flow<BaseResponse<Boolean>> =
userRepository.checkUserNameValidation("qkqh") //todo api 만들어지면 수정
Copy link

Copilot AI Apr 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The use-case currently calls 'checkUserNameValidation', which does not align with token validation. Update the repository method to reflect token validation once the proper API is available.

Copilot uses AI. Check for mistakes.
Comment on lines 68 to 71
sealed class IntroUiState {
object Loading : IntroUiState()
object Success : IntroUiState()
object NoValidToken : IntroUiState()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이렇게 모든 화면에 대해서 UIState를 만들면 모든 ViewModel에서 같은 구조의 클래스(로딩, 성공, 실패, 초기 등)을 만드는 게 귀찮을 거 같긴한데..

최상위 uistate를 만드는 건 어떨까요..?

sealed interface UiState<out T> {
    object Initial : UiState<Nothing>

    object Loading : UiState<Nothing>

    data class Success<out T>(
        val data: T? = null,
    ) : UiState<T>

    data class Failure(
        val errorMessage: String,
    ) : UiState<Nothing>
}

이 인터페이스를 사용해서 제네릭 부분에 각 화면의 state를 넣어주는 거에요
전 이렇게 한번 해봤던 적이 있어서,,!

NoValidToken 같은 각 화면에 해당하는 state는 IntroState로 따로 만들어서

sealed class IntroState {
    object NoValidToken : IntroState()
    object ValidToken : IntroState()
}

이런식으로 사용하면 어떨까요??

private val _uiState = MutableStateFlow<UiState<IntroState>>(UiState.Loading)
val uiState: StateFlow<UiState<IntroState>> = _uiState.asStateFlow()

그럼 메뉴화면에서도 uistate 금방 적용하지 않을까 싶은데.. 로딩이라도 일단?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오오 좋습니다. UiState는 저렇게 제네릭으로 쓰고 UiEvent는 일단 Toast만 해서 이것도 돌려 쓸 수 있게 바꿔볼게요!

근데 한가지 고민은, UiState의 Failure(=Error)와 UiEvent의 Toast가 결국은 같은 에러 상황에서 쓰이는건데 UiState랑 UiEvent를 같은 상황에 대해서 둘다 써야할까요?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UiEvent에 Toast를 넣은 이유는 성공의 경우에도 토스트를 띄우는 액션이 있어서 그렇게 했습니다. 딱히 성공/실패랑 연관이 있는 액션은 아니라고 생각해서요!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

넵 저도 그렇게 생각해요! 모든 에러 경우에서 토스트를 띄우는 상황이 아니라면 uistate 와는 별개로 쓰는 게 좋을 것 같습니다!
지금 반영해주신 구조가 좋을 것 같습니다 👍

Comment on lines 39 to 41
is IntroUiState.NoValidToken -> {
// 로그인 액티비티로 이동
startActivity<LoginActivity>()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when (val state = uiState) {
    is UiState.Loading -> 그냥 뷰 보여주기
    is UiState.Failure -> showToast(state.errorMessage)
    is UiState.Success -> {
        when (state.data) {
            IntroState.ValidToken -> startActivity<MainActivity>()
            IntroState.NoValidToken -> startActivity<LoginActivity>()
        }
    }
    UiState.Initial -> Unit
}

아래 처럼 적용하면 여기도 이런 식으루?

Copy link
Member Author

@HI-JIN2 HI-JIN2 Apr 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

음... 근데 NoValidToken는 성공은 아니라서.. 모델링 이거 어렵네요ㅜㅜ

Comment on lines 74 to 76
sealed class IntroUiEvent {
data class ShowToast(val error: String) : IntroUiEvent()
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

그리고 이벤트 분리하자는건 이런 식 맞습니다!! 굿

@kangyuri1114
Copy link
Member

404 500처리 혹시 모르니 완전 주석처리 보다는 조건문 안에 로그라도 두는 거 어떨까요

@HI-JIN2
Copy link
Member Author

HI-JIN2 commented Apr 10, 2025

404 500처리 혹시 모르니 완전 주석처리 보다는 조건문 안에 로그라도 두는 거 어떨까요

404랑 500은 토큰 이슈랑은 정말 상관이 없는 문제 같긴한데.. 로그는 남겨 두겠습니다.

@HI-JIN2 HI-JIN2 requested a review from kangyuri1114 April 10, 2025 06:24
@HI-JIN2
Copy link
Member Author

HI-JIN2 commented Apr 10, 2025

@kangyuri1114 최종 코드 두었습니다!
UiState 안에 Success를 제네릭으로 끼우다보니 뷰에 성공일 때 넘겨줄 데이터가 없어도 해당 화면의 State를 정의해놔야하더라구요... 그래서 IntroState에는 ValidToken 하나 두었습니다.

그리고 NoValideToken은 토큰이 있는데 유효하지 않은 경우인데요, 이 경우나 아예 토큰이 없는 경우이나 차피 로그인 화면으로 가야하는건 똑같고, 또 NoValideToken은 Success로 정의하는건 아닌 것 같아서 그냥 공통으로 Error로 두었습니다.

그리고 공통 구조에서 Error시 메시지 전달은 뺐는데, 그 message의 역할이 없다고 생각해서 뺐어요. 에러 메시지는 차피 UiEvent의 토스트를 통해서 보여줄거라서 필요 없을 것 같은데, 유리님은 어떻게 생각하시낭용??!?

if (it.result == true) { //토큰이 있고 유효함
_uiState.value = UiState.Success(IntroState.ValidToken)
} else { //토큰이 있어도 유효하지 않음
_uiState.value = UiState.Error
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저도 여기 Error 로 두는 것 좋은 것 같아요~

@HI-JIN2 HI-JIN2 merged commit 8e008c6 into develop Apr 12, 2025
1 check passed
@HI-JIN2 HI-JIN2 deleted the feat/loading-bar branch April 12, 2025 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants