From 6fff12390b3a68a2dfc87e0701f7c841b84a235e Mon Sep 17 00:00:00 2001 From: Kyle Van Essen Date: Sun, 9 Aug 2026 20:37:31 -0700 Subject: [PATCH 1/3] feat(SnapshotKitTesting): add deterministic measurement readiness --- .github/workflows/ci.yml | 4 + Shared/SnapshotKit/AGENTS.md | 6 +- Shared/SnapshotKit/README.md | 6 +- Shared/SnapshotKit/Sources/SnapshotCase.swift | 8 + .../SnapshotKit/Tests/SnapshotCaseTests.swift | 16 ++ Shared/SnapshotKitTesting/AGENTS.md | 15 +- Shared/SnapshotKitTesting/README.md | 14 +- .../Sources/AssertSnapshots.swift | 19 +- .../Sources/SnapshotCaptureTiming.swift | 9 +- .../Sources/SnapshotImageRendering.swift | 165 +++++++++++++----- .../Sources/SnapshotMeasurementHook.swift | 49 ++++++ .../Sources/SnapshotRenderingSupport.swift | 10 +- .../Sources/SnapshotSettleTimeoutPolicy.swift | 43 +++++ .../Tests/PreMeasureHookTests.swift | 68 ++++++++ .../Tests/SnapshotCaptureTimingTests.swift | 4 +- .../Tests/SnapshotMeasurementHookTests.swift | 28 +++ .../SnapshotSettleTimeoutPolicyTests.swift | 28 +++ test | 2 + 18 files changed, 430 insertions(+), 64 deletions(-) create mode 100644 Shared/SnapshotKitTesting/Sources/SnapshotMeasurementHook.swift create mode 100644 Shared/SnapshotKitTesting/Sources/SnapshotSettleTimeoutPolicy.swift create mode 100644 Shared/SnapshotKitTesting/Tests/PreMeasureHookTests.swift create mode 100644 Shared/SnapshotKitTesting/Tests/SnapshotMeasurementHookTests.swift create mode 100644 Shared/SnapshotKitTesting/Tests/SnapshotSettleTimeoutPolicyTests.swift diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ee8d00aab..c7d629b5d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -140,6 +140,10 @@ jobs: timeout-minutes: 60 env: TEST_WORKDIR: ${{ github.workspace }}/test-output-${{ matrix.shard }} + # Hosted snapshot rendering is substantially slower on these shared + # runners. Scale only maximum settle/hook ceilings; successful captures + # still use the same floor and pixel-stability proof as local runs. + SNAPSHOT_SETTLE_TIMEOUT_MULTIPLIER: "2" steps: # Reference images live in Git LFS (see .gitattributes), so fetch them — # without this the images arrive as pointer files and every comparison fails. diff --git a/Shared/SnapshotKit/AGENTS.md b/Shared/SnapshotKit/AGENTS.md index deaa2889d..93a54cd84 100644 --- a/Shared/SnapshotKit/AGENTS.md +++ b/Shared/SnapshotKit/AGENTS.md @@ -57,8 +57,10 @@ Complements the root [`AGENTS.md`](../../AGENTS.md) — read that first. subjects. - **Measurement readiness never replaces capture readiness.** Keep `measurementReadiness` at `.sameAsCapture` when async work can change ideal - height. Use `.immediate` only for synchronously sized fixtures; the case's - final `settle` remains independently load-bearing. Guards: + height, or await that work deterministically from `onReadyToMeasure` and use + `.settled` for the probe. Use `.immediate` only for synchronously sized + fixtures; the case's final `settle` remains independently load-bearing. A + measurement hook is intrinsic/full-content-only. Guards: `AsyncContentCaptureTests` and `LargeViewCaptureTests`. ## Testing diff --git a/Shared/SnapshotKit/README.md b/Shared/SnapshotKit/README.md index a223f71b9..569711495 100644 --- a/Shared/SnapshotKit/README.md +++ b/Shared/SnapshotKit/README.md @@ -61,6 +61,10 @@ capture + comparison pipeline lives in the sibling `.immediate` to skip the sizing probe's settle while retaining the final capture's `.settled` or `.settledAtLeast` policy; `.settled` decouples ordinary sizing quiescence from a raised final-capture floor. + When async content changes ideal height, `onReadyToMeasure` can instead await + a deterministic completion signal after the intrinsic probe is hosted and + laid out but before it settles and measures. The hook is invalid for fixed + sizing and is bounded by the capture's effective settle ceiling. An optional `onReadyToSnapshot` hook runs in the capture pipeline after the content has settled and just before the image is taken — the deterministic point to focus a field or trigger a presented state; its effects are settled @@ -125,7 +129,7 @@ assertSnapshots(of: MyBadge.self) preview cutsheet** — VoiceOver-annotated captures need the test-only library and can't render in a plain Preview. They still run as snapshot tests. The cutsheet also cannot reproduce the capture pipeline's UIKit-backed - `List`/`Form` height measurement, safe-area override, ready hook, or + `List`/`Form` height measurement, safe-area override, ready hooks, or tile-and-stitch pass, so CI's rendered dimensions remain authoritative. - The Where app wraps content in its Broadway design-system root via a `whereSnapshot(...)` adapter in `WhereUI`; SnapshotKit itself stays diff --git a/Shared/SnapshotKit/Sources/SnapshotCase.swift b/Shared/SnapshotKit/Sources/SnapshotCase.swift index f05a4e890..b951fd9f4 100644 --- a/Shared/SnapshotKit/Sources/SnapshotCase.swift +++ b/Shared/SnapshotKit/Sources/SnapshotCase.swift @@ -51,6 +51,12 @@ public struct SnapshotCase: Identifiable { public let settle: SnapshotSettle /// When intrinsic/full-content sizing may measure the content. public let measurementReadiness: SnapshotMeasurementReadiness + /// Runs after intrinsic/full-content content is hosted and laid out, but + /// before it settles and is measured. Use it to await a deterministic + /// content-ready signal when the loaded state changes ideal height. `nil` + /// for synchronously measurable content; fixed-size configurations reject + /// a hook because they do not have a measurement phase. + public let onReadyToMeasure: (@MainActor () async -> Void)? /// Runs in the capture pipeline after the content has settled and before /// the image is taken — the deterministic point to focus a field or trigger /// a presented state. Its effects are settled again before capture. `nil` @@ -77,6 +83,7 @@ public struct SnapshotCase: Identifiable { name: String, configurations: [SnapshotConfiguration], measurementReadiness: SnapshotMeasurementReadiness = .sameAsCapture, + onReadyToMeasure: (@MainActor () async -> Void)? = nil, settle: SnapshotSettle = .settled, onReadyToSnapshot: (@MainActor () async -> Void)? = nil, @ViewBuilder content: @escaping @MainActor () -> some View, @@ -84,6 +91,7 @@ public struct SnapshotCase: Identifiable { self.name = name self.configurations = configurations self.measurementReadiness = measurementReadiness + self.onReadyToMeasure = onReadyToMeasure self.settle = settle self.onReadyToSnapshot = onReadyToSnapshot contentFactory = { AnyView(content()) } diff --git a/Shared/SnapshotKit/Tests/SnapshotCaseTests.swift b/Shared/SnapshotKit/Tests/SnapshotCaseTests.swift index b487308f8..14b43414b 100644 --- a/Shared/SnapshotKit/Tests/SnapshotCaseTests.swift +++ b/Shared/SnapshotKit/Tests/SnapshotCaseTests.swift @@ -28,6 +28,22 @@ struct SnapshotCaseTests { #expect(snapshotCase.measurementReadiness == .immediate) } + @Test func onReadyToMeasureDefaultsToNil() { + let snapshotCase = SnapshotCase(name: "States", configurations: []) { Color.red } + #expect(snapshotCase.onReadyToMeasure == nil) + } + + @Test func onReadyToMeasureStoresTheDeclaredHook() async { + var hookRan = false + let snapshotCase = SnapshotCase( + name: "States", + configurations: [], + onReadyToMeasure: { hookRan = true }, + ) { Color.red } + await snapshotCase.onReadyToMeasure?() + #expect(hookRan) + } + @Test func settleStoresTheDeclaredMode() { let snapshotCase = SnapshotCase(name: "States", configurations: [], settle: .immediate) { Color.red diff --git a/Shared/SnapshotKitTesting/AGENTS.md b/Shared/SnapshotKitTesting/AGENTS.md index 1d29cf84a..29022fbc3 100644 --- a/Shared/SnapshotKitTesting/AGENTS.md +++ b/Shared/SnapshotKitTesting/AGENTS.md @@ -75,10 +75,12 @@ Complements the root [`AGENTS.md`](../../AGENTS.md) — read that first. - **The runner fails fast, once, on setup problems** (a simulator that doesn't match the `SNAPSHOT_EXPECTED_*` pins, two variants sharing one reference name) — one clear issue, never hundreds of pixel diffs. -- **An unsettled capture is a failure, not a silent fallback.** Don't "fix" a - settle timeout by widening the budget — freeze the motion behind - `\.isCapturingSnapshot`, or use `.settledAtLeast` only for genuinely slow - (not endless) content. +- **An unsettled capture is a failure, not a silent fallback.** Freeze endless + motion behind `\.isCapturingSnapshot`, and use deterministic readiness or + `.settledAtLeast` for finite work. The explicit CI multiplier may scale only + maximum settle/hook ceilings; never the floor, quiet proof, cadence, or image + tolerance. Guards: `SnapshotSettleTimeoutPolicyTests` and + `SnapshotRenderingSupportTests`. - **A settled capture is not a ready capture.** The loop proves the pixels stopped changing, not that the content the case meant to show ever arrived — a loading placeholder is perfectly pixel-stable, so a gap between phases of @@ -90,6 +92,11 @@ Complements the root [`AGENTS.md`](../../AGENTS.md) — read that first. completion signal from `onReadyToSnapshot` (`root.LoggedIn`). Both incidents, and how each was found, are ledgered in [`Where/TODOs.md`](../../Where/TODOs.md). +- **Height-changing readiness runs before measurement.** Intrinsic/full-content + cases await `onReadyToMeasure` only after the sizing probe is hosted and laid + out, then settle and resolve height. The hook is bounded by the effective + settle ceiling, must cooperate with cancellation, and is rejected for fixed + sizing. Guards: `PreMeasureHookTests` and `SnapshotMeasurementHookTests`. - **`.timedOut` requires observed motion; starvation is `.starved`.** A change-free settle loop keeps running until it can prove stability (a starved machine can fit fewer passes than stability needs), and only a hard diff --git a/Shared/SnapshotKitTesting/README.md b/Shared/SnapshotKitTesting/README.md index 41a5a8ecc..25514c2b0 100644 --- a/Shared/SnapshotKitTesting/README.md +++ b/Shared/SnapshotKitTesting/README.md @@ -53,7 +53,11 @@ re-exports `SnapshotKit` and `SnapshotTesting`, so a test author needs a single when their fixture's height is synchronous: only the sizing probe skips its settle, while the final capture still pays the case's declared `settle` and can observe async visual changes. Keep the default `.sameAsCapture` when an - async load can change ideal height. A case's + async load can change ideal height. Such a case may instead provide + `onReadyToMeasure`, which runs while the intrinsic probe is hosted and laid + out, before its settle and size resolution. The hook must cooperate with + cancellation, is bounded by the effective settle ceiling, and is rejected + for fixed sizing. A case's optional `onReadyToSnapshot` hook runs after that settle and before the accessibility parse / capture — the deterministic point to focus a field or trigger a presented state — and its effects are settled again before the @@ -135,7 +139,7 @@ default plain output. `./test --snapshots --timings` sets `SNAPSHOT_TIMING` and prints a per-phase breakdown — `settle`, `tileStitch`, `compare`, `pngRoundTrip`, `host`, -`accessibilityParse`, `hook`, `intrinsicMeasure`, `drain` — plus the settle pass +`accessibilityParse`, `hook`, `measurementHook`, `intrinsicMeasure`, `drain` — plus the settle pass distribution, sizing/readiness/capture-settle metadata, and the slowest individual captures. Reach for it before optimizing anything here: it is what showed that `drainInFlightAnimations` was burning a @@ -145,6 +149,12 @@ passes is what the remaining time buys. `SNAPSHOT_SETTLE` selects the stability mechanism (`pixel`, `quiescence`, `both`); see [`AGENTS.md`](AGENTS.md) for why `pixel` is the only safe default. +`SNAPSHOT_SETTLE_TIMEOUT_MULTIPLIER` scales the maximum observed-motion and +readiness-hook ceilings from 1× through 4×. It does not change minimum floors, +the quiet-window proof, render cadence, or image tolerances, so stable captures +finish at the same point. Local runs leave it unset (1×); snapshot CI explicitly +uses 2×. `./test` forwards it into the hosted test process. + ## Requirements - Runs in a hosted test bundle (needs a host app window; in this repo that's diff --git a/Shared/SnapshotKitTesting/Sources/AssertSnapshots.swift b/Shared/SnapshotKitTesting/Sources/AssertSnapshots.swift index f1d8722dc..ca381458c 100644 --- a/Shared/SnapshotKitTesting/Sources/AssertSnapshots.swift +++ b/Shared/SnapshotKitTesting/Sources/AssertSnapshots.swift @@ -14,7 +14,7 @@ import UIKit /// /// `async` because the render pipeline must suspend for SwiftUI `.task`-driven /// content to load before capture — see -/// ``renderSnapshotImage(of:named:sizing:safeAreaInsets:isAccessibility:settle:onReadyToSnapshot:)``. +/// ``renderSnapshotImage(of:named:sizing:safeAreaInsets:isAccessibility:measurementReadiness:onReadyToMeasure:settle:onReadyToSnapshot:)``. @MainActor public func assertSnapshots( of provider: (some SnapshotProviding).Type, @@ -26,6 +26,12 @@ public func assertSnapshots( column: UInt = #column, ) async { guard simulatorMatchesSnapshotExpectations() else { return } + do { + _ = try SnapshotSettleTimeoutPolicy.fromEnvironment() + } catch { + Issue.record(error) + return + } let snapshots = provider.snapshots let duplicates = duplicateSnapshotIdentifiers(in: snapshots) guard duplicates.isEmpty else { @@ -46,6 +52,7 @@ public func assertSnapshots( named: snapshotCase.name, configurations: snapshotCase.configurations, measurementReadiness: snapshotCase.measurementReadiness, + onReadyToMeasure: snapshotCase.onReadyToMeasure, settle: snapshotCase.settle, onReadyToSnapshot: snapshotCase.onReadyToSnapshot, record: record, @@ -66,6 +73,7 @@ public func assertSnapshots( named name: String, configurations: [SnapshotConfiguration], measurementReadiness: SnapshotMeasurementReadiness = .sameAsCapture, + onReadyToMeasure: (@MainActor () async -> Void)? = nil, settle: SnapshotSettle = .settled, onReadyToSnapshot: (@MainActor () async -> Void)? = nil, record: SnapshotTestingConfiguration.Record? = nil, @@ -99,6 +107,13 @@ public func assertSnapshots( // default plain output. let resolvedRecord = record ?? environmentRecordMode() let resolvedDiffTool = environmentDiffTool() + let settleTimeoutPolicy: SnapshotSettleTimeoutPolicy + do { + settleTimeoutPolicy = try .fromEnvironment() + } catch { + Issue.record(error) + return + } // Read once per call rather than per configuration: the environment can't // change mid-run, and the pixel walk is the cost worth gating, not this. let isDiffReportingEnabled = SnapshotDiffReporting.isEnabledByEnvironment @@ -140,8 +155,10 @@ public func assertSnapshots( safeAreaInsets: configuration.device.safeAreaInsets.uiEdgeInsets, isAccessibility: configuration.snapshotType == .accessibility, measurementReadiness: measurementReadiness, + onReadyToMeasure: onReadyToMeasure, settle: settle, onReadyToSnapshot: onReadyToSnapshot, + settleTimeoutPolicy: settleTimeoutPolicy, timing: timing, ) } catch { diff --git a/Shared/SnapshotKitTesting/Sources/SnapshotCaptureTiming.swift b/Shared/SnapshotKitTesting/Sources/SnapshotCaptureTiming.swift index a320a492e..8f992a7a0 100644 --- a/Shared/SnapshotKitTesting/Sources/SnapshotCaptureTiming.swift +++ b/Shared/SnapshotKitTesting/Sources/SnapshotCaptureTiming.swift @@ -5,14 +5,15 @@ import SnapshotKit /// /// The cases are the pipeline's own steps in the order they run, so a timing /// line reads as a walk through `renderSnapshotImage`. `settle`-shaped work -/// appears under three separate keys rather than one: the intrinsic-sizing -/// probe runs its own settle before the capture's, and a case's -/// `onReadyToSnapshot` hook is followed by a second one, so folding all three -/// together would hide which of them a slow case is actually paying for. +/// appears under separate keys rather than one: the intrinsic-sizing probe, +/// its deterministic readiness hook, the final settle, and a case's +/// `onReadyToSnapshot` hook each answer different performance questions. @_spi(Testing) public enum SnapshotCapturePhase: String, Sendable, CaseIterable { /// Hosting and settling the throwaway probe that measures `.intrinsic` / /// `.fullContent` content. Zero for `.fixed` sizing, which is most captures. case intrinsicMeasure + /// A deterministic readiness hook run while the intrinsic probe is hosted. + case measurementHook /// Attaching the capture wrapper to the host root and laying it out — the /// real UIKit appearance transition, so SwiftUI `onAppear` / `.task` start. case host diff --git a/Shared/SnapshotKitTesting/Sources/SnapshotImageRendering.swift b/Shared/SnapshotKitTesting/Sources/SnapshotImageRendering.swift index ee2042a0a..f4df11f09 100644 --- a/Shared/SnapshotKitTesting/Sources/SnapshotImageRendering.swift +++ b/Shared/SnapshotKitTesting/Sources/SnapshotImageRendering.swift @@ -32,6 +32,15 @@ public enum SnapshotSizing: Sendable { /// A capture failure that callers can report without comparing or recording an /// invalid image. public enum SnapshotRenderingError: Error, Equatable, Sendable { + /// The CI-only settle multiplier was malformed or outside its supported + /// safety range. + case invalidSettleTimeoutMultiplier(value: String) + /// A pre-measure hook was supplied for a fixed-size capture, where there is + /// no intrinsic measurement host on which to run it. + case measurementHookRequiresIntrinsicSizing(name: String) + /// Deterministic content readiness did not arrive before the capture's + /// effective settle ceiling. + case measurementReadinessTimedOut(name: String, budget: TimeInterval) /// Full-content measurement did not reach a stable height within the /// bounded fixed-point pass budget. case intrinsicHeightDidNotConverge(name: String, measuredHeights: [CGFloat]) @@ -40,6 +49,12 @@ public enum SnapshotRenderingError: Error, Equatable, Sendable { extension SnapshotRenderingError: LocalizedError { public var errorDescription: String? { switch self { + case let .invalidSettleTimeoutMultiplier(value): + return "Invalid SNAPSHOT_SETTLE_TIMEOUT_MULTIPLIER \"\(value)\"; use a finite value from 1 through 4." + case let .measurementHookRequiresIntrinsicSizing(name): + return "Snapshot \(name) declares onReadyToMeasure, but fixed sizing has no intrinsic measurement phase." + case let .measurementReadinessTimedOut(name, budget): + return "Snapshot \(name) measurement readiness did not complete within \(budget.formatted())s." case let .intrinsicHeightDidNotConverge(name, measuredHeights): let heights = measuredHeights.map { String(format: "%.1f", $0) } .joined(separator: ", ") @@ -71,6 +86,11 @@ extension SnapshotRenderingError: LocalizedError { /// hook runs, so a hook must not change the content's ideal size. /// `measurementReadiness` controls only the settle before intrinsic sizing; it /// never shortens the final capture settle. +/// `onReadyToMeasure` runs earlier, after an intrinsic probe is hosted and laid +/// out but before that settle and size resolution. It is for deterministic +/// readiness signals whose completion can change ideal height. The hook is +/// invalid for `.fixed` sizing, is bounded by the capture's effective settle +/// ceiling, and must cooperate with task cancellation. /// /// `async` is load-bearing, not a convenience: the settle phase must *suspend* /// (freeing the main actor) for SwiftUI `.task`-driven content to load — see @@ -105,18 +125,22 @@ public func renderSnapshotImage( safeAreaInsets: UIEdgeInsets? = .zero, isAccessibility: Bool = false, measurementReadiness: SnapshotMeasurementReadiness = .sameAsCapture, + onReadyToMeasure: (@MainActor () async -> Void)? = nil, settle: SnapshotSettle = .settled, onReadyToSnapshot: (@MainActor () async -> Void)? = nil, ) async throws -> UIImage { - try await renderSnapshotCapture( + let settleTimeoutPolicy = try SnapshotSettleTimeoutPolicy.fromEnvironment() + return try await renderSnapshotCapture( of: viewController, named: name, sizing: sizing, safeAreaInsets: safeAreaInsets, isAccessibility: isAccessibility, measurementReadiness: measurementReadiness, + onReadyToMeasure: onReadyToMeasure, settle: settle, onReadyToSnapshot: onReadyToSnapshot, + settleTimeoutPolicy: settleTimeoutPolicy, timing: SnapshotCaptureTiming(identifier: name, isEnabled: false), ).image } @@ -133,7 +157,7 @@ public func renderSnapshotImage( public let pngData: Data } -/// ``renderSnapshotImage(of:named:sizing:safeAreaInsets:isAccessibility:settle:onReadyToSnapshot:)`` +/// ``renderSnapshotImage(of:named:sizing:safeAreaInsets:isAccessibility:measurementReadiness:onReadyToMeasure:settle:onReadyToSnapshot:)`` /// with a caller-supplied phase recorder, so `assertSnapshots` can attribute the /// capture *and* the comparison that follows it to one line of output, and can /// compare the captured bytes against the reference without re-encoding. @@ -145,8 +169,10 @@ public func renderSnapshotImage( safeAreaInsets: UIEdgeInsets?, isAccessibility: Bool, measurementReadiness: SnapshotMeasurementReadiness, + onReadyToMeasure: (@MainActor () async -> Void)?, settle: SnapshotSettle, onReadyToSnapshot: (@MainActor () async -> Void)?, + settleTimeoutPolicy: SnapshotSettleTimeoutPolicy, timing: SnapshotCaptureTiming, ) async throws -> SnapshotCapture { try await SnapshotCaptureLock.withLock { @@ -157,15 +183,17 @@ public func renderSnapshotImage( safeAreaInsets: safeAreaInsets, isAccessibility: isAccessibility, measurementReadiness: measurementReadiness, + onReadyToMeasure: onReadyToMeasure, settle: settle, onReadyToSnapshot: onReadyToSnapshot, + settleTimeoutPolicy: settleTimeoutPolicy, timing: timing, ) } } /// The capture body of -/// ``renderSnapshotImage(of:named:sizing:safeAreaInsets:isAccessibility:settle:onReadyToSnapshot:)``, +/// ``renderSnapshotImage(of:named:sizing:safeAreaInsets:isAccessibility:measurementReadiness:onReadyToMeasure:settle:onReadyToSnapshot:)``, /// run while holding ``SnapshotCaptureLock``. @MainActor private func renderSnapshotImageLocked( @@ -175,8 +203,10 @@ private func renderSnapshotImageLocked( safeAreaInsets: UIEdgeInsets?, isAccessibility: Bool, measurementReadiness: SnapshotMeasurementReadiness, + onReadyToMeasure: (@MainActor () async -> Void)?, settle: SnapshotSettle, onReadyToSnapshot: (@MainActor () async -> Void)?, + settleTimeoutPolicy: SnapshotSettleTimeoutPolicy, timing: SnapshotCaptureTiming, ) async throws -> SnapshotCapture { func capture() async throws -> SnapshotCapture { @@ -205,17 +235,18 @@ private func renderSnapshotImageLocked( // surfaces it to SwiftUI as `\.isCapturingSnapshot`. viewController.traitOverrides[SnapshotCaptureTrait.self] = true - try await timing.measure(.intrinsicMeasure) { - try await resolveContentSize( - of: viewController, - named: name, - sizing: sizing, - settle: measurementReadiness.resolvedSettle(captureSettle: settle), - hostedIn: hostRoot, - window: window, - timing: timing, - ) - } + try await resolveContentSize( + of: viewController, + named: name, + sizing: sizing, + settle: measurementReadiness.resolvedSettle(captureSettle: settle), + onReadyToMeasure: onReadyToMeasure, + measurementHookMaximumDuration: settleTimeoutPolicy.maximumDuration(for: settle), + hostedIn: hostRoot, + window: window, + timing: timing, + settleTimeoutPolicy: settleTimeoutPolicy, + ) let captureViewController: UIViewController = isAccessibility ? AccessibilitySnapshotViewController(wrapping: viewController) @@ -241,6 +272,7 @@ private func renderSnapshotImageLocked( named: name, settle: settle, timing: timing, + timeoutPolicy: settleTimeoutPolicy, ) }, phase: "content", @@ -263,6 +295,7 @@ private func renderSnapshotImageLocked( named: name, settle: settle, timing: timing, + timeoutPolicy: settleTimeoutPolicy, ) }, phase: "onReadyToSnapshot", @@ -374,21 +407,63 @@ private func resolveContentSize( named name: String, sizing: SnapshotSizing, settle: SnapshotSettle, + onReadyToMeasure: (@MainActor () async -> Void)?, + measurementHookMaximumDuration: TimeInterval, hostedIn hostRoot: UIViewController, window: UIWindow, timing: SnapshotCaptureTiming, + settleTimeoutPolicy: SnapshotSettleTimeoutPolicy, ) async throws { - guard case let .intrinsic(width, minimumHeight) = sizing else { return } + guard case let .intrinsic(width, minimumHeight) = sizing else { + if onReadyToMeasure != nil { + throw SnapshotRenderingError.measurementHookRequiresIntrinsicSizing(name: name) + } + return + } + + let probeWrapper = timing.measure(.intrinsicMeasure) { + let probeHeight = max(window.bounds.height, 1) + viewController.view.frame = CGRect(x: 0, y: 0, width: width, height: probeHeight) + + let wrapper = SnapshotWrappingViewController(viewController) + hostChildForCapture(wrapper, in: hostRoot) + wrapper.view.setNeedsLayout() + CATransaction.performWithoutAnimation(wrapper.view.layoutIfNeeded) + return wrapper + } + defer { + // Detach the content VC from the probe wrapper first (so the caller can + // re-wrap it), then tear the probe wrapper down — including on a hook + // timeout or cancellation. + viewController.willMove(toParent: nil) + viewController.removeFromParent() + removeChildAfterCapture(probeWrapper) + } - let probeHeight = max(window.bounds.height, 1) - viewController.view.frame = CGRect(x: 0, y: 0, width: width, height: probeHeight) + if let onReadyToMeasure { + try await timing.measure(.measurementHook) { + try await runSnapshotMeasurementHook( + named: name, + maximumDuration: measurementHookMaximumDuration, + hook: onReadyToMeasure, + ) + } + timing.measure(.intrinsicMeasure) { + probeWrapper.view.setNeedsLayout() + CATransaction.performWithoutAnimation(probeWrapper.view.layoutIfNeeded) + } + } - let probeWrapper = SnapshotWrappingViewController(viewController) - hostChildForCapture(probeWrapper, in: hostRoot) - probeWrapper.view.setNeedsLayout() - CATransaction.performWithoutAnimation(probeWrapper.view.layoutIfNeeded) await reportIfUnsettled( - settleForCapture(probeWrapper.view, named: name, settle: settle, timing: timing), + timing.measure(.intrinsicMeasure) { + await settleForCapture( + probeWrapper.view, + named: name, + settle: settle, + timing: timing, + timeoutPolicy: settleTimeoutPolicy, + ) + }, phase: "intrinsic measurement", of: viewController, named: name, @@ -410,38 +485,36 @@ private func resolveContentSize( return measured } - var measured = measureContent() - var measuredHeights = [measured.height] - var didConverge = false - for _ in 0 ..< 10 { - viewController.view.frame = CGRect(origin: .zero, size: measured) - probeWrapper.view.frame.size = measured - CATransaction.performWithoutAnimation(probeWrapper.view.layoutIfNeeded) - let remeasured = measureContent() - measuredHeights.append(remeasured.height) - if abs(remeasured.height - measured.height) < 0.5 { + let measurement = timing.measure(.intrinsicMeasure) { + var measured = measureContent() + var measuredHeights = [measured.height] + var didConverge = false + for _ in 0 ..< 10 { + viewController.view.frame = CGRect(origin: .zero, size: measured) + probeWrapper.view.frame.size = measured + CATransaction.performWithoutAnimation(probeWrapper.view.layoutIfNeeded) + let remeasured = measureContent() + measuredHeights.append(remeasured.height) + if abs(remeasured.height - measured.height) < 0.5 { + measured = remeasured + didConverge = true + break + } measured = remeasured - didConverge = true - break } - measured = remeasured + return (measured, measuredHeights, didConverge) } - - // Detach the content VC from the probe wrapper first (so the caller can - // re-wrap it), then tear the probe wrapper down. - viewController.willMove(toParent: nil) - viewController.removeFromParent() - removeChildAfterCapture(probeWrapper) - - guard didConverge else { + guard measurement.2 else { throw SnapshotRenderingError.intrinsicHeightDidNotConverge( name: name, - measuredHeights: measuredHeights, + measuredHeights: measurement.1, ) } - viewController.view.frame = CGRect(origin: .zero, size: measured) - CATransaction.performWithoutAnimation(viewController.view.layoutIfNeeded) + timing.measure(.intrinsicMeasure) { + viewController.view.frame = CGRect(origin: .zero, size: measurement.0) + CATransaction.performWithoutAnimation(viewController.view.layoutIfNeeded) + } } extension UIView { diff --git a/Shared/SnapshotKitTesting/Sources/SnapshotMeasurementHook.swift b/Shared/SnapshotKitTesting/Sources/SnapshotMeasurementHook.swift new file mode 100644 index 000000000..862432d02 --- /dev/null +++ b/Shared/SnapshotKitTesting/Sources/SnapshotMeasurementHook.swift @@ -0,0 +1,49 @@ +import Foundation + +private enum SnapshotMeasurementHookResult { + case completed + case timedOut + case cancelled +} + +/// Runs a pre-measure hook with a bounded lifetime, cancelling the losing side +/// of the race. Hooks that suspend must cooperate with cancellation so a timed +/// out capture can finish tearing down its hosted probe. +@MainActor +@_spi(Testing) public func runSnapshotMeasurementHook( + named name: String, + maximumDuration: TimeInterval, + hook: @MainActor @escaping () async -> Void, +) async throws { + let result = await withTaskGroup(of: SnapshotMeasurementHookResult.self) { group in + group.addTask { + await hook() + return .completed + } + group.addTask { + do { + try await Task.sleep(for: .seconds(maximumDuration)) + return .timedOut + } catch { + return .cancelled + } + } + + let first = await group.next() ?? .cancelled + group.cancelAll() + return first + } + + try Task.checkCancellation() + switch result { + case .completed: + return + case .timedOut: + throw SnapshotRenderingError.measurementReadinessTimedOut( + name: name, + budget: maximumDuration, + ) + case .cancelled: + throw CancellationError() + } +} diff --git a/Shared/SnapshotKitTesting/Sources/SnapshotRenderingSupport.swift b/Shared/SnapshotKitTesting/Sources/SnapshotRenderingSupport.swift index 35a54f3c3..ae1b831df 100644 --- a/Shared/SnapshotKitTesting/Sources/SnapshotRenderingSupport.swift +++ b/Shared/SnapshotKitTesting/Sources/SnapshotRenderingSupport.swift @@ -84,10 +84,16 @@ func settleForCapture( named name: String, settle: SnapshotSettle, timing: SnapshotCaptureTiming, + timeoutPolicy: SnapshotSettleTimeoutPolicy, ) async -> SettleOutcome { switch settle { case .settled: - return await settleContent(view, named: name, timing: timing) + return await settleContent( + view, + named: name, + maxDuration: timeoutPolicy.maximumDuration(for: settle), + timing: timing, + ) case let .settledAtLeast(minDuration): // Keep the hang budget for never-quiescing content above the raised // floor, so the minimum is always honored. @@ -95,7 +101,7 @@ func settleForCapture( view, named: name, minDuration: minDuration, - maxDuration: max(2.5, minDuration + 2.5), + maxDuration: timeoutPolicy.maximumDuration(for: settle), timing: timing, ) case .immediate: diff --git a/Shared/SnapshotKitTesting/Sources/SnapshotSettleTimeoutPolicy.swift b/Shared/SnapshotKitTesting/Sources/SnapshotSettleTimeoutPolicy.swift new file mode 100644 index 000000000..957a5f3b0 --- /dev/null +++ b/Shared/SnapshotKitTesting/Sources/SnapshotSettleTimeoutPolicy.swift @@ -0,0 +1,43 @@ +import Foundation +import SnapshotKit + +/// The environment-selected patience applied to snapshot settle ceilings. +/// +/// The multiplier deliberately affects only maximum durations: it gives slow +/// runners longer to finish genuinely moving content without changing when a +/// stable capture succeeds. +@_spi(Testing) public struct SnapshotSettleTimeoutPolicy: Equatable, Sendable { + public static let environmentKey = "SNAPSHOT_SETTLE_TIMEOUT_MULTIPLIER" + + public let multiplier: Double + + public static func fromEnvironment( + _ environment: [String: String] = ProcessInfo.processInfo.environment, + ) throws -> Self { + try parse(environment[environmentKey]) + } + + /// Parses the environment value without mutating process-global state. + public static func parse(_ value: String?) throws -> Self { + guard let value else { + return Self(multiplier: 1) + } + guard let multiplier = Double(value), + multiplier.isFinite, + (1 ... 4).contains(multiplier) + else { + throw SnapshotRenderingError.invalidSettleTimeoutMultiplier(value: value) + } + return Self(multiplier: multiplier) + } + + public func maximumDuration(for settle: SnapshotSettle) -> TimeInterval { + let baseDuration = switch settle { + case .settled, .immediate: + 2.5 + case let .settledAtLeast(minDuration): + max(2.5, minDuration + 2.5) + } + return baseDuration * multiplier + } +} diff --git a/Shared/SnapshotKitTesting/Tests/PreMeasureHookTests.swift b/Shared/SnapshotKitTesting/Tests/PreMeasureHookTests.swift new file mode 100644 index 000000000..47a051731 --- /dev/null +++ b/Shared/SnapshotKitTesting/Tests/PreMeasureHookTests.swift @@ -0,0 +1,68 @@ +import Observation +@_spi(Testing) import SnapshotKitTesting +import SwiftUI +import TestHostSupport +import Testing +import UIKit + +/// Regression guards for `onReadyToMeasure`: the hook runs only while the +/// intrinsic probe is hosted, and its completion precedes size resolution. +@MainActor +struct PreMeasureHookTests { + @Test func hostedHookCanReleaseHeightChangingAsyncContentBeforeMeasurement() async throws { + try waitFor { hostKeyWindow() != nil } + let model = PreMeasureProbeModel() + let host = UIHostingController(rootView: PreMeasureProbeView(model: model)) + host.view.frame = CGRect(x: 0, y: 0, width: 100, height: 1) + + let image = try await renderSnapshotImage( + of: host, + named: "pre-measure-hook-probe", + sizing: .intrinsic(width: 100, minimumHeight: 0), + safeAreaInsets: .zero, + measurementReadiness: .immediate, + onReadyToMeasure: { + while model.hostedTaskRan == false { + await Task.yield() + } + }, + settle: .immediate, + ) + + #expect(model.hostedTaskRan) + #expect(image.size.height == 180) + } + + @Test func fixedCaptureRejectsAMeasurementHook() async throws { + try waitFor { hostKeyWindow() != nil } + let host = UIHostingController(rootView: Color.green) + host.view.frame = CGRect(x: 0, y: 0, width: 100, height: 100) + + await #expect( + throws: SnapshotRenderingError.measurementHookRequiresIntrinsicSizing( + name: "fixed-pre-measure-hook", + ), + ) { + try await renderSnapshotImage( + of: host, + named: "fixed-pre-measure-hook", + onReadyToMeasure: {}, + ) + } + } +} + +@Observable +private final class PreMeasureProbeModel { + var hostedTaskRan = false +} + +private struct PreMeasureProbeView: View { + let model: PreMeasureProbeModel + + var body: some View { + Color.green + .frame(width: 100, height: model.hostedTaskRan ? 180 : 40) + .task { model.hostedTaskRan = true } + } +} diff --git a/Shared/SnapshotKitTesting/Tests/SnapshotCaptureTimingTests.swift b/Shared/SnapshotKitTesting/Tests/SnapshotCaptureTimingTests.swift index 9c5f8a28c..105328556 100644 --- a/Shared/SnapshotKitTesting/Tests/SnapshotCaptureTimingTests.swift +++ b/Shared/SnapshotKitTesting/Tests/SnapshotCaptureTimingTests.swift @@ -45,10 +45,10 @@ struct SnapshotCaptureTimingTests { @Test func asyncAndSyncPhasesBothRecord() async throws { let timing = SnapshotCaptureTiming(identifier: "mixed", isEnabled: true) timing.measure(.host) { spin(for: .milliseconds(10)) } - await timing.measure(.hook) { try? await Task.sleep(for: .milliseconds(20)) } + await timing.measure(.measurementHook) { try? await Task.sleep(for: .milliseconds(20)) } let line = try decodedLine(from: timing) - #expect(line.phases.keys.sorted() == ["hook", "host"]) + #expect(line.phases.keys.sorted() == ["host", "measurementHook"]) } @Test func settlePassesAccumulateAndCaptureShapeIsReported() throws { diff --git a/Shared/SnapshotKitTesting/Tests/SnapshotMeasurementHookTests.swift b/Shared/SnapshotKitTesting/Tests/SnapshotMeasurementHookTests.swift new file mode 100644 index 000000000..2b1d63330 --- /dev/null +++ b/Shared/SnapshotKitTesting/Tests/SnapshotMeasurementHookTests.swift @@ -0,0 +1,28 @@ +@_spi(Testing) import SnapshotKitTesting +import Testing + +@MainActor +struct SnapshotMeasurementHookTests { + @Test func completedHookReturnsBeforeItsBudget() async throws { + var didRun = false + + try await runSnapshotMeasurementHook(named: "complete", maximumDuration: 1) { + didRun = true + } + + #expect(didRun) + } + + @Test func timedOutHookThrowsTheCaptureError() async { + await #expect( + throws: SnapshotRenderingError.measurementReadinessTimedOut( + name: "timeout", + budget: 0.02, + ), + ) { + try await runSnapshotMeasurementHook(named: "timeout", maximumDuration: 0.02) { + try? await Task.sleep(for: .seconds(60)) + } + } + } +} diff --git a/Shared/SnapshotKitTesting/Tests/SnapshotSettleTimeoutPolicyTests.swift b/Shared/SnapshotKitTesting/Tests/SnapshotSettleTimeoutPolicyTests.swift new file mode 100644 index 000000000..9b98d7b6e --- /dev/null +++ b/Shared/SnapshotKitTesting/Tests/SnapshotSettleTimeoutPolicyTests.swift @@ -0,0 +1,28 @@ +import SnapshotKit +@_spi(Testing) import SnapshotKitTesting +import Testing + +struct SnapshotSettleTimeoutPolicyTests { + @Test func missingValueKeepsLocalTimeoutsUnscaled() throws { + let policy = try SnapshotSettleTimeoutPolicy.parse(nil) + + #expect(policy.multiplier == 1) + #expect(policy.maximumDuration(for: .settled) == 2.5) + #expect(policy.maximumDuration(for: .settledAtLeast(minDuration: 1.5)) == 4) + } + + @Test func validMultiplierScalesOnlyTheMaximumBudget() throws { + let policy = try SnapshotSettleTimeoutPolicy.parse("2") + + #expect(policy.multiplier == 2) + #expect(policy.maximumDuration(for: .settled) == 5) + #expect(policy.maximumDuration(for: .settledAtLeast(minDuration: 1.5)) == 8) + } + + @Test(arguments: ["", "0", "0.5", "5", "nan", "infinity", "slow"]) + func invalidMultiplierFailsSetup(value: String) { + #expect(throws: SnapshotRenderingError.invalidSettleTimeoutMultiplier(value: value)) { + try SnapshotSettleTimeoutPolicy.parse(value) + } + } +} diff --git a/test b/test index b7881fe47..e08e55ad5 100755 --- a/test +++ b/test @@ -697,6 +697,8 @@ RUN_ENV=() [ -n "$RECORD" ] && RUN_ENV+=("TEST_RUNNER_SNAPSHOT_RECORD=$RECORD") [ "$TIMINGS" = true ] && RUN_ENV+=("TEST_RUNNER_SNAPSHOT_TIMING=1") [ "$REVIEW" = true ] && RUN_ENV+=("TEST_RUNNER_SNAPSHOT_DIFF=1") +[ -n "${SNAPSHOT_SETTLE_TIMEOUT_MULTIPLIER:-}" ] \ + && RUN_ENV+=("TEST_RUNNER_SNAPSHOT_SETTLE_TIMEOUT_MULTIPLIER=$SNAPSHOT_SETTLE_TIMEOUT_MULTIPLIER") # SwiftPM's generated `Bundle.module` accessors honor # PACKAGE_RESOURCE_BUNDLE_PATH as their first lookup candidate (DEBUG-only, From 17184282477721cd7697aa822d8b86cf7a749270 Mon Sep 17 00:00:00 2001 From: Kyle Van Essen Date: Sun, 9 Aug 2026 20:37:51 -0700 Subject: [PATCH 2/3] fix(Flyover): await visible previews before measurement --- Shared/Flyover/AGENTS.md | 3 + Shared/Flyover/README.md | 4 + .../SnapshotTests/FlyoverSnapshotTests.swift | 31 ++++-- .../Flyover/Sources/FlyoverCanvasView.swift | 12 +++ Shared/Flyover/Sources/FlyoverModel.swift | 18 +++- .../Sources/FlyoverPreviewReadiness.swift | 101 ++++++++++++++++++ .../Sources/FlyoverScreenContent.swift | 42 ++++++-- Shared/Flyover/Sources/FlyoverView.swift | 5 + .../Tests/FlyoverPreviewReadinessTests.swift | 101 ++++++++++++++++++ 9 files changed, 298 insertions(+), 19 deletions(-) create mode 100644 Shared/Flyover/Sources/FlyoverPreviewReadiness.swift create mode 100644 Shared/Flyover/Tests/FlyoverPreviewReadinessTests.swift diff --git a/Shared/Flyover/AGENTS.md b/Shared/Flyover/AGENTS.md index 34665ee1e..9b580da7a 100644 --- a/Shared/Flyover/AGENTS.md +++ b/Shared/Flyover/AGENTS.md @@ -40,6 +40,9 @@ conventions. - Invoke variant builders through the serial deferred load coordinator, never synchronously from a SwiftUI `body`; preview fixtures may open expensive in-memory stores. +- Canvas preview readiness is the latest nonempty visible-load expectation; + variant/generation changes supersede stale completions, and cancelled waiters + must resume. `FlyoverSnapshotTests` awaits it before intrinsic measurement. - Global traits are session-only and apply to registered content, not Flyover chrome. - Register forward push/modal routes only. Flyover derives Back/Dismiss cues diff --git a/Shared/Flyover/README.md b/Shared/Flyover/README.md index e7f4faa2b..f95764856 100644 --- a/Shared/Flyover/README.md +++ b/Shared/Flyover/README.md @@ -119,6 +119,10 @@ single pinned preview until it is paused. Opening the focused inspector unloads the underlying canvas previews. Variant builders are deferred and serialized, with a render opportunity between builds, so expensive preview-model construction cannot accumulate in one SwiftUI update. +The canvas also tracks the latest nonempty set of visible variant/generation +loads. Snapshot capture awaits that production loading signal before intrinsic +measurement, so a slow runner cannot size or capture a partially loaded canvas; +viewport or variant changes supersede stale completions. The initial canvas zoom fits the graph to the available width so its cards are immediately legible and the remaining groups can be reached by vertical diff --git a/Shared/Flyover/SnapshotTests/FlyoverSnapshotTests.swift b/Shared/Flyover/SnapshotTests/FlyoverSnapshotTests.swift index 8f576f0a5..1f886517a 100644 --- a/Shared/Flyover/SnapshotTests/FlyoverSnapshotTests.swift +++ b/Shared/Flyover/SnapshotTests/FlyoverSnapshotTests.swift @@ -8,17 +8,26 @@ import Testing struct FlyoverSnapshotTests { @Test func canvasAndList() async { let catalog = Self.catalog() - await assertSnapshots( - of: FlyoverView(catalog: catalog), - named: "FlyoverCanvas", - configurations: SnapshotConfiguration.combinations( - devices: [.iPadFullContent], - colorSchemes: [.light, .dark], - ), - // Canvas previews load serially, so a cold CI host can still be resolving - // the visible screen trees after the default settling budget. - settle: .settledAtLeast(minDuration: 1.5), - ) + for configuration in SnapshotConfiguration.combinations( + devices: [.iPadFullContent], + colorSchemes: [.light, .dark], + ) { + // A fresh model gives each appearance an independent readiness + // expectation and set of serial preview loads. + let model = FlyoverModel(catalog: catalog) + await assertSnapshots( + of: FlyoverView(catalog: catalog, model: model), + named: "FlyoverCanvas", + configurations: [configuration], + measurementReadiness: .settled, + onReadyToMeasure: { + await model.waitUntilVisiblePreviewsAreLoaded() + }, + // The deterministic hook covers preview loading. Keep the floor + // for the genuinely time-based glass material adaptation. + settle: .settledAtLeast(minDuration: 1.5), + ) + } await assertSnapshots( of: FlyoverView(catalog: catalog), diff --git a/Shared/Flyover/Sources/FlyoverCanvasView.swift b/Shared/Flyover/Sources/FlyoverCanvasView.swift index 651f60958..b933af875 100644 --- a/Shared/Flyover/Sources/FlyoverCanvasView.swift +++ b/Shared/Flyover/Sources/FlyoverCanvasView.swift @@ -25,6 +25,15 @@ struct FlyoverCanvasView: View { } else { renderPlan.liveScreenIDs } + let expectedPreviewLoads: Set.LoadKey> = if model + .hasAppliedInitialCanvasZoom + { + Set(catalog.screens.compactMap { screen in + liveScreenIDs.contains(screen.id) ? model.previewLoadKey(for: screen) : nil + }) + } else { + [] + } GeometryReader { proxy in ScrollView([.horizontal, .vertical]) { @@ -81,6 +90,9 @@ struct FlyoverCanvasView: View { .task { applyInitialWidthFit(layout: layout, in: proxy.size) } + .task(id: expectedPreviewLoads) { + model.previewReadiness.expect(expectedPreviewLoads) + } } } diff --git a/Shared/Flyover/Sources/FlyoverModel.swift b/Shared/Flyover/Sources/FlyoverModel.swift index f600800b1..95832a08c 100644 --- a/Shared/Flyover/Sources/FlyoverModel.swift +++ b/Shared/Flyover/Sources/FlyoverModel.swift @@ -19,8 +19,9 @@ final class FlyoverModel { private(set) var previewedScreenID: ScreenID? let contentLoadCoordinator = FlyoverContentLoadCoordinator() + let previewReadiness = FlyoverPreviewReadiness() private let frameStates: [ScreenID: FlyoverFrameState] - private var hasAppliedInitialCanvasZoom = false + private(set) var hasAppliedInitialCanvasZoom = false init(catalog: FlyoverCatalog) { frameStates = catalog.screens.reduce(into: [:]) { states, screen in @@ -53,6 +54,21 @@ final class FlyoverModel { return screen.variants.first { $0.id == selectedID } ?? screen.variants[0] } + func previewLoadKey(for screen: FlyoverScreen) -> FlyoverPreviewReadiness + .LoadKey + { + let state = state(for: screen) + return FlyoverPreviewReadiness.LoadKey( + screenID: screen.id, + variantID: variant(for: screen).id, + generation: state.generation, + ) + } + + func waitUntilVisiblePreviewsAreLoaded() async { + await previewReadiness.waitUntilReady() + } + func focus(_ screen: FlyoverScreen) { focusedSelection = FlyoverSelection(id: screen.id) } diff --git a/Shared/Flyover/Sources/FlyoverPreviewReadiness.swift b/Shared/Flyover/Sources/FlyoverPreviewReadiness.swift new file mode 100644 index 000000000..818d2cf90 --- /dev/null +++ b/Shared/Flyover/Sources/FlyoverPreviewReadiness.swift @@ -0,0 +1,101 @@ +import Foundation + +/// Tracks the latest nonempty set of canvas previews whose real content must +/// finish loading before an intrinsic snapshot may measure Flyover. +@MainActor +final class FlyoverPreviewReadiness { + struct LoadKey: Hashable { + let screenID: ScreenID + let variantID: FlyoverVariantID + let generation: Int + } + + private(set) var expectedKeys: Set? + private(set) var expectationGeneration = 0 + private(set) var waiterCount = 0 + private var activeKeys: Set = [] + private var completedKeys: Set = [] + private var waiters: [UUID: CheckedContinuation] = [:] + + var isReadyForLatestExpectation: Bool { + guard let expectedKeys else { + return false + } + return expectedKeys.isSubset(of: completedKeys) + } + + /// Supersedes the previous viewport expectation. Empty sets are ignored so + /// an intrinsic capture cannot declare readiness before scroll geometry has + /// published its first visible region. + func expect(_ keys: Set) { + guard keys.isEmpty == false, keys != expectedKeys else { + return + } + expectedKeys = keys + expectationGeneration += 1 + completedKeys.formIntersection(keys) + resumeWaitersIfReady() + } + + func beganLoading(_ key: LoadKey) { + activeKeys.insert(key) + completedKeys.remove(key) + } + + func finishedLoading(_ key: LoadKey) { + guard activeKeys.contains(key) else { + return + } + completedKeys.insert(key) + resumeWaitersIfReady() + } + + func unloaded(_ key: LoadKey) { + activeKeys.remove(key) + completedKeys.remove(key) + } + + /// Waits for whichever nonempty expectation is current when readiness is + /// reached. If viewport or variant state changes while suspended, the new + /// expectation replaces the old one instead of allowing stale completions + /// to release the waiter. + func waitUntilReady() async { + guard isReadyForLatestExpectation == false else { + return + } + + let waiterID = UUID() + await withTaskCancellationHandler { + await withCheckedContinuation { continuation in + guard Task.isCancelled == false else { + continuation.resume() + return + } + waiters[waiterID] = continuation + waiterCount = waiters.count + resumeWaitersIfReady() + } + } onCancel: { + Task { @MainActor in + self.cancelWaiter(waiterID) + } + } + } + + private func resumeWaitersIfReady() { + guard isReadyForLatestExpectation else { + return + } + let readyWaiters = Array(waiters.values) + waiters.removeAll() + waiterCount = 0 + for waiter in readyWaiters { + waiter.resume() + } + } + + private func cancelWaiter(_ id: UUID) { + waiters.removeValue(forKey: id)?.resume() + waiterCount = waiters.count + } +} diff --git a/Shared/Flyover/Sources/FlyoverScreenContent.swift b/Shared/Flyover/Sources/FlyoverScreenContent.swift index f763bc128..501237692 100644 --- a/Shared/Flyover/Sources/FlyoverScreenContent.swift +++ b/Shared/Flyover/Sources/FlyoverScreenContent.swift @@ -6,7 +6,7 @@ struct FlyoverScreenContent: View { let screen: FlyoverScreen let model: FlyoverModel let isOverview: Bool - @State private var content: AnyView? + @State private var loadedContent: LoadedContent? @Environment(\.colorScheme) private var systemColorScheme @Environment(\.flyoverStylesheet) private var stylesheet @@ -20,16 +20,17 @@ struct FlyoverScreenContent: View { generation: state.generation, isOverview: isOverview, ) + let previewLoadKey = model.previewLoadKey(for: screen) Group { - if let content { + if let loadedContent, loadedContent.id == contentID { switch screen.navigationContainer { case .stack: NavigationStack { - content + loadedContent.content } case .none: - content + loadedContent.content } } else { ProgressView() @@ -56,7 +57,23 @@ struct FlyoverScreenContent: View { ) .allowsHitTesting(isOverview == false) .task(id: contentID) { - content = nil + if loadedContent?.id == contentID { + if isOverview { + model.previewReadiness.beganLoading(previewLoadKey) + model.previewReadiness.finishedLoading(previewLoadKey) + } + return + } + loadedContent = nil + if isOverview { + model.previewReadiness.beganLoading(previewLoadKey) + } + var didFinishLoading = false + defer { + if isOverview, didFinishLoading == false { + model.previewReadiness.unloaded(previewLoadKey) + } + } await model.contentLoadCoordinator.perform { guard Task.isCancelled == false else { return @@ -69,11 +86,17 @@ struct FlyoverScreenContent: View { guard Task.isCancelled == false else { return } - content = loadedContent + self.loadedContent = LoadedContent(id: contentID, content: loadedContent) + didFinishLoading = true + if isOverview { + model.previewReadiness.finishedLoading(previewLoadKey) + } } } .onDisappear { - content = nil + if isOverview { + model.previewReadiness.unloaded(previewLoadKey) + } } } @@ -90,4 +113,9 @@ struct FlyoverScreenContent: View { let generation: Int let isOverview: Bool } + + private struct LoadedContent { + let id: ContentID + let content: AnyView + } } diff --git a/Shared/Flyover/Sources/FlyoverView.swift b/Shared/Flyover/Sources/FlyoverView.swift index bfd4c34ad..28c587dac 100644 --- a/Shared/Flyover/Sources/FlyoverView.swift +++ b/Shared/Flyover/Sources/FlyoverView.swift @@ -11,6 +11,11 @@ public struct FlyoverView: View { _model = State(initialValue: FlyoverModel(catalog: catalog)) } + init(catalog: FlyoverCatalog, model: FlyoverModel) { + self.catalog = catalog + _model = State(initialValue: model) + } + public var body: some View { FlyoverRootView(catalog: catalog, model: model) .broadwayRoot() diff --git a/Shared/Flyover/Tests/FlyoverPreviewReadinessTests.swift b/Shared/Flyover/Tests/FlyoverPreviewReadinessTests.swift new file mode 100644 index 000000000..74397cf19 --- /dev/null +++ b/Shared/Flyover/Tests/FlyoverPreviewReadinessTests.swift @@ -0,0 +1,101 @@ +@testable import Flyover +import Testing + +@MainActor +struct FlyoverPreviewReadinessTests { + @Test func waitsForANonemptyLatestExpectationAndEveryExpectedLoad() async { + let readiness = FlyoverPreviewReadiness() + let first = key(.first) + let second = key(.second) + let waiter = Task { @MainActor in + await readiness.waitUntilReady() + } + await waitUntil { readiness.waiterCount == 1 } + + readiness.expect([]) + #expect(readiness.isReadyForLatestExpectation == false) + + readiness.expect([first, second]) + readiness.beganLoading(first) + readiness.finishedLoading(first) + #expect(readiness.isReadyForLatestExpectation == false) + + readiness.beganLoading(second) + readiness.finishedLoading(second) + await waiter.value + #expect(readiness.isReadyForLatestExpectation) + } + + @Test func changedExpectationSupersedesStaleCompletions() async { + let readiness = FlyoverPreviewReadiness() + let firstGeneration = key(.first, generation: 0) + let secondGeneration = key(.first, generation: 1) + readiness.expect([firstGeneration]) + readiness.beganLoading(firstGeneration) + + let waiter = Task { @MainActor in + await readiness.waitUntilReady() + } + await waitUntil { readiness.waiterCount == 1 } + + readiness.expect([secondGeneration]) + readiness.unloaded(firstGeneration) + readiness.finishedLoading(firstGeneration) + #expect(readiness.isReadyForLatestExpectation == false) + + readiness.beganLoading(secondGeneration) + readiness.finishedLoading(secondGeneration) + await waiter.value + #expect(readiness.isReadyForLatestExpectation) + #expect(readiness.expectationGeneration == 2) + } + + @Test func completionMayArriveBeforeItsFirstExpectationIsPublished() async { + let readiness = FlyoverPreviewReadiness() + let first = key(.first) + + readiness.beganLoading(first) + readiness.finishedLoading(first) + readiness.expect([first]) + + #expect(readiness.isReadyForLatestExpectation) + await readiness.waitUntilReady() + } + + @Test func cancellationRemovesAWaitingCapture() async { + let readiness = FlyoverPreviewReadiness() + readiness.expect([key(.first)]) + let waiter = Task { @MainActor in + await readiness.waitUntilReady() + } + await waitUntil { readiness.waiterCount == 1 } + + waiter.cancel() + await waiter.value + + #expect(readiness.waiterCount == 0) + #expect(readiness.isReadyForLatestExpectation == false) + } + + private func key( + _ screenID: TestScreen, + generation: Int = 0, + ) -> FlyoverPreviewReadiness.LoadKey { + FlyoverPreviewReadiness.LoadKey( + screenID: screenID, + variantID: FlyoverVariantID("default"), + generation: generation, + ) + } + + private func waitUntil(_ predicate: () -> Bool) async { + while predicate() == false { + await Task.yield() + } + } + + private enum TestScreen: Hashable { + case first + case second + } +} From e6b284ca51b0e3e145ab4d9f8a3d6538abd44997 Mon Sep 17 00:00:00 2001 From: Kyle Van Essen Date: Sun, 9 Aug 2026 21:33:50 -0700 Subject: [PATCH 3/3] fix(SnapshotKitTesting): preserve capture cancellation --- .../Sources/AssertSnapshots.swift | 2 + .../Tests/AssertSnapshotsTests.swift | 39 +++++++++++++++++++ .../Tests/PreMeasureHookTests.swift | 2 +- 3 files changed, 42 insertions(+), 1 deletion(-) create mode 100644 Shared/SnapshotKitTesting/Tests/AssertSnapshotsTests.swift diff --git a/Shared/SnapshotKitTesting/Sources/AssertSnapshots.swift b/Shared/SnapshotKitTesting/Sources/AssertSnapshots.swift index ca381458c..62f3eb735 100644 --- a/Shared/SnapshotKitTesting/Sources/AssertSnapshots.swift +++ b/Shared/SnapshotKitTesting/Sources/AssertSnapshots.swift @@ -161,6 +161,8 @@ public func assertSnapshots( settleTimeoutPolicy: settleTimeoutPolicy, timing: timing, ) + } catch is CancellationError { + return } catch { Issue.record(error) continue diff --git a/Shared/SnapshotKitTesting/Tests/AssertSnapshotsTests.swift b/Shared/SnapshotKitTesting/Tests/AssertSnapshotsTests.swift new file mode 100644 index 000000000..188726fa6 --- /dev/null +++ b/Shared/SnapshotKitTesting/Tests/AssertSnapshotsTests.swift @@ -0,0 +1,39 @@ +import SnapshotKit +import SnapshotKitTesting +import SwiftUI +import TestHostSupport +import Testing + +@MainActor +struct AssertSnapshotsTests { + @Test func cancellationDuringMeasurementReadinessEndsQuietly() async throws { + try waitFor { hostKeyWindow() != nil } + let probe = MeasurementHookCancellationProbe() + let assertion = Task { @MainActor in + await assertSnapshots( + of: Color.green.frame(width: 100, height: 100), + named: "cancelled-measurement-readiness", + configurations: [SnapshotConfiguration()], + measurementReadiness: .immediate, + onReadyToMeasure: { + probe.didStart = true + while Task.isCancelled == false { + await Task.yield() + } + }, + settle: .immediate, + ) + } + + while probe.didStart == false { + await Task.yield() + } + assertion.cancel() + await assertion.value + } +} + +@MainActor +private final class MeasurementHookCancellationProbe { + var didStart = false +} diff --git a/Shared/SnapshotKitTesting/Tests/PreMeasureHookTests.swift b/Shared/SnapshotKitTesting/Tests/PreMeasureHookTests.swift index 47a051731..5c23d87eb 100644 --- a/Shared/SnapshotKitTesting/Tests/PreMeasureHookTests.swift +++ b/Shared/SnapshotKitTesting/Tests/PreMeasureHookTests.swift @@ -22,7 +22,7 @@ struct PreMeasureHookTests { safeAreaInsets: .zero, measurementReadiness: .immediate, onReadyToMeasure: { - while model.hostedTaskRan == false { + while model.hostedTaskRan == false, Task.isCancelled == false { await Task.yield() } },