Open
Conversation
UIState no longer exposes the raw backend list. Filtered highlights are now precomputed in the ViewModel, so the UI can observe state directly without recalculating on every recomposition.
Member
|
The PR description image shows the Highlights screen of the Score Android app, displaying:
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughAdds a Highlights feature: new GraphQL query and schema fields, mappers converting GraphQL results to nullable-dated domain models, a repository fetching and exposing highlights via StateFlow, a Hilt ViewModel producing derived/filterable lists, and multiple UI and navigation updates to consume the new flow and SportSelection type. Changes
Sequence DiagramsequenceDiagram
participant UI as User/UI
participant VM as HighlightsViewModel
participant Repo as HighlightsRepository
participant Apollo as ApolloClient
participant API as GraphQL API
participant Flow as highlightsFlow
UI->>VM: Init / onRefresh()
VM->>Repo: fetchHighlights()
Repo->>Repo: emit Loading
Repo->>Apollo: execute HighlightsQuery (with 5s timeout)
Apollo->>API: GraphQL request (Highlights)
API-->>Apollo: articles + youtubeVideos
Apollo-->>Repo: query result
Repo->>Repo: map items -> HighlightData (toHighlightData, parse dates)
Repo->>Flow: emit Success(highlights)
Flow-->>VM: collect Success
VM->>VM: sort by date, compute filtered/today/past3 lists
VM-->>UI: update UI state
UI->>VM: onSportSelected / onQueryChange / onSearch
VM->>VM: recompute derived lists
VM-->>UI: update UI (filtered results)
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/java/com/cornellappdev/score/screen/HighlightsSubScreen.kt (1)
99-99:⚠️ Potential issue | 🟠 MajorThe
navigateBackparameter is not being used.
HighlightsSubScreenHeaderis called with an empty lambda{}instead of the actualnavigateBackcallback passed toHighlightsSubScreen. This breaks the back navigation functionality.🐛 Proposed fix
- HighlightsSubScreenHeader(header, {}) + HighlightsSubScreenHeader(header, navigateBack = onCloseClick)Note: Using
onCloseClickas it appears to be the intended close/back action. Alternatively, add a dedicatednavigateBackparameter if distinct behavior is needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/cornellappdev/score/screen/HighlightsSubScreen.kt` at line 99, HighlightsSubScreen is passing an empty lambda to HighlightsSubScreenHeader which ignores the provided navigateBack callback; replace the empty lambda with the actual navigateBack callback so the header's onCloseClick (or the intended close/back action) invokes navigateBack. Locate the call to HighlightsSubScreenHeader in HighlightsSubScreen and pass navigateBack (or wire navigateBack into onCloseClick) instead of {} to restore back navigation.
🧹 Nitpick comments (4)
app/src/main/java/com/cornellappdev/score/viewmodel/HighlightsViewModel.kt (1)
41-41: Inconsistent sort order:filteredsorted ascending whileloadedStatedata is descending.Line 72 sorts
response.datadescending by date before callingbuildDerivedLists, but line 41 re-sortsfilteredascending. This inconsistency may cause confusion. Clarify the intended order or align them.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/cornellappdev/score/viewmodel/HighlightsViewModel.kt` at line 41, filtered is currently sorted ascending (val filtered = validHighlights.sortedBy { it.date }) while elsewhere (response.data before buildDerivedLists and loadedState) uses descending order; make the ordering consistent by changing filtered to use descending sorting (e.g., use sortedByDescending { it.date }) or, if ascending is intended, adjust the earlier descending sort in the loadedState/response.data preparation and buildDerivedLists call to ascending—update the filtered declaration and any related assumptions in buildDerivedLists/loadedState to match the chosen order.app/src/main/java/com/cornellappdev/score/model/Highlights.kt (1)
67-71: Gender inference logic may be incomplete.The code assumes
FEMALEif the title doesn't contain "Men's". This doesn't account for:
- Co-ed sports
- Titles without gender indication (e.g., "Cornell Tennis Wins Championship")
- "Women's" explicit match
Consider making
gendernullable or adding explicit "Women's" check:♻️ Proposed alternative
gender = if (title.contains("Men's")) { GenderDivision.MALE - } else { + } else if (title.contains("Women's")) { GenderDivision.FEMALE - }, + } else { + null // Unknown gender + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/cornellappdev/score/model/Highlights.kt` around lines 67 - 71, The gender inference in the Highlights.kt assignment currently sets GenderDivision.FEMALE whenever title does not contain "Men's"; update the logic in the gender assignment (the expression using title.contains("Men's")) to explicitly detect "Men's" -> GenderDivision.MALE, "Women's" (or "Women’s") -> GenderDivision.FEMALE, and common co‑ed indicators (e.g., "Co‑ed", "Coed", "Mixed") -> GenderDivision.COED if you have that enum, otherwise set gender to null; make the gender field nullable in the containing data class/constructor if not already and use case‑insensitive matching on the title string to avoid false negatives.app/src/main/java/com/cornellappdev/score/util/TestingConstants.kt (1)
231-283: Test data withnulldates will be filtered out by ViewModel.All highlights in
highlightsListhavedate = null(e.g., line 237). InHighlightsViewModel.buildDerivedLists(), line 32 filters out highlights with null dates:.filter { it.date != null }. This means these test highlights won't appear intodayHighlightsorpastThreeDaysHighlightsduring preview/testing.Consider using actual
LocalDatevalues for more realistic test coverage:♻️ Proposed fix example
HighlightData.Video (VideoHighlightData( "vs Columbia", "maxresdefault.jpg", "https://cornellsun.com/article/london-mcdavid-is-making-a-name-for-herself-at-cornell", - null, + LocalDate.now(), "11/09", Sport.BASEBALL, GenderDivision.MALE, "0:44" )),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/cornellappdev/score/util/TestingConstants.kt` around lines 231 - 283, The test highlights in highlightsList use null dates so they get filtered out by HighlightsViewModel.buildDerivedLists() (.filter { it.date != null }); update the HighlightData instances (VideoHighlightData and ArticleHighlightData inside highlightsList) to supply concrete LocalDate values (e.g., LocalDate.now() and LocalDate.now().minusDays(1/2/3) as appropriate) instead of null so the preview/test data appears in todayHighlights and pastThreeDaysHighlights; ensure the date field types match the model (LocalDate) and adjust any imports or constructors if needed.app/src/main/java/com/cornellappdev/score/screen/HighlightsScreen.kt (1)
49-52: TODO: Create highlights-specific loading screen.The comment acknowledges that
LoadingScreenwith "Loading Schedules..." text is inappropriate for the highlights feature. Track this for follow-up.Do you want me to open an issue to track creating a highlights-specific loading screen?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/cornellappdev/score/screen/HighlightsScreen.kt` around lines 49 - 52, The HighlightsScreen currently shows a generic LoadingScreen with "Loading Schedules..." in the ApiResponse.Loading branch; replace this with a highlights-specific loading UI by creating a new component (e.g., HighlightsLoadingScreen) or updating LoadingScreen to accept contextual text, then use that from HighlightsScreen (replace the LoadingScreen call in the ApiResponse.Loading branch). Ensure the new component or prop displays "Loading Highlights..." (and any highlights-specific placeholders) and update any references to LoadingScreen in HighlightsScreen to call the new name or pass the appropriate prop so the loading text is correct.
🤖 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/score/model/HighlightsRepository.kt`:
- Around line 26-29: The KDoc at the top of HighlightsRepository incorrectly
mentions "games" and `upcomingGamesFlow`; update the comment to describe
fetching highlights and emitting the repository's highlights flow (e.g.,
`highlightsFlow` or the actual flow property in HighlightsRepository). Edit the
comment block above the asynchronous fetch method or class to reference
highlights, the correct flow name used in this file, and briefly state its
behavior (asynchronous fetch and emission) so the documentation matches
functions like HighlightsRepository.fetch... and the highlights flow identifier.
In `@app/src/main/java/com/cornellappdev/score/screen/HighlightsScreen.kt`:
- Around line 59-61: The pull-to-refresh indicator never shows because
ScorePullToRefreshBox's isRefreshing uses uiState.loadedState ==
ApiResponse.Loading inside the ApiResponse.Success branch; add a dedicated
boolean refresh state and wire it through the ViewModel instead of reusing
loadedState. Update HighlightsUiState to include isRefreshing (or add a separate
loading state), set and clear it from highlightsViewModel.onRefresh()/refresh
handling, and pass uiState.isRefreshing to ScorePullToRefreshBox.isRefreshing so
the indicator can show while content is displayed (referencing
ScorePullToRefreshBox, uiState.loadedState, HighlightsUiState, and
highlightsViewModel.onRefresh).
In `@app/src/main/java/com/cornellappdev/score/util/DateUtil.kt`:
- Around line 72-76: Update the KDoc for the ISO-8601 parsing function in
DateUtil.kt so the `@return` matches the actual return type (nullable LocalDate)
instead of stating LocalDateTime; explicitly document that the function returns
a LocalDate? (nullable) and that it returns null for invalid/unsupported
formats, and keep the format example ("yyyy-MM-dd'T'HH:mm:ss'Z'") and parameter
description unchanged so readers can find the parsing function (DateUtil's
ISO-8601 parsing method) and understand the nullable return contract.
- Around line 82-83: The catch block in DateUtil.kt that currently swallows
exceptions and returns null should instead log a warning before returning null:
update the catch (e: Exception) block inside the date-parsing function to call
your logging facility (e.g., Log.w or Timber.w) with a concise message that
includes the exception and the offending input string/variable (so we can
identify which timestamp failed) and then continue returning null to preserve
behavior.
In `@app/src/main/java/com/cornellappdev/score/viewmodel/HighlightsViewModel.kt`:
- Around line 43-48: pastThreeDaysHighlights currently uses filter { it.date!!
>= threeDaysAgo } which includes today and duplicates items in todayHighlights;
update the filter in HighlightsViewModel.kt to exclude today by requiring the
date be >= threeDaysAgo AND < today (e.g., filter { val d = it.date!!; d >=
threeDaysAgo && d < today }) so pastThreeDaysHighlights covers only the prior 3
days and not today's items.
---
Outside diff comments:
In `@app/src/main/java/com/cornellappdev/score/screen/HighlightsSubScreen.kt`:
- Line 99: HighlightsSubScreen is passing an empty lambda to
HighlightsSubScreenHeader which ignores the provided navigateBack callback;
replace the empty lambda with the actual navigateBack callback so the header's
onCloseClick (or the intended close/back action) invokes navigateBack. Locate
the call to HighlightsSubScreenHeader in HighlightsSubScreen and pass
navigateBack (or wire navigateBack into onCloseClick) instead of {} to restore
back navigation.
---
Nitpick comments:
In `@app/src/main/java/com/cornellappdev/score/model/Highlights.kt`:
- Around line 67-71: The gender inference in the Highlights.kt assignment
currently sets GenderDivision.FEMALE whenever title does not contain "Men's";
update the logic in the gender assignment (the expression using
title.contains("Men's")) to explicitly detect "Men's" -> GenderDivision.MALE,
"Women's" (or "Women’s") -> GenderDivision.FEMALE, and common co‑ed indicators
(e.g., "Co‑ed", "Coed", "Mixed") -> GenderDivision.COED if you have that enum,
otherwise set gender to null; make the gender field nullable in the containing
data class/constructor if not already and use case‑insensitive matching on the
title string to avoid false negatives.
In `@app/src/main/java/com/cornellappdev/score/screen/HighlightsScreen.kt`:
- Around line 49-52: The HighlightsScreen currently shows a generic
LoadingScreen with "Loading Schedules..." in the ApiResponse.Loading branch;
replace this with a highlights-specific loading UI by creating a new component
(e.g., HighlightsLoadingScreen) or updating LoadingScreen to accept contextual
text, then use that from HighlightsScreen (replace the LoadingScreen call in the
ApiResponse.Loading branch). Ensure the new component or prop displays "Loading
Highlights..." (and any highlights-specific placeholders) and update any
references to LoadingScreen in HighlightsScreen to call the new name or pass the
appropriate prop so the loading text is correct.
In `@app/src/main/java/com/cornellappdev/score/util/TestingConstants.kt`:
- Around line 231-283: The test highlights in highlightsList use null dates so
they get filtered out by HighlightsViewModel.buildDerivedLists() (.filter {
it.date != null }); update the HighlightData instances (VideoHighlightData and
ArticleHighlightData inside highlightsList) to supply concrete LocalDate values
(e.g., LocalDate.now() and LocalDate.now().minusDays(1/2/3) as appropriate)
instead of null so the preview/test data appears in todayHighlights and
pastThreeDaysHighlights; ensure the date field types match the model (LocalDate)
and adjust any imports or constructors if needed.
In `@app/src/main/java/com/cornellappdev/score/viewmodel/HighlightsViewModel.kt`:
- Line 41: filtered is currently sorted ascending (val filtered =
validHighlights.sortedBy { it.date }) while elsewhere (response.data before
buildDerivedLists and loadedState) uses descending order; make the ordering
consistent by changing filtered to use descending sorting (e.g., use
sortedByDescending { it.date }) or, if ascending is intended, adjust the earlier
descending sort in the loadedState/response.data preparation and
buildDerivedLists call to ascending—update the filtered declaration and any
related assumptions in buildDerivedLists/loadedState to match the chosen order.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d99ea79c-3618-43a1-80ed-9ea11e23e457
📒 Files selected for processing (18)
app/src/main/graphql/Highlights.graphqlapp/src/main/graphql/schema.graphqlsapp/src/main/java/com/cornellappdev/score/components/highlights/ArticleHighlightsCard.ktapp/src/main/java/com/cornellappdev/score/components/highlights/HighlightsFilter.ktapp/src/main/java/com/cornellappdev/score/components/highlights/HighlightsScreenSearchFilterBar.ktapp/src/main/java/com/cornellappdev/score/components/highlights/HighlightsSearchBar.ktapp/src/main/java/com/cornellappdev/score/components/highlights/VideoHighlightsCard.ktapp/src/main/java/com/cornellappdev/score/model/Highlights.ktapp/src/main/java/com/cornellappdev/score/model/HighlightsRepository.ktapp/src/main/java/com/cornellappdev/score/nav/ScoreNavHost.ktapp/src/main/java/com/cornellappdev/score/nav/root/RootNavigation.ktapp/src/main/java/com/cornellappdev/score/screen/HighlightsScreen.ktapp/src/main/java/com/cornellappdev/score/screen/HighlightsSearchScreen.ktapp/src/main/java/com/cornellappdev/score/screen/HighlightsSubScreen.ktapp/src/main/java/com/cornellappdev/score/screen/HomeScreen.ktapp/src/main/java/com/cornellappdev/score/util/DateUtil.ktapp/src/main/java/com/cornellappdev/score/util/TestingConstants.ktapp/src/main/java/com/cornellappdev/score/viewmodel/HighlightsViewModel.kt
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
app/src/main/java/com/cornellappdev/score/model/HighlightsRepository.kt (1)
26-29:⚠️ Potential issue | 🟡 MinorKDoc still points to the wrong flow.
This comment still references
upcomingGamesFlow, but this repository exposeshighlightsFlow.Proposed fix
/** * Asynchronously fetches the list of highlights from the API. Once finished, will send down - * `upcomingGamesFlow` to be observed. + * `highlightsFlow` to be observed. */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/cornellappdev/score/model/HighlightsRepository.kt` around lines 26 - 29, The KDoc for the async fetch method incorrectly references `upcomingGamesFlow` instead of the repository's `highlightsFlow`; update the comment in HighlightsRepository.kt to mention `highlightsFlow` (and any related symbol such as the fetch method name or property `highlightsFlow`) so the documentation matches the exposed flow and behavior.
🤖 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/score/model/HighlightsRepository.kt`:
- Around line 51-53: In HighlightsRepository where you currently have catch (e:
Exception) that logs and sets _highlightsFlow.value = ApiResponse.Error, change
the error handling so CancellationException (and subclasses) are not swallowed:
explicitly rethrow CancellationException (or check e is CancellationException ->
throw) so external coroutine cancellation propagates, and handle
TimeoutCancellationException separately if you want to convert only timeouts to
ApiResponse.Error; keep logging via Log.e("HighlightsRepository", ...) but only
set _highlightsFlow.value = ApiResponse.Error for non-cancellation failures (or
specifically for TimeoutCancellationException).
In `@app/src/main/java/com/cornellappdev/score/screen/HighlightsScreen.kt`:
- Around line 49-52: The loading UI for HighlightsScreen shows an incorrect
secondary message; in the ApiResponse.Loading branch where LoadingScreen is
called (currently LoadingScreen("Loading Highlights...", "Loading
Schedules...")), update the second argument to the correct highlights copy
(e.g., "Loading Highlights..." or "Loading content...") so both messages reflect
the highlights feature; modify the call inside the is ApiResponse.Loading block
in HighlightsScreen.kt accordingly.
- Around line 84-86: The Column in HighlightsScreen.kt is not scrollable,
causing content overflow; make the container scrollable by replacing or updating
the Column to use a vertical scrollable layout (e.g., add
Modifier.verticalScroll(rememberScrollState()) or switch to a LazyColumn) so
multiple HighlightsCardRow sections ("Today", "Past 3 days") can be reached;
update the modifier on the Column (or replace Column with LazyColumn) to include
the scrolling modifier while preserving existing modifiers and spacing.
---
Duplicate comments:
In `@app/src/main/java/com/cornellappdev/score/model/HighlightsRepository.kt`:
- Around line 26-29: The KDoc for the async fetch method incorrectly references
`upcomingGamesFlow` instead of the repository's `highlightsFlow`; update the
comment in HighlightsRepository.kt to mention `highlightsFlow` (and any related
symbol such as the fetch method name or property `highlightsFlow`) so the
documentation matches the exposed flow and behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4cea61a6-f7a0-4f60-958f-1a3c9c5e8e74
📒 Files selected for processing (4)
app/src/main/java/com/cornellappdev/score/model/HighlightsRepository.ktapp/src/main/java/com/cornellappdev/score/screen/HighlightsScreen.ktapp/src/main/java/com/cornellappdev/score/util/DateUtil.ktapp/src/main/java/com/cornellappdev/score/viewmodel/HighlightsViewModel.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/com/cornellappdev/score/viewmodel/HighlightsViewModel.kt
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
app/src/main/java/com/cornellappdev/score/screen/HighlightsScreen.kt (1)
51-54:⚠️ Potential issue | 🟡 MinorUpdate the secondary loading copy for Highlights.
The loading UI still says
"Loading Schedules..."on the highlights screen.📝 Proposed fix
- LoadingScreen("Loading Highlights...", "Loading Schedules...") + LoadingScreen("Loading Highlights...", "Fetching the latest highlights...")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/cornellappdev/score/screen/HighlightsScreen.kt` around lines 51 - 54, In the ApiResponse.Loading branch of HighlightsScreen (the LoadingScreen call), the secondary loading string currently reads "Loading Schedules..."; update that argument to the correct highlights copy (e.g., "Loading Highlights...") so both primary and secondary messages reflect the highlights screen; locate the LoadingScreen invocation in HighlightsScreen.kt within the ApiResponse.Loading case and replace the second string parameter accordingly.app/src/main/java/com/cornellappdev/score/model/HighlightsRepository.kt (1)
28-31:⚠️ Potential issue | 🟡 MinorFix the stale flow name in this KDoc.
This repository exposes
highlightsFlow, but the comment still saysupcomingGamesFlow, which is misleading for callers.📝 Proposed fix
/** * Asynchronously fetches the list of highlights from the API. Once finished, will send down - * `upcomingGamesFlow` to be observed. + * `highlightsFlow` to be observed. */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/cornellappdev/score/model/HighlightsRepository.kt` around lines 28 - 31, Update the KDoc in HighlightsRepository to reference the correct flow name: replace the stale `upcomingGamesFlow` mention with `highlightsFlow` (the exposed flow from HighlightsRepository, e.g., in the fetchHighlights()/HighlightsRepository class comment) so the documentation correctly describes what callers observe.
🤖 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/score/model/HighlightsRepository.kt`:
- Around line 32-60: fetchHighlights() currently launches multiple appScope jobs
that concurrently write to _highlightsFlow, letting late/stale results overwrite
newer ones; fix by serializing or cancelling prior fetches before starting a new
one. Implement one of: (a) add a private var currentFetchJob: Job? in
HighlightsRepository and before calling appScope.launch cancel and join
currentFetchJob, then assign the newly launched job to currentFetchJob so only
the latest job can update _highlightsFlow; or (b) introduce a requestToken
(atomic Long) that you increment at start of fetchHighlights() and capture
locally, then only set _highlightsFlow if the token still matches (ensuring
stale results are ignored). Reference: fetchHighlights(), _highlightsFlow,
appScope, TIMEOUT_TIME_MILLIS, ApiResponse.
---
Duplicate comments:
In `@app/src/main/java/com/cornellappdev/score/model/HighlightsRepository.kt`:
- Around line 28-31: Update the KDoc in HighlightsRepository to reference the
correct flow name: replace the stale `upcomingGamesFlow` mention with
`highlightsFlow` (the exposed flow from HighlightsRepository, e.g., in the
fetchHighlights()/HighlightsRepository class comment) so the documentation
correctly describes what callers observe.
In `@app/src/main/java/com/cornellappdev/score/screen/HighlightsScreen.kt`:
- Around line 51-54: In the ApiResponse.Loading branch of HighlightsScreen (the
LoadingScreen call), the secondary loading string currently reads "Loading
Schedules..."; update that argument to the correct highlights copy (e.g.,
"Loading Highlights...") so both primary and secondary messages reflect the
highlights screen; locate the LoadingScreen invocation in HighlightsScreen.kt
within the ApiResponse.Loading case and replace the second string parameter
accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2d8adb9f-2eaf-4717-b3a5-08256289a280
📒 Files selected for processing (2)
app/src/main/java/com/cornellappdev/score/model/HighlightsRepository.ktapp/src/main/java/com/cornellappdev/score/screen/HighlightsScreen.kt
Comment on lines
+32
to
+60
| fun fetchHighlights() = appScope.launch { | ||
| _highlightsFlow.value = ApiResponse.Loading | ||
| try { | ||
| val result = | ||
| withTimeout(TIMEOUT_TIME_MILLIS) { | ||
| apolloClient.query(HighlightsQuery()).execute().toResult() | ||
| } | ||
|
|
||
| if (result.isSuccess) { | ||
| val highlights = result.getOrNull() | ||
| val highlightsList = | ||
| highlights?.articles.orEmpty().mapNotNull { it?.toHighlightData() } + | ||
| highlights?.youtubeVideos.orEmpty().mapNotNull { it?.toHighlightData() } | ||
|
|
||
| _highlightsFlow.value = | ||
| ApiResponse.Success(highlightsList) | ||
|
|
||
| } else { | ||
| _highlightsFlow.value = ApiResponse.Error | ||
| } | ||
|
|
||
| } catch (e: TimeoutCancellationException) { | ||
| Log.e("HighlightsRepository", "Highlights request timed out", e) | ||
| _highlightsFlow.value = ApiResponse.Error | ||
| } catch (e: CancellationException) { | ||
| throw e | ||
| } catch (e: Exception) { | ||
| Log.e("HighlightsRepository", "Error fetching highlights", e) | ||
| _highlightsFlow.value = ApiResponse.Error |
There was a problem hiding this comment.
Prevent overlapping refreshes from racing the shared flow.
Each fetchHighlights() call launches a new app-scoped job, and every job writes _highlightsFlow. If an earlier request finishes or times out after a later refresh succeeds, it can overwrite fresh data with stale data or ApiResponse.Error. The injected scope is application-wide in app/src/main/java/com/cornellappdev/score/di/CoroutineScopeModule.kt:1-21, so this is not naturally tied to a screen refresh lifecycle.
🔒 Proposed serialization fix
import kotlinx.coroutines.flow.MutableStateFlow
import kotlinx.coroutines.flow.asStateFlow
import kotlinx.coroutines.launch
+import kotlinx.coroutines.sync.Mutex
+import kotlinx.coroutines.sync.withLock
import kotlinx.coroutines.withTimeout
@@
private val _highlightsFlow =
MutableStateFlow<ApiResponse<List<HighlightData>>>(ApiResponse.Loading)
val highlightsFlow = _highlightsFlow.asStateFlow()
+ private val fetchMutex = Mutex()
@@
- fun fetchHighlights() = appScope.launch {
- _highlightsFlow.value = ApiResponse.Loading
- try {
- val result =
- withTimeout(TIMEOUT_TIME_MILLIS) {
- apolloClient.query(HighlightsQuery()).execute().toResult()
- }
+ fun fetchHighlights() = appScope.launch {
+ fetchMutex.withLock {
+ _highlightsFlow.value = ApiResponse.Loading
+ try {
+ val result =
+ withTimeout(TIMEOUT_TIME_MILLIS) {
+ apolloClient.query(HighlightsQuery()).execute().toResult()
+ }
- if (result.isSuccess) {
- val highlights = result.getOrNull()
- val highlightsList =
- highlights?.articles.orEmpty().mapNotNull { it?.toHighlightData() } +
- highlights?.youtubeVideos.orEmpty().mapNotNull { it?.toHighlightData() }
+ if (result.isSuccess) {
+ val highlights = result.getOrNull()
+ val highlightsList =
+ highlights?.articles.orEmpty().mapNotNull { it?.toHighlightData() } +
+ highlights?.youtubeVideos.orEmpty()
+ .mapNotNull { it?.toHighlightData() }
- _highlightsFlow.value =
- ApiResponse.Success(highlightsList)
+ _highlightsFlow.value =
+ ApiResponse.Success(highlightsList)
- } else {
+ } else {
+ _highlightsFlow.value = ApiResponse.Error
+ }
+
+ } catch (e: TimeoutCancellationException) {
+ Log.e("HighlightsRepository", "Highlights request timed out", e)
_highlightsFlow.value = ApiResponse.Error
+ } catch (e: CancellationException) {
+ throw e
+ } catch (e: Exception) {
+ Log.e("HighlightsRepository", "Error fetching highlights", e)
+ _highlightsFlow.value = ApiResponse.Error
}
-
- } catch (e: TimeoutCancellationException) {
- Log.e("HighlightsRepository", "Highlights request timed out", e)
- _highlightsFlow.value = ApiResponse.Error
- } catch (e: CancellationException) {
- throw e
- } catch (e: Exception) {
- Log.e("HighlightsRepository", "Error fetching highlights", e)
- _highlightsFlow.value = ApiResponse.Error
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/java/com/cornellappdev/score/model/HighlightsRepository.kt`
around lines 32 - 60, fetchHighlights() currently launches multiple appScope
jobs that concurrently write to _highlightsFlow, letting late/stale results
overwrite newer ones; fix by serializing or cancelling prior fetches before
starting a new one. Implement one of: (a) add a private var currentFetchJob:
Job? in HighlightsRepository and before calling appScope.launch cancel and join
currentFetchJob, then assign the newly launched job to currentFetchJob so only
the latest job can update _highlightsFlow; or (b) introduce a requestToken
(atomic Long) that you increment at start of fetchHighlights() and capture
locally, then only set _highlightsFlow if the token still matches (ensuring
stale results are ignored). Reference: fetchHighlights(), _highlightsFlow,
appScope, TIMEOUT_TIME_MILLIS, ApiResponse.
* Set up navigation between highlights screens * Small style/user flow fixes * refactor query and filtering functionality to VM * Recent searches + MVVM refactoring * Add loading screens * fix imports
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/java/com/cornellappdev/score/nav/root/RootNavigation.kt (1)
117-138:⚠️ Potential issue | 🔴 CriticalEnum route parameters require explicit
NavTypemapping in navigation-compose 2.8.2.
HighlightsSubScreenTypecannot be used as a route parameter without a customNavType. Androidx Navigation Compose 2.8.2 does not automatically serialize enums in type-safe@Serializabledestinations. You must register a custom NavType via thetypeMapparameter:composable<ScoreScreens.HighlightsSubScreen>( typeMap = mapOf( typeOf<HighlightsSubScreenType>() to NavType.EnumType(HighlightsSubScreenType::class.java) ) ) { /* ... */ }Apply the same mapping to any other
composable<T>()calls forHighlightsSearchScreenand other routes with enum parameters. Without this, you will encounter a runtimeNavTypeerror orIllegalArgumentExceptionwhen navigating to these screens.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/cornellappdev/score/nav/root/RootNavigation.kt` around lines 117 - 138, Register a custom NavType mapping for the enum HighlightsSubScreenType and any other enum route parameters by supplying a typeMap when declaring the composable destinations; update the composable<ScoreScreens.HighlightsSubScreen> and composable<ScoreScreens.HighlightsSearchScreen> (and any composable<T>() that carries enum params) to include a typeMap entry mapping typeOf<HighlightsSubScreenType>() to NavType.EnumType(HighlightsSubScreenType::class.java) so the navigation system can serialize/deserialize the enum and avoid the runtime NavType/IllegalArgumentException.
🧹 Nitpick comments (9)
app/src/main/java/com/cornellappdev/score/components/highlights/HighlightsLoadingScreen.kt (1)
127-138:modifierparameter is ignored for the root layout.The outer
ColumnhardcodesModifier.fillMaxWidth()while themodifierparameter is only forwarded to the inner contentColumnon line 136. Idiomatic Compose guidance is that the caller-suppliedmodifiershould be applied to the top-level layout of the composable so callers can control sizing/positioning. Consider:♻️ Proposed fix
Column( - modifier = Modifier.fillMaxWidth() + modifier = modifier.fillMaxWidth() ) { HighlightsSubScreenHeader( header = header, navigateBack = {}) Column( - modifier = modifier - .fillMaxWidth() + modifier = Modifier + .fillMaxWidth() .padding(start = 24.dp, end = 24.dp, top = 24.dp),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/cornellappdev/score/components/highlights/HighlightsLoadingScreen.kt` around lines 127 - 138, SubHighlightsLoadingScreen currently ignores the caller-supplied modifier by hardcoding Modifier.fillMaxWidth() on the root Column and applying the parameter only to the inner Column; change the root Column to use the passed-in modifier (e.g., modifier.fillMaxWidth()) so callers control the composable, and switch the inner Column to use a fresh Modifier for fillMaxWidth() and padding; update references to SubHighlightsLoadingScreen, the root Column, and the inner Column modifier usage accordingly.app/src/main/java/com/cornellappdev/score/components/highlights/HighlightsFilter.kt (1)
82-94: Optional: hoist thefilterIsInstancecall.
sportList.filterIsInstance<SportSelection.SportSelect>()runs on each recomposition of the row. For a small, stable list this is negligible, but you can memoize withremember(sportList) { ... }(or filter at the call site) to avoid re-allocating the list and re-keying items unnecessarily.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/cornellappdev/score/components/highlights/HighlightsFilter.kt` around lines 82 - 94, The call sportList.filterIsInstance<SportSelection.SportSelect>() is executed on every recomposition; hoist and memoize it by creating a remembered local (e.g., val filteredSports = remember(sportList) { sportList.filterIsInstance<SportSelection.SportSelect>() }) and use filteredSports in the items(...) call and its key to avoid repeated allocations and re-keying inside HighlightsFilterButton/HighlightsFilter row.app/src/main/java/com/cornellappdev/score/components/highlights/HighlightsCardLazyColumn.kt (1)
80-92: Consider supplying a stablekeytoitems.Without a
key,LazyColumnwill recompose items by index wheneverfilteredResultsis reordered or filtered (e.g., on every keystroke viaonQueryChange). A stable key (for example derived from a content id, ortitle + dateString) will preserve item state and improve list-diff performance during animated transitions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/cornellappdev/score/components/highlights/HighlightsCardLazyColumn.kt` around lines 80 - 92, The LazyColumn's items call over filteredResults lacks a stable key, causing unnecessary recompositions and lost item state when the list is reordered or filtered; update the items(...) invocation inside HighlightsCardLazyColumn to pass a stable key (e.g., item.data.id or a composite like "${item.data.title}-${item.data.dateString}") so that the Compose list diffing can preserve state for entries rendered by VideoHighlightCard and ArticleHighlightCard; ensure the key expression uniquely identifies each HighlightData (both HighlightData.Video and HighlightData.Article) and use that as the items key parameter.app/src/main/java/com/cornellappdev/score/components/highlights/HighlightsCardRow.kt (1)
32-52: Don't derive navigation from the display header string.
toSubScreen(if (rowHeader == "Today") TODAY else PAST3DAYS)silently routes toPAST3DAYSas soon as the header text drifts (localization, copy tweak, or a third category the designers are already considering per the PR description). Pass the targetHighlightsSubScreenTypein explicitly and keeprowHeaderpurely presentational.♻️ Proposed fix
fun HighlightsCardRow( highlightsList: List<HighlightData>, rowHeader: String, - toSubScreen: (HighlightsSubScreenType) -> Unit + subScreenType: HighlightsSubScreenType, + toSubScreen: (HighlightsSubScreenType) -> Unit, ) { ... Row( - modifier = Modifier.clickable { toSubScreen(if (rowHeader == "Today") HighlightsSubScreenType.TODAY else HighlightsSubScreenType.PAST3DAYS) }, + modifier = Modifier.clickable { toSubScreen(subScreenType) }, verticalAlignment = Alignment.CenterVertically ) {Update the call site in
HighlightsScreento passHighlightsSubScreenType.TODAY/PAST3DAYSalongside the header string.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/cornellappdev/score/components/highlights/HighlightsCardRow.kt` around lines 32 - 52, The HighlightsCardRow is deriving navigation from the presentational rowHeader string (using toSubScreen(if (rowHeader == "Today") ...)), which is fragile; change the API so HighlightsCardRow accepts an explicit HighlightsSubScreenType parameter (e.g., add a parameter named targetSubScreen: HighlightsSubScreenType) and use that when invoking toSubScreen instead of comparing rowHeader, then update the call site in HighlightsScreen to pass the correct HighlightsSubScreenType (HighlightsSubScreenType.TODAY or PAST3DAYS) alongside the header string.app/src/main/java/com/cornellappdev/score/nav/root/RootNavigation.kt (1)
128-138: Minor:else -> throwon unknown routes.
toScreen()throwsIllegalArgumentExceptionfor anything not explicitly listed. With the newHighlightsSubScreenbranch added this is fine, but note that thebottomBarinRootNavigationcallsnavBackStackEntry?.toScreen()every recomposition, so any future route you forget to register here will crash the whole scaffold instead of just hiding/showing the nav bar. Consider returningnulland treating unknown as "show bottom bar".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/cornellappdev/score/nav/root/RootNavigation.kt` around lines 128 - 138, The toScreen() helper (NavBackStackEntry.toScreen) currently throws IllegalArgumentException for unknown routes which can crash the scaffold on recomposition; change the else branch to return null instead of throwing so unknown/forgotten routes simply hide the bottom bar. Update NavBackStackEntry.toScreen to return null for the default case (keep existing mapped branches unchanged) and verify callers such as the bottomBar usage in RootNavigation continue to treat a null result as "show/hide bottom bar" rather than expecting a non-null value.app/src/main/java/com/cornellappdev/score/viewmodel/HighlightsViewModel.kt (3)
100-112: Redundant initial sort.
response.data.sortedByDescending { it.date }at line 101 is immediately re-sorted insidebuildDerivedLists(line 53). StoringsortedinloadedStatealso means any lateronQueryChange/onSportSelectedreads already-sorted data and sorts it again. You can drop the outer sort and storeresponse.datainloadedState;buildDerivedListsis the single source of ordering truth.♻️ Proposed fix
is ApiResponse.Success -> { - val sorted = response.data.sortedByDescending { it.date } - val (filtered, today, pastThreeDays) = - buildDerivedLists(sorted, sportSelect, query) + buildDerivedLists(response.data, sportSelect, query) copy( - loadedState = ApiResponse.Success(sorted), + loadedState = ApiResponse.Success(response.data), isRefreshing = false, filteredHighlights = filtered, todayHighlights = today, pastThreeDaysHighlights = pastThreeDays ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/cornellappdev/score/viewmodel/HighlightsViewModel.kt` around lines 100 - 112, The outer sort is redundant because buildDerivedLists does the ordering; change the ApiResponse.Success branch so loadedState stores the raw response.data (not response.data.sortedByDescending) and pass response.data into buildDerivedLists (keep calling buildDerivedLists(sorted?, sportSelect, query) but pass the unsorted list) so sorting only happens inside buildDerivedLists; update references to the local variable `sorted` (in the ApiResponse.Success branch) to use the original `response.data`, ensuring onQueryChange/onSportSelected always derive ordering from buildDerivedLists rather than re-sorting an already-sorted loadedState.
55-64: Dead defensive null check inpastThreeDays.
filteredBySporton line 38 already filters out items whosedateis null, andsortedis derived from that list, sodate.datehere cannot be null. The?.let { ... } ?: falseis unreachable and inconsistent with line 55, which usesit.date == todaydirectly. Simplify for clarity.♻️ Proposed fix
- val pastThreeDays = sorted.filter { date -> - date.date?.let { - it < today && it >= threeDaysAgo - } ?: false - } + val pastThreeDays = sorted.filter { h -> + val d = h.date!! + d < today && d >= threeDaysAgo + }Also consider renaming the lambda parameter away from
datesince the element is aHighlightData, not a date.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/cornellappdev/score/viewmodel/HighlightsViewModel.kt` around lines 55 - 64, The pastThreeDays filter contains an unnecessary null-safe check on date.date even though filteredBySport already removed null dates and sorted is derived from it; update the lambda in pastThreeDays to use a non-null access (e.g., replace the date.date?.let { ... } ?: false pattern with a direct comparison like it.date < today && it.date >= threeDaysAgo), rename the lambda parameter from date to something like highlight or item to reflect that elements are HighlightData, and keep the logic consistent with todayHighlights which uses it.date == today.
95-146: Heads-up:fetchHighlights()fires on every VM init.
HighlightsRepositoryis@SingletonbutHighlightsViewModelis not, so every time the Highlights graph is (re)entered a new ViewModel is created and the repository re-fetches from Apollo. This is probably the intended "refresh on navigate" behavior, but if you want the cachedStateFlowto be authoritative, consider only callingfetchHighlights()whenloadedStateis not alreadySuccess, or gating it on an explicitonRefresh(which already exists).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/cornellappdev/score/viewmodel/HighlightsViewModel.kt` around lines 95 - 146, The init currently always calls highlightsRepository.fetchHighlights(), causing a refetch on every VM creation; change init to only call fetchHighlights() when the current loadedState is not ApiResponse.Success (i.e., if (loadedState !is ApiResponse.Success) fetchHighlights()), or alternatively remove the auto-call and rely on the existing onRefresh() to trigger highlightsRepository.fetchHighlights(); ensure you still collect highlightsRepository.highlightsFlow in init and update state via applyMutation so loadedState, filteredHighlights, todayHighlights, and pastThreeDaysHighlights are set from the flow.app/src/main/java/com/cornellappdev/score/screen/HighlightsScreen.kt (1)
118-121: Avoid deriving navigation from display text.
HighlightsCardRow.ktmapsrowHeader == "Today"toTODAYand everything else toPAST3DAYS. These call sites currently work, but navigation will become fragile if copy changes or gets localized. Prefer passing theHighlightsSubScreenTypeexplicitly intoHighlightsCardRow.♻️ Suggested direction
-HighlightsCardRow(todayHighlightsList, "Today", toSubScreen) +HighlightsCardRow(todayHighlightsList, "Today", HighlightsSubScreenType.TODAY, toSubScreen) -HighlightsCardRow(pastThreeHighlightsList, "Past 3 days", toSubScreen) +HighlightsCardRow(pastThreeHighlightsList, "Past 3 days", HighlightsSubScreenType.PAST3DAYS, toSubScreen)Then update
HighlightsCardRowto calltoSubScreen(subScreenType)instead of inferring fromrowHeader.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/cornellappdev/score/screen/HighlightsScreen.kt` around lines 118 - 121, The current code derives navigation from display text ("Today"/"Past 3 days") which is fragile; change the call sites to pass an explicit HighlightsSubScreenType into HighlightsCardRow (e.g., HighlightsCardRow(todayHighlightsList, "Today", toSubScreen, HighlightsSubScreenType.TODAY) and HighlightsCardRow(pastThreeHighlightsList, "Past 3 days", toSubScreen, HighlightsSubScreenType.PAST3DAYS)), then update HighlightsCardRow to accept a new parameter (subScreenType: HighlightsSubScreenType) and call toSubScreen(subScreenType) instead of inferring from rowHeader/rowHeader == "Today". Ensure function signatures for HighlightsCardRow and any callers are updated accordingly.
🤖 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/score/components/highlights/HighlightsFilter.kt`:
- Around line 100-103: The preview's onFilterSelected lambda currently computes
`{ !isSelected }` but doesn't update state and no longer matches the
`(SportSelection) -> Unit` signature; update the Preview in
HighlightsFilterButtonPreview so the lambda accepts the SportSelection parameter
and toggles the local isSelected state (e.g., `{ _: SportSelection -> isSelected
= !isSelected }`), ensuring the local var isSelected is mutated when
HighlightsFilterButton's onFilterSelected is invoked so clicks in the preview
reflect real behavior.
In
`@app/src/main/java/com/cornellappdev/score/components/highlights/HighlightsLoadingScreen.kt`:
- Line 157: The Spacer in SubHighlightsLoadingScreen is mistakenly using the
function parameter "modifier" instead of the Compose "Modifier", causing
callers' modifiers to be applied to the spacer; update the Spacer call (in
HighlightsLoadingScreen/SubHighlightsLoadingScreen where you see
Spacer(modifier.height(24.dp))) to use Spacer(Modifier.height(24.dp)) so the
spacer only applies its own height and does not inherit the parent/caller
modifier.
- Around line 44-122: The issue is that LoadingStateBox currently forces
.fillMaxWidth() after applying the caller-supplied modifier, which silently
overrides any Modifier.width(...) passed by callers; fix by removing the
unconditional .fillMaxWidth() chaining inside the LoadingStateBox composable
(the trailing .fillMaxWidth() call) or, alternatively, move that .fillMaxWidth()
into the default value for the modifier parameter of LoadingStateBox so callers'
modifiers (e.g., Modifier.width(...)) are respected.
- Around line 174-183: Remove the IDE-inserted fully-qualified prefix by
importing com.cornellappdev.score.components.ScorePreview and replacing usages
of _root_ide_package_.com.cornellappdev.score.components.ScorePreview in the
preview functions; update HighlightsLoadingScreenPreview and
SubHighlightsLoadingScreenPreview to call ScorePreview directly (and add the
import at top) so the previews use the normal ScorePreview reference instead of
the _root_ide_package_ prefix.
In `@app/src/main/java/com/cornellappdev/score/screen/HighlightsSearchScreen.kt`:
- Line 25: The results header in HighlightsSearchScreen is using the
preview/sample list size (highlightsList.size) which can be incorrect in
production; update the header rendering logic to use the actual filtered results
collection (filteredResults.size) instead of highlightsList.size wherever the
header count is shown (e.g., the code that builds the "Results" header in
HighlightsSearchScreen.kt), so the displayed number reflects the current
filteredResults length.
- Around line 39-58: The header is switched by searchScreenType but the
displayed results always use uiState.filteredHighlights; change
HighlightsSearchScreen to pass section-scoped results based on searchScreenType
(e.g., when searchScreenType == TODAY use uiState.filteredHighlightsToday, when
PAST3DAYS use uiState.filteredFilteredPast3Days, else use
uiState.filteredHighlights or ALL), or if those properties don't exist, add a
getter on HighlightsViewModel (e.g., getFilteredHighlightsFor(searchScreenType):
List<Highlight>) that returns the appropriate filtered list and call that here
instead of uiState.filteredHighlights so the UI shows only the section-scoped
results matching the header.
---
Outside diff comments:
In `@app/src/main/java/com/cornellappdev/score/nav/root/RootNavigation.kt`:
- Around line 117-138: Register a custom NavType mapping for the enum
HighlightsSubScreenType and any other enum route parameters by supplying a
typeMap when declaring the composable destinations; update the
composable<ScoreScreens.HighlightsSubScreen> and
composable<ScoreScreens.HighlightsSearchScreen> (and any composable<T>() that
carries enum params) to include a typeMap entry mapping
typeOf<HighlightsSubScreenType>() to
NavType.EnumType(HighlightsSubScreenType::class.java) so the navigation system
can serialize/deserialize the enum and avoid the runtime
NavType/IllegalArgumentException.
---
Nitpick comments:
In
`@app/src/main/java/com/cornellappdev/score/components/highlights/HighlightsCardLazyColumn.kt`:
- Around line 80-92: The LazyColumn's items call over filteredResults lacks a
stable key, causing unnecessary recompositions and lost item state when the list
is reordered or filtered; update the items(...) invocation inside
HighlightsCardLazyColumn to pass a stable key (e.g., item.data.id or a composite
like "${item.data.title}-${item.data.dateString}") so that the Compose list
diffing can preserve state for entries rendered by VideoHighlightCard and
ArticleHighlightCard; ensure the key expression uniquely identifies each
HighlightData (both HighlightData.Video and HighlightData.Article) and use that
as the items key parameter.
In
`@app/src/main/java/com/cornellappdev/score/components/highlights/HighlightsCardRow.kt`:
- Around line 32-52: The HighlightsCardRow is deriving navigation from the
presentational rowHeader string (using toSubScreen(if (rowHeader == "Today")
...)), which is fragile; change the API so HighlightsCardRow accepts an explicit
HighlightsSubScreenType parameter (e.g., add a parameter named targetSubScreen:
HighlightsSubScreenType) and use that when invoking toSubScreen instead of
comparing rowHeader, then update the call site in HighlightsScreen to pass the
correct HighlightsSubScreenType (HighlightsSubScreenType.TODAY or PAST3DAYS)
alongside the header string.
In
`@app/src/main/java/com/cornellappdev/score/components/highlights/HighlightsFilter.kt`:
- Around line 82-94: The call
sportList.filterIsInstance<SportSelection.SportSelect>() is executed on every
recomposition; hoist and memoize it by creating a remembered local (e.g., val
filteredSports = remember(sportList) {
sportList.filterIsInstance<SportSelection.SportSelect>() }) and use
filteredSports in the items(...) call and its key to avoid repeated allocations
and re-keying inside HighlightsFilterButton/HighlightsFilter row.
In
`@app/src/main/java/com/cornellappdev/score/components/highlights/HighlightsLoadingScreen.kt`:
- Around line 127-138: SubHighlightsLoadingScreen currently ignores the
caller-supplied modifier by hardcoding Modifier.fillMaxWidth() on the root
Column and applying the parameter only to the inner Column; change the root
Column to use the passed-in modifier (e.g., modifier.fillMaxWidth()) so callers
control the composable, and switch the inner Column to use a fresh Modifier for
fillMaxWidth() and padding; update references to SubHighlightsLoadingScreen, the
root Column, and the inner Column modifier usage accordingly.
In `@app/src/main/java/com/cornellappdev/score/nav/root/RootNavigation.kt`:
- Around line 128-138: The toScreen() helper (NavBackStackEntry.toScreen)
currently throws IllegalArgumentException for unknown routes which can crash the
scaffold on recomposition; change the else branch to return null instead of
throwing so unknown/forgotten routes simply hide the bottom bar. Update
NavBackStackEntry.toScreen to return null for the default case (keep existing
mapped branches unchanged) and verify callers such as the bottomBar usage in
RootNavigation continue to treat a null result as "show/hide bottom bar" rather
than expecting a non-null value.
In `@app/src/main/java/com/cornellappdev/score/screen/HighlightsScreen.kt`:
- Around line 118-121: The current code derives navigation from display text
("Today"/"Past 3 days") which is fragile; change the call sites to pass an
explicit HighlightsSubScreenType into HighlightsCardRow (e.g.,
HighlightsCardRow(todayHighlightsList, "Today", toSubScreen,
HighlightsSubScreenType.TODAY) and HighlightsCardRow(pastThreeHighlightsList,
"Past 3 days", toSubScreen, HighlightsSubScreenType.PAST3DAYS)), then update
HighlightsCardRow to accept a new parameter (subScreenType:
HighlightsSubScreenType) and call toSubScreen(subScreenType) instead of
inferring from rowHeader/rowHeader == "Today". Ensure function signatures for
HighlightsCardRow and any callers are updated accordingly.
In `@app/src/main/java/com/cornellappdev/score/viewmodel/HighlightsViewModel.kt`:
- Around line 100-112: The outer sort is redundant because buildDerivedLists
does the ordering; change the ApiResponse.Success branch so loadedState stores
the raw response.data (not response.data.sortedByDescending) and pass
response.data into buildDerivedLists (keep calling buildDerivedLists(sorted?,
sportSelect, query) but pass the unsorted list) so sorting only happens inside
buildDerivedLists; update references to the local variable `sorted` (in the
ApiResponse.Success branch) to use the original `response.data`, ensuring
onQueryChange/onSportSelected always derive ordering from buildDerivedLists
rather than re-sorting an already-sorted loadedState.
- Around line 55-64: The pastThreeDays filter contains an unnecessary null-safe
check on date.date even though filteredBySport already removed null dates and
sorted is derived from it; update the lambda in pastThreeDays to use a non-null
access (e.g., replace the date.date?.let { ... } ?: false pattern with a direct
comparison like it.date < today && it.date >= threeDaysAgo), rename the lambda
parameter from date to something like highlight or item to reflect that elements
are HighlightData, and keep the logic consistent with todayHighlights which uses
it.date == today.
- Around line 95-146: The init currently always calls
highlightsRepository.fetchHighlights(), causing a refetch on every VM creation;
change init to only call fetchHighlights() when the current loadedState is not
ApiResponse.Success (i.e., if (loadedState !is ApiResponse.Success)
fetchHighlights()), or alternatively remove the auto-call and rely on the
existing onRefresh() to trigger highlightsRepository.fetchHighlights(); ensure
you still collect highlightsRepository.highlightsFlow in init and update state
via applyMutation so loadedState, filteredHighlights, todayHighlights, and
pastThreeDaysHighlights are set from the flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 736eaa8c-2b7c-44f6-89ec-04ea57ecb147
📒 Files selected for processing (13)
app/src/main/java/com/cornellappdev/score/components/highlights/HighlightsCardLazyColumn.ktapp/src/main/java/com/cornellappdev/score/components/highlights/HighlightsCardRow.ktapp/src/main/java/com/cornellappdev/score/components/highlights/HighlightsFilter.ktapp/src/main/java/com/cornellappdev/score/components/highlights/HighlightsLoadingScreen.ktapp/src/main/java/com/cornellappdev/score/components/highlights/HighlightsScreenSearchFilterBar.ktapp/src/main/java/com/cornellappdev/score/components/highlights/HighlightsSearchBar.ktapp/src/main/java/com/cornellappdev/score/components/highlights/RecentSearches.ktapp/src/main/java/com/cornellappdev/score/nav/ScoreNavHost.ktapp/src/main/java/com/cornellappdev/score/nav/root/RootNavigation.ktapp/src/main/java/com/cornellappdev/score/screen/HighlightsScreen.ktapp/src/main/java/com/cornellappdev/score/screen/HighlightsSearchScreen.ktapp/src/main/java/com/cornellappdev/score/screen/HighlightsSubScreen.ktapp/src/main/java/com/cornellappdev/score/viewmodel/HighlightsViewModel.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- app/src/main/java/com/cornellappdev/score/components/highlights/HighlightsScreenSearchFilterBar.kt
- app/src/main/java/com/cornellappdev/score/screen/HighlightsSubScreen.kt
Comment on lines
100
to
103
| private fun HighlightsFilterButtonPreview() { | ||
| var isSelected by remember { mutableStateOf(false) } | ||
| HighlightsFilterButton(Sport.BASEBALL, { isSelected = !isSelected }, isSelected = isSelected) | ||
| HighlightsFilterButton(Sport.BASEBALL, { !isSelected }, isSelected = isSelected) | ||
| } |
There was a problem hiding this comment.
Preview click no longer toggles isSelected.
The lambda passed to onFilterSelected now has signature (SportSelection) -> Unit. { !isSelected } just computes and discards !isSelected; the preview button won't actually toggle its selected state on click. Wire the state update through the callback so the preview reflects real behavior.
🛠️ Proposed fix
- var isSelected by remember { mutableStateOf(false) }
- HighlightsFilterButton(Sport.BASEBALL, { !isSelected }, isSelected = isSelected)
+ var isSelected by remember { mutableStateOf(false) }
+ HighlightsFilterButton(
+ sport = Sport.BASEBALL,
+ onFilterSelected = { isSelected = !isSelected },
+ isSelected = isSelected
+ )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/src/main/java/com/cornellappdev/score/components/highlights/HighlightsFilter.kt`
around lines 100 - 103, The preview's onFilterSelected lambda currently computes
`{ !isSelected }` but doesn't update state and no longer matches the
`(SportSelection) -> Unit` signature; update the Preview in
HighlightsFilterButtonPreview so the lambda accepts the SportSelection parameter
and toggles the local isSelected state (e.g., `{ _: SportSelection -> isSelected
= !isSelected }`), ensuring the local var isSelected is mutated when
HighlightsFilterButton's onFilterSelected is invoked so clicks in the preview
reflect real behavior.
Comment on lines
+44
to
+122
| LoadingStateBox(100, 40.dp) | ||
| } | ||
| Spacer(modifier = Modifier.height(16.dp)) | ||
| Row( | ||
| modifier = Modifier.padding(start = 24.dp), | ||
| horizontalArrangement = Arrangement.spacedBy(12.dp), | ||
| ) { | ||
| repeat(4) { | ||
| LoadingStateBox( | ||
| 100, 30.dp, modifier = Modifier.width(85.dp) | ||
| ) | ||
| } | ||
| } | ||
| Spacer(modifier = Modifier.height(24.dp)) | ||
| Row( | ||
| modifier = Modifier | ||
| .padding(horizontal = 24.dp) | ||
| .fillMaxWidth(), | ||
| horizontalArrangement = Arrangement.SpaceBetween, | ||
| verticalAlignment = Alignment.CenterVertically | ||
| ) { | ||
| Text( | ||
| text = "Loading Today...", style = heading2, color = GrayStroke | ||
| ) | ||
| LoadingStateBox( | ||
| 100, 15.dp, modifier = Modifier.width( | ||
| 50.dp | ||
| ) | ||
| ) | ||
| } | ||
| Spacer(modifier = Modifier.height(16.dp)) | ||
| Row( | ||
| modifier = Modifier.padding(start = 24.dp), | ||
| horizontalArrangement = Arrangement.spacedBy(16.dp) | ||
| ) { | ||
| LoadingStateBox( | ||
| 12, 192.dp, modifier = Modifier.width( | ||
| 241.dp | ||
| ) | ||
| ) | ||
| LoadingStateBox( | ||
| 12, 192.dp, modifier = Modifier.width( | ||
| 241.dp | ||
| ) | ||
| ) | ||
| } | ||
| Spacer(modifier = Modifier.height(24.dp)) | ||
| Row( | ||
| modifier = Modifier | ||
| .padding(horizontal = 24.dp) | ||
| .fillMaxWidth(), | ||
| horizontalArrangement = Arrangement.SpaceBetween, | ||
| verticalAlignment = Alignment.CenterVertically | ||
| ) { | ||
| Text( | ||
| text = "Loading Past 3 Days...", style = heading2, color = GrayStroke | ||
| ) | ||
| LoadingStateBox( | ||
| 100, 15.dp, modifier = Modifier.width( | ||
| 50.dp | ||
| ) | ||
| ) | ||
| } | ||
| Spacer(modifier = Modifier.height(16.dp)) | ||
| Row( | ||
| modifier = Modifier.padding(start = 24.dp), | ||
| horizontalArrangement = Arrangement.spacedBy(16.dp) | ||
| ) { | ||
| LoadingStateBox( | ||
| 12, 192.dp, modifier = Modifier.width( | ||
| 241.dp | ||
| ) | ||
| ) | ||
| LoadingStateBox( | ||
| 12, 192.dp, modifier = Modifier.width( | ||
| 241.dp | ||
| ) | ||
| ) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -a 'LoadingStateBox.kt' | xargs -I{} sed -n '1,60p' {}Repository: cuappdev/score-android
Length of output: 1371
Modifier.width(...) passed to LoadingStateBox is silently overridden.
LoadingStateBox applies your modifier, then unconditionally chains .fillMaxWidth() at the end (line 38 in LoadingStateBox.kt). This overrides the Modifier.width() values you pass here—every box will fill the parent width regardless.
Remove the trailing .fillMaxWidth() from LoadingStateBox.kt (line 38), or move it to the default modifier parameter only (line 23), so that caller-supplied widths are respected.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/src/main/java/com/cornellappdev/score/components/highlights/HighlightsLoadingScreen.kt`
around lines 44 - 122, The issue is that LoadingStateBox currently forces
.fillMaxWidth() after applying the caller-supplied modifier, which silently
overrides any Modifier.width(...) passed by callers; fix by removing the
unconditional .fillMaxWidth() chaining inside the LoadingStateBox composable
(the trailing .fillMaxWidth() call) or, alternatively, move that .fillMaxWidth()
into the default value for the modifier parameter of LoadingStateBox so callers'
modifiers (e.g., Modifier.width(...)) are respected.
| ) | ||
| } | ||
| } | ||
| Spacer(modifier.height(24.dp)) |
There was a problem hiding this comment.
Bug: Spacer uses caller's modifier instead of Modifier.
Spacer(modifier.height(24.dp)) applies the outer modifier parameter (which the caller may have populated with size/layout constraints such as fillMaxSize, weights, backgrounds, etc.) to a spacer. This is almost certainly a typo — every other spacer in this file uses Modifier.height(...). As written, passing any non-trivial modifier to SubHighlightsLoadingScreen can cause the spacer to consume unexpected space or inherit unwanted styling.
🐛 Proposed fix
- Spacer(modifier.height(24.dp))
+ Spacer(Modifier.height(24.dp))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Spacer(modifier.height(24.dp)) | |
| Spacer(Modifier.height(24.dp)) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/src/main/java/com/cornellappdev/score/components/highlights/HighlightsLoadingScreen.kt`
at line 157, The Spacer in SubHighlightsLoadingScreen is mistakenly using the
function parameter "modifier" instead of the Compose "Modifier", causing
callers' modifiers to be applied to the spacer; update the Spacer call (in
HighlightsLoadingScreen/SubHighlightsLoadingScreen where you see
Spacer(modifier.height(24.dp))) to use Spacer(Modifier.height(24.dp)) so the
spacer only applies its own height and does not inherit the parent/caller
modifier.
Comment on lines
+174
to
+183
| private fun HighlightsLoadingScreenPreview() = | ||
| _root_ide_package_.com.cornellappdev.score.components.ScorePreview { | ||
| HighlightsLoadingScreen("Loading Highlights...") | ||
| } | ||
|
|
||
| @Preview | ||
| @Composable | ||
| private fun SubHighlightsLoadingScreenPreview() = | ||
| _root_ide_package_.com.cornellappdev.score.components.ScorePreview { | ||
| SubHighlightsLoadingScreen("Past 3 Days") |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove the _root_ide_package_. IDE-generated prefix.
_root_ide_package_ is an artifact that IntelliJ/Android Studio inserts when auto-completing a fully-qualified name in a file that has an import conflict; it's not valid-looking Kotlin for production source and should be replaced with a normal import of com.cornellappdev.score.components.ScorePreview.
♻️ Proposed fix
import com.cornellappdev.score.components.LoadingStateBox
+import com.cornellappdev.score.components.ScorePreview
...
private fun HighlightsLoadingScreenPreview() =
- _root_ide_package_.com.cornellappdev.score.components.ScorePreview {
+ ScorePreview {
HighlightsLoadingScreen("Loading Highlights...")
}
...
private fun SubHighlightsLoadingScreenPreview() =
- _root_ide_package_.com.cornellappdev.score.components.ScorePreview {
+ ScorePreview {
SubHighlightsLoadingScreen("Past 3 Days")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private fun HighlightsLoadingScreenPreview() = | |
| _root_ide_package_.com.cornellappdev.score.components.ScorePreview { | |
| HighlightsLoadingScreen("Loading Highlights...") | |
| } | |
| @Preview | |
| @Composable | |
| private fun SubHighlightsLoadingScreenPreview() = | |
| _root_ide_package_.com.cornellappdev.score.components.ScorePreview { | |
| SubHighlightsLoadingScreen("Past 3 Days") | |
| private fun HighlightsLoadingScreenPreview() = | |
| ScorePreview { | |
| HighlightsLoadingScreen("Loading Highlights...") | |
| } | |
| `@Preview` | |
| `@Composable` | |
| private fun SubHighlightsLoadingScreenPreview() = | |
| ScorePreview { | |
| SubHighlightsLoadingScreen("Past 3 Days") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/src/main/java/com/cornellappdev/score/components/highlights/HighlightsLoadingScreen.kt`
around lines 174 - 183, Remove the IDE-inserted fully-qualified prefix by
importing com.cornellappdev.score.components.ScorePreview and replacing usages
of _root_ide_package_.com.cornellappdev.score.components.ScorePreview in the
preview functions; update HighlightsLoadingScreenPreview and
SubHighlightsLoadingScreenPreview to call ScorePreview directly (and add the
import at top) so the previews use the normal ScorePreview reference instead of
the _root_ide_package_ prefix.
| import com.cornellappdev.score.model.Sport | ||
| import com.cornellappdev.score.model.SportSelection | ||
| import com.cornellappdev.score.theme.Style.heading2 | ||
| import com.cornellappdev.score.util.highlightsList |
There was a problem hiding this comment.
Use the actual filtered result count in the results header.
Line 101 uses highlightsList.size, which comes from preview/sample data and can show the wrong count in production. Use filteredResults.size instead.
🐛 Proposed fix
-import com.cornellappdev.score.util.highlightsList
import com.cornellappdev.score.util.recentSearchList
import com.cornellappdev.score.util.sportSelectionList
import com.cornellappdev.score.viewmodel.HighlightsViewModel
@@
HighlightsCardLazyColumn(
recentSearchList,
query,
filteredResults,
onItemClick,
onCloseClick,
- { HighlightsCardLazyColumnResultsHeader(highlightsList.size) })
+ { HighlightsCardLazyColumnResultsHeader(filteredResults.size) })Also applies to: 95-101
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/java/com/cornellappdev/score/screen/HighlightsSearchScreen.kt`
at line 25, The results header in HighlightsSearchScreen is using the
preview/sample list size (highlightsList.size) which can be incorrect in
production; update the header rendering logic to use the actual filtered results
collection (filteredResults.size) instead of highlightsList.size wherever the
header count is shown (e.g., the code that builds the "Results" header in
HighlightsSearchScreen.kt), so the displayed number reflects the current
filteredResults length.
Comment on lines
+39
to
+58
| val header = when (searchScreenType) { | ||
| HighlightsSubScreenType.TODAY -> "Search today" | ||
| HighlightsSubScreenType.PAST3DAYS -> "Search past 3 days" | ||
| HighlightsSubScreenType.ALL -> "Search all highlights" | ||
| } | ||
|
|
||
| HighlightsSearchScreenContent( | ||
| sportList = uiState.sportSelectionList, | ||
| onFilterSelected = { highlightsViewModel.onSportSelected(it) }, | ||
| recentSearchList = uiState.recentSearches, | ||
| filteredResults = uiState.filteredHighlights, | ||
| query = uiState.query, | ||
| selectedFilter = uiState.sportSelect, | ||
| onQueryChange = { highlightsViewModel.onQueryChange(it) }, | ||
| onSearch = { highlightsViewModel.onSearch(it) }, | ||
| header = header, | ||
| onItemClick = { highlightsViewModel.onSearchRecent(it) }, | ||
| onCloseClick = { highlightsViewModel.onRemoveRecent(it) }, | ||
| navigateBack = navigateBack | ||
| ) |
There was a problem hiding this comment.
Scope search results to searchScreenType.
searchScreenType currently changes only the header. For TODAY and PAST3DAYS, Line 49 still sends uiState.filteredHighlights, so a screen titled “Search today” can show results outside today’s section. Use section-scoped filtered results here, or expose them from HighlightsViewModel.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/java/com/cornellappdev/score/screen/HighlightsSearchScreen.kt`
around lines 39 - 58, The header is switched by searchScreenType but the
displayed results always use uiState.filteredHighlights; change
HighlightsSearchScreen to pass section-scoped results based on searchScreenType
(e.g., when searchScreenType == TODAY use uiState.filteredHighlightsToday, when
PAST3DAYS use uiState.filteredFilteredPast3Days, else use
uiState.filteredHighlights or ALL), or if those properties don't exist, add a
getter on HighlightsViewModel (e.g., getFilteredHighlightsFor(searchScreenType):
List<Highlight>) that returns the appropriate filtered list and call that here
instead of uiState.filteredHighlights so the UI shows only the section-scoped
results matching the header.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
highlights UI bug fixes/improvements, initial highlights networking
Changes Made
Test Coverage
visually matches figma, double checked that all highlights in specified date range are shown
Next Steps
Screenshots (delete if not applicable)
(note, we realized showing only "today" and "past 3 day" categories could be a flaw in cases like this where there's very few recent highlights. our designer is working on improving this!)
Summary by CodeRabbit
New Features
Improvements