From dad6c85fbffdacb54d38bc3f002194943b737812 Mon Sep 17 00:00:00 2001 From: mosliem Date: Fri, 21 Aug 2026 23:23:07 +0300 Subject: [PATCH 1/5] Let hosts embed TripPlannerView in their own navigation TripPlannerView wrapped itself in a NavigationStack with its own title and close button, which is right for a full-screen or modal presentation but produces two headers and two close controls inside navigation a host already owns. Adds a `chrome` parameter, defaulted to `.standalone` so existing callers are unaffected. `.embedded` renders the planner body alone and leaves the container, title and close affordance to the host. Navigation-scoped modifiers move into the standalone branch, since navigationTitle and toolbar are no-ops without an enclosing container. An embedded host owns the close control, so it also needs the cleanup the close button used to perform: TripPlanner.reset() exposes it. --- .../OTPKit/Presentation/TripPlanner.swift | 20 ++- .../TripPlanner/TripPlannerView.swift | 130 ++++++++++++------ OTPKit/Tests/TripPlannerChromeTests.swift | 85 ++++++++++++ 3 files changed, 192 insertions(+), 43 deletions(-) create mode 100644 OTPKit/Tests/TripPlannerChromeTests.swift diff --git a/OTPKit/Sources/OTPKit/Presentation/TripPlanner.swift b/OTPKit/Sources/OTPKit/Presentation/TripPlanner.swift index 5ebf4b9..f4178aa 100644 --- a/OTPKit/Sources/OTPKit/Presentation/TripPlanner.swift +++ b/OTPKit/Sources/OTPKit/Presentation/TripPlanner.swift @@ -75,12 +75,17 @@ public class TripPlanner { /// a via point, so pair this with `.transitBikeRental` rather than `.bikeRental`. /// - transportMode: Preselected transport mode. Ignored when the injected API /// service cannot support it (e.g. rental modes on an OTP 1.x REST backend). - /// - onClose: Called when the rider dismisses the planner. + /// - chrome: Whether the planner supplies its own navigation container, title + /// and close button. Pass `.embedded` when presenting inside navigation the + /// host already owns, and call `reset()` when dismissing. + /// - onClose: Called when the rider dismisses the planner. Never called when + /// `chrome` is `.embedded`, which renders no close button. public func createTripPlannerView( origin: Location? = nil, destination: Location? = nil, viaPoint: CLLocationCoordinate2D? = nil, transportMode: TransportMode? = nil, + chrome: TripPlannerChrome = .standalone, onClose: @escaping VoidBlock ) -> some View { // The prefill is all-or-nothing: a via point paired with an unsupported mode @@ -102,7 +107,9 @@ public class TripPlanner { viewModel: viewModel, mapCoordinator: mapCoordinator, origin: origin, - destination: destination, onClose: onClose) + destination: destination, + chrome: chrome, + onClose: onClose) return view .environment(\.otpTheme, viewModel.config.themeConfiguration) @@ -110,4 +117,13 @@ public class TripPlanner { .environmentObject(mapCoordinator) .environmentObject(viewModel) } + + /// Clears planned trip state and anything drawn for it on the map. + /// + /// The `.standalone` close button does this on the rider's behalf. A host using + /// `.embedded` chrome owns the close control instead, so it has to call this as + /// it dismisses — otherwise the next presentation reopens on the previous trip. + public func reset() { + viewModel.resetTripPlanner() + } } diff --git a/OTPKit/Sources/OTPKit/Presentation/TripPlanner/TripPlannerView.swift b/OTPKit/Sources/OTPKit/Presentation/TripPlanner/TripPlannerView.swift index 087f8c1..99ab96f 100644 --- a/OTPKit/Sources/OTPKit/Presentation/TripPlanner/TripPlannerView.swift +++ b/OTPKit/Sources/OTPKit/Presentation/TripPlanner/TripPlannerView.swift @@ -8,8 +8,29 @@ import SwiftUI import MapKit +/// How `TripPlannerView` frames its own content. +public enum TripPlannerChrome: Sendable { + /// Wrap the planner in its own `NavigationStack`, navigation title and close + /// button. Suits a full-screen or modally presented planner, and is the default + /// so existing integrations are unaffected. + case standalone + + /// Render the planner body alone, leaving the navigation container, title and + /// close affordance to the host. + /// + /// For hosts that present the planner inside navigation they already own — a + /// sheet in their own stack, say — where `standalone` would produce two headers + /// and two close buttons. The host is then responsible for dismissal, and should + /// call `TripPlanner.reset()` as it dismisses so the next presentation starts + /// clean; `onClose` is never invoked in this mode, because the control that + /// would call it belongs to the host. + case embedded +} + /// Main view for planning trips, showing controls and results. -/// Full-screen interface with navigation bar, top controls, and inline results. +/// +/// Supplies its own navigation chrome by default; pass `chrome: .embedded` to render +/// the body alone inside a host's own navigation. public struct TripPlannerView: View { /// ViewModel managing trip planning state and logic @StateObject private var tripPlannerVM: TripPlannerViewModel @@ -22,6 +43,9 @@ public struct TripPlannerView: View { @State private var directionSheetDetent: PresentationDetent = DirectionsSheetView.tipDetent + /// Whether this view supplies its own navigation container and close button. + private let chrome: TripPlannerChrome + private let onClose: VoidBlock /// Initializes the TripPlannerView with a map provider, configuration, and optional locations @@ -31,12 +55,16 @@ public struct TripPlannerView: View { /// - mapCoordinator: The MapCoordinator object /// - origin: Optional starting location (if nil, current location will be used) /// - destination: Optional destination location - /// - onClose: A callback invoked when the close button is tapped. + /// - chrome: Whether the view supplies its own navigation container, title and + /// close button. Defaults to `.standalone`. + /// - onClose: A callback invoked when the close button is tapped. Never called + /// when `chrome` is `.embedded`, which renders no close button. public init( viewModel: TripPlannerViewModel, mapCoordinator: MapCoordinator, origin: Location? = nil, destination: Location? = nil, + chrome: TripPlannerChrome = .standalone, onClose: @escaping VoidBlock ) { viewModel.selectedOrigin = origin @@ -44,49 +72,12 @@ public struct TripPlannerView: View { self._tripPlannerVM = StateObject(wrappedValue: viewModel) self._mapCoordinator = StateObject(wrappedValue: mapCoordinator) + self.chrome = chrome self.onClose = onClose } public var body: some View { - NavigationStack { - ScrollView(.vertical, showsIndicators: false) { - LazyVStack(spacing: 0, pinnedViews: []) { - // Top controls for location selection and trip planning - TopControlsOverlay(selectedMode: $selectedMode) - .padding(.bottom, 24) - - // Trip results (shown inline when available) - if !tripPlannerVM.itineraries.isEmpty { - tripResultsSection - .transition(.asymmetric( - insertion: .move(edge: .top).combined(with: .opacity), - removal: .opacity - )) - } - - // Bottom spacer for proper scrolling and safe area - Spacer(minLength: 120) - } - .padding(.top, 8) - } - .scrollDismissesKeyboard(.interactively) - .navigationTitle(OTPLoc("trip_planner.title", comment: "Title of the trip planning screen")) - .toolbarTitleDisplayMode(.inlineLarge) - .toolbar { - ToolbarItem(placement: .topBarTrailing) { - Button(OTPLoc("common.close", comment: "Close button"), systemImage: "xmark") { - tripPlannerVM.resetTripPlanner() - self.onClose() - } - } - } - .overlay { - // Loading overlay - if tripPlannerVM.isLoading { - LoadingOverlay() - } - } - } + framedContent .task { // Auto-set current location as origin if no origin is provided if tripPlannerVM.selectedOrigin == nil { @@ -105,6 +96,63 @@ public struct TripPlannerView: View { .environmentObject(tripPlannerVM) } + // MARK: - Layout + + /// The planner body, identical in both chrome modes. Navigation-scoped modifiers + /// stay out of here: `navigationTitle` and `toolbar` are no-ops without an + /// enclosing container, so they belong to the `.standalone` branch alone. + private var plannerContent: some View { + ScrollView(.vertical, showsIndicators: false) { + LazyVStack(spacing: 0, pinnedViews: []) { + // Top controls for location selection and trip planning + TopControlsOverlay(selectedMode: $selectedMode) + .padding(.bottom, 24) + + // Trip results (shown inline when available) + if !tripPlannerVM.itineraries.isEmpty { + tripResultsSection + .transition(.asymmetric( + insertion: .move(edge: .top).combined(with: .opacity), + removal: .opacity + )) + } + + // Bottom spacer for proper scrolling and safe area + Spacer(minLength: 120) + } + .padding(.top, 8) + } + .scrollDismissesKeyboard(.interactively) + .overlay { + // Loading overlay + if tripPlannerVM.isLoading { + LoadingOverlay() + } + } + } + + @ViewBuilder + private var framedContent: some View { + switch chrome { + case .standalone: + NavigationStack { + plannerContent + .navigationTitle(OTPLoc("trip_planner.title", comment: "Title of the trip planning screen")) + .toolbarTitleDisplayMode(.inlineLarge) + .toolbar { + ToolbarItem(placement: .topBarTrailing) { + Button(OTPLoc("common.close", comment: "Close button"), systemImage: "xmark") { + tripPlannerVM.resetTripPlanner() + self.onClose() + } + } + } + } + case .embedded: + plannerContent + } + } + // MARK: - Trip Results Section private var tripResultsSection: some View { diff --git a/OTPKit/Tests/TripPlannerChromeTests.swift b/OTPKit/Tests/TripPlannerChromeTests.swift new file mode 100644 index 0000000..04b2170 --- /dev/null +++ b/OTPKit/Tests/TripPlannerChromeTests.swift @@ -0,0 +1,85 @@ +/* + * Copyright (C) Open Transit Software Foundation + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import Testing +import CoreLocation +import Foundation +@testable import OTPKit + +/// Covers the embedded-chrome integration path: a host that supplies its own +/// navigation gets no close button from OTPKit, so `TripPlanner.reset()` is the +/// only way it can return the planner to a clean state. +@Suite("TripPlanner embedded chrome") +@MainActor +struct TripPlannerChromeTests { + + private func makePlanner( + mapProvider: MockMapProvider + ) -> TripPlanner { + TripPlanner( + otpConfig: TestFixtures.makeOTPConfiguration(), + apiService: TestFixtures.MockAPIService(), + mapProvider: mapProvider, + notificationCenter: NotificationCenter() + ) + } + + @Test("Both chrome modes build a view without touching planner state") + func chromeModesBuildIndependently() { + let mapProvider = MockMapProvider() + let planner = makePlanner(mapProvider: mapProvider) + + _ = planner.createTripPlannerView(chrome: .standalone) {} + _ = planner.createTripPlannerView(chrome: .embedded) {} + + // Chrome only decides how the body is framed. Choosing it must not clear a + // trip or redraw the map, because a host may rebuild the view repeatedly. + #expect(mapProvider.clearAllRoutesCalls == 0) + #expect(mapProvider.clearAllAnnotationsCalls == 0) + } + + @Test("Chrome defaults to standalone, preserving existing integrations") + func chromeDefaultsToStandalone() { + let mapProvider = MockMapProvider() + let planner = makePlanner(mapProvider: mapProvider) + + // Compiles only while `chrome` has a default, which is what keeps the + // parameter additive for callers that predate it. + _ = planner.createTripPlannerView {} + + #expect(mapProvider.clearAllRoutesCalls == 0) + } + + @Test("reset() clears the route an embedded host cannot close out of") + func resetClearsMapState() { + let mapProvider = MockMapProvider() + let planner = makePlanner(mapProvider: mapProvider) + + _ = planner.createTripPlannerView( + viaPoint: CLLocationCoordinate2D(latitude: 47.6, longitude: -122.3), + transportMode: .transit, + chrome: .embedded + ) {} + + planner.reset() + + // `resetTripPlanner()` routes through the map coordinator, so a cleared + // route and cleared locations are the observable proof it ran. + #expect(mapProvider.clearAllRoutesCalls > 0) + #expect(mapProvider.removeAnnotationCalls.contains("origin")) + #expect(mapProvider.removeAnnotationCalls.contains("destination")) + } +} From f001809dbba8e755490df7ead2fc1828c50c072c Mon Sep 17 00:00:00 2001 From: mosliem Date: Fri, 21 Aug 2026 23:56:51 +0300 Subject: [PATCH 2/5] Wait for the in-flight fetch instead of sleeping in the cancellation test `supersededFetchIsCancelled` slept 50ms and assumed the first fetch was still parked in its 200ms scripted delay. On a contended CI runner that sleep overruns the delay, so the first fetch completes and delivers "stale" before the superseding viewport is ever set -- the failure that turned CI red on this PR and on #161, neither of which touches rental code. The scripted service now signals when a fetch has started, so the test supersedes at the right moment by construction rather than by racing the scheduler. The first fetch's delay is long enough that only cancellation can end it, and the delay is cleared before the second viewport so the superseding fetch returns immediately. --- OTPKit/Tests/VehicleRentalSourceTests.swift | 26 +++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-) diff --git a/OTPKit/Tests/VehicleRentalSourceTests.swift b/OTPKit/Tests/VehicleRentalSourceTests.swift index 3348338..dae7de5 100644 --- a/OTPKit/Tests/VehicleRentalSourceTests.swift +++ b/OTPKit/Tests/VehicleRentalSourceTests.swift @@ -25,6 +25,7 @@ struct VehicleRentalSourceTests { private(set) var calls: [RentalServiceCall] = [] private var results: [Result] private var delay: Duration = .zero + private var callCountWaiters: [(threshold: Int, continuation: CheckedContinuation)] = [] init(results: [Result]) { self.results = results @@ -34,11 +35,29 @@ struct VehicleRentalSourceTests { self.delay = delay } + /// Suspends until at least `count` fetches have started. Tests that need a + /// fetch to be genuinely in flight wait on this rather than sleeping for a + /// fraction of `delay`: a contended CI runner can overrun any such sleep, + /// which makes the assertion race the scheduler instead of testing the source. + func waitForCalls(_ count: Int) async { + guard calls.count < count else { return } + await withCheckedContinuation { continuation in + callCountWaiters.append((count, continuation)) + } + } + + private func notifyCallCountWaiters() { + let ready = callCountWaiters.filter { calls.count >= $0.threshold } + callCountWaiters.removeAll { calls.count >= $0.threshold } + for waiter in ready { waiter.continuation.resume() } + } + func fetchVehicleRentals( in boundingBox: VehicleRentalBoundingBox, formFactors: Set? ) async throws -> VehicleRentalFetchResult { calls.append(RentalServiceCall(boundingBox: boundingBox, formFactors: formFactors)) + notifyCallCountWaiters() // Claim the scripted result at call time, before any delay: a cancelled // call must still consume its result so later calls stay aligned with @@ -212,7 +231,9 @@ struct VehicleRentalSourceTests { .success(VehicleRentalFetchResult(rentals: first)), .success(VehicleRentalFetchResult(rentals: second)) ]) - await service.setDelay(.milliseconds(200)) + // Long enough that the first fetch can only ever leave this sleep by being + // cancelled, so the test never depends on how fast the runner is. + await service.setDelay(.seconds(30)) let source = Self.makeSource(service: service) var snapshots = source.snapshots.makeAsyncIterator() @@ -224,7 +245,8 @@ struct VehicleRentalSourceTests { } await source.setViewport(Self.seattleBox) - try await Task.sleep(for: .milliseconds(50)) // let the first fetch get in flight + await service.waitForCalls(1) // the first fetch is now in flight and parked + await service.setDelay(.zero) // so the superseding fetch can complete await source.setViewport(Self.pannedBox(0.01)) let snapshot = try #require(await snapshots.next()) From 707b6fdd9ca494d41c01270094dda7464a6f1bf2 Mon Sep 17 00:00:00 2001 From: mosliem Date: Sat, 22 Aug 2026 00:39:42 +0300 Subject: [PATCH 3/5] Address review feedback on embeddable chrome MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Tests first, since that was the substance of the review. The three chrome tests asserted things that held regardless of the implementation: building a SwiftUI view struct never touches the map, so the "no map calls" assertions were trivially true, and `MapCoordinator.clearLocations()` removes both annotation identifiers whether or not anything was drawn, so `reset()` looked verified while nothing was. Both now drive a real trip through the view model and assert on state that only changes if the code under test runs. Added ViewInspector coverage of the actual feature — `.standalone` produces a NavigationStack, `.embedded` does not — which nothing tested before; swapping the two cases now fails three tests instead of none. `onClose` was required but could never fire under `.embedded`, so a host passing real dismissal logic got a silent no-op. It is optional now, which keeps every existing trailing-closure call site compiling. `TripPlannerView.init` assigned origin and destination unconditionally, so rebuilding the view — which SwiftUI hosts do freely — cleared the rider's selections, and contradicted the comment claiming nil parameters leave state untouched. Only supplied values are applied now; `reset()` is the deliberate way to clear. Also: moved `TripPlannerChrome` to its own file, corrected the `plannerContent` comment (the navigation modifiers are not no-ops when embedded in a host's stack — they would overwrite the host's title and toolbar, which is the real reason to keep them in the standalone branch), gave `waitForCalls` a deadline so a regression fails with a message instead of hanging until xcodebuild gives up, documented `reset()`'s scope and the dismissal-not-onDisappear timing, and updated the stale signature in CLAUDE.md plus README guidance for embedding. --- CLAUDE.md | 2 +- .../OTPKit/Presentation/TripPlanner.swift | 31 +++-- .../TripPlanner/TripPlannerChrome.swift | 37 +++++ .../TripPlanner/TripPlannerView.swift | 54 ++++---- OTPKit/Tests/TripPlannerChromeTests.swift | 130 ++++++++++++++---- OTPKit/Tests/VehicleRentalSourceTests.swift | 51 +++++-- README.markdown | 13 ++ 7 files changed, 246 insertions(+), 72 deletions(-) create mode 100644 OTPKit/Sources/OTPKit/Presentation/TripPlanner/TripPlannerChrome.swift diff --git a/CLAUDE.md b/CLAUDE.md index 7ea07fa..63f621c 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -45,7 +45,7 @@ Optional pre-push hooks via pre-commit (`pre-commit install --hook-type pre-push 2. An `APIService` implementation. 3. An `OTPMapProvider` implementation. -`TripPlanner` internally wires up `MapCoordinator` and `TripPlannerViewModel`, and `createTripPlannerView(origin:destination:viaPoint:transportMode:onClose:)` returns the SwiftUI UI (`TripPlannerView`), which the demo hosts in a `PanelHostingController` bottom sheet. Cross-object events flow through `NotificationCenter` (injectable; see `Core/Notifications.swift`). +`TripPlanner` internally wires up `MapCoordinator` and `TripPlannerViewModel`, and `createTripPlannerView(origin:destination:viaPoint:transportMode:chrome:onClose:)` returns the SwiftUI UI (`TripPlannerView`), which the demo hosts in a `PanelHostingController` bottom sheet. `chrome` (`TripPlannerChrome`) decides whether the planner supplies its own `NavigationStack`, title and close button (`.standalone`, the default) or renders the body alone inside navigation the host owns (`.embedded`); an embedded host owns dismissal, leaves `onClose` nil, and calls `TripPlanner.reset()` when it dismisses. Cross-object events flow through `NotificationCenter` (injectable; see `Core/Notifications.swift`). ### The map is inversion-of-control diff --git a/OTPKit/Sources/OTPKit/Presentation/TripPlanner.swift b/OTPKit/Sources/OTPKit/Presentation/TripPlanner.swift index f4178aa..1129b57 100644 --- a/OTPKit/Sources/OTPKit/Presentation/TripPlanner.swift +++ b/OTPKit/Sources/OTPKit/Presentation/TripPlanner.swift @@ -26,11 +26,15 @@ public class TripPlanner { /// Map provider for operations private let mapProvider: OTPMapProvider - private let mapCoordinator: MapCoordinator + /// Internal rather than private so tests can observe the state `reset()` clears. + /// Not part of the public API. + let mapCoordinator: MapCoordinator private let notificationCenter: NotificationCenter - private let viewModel: TripPlannerViewModel + /// Internal rather than private so tests can observe the state `reset()` clears. + /// Not part of the public API. + let viewModel: TripPlannerViewModel // MARK: - Initialization @@ -67,8 +71,9 @@ public class TripPlanner { /// Creates the trip planner UI, optionally prefilled. /// /// - Parameters: - /// - origin: Prefilled origin location. - /// - destination: Prefilled destination location. + /// - origin: Prefilled origin location. Nil leaves any existing selection alone. + /// - destination: Prefilled destination location. Nil leaves any existing + /// selection alone. /// - viaPoint: An intermediate coordinate every planned trip must pass through — /// the "plan a trip using this bike" entry point passes the vehicle's location. /// Note: OTP servers may require a transit mode in the request to route through @@ -78,20 +83,22 @@ public class TripPlanner { /// - chrome: Whether the planner supplies its own navigation container, title /// and close button. Pass `.embedded` when presenting inside navigation the /// host already owns, and call `reset()` when dismissing. - /// - onClose: Called when the rider dismisses the planner. Never called when - /// `chrome` is `.embedded`, which renders no close button. + /// - onClose: Called when the rider dismisses the planner. Leave it nil when + /// `chrome` is `.embedded`, which renders no close button and so can never + /// call it. public func createTripPlannerView( origin: Location? = nil, destination: Location? = nil, viaPoint: CLLocationCoordinate2D? = nil, transportMode: TransportMode? = nil, chrome: TripPlannerChrome = .standalone, - onClose: @escaping VoidBlock + onClose: VoidBlock? = nil ) -> some View { // 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. + // leave existing state untouched — here and in `TripPlannerView.init` — so + // re-invoking the factory is harmless. let modeIsAvailable = transportMode.map { viewModel.availableTransportModes.contains($0) } ?? true if modeIsAvailable { if let transportMode { @@ -118,11 +125,17 @@ public class TripPlanner { .environmentObject(viewModel) } - /// Clears planned trip state and anything drawn for it on the map. + /// Clears planned trip state and anything drawn for it on the map: the selected + /// origin, destination and via point, the plan response and selected itinerary, + /// any error, and the route and location annotations on the map. Trip options and + /// the transport mode return to their defaults. /// /// The `.standalone` close button does this on the rider's behalf. A host using /// `.embedded` chrome owns the close control instead, so it has to call this as /// it dismisses — otherwise the next presentation reopens on the previous trip. + /// + /// Call it at the point of dismissal, not from `onDisappear`, which also fires + /// when the host pushes another screen or the app is backgrounded. public func reset() { viewModel.resetTripPlanner() } diff --git a/OTPKit/Sources/OTPKit/Presentation/TripPlanner/TripPlannerChrome.swift b/OTPKit/Sources/OTPKit/Presentation/TripPlanner/TripPlannerChrome.swift new file mode 100644 index 0000000..61e6846 --- /dev/null +++ b/OTPKit/Sources/OTPKit/Presentation/TripPlanner/TripPlannerChrome.swift @@ -0,0 +1,37 @@ +// +// TripPlannerChrome.swift +// OTPKit +// + +import Foundation + +/// How `TripPlannerView` frames its own content. +public enum TripPlannerChrome: Sendable, Equatable { + /// Wrap the planner in its own `NavigationStack`, navigation title and close + /// button. Suits a full-screen or modally presented planner, and is the default + /// so existing integrations are unaffected. + case standalone + + /// Render the planner body alone, leaving the navigation container, title and + /// close affordance to the host. + /// + /// For hosts that present the planner inside navigation they already own — a + /// sheet in their own stack, say — where `standalone` would produce two headers + /// and two close buttons. The host owns dismissal in this mode, so `onClose` is + /// never invoked: the control that would call it belongs to the host. Leave + /// `onClose` nil rather than passing a closure that cannot fire. + /// + /// The host also inherits the cleanup the close button used to perform, so call + /// `TripPlanner.reset()` at the point of dismissal — not on view disappearance, + /// which also fires when the host pushes another screen or backgrounds the app: + /// + /// ```swift + /// .sheet(isPresented: $showingPlanner, onDismiss: { planner.reset() }) { + /// NavigationStack { + /// planner.createTripPlannerView(chrome: .embedded) + /// .navigationTitle("Plan a trip") + /// } + /// } + /// ``` + case embedded +} diff --git a/OTPKit/Sources/OTPKit/Presentation/TripPlanner/TripPlannerView.swift b/OTPKit/Sources/OTPKit/Presentation/TripPlanner/TripPlannerView.swift index 99ab96f..e50eadc 100644 --- a/OTPKit/Sources/OTPKit/Presentation/TripPlanner/TripPlannerView.swift +++ b/OTPKit/Sources/OTPKit/Presentation/TripPlanner/TripPlannerView.swift @@ -8,25 +8,6 @@ import SwiftUI import MapKit -/// How `TripPlannerView` frames its own content. -public enum TripPlannerChrome: Sendable { - /// Wrap the planner in its own `NavigationStack`, navigation title and close - /// button. Suits a full-screen or modally presented planner, and is the default - /// so existing integrations are unaffected. - case standalone - - /// Render the planner body alone, leaving the navigation container, title and - /// close affordance to the host. - /// - /// For hosts that present the planner inside navigation they already own — a - /// sheet in their own stack, say — where `standalone` would produce two headers - /// and two close buttons. The host is then responsible for dismissal, and should - /// call `TripPlanner.reset()` as it dismisses so the next presentation starts - /// clean; `onClose` is never invoked in this mode, because the control that - /// would call it belongs to the host. - case embedded -} - /// Main view for planning trips, showing controls and results. /// /// Supplies its own navigation chrome by default; pass `chrome: .embedded` to render @@ -46,29 +27,40 @@ public struct TripPlannerView: View { /// Whether this view supplies its own navigation container and close button. private let chrome: TripPlannerChrome - private let onClose: VoidBlock + private let onClose: VoidBlock? /// Initializes the TripPlannerView with a map provider, configuration, and optional locations /// This is the main entry point for using OTPKit /// - Parameters: /// - viewModel: The TripPlannerViewModel /// - mapCoordinator: The MapCoordinator object - /// - origin: Optional starting location (if nil, current location will be used) - /// - destination: Optional destination location + /// - origin: Prefilled starting location. Nil leaves any existing selection + /// alone; the view falls back to the current location only when nothing is + /// selected yet. + /// - destination: Prefilled destination location. Nil leaves any existing + /// selection alone. /// - chrome: Whether the view supplies its own navigation container, title and /// close button. Defaults to `.standalone`. - /// - onClose: A callback invoked when the close button is tapped. Never called - /// when `chrome` is `.embedded`, which renders no close button. + /// - onClose: A callback invoked when the close button is tapped. Leave it nil + /// when `chrome` is `.embedded`, which renders no close button and so can + /// never call it. public init( viewModel: TripPlannerViewModel, mapCoordinator: MapCoordinator, origin: Location? = nil, destination: Location? = nil, chrome: TripPlannerChrome = .standalone, - onClose: @escaping VoidBlock + onClose: VoidBlock? = nil ) { - viewModel.selectedOrigin = origin - viewModel.selectedDestination = destination + // Only a supplied value is applied. Assigning nil here would clear the + // rider's selection every time the host rebuilt the view, which SwiftUI + // hosts do freely; `TripPlanner.reset()` is the way to clear on purpose. + if let origin { + viewModel.selectedOrigin = origin + } + if let destination { + viewModel.selectedDestination = destination + } self._tripPlannerVM = StateObject(wrappedValue: viewModel) self._mapCoordinator = StateObject(wrappedValue: mapCoordinator) @@ -99,8 +91,10 @@ public struct TripPlannerView: View { // MARK: - Layout /// The planner body, identical in both chrome modes. Navigation-scoped modifiers - /// stay out of here: `navigationTitle` and `toolbar` are no-ops without an - /// enclosing container, so they belong to the `.standalone` branch alone. + /// stay out of here: an `.embedded` planner sits inside the host's own + /// `NavigationStack`, where `navigationTitle` and `toolbar` would take effect and + /// overwrite the host's title and bar items. They belong to `.standalone`, which + /// supplies the container they are meant to configure. private var plannerContent: some View { ScrollView(.vertical, showsIndicators: false) { LazyVStack(spacing: 0, pinnedViews: []) { @@ -143,7 +137,7 @@ public struct TripPlannerView: View { ToolbarItem(placement: .topBarTrailing) { Button(OTPLoc("common.close", comment: "Close button"), systemImage: "xmark") { tripPlannerVM.resetTripPlanner() - self.onClose() + self.onClose?() } } } diff --git a/OTPKit/Tests/TripPlannerChromeTests.swift b/OTPKit/Tests/TripPlannerChromeTests.swift index 04b2170..e3f9214 100644 --- a/OTPKit/Tests/TripPlannerChromeTests.swift +++ b/OTPKit/Tests/TripPlannerChromeTests.swift @@ -17,6 +17,8 @@ import Testing import CoreLocation import Foundation +import SwiftUI +import ViewInspector @testable import OTPKit /// Covers the embedded-chrome integration path: a host that supplies its own @@ -27,59 +29,139 @@ import Foundation struct TripPlannerChromeTests { private func makePlanner( - mapProvider: MockMapProvider + mapProvider: MockMapProvider, + apiService: TestFixtures.MockAPIService = TestFixtures.MockAPIService() ) -> TripPlanner { TripPlanner( otpConfig: TestFixtures.makeOTPConfiguration(), - apiService: TestFixtures.MockAPIService(), + apiService: apiService, mapProvider: mapProvider, notificationCenter: NotificationCenter() ) } - @Test("Both chrome modes build a view without touching planner state") - func chromeModesBuildIndependently() { + /// Puts a real trip on the planner: origin and destination annotations on the + /// map, and an itinerary in the view model. Without this, assertions about + /// clearing pass vacuously — `MapCoordinator.clearLocations()` removes both + /// annotation identifiers whether or not anything was ever drawn. + private func planTrip(on planner: TripPlanner) async { + let viewModel = planner.viewModel + viewModel.handleLocationSelection(TestHelpers.location(title: "Origin"), for: .origin) + viewModel.handleLocationSelection(TestHelpers.location(title: "Destination"), for: .destination) + viewModel.planTrip() + await viewModel.activePlanTask?.value + } + + // MARK: - Framing + + @Test("standalone wraps the planner in its own navigation chrome") + func standaloneSuppliesNavigationChrome() throws { + let planner = makePlanner(mapProvider: MockMapProvider()) + let view = TripPlannerView( + viewModel: planner.viewModel, + mapCoordinator: planner.mapCoordinator, + chrome: .standalone + ) {} + + let body = try view.inspect() + #expect(throws: Never.self) { try body.find(ViewType.NavigationStack.self) } + } + + @Test("embedded renders the body alone, leaving navigation to the host") + func embeddedOmitsNavigationChrome() throws { + let planner = makePlanner(mapProvider: MockMapProvider()) + let view = TripPlannerView( + viewModel: planner.viewModel, + mapCoordinator: planner.mapCoordinator, + chrome: .embedded + ) + + let body = try view.inspect() + // The distinguishing property of `.embedded`: no container of OTPKit's own, + // so the host's navigation title and toolbar survive. + #expect(throws: (any Error).self) { try body.find(ViewType.NavigationStack.self) } + // The body itself is still there — `.embedded` drops the chrome, not the planner. + #expect(throws: Never.self) { try body.find(ViewType.ScrollView.self) } + } + + // MARK: - Prefill + + @Test("Rebuilding the view in either chrome mode preserves a planned trip") + func chromeModesPreservePlannedTrip() async throws { let mapProvider = MockMapProvider() let planner = makePlanner(mapProvider: mapProvider) + await planTrip(on: planner) + + let routesClearedWhilePlanning = mapProvider.clearAllRoutesCalls _ = planner.createTripPlannerView(chrome: .standalone) {} - _ = planner.createTripPlannerView(chrome: .embedded) {} + _ = planner.createTripPlannerView(chrome: .embedded) - // Chrome only decides how the body is framed. Choosing it must not clear a - // trip or redraw the map, because a host may rebuild the view repeatedly. - #expect(mapProvider.clearAllRoutesCalls == 0) - #expect(mapProvider.clearAllAnnotationsCalls == 0) + // Chrome only decides how the body is framed. A SwiftUI host rebuilds its + // views freely, so neither the rider's selections nor the map may be + // disturbed by doing so. + #expect(planner.viewModel.selectedOrigin != nil) + #expect(planner.viewModel.selectedDestination != nil) + #expect(mapProvider.clearAllRoutesCalls == routesClearedWhilePlanning) + #expect(mapProvider.removeAnnotationCalls.isEmpty) } @Test("Chrome defaults to standalone, preserving existing integrations") - func chromeDefaultsToStandalone() { - let mapProvider = MockMapProvider() - let planner = makePlanner(mapProvider: mapProvider) + func chromeDefaultsToStandalone() throws { + let planner = makePlanner(mapProvider: MockMapProvider()) - // Compiles only while `chrome` has a default, which is what keeps the - // parameter additive for callers that predate it. - _ = planner.createTripPlannerView {} + // The pre-existing call shape: no `chrome`, close handler as a trailing + // closure. It must still compile and still produce standalone chrome. + let view = planner.createTripPlannerView {} - #expect(mapProvider.clearAllRoutesCalls == 0) + #expect(throws: Never.self) { try view.inspect().find(ViewType.NavigationStack.self) } } - @Test("reset() clears the route an embedded host cannot close out of") - func resetClearsMapState() { + // MARK: - reset() + + @Test("reset() clears the trip an embedded host cannot close out of") + func resetClearsPlannedTrip() async throws { let mapProvider = MockMapProvider() let planner = makePlanner(mapProvider: mapProvider) + await planTrip(on: planner) + + _ = planner.createTripPlannerView(chrome: .embedded) + + // Precondition: there is genuinely something to clear. + #expect(planner.viewModel.selectedOrigin != nil) + #expect(planner.viewModel.selectedDestination != nil) + #expect(planner.viewModel.tripPlanResponse != nil) + #expect(mapProvider.addAnnotationCalls.contains { $0.identifier == "origin" }) + #expect(mapProvider.addAnnotationCalls.contains { $0.identifier == "destination" }) + + planner.reset() + + // View model state is the assertion that bites: the map provider removes + // both annotation identifiers unconditionally, so map calls alone would + // pass even if `reset()` did nothing. + #expect(planner.viewModel.selectedOrigin == nil) + #expect(planner.viewModel.selectedDestination == nil) + #expect(planner.viewModel.viaPoint == nil) + #expect(planner.viewModel.tripPlanResponse == nil) + #expect(planner.viewModel.selectedItinerary == nil) + #expect(mapProvider.clearAllRoutesCalls > 0) + #expect(mapProvider.removeAnnotationCalls.contains("origin")) + #expect(mapProvider.removeAnnotationCalls.contains("destination")) + } + + @Test("reset() clears a via point set through the factory") + func resetClearsViaPoint() throws { + let planner = makePlanner(mapProvider: MockMapProvider()) _ = planner.createTripPlannerView( viaPoint: CLLocationCoordinate2D(latitude: 47.6, longitude: -122.3), transportMode: .transit, chrome: .embedded - ) {} + ) + #expect(planner.viewModel.viaPoint != nil) planner.reset() - // `resetTripPlanner()` routes through the map coordinator, so a cleared - // route and cleared locations are the observable proof it ran. - #expect(mapProvider.clearAllRoutesCalls > 0) - #expect(mapProvider.removeAnnotationCalls.contains("origin")) - #expect(mapProvider.removeAnnotationCalls.contains("destination")) + #expect(planner.viewModel.viaPoint == nil) } } diff --git a/OTPKit/Tests/VehicleRentalSourceTests.swift b/OTPKit/Tests/VehicleRentalSourceTests.swift index dae7de5..f9d8787 100644 --- a/OTPKit/Tests/VehicleRentalSourceTests.swift +++ b/OTPKit/Tests/VehicleRentalSourceTests.swift @@ -16,6 +16,17 @@ private struct RentalServiceCall: Sendable { let formFactors: Set? } +/// Thrown when `waitForCalls` gives up, so a source that stops issuing fetches +/// fails the test with a readable message instead of suspending until xcodebuild +/// kills the run. +private struct CallWaitTimeout: Error, CustomStringConvertible { + let expected: Int + let observed: Int + var description: String { + "waitForCalls timed out waiting for \(expected) fetch(es); observed \(observed)" + } +} + @Suite("VehicleRentalSource") struct VehicleRentalSourceTests { @@ -25,7 +36,7 @@ struct VehicleRentalSourceTests { private(set) var calls: [RentalServiceCall] = [] private var results: [Result] private var delay: Duration = .zero - private var callCountWaiters: [(threshold: Int, continuation: CheckedContinuation)] = [] + private var callCountWaiters: [UUID: (threshold: Int, continuation: CheckedContinuation)] = [:] init(results: [Result]) { self.results = results @@ -39,17 +50,41 @@ struct VehicleRentalSourceTests { /// fetch to be genuinely in flight wait on this rather than sleeping for a /// fraction of `delay`: a contended CI runner can overrun any such sleep, /// which makes the assertion race the scheduler instead of testing the source. - func waitForCalls(_ count: Int) async { + /// + /// The deadline is a backstop, not a timing assumption — it is far longer + /// than any healthy fetch needs, and exists only so a regression surfaces as + /// a failed expectation rather than a hung job. + func waitForCalls(_ count: Int, timeout: Duration = .seconds(10)) async throws { guard calls.count < count else { return } - await withCheckedContinuation { continuation in - callCountWaiters.append((count, continuation)) + + let id = UUID() + let timeoutTask = Task { [weak self] in + // A thrown sleep means the wait already finished and cancelled us. + do { try await Task.sleep(for: timeout) } catch { return } + await self?.timeOutWaiter(id, expected: count) + } + defer { timeoutTask.cancel() } + + try await withCheckedThrowingContinuation { continuation in + callCountWaiters[id] = (count, continuation) } } private func notifyCallCountWaiters() { - let ready = callCountWaiters.filter { calls.count >= $0.threshold } - callCountWaiters.removeAll { calls.count >= $0.threshold } - for waiter in ready { waiter.continuation.resume() } + // Removing before resuming keeps a continuation from being resumed + // twice, which would trap. Iteration is over a copy, so mutating the + // dictionary inside the loop is safe. + for (id, waiter) in callCountWaiters where calls.count >= waiter.threshold { + callCountWaiters.removeValue(forKey: id) + waiter.continuation.resume() + } + } + + private func timeOutWaiter(_ id: UUID, expected: Int) { + guard let waiter = callCountWaiters.removeValue(forKey: id) else { return } + waiter.continuation.resume( + throwing: CallWaitTimeout(expected: expected, observed: calls.count) + ) } func fetchVehicleRentals( @@ -245,7 +280,7 @@ struct VehicleRentalSourceTests { } await source.setViewport(Self.seattleBox) - await service.waitForCalls(1) // the first fetch is now in flight and parked + try await service.waitForCalls(1) // the first fetch is now in flight and parked await service.setDelay(.zero) // so the superseding fetch can complete await source.setViewport(Self.pannedBox(0.01)) diff --git a/README.markdown b/README.markdown index 6026e3d..6792b01 100644 --- a/README.markdown +++ b/README.markdown @@ -99,6 +99,19 @@ class ViewController: UIViewController { `createTripPlannerView` returns a plain SwiftUI view, so you're not locked into the bottom-sheet presentation — host it however your app's navigation works. It also accepts optional prefill parameters (`origin`, `destination`, `viaPoint`, `transportMode`) for deep-linking straight into a planned trip. +If you're presenting the planner inside navigation you already own, pass `chrome: .embedded` so OTPKit doesn't add a second header and close button: + +```swift +.sheet(isPresented: $showingPlanner, onDismiss: { planner.reset() }) { + NavigationStack { + planner.createTripPlannerView(chrome: .embedded) + .navigationTitle("Plan a trip") + } +} +``` + +An embedded planner has no close button of its own, so dismissal is yours to handle: leave `onClose` nil and call `TripPlanner.reset()` at your dismissal point, or the next presentation reopens on the previous trip. + ### Which API service do I use? | Your OTP server | Use | Notes | From e2cd1f22fac03ded2b4de2052408af1b8c9b978d Mon Sep 17 00:00:00 2001 From: mosliem Date: Sat, 22 Aug 2026 01:01:45 +0300 Subject: [PATCH 4/5] Prove the cancelled fetch stayed silent instead of sleeping on it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The cancellation assertion slept 50ms and checked the failure stream was still empty, which only ever says nothing arrived yet — the shape most likely to pass for the wrong reason on a contended runner. Script a third fetch that fails for real and require that the first failure out of the stream is that one: a spurious cancellation failure would be buffered ahead of it and surface instead. waitForCalls has its own deadline, but the stream reads do not, so the test takes a time limit to keep a stalled source failing rather than hanging. Removing the collector task leaves Box unused. --- OTPKit/Tests/VehicleRentalSourceTests.swift | 33 +++++++++------------ 1 file changed, 14 insertions(+), 19 deletions(-) diff --git a/OTPKit/Tests/VehicleRentalSourceTests.swift b/OTPKit/Tests/VehicleRentalSourceTests.swift index f9d8787..e2c82b8 100644 --- a/OTPKit/Tests/VehicleRentalSourceTests.swift +++ b/OTPKit/Tests/VehicleRentalSourceTests.swift @@ -258,26 +258,23 @@ struct VehicleRentalSourceTests { #expect(calls.first?.boundingBox == Self.seattleBox) } - @Test("A superseded in-flight fetch is cancelled, not reported as a failure") + // `waitForCalls` carries its own deadline, but the stream reads below do not: + // the time limit is what keeps a source that stops emitting from hanging the job. + @Test("A superseded in-flight fetch is cancelled, not reported as a failure", .timeLimit(.minutes(1))) func supersededFetchIsCancelled() async throws { let first = [Self.makeRental(id: "stale")] let second = [Self.makeRental(id: "fresh")] let service = ScriptedRentalService(results: [ .success(VehicleRentalFetchResult(rentals: first)), - .success(VehicleRentalFetchResult(rentals: second)) + .success(VehicleRentalFetchResult(rentals: second)), + .failure(ScriptedError()) ]) // Long enough that the first fetch can only ever leave this sleep by being // cancelled, so the test never depends on how fast the runner is. await service.setDelay(.seconds(30)) let source = Self.makeSource(service: service) var snapshots = source.snapshots.makeAsyncIterator() - - let failures = Box() - let failureWatcher = Task { - for await failure in source.fetchFailures { - await failures.append(failure.message) - } - } + var failures = source.fetchFailures.makeAsyncIterator() await source.setViewport(Self.seattleBox) try await service.waitForCalls(1) // the first fetch is now in flight and parked @@ -288,9 +285,14 @@ struct VehicleRentalSourceTests { #expect(snapshot.added.map(\.id) == ["fresh"]) #expect(await service.calls.count == 2) - try await Task.sleep(for: .milliseconds(50)) - #expect(await failures.values.isEmpty) - failureWatcher.cancel() + // Waiting on a real failure is what proves the cancellation stayed silent, and + // it beats sleeping and asserting the stream is still empty: that only says + // nothing arrived *yet*. A third fetch fails for real, and the first failure to + // come out of the stream has to be that one — had the superseded fetch reported + // itself, it would already be buffered ahead of this and surface here instead. + await source.setViewport(Self.pannedBox(0.02)) + let failure = try #require(await failures.next()) + #expect(failure.underlyingError is ScriptedError) } @Test("A nil viewport clears everything immediately") @@ -455,11 +457,4 @@ struct VehicleRentalSourceTests { let snapshot = try #require(await snapshots.next()) #expect(snapshot.added.map(\.id) == ["a"]) } - - // MARK: - Helpers - - private actor Box { - private(set) var values: [String] = [] - func append(_ value: String) { values.append(value) } - } } From 066f42fd21f60beb5d48bdda1743a31c44a22ff5 Mon Sep 17 00:00:00 2001 From: Aaron Brethorst Date: Fri, 21 Aug 2026 22:10:29 -0700 Subject: [PATCH 5/5] Don't render a close button that has nowhere to go MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `onClose` became optional so `.embedded` hosts could leave it nil, but the `.standalone` toolbar item still rendered unconditionally. A host calling `createTripPlannerView()` with everything defaulted — which now compiles, and didn't before — got a close button that called `resetTripPlanner()` and then `onClose?()`, wiping the rider's planned trip without dismissing anything. Gate the item on having a handler, so the button exists only when tapping it can actually close the planner. --- .../Presentation/TripPlanner/TripPlannerView.swift | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/OTPKit/Sources/OTPKit/Presentation/TripPlanner/TripPlannerView.swift b/OTPKit/Sources/OTPKit/Presentation/TripPlanner/TripPlannerView.swift index e50eadc..0a9d4b8 100644 --- a/OTPKit/Sources/OTPKit/Presentation/TripPlanner/TripPlannerView.swift +++ b/OTPKit/Sources/OTPKit/Presentation/TripPlanner/TripPlannerView.swift @@ -134,10 +134,15 @@ public struct TripPlannerView: View { .navigationTitle(OTPLoc("trip_planner.title", comment: "Title of the trip planning screen")) .toolbarTitleDisplayMode(.inlineLarge) .toolbar { - ToolbarItem(placement: .topBarTrailing) { - Button(OTPLoc("common.close", comment: "Close button"), systemImage: "xmark") { - tripPlannerVM.resetTripPlanner() - self.onClose?() + // Only when there is somewhere to go. `onClose` is optional for + // `.embedded`, and a close button that resets the trip and then + // does nothing would leave the rider staring at a blank planner. + if let onClose { + ToolbarItem(placement: .topBarTrailing) { + Button(OTPLoc("common.close", comment: "Close button"), systemImage: "xmark") { + tripPlannerVM.resetTripPlanner() + onClose() + } } } }