Conversation
…s screens. update contributor information
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 50 minutes and 58 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis pull request introduces route display filtering and sorting logic via a new Changes
Sequence DiagramsequenceDiagram
participant RouteVM as RouteViewModel
participant Processor as RouteOptionsDisplayProcessor
participant StateFlow as displayedRouteFlow
participant RouteScreen
RouteVM->>RouteVM: lastRouteFlow emits ApiResponse<RouteOptions>
alt Response is Success
RouteVM->>RouteVM: Check arriveByFlow value
alt Arrive By Mode
RouteVM->>Processor: filterAndSortForArriveBy(cutoff)
Processor-->>RouteVM: Filtered RouteOptions (by arrival cutoff)
else Leave At/Leave Now Mode
RouteVM->>Processor: filterAndSortForLeaveCutoff(cutoff, maxRoutes)
Processor-->>RouteVM: Filtered RouteOptions (by departure cutoff & tie logic)
end
RouteVM->>StateFlow: Emit transformed ApiResponse.Success
else Response is not Success
RouteVM->>StateFlow: Pass through unchanged
end
StateFlow->>RouteScreen: displayedRouteFlow state updates
RouteScreen->>RouteScreen: Re-render route list with filtered/sorted routes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
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/transit/ui/screens/settings/AboutScreen.kt (1)
129-229:⚠️ Potential issue | 🟡 MinorAdd vertical scrolling to the Column — it currently lacks a
verticalScrollmodifier but contains substantial content: back button, title, logo, Pod Leads (9 names), five team rows (iOS: 17, Android: 17, Design: 10, Marketing: 11, Backend: 12 members), and a website button. On smaller devices or with larger font scales, lower content will be clipped. Add.verticalScroll(rememberScrollState())to theColumnmodifier (requires importingverticalScrollandrememberScrollStatefromandroidx.compose.foundation), or consider switching to aLazyColumn.🤖 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/screens/settings/AboutScreen.kt` around lines 129 - 229, The Column used in AboutScreen (the Column block that wraps IconButton, Texts, Image, MemberList, the for loop over names, etc.) is not scrollable and can clip content; update its modifier chain to include .verticalScroll(rememberScrollState()) (or replace the Column with a LazyColumn) so the entire content can scroll on small screens/large font sizes, and add the imports androidx.compose.foundation.verticalScroll and androidx.compose.foundation.rememberScrollState; ensure you modify the same Column that contains IconButton/onBackClick and the for ((team, members) in names) loop.
🧹 Nitpick comments (2)
app/src/main/java/com/cornellappdev/transit/ui/screens/SettingsScreen.kt (1)
38-43: Consider orderingonBackClickfirst and giving callbacks default values.Putting the navigation/back callback first is a common convention and matches the visual order (back button is at the top). Additionally, defaulting the lambdas to
{}would make previews and future call sites less brittle (the preview at Line 114 currently relies on positional ordering of four identical() -> Unitparameters, which is easy to misalign silently).-fun SettingsScreen( - onSupportClick: () -> Unit, - onAboutClick: () -> Unit, - onNotificationsAndPrivacyClick: () -> Unit, - onBackClick: () -> Unit -) { +fun SettingsScreen( + onBackClick: () -> Unit = {}, + onAboutClick: () -> Unit = {}, + onNotificationsAndPrivacyClick: () -> Unit = {}, + onSupportClick: () -> Unit = {}, +) {🤖 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/screens/SettingsScreen.kt` around lines 38 - 43, Reorder the SettingsScreen parameters so onBackClick appears first and give all callback parameters default empty lambdas ({}), e.g. change the function signature of SettingsScreen to accept onBackClick: () -> Unit as the first parameter and make onSupportClick, onAboutClick, onNotificationsAndPrivacyClick default to {}; update any callers and the Preview that currently rely on positional arguments to either use named arguments or rely on the new defaults to avoid misalignment. Ensure references to SettingsScreen in the file are adjusted to the new parameter order or use named parameters.app/src/main/java/com/cornellappdev/transit/ui/screens/settings/AboutScreen.kt (1)
136-143: Duplicated back-button composable across three screens.The same
IconButton+KeyboardArrowLeft+IconGraytint block is repeated verbatim inSettingsScreen.kt,AboutScreen.kt, andSupportScreen.kt. Consider extracting a smallBackButton(onClick: () -> Unit)composable (e.g., inui/components/) to keep styling consistent and reduce duplication.♻️ Example extraction
`@Composable` fun BackButton(onBackClick: () -> Unit, modifier: Modifier = Modifier) { IconButton(onClick = onBackClick, modifier = modifier) { Icon( imageVector = Icons.AutoMirrored.Outlined.KeyboardArrowLeft, contentDescription = "Go back", tint = IconGray, modifier = Modifier.size(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/transit/ui/screens/settings/AboutScreen.kt` around lines 136 - 143, Duplicate back-button UI (IconButton + Icons.AutoMirrored.Outlined.KeyboardArrowLeft + IconGray tint) appears in AboutScreen, SettingsScreen, and SupportScreen; extract a reusable `@Composable` BackButton(onBackClick: () -> Unit, modifier: Modifier = Modifier) into your ui/components package and replace the repeated blocks with a call to BackButton(onBackClick = ...) in AboutScreen.kt, SettingsScreen.kt, and SupportScreen.kt so styling and behavior are centralized while preserving the IconButton, KeyboardArrowLeft imageVector, IconGray tint, and Modifier.size(24.dp).
🤖 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/util/RouteOptionsDisplayProcessor.kt`:
- Around line 284-296: The current logic computes a fallbackTransit and then
does ranked = (preferred + fallbackTransit?) .sortedWith { ... } and finally
takes ranked.take(maxRoutes), which can drop the fallbackTransit; modify the
post-sort trimming to ensure fallbackTransit (the transit option found in the
fallback computation) is always included when non-null: after sorting (in the
variable ranked) if maxRoutes != null then take the first maxRoutes but if
fallbackTransit != null and fallbackTransit is not in that slice, replace the
last element of the slice with fallbackTransit (or otherwise insert
fallbackTransit and trim to maxRoutes) before calling toRouteOptions; keep using
compareByEffectiveLeaveTime for sorting and preserve preferred/fallbackTransit
variables and the final toRouteOptions call.
- Around line 80-92: The function Route.isLegalForLeaveCutoff currently treats
null from firstBoardingDepartureInstantOrNull() as “walking-only”; instead, if
firstBoardingDepartureInstantOrNull() returns null and the route is NOT a
walking-only route, return false (fail closed). Update isLegalForLeaveCutoff so
that immediately after calling firstBoardingDepartureInstantOrNull() you check
the route type (use the existing walking-only predicate on Route, e.g.,
isWalkingOnly or equivalent) and only treat null as legal when the route is
actually walking-only; otherwise return false, then proceed to use
walkingDurationBeforeFirstBoardingOrNull() and the isBefore check as before.
---
Outside diff comments:
In
`@app/src/main/java/com/cornellappdev/transit/ui/screens/settings/AboutScreen.kt`:
- Around line 129-229: The Column used in AboutScreen (the Column block that
wraps IconButton, Texts, Image, MemberList, the for loop over names, etc.) is
not scrollable and can clip content; update its modifier chain to include
.verticalScroll(rememberScrollState()) (or replace the Column with a LazyColumn)
so the entire content can scroll on small screens/large font sizes, and add the
imports androidx.compose.foundation.verticalScroll and
androidx.compose.foundation.rememberScrollState; ensure you modify the same
Column that contains IconButton/onBackClick and the for ((team, members) in
names) loop.
---
Nitpick comments:
In
`@app/src/main/java/com/cornellappdev/transit/ui/screens/settings/AboutScreen.kt`:
- Around line 136-143: Duplicate back-button UI (IconButton +
Icons.AutoMirrored.Outlined.KeyboardArrowLeft + IconGray tint) appears in
AboutScreen, SettingsScreen, and SupportScreen; extract a reusable `@Composable`
BackButton(onBackClick: () -> Unit, modifier: Modifier = Modifier) into your
ui/components package and replace the repeated blocks with a call to
BackButton(onBackClick = ...) in AboutScreen.kt, SettingsScreen.kt, and
SupportScreen.kt so styling and behavior are centralized while preserving the
IconButton, KeyboardArrowLeft imageVector, IconGray tint, and
Modifier.size(24.dp).
In `@app/src/main/java/com/cornellappdev/transit/ui/screens/SettingsScreen.kt`:
- Around line 38-43: Reorder the SettingsScreen parameters so onBackClick
appears first and give all callback parameters default empty lambdas ({}), e.g.
change the function signature of SettingsScreen to accept onBackClick: () ->
Unit as the first parameter and make onSupportClick, onAboutClick,
onNotificationsAndPrivacyClick default to {}; update any callers and the Preview
that currently rely on positional arguments to either use named arguments or
rely on the new defaults to avoid misalignment. Ensure references to
SettingsScreen in the file are adjusted to the new parameter order or use named
parameters.
🪄 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: 8cfd9dba-fbe7-46a2-bf4b-f75a12a2a0ae
📒 Files selected for processing (9)
app/src/main/java/com/cornellappdev/transit/ui/NavigationController.ktapp/src/main/java/com/cornellappdev/transit/ui/screens/HomeScreen.ktapp/src/main/java/com/cornellappdev/transit/ui/screens/RouteScreen.ktapp/src/main/java/com/cornellappdev/transit/ui/screens/SettingsScreen.ktapp/src/main/java/com/cornellappdev/transit/ui/screens/settings/AboutScreen.ktapp/src/main/java/com/cornellappdev/transit/ui/screens/settings/SupportScreen.ktapp/src/main/java/com/cornellappdev/transit/ui/viewmodels/RouteViewModel.ktapp/src/main/java/com/cornellappdev/transit/util/RouteOptionsDisplayProcessor.ktapp/src/main/java/com/cornellappdev/transit/util/TransitConstants.kt
| import com.cornellappdev.transit.ui.theme.TransitBlue | ||
| import com.cornellappdev.transit.ui.theme.robotoFamily | ||
|
|
||
| private val names = mapOf( |
There was a problem hiding this comment.
Nit: can probably be made into another issue but hardcoded strings like these should ideally be extracted into some sort of strings.xml or constants file instead of living in the screen
| @@ -233,5 +256,5 @@ fun AboutScreen() { | |||
| @Preview(showBackground = true) | |||
| @Composable | |||
| private fun PreviewAboutScreen() { | |||
There was a problem hiding this comment.
Nit: for consistency, should stick with Preview after AboutScreen
| @@ -233,5 +256,5 @@ fun AboutScreen() { | |||
| @Preview(showBackground = true) | |||
| @Composable | |||
| private fun PreviewAboutScreen() { | |||
There was a problem hiding this comment.
Also, there's a preview rendering issue here as well.
| import java.time.Instant | ||
|
|
||
| /** Parse an ISO-8601 instant string and return null when parsing fails. */ | ||
| private fun String.toInstantOrNull(): Instant? = runCatching { Instant.parse(this) }.getOrNull() |
There was a problem hiding this comment.
Nit: lots of OrNull throughout this file with null used to represent failures, but there is no other indication of failures or errors which can make it harder to debug later on. Maybe some sealed class might help but not the most important for now.
|
|
||
| val bestWalkingEffort = eligibleRoutes | ||
| .map { it.route } | ||
| .filter { !it.isTransitRoute() } |
There was a problem hiding this comment.
isTransitRoute is used in these higher order functions a couple of times, so it might make sense to store/precompute this somewhere so it's not repeated
2 participants
Overview
Changes Made
Test Coverage
Next Steps (delete if not applicable)
Summary by CodeRabbit
Release Notes
New Features
Updates
Improvements