Hey, I have a Profile UI where, when the user land...

# codereview

[Vivek Modi]

Hey, I have a Profile UI where, when the user lands on the screen, I want to show a loading indicator while fetching the API response. If the request is successful, I display the user data; if not, I show an error message using a Flow.
In the successful response, the user has three available actions: Delete Account, Change Password, and Logout.
For Delete Account and Change Password, I need to navigate to another Compose screen, and I’m still using event flow for these actions.
For Logout, I need to call an API. While the API call is in progress, I show a loading indicator, and stop it once the process is complete. On success, I clear the token and other relevant user data.
In my ViewModel, I’m calling domain layer interfaces for login and profile operations. The data layer handles API requests, and both repositories return a Result. I combine these results using StateFlow.

[Vivek Modi]

I have a few questions:
1. Is the way I’ve structured UI events and UI states in the UI layer correct?
2. Should I use use-cases in the domain layer to return Flow?
3. Is it okay that my navigation logic is split—half handled in the ViewModel for actions and half in the Composable functions?
4. Should token-clearing logic be handled inside a use-case?
5. Could you please review the code I'm attaching below and suggest any improvements?

[Vivek Modi]

ProfileUiEvent

sealed interface ProfileUiEvent {
    data object OnChangePasswordClick : ProfileUiEvent
    data object DeleteAccountClick : ProfileUiEvent
    data object LogoutClick : ProfileUiEvent
    data class ShowError(val message: String?) : ProfileUiEvent
}

ProfileViewModel

class ProfileViewModel(
    private val userInfoApi: UserInfoApi,
    private val logoutApi: LogoutApi,
    private val tokenDataSource: TokenDataSource,
    private val userSession: UserSessionDataSource,
) : ViewModel() {

    private val eventChannel = Channel<ProfileUiEvent>()
    val events = eventChannel.receiveAsFlow()

    private val loadingState = MutableStateFlow(true)

    val profileUiState = combine(
        flow {
            emit(userInfoApi.getUserInfo()).also {
                loadingState.update { false }
            }
        },
        loadingState,
    ) { userInfo, loadingState ->
        userInfo.fold(
            onSuccess = {
                ProfileUiState(userInfo = it, isLoading = loadingState)
            },
            onFailure = {
                ProfileUiState(isLoading = loadingState)
                eventChannel.send(ProfileUiEvent.ShowError(it.message))
            }
        )
    }.stateIn(
        scope = viewModelScope,
        started = SharingStarted.Lazily,
        initialValue = ProfileUiState(isLoading = true)
    )

    fun onAction(action: ProfileUiEvent) {
        when (action) {
            ProfileUiEvent.LogoutClick -> {
                logoutUser()
            }

            else -> {}
        }
    }

    private fun logoutUser() {
        viewModelScope.launch {
            loadingState.update { true }
            val result = logoutApi.logout()
            loadingState.update { false }
            result.fold(
                onSuccess = {
                    onLogoutSuccess()
                },
                onFailure = {
                    eventChannel.send(ProfileUiEvent.ShowError(it.message))
                }
            )
        }
    }

    private suspend fun onLogoutSuccess() {
        tokenDataSource.clearTokens()
        userSession.logout("Logout")
    }
}

UserInfoApi

interface UserInfoApi {
    suspend fun getUserInfo(): Result<UserInfo>
}

LogoutApi

interface LogoutApi {
    suspend fun logout(): Result<Unit>
}

ProfileScreenRoute

@Composable
fun ProfileScreenRoute(
    onBackPress: () -> Unit,
    onNavigateToChangePassword: () -> Unit,
    onNavigateToDeleteAccount: () -> Unit,
    viewModel: ProfileViewModel = koinViewModel(),
) {
    val context = LocalContext.current
    val currentOnNavigateToChangePassword by rememberUpdatedState(onNavigateToChangePassword)
    val currentOnNavigateToDeleteAccount by rememberUpdatedState(onNavigateToDeleteAccount)
    val profileUiState by viewModel.profileUiState.collectAsStateWithLifecycle()

    ObserveAsEvent(viewModel.events) { event ->
        when (event) {
            is ProfileUiEvent.OnChangePasswordClick -> {
                currentOnNavigateToChangePassword()
            }

            is ProfileUiEvent.DeleteAccountClick -> {
                currentOnNavigateToDeleteAccount()
            }

            is ProfileUiEvent.ShowError -> {
                Toast.makeText(context, event.message, Toast.LENGTH_SHORT).show()
            }

            else -> {}
        }
    }

    BackHandler(onBack = onBackPress)

    ProfileScreen(
        uiState = profileUiState,
        onAction = viewModel::onAction
    )
}

[Pablichjenkov]

Are you using compose-navigation? - if so, can you share the NavGraph setup

[Pablichjenkov]

In general you should follow these rules
1- Each screen should have its own ViewModel.
2- ViewModels do not reference other ViewModels directly.
3- Data from one screen to the other should be shared using the domain layer. You can also use the previousNavBackstackEntry.savedState API for this but using the domain/data layer scales better.

[Pablichjenkov]

https://x.com/marcinmoskala/status/1907719762097885665?t=g5n_DdqU6iaBQrvHtwF1aQ&s=19

[Pablichjenkov]

A visual representation of it, check the red rectangle

[Vivek Modi]

thanks for sharing all those documents. yes I'm using compose navigation library. This is my code for navhost

@Composable
fun AppNavHost(
    startDestination: Any,
    navHostController: NavHostController = rememberNavController()
) {
    val currentActivity = LocalActivity.current

    AppAppScaffold(
        bottomBar = {
            HomeTopLevelNavigation(navHostController)
        }
    ) { contentPadding ->
        CompositionLocalProvider(LocalScaffoldPadding provides ScaffoldPadding(contentPadding)) {
            NavHost(
                navController = navHostController,
                startDestination = startDestination
            ) {

// more routes here                composable<HomeGraphNavigation.ProfileScreen> {
                    ProfileScreenRoute(
                        onBackPress = {
                            currentActivity?.finish()
                        },
                        onNavigateToChangePassword = navHostController::navigateToChangePassword,
                        onNavigateToDeleteAccount = navHostController::navigateToDeleteAccount,
                    )
                }
            }
        }
    }
}

[Vivek Modi]

fun NavController.navigateToChangePassword() {
    navigate(AuthNavigation.ChangePasswordNavigation)
}

[Vivek Modi]

fun NavController.navigateToDeleteAccount() {
    navigate(AuthNavigation.DeleteAccountNavigation)
}

[Pablichjenkov]

Looks good overall, I'd prefer using a class HomeNavigationManager encapsulating the navController rather than using extension functions to navigate to other screens. Is easier to mock when testing

[Vivek Modi]

Do you have an example for this ?

[Pablichjenkov]

Oh sorry not open source but what I mean is simply have a class ProfileNavigationManager(navController, dataManagers, ...)
Then in your navGraph builder function , just pass an instance of it, anytime you want to navigate, instead of calling directly navController.navigate(...) better delegate the where to go next navigation logic to ProfileNavigationManager class. It looks a bit more organized than having this logic in extension functions.
And you have the needed DataManagers to determine what is the next screen all in one place where you can mock all them