Feature: Add OrganizationsScreen to Profile#14
Conversation
- Create `OrganizationsScreen` with Material 3 styling - Create `OrganizationsViewModel` and `OrganizationsUiState` to manage screen state - Update `ProfileScreen` to navigate to `OrganizationsScreen` - Update `App.kt` and `NavRoutes.kt` with the new route - Update `ProfileUiState` and `ProfileViewModel` to include organization description - Add necessary string resources
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a new Organizations screen, adding the required navigation routes, UI components, and a ViewModel to fetch and display user organizations. The reviewer identified a high-severity issue where ViewModels are instantiated directly within navigation composables, potentially causing resource leaks. Additionally, suggestions were made to improve list performance with unique keys, enhance accessibility for organization avatars, and optimize resource usage by sharing the API client instance across the application.
| } | ||
| composable<Route.Organizations> { | ||
| OrganizationsScreen( | ||
| viewModel = OrganizationsViewModel(), |
There was a problem hiding this comment.
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.
| contentPadding = androidx.compose.foundation.layout.PaddingValues(Dimens.SpacingMedium), | ||
| verticalArrangement = Arrangement.spacedBy(Dimens.SpacingSmall) | ||
| ) { | ||
| items(state.orgs) { org -> |
There was a problem hiding this comment.
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.
| items(state.orgs) { org -> | |
| items(state.orgs, key = { it.id }) { org -> |
| ) { | ||
| AsyncImage( | ||
| model = org.avatarUrl, | ||
| contentDescription = null, |
There was a problem hiding this comment.
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.
| contentDescription = null, | |
| contentDescription = org.login, |
| import kotlinx.coroutines.launch | ||
|
|
||
| class OrganizationsViewModel : ViewModel() { | ||
| private val apiClient = GitHubApiClient(createTokenStorage()) |
There was a problem hiding this comment.
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.
…ons-screen-5274895228509985874 # Conflicts: # composeApp/src/commonMain/kotlin/dev/therealashik/github/App.kt # composeApp/src/commonMain/kotlin/dev/therealashik/github/NavRoutes.kt # composeApp/src/commonMain/kotlin/dev/therealashik/github/profile/ProfileScreen.kt
1 participant
Wired up the "Organizations" row in
ProfileScreenso it navigates to a newOrganizationsScreenthat displays the authenticated user's GitHub organizations in a Material 3 styled, grouped list.PR created automatically by Jules for task 5274895228509985874 started by @TheRealAshik