Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
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
2 changes: 2 additions & 0 deletions composeApp/src/commonMain/composeResources/values/strings.xml
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,8 @@
<string name="nav_starred">Starred</string>
<string name="nav_projects">Projects</string>

<string name="starred_title">Starred</string>

<!-- Repository List Screen -->
<string name="repo_list_title">Repositories</string>
<string name="repo_owner">TheRealAshik</string>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ import androidx.navigation.compose.composable
import androidx.navigation.compose.rememberNavController
import dev.therealashik.github.profile.ProfileScreen
import dev.therealashik.github.profile.ProfileViewModel
import dev.therealashik.github.profile.StarredScreen
import dev.therealashik.github.profile.StarredViewModel
import dev.therealashik.github.repository.RepositoryListScreen
import dev.therealashik.github.repository.RepositoryListViewModel
import dev.therealashik.github.settings.AddPatScreen
Expand Down Expand Up @@ -45,9 +47,16 @@ fun App() {
viewModel = ProfileViewModel(),
onBack = { navController.popBackStack() },
onNavigateToRepositories = { navController.navigate(Route.Repositories) },
onNavigateToStarred = { navController.navigate(Route.Starred) },
onNavigateToSettings = { navController.navigate(Route.Settings) }
)
}
composable<Route.Starred> {
StarredScreen(
viewModel = StarredViewModel(),
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

Instantiating the ViewModel directly inside the composable block causes a new instance to be created on every recomposition. Furthermore, since it's not obtained through a ViewModelStoreOwner (e.g., using viewModel()), its onCleared() method will never be called, leading to a resource leak of the HttpClient inside GitHubApiClient. Consider using a proper ViewModel provider that respects the navigation backstack lifecycle.

onBack = { navController.popBackStack() }
)
}
composable<Route.Repositories> {
RepositoryListScreen(
viewModel = RepositoryListViewModel(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,4 +11,5 @@ sealed interface Route {
@Serializable data object NotificationOptions : Route
@Serializable data object CodeOptions : Route
@Serializable data object AddPat : Route
@Serializable data object Starred : Route
}
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ fun ProfileScreen(
viewModel: ProfileViewModel,
onBack: () -> Unit,
onNavigateToRepositories: () -> Unit,
onNavigateToStarred: () -> Unit,
onNavigateToSettings: () -> Unit = {}
) {
val uiState by viewModel.uiState.collectAsState()
Expand Down Expand Up @@ -105,7 +106,8 @@ fun ProfileScreen(
HorizontalDivider(color = MaterialTheme.colorScheme.surfaceVariant)
NavigationListSection(
state = state,
onNavigateToRepositories = onNavigateToRepositories
onNavigateToRepositories = onNavigateToRepositories,
onNavigateToStarred = onNavigateToStarred
)
}
}
Expand Down Expand Up @@ -349,7 +351,8 @@ private fun PopularReposSection(popularRepos: List<PopularRepo>) {
@Composable
private fun NavigationListSection(
state: ProfileUiState.Success,
onNavigateToRepositories: () -> Unit
onNavigateToRepositories: () -> Unit,
onNavigateToStarred: () -> Unit
) {
Column {
NavigationItem(
Expand Down Expand Up @@ -410,7 +413,7 @@ private fun NavigationListSection(
},
label = stringResource(Res.string.nav_starred),
count = state.starredCount,
onClick = { /* TODO */ }
onClick = onNavigateToStarred
)
NavigationItem(
icon = {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,146 @@
package dev.therealashik.github.profile

import androidx.compose.foundation.background
import androidx.compose.foundation.layout.*
import androidx.compose.foundation.lazy.LazyColumn
import androidx.compose.foundation.lazy.items
import androidx.compose.foundation.shape.CircleShape
import androidx.compose.material.icons.Icons
import androidx.compose.material.icons.automirrored.filled.ArrowBack
import androidx.compose.material.icons.filled.Star
import androidx.compose.material3.*
import androidx.compose.runtime.Composable
import androidx.compose.runtime.collectAsState
import androidx.compose.runtime.getValue
import androidx.compose.ui.Alignment
import androidx.compose.ui.Modifier
import androidx.compose.ui.draw.clip
import androidx.compose.ui.text.font.FontWeight
import dev.therealashik.github.Dimens
import github.composeapp.generated.resources.Res
import github.composeapp.generated.resources.content_description_back
import github.composeapp.generated.resources.retry
import github.composeapp.generated.resources.starred_title
import org.jetbrains.compose.resources.stringResource

@OptIn(ExperimentalMaterial3Api::class)
@Composable
fun StarredScreen(
viewModel: StarredViewModel,
onBack: () -> Unit
) {
val uiState by viewModel.uiState.collectAsState()

Scaffold(
topBar = {
TopAppBar(
title = { Text(stringResource(Res.string.starred_title)) },
navigationIcon = {
IconButton(onClick = onBack) {
Icon(
imageVector = Icons.AutoMirrored.Filled.ArrowBack,
contentDescription = stringResource(Res.string.content_description_back)
)
}
},
colors = TopAppBarDefaults.topAppBarColors(
containerColor = MaterialTheme.colorScheme.surface,
titleContentColor = MaterialTheme.colorScheme.onSurface,
navigationIconContentColor = MaterialTheme.colorScheme.onSurface
)
)
}
) { innerPadding ->
when (val state = uiState) {
is StarredUiState.Loading -> {
Box(
modifier = Modifier.fillMaxSize().padding(innerPadding),
contentAlignment = Alignment.Center
) { CircularProgressIndicator() }
}
is StarredUiState.Error -> {
Box(
modifier = Modifier.fillMaxSize().padding(innerPadding),
contentAlignment = Alignment.Center
) {
Column(horizontalAlignment = Alignment.CenterHorizontally) {
Text(state.message, color = MaterialTheme.colorScheme.error)
Spacer(modifier = Modifier.height(Dimens.SpacingMedium))
Button(onClick = viewModel::loadData) { Text(stringResource(Res.string.retry)) }
}
}
}
is StarredUiState.Success -> {
LazyColumn(
modifier = Modifier
.fillMaxSize()
.padding(innerPadding),
contentPadding = PaddingValues(vertical = Dimens.SpacingMedium),
verticalArrangement = Arrangement.spacedBy(Dimens.SpacingMedium)
) {
items(state.starredRepos) { repo ->
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

It is recommended to provide a unique key for items in a LazyColumn. This helps Compose maintain the state of items correctly during recompositions and list updates, improving performance.

Suggested change
items(state.starredRepos) { repo ->
items(items = state.starredRepos, key = { it.id }) { repo ->

ElevatedCard(
modifier = Modifier
.fillMaxWidth()
.padding(horizontal = Dimens.SpacingMedium),
colors = CardDefaults.elevatedCardColors(
containerColor = MaterialTheme.colorScheme.surfaceContainerLow
)
) {
Column(
modifier = Modifier
.fillMaxWidth()
.padding(Dimens.SpacingMedium)
) {
Text(
text = repo.name,
style = MaterialTheme.typography.titleMedium.copy(fontWeight = FontWeight.Bold),
color = MaterialTheme.colorScheme.onSurface
)
if (repo.description != null) {
Spacer(modifier = Modifier.height(Dimens.SpacingSmall))
Text(
text = repo.description,
style = MaterialTheme.typography.bodyMedium,
color = MaterialTheme.colorScheme.onSurfaceVariant
)
}
Spacer(modifier = Modifier.height(Dimens.SpacingMedium))
Row(verticalAlignment = Alignment.CenterVertically) {
Icon(
imageVector = Icons.Filled.Star,
contentDescription = null,
tint = MaterialTheme.colorScheme.onSurfaceVariant,
modifier = Modifier.size(Dimens.IconSizeSmall)
)
Spacer(modifier = Modifier.width(Dimens.SpacingExtraSmall))
Text(
text = repo.stars.toString(),
style = MaterialTheme.typography.bodySmall,
color = MaterialTheme.colorScheme.onSurfaceVariant
)

if (repo.language != null) {
Spacer(modifier = Modifier.width(Dimens.SpacingMedium))
Box(
modifier = Modifier
.size(Dimens.IndicatorSize)
.clip(CircleShape)
.background(MaterialTheme.colorScheme.primary)
)
Spacer(modifier = Modifier.width(Dimens.SpacingExtraSmall))
Text(
text = repo.language,
style = MaterialTheme.typography.bodySmall,
color = MaterialTheme.colorScheme.onSurfaceVariant
)
}
}
}
}
}
}
}
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
package dev.therealashik.github.profile

sealed class StarredUiState {
data object Loading : StarredUiState()
data class Error(val message: String) : StarredUiState()
data class Success(val starredRepos: List<StarredRepoItem>) : StarredUiState()
}

data class StarredRepoItem(
val id: String,
val name: String,
val description: String?,
val stars: Int,
val language: String?
)
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
package dev.therealashik.github.profile

import androidx.lifecycle.ViewModel
import androidx.lifecycle.viewModelScope
import dev.therealashik.github.data.GitHubApiClient
import dev.therealashik.github.data.createTokenStorage
import kotlinx.coroutines.flow.MutableStateFlow
import kotlinx.coroutines.flow.StateFlow
import kotlinx.coroutines.flow.asStateFlow
import kotlinx.coroutines.launch

class StarredViewModel : ViewModel() {
private val apiClient = GitHubApiClient(createTokenStorage())
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

Creating a new GitHubApiClient (and thus a new HttpClient) inside every ViewModel is inefficient. HttpClient is a heavy resource that should be shared across the application. Consider injecting a shared instance of the API client or the HTTP client itself.

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

init {
loadData()
}

fun loadData() {
viewModelScope.launch {
_uiState.value = StarredUiState.Loading
apiClient.getStarredRepos(perPage = 100)
.onSuccess { repos ->
_uiState.value = StarredUiState.Success(
repos.map { r ->
StarredRepoItem(
id = r.id.toString(),
name = r.name,
description = r.description,
stars = r.stars,
language = r.language
)
}
)
}
.onFailure { _uiState.value = StarredUiState.Error(it.message ?: "Unknown error") }
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The error message "Unknown error" is hardcoded. It should be moved to a string resource to support internationalization.

}
}

override fun onCleared() {
super.onCleared()
apiClient.close()
}
}
Loading