Infinite loop encountered using NavHost and viewModel

Support
1

Good morning,

I’m currently trying to solve the Bookshelf app project proposed by Google.
I think I didn’t really understand some concepts and that’s the reason why I encountered the following issue.

The idea of the app is to download a list of books with an API, to display the images of the books on the screen and beeing able to click on one of them to display the book information.

I’m able to download the books and display them on the screen. However, when I click on one of them, It seems that an infinite loop starts and “getBook” is always called. It seems that the bookUiState is always “Loading” but I suppose it’s because of the infinite loop.

Could you help me ?
Do you have some advice of “good practices” ?

Here is my code, If you think that the issue is coming from another place I’ll post my entire code, but I think it’ll be in this part.

BookshelfNavHost

@Composable
fun BookshelfNavHost(viewModel : BookshelfViewModel,
                     navController: NavHostController,
                     modifier: Modifier = Modifier) {


    NavHost(
        navController = navController,
        startDestination = AppScreen.Home.name,
        modifier = modifier
    ) {
        //val viewModel : BookshelfViewModel = viewModel(factory = BookshelfViewModel.Factory)

        composable(route = AppScreen.Home.name) {
            HomeScreen(
                bookshelfUiState = viewModel.bookshelfUiState,
                onClick = {bookId ->
                    Log.d("NavHost", viewModel.getBook().toString())
                    viewModel.updateCurrentSelectedBookId(bookId)
                    viewModel.getBook()
                    Log.d("NavHost", viewModel.getBook().toString())
                    navController.navigate(AppScreen.Details.name)}
            )
        }

        composable(route = AppScreen.Details.name) {

            Log.d("NavHost", viewModel.getBook().toString())
            DetailsScreen(viewModel.selectedBookId,
                bookUiState = viewModel.bookUiState,
            )
        }
    }

}

BookshelfViewModel.kt

sealed interface BookshelfUiState {
    data class Success(val bookshelf: Bookshelf?) : BookshelfUiState
    object Error : BookshelfUiState
    object Loading : BookshelfUiState
}

sealed interface BookUiState {
    data class Success(val book: Book?) : BookUiState
    object Error : BookUiState
    object Loading : BookUiState
}



class BookshelfViewModel(private val bookshelfRepository: BookshelfRepository
) : ViewModel() {
    /** The mutable State that stores the status of the most recent request */
    var bookshelfUiState: BookshelfUiState by mutableStateOf(BookshelfUiState.Loading)
        private set

    var bookUiState: BookUiState by mutableStateOf(BookUiState.Loading)
        private set

    var selectedBookId : String by mutableStateOf("")

    init {
        getBookshelf()
    }

    fun getBookshelf() {
        viewModelScope.launch {
            bookshelfUiState = BookshelfUiState.Loading
            bookshelfUiState = try {
                BookshelfUiState.Success(bookshelfRepository.getBookshelf())
            } catch (e: IOException) {
                BookshelfUiState.Error
            } catch (e: HttpException) {
                BookshelfUiState.Error
            }
        }
    }

    fun getBook() {
        Log.d("getBook", selectedBookId)
        viewModelScope.launch {
            bookUiState = BookUiState.Loading
            bookUiState = try {
                BookUiState.Success(bookshelfRepository.getBook(selectedBookId))
            } catch (e: IOException) {
                BookUiState.Error
            } catch (e: HttpException) {
                BookUiState.Error
            }
        }

    }
    

    fun updateCurrentSelectedBookId(newSelectedBookId: String) {
        selectedBookId = newSelectedBookId
    }

    companion object {
        val Factory: ViewModelProvider.Factory = viewModelFactory {
            initializer {
                val application = (this[APPLICATION_KEY] as BookshelfApplication)
                val bookshelfRepository = application.container.bookshelfRepository
                BookshelfViewModel(bookshelfRepository = bookshelfRepository)
            }
        }
    }
}

DetailsScreen.kt

@Composable
fun DetailsScreen(bookId : String,
                  bookUiState: BookUiState,
                  modifier : Modifier = Modifier){


    when (bookUiState) {

        is BookUiState.Loading -> {
            Log.d("DetailsScreen", "Loading Screen")
            LoadingScreen(modifier = modifier.fillMaxSize())
        }
        is BookUiState.Success ->

            if (bookUiState.book != null) {
                Log.d("DetailsScreen", bookUiState.book.toString())
                DetailsScreenContent(bookUiState.book?.volumeInfo!!,
                    modifier = modifier.fillMaxWidth())
            } else {
                ErrorScreen( modifier = modifier.fillMaxSize())
            }

        is BookUiState.Error -> ErrorScreen( modifier = modifier.fillMaxSize())
    }


}
2

I don’t know Compose, so I apologize if I say something wrong, but I believe there are multiple flaws in your code:

1. Composable functions are executed on each recomposition, so potentially tens times a second. Because of that we should not invoke any operations from them as you do (viewModel.getBook()).
2. getBook() and getBookshelf() are counter-intuitive. They sound like they acquire and return some data, but they don’t - they only schedule background operations and return nothing.
3. Apparently, the above confused even yourself as you log viewModel.getBook().toString() in multiple places, even if it doesn’t return anything.
4. Invoking viewModel.getBook() 3 times in onClick {} means you launch 3 separate background operations for acquiring a book.
5. “Infinite loop” may be caused by the fact that each recomposition triggers loading of a book and loading of a book causes recomposition. However, if I understand how Compose works (I definitely don’t!), then changing of a book should recompose only the DetailsScreen and getBook() is triggered outside of it, so such cyclic dependency probably shouldn’t happen here. But I’m not really sure.
6. As far as I understand the reactive UI concept, we should generally don’t invoke reloading of data manually as you do. When you update the selected book id, view model should know it should start reloading the BookUiState. View (composable functions) should only change the selected book id and should observe state object from the view model, but it shouldn’t invoke functions like getBook() or getBookshelf(). Let someone correct me if I’m wrong.

1 Like
3

Sorry for the late answer,

I don’t understand really why but the issue is gone.

  1. From my understanding, the composable fuunctions are executed only when needed (user interaction or an updated value), I suppose you are talking about the call viewModel.getBook() in the log.d function, in that situation I agree on the fact I should not have written that.
  2. I understand what you mean and it’s a good remark, but some solutions I saw on Github have written this like that, and it’s also how the Mars app is made on android, but yes the names could have been different.
  3. Completely agree, in fact I was more interested in knowing where the bugs were without thinking about the meaning of what I asked for. I was a bit frustrated.
  4. Yes but I added the log.d function as an old manner I should have done that with the debugger but you’re right.
  5. Not sure of what you mean. Or are you talking about what I with the call to “viewModel.getBook()” in the compose function (log.d) and Yes probably right. Indeed, it’s what caused the issue, the reason why I did this was because I didn’t understand why the detailsScreen didn’t work. Then I changed some parts of the code and the log.d was not well placed then the issue occured.
  6. I don’t really understand your point. Do you mean that I should call the getBook()/getBookshelf in the end of update book ?
4

5. I meant that if inside a composable function we invoke another function which causes a data reload, it may cause a kind of infinite loop. Recomposition results in reloading of data and reloading of data causes a recomposition. But I don’t know if this is/was happening for you.

6. Again, I’m not at an expert in Compose or MVVM, but I think your view should only change the selected book id and then the view-model would automatically reload the book details accordingly. This way view doesn’t know the logic of the data loading, this logic is implemented entirely in the view-model.