Bikeshare: VehicleRentalSource, via-point planning, rental Progress Rail - #155
Merged
Conversation
- VehicleRentalSource: stateful viewport-to-snapshot pipeline (coalescing, cancellation, bbox padding, id-keyed diffing, partial-error forwarding) with a separate fetchFailures stream for host availability states - TripPlanRequest.viaPoint wired through both services: GraphQL plan query gains $via (PlanViaLocationInput, verified live against Sound Transit), REST gains intermediatePlaces - TransportMode.transitBikeRental (Transit + Bikeshare) with localized names in all 13 locales; capability-gated like .bikeRental - TripPlanner.createTripPlannerView accepts prefilled viaPoint + mode, the entry point for 'Plan a trip using this bike' - MapCoordinator: BICYCLE legs get bike color/icon; rental pickup/dropoff annotations via new OTPAnnotationType.rentalVehicle in rental purple
- TripProgress: rental rides count as .riding; rental legs produce pickup -> ride -> dropoff rows mirroring the transit board/ride/getOff shape; stopsRemaining stays transit-only; segments gain isRental - Rail views: rental row content, purple marks and ride bars, rental variants in the tip detent and progress bar - Placeholder hygiene: 'Default vehicle type' never reaches the UI (Leg.riderFacingToName falls back to a localized generic) - Legacy views: BICYCLE/BIKE arms in DirectionLegView and the itinerary preview flow, purple-tinted when the ride is a rental - shouldMergeLegs explicitly refuses to merge across a rental boundary - New rail strings localized in all 13 locales; TripProgress rental tests
Services are shared across isolation domains by design: VehicleRentalSource (an actor) fetches through one while hosts hold it on the main actor. The app's Swift 6 region-isolation checking rejects handing a non-Sendable existential into the actor. Conformers are typically actors already.
Simplify: - One placeholder-name predicate (String.isRentalPlaceholderName) behind the three call sites; MapCoordinator uses isRentalRide and riderFacingName; rail row builders and mode icons deduplicated - VehicleRentalSource: identical viewports no longer refetch; the debounce task holds self weakly so an abandoned source deallocates immediately Review fixes: - planQuery only declares/passes via when the request has a via point: GraphQL validates documents statically and the via argument only exists on OTP 2.7+, so declaring it unconditionally broke every plan request against older 2.x servers - wireTransportModes expands composite modes at the serialization boundary in both services, so .transitBikeRental's fabricated raw value can never leak into a request - A failed fetch forgets its viewport so a stationary map retries on the next (identical) region emission instead of staying dimmed forever - fetchFailures buffers only the newest failure (advisory stream; never grows unbounded when unconsumed) - Changing transport mode clears a stale via point; the prefilled planner entry is all-or-nothing and no longer resets state when re-invoked - A gap before a rental leg is now .waiting at the pickup (current pickup row, correct tip copy) instead of a bogus walking phase with no current row New regression tests for each fix; full suite green.
File-length/type-length pragmas for the state machine and GraphQL documents; un-nest the scripted service's call record; reattach a doc comment the lint pragma had orphaned.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds bikeshare transport-mode planning with optional via points, a debounced vehicle-rental source, rental-aware trip progress, map annotations, dedicated itinerary and direction views, localized rental strings, and tests. ChangesRental routing and trip experience
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant TripPlanner
participant TripPlannerViewModel
participant GraphQLAPIService
participant RestAPIService
TripPlanner->>TripPlannerViewModel: provide transportMode and viaPoint
TripPlannerViewModel->>GraphQLAPIService: fetchPlan with TripPlanRequest
GraphQLAPIService-->>TripPlannerViewModel: plan response
TripPlannerViewModel->>RestAPIService: fetchPlan with intermediatePlaces
RestAPIService-->>TripPlannerViewModel: plan response
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 |
3 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
OTPKit/Sources/OTPKit/Network/VehicleRentalSource.swift (3)
177-184: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftNo self-healing retry after a fetch failure.
Clearing
viewportonly enables a retry if the host re-emits a region. On a stationary map with no further region callbacks, the layer stays empty indefinitely. A single delayed retry (or bounded backoff) before surfacing the failure would make recovery independent of host behavior.🤖 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/VehicleRentalSource.swift` around lines 177 - 184, The fetch failure handling around the generation guard must schedule a delayed retry before surfacing the failure, so recovery does not depend on another viewport emission. Add a single retry or bounded-backoff retry using the existing fetch/viewport flow, retain generation checks and viewport reset, and only yield FetchFailure after the retry path is exhausted.
130-144: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
reset()yields a snapshot even when nothing was delivered.A flapping zoom gate calling
setViewport(nil)repeatedly wakes every consumer with a no-op snapshot. Consider yielding only whenremovedis non-empty (or documenting the unconditional emission as intentional "cleared" signalling).🤖 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/VehicleRentalSource.swift` around lines 130 - 144, Update VehicleRentalSource.reset() so snapshotContinuation.yield is called only when the removed collection is non-empty, preventing no-op snapshots when reset occurs without delivered vehicles; preserve clearing state and cancellation behavior.
226-232: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winGuard against padding factors below 1.
boundingBoxPaddingis a public init parameter with no validation;0collapses the box and a negative value inverts it (minimum > maximum), sending a nonsensical bbox to the server.♻️ Suggested guard
func padded(by factor: Double) -> VehicleRentalBoundingBox { - guard factor != 1 else { return self } + guard factor > 1 else { return self }🤖 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/VehicleRentalSource.swift` around lines 226 - 232, Update VehicleRentalBoundingBox.padded(by:) to validate that factor is at least 1 before calculating the adjusted spans; preserve the existing identity behavior for factor == 1 and prevent zero or negative factors from collapsing or inverting the bounding box, using the established safe fallback behavior for invalid input.OTPKit/Sources/OTPKit/Core/Models/OTP/VehicleRentalSnapshot.swift (1)
22-38: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider
Codableconformance for this model.Repo guidelines require models to conform to
Codable; if this diff type is intentionally in-memory only, a short note saying so would keep the exception explicit. As per coding guidelines, "All models must conform toCodablefor JSON serialization."🤖 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/Core/Models/OTP/VehicleRentalSnapshot.swift` around lines 22 - 38, Update VehicleRentalSnapshot to conform to Codable, ensuring all stored properties remain Codable-compatible and synthesized conformance works. If this type is intentionally in-memory only, document that exception directly on the model instead of adding Codable.Source: Coding guidelines
OTPKit/Sources/OTPKit/Core/Models/OTP/TransportMode.swift (1)
90-96: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider an exhaustive switch so future composite modes can't silently skip expansion.
The
self == .transitBikeRentalcheck is easy to miss when a second composite is added; aswitchgets compiler enforcement.♻️ Suggested change
public var wireModes: [TransportMode] { - self == .transitBikeRental ? apiModes : [self] + switch self { + case .transitBikeRental: + return apiModes + case .transit, .walk, .bike, .car, .bikeRental: + return [self] + } }🤖 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/Core/Models/OTP/TransportMode.swift` around lines 90 - 96, Update the wireModes computed property to use an exhaustive switch over TransportMode, returning apiModes for .transitBikeRental and [self] for primitive modes. Structure the switch so adding future composite modes requires an explicit expansion case rather than silently defaulting to [self].OTPKit/Tests/VehicleRentalSourceTests.swift (1)
153-167: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffWall-clock sleeps make these assertions CI-flaky.
The 30/50/200 ms margins around the 1 ms coalescing interval are tight under a loaded runner. Polling the recorded call count with a deadline (or a confirmation-based wait) would be more robust than fixed sleeps.
Also applies to: 207-237, 350-364
🤖 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/Tests/VehicleRentalSourceTests.swift` around lines 153 - 167, Replace the fixed wall-clock sleeps in identicalViewportDeduplicated and the corresponding test sections with a deadline-based poll or confirmation-based wait on the recorded service call count. Ensure each assertion waits until the expected count is observed or the deadline fails, preserving the existing coalescing and deduplication expectations.
🤖 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/TripPlanner.swift`:
- Around line 85-98: Update the prefill logic around modeIsAvailable so it
validates the effective transport mode: use the passed transportMode when
present, otherwise the viewModel’s currently selected mode, and only apply
viaPoint when that mode supports it. Preserve the all-or-nothing behavior so an
unsupported viaPoint/mode combination leaves existing state unchanged.
In `@OTPKit/Tests/VehicleRentalSourceTests.swift`:
- Around line 286-307: Update the scripted results in failureReported() to
include a third, successful VehicleRentalFetchResult for the recovery fetch
after the existing success and failure results. Keep the failure assertion and
subsequent snapshot expectations unchanged.
---
Nitpick comments:
In `@OTPKit/Sources/OTPKit/Core/Models/OTP/TransportMode.swift`:
- Around line 90-96: Update the wireModes computed property to use an exhaustive
switch over TransportMode, returning apiModes for .transitBikeRental and [self]
for primitive modes. Structure the switch so adding future composite modes
requires an explicit expansion case rather than silently defaulting to [self].
In `@OTPKit/Sources/OTPKit/Core/Models/OTP/VehicleRentalSnapshot.swift`:
- Around line 22-38: Update VehicleRentalSnapshot to conform to Codable,
ensuring all stored properties remain Codable-compatible and synthesized
conformance works. If this type is intentionally in-memory only, document that
exception directly on the model instead of adding Codable.
In `@OTPKit/Sources/OTPKit/Network/VehicleRentalSource.swift`:
- Around line 177-184: The fetch failure handling around the generation guard
must schedule a delayed retry before surfacing the failure, so recovery does not
depend on another viewport emission. Add a single retry or bounded-backoff retry
using the existing fetch/viewport flow, retain generation checks and viewport
reset, and only yield FetchFailure after the retry path is exhausted.
- Around line 130-144: Update VehicleRentalSource.reset() so
snapshotContinuation.yield is called only when the removed collection is
non-empty, preventing no-op snapshots when reset occurs without delivered
vehicles; preserve clearing state and cancellation behavior.
- Around line 226-232: Update VehicleRentalBoundingBox.padded(by:) to validate
that factor is at least 1 before calculating the adjusted spans; preserve the
existing identity behavior for factor == 1 and prevent zero or negative factors
from collapsing or inverting the bounding box, using the established safe
fallback behavior for invalid input.
In `@OTPKit/Tests/VehicleRentalSourceTests.swift`:
- Around line 153-167: Replace the fixed wall-clock sleeps in
identicalViewportDeduplicated and the corresponding test sections with a
deadline-based poll or confirmation-based wait on the recorded service call
count. Ensure each assertion waits until the expected count is observed or the
deadline fails, preserving the existing coalescing and deduplication
expectations.
🪄 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: 990778e8-b555-4330-be63-47ab2100f6be
📒 Files selected for processing (44)
OTPKit/Sources/OTPKit/Core/Extensions/ColorExtension.swiftOTPKit/Sources/OTPKit/Core/Extensions/StringExtension.swiftOTPKit/Sources/OTPKit/Core/Map/MapCoordinator.swiftOTPKit/Sources/OTPKit/Core/Map/OTPMapProvider.swiftOTPKit/Sources/OTPKit/Core/Models/OTP/Leg.swiftOTPKit/Sources/OTPKit/Core/Models/OTP/TransportMode.swiftOTPKit/Sources/OTPKit/Core/Models/OTP/TripPlanRequest.swiftOTPKit/Sources/OTPKit/Core/Models/OTP/VehicleRental.swiftOTPKit/Sources/OTPKit/Core/Models/OTP/VehicleRentalSnapshot.swiftOTPKit/Sources/OTPKit/Core/TripProgress/TripProgress.swiftOTPKit/Sources/OTPKit/Network/GraphQLAPIService.swiftOTPKit/Sources/OTPKit/Network/RestAPIService.swiftOTPKit/Sources/OTPKit/Network/VehicleRentalService.swiftOTPKit/Sources/OTPKit/Network/VehicleRentalSource.swiftOTPKit/Sources/OTPKit/Presentation/Sheets/Directions/Components/DirectionLegs/DirectionLegBikeView.swiftOTPKit/Sources/OTPKit/Presentation/Sheets/Directions/Components/DirectionLegs/DirectionLegView.swiftOTPKit/Sources/OTPKit/Presentation/Sheets/Directions/Components/DirectionLegs/DirectionLegWalkView.swiftOTPKit/Sources/OTPKit/Presentation/Sheets/Directions/Rail/InTripRailView.swiftOTPKit/Sources/OTPKit/Presentation/Sheets/Directions/Rail/RailRowContentViews.swiftOTPKit/Sources/OTPKit/Presentation/Sheets/Directions/Rail/TipContentView.swiftOTPKit/Sources/OTPKit/Presentation/Sheets/Directions/Rail/TripProgressBarView.swiftOTPKit/Sources/OTPKit/Presentation/Sheets/TripPlanner/Components/ItineraryLegs/ItineraryLegBikeView.swiftOTPKit/Sources/OTPKit/Presentation/Sheets/TripPlanner/Components/ItineraryPreviewView.swiftOTPKit/Sources/OTPKit/Presentation/TripPlanner.swiftOTPKit/Sources/OTPKit/Presentation/ViewModel/TripPlannerViewModel.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/GraphQLAPIServiceTests.swiftOTPKit/Tests/Helpers/TestFixtures.swiftOTPKit/Tests/TripPlanRequestTests.swiftOTPKit/Tests/TripPlannerViewModelTests.swiftOTPKit/Tests/TripProgressTests.swiftOTPKit/Tests/VehicleRentalSourceTests.swift
Comment on lines
+85
to
+98
| // The prefill is all-or-nothing: a via point paired with an unsupported mode | ||
| // must not be applied alone, or the planner would route the rider through a | ||
| // rental vehicle's location in a mode that can't use it. Nil parameters | ||
| // leave existing state untouched, so re-invoking the factory is harmless. | ||
| let modeIsAvailable = transportMode.map { viewModel.availableTransportModes.contains($0) } ?? true | ||
| if modeIsAvailable { | ||
| if let transportMode { | ||
| viewModel.selectTransportMode(transportMode) | ||
| } | ||
| if let viaPoint { | ||
| // After the mode change: selecting a mode clears any stale via point. | ||
| viewModel.viaPoint = viaPoint | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
A viaPoint passed without transportMode bypasses the all-or-nothing guard.
When transportMode is nil, modeIsAvailable is true and the via point is applied against whatever mode is currently selected — including .walk or a rental mode on a non-rental backend, which is the exact case the comment says must not happen. Validate the effective mode instead.
🐛 Proposed fix
- let modeIsAvailable = transportMode.map { viewModel.availableTransportModes.contains($0) } ?? true
+ let effectiveMode = transportMode ?? viewModel.selectedTransportMode
+ let modeIsAvailable = viewModel.availableTransportModes.contains(effectiveMode)
if modeIsAvailable {📝 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
| // The prefill is all-or-nothing: a via point paired with an unsupported mode | |
| // must not be applied alone, or the planner would route the rider through a | |
| // rental vehicle's location in a mode that can't use it. Nil parameters | |
| // leave existing state untouched, so re-invoking the factory is harmless. | |
| let modeIsAvailable = transportMode.map { viewModel.availableTransportModes.contains($0) } ?? true | |
| if modeIsAvailable { | |
| if let transportMode { | |
| viewModel.selectTransportMode(transportMode) | |
| } | |
| if let viaPoint { | |
| // After the mode change: selecting a mode clears any stale via point. | |
| viewModel.viaPoint = viaPoint | |
| } | |
| } | |
| // The prefill is all-or-nothing: a via point paired with an unsupported mode | |
| // must not be applied alone, or the planner would route the rider through a | |
| // rental vehicle's location in a mode that can't use it. Nil parameters | |
| // leave existing state untouched, so re-invoking the factory is harmless. | |
| let effectiveMode = transportMode ?? viewModel.selectedTransportMode | |
| let modeIsAvailable = viewModel.availableTransportModes.contains(effectiveMode) | |
| if modeIsAvailable { | |
| if let transportMode { | |
| viewModel.selectTransportMode(transportMode) | |
| } | |
| if let viaPoint { | |
| // After the mode change: selecting a mode clears any stale via point. | |
| viewModel.viaPoint = viaPoint | |
| } | |
| } |
🤖 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/TripPlanner.swift` around lines 85 - 98,
Update the prefill logic around modeIsAvailable so it validates the effective
transport mode: use the passed transportMode when present, otherwise the
viewModel’s currently selected mode, and only apply viaPoint when that mode
supports it. Preserve the all-or-nothing behavior so an unsupported
viaPoint/mode combination leaves existing state unchanged.
Member
Author
|
Follow-up on the bypassed pre-push hook: after recreating the simulator device, the full |
Two problems that CI's Xcode 26.2 exposes but the local Xcode 27 beta hides. TripPlanner.swift used CLLocationCoordinate2D for the new viaPoint parameter without importing CoreLocation. The iOS 27 SDK makes CoreLocation visible transitively through UIKit/SwiftUI, so this built locally; the iOS 26.2 SDK does not, so CI failed with "cannot find type 'CLLocationCoordinate2D' in scope". The two existing files using CL types both import MapKit, which genuinely re-exports CoreLocation on both SDKs, which is why they never broke. VehicleRentalSourceTests.failureReported() hung forever. It triggers three fetches but scripted only two results, and ScriptedRentalService makes its last result sticky -- so the recovery fetch failed again, emitting on fetchFailures instead of snapshots while the test awaited snapshots.next(). Added the third success entry the test's own comment assumes. CI never reached this test because the build failed first, so fixing only the import would have left CI hanging instead of failing. Full suite: 229 tests in 20 suites pass, SwiftLint --strict clean.
aaronbrethorst
added a commit
to mosliem/onebusaway-ios
that referenced
this pull request
Jul 30, 2026
The rental layers consume VehicleRentalSource, TripPlanRequest.viaPoint, and TransportMode.transitBikeRental from OneBusAway/otpkit#155. Switch back to a version range once that merges and a release is tagged.
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
Completes the OTPKit side of the bikeshare spec (v5.1) on top of #154:
VehicleRentalSource— a stateful actor turning viewport updates into diffedVehicleRentalSnapshots: 250 ms trailing coalescing, cancel-in-flight, identical-viewport dedupe, 1.1× bbox padding, id-keyedadded/removed/updateddiffing, partial-error forwarding, and a separate boundedfetchFailuresstream that drives host "unavailable" states. Hosts apply the diff directly; no debounce/cancellation/diffing lives in app code.TripPlanRequest.viaPointflows to both services: the GraphQLplanquery gainsvia: [PlanViaLocationInput!](verified live against Sound Transit; the argument is declared in the query document only when a via point is present, since it only exists on OTP 2.7+), and REST gainsintermediatePlaces.TripPlanner.createTripPlannerView(viaPoint:transportMode:)is the prefilled entry behind "Plan a trip using this bike".TransportMode.transitBikeRental("Transit + Bikeshare") — capability-gated like.bikeRental; both serializers expand composites throughwireTransportModesso its fabricated raw value can never reach the wire. Localized in all 13 locales..waitingphase at the pickup; rental purple accents in the rail, tip detent, and progress bar; the feed placeholder "Default vehicle type" never reaches the UI (Leg.riderFacingToName).OTPAnnotationType.rentalVehicle(rental purple #7B4FD1), rental pickup/dropoff annotations,BICYCLElegs get bike color/icon, and the legacyDirectionLegView/itinerary-preview flows get bicycle/rental arms.VehicleRentalServiceis nowSendable(services are shared across isolation domains by design).Verified live findings encoded in code/docs: via routing fails with
NO_DIRECT_MODE_CONNECTIONunless a transit mode is in the request, so the plan-a-trip entry should pair the via point with.transitBikeRental.Test plan
xcodebuild testsuite green (all suites incl. ~25 new tests: source coalescing/cancellation/diffing/dedupe/failure-retry, via serialization + omission, composite-mode expansion, rental rail phases/rows, placeholder hygiene, merge-guard)planQuerydocument POSTed to the live Sound Transit server, with and without via — routes through the via location and picks up a rental (rentedBike: truelegs)vehicleRentalsByBboxexercised end-to-end through the OBA iOS app against the live Lime Seattle feed (~12.7k entities) — see the companion OneBusAway/onebusaway-ios PR for screenshotsswiftlint lint --strictmanually)--no-verify: localxcodebuild testwedged mid-session (testmanagerd crash; every subsequent test session hangs at startup across a CoreSimulator reset and a recreated device). The hook's two checks were run manually instead — SwiftLint strict is clean, the full suite passed on the last behavior-affecting commit, and the final (comment/lint-pragma-only) commit was verified withbuild-for-testing. CI should be the arbiter here.Review notes
Deliberate calls a reviewer may want to weigh in on:
TripProgress.Segment.isRentalis derivable fromlegs[legIndex]but kept so the publicSegmentis self-describing.Leghelper.MapCoordinator/DirectionLegViewextend raw mode-string switches ("BICYCLE", "BIKE") per local convention instead of migrating those switches toLegMode..transitBikeRentallives inTransportModerather than a separate UI-selection type; thewireTransportModesboundary keeps it off the wire.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Localization