Update library UI to conform to MVP release requirements#136
Update library UI to conform to MVP release requirements#136RyanCheung555 wants to merge 1 commit intomainfrom
Conversation
…o route when clicked instead of empty detail screen
📝 WalkthroughWalkthroughThis change introduces dynamic library card state management by adding a new Changes
Sequence DiagramsequenceDiagram
participant Repo as Repository
participant VM as HomeViewModel
participant Screen as HomeScreen
participant Sheet as EcosystemBottomSheetContent
participant UI as Library Card UI
Repo->>VM: libraryFlow emits List<Library>
VM->>VM: Filter excluded libraries
VM->>VM: Map to LibraryCardUiState<br/>(with placeholderRes)
VM->>Screen: libraryCardsFlow StateFlow<br/>(ApiResponse<List<LibraryCardUiState>>)
Screen->>Screen: collectAsStateWithLifecycle()
Screen->>Sheet: Pass libraryCardsApiResponse
Sheet->>Sheet: Render favorites via<br/>libraryCardByPlace
Sheet->>Sheet: Render list via<br/>libraryList function
Sheet->>UI: RoundedImagePlaceCard<br/>(library, placeholderRes)
UI->>Screen: navigateToPlace(library.toPlace())
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip You can disable the changed files summary in the walkthrough.Disable the |
There was a problem hiding this comment.
Pull request overview
Updates the Home ecosystem “Libraries” UI to align with MVP requirements by introducing per-library placeholder images, filtering out unwanted library entries, and routing users to the route screen instead of showing the detailed bottom sheet content for libraries.
Changes:
- Added
LibraryCardUiStateand alibraryCardsFlowto provide UI-ready library data with hard-coded drawable placeholders and excluded-library filtering. - Updated ecosystem bottom sheet to render library cards using the new UI state (including per-library images) and to navigate directly to the route screen on library tap.
- Added a new drawable asset for Mann Library.
Reviewed changes
Copilot reviewed 3 out of 16 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| app/src/main/res/drawable/library_img_mann_library.jpg | Adds a new library image asset used as a placeholder for Mann Library. |
| app/src/main/java/com/cornellappdev/transit/ui/viewmodels/HomeViewModel.kt | Introduces LibraryCardUiState, libraryCardsFlow, image override mapping, and excluded-library filtering applied to libraries and favorites. |
| app/src/main/java/com/cornellappdev/transit/ui/screens/HomeScreen.kt | Plumbs libraryCardsFlow into the bottom sheet content. |
| app/src/main/java/com/cornellappdev/transit/ui/components/home/EcosystemBottomSheetContent.kt | Switches library rendering to LibraryCardUiState, uses per-library placeholder images, and navigates to route options on library selection. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| val libraryCardsFlow: StateFlow<ApiResponse<List<LibraryCardUiState>>> = | ||
| routeRepository.libraryFlow.map { response -> | ||
| when (response) { | ||
| is ApiResponse.Success -> { | ||
| ApiResponse.Success( | ||
| response.data | ||
| .filterNot { it.isExcludedLibrary() } | ||
| .map { it.toLibraryCardUiState() } | ||
| ) | ||
| } | ||
| is ApiResponse.Pending -> ApiResponse.Pending | ||
| is ApiResponse.Error -> ApiResponse.Error | ||
| } |
There was a problem hiding this comment.
libraryCardsFlow repeats the same excluded-library filtering logic that also exists in withExcludedLibrariesRemoved()/Library.isExcludedLibrary(). Consider reusing the shared helper when building libraryCardsFlow so the exclusion behavior stays centralized.
| activeFilter: FilterState, | ||
| onFilterClick: (FilterState) -> Unit, | ||
| staticPlaces: StaticPlaces, | ||
| libraryCardsApiResponse: ApiResponse<List<LibraryCardUiState>>, | ||
| favorites: Set<Place>, |
There was a problem hiding this comment.
The KDoc above EcosystemBottomSheetContent doesn’t mention the new libraryCardsApiResponse parameter, and the staticPlaces description implies libraries come only from staticPlaces. Please update the KDoc to reflect the new source of library UI state.
| staticPlaces: StaticPlaces, | ||
| libraryCardsApiResponse: ApiResponse<List<LibraryCardUiState>>, | ||
| navigateToPlace: (Place) -> Unit, | ||
| navigateToDetails: (DetailedEcosystemPlace) -> Unit, |
There was a problem hiding this comment.
libraryList still takes navigateToDetails, but it’s unused because the call is commented out. Consider removing the parameter (and corresponding call sites) or renaming it to _navigateToDetails until it’s needed again, to avoid unused-parameter warnings and confusion about expected behavior.
| navigateToDetails: (DetailedEcosystemPlace) -> Unit, | |
| _navigateToDetails: (DetailedEcosystemPlace) -> Unit, |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/src/main/java/com/cornellappdev/transit/ui/viewmodels/HomeViewModel.kt (1)
466-515: Match overrides and exclusions on a stable key, notlocation.These tables currently depend on
location.trim().lowercase(), so a backend rename or punctuation tweak will silently pick the wrong placeholder or stop excluding the intended library. Given the planned backend refresh, this is safer if it runs on a stable identifier before the model is converted toPlace.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/cornellappdev/transit/ui/viewmodels/HomeViewModel.kt` around lines 466 - 515, The override/exclusion logic uses mutable location strings causing fragility; change all maps and checks to use a stable identifier (e.g., Library.id or Place.id) instead of location. Replace libraryImageOverridesByLocation and excludedLibraryLocations keys to the stable id values; update Library.isExcludedLibrary(), Place.isExcludedLibraryPlace(), Library.toLibraryCardUiState() and the ApiResponse.withExcludedLibrariesRemoved() filter to reference that stable id field (normalizing only if needed) so lookups use the stable id property rather than location.trim().lowercase().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/src/main/java/com/cornellappdev/transit/ui/components/home/EcosystemBottomSheetContent.kt`:
- Around line 582-587: When handling libraryCardsApiResponse, don't render
nothing for ApiResponse.Pending and ApiResponse.Error: update the when branches
for libraryCardsApiResponse to display a loading indicator when the state is
ApiResponse.Pending (reuse the existing spinner/composable used elsewhere), and
display an "unavailable" error state when the state is ApiResponse.Error (show a
message and optional retry action or placeholder composable). Specifically
modify the ApiResponse.Pending branch to call the existing spinner composable
and the ApiResponse.Error branch to call or create a lightweight
librariesUnavailable/error placeholder (with a retry lambda that re-fetches the
library cards), referencing libraryCardsApiResponse, ApiResponse.Pending and
ApiResponse.Error to locate the change.
In `@app/src/main/java/com/cornellappdev/transit/ui/viewmodels/HomeViewModel.kt`:
- Around line 168-170: Centralize filtering of excluded libraries by extracting
a helper that removes items where isExcludedLibraryPlace() is true and apply it
to the source favorites flow in HomeViewModel (e.g., replace direct uses of
favorites / favoritesFlow with a filteredFavoritesFlow); then update derived
states that currently build from raw favorites — such as
filteredSortedFavorites, ecosystemFavoritesUiState, and where
SearchBarUIState.RecentAndFavorites is constructed — to consume this
filteredFavoritesFlow so excluded library favorites are removed before any UI
state is created.
---
Nitpick comments:
In `@app/src/main/java/com/cornellappdev/transit/ui/viewmodels/HomeViewModel.kt`:
- Around line 466-515: The override/exclusion logic uses mutable location
strings causing fragility; change all maps and checks to use a stable identifier
(e.g., Library.id or Place.id) instead of location. Replace
libraryImageOverridesByLocation and excludedLibraryLocations keys to the stable
id values; update Library.isExcludedLibrary(), Place.isExcludedLibraryPlace(),
Library.toLibraryCardUiState() and the
ApiResponse.withExcludedLibrariesRemoved() filter to reference that stable id
field (normalizing only if needed) so lookups use the stable id property rather
than location.trim().lowercase().
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d96bd720-d94b-4597-a205-124e9890a74d
⛔ Files ignored due to path filters (13)
app/src/main/res/drawable/library_img_africana_studies.jpegis excluded by!**/*.jpegapp/src/main/res/drawable/library_img_carpenter_hall.jpgis excluded by!**/*.jpgapp/src/main/res/drawable/library_img_clark_hall.jpgis excluded by!**/*.jpgapp/src/main/res/drawable/library_img_comstock_hall.jpgis excluded by!**/*.jpgapp/src/main/res/drawable/library_img_ives_hall.jpgis excluded by!**/*.jpgapp/src/main/res/drawable/library_img_lincoln_hall.jpgis excluded by!**/*.jpgapp/src/main/res/drawable/library_img_malott_hall.jpgis excluded by!**/*.jpgapp/src/main/res/drawable/library_img_mann_library.jpgis excluded by!**/*.jpgapp/src/main/res/drawable/library_img_myron_taylor_hall.jpgis excluded by!**/*.jpgapp/src/main/res/drawable/library_img_rand_hall.jpgis excluded by!**/*.jpgapp/src/main/res/drawable/library_img_sage_hall.jpegis excluded by!**/*.jpegapp/src/main/res/drawable/library_img_statler_hall.jpgis excluded by!**/*.jpgapp/src/main/res/drawable/library_img_vet_center.jpgis excluded by!**/*.jpg
📒 Files selected for processing (3)
app/src/main/java/com/cornellappdev/transit/ui/components/home/EcosystemBottomSheetContent.ktapp/src/main/java/com/cornellappdev/transit/ui/screens/HomeScreen.ktapp/src/main/java/com/cornellappdev/transit/ui/viewmodels/HomeViewModel.kt
| when (libraryCardsApiResponse) { | ||
| is ApiResponse.Error -> { | ||
| } | ||
|
|
||
| is ApiResponse.Pending -> { | ||
| } |
There was a problem hiding this comment.
Don't leave the libraries tab blank while loading or after a failure.
Both branches render nothing, so a slow or failed fetch is indistinguishable from “there are no libraries.” At minimum, the Pending case should show the existing spinner, and the Error case should show some unavailable state instead of an empty list.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/src/main/java/com/cornellappdev/transit/ui/components/home/EcosystemBottomSheetContent.kt`
around lines 582 - 587, When handling libraryCardsApiResponse, don't render
nothing for ApiResponse.Pending and ApiResponse.Error: update the when branches
for libraryCardsApiResponse to display a loading indicator when the state is
ApiResponse.Pending (reuse the existing spinner/composable used elsewhere), and
display an "unavailable" error state when the state is ApiResponse.Error (show a
message and optional retry action or placeholder composable). Specifically
modify the ApiResponse.Pending branch to call the existing spinner composable
and the ApiResponse.Error branch to call or create a lightweight
librariesUnavailable/error placeholder (with a retry lambda that re-fetches the
library cards), referencing libraryCardsApiResponse, ApiResponse.Pending and
ApiResponse.Error to locate the change.
| val filteredSortedFavorites = favorites.asSequence() | ||
| .filter { allowedTypes.isEmpty() || it.type in allowedTypes } | ||
| .filterNot { it.isExcludedLibraryPlace() } |
There was a problem hiding this comment.
Apply the exclusion before favorites reach any UI surface.
Line 170 only strips excluded libraries out of ecosystemFavoritesUiState. This same ViewModel still feeds raw favorites into SearchBarUIState.RecentAndFavorites, so previously-favorited excluded libraries will keep showing up in search suggestions while disappearing from the ecosystem sheet. Centralize this filter once instead of hiding them in just one derived state.
Possible extraction
+private fun Set<Place>.withoutExcludedLibraryPlaces(): Set<Place> =
+ filterNot { it.isExcludedLibraryPlace() }.toSet()Then reuse that helper both here and anywhere RecentAndFavorites is built from favoritesFlow.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/java/com/cornellappdev/transit/ui/viewmodels/HomeViewModel.kt`
around lines 168 - 170, Centralize filtering of excluded libraries by extracting
a helper that removes items where isExcludedLibraryPlace() is true and apply it
to the source favorites flow in HomeViewModel (e.g., replace direct uses of
favorites / favoritesFlow with a filteredFavoritesFlow); then update derived
states that currently build from raw favorites — such as
filteredSortedFavorites, ecosystemFavoritesUiState, and where
SearchBarUIState.RecentAndFavorites is constructed — to consume this
filteredFavoritesFlow so excluded library favorites are removed before any UI
state is created.
Overview
Changes Made
Test Coverage
Next Steps
Screenshots
Library Implementation Showcase
library_showcase.webm
Summary by CodeRabbit
New Features
Refactor