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
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,7 @@
<!-- Profile Navigation List -->
<string name="nav_repositories">Repositories</string>
<string name="nav_organizations">Organizations</string>
<string name="orgs_empty_state">No organizations found</string>
<string name="nav_starred">Starred</string>
<string name="nav_projects">Projects</string>

Expand Down
11 changes: 10 additions & 1 deletion composeApp/src/commonMain/kotlin/dev/therealashik/github/App.kt
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.OrganizationsScreen
import dev.therealashik.github.profile.OrganizationsViewModel
import dev.therealashik.github.profile.StarredScreen
import dev.therealashik.github.profile.StarredViewModel
import dev.therealashik.github.repository.RepositoryListScreen
Expand Down Expand Up @@ -48,7 +50,14 @@ fun App() {
onBack = { navController.popBackStack() },
onNavigateToRepositories = { navController.navigate(Route.Repositories) },
onNavigateToStarred = { navController.navigate(Route.Starred) },
onNavigateToSettings = { navController.navigate(Route.Settings) }
onNavigateToSettings = { navController.navigate(Route.Settings) },
onNavigateToOrganizations = { navController.navigate(Route.Organizations) }
)
}
composable<Route.Organizations> {
OrganizationsScreen(
viewModel = OrganizationsViewModel(),
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 is problematic. This causes a new instance to be created on every recomposition of the navigation destination. Furthermore, because this instance is not managed by a ViewModelStore, its onCleared() method will never be called, leading to resource leaks (e.g., the apiClient and its HttpClient will remain open). You should use a proper ViewModel provider like viewModel() from the androidx.lifecycle:lifecycle-viewmodel-compose library to ensure the instance is correctly scoped to the backstack entry and its lifecycle is managed.

onBack = { navController.popBackStack() }
)
}
composable<Route.Starred> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,5 +11,6 @@ sealed interface Route {
@Serializable data object NotificationOptions : Route
@Serializable data object CodeOptions : Route
@Serializable data object AddPat : Route
@Serializable data object Organizations : Route
@Serializable data object Starred : Route
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,155 @@
package dev.therealashik.github.profile

import androidx.compose.foundation.layout.Arrangement
import androidx.compose.foundation.layout.Box
import androidx.compose.foundation.layout.Column
import androidx.compose.foundation.layout.Row
import androidx.compose.foundation.layout.Spacer
import androidx.compose.foundation.layout.fillMaxSize
import androidx.compose.foundation.layout.fillMaxWidth
import androidx.compose.foundation.layout.height
import androidx.compose.foundation.layout.padding
import androidx.compose.foundation.layout.size
import androidx.compose.foundation.layout.width
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.material3.Button
import androidx.compose.material3.Card
import androidx.compose.material3.CardDefaults
import androidx.compose.material3.CircularProgressIndicator
import androidx.compose.material3.ExperimentalMaterial3Api
import androidx.compose.material3.Icon
import androidx.compose.material3.IconButton
import androidx.compose.material3.MaterialTheme
import androidx.compose.material3.Scaffold
import androidx.compose.material3.Text
import androidx.compose.material3.TopAppBar
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 coil3.compose.AsyncImage
import dev.therealashik.github.Dimens
import github.composeapp.generated.resources.Res
import github.composeapp.generated.resources.content_description_back
import github.composeapp.generated.resources.nav_organizations
import github.composeapp.generated.resources.orgs_empty_state
import github.composeapp.generated.resources.retry
import org.jetbrains.compose.resources.stringResource

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

Scaffold(
topBar = {
TopAppBar(
title = { Text(stringResource(Res.string.nav_organizations)) },
navigationIcon = {
IconButton(onClick = onBack) {
Icon(
imageVector = Icons.AutoMirrored.Filled.ArrowBack,
contentDescription = stringResource(Res.string.content_description_back)
)
}
}
)
}
) { innerPadding ->
Box(
modifier = Modifier
.fillMaxSize()
.padding(innerPadding)
) {
when (val state = uiState) {
is OrganizationsUiState.Loading -> {
CircularProgressIndicator(modifier = Modifier.align(Alignment.Center))
}
is OrganizationsUiState.Error -> {
Column(
modifier = Modifier.align(Alignment.Center),
horizontalAlignment = Alignment.CenterHorizontally
) {
Text(
text = state.message,
color = MaterialTheme.colorScheme.error
)
Spacer(modifier = Modifier.height(Dimens.SpacingMedium))
Button(onClick = { viewModel.loadData() }) {
Text(stringResource(Res.string.retry))
}
}
}
is OrganizationsUiState.Success -> {
if (state.orgs.isEmpty()) {
Text(
text = stringResource(Res.string.orgs_empty_state),
style = MaterialTheme.typography.bodyLarge,
modifier = Modifier.align(Alignment.Center)
)
} else {
LazyColumn(
modifier = Modifier.fillMaxSize(),
contentPadding = androidx.compose.foundation.layout.PaddingValues(Dimens.SpacingMedium),
verticalArrangement = Arrangement.spacedBy(Dimens.SpacingSmall)
) {
items(state.orgs) { org ->
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 highly recommended to provide a unique key for items in a LazyColumn. This helps the Compose runtime to efficiently track item positions during list updates and preserves the state of individual items (like scroll position or focus) when the list changes.

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

OrgItem(org = org)
}
}
}
}
}
}
}
}

@Composable
private fun OrgItem(org: OrgSummary) {
Card(
modifier = Modifier.fillMaxWidth(),
colors = CardDefaults.cardColors(
containerColor = MaterialTheme.colorScheme.surfaceVariant
)
) {
Row(
modifier = Modifier
.fillMaxWidth()
.padding(Dimens.SpacingMedium),
verticalAlignment = Alignment.CenterVertically
) {
AsyncImage(
model = org.avatarUrl,
contentDescription = null,
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

For better accessibility, consider providing a meaningful contentDescription for the organization avatar instead of null. Using the organization's login name is a good practice for screen readers.

Suggested change
contentDescription = null,
contentDescription = org.login,

modifier = Modifier
.size(Dimens.IconSizeHuge)
.clip(CircleShape)
)
Spacer(modifier = Modifier.width(Dimens.SpacingMedium))
Column {
Text(
text = org.login,
style = MaterialTheme.typography.titleMedium,
color = MaterialTheme.colorScheme.onSurfaceVariant
)
if (!org.description.isNullOrEmpty()) {
Spacer(modifier = Modifier.height(Dimens.SpacingExtraSmall))
Text(
text = org.description,
style = MaterialTheme.typography.bodyMedium,
color = MaterialTheme.colorScheme.onSurfaceVariant
)
}
}
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
package dev.therealashik.github.profile

sealed class OrganizationsUiState {
data object Loading : OrganizationsUiState()
data class Error(val message: String) : OrganizationsUiState()
data class Success(val orgs: List<OrgSummary>) : OrganizationsUiState()
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
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 OrganizationsViewModel : 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.

medium

Creating a new GitHubApiClient (which internally creates a new HttpClient) inside every ViewModel is inefficient. HttpClient is a heavy resource that should be shared across the entire application to benefit from connection pooling and reduced overhead. Consider using dependency injection or a singleton pattern to provide a shared instance of the API client or the underlying HTTP client.

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

init {
loadData()
}

fun loadData() {
viewModelScope.launch {
_uiState.value = OrganizationsUiState.Loading
apiClient.getUserOrgs()
.onSuccess { orgs ->
_uiState.value = OrganizationsUiState.Success(
orgs.map { o ->
OrgSummary(
id = o.id,
login = o.login,
avatarUrl = o.avatarUrl,
description = o.description
)
}
)
}
.onFailure { error ->
_uiState.value = OrganizationsUiState.Error(error.message ?: "Unknown error")
}
}
}

override fun onCleared() {
super.onCleared()
apiClient.close()
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,8 @@ fun ProfileScreen(
onBack: () -> Unit,
onNavigateToRepositories: () -> Unit,
onNavigateToStarred: () -> Unit,
onNavigateToSettings: () -> Unit = {}
onNavigateToSettings: () -> Unit,
onNavigateToOrganizations: () -> Unit
) {
val uiState by viewModel.uiState.collectAsState()

Expand Down Expand Up @@ -107,7 +108,8 @@ fun ProfileScreen(
NavigationListSection(
state = state,
onNavigateToRepositories = onNavigateToRepositories,
onNavigateToStarred = onNavigateToStarred
onNavigateToStarred = onNavigateToStarred,
onNavigateToOrganizations = onNavigateToOrganizations
)
}
}
Expand Down Expand Up @@ -352,7 +354,8 @@ private fun PopularReposSection(popularRepos: List<PopularRepo>) {
private fun NavigationListSection(
state: ProfileUiState.Success,
onNavigateToRepositories: () -> Unit,
onNavigateToStarred: () -> Unit
onNavigateToStarred: () -> Unit,
onNavigateToOrganizations: () -> Unit
) {
Column {
NavigationItem(
Expand Down Expand Up @@ -393,7 +396,7 @@ private fun NavigationListSection(
},
label = stringResource(Res.string.nav_organizations),
count = state.orgs.size,
onClick = { /* TODO */ }
onClick = onNavigateToOrganizations
)
NavigationItem(
icon = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,5 +34,6 @@ data class PopularRepo(
data class OrgSummary(
val id: Long,
val login: String,
val avatarUrl: String
val avatarUrl: String,
val description: String? = null
)
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ class ProfileViewModel : ViewModel() {
language = repo.language
)
},
orgs = orgs.map { OrgSummary(it.id, it.login, it.avatarUrl) },
orgs = orgs.map { OrgSummary(it.id, it.login, it.avatarUrl, it.description) },
starredCount = starred.size,
statusEmoji = status?.emoji,
statusMessage = status?.message
Expand Down
Loading