Progress Rail: redesigned in-trip panel - #153
Merged
Merged
Conversation
Implements the In-Trip Panel design (Claude Design 7bb92a3b): the horizontal pager becomes one vertical timeline built from a repeated three-column RailRowView (time gutter / rail marks / content), driven by a pure, clock-based TripProgress engine with the two-cursor model: currentLeg advances only with the clock, focusedLeg follows taps and the tip-detent stepper, and a Back-to-now pill re-tethers. - Destination header pinned at every detent; violet late-arrival treatment - Now-card for the current step: boarding countdown, stop code reference, riding stop ladder; done rows collapse to one line at 50% - 160pt tip detent with stepper, segmented progress bar, and status footer - Leg now decodes departureDelay/arrivalDelay (REST + GraphQL) feeding the fixed status vocabulary: On time / n min late / n min early / Scheduled - Rows are combined accessibility elements; the time gutter moves inline at accessibility type sizes; auto-advance posts a haptic + VO announcement - 47 new strings localized across all 13 locales - TripProgress unit tests
Before the first leg's start time the clock-derived phase alone kept the footer stuck on Start Trip: tapping it only collapsed the sheet, and the Hide / End Trip footer could never appear, leaving no way to end a trip. Start Trip now flips a tripStarted flag (with a haptic, collapse to tip, and map refocus on the first step), the in-trip footer keys off that flag or the clock — whichever says the trip is underway — and the pre-trip footer gains an End Trip button so a planned trip can be abandoned without starting it.
Simplify: one TripProgress per tick passed down instead of rebuilt per child; shared copy helpers (localizedActivityName, leaveInstruction, boardingCountdown, Step.localizedInstruction reusing the existing leg.step_turn/continue keys, Leg.riderFacingRouteName); focus flows through the binding alone with the map driven from one onChange; dead abstractions removed (showsTime, isBoard, phase early-return, unused params); unused keys dropped from all locales. Review fixes: route-name fallback goes through the localized modeDisplayName instead of mode.capitalized (d012225 regression); the arrive footer formats its key with the real argument so placeholder-first locales keep word order; arrival no longer pans the map back to the trip origin; tapping the current row re-tethers instead of focusing; the pre-trip footer counts down instead of claiming "Walking"; the sheet opens at medium pre-trip so Start Trip is visible, and Hide/End Trip appear at medium as well as large; Back-to-now pill shows for any focused row; orphaned directions.swipe_hint removed from all 13 locales.
|
Warning Review limit reached
Next review available in: 10 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughAdds real-time delay modeling, clock-driven trip progress calculations, and a rail-based directions sheet with localized status, walking, transit, transfer, and arrival content. ChangesTrip guidance experience
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant TimelineView
participant DirectionsSheetView
participant TripProgress
participant InTripRailView
TimelineView->>DirectionsSheetView: provide current time
DirectionsSheetView->>TripProgress: calculate phase and rows
DirectionsSheetView->>InTripRailView: render current progress
InTripRailView-->>DirectionsSheetView: return focus or back-to-now action
DirectionsSheetView->>DirectionsSheetView: update map and accessibility state
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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: 6
🧹 Nitpick comments (2)
OTPKit/Sources/OTPKit/Network/GraphQLAPIService.swift (1)
187-189: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAvoid relying on OTP GraphQL deprecated delay fields.
deparutureDelay/arrivalDelayonLegare deprecated in current OTP GraphQL GTFS versions in favor ofend.estimated.delay/start.estimated.delay. This works for the current server target, but future OTP upgrades can drop deprecated fields, which would break parsing here since those values are also mapped throughGraphQLLeg→Leg. Confirm the supported OTP server versions; if these are unsupported, migrate the query and response mapping together.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@OTPKit/Sources/OTPKit/Network/GraphQLAPIService.swift` around lines 187 - 189, Update the GraphQL query around realTime, departureDelay, and arrivalDelay to use the supported end.estimated.delay and start.estimated.delay fields instead of deprecated Leg delay fields. Update GraphQLLeg-to-Leg response mapping consistently so delay values retain their current semantics, and verify the fields against the supported OTP server versions.OTPKit/Sources/OTPKit/Presentation/Sheets/Directions/Rail/TipContentView.swift (1)
249-283: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider moving the "next boarding" lookup rule into
TripProgress.
footerLeadingTextcomputes which leg counts as the "next boarding" (with the riding/waiting exclusion rule) directly in the view. SinceTripProgressalready exposes similar rider-facing queries (waitAfterLeg,isSameStopTransfer,stopsRemaining), moving this lookup there would keep the business rule covered byTripProgressTestsand keep this view limited to formatting, per the OTPKit convention of moving complex logic into the model layer.As per coding guidelines, "Keep SwiftUI views small and focused ... and move complex logic into view models."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@OTPKit/Sources/OTPKit/Presentation/Sheets/Directions/Rail/TipContentView.swift` around lines 249 - 283, Move the next-boarding selection rule from TipContentView.footerLeadingText into TripProgress as a rider-facing query, preserving the currentLegIndex and riding/waiting exclusion behavior. Update footerLeadingText to call that TripProgress API and retain only localization and formatting, adding or updating TripProgressTests to cover the lookup rule.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@OTPKit/Sources/OTPKit/Presentation/Sheets/Directions/DirectionsSheetView.swift`:
- Around line 38-41: Update DirectionsSheetView.tipDetent and the associated
TipContentView layout so the compact guidance sheet accommodates Dynamic Type
accessibility text without clipping. Replace the fixed-height-only behavior with
content-aware sizing or an overflow/scroll fallback, while preserving the
compact detent for text that fits.
In
`@OTPKit/Sources/OTPKit/Presentation/Sheets/Directions/Rail/InTripRailView.swift`:
- Around line 143-147: The completed-row styling in the .board/.ride case only
treats .board as thin; update the condition to apply to both row kinds when
row.state is .done and isRiding(index) is false. Preserve the route-colored .bar
styling for active or still-riding segments.
- Around line 63-70: Update the rail view’s appearance handling to scroll to the
initial currentRowID when the sheet first displays. Add an onAppear path that
reuses the existing tethered guard and scrollProxy.scrollTo behavior from the
currentRowID onChange handler, preserving the centered anchor and avoiding
scrolling when focusedLegIndex is set.
In
`@OTPKit/Sources/OTPKit/Presentation/Sheets/Directions/Rail/RailRowContentViews.swift`:
- Around line 240-266: Update rideSummaryLine and subtitleLine so missing
leg.headsign values still render the available stop count or duration, using a
headsign-less localized summary instead of returning no view. Preserve the
existing headsign-based formatting when a headsign exists and fall back between
progress.totalStops(onLegAt:) and leg.duration as currently done.
In `@OTPKit/Sources/OTPKit/Presentation/Sheets/Directions/Rail/RouteBadge.swift`:
- Around line 37-52: The isPerceptuallyLight property should linearize each sRGB
red, green, and blue component before calculating relative luminance, then
choose the result based on which foreground—black or white—produces the higher
WCAG contrast ratio rather than using a fixed luminance threshold. Preserve the
existing UIColor extraction fallback behavior.
In `@OTPKit/Sources/OTPKit/Resources/ru.lproj/Localizable.strings`:
- Line 269: Update the rail.transfers_fmt localization to use count-aware
Russian pluralization, matching the existing rail.stops_fmt or
rail.stops_to_go_fmt pattern; ensure 2–4 transfers use “пересадки” while 0 and
5+ retain “пересадок”.
---
Nitpick comments:
In `@OTPKit/Sources/OTPKit/Network/GraphQLAPIService.swift`:
- Around line 187-189: Update the GraphQL query around realTime, departureDelay,
and arrivalDelay to use the supported end.estimated.delay and
start.estimated.delay fields instead of deprecated Leg delay fields. Update
GraphQLLeg-to-Leg response mapping consistently so delay values retain their
current semantics, and verify the fields against the supported OTP server
versions.
In
`@OTPKit/Sources/OTPKit/Presentation/Sheets/Directions/Rail/TipContentView.swift`:
- Around line 249-283: Move the next-boarding selection rule from
TipContentView.footerLeadingText into TripProgress as a rider-facing query,
preserving the currentLegIndex and riding/waiting exclusion behavior. Update
footerLeadingText to call that TripProgress API and retain only localization and
formatting, adding or updating TripProgressTests to cover the lookup rule.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: eec09fcf-8d3d-4682-be63-122b51feeff3
📒 Files selected for processing (32)
OTPKit/Sources/OTPKit/Core/Models/OTP/Leg.swiftOTPKit/Sources/OTPKit/Core/Models/OTP/RealTimeStatus.swiftOTPKit/Sources/OTPKit/Core/Models/OTP/RelativeDirection.swiftOTPKit/Sources/OTPKit/Core/TripProgress/TripProgress.swiftOTPKit/Sources/OTPKit/Core/Utilities/Formatters.swiftOTPKit/Sources/OTPKit/Network/GraphQLAPIService.swiftOTPKit/Sources/OTPKit/Network/GraphQLPlanResponse.swiftOTPKit/Sources/OTPKit/Presentation/Sheets/Directions/Components/DirectionLegs/DirectionLegWalkView.swiftOTPKit/Sources/OTPKit/Presentation/Sheets/Directions/DirectionsSheetView.swiftOTPKit/Sources/OTPKit/Presentation/Sheets/Directions/PagedDirectionsView.swiftOTPKit/Sources/OTPKit/Presentation/Sheets/Directions/Rail/InTripRailView.swiftOTPKit/Sources/OTPKit/Presentation/Sheets/Directions/Rail/RailMarkColumn.swiftOTPKit/Sources/OTPKit/Presentation/Sheets/Directions/Rail/RailRowContentViews.swiftOTPKit/Sources/OTPKit/Presentation/Sheets/Directions/Rail/RailRowView.swiftOTPKit/Sources/OTPKit/Presentation/Sheets/Directions/Rail/RouteBadge.swiftOTPKit/Sources/OTPKit/Presentation/Sheets/Directions/Rail/TipContentView.swiftOTPKit/Sources/OTPKit/Presentation/Sheets/Directions/Rail/TripHeaderView.swiftOTPKit/Sources/OTPKit/Presentation/Sheets/Directions/Rail/TripProgressBarView.swiftOTPKit/Sources/OTPKit/Resources/ar.lproj/Localizable.stringsOTPKit/Sources/OTPKit/Resources/en.lproj/Localizable.stringsOTPKit/Sources/OTPKit/Resources/es.lproj/Localizable.stringsOTPKit/Sources/OTPKit/Resources/fil.lproj/Localizable.stringsOTPKit/Sources/OTPKit/Resources/fr.lproj/Localizable.stringsOTPKit/Sources/OTPKit/Resources/it.lproj/Localizable.stringsOTPKit/Sources/OTPKit/Resources/ko.lproj/Localizable.stringsOTPKit/Sources/OTPKit/Resources/pl.lproj/Localizable.stringsOTPKit/Sources/OTPKit/Resources/pt-BR.lproj/Localizable.stringsOTPKit/Sources/OTPKit/Resources/ru.lproj/Localizable.stringsOTPKit/Sources/OTPKit/Resources/vi.lproj/Localizable.stringsOTPKit/Sources/OTPKit/Resources/zh-Hans.lproj/Localizable.stringsOTPKit/Sources/OTPKit/Resources/zh-Hant.lproj/Localizable.stringsOTPKit/Tests/TripProgressTests.swift
💤 Files with no reviewable changes (1)
- OTPKit/Sources/OTPKit/Presentation/Sheets/Directions/PagedDirectionsView.swift
Comment on lines
+143
to
+147
| case .board(let index), .ride(let index): | ||
| if row.state == .done, case .board = row.kind, !isRiding(index) { | ||
| return .thin | ||
| } | ||
| return .bar(progress.legs[index].routeUIColor ?? Color(.systemGray2)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Completed ride spans stay styled as active transit.
A completed .ride always returns .bar; only completed .board rows become thin. After alighting, the completed ride segment remains route-colored, contrary to the stated “ahead of (or under) the rider” rule. Apply the completed/non-riding check to both .board and .ride.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@OTPKit/Sources/OTPKit/Presentation/Sheets/Directions/Rail/InTripRailView.swift`
around lines 143 - 147, The completed-row styling in the .board/.ride case only
treats .board as thin; update the condition to apply to both row kinds when
row.state is .done and isRiding(index) is false. Preserve the route-colored .bar
styling for active or still-riding segments.
Comment on lines
+240
to
+266
| @ViewBuilder | ||
| private var rideSummaryLine: some View { | ||
| if let headsign = leg.headsign, let total = progress.totalStops(onLegAt: legIndex) { | ||
| let ride = OTPLoc("rail.ride_stops_duration_fmt", | ||
| comment: "Stop count and duration of the ride", | ||
| RailText.stops(total), | ||
| Formatters.formatTimeDuration(leg.duration)) | ||
| Text(OTPLoc("rail.toward_stops_fmt", | ||
| comment: "Vehicle headsign, then ride summary", headsign, ride)) | ||
| .font(.subheadline) | ||
| .foregroundStyle(.secondary) | ||
| } else { | ||
| subtitleLine | ||
| } | ||
| } | ||
|
|
||
| @ViewBuilder | ||
| private var subtitleLine: some View { | ||
| if let headsign = leg.headsign { | ||
| let stops = progress.totalStops(onLegAt: legIndex).map(RailText.stops) ?? | ||
| Formatters.formatTimeDuration(leg.duration) | ||
| Text(OTPLoc("rail.toward_stops_fmt", | ||
| comment: "Vehicle headsign, then ride summary", headsign, stops)) | ||
| .font(.subheadline) | ||
| .foregroundStyle(.secondary) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Boarding detail loses stop/duration info entirely when leg.headsign is nil.
Both rideSummaryLine and subtitleLine are gated on leg.headsign being present. If a transit leg has no headsign (plausible for some agencies), the entire secondary line in waitingDetail/focusedDetail/summary disappears — riders get no stop count or duration for that boarding, even though progress.totalStops or leg.duration may still be available.
Consider falling back to a headsign-less variant (e.g. just "12 stops · 8 min") instead of rendering nothing.
♻️ Possible fallback
`@ViewBuilder`
private var subtitleLine: some View {
if let headsign = leg.headsign {
let stops = progress.totalStops(onLegAt: legIndex).map(RailText.stops) ??
Formatters.formatTimeDuration(leg.duration)
Text(OTPLoc("rail.toward_stops_fmt",
comment: "Vehicle headsign, then ride summary", headsign, stops))
.font(.subheadline)
.foregroundStyle(.secondary)
+ } else if let total = progress.totalStops(onLegAt: legIndex) {
+ Text(RailText.stops(total))
+ .font(.subheadline)
+ .foregroundStyle(.secondary)
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@OTPKit/Sources/OTPKit/Presentation/Sheets/Directions/Rail/RailRowContentViews.swift`
around lines 240 - 266, Update rideSummaryLine and subtitleLine so missing
leg.headsign values still render the available stop count or duration, using a
headsign-less localized summary instead of returning no view. Preserve the
existing headsign-based formatting when a headsign exists and fall back between
progress.totalStops(onLegAt:) and leg.duration as currently done.
| "rail.arrive_at_fmt" = "Прибытие: %@"; | ||
| "rail.time_left_fmt" = "Осталось %@"; | ||
| "rail.transfer_one" = "1 пересадка"; | ||
| "rail.transfers_fmt" = "Пересадок: %d"; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the relevant localization keys and nearby count-based patterns.
fd -a 'Localizable.strings$' . | sed 's#^\./##'
echo '--- targeted matches ---'
rg -n 'rail\.(transfers|transfers_fmt|transfer_one|stops_fmt|stops_to_go_fmt)|rail_stops|stops_to_go' OTPKit/Sources/OTPKit/Resources -g '*.strings' || true
echo '--- relevant file snippet ---'
sed -n '168,276p' OTPKit/Sources/OTPKit/Resources/ru.lproj/Localizable.strings
EOFRepository: OneBusAway/otpkit
Length of output: 10809
Use a count-aware plural string for rail.transfers_fmt.
"Пересадок: %d" is only correct for 0/5+ transfers; for 2, 3, 4 transfers the noun should be genitive singular (пересадки). Use .stringsdict plurals or the abbreviated stop pattern already used for rail.stops_fmt/rail.stops_to_go_fmt.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@OTPKit/Sources/OTPKit/Resources/ru.lproj/Localizable.strings` at line 269,
Update the rail.transfers_fmt localization to use count-aware Russian
pluralization, matching the existing rail.stops_fmt or rail.stops_to_go_fmt
pattern; ensure 2–4 transfers use “пересадки” while 0 and 5+ retain “пересадок”.
- Scroll the rail to the current row on first display, not just on auto-advance (onChange never fires for the initial value). - Keep the boarding subtitle when a leg has no headsign: fall back to the stop count or duration instead of dropping the line. - Pick RouteBadge text color by WCAG contrast ratio over linearized sRGB, instead of a gamma-encoded luminance threshold. - Make the tip detent a CustomPresentationDetent that resolves to 300pt at accessibility text sizes (scrolling past that), so Dynamic Type never clips against the fixed 160pt height. - Move the footer's next-boarding rule into TripProgress as nextBoardingIndex, with a unit test. - Give the rail enough bottom padding that the Back-to-now pill can't cover the Arrive row (simulator-found).
Member
Author
|
Addressed the review feedback in 7d5900c: Fixed
Not changed, with reasons
Verified after the changes: 184 package tests pass, SwiftLint clean, and a full simulator walkthrough (plan → start trip → tip detent at 160pt → expand → focus/back-to-now → end trip) passed. The walkthrough also caught the Back-to-now pill overlapping the final Arrive row, fixed in the same commit. |
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
1 participant
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.
Summary
currentLegadvances only with the clock via a pure, unit-testedTripProgressengine;focusedLegfollows taps and the tip-detent ‹ › stepper, with a Back-to-now pill to re-tether.Legnow decodesdepartureDelay/arrivalDelay(REST + GraphQL) feeding the fixed status vocabulary (On time / n min late / n min early / Scheduled). 47 new strings localized across all 13 locales.Test plan
TripProgressphase machine, rail row states, stops-remaining estimation, same-stop transfers, segments, delay decoding, localization key parity across 13 locales)main, pointed at this branch's package) builds against the new OTPKitReview notes
Non-blocking findings from the multi-agent review, left as-is deliberately — reviewer input welcome:
tripStartedlives as@Stateon the sheet rather than inTripPlannerViewModel; it resets if the sheet is ever re-created mid-presentation (not observed in practice).TripProgressbuilds onItinerary.relevantLegs, which drops ≤60s walk legs; a trip that ends with a sub-minute walk can announce arrival up to a minute early, and one that starts with one shows the pre-trip banner for its first minute..stringsdict; ru/pl/ar translators worked around it with count-neutral phrasing.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by CodeRabbit
New Features
Bug Fixes