Skip to content

Commit

Permalink
fix: Display error state when trip doesn't exist (#527)
Browse files Browse the repository at this point in the history 
* fix: Display error state when trip doesn't exist

We can get added trip IDs that don't exist in the API in trip details,
which before these changes was resulting in shimmer loading forever.
Worse, it would continue to shimmer load even after switching to a
different vehicle on the map.

* fix: Fix tests and PR feedback
  • Loading branch information
@EmmaSimon
EmmaSimon authored Nov 12, 2024
1 parent b7f6432 commit 49c744d
Show file tree
Hide file tree
Showing 6 changed files with 103 additions and 61 deletions.
19 changes: 14 additions & 5 deletions iosApp/iosApp/Pages/TripDetails/TripDetailsPage.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ struct TripDetailsPage: View {
@State var globalResponse: GlobalResponse?
@State var tripPredictionsRepository: ITripPredictionsRepository
@State var tripPredictions: PredictionsStreamDataResponse?
@State var tripPredictionsLoaded: Bool = false
@State var tripRepository: ITripRepository
@State var trip: Trip?
@State var tripSchedulesResponse: TripSchedulesResponse?
Expand Down Expand Up @@ -71,9 +72,10 @@ struct TripDetailsPage: View {
var body: some View {
VStack(spacing: 16) {
header
if let trip, let globalResponse, let vehicle = vehicleResponse?.vehicle,
if tripPredictionsLoaded, let globalResponse, let vehicle = vehicleResponse?.vehicle,
let stops = TripDetailsStopList.companion.fromPieces(
trip: trip,
tripId: tripId,
directionId: trip?.directionId ?? vehicle.directionId,
tripSchedules: tripSchedulesResponse,
tripPredictions: tripPredictions,
vehicle: vehicle,
Expand Down Expand Up @@ -120,6 +122,10 @@ struct TripDetailsPage: View {
.onAppear { joinRealtime() }
.onDisappear { leaveRealtime() }
.onChange(of: tripId) {
errorBannerVM.errorRepository.clearDataError(key: "TripDetailsPage.loadTripSchedules")
errorBannerVM.errorRepository.clearDataError(key: "TripDetailsPage.loadTrip")
loadTripSchedules()
loadTrip()
leavePredictions()
joinPredictions(tripId: $0)
}
Expand Down Expand Up @@ -221,12 +227,14 @@ struct TripDetailsPage: View {
case let .ok(result): tripPredictions = result.data
case .error: break
}
tripPredictionsLoaded = true
checkPredictionsStale()
}
}
}

private func leavePredictions() {
tripPredictionsLoaded = false
tripPredictionsRepository.disconnect()
}

Expand Down Expand Up @@ -276,7 +284,7 @@ struct TripDetailsPage: View {
} else {
nil
}
let route: Route? = if let routeId = trip?.routeId {
let route: Route? = if let routeId = trip?.routeId ?? vehicle?.routeId {
globalResponse?.routes[routeId]
} else {
nil
Expand All @@ -285,14 +293,15 @@ struct TripDetailsPage: View {
vehicle: vehicle,
route: route,
stop: vehicleStop,
trip: trip
tripId: tripId
)
}

@ViewBuilder
var header: some View {
let trip: Trip? = tripPredictions?.trips[tripId]
let route: Route? = if let routeId = trip?.routeId {
let vehicle: Vehicle? = vehicleResponse?.vehicle
let route: Route? = if let routeId = trip?.routeId ?? vehicle?.routeId {
globalResponse?.routes[routeId]
} else {
nil
Expand Down
14 changes: 7 additions & 7 deletions iosApp/iosApp/Pages/TripDetails/VehicleCardView.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,18 +14,18 @@ struct VehicleCardView: View {
let vehicle: Vehicle?
let route: Route?
let stop: Stop?
let trip: Trip?
let tripId: String

var body: some View {
if let vehicle, let route, let stop, let trip {
if let vehicle, let route, let stop {
VehicleOnTripView(
vehicle: vehicle,
route: route,
stop: stop,
trip: trip
tripId: tripId
)
} else {
Text("Loading...")
EmptyView()
}
}
}
Expand All @@ -34,7 +34,7 @@ struct VehicleOnTripView: View {
let vehicle: Vehicle
let route: Route
let stop: Stop
let trip: Trip
let tripId: String

var backgroundColor: Color {
Color(hex: route.color)
Expand All @@ -58,7 +58,7 @@ struct VehicleOnTripView: View {

@ViewBuilder
private var description: some View {
if vehicle.tripId == trip.id {
if vehicle.tripId == tripId {
VStack(alignment: .leading, spacing: 2) {
vehicleStatusDescription(vehicle.currentStatus)
.font(Typography.caption)
Expand Down Expand Up @@ -162,7 +162,7 @@ struct VehicleCardView_Previews: PreviewProvider {
}

List {
VehicleCardView(vehicle: vehicle, route: red, stop: stop, trip: trip)
VehicleCardView(vehicle: vehicle, route: red, stop: stop, tripId: trip.id)
}
.previewDisplayName("VehicleCard")
}
Expand Down
37 changes: 20 additions & 17 deletions iosApp/iosAppTests/Pages/TripDetails/TripDetailsPageTests.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -172,21 +172,21 @@ final class TripDetailsPageTests: XCTestCase {
let vehicleId = "999"
let nearbyVM = NearbyViewModel()
nearbyVM.alerts = .init(alerts: [:])
var sut = TripDetailsPage(
let sut = TripDetailsPage(
tripId: tripId,
vehicleId: vehicleId,
routeId: trip.routeId,
target: .init(stopId: stop1.id, stopSequence: 998),
errorBannerVM: .init(),
nearbyVM: nearbyVM,
mapVM: .init(),
globalRepository: MockGlobalRepository(response: .init(objects: objects, patternIdsByStop: [:])),
globalRepository: MockGlobalRepository(response: .init(objects: objects)),
tripPredictionsRepository: FakeTripPredictionsRepository(response: .init(objects: objects)),
tripRepository: tripRepository,
vehicleRepository: FakeVehicleRepository(response: .init(vehicle: vehicle))
)

let splitViewExp = sut.on(\.didLoadData) { view in
let splitViewExp = sut.inspection.inspect(onReceive: tripSchedulesLoaded, after: 0.5) { view in
XCTAssertNotNil(try view.find(TripDetailsStopListSplitView.self))
}

Expand Down Expand Up @@ -307,15 +307,13 @@ final class TripDetailsPageTests: XCTestCase {
wait(for: [routeExp], timeout: 5)
}

@MainActor func testTripRequestError() throws {
@MainActor func testMissingTrip() throws {
let objects = ObjectCollectionBuilder()

let trip = objects.trip { trip in
trip.routeId = "Red"
}
let tripId = "ADDED-trip"

let vehicle = objects.vehicle { vehicle in
vehicle.tripId = trip.id
vehicle.tripId = tripId
vehicle.currentStatus = .inTransitTo
}

Expand All @@ -336,15 +334,16 @@ final class TripDetailsPageTests: XCTestCase {
onConnect: { _ in tripPredictionsLoaded.send() }
)

let tripId = trip.id
let vehicleId = vehicle.id
let nearbyVM = NearbyViewModel(alertsRepository: MockAlertsRepository())
nearbyVM.alerts = .init(alerts: [:])
let sut = TripDetailsPage(
tripId: tripId,
vehicleId: vehicleId,
routeId: trip.routeId,
routeId: "Red",
target: nil,
errorBannerVM: .init(),
nearbyVM: .init(),
nearbyVM: nearbyVM,
mapVM: .init(),
globalRepository: MockGlobalRepository(response: globalData),
tripPredictionsRepository: tripPredictionsRepository,
Expand All @@ -355,9 +354,8 @@ final class TripDetailsPageTests: XCTestCase {
let everythingLoaded = tripSchedulesLoaded.zip(tripPredictionsLoaded)

let routeExp = sut.inspection.inspect(onReceive: everythingLoaded, after: 0.1) { view in
XCTAssertNotNil(try view.find(where: { view in
(try? view.modifier(LoadingPlaceholderModifier.self)) != nil
}))
XCTAssertNotNil(try view.find(VehicleCardView.self))
XCTAssertThrowsError(try view.find(TripDetailsStopView.self))
}

ViewHosting.host(view: sut)
Expand Down Expand Up @@ -647,11 +645,11 @@ final class TripDetailsPageTests: XCTestCase {
}

class FakeTripPredictionsRepository: ITripPredictionsRepository {
let response: PredictionsStreamDataResponse
let response: PredictionsStreamDataResponse?
let onConnect: ((_ tripId: String) -> Void)?
let onDisconnect: (() -> Void)?

init(response: PredictionsStreamDataResponse,
init(response: PredictionsStreamDataResponse?,
onConnect: ((_ tripId: String) -> Void)? = nil,
onDisconnect: (() -> Void)? = nil) {
self.response = response
Expand All @@ -664,7 +662,12 @@ final class TripDetailsPageTests: XCTestCase {
onReceive: @escaping (ApiResult<PredictionsStreamDataResponse>) -> Void
) {
onConnect?(tripId)
onReceive(ApiResultOk(data: response))
let result = if let response {
ApiResultOk(data: response)
} else {
ApiResultError<PredictionsStreamDataResponse>(code: 404, message: "Failed to load predictions")
}
onReceive(result)
}

var lastUpdated: Instant?
Expand Down
12 changes: 6 additions & 6 deletions iosApp/iosAppTests/Pages/TripDetails/VehicleCardViewTests.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ final class VehicleCardViewTests: XCTestCase {
}
let stop = objects.stop { _ in }

let sut = VehicleCardView(vehicle: vehicle, route: route, stop: stop, trip: trip)
let sut = VehicleCardView(vehicle: vehicle, route: route, stop: stop, tripId: trip.id)
XCTAssertNotNil(try sut.inspect().find(text: "This vehicle is completing another trip"))
}

Expand All @@ -49,7 +49,7 @@ final class VehicleCardViewTests: XCTestCase {
stop.name = "Davis"
}

let sut = VehicleCardView(vehicle: vehicle, route: route, stop: stop, trip: trip)
let sut = VehicleCardView(vehicle: vehicle, route: route, stop: stop, tripId: trip.id)
XCTAssertNotNil(try sut.inspect().find(text: "Approaching"))
XCTAssertNotNil(try sut.inspect().find(text: "Davis"))
}
Expand All @@ -67,7 +67,7 @@ final class VehicleCardViewTests: XCTestCase {
let route = objects.route { _ in }
let stop = objects.stop { _ in }

let sut = VehicleCardView(vehicle: vehicle, route: route, stop: stop, trip: trip)
let sut = VehicleCardView(vehicle: vehicle, route: route, stop: stop, tripId: trip.id)
XCTAssertNotNil(try sut.inspect().find(text: "Next stop"))
}

Expand All @@ -84,7 +84,7 @@ final class VehicleCardViewTests: XCTestCase {
let route = objects.route { _ in }
let stop = objects.stop { _ in }

let sut = VehicleCardView(vehicle: vehicle, route: route, stop: stop, trip: trip)
let sut = VehicleCardView(vehicle: vehicle, route: route, stop: stop, tripId: trip.id)
XCTAssertNotNil(try sut.inspect().find(text: "Now at"))
}

Expand All @@ -95,7 +95,7 @@ final class VehicleCardViewTests: XCTestCase {
trip.headsign = "Alewife"
}

let sut = VehicleCardView(vehicle: nil, route: nil, stop: nil, trip: trip)
XCTAssertNotNil(try sut.inspect().find(text: "Loading..."))
let sut = VehicleCardView(vehicle: nil, route: nil, stop: nil, tripId: trip.id)
XCTAssertThrowsError(try sut.inspect().find(VehicleOnTripView.self))
}
}
40 changes: 23 additions & 17 deletions shared/src/commonMain/kotlin/com/mbta/tid/mbta_app/model/TripDetailsStopList.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -126,24 +126,28 @@ data class TripDetailsStopList(val stops: List<Entry>) {
}

fun fromPieces(
trip: Trip,
tripId: String,
directionId: Int,
tripSchedules: TripSchedulesResponse?,
tripPredictions: PredictionsStreamDataResponse?,
vehicle: Vehicle?,
alertsData: AlertsStreamDataResponse?,
globalData: GlobalResponse,
): TripDetailsStopList? {
if (tripPredictions == null || alertsData == null) return null
if (alertsData == null) return null
val entries = mutableMapOf<Int, WorkingEntry>()

val predictions =
deduplicatePredictionsByStopSequence(
tripPredictions.predictions.values,
tripSchedules,
globalData
)
var predictions = emptyList<Prediction>()
if (tripPredictions != null) {
predictions =
deduplicatePredictionsByStopSequence(
tripPredictions.predictions.values,
tripSchedules,
globalData
)

predictions.forEach { prediction -> entries.putPrediction(prediction, vehicle) }
predictions.forEach { prediction -> entries.putPrediction(prediction, vehicle) }
}

if (tripSchedules is TripSchedulesResponse.Schedules) {
tripSchedules.schedules.forEach { entries.putSchedule(it) }
Expand All @@ -159,15 +163,15 @@ data class TripDetailsStopList(val stops: List<Entry>) {
}

if (entries.isEmpty()) {
return null
return TripDetailsStopList(emptyList())
}
return TripDetailsStopList(
entries.entries
.sortedBy { it.key }
.dropWhile {
if (
vehicle == null ||
vehicle.tripId != trip.id ||
vehicle.tripId != tripId ||
vehicle.currentStopSequence == null
) {
false
Expand All @@ -179,7 +183,7 @@ data class TripDetailsStopList(val stops: List<Entry>) {
Entry(
globalData.stops.getValue(it.value.stopId),
it.value.stopSequence,
getAlert(it.value, alertsData, globalData, trip),
getAlert(it.value, alertsData, globalData, tripId, directionId),
it.value.schedule,
it.value.prediction,
it.value.vehicle,
Expand Down Expand Up @@ -316,8 +320,9 @@ data class TripDetailsStopList(val stops: List<Entry>) {
while (scheduleIndex in stopIds.indices && predictionIndex in predictions.indices) {
val stopId = stopIds[scheduleIndex]
val prediction = predictions[predictionIndex]
check(Stop.equalOrFamily(stopId, prediction.stopId, globalData.stops)) {
"predictions=$predictions stopIds=$stopIds predictionIndex=$predictionIndex scheduleIndex=$scheduleIndex"
if (!Stop.equalOrFamily(stopId, prediction.stopId, globalData.stops)) {
scheduleIndex--
continue
}
lastDelta = lastStopSequence?.minus(prediction.stopSequence)
lastStopSequence = prediction.stopSequence
Expand Down Expand Up @@ -359,7 +364,8 @@ data class TripDetailsStopList(val stops: List<Entry>) {
entry: WorkingEntry,
alertsData: AlertsStreamDataResponse?,
globalData: GlobalResponse,
trip: Trip
tripId: String,
directionId: Int
): Alert? {
val entryTime = entry.prediction?.predictionTime ?: entry.schedule?.scheduleTime
val entryRoute = entry.prediction?.routeId ?: entry.schedule?.routeId
Expand All @@ -371,11 +377,11 @@ data class TripDetailsStopList(val stops: List<Entry>) {
alert.isActive(entryTime) &&
alert.anyInformedEntity {
it.appliesTo(
directionId = trip.directionId,
directionId = directionId,
routeId = entryRoute,
routeType = entryRouteType,
stopId = entry.stopId,
tripId = trip.id
tripId = tripId
)
} &&
// there's no UI yet for secondary alerts in trip details
Expand Down
Loading

0 comments on commit 49c744d

Please sign in to comment.