Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package com.example.fruitties.android.ui

import androidx.compose.foundation.clickable
import androidx.compose.foundation.layout.Arrangement
import androidx.compose.foundation.layout.Box
import androidx.compose.foundation.layout.Column
import androidx.compose.foundation.layout.PaddingValues
import androidx.compose.foundation.layout.Row
Expand All @@ -27,10 +28,10 @@ import androidx.compose.foundation.layout.fillMaxWidth
import androidx.compose.foundation.layout.padding
import androidx.compose.foundation.layout.width
import androidx.compose.foundation.lazy.LazyColumn
import androidx.compose.foundation.lazy.items
import androidx.compose.material.icons.Icons
import androidx.compose.material.icons.filled.ShoppingCart
import androidx.compose.material3.Button
import androidx.compose.material3.CircularProgressIndicator
import androidx.compose.material3.ExperimentalMaterial3Api
import androidx.compose.material3.FloatingActionButton
import androidx.compose.material3.Icon
Expand All @@ -46,10 +47,14 @@ import androidx.compose.ui.Alignment
import androidx.compose.ui.Modifier
import androidx.compose.ui.input.nestedscroll.nestedScroll
import androidx.compose.ui.res.stringResource
import androidx.compose.ui.text.style.TextAlign
import androidx.compose.ui.text.style.TextOverflow
import androidx.compose.ui.tooling.preview.Preview
import androidx.compose.ui.unit.dp
import androidx.lifecycle.viewmodel.compose.viewModel
import androidx.paging.LoadState
import androidx.paging.compose.collectAsLazyPagingItems
import androidx.paging.compose.itemKey
import com.example.fruitties.android.LocalAppContainer
import com.example.fruitties.android.R
import com.example.fruitties.model.Fruittie
Expand All @@ -64,6 +69,7 @@ fun ListScreen(
factory = LocalAppContainer.current.mainViewModelFactory,
),
) {
val lazyPagingItems = viewModel.fruittiesPagingData.collectAsLazyPagingItems()
val uiState by viewModel.homeUiState.collectAsState()
val topAppBarScrollBehavior = TopAppBarDefaults.enterAlwaysScrollBehavior()
Scaffold(
Expand Down Expand Up @@ -102,13 +108,45 @@ fun ListScreen(
.padding(paddingValues)
.consumeWindowInsets(paddingValues),
) {
items(items = uiState.fruitties, key = { it.id }) { item ->
FruittieItem(
item = item,
onClick = onFruittieClick,
onAddToCart = viewModel::addItemToCart,
modifier = Modifier.fillMaxWidth(),
)
items(
count = lazyPagingItems.itemCount,
key = lazyPagingItems.itemKey { it.id }
) { index ->
val item = lazyPagingItems[index]
if (item != null) {
FruittieItem(
item = item,
onClick = onFruittieClick,
onAddToCart = viewModel::addItemToCart,
modifier = Modifier.fillMaxWidth(),
)
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

The current implementation handles the append load state, which is great for showing loading/error indicators at the end of the list. However, it's missing handling for the refresh load state. This is crucial for providing feedback to the user during the initial data load or when the list is manually refreshed (e.g., via swipe-to-refresh).

Consider checking lazyPagingItems.loadState.refresh outside the LazyColumn to show a full-screen loading indicator or an error screen with a retry button. This will significantly improve the user experience for initial loading and error recovery scenarios.


item {
when (lazyPagingItems.loadState.append) {
is LoadState.Loading -> {
Box(
modifier = Modifier
.fillMaxWidth()
.padding(16.dp),
contentAlignment = Alignment.Center,
) {
CircularProgressIndicator()
}
}
is LoadState.Error -> {
Text(
text = "Error loading more items",
modifier = Modifier
.fillMaxWidth()
.padding(16.dp),
textAlign = TextAlign.Center,
color = MaterialTheme.colorScheme.error,
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

When an error occurs while loading more items, the user has no way to retry the action. It's good practice to provide a 'Retry' button that calls lazyPagingItems.retry() to allow users to recover from transient errors.

                        Column(
                            modifier = Modifier.fillMaxWidth().padding(16.dp),
                            horizontalAlignment = Alignment.CenterHorizontally,
                            verticalArrangement = Arrangement.spacedBy(8.dp)
                        ) {
                            Text(
                                text = "Error loading more items",
                                textAlign = TextAlign.Center,
                                color = MaterialTheme.colorScheme.error
                            )
                            Button(onClick = { lazyPagingItems.retry() }) {
                                Text("Retry")
                            }
                        }

}
else -> {}
}
}
}
}
Expand Down
5 changes: 2 additions & 3 deletions Fruitties/iosApp/iosApp/ui/ContentView.swift
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,9 @@ struct ContentView: View {
factory: appContainer.value.mainViewModelFactory
)
NavigationStack {
Observing(mainViewModel.homeUiState) { homeUIState in
Observing(mainViewModel.fruittiesPagingData) { pagingData in
List {
ForEach(homeUIState.fruitties, id: \.self) {
value in
ForEach(pagingData, id: \.self) { value in
HStack {
NavigationLink {
FruittieScreen(fruittie: value)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ actual class Factory(
name = dbFile.absolutePath,
).setDriver(BundledSQLiteDriver())
.setQueryCoroutineContext(Dispatchers.IO)
.fallbackToDestructiveMigration(dropAllTables = true)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

critical

fallbackToDestructiveMigration is useful during development, but it's dangerous for production applications as it will delete all user data from the database upon a schema version change. For a production release, you must implement proper Migration paths to preserve user data.

.build()
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,17 +15,21 @@
*/
package com.example.fruitties

import androidx.paging.ExperimentalPagingApi
import androidx.paging.Pager
import androidx.paging.PagingConfig
import androidx.paging.PagingData
import com.example.fruitties.database.AppDatabase
import com.example.fruitties.database.CartDataStore
import com.example.fruitties.model.CartItemDetails
import com.example.fruitties.model.Fruittie
import com.example.fruitties.network.FruittieApi
import com.example.fruitties.paging.FruittieRemoteMediator
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.ExperimentalCoroutinesApi
import kotlinx.coroutines.flow.Flow
import kotlinx.coroutines.flow.map
import kotlinx.coroutines.flow.mapLatest
import kotlinx.coroutines.launch

class DataRepository(
private val api: FruittieApi,
Expand Down Expand Up @@ -53,26 +57,23 @@ class DataRepository(
cartDataStore.remove(fruittie)
}

fun getData(): Flow<List<Fruittie>> {
scope.launch {
if (database.fruittieDao().count() < 1) {
refreshData()
}
}
return loadData()
}
@OptIn(ExperimentalPagingApi::class)
fun getPagingData(): Flow<PagingData<Fruittie>> =
Pager(
config = PagingConfig(
pageSize = 20,
prefetchDistance = 10,
enablePlaceholders = false,
initialLoadSize = 20
),
Comment on lines +63 to +68

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The PagingConfig uses hardcoded 'magic numbers' for pageSize, prefetchDistance, and initialLoadSize. It's a good practice to extract these values into constants (e.g., in a companion object). This improves readability and makes it easier to manage these configuration values in one place.

For example:

companion object {
    private const val PAGE_SIZE = 20
    private const val PREFETCH_DISTANCE = 10
}
Suggested change
config = PagingConfig(
pageSize = 20,
prefetchDistance = 10,
enablePlaceholders = false,
initialLoadSize = 20
),
config = PagingConfig(
pageSize = 20, // Consider using a constant, e.g., PAGE_SIZE
prefetchDistance = 10, // Consider using a constant, e.g., PREFETCH_DISTANCE
enablePlaceholders = false,
initialLoadSize = 20 // Consider using a constant, e.g., PAGE_SIZE
),

remoteMediator = FruittieRemoteMediator(api, database),
pagingSourceFactory = { database.fruittieDao().pagingSource() },
).flow

suspend fun getFruittie(id: Long): Fruittie? = database.fruittieDao().getFruittie(id)

fun fruittieInCart(id: Long): Flow<Int> =
cartDataStore.cart.map { cart ->
cart.items.find { it.id == id }?.count ?: 0
}

fun loadData(): Flow<List<Fruittie>> = database.fruittieDao().getAllAsFlow()

suspend fun refreshData() {
val response = api.getData()
database.fruittieDao().insert(response.feed)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,14 @@ import androidx.room.Database
import androidx.room.RoomDatabase
import androidx.room.RoomDatabaseConstructor
import com.example.fruitties.model.Fruittie
import com.example.fruitties.model.RemoteKeys

@Database(entities = [Fruittie::class], version = 1)
@Database(entities = [Fruittie::class, RemoteKeys::class], version = 2)
@ConstructedBy(AppDatabaseConstructor::class)
abstract class AppDatabase : RoomDatabase() {
abstract fun fruittieDao(): FruittieDao

abstract fun remoteKeysDao(): RemoteKeysDao
}

// The Room compiler generates the `actual` implementations.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
*/
package com.example.fruitties.database

import androidx.paging.PagingSource
import androidx.room.Dao
import androidx.room.Insert
import androidx.room.MapColumn
Expand All @@ -31,9 +32,15 @@ interface FruittieDao {
@Insert(onConflict = OnConflictStrategy.REPLACE)
suspend fun insert(fruitties: List<Fruittie>)

@Query("SELECT * FROM Fruittie ORDER BY id DESC")
fun pagingSource(): PagingSource<Int, Fruittie>

@Query("SELECT * FROM Fruittie")
fun getAllAsFlow(): Flow<List<Fruittie>>

@Query("DELETE FROM Fruittie")
suspend fun clearAll()

@Query("SELECT COUNT(*) as count FROM Fruittie")
suspend fun count(): Int

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
package com.example.fruitties.database

import androidx.room.Dao
import androidx.room.Insert
import androidx.room.OnConflictStrategy
import androidx.room.Query
import com.example.fruitties.model.RemoteKeys

@Dao
interface RemoteKeysDao {
@Insert(onConflict = OnConflictStrategy.REPLACE)
suspend fun insertAll(remoteKey: List<RemoteKeys>)

@Query("SELECT * FROM remote_keys WHERE fruittieId = :id")
suspend fun getRemoteKeyByFruittieId(id: Long): RemoteKeys?

@Query("DELETE FROM remote_keys")
suspend fun clearRemoteKeys()
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import kotlinx.serialization.Serializable
@Serializable
@Entity
data class Fruittie(
@PrimaryKey(autoGenerate = true) val id: Long = 0,
@PrimaryKey val id: Long = 0,
@SerialName("name")
val name: String,
@SerialName("full_name")
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
package com.example.fruitties.model

import androidx.room.Entity
import androidx.room.PrimaryKey
import kotlin.time.Clock
import kotlin.time.ExperimentalTime

@Entity(tableName = "remote_keys")
data class RemoteKeys @OptIn(ExperimentalTime::class) constructor(
@PrimaryKey
val fruittieId: Long,
val prevKey: Int?,
val nextKey: Int?,
val createdAt: Long = Clock.System.now().toEpochMilliseconds()
)
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
package com.example.fruitties.paging

import androidx.paging.ExperimentalPagingApi
import androidx.paging.LoadType
import androidx.paging.PagingState
import androidx.paging.RemoteMediator
import com.example.fruitties.database.AppDatabase
import com.example.fruitties.model.Fruittie
import com.example.fruitties.model.RemoteKeys
import com.example.fruitties.network.FruittieApi

@OptIn(ExperimentalPagingApi::class)
class FruittieRemoteMediator(
private val api: FruittieApi,
private val database: AppDatabase,
) : RemoteMediator<Int, Fruittie>() {
private val fruittieDao = database.fruittieDao()
private val remoteKeysDao = database.remoteKeysDao()

override suspend fun load(
loadType: LoadType,
state: PagingState<Int, Fruittie>,
): MediatorResult {
return try {
val page = when (loadType) {
LoadType.REFRESH -> {
val remoteKeys = getRemoteKeyClosestToCurrentPosition(state)
remoteKeys?.nextKey?.minus(1) ?: 0
}
LoadType.PREPEND -> {
val remoteKeys = getRemoteKeyForFirstItem(state)
val prevKey = remoteKeys?.prevKey
?: return MediatorResult.Success(endOfPaginationReached = remoteKeys != null)
prevKey
}
LoadType.APPEND -> {
val remoteKeys = getRemoteKeyForLastItem(state)
val nextKey = remoteKeys?.nextKey
?: return MediatorResult.Success(endOfPaginationReached = remoteKeys != null)
nextKey
}
}

val response = api.getData(page)
val fruitties = response.feed
val endOfPaginationReached = fruitties.isEmpty() || page >= response.totalPages - 1

if (loadType == LoadType.REFRESH) {
remoteKeysDao.clearRemoteKeys()
fruittieDao.clearAll()
}

val prevKey = if (page == 0) null else page - 1
val nextKey = if (endOfPaginationReached) null else page + 1
val keys = fruitties.map {
RemoteKeys(
fruittieId = it.id,
prevKey = prevKey,
nextKey = nextKey,
)
}
remoteKeysDao.insertAll(keys)
fruittieDao.insert(fruitties)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

critical

The database operations (clearing old data, inserting new data and remote keys) are not performed within a transaction. If an error occurs during one of these operations (e.g., while inserting fruitties), the database could be left in an inconsistent state (e.g., keys inserted but items not, or tables cleared but new data not inserted). All these database modifications should be wrapped in a single atomic transaction using database.withTransaction { ... } to ensure data integrity.

Suggested change
if (loadType == LoadType.REFRESH) {
remoteKeysDao.clearRemoteKeys()
fruittieDao.clearAll()
}
val prevKey = if (page == 0) null else page - 1
val nextKey = if (endOfPaginationReached) null else page + 1
val keys = fruitties.map {
RemoteKeys(
fruittieId = it.id,
prevKey = prevKey,
nextKey = nextKey,
)
}
remoteKeysDao.insertAll(keys)
fruittieDao.insert(fruitties)
database.withTransaction {
if (loadType == LoadType.REFRESH) {
remoteKeysDao.clearRemoteKeys()
fruittieDao.clearAll()
}
val prevKey = if (page == 0) null else page - 1
val nextKey = if (endOfPaginationReached) null else page + 1
val keys = fruitties.map {
RemoteKeys(
fruittieId = it.id,
prevKey = prevKey,
nextKey = nextKey,
)
}
remoteKeysDao.insertAll(keys)
fruittieDao.insert(fruitties)
}


MediatorResult.Success(endOfPaginationReached = endOfPaginationReached)
} catch (e: Exception) {
MediatorResult.Error(e)
}
}

private suspend fun getRemoteKeyClosestToCurrentPosition(state: PagingState<Int, Fruittie>): RemoteKeys? =
state.anchorPosition?.let { position ->
state.closestItemToPosition(position)?.id?.let { id ->
remoteKeysDao.getRemoteKeyByFruittieId(id)
}
}

private suspend fun getRemoteKeyForFirstItem(state: PagingState<Int, Fruittie>): RemoteKeys? =
state.pages.firstOrNull { it.data.isNotEmpty() }?.data?.firstOrNull()?.let { fruittie ->
remoteKeysDao.getRemoteKeyByFruittieId(fruittie.id)
}

private suspend fun getRemoteKeyForLastItem(state: PagingState<Int, Fruittie>): RemoteKeys? =
state.pages.lastOrNull { it.data.isNotEmpty() }?.data?.lastOrNull()?.let { fruittie ->
remoteKeysDao.getRemoteKeyByFruittieId(fruittie.id)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,15 @@ package com.example.fruitties.viewmodel

import androidx.lifecycle.ViewModel
import androidx.lifecycle.viewModelScope
import androidx.paging.PagingData
import androidx.paging.cachedIn
import co.touchlab.kermit.Logger
import com.example.fruitties.DataRepository
import com.example.fruitties.model.Fruittie
import kotlinx.coroutines.flow.Flow
import kotlinx.coroutines.flow.SharingStarted
import kotlinx.coroutines.flow.StateFlow
import kotlinx.coroutines.flow.combine
import kotlinx.coroutines.flow.map
import kotlinx.coroutines.flow.stateIn
import kotlinx.coroutines.launch

Expand All @@ -39,12 +42,14 @@ class MainViewModel(
Logger.v { "MainViewModel cleared" }
}

val fruittiesPagingData: Flow<PagingData<Fruittie>> =
repository.getPagingData().cachedIn(viewModelScope)

val homeUiState: StateFlow<HomeUiState> =
repository
.getData()
.combine(repository.cartDetails) { fruitties, cartState ->
.cartDetails
.map { cartState ->
HomeUiState(
fruitties = fruitties,
cartItemCount = cartState.sumOf { item -> item.count },
)
}.stateIn(
Expand All @@ -64,7 +69,6 @@ class MainViewModel(
* Ui State for the home screen
*/
data class HomeUiState(
val fruitties: List<Fruittie> = listOf(),
val cartItemCount: Int = 0,
)

Expand Down
Loading